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

[Conor Dooley <conor@kernel.org>]

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

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

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