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

[Jerome Brunet]

On mar. 15 sept. 2026 at 15:58, Vyacheslav Yurkov <uvv.mail@xxxxxxxxx> wrote:

> On 15.09.2026 12:09, Jerome Brunet wrote:
>> On mar. 15 sept. 2026 at 09:27, Vyacheslav Yurkov via B4 Relay <devnull+V.Yurkov.EXT.bruker.com@xxxxxxxxxx> wrote:
>> 
>>> From: Vyacheslav Yurkov <V.Yurkov.EXT@xxxxxxxxxx>
>>>
>>> A gpio-locked clock exposes a clock, which status is determined by a
>>> GPIO signal. The common use-case is a FPGA-assisted clocking design
>>> where peripheral clocks are generated by FPGA PLLs that are outside
>>> CPU control, with clock-valid/PLL-lock status exposed through GPIO signals.
>>> Consumers can use the output clock to wait until the input clock is locked
>>> and only then initialize dependent peripherals.
>>>
>> 
>> We already have gpio gate driver in drivers/clk/clk-gpio.c
>> 
>> It would be much better if you could just extend that one that take
>> optionally take a clock input like you do here.
>
> It is a bit more than that. The gated clock requires "enable-gpios" 
> property, while gpio-locked clock needs "locked-gpios". Technically I 
> could re-use the "enable-gpios", but that might lead to a confusion, 
> because semantically they are used for different purpose. Do you think 
> the extension of clk-gpio.c would be still better in this case?

Apologies, I've mis-read you initial submission.
Basically this is a read-only gate controlled by a gpio.

I think it still belongs in clk-gpio with its own ops, along with the
gate and mux that are already there.

enabled-gpio for the pin maybe ?

>
>>> Signed-off-by: Vyacheslav Yurkov <V.Yurkov.EXT@xxxxxxxxxx>
>>> ---
>>>   drivers/clk/Makefile          |   1 +
>>>   drivers/clk/clk-gpio-locked.c | 169 ++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 170 insertions(+)
>>>
>>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>>> index b18af485d7f0..b904f292d683 100644
>>> --- a/drivers/clk/Makefile
>>> +++ b/drivers/clk/Makefile
>>> @@ -46,6 +46,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

Drop every reference to locked ... this is very PLL oriented and the
driver you are proposing is more generic than that


>>>   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..b648f8763922
>>> --- /dev/null
>>> +++ b/drivers/clk/clk-gpio-locked.c
>>> @@ -0,0 +1,169 @@
>>> +// 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>
>>> +
>>> +/**
>>> + * struct gpio_locked_clk_priv - private state for the whole driver
>>> + * @dev:		platform device
>>> + *
>>> + * @input_clk	input clock
>>> + * @gpios:		input GPIO descriptor
>>> + *
>>> + * @output_hw_clk:      output clock HW descriptor
>>> + * @output_clock_name:  output clock name
>>> + */
>>> +struct gpio_locked_clk_priv {
>>> +	struct device *dev;
>>> +
>>> +	struct clk *input_clk;
>>> +	struct gpio_desc *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 gpio_locked_clk_is_enabled(struct clk_hw *hw)
>>> +{
>>> +	struct gpio_locked_clk_priv *priv = to_gpio_locked_clk_priv(hw);
>>> +
>>> +	int data = gpiod_get_value(priv->gpios);
>>> +
>>> +	if (data < 0) {
>>> +		dev_err(priv->dev, "Failed to get data gpio val: %d\n",
>>> +				data);
>>> +		return data;
>>> +	} else if (!data) {
>>> +		dev_warn(priv->dev, "GPIO is not ready");
>>> +		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);
>>> +}

You need to explain your problem a bit more because this is not OK. Your
clock should really just provide .is_enabled() AFAICT

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

Same, I dont get why you need that. Not needed if there a single parent

>>> +
>>> +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_outputs(struct gpio_locked_clk_priv *priv)
>>> +{
>>> +	struct device *dev = priv->dev;
>>> +	struct device_node *np = dev->of_node;
>>> +	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);
>>> +
>>> +	priv->output_hw_clk.init =
>>> +		CLK_HW_INIT_FW_NAME(priv->output_clock_name,
>>> +				    __clk_get_name(priv->input_clk),

No, just put the name as it is in DT (input?) or go for index 0 possibly
?

No call to devm_clk_get_enabled() from this driver to its input. A clock
controller should not do that. The consuming device will get this clock,
enable it and the enable will trickle down to the provider.


>>> +				    &gpio_locked_clk_ops, 0);
>>> +
>>> +	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);
>>> +

Drop those prints

>>> +	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);
>>> +
>>> +	priv->input_clk = devm_clk_get_enabled(priv->dev, NULL);
>>> +	if (IS_ERR(priv->input_clk))
>>> +		return dev_err_probe(priv->dev, PTR_ERR(priv->input_clk),
>>> +				"Failed to get locked fixed clock, not yet ready\n");
>> 
>> In your use case it might be fixed, but nothing says it is in general
>> 
> Good point, thanks.

-- 
Jerome