Re: [PATCH v7 2/2] clk: Add gpio-locked clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: gpio-locked-clock prepare callback inverts CCF error logic
- [Medium] clk: gpio-locked-clock ignores DT parent count and hardcodes 1
--

commit e6f2116296103d738da7176d4979ce3c21f6786f
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

clk: Add gpio-locked clock driver

This commit adds a gpio-locked clock driver, exposing a clock whose status
is determined by a GPIO signal. Consumers can use the output clock to wait
until the input clock is locked before initializing dependent peripherals.

> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index 9099c57e27156..a264ba21baf26 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
[ ... ]
> @@ -138,6 +138,19 @@ static const struct clk_ops clk_gpio_mux_ops = {
>  	.determine_rate = __clk_mux_determine_rate,
>  };
>  
> +/* We can't prepare the clock, but the Common Clock Framework calls only
> + * prepare() not is_prepared(), therefore we fallback on the actuall GPIO value.
> + */
> +static int clk_gpio_locked_prepare(struct clk_hw *hw)
> +{
> +	return clk_sleeping_gpio_gate_is_prepared(hw);
> +}

[Severity: High]
Does this return value interact correctly with the Common Clock Framework?

The function clk_sleeping_gpio_gate_is_prepared() returns the raw boolean
GPIO status (1 if locked, 0 if unlocked), but the prepare callback typically
expects 0 for success and a negative integer for failure.

Because the framework interprets any non-zero value as an error, a locked
clock returning 1 might cause clock preparation to fail when it should
succeed. Conversely, an unlocked clock returning 0 might be treated as a
success, causing downstream consumers to proceed while the PLL is still
unlocked.

[ ... ]
> @@ -192,6 +210,12 @@ static struct clk_hw *clk_hw_register_gpio_mux(struct device *dev,
>  	return clk_register_gpio(dev, 2, gpiod, &clk_gpio_mux_ops);
>  }
>  
> +static struct clk_hw *clk_hw_register_gpio_locked(struct device *dev,
> +						  struct gpio_desc *gpiod)
> +{
> +	return clk_register_gpio(dev, 1, gpiod, &clk_gpio_locked_ops);
> +}

[Severity: Medium]
Is it intentional to hardcode 1 parent here instead of using the parsed
num_parents from the device tree?

If the clock is defined in the device tree without a parent (acting as a
root clock), passing 1 causes the framework to expect a parent at index 0.
This could cause parent resolution to fail, leaving the clock permanently
orphaned with a 0 Hz rate, which breaks downstream consumers relying on
rate propagation.

Could we pass the dynamically parsed num_parents from gpio_clk_driver_probe()
downward, or validate that it equals 1, similar to how the gpio-mux-clock
explicitly validates its parent count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927-feature-clock-guard-v7-0-8dd19568d9a2@bruker.com?part=2