diff mbox series

[-next,v2,11/25] spi: dw-bt1: Use helper function devm_clk_get_enabled()

Message ID 20230823133938.1359106-12-lizetao1@huawei.com (mailing list archive)
State New, archived
Headers show
Series [-next,v2,01/25] spi: ar934x: Use helper function devm_clk_get_enabled() | expand

Commit Message

Li Zetao Aug. 23, 2023, 1:39 p.m. UTC
Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
replaced by devm_clk_get_enabled() when driver enables (and possibly
prepares) the clocks for the whole lifetime of the device. Moreover, it is
no longer necessary to unprepare and disable the clocks explicitly.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: None

 drivers/spi/spi-dw-bt1.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

Comments

Serge Semin Aug. 23, 2023, 2:35 p.m. UTC | #1
On Wed, Aug 23, 2023 at 09:39:24PM +0800, Li Zetao wrote:
> Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
> and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
> replaced by devm_clk_get_enabled() when driver enables (and possibly
> prepares) the clocks for the whole lifetime of the device. Moreover, it is
> no longer necessary to unprepare and disable the clocks explicitly.
> 

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Acked-by: Serge Semin <fancer.lancer@gmail.com>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>

Just for the record, the tags order is normally supposed to reflect a
patch route. Even though AFAICS it's explicitly defined for the Sob
tags only (see Documentation/process/submitting-patches.rst), but
almost all kernel developers apply that rule to all the tags. See the
kernel git log as the brightest example. So your Sob tag should have
been kept first, then either my Ab or Jonathan' Rb tags.

-Serge(y)

> ---
> v1 -> v2: None
> 
>  drivers/spi/spi-dw-bt1.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-bt1.c b/drivers/spi/spi-dw-bt1.c
> index 5e1c01822967..5391bcac305c 100644
> --- a/drivers/spi/spi-dw-bt1.c
> +++ b/drivers/spi/spi-dw-bt1.c
> @@ -269,43 +269,32 @@ static int dw_spi_bt1_probe(struct platform_device *pdev)
>  
>  	dws->paddr = mem->start;
>  
> -	dwsbt1->clk = devm_clk_get(&pdev->dev, NULL);
> +	dwsbt1->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>  	if (IS_ERR(dwsbt1->clk))
>  		return PTR_ERR(dwsbt1->clk);
>  
> -	ret = clk_prepare_enable(dwsbt1->clk);
> -	if (ret)
> -		return ret;
> -
>  	dws->bus_num = pdev->id;
>  	dws->reg_io_width = 4;
>  	dws->max_freq = clk_get_rate(dwsbt1->clk);
> -	if (!dws->max_freq) {
> -		ret = -EINVAL;
> -		goto err_disable_clk;
> -	}
> +	if (!dws->max_freq)
> +		return -EINVAL;
>  
>  	init_func = device_get_match_data(&pdev->dev);
>  	ret = init_func(pdev, dwsbt1);
>  	if (ret)
> -		goto err_disable_clk;
> +		return ret;
>  
>  	pm_runtime_enable(&pdev->dev);
>  
>  	ret = dw_spi_add_host(&pdev->dev, dws);
>  	if (ret) {
>  		pm_runtime_disable(&pdev->dev);
> -		goto err_disable_clk;
> +		return ret;
>  	}
>  
>  	platform_set_drvdata(pdev, dwsbt1);
>  
>  	return 0;
> -
> -err_disable_clk:
> -	clk_disable_unprepare(dwsbt1->clk);
> -
> -	return ret;
>  }
>  
>  static void dw_spi_bt1_remove(struct platform_device *pdev)
> @@ -315,8 +304,6 @@ static void dw_spi_bt1_remove(struct platform_device *pdev)
>  	dw_spi_remove_host(&dwsbt1->dws);
>  
>  	pm_runtime_disable(&pdev->dev);
> -
> -	clk_disable_unprepare(dwsbt1->clk);
>  }
>  
>  static const struct of_device_id dw_spi_bt1_of_match[] = {
> -- 
> 2.34.1
>
Jonathan Cameron Aug. 23, 2023, 5:13 p.m. UTC | #2
On Wed, 23 Aug 2023 17:35:35 +0300
Serge Semin <fancer.lancer@gmail.com> wrote:

> On Wed, Aug 23, 2023 at 09:39:24PM +0800, Li Zetao wrote:
> > Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
> > and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
> > replaced by devm_clk_get_enabled() when driver enables (and possibly
> > prepares) the clocks for the whole lifetime of the device. Moreover, it is
> > no longer necessary to unprepare and disable the clocks explicitly.
> >   
> 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Acked-by: Serge Semin <fancer.lancer@gmail.com>
> > Signed-off-by: Li Zetao <lizetao1@huawei.com>  
> 
> Just for the record, the tags order is normally supposed to reflect a
> patch route. Even though AFAICS it's explicitly defined for the Sob
> tags only (see Documentation/process/submitting-patches.rst), but
> almost all kernel developers apply that rule to all the tags. See the
> kernel git log as the brightest example. So your Sob tag should have
> been kept first, then either my Ab or Jonathan' Rb tags.

There have been far too many discussions about this over the years.
There is an argument (I'm not saying I agree with it) that tags given
on an earlier version than the one posted should be before the SoB of
the author on basis that the SoB is occurring later (as it's on the new
version).  That is a slightly stronger argument if there are changes
to the patch, but then things get inconsistent between patches with minor
changes and those without. *sigh*

So this pattern is fairly common if a bit weird in my view :)

Jonathan

> 
> -Serge(y)
> 
> > ---
> > v1 -> v2: None
> > 
> >  drivers/spi/spi-dw-bt1.c | 23 +++++------------------
> >  1 file changed, 5 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-dw-bt1.c b/drivers/spi/spi-dw-bt1.c
> > index 5e1c01822967..5391bcac305c 100644
> > --- a/drivers/spi/spi-dw-bt1.c
> > +++ b/drivers/spi/spi-dw-bt1.c
> > @@ -269,43 +269,32 @@ static int dw_spi_bt1_probe(struct platform_device *pdev)
> >  
> >  	dws->paddr = mem->start;
> >  
> > -	dwsbt1->clk = devm_clk_get(&pdev->dev, NULL);
> > +	dwsbt1->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> >  	if (IS_ERR(dwsbt1->clk))
> >  		return PTR_ERR(dwsbt1->clk);
> >  
> > -	ret = clk_prepare_enable(dwsbt1->clk);
> > -	if (ret)
> > -		return ret;
> > -
> >  	dws->bus_num = pdev->id;
> >  	dws->reg_io_width = 4;
> >  	dws->max_freq = clk_get_rate(dwsbt1->clk);
> > -	if (!dws->max_freq) {
> > -		ret = -EINVAL;
> > -		goto err_disable_clk;
> > -	}
> > +	if (!dws->max_freq)
> > +		return -EINVAL;
> >  
> >  	init_func = device_get_match_data(&pdev->dev);
> >  	ret = init_func(pdev, dwsbt1);
> >  	if (ret)
> > -		goto err_disable_clk;
> > +		return ret;
> >  
> >  	pm_runtime_enable(&pdev->dev);
> >  
> >  	ret = dw_spi_add_host(&pdev->dev, dws);
> >  	if (ret) {
> >  		pm_runtime_disable(&pdev->dev);
> > -		goto err_disable_clk;
> > +		return ret;
> >  	}
> >  
> >  	platform_set_drvdata(pdev, dwsbt1);
> >  
> >  	return 0;
> > -
> > -err_disable_clk:
> > -	clk_disable_unprepare(dwsbt1->clk);
> > -
> > -	return ret;
> >  }
> >  
> >  static void dw_spi_bt1_remove(struct platform_device *pdev)
> > @@ -315,8 +304,6 @@ static void dw_spi_bt1_remove(struct platform_device *pdev)
> >  	dw_spi_remove_host(&dwsbt1->dws);
> >  
> >  	pm_runtime_disable(&pdev->dev);
> > -
> > -	clk_disable_unprepare(dwsbt1->clk);
> >  }
> >  
> >  static const struct of_device_id dw_spi_bt1_of_match[] = {
> > -- 
> > 2.34.1
> >
Serge Semin Aug. 25, 2023, 6:01 p.m. UTC | #3
On Wed, Aug 23, 2023 at 06:13:44PM +0100, Jonathan Cameron wrote:
> On Wed, 23 Aug 2023 17:35:35 +0300
> Serge Semin <fancer.lancer@gmail.com> wrote:
> 
> > On Wed, Aug 23, 2023 at 09:39:24PM +0800, Li Zetao wrote:
> > > Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
> > > and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
> > > replaced by devm_clk_get_enabled() when driver enables (and possibly
> > > prepares) the clocks for the whole lifetime of the device. Moreover, it is
> > > no longer necessary to unprepare and disable the clocks explicitly.
> > >   
> > 
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Acked-by: Serge Semin <fancer.lancer@gmail.com>
> > > Signed-off-by: Li Zetao <lizetao1@huawei.com>  
> > 
> > Just for the record, the tags order is normally supposed to reflect a
> > patch route. Even though AFAICS it's explicitly defined for the Sob
> > tags only (see Documentation/process/submitting-patches.rst), but
> > almost all kernel developers apply that rule to all the tags. See the
> > kernel git log as the brightest example. So your Sob tag should have
> > been kept first, then either my Ab or Jonathan' Rb tags.
> 

> There have been far too many discussions about this over the years.
> There is an argument (I'm not saying I agree with it) that tags given
> on an earlier version than the one posted should be before the SoB of
> the author on basis that the SoB is occurring later (as it's on the new
> version).  That is a slightly stronger argument if there are changes
> to the patch, but then things get inconsistent between patches with minor
> changes and those without. *sigh*
> 
> So this pattern is fairly common if a bit weird in my view :)

Ah, I didn't know it was a holy-warring topic.) My judgement was based
on the vast majority of commits in the kernel git log and on my
experience in the patches review. Anyway in order not to raise it
again let's consider my comment as a general note with no obligation
towards the patch author.)

-Serge(y)

> 
> Jonathan
> 
> > 
> > -Serge(y)
> > 
> > > ---
> > > v1 -> v2: None
> > > 
> > >  drivers/spi/spi-dw-bt1.c | 23 +++++------------------
> > >  1 file changed, 5 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-dw-bt1.c b/drivers/spi/spi-dw-bt1.c
> > > index 5e1c01822967..5391bcac305c 100644
> > > --- a/drivers/spi/spi-dw-bt1.c
> > > +++ b/drivers/spi/spi-dw-bt1.c
> > > @@ -269,43 +269,32 @@ static int dw_spi_bt1_probe(struct platform_device *pdev)
> > >  
> > >  	dws->paddr = mem->start;
> > >  
> > > -	dwsbt1->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	dwsbt1->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > >  	if (IS_ERR(dwsbt1->clk))
> > >  		return PTR_ERR(dwsbt1->clk);
> > >  
> > > -	ret = clk_prepare_enable(dwsbt1->clk);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	dws->bus_num = pdev->id;
> > >  	dws->reg_io_width = 4;
> > >  	dws->max_freq = clk_get_rate(dwsbt1->clk);
> > > -	if (!dws->max_freq) {
> > > -		ret = -EINVAL;
> > > -		goto err_disable_clk;
> > > -	}
> > > +	if (!dws->max_freq)
> > > +		return -EINVAL;
> > >  
> > >  	init_func = device_get_match_data(&pdev->dev);
> > >  	ret = init_func(pdev, dwsbt1);
> > >  	if (ret)
> > > -		goto err_disable_clk;
> > > +		return ret;
> > >  
> > >  	pm_runtime_enable(&pdev->dev);
> > >  
> > >  	ret = dw_spi_add_host(&pdev->dev, dws);
> > >  	if (ret) {
> > >  		pm_runtime_disable(&pdev->dev);
> > > -		goto err_disable_clk;
> > > +		return ret;
> > >  	}
> > >  
> > >  	platform_set_drvdata(pdev, dwsbt1);
> > >  
> > >  	return 0;
> > > -
> > > -err_disable_clk:
> > > -	clk_disable_unprepare(dwsbt1->clk);
> > > -
> > > -	return ret;
> > >  }
> > >  
> > >  static void dw_spi_bt1_remove(struct platform_device *pdev)
> > > @@ -315,8 +304,6 @@ static void dw_spi_bt1_remove(struct platform_device *pdev)
> > >  	dw_spi_remove_host(&dwsbt1->dws);
> > >  
> > >  	pm_runtime_disable(&pdev->dev);
> > > -
> > > -	clk_disable_unprepare(dwsbt1->clk);
> > >  }
> > >  
> > >  static const struct of_device_id dw_spi_bt1_of_match[] = {
> > > -- 
> > > 2.34.1
> > >   
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-bt1.c b/drivers/spi/spi-dw-bt1.c
index 5e1c01822967..5391bcac305c 100644
--- a/drivers/spi/spi-dw-bt1.c
+++ b/drivers/spi/spi-dw-bt1.c
@@ -269,43 +269,32 @@  static int dw_spi_bt1_probe(struct platform_device *pdev)
 
 	dws->paddr = mem->start;
 
-	dwsbt1->clk = devm_clk_get(&pdev->dev, NULL);
+	dwsbt1->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(dwsbt1->clk))
 		return PTR_ERR(dwsbt1->clk);
 
-	ret = clk_prepare_enable(dwsbt1->clk);
-	if (ret)
-		return ret;
-
 	dws->bus_num = pdev->id;
 	dws->reg_io_width = 4;
 	dws->max_freq = clk_get_rate(dwsbt1->clk);
-	if (!dws->max_freq) {
-		ret = -EINVAL;
-		goto err_disable_clk;
-	}
+	if (!dws->max_freq)
+		return -EINVAL;
 
 	init_func = device_get_match_data(&pdev->dev);
 	ret = init_func(pdev, dwsbt1);
 	if (ret)
-		goto err_disable_clk;
+		return ret;
 
 	pm_runtime_enable(&pdev->dev);
 
 	ret = dw_spi_add_host(&pdev->dev, dws);
 	if (ret) {
 		pm_runtime_disable(&pdev->dev);
-		goto err_disable_clk;
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, dwsbt1);
 
 	return 0;
-
-err_disable_clk:
-	clk_disable_unprepare(dwsbt1->clk);
-
-	return ret;
 }
 
 static void dw_spi_bt1_remove(struct platform_device *pdev)
@@ -315,8 +304,6 @@  static void dw_spi_bt1_remove(struct platform_device *pdev)
 	dw_spi_remove_host(&dwsbt1->dws);
 
 	pm_runtime_disable(&pdev->dev);
-
-	clk_disable_unprepare(dwsbt1->clk);
 }
 
 static const struct of_device_id dw_spi_bt1_of_match[] = {