Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices

["Mathieu Dubois-Briand" <mathieu.dubois-briand@xxxxxxxxxxx>]

On Tue Dec 24, 2024 at 10:00 AM CET, Krzysztof Kozlowski wrote:
> On 23/12/2024 16:20, Mathieu Dubois-Briand wrote:
> > On Sat Dec 21, 2024 at 9:34 PM CET, Krzysztof Kozlowski wrote:
> >> On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
> >>> ---
> >>> 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@xxxxxxxxxxx>
> >>> +  - Mathieu Dubois-Briand <mathieu.dubois-briand@xxxxxxxxxxx>
> >>> +
> >>> +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?
> >>
> > 
> > Ok, so maybe my approach here is completely wrong. I'm not sure what
> > would be the best way to describe the device here, if you have any
> > suggestion I would be happy to use it. Let me try to summarize the GPIO
> > setup of the chip.
> > 
> > First we have two series of GPIOs on the chips, which I tend to think
> > about as two separate "banks". Thus two separate subnodes of the max7360
> > node.
>
> First, splitting MFD device into multiple children is pretty often wrong
> approach because it tries to mimic Linux driver design.
>
> Such split in DT makes sense if these are really separate blocks, e.g.
> separate I2C addresses, re-usable on different designs.
>
> In this case Functional Block Diagram shows separate blocks, but still
> the same I2C block. This can be one device. This can be also two devices
> if that's easier to represent in DT.

Ok, I get it. So I could try to remove the children, but I'm not really
sure about the way to go:
- About the two series of GPIOs, how should I represent them? In a
  continuous way, like 0-7 is gpios and 8+ is gpos? Or maybe setting
  #gpio-cells to 3 and using the added cell to select between gpios and
  gpos ?
- About the interrupt-controller: today we have a children where all
  gpios have a corresponding interrupt and another one without any
  interrupt. If I remove the children, we will have a mix of both. I
  don't think there is anything preventing to do this, but is this ok?

So I'm keeping the two children for now, but I'm open to the possibility
of removing them.

>
> But in any case binding description should explain this.
>

Ok, I will add some documentation.

> > 
> > - On one side we have what I refer to as GPIOs, here with
> >   maxim,max7360-gpio:
> >   - PORT0 to PORT7 pins of the chip.
> >   - Shared with PWM and rotary encoder functionalities. Functionality
> >     selection can be made independently for each pin. This selection is
> >     not described here. Runtime will have to ensure the same pin is not
> >     used by two drivers at the same time. E.g. we cannot have at the
> >     same time GPIO4 and PWM4.
> >   - Supports input and interrupts.
> >   - Outputs may be configured as constant current.
> >   - 8 GPIOS supported, so ngpios maximum is 8. Thinking about it now, we
> >     should probably also set minimum to 8, I don't see any reason to
> >     have ngpios set to something less.
> > 
> > On the other side, we have what I refer to as GPOs, here with
> > maxim,max7360-gpo compatible:
> >   - COL2 to COL7 pins of the chip.
> >   - Shared with the keypad functionality. Selections is made by
> >     partitioning the pins: first pins for keypad columns, last pins for
> >     GPOs. Partition is described here by ngpios and on keypad node by
> >     keypad,num-columns. Runtime will have to ensure values are coherent
> >     and configure the chip accordingly.
> >   - Only support outputs.
> >   - No support for constant current mode.
> >   - Supports 0 to 6 GPOs, so ngpios maximum is 6.
> > 
> >>> +
> >>> +  gpio-controller: true
> >>> +
> >>> +  "#gpio-cells":
> >>> +    const: 2
> >>> +
> >>> +  ngpios:
> >>> +    minimum: 0
> >>> +    maximum: 8
> >>
> >> Why this is flexible?
> >>
> > 
> > I believe this makes sense, as this keypad/gpos partition really changes
> > the actual number of GPIOS. Yet we could argue that this is just runtime
> > configuration. Tell me what you think about it, if you think this should
> > be a fixed value, I will find a way.
>
> Depends whether this is actual runtime configuration. If you configure
> keypad in DT, then the pins go away from GPIOs (especially considering
> that board might have these pins really connected to keypad). Anyway,
> explain this briefly in binding description.

Keypad is configured in DT and yes, the pins partition is a consequence
of the hardware implementation on the board. So on second thought I
believe this is cannot be a runtime configuration and should be
described in the DT.

I will add some documentation about it.

>
> > 
> Best regards,
> Krzysztof

Thanks again for your review.
Mathieu

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