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

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

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.

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


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