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

[Fredrik M Olsson <fredriol@axis.com>]

On 1/31/26 01:03, Alexandre Belloni wrote:
> [Some people who received this message don't often get email from alexandre.belloni@bootlin.com. 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