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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200914204126.GB1681290@dtor-ws>
Date:   Mon, 14 Sep 2020 13:41:26 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Artur Rojek <contact@...ur-rojek.eu>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jonathan Cameron <jic23@...nel.org>,
        Paul Cercueil <paul@...pouillou.net>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver.

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@...ur-rojek.eu>
> Tested-by: Paul Cercueil <paul@...pouillou.net>
> Tested-by: Heiko Stuebner <heiko@...ech.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