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
