Message-ID: <CAG40kxERY2r2cj58kjVMMg1JVOChRKraKYFo_K5C5fnx0g80Qw@mail.gmail.com>
Date: Sat, 29 Jun 2024 17:26:56 +0900
From: Hironori KIKUCHI <kikuchan98@...il.com>
To: Conor Dooley <conor@...nel.org>
Cc: linux-kernel@...r.kernel.org, Jagan Teki <jagan@...rulasolutions.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Sam Ravnborg <sam@...nborg.org>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

Hello Conor,

Thank you for your reply.

On Sat, Jun 29, 2024 at 1:27 AM Conor Dooley <conor@...nel.org> wrote:
>
> 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@...il.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.

Well, the "RG28XX" is the actual product name of the device...
Besides, there is no vendor name or model name on the panel; there is
no information at all.
Since the panel cannot be disassembled from the housing of the device,
I named it like this.

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

This might be the first case that a driver supports both DSI and SPI
for a panel.
The panel can be either a DSI device or an SPI device.
I'm not sure if this is the right way to represent it in the documentation...

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

There are 3 types of connections that the driver supports: DSI, SPI
with D/CX pin, and SPI without D/CX pin.
Although most SPI panels don't have a D/CX pin, theoretically "SPI
with D/CX pin" exists.
So, it's just prepared for that.

IMHO, once it's found, the list should be negated. List panels for SPI
with D/CX pin, instead.

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

Best regards,
kikuchan.

Powered by blists - more mailing lists