[v3,1/2] dt-bindings: display: bridge: add TI TDP158

Commit Message

Marc Gonzalez June 27, 2024, 11:13 a.m. UTC

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 <[e-mail]>
---
 .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)

Comments

Conor Dooley June 27, 2024, 4:25 p.m. UTC | #1

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 <[e-mail]>
> ---
>  .../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 <[e-mail]>
> +  - Pierre-Hugues Husson <[e-mail]>
> +
> +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

Marc Gonzalez June 27, 2024, 4:45 p.m. UTC | #2

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 <[e-mail]>
>> ---
>>  .../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 <[e-mail]>
>> +  - Pierre-Hugues Husson <[e-mail]>
>> +
>> +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

Krzysztof Kozlowski June 28, 2024, 7:36 a.m. UTC | #3

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 <[e-mail]>
>>> ---
>>>  .../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 <[e-mail]>
>>> +  - Pierre-Hugues Husson <[e-mail]>
>>> +
>>> +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

Dmitry Baryshkov June 28, 2024, 7:49 a.m. UTC | #4

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 <[e-mail]>
> >>> ---
> >>>  .../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 <[e-mail]>
> >>> +  - Pierre-Hugues Husson <[e-mail]>
> >>> +
> >>> +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.

Maxime Ripard July 1, 2024, 1:50 p.m. UTC | #5

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 <[e-mail]>
> ---
>  .../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 <[e-mail]>
> +  - Pierre-Hugues Husson <[e-mail]>
> +
> +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

Marc Gonzalez July 1, 2024, 2:31 p.m. UTC | #6

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 <[e-mail]>
>>>>> ---
>>>>>  .../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 <[e-mail]>
>>>>> +  - Pierre-Hugues Husson <[e-mail]>
>>>>> +
>>>>> +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

Marc Gonzalez July 1, 2024, 3:36 p.m. UTC | #7

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 <[e-mail]>
>> ---
>>  .../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 <[e-mail]>
>> +  - Pierre-Hugues Husson <[e-mail]>
>> +
>> +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

Marc Gonzalez July 4, 2024, 5:04 p.m. UTC | #8

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

Maxime Ripard July 8, 2024, 2:59 p.m. UTC | #9

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 <[e-mail]>
> >> ---
> >>  .../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 <[e-mail]>
> >> +  - Pierre-Hugues Husson <[e-mail]>
> >> +
> >> +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

Dmitry Baryshkov July 8, 2024, 8:29 p.m. UTC | #10

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 <[e-mail]>
> > >> ---
> > >>  .../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 <[e-mail]>
> > >> +  - Pierre-Hugues Husson <[e-mail]>
> > >> +
> > >> +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.

Maxime Ripard July 15, 2024, 2:40 p.m. UTC | #11

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

Maxime Ripard July 15, 2024, 2:42 p.m. UTC | #12

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 <[e-mail]>
> > > >> ---
> > > >>  .../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 <[e-mail]>
> > > >> +  - Pierre-Hugues Husson <[e-mail]>
> > > >> +
> > > >> +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

Dmitry Baryshkov July 15, 2024, 4:38 p.m. UTC | #13

On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <[e-mail]> 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 <[e-mail]>
> > > > >> ---
> > > > >>  .../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 <[e-mail]>
> > > > >> +  - Pierre-Hugues Husson <[e-mail]>
> > > > >> +
> > > > >> +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

Maxime Ripard July 16, 2024, 9:24 a.m. UTC | #14

Hi,

On Mon, Jul 15, 2024 at 07:38:34PM GMT, Dmitry Baryshkov wrote:
> On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <[e-mail]> 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 <[e-mail]>
> > > > > >> ---
> > > > > >>  .../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 <[e-mail]>
> > > > > >> +  - Pierre-Hugues Husson <[e-mail]>
> > > > > >> +
> > > > > >> +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

Dmitry Baryshkov July 16, 2024, 10:59 a.m. UTC | #15

On Tue, 16 Jul 2024 at 12:24, Maxime Ripard <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > > > >> ---
> > > > > > >>  .../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 <[e-mail]>
> > > > > > >> +  - Pierre-Hugues Husson <[e-mail]>
> > > > > > >> +
> > > > > > >> +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 :-(

Marc Gonzalez July 23, 2024, 3:17 p.m. UTC | #16

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 <[e-mail]>
>> ---
>>  .../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 <[e-mail]>
>> +  - Pierre-Hugues Husson <[e-mail]>
>> +
>> +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

Conor Dooley July 23, 2024, 7:57 p.m. UTC | #17

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 <[e-mail]>
> >> ---
> >>  .../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 <[e-mail]>
> >> +  - Pierre-Hugues Husson <[e-mail]>
> >> +
> >> +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.

Maxime Ripard July 24, 2024, 2:03 p.m. UTC | #18

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 <[e-mail]>
> > >> ---
> > >>  .../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 <[e-mail]>
> > >> +  - Pierre-Hugues Husson <[e-mail]>
> > >> +
> > >> +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

Marc Gonzalez July 24, 2024, 5:18 p.m. UTC | #19

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 <[e-mail]>
>>>>>> ---
>>>>>>  .../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 <[e-mail]>
>>>>>> +  - Pierre-Hugues Husson <[e-mail]>
>>>>>> +
>>>>>> +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

Marc Gonzalez July 24, 2024, 5:25 p.m. UTC | #20

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 <[e-mail]> 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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>  .../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 <[e-mail]>
>>>>>>>> +  - Pierre-Hugues Husson <[e-mail]>
>>>>>>>> +
>>>>>>>> +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>;
				};
			};
		};
	};
};

Dmitry Baryshkov July 24, 2024, 5:25 p.m. UTC | #21

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 <[e-mail]>
> >>>>>> ---
> >>>>>>  .../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 <[e-mail]>
> >>>>>> +  - Pierre-Hugues Husson <[e-mail]>
> >>>>>> +
> >>>>>> +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.

Marc Gonzalez July 24, 2024, 5:28 p.m. UTC | #22

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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>  .../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 <[e-mail]>
>>>>>>>> +  - Pierre-Hugues Husson <[e-mail]>
>>>>>>>> +
>>>>>>>> +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.

Marc Gonzalez July 24, 2024, 5:34 p.m. UTC | #23

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

Marc Gonzalez July 24, 2024, 5:59 p.m. UTC | #24

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

Maxime Ripard July 30, 2024, 8:27 a.m. UTC | #25

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

Maxime Ripard July 30, 2024, 8:44 a.m. UTC | #26

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 <[e-mail]>
> >>>>>>>> ---
> >>>>>>>>  .../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 <[e-mail]>
> >>>>>>>> +  - Pierre-Hugues Husson <[e-mail]>
> >>>>>>>> +
> >>>>>>>> +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

Dmitry Baryshkov July 30, 2024, 8:46 a.m. UTC | #27

On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <[e-mail]> 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).

Maxime Ripard July 30, 2024, 9:30 a.m. UTC | #28

On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote:
> On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <[e-mail]> 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

Marc Gonzalez July 30, 2024, 9:44 a.m. UTC | #29

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 <[e-mail]> 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

Dmitry Baryshkov July 30, 2024, 10:38 a.m. UTC | #30

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 <[e-mail]> 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.

Patch

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 <[e-mail]>
+  - Pierre-Hugues Husson <[e-mail]>
+
+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