Re: [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver.

[Dmitry Torokhov]

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 <contact@xxxxxxxxxxxxxx>
> Tested-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> Tested-by: Heiko Stuebner <heiko@xxxxxxxxx>
> ---
> 
> 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