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
