diff mbox series

[4/6] drivers/thermal/rcar_gen3_thermal: Convert to devm_platform_ioremap_resource()

Message ID 20230626124334.15100-4-frank.li@vivo.com
State New
Headers
Series [1/6] drivers/thermal/k3: Convert to devm_platform_ioremap_resource() |

Commit Message

李扬韬 June 26, 2023, 12:43 p.m. UTC 
  Use devm_platform_ioremap_resource() to simplify code.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Niklas Söderlund June 26, 2023, 4:28 p.m. UTC | #1 
Hi Yangtao,

Thanks for your work.

On 2023-06-26 20:43:31 +0800, Yangtao Li wrote:
> Use devm_platform_ioremap_resource() to simplify code.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>

This do indeed simplify the code, but it also breaks the driver :-)

Before the change, failing to find a resource at position "i", breaks 
the probe loop, and probing continues and the number of resource 
described are the number of TSC find are used.

After the change failing to find all possible TCS will fail the whole 
probe process, even if some TCS where described. And not describing max 
number of TCS on each system is perfectly fine.

Nacked-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  drivers/thermal/rcar_gen3_thermal.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 9029d01e029b..5c623f13d9ec 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -481,7 +481,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  {
>  	struct rcar_gen3_thermal_priv *priv;
>  	struct device *dev = &pdev->dev;
> -	struct resource *res;
>  	struct thermal_zone_device *zone;
>  	unsigned int i;
>  	int ret;
> @@ -504,17 +503,13 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  	for (i = 0; i < TSC_MAX_NUM; i++) {
>  		struct rcar_gen3_thermal_tsc *tsc;
>  
> -		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> -		if (!res)
> -			break;
> -
>  		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
>  		if (!tsc) {
>  			ret = -ENOMEM;
>  			goto error_unregister;
>  		}
>  
> -		tsc->base = devm_ioremap_resource(dev, res);
> +		tsc->base = devm_platform_ioremap_resource(pdev, i);
>  		if (IS_ERR(tsc->base)) {
>  			ret = PTR_ERR(tsc->base);
>  			goto error_unregister;
> -- 
> 2.39.0
>
李扬韬 June 27, 2023, 2:58 a.m. UTC | #2 
Hi Niklas,

On 2023/6/27 0:28, Niklas Söderlund wrote:

> Hi Yangtao,
>
> Thanks for your work.
>
> On 2023-06-26 20:43:31 +0800, Yangtao Li wrote:
>> Use devm_platform_ioremap_resource() to simplify code.
>>
>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> This do indeed simplify the code, but it also breaks the driver :-)

How about the patch below? Can the following rcar driver also take a 
similar approach?


diff --git a/drivers/thermal/rcar_gen3_thermal.c 
b/drivers/thermal/rcar_gen3_thermal.c
index 9029d01e029b..0cd9a030eb9e 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -481,7 +481,6 @@ static int rcar_gen3_thermal_probe(struct 
platform_device *pdev)
  {
         struct rcar_gen3_thermal_priv *priv;
         struct device *dev = &pdev->dev;
-       struct resource *res;
         struct thermal_zone_device *zone;
         unsigned int i;
         int ret;
@@ -503,22 +502,23 @@ static int rcar_gen3_thermal_probe(struct 
platform_device *pdev)

         for (i = 0; i < TSC_MAX_NUM; i++) {
                 struct rcar_gen3_thermal_tsc *tsc;
+               void __iomem *base;

-               res = platform_get_resource(pdev, IORESOURCE_MEM, i);
-               if (!res)
-                       break;
+               base = devm_platform_ioremap_resource(pdev, i);
+               if (IS_ERR(base)) {
+                       if (PTR_ERR(base) == -EINVAL)
+                               break;

-               tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
-               if (!tsc) {
-                       ret = -ENOMEM;
+                       ret = PTR_ERR(base);
                         goto error_unregister;
                 }

-               tsc->base = devm_ioremap_resource(dev, res);
-               if (IS_ERR(tsc->base)) {
-                       ret = PTR_ERR(tsc->base);
+               tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
+               if (!tsc) {
+                       ret = -ENOMEM;
                         goto error_unregister;
                 }
+               tsc->base = base;

                 priv->tscs[i] = tsc;
         }


> Before the change, failing to find a resource at position "i", breaks
> the probe loop, and probing continues and the number of resource
> described are the number of TSC find are used.
>
> After the change failing to find all possible TCS will fail the whole
> probe process, even if some TCS where described. And not describing max
> number of TCS on each system is perfectly fine.
>
> Nacked-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
>> ---
>>   drivers/thermal/rcar_gen3_thermal.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
>> index 9029d01e029b..5c623f13d9ec 100644
>> --- a/drivers/thermal/rcar_gen3_thermal.c
>> +++ b/drivers/thermal/rcar_gen3_thermal.c
>> @@ -481,7 +481,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>>   {
>>   	struct rcar_gen3_thermal_priv *priv;
>>   	struct device *dev = &pdev->dev;
>> -	struct resource *res;
>>   	struct thermal_zone_device *zone;
>>   	unsigned int i;
>>   	int ret;
>> @@ -504,17 +503,13 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>>   	for (i = 0; i < TSC_MAX_NUM; i++) {
>>   		struct rcar_gen3_thermal_tsc *tsc;
>>   
>> -		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>> -		if (!res)
>> -			break;
>> -
>>   		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
>>   		if (!tsc) {
>>   			ret = -ENOMEM;
>>   			goto error_unregister;
>>   		}
>>   
>> -		tsc->base = devm_ioremap_resource(dev, res);
>> +		tsc->base = devm_platform_ioremap_resource(pdev, i);
>>   		if (IS_ERR(tsc->base)) {
>>   			ret = PTR_ERR(tsc->base);
>>   			goto error_unregister;
>> -- 
>> 2.39.0
>>
Niklas Söderlund June 27, 2023, 7:47 a.m. UTC | #3 
Hi Yangtao,

On 2023-06-27 10:58:16 +0800, Yangtao Li wrote:
> Hi Niklas,
> 
> On 2023/6/27 0:28, Niklas Söderlund wrote:
> 
> > Hi Yangtao,
> > 
> > Thanks for your work.
> > 
> > On 2023-06-26 20:43:31 +0800, Yangtao Li wrote:
> > > Use devm_platform_ioremap_resource() to simplify code.
> > > 
> > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > This do indeed simplify the code, but it also breaks the driver :-)
> 
> How about the patch below? Can the following rcar driver also take a similar
> approach?

Maybe it could, I would need to test it and I'm traveling this week with 
no access to hardware. But I don't like the change you propose, as it 
makes the code more complex without any other goal then to 
s/platform_get_resource/devm_platform_ioremap_resource/.

If you have a reason, like trying to remove platform_get_resource() from 
the kernel or such I will consider the change. But if you only want to 
change things *because* I think the current code do the right thing in a 
clear way, look for resource, if found map it else use what resources 
have been found already. Adding special case based on return code is 
IMHO more complex, again if you need to go that route please add a 
comment describing the special case.

> 
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c
> b/drivers/thermal/rcar_gen3_thermal.c
> index 9029d01e029b..0cd9a030eb9e 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -481,7 +481,6 @@ static int rcar_gen3_thermal_probe(struct
> platform_device *pdev)
>  {
>         struct rcar_gen3_thermal_priv *priv;
>         struct device *dev = &pdev->dev;
> -       struct resource *res;
>         struct thermal_zone_device *zone;
>         unsigned int i;
>         int ret;
> @@ -503,22 +502,23 @@ static int rcar_gen3_thermal_probe(struct
> platform_device *pdev)
> 
>         for (i = 0; i < TSC_MAX_NUM; i++) {
>                 struct rcar_gen3_thermal_tsc *tsc;
> +               void __iomem *base;
> 
> -               res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> -               if (!res)
> -                       break;
> +               base = devm_platform_ioremap_resource(pdev, i);
> +               if (IS_ERR(base)) {
> +                       if (PTR_ERR(base) == -EINVAL)
> +                               break;
> 
> -               tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> -               if (!tsc) {
> -                       ret = -ENOMEM;
> +                       ret = PTR_ERR(base);
>                         goto error_unregister;
>                 }
> 
> -               tsc->base = devm_ioremap_resource(dev, res);
> -               if (IS_ERR(tsc->base)) {
> -                       ret = PTR_ERR(tsc->base);
> +               tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> +               if (!tsc) {
> +                       ret = -ENOMEM;
>                         goto error_unregister;
>                 }
> +               tsc->base = base;
> 
>                 priv->tscs[i] = tsc;
>         }
> 
> 
> > Before the change, failing to find a resource at position "i", breaks
> > the probe loop, and probing continues and the number of resource
> > described are the number of TSC find are used.
> > 
> > After the change failing to find all possible TCS will fail the whole
> > probe process, even if some TCS where described. And not describing max
> > number of TCS on each system is perfectly fine.
> > 
> > Nacked-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > 
> > > ---
> > >   drivers/thermal/rcar_gen3_thermal.c | 7 +------
> > >   1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > > index 9029d01e029b..5c623f13d9ec 100644
> > > --- a/drivers/thermal/rcar_gen3_thermal.c
> > > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > > @@ -481,7 +481,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> > >   {
> > >   	struct rcar_gen3_thermal_priv *priv;
> > >   	struct device *dev = &pdev->dev;
> > > -	struct resource *res;
> > >   	struct thermal_zone_device *zone;
> > >   	unsigned int i;
> > >   	int ret;
> > > @@ -504,17 +503,13 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> > >   	for (i = 0; i < TSC_MAX_NUM; i++) {
> > >   		struct rcar_gen3_thermal_tsc *tsc;
> > > -		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > > -		if (!res)
> > > -			break;
> > > -
> > >   		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> > >   		if (!tsc) {
> > >   			ret = -ENOMEM;
> > >   			goto error_unregister;
> > >   		}
> > > -		tsc->base = devm_ioremap_resource(dev, res);
> > > +		tsc->base = devm_platform_ioremap_resource(pdev, i);
> > >   		if (IS_ERR(tsc->base)) {
> > >   			ret = PTR_ERR(tsc->base);
> > >   			goto error_unregister;
> > > -- 
> > > 2.39.0
> > >
diff mbox series

Patch

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 9029d01e029b..5c623f13d9ec 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -481,7 +481,6 @@  static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 {
 	struct rcar_gen3_thermal_priv *priv;
 	struct device *dev = &pdev->dev;
-	struct resource *res;
 	struct thermal_zone_device *zone;
 	unsigned int i;
 	int ret;
@@ -504,17 +503,13 @@  static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	for (i = 0; i < TSC_MAX_NUM; i++) {
 		struct rcar_gen3_thermal_tsc *tsc;
 
-		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
-		if (!res)
-			break;
-
 		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
 		if (!tsc) {
 			ret = -ENOMEM;
 			goto error_unregister;
 		}
 
-		tsc->base = devm_ioremap_resource(dev, res);
+		tsc->base = devm_platform_ioremap_resource(pdev, i);
 		if (IS_ERR(tsc->base)) {
 			ret = PTR_ERR(tsc->base);
 			goto error_unregister;