diff mbox series

[v1,1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

Message ID 20240618081515.1215552-2-kikuchan98@gmail.com
State Changes Requested
Headers show
Series drm/panel: st7701: Add Anbernic RG28XX panel support | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Hironori KIKUCHI June 18, 2024, 8:15 a.m. UTC
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(-)

Comments

Krzysztof Kozlowski June 18, 2024, 9:17 a.m. UTC | #1
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
Hironori KIKUCHI June 21, 2024, 10:59 a.m. UTC | #2
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.
Krzysztof Kozlowski June 21, 2024, 3:17 p.m. UTC | #3
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 mbox series

Patch

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 = <&reg_lcd>;
+            IOVCC-supply = <&reg_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>;
+                };
+            };
+        };
+    };