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.han@xxxxxxxxxxx>
> > > ---
> > >    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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/