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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260131000350d1fac76c@mail.local>
Date: Sat, 31 Jan 2026 01:03:50 +0100
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Fredrik M Olsson <fredrik.m.olsson@...s.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Nobuhiro Iwamatsu <nobuhiro.iwamatsu.x90@...l.toshiba>,
	linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel@...s.com
Subject: Re: [PATCH 3/4] rtc: ds1307: Add Driver for Epson RX8901CE

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