Re: [PATCH 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

From: Yadwinder Singh Brar
Date: Tue Dec 29 2015 - 21:53:53 EST

Next message: Gang He: "Re: [PATCH v3 0/4] Add online file check feature"
Previous message: Eric W. Biederman: "Re: nf_unregister_net_hook: hook not found!"
In reply to: Krzysztof Kozlowski: "[PATCH 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krysztof,

On Tue, Dec 29, 2015 at 5:53 PM, Krzysztof Kozlowski
<k.kozlowski@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Gang He: "Re: [PATCH v3 0/4] Add online file check feature"
Previous message: Eric W. Biederman: "Re: nf_unregister_net_hook: hook not found!"
In reply to: Krzysztof Kozlowski: "[PATCH 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]