Date: Sun, 30 Jun 2024 15:17:10 +0100
From: Conor Dooley <conor@...nel.org>
To: Hironori KIKUCHI <kikuchan98@...il.com>
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

On Sat, Jun 29, 2024 at 05:26:56PM +0900, Hironori KIKUCHI wrote:
> 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...

Ah, I see. I didn't realise that.

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

The commit message sounded like the panel was capable of doing SPI
instead of DSI, is that not the case and the panel is actually capable
of doing both, just happens to be connected as SPI in this particular
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.

To be honest, I'd just delete this complication until something arrives
that actually uses that pin.

Cheers,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists