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 8:07 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Fri, May 26, 2023 at 09:37:02AM +0800, Binbin Zhou wrote:
> > On Fri, May 26, 2023 at 1:05 AM Conor Dooley <conor@kernel.org> wrote:
> > > On Thu, May 25, 2023 at 08:55:23PM +0800, Binbin Zhou wrote:
>
> >> > > +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.
>
> Right. You don't actually have to list them all *in the driver* though,
> just in the binding and in the devicetree. I think what you have missed
> is:
> > > 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"
>
> This is what you would put in the compatible section of a devicetree
> node, using "fallback compatibles". So for a ls1c you put in
> compatible = "loongson,ls1c-rtc", "loongson,ls1b-rtc";
> and the kernel first tries to find a driver that supports
> "loongson,ls1c-rtc" but if that fails it tries to find one that supports
> "loongson,ls1b-rtc". This gives you the best of both worlds - you can
> add support easily for new systems (when an ls1d comes out, you don't
> even need to change the driver for it to just work!) and you have a
> soc-specific compatible in case you need to add some workaround for
> hardware errata etc in the future.

Hi Conor:

I seem to understand what you are talking about.
I hadn't delved into "fallback compatibles" before, so thanks for the
detailed explanation.

In fact, I have thought before if there is a good way to do it other
than adding comptable to the driver frequently, and "fallback
compatibles" should be the most suitable.

So in the dt-bindings file, should we just write this:

  compatible:
    oneOf:
      - items:
          - enum:
              - loongson,ls1c-rtc
          - const: loongson,ls1b-rtc
      - items:
          - enum:
              - loongson,ls2k0500-rtc
              - loongson,ls2k2000-rtc
          - const: loongson,ls7a-rtc
      - items:
          - const: loongson,ls2k1000-rtc

Thanks.
Binbin

>
> > > 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.
>
> Right, but it has been merged - you are deleting the driver that supports
> it after all - which means that any dtb with the old compatible will
> stop working.
> I don't disagree with Krzysztof that having wildcard based compatibles
> is bad, but I do not think that regressing rtc support for systems with
> these old devicetrees is the right way to go either.
>
> Thanks,
> Conor.