[PATCH v2 0/2] iio/adc-joystick: buffer data parsing fixes

[PATCH v2 0/2] iio/adc-joystick: buffer data parsing fixes

[Artur Rojek]

Hi all,

this is a long overdue v2 for fixes [1] related to adc-joystick channel
parsing.

Patch [1/2] now also addresses a case where a readout of ADC data
containing samples for two channels, where only one channel is active,
would push the data into a buffer without discarding the disabled
channel first.

Patch [2/2] addresses concerns about exposure to channels the clients
shouldn't know about. The IIO helpers from v1 are gone and the offsets
are now calculated based only on the channels adc-joystick has access
to.

Tested on GCW Zero (by me) and on Anbernic RG350 (by Paul).

[1] https://lore.kernel.org/all/20220817105643.95710-1-contact@artur-rojek.eu/ 

Cheers,
Artur

Artur Rojek (2):
  iio/adc: ingenic: Fix channel offsets in buffer
  input: joystick: Fix buffer data parsing

 drivers/iio/adc/ingenic-adc.c         |  20 +++--
 drivers/input/joystick/adc-joystick.c | 102 +++++++++++++++++++++++---
 2 files changed, 103 insertions(+), 19 deletions(-)

-- 
2.40.1

[PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Artur Rojek]

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

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

[PATCH v2 2/2] input: joystick: Fix buffer data parsing

[Artur Rojek]

Don't try to access buffer data of a channel by its scan index. Instead,
calculate its offset in the buffer.

This is necessary, as the scan index of a channel does not represent its
position in a buffer - the buffer will contain data for enabled channels
only, affecting data offsets and alignment.

While at it, also fix minor style issue in probe.

Reported-by: Chris Morgan <macromorgan@hotmail.com>
Closes: https://lore.kernel.org/linux-input/20220408212857.9583-1-macroalpha82@gmail.com/
Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
---

v2: - provide new implementation for calculating channel offsets 
    - cache the resulting offsets
    - fix minor style issue in probe
    - drop the "Fixes" tag

 drivers/input/joystick/adc-joystick.c | 102 +++++++++++++++++++++++---
 1 file changed, 90 insertions(+), 12 deletions(-)

diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index c0deff5d4282..2f9f0cae8f95 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/sort.h>
 
 #include <asm/unaligned.h>
 
@@ -25,6 +26,7 @@ struct adc_joystick {
 	struct iio_cb_buffer *buffer;
 	struct adc_joystick_axis *axes;
 	struct iio_channel *chans;
+	int *offsets;
 	int num_chans;
 	bool polled;
 };
@@ -47,35 +49,38 @@ static int adc_joystick_handle(const void *data, void *private)
 {
 	struct adc_joystick *joy = private;
 	enum iio_endian endianness;
-	int bytes, msb, val, idx, i;
-	const u16 *data_u16;
+	int bytes, msb, val, off, i;
+	const u8 *chan_data;
 	bool sign;
 
 	bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
 
 	for (i = 0; i < joy->num_chans; ++i) {
-		idx = joy->chans[i].channel->scan_index;
 		endianness = joy->chans[i].channel->scan_type.endianness;
 		msb = joy->chans[i].channel->scan_type.realbits - 1;
 		sign = tolower(joy->chans[i].channel->scan_type.sign) == 's';
+		off = joy->offsets[i];
+
+		if (off < 0)
+			return -EINVAL;
+
+		chan_data = (const u8 *)data + off;
 
 		switch (bytes) {
 		case 1:
-			val = ((const u8 *)data)[idx];
+			val = *chan_data;
 			break;
 		case 2:
-			data_u16 = (const u16 *)data + idx;
-
 			/*
 			 * Data is aligned to the sample size by IIO core.
 			 * Call `get_unaligned_xe16` to hide type casting.
 			 */
 			if (endianness == IIO_BE)
-				val = get_unaligned_be16(data_u16);
+				val = get_unaligned_be16(chan_data);
 			else if (endianness == IIO_LE)
-				val = get_unaligned_le16(data_u16);
+				val = get_unaligned_le16(chan_data);
 			else /* IIO_CPU */
-				val = *data_u16;
+				val = *(const u16 *)chan_data;
 			break;
 		default:
 			return -EINVAL;
@@ -94,6 +99,69 @@ static int adc_joystick_handle(const void *data, void *private)
 	return 0;
 }
 
+static int adc_joystick_si_cmp(const void *a, const void *b, const void *priv)
+{
+	const struct iio_channel *chans = priv;
+
+	return chans[*(int *)a].channel->scan_index -
+	       chans[*(int *)b].channel->scan_index;
+}
+
+static int *adc_joystick_get_chan_offsets(struct iio_channel *chans, int count)
+{
+	struct iio_dev *indio_dev = chans[0].indio_dev;
+	const struct iio_chan_spec *ch;
+	int *offsets, *si_order;
+	int idx, i, si, length, offset = 0;
+
+	offsets = kmalloc_array(count, sizeof(int), GFP_KERNEL);
+	if (!offsets)
+		return ERR_PTR(-ENOMEM);
+
+	si_order = kmalloc_array(count, sizeof(int), GFP_KERNEL);
+	if (!si_order) {
+		kfree(offsets);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0; i < count; ++i)
+		si_order[i] = i;
+	/* Channels in buffer are ordered by scan index. Sort to match that. */
+	sort_r(si_order, count, sizeof(int), adc_joystick_si_cmp, NULL, chans);
+
+	for (i = 0; i < count; ++i) {
+		idx = si_order[i];
+		ch = chans[idx].channel;
+		si = ch->scan_index;
+
+		if (si < 0 || !test_bit(si, indio_dev->active_scan_mask)) {
+			offsets[idx] = -1;
+			continue;
+		}
+
+		/* Channels sharing scan indices also share the samples. */
+		if (idx > 0 && si == chans[idx - 1].channel->scan_index) {
+			offsets[idx] = offsets[idx - 1];
+			continue;
+		}
+
+		offsets[idx] = offset;
+
+		length = ch->scan_type.storagebits / 8;
+		if (ch->scan_type.repeat > 1)
+			length *= ch->scan_type.repeat;
+
+		/* Account for channel alignment. */
+		if (offset % length)
+			offset += length - (offset % length);
+		offset += length;
+	}
+
+	kfree(si_order);
+
+	return offsets;
+}
+
 static int adc_joystick_open(struct input_dev *dev)
 {
 	struct adc_joystick *joy = input_get_drvdata(dev);
@@ -101,10 +169,19 @@ static int adc_joystick_open(struct input_dev *dev)
 	int ret;
 
 	ret = iio_channel_start_all_cb(joy->buffer);
-	if (ret)
+	if (ret) {
 		dev_err(devp, "Unable to start callback buffer: %d\n", ret);
+		return ret;
+	}
 
-	return ret;
+	joy->offsets = adc_joystick_get_chan_offsets(joy->chans,
+						     joy->num_chans);
+	if (IS_ERR(joy->offsets)) {
+		dev_err(devp, "Unable to allocate channel offsets\n");
+		return PTR_ERR(joy->offsets);
+	}
+
+	return 0;
 }
 
 static void adc_joystick_close(struct input_dev *dev)
@@ -112,6 +189,7 @@ static void adc_joystick_close(struct input_dev *dev)
 	struct adc_joystick *joy = input_get_drvdata(dev);
 
 	iio_channel_stop_all_cb(joy->buffer);
+	kfree(joy->offsets);
 }
 
 static void adc_joystick_cleanup(void *data)
@@ -269,7 +347,7 @@ static int adc_joystick_probe(struct platform_device *pdev)
 
 		error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
 						 joy->buffer);
-		if (error)  {
+		if (error) {
 			dev_err(dev, "Unable to add action\n");
 			return error;
 		}
-- 
2.40.1

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Andy Shevchenko]

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;
> +               }
>         }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Andy Shevchenko]

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;
> > +               }
> >         }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Paul Cercueil]

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;
> > +               }
> >         }
> 

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Paul Cercueil]

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;
> > > +               }
> > >         }
> 
> 

Re: [PATCH v2 2/2] input: joystick: Fix buffer data parsing

[Andy Shevchenko]

On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Don't try to access buffer data of a channel by its scan index. Instead,
> calculate its offset in the buffer.
>
> This is necessary, as the scan index of a channel does not represent its
> position in a buffer - the buffer will contain data for enabled channels
> only, affecting data offsets and alignment.
>
> While at it, also fix minor style issue in probe.

a minor
the probe

> Reported-by: Chris Morgan <macromorgan@hotmail.com>
> Closes: https://lore.kernel.org/linux-input/20220408212857.9583-1-macroalpha82@gmail.com/

What is this tag? Anything documented? Otherwise use BugLink or Link.

...

>         struct adc_joystick_axis *axes;
>         struct iio_channel *chans;
> +       int *offsets;

Why not unsigned? I.o.w. is there any meaning for negative values?

>         int num_chans;
>         bool polled;

...

> +               off = joy->offsets[i];

> +

Move this blank line to be before the previous line.

> +               if (off < 0)
> +                       return -EINVAL;

...

>                 case 1:
> -                       val = ((const u8 *)data)[idx];
> +                       val = *chan_data;

Might be also

   get_unaligned((const u8 *)chan_data);

And with all this (see below) the chan_data actually can be declared
as const void *.

>                         break;
>                 case 2:
> -                       data_u16 = (const u16 *)data + idx;
> -
>                         /*
>                          * Data is aligned to the sample size by IIO core.
>                          * Call `get_unaligned_xe16` to hide type casting.
>                          */
>                         if (endianness == IIO_BE)
> -                               val = get_unaligned_be16(data_u16);
> +                               val = get_unaligned_be16(chan_data);
>                         else if (endianness == IIO_LE)
> -                               val = get_unaligned_le16(data_u16);
> +                               val = get_unaligned_le16(chan_data);
>                         else /* IIO_CPU */
> -                               val = *data_u16;
> +                               val = *(const u16 *)chan_data;

This probably needs to be

   get_unaligned((const u16 *)chan_data);

for the sake of consistency with the above.

>                         break;
>                 default:
>                         return -EINVAL;

...

> +static int adc_joystick_si_cmp(const void *a, const void *b, const void *priv)
> +{
> +       const struct iio_channel *chans = priv;
> +
> +       return chans[*(int *)a].channel->scan_index -
> +              chans[*(int *)b].channel->scan_index;

Discarding const?

> +}

...

> +       offsets = kmalloc_array(count, sizeof(int), GFP_KERNEL);

sizeof(*offsets)

> +       if (!offsets)
> +               return ERR_PTR(-ENOMEM);

...

> +       si_order = kmalloc_array(count, sizeof(int), GFP_KERNEL);

sizeof(*si_order)

> +       if (!si_order) {
> +               kfree(offsets);
> +               return ERR_PTR(-ENOMEM);
> +       }

...

> +       /* Channels in buffer are ordered by scan index. Sort to match that. */

the buffer

> +       sort_r(si_order, count, sizeof(int), adc_joystick_si_cmp, NULL, chans);

sizeof(*si_order) ?

sizeof(int) is a bit odd, the above will tell better without even
knowing the sort_r() parameters what it is about.

...

> +       for (i = 0; i < count; ++i) {
> +               idx = si_order[i];
> +               ch = chans[idx].channel;
> +               si = ch->scan_index;
> +
> +               if (si < 0 || !test_bit(si, indio_dev->active_scan_mask)) {
> +                       offsets[idx] = -1;
> +                       continue;
> +               }
> +
> +               /* Channels sharing scan indices also share the samples. */
> +               if (idx > 0 && si == chans[idx - 1].channel->scan_index) {
> +                       offsets[idx] = offsets[idx - 1];
> +                       continue;
> +               }
> +
> +               offsets[idx] = offset;

> +               length = ch->scan_type.storagebits / 8;

BITS_PER_BYTE ?

> +               if (ch->scan_type.repeat > 1)
> +                       length *= ch->scan_type.repeat;

> +               /* Account for channel alignment. */
> +               if (offset % length)
> +                       offset += length - (offset % length);

Would one of ALIGN() / rounddown / etc work here?

> +               offset += length;
> +       }

...

> +       joy->offsets = adc_joystick_get_chan_offsets(joy->chans,
> +                                                    joy->num_chans);
> +       if (IS_ERR(joy->offsets)) {
> +               dev_err(devp, "Unable to allocate channel offsets\n");

> +               return PTR_ERR(joy->offsets);
> +       }
> +
> +       return 0;

return PTR_ERR_OR_ZERO() ?

...

>                 error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
>                                                  joy->buffer);
> -               if (error)  {
> +               if (error) {
>                         dev_err(dev, "Unable to add action\n");
>                         return error;
>                 }

Unrelated change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Andy Shevchenko]

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;
> > > > +               }
> > > >         }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Paul Cercueil]

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;
> > > > > +               }
> > > > >         }
> 

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Andy Shevchenko]

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

Re: [PATCH v2 1/2] iio/adc: ingenic: Fix channel offsets in buffer

[Jonathan Cameron]

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