On Sun, 22 Jul 2018 21:00:51 +0200
Marcus Folkesson <marcus.folkesson@xxxxxxxxx> wrote:
> Hi Jonathan,
>
> Thanks, all good catches.
>
> On Sun, Jul 22, 2018 at 09:08:38AM +0100, Jonathan Cameron wrote:
> > On Sat, 21 Jul 2018 23:19:48 +0200 (CEST)
> > Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> wrote:
> >
> > > Hello,
> > >
> > > > MCP3911 is a dual channel Analog Front End (AFE) containing two
> > > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
> > >
> > > some comments below...
> >
> > +CC Mark for the unusual SPI addressing stuff. I'm mostly interested in what
> > precedent there is for bindings etc.
> >
>
> Yep, I'm not entirely sure that the SPI framework can handle multiple
> clients on the same CS.
> The reason why we created device-addr is that the chip supports that and
> may have hardcoded chip address from factory.
> The chip address is also part of the protocol so we have to specify it.
It will certainly be 'interesting' if we ever try to support it.
>
...
...
> > > > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len)
> > > > +{
> > > > + dev_dbg(&adc->spi->dev, "Writing 0x%x to register 0x%x\n", val, reg);
> > > > +
> > > > + cpu_to_be32s(&val);
> > > > + val >>= (3-len)*8;
> > Hmm. It might be worth considering regmap here to handle all this stuff for
> > you rather than re rolling the same stuff.
> >
>
> We were looking at regmap, but it does not seems to support registers of
> different size.
> This chip has register values of 8, 16 and 24 bits.
Fair enough. I thought we were really looking at autoincrement, of
the address for a fixed sized register - which is fine in regmap.
Those wider registers are described as having ADDR and ADDR+1 etc.
>
> > > > + val |= REG_WRITE(reg, adc->dev_addr);
> > > > +
> > > > + return spi_write(adc->spi, &val, len+1);
> > > > +}
> > > > +
> > > > +
> > > > +static int mcp3911_read_raw(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *channel, int *val,
> > > > + int *val2, long mask)
> > > > +{
> > > > + struct mcp3911 *adc = iio_priv(indio_dev);
> > > > + int ret = -EINVAL;
> > > > +
> > > > + mutex_lock(&adc->lock);
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_RAW:
> > > > + ret = mcp3911_read(adc,
> > > > + MCP3911_CHANNEL(channel->channel), val, 3);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = IIO_VAL_INT;
> > > > + break;
> > > > +
> > > > + case IIO_CHAN_INFO_OFFSET:
> > > > + ret = mcp3911_read(adc,
> > > > + MCP3911_OFFCAL(channel->channel), val, 3);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = IIO_VAL_INT;
> > > > + break;
> > > > +
> > > > + case IIO_CHAN_INFO_HARDWAREGAIN:
> > > > + ret = mcp3911_get_hwgain(adc, channel->channel, val);
> >
> > I'm not convinced it's useful to expose this as it right control for this
> > is scale.
> >
>
> Hmm, all other drivers that are using HARDWAREGAIN (ina2xx-adc, stx104 +
> a few more that are not ADC:s) are, what I can tell, exposing it.
>
> But maybe it should'nt.
Yup, as ever things are a bit messy. However, best to not expose it unless
there is a very good reason.
> > > > +
> > > > + break;
> > > > +
> > > > + case IIO_CHAN_INFO_HARDWAREGAIN:
> >
> > Default choice (by precedent) is to control variable gain
> > front ends via the scale parameter. Hardware gain
> > is not meant to have any 'visible' impact on the output
> > value - most commonly used when the thing we are measuring
> > is not amplitude of anything.
>
> Hmm, Ok. I'm not sure I understand how hardware gain is supposed to work
> then.
For this use case it isn't.
> Maybe I just remove it.
That's the best plan.
...
> > > > +
> > > > +static int mcp3911_config_of(struct mcp3911 *adc)
> > > > +{
> > > > + u32 configreg;
> > > > + u32 statuscomreg;
> > > > + int ret;
> > > > +
> > > > + of_property_read_u32(adc->np, "device-addr", &adc->dev_addr);
> > This is 'interesting' - I wonder if there is any precedence for it.
> >
>
> I guess we still need it since the device may have a hardcoded (from
> factory) address that we need to deal with.
Fair enough. Still good to hear from Mark on whether there are other similar
devices so the binding can be consistent.
> > > > +
> > > > + of_property_read_u32(adc->np, "ch0-width", &adc->width[0]);
> > > > + switch (adc->width[0]) {
> > > > + case 24:
> > > > + statuscomreg &= ~MCP3911_STATUSCOM_CH0_24WIDTH;
> > > > + dev_dbg(&adc->spi->dev, "set channel 0 into 24bit mode\n");
> > > > + break;
> > > > + case 16:
> > > > + statuscomreg |= MCP3911_STATUSCOM_CH0_24WIDTH;
> > > > + dev_dbg(&adc->spi->dev, "set channel 0 into 16bit mode\n");
> > > > + break;
> > > > + default:
> > > > + adc->width[0] = 24;
> > > > + dev_info(&adc->spi->dev, "invalid width for channel 0. Use 24bit.\n");
> > > > + break;
> > > > + }
> > This feels like something that isn't really a dt choice, as it's not down to
> > wiring but rather down to precision desired.
>
> You are right. I will remove them and stick to 24bit width.
>
That's the easiest option. If anyone 'really' wants 16bit we'll figure it out
later.
..
Thanks
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
