| Message ID | 20210219124012.92897-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: Add output buffer support | expand |
On Fri, 19 Feb 2021 14:40:06 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > Changelog v2 -> v3: > * https://lore.kernel.org/linux-iio/20210217083438.37865-4-alexandru.ardelean@analog.com/T/#m396545e0c6cc9d58e17f4d79b6fc707fd0373d89 > * adding only infrastructure pieces for output DAC buffers, unfortunately I > couldn't finish a complete DAC change to showcase these changes For that I wonder if the driver you did previously would work with the hrtimer trigger (so just drop the pwm stuff). Obviously we'd want someone to sanity check that with the hardware. An alternative (for now) would be to add a simple example to the dummy driver. I'm not keen to take this series without the user, but I'll review it on basis we'll get that sorted fairly soon in some fashion. Jonathan > * patch 'iio: Add output buffer support' > - moved new 'bufferY/direction' attribute at the end and added > comment about what it should be added at the end > * removed Lars' comment '/* need a way of knowing if there may be enough data... */' > * updated some various formatting; > > Alexandru Ardelean (1): > iio: triggered-buffer: extend support to configure output buffers > > Lars-Peter Clausen (5): > iio: Add output buffer support > iio: kfifo-buffer: Add output buffer support > iio: buffer-dma: Allow to provide custom buffer ops > iio: buffer-dma: Add output buffer support > iio: buffer-dma: add support for cyclic DMA transfers > > Documentation/ABI/testing/sysfs-bus-iio | 7 + > drivers/iio/accel/adxl372.c | 1 + > drivers/iio/accel/bmc150-accel-core.c | 1 + > drivers/iio/adc/adi-axi-adc.c | 4 +- > drivers/iio/adc/at91-sama5d2_adc.c | 4 +- > drivers/iio/buffer/industrialio-buffer-dma.c | 120 ++++++++++++++-- > .../buffer/industrialio-buffer-dmaengine.c | 72 +++++++--- > .../buffer/industrialio-triggered-buffer.c | 8 +- > drivers/iio/buffer/kfifo_buf.c | 50 +++++++ > .../cros_ec_sensors/cros_ec_sensors_core.c | 1 + > .../common/hid-sensors/hid-sensor-trigger.c | 5 +- > drivers/iio/industrialio-buffer.c | 133 +++++++++++++++++- > include/linux/iio/buffer-dma.h | 11 +- > include/linux/iio/buffer-dmaengine.h | 8 +- > include/linux/iio/buffer.h | 7 + > include/linux/iio/buffer_impl.h | 11 ++ > include/linux/iio/triggered_buffer.h | 11 +- > include/uapi/linux/iio/buffer.h | 1 + > 18 files changed, 412 insertions(+), 43 deletions(-) >
Hi Jonathan and others, With output/dac buffer support the semantics of the scan_element type may change. Today the Format is [be|le]:[s|u]bits/storagebitsXrepeat[>>shift]. While shift (if specified) is the shift that needs to be applied prior to masking out unused bits. So far so good and it sounds universal. However, we use the right shift (operator) for that, which makes sense for capture devices. For output devices the more logical operator would be the left shift. I'm not proposing a new Format here. I just want to get some agreement that for an output device le:s12/16>>4 is understood as a left shift of 4, since the unused bits are then on the LSB. Thoughts? Best Regards, Michael Analog Devices GmbH Michael Hennerich Otl-Aicher Strasse 60-64 D-80807 Muenchen; Germany Sitz der Gesellschaft München, Registergericht München HRB 40368, Geschäftsführer: Stefan Steyerl, Michael Paul Sondel, Yoon Ah Oh
On Fri, 5 Mar 2021 08:57:08 +0000 "Hennerich, Michael" <Michael.Hennerich@analog.com> wrote: > Hi Jonathan and others, > > With output/dac buffer support the semantics of the scan_element type may change. > > Today the Format is [be|le]:[s|u]bits/storagebitsXrepeat[>>shift]. > > While shift (if specified) is the shift that needs to be applied prior to masking out unused bits. > > So far so good and it sounds universal. > > However, we use the right shift (operator) for that, which makes sense for capture devices. > For output devices the more logical operator would be the left shift. > > I'm not proposing a new Format here. I just want to get some agreement that for an output device > > le:s12/16>>4 > > is understood as a left shift of 4, since the unused bits are then on the LSB. Good question. Guess I wasn't thinking ahead when I came up with that :) I'm not sure I'd mind if we did decide to define a new format for output buffers. Feels like it should be easy to do. What do others think? Jonathan > > Thoughts? > > Best Regards, > Michael > > Analog Devices GmbH > Michael Hennerich > Otl-Aicher Strasse 60-64 > D-80807 Muenchen; Germany > > Sitz der Gesellschaft München, Registergericht München HRB 40368, > Geschäftsführer: Stefan Steyerl, Michael Paul Sondel, Yoon Ah Oh > >
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, March 6, 2021 6:35 PM > To: Hennerich, Michael <Michael.Hennerich@analog.com> > Cc: zzzzArdelean, zzzzAlexandru <alexandru.Ardelean@analog.com>; > linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > lars@metafoo.de; Sa, Nuno <Nuno.Sa@analog.com>; Bogdan, Dragos > <Dragos.Bogdan@analog.com> > Subject: Re: [PATCH v3 0/6] iio: Add output buffer support > > On Fri, 5 Mar 2021 08:57:08 +0000 > "Hennerich, Michael" <Michael.Hennerich@analog.com> wrote: > > > Hi Jonathan and others, > > > > With output/dac buffer support the semantics of the scan_element > type may change. > > > > Today the Format is [be|le]:[s|u]bits/storagebitsXrepeat[>>shift]. > > > > While shift (if specified) is the shift that needs to be applied prior to > masking out unused bits. > > > > So far so good and it sounds universal. > > > > However, we use the right shift (operator) for that, which makes > sense for capture devices. > > For output devices the more logical operator would be the left shift. > > > > I'm not proposing a new Format here. I just want to get some > agreement that for an output device > > > > le:s12/16>>4 > > > > is understood as a left shift of 4, since the unused bits are then on > the LSB. > > Good question. Guess I wasn't thinking ahead when I came up with > that :) > > I'm not sure I'd mind if we did decide to define a new format for > output > buffers. Feels like it should be easy to do. > > What do others think? > I guess the most straight forward thing would be just to add a 'shift_l' variable to 'struct scan_type'' and make sure either 'shift_l' or 'shift' is defined and then properly export either ">>" or "<<" to userspace? - Nuno Sá
On Mon, 8 Mar 2021 10:07:05 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Saturday, March 6, 2021 6:35 PM > > To: Hennerich, Michael <Michael.Hennerich@analog.com> > > Cc: zzzzArdelean, zzzzAlexandru <alexandru.Ardelean@analog.com>; > > linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > > lars@metafoo.de; Sa, Nuno <Nuno.Sa@analog.com>; Bogdan, Dragos > > <Dragos.Bogdan@analog.com> > > Subject: Re: [PATCH v3 0/6] iio: Add output buffer support > > > > On Fri, 5 Mar 2021 08:57:08 +0000 > > "Hennerich, Michael" <Michael.Hennerich@analog.com> wrote: > > > > > Hi Jonathan and others, > > > > > > With output/dac buffer support the semantics of the scan_element > > type may change. > > > > > > Today the Format is [be|le]:[s|u]bits/storagebitsXrepeat[>>shift]. > > > > > > While shift (if specified) is the shift that needs to be applied prior to > > masking out unused bits. > > > > > > So far so good and it sounds universal. > > > > > > However, we use the right shift (operator) for that, which makes > > sense for capture devices. > > > For output devices the more logical operator would be the left shift. > > > > > > I'm not proposing a new Format here. I just want to get some > > agreement that for an output device > > > > > > le:s12/16>>4 > > > > > > is understood as a left shift of 4, since the unused bits are then on > > the LSB. > > > > Good question. Guess I wasn't thinking ahead when I came up with > > that :) > > > > I'm not sure I'd mind if we did decide to define a new format for > > output > > buffers. Feels like it should be easy to do. > > > > What do others think? > > > > I guess the most straight forward thing would be just to add a 'shift_l' variable > to 'struct scan_type'' and make sure either 'shift_l' or 'shift' is defined and then > properly export either ">>" or "<<" to userspace? Given we already know it's an output channel, can we not just use that to make the decision? Jonathan > > - Nuno Sá >
> -----Original Message----- > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Sent: Monday, March 8, 2021 12:52 PM > To: Sa, Nuno <Nuno.Sa@analog.com>; Jonathan Cameron > <jic23@kernel.org> > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; > zzzzArdelean, zzzzAlexandru <alexandru.Ardelean@analog.com>; > linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > lars@metafoo.de; Bogdan, Dragos <Dragos.Bogdan@analog.com> > Subject: Re: [PATCH v3 0/6] iio: Add output buffer support > > [External] > > On Mon, 8 Mar 2021 10:07:05 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > -----Original Message----- > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Saturday, March 6, 2021 6:35 PM > > > To: Hennerich, Michael <Michael.Hennerich@analog.com> > > > Cc: zzzzArdelean, zzzzAlexandru > <alexandru.Ardelean@analog.com>; > > > linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > > > lars@metafoo.de; Sa, Nuno <Nuno.Sa@analog.com>; Bogdan, > Dragos > > > <Dragos.Bogdan@analog.com> > > > Subject: Re: [PATCH v3 0/6] iio: Add output buffer support > > > > > > On Fri, 5 Mar 2021 08:57:08 +0000 > > > "Hennerich, Michael" <Michael.Hennerich@analog.com> wrote: > > > > > > > Hi Jonathan and others, > > > > > > > > With output/dac buffer support the semantics of the > scan_element > > > type may change. > > > > > > > > Today the Format is > [be|le]:[s|u]bits/storagebitsXrepeat[>>shift]. > > > > > > > > While shift (if specified) is the shift that needs to be applied prior > to > > > masking out unused bits. > > > > > > > > So far so good and it sounds universal. > > > > > > > > However, we use the right shift (operator) for that, which makes > > > sense for capture devices. > > > > For output devices the more logical operator would be the left > shift. > > > > > > > > I'm not proposing a new Format here. I just want to get some > > > agreement that for an output device > > > > > > > > le:s12/16>>4 > > > > > > > > is understood as a left shift of 4, since the unused bits are then > on > > > the LSB. > > > > > > Good question. Guess I wasn't thinking ahead when I came up with > > > that :) > > > > > > I'm not sure I'd mind if we did decide to define a new format for > > > output > > > buffers. Feels like it should be easy to do. > > > > > > What do others think? > > > > > > > I guess the most straight forward thing would be just to add a 'shift_l' > variable > > to 'struct scan_type'' and make sure either 'shift_l' or 'shift' is defined > and then > > properly export either ">>" or "<<" to userspace? > > Given we already know it's an output channel, can we not just use that > to make the decision? > > Jonathan I would argue that having two variables gives us more flexibility for whatever the future brings us :). But if we can sanely say that an output buffer will always use left shifts, then we could definitely use that information... I mean, we are already doing that assumption for input buffers and right shifts... - Nuno Sá
> -----Original Message----- > From: Sa, Nuno <Nuno.Sa@analog.com> > Sent: Monday, March 8, 2021 2:01 PM > To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>; Jonathan > Cameron <jic23@kernel.org> > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; > zzzzArdelean, zzzzAlexandru <alexandru.Ardelean@analog.com>; > linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > lars@metafoo.de; Bogdan, Dragos <Dragos.Bogdan@analog.com> > Subject: RE: [PATCH v3 0/6] iio: Add output buffer support > > [External] > > > > > -----Original Message----- > > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > > Sent: Monday, March 8, 2021 12:52 PM > > To: Sa, Nuno <Nuno.Sa@analog.com>; Jonathan Cameron > > <jic23@kernel.org> > > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; > > zzzzArdelean, zzzzAlexandru <alexandru.Ardelean@analog.com>; > > linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > > lars@metafoo.de; Bogdan, Dragos <Dragos.Bogdan@analog.com> > > Subject: Re: [PATCH v3 0/6] iio: Add output buffer support > > > > [External] > > > > On Mon, 8 Mar 2021 10:07:05 +0000 > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > -----Original Message----- > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > Sent: Saturday, March 6, 2021 6:35 PM > > > > To: Hennerich, Michael <Michael.Hennerich@analog.com> > > > > Cc: zzzzArdelean, zzzzAlexandru > > <alexandru.Ardelean@analog.com>; > > > > linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > > > > lars@metafoo.de; Sa, Nuno <Nuno.Sa@analog.com>; Bogdan, > > Dragos > > > > <Dragos.Bogdan@analog.com> > > > > Subject: Re: [PATCH v3 0/6] iio: Add output buffer support > > > > > > > > On Fri, 5 Mar 2021 08:57:08 +0000 > > > > "Hennerich, Michael" <Michael.Hennerich@analog.com> wrote: > > > > > > > > > Hi Jonathan and others, > > > > > > > > > > With output/dac buffer support the semantics of the > > scan_element > > > > type may change. > > > > > > > > > > Today the Format is > > [be|le]:[s|u]bits/storagebitsXrepeat[>>shift]. > > > > > > > > > > While shift (if specified) is the shift that needs to be applied > prior > > to > > > > masking out unused bits. > > > > > > > > > > So far so good and it sounds universal. > > > > > > > > > > However, we use the right shift (operator) for that, which > makes > > > > sense for capture devices. > > > > > For output devices the more logical operator would be the left > > shift. > > > > > > > > > > I'm not proposing a new Format here. I just want to get some > > > > agreement that for an output device > > > > > > > > > > le:s12/16>>4 > > > > > > > > > > is understood as a left shift of 4, since the unused bits are then > > on > > > > the LSB. > > > > > > > > Good question. Guess I wasn't thinking ahead when I came up > with > > > > that :) > > > > > > > > I'm not sure I'd mind if we did decide to define a new format for > > > > output > > > > buffers. Feels like it should be easy to do. > > > > > > > > What do others think? > > > > > > > > > > I guess the most straight forward thing would be just to add a > 'shift_l' > > variable > > > to 'struct scan_type'' and make sure either 'shift_l' or 'shift' is > defined > > and then > > > properly export either ">>" or "<<" to userspace? > > > > Given we already know it's an output channel, can we not just use > that > > to make the decision? > > > > Jonathan > > I would argue that having two variables gives us more flexibility for > whatever > the future brings us :). But if we can sanely say that an output buffer > will > always use left shifts, then we could definitely use that information... I > mean, > we are already doing that assumption for input buffers and right > shifts... Hmm, giving it a bit more thought I think you can disregard the above. Using the information that it's an output channel should be more than enough... - Nuno Sá
Changelog v2 -> v3: * https://lore.kernel.org/linux-iio/20210217083438.37865-4-alexandru.ardelean@analog.com/T/#m396545e0c6cc9d58e17f4d79b6fc707fd0373d89 * adding only infrastructure pieces for output DAC buffers, unfortunately I couldn't finish a complete DAC change to showcase these changes * patch 'iio: Add output buffer support' - moved new 'bufferY/direction' attribute at the end and added comment about what it should be added at the end * removed Lars' comment '/* need a way of knowing if there may be enough data... */' * updated some various formatting; Alexandru Ardelean (1): iio: triggered-buffer: extend support to configure output buffers Lars-Peter Clausen (5): iio: Add output buffer support iio: kfifo-buffer: Add output buffer support iio: buffer-dma: Allow to provide custom buffer ops iio: buffer-dma: Add output buffer support iio: buffer-dma: add support for cyclic DMA transfers Documentation/ABI/testing/sysfs-bus-iio | 7 + drivers/iio/accel/adxl372.c | 1 + drivers/iio/accel/bmc150-accel-core.c | 1 + drivers/iio/adc/adi-axi-adc.c | 4 +- drivers/iio/adc/at91-sama5d2_adc.c | 4 +- drivers/iio/buffer/industrialio-buffer-dma.c | 120 ++++++++++++++-- .../buffer/industrialio-buffer-dmaengine.c | 72 +++++++--- .../buffer/industrialio-triggered-buffer.c | 8 +- drivers/iio/buffer/kfifo_buf.c | 50 +++++++ .../cros_ec_sensors/cros_ec_sensors_core.c | 1 + .../common/hid-sensors/hid-sensor-trigger.c | 5 +- drivers/iio/industrialio-buffer.c | 133 +++++++++++++++++- include/linux/iio/buffer-dma.h | 11 +- include/linux/iio/buffer-dmaengine.h | 8 +- include/linux/iio/buffer.h | 7 + include/linux/iio/buffer_impl.h | 11 ++ include/linux/iio/triggered_buffer.h | 11 +- include/uapi/linux/iio/buffer.h | 1 + 18 files changed, 412 insertions(+), 43 deletions(-)