Re: [PATCH v7 7/7] input: joystick: Add ADC attached joystick driver.

Paul Cercueil
2020-05-19T21:03:09Z

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