Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754160AbbL3DiI (ORCPT ); Tue, 29 Dec 2015 22:38:08 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:27150 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753247AbbL3DiG (ORCPT ); Tue, 29 Dec 2015 22:38:06 -0500 X-AuditID: cbfec7f5-f79b16d000005389-bc-5683519a5f73 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> 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: <56835199.50402@samsung.com> Date: Wed, 30 Dec 2015 12:38:01 +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: Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrELMWRmVeSWpSXmKPExsVy+t/xa7qzApvDDE5/lrFYcvEqu0XHtcVM Fg/mbWOzeP3C0OL+16OMFpd3zWGzmHF+H5PF/s4ORouLK74wWcz93cjqwOXxZNNFRo+ds+6y e+yZeJLN4861PWwefVtWMXpMn/eTyePzJrkA9igum5TUnMyy1CJ9uwSujA1Ld7EWfA6s+NE8 ib2Bsd2ii5GTQ0LARGLKzeWMELaYxIV769m6GLk4hASWMkqcfHADynnKKDFhw2s2kCphgTSJ L/d/s4DYIgIGEhOXzGOFKDrFKHFuzykwh1ngDpPEhs4jYHPZBIwlNi9fAtbNK6Ah8W7NJ+Yu Rg4OFgFViTNrdEFMUYEIiUU7MiEqBCV+TL4HNp9TIFhiwb8/TCAlzALqElOm5IKEmQXkJTav ecs8gVFgFpKOWQhVs5BULWBkXsUomlqaXFCclJ5rpFecmFtcmpeul5yfu4kREhNfdzAuPWZ1 iFGAg1GJh/eEUHOYEGtiWXFl7iFGCQ5mJRHeTC2gEG9KYmVValF+fFFpTmrxIUZpDhYlcd6Z u96HCAmkJ5akZqemFqQWwWSZODilGhirZNwiuNVUjXM35Je+fXdDLq7cWdHzrmtjiQfr/koH 1bTr3f98L7tKbTnidGbLhclc71PEmhastm08tyP69tY4fUHFbKWQ4D8O1sJ/vkx/aVh5Vtb8 1tE9h53Fins2v+gMXX2Fbc7qJhO12aalIXdZLr773TbZcW8u06t9iiL2fEcfrl/530SJpTgj 0VCLuag4EQCSUnvchQIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10484 Lines: 249 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]. Actually that is the funny part for S2MPS15. The logic is similar to other devices: WUDR for time update and AUDR for for alarm update but... The bits are opposite between S2MPS11/13/14 and S2MPS15. Thanks for looking and reviewing! Best regards, Krzysztof -- 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/