On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@xxxxxxxxxxxxxx> wrote: > > Don't try to access buffer data of a channel by its scan index. Instead, > calculate its offset in the buffer. > > This is necessary, as the scan index of a channel does not represent its > position in a buffer - the buffer will contain data for enabled channels > only, affecting data offsets and alignment. > > While at it, also fix minor style issue in probe. a minor the probe > Reported-by: Chris Morgan <macromorgan@xxxxxxxxxxx> > Closes: https://lore.kernel.org/linux-input/20220408212857.9583-1-macroalpha82@xxxxxxxxx/ What is this tag? Anything documented? Otherwise use BugLink or Link. ... > struct adc_joystick_axis *axes; > struct iio_channel *chans; > + int *offsets; Why not unsigned? I.o.w. is there any meaning for negative values? > int num_chans; > bool polled; ... > + off = joy->offsets[i]; > + Move this blank line to be before the previous line. > + if (off < 0) > + return -EINVAL; ... > case 1: > - val = ((const u8 *)data)[idx]; > + val = *chan_data; Might be also get_unaligned((const u8 *)chan_data); And with all this (see below) the chan_data actually can be declared as const void *. > break; > case 2: > - data_u16 = (const u16 *)data + idx; > - > /* > * Data is aligned to the sample size by IIO core. > * Call `get_unaligned_xe16` to hide type casting. > */ > if (endianness == IIO_BE) > - val = get_unaligned_be16(data_u16); > + val = get_unaligned_be16(chan_data); > else if (endianness == IIO_LE) > - val = get_unaligned_le16(data_u16); > + val = get_unaligned_le16(chan_data); > else /* IIO_CPU */ > - val = *data_u16; > + val = *(const u16 *)chan_data; This probably needs to be get_unaligned((const u16 *)chan_data); for the sake of consistency with the above. > break; > default: > return -EINVAL; ... > +static int adc_joystick_si_cmp(const void *a, const void *b, const void *priv) > +{ > + const struct iio_channel *chans = priv; > + > + return chans[*(int *)a].channel->scan_index - > + chans[*(int *)b].channel->scan_index; Discarding const? > +} ... > + offsets = kmalloc_array(count, sizeof(int), GFP_KERNEL); sizeof(*offsets) > + if (!offsets) > + return ERR_PTR(-ENOMEM); ... > + si_order = kmalloc_array(count, sizeof(int), GFP_KERNEL); sizeof(*si_order) > + if (!si_order) { > + kfree(offsets); > + return ERR_PTR(-ENOMEM); > + } ... > + /* Channels in buffer are ordered by scan index. Sort to match that. */ the buffer > + sort_r(si_order, count, sizeof(int), adc_joystick_si_cmp, NULL, chans); sizeof(*si_order) ? sizeof(int) is a bit odd, the above will tell better without even knowing the sort_r() parameters what it is about. ... > + for (i = 0; i < count; ++i) { > + idx = si_order[i]; > + ch = chans[idx].channel; > + si = ch->scan_index; > + > + if (si < 0 || !test_bit(si, indio_dev->active_scan_mask)) { > + offsets[idx] = -1; > + continue; > + } > + > + /* Channels sharing scan indices also share the samples. */ > + if (idx > 0 && si == chans[idx - 1].channel->scan_index) { > + offsets[idx] = offsets[idx - 1]; > + continue; > + } > + > + offsets[idx] = offset; > + length = ch->scan_type.storagebits / 8; BITS_PER_BYTE ? > + if (ch->scan_type.repeat > 1) > + length *= ch->scan_type.repeat; > + /* Account for channel alignment. */ > + if (offset % length) > + offset += length - (offset % length); Would one of ALIGN() / rounddown / etc work here? > + offset += length; > + } ... > + joy->offsets = adc_joystick_get_chan_offsets(joy->chans, > + joy->num_chans); > + if (IS_ERR(joy->offsets)) { > + dev_err(devp, "Unable to allocate channel offsets\n"); > + return PTR_ERR(joy->offsets); > + } > + > + return 0; return PTR_ERR_OR_ZERO() ? ... > error = devm_add_action_or_reset(dev, adc_joystick_cleanup, > joy->buffer); > - if (error) { > + if (error) { > dev_err(dev, "Unable to add action\n"); > return error; > } Unrelated change. -- With Best Regards, Andy Shevchenko
