diff mbox series

[v9,1/2] dt-bindings: input: Add docs for ADC driven joystick.

Message ID 20200905163403.64390-1-contact@artur-rojek.eu
State Accepted
Commit 7956b0d4694fb4bf75c4b1b4bcb6cf7092bd5195
Headers show
Series [v9,1/2] dt-bindings: input: Add docs for ADC driven joystick. | expand

Commit Message

Artur Rojek Sept. 5, 2020, 4:34 p.m. UTC
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

Comments

Andy Shevchenko Sept. 6, 2020, 9:22 a.m. UTC | #1
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;
> +               }
> +       }
Dmitry Torokhov Sept. 14, 2020, 8:34 p.m. UTC | #2
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.
diff mbox series

Patch

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>;
+      };
+    };