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@xxxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> .../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
