by Marcus Folkesson

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] iio: adc: mcp3911: add support for buffers

Hi Jonathan,

On Sat, Jun 25, 2022 at 12:45:37PM +0100, Jonathan Cameron wrote:
> On Sat, 25 Jun 2022 12:38:46 +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]>
> Hi Marcus,
>
> Good to see this feature. A few comments inline, mostly around
> optimising the data flow / device accesses.
>
> Thanks,
>
> Jonathan
>
> > ---
> >
> > Notes:
> > v2:
> > - No changes
> >
> > drivers/iio/adc/mcp3911.c | 58 ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > index 25a235cce56c..2a4bf374f140 100644
> > --- a/drivers/iio/adc/mcp3911.c
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -9,6 +9,10 @@
> > #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>
> > @@ -54,6 +58,10 @@ struct mcp3911 {
> > struct regulator *vref;
> > struct clk *clki;
> > u32 dev_addr;
> > + struct {
> > + u32 channels[2];
> > + s64 ts __aligned(8);
> > + } scan;
> > };
> >
> > static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
> > @@ -187,16 +195,58 @@ 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, \
> > + }, \
> > }
> >
> > 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);
> > + int scan_index;
> > + int i = 0;
> > + u32 val;
> > +
> > + mutex_lock(&adc->lock);
> > + 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];
> > + int ret = mcp3911_read(adc,
> > + MCP3911_CHANNEL(scan_chan->channel), &val, 3);
>
> I was a bit surprised not to see some overlap of address setting and
> read out here if both channels are enabled, so opened up the data sheet.
>
> Can we take advantage of "Continuous communication looping on address set"

Sure, I will make it use continuous reads when both channels are enabled,
thanks.

> (6.7 on the datasheet). Also for buffered capture we'd normally make
> things like shifting and endian conversion a userspace problem. Can we
> not do that here for some reason? You'd need to take care to ensure

The endian conversion&shifting was actually the reason for why I did not
make use of continoues reads from the beginning.

> any buffers that might be used directly for DMA obey DMA safety rules
> (currently avoided by using spi_write_then_read), but it would be
> nice to do less data manipulation int his path if we can.

I will change to spi_sync() and skip the data manipulation.

>
>
>
> > +
> > + if (ret < 0) {
> > + dev_warn(&adc->spi->dev,
> > + "failed to get conversion data\n");
> > + goto out;
> > + }
> > +
> > + adc->scan.channels[i] = val;
> > + 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,
> > @@ -303,7 +353,7 @@ static int mcp3911_probe(struct spi_device *spi)
> > goto clk_disable;
> >
> > indio_dev->name = spi_get_device_id(spi)->name;
> > - indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
>
> The core sets INDIO_BUFFER_TRIGGERED as part of devm_iio_triggered_buffer_setup()
> so you need to set DIRECT_MODE here (that one isn't visible to the core)

Ok, thank you. I sent patches that fixes this in two other ADC-drivers
as well to avoid more people following the same thing.

>
> > indio_dev->info = &mcp3911_info;
> > spi_set_drvdata(spi, indio_dev);
> >
> > @@ -312,6 +362,12 @@ static int mcp3911_probe(struct spi_device *spi)
> >
> > mutex_init(&adc->lock);
> >
> > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> Can't use devm here for the same reason it was inappropriate in patch 2.
>
> I'd suggest a precursor patch(es) to move the driver fully over to
> devm_ managed such that you don't need a remove function and the ordering
> is maintained.

Yep, I will keep this and fix patch 2 instead.

> > + NULL,
> > + mcp3911_trigger_handler, NULL);
> > + if (ret)
> > + goto clk_disable;
> > +
> > ret = devm_iio_device_register(&adc->spi->dev, indio_dev);
> > if (ret)
> > goto clk_disable;
>

/Marcus

Attachments:

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

2022-07-03 19:26:35

by Marcus Folkesson

[permalink] [raw]

Subject: Re: [PATCH v2 04/10] iio: adc: mcp3911: add support for interrupts

On Sat, Jun 25, 2022 at 01:06:37PM +0100, Jonathan Cameron wrote:
>
> ...
>
> > > static int mcp3911_probe(struct spi_device *spi)
> > > {
> > > struct iio_dev *indio_dev;
> > > @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi)
> > > if (ret)
> > > goto clk_disable;
> > >
> > > + if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
> > > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
> > > + 0, 2);
> > > + else
> > > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
> > > + MCP3911_STATUSCOM_DRHIZ, 2);
> > > + if (ret < 0)
> > > + goto clk_disable;
> > > +
> > > indio_dev->name = spi_get_device_id(spi)->name;
> > > indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> > > indio_dev->info = &mcp3911_info;
> > > @@ -362,6 +401,32 @@ static int mcp3911_probe(struct spi_device *spi)
> > >
> > > mutex_init(&adc->lock);
> > >
> > > + if (spi->irq > 0) {
> > > + adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> > > + indio_dev->name,
> > > + iio_device_id(indio_dev));
> > > + if (!adc->trig)
> > > + goto clk_disable;
> > You definitely want to use devm managed cleanup for these.
> >
> > There is a patch set that adds these as standard functions, but I haven't
> > yet seen it being picked up for this cycle (reviews have been slow coming).
> >
> > https://lore.kernel.org/all/[email protected]/
>
> Ah, elsewhere in my unread email was a thread that says it's in clk-next so
> will be in the next merge window. I haven't confirmed, but looks like Stephen
> put up an immutable branch so I could pull it into the IIO togreg branch if you
> want to transition directly to that new code. @Stephen, would you be fine
> with me pulling your clk-devm-enable branch into IIO (with the fix which
> got posted earlier in the week presumably also going on that branch when
> you push out?)

Please do, thanks

>
> Thanks,
>
> Jonathan
>
>
>
> >
> > In meantime role your own with devm_add_action_or_reset()

Best regards,
Marcus Folkesson

Attachments:

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

2022-07-07 17:49:15

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] iio: adc: mcp3911: add support for buffers

> > > static const struct iio_info mcp3911_info = {
> > > .read_raw = mcp3911_read_raw,
> > > .write_raw = mcp3911_write_raw,
> > > @@ -303,7 +353,7 @@ static int mcp3911_probe(struct spi_device *spi)
> > > goto clk_disable;
> > >
> > > indio_dev->name = spi_get_device_id(spi)->name;
> > > - indio_dev->modes = INDIO_DIRECT_MODE;
> > > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> >
> > The core sets INDIO_BUFFER_TRIGGERED as part of devm_iio_triggered_buffer_setup()
> > so you need to set DIRECT_MODE here (that one isn't visible to the core)
>
> Ok, thank you. I sent patches that fixes this in two other ADC-drivers
> as well to avoid more people following the same thing.

Thanks. Much appreciated!