Re: [PATCH v7 2/3] clk: cs2600: Add Fractional-N clock driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver fails to toggle the FREEZE_EN bit after writing to frozen registers, preventing new configurations from taking effect.
- [Medium] cs2600_pll_out_recalc_rate computes a new theoretical ratio instead of querying the actual hardware frequency.
- [Medium] The driver exposes the internal PLL_OUT clock to consumers despite explicit comments intending to restrict it.
- [Medium] dev_err is inappropriately called in determine_rate and ratio calculations for unsupported parameters, causing log spam.
--

--- Patch [2]: [PATCH v7 2/3] clk: cs2600: Add Fractional-N clock driver ---
commit 75202f90081b654ba7061c4d517a6849df860547
Author: Paul Handrigan <[email protected]>

    clk: cs2600: Add Fractional-N clock driver
    
    Add driver for the Cirrus Logic CS2600 fractional-N clock synthesizer
    and multiplier.

> diff --git a/drivers/clk/cirrus/clk-cs2600.c b/drivers/clk/cirrus/clk-cs2600.c
> new file mode 100644
> index 0000000000000..977a4bf3c31dc
> --- /dev/null
> +++ b/drivers/clk/cirrus/clk-cs2600.c
[ ... ]
> +static int cs2600_calc_ratio(struct cs2600 *cs2600,
> +			     unsigned long rate,
> +			     unsigned long parent_rate,
> +			     unsigned int *ratio_out,
> +			     bool *hi_res)
> +{
[ ... ]
> +	if (ratio > CS2600_12_20_RATIO_MAX) {
> +		*hi_res = false;
> +		ratio >>= (CS2600_12_20_SHIFT - CS2600_20_12_SHIFT);
> +
> +		if (ratio > CS2600_20_12_RATIO_MAX) {
> +			dev_err(cs2600->dev, "Ratio %lu:%lu too large\n", parent_rate, rate);

[Severity: Medium]
Will this dev_err() trigger during normal speculative CCF operations like
clk_round_rate()? If determine_rate() passes an unachievable rate, this could
spam the kernel log.

[ ... ]
> +static unsigned long cs2600_pll_out_recalc_rate(struct clk_hw *hw,
> +						unsigned long parent_rate)
> +{
> +	struct cs2600_clk_hw *clk_hw = hw_to_cs2600_clk(hw);
> +	struct cs2600 *cs2600 = clk_hw->priv;
> +
> +	if ((parent_rate == 0) || (cs2600->pll_target_rate == 0))
> +		return 0;
> +
> +	if (parent_rate == cs2600->pll_parent_rate)
> +		return cs2600->pll_target_rate;
> +
> +	return cs2600_calc_rounded_integer_rate(cs2600, parent_rate, cs2600->pll_target_rate);

[Severity: Medium]
Does this bypass hardware querying? The CCF expects recalc_rate() to query
the physical hardware to inherit settings (like those from the bootloader)
and report the current state.

By returning a computed rate based on the uninitialized software variable
pll_target_rate, won't CCF assume the hardware is running at 0Hz until
set_rate() is explicitly called?

[ ... ]
> +static int cs2600_pll_out_set_rate(struct clk_hw *hw, unsigned long rate,
> +				   unsigned long parent_rate)
> +{
[ ... ]
> +	ret = cs2600_write_ratio(cs2600, CS2600_RATIO_SLOT_1, ratio);

[Severity: High]
Is a step to toggle FREEZE_EN missing here? The commit message states
FREEZE_EN is toggled after writing to frozen bits. Since CS2600_FREEZE_EN
is never actually used in the code, won't these writes remain trapped in the
holding buffer and fail to update the active hardware registers?

[ ... ]
> +static int cs2600_pll_out_set_parent(struct clk_hw *hw, u8 index)
> +{
[ ... ]
> +	ret = regmap_update_bits(regmap, CS2600_PLL_CFG2, CS2600_PLL_MODE_SEL,
> +				 index);

[Severity: High]
Does this also need to toggle FREEZE_EN to latch the mode selection into the
active registers?

[ ... ]
> +static int cs2600_bclk_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
[ ... ]
> +	div_index = cs2600_get_clk_div_index(cs2600_bclk_div, req->rate, req->best_parent_rate);
> +	if (div_index < 0) {
> +		dev_err(cs2600->dev, "BCLK_OUT cannot be derived from the parent rate");

[Severity: Medium]
Since determine_rate() is often called speculatively, could this dev_err()
fill the kernel log when CCF tests unachievable rate requests?

[ ... ]
> +static int cs2600_fsync_determine_rate(struct clk_hw *hw,
> +				       struct clk_rate_request *req)
> +{
[ ... ]
> +	div_index = cs2600_get_clk_div_index(cs2600_fsync_div, req->rate, req->best_parent_rate);
> +	if (div_index < 0) {
> +		if (req->best_parent_rate)
> +			dev_err(cs2600->dev, "FSYNC_OUT cannot be derived from the parent rate");

[Severity: Medium]
Could this dev_err() also spam the log during speculative rate queries?

[ ... ]
> +static struct clk_hw *cs2600_of_clk_get(struct of_phandle_args *clkspec,
> +					void *data)
> +{
> +	unsigned int index = clkspec->args[0];
> +	struct cs2600 *cs2600 = data;
> +
> +	/* PLL_OUT is an internal clock */
> +	if (index > CS2600_OUT_CLK_MAX - 1) {

[Severity: Medium]
Does this actually prevent consumers from requesting the internal PLL_OUT
clock?

The bounds check allows index == CS2600_PLL_OUT (which is 0), potentially
allowing external device tree nodes to bypass the output clocks and
manipulate the internal PLL directly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2