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
