| 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: Thu, 21 Jan 2016 10:56:07 +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 5/8] rtc: max77686: Add max77802 support
On 21.01.2016 02:14, 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) They minium delay before a RTC update (16ms 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>
> ---
>
> drivers/rtc/rtc-max77686.c | 176 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 128 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 7316e41820c7..7a144e7ecd27 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
> *
> @@ -43,6 +43,13 @@
>
> #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 ALARM_ENABLE_VALUE 0x77
MAX77802_ALARM_ENABLE_VALUE
(it is specific to 77802, right?)
> +
> enum {
> RTC_SEC = 0,
> RTC_MIN,
> @@ -58,6 +65,10 @@ struct rtc_driver_data {
> unsigned long delay;
> int mask;
> const unsigned int *map;
> + /* Has a separate alarm enable register? */
> + bool rtcae;
> + /* Has a separate I2C regmap for the RTC? */
> + bool rtcrm;
Both members are a tongue twisters. :)
'rtcae' you are mostly using in an inverted way (!rtcae) so how about:
'alarm_enable_bit'?
'rtcrm' - 'separate_i2c_addr'?
By the way, I was thinking that you would do decoupling of i2c and
regmap here. It is not required (more useful for Laxman's patch) but it
might by a part of these series.
> };
>
> struct max77686_rtc_info {
> @@ -108,6 +119,8 @@ enum rtc_reg {
> REG_ALARM2_MONTH,
> REG_ALARM2_YEAR,
> REG_ALARM2_DATE,
> + REG_RTC_AE1,
> + REG_RTC_AE2,
> REG_RTC_END,
> };
>
> @@ -120,13 +133,36 @@ static const unsigned int max77686_map[REG_RTC_END] = {
> MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
> MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
> MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
> - MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
> + MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE, REG_RTC_NONE, REG_RTC_NONE,
> };
>
> static const struct rtc_driver_data max77686_drv_data = {
> .delay = 1600,
> .mask = 0x7f,
> .map = max77686_map,
> + .rtcae = false,
> + .rtcrm = true,
> +};
> +
> +static const unsigned int max77802_map[REG_RTC_END] = {
> + MAX77802_RTC_CONTROLM, MAX77802_RTC_CONTROL, MAX77802_RTC_UPDATE0,
> + REG_RTC_NONE, MAX77802_WTSR_SMPL_CNTL, MAX77802_RTC_SEC,
> + MAX77802_RTC_MIN, MAX77802_RTC_HOUR, MAX77802_RTC_WEEKDAY,
> + MAX77802_RTC_MONTH, MAX77802_RTC_YEAR, MAX77802_RTC_DATE,
> + MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN, MAX77802_ALARM1_HOUR,
> + MAX77686_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH, MAX77802_ALARM1_YEAR,
> + MAX77802_ALARM1_DATE, MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN,
> + MAX77802_ALARM1_HOUR, MAX77802_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH,
> + MAX77802_ALARM1_YEAR, MAX77802_ALARM1_DATE, MAX77802_RTC_AE1,
> + MAX77802_RTC_AE2,
> +};
> +
> +static const struct rtc_driver_data max77802_drv_data = {
> + .delay = 200,
> + .mask = 0xff,
> + .map = max77802_map,
> + .rtcae = true,
> + .rtcrm = false,
> };
>
> static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> @@ -148,12 +184,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->rtcae)
> + 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;
> @@ -161,13 +205,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->rtcae) {
> + 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);
Maybe in a separate patch use dev_warn()? It wasn't possible before
because you need 'info' argument but it is possible.
> + return -EINVAL;
> + }
> + } else {
> + data[RTC_YEAR] = tm->tm_year;
> }
> +
> return 0;
> }
>
> @@ -232,7 +282,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
> u8 data[RTC_NR_TIME];
> int ret;
>
> - ret = max77686_rtc_tm_to_data(tm, data);
> + ret = max77686_rtc_tm_to_data(tm, data, info);
> if (ret < 0)
> return ret;
>
> @@ -279,11 +329,24 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> max77686_rtc_data_to_tm(data, &alrm->time, info);
>
> alrm->enabled = 0;
> - for (i = 0; i < RTC_NR_TIME; i++) {
> - if (data[i] & ALARM_ENABLE_MASK) {
> - alrm->enabled = 1;
> - break;
> +
> + if (!info->drv_data->rtcae) {
> + for (i = 0; i < RTC_NR_TIME; i++) {
> + if (data[i] & ALARM_ENABLE_MASK) {
> + alrm->enabled = 1;
> + break;
> + }
> }
> + } else {
> + ret = regmap_read(info->max77686->regmap,
> + map[REG_RTC_AE1], &val);
> + if (ret < 0) {
> + dev_err(info->dev, "%s:%d fail to read alarm enable(%d)\n",
> + __func__, __LINE__, ret);
I don't like the func/LINE. I know that driver is using them already but
I think it is better not to add new usages of it.
> + goto out;
Actually I think there is a bug here already. The function will always
return '0'. Instead the 'out' label should return 'ret'. Can you fix it
in separate patch (with reported-by :) )?
> + }
> + if (val)
> + alrm->enabled = 1;
> }
>
> alrm->pending = 0;
> @@ -316,21 +379,27 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
> if (ret < 0)
> goto out;
>
> - ret = regmap_bulk_read(info->max77686->rtc_regmap,
> - map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> - if (ret < 0) {
> - dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> + if (!info->drv_data->rtcae) {
> + ret = regmap_bulk_read(info->max77686->rtc_regmap,
> + map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> + if (ret < 0) {
> + dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> __func__, ret);
> - goto out;
> - }
> + goto out;
> + }
>
> - max77686_rtc_data_to_tm(data, &tm, info);
> + max77686_rtc_data_to_tm(data, &tm, info);
>
> - for (i = 0; i < RTC_NR_TIME; i++)
> - data[i] &= ~ALARM_ENABLE_MASK;
> + for (i = 0; i < RTC_NR_TIME; i++)
> + data[i] &= ~ALARM_ENABLE_MASK;
> +
> + ret = regmap_bulk_write(info->max77686->rtc_regmap,
> + map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> + } else {
> + ret = regmap_write(info->max77686->regmap,
> + map[REG_RTC_AE1], 0);
> + }
>
> - ret = regmap_bulk_write(info->max77686->rtc_regmap,
> - map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> if (ret < 0) {
> dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
> __func__, ret);
> @@ -356,29 +425,35 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
> if (ret < 0)
> goto out;
>
> - ret = regmap_bulk_read(info->max77686->rtc_regmap,
> - map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> - if (ret < 0) {
> - dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> + if (!info->drv_data->rtcae) {
> + ret = regmap_bulk_read(info->max77686->rtc_regmap,
> + map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> + if (ret < 0) {
> + dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> __func__, ret);
> - goto out;
> - }
> -
> - max77686_rtc_data_to_tm(data, &tm, info);
> + goto out;
> + }
>
> - data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
> - data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> - data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
> - data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
> - if (data[RTC_MONTH] & 0xf)
> - data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> - if (data[RTC_YEAR] & info->drv_data->mask)
> - data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
> - if (data[RTC_DATE] & 0x1f)
> - data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
> + max77686_rtc_data_to_tm(data, &tm, info);
> +
> + data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
> + data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> + data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
> + data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
> + if (data[RTC_MONTH] & 0xf)
> + data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> + if (data[RTC_YEAR] & info->drv_data->mask)
> + data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
> + if (data[RTC_DATE] & 0x1f)
> + data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
> +
> + ret = regmap_bulk_write(info->max77686->rtc_regmap,
> + map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> + } else {
> + ret = regmap_write(info->max77686->regmap,
> + map[REG_RTC_AE1], ALARM_ENABLE_VALUE);
> + }
>
> - ret = regmap_bulk_write(info->max77686->rtc_regmap,
> - map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> if (ret < 0) {
> dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
> __func__, ret);
> @@ -396,7 +471,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> u8 data[RTC_NR_TIME];
> int ret;
>
> - ret = max77686_rtc_tm_to_data(&alrm->time, data);
> + ret = max77686_rtc_tm_to_data(&alrm->time, data, info);
> if (ret < 0)
> return ret;
>
> @@ -490,6 +565,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
> {
> struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
> struct max77686_rtc_info *info;
> + const struct platform_device_id *id = pdev->id_entry;
> int ret;
>
> dev_info(&pdev->dev, "%s\n", __func__);
> @@ -503,7 +579,10 @@ static int max77686_rtc_probe(struct platform_device *pdev)
> info->dev = &pdev->dev;
> info->max77686 = max77686;
> info->rtc = max77686->rtc;
> - info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;
Comment for previous patch: use platform_get_device_id(pdev).
Best regards,
Krzysztof
Powered by blists - more mailing lists