Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin]

On 2018-05-24 8:18 PM, Heiko Stuebner wrote:

Hi Levin,

Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:

Hi all, I'd like to quote reply of Robin Murphy at
   http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html


I would suggest s/pin number/bit number in the associated GRF register/
here. At least in this RK3328 case there's only one pin, which isn't
numbered, and if you naively considered it pin 0 of this 'bank' you'd
already have the wrong number. Since we're dealing with the "random
SoC-specific controls" region of the GRF as opposed to the
relatively-consistent and organised pinmux parts, I don't think we
should rely on any assumptions about how things are laid out.

I was initially going to suggest a more specific compatible string as
well, but on reflection I think the generic "rockchip,gpio-syscon" for
basic "flip this single GRF bit" functionality actually is the right way
to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
total related to this pin - the enable, value, and some pull controls
(which I assume apply when the output is disabled) - if at some point in
future we *did* want to start explicitly controlling the rest of them
too, then would be a good time to define a separate
"rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
for that specialised functionality, independently of this basic one.


Shall we go the generic "rockchip,gpio-syscon" way, or the specific
   "rockchip,rk3328-gpio-mute" way? I prefer the former one.

The property of "gpio,syscon-dev" in gpio-syscon driver should be
documented.
Since the gpio controller is defined in the dtsi file, which inevitably
contains voodoo
register addresses. But at the board level dts file, there won't be more
register
addresses.

Past experience shows that the GRF is not really suitable for
generalization, as it's more of a dumping ground where chip designers
can put everything that's left over. This is especially true for
GRF_SOC_CONx registers, that really only contain pretty random bits.

So personally, I'd really prefer soc-specific compatibles as everywhere
else, instead of trying to push stuff into the devicetree that won't hold
up on future socs.



On 2018-05-24 3:53 AM, Rob Herring wrote:

On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:

On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:

On 2018-05-23 2:02 AM, Rob Herring wrote:

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41

++

drivers/gpio/gpio-syscon.c | 30



2 files changed, 71 insertions(+)
create mode 100644

Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git
a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.

There's no need for this child node. Just make the parent node a gpio
controller.

Rob

Hi Rob, it is not clear to me. Do you suggest that the grf node should be
a
gpio controller,
like below?

+grf: syscon at ff10 {
+compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
"syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin]

On 2018-05-24 8:18 PM, Heiko Stuebner wrote:

Hi Levin,

Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:

Hi all, I'd like to quote reply of Robin Murphy at
   http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html


I would suggest s/pin number/bit number in the associated GRF register/
here. At least in this RK3328 case there's only one pin, which isn't
numbered, and if you naively considered it pin 0 of this 'bank' you'd
already have the wrong number. Since we're dealing with the "random
SoC-specific controls" region of the GRF as opposed to the
relatively-consistent and organised pinmux parts, I don't think we
should rely on any assumptions about how things are laid out.

I was initially going to suggest a more specific compatible string as
well, but on reflection I think the generic "rockchip,gpio-syscon" for
basic "flip this single GRF bit" functionality actually is the right way
to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
total related to this pin - the enable, value, and some pull controls
(which I assume apply when the output is disabled) - if at some point in
future we *did* want to start explicitly controlling the rest of them
too, then would be a good time to define a separate
"rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
for that specialised functionality, independently of this basic one.


Shall we go the generic "rockchip,gpio-syscon" way, or the specific
   "rockchip,rk3328-gpio-mute" way? I prefer the former one.

The property of "gpio,syscon-dev" in gpio-syscon driver should be
documented.
Since the gpio controller is defined in the dtsi file, which inevitably
contains voodoo
register addresses. But at the board level dts file, there won't be more
register
addresses.

Past experience shows that the GRF is not really suitable for
generalization, as it's more of a dumping ground where chip designers
can put everything that's left over. This is especially true for
GRF_SOC_CONx registers, that really only contain pretty random bits.

So personally, I'd really prefer soc-specific compatibles as everywhere
else, instead of trying to push stuff into the devicetree that won't hold
up on future socs.



On 2018-05-24 3:53 AM, Rob Herring wrote:

On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:

On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:

On 2018-05-23 2:02 AM, Rob Herring wrote:

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41

++

drivers/gpio/gpio-syscon.c | 30



2 files changed, 71 insertions(+)
create mode 100644

Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git
a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.

There's no need for this child node. Just make the parent node a gpio
controller.

Rob

Hi Rob, it is not clear to me. Do you suggest that the grf node should be
a
gpio controller,
like below?

+grf: syscon at ff10 {
+compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
"syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338

So the gpio controller should definitly 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner  wrote:
> Hi Rob,
>
> Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
>> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
>> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
>> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
>> >> >>> From: Levin Du 
>> >> >>>
>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >> >>> which do not belong to the general pinctrl.
>> >> >>>
>> >> >>> Adding gpio-syscon support makes controlling regulator or
>> >> >>> LED using these special pins very easy by reusing existing
>> >> >>> drivers, such as gpio-regulator and led-gpio.
>> >> >>>
>> >> >>> Signed-off-by: Levin Du 
>> >> >>>
>> >> >>> ---
>> >> >>>
>> >> >>> 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,gpio-syscon.txt | 41
>> >> >>>
>> >> >>> ++
>> >> >>>
>> >> >>>   drivers/gpio/gpio-syscon.c | 30
>> >> >>>
>> >> >>> 
>> >> >>>
>> >> >>>   2 files changed, 71 insertions(+)
>> >> >>>   create mode 100644
>> >> >>>
>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> diff --git
>> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> new file mode 100644
>> >> >>> index 000..b1b2a67
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> @@ -0,0 +1,41 @@
>> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >> >>> +  the second cell is used to specify the gpio polarity:
>> >> >>> +0 = Active high,
>> >> >>> +1 = Active low.
>> >> >>
>> >> >> There's no need for this child node. Just make the parent node a gpio
>> >> >> controller.
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should 
>> >> > be
>> >> > a
>> >> > gpio controller,
>> >> > like below?
>> >> >
>> >> > +grf: syscon at ff10 {
>> >> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> >> > "syscon", "simple-mfd";
>> >>
>> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> >
>> > I would disagree quite a bit here. The grf are the "general register 
>> > files",
>> > a bunch of registers used for quite a lot of things, and so it seems
>> > among other users, also a gpio-controller for some more random pins
>> > not controlled through the regular gpio controllers.
>> >
>> > For a more fully stocked grf, please see
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>> >
>> > So the gpio controller should definitly also be a subnode.
>>
>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>> above, then yes that makes sense. But that's not what has been
>> presented. Please make some attempt at defining *all* the functions.
>> An actual binding would be nice, but I'll settle for just a list of
>> things. The list should have functions that have DT dependencies (like
>> clocks for phys in the above) because until you do, you don't need
>> child nodes.
>
> That's the problem with the Rockchip-GRF, you only realize its content
> when implementing specific features.
>
> Like on the rk3399 the table of the register-list of the GRF alone is 11
> pages long with the register details tables taking up another 230 pages.
> And functional description is often somewhat thin.

But surely one can scan thru it and have some clue what functions
there are. For example, does this chip have phy registers in GRF?

> So I'm not sure I fully understand what you're asking, but in general
> we define the bindings for sub-devices when tackling these individual
> components, see for example
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

Yes, and in that case it makes sense. The individual functions
themselves have resources defined in DT like clocks. What I don't want
to see are child nodes defining *only* a compatible and any 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner  wrote:
> Hi Rob,
>
> Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
>> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
>> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
>> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
>> >> >>> From: Levin Du 
>> >> >>>
>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >> >>> which do not belong to the general pinctrl.
>> >> >>>
>> >> >>> Adding gpio-syscon support makes controlling regulator or
>> >> >>> LED using these special pins very easy by reusing existing
>> >> >>> drivers, such as gpio-regulator and led-gpio.
>> >> >>>
>> >> >>> Signed-off-by: Levin Du 
>> >> >>>
>> >> >>> ---
>> >> >>>
>> >> >>> 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,gpio-syscon.txt | 41
>> >> >>>
>> >> >>> ++
>> >> >>>
>> >> >>>   drivers/gpio/gpio-syscon.c | 30
>> >> >>>
>> >> >>> 
>> >> >>>
>> >> >>>   2 files changed, 71 insertions(+)
>> >> >>>   create mode 100644
>> >> >>>
>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> diff --git
>> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> new file mode 100644
>> >> >>> index 000..b1b2a67
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> @@ -0,0 +1,41 @@
>> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >> >>> +  the second cell is used to specify the gpio polarity:
>> >> >>> +0 = Active high,
>> >> >>> +1 = Active low.
>> >> >>
>> >> >> There's no need for this child node. Just make the parent node a gpio
>> >> >> controller.
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should 
>> >> > be
>> >> > a
>> >> > gpio controller,
>> >> > like below?
>> >> >
>> >> > +grf: syscon at ff10 {
>> >> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> >> > "syscon", "simple-mfd";
>> >>
>> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> >
>> > I would disagree quite a bit here. The grf are the "general register 
>> > files",
>> > a bunch of registers used for quite a lot of things, and so it seems
>> > among other users, also a gpio-controller for some more random pins
>> > not controlled through the regular gpio controllers.
>> >
>> > For a more fully stocked grf, please see
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>> >
>> > So the gpio controller should definitly also be a subnode.
>>
>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>> above, then yes that makes sense. But that's not what has been
>> presented. Please make some attempt at defining *all* the functions.
>> An actual binding would be nice, but I'll settle for just a list of
>> things. The list should have functions that have DT dependencies (like
>> clocks for phys in the above) because until you do, you don't need
>> child nodes.
>
> That's the problem with the Rockchip-GRF, you only realize its content
> when implementing specific features.
>
> Like on the rk3399 the table of the register-list of the GRF alone is 11
> pages long with the register details tables taking up another 230 pages.
> And functional description is often somewhat thin.

But surely one can scan thru it and have some clue what functions
there are. For example, does this chip have phy registers in GRF?

> So I'm not sure I fully understand what you're asking, but in general
> we define the bindings for sub-devices when tackling these individual
> components, see for example
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

Yes, and in that case it makes sense. The individual functions
themselves have resources defined in DT like clocks. What I don't want
to see are child nodes defining *only* a compatible and any provider
properties (e.g. #gpio-cells). The only reason to do that is to make
Linux bind a 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Levin,

Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:
> Hi all, I'd like to quote reply of Robin Murphy at
>   http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html
> 
> >
> > I would suggest s/pin number/bit number in the associated GRF register/
> > here. At least in this RK3328 case there's only one pin, which isn't
> > numbered, and if you naively considered it pin 0 of this 'bank' you'd
> > already have the wrong number. Since we're dealing with the "random
> > SoC-specific controls" region of the GRF as opposed to the
> > relatively-consistent and organised pinmux parts, I don't think we
> > should rely on any assumptions about how things are laid out.
> >
> > I was initially going to suggest a more specific compatible string as
> > well, but on reflection I think the generic "rockchip,gpio-syscon" for
> > basic "flip this single GRF bit" functionality actually is the right way
> > to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
> > total related to this pin - the enable, value, and some pull controls
> > (which I assume apply when the output is disabled) - if at some point in
> > future we *did* want to start explicitly controlling the rest of them
> > too, then would be a good time to define a separate
> > "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
> > for that specialised functionality, independently of this basic one.
> 
> 
> Shall we go the generic "rockchip,gpio-syscon" way, or the specific
>   "rockchip,rk3328-gpio-mute" way? I prefer the former one.
> 
> The property of "gpio,syscon-dev" in gpio-syscon driver should be 
> documented.
> Since the gpio controller is defined in the dtsi file, which inevitably 
> contains voodoo
> register addresses. But at the board level dts file, there won't be more 
> register
> addresses.

Past experience shows that the GRF is not really suitable for
generalization, as it's more of a dumping ground where chip designers
can put everything that's left over. This is especially true for
GRF_SOC_CONx registers, that really only contain pretty random bits.

So personally, I'd really prefer soc-specific compatibles as everywhere
else, instead of trying to push stuff into the devicetree that won't hold
up on future socs.


> On 2018-05-24 3:53 AM, Rob Herring wrote:
> > On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
> >> Hi Rob, Levin,
> >>
> >> sorry for being late to the party.
> >>
> >> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >>> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
>  On 2018-05-23 2:02 AM, Rob Herring wrote:
> > On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> >> From: Levin Du 
> >>
> >> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >> which do not belong to the general pinctrl.
> >>
> >> Adding gpio-syscon support makes controlling regulator or
> >> LED using these special pins very easy by reusing existing
> >> drivers, such as gpio-regulator and led-gpio.
> >>
> >> Signed-off-by: Levin Du 
> >>
> >> ---
> >>
> >> 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,gpio-syscon.txt | 41
> >>
> >> ++
> >>
> >>drivers/gpio/gpio-syscon.c | 30
> >>
> >> 
> >>
> >>2 files changed, 71 insertions(+)
> >>create mode 100644
> >>
> >> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> new file mode 100644
> >> index 000..b1b2a67
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> @@ -0,0 +1,41 @@
> >> +* Rockchip GPIO support for GRF_SOC_CON registers
> >> +
> >> +Required properties:
> >> +- compatible: Should contain "rockchip,gpio-syscon".
> >> +- gpio-controller: Marks the device node as a gpio controller.
> >> +- #gpio-cells: Should be two. The first cell is the pin number and
> >> +  the second cell is used to specify the gpio polarity:
> >> +0 = Active high,
> >> +1 = Active low.
> > There's no need for this child node. Just make the parent node a gpio
> > controller.
> >
> > Rob
>  Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>  a
>  gpio controller,
>  like below?
> 
>  +grf: syscon at ff10 {
>  +compatible = 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Levin,

Am Donnerstag, 24. Mai 2018, 03:59:36 CEST schrieb Levin Du:
> Hi all, I'd like to quote reply of Robin Murphy at
>   http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html
> 
> >
> > I would suggest s/pin number/bit number in the associated GRF register/
> > here. At least in this RK3328 case there's only one pin, which isn't
> > numbered, and if you naively considered it pin 0 of this 'bank' you'd
> > already have the wrong number. Since we're dealing with the "random
> > SoC-specific controls" region of the GRF as opposed to the
> > relatively-consistent and organised pinmux parts, I don't think we
> > should rely on any assumptions about how things are laid out.
> >
> > I was initially going to suggest a more specific compatible string as
> > well, but on reflection I think the generic "rockchip,gpio-syscon" for
> > basic "flip this single GRF bit" functionality actually is the right way
> > to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
> > total related to this pin - the enable, value, and some pull controls
> > (which I assume apply when the output is disabled) - if at some point in
> > future we *did* want to start explicitly controlling the rest of them
> > too, then would be a good time to define a separate
> > "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
> > for that specialised functionality, independently of this basic one.
> 
> 
> Shall we go the generic "rockchip,gpio-syscon" way, or the specific
>   "rockchip,rk3328-gpio-mute" way? I prefer the former one.
> 
> The property of "gpio,syscon-dev" in gpio-syscon driver should be 
> documented.
> Since the gpio controller is defined in the dtsi file, which inevitably 
> contains voodoo
> register addresses. But at the board level dts file, there won't be more 
> register
> addresses.

Past experience shows that the GRF is not really suitable for
generalization, as it's more of a dumping ground where chip designers
can put everything that's left over. This is especially true for
GRF_SOC_CONx registers, that really only contain pretty random bits.

So personally, I'd really prefer soc-specific compatibles as everywhere
else, instead of trying to push stuff into the devicetree that won't hold
up on future socs.


> On 2018-05-24 3:53 AM, Rob Herring wrote:
> > On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
> >> Hi Rob, Levin,
> >>
> >> sorry for being late to the party.
> >>
> >> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >>> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
>  On 2018-05-23 2:02 AM, Rob Herring wrote:
> > On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> >> From: Levin Du 
> >>
> >> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >> which do not belong to the general pinctrl.
> >>
> >> Adding gpio-syscon support makes controlling regulator or
> >> LED using these special pins very easy by reusing existing
> >> drivers, such as gpio-regulator and led-gpio.
> >>
> >> Signed-off-by: Levin Du 
> >>
> >> ---
> >>
> >> 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,gpio-syscon.txt | 41
> >>
> >> ++
> >>
> >>drivers/gpio/gpio-syscon.c | 30
> >>
> >> 
> >>
> >>2 files changed, 71 insertions(+)
> >>create mode 100644
> >>
> >> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> new file mode 100644
> >> index 000..b1b2a67
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> @@ -0,0 +1,41 @@
> >> +* Rockchip GPIO support for GRF_SOC_CON registers
> >> +
> >> +Required properties:
> >> +- compatible: Should contain "rockchip,gpio-syscon".
> >> +- gpio-controller: Marks the device node as a gpio controller.
> >> +- #gpio-cells: Should be two. The first cell is the pin number and
> >> +  the second cell is used to specify the gpio polarity:
> >> +0 = Active high,
> >> +1 = Active low.
> > There's no need for this child node. Just make the parent node a gpio
> > controller.
> >
> > Rob
>  Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>  a
>  gpio controller,
>  like below?
> 
>  +grf: syscon at ff10 {
>  +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>  "syscon", "simple-mfd";
> >>> Yes, but 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Rob,

Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> >> >>> From: Levin Du 
> >> >>>
> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >> >>> which do not belong to the general pinctrl.
> >> >>>
> >> >>> Adding gpio-syscon support makes controlling regulator or
> >> >>> LED using these special pins very easy by reusing existing
> >> >>> drivers, such as gpio-regulator and led-gpio.
> >> >>>
> >> >>> Signed-off-by: Levin Du 
> >> >>>
> >> >>> ---
> >> >>>
> >> >>> 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,gpio-syscon.txt | 41
> >> >>>
> >> >>> ++
> >> >>>
> >> >>>   drivers/gpio/gpio-syscon.c | 30
> >> >>>
> >> >>> 
> >> >>>
> >> >>>   2 files changed, 71 insertions(+)
> >> >>>   create mode 100644
> >> >>>
> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>>
> >> >>> diff --git
> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> new file mode 100644
> >> >>> index 000..b1b2a67
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> @@ -0,0 +1,41 @@
> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >> >>> +
> >> >>> +Required properties:
> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >> >>> +  the second cell is used to specify the gpio polarity:
> >> >>> +0 = Active high,
> >> >>> +1 = Active low.
> >> >>
> >> >> There's no need for this child node. Just make the parent node a gpio
> >> >> controller.
> >> >>
> >> >> Rob
> >> >
> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> >> > a
> >> > gpio controller,
> >> > like below?
> >> >
> >> > +grf: syscon at ff10 {
> >> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> >> > "syscon", "simple-mfd";
> >>
> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
> >
> > I would disagree quite a bit here. The grf are the "general register files",
> > a bunch of registers used for quite a lot of things, and so it seems
> > among other users, also a gpio-controller for some more random pins
> > not controlled through the regular gpio controllers.
> >
> > For a more fully stocked grf, please see
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
> >
> > So the gpio controller should definitly also be a subnode.
> 
> Sigh, yes, if there are a bunch of functions needing subnodes like the
> above, then yes that makes sense. But that's not what has been
> presented. Please make some attempt at defining *all* the functions.
> An actual binding would be nice, but I'll settle for just a list of
> things. The list should have functions that have DT dependencies (like
> clocks for phys in the above) because until you do, you don't need
> child nodes.

That's the problem with the Rockchip-GRF, you only realize its content
when implementing specific features. 

Like on the rk3399 the table of the register-list of the GRF alone is 11
pages long with the register details tables taking up another 230 pages.
And functional description is often somewhat thin.

So I'm not sure I fully understand what you're asking, but in general
we define the bindings for sub-devices when tackling these individual
components, see for example
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

which also has a real phy-driver behind it and binding against that
subnode of the GRF simple-mfd.

These are real IP blocks somewhere on the socs, with regular supplies
like resets, clocks etc in most cases. Only their controlling registers
got dumped into the GRF for some reason.

And in retrospect it really looks like we're doing something right,
because it seems these bindings seem quite stable over time.


> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stuebner]

Hi Rob,

Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> >> >>> From: Levin Du 
> >> >>>
> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >> >>> which do not belong to the general pinctrl.
> >> >>>
> >> >>> Adding gpio-syscon support makes controlling regulator or
> >> >>> LED using these special pins very easy by reusing existing
> >> >>> drivers, such as gpio-regulator and led-gpio.
> >> >>>
> >> >>> Signed-off-by: Levin Du 
> >> >>>
> >> >>> ---
> >> >>>
> >> >>> 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,gpio-syscon.txt | 41
> >> >>>
> >> >>> ++
> >> >>>
> >> >>>   drivers/gpio/gpio-syscon.c | 30
> >> >>>
> >> >>> 
> >> >>>
> >> >>>   2 files changed, 71 insertions(+)
> >> >>>   create mode 100644
> >> >>>
> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>>
> >> >>> diff --git
> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> new file mode 100644
> >> >>> index 000..b1b2a67
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >> >>> @@ -0,0 +1,41 @@
> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >> >>> +
> >> >>> +Required properties:
> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >> >>> +  the second cell is used to specify the gpio polarity:
> >> >>> +0 = Active high,
> >> >>> +1 = Active low.
> >> >>
> >> >> There's no need for this child node. Just make the parent node a gpio
> >> >> controller.
> >> >>
> >> >> Rob
> >> >
> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> >> > a
> >> > gpio controller,
> >> > like below?
> >> >
> >> > +grf: syscon at ff10 {
> >> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> >> > "syscon", "simple-mfd";
> >>
> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
> >
> > I would disagree quite a bit here. The grf are the "general register files",
> > a bunch of registers used for quite a lot of things, and so it seems
> > among other users, also a gpio-controller for some more random pins
> > not controlled through the regular gpio controllers.
> >
> > For a more fully stocked grf, please see
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
> >
> > So the gpio controller should definitly also be a subnode.
> 
> Sigh, yes, if there are a bunch of functions needing subnodes like the
> above, then yes that makes sense. But that's not what has been
> presented. Please make some attempt at defining *all* the functions.
> An actual binding would be nice, but I'll settle for just a list of
> things. The list should have functions that have DT dependencies (like
> clocks for phys in the above) because until you do, you don't need
> child nodes.

That's the problem with the Rockchip-GRF, you only realize its content
when implementing specific features. 

Like on the rk3399 the table of the register-list of the GRF alone is 11
pages long with the register details tables taking up another 230 pages.
And functional description is often somewhat thin.

So I'm not sure I fully understand what you're asking, but in general
we define the bindings for sub-devices when tackling these individual
components, see for example
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

which also has a real phy-driver behind it and binding against that
subnode of the GRF simple-mfd.

These are real IP blocks somewhere on the socs, with regular supplies
like resets, clocks etc in most cases. Only their controlling registers
got dumped into the GRF for some reason.

And in retrospect it really looks like we're doing something right,
because it seems these bindings seem quite stable over time.


> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> > all the register voodoo in the 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Thu, May 24, 2018 at 10:35 AM, Heiko Stübner  wrote:
> Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
>> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner  wrote:
>> > So the gpio controller should definitly also be a subnode.
>> >
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>> >
>> > So it should probably look like
>> >
>> > grf: syscon at ff10 {
>> >
>> > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>> >
>> > [all the other syscon sub-devices]
>> >
>> > gpio_mute: gpio-mute {
>> >
>> > compatible = "rockchip,rk3328-gpio-mute";
>> > gpio-controller;
>> > #gpio-cells = <2>;
>> >
>> > };
>>
>> I'm sceptic.
>>
>> That doesn't sound like "general purpose input output" at all.
>>
>> It sounds like special purpose, for a mute button.
>>
>> Does it use IRQ? I would recommend implementing
>> drivers/input/keyboard/syscon-keys.c in the same vein
>> as drivers/leds/leds-syscon.c so you can avoid indirection
>> through GPIO for no good reason at all.
>
> To quote Levin from the other mail:
> 
> The "mute" pin is a output only GPIO, which is already supported by
> setting flags in the gpio-syscon
>   driver. And yes, this pin has a defined function, but can also be used
> for general purpose operation.
> 
>
> So to summarize, the documentation calls it "mute", but it is usable as
> a general pin, which is the reason Levin is working on it - because on his
> board this pin is used to switch between two voltages (aka a gpio-regulator)
> for the sdmmc controller [3.3V + 1.8V].

OK then, I was wrong! :)

Go ahead with this, sorry for the fuzz.

Yours,
Linus Walleij

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Thu, May 24, 2018 at 10:35 AM, Heiko Stübner  wrote:
> Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
>> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner  wrote:
>> > So the gpio controller should definitly also be a subnode.
>> >
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>> >
>> > So it should probably look like
>> >
>> > grf: syscon at ff10 {
>> >
>> > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>> >
>> > [all the other syscon sub-devices]
>> >
>> > gpio_mute: gpio-mute {
>> >
>> > compatible = "rockchip,rk3328-gpio-mute";
>> > gpio-controller;
>> > #gpio-cells = <2>;
>> >
>> > };
>>
>> I'm sceptic.
>>
>> That doesn't sound like "general purpose input output" at all.
>>
>> It sounds like special purpose, for a mute button.
>>
>> Does it use IRQ? I would recommend implementing
>> drivers/input/keyboard/syscon-keys.c in the same vein
>> as drivers/leds/leds-syscon.c so you can avoid indirection
>> through GPIO for no good reason at all.
>
> To quote Levin from the other mail:
> 
> The "mute" pin is a output only GPIO, which is already supported by
> setting flags in the gpio-syscon
>   driver. And yes, this pin has a defined function, but can also be used
> for general purpose operation.
> 
>
> So to summarize, the documentation calls it "mute", but it is usable as
> a general pin, which is the reason Levin is working on it - because on his
> board this pin is used to switch between two voltages (aka a gpio-regulator)
> for the sdmmc controller [3.3V + 1.8V].

OK then, I was wrong! :)

Go ahead with this, sorry for the fuzz.

Yours,
Linus Walleij

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Linus,

Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner  wrote:
> > So the gpio controller should definitly also be a subnode.
> > 
> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> > all the register voodoo in the driver itself and not define it in the dt.
> > 
> > So it should probably look like
> > 
> > grf: syscon at ff10 {
> > 
> > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> > 
> > [all the other syscon sub-devices]
> > 
> > gpio_mute: gpio-mute {
> > 
> > compatible = "rockchip,rk3328-gpio-mute";
> > gpio-controller;
> > #gpio-cells = <2>;
> > 
> > };
> 
> I'm sceptic.
> 
> That doesn't sound like "general purpose input output" at all.
> 
> It sounds like special purpose, for a mute button.
> 
> Does it use IRQ? I would recommend implementing
> drivers/input/keyboard/syscon-keys.c in the same vein
> as drivers/leds/leds-syscon.c so you can avoid indirection
> through GPIO for no good reason at all.

To quote Levin from the other mail:

The "mute" pin is a output only GPIO, which is already supported by 
setting flags in the gpio-syscon
  driver. And yes, this pin has a defined function, but can also be used 
for general purpose operation.


So to summarize, the documentation calls it "mute", but it is usable as
a general pin, which is the reason Levin is working on it - because on his
board this pin is used to switch between two voltages (aka a gpio-regulator)
for the sdmmc controller [3.3V + 1.8V].

Available pin settings are output-enable + of course the high/low setting
and I think I remember there is even a pull setting for it in the GRF 
somewhere - but my memory might be fuzzy here.


Heiko

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Linus,

Am Donnerstag, 24. Mai 2018, 10:28:44 CEST schrieb Linus Walleij:
> On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner  wrote:
> > So the gpio controller should definitly also be a subnode.
> > 
> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> > all the register voodoo in the driver itself and not define it in the dt.
> > 
> > So it should probably look like
> > 
> > grf: syscon at ff10 {
> > 
> > compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> > 
> > [all the other syscon sub-devices]
> > 
> > gpio_mute: gpio-mute {
> > 
> > compatible = "rockchip,rk3328-gpio-mute";
> > gpio-controller;
> > #gpio-cells = <2>;
> > 
> > };
> 
> I'm sceptic.
> 
> That doesn't sound like "general purpose input output" at all.
> 
> It sounds like special purpose, for a mute button.
> 
> Does it use IRQ? I would recommend implementing
> drivers/input/keyboard/syscon-keys.c in the same vein
> as drivers/leds/leds-syscon.c so you can avoid indirection
> through GPIO for no good reason at all.

To quote Levin from the other mail:

The "mute" pin is a output only GPIO, which is already supported by 
setting flags in the gpio-syscon
  driver. And yes, this pin has a defined function, but can also be used 
for general purpose operation.


So to summarize, the documentation calls it "mute", but it is usable as
a general pin, which is the reason Levin is working on it - because on his
board this pin is used to switch between two voltages (aka a gpio-regulator)
for the sdmmc controller [3.3V + 1.8V].

Available pin settings are output-enable + of course the high/low setting
and I think I remember there is even a pull setting for it in the GRF 
somewhere - but my memory might be fuzzy here.


Heiko

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner  wrote:

> So the gpio controller should definitly also be a subnode.
>
> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.
>
> So it should probably look like
>
> grf: syscon at ff10 {
> compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
> [all the other syscon sub-devices]
>
> gpio_mute: gpio-mute {
> compatible = "rockchip,rk3328-gpio-mute";
> gpio-controller;
> #gpio-cells = <2>;
> };

I'm sceptic.

That doesn't sound like "general purpose input output" at all.

It sounds like special purpose, for a mute button.

Does it use IRQ? I would recommend implementing
drivers/input/keyboard/syscon-keys.c in the same vein
as drivers/leds/leds-syscon.c so you can avoid indirection
through GPIO for no good reason at all.

I already have other good uses for such a generic
input driver.

Yours,
Linus Walleij

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Linus Walleij]

On Wed, May 23, 2018 at 5:12 PM, Heiko Stübner  wrote:

> So the gpio controller should definitly also be a subnode.
>
> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.
>
> So it should probably look like
>
> grf: syscon at ff10 {
> compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
> [all the other syscon sub-devices]
>
> gpio_mute: gpio-mute {
> compatible = "rockchip,rk3328-gpio-mute";
> gpio-controller;
> #gpio-cells = <2>;
> };

I'm sceptic.

That doesn't sound like "general purpose input output" at all.

It sounds like special purpose, for a mute button.

Does it use IRQ? I would recommend implementing
drivers/input/keyboard/syscon-keys.c in the same vein
as drivers/leds/leds-syscon.c so you can avoid indirection
through GPIO for no good reason at all.

I already have other good uses for such a generic
input driver.

Yours,
Linus Walleij

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

Hi all, I'd like to quote reply of Robin Murphy at
 http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html



I would suggest s/pin number/bit number in the associated GRF register/
here. At least in this RK3328 case there's only one pin, which isn't
numbered, and if you naively considered it pin 0 of this 'bank' you'd
already have the wrong number. Since we're dealing with the "random
SoC-specific controls" region of the GRF as opposed to the
relatively-consistent and organised pinmux parts, I don't think we
should rely on any assumptions about how things are laid out.

I was initially going to suggest a more specific compatible string as
well, but on reflection I think the generic "rockchip,gpio-syscon" for
basic "flip this single GRF bit" functionality actually is the right way
to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
total related to this pin - the enable, value, and some pull controls
(which I assume apply when the output is disabled) - if at some point in
future we *did* want to start explicitly controlling the rest of them
too, then would be a good time to define a separate
"rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
for that specialised functionality, independently of this basic one.



Shall we go the generic "rockchip,gpio-syscon" way, or the specific
 "rockchip,rk3328-gpio-mute" way? I prefer the former one.

The property of "gpio,syscon-dev" in gpio-syscon driver should be 
documented.
Since the gpio controller is defined in the dtsi file, which inevitably 
contains voodoo
register addresses. But at the board level dts file, there won't be more 
register

addresses.

On 2018-05-24 3:53 AM, Rob Herring wrote:

On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:

On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:

On 2018-05-23 2:02 AM, Rob Herring wrote:

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41

++

   drivers/gpio/gpio-syscon.c | 30



   2 files changed, 71 insertions(+)
   create mode 100644

Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git
a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.

There's no need for this child node. Just make the parent node a gpio
controller.

Rob

Hi Rob, it is not clear to me. Do you suggest that the grf node should be
a
gpio controller,
like below?

+grf: syscon at ff10 {
+compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
"syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338

So the gpio controller should definitly also be a subnode.

Sigh, yes, if there are a bunch of functions needing subnodes like the
above, then yes that makes sense. But that's not what has been
presented. Please make some attempt at defining *all* the functions.
An actual binding would be nice, but I'll settle for just a list of
things. The list should have functions that have DT dependencies (like
clocks for phys in the above) because until you do, you don't need
child nodes.


In rk3328.dtsi file, there are lots 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

Hi all, I'd like to quote reply of Robin Murphy at
 http://lists.infradead.org/pipermail/linux-rockchip/2018-May/020619.html



I would suggest s/pin number/bit number in the associated GRF register/
here. At least in this RK3328 case there's only one pin, which isn't
numbered, and if you naively considered it pin 0 of this 'bank' you'd
already have the wrong number. Since we're dealing with the "random
SoC-specific controls" region of the GRF as opposed to the
relatively-consistent and organised pinmux parts, I don't think we
should rely on any assumptions about how things are laid out.

I was initially going to suggest a more specific compatible string as
well, but on reflection I think the generic "rockchip,gpio-syscon" for
basic "flip this single GRF bit" functionality actually is the right way
to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
total related to this pin - the enable, value, and some pull controls
(which I assume apply when the output is disabled) - if at some point in
future we *did* want to start explicitly controlling the rest of them
too, then would be a good time to define a separate
"rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
for that specialised functionality, independently of this basic one.



Shall we go the generic "rockchip,gpio-syscon" way, or the specific
 "rockchip,rk3328-gpio-mute" way? I prefer the former one.

The property of "gpio,syscon-dev" in gpio-syscon driver should be 
documented.
Since the gpio controller is defined in the dtsi file, which inevitably 
contains voodoo
register addresses. But at the board level dts file, there won't be more 
register

addresses.

On 2018-05-24 3:53 AM, Rob Herring wrote:

On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:

On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:

On 2018-05-23 2:02 AM, Rob Herring wrote:

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41

++

   drivers/gpio/gpio-syscon.c | 30



   2 files changed, 71 insertions(+)
   create mode 100644

Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git
a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.

There's no need for this child node. Just make the parent node a gpio
controller.

Rob

Hi Rob, it is not clear to me. Do you suggest that the grf node should be
a
gpio controller,
like below?

+grf: syscon at ff10 {
+compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
"syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338

So the gpio controller should definitly also be a subnode.

Sigh, yes, if there are a bunch of functions needing subnodes like the
above, then yes that makes sense. But that's not what has been
presented. Please make some attempt at defining *all* the functions.
An actual binding would be nice, but I'll settle for just a list of
things. The list should have functions that have DT dependencies (like
clocks for phys in the above) because until you do, you don't need
child nodes.


In rk3328.dtsi file, there are lots of line "rockchip,grf = <>;" in 
various nodes,
such as tsadc,  cru, gmac2io, 

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
> Hi Rob, Levin,
>
> sorry for being late to the party.
>
> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
>> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
>> >>> From: Levin Du 
>> >>>
>> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >>> which do not belong to the general pinctrl.
>> >>>
>> >>> Adding gpio-syscon support makes controlling regulator or
>> >>> LED using these special pins very easy by reusing existing
>> >>> drivers, such as gpio-regulator and led-gpio.
>> >>>
>> >>> Signed-off-by: Levin Du 
>> >>>
>> >>> ---
>> >>>
>> >>> 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,gpio-syscon.txt | 41
>> >>>
>> >>> ++
>> >>>
>> >>>   drivers/gpio/gpio-syscon.c | 30
>> >>>
>> >>> 
>> >>>
>> >>>   2 files changed, 71 insertions(+)
>> >>>   create mode 100644
>> >>>
>> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>>
>> >>> diff --git
>> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> new file mode 100644
>> >>> index 000..b1b2a67
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> @@ -0,0 +1,41 @@
>> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >>> +
>> >>> +Required properties:
>> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >>> +  the second cell is used to specify the gpio polarity:
>> >>> +0 = Active high,
>> >>> +1 = Active low.
>> >>
>> >> There's no need for this child node. Just make the parent node a gpio
>> >> controller.
>> >>
>> >> Rob
>> >
>> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> > a
>> > gpio controller,
>> > like below?
>> >
>> > +grf: syscon at ff10 {
>> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> > "syscon", "simple-mfd";
>>
>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>
> I would disagree quite a bit here. The grf are the "general register files",
> a bunch of registers used for quite a lot of things, and so it seems
> among other users, also a gpio-controller for some more random pins
> not controlled through the regular gpio controllers.
>
> For a more fully stocked grf, please see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>
> So the gpio controller should definitly also be a subnode.

Sigh, yes, if there are a bunch of functions needing subnodes like the
above, then yes that makes sense. But that's not what has been
presented. Please make some attempt at defining *all* the functions.
An actual binding would be nice, but I'll settle for just a list of
things. The list should have functions that have DT dependencies (like
clocks for phys in the above) because until you do, you don't need
child nodes.

> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.

Is there really just one GPIO? If it has a defined function, then is
it really GP? Can you control direction? I know Linus W doesn't like
that kind of abuse of GPIO.

> So it should probably look like
>
> grf: syscon at ff10 {
> compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
> [all the other syscon sub-devices]
>
> gpio_mute: gpio-mute {
> compatible = "rockchip,rk3328-gpio-mute";
> gpio-controller;
> #gpio-cells = <2>;
> };
>
> [more other syscon sub-devices]
> };

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner  wrote:
> Hi Rob, Levin,
>
> sorry for being late to the party.
>
> Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
>> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
>> >>> From: Levin Du 
>> >>>
>> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >>> which do not belong to the general pinctrl.
>> >>>
>> >>> Adding gpio-syscon support makes controlling regulator or
>> >>> LED using these special pins very easy by reusing existing
>> >>> drivers, such as gpio-regulator and led-gpio.
>> >>>
>> >>> Signed-off-by: Levin Du 
>> >>>
>> >>> ---
>> >>>
>> >>> 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,gpio-syscon.txt | 41
>> >>>
>> >>> ++
>> >>>
>> >>>   drivers/gpio/gpio-syscon.c | 30
>> >>>
>> >>> 
>> >>>
>> >>>   2 files changed, 71 insertions(+)
>> >>>   create mode 100644
>> >>>
>> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>>
>> >>> diff --git
>> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> new file mode 100644
>> >>> index 000..b1b2a67
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >>> @@ -0,0 +1,41 @@
>> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >>> +
>> >>> +Required properties:
>> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >>> +  the second cell is used to specify the gpio polarity:
>> >>> +0 = Active high,
>> >>> +1 = Active low.
>> >>
>> >> There's no need for this child node. Just make the parent node a gpio
>> >> controller.
>> >>
>> >> Rob
>> >
>> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> > a
>> > gpio controller,
>> > like below?
>> >
>> > +grf: syscon at ff10 {
>> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> > "syscon", "simple-mfd";
>>
>> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>
> I would disagree quite a bit here. The grf are the "general register files",
> a bunch of registers used for quite a lot of things, and so it seems
> among other users, also a gpio-controller for some more random pins
> not controlled through the regular gpio controllers.
>
> For a more fully stocked grf, please see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>
> So the gpio controller should definitly also be a subnode.

Sigh, yes, if there are a bunch of functions needing subnodes like the
above, then yes that makes sense. But that's not what has been
presented. Please make some attempt at defining *all* the functions.
An actual binding would be nice, but I'll settle for just a list of
things. The list should have functions that have DT dependencies (like
clocks for phys in the above) because until you do, you don't need
child nodes.

> The gpio in question is called "mute", so I'd think the gpio-syscon driver
> should just define a "rockchip,rk3328-gpio-mute" compatible and contain
> all the register voodoo in the driver itself and not define it in the dt.

Is there really just one GPIO? If it has a defined function, then is
it really GP? Can you control direction? I know Linus W doesn't like
that kind of abuse of GPIO.

> So it should probably look like
>
> grf: syscon at ff10 {
> compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
>
> [all the other syscon sub-devices]
>
> gpio_mute: gpio-mute {
> compatible = "rockchip,rk3328-gpio-mute";
> gpio-controller;
> #gpio-cells = <2>;
> };
>
> [more other syscon sub-devices]
> };

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> >>> From: Levin Du 
> >>> 
> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >>> which do not belong to the general pinctrl.
> >>> 
> >>> Adding gpio-syscon support makes controlling regulator or
> >>> LED using these special pins very easy by reusing existing
> >>> drivers, such as gpio-regulator and led-gpio.
> >>> 
> >>> Signed-off-by: Levin Du 
> >>> 
> >>> ---
> >>> 
> >>> 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,gpio-syscon.txt | 41
> >>> 
> >>> ++
> >>> 
> >>>   drivers/gpio/gpio-syscon.c | 30
> >>> 
> >>> 
> >>> 
> >>>   2 files changed, 71 insertions(+)
> >>>   create mode 100644
> >>> 
> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> 
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> new file mode 100644
> >>> index 000..b1b2a67
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> @@ -0,0 +1,41 @@
> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >>> +
> >>> +Required properties:
> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >>> +  the second cell is used to specify the gpio polarity:
> >>> +0 = Active high,
> >>> +1 = Active low.
> >> 
> >> There's no need for this child node. Just make the parent node a gpio
> >> controller.
> >> 
> >> Rob
> > 
> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> > a
> > gpio controller,
> > like below?
> > 
> > +grf: syscon at ff10 {
> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> > "syscon", "simple-mfd";
> 
> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338

So the gpio controller should definitly also be a subnode.

The gpio in question is called "mute", so I'd think the gpio-syscon driver
should just define a "rockchip,rk3328-gpio-mute" compatible and contain
all the register voodoo in the driver itself and not define it in the dt.

So it should probably look like

grf: syscon at ff10 {
compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";

[all the other syscon sub-devices]

gpio_mute: gpio-mute {
compatible = "rockchip,rk3328-gpio-mute";
gpio-controller;
#gpio-cells = <2>;
};

[more other syscon sub-devices]
};


Heiko   




> > +//...
> > +gpio-controller;
> > +#gpio-cells = <2>;
> > +gpio,syscon-dev = < 0x0428 0>;
> 
> And drop this. It may be used in the kernel, but it is not a
> documented property.
> 
> > +};
> > 
> > or just reserve the following case in the doc?
> > 
> > +grf: syscon at ff10 {
> > +compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> > +//...
> > +};
> > +
> > +gpio_mute: gpio-mute {
> > +compatible = "rockchip,gpio-syscon";
> > +gpio-controller;
> > +#gpio-cells = <2>;
> > +gpio,syscon-dev = < 0x0428 0>;
> > +};
> > 
> > 
> > Thanks
> > Levin
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Heiko Stübner]

Hi Rob, Levin,

sorry for being late to the party.

Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
> On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
> > On 2018-05-23 2:02 AM, Rob Herring wrote:
> >> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> >>> From: Levin Du 
> >>> 
> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> >>> which do not belong to the general pinctrl.
> >>> 
> >>> Adding gpio-syscon support makes controlling regulator or
> >>> LED using these special pins very easy by reusing existing
> >>> drivers, such as gpio-regulator and led-gpio.
> >>> 
> >>> Signed-off-by: Levin Du 
> >>> 
> >>> ---
> >>> 
> >>> 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,gpio-syscon.txt | 41
> >>> 
> >>> ++
> >>> 
> >>>   drivers/gpio/gpio-syscon.c | 30
> >>> 
> >>> 
> >>> 
> >>>   2 files changed, 71 insertions(+)
> >>>   create mode 100644
> >>> 
> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> 
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> new file mode 100644
> >>> index 000..b1b2a67
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> >>> @@ -0,0 +1,41 @@
> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
> >>> +
> >>> +Required properties:
> >>> +- compatible: Should contain "rockchip,gpio-syscon".
> >>> +- gpio-controller: Marks the device node as a gpio controller.
> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
> >>> +  the second cell is used to specify the gpio polarity:
> >>> +0 = Active high,
> >>> +1 = Active low.
> >> 
> >> There's no need for this child node. Just make the parent node a gpio
> >> controller.
> >> 
> >> Rob
> > 
> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
> > a
> > gpio controller,
> > like below?
> > 
> > +grf: syscon at ff10 {
> > +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> > "syscon", "simple-mfd";
> 
> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

I would disagree quite a bit here. The grf are the "general register files",
a bunch of registers used for quite a lot of things, and so it seems
among other users, also a gpio-controller for some more random pins
not controlled through the regular gpio controllers.

For a more fully stocked grf, please see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338

So the gpio controller should definitly also be a subnode.

The gpio in question is called "mute", so I'd think the gpio-syscon driver
should just define a "rockchip,rk3328-gpio-mute" compatible and contain
all the register voodoo in the driver itself and not define it in the dt.

So it should probably look like

grf: syscon at ff10 {
compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";

[all the other syscon sub-devices]

gpio_mute: gpio-mute {
compatible = "rockchip,rk3328-gpio-mute";
gpio-controller;
#gpio-cells = <2>;
};

[more other syscon sub-devices]
};


Heiko   




> > +//...
> > +gpio-controller;
> > +#gpio-cells = <2>;
> > +gpio,syscon-dev = < 0x0428 0>;
> 
> And drop this. It may be used in the kernel, but it is not a
> documented property.
> 
> > +};
> > 
> > or just reserve the following case in the doc?
> > 
> > +grf: syscon at ff10 {
> > +compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> > +//...
> > +};
> > +
> > +gpio_mute: gpio-mute {
> > +compatible = "rockchip,gpio-syscon";
> > +gpio-controller;
> > +#gpio-cells = <2>;
> > +gpio,syscon-dev = < 0x0428 0>;
> > +};
> > 
> > 
> > Thanks
> > Levin
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>
>> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
>>>
>>> From: Levin Du 
>>>
>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>> which do not belong to the general pinctrl.
>>>
>>> Adding gpio-syscon support makes controlling regulator or
>>> LED using these special pins very easy by reusing existing
>>> drivers, such as gpio-regulator and led-gpio.
>>>
>>> Signed-off-by: Levin Du 
>>>
>>> ---
>>>
>>> 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,gpio-syscon.txt | 41
>>> ++
>>>   drivers/gpio/gpio-syscon.c | 30
>>> 
>>>   2 files changed, 71 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> new file mode 100644
>>> index 000..b1b2a67
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> @@ -0,0 +1,41 @@
>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>> +
>>> +Required properties:
>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>> +- gpio-controller: Marks the device node as a gpio controller.
>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>> +  the second cell is used to specify the gpio polarity:
>>> +0 = Active high,
>>> +1 = Active low.
>>
>> There's no need for this child node. Just make the parent node a gpio
>> controller.
>>
>> Rob
>
> Hi Rob, it is not clear to me. Do you suggest that the grf node should be a
> gpio controller,
> like below?
>
> +grf: syscon at ff10 {
> +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> "syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

> +//...
> +gpio-controller;
> +#gpio-cells = <2>;
> +gpio,syscon-dev = < 0x0428 0>;

And drop this. It may be used in the kernel, but it is not a
documented property.

> +};
>
> or just reserve the following case in the doc?
>
> +grf: syscon at ff10 {
> +compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> +//...
> +};
> +
> +gpio_mute: gpio-mute {
> +compatible = "rockchip,gpio-syscon";
> +gpio-controller;
> +#gpio-cells = <2>;
> +gpio,syscon-dev = < 0x0428 0>;
> +};
>
>
> Thanks
> Levin
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Tue, May 22, 2018 at 9:02 PM, Levin Du  wrote:
> On 2018-05-23 2:02 AM, Rob Herring wrote:
>>
>> On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
>>>
>>> From: Levin Du 
>>>
>>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>>> which do not belong to the general pinctrl.
>>>
>>> Adding gpio-syscon support makes controlling regulator or
>>> LED using these special pins very easy by reusing existing
>>> drivers, such as gpio-regulator and led-gpio.
>>>
>>> Signed-off-by: Levin Du 
>>>
>>> ---
>>>
>>> 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,gpio-syscon.txt | 41
>>> ++
>>>   drivers/gpio/gpio-syscon.c | 30
>>> 
>>>   2 files changed, 71 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> new file mode 100644
>>> index 000..b1b2a67
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>>> @@ -0,0 +1,41 @@
>>> +* Rockchip GPIO support for GRF_SOC_CON registers
>>> +
>>> +Required properties:
>>> +- compatible: Should contain "rockchip,gpio-syscon".
>>> +- gpio-controller: Marks the device node as a gpio controller.
>>> +- #gpio-cells: Should be two. The first cell is the pin number and
>>> +  the second cell is used to specify the gpio polarity:
>>> +0 = Active high,
>>> +1 = Active low.
>>
>> There's no need for this child node. Just make the parent node a gpio
>> controller.
>>
>> Rob
>
> Hi Rob, it is not clear to me. Do you suggest that the grf node should be a
> gpio controller,
> like below?
>
> +grf: syscon at ff10 {
> +compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
> "syscon", "simple-mfd";

Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".

> +//...
> +gpio-controller;
> +#gpio-cells = <2>;
> +gpio,syscon-dev = < 0x0428 0>;

And drop this. It may be used in the kernel, but it is not a
documented property.

> +};
>
> or just reserve the following case in the doc?
>
> +grf: syscon at ff10 {
> +compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> +//...
> +};
> +
> +gpio_mute: gpio-mute {
> +compatible = "rockchip,gpio-syscon";
> +gpio-controller;
> +#gpio-cells = <2>;
> +gpio,syscon-dev = < 0x0428 0>;
> +};
>
>
> Thanks
> Levin
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

On 2018-05-23 2:02 AM, Rob Herring wrote:

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41 ++
  drivers/gpio/gpio-syscon.c | 30 
  2 files changed, 71 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt 
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.

There's no need for this child node. Just make the parent node a gpio
controller.

Rob
Hi Rob, it is not clear to me. Do you suggest that the grf node should 
be a gpio controller,

like below?

+    grf: syscon at ff10 {
+        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", 
"syscon", "simple-mfd";

+        //...
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio,syscon-dev = < 0x0428 0>;
+    };

or just reserve the following case in the doc?

+    grf: syscon at ff10 {
+        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+        //...
+    };
+
+    gpio_mute: gpio-mute {
+        compatible = "rockchip,gpio-syscon";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio,syscon-dev = < 0x0428 0>;
+    };


Thanks
Levin

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Levin Du]

On 2018-05-23 2:02 AM, Rob Herring wrote:

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41 ++
  drivers/gpio/gpio-syscon.c | 30 
  2 files changed, 71 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt 
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.

There's no need for this child node. Just make the parent node a gpio
controller.

Rob
Hi Rob, it is not clear to me. Do you suggest that the grf node should 
be a gpio controller,

like below?

+    grf: syscon at ff10 {
+        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf", 
"syscon", "simple-mfd";

+        //...
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio,syscon-dev = < 0x0428 0>;
+    };

or just reserve the following case in the doc?

+    grf: syscon at ff10 {
+        compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+        //...
+    };
+
+    gpio_mute: gpio-mute {
+        compatible = "rockchip,gpio-syscon";
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio,syscon-dev = < 0x0428 0>;
+    };


Thanks
Levin

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> From: Levin Du 
> 
> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> which do not belong to the general pinctrl.
> 
> Adding gpio-syscon support makes controlling regulator or
> LED using these special pins very easy by reusing existing
> drivers, such as gpio-regulator and led-gpio.
> 
> Signed-off-by: Levin Du 
> 
> ---
> 
> 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,gpio-syscon.txt | 41 
> ++
>  drivers/gpio/gpio-syscon.c | 30 
>  2 files changed, 71 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt 
> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> new file mode 100644
> index 000..b1b2a67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> @@ -0,0 +1,41 @@
> +* Rockchip GPIO support for GRF_SOC_CON registers
> +
> +Required properties:
> +- compatible: Should contain "rockchip,gpio-syscon".
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be two. The first cell is the pin number and
> +  the second cell is used to specify the gpio polarity:
> +0 = Active high,
> +1 = Active low.

There's no need for this child node. Just make the parent node a gpio 
controller.

Rob

Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Fri, May 18, 2018 at 11:52:05AM +0800, d...@t-chip.com.cn wrote:
> From: Levin Du 
> 
> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> which do not belong to the general pinctrl.
> 
> Adding gpio-syscon support makes controlling regulator or
> LED using these special pins very easy by reusing existing
> drivers, such as gpio-regulator and led-gpio.
> 
> Signed-off-by: Levin Du 
> 
> ---
> 
> 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,gpio-syscon.txt | 41 
> ++
>  drivers/gpio/gpio-syscon.c | 30 
>  2 files changed, 71 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt 
> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> new file mode 100644
> index 000..b1b2a67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> @@ -0,0 +1,41 @@
> +* Rockchip GPIO support for GRF_SOC_CON registers
> +
> +Required properties:
> +- compatible: Should contain "rockchip,gpio-syscon".
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be two. The first cell is the pin number and
> +  the second cell is used to specify the gpio polarity:
> +0 = Active high,
> +1 = Active low.

There's no need for this child node. Just make the parent node a gpio 
controller.

Rob

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[djw]

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41 ++
 drivers/gpio/gpio-syscon.c | 30 
 2 files changed, 71 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt 
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.
+- gpio,syscon-dev: Should contain .
+  If declared as child of the grf node, the grf_phandle can be 0.
+
+Example:
+
+1. As child of grf node:
+
+   grf: syscon@ff10 {
+   compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+
+   gpio_mute: gpio-mute {
+   compatible = "rockchip,gpio-syscon";
+   gpio-controller;
+   #gpio-cells = <2>;
+   gpio,syscon-dev = <0 0x0428 0>;
+   };
+   };
+
+
+2. Not child of grf node:
+
+   grf: syscon@ff10 {
+   compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+   //...
+   };
+
+   gpio_mute: gpio-mute {
+   compatible = "rockchip,gpio-syscon";
+   gpio-controller;
+   #gpio-cells = <2>;
+   gpio,syscon-dev = < 0x0428 0>;
+   };
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 7325b86..e24b408 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -135,6 +135,32 @@ static const struct syscon_gpio_data clps711x_mctrl_gpio = 
{
.dat_bit_offset = 0x40 * 8 + 8,
 };
 
+static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int val)
+{
+   struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
+   unsigned int offs;
+   u8 bit;
+   u32 data;
+   int ret;
+
+   offs = priv->dreg_offset + priv->data->dat_bit_offset + offset;
+   bit = offs % SYSCON_REG_BITS;
+   data = (val ? BIT(bit) : 0) | BIT(bit + 16);
+   ret = regmap_write(priv->syscon,
+  (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE,
+  data);
+   if (ret < 0)
+   dev_err(chip->parent, "gpio write failed ret(%d)\n", ret);
+}
+
+static const struct syscon_gpio_data rockchip_gpio_syscon = {
+   /* Rockchip GRF_SOC_CON Bits 0-15 */
+   .flags  = GPIO_SYSCON_FEAT_OUT,
+   .bit_count  = 16,
+   .set= rockchip_gpio_set,
+};
+
 #define KEYSTONE_LOCK_BIT BIT(0)
 
 static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
@@ -175,6 +201,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
.compatible = "ti,keystone-dsp-gpio",
.data   = _dsp_gpio,
},
+   {
+   .compatible = "rockchip,gpio-syscon",
+   .data   = _gpio_syscon,
+   },
{ }
 };
 MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
-- 
2.7.4

[PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[djw]

From: Levin Du 

Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.

Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.

Signed-off-by: Levin Du 

---

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,gpio-syscon.txt | 41 ++
 drivers/gpio/gpio-syscon.c | 30 
 2 files changed, 71 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt 
b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 000..b1b2a67
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+0 = Active high,
+1 = Active low.
+- gpio,syscon-dev: Should contain .
+  If declared as child of the grf node, the grf_phandle can be 0.
+
+Example:
+
+1. As child of grf node:
+
+   grf: syscon@ff10 {
+   compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+
+   gpio_mute: gpio-mute {
+   compatible = "rockchip,gpio-syscon";
+   gpio-controller;
+   #gpio-cells = <2>;
+   gpio,syscon-dev = <0 0x0428 0>;
+   };
+   };
+
+
+2. Not child of grf node:
+
+   grf: syscon@ff10 {
+   compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+   //...
+   };
+
+   gpio_mute: gpio-mute {
+   compatible = "rockchip,gpio-syscon";
+   gpio-controller;
+   #gpio-cells = <2>;
+   gpio,syscon-dev = < 0x0428 0>;
+   };
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 7325b86..e24b408 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -135,6 +135,32 @@ static const struct syscon_gpio_data clps711x_mctrl_gpio = 
{
.dat_bit_offset = 0x40 * 8 + 8,
 };
 
+static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int val)
+{
+   struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
+   unsigned int offs;
+   u8 bit;
+   u32 data;
+   int ret;
+
+   offs = priv->dreg_offset + priv->data->dat_bit_offset + offset;
+   bit = offs % SYSCON_REG_BITS;
+   data = (val ? BIT(bit) : 0) | BIT(bit + 16);
+   ret = regmap_write(priv->syscon,
+  (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE,
+  data);
+   if (ret < 0)
+   dev_err(chip->parent, "gpio write failed ret(%d)\n", ret);
+}
+
+static const struct syscon_gpio_data rockchip_gpio_syscon = {
+   /* Rockchip GRF_SOC_CON Bits 0-15 */
+   .flags  = GPIO_SYSCON_FEAT_OUT,
+   .bit_count  = 16,
+   .set= rockchip_gpio_set,
+};
+
 #define KEYSTONE_LOCK_BIT BIT(0)
 
 static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
@@ -175,6 +201,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
.compatible = "ti,keystone-dsp-gpio",
.data   = _dsp_gpio,
},
+   {
+   .compatible = "rockchip,gpio-syscon",
+   .data   = _gpio_syscon,
+   },
{ }
 };
 MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
-- 
2.7.4