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
This is useful for consumers that wish to parse raw buffer data.
Tested-by: Paul Cercueil <[email protected]>
Signed-off-by: Artur Rojek <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Artur Rojek <[email protected]>
---
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 <[email protected]>");
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
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
>
On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <[email protected]> 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
On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <[email protected]> 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
On Fri, Aug 19, 2022 at 1:33 PM Artur Rojek <[email protected]> wrote:
> On 2022-08-19 10:17, Andy Shevchenko wrote:
> > On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <[email protected]>
> > 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
On 2022-08-19 10:17, Andy Shevchenko wrote:
> On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <[email protected]>
> 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?
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
>>
On Fri, 19 Aug 2022 11:14:04 +0300
Andy Shevchenko <[email protected]> wrote:
> On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <[email protected]> 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
On Wed, 17 Aug 2022 12:56:42 +0200
Artur Rojek <[email protected]> wrote:
> This is useful for consumers that wish to parse raw buffer data.
>
> Tested-by: Paul Cercueil <[email protected]>
> Signed-off-by: Artur Rojek <[email protected]>
> ---
> 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);
On Wed, 17 Aug 2022 12:56:41 +0200
Artur Rojek <[email protected]> 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 <[email protected]>
> Signed-off-by: Artur Rojek <[email protected]>
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 <[email protected]>");
> 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.