| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 27 Jan 2016 10:59:17 +0900
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Javier Martinez Canillas <javier@....samsung.com>,
linux-kernel@...r.kernel.org
Cc: Kukjin Kim <kgene@...nel.org>, rtc-linux@...glegroups.com,
Chanwoo Choi <cw00.choi@...sung.com>,
Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
Laxman Dewangan <ldewangan@...dia.com>,
linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v3 06/10] rtc: max77686: Add max77802 support
On 27.01.2016 04:20, Javier Martinez Canillas wrote:
> The MAX77686 and MAX77802 RTC IP blocks are very similar with only
> these differences:
>
> 0) The RTC registers layout and addresses are different.
>
> 1) The MAX77686 use 1 bit of the sec/min/hour/etc registers as the
> alarm enable while MAX77802 has a separate register for that.
>
> 2) The MAX77686 RTCYEAR register valid values range is 0..99 while
> for MAX77802 is 0..199.
>
> 3) The MAX77686 has a separate I2C address for the RTC registers
> while the MAX77802 uses the same I2C address as the PMIC regs.
>
> 5) The minimum delay before a RTC update (16 msecs vs 200 usecs).
>
> There are separate drivers for MAX77686 and MAX77802 RTC IP blocks
> but the differences are not that big so the driver can be extended
> to support both instead of duplicating a lot of code in 2 drivers.
>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
> Signed-off-by: Javier Martinez Canillas <javier@....samsung.com>
> Acked-by: Laxman Dewangan <ldewangan@...dia.com>
>
> ---
>
> Changes in v3:
> - Add Laxman Dewangan's Acked-by tag to patch #6.
>
> Changes in v2:
> - Add a MAX77802 prefix to ALARM_ENABLE_VALUE. Suggested by Krzysztof Kozlowski.
> - Rename .rtcae to .alarm_enable_reg and .rtcrm to .separate_i2c_addr.
> Suggested by Krzysztof Kozlowski.
> - Don't use func and LINE in error messages. Suggested by Krzysztof Kozlowski.
> - Remove REG_RTC_AE2 since is not used by neither max77686 nor max77802.
> - Check if REG_RTC_AE1 has a valid address before accessing it.
>
> drivers/rtc/rtc-max77686.c | 208 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 162 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 1f501b6fc314..09fc73016d3a 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -1,5 +1,5 @@
> /*
> - * RTC driver for Maxim MAX77686
> + * RTC driver for Maxim MAX77686 and MAX77802
> *
> * Copyright (C) 2012 Samsung Electronics Co.Ltd
> *
> @@ -41,6 +41,15 @@
> #define ALARM_ENABLE_SHIFT 7
> #define ALARM_ENABLE_MASK (1 << ALARM_ENABLE_SHIFT)
>
> +#define REG_RTC_NONE 0xdeadbeef
> +
> +/*
> + * MAX77802 has separate register (RTCAE1) for alarm enable instead
> + * using 1 bit from registers RTC{SEC,MIN,HOUR,DAY,MONTH,YEAR,DATE}
> + * as in done in MAX77686.
> + */
> +#define MAX77802_ALARM_ENABLE_VALUE 0x77
> +
> enum {
> RTC_SEC = 0,
> RTC_MIN,
> @@ -59,6 +68,10 @@ struct max77686_rtc_driver_data {
> u8 mask;
> /* Registers offset to I2C addresses map */
> const unsigned int *map;
> + /* Has a separate alarm enable register? */
> + bool alarm_enable_reg;
> + /* Has a separate I2C regmap for the RTC? */
> + bool separate_i2c_addr;
> };
>
> struct max77686_rtc_info {
> @@ -108,6 +121,7 @@ enum max77686_rtc_reg_offset {
> REG_ALARM2_MONTH,
> REG_ALARM2_YEAR,
> REG_ALARM2_DATE,
> + REG_RTC_AE1,
> REG_RTC_END,
> };
>
> @@ -138,12 +152,52 @@ static const unsigned int max77686_map[REG_RTC_END] = {
> [REG_ALARM2_MONTH] = MAX77686_ALARM2_MONTH,
> [REG_ALARM2_YEAR] = MAX77686_ALARM2_YEAR,
> [REG_ALARM2_DATE] = MAX77686_ALARM2_DATE,
> + [REG_RTC_AE1] = REG_RTC_NONE,
> };
>
> static const struct max77686_rtc_driver_data max77686_drv_data = {
> .delay = 16000,
> .mask = 0x7f,
> .map = max77686_map,
> + .alarm_enable_reg = false,
> + .separate_i2c_addr = true,
> +};
> +
> +static const unsigned int max77802_map[REG_RTC_END] = {
> + [REG_RTC_CONTROLM] = MAX77802_RTC_CONTROLM,
> + [REG_RTC_CONTROL] = MAX77802_RTC_CONTROL,
> + [REG_RTC_UPDATE0] = MAX77802_RTC_UPDATE0,
> + [REG_WTSR_SMPL_CNTL] = MAX77802_WTSR_SMPL_CNTL,
> + [REG_RTC_SEC] = MAX77802_RTC_SEC,
> + [REG_RTC_MIN] = MAX77802_RTC_MIN,
> + [REG_RTC_HOUR] = MAX77802_RTC_HOUR,
> + [REG_RTC_WEEKDAY] = MAX77802_RTC_WEEKDAY,
> + [REG_RTC_MONTH] = MAX77802_RTC_MONTH,
> + [REG_RTC_YEAR] = MAX77802_RTC_YEAR,
> + [REG_RTC_DATE] = MAX77802_RTC_DATE,
> + [REG_ALARM1_SEC] = MAX77802_ALARM1_SEC,
> + [REG_ALARM1_MIN] = MAX77802_ALARM1_MIN,
> + [REG_ALARM1_HOUR] = MAX77802_ALARM1_HOUR,
> + [REG_ALARM1_WEEKDAY] = MAX77802_ALARM1_WEEKDAY,
> + [REG_ALARM1_MONTH] = MAX77802_ALARM1_MONTH,
> + [REG_ALARM1_YEAR] = MAX77802_ALARM1_YEAR,
> + [REG_ALARM1_DATE] = MAX77802_ALARM1_DATE,
> + [REG_ALARM2_SEC] = MAX77802_ALARM2_SEC,
> + [REG_ALARM2_MIN] = MAX77802_ALARM2_MIN,
> + [REG_ALARM2_HOUR] = MAX77802_ALARM2_HOUR,
> + [REG_ALARM2_WEEKDAY] = MAX77802_ALARM2_WEEKDAY,
> + [REG_ALARM2_MONTH] = MAX77802_ALARM2_MONTH,
> + [REG_ALARM2_YEAR] = MAX77802_ALARM2_YEAR,
> + [REG_ALARM2_DATE] = MAX77802_ALARM2_DATE,
> + [REG_RTC_AE1] = MAX77802_RTC_AE1,
> +};
> +
> +static const struct max77686_rtc_driver_data max77802_drv_data = {
> + .delay = 200,
> + .mask = 0xff,
> + .map = max77802_map,
> + .alarm_enable_reg = true,
> + .separate_i2c_addr = false,
> };
>
> static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> @@ -165,12 +219,20 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
> tm->tm_mday = data[RTC_DATE] & 0x1f;
> tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
> - tm->tm_year = (data[RTC_YEAR] & mask) + 100;
> + tm->tm_year = data[RTC_YEAR] & mask;
> tm->tm_yday = 0;
> tm->tm_isdst = 0;
> +
> + /*
> + * MAX77686 uses 1 bit from sec/min/hour/etc RTC registers and the
> + * year values are just 0..99 so add 100 to support up to 2099.
> + */
> + if (!info->drv_data->alarm_enable_reg)
> + tm->tm_year += 100;
> }
>
> -static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
> +static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
> + struct max77686_rtc_info *info)
> {
> data[RTC_SEC] = tm->tm_sec;
> data[RTC_MIN] = tm->tm_min;
> @@ -178,13 +240,19 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
> data[RTC_WEEKDAY] = 1 << tm->tm_wday;
> data[RTC_DATE] = tm->tm_mday;
> data[RTC_MONTH] = tm->tm_mon + 1;
> - data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
>
> - if (tm->tm_year < 100) {
> - pr_warn("RTC cannot handle the year %d. Assume it's 2000.\n",
> - 1900 + tm->tm_year);
> - return -EINVAL;
> + if (!info->drv_data->alarm_enable_reg) {
I don't like all these inverted checks. They are error-prone. This is
why I propose different name. However since you want to stick to this
name of this property, then easier to read would be:
if (info->drv_data->alarm_enable_reg) {
data[RTC_YEAR] = tm->tm_year;
} else {
max77686-stuff...
}
Can you reverse it here and in other places?
The patch beside that is okay and works fine:
Tested on Trats2 (max77686):
Tested-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
BR,
Krzysztof
> + data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
> +
> + if (tm->tm_year < 100) {
> + pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
> + 1900 + tm->tm_year);
> + return -EINVAL;
> + }
> + } else {
> + data[RTC_YEAR] = tm->tm_year;
> }
> +
> return 0;
> }
>
Powered by blists - more mailing lists