Re: [PATCH v3 1/7] dt-bindings: mfd: gpio: Add MAX7360

[Mathieu Dubois-Briand]

On Tue Jan 14, 2025 at 9:11 AM CET, Krzysztof Kozlowski wrote:
> On Mon, Jan 13, 2025 at 01:42:25PM +0100, Mathieu Dubois-Briand wrote:
> > +
> > +  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.
>
> Missing type. I guess you wanted to reference rotary encoder schema,
> next to input and matrix-keymap?
>

I'm not sure I fully understood your suggestion. Do you mean adding a
reference to rotary-encoder.yaml, at the root of the document? Like:

allOf:
  - $ref: /schemas/input/matrix-keymap.yaml#
  - $ref: /schemas/input/input.yaml#
  - $ref: /schemas/input/rotary-encoder.yaml#

I did base the schema of the rotary encoder part on rotary-encoder.yaml,
but I believe we cannot reference it directly: it adds some properties
that do not make sense here (gpios, rotary-encoder,steps...) and also
some of them are mandatory.

Yet I see that I'm not referring to any type here. Also I did not
specify the default value. Would the following be OK?

  linux,axis:
    description: The input subsystem axis to map to the rotary encoder.
    $ref: /schemas/types.yaml#/definitions/uint32
    default: 0

> > +      COL2 to COL7 general purpose output pins configuration.
> > +      Allows to use unused keypad columns as outputs.
> > +      The MAX7360 has 8 column lines and 6 of them can be used as GPOs. Value
> > +      of ngpios must be coherent with the value of keypad,num-columns, as their
> > +      sum must not exceed the number of physical lines.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - linux,keymap
> > +  - linux,axis
> > +  - "#pwm-cells"
>
> gpio and gpo nodes are optional? How would the driver behave? I assume
> you need to define the partition between GPIOs, especially that 'ngpios'
> are a required property in their schema.
>

No, you are right. In my mind it was optional, but current driver
implementation will complain if the gpo node is missing. I could make it
optional in the code, but it's probably better to make it required in
the device tree, so the hardware is correctly described.

> > 
> > -- 
> > 2.39.5
> > 

I have fixed the other points listed in your mail. Thanks for your review.


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