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

Krzysztof Kozlowski
2024-08-26T08:12:50Z

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@ti.com>
>>> ---
>>> .../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@ideasonboard.com>
>>> + - Aradhya Bhatia <a-bhatia1@ti.com>
>>> +
>>> +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