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

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

On 26.03.2026 11:44, Conor Dooley wrote:
> On Thu, Mar 26, 2026 at 10:54:52AM +0100, Vyacheslav Yurkov wrote:
>> 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>;
> 
> Unfortunately, this doesn't contain the part that I wanted to see - who
> the providers of these clocks here actually are.
> 
> To be frank, I am not sure how this block would know that these clocks
> are enabled but their providers do not. I can think of a few ideas for
> how this block would know, but I don't understand why the providers
> themselves don't, and therefore why you need this gpio to tell you.
> 
>>      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.
> 
> .set_rate is not rate acquisition. .round_rate and .determine_rate are.
> I thought they were mandatory, but for a gate clock I guess they are not
> and the parent rate gets used automatically.

Before I send a v2 I'd like to clarify a few more things:
- I provided a schematics by means of the URL. I believe there's no 
unified way to provide something like that in the documentation, is 
there? So the only way to describe it properly would be to summarize the 
description from the mailing list, right?
- I'm going over the Common Clk Framework again, and perhaps I 
understood it wrong. You mentioned that I have to implement is_enabled, 
but I implemented is_prepared. It seems that I just have to move my 
is_prepared implementation to is_enabled. Does that sound correct?
- In my particular use case I don't need enable/disable ops, but to keep 
the driver generic, I'd probably want to have the bulk_enable 
implementation inside, because I don't know which clocks are assigned in 
a device tree. The clk_core_enable function only enables 1 parent clock, 
not the the list of parent clocks. Or I'm missing something here?

Thanks,
Slava