diff mbox

[1/2] thermal: rcar: tidyup registration failure case

Message ID 874ng0qb9f.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State Superseded, archived
Delegated to: Zhang Rui
Headers show

Commit Message

Kuninori Morimoto March 25, 2013, 5:49 a.m. UTC
Current rcar_thermal driver didn't care about rcar_theraml_irq_disable()
when registration failure case on _probe(), and _remove().
And, it returns without unregistering thermal zone when
registration failure case on _probe().
This patch fixes these issue.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/thermal/rcar_thermal.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Zhang Rui March 25, 2013, 8:40 a.m. UTC | #1
On Sun, 2013-03-24 at 22:49 -0700, Kuninori Morimoto wrote:
> Current rcar_thermal driver didn't care about rcar_theraml_irq_disable()
> when registration failure case on _probe(), and _remove().
> And, it returns without unregistering thermal zone when
> registration failure case on _probe().
> This patch fixes these issue.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/thermal/rcar_thermal.c |   20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 28f0919..359e943 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -419,13 +419,15 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>  		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  		if (!priv) {
>  			dev_err(dev, "Could not allocate priv\n");
> -			return -ENOMEM;
> +			ret = -ENOMEM;
> +			goto error_unregister;
>  		}
>  
>  		priv->base = devm_request_and_ioremap(dev, res);
>  		if (!priv->base) {
>  			dev_err(dev, "Unable to ioremap priv register\n");
> -			return -ENOMEM;
> +			ret = -ENOMEM;
> +			goto error_unregister;
>  		}
>  
>  		priv->common = common;
> @@ -444,10 +446,10 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>  			goto error_unregister;
>  		}
>  
> -		list_move_tail(&priv->list, &common->head);
> -
>  		if (rcar_has_irq_support(priv))
>  			rcar_thermal_irq_enable(priv);
> +
> +		list_move_tail(&priv->list, &common->head);
>  	}
>  
why do you need this change for the rcar_thermal_irq_disable issue?

thanks,
rui
>  	platform_set_drvdata(pdev, common);
> @@ -457,8 +459,11 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>  	return 0;
>  
>  error_unregister:
> -	rcar_thermal_for_each_priv(priv, common)
> +	rcar_thermal_for_each_priv(priv, common) {
>  		thermal_zone_device_unregister(priv->zone);
> +		if (rcar_has_irq_support(priv))
> +			rcar_thermal_irq_disable(priv);
> +	}
>  
>  	return -ENODEV;
>  }
> @@ -468,8 +473,11 @@ static int rcar_thermal_remove(struct platform_device *pdev)
>  	struct rcar_thermal_common *common = platform_get_drvdata(pdev);
>  	struct rcar_thermal_priv *priv;
>  
> -	rcar_thermal_for_each_priv(priv, common)
> +	rcar_thermal_for_each_priv(priv, common) {
>  		thermal_zone_device_unregister(priv->zone);
> +		if (rcar_has_irq_support(priv))
> +			rcar_thermal_irq_disable(priv);
> +	}
>  
>  	platform_set_drvdata(pdev, NULL);
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kuninori Morimoto March 26, 2013, 12:16 a.m. UTC | #2
Hi Zhang

Thank you for checking patch

> > @@ -444,10 +446,10 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> >  			goto error_unregister;
> >  		}
> >  
> > -		list_move_tail(&priv->list, &common->head);
> > -
> >  		if (rcar_has_irq_support(priv))
> >  			rcar_thermal_irq_enable(priv);
> > +
> > +		list_move_tail(&priv->list, &common->head);
> >  	}
> >  
> why do you need this change for the rcar_thermal_irq_disable issue?

I thought it is readable (or easy to understand)
if it was added to the list after all the processings finish. 

listed priv's irq was enabled -> listed priv's irq should be disabled

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kuninori Morimoto March 26, 2013, 5:53 a.m. UTC | #3
Hi Zhang, again

> Thank you for checking patch
> 
> > > @@ -444,10 +446,10 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> > >  			goto error_unregister;
> > >  		}
> > >  
> > > -		list_move_tail(&priv->list, &common->head);
> > > -
> > >  		if (rcar_has_irq_support(priv))
> > >  			rcar_thermal_irq_enable(priv);
> > > +
> > > +		list_move_tail(&priv->list, &common->head);
> > >  	}
> > >  
> > why do you need this change for the rcar_thermal_irq_disable issue?
> 
> I thought it is readable (or easy to understand)
> if it was added to the list after all the processings finish. 
> 
> listed priv's irq was enabled -> listed priv's irq should be disabled

I noticed that this patch set had 2 bugs
Please give me 2nd chance

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kuninori Morimoto March 26, 2013, 6:07 a.m. UTC | #4
Hi Zhang

These are v2 patch set of rcar_thermal driver fixup
I noticed that v1 patch had bug.

These patch set are for linus/master branch,
and, these requires zhang/next's

f0e68fc3caf677e834f7bd0f601800e686b56c98
(thermal: rcar: fix missing unlock on error in rcar_thermal_update_temp())

fb84d9907f0ff0e3f7d70d55039ddf0f78d2a472
(thermal: rcar_thermal: propagate return value of thermal_zone_device_register)

current linus/master branch needs above 2 patches,
and these patch set are based on these.

I Cc:ed SH-ARM ML

Kuninori Morimoto (2):
      thermal: rcar: tidyup registration failure case
      thermal: rcar: add pm_runtime_xxx() support

 drivers/thermal/rcar_thermal.c |   31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui March 26, 2013, 6:30 a.m. UTC | #5
On Mon, 2013-03-25 at 23:07 -0700, Kuninori Morimoto wrote:
> Hi Zhang
> 
> These are v2 patch set of rcar_thermal driver fixup
> I noticed that v1 patch had bug.
> 
it seems that you did not run any test for the previous patch set, not
even build test.

have you tried this patch set before sending out?

thanks,
rui
> These patch set are for linus/master branch,
> and, these requires zhang/next's
> 
> f0e68fc3caf677e834f7bd0f601800e686b56c98
> (thermal: rcar: fix missing unlock on error in rcar_thermal_update_temp())
> 
> fb84d9907f0ff0e3f7d70d55039ddf0f78d2a472
> (thermal: rcar_thermal: propagate return value of thermal_zone_device_register)
> 
> current linus/master branch needs above 2 patches,
> and these patch set are based on these.
> 
> I Cc:ed SH-ARM ML
> 
> Kuninori Morimoto (2):
>       thermal: rcar: tidyup registration failure case
>       thermal: rcar: add pm_runtime_xxx() support
> 
>  drivers/thermal/rcar_thermal.c |   31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> Best regards
> ---
> Kuninori Morimoto
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kuninori Morimoto March 26, 2013, 6:45 a.m. UTC | #6
Hi Zhang

> On Mon, 2013-03-25 at 23:07 -0700, Kuninori Morimoto wrote:
> > Hi Zhang
> > 
> > These are v2 patch set of rcar_thermal driver fixup
> > I noticed that v1 patch had bug.
> > 
> it seems that you did not run any test for the previous patch set, not
> even build test.

Sorry about my v1 patch set.

My v1 (v2 too) based on not only your branch (= your branch + SH-ML patch set)
Of course I tested v1 patch set,

But I missed when I git-format-patch.
I'm sorry about it again

> have you tried this patch set before sending out?

Yes, these are tesed on r8a73a4 APE6 board


Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 28f0919..359e943 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -419,13 +419,15 @@  static int rcar_thermal_probe(struct platform_device *pdev)
 		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 		if (!priv) {
 			dev_err(dev, "Could not allocate priv\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error_unregister;
 		}
 
 		priv->base = devm_request_and_ioremap(dev, res);
 		if (!priv->base) {
 			dev_err(dev, "Unable to ioremap priv register\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error_unregister;
 		}
 
 		priv->common = common;
@@ -444,10 +446,10 @@  static int rcar_thermal_probe(struct platform_device *pdev)
 			goto error_unregister;
 		}
 
-		list_move_tail(&priv->list, &common->head);
-
 		if (rcar_has_irq_support(priv))
 			rcar_thermal_irq_enable(priv);
+
+		list_move_tail(&priv->list, &common->head);
 	}
 
 	platform_set_drvdata(pdev, common);
@@ -457,8 +459,11 @@  static int rcar_thermal_probe(struct platform_device *pdev)
 	return 0;
 
 error_unregister:
-	rcar_thermal_for_each_priv(priv, common)
+	rcar_thermal_for_each_priv(priv, common) {
 		thermal_zone_device_unregister(priv->zone);
+		if (rcar_has_irq_support(priv))
+			rcar_thermal_irq_disable(priv);
+	}
 
 	return -ENODEV;
 }
@@ -468,8 +473,11 @@  static int rcar_thermal_remove(struct platform_device *pdev)
 	struct rcar_thermal_common *common = platform_get_drvdata(pdev);
 	struct rcar_thermal_priv *priv;
 
-	rcar_thermal_for_each_priv(priv, common)
+	rcar_thermal_for_each_priv(priv, common) {
 		thermal_zone_device_unregister(priv->zone);
+		if (rcar_has_irq_support(priv))
+			rcar_thermal_irq_disable(priv);
+	}
 
 	platform_set_drvdata(pdev, NULL);