| Message ID | 20220817105643.95710-1-contact@artur-rojek.eu |
|---|---|
| Headers | show |
| Series | iio/adc-joystick: buffer data parsing fixes | expand |
On Wed, Aug 17, 2022 at 12:56:39PM +0200, Artur Rojek wrote: > Hi all, > > this patch set fixes the way channel data is being parsed in the > adc-joystick driver. To achieve that, it also introduces helpers in the > IIO subsystem. As a side effect of those changes, a bug in ingenic-adc > has been exposed, which this patch set promptly rectifies. > > Tested on GCW Zero (by me) and on Anbernic RG350 (by Paul). > > Chris: > As you have originally reported the issue, would you be able to test > the above changes on your setup (Odroid Go Advance, was it)? I can confirm this fixes the issue I experienced, I can see both channels of the joystick now when using an hrtimer as a trigger. This patch also does not interfere with the polling work in progress, as that still works as expected too (polling work is still desired though). Thank you. > > Artur Rojek (4): > iio/adc: ingenic: fix channel offsets in buffer > iio: add iio_channel_cb_get_iio_buffer helper > iio: add helper function for reading channel offset in buffer > input: joystick: Fix buffer data parsing > > drivers/iio/adc/ingenic-adc.c | 7 +++--- > drivers/iio/buffer/industrialio-buffer-cb.c | 7 ++++++ > drivers/iio/industrialio-buffer.c | 28 +++++++++++++++++++++ > drivers/input/joystick/adc-joystick.c | 26 ++++++++++++------- > include/linux/iio/buffer.h | 4 +++ > include/linux/iio/consumer.h | 12 +++++++++ > 6 files changed, 71 insertions(+), 13 deletions(-) > > -- > 2.37.2 >
On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> wrote: > > Consumers expect the buffer to only contain enabled channels. While > preparing the buffer, the driver also (incorrectly) inserts empty data > for disabled channels, causing the enabled channels to appear at wrong > offsets. Fix that. What consumers? Have you tested on all of them? Please, elaborate. It might be that some of them have to be fixed. In such case you need to report the issue to their respective channels and put the corresponding links here. P.S. It doesn't mean I'm against the patch.
On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> wrote: > > Introduce a helper function to retrieve an iio_buffer from > iio_cb_buffer. > > This is useful for consumers that need to extract metadata about > the buffer, e.g. get the channel offsets. I'm wondering if we should start using the IIO namespace for new exported symbols.
Hi Andy, Le ven., août 19 2022 at 11:12:38 +0300, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> > wrote: >> >> Consumers expect the buffer to only contain enabled channels. While >> preparing the buffer, the driver also (incorrectly) inserts empty >> data >> for disabled channels, causing the enabled channels to appear at >> wrong >> offsets. Fix that. > > What consumers? Have you tested on all of them? Please, elaborate. It > might be that some of them have to be fixed. In such case you need to > report the issue to their respective channels and put the > corresponding links here. There are no consumers to fix, only this driver. I believe it wasn't noticed until now because all consumers were only using channels 0 and 1. Cheers, -Paul > P.S. It doesn't mean I'm against the patch. > > -- > With Best Regards, > Andy Shevchenko
On Fri, Aug 19, 2022 at 1:07 PM Paul Cercueil <paul@crapouillou.net> wrote: > > Hi Andy, > > Le ven., août 19 2022 at 11:12:38 +0300, Andy Shevchenko > <andy.shevchenko@gmail.com> a écrit : > > On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> > > wrote: > >> > >> Consumers expect the buffer to only contain enabled channels. While > >> preparing the buffer, the driver also (incorrectly) inserts empty > >> data > >> for disabled channels, causing the enabled channels to appear at > >> wrong > >> offsets. Fix that. > > > > What consumers? Have you tested on all of them? Please, elaborate. It > > might be that some of them have to be fixed. In such case you need to > > report the issue to their respective channels and put the > > corresponding links here. > > There are no consumers to fix, only this driver. I believe it wasn't > noticed until now because all consumers were only using channels 0 and > 1. Something like this explanation is missed in the commit message, with that added (in the above or similar form) Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > P.S. It doesn't mean I'm against the patch.
On 2022-08-18 20:28, Chris Morgan wrote: > On Wed, Aug 17, 2022 at 12:56:39PM +0200, Artur Rojek wrote: >> Hi all, >> >> this patch set fixes the way channel data is being parsed in the >> adc-joystick driver. To achieve that, it also introduces helpers in >> the >> IIO subsystem. As a side effect of those changes, a bug in ingenic-adc >> has been exposed, which this patch set promptly rectifies. >> >> Tested on GCW Zero (by me) and on Anbernic RG350 (by Paul). >> >> Chris: >> As you have originally reported the issue, would you be able to test >> the above changes on your setup (Odroid Go Advance, was it)? > > I can confirm this fixes the issue I experienced, I can see both > channels of the joystick now when using an hrtimer as a trigger. > > This patch also does not interfere with the polling work in progress, > as that still works as expected too (polling work is still desired > though). > > Thank you. Perfect, thanks for testing! Can I add your Tested-by for v2 of this patchset? Cheers, Artur > >> >> Artur Rojek (4): >> iio/adc: ingenic: fix channel offsets in buffer >> iio: add iio_channel_cb_get_iio_buffer helper >> iio: add helper function for reading channel offset in buffer >> input: joystick: Fix buffer data parsing >> >> drivers/iio/adc/ingenic-adc.c | 7 +++--- >> drivers/iio/buffer/industrialio-buffer-cb.c | 7 ++++++ >> drivers/iio/industrialio-buffer.c | 28 >> +++++++++++++++++++++ >> drivers/input/joystick/adc-joystick.c | 26 ++++++++++++------- >> include/linux/iio/buffer.h | 4 +++ >> include/linux/iio/consumer.h | 12 +++++++++ >> 6 files changed, 71 insertions(+), 13 deletions(-) >> >> -- >> 2.37.2 >>
On Fri, 19 Aug 2022 11:14:04 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> wrote: > > > > Introduce a helper function to retrieve an iio_buffer from > > iio_cb_buffer. > > > > This is useful for consumers that need to extract metadata about > > the buffer, e.g. get the channel offsets. > > I'm wondering if we should start using the IIO namespace for new > exported symbols. > I'd rather not jump ahead with that because I want to come up with a coherent set of IIO namespaces to separate core / drivers / consumers and platform type code (there's a bit of that left) plus maybe even trigger and buffer implementations. We should probably get on with that though! Jonathan
On Wed, 17 Aug 2022 12:56:41 +0200 Artur Rojek <contact@artur-rojek.eu> wrote: > Introduce a helper function to retrieve an iio_buffer from > iio_cb_buffer. > > This is useful for consumers that need to extract metadata about > the buffer, e.g. get the channel offsets. > > Tested-by: Paul Cercueil <paul@crapouillou.net> > Signed-off-by: Artur Rojek <contact@artur-rojek.eu> Hmm. I'm not keen on breaking this boundary between exposed interface and implementation like this. The intent was always that the consumer knew what it was requesting and had access to all the channel information so should know what the buffer alignment is. In this driver there is a call to devm_iio_channel_get_all() which returns the channels. The buffer offsets can be calculated from that information as the alignement in a buffer a consumer sees is entirely controlled by that information. It might be helpful to provide some helper functions to allow the consumer to establish where particular channels are though. (which will look very like what userspace code has to do as the information available is much the same). Perhaps I'm missing some information that is missing from what is exposed to consumers? Jonathan > --- > drivers/iio/buffer/industrialio-buffer-cb.c | 7 +++++++ > include/linux/iio/consumer.h | 12 ++++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c > index 4c12b7a94af5..47d6e28b4d36 100644 > --- a/drivers/iio/buffer/industrialio-buffer-cb.c > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c > @@ -151,6 +151,13 @@ struct iio_dev > } > EXPORT_SYMBOL_GPL(iio_channel_cb_get_iio_dev); > > +struct iio_buffer > +*iio_channel_cb_get_iio_buffer(struct iio_cb_buffer *cb_buffer) > +{ > + return &cb_buffer->buffer; > +} > +EXPORT_SYMBOL_GPL(iio_channel_cb_get_iio_buffer); > + > MODULE_AUTHOR("Jonathan Cameron <jic23@kernel.org>"); > MODULE_DESCRIPTION("Industrial I/O callback buffer"); > MODULE_LICENSE("GPL"); > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h > index 6802596b017c..c28925d5b69c 100644 > --- a/include/linux/iio/consumer.h > +++ b/include/linux/iio/consumer.h > @@ -196,6 +196,18 @@ struct iio_channel > struct iio_dev > *iio_channel_cb_get_iio_dev(const struct iio_cb_buffer *cb_buffer); > > +/** > + * iio_channel_cb_get_iio_buffer() - get access to the underlying buffer. > + * @cb_buffer: The callback buffer from whom we want the buffer > + * information. > + * > + * This function allows one to obtain information about the buffer. > + * The primary aim is to allow drivers that are consuming a buffer to query > + * things like channel offsets in the buffer. > + */ > +struct iio_buffer > +*iio_channel_cb_get_iio_buffer(struct iio_cb_buffer *cb_buffer); > + > /** > * iio_read_channel_raw() - read from a given channel > * @chan: The channel being queried.