linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9 1/2] dt-bindings: input: Add docs for ADC driven joystick.
@ 2020-09-05 16:34 Artur Rojek
  2020-09-05 16:34 ` [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver Artur Rojek
  0 siblings, 1 reply; 6+ messages in thread
From: Artur Rojek @ 2020-09-05 16:34 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Andy Shevchenko
  Cc: Heiko Stuebner, Ezequiel Garcia, linux-input, devicetree,
	linux-kernel, Artur Rojek, Rob Herring

Add documentation for the adc-joystick driver, used to provide support
for joysticks connected over ADC.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Changes:
    v6-v9: no change

 .../bindings/input/adc-joystick.yaml          | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/adc-joystick.yaml

diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml b/Documentation/devicetree/bindings/input/adc-joystick.yaml
new file mode 100644
index 000000000000..054406bbd22b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
@@ -0,0 +1,121 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019-2020 Artur Rojek
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/input/adc-joystick.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: ADC attached joystick
+
+maintainers:
+  - Artur Rojek <contact@artur-rojek.eu>
+
+description: >
+  Bindings for joystick devices connected to ADC controllers supporting
+  the Industrial I/O subsystem.
+
+properties:
+  compatible:
+    const: adc-joystick
+
+  io-channels:
+    minItems: 1
+    maxItems: 1024
+    description: >
+      List of phandle and IIO specifier pairs.
+      Each pair defines one ADC channel to which a joystick axis is connected.
+      See Documentation/devicetree/bindings/iio/iio-bindings.txt for details.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+required:
+  - compatible
+  - io-channels
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+patternProperties:
+  "^axis@[0-9a-f]+$":
+    type: object
+    description: >
+      Represents a joystick axis bound to the given ADC channel.
+      For each entry in the io-channels list, one axis subnode with a matching
+      reg property must be specified.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 1023
+        description: Index of an io-channels list entry bound to this axis.
+
+      linux,code:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: EV_ABS specific event code generated by the axis.
+
+      abs-range:
+        allOf:
+          - $ref: /schemas/types.yaml#/definitions/uint32-array
+          - items:
+              - description: minimum value
+              - description: maximum value
+        description: >
+          Minimum and maximum values produced by the axis.
+          For an ABS_X axis this will be the left-most and right-most
+          inclination of the joystick. If min > max, it is left to userspace to
+          treat the axis as inverted.
+          This property is interpreted as two signed 32 bit values.
+
+      abs-fuzz:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: >
+          Amount of noise in the input value.
+          Omitting this property indicates the axis is precise.
+
+      abs-flat:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: >
+          Axial "deadzone", or area around the center position, where the axis
+          is considered to be at rest.
+          Omitting this property indicates the axis always returns to exactly
+          the center position.
+
+    required:
+      - reg
+      - linux,code
+      - abs-range
+
+    additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/iio/adc/ingenic,adc.h>
+    #include <dt-bindings/input/input.h>
+
+    joystick: adc-joystick {
+      compatible = "adc-joystick";
+      io-channels = <&adc INGENIC_ADC_TOUCH_XP>,
+                    <&adc INGENIC_ADC_TOUCH_YP>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      axis@0 {
+              reg = <0>;
+              linux,code = <ABS_X>;
+              abs-range = <3300 0>;
+              abs-fuzz = <4>;
+              abs-flat = <200>;
+      };
+      axis@1 {
+              reg = <1>;
+              linux,code = <ABS_Y>;
+              abs-range = <0 3300>;
+              abs-fuzz = <4>;
+              abs-flat = <200>;
+      };
+    };
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver.
  2020-09-05 16:34 [PATCH v9 1/2] dt-bindings: input: Add docs for ADC driven joystick Artur Rojek
@ 2020-09-05 16:34 ` Artur Rojek
  2020-09-06  9:22   ` Andy Shevchenko
  2020-09-14 20:41   ` Dmitry Torokhov
  0 siblings, 2 replies; 6+ messages in thread
From: Artur Rojek @ 2020-09-05 16:34 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Andy Shevchenko
  Cc: Heiko Stuebner, Ezequiel Garcia, linux-input, devicetree,
	linux-kernel, Artur Rojek

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

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
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.

 drivers/input/joystick/Kconfig        |  10 ++
 drivers/input/joystick/Makefile       |   1 +
 drivers/input/joystick/adc-joystick.c | 249 ++++++++++++++++++++++++++
 3 files changed, 260 insertions(+)
 create mode 100644 drivers/input/joystick/adc-joystick.c

diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index eb031b7a4866..b080f0cfb068 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -42,6 +42,16 @@ config JOYSTICK_A3D
 	  To compile this driver as a module, choose M here: the
 	  module will be called a3d.
 
+config JOYSTICK_ADC
+	tristate "Simple joystick connected over ADC"
+	depends on IIO
+	select IIO_BUFFER_CB
+	help
+	  Say Y here if you have a simple joystick connected over ADC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called adc-joystick.
+
 config JOYSTICK_ADI
 	tristate "Logitech ADI digital joysticks and gamepads"
 	select GAMEPORT
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 8656023f6ef5..58232b3057d3 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -6,6 +6,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_JOYSTICK_A3D)		+= a3d.o
+obj-$(CONFIG_JOYSTICK_ADC)		+= adc-joystick.o
 obj-$(CONFIG_JOYSTICK_ADI)		+= adi.o
 obj-$(CONFIG_JOYSTICK_AMIGA)		+= amijoy.o
 obj-$(CONFIG_JOYSTICK_AS5011)		+= as5011.o
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
new file mode 100644
index 000000000000..e3ca15eed512
--- /dev/null
+++ b/drivers/input/joystick/adc-joystick.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Input driver for joysticks connected over ADC.
+ * Copyright (c) 2019-2020 Artur Rojek <contact@artur-rojek.eu>
+ */
+#include <linux/ctype.h>
+#include <linux/input.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/consumer.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+struct adc_joystick_axis {
+	u32 code;
+	s32 range[2];
+	s32 fuzz;
+	s32 flat;
+};
+
+struct adc_joystick {
+	struct input_dev *input;
+	struct iio_cb_buffer *buffer;
+	struct adc_joystick_axis *axes;
+	struct iio_channel *chans;
+	int num_chans;
+};
+
+static int adc_joystick_handle(const void *data, void *private)
+{
+	struct adc_joystick *joy = private;
+	enum iio_endian endianness;
+	int bytes, msb, val, idx, i;
+	bool sign;
+
+	bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
+
+	for (i = 0; i < joy->num_chans; ++i) {
+		idx = joy->chans[i].channel->scan_index;
+		endianness = joy->chans[i].channel->scan_type.endianness;
+		msb = joy->chans[i].channel->scan_type.realbits - 1;
+		sign = (tolower(joy->chans[i].channel->scan_type.sign) == 's');
+
+		switch (bytes) {
+		case 1:
+			val = ((const u8 *)data)[idx];
+			break;
+		case 2:
+			if (endianness == IIO_BE)
+				val = be16_to_cpu(((const __be16 *)data)[idx]);
+			else if (endianness == IIO_LE)
+				val = le16_to_cpu(((const __le16 *)data)[idx]);
+			else /* IIO_CPU */
+				val = ((const u16 *)data)[idx];
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		val >>= joy->chans[i].channel->scan_type.shift;
+		if (sign)
+			val = sign_extend32(val, msb);
+		else
+			val &= GENMASK(msb, 0);
+		input_report_abs(joy->input, joy->axes[i].code, val);
+	}
+
+	input_sync(joy->input);
+
+	return 0;
+}
+
+static int adc_joystick_open(struct input_dev *dev)
+{
+	struct adc_joystick *joy = input_get_drvdata(dev);
+	struct device *devp = &dev->dev;
+	int ret;
+
+	ret = iio_channel_start_all_cb(joy->buffer);
+	if (ret)
+		dev_err(devp, "Unable to start callback buffer: %d\n", ret);
+
+	return ret;
+}
+
+static void adc_joystick_close(struct input_dev *dev)
+{
+	struct adc_joystick *joy = input_get_drvdata(dev);
+
+	iio_channel_stop_all_cb(joy->buffer);
+}
+
+static void adc_joystick_cleanup(void *data)
+{
+	iio_channel_release_all_cb(data);
+}
+
+static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
+{
+	struct adc_joystick_axis *axes;
+	struct fwnode_handle *child;
+	int num_axes, ret, i;
+
+	num_axes = device_get_child_node_count(dev);
+	if (!num_axes) {
+		dev_err(dev, "Unable to find child nodes\n");
+		return -EINVAL;
+	}
+
+	if (num_axes != joy->num_chans) {
+		dev_err(dev, "Got %d child nodes for %d channels\n",
+			num_axes, joy->num_chans);
+		return -EINVAL;
+	}
+
+	axes = devm_kmalloc_array(dev, num_axes, sizeof(*axes), GFP_KERNEL);
+	if (!axes)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret) {
+			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;
+}
+
+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");
+	}
+
+	/* 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)) {
+			dev_err(dev, "Unsupported channel storage size\n");
+			return -EINVAL;
+		}
+		if (bits != joy->chans[0].channel->scan_type.storagebits) {
+			dev_err(dev, "Channels must have equal storage size\n");
+			return -EINVAL;
+		}
+	}
+	joy->num_chans = i;
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "Unable to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	joy->input = input;
+	input->name = pdev->name;
+	input->id.bustype = BUS_HOST;
+	input->open = adc_joystick_open;
+	input->close = adc_joystick_close;
+
+	ret = adc_joystick_set_axes(dev, joy);
+	if (ret)
+		return ret;
+
+	input_set_drvdata(input, joy);
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "Unable to register input device\n");
+		return ret;
+	}
+
+	joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
+	if (IS_ERR(joy->buffer)) {
+		dev_err(dev, "Unable to allocate callback buffer\n");
+		return PTR_ERR(joy->buffer);
+	}
+
+	ret = devm_add_action_or_reset(dev, adc_joystick_cleanup, joy->buffer);
+	if (ret)
+		dev_err(dev, "Unable to add action\n");
+
+	return ret;
+}
+
+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 = adc_joystick_of_match,
+	},
+	.probe = adc_joystick_probe,
+};
+module_platform_driver(adc_joystick_driver);
+
+MODULE_DESCRIPTION("Input driver for joysticks connected over ADC");
+MODULE_AUTHOR("Artur Rojek <contact@artur-rojek.eu>");
+MODULE_LICENSE("GPL");
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver.
  2020-09-05 16:34 ` [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver Artur Rojek
@ 2020-09-06  9:22   ` Andy Shevchenko
  2020-09-06 12:09     ` Artur Rojek
  2020-09-14 20:41   ` Dmitry Torokhov
  1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-09-06  9:22 UTC (permalink / raw)
  To: Artur Rojek
  Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Heiko Stuebner, Ezequiel Garcia, linux-input,
	devicetree, Linux Kernel Mailing List

On Sat, Sep 5, 2020 at 7:34 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Add a driver for joystick devices connected to ADC controllers
> supporting the Industrial I/O subsystem.

...

> +static int adc_joystick_handle(const void *data, void *private)
> +{
> +       struct adc_joystick *joy = private;
> +       enum iio_endian endianness;
> +       int bytes, msb, val, idx, i;
> +       bool sign;
> +
> +       bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
> +
> +       for (i = 0; i < joy->num_chans; ++i) {
> +               idx = joy->chans[i].channel->scan_index;
> +               endianness = joy->chans[i].channel->scan_type.endianness;
> +               msb = joy->chans[i].channel->scan_type.realbits - 1;

> +               sign = (tolower(joy->chans[i].channel->scan_type.sign) == 's');

Redundant parentheses.

> +               switch (bytes) {
> +               case 1:
> +                       val = ((const u8 *)data)[idx];
> +                       break;
> +               case 2:

> +                       if (endianness == IIO_BE)
> +                               val = be16_to_cpu(((const __be16 *)data)[idx]);
> +                       else if (endianness == IIO_LE)
> +                               val = le16_to_cpu(((const __le16 *)data)[idx]);
> +                       else /* IIO_CPU */
> +                               val = ((const u16 *)data)[idx];
> +                       break;

Hmm... I don't like explicit castings to restricted types. On top of
that is it guaranteed that pointer to data will be aligned?
As a solution for the first two I would recommend to use
get_unaligned_be16() / get_unaligned_le16().
The last one is an interesting case and if data can be unaligned needs
to be fixed.

> +               default:
> +                       return -EINVAL;
> +               }
> +
> +               val >>= joy->chans[i].channel->scan_type.shift;
> +               if (sign)
> +                       val = sign_extend32(val, msb);
> +               else

> +                       val &= GENMASK(msb, 0);

It includes msb. Is it expected?

> +               input_report_abs(joy->input, joy->axes[i].code, val);
> +       }
> +
> +       input_sync(joy->input);
> +
> +       return 0;
> +}

...

> +static int adc_joystick_open(struct input_dev *dev)

> +static void adc_joystick_close(struct input_dev *dev)

Just wondering if this is protected against object lifetime cases.

...

> +err:

err_fwnode_put: ?

> +       fwnode_handle_put(child);
> +       return ret;

...

> +       /* 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)) {
> +                       dev_err(dev, "Unsupported channel storage size\n");

> +                       return -EINVAL;

-ERANGE?

> +               }
> +               if (bits != joy->chans[0].channel->scan_type.storagebits) {
> +                       dev_err(dev, "Channels must have equal storage size\n");
> +                       return -EINVAL;
> +               }
> +       }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver.
  2020-09-06  9:22   ` Andy Shevchenko
@ 2020-09-06 12:09     ` Artur Rojek
  2020-09-14 20:34       ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Artur Rojek @ 2020-09-06 12:09 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Heiko Stuebner, Ezequiel Garcia, linux-input,
	devicetree, Linux Kernel Mailing List

Hi Andy,

thanks for the review, replies inline.

On 2020-09-06 11:22, Andy Shevchenko wrote:
> On Sat, Sep 5, 2020 at 7:34 PM Artur Rojek <contact@artur-rojek.eu> 
> wrote:
>> 
>> Add a driver for joystick devices connected to ADC controllers
>> supporting the Industrial I/O subsystem.
> 
> ...
> 
>> +static int adc_joystick_handle(const void *data, void *private)
>> +{
>> +       struct adc_joystick *joy = private;
>> +       enum iio_endian endianness;
>> +       int bytes, msb, val, idx, i;
>> +       bool sign;
>> +
>> +       bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
>> +
>> +       for (i = 0; i < joy->num_chans; ++i) {
>> +               idx = joy->chans[i].channel->scan_index;
>> +               endianness = 
>> joy->chans[i].channel->scan_type.endianness;
>> +               msb = joy->chans[i].channel->scan_type.realbits - 1;
> 
>> +               sign = (tolower(joy->chans[i].channel->scan_type.sign) 
>> == 's');
> 
> Redundant parentheses.
> 
>> +               switch (bytes) {
>> +               case 1:
>> +                       val = ((const u8 *)data)[idx];
>> +                       break;
>> +               case 2:
> 
>> +                       if (endianness == IIO_BE)
>> +                               val = be16_to_cpu(((const __be16 
>> *)data)[idx]);
>> +                       else if (endianness == IIO_LE)
>> +                               val = le16_to_cpu(((const __le16 
>> *)data)[idx]);
>> +                       else /* IIO_CPU */
>> +                               val = ((const u16 *)data)[idx];
>> +                       break;
> 
> Hmm... I don't like explicit castings to restricted types. On top of
> that is it guaranteed that pointer to data will be aligned?
The buffer comes from the IIO core, it is aligned to the sample size.
> As a solution for the first two I would recommend to use
> get_unaligned_be16() / get_unaligned_le16().
> The last one is an interesting case and if data can be unaligned needs
> to be fixed.
> 
>> +               default:
>> +                       return -EINVAL;
>> +               }
>> +
>> +               val >>= joy->chans[i].channel->scan_type.shift;
>> +               if (sign)
>> +                       val = sign_extend32(val, msb);
>> +               else
> 
>> +                       val &= GENMASK(msb, 0);
> 
> It includes msb. Is it expected?
Yes, that's expected as `msb = joy->chans[i].channel->scan_type.realbits 
- 1`.
> 
>> +               input_report_abs(joy->input, joy->axes[i].code, val);
>> +       }
>> +
>> +       input_sync(joy->input);
>> +
>> +       return 0;
>> +}
> 
> ...
> 
>> +static int adc_joystick_open(struct input_dev *dev)
> 
>> +static void adc_joystick_close(struct input_dev *dev)
> 
> Just wondering if this is protected against object lifetime cases.
Can you clarify that in more details?
> 
> ...
> 
>> +err:
> 
> err_fwnode_put: ?
> 
>> +       fwnode_handle_put(child);
>> +       return ret;
> 
> ...
> 
>> +       /* 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)) {
>> +                       dev_err(dev, "Unsupported channel storage 
>> size\n");
> 
>> +                       return -EINVAL;
> 
> -ERANGE?
> 
>> +               }
>> +               if (bits != 
>> joy->chans[0].channel->scan_type.storagebits) {
>> +                       dev_err(dev, "Channels must have equal storage 
>> size\n");
>> +                       return -EINVAL;
>> +               }
>> +       }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver.
  2020-09-06 12:09     ` Artur Rojek
@ 2020-09-14 20:34       ` Dmitry Torokhov
  0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2020-09-14 20:34 UTC (permalink / raw)
  To: Artur Rojek
  Cc: Andy Shevchenko, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Heiko Stuebner, Ezequiel Garcia, linux-input,
	devicetree, Linux Kernel Mailing List

On Sun, Sep 06, 2020 at 02:09:28PM +0200, Artur Rojek wrote:
> Hi Andy,
> 
> thanks for the review, replies inline.
> 
> On 2020-09-06 11:22, Andy Shevchenko wrote:
> > On Sat, Sep 5, 2020 at 7:34 PM Artur Rojek <contact@artur-rojek.eu>
> > wrote:
> > 
> > > +static int adc_joystick_open(struct input_dev *dev)
> > 
> > > +static void adc_joystick_close(struct input_dev *dev)
> > 
> > Just wondering if this is protected against object lifetime cases.
> Can you clarify that in more details?

If there are lifetime issues they would be in input core, not individual
driver. But input core ensures that it calls close (if open was called
earlier) before doing input device teardown.

> > 
> > ...
> > 
> > > +err:
> > 
> > err_fwnode_put: ?
> > 
> > > +       fwnode_handle_put(child);
> > > +       return ret;
> > 
> > ...
> > 
> > > +       /* 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)) {
> > > +                       dev_err(dev, "Unsupported channel storage
> > > size\n");
> > 
> > > +                       return -EINVAL;
> > 
> > -ERANGE?

/* Math result not representable */

? Seems not any better than -EINVAL.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver.
  2020-09-05 16:34 ` [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver Artur Rojek
  2020-09-06  9:22   ` Andy Shevchenko
@ 2020-09-14 20:41   ` Dmitry Torokhov
  1 sibling, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2020-09-14 20:41 UTC (permalink / raw)
  To: Artur Rojek
  Cc: Rob Herring, Mark Rutland, Jonathan Cameron, Paul Cercueil,
	Andy Shevchenko, Heiko Stuebner, Ezequiel Garcia, linux-input,
	devicetree, linux-kernel

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@artur-rojek.eu>
> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-09-14 20:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-05 16:34 [PATCH v9 1/2] dt-bindings: input: Add docs for ADC driven joystick Artur Rojek
2020-09-05 16:34 ` [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver Artur Rojek
2020-09-06  9:22   ` Andy Shevchenko
2020-09-06 12:09     ` Artur Rojek
2020-09-14 20:34       ` Dmitry Torokhov
2020-09-14 20:41   ` Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).