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

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

[Artur Rojek]

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

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

[PATCH 1/4] 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 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(-)

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

[PATCH 2/4] iio: add iio_channel_cb_get_iio_buffer helper

[Artur Rojek]

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

[PATCH 3/4] iio: add helper function for reading channel offset in buffer

[Artur Rojek]

This is useful for consumers that wish to parse raw buffer data.

Tested-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/iio/industrialio-buffer.c | 28 ++++++++++++++++++++++++++++
 include/linux/iio/buffer.h        |  4 ++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 228598b82a2f..cf23736610d9 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -691,6 +691,34 @@ static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev,
 	return bytes;
 }
 
+int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      struct iio_buffer *buffer)
+{
+	int length, offset = 0;
+	unsigned int si;
+
+	if (chan->scan_index < 0 ||
+	    !test_bit(chan->scan_index, buffer->scan_mask)) {
+		return -EINVAL;
+	}
+
+	for (si = 0; si < chan->scan_index; ++si) {
+		if (!test_bit(si, buffer->scan_mask))
+			continue;
+
+		length = iio_storage_bytes_for_si(indio_dev, si);
+
+		/* Account for channel alignment. */
+		if (offset % length)
+			offset += length - (offset % length);
+		offset += length;
+	}
+
+	return offset;
+}
+EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer);
+
 static unsigned int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index 418b1307d3f2..b1db74772e77 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -16,6 +16,10 @@ enum iio_buffer_direction {
 	IIO_BUFFER_DIRECTION_OUT,
 };
 
+int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      struct iio_buffer *buffer);
+
 int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
 
 int iio_pop_from_buffer(struct iio_buffer *buffer, void *data);
-- 
2.37.2

[PATCH 4/4] input: joystick: Fix buffer data parsing

[Artur Rojek]

Don't try to access buffer data of a channel by its scan index. Instead,
use the newly introduced `iio_find_channel_offset_in_buffer` to get the
correct data offset.

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

Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick driver.")
Reported-by: Chris Morgan <macromorgan@hotmail.com>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/input/joystick/adc-joystick.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index c0deff5d4282..aed853ebe1d1 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -6,6 +6,7 @@
 #include <linux/ctype.h>
 #include <linux/input.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/consumer.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -46,36 +47,43 @@ static void adc_joystick_poll(struct input_dev *input)
 static int adc_joystick_handle(const void *data, void *private)
 {
 	struct adc_joystick *joy = private;
+	struct iio_buffer *buffer;
 	enum iio_endian endianness;
-	int bytes, msb, val, idx, i;
-	const u16 *data_u16;
+	int bytes, msb, val, off;
+	const u8 *chan_data;
+	unsigned int i;
 	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';
+		buffer = iio_channel_cb_get_iio_buffer(joy->buffer);
+		off = iio_find_channel_offset_in_buffer(joy->chans[i].indio_dev,
+							joy->chans[i].channel,
+							buffer);
+		if (off < 0)
+			return off;
+
+		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;
-- 
2.37.2

Re: [PATCH 0/4] iio/adc-joystick: buffer data parsing fixes

[Chris Morgan]

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
> 

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: add iio_channel_cb_get_iio_buffer helper

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: add helper function for reading channel offset in buffer

[Andy Shevchenko]

On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> This is useful for consumers that wish to parse raw buffer data.

...

> +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev,
> +                                     const struct iio_chan_spec *chan,
> +                                     struct iio_buffer *buffer)
> +{
> +       int length, offset = 0;
> +       unsigned int si;
> +
> +       if (chan->scan_index < 0 ||
> +           !test_bit(chan->scan_index, buffer->scan_mask)) {
> +               return -EINVAL;
> +       }

Have you run checkpatch? The {} are redundant. But personally I would
split this into two separate conditionals.

> +       for (si = 0; si < chan->scan_index; ++si) {

Just a side crying: where did you, people, get this pre-increment pattern from?!

> +               if (!test_bit(si, buffer->scan_mask))
> +                       continue;

NIH for_each_set_bit()

> +               length = iio_storage_bytes_for_si(indio_dev, si);
> +
> +               /* Account for channel alignment. */
> +               if (offset % length)
> +                       offset += length - (offset % length);
> +               offset += length;
> +       }
> +
> +       return offset;
> +}
> +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer);

Same Q as per previous patch: IIO namespace?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Don't try to access buffer data of a channel by its scan index. Instead,
> use the newly introduced `iio_find_channel_offset_in_buffer` to get the
> correct data offset.
>
> The scan index of a channel does not represent its position in a buffer,
> as the buffer will contain data for enabled channels only, affecting
> data offsets and alignment.

> Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick driver.")

You may not use Fixes here because it has dependencies. The possible
solutions are:
1/
a) create a real fix for the existing code;
b) refactor it.

2/
 put the Fixes tag to all dependencies (which is questionable).


> Reported-by: Chris Morgan <macromorgan@hotmail.com>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>

Try to keep tags in chronological order, I do not believe the change
may be tested before it has been created.

-- 
With Best Regards,
Andy Shevchenko

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

[Paul Cercueil]

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

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

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


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: add helper function for reading channel offset in buffer

[Artur Rojek]

On 2022-08-19 10:17, Andy Shevchenko wrote:
> On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> 
> wrote:
>> 
>> This is useful for consumers that wish to parse raw buffer data.
> 
> ...
> 
>> +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev,
>> +                                     const struct iio_chan_spec 
>> *chan,
>> +                                     struct iio_buffer *buffer)
>> +{
>> +       int length, offset = 0;
>> +       unsigned int si;
>> +
>> +       if (chan->scan_index < 0 ||
>> +           !test_bit(chan->scan_index, buffer->scan_mask)) {
>> +               return -EINVAL;
>> +       }
> 
> Have you run checkpatch? The {} are redundant. But personally I would
> split this into two separate conditionals.
I did run checkpatch on it - all patches were ready for submission.
I don't find the {} redundant for multi-line statements, like this one,
and I personally prefer to check conditions that return the same error
type together.
> 
>> +       for (si = 0; si < chan->scan_index; ++si) {
> 
> Just a side crying: where did you, people, get this pre-increment 
> pattern from?!
> 
>> +               if (!test_bit(si, buffer->scan_mask))
>> +                       continue;
> 
> NIH for_each_set_bit()
> 
>> +               length = iio_storage_bytes_for_si(indio_dev, si);
>> +
>> +               /* Account for channel alignment. */
>> +               if (offset % length)
>> +                       offset += length - (offset % length);
>> +               offset += length;
>> +       }
>> +
>> +       return offset;
>> +}
>> +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer);
> 
> Same Q as per previous patch: IIO namespace?

Re: [PATCH 0/4] iio/adc-joystick: buffer data parsing fixes

[Artur Rojek]

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

Re: [PATCH 3/4] iio: add helper function for reading channel offset in buffer

[Andy Shevchenko]

On Fri, Aug 19, 2022 at 1:33 PM Artur Rojek <contact@artur-rojek.eu> wrote:
> On 2022-08-19 10:17, Andy Shevchenko wrote:
> > On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu>
> > wrote:

...

> >> +       if (chan->scan_index < 0 ||
> >> +           !test_bit(chan->scan_index, buffer->scan_mask)) {
> >> +               return -EINVAL;
> >> +       }
> >
> > Have you run checkpatch? The {} are redundant. But personally I would
> > split this into two separate conditionals.
> I did run checkpatch on it - all patches were ready for submission.
> I don't find the {} redundant for multi-line statements, like this one,

This is a one-line conditional. So, *unlike* this one.

> and I personally prefer to check conditions that return the same error
> type together.

I see that the maintainer's input is needed here, because even if the
error code is the same, the semantics are different and I consider
that to prevail on the combining.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: add iio_channel_cb_get_iio_buffer helper

[Jonathan Cameron]

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

Re: [PATCH 2/4] iio: add iio_channel_cb_get_iio_buffer helper

[Jonathan Cameron]

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.

Re: [PATCH 3/4] iio: add helper function for reading channel offset in buffer

[Jonathan Cameron]

On Wed, 17 Aug 2022 12:56:42 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> This is useful for consumers that wish to parse raw buffer data.
> 
> Tested-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  drivers/iio/industrialio-buffer.c | 28 ++++++++++++++++++++++++++++
>  include/linux/iio/buffer.h        |  4 ++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 228598b82a2f..cf23736610d9 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -691,6 +691,34 @@ static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev,
>  	return bytes;
>  }
>  
> +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      struct iio_buffer *buffer)
> +{
> +	int length, offset = 0;
> +	unsigned int si;
> +
> +	if (chan->scan_index < 0 ||
> +	    !test_bit(chan->scan_index, buffer->scan_mask)) {
> +		return -EINVAL;
> +	}
> +
> +	for (si = 0; si < chan->scan_index; ++si) {
> +		if (!test_bit(si, buffer->scan_mask))

You are walking channels that the consumer should not even know about
here.

I think you can establish the same easily enough from the channels it
does know about.  It would be fiddly if you had lots of channels (as
you might be best off sorting them) but for small numbers of channels
loop over the array to find lowest scan_index - compute offset due
to that then move on to next one etc until you reach the scan index
of the channel you want.


> +			continue;
> +
> +		length = iio_storage_bytes_for_si(indio_dev, si);
> +
> +		/* Account for channel alignment. */
> +		if (offset % length)
> +			offset += length - (offset % length);
> +		offset += length;
> +	}
> +
> +	return offset;
> +}
> +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer);
> +
>  static unsigned int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index 418b1307d3f2..b1db74772e77 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -16,6 +16,10 @@ enum iio_buffer_direction {
>  	IIO_BUFFER_DIRECTION_OUT,
>  };
>  
> +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      struct iio_buffer *buffer);
> +
>  int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
>  
>  int iio_pop_from_buffer(struct iio_buffer *buffer, void *data);

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

[Jonathan Cameron]

On Wed, 17 Aug 2022 12:56:43 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> Don't try to access buffer data of a channel by its scan index. Instead,
> use the newly introduced `iio_find_channel_offset_in_buffer` to get the
> correct data offset.
> 
> The scan index of a channel does not represent its position in a buffer,
> as the buffer will contain data for enabled channels only, affecting
> data offsets and alignment.
> 
> Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick driver.")
> Reported-by: Chris Morgan <macromorgan@hotmail.com>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  drivers/input/joystick/adc-joystick.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
> index c0deff5d4282..aed853ebe1d1 100644
> --- a/drivers/input/joystick/adc-joystick.c
> +++ b/drivers/input/joystick/adc-joystick.c
> @@ -6,6 +6,7 @@
>  #include <linux/ctype.h>
>  #include <linux/input.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -46,36 +47,43 @@ static void adc_joystick_poll(struct input_dev *input)
>  static int adc_joystick_handle(const void *data, void *private)
>  {
>  	struct adc_joystick *joy = private;
> +	struct iio_buffer *buffer;
>  	enum iio_endian endianness;
> -	int bytes, msb, val, idx, i;
> -	const u16 *data_u16;
> +	int bytes, msb, val, off;
> +	const u8 *chan_data;
> +	unsigned int i;
>  	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';
> +		buffer = iio_channel_cb_get_iio_buffer(joy->buffer);
> +		off = iio_find_channel_offset_in_buffer(joy->chans[i].indio_dev,
> +							joy->chans[i].channel,
> +							buffer);

With this call replaced with one that instead uses

		off = iio_find_channel_offset_in_buffer(joy->chans, i);

which I'm fairly sure is enough via the info in chans[x]->channel to establish this offset.

All is good, though you should probably cache it as doing that maths every
time seems excessive.


> +		if (off < 0)
> +			return off;
> +
> +		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);

I obviously missed this previously but these are aligned so we don't need the
unaligned form.

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

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

[Paul Cercueil]

Hi Jonathan,

Le ven., août 19 2022 at 18:53:39 +0100, Jonathan Cameron 
<jic23@kernel.org> a écrit :
> On Wed, 17 Aug 2022 12:56:43 +0200
> Artur Rojek <contact@artur-rojek.eu> wrote:
> 
>>  Don't try to access buffer data of a channel by its scan index. 
>> Instead,
>>  use the newly introduced `iio_find_channel_offset_in_buffer` to get 
>> the
>>  correct data offset.
>> 
>>  The scan index of a channel does not represent its position in a 
>> buffer,
>>  as the buffer will contain data for enabled channels only, affecting
>>  data offsets and alignment.
>> 
>>  Fixes: 2c2b364fddd5 ("Input: joystick - add ADC attached joystick 
>> driver.")
>>  Reported-by: Chris Morgan <macromorgan@hotmail.com>
>>  Tested-by: Paul Cercueil <paul@crapouillou.net>
>>  Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>>   drivers/input/joystick/adc-joystick.c | 26 
>> +++++++++++++++++---------
>>   1 file changed, 17 insertions(+), 9 deletions(-)
>> 
>>  diff --git a/drivers/input/joystick/adc-joystick.c 
>> b/drivers/input/joystick/adc-joystick.c
>>  index c0deff5d4282..aed853ebe1d1 100644
>>  --- a/drivers/input/joystick/adc-joystick.c
>>  +++ b/drivers/input/joystick/adc-joystick.c
>>  @@ -6,6 +6,7 @@
>>   #include <linux/ctype.h>
>>   #include <linux/input.h>
>>   #include <linux/iio/iio.h>
>>  +#include <linux/iio/buffer.h>
>>   #include <linux/iio/consumer.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>>  @@ -46,36 +47,43 @@ static void adc_joystick_poll(struct input_dev 
>> *input)
>>   static int adc_joystick_handle(const void *data, void *private)
>>   {
>>   	struct adc_joystick *joy = private;
>>  +	struct iio_buffer *buffer;
>>   	enum iio_endian endianness;
>>  -	int bytes, msb, val, idx, i;
>>  -	const u16 *data_u16;
>>  +	int bytes, msb, val, off;
>>  +	const u8 *chan_data;
>>  +	unsigned int i;
>>   	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';
>>  +		buffer = iio_channel_cb_get_iio_buffer(joy->buffer);
>>  +		off = iio_find_channel_offset_in_buffer(joy->chans[i].indio_dev,
>>  +							joy->chans[i].channel,
>>  +							buffer);
> 
> With this call replaced with one that instead uses
> 
> 		off = iio_find_channel_offset_in_buffer(joy->chans, i);
> 
> which I'm fairly sure is enough via the info in chans[x]->channel to 
> establish this offset.
> 
> All is good, though you should probably cache it as doing that maths 
> every
> time seems excessive.
> 
> 
>>  +		if (off < 0)
>>  +			return off;
>>  +
>>  +		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);
> 
> I obviously missed this previously but these are aligned so we don't 
> need the
> unaligned form.

Yes, the comment above says that it's used to hide type casting.

Cheers,
-Paul

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

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

[Jonathan Cameron]

> >>   		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);  
> > 
> > I obviously missed this previously but these are aligned so we don't 
> > need the
> > unaligned form.  
> 
> Yes, the comment above says that it's used to hide type casting.
oops :)

Thanks for pointing out my lack of observation!

Jonathan