Re: [PATCH] rtc-pm8xxx: Fix issue in RTC write path

[Alexandre Belloni <alexandre.belloni@free-electrons.com>]

Hi,

this was not sent to the correct mailingr- list, please use a more recent
kernel ;)

On 28/02/2018 at 14:21:18 +0530, Mohit Aggarwal wrote:
> In order to set time in rtc, need to disable
> rtc hw before writing into rtc registers.
> 
> Also fixes disabling of alarm while setting
> rtc time.
> 
> Change-Id: Ia2c9c922dd78e2ecb92554d5722c894706d83ac0

checkpatch rightfully complains loudly about that line.

> Signed-off-by: Mohit Aggarwal <maggarwa@codeaurora.org>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 49 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index fac8355..5309edc 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -74,16 +74,18 @@ struct pm8xxx_rtc {
>  /*
>   * Steps to write the RTC registers.
>   * 1. Disable alarm if enabled.
> - * 2. Write 0x00 to LSB.
> - * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0].
> - * 4. Enable alarm if disabled in step 1.
> + * 2. Disable rtc if enabled.
> + * 3. Write 0x00 to LSB.
> + * 4. Write Byte[1], Byte[2], Byte[3] then Byte[0].
> + * 5. Enable rtc if disabled in step 2.
> + * 6. Enable alarm if disabled in step 1.
>   */
>  static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  {
>  	int rc, i;
>  	unsigned long secs, irq_flags;
> -	u8 value[NUM_8_BIT_RTC_REGS], alarm_enabled = 0;
> -	unsigned int ctrl_reg;
> +	u8 value[NUM_8_BIT_RTC_REGS], alarm_enabled = 0, rtc_disabled = 0;
> +	unsigned int ctrl_reg, rtc_ctrl_reg;
>  	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
>  	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
>  
> @@ -92,23 +94,38 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  
>  	rtc_tm_to_time(tm, &secs);
>  
> +	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
> +
>  	for (i = 0; i < NUM_8_BIT_RTC_REGS; i++) {
>  		value[i] = secs & 0xFF;
>  		secs >>= 8;
>  	}
>  
> -	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
> -
>  	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
>  
> -	rc = regmap_read(rtc_dd->regmap, regs->ctrl, &ctrl_reg);
> +	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
>  	if (rc)
>  		goto rtc_rw_fail;
>  
>  	if (ctrl_reg & regs->alarm_en) {
>  		alarm_enabled = 1;
>  		ctrl_reg &= ~regs->alarm_en;
> -		rc = regmap_write(rtc_dd->regmap, regs->ctrl, ctrl_reg);
> +		rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
> +		if (rc) {
> +			dev_err(dev, "Write to RTC Alarm control register failed\n");
> +			goto rtc_rw_fail;
> +		}
> +	}
> +
> +	/* Disable RTC H/w before writing on RTC register */
> +	rc = regmap_read(rtc_dd->regmap, regs->ctrl, &rtc_ctrl_reg);
> +	if (rc)
> +		goto rtc_rw_fail;
> +
> +	if (rtc_ctrl_reg & PM8xxx_RTC_ENABLE) {
> +		rtc_disabled = 1;
> +		rtc_ctrl_reg &= ~PM8xxx_RTC_ENABLE;
> +		rc = regmap_write(rtc_dd->regmap, regs->ctrl, rtc_ctrl_reg);
>  		if (rc) {
>  			dev_err(dev, "Write to RTC control register failed\n");
>  			goto rtc_rw_fail;
> @@ -137,11 +154,21 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  		goto rtc_rw_fail;
>  	}
>  
> +	/* Enable RTC H/w after writing on RTC register */
> +	if (rtc_disabled) {
> +		rtc_ctrl_reg |= PM8xxx_RTC_ENABLE;
> +		rc = regmap_write(rtc_dd->regmap, regs->ctrl, rtc_ctrl_reg);
> +		if (rc) {
> +			dev_err(dev, "Write to RTC control register failed\n");
> +			goto rtc_rw_fail;
> +		}
> +	}
> +
>  	if (alarm_enabled) {
>  		ctrl_reg |= regs->alarm_en;
> -		rc = regmap_write(rtc_dd->regmap, regs->ctrl, ctrl_reg);
> +		rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
>  		if (rc) {
> -			dev_err(dev, "Write to RTC control register failed\n");
> +			dev_err(dev, "Write to RTC Alarm control register failed\n");
>  			goto rtc_rw_fail;
>  		}
>  	}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com