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

[Yangtao Li]

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@xxxxxxxx>
> 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@xxxxxxxxxxxx>
>
> > ---
> >   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