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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: gpio: negative error code from GPIO read evaluated as success
- [Medium] clk: gpio: clk_prepare callback violates CCF abstraction by failing to block
- [Medium] clk: gpio: gpio-locked-clock ignores DT num_parents, forcing orphan state
--

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

clk: Add gpio-locked clock driver

This commit adds a gpio-locked clock driver where the clock's valid status
is determined by a GPIO signal. It allows consumers to wait on the clock
lock before initializing dependent peripherals.

> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index 9099c57e27156..db68b8bc44c5a 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
> @@ -138,6 +138,25 @@ static const struct clk_ops clk_gpio_mux_ops = {

[ ... ]

> +/* 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.
> + * The function clk_sleeping_gpio_gate_is_prepared() returns the raw boolean
> + * GPIO status (1 if locked, 0 if unlocked), but the prepare callback
> + * clk_core_prepare() expects 0 for success and non-zero integer for failure.
> + */
> +static int clk_gpio_locked_prepare(struct clk_hw *hw)
> +{
> +	if (clk_sleeping_gpio_gate_is_prepared(hw))
> +		return 0;

[Severity: High]
Will this evaluate a negative error code as a success?

The underlying function clk_sleeping_gpio_gate_is_prepared() calls
gpiod_get_value_cansleep():

static int clk_sleeping_gpio_gate_is_prepared(struct clk_hw *hw)
{
        struct clk_gpio *clk = to_clk_gpio(hw);
        return gpiod_get_value_cansleep(clk->gpiod);
}

If gpiod_get_value_cansleep() returns a negative error code (such as -EIO)
due to a hardware read failure, the if condition here will evaluate it as
true.

This would return 0 (success) to the Common Clock Framework, incorrectly
masking the hardware error and falsely signaling that the PLL/clock lock
was successfully acquired.

> +
> +	return -EBUSY;

[Severity: Medium]
Does returning -EBUSY here violate the Common Clock Framework abstraction?

The clk_prepare callback is permitted to sleep and is generally expected
to block until the hardware is ready. By returning an error immediately
instead of blocking until the clock is locked, standard device drivers
calling clk_prepare_enable() might permanently fail their probe sequence.

Will this force consumers to implement non-standard polling logic to
handle the -EBUSY return value?

> +}
> +
> +static const struct clk_ops clk_gpio_locked_ops = {

[ ... ]

> @@ -192,6 +216,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 the number of parents to 1 here?

In the gpio_clk_driver_probe() function, the true parent count is parsed
from the device tree:

num_parents = of_clk_get_parent_count(node);

If the clock is configured in the device tree as an independent root clock
(for example, an FPGA PLL with no parent clock, meaning num_parents is 0),
hardcoding it to 1 here will cause CCF to attempt to resolve a parent at
index 0.

Could this cause clk_core_get() to fail internal lookups, leading the clock
to silently fall back to an orphan state?

> +}
> +
>  static int gpio_clk_driver_probe(struct platform_device *pdev)
>  {

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