[PATCH 0/4] Basic support for TI TDP158

[PATCH 0/4] Basic support for TI TDP158

[Marc Gonzalez]

---
Marc Gonzalez (4):
      dt-bindings: display: simple-bridge: add ti,tdp158
      drm: bridge: simple-bridge: use dev pointer in probe
      drm: bridge: simple-bridge: use only devm* in probe
      drm: bridge: simple-bridge: add tdp158 support

 .../bindings/display/bridge/simple-bridge.yaml     |  4 +
 drivers/gpu/drm/bridge/simple-bridge.c             | 85 +++++++++++++++++-----
 2 files changed, 71 insertions(+), 18 deletions(-)
---
base-commit: 17b591a4a192a8a11faad30520b8f6a9137ac514
change-id: 20240617-tdp158-418200d6cc0b

Best regards,
-- 
Marc Gonzalez <mgonzalez@freebox.fr>

[PATCH 1/4] dt-bindings: display: simple-bridge: add ti,tdp158

[Marc Gonzalez]

In default mode, this device works transparently.

Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
 Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
index 43cf4df9811a5..5f0c9687538bf 100644
--- a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
@@ -31,6 +31,7 @@ properties:
           - ti,opa362
           - ti,ths8134
           - ti,ths8135
+          - ti,tdp158
 
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
@@ -52,6 +53,9 @@ properties:
     maxItems: 1
     description: GPIO controlling bridge enable
 
+  vcc-supply:
+    description: Power supply for the bridge
+
   vdd-supply:
     description: Power supply for the bridge
 

-- 
2.34.1

[PATCH 2/4] drm: bridge: simple-bridge: use dev pointer in probe

[Marc Gonzalez]

Prepare to factorize probe function.

Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
 drivers/gpu/drm/bridge/simple-bridge.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index 5813a2c4fc5ee..d672e34970e18 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -169,18 +169,19 @@ static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
 
 static int simple_bridge_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct simple_bridge *sbridge;
 	struct device_node *remote;
 
-	sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
+	sbridge = devm_kzalloc(dev, sizeof(*sbridge), GFP_KERNEL);
 	if (!sbridge)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, sbridge);
 
-	sbridge->info = of_device_get_match_data(&pdev->dev);
+	sbridge->info = of_device_get_match_data(dev);
 
 	/* Get the next bridge in the pipeline. */
-	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
+	remote = of_graph_get_remote_node(dev->of_node, 1, -1);
 	if (!remote)
 		return -EINVAL;
 
@@ -188,29 +189,29 @@ static int simple_bridge_probe(struct platform_device *pdev)
 	of_node_put(remote);
 
 	if (!sbridge->next_bridge) {
-		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
+		dev_dbg(dev, "Next bridge not found, deferring probe\n");
 		return -EPROBE_DEFER;
 	}
 
 	/* Get the regulator and GPIO resources. */
-	sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
+	sbridge->vdd = devm_regulator_get_optional(dev, "vdd");
 	if (IS_ERR(sbridge->vdd)) {
 		int ret = PTR_ERR(sbridge->vdd);
 		if (ret == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 		sbridge->vdd = NULL;
-		dev_dbg(&pdev->dev, "No vdd regulator found: %d\n", ret);
+		dev_dbg(dev, "No vdd regulator found: %d\n", ret);
 	}
 
-	sbridge->enable = devm_gpiod_get_optional(&pdev->dev, "enable",
+	sbridge->enable = devm_gpiod_get_optional(dev, "enable",
 						  GPIOD_OUT_LOW);
 	if (IS_ERR(sbridge->enable))
-		return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->enable),
+		return dev_err_probe(dev, PTR_ERR(sbridge->enable),
 				     "Unable to retrieve enable GPIO\n");
 
 	/* Register the bridge. */
 	sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
-	sbridge->bridge.of_node = pdev->dev.of_node;
+	sbridge->bridge.of_node = dev->of_node;
 	sbridge->bridge.timings = sbridge->info->timings;
 
 	drm_bridge_add(&sbridge->bridge);

-- 
2.34.1

[PATCH 3/4] drm: bridge: simple-bridge: use only devm* in probe

[Marc Gonzalez]

Once probe uses only devm functions, remove() becomes unnecessary.

Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
 drivers/gpu/drm/bridge/simple-bridge.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index d672e34970e18..f1e458a15882f 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -214,16 +214,7 @@ static int simple_bridge_probe(struct platform_device *pdev)
 	sbridge->bridge.of_node = dev->of_node;
 	sbridge->bridge.timings = sbridge->info->timings;
 
-	drm_bridge_add(&sbridge->bridge);
-
-	return 0;
-}
-
-static void simple_bridge_remove(struct platform_device *pdev)
-{
-	struct simple_bridge *sbridge = platform_get_drvdata(pdev);
-
-	drm_bridge_remove(&sbridge->bridge);
+	return devm_drm_bridge_add(dev, &sbridge->bridge);
 }
 
 /*
@@ -300,7 +291,6 @@ MODULE_DEVICE_TABLE(of, simple_bridge_match);
 
 static struct platform_driver simple_bridge_driver = {
 	.probe	= simple_bridge_probe,
-	.remove_new = simple_bridge_remove,
 	.driver		= {
 		.name		= "simple-bridge",
 		.of_match_table	= simple_bridge_match,

-- 
2.34.1

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

[Marc Gonzalez]

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@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@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);
+		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);
 
 	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);
+	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);
+	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,
+	.driver = {
+		.name = "i2c-simple-bridge",
+		.of_match_table = i2c_match_table,
+	},
+};
+module_i2c_driver(i2c_simple_bridge_driver);
+
 MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
 MODULE_DESCRIPTION("Simple DRM bridge driver");
 MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH 1/4] dt-bindings: display: simple-bridge: add ti,tdp158

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Mon, Jun 17, 2024 at 06:02:59PM +0200, Marc Gonzalez wrote:
> In default mode, this device works transparently.

Please explain what makes this device incompatible with the existing
ones. For example, why not make the new compatible fall back to
ti,ths8134?

> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> index 43cf4df9811a5..5f0c9687538bf 100644
> --- a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> @@ -31,6 +31,7 @@ properties:
>            - ti,opa362
>            - ti,ths8134
>            - ti,ths8135
> +          - ti,tdp158
>  
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
> @@ -52,6 +53,9 @@ properties:
>      maxItems: 1
>      description: GPIO controlling bridge enable
>  
> +  vcc-supply:
> +    description: Power supply for the bridge
> +
>    vdd-supply:
>      description: Power supply for the bridge
>  
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] dt-bindings: display: simple-bridge: add ti,tdp158

[Dmitry Baryshkov]

On Mon, Jun 17, 2024 at 06:02:59PM GMT, Marc Gonzalez wrote:
> In default mode, this device works transparently.
> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> index 43cf4df9811a5..5f0c9687538bf 100644
> --- a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> @@ -31,6 +31,7 @@ properties:
>            - ti,opa362
>            - ti,ths8134
>            - ti,ths8135
> +          - ti,tdp158
>  
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
> @@ -52,6 +53,9 @@ properties:
>      maxItems: 1
>      description: GPIO controlling bridge enable
>  
> +  vcc-supply:
> +    description: Power supply for the bridge
> +
>    vdd-supply:
>      description: Power supply for the bridge

I'd suggest having a separate _bindings_ file. This way you can point
out that it's an I2C device sitting on the I2C bus. And once somebody
has to extend the bindings to support 'smarter' programming of this chip
they can edit just that file. It's still fine to use the simple-bridge
driver for the device defined in that bindings file.

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm: bridge: simple-bridge: use dev pointer in probe

[Dmitry Baryshkov]

On Mon, Jun 17, 2024 at 06:03:00PM GMT, Marc Gonzalez wrote:
> Prepare to factorize probe function.

what and why?

The patch itself LGTM

> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 


-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm: bridge: simple-bridge: use only devm* in probe

[Dmitry Baryshkov]

On Mon, Jun 17, 2024 at 06:03:01PM GMT, Marc Gonzalez wrote:
> Once probe uses only devm functions, remove() becomes unnecessary.

Breves vibrantesque sententiae

With the hope of getting an expanded commit message:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index d672e34970e18..f1e458a15882f 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -214,16 +214,7 @@ static int simple_bridge_probe(struct platform_device *pdev)
>  	sbridge->bridge.of_node = dev->of_node;
>  	sbridge->bridge.timings = sbridge->info->timings;
>  
> -	drm_bridge_add(&sbridge->bridge);
> -
> -	return 0;
> -}
> -
> -static void simple_bridge_remove(struct platform_device *pdev)
> -{
> -	struct simple_bridge *sbridge = platform_get_drvdata(pdev);
> -
> -	drm_bridge_remove(&sbridge->bridge);
> +	return devm_drm_bridge_add(dev, &sbridge->bridge);
>  }
>  
>  /*
> @@ -300,7 +291,6 @@ MODULE_DEVICE_TABLE(of, simple_bridge_match);
>  
>  static struct platform_driver simple_bridge_driver = {
>  	.probe	= simple_bridge_probe,
> -	.remove_new = simple_bridge_remove,
>  	.driver		= {
>  		.name		= "simple-bridge",
>  		.of_match_table	= simple_bridge_match,
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [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@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@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@free-electrons.com>");
>  MODULE_DESCRIPTION("Simple DRM bridge driver");
>  MODULE_LICENSE("GPL");
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] dt-bindings: display: simple-bridge: add ti,tdp158

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Mon, Jun 17, 2024 at 10:37:18PM GMT, Dmitry Baryshkov wrote:
> On Mon, Jun 17, 2024 at 06:02:59PM GMT, Marc Gonzalez wrote:
> > In default mode, this device works transparently.
> > 
> > Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> > ---
> >  Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> > index 43cf4df9811a5..5f0c9687538bf 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> > @@ -31,6 +31,7 @@ properties:
> >            - ti,opa362
> >            - ti,ths8134
> >            - ti,ths8135
> > +          - ti,tdp158
> >  
> >    ports:
> >      $ref: /schemas/graph.yaml#/properties/ports
> > @@ -52,6 +53,9 @@ properties:
> >      maxItems: 1
> >      description: GPIO controlling bridge enable
> >  
> > +  vcc-supply:
> > +    description: Power supply for the bridge
> > +
> >    vdd-supply:
> >      description: Power supply for the bridge
> 
> I'd suggest having a separate _bindings_ file. This way you can point
> out that it's an I2C device sitting on the I2C bus. And once somebody
> has to extend the bindings to support 'smarter' programming of this chip
> they can edit just that file. It's still fine to use the simple-bridge
> driver for the device defined in that bindings file.

Yeah, we want a separate binding for that one, that, just like we
discussed in v1, can be extended to support the other setups this bridge
can be used for.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] drm: bridge: simple-bridge: use dev pointer in probe

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

On Mon, Jun 17, 2024 at 06:03:00PM GMT, Marc Gonzalez wrote:
> Prepare to factorize probe function.
> 
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>

... And since we would need a separate binding file, we probably don't
want to add things to simple-bridge that would not be part of its
binding.

Even more so when bridge drivers are that simple to write.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/4] drm: bridge: simple-bridge: use only devm* in probe

[Marc Gonzalez]

On 18/06/2024 00:28, Dmitry Baryshkov wrote:

> On Mon, Jun 17, 2024 at 06:03:01PM GMT, Marc Gonzalez wrote:
>
>> Once probe uses only devm functions, remove() becomes unnecessary.
> 
> Breves vibrantesque sententiae
> 
> With the hope of getting an expanded commit message:
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I'm not quite sure what else to say.

Using only devm* functions in probe, the remove() callback is
no longer needed, since devm unwind will free all resources.

Is that better?

NB: this patch is not *required* but I thought "might as well
change it while I'm in the code".

Regards

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

[Marc Gonzalez]

On 18/06/2024 00:33, Dmitry Baryshkov wrote:

> On Mon, Jun 17, 2024 at 06:03:02PM GMT, Marc Gonzalez wrote:
> 
>> +	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?

The value comes from datasheet "8.3.2 Operation Timing"
Table 1. Power Up and Operation Timing Requirements
VDD supply ramp up requirements, max = 100 ms
VCC supply ramp up requirements, max = 100 ms

Did I read the spec wrong? (Very possible)

Are you saying this could/should be a property of the regulator?
What if the regulator gates several different blocks?


>>  	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...

Oooh, it didn't occur to me that the only reason to store drvdata was
to have it available in the remove callback...


> Does this work if the driver is built as a module?

Not sure there's any point in testing since Maxime NACKed the approach.

Regards

Re: [PATCH 0/4] Basic support for TI TDP158

[Marc Gonzalez]

On 17/06/2024 18:02, Marc Gonzalez wrote:

> Marc Gonzalez (4):
>       dt-bindings: display: simple-bridge: add ti,tdp158
>       drm: bridge: simple-bridge: use dev pointer in probe
>       drm: bridge: simple-bridge: use only devm* in probe
>       drm: bridge: simple-bridge: add tdp158 support
> 
>  .../bindings/display/bridge/simple-bridge.yaml     |  4 +
>  drivers/gpu/drm/bridge/simple-bridge.c             | 85 +++++++++++++++++-----
>  2 files changed, 71 insertions(+), 18 deletions(-)

Series has been NACKed.

Can be marked as "Rejected" in patchwork.

Please ignore.

Re: [PATCH 0/4] Basic support for TI TDP158

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Tue, Jun 18, 2024 at 03:07:16PM GMT, Marc Gonzalez wrote:
> On 17/06/2024 18:02, Marc Gonzalez wrote:
> 
> > Marc Gonzalez (4):
> >       dt-bindings: display: simple-bridge: add ti,tdp158
> >       drm: bridge: simple-bridge: use dev pointer in probe
> >       drm: bridge: simple-bridge: use only devm* in probe
> >       drm: bridge: simple-bridge: add tdp158 support
> > 
> >  .../bindings/display/bridge/simple-bridge.yaml     |  4 +
> >  drivers/gpu/drm/bridge/simple-bridge.c             | 85 +++++++++++++++++-----
> >  2 files changed, 71 insertions(+), 18 deletions(-)
> 
> Series has been NACKed.

That's a gross misrepresentation. It wasn't NAK'd, changes were requested.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/4] Basic support for TI TDP158

[Marc Gonzalez]

On 18/06/2024 16:36, Maxime Ripard wrote:

> On Tue, Jun 18, 2024 at 03:07:16PM GMT, Marc Gonzalez wrote:
>
>> On 17/06/2024 18:02, Marc Gonzalez wrote:
>>
>>> Marc Gonzalez (4):
>>>       dt-bindings: display: simple-bridge: add ti,tdp158
>>>       drm: bridge: simple-bridge: use dev pointer in probe
>>>       drm: bridge: simple-bridge: use only devm* in probe
>>>       drm: bridge: simple-bridge: add tdp158 support
>>>
>>>  .../bindings/display/bridge/simple-bridge.yaml     |  4 +
>>>  drivers/gpu/drm/bridge/simple-bridge.c             | 85 +++++++++++++++++-----
>>>  2 files changed, 71 insertions(+), 18 deletions(-)
>>
>> Series has been NACKed.
> 
> That's a gross misrepresentation. It wasn't NAK'd, changes were requested.

OK, my bad.

Changes have been requested.

This series is obsolete.

Re: [PATCH 3/4] drm: bridge: simple-bridge: use only devm* in probe

[Dmitry Baryshkov]

On Tue, Jun 18, 2024 at 01:37:15PM GMT, Marc Gonzalez wrote:
> On 18/06/2024 00:28, Dmitry Baryshkov wrote:
> 
> > On Mon, Jun 17, 2024 at 06:03:01PM GMT, Marc Gonzalez wrote:
> >
> >> Once probe uses only devm functions, remove() becomes unnecessary.
> > 
> > Breves vibrantesque sententiae
> > 
> > With the hope of getting an expanded commit message:
> > 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I'm not quite sure what else to say.
> 
> Using only devm* functions in probe, the remove() callback is
> no longer needed, since devm unwind will free all resources.
> 
> Is that better?

Yes. Or something like:

The only call in remove() is drm_bridge_remove(). Use devm_bridge_add(),
removing the need to call drm_bridge_remove() manually and drop the
driver's remove callback completely.

> 
> NB: this patch is not *required* but I thought "might as well
> change it while I'm in the code".
> 
> Regards
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Tue, Jun 18, 2024 at 01:48:48PM GMT, Marc Gonzalez wrote:
> On 18/06/2024 00:33, Dmitry Baryshkov wrote:
> 
> > On Mon, Jun 17, 2024 at 06:03:02PM GMT, Marc Gonzalez wrote:
> > 
> >> +	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?
> 
> The value comes from datasheet "8.3.2 Operation Timing"
> Table 1. Power Up and Operation Timing Requirements
> VDD supply ramp up requirements, max = 100 ms
> VCC supply ramp up requirements, max = 100 ms
> 
> Did I read the spec wrong? (Very possible)

I didn't check the spec. I was pointing that that you were adding
msleeps() into a generic path, but the commit message had no explanation
for that.

> 
> Are you saying this could/should be a property of the regulator?
> What if the regulator gates several different blocks?

I agree here. Yes, it should be done in the driver.

> 
> 
> >>  	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...
> 
> Oooh, it didn't occur to me that the only reason to store drvdata was
> to have it available in the remove callback...
> 
> 
> > Does this work if the driver is built as a module?
> 
> Not sure there's any point in testing since Maxime NACKed the approach.

Yep :-(

> 
> Regards
> 

-- 
With best wishes
Dmitry