Patchwork [1/4] rtc: sa1100: enable/disable rtc when probe/remove the device

login
register
mail settings
Submitter Chao Xie
Date Nov. 29, 2012, 2:21 a.m.
Message ID <1354155670-6267-1-git-send-email-chao.xie@marvell.com>
Download mbox | patch
Permalink /patch/202644/
State New
Headers show

Comments

Chao Xie - Nov. 29, 2012, 2:21 a.m.
The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/rtc/rtc-sa1100.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Russell King - ARM Linux - Nov. 29, 2012, 10:25 a.m.
On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
> The original sa1100_rtc_open/sa1100_rtc_release will be called
> when the /dev/rtc0 is opened or closed.
> In fact, these two functions will enable/disable the clock, and
> register/unregister the irqs.
> User application will use /dev/rtc0 to read the rtc time or set
> the alarm. The rtc should still run indepent of open/close the
> rtc device.
> So only enable clock and register the irqs when probe the device,
> and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.
Haojian Zhuang - Nov. 30, 2012, 7:04 a.m.
On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
>> The original sa1100_rtc_open/sa1100_rtc_release will be called
>> when the /dev/rtc0 is opened or closed.
>> In fact, these two functions will enable/disable the clock, and
>> register/unregister the irqs.
>> User application will use /dev/rtc0 to read the rtc time or set
>> the alarm. The rtc should still run indepent of open/close the
>> rtc device.
>> So only enable clock and register the irqs when probe the device,
>> and disable clock and unregister the irqs when remove the device.
>
> NAK.  I don't think you properly understand what's going on here if you
> think moving the entire open and release functions into the probe and
> remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().
Chao Xie - Dec. 3, 2012, 1:39 a.m.
On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
>>> The original sa1100_rtc_open/sa1100_rtc_release will be called
>>> when the /dev/rtc0 is opened or closed.
>>> In fact, these two functions will enable/disable the clock, and
>>> register/unregister the irqs.
>>> User application will use /dev/rtc0 to read the rtc time or set
>>> the alarm. The rtc should still run indepent of open/close the
>>> rtc device.
>>> So only enable clock and register the irqs when probe the device,
>>> and disable clock and unregister the irqs when remove the device.
>>
>> NAK.  I don't think you properly understand what's going on here if you
>> think moving the entire open and release functions into the probe and
>> remove functions is the right thing to do.
>
> Since PXA27x & PXA3xx supports dual rtc device at the same time,
> user could choose use either of rtc at run time. Then clk & irq are setup
> in open().
>
> Chao,
> So you shouldn't remove them into probe().

How can user to choose one RTC?
The user API, for example "date" will open the device, get the time
and then close the device.
WHen set a alarm, it is the same routine. open->set->close.
The open routine will enable clock and register irq, close will
disable the clock and unregister irq. It means that if only open is
invoked, rtc begins to work, and after close is invoked rtc stops
working. It means that the user can not get correct time and can not
get right alarm.
Chao Xie - Dec. 3, 2012, 2:53 a.m.
On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
>>>> The original sa1100_rtc_open/sa1100_rtc_release will be called
>>>> when the /dev/rtc0 is opened or closed.
>>>> In fact, these two functions will enable/disable the clock, and
>>>> register/unregister the irqs.
>>>> User application will use /dev/rtc0 to read the rtc time or set
>>>> the alarm. The rtc should still run indepent of open/close the
>>>> rtc device.
>>>> So only enable clock and register the irqs when probe the device,
>>>> and disable clock and unregister the irqs when remove the device.
>>>
>>> NAK.  I don't think you properly understand what's going on here if you
>>> think moving the entire open and release functions into the probe and
>>> remove functions is the right thing to do.
>>
>> Since PXA27x & PXA3xx supports dual rtc device at the same time,
>> user could choose use either of rtc at run time. Then clk & irq are setup
>> in open().
>>
>> Chao,
>> So you shouldn't remove them into probe().
>
> How can user to choose one RTC?
> The user API, for example "date" will open the device, get the time
> and then close the device.
> WHen set a alarm, it is the same routine. open->set->close.
> The open routine will enable clock and register irq, close will
> disable the clock and unregister irq. It means that if only open is
> invoked, rtc begins to work, and after close is invoked rtc stops
> working. It means that the user can not get correct time and can not
> get right alarm.

hi
I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.
Devendra Naga - Dec. 3, 2012, 5:02 a.m.
CCing Andrew as he is the maintainer of the rtc subsystem

On Wed, Nov 28, 2012 at 9:21 PM, Chao Xie <chao.xie@marvell.com> wrote:
> The original sa1100_rtc_open/sa1100_rtc_release will be called
> when the /dev/rtc0 is opened or closed.
> In fact, these two functions will enable/disable the clock, and
> register/unregister the irqs.
> User application will use /dev/rtc0 to read the rtc time or set
> the alarm. The rtc should still run indepent of open/close the
> rtc device.
> So only enable clock and register the irqs when probe the device,
> and disable clock and unregister the irqs when remove the device.
>
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  drivers/rtc/rtc-sa1100.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rtc/rtc-sa1100.c b/drivers/rtc/rtc-sa1100.c
> index 50a5c4a..e933327 100644
> --- a/drivers/rtc/rtc-sa1100.c
> +++ b/drivers/rtc/rtc-sa1100.c
> @@ -218,8 +218,6 @@ static int sa1100_rtc_proc(struct device *dev, struct seq_file *seq)
>  }
>
>  static const struct rtc_class_ops sa1100_rtc_ops = {
> -       .open = sa1100_rtc_open,
> -       .release = sa1100_rtc_release,
>         .read_time = sa1100_rtc_read_time,
>         .set_time = sa1100_rtc_set_time,
>         .read_alarm = sa1100_rtc_read_alarm,
> @@ -253,6 +251,10 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
>         spin_lock_init(&info->lock);
>         platform_set_drvdata(pdev, info);
>
> +       ret = sa1100_rtc_open(&pdev->dev);
> +       if (ret)
> +               goto err_open;
> +
>         /*
>          * According to the manual we should be able to let RTTR be zero
>          * and then a default diviser for a 32.768KHz clock is used.
> @@ -305,7 +307,9 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
>
>         return 0;
>  err_dev:
> +       sa1100_rtc_release(&pdev->dev);
>         platform_set_drvdata(pdev, NULL);
> +err_open:
>         clk_put(info->clk);
>  err_clk:
>         kfree(info);
> @@ -318,6 +322,7 @@ static int sa1100_rtc_remove(struct platform_device *pdev)
>
>         if (info) {
>                 rtc_device_unregister(info->rtc);
> +               sa1100_rtc_release(&pdev->dev);
>                 clk_put(info->clk);
>                 platform_set_drvdata(pdev, NULL);
>                 kfree(info);
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Haojian Zhuang - Dec. 3, 2012, 5:33 a.m.
On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
>>>> The original sa1100_rtc_open/sa1100_rtc_release will be called
>>>> when the /dev/rtc0 is opened or closed.
>>>> In fact, these two functions will enable/disable the clock, and
>>>> register/unregister the irqs.
>>>> User application will use /dev/rtc0 to read the rtc time or set
>>>> the alarm. The rtc should still run indepent of open/close the
>>>> rtc device.
>>>> So only enable clock and register the irqs when probe the device,
>>>> and disable clock and unregister the irqs when remove the device.
>>>
>>> NAK.  I don't think you properly understand what's going on here if you
>>> think moving the entire open and release functions into the probe and
>>> remove functions is the right thing to do.
>>
>> Since PXA27x & PXA3xx supports dual rtc device at the same time,
>> user could choose use either of rtc at run time. Then clk & irq are setup
>> in open().
>>
>> Chao,
>> So you shouldn't remove them into probe().
>
> How can user to choose one RTC?

/dev/rtc0 & /dev/rtc1.

The user can choose the rtc by iteself. Is it right?

> The user API, for example "date" will open the device, get the time
> and then close the device.

"date" command is always using /dev/rtc. It's the alias of rtc0 or rtc1.

> WHen set a alarm, it is the same routine. open->set->close.
> The open routine will enable clock and register irq, close will
> disable the clock and unregister irq. It means that if only open is
> invoked, rtc begins to work, and after close is invoked rtc stops
> working. It means that the user can not get correct time and can not
> get right alarm.
Haojian Zhuang - Dec. 3, 2012, 5:35 a.m.
On Mon, Dec 3, 2012 at 10:53 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
>> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
>> <haojian.zhuang@gmail.com> wrote:
>>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> wrote:
>>>> On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
>>>>> The original sa1100_rtc_open/sa1100_rtc_release will be called
>>>>> when the /dev/rtc0 is opened or closed.
>>>>> In fact, these two functions will enable/disable the clock, and
>>>>> register/unregister the irqs.
>>>>> User application will use /dev/rtc0 to read the rtc time or set
>>>>> the alarm. The rtc should still run indepent of open/close the
>>>>> rtc device.
>>>>> So only enable clock and register the irqs when probe the device,
>>>>> and disable clock and unregister the irqs when remove the device.
>>>>
>>>> NAK.  I don't think you properly understand what's going on here if you
>>>> think moving the entire open and release functions into the probe and
>>>> remove functions is the right thing to do.
>>>
>>> Since PXA27x & PXA3xx supports dual rtc device at the same time,
>>> user could choose use either of rtc at run time. Then clk & irq are setup
>>> in open().
>>>
>>> Chao,
>>> So you shouldn't remove them into probe().
>>
>> How can user to choose one RTC?
>> The user API, for example "date" will open the device, get the time
>> and then close the device.
>> WHen set a alarm, it is the same routine. open->set->close.
>> The open routine will enable clock and register irq, close will
>> disable the clock and unregister irq. It means that if only open is
>> invoked, rtc begins to work, and after close is invoked rtc stops
>> working. It means that the user can not get correct time and can not
>> get right alarm.
>
> hi
> I want to correct what i said. For the irq register/unregister i think
> can be done at open/release. But for clock enable/disable, i do not
> think so. If clock is disabled, as i think RTC will not work. User API
> still use open->get_time->close for "date" command. It means that RTC
> will not return correct date to user.

I didn't get your point. Do you mean that RTC can't work without your patch?
Chao Xie - Dec. 3, 2012, 5:42 a.m.
On Mon, Dec 3, 2012 at 1:35 PM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote:
> On Mon, Dec 3, 2012 at 10:53 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
>> On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
>>> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
>>> <haojian.zhuang@gmail.com> wrote:
>>>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>>> On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
>>>>>> The original sa1100_rtc_open/sa1100_rtc_release will be called
>>>>>> when the /dev/rtc0 is opened or closed.
>>>>>> In fact, these two functions will enable/disable the clock, and
>>>>>> register/unregister the irqs.
>>>>>> User application will use /dev/rtc0 to read the rtc time or set
>>>>>> the alarm. The rtc should still run indepent of open/close the
>>>>>> rtc device.
>>>>>> So only enable clock and register the irqs when probe the device,
>>>>>> and disable clock and unregister the irqs when remove the device.
>>>>>
>>>>> NAK.  I don't think you properly understand what's going on here if you
>>>>> think moving the entire open and release functions into the probe and
>>>>> remove functions is the right thing to do.
>>>>
>>>> Since PXA27x & PXA3xx supports dual rtc device at the same time,
>>>> user could choose use either of rtc at run time. Then clk & irq are setup
>>>> in open().
>>>>
>>>> Chao,
>>>> So you shouldn't remove them into probe().
>>>
>>> How can user to choose one RTC?
>>> The user API, for example "date" will open the device, get the time
>>> and then close the device.
>>> WHen set a alarm, it is the same routine. open->set->close.
>>> The open routine will enable clock and register irq, close will
>>> disable the clock and unregister irq. It means that if only open is
>>> invoked, rtc begins to work, and after close is invoked rtc stops
>>> working. It means that the user can not get correct time and can not
>>> get right alarm.
>>
>> hi
>> I want to correct what i said. For the irq register/unregister i think
>> can be done at open/release. But for clock enable/disable, i do not
>> think so. If clock is disabled, as i think RTC will not work. User API
>> still use open->get_time->close for "date" command. It means that RTC
>> will not return correct date to user.
>
> I didn't get your point. Do you mean that RTC can't work without your patch?
hi
open/release will done two things
1. enable/disable clock
2. register/unregister the irq
i checked the rtc dev code. register/unregister irq is prefered to be
done at open/release routine, and the users will keep rtc to be opened
if he want to make use of the interrupts.
the only problemis the clock. It should be on always.
If user want to get the rtc value, or set alarm. it will  follow
open->ioctrl->release. After release the clock is offed, and i do not
think rtc will has its value updated if the clock is offed.
Russell King - ARM Linux - Dec. 3, 2012, 9:48 a.m.
On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:
> I want to correct what i said. For the irq register/unregister i think
> can be done at open/release. But for clock enable/disable, i do not
> think so. If clock is disabled, as i think RTC will not work. User API
> still use open->get_time->close for "date" command. It means that RTC
> will not return correct date to user.

"I think" is not good enough for patches like this.  Please test it.

On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
clock; it has no effect.  For MMP, I don't have access to the TRMs so
that's something you're going to have to find out.
Chao Xie - Dec. 4, 2012, 2:51 a.m.
On Mon, Dec 3, 2012 at 5:48 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:
>> I want to correct what i said. For the irq register/unregister i think
>> can be done at open/release. But for clock enable/disable, i do not
>> think so. If clock is disabled, as i think RTC will not work. User API
>> still use open->get_time->close for "date" command. It means that RTC
>> will not return correct date to user.
>
> "I think" is not good enough for patches like this.  Please test it.
>
> On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
> clock; it has no effect.  For MMP, I don't have access to the TRMs so
> that's something you're going to have to find out.

I tested at pxa910 which uses rtc-sa1100 as driver.
the test procedure is simple
open->ioctl(RTC_RD_TIME)->close
With the original code, the rtc will not update, i always get the same value

remove clock disable/enable in release/open, and enable/disable clock
at probe/remove.
the rtc can update, and i can read the correct rtc value.
Haojian Zhuang - Dec. 4, 2012, 7:01 a.m.
On Tuesday, December 4, 2012, Chao Xie wrote:
>
> On Mon, Dec 3, 2012 at 5:48 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:
> >> I want to correct what i said. For the irq register/unregister i think
> >> can be done at open/release. But for clock enable/disable, i do not
> >> think so. If clock is disabled, as i think RTC will not work. User API
> >> still use open->get_time->close for "date" command. It means that RTC
> >> will not return correct date to user.
> >
> > "I think" is not good enough for patches like this.  Please test it.
> >
> > On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
> > clock; it has no effect.  For MMP, I don't have access to the TRMs so
> > that's something you're going to have to find out.
>
> I tested at pxa910 which uses rtc-sa1100 as driver.
> the test procedure is simple
> open->ioctl(RTC_RD_TIME)->close
> With the original code, the rtc will not update, i always get the same value
>
> remove clock disable/enable in release/open, and enable/disable clock
> at probe/remove.
> the rtc can update, and i can read the correct rtc value.

Yes, clock shouldn't be controlled in open/close. Otherwise, RTC can't
work since
clock is already gated. I didn't find the issue since there're two RTC
enabled in my
board. The default one impacts the test.

I think that you need send the patch that only move clock operation
into probe().

Patch

diff --git a/drivers/rtc/rtc-sa1100.c b/drivers/rtc/rtc-sa1100.c
index 50a5c4a..e933327 100644
--- a/drivers/rtc/rtc-sa1100.c
+++ b/drivers/rtc/rtc-sa1100.c
@@ -218,8 +218,6 @@  static int sa1100_rtc_proc(struct device *dev, struct seq_file *seq)
 }
 
 static const struct rtc_class_ops sa1100_rtc_ops = {
-	.open = sa1100_rtc_open,
-	.release = sa1100_rtc_release,
 	.read_time = sa1100_rtc_read_time,
 	.set_time = sa1100_rtc_set_time,
 	.read_alarm = sa1100_rtc_read_alarm,
@@ -253,6 +251,10 @@  static int sa1100_rtc_probe(struct platform_device *pdev)
 	spin_lock_init(&info->lock);
 	platform_set_drvdata(pdev, info);
 
+	ret = sa1100_rtc_open(&pdev->dev);
+	if (ret)
+		goto err_open;
+
 	/*
 	 * According to the manual we should be able to let RTTR be zero
 	 * and then a default diviser for a 32.768KHz clock is used.
@@ -305,7 +307,9 @@  static int sa1100_rtc_probe(struct platform_device *pdev)
 
 	return 0;
 err_dev:
+	sa1100_rtc_release(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
+err_open:
 	clk_put(info->clk);
 err_clk:
 	kfree(info);
@@ -318,6 +322,7 @@  static int sa1100_rtc_remove(struct platform_device *pdev)
 
 	if (info) {
 		rtc_device_unregister(info->rtc);
+		sa1100_rtc_release(&pdev->dev);
 		clk_put(info->clk);
 		platform_set_drvdata(pdev, NULL);
 		kfree(info);