Message-ID: <CAG40kxHxnSNp1_4fX0fOWypLunPm-NnH1UEKZNQgM0g-Z1u1DQ@mail.gmail.com>
Date: Tue, 2 Jul 2024 02:28:27 +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

Hi Conor,

On Sun, Jun 30, 2024 at 11:17 PM Conor Dooley <conor@...nel.org> wrote:
>
> 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?

Sorry for the confusion.
I think it depends on what the "panel" refers to...
Although the "panel driver IC" (ST7701 variant) is capable of doing
both, the "panel assy" (including its cable) of the RG28XX only has an
SPI interface in its pinout.
If the compatible string "rg28xx-panel" represents the assy, then I
could say the "panel" only supports SPI, and the other panels only
support DSI.
But if it only represents a specific LCD panel and its driver IC with
control parameters, then it theoretically supports both, and it might
be sufficient to just include spi-peripheral-props, as in v1.
v1: https://lore.kernel.org/linux-kernel/20240618081515.1215552-2-kikuchan98@gmail.com/

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

Best regards,
kikuchan.

Powered by blists - more mailing lists