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

Commit Message

Sia Jee Heng Nov. 29, 2023, 6 a.m. UTC

Add new compatible string for UART in the StarFive JH8100 SoC.

Signed-off-by: Sia Jee Heng <[e-mail]>
Reviewed-by: Ley Foon Tan <[e-mail]>
---
 Documentation/devicetree/bindings/serial/cdns,uart.yaml | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Krzysztof Kozlowski Nov. 29, 2023, 8:26 a.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Ley Foon Tan <[e-mail]>

The patch is quite different than v1. Are you sure the review is
applicable? If it was given for v2, where is it?

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

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


Best regards,
Krzysztof

Sia Jee Heng Nov. 29, 2023, 10:33 a.m. UTC | #2

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Wednesday, November 29, 2023 4:26 PM
> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Michael Zhu <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
> <[e-mail]>
> 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 <[e-mail]>
> > Reviewed-by: Ley Foon Tan <[e-mail]>
> 
> 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.
> 
> > ---
> >  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 ?
> 
> > +        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
      - description: UART controller for Zynq-7xxx SoC
        items:
          - const: xlnx,xuartps
          - const: cdns,uart-r1p8
      - description: UART controller for Zynq Ultrascale+ MPSoC
        items:
          - const: xlnx,zynqmp-uart
          - const: cdns,uart-r1p12
> 
> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski Nov. 29, 2023, 10:53 a.m. UTC | #3

On 29/11/2023 11:33, JeeHeng Sia wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Wednesday, November 29, 2023 4:26 PM
>> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Michael Zhu <[e-mail]>; [e-mail]
>> Cc: [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
>> <[e-mail]>
>> 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 <[e-mail]>
>>> Reviewed-by: Ley Foon Tan <[e-mail]>
>>
>> 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

Sia Jee Heng Nov. 29, 2023, 1:15 p.m. UTC | #4

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Wednesday, November 29, 2023 6:53 PM
> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Michael Zhu <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
> <[e-mail]>
> Subject: Re: [PATCH v2 5/6] dt-bindings: serial: cdns: Add new compatible string for StarFive JH8100 UART
> 
> On 29/11/2023 11:33, JeeHeng Sia wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Wednesday, November 29, 2023 4:26 PM
> >> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Michael Zhu <[e-mail]>; [e-mail]
> >> Cc: [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
> >> <[e-mail]>
> >> 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 <[e-mail]>
> >>> Reviewed-by: Ley Foon Tan <[e-mail]>
> >>
> >> 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.
I am sorry, but may I know the preferred length of characters?
> 
> How does this answer my concern about review tag?
My bad. I should have obtained your consensus before proceeding with the changes
to this patch, especially after dropping patch 5 as requested in version 1.
> 
> Do you understand that my comments are inline under the exact line which
> is questioned?
Yes, lesson learned.
> 
> >>
> >>> ---
> >>>  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"
Ok.
> 
> >>
> >>> +        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.
Noted.
> 
> 
> 
> Best regards,
> Krzysztof

Patch

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
+        items:
+          - const: starfive,jh8100-uart
+          - const: cdns,uart-r1p8
 
   reg:
     maxItems: 1