Messages in this thread | | | Date | Tue, 19 May 2020 23:02:54 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH v7 7/7] input: joystick: Add ADC attached joystick driver. |
| |
Hi Andy,
Le mar. 19 mai 2020 à 23:43, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Sun, May 17, 2020 at 10:49 PM Artur Rojek <contact@artur-rojek.eu> > wrote: >> >> Add a driver for joystick devices connected to ADC controllers >> supporting the Industrial I/O subsystem. > > ... > >> +static int adc_joystick_handle(const void *data, void *private) >> +{ >> + struct adc_joystick *joy = private; >> + enum iio_endian endianness; >> + int bytes, msb, val, i; >> + bool sign; >> + >> + bytes = joy->chans[0].channel->scan_type.storagebits >> 3; >> + >> + for (i = 0; i < joy->num_chans; ++i) { >> + 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'); > > Do we need tolower()?
I'll answer this one:
The sign can be uppercase to specify that the value is sign-extended in all the storage bits.
-Paul
>> + >> + switch (bytes) { >> + case 1: >> + val = ((const u8 *)data)[i]; >> + break; >> + case 2: >> + if (endianness == IIO_BE) > >> + val = be16_to_cpu(((const u16 >> *)data)[i]); > > Yeah, you have to provide bitwise types to satisfy sparse. > Maybe using *_to_cpup() will cure this. > >> + else if (endianness == IIO_LE) >> + val = le16_to_cpu(((const u16 >> *)data)[i]); >> + else /* IIO_CPU */ >> + val = ((const u16 *)data)[i]; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + val >>= joy->chans[i].channel->scan_type.shift; >> + if (sign) >> + val = sign_extend32(val, msb); >> + else >> + val &= GENMASK(msb, 0); >> + input_report_abs(joy->input, joy->axes[i].code, >> val); >> + } >> + >> + input_sync(joy->input); >> + >> + return 0; >> +} > > ... > >> + /* Count how many channels we got. NULL terminated. */ >> + while (joy->chans[joy->num_chans].indio_dev) >> + joy->num_chans++; > > I don't see how useful this is. Why not simple do below... > >> + bits = joy->chans[0].channel->scan_type.storagebits; >> + if (!bits || (bits > 16)) { >> + dev_err(dev, "Unsupported channel storage size"); >> + return -EINVAL; >> + } >> + for (i = 1; i < joy->num_chans; ++i) >> + if (joy->chans[i].channel->scan_type.storagebits != >> bits) { >> + dev_err(dev, "Channels must have equal >> storage size"); >> + return -EINVAL; >> + } > > ...something like > > for (i = 0; joy->chans[i].indio_dev; i++) { > bits = joy->chans[i].channel->scan_type.storagebits; > if (bits ...) { > ...error handling... > } > if (bits != joy->chans[0].channel->scan_type.storagebits) { > ...second level of error handling... > } > } > > ... > >> +static const struct of_device_id adc_joystick_of_match[] = { >> + { .compatible = "adc-joystick", }, > >> + { }, > > No need comma. > >> +}; > > -- > With Best Regards, > Andy Shevchenko
| |