On 16/07/2024 11:24, Maxime Ripard wrote: > On Mon, Jul 15, 2024 at 07:38:34PM GMT, Dmitry Baryshkov wrote: >> On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <mripard@xxxxxxxxxx> wrote: >>> On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote: >>>> On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote: >>>>> On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote: >>>>>> On 01/07/2024 15:50, Maxime Ripard wrote: >>>>>>> On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote: >>>>>>> >>>>>>>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver. >>>>>>>> It supports DVI 1.0, HDMI 1.4b and 2.0b. >>>>>>>> It supports 4 TMDS channels, HPD, and a DDC interface. >>>>>>>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC) >>>>>>>> for power reduction. Several methods of power management are >>>>>>>> implemented to reduce overall power consumption. >>>>>>>> It supports fixed receiver EQ gain using I2C or pin strap to >>>>>>>> compensate for different lengths input cable or board traces. >>>>>>>> >>>>>>>> Features >>>>>>>> >>>>>>>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input >>>>>>>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps >>>>>>>> data rate, compatible with HDMI 2.0b electrical parameters >>>>>>>> - DisplayPort dual-mode standard version 1.1 >>>>>>>> - Programmable fixed receiver equalizer up to 15.5dB >>>>>>>> - Global or independent high speed lane control, pre-emphasis >>>>>>>> and transmit swing, and slew rate control >>>>>>>> - I2C or pin strap programmable >>>>>>>> - Configurable as a DisplayPort redriver through I2C >>>>>>>> - Full lane swap on main lanes >>>>>>>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown) >>>>>>>> >>>>>>>> https://www.ti.com/lit/ds/symlink/tdp158.pdf >>>>>>>> >>>>>>>> Signed-off-by: Marc Gonzalez <mgonzalez@xxxxxxxxxx> >>>>>>>> --- >>>>>>>> .../bindings/display/bridge/ti,tdp158.yaml | 51 ++++++++++++++++++++++ >>>>>>>> 1 file changed, 51 insertions(+) >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml >>>>>>>> new file mode 100644 >>>>>>>> index 0000000000000..21c8585c3bb2d >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml >>>>>>>> @@ -0,0 +1,51 @@ >>>>>>>> +# SPDX-License-Identifier: GPL-2.0-only >>>>>>>> +%YAML 1.2 >>>>>>>> +--- >>>>>>>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml# >>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>> + >>>>>>>> +title: TI TDP158 HDMI to TMDS Redriver >>>>>>>> + >>>>>>>> +maintainers: >>>>>>>> + - Arnaud Vrac <avrac@xxxxxxxxxx> >>>>>>>> + - Pierre-Hugues Husson <phhusson@xxxxxxxxxx> >>>>>>>> + >>>>>>>> +properties: >>>>>>>> + compatible: >>>>>>>> + const: ti,tdp158 >>>>>>>> + >>>>>>>> + reg: >>>>>>>> + description: I2C address of the device >>>>>>>> + >>>>>>>> + enable-gpios: >>>>>>>> + description: GPIO controlling bridge enable >>>>>>>> + >>>>>>>> + vcc-supply: >>>>>>>> + description: Power supply 3.3V >>>>>>>> + >>>>>>>> + vdd-supply: >>>>>>>> + description: Power supply 1.1V >>>>>>>> + >>>>>>>> + ports: >>>>>>>> + $ref: /schemas/graph.yaml#/properties/ports >>>>>>>> + >>>>>>>> + properties: >>>>>>>> + port@0: >>>>>>>> + $ref: /schemas/graph.yaml#/properties/port >>>>>>>> + description: Bridge input >>>>>>>> + >>>>>>>> + port@1: >>>>>>>> + $ref: /schemas/graph.yaml#/properties/port >>>>>>>> + description: Bridge output >>>>>>>> + >>>>>>>> + required: >>>>>>>> + - port@0 >>>>>>>> + - port@1 >>>>>>> >>>>>>> The device supports DVI, HDMI or DP input, with various requirements and >>>>>>> capabilities depending on the input. Your binding doesn't address that. >>>>>>> >>>>>>> Similarly, it can do lane-swapping, so we should probably have a >>>>>>> property to describe what mapping we want to use. >>>>>>> >>>>>>> The i2c register access (and the whole behaviour of the device) is >>>>>>> constrained on the I2C_EN pin status, and you can't read it from the >>>>>>> device, so it's also something we need to have in the DT. >>>>>> >>>>>> We are using the device in its default configuration. >>>>>> (Power on via OE, then it works as expected) >>>>> >>>>> I know, but that doesn't really matter for a binding. >>>>> >>>>>> Can we leave any additional properties to be defined by whomever needs >>>>>> them in the future? >>>>> >>>>> If you can guarantee that doing so would be backward compatible, sure. >>>>> But that means being able to answer those questions with a reasonable >>>>> plan. >>>> >>>> I think proposed bindings are generic enough to cover other possible >>>> usecases in future. >>> >>> I don't think it is. The current binding is for a I2C device that >>> shouldn't be accessed through I2C. >>> >>> It's working for now because the driver doesn't do any access, so it's >>> all great, but as soon as we add support for the other case, then we'll >>> have to add a property that states that while it's an i2c device, it >>> shouldn't be accessed. >>> >>> And adding such a property is a compatibility-breaking change. >> >> Please correct me if I'm wrong. We have following usecases. >> >> 1. I2C_EN pulled low. TI158 is in the pin strap mode, it is not >> connected to the I2C bus. A0, A1, SDA and SCL pins are used for >> strapping the settings. >> board DT file should describe the bridge as a platform device >> sitting directly under the root node. > > DT maintainers have required that reg is always present in the other > sub-thread. > >> 2. I2C_EN pulled high. TI158 is in the I2C mode. It is connected to >> the I2C bus, A0/A1 pins set the I2C bus address. The device is >> controlled over the I2C bus >> >> 2.a. The same as 2, but the device is not controlled at all, default >> settings are fine. >> >> The driver covers usecase 2.a. The bindings allow extending the driver >> to the usecase 2 (e.g. via optional properties which specify >> bord-specific settings) >> >> The usecase 1 is a completely separate topic, it requires a different >> schema file, specifying no i2c address, only voltages supplies and >> enable-gpios. > > I could have mis-unnderstood, but my understanding was that they were > running it with I2C_EN tied low. > > Of course, that's one of the thing that is completely missing from the > commit log, so who knows. On our board, the device is sitting on I2C bus blsp2_i2c1. Therefore, I2C_EN is hard-wired to HIGH. (As it must be for I2C to function.) &blsp2_i2c1 { status = "okay"; tdp158@5e { compatible = "ti,tdp158"; reg = <0x5e>; enable-gpios = <&tlmm 17 GPIO_ACTIVE_HIGH>; pinctrl-0 = <&hdmi_en>; pinctrl-names = "default"; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; tdp158_in: endpoint { remote-endpoint = <&hdmi_out>; }; }; port@1 { reg = <1>; tdp158_out: endpoint { remote-endpoint = <&hdmi_con>; }; }; }; }; };
