Message-Id: <D6JXDWUG8PIC.1HWXX7RCA9N6E@bootlin.com>
Date: Tue, 24 Dec 2024 13:22:06 +0100
From: "Mathieu Dubois-Briand" <mathieu.dubois-briand@...tlin.com>
To: "Krzysztof Kozlowski" <krzk@...nel.org>, "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 v2 1/7] dt-bindings: mfd: gpio: Add MAX7360

On Tue Dec 24, 2024 at 10:12 AM CET, Krzysztof Kozlowski wrote:
> On 23/12/2024 17:42, Mathieu Dubois-Briand wrote:
> > Add device tree bindings for Maxim Integrated MAX7360 device with
> > support for keypad, rotary, gpios and pwm functionalities.
> > 
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
> > ---
> >  .../bindings/gpio/maxim,max7360-gpio.yaml          |  80 +++++++++++++++
> >  .../devicetree/bindings/mfd/maxim,max7360.yaml     | 107 +++++++++++++++++++++
> >  2 files changed, 187 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
> > new file mode 100644
> > index 000000000000..6e6133ce6e68
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/maxim,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 chipset
> > +  https://www.analog.com/en/products/max7360.html
>
> Don't send new versions so fast, especially for larger patchsets, so we
> can finish previous discussion.
>
> You gave me yesterday around 1 hour to respond to your last email and
> then you sent v2.
>

Soddy, I didn't mean to. I thought having a v2 would be easier to show
how I addressed the previous comments, but I definitely was too fast. I
will make sure to wait more next time.

> Please implement my last comments on v1.
>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - maxim,max7360-gpio
> > +      - maxim,max7360-gpo
>
>
> ...
>
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    gpio {
> > +      compatible = "maxim,max7360-gpio";
> > +
> > +      gpio-controller;
> > +      #gpio-cells = <0x2>;
>
> Not a hex, <2>
>
> > +      ngpios = <8>;
> > +      maxim,constant-current-disable = <0x06>;
> > +
> > +      interrupt-controller;
> > +      #interrupt-cells = <0x2>;
>
> Not a hex, <2>
>

Ok, I will fix both values.

> > +      };
> > diff --git a/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
> > new file mode 100644
> > index 000000000000..1f761707070a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
> > @@ -0,0 +1,107 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/maxim,max7360.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Maxim MAX7360 Keypad, Rotary encoder, PWM and GPIO controller
> > +
> > +maintainers:
> > +  - Kamel Bouhara <kamel.bouhara@...tlin.com>
> > +  - Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
> > +
> > +description: |
> > +  Maxim MAX7360 device, with following functions:
> > +    - keypad controller
> > +    - rotary controller
> > +    - GPIO and GPO controller
> > +    - PWM controller
> > +
> > +  https://www.analog.com/en/products/max7360.html
> > +
> > +allOf:
> > +  - $ref: /schemas/input/matrix-keymap.yaml#
> > +  - $ref: /schemas/input/input.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - maxim,max7360
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    minItems: 2
>
> You can drop minItems.
>

Ok, fixed.

> > +    maxItems: 2
> > +> +  interrupt-names:
> > +    items:
> > +      - const: inti
> > +      - const: intk
> > +
> > +  keypad-debounce-delay-ms:
> > +    description: Keypad debounce delay in ms
> > +    minimum: 9
> > +    maximum: 40
> > +    default: 9
> > +
> > +  autorepeat: true
> > +
> > +  rotary-debounce-delay-ms:
> > +    description: Rotary encoder debounce delay in ms
> > +    minimum: 0
> > +    maximum: 15
> > +    default: 0
> > +
> > +  linux,axis:
> > +    description: The input subsystem axis to map to this rotary encoder.
> > +
> > +  "#pwm-cells":
> > +    const: 3
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - linux,keymap
> > +  - linux,axis
> > +  - "#pwm-cells"
> > +
> > +unevaluatedProperties: false
> > +
>
> Well, I still see it incomplete... and to prove it, please post your DTS
> for entire max7360 and validate it against bindings.
>
> There is no way this works, unless GPIO is not part of this device but
> then it is obviously incorrect design.
>

Ok, it looks like I completely missed how it was supposed to be
described. So, if I got it right, I need to:
- Add two properties, gpio and gpo, with $ref: /schemas/gpio/maxim,max7360-gpio.yaml#
- Add the two gpio and gpo child nodes in the example.

>
> Best regards,
> Krzysztof

Thanks for your review,
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists