From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752628AbcAUB4P (ORCPT ); Wed, 20 Jan 2016 20:56:15 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:34503 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751193AbcAUB4J (ORCPT ); Wed, 20 Jan 2016 20:56:09 -0500 X-AuditID: cbfec7f4-f79026d00000418a-b0-56a03ab51d96 Subject: Re: [PATCH 5/8] rtc: max77686: Add max77802 support To: Javier Martinez Canillas , linux-kernel@vger.kernel.org References: <1453310088-29985-1-git-send-email-javier@osg.samsung.com> <1453310088-29985-6-git-send-email-javier@osg.samsung.com> Cc: Kukjin Kim , rtc-linux@googlegroups.com, Chanwoo Choi , Alexandre Belloni , Laxman Dewangan , linux-samsung-soc@vger.kernel.org From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <56A03AB7.3060109@samsung.com> Date: Thu, 21 Jan 2016 10:56:07 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 In-reply-to: <1453310088-29985-6-git-send-email-javier@osg.samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrJLMWRmVeSWpSXmKPExsVy+t/xy7pbrRaEGfztMrbouLaYyeL6l+es Fm/ermGyeP3C0KL/8Wtmi6X7VrNYXN41h81ixvl9TBb7OzsYHTg9nmy6yOixZ+JJNo9NqzrZ PHqb37F5bOm/y+7Rt2UVo8fnTXIB7FFcNimpOZllqUX6dglcGQ0PfrAUrI+tmPp0MmMD42HP LkZODgkBE4mVu26xQthiEhfurWfrYuTiEBJYyijx6+QSRgjnKaPEkf7H7CBVwgI2Ek/Wb2cE sUUEQiX+XbwNVdTMKLH7+UEmkASzwDtGiQl7bEFsNgFjic3Ll7BBrJCT6O2exNLFyMHBK6Al sbiDHyTMIqAq8WzFMbCZogIREoc7u8B28QoISvyYfI8FxOYUcJeYefgCM0grs4CexP2LWhCb 5CU2r3nLPIFRcBaSjlkIVbOQVC1gZF7FKJpamlxQnJSea6hXnJhbXJqXrpecn7uJERIhX3Yw Lj5mdYhRgINRiYf3xrX5YUKsiWXFlbmHGCU4mJVEeOVUFoQJ8aYkVlalFuXHF5XmpBYfYpTm YFES5527632IkEB6YklqdmpqQWoRTJaJg1OqgbFKvkCLa8+1hCsxghqHp1UF7l3OXjhDb75w 0eVb0ls9o9X2bEz2nZGy5+SZ3g0WK7PPfL38oW7qJ/8G9vrLmQEs5nn/S7lm5fBM+Sfw+/ii Folt0/iS2V5Iraw0iDrRwXeu4GXRrFeFTU+vc7F8E9vtPbXcJV5z20UpiRB7Be7PJdKl3Bl+ K5RYijMSDbWYi4oTAZUE/LaMAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Javier Martinez Canillas > --- > > 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