Re: [PATCH v2 5/6] dt-bindings: serial: cdns: Add new compatible string for StarFive JH8100 UART

[Krzysztof Kozlowski]

On 29/11/2023 11:33, JeeHeng Sia wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>> Sent: Wednesday, November 29, 2023 4:26 PM
>> To: JeeHeng Sia <jeeheng.sia@xxxxxxxxxxxxxxxx>; kernel@xxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
>> krzk@xxxxxxxxxx; conor+dt@xxxxxxxxxx; paul.walmsley@xxxxxxxxxx; palmer@xxxxxxxxxxx; aou@xxxxxxxxxxxxxxxxx;
>> daniel.lezcano@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; conor@xxxxxxxxxx; anup@xxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
>> jirislaby@xxxxxxxxxx; michal.simek@xxxxxxx; Michael Zhu <michael.zhu@xxxxxxxxxxxxxxxx>; drew@xxxxxxxxxxxxxxx
>> Cc: devicetree@xxxxxxxxxxxxxxx; linux-riscv@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Leyfoon Tan
>> <leyfoon.tan@xxxxxxxxxxxxxxxx>
>> Subject: Re: [PATCH v2 5/6] dt-bindings: serial: cdns: Add new compatible string for StarFive JH8100 UART
>>
>> On 29/11/2023 07:00, Sia Jee Heng wrote:
>>> Add new compatible string for UART in the StarFive JH8100 SoC.
>>>
>>> Signed-off-by: Sia Jee Heng <jeeheng.sia@xxxxxxxxxxxxxxxx>
>>> Reviewed-by: Ley Foon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>
>>
>> The patch is quite different than v1. Are you sure the review is
>> applicable? If it was given for v2, where is it?
> This patch is impacted by the comment suggesting the exclusion of patch 5 in V1. In V2, this patch adds compatible for cdns-uart-r1p8, allowing us to continue using the cdns uart.

Please wrap your replies.

How does this answer my concern about review tag?

Do you understand that my comments are inline under the exact line which
is questioned?

>>
>>> ---
>>>  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
>>> index e35ad1109efc..0d05305778f2 100644
>>> --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
>>> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
>>> @@ -20,6 +20,10 @@ properties:
>>>          items:
>>>            - const: xlnx,zynqmp-uart
>>>            - const: cdns,uart-r1p12
>>> +      - description: UART controller for StarFive JH8100 SoC
>>
>> This is duplicating compatible, drop.
> Do you mean drop compatible for starfive,jh8100-uart ?

No, drop description and use directly " - items"

>>
>>> +        items:
>>> +          - const: starfive,jh8100-uart
>>> +          - const: cdns,uart-r1p8
>>
>> Don't add things to the end of the list, but keep order. I would suggest
>> to put it at the beginning, so before Xilinx.
> I'm trying to get what you're asking, but it's a bit confusing for me. So, I thought it might be easier if I just share the code below. Please let me know if this addresses your comment?
> properties:
>   compatible:
>     oneOf:
>       - description: UART controller for StarFive JH8100 SoC
>         items:
>           - const: cdns,uart-r1p8

Order is fixed, thanks. But drop description and bring back specific
compatible. You must have specific compatibles, always.



Best regards,
Krzysztof