Re: [PATCH v3 1/2] dt-bindings: Add GPIO-locked fixed clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The driver fails to probe with `-EINVAL` because it incorrectly uses `devm_clk_bulk_get_all` IDs as CCF `parent_names`.
- [High] The driver registers multiple required input clocks as CCF parents, causing `clk_enable()` to permanently fail.
- [High] The binding lacks a `clock-frequency` property, resulting in a 0 Hz clock when no parents are specified.
- [High] The .is_enabled callback incorrectly returns 0 when the clock is ready and a negative error code when it is not.
- [Medium] The driver reads the 'gpio-names' property which is missing from the dt-binding schema.
- [Low] The `clocks` property is missing an array size constraint (`maxItems` or `items`).
- [Low] The example uses a device-specific node name instead of a generic node name.
--

commit f442ab0fb77267ab8b660fa86301ab0aa027e186
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
Subject: dt-bindings: Add GPIO-locked fixed clock

This commit introduces a new device tree binding and corresponding driver
support for a GPIO-locked fixed clock provider. The clock's availability
depends on prerequisite conditions, including parent clocks and GPIO lock
indicators. This allows modeling FPGA-generated clocks that expose lock
status through GPIOs.

> diff --git a/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
> @@ -0,0 +1,70 @@
[ ... ]
> +properties:
> +  compatible:
> +    const: gpio-locked-fixed-clock
> +
> +  "#clock-cells":
> +    const: 0
> +
> +  clocks:
> +    description: Input clocks whose validity is monitored by this provider.

[Severity: Low]
Does the clocks property need an array size constraint such as maxItems
or items? Without it, dt_binding_check might emit a meta-schema validation
error.

[ ... ]
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    clk_gpio_locked: gpio-locked-fixed-clock {

[Severity: Low]
Is gpio-locked-fixed-clock the appropriate node name here? Standard device
tree rules typically require generic node names (e.g., clock-controller { or
clock {) rather than device-specific names.

> +        compatible = "gpio-locked-fixed-clock";
[ ... ]
> diff --git a/drivers/clk/clk-gpio-locked.c b/drivers/clk/clk-gpio-locked.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/clk-gpio-locked.c
> @@ -0,0 +1,306 @@
[ ... ]
> +static int gpio_locked_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct gpio_locked_clk_priv *priv = to_gpio_locked_clk_priv(hw);
> +	int ret = 0;
> +
> +	if (priv->num_gpios > 0) {
> +		ret = is_gpio_ready(priv);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	// Now check for the clocks
> +	for (int i = 0; i < priv->num_clks; i++) {
> +		struct clk_hw *hw_clk = __clk_get_hw(priv->clks[i].clk);
> +
> +		if (!clk_hw_is_enabled(hw_clk)) {
> +			dev_dbg(priv->dev, "Clock %i (%s) is not ready",
> +				i, priv->clks[i].id);
> +			return -EBUSY;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does .is_enabled correctly return the clock status here?

The Common Clock Framework expects this callback to return a non-zero value
(true) when the clock is ready, and 0 (false) when it is disabled. Returning
0 on success and negative error codes (like -EBUSY) on failure might cause
CCF to implicitly cast the negative error codes to true, falsely assuming
the clock is enabled when it is actually unavailable.

[ ... ]
> +static const struct clk_ops gpio_locked_clk_ops = {
> +	.enable =	gpio_locked_clk_enable,
> +	.is_enabled =	gpio_locked_clk_is_enabled,
> +	.get_parent =	gpio_locked_clk_get_parent,
> +};

[Severity: High]
The commit message mentions this provider exposes a fixed-rate clock, but
the clock-frequency property is missing from the binding, and .recalc_rate
is not implemented here. 

If the clocks property is omitted in the device tree (which the schema
allows), won't the root clock default to 0 Hz? How will downstream consumers
know the correct clock rate without a way to define it?

[ ... ]
> +static int gpio_locked_clk_parse_gpios(struct gpio_locked_clk_priv *priv)
> +{
[ ... ]
> +	for (i = 0; i < priv->num_gpios; i++) {
> +		of_property_read_string_index(np, "gpio-names", i,
> +				&priv->gpio_names[i]);

[Severity: Medium]
Will this logic cause dt_binding_check to fail if a device tree includes
gpio-names?

The driver attempts to read the gpio-names property, but it is missing
from the dt-binding schema, which strictly enforces additionalProperties:
false. If it is omitted from the device tree to pass validation, won't
priv->gpio_names[i] remain NULL and cause debug warnings?

[ ... ]
> +static int gpio_locked_clk_parse_outputs(struct gpio_locked_clk_priv *priv)
> +{
[ ... ]
> +	init.name = priv->output_clock_name;
> +	init.ops = &gpio_locked_clk_ops;
> +	init.flags = 0;
> +	init.num_parents = priv->num_clks;

[Severity: High]
Can clk_enable() permanently fail if there are multiple input clocks?

By setting init.num_parents = priv->num_clks, this configures the clock as
a CCF multiplexer. However, CCF's clk_core_enable() only enables the
single active parent returned by .get_parent() (which the driver hardcodes
to return 0). Since the .is_enabled() callback mandates that all input
clocks must be enabled, won't the check fail and return -EBUSY for the other
unenabled parents?

> +
> +	if (priv->num_clks) {
> +		const char **parent_names;
> +		int j;
> +
> +		parent_names = devm_kcalloc(dev, priv->num_clks,
> +					    sizeof(*parent_names),
> +					    GFP_KERNEL);
> +		if (!parent_names)
> +			return -ENOMEM;
> +
> +		for (j = 0; j < priv->num_clks; j++)
> +			parent_names[j] = priv->clks[j].id;

[Severity: High]
Is it safe to assign priv->clks[j].id to parent_names[j] here?

Since devm_clk_bulk_get_all() populates clks[i].id from the clock-names
property, and clock-names is explicitly omitted from the DT binding schema
(and forbidden by additionalProperties: false), won't clks[i].id evaluate
to NULL?

This could cause clk_core_populate_parent_map() to emit a WARN and return
-EINVAL during registration. Would using .parent_hws instead of
.parent_names be a more reliable approach?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-feature-clock-guard-v3-0-01cca0aa04a5@bruker.com?part=1