| Message ID | 20240627-tdp158-v3-1-fb2fbc808346@freebox.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Basic support for TI TDP158 | expand |
On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> > --- > .../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@freebox.fr> > + - Pierre-Hugues Husson <phhusson@freebox.fr> > + > +properties: > + compatible: > + const: ti,tdp158 > + > + reg: > + description: I2C address of the device Is reg not required? How do you communicate with the device if the i2c bus is not connected? Is the enable GPIO enough to operate it in some situations? Otherwise this looks good to me, but given Maxime commented about the complexity of the device, I'm probably out of my depth! > +required: > + - compatible > + - vcc-supply > + - vdd-supply > + - ports
On 27/06/2024 18:25, Conor Dooley wrote: > On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> >> --- >> .../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@freebox.fr> >> + - Pierre-Hugues Husson <phhusson@freebox.fr> >> + >> +properties: >> + compatible: >> + const: ti,tdp158 >> + >> + reg: >> + description: I2C address of the device > > Is reg not required? How do you communicate with the device if the i2c > bus is not connected? Is the enable GPIO enough to operate it in some > situations? > > Otherwise this looks good to me, but given Maxime commented about the > complexity of the device, I'm probably out of my depth! Valid question. As discussed in my brilliantly expanded commit message (:p) the device can be configured in various ways, either through I2C registers or by pin strap. We use the device in its default settings, so we don't touch any I2C registers, thus I'm not sure the reg property is required. >> +required: >> + - compatible >> + - vcc-supply >> + - vdd-supply >> + - ports Regards
On 27/06/2024 18:45, Marc Gonzalez wrote: > On 27/06/2024 18:25, Conor Dooley wrote: > >> On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> >>> --- >>> .../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@freebox.fr> >>> + - Pierre-Hugues Husson <phhusson@freebox.fr> >>> + >>> +properties: >>> + compatible: >>> + const: ti,tdp158 >>> + >>> + reg: >>> + description: I2C address of the device >> >> Is reg not required? How do you communicate with the device if the i2c >> bus is not connected? Is the enable GPIO enough to operate it in some >> situations? >> >> Otherwise this looks good to me, but given Maxime commented about the >> complexity of the device, I'm probably out of my depth! > > Valid question. > > As discussed in my brilliantly expanded commit message (:p) > the device can be configured in various ways, either through I2C registers > or by pin strap. We use the device in its default settings, so we don't > touch any I2C registers, thus I'm not sure the reg property is required. But then how would it be represented in the DT? Where / under which parent? If this is supposed to be always in I2C bus in DT, then you always need reg. If you could place it in other place, then your reasoning is valid - reg is optional. Best regards, Krzysztof
On Fri, Jun 28, 2024 at 09:36:57AM GMT, Krzysztof Kozlowski wrote: > On 27/06/2024 18:45, Marc Gonzalez wrote: > > On 27/06/2024 18:25, Conor Dooley wrote: > > > >> On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> > >>> --- > >>> .../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@freebox.fr> > >>> + - Pierre-Hugues Husson <phhusson@freebox.fr> > >>> + > >>> +properties: > >>> + compatible: > >>> + const: ti,tdp158 > >>> + > >>> + reg: > >>> + description: I2C address of the device > >> > >> Is reg not required? How do you communicate with the device if the i2c > >> bus is not connected? Is the enable GPIO enough to operate it in some > >> situations? > >> > >> Otherwise this looks good to me, but given Maxime commented about the > >> complexity of the device, I'm probably out of my depth! > > > > Valid question. > > > > As discussed in my brilliantly expanded commit message (:p) > > the device can be configured in various ways, either through I2C registers > > or by pin strap. We use the device in its default settings, so we don't > > touch any I2C registers, thus I'm not sure the reg property is required. > > But then how would it be represented in the DT? Where / under which parent? > > If this is supposed to be always in I2C bus in DT, then you always need > reg. If you could place it in other place, then your reasoning is valid > - reg is optional. As far as I understood, the device is connected to I2C bus, it just doesn't need to be programmed. So I'd conclude that reg is required.
Hi, 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@freebox.fr> > --- > .../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@freebox.fr> > + - Pierre-Hugues Husson <phhusson@freebox.fr> > + > +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. Maxime
On 28/06/2024 09:49, Dmitry Baryshkov wrote: > On Fri, Jun 28, 2024 at 09:36:57AM GMT, Krzysztof Kozlowski wrote: >> On 27/06/2024 18:45, Marc Gonzalez wrote: >>> On 27/06/2024 18:25, Conor Dooley wrote: >>>> On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> >>>>> --- >>>>> .../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@freebox.fr> >>>>> + - Pierre-Hugues Husson <phhusson@freebox.fr> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: ti,tdp158 >>>>> + >>>>> + reg: >>>>> + description: I2C address of the device >>>> >>>> Is reg not required? How do you communicate with the device if the i2c >>>> bus is not connected? Is the enable GPIO enough to operate it in some >>>> situations? >>>> >>>> Otherwise this looks good to me, but given Maxime commented about the >>>> complexity of the device, I'm probably out of my depth! >>> >>> Valid question. >>> >>> As discussed in my brilliantly expanded commit message (:p) >>> the device can be configured in various ways, either through I2C registers >>> or by pin strap. We use the device in its default settings, so we don't >>> touch any I2C registers, thus I'm not sure the reg property is required. >> >> But then how would it be represented in the DT? Where / under which parent? >> >> If this is supposed to be always in I2C bus in DT, then you always need >> reg. If you could place it in other place, then your reasoning is valid >> - reg is optional. > > As far as I understood, the device is connected to I2C bus, it just > doesn't need to be programmed. So I'd conclude that reg is required. Just to be clear (and as far as I understand), the TDP158 can be configured via 2 different methods: - dynamically at run-time, through I2C registers (requires an I2C bus) - statically at layout-time through pin straps (no I2C bus required) On our board, the TDP158 is connected to blsp2_i2c1. So, in my understanding, the "reg" property would be required for the first method, but is not applicable for the second method. I don't feel strongly about the issue, so I can mark the "reg" property as required if it makes more sense. Regards
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@freebox.fr> >> --- >> .../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@freebox.fr> >> + - Pierre-Hugues Husson <phhusson@freebox.fr> >> + >> +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) Can we leave any additional properties to be defined by whomever needs them in the future? Regards
On 01/07/2024 15:50, Maxime Ripard wrote: > 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. I think the purpose of the I2C_EN pin might have been misunderstood. I2C_EN is not meant to be toggled, ever, by anyone from this planet. I2C_EN is a layout-time setting, decided by a board manufacturer: - If the TDP158 is fully configured once-and-for-all at layout-time, then no I2C bus is required, and I2C_EN is pulled down forever. - If the board manufacturer wants to keep open the possibility to adjust some parameters at run-time, then they must connect the device to an I2C bus, and I2C_EN is pulled up forever. The only reason I see to expose I2C_EN in a binding is: if we want to support the fully static setup, AND it is not acceptable to support it from an i2c_driver, then there might need to be a way to say "you are not an i2c client, you must fail in probe". Or I don't understand anything about device tree bindings (which is entirely possible). Regards
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@freebox.fr> > >> --- > >> .../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@freebox.fr> > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > >> + > >> +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. Maxime
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@freebox.fr> > > >> --- > > >> .../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@freebox.fr> > > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > > >> + > > >> +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.
On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: > On 01/07/2024 15:50, Maxime Ripard wrote: > > > 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. > > I think the purpose of the I2C_EN pin might have been misunderstood. > > I2C_EN is not meant to be toggled, ever, by anyone from this planet. Toggled, probably not. Connected to a GPIO and the kernel has to assert a level at boot, I've seen worse hardware design already. > I2C_EN is a layout-time setting, decided by a board manufacturer: > > - If the TDP158 is fully configured once-and-for-all at layout-time, > then no I2C bus is required, and I2C_EN is pulled down forever. > > - If the board manufacturer wants to keep open the possibility > to adjust some parameters at run-time, then they must connect > the device to an I2C bus, and I2C_EN is pulled up forever. How do you express both cases in your current binding? Maxime
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@freebox.fr> > > > >> --- > > > >> .../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@freebox.fr> > > > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > > > >> + > > > >> +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. Maxime
On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <mripard@kernel.org> 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@freebox.fr> > > > > >> --- > > > > >> .../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@freebox.fr> > > > > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > > > > >> + > > > > >> +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. 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. -- With best wishes Dmitry
Hi, On Mon, Jul 15, 2024 at 07:38:34PM GMT, Dmitry Baryshkov wrote: > On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <mripard@kernel.org> 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@freebox.fr> > > > > > >> --- > > > > > >> .../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@freebox.fr> > > > > > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > > > > > >> + > > > > > >> +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. Maxime
On Tue, 16 Jul 2024 at 12:24, Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > On Mon, Jul 15, 2024 at 07:38:34PM GMT, Dmitry Baryshkov wrote: > > On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <mripard@kernel.org> 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@freebox.fr> > > > > > > >> --- > > > > > > >> .../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@freebox.fr> > > > > > > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > > > > > > >> + > > > > > > >> +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. If I2C_EN is pulled low, there is no reg, as there is no i2c bus whatsoever. I2C pins are repurposed as pin straps, An pins are repurposed as pin straps. I think DT maintainers wanted reg for the 2.a. case - in other words the bridge is present on the I2C bus, but it is not being programmed. > > > 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. That was my initial assumption, but I think Arnoud pointed out that the bridge is connected to I2C, just not controlled as defaults are sane. > Of course, that's one of the thing that is completely missing from the > commit log, so who knows. Or from the cover letter :-(
On 27/06/2024 18:25, Conor Dooley wrote: > On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> >> --- >> .../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@freebox.fr> >> + - Pierre-Hugues Husson <phhusson@freebox.fr> >> + >> +properties: >> + compatible: >> + const: ti,tdp158 >> + >> + reg: >> + description: I2C address of the device > > Is reg not required? How do you communicate with the device if the i2c > bus is not connected? Is the enable GPIO enough to operate it in some > situations? > > Otherwise this looks good to me, but given Maxime commented about the > complexity of the device, I'm probably out of my depth! Hello Conor, A cycle has been detected: Above, you defer to Maxime. Yet later, he wrote: "DT maintainers have required that reg is always present" I propose we NOT mark the "reg" property as required. (Thus, keep the binding as proposed in v3.) Rationale: - The device can be statically configured by pin straps, in which case it is NOT connected to an I2C bus. - Even if the device IS connected to an I2C bus, no I2C accesses are required if the default configuration meets the ODM's needs. Is that OK with you? Can I get your Amen? Regards
On Tue, Jul 23, 2024 at 05:17:12PM +0200, Marc Gonzalez wrote: > On 27/06/2024 18:25, Conor Dooley wrote: > > > On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> > >> --- > >> .../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@freebox.fr> > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > >> + > >> +properties: > >> + compatible: > >> + const: ti,tdp158 > >> + > >> + reg: > >> + description: I2C address of the device > > > > Is reg not required? How do you communicate with the device if the i2c > > bus is not connected? Is the enable GPIO enough to operate it in some > > situations? > > > > Otherwise this looks good to me, but given Maxime commented about the > > complexity of the device, I'm probably out of my depth! > > Hello Conor, > > A cycle has been detected: > Above, you defer to Maxime. > Yet later, he wrote: > "DT maintainers have required that reg is always present" I think he was actually talking about Krzysztof. > I propose we NOT mark the "reg" property as required. > (Thus, keep the binding as proposed in v3.) > > Rationale: > > - The device can be statically configured by pin straps, > in which case it is NOT connected to an I2C bus. I'm inclined to say that, because connecting the i2c bus is not required at all for the device to be usable in some circumstances that we should not require reg. Someone could, in theory, use the device with a SoC without an i2c controller, right? > - Even if the device IS connected to an I2C bus, > no I2C accesses are required if the default configuration > meets the ODM's needs. In this case, I think a reg property is required actually, because it is on the bus, and devices on an i2c bus must have a reg property. That aside, even if the ODM doesn't want to change the defaults, the owner might. > Is that OK with you? Can I get your Amen? Sure.
On Tue, Jul 23, 2024 at 08:57:03PM GMT, Conor Dooley wrote: > On Tue, Jul 23, 2024 at 05:17:12PM +0200, Marc Gonzalez wrote: > > On 27/06/2024 18:25, Conor Dooley wrote: > > > > > On Thu, Jun 27, 2024 at 01:13:03PM +0200, 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@freebox.fr> > > >> --- > > >> .../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@freebox.fr> > > >> + - Pierre-Hugues Husson <phhusson@freebox.fr> > > >> + > > >> +properties: > > >> + compatible: > > >> + const: ti,tdp158 > > >> + > > >> + reg: > > >> + description: I2C address of the device > > > > > > Is reg not required? How do you communicate with the device if the i2c > > > bus is not connected? Is the enable GPIO enough to operate it in some > > > situations? > > > > > > Otherwise this looks good to me, but given Maxime commented about the > > > complexity of the device, I'm probably out of my depth! > > > > Hello Conor, > > > > A cycle has been detected: > > Above, you defer to Maxime. > > Yet later, he wrote: > > "DT maintainers have required that reg is always present" > > I think he was actually talking about Krzysztof. I was. > > I propose we NOT mark the "reg" property as required. > > (Thus, keep the binding as proposed in v3.) > > > > Rationale: > > > > - The device can be statically configured by pin straps, > > in which case it is NOT connected to an I2C bus. > > I'm inclined to say that, because connecting the i2c bus is not required > at all for the device to be usable in some circumstances that we should > not require reg. Someone could, in theory, use the device with a SoC > without an i2c controller, right? > > > - Even if the device IS connected to an I2C bus, > > no I2C accesses are required if the default configuration > > meets the ODM's needs. > > In this case, I think a reg property is required actually, because it is > on the bus, and devices on an i2c bus must have a reg property. That > aside, even if the ODM doesn't want to change the defaults, the owner > might. > > > Is that OK with you? Can I get your Amen? > > Sure. We still have an entire sub-thread to this conversation that has been completely ignored by Marc. Upstreaming is process where both sides need to be involved, and I'm not seeing that so far. So, yeah, until that other sub-thread is somewhat resolved, I don't see how we can vet those bindings. Maxime
On 15/07/2024 16:42, Maxime Ripard 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@freebox.fr> >>>>>> --- >>>>>> .../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@freebox.fr> >>>>>> + - Pierre-Hugues Husson <phhusson@freebox.fr> >>>>>> + >>>>>> +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. Why do you say: "current binding is for a I2C device that shouldn't be accessed through I2C" ? As a matter of fact, my debug code queries the device ID using regmap_read() to make sure I set the correct I2C address. It's not that the device "SHOULD NOT" be accessed. It's just that the driver DOES NOT NEED TO access the device, simply because the default settings work fine. Regards
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@kernel.org> 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@freebox.fr> >>>>>>>> --- >>>>>>>> .../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@freebox.fr> >>>>>>>> + - Pierre-Hugues Husson <phhusson@freebox.fr> >>>>>>>> + >>>>>>>> +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>; }; }; }; }; };
On Wed, Jul 24, 2024 at 07:18:39PM GMT, Marc Gonzalez wrote: > On 15/07/2024 16:42, Maxime Ripard 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@freebox.fr> > >>>>>> --- > >>>>>> .../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@freebox.fr> > >>>>>> + - Pierre-Hugues Husson <phhusson@freebox.fr> > >>>>>> + > >>>>>> +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. > > Why do you say: > "current binding is for a I2C device that > shouldn't be accessed through I2C" ? > > As a matter of fact, my debug code queries the device ID using > regmap_read() to make sure I set the correct I2C address. Please note: bingdings do not cover the driver at all. The driver might do whatever it wants. The bindings describe the hardware. > > It's not that the device "SHOULD NOT" be accessed. > > It's just that the driver DOES NOT NEED TO access the device, > simply because the default settings work fine.
On 24/07/2024 19:25, Dmitry Baryshkov wrote: > On Wed, Jul 24, 2024 at 07:18:39PM GMT, Marc Gonzalez wrote: >> On 15/07/2024 16:42, Maxime Ripard 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@freebox.fr> >>>>>>>> --- >>>>>>>> .../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@freebox.fr> >>>>>>>> + - Pierre-Hugues Husson <phhusson@freebox.fr> >>>>>>>> + >>>>>>>> +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. >> >> Why do you say: >> "current binding is for a I2C device that >> shouldn't be accessed through I2C" ? >> >> As a matter of fact, my debug code queries the device ID using >> regmap_read() to make sure I set the correct I2C address. > > Please note: bingdings do not cover the driver at all. The driver might > do whatever it wants. The bindings describe the hardware. OK. How does the proposed binding say "I2C device shouldn't be accessed through I2C" ? The tfp410 has the same property that it can be configured either through pin straps or via I2C.
On 15/07/2024 18:38, Dmitry Baryshkov wrote: > 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. > > 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 > board-specific settings) OK, I think I understand (maybe). You're saying: the current binding doesn't specify any particular setting because the default settings are OK. We can switch to use-case 2. simply by adding a prop that will change one specific setting (backward compatible) > 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 have tested this. We can support use-case 1. by registering a module_platform_driver with the same compatible property. The probe function gets called only for a node that is a child of root. No different binding required. Regards
On 15/07/2024 16:40, Maxime Ripard wrote: > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: >> On 01/07/2024 15:50, Maxime Ripard wrote: >> >>> 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. >> >> I think the purpose of the I2C_EN pin might have been misunderstood. >> >> I2C_EN is not meant to be toggled, ever, by anyone from this planet. > > Toggled, probably not. Connected to a GPIO and the kernel has to assert > a level at boot, I've seen worse hardware design already. > >> I2C_EN is a layout-time setting, decided by a board manufacturer: >> >> - If the TDP158 is fully configured once-and-for-all at layout-time, >> then no I2C bus is required, and I2C_EN is pulled down forever. >> >> - If the board manufacturer wants to keep open the possibility >> to adjust some parameters at run-time, then they must connect >> the device to an I2C bus, and I2C_EN is pulled up forever. > > How do you express both cases in your current binding? It's not that I'm ignoring your question. It's that I don't understand what you're asking. SITUATION 1 tdp158 is pin strapped. Device node is child of root node. Properties in proposed binding are valid (regulators and power-on pin) Can be supported via module_platform_driver. SITUATION 2 tdp158 is sitting on I2C bus. Device node is child of i2c bus node. (robh said missing reg prop would be flagged by the compiler) Properties in proposed binding are valid (regulators and power-on pin) Supported via module_i2c_driver. If some settings-specific properties are added later, like skew, they would only be valid for the I2C programmable mode, obviously. Regards
On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote: > On 15/07/2024 16:40, Maxime Ripard wrote: > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: > >> On 01/07/2024 15:50, Maxime Ripard wrote: > >> > >>> 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. > >> > >> I think the purpose of the I2C_EN pin might have been misunderstood. > >> > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet. > > > > Toggled, probably not. Connected to a GPIO and the kernel has to assert > > a level at boot, I've seen worse hardware design already. > > > >> I2C_EN is a layout-time setting, decided by a board manufacturer: > >> > >> - If the TDP158 is fully configured once-and-for-all at layout-time, > >> then no I2C bus is required, and I2C_EN is pulled down forever. > >> > >> - If the board manufacturer wants to keep open the possibility > >> to adjust some parameters at run-time, then they must connect > >> the device to an I2C bus, and I2C_EN is pulled up forever. > > > > How do you express both cases in your current binding? > > It's not that I'm ignoring your question. > > It's that I don't understand what you're asking. And that's fine, you just need to say so. Generally speaking, you're focusing on the driver. The driver is not the issue here. You can do whatever you want in the driver for all I care, we can change that later on as we wish. The binding however cannot change, so it *has* to ideally cover all possible situations the hardware can be used in, or at a minimum leave the door open to support those without a compatibility breakage. That's why I've been asking those questions, because so far the only thing you've claimed is that "I can't test the driver for anything else", but, again, whether there's a driver or not, or if it's functional, is completely missing the point. > SITUATION 1 > tdp158 is pin strapped. > Device node is child of root node. > Properties in proposed binding are valid (regulators and power-on pin) > Can be supported via module_platform_driver. > > SITUATION 2 > tdp158 is sitting on I2C bus. > Device node is child of i2c bus node. > (robh said missing reg prop would be flagged by the compiler) > Properties in proposed binding are valid (regulators and power-on pin) > Supported via module_i2c_driver. > > If some settings-specific properties are added later, like skew, > they would only be valid for the I2C programmable mode, obviously. I think there's a couple more combinations: - The device is connected on an I2C bus, but I2C_EN is tied low - The device is connected on an I2C bus, but I2C_EN is connected to a GPIO and the kernel needs to assert its state at boot. The GPIO case can be easily dealt with later on using an optional GPIO in the binding, but the current binding infers the I2C_EN level from the bus it's connected to, and I think we don't have a good way to deal with cases that would break that assumption. So I think we need an extra property to report the state of the i2c_en pin (and would be mutually exclusive with the GPIO if we ever have to support it). Maxime
On Wed, Jul 24, 2024 at 07:28:41PM GMT, Marc Gonzalez wrote: > On 24/07/2024 19:25, Dmitry Baryshkov wrote: > > On Wed, Jul 24, 2024 at 07:18:39PM GMT, Marc Gonzalez wrote: > >> On 15/07/2024 16:42, Maxime Ripard 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@freebox.fr> > >>>>>>>> --- > >>>>>>>> .../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@freebox.fr> > >>>>>>>> + - Pierre-Hugues Husson <phhusson@freebox.fr> > >>>>>>>> + > >>>>>>>> +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. > >> > >> Why do you say: > >> "current binding is for a I2C device that > >> shouldn't be accessed through I2C" ? > >> > >> As a matter of fact, my debug code queries the device ID using > >> regmap_read() to make sure I set the correct I2C address. > > > > Please note: bingdings do not cover the driver at all. The driver might > > do whatever it wants. The bindings describe the hardware. > > OK. > How does the proposed binding say > "I2C device shouldn't be accessed through I2C" ? It doesn't, but then again, neither your commit log or cover letter say "it can be accessed by i2c" either, so we went on the wrong track. Maxime
On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote: > > On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote: > > On 15/07/2024 16:40, Maxime Ripard wrote: > > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: > > >> On 01/07/2024 15:50, Maxime Ripard wrote: > > >> > > >>> 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. > > >> > > >> I think the purpose of the I2C_EN pin might have been misunderstood. > > >> > > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet. > > > > > > Toggled, probably not. Connected to a GPIO and the kernel has to assert > > > a level at boot, I've seen worse hardware design already. > > > > > >> I2C_EN is a layout-time setting, decided by a board manufacturer: > > >> > > >> - If the TDP158 is fully configured once-and-for-all at layout-time, > > >> then no I2C bus is required, and I2C_EN is pulled down forever. > > >> > > >> - If the board manufacturer wants to keep open the possibility > > >> to adjust some parameters at run-time, then they must connect > > >> the device to an I2C bus, and I2C_EN is pulled up forever. > > > > > > How do you express both cases in your current binding? > > > > It's not that I'm ignoring your question. > > > > It's that I don't understand what you're asking. > > And that's fine, you just need to say so. > > Generally speaking, you're focusing on the driver. The driver is not the > issue here. You can do whatever you want in the driver for all I care, > we can change that later on as we wish. > > The binding however cannot change, so it *has* to ideally cover all > possible situations the hardware can be used in, or at a minimum leave > the door open to support those without a compatibility breakage. > > That's why I've been asking those questions, because so far the only > thing you've claimed is that "I can't test the driver for anything > else", but, again, whether there's a driver or not, or if it's > functional, is completely missing the point. > > > SITUATION 1 > > tdp158 is pin strapped. > > Device node is child of root node. > > Properties in proposed binding are valid (regulators and power-on pin) > > Can be supported via module_platform_driver. > > > > SITUATION 2 > > tdp158 is sitting on I2C bus. > > Device node is child of i2c bus node. > > (robh said missing reg prop would be flagged by the compiler) > > Properties in proposed binding are valid (regulators and power-on pin) > > Supported via module_i2c_driver. > > > > If some settings-specific properties are added later, like skew, > > they would only be valid for the I2C programmable mode, obviously. > > I think there's a couple more combinations: > > - The device is connected on an I2C bus, but I2C_EN is tied low No, this is not possible. I2C pins are repurposed if I2C_EN is low. You can not call that an i2c bus anymore. > - The device is connected on an I2C bus, but I2C_EN is connected to a > GPIO and the kernel needs to assert its state at boot. This is a pretty strange configuration. The I2C_EN pin isn't supposed to be toggled dynamically. Anyway, if that happens, I'd use pinctrl / hog to control the pin. > > The GPIO case can be easily dealt with later on using an optional GPIO > in the binding, but the current binding infers the I2C_EN level from the > bus it's connected to, and I think we don't have a good way to deal with > cases that would break that assumption. > > So I think we need an extra property to report the state of the i2c_en > pin (and would be mutually exclusive with the GPIO if we ever have to > support it).
On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote: > On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote: > > > > On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote: > > > On 15/07/2024 16:40, Maxime Ripard wrote: > > > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: > > > >> On 01/07/2024 15:50, Maxime Ripard wrote: > > > >> > > > >>> 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. > > > >> > > > >> I think the purpose of the I2C_EN pin might have been misunderstood. > > > >> > > > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet. > > > > > > > > Toggled, probably not. Connected to a GPIO and the kernel has to assert > > > > a level at boot, I've seen worse hardware design already. > > > > > > > >> I2C_EN is a layout-time setting, decided by a board manufacturer: > > > >> > > > >> - If the TDP158 is fully configured once-and-for-all at layout-time, > > > >> then no I2C bus is required, and I2C_EN is pulled down forever. > > > >> > > > >> - If the board manufacturer wants to keep open the possibility > > > >> to adjust some parameters at run-time, then they must connect > > > >> the device to an I2C bus, and I2C_EN is pulled up forever. > > > > > > > > How do you express both cases in your current binding? > > > > > > It's not that I'm ignoring your question. > > > > > > It's that I don't understand what you're asking. > > > > And that's fine, you just need to say so. > > > > Generally speaking, you're focusing on the driver. The driver is not the > > issue here. You can do whatever you want in the driver for all I care, > > we can change that later on as we wish. > > > > The binding however cannot change, so it *has* to ideally cover all > > possible situations the hardware can be used in, or at a minimum leave > > the door open to support those without a compatibility breakage. > > > > That's why I've been asking those questions, because so far the only > > thing you've claimed is that "I can't test the driver for anything > > else", but, again, whether there's a driver or not, or if it's > > functional, is completely missing the point. > > > > > SITUATION 1 > > > tdp158 is pin strapped. > > > Device node is child of root node. > > > Properties in proposed binding are valid (regulators and power-on pin) > > > Can be supported via module_platform_driver. > > > > > > SITUATION 2 > > > tdp158 is sitting on I2C bus. > > > Device node is child of i2c bus node. > > > (robh said missing reg prop would be flagged by the compiler) > > > Properties in proposed binding are valid (regulators and power-on pin) > > > Supported via module_i2c_driver. > > > > > > If some settings-specific properties are added later, like skew, > > > they would only be valid for the I2C programmable mode, obviously. > > > > I think there's a couple more combinations: > > > > - The device is connected on an I2C bus, but I2C_EN is tied low > > No, this is not possible. I2C pins are repurposed if I2C_EN is low. > You can not call that an i2c bus anymore. > > > - The device is connected on an I2C bus, but I2C_EN is connected to a > > GPIO and the kernel needs to assert its state at boot. > > This is a pretty strange configuration. The I2C_EN pin isn't supposed > to be toggled dynamically. Anyway, if that happens, I'd use pinctrl / > hog to control the pin. ACK. I still believe it would be valuable, but I don't really want to be part of that conversation anymore. Marc, do whatever you want. Maxime
On 30/07/2024 11:30, Maxime Ripard wrote: > On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote: >> On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote: >>> On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote: >>>> On 15/07/2024 16:40, Maxime Ripard wrote: >>>>> On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: >>>>>> On 01/07/2024 15:50, Maxime Ripard wrote: >>>>>> >>>>>>> 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. >>>>>> >>>>>> I think the purpose of the I2C_EN pin might have been misunderstood. >>>>>> >>>>>> I2C_EN is not meant to be toggled, ever, by anyone from this planet. >>>>> >>>>> Toggled, probably not. Connected to a GPIO and the kernel has to assert >>>>> a level at boot, I've seen worse hardware design already. >>>>> >>>>>> I2C_EN is a layout-time setting, decided by a board manufacturer: >>>>>> >>>>>> - If the TDP158 is fully configured once-and-for-all at layout-time, >>>>>> then no I2C bus is required, and I2C_EN is pulled down forever. >>>>>> >>>>>> - If the board manufacturer wants to keep open the possibility >>>>>> to adjust some parameters at run-time, then they must connect >>>>>> the device to an I2C bus, and I2C_EN is pulled up forever. >>>>> >>>>> How do you express both cases in your current binding? >>>> >>>> It's not that I'm ignoring your question. >>>> >>>> It's that I don't understand what you're asking. >>> >>> And that's fine, you just need to say so. >>> >>> Generally speaking, you're focusing on the driver. The driver is not the >>> issue here. You can do whatever you want in the driver for all I care, >>> we can change that later on as we wish. >>> >>> The binding however cannot change, so it *has* to ideally cover all >>> possible situations the hardware can be used in, or at a minimum leave >>> the door open to support those without a compatibility breakage. >>> >>> That's why I've been asking those questions, because so far the only >>> thing you've claimed is that "I can't test the driver for anything >>> else", but, again, whether there's a driver or not, or if it's >>> functional, is completely missing the point. >>> >>>> SITUATION 1 >>>> tdp158 is pin strapped. >>>> Device node is child of root node. >>>> Properties in proposed binding are valid (regulators and power-on pin) >>>> Can be supported via module_platform_driver. >>>> >>>> SITUATION 2 >>>> tdp158 is sitting on I2C bus. >>>> Device node is child of i2c bus node. >>>> (robh said missing reg prop would be flagged by the compiler) >>>> Properties in proposed binding are valid (regulators and power-on pin) >>>> Supported via module_i2c_driver. >>>> >>>> If some settings-specific properties are added later, like skew, >>>> they would only be valid for the I2C programmable mode, obviously. >>> >>> I think there's a couple more combinations: >>> >>> - The device is connected on an I2C bus, but I2C_EN is tied low >> >> No, this is not possible. I2C pins are repurposed if I2C_EN is low. >> You can not call that an i2c bus anymore. >> >>> - The device is connected on an I2C bus, but I2C_EN is connected to a >>> GPIO and the kernel needs to assert its state at boot. >> >> This is a pretty strange configuration. The I2C_EN pin isn't supposed >> to be toggled dynamically. Anyway, if that happens, I'd use pinctrl / >> hog to control the pin. > > ACK. I still believe it would be valuable, but I don't really want to be > part of that conversation anymore. Marc, do whatever you want. OK. I'll send the v4 sitting in my queue. Regards
On Tue, Jul 30, 2024 at 11:30:01AM GMT, Maxime Ripard wrote: > On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote: > > On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote: > > > > On 15/07/2024 16:40, Maxime Ripard wrote: > > > > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: > > > > >> On 01/07/2024 15:50, Maxime Ripard wrote: > > > > >> > > > > >>> 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. > > > > >> > > > > >> I think the purpose of the I2C_EN pin might have been misunderstood. > > > > >> > > > > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet. > > > > > > > > > > Toggled, probably not. Connected to a GPIO and the kernel has to assert > > > > > a level at boot, I've seen worse hardware design already. > > > > > > > > > >> I2C_EN is a layout-time setting, decided by a board manufacturer: > > > > >> > > > > >> - If the TDP158 is fully configured once-and-for-all at layout-time, > > > > >> then no I2C bus is required, and I2C_EN is pulled down forever. > > > > >> > > > > >> - If the board manufacturer wants to keep open the possibility > > > > >> to adjust some parameters at run-time, then they must connect > > > > >> the device to an I2C bus, and I2C_EN is pulled up forever. > > > > > > > > > > How do you express both cases in your current binding? > > > > > > > > It's not that I'm ignoring your question. > > > > > > > > It's that I don't understand what you're asking. > > > > > > And that's fine, you just need to say so. > > > > > > Generally speaking, you're focusing on the driver. The driver is not the > > > issue here. You can do whatever you want in the driver for all I care, > > > we can change that later on as we wish. > > > > > > The binding however cannot change, so it *has* to ideally cover all > > > possible situations the hardware can be used in, or at a minimum leave > > > the door open to support those without a compatibility breakage. > > > > > > That's why I've been asking those questions, because so far the only > > > thing you've claimed is that "I can't test the driver for anything > > > else", but, again, whether there's a driver or not, or if it's > > > functional, is completely missing the point. > > > > > > > SITUATION 1 > > > > tdp158 is pin strapped. > > > > Device node is child of root node. > > > > Properties in proposed binding are valid (regulators and power-on pin) > > > > Can be supported via module_platform_driver. > > > > > > > > SITUATION 2 > > > > tdp158 is sitting on I2C bus. > > > > Device node is child of i2c bus node. > > > > (robh said missing reg prop would be flagged by the compiler) > > > > Properties in proposed binding are valid (regulators and power-on pin) > > > > Supported via module_i2c_driver. > > > > > > > > If some settings-specific properties are added later, like skew, > > > > they would only be valid for the I2C programmable mode, obviously. > > > > > > I think there's a couple more combinations: > > > > > > - The device is connected on an I2C bus, but I2C_EN is tied low > > > > No, this is not possible. I2C pins are repurposed if I2C_EN is low. > > You can not call that an i2c bus anymore. > > > > > - The device is connected on an I2C bus, but I2C_EN is connected to a > > > GPIO and the kernel needs to assert its state at boot. > > > > This is a pretty strange configuration. The I2C_EN pin isn't supposed > > to be toggled dynamically. Anyway, if that happens, I'd use pinctrl / > > hog to control the pin. > > ACK. I still believe it would be valuable, but I don't really want to be > part of that conversation anymore. Marc, do whatever you want. Just to explain it, from my way of thinking the I2C_EN pin is the same as the address-strapping pins: they are usually hardwired by the device manufacturer.
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@freebox.fr> + - Pierre-Hugues Husson <phhusson@freebox.fr> + +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 + +required: + - compatible + - vcc-supply + - vdd-supply + - ports + +additionalProperties: false
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@freebox.fr> --- .../bindings/display/bridge/ti,tdp158.yaml | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+)