diff mbox series

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

Message ID 20221215150214.1109074-12-hugo@hugovil.com
State New
Headers
Series rtc: pcf2127: add PCF2131 driver |

Commit Message

Hugo Villeneuve Dec. 15, 2022, 3:02 p.m. UTC 
  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(-)

Comments

Bruno Thomsen Jan. 7, 2023, 6:44 p.m. UTC | #1 
Den tor. 15. dec. 2022 kl. 16.19 skrev Hugo Villeneuve <hugo@hugovil.com>:
>
> 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. */

Replace 0xA4 with (BIT(2) | BIT(5) | BIT(7)) or
(PCF2131_SR_RESET_READ_PATTERN | 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) {
> +               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");
> +                       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 Jan. 20, 2023, 5:09 p.m. UTC | #2 
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
>
Hugo Villeneuve Jan. 23, 2023, 8:55 p.m. UTC | #3 
On Sat, 7 Jan 2023 19:44:23 +0100
Bruno Thomsen <bruno.thomsen@gmail.com> wrote:

> Den tor. 15. dec. 2022 kl. 16.19 skrev Hugo Villeneuve <hugo@hugovil.com>:
> >
> > 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. */
> 
> Replace 0xA4 with (BIT(2) | BIT(5) | BIT(7)) or
> (PCF2131_SR_RESET_READ_PATTERN | BIT(7))

Done.

 
> >  /* 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) {
> > +               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");
> > +                       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
> >
>
Hugo Villeneuve Jan. 23, 2023, 9:57 p.m. UTC | #4 
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)?



> 
> > +		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,

???


> > +			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
>
Alexandre Belloni June 21, 2023, 7:36 p.m. UTC | #5 
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>
diff mbox series

Patch

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) {
+		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");
+			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;
 }