> 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.
>
> 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
My recommendation is leaving compatible string as is.
Thanks
- Jiaxun
>
> Thanks.
> Binbin
