Patchwork [v2,1/6] usb: xhci-mtk: get optional clock by devm_clk_get_optional()

login
register
mail settings
Submitter Chunfeng Yun
Date April 10, 2019, 6:47 a.m.
Message ID <bc6cdc2dce698dddbd692ba100fbf08c15c7b023.1554877692.git.chunfeng.yun@mediatek.com>
Download mbox | patch
Permalink /patch/769445/
State New
Headers show

Comments

Chunfeng Yun - April 10, 2019, 6:47 a.m.
Use devm_clk_get_optional() to get optional clock

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: no changes
---
 drivers/usb/host/xhci-mtk.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)
Greg Kroah-Hartman - April 16, 2019, 10:12 a.m.
On Wed, Apr 10, 2019 at 02:47:24PM +0800, Chunfeng Yun wrote:
> Use devm_clk_get_optional() to get optional clock
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2: no changes
> ---
>  drivers/usb/host/xhci-mtk.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> index 60987c787e44..026fe18972d3 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -206,19 +206,6 @@ static int xhci_mtk_ssusb_config(struct xhci_hcd_mtk *mtk)
>  	return xhci_mtk_host_enable(mtk);
>  }
>  
> -/* ignore the error if the clock does not exist */
> -static struct clk *optional_clk_get(struct device *dev, const char *id)
> -{
> -	struct clk *opt_clk;
> -
> -	opt_clk = devm_clk_get(dev, id);
> -	/* ignore error number except EPROBE_DEFER */
> -	if (IS_ERR(opt_clk) && (PTR_ERR(opt_clk) != -EPROBE_DEFER))

Are you sure about this?

devm_clk_get_optional() does not check for -EPROBE_DEFER, so you just
changed the functionality of this code.  Is that ok?  If so, please call
it out explicitly in your changelog comment when you resend this.

thanks,

greg k-h
Greg Kroah-Hartman - April 16, 2019, 10:15 a.m.
On Wed, Apr 10, 2019 at 02:47:24PM +0800, Chunfeng Yun wrote:
> Use devm_clk_get_optional() to get optional clock
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2: no changes
> ---
>  drivers/usb/host/xhci-mtk.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> index 60987c787e44..026fe18972d3 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -206,19 +206,6 @@ static int xhci_mtk_ssusb_config(struct xhci_hcd_mtk *mtk)
>  	return xhci_mtk_host_enable(mtk);
>  }
>  
> -/* ignore the error if the clock does not exist */
> -static struct clk *optional_clk_get(struct device *dev, const char *id)
> -{
> -	struct clk *opt_clk;
> -
> -	opt_clk = devm_clk_get(dev, id);
> -	/* ignore error number except EPROBE_DEFER */
> -	if (IS_ERR(opt_clk) && (PTR_ERR(opt_clk) != -EPROBE_DEFER))
> -		opt_clk = NULL;
> -
> -	return opt_clk;
> -}
> -
>  static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
>  {
>  	struct device *dev = mtk->dev;
> @@ -229,15 +216,15 @@ static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
>  		return PTR_ERR(mtk->sys_clk);
>  	}
>  
> -	mtk->ref_clk = optional_clk_get(dev, "ref_ck");
> +	mtk->ref_clk = devm_clk_get_optional(dev, "ref_ck");
>  	if (IS_ERR(mtk->ref_clk))
>  		return PTR_ERR(mtk->ref_clk);
>  
> -	mtk->mcu_clk = optional_clk_get(dev, "mcu_ck");
> +	mtk->mcu_clk = devm_clk_get_optional(dev, "mcu_ck");
>  	if (IS_ERR(mtk->mcu_clk))
>  		return PTR_ERR(mtk->mcu_clk);
>  
> -	mtk->dma_clk = optional_clk_get(dev, "dma_ck");
> +	mtk->dma_clk = devm_clk_get_optional(dev, "dma_ck");

Also, now that this clock (and the others here) are controlled by the
devm infrastructure, do you have to change when you were cleaning up and
stopping the clock?  That logic seems to be the same here, which does
not seem to be correct to me.

Please also document that in your changelog comments when resending this
series, as all of these patches have this issue as well.

thanks,

greg k-h
Chunfeng Yun - April 17, 2019, 5:53 a.m.
On Tue, 2019-04-16 at 12:12 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 10, 2019 at 02:47:24PM +0800, Chunfeng Yun wrote:
> > Use devm_clk_get_optional() to get optional clock
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: no changes
> > ---
> >  drivers/usb/host/xhci-mtk.c | 19 +++----------------
> >  1 file changed, 3 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> > index 60987c787e44..026fe18972d3 100644
> > --- a/drivers/usb/host/xhci-mtk.c
> > +++ b/drivers/usb/host/xhci-mtk.c
> > @@ -206,19 +206,6 @@ static int xhci_mtk_ssusb_config(struct xhci_hcd_mtk *mtk)
> >  	return xhci_mtk_host_enable(mtk);
> >  }
> >  
> > -/* ignore the error if the clock does not exist */
> > -static struct clk *optional_clk_get(struct device *dev, const char *id)
> > -{
> > -	struct clk *opt_clk;
> > -
> > -	opt_clk = devm_clk_get(dev, id);
> > -	/* ignore error number except EPROBE_DEFER */
> > -	if (IS_ERR(opt_clk) && (PTR_ERR(opt_clk) != -EPROBE_DEFER))
> 
> Are you sure about this?
> 
> devm_clk_get_optional() does not check for -EPROBE_DEFER, so you just
> changed the functionality of this code.  Is that ok?  
Yes, checking for -ENOENT covers more error cases than -EPROBE_DEFER,
and the original purpose also wants to ignore non-exist clock

> If so, please call
> it out explicitly in your changelog comment when you resend this.
I'll add it in next version

Thanks a lot

> 
> thanks,
> 
> greg k-h
Chunfeng Yun - April 17, 2019, 5:53 a.m.
On Tue, 2019-04-16 at 12:15 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 10, 2019 at 02:47:24PM +0800, Chunfeng Yun wrote:
> > Use devm_clk_get_optional() to get optional clock
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: no changes
> > ---
> >  drivers/usb/host/xhci-mtk.c | 19 +++----------------
> >  1 file changed, 3 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> > index 60987c787e44..026fe18972d3 100644
> > --- a/drivers/usb/host/xhci-mtk.c
> > +++ b/drivers/usb/host/xhci-mtk.c
> > @@ -206,19 +206,6 @@ static int xhci_mtk_ssusb_config(struct xhci_hcd_mtk *mtk)
> >  	return xhci_mtk_host_enable(mtk);
> >  }
> >  
> > -/* ignore the error if the clock does not exist */
> > -static struct clk *optional_clk_get(struct device *dev, const char *id)
> > -{
> > -	struct clk *opt_clk;
> > -
> > -	opt_clk = devm_clk_get(dev, id);
> > -	/* ignore error number except EPROBE_DEFER */
> > -	if (IS_ERR(opt_clk) && (PTR_ERR(opt_clk) != -EPROBE_DEFER))
> > -		opt_clk = NULL;
> > -
> > -	return opt_clk;
> > -}
> > -
> >  static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
> >  {
> >  	struct device *dev = mtk->dev;
> > @@ -229,15 +216,15 @@ static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
> >  		return PTR_ERR(mtk->sys_clk);
> >  	}
> >  
> > -	mtk->ref_clk = optional_clk_get(dev, "ref_ck");
> > +	mtk->ref_clk = devm_clk_get_optional(dev, "ref_ck");
> >  	if (IS_ERR(mtk->ref_clk))
> >  		return PTR_ERR(mtk->ref_clk);
> >  
> > -	mtk->mcu_clk = optional_clk_get(dev, "mcu_ck");
> > +	mtk->mcu_clk = devm_clk_get_optional(dev, "mcu_ck");
> >  	if (IS_ERR(mtk->mcu_clk))
> >  		return PTR_ERR(mtk->mcu_clk);
> >  
> > -	mtk->dma_clk = optional_clk_get(dev, "dma_ck");
> > +	mtk->dma_clk = devm_clk_get_optional(dev, "dma_ck");
> 
> Also, now that this clock (and the others here) are controlled by the
> devm infrastructure, do you have to change when you were cleaning up and
> stopping the clock?  
No need, optional_clk_get() uses devm_clk_get() to get clock

Thanks

> That logic seems to be the same here, which does
> not seem to be correct to me.
> 
> Please also document that in your changelog comments when resending this
> series, as all of these patches have this issue as well.
> 
> thanks,
> 
> greg k-h

Patch

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 60987c787e44..026fe18972d3 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -206,19 +206,6 @@  static int xhci_mtk_ssusb_config(struct xhci_hcd_mtk *mtk)
 	return xhci_mtk_host_enable(mtk);
 }
 
-/* ignore the error if the clock does not exist */
-static struct clk *optional_clk_get(struct device *dev, const char *id)
-{
-	struct clk *opt_clk;
-
-	opt_clk = devm_clk_get(dev, id);
-	/* ignore error number except EPROBE_DEFER */
-	if (IS_ERR(opt_clk) && (PTR_ERR(opt_clk) != -EPROBE_DEFER))
-		opt_clk = NULL;
-
-	return opt_clk;
-}
-
 static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
 {
 	struct device *dev = mtk->dev;
@@ -229,15 +216,15 @@  static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
 		return PTR_ERR(mtk->sys_clk);
 	}
 
-	mtk->ref_clk = optional_clk_get(dev, "ref_ck");
+	mtk->ref_clk = devm_clk_get_optional(dev, "ref_ck");
 	if (IS_ERR(mtk->ref_clk))
 		return PTR_ERR(mtk->ref_clk);
 
-	mtk->mcu_clk = optional_clk_get(dev, "mcu_ck");
+	mtk->mcu_clk = devm_clk_get_optional(dev, "mcu_ck");
 	if (IS_ERR(mtk->mcu_clk))
 		return PTR_ERR(mtk->mcu_clk);
 
-	mtk->dma_clk = optional_clk_get(dev, "dma_ck");
+	mtk->dma_clk = devm_clk_get_optional(dev, "dma_ck");
 	return PTR_ERR_OR_ZERO(mtk->dma_clk);
 }