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

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

[Marc Gonzalez]

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

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

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

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

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

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

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

[Marc Gonzalez]

The TI TDP158 is an HDMI to TMDS Redriver.

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

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

-- 
2.34.1

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

[Conor Dooley]

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

On Tue, Jun 25, 2024 at 06:38:12PM +0200, Marc Gonzalez wrote:
> The TI TDP158 is an HDMI to TMDS Redriver.
> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  .../bindings/display/bridge/ti,tdp158.yaml         | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> new file mode 100644
> index 0000000000000..b687699e2ba80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI TDP158 HDMI to TMDS Redriver
> +
> +maintainers:
> +  - Arnaud Vrac <avrac@freebox.fr>
> +
> +properties:
> +  compatible:
> +    const: ti,tdp158
> +
> +  reg:
> +    description: I2C address of the device
> +
> +  enable-gpios:
> +    description: GPIO controlling bridge enable
> +
> +  vcc-supply:
> +    description: Power supply 3.3V
> +
> +  vdd-supply:
> +    description: Power supply 1.1V

Are these supplies not also required? Surely the device needs the power
to function?

Cheers,
Conor.

> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Bridge input
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Bridge output
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - ports
> +
> +additionalProperties: false
> 
> -- 
> 2.34.1
> 

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

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

[Marc Gonzalez]

On 26/06/2024 18:08, Conor Dooley wrote:

> On Tue, Jun 25, 2024 at 06:38:12PM +0200, Marc Gonzalez wrote:
>
>> The TI TDP158 is an HDMI to TMDS Redriver.
>>
>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>> ---
>>  .../bindings/display/bridge/ti,tdp158.yaml         | 48 ++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> new file mode 100644
>> index 0000000000000..b687699e2ba80
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
>> @@ -0,0 +1,48 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: TI TDP158 HDMI to TMDS Redriver
>> +
>> +maintainers:
>> +  - Arnaud Vrac <avrac@freebox.fr>
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,tdp158
>> +
>> +  reg:
>> +    description: I2C address of the device
>> +
>> +  enable-gpios:
>> +    description: GPIO controlling bridge enable
>> +
>> +  vcc-supply:
>> +    description: Power supply 3.3V
>> +
>> +  vdd-supply:
>> +    description: Power supply 1.1V
> 
> Are these supplies not also required? Surely the device needs the power
> to function?

Maybe if the hamsters spin fast enough in their wheels,
these supplies won't be required? :)

The reason I hesitated to mark them as required,
is because the HW engineer told us that on our board
they were connected to a power line that is shared
between several functional blocks.

I suppose that's not a reason?

Required means "device doesn't work if they're not connected" ?

Regards

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

[Conor Dooley]

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

On Wed, Jun 26, 2024 at 07:20:46PM +0200, Marc Gonzalez wrote:
> On 26/06/2024 18:08, Conor Dooley wrote:
> 
> > On Tue, Jun 25, 2024 at 06:38:12PM +0200, Marc Gonzalez wrote:
> >
> >> The TI TDP158 is an HDMI to TMDS Redriver.
> >>
> >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> >> ---
> >>  .../bindings/display/bridge/ti,tdp158.yaml         | 48 ++++++++++++++++++++++
> >>  1 file changed, 48 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >> new file mode 100644
> >> index 0000000000000..b687699e2ba80
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> >> @@ -0,0 +1,48 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: TI TDP158 HDMI to TMDS Redriver
> >> +
> >> +maintainers:
> >> +  - Arnaud Vrac <avrac@freebox.fr>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: ti,tdp158
> >> +
> >> +  reg:
> >> +    description: I2C address of the device
> >> +
> >> +  enable-gpios:
> >> +    description: GPIO controlling bridge enable
> >> +
> >> +  vcc-supply:
> >> +    description: Power supply 3.3V
> >> +
> >> +  vdd-supply:
> >> +    description: Power supply 1.1V
> > 
> > Are these supplies not also required? Surely the device needs the power
> > to function?
> 
> Maybe if the hamsters spin fast enough in their wheels,
> these supplies won't be required? :)
> 
> The reason I hesitated to mark them as required,
> is because the HW engineer told us that on our board
> they were connected to a power line that is shared
> between several functional blocks.
> 
> I suppose that's not a reason?

Then all of those blocks should have their supplies described in the
devicetree! FWIW, if you don't put them in your dts, you'll get
validation failures but the regulator core will produce dummy regulators
so your driver should "just workTM". I'd suggest that you add the
supplies though to these other functional blocks so that the OS can
manage them properly.

> Required means "device doesn't work if they're not connected" ?

Correct.

Thanks,
Conor.


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

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

[Maxime Ripard]

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

On Tue, Jun 25, 2024 at 06:38:12PM GMT, Marc Gonzalez wrote:
> The TI TDP158 is an HDMI to TMDS Redriver.
> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>

Your commit log needs some work.

In particular, that device is more complex than what you're saying, and
explaining the full capabilities of the device will allow people to help
you create bindings that will be able to exploit all those capabilities
without breaking the backward compatibility.

Maxime

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

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

[Marc Gonzalez]

The TI TDP158 is an AC-Coupled HDMI signal to TMDS Redriver supporting
DVI 1.0 and HDMI 1.4b and 2.0b output signals.

The default settings work fine for our use-case.

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

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

-- 
2.34.1

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

[Dmitry Baryshkov]

On Tue, Jun 25, 2024 at 06:38:13PM GMT, Marc Gonzalez wrote:
> The TI TDP158 is an AC-Coupled HDMI signal to TMDS Redriver supporting
> DVI 1.0 and HDMI 1.4b and 2.0b output signals.
> 
> The default settings work fine for our use-case.
> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  drivers/gpu/drm/bridge/Kconfig     |   6 +++
>  drivers/gpu/drm/bridge/Makefile    |   1 +
>  drivers/gpu/drm/bridge/ti-tdp158.c | 103 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 110 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index c621be1a99a89..0859f85cb4b69 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -368,6 +368,12 @@ config DRM_TI_DLPC3433
>  	  It supports up to 720p resolution with 60 and 120 Hz refresh
>  	  rates.
>  
> +config DRM_TI_TDP158
> +	tristate "TI TDP158 HDMI/TMDS bridge"
> +	depends on OF
> +	help
> +	  Texas Instruments TDP158 HDMI/TMDS Bridge driver

Please run ./scripts/checkpatch.pl on your patch.

> +
>  config DRM_TI_TFP410
>  	tristate "TI TFP410 DVI/HDMI bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 7df87b582dca3..3daf803ce80b6 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>  obj-$(CONFIG_DRM_TI_DLPC3433) += ti-dlpc3433.o
>  obj-$(CONFIG_DRM_TI_SN65DSI83) += ti-sn65dsi83.o
>  obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
> +obj-$(CONFIG_DRM_TI_TDP158) += ti-tdp158.o
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>  obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
>  obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
> diff --git a/drivers/gpu/drm/bridge/ti-tdp158.c b/drivers/gpu/drm/bridge/ti-tdp158.c
> new file mode 100644
> index 0000000000000..b65132e3598fc
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-tdp158.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Freebox SAS
> + */
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <linux/i2c.h>
> +
> +struct tdp158 {
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +	struct gpio_desc *enable; // Operation Enable - pin 36
> +	struct regulator *vcc; // 3.3V
> +	struct regulator *vdd; // 1.1V
> +};
> +
> +static void tdp158_enable(struct drm_bridge *bridge, struct drm_bridge_state *prev)
> +{
> +	int err;
> +	struct tdp158 *tdp158 = bridge->driver_private;
> +
> +	if ((err = regulator_enable(tdp158->vcc)))
> +		pr_err("%s: vcc: %d", __func__, err);

- dev_err
- please expand error messages
- ERROR: do not use assignment in if condition

> +
> +	if ((err = regulator_enable(tdp158->vdd)))
> +		pr_err("%s: vdd: %d", __func__, err);
> +
> +	gpiod_set_value_cansleep(tdp158->enable, 1);
> +}
> +
> +static void tdp158_disable(struct drm_bridge *bridge, struct drm_bridge_state *prev)
> +{
> +	struct tdp158 *tdp158 = bridge->driver_private;
> +
> +	gpiod_set_value_cansleep(tdp158->enable, 0);
> +	regulator_disable(tdp158->vdd);
> +	regulator_disable(tdp158->vcc);
> +}
> +
> +static int tdp158_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
> +{
> +	struct tdp158 *tdp158 = bridge->driver_private;

empty line

> +	return drm_bridge_attach(bridge->encoder, tdp158->next, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);

No. Pass flags directly.

> +}
> +
> +static const struct drm_bridge_funcs tdp158_bridge_funcs = {
> +	.attach = tdp158_attach,
> +	.atomic_enable = tdp158_enable,
> +	.atomic_disable = tdp158_disable,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
> +};
> +
> +static int tdp158_bridge_probe(struct i2c_client *client)
> +{
> +	struct tdp158 *tdp158;
> +	struct device *dev = &client->dev;
> +
> +	tdp158 = devm_kzalloc(dev, sizeof(*tdp158), GFP_KERNEL);
> +	if (!tdp158)
> +		return -ENOMEM;
> +
> +	tdp158->next = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);

Missing `select DRM_PANEL_BRIDGE`

> +	if (IS_ERR(tdp158->next))
> +		return dev_err_probe(dev, PTR_ERR(tdp158->next), "next");

This results in a cryptic message 'foo: ESOMETHING: next'. Please
expand.

> +
> +	tdp158->vcc = devm_regulator_get(dev, "vcc");
> +	if (IS_ERR(tdp158->vcc))
> +		return dev_err_probe(dev, PTR_ERR(tdp158->vcc), "vcc");
> +
> +	tdp158->vdd = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(tdp158->vdd))
> +		return dev_err_probe(dev, PTR_ERR(tdp158->vdd), "vdd");
> +
> +	tdp158->enable = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(tdp158->enable))
> +		return dev_err_probe(dev, PTR_ERR(tdp158->enable), "enable");
> +
> +	tdp158->bridge.of_node = dev->of_node;
> +	tdp158->bridge.funcs = &tdp158_bridge_funcs;
> +	tdp158->bridge.driver_private = tdp158;
> +
> +	return devm_drm_bridge_add(dev, &tdp158->bridge);
> +}
> +
> +static const struct of_device_id tdp158_bridge_match_table[] = {
> +	{ .compatible = "ti,tdp158" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tdp158_bridge_match_table);
> +
> +static struct i2c_driver tdp158_bridge_driver = {
> +	.probe = tdp158_bridge_probe,
> +	.driver = {
> +		.name = "tdp158-bridge",
> +		.of_match_table = tdp158_bridge_match_table,
> +	},
> +};
> +module_i2c_driver(tdp158_bridge_driver);
> +
> +MODULE_DESCRIPTION("TI TDP158 driver");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

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

[Marc Gonzalez]

On 26/06/2024 06:47, Dmitry Baryshkov wrote:

> On Tue, Jun 25, 2024 at 06:38:13PM GMT, Marc Gonzalez wrote:
>
>> The TI TDP158 is an AC-Coupled HDMI signal to TMDS Redriver supporting
>> DVI 1.0 and HDMI 1.4b and 2.0b output signals.
>>
>> The default settings work fine for our use-case.
>>
>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig     |   6 +++
>>  drivers/gpu/drm/bridge/Makefile    |   1 +
>>  drivers/gpu/drm/bridge/ti-tdp158.c | 103 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 110 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index c621be1a99a89..0859f85cb4b69 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -368,6 +368,12 @@ config DRM_TI_DLPC3433
>>  	  It supports up to 720p resolution with 60 and 120 Hz refresh
>>  	  rates.
>>  
>> +config DRM_TI_TDP158
>> +	tristate "TI TDP158 HDMI/TMDS bridge"
>> +	depends on OF
>> +	help
>> +	  Texas Instruments TDP158 HDMI/TMDS Bridge driver
> 
> Please run ./scripts/checkpatch.pl on your patch.

Oops, sorry, will do.
For the record, I did run:
$ make -j16 dt_binding_check DT_SCHEMA_FILES=ti,tdp158.yaml


>> +	if ((err = regulator_enable(tdp158->vcc)))
>> +		pr_err("%s: vcc: %d", __func__, err);
> 
> - dev_err
> - please expand error messages
> - ERROR: do not use assignment in if condition

simple-bridge.c uses DRM_ERROR, but it says:
"NOTE: this is deprecated in favor of pr_err()"
Hence, I used pr_err.
Are you saying I need to record the dev,
just to be able to call dev_err?


> empty line

Will do.

>> +	return drm_bridge_attach(bridge->encoder, tdp158->next, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> 
> No. Pass flags directly.

Oh, just pass the flags argument here? OK.


>> +	tdp158->next = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> 
> Missing `select DRM_PANEL_BRIDGE`

OK.

>> +	if (IS_ERR(tdp158->next))
>> +		return dev_err_probe(dev, PTR_ERR(tdp158->next), "next");
> 
> This results in a cryptic message 'foo: ESOMETHING: next'. Please
> expand.

OK.

Thanks for the in-depth review & suggestions.
Will respin with fixes.

Regards

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

[Dmitry Baryshkov]

On Wed, Jun 26, 2024 at 05:26:10PM GMT, Marc Gonzalez wrote:
> On 26/06/2024 06:47, Dmitry Baryshkov wrote:
> 
> > On Tue, Jun 25, 2024 at 06:38:13PM GMT, Marc Gonzalez wrote:
> >
> >> The TI TDP158 is an AC-Coupled HDMI signal to TMDS Redriver supporting
> >> DVI 1.0 and HDMI 1.4b and 2.0b output signals.
> >>
> >> The default settings work fine for our use-case.
> >>
> >> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> >> ---
> >>  drivers/gpu/drm/bridge/Kconfig     |   6 +++
> >>  drivers/gpu/drm/bridge/Makefile    |   1 +
> >>  drivers/gpu/drm/bridge/ti-tdp158.c | 103 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 110 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> >> index c621be1a99a89..0859f85cb4b69 100644
> >> --- a/drivers/gpu/drm/bridge/Kconfig
> >> +++ b/drivers/gpu/drm/bridge/Kconfig
> >> @@ -368,6 +368,12 @@ config DRM_TI_DLPC3433
> >>  	  It supports up to 720p resolution with 60 and 120 Hz refresh
> >>  	  rates.
> >>  
> >> +config DRM_TI_TDP158
> >> +	tristate "TI TDP158 HDMI/TMDS bridge"
> >> +	depends on OF
> >> +	help
> >> +	  Texas Instruments TDP158 HDMI/TMDS Bridge driver
> > 
> > Please run ./scripts/checkpatch.pl on your patch.
> 
> Oops, sorry, will do.
> For the record, I did run:
> $ make -j16 dt_binding_check DT_SCHEMA_FILES=ti,tdp158.yaml
> 
> 
> >> +	if ((err = regulator_enable(tdp158->vcc)))
> >> +		pr_err("%s: vcc: %d", __func__, err);
> > 
> > - dev_err
> > - please expand error messages
> > - ERROR: do not use assignment in if condition
> 
> simple-bridge.c uses DRM_ERROR, but it says:
> "NOTE: this is deprecated in favor of pr_err()"
> Hence, I used pr_err.
> Are you saying I need to record the dev,
> just to be able to call dev_err?

Yes, please. Otherwise it's just random 'foo: vcc: -1'. Note, that most
of the error-reporting codes doesn't use __func__.

> 
> 
> > empty line
> 
> Will do.
> 
> >> +	return drm_bridge_attach(bridge->encoder, tdp158->next, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > 
> > No. Pass flags directly.
> 
> Oh, just pass the flags argument here? OK.

Yes

> 
> 
> >> +	tdp158->next = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > 
> > Missing `select DRM_PANEL_BRIDGE`
> 
> OK.
> 
> >> +	if (IS_ERR(tdp158->next))
> >> +		return dev_err_probe(dev, PTR_ERR(tdp158->next), "next");
> > 
> > This results in a cryptic message 'foo: ESOMETHING: next'. Please
> > expand.
> 
> OK.
> 
> Thanks for the in-depth review & suggestions.
> Will respin with fixes.
> 
> Regards
> 

-- 
With best wishes
Dmitry