On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
> > 2023年5月27日 10:22，Binbin Zhou <zhoubb.aaron@xxxxxxxxx> 写道：
> > On Fri, May 26, 2023 at 8:07 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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.
> >
> > 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:
Not quite, because you still need to allow for ls1b-rtc and ls7a-rtc
appearing on their own. That's just two more entries like the
ls2k1000-rtc one.
> >
> > 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
This one is just "const:", you don't need "items: const:".
I didn't test this, but I figure it would be:
compatible:
oneOf:
- items:
- enum:
- loongson,ls1c-rtc
- const: loongson,ls1b-rtc
- items:
- enum:
- loongson,ls2k0500-rtc
- loongson,ls2k2000-rtc
- const: loongson,ls7a-rtc
- const: loongson,ls1b-rtc
- const: loongson,ls2k1000-rtc
- const: loongson,ls7a-rtc
> My recommendation is leaving compatible string as is.
"as is" meaning "as it is right now in Linus' tree", or "as it is in
this patch"?
Cheers,
Conor.
Attachment:
signature.asc
Description: PGP signature
