| Message ID | 1348225223-32156-1-git-send-email-vbyravarasu@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
On 09/21/2012 05:00 AM, Venu Byravarasu wrote: > As RTC driver needs only irq number from platform data, > using platform_get_irq(), instead of generic dev_get_platdata(). I assume this patch depends on "mfd: tps65910: Add alarm interrupt of TPS65910 RTC to mfd device list" which you posted just before? If so, the two patches should go through the same tree to avoid "git bisect" issues. Also, I thought you needed to fix the MFD driver to call mfd_add_devices() only after all the IRQ stuff had been set up - otherwise, when the RTC driver calls devm_request_threaded_irq(), the parent IRQ domain that the IRQ points at won't exist, and the call will fail. Also, the MFD patch you sent to create add the IRQ resource into the RTC device's resource list sets the IRQ number to TPS65910_IRQ_RTC_ALARM, which is the offset within the IRQ domain, not the absolute Linux IRQ number. Where does the base of the IRQ domain get added on such that this is an actual IRQ numbmer not an offset? > - dev_dbg(&pdev->dev, "Enabling rtc-tps65910.\n"); > + dev_dbg(&pdev->dev, "Enabling tps65910 rtc.\n"); Unrelated change. > - irq = pmic_plat_data->irq_base; > + irq = platform_get_irq(pdev, 0); There are two spaces there before the =. > - "rtc-tps65910", &pdev->dev); > + "tps65910-rtc", &pdev->dev); Unrelated change. > - .name = "rtc-tps65910", > + .name = "tps65910-rtc", Unrelated change.
> -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Friday, September 21, 2012 9:21 PM > To: Venu Byravarasu > Cc: akpm@linux-foundation.org; a.zummo@towertech.it; linux- > kernel@vger.kernel.org; rtc-linux@googlegroups.com > Subject: Re: [PATCH] rtc: tps65910: Use platform_get_irq to get RTC irq > details > > On 09/21/2012 05:00 AM, Venu Byravarasu wrote: > > As RTC driver needs only irq number from platform data, > > using platform_get_irq(), instead of generic dev_get_platdata(). > > I assume this patch depends on "mfd: tps65910: Add alarm interrupt of > TPS65910 RTC to mfd device list" which you posted just before? If so, > the two patches should go through the same tree to avoid "git bisect" > issues. > > Also, I thought you needed to fix the MFD driver to call > mfd_add_devices() only after all the IRQ stuff had been set up - > otherwise, when the RTC driver calls devm_request_threaded_irq(), the > parent IRQ domain that the IRQ points at won't exist, and the call will > fail. No, I do not agree completely here. Current patch just changes the way to get irq info in the RTC driver. If proper irq number is passed from MFD, then it proceeds further and deals with it. In case of missing valid irq info, this returns error as in earlier case. I agree with you that this patch alone cannot make RTC completely functional, till we get mfd patch as well. However should lack of mfd changes really block this patch, as these two are independent drivers. > > Also, the MFD patch you sent to create add the IRQ resource into the RTC > device's resource list sets the IRQ number to TPS65910_IRQ_RTC_ALARM, > which is the offset within the IRQ domain, not the absolute Linux IRQ > number. Where does the base of the IRQ domain get added on such that > this is an actual IRQ numbmer not an offset? > > > - dev_dbg(&pdev->dev, "Enabling rtc-tps65910.\n"); > > + dev_dbg(&pdev->dev, "Enabling tps65910 rtc.\n"); > > Unrelated change. Fine, I can move all of the unrelated changes mentioned here to different patch. > > > - irq = pmic_plat_data->irq_base; > > + irq = platform_get_irq(pdev, 0); > > There are two spaces there before the =. > > > - "rtc-tps65910", &pdev->dev); > > + "tps65910-rtc", &pdev->dev); > > Unrelated change. > > > - .name = "rtc-tps65910", > > + .name = "tps65910-rtc", > > Unrelated change.
On 09/24/2012 12:18 AM, Venu Byravarasu wrote: >> -----Original Message----- >> From: Stephen Warren [mailto:swarren@wwwdotorg.org] >> Sent: Friday, September 21, 2012 9:21 PM >> To: Venu Byravarasu >> Cc: akpm@linux-foundation.org; a.zummo@towertech.it; linux- >> kernel@vger.kernel.org; rtc-linux@googlegroups.com >> Subject: Re: [PATCH] rtc: tps65910: Use platform_get_irq to get RTC irq >> details >> >> On 09/21/2012 05:00 AM, Venu Byravarasu wrote: >>> As RTC driver needs only irq number from platform data, >>> using platform_get_irq(), instead of generic dev_get_platdata(). >> >> I assume this patch depends on "mfd: tps65910: Add alarm interrupt of >> TPS65910 RTC to mfd device list" which you posted just before? If so, >> the two patches should go through the same tree to avoid "git bisect" >> issues. >> >> Also, I thought you needed to fix the MFD driver to call >> mfd_add_devices() only after all the IRQ stuff had been set up - >> otherwise, when the RTC driver calls devm_request_threaded_irq(), the >> parent IRQ domain that the IRQ points at won't exist, and the call will >> fail. > > No, I do not agree completely here. > Current patch just changes the way to get irq info in the RTC driver. > If proper irq number is passed from MFD, then it proceeds further and deals with it. > In case of missing valid irq info, this returns error as in earlier case. > > I agree with you that this patch alone cannot make RTC completely functional, > till we get mfd patch as well. > > However should lack of mfd changes really block this patch, as these two are > independent drivers. OK, if this feature (either the RTC as a whole, or the RTC driver retrieving and using the interrupt) doesn't already work, then as you say there are no dependencies, so this is fine.
> -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Monday, September 24, 2012 10:54 PM > To: Venu Byravarasu > Cc: akpm@linux-foundation.org; a.zummo@towertech.it; linux- > kernel@vger.kernel.org; rtc-linux@googlegroups.com > Subject: Re: [PATCH] rtc: tps65910: Use platform_get_irq to get RTC irq > details > > On 09/24/2012 12:18 AM, Venu Byravarasu wrote: > >> -----Original Message----- > >> From: Stephen Warren [mailto:swarren@wwwdotorg.org] > >> Sent: Friday, September 21, 2012 9:21 PM > >> To: Venu Byravarasu > >> Cc: akpm@linux-foundation.org; a.zummo@towertech.it; linux- > >> kernel@vger.kernel.org; rtc-linux@googlegroups.com > >> Subject: Re: [PATCH] rtc: tps65910: Use platform_get_irq to get RTC irq > >> details > >> > >> On 09/21/2012 05:00 AM, Venu Byravarasu wrote: > >>> As RTC driver needs only irq number from platform data, > >>> using platform_get_irq(), instead of generic dev_get_platdata(). > >> > >> I assume this patch depends on "mfd: tps65910: Add alarm interrupt of > >> TPS65910 RTC to mfd device list" which you posted just before? If so, > >> the two patches should go through the same tree to avoid "git bisect" > >> issues. > >> > >> Also, I thought you needed to fix the MFD driver to call > >> mfd_add_devices() only after all the IRQ stuff had been set up - > >> otherwise, when the RTC driver calls devm_request_threaded_irq(), the > >> parent IRQ domain that the IRQ points at won't exist, and the call will > >> fail. > > > > No, I do not agree completely here. > > Current patch just changes the way to get irq info in the RTC driver. > > If proper irq number is passed from MFD, then it proceeds further and > deals with it. > > In case of missing valid irq info, this returns error as in earlier case. > > > > I agree with you that this patch alone cannot make RTC completely > functional, > > till we get mfd patch as well. > > > > However should lack of mfd changes really block this patch, as these two > are > > independent drivers. > > OK, if this feature (either the RTC as a whole, or the RTC driver > retrieving and using the interrupt) doesn't already work, then as you > say there are no dependencies, so this is fine. Thanks Stephen. Will remove the unrelated changes as pointed by you on 1st patch and send updated patch.
diff --git a/drivers/rtc/rtc-tps65910.c b/drivers/rtc/rtc-tps65910.c index 691ab96..e3ffae0 100644 --- a/drivers/rtc/rtc-tps65910.c +++ b/drivers/rtc/rtc-tps65910.c @@ -226,7 +226,6 @@ static int __devinit tps65910_rtc_probe(struct platform_device *pdev) { struct tps65910 *tps65910 = NULL; struct tps65910_rtc *tps_rtc = NULL; - struct tps65910_board *pmic_plat_data; int ret; int irq; u32 rtc_reg; @@ -247,28 +246,27 @@ static int __devinit tps65910_rtc_probe(struct platform_device *pdev) if (ret < 0) return ret; - dev_dbg(&pdev->dev, "Enabling rtc-tps65910.\n"); + dev_dbg(&pdev->dev, "Enabling tps65910 rtc.\n"); rtc_reg = TPS65910_RTC_CTRL_STOP_RTC; ret = regmap_write(tps65910->regmap, TPS65910_RTC_CTRL, rtc_reg); if (ret < 0) return ret; - pmic_plat_data = dev_get_platdata(tps65910->dev); - irq = pmic_plat_data->irq_base; + irq = platform_get_irq(pdev, 0); if (irq <= 0) { dev_warn(&pdev->dev, "Wake up is not possible as irq = %d\n", irq); return ret; } - irq += TPS65910_IRQ_RTC_ALARM; ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, tps65910_rtc_interrupt, IRQF_TRIGGER_LOW, - "rtc-tps65910", &pdev->dev); + "tps65910-rtc", &pdev->dev); if (ret < 0) { - dev_err(&pdev->dev, "IRQ is not free.\n"); + dev_err(&pdev->dev, "IRQ %d is not free.\n", irq); return ret; } + device_init_wakeup(&pdev->dev, 1); tps_rtc->rtc = rtc_device_register(pdev->name, &pdev->dev, @@ -341,7 +339,7 @@ static struct platform_driver tps65910_rtc_driver = { .remove = __devexit_p(tps65910_rtc_remove), .driver = { .owner = THIS_MODULE, - .name = "rtc-tps65910", + .name = "tps65910-rtc", .pm = DEV_PM_OPS, }, };
As RTC driver needs only irq number from platform data, using platform_get_irq(), instead of generic dev_get_platdata(). Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com> --- drivers/rtc/rtc-tps65910.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-)