Re: [PATCH 2/3] rtc: pcf2127: Add support for the clkout clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: pcf2127: rate 0 in pcf2127_clkout_rate2cof() sets COF to HiZ
--

commit fd3d38a37715a1ab67448b3adb80c6b0ee46a7e2
Author: Herve Codina <herve.codina@bootlin.com>

rtc: pcf2127: Add support for the clkout clock

This commit adds common clock framework support for the CLKOUT output clock
on PCF2127, PCF2129, PCA2129, and PCF2131 RTC devices, allowing configuration
of frequencies from 1 Hz to 32768 Hz and disabling the output into HiZ.

> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index ff2023908f5a3..2c442770ebc52 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c

[ ... ]

> @@ -626,6 +634,180 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
>  	return devm_watchdog_register_device(dev, &pcf2127->wdd);
>  }
>  
> +#ifdef CONFIG_COMMON_CLK
> +#define clkout_hw_to_pcf2127(_hw) container_of(_hw, struct pcf2127, clkout_hw)
> +
> +/* Rates indexed by COF values */
> +static int pcf2127_clkout_rates[] = {
> +	32768,
> +	16384,
> +	8192,
> +	4096,
> +	2048,
> +	1024,
> +	1,
> +	0, /* 0Hz (COF 0b111) will put the clockout signal to HiZ */
> +};
> +
> +static u32 pcf2127_clkout_rate2cof(unsigned long rate)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pcf2127_clkout_rates); i++) {
> +		if (pcf2127_clkout_rates[i] <= rate)
> +			return i;
> +	}
> +
> +	return 0; /* Maximum output clock rate */
> +}
> +
> +static int pcf2127_clkout_cof2rate(u32 cof)
> +{
> +	return pcf2127_clkout_rates[cof];
> +}
> +
> +static unsigned long pcf2127_clkout_recalc_rate(struct clk_hw *hw,
> +						unsigned long parent_rate)
> +{
> +	struct pcf2127 *pcf2127 = clkout_hw_to_pcf2127(hw);
> +
> +	return pcf2127_clkout_cof2rate(pcf2127->clkout_cof);
> +}
> +
> +static int pcf2127_clkout_determine_rate(struct clk_hw *hw,
> +					 struct clk_rate_request *req)
> +{
> +	u32 cof;
> +
> +	cof = pcf2127_clkout_rate2cof(req->rate);
> +	req->rate = pcf2127_clkout_cof2rate(cof);
> +
> +	return 0;
> +}
> +
> +static int pcf2127_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> +				   unsigned long parent_rate)
> +{
> +	struct pcf2127 *pcf2127 = clkout_hw_to_pcf2127(hw);
> +	u32 cof;
> +	int ret;
> +
> +	cof = pcf2127_clkout_rate2cof(rate);
> +
> +	/*
> +	 * Setting COF will enable the clock (i.e. output not in HiZ anymore)
> +	 * Perform the operation only if already enabled.
> +	 * Otherwise, do nothing. The expected COF will be set on prepare()
> +	 */
> +	if (pcf2127->clkout_enabled) {
> +		ret = regmap_update_bits(pcf2127->regmap, pcf2127->cfg->reg_clkout,
> +					 PCF2127_CLKOUT_COF,
> +					 FIELD_PREP(PCF2127_CLKOUT_COF, cof));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	pcf2127->clkout_cof = cof;
> +	return 0;
> +}

[Severity: Medium]
Since rate is an unsigned long and pcf2127_clkout_rates[7] is 0, the condition
pcf2127_clkout_rates[i] <= rate in pcf2127_clkout_rate2cof() will always match
at index 7 (PCF2127_CLKOUT_COF_HIZ) when rate is 0, making the fallback
return 0 unreachable.

If a consumer passes rate == 0 to pcf2127_clkout_determine_rate() or
pcf2127_clkout_set_rate(), pcf2127_clkout_rate2cof(0) returns 7
(PCF2127_CLKOUT_COF_HIZ). In pcf2127_clkout_set_rate(), this writes
PCF2127_CLKOUT_COF_HIZ to reg_clkout if pcf2127->clkout_enabled is true
(putting the CLKOUT pin into HiZ while pcf2127->clkout_enabled remains true),
and caches PCF2127_CLKOUT_COF_HIZ in pcf2127->clkout_cof so subsequent calls
to pcf2127_clkout_prepare() -> pcf2127_clkout_control(hw, true) will also
leave the pin in HiZ.

Should the 0 entry be removed from pcf2127_clkout_rates[] (or skipped in
pcf2127_clkout_rate2cof()) so that pcf2127->clkout_cof only ever holds active
clock divider values (0..6)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102130712.25104-1-herve.codina@bootlin.com?part=2