Levin <d...@t-chip.com.cn> writes: > Rob Herring <r...@kernel.org> writes: > >> On Sat, Jun 02, 2018 at 04:40:09PM +0800, Levin Du wrote: >>> >>> Rob Herring <robh...@kernel.org> writes: >>> >>> > On Thu, May 31, 2018 at 9:05 PM, Levin <d...@t-chip.com.cn> wrote: >>> > > Hi Rob, >>> > > >>> > > >>> > > On 2018-05-31 10:45 PM, Rob Herring wrote: >>> > > > >>> > > > On Wed, May 30, 2018 at 10:27 PM, <d...@t-chip.com.cn> wrote: >>> > > > > >>> > > > > From: Levin Du <d...@t-chip.com.cn> >>> > > > > >>> > > > > 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 <d...@t-chip.com.cn> >>> > > > > >>> > > > > --- >>> > > > > >>> > > > > 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
Hi Rob, Is there anything I can do to move forward? I know that this patch is far from a perfect solution. But it can be refactored later on. Best Regards, Levin