Re: [PATCH v3 11/14] rtc: pcf2127: adapt time/date registers write sequence for PCF2131

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 23/01/2023 16:57:41-0500, Hugo Villeneuve wrote:
> On Fri, 20 Jan 2023 18:09:41 +0100
> Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> 
> > 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.
> 
> In fact you must actually SET the CPR bit to clear the prescaler, confusing!
> 
> I was already setting the CPR bit (clearing prescaler), so I modified the confusing comment.
> 
> The CPR bit is only present IF the reset register is also present, that is why I simply used the presence of the reset register to take the correct action. This avoids to define a new bit or matching on a device model for that functionality (adding newer models could potentially mean modifying the model match).
> 
> But if you absolutely want to match on the model, I would like to know how you would like to practically do it (maybe an example)?
> 

You can keep pcf21xx_type around, in pcf21xx_config for example.

> 
> 
> > 
> > > +		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.
> 
> I converted the dev_err messages to dev_dbg.
> 
> In the original driver and in the same function, there is also a dev_err to handle regmap_bulk_write() failure. Do you suggest that we also make it less verbose:
> 
> err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->reg_time_base, buf, i);
>  	if (err) {
>  		dev_err(dev,
> 
> ???

yes, you can remove it as part of your previous patches.

> 
> 
> > > +			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
> > 
> 
> 
> -- 
> Hugo Villeneuve <hugo@hugovil.com>

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