| 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
| ||
|
Message-ID: <58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org>
Date: Sat, 21 Dec 2024 21:34:17 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>,
Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Kamel Bouhara <kamel.bouhara@...tlin.com>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
<brgl@...ev.pl>, Dmitry Torokhov <dmitry.torokhov@...il.com>,
Uwe Kleine-König <ukleinek@...nel.org>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-input@...r.kernel.org,
linux-pwm@...r.kernel.org, Grégory Clement
<gregory.clement@...tlin.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices
On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
> ---
> .../devicetree/bindings/gpio/max7360-gpio.yaml | 96 ++++++++++++++++++++++
> .../devicetree/bindings/input/max7360-keypad.yaml | 67 +++++++++++++++
> .../devicetree/bindings/input/max7360-rotary.yaml | 52 ++++++++++++
> .../devicetree/bindings/pwm/max7360-pwm.yaml | 35 ++++++++
> 4 files changed, 250 insertions(+)
I don't understand how this patchset was split. MFD binding cannot be
empty and cannot be before child devices.
All filenames are wrong here: use compatibles.
>
> diff --git a/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> new file mode 100644
> index 000000000000..3c006dc0380b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/max7360-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 GPIO controller
> +
> +maintainers:
> + - Kamel Bouhara <kamel.bouhara@...tlin.com>
> + - Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
> +
> +description: |
> + Maxim MAX7360 GPIO controller, in MAX7360 MFD
> + https://www.analog.com/en/products/max7360.html
> +
> +properties:
> + compatible:
> + enum:
> + - maxim,max7360-gpio
> + - maxim,max7360-gpo
Why? What are the differences?
> +
> + gpio-controller: true
> +
> + "#gpio-cells":
> + const: 2
> +
> + ngpios:
> + minimum: 0
> + maximum: 8
Why this is flexible?
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 2
> +
> + interrupts:
> + description: The interrupt line the device is connected to.
Drop
> + maxItems: 1
> +
> + constant-current-disable:
You always need vendor prefix.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Bit field, each bit disables constant-current output of the
> + associated GPIO.
Oddly aligned.
Missing constraints.
> +
> +
> +required:
> + - compatible
> + - gpio-controller
> + - ngpios
> +
> +if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - maxim,max7360-gpio
> +then:
> + required:
> + - interrupt-controller
> + - interrupts
> +else:
> + properties:
> + interrupt-controller: false
> + interrupts: false
> + constant-current-disable: false
> +
> + ngpios:
> + maximum: 6
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + max7360_gpo: max7360_gpo {
Plaese follow DTS coding style.
Keep only one, complete example.
> + compatible = "maxim,max7360-gpo";
> + gpio-controller;
> + #gpio-cells = <0x2>;
> + ngpios = <4>;
Odd indentation. Your MFD patch had very different one.
> + };
> +
> + max7360_gpio: max7360_gpio {
> + compatible = "maxim,max7360-gpio";
> +
> + gpio-controller;
> + #gpio-cells = <0x2>;
> + ngpios = <8>;
> + constant-current-disable = <0x06>;
> +
> + interrupt-controller;
> + #interrupt-cells = <0x2>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> + };
> diff --git a/Documentation/devicetree/bindings/input/max7360-keypad.yaml b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
> new file mode 100644
> index 000000000000..8bc3c841465b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/max7360-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 Keypad Controller
> +
> +maintainers:
> + - Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
> +
> +description: |
> + Maxim MAX7360 Keypad Controller, in MAX7360 MFD
> + https://www.analog.com/en/products/max7360.html
> +
> +allOf:
> + - $ref: matrix-keymap.yaml#
> + - $ref: input.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - maxim,max7360-keypad
> +
> + interrupts:
> + description: The interrupt line the device is connected to.
Really? Each separate device has its own interrupt line? How is it
possible if diagram here:
https://www.analog.com/en/products/max7360.html
has only one interrupt?
Fold the binding into the parent node.
> + maxItems: 1
> +
> + debounce-delay-ms:
> + description: Debounce delay in ms
> + minimum: 9
> + maximum: 40
> + default: 9
> +
> + linux,input-no-autorepeat:
> + description: If present, the keys will not autorepeat when pressed
> +
> +required:
> + - compatible
> + - interrupts
> + - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/input/input.h>
> +
> + max7360_keypad {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Please read and follow DTS coding style.
> + compatible = "maxim,max7360-keypad";
> +
> + keypad,num-rows = <8>;
> + keypad,num-columns = <4>;
> +
> + linux,keymap = <
> + MATRIX_KEY(0x00, 0x00, KEY_F5)
> + MATRIX_KEY(0x01, 0x00, KEY_F4)
> + MATRIX_KEY(0x02, 0x01, KEY_F6)
> + >;
> +
> + interrupt-parent = <&gpio1>;
> + interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> +
> + debounce-delay-ms = <10>;
> + linux,input-no-autorepeat;
> + };
> diff --git a/Documentation/devicetree/bindings/input/max7360-rotary.yaml b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
> new file mode 100644
> index 000000000000..19afa8344249
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/max7360-rotary.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 Rotary Encoder
> +
> +maintainers:
> + - Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
> +
> +description: |
> + Maxim MAX7360 Rotary Encoder, in MAX7360 MFD
> + https://www.analog.com/en/products/max7360.html
> +
> +properties:
> + compatible:
> + enum:
> + - maxim,max7360-rotary
> +
> + interrupts:
> + description: The interrupt line the device is connected to.
> + maxItems: 1
> +
> + debounce-delay-ms:
> + description: Debounce delay in ms
> + minimum: 0
> + maximum: 15
> + default: 0
> +
> + linux,axis:
> + description: The input subsystem axis to map to this rotary encoder.
> +
Fold into parent node.
> +description: |
> + Maxim MAX7360 PWM controller, in MAX7360 MFD
> + https://www.analog.com/en/products/max7360.html
> +
> +properties:
> + compatible:
> + enum:
> + - maxim,max7360-pwm
> +
> + "#pwm-cells":
> + const: 2
> +
> +
Fold into parent.
Best regards,
Krzysztof
Powered by blists - more mailing lists