| Message ID | 20220408212857.9583-1-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | input: adc-joystick: Stop using scan_index for reading data | expand |
Hi Chris, On Fri, Apr 08, 2022 at 04:28:57PM -0500, Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > 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... Let's add Jonathan and Arthur for their take on this. > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > --- > 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.
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@hotmail.com> >> >> 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. Cheers, Artur > > Let's add Jonathan and Arthur for their take on this. > >> >> Signed-off-by: Chris Morgan <macromorgan@hotmail.com> >> --- >> 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.
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@hotmail.com> > > > > > > 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. > > Cheers, > Artur > > > > > Let's add Jonathan and Arthur for their take on this. > > > > > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > > --- > > > 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.
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@hotmail.com> >> > > >> > > 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); + ``` ...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. 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@hotmail.com> >> > > --- >> > > 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.
On Sat, 9 Apr 2022 20:39:18 -0500 Chris Morgan <macromorgan@hotmail.com> 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@hotmail.com> > > > > > > > > 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@hotmail.com> > > > > --- > > > > 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.
On Sun, 10 Apr 2022 16:58:48 +0200 Artur Rojek <contact@artur-rojek.eu> 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@hotmail.com> > >> > > > >> > > 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@hotmail.com> > >> > > --- > >> > > 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.
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@artur-rojek.eu> 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@hotmail.com> > > >> > > > > >> > > 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@hotmail.com> > > >> > > --- > > >> > > 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. >
On Mon, 11 Apr 2022 17:07:17 -0500 Chris Morgan <macromorgan@hotmail.com> wrote: > 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@artur-rojek.eu> 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@hotmail.com> > > > >> > > > > > >> > > 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? No each channel is naturally aligned after the earlier channels. (Short of oddiities around x86_32) it's the same as alignment in a c structure containing only the channels that are enabled. So lets take an example. Channel 0 - 16 bits. Channel 1 - 8 bits Channel 2 - 16 bits channel 3 - 64 bits (usually timestamp. If all enabled, then we end up with 0 2 3 4 6 8 16 [channel 0|channel 1| pad|channel 2 | pad | channel 3 | If only 0 and 2 0 2 4 |channel 0 | channels 2| If only 0 and 3 0 2 8 16 | channel 0| Lots of padding | channel 3 | etc. > > 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. No. Translates the scan index to an offset into the buffer. Index is not sufficient because it depends on what other channels are enabled. > > 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. Nope. As above. > - 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? More complex than that. Because of the alignment requirements. > > (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). You can set a trigger in code using iio_trigger_get() though we have provision for using hrtimer triggers this way. I vaguely recall a driver that provides both the trigger and acts as a consumer it might be the potentiostat/lmp910000 Jonathan > > 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@hotmail.com> > > > >> > > --- > > > >> > > 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. > >
On Tue, Apr 12, 2022 at 05:44:47PM +0100, Jonathan Cameron wrote: > On Mon, 11 Apr 2022 17:07:17 -0500 > Chris Morgan <macromorgan@hotmail.com> wrote: > > > 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@artur-rojek.eu> 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@hotmail.com> > > > > >> > > > > > > >> > > 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? > No each channel is naturally aligned after the earlier channels. > (Short of oddiities around x86_32) it's the same as alignment in a c structure > containing only the channels that are enabled. > > So lets take an example. > Channel 0 - 16 bits. > Channel 1 - 8 bits > Channel 2 - 16 bits > channel 3 - 64 bits (usually timestamp. > > If all enabled, then we end up with > 0 2 3 4 6 8 16 > [channel 0|channel 1| pad|channel 2 | pad | channel 3 | > > > If only 0 and 2 > 0 2 4 > |channel 0 | channels 2| > > If only 0 and 3 > 0 2 8 16 > | channel 0| Lots of padding | channel 3 | > > etc. > > > > > 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. > > No. Translates the scan index to an offset into the buffer. Index is > not sufficient because it depends on what other channels are enabled. > > > > > 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. > > Nope. As above. > > > - 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? > > More complex than that. Because of the alignment requirements. > > > > > (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). > > You can set a trigger in code using iio_trigger_get() though we have > provision for using hrtimer triggers this way. I vaguely > recall a driver that provides both the trigger and acts as a consumer > it might be the potentiostat/lmp910000 I appreciate it, I'll look at that driver. In the mean time I think I will have to defer to your expertise on the solution for this, but I'm happy to test it once you think you have something working. Thank you. > > > Jonathan > > > > > > 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@hotmail.com> > > > > >> > > --- > > > > >> > > 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. > > > >
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.