[PATCH v2 0/2] A proposal to add a gpio-locked fixed clock driver.

[PATCH v2 0/2] A proposal to add a gpio-locked fixed clock driver.

[Vyacheslav Yurkov via B4 Relay]

A gpio-locked fixed clock aggregates one or more input clocks and/or one
or more GPIOs. It's similar to a gated-fixed-clock, but GPIO direction is
inverted. Consumers can use the output clock to wait until all input
clocks are locked and only then initialize / access dependent peripherals.

The usage example for such a driver is when peripherals depend on PLLs in
a FPGA, which can't be directly accessed by the CPU, but need a GPIO pin
to check whether clock is actually usable. E.g. some of the IPs might not
have a proper split between registers and IP core, which means that if an
external clock and/or PLL lock is missing and one tries to access the 
registers, the response never comes, thus the CPU stalls.

Signed-off-by: Vyacheslav Yurkov <uvv.mail@gmail.com>
Signed-off-by: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
---
Changes in v2:
- Renamed to clk-gpio-locked to express intent.
- Provide enable() / is_enabled() operations so the clock behaves as
  expected
- Fixed DTS errors / warnings
- Link to v1: https://lore.kernel.org/r/20260318-feature-clock-guard-v1-0-6137cb4084b7@bruker.com

---
Vyacheslav Yurkov (2):
      clk: Add gpio-locked clock driver
      dt-bindings: Add GPIO locked fixed clock

 .../bindings/clock/gpio-locked-fixed-clock.yaml    |  77 ++++++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-gpio-locked.c                      | 306 +++++++++++++++++++++
 3 files changed, 384 insertions(+)
---
base-commit: 917719c412c48687d4a176965d1fa35320ec457c
change-id: 20260318-feature-clock-guard-f20a2c35b965

Best regards,
-- 
Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

[PATCH v2 1/2] clk: Add gpio-locked clock driver

[Vyacheslav Yurkov via B4 Relay]

From: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

A gpio-locked clock aggregates one or more input clocks and/or one or
more GPIOs. It's similar to a gated-fixed-clock, but GPIO direction is
inverted. Consumers can use the output clock to wait until all input
clocks are locked and only then initialize dependent peripherals.

Signed-off-by: Vyacheslav Yurkov <uvv.mail@gmail.com>
Signed-off-by: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
---
 drivers/clk/Makefile          |   1 +
 drivers/clk/clk-gpio-locked.c | 306 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 307 insertions(+)

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index a3e2862ebd7e..464ca1c1d8a8 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_CLK_FD_KUNIT_TEST) += clk-fractional-divider_test.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-gpio.o
 ifeq ($(CONFIG_OF), y)
 obj-$(CONFIG_COMMON_CLK)	+= clk-conf.o
+obj-$(CONFIG_COMMON_CLK)	+= clk-gpio-locked.o
 endif
 
 # KUnit specific helpers
diff --git a/drivers/clk/clk-gpio-locked.c b/drivers/clk/clk-gpio-locked.c
new file mode 100644
index 000000000000..79098f9b6532
--- /dev/null
+++ b/drivers/clk/clk-gpio-locked.c
@@ -0,0 +1,306 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Clock Controller Guard Driver
+ *
+ * Copyright 2026 Bruker Corporation
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define MAX_INPUT_GPIO_COUNT 32
+
+/**
+ * struct gpio_locked_clk_priv - private state for the whole driver
+ * @dev:		platform device
+ *
+ * @clks:		array of input clock descriptors
+ * @num_clks:		number of entries in @inputs
+ *
+ * @gpios:		array of GPIO descriptors
+ * @gpio_names:		GPIO names
+ * @num_gpios:		number of input GPIOs
+ *
+ * @output_hw_clk:      output clock HW descriptor
+ * @output_clock_name:  output clock name
+ */
+struct gpio_locked_clk_priv {
+	struct device *dev;
+
+	struct clk_bulk_data *clks;
+	int num_clks;
+
+	struct gpio_descs *gpios;
+	const char **gpio_names;
+	int num_gpios;
+
+	struct clk_hw output_hw_clk;
+	const char *output_clock_name;
+};
+
+#define to_gpio_locked_clk_priv(_hw) \
+	container_of(_hw, struct gpio_locked_clk_priv, output_hw_clk)
+
+static int is_gpio_ready(struct gpio_locked_clk_priv *priv)
+{
+	unsigned long values[BITS_TO_LONGS(MAX_INPUT_GPIO_COUNT)] = {0};
+	int ret = 0;
+
+	if (priv->num_gpios == 0)
+		return 0;
+
+	ret = gpiod_get_array_value(priv->gpios->ndescs,
+		priv->gpios->desc,
+		priv->gpios->info,
+		values);
+
+	if (ret) {
+		dev_err(priv->dev, "Failed to read GPIOs");
+		return -EIO;
+	}
+
+	for (int i = 0; i < priv->gpios->ndescs; i++) {
+		if (!test_bit(i, values)) {
+			dev_warn(priv->dev, "GPIO %s is not ready", priv->gpio_names[i]);
+			return -EBUSY;
+		}
+	}
+
+	return 0;
+}
+
+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;
+}
+
+/* We can't enable the clock, but the Common Clock Framework calls only
+ * enable() not is_enabled()
+ */
+static int gpio_locked_clk_enable(struct clk_hw *hw)
+{
+	return gpio_locked_clk_is_enabled(hw);
+}
+
+/* We have to implement it, but we are not going to control
+ * parent clock selection
+ */
+static u8 gpio_locked_clk_get_parent(struct clk_hw *hw)
+{
+	return 0;
+}
+
+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,
+};
+
+static int gpio_locked_clk_parse_inputs(struct gpio_locked_clk_priv *priv)
+{
+	struct device *dev = priv->dev;
+	int ret;
+
+	ret = devm_clk_bulk_get_all(dev, &priv->clks);
+	if (ret < 0) {
+		dev_err(dev, "failed to get input clocks: %d\n", ret);
+		return ret ? ret : -ENOENT;
+	}
+
+	priv->num_clks = ret;
+
+	if (priv->num_clks == 0)
+		dev_info(dev, "No input clocks provided\n");
+
+	for (int i = 0; i < priv->num_clks; i++)
+		dev_dbg(dev, "input clk[%d]: name='%s' rate=%lu Hz\n",
+			i, priv->clks[i].id,
+			clk_get_rate(priv->clks[i].clk));
+
+	return 0;
+}
+
+static int gpio_locked_clk_parse_gpios(struct gpio_locked_clk_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	int i;
+
+	priv->gpios = devm_gpiod_get_array_optional(dev, "locked", GPIOD_ASIS);
+	if (IS_ERR(priv->gpios)) {
+		dev_err(dev, "failed to get GPIO array: %ld\n",
+				PTR_ERR(priv->gpios));
+		return PTR_ERR(priv->gpios);
+	}
+
+	if (!priv->gpios) {
+		dev_info(dev, "No GPIOs provided, continue\n");
+		priv->num_gpios = 0;
+		return 0;
+	}
+
+	priv->num_gpios = priv->gpios->ndescs;
+	if (priv->num_gpios > MAX_INPUT_GPIO_COUNT) {
+		dev_err(priv->dev, "Maximum number of input GPIOs is 32\n");
+		return -EINVAL;
+	}
+
+	/* gpio_descs carries no names, so read "gpio-names" separately */
+	priv->gpio_names = devm_kcalloc(dev, priv->num_gpios, sizeof(*priv->gpio_names),
+			GFP_KERNEL);
+	if (!priv->gpio_names)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->num_gpios; i++) {
+		of_property_read_string_index(np, "gpio-names", i,
+				&priv->gpio_names[i]);
+
+		dev_dbg(dev, "gpio[%d]: name='%s'\n",
+				i, priv->gpio_names[i] ? priv->gpio_names[i] : "(unnamed)");
+	}
+
+	return 0;
+}
+
+static int gpio_locked_clk_parse_outputs(struct gpio_locked_clk_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct clk_init_data init = {};
+	int ret;
+
+	of_property_read_string_index(np, "clock-output-names", 0,
+		&priv->output_clock_name);
+
+	if (!priv->output_clock_name)
+		priv->output_clock_name = dev_name(priv->dev);
+
+	init.name = priv->output_clock_name;
+	init.ops = &gpio_locked_clk_ops;
+	init.flags = 0;
+	init.num_parents = priv->num_clks;
+
+	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;
+
+		init.parent_names = parent_names;
+	}
+
+	priv->output_hw_clk.init = &init;
+
+	ret = devm_clk_hw_register(dev, &priv->output_hw_clk);
+	if (ret) {
+		dev_err(dev, "failed to register output clk'%s': %d\n",
+			priv->output_clock_name, ret);
+		return ret;
+	}
+
+	dev_info(priv->dev, "Output clock '%s' registered\n", priv->output_clock_name);
+
+	return 0;
+}
+
+static int gpio_locked_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_locked_clk_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	ret = gpio_locked_clk_parse_inputs(priv);
+	if (ret)
+		return ret;
+
+	ret = gpio_locked_clk_parse_gpios(priv);
+	if (ret)
+		return ret;
+
+	if (priv->num_clks == 0 && priv->num_gpios == 0) {
+		dev_err(priv->dev, "At least 1 input clock or input GPIO is required\n");
+		return -EINVAL;
+	}
+
+	ret = gpio_locked_clk_parse_outputs(priv);
+	if (ret)
+		return ret;
+
+	ret = devm_of_clk_add_hw_provider(priv->dev, of_clk_hw_simple_get,
+		&priv->output_hw_clk);
+	if (ret) {
+		dev_err(priv->dev, "failed to register clock provider '%s': %d\n",
+			priv->output_clock_name, ret);
+		return ret;
+	}
+
+	dev_info(dev, "registered %u input clocks, %u GPIOs\n",
+		 priv->num_clks, priv->num_gpios);
+
+	return 0;
+}
+
+static void gpio_locked_clk_remove(struct platform_device *pdev)
+{
+	dev_dbg(&pdev->dev, "removed\n");
+}
+
+static const struct of_device_id gpio_locked_clk_of_match[] = {
+	{ .compatible = "gpio-locked-fixed-clock" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, gpio_locked_clk_of_match);
+
+static struct platform_driver gpio_locked_clk_driver = {
+	.probe  = gpio_locked_clk_probe,
+	.remove = gpio_locked_clk_remove,
+	.driver = {
+		.name           = "gpio-locked-fixed-clock",
+		.of_match_table = gpio_locked_clk_of_match,
+	},
+};
+module_platform_driver(gpio_locked_clk_driver);
+
+MODULE_AUTHOR("Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>");
+MODULE_DESCRIPTION("GPIO-locked clock driver");
+MODULE_LICENSE("Dual BSD/GPL");

-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Bypassing the consumer clock API to use `__clk_get_hw()` on parent clocks leads to a Use-After-Free condition.
- [High] The `.is_enabled` callback returns inverted boolean states, breaking CCF state tracking.
- [High] The driver calls `gpiod_get_array_value()` within atomic CCF callbacks, triggering a sleep-in-atomic panic for sleepable GPIOs.
- [High] The driver registers multiple input clocks as parents of a multiplexer, which permanently disables all but the first input clock.
- [High] The driver incorrectly assigns local consumer connection IDs to `init.parent_names`, causing CCF parent resolution to fail.
- [Medium] The driver fails to explicitly configure the GPIO pins as inputs, potentially reading output latches.
- [Medium] The `is_gpio_ready` function prints warnings inside an atomic context and can pass a NULL pointer to a `%s` format specifier.
--

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

clk: Add gpio-locked clock driver

A gpio-locked clock aggregates one or more input clocks and/or one or
more GPIOs. Consumers can use the output clock to wait until all input
clocks are locked and only then initialize dependent peripherals.

> diff --git a/drivers/clk/clk-gpio-locked.c b/drivers/clk/clk-gpio-locked.c
> index 0000000000000..79098f9b65326
> --- /dev/null
> +++ b/drivers/clk/clk-gpio-locked.c
[ ... ]
> +static int is_gpio_ready(struct gpio_locked_clk_priv *priv)
> +{
> +	unsigned long values[BITS_TO_LONGS(MAX_INPUT_GPIO_COUNT)] = {0};
> +	int ret = 0;
> +
> +	if (priv->num_gpios == 0)
> +		return 0;
> +
> +	ret = gpiod_get_array_value(priv->gpios->ndescs,
> +		priv->gpios->desc,
> +		priv->gpios->info,
> +		values);

Will this trigger a sleep-in-atomic panic for sleepable GPIOs?

is_gpio_ready() is called from the .enable and .is_enabled callbacks,
which execute in an atomic context holding the enable_lock spinlock.

If a consumer attaches a GPIO from a sleepable bus controller (e.g., an I2C or
SPI expander), the underlying GPIO driver will attempt to sleep while the
spinlock is held.

> +
> +	if (ret) {
> +		dev_err(priv->dev, "Failed to read GPIOs");
> +		return -EIO;
> +	}
> +
> +	for (int i = 0; i < priv->gpios->ndescs; i++) {
> +		if (!test_bit(i, values)) {
> +			dev_warn(priv->dev, "GPIO %s is not ready", priv->gpio_names[i]);

Can this spam the kernel log from an atomic context?

This warning executes under a spinlock during CCF sysfs reads or consumer
polling.

Also, if the gpio-names DT property is missing, priv->gpio_names[i] will
be NULL, which is then passed to the %s format specifier.

> +			return -EBUSY;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +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);

Does bypassing the consumer clock API lead to a Use-After-Free condition?

The driver acquires consumer clock handles (struct clk *) via
devm_clk_bulk_get_all(). However, this extracts the provider's
struct clk_hw * pointer using the internal API __clk_get_hw().

If the parent clock's provider is unbound (e.g., via sysfs), its clk_hw
memory is freed, but the consumer's struct clk * handle remains valid.
Subsequent state checks will dereference the freed hw pointer.

> +
> +		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;
> +}

Does this return inverted boolean states?

The Common Clock Framework (CCF) expects the .is_enabled callback to return
a boolean-like integer (non-zero for enabled, 0 for disabled).

This function returns 0 when all inputs are locked and ready, and -EBUSY
(which evaluates to a non-zero, true value) when the inputs are not ready.
This will cause CCF to track the clock as actively enabled when its
dependencies are unavailable, and disabled when it is fully ready.

[ ... ]
> +static u8 gpio_locked_clk_get_parent(struct clk_hw *hw)
> +{
> +	return 0;
> +}
[ ... ]
> +static int gpio_locked_clk_parse_gpios(struct gpio_locked_clk_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	int i;
> +
> +	priv->gpios = devm_gpiod_get_array_optional(dev, "locked", GPIOD_ASIS);

Could this read output latches instead of the physical external signal?

The GPIOD_ASIS flag leaves the pins in their current or default state.
If the bootloader or prior configuration left a pin as an output,
gpiod_get_array_value() will read the output latch value.

Explicitly requesting GPIOD_IN configures the pins safely as inputs.

[ ... ]
> +static int gpio_locked_clk_parse_outputs(struct gpio_locked_clk_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	struct clk_init_data init = {};
> +	int ret;
> +
> +	of_property_read_string_index(np, "clock-output-names", 0,
> +		&priv->output_clock_name);
> +
> +	if (!priv->output_clock_name)
> +		priv->output_clock_name = dev_name(priv->dev);
> +
> +	init.name = priv->output_clock_name;
> +	init.ops = &gpio_locked_clk_ops;
> +	init.flags = 0;
> +	init.num_parents = priv->num_clks;

Does this register multiple input clocks as parents of a multiplexer?

By setting init.num_parents = priv->num_clks and defining a .get_parent
callback that strictly returns 0, the driver configures itself as a clock
multiplexer.

In the CCF topology, a multiplexer only activates its currently selected
parent (index 0). Any additional input clocks (parents 1 to N) will never
be prepared or enabled by the framework.

> +
> +	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;
> +
> +		init.parent_names = parent_names;

Will this cause CCF parent resolution to fail?

The driver assigns priv->clks[j].id to init.parent_names[j]. This id
string populated by devm_clk_bulk_get_all() is the local consumer name from
the device tree's clock-names property.

However, init.parent_names requires the globally registered provider names
of the parent clocks. Supplying the local consumer IDs will cause string
matching to fail, leaving the clock orphaned.

> +	}
> +
> +	priv->output_hw_clk.init = &init;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-feature-clock-guard-v2-0-6c25458d5340@bruker.com?part=1

[PATCH v2 2/2] dt-bindings: Add GPIO locked fixed clock

[Vyacheslav Yurkov via B4 Relay]

From: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

Describe device tree binding for a fixed clock, which state can only be
determined by the external GPIO. It's similar to gated-fixed-clock, but
the GPIO direction is inverted.

Signed-off-by: Vyacheslav Yurkov <uvv.mail@gmail.com>
Signed-off-by: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
---
 .../bindings/clock/gpio-locked-fixed-clock.yaml    | 77 ++++++++++++++++++++++
 1 file changed, 77 insertions(+)

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
index 000000000000..e0256bbd441d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/gpio-locked-fixed-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO Locked Fixed Clock
+
+maintainers:
+  - Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
+
+description: |
+  Clock controller that aggregates input clocks and/or GPIO
+  signals and exposes them as a single clock output.
+
+properties:
+  compatible:
+    const: gpio-locked-fixed-clock
+
+  "#clock-cells":
+    const: 0
+
+  clocks:
+    description: Input clocks that externally locked.
+
+  clock-names:
+    description: Input clock names.
+
+  clock-output-names:
+    description: Names of the clock provided by this controller.
+    maxItems: 1
+
+  locked-gpios:
+    description: |
+      GPIOs to check the lock state.
+    minItems: 1
+    maxItems: 32
+
+  gpio-names:
+    description: Names corresponding to each GPIO.
+    minItems: 1
+    maxItems: 32
+
+required:
+  - compatible
+  - "#clock-cells"
+anyOf:
+  - required:
+      - clocks
+  - required:
+      - locked-gpios
+dependentRequired:
+  gpio-names:
+    - locked-gpios
+  clock-names:
+    - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    clk_gpio_locked: gpio-locked-fixed-clock {
+        compatible = "gpio-locked-fixed-clock";
+        #clock-cells = <0>;
+
+        clocks = <&clk0 0>, <&pll 0>;
+
+        locked-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>,
+                <&gpio0 5 GPIO_ACTIVE_HIGH>,
+                <&gpio1 2 GPIO_ACTIVE_LOW>;
+
+        gpio-names = "gpio0", "gpio1", "gpio2";
+
+        clock-output-names = "clkout0";
+    };

-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unnecessary new binding for a trivial GPIO polarity inversion, fundamentally misunderstanding Device Tree abstractions.
- [High] Broken `anyOf` constraint allows incomplete and invalid clock configurations.
- [Medium] Ambiguous clock aggregation logic and missing size constraints on array properties.
- [Low] Improper use of the generic `gpio-names` property for custom GPIO arrays.
- [Low] Non-generic device tree node name used in the example.
--

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

dt-bindings: Add GPIO locked fixed clock

This commit introduces a new device tree binding for a fixed clock whose
state is controlled by an external GPIO.

>     Describe device tree binding for a fixed clock, which state can only be
>     determined by the external GPIO. It's similar to gated-fixed-clock, but
>     the GPIO direction is inverted.

Device tree natively handles signal polarities via the GPIO specifier flags
like GPIO_ACTIVE_LOW. If a gated clock has an active-low enable signal,
could the existing gpio-gate-clock binding be used instead of creating a new
binding just for an inverted GPIO direction?

> 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
> index 0000000000000..e0256bbd441de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
> @@ -0,0 +1,77 @@

[ ... ]

> +  clocks:
> +    description: Input clocks that externally locked.

Since #clock-cells is set to 0, meaning this controller provides exactly
one output clock, how do multiple parent clocks logically map to the single
output? Should this property have a maxItems: 1 constraint?

[ ... ]

> +  gpio-names:
> +    description: Names corresponding to each GPIO.
> +    minItems: 1
> +    maxItems: 32

Is it appropriate to use the generic gpio-names property here? In device
tree bindings, gpio-names is typically strictly intended to pair with the
standard gpios property, rather than naming custom prefixed arrays like
locked-gpios.

[ ... ]

> +anyOf:
> +  - required:
> +      - clocks
> +  - required:
> +      - locked-gpios

Does this anyOf constraint allow for invalid clock configurations?

If clocks is omitted, there is no parent clock and no clock-frequency property
supported to define an intrinsic rate. If locked-gpios is omitted, the clock
operates without a locking GPIO, defeating the purpose of the binding.

Should both of these properties be explicitly required instead?

[ ... ]

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

This isn't a bug, but device tree specifications generally require node names
to be generic and reflect the class of the device (like clock or
clock-controller) rather than mirroring the compatible string.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-feature-clock-guard-v2-0-6c25458d5340@bruker.com?part=2