| Message ID | 1354155670-6267-1-git-send-email-chao.xie@marvell.com |
|---|---|
| State | Rejected |
| Headers | show |
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.
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().
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.
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.
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/
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.
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?
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.
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.
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.
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().
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);
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(-)