Re: [PATCH v8 3/7] clk: nuvoton: ma35d1: Retrieve HXT/LXT from DT when possible

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Jerome,

>>>> -	hws[HXT] = ma35d1_clk_fixed("hxt", 24000000);
>>>
>>> [Severity: Critical]
>>> Does this unconditional removal break ABI compatibility with older device
>>> trees?
>>>
>>> Because HXT (index 0) is exported as a public ABI in the DT bindings,
>>> removing its explicit registration leaves hws[HXT] as NULL in the provider's
>>> clock array. If an existing downstream or older device tree consumer explicitly
>>> references this clock via clocks = <&clk HXT>, won't the provider's
>>> of_clk_hw_onecell_get() fail to resolve the clock and return an error?
>>
>> Jerome, your wisdom matters here :-) For me this is a false positive,
>> because nobody outside this driver should access hws[HXT] and the clock
>> is still referenced through the DT, just not provided by this
>> driver. For me this is not an issue we should consider, let me know
>> otherwise.
>
> Strictly speaking there is (used to be) a clock at this index so it is
> not a false positive. Someone crazy enough could have gone through here
> to get the OSC even if does not make a lot of sense.
>
> This clock should never have existed to start with but it got in the
> bindings ... and that is the pain of dealing with bad bindings :/
>
> You've got 2 options:
> * Removing the ID from the bindings explaining clearly that it conflicts
>   with the fact this clock is actually an input according to the same
>   bindings. I suspect the DT maintainers won't like this much ...
>
> * Keep the bad support binding in the driver: You'll need a pass-through
>   clock element where the parent data is the fw_name and a fallback to
>   an `hxt-fake-internal' fixed clock. Feel free to add a comment
>   explaining how bad it is to use that clock and what user should be
>   doing instead.

Too bad. I am pretty sure nobody ever used that yet (since it is both
very recent and fundamentally broken) but okay, I'll keep registering a
dummy fixed clock there in case anybody ever cares about that lookup.

> You'll probably need to play that last trick with the lxt anyway ?

Yes, it is already handled indeed.

Thanks!
Miquèl