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

[Krzysztof Kozlowski <krzk@kernel.org>]

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