> -----Original Message----- > From: linux-omap-owner@xxxxxxxxxxxxxxx [mailto:linux-omap- > owner@xxxxxxxxxxxxxxx] On Behalf Of Kevin Hilman > Sent: Saturday, August 22, 2009 9:59 PM > To: Roger Quadros > Cc: Varadarajan, Charu Latha; linux-omap@xxxxxxxxxxxxxxx; > tony@xxxxxxxxxxx; david-b@xxxxxxxxxxx; sameo@xxxxxxxxxxxxxxx; > p_gortmaker@xxxxxxxxx > Subject: Re: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure > init > > Roger Quadros <ext-roger.quadros@xxxxxxxxx> writes: > > > ext charu@xxxxxx 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@xxxxxx> > > > > 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-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
