Hi Krzysztof, On 19:10 Thu 31 Oct , Krzysztof Kozlowski wrote: > On 31/10/2024 15:07, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 08:26 Tue 29 Oct , Krzysztof Kozlowski wrote: > >> On Mon, Oct 28, 2024 at 03:07:19PM +0100, Andrea della Porta wrote: > >>> Add device tree bindings for the gpio/pin/mux controller that is part of > >>> the RP1 multi function device, and relative entries in MAINTAINERS file. > >>> > >>> Signed-off-by: Andrea della Porta <andrea.porta@xxxxxxxx> > >>> --- > >>> .../pinctrl/raspberrypi,rp1-gpio.yaml | 163 ++++++++++++++++++ > >>> MAINTAINERS | 2 + > >>> 2 files changed, 165 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > >>> > >>> diff --git a/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > >>> new file mode 100644 > >>> index 000000000000..465a53a6d84f > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > >>> @@ -0,0 +1,163 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/pinctrl/raspberrypi,rp1-gpio.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: RaspberryPi RP1 GPIO/Pinconf/Pinmux Controller submodule > >>> + > >>> +maintainers: > >>> + - Andrea della Porta <andrea.porta@xxxxxxxx> > >>> + > >>> +description: > >>> + The RP1 chipset is a Multi Function Device containing, among other sub-peripherals, > >>> + a gpio/pinconf/mux controller whose 54 pins are grouped into 3 banks. It works also > >> > >> Please wrap code according to coding style (checkpatch is not a coding > >> style description but only a tool). > > > > Ack. > > > >> > >>> + as an interrupt controller for those gpios. > >>> + > >>> +properties: > >>> + compatible: > >>> + const: raspberrypi,rp1-gpio > >>> + > >>> + reg: > >>> + maxItems: 3 > >>> + description: One reg specifier for each one of the 3 pin banks. > >>> + > >>> + '#gpio-cells': > >>> + description: The first cell is the pin number and the second cell is used > >>> + to specify the flags (see include/dt-bindings/gpio/gpio.h). > >>> + const: 2 > >>> + > >>> + gpio-controller: true > >>> + > >>> + gpio-ranges: > >>> + maxItems: 1 > >>> + > >>> + gpio-line-names: > >>> + maxItems: 54 > >>> + > >>> + interrupts: > >>> + maxItems: 3 > >>> + description: One interrupt specifier for each one of the 3 pin banks. > >>> + > >>> + '#interrupt-cells': > >>> + description: > >>> + Specifies the Bank number [0, 1, 2] and Flags as defined in > >>> + include/dt-bindings/interrupt-controller/irq.h. > >>> + const: 2 > >>> + > >>> + interrupt-controller: true > >>> + > >>> +additionalProperties: > >> > >> Not much improved. You are supposed to have here pattern, just like > >> other bindings. I asked for this last time. > >> > >> And there are examples using it - almost all or most of pinctrl > >> bindings, including bindings having subnodes (but you do not use such > >> case here). > > > > This is the same approach used in [1], which seems quite recent. I did't > > 2021, so not that recent, but you are right that it's not the example I > would recommend. See rather: > git grep pins -- Documentation/devicetree/bindings/pinctrl/ | grep '\$' > > > pins, groups, states, etc. Perfect. Thanks for the example suggestion. > > > use pattern because I wouldn't really want to enforce a particular naming > > scheme. Subnodes are used, please see below. Since pinctrl.yaml explicitly > > But we want to enforce, because it brings uniformity and matches > partially generic naming patterns. Ack. > > > says that there is no common binding but each device has its own, I > > thought that was reasonable choice. Should I enforce some common pattern, > > then? > > Yes, you should. Again, look at other bindings, e.g. qcom tlmm or lpass lpi. Ok. > > > > >> > >>> + anyOf: > >>> + - type: object > >>> + additionalProperties: false > >>> + allOf: > >>> + - $ref: pincfg-node.yaml# > >>> + - $ref: pinmux-node.yaml# > >>> + > >>> + description: > >>> + Pin controller client devices use pin configuration subnodes (children > >>> + and grandchildren) for desired pin configuration. > >>> + Client device subnodes use below standard properties. > >>> + > >>> + properties: > >>> + pins: > >>> + description: > >>> + A string (or list of strings) adhering to the pattern 'gpio[0-5][0-9]' > >>> + function: true > >>> + bias-disable: true > >>> + bias-pull-down: true > >>> + bias-pull-up: true > >>> + slew-rate: > >>> + description: 0 is slow slew rate, 1 is fast slew rate > >>> + enum: [ 0, 1 ] > >>> + drive-strength: > >>> + enum: [ 2, 4, 8, 12 ] > >>> + > >>> + - type: object > >>> + additionalProperties: > >>> + $ref: "#/additionalProperties/anyOf/0" > >> > >> Your example does not use any subnodes, so this looks not needed. > > > > The example has subnodes, as in the following excerpt from the example: > > I meant, you do not need properties in subnodes (1st level). You only > want properties in subnodes of subnodes, so 2nd level. What is the point > of allowing them in 1st level? I will add those two sub-nodes to the example: rp1-i2s0-default-state { function = "i2s0"; pins = "gpio18", "gpio19", "gpio20", "gpio21"; bias-disable; }; rp1-uart0-default-state { txd-pins { function = "uart0"; pins = "gpio14"; bias-disable; }; rxd-pins { function = "uart0"; pins = "gpio15"; bias-pull-up; }; }; The first is just a group of pins with the same settings, the second is a pin group with different settings per pin. This is basically the same usage as in qcom,sm4250-lpass-lpi-pinctrl.yaml. Many thanks, Andrea > > > > Best regards, > Krzysztof >
