Patchwork rtc-pxa: why are irq's only requested in the open call()?

login
register
mail settings
Submitter Jonathan Cameron
Date March 2, 2010, 5:51 p.m.
Message ID <4B8D501B.9010607@cam.ac.uk>
Download mbox | patch
Permalink /patch/46644/
State New
Headers show

Comments

Jonathan Cameron - March 2, 2010, 5:51 p.m.
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(-)
Jonathan Cameron - March 3, 2010, 2:02 p.m.
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);
Alessandro Zummo - March 3, 2010, 2:08 p.m.
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 ;)
Jonathan Cameron - March 5, 2010, 12:17 p.m.
> 
> 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);

Patch

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);