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
