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

[Herve Codina <herve.codina@bootlin.com>]

On Mon, 02 Nov 2026 13:13:46 +0000
sashiko-bot@kernel.org wrote:

> 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.

This is exactly what it is expected.

return 0 is unreachable but pcf2127_clkout_rate2cof() returns a u32.

Without the last return 0 (even if unreachable) or any other u32 returned value,
GCC obviously complied
   error: control reaches end of non-void function

> 
> 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)?
> 

No.

All rates should be present in pcf2127_clkout_rates[]. The 0Hz rate value is
a valid rate even if the pin will be set in HiZ in that case.

pcf2127->clkout_cof should hold the value related to OHz rate.

A user can ask a 0Hz rate and call clk_prepare(). This is a valid use case.

Best regards,
Hervé