| Message ID | 20240618081515.1215552-2-kikuchan98@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | drm/panel: st7701: Add Anbernic RG28XX panel support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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@gmail.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. > > 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". > - 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. > + 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. Best regards, Krzysztof
Hello Krzysztof, Thank you for your reply! On Tue, Jun 18, 2024 at 6:17 PM Krzysztof Kozlowski <krzk@kernel.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@gmail.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.
On 21/06/2024 12:59, Hironori KIKUCHI wrote: > Hello Krzysztof, > > Thank you for your reply! > > On Tue, Jun 18, 2024 at 6:17 PM Krzysztof Kozlowski <krzk@kernel.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@gmail.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. > --- OK. > >> >>> >>> 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. OK > >> >> >>> - 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? You can disallow for all existing panels, if you are unsure. > > > BTW, does panel's compatible string consider to include it's interface? No, the compatible defines the device, not its wiring (bus). The parent node defines which bus is needed. > ie, what if two panels use the exact same commands and timings, but > over different interface, > ... are they "compatible" or not? Best regards, Krzysztof
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 - 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: | + 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. + port: true reset-gpios: true rotation: true @@ -57,7 +66,7 @@ required: - port - reset-gpios -additionalProperties: false +unevaluatedProperties: false examples: - | @@ -82,3 +91,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>; + }; + }; + }; + };
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@gmail.com> --- .../display/panel/sitronix,st7701.yaml | 36 +++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)