Re: [PATCH v2 01/16] dt-bindings: clock: at91: Split up per SoC partially

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/02/2025 08:16, Alexander Dahl wrote:
> Hello everyone,
> 
> Am Mon, Feb 10, 2025 at 06:05:31PM +0100 schrieb Krzysztof Kozlowski:
>> On 10/02/2025 17:44, Alexander Dahl wrote:
>>> Before adding even more new indexes creating more holes in the
>>> clk at91 drivers pmc_data->chws arrays, split this up.
>>>
>>> This is a partial split up only for SoCs affected by upcoming changes
>>> and by that PMC_MAIN + x hack, others could follow by the same scheme.
>>>
>>> Binding splitup was proposed for several reasons:
>>>
>>> 1) keep the driver code simple, readable, and efficient
>>> 2) avoid accidental array index duplication
>>> 3) avoid memory waste by creating more and more unused array members.
>>>
>>> Old values are kept to not break dts, and to maintain dt ABI.
>>>
>>> Link: https://lore.kernel.org/linux-devicetree/20250207-jailbird-circus-bcc04ee90e05@thorsis.com/T/#u
>>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
>>> ---
>>>
>>> Notes:
>>>     v2:
>>>     - new patch, not present in v1
>>>
>>>  .../dt-bindings/clock/microchip,sam9x60-pmc.h | 19 +++++++++++
>>>  .../dt-bindings/clock/microchip,sam9x7-pmc.h  | 25 +++++++++++++++
>>>  .../clock/microchip,sama7d65-pmc.h            | 32 +++++++++++++++++++
>>>  .../dt-bindings/clock/microchip,sama7g5-pmc.h | 24 ++++++++++++++
>>>  4 files changed, 100 insertions(+)
>>>  create mode 100644 include/dt-bindings/clock/microchip,sam9x60-pmc.h
>>>  create mode 100644 include/dt-bindings/clock/microchip,sam9x7-pmc.h
>>>  create mode 100644 include/dt-bindings/clock/microchip,sama7d65-pmc.h
>>>  create mode 100644 include/dt-bindings/clock/microchip,sama7g5-pmc.h
>>>
>>> diff --git a/include/dt-bindings/clock/microchip,sam9x60-pmc.h b/include/dt-bindings/clock/microchip,sam9x60-pmc.h
>>> new file mode 100644
>>> index 0000000000000..e01e867e8c4da
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/microchip,sam9x60-pmc.h
>>> @@ -0,0 +1,19 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>>> +/*
>>> + * The constants defined in this header are being used in dts and in
>>> + * at91 sam9x60 clock driver.
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H
>>> +#define _DT_BINDINGS_CLOCK_MICROCHIP_SAM9X60_PMC_H
>>> +
>>> +#include <dt-bindings/clock/at91.h>
>>> +
>>> +/* old from before bindings splitup */
>>> +#define SAM9X60_PMC_MCK		PMC_MCK		/* 1 */
>>> +#define SAM9X60_PMC_UTMI	PMC_UTMI	/* 2 */
>>> +#define SAM9X60_PMC_MAIN	PMC_MAIN	/* 3 */
>>> +
>>> +#define SAM9X60_PMC_PLLACK	PMC_PLLACK	/* 7 */
>>
>> IIUC, you want to have bindings per SoC, so why not adding proper
>> constants here instead of including entire old binding header? The
>> binding header should be entirely abandoned later.
> 
> Which binding header should be abandoned entirely?

The one you claim to split. I assume it is the same  one included here.

> 
> The bindings per SoC idea was proposed in series v1 feedback.  I'm
> neither a binding nor a clock expeert.  As far as I understood it's
> important to keep the exact same values as before to not change any

Yes, I did not propose to change any IDs.

> ABI.  The non SoC specific values are still used on older SoCs of the
> at91 family, so this is why I used the old constants for now.
> 
> These PMC indexes are not the only definitions in
> dt-bindings/clock/at91.h however, there are more which are not SoC
> specific.
> 
> I'd like some thoughts from the Microchip maintainers here,
> what's their idea on how to proceed with the at91 clock stuff?
> 
> This works for me, but the current state is more or less still an idea
> as base for discussion.  Please don't make it overly complicated, this
> is not the primary focus of my work.

You made this binding more complicated than it should be - instead of
simple list of clocks you include some other file and split between old
and new.

Best regards,
Krzysztof