[PATCH v3 0/2] Basic support for TI TDP158

[PATCH v3 0/2] Basic support for TI TDP158

[Marc Gonzalez]

---
Changes in v3:
- Add 'select DRM_PANEL_BRIDGE' in driver Kconfig
- Fix checkpatch errors
- log errors using dev_err (so save dev pointer)
- expand a few error messages
- expand commit messages with info from the datasheet
- mark regulators as required in the DT binding
- Link to v2: https://lore.kernel.org/r/20240625-tdp158-v2-0-a3b344707fa7@freebox.fr

Changes in v2:
- Don't overload simple-bridge, spin new minimal driver
- New driver, new binding
- Default device settings work fine for us, so we don't tweak registers
- Link to v1: https://lore.kernel.org/r/20240617-tdp158-v1-0-df98ef7dec6d@freebox.fr

Getting unusual message at run-time, need to check.

[    2.389848] platform c9a0000.hdmi-tx: Fixed dependency cycle(s) with /soc@0/i2c@c1b5000/tdp158@5e
[    2.391089] i2c 2-005e: Fixed dependency cycle(s) with /soc@0/display-subsystem@c900000/hdmi-tx@c9a0000

---
Marc Gonzalez (2):
      dt-bindings: display: bridge: add TI TDP158
      drm/bridge: add support for TI TDP158

 .../bindings/display/bridge/ti,tdp158.yaml         |  51 ++++++++++
 drivers/gpu/drm/bridge/Kconfig                     |   7 ++
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/ti-tdp158.c                 | 108 +++++++++++++++++++++
 4 files changed, 167 insertions(+)
---
base-commit: d47e2c964a51cbaa14a8c0ac641f85349584fae9
change-id: 20240617-tdp158-418200d6cc0b

Best regards,
-- 
Marc Gonzalez <mgonzalez@freebox.fr>

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

[Marc Gonzalez]

TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
It supports DVI 1.0, HDMI 1.4b and 2.0b.
It supports 4 TMDS channels, HPD, and a DDC interface.
It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
for power reduction. Several methods of power management are
implemented to reduce overall power consumption.
It supports fixed receiver EQ gain using I2C or pin strap to
compensate for different lengths input cable or board traces.

Features

- AC-coupled TMDS or DisplayPort dual-mode physical layer input
to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
data rate, compatible with HDMI 2.0b electrical parameters
- DisplayPort dual-mode standard version 1.1
- Programmable fixed receiver equalizer up to 15.5dB
- Global or independent high speed lane control, pre-emphasis
and transmit swing, and slew rate control
- I2C or pin strap programmable
- Configurable as a DisplayPort redriver through I2C
- Full lane swap on main lanes
- Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)

https://www.ti.com/lit/ds/symlink/tdp158.pdf

Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
 .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
new file mode 100644
index 0000000000000..21c8585c3bb2d
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI TDP158 HDMI to TMDS Redriver
+
+maintainers:
+  - Arnaud Vrac <avrac@freebox.fr>
+  - Pierre-Hugues Husson <phhusson@freebox.fr>
+
+properties:
+  compatible:
+    const: ti,tdp158
+
+  reg:
+    description: I2C address of the device
+
+  enable-gpios:
+    description: GPIO controlling bridge enable
+
+  vcc-supply:
+    description: Power supply 3.3V
+
+  vdd-supply:
+    description: Power supply 1.1V
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Bridge input
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Bridge output
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - vcc-supply
+  - vdd-supply
+  - ports
+
+additionalProperties: false

-- 
2.34.1

[PATCH v3 2/2] drm/bridge: add support for TI TDP158

[Marc Gonzalez]

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

The default settings work fine for our use-case.
So this basic driver doesn't tweak any I2C registers.

Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
 drivers/gpu/drm/bridge/Kconfig     |   7 +++
 drivers/gpu/drm/bridge/Makefile    |   1 +
 drivers/gpu/drm/bridge/ti-tdp158.c | 108 +++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index c621be1a99a89..c0ab5b620b57d 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -368,6 +368,13 @@ config DRM_TI_DLPC3433
 	  It supports up to 720p resolution with 60 and 120 Hz refresh
 	  rates.
 
+config DRM_TI_TDP158
+	tristate "TI TDP158 HDMI/TMDS bridge"
+	depends on OF
+	select DRM_PANEL_BRIDGE
+	help
+	  Texas Instruments TDP158 HDMI/TMDS Bridge driver
+
 config DRM_TI_TFP410
 	tristate "TI TFP410 DVI/HDMI bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 7df87b582dca3..3daf803ce80b6 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
 obj-$(CONFIG_DRM_TI_DLPC3433) += ti-dlpc3433.o
 obj-$(CONFIG_DRM_TI_SN65DSI83) += ti-sn65dsi83.o
 obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
+obj-$(CONFIG_DRM_TI_TDP158) += ti-tdp158.o
 obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
 obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
 obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
diff --git a/drivers/gpu/drm/bridge/ti-tdp158.c b/drivers/gpu/drm/bridge/ti-tdp158.c
new file mode 100644
index 0000000000000..58827c4d59a08
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-tdp158.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Freebox SAS
+ */
+#include <drm/drm_bridge.h>
+#include <drm/drm_atomic_helper.h>
+#include <linux/i2c.h>
+
+struct tdp158 {
+	struct drm_bridge bridge;
+	struct drm_bridge *next;
+	struct gpio_desc *enable; // Operation Enable - pin 36
+	struct regulator *vcc; // 3.3V
+	struct regulator *vdd; // 1.1V
+	struct device *dev;
+};
+
+static void tdp158_enable(struct drm_bridge *bridge, struct drm_bridge_state *prev)
+{
+	int err;
+	struct tdp158 *tdp158 = bridge->driver_private;
+
+	err = regulator_enable(tdp158->vcc);
+	if (err)
+		dev_err(tdp158->dev, "failed to enable vcc: %d", err);
+
+	err = regulator_enable(tdp158->vdd);
+	if (err)
+		dev_err(tdp158->dev, "failed to enable vdd: %d", err);
+
+	gpiod_set_value_cansleep(tdp158->enable, 1);
+}
+
+static void tdp158_disable(struct drm_bridge *bridge, struct drm_bridge_state *prev)
+{
+	struct tdp158 *tdp158 = bridge->driver_private;
+
+	gpiod_set_value_cansleep(tdp158->enable, 0);
+	regulator_disable(tdp158->vdd);
+	regulator_disable(tdp158->vcc);
+}
+
+static int tdp158_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
+{
+	struct tdp158 *tdp158 = bridge->driver_private;
+
+	return drm_bridge_attach(bridge->encoder, tdp158->next, bridge, flags);
+}
+
+static const struct drm_bridge_funcs tdp158_bridge_funcs = {
+	.attach = tdp158_attach,
+	.atomic_enable = tdp158_enable,
+	.atomic_disable = tdp158_disable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+};
+
+static int tdp158_bridge_probe(struct i2c_client *client)
+{
+	struct tdp158 *tdp158;
+	struct device *dev = &client->dev;
+
+	tdp158 = devm_kzalloc(dev, sizeof(*tdp158), GFP_KERNEL);
+	if (!tdp158)
+		return -ENOMEM;
+
+	tdp158->next = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+	if (IS_ERR(tdp158->next))
+		return dev_err_probe(dev, PTR_ERR(tdp158->next), "missing bridge");
+
+	tdp158->vcc = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(tdp158->vcc))
+		return dev_err_probe(dev, PTR_ERR(tdp158->vcc), "vcc");
+
+	tdp158->vdd = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(tdp158->vdd))
+		return dev_err_probe(dev, PTR_ERR(tdp158->vdd), "vdd");
+
+	tdp158->enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(tdp158->enable))
+		return dev_err_probe(dev, PTR_ERR(tdp158->enable), "enable");
+
+	tdp158->bridge.of_node = dev->of_node;
+	tdp158->bridge.funcs = &tdp158_bridge_funcs;
+	tdp158->bridge.driver_private = tdp158;
+	tdp158->dev = dev;
+
+	return devm_drm_bridge_add(dev, &tdp158->bridge);
+}
+
+static const struct of_device_id tdp158_bridge_match_table[] = {
+	{ .compatible = "ti,tdp158" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tdp158_bridge_match_table);
+
+static struct i2c_driver tdp158_bridge_driver = {
+	.probe = tdp158_bridge_probe,
+	.driver = {
+		.name = "tdp158-bridge",
+		.of_match_table = tdp158_bridge_match_table,
+	},
+};
+module_i2c_driver(tdp158_bridge_driver);
+
+MODULE_DESCRIPTION("TI TDP158 driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> It supports 4 TMDS channels, HPD, and a DDC interface.
> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> for power reduction. Several methods of power management are
> implemented to reduce overall power consumption.
> It supports fixed receiver EQ gain using I2C or pin strap to
> compensate for different lengths input cable or board traces.
> 
> Features
> 
> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> data rate, compatible with HDMI 2.0b electrical parameters
> - DisplayPort dual-mode standard version 1.1
> - Programmable fixed receiver equalizer up to 15.5dB
> - Global or independent high speed lane control, pre-emphasis
> and transmit swing, and slew rate control
> - I2C or pin strap programmable
> - Configurable as a DisplayPort redriver through I2C
> - Full lane swap on main lanes
> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> 
> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> new file mode 100644
> index 0000000000000..21c8585c3bb2d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI TDP158 HDMI to TMDS Redriver
> +
> +maintainers:
> +  - Arnaud Vrac <avrac@freebox.fr>
> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> +
> +properties:
> +  compatible:
> +    const: ti,tdp158
> +
> +  reg:
> +    description: I2C address of the device

Is reg not required? How do you communicate with the device if the i2c
bus is not connected? Is the enable GPIO enough to operate it in some
situations?

Otherwise this looks good to me, but given Maxime commented about the
complexity of the device, I'm probably out of my depth!

> +required:
> +  - compatible
> +  - vcc-supply
> +  - vdd-supply
> +  - ports


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Marc Gonzalez]

On 27/06/2024 18:25, Conor Dooley wrote:

> On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
>
>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
>> It supports 4 TMDS channels, HPD, and a DDC interface.
>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
>> for power reduction. Several methods of power management are
>> implemented to reduce overall power consumption.
>> It supports fixed receiver EQ gain using I2C or pin strap to
>> compensate for different lengths input cable or board traces.
>>
>> Features
>>
>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
>> data rate, compatible with HDMI 2.0b electrical parameters
>> - DisplayPort dual-mode standard version 1.1
>> - Programmable fixed receiver equalizer up to 15.5dB
>> - Global or independent high speed lane control, pre-emphasis
>> and transmit swing, and slew rate control
>> - I2C or pin strap programmable
>> - Configurable as a DisplayPort redriver through I2C
>> - Full lane swap on main lanes
>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
>>
>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
>>
>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>> ---
>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> new file mode 100644
>> index 0000000000000..21c8585c3bb2d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: TI TDP158 HDMI to TMDS Redriver
>> +
>> +maintainers:
>> +  - Arnaud Vrac <avrac@freebox.fr>
>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,tdp158
>> +
>> +  reg:
>> +    description: I2C address of the device
> 
> Is reg not required? How do you communicate with the device if the i2c
> bus is not connected? Is the enable GPIO enough to operate it in some
> situations?
> 
> Otherwise this looks good to me, but given Maxime commented about the
> complexity of the device, I'm probably out of my depth!

Valid question.

As discussed in my brilliantly expanded commit message (:p)
the device can be configured in various ways, either through I2C registers
or by pin strap. We use the device in its default settings, so we don't
touch any I2C registers, thus I'm not sure the reg property is required.

>> +required:
>> +  - compatible
>> +  - vcc-supply
>> +  - vdd-supply
>> +  - ports


Regards

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

[Krzysztof Kozlowski]

On 27/06/2024 18:45, Marc Gonzalez wrote:
> On 27/06/2024 18:25, Conor Dooley wrote:
> 
>> On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
>>
>>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
>>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
>>> It supports 4 TMDS channels, HPD, and a DDC interface.
>>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
>>> for power reduction. Several methods of power management are
>>> implemented to reduce overall power consumption.
>>> It supports fixed receiver EQ gain using I2C or pin strap to
>>> compensate for different lengths input cable or board traces.
>>>
>>> Features
>>>
>>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
>>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
>>> data rate, compatible with HDMI 2.0b electrical parameters
>>> - DisplayPort dual-mode standard version 1.1
>>> - Programmable fixed receiver equalizer up to 15.5dB
>>> - Global or independent high speed lane control, pre-emphasis
>>> and transmit swing, and slew rate control
>>> - I2C or pin strap programmable
>>> - Configurable as a DisplayPort redriver through I2C
>>> - Full lane swap on main lanes
>>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
>>>
>>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
>>>
>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>>> ---
>>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>>>  1 file changed, 51 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>> new file mode 100644
>>> index 0000000000000..21c8585c3bb2d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>> @@ -0,0 +1,51 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: TI TDP158 HDMI to TMDS Redriver
>>> +
>>> +maintainers:
>>> +  - Arnaud Vrac <avrac@freebox.fr>
>>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ti,tdp158
>>> +
>>> +  reg:
>>> +    description: I2C address of the device
>>
>> Is reg not required? How do you communicate with the device if the i2c
>> bus is not connected? Is the enable GPIO enough to operate it in some
>> situations?
>>
>> Otherwise this looks good to me, but given Maxime commented about the
>> complexity of the device, I'm probably out of my depth!
> 
> Valid question.
> 
> As discussed in my brilliantly expanded commit message (:p)
> the device can be configured in various ways, either through I2C registers
> or by pin strap. We use the device in its default settings, so we don't
> touch any I2C registers, thus I'm not sure the reg property is required.

But then how would it be represented in the DT? Where / under which parent?

If this is supposed to be always in I2C bus in DT, then you always need
reg. If you could place it in other place, then your reasoning is valid
- reg is optional.

Best regards,
Krzysztof

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

[Dmitry Baryshkov]

On Fri, Jun 28, 2024 at 09:36:57AM GMT, Krzysztof Kozlowski wrote:
> On 27/06/2024 18:45, Marc Gonzalez wrote:
> > On 27/06/2024 18:25, Conor Dooley wrote:
> > 
> >> On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
> >>
> >>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> >>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> >>> It supports 4 TMDS channels, HPD, and a DDC interface.
> >>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> >>> for power reduction. Several methods of power management are
> >>> implemented to reduce overall power consumption.
> >>> It supports fixed receiver EQ gain using I2C or pin strap to
> >>> compensate for different lengths input cable or board traces.
> >>>
> >>> Features
> >>>
> >>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> >>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> >>> data rate, compatible with HDMI 2.0b electrical parameters
> >>> - DisplayPort dual-mode standard version 1.1
> >>> - Programmable fixed receiver equalizer up to 15.5dB
> >>> - Global or independent high speed lane control, pre-emphasis
> >>> and transmit swing, and slew rate control
> >>> - I2C or pin strap programmable
> >>> - Configurable as a DisplayPort redriver through I2C
> >>> - Full lane swap on main lanes
> >>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> >>>
> >>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> >>>
> >>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> >>> ---
> >>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> >>>  1 file changed, 51 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >>> new file mode 100644
> >>> index 0000000000000..21c8585c3bb2d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >>> @@ -0,0 +1,51 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: TI TDP158 HDMI to TMDS Redriver
> >>> +
> >>> +maintainers:
> >>> +  - Arnaud Vrac <avrac@freebox.fr>
> >>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: ti,tdp158
> >>> +
> >>> +  reg:
> >>> +    description: I2C address of the device
> >>
> >> Is reg not required? How do you communicate with the device if the i2c
> >> bus is not connected? Is the enable GPIO enough to operate it in some
> >> situations?
> >>
> >> Otherwise this looks good to me, but given Maxime commented about the
> >> complexity of the device, I'm probably out of my depth!
> > 
> > Valid question.
> > 
> > As discussed in my brilliantly expanded commit message (:p)
> > the device can be configured in various ways, either through I2C registers
> > or by pin strap. We use the device in its default settings, so we don't
> > touch any I2C registers, thus I'm not sure the reg property is required.
> 
> But then how would it be represented in the DT? Where / under which parent?
> 
> If this is supposed to be always in I2C bus in DT, then you always need
> reg. If you could place it in other place, then your reasoning is valid
> - reg is optional.

As far as I understood, the device is connected to I2C bus, it just
doesn't need to be programmed. So I'd conclude that reg is required.

-- 
With best wishes
Dmitry

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3274 bytes --]

Hi,

On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> It supports 4 TMDS channels, HPD, and a DDC interface.
> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> for power reduction. Several methods of power management are
> implemented to reduce overall power consumption.
> It supports fixed receiver EQ gain using I2C or pin strap to
> compensate for different lengths input cable or board traces.
> 
> Features
> 
> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> data rate, compatible with HDMI 2.0b electrical parameters
> - DisplayPort dual-mode standard version 1.1
> - Programmable fixed receiver equalizer up to 15.5dB
> - Global or independent high speed lane control, pre-emphasis
> and transmit swing, and slew rate control
> - I2C or pin strap programmable
> - Configurable as a DisplayPort redriver through I2C
> - Full lane swap on main lanes
> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> 
> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> new file mode 100644
> index 0000000000000..21c8585c3bb2d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI TDP158 HDMI to TMDS Redriver
> +
> +maintainers:
> +  - Arnaud Vrac <avrac@freebox.fr>
> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> +
> +properties:
> +  compatible:
> +    const: ti,tdp158
> +
> +  reg:
> +    description: I2C address of the device
> +
> +  enable-gpios:
> +    description: GPIO controlling bridge enable
> +
> +  vcc-supply:
> +    description: Power supply 3.3V
> +
> +  vdd-supply:
> +    description: Power supply 1.1V
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Bridge input
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Bridge output
> +
> +    required:
> +      - port@0
> +      - port@1

The device supports DVI, HDMI or DP input, with various requirements and
capabilities depending on the input. Your binding doesn't address that.

Similarly, it can do lane-swapping, so we should probably have a
property to describe what mapping we want to use.

The i2c register access (and the whole behaviour of the device) is
constrained on the I2C_EN pin status, and you can't read it from the
device, so it's also something we need to have in the DT.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Marc Gonzalez]

On 28/06/2024 09:49, Dmitry Baryshkov wrote:
> On Fri, Jun 28, 2024 at 09:36:57AM GMT, Krzysztof Kozlowski wrote:
>> On 27/06/2024 18:45, Marc Gonzalez wrote:
>>> On 27/06/2024 18:25, Conor Dooley wrote:
>>>> On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
>>>>
>>>>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
>>>>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
>>>>> It supports 4 TMDS channels, HPD, and a DDC interface.
>>>>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
>>>>> for power reduction. Several methods of power management are
>>>>> implemented to reduce overall power consumption.
>>>>> It supports fixed receiver EQ gain using I2C or pin strap to
>>>>> compensate for different lengths input cable or board traces.
>>>>>
>>>>> Features
>>>>>
>>>>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
>>>>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
>>>>> data rate, compatible with HDMI 2.0b electrical parameters
>>>>> - DisplayPort dual-mode standard version 1.1
>>>>> - Programmable fixed receiver equalizer up to 15.5dB
>>>>> - Global or independent high speed lane control, pre-emphasis
>>>>> and transmit swing, and slew rate control
>>>>> - I2C or pin strap programmable
>>>>> - Configurable as a DisplayPort redriver through I2C
>>>>> - Full lane swap on main lanes
>>>>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
>>>>>
>>>>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
>>>>>
>>>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>>>>> ---
>>>>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>>>>>  1 file changed, 51 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>>>> new file mode 100644
>>>>> index 0000000000000..21c8585c3bb2d
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>>>> @@ -0,0 +1,51 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: TI TDP158 HDMI to TMDS Redriver
>>>>> +
>>>>> +maintainers:
>>>>> +  - Arnaud Vrac <avrac@freebox.fr>
>>>>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: ti,tdp158
>>>>> +
>>>>> +  reg:
>>>>> +    description: I2C address of the device
>>>>
>>>> Is reg not required? How do you communicate with the device if the i2c
>>>> bus is not connected? Is the enable GPIO enough to operate it in some
>>>> situations?
>>>>
>>>> Otherwise this looks good to me, but given Maxime commented about the
>>>> complexity of the device, I'm probably out of my depth!
>>>
>>> Valid question.
>>>
>>> As discussed in my brilliantly expanded commit message (:p)
>>> the device can be configured in various ways, either through I2C registers
>>> or by pin strap. We use the device in its default settings, so we don't
>>> touch any I2C registers, thus I'm not sure the reg property is required.
>>
>> But then how would it be represented in the DT? Where / under which parent?
>>
>> If this is supposed to be always in I2C bus in DT, then you always need
>> reg. If you could place it in other place, then your reasoning is valid
>> - reg is optional.
> 
> As far as I understood, the device is connected to I2C bus, it just
> doesn't need to be programmed. So I'd conclude that reg is required.

Just to be clear (and as far as I understand),
the TDP158 can be configured via 2 different methods:
- dynamically at run-time, through I2C registers (requires an I2C bus)
- statically at layout-time through pin straps (no I2C bus required)

On our board, the TDP158 is connected to blsp2_i2c1.

So, in my understanding, the "reg" property would be required
for the first method, but is not applicable for the second method.

I don't feel strongly about the issue, so I can mark the "reg"
property as required if it makes more sense.

Regards

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

[Marc Gonzalez]

On 01/07/2024 15:50, Maxime Ripard wrote:

> On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
>
>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
>> It supports 4 TMDS channels, HPD, and a DDC interface.
>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
>> for power reduction. Several methods of power management are
>> implemented to reduce overall power consumption.
>> It supports fixed receiver EQ gain using I2C or pin strap to
>> compensate for different lengths input cable or board traces.
>>
>> Features
>>
>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
>> data rate, compatible with HDMI 2.0b electrical parameters
>> - DisplayPort dual-mode standard version 1.1
>> - Programmable fixed receiver equalizer up to 15.5dB
>> - Global or independent high speed lane control, pre-emphasis
>> and transmit swing, and slew rate control
>> - I2C or pin strap programmable
>> - Configurable as a DisplayPort redriver through I2C
>> - Full lane swap on main lanes
>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
>>
>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
>>
>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>> ---
>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> new file mode 100644
>> index 0000000000000..21c8585c3bb2d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: TI TDP158 HDMI to TMDS Redriver
>> +
>> +maintainers:
>> +  - Arnaud Vrac <avrac@freebox.fr>
>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,tdp158
>> +
>> +  reg:
>> +    description: I2C address of the device
>> +
>> +  enable-gpios:
>> +    description: GPIO controlling bridge enable
>> +
>> +  vcc-supply:
>> +    description: Power supply 3.3V
>> +
>> +  vdd-supply:
>> +    description: Power supply 1.1V
>> +
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: Bridge input
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: Bridge output
>> +
>> +    required:
>> +      - port@0
>> +      - port@1
> 
> The device supports DVI, HDMI or DP input, with various requirements and
> capabilities depending on the input. Your binding doesn't address that.
> 
> Similarly, it can do lane-swapping, so we should probably have a
> property to describe what mapping we want to use.
> 
> The i2c register access (and the whole behaviour of the device) is
> constrained on the I2C_EN pin status, and you can't read it from the
> device, so it's also something we need to have in the DT.

We are using the device in its default configuration.
(Power on via OE, then it works as expected)

Can we leave any additional properties to be defined
by whomever needs them in the future?

Regards

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

[Marc Gonzalez]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]

On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
> On 01/07/2024 15:50, Maxime Ripard wrote:
> 
> > On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> >
> >> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> >> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> >> It supports 4 TMDS channels, HPD, and a DDC interface.
> >> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> >> for power reduction. Several methods of power management are
> >> implemented to reduce overall power consumption.
> >> It supports fixed receiver EQ gain using I2C or pin strap to
> >> compensate for different lengths input cable or board traces.
> >>
> >> Features
> >>
> >> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> >> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> >> data rate, compatible with HDMI 2.0b electrical parameters
> >> - DisplayPort dual-mode standard version 1.1
> >> - Programmable fixed receiver equalizer up to 15.5dB
> >> - Global or independent high speed lane control, pre-emphasis
> >> and transmit swing, and slew rate control
> >> - I2C or pin strap programmable
> >> - Configurable as a DisplayPort redriver through I2C
> >> - Full lane swap on main lanes
> >> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> >>
> >> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> >>
> >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> >> ---
> >>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >> new file mode 100644
> >> index 0000000000000..21c8585c3bb2d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >> @@ -0,0 +1,51 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: TI TDP158 HDMI to TMDS Redriver
> >> +
> >> +maintainers:
> >> +  - Arnaud Vrac <avrac@freebox.fr>
> >> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: ti,tdp158
> >> +
> >> +  reg:
> >> +    description: I2C address of the device
> >> +
> >> +  enable-gpios:
> >> +    description: GPIO controlling bridge enable
> >> +
> >> +  vcc-supply:
> >> +    description: Power supply 3.3V
> >> +
> >> +  vdd-supply:
> >> +    description: Power supply 1.1V
> >> +
> >> +  ports:
> >> +    $ref: /schemas/graph.yaml#/properties/ports
> >> +
> >> +    properties:
> >> +      port@0:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        description: Bridge input
> >> +
> >> +      port@1:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        description: Bridge output
> >> +
> >> +    required:
> >> +      - port@0
> >> +      - port@1
> > 
> > The device supports DVI, HDMI or DP input, with various requirements and
> > capabilities depending on the input. Your binding doesn't address that.
> > 
> > Similarly, it can do lane-swapping, so we should probably have a
> > property to describe what mapping we want to use.
> > 
> > The i2c register access (and the whole behaviour of the device) is
> > constrained on the I2C_EN pin status, and you can't read it from the
> > device, so it's also something we need to have in the DT.
> 
> We are using the device in its default configuration.
> (Power on via OE, then it works as expected)

I know, but that doesn't really matter for a binding.

> Can we leave any additional properties to be defined by whomever needs
> them in the future?

If you can guarantee that doing so would be backward compatible, sure.
But that means being able to answer those questions with a reasonable
plan.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Dmitry Baryshkov]

On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
> On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
> > On 01/07/2024 15:50, Maxime Ripard wrote:
> > 
> > > On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> > >
> > >> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> > >> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> > >> It supports 4 TMDS channels, HPD, and a DDC interface.
> > >> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> > >> for power reduction. Several methods of power management are
> > >> implemented to reduce overall power consumption.
> > >> It supports fixed receiver EQ gain using I2C or pin strap to
> > >> compensate for different lengths input cable or board traces.
> > >>
> > >> Features
> > >>
> > >> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> > >> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> > >> data rate, compatible with HDMI 2.0b electrical parameters
> > >> - DisplayPort dual-mode standard version 1.1
> > >> - Programmable fixed receiver equalizer up to 15.5dB
> > >> - Global or independent high speed lane control, pre-emphasis
> > >> and transmit swing, and slew rate control
> > >> - I2C or pin strap programmable
> > >> - Configurable as a DisplayPort redriver through I2C
> > >> - Full lane swap on main lanes
> > >> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> > >>
> > >> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> > >>
> > >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> > >> ---
> > >>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> > >>  1 file changed, 51 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > >> new file mode 100644
> > >> index 0000000000000..21c8585c3bb2d
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > >> @@ -0,0 +1,51 @@
> > >> +# SPDX-License-Identifier: GPL-2.0-only
> > >> +%YAML 1.2
> > >> +---
> > >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> +
> > >> +title: TI TDP158 HDMI to TMDS Redriver
> > >> +
> > >> +maintainers:
> > >> +  - Arnaud Vrac <avrac@freebox.fr>
> > >> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    const: ti,tdp158
> > >> +
> > >> +  reg:
> > >> +    description: I2C address of the device
> > >> +
> > >> +  enable-gpios:
> > >> +    description: GPIO controlling bridge enable
> > >> +
> > >> +  vcc-supply:
> > >> +    description: Power supply 3.3V
> > >> +
> > >> +  vdd-supply:
> > >> +    description: Power supply 1.1V
> > >> +
> > >> +  ports:
> > >> +    $ref: /schemas/graph.yaml#/properties/ports
> > >> +
> > >> +    properties:
> > >> +      port@0:
> > >> +        $ref: /schemas/graph.yaml#/properties/port
> > >> +        description: Bridge input
> > >> +
> > >> +      port@1:
> > >> +        $ref: /schemas/graph.yaml#/properties/port
> > >> +        description: Bridge output
> > >> +
> > >> +    required:
> > >> +      - port@0
> > >> +      - port@1
> > > 
> > > The device supports DVI, HDMI or DP input, with various requirements and
> > > capabilities depending on the input. Your binding doesn't address that.
> > > 
> > > Similarly, it can do lane-swapping, so we should probably have a
> > > property to describe what mapping we want to use.
> > > 
> > > The i2c register access (and the whole behaviour of the device) is
> > > constrained on the I2C_EN pin status, and you can't read it from the
> > > device, so it's also something we need to have in the DT.
> > 
> > We are using the device in its default configuration.
> > (Power on via OE, then it works as expected)
> 
> I know, but that doesn't really matter for a binding.
> 
> > Can we leave any additional properties to be defined by whomever needs
> > them in the future?
> 
> If you can guarantee that doing so would be backward compatible, sure.
> But that means being able to answer those questions with a reasonable
> plan.

I think proposed bindings are generic enough to cover other possible
usecases in future.

-- 
With best wishes
Dmitry

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5140 bytes --]

On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote:
> On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
> > On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
> > > On 01/07/2024 15:50, Maxime Ripard wrote:
> > > 
> > > > On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> > > >
> > > >> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> > > >> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> > > >> It supports 4 TMDS channels, HPD, and a DDC interface.
> > > >> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> > > >> for power reduction. Several methods of power management are
> > > >> implemented to reduce overall power consumption.
> > > >> It supports fixed receiver EQ gain using I2C or pin strap to
> > > >> compensate for different lengths input cable or board traces.
> > > >>
> > > >> Features
> > > >>
> > > >> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> > > >> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> > > >> data rate, compatible with HDMI 2.0b electrical parameters
> > > >> - DisplayPort dual-mode standard version 1.1
> > > >> - Programmable fixed receiver equalizer up to 15.5dB
> > > >> - Global or independent high speed lane control, pre-emphasis
> > > >> and transmit swing, and slew rate control
> > > >> - I2C or pin strap programmable
> > > >> - Configurable as a DisplayPort redriver through I2C
> > > >> - Full lane swap on main lanes
> > > >> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> > > >>
> > > >> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> > > >>
> > > >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> > > >> ---
> > > >>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> > > >>  1 file changed, 51 insertions(+)
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > > >> new file mode 100644
> > > >> index 0000000000000..21c8585c3bb2d
> > > >> --- /dev/null
> > > >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > > >> @@ -0,0 +1,51 @@
> > > >> +# SPDX-License-Identifier: GPL-2.0-only
> > > >> +%YAML 1.2
> > > >> +---
> > > >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> > > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > >> +
> > > >> +title: TI TDP158 HDMI to TMDS Redriver
> > > >> +
> > > >> +maintainers:
> > > >> +  - Arnaud Vrac <avrac@freebox.fr>
> > > >> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> > > >> +
> > > >> +properties:
> > > >> +  compatible:
> > > >> +    const: ti,tdp158
> > > >> +
> > > >> +  reg:
> > > >> +    description: I2C address of the device
> > > >> +
> > > >> +  enable-gpios:
> > > >> +    description: GPIO controlling bridge enable
> > > >> +
> > > >> +  vcc-supply:
> > > >> +    description: Power supply 3.3V
> > > >> +
> > > >> +  vdd-supply:
> > > >> +    description: Power supply 1.1V
> > > >> +
> > > >> +  ports:
> > > >> +    $ref: /schemas/graph.yaml#/properties/ports
> > > >> +
> > > >> +    properties:
> > > >> +      port@0:
> > > >> +        $ref: /schemas/graph.yaml#/properties/port
> > > >> +        description: Bridge input
> > > >> +
> > > >> +      port@1:
> > > >> +        $ref: /schemas/graph.yaml#/properties/port
> > > >> +        description: Bridge output
> > > >> +
> > > >> +    required:
> > > >> +      - port@0
> > > >> +      - port@1
> > > > 
> > > > The device supports DVI, HDMI or DP input, with various requirements and
> > > > capabilities depending on the input. Your binding doesn't address that.
> > > > 
> > > > Similarly, it can do lane-swapping, so we should probably have a
> > > > property to describe what mapping we want to use.
> > > > 
> > > > The i2c register access (and the whole behaviour of the device) is
> > > > constrained on the I2C_EN pin status, and you can't read it from the
> > > > device, so it's also something we need to have in the DT.
> > > 
> > > We are using the device in its default configuration.
> > > (Power on via OE, then it works as expected)
> > 
> > I know, but that doesn't really matter for a binding.
> > 
> > > Can we leave any additional properties to be defined by whomever needs
> > > them in the future?
> > 
> > If you can guarantee that doing so would be backward compatible, sure.
> > But that means being able to answer those questions with a reasonable
> > plan.
> 
> I think proposed bindings are generic enough to cover other possible
> usecases in future.

I don't think it is. The current binding is for a I2C device that
shouldn't be accessed through I2C.

It's working for now because the driver doesn't do any access, so it's
all great, but as soon as we add support for the other case, then we'll
have to add a property that states that while it's an i2c device, it
shouldn't be accessed.

And adding such a property is a compatibility-breaking change.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Dmitry Baryshkov]

On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote:
> > On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
> > > On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
> > > > On 01/07/2024 15:50, Maxime Ripard wrote:
> > > >
> > > > > On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> > > > >
> > > > >> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> > > > >> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> > > > >> It supports 4 TMDS channels, HPD, and a DDC interface.
> > > > >> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> > > > >> for power reduction. Several methods of power management are
> > > > >> implemented to reduce overall power consumption.
> > > > >> It supports fixed receiver EQ gain using I2C or pin strap to
> > > > >> compensate for different lengths input cable or board traces.
> > > > >>
> > > > >> Features
> > > > >>
> > > > >> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> > > > >> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> > > > >> data rate, compatible with HDMI 2.0b electrical parameters
> > > > >> - DisplayPort dual-mode standard version 1.1
> > > > >> - Programmable fixed receiver equalizer up to 15.5dB
> > > > >> - Global or independent high speed lane control, pre-emphasis
> > > > >> and transmit swing, and slew rate control
> > > > >> - I2C or pin strap programmable
> > > > >> - Configurable as a DisplayPort redriver through I2C
> > > > >> - Full lane swap on main lanes
> > > > >> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> > > > >>
> > > > >> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> > > > >>
> > > > >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> > > > >> ---
> > > > >>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> > > > >>  1 file changed, 51 insertions(+)
> > > > >>
> > > > >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > > > >> new file mode 100644
> > > > >> index 0000000000000..21c8585c3bb2d
> > > > >> --- /dev/null
> > > > >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > > > >> @@ -0,0 +1,51 @@
> > > > >> +# SPDX-License-Identifier: GPL-2.0-only
> > > > >> +%YAML 1.2
> > > > >> +---
> > > > >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> > > > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > >> +
> > > > >> +title: TI TDP158 HDMI to TMDS Redriver
> > > > >> +
> > > > >> +maintainers:
> > > > >> +  - Arnaud Vrac <avrac@freebox.fr>
> > > > >> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> > > > >> +
> > > > >> +properties:
> > > > >> +  compatible:
> > > > >> +    const: ti,tdp158
> > > > >> +
> > > > >> +  reg:
> > > > >> +    description: I2C address of the device
> > > > >> +
> > > > >> +  enable-gpios:
> > > > >> +    description: GPIO controlling bridge enable
> > > > >> +
> > > > >> +  vcc-supply:
> > > > >> +    description: Power supply 3.3V
> > > > >> +
> > > > >> +  vdd-supply:
> > > > >> +    description: Power supply 1.1V
> > > > >> +
> > > > >> +  ports:
> > > > >> +    $ref: /schemas/graph.yaml#/properties/ports
> > > > >> +
> > > > >> +    properties:
> > > > >> +      port@0:
> > > > >> +        $ref: /schemas/graph.yaml#/properties/port
> > > > >> +        description: Bridge input
> > > > >> +
> > > > >> +      port@1:
> > > > >> +        $ref: /schemas/graph.yaml#/properties/port
> > > > >> +        description: Bridge output
> > > > >> +
> > > > >> +    required:
> > > > >> +      - port@0
> > > > >> +      - port@1
> > > > >
> > > > > The device supports DVI, HDMI or DP input, with various requirements and
> > > > > capabilities depending on the input. Your binding doesn't address that.
> > > > >
> > > > > Similarly, it can do lane-swapping, so we should probably have a
> > > > > property to describe what mapping we want to use.
> > > > >
> > > > > The i2c register access (and the whole behaviour of the device) is
> > > > > constrained on the I2C_EN pin status, and you can't read it from the
> > > > > device, so it's also something we need to have in the DT.
> > > >
> > > > We are using the device in its default configuration.
> > > > (Power on via OE, then it works as expected)
> > >
> > > I know, but that doesn't really matter for a binding.
> > >
> > > > Can we leave any additional properties to be defined by whomever needs
> > > > them in the future?
> > >
> > > If you can guarantee that doing so would be backward compatible, sure.
> > > But that means being able to answer those questions with a reasonable
> > > plan.
> >
> > I think proposed bindings are generic enough to cover other possible
> > usecases in future.
>
> I don't think it is. The current binding is for a I2C device that
> shouldn't be accessed through I2C.
>
> It's working for now because the driver doesn't do any access, so it's
> all great, but as soon as we add support for the other case, then we'll
> have to add a property that states that while it's an i2c device, it
> shouldn't be accessed.
>
> And adding such a property is a compatibility-breaking change.

Please correct me if I'm wrong. We have following usecases.

1. I2C_EN pulled low. TI158 is in the pin strap mode, it is not
connected to the I2C bus. A0, A1, SDA and SCL pins are used for
strapping the settings.
    board DT file should describe the bridge as a platform device
sitting directly under the root node.

2. I2C_EN pulled high. TI158 is in the I2C mode. It is connected to
the I2C bus, A0/A1 pins set the I2C bus address. The device is
controlled over the I2C bus

2.a. The same as 2, but the device is not controlled at all, default
settings are fine.

The driver covers usecase 2.a. The bindings allow extending the driver
to the usecase 2 (e.g. via optional properties which specify
bord-specific settings)

The usecase 1 is a completely separate topic, it requires a different
schema file, specifying no i2c address, only voltages supplies and
enable-gpios.



--
With best wishes
Dmitry

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7000 bytes --]

Hi,

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

DT maintainers have required that reg is always present in the other
sub-thread.

> 2. I2C_EN pulled high. TI158 is in the I2C mode. It is connected to
> the I2C bus, A0/A1 pins set the I2C bus address. The device is
> controlled over the I2C bus
> 
> 2.a. The same as 2, but the device is not controlled at all, default
> settings are fine.
> 
> The driver covers usecase 2.a. The bindings allow extending the driver
> to the usecase 2 (e.g. via optional properties which specify
> bord-specific settings)
> 
> The usecase 1 is a completely separate topic, it requires a different
> schema file, specifying no i2c address, only voltages supplies and
> enable-gpios.

I could have mis-unnderstood, but my understanding was that they were
running it with I2C_EN tied low.

Of course, that's one of the thing that is completely missing from the
commit log, so who knows.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Dmitry Baryshkov]

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

If I2C_EN is pulled low, there is no reg, as there is no i2c bus
whatsoever. I2C pins are repurposed as pin straps, An pins are
repurposed as pin straps.

I think DT maintainers wanted reg for the 2.a. case - in other words
the bridge is present on the I2C bus, but it is not being programmed.

>
> > 2. I2C_EN pulled high. TI158 is in the I2C mode. It is connected to
> > the I2C bus, A0/A1 pins set the I2C bus address. The device is
> > controlled over the I2C bus
> >
> > 2.a. The same as 2, but the device is not controlled at all, default
> > settings are fine.
> >
> > The driver covers usecase 2.a. The bindings allow extending the driver
> > to the usecase 2 (e.g. via optional properties which specify
> > bord-specific settings)
> >
> > The usecase 1 is a completely separate topic, it requires a different
> > schema file, specifying no i2c address, only voltages supplies and
> > enable-gpios.
>
> I could have mis-unnderstood, but my understanding was that they were
> running it with I2C_EN tied low.

That was my initial assumption, but I think Arnoud pointed out that
the bridge is connected to I2C, just not controlled as defaults are
sane.

> Of course, that's one of the thing that is completely missing from the
> commit log, so who knows.

Or from the cover letter :-(

-- 
With best wishes
Dmitry

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

[Marc Gonzalez]

On 27/06/2024 18:25, Conor Dooley wrote:

> On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
>
>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
>> It supports 4 TMDS channels, HPD, and a DDC interface.
>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
>> for power reduction. Several methods of power management are
>> implemented to reduce overall power consumption.
>> It supports fixed receiver EQ gain using I2C or pin strap to
>> compensate for different lengths input cable or board traces.
>>
>> Features
>>
>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
>> data rate, compatible with HDMI 2.0b electrical parameters
>> - DisplayPort dual-mode standard version 1.1
>> - Programmable fixed receiver equalizer up to 15.5dB
>> - Global or independent high speed lane control, pre-emphasis
>> and transmit swing, and slew rate control
>> - I2C or pin strap programmable
>> - Configurable as a DisplayPort redriver through I2C
>> - Full lane swap on main lanes
>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
>>
>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
>>
>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>> ---
>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> new file mode 100644
>> index 0000000000000..21c8585c3bb2d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: TI TDP158 HDMI to TMDS Redriver
>> +
>> +maintainers:
>> +  - Arnaud Vrac <avrac@freebox.fr>
>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,tdp158
>> +
>> +  reg:
>> +    description: I2C address of the device
> 
> Is reg not required? How do you communicate with the device if the i2c
> bus is not connected? Is the enable GPIO enough to operate it in some
> situations?
> 
> Otherwise this looks good to me, but given Maxime commented about the
> complexity of the device, I'm probably out of my depth!

Hello Conor,

A cycle has been detected:
Above, you defer to Maxime.
Yet later, he wrote:
"DT maintainers have required that reg is always present"


I propose we NOT mark the "reg" property as required.
(Thus, keep the binding as proposed in v3.)

Rationale:

- The device can be statically configured by pin straps,
in which case it is NOT connected to an I2C bus.

- Even if the device IS connected to an I2C bus,
no I2C accesses are required if the default configuration
meets the ODM's needs.

Is that OK with you? Can I get your Amen?

Regards

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3939 bytes --]

On Tue, Jul 23, 2024 at 05:17:12PM +0200, Marc Gonzalez wrote:
> On 27/06/2024 18:25, Conor Dooley wrote:
> 
> > On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
> >
> >> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> >> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> >> It supports 4 TMDS channels, HPD, and a DDC interface.
> >> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> >> for power reduction. Several methods of power management are
> >> implemented to reduce overall power consumption.
> >> It supports fixed receiver EQ gain using I2C or pin strap to
> >> compensate for different lengths input cable or board traces.
> >>
> >> Features
> >>
> >> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> >> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> >> data rate, compatible with HDMI 2.0b electrical parameters
> >> - DisplayPort dual-mode standard version 1.1
> >> - Programmable fixed receiver equalizer up to 15.5dB
> >> - Global or independent high speed lane control, pre-emphasis
> >> and transmit swing, and slew rate control
> >> - I2C or pin strap programmable
> >> - Configurable as a DisplayPort redriver through I2C
> >> - Full lane swap on main lanes
> >> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> >>
> >> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> >>
> >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> >> ---
> >>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >> new file mode 100644
> >> index 0000000000000..21c8585c3bb2d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >> @@ -0,0 +1,51 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: TI TDP158 HDMI to TMDS Redriver
> >> +
> >> +maintainers:
> >> +  - Arnaud Vrac <avrac@freebox.fr>
> >> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: ti,tdp158
> >> +
> >> +  reg:
> >> +    description: I2C address of the device
> > 
> > Is reg not required? How do you communicate with the device if the i2c
> > bus is not connected? Is the enable GPIO enough to operate it in some
> > situations?
> > 
> > Otherwise this looks good to me, but given Maxime commented about the
> > complexity of the device, I'm probably out of my depth!
> 
> Hello Conor,
> 
> A cycle has been detected:
> Above, you defer to Maxime.
> Yet later, he wrote:
> "DT maintainers have required that reg is always present"

I think he was actually talking about Krzysztof.

> I propose we NOT mark the "reg" property as required.
> (Thus, keep the binding as proposed in v3.)
> 
> Rationale:
> 
> - The device can be statically configured by pin straps,
> in which case it is NOT connected to an I2C bus.

I'm inclined to say that, because connecting the i2c bus is not required
at all for the device to be usable in some circumstances that we should
not require reg. Someone could, in theory, use the device with a SoC
without an i2c controller, right?

> - Even if the device IS connected to an I2C bus,
> no I2C accesses are required if the default configuration
> meets the ODM's needs.

In this case, I think a reg property is required actually, because it is
on the bus, and devices on an i2c bus must have a reg property. That
aside, even if the ODM doesn't want to change the defaults, the owner
might.

> Is that OK with you? Can I get your Amen?

Sure.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4523 bytes --]

On Tue, Jul 23, 2024 at 08:57:03PM GMT, Conor Dooley wrote:
> On Tue, Jul 23, 2024 at 05:17:12PM +0200, Marc Gonzalez wrote:
> > On 27/06/2024 18:25, Conor Dooley wrote:
> > 
> > > On Thu, Jun 27, 2024 at 01:13:03PM +0200, Marc Gonzalez wrote:
> > >
> > >> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> > >> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> > >> It supports 4 TMDS channels, HPD, and a DDC interface.
> > >> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> > >> for power reduction. Several methods of power management are
> > >> implemented to reduce overall power consumption.
> > >> It supports fixed receiver EQ gain using I2C or pin strap to
> > >> compensate for different lengths input cable or board traces.
> > >>
> > >> Features
> > >>
> > >> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> > >> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> > >> data rate, compatible with HDMI 2.0b electrical parameters
> > >> - DisplayPort dual-mode standard version 1.1
> > >> - Programmable fixed receiver equalizer up to 15.5dB
> > >> - Global or independent high speed lane control, pre-emphasis
> > >> and transmit swing, and slew rate control
> > >> - I2C or pin strap programmable
> > >> - Configurable as a DisplayPort redriver through I2C
> > >> - Full lane swap on main lanes
> > >> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> > >>
> > >> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> > >>
> > >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> > >> ---
> > >>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> > >>  1 file changed, 51 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > >> new file mode 100644
> > >> index 0000000000000..21c8585c3bb2d
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > >> @@ -0,0 +1,51 @@
> > >> +# SPDX-License-Identifier: GPL-2.0-only
> > >> +%YAML 1.2
> > >> +---
> > >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> +
> > >> +title: TI TDP158 HDMI to TMDS Redriver
> > >> +
> > >> +maintainers:
> > >> +  - Arnaud Vrac <avrac@freebox.fr>
> > >> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    const: ti,tdp158
> > >> +
> > >> +  reg:
> > >> +    description: I2C address of the device
> > > 
> > > Is reg not required? How do you communicate with the device if the i2c
> > > bus is not connected? Is the enable GPIO enough to operate it in some
> > > situations?
> > > 
> > > Otherwise this looks good to me, but given Maxime commented about the
> > > complexity of the device, I'm probably out of my depth!
> > 
> > Hello Conor,
> > 
> > A cycle has been detected:
> > Above, you defer to Maxime.
> > Yet later, he wrote:
> > "DT maintainers have required that reg is always present"
> 
> I think he was actually talking about Krzysztof.

I was.

> > I propose we NOT mark the "reg" property as required.
> > (Thus, keep the binding as proposed in v3.)
> > 
> > Rationale:
> > 
> > - The device can be statically configured by pin straps,
> > in which case it is NOT connected to an I2C bus.
> 
> I'm inclined to say that, because connecting the i2c bus is not required
> at all for the device to be usable in some circumstances that we should
> not require reg. Someone could, in theory, use the device with a SoC
> without an i2c controller, right?
> 
> > - Even if the device IS connected to an I2C bus,
> > no I2C accesses are required if the default configuration
> > meets the ODM's needs.
> 
> In this case, I think a reg property is required actually, because it is
> on the bus, and devices on an i2c bus must have a reg property. That
> aside, even if the ODM doesn't want to change the defaults, the owner
> might.
> 
> > Is that OK with you? Can I get your Amen?
> 
> Sure.

We still have an entire sub-thread to this conversation that has been
completely ignored by Marc. Upstreaming is process where both sides need
to be involved, and I'm not seeing that so far.

So, yeah, until that other sub-thread is somewhat resolved, I don't see
how we can vet those bindings.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Marc Gonzalez]

On 15/07/2024 16:42, Maxime Ripard wrote:
> On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote:
>> On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
>>> On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
>>>> On 01/07/2024 15:50, Maxime Ripard wrote:
>>>>
>>>>> On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
>>>>>
>>>>>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
>>>>>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
>>>>>> It supports 4 TMDS channels, HPD, and a DDC interface.
>>>>>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
>>>>>> for power reduction. Several methods of power management are
>>>>>> implemented to reduce overall power consumption.
>>>>>> It supports fixed receiver EQ gain using I2C or pin strap to
>>>>>> compensate for different lengths input cable or board traces.
>>>>>>
>>>>>> Features
>>>>>>
>>>>>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
>>>>>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
>>>>>> data rate, compatible with HDMI 2.0b electrical parameters
>>>>>> - DisplayPort dual-mode standard version 1.1
>>>>>> - Programmable fixed receiver equalizer up to 15.5dB
>>>>>> - Global or independent high speed lane control, pre-emphasis
>>>>>> and transmit swing, and slew rate control
>>>>>> - I2C or pin strap programmable
>>>>>> - Configurable as a DisplayPort redriver through I2C
>>>>>> - Full lane swap on main lanes
>>>>>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
>>>>>>
>>>>>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
>>>>>>
>>>>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>>>>>> ---
>>>>>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>>>>>>  1 file changed, 51 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>>>>> new file mode 100644
>>>>>> index 0000000000000..21c8585c3bb2d
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>>>>> @@ -0,0 +1,51 @@
>>>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: TI TDP158 HDMI to TMDS Redriver
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Arnaud Vrac <avrac@freebox.fr>
>>>>>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: ti,tdp158
>>>>>> +
>>>>>> +  reg:
>>>>>> +    description: I2C address of the device
>>>>>> +
>>>>>> +  enable-gpios:
>>>>>> +    description: GPIO controlling bridge enable
>>>>>> +
>>>>>> +  vcc-supply:
>>>>>> +    description: Power supply 3.3V
>>>>>> +
>>>>>> +  vdd-supply:
>>>>>> +    description: Power supply 1.1V
>>>>>> +
>>>>>> +  ports:
>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>> +
>>>>>> +    properties:
>>>>>> +      port@0:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: Bridge input
>>>>>> +
>>>>>> +      port@1:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: Bridge output
>>>>>> +
>>>>>> +    required:
>>>>>> +      - port@0
>>>>>> +      - port@1
>>>>>
>>>>> The device supports DVI, HDMI or DP input, with various requirements and
>>>>> capabilities depending on the input. Your binding doesn't address that.
>>>>>
>>>>> Similarly, it can do lane-swapping, so we should probably have a
>>>>> property to describe what mapping we want to use.
>>>>>
>>>>> The i2c register access (and the whole behaviour of the device) is
>>>>> constrained on the I2C_EN pin status, and you can't read it from the
>>>>> device, so it's also something we need to have in the DT.
>>>>
>>>> We are using the device in its default configuration.
>>>> (Power on via OE, then it works as expected)
>>>
>>> I know, but that doesn't really matter for a binding.
>>>
>>>> Can we leave any additional properties to be defined by whomever needs
>>>> them in the future?
>>>
>>> If you can guarantee that doing so would be backward compatible, sure.
>>> But that means being able to answer those questions with a reasonable
>>> plan.
>>
>> I think proposed bindings are generic enough to cover other possible
>> usecases in future.
> 
> I don't think it is. The current binding is for a I2C device that
> shouldn't be accessed through I2C.
> 
> It's working for now because the driver doesn't do any access, so it's
> all great, but as soon as we add support for the other case, then we'll
> have to add a property that states that while it's an i2c device, it
> shouldn't be accessed.
> 
> And adding such a property is a compatibility-breaking change.

Why do you say:
"current binding is for a I2C device that
shouldn't be accessed through I2C" ?

As a matter of fact, my debug code queries the device ID using
regmap_read() to make sure I set the correct I2C address.

It's not that the device "SHOULD NOT" be accessed.

It's just that the driver DOES NOT NEED TO access the device,
simply because the default settings work fine.

Regards

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

[Marc Gonzalez]

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

On our board, the device is sitting on I2C bus blsp2_i2c1.
Therefore, I2C_EN is hard-wired to HIGH.
(As it must be for I2C to function.)

&blsp2_i2c1 {
	status = "okay";

	tdp158@5e {
		compatible = "ti,tdp158";
		reg = <0x5e>;
		enable-gpios = <&tlmm 17 GPIO_ACTIVE_HIGH>;
		pinctrl-0 = <&hdmi_en>;
		pinctrl-names = "default";
		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				reg = <0>;

				tdp158_in: endpoint {
					remote-endpoint = <&hdmi_out>;
				};
			};

			port@1 {
				reg = <1>;

				tdp158_out: endpoint {
					remote-endpoint = <&hdmi_con>;
				};
			};
		};
	};
};

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

[Dmitry Baryshkov]

On Wed, Jul 24, 2024 at 07:18:39PM GMT, Marc Gonzalez wrote:
> On 15/07/2024 16:42, Maxime Ripard wrote:
> > On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote:
> >> On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
> >>> On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
> >>>> On 01/07/2024 15:50, Maxime Ripard wrote:
> >>>>
> >>>>> On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> >>>>>
> >>>>>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> >>>>>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> >>>>>> It supports 4 TMDS channels, HPD, and a DDC interface.
> >>>>>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> >>>>>> for power reduction. Several methods of power management are
> >>>>>> implemented to reduce overall power consumption.
> >>>>>> It supports fixed receiver EQ gain using I2C or pin strap to
> >>>>>> compensate for different lengths input cable or board traces.
> >>>>>>
> >>>>>> Features
> >>>>>>
> >>>>>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> >>>>>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> >>>>>> data rate, compatible with HDMI 2.0b electrical parameters
> >>>>>> - DisplayPort dual-mode standard version 1.1
> >>>>>> - Programmable fixed receiver equalizer up to 15.5dB
> >>>>>> - Global or independent high speed lane control, pre-emphasis
> >>>>>> and transmit swing, and slew rate control
> >>>>>> - I2C or pin strap programmable
> >>>>>> - Configurable as a DisplayPort redriver through I2C
> >>>>>> - Full lane swap on main lanes
> >>>>>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> >>>>>>
> >>>>>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> >>>>>>
> >>>>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> >>>>>> ---
> >>>>>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> >>>>>>  1 file changed, 51 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >>>>>> new file mode 100644
> >>>>>> index 0000000000000..21c8585c3bb2d
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >>>>>> @@ -0,0 +1,51 @@
> >>>>>> +# SPDX-License-Identifier: GPL-2.0-only
> >>>>>> +%YAML 1.2
> >>>>>> +---
> >>>>>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>> +
> >>>>>> +title: TI TDP158 HDMI to TMDS Redriver
> >>>>>> +
> >>>>>> +maintainers:
> >>>>>> +  - Arnaud Vrac <avrac@freebox.fr>
> >>>>>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> >>>>>> +
> >>>>>> +properties:
> >>>>>> +  compatible:
> >>>>>> +    const: ti,tdp158
> >>>>>> +
> >>>>>> +  reg:
> >>>>>> +    description: I2C address of the device
> >>>>>> +
> >>>>>> +  enable-gpios:
> >>>>>> +    description: GPIO controlling bridge enable
> >>>>>> +
> >>>>>> +  vcc-supply:
> >>>>>> +    description: Power supply 3.3V
> >>>>>> +
> >>>>>> +  vdd-supply:
> >>>>>> +    description: Power supply 1.1V
> >>>>>> +
> >>>>>> +  ports:
> >>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>>> +
> >>>>>> +    properties:
> >>>>>> +      port@0:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: Bridge input
> >>>>>> +
> >>>>>> +      port@1:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: Bridge output
> >>>>>> +
> >>>>>> +    required:
> >>>>>> +      - port@0
> >>>>>> +      - port@1
> >>>>>
> >>>>> The device supports DVI, HDMI or DP input, with various requirements and
> >>>>> capabilities depending on the input. Your binding doesn't address that.
> >>>>>
> >>>>> Similarly, it can do lane-swapping, so we should probably have a
> >>>>> property to describe what mapping we want to use.
> >>>>>
> >>>>> The i2c register access (and the whole behaviour of the device) is
> >>>>> constrained on the I2C_EN pin status, and you can't read it from the
> >>>>> device, so it's also something we need to have in the DT.
> >>>>
> >>>> We are using the device in its default configuration.
> >>>> (Power on via OE, then it works as expected)
> >>>
> >>> I know, but that doesn't really matter for a binding.
> >>>
> >>>> Can we leave any additional properties to be defined by whomever needs
> >>>> them in the future?
> >>>
> >>> If you can guarantee that doing so would be backward compatible, sure.
> >>> But that means being able to answer those questions with a reasonable
> >>> plan.
> >>
> >> I think proposed bindings are generic enough to cover other possible
> >> usecases in future.
> > 
> > I don't think it is. The current binding is for a I2C device that
> > shouldn't be accessed through I2C.
> > 
> > It's working for now because the driver doesn't do any access, so it's
> > all great, but as soon as we add support for the other case, then we'll
> > have to add a property that states that while it's an i2c device, it
> > shouldn't be accessed.
> > 
> > And adding such a property is a compatibility-breaking change.
> 
> Why do you say:
> "current binding is for a I2C device that
> shouldn't be accessed through I2C" ?
> 
> As a matter of fact, my debug code queries the device ID using
> regmap_read() to make sure I set the correct I2C address.

Please note: bingdings do not cover the driver at all. The driver might
do whatever it wants. The bindings describe the hardware.

> 
> It's not that the device "SHOULD NOT" be accessed.
> 
> It's just that the driver DOES NOT NEED TO access the device,
> simply because the default settings work fine.

-- 
With best wishes
Dmitry

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

[Marc Gonzalez]

On 24/07/2024 19:25, Dmitry Baryshkov wrote:
> On Wed, Jul 24, 2024 at 07:18:39PM GMT, Marc Gonzalez wrote:
>> On 15/07/2024 16:42, Maxime Ripard wrote:
>>> On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote:
>>>> On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
>>>>> On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
>>>>>> On 01/07/2024 15:50, Maxime Ripard wrote:
>>>>>>
>>>>>>> On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
>>>>>>>
>>>>>>>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
>>>>>>>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
>>>>>>>> It supports 4 TMDS channels, HPD, and a DDC interface.
>>>>>>>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
>>>>>>>> for power reduction. Several methods of power management are
>>>>>>>> implemented to reduce overall power consumption.
>>>>>>>> It supports fixed receiver EQ gain using I2C or pin strap to
>>>>>>>> compensate for different lengths input cable or board traces.
>>>>>>>>
>>>>>>>> Features
>>>>>>>>
>>>>>>>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
>>>>>>>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
>>>>>>>> data rate, compatible with HDMI 2.0b electrical parameters
>>>>>>>> - DisplayPort dual-mode standard version 1.1
>>>>>>>> - Programmable fixed receiver equalizer up to 15.5dB
>>>>>>>> - Global or independent high speed lane control, pre-emphasis
>>>>>>>> and transmit swing, and slew rate control
>>>>>>>> - I2C or pin strap programmable
>>>>>>>> - Configurable as a DisplayPort redriver through I2C
>>>>>>>> - Full lane swap on main lanes
>>>>>>>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
>>>>>>>>
>>>>>>>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
>>>>>>>>
>>>>>>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>>>>>>>> ---
>>>>>>>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
>>>>>>>>  1 file changed, 51 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000000..21c8585c3bb2d
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>>>>>>>> @@ -0,0 +1,51 @@
>>>>>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>>>>>> +%YAML 1.2
>>>>>>>> +---
>>>>>>>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>> +
>>>>>>>> +title: TI TDP158 HDMI to TMDS Redriver
>>>>>>>> +
>>>>>>>> +maintainers:
>>>>>>>> +  - Arnaud Vrac <avrac@freebox.fr>
>>>>>>>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> +  compatible:
>>>>>>>> +    const: ti,tdp158
>>>>>>>> +
>>>>>>>> +  reg:
>>>>>>>> +    description: I2C address of the device
>>>>>>>> +
>>>>>>>> +  enable-gpios:
>>>>>>>> +    description: GPIO controlling bridge enable
>>>>>>>> +
>>>>>>>> +  vcc-supply:
>>>>>>>> +    description: Power supply 3.3V
>>>>>>>> +
>>>>>>>> +  vdd-supply:
>>>>>>>> +    description: Power supply 1.1V
>>>>>>>> +
>>>>>>>> +  ports:
>>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      port@0:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: Bridge input
>>>>>>>> +
>>>>>>>> +      port@1:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: Bridge output
>>>>>>>> +
>>>>>>>> +    required:
>>>>>>>> +      - port@0
>>>>>>>> +      - port@1
>>>>>>>
>>>>>>> The device supports DVI, HDMI or DP input, with various requirements and
>>>>>>> capabilities depending on the input. Your binding doesn't address that.
>>>>>>>
>>>>>>> Similarly, it can do lane-swapping, so we should probably have a
>>>>>>> property to describe what mapping we want to use.
>>>>>>>
>>>>>>> The i2c register access (and the whole behaviour of the device) is
>>>>>>> constrained on the I2C_EN pin status, and you can't read it from the
>>>>>>> device, so it's also something we need to have in the DT.
>>>>>>
>>>>>> We are using the device in its default configuration.
>>>>>> (Power on via OE, then it works as expected)
>>>>>
>>>>> I know, but that doesn't really matter for a binding.
>>>>>
>>>>>> Can we leave any additional properties to be defined by whomever needs
>>>>>> them in the future?
>>>>>
>>>>> If you can guarantee that doing so would be backward compatible, sure.
>>>>> But that means being able to answer those questions with a reasonable
>>>>> plan.
>>>>
>>>> I think proposed bindings are generic enough to cover other possible
>>>> usecases in future.
>>>
>>> I don't think it is. The current binding is for a I2C device that
>>> shouldn't be accessed through I2C.
>>>
>>> It's working for now because the driver doesn't do any access, so it's
>>> all great, but as soon as we add support for the other case, then we'll
>>> have to add a property that states that while it's an i2c device, it
>>> shouldn't be accessed.
>>>
>>> And adding such a property is a compatibility-breaking change.
>>
>> Why do you say:
>> "current binding is for a I2C device that
>> shouldn't be accessed through I2C" ?
>>
>> As a matter of fact, my debug code queries the device ID using
>> regmap_read() to make sure I set the correct I2C address.
> 
> Please note: bingdings do not cover the driver at all. The driver might
> do whatever it wants. The bindings describe the hardware.

OK.
How does the proposed binding say
"I2C device shouldn't be accessed through I2C" ?

The tfp410 has the same property that it can be configured
either through pin straps or via I2C.

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

[Marc Gonzalez]

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

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

[Marc Gonzalez]

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

Re: [PATCH v3 0/2] Basic support for TI TDP158

[Marc Gonzalez]

On 27/06/2024 13:13, Marc Gonzalez wrote:

> Changes in v3:
> - Add 'select DRM_PANEL_BRIDGE' in driver Kconfig
> - Fix checkpatch errors
> - log errors using dev_err (so save dev pointer)
> - expand a few error messages
> - expand commit messages with info from the datasheet
> - mark regulators as required in the DT binding
> - Link to v2: https://lore.kernel.org/r/20240625-tdp158-v2-0-a3b344707fa7@freebox.fr

Series has been rebased on top of v6.11-rc1

Current changelog is:

Changes in v4:
- Add blurb about I2C vs pin strap mode in cover letter
- Add blurb about I2C vs pin strap mode in binding commit message
- Add blurb about I2C mode in driver commit message
- Add comment in binding explaining when reg is required
- Add comment in binding describing Operation Enable / Reset Pin
- Link to v3: https://lore.kernel.org/r/20240627-tdp158-v3-0-fb2fbc808346@freebox.fr


For reference, binding commit message blurb:

    Like the TFP410, the TDP158 can be set up in 2 different ways:
    1) hard-coding its configuration settings using pin-strapping resistors
    2) placing it on an I2C bus, and defer set-up until run-time
    
    The mode is selected via pin 8 = I2C_EN
    I2C_EN high = I2C Control Mode
    I2C_EN low  = Pin Strap Mode
    
    On our board, I2C_EN is pulled high.


driver commit message blurb:

    On our board, I2C_EN is pulled high.
    Thus, this code defines a module_i2c_driver.
    
    The default settings work fine for our use-case.
    So this basic driver doesn't need to tweak any I2C registers.


binding comments:

+# The reg property is required if and only if the device is connected
+# to an I2C bus. In pin strap mode, reg must not be specified.
+  reg:
+    description: I2C address of the device
+
+# Pin 36 = Operation Enable / Reset Pin
+# OE = L: Power Down Mode
+# OE = H: Normal Operation
+# Internal weak pullup: device resets on H to L transitions
+  enable-gpios:
+    description: GPIO controlling bridge enable


I plan to send a formal v4 in a few hours.

Regards

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3347 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6320 bytes --]

On Wed, Jul 24, 2024 at 07:28:41PM GMT, Marc Gonzalez wrote:
> On 24/07/2024 19:25, Dmitry Baryshkov wrote:
> > On Wed, Jul 24, 2024 at 07:18:39PM GMT, Marc Gonzalez wrote:
> >> On 15/07/2024 16:42, Maxime Ripard wrote:
> >>> On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote:
> >>>> On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
> >>>>> On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
> >>>>>> On 01/07/2024 15:50, Maxime Ripard wrote:
> >>>>>>
> >>>>>>> On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> >>>>>>>
> >>>>>>>> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> >>>>>>>> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> >>>>>>>> It supports 4 TMDS channels, HPD, and a DDC interface.
> >>>>>>>> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> >>>>>>>> for power reduction. Several methods of power management are
> >>>>>>>> implemented to reduce overall power consumption.
> >>>>>>>> It supports fixed receiver EQ gain using I2C or pin strap to
> >>>>>>>> compensate for different lengths input cable or board traces.
> >>>>>>>>
> >>>>>>>> Features
> >>>>>>>>
> >>>>>>>> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> >>>>>>>> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> >>>>>>>> data rate, compatible with HDMI 2.0b electrical parameters
> >>>>>>>> - DisplayPort dual-mode standard version 1.1
> >>>>>>>> - Programmable fixed receiver equalizer up to 15.5dB
> >>>>>>>> - Global or independent high speed lane control, pre-emphasis
> >>>>>>>> and transmit swing, and slew rate control
> >>>>>>>> - I2C or pin strap programmable
> >>>>>>>> - Configurable as a DisplayPort redriver through I2C
> >>>>>>>> - Full lane swap on main lanes
> >>>>>>>> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> >>>>>>>>
> >>>>>>>> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> >>>>>>>>
> >>>>>>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> >>>>>>>> ---
> >>>>>>>>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> >>>>>>>>  1 file changed, 51 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >>>>>>>> new file mode 100644
> >>>>>>>> index 0000000000000..21c8585c3bb2d
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >>>>>>>> @@ -0,0 +1,51 @@
> >>>>>>>> +# SPDX-License-Identifier: GPL-2.0-only
> >>>>>>>> +%YAML 1.2
> >>>>>>>> +---
> >>>>>>>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> >>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>>> +
> >>>>>>>> +title: TI TDP158 HDMI to TMDS Redriver
> >>>>>>>> +
> >>>>>>>> +maintainers:
> >>>>>>>> +  - Arnaud Vrac <avrac@freebox.fr>
> >>>>>>>> +  - Pierre-Hugues Husson <phhusson@freebox.fr>
> >>>>>>>> +
> >>>>>>>> +properties:
> >>>>>>>> +  compatible:
> >>>>>>>> +    const: ti,tdp158
> >>>>>>>> +
> >>>>>>>> +  reg:
> >>>>>>>> +    description: I2C address of the device
> >>>>>>>> +
> >>>>>>>> +  enable-gpios:
> >>>>>>>> +    description: GPIO controlling bridge enable
> >>>>>>>> +
> >>>>>>>> +  vcc-supply:
> >>>>>>>> +    description: Power supply 3.3V
> >>>>>>>> +
> >>>>>>>> +  vdd-supply:
> >>>>>>>> +    description: Power supply 1.1V
> >>>>>>>> +
> >>>>>>>> +  ports:
> >>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>>>>> +
> >>>>>>>> +    properties:
> >>>>>>>> +      port@0:
> >>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>>>> +        description: Bridge input
> >>>>>>>> +
> >>>>>>>> +      port@1:
> >>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>>>> +        description: Bridge output
> >>>>>>>> +
> >>>>>>>> +    required:
> >>>>>>>> +      - port@0
> >>>>>>>> +      - port@1
> >>>>>>>
> >>>>>>> The device supports DVI, HDMI or DP input, with various requirements and
> >>>>>>> capabilities depending on the input. Your binding doesn't address that.
> >>>>>>>
> >>>>>>> Similarly, it can do lane-swapping, so we should probably have a
> >>>>>>> property to describe what mapping we want to use.
> >>>>>>>
> >>>>>>> The i2c register access (and the whole behaviour of the device) is
> >>>>>>> constrained on the I2C_EN pin status, and you can't read it from the
> >>>>>>> device, so it's also something we need to have in the DT.
> >>>>>>
> >>>>>> We are using the device in its default configuration.
> >>>>>> (Power on via OE, then it works as expected)
> >>>>>
> >>>>> I know, but that doesn't really matter for a binding.
> >>>>>
> >>>>>> Can we leave any additional properties to be defined by whomever needs
> >>>>>> them in the future?
> >>>>>
> >>>>> If you can guarantee that doing so would be backward compatible, sure.
> >>>>> But that means being able to answer those questions with a reasonable
> >>>>> plan.
> >>>>
> >>>> I think proposed bindings are generic enough to cover other possible
> >>>> usecases in future.
> >>>
> >>> I don't think it is. The current binding is for a I2C device that
> >>> shouldn't be accessed through I2C.
> >>>
> >>> It's working for now because the driver doesn't do any access, so it's
> >>> all great, but as soon as we add support for the other case, then we'll
> >>> have to add a property that states that while it's an i2c device, it
> >>> shouldn't be accessed.
> >>>
> >>> And adding such a property is a compatibility-breaking change.
> >>
> >> Why do you say:
> >> "current binding is for a I2C device that
> >> shouldn't be accessed through I2C" ?
> >>
> >> As a matter of fact, my debug code queries the device ID using
> >> regmap_read() to make sure I set the correct I2C address.
> > 
> > Please note: bingdings do not cover the driver at all. The driver might
> > do whatever it wants. The bindings describe the hardware.
> 
> OK.
> How does the proposed binding say
> "I2C device shouldn't be accessed through I2C" ?

It doesn't, but then again, neither your commit log or cover letter say
"it can be accessed by i2c" either, so we went on the wrong track.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Dmitry Baryshkov]

On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote:
> > On 15/07/2024 16:40, Maxime Ripard wrote:
> > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote:
> > >> On 01/07/2024 15:50, Maxime Ripard wrote:
> > >>
> > >>> The i2c register access (and the whole behaviour of the device) is
> > >>> constrained on the I2C_EN pin status, and you can't read it from the
> > >>> device, so it's also something we need to have in the DT.
> > >>
> > >> I think the purpose of the I2C_EN pin might have been misunderstood.
> > >>
> > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet.
> > >
> > > Toggled, probably not. Connected to a GPIO and the kernel has to assert
> > > a level at boot, I've seen worse hardware design already.
> > >
> > >> I2C_EN is a layout-time setting, decided by a board manufacturer:
> > >>
> > >> - If the TDP158 is fully configured once-and-for-all at layout-time,
> > >> then no I2C bus is required, and I2C_EN is pulled down forever.
> > >>
> > >> - If the board manufacturer wants to keep open the possibility
> > >> to adjust some parameters at run-time, then they must connect
> > >> the device to an I2C bus, and I2C_EN is pulled up forever.
> > >
> > > How do you express both cases in your current binding?
> >
> > It's not that I'm ignoring your question.
> >
> > It's that I don't understand what you're asking.
>
> And that's fine, you just need to say so.
>
> Generally speaking, you're focusing on the driver. The driver is not the
> issue here. You can do whatever you want in the driver for all I care,
> we can change that later on as we wish.
>
> The binding however cannot change, so it *has* to ideally cover all
> possible situations the hardware can be used in, or at a minimum leave
> the door open to support those without a compatibility breakage.
>
> That's why I've been asking those questions, because so far the only
> thing you've claimed is that "I can't test the driver for anything
> else", but, again, whether there's a driver or not, or if it's
> functional, is completely missing the point.
>
> > SITUATION 1
> > tdp158 is pin strapped.
> > Device node is child of root node.
> > Properties in proposed binding are valid (regulators and power-on pin)
> > Can be supported via module_platform_driver.
> >
> > SITUATION 2
> > tdp158 is sitting on I2C bus.
> > Device node is child of i2c bus node.
> > (robh said missing reg prop would be flagged by the compiler)
> > Properties in proposed binding are valid (regulators and power-on pin)
> > Supported via module_i2c_driver.
> >
> > If some settings-specific properties are added later, like skew,
> > they would only be valid for the I2C programmable mode, obviously.
>
> I think there's a couple more combinations:
>
>   - The device is connected on an I2C bus, but I2C_EN is tied low

No, this is not possible. I2C pins are repurposed if I2C_EN is low.
You can not call that an i2c bus anymore.

>   - The device is connected on an I2C bus, but I2C_EN is connected to a
>     GPIO and the kernel needs to assert its state at boot.

This is a pretty strange configuration.  The I2C_EN pin isn't supposed
to be toggled dynamically. Anyway, if that happens, I'd use pinctrl /
hog to control the pin.

>
> The GPIO case can be easily dealt with later on using an optional GPIO
> in the binding, but the current binding infers the I2C_EN level from the
> bus it's connected to, and I think we don't have a good way to deal with
> cases that would break that assumption.
>
> So I think we need an extra property to report the state of the i2c_en
> pin (and would be mutually exclusive with the GPIO if we ever have to
> support it).

-- 
With best wishes
Dmitry

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3765 bytes --]

On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote:
> On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote:
> > > On 15/07/2024 16:40, Maxime Ripard wrote:
> > > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote:
> > > >> On 01/07/2024 15:50, Maxime Ripard wrote:
> > > >>
> > > >>> The i2c register access (and the whole behaviour of the device) is
> > > >>> constrained on the I2C_EN pin status, and you can't read it from the
> > > >>> device, so it's also something we need to have in the DT.
> > > >>
> > > >> I think the purpose of the I2C_EN pin might have been misunderstood.
> > > >>
> > > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet.
> > > >
> > > > Toggled, probably not. Connected to a GPIO and the kernel has to assert
> > > > a level at boot, I've seen worse hardware design already.
> > > >
> > > >> I2C_EN is a layout-time setting, decided by a board manufacturer:
> > > >>
> > > >> - If the TDP158 is fully configured once-and-for-all at layout-time,
> > > >> then no I2C bus is required, and I2C_EN is pulled down forever.
> > > >>
> > > >> - If the board manufacturer wants to keep open the possibility
> > > >> to adjust some parameters at run-time, then they must connect
> > > >> the device to an I2C bus, and I2C_EN is pulled up forever.
> > > >
> > > > How do you express both cases in your current binding?
> > >
> > > It's not that I'm ignoring your question.
> > >
> > > It's that I don't understand what you're asking.
> >
> > And that's fine, you just need to say so.
> >
> > Generally speaking, you're focusing on the driver. The driver is not the
> > issue here. You can do whatever you want in the driver for all I care,
> > we can change that later on as we wish.
> >
> > The binding however cannot change, so it *has* to ideally cover all
> > possible situations the hardware can be used in, or at a minimum leave
> > the door open to support those without a compatibility breakage.
> >
> > That's why I've been asking those questions, because so far the only
> > thing you've claimed is that "I can't test the driver for anything
> > else", but, again, whether there's a driver or not, or if it's
> > functional, is completely missing the point.
> >
> > > SITUATION 1
> > > tdp158 is pin strapped.
> > > Device node is child of root node.
> > > Properties in proposed binding are valid (regulators and power-on pin)
> > > Can be supported via module_platform_driver.
> > >
> > > SITUATION 2
> > > tdp158 is sitting on I2C bus.
> > > Device node is child of i2c bus node.
> > > (robh said missing reg prop would be flagged by the compiler)
> > > Properties in proposed binding are valid (regulators and power-on pin)
> > > Supported via module_i2c_driver.
> > >
> > > If some settings-specific properties are added later, like skew,
> > > they would only be valid for the I2C programmable mode, obviously.
> >
> > I think there's a couple more combinations:
> >
> >   - The device is connected on an I2C bus, but I2C_EN is tied low
> 
> No, this is not possible. I2C pins are repurposed if I2C_EN is low.
> You can not call that an i2c bus anymore.
> 
> >   - The device is connected on an I2C bus, but I2C_EN is connected to a
> >     GPIO and the kernel needs to assert its state at boot.
> 
> This is a pretty strange configuration.  The I2C_EN pin isn't supposed
> to be toggled dynamically. Anyway, if that happens, I'd use pinctrl /
> hog to control the pin.

ACK. I still believe it would be valuable, but I don't really want to be
part of that conversation anymore. Marc, do whatever you want.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Marc Gonzalez]

On 30/07/2024 11:30, Maxime Ripard wrote:
> On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote:
>> On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote:
>>> On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote:
>>>> On 15/07/2024 16:40, Maxime Ripard wrote:
>>>>> On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote:
>>>>>> On 01/07/2024 15:50, Maxime Ripard wrote:
>>>>>>
>>>>>>> The i2c register access (and the whole behaviour of the device) is
>>>>>>> constrained on the I2C_EN pin status, and you can't read it from the
>>>>>>> device, so it's also something we need to have in the DT.
>>>>>>
>>>>>> I think the purpose of the I2C_EN pin might have been misunderstood.
>>>>>>
>>>>>> I2C_EN is not meant to be toggled, ever, by anyone from this planet.
>>>>>
>>>>> Toggled, probably not. Connected to a GPIO and the kernel has to assert
>>>>> a level at boot, I've seen worse hardware design already.
>>>>>
>>>>>> I2C_EN is a layout-time setting, decided by a board manufacturer:
>>>>>>
>>>>>> - If the TDP158 is fully configured once-and-for-all at layout-time,
>>>>>> then no I2C bus is required, and I2C_EN is pulled down forever.
>>>>>>
>>>>>> - If the board manufacturer wants to keep open the possibility
>>>>>> to adjust some parameters at run-time, then they must connect
>>>>>> the device to an I2C bus, and I2C_EN is pulled up forever.
>>>>>
>>>>> How do you express both cases in your current binding?
>>>>
>>>> It's not that I'm ignoring your question.
>>>>
>>>> It's that I don't understand what you're asking.
>>>
>>> And that's fine, you just need to say so.
>>>
>>> Generally speaking, you're focusing on the driver. The driver is not the
>>> issue here. You can do whatever you want in the driver for all I care,
>>> we can change that later on as we wish.
>>>
>>> The binding however cannot change, so it *has* to ideally cover all
>>> possible situations the hardware can be used in, or at a minimum leave
>>> the door open to support those without a compatibility breakage.
>>>
>>> That's why I've been asking those questions, because so far the only
>>> thing you've claimed is that "I can't test the driver for anything
>>> else", but, again, whether there's a driver or not, or if it's
>>> functional, is completely missing the point.
>>>
>>>> SITUATION 1
>>>> tdp158 is pin strapped.
>>>> Device node is child of root node.
>>>> Properties in proposed binding are valid (regulators and power-on pin)
>>>> Can be supported via module_platform_driver.
>>>>
>>>> SITUATION 2
>>>> tdp158 is sitting on I2C bus.
>>>> Device node is child of i2c bus node.
>>>> (robh said missing reg prop would be flagged by the compiler)
>>>> Properties in proposed binding are valid (regulators and power-on pin)
>>>> Supported via module_i2c_driver.
>>>>
>>>> If some settings-specific properties are added later, like skew,
>>>> they would only be valid for the I2C programmable mode, obviously.
>>>
>>> I think there's a couple more combinations:
>>>
>>>   - The device is connected on an I2C bus, but I2C_EN is tied low
>>
>> No, this is not possible. I2C pins are repurposed if I2C_EN is low.
>> You can not call that an i2c bus anymore.
>>
>>>   - The device is connected on an I2C bus, but I2C_EN is connected to a
>>>     GPIO and the kernel needs to assert its state at boot.
>>
>> This is a pretty strange configuration.  The I2C_EN pin isn't supposed
>> to be toggled dynamically. Anyway, if that happens, I'd use pinctrl /
>> hog to control the pin.
> 
> ACK. I still believe it would be valuable, but I don't really want to be
> part of that conversation anymore. Marc, do whatever you want.

OK.

I'll send the v4 sitting in my queue.

Regards

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

[Dmitry Baryshkov]

On Tue, Jul 30, 2024 at 11:30:01AM GMT, Maxime Ripard wrote:
> On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote:
> > On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote:
> > > > On 15/07/2024 16:40, Maxime Ripard wrote:
> > > > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote:
> > > > >> On 01/07/2024 15:50, Maxime Ripard wrote:
> > > > >>
> > > > >>> The i2c register access (and the whole behaviour of the device) is
> > > > >>> constrained on the I2C_EN pin status, and you can't read it from the
> > > > >>> device, so it's also something we need to have in the DT.
> > > > >>
> > > > >> I think the purpose of the I2C_EN pin might have been misunderstood.
> > > > >>
> > > > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet.
> > > > >
> > > > > Toggled, probably not. Connected to a GPIO and the kernel has to assert
> > > > > a level at boot, I've seen worse hardware design already.
> > > > >
> > > > >> I2C_EN is a layout-time setting, decided by a board manufacturer:
> > > > >>
> > > > >> - If the TDP158 is fully configured once-and-for-all at layout-time,
> > > > >> then no I2C bus is required, and I2C_EN is pulled down forever.
> > > > >>
> > > > >> - If the board manufacturer wants to keep open the possibility
> > > > >> to adjust some parameters at run-time, then they must connect
> > > > >> the device to an I2C bus, and I2C_EN is pulled up forever.
> > > > >
> > > > > How do you express both cases in your current binding?
> > > >
> > > > It's not that I'm ignoring your question.
> > > >
> > > > It's that I don't understand what you're asking.
> > >
> > > And that's fine, you just need to say so.
> > >
> > > Generally speaking, you're focusing on the driver. The driver is not the
> > > issue here. You can do whatever you want in the driver for all I care,
> > > we can change that later on as we wish.
> > >
> > > The binding however cannot change, so it *has* to ideally cover all
> > > possible situations the hardware can be used in, or at a minimum leave
> > > the door open to support those without a compatibility breakage.
> > >
> > > That's why I've been asking those questions, because so far the only
> > > thing you've claimed is that "I can't test the driver for anything
> > > else", but, again, whether there's a driver or not, or if it's
> > > functional, is completely missing the point.
> > >
> > > > SITUATION 1
> > > > tdp158 is pin strapped.
> > > > Device node is child of root node.
> > > > Properties in proposed binding are valid (regulators and power-on pin)
> > > > Can be supported via module_platform_driver.
> > > >
> > > > SITUATION 2
> > > > tdp158 is sitting on I2C bus.
> > > > Device node is child of i2c bus node.
> > > > (robh said missing reg prop would be flagged by the compiler)
> > > > Properties in proposed binding are valid (regulators and power-on pin)
> > > > Supported via module_i2c_driver.
> > > >
> > > > If some settings-specific properties are added later, like skew,
> > > > they would only be valid for the I2C programmable mode, obviously.
> > >
> > > I think there's a couple more combinations:
> > >
> > >   - The device is connected on an I2C bus, but I2C_EN is tied low
> > 
> > No, this is not possible. I2C pins are repurposed if I2C_EN is low.
> > You can not call that an i2c bus anymore.
> > 
> > >   - The device is connected on an I2C bus, but I2C_EN is connected to a
> > >     GPIO and the kernel needs to assert its state at boot.
> > 
> > This is a pretty strange configuration.  The I2C_EN pin isn't supposed
> > to be toggled dynamically. Anyway, if that happens, I'd use pinctrl /
> > hog to control the pin.
> 
> ACK. I still believe it would be valuable, but I don't really want to be
> part of that conversation anymore. Marc, do whatever you want.

Just to explain it, from my way of thinking the I2C_EN pin is the same
as the address-strapping pins: they are usually hardwired by the device
manufacturer.

-- 
With best wishes
Dmitry