On Mon, Jun 17, 2024 at 06:03:02PM 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.
>
> Signed-off-by: Marc Gonzalez <mgonzalez@xxxxxxxxxx>
> ---
> drivers/gpu/drm/bridge/simple-bridge.c | 64 ++++++++++++++++++++++++++++++++--
> 1 file changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index f1e458a15882f..745d253e55f7e 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -6,6 +6,8 @@
> * Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> */
>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> #include <linux/module.h>
> #include <linux/of.h>
> @@ -32,6 +34,7 @@ struct simple_bridge {
> const struct simple_bridge_info *info;
>
> struct drm_bridge *next_bridge;
> + struct regulator *vcc;
> struct regulator *vdd;
> struct gpio_desc *enable;
> };
> @@ -142,8 +145,16 @@ static void simple_bridge_enable(struct drm_bridge *bridge)
> struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);
> int ret;
>
> + if (sbridge->vcc) {
> + ret = regulator_enable(sbridge->vcc);
> + msleep(100);
At least this should be documented or explained in the commit message.
Is it absolutely necessary? Can you use regulator-enable-ramp-delay or
any other DT property instead?
> + if (ret)
> + DRM_ERROR("Failed to enable vcc regulator: %d\n", ret);
> + }
> +
> if (sbridge->vdd) {
> ret = regulator_enable(sbridge->vdd);
> + msleep(100);
> if (ret)
> DRM_ERROR("Failed to enable vdd regulator: %d\n", ret);
> }
> @@ -159,6 +170,9 @@ static void simple_bridge_disable(struct drm_bridge *bridge)
>
> if (sbridge->vdd)
> regulator_disable(sbridge->vdd);
> +
> + if (sbridge->vcc)
> + regulator_disable(sbridge->vcc);
> }
>
> static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
> @@ -167,16 +181,14 @@ static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
> .disable = simple_bridge_disable,
> };
>
> -static int simple_bridge_probe(struct platform_device *pdev)
> +static int common_probe(struct device *dev, struct simple_bridge **res)
> {
> - struct device *dev = &pdev->dev;
> struct simple_bridge *sbridge;
> struct device_node *remote;
>
> sbridge = devm_kzalloc(dev, sizeof(*sbridge), GFP_KERNEL);
> if (!sbridge)
> return -ENOMEM;
> - platform_set_drvdata(pdev, sbridge);
I think this call can get dropped together with the remove() being
gone...
>
> sbridge->info = of_device_get_match_data(dev);
>
> @@ -203,6 +215,15 @@ static int simple_bridge_probe(struct platform_device *pdev)
> dev_dbg(dev, "No vdd regulator found: %d\n", ret);
> }
>
> + sbridge->vcc = devm_regulator_get_optional(dev, "vcc");
> + if (IS_ERR(sbridge->vcc)) {
> + int ret = PTR_ERR(sbridge->vcc);
> + if (ret == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + sbridge->vcc = NULL;
> + dev_dbg(dev, "No vcc regulator found: %d\n", ret);
> + }
> +
> sbridge->enable = devm_gpiod_get_optional(dev, "enable",
> GPIOD_OUT_LOW);
> if (IS_ERR(sbridge->enable))
> @@ -213,10 +234,27 @@ static int simple_bridge_probe(struct platform_device *pdev)
> sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
> sbridge->bridge.of_node = dev->of_node;
> sbridge->bridge.timings = sbridge->info->timings;
> + *res = sbridge;
>
> return devm_drm_bridge_add(dev, &sbridge->bridge);
> }
>
> +static int simple_bridge_probe(struct platform_device *pdev)
> +{
> + struct simple_bridge *sbridge = NULL;
> + int err = common_probe(&pdev->dev, &sbridge);
> + platform_set_drvdata(pdev, sbridge);
... so, this becomes unnecessary...
> + return err;
> +}
> +
> +static int i2c_probe(struct i2c_client *client)
> +{
> + struct simple_bridge *sbridge = NULL;
> + int err = common_probe(&client->dev, &sbridge);
> + i2c_set_clientdata(client, sbridge);
... and this too.
> + return err;
> +}
> +
> /*
> * We assume the ADV7123 DAC is the "default" for historical reasons
> * Information taken from the ADV7123 datasheet, revision D.
> @@ -298,6 +336,26 @@ static struct platform_driver simple_bridge_driver = {
> };
> module_platform_driver(simple_bridge_driver);
>
> +static const struct of_device_id i2c_match_table[] = {
> + {
> + .compatible = "ti,tdp158",
> + .data = &(const struct simple_bridge_info) {
> + .connector_type = DRM_MODE_CONNECTOR_HDMIA,
> + },
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, i2c_match_table);
> +
> +static struct i2c_driver i2c_simple_bridge_driver = {
> + .probe = i2c_probe,
i2c_simple_bridge_probe, or better simple_bridge_i2c_probe. Same applies
to to the driver name and i2c_driver struct name.
> + .driver = {
> + .name = "i2c-simple-bridge",
> + .of_match_table = i2c_match_table,
> + },
> +};
> +module_i2c_driver(i2c_simple_bridge_driver);
Does this work if the driver is built as a module?
> +
> MODULE_AUTHOR("Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>");
> MODULE_DESCRIPTION("Simple DRM bridge driver");
> MODULE_LICENSE("GPL");
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
