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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Le sam. 18 avril 2020 à 0:10, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> a écrit :
On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@xxxxxxxxxxxxxx> 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






[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux