diff mbox series

[1/4] iio/adc: ingenic: fix channel offsets in buffer

Message ID 20220817105643.95710-2-contact@artur-rojek.eu (mailing list archive)
State Handled Elsewhere
Headers show
Series iio/adc-joystick: buffer data parsing fixes | expand

Commit Message

Artur Rojek Aug. 17, 2022, 10:56 a.m. UTC
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(-)

Comments

Andy Shevchenko Aug. 19, 2022, 8:12 a.m. UTC | #1
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.
Paul Cercueil Aug. 19, 2022, 10:07 a.m. UTC | #2
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
Andy Shevchenko Aug. 19, 2022, 10:15 a.m. UTC | #3
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 mbox series

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);