Re: [PATCH 3/4] rtc: ds1307: Add Driver for Epson RX8901CE

[Fredrik M Olsson]

On 1/31/26 01:03, Alexandre Belloni wrote:
> [Some people who received this message don't often get email from alexandre.belloni@xxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hello,
>
> On 19/12/2025 13:10:37+0100, Fredrik M Olsson wrote:
> >   #define MCP794XX_REG_CONTROL         0x07
> >   #    define MCP794XX_BIT_ALM0_EN     0x10
> >   #    define MCP794XX_BIT_ALM1_EN     0x20
> > @@ -226,6 +233,19 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> >                        dev_warn_once(dev, "oscillator failed, set time!\n");
> >                        return -EINVAL;
> >                }
> > +     } else if (ds1307->type == rx_8901) {
> > +             unsigned int regflag;
> > +
> > +             ret = regmap_read(ds1307->regmap, RX8901_REG_INTF, &regflag);
> > +             if (ret) {
> > +                     dev_err(dev, "%s error %d\n", "read", ret);
>
> The multiple dev_err you are adding should be dev_dbg as there is no
> other actions for the user than to restart the operation when it fails
> so there is not actual value apart when debugging.

Okay thanks I will update that for v2.

> > +                     return ret;
> > +             }
> > +
> > +             if (regflag & RX8901_REG_INTF_VLF) {
> > +                     dev_warn_once(dev, "oscillator failed, set time!\n");
> > +                     return -EINVAL;
> > +             }
> >        }
> >
> >        /* read the RTC date and time registers all at once */
> > @@ -307,7 +327,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> >        tmp = regs[DS1307_REG_HOUR] & 0x3f;
> >        t->tm_hour = bcd2bin(tmp);
> >        /* rx8130 is bit position, not BCD */
> > -     if (ds1307->type == rx_8130)
> > +     if (ds1307->type == rx_8130 || ds1307->type == rx_8901)
> >                t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f) - 1;
> >        else
> >                t->tm_wday = bcd2bin(regs[DS1307_REG_WDAY] & 0x07) - 1;
> > @@ -358,7 +378,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
> >        regs[DS1307_REG_MIN] = bin2bcd(t->tm_min);
> >        regs[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
> >        /* rx8130 is bit position, not BCD */
> > -     if (ds1307->type == rx_8130)
> > +     if (ds1307->type == rx_8130 || ds1307->type == rx_8901)
> >                regs[DS1307_REG_WDAY] = 1 << t->tm_wday;
> >        else
> >                regs[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
> > @@ -422,6 +442,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
> >                        dev_err(dev, "%s error %d\n", "write", result);
> >                        return result;
> >                }
> > +     } else if (ds1307->type == rx_8901) {
> > +             /*
> > +              * clear Voltage Loss Flag as data is available now (writing 1
> > +              * to the other bits in the INTF register has no effect)
> > +              */
> > +             result = regmap_write(ds1307->regmap, RX8901_REG_INTF,
> > +                                   0xff ^ RX8901_REG_INTF_VLF);
> > +             if (result) {
> > +                     dev_err(dev, "%s error %d\n", "write", result);
> > +                     return result;
> > +             }
> >        }
> >
> >        return 0;
> > @@ -568,6 +599,17 @@ static u8 do_trickle_setup_rx8130(struct ds1307 *ds1307, u32 ohms, bool diode)
> >        return setup;
> >   }
> >
> > +static u8 do_trickle_setup_rx8901(struct ds1307 *ds1307, u32 ohms, bool diode)
> > +{
> > +     /* make sure that the backup battery is enabled */
> > +     u8 setup = RX8901_REG_PWSW_CFG_INIEN;
>
> You can't do this as this will cause issues in the future for people
> wanting to keep this bit disabled (the main reason being that you don't
> want to draw power from the battery while your device sits on a shelf).
> You have to handle the RTC_PARAM_BACKUP_SWITCH_MODE parameter and then
> switch it from userspace.

Here my intention was to do something similar to what was done before 
for the rx8130 device [1], which as I understand it require similar 
setup initialization as the rx8901 device does. Is using the 
RTC_PARAM_BACKUP_SWITCH_MODE the new way of performing this initialization?

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


[1] 0026f1604c9b (rtc: ds1307: enable rx8130's backup battery, make it 
chargeable optionally)


--
/Fredrik