by Marcus Folkesson

[permalink] [raw]

Subject: Re: [PATCH v4 5/9] iio: adc: mcp3911: add support for buffers

Hi Jonathan,

On Sun, Jul 31, 2022 at 05:51:21PM +0100, Jonathan Cameron wrote:
> On Fri, 22 Jul 2022 15:07:22 +0200
> Marcus Folkesson <[email protected]> wrote:
>
> > Add support for buffers to make the driver fit for more usecases.
> >
> > Signed-off-by: Marcus Folkesson <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
>
> Assuming the Kconfig change from previous patch is pulled into this one...

Yep

>
> A few questions / comments inline.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/adc/mcp3911.c | 96 ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 89 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > index 00dadb1761dc..96c0a2a50c7c 100644
> > --- a/drivers/iio/adc/mcp3911.c
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -5,15 +5,22 @@
> > * Copyright (C) 2018 Marcus Folkesson <[email protected]>
> > * Copyright (C) 2018 Kent Gustavsson <[email protected]>
> > */
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > #include <linux/clk.h>
> > #include <linux/delay.h>
> > #include <linux/err.h>
> > #include <linux/iio/iio.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/trigger.h>
> > #include <linux/module.h>
> > #include <linux/mod_devicetable.h>
> > #include <linux/property.h>
> > #include <linux/regulator/consumer.h>
> > #include <linux/spi/spi.h>
>
> Line break here to separate the 'chunks' of includes.
>

OK

> > +#include <asm/unaligned.h>
> >
> > #define MCP3911_REG_CHANNEL0 0x00
> > #define MCP3911_REG_CHANNEL1 0x03
> > @@ -22,6 +29,7 @@
> > #define MCP3911_REG_GAIN 0x09
> >
> > #define MCP3911_REG_STATUSCOM 0x0a
> > +#define MCP3911_STATUSCOM_READ GENMASK(7, 6)
> > #define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4)
> > #define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3)
> > #define MCP3911_STATUSCOM_EN_OFFCAL BIT(2)
> > @@ -54,6 +62,13 @@ struct mcp3911 {
> > struct regulator *vref;
> > struct clk *clki;
> > u32 dev_addr;
> > + struct {
> > + u32 channels[MCP3911_NUM_CHANNELS];
> > + s64 ts __aligned(8);
> > + } scan;
> > +
> > + u8 tx_buf[MCP3911_NUM_CHANNELS * 3] __aligned(IIO_DMA_MINALIGN);
> > + u8 rx_buf[MCP3911_NUM_CHANNELS * 3];
> > };
> >
> > static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > @@ -196,16 +211,63 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
> > .type = IIO_VOLTAGE, \
> > .indexed = 1, \
> > .channel = idx, \
> > + .scan_index = idx, \
> > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > BIT(IIO_CHAN_INFO_OFFSET) | \
> > BIT(IIO_CHAN_INFO_SCALE), \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .realbits = 24, \
> > + .storagebits = 32, \
> > + .endianness = IIO_BE, \
> > + }, \
> > }
> >
> > static const struct iio_chan_spec mcp3911_channels[] = {
> > MCP3911_CHAN(0),
> > MCP3911_CHAN(1),
> > + IIO_CHAN_SOFT_TIMESTAMP(2),
> > };
> >
> > +static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct mcp3911 *adc = iio_priv(indio_dev);
> > + struct spi_transfer xfer = {
> > + .tx_buf = adc->tx_buf,
> > + .rx_buf = adc->rx_buf,
> > + .len = sizeof(adc->rx_buf),
> > + };
> > + int scan_index;
> > + int i = 0;
> > + int ret;
> > +
> > + mutex_lock(&adc->lock);
> > + adc->tx_buf[0] = MCP3911_REG_READ(MCP3911_CHANNEL(0), adc->dev_addr);
> > + ret = spi_sync_transfer(adc->spi, &xfer, 1);
> > + if (ret < 0) {
> > + dev_warn(&adc->spi->dev,
> > + "failed to get conversion data\n");
> > + goto out;
> > + }
> > +
> > + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> > + indio_dev->masklength) {
> > + const struct iio_chan_spec *scan_chan = &indio_dev->channels[scan_index];
> > +
> > + adc->scan.channels[i] = get_unaligned_be24(&adc->rx_buf[scan_chan->channel * 3]);
>
> This has me a little curious. It seems to be potentially reading from byte 0 which in the spi
> transfer is at the same time as the tx that tells the device what the command is. I'd expect
> it to be one byte later. Easiest way to do that being to have two transfers (though you could
> just add to the offset). I might be misremembering how the spi_transfer stuff works though.
> Been a while since I hacked anything SPI based...
>

Good catch.
I did not even notice that the resolution of my plot-script was wrong...

> > + i++;
> > + }
> > + iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
> > + iio_get_time_ns(indio_dev));
> > +out:
> > + mutex_unlock(&adc->lock);
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static const struct iio_info mcp3911_info = {
> > .read_raw = mcp3911_read_raw,
> > .write_raw = mcp3911_write_raw,
> > @@ -214,7 +276,7 @@ static const struct iio_info mcp3911_info = {
> > static int mcp3911_config(struct mcp3911 *adc)
> > {
> > struct device *dev = &adc->spi->dev;
> > - u32 configreg;
> > + u32 regval;
> > int ret;
> >
> > ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
> > @@ -233,29 +295,43 @@ static int mcp3911_config(struct mcp3911 *adc)
> > }
> > dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
> >
> > - ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2);
> > + ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &regval, 2);
>
> If I was being fussy I'd ask you to pull the refactoring in here out as a precusor
> patch to simplify reviewing the new stuff a little. However it's fairly minor
> so I'll let that go this time.

Ok, I got you wrong. Thanks.

Best regards
Marcus Folkesson

Attachments:

(No filename) (5.92 kB)
signature.asc (849.00 B)
Download all attachments

2022-08-01 07:50:57

On Fri, 22 Jul 2022 15:07:20 +0200
Marcus Folkesson <[email protected]> wrote:

> The ADC conversion is actually not rail-to-rail but with a factor 1.5.
> Make use of this factor when calculating actual voltage.
>
> Fixes: 3a89b289df5d ("iio: adc: add support for mcp3911")
> Signed-off-by: Marcus Folkesson <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
Patches 1-3 now applied to the fixes-togreg branch of iio.git
which is currently based on char-misc-linus which is at
Linus' tree just after he took the char-misc pull requests for
the merge window. I might rebase this on rc1 once available
or I might just use the base as is. Either way, it should be
fine in linux-next

Thanks,

Jonathan

> ---
> drivers/iio/adc/mcp3911.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index f8875076ae80..890af7dca62d 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -40,8 +40,8 @@
> #define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3)
> #define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6)
>
> -/* Internal voltage reference in uV */
> -#define MCP3911_INT_VREF_UV 1200000
> +/* Internal voltage reference in mV */
> +#define MCP3911_INT_VREF_MV 1200
>
> #define MCP3911_REG_READ(reg, id) ((((reg) << 1) | ((id) << 5) | (1 << 0)) & 0xff)
> #define MCP3911_REG_WRITE(reg, id) ((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
> @@ -139,11 +139,18 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
>
> *val = ret / 1000;
> } else {
> - *val = MCP3911_INT_VREF_UV;
> + *val = MCP3911_INT_VREF_MV;
> }
>
> - *val2 = 24;
> - ret = IIO_VAL_FRACTIONAL_LOG2;
> + /*
> + * For 24bit Conversion
> + * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
> + * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
> + */
> +
> + /* val2 = (2^23 * 1.5) */
> + *val2 = 12582912;
> + ret = IIO_VAL_FRACTIONAL;
> break;
> }
>