Re: [PATCH v3 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter

[Krzysztof Kozlowski]

On 26/08/2024 07:47, Aradhya Bhatia wrote:
> Hi Krzysztof,
> 
> Thank you for the reviewing the patches.
> 
> 
> On 7/21/24 21:06, Krzysztof Kozlowski wrote:
>> On 16/07/2024 10:42, Aradhya Bhatia wrote:
>>> The OLDI (transmitters) TXes do not have registers of their own, and are
>>> dependent on the source video-ports from the DSS to provide
>>> configuration data. This hardware doesn't directly sit on the internal
>>> bus of the SoC, but does so via the DSS. Hence, the OLDI TXes are
>>> supposed to be child nodes under the DSS, and not independent devices.
>>>
>>> Two of the OLDI TXes can function in tandem to output dual-link OLDI
>>> output, or cloned single-link outputs. In these cases, one OLDI will be
>>> the primary OLDI, and the other one, a companion.
>>>
>>> The OLDI functionality is further supported by a system-control module,
>>> which contains a few registers to control OLDI IO power and
>>> characteristics.
>>>
>>> Add devicetree binding schema for AM625 OLDI TXes.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
>>> ---
>>>  .../bindings/display/ti/ti,am625-oldi.yaml    | 153 ++++++++++++++++++
>>>  MAINTAINERS                                   |   1 +
>>>  2 files changed, 154 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>>> new file mode 100644
>>> index 000000000000..0a96e600bc0b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
>>> @@ -0,0 +1,153 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/ti/ti,am625-oldi.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Texas Instruments AM625 OLDI Transmitter
>>> +
>>> +maintainers:
>>> +  - Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>>> +  - Aradhya Bhatia <a-bhatia1@xxxxxx>
>>> +
>>> +description: |
>>
>> Do not need '|' unless you need to preserve formatting.
> 
> Okay!
> 
>>
>>> +  The AM625 TI Keystone OpenLDI transmitter (OLDI TX) supports serialized RGB
>>> +  pixel data transmission between host and flat panel display over LVDS (Low
>>> +  Voltage Differential Sampling) interface. The OLDI TX consists of 7-to-1 data
>>> +  serializers, and 4-data and 1-clock LVDS outputs. It supports the LVDS output
>>> +  formats "jeida-18", "jeida-24" and "vesa-18", and can accept 24-bit RGB or
>>> +  padded and un-padded 18-bit RGB bus formats as input.
>>> +
>>> +properties:
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>
>> How does it even work without compatible? How is this schema selected?
>> If by part of your next patch, then this is not a proper split - this
>> patch itself is noop. Squash the patches.
>>
> 
> Yes, it is supposed to be picked like the next patch does it. I can
> squash these both.
> 
>>> +  clocks:
>>> +    maxItems: 1
>>> +    description: serial clock input for the OLDI transmitters
>>> +
>>> +  clock-names:
>>> +    const: s_clk
>>
>> Drop _clk or name it correctly.
> 
> Alright!
> 
>>
>>> +
>>> +  ti,companion-oldi:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      phandle to companion OLDI transmitter. This property is mandatory for the
>>> +      primarty OLDI TX if the OLDI TXes are expected to work either in dual-lvds
>>> +      mode or in clone mode. This property should point to the secondary OLDI
>>> +      TX.
>>> +
>>> +  ti,secondary-oldi:
>>> +    type: boolean
>>> +    description: Boolean property to mark an OLDI TX as secondary node.
>>
>> Why? Lack companion tells it, doesn't it?
> 
> A lack of companion doesn't mean secondary-OLDI automatically, actually.
> 
> There is also a possible configuration where 2 OLDI TXes could be
> individually connected to 2 different sources => 2x single Link
> configuration. The OLDI TXes would then work independently.

You are responding for something month old. I am not in the context anymore.

Probably you miss proper graphs here, not such property.

> 
>>
>>> +
>>> +  ti,oldi-io-ctrl:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      phandle to syscon device node mapping OLDI IO_CTRL registers found in the
>>> +      control MMR region. This property is needed for OLDI interface to work.
>>
>> "This property is needed for OLDI interface to work." tells nothing.
>> Everything is needed for everything to work. Be specific.
>>
> 
> Yes! Will fix this.
> 
>>> +
>>> +  ports:
>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>> +
>>> +    properties:
>>> +      port@0:
>>> +        $ref: /schemas/graph.yaml#/properties/port
>>> +        description: Parallel RGB input port
>>> +
>>> +      port@1:
>>> +        $ref: /schemas/graph.yaml#/properties/port
>>> +        description: LVDS output port
>>> +
>>> +    required:
>>> +      - port@0
>>> +      - port@1
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        ti,secondary-oldi: true
>>
>> This does not work... Test your schema.
>>
> 
> I tested again just now. At least the schema check didn't report any
> error. I used the v2024.05 dtschema too.

No, test your condition. Come with DTS with exercises this if. You will
see this DOES NOT WORK. This is just no-op, does not perform any useful
work. So test the code that it actually performs what you want it to do.

> 
> This github gist[0] captures all details of this test.
> 
> Could you instead please elaborate what maybe wrong here, and I will try
> to fix that.

Look at example-schema or any of my talks with useful references.

> 
> 
>>> +    then:
>>> +      properties:
>>> +        ti,companion-oldi: false
>>> +        ti,oldi-io-ctrl: false
>>> +        clocks: false
>>> +        clock-names: false
>>> +
>>> +    else:
>>> +      required:
>>> +        - ti,oldi-io-ctrl
>>> +        - clocks
>>> +        - clock-names
>>> +
>>> +required:
>>> +  - reg
>>> +  - ports
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
>>> +
>>> +    oldi_txes {
>>
>> No underscores in node names.
>>
>> Node names should be generic. See also an explanation and list of
>> examples (not exhaustive) in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Right. Will make the name generic.
> 
>>
>>
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        oldi: oldi@0 {
>> What is the "reg" for?
> 
> The reg is for indexing purposes so that the driver can distinguish
> between which OLDI TX is under question. Since, the syscon controller
> has different power control registers and bits for different OLDIs - its
> important for the driver to be able to tell one from another.

Again, not sure, not in context. Patch is not even in the inbox anymore.

Best regards,
Krzysztof