Re: [PATCH v3 2/5] gpio: syscon: rockchip: add GPIO_MUTE support for rk3328

[Levin Du]

Rob Herring <robh+dt@xxxxxxxxxx> writes:

> On Thu, May 31, 2018 at 9:05 PM, Levin <djw@xxxxxxxxxxxxx> 
> wrote:
> > Hi Rob,
> >
> >
> > On 2018-05-31 10:45 PM, Rob Herring wrote:
> > > On Wed, May 30, 2018 at 10:27 PM,  <djw@xxxxxxxxxxxxx> wrote:
> > > > From: Levin Du <djw@xxxxxxxxxxxxx>
> > > >
> > > > In Rockchip RK3328, the output only GPIO_MUTE pin, originally 
> > > > for codec
> > > > mute control, can also be used for general purpose. It is 
> > > > manipulated by
> > > > the GRF_SOC_CON10 register.
> > > >
> > > > Signed-off-by: Levin Du <djw@xxxxxxxxxxxxx>
> > > >
> > > > ---
> > > >
> > > > Changes in v3:
> > > > - Change from general gpio-syscon to specific 
> > > > rk3328-gpio-mute
> > > >
> > > > Changes in v2:
> > > > - Rename gpio_syscon10 to gpio_mute in doc
> > > >
> > > > Changes in v1:
> > > > - Refactured for general gpio-syscon usage for Rockchip SoCs.
> > > > - Add doc rockchip,gpio-syscon.txt
> > > >
> > > >   .../bindings/gpio/rockchip,rk3328-gpio-mute.txt    | 28
> > > > +++++++++++++++++++
> > > >   drivers/gpio/gpio-syscon.c                         | 31
> > > > ++++++++++++++++++++++
> > > >   2 files changed, 59 insertions(+)
> > > >   create mode 100644
> > > > Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > > new file mode 100644
> > > > index 0000000..10bc632
> > > > --- /dev/null
> > > > +++
> > > > b/Documentation/devicetree/bindings/gpio/rockchip,rk3328-gpio-mute.txt
> > > > @@ -0,0 +1,28 @@
> > > > +Rockchip RK3328 GPIO controller dedicated for the GPIO_MUTE 
> > > > pin.
> > > > +
> > > > +In Rockchip RK3328, the output only GPIO_MUTE pin, 
> > > > originally for codec
> > > > mute
> > > > +control, can also be used for general purpose. It is 
> > > > manipulated by the
> > > > +GRF_SOC_CON10 register.
> > > > +
> > > > +Required properties:
> > > > +- compatible: Should contain "rockchip,rk3328-gpio-mute".
> > > > +- gpio-controller: Marks the device node as a gpio 
> > > > controller.
> > > > +- #gpio-cells: Should be 2. The first cell is the pin number 
> > > > and
> > > > +  the second cell is used to specify the gpio polarity:
> > > > +    0 = Active high,
> > > > +    1 = Active low.
> > > > +
> > > > +Example:
> > > > +
> > > > +       grf: syscon@ff100000 {
> > > > +               compatible = "rockchip,rk3328-grf", "syscon",
> > > > "simple-mfd";
> > > > +
> > > > +               gpio_mute: gpio-mute {
> > >
> > > Node names should be generic:
> > >
> > > gpio {
> > >
> > > This also means you can't add another GPIO node in the future 
> > > and
> > > you'll have to live with "rockchip,rk3328-gpio-mute" covering 
> > > more
> > > than 1 GPIO if you do need to add more GPIOs.
> >
> >
> > As the first line describes, this GPIO controller is dedicated 
> > for the
> > GPIO_MUTE pin.
> > There's only one GPIO pin in the GRF_SOC_CON10 register. 
> > Therefore the
> > gpio_mute
> > name is proper IMHO.
>
> It's how many GPIOs in the GRF, not this register. What I'm 
> saying is
> when you come along later to add another GPIO in the GRF, you 
> had
> better just add it to this same node. I'm not going to accept 
> another
> GPIO controller node within the GRF. You have the cells to 
> support
> more than 1, so it would only be a driver change. The compatible
> string would then not be ideally named at that point. But 
> compatible
> strings are just unique identifiers, so it doesn't really matter 
> what
> the string is.

I'll try my best to introduce the situation here. The GRF, 
GPIO0~GPIO3
are register blocks in the RK3328 Soc. The GPIO0~GPIO3 contain 
registers
for GPIO operations like reading/writing data, setting direction,
interruption etc, which corresponds to the GPIO banks 
(gpio0~gpio3)
defined in rk3328.dtsi:

	pinctrl: pinctrl {
		compatible = "rockchip,rk3328-pinctrl";
		rockchip,grf = <&grf>;
		#address-cells = <2>;
		#size-cells = <2>;
		ranges;

		gpio0: gpio0@ff210000 {
			compatible = "rockchip,gpio-bank";
			reg = <0x0 0xff210000 0x0 0x100>;
			interrupts = <GIC_SPI 51 
			IRQ_TYPE_LEVEL_HIGH>;
			clocks = <&cru PCLK_GPIO0>;

			gpio-controller;
			#gpio-cells = <2>;

			interrupt-controller;
			#interrupt-cells = <2>;
		};

		gpio1: gpio1@ff220000 {
               //...
		};

		gpio2: gpio2@ff230000 {
               //...
		};

		gpio3: gpio3@ff240000 {
               //...
		};
        }

However, these general GPIO pins has multiplexed functions and 
their
pull up/down and driving strength can also be configured. These 
settings
are manipulated by the GRF registers in pinctrl driver. Quoted 
from the
TRM, the GRF has the following function:

- IOMUX control
- Control the state of GPIO in power-down mode
- GPIO PAD pull down and pull up control
- Used for common system control
- Used to record the system state

Therefore the functions of the GRF are messy and scattered in 
different
nodes. The so-called GPIO_MUTE does not belong to GPIO0~GPIO3. It 
is
manipulated by the GRF_SOC_CON10 register in the GRF block.

> I'm being told both "this is the only GPIO" and "the GRF has too 
> many
> different functions for us to tell you what they all are". So 
> which is
> it?
>
> Rob

They are both true, but lack of context. See the above 
description.

Thanks,
Levin