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

[Alexander Dahl <ada@thorsis.com>]

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 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
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.

Thanks and greets
Alex

> 
> 
> Best regards,
> Krzysztof