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

[Kevin Hilman <khilman@deeprootsystems.com>]

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