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

[Dmitry Baryshkov]

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");
> 

-- 
With best wishes
Dmitry