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

[Dmitry Krivoschekov <dmitry.krivoschekov@gmail.com>]

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
>