| Message ID | 20220817105643.95710-2-contact@artur-rojek.eu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio/adc-joystick: buffer data parsing fixes | expand |
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.
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.
diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c index a7325dbbb99a..5a932c375a89 100644 --- a/drivers/iio/adc/ingenic-adc.c +++ b/drivers/iio/adc/ingenic-adc.c @@ -804,11 +804,10 @@ static irqreturn_t ingenic_adc_irq(int irq, void *data) unsigned int i; u32 tdat[3]; - for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) { + memset(tdat, 0, ARRAY_SIZE(tdat)); + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { if (mask & 0x3) - tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH); - else - tdat[i] = 0; + tdat[i++] = readl(adc->base + JZ_ADC_REG_ADTCH); } iio_push_to_buffers(iio_dev, tdat);
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. Fixes: b96952f498db ("IIO: Ingenic JZ47xx: Add touchscreen mode.") Tested-by: Paul Cercueil <paul@crapouillou.net> Signed-off-by: Artur Rojek <contact@artur-rojek.eu> --- drivers/iio/adc/ingenic-adc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)