[PATCH resend v2] TWL4030-RTC: Fix the broken functionality

[PATCH resend v2] TWL4030-RTC: Fix the broken functionality

[Pakaravoor, Jagadeesh]

From: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>

rtc_irq_set_freq() function takes only powers of 2 as a valid
argument. This stipulates that an ioctl call on /dev/rtc0
can accept values of 1,2,4 and 8 only. But the function 
twl4030_rtc_irq_set_freq() takes only values 0-3. RTC_INTERRUPTS_REG
of TWL4030 also requires value in the range 0-3 for the interrupt
period. Hence it is required to map the argument from the powers of 2,
to the corresponding numbers 0-3, via a call to ilog2.

Signed-off-by: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
---
Fixing a bug in the last patch: argument to set_rtc_irq_bit()
function should have been "value" instead of "freq".
Index: my-local-git-dir/drivers/rtc/rtc-twl4030.c
===================================================================
--- my-local-git-dir.orig/drivers/rtc/rtc-twl4030.c	2008-08-22 17:37:33.000000000 +0530
+++ my-local-git-dir/drivers/rtc/rtc-twl4030.c	2008-08-22 19:25:41.616786397 +0530
@@ -38,6 +38,7 @@
 #include <linux/i2c/twl4030-rtc.h>
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/log2.h>
 
 #include <asm/mach/time.h>
 #include <asm/system.h>
@@ -363,14 +364,26 @@ out:
 static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
 {
 	struct rtc_device *rtc = dev_get_drvdata(dev);
+	u8 value;
 
-	if (freq < 0 || freq > 3)
+	/* RTC_INTERRUPTS_REG takes the value of 0 for interrupts every
+	 * second, 1 for every minute, 2 every hour and 3 every day.
+	 * But freq is in 2^N format, which needs to be converted back.
+	 */
+	value = ilog2(freq);
+	if (value < 0 || value > 3)
 		return -EINVAL;
 
-	rtc->irq_freq = freq;
+	rtc->irq_freq = value;
+
+	/* Clear the current periodicity of irq*/
+	mask_rtc_irq_bit(0x3);
 
-	/* set rtc irq freq to user defined value */
-	set_rtc_irq_bit(freq);
+	/* If the new value is non-zero, set the new periodicity */
+	if (value) {
+		/* set rtc irq freq to user defined value */
+		set_rtc_irq_bit(value);
+	}
 
 	return 0;
 }

--
With Regards,
Jagadeesh Bhaskar P
 
----------------------------
Some men see things as they are and say why - I dream things that never were and say why not.
- George Bernard Shaw
-------------------

Re: [PATCH resend v2] TWL4030-RTC: Fix the broken functionality

[Dmitry Krivoschekov]

Pakaravoor, Jagadeesh wrote:
> From: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
>
> rtc_irq_set_freq() function takes only powers of 2 as a valid
> argument. This stipulates that an ioctl call on /dev/rtc0
> can accept values of 1,2,4 and 8 only. But the function 
> twl4030_rtc_irq_set_freq() takes only values 0-3. RTC_INTERRUPTS_REG
> of TWL4030 also requires value in the range 0-3 for the interrupt
> period. Hence it is required to map the argument from the powers of 2,
> to the corresponding numbers 0-3, via a call to ilog2.
>
Note, the rtc_irq_set_freq() function accepts a frequency measured in
Hz. That is, those values (1,2,4 and 8) should be interpreted as 1Hz,
2Hz, 4Hz and 8Hz while you consider them as 1 == 1 sec period, 2 == 1
minute period, 4 == 1 hour period and 8 == 1 day period, that is wrong.

Yes, TWL4030 RTC has the capability to set only those four periods which
does not fit well to periodic IRQ concept of RTC framework. But it does
not mean you can violate a common concept of periodic IRQs.

The patch might be acceptable as a temporary workaround, but obviously
it won't be accepted in mainline, as well as commit
7ef9abe1e9cf53c2dd7556f12d35b06053847f72 which has introduced periodic
IRQ functionality to rtc-twl4030 driver.


Thanks,
Dmitry

> Signed-off-by: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
> ---
> Fixing a bug in the last patch: argument to set_rtc_irq_bit()
> function should have been "value" instead of "freq".
> Index: my-local-git-dir/drivers/rtc/rtc-twl4030.c
> ===================================================================
> --- my-local-git-dir.orig/drivers/rtc/rtc-twl4030.c	2008-08-22 17:37:33.000000000 +0530
> +++ my-local-git-dir/drivers/rtc/rtc-twl4030.c	2008-08-22 19:25:41.616786397 +0530
> @@ -38,6 +38,7 @@
>  #include <linux/i2c/twl4030-rtc.h>
>  #include <linux/io.h>
>  #include <linux/irq.h>
> +#include <linux/log2.h>
>  
>  #include <asm/mach/time.h>
>  #include <asm/system.h>
> @@ -363,14 +364,26 @@ out:
>  static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
>  {
>  	struct rtc_device *rtc = dev_get_drvdata(dev);
> +	u8 value;
>  
> -	if (freq < 0 || freq > 3)
> +	/* RTC_INTERRUPTS_REG takes the value of 0 for interrupts every
> +	 * second, 1 for every minute, 2 every hour and 3 every day.
> +	 * But freq is in 2^N format, which needs to be converted back.
> +	 */
> +	value = ilog2(freq);
> +	if (value < 0 || value > 3)
>  		return -EINVAL;
>  
> -	rtc->irq_freq = freq;
> +	rtc->irq_freq = value;
> +
> +	/* Clear the current periodicity of irq*/
> +	mask_rtc_irq_bit(0x3);
>  
> -	/* set rtc irq freq to user defined value */
> -	set_rtc_irq_bit(freq);
> +	/* If the new value is non-zero, set the new periodicity */
> +	if (value) {
> +		/* set rtc irq freq to user defined value */
> +		set_rtc_irq_bit(value);
> +	}
>  
>  	return 0;
>  }
>
> --
> With Regards,
> Jagadeesh Bhaskar P
>  
> ----------------------------
> Some men see things as they are and say why - I dream things that never were and say why not.
> - George Bernard Shaw
> -------------------
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH resend v2] TWL4030-RTC: Fix the broken functionality

[Tony Lindgren]

* Dmitry Krivoschekov <dmitry.krivoschekov@gmail.com> [080825 05:31]:
> Pakaravoor, Jagadeesh wrote:
> > From: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
> >
> > rtc_irq_set_freq() function takes only powers of 2 as a valid
> > argument. This stipulates that an ioctl call on /dev/rtc0
> > can accept values of 1,2,4 and 8 only. But the function 
> > twl4030_rtc_irq_set_freq() takes only values 0-3. RTC_INTERRUPTS_REG
> > of TWL4030 also requires value in the range 0-3 for the interrupt
> > period. Hence it is required to map the argument from the powers of 2,
> > to the corresponding numbers 0-3, via a call to ilog2.
> >
> Note, the rtc_irq_set_freq() function accepts a frequency measured in
> Hz. That is, those values (1,2,4 and 8) should be interpreted as 1Hz,
> 2Hz, 4Hz and 8Hz while you consider them as 1 == 1 sec period, 2 == 1
> minute period, 4 == 1 hour period and 8 == 1 day period, that is wrong.
> 
> Yes, TWL4030 RTC has the capability to set only those four periods which
> does not fit well to periodic IRQ concept of RTC framework. But it does
> not mean you can violate a common concept of periodic IRQs.
> 
> The patch might be acceptable as a temporary workaround, but obviously
> it won't be accepted in mainline, as well as commit
> 7ef9abe1e9cf53c2dd7556f12d35b06053847f72 which has introduced periodic
> IRQ functionality to rtc-twl4030 driver.

Sounds like this still needs work, so not pushing.

Tony

> 
> 
> Thanks,
> Dmitry
> 
> > Signed-off-by: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
> > ---
> > Fixing a bug in the last patch: argument to set_rtc_irq_bit()
> > function should have been "value" instead of "freq".
> > Index: my-local-git-dir/drivers/rtc/rtc-twl4030.c
> > ===================================================================
> > --- my-local-git-dir.orig/drivers/rtc/rtc-twl4030.c	2008-08-22 17:37:33.000000000 +0530
> > +++ my-local-git-dir/drivers/rtc/rtc-twl4030.c	2008-08-22 19:25:41.616786397 +0530
> > @@ -38,6 +38,7 @@
> >  #include <linux/i2c/twl4030-rtc.h>
> >  #include <linux/io.h>
> >  #include <linux/irq.h>
> > +#include <linux/log2.h>
> >  
> >  #include <asm/mach/time.h>
> >  #include <asm/system.h>
> > @@ -363,14 +364,26 @@ out:
> >  static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
> >  {
> >  	struct rtc_device *rtc = dev_get_drvdata(dev);
> > +	u8 value;
> >  
> > -	if (freq < 0 || freq > 3)
> > +	/* RTC_INTERRUPTS_REG takes the value of 0 for interrupts every
> > +	 * second, 1 for every minute, 2 every hour and 3 every day.
> > +	 * But freq is in 2^N format, which needs to be converted back.
> > +	 */
> > +	value = ilog2(freq);
> > +	if (value < 0 || value > 3)
> >  		return -EINVAL;
> >  
> > -	rtc->irq_freq = freq;
> > +	rtc->irq_freq = value;
> > +
> > +	/* Clear the current periodicity of irq*/
> > +	mask_rtc_irq_bit(0x3);
> >  
> > -	/* set rtc irq freq to user defined value */
> > -	set_rtc_irq_bit(freq);
> > +	/* If the new value is non-zero, set the new periodicity */
> > +	if (value) {
> > +		/* set rtc irq freq to user defined value */
> > +		set_rtc_irq_bit(value);
> > +	}
> >  
> >  	return 0;
> >  }
> >
> > --
> > With Regards,
> > Jagadeesh Bhaskar P
> >  
> > ----------------------------
> > Some men see things as they are and say why - I dream things that never were and say why not.
> > - George Bernard Shaw
> > -------------------
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend v2] TWL4030-RTC: Fix the broken functionality

[David Brownell]

On Tuesday 09 September 2008, Tony Lindgren wrote:
> 
> > The patch might be acceptable as a temporary workaround, but obviously
> > it won't be accepted in mainline, as well as commit
> > 7ef9abe1e9cf53c2dd7556f12d35b06053847f72 which has introduced periodic
> > IRQ functionality to rtc-twl4030 driver.
> 
> Sounds like this still needs work, so not pushing.

More like:  fixed by the RTC cleanup patch I sent, which
you said you're merging.  It removed that periodic IRQ
functionality, since the RTC interface only supports
periods of 2^N Hz while this RTC only supports periods
of 1 Hz ("update" irq) and below.

- Dave

Re: [PATCH resend v2] TWL4030-RTC: Fix the broken functionality

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [080909 11:36]:
> On Tuesday 09 September 2008, Tony Lindgren wrote:
> > 
> > > The patch might be acceptable as a temporary workaround, but obviously
> > > it won't be accepted in mainline, as well as commit
> > > 7ef9abe1e9cf53c2dd7556f12d35b06053847f72 which has introduced periodic
> > > IRQ functionality to rtc-twl4030 driver.
> > 
> > Sounds like this still needs work, so not pushing.
> 
> More like:  fixed by the RTC cleanup patch I sent, which
> you said you're merging.  It removed that periodic IRQ
> functionality, since the RTC interface only supports
> periods of 2^N Hz while this RTC only supports periods
> of 1 Hz ("update" irq) and below.

OK cool.

Tony