From mboxrd@z Thu Jan 1 00:00:00 1970 From: "A.s. Dong" Subject: RE: [PATCH V2 4/8] gpio: vf610: add optional clock support Date: Tue, 30 Oct 2018 15:34:20 +0000 Message-ID: References: <1540295058-26090-1-git-send-email-aisheng.dong@nxp.com> <1540295058-26090-5-git-send-email-aisheng.dong@nxp.com> <24253e7336192f7afeefe385953d04b3@agner.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <24253e7336192f7afeefe385953d04b3@agner.ch> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Stefan Agner Cc: "dongas86@gmail.com" , Linus Walleij , "linux@armlinux.org.uk" , "linux-gpio@vger.kernel.org" , "robh+dt@kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , Fabio Estevam , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" List-Id: linux-gpio@vger.kernel.org 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 > >> > Cc: Stefan Agner > >> > Cc: Shawn Guo > >> > Cc: linux-gpio@vger.kernel.org > >> > Signed-off-by: Dong Aisheng > >> > --- > >> > 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 > >> > +#include > >> > #include > >> > #include > >> > #include > >> > @@ -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; From mboxrd@z Thu Jan 1 00:00:00 1970 From: aisheng.dong@nxp.com (A.s. Dong) Date: Tue, 30 Oct 2018 15:34:20 +0000 Subject: [PATCH V2 4/8] gpio: vf610: add optional clock support In-Reply-To: <24253e7336192f7afeefe385953d04b3@agner.ch> References: <1540295058-26090-1-git-send-email-aisheng.dong@nxp.com> <1540295058-26090-5-git-send-email-aisheng.dong@nxp.com> <24253e7336192f7afeefe385953d04b3@agner.ch> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Stefan, [...] > > On 26.10.2018 05:49, A.s. Dong wrote: > >> -----Original Message----- > >> From: Stefan Agner [mailto:stefan at 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 > >> > Cc: Stefan Agner > >> > Cc: Shawn Guo > >> > Cc: linux-gpio at vger.kernel.org > >> > Signed-off-by: Dong Aisheng > >> > --- > >> > 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 > >> > +#include > >> > #include > >> > #include > >> > #include > >> > @@ -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;