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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5867 bytes --]

On Sat, May 27, 2023 at 05:13:39PM +0100, Jiaxun Yang wrote:
> > 2023年5月27日 10:22，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.
> > 
> > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]