Re: [PATCH] rtc: pcf85063: disable the clkout output by default

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Unconditionally gating the CLKOUT clock during probe overrides the hardware power-on state and breaks Device Tree ABI backward compatibility.
- [Low] The patch introduces a compilation failure because the variable `ret` is undeclared, and returning it directly causes a type mismatch.

Pre-existing issues:
- [High] The driver's clock implementation violates the Common Clock Framework (CCF) contract by implicitly enabling the clock during set_rate and discarding the configured rate after unprepare.
- [High] The clock provider registered via `of_clk_add_provider` is never removed, causing a memory leak and a dangling pointer on driver unbind, despite the commit message claiming otherwise.
--

--- Patch [1]: [PATCH] rtc: pcf85063: disable the clkout output by default ---
commit f0b8f2118b75da5ed86252e825d0385a7e1fe816
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

rtc: pcf85063: disable the clkout output by default

This patch gates the PCF85063 CLKOUT pin by default before registering the
OF provider to save power, unless a consumer explicitly claims it.

> Therefore gate CLKOUT in the driver before registering the OF provider,
> mirroring the pcf8563 and hym8563 siblings which already force an off state
> at registration. Doing it before devm_of_clk_add_hw_provider() leaves the
> real consumers in the device tree free to re-enable the output afterwards,
> while an otherwise unused CLKOUT now stays off.

[Severity: High]
This is a pre-existing issue, but the commit message mentions using
devm_of_clk_add_hw_provider(), which contradicts the actual code. The code
still uses of_clk_add_provider() without a corresponding removal, which
leads to a dangling pointer and memory leak if the driver is unbound:

drivers/rtc/rtc-pcf85063.c:pcf85063_clkout_register_clk() {
    ...
    if (!IS_ERR(clk))
        of_clk_add_provider(node, of_clk_src_simple_get, clk);
    ...
}

Since devm_clk_register() automatically frees the clock allocation on
unbind, but of_clk_del_provider() is never called, the global
of_clk_providers list will contain a pointer to freed memory. Should the
driver be updated to actually use devm_of_clk_add_hw_provider() to ensure
proper cleanup?

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index 8cb9ffc73f6d9..9ccfc893be8d3 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -523,6 +523,12 @@ static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
>  	/* optional override of the clockname */
>  	of_property_read_string(node, "clock-output-names", &init.name);
>  
> +	/* power-on default is the 32768 Hz output on; gate it until claimed */
> +	ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> +				 PCF85063_REG_CLKO_F_MASK, PCF85063_REG_CLKO_F_OFF);

[Severity: Critical]
Does unconditionally gating the CLKOUT clock during probe break Device Tree
ABI backward compatibility? Legacy hardware platforms might rely on the
default active 32.768 kHz clock output without explicitly defining a clock
consumer in their device tree. Will those systems lose their clock signal
when booting a kernel with this patch?

> +	if (ret)
> +		return ret;

[Severity: Low]
Does this code compile with CONFIG_COMMON_CLK enabled? It looks like the
variable ret is undeclared in pcf85063_clkout_register_clk(). Also, since
the function returns a struct clk pointer, wouldn't returning an integer
error code directly cause a type mismatch?

[Severity: High]
This isn't a bug introduced by this patch, but does the current clock
implementation violate the Common Clock Framework contract?

Looking at pcf85063_clkout_set_rate(), setting a rate implicitly enables
the clock by modifying the hardware register directly:

drivers/rtc/rtc-pcf85063.c:pcf85063_clkout_set_rate() {
    ...
    return regmap_update_bits(pcf85063->regmap,
                              PCF85063_REG_CTRL2,
                              PCF85063_REG_CLKO_F_MASK, i);
}

Furthermore, if a consumer disables the clock and later re-enables it,
pcf85063_clkout_control() unconditionally resets the rate to 32768 Hz,
discarding any previously configured custom rates:

drivers/rtc/rtc-pcf85063.c:pcf85063_clkout_control() {
    ...
    if (enable) {
        if (buf == PCF85063_REG_CLKO_F_OFF)
            buf = PCF85063_REG_CLKO_F_32768HZ;
    ...
}

Will this cause custom clock rates set by consumers or device tree to be
silently lost across enable/disable cycles?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824080801.709764-1-alexander.sverdlin@siemens.com?part=1