On Sun, Apr 10, 2022 at 06:43:18PM +0100, Jonathan Cameron wrote:
> 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
>
Forgive me for being new to the iio subsystem, but how can I find out
the alignment? Is it simply a matter of assuming the largest value is
always the size of the alignment (meaning if we have a single channel
enabled with a 2 byte value all channels are 2 bytes in size, even
if the channel itself only reports 1 byte of data?
So if I understand correctly we need a helper function that translates
the scan index (the channel?) into the joystick index, and by doing
so we need to find the offset of each byte in the data buffer.
So basically a helper function that does the following for a given
buffer/channel:
- Gets the channels that are enabled (presume from the buffer
scan_mask).
- Identifies the data size of each channel.
- Sets the offset size to the largest value for the channel size.
- Returns the offset for the given channel in the buffer.
Then, in the adc-joystick driver we can store this offset in the
driver data for each axis so we know exactly where in the buffer
this data is.
Does that sound about right? Or am I way off?
(also, completely unrelated but while I have a captive audience, is
there an easy way to either send a single trigger or set up a trigger
for a given driver? I'd like to add a polling function to this and
I'm trying to find the correct way to do it, but so far I've only
found a userspace way to use an hrtimer set up via sysfs, I'd like
to make it a driver option if I can).
Thank you very much!
>
>
>
> > +
> > ```
> >
> > ...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.
>
