[PATCH v2 0/2] drm: bridge: Add TI tmds181 and sn65dp159 driver (RFC)

[PATCH v2 0/2] drm: bridge: Add TI tmds181 and sn65dp159 driver (RFC)

[Mike Looijmans]

In the past I've seen (and contributed to) hacks that model the chips as
phy or even (really!) clock drivers. Since the chip usually sits between
a signal that is (almost) HDMI and a HDMI connector, I decided to stop
lying and write it as a DRM bridge driver.

Our experience with these chips is that they work best under manual control
enabling them only once the signal is active. At low resolutions (under 4k),
the optimal setting is usually to only use redriver mode. Setting the
termination to 150-300 Ohms improves EMC performance at lower resolutions,
hence the driver enables 75-150 Ohms for HDMI2 modes and defaults to
150-300 Ohm termination for other modes.


Changes in v2:
Document driver specific bindings like slew-rate and threshold
Use atomic_enable/disable
Use #defines for bit fields in registers
Allow HDMI 2 compliance
Filter modes on clock range
Use cross-over pixel frequency instead of manual overides
Devicetree bindings according to standards

Mike Looijmans (2):
  dt-bindings: drm/bridge: ti-tmds181: Add TI TMDS181 and SN65DP159
    bindings
  drm: bridge: Add TI tmds181 and sn65dp159 driver

 .../bindings/display/bridge/ti,tmds181.yaml   | 143 +++++
 drivers/gpu/drm/bridge/Kconfig                |  11 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/ti-tmds181.c           | 561 ++++++++++++++++++
 4 files changed, 716 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
 create mode 100644 drivers/gpu/drm/bridge/ti-tmds181.c

-- 
2.43.0

base-commit: 53e760d8949895390e256e723e7ee46618310361
branch: drm-ti-tmds181

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

[PATCH v2 1/2] dt-bindings: drm/bridge: ti-tmds181: Add TI TMDS181 and SN65DP159 bindings

[Mike Looijmans]

Add DT binding document for TI TMDS181 and SN65DP159 HDMI retimers.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

Changes in v2:
Document driver specific bindings like slew-rate and threshold

 .../bindings/display/bridge/ti,tmds181.yaml   | 143 ++++++++++++++++++
 1 file changed, 143 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
new file mode 100644
index 000000000000..816bea54846f
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
@@ -0,0 +1,143 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/ti,tmds181.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TMDS181 and SN65DP159 HDMI retimer/redriver chips
+
+maintainers:
+  - Mike Looijmans <mike.looijmans@topic.nl>
+
+description: |
+  Texas Instruments TMDS181 and SN65DP159 retimer and redriver chips.
+  https://www.ti.com/product/TMDS181
+  https://www.ti.com/product/TMDS181
+
+properties:
+  compatible:
+    enum:
+      - ti,tmds181
+      - ti,sn65dp159
+
+  reg:
+    enum:
+      - 0x5b
+      - 0x5c
+      - 0x5d
+      - 0x5e
+
+  oe-gpios:
+    maxItems: 1
+    description: GPIO specifier for OE pin (active high).
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description: Video port for HDMI (ish) input
+
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+
+      port@1:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description: Video port for HDMI output (panel or bridge)
+
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+
+    required:
+      - port@0
+      - port@1
+
+  ti,source-mode:
+    type: boolean
+    description:
+      Force chip to operate in "source" mode. Allows to use
+      a TMDS181 chip (which defaults to sink) as cable driver.
+
+  ti,sink-mode:
+    type: boolean
+    description:
+      Force chip to operate in "sink" mode. Allows to use
+      a DP159 chip (defaults to source) for incoming signals.
+
+  ti,retimer-threshold-hz:
+    minimum: 25000000
+    maximum: 600000000
+    default: 200000000
+    description:
+      Cross-over point. Up until this pixel clock frequency
+      the chip remains in the low-power redriver mode. Above
+      the threshold the chip should operate in retimer mode.
+
+  ti,dvi-mode:
+    type: boolean
+    description: Makes the DP159 chip operate in DVI mode.
+
+  ti,slew-rate:
+    minimum: 0
+    maximum: 3
+    default: 3
+    description: Set slew rate, 0 is slowest, 3 is fastest.
+
+  ti,disable-equalizer:
+    type: boolean
+    description: Disable the equalizer (to save power).
+
+  ti,adaptive-equalizer:
+    type: boolean
+    description: Set the equalizer to adaptive mode.
+
+required:
+  - compatible
+  - reg
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        bridge@5b {
+            compatible = "ti,sn65dp159";
+            reg = <0x5b>;
+
+            oe-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+
+                    endpoint {
+                        remote-endpoint = <&encoder_out>;
+                    };
+                };
+
+                port@1 {
+                    reg = <1>;
+
+                    endpoint {
+                        remote-endpoint = <&hdmi_connector_in>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.43.0


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

[PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver

[Mike Looijmans]

The tmds181 and sn65dp159 are "retimers" and hence can be considered
HDMI-to-HDMI bridges. Typical usage is to convert the output of an
FPGA into a valid HDMI signal, and it will typically be inserted
between an encoder and hdmi-connector.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

Changes in v2:
Use atomic_enable/disable
Use #defines for bit fields in registers
Allow HDMI 2 compliance
Filter modes on clock range
Use cross-over pixel frequency instead of manual overides
Devicetree bindings according to standards

 drivers/gpu/drm/bridge/Kconfig      |  11 +
 drivers/gpu/drm/bridge/Makefile     |   1 +
 drivers/gpu/drm/bridge/ti-tmds181.c | 561 ++++++++++++++++++++++++++++
 3 files changed, 573 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-tmds181.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index b9e0ca85226a..753177fc9b50 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -430,6 +430,17 @@ config DRM_TI_SN65DSI86
 	help
 	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
 
+config DRM_TI_TMDS181
+        tristate "TI TMDS181 and SN65DP159 HDMI retimer bridge driver"
+	depends on OF
+	select DRM_KMS_HELPER
+	select REGMAP_I2C
+	help
+	  Enable this to support the TI TMDS181 and SN65DP159 HDMI retimers.
+	  The SN65DP159 provides output into a cable (source) whereas the
+	  TMDS181 is meant to forward a cable signal into a PCB (sink). Either
+	  can be set up as source or sink though.
+
 config DRM_TI_TPD12S015
 	tristate "TI TPD12S015 HDMI level shifter and ESD protection"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 245e8a27e3fc..f4b5089e903c 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -39,6 +39,7 @@ 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_TMDS181) += ti-tmds181.o
 obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
 obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
 obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
diff --git a/drivers/gpu/drm/bridge/ti-tmds181.c b/drivers/gpu/drm/bridge/ti-tmds181.c
new file mode 100644
index 000000000000..34cbdb066820
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-tmds181.c
@@ -0,0 +1,561 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TI tmds181 and sn65dp159 HDMI redriver and retimer chips
+ *
+ * Copyright (C) 2018 - 2025 Topic Embedded Products <www.topic.nl>
+ *
+ * based on code
+ * Copyright (C) 2007 Hans Verkuil
+ * Copyright (C) 2016, 2017 Leon Woestenberg <leon@sidebranch.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+
+MODULE_DESCRIPTION("I2C device driver for DP159 and TMDS181 redriver/retimer");
+MODULE_AUTHOR("Mike Looijmans");
+MODULE_LICENSE("GPL");
+
+#define TMDS181_REG_ID		0
+#define TMDS181_REG_REV		0x8
+#define TMDS181_REG_CTRL9	0x9
+/* Registers A and B have a volatile bit, but we don't use it, so cache is ok */
+#define TMDS181_REG_CTRLA	0xA
+#define TMDS181_REG_CTRLB	0xB
+#define TMDS181_REG_CTRLC	0xC
+#define TMDS181_REG_EQUALIZER	0xD
+#define TMDS181_REG_EYESCAN	0xE
+
+#define TMDS181_CTRL9_SIG_EN			BIT(4)
+#define TMDS181_CTRL9_PD_EN			BIT(3)
+#define TMDS181_CTRL9_HPD_AUTO_PWRDWN_DISABLE	BIT(2)
+#define TMDS181_CTRL9_I2C_DR_CTL		GENMASK(1, 0)
+
+#define TMDS181_CTRLA_MODE_SINK			BIT(7)
+#define TMDS181_CTRLA_HPDSNK_GATE_EN		BIT(6)
+#define TMDS181_CTRLA_EQ_ADA_EN			BIT(5)
+#define TMDS181_CTRLA_EQ_EN			BIT(4)
+#define TMDS181_CTRLA_AUX_BRG_EN		BIT(3)
+#define TMDS181_CTRLA_APPLY			BIT(2)
+#define TMDS181_CTRLA_DEV_FUNC_MODE		GENMASK(1, 0)
+
+#define TMDS181_CTRLB_SLEW_CTL			GENMASK(7, 6)
+#define TMDS181_CTRLB_HDMI_SEL_DVI		BIT(5)
+#define TMDS181_CTRLB_TX_TERM_CTL		GENMASK(4, 3)
+#define TMDS181_CTRLB_DDC_DR_SEL		BIT(2)
+#define TMDS181_CTRLB_TMDS_CLOCK_RATIO_STATUS	BIT(1)
+#define TMDS181_CTRLB_DDC_TRAIN_SET		BIT(0)
+
+#define TMDS181_CTRLB_TX_TERM_150_300_OHMS	1
+#define TMDS181_CTRLB_TX_TERM_75_150_OHMS	3
+
+#define TMDS181_CTRLC_VSWING_DATA		GENMASK(7, 5)
+#define TMDS181_CTRLC_VSWING_CLK		GENMASK(4, 2)
+#define TMDS181_CTRLC_HDMI_TWPST1		GENMASK(1, 0)
+
+#define TMDS181_EQ_DATA_LANE			GENMASK(5, 3)
+#define TMDS181_EQ_CLOCK_LANE			GENMASK(2, 1)
+#define TMDS181_EQ_DIS_HDMI2_SWG		BIT(0)
+
+/* Above this data rate HDMI2 standards apply (TX termination) */
+#define HDMI2_PIXEL_RATE_KHZ	340000
+
+enum tmds181_chip {
+	tmds181,
+	dp159,
+};
+
+struct tmds181_data {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct drm_bridge *next_bridge;
+	struct drm_bridge bridge;
+	u32 retimer_threshold_khz;
+	enum tmds181_chip chip;
+};
+
+static inline struct tmds181_data *
+drm_bridge_to_tmds181_data(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct tmds181_data, bridge);
+}
+
+/* Set "apply" bit in control register after making changes */
+static int tmds181_apply_changes(struct tmds181_data *data)
+{
+	return regmap_write_bits(data->regmap, TMDS181_REG_CTRLA,
+				 TMDS181_CTRLA_APPLY, TMDS181_CTRLA_APPLY);
+}
+
+static int tmds181_attach(struct drm_bridge *bridge,
+			  enum drm_bridge_attach_flags flags)
+{
+	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
+
+	return drm_bridge_attach(bridge->encoder, data->next_bridge, bridge,
+				 flags);
+}
+
+static enum drm_mode_status
+tmds181_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info,
+		   const struct drm_display_mode *mode)
+{
+	/* Clock limits: clk between 25 and 350 MHz, clk is 1/10 of bit clock */
+	if (mode->clock < 25000)
+		return MODE_CLOCK_LOW;
+
+	/* The actual HDMI clock (if provided) cannot exceed 350MHz */
+	if (mode->crtc_clock > 350000)
+		return MODE_CLOCK_HIGH;
+
+	/*
+	 * When in HDMI 2 mode, the clock is 1/40th of the bitrate. The limit is
+	 * then the data rate of 6Gbps, which would use a 600MHz pixel clock.
+	 */
+	if (mode->clock > 600000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static void tmds181_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
+{
+	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
+	struct drm_atomic_state *state = old_bridge_state->base.state;
+	const struct drm_bridge_state *bridge_state;
+	const struct drm_crtc_state *crtc_state;
+	const struct drm_display_mode *mode;
+	struct drm_connector *connector;
+	struct drm_crtc *crtc;
+	unsigned int val;
+
+	/*
+	 * Retrieve the CRTC adjusted mode. This requires a little dance to go
+	 * from the bridge to the encoder, to the connector and to the CRTC.
+	 */
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
+	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	mode = &crtc_state->adjusted_mode;
+
+	/* Set retimer/redriver mode based on pixel clock */
+	val = mode->clock > data->retimer_threshold_khz ? TMDS181_CTRLA_DEV_FUNC_MODE : 0;
+	regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
+			   TMDS181_CTRLA_DEV_FUNC_MODE, val);
+
+	/* Configure TX termination based on pixel clock */
+	val = mode->clock > HDMI2_PIXEL_RATE_KHZ ?
+				TMDS181_CTRLB_TX_TERM_75_150_OHMS :
+				TMDS181_CTRLB_TX_TERM_150_300_OHMS;
+	regmap_update_bits(data->regmap, TMDS181_REG_CTRLB,
+			   TMDS181_CTRLB_TX_TERM_CTL,
+			   FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, val));
+
+	regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
+			   TMDS181_CTRL9_PD_EN, 0);
+}
+
+static void tmds181_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
+{
+	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
+
+	/* Set the PD_EN bit */
+	regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
+			   TMDS181_CTRL9_PD_EN, TMDS181_CTRL9_PD_EN);
+}
+
+static const struct drm_bridge_funcs tmds181_bridge_funcs = {
+	.attach		= tmds181_attach,
+	.mode_valid	= tmds181_mode_valid,
+	.atomic_enable	= tmds181_enable,
+	.atomic_disable	= tmds181_disable,
+
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+};
+
+static const char * const tmds181_modes[] = {
+	"redriver",
+	"auto1",
+	"auto2",
+	"retimer",
+};
+
+static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct tmds181_data *data = dev_get_drvdata(dev);
+	const char *equalizer;
+	u32 val;
+	int ret;
+
+	ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val & BIT(4)) {
+		if (val & BIT(5))
+			equalizer = "eq-adaptive";
+		else
+			equalizer = "eq-fixed";
+	} else {
+		equalizer = "eq-disabled";
+	}
+
+	return scnprintf(buf, PAGE_SIZE, "%6s %s %s\n",
+			(val & BIT(7)) ? "sink" : "source",
+			tmds181_modes[val & 0x03],
+			equalizer);
+}
+
+static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t len)
+{
+	struct tmds181_data *data = dev_get_drvdata(dev);
+	u32 val;
+	int ret;
+	int i;
+
+	/* Strip trailing newline(s) for being user friendly */
+	while (len && buf[len] == '\n')
+		--len;
+
+	/* Need at least 4 actual characters */
+	if (len < 4)
+		return -EINVAL;
+
+	ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(tmds181_modes); ++i) {
+		if (strncmp(tmds181_modes[i], buf, len) == 0) {
+			val &= ~TMDS181_CTRLA_DEV_FUNC_MODE;
+			val |= FIELD_PREP(TMDS181_CTRLA_DEV_FUNC_MODE, i);
+			break;
+		}
+	}
+
+	if (strncmp("sink", buf, len) == 0)
+		val |= TMDS181_CTRLA_MODE_SINK;
+
+	if (strncmp("source", buf, len) == 0)
+		val &= ~TMDS181_CTRLA_MODE_SINK;
+
+	if (strncmp("eq-", buf, 3) == 0) {
+		switch (buf[3]) {
+		case 'a': /* adaptive */
+			val |= TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN;
+			break;
+		case 'f': /* fixed */
+			val |= TMDS181_CTRLA_EQ_EN;
+			val &= ~TMDS181_CTRLA_EQ_ADA_EN;
+			break;
+		case 'd': /* disable */
+			val &= ~(TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN);
+			break;
+		}
+	}
+
+	/* Always set the "apply changes" bit */
+	val |= TMDS181_CTRLA_APPLY;
+
+	ret = regmap_write(data->regmap, TMDS181_REG_CTRLA, val);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+/* termination for HDMI TX: 0=off, 1=150..300, 3=75..150 ohms */
+static ssize_t termination_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct tmds181_data *data = dev_get_drvdata(dev);
+	u32 val;
+	int ret;
+
+	ret = regmap_read(data->regmap, TMDS181_REG_CTRLB, &val);
+	if (ret < 0)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			 FIELD_GET(TMDS181_CTRLB_TX_TERM_CTL, val));
+}
+
+static ssize_t termination_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct tmds181_data *data = dev_get_drvdata(dev);
+	u32 val;
+	unsigned long newval;
+	int ret;
+
+	ret = regmap_read(data->regmap, TMDS181_REG_CTRLB, &val);
+	if (ret < 0)
+		return ret;
+
+	ret = kstrtoul((const char *) buf, 10, &newval);
+	if (ret)
+		return ret;
+
+	if (newval > 3)
+		return -EINVAL;
+
+	val &= ~TMDS181_CTRLB_TX_TERM_CTL;
+	val |= FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, newval);
+
+	ret = regmap_write(data->regmap, TMDS181_REG_CTRLB, val);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static DEVICE_ATTR_RW(mode);
+static DEVICE_ATTR_RW(termination);
+
+static struct attribute *tmds181_attrs[] = {
+	&dev_attr_mode.attr,
+	&dev_attr_termination.attr,
+	NULL,
+};
+
+static const struct attribute_group tmds181_attr_group = {
+	.attrs = tmds181_attrs,
+};
+
+static const u8 tmds181_id_tmds181[8] __nonstring = "TMDS181 ";
+static const u8 tmds181_id_dp159[8]   __nonstring = "DP159   ";
+
+static int tmds181_check_id(struct tmds181_data *data)
+{
+	int ret;
+	int retry;
+	u8 buffer[8];
+
+	for (retry = 0; retry < 20; ++retry) {
+		ret = regmap_bulk_read(data->regmap, TMDS181_REG_ID, buffer,
+				       sizeof(buffer));
+		if (!ret)
+			break;
+
+		/* Compensate for very long OE power-up delays due to the cap */
+		usleep_range(5000, 10000);
+	}
+
+	if (ret) {
+		dev_err(&data->client->dev, "I2C read ID failed\n");
+		return ret;
+	}
+
+	if (memcmp(buffer, tmds181_id_tmds181, sizeof(buffer)) == 0) {
+		dev_info(&data->client->dev, "Detected: TMDS181\n");
+		data->chip = tmds181;
+		return 0;
+	}
+
+	if (memcmp(buffer, tmds181_id_dp159, sizeof(buffer)) == 0) {
+		dev_info(&data->client->dev, "Detected: DP159\n");
+		data->chip = dp159;
+		return 0;
+	}
+
+	dev_err(&data->client->dev, "Unknown ID: %*pE\n", (int)sizeof(buffer), buffer);
+
+	return -ENODEV;
+}
+
+static bool tmds181_regmap_is_volatile(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	/* IBERT result and status registers, not used yet */
+	case 0x15:
+	case 0x17 ... 0x1F:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config tmds181_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+	.max_register = 0x20,
+	.volatile_reg = tmds181_regmap_is_volatile,
+};
+
+static int tmds181_probe(struct i2c_client *client)
+{
+	struct tmds181_data *data;
+	struct gpio_desc *oe_gpio;
+	int ret;
+	u32 param;
+	u8 val;
+
+	/* Check if the adapter supports the needed features */
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -EIO;
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	i2c_set_clientdata(client, data);
+	data->regmap = devm_regmap_init_i2c(client, &tmds181_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	/* The "OE" pin acts as a reset */
+	oe_gpio = devm_gpiod_get_optional(&client->dev, "oe", GPIOD_OUT_LOW);
+	if (IS_ERR(oe_gpio)) {
+		ret = PTR_ERR(oe_gpio);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&client->dev, "failed to acquire 'oe' gpio\n");
+		return ret;
+	}
+	if (oe_gpio) {
+		/* Need at least 100us reset pulse */
+		usleep_range(100, 200);
+		gpiod_set_value_cansleep(oe_gpio, 1);
+	}
+
+	/* Reading the ID also provides enough time for the reset */
+	ret = tmds181_check_id(data);
+	if (ret)
+		return ret;
+
+	/*
+	 * We take care of power control, so disable the chips PM functions, and
+	 * allow the DDC to run at 400kHz
+	 */
+	regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
+			TMDS181_CTRL9_SIG_EN | TMDS181_CTRL9_PD_EN |
+			TMDS181_CTRL9_HPD_AUTO_PWRDWN_DISABLE |
+			TMDS181_CTRL9_I2C_DR_CTL,
+			TMDS181_CTRL9_PD_EN |
+			TMDS181_CTRL9_HPD_AUTO_PWRDWN_DISABLE |
+			TMDS181_CTRL9_I2C_DR_CTL);
+
+	/* Apply configuration changes */
+	if (of_property_read_bool(client->dev.of_node, "source-mode"))
+		regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
+				   TMDS181_CTRLA_MODE_SINK, 0);
+	if (of_property_read_bool(client->dev.of_node, "sink-mode"))
+		regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
+				   TMDS181_CTRLA_MODE_SINK, TMDS181_CTRLA_MODE_SINK);
+
+	/*
+	 * Using the automatic modes of the chip uses considerable power as it
+	 * will keep the PLL running at all times. So instead, define our own
+	 * threshold for the pixel rate. This also allows to use a sane default
+	 * of 200MHz pixel rate for the redriver-retimer crossover point, as the
+	 * modes below 3k don't show any benefit from the retimer.
+	 */
+	data->retimer_threshold_khz = 200000;
+	if (!of_property_read_u32(client->dev.of_node,
+				  "ti,retimer-threshold-hz", &param))
+		data->retimer_threshold_khz = param / 1000;
+
+	/* Default to low-power redriver mode */
+	regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
+			   TMDS181_CTRLA_DEV_FUNC_MODE, 0x00);
+
+	if (of_property_read_bool(client->dev.of_node, "ti,adaptive-equalizer"))
+		regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
+				   TMDS181_CTRLA_EQ_EN | TMDS181_CTRLA_EQ_ADA_EN,
+				   TMDS181_CTRLA_EQ_EN | TMDS181_CTRLA_EQ_ADA_EN);
+	if (of_property_read_bool(client->dev.of_node, "ti,disable-equalizer"))
+		regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
+				   TMDS181_CTRLA_EQ_EN | TMDS181_CTRLA_EQ_ADA_EN,
+				   0);
+
+	switch (data->chip) {
+	case dp159:
+		val = 0;
+		if (!of_property_read_u32(client->dev.of_node,
+					"slew-rate", &param)) {
+			if (param > 3) {
+				dev_err(&client->dev, "invalid slew-rate\n");
+				return -EINVAL;
+			}
+			/* Implement 0 = slow, 3 = fast slew rate */
+			val = FIELD_PREP(TMDS181_CTRLB_SLEW_CTL, (3 - param));
+		}
+		if (of_property_read_bool(client->dev.of_node, "ti,dvi-mode"))
+			val |= TMDS181_CTRLB_HDMI_SEL_DVI;
+		break;
+	default:
+		val = TMDS181_CTRLB_DDC_DR_SEL;
+		break;
+	}
+
+	/* Default to low-speed termination */
+	val |= FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, TMDS181_CTRLB_TX_TERM_150_300_OHMS);
+
+	ret = regmap_write(data->regmap, TMDS181_REG_CTRLB, val);
+	if (ret < 0) {
+		dev_err(&client->dev, "regmap_write(B) failed\n");
+		return ret;
+	}
+
+	ret = sysfs_create_group(&client->dev.kobj, &tmds181_attr_group);
+	if (ret)
+		dev_err(&client->dev, "sysfs_create_group error: %d\n", ret);
+
+	/* Find next bridge in chain */
+	data->next_bridge = devm_drm_of_get_bridge(&client->dev, client->dev.of_node, 1, 0);
+	if (IS_ERR(data->next_bridge))
+		return dev_err_probe(&client->dev, PTR_ERR(data->next_bridge),
+				     "Failed to find next bridge\n");
+
+	/* Register the bridge. */
+	data->bridge.funcs = &tmds181_bridge_funcs;
+	data->bridge.of_node = client->dev.of_node;
+
+	return devm_drm_bridge_add(&client->dev, &data->bridge);
+}
+
+static const struct i2c_device_id tmds181_id[] = {
+	{ "tmds181", tmds181 },
+	{ "sn65dp159", dp159 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, tmds181_id);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tmds181_of_match[] = {
+	{ .compatible = "ti,tmds181", },
+	{ .compatible = "ti,sn65dp159", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tmds181_of_match);
+#endif
+
+static struct i2c_driver tmds181_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name	= "tmds181",
+		.of_match_table = of_match_ptr(tmds181_of_match),
+	},
+	.probe		= tmds181_probe,
+	.id_table	= tmds181_id,
+};
+
+module_i2c_driver(tmds181_driver);
-- 
2.43.0


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH v2 1/2] dt-bindings: drm/bridge: ti-tmds181: Add TI TMDS181 and SN65DP159 bindings

[Rob Herring (Arm)]

On Tue, 19 Aug 2025 07:31:14 +0200, Mike Looijmans wrote:
> Add DT binding document for TI TMDS181 and SN65DP159 HDMI retimers.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> Changes in v2:
> Document driver specific bindings like slew-rate and threshold
> 
>  .../bindings/display/bridge/ti,tmds181.yaml   | 143 ++++++++++++++++++
>  1 file changed, 143 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml: ti,slew-rate: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250819053205.9976-2-mike.looijmans@topic.nl

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 1/2] dt-bindings: drm/bridge: ti-tmds181: Add TI TMDS181 and SN65DP159 bindings

[Krzysztof Kozlowski]

On Tue, Aug 19, 2025 at 07:31:14AM +0200, Mike Looijmans wrote:
> Add DT binding document for TI TMDS181 and SN65DP159 HDMI retimers.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> Changes in v2:
> Document driver specific bindings like slew-rate and threshold
> 
>  .../bindings/display/bridge/ti,tmds181.yaml   | 143 ++++++++++++++++++
>  1 file changed, 143 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
> new file mode 100644
> index 000000000000..816bea54846f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
> @@ -0,0 +1,143 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,tmds181.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TMDS181 and SN65DP159 HDMI retimer/redriver chips
> +
> +maintainers:
> +  - Mike Looijmans <mike.looijmans@topic.nl>
> +
> +description: |
> +  Texas Instruments TMDS181 and SN65DP159 retimer and redriver chips.
> +  https://www.ti.com/product/TMDS181
> +  https://www.ti.com/product/TMDS181
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tmds181
> +      - ti,sn65dp159

Nothing improved, you did not respond to review, you did not say why it
is not implemented in changelog. You did not explain that choice in
commit msg, either.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver

[Krzysztof Kozlowski]

On Tue, Aug 19, 2025 at 07:31:15AM +0200, Mike Looijmans wrote:
> +	switch (data->chip) {
> +	case dp159:
> +		val = 0;
> +		if (!of_property_read_u32(client->dev.of_node,
> +					"slew-rate", &param)) {

You never tested your code or your DTS.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: drm/bridge: ti-tmds181: Add TI TMDS181 and SN65DP159 bindings

[Mike Looijmans]

On 19-08-2025 09:03, Krzysztof Kozlowski wrote:
> On Tue, Aug 19, 2025 at 07:31:14AM +0200, Mike Looijmans wrote:
>> Add DT binding document for TI TMDS181 and SN65DP159 HDMI retimers.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>
>> ---
>>
>> Changes in v2:
>> Document driver specific bindings like slew-rate and threshold
>>
>>   .../bindings/display/bridge/ti,tmds181.yaml   | 143 ++++++++++++++++++
>>   1 file changed, 143 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
>> new file mode 100644
>> index 000000000000..816bea54846f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tmds181.yaml
>> @@ -0,0 +1,143 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ti,tmds181.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: TMDS181 and SN65DP159 HDMI retimer/redriver chips
>> +
>> +maintainers:
>> +  - Mike Looijmans <mike.looijmans@topic.nl>
>> +
>> +description: |
>> +  Texas Instruments TMDS181 and SN65DP159 retimer and redriver chips.
>> +  https://www.ti.com/product/TMDS181
>> +  https://www.ti.com/product/TMDS181
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,tmds181
>> +      - ti,sn65dp159
> Nothing improved, you did not respond to review, you did not say why it
> is not implemented in changelog. You did not explain that choice in
> commit msg, either.

Apologies, my bad. I totally forgot.


>
> Best regards,
> Krzysztof

-- 
Mike Looijmans

Re: [PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver

[Maxime Ripard]

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

On Tue, Aug 19, 2025 at 07:31:15AM +0200, Mike Looijmans wrote:
> The tmds181 and sn65dp159 are "retimers" and hence can be considered
> HDMI-to-HDMI bridges. Typical usage is to convert the output of an
> FPGA into a valid HDMI signal, and it will typically be inserted
> between an encoder and hdmi-connector.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
> Changes in v2:
> Use atomic_enable/disable
> Use #defines for bit fields in registers
> Allow HDMI 2 compliance
> Filter modes on clock range
> Use cross-over pixel frequency instead of manual overides
> Devicetree bindings according to standards
> 
>  drivers/gpu/drm/bridge/Kconfig      |  11 +
>  drivers/gpu/drm/bridge/Makefile     |   1 +
>  drivers/gpu/drm/bridge/ti-tmds181.c | 561 ++++++++++++++++++++++++++++
>  3 files changed, 573 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ti-tmds181.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index b9e0ca85226a..753177fc9b50 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -430,6 +430,17 @@ config DRM_TI_SN65DSI86
>  	help
>  	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>  
> +config DRM_TI_TMDS181
> +        tristate "TI TMDS181 and SN65DP159 HDMI retimer bridge driver"
> +	depends on OF
> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C
> +	help
> +	  Enable this to support the TI TMDS181 and SN65DP159 HDMI retimers.
> +	  The SN65DP159 provides output into a cable (source) whereas the
> +	  TMDS181 is meant to forward a cable signal into a PCB (sink). Either
> +	  can be set up as source or sink though.
> +
>  config DRM_TI_TPD12S015
>  	tristate "TI TPD12S015 HDMI level shifter and ESD protection"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 245e8a27e3fc..f4b5089e903c 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -39,6 +39,7 @@ 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_TMDS181) += ti-tmds181.o
>  obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
>  obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>  obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
> diff --git a/drivers/gpu/drm/bridge/ti-tmds181.c b/drivers/gpu/drm/bridge/ti-tmds181.c
> new file mode 100644
> index 000000000000..34cbdb066820
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-tmds181.c
> @@ -0,0 +1,561 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TI tmds181 and sn65dp159 HDMI redriver and retimer chips
> + *
> + * Copyright (C) 2018 - 2025 Topic Embedded Products <www.topic.nl>
> + *
> + * based on code
> + * Copyright (C) 2007 Hans Verkuil
> + * Copyright (C) 2016, 2017 Leon Woestenberg <leon@sidebranch.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +
> +MODULE_DESCRIPTION("I2C device driver for DP159 and TMDS181 redriver/retimer");
> +MODULE_AUTHOR("Mike Looijmans");
> +MODULE_LICENSE("GPL");
> +
> +#define TMDS181_REG_ID		0
> +#define TMDS181_REG_REV		0x8
> +#define TMDS181_REG_CTRL9	0x9
> +/* Registers A and B have a volatile bit, but we don't use it, so cache is ok */
> +#define TMDS181_REG_CTRLA	0xA
> +#define TMDS181_REG_CTRLB	0xB
> +#define TMDS181_REG_CTRLC	0xC
> +#define TMDS181_REG_EQUALIZER	0xD
> +#define TMDS181_REG_EYESCAN	0xE
> +
> +#define TMDS181_CTRL9_SIG_EN			BIT(4)
> +#define TMDS181_CTRL9_PD_EN			BIT(3)
> +#define TMDS181_CTRL9_HPD_AUTO_PWRDWN_DISABLE	BIT(2)
> +#define TMDS181_CTRL9_I2C_DR_CTL		GENMASK(1, 0)
> +
> +#define TMDS181_CTRLA_MODE_SINK			BIT(7)
> +#define TMDS181_CTRLA_HPDSNK_GATE_EN		BIT(6)
> +#define TMDS181_CTRLA_EQ_ADA_EN			BIT(5)
> +#define TMDS181_CTRLA_EQ_EN			BIT(4)
> +#define TMDS181_CTRLA_AUX_BRG_EN		BIT(3)
> +#define TMDS181_CTRLA_APPLY			BIT(2)
> +#define TMDS181_CTRLA_DEV_FUNC_MODE		GENMASK(1, 0)
> +
> +#define TMDS181_CTRLB_SLEW_CTL			GENMASK(7, 6)
> +#define TMDS181_CTRLB_HDMI_SEL_DVI		BIT(5)
> +#define TMDS181_CTRLB_TX_TERM_CTL		GENMASK(4, 3)
> +#define TMDS181_CTRLB_DDC_DR_SEL		BIT(2)
> +#define TMDS181_CTRLB_TMDS_CLOCK_RATIO_STATUS	BIT(1)
> +#define TMDS181_CTRLB_DDC_TRAIN_SET		BIT(0)
> +
> +#define TMDS181_CTRLB_TX_TERM_150_300_OHMS	1
> +#define TMDS181_CTRLB_TX_TERM_75_150_OHMS	3
> +
> +#define TMDS181_CTRLC_VSWING_DATA		GENMASK(7, 5)
> +#define TMDS181_CTRLC_VSWING_CLK		GENMASK(4, 2)
> +#define TMDS181_CTRLC_HDMI_TWPST1		GENMASK(1, 0)
> +
> +#define TMDS181_EQ_DATA_LANE			GENMASK(5, 3)
> +#define TMDS181_EQ_CLOCK_LANE			GENMASK(2, 1)
> +#define TMDS181_EQ_DIS_HDMI2_SWG		BIT(0)
> +
> +/* Above this data rate HDMI2 standards apply (TX termination) */
> +#define HDMI2_PIXEL_RATE_KHZ	340000
> +
> +enum tmds181_chip {
> +	tmds181,
> +	dp159,
> +};
> +
> +struct tmds181_data {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct drm_bridge *next_bridge;
> +	struct drm_bridge bridge;
> +	u32 retimer_threshold_khz;
> +	enum tmds181_chip chip;
> +};
> +
> +static inline struct tmds181_data *
> +drm_bridge_to_tmds181_data(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct tmds181_data, bridge);
> +}
> +
> +/* Set "apply" bit in control register after making changes */
> +static int tmds181_apply_changes(struct tmds181_data *data)
> +{
> +	return regmap_write_bits(data->regmap, TMDS181_REG_CTRLA,
> +				 TMDS181_CTRLA_APPLY, TMDS181_CTRLA_APPLY);
> +}
> +
> +static int tmds181_attach(struct drm_bridge *bridge,
> +			  enum drm_bridge_attach_flags flags)
> +{
> +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, data->next_bridge, bridge,
> +				 flags);
> +}
> +
> +static enum drm_mode_status
> +tmds181_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info,
> +		   const struct drm_display_mode *mode)
> +{
> +	/* Clock limits: clk between 25 and 350 MHz, clk is 1/10 of bit clock */
> +	if (mode->clock < 25000)
> +		return MODE_CLOCK_LOW;

I'm a bit confused by your comment here. Why do we care about the bit clock here?

> +	/* The actual HDMI clock (if provided) cannot exceed 350MHz */
> +	if (mode->crtc_clock > 350000)
> +		return MODE_CLOCK_HIGH;

And what do you call the "actual HDMI clock" and why wouldn't it be provided?

> +	/*
> +	 * When in HDMI 2 mode, the clock is 1/40th of the bitrate. The limit is
> +	 * then the data rate of 6Gbps, which would use a 600MHz pixel clock.
> +	 */
> +	if (mode->clock > 600000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
> +static void tmds181_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
> +{
> +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
> +	struct drm_atomic_state *state = old_bridge_state->base.state;
> +	const struct drm_bridge_state *bridge_state;
> +	const struct drm_crtc_state *crtc_state;
> +	const struct drm_display_mode *mode;
> +	struct drm_connector *connector;
> +	struct drm_crtc *crtc;
> +	unsigned int val;
> +
> +	/*
> +	 * Retrieve the CRTC adjusted mode. This requires a little dance to go
> +	 * from the bridge to the encoder, to the connector and to the CRTC.
> +	 */
> +	connector = drm_atomic_get_new_connector_for_encoder(state,
> +							     bridge->encoder);
> +	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	mode = &crtc_state->adjusted_mode;
> +
> +	/* Set retimer/redriver mode based on pixel clock */
> +	val = mode->clock > data->retimer_threshold_khz ? TMDS181_CTRLA_DEV_FUNC_MODE : 0;
> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
> +			   TMDS181_CTRLA_DEV_FUNC_MODE, val);
> +
> +	/* Configure TX termination based on pixel clock */
> +	val = mode->clock > HDMI2_PIXEL_RATE_KHZ ?
> +				TMDS181_CTRLB_TX_TERM_75_150_OHMS :
> +				TMDS181_CTRLB_TX_TERM_150_300_OHMS;
> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRLB,
> +			   TMDS181_CTRLB_TX_TERM_CTL,
> +			   FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, val));
> +
> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
> +			   TMDS181_CTRL9_PD_EN, 0);
> +}
> +
> +static void tmds181_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
> +{
> +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
> +
> +	/* Set the PD_EN bit */
> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
> +			   TMDS181_CTRL9_PD_EN, TMDS181_CTRL9_PD_EN);
> +}
> +
> +static const struct drm_bridge_funcs tmds181_bridge_funcs = {
> +	.attach		= tmds181_attach,
> +	.mode_valid	= tmds181_mode_valid,
> +	.atomic_enable	= tmds181_enable,
> +	.atomic_disable	= tmds181_disable,
> +
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +};
> +
> +static const char * const tmds181_modes[] = {
> +	"redriver",
> +	"auto1",
> +	"auto2",
> +	"retimer",
> +};
> +
> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct tmds181_data *data = dev_get_drvdata(dev);
> +	const char *equalizer;
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val & BIT(4)) {
> +		if (val & BIT(5))
> +			equalizer = "eq-adaptive";
> +		else
> +			equalizer = "eq-fixed";
> +	} else {
> +		equalizer = "eq-disabled";
> +	}
> +
> +	return scnprintf(buf, PAGE_SIZE, "%6s %s %s\n",
> +			(val & BIT(7)) ? "sink" : "source",
> +			tmds181_modes[val & 0x03],
> +			equalizer);
> +}
> +
> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t len)
> +{
> +	struct tmds181_data *data = dev_get_drvdata(dev);
> +	u32 val;
> +	int ret;
> +	int i;
> +
> +	/* Strip trailing newline(s) for being user friendly */
> +	while (len && buf[len] == '\n')
> +		--len;
> +
> +	/* Need at least 4 actual characters */
> +	if (len < 4)
> +		return -EINVAL;
> +
> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(tmds181_modes); ++i) {
> +		if (strncmp(tmds181_modes[i], buf, len) == 0) {
> +			val &= ~TMDS181_CTRLA_DEV_FUNC_MODE;
> +			val |= FIELD_PREP(TMDS181_CTRLA_DEV_FUNC_MODE, i);
> +			break;
> +		}
> +	}
> +
> +	if (strncmp("sink", buf, len) == 0)
> +		val |= TMDS181_CTRLA_MODE_SINK;
> +
> +	if (strncmp("source", buf, len) == 0)
> +		val &= ~TMDS181_CTRLA_MODE_SINK;
> +
> +	if (strncmp("eq-", buf, 3) == 0) {
> +		switch (buf[3]) {
> +		case 'a': /* adaptive */
> +			val |= TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN;
> +			break;
> +		case 'f': /* fixed */
> +			val |= TMDS181_CTRLA_EQ_EN;
> +			val &= ~TMDS181_CTRLA_EQ_ADA_EN;
> +			break;
> +		case 'd': /* disable */
> +			val &= ~(TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN);
> +			break;
> +		}
> +	}
> +
> +	/* Always set the "apply changes" bit */
> +	val |= TMDS181_CTRLA_APPLY;
> +
> +	ret = regmap_write(data->regmap, TMDS181_REG_CTRLA, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}

The datasheet seems to define when to use a mode over another. Why would
we want to force it over what it requires?

What happens if that change is made during an atomic_commit?

> +/* termination for HDMI TX: 0=off, 1=150..300, 3=75..150 ohms */
> +static ssize_t termination_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct tmds181_data *data = dev_get_drvdata(dev);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLB, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
> +			 FIELD_GET(TMDS181_CTRLB_TX_TERM_CTL, val));
> +}
> +
> +static ssize_t termination_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	struct tmds181_data *data = dev_get_drvdata(dev);
> +	u32 val;
> +	unsigned long newval;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLB, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = kstrtoul((const char *) buf, 10, &newval);
> +	if (ret)
> +		return ret;
> +
> +	if (newval > 3)
> +		return -EINVAL;
> +
> +	val &= ~TMDS181_CTRLB_TX_TERM_CTL;
> +	val |= FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, newval);
> +
> +	ret = regmap_write(data->regmap, TMDS181_REG_CTRLB, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}

Same question. The datasheet defines which termination to use based on
the pixel clock. Why should we want anything else?

Also, since it's in userspace, where is the documentation and userspace
code using that API?

> +static DEVICE_ATTR_RW(mode);
> +static DEVICE_ATTR_RW(termination);
> +
> +static struct attribute *tmds181_attrs[] = {
> +	&dev_attr_mode.attr,
> +	&dev_attr_termination.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group tmds181_attr_group = {
> +	.attrs = tmds181_attrs,
> +};
> +
> +static const u8 tmds181_id_tmds181[8] __nonstring = "TMDS181 ";
> +static const u8 tmds181_id_dp159[8]   __nonstring = "DP159   ";
> +
> +static int tmds181_check_id(struct tmds181_data *data)
> +{
> +	int ret;
> +	int retry;
> +	u8 buffer[8];
> +
> +	for (retry = 0; retry < 20; ++retry) {
> +		ret = regmap_bulk_read(data->regmap, TMDS181_REG_ID, buffer,
> +				       sizeof(buffer));
> +		if (!ret)
> +			break;
> +
> +		/* Compensate for very long OE power-up delays due to the cap */
> +		usleep_range(5000, 10000);
> +	}
> +
> +	if (ret) {
> +		dev_err(&data->client->dev, "I2C read ID failed\n");
> +		return ret;
> +	}
> +
> +	if (memcmp(buffer, tmds181_id_tmds181, sizeof(buffer)) == 0) {
> +		dev_info(&data->client->dev, "Detected: TMDS181\n");
> +		data->chip = tmds181;
> +		return 0;
> +	}
> +
> +	if (memcmp(buffer, tmds181_id_dp159, sizeof(buffer)) == 0) {
> +		dev_info(&data->client->dev, "Detected: DP159\n");
> +		data->chip = dp159;
> +		return 0;
> +	}
> +
> +	dev_err(&data->client->dev, "Unknown ID: %*pE\n", (int)sizeof(buffer), buffer);
> +
> +	return -ENODEV;
> +}
> +
> +static bool tmds181_regmap_is_volatile(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	/* IBERT result and status registers, not used yet */
> +	case 0x15:
> +	case 0x17 ... 0x1F:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config tmds181_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,
> +	.max_register = 0x20,
> +	.volatile_reg = tmds181_regmap_is_volatile,
> +};
> +
> +static int tmds181_probe(struct i2c_client *client)
> +{
> +	struct tmds181_data *data;
> +	struct gpio_desc *oe_gpio;
> +	int ret;
> +	u32 param;
> +	u8 val;
> +
> +	/* Check if the adapter supports the needed features */
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -EIO;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;

devm_drm_bridge_alloc()

Maxime

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

Re: [PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver

[Mike Looijmans]

On 19-08-2025 15:22, Maxime Ripard wrote:
> On Tue, Aug 19, 2025 at 07:31:15AM +0200, Mike Looijmans wrote:
>> The tmds181 and sn65dp159 are "retimers" and hence can be considered
>> HDMI-to-HDMI bridges. Typical usage is to convert the output of an
>> FPGA into a valid HDMI signal, and it will typically be inserted
>> between an encoder and hdmi-connector.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>
>> Changes in v2:
>> Use atomic_enable/disable
>> Use #defines for bit fields in registers
>> Allow HDMI 2 compliance
>> Filter modes on clock range
>> Use cross-over pixel frequency instead of manual overides
>> Devicetree bindings according to standards
>>
>>   drivers/gpu/drm/bridge/Kconfig      |  11 +
>>   drivers/gpu/drm/bridge/Makefile     |   1 +
>>   drivers/gpu/drm/bridge/ti-tmds181.c | 561 ++++++++++++++++++++++++++++
>>   3 files changed, 573 insertions(+)
>>   create mode 100644 drivers/gpu/drm/bridge/ti-tmds181.c
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index b9e0ca85226a..753177fc9b50 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -430,6 +430,17 @@ config DRM_TI_SN65DSI86
>>   	help
>>   	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>>   
>> +config DRM_TI_TMDS181
>> +        tristate "TI TMDS181 and SN65DP159 HDMI retimer bridge driver"
>> +	depends on OF
>> +	select DRM_KMS_HELPER
>> +	select REGMAP_I2C
>> +	help
>> +	  Enable this to support the TI TMDS181 and SN65DP159 HDMI retimers.
>> +	  The SN65DP159 provides output into a cable (source) whereas the
>> +	  TMDS181 is meant to forward a cable signal into a PCB (sink). Either
>> +	  can be set up as source or sink though.
>> +
>>   config DRM_TI_TPD12S015
>>   	tristate "TI TPD12S015 HDMI level shifter and ESD protection"
>>   	depends on OF
>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>> index 245e8a27e3fc..f4b5089e903c 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -39,6 +39,7 @@ 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_TMDS181) += ti-tmds181.o
>>   obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
>>   obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>>   obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
>> diff --git a/drivers/gpu/drm/bridge/ti-tmds181.c b/drivers/gpu/drm/bridge/ti-tmds181.c
>> new file mode 100644
>> index 000000000000..34cbdb066820
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-tmds181.c
>> @@ -0,0 +1,561 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * TI tmds181 and sn65dp159 HDMI redriver and retimer chips
>> + *
>> + * Copyright (C) 2018 - 2025 Topic Embedded Products <www.topic.nl>
>> + *
>> + * based on code
>> + * Copyright (C) 2007 Hans Verkuil
>> + * Copyright (C) 2016, 2017 Leon Woestenberg <leon@sidebranch.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/i2c.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/regmap.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/delay.h>
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_bridge.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_print.h>
>> +#include <drm/drm_probe_helper.h>
>> +
>> +MODULE_DESCRIPTION("I2C device driver for DP159 and TMDS181 redriver/retimer");
>> +MODULE_AUTHOR("Mike Looijmans");
>> +MODULE_LICENSE("GPL");
>> +
>> +#define TMDS181_REG_ID		0
>> +#define TMDS181_REG_REV		0x8
>> +#define TMDS181_REG_CTRL9	0x9
>> +/* Registers A and B have a volatile bit, but we don't use it, so cache is ok */
>> +#define TMDS181_REG_CTRLA	0xA
>> +#define TMDS181_REG_CTRLB	0xB
>> +#define TMDS181_REG_CTRLC	0xC
>> +#define TMDS181_REG_EQUALIZER	0xD
>> +#define TMDS181_REG_EYESCAN	0xE
>> +
>> +#define TMDS181_CTRL9_SIG_EN			BIT(4)
>> +#define TMDS181_CTRL9_PD_EN			BIT(3)
>> +#define TMDS181_CTRL9_HPD_AUTO_PWRDWN_DISABLE	BIT(2)
>> +#define TMDS181_CTRL9_I2C_DR_CTL		GENMASK(1, 0)
>> +
>> +#define TMDS181_CTRLA_MODE_SINK			BIT(7)
>> +#define TMDS181_CTRLA_HPDSNK_GATE_EN		BIT(6)
>> +#define TMDS181_CTRLA_EQ_ADA_EN			BIT(5)
>> +#define TMDS181_CTRLA_EQ_EN			BIT(4)
>> +#define TMDS181_CTRLA_AUX_BRG_EN		BIT(3)
>> +#define TMDS181_CTRLA_APPLY			BIT(2)
>> +#define TMDS181_CTRLA_DEV_FUNC_MODE		GENMASK(1, 0)
>> +
>> +#define TMDS181_CTRLB_SLEW_CTL			GENMASK(7, 6)
>> +#define TMDS181_CTRLB_HDMI_SEL_DVI		BIT(5)
>> +#define TMDS181_CTRLB_TX_TERM_CTL		GENMASK(4, 3)
>> +#define TMDS181_CTRLB_DDC_DR_SEL		BIT(2)
>> +#define TMDS181_CTRLB_TMDS_CLOCK_RATIO_STATUS	BIT(1)
>> +#define TMDS181_CTRLB_DDC_TRAIN_SET		BIT(0)
>> +
>> +#define TMDS181_CTRLB_TX_TERM_150_300_OHMS	1
>> +#define TMDS181_CTRLB_TX_TERM_75_150_OHMS	3
>> +
>> +#define TMDS181_CTRLC_VSWING_DATA		GENMASK(7, 5)
>> +#define TMDS181_CTRLC_VSWING_CLK		GENMASK(4, 2)
>> +#define TMDS181_CTRLC_HDMI_TWPST1		GENMASK(1, 0)
>> +
>> +#define TMDS181_EQ_DATA_LANE			GENMASK(5, 3)
>> +#define TMDS181_EQ_CLOCK_LANE			GENMASK(2, 1)
>> +#define TMDS181_EQ_DIS_HDMI2_SWG		BIT(0)
>> +
>> +/* Above this data rate HDMI2 standards apply (TX termination) */
>> +#define HDMI2_PIXEL_RATE_KHZ	340000
>> +
>> +enum tmds181_chip {
>> +	tmds181,
>> +	dp159,
>> +};
>> +
>> +struct tmds181_data {
>> +	struct i2c_client *client;
>> +	struct regmap *regmap;
>> +	struct drm_bridge *next_bridge;
>> +	struct drm_bridge bridge;
>> +	u32 retimer_threshold_khz;
>> +	enum tmds181_chip chip;
>> +};
>> +
>> +static inline struct tmds181_data *
>> +drm_bridge_to_tmds181_data(struct drm_bridge *bridge)
>> +{
>> +	return container_of(bridge, struct tmds181_data, bridge);
>> +}
>> +
>> +/* Set "apply" bit in control register after making changes */
>> +static int tmds181_apply_changes(struct tmds181_data *data)
>> +{
>> +	return regmap_write_bits(data->regmap, TMDS181_REG_CTRLA,
>> +				 TMDS181_CTRLA_APPLY, TMDS181_CTRLA_APPLY);
>> +}
>> +
>> +static int tmds181_attach(struct drm_bridge *bridge,
>> +			  enum drm_bridge_attach_flags flags)
>> +{
>> +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
>> +
>> +	return drm_bridge_attach(bridge->encoder, data->next_bridge, bridge,
>> +				 flags);
>> +}
>> +
>> +static enum drm_mode_status
>> +tmds181_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info,
>> +		   const struct drm_display_mode *mode)
>> +{
>> +	/* Clock limits: clk between 25 and 350 MHz, clk is 1/10 of bit clock */
>> +	if (mode->clock < 25000)
>> +		return MODE_CLOCK_LOW;
> I'm a bit confused by your comment here. Why do we care about the bit clock here?

Purpose here is to filter out modes that certainly won't work. For 
example, a standard 640x480 mode would have a 24MHz clock, which this 
chip doesn't support according to the datasheet.


>> +	/* The actual HDMI clock (if provided) cannot exceed 350MHz */
>> +	if (mode->crtc_clock > 350000)
>> +		return MODE_CLOCK_HIGH;
> And what do you call the "actual HDMI clock" and why wouldn't it be provided?

The clock signal on the HDMI cable.

Again, aim is to block modes that certainly won't work.

While developing the driver (and logging lots) the crtc_clock was often 
just "0".

This is still tricky though. What if we'd have a DSI-to-HDMI bridge in 
between this and the crtc, and outputting a 4k display mode? The 
DSI-to-HDMI bridge output clock (to the retimer) won't match the CRTC 
clock, nor would it match the pixel clock (using 1/40 clock rate).

So maybe I'll have to drop this filter. Or figure out how to obtain the 
incoming clock rate. Currently don't have the hardware to test the 
limits here.


>
>> +	/*
>> +	 * When in HDMI 2 mode, the clock is 1/40th of the bitrate. The limit is
>> +	 * then the data rate of 6Gbps, which would use a 600MHz pixel clock.
>> +	 */
>> +	if (mode->clock > 600000)
>> +		return MODE_CLOCK_HIGH;
>> +
>> +	return MODE_OK;
>> +}
>> +
>> +static void tmds181_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
>> +{
>> +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
>> +	struct drm_atomic_state *state = old_bridge_state->base.state;
>> +	const struct drm_bridge_state *bridge_state;
>> +	const struct drm_crtc_state *crtc_state;
>> +	const struct drm_display_mode *mode;
>> +	struct drm_connector *connector;
>> +	struct drm_crtc *crtc;
>> +	unsigned int val;
>> +
>> +	/*
>> +	 * Retrieve the CRTC adjusted mode. This requires a little dance to go
>> +	 * from the bridge to the encoder, to the connector and to the CRTC.
>> +	 */
>> +	connector = drm_atomic_get_new_connector_for_encoder(state,
>> +							     bridge->encoder);
>> +	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
>> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> +	mode = &crtc_state->adjusted_mode;
>> +
>> +	/* Set retimer/redriver mode based on pixel clock */
>> +	val = mode->clock > data->retimer_threshold_khz ? TMDS181_CTRLA_DEV_FUNC_MODE : 0;
>> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
>> +			   TMDS181_CTRLA_DEV_FUNC_MODE, val);
>> +
>> +	/* Configure TX termination based on pixel clock */
>> +	val = mode->clock > HDMI2_PIXEL_RATE_KHZ ?
>> +				TMDS181_CTRLB_TX_TERM_75_150_OHMS :
>> +				TMDS181_CTRLB_TX_TERM_150_300_OHMS;
>> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRLB,
>> +			   TMDS181_CTRLB_TX_TERM_CTL,
>> +			   FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, val));
>> +
>> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
>> +			   TMDS181_CTRL9_PD_EN, 0);
>> +}
>> +
>> +static void tmds181_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
>> +{
>> +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
>> +
>> +	/* Set the PD_EN bit */
>> +	regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
>> +			   TMDS181_CTRL9_PD_EN, TMDS181_CTRL9_PD_EN);
>> +}
>> +
>> +static const struct drm_bridge_funcs tmds181_bridge_funcs = {
>> +	.attach		= tmds181_attach,
>> +	.mode_valid	= tmds181_mode_valid,
>> +	.atomic_enable	= tmds181_enable,
>> +	.atomic_disable	= tmds181_disable,
>> +
>> +	.atomic_reset = drm_atomic_helper_bridge_reset,
>> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>> +};
>> +
>> +static const char * const tmds181_modes[] = {
>> +	"redriver",
>> +	"auto1",
>> +	"auto2",
>> +	"retimer",
>> +};
>> +
>> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
>> +			 char *buf)
>> +{
>> +	struct tmds181_data *data = dev_get_drvdata(dev);
>> +	const char *equalizer;
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (val & BIT(4)) {
>> +		if (val & BIT(5))
>> +			equalizer = "eq-adaptive";
>> +		else
>> +			equalizer = "eq-fixed";
>> +	} else {
>> +		equalizer = "eq-disabled";
>> +	}
>> +
>> +	return scnprintf(buf, PAGE_SIZE, "%6s %s %s\n",
>> +			(val & BIT(7)) ? "sink" : "source",
>> +			tmds181_modes[val & 0x03],
>> +			equalizer);
>> +}
>> +
>> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
>> +			  const char *buf, size_t len)
>> +{
>> +	struct tmds181_data *data = dev_get_drvdata(dev);
>> +	u32 val;
>> +	int ret;
>> +	int i;
>> +
>> +	/* Strip trailing newline(s) for being user friendly */
>> +	while (len && buf[len] == '\n')
>> +		--len;
>> +
>> +	/* Need at least 4 actual characters */
>> +	if (len < 4)
>> +		return -EINVAL;
>> +
>> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tmds181_modes); ++i) {
>> +		if (strncmp(tmds181_modes[i], buf, len) == 0) {
>> +			val &= ~TMDS181_CTRLA_DEV_FUNC_MODE;
>> +			val |= FIELD_PREP(TMDS181_CTRLA_DEV_FUNC_MODE, i);
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (strncmp("sink", buf, len) == 0)
>> +		val |= TMDS181_CTRLA_MODE_SINK;
>> +
>> +	if (strncmp("source", buf, len) == 0)
>> +		val &= ~TMDS181_CTRLA_MODE_SINK;
>> +
>> +	if (strncmp("eq-", buf, 3) == 0) {
>> +		switch (buf[3]) {
>> +		case 'a': /* adaptive */
>> +			val |= TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN;
>> +			break;
>> +		case 'f': /* fixed */
>> +			val |= TMDS181_CTRLA_EQ_EN;
>> +			val &= ~TMDS181_CTRLA_EQ_ADA_EN;
>> +			break;
>> +		case 'd': /* disable */
>> +			val &= ~(TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN);
>> +			break;
>> +		}
>> +	}
>> +
>> +	/* Always set the "apply changes" bit */
>> +	val |= TMDS181_CTRLA_APPLY;
>> +
>> +	ret = regmap_write(data->regmap, TMDS181_REG_CTRLA, val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return len;
>> +}
> The datasheet seems to define when to use a mode over another. Why would
> we want to force it over what it requires?
>
> What happens if that change is made during an atomic_commit?

Hmm, this is mostly a leftover from EMC and compliance testing (wanted 
to be able to control a few knobs) several years ago. Best is to just 
remove this.


>
>> +/* termination for HDMI TX: 0=off, 1=150..300, 3=75..150 ohms */
>> +static ssize_t termination_show(struct device *dev,
>> +		struct device_attribute *attr, char *buf)
>> +{
>> +	struct tmds181_data *data = dev_get_drvdata(dev);
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLB, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +			 FIELD_GET(TMDS181_CTRLB_TX_TERM_CTL, val));
>> +}
>> +
>> +static ssize_t termination_store(struct device *dev,
>> +		struct device_attribute *attr, const char *buf, size_t len)
>> +{
>> +	struct tmds181_data *data = dev_get_drvdata(dev);
>> +	u32 val;
>> +	unsigned long newval;
>> +	int ret;
>> +
>> +	ret = regmap_read(data->regmap, TMDS181_REG_CTRLB, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = kstrtoul((const char *) buf, 10, &newval);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (newval > 3)
>> +		return -EINVAL;
>> +
>> +	val &= ~TMDS181_CTRLB_TX_TERM_CTL;
>> +	val |= FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, newval);
>> +
>> +	ret = regmap_write(data->regmap, TMDS181_REG_CTRLB, val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return len;
>> +}
> Same question. The datasheet defines which termination to use based on
> the pixel clock. Why should we want anything else?
>
> Also, since it's in userspace, where is the documentation and userspace
> code using that API?

Also leftover from compliance and EMC testing. We've learned what the 
best settings are and that's now part of the driver code, so this should 
be removed.


>
>> +static DEVICE_ATTR_RW(mode);
>> +static DEVICE_ATTR_RW(termination);
>> +
>> +static struct attribute *tmds181_attrs[] = {
>> +	&dev_attr_mode.attr,
>> +	&dev_attr_termination.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group tmds181_attr_group = {
>> +	.attrs = tmds181_attrs,
>> +};
>> +
>> +static const u8 tmds181_id_tmds181[8] __nonstring = "TMDS181 ";
>> +static const u8 tmds181_id_dp159[8]   __nonstring = "DP159   ";
>> +
>> +static int tmds181_check_id(struct tmds181_data *data)
>> +{
>> +	int ret;
>> +	int retry;
>> +	u8 buffer[8];
>> +
>> +	for (retry = 0; retry < 20; ++retry) {
>> +		ret = regmap_bulk_read(data->regmap, TMDS181_REG_ID, buffer,
>> +				       sizeof(buffer));
>> +		if (!ret)
>> +			break;
>> +
>> +		/* Compensate for very long OE power-up delays due to the cap */
>> +		usleep_range(5000, 10000);
>> +	}
>> +
>> +	if (ret) {
>> +		dev_err(&data->client->dev, "I2C read ID failed\n");
>> +		return ret;
>> +	}
>> +
>> +	if (memcmp(buffer, tmds181_id_tmds181, sizeof(buffer)) == 0) {
>> +		dev_info(&data->client->dev, "Detected: TMDS181\n");
>> +		data->chip = tmds181;
>> +		return 0;
>> +	}
>> +
>> +	if (memcmp(buffer, tmds181_id_dp159, sizeof(buffer)) == 0) {
>> +		dev_info(&data->client->dev, "Detected: DP159\n");
>> +		data->chip = dp159;
>> +		return 0;
>> +	}
>> +
>> +	dev_err(&data->client->dev, "Unknown ID: %*pE\n", (int)sizeof(buffer), buffer);
>> +
>> +	return -ENODEV;
>> +}
>> +
>> +static bool tmds181_regmap_is_volatile(struct device *dev, unsigned int reg)
>> +{
>> +	switch (reg) {
>> +	/* IBERT result and status registers, not used yet */
>> +	case 0x15:
>> +	case 0x17 ... 0x1F:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static const struct regmap_config tmds181_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.cache_type = REGCACHE_RBTREE,
>> +	.max_register = 0x20,
>> +	.volatile_reg = tmds181_regmap_is_volatile,
>> +};
>> +
>> +static int tmds181_probe(struct i2c_client *client)
>> +{
>> +	struct tmds181_data *data;
>> +	struct gpio_desc *oe_gpio;
>> +	int ret;
>> +	u32 param;
>> +	u8 val;
>> +
>> +	/* Check if the adapter supports the needed features */
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>> +		return -EIO;
>> +
>> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
> devm_drm_bridge_alloc()

Will do.


>
> Maxime


-- 
Mike Looijmans

Re: [PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver

[kernel test robot]

Hi Mike,

kernel test robot noticed the following build errors:

[auto build test ERROR on 53e760d8949895390e256e723e7ee46618310361]

url:    https://github.com/intel-lab-lkp/linux/commits/Mike-Looijmans/dt-bindings-drm-bridge-ti-tmds181-Add-TI-TMDS181-and-SN65DP159-bindings/20250819-133458
base:   53e760d8949895390e256e723e7ee46618310361
patch link:    https://lore.kernel.org/r/20250819053205.9976-3-mike.looijmans%40topic.nl
patch subject: [PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver
config: parisc-allmodconfig (https://download.01.org/0day-ci/archive/20250820/202508200712.FUHg7vmw-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250820/202508200712.FUHg7vmw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508200712.FUHg7vmw-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/bridge/ti-tmds181.c: In function 'tmds181_enable':
>> drivers/gpu/drm/bridge/ti-tmds181.c:165:28: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
     165 |                            FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, val));
         |                            ^~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c:137:40: warning: unused variable 'bridge_state' [-Wunused-variable]
     137 |         const struct drm_bridge_state *bridge_state;
         |                                        ^~~~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c: At top level:
   drivers/gpu/drm/bridge/ti-tmds181.c:181:27: error: initialization of 'int (*)(struct drm_bridge *, struct drm_encoder *, enum drm_bridge_attach_flags)' from incompatible pointer type 'int (*)(struct drm_bridge *, enum drm_bridge_attach_flags)' [-Wincompatible-pointer-types]
     181 |         .attach         = tmds181_attach,
         |                           ^~~~~~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c:181:27: note: (near initialization for 'tmds181_bridge_funcs.attach')
   drivers/gpu/drm/bridge/ti-tmds181.c:102:12: note: 'tmds181_attach' declared here
     102 | static int tmds181_attach(struct drm_bridge *bridge,
         |            ^~~~~~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c:183:27: error: initialization of 'void (*)(struct drm_bridge *, struct drm_atomic_state *)' from incompatible pointer type 'void (*)(struct drm_bridge *, struct drm_bridge_state *)' [-Wincompatible-pointer-types]
     183 |         .atomic_enable  = tmds181_enable,
         |                           ^~~~~~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c:183:27: note: (near initialization for 'tmds181_bridge_funcs.atomic_enable')
   drivers/gpu/drm/bridge/ti-tmds181.c:133:13: note: 'tmds181_enable' declared here
     133 | static void tmds181_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
         |             ^~~~~~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c:184:27: error: initialization of 'void (*)(struct drm_bridge *, struct drm_atomic_state *)' from incompatible pointer type 'void (*)(struct drm_bridge *, struct drm_bridge_state *)' [-Wincompatible-pointer-types]
     184 |         .atomic_disable = tmds181_disable,
         |                           ^~~~~~~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c:184:27: note: (near initialization for 'tmds181_bridge_funcs.atomic_disable')
   drivers/gpu/drm/bridge/ti-tmds181.c:171:13: note: 'tmds181_disable' declared here
     171 | static void tmds181_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
         |             ^~~~~~~~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c: In function 'termination_show':
>> drivers/gpu/drm/bridge/ti-tmds181.c:297:26: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
     297 |                          FIELD_GET(TMDS181_CTRLB_TX_TERM_CTL, val));
         |                          ^~~~~~~~~
   drivers/gpu/drm/bridge/ti-tmds181.c: At top level:
   drivers/gpu/drm/bridge/ti-tmds181.c:96:12: warning: 'tmds181_apply_changes' defined but not used [-Wunused-function]
      96 | static int tmds181_apply_changes(struct tmds181_data *data)
         |            ^~~~~~~~~~~~~~~~~~~~~


vim +/FIELD_PREP +165 drivers/gpu/drm/bridge/ti-tmds181.c

   132	
   133	static void tmds181_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
   134	{
   135		struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
   136		struct drm_atomic_state *state = old_bridge_state->base.state;
   137		const struct drm_bridge_state *bridge_state;
   138		const struct drm_crtc_state *crtc_state;
   139		const struct drm_display_mode *mode;
   140		struct drm_connector *connector;
   141		struct drm_crtc *crtc;
   142		unsigned int val;
   143	
   144		/*
   145		 * Retrieve the CRTC adjusted mode. This requires a little dance to go
   146		 * from the bridge to the encoder, to the connector and to the CRTC.
   147		 */
   148		connector = drm_atomic_get_new_connector_for_encoder(state,
   149								     bridge->encoder);
   150		crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
   151		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
   152		mode = &crtc_state->adjusted_mode;
   153	
   154		/* Set retimer/redriver mode based on pixel clock */
   155		val = mode->clock > data->retimer_threshold_khz ? TMDS181_CTRLA_DEV_FUNC_MODE : 0;
   156		regmap_update_bits(data->regmap, TMDS181_REG_CTRLA,
   157				   TMDS181_CTRLA_DEV_FUNC_MODE, val);
   158	
   159		/* Configure TX termination based on pixel clock */
   160		val = mode->clock > HDMI2_PIXEL_RATE_KHZ ?
   161					TMDS181_CTRLB_TX_TERM_75_150_OHMS :
   162					TMDS181_CTRLB_TX_TERM_150_300_OHMS;
   163		regmap_update_bits(data->regmap, TMDS181_REG_CTRLB,
   164				   TMDS181_CTRLB_TX_TERM_CTL,
 > 165				   FIELD_PREP(TMDS181_CTRLB_TX_TERM_CTL, val));
   166	
   167		regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
   168				   TMDS181_CTRL9_PD_EN, 0);
   169	}
   170	
   171	static void tmds181_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
   172	{
   173		struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
   174	
   175		/* Set the PD_EN bit */
   176		regmap_update_bits(data->regmap, TMDS181_REG_CTRL9,
   177				   TMDS181_CTRL9_PD_EN, TMDS181_CTRL9_PD_EN);
   178	}
   179	
   180	static const struct drm_bridge_funcs tmds181_bridge_funcs = {
   181		.attach		= tmds181_attach,
   182		.mode_valid	= tmds181_mode_valid,
   183		.atomic_enable	= tmds181_enable,
   184		.atomic_disable	= tmds181_disable,
   185	
   186		.atomic_reset = drm_atomic_helper_bridge_reset,
   187		.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
   188		.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
   189	};
   190	
   191	static const char * const tmds181_modes[] = {
   192		"redriver",
   193		"auto1",
   194		"auto2",
   195		"retimer",
   196	};
   197	
   198	static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
   199				 char *buf)
   200	{
   201		struct tmds181_data *data = dev_get_drvdata(dev);
   202		const char *equalizer;
   203		u32 val;
   204		int ret;
   205	
   206		ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
   207		if (ret < 0)
   208			return ret;
   209	
   210		if (val & BIT(4)) {
   211			if (val & BIT(5))
   212				equalizer = "eq-adaptive";
   213			else
   214				equalizer = "eq-fixed";
   215		} else {
   216			equalizer = "eq-disabled";
   217		}
   218	
   219		return scnprintf(buf, PAGE_SIZE, "%6s %s %s\n",
   220				(val & BIT(7)) ? "sink" : "source",
   221				tmds181_modes[val & 0x03],
   222				equalizer);
   223	}
   224	
   225	static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
   226				  const char *buf, size_t len)
   227	{
   228		struct tmds181_data *data = dev_get_drvdata(dev);
   229		u32 val;
   230		int ret;
   231		int i;
   232	
   233		/* Strip trailing newline(s) for being user friendly */
   234		while (len && buf[len] == '\n')
   235			--len;
   236	
   237		/* Need at least 4 actual characters */
   238		if (len < 4)
   239			return -EINVAL;
   240	
   241		ret = regmap_read(data->regmap, TMDS181_REG_CTRLA, &val);
   242		if (ret < 0)
   243			return ret;
   244	
   245		for (i = 0; i < ARRAY_SIZE(tmds181_modes); ++i) {
   246			if (strncmp(tmds181_modes[i], buf, len) == 0) {
   247				val &= ~TMDS181_CTRLA_DEV_FUNC_MODE;
   248				val |= FIELD_PREP(TMDS181_CTRLA_DEV_FUNC_MODE, i);
   249				break;
   250			}
   251		}
   252	
   253		if (strncmp("sink", buf, len) == 0)
   254			val |= TMDS181_CTRLA_MODE_SINK;
   255	
   256		if (strncmp("source", buf, len) == 0)
   257			val &= ~TMDS181_CTRLA_MODE_SINK;
   258	
   259		if (strncmp("eq-", buf, 3) == 0) {
   260			switch (buf[3]) {
   261			case 'a': /* adaptive */
   262				val |= TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN;
   263				break;
   264			case 'f': /* fixed */
   265				val |= TMDS181_CTRLA_EQ_EN;
   266				val &= ~TMDS181_CTRLA_EQ_ADA_EN;
   267				break;
   268			case 'd': /* disable */
   269				val &= ~(TMDS181_CTRLA_EQ_ADA_EN | TMDS181_CTRLA_EQ_EN);
   270				break;
   271			}
   272		}
   273	
   274		/* Always set the "apply changes" bit */
   275		val |= TMDS181_CTRLA_APPLY;
   276	
   277		ret = regmap_write(data->regmap, TMDS181_REG_CTRLA, val);
   278		if (ret < 0)
   279			return ret;
   280	
   281		return len;
   282	}
   283	
   284	/* termination for HDMI TX: 0=off, 1=150..300, 3=75..150 ohms */
   285	static ssize_t termination_show(struct device *dev,
   286			struct device_attribute *attr, char *buf)
   287	{
   288		struct tmds181_data *data = dev_get_drvdata(dev);
   289		u32 val;
   290		int ret;
   291	
   292		ret = regmap_read(data->regmap, TMDS181_REG_CTRLB, &val);
   293		if (ret < 0)
   294			return ret;
   295	
   296		return scnprintf(buf, PAGE_SIZE, "%u\n",
 > 297				 FIELD_GET(TMDS181_CTRLB_TX_TERM_CTL, val));
   298	}
   299	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver

[Maxime Ripard]

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

On Tue, Aug 19, 2025 at 04:25:29PM +0200, Mike Looijmans wrote:
> On 19-08-2025 15:22, Maxime Ripard wrote:
> > On Tue, Aug 19, 2025 at 07:31:15AM +0200, Mike Looijmans wrote:
> > > The tmds181 and sn65dp159 are "retimers" and hence can be considered
> > > HDMI-to-HDMI bridges. Typical usage is to convert the output of an
> > > FPGA into a valid HDMI signal, and it will typically be inserted
> > > between an encoder and hdmi-connector.
> > > 
> > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> > > ---
> > > 
> > > Changes in v2:
> > > Use atomic_enable/disable
> > > Use #defines for bit fields in registers
> > > Allow HDMI 2 compliance
> > > Filter modes on clock range
> > > Use cross-over pixel frequency instead of manual overides
> > > Devicetree bindings according to standards
> > > 
> > >   drivers/gpu/drm/bridge/Kconfig      |  11 +
> > >   drivers/gpu/drm/bridge/Makefile     |   1 +
> > >   drivers/gpu/drm/bridge/ti-tmds181.c | 561 ++++++++++++++++++++++++++++
> > >   3 files changed, 573 insertions(+)
> > >   create mode 100644 drivers/gpu/drm/bridge/ti-tmds181.c
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > > index b9e0ca85226a..753177fc9b50 100644
> > > --- a/drivers/gpu/drm/bridge/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/Kconfig
> > > @@ -430,6 +430,17 @@ config DRM_TI_SN65DSI86
> > >   	help
> > >   	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
> > > +config DRM_TI_TMDS181
> > > +        tristate "TI TMDS181 and SN65DP159 HDMI retimer bridge driver"
> > > +	depends on OF
> > > +	select DRM_KMS_HELPER
> > > +	select REGMAP_I2C
> > > +	help
> > > +	  Enable this to support the TI TMDS181 and SN65DP159 HDMI retimers.
> > > +	  The SN65DP159 provides output into a cable (source) whereas the
> > > +	  TMDS181 is meant to forward a cable signal into a PCB (sink). Either
> > > +	  can be set up as source or sink though.
> > > +
> > >   config DRM_TI_TPD12S015
> > >   	tristate "TI TPD12S015 HDMI level shifter and ESD protection"
> > >   	depends on OF
> > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > > index 245e8a27e3fc..f4b5089e903c 100644
> > > --- a/drivers/gpu/drm/bridge/Makefile
> > > +++ b/drivers/gpu/drm/bridge/Makefile
> > > @@ -39,6 +39,7 @@ 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_TMDS181) += ti-tmds181.o
> > >   obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
> > >   obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
> > >   obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
> > > diff --git a/drivers/gpu/drm/bridge/ti-tmds181.c b/drivers/gpu/drm/bridge/ti-tmds181.c
> > > new file mode 100644
> > > index 000000000000..34cbdb066820
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/ti-tmds181.c
> > > @@ -0,0 +1,561 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * TI tmds181 and sn65dp159 HDMI redriver and retimer chips
> > > + *
> > > + * Copyright (C) 2018 - 2025 Topic Embedded Products <www.topic.nl>
> > > + *
> > > + * based on code
> > > + * Copyright (C) 2007 Hans Verkuil
> > > + * Copyright (C) 2016, 2017 Leon Woestenberg <leon@sidebranch.com>
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/of.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/delay.h>
> > > +
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_bridge.h>
> > > +#include <drm/drm_crtc.h>
> > > +#include <drm/drm_print.h>
> > > +#include <drm/drm_probe_helper.h>
> > > +
> > > +MODULE_DESCRIPTION("I2C device driver for DP159 and TMDS181 redriver/retimer");
> > > +MODULE_AUTHOR("Mike Looijmans");
> > > +MODULE_LICENSE("GPL");
> > > +
> > > +#define TMDS181_REG_ID		0
> > > +#define TMDS181_REG_REV		0x8
> > > +#define TMDS181_REG_CTRL9	0x9
> > > +/* Registers A and B have a volatile bit, but we don't use it, so cache is ok */
> > > +#define TMDS181_REG_CTRLA	0xA
> > > +#define TMDS181_REG_CTRLB	0xB
> > > +#define TMDS181_REG_CTRLC	0xC
> > > +#define TMDS181_REG_EQUALIZER	0xD
> > > +#define TMDS181_REG_EYESCAN	0xE
> > > +
> > > +#define TMDS181_CTRL9_SIG_EN			BIT(4)
> > > +#define TMDS181_CTRL9_PD_EN			BIT(3)
> > > +#define TMDS181_CTRL9_HPD_AUTO_PWRDWN_DISABLE	BIT(2)
> > > +#define TMDS181_CTRL9_I2C_DR_CTL		GENMASK(1, 0)
> > > +
> > > +#define TMDS181_CTRLA_MODE_SINK			BIT(7)
> > > +#define TMDS181_CTRLA_HPDSNK_GATE_EN		BIT(6)
> > > +#define TMDS181_CTRLA_EQ_ADA_EN			BIT(5)
> > > +#define TMDS181_CTRLA_EQ_EN			BIT(4)
> > > +#define TMDS181_CTRLA_AUX_BRG_EN		BIT(3)
> > > +#define TMDS181_CTRLA_APPLY			BIT(2)
> > > +#define TMDS181_CTRLA_DEV_FUNC_MODE		GENMASK(1, 0)
> > > +
> > > +#define TMDS181_CTRLB_SLEW_CTL			GENMASK(7, 6)
> > > +#define TMDS181_CTRLB_HDMI_SEL_DVI		BIT(5)
> > > +#define TMDS181_CTRLB_TX_TERM_CTL		GENMASK(4, 3)
> > > +#define TMDS181_CTRLB_DDC_DR_SEL		BIT(2)
> > > +#define TMDS181_CTRLB_TMDS_CLOCK_RATIO_STATUS	BIT(1)
> > > +#define TMDS181_CTRLB_DDC_TRAIN_SET		BIT(0)
> > > +
> > > +#define TMDS181_CTRLB_TX_TERM_150_300_OHMS	1
> > > +#define TMDS181_CTRLB_TX_TERM_75_150_OHMS	3
> > > +
> > > +#define TMDS181_CTRLC_VSWING_DATA		GENMASK(7, 5)
> > > +#define TMDS181_CTRLC_VSWING_CLK		GENMASK(4, 2)
> > > +#define TMDS181_CTRLC_HDMI_TWPST1		GENMASK(1, 0)
> > > +
> > > +#define TMDS181_EQ_DATA_LANE			GENMASK(5, 3)
> > > +#define TMDS181_EQ_CLOCK_LANE			GENMASK(2, 1)
> > > +#define TMDS181_EQ_DIS_HDMI2_SWG		BIT(0)
> > > +
> > > +/* Above this data rate HDMI2 standards apply (TX termination) */
> > > +#define HDMI2_PIXEL_RATE_KHZ	340000
> > > +
> > > +enum tmds181_chip {
> > > +	tmds181,
> > > +	dp159,
> > > +};
> > > +
> > > +struct tmds181_data {
> > > +	struct i2c_client *client;
> > > +	struct regmap *regmap;
> > > +	struct drm_bridge *next_bridge;
> > > +	struct drm_bridge bridge;
> > > +	u32 retimer_threshold_khz;
> > > +	enum tmds181_chip chip;
> > > +};
> > > +
> > > +static inline struct tmds181_data *
> > > +drm_bridge_to_tmds181_data(struct drm_bridge *bridge)
> > > +{
> > > +	return container_of(bridge, struct tmds181_data, bridge);
> > > +}
> > > +
> > > +/* Set "apply" bit in control register after making changes */
> > > +static int tmds181_apply_changes(struct tmds181_data *data)
> > > +{
> > > +	return regmap_write_bits(data->regmap, TMDS181_REG_CTRLA,
> > > +				 TMDS181_CTRLA_APPLY, TMDS181_CTRLA_APPLY);
> > > +}
> > > +
> > > +static int tmds181_attach(struct drm_bridge *bridge,
> > > +			  enum drm_bridge_attach_flags flags)
> > > +{
> > > +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
> > > +
> > > +	return drm_bridge_attach(bridge->encoder, data->next_bridge, bridge,
> > > +				 flags);
> > > +}
> > > +
> > > +static enum drm_mode_status
> > > +tmds181_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info,
> > > +		   const struct drm_display_mode *mode)
> > > +{
> > > +	/* Clock limits: clk between 25 and 350 MHz, clk is 1/10 of bit clock */
> > > +	if (mode->clock < 25000)
> > > +		return MODE_CLOCK_LOW;
> > I'm a bit confused by your comment here. Why do we care about the bit clock here?
> 
> Purpose here is to filter out modes that certainly won't work. For example,
> a standard 640x480 mode would have a 24MHz clock, which this chip doesn't
> support according to the datasheet.

Sure, but you're talking about the pixel clock here, not the bit clock.
So I'm still not sure why you need to mention it in the comment.

> > > +	/* The actual HDMI clock (if provided) cannot exceed 350MHz */
> > > +	if (mode->crtc_clock > 350000)
> > > +		return MODE_CLOCK_HIGH;
> > And what do you call the "actual HDMI clock" and why wouldn't it be provided?
> 
> The clock signal on the HDMI cable.

That's not mode->clock or mode->crtc_clock then. The datasheets mention
that it can go from 0.25 to 6 Gbps. That's the TMDS char rate, which is
calculated using drm_hdmi_compute_mode_clock().

> Again, aim is to block modes that certainly won't work.
> 
> While developing the driver (and logging lots) the crtc_clock was
> often just "0".
> 
> This is still tricky though. What if we'd have a DSI-to-HDMI bridge in
> between this and the crtc, and outputting a 4k display mode? The
> DSI-to-HDMI bridge output clock (to the retimer) won't match the CRTC
> clock, nor would it match the pixel clock (using 1/40 clock rate).

It's not tricky, really. DRM internals only ever talks in pixel clock
rates, ie assuming a pixel gets sent out for each clock cycle.

It will not be the case if the buses are somewhat-serial for example,
but it allows each bus driver to derive its own internal clock from the
pixel clock rate and its parameters.

So your example works just fine, because on the DSI bus and on the HDMI
bus, you'd still get the same pixel clock rate.

Maxime

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

Re: [PATCH v2 2/2] drm: bridge: Add TI tmds181 and sn65dp159 driver

[Mike Looijmans]

On 25-08-2025 16:14, Maxime Ripard wrote:
> On Tue, Aug 19, 2025 at 04:25:29PM +0200, Mike Looijmans wrote:
>> On 19-08-2025 15:22, Maxime Ripard wrote:
>>> On Tue, Aug 19, 2025 at 07:31:15AM +0200, Mike Looijmans wrote:
>>>> The tmds181 and sn65dp159 are "retimers" and hence can be considered
>>>> HDMI-to-HDMI bridges. Typical usage is to convert the output of an
>>>> FPGA into a valid HDMI signal, and it will typically be inserted
>>>> between an encoder and hdmi-connector.
>>>>
>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> Use atomic_enable/disable
>>>> Use #defines for bit fields in registers
>>>> Allow HDMI 2 compliance
>>>> Filter modes on clock range
>>>> Use cross-over pixel frequency instead of manual overides
>>>> Devicetree bindings according to standards
>>>>
>>>>    drivers/gpu/drm/bridge/Kconfig      |  11 +
>>>>    drivers/gpu/drm/bridge/Makefile     |   1 +
>>>>    drivers/gpu/drm/bridge/ti-tmds181.c | 561 ++++++++++++++++++++++++++++
>>>>    3 files changed, 573 insertions(+)
>>>>    create mode 100644 drivers/gpu/drm/bridge/ti-tmds181.c
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>>> index b9e0ca85226a..753177fc9b50 100644
>>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>>> @@ -430,6 +430,17 @@ config DRM_TI_SN65DSI86
>>>>    	help
>>>>    	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>>>> +config DRM_TI_TMDS181
>>>> +        tristate "TI TMDS181 and SN65DP159 HDMI retimer bridge driver"
>>>> +	depends on OF
>>>> +	select DRM_KMS_HELPER
>>>> +	select REGMAP_I2C
>>>> +	help
>>>> +	  Enable this to support the TI TMDS181 and SN65DP159 HDMI retimers.
>>>> +	  The SN65DP159 provides output into a cable (source) whereas the
>>>> +	  TMDS181 is meant to forward a cable signal into a PCB (sink). Either
>>>> +	  can be set up as source or sink though.
>>>> +
>>>>    config DRM_TI_TPD12S015
>>>>    	tristate "TI TPD12S015 HDMI level shifter and ESD protection"
>>>>    	depends on OF
>>>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>>>> index 245e8a27e3fc..f4b5089e903c 100644
>>>> --- a/drivers/gpu/drm/bridge/Makefile
>>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>>> @@ -39,6 +39,7 @@ 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_TMDS181) += ti-tmds181.o
>>>>    obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
>>>>    obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>>>>    obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
>>>> diff --git a/drivers/gpu/drm/bridge/ti-tmds181.c b/drivers/gpu/drm/bridge/ti-tmds181.c
>>>> new file mode 100644
>>>> index 000000000000..34cbdb066820
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/bridge/ti-tmds181.c
>>>> @@ -0,0 +1,561 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * TI tmds181 and sn65dp159 HDMI redriver and retimer chips
>>>> + *
>>>> + * Copyright (C) 2018 - 2025 Topic Embedded Products <www.topic.nl>
>>>> + *
>>>> + * based on code
>>>> + * Copyright (C) 2007 Hans Verkuil
>>>> + * Copyright (C) 2016, 2017 Leon Woestenberg <leon@sidebranch.com>
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/delay.h>
>>>> +
>>>> +#include <drm/drm_atomic_helper.h>
>>>> +#include <drm/drm_bridge.h>
>>>> +#include <drm/drm_crtc.h>
>>>> +#include <drm/drm_print.h>
>>>> +#include <drm/drm_probe_helper.h>
>>>> +
>>>> +MODULE_DESCRIPTION("I2C device driver for DP159 and TMDS181 redriver/retimer");
>>>> +MODULE_AUTHOR("Mike Looijmans");
>>>> +MODULE_LICENSE("GPL");
>>>> +
>>>> +#define TMDS181_REG_ID		0
>>>> +#define TMDS181_REG_REV		0x8
>>>> +#define TMDS181_REG_CTRL9	0x9
>>>> +/* Registers A and B have a volatile bit, but we don't use it, so cache is ok */
>>>> +#define TMDS181_REG_CTRLA	0xA
>>>> +#define TMDS181_REG_CTRLB	0xB
>>>> +#define TMDS181_REG_CTRLC	0xC
>>>> +#define TMDS181_REG_EQUALIZER	0xD
>>>> +#define TMDS181_REG_EYESCAN	0xE
>>>> +
>>>> +#define TMDS181_CTRL9_SIG_EN			BIT(4)
>>>> +#define TMDS181_CTRL9_PD_EN			BIT(3)
>>>> +#define TMDS181_CTRL9_HPD_AUTO_PWRDWN_DISABLE	BIT(2)
>>>> +#define TMDS181_CTRL9_I2C_DR_CTL		GENMASK(1, 0)
>>>> +
>>>> +#define TMDS181_CTRLA_MODE_SINK			BIT(7)
>>>> +#define TMDS181_CTRLA_HPDSNK_GATE_EN		BIT(6)
>>>> +#define TMDS181_CTRLA_EQ_ADA_EN			BIT(5)
>>>> +#define TMDS181_CTRLA_EQ_EN			BIT(4)
>>>> +#define TMDS181_CTRLA_AUX_BRG_EN		BIT(3)
>>>> +#define TMDS181_CTRLA_APPLY			BIT(2)
>>>> +#define TMDS181_CTRLA_DEV_FUNC_MODE		GENMASK(1, 0)
>>>> +
>>>> +#define TMDS181_CTRLB_SLEW_CTL			GENMASK(7, 6)
>>>> +#define TMDS181_CTRLB_HDMI_SEL_DVI		BIT(5)
>>>> +#define TMDS181_CTRLB_TX_TERM_CTL		GENMASK(4, 3)
>>>> +#define TMDS181_CTRLB_DDC_DR_SEL		BIT(2)
>>>> +#define TMDS181_CTRLB_TMDS_CLOCK_RATIO_STATUS	BIT(1)
>>>> +#define TMDS181_CTRLB_DDC_TRAIN_SET		BIT(0)
>>>> +
>>>> +#define TMDS181_CTRLB_TX_TERM_150_300_OHMS	1
>>>> +#define TMDS181_CTRLB_TX_TERM_75_150_OHMS	3
>>>> +
>>>> +#define TMDS181_CTRLC_VSWING_DATA		GENMASK(7, 5)
>>>> +#define TMDS181_CTRLC_VSWING_CLK		GENMASK(4, 2)
>>>> +#define TMDS181_CTRLC_HDMI_TWPST1		GENMASK(1, 0)
>>>> +
>>>> +#define TMDS181_EQ_DATA_LANE			GENMASK(5, 3)
>>>> +#define TMDS181_EQ_CLOCK_LANE			GENMASK(2, 1)
>>>> +#define TMDS181_EQ_DIS_HDMI2_SWG		BIT(0)
>>>> +
>>>> +/* Above this data rate HDMI2 standards apply (TX termination) */
>>>> +#define HDMI2_PIXEL_RATE_KHZ	340000
>>>> +
>>>> +enum tmds181_chip {
>>>> +	tmds181,
>>>> +	dp159,
>>>> +};
>>>> +
>>>> +struct tmds181_data {
>>>> +	struct i2c_client *client;
>>>> +	struct regmap *regmap;
>>>> +	struct drm_bridge *next_bridge;
>>>> +	struct drm_bridge bridge;
>>>> +	u32 retimer_threshold_khz;
>>>> +	enum tmds181_chip chip;
>>>> +};
>>>> +
>>>> +static inline struct tmds181_data *
>>>> +drm_bridge_to_tmds181_data(struct drm_bridge *bridge)
>>>> +{
>>>> +	return container_of(bridge, struct tmds181_data, bridge);
>>>> +}
>>>> +
>>>> +/* Set "apply" bit in control register after making changes */
>>>> +static int tmds181_apply_changes(struct tmds181_data *data)
>>>> +{
>>>> +	return regmap_write_bits(data->regmap, TMDS181_REG_CTRLA,
>>>> +				 TMDS181_CTRLA_APPLY, TMDS181_CTRLA_APPLY);
>>>> +}
>>>> +
>>>> +static int tmds181_attach(struct drm_bridge *bridge,
>>>> +			  enum drm_bridge_attach_flags flags)
>>>> +{
>>>> +	struct tmds181_data *data = drm_bridge_to_tmds181_data(bridge);
>>>> +
>>>> +	return drm_bridge_attach(bridge->encoder, data->next_bridge, bridge,
>>>> +				 flags);
>>>> +}
>>>> +
>>>> +static enum drm_mode_status
>>>> +tmds181_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info,
>>>> +		   const struct drm_display_mode *mode)
>>>> +{
>>>> +	/* Clock limits: clk between 25 and 350 MHz, clk is 1/10 of bit clock */
>>>> +	if (mode->clock < 25000)
>>>> +		return MODE_CLOCK_LOW;
>>> I'm a bit confused by your comment here. Why do we care about the bit clock here?
>> Purpose here is to filter out modes that certainly won't work. For example,
>> a standard 640x480 mode would have a 24MHz clock, which this chip doesn't
>> support according to the datasheet.
> Sure, but you're talking about the pixel clock here, not the bit clock.
> So I'm still not sure why you need to mention it in the comment.
>
>>>> +	/* The actual HDMI clock (if provided) cannot exceed 350MHz */
>>>> +	if (mode->crtc_clock > 350000)
>>>> +		return MODE_CLOCK_HIGH;
>>> And what do you call the "actual HDMI clock" and why wouldn't it be provided?
>> The clock signal on the HDMI cable.
> That's not mode->clock or mode->crtc_clock then. The datasheets mention
> that it can go from 0.25 to 6 Gbps. That's the TMDS char rate, which is
> calculated using drm_hdmi_compute_mode_clock().

Added that, but it requires me to provide the bit depth and format, 
which this bridge doesn't know. I can at least assume 8 bit and 
HDMI_COLORSPACE_RGB as a "worst case" check for the upper limit, as 
those are certain to fail.

Fortunately the chip doesn't really care what's being transmitted, so 
not knowing the HDMI format is no big issue. Users can always lower the 
retimer threshold if their hardware does need it, which in the worst 
case would lead to the chip consuming more power than needed (but likely 
still less than without this driver).


>
>> Again, aim is to block modes that certainly won't work.
>>
>> While developing the driver (and logging lots) the crtc_clock was
>> often just "0".
>>
>> This is still tricky though. What if we'd have a DSI-to-HDMI bridge in
>> between this and the crtc, and outputting a 4k display mode? The
>> DSI-to-HDMI bridge output clock (to the retimer) won't match the CRTC
>> clock, nor would it match the pixel clock (using 1/40 clock rate).
> It's not tricky, really. DRM internals only ever talks in pixel clock
> rates, ie assuming a pixel gets sent out for each clock cycle.
>
> It will not be the case if the buses are somewhat-serial for example,
> but it allows each bus driver to derive its own internal clock from the
> pixel clock rate and its parameters.
>
> So your example works just fine, because on the DSI bus and on the HDMI
> bus, you'd still get the same pixel clock rate.

Indeed.

>
> Maxime


-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl