Re: [PATCH v8 5/7] clk: nuvoton: ma35d1: Reparent SYSPLL correctly

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

On 12/10/2026 at 17:30:49 +02, Jerome Brunet <jbrunet@baylibre.com> wrote:

> On 2026-10-06 09:57 +0200, Miquel Raynal wrote:
>> On 05/10/2026 at 11:12:35 +02, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> 
>> > On Sun 04 Oct 2026 at 23:19, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>> >
>> >> On 04/10/2026 at 20:55:50 GMT, sashiko-bot@kernel.org wrote:
>> >>
>> >>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> >>> - [Low] clk: nuvoton: ma35d1: Modification of global static state
>> >>> during probe
>> >>
>> >> There is a single clock controller. Should never be a problem. I can
>> >> certainly move it in to more local scope if someone asks, but it's not
>> >> practically needed.
>> >
>> > I may be missing something key here but why is not in your defined
>> > indexes like the rest of the clocks ?
>> 
>> SYSPLL and its parent (HXT_GATE) are in the hws[] array. Are you
>> suggesting to use the hw_clk pointer directly there instead of using the
>> parent data structure as an envelope? Because the issue raised is the
>> global definition of the struct clk_parent_data and then its
>> modification (since I need to reference the hws[SYSPLL] pointer inside).
>
> The problem sashiko is pointing at is that you are creating a global and then use
> it as a local variable, just assigning the the .hw
>
> So 2 possibilities:
> * You just need the .hw then I don't really get why you are doing it differently
>   for this clock ?

True.

> * OR, this is one of the those clock where you want to check DT first then

It is not.

>   fallback to a fixed clock, then yes you need parent_data. don't forget
>   the fw_name (which I did not see here) and use a local variable for that,
>   not a global
>
> Did I miss something ?

You got it absolutely right, and while implementing the change I
remember why I used the name instead of the hw pointer:

     clk_hw_register_fixed_rate_parent_hw() exists, but
devm_clk_hw_register_fixed_rate_parent_hw() did not.

So since it looks like most of these helpers have their devm_
counterpart, I will add this new helper in clk-provider.h. I plan on
doing that addition in this commit because I think that is what Boyd
used to prefer, but I can add it in a preparation patch that is your own
preference, I don't mind.

Thanks for all the guidance!
Miquèl