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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG40kxHaAwGowQ0dRoEkGSiAUJA5SyKw3SSECUmBApKaAjcHKw@mail.gmail.com>
Date: Fri, 21 Jun 2024 19:59:06 +0900
From: Hironori KIKUCHI <kikuchan98@...il.com>
To: Krzysztof Kozlowski <krzk@...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 v1 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

Hello Krzysztof,

Thank you for your reply!

On Tue, Jun 18, 2024 at 6:17 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 18/06/2024 10:15, Hironori KIKUCHI wrote:
> > The RG28XX panel is a panel specific to the Anbernic RG28XX.
> > It is 2.8 inches in size (diagonally) with a resolution of 480x640.
> >
> > Signed-off-by: Hironori KIKUCHI <kikuchan98@...il.com>
> > ---
> >  .../display/panel/sitronix,st7701.yaml        | 36 +++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
>
> Nothing explains in the commit msg why rg28xx is actually st7701.
> Changing interface to SPI suggests it is not.

Thanks, I'll explain like this;
---
dt-bindings: display: st7701: Add Anbernic RG28XX panel

The RG28XX panel is a panel specific to the Anbernic RG28XX
handheld device. It is 2.8 inches in size (diagonally) with a
resolution of 480x640.

This panel is driven by a variant of 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, it is connected over SPI, instead of MIPI DSI. So
add and modify for SPI as well.
---

>
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> > index b348f5bf0a9..04f6751ccca 100644
> > --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7701.yaml
> > @@ -22,19 +22,21 @@ description: |
> >
> >  allOf:
> >    - $ref: panel-common.yaml#
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >
> >  properties:
> >    compatible:
> >      items:
> >        - enum:
> >            - anbernic,rg-arc-panel
> > +          - anbernic,rg28xx-panel
>
> What is xx? Wildcards are not allowed, in general.
>
> Can it be anything else than panel? If not, then drop "-panel".

It's supprising but it actually is a product name of the handheld device...
The panel comes with the device, and part# is completely unknown.

>
>
> >            - 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 +45,13 @@ properties:
> >    IOVCC-supply:
> >      description: I/O system regulator
> >
> > +  dc-gpios:
> > +    maxItems: 1
> > +    description: |
>
> Do not need '|' unless you need to preserve formatting.

Thanks, I'll remove it.

>
> > +      Controller data/command selection (D/CX) in 4-line SPI mode.
> > +      If not set, the controller is in 3-line SPI mode.
> > +      No effect for DSI.
>
> Which devices can be connected over SPI? It seems not all, so this
> should be disallowed (": false" in allOf:if:then:; move the allOf to
> bottom like in example-schema) for them.

Hmm... That's a difficult question...

There are 3 types of connection that trying to support:
DSI, SPI with D/CX pin, and SPI without D/CX pin.

The dc-gpios is required for SPI with D/CX pin, but not for others.

DSI:
- anbernic,rg-arc-panel
- densitron,dmt028vghmcmi-1a
- elida,kd50t048a
- techstar,ts8550b

SPI without D/CX pin:
- anbernic,rg28xx-panel

But, there are no panels with D/CX pin so far.
How should I deal with this? just disallow all, perhaps?


BTW, does panel's compatible string consider to include it's interface?
ie, what if two panels use the exact same commands and timings, but
over different interface,
... are they "compatible" or not?

>
> Best regards,
> Krzysztof
>

Best regards,
kikuchan.