Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.

Paul Cercueil
2020-04-17T21:24:13Z

Hi Andy,

Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
<andy.shevchenko@gmail.com> a écrit :
> On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@artur-rojek.eu>
> wrote:
>>
>> Add a driver for joystick devices connected to ADC controllers
>> supporting the Industrial I/O subsystem.
>
> ...
>
>> +#include <linux/of.h>
>
> Do you really need this? (See below as well)
>
> ...
>
>> + sign =
>> (tolower(joy->chans[i].channel->scan_type.sign) == 's');
>
> Too many parentheses. But here it's up to you,
>
> ...
>
>> + case 2:
>
>> + val = ((const u16 *)data)[i];
>
> Can't you do this in each branch below?
>
>> + if (endianness == IIO_BE)
>> + val = be16_to_cpu(val);
>> + else if (endianness == IIO_LE)
>> + val = le16_to_cpu(val);
>> + break;
>
> ...
>
>> + device_for_each_child_node(dev, child) {
>> + ret = fwnode_property_read_u32(child, "reg", &i);
>> + if (ret || i >= num_axes) {
>> + dev_err(dev, "reg invalid or missing");
>> + goto err;
>> + }
>> +
>> + if (fwnode_property_read_u32(child, "linux,code",
>> + &axes[i].code)) {
>> + dev_err(dev, "linux,code invalid or
>> missing");
>> + goto err;
>> + }
>> +
>> + if (fwnode_property_read_u32_array(child,
>> "abs-range",
>> + axes[i].range,
>> 2)) {
>> + dev_err(dev, "abs-range invalid or
>> missing");
>> + goto err;
>> + }
>
>> + }
>> +
>> + joy->axes = axes;
>> +
>> + return 0;
>> +
>> +err:
>> + fwnode_handle_put(child);
>
>> + return -EINVAL;
>
> Can we avoid shadowing the actual error code?
>
> ...
>
>> + bits = joy->chans[0].channel->scan_type.storagebits;
>
>> + if (!bits || (bits >> 3) > 2) {
>
> Wouldn't be clear to use simple 'bits > 16'?
>
>> + dev_err(dev, "Unsupported channel storage size");
>> + return -EINVAL;
>> + }
>
> ...
>
>> +static const struct of_device_id adc_joystick_of_match[] = {
>> + { .compatible = "adc-joystick", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
>> +
>> +static struct platform_driver adc_joystick_driver = {
>> + .driver = {
>> + .name = "adc-joystick",
>
>> + .of_match_table =
>> of_match_ptr(adc_joystick_of_match),
>
> Drop this a bit harmful of_match_ptr() macro. It should go with ugly
> #ifdeffery. Here you simple introduced a compiler warning.

I assume you mean #ifdef around the of_device_id + module table macro?

> On top of that, you are using device property API, OF use in this case
> is contradictory (at lest to some extend).

I don't see why. The fact that the driver can work when probed from
platform code, doesn't mean that it shouldn't have a table to probe
from devicetree.

-Paul

>
>> + },
>> + .probe = adc_joystick_probe,
>> +};
>
> --
> With Best Regards,
> Andy Shevchenko