On Sat, 9 Apr 2022 20:39:18 -0500
Chris Morgan <macromorgan@xxxxxxxxxxx> 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?
No the ADC driver should be packing the data (actually it may well be
the IIO core doing repacking depending on how clever the ADC in question is).
The joystick driver is too simplistic unfortunately. scan_index is not the index
of the data in the current scan - it is only guaranteed to be monotonic
in the sense that for any given channel it's position will be (packed as
best possible whilst being naturally aligned) after all channels with
lower scan indicies. scan_index effectively refers to the order of
channels if 'all' channels are enabled.
Hence the driver should work out the ordering from scan_index
but not assume it corresponds to the position of the data
(to make things worse there is no guarantee that the channels have the
same number of bits so that should also be accounted for).
So to take a pathlogical example. If you have x,y mapping to
scan index 5, 2 then only these two channels will be enabled.
The driver correctly handles the ordering because of the mapping
from channel to access code. However to find the position
it needs to walk the channels. It needs to know that 5 is
actually offset by whatever storagebits value the channel with
index 2 has (potentially with additional padding to ensure
we maintainer natural alignment - imagine, e.g. the channel with
scan_index = 2 might be 8 bit and the one with scan_index 5 might
be 16 bit. In that case you would need to pad so the scan_index
5 channel is 16 bit aligned in the buffer. If they were the
other way around there would be no padding between the channels.
The ordering of channels from the joystick channel specific bit
of the binding comes from device tree 'reg' value and the ordering
of the IIO map comes from io-channels ordering. So these should
I think map one to one (without messing around with actual tests
I'm not 100% sure on these two ordering questions).
When the driver then maps all the channels in io-channels they will
in turn have scan_index values (and sizes) Unfortunately to know
the ordering we'll have to index over them and put them in the
appropriate order. I'd suggest just storing the resulting
offset in the data buffer during that initial scan so it can
be easily used later.
So some more complexity to handle I'm afraid!
Jonathan
+CC linux-iio as this issue might occur with other consumers - I clearly
haven't been paying close enough attention :(
>
> Thank you.
>
> >
> > 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.
