| 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 |
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
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
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");
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"); >
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.
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
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 --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, + }, }, {}, };