Re: [PATCH v1 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/06/2024 10:15, Hironori KIKUCHI wrote:
> The RG28XX panel is a panel specific to the Anbernic RG28XX.
> It is 2.8 inches in size (diagonally) with a resolution of 480x640.
> 
> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> ---
>  .../display/panel/sitronix,st7701.yaml        | 36 +++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)

Nothing explains in the commit msg why rg28xx is actually st7701.
Changing interface to SPI suggests it is not.

> 
> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> index b348f5bf0a9..04f6751ccca 100644
> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> @@ -22,19 +22,21 @@ description: |
>  
>  allOf:
>    - $ref: panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>  
>  properties:
>    compatible:
>      items:
>        - enum:
>            - anbernic,rg-arc-panel
> +          - anbernic,rg28xx-panel

What is xx? Wildcards are not allowed, in general.

Can it be anything else than panel? If not, then drop "-panel".


>            - densitron,dmt028vghmcmi-1a
>            - elida,kd50t048a
>            - techstar,ts8550b
>        - const: sitronix,st7701
>  
>    reg:
> -    description: DSI virtual channel used by that screen
> +    description: DSI / SPI channel used by that screen
>      maxItems: 1
>  
>    VCC-supply:
> @@ -43,6 +45,13 @@ properties:
>    IOVCC-supply:
>      description: I/O system regulator
>  
> +  dc-gpios:
> +    maxItems: 1
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Controller data/command selection (D/CX) in 4-line SPI mode.
> +      If not set, the controller is in 3-line SPI mode.
> +      No effect for DSI.

Which devices can be connected over SPI? It seems not all, so this
should be disallowed (": false" in allOf:if:then:; move the allOf to
bottom like in example-schema) for them.

Best regards,
Krzysztof