[PATCH 0/4] DRM: BRIDGE: TFP410: Add i2c support

[PATCH 0/4] DRM: BRIDGE: TFP410: Add i2c support

[Jonathan Cormier]

The TFP410 driver does not support I2C.  As such, the device remains in
Power Down if the I2C is enabled by the bootstrap pins.

Add basic support for the I2C interface, and provide support to take
the device out of power down when enabled.  Also read the bootstrap mode
pins via the CTL_1_MODE register when using the I2C bus.

Also allow polling device to support hdmi/dvi hotplug detection.

To: Andrzej Hajda <andrzej.hajda@intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Robert Foss <robert.foss@linaro.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
To: Jonas Karlman <jonas@kwiboo.se>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: David Airlie <airlied@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Jyri Sarha <jsarha@ti.com>
Cc: dri-devel@lists.freedesktop.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Michael Williamson <michael.williamson@criticallink.com>
Cc: Bob Duke <bduke@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>

---
Jonathan Cormier (1):
      dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

Michael Williamson (3):
      DRM: BRIDGE: TFP410: Support basic I2C interface
      DRM: BRIDGE: TFP410: Fix logic to configured polled HPD
      DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.

 .../bindings/display/bridge/ti,tfp410.yaml         |  42 ++++++++
 drivers/gpu/drm/bridge/ti-tfp410.c                 | 110 +++++++++++++++------
 2 files changed, 124 insertions(+), 28 deletions(-)
---
base-commit: 93f875a8526a291005e7f38478079526c843cbec
change-id: 20230125-tfp410_i2c-3b270b0bf3e0

Best regards,
-- 
Jonathan Cormier <jcormier@criticallink.com>

[PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

[Jonathan Cormier]

Add a i2c example with HDMI connector

Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
---
 .../bindings/display/bridge/ti,tfp410.yaml         | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
index 4c5dd8ec2951..456214f14b47 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
@@ -116,4 +116,46 @@ examples:
         };
     };
 
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      hdmi_encoder: tfp410@38 {
+        compatible = "ti,tfp410";
+        reg = <0x38>;
+
+        ports {
+          address-cells = <1>;
+          size-cells = <0>;
+
+          port@0 {
+            reg = <0>;
+            tfp410_in: endpoint {
+              remote-endpoint = <&dpi1_out>;
+            };
+          };
+
+          port@1 {
+            reg = <1>;
+            tfp410_out: endpoint {
+              remote-endpoint = <&hdmi_connector_in>;
+            };
+          };
+        };
+      };
+    };
+
+    hdmi: hdmi_connector {
+      compatible = "hdmi-connector";
+      label = "hdmi";
+      type = "a";
+      ddc-i2c-bus = <&i2c1>;
+      port {
+        hdmi_connector_in: endpoint {
+          remote-endpoint = <&tfp410_out>;
+        };
+      };
+    };
+
 ...

-- 
2.25.1

[PATCH 2/4] DRM: BRIDGE: TFP410: Support basic I2C interface

[Jonathan Cormier]

From: Michael Williamson <michael.williamson@criticallink.com>

The TFP410 driver does not support I2C.  As such, the device remains in
Power Down if the I2C is enabled by the bootstrap pins.

Add basic support for the I2C interface, and provide support to take
the device out of power down when enabled.  Also read the bootstrap mode
pins via the CTL_1_MODE register when using the I2C bus.

Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 95 +++++++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index b9635abbad16..323a6d9ed188 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -6,6 +6,7 @@
 
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/regmap.h>
 #include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
@@ -21,6 +22,20 @@
 
 #define HOTPLUG_DEBOUNCE_MS		1100
 
+#define TFP410_REG_CTL_1_MODE	0x08
+#define TFP410_BIT_PD   BIT(0)
+#define TFP410_BIT_EDGE BIT(1)
+#define TFP410_BIT_BSEL BIT(2)
+#define TFP410_BIT_DSEL BIT(3)
+
+static const struct regmap_config tfp410_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = 0xff,
+	.cache_type = REGCACHE_NONE,
+};
+
 struct tfp410 {
 	struct drm_bridge	bridge;
 	struct drm_connector	connector;
@@ -33,6 +48,8 @@ struct tfp410 {
 	struct drm_bridge	*next_bridge;
 
 	struct device *dev;
+	struct i2c_client *i2c;
+	struct regmap *regmap;
 };
 
 static inline struct tfp410 *
@@ -183,6 +200,9 @@ static void tfp410_enable(struct drm_bridge *bridge)
 {
 	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
 
+	if (dvi->i2c)
+		regmap_set_bits(dvi->regmap, TFP410_REG_CTL_1_MODE, TFP410_BIT_PD);
+
 	gpiod_set_value_cansleep(dvi->powerdown, 0);
 }
 
@@ -190,6 +210,9 @@ static void tfp410_disable(struct drm_bridge *bridge)
 {
 	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
 
+	if (dvi->i2c)
+		regmap_clear_bits(dvi->regmap, TFP410_REG_CTL_1_MODE, TFP410_BIT_PD);
+
 	gpiod_set_value_cansleep(dvi->powerdown, 1);
 }
 
@@ -221,38 +244,48 @@ static const struct drm_bridge_timings tfp410_default_timings = {
 	.hold_time_ps = 1300,
 };
 
-static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
+static int tfp410_parse_timings(struct tfp410 *dvi)
 {
 	struct drm_bridge_timings *timings = &dvi->timings;
 	struct device_node *ep;
 	u32 pclk_sample = 0;
 	u32 bus_width = 24;
 	u32 deskew = 0;
+	unsigned int val = 0;
+	int ret = 0;
 
 	/* Start with defaults. */
 	*timings = tfp410_default_timings;
 
-	if (i2c)
+	if (dvi->i2c) {
 		/*
-		 * In I2C mode timings are configured through the I2C interface.
-		 * As the driver doesn't support I2C configuration yet, we just
-		 * go with the defaults (BSEL=1, DSEL=1, DKEN=0, EDGE=1).
+		 * For now, assume settings are latched from pins on reset / power up.
+		 * Should add options to optionally set them out of DT properties.
 		 */
-		return 0;
-
-	/*
-	 * In non-I2C mode, timings are configured through the BSEL, DSEL, DKEN
-	 * and EDGE pins. They are specified in DT through endpoint properties
-	 * and vendor-specific properties.
-	 */
-	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
-	if (!ep)
-		return -EINVAL;
-
-	/* Get the sampling edge from the endpoint. */
-	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
-	of_property_read_u32(ep, "bus-width", &bus_width);
-	of_node_put(ep);
+		ret = regmap_read(dvi->regmap, TFP410_REG_CTL_1_MODE, &val);
+		if (ret) {
+			dev_err(dvi->dev, "Read failed on CTL_1_MODE\n");
+			return ret;
+		}
+		pclk_sample = (val & TFP410_BIT_EDGE) ? 1 : 0;
+		bus_width = (val & TFP410_BIT_BSEL) ? 24 : 12;
+		dev_dbg(dvi->dev, "(0x%02X) : detected %d bus width, %s edge sampling\n",
+			val, bus_width, pclk_sample ? "positive" : "negative");
+	} else {
+		/*
+		 * In non-I2C mode, timings are configured through the BSEL, DSEL, DKEN
+		 * and EDGE pins. They are specified in DT through endpoint properties
+		 * and vendor-specific properties.
+		 */
+		ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
+		if (!ep)
+			return -EINVAL;
+
+		/* Get the sampling edge from the endpoint. */
+		of_property_read_u32(ep, "pclk-sample", &pclk_sample);
+		of_property_read_u32(ep, "bus-width", &bus_width);
+		of_node_put(ep);
+	}
 
 	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
 
@@ -291,7 +324,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
 	return 0;
 }
 
-static int tfp410_init(struct device *dev, bool i2c)
+static int tfp410_init(struct device *dev, struct i2c_client *i2c)
 {
 	struct device_node *node;
 	struct tfp410 *dvi;
@@ -313,15 +346,24 @@ static int tfp410_init(struct device *dev, bool i2c)
 	dvi->bridge.of_node = dev->of_node;
 	dvi->bridge.timings = &dvi->timings;
 	dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
+	dvi->i2c = i2c;
+
+	if (i2c) {
+		dvi->regmap = devm_regmap_init_i2c(i2c, &tfp410_regmap_config);
+		if (IS_ERR(dvi->regmap))
+			return PTR_ERR(dvi->regmap);
+	}
 
-	ret = tfp410_parse_timings(dvi, i2c);
+	ret = tfp410_parse_timings(dvi);
 	if (ret)
 		return ret;
 
 	/* Get the next bridge, connected to port@1. */
 	node = of_graph_get_remote_node(dev->of_node, 1, -1);
-	if (!node)
+	if (!node) {
+		dev_err(dev, "Could not find remote node\n");
 		return -ENODEV;
+	}
 
 	dvi->next_bridge = of_drm_find_bridge(node);
 	of_node_put(node);
@@ -352,7 +394,7 @@ static void tfp410_fini(struct device *dev)
 
 static int tfp410_probe(struct platform_device *pdev)
 {
-	return tfp410_init(&pdev->dev, false);
+	return tfp410_init(&pdev->dev, NULL);
 }
 
 static int tfp410_remove(struct platform_device *pdev)
@@ -378,7 +420,6 @@ static struct platform_driver tfp410_platform_driver = {
 };
 
 #if IS_ENABLED(CONFIG_I2C)
-/* There is currently no i2c functionality. */
 static int tfp410_i2c_probe(struct i2c_client *client,
 			    const struct i2c_device_id *id)
 {
@@ -391,7 +432,7 @@ static int tfp410_i2c_probe(struct i2c_client *client,
 		return -ENXIO;
 	}
 
-	return tfp410_init(&client->dev, true);
+	return tfp410_init(&client->dev, client);
 }
 
 static void tfp410_i2c_remove(struct i2c_client *client)
@@ -408,7 +449,7 @@ MODULE_DEVICE_TABLE(i2c, tfp410_i2c_ids);
 static struct i2c_driver tfp410_i2c_driver = {
 	.driver = {
 		.name	= "tfp410",
-		.of_match_table = of_match_ptr(tfp410_match),
+		.of_match_table = tfp410_match,
 	},
 	.id_table	= tfp410_i2c_ids,
 	.probe		= tfp410_i2c_probe,

-- 
2.25.1

[PATCH 3/4] DRM: BRIDGE: TFP410: Fix logic to configured polled HPD

[Jonathan Cormier]

From: Michael Williamson <michael.williamson@criticallink.com>

The logic to configure polling (vs async/irq notification) of hot-plug
events was not correct.  If the connected bridge requires polling,
then inform the upstream bridge we also require polling.

Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index 323a6d9ed188..837e1f81a0ff 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -155,7 +155,7 @@ static int tfp410_attach(struct drm_bridge *bridge,
 		return -ENODEV;
 	}
 
-	if (dvi->next_bridge->ops & DRM_BRIDGE_OP_DETECT)
+	if (dvi->next_bridge->ops & DRM_BRIDGE_OP_HPD)
 		dvi->connector.polled = DRM_CONNECTOR_POLL_HPD;
 	else
 		dvi->connector.polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;

-- 
2.25.1

[PATCH 4/4] DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.

[Jonathan Cormier]

From: Michael Williamson <michael.williamson@criticallink.com>

If the I2C bus is connected on the TFP410, then use the register
status bit to determine connection state.  This is needed, in particular,
for polling the state when the Hot Plug detect is not connected to
a controlling CPU via GPIO/IRQ lane.

Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index 837e1f81a0ff..ac216eaec3c8 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -28,6 +28,9 @@
 #define TFP410_BIT_BSEL BIT(2)
 #define TFP410_BIT_DSEL BIT(3)
 
+#define TFP410_REG_CTL_2_MODE	0x09
+#define TFP410_BIT_HTPLG BIT(1)
+
 static const struct regmap_config tfp410_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -105,6 +108,16 @@ static enum drm_connector_status
 tfp410_connector_detect(struct drm_connector *connector, bool force)
 {
 	struct tfp410 *dvi = drm_connector_to_tfp410(connector);
+	u32 val;
+	unsigned int ret;
+
+	if (dvi->i2c) {
+		ret = regmap_test_bits(dvi->regmap, TFP410_REG_CTL_2_MODE, TFP410_BIT_HTPLG);
+		if (ret < 0)
+			dev_err(dvi->dev, "%s failed to read HTPLG bit : %d\n", __func__, ret);
+		else
+			return ret ? connector_status_connected : connector_status_disconnected;
+	}
 
 	return drm_bridge_detect(dvi->next_bridge);
 }

-- 
2.25.1

Re: [PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

[Laurent Pinchart]

Hi Jonathan,

Thank you for the patch.

On Wed, Jan 25, 2023 at 04:09:09PM -0500, Jonathan Cormier wrote:
> Add a i2c example with HDMI connector
> 
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> ---
>  .../bindings/display/bridge/ti,tfp410.yaml         | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> index 4c5dd8ec2951..456214f14b47 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> @@ -116,4 +116,46 @@ examples:
>          };
>      };
>  
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;

Please use 4 spaces for indentation, as in the other example.

> +
> +      hdmi_encoder: tfp410@38 {
> +        compatible = "ti,tfp410";
> +        reg = <0x38>;
> +
> +        ports {
> +          address-cells = <1>;
> +          size-cells = <0>;
> +
> +          port@0 {
> +            reg = <0>;
> +            tfp410_in: endpoint {
> +              remote-endpoint = <&dpi1_out>;
> +            };
> +          };
> +
> +          port@1 {
> +            reg = <1>;
> +            tfp410_out: endpoint {
> +              remote-endpoint = <&hdmi_connector_in>;
> +            };
> +          };
> +        };
> +      };
> +    };
> +
> +    hdmi: hdmi_connector {
> +      compatible = "hdmi-connector";
> +      label = "hdmi";
> +      type = "a";
> +      ddc-i2c-bus = <&i2c1>;
> +      port {
> +        hdmi_connector_in: endpoint {
> +          remote-endpoint = <&tfp410_out>;
> +        };
> +      };
> +    };
> +

You can drop the hdmi connector, the example will still validate.

>  ...

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

[Jon Cormier]

On Wed, Jan 25, 2023 at 4:24 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jonathan,
>
> Thank you for the patch.
>
> On Wed, Jan 25, 2023 at 04:09:09PM -0500, Jonathan Cormier wrote:
> > Add a i2c example with HDMI connector
> >
> > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> > ---
> >  .../bindings/display/bridge/ti,tfp410.yaml         | 42 ++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> > index 4c5dd8ec2951..456214f14b47 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> > @@ -116,4 +116,46 @@ examples:
> >          };
> >      };
> >
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
>
> Please use 4 spaces for indentation, as in the other example.
Will do, the whole file is 2 space indents. I didn't notice the
examples switch to 4 spaces.
>
> > +
> > +      hdmi_encoder: tfp410@38 {
> > +        compatible = "ti,tfp410";
> > +        reg = <0x38>;
> > +
> > +        ports {
> > +          address-cells = <1>;
> > +          size-cells = <0>;
> > +
> > +          port@0 {
> > +            reg = <0>;
> > +            tfp410_in: endpoint {
> > +              remote-endpoint = <&dpi1_out>;
> > +            };
> > +          };
> > +
> > +          port@1 {
> > +            reg = <1>;
> > +            tfp410_out: endpoint {
> > +              remote-endpoint = <&hdmi_connector_in>;
> > +            };
> > +          };
> > +        };
> > +      };
> > +    };
> > +
> > +    hdmi: hdmi_connector {
> > +      compatible = "hdmi-connector";
> > +      label = "hdmi";
> > +      type = "a";
> > +      ddc-i2c-bus = <&i2c1>;
> > +      port {
> > +        hdmi_connector_in: endpoint {
> > +          remote-endpoint = <&tfp410_out>;
> > +        };
> > +      };
> > +    };
> > +
>
> You can drop the hdmi connector, the example will still validate.
Okay
>
> >  ...
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Jonathan Cormier
Software Engineer

Voice:  315.425.4045 x222



http://www.CriticalLink.com
6712 Brooklawn Parkway, Syracuse, NY 13211

Re: [PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

[Rob Herring]

On Wed, 25 Jan 2023 16:09:09 -0500, Jonathan Cormier wrote:
> Add a i2c example with HDMI connector
> 
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> ---
>  .../bindings/display/bridge/ti,tfp410.yaml         | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/display/bridge/ti,tfp410.example.dts:77.37-79.19: ERROR (duplicate_label): /example-1/i2c/tfp410@38/ports/port@0/endpoint: Duplicate label 'tfp410_in' on /example-1/i2c/tfp410@38/ports/port@0/endpoint and /example-0/encoder/ports/port@0/endpoint
Documentation/devicetree/bindings/display/bridge/ti,tfp410.example.dts:84.38-86.19: ERROR (duplicate_label): /example-1/i2c/tfp410@38/ports/port@1/endpoint: Duplicate label 'tfp410_out' on /example-1/i2c/tfp410@38/ports/port@1/endpoint and /example-0/encoder/ports/port@1/endpoint
ERROR: Input tree has errors, aborting (use -f to force output)
make[1]: *** [scripts/Makefile.lib:434: Documentation/devicetree/bindings/display/bridge/ti,tfp410.example.dtb] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230125-tfp410_i2c-v1-1-66a4d4e390b7@criticallink.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

[Krzysztof Kozlowski]

On 25/01/2023 22:09, Jonathan Cormier wrote:
> Add a i2c example with HDMI connector

Why? It's the same - but more on this below.

> 
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>

You need to test the bindings before sending and fix the errors.

> ---
>  .../bindings/display/bridge/ti,tfp410.yaml         | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> index 4c5dd8ec2951..456214f14b47 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
> @@ -116,4 +116,46 @@ examples:
>          };
>      };
>  
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      hdmi_encoder: tfp410@38 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +        compatible = "ti,tfp410";
> +        reg = <0x38>;
> +
> +        ports {
> +          address-cells = <1>;
> +          size-cells = <0>;
> +
> +          port@0 {
> +            reg = <0>;
> +            tfp410_in: endpoint {
> +              remote-endpoint = <&dpi1_out>;
> +            };
> +          };
> +
> +          port@1 {
> +            reg = <1>;
> +            tfp410_out: endpoint {
> +              remote-endpoint = <&hdmi_connector_in>;
> +            };

That's the same example as existing one, so it looks useless. I don't
see benefits of this example.

> +          };
> +        };
> +      };
> +    };
> +
> +    hdmi: hdmi_connector {

Drop. Incorrect name and not really related.

> +      compatible = "hdmi-connector";
> +      label = "hdmi";
> +      type = "a";
> +      ddc-i2c-bus = <&i2c1>;
> +      port {
> +        hdmi_connector_in: endpoint {
> +          remote-endpoint = <&tfp410_out>;
> +        };
> +      };
> +    };
> +
>  ...
> 

Best regards,
Krzysztof

Re: [PATCH 2/4] DRM: BRIDGE: TFP410: Support basic I2C interface

[Krzysztof Kozlowski]

On 25/01/2023 22:09, Jonathan Cormier wrote:
> From: Michael Williamson <michael.williamson@criticallink.com>
> 
> The TFP410 driver does not support I2C.  As such, the device remains in
> Power Down if the I2C is enabled by the bootstrap pins.
> 
> Add basic support for the I2C interface, and provide support to take
> the device out of power down when enabled.  Also read the bootstrap mode
> pins via the CTL_1_MODE register when using the I2C bus.
> 
> Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
> ---
>  drivers/gpu/drm/bridge/ti-tfp410.c | 95 +++++++++++++++++++++++++++-----------
>  1 file changed, 68 insertions(+), 27 deletions(-)

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

hint: it is entirely different.

> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index b9635abbad16..323a6d9ed188 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> +#include <linux/regmap.h>
>  #include <linux/media-bus-format.h>
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
> @@ -21,6 +22,20 @@

(...)

>  
>  static void tfp410_i2c_remove(struct i2c_client *client)
> @@ -408,7 +449,7 @@ MODULE_DEVICE_TABLE(i2c, tfp410_i2c_ids);
>  static struct i2c_driver tfp410_i2c_driver = {
>  	.driver = {
>  		.name	= "tfp410",
> -		.of_match_table = of_match_ptr(tfp410_match),
> +		.of_match_table = tfp410_match,

This does not look related to the patch.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: display: bridge: tfp410: Add tfp410 i2c example

[Krzysztof Kozlowski]

On 26/01/2023 19:36, Jon Cormier wrote:
> On Thu, Jan 26, 2023 at 10:40 AM Krzysztof Kozlowski <
> krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 25/01/2023 22:09, Jonathan Cormier wrote:
>>> Add a i2c example with HDMI connector
>>
>> Why? It's the same - but more on this below.
>>
> The existing example is for the previous setup where it was configured as
> its own device.  It seemed necessary now that the driver is going to
> support being connected to an i2c bus to show it being used as such.
> 
>>
>>>
>>> Signed-off-by: Jonathan Cormier <jcormier@criticallink.com>
>>
>> You need to test the bindings before sending and fix the errors.
>>
> Will do
> 
>>
>>> ---
>>>  .../bindings/display/bridge/ti,tfp410.yaml         | 42
>> ++++++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>
>>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
>> b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
>>> index 4c5dd8ec2951..456214f14b47 100644
>>> --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml
>>> @@ -116,4 +116,46 @@ examples:
>>>          };
>>>      };
>>>
>>> +  - |
>>> +    i2c {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      hdmi_encoder: tfp410@38 {
>>
>> Node names should be generic.
>>
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
>> Can do
> 
>>> +        compatible = "ti,tfp410";
>>> +        reg = <0x38>;
>>> +
>>> +        ports {
>>> +          address-cells = <1>;
>>> +          size-cells = <0>;
>>> +
>>> +          port@0 {
>>> +            reg = <0>;
>>> +            tfp410_in: endpoint {
>>> +              remote-endpoint = <&dpi1_out>;
>>> +            };
>>> +          };
>>> +
>>> +          port@1 {
>>> +            reg = <1>;
>>> +            tfp410_out: endpoint {
>>> +              remote-endpoint = <&hdmi_connector_in>;
>>> +            };
>>
>> That's the same example as existing one, so it looks useless. I don't
>> see benefits of this example.
>>
> It's mostly the same, except defined inside an i2c bus, with the reg value
> set. Without the powerdown-gpios or ti,deskew.
> And without the pclk-sample and bus-width (these are now read from i2c)
> And I included the hdmi_connector so it would be a more complete and useful
> example of how it could be used. 

hdmi_connector is being dropped because it is not related.


> The TFP410 doesn't handle the ddc i2c bus
> on its own so a separate connector node is needed.  I'll drop it if that's
> preferred.
> 

If you had here different ports, it would be different case. But as of
now the only important part is having reg and not having gpios, so
basically almost the same example. No need for it.

Best regards,
Krzysztof

Re: [PATCH 4/4] DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.

[kernel test robot]

Hi Jonathan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 93f875a8526a291005e7f38478079526c843cbec]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Cormier/dt-bindings-display-bridge-tfp410-Add-tfp410-i2c-example/20230128-183627
base:   93f875a8526a291005e7f38478079526c843cbec
patch link:    https://lore.kernel.org/r/20230125-tfp410_i2c-v1-4-66a4d4e390b7%40criticallink.com
patch subject: [PATCH 4/4] DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230129/202301290252.zgcWeegX-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c4659fa4c02b62087c095ca99978e5eac8b490de
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jonathan-Cormier/dt-bindings-display-bridge-tfp410-Add-tfp410-i2c-example/20230128-183627
        git checkout c4659fa4c02b62087c095ca99978e5eac8b490de
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/bridge/ti-tfp410.c: In function 'tfp410_connector_detect':
>> drivers/gpu/drm/bridge/ti-tfp410.c:111:13: warning: unused variable 'val' [-Wunused-variable]
     111 |         u32 val;
         |             ^~~


vim +/val +111 drivers/gpu/drm/bridge/ti-tfp410.c

   106	
   107	static enum drm_connector_status
   108	tfp410_connector_detect(struct drm_connector *connector, bool force)
   109	{
   110		struct tfp410 *dvi = drm_connector_to_tfp410(connector);
 > 111		u32 val;
   112		unsigned int ret;
   113	
   114		if (dvi->i2c) {
   115			ret = regmap_test_bits(dvi->regmap, TFP410_REG_CTL_2_MODE, TFP410_BIT_HTPLG);
   116			if (ret < 0)
   117				dev_err(dvi->dev, "%s failed to read HTPLG bit : %d\n", __func__, ret);
   118			else
   119				return ret ? connector_status_connected : connector_status_disconnected;
   120		}
   121	
   122		return drm_bridge_detect(dvi->next_bridge);
   123	}
   124	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 4/4] DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.

[kernel test robot]

Hi Jonathan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 93f875a8526a291005e7f38478079526c843cbec]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Cormier/dt-bindings-display-bridge-tfp410-Add-tfp410-i2c-example/20230128-183627
base:   93f875a8526a291005e7f38478079526c843cbec
patch link:    https://lore.kernel.org/r/20230125-tfp410_i2c-v1-4-66a4d4e390b7%40criticallink.com
patch subject: [PATCH 4/4] DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.
config: i386-randconfig-a006 (https://download.01.org/0day-ci/archive/20230129/202301290803.ouS19eab-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/c4659fa4c02b62087c095ca99978e5eac8b490de
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jonathan-Cormier/dt-bindings-display-bridge-tfp410-Add-tfp410-i2c-example/20230128-183627
        git checkout c4659fa4c02b62087c095ca99978e5eac8b490de
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/bridge/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ti-tfp410.c:111:6: warning: unused variable 'val' [-Wunused-variable]
           u32 val;
               ^
   1 warning generated.


vim +/val +111 drivers/gpu/drm/bridge/ti-tfp410.c

   106	
   107	static enum drm_connector_status
   108	tfp410_connector_detect(struct drm_connector *connector, bool force)
   109	{
   110		struct tfp410 *dvi = drm_connector_to_tfp410(connector);
 > 111		u32 val;
   112		unsigned int ret;
   113	
   114		if (dvi->i2c) {
   115			ret = regmap_test_bits(dvi->regmap, TFP410_REG_CTL_2_MODE, TFP410_BIT_HTPLG);
   116			if (ret < 0)
   117				dev_err(dvi->dev, "%s failed to read HTPLG bit : %d\n", __func__, ret);
   118			else
   119				return ret ? connector_status_connected : connector_status_disconnected;
   120		}
   121	
   122		return drm_bridge_detect(dvi->next_bridge);
   123	}
   124	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 4/4] DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.

[Jon Cormier]

On Sat, Jan 28, 2023 at 7:47 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Jonathan,
>
> Thank you for the patch! Perhaps something to improve:
Good bot.
>
> [auto build test WARNING on 93f875a8526a291005e7f38478079526c843cbec]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Cormier/dt-bindings-display-bridge-tfp410-Add-tfp410-i2c-example/20230128-183627
> base:   93f875a8526a291005e7f38478079526c843cbec
> patch link:    https://lore.kernel.org/r/20230125-tfp410_i2c-v1-4-66a4d4e390b7%40criticallink.com
> patch subject: [PATCH 4/4] DRM: BRIDGE: TFP410: If connected, use I2C for polled HPD status.
> config: i386-randconfig-a006 (https://download.01.org/0day-ci/archive/20230129/202301290803.ouS19eab-lkp@intel.com/config)
> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/c4659fa4c02b62087c095ca99978e5eac8b490de
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Jonathan-Cormier/dt-bindings-display-bridge-tfp410-Add-tfp410-i2c-example/20230128-183627
>         git checkout c4659fa4c02b62087c095ca99978e5eac8b490de
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/bridge/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/gpu/drm/bridge/ti-tfp410.c:111:6: warning: unused variable 'val' [-Wunused-variable]
>            u32 val;
>                ^
>    1 warning generated.
This has already been fixed in V2 of the patch series.
>
>
> vim +/val +111 drivers/gpu/drm/bridge/ti-tfp410.c
>
>    106
>    107  static enum drm_connector_status
>    108  tfp410_connector_detect(struct drm_connector *connector, bool force)
>    109  {
>    110          struct tfp410 *dvi = drm_connector_to_tfp410(connector);
>  > 111          u32 val;
>    112          unsigned int ret;
>    113
>    114          if (dvi->i2c) {
>    115                  ret = regmap_test_bits(dvi->regmap, TFP410_REG_CTL_2_MODE, TFP410_BIT_HTPLG);
>    116                  if (ret < 0)
>    117                          dev_err(dvi->dev, "%s failed to read HTPLG bit : %d\n", __func__, ret);
>    118                  else
>    119                          return ret ? connector_status_connected : connector_status_disconnected;
>    120          }
>    121
>    122          return drm_bridge_detect(dvi->next_bridge);
>    123  }
>    124
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests