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

["A.s. Dong" <aisheng.dong@xxxxxxx>]

Hi Stefan,

[...]
> 
> On 26.10.2018 05:49, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Stefan Agner [mailto:stefan@xxxxxxxx]
> >> 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@xxxxxxxxxx>
> >> > Cc: Stefan Agner <stefan@xxxxxxxx>
> >> > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> >> > Cc: linux-gpio@xxxxxxxxxxxxxxx
> >> > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxx>
> >> > ---
> >> > 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;