Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754196AbbL3Dll (ORCPT ); Tue, 29 Dec 2015 22:41:41 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:18511 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754007AbbL3Dlj (ORCPT ); Tue, 29 Dec 2015 22:41:39 -0500 X-AuditID: cbfec7f5-f79b16d000005389-9c-56835270df98 Subject: Re: [PATCH 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions To: Yadwinder Singh Brar References: <1451440385-8654-1-git-send-email-k.kozlowski@samsung.com> <1451440385-8654-3-git-send-email-k.kozlowski@samsung.com> <56835199.50402@samsung.com> Cc: Sangbeom Kim , Alessandro Zummo , Alexandre Belloni , Lee Jones , linux-kernel , linux-samsung-soc , rtc-linux@googlegroups.com, Alim Akhtar From: Krzysztof Kozlowski Message-id: <56835270.2020603@samsung.com> Date: Wed, 30 Dec 2015 12:41:36 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-version: 1.0 In-reply-to: <56835199.50402@samsung.com> Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrILMWRmVeSWpSXmKPExsVy+t/xy7oFQc1hBnPmaFgsuXiV3aLj2mIm iwfztrFZvH5haHH/61FGi8u75rBZzDi/j8lif2cHo8XFFV+YLOb+bmR14PJ4sukio8fOWXfZ PfZMPMnmcefaHjaPvi2rGD2mz/vJ5PF5k1wAexSXTUpqTmZZapG+XQJXRuuhF6wFr4IqWi+s YWtgnGTZxcjBISFgIrF8Xl0XIyeQKSZx4d56ti5GLg4hgaWMEv1PbzJBOE8ZJd7taWUDqRIW SJP4cv83C4gtImAgMXHJPFaIopeMEu0XV7CDOMwCd5gkNnQeYQSpYhMwlti8fAkbyDpeAS2J Ld/NQMIsAqoSz1/sZQYJiwpESCzakQkS5hUQlPgx+R7YfE4BTYlH064wgZQwC6hLTJmSCxJm FpCX2LzmLfMERoFZSDpmIVTNQlK1gJF5FaNoamlyQXFSeq6RXnFibnFpXrpecn7uJkZIRHzd wbj0mNUhRgEORiUe3hNCzWFCrIllxZW5hxglOJiVRHgztYBCvCmJlVWpRfnxRaU5qcWHGKU5 WJTEeWfueh8iJJCeWJKanZpakFoEk2Xi4JRqYNT9uzJf4LgC65Uz9qs/697S4WP/ppmda1L9 Yu/NBJv5fnPW/nz1oMd59cJbB8Izbj5TXf68cq+9c0D4z5TT59/NsdlxzuvL3om3PSu+3ao5 f7gEqCVNhs2g1mjzzUmnb9f1lu6QiP0048Oe8KkFR5Z2pTzfmJ7rauMQcmrvRrGUrL+3pxz8 yajEUpyRaKjFXFScCADVtoBPhAIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10605 Lines: 247 On 30.12.2015 12:38, Krzysztof Kozlowski wrote: > On 30.12.2015 11:53, Yadwinder Singh Brar wrote: >> Hi Krysztof, >> >> On Tue, Dec 29, 2015 at 5:53 PM, Krzysztof Kozlowski >> wrote: >>> Before updating time and alarm the driver must set appropriate mask in >>> UDR register. For that purpose the driver uses common register >>> configuration and a lot of exceptions per device in the code. The >>> exceptions are not obvious, for example except the change in the logic >>> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and >>> S2MPS15). This leads to quite complicated code. >>> >>> Try to make it more obvious by: >>> 1. Documenting the UDR masks for devices and operations. >>> 2. Adding fields in register configuration structure for each operation >>> (read time, write time and alarm). >>> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus >>> removing exceptions for them. >>> >>> Signed-off-by: Krzysztof Kozlowski >>> >>> --- >>> >>> Tested only on S2MPS11 (Odroid XU4). >>> --- >>> drivers/rtc/rtc-s5m.c | 110 +++++++++++++++++++++++++++------------- >>> include/linux/mfd/samsung/rtc.h | 2 + >>> 2 files changed, 77 insertions(+), 35 deletions(-) >>> >>> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c >>> index 559db8f72117..7407d7394bb4 100644 >>> --- a/drivers/rtc/rtc-s5m.c >>> +++ b/drivers/rtc/rtc-s5m.c >>> @@ -38,7 +38,22 @@ >>> */ >>> #define UDR_READ_RETRY_CNT 5 >>> >>> -/* Registers used by the driver which are different between chipsets. */ >>> +/* >>> + * Registers used by the driver which are different between chipsets. >>> + * >>> + * Operations like read time and write alarm/time require updating >>> + * specific fields in UDR register. These fields usually are auto-cleared >>> + * (with some exceptions). >>> + * >>> + * Table of operations per device: >>> + * >>> + * Device | Write time | Read time | Write alarm >>> + * ================================================= >>> + * S5M8767 | UDR + TIME | | UDR >>> + * S2MPS11/14 | WUDR | RUDR | WUDR + RUDR >>> + * S2MPS13 | WUDR | RUDR | WUDR + AUDR >>> + * S2MPS15 | WUDR | RUDR | AUDR >>> + */ >>> struct s5m_rtc_reg_config { >>> /* Number of registers used for setting time/alarm0/alarm1 */ >>> unsigned int regs_count; >>> @@ -58,8 +73,13 @@ struct s5m_rtc_reg_config { >>> unsigned int udr_update; >>> /* Auto-cleared mask in UDR field for writing time and alarm */ >>> unsigned int autoclear_udr_mask; >>> - /* Mask for UDR field in 'udr_update' register */ >>> - unsigned int udr_mask; >>> + /* >>> + * Masks in UDR field for time and alarm operations. >>> + * The read time mask can be 0. Rest should not. >>> + */ >>> + unsigned int read_time_udr_mask; >>> + unsigned int write_time_udr_mask; >>> + unsigned int write_alarm_udr_mask; >>> }; >>> >>> /* Register map for S5M8763 and S5M8767 */ >>> @@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = { >>> .alarm1 = S5M_ALARM1_SEC, >>> .udr_update = S5M_RTC_UDR_CON, >>> .autoclear_udr_mask = S5M_RTC_UDR_MASK, >>> - .udr_mask = S5M_RTC_UDR_MASK, >>> + .read_time_udr_mask = 0, /* Not needed */ >>> + .write_time_udr_mask = S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK, >>> + .write_alarm_udr_mask = S5M_RTC_UDR_MASK, >>> +}; >>> + >>> +/* Register map for S2MPS13 */ >>> +static const struct s5m_rtc_reg_config s2mps13_rtc_regs = { >>> + .regs_count = 7, >>> + .time = S2MPS_RTC_SEC, >>> + .ctrl = S2MPS_RTC_CTRL, >>> + .alarm0 = S2MPS_ALARM0_SEC, >>> + .alarm1 = S2MPS_ALARM1_SEC, >>> + .udr_update = S2MPS_RTC_UDR_CON, >>> + .autoclear_udr_mask = S2MPS_RTC_WUDR_MASK, >>> + .read_time_udr_mask = S2MPS_RTC_RUDR_MASK, >>> + .write_time_udr_mask = S2MPS_RTC_WUDR_MASK, >>> + .write_alarm_udr_mask = S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK, >>> +}; >>> + >>> +/* Register map for S2MPS11/14 */ >>> +static const struct s5m_rtc_reg_config s2mps14_rtc_regs = { >>> + .regs_count = 7, >>> + .time = S2MPS_RTC_SEC, >>> + .ctrl = S2MPS_RTC_CTRL, >>> + .alarm0 = S2MPS_ALARM0_SEC, >>> + .alarm1 = S2MPS_ALARM1_SEC, >>> + .udr_update = S2MPS_RTC_UDR_CON, >>> + .autoclear_udr_mask = S2MPS_RTC_WUDR_MASK, >>> + .read_time_udr_mask = S2MPS_RTC_RUDR_MASK, >>> + .write_time_udr_mask = S2MPS_RTC_WUDR_MASK, >>> + .write_alarm_udr_mask = S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK, >>> }; >>> >>> /* >>> - * Register map for S2MPS14. >>> - * It may be also suitable for S2MPS11 but this was not tested. >>> + * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR bits >>> + * are swapped. >>> */ >>> -static const struct s5m_rtc_reg_config s2mps_rtc_regs = { >>> +static const struct s5m_rtc_reg_config s2mps15_rtc_regs = { >>> .regs_count = 7, >>> .time = S2MPS_RTC_SEC, >>> .ctrl = S2MPS_RTC_CTRL, >>> @@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = { >>> .alarm1 = S2MPS_ALARM1_SEC, >>> .udr_update = S2MPS_RTC_UDR_CON, >>> .autoclear_udr_mask = S2MPS_RTC_WUDR_MASK, >>> - .udr_mask = S2MPS_RTC_WUDR_MASK, >>> + .read_time_udr_mask = S2MPS_RTC_RUDR_MASK, >>> + .write_time_udr_mask = S2MPS15_RTC_WUDR_MASK, >>> + .write_alarm_udr_mask = S2MPS15_RTC_AUDR_MASK, >>> }; >>> >>> struct s5m_rtc_info { >>> @@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info) >>> return ret; >>> } >>> >>> - switch (info->device_type) { >>> - case S5M8763X: >>> - case S5M8767X: >>> - data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK; >>> - case S2MPS15X: >>> - /* As per UM, for write time register, set WUDR bit to high */ >>> - data |= S2MPS15_RTC_WUDR_MASK; >>> - break; >>> - case S2MPS14X: >>> - case S2MPS13X: >>> - data |= info->regs->udr_mask; >>> - break; >>> - default: >>> - return -EINVAL; >>> - } >>> + data |= info->regs->write_time_udr_mask; >>> >>> ret = regmap_write(info->regmap, info->regs->udr_update, data); >>> if (ret < 0) { >>> @@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info) >>> return ret; >>> } >>> >>> - data |= info->regs->udr_mask; >>> + data |= info->regs->write_alarm_udr_mask; >>> switch (info->device_type) { >>> case S5M8763X: >>> case S5M8767X: >>> data &= ~S5M_RTC_TIME_EN_MASK; >>> break; >>> case S2MPS15X: >>> - /* As per UM, for write alarm, set A_UDR(bit[4]) to high >>> - * udr_mask above sets bit[4] >>> - */ >>> - break; >>> case S2MPS14X: >>> - data |= S2MPS_RTC_RUDR_MASK; >>> - break; >>> case S2MPS13X: >>> - data |= S2MPS13_RTC_AUDR_MASK; >>> + /* No exceptions needed */ >>> break; >>> default: >>> return -EINVAL; >>> @@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm) >>> u8 data[info->regs->regs_count]; >>> int ret; >>> >>> - if (info->device_type == S2MPS15X || info->device_type == S2MPS14X || >>> - info->device_type == S2MPS13X) { >>> + if (info->regs->read_time_udr_mask) { >>> ret = regmap_update_bits(info->regmap, >>> info->regs->udr_update, >>> - S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK); >>> + info->regs->read_time_udr_mask, >>> + info->regs->read_time_udr_mask); >>> if (ret) { >>> dev_err(dev, >>> "Failed to prepare registers for time reading: %d\n", >>> @@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pdev) >>> >>> switch (platform_get_device_id(pdev)->driver_data) { >>> case S2MPS15X: >>> + regmap_cfg = &s2mps14_rtc_regmap_config; >>> + info->regs = &s2mps15_rtc_regs; >>> + alarm_irq = S2MPS14_IRQ_RTCA0; >>> + break; >>> case S2MPS14X: >>> + regmap_cfg = &s2mps14_rtc_regmap_config; >>> + info->regs = &s2mps14_rtc_regs; >>> + alarm_irq = S2MPS14_IRQ_RTCA0; >>> + break; >>> case S2MPS13X: >>> regmap_cfg = &s2mps14_rtc_regmap_config; >>> - info->regs = &s2mps_rtc_regs; >>> + info->regs = &s2mps13_rtc_regs; >>> alarm_irq = S2MPS14_IRQ_RTCA0; >>> break; >>> case S5M8763X: >>> diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/rtc.h >>> index a65e4655d470..af0df5f3f651 100644 >>> --- a/include/linux/mfd/samsung/rtc.h >>> +++ b/include/linux/mfd/samsung/rtc.h >>> @@ -105,6 +105,8 @@ enum s2mps_rtc_reg { >>> #define S5M_RTC_UDR_MASK (1 << S5M_RTC_UDR_SHIFT) >>> #define S2MPS_RTC_WUDR_SHIFT 4 >>> #define S2MPS_RTC_WUDR_MASK (1 << S2MPS_RTC_WUDR_SHIFT) >>> +#define S2MPS15_RTC_AUDR_SHIFT 4 >>> +#define S2MPS15_RTC_AUDR_MASK (4 << S2MPS15_RTC_AUDR_SHIFT) >> >> Shouldn't be ? >> +#define S2MPS15_RTC_AUDR_MASK (1 << S2MPS15_RTC_AUDR_SHIFT) >> >> because "As per UM, for write alarm, set A_UDR(bit[4]) to high" >> > > Nope. :) You wrote set bit[4], not bit[1]. Oh damn, you are right. The value should be '1'. Shame on me... Thanks for pointing this out. BR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/