Re: [PATCH v11 3/3] rtc: Add Raspberry Pi 5 RTC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] rtc: rpi: missing alarm pending clear outside probe
- [Low] core: Unused variable `firmware` in rpi_register_rtc_driver()
--

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

rtc: Add Raspberry Pi 5 RTC driver

This commit upstreams the downstream Raspberry Pi 5 RTC driver. It adds
support for the custom DA9091, which is accessed through the firmware mailbox.

> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 0aa322e9a2e73..b22c59a429dea 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
[ ... ]
> @@ -231,6 +232,27 @@ static void rpi_register_clk_driver(struct device *dev)
>  						-1, NULL, 0);
>  }
>  
> +static void rpi_register_rtc_driver(struct device *dev)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *firmware;

[Severity: Low]
Is the firmware variable needed here? It appears to be declared but
never used in rpi_register_rtc_driver(), which will trigger a
-Wunused-variable compiler warning and fail builds configured with -Werror.

> +
> +	/*
> +	 * Check if our model of Raspberry Pi board is from the 5th gen
> +	 * as the RTC is only present on that model.
> +	 */
> +	if (!of_machine_is_compatible("brcm,bcm2712"))
> +		return;
[ ... ]
> diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
> new file mode 100644
> index 0000000000000..6d5c8111f4db6
> --- /dev/null
> +++ b/drivers/rtc/rtc-raspberrypi.c
[ ... ]
> +static int rpi_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	struct rpi_rtc_data *rtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_ALARM),
> +		.val = cpu_to_le32(rtc_tm_to_time64(&alarm->time))
> +	};
> +	int err;
> +
> +	err = rpi_firmware_property(rtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				    &data, sizeof(data));
> +
> +	if (!err)
> +		err = rpi_rtc_alarm_irq_enable(dev, alarm->enabled);

[Severity: Medium]
Should we explicitly clear the alarm pending bit when setting a new alarm?

rpi_rtc_alarm_clear_pending() is called once during rpi_rtc_probe(), but
the pending state does not seem to be cleared when new alarms are set
here or during a resume path. If an alarm triggers while the system is
awake (or if the system uses S2idle/S3 suspend where probe is not re-run),
could the lingering pending bit prevent subsequent wake alarms from firing
or cause immediate spurious wakeups?

> +
> +	return err;
> +}

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