Hi Andy,

thanks for the review, replies inline.

On 2020-09-06 11:22, Andy Shevchenko wrote:

On Sat, Sep 5, 2020 at 7:34 PM Artur Rojek [off-list ref] 
wrote:

quoted

Add a driver for joystick devices connected to ADC controllers
supporting the Industrial I/O subsystem.

...

quoted

+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;
+       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;

quoted

+               sign = (tolower(joy->chans[i].channel->scan_type.sign) 
== 's');

Redundant parentheses.

quoted

+               switch (bytes) {
+               case 1:
+                       val = ((const u8 *)data)[idx];
+                       break;
+               case 2:

quoted

+                       if (endianness == IIO_BE)
+                               val = be16_to_cpu(((const __be16 
*)data)[idx]);
+                       else if (endianness == IIO_LE)
+                               val = le16_to_cpu(((const __le16 
*)data)[idx]);
+                       else /* IIO_CPU */
+                               val = ((const u16 *)data)[idx];
+                       break;

Hmm... I don't like explicit castings to restricted types. On top of
that is it guaranteed that pointer to data will be aligned?

The buffer comes from the IIO core, it is aligned to the sample size.

As a solution for the first two I would recommend to use
get_unaligned_be16() / get_unaligned_le16().
The last one is an interesting case and if data can be unaligned needs
to be fixed.

quoted

+               default:
+                       return -EINVAL;
+               }
+
+               val >>= joy->chans[i].channel->scan_type.shift;
+               if (sign)
+                       val = sign_extend32(val, msb);
+               else

quoted

+                       val &= GENMASK(msb, 0);

It includes msb. Is it expected?

Yes, that's expected as `msb = joy->chans[i].channel->scan_type.realbits 
- 1`.

quoted

+               input_report_abs(joy->input, joy->axes[i].code, val);
+       }
+
+       input_sync(joy->input);
+
+       return 0;
+}

...

quoted

+static int adc_joystick_open(struct input_dev *dev)

quoted

+static void adc_joystick_close(struct input_dev *dev)

Just wondering if this is protected against object lifetime cases.

Can you clarify that in more details?

...

quoted

+err:

err_fwnode_put: ?

quoted

+       fwnode_handle_put(child);
+       return ret;

...

quoted

+       /* Count how many channels we got. NULL terminated. */
+       for (i = 0; joy->chans[i].indio_dev; ++i) {
+               bits = joy->chans[i].channel->scan_type.storagebits;
+               if (!bits || (bits > 16)) {
+                       dev_err(dev, "Unsupported channel storage 
size\n");

quoted

+                       return -EINVAL;

-ERANGE?

quoted

+               }
+               if (bits != 
joy->chans[0].channel->scan_type.storagebits) {
+                       dev_err(dev, "Channels must have equal storage 
size\n");
+                       return -EINVAL;
+               }
+       }