Hi Rob, Thanks for your feedback! > From: Rob Herring <robh@xxxxxxxxxx> > Sent: 14 December 2022 16:11 > To: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx> > Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver > bindings > > On Tue, Dec 13, 2022 at 10:43:06PM +0000, Fabrizio Castro wrote: > > Add dt-bindings document for the RZ/V2M PWC GPIO driver. > > Bindings are for h/w blocks/devices, not a specific driver. Apologies, I will reword the changelog in v2. > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx> > > --- > > .../bindings/gpio/renesas,rzv2m-pwc-gpio.yaml | 62 +++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml > > > > diff --git a/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc- > gpio.yaml b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc- > gpio.yaml > > new file mode 100644 > > index 000000000000..ecc034d53259 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/renesas,rzv2m-pwc-gpio.yaml > > @@ -0,0 +1,62 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre > e.org%2Fschemas%2Fgpio%2Frenesas%2Crzv2m-pwc- > gpio.yaml%23&data=05%7C01%7Cfabrizio.castro.jz%40renesas.com%7C603623c > 766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638 > 066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz > IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=o46ncDZK8YK5HYJ > ZYDXuq3yfEA34vnaxEsIDBlcroc0%3D&reserved=0 > > +$schema: > https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre > e.org%2Fmeta- > schemas%2Fcore.yaml%23&data=05%7C01%7Cfabrizio.castro.jz%40renesas.com > %7C603623c766f4421b85bd08daddedcb8c%7C53d82571da1947e49cb4625a166a4a2a%7C0 > %7C0%7C638066310628408926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=VoWvV > pW782DVH2zdTKIesyzqm6sjiFyacbl833%2BjRis%3D&reserved=0 > > + > > +title: Renesas RZ/V2M External Power Sequence Controller (PWC) GPIO > > + > > +description: |+ > > + The PWC IP found in the RZ/V2M family of chips comes with General- > Purpose > > + Output pins, alongside the below functions > > + - external power supply on/off sequence generation > > + - on/off signal generation for the LPDDR4 core power supply (LPVDD) > > + - key input signals processing > > + This node uses syscon to map the register used to control the GPIOs > > + (the register map is retrieved from the parent dt-node), and the node > should > > + be represented as a sub node of a "syscon", "simple-mfd" node. > > + > > +maintainers: > > + - Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx> > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - renesas,r9a09g011-pwc-gpio # RZ/V2M > > + - renesas,r9a09g055-pwc-gpio # RZ/V2MA > > + - const: renesas,rzv2m-pwc-gpio > > + > > + offset: > > Too generic of a name. We want any given property name (globally) to > have 1 type. With the below comment, this should be replaced with 'reg' > instead if you have child nodes. My understanding is that syscon subnodes normally use this name for exactly the same purpose, for example: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-poweroff.yaml#L27 https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml#L30 What am I missing? > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: | > > + Offset in the register map for controlling the GPIOs (in bytes). > > + > > + regmap: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: Phandle to the register map node. > > Looks like GPIO is a sub-function of some other block. Define the > binding for that entire block. That's defined in patch 3 from this series. I have sent it as patch 3 because that document references: * /schemas/gpio/renesas,rzv2m-pwc-gpio.yaml * /schemas/power/reset/renesas,rzv2m-pwc-poweroff.yaml Which are defined in this patch and in patch 2 in the series. Do you want me to move patch 3 to patch 1 in v2? > GPIO can be either either a function of > that node (just add GPIO provider properties) or you can have GPIO child > nodes. Depends on what the entire block looks like to decide. Do you > have multiple instances of the GPIO block would be one reason to have > child nodes. >From a pure HW point of view, this GPIO block is contained inside the PWC block (as PWC is basically a MFD device), and it only deals with 2 General-Purpose Output pins, both controlled by 1 (and the same) register, therefore the GPIO block is only 1 child. If possible, I would like to keep the functionality offered by the sub-blocks of PWC contained in separated drivers and DT nodes (either non-child nodes or child nodes). My understanding is that simple-mfd will automatically probe the children of the simple-mfd node, and also hierarchically it makes sense to me to have the DT nodes of the PWC sub-blocks as children of the "syscon", "simple-mfd" node. I have found other instances of this same architecture in the kernel already (plenty from NXP/Freescale), for example: https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mm.dtsi#L585 https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mn.dtsi#L586 https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mp.dtsi#L451 https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mq.dtsi#L616 https://github.com/torvalds/linux/blob/master/arch/mips/boot/dts/mti/sead3.dts#L93 etc... Something like the below could also work, but I don't think it would represent the HW accurately: pwc: pwc@a3700000 { compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc", "syscon", "simple-mfd"; reg = <0 0xa3700000 0 0x800>; }; pwc-gpio { compatible = "renesas,r9a09g011-pwc-gpio", "renesas,rzv2m-pwc-gpio"; regmap = <&pwc>; gpio-controller; #gpio-cells = <2>; }; pwc-poweroff { compatible = "renesas,r9a09g011-pwc-poweroff", "renesas,rzv2m-pwc-poweroff"; regmap = <&pwc>; }; I think the below describes things better: pwc: pwc@a3700000 { compatible = "renesas,r9a09g011-pwc", "renesas,rzv2m-pwc", "syscon", "simple-mfd"; reg = <0 0xa3700000 0 0x800>; gpio { compatible = "renesas,r9a09g011-pwc-gpio", "renesas,rzv2m-pwc-gpio"; regmap = <&pwc>; offset = <0x80>; gpio-controller; #gpio-cells = <2>; }; poweroff { compatible = "renesas,r9a09g011-pwc-poweroff", "renesas,rzv2m-pwc-poweroff"; regmap = <&pwc>; }; }; Do you think the bindings I have sent out are causing confusion here? What else can I improve? Thanks, Fab > > > + > > + gpio-controller: true > > + > > + '#gpio-cells': > > + const: 2 > > + > > +required: > > + - compatible > > + - regmap > > + - offset > > + - gpio-controller > > + - '#gpio-cells' > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + gpio { > > + compatible = "renesas,r9a09g011-pwc-gpio", > > + "renesas,rzv2m-pwc-gpio"; > > + regmap = <®mapnode>; > > + offset = <0x80>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > -- > > 2.34.1 > > > >
