Re: [PATCH V4 1/5] dt-bindings: rtc: Remove the LS2X from the trivial RTCs

[Binbin Zhou <zhoubb.aaron@gmail.com>]

On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Binbin,
>
> On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
> > Move Loongson RTC bindings from trivial-rtc.yaml into loongson,rtc.yaml.
> >
> > Also, we will discard the use of wildcards in compatible (ls2x-rtc),
> > soc-based compatible is more accurate for hardware differences between
> > chips.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  .../devicetree/bindings/rtc/loongson,rtc.yaml | 47 +++++++++++++++++++
> >  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
> >  2 files changed, 47 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> > new file mode 100644
> > index 000000000000..68e56829e390
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/loongson,rtc.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/rtc/loongson,rtc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Loongson Real-Time Clock
> > +
> > +maintainers:
> > +  - Binbin Zhou <zhoubinbin@loongson.cn>
> > +
> > +allOf:
> > +  - $ref: rtc.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - loongson,ls1b-rtc
> > +      - loongson,ls1c-rtc
> > +      - loongson,ls7a-rtc
> > +      - loongson,ls2k0500-rtc
> > +      - loongson,ls2k1000-rtc
> > +      - loongson,ls2k2000-rtc
>
> |+static const struct of_device_id loongson_rtc_of_match[] = {
> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> |+       { .compatible = "loongson,ls1c-rtc", .data = &ls1x_rtc_config },
> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> |+       { .compatible = "loongson,ls2k0500-rtc", .data = &generic_rtc_config },
> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> |+       { .compatible = "loongson,ls2k2000-rtc", .data = &generic_rtc_config },
> |+       { /* sentinel */ }
> |+};
>
> This is a sign to me that your compatibles here are could do with some
> fallbacks. Both of the ls1 ones are compatible with each other & there
> are three that are generic.
>
> I would allow the following:
> "loongson,ls1b-rtc"
> "loongson,ls1c-rtc", "loongson,ls1b-rtc"
> "loongson,ls7a-rtc"
> "loongson,ls2k0500-rtc", "loongson,ls7a-rtc"
> "loongson,ls2k2000-rtc", "loongson,ls7a-rtc"
> "loongson,ls2k1000-rtc"
>
> And then the driver only needs:
> |+static const struct of_device_id loongson_rtc_of_match[] = {
> |+       { .compatible = "loongson,ls1b-rtc", .data = &ls1x_rtc_config },
> |+       { .compatible = "loongson,ls7a-rtc", .data = &generic_rtc_config },
> |+       { .compatible = "loongson,ls2k1000-rtc", .data = &ls2k1000_rtc_config },
> |+       { /* sentinel */ }
> |+};
>
> And ~if~when you add support for more devices in the future that are
> compatible with the existing ones no code changes are required.

Hi Conor:

Thanks for your reply.

Yes, this is looking much cleaner. But it can't show every chip that
supports that driver.

As we know, Loongson is a family of chips:
ls1b/ls1c represent the Loongson-1 family of CPU chips;
ls7a represents the Loongson LS7A bridge chip;
ls2k0500/ls2k1000/ls2k2000 represent the Loongson-2 family of CPU chips.

Based on my previous conversations with Krzysztof, it seems that
soc-based to order compatible is more popular, so I have listed all
the chips that support that RTC driver.

>
> To maintain compatibility with the existing devicetrees, should the old
> "loongson,ls2x-rtc" be kept in the driver?

No, It seems that wildcards in compatible are not allowed."
loongson,ls2x-rtc" itself was part of this patch series at one time,
but apparently it is not the right way to describe these chips.

Here is Krzysztof's previous reply:
https://lore.kernel.org/linux-rtc/05ebf834-2220-d1e6-e07a-529b8f9cb100@linaro.org/

Thanks.
Binbin

>
> Thanks,
> Conor.
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    rtc_dev: rtc@1fe27800 {
> > +      compatible = "loongson,ls2k0500-rtc";
> > +      reg = <0x1fe27800 0x100>;
> > +
> > +      interrupt-parent = <&liointc1>;
> > +      interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
> > +    };
> > +
> > +...
> > diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > index a3603e638c37..9af77f21bb7f 100644
> > --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
> > @@ -47,8 +47,6 @@ properties:
> >        - isil,isl1218
> >        # Intersil ISL12022 Real-time Clock
> >        - isil,isl12022
> > -      # Loongson-2K Socs/LS7A bridge Real-time Clock
> > -      - loongson,ls2x-rtc
> >        # Real Time Clock Module with I2C-Bus
> >        - microcrystal,rv3029
> >        # Real Time Clock
> > --
> > 2.39.1
> >