Hello Conor,
Thank you for your reply.
On Sat, Jun 29, 2024 at 1:27 AM Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > .../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 = <®_lcd>;
> > + IOVCC-supply = <®_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.
