> 2023年5月27日 17:23，Conor Dooley <conor@xxxxxxxxxx> 写道：
>
> 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"?
Ah sorry I meant in this patch.
Since there won’t be any new ls1x chip that will boot Linux any time soon (due to
Loongson move away from MIPS but LoongArch32 is undefined for now), and
rest compatible strings are wide enough to cover their family, I think the present
compatible strings in this patch describes hardware best.
Thanks
- Jiaxun
>
> Cheers,
> Conor.
