Re: [PATCH v2 2/3] dt-bindings: soc: samsung: usi: add USIv1 and samsung,exynos8895-usi

[Krzysztof Kozlowski]

On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote:
>  
>    reg:
>      maxItems: 1
> @@ -64,7 +75,6 @@ properties:
>  
>    samsung,mode:
>      $ref: /schemas/types.yaml#/definitions/uint32
> -    enum: [0, 1, 2, 3]

Widest constraints stay here, so minimum/maximum.

>      description:
>        Selects USI function (which serial protocol to use). Refer to
>        <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values.
> @@ -101,18 +111,42 @@ required:
>    - samsung,sysreg
>    - samsung,mode
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - samsung,exynos850-usi
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 1
> +
> +        samsung,mode:
> +          enum: [0, 1, 2, 3]
> +
> +      required:
> +        - reg
> +
> +    else:
> +      properties:
> +        reg: false
> +        samsung,clkreq-on: false
> +
> +        samsung,mode:
> +          enum: [4, 5, 6, 7, 8, 9, 10]

Is it really true? Previously for example GS101 had values 0-3, now you
claim has values 4-10, so an ABI change without explanation.

> +
>  if:

Missing allOf:

>    properties:
>      compatible:
>        contains:
>          enum:
>            - samsung,exynos850-usi
> +          - samsung,exynos8895-usi

Effect is not readable and not correct. You have two if:then:else.
Usually it is easier to just have separate if: for each group of
variants and define/constrain complete for such group within its if:.

>  
>  then:
>    properties:
> -    reg:
> -      maxItems: 1
> -
>      clocks:
>        items:
>          - description: Bus (APB) clock
> @@ -122,16 +156,13 @@ then:
>        maxItems: 2
>  
>    required:
> -    - reg
>      - clocks
>      - clock-names
>  
>  else:
>    properties:
> -    reg: false
>      clocks: false
>      clock-names: false
> -    samsung,clkreq-on: false
>  
>  additionalProperties: false
>  
> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
> index a01af169d..4c077c9a8 100644
> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
> @@ -13,5 +13,12 @@
>  #define USI_V2_UART		1
>  #define USI_V2_SPI		2
>  #define USI_V2_I2C		3
> +#define USI_V1_NONE		4

Drop, it is already there.

> +#define USI_V1_I2C0		5
> +#define USI_V1_I2C1		6
> +#define USI_V1_I2C0_1		7
> +#define USI_V1_SPI		8

Drop

> +#define USI_V1_UART		9

Drop

> +#define USI_V1_UART_I2C1	10


Best regards,
Krzysztof