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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] rtc: unconditional dev_err on unsupported platforms
- [Medium] rtc: dt property name mismatch with schema and standard
- [Medium] rtc: ignored devm_device_init_wakeup return value
- [Low] kconfig: missing dependency on RASPBERRYPI_FIRMWARE
--

commit 63b49baa5affb33b58f7190d1f915c0156fa6365
Author: Sander Speetjens <sander.speetjens@gmail.com>

rtc: Add Raspberry Pi 5 RTC driver

This commit upstream supports the Raspberry Pi 5 RTC by interacting with
the custom DA9091 over the firmware mailbox. It binds as a child of the
firmware device and configures trickle charging based on device tree
settings.

> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 0aa322e9a2e73..b284c689f7828 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
[ ... ]
> @@ -231,6 +232,21 @@ static void rpi_register_clk_driver(struct device *dev)
>  						-1, NULL, 0);
>  }
>  
> +static void rpi_register_rtc_driver(struct device *dev)
> +{
> +	struct device_node *firmware;
> +
> +	firmware = of_get_compatible_child(dev->of_node,
> +					   "raspberrypi,firmware-rtc");
> +	if (firmware) {
> +		of_node_put(firmware);
> +		return;
> +	}
> +
> +	rpi_rtc = platform_device_register_data(dev, "raspberrypi-rtc",
> +						-1, NULL, 0);

[Severity: Medium]
Will this unconditionally register the rtc platform device on legacy
Raspberry Pi models?

If rpi_rtc is registered on all models, the probe function will run
universally. When rpi_rtc_probe() executes on a model other than the
Raspberry Pi 5, it prints an error message via dev_err() and aborts:

    if (!model || strncmp(model, "Raspberry Pi 5", 14) != 0) {
            dev_err(dev, "RTC is only available on Raspberry Pi 5\n");
            return -ENODEV;
    }

This seems like it would produce a spurious error message in the kernel log
on every boot for all older models where the hardware doesn't exist.

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 05b9233b94189..382973d46e06d 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
[ ... ]
> @@ -1999,6 +1999,17 @@ config RTC_DRV_R7301
>  	   This driver can also be built as a module. If so, the module
>  	   will be called rtc-r7301.
>  
> +config RTC_DRV_RPI
> +        tristate "Raspberry Pi RTC"
> +        depends on ARCH_BRCMSTB || COMPILE_TEST
> +        default ARCH_BRCMSTB

[Severity: Low]
Does this configuration allow RTC_DRV_RPI to be built-in when
RASPBERRYPI_FIRMWARE is built as a module?

The driver calls devm_rpi_firmware_get() and rpi_firmware_property(),
which are exported by the raspberrypi-firmware module. If the RTC driver
is built-in and the firmware driver is a module, this will result in an
undefined reference linker error. Should there be a dependency on
RASPBERRYPI_FIRMWARE?

> diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
> new file mode 100644
> index 0000000000000..af775ae55a871
> --- /dev/null
> +++ b/drivers/rtc/rtc-raspberrypi.c
[ ... ]
> +static int rpi_rtc_probe(struct platform_device *pdev)
> +{
> +	struct rpi_rtc_data *vrtc;
> +	struct device *dev = &pdev->dev;
> +	struct rpi_firmware *firmware;
> +	int ret;
[ ... ]
> +	vrtc = devm_kzalloc(dev, sizeof(*vrtc), GFP_KERNEL);
> +	if (!vrtc)
> +		return -ENOMEM;
> +
> +	vrtc->fw = firmware;
> +
> +	devm_device_init_wakeup(dev);

[Severity: Medium]
Should the return value of devm_device_init_wakeup() be checked here?

This function registers a devres action and can fail, for instance, due to
an out of memory condition. By ignoring the integer error code, the driver
might continue with an uninitialized or broken wakeup state on failure.

> +
> +	platform_set_drvdata(pdev, vrtc);
[ ... ]
> +	rpi_rtc_alarm_clear_pending(dev);
> +
> +	vrtc->bbat_vchg_millivolts = 0;
> +	of_property_read_u32(dev->parent->of_node, "trickle-charge-millivolt",
> +			&vrtc->bbat_vchg_millivolts);

[Severity: Medium]
Does this device tree property name match the binding schemas?

The standard RTC bindings typically use trickle-voltage-millivolt, and the
driver's own schema might define charging-voltage-millivolt, but this
attempts to read trickle-charge-millivolt. If a device tree follows the
standard schema, this property lookup will fail and leave trickle charging
silently disabled.

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