Re: [PATCH v5 2/2] rtc: Add Raspberry Pi 5 RTC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rtc: raspberrypi: Missing endianness conversion in firmware communication
- [Medium] rtc: raspberrypi: Incorrect DT node for trickle voltage property
- [Medium] rtc: raspberrypi: Missing MODULE_ALIAS prevents autoloading
--

commit 2afa604e9b62ac37552a0a69c5399fed7850a086
Author: Sander Speetjens <sander.speetjens@gmail.com>

rtc: Add Raspberry Pi 5 RTC driver

Upstreaming the downstream Raspberry Pi 5 RTC driver. This driver supports
the custom DA9091, which is accessed through the firmware mailbox.

> diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
> new file mode 100644
> index 0000000000000..494b949374064
> --- /dev/null
> +++ b/drivers/rtc/rtc-raspberrypi.c
[ ... ]
> +static int rpi_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = { RTC_TIME };
> +	int err;
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +				    &data, sizeof(data));
> +	rtc_time64_to_tm(data[1], tm);

[Severity: High]
Will this driver run correctly on big-endian kernels?

The raw payload returned by the firmware is treated as host-endian here. If
the firmware expects and returns little-endian payloads (as is common for
mailbox interfaces), should this code use le32_to_cpu() to prevent byte
transposition issues?

> +	return err;
> +}
> +
> +static int rpi_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = { RTC_TIME, rtc_tm_to_time64(tm) };

[Severity: High]
Similar to rpi_rtc_read_time(), the timestamp and sub-command are placed
into a host-endian u32 array without conversion.

Should this value be converted to little-endian using cpu_to_le32() before
sending it to the firmware?

> +
> +	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				     &data, sizeof(data));
> +}
[ ... ]
> +static int rpi_rtc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	vrtc->rtc->ops = &rpi_rtc_ops;
> +
> +	rpi_rtc_alarm_clear_pending(dev);
> +
> +	vrtc->bbat_vchg_millivolts = 0;
> +	of_property_read_u32(dev->parent->of_node, "trickle-voltage-millivolt",
> +			     &vrtc->bbat_vchg_millivolts);

[Severity: Medium]
Does this read the property from the correct device tree node?

It appears this reads trickle-voltage-millivolt from the parent firmware
node (dev->parent->of_node) instead of the child RTC node. If a correct
device tree specifies this property on the RTC node to follow DT encapsulation
rules, will that configuration be ignored here?

> +
> +	rpi_rtc_set_limits(dev);
> +	rpi_rtc_set_charge_voltage(dev);
> +
> +	return devm_rtc_register_device(vrtc->rtc);
> +}
> +
> +static const struct of_device_id rpi_rtc_dt_match[] = {
> +	{ .compatible = "raspberrypi,firmware-rtc" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rpi_rtc_dt_match);
> +
> +static struct platform_driver rpi_rtc_driver = {
> +	.driver = {
> +		.name = "raspberrypi-rtc",
> +		.of_match_table = of_match_ptr(rpi_rtc_dt_match),
> +	},
> +	.probe = rpi_rtc_probe
> +};

[Severity: Medium]
Will this module autoload correctly on systems using manual registration?

The firmware driver registers a platform device named raspberrypi-rtc, which
generates a platform:raspberrypi-rtc uevent. Without a matching MODULE_ALIAS
declaration here, it seems udev might not be able to match the fallback
device to the driver module.

> +
> +module_platform_driver(rpi_rtc_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924132217.1281752-1-sander.speetjens@gmail.com?part=2