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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 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




[Index of Archives]     [LKML Archive]     [Linux ARM Kernel]     [Linux ARM]     [Git]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux