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