On Sun, 10 Apr 2022 16:58:48 +0200
Artur Rojek <contact@xxxxxxxxxxxxxx> wrote:
> On 2022-04-10 03:39, Chris Morgan wrote:
> > On Sat, Apr 09, 2022 at 12:08:57PM +0200, Artur Rojek wrote:
> >> Hi Chris & Dmitry,
> >>
> >> On 2022-04-09 04:07, Dmitry Torokhov wrote:
> >> > Hi Chris,
> >> >
> >> > On Fri, Apr 08, 2022 at 04:28:57PM -0500, Chris Morgan wrote:
> >> > > From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> >> > >
> >> > > For my Odroid Go Advance I noticed that the adc-joystick driver was
> >> > > only reporting the y channel and on the x axis. After debugging, I
> >> > > found that the driver was trying to read values from channels 0 and
> >> > > 1 even though my device is using channels 1 and 2. By changing the
> >> > > code
> >> > > to use the axis index instead of the scan index when unpacking the
> >> > > data
> >> > > from the buffer, the joystick begins working as expected.
> >> >
> >> > This sounds like some sort of misconfiguration, as your change
> >> > effectively removes the ability of using just some ADC channels for
> >> > joystick functionality...
> >>
> >> I agree, this sounds like either a case of misconfiguration, or an
> >> issue in
> >> the ADC driver that this device is using.
> >> The axis index corresponds to the iio channel associated with the
> >> joystick,
> >> but NOT to the order at which data is sampled by ADC.
> >> That's why each channel has a `scan_index` field. It sounds like in
> >> Chris'
> >> case the channels have wrong scan indices.
> >> I'd start by verifying that in the ADC driver that is being used.
> >>
> >> In any case, this patch is wrong and removes functionality that
> >> existing
> >> devices depend on.
> >
> > I appreciate the feedback. If this driver is working as expected then
> > that means the issue I am experiencing is further up the stack. Based
> > on troubleshooting by getting the raw data that the rockchip-saradc
> > driver was putting into the triggered buffer and seeing what the
> > adc-joystick saw coming out of the triggered buffer I wonder if the
> > issue is with the rockchip-saradc driver? I noticed that the buffer
> > pushed by the driver's trigger handler would only (appear to) send the
> > channels that I was requesting data for. So basically the data buffer
> > would have the correct values in [0] and [1], but the adc-joystick
> > driver by using the idx would fetch values from [1] for x (which has
> > the y axis data) and [2] for y (which would have arbitrary data in
> > it, usually something around 65406 or so).
> >
> > Do you think I should start looking at the rockchip-saradc driver then?
> > Should the saradc be putting stuff in the buffer for every channel with
> > empty data for channels that aren't to be reported?
> >
> > Thank you.
>
> Chris,
>
> I analyzed the IIO core code some more and I think you are correct in
> your assessment.
> The data buffer that `adc-joystick` receives will be the length of all
> the *active* channels combined.
> That would mean scan index specifies the order of the channels by which
> they appear in the buffer, NOT their offsets in it.
>
> That said, we can't rely on channel order from `joy->chans =
> devm_iio_channel_get_all(dev);`,
> as channels might have out-of-order scan indices and thus this sequence
> can't be used to iterate the buffer.
> I think the best approach would be to add an IIO helper to find a
> channel offset in a buffer.
> Say, something like this:
>
> ```
> --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> @@ -151,6 +151,27 @@ struct iio_dev
> }
> EXPORT_SYMBOL_GPL(iio_channel_cb_get_iio_dev);
>
> +int iio_find_channel_offset_in_buffer(const struct iio_channel
> *channel,
> + struct iio_cb_buffer *buffer)
> +{
> + const struct iio_chan_spec *chan = channel->channel;
> + struct iio_dev *indio_dev = channel->indio_dev;
> + struct iio_buffer *buf = &buffer->buffer;
> + int ind, i = 0;
> +
> + if (chan->scan_index < 0)
> + return -EINVAL;
> +
> + for_each_set_bit(ind, buf->scan_mask, indio_dev->masklength) {
> + if (ind == chan->scan_index)
> + return i;
> + ++i;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer);
Almost. You need to take into account the size of each
channel and the alignment rules as well (everything is naturally
aligned) which will unfortunately require searching the channel
list for each channel with a scan_index below this one.
Useful function to hav so I'm in favour of adding something like
this it will just need to also take the iio_dev to get access to the
sizes of other channels.
Note similar code occurs in iio_buffer_update_demux() (though what you need
here is thankfully rather simpler).
We have iio_storage_bytes_for_si() to make things a bit simpler as it'll
do the reverse lookups for you.
You'll need to export some of the utility functions.
However, I'd put this in a more generic location as it's potentially useful
for cases other than callback buffers and that should reduce what you need
to export. Probably put it in industrialio-buffer.c / iio/buffer.h
> +
> ```
>
> ...and then the only change in `adc-joystick` has to be:
>
> ```
> --- a/drivers/input/joystick/adc-joystick.c
> +++ b/drivers/input/joystick/adc-joystick.c
> @@ -39,10 +39,13 @@ static int adc_joystick_handle(const void *data,
> void *private)
> 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';
> + idx = iio_find_channel_offset_in_buffer(&joy->chans[i],
> + joy->buffer);
> + if (idx < 0)
> + return idx;
>
> switch (bytes) {
> case 1:
> ```
>
> On a side note, this potentially uncovered an issue in an unrelated
> `ingenic-adc` driver,
> where data pushed into the buffer is always the size of all the
> available channels, not just active ones.
That would be fine, if it were also setting available_scan_masks as then
the IIO core would repack only the requested channels (which is
what that update_demux mentioned earlier sets up).
However, given the driver is doing readl() only for the channels
that are enabled, it probably makes more sense to pack them correctly.
Jonathan
> I was using that driver while writing `adc-joystick`, which explains why
> I never encountered your problem.
>
> With all that said, let's wait for Jonathan to speak out before we
> proceed with v2.
>
> Cheers,
> Artur
>
> >
> >>
> >> Cheers,
> >> Artur
> >>
> >> >
> >> > Let's add Jonathan and Arthur for their take on this.
> >> >
> >> > >
> >> > > Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> >> > > ---
> >> > > drivers/input/joystick/adc-joystick.c | 7 +++----
> >> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/input/joystick/adc-joystick.c
> >> > > b/drivers/input/joystick/adc-joystick.c
> >> > > index 78ebca7d400a..fe3bbd0d4566 100644
> >> > > --- a/drivers/input/joystick/adc-joystick.c
> >> > > +++ b/drivers/input/joystick/adc-joystick.c
> >> > > @@ -32,24 +32,23 @@ 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;
> >> > > + int bytes, msb, val, i;
> >> > > const u16 *data_u16;
> >> > > 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';
> >> > >
> >> > > switch (bytes) {
> >> > > case 1:
> >> > > - val = ((const u8 *)data)[idx];
> >> > > + val = ((const u8 *)data)[i];
> >> > > break;
> >> > > case 2:
> >> > > - data_u16 = (const u16 *)data + idx;
> >> > > + data_u16 = (const u16 *)data + i;
> >> > >
> >> > > /*
> >> > > * Data is aligned to the sample size by IIO core.
> >> > > --
> >> > > 2.25.1
> >> > >
> >> >
> >> > Thanks.
