Re: [PATCH 1/2] rtc: rtc-twl: Use devm_*() functions

[Grygorii Strashko]

On 12/06/2013 03:49 AM, Jingoo Han wrote:

On Friday, December 06, 2013 1:51 AM, Grygorii Strashko wrote:

On 12/05/2013 03:03 AM, Jingoo Han wrote:

Use devm_*() functions to make cleanup paths simpler, and remove
unnecessary remove().

Signed-off-by: Jingoo Han <jg1....@samsung.com>
---
   drivers/rtc/rtc-twl.c |   38 +++++++++++++-------------------------
   1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c
index c2e80d7..1915464 100644
--- a/drivers/rtc/rtc-twl.c
+++ b/drivers/rtc/rtc-twl.c
@@ -479,7 +479,7 @@ static int twl_rtc_probe(struct platform_device *pdev)
        u8 rd_reg;

        if (irq <= 0)
-               goto out1;
+               return ret;

        /* Initialize the register map */
        if (twl_class_is_4030())
@@ -489,7 +489,7 @@ static int twl_rtc_probe(struct platform_device *pdev)

        ret = twl_rtc_read_u8(&rd_reg, REG_RTC_STATUS_REG);
        if (ret < 0)
-               goto out1;
+               return ret;

        if (rd_reg & BIT_RTC_STATUS_REG_POWER_UP_M)
                dev_warn(&pdev->dev, "Power up reset detected.\n");
@@ -500,7 +500,7 @@ static int twl_rtc_probe(struct platform_device *pdev)
        /* Clear RTC Power up reset and pending alarm interrupts */
        ret = twl_rtc_write_u8(rd_reg, REG_RTC_STATUS_REG);
        if (ret < 0)
-               goto out1;
+               return ret;

        if (twl_class_is_6030()) {
                twl6030_interrupt_unmask(TWL6030_RTC_INT_MASK,
@@ -512,7 +512,7 @@ static int twl_rtc_probe(struct platform_device *pdev)
        dev_info(&pdev->dev, "Enabling TWL-RTC\n");
        ret = twl_rtc_write_u8(BIT_RTC_CTRL_REG_STOP_RTC_M, REG_RTC_CTRL_REG);
        if (ret < 0)
-               goto out1;
+               return ret;

        /* ensure interrupts are disabled, bootloaders can be strange */
        ret = twl_rtc_write_u8(0, REG_RTC_INTERRUPTS_REG);
@@ -522,34 +522,29 @@ static int twl_rtc_probe(struct platform_device *pdev)
        /* init cached IRQ enable bits */
        ret = twl_rtc_read_u8(&rtc_irq_bits, REG_RTC_INTERRUPTS_REG);
        if (ret < 0)
-               goto out1;
+               return ret;

        device_init_wakeup(&pdev->dev, 1);

-       rtc = rtc_device_register(pdev->name,
-                                 &pdev->dev, &twl_rtc_ops, THIS_MODULE);
+       rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
+                                       &twl_rtc_ops, THIS_MODULE);
        if (IS_ERR(rtc)) {
-               ret = PTR_ERR(rtc);
                dev_err(&pdev->dev, "can't register RTC device, err %ld\n",
                        PTR_ERR(rtc));
-               goto out1;
+               return PTR_ERR(rtc);
        }

-       ret = request_threaded_irq(irq, NULL, twl_rtc_interrupt,
-                                  IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-                                  dev_name(&rtc->dev), rtc);
+       ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+                                       twl_rtc_interrupt,
+                                       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+                                       dev_name(&rtc->dev), rtc);

Looks like this will change driver release order and free_irq
will be called after RTC device is unregistered.
I think, you need to request irq first -> then register RTC device.
Right?

Hi Grygorii Strashko,

No, free_irq() will be called before RTC device is unregistered.

In probe(), the following functions are called as below:
   1. devm_rtc_device_register() -> rtc_device_register()
   2. devm_request_threaded_irq() -> request_threaded_irq()

The release functions will be inversely called.
When removing rtc-twl.
   1. devm_irq_release() -> free_irq()
   2. devm_rtc_device_release() -> rtc_device_unregister()

Thus, it is safe.
Thank you for your feedback. :-)

You are right. Sorry, my mistake :)

Regards,
-grygorii

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/