diff mbox series

[v2] drm/bridge: simple-bridge: Add support for TI TDP158

Message ID b41f2f86-0d99-4199-92a9-42cbb9d6a6d5@freebox.fr (mailing list archive)
State New, archived
Headers show
Series [v2] drm/bridge: simple-bridge: Add support for TI TDP158 | expand

Commit Message

Marc Gonzalez May 27, 2024, 4:06 p.m. UTC
From: Arnaud Vrac <avrac@freebox.fr>

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.

Since it's an I2C-programmable bridge, it could have a proper driver,
but the default settings work fine, thus simple bridge is sufficient.

Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
Change in v2: send from correct address
---
 drivers/gpu/drm/bridge/simple-bridge.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Dmitry Baryshkov May 28, 2024, 1:13 a.m. UTC | #1
On Mon, May 27, 2024 at 06:06:05PM +0200, Marc Gonzalez wrote:
> From: Arnaud Vrac <avrac@freebox.fr>
> 
> 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.
> 
> Since it's an I2C-programmable bridge, it could have a proper driver,
> but the default settings work fine, thus simple bridge is sufficient.
> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
> Change in v2: send from correct address
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index 5813a2c4fc5ee..b138279864750 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -292,6 +292,11 @@ static const struct of_device_id simple_bridge_match[] = {
>  			.timings = &ti_ths8134_bridge_timings,
>  			.connector_type = DRM_MODE_CONNECTOR_VGA,
>  		},
> +	}, {
> +		.compatible = "ti,tdp158",
> +		.data = &(const struct simple_bridge_info) {
> +			.connector_type = DRM_MODE_CONNECTOR_HDMIA,

Bindings please. Also, note that per the datasheet the bridge uses two
supplies, Vcc for 3.3V and Vdd for 1.1V, so it doesn't fully fit the
simple-bridge.c (which might need to be adjusted for the second supply).
Chapter 7.3.2 of the datasheet points out that Vcc should be brought up
before Vdd.

> +		},
>  	},
>  	{},
>  };
> -- 
> 2.34.1
Arnaud Vrac May 28, 2024, 8:02 a.m. UTC | #2
On 28/05/2024 03:13, Dmitry Baryshkov wrote:
> On Mon, May 27, 2024 at 06:06:05PM +0200, Marc Gonzalez wrote:
>> From: Arnaud Vrac <avrac@freebox.fr>
>>
>> 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.
>>
>> Since it's an I2C-programmable bridge, it could have a proper driver,
>> but the default settings work fine, thus simple bridge is sufficient.
>>
>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>> ---
>> Change in v2: send from correct address
>> ---
>>   drivers/gpu/drm/bridge/simple-bridge.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
>> index 5813a2c4fc5ee..b138279864750 100644
>> --- a/drivers/gpu/drm/bridge/simple-bridge.c
>> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
>> @@ -292,6 +292,11 @@ static const struct of_device_id simple_bridge_match[] = {
>>   			.timings = &ti_ths8134_bridge_timings,
>>   			.connector_type = DRM_MODE_CONNECTOR_VGA,
>>   		},
>> +	}, {
>> +		.compatible = "ti,tdp158",
>> +		.data = &(const struct simple_bridge_info) {
>> +			.connector_type = DRM_MODE_CONNECTOR_HDMIA,
> 
> Bindings please. Also, note that per the datasheet the bridge uses two
> supplies, Vcc for 3.3V and Vdd for 1.1V, so it doesn't fully fit the
> simple-bridge.c (which might need to be adjusted for the second supply).
> Chapter 7.3.2 of the datasheet points out that Vcc should be brought up
> before Vdd.

Good point, on our board Vcc/Vdd are always on so I didn't catch that.

Thanks,
-Arnaud
Marc Gonzalez June 13, 2024, 2:12 a.m. UTC | #3
On 28/05/2024 03:13, Dmitry Baryshkov wrote:

> Bindings please. Also, note that per the datasheet the bridge uses two
> supplies, Vcc for 3.3V and Vdd for 1.1V, so it doesn't fully fit the
> simple-bridge.c (which might need to be adjusted for the second supply).
> Chapter 7.3.2 of the datasheet points out that Vcc should be brought up
> before Vdd.

Is something simple like below acceptable?


// SPDX-License-Identifier: GPL-2.0-only
/*
 * Copyright 2024 Freebox SAS
 */

#include <drm/drm_bridge.h>
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/delay.h>

struct tdp158 {
	struct i2c_client *client;
	struct gpio_desc *OE; // Operation Enable
	struct drm_bridge bridge;
};

static int tdp158_probe(struct i2c_client *client)
{
	struct device *dev = &client->dev;
	struct tdp158 *tdp158;
	int err;

	tdp158 = devm_kzalloc(dev, sizeof(*tdp158), GFP_KERNEL);
	if (!tdp158)
		return -ENOMEM;

	tdp158->client = client;
	i2c_set_clientdata(client, tdp158);

	err = devm_regulator_get_enable(dev, "Vcc"); // 3.3V
	msleep(100);
	if (err)
		return dev_err_probe(dev, err, "Vcc");

	err = devm_regulator_get_enable(dev, "Vdd"); // 1.1V
	msleep(100);
	if (err)
		return dev_err_probe(dev, err, "Vdd");

	tdp158->OE = devm_gpiod_get(dev, "OE", GPIOD_OUT_LOW);
	if (IS_ERR(tdp158->OE))
		return dev_err_probe(dev, PTR_ERR(tdp158->OE), "OE pin");

	gpiod_set_value_cansleep(tdp158->OE, 1);

	tdp158->bridge.of_node = dev->of_node;

	return devm_drm_bridge_add(dev, &tdp158->bridge);
}

static const struct of_device_id tdp158_match_table[] = {
	{ .compatible = "ti,tdp158" },
	{ }
};
MODULE_DEVICE_TABLE(of, tdp158_match_table);

static struct i2c_driver tdp158_driver = {
	.probe = tdp158_probe,
	.driver = {
		.name = "tdp158",
		.of_match_table = tdp158_match_table,
	},
};

module_i2c_driver(tdp158_driver);

MODULE_DESCRIPTION("TI TDP158 driver");
MODULE_LICENSE("GPL");
Dmitry Baryshkov June 13, 2024, 10:25 a.m. UTC | #4
On Thu, Jun 13, 2024 at 04:12:22AM +0200, Marc Gonzalez wrote:
> On 28/05/2024 03:13, Dmitry Baryshkov wrote:
> 
> > Bindings please. Also, note that per the datasheet the bridge uses two
> > supplies, Vcc for 3.3V and Vdd for 1.1V, so it doesn't fully fit the
> > simple-bridge.c (which might need to be adjusted for the second supply).
> > Chapter 7.3.2 of the datasheet points out that Vcc should be brought up
> > before Vdd.
> 
> Is something simple like below acceptable?

Well, the question was about bindings, not for the driver snippet.
Anyway:

> 	err = devm_regulator_get_enable(dev, "Vcc"); // 3.3V

Usually all regulators are lowercase. so "vcc"

Nit: I think enabling regulators should be deferred to the enable (or
pre_enable) time.

> 	msleep(100);
> 	if (err)
> 		return dev_err_probe(dev, err, "Vcc");
> 
> 	err = devm_regulator_get_enable(dev, "Vdd"); // 1.1V

And here.

> 	msleep(100);
> 	if (err)
> 		return dev_err_probe(dev, err, "Vdd");
> 
> 	tdp158->OE = devm_gpiod_get(dev, "OE", GPIOD_OUT_LOW);
> 	if (IS_ERR(tdp158->OE))
> 		return dev_err_probe(dev, PTR_ERR(tdp158->OE), "OE pin");

"enable"

> 	gpiod_set_value_cansleep(tdp158->OE, 1);

Again, set it at runtime, please.

> 
> 	tdp158->bridge.of_node = dev->of_node;
> 
> 	return devm_drm_bridge_add(dev, &tdp158->bridge);
> }
> 
> static const struct of_device_id tdp158_match_table[] = {
> 	{ .compatible = "ti,tdp158" },
> 	{ }
> };
> MODULE_DEVICE_TABLE(of, tdp158_match_table);
> 
> static struct i2c_driver tdp158_driver = {
> 	.probe = tdp158_probe,
> 	.driver = {
> 		.name = "tdp158",
> 		.of_match_table = tdp158_match_table,
> 	},
> };
> 
> module_i2c_driver(tdp158_driver);
> 
> MODULE_DESCRIPTION("TI TDP158 driver");
> MODULE_LICENSE("GPL");
>
Dmitry Baryshkov June 13, 2024, 10:26 a.m. UTC | #5
On Thu, Jun 13, 2024 at 04:12:22AM +0200, Marc Gonzalez wrote:
> On 28/05/2024 03:13, Dmitry Baryshkov wrote:
> 
> > Bindings please. Also, note that per the datasheet the bridge uses two
> > supplies, Vcc for 3.3V and Vdd for 1.1V, so it doesn't fully fit the
> > simple-bridge.c (which might need to be adjusted for the second supply).
> > Chapter 7.3.2 of the datasheet points out that Vcc should be brought up
> > before Vdd.
> 
> Is something simple like below acceptable?

Note, I'd really suggest extending simple-bridge.c instead to handle the
second regulator.
Marc Gonzalez June 13, 2024, 10:29 a.m. UTC | #6
On 13/06/2024 12:26, Dmitry Baryshkov wrote:

> On Thu, Jun 13, 2024 at 04:12:22AM +0200, Marc Gonzalez wrote:
>
>> On 28/05/2024 03:13, Dmitry Baryshkov wrote:
>>
>>> Bindings please. Also, note that per the datasheet the bridge uses two
>>> supplies, Vcc for 3.3V and Vdd for 1.1V, so it doesn't fully fit the
>>> simple-bridge.c (which might need to be adjusted for the second supply).
>>> Chapter 7.3.2 of the datasheet points out that Vcc should be brought up
>>> before Vdd.
>>
>> Is something simple like below acceptable?
> 
> Note, I'd really suggest extending simple-bridge.c instead to handle the
> second regulator.

I'm confused.

simple-bridge.c is not "I2C-aware" ?

Both you and Maxime mentioned there should be some I2C handling?

Regards
Dmitry Baryshkov June 13, 2024, 10:58 a.m. UTC | #7
On Thu, Jun 13, 2024 at 12:29:26PM +0200, Marc Gonzalez wrote:
> On 13/06/2024 12:26, Dmitry Baryshkov wrote:
> 
> > On Thu, Jun 13, 2024 at 04:12:22AM +0200, Marc Gonzalez wrote:
> >
> >> On 28/05/2024 03:13, Dmitry Baryshkov wrote:
> >>
> >>> Bindings please. Also, note that per the datasheet the bridge uses two
> >>> supplies, Vcc for 3.3V and Vdd for 1.1V, so it doesn't fully fit the
> >>> simple-bridge.c (which might need to be adjusted for the second supply).
> >>> Chapter 7.3.2 of the datasheet points out that Vcc should be brought up
> >>> before Vdd.
> >>
> >> Is something simple like below acceptable?
> > 
> > Note, I'd really suggest extending simple-bridge.c instead to handle the
> > second regulator.
> 
> I'm confused.
> 
> simple-bridge.c is not "I2C-aware" ?
> 
> Both you and Maxime mentioned there should be some I2C handling?

Ah, true. Well. you can add second driver to the simple-bridge.c, while
sharing all the internal data.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index 5813a2c4fc5ee..b138279864750 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -292,6 +292,11 @@  static const struct of_device_id simple_bridge_match[] = {
 			.timings = &ti_ths8134_bridge_timings,
 			.connector_type = DRM_MODE_CONNECTOR_VGA,
 		},
+	}, {
+		.compatible = "ti,tdp158",
+		.data = &(const struct simple_bridge_info) {
+			.connector_type = DRM_MODE_CONNECTOR_HDMIA,
+		},
 	},
 	{},
 };