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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4596 bytes --]

On Fri, Jun 28, 2024 at 02:10:15PM +0900, Hironori KIKUCHI wrote:
> The RG28XX panel is a display panel of the Anbernic RG28XX, a handheld
> gaming device from Anbernic. It is 2.8 inches in size (diagonally) with
> a resolution of 480x640.
> 
> This panel is driven by a variant of the ST7701 driver IC internally,
> confirmed by dumping and analyzing its BSP initialization sequence
> by using a logic analyzer. It is very similar to the existing
> densitron,dmt028vghmcmi-1a panel, but differs in some unknown
> register values, so add a new entry for the panel to distinguish them.
> 
> Additionally, the panel is connected via SPI instead of MIPI DSI.
> So add and modify for SPI as well.
> 
> Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com>
> ---
>  .../display/panel/sitronix,st7701.yaml        | 69 +++++++++++++++++--
>  1 file changed, 64 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> index b348f5bf0a9..835ea436531 100644
> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> @@ -20,21 +20,19 @@ description: |
>    Densitron DMT028VGHMCMI-1A is 480x640, 2-lane MIPI DSI LCD panel
>    which has built-in ST7701 chip.
>  
> -allOf:
> -  - $ref: panel-common.yaml#
> -
>  properties:
>    compatible:
>      items:
>        - enum:
>            - anbernic,rg-arc-panel
> +          - anbernic,rg28xx-panel

Please no wildcards in compatibles - what is the actual model of this
panel? I don't really want to see the model of the handheld here if
possible.

>            - 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 +41,13 @@ properties:
>    IOVCC-supply:
>      description: I/O system regulator
>  
> +  dc-gpios:
> +    maxItems: 1
> +    description:
> +      Controller data/command selection (D/CX) in 4-line SPI mode.
> +      If not set, the controller is in 3-line SPI mode.
> +      Disallowed for DSI.
> +
>    port: true
>    reset-gpios: true
>    rotation: true
> @@ -57,7 +62,38 @@ required:
>    - port
>    - reset-gpios
>  
> -additionalProperties: false
> +allOf:
> +  - $ref: panel-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            # SPI connected panels
> +            enum:
> +              - anbernic,rg28xx-panel
> +    then:
> +      $ref: /schemas/spi/spi-peripheral-props.yaml

I'm not really keen on this. I'd rather see a different binding for the
SPI version compared to the MIPI ones. Is doing things like this common
for panels? If it is, I'll turn a blind eye..

> +
> +  - if:
> +      properties:
> +        compatible:
> +          not:
> +            contains:
> +              # DSI or SPI without D/CX pin
> +              enum:
> +                - anbernic,rg-arc-panel
> +                - anbernic,rg28xx-panel
> +                - densitron,dmt028vghmcmi-1a
> +                - elida,kd50t048a
> +                - techstar,ts8550b

This is all the compatibles in the file, so nothing is allowed to use
dc-gpios? Why bother adding it?

Confused,
Conor.

> +    then:
> +      required:
> +        - dc-gpios
> +    else:
> +      properties:
> +        dc-gpios: false
> +
> +unevaluatedProperties: false
>  
>  examples:
>    - |
> @@ -82,3 +118,26 @@ examples:
>              };
>          };
>      };
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        panel@0 {
> +            compatible = "anbernic,rg28xx-panel", "sitronix,st7701";
> +            reg = <0>;
> +            spi-max-frequency = <3125000>;
> +            VCC-supply = <&reg_lcd>;
> +            IOVCC-supply = <&reg_lcd>;
> +            reset-gpios = <&pio 8 14 GPIO_ACTIVE_HIGH>; /* LCD-RST: PI14 */
> +            backlight = <&backlight>;
> +
> +            port {
> +                panel_in_rgb: endpoint {
> +                    remote-endpoint = <&tcon_lcd0_out_lcd>;
> +                };
> +            };
> +        };
> +    };
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]