From: Hugo Villeneuve <[email protected]>
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 <[email protected]>
---
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) {
+ 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
Den tor. 15. dec. 2022 kl. 16.19 skrev Hugo Villeneuve <[email protected]>:
>
> From: Hugo Villeneuve <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
On 15/12/2022 10:02:12-0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Sat, 7 Jan 2023 19:44:23 +0100
Bruno Thomsen <[email protected]> wrote:
> Den tor. 15. dec. 2022 kl. 16.19 skrev Hugo Villeneuve <[email protected]>:
> >
> > From: Hugo Villeneuve <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
On Fri, 20 Jan 2023 18:09:41 +0100
Alexandre Belloni <[email protected]> wrote:
> On 15/12/2022 10:02:12-0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
>
--
Hugo Villeneuve <[email protected]>
On 23/01/2023 16:57:41-0500, Hugo Villeneuve wrote:
> On Fri, 20 Jan 2023 18:09:41 +0100
> Alexandre Belloni <[email protected]> wrote:
>
> > On 15/12/2022 10:02:12-0500, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com