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.
On top of that, you are using device property API, OF use in this case
is contradictory (at lest to some extend).
> + },
> + .probe = adc_joystick_probe,
> +};
--
With Best Regards,
Andy Shevchenko
