Re: [PATCH v2 1/6] dt-bindings: display: Add yamls for JH7110 display system

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 29/11/2023 04:13, Keith Zhao wrote:
> 
> 
> On 2023/10/25 20:50, Krzysztof Kozlowski wrote:
>> On 25/10/2023 12:39, Keith Zhao wrote:
>>> StarFive SoCs JH7110 display system:
>>
>> A nit, subject: drop second/last, redundant "yamls for". The
>> "dt-bindings" prefix is already stating that these are bindings, so
>> format is fixed.
>>
>>> lcd-controller bases verisilicon dc8200 IP,
>>> and hdmi bases Innosilicon IP. Add bindings for them.
>>
>> Please make it a proper sentences, with proper wrapping.
>>
>>>
>>> also update MAINTAINERS for dt-bindings
>>
>> Not a sentence, but also not really needed.ok I see.
>>
>>>
>>> about this patch, I tested the dtbs_check and dt_binding_check
>>> with the result pass.
>>> Based on the feedback of the previous version, the corresponding arrangement is made
>>
>> Not relevant, so not really suitable for commit msg.
>>
>>>
>>> Signed-off-by: Keith Zhao <keith.zhao@starfivetech.com>
>>> ---
>>>  .../starfive/starfive,display-subsystem.yaml  |  41 +++++++
>>>  .../starfive/starfive,jh7110-dc8200.yaml      | 109 ++++++++++++++++++
>>>  .../starfive/starfive,jh7110-inno-hdmi.yaml   |  85 ++++++++++++++
>>>  MAINTAINERS                                   |   7 ++
>>>  4 files changed, 242 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml b/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml
>>> new file mode 100644
>>> index 000000000..f45b97b08
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml
>>> @@ -0,0 +1,41 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/starfive/starfive,display-subsystem.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Starfive DRM master device
>>
>> What is DRM in hardware? I know Digital Rights Management, but then
>> subsystem seems wrong. If you mean Linux DRM, then Linux is not a
>> hardware, so drop all Linuxisms and describe hardware.
> ok , will only keep hardware describe in my next version
>>
>>
>>> +
>>> +maintainers:
>>> +  - Keith Zhao <keith.zhao@starfivetech.com>
>>> +  - ShengYang Chen <shengyang.chen@starfivetech.com>
>>> +
>>> +description:
>>> +  The Starfive DRM master device is a virtual device needed to list all
>>
>> Virtual device? Then not suitable for bindings, sorry.
>>
>>> +  display controller or other display interface nodes that comprise the
>>> +  graphics subsystem.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: starfive,display-subsystem
>>> +
>>> +  ports:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>
>> No, ports is not phandle-array. ports is object, always.
>>
>>> +    description:
>>> +      Should contain a list of phandles pointing to display interface ports
>>> +      of display controller devices. Display controller definitions as defined
>>> +      in Documentation/devicetree/bindings/display/starfive/
>>> +      starfive,jh7110-dc8200.yaml
>>
>> Use standard graph ports, not some own, custom property.
>>
>> Anyway, entire binding should be dropped. You do not need it even.
> Hi Krzysztof:
> Virtual device is not suitable for bindings, matbe I need associate it with the real hardware.
> such as the top clocks & reset , irq , etc.
> Currently I configure them in another yaml file. Logically speaking, this is more suitable.
> 
> Can adding the corresponding hardware description change its fate of being deleted?

I am not sure if I follow. Bindings and DTS describe the hardware, so if
you configure device A clocks in a device B node, then it is not
correct. If you add binding for something not being a real device, it is
not correct.

Feel free to bring proper hardware description, not Linux. This entire
binding was written to describe Linux driver, which is not correct.

Best regards,
Krzysztof