Re: [PATCH 3/8] dt-bindings: serial: fsl-linflexuart: Add compatible for S32G2

[Andreas Färber]

On 05.08.21 08:54, Chester Lin wrote:
> Add a compatible string for the uart binding of NXP S32G2 platforms. Here
> we use "s32v234-linflexuart" as fallback since the current linflexuart
> driver still works on S32G2.
> 
> Signed-off-by: Chester Lin <clin@xxxxxxxx>
> ---
>  .../bindings/serial/fsl,s32-linflexuart.yaml  | 26 ++++++++++++++++---
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> index acfe34706ccb..e731f3f6cea4 100644
> --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> @@ -21,10 +21,20 @@ allOf:
>  
>  properties:
>    compatible:
> -    description: The LINFlexD controller on S32V234 SoC, which can be
> -      configured in UART mode.
> -    items:
> -      - const: fsl,s32v234-linflexuart
> +    minItems: 1
> +    maxItems: 2

Are these necessary for oneOf?

> +    oneOf:
> +      - description: The LINFlexD controller on S32G2 SoC, which can be
> +          configured in UART mode.
> +        items:
> +          - enum:
> +              - fsl,s32g2-linflexuart
> +          - const: fsl,s32v234-linflexuart

This reads inconsistent to me: Either this oneOf is for S32G2 only, then
please turn the enum into a const. Or change the description to "on SoCs
compatible with S32V234" if we expect the enum list to grow.

I believe the idea here was to avoid unnecessary driver compatible and
earlycon compatible additions, while preparing for eventual quirks
specific to S32G2.

@NXP: Should this be s32g2- like above or s32g274a- specifically? Do you
agree this is a useful thing to prepare here, as opposed to using only
s32v234- in the s32g2* DT?

I assume the ordering is done alphabetically as S32G < S32V;
alternatively we might order S32V234 first and then the compatible ones.

> +
> +      - description: The LINFlexD controller on S32V234 SoC, which can be
> +          configured in UART mode.
> +        items:
> +          - const: fsl,s32v234-linflexuart

To minimize this S32G2 patch, would it be valid to do oneOf for the
single S32V in the preceding patch already? Then we would avoid the text
movement and re-indentation above and more easily see the lines newly
getting added for S32G2.

>  
>    reg:
>      maxItems: 1
> @@ -41,8 +51,16 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    /* S32V234 */

Could this be:
  - description: S32V234
    |
?

>      serial@40053000 {
>          compatible = "fsl,s32v234-linflexuart";
>          reg = <0x40053000 0x1000>;
>          interrupts = <0 59 4>;
>      };
> +
> +    /* S32G2 */

This should not be part of the S32V example, but a new one:

  - |

(or with description, as discussed above)

> +    serial@401c8000 {
> +        compatible = "fsl,s32g2-linflexuart", "fsl,s32v234-linflexuart";

Potentially affected by naming discussions above.

> +        reg = <0x401c8000 0x3000>;
> +        interrupts = <0 82 1>;
> +    };

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)