From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752777AbcA0B72 (ORCPT ); Tue, 26 Jan 2016 20:59:28 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:14358 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751333AbcA0B7Y (ORCPT ); Tue, 26 Jan 2016 20:59:24 -0500 X-AuditID: cbfec7f4-f79026d00000418a-a6-56a8247980d9 Subject: Re: [PATCH v3 06/10] rtc: max77686: Add max77802 support To: Javier Martinez Canillas , linux-kernel@vger.kernel.org References: <1453836020-29579-1-git-send-email-javier@osg.samsung.com> <1453836020-29579-7-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 Message-id: <56A82475.1040602@samsung.com> Date: Wed, 27 Jan 2016 10:59:17 +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: <1453836020-29579-7-git-send-email-javier@osg.samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrDLMWRmVeSWpSXmKPExsVy+t/xy7qVKivCDJ5PkLLouLaYyeL6l+es Fm/ermGyeP3C0KL/8Wtmi6X7VrNYXN41h81ixvl9TBb7OzsYHTg9nmy6yOixZ+JJNo9NqzrZ PHqb37F5bOm/y+7Rt2UVo8fnTXIB7FFcNimpOZllqUX6dglcGRtXPmEseGxZcXJxE1sD4xHd LkZODgkBE4lXN68xQ9hiEhfurWfrYuTiEBJYyijx/es7RgjnKaPE1VtTWUGqhAUcJXr3LGME sUUEQiX+XbwNVdTMKNH2bT4LSIJZ4B2jxIQ9tiA2m4CxxOblS4DGcnDwCmhJvHmUB2KyCKhK tL3UBqkQFYiQONzZxQ5i8woISvyYfA9sCqeAu8Si+bsYQcqZBfQk7l/UghguL7F5zVvmCYwC s5B0zEKomoWkagEj8ypG0dTS5ILipPRcQ73ixNzi0rx0veT83E2MkDj4soNx8TGrQ4wCHIxK PLw39i8PE2JNLCuuzD3EKMHBrCTC26O0IkyINyWxsiq1KD++qDQntfgQozQHi5I479xd70OE BNITS1KzU1MLUotgskwcnFINjBWKC0OnvtUzn/7Yevfa6Ny5MWvmTWt7fl8wZMfHVUu+Llnu o+n79PYqz53mFUETmXampRyTuuT+2uTPhSWbFoq9a3tm11w5k/mdFMf/nYtiLFKCPS6lJ/Fc 2a2276K2R8HWra1Or3bkHvqhLBK+/6ywskoCyy3Ld0bXZ0wxvDFvX1dgoHlDRY8SS3FGoqEW c1FxIgD5SlvdfwIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Javier Martinez Canillas > Acked-by: Laxman Dewangan > > --- > > 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 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; > } >