| Message ID | 1451440385-8654-3-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Krysztof, On Tue, Dec 29, 2015 at 5:53 PM, Krzysztof Kozlowski <k.kozlowski@samsung.com> 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 <k.kozlowski@samsung.com> > > --- > > 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" Regards, Yadwinder > #define S2MPS13_RTC_AUDR_SHIFT 1 > #define S2MPS13_RTC_AUDR_MASK (1 << S2MPS13_RTC_AUDR_SHIFT) > #define S2MPS15_RTC_WUDR_SHIFT 1 > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30.12.2015 11:53, Yadwinder Singh Brar wrote: > Hi Krysztof, > > On Tue, Dec 29, 2015 at 5:53 PM, Krzysztof Kozlowski > <k.kozlowski@samsung.com> 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 <k.kozlowski@samsung.com> >> >> --- >> >> 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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> <k.kozlowski@samsung.com> 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 <k.kozlowski@samsung.com> >>> >>> --- >>> >>> 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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) #define S2MPS13_RTC_AUDR_SHIFT 1 #define S2MPS13_RTC_AUDR_MASK (1 << S2MPS13_RTC_AUDR_SHIFT) #define S2MPS15_RTC_WUDR_SHIFT 1
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 <k.kozlowski@samsung.com> --- 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(-)