[1/2] thermal: qoriq: Use devm_add_action_or_reset() to handle all cleanups
diff mbox series

Message ID 1583903252-2058-1-git-send-email-Anson.Huang@nxp.com
State New
Delegated to: Daniel Lezcano
Headers show
Series
  • [1/2] thermal: qoriq: Use devm_add_action_or_reset() to handle all cleanups
Related show

Commit Message

Anson Huang March 11, 2020, 5:07 a.m. UTC
Use devm_add_action_or_reset() to handle all cleanups of failure in
.probe and .remove, then .remove callback can be dropped.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/thermal/qoriq_thermal.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

Comments

Amit Kucheria March 11, 2020, 8:59 a.m. UTC | #1
On Wed, Mar 11, 2020 at 10:44 AM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> Use devm_add_action_or_reset() to handle all cleanups of failure in
> .probe and .remove, then .remove callback can be dropped.


Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>

> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/thermal/qoriq_thermal.c | 35 ++++++++++++++---------------------
>  1 file changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
> index 874bc46..67a8d84 100644
> --- a/drivers/thermal/qoriq_thermal.c
> +++ b/drivers/thermal/qoriq_thermal.c
> @@ -228,6 +228,14 @@ static const struct regmap_access_table qoriq_rd_table = {
>         .n_yes_ranges   = ARRAY_SIZE(qoriq_yes_ranges),
>  };
>
> +static void qoriq_tmu_action(void *p)
> +{
> +       struct qoriq_tmu_data *data = p;
> +
> +       regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
> +       clk_disable_unprepare(data->clk);
> +}
> +
>  static int qoriq_tmu_probe(struct platform_device *pdev)
>  {
>         int ret;
> @@ -278,6 +286,10 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       ret = devm_add_action_or_reset(dev, qoriq_tmu_action, data);
> +       if (ret)
> +               return ret;
> +
>         /* version register offset at: 0xbf8 on both v1 and v2 */
>         ret = regmap_read(data->regmap, REGS_IPBRR(0), &ver);
>         if (ret) {
> @@ -290,35 +302,17 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
>
>         ret = qoriq_tmu_calibration(dev, data); /* TMU calibration */
>         if (ret < 0)
> -               goto err;
> +               return ret;
>
>         ret = qoriq_tmu_register_tmu_zone(dev, data);
>         if (ret < 0) {
>                 dev_err(dev, "Failed to register sensors\n");
> -               ret = -ENODEV;
> -               goto err;
> +               return ret;
>         }
>
>         platform_set_drvdata(pdev, data);
>
>         return 0;
> -
> -err:
> -       clk_disable_unprepare(data->clk);
> -
> -       return ret;
> -}
> -
> -static int qoriq_tmu_remove(struct platform_device *pdev)
> -{
> -       struct qoriq_tmu_data *data = platform_get_drvdata(pdev);
> -
> -       /* Disable monitoring */
> -       regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
> -
> -       clk_disable_unprepare(data->clk);
> -
> -       return 0;
>  }
>
>  static int __maybe_unused qoriq_tmu_suspend(struct device *dev)
> @@ -365,7 +359,6 @@ static struct platform_driver qoriq_tmu = {
>                 .of_match_table = qoriq_tmu_match,
>         },
>         .probe  = qoriq_tmu_probe,
> -       .remove = qoriq_tmu_remove,
>  };
>  module_platform_driver(qoriq_tmu);
>
> --
> 2.7.4
>
Daniel Lezcano March 12, 2020, 11:24 a.m. UTC | #2
On 11/03/2020 06:07, Anson Huang wrote:
> Use devm_add_action_or_reset() to handle all cleanups of failure in
> .probe and .remove, then .remove callback can be dropped.

Is this change compatible with the tristate?

> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/thermal/qoriq_thermal.c | 35 ++++++++++++++---------------------
>  1 file changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
> index 874bc46..67a8d84 100644
> --- a/drivers/thermal/qoriq_thermal.c
> +++ b/drivers/thermal/qoriq_thermal.c
> @@ -228,6 +228,14 @@ static const struct regmap_access_table qoriq_rd_table = {
>  	.n_yes_ranges	= ARRAY_SIZE(qoriq_yes_ranges),
>  };
>  
> +static void qoriq_tmu_action(void *p)
> +{
> +	struct qoriq_tmu_data *data = p;
> +
> +	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
> +	clk_disable_unprepare(data->clk);
> +}
> +
>  static int qoriq_tmu_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -278,6 +286,10 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = devm_add_action_or_reset(dev, qoriq_tmu_action, data);
> +	if (ret)
> +		return ret;
> +
>  	/* version register offset at: 0xbf8 on both v1 and v2 */
>  	ret = regmap_read(data->regmap, REGS_IPBRR(0), &ver);
>  	if (ret) {
> @@ -290,35 +302,17 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
>  
>  	ret = qoriq_tmu_calibration(dev, data);	/* TMU calibration */
>  	if (ret < 0)
> -		goto err;
> +		return ret;
>  
>  	ret = qoriq_tmu_register_tmu_zone(dev, data);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to register sensors\n");
> -		ret = -ENODEV;
> -		goto err;
> +		return ret;
>  	}
>  
>  	platform_set_drvdata(pdev, data);
>  
>  	return 0;
> -
> -err:
> -	clk_disable_unprepare(data->clk);
> -
> -	return ret;
> -}
> -
> -static int qoriq_tmu_remove(struct platform_device *pdev)
> -{
> -	struct qoriq_tmu_data *data = platform_get_drvdata(pdev);
> -
> -	/* Disable monitoring */
> -	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
> -
> -	clk_disable_unprepare(data->clk);
> -
> -	return 0;
>  }
>  
>  static int __maybe_unused qoriq_tmu_suspend(struct device *dev)
> @@ -365,7 +359,6 @@ static struct platform_driver qoriq_tmu = {
>  		.of_match_table	= qoriq_tmu_match,
>  	},
>  	.probe	= qoriq_tmu_probe,
> -	.remove	= qoriq_tmu_remove,
>  };
>  module_platform_driver(qoriq_tmu);
>  
>
Anson Huang March 12, 2020, 11:47 a.m. UTC | #3
Hi, Daniel

> Subject: Re: [PATCH 1/2] thermal: qoriq: Use devm_add_action_or_reset() to
> handle all cleanups
> 
> On 11/03/2020 06:07, Anson Huang wrote:
> > Use devm_add_action_or_reset() to handle all cleanups of failure in
> > .probe and .remove, then .remove callback can be dropped.
> 
> Is this change compatible with the tristate?

I think so, any concern need me to double confirm?

Thanks,
Anson

> 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/thermal/qoriq_thermal.c | 35
> > ++++++++++++++---------------------
> >  1 file changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/thermal/qoriq_thermal.c
> > b/drivers/thermal/qoriq_thermal.c index 874bc46..67a8d84 100644
> > --- a/drivers/thermal/qoriq_thermal.c
> > +++ b/drivers/thermal/qoriq_thermal.c
> > @@ -228,6 +228,14 @@ static const struct regmap_access_table
> qoriq_rd_table = {
> >  	.n_yes_ranges	= ARRAY_SIZE(qoriq_yes_ranges),
> >  };
> >
> > +static void qoriq_tmu_action(void *p) {
> > +	struct qoriq_tmu_data *data = p;
> > +
> > +	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
> > +	clk_disable_unprepare(data->clk);
> > +}
> > +
> >  static int qoriq_tmu_probe(struct platform_device *pdev)  {
> >  	int ret;
> > @@ -278,6 +286,10 @@ static int qoriq_tmu_probe(struct platform_device
> *pdev)
> >  		return ret;
> >  	}
> >
> > +	ret = devm_add_action_or_reset(dev, qoriq_tmu_action, data);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* version register offset at: 0xbf8 on both v1 and v2 */
> >  	ret = regmap_read(data->regmap, REGS_IPBRR(0), &ver);
> >  	if (ret) {
> > @@ -290,35 +302,17 @@ static int qoriq_tmu_probe(struct
> > platform_device *pdev)
> >
> >  	ret = qoriq_tmu_calibration(dev, data);	/* TMU calibration */
> >  	if (ret < 0)
> > -		goto err;
> > +		return ret;
> >
> >  	ret = qoriq_tmu_register_tmu_zone(dev, data);
> >  	if (ret < 0) {
> >  		dev_err(dev, "Failed to register sensors\n");
> > -		ret = -ENODEV;
> > -		goto err;
> > +		return ret;
> >  	}
> >
> >  	platform_set_drvdata(pdev, data);
> >
> >  	return 0;
> > -
> > -err:
> > -	clk_disable_unprepare(data->clk);
> > -
> > -	return ret;
> > -}
> > -
> > -static int qoriq_tmu_remove(struct platform_device *pdev) -{
> > -	struct qoriq_tmu_data *data = platform_get_drvdata(pdev);
> > -
> > -	/* Disable monitoring */
> > -	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
> > -
> > -	clk_disable_unprepare(data->clk);
> > -
> > -	return 0;
> >  }
> >
> >  static int __maybe_unused qoriq_tmu_suspend(struct device *dev) @@
> > -365,7 +359,6 @@ static struct platform_driver qoriq_tmu = {
> >  		.of_match_table	= qoriq_tmu_match,
> >  	},
> >  	.probe	= qoriq_tmu_probe,
> > -	.remove	= qoriq_tmu_remove,
> >  };
> >  module_platform_driver(qoriq_tmu);
> >
> >
> 
> 
> --
> 
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7C37ea3
> 145642b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637196090558448813&amp;sdata=%2FCfH8GPxd57CDlau1pMcq
> LH7GjGIh%2Fu%2Bfq7teGsS8KM%3D&amp;reserved=0> Linaro.org │ Open
> source software for ARM SoCs
> 
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7CAnson.Huang%4
> 0nxp.com%7C37ea3145642b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637196090558448813&amp;sdata=KBUWpT
> 4quqo08r8YhbMVk%2Fyf2jIT1CKgc5i3jI9gCwo%3D&amp;reserved=0>
> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7CAnson.Huang%40nxp.c
> om%7C37ea3145642b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C637196090558448813&amp;sdata=%2FEgeRMWd
> qbbaOMl2Tfb%2BtLmfttqN1WcFbl9%2B5tXtOic%3D&amp;reserved=0>
> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-
> blog%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7C37ea314564
> 2b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637196090558448813&amp;sdata=SXZow%2F0B%2BIgpfOfAUIG1NsR
> mcHp868ve%2Ffkejg3ZDvE%3D&amp;reserved=0> Blog
Daniel Lezcano March 12, 2020, 11:56 a.m. UTC | #4
On 12/03/2020 12:47, Anson Huang wrote:
> Hi, Daniel
> 
>> Subject: Re: [PATCH 1/2] thermal: qoriq: Use devm_add_action_or_reset() to
>> handle all cleanups
>>
>> On 11/03/2020 06:07, Anson Huang wrote:
>>> Use devm_add_action_or_reset() to handle all cleanups of failure in
>>> .probe and .remove, then .remove callback can be dropped.
>>
>> Is this change compatible with the tristate?
> 
> I think so, any concern need me to double confirm?

TBH, I discovered the function with your patch. My concern is if the
callback is called when unloading the module.


>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>>> ---
>>>  drivers/thermal/qoriq_thermal.c | 35
>>> ++++++++++++++---------------------
>>>  1 file changed, 14 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/thermal/qoriq_thermal.c
>>> b/drivers/thermal/qoriq_thermal.c index 874bc46..67a8d84 100644
>>> --- a/drivers/thermal/qoriq_thermal.c
>>> +++ b/drivers/thermal/qoriq_thermal.c
>>> @@ -228,6 +228,14 @@ static const struct regmap_access_table
>> qoriq_rd_table = {
>>>  	.n_yes_ranges	= ARRAY_SIZE(qoriq_yes_ranges),
>>>  };
>>>
>>> +static void qoriq_tmu_action(void *p) {
>>> +	struct qoriq_tmu_data *data = p;
>>> +
>>> +	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
>>> +	clk_disable_unprepare(data->clk);
>>> +}
>>> +
>>>  static int qoriq_tmu_probe(struct platform_device *pdev)  {
>>>  	int ret;
>>> @@ -278,6 +286,10 @@ static int qoriq_tmu_probe(struct platform_device
>> *pdev)
>>>  		return ret;
>>>  	}
>>>
>>> +	ret = devm_add_action_or_reset(dev, qoriq_tmu_action, data);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>  	/* version register offset at: 0xbf8 on both v1 and v2 */
>>>  	ret = regmap_read(data->regmap, REGS_IPBRR(0), &ver);
>>>  	if (ret) {
>>> @@ -290,35 +302,17 @@ static int qoriq_tmu_probe(struct
>>> platform_device *pdev)
>>>
>>>  	ret = qoriq_tmu_calibration(dev, data);	/* TMU calibration */
>>>  	if (ret < 0)
>>> -		goto err;
>>> +		return ret;
>>>
>>>  	ret = qoriq_tmu_register_tmu_zone(dev, data);
>>>  	if (ret < 0) {
>>>  		dev_err(dev, "Failed to register sensors\n");
>>> -		ret = -ENODEV;
>>> -		goto err;
>>> +		return ret;
>>>  	}
>>>
>>>  	platform_set_drvdata(pdev, data);
>>>
>>>  	return 0;
>>> -
>>> -err:
>>> -	clk_disable_unprepare(data->clk);
>>> -
>>> -	return ret;
>>> -}
>>> -
>>> -static int qoriq_tmu_remove(struct platform_device *pdev) -{
>>> -	struct qoriq_tmu_data *data = platform_get_drvdata(pdev);
>>> -
>>> -	/* Disable monitoring */
>>> -	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
>>> -
>>> -	clk_disable_unprepare(data->clk);
>>> -
>>> -	return 0;
>>>  }
>>>
>>>  static int __maybe_unused qoriq_tmu_suspend(struct device *dev) @@
>>> -365,7 +359,6 @@ static struct platform_driver qoriq_tmu = {
>>>  		.of_match_table	= qoriq_tmu_match,
>>>  	},
>>>  	.probe	= qoriq_tmu_probe,
>>> -	.remove	= qoriq_tmu_remove,
>>>  };
>>>  module_platform_driver(qoriq_tmu);
>>>
>>>
>>
>>
>> --
>>
>> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
>> linaro.org%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7C37ea3
>> 145642b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
>> C0%7C0%7C637196090558448813&amp;sdata=%2FCfH8GPxd57CDlau1pMcq
>> LH7GjGIh%2Fu%2Bfq7teGsS8KM%3D&amp;reserved=0> Linaro.org │ Open
>> source software for ARM SoCs
>>
>> Follow Linaro:
>> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
>> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7CAnson.Huang%4
>> 0nxp.com%7C37ea3145642b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa
>> 92cd99c5c301635%7C0%7C0%7C637196090558448813&amp;sdata=KBUWpT
>> 4quqo08r8YhbMVk%2Fyf2jIT1CKgc5i3jI9gCwo%3D&amp;reserved=0>
>> Facebook |
>> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
>> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7CAnson.Huang%40nxp.c
>> om%7C37ea3145642b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa92cd9
>> 9c5c301635%7C0%7C0%7C637196090558448813&amp;sdata=%2FEgeRMWd
>> qbbaOMl2Tfb%2BtLmfttqN1WcFbl9%2B5tXtOic%3D&amp;reserved=0>
>> Twitter |
>> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
>> linaro.org%2Flinaro-
>> blog%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7C37ea314564
>> 2b47b4df7208d7c677e57d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>> C0%7C637196090558448813&amp;sdata=SXZow%2F0B%2BIgpfOfAUIG1NsR
>> mcHp868ve%2Ffkejg3ZDvE%3D&amp;reserved=0> Blog
>
Anson Huang March 12, 2020, 12:19 p.m. UTC | #5
Hi, Daniel

> Subject: Re: [PATCH 1/2] thermal: qoriq: Use devm_add_action_or_reset() to
> handle all cleanups
> 
> On 12/03/2020 12:47, Anson Huang wrote:
> > Hi, Daniel
> >
> >> Subject: Re: [PATCH 1/2] thermal: qoriq: Use
> >> devm_add_action_or_reset() to handle all cleanups
> >>
> >> On 11/03/2020 06:07, Anson Huang wrote:
> >>> Use devm_add_action_or_reset() to handle all cleanups of failure in
> >>> .probe and .remove, then .remove callback can be dropped.
> >>
> >> Is this change compatible with the tristate?
> >
> > I think so, any concern need me to double confirm?
> 
> TBH, I discovered the function with your patch. My concern is if the callback
> is called when unloading the module.

I think so as per my memory, see similar patches as below:


commit 19ec11a2233d24a7811836fa735203aaccf95a23
Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date:   Thu Jul 11 10:29:35 2019 +0200

    gpio: em: remove the gpiochip before removing the irq domain

    In commit 8764c4ca5049 ("gpio: em: use the managed version of
    gpiochip_add_data()") we implicitly altered the ordering of resource
    freeing: since gpiochip_remove() calls gpiochip_irqchip_remove()
    internally, we now can potentially use the irq_domain after it was
    destroyed in the remove() callback (as devm resources are freed after
    remove() has returned).

    Use devm_add_action_or_reset() to keep the ordering right and entirely
    kill the remove() callback in the driver.


commit d9aa5ca429ad30dde96e5966173d18004f16f312
Author: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date:   Fri Apr 19 10:25:01 2019 +0200

    rtc: ds2404: simplify .probe and remove .remove

    Use devm_add_action_or_reset to simplify .probe and remove .remove

    Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

drivers/rtc/rtc-ds2404.c


Thanks,
Anson
Anson Huang March 17, 2020, 1:14 a.m. UTC | #6
Hi, Daniel

> Subject: RE: [PATCH 1/2] thermal: qoriq: Use devm_add_action_or_reset() to
> handle all cleanups
> 
> Hi, Daniel
> 
> > Subject: Re: [PATCH 1/2] thermal: qoriq: Use
> > devm_add_action_or_reset() to handle all cleanups
> >
> > On 12/03/2020 12:47, Anson Huang wrote:
> > > Hi, Daniel
> > >
> > >> Subject: Re: [PATCH 1/2] thermal: qoriq: Use
> > >> devm_add_action_or_reset() to handle all cleanups
> > >>
> > >> On 11/03/2020 06:07, Anson Huang wrote:
> > >>> Use devm_add_action_or_reset() to handle all cleanups of failure
> > >>> in .probe and .remove, then .remove callback can be dropped.
> > >>
> > >> Is this change compatible with the tristate?
> > >
> > > I think so, any concern need me to double confirm?
> >
> > TBH, I discovered the function with your patch. My concern is if the
> > callback is called when unloading the module.
> 
> I think so as per my memory, see similar patches as below:
> 
> 
> commit 19ec11a2233d24a7811836fa735203aaccf95a23
> Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Date:   Thu Jul 11 10:29:35 2019 +0200
> 
>     gpio: em: remove the gpiochip before removing the irq domain
> 
>     In commit 8764c4ca5049 ("gpio: em: use the managed version of
>     gpiochip_add_data()") we implicitly altered the ordering of resource
>     freeing: since gpiochip_remove() calls gpiochip_irqchip_remove()
>     internally, we now can potentially use the irq_domain after it was
>     destroyed in the remove() callback (as devm resources are freed after
>     remove() has returned).
> 
>     Use devm_add_action_or_reset() to keep the ordering right and entirely
>     kill the remove() callback in the driver.
> 
> 
> commit d9aa5ca429ad30dde96e5966173d18004f16f312
> Author: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Date:   Fri Apr 19 10:25:01 2019 +0200
> 
>     rtc: ds2404: simplify .probe and remove .remove
> 
>     Use devm_add_action_or_reset to simplify .probe and remove .remove
> 
>     Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> 
> drivers/rtc/rtc-ds2404.c

Any further concern?

Thanks,
Anson
Daniel Lezcano March 17, 2020, 9:07 a.m. UTC | #7
On 17/03/2020 02:14, Anson Huang wrote:
> Hi, Daniel
> 
>> Subject: RE: [PATCH 1/2] thermal: qoriq: Use devm_add_action_or_reset() to
>> handle all cleanups
>>

[ ... ]

>>>>> Is this change compatible with the tristate?
>>>>
>>>> I think so, any concern need me to double confirm?
>>>
>>> TBH, I discovered the function with your patch. My concern is if the
>>> callback is called when unloading the module.
>>
>> I think so as per my memory, see similar patches as below:
>>
>>
>> commit 19ec11a2233d24a7811836fa735203aaccf95a23
>> Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> Date:   Thu Jul 11 10:29:35 2019 +0200
>>
>>     gpio: em: remove the gpiochip before removing the irq domain
>>
>>     In commit 8764c4ca5049 ("gpio: em: use the managed version of
>>     gpiochip_add_data()") we implicitly altered the ordering of resource
>>     freeing: since gpiochip_remove() calls gpiochip_irqchip_remove()
>>     internally, we now can potentially use the irq_domain after it was
>>     destroyed in the remove() callback (as devm resources are freed after
>>     remove() has returned).
>>
>>     Use devm_add_action_or_reset() to keep the ordering right and entirely
>>     kill the remove() callback in the driver.
>>
>>
>> commit d9aa5ca429ad30dde96e5966173d18004f16f312
>> Author: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Date:   Fri Apr 19 10:25:01 2019 +0200
>>
>>     rtc: ds2404: simplify .probe and remove .remove
>>
>>     Use devm_add_action_or_reset to simplify .probe and remove .remove
>>
>>     Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>
>> drivers/rtc/rtc-ds2404.c
> 
> Any further concern?

No more concerns, I've applied the patches.

Thanks
  -- Daniel

Patch
diff mbox series

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index 874bc46..67a8d84 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -228,6 +228,14 @@  static const struct regmap_access_table qoriq_rd_table = {
 	.n_yes_ranges	= ARRAY_SIZE(qoriq_yes_ranges),
 };
 
+static void qoriq_tmu_action(void *p)
+{
+	struct qoriq_tmu_data *data = p;
+
+	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
+	clk_disable_unprepare(data->clk);
+}
+
 static int qoriq_tmu_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -278,6 +286,10 @@  static int qoriq_tmu_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, qoriq_tmu_action, data);
+	if (ret)
+		return ret;
+
 	/* version register offset at: 0xbf8 on both v1 and v2 */
 	ret = regmap_read(data->regmap, REGS_IPBRR(0), &ver);
 	if (ret) {
@@ -290,35 +302,17 @@  static int qoriq_tmu_probe(struct platform_device *pdev)
 
 	ret = qoriq_tmu_calibration(dev, data);	/* TMU calibration */
 	if (ret < 0)
-		goto err;
+		return ret;
 
 	ret = qoriq_tmu_register_tmu_zone(dev, data);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register sensors\n");
-		ret = -ENODEV;
-		goto err;
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, data);
 
 	return 0;
-
-err:
-	clk_disable_unprepare(data->clk);
-
-	return ret;
-}
-
-static int qoriq_tmu_remove(struct platform_device *pdev)
-{
-	struct qoriq_tmu_data *data = platform_get_drvdata(pdev);
-
-	/* Disable monitoring */
-	regmap_write(data->regmap, REGS_TMR, TMR_DISABLE);
-
-	clk_disable_unprepare(data->clk);
-
-	return 0;
 }
 
 static int __maybe_unused qoriq_tmu_suspend(struct device *dev)
@@ -365,7 +359,6 @@  static struct platform_driver qoriq_tmu = {
 		.of_match_table	= qoriq_tmu_match,
 	},
 	.probe	= qoriq_tmu_probe,
-	.remove	= qoriq_tmu_remove,
 };
 module_platform_driver(qoriq_tmu);