RE: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver bindings

[Fabrizio Castro <fabrizio.castro.jz@renesas.com>]

Hi Krzysztof,

Thanks for your feedback.

> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 15 December 2022 09:49
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Rob Herring
> <robh@kernel.org>
> Subject: Re: [PATCH 1/5] dt-bindings: gpio: Add RZ/V2M PWC GPIO driver
> bindings
> 
> On 14/12/2022 19:26, Fabrizio Castro wrote:
> > Hi Rob,
> >
> > Thanks for your feedback!
> >
> >> From: Rob Herring <robh@kernel.org>
> >> Sent: 14 December 2022 16:11
> >> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> >> 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@renesas.com>
> >>> ---
> >>>  .../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
> >>> +---
> >>> +
> >>> +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@renesas.com>
> >>> +
> >>> +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:
> >
> >
> > What am I missing?
> 
> These are generic drivers, so they need offset as they do not match any
> specific programming model. You are not making a generic device. Address
> offsets are not suitable in most cases for DTS. There are of course
> exceptions so you can present reasons why this one is exception.

Thanks for the explanation

> >
> >>
> >>> +    $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?
> 
> We do not want regmap, but proper definition of entire hardware.

Will do. I'll drop regmap.

> 
> >
> >> 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).
> 
> Driver do not matter for bindings. We talk about regmap field which - as
> you explained above - is not needed.

Okay, I'll rework, and I'll send v2.

Thanks,
Fab

> 
> 
> >
> > 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:
> 
> I don't understand. You do not have here simple-mfd and it still does
> not explain Rob's comment and regmap.
> 
> >
> > 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>;
> 
> You speak about two different things. So again - drop regmap. You do not
> need it.
> 
> > 		offset = <0x80>;
> > 		gpio-controller;
> > 		#gpio-cells = <2>;
> > 	};
> >
> > 	poweroff {
> > 		compatible = "renesas,r9a09g011-pwc-poweroff",
> > 			     "renesas,rzv2m-pwc-poweroff";
> > 		regmap = <&pwc>;
> 
> Drop regmap.
> 
> > 	};
> > };
> >
> 
> Best regards,
> Krzysztof