Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 20 Jan 2023 18:09:41 +0100
From:   Alexandre Belloni <alexandre.belloni@bootlin.com>
To:     Hugo Villeneuve <hugo@hugovil.com>
Cc:     a.zummo@towertech.it, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-rtc@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Hugo Villeneuve <hvilleneuve@dimonoff.com>
Subject: Re: [PATCH v3 11/14] rtc: pcf2127: adapt time/date registers write
 sequence for PCF2131
Message-ID: <Y8rK1dgpNJaSy/Gb@mail.local>
References: <20221215150214.1109074-1-hugo@hugovil.com>
 <20221215150214.1109074-12-hugo@hugovil.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221215150214.1109074-12-hugo@hugovil.com>
Precedence: bulk

On 15/12/2022 10:02:12-0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> The sequence for updating the time/date registers is slightly
> different between PCF2127/29 and PCF2131.
> 
> For PCF2127/29, during write operations, the time counting
> circuits (memory locations 03h through 09h) are automatically blocked.
> 
> For PCF2131, time/date registers write access requires setting the
> STOP bit and sending the clear prescaler instruction (CPR). STOP then
> needs to be released once write operation is completed.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/rtc/rtc-pcf2127.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index e4b78b9c03f9..11fbdab6bf01 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -39,6 +39,7 @@
>  #define PCF2127_REG_CTRL1		0x00
>  #define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
>  #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
> +#define PCF2127_BIT_CTRL1_STOP			BIT(5)
>  /* Control register 2 */
>  #define PCF2127_REG_CTRL2		0x01
>  #define PCF2127_BIT_CTRL2_AIE			BIT(1)
> @@ -70,6 +71,7 @@
>  #define PCF2131_REG_SR_RESET		0x05
>  #define PCF2131_SR_RESET_READ_PATTERN	0b00100100 /* Fixed pattern. */
>  #define PCF2131_SR_RESET_RESET_CMD	0x2C /* SR is bit 3. */
> +#define PCF2131_SR_RESET_CPR_CMD	0xA4 /* CPR is bit 7. */
>  /* Time and date registers */
>  #define PCF2127_REG_TIME_DATE_BASE	0x03
>  #define PCF2131_REG_TIME_DATE_BASE	0x07 /* Register 0x06 is 100th seconds,
> @@ -307,7 +309,31 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	/* year */
>  	buf[i++] = bin2bcd(tm->tm_year - 100);
>  
> -	/* write register's data */
> +	/* Write access to time registers:
> +	 * PCF2127/29: no special action required.
> +	 * PCF2131:    requires setting the STOP bit. STOP bit needs to
> +	 *             be cleared after time registers are updated.
> +	 *             It is also recommended to set CPR bit, although
> +	 *             write access will work without it.
> +	 */
> +	if (pcf2127->cfg->has_reset_reg) {

This should probably be tied to the actual rtc model rather than the
presence of the reset register.
You MUST clear CPR to be able to set the time precisely.

> +		err = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
> +					 PCF2127_BIT_CTRL1_STOP,
> +					 PCF2127_BIT_CTRL1_STOP);
> +		if (err) {
> +			dev_err(dev, "setting STOP bit failed\n");

This really needs to be less verbose. There is nothing a user can really
do after having seen this message. Having an error in userspace will
anyway prompt the user to retry the operation which is the only action
it can do.

> +			return err;
> +		}
> +
> +		err = regmap_write(pcf2127->regmap, pcf2127->cfg->reg_reset,
> +				   PCF2131_SR_RESET_CPR_CMD);
> +		if (err) {
> +			dev_err(dev, "sending CPR cmd failed\n");
> +			return err;
> +		}
> +	}
> +
> +	/* write time register's data */
>  	err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
>  	if (err) {
>  		dev_err(dev,
> @@ -315,6 +341,16 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  		return err;
>  	}
>  
> +	if (pcf2127->cfg->has_reset_reg) {
> +		/* Clear STOP bit (PCF2131 only) after write is completed. */
> +		err = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
> +					 PCF2127_BIT_CTRL1_STOP, 0);
> +		if (err) {
> +			dev_err(dev, "clearing STOP bit failed\n");
> +			return err;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.30.2
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com