Hi Artur,

On Sat, Sep 05, 2020 at 06:34:03PM +0200, Artur Rojek wrote:

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

Signed-off-by: Artur Rojek <redacted>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---

Changes:
    v8: - respect scan index when reading channel data,
        - solve sparse warnings related to *_to_cpu calls,
        - simplify channel count logic,
        - drop the redundant comma from adc_joystick_of_match[]
    
    v9: - use `dev_err_probe`,
        - add missing CR to error messages,
        - remove unnecessary line breaks in `adc_joystick_set_axes`,
        - remove redundant error code print from `adc_joystick_probe`,
        - no need to pass `dev.parent` to `dev_err` in `adc_joystick_open`,
        - print error code in `adc_joystick_open`

Notes:
	Dmitry, Jonathan, because of the above changes, I dropped your
        Acked-by.

So I am still happy with the driver, just a bit of bikeshedding since it
looks like it can go through my tree now:

+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret) {

Call this "error"?

+			dev_err(dev, "reg invalid or missing\n");
+			goto err;
+		}
+
+		if (i >= num_axes) {
+			ret = -EINVAL;
+			dev_err(dev, "No matching axis for reg %d\n", i);
+			goto err;
+		}
+
+		ret = fwnode_property_read_u32(child, "linux,code",
+					     &axes[i].code);
+		if (ret) {
+			dev_err(dev, "linux,code invalid or missing\n");
+			goto err;
+		}
+
+		ret = fwnode_property_read_u32_array(child, "abs-range",
+						   axes[i].range, 2);
+		if (ret) {
+			dev_err(dev, "abs-range invalid or missing\n");
+			goto err;
+		}
+
+		fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz);
+		fwnode_property_read_u32(child, "abs-flat", &axes[i].flat);
+
+		input_set_abs_params(joy->input, axes[i].code,
+				     axes[i].range[0], axes[i].range[1],
+				     axes[i].fuzz, axes[i].flat);
+		input_set_capability(joy->input, EV_ABS, axes[i].code);
+	}
+
+	joy->axes = axes;
+
+	return 0;
+
+err:
+	fwnode_handle_put(child);
+	return ret;

"return error;"

In general, I prefer "error" for the variable name when it returned in
error paths only, and "ret", "retval", etc. when it is used in both
error and success paths.

+}
+
+static int adc_joystick_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct adc_joystick *joy;
+	struct input_dev *input;
+	int bits, ret, i;
+
+	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
+	if (!joy)
+		return -ENOMEM;
+
+	joy->chans = devm_iio_channel_get_all(dev);
+	if (IS_ERR(joy->chans)) {
+		return dev_err_probe(dev, PTR_ERR(joy->chans),
+				     "Unable to get IIO channels\n");

I am not a fan of this API (dev_err_probe), so can we not use it just
yet? I believe Rob is looking into pushing this into resources
providers, at least when they have device for which resources are being
acquired.

+	}
+
+	/* 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)) {

I do not think we need parenthesis around second part of the condition.

Hmm, why don't we have "is_in_range(val, lower, upper)" yet?

Thanks.

-- 
Dmitry