[PATCH V2 3/8] dt-bindings: gpio: vf610: add optional clocks property

[PATCH V2 3/8] dt-bindings: gpio: vf610: add optional clocks property

[A.s. Dong]

On some SoCs(e.g. MX7ULP), GPIO clock is gatable and maybe
disabled by default. Users have to make sure it's enabled before
being able to access controller registers, otherwise an external
abort error may occur. Let's add the optional clocks property to
handle this case.

For ULP GPIO clock, it includes two separate clocks: one is for
GPIO controller Input/Output function clock while another is
GPIO port control clock for interrupt function.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: linux-gpio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v1->v2:
 * new patch
---
 Documentation/devicetree/bindings/gpio/gpio-vf610.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-vf610.txt b/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
index 0ccbae4..ae254aa 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-vf610.txt
@@ -24,6 +24,12 @@ Required properties for GPIO node:
       4 = active high level-sensitive.
       8 = active low level-sensitive.
 
+Optional properties:
+-clocks:	Must contain an entry for each entry in clock-names.
+		See common clock-bindings.txt for details.
+-clock-names:	A list of clock names. For imx7ulp, it must contain
+		"gpio", "port".
+
 Note: Each GPIO port should have an alias correctly numbered in "aliases"
 node.
 
-- 
2.7.4

Re: [PATCH V2 3/8] dt-bindings: gpio: vf610: add optional clocks property

[Rob Herring]

On Tue, 23 Oct 2018 11:49:13 +0000, "A.s. Dong" wrote:
> On some SoCs(e.g. MX7ULP), GPIO clock is gatable and maybe
> disabled by default. Users have to make sure it's enabled before
> being able to access controller registers, otherwise an external
> abort error may occur. Let's add the optional clocks property to
> handle this case.
> 
> For ULP GPIO clock, it includes two separate clocks: one is for
> GPIO controller Input/Output function clock while another is
> GPIO port control clock for interrupt function.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: linux-gpio@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v1->v2:
>  * new patch
> ---
>  Documentation/devicetree/bindings/gpio/gpio-vf610.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH V2 4/8] gpio: vf610: add optional clock support

[A.s. Dong]

Some SoCs need the gpio clock to be enabled before accessing
HW registers. This patch add the optional clock handling.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v1->v2:
 * new patch
---
 drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index d4ad6d0..cbc4f44 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/init.h>
@@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	struct clk *clk_gpio, *clk_port;
 	struct vf610_gpio_port *port;
 	struct resource *iores;
 	struct gpio_chip *gc;
@@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device *pdev)
 	if (port->irq < 0)
 		return port->irq;
 
+	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
+	clk_port = devm_clk_get(&pdev->dev, "port");
+	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
+	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
+		return -EPROBE_DEFER;
+	} else if (!IS_ERR_OR_NULL(clk_gpio) &&
+		   !IS_ERR_OR_NULL(clk_port)) {
+		ret = clk_prepare_enable(clk_gpio);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(clk_port);
+		if (ret) {
+			clk_disable_unprepare(clk_gpio);
+			return ret;
+		}
+	}
+
 	gc = &port->gc;
 	gc->of_node = np;
 	gc->parent = dev;
-- 
2.7.4

Re: [PATCH V2 4/8] gpio: vf610: add optional clock support

[Russell King - ARM Linux]

On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote:
> +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> +	clk_port = devm_clk_get(&pdev->dev, "port");
> +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
> +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {

	if (clk_gpio == ERR_PTR(-EPROBE_DEFER) ||
	    clk_port == ERR_PTR(-EPROBE_DEFER)) {

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH V2 4/8] gpio: vf610: add optional clock support

[A.s. Dong]

Hi Russell,

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk]
> Sent: Tuesday, October 23, 2018 8:04 PM
[...]
> On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote:
> > +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> > +	clk_port = devm_clk_get(&pdev->dev, "port");
> > +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
> > +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
> 
> 	if (clk_gpio == ERR_PTR(-EPROBE_DEFER) ||
> 	    clk_port == ERR_PTR(-EPROBE_DEFER)) {
> 

Thanks for the suggestion. I will update it in next series.
Before that, let's wait a moment to see if any more review comments.

BTW, as I see kernel currently is widely using PTR_ERR(ptr) to compare
-EPROBE_DEFER, I'm not quite get the point why the new approach
you suggested is better, is it less error-prone? Or something else?
would you please help clarify a bit more?

Regards
Dong Aisheng

> --
> RMK's Patch system:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> armlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Caishen
> g.dong%40nxp.com%7C34fcfce2f5d042efd0b808d638dfaaed%7C686ea1d3bc
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C636758930627945785&sdata=
> PME05RkmX0mxRmhO%2Bj%2FofV3VN2VMx3FWU9bbqFr3XAg%3D&res
> erved=0
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps
> up According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH V2 4/8] gpio: vf610: add optional clock support

[Uwe Kleine-König]

On Tue, Oct 23, 2018 at 12:23:12PM +0000, A.s. Dong wrote:
> Hi Russell,
> 
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk]
> > Sent: Tuesday, October 23, 2018 8:04 PM
> [...]
> > On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote:
> > > +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> > > +	clk_port = devm_clk_get(&pdev->dev, "port");
> > > +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
> > > +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
> > 
> > 	if (clk_gpio == ERR_PTR(-EPROBE_DEFER) ||
> > 	    clk_port == ERR_PTR(-EPROBE_DEFER)) {
> > 
> 
> Thanks for the suggestion. I will update it in next series.
> Before that, let's wait a moment to see if any more review comments.
> 
> BTW, as I see kernel currently is widely using PTR_ERR(ptr) to compare
> -EPROBE_DEFER, I'm not quite get the point why the new approach
> you suggested is better, is it less error-prone? Or something else?
> would you please help clarify a bit more?

See the discussion in https://lore.kernel.org/patchwork/patch/999602/.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH V2 4/8] gpio: vf610: add optional clock support

[A.s. Dong]

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: Tuesday, October 23, 2018 8:42 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>; dongas86@gmail.com;
> Linus Walleij <linus.walleij@linaro.org>; Stefan Agner <stefan@agner.ch>;
> linux-gpio@vger.kernel.org; robh+dt@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; shawnguo@kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH V2 4/8] gpio: vf610: add optional clock support
> 
> On Tue, Oct 23, 2018 at 12:23:12PM +0000, A.s. Dong wrote:
> > Hi Russell,
> >
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk]
> > > Sent: Tuesday, October 23, 2018 8:04 PM
> > [...]
> > > On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote:
> > > > +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> > > > +	clk_port = devm_clk_get(&pdev->dev, "port");
> > > > +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
> > > > +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
> > >
> > > 	if (clk_gpio == ERR_PTR(-EPROBE_DEFER) ||
> > > 	    clk_port == ERR_PTR(-EPROBE_DEFER)) {
> > >
> >
> > Thanks for the suggestion. I will update it in next series.
> > Before that, let's wait a moment to see if any more review comments.
> >
> > BTW, as I see kernel currently is widely using PTR_ERR(ptr) to compare
> > -EPROBE_DEFER, I'm not quite get the point why the new approach you
> > suggested is better, is it less error-prone? Or something else?
> > would you please help clarify a bit more?
> 
> See the discussion in
> https://lore.kernel.org/patchwork/patch/999602/ 
>

Thanks for sharing the info.
It does help.

Regards
Dong Aisheng
 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7C06
> 5f26466899474bf61a08d638e4e308%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C636758953039817883&amp;sdata=hSgiadfyiFoK4AVTF2BR
> guOU5H8C77%2BY2bdjViC4Sfw%3D&amp;reserved=0  |

Re: [PATCH V2 4/8] gpio: vf610: add optional clock support

[Stefan Agner]

On 23.10.2018 13:49, A.s. Dong wrote:
> Some SoCs need the gpio clock to be enabled before accessing
> HW registers. This patch add the optional clock handling.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> v1->v2:
>  * new patch
> ---
>  drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> index d4ad6d0..cbc4f44 100644
> --- a/drivers/gpio/gpio-vf610.c
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/gpio.h>
>  #include <linux/init.h>
> @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> +	struct clk *clk_gpio, *clk_port;
>  	struct vf610_gpio_port *port;
>  	struct resource *iores;
>  	struct gpio_chip *gc;
> @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device *pdev)
>  	if (port->irq < 0)
>  		return port->irq;
>  
> +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> +	clk_port = devm_clk_get(&pdev->dev, "port");

Are you sure that those are two independent clocks? On i.MX 7 usually
there was a single clock gate controlling multiple clocks at once (which
should be modeled as a single clock gate in the clock tree).

--
Stefan 

> +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
> +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
> +		return -EPROBE_DEFER;
> +	} else if (!IS_ERR_OR_NULL(clk_gpio) &&
> +		   !IS_ERR_OR_NULL(clk_port)) {
> +		ret = clk_prepare_enable(clk_gpio);
> +		if (ret)
> +			return ret;
> +
> +		ret = clk_prepare_enable(clk_port);
> +		if (ret) {
> +			clk_disable_unprepare(clk_gpio);
> +			return ret;
> +		}
> +	}
> +
>  	gc = &port->gc;
>  	gc->of_node = np;
>  	gc->parent = dev;

RE: [PATCH V2 4/8] gpio: vf610: add optional clock support

[A.s. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Friday, October 26, 2018 1:59 AM
[...]
> 
> On 23.10.2018 13:49, A.s. Dong wrote:
> > Some SoCs need the gpio clock to be enabled before accessing HW
> > registers. This patch add the optional clock handling.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: linux-gpio@vger.kernel.org
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > v1->v2:
> >  * new patch
> > ---
> >  drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> > index d4ad6d0..cbc4f44 100644
> > --- a/drivers/gpio/gpio-vf610.c
> > +++ b/drivers/gpio/gpio-vf610.c
> > @@ -16,6 +16,7 @@
> >   */
> >
> >  #include <linux/bitops.h>
> > +#include <linux/clk.h>
> >  #include <linux/err.h>
> >  #include <linux/gpio.h>
> >  #include <linux/init.h>
> > @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device
> > *pdev)  {
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> > +	struct clk *clk_gpio, *clk_port;
> >  	struct vf610_gpio_port *port;
> >  	struct resource *iores;
> >  	struct gpio_chip *gc;
> > @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device
> *pdev)
> >  	if (port->irq < 0)
> >  		return port->irq;
> >
> > +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> > +	clk_port = devm_clk_get(&pdev->dev, "port");
> 
> Are you sure that those are two independent clocks? On i.MX 7 usually there
> was a single clock gate controlling multiple clocks at once (which should be
> modeled as a single clock gate in the clock tree).
> 

Yes, they're two separate clocks in HW for gpio and port controller respectively.
One is for GPIO general purpose input/output function which another for port Interrupt.
Just like we have separate register ranges for gpio and port.

Regards
Dong Aisheng

> --
> Stefan
> 
> > +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
> > +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
> > +		return -EPROBE_DEFER;
> > +	} else if (!IS_ERR_OR_NULL(clk_gpio) &&
> > +		   !IS_ERR_OR_NULL(clk_port)) {
> > +		ret = clk_prepare_enable(clk_gpio);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = clk_prepare_enable(clk_port);
> > +		if (ret) {
> > +			clk_disable_unprepare(clk_gpio);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	gc = &port->gc;
> >  	gc->of_node = np;
> >  	gc->parent = dev;

Re: [PATCH V2 4/8] gpio: vf610: add optional clock support

[Stefan Agner]

On 26.10.2018 05:49, A.s. Dong wrote:
>> -----Original Message-----
>> From: Stefan Agner [mailto:stefan@agner.ch]
>> Sent: Friday, October 26, 2018 1:59 AM
> [...]
>>
>> On 23.10.2018 13:49, A.s. Dong wrote:
>> > Some SoCs need the gpio clock to be enabled before accessing HW
>> > registers. This patch add the optional clock handling.
>> >
>> > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > Cc: Stefan Agner <stefan@agner.ch>
>> > Cc: Shawn Guo <shawnguo@kernel.org>
>> > Cc: linux-gpio@vger.kernel.org
>> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > ---
>> > v1->v2:
>> >  * new patch
>> > ---
>> >  drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++
>> >  1 file changed, 20 insertions(+)
>> >
>> > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
>> > index d4ad6d0..cbc4f44 100644
>> > --- a/drivers/gpio/gpio-vf610.c
>> > +++ b/drivers/gpio/gpio-vf610.c
>> > @@ -16,6 +16,7 @@
>> >   */
>> >
>> >  #include <linux/bitops.h>
>> > +#include <linux/clk.h>
>> >  #include <linux/err.h>
>> >  #include <linux/gpio.h>
>> >  #include <linux/init.h>
>> > @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device
>> > *pdev)  {
>> >  	struct device *dev = &pdev->dev;
>> >  	struct device_node *np = dev->of_node;
>> > +	struct clk *clk_gpio, *clk_port;
>> >  	struct vf610_gpio_port *port;
>> >  	struct resource *iores;
>> >  	struct gpio_chip *gc;
>> > @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device
>> *pdev)
>> >  	if (port->irq < 0)
>> >  		return port->irq;
>> >
>> > +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
>> > +	clk_port = devm_clk_get(&pdev->dev, "port");
>>
>> Are you sure that those are two independent clocks? On i.MX 7 usually there
>> was a single clock gate controlling multiple clocks at once (which should be
>> modeled as a single clock gate in the clock tree).
>>
> 
> Yes, they're two separate clocks in HW for gpio and port controller
> respectively.
> One is for GPIO general purpose input/output function which another
> for port Interrupt.
> Just like we have separate register ranges for gpio and port.

Oh I see, that is the same with Vybrid actually. However, in Vybrid, for
some reason, there is only a clock for port (Port X multiplexing
control).

Can we make port clock independently optional?

E.g.

	clk_port = devm_clk_get(&pdev->dev, "port");
	if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
		return -EPROBE_DEFER;

	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
	if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
		return -EPROBE_DEFER;

	if (!IS_ERR_OR_NULL(clk_port)) {
		ret = clk_prepare_enable(clk_port);
		if (ret)
			return ret;
	}

	if (!IS_ERR_OR_NULL(clk_gpio))
		ret = clk_prepare_enable(clk_gpio);
		if (ret) {
			clk_disable_unprepare(clk_port);
			return ret;
		}
	}

Also there is some error handling a bit further down which needs proper
disabling the clocks.

--
Stefan


> 
> Regards
> Dong Aisheng
> 
>> --
>> Stefan
>>
>> > +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
>> > +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
>> > +		return -EPROBE_DEFER;
>> > +	} else if (!IS_ERR_OR_NULL(clk_gpio) &&
>> > +		   !IS_ERR_OR_NULL(clk_port)) {
>> > +		ret = clk_prepare_enable(clk_gpio);
>> > +		if (ret)
>> > +			return ret;
>> > +
>> > +		ret = clk_prepare_enable(clk_port);
>> > +		if (ret) {
>> > +			clk_disable_unprepare(clk_gpio);
>> > +			return ret;
>> > +		}
>> > +	}
>> > +
>> >  	gc = &port->gc;
>> >  	gc->of_node = np;
>> >  	gc->parent = dev;

RE: [PATCH V2 4/8] gpio: vf610: add optional clock support

[A.s. Dong]

Hi Stefan,

[...]
> 
> On 26.10.2018 05:49, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Stefan Agner [mailto:stefan@agner.ch]
> >> Sent: Friday, October 26, 2018 1:59 AM
> > [...]
> >>
> >> On 23.10.2018 13:49, A.s. Dong wrote:
> >> > Some SoCs need the gpio clock to be enabled before accessing HW
> >> > registers. This patch add the optional clock handling.
> >> >
> >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >> > Cc: Stefan Agner <stefan@agner.ch>
> >> > Cc: Shawn Guo <shawnguo@kernel.org>
> >> > Cc: linux-gpio@vger.kernel.org
> >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >> > ---
> >> > v1->v2:
> >> >  * new patch
> >> > ---
> >> >  drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++
> >> >  1 file changed, 20 insertions(+)
> >> >
> >> > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> >> > index d4ad6d0..cbc4f44 100644
> >> > --- a/drivers/gpio/gpio-vf610.c
> >> > +++ b/drivers/gpio/gpio-vf610.c
> >> > @@ -16,6 +16,7 @@
> >> >   */
> >> >
> >> >  #include <linux/bitops.h>
> >> > +#include <linux/clk.h>
> >> >  #include <linux/err.h>
> >> >  #include <linux/gpio.h>
> >> >  #include <linux/init.h>
> >> > @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct
> >> > platform_device
> >> > *pdev)  {
> >> >  	struct device *dev = &pdev->dev;
> >> >  	struct device_node *np = dev->of_node;
> >> > +	struct clk *clk_gpio, *clk_port;
> >> >  	struct vf610_gpio_port *port;
> >> >  	struct resource *iores;
> >> >  	struct gpio_chip *gc;
> >> > @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct
> >> > platform_device
> >> *pdev)
> >> >  	if (port->irq < 0)
> >> >  		return port->irq;
> >> >
> >> > +	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> >> > +	clk_port = devm_clk_get(&pdev->dev, "port");
> >>
> >> Are you sure that those are two independent clocks? On i.MX 7 usually
> >> there was a single clock gate controlling multiple clocks at once
> >> (which should be modeled as a single clock gate in the clock tree).
> >>
> >
> > Yes, they're two separate clocks in HW for gpio and port controller
> > respectively.
> > One is for GPIO general purpose input/output function which another
> > for port Interrupt.
> > Just like we have separate register ranges for gpio and port.
> 
> Oh I see, that is the same with Vybrid actually. However, in Vybrid, for some
> reason, there is only a clock for port (Port X multiplexing control).
> 
> Can we make port clock independently optional?
> 
> E.g.
> 
> 	clk_port = devm_clk_get(&pdev->dev, "port");
> 	if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
> 		return -EPROBE_DEFER;
> 
> 	clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> 	if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
> 		return -EPROBE_DEFER;
> 
> 	if (!IS_ERR_OR_NULL(clk_port)) {
> 		ret = clk_prepare_enable(clk_port);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	if (!IS_ERR_OR_NULL(clk_gpio))
> 		ret = clk_prepare_enable(clk_gpio);
> 		if (ret) {
> 			clk_disable_unprepare(clk_port);
> 			return ret;
> 		}
> 	}
> 
> Also there is some error handling a bit further down which needs proper
> disabling the clocks.
> 

Got it, thanks for the suggestion.
Will change in next version.

Regards
Dong Aisheng

> --
> Stefan
> 
> 
> >
> > Regards
> > Dong Aisheng
> >
> >> --
> >> Stefan
> >>
> >> > +	if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) ||
> >> > +	    (PTR_ERR(clk_port) == -EPROBE_DEFER)) {
> >> > +		return -EPROBE_DEFER;
> >> > +	} else if (!IS_ERR_OR_NULL(clk_gpio) &&
> >> > +		   !IS_ERR_OR_NULL(clk_port)) {
> >> > +		ret = clk_prepare_enable(clk_gpio);
> >> > +		if (ret)
> >> > +			return ret;
> >> > +
> >> > +		ret = clk_prepare_enable(clk_port);
> >> > +		if (ret) {
> >> > +			clk_disable_unprepare(clk_gpio);
> >> > +			return ret;
> >> > +		}
> >> > +	}
> >> > +
> >> >  	gc = &port->gc;
> >> >  	gc->of_node = np;
> >> >  	gc->parent = dev;