Re: [PATCH 2/2] dt-bindings: Add clock guard DT description

[Vyacheslav Yurkov <uvv.mail@gmail.com>]

On 23.03.2026 21:14, Conor Dooley wrote:

> 
> The binding you've got says "GPIOs used to control or guard the clocks",
> which is not what you're saying that is going on in this mail. A more
> suitable description would be "GPIOs used to check the status of the
> clocks".

Agree, the description I provided is not very accurate.

> I want to see an example dts user for this please.

DTS example:
   clock_guard: clock_controller_guard {
     compatible = "clock-controller-guard";
     #clock-cells = <1>;
     clocks = <&h2f_clk 0>, <&clk_fgpa_rx 0>, <clk_fpga_tx 0>;
     clock-names = "h2f_clk0", "clk_fpga_rx", "clk_fpga_tx";
     gpios = <&fpga_ip 0 GPIO_ACTIVE_HIGH>, <&fpga_ip 1 GPIO_ACTIVE_HIGH>;
     gpio-names = "gpio-input0", "gpio-input1";
     clock-output-names = "clkctrl-guard";
   };

   custom_device {
     compatible = "...";
     ...
     #clock-cells = <1>;
     clocks = <&clock_guard 0>;
     clock-names = "clock-guard";
   };

The driver usage exaple:

clk = devm_clk_get(dev, "clock-guard");
if (IS_ERR(clk))
	return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");

ret = clk_prepare_enable(clk);
if (ret) {
	dev_warn(dev, "Clock is not ready, %d\n", ret);
	return -EPROBE_DEFER;
}


> TBH, I don't understand your driver implementation either and why it has
> 
> +static const struct clk_ops clkctrl_guard_ops = {
> 
> +	.enable =	clkctrl_guard_enable,
> +	.disable =	clkctrl_guard_disable,
> +	.prepare =	clkctrl_guard_prepare,
> +	.unprepare =	clkctrl_guard_unprepare,
> +	.is_prepared =	clkctrl_guard_is_prepared,
> 
> any of these 4 implemented when you have no control over the clock.
> I didn't think it was required to call your parent clocks enables in
> your own enable either, thought that was handled by the core recursively
> calling clk_enable() on clk->parent. The one thing I would expect you to
> have implemented ops wise is is_enabled, which you don't have.
> Also no sign of any rate acquisition functions, which I thought were
> mandatory.
> 
> +	.get_parent =	clkctrl_guard_get_parent,
> +};

Good point on .is_enabled, I indeed missed that. As for the rate 
acquisition functions I referred to this table 
https://docs.kernel.org/driver-api/clk.html#id4 , and it see that 
.set_rate is actually optional.