[RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure init

[RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure init

[charu]

Triton2 RTC code changes for fixing periodic interrupt feature in RTC.
rtc-twlcore.c does initialisation of the msecure gpio pin. 
Board files indicate msecure gpio line through twl4030 platform data. 
twl4030-core.c passes this information to RTC driver.
Board files does msecure gpio mux configuration.


Signed-off-by: Charulatha V <charu@ti.com>
---
 drivers/rtc/rtc-twl4030.c |   63 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 62 insertions(+), 1 deletions(-)

diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c
index 9c8c70c..614adf0 100644
--- a/drivers/rtc/rtc-twl4030.c
+++ b/drivers/rtc/rtc-twl4030.c
@@ -29,7 +29,12 @@
 #include <linux/interrupt.h>
 
 #include <linux/i2c/twl4030.h>
+#include <linux/gpio.h>
 
+/*
+ * To find if the value is a power of two
+ */
+#define is_power_of_two(x)    (!((x) & ((x)-1)))
 
 /*
  * RTC block register offsets (use TWL_MODULE_RTC)
@@ -86,6 +91,37 @@
 /*----------------------------------------------------------------------*/
 
 /*
+ * msecure line initialisation for TWL4030 RTC registers write access
+ */
+static int msecure_init(struct twl4030_rtc_platform_data *pdata)
+{
+	int ret = 0;
+	if (pdata == NULL)
+		goto out;
+
+	ret = gpio_request(pdata->msecure_gpio, "msecure");
+	if (ret < 0) {
+		pr_err("twl4030_rtc: can't reserve msecure GPIO:%d !\n"
+			"RTC functionality will not be available\n",
+			pdata->msecure_gpio);
+		goto out;
+	}
+	/*
+	 * TWL4030 will be in secure mode if msecure line from OMAP is low.
+	 * Make msecure line high in order to change the TWL4030 RTC time
+	 * and calender registers.
+	 */
+	ret = gpio_direction_output(pdata->msecure_gpio, 1);
+	if (ret < 0)
+		pr_err("twl4030_rtc: can't set msecure GPIO direction:%d !\n"
+			"RTC functionality will not be available\n",
+			pdata->msecure_gpio);
+
+out:
+	return ret;
+}
+
+/*
  * Supports 1 byte read from TWL4030 RTC register.
  */
 static int twl4030_rtc_read_u8(u8 *data, u8 reg)
@@ -128,7 +164,6 @@ static int set_rtc_irq_bit(unsigned char bit)
 	int ret;
 
 	val = rtc_irq_bits | bit;
-	val &= ~BIT_RTC_INTERRUPTS_REG_EVERY_M;
 	ret = twl4030_rtc_write_u8(val, REG_RTC_INTERRUPTS_REG);
 	if (ret == 0)
 		rtc_irq_bits = val;
@@ -318,6 +353,25 @@ out:
 	return ret;
 }
 
+static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
+{
+	int ret, val = 1;
+	int regbit = 0;
+
+	if ((!is_power_of_2(freq)) || (freq > 8) || (freq <= 0))
+		return -EINVAL;
+
+	while ((freq & val) == 0) {
+		val = val << 1;
+		regbit++;
+	}
+	ret = set_rtc_irq_bit(regbit);
+	if (ret)
+		dev_err(dev, "rtc_irq_set_freq error %d\n", ret);
+
+	return ret;
+}
+
 static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc)
 {
 	unsigned long events = 0;
@@ -383,6 +437,7 @@ static struct rtc_class_ops twl4030_rtc_ops = {
 	.set_alarm	= twl4030_rtc_set_alarm,
 	.alarm_irq_enable = twl4030_rtc_alarm_irq_enable,
 	.update_irq_enable = twl4030_rtc_update_irq_enable,
+	.irq_set_freq   = twl4030_rtc_irq_set_freq,
 };
 
 /*----------------------------------------------------------------------*/
@@ -390,13 +445,19 @@ static struct rtc_class_ops twl4030_rtc_ops = {
 static int __devinit twl4030_rtc_probe(struct platform_device *pdev)
 {
 	struct rtc_device *rtc;
+	struct twl4030_rtc_platform_data *pdata = pdev->dev.platform_data;
 	int ret = 0;
 	int irq = platform_get_irq(pdev, 0);
+
 	u8 rd_reg;
 
 	if (irq <= 0)
 		return -EINVAL;
 
+	ret = msecure_init(pdata);
+	if (ret)
+		goto out0;
+
 	rtc = rtc_device_register(pdev->name,
 				  &pdev->dev, &twl4030_rtc_ops, THIS_MODULE);
 	if (IS_ERR(rtc)) {
-- 
1.6.0.4

Re: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure init

[Roger Quadros]

ext charu@ti.com wrote:
> Triton2 RTC code changes for fixing periodic interrupt feature in RTC.
> rtc-twlcore.c does initialisation of the msecure gpio pin. 
> Board files indicate msecure gpio line through twl4030 platform data. 
> twl4030-core.c passes this information to RTC driver.
> Board files does msecure gpio mux configuration.
> 
> 
> Signed-off-by: Charulatha V <charu@ti.com>

Periodic interrupts and msecure are 2 different entities. I think they should be 
implemented in different patches.

> ---
>  drivers/rtc/rtc-twl4030.c |   63 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 62 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c
> index 9c8c70c..614adf0 100644
> --- a/drivers/rtc/rtc-twl4030.c
> +++ b/drivers/rtc/rtc-twl4030.c
> @@ -29,7 +29,12 @@
>  #include <linux/interrupt.h>
>  
>  #include <linux/i2c/twl4030.h>
> +#include <linux/gpio.h>
>  
> +/*
> + * To find if the value is a power of two
> + */
> +#define is_power_of_two(x)    (!((x) & ((x)-1)))
>  
>  /*
>   * RTC block register offsets (use TWL_MODULE_RTC)
> @@ -86,6 +91,37 @@
>  /*----------------------------------------------------------------------*/
>  
>  /*
> + * msecure line initialisation for TWL4030 RTC registers write access
> + */
> +static int msecure_init(struct twl4030_rtc_platform_data *pdata)
> +{
> +	int ret = 0;
> +	if (pdata == NULL)
> +		goto out;
> +
> +	ret = gpio_request(pdata->msecure_gpio, "msecure");
> +	if (ret < 0) {

if (ret) should suffice

> +		pr_err("twl4030_rtc: can't reserve msecure GPIO:%d !\n"
> +			"RTC functionality will not be available\n",
> +			pdata->msecure_gpio);
> +		goto out;
> +	}
> +	/*
> +	 * TWL4030 will be in secure mode if msecure line from OMAP is low.
> +	 * Make msecure line high in order to change the TWL4030 RTC time
> +	 * and calender registers.
> +	 */
> +	ret = gpio_direction_output(pdata->msecure_gpio, 1);
> +	if (ret < 0)

ditto

> +		pr_err("twl4030_rtc: can't set msecure GPIO direction:%d !\n"
> +			"RTC functionality will not be available\n",
> +			pdata->msecure_gpio);
> +
> +out:
> +	return ret;
> +}
> +
> +/*
>   * Supports 1 byte read from TWL4030 RTC register.
>   */
>  static int twl4030_rtc_read_u8(u8 *data, u8 reg)
> @@ -128,7 +164,6 @@ static int set_rtc_irq_bit(unsigned char bit)
>  	int ret;
>  
>  	val = rtc_irq_bits | bit;
> -	val &= ~BIT_RTC_INTERRUPTS_REG_EVERY_M;
>  	ret = twl4030_rtc_write_u8(val, REG_RTC_INTERRUPTS_REG);
>  	if (ret == 0)
>  		rtc_irq_bits = val;
> @@ -318,6 +353,25 @@ out:
>  	return ret;
>  }
>  
> +static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
> +{
> +	int ret, val = 1;
> +	int regbit = 0;
> +
> +	if ((!is_power_of_2(freq)) || (freq > 8) || (freq <= 0))
> +		return -EINVAL;

0 is valid freq. it means disable periodic interrupts.

> +
> +	while ((freq & val) == 0) {
> +		val = val << 1;
> +		regbit++;
> +	}

as per your implementation, if user sets interrupt rate of 4 Hz then you will 
set regbit to 2 which means interrupt every hour? i.e. 0.00027 Hz. no?

> +	ret = set_rtc_irq_bit(regbit);
> +	if (ret)
> +		dev_err(dev, "rtc_irq_set_freq error %d\n", ret);
> +
> +	return ret;
> +}
> +
>  static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc)
>  {
>  	unsigned long events = 0;
> @@ -383,6 +437,7 @@ static struct rtc_class_ops twl4030_rtc_ops = {
>  	.set_alarm	= twl4030_rtc_set_alarm,
>  	.alarm_irq_enable = twl4030_rtc_alarm_irq_enable,
>  	.update_irq_enable = twl4030_rtc_update_irq_enable,
> +	.irq_set_freq   = twl4030_rtc_irq_set_freq,

IMHO this does not make sense.
twl4030 supports a max interrupt rate of 1 Hz (i.e. 1 sec). So you can only 
support freq values of 0 and 1 i.e. 0 for disabled and 1 for 1 sec interrupt.
This functionality is already achieved by update_irq_enable.

-roger

Re: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure init

[Kevin Hilman]

Roger Quadros <ext-roger.quadros@nokia.com> writes:

> ext charu@ti.com wrote:
>> Triton2 RTC code changes for fixing periodic interrupt feature in RTC.
>> rtc-twlcore.c does initialisation of the msecure gpio pin. Board
>> files indicate msecure gpio line through twl4030 platform
>> data. twl4030-core.c passes this information to RTC driver.
>> Board files does msecure gpio mux configuration.
>>
>>
>> Signed-off-by: Charulatha V <charu@ti.com>
>
> Periodic interrupts and msecure are 2 different entities. I think they
> should be implemented in different patches.

Agreed.  The "fix" part of this should be a separated out with a
detailed description of both the problem and the fix.

Kevin

>> ---
>>  drivers/rtc/rtc-twl4030.c |   63 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 62 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c
>> index 9c8c70c..614adf0 100644
>> --- a/drivers/rtc/rtc-twl4030.c
>> +++ b/drivers/rtc/rtc-twl4030.c
>> @@ -29,7 +29,12 @@
>>  #include <linux/interrupt.h>
>>   #include <linux/i2c/twl4030.h>
>> +#include <linux/gpio.h>
>>  +/*
>> + * To find if the value is a power of two
>> + */
>> +#define is_power_of_two(x)    (!((x) & ((x)-1)))
>>   /*
>>   * RTC block register offsets (use TWL_MODULE_RTC)
>> @@ -86,6 +91,37 @@
>>  /*----------------------------------------------------------------------*/
>>   /*
>> + * msecure line initialisation for TWL4030 RTC registers write access
>> + */
>> +static int msecure_init(struct twl4030_rtc_platform_data *pdata)
>> +{
>> +	int ret = 0;
>> +	if (pdata == NULL)
>> +		goto out;
>> +
>> +	ret = gpio_request(pdata->msecure_gpio, "msecure");
>> +	if (ret < 0) {
>
> if (ret) should suffice
>
>> +		pr_err("twl4030_rtc: can't reserve msecure GPIO:%d !\n"
>> +			"RTC functionality will not be available\n",
>> +			pdata->msecure_gpio);
>> +		goto out;
>> +	}
>> +	/*
>> +	 * TWL4030 will be in secure mode if msecure line from OMAP is low.
>> +	 * Make msecure line high in order to change the TWL4030 RTC time
>> +	 * and calender registers.
>> +	 */
>> +	ret = gpio_direction_output(pdata->msecure_gpio, 1);
>> +	if (ret < 0)
>
> ditto
>
>> +		pr_err("twl4030_rtc: can't set msecure GPIO direction:%d !\n"
>> +			"RTC functionality will not be available\n",
>> +			pdata->msecure_gpio);
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +/*
>>   * Supports 1 byte read from TWL4030 RTC register.
>>   */
>>  static int twl4030_rtc_read_u8(u8 *data, u8 reg)
>> @@ -128,7 +164,6 @@ static int set_rtc_irq_bit(unsigned char bit)
>>  	int ret;
>>   	val = rtc_irq_bits | bit;
>> -	val &= ~BIT_RTC_INTERRUPTS_REG_EVERY_M;
>>  	ret = twl4030_rtc_write_u8(val, REG_RTC_INTERRUPTS_REG);
>>  	if (ret == 0)
>>  		rtc_irq_bits = val;
>> @@ -318,6 +353,25 @@ out:
>>  	return ret;
>>  }
>>  +static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
>> +{
>> +	int ret, val = 1;
>> +	int regbit = 0;
>> +
>> +	if ((!is_power_of_2(freq)) || (freq > 8) || (freq <= 0))
>> +		return -EINVAL;
>
> 0 is valid freq. it means disable periodic interrupts.
>
>> +
>> +	while ((freq & val) == 0) {
>> +		val = val << 1;
>> +		regbit++;
>> +	}
>
> as per your implementation, if user sets interrupt rate of 4 Hz then
> you will set regbit to 2 which means interrupt every hour?
> i.e. 0.00027 Hz. no?
>
>> +	ret = set_rtc_irq_bit(regbit);
>> +	if (ret)
>> +		dev_err(dev, "rtc_irq_set_freq error %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>>  static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc)
>>  {
>>  	unsigned long events = 0;
>> @@ -383,6 +437,7 @@ static struct rtc_class_ops twl4030_rtc_ops = {
>>  	.set_alarm	= twl4030_rtc_set_alarm,
>>  	.alarm_irq_enable = twl4030_rtc_alarm_irq_enable,
>>  	.update_irq_enable = twl4030_rtc_update_irq_enable,
>> +	.irq_set_freq   = twl4030_rtc_irq_set_freq,
>
> IMHO this does not make sense.
> twl4030 supports a max interrupt rate of 1 Hz (i.e. 1 sec). So you can
> only support freq values of 0 and 1 i.e. 0 for disabled and 1 for 1
> sec interrupt.
> This functionality is already achieved by update_irq_enable.
>
> -roger
> --
> 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: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure init

[Trilok Soni]

Hi Charu,

On Wed, Aug 12, 2009 at 9:15 PM, <charu@ti.com> wrote:
> Triton2 RTC code changes for fixing periodic interrupt feature in RTC.
> rtc-twlcore.c does initialisation of the msecure gpio pin.
> Board files indicate msecure gpio line through twl4030 platform data.
> twl4030-core.c passes this information to RTC driver.
> Board files does msecure gpio mux configuration.
>
>
> Signed-off-by: Charulatha V <charu@ti.com>
> ---
>  drivers/rtc/rtc-twl4030.c |   63 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 62 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c
> index 9c8c70c..614adf0 100644
> --- a/drivers/rtc/rtc-twl4030.c
> +++ b/drivers/rtc/rtc-twl4030.c
> @@ -29,7 +29,12 @@
>  #include <linux/interrupt.h>
>
>  #include <linux/i2c/twl4030.h>
> +#include <linux/gpio.h>
>
> +/*
> + * To find if the value is a power of two
> + */
> +#define is_power_of_two(x)    (!((x) & ((x)-1)))
>

There is already is_power_of_2 or something like that in kernel,
please don't re-define. Probably check bits related code.

>  /*
>  * RTC block register offsets (use TWL_MODULE_RTC)
> @@ -86,6 +91,37 @@
>  /*----------------------------------------------------------------------*/
>
>  /*
> + * msecure line initialisation for TWL4030 RTC registers write access
> + */
> +static int msecure_init(struct twl4030_rtc_platform_data *pdata)
> +{
> +       int ret = 0;
> +       if (pdata == NULL)
> +               goto out;
> +
> +       ret = gpio_request(pdata->msecure_gpio, "msecure");
> +       if (ret < 0) {
> +               pr_err("twl4030_rtc: can't reserve msecure GPIO:%d !\n"
> +                       "RTC functionality will not be available\n",
> +                       pdata->msecure_gpio);
> +               goto out;
> +       }
> +       /*
> +        * TWL4030 will be in secure mode if msecure line from OMAP is low.
> +        * Make msecure line high in order to change the TWL4030 RTC time
> +        * and calender registers.
> +        */
> +       ret = gpio_direction_output(pdata->msecure_gpio, 1);
> +       if (ret < 0)
> +               pr_err("twl4030_rtc: can't set msecure GPIO direction:%d !\n"
> +                       "RTC functionality will not be available\n",
> +                       pdata->msecure_gpio);
> +
> +out:
> +       return ret;
> +}
> +
> +/*
>  * Supports 1 byte read from TWL4030 RTC register.
>  */
>  static int twl4030_rtc_read_u8(u8 *data, u8 reg)
> @@ -128,7 +164,6 @@ static int set_rtc_irq_bit(unsigned char bit)
>        int ret;
>
>        val = rtc_irq_bits | bit;
> -       val &= ~BIT_RTC_INTERRUPTS_REG_EVERY_M;
>        ret = twl4030_rtc_write_u8(val, REG_RTC_INTERRUPTS_REG);
>        if (ret == 0)
>                rtc_irq_bits = val;
> @@ -318,6 +353,25 @@ out:
>        return ret;
>  }
>
> +static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
> +{
> +       int ret, val = 1;
> +       int regbit = 0;
> +
> +       if ((!is_power_of_2(freq)) || (freq > 8) || (freq <= 0))
> +               return -EINVAL;
> +
> +       while ((freq & val) == 0) {
> +               val = val << 1;
> +               regbit++;
> +       }
> +       ret = set_rtc_irq_bit(regbit);
> +       if (ret)
> +               dev_err(dev, "rtc_irq_set_freq error %d\n", ret);
> +
> +       return ret;
> +}
> +
>  static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc)
>  {
>        unsigned long events = 0;
> @@ -383,6 +437,7 @@ static struct rtc_class_ops twl4030_rtc_ops = {
>        .set_alarm      = twl4030_rtc_set_alarm,
>        .alarm_irq_enable = twl4030_rtc_alarm_irq_enable,
>        .update_irq_enable = twl4030_rtc_update_irq_enable,
> +       .irq_set_freq   = twl4030_rtc_irq_set_freq,
>  };
>
>  /*----------------------------------------------------------------------*/
> @@ -390,13 +445,19 @@ static struct rtc_class_ops twl4030_rtc_ops = {
>  static int __devinit twl4030_rtc_probe(struct platform_device *pdev)
>  {
>        struct rtc_device *rtc;
> +       struct twl4030_rtc_platform_data *pdata = pdev->dev.platform_data;
>        int ret = 0;
>        int irq = platform_get_irq(pdev, 0);
> +
>        u8 rd_reg;
>
>        if (irq <= 0)
>                return -EINVAL;
>
> +       ret = msecure_init(pdata);
> +       if (ret)
> +               goto out0;
> +
>        rtc = rtc_device_register(pdev->name,
>                                  &pdev->dev, &twl4030_rtc_ops, THIS_MODULE);
>        if (IS_ERR(rtc)) {
> --
> 1.6.0.4
>
> --
> 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
>



-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
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: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure init

[Varadarajan, Charu Latha]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Saturday, August 22, 2009 9:59 PM
> To: Roger Quadros
> Cc: Varadarajan, Charu Latha; linux-omap@vger.kernel.org;
> tony@atomide.com; david-b@pacbell.net; sameo@linux.intel.com;
> p_gortmaker@yahoo.com
> Subject: Re: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure
> init
>
> Roger Quadros <ext-roger.quadros@nokia.com> writes:
>
> > ext charu@ti.com wrote:
> >> Triton2 RTC code changes for fixing periodic interrupt feature in RTC.
> >> rtc-twlcore.c does initialisation of the msecure gpio pin. Board
> >> files indicate msecure gpio line through twl4030 platform
> >> data. twl4030-core.c passes this information to RTC driver.
> >> Board files does msecure gpio mux configuration.
> >>
> >>
> >> Signed-off-by: Charulatha V <charu@ti.com>
> >
> > Periodic interrupts and msecure are 2 different entities. I think they
> > should be implemented in different patches.
>
> Agreed.  The "fix" part of this should be a separated out with a
> detailed description of both the problem and the fix.
>
> Kevin
Okay.
>
> >> ---
> >>  drivers/rtc/rtc-twl4030.c |   63
> ++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 files changed, 62 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c
> >> index 9c8c70c..614adf0 100644
> >> --- a/drivers/rtc/rtc-twl4030.c
> >> +++ b/drivers/rtc/rtc-twl4030.c
> >> @@ -29,7 +29,12 @@
> >>  #include <linux/interrupt.h>
> >>   #include <linux/i2c/twl4030.h>
> >> +#include <linux/gpio.h>
> >>  +/*
> >> + * To find if the value is a power of two
> >> + */
> >> +#define is_power_of_two(x)    (!((x) & ((x)-1)))
> >>   /*
> >>   * RTC block register offsets (use TWL_MODULE_RTC)
> >> @@ -86,6 +91,37 @@
> >>  /*--------------------------------------------------------------------
> --*/
> >>   /*
> >> + * msecure line initialisation for TWL4030 RTC registers write access
> >> + */
> >> +static int msecure_init(struct twl4030_rtc_platform_data *pdata)
> >> +{
> >> +  int ret = 0;
> >> +  if (pdata == NULL)
> >> +          goto out;
> >> +
> >> +  ret = gpio_request(pdata->msecure_gpio, "msecure");
> >> +  if (ret < 0) {
> >
> > if (ret) should suffice
> >
> >> +          pr_err("twl4030_rtc: can't reserve msecure GPIO:%d !\n"
> >> +                  "RTC functionality will not be available\n",
> >> +                  pdata->msecure_gpio);
> >> +          goto out;
> >> +  }
> >> +  /*
> >> +   * TWL4030 will be in secure mode if msecure line from OMAP is low.
> >> +   * Make msecure line high in order to change the TWL4030 RTC time
> >> +   * and calender registers.
> >> +   */
> >> +  ret = gpio_direction_output(pdata->msecure_gpio, 1);
> >> +  if (ret < 0)
> >
> > ditto
> >
> >> +          pr_err("twl4030_rtc: can't set msecure GPIO direction:%d !\n"
> >> +                  "RTC functionality will not be available\n",
> >> +                  pdata->msecure_gpio);
> >> +
> >> +out:
> >> +  return ret;
> >> +}
Agreed.
> >> +
> >> +/*
> >>   * Supports 1 byte read from TWL4030 RTC register.
> >>   */
> >>  static int twl4030_rtc_read_u8(u8 *data, u8 reg)
> >> @@ -128,7 +164,6 @@ static int set_rtc_irq_bit(unsigned char bit)
> >>    int ret;
> >>    val = rtc_irq_bits | bit;
> >> -  val &= ~BIT_RTC_INTERRUPTS_REG_EVERY_M;

> >>    ret = twl4030_rtc_write_u8(val, REG_RTC_INTERRUPTS_REG);
> >>    if (ret == 0)
> >>            rtc_irq_bits = val;
> >> @@ -318,6 +353,25 @@ out:
> >>    return ret;
> >>  }
> >>  +static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
> >> +{
> >> +  int ret, val = 1;
> >> +  int regbit = 0;
> >> +
> >> +  if ((!is_power_of_2(freq)) || (freq > 8) || (freq <= 0))
> >> +          return -EINVAL;
> >
> > 0 is valid freq. it means disable periodic interrupts.
> >
> >> +
> >> +  while ((freq & val) == 0) {
> >> +          val = val << 1;
> >> +          regbit++;
> >> +  }
> >
> > as per your implementation, if user sets interrupt rate of 4 Hz then
> > you will set regbit to 2 which means interrupt every hour?
> > i.e. 0.00027 Hz. no?
> >
> >> +  ret = set_rtc_irq_bit(regbit);
> >> +  if (ret)
> >> +          dev_err(dev, "rtc_irq_set_freq error %d\n", ret);
> >> +
> >> +  return ret;
> >> +}
> >> +
> >>  static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc)
> >>  {
> >>    unsigned long events = 0;
> >> @@ -383,6 +437,7 @@ static struct rtc_class_ops twl4030_rtc_ops = {
> >>    .set_alarm      = twl4030_rtc_set_alarm,
> >>    .alarm_irq_enable = twl4030_rtc_alarm_irq_enable,
> >>    .update_irq_enable = twl4030_rtc_update_irq_enable,
> >> +  .irq_set_freq   = twl4030_rtc_irq_set_freq,
> >
> > IMHO this does not make sense.
> > twl4030 supports a max interrupt rate of 1 Hz (i.e. 1 sec). So you can
> > only support freq values of 0 and 1 i.e. 0 for disabled and 1 for 1
> > sec interrupt.
> > This functionality is already achieved by update_irq_enable.
> >
> > -roger
> > --
Agreed that twl4030_rtc_update_irq_enable is enough if the periodic interrupt is required only for 1 Hz. 
If this is enough, then any ioctl call with RTC_IRQP_SET to twl4030 chip's RTC can do only
1 Hz freq setting. This means that "RTC_IRQP_SET" is not required at all for  twl4030 chip's RTC
and only "RTC_UIE_ON" is enough. Please clarify.
The twl4030 chip also supports interrupt every min, day and hour. Don't we need support for this? 
 
-V Charu Latha