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
