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

[Levin]

Rob Herring <robh@xxxxxxxxxx> writes:

> On Sat, Jun 02, 2018 at 04:40:09PM +0800, Levin Du wrote:
>> 
>> 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:
>
> I'm only talking about GRF functions, not "regular" GPIOs.
>
>> 	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.
>
> What I meant was "only GPIO in GRF registers"...
>
> Rob

I check the TRM and schematic once again. In GRF resters, there are also
HDMI GPIOs, which are already covered by the HDMI driver. Aside from
those, MUTE_GPIO is the only GPIO.

Levin