| Message ID | 20210818111139.330636-4-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Bring software triggers support to MAX1027-like ADCs | expand |
> -----Original Message----- > From: Miquel Raynal <miquel.raynal@bootlin.com> > Sent: Wednesday, August 18, 2021 1:11 PM > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal > <miquel.raynal@bootlin.com> > Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested > samples > > [External] > > When a triggered scan occurs, the identity of the desired channels is > known in indio_dev->active_scan_mask. Instead of reading and > pushing to > the IIO buffers all channels each time, scan the minimum amount of > channels (0 to maximum requested chan, to be exact) and only > provide the > samples requested by the user. > > For example, if the user wants channels 1, 4 and 5, all channels from > 0 to 5 will be scanned but only the desired channels will be pushed to > the IIO buffers. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > index b753658bb41e..8ab660f596b5 100644 > --- a/drivers/iio/adc/max1027.c > +++ b/drivers/iio/adc/max1027.c > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct > iio_trigger *trig, bool state) > struct max1027_state *st = iio_priv(indio_dev); > int ret; > > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev- > >masklength)) > + return -EINVAL; > + I'm not sure this can actually happen. If you try to enable the buffer with no scan element, it should give you an error before you reach this point... > if (state) { > /* Start acquisition on cnvst */ > st->reg = MAX1027_SETUP_REG | > MAX1027_CKS_MODE0 | > @@ -368,9 +371,12 @@ static int max1027_set_trigger_state(struct > iio_trigger *trig, bool state) > if (ret < 0) > return ret; > > - /* Scan from 0 to max */ > - st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) | > - MAX1027_SCAN_N_M | MAX1027_TEMP; > + /* > + * Scan from 0 to the highest requested channel. The > temperature > + * could be avoided but it simplifies a bit the logic. > + */ > + st->reg = MAX1027_CONV_REG | > MAX1027_SCAN_0_N | MAX1027_TEMP; > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > >active_scan_mask) - 2); > ret = spi_write(st->spi, &st->reg, 1); > if (ret < 0) > return ret; > @@ -391,11 +397,22 @@ static irqreturn_t > max1027_trigger_handler(int irq, void *private) > struct iio_poll_func *pf = private; > struct iio_dev *indio_dev = pf->indio_dev; > struct max1027_state *st = iio_priv(indio_dev); > + unsigned int scanned_chans = fls(*indio_dev- > >active_scan_mask); > + u16 *buf = st->buffer; I think sparse will complain here. buffer is a __be16 restricted type so you should not mix those... > + unsigned int bit; > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > private); > > /* fill buffer with all channel */ > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2); > + spi_read(st->spi, st->buffer, scanned_chans * 2); > + > + /* Only keep the channels selected by the user */ > + for_each_set_bit(bit, indio_dev->active_scan_mask, > + indio_dev->masklength) { > + if (buf[0] != st->buffer[bit]) > + buf[0] = st->buffer[bit]; Since we are here, when looking into the driver, I realized that st->buffer is not DMA safe. In IIO, we kind of want to enforce that all buffers that are passed to spi/i2c buses are safe... Maybe this is something you can include in your series. - Nuno Sá > + buf++; > + } > > iio_push_to_buffers(indio_dev, st->buffer); > > -- > 2.27.0
On Wed, 18 Aug 2021 13:11:26 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > When a triggered scan occurs, the identity of the desired channels is > known in indio_dev->active_scan_mask. Instead of reading and pushing to > the IIO buffers all channels each time, scan the minimum amount of > channels (0 to maximum requested chan, to be exact) and only provide the > samples requested by the user. > > For example, if the user wants channels 1, 4 and 5, all channels from > 0 to 5 will be scanned That's a reasonably optimisation > but only the desired channels will be pushed to > the IIO buffers. Don't do this last bit. The core handles demuxing the channels via active_scan_masks. As a general rule it does a more efficient job of this than a hand coded version in a driver (precached copy rules etc). The core has to have the logic anyway to support multiple consumers (e.g. in kernel consumer and userspace) so we reuse it for these cases. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > index b753658bb41e..8ab660f596b5 100644 > --- a/drivers/iio/adc/max1027.c > +++ b/drivers/iio/adc/max1027.c > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state) > struct max1027_state *st = iio_priv(indio_dev); > int ret; > > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) > + return -EINVAL; > + > if (state) { > /* Start acquisition on cnvst */ > st->reg = MAX1027_SETUP_REG | MAX1027_CKS_MODE0 | > @@ -368,9 +371,12 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state) > if (ret < 0) > return ret; > > - /* Scan from 0 to max */ > - st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) | > - MAX1027_SCAN_N_M | MAX1027_TEMP; > + /* > + * Scan from 0 to the highest requested channel. The temperature > + * could be avoided but it simplifies a bit the logic. > + */ > + st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N | MAX1027_TEMP; > + st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2); This should be combined with appropriate additions to available_scan_masks arrays so the IIO core can handle choosing the right one to match enabled channels. > ret = spi_write(st->spi, &st->reg, 1); > if (ret < 0) > return ret; > @@ -391,11 +397,22 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private) > struct iio_poll_func *pf = private; > struct iio_dev *indio_dev = pf->indio_dev; > struct max1027_state *st = iio_priv(indio_dev); > + unsigned int scanned_chans = fls(*indio_dev->active_scan_mask); > + u16 *buf = st->buffer; > + unsigned int bit; > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); > > /* fill buffer with all channel */ > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2); > + spi_read(st->spi, st->buffer, scanned_chans * 2); > + > + /* Only keep the channels selected by the user */ > + for_each_set_bit(bit, indio_dev->active_scan_mask, > + indio_dev->masklength) { > + if (buf[0] != st->buffer[bit]) > + buf[0] = st->buffer[bit]; > + buf++; > + } > > iio_push_to_buffers(indio_dev, st->buffer); >
On Fri, 20 Aug 2021 07:10:48 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > -----Original Message----- > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > Sent: Wednesday, August 18, 2021 1:11 PM > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > <lars@metafoo.de> > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal > > <miquel.raynal@bootlin.com> > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested > > samples > > > > [External] > > > > When a triggered scan occurs, the identity of the desired channels is > > known in indio_dev->active_scan_mask. Instead of reading and > > pushing to > > the IIO buffers all channels each time, scan the minimum amount of > > channels (0 to maximum requested chan, to be exact) and only > > provide the > > samples requested by the user. > > > > For example, if the user wants channels 1, 4 and 5, all channels from > > 0 to 5 will be scanned but only the desired channels will be pushed to > > the IIO buffers. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > index b753658bb41e..8ab660f596b5 100644 > > --- a/drivers/iio/adc/max1027.c > > +++ b/drivers/iio/adc/max1027.c > > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct > > iio_trigger *trig, bool state) > > struct max1027_state *st = iio_priv(indio_dev); > > int ret; > > > > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev- > > >masklength)) > > + return -EINVAL; > > + > > I'm not sure this can actually happen. If you try to enable the buffer > with no scan element, it should give you an error before you reach > this point... > > > if (state) { > > /* Start acquisition on cnvst */ > > st->reg = MAX1027_SETUP_REG | > > MAX1027_CKS_MODE0 | > > @@ -368,9 +371,12 @@ static int max1027_set_trigger_state(struct > > iio_trigger *trig, bool state) > > if (ret < 0) > > return ret; > > > > - /* Scan from 0 to max */ > > - st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) | > > - MAX1027_SCAN_N_M | MAX1027_TEMP; > > + /* > > + * Scan from 0 to the highest requested channel. The > > temperature > > + * could be avoided but it simplifies a bit the logic. > > + */ > > + st->reg = MAX1027_CONV_REG | > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > >active_scan_mask) - 2); > > ret = spi_write(st->spi, &st->reg, 1); > > if (ret < 0) > > return ret; > > @@ -391,11 +397,22 @@ static irqreturn_t > > max1027_trigger_handler(int irq, void *private) > > struct iio_poll_func *pf = private; > > struct iio_dev *indio_dev = pf->indio_dev; > > struct max1027_state *st = iio_priv(indio_dev); > > + unsigned int scanned_chans = fls(*indio_dev- > > >active_scan_mask); > > + u16 *buf = st->buffer; > > I think sparse will complain here. buffer is a __be16 restricted > type so you should not mix those... > > + unsigned int bit; > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > private);in/20210818_miquel_raynal_bring_software_triggers_support_to_max1027_like_adcs.mbx > > > > /* fill buffer with all channel */ > > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2); > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > + > > + /* Only keep the channels selected by the user */ > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > + indio_dev->masklength) { > > + if (buf[0] != st->buffer[bit]) > > + buf[0] = st->buffer[bit]; > > Since we are here, when looking into the driver, I realized > that st->buffer is not DMA safe. In IIO, we kind of want to enforce > that all buffers that are passed to spi/i2c buses are safe... Maybe > this is something you can include in your series. Why is it not? st->buffer is result of a devm_kmalloc_array() call and that should provide a DMA safe buffer as I understand it. > > - Nuno Sá > > > + buf++; > > + } > > > > iio_push_to_buffers(indio_dev, st->buffer); > > > > -- > > 2.27.0 >
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Monday, August 30, 2021 12:08 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter Clausen > <lars@metafoo.de>; Thomas Petazzoni > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > samples > > [External] > > On Fri, 20 Aug 2021 07:10:48 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > -----Original Message----- > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > <lars@metafoo.de> > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal > > > <miquel.raynal@bootlin.com> > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested > > > samples > > > > > > [External] > > > > > > When a triggered scan occurs, the identity of the desired channels > is > > > known in indio_dev->active_scan_mask. Instead of reading and > > > pushing to > > > the IIO buffers all channels each time, scan the minimum amount > of > > > channels (0 to maximum requested chan, to be exact) and only > > > provide the > > > samples requested by the user. > > > > > > For example, if the user wants channels 1, 4 and 5, all channels > from > > > 0 to 5 will be scanned but only the desired channels will be pushed > to > > > the IIO buffers. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > > index b753658bb41e..8ab660f596b5 100644 > > > --- a/drivers/iio/adc/max1027.c > > > +++ b/drivers/iio/adc/max1027.c > > > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct > > > iio_trigger *trig, bool state) > > > struct max1027_state *st = iio_priv(indio_dev); > > > int ret; > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev- > > > >masklength)) > > > + return -EINVAL; > > > + > > > > I'm not sure this can actually happen. If you try to enable the buffer > > with no scan element, it should give you an error before you reach > > this point... > > > > > if (state) { > > > /* Start acquisition on cnvst */ > > > st->reg = MAX1027_SETUP_REG | > > > MAX1027_CKS_MODE0 | > > > @@ -368,9 +371,12 @@ static int max1027_set_trigger_state(struct > > > iio_trigger *trig, bool state) > > > if (ret < 0) > > > return ret; > > > > > > - /* Scan from 0 to max */ > > > - st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) | > > > - MAX1027_SCAN_N_M | MAX1027_TEMP; > > > + /* > > > + * Scan from 0 to the highest requested channel. The > > > temperature > > > + * could be avoided but it simplifies a bit the logic. > > > + */ > > > + st->reg = MAX1027_CONV_REG | > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > >active_scan_mask) - 2); > > > ret = spi_write(st->spi, &st->reg, 1); > > > if (ret < 0) > > > return ret; > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > max1027_trigger_handler(int irq, void *private) > > > struct iio_poll_func *pf = private; > > > struct iio_dev *indio_dev = pf->indio_dev; > > > struct max1027_state *st = iio_priv(indio_dev); > > > + unsigned int scanned_chans = fls(*indio_dev- > > > >active_scan_mask); > > > + u16 *buf = st->buffer; > > > > I think sparse will complain here. buffer is a __be16 restricted > > type so you should not mix those... > > > + unsigned int bit; > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > _to_max1027_like_adcs.mbx > > > > > > /* fill buffer with all channel */ > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2); > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > + > > > + /* Only keep the channels selected by the user */ > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > + indio_dev->masklength) { > > > + if (buf[0] != st->buffer[bit]) > > > + buf[0] = st->buffer[bit]; > > > > Since we are here, when looking into the driver, I realized > > that st->buffer is not DMA safe. In IIO, we kind of want to enforce > > that all buffers that are passed to spi/i2c buses are safe... Maybe > > this is something you can include in your series. > > Why is it not? st->buffer is result of a devm_kmalloc_array() call and > that should provide a DMA safe buffer as I understand it. > That's a good question. I'm not sure how I came to that conclusion which is clearly wrong. Though I think the buffer might share the line with the mutex... - Nuno Sá
On Mon, 30 Aug 2021 10:49:50 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Monday, August 30, 2021 12:08 PM > > To: Sa, Nuno <Nuno.Sa@analog.com> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter Clausen > > <lars@metafoo.de>; Thomas Petazzoni > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > > samples > > > > [External] > > > > On Fri, 20 Aug 2021 07:10:48 +0000 > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > -----Original Message----- > > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > > <lars@metafoo.de> > > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal > > > > <miquel.raynal@bootlin.com> > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested > > > > samples > > > > > > > > [External] > > > > > > > > When a triggered scan occurs, the identity of the desired channels > > is > > > > known in indio_dev->active_scan_mask. Instead of reading and > > > > pushing to > > > > the IIO buffers all channels each time, scan the minimum amount > > of > > > > channels (0 to maximum requested chan, to be exact) and only > > > > provide the > > > > samples requested by the user. > > > > > > > > For example, if the user wants channels 1, 4 and 5, all channels > > from > > > > 0 to 5 will be scanned but only the desired channels will be pushed > > to > > > > the IIO buffers. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > > > > index b753658bb41e..8ab660f596b5 100644 > > > > --- a/drivers/iio/adc/max1027.c > > > > +++ b/drivers/iio/adc/max1027.c > > > > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct > > > > iio_trigger *trig, bool state) > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > int ret; > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev- > > > > >masklength)) > > > > + return -EINVAL; > > > > + > > > > > > I'm not sure this can actually happen. If you try to enable the buffer > > > with no scan element, it should give you an error before you reach > > > this point... > > > > > > > if (state) { > > > > /* Start acquisition on cnvst */ > > > > st->reg = MAX1027_SETUP_REG | > > > > MAX1027_CKS_MODE0 | > > > > @@ -368,9 +371,12 @@ static int max1027_set_trigger_state(struct > > > > iio_trigger *trig, bool state) > > > > if (ret < 0) > > > > return ret; > > > > > > > > - /* Scan from 0 to max */ > > > > - st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) | > > > > - MAX1027_SCAN_N_M | MAX1027_TEMP; > > > > + /* > > > > + * Scan from 0 to the highest requested channel. The > > > > temperature > > > > + * could be avoided but it simplifies a bit the logic. > > > > + */ > > > > + st->reg = MAX1027_CONV_REG | > > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > > >active_scan_mask) - 2); > > > > ret = spi_write(st->spi, &st->reg, 1); > > > > if (ret < 0) > > > > return ret; > > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > > max1027_trigger_handler(int irq, void *private) > > > > struct iio_poll_func *pf = private; > > > > struct iio_dev *indio_dev = pf->indio_dev; > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > + unsigned int scanned_chans = fls(*indio_dev- > > > > >active_scan_mask); > > > > + u16 *buf = st->buffer; > > > > > > I think sparse will complain here. buffer is a __be16 restricted > > > type so you should not mix those... > > > > + unsigned int bit; > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, > > > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > > _to_max1027_like_adcs.mbx > > > > > > > > /* fill buffer with all channel */ > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2); > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > > + > > > > + /* Only keep the channels selected by the user */ > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > > + indio_dev->masklength) { > > > > + if (buf[0] != st->buffer[bit]) > > > > + buf[0] = st->buffer[bit]; > > > > > > Since we are here, when looking into the driver, I realized > > > that st->buffer is not DMA safe. In IIO, we kind of want to enforce > > > that all buffers that are passed to spi/i2c buses are safe... Maybe > > > this is something you can include in your series. > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() call and > > that should provide a DMA safe buffer as I understand it. > > > > That's a good question. I'm not sure how I came to that conclusion which > is clearly wrong. Though I think the buffer might share the line with the > mutex... Pointer shares a line. The buffer it points to doesn't as allocated by separate heap allocation. J > > - Nuno Sá >
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Monday, August 30, 2021 4:30 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter Clausen > <lars@metafoo.de>; Thomas Petazzoni > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > samples > > [External] > > On Mon, 30 Aug 2021 10:49:50 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > -----Original Message----- > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Monday, August 30, 2021 12:08 PM > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > Clausen > > > <lars@metafoo.de>; Thomas Petazzoni > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > requested > > > samples > > > > > > [External] > > > > > > On Fri, 20 Aug 2021 07:10:48 +0000 > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > -----Original Message----- > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > > > <lars@metafoo.de> > > > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel > Raynal > > > > > <miquel.raynal@bootlin.com> > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the > requested > > > > > samples > > > > > > > > > > [External] > > > > > > > > > > When a triggered scan occurs, the identity of the desired > channels > > > is > > > > > known in indio_dev->active_scan_mask. Instead of reading and > > > > > pushing to > > > > > the IIO buffers all channels each time, scan the minimum > amount > > > of > > > > > channels (0 to maximum requested chan, to be exact) and only > > > > > provide the > > > > > samples requested by the user. > > > > > > > > > > For example, if the user wants channels 1, 4 and 5, all channels > > > from > > > > > 0 to 5 will be scanned but only the desired channels will be > pushed > > > to > > > > > the IIO buffers. > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > --- > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/adc/max1027.c > b/drivers/iio/adc/max1027.c > > > > > index b753658bb41e..8ab660f596b5 100644 > > > > > --- a/drivers/iio/adc/max1027.c > > > > > +++ b/drivers/iio/adc/max1027.c > > > > > @@ -360,6 +360,9 @@ static int > max1027_set_trigger_state(struct > > > > > iio_trigger *trig, bool state) > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > int ret; > > > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, > indio_dev- > > > > > >masklength)) > > > > > + return -EINVAL; > > > > > + > > > > > > > > I'm not sure this can actually happen. If you try to enable the > buffer > > > > with no scan element, it should give you an error before you > reach > > > > this point... > > > > > > > > > if (state) { > > > > > /* Start acquisition on cnvst */ > > > > > st->reg = MAX1027_SETUP_REG | > > > > > MAX1027_CKS_MODE0 | > > > > > @@ -368,9 +371,12 @@ static int > max1027_set_trigger_state(struct > > > > > iio_trigger *trig, bool state) > > > > > if (ret < 0) > > > > > return ret; > > > > > > > > > > - /* Scan from 0 to max */ > > > > > - st->reg = MAX1027_CONV_REG | > MAX1027_CHAN(0) | > > > > > - MAX1027_SCAN_N_M | > MAX1027_TEMP; > > > > > + /* > > > > > + * Scan from 0 to the highest requested > channel. The > > > > > temperature > > > > > + * could be avoided but it simplifies a bit the > logic. > > > > > + */ > > > > > + st->reg = MAX1027_CONV_REG | > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > > > >active_scan_mask) - 2); > > > > > ret = spi_write(st->spi, &st->reg, 1); > > > > > if (ret < 0) > > > > > return ret; > > > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > > > max1027_trigger_handler(int irq, void *private) > > > > > struct iio_poll_func *pf = private; > > > > > struct iio_dev *indio_dev = pf->indio_dev; > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > + unsigned int scanned_chans = fls(*indio_dev- > > > > > >active_scan_mask); > > > > > + u16 *buf = st->buffer; > > > > > > > > I think sparse will complain here. buffer is a __be16 restricted > > > > type so you should not mix those... > > > > > + unsigned int bit; > > > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, > irq, > > > > > > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > > > _to_max1027_like_adcs.mbx > > > > > > > > > > /* fill buffer with all channel */ > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * > 2); > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > > > + > > > > > + /* Only keep the channels selected by the user */ > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > > > + indio_dev->masklength) { > > > > > + if (buf[0] != st->buffer[bit]) > > > > > + buf[0] = st->buffer[bit]; > > > > > > > > Since we are here, when looking into the driver, I realized > > > > that st->buffer is not DMA safe. In IIO, we kind of want to > enforce > > > > that all buffers that are passed to spi/i2c buses are safe... Maybe > > > > this is something you can include in your series. > > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() call > and > > > that should provide a DMA safe buffer as I understand it. > > > > > > > That's a good question. I'm not sure how I came to that conclusion > which > > is clearly wrong. Though I think the buffer might share the line with > the > > mutex... > Pointer shares a line. The buffer it points to doesn't as allocated > by separate heap allocation. > Ups, sure :facepalm: - Nuno Sá
Hello, "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Mon, 30 Aug 2021 15:02:26 +0000: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Monday, August 30, 2021 4:30 PM > > To: Sa, Nuno <Nuno.Sa@analog.com> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter Clausen > > <lars@metafoo.de>; Thomas Petazzoni > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > > samples > > > > [External] > > > > On Mon, 30 Aug 2021 10:49:50 +0000 > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > -----Original Message----- > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > Sent: Monday, August 30, 2021 12:08 PM > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > > Clausen > > > > <lars@metafoo.de>; Thomas Petazzoni > > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > > linux- > > > > kernel@vger.kernel.org > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > > requested > > > > samples > > > > > > > > [External] > > > > > > > > On Fri, 20 Aug 2021 07:10:48 +0000 > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > > > > <lars@metafoo.de> > > > > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > > > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel > > Raynal > > > > > > <miquel.raynal@bootlin.com> > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the > > requested > > > > > > samples > > > > > > > > > > > > [External] > > > > > > > > > > > > When a triggered scan occurs, the identity of the desired > > channels > > > > is > > > > > > known in indio_dev->active_scan_mask. Instead of reading and > > > > > > pushing to > > > > > > the IIO buffers all channels each time, scan the minimum > > amount > > > > of > > > > > > channels (0 to maximum requested chan, to be exact) and only > > > > > > provide the > > > > > > samples requested by the user. > > > > > > > > > > > > For example, if the user wants channels 1, 4 and 5, all channels > > > > from > > > > > > 0 to 5 will be scanned but only the desired channels will be > > pushed > > > > to > > > > > > the IIO buffers. > > > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > --- > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/iio/adc/max1027.c > > b/drivers/iio/adc/max1027.c > > > > > > index b753658bb41e..8ab660f596b5 100644 > > > > > > --- a/drivers/iio/adc/max1027.c > > > > > > +++ b/drivers/iio/adc/max1027.c > > > > > > @@ -360,6 +360,9 @@ static int > > max1027_set_trigger_state(struct > > > > > > iio_trigger *trig, bool state) > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > int ret; > > > > > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, > > indio_dev- > > > > > > >masklength)) > > > > > > + return -EINVAL; > > > > > > + > > > > > > > > > > I'm not sure this can actually happen. If you try to enable the > > buffer > > > > > with no scan element, it should give you an error before you > > reach > > > > > this point... > > > > > > > > > > > if (state) { > > > > > > /* Start acquisition on cnvst */ > > > > > > st->reg = MAX1027_SETUP_REG | > > > > > > MAX1027_CKS_MODE0 | > > > > > > @@ -368,9 +371,12 @@ static int > > max1027_set_trigger_state(struct > > > > > > iio_trigger *trig, bool state) > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > > > > > > > - /* Scan from 0 to max */ > > > > > > - st->reg = MAX1027_CONV_REG | > > MAX1027_CHAN(0) | > > > > > > - MAX1027_SCAN_N_M | > > MAX1027_TEMP; > > > > > > + /* > > > > > > + * Scan from 0 to the highest requested > > channel. The > > > > > > temperature > > > > > > + * could be avoided but it simplifies a bit the > > logic. > > > > > > + */ > > > > > > + st->reg = MAX1027_CONV_REG | > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > > > > >active_scan_mask) - 2); > > > > > > ret = spi_write(st->spi, &st->reg, 1); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > > > > max1027_trigger_handler(int irq, void *private) > > > > > > struct iio_poll_func *pf = private; > > > > > > struct iio_dev *indio_dev = pf->indio_dev; > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > + unsigned int scanned_chans = fls(*indio_dev- > > > > > > >active_scan_mask); > > > > > > + u16 *buf = st->buffer; > > > > > > > > > > I think sparse will complain here. buffer is a __be16 restricted > > > > > type so you should not mix those... > > > > > > + unsigned int bit; > > > > > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, > > irq, > > > > > > > > > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > > > > _to_max1027_like_adcs.mbx > > > > > > > > > > > > /* fill buffer with all channel */ > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * > > 2); > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > > > > + > > > > > > + /* Only keep the channels selected by the user */ > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > > > > + indio_dev->masklength) { > > > > > > + if (buf[0] != st->buffer[bit]) > > > > > > + buf[0] = st->buffer[bit]; > > > > > > > > > > Since we are here, when looking into the driver, I realized > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to > > enforce > > > > > that all buffers that are passed to spi/i2c buses are safe... Maybe > > > > > this is something you can include in your series. > > > > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() call > > and > > > > that should provide a DMA safe buffer as I understand it. > > > > > > > > > > That's a good question. I'm not sure how I came to that conclusion > > which > > > is clearly wrong. Though I think the buffer might share the line with > > the > > > mutex... > > Pointer shares a line. The buffer it points to doesn't as allocated > > by separate heap allocation. > > > > Ups, sure :facepalm: My understanding [1] was that devm_ allocations were generally not suitable for DMA and should not be used for this particular purpose because of the extra 16 bytes allocated for storing the devm magic somewhere, which shifts the entire buffer and prevents it to always be aligned on a cache line. I will propose a patch to switch to kmalloc_array() instead. [1] https://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma Thanks, Miquèl
On Wed, 1 Sep 2021 10:12:09 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hello, > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Mon, 30 Aug 2021 15:02:26 > +0000: > > > > -----Original Message----- > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Monday, August 30, 2021 4:30 PM > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter Clausen > > > <lars@metafoo.de>; Thomas Petazzoni > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > > > samples > > > > > > [External] > > > > > > On Mon, 30 Aug 2021 10:49:50 +0000 > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > -----Original Message----- > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > > Sent: Monday, August 30, 2021 12:08 PM > > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > > > Clausen > > > > > <lars@metafoo.de>; Thomas Petazzoni > > > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > > > linux- > > > > > kernel@vger.kernel.org > > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > > > requested > > > > > samples > > > > > > > > > > [External] > > > > > > > > > > On Fri, 20 Aug 2021 07:10:48 +0000 > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > > > > > <lars@metafoo.de> > > > > > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux- > > > > > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel > > > Raynal > > > > > > > <miquel.raynal@bootlin.com> > > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the > > > requested > > > > > > > samples > > > > > > > > > > > > > > [External] > > > > > > > > > > > > > > When a triggered scan occurs, the identity of the desired > > > channels > > > > > is > > > > > > > known in indio_dev->active_scan_mask. Instead of reading and > > > > > > > pushing to > > > > > > > the IIO buffers all channels each time, scan the minimum > > > amount > > > > > of > > > > > > > channels (0 to maximum requested chan, to be exact) and only > > > > > > > provide the > > > > > > > samples requested by the user. > > > > > > > > > > > > > > For example, if the user wants channels 1, 4 and 5, all channels > > > > > from > > > > > > > 0 to 5 will be scanned but only the desired channels will be > > > pushed > > > > > to > > > > > > > the IIO buffers. > > > > > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > --- > > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- > > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/max1027.c > > > b/drivers/iio/adc/max1027.c > > > > > > > index b753658bb41e..8ab660f596b5 100644 > > > > > > > --- a/drivers/iio/adc/max1027.c > > > > > > > +++ b/drivers/iio/adc/max1027.c > > > > > > > @@ -360,6 +360,9 @@ static int > > > max1027_set_trigger_state(struct > > > > > > > iio_trigger *trig, bool state) > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > int ret; > > > > > > > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, > > > indio_dev- > > > > > > > >masklength)) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > > > > > > I'm not sure this can actually happen. If you try to enable the > > > buffer > > > > > > with no scan element, it should give you an error before you > > > reach > > > > > > this point... > > > > > > > > > > > > > if (state) { > > > > > > > /* Start acquisition on cnvst */ > > > > > > > st->reg = MAX1027_SETUP_REG | > > > > > > > MAX1027_CKS_MODE0 | > > > > > > > @@ -368,9 +371,12 @@ static int > > > max1027_set_trigger_state(struct > > > > > > > iio_trigger *trig, bool state) > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > > > > > > > > - /* Scan from 0 to max */ > > > > > > > - st->reg = MAX1027_CONV_REG | > > > MAX1027_CHAN(0) | > > > > > > > - MAX1027_SCAN_N_M | > > > MAX1027_TEMP; > > > > > > > + /* > > > > > > > + * Scan from 0 to the highest requested > > > channel. The > > > > > > > temperature > > > > > > > + * could be avoided but it simplifies a bit the > > > logic. > > > > > > > + */ > > > > > > > + st->reg = MAX1027_CONV_REG | > > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > > > > > >active_scan_mask) - 2); > > > > > > > ret = spi_write(st->spi, &st->reg, 1); > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > > > > > max1027_trigger_handler(int irq, void *private) > > > > > > > struct iio_poll_func *pf = private; > > > > > > > struct iio_dev *indio_dev = pf->indio_dev; > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > + unsigned int scanned_chans = fls(*indio_dev- > > > > > > > >active_scan_mask); > > > > > > > + u16 *buf = st->buffer; > > > > > > > > > > > > I think sparse will complain here. buffer is a __be16 restricted > > > > > > type so you should not mix those... > > > > > > > + unsigned int bit; > > > > > > > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, > > > irq, > > > > > > > > > > > > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > > > > > _to_max1027_like_adcs.mbx > > > > > > > > > > > > > > /* fill buffer with all channel */ > > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * > > > 2); > > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > > > > > + > > > > > > > + /* Only keep the channels selected by the user */ > > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > > > > > + indio_dev->masklength) { > > > > > > > + if (buf[0] != st->buffer[bit]) > > > > > > > + buf[0] = st->buffer[bit]; > > > > > > > > > > > > Since we are here, when looking into the driver, I realized > > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to > > > enforce > > > > > > that all buffers that are passed to spi/i2c buses are safe... Maybe > > > > > > this is something you can include in your series. > > > > > > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() call > > > and > > > > > that should provide a DMA safe buffer as I understand it. > > > > > > > > > > > > > That's a good question. I'm not sure how I came to that conclusion > > > which > > > > is clearly wrong. Though I think the buffer might share the line with > > > the > > > > mutex... > > > Pointer shares a line. The buffer it points to doesn't as allocated > > > by separate heap allocation. > > > > > > > Ups, sure :facepalm: > > My understanding [1] was that devm_ allocations were generally not > suitable for DMA and should not be used for this particular purpose > because of the extra 16 bytes allocated for storing the devm magic > somewhere, which shifts the entire buffer and prevents it to always be > aligned on a cache line. I will propose a patch to switch to > kmalloc_array() instead. > > [1] https://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma That shouldn't actually matter because here we don't care about it being aligned on a cacheline - but we do care about it being aligned so that nothing else we might touch is in the same cacheline. Note the thread you link talks about this. If we were possibly doing additional devm_ managed allocations whilst DMA was active then it might be a problem, but I'm fairly sure we aren't doing that here. Not there might be a bus controller that has stricter alignment requirements - whether we need to be careful of those isn't particularly clear to me. There are lots of places in IIO where we cachealign a set of buffers, but for example the rx is after the tx buffers and isn't aligned as would be required. As such I'm fairly sure it's not a problem. Jonathan > > Thanks, > Miquèl
> -----Original Message----- > From: Miquel Raynal <miquel.raynal@bootlin.com> > Sent: Wednesday, September 1, 2021 10:12 AM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; Thomas Petazzoni > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > samples > > [External] > > Hello, > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Mon, 30 Aug 2021 > 15:02:26 > +0000: > > > > -----Original Message----- > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Monday, August 30, 2021 4:30 PM > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > Clausen > > > <lars@metafoo.de>; Thomas Petazzoni > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > requested > > > samples > > > > > > [External] > > > > > > On Mon, 30 Aug 2021 10:49:50 +0000 > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > -----Original Message----- > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > > Sent: Monday, August 30, 2021 12:08 PM > > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > > > Clausen > > > > > <lars@metafoo.de>; Thomas Petazzoni > > > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > > > linux- > > > > > kernel@vger.kernel.org > > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > > > requested > > > > > samples > > > > > > > > > > [External] > > > > > > > > > > On Fri, 20 Aug 2021 07:10:48 +0000 > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter > Clausen > > > > > > > <lars@metafoo.de> > > > > > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; > linux- > > > > > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel > > > Raynal > > > > > > > <miquel.raynal@bootlin.com> > > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the > > > requested > > > > > > > samples > > > > > > > > > > > > > > [External] > > > > > > > > > > > > > > When a triggered scan occurs, the identity of the desired > > > channels > > > > > is > > > > > > > known in indio_dev->active_scan_mask. Instead of reading > and > > > > > > > pushing to > > > > > > > the IIO buffers all channels each time, scan the minimum > > > amount > > > > > of > > > > > > > channels (0 to maximum requested chan, to be exact) and > only > > > > > > > provide the > > > > > > > samples requested by the user. > > > > > > > > > > > > > > For example, if the user wants channels 1, 4 and 5, all > channels > > > > > from > > > > > > > 0 to 5 will be scanned but only the desired channels will be > > > pushed > > > > > to > > > > > > > the IIO buffers. > > > > > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > --- > > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++- > --- > > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/max1027.c > > > b/drivers/iio/adc/max1027.c > > > > > > > index b753658bb41e..8ab660f596b5 100644 > > > > > > > --- a/drivers/iio/adc/max1027.c > > > > > > > +++ b/drivers/iio/adc/max1027.c > > > > > > > @@ -360,6 +360,9 @@ static int > > > max1027_set_trigger_state(struct > > > > > > > iio_trigger *trig, bool state) > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > int ret; > > > > > > > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, > > > indio_dev- > > > > > > > >masklength)) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > > > > > > I'm not sure this can actually happen. If you try to enable the > > > buffer > > > > > > with no scan element, it should give you an error before you > > > reach > > > > > > this point... > > > > > > > > > > > > > if (state) { > > > > > > > /* Start acquisition on cnvst */ > > > > > > > st->reg = MAX1027_SETUP_REG | > > > > > > > MAX1027_CKS_MODE0 | > > > > > > > @@ -368,9 +371,12 @@ static int > > > max1027_set_trigger_state(struct > > > > > > > iio_trigger *trig, bool state) > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > > > > > > > > - /* Scan from 0 to max */ > > > > > > > - st->reg = MAX1027_CONV_REG | > > > MAX1027_CHAN(0) | > > > > > > > - MAX1027_SCAN_N_M | > > > MAX1027_TEMP; > > > > > > > + /* > > > > > > > + * Scan from 0 to the highest requested > > > channel. The > > > > > > > temperature > > > > > > > + * could be avoided but it simplifies a bit the > > > logic. > > > > > > > + */ > > > > > > > + st->reg = MAX1027_CONV_REG | > > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > > > > > >active_scan_mask) - 2); > > > > > > > ret = spi_write(st->spi, &st->reg, 1); > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > > > > > max1027_trigger_handler(int irq, void *private) > > > > > > > struct iio_poll_func *pf = private; > > > > > > > struct iio_dev *indio_dev = pf->indio_dev; > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > + unsigned int scanned_chans = fls(*indio_dev- > > > > > > > >active_scan_mask); > > > > > > > + u16 *buf = st->buffer; > > > > > > > > > > > > I think sparse will complain here. buffer is a __be16 restricted > > > > > > type so you should not mix those... > > > > > > > + unsigned int bit; > > > > > > > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, > > > irq, > > > > > > > > > > > > > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > > > > > _to_max1027_like_adcs.mbx > > > > > > > > > > > > > > /* fill buffer with all channel */ > > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * > > > 2); > > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > > > > > + > > > > > > > + /* Only keep the channels selected by the user */ > > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > > > > > + indio_dev->masklength) { > > > > > > > + if (buf[0] != st->buffer[bit]) > > > > > > > + buf[0] = st->buffer[bit]; > > > > > > > > > > > > Since we are here, when looking into the driver, I realized > > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to > > > enforce > > > > > > that all buffers that are passed to spi/i2c buses are safe... > Maybe > > > > > > this is something you can include in your series. > > > > > > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() > call > > > and > > > > > that should provide a DMA safe buffer as I understand it. > > > > > > > > > > > > > That's a good question. I'm not sure how I came to that > conclusion > > > which > > > > is clearly wrong. Though I think the buffer might share the line > with > > > the > > > > mutex... > > > Pointer shares a line. The buffer it points to doesn't as allocated > > > by separate heap allocation. > > > > > > > Ups, sure :facepalm: > > My understanding [1] was that devm_ allocations were generally not > suitable for DMA and should not be used for this particular purpose > because of the extra 16 bytes allocated for storing the devm magic > somewhere, which shifts the entire buffer and prevents it to always > be > aligned on a cache line. I will propose a patch to switch to > kmalloc_array() instead. I do not think this is a problem anymore [1]. Nowadays, 'devm_kmalloc' should give you the same alignment guarantees as 'kmalloc' [1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L35 - Nuno Sá
On Mon, 6 Sep 2021 08:59:55 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > -----Original Message----- > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > Sent: Wednesday, September 1, 2021 10:12 AM > > To: Sa, Nuno <Nuno.Sa@analog.com> > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > <lars@metafoo.de>; Thomas Petazzoni > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > > samples > > > > [External] > > > > Hello, > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Mon, 30 Aug 2021 > > 15:02:26 > > +0000: > > > > > > -----Original Message----- > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > Sent: Monday, August 30, 2021 4:30 PM > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > > Clausen > > > > <lars@metafoo.de>; Thomas Petazzoni > > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > > linux- > > > > kernel@vger.kernel.org > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > > requested > > > > samples > > > > > > > > [External] > > > > > > > > On Mon, 30 Aug 2021 10:49:50 +0000 > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > > > Sent: Monday, August 30, 2021 12:08 PM > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > > > > Clausen > > > > > > <lars@metafoo.de>; Thomas Petazzoni > > > > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > > > > linux- > > > > > > kernel@vger.kernel.org > > > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > > > > requested > > > > > > samples > > > > > > > > > > > > [External] > > > > > > > > > > > > On Fri, 20 Aug 2021 07:10:48 +0000 > > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter > > Clausen > > > > > > > > <lars@metafoo.de> > > > > > > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; > > linux- > > > > > > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel > > > > Raynal > > > > > > > > <miquel.raynal@bootlin.com> > > > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the > > > > requested > > > > > > > > samples > > > > > > > > > > > > > > > > [External] > > > > > > > > > > > > > > > > When a triggered scan occurs, the identity of the desired > > > > channels > > > > > > is > > > > > > > > known in indio_dev->active_scan_mask. Instead of reading > > and > > > > > > > > pushing to > > > > > > > > the IIO buffers all channels each time, scan the minimum > > > > amount > > > > > > of > > > > > > > > channels (0 to maximum requested chan, to be exact) and > > only > > > > > > > > provide the > > > > > > > > samples requested by the user. > > > > > > > > > > > > > > > > For example, if the user wants channels 1, 4 and 5, all > > channels > > > > > > from > > > > > > > > 0 to 5 will be scanned but only the desired channels will be > > > > pushed > > > > > > to > > > > > > > > the IIO buffers. > > > > > > > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > > --- > > > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++- > > --- > > > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/max1027.c > > > > b/drivers/iio/adc/max1027.c > > > > > > > > index b753658bb41e..8ab660f596b5 100644 > > > > > > > > --- a/drivers/iio/adc/max1027.c > > > > > > > > +++ b/drivers/iio/adc/max1027.c > > > > > > > > @@ -360,6 +360,9 @@ static int > > > > max1027_set_trigger_state(struct > > > > > > > > iio_trigger *trig, bool state) > > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > > int ret; > > > > > > > > > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, > > > > indio_dev- > > > > > > > > >masklength)) > > > > > > > > + return -EINVAL; > > > > > > > > + > > > > > > > > > > > > > > I'm not sure this can actually happen. If you try to enable the > > > > buffer > > > > > > > with no scan element, it should give you an error before you > > > > reach > > > > > > > this point... > > > > > > > > > > > > > > > if (state) { > > > > > > > > /* Start acquisition on cnvst */ > > > > > > > > st->reg = MAX1027_SETUP_REG | > > > > > > > > MAX1027_CKS_MODE0 | > > > > > > > > @@ -368,9 +371,12 @@ static int > > > > max1027_set_trigger_state(struct > > > > > > > > iio_trigger *trig, bool state) > > > > > > > > if (ret < 0) > > > > > > > > return ret; > > > > > > > > > > > > > > > > - /* Scan from 0 to max */ > > > > > > > > - st->reg = MAX1027_CONV_REG | > > > > MAX1027_CHAN(0) | > > > > > > > > - MAX1027_SCAN_N_M | > > > > MAX1027_TEMP; > > > > > > > > + /* > > > > > > > > + * Scan from 0 to the highest requested > > > > channel. The > > > > > > > > temperature > > > > > > > > + * could be avoided but it simplifies a bit the > > > > logic. > > > > > > > > + */ > > > > > > > > + st->reg = MAX1027_CONV_REG | > > > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > > > > > > >active_scan_mask) - 2); > > > > > > > > ret = spi_write(st->spi, &st->reg, 1); > > > > > > > > if (ret < 0) > > > > > > > > return ret; > > > > > > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > > > > > > max1027_trigger_handler(int irq, void *private) > > > > > > > > struct iio_poll_func *pf = private; > > > > > > > > struct iio_dev *indio_dev = pf->indio_dev; > > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > > + unsigned int scanned_chans = fls(*indio_dev- > > > > > > > > >active_scan_mask); > > > > > > > > + u16 *buf = st->buffer; > > > > > > > > > > > > > > I think sparse will complain here. buffer is a __be16 restricted > > > > > > > type so you should not mix those... > > > > > > > > + unsigned int bit; > > > > > > > > > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, > > > > irq, > > > > > > > > > > > > > > > > > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > > > > > > _to_max1027_like_adcs.mbx > > > > > > > > > > > > > > > > /* fill buffer with all channel */ > > > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * > > > > 2); > > > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > > > > > > + > > > > > > > > + /* Only keep the channels selected by the user */ > > > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > > > > > > + indio_dev->masklength) { > > > > > > > > + if (buf[0] != st->buffer[bit]) > > > > > > > > + buf[0] = st->buffer[bit]; > > > > > > > > > > > > > > Since we are here, when looking into the driver, I realized > > > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to > > > > enforce > > > > > > > that all buffers that are passed to spi/i2c buses are safe... > > Maybe > > > > > > > this is something you can include in your series. > > > > > > > > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() > > call > > > > and > > > > > > that should provide a DMA safe buffer as I understand it. > > > > > > > > > > > > > > > > That's a good question. I'm not sure how I came to that > > conclusion > > > > which > > > > > is clearly wrong. Though I think the buffer might share the line > > with > > > > the > > > > > mutex... > > > > Pointer shares a line. The buffer it points to doesn't as allocated > > > > by separate heap allocation. > > > > > > > > > > Ups, sure :facepalm: > > > > My understanding [1] was that devm_ allocations were generally not > > suitable for DMA and should not be used for this particular purpose > > because of the extra 16 bytes allocated for storing the devm magic > > somewhere, which shifts the entire buffer and prevents it to always > > be > > aligned on a cache line. I will propose a patch to switch to > > kmalloc_array() instead. > > I do not think this is a problem anymore [1]. Nowadays, 'devm_kmalloc' > should give you the same alignment guarantees as 'kmalloc' > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L35 Great info. I remembered a discussion about fixing that, but couldn't find the patch. For some reason I didn't just check the code :) Thanks. Jonathan > > - Nuno Sá
Hi Jonathan, jic23@kernel.org wrote on Mon, 6 Sep 2021 17:56:57 +0100: > On Mon, 6 Sep 2021 08:59:55 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > -----Original Message----- > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > Sent: Wednesday, September 1, 2021 10:12 AM > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > <lars@metafoo.de>; Thomas Petazzoni > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested > > > samples > > > > > > [External] > > > > > > Hello, > > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote on Mon, 30 Aug 2021 > > > 15:02:26 > > > +0000: > > > > > > > > -----Original Message----- > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > > Sent: Monday, August 30, 2021 4:30 PM > > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > > > Clausen > > > > > <lars@metafoo.de>; Thomas Petazzoni > > > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > > > linux- > > > > > kernel@vger.kernel.org > > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > > > requested > > > > > samples > > > > > > > > > > [External] > > > > > > > > > > On Mon, 30 Aug 2021 10:49:50 +0000 > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > > > > Sent: Monday, August 30, 2021 12:08 PM > > > > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter > > > > > Clausen > > > > > > > <lars@metafoo.de>; Thomas Petazzoni > > > > > > > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; > > > > > linux- > > > > > > > kernel@vger.kernel.org > > > > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the > > > > > requested > > > > > > > samples > > > > > > > > > > > > > > [External] > > > > > > > > > > > > > > On Fri, 20 Aug 2021 07:10:48 +0000 > > > > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM > > > > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter > > > Clausen > > > > > > > > > <lars@metafoo.de> > > > > > > > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; > > > linux- > > > > > > > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel > > > > > Raynal > > > > > > > > > <miquel.raynal@bootlin.com> > > > > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the > > > > > requested > > > > > > > > > samples > > > > > > > > > > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > When a triggered scan occurs, the identity of the desired > > > > > channels > > > > > > > is > > > > > > > > > known in indio_dev->active_scan_mask. Instead of reading > > > and > > > > > > > > > pushing to > > > > > > > > > the IIO buffers all channels each time, scan the minimum > > > > > amount > > > > > > > of > > > > > > > > > channels (0 to maximum requested chan, to be exact) and > > > only > > > > > > > > > provide the > > > > > > > > > samples requested by the user. > > > > > > > > > > > > > > > > > > For example, if the user wants channels 1, 4 and 5, all > > > channels > > > > > > > from > > > > > > > > > 0 to 5 will be scanned but only the desired channels will be > > > > > pushed > > > > > > > to > > > > > > > > > the IIO buffers. > > > > > > > > > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > > > --- > > > > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++- > > > --- > > > > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/max1027.c > > > > > b/drivers/iio/adc/max1027.c > > > > > > > > > index b753658bb41e..8ab660f596b5 100644 > > > > > > > > > --- a/drivers/iio/adc/max1027.c > > > > > > > > > +++ b/drivers/iio/adc/max1027.c > > > > > > > > > @@ -360,6 +360,9 @@ static int > > > > > max1027_set_trigger_state(struct > > > > > > > > > iio_trigger *trig, bool state) > > > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask, > > > > > indio_dev- > > > > > > > > > >masklength)) > > > > > > > > > + return -EINVAL; > > > > > > > > > + > > > > > > > > > > > > > > > > I'm not sure this can actually happen. If you try to enable the > > > > > buffer > > > > > > > > with no scan element, it should give you an error before you > > > > > reach > > > > > > > > this point... > > > > > > > > > > > > > > > > > if (state) { > > > > > > > > > /* Start acquisition on cnvst */ > > > > > > > > > st->reg = MAX1027_SETUP_REG | > > > > > > > > > MAX1027_CKS_MODE0 | > > > > > > > > > @@ -368,9 +371,12 @@ static int > > > > > max1027_set_trigger_state(struct > > > > > > > > > iio_trigger *trig, bool state) > > > > > > > > > if (ret < 0) > > > > > > > > > return ret; > > > > > > > > > > > > > > > > > > - /* Scan from 0 to max */ > > > > > > > > > - st->reg = MAX1027_CONV_REG | > > > > > MAX1027_CHAN(0) | > > > > > > > > > - MAX1027_SCAN_N_M | > > > > > MAX1027_TEMP; > > > > > > > > > + /* > > > > > > > > > + * Scan from 0 to the highest requested > > > > > channel. The > > > > > > > > > temperature > > > > > > > > > + * could be avoided but it simplifies a bit the > > > > > logic. > > > > > > > > > + */ > > > > > > > > > + st->reg = MAX1027_CONV_REG | > > > > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP; > > > > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev- > > > > > > > > > >active_scan_mask) - 2); > > > > > > > > > ret = spi_write(st->spi, &st->reg, 1); > > > > > > > > > if (ret < 0) > > > > > > > > > return ret; > > > > > > > > > @@ -391,11 +397,22 @@ static irqreturn_t > > > > > > > > > max1027_trigger_handler(int irq, void *private) > > > > > > > > > struct iio_poll_func *pf = private; > > > > > > > > > struct iio_dev *indio_dev = pf->indio_dev; > > > > > > > > > struct max1027_state *st = iio_priv(indio_dev); > > > > > > > > > + unsigned int scanned_chans = fls(*indio_dev- > > > > > > > > > >active_scan_mask); > > > > > > > > > + u16 *buf = st->buffer; > > > > > > > > > > > > > > > > I think sparse will complain here. buffer is a __be16 restricted > > > > > > > > type so you should not mix those... > > > > > > > > > + unsigned int bit; > > > > > > > > > > > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, > > > > > irq, > > > > > > > > > > > > > > > > > > > > > > > > private);in/20210818_miquel_raynal_bring_software_triggers_support > > > > > > > _to_max1027_like_adcs.mbx > > > > > > > > > > > > > > > > > > /* fill buffer with all channel */ > > > > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * > > > > > 2); > > > > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2); > > > > > > > > > + > > > > > > > > > + /* Only keep the channels selected by the user */ > > > > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask, > > > > > > > > > + indio_dev->masklength) { > > > > > > > > > + if (buf[0] != st->buffer[bit]) > > > > > > > > > + buf[0] = st->buffer[bit]; > > > > > > > > > > > > > > > > Since we are here, when looking into the driver, I realized > > > > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to > > > > > enforce > > > > > > > > that all buffers that are passed to spi/i2c buses are safe... > > > Maybe > > > > > > > > this is something you can include in your series. > > > > > > > > > > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() > > > call > > > > > and > > > > > > > that should provide a DMA safe buffer as I understand it. > > > > > > > > > > > > > > > > > > > That's a good question. I'm not sure how I came to that > > > conclusion > > > > > which > > > > > > is clearly wrong. Though I think the buffer might share the line > > > with > > > > > the > > > > > > mutex... > > > > > Pointer shares a line. The buffer it points to doesn't as allocated > > > > > by separate heap allocation. > > > > > > > > > > > > > Ups, sure :facepalm: > > > > > > My understanding [1] was that devm_ allocations were generally not > > > suitable for DMA and should not be used for this particular purpose > > > because of the extra 16 bytes allocated for storing the devm magic > > > somewhere, which shifts the entire buffer and prevents it to always > > > be > > > aligned on a cache line. I will propose a patch to switch to > > > kmalloc_array() instead. > > > > I do not think this is a problem anymore [1]. Nowadays, 'devm_kmalloc' > > should give you the same alignment guarantees as 'kmalloc' > > > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L35 > Great info. I remembered a discussion about fixing that, but couldn't find > the patch. For some reason I didn't just check the code :) Nice! I didn't know about that, thanks a lot for sharing. So this patch can be safely discarded then. Thanks, Miquèl
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c index b753658bb41e..8ab660f596b5 100644 --- a/drivers/iio/adc/max1027.c +++ b/drivers/iio/adc/max1027.c @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state) struct max1027_state *st = iio_priv(indio_dev); int ret; + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) + return -EINVAL; + if (state) { /* Start acquisition on cnvst */ st->reg = MAX1027_SETUP_REG | MAX1027_CKS_MODE0 | @@ -368,9 +371,12 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state) if (ret < 0) return ret; - /* Scan from 0 to max */ - st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) | - MAX1027_SCAN_N_M | MAX1027_TEMP; + /* + * Scan from 0 to the highest requested channel. The temperature + * could be avoided but it simplifies a bit the logic. + */ + st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N | MAX1027_TEMP; + st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2); ret = spi_write(st->spi, &st->reg, 1); if (ret < 0) return ret; @@ -391,11 +397,22 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private) struct iio_poll_func *pf = private; struct iio_dev *indio_dev = pf->indio_dev; struct max1027_state *st = iio_priv(indio_dev); + unsigned int scanned_chans = fls(*indio_dev->active_scan_mask); + u16 *buf = st->buffer; + unsigned int bit; pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private); /* fill buffer with all channel */ - spi_read(st->spi, st->buffer, indio_dev->masklength * 2); + spi_read(st->spi, st->buffer, scanned_chans * 2); + + /* Only keep the channels selected by the user */ + for_each_set_bit(bit, indio_dev->active_scan_mask, + indio_dev->masklength) { + if (buf[0] != st->buffer[bit]) + buf[0] = st->buffer[bit]; + buf++; + } iio_push_to_buffers(indio_dev, st->buffer);
When a triggered scan occurs, the identity of the desired channels is known in indio_dev->active_scan_mask. Instead of reading and pushing to the IIO buffers all channels each time, scan the minimum amount of channels (0 to maximum requested chan, to be exact) and only provide the samples requested by the user. For example, if the user wants channels 1, 4 and 5, all channels from 0 to 5 will be scanned but only the desired channels will be pushed to the IIO buffers. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)