[PATCH 4/4] drm: bridge: simple-bridge: add tdp158 support

[Dmitry Baryshkov]

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 at freebox.fr>
> ---
>  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 at free-electrons.com>
>   */
>  
> +#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 at free-electrons.com>");
>  MODULE_DESCRIPTION("Simple DRM bridge driver");
>  MODULE_LICENSE("GPL");
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry