| Message ID | 20230521225901.388455-2-contact@artur-rojek.eu |
|---|---|
| State | New |
| Headers | show |
| Series | iio/adc-joystick: buffer data parsing fixes | expand |
On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> wrote: > > Consumers expect the buffer to only contain enabled channels. While > preparing the buffer, the driver makes two mistakes: > 1) It inserts empty data for disabled channels. > 2) Each ADC readout contains samples for two 16-bit channels. If either > of them is active, the whole 32-bit sample is pushed into the buffer > as-is. > > Both of those issues cause the active channels to appear at the wrong > offsets in the buffer. Fix the above by demuxing samples for active > channels only. > > This has remained unnoticed, as all the consumers so far were only using > channels 0 and 1, leaving them unaffected by changes introduced in this > commit. ... > + u16 tdat[6]; > + u32 val; > + > + memset(tdat, 0, ARRAY_SIZE(tdat)); Yeah, as LKP tells us this should be sizeof() instead of ARRAY_SIZE(). > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { > + if (mask & 0x3) { (for the consistency it has to be GENMASK(), but see below) First of all, strictly speaking we should use the full mask without limiting it to the 0 element in the array (I'm talking about active_scan_mask). That said, we may actually use bit operations here in a better way, i.e. unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - 1); j = 0; for_each_set_bit(i, active_scan_mask, ...) { val = readl(...); /* Two channels per sample. Demux active. */ tdat[j++] = val >> (16 * (i % 2)); } > + val = readl(adc->base + JZ_ADC_REG_ADTCH); > + /* Two channels per sample. Demux active. */ > + if (mask & BIT(0)) > + tdat[i++] = val & 0xffff; > + if (mask & BIT(1)) > + tdat[i++] = val >> 16; > + } > }
On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> wrote: ... > > + u16 tdat[6]; > > + u32 val; > > + > > + memset(tdat, 0, ARRAY_SIZE(tdat)); > > Yeah, as LKP tells us this should be sizeof() instead of ARRAY_SIZE(). > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { > > + if (mask & 0x3) { > > (for the consistency it has to be GENMASK(), but see below) > > First of all, strictly speaking we should use the full mask without > limiting it to the 0 element in the array (I'm talking about > active_scan_mask). > > That said, we may actually use bit operations here in a better way, i.e. > > unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - 1); > > j = 0; > for_each_set_bit(i, active_scan_mask, ...) { > val = readl(...); > /* Two channels per sample. Demux active. */ > tdat[j++] = val >> (16 * (i % 2)); Alternatively /* Two channels per sample. Demux active. */ if (i % 2) tdat[j++] = upper_16_bits(val); else tdat[j++] = lower_16_bits(val); which may be better to read. > } > > > + val = readl(adc->base + JZ_ADC_REG_ADTCH); > > + /* Two channels per sample. Demux active. */ > > + if (mask & BIT(0)) > > + tdat[i++] = val & 0xffff; > > + if (mask & BIT(1)) > > + tdat[i++] = val >> 16; > > + } > > }
Hi, Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit : > On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> > wrote: > > > > Consumers expect the buffer to only contain enabled channels. While > > preparing the buffer, the driver makes two mistakes: > > 1) It inserts empty data for disabled channels. > > 2) Each ADC readout contains samples for two 16-bit channels. If > > either > > of them is active, the whole 32-bit sample is pushed into the > > buffer > > as-is. > > > > Both of those issues cause the active channels to appear at the > > wrong > > offsets in the buffer. Fix the above by demuxing samples for active > > channels only. > > > > This has remained unnoticed, as all the consumers so far were only > > using > > channels 0 and 1, leaving them unaffected by changes introduced in > > this > > commit. > > ... > > > + u16 tdat[6]; > > + u32 val; > > + > > + memset(tdat, 0, ARRAY_SIZE(tdat)); > > Yeah, as LKP tells us this should be sizeof() instead of > ARRAY_SIZE(). Probably "u16 tdat[6] = { 0 };" would work too? Cheers, -Paul > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { > > + if (mask & 0x3) { > > (for the consistency it has to be GENMASK(), but see below) > > First of all, strictly speaking we should use the full mask without > limiting it to the 0 element in the array (I'm talking about > active_scan_mask). > > That said, we may actually use bit operations here in a better way, > i.e. > > unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - > 1); > > j = 0; > for_each_set_bit(i, active_scan_mask, ...) { > val = readl(...); > /* Two channels per sample. Demux active. */ > tdat[j++] = val >> (16 * (i % 2)); > } > > > + val = readl(adc->base + JZ_ADC_REG_ADTCH); > > + /* Two channels per sample. Demux active. > > */ > > + if (mask & BIT(0)) > > + tdat[i++] = val & 0xffff; > > + if (mask & BIT(1)) > > + tdat[i++] = val >> 16; > > + } > > } >
Hi Andy, Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit : > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek > > <contact@artur-rojek.eu> wrote: > > ... > > > > + u16 tdat[6]; > > > + u32 val; > > > + > > > + memset(tdat, 0, ARRAY_SIZE(tdat)); > > > > Yeah, as LKP tells us this should be sizeof() instead of > > ARRAY_SIZE(). > > > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { > > > + if (mask & 0x3) { > > > > (for the consistency it has to be GENMASK(), but see below) > > > > First of all, strictly speaking we should use the full mask without > > limiting it to the 0 element in the array (I'm talking about > > active_scan_mask). > > > > That said, we may actually use bit operations here in a better way, > > i.e. > > > > unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - > > 1); > > > > j = 0; > > for_each_set_bit(i, active_scan_mask, ...) { > > val = readl(...); > > /* Two channels per sample. Demux active. */ > > tdat[j++] = val >> (16 * (i % 2)); > > Alternatively > > /* Two channels per sample. Demux active. */ > if (i % 2) > tdat[j++] = upper_16_bits(val); > else > tdat[j++] = lower_16_bits(val); > > which may be better to read. It's not if/else though. You would check (i % 2) for the upper 16 bits, and (i % 1) for the lower 16 bits. Both can be valid at the same time. Cheers, -Paul > > > } > > > > > + val = readl(adc->base + > > > JZ_ADC_REG_ADTCH); > > > + /* Two channels per sample. Demux active. > > > */ > > > + if (mask & BIT(0)) > > > + tdat[i++] = val & 0xffff; > > > + if (mask & BIT(1)) > > > + tdat[i++] = val >> 16; > > > + } > > > } > >
On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit : > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek > > > <contact@artur-rojek.eu> wrote: ... > > > > + u16 tdat[6]; > > > > + u32 val; > > > > + > > > > + memset(tdat, 0, ARRAY_SIZE(tdat)); > > > > > > Yeah, as LKP tells us this should be sizeof() instead of > > > ARRAY_SIZE(). > > > > > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { > > > > + if (mask & 0x3) { > > > > > > (for the consistency it has to be GENMASK(), but see below) > > > > > > First of all, strictly speaking we should use the full mask without > > > limiting it to the 0 element in the array (I'm talking about > > > active_scan_mask). > > > > > > That said, we may actually use bit operations here in a better way, > > > i.e. > > > > > > unsigned long mask = active_scan_mask[0] & (active_scan_mask[0] - > > > 1); > > > > > > j = 0; > > > for_each_set_bit(i, active_scan_mask, ...) { > > > val = readl(...); > > > /* Two channels per sample. Demux active. */ > > > tdat[j++] = val >> (16 * (i % 2)); > > > > Alternatively > > > > /* Two channels per sample. Demux active. */ > > if (i % 2) > > tdat[j++] = upper_16_bits(val); > > else > > tdat[j++] = lower_16_bits(val); > > > > which may be better to read. > > It's not if/else though. You would check (i % 2) for the upper 16 bits, > and (i % 1) for the lower 16 bits. Both can be valid at the same time. Are you sure? Have you looked into the proposed code carefully? What probably can be done differently is the read part, that can be called once. But looking at it I'm not sure how it's supposed to work at all, since the address is always the same. How does the code and hardware are in sync with the channels? > > > } > > > > > > > + val = readl(adc->base + > > > > JZ_ADC_REG_ADTCH); > > > > + /* Two channels per sample. Demux active. > > > > */ > > > > + if (mask & BIT(0)) > > > > + tdat[i++] = val & 0xffff; > > > > + if (mask & BIT(1)) > > > > + tdat[i++] = val >> 16; > > > > + } > > > > }
On Mon, May 22, 2023 at 1:20 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 22 mai 2023 à 13:15 +0300, Andy Shevchenko a écrit : > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> > > wrote: ... > > > + memset(tdat, 0, ARRAY_SIZE(tdat)); > > > > Yeah, as LKP tells us this should be sizeof() instead of > > ARRAY_SIZE(). > > Probably "u16 tdat[6] = { 0 };" would work too? Without 0 also would work :-)
Hi Andy, Le lundi 22 mai 2023 à 14:05 +0300, Andy Shevchenko a écrit : > On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net> > wrote: > > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit : > > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek > > > > <contact@artur-rojek.eu> wrote: > > ... > > > > > > + u16 tdat[6]; > > > > > + u32 val; > > > > > + > > > > > + memset(tdat, 0, ARRAY_SIZE(tdat)); > > > > > > > > Yeah, as LKP tells us this should be sizeof() instead of > > > > ARRAY_SIZE(). > > > > > > > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) > > > > > { > > > > > + if (mask & 0x3) { > > > > > > > > (for the consistency it has to be GENMASK(), but see below) > > > > > > > > First of all, strictly speaking we should use the full mask > > > > without > > > > limiting it to the 0 element in the array (I'm talking about > > > > active_scan_mask). > > > > > > > > That said, we may actually use bit operations here in a better > > > > way, > > > > i.e. > > > > > > > > unsigned long mask = active_scan_mask[0] & > > > > (active_scan_mask[0] - > > > > 1); > > > > > > > > j = 0; > > > > for_each_set_bit(i, active_scan_mask, ...) { > > > > val = readl(...); > > > > /* Two channels per sample. Demux active. */ > > > > tdat[j++] = val >> (16 * (i % 2)); > > > > > > Alternatively > > > > > > /* Two channels per sample. Demux active. */ > > > if (i % 2) > > > tdat[j++] = upper_16_bits(val); > > > else > > > tdat[j++] = lower_16_bits(val); > > > > > > which may be better to read. > > > > It's not if/else though. You would check (i % 2) for the upper 16 > > bits, > > and (i % 1) for the lower 16 bits. Both can be valid at the same > > time. > > Are you sure? Have you looked into the proposed code carefully? Yes. I co-wrote the original code, I know what it's supposed to do. > > What probably can be done differently is the read part, that can be > called once. But looking at it I'm not sure how it's supposed to work > at all, since the address is always the same. How does the code and > hardware are in sync with the channels? It's a FIFO. Cheers, -Paul > > > > > } > > > > > > > > > + val = readl(adc->base + > > > > > JZ_ADC_REG_ADTCH); > > > > > + /* Two channels per sample. Demux > > > > > active. > > > > > */ > > > > > + if (mask & BIT(0)) > > > > > + tdat[i++] = val & 0xffff; > > > > > + if (mask & BIT(1)) > > > > > + tdat[i++] = val >> 16; > > > > > + } > > > > > } >
On Mon, May 22, 2023 at 2:35 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 22 mai 2023 à 14:05 +0300, Andy Shevchenko a écrit : > > On Mon, May 22, 2023 at 1:23 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > > > Le lundi 22 mai 2023 à 13:18 +0300, Andy Shevchenko a écrit : > > > > On Mon, May 22, 2023 at 1:15 PM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Mon, May 22, 2023 at 1:59 AM Artur Rojek > > > > > <contact@artur-rojek.eu> wrote: ... > > > > > > + u16 tdat[6]; > > > > > > + u32 val; > > > > > > + > > > > > > + memset(tdat, 0, ARRAY_SIZE(tdat)); > > > > > > > > > > Yeah, as LKP tells us this should be sizeof() instead of > > > > > ARRAY_SIZE(). > > > > > > > > > > > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) > > > > > > { > > > > > > + if (mask & 0x3) { > > > > > > > > > > (for the consistency it has to be GENMASK(), but see below) > > > > > > > > > > First of all, strictly speaking we should use the full mask > > > > > without > > > > > limiting it to the 0 element in the array (I'm talking about > > > > > active_scan_mask). > > > > > > > > > > That said, we may actually use bit operations here in a better > > > > > way, > > > > > i.e. > > > > > > > > > > unsigned long mask = active_scan_mask[0] & > > > > > (active_scan_mask[0] - > > > > > 1); > > > > > > > > > > j = 0; > > > > > for_each_set_bit(i, active_scan_mask, ...) { > > > > > val = readl(...); > > > > > /* Two channels per sample. Demux active. */ > > > > > tdat[j++] = val >> (16 * (i % 2)); > > > > > > > > Alternatively > > > > > > > > /* Two channels per sample. Demux active. */ > > > > if (i % 2) > > > > tdat[j++] = upper_16_bits(val); > > > > else > > > > tdat[j++] = lower_16_bits(val); > > > > > > > > which may be better to read. > > > > > > It's not if/else though. You would check (i % 2) for the upper 16 > > > bits, > > > and (i % 1) for the lower 16 bits. Both can be valid at the same > > > time. (i can't be two bits at the same time in my proposal) > > Are you sure? Have you looked into the proposed code carefully? > > Yes. I co-wrote the original code, I know what it's supposed to do. Yes, but I'm talking about my version to which you commented and I think it is the correct approach with 'else'. The problematic part in my proposal is FIFO reading. So, I have tried to come up with the working solution, but it seems it's too premature optimization here that's not needed and code, taking into account readability, will become a bit longer. That said, let's go with your version for now (implying the GENMASK() and upper/lower_16_bits() macros in use). > > What probably can be done differently is the read part, that can be > > called once. But looking at it I'm not sure how it's supposed to work > > at all, since the address is always the same. How does the code and > > hardware are in sync with the channels? > > It's a FIFO. A-ha. > > > > > } > > > > > > > > > > > + val = readl(adc->base + > > > > > > JZ_ADC_REG_ADTCH); > > > > > > + /* Two channels per sample. Demux > > > > > > active. > > > > > > */ > > > > > > + if (mask & BIT(0)) > > > > > > + tdat[i++] = val & 0xffff; > > > > > > + if (mask & BIT(1)) > > > > > > + tdat[i++] = val >> 16; > > > > > > + } > > > > > > } -- With Best Regards, Andy Shevchenko
On Mon, 22 May 2023 00:59:00 +0200 Artur Rojek <contact@artur-rojek.eu> wrote: > Consumers expect the buffer to only contain enabled channels. While > preparing the buffer, the driver makes two mistakes: > 1) It inserts empty data for disabled channels. > 2) Each ADC readout contains samples for two 16-bit channels. If either > of them is active, the whole 32-bit sample is pushed into the buffer > as-is. > > Both of those issues cause the active channels to appear at the wrong > offsets in the buffer. Fix the above by demuxing samples for active > channels only. > > This has remained unnoticed, as all the consumers so far were only using > channels 0 and 1, leaving them unaffected by changes introduced in this > commit. > > Signed-off-by: Artur Rojek <contact@artur-rojek.eu> > Tested-by: Paul Cercueil <paul@crapouillou.net> Lazy me suggestions that, as we didn't notice this before, clearly the vast majority of times the channels are both enabled. As such you 'could' just set available_scan_masks and burn the overhead of reading channels you don't want, instead letting the IIO core demux deal with the data movement if needed. > --- > > v2: - demux active channels from ADC readouts > - clarify in the commit description that this patch doesn't impact > existing consumers of this driver > > drivers/iio/adc/ingenic-adc.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c > index a7325dbbb99a..093710a7ad4c 100644 > --- a/drivers/iio/adc/ingenic-adc.c > +++ b/drivers/iio/adc/ingenic-adc.c > @@ -802,13 +802,19 @@ static irqreturn_t ingenic_adc_irq(int irq, void *data) > struct ingenic_adc *adc = iio_priv(iio_dev); > unsigned long mask = iio_dev->active_scan_mask[0]; > unsigned int i; > - u32 tdat[3]; > - > - for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) { > - if (mask & 0x3) > - tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH); > - else > - tdat[i] = 0; > + u16 tdat[6]; > + u32 val; > + > + memset(tdat, 0, ARRAY_SIZE(tdat)); > + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { > + if (mask & 0x3) { > + val = readl(adc->base + JZ_ADC_REG_ADTCH); > + /* Two channels per sample. Demux active. */ > + if (mask & BIT(0)) > + tdat[i++] = val & 0xffff; > + if (mask & BIT(1)) > + tdat[i++] = val >> 16; > + } > } > > iio_push_to_buffers(iio_dev, tdat);
diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c index a7325dbbb99a..093710a7ad4c 100644 --- a/drivers/iio/adc/ingenic-adc.c +++ b/drivers/iio/adc/ingenic-adc.c @@ -802,13 +802,19 @@ static irqreturn_t ingenic_adc_irq(int irq, void *data) struct ingenic_adc *adc = iio_priv(iio_dev); unsigned long mask = iio_dev->active_scan_mask[0]; unsigned int i; - u32 tdat[3]; - - for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) { - if (mask & 0x3) - tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH); - else - tdat[i] = 0; + u16 tdat[6]; + u32 val; + + memset(tdat, 0, ARRAY_SIZE(tdat)); + for (i = 0; mask && i < ARRAY_SIZE(tdat); mask >>= 2) { + if (mask & 0x3) { + val = readl(adc->base + JZ_ADC_REG_ADTCH); + /* Two channels per sample. Demux active. */ + if (mask & BIT(0)) + tdat[i++] = val & 0xffff; + if (mask & BIT(1)) + tdat[i++] = val >> 16; + } } iio_push_to_buffers(iio_dev, tdat);