Re: [PATCH v3 4/4] rtc: pcf8525: Add temperature sensor support via hwmon

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The RTC driver registers a hardware monitoring device directly instead of using an auxiliary device driver, violating HWMON subsystem layering guidelines.
- [Medium] If hwmon device registration fails, the analog temperature sensor is left permanently enabled, causing unnecessary backup battery drain.
- [Medium] The analog temperature sensor is not disabled during system suspend, causing continuous backup battery drain.
--

commit 8de48a2b8b05df0e0fd29e3c00baaf40c4a06d38
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
Date:   Thu Sep 10 06:09:12 2026 +0000

rtc: pcf8525: Add temperature sensor support via hwmon

This commit adds hardware monitoring support for the PCF8525 internal
temperature sensor. It registers the sensor via the hwmon interface to
expose temperature reading and update interval configuration.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index c2eab983f1ff0..a2958eb3a5026 100644
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c

[ ... ]

> +static void pcf8525_hwmon_register(struct device *dev,
> +				   struct pcf8525 *pcf8525)
> +{
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_RTC_DRV_PCF8525_HWMON))
> +		return;
> +
> +	/*
> +	 * Enable digital temperature readout first so that the hardware is
> +	 * ready before the sysfs attributes are exposed to userspace.  The
> +	 * previous order (register hwmon device first) left a window where
> +	 * a concurrent sysfs read would return invalid data, and left the
> +	 * device permanently broken if this step subsequently failed.
> +	 */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
> +				 PCF8525_CTRL5_TEMP_RD_EN,
> +				 PCF8525_CTRL5_TEMP_RD_EN);

[Severity: Medium]
Once the temperature sensor is enabled here, it stays active for the lifetime
of the device. Looking at pcf8525_suspend() in this file, it does not clear
the PCF8525_CTRL5_TEMP_RD_EN bit.

Does the analog temperature sensor need to be disabled during system suspend
to prevent continuous backup battery drain while the system is asleep?

> +	if (ret) {
> +		dev_warn(dev, "failed to enable temperature readout: %d\n", ret);
> +		return;
> +	}
> +
> +	/*
> +	 * Register a devm cleanup action to disable TEMP_RD_EN when the
> +	 * driver is unbound, preventing the analog sensor from draining
> +	 * the backup battery after driver removal.
> +	 */
> +	if (devm_add_action_or_reset(dev, pcf8525_hwmon_disable_temp, pcf8525)) {
> +		dev_warn(dev, "failed to register HWMON cleanup action\n");
> +		return;
> +	}
> +
> +	/* Only expose sysfs attributes once the hardware is fully ready. */
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "pcf8525",
> +							 pcf8525,
> +							 &pcf8525_hwmon_chip_info,
> +							 NULL);

[Severity: Medium]
According to the HWMON subsystem guidelines, shouldn't hardware monitoring
functionality for multi-function devices be implemented as an auxiliary device
driver residing in drivers/hwmon/ instead of being registered directly from
drivers/rtc/?

> +	if (IS_ERR(hwmon_dev))
> +		dev_warn(dev, "failed to register HWMON device: %ld\n",
> +			 PTR_ERR(hwmon_dev));

[Severity: Medium]
If hwmon device registration fails here, it only logs a warning and returns.

Since the probe function ignores this failure and succeeds, the device remains
bound. Does this mean the devm cleanup action setup earlier will not execute
until device removal, leaving the analog temperature sensor permanently
enabled and unnecessarily draining the backup battery if hwmon registration
fails?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910060912.2443551-1-shivprakash.gupta@nxp.com?part=4