Re: [PATCH v2 1/3] clk: nuvoton: ma35d1: Add missing WDT/WWDT parent clocks

[Miquel Raynal]

Hello Krzysztof,

>> Reported-by: Jacky Huang <ychuang570808@xxxxxxxxx>
>> Closes: https://lore.kernel.org/linux-clk/b8a8a65b-e302-4747-bc90-4920fc48795a@xxxxxxxxx/
>> Suggested-by: Jacky Huang <ychuang570808@xxxxxxxxx>
>> Fixes: 691521a367cf ("clk: nuvoton: Add clock driver for ma35d1 clock controller")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>> ---
>>  drivers/clk/nuvoton/clk-ma35d1.c               | 2 ++
>>  include/dt-bindings/clock/nuvoton,ma35d1-clk.h | 4 +++-
>
> Please do not combine patches.

I thought splitting these changes would not make sense in this (= a fix)
case, but fine.

[...]

>> @@ -248,6 +248,8 @@
>>  #define ADC_GATE	233
>>  #define EADC_DIV	234
>>  #define EADC_GATE	235
>> -#define	CLK_MAX_IDX	236
>> +#define PCLK3_DIV4096	236
>> +#define PCLK4_DIV4096	237
>> +#define	CLK_MAX_IDX	238
>
> This cannot be updated. If it has to, then not a binding and should be
> first dropped from the bindings like we did for most of the platforms
> having similar issue.

When you say "This cannot be updated", do we agree that you mean
"CLK_MAX_IDX cannot be updated"?

I'm not sure I fully understand the reasoning behind, because the whole
point of defining this CLK_MAX_IDX value is to let drivers know the
number of clocks, so it must obviously follow the updates in this
file. I cannot just drop it, the driver does not work without it. If we
enlarge that number, what is the problem as long as we keep the other
numbers identical?

Maybe one way to fix that mess is to:
1- Add the 2 new values (238 and 239) + a new define:
   NUVOTON_MA35D1_MAX_IDX 240
   238 would then have two definitions for a short period of time.
2- Replace CLK_MAX_IDX by NUVOTON_MA35D1_MAX_IDX in the driver
3- Drop CLK_MAX_IDX in the bindings.
or
3bis- Keep CLK_MAX_IDX in the bindings, but add a comment stating that
      it is a stale definition in step 2. This solution looks horrible
      :-)

Let me know what is the best way forward here, the goal being to merge
the big fix that I do care about (currently PATCH 2/3). Fixing the
missing watchdog clock definition has been asked by Jacky in v1, but I
will drop this part and keep v1 if it turns out to be too much churn.

Thanks,
Miquèl