| Message ID | 4B8D501B.9010607@cam.ac.uk |
|---|---|
| State | Superseded |
| Headers | show |
On 03/02/10 17:51, Jonathan Cameron wrote: > Dear Robert, > > I've run into an issue with your rtc-pxa driver. > If one is using the driver via the in kernel interfaces > (rather than from userspace,) in my case for the periodic > timer to drive an IIO trigger, then the open call never > occurs and hence the relevant irq's are never requested. > > Obviously putting those calls in open is nice and tidy > for a driver only accessed by userspace, but for in kernel > calls, I guess they should be requested in the driver > probe rather than on file open. > > Do you have any objection to a patch to do this? As Robert has pointed out, in rtc-sa1100 and rtc-pxa the reason for this configuration is to allow them to coexist. They unfortunately use the same interrupt status registers etc and so it is left to userspace to only open one or the other at a time. I'm not quite sure if we can manage sharing this interrupt properly so as an alternative how about adding either an additional op to ensure that opening inside the kernel has the same effect as opening the file or just call the open op directly in rtc_class_open. The only driver doing anything other than interrupt requesting in here is rtc-m41t80 and I am far from sure what that is doing? Obviously we would also need to make a call to release in the appropriate place and more drivers do make use of this op. What do people think? While we are here, is there a similar arguement for the arrangement in rtc-s3c or is that just a case of copy and paste? Jonathan > > For reference, rtc-s3c, rtc-sa1100 do the same sort of thing and > so I guess would cause the same issues with in kernel > users. I can provide patches for both of those, but can only > test the sa1100 driver (on pxa27x). > > Thanks, > > Jonathan > > p.s I haven't tested the following as yet, but will do that this > evening. > > > Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk> > --- > drivers/rtc/rtc-pxa.c | 55 +++++++++++++++++++----------------------------- > 1 files changed, 22 insertions(+), 33 deletions(-) > > diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c > index e6351b7..1996971 100644 > --- a/drivers/rtc/rtc-pxa.c > +++ b/drivers/rtc/rtc-pxa.c > @@ -169,34 +169,6 @@ static irqreturn_t pxa_rtc_irq(int irq, void *dev_id) > return IRQ_HANDLED; > } > > -static int pxa_rtc_open(struct device *dev) > -{ > - struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev); > - int ret; > - > - ret = request_irq(pxa_rtc->irq_1Hz, pxa_rtc_irq, IRQF_DISABLED, > - "rtc 1Hz", dev); > - if (ret < 0) { > - dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_1Hz, > - ret); > - goto err_irq_1Hz; > - } > - ret = request_irq(pxa_rtc->irq_Alrm, pxa_rtc_irq, IRQF_DISABLED, > - "rtc Alrm", dev); > - if (ret < 0) { > - dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_Alrm, > - ret); > - goto err_irq_Alrm; > - } > - > - return 0; > - > -err_irq_Alrm: > - free_irq(pxa_rtc->irq_1Hz, dev); > -err_irq_1Hz: > - return ret; > -} > - > static void pxa_rtc_release(struct device *dev) > { > struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev); > @@ -204,9 +176,6 @@ static void pxa_rtc_release(struct device *dev) > spin_lock_irq(&pxa_rtc->lock); > rtsr_clear_bits(pxa_rtc, RTSR_PIALE | RTSR_RDALE1 | RTSR_HZE); > spin_unlock_irq(&pxa_rtc->lock); > - > - free_irq(pxa_rtc->irq_Alrm, dev); > - free_irq(pxa_rtc->irq_1Hz, dev); > } > > static int pxa_periodic_irq_set_freq(struct device *dev, int freq) > @@ -337,7 +306,6 @@ static int pxa_rtc_proc(struct device *dev, struct seq_file *seq) > } > > static const struct rtc_class_ops pxa_rtc_ops = { > - .open = pxa_rtc_open, > .release = pxa_rtc_release, > .ioctl = pxa_rtc_ioctl, > .read_time = pxa_rtc_read_time, > @@ -410,15 +378,34 @@ static int __init pxa_rtc_probe(struct platform_device *pdev) > goto err_rtc_reg; > } > > + ret = request_irq(pxa_rtc->irq_1Hz, pxa_rtc_irq, IRQF_DISABLED, > + "rtc 1Hz", dev); > + if (ret < 0) { > + dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_1Hz, > + ret); > + goto err_irq_1Hz; > + } > + ret = request_irq(pxa_rtc->irq_Alrm, pxa_rtc_irq, IRQF_DISABLED, > + "rtc Alrm", dev); > + if (ret < 0) { > + dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_Alrm, > + ret); > + goto err_irq_Alrm; > + } > + > device_init_wakeup(dev, 1); > > return 0; > - > +err_irq_Alrm: > + free_irq(pxa_rtc->irq_1Hz, dev); > +err_irq_1Hz: > + rtc_device_unregister(pxa_rtc->rtc); > err_rtc_reg: > iounmap(pxa_rtc->base); > err_ress: > err_map: > kfree(pxa_rtc); > + > return ret; > } > > @@ -426,6 +413,8 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev) > { > struct pxa_rtc *pxa_rtc = platform_get_drvdata(pdev); > > + free_irq(pxa_rtc->irq_Alrm, &pdev->dev); > + free_irq(pxa_rtc->irq_1Hz, &pdev->dev); > rtc_device_unregister(pxa_rtc->rtc); > > spin_lock_irq(&pxa_rtc->lock);
On Wed, 03 Mar 2010 14:02:52 +0000 Jonathan Cameron <jic23@cam.ac.uk> wrote: > to ensure that opening inside the kernel has the same effect as > opening the file or just call the open op directly in rtc_class_open. > The only driver doing anything other than interrupt requesting in here > is rtc-m41t80 and I am far from sure what that is doing? we might amend rtc_class_open to handle this issue. patches will be appreciated ;)
> > p.s I haven't tested the following as yet, but will do that this > evening. Now tested. Seems to work as expected with rtc-pxa. > > > Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk> > --- > drivers/rtc/rtc-pxa.c | 55 +++++++++++++++++++----------------------------- > 1 files changed, 22 insertions(+), 33 deletions(-) > > diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c > index e6351b7..1996971 100644 > --- a/drivers/rtc/rtc-pxa.c > +++ b/drivers/rtc/rtc-pxa.c > @@ -169,34 +169,6 @@ static irqreturn_t pxa_rtc_irq(int irq, void *dev_id) > return IRQ_HANDLED; > } > > -static int pxa_rtc_open(struct device *dev) > -{ > - struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev); > - int ret; > - > - ret = request_irq(pxa_rtc->irq_1Hz, pxa_rtc_irq, IRQF_DISABLED, > - "rtc 1Hz", dev); > - if (ret < 0) { > - dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_1Hz, > - ret); > - goto err_irq_1Hz; > - } > - ret = request_irq(pxa_rtc->irq_Alrm, pxa_rtc_irq, IRQF_DISABLED, > - "rtc Alrm", dev); > - if (ret < 0) { > - dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_Alrm, > - ret); > - goto err_irq_Alrm; > - } > - > - return 0; > - > -err_irq_Alrm: > - free_irq(pxa_rtc->irq_1Hz, dev); > -err_irq_1Hz: > - return ret; > -} > - > static void pxa_rtc_release(struct device *dev) > { > struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev); > @@ -204,9 +176,6 @@ static void pxa_rtc_release(struct device *dev) > spin_lock_irq(&pxa_rtc->lock); > rtsr_clear_bits(pxa_rtc, RTSR_PIALE | RTSR_RDALE1 | RTSR_HZE); > spin_unlock_irq(&pxa_rtc->lock); > - > - free_irq(pxa_rtc->irq_Alrm, dev); > - free_irq(pxa_rtc->irq_1Hz, dev); > } > > static int pxa_periodic_irq_set_freq(struct device *dev, int freq) > @@ -337,7 +306,6 @@ static int pxa_rtc_proc(struct device *dev, struct seq_file *seq) > } > > static const struct rtc_class_ops pxa_rtc_ops = { > - .open = pxa_rtc_open, > .release = pxa_rtc_release, > .ioctl = pxa_rtc_ioctl, > .read_time = pxa_rtc_read_time, > @@ -410,15 +378,34 @@ static int __init pxa_rtc_probe(struct platform_device *pdev) > goto err_rtc_reg; > } > > + ret = request_irq(pxa_rtc->irq_1Hz, pxa_rtc_irq, IRQF_DISABLED, > + "rtc 1Hz", dev); > + if (ret < 0) { > + dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_1Hz, > + ret); > + goto err_irq_1Hz; > + } > + ret = request_irq(pxa_rtc->irq_Alrm, pxa_rtc_irq, IRQF_DISABLED, > + "rtc Alrm", dev); > + if (ret < 0) { > + dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_Alrm, > + ret); > + goto err_irq_Alrm; > + } > + > device_init_wakeup(dev, 1); > > return 0; > - > +err_irq_Alrm: > + free_irq(pxa_rtc->irq_1Hz, dev); > +err_irq_1Hz: > + rtc_device_unregister(pxa_rtc->rtc); > err_rtc_reg: > iounmap(pxa_rtc->base); > err_ress: > err_map: > kfree(pxa_rtc); > + > return ret; > } > > @@ -426,6 +413,8 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev) > { > struct pxa_rtc *pxa_rtc = platform_get_drvdata(pdev); > > + free_irq(pxa_rtc->irq_Alrm, &pdev->dev); > + free_irq(pxa_rtc->irq_1Hz, &pdev->dev); > rtc_device_unregister(pxa_rtc->rtc); > > spin_lock_irq(&pxa_rtc->lock);
diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c index e6351b7..1996971 100644 --- a/drivers/rtc/rtc-pxa.c +++ b/drivers/rtc/rtc-pxa.c @@ -169,34 +169,6 @@ static irqreturn_t pxa_rtc_irq(int irq, void *dev_id) return IRQ_HANDLED; } -static int pxa_rtc_open(struct device *dev) -{ - struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev); - int ret; - - ret = request_irq(pxa_rtc->irq_1Hz, pxa_rtc_irq, IRQF_DISABLED, - "rtc 1Hz", dev); - if (ret < 0) { - dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_1Hz, - ret); - goto err_irq_1Hz; - } - ret = request_irq(pxa_rtc->irq_Alrm, pxa_rtc_irq, IRQF_DISABLED, - "rtc Alrm", dev); - if (ret < 0) { - dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_Alrm, - ret); - goto err_irq_Alrm; - } - - return 0; - -err_irq_Alrm: - free_irq(pxa_rtc->irq_1Hz, dev); -err_irq_1Hz: - return ret; -} - static void pxa_rtc_release(struct device *dev) { struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev); @@ -204,9 +176,6 @@ static void pxa_rtc_release(struct device *dev) spin_lock_irq(&pxa_rtc->lock); rtsr_clear_bits(pxa_rtc, RTSR_PIALE | RTSR_RDALE1 | RTSR_HZE); spin_unlock_irq(&pxa_rtc->lock); - - free_irq(pxa_rtc->irq_Alrm, dev); - free_irq(pxa_rtc->irq_1Hz, dev); } static int pxa_periodic_irq_set_freq(struct device *dev, int freq) @@ -337,7 +306,6 @@ static int pxa_rtc_proc(struct device *dev, struct seq_file *seq) } static const struct rtc_class_ops pxa_rtc_ops = { - .open = pxa_rtc_open, .release = pxa_rtc_release, .ioctl = pxa_rtc_ioctl, .read_time = pxa_rtc_read_time, @@ -410,15 +378,34 @@ static int __init pxa_rtc_probe(struct platform_device *pdev) goto err_rtc_reg; } + ret = request_irq(pxa_rtc->irq_1Hz, pxa_rtc_irq, IRQF_DISABLED, + "rtc 1Hz", dev); + if (ret < 0) { + dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_1Hz, + ret); + goto err_irq_1Hz; + } + ret = request_irq(pxa_rtc->irq_Alrm, pxa_rtc_irq, IRQF_DISABLED, + "rtc Alrm", dev); + if (ret < 0) { + dev_err(dev, "can't get irq %i, err %d\n", pxa_rtc->irq_Alrm, + ret); + goto err_irq_Alrm; + } + device_init_wakeup(dev, 1); return 0; - +err_irq_Alrm: + free_irq(pxa_rtc->irq_1Hz, dev); +err_irq_1Hz: + rtc_device_unregister(pxa_rtc->rtc); err_rtc_reg: iounmap(pxa_rtc->base); err_ress: err_map: kfree(pxa_rtc); + return ret; } @@ -426,6 +413,8 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev) { struct pxa_rtc *pxa_rtc = platform_get_drvdata(pdev); + free_irq(pxa_rtc->irq_Alrm, &pdev->dev); + free_irq(pxa_rtc->irq_1Hz, &pdev->dev); rtc_device_unregister(pxa_rtc->rtc); spin_lock_irq(&pxa_rtc->lock);
Dear Robert, I've run into an issue with your rtc-pxa driver. If one is using the driver via the in kernel interfaces (rather than from userspace,) in my case for the periodic timer to drive an IIO trigger, then the open call never occurs and hence the relevant irq's are never requested. Obviously putting those calls in open is nice and tidy for a driver only accessed by userspace, but for in kernel calls, I guess they should be requested in the driver probe rather than on file open. Do you have any objection to a patch to do this? For reference, rtc-s3c, rtc-sa1100 do the same sort of thing and so I guess would cause the same issues with in kernel users. I can provide patches for both of those, but can only test the sa1100 driver (on pxa27x). Thanks, Jonathan p.s I haven't tested the following as yet, but will do that this evening. Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk> --- drivers/rtc/rtc-pxa.c | 55 +++++++++++++++++++----------------------------- 1 files changed, 22 insertions(+), 33 deletions(-)