[PATCH 0/3] drm/tilcdc: Add bridge support and sync-lost flood recovery

[PATCH 0/3] drm/tilcdc: Add bridge support and sync-lost flood recovery

[Jyri Sarha]

The first patch is an independent on and I've been testing it for
quite a while now.

The tfp410 bridge driver and the tilcdc bridge support are tested with
BeagleBone DVI-D Cape Rev A3. The tfp410 bridge driver is missing a
lot of features, because the DVI-D cape does not have too many wires
connected. They missing features can be added later when they are
needed.

Jyri Sarha (3):
  drm/tilcdc: Recover from sync lost error flood by resetting the LCDC
  drm/bridge: Add ti-ftp410 HDMI transmitter driver
  drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

 .../bindings/display/bridge/ti,tfp410.txt          |  30 ++++
 .../devicetree/bindings/display/tilcdc/tilcdc.txt  |   7 +-
 drivers/gpu/drm/bridge/Kconfig                     |   7 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/ti-tfp410.c                 | 199 +++++++++++++++++++++
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c               |  26 ++-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |   7 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   2 +
 drivers/gpu/drm/tilcdc/tilcdc_external.c           | 140 +++++++++++++--
 drivers/gpu/drm/tilcdc/tilcdc_external.h           |   1 +
 10 files changed, 397 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-tfp410.c

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/tilcdc: Recover from sync lost error flood by resetting the LCDC

[Jyri Sarha]

Recover from sync lost error flood by resetting the LCDC instead of
turning off the SYNC_LOST error IRQ. When LCDC starves on limited
memory bandwidth it may sometimes result an error situation when the
picture may have shifted couple of pixels to right and SYNC_LOST
interrupt is generated on every frame. LCDC main reset recovers from
this situation and causes a brief blanking on the screen.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 0d09acc..c787349 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -55,6 +55,7 @@ struct tilcdc_crtc {
 
 	int sync_lost_count;
 	bool frame_intact;
+	struct work_struct recover_work;
 };
 #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
 
@@ -252,6 +253,25 @@ static bool tilcdc_crtc_is_on(struct drm_crtc *crtc)
 	return crtc->state && crtc->state->enable && crtc->state->active;
 }
 
+static void tilcdc_crtc_recover_work(struct work_struct *work)
+{
+	struct tilcdc_crtc *tilcdc_crtc =
+		container_of(work, struct tilcdc_crtc, recover_work);
+	struct drm_crtc *crtc = &tilcdc_crtc->base;
+
+	dev_info(crtc->dev->dev, "%s: Reset CRTC", __func__);
+
+	drm_modeset_lock_crtc(crtc, NULL);
+
+	if (!tilcdc_crtc_is_on(crtc))
+		goto out;
+
+	tilcdc_crtc_disable(crtc);
+	tilcdc_crtc_enable(crtc);
+out:
+	drm_modeset_unlock_crtc(crtc);
+}
+
 static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
@@ -838,9 +858,12 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
 			tilcdc_crtc->frame_intact = false;
 			if (tilcdc_crtc->sync_lost_count++ >
 			    SYNC_LOST_COUNT_LIMIT) {
-				dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, disabling the interrupt", __func__, stat);
+				dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);
+				queue_work(system_wq,
+					   &tilcdc_crtc->recover_work);
 				tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
 					     LCDC_SYNC_LOST);
+				tilcdc_crtc->sync_lost_count = 0;
 			}
 		}
 
@@ -880,6 +903,7 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 			"unref", unref_worker);
 
 	spin_lock_init(&tilcdc_crtc->irq_lock);
+	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
 
 	ret = drm_crtc_init_with_planes(dev, crtc,
 					&tilcdc_crtc->primary,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Jyri Sarha]

Add very basic ti-ftp410 HDMI transmitter driver. The only feature
separating this from a completely dummy bridge is the DDC i2c
support. However, other HW specific features may be added later when
needed. For instance there is a set of registers behind i2c if it is
connected. The implementations is tested against my new tilcdc bridge
support and works with BeagleBone DVI-D Cape Rev A3. A DT binding
document is also added.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../bindings/display/bridge/ti,tfp410.txt          |  30 ++++
 drivers/gpu/drm/bridge/Kconfig                     |   7 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/ti-tfp410.c                 | 199 +++++++++++++++++++++
 4 files changed, 237 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-tfp410.c

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
new file mode 100644
index 0000000..dc93713
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
@@ -0,0 +1,30 @@
+TFP410 HDMI/DVI bridge bindings
+
+Required properties:
+	- compatible: "ti,tfp410"
+
+Optional properties:
+	- ddc-i2c: phandle of an I2C controller used for DDC EDID probing
+
+Optional subnodes:
+	- video input: this subnode can contain a video input port node
+	  to connect the bridge to a display controller output (See this
+	  documentation [1]).
+
+[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+	hdmi-bridge {
+		compatible = "ti,tfp410";
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				bridge_in: endpoint {
+					remote-endpoint = <&dc_out>;
+				};
+			};
+		};
+	};
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index bd6acc8..a424e03 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -81,6 +81,13 @@ config DRM_TOSHIBA_TC358767
 	---help---
 	  Toshiba TC358767 eDP bridge chip driver.
 
+config DRM_TI_TFP410
+	tristate "TI TFP410 DVI/HDMI bridge"
+	depends on OF
+	select DRM_KMS_HELPER
+	---help---
+	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
+
 source "drivers/gpu/drm/bridge/analogix/Kconfig"
 
 source "drivers/gpu/drm/bridge/adv7511/Kconfig"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 97ed1a5..8b065d9 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
+obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
new file mode 100644
index 0000000..b0753d2
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2016 Texas Instruments
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/of_graph.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+
+struct tfp410 {
+	struct drm_bridge	bridge;
+	struct drm_connector	connector;
+
+	struct i2c_adapter	*ddc;
+
+	struct device *dev;
+};
+
+static inline struct tfp410 *
+drm_bridge_to_tfp410(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct tfp410, bridge);
+}
+
+static inline struct tfp410 *
+drm_connector_to_tfp410(struct drm_connector *connector)
+{
+	return container_of(connector, struct tfp410, connector);
+}
+
+static int tfp410_get_modes(struct drm_connector *connector)
+{
+	struct tfp410 *hdmi = drm_connector_to_tfp410(connector);
+	struct edid *edid;
+	int ret;
+
+	if (!hdmi->ddc)
+		goto fallback;
+
+	edid = drm_get_edid(connector, hdmi->ddc);
+	if (!edid) {
+		DRM_INFO("EDID read failed. Fallback to standard modes\n");
+		goto fallback;
+	}
+
+	drm_mode_connector_update_edid_property(connector, edid);
+
+	return drm_add_edid_modes(connector, edid);
+fallback:
+	/* No EDID, fallback on the XGA standard modes */
+	ret = drm_add_modes_noedid(connector, 1920, 1200);
+
+	/* And prefer a mode pretty much anything can handle */
+	drm_set_preferred_mode(connector, 1024, 768);
+
+	return ret;
+}
+
+static const struct drm_connector_helper_funcs tfp410_con_helper_funcs = {
+	.get_modes	= tfp410_get_modes,
+};
+
+static enum drm_connector_status
+tfp410_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tfp410 *hdmi = drm_connector_to_tfp410(connector);
+
+	if (hdmi->ddc) {
+		if (drm_probe_ddc(hdmi->ddc))
+			return connector_status_connected;
+		else
+			return connector_status_disconnected;
+	}
+
+	return connector_status_unknown;
+}
+
+static const struct drm_connector_funcs tfp410_con_funcs = {
+	.dpms			= drm_atomic_helper_connector_dpms,
+	.detect			= tfp410_connector_detect,
+	.fill_modes		= drm_helper_probe_single_connector_modes,
+	.destroy		= drm_connector_cleanup,
+	.reset			= drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
+};
+
+static int tfp410_attach(struct drm_bridge *bridge)
+{
+	struct tfp410 *hdmi = drm_bridge_to_tfp410(bridge);
+	int ret;
+
+	if (!bridge->encoder) {
+		dev_err(hdmi->dev, "Missing encoder\n");
+		return -ENODEV;
+	}
+
+	drm_connector_helper_add(&hdmi->connector,
+				 &tfp410_con_helper_funcs);
+	ret = drm_connector_init(bridge->dev, &hdmi->connector,
+				 &tfp410_con_funcs, DRM_MODE_CONNECTOR_HDMIA);
+	if (ret) {
+		dev_err(hdmi->dev, "drm_connector_init() failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_mode_connector_attach_encoder(&hdmi->connector,
+					  bridge->encoder);
+
+	return 0;
+}
+
+static const struct drm_bridge_funcs tfp410_bridge_funcs = {
+	.attach		= tfp410_attach,
+};
+
+static int tfp410_probe(struct platform_device *pdev)
+{
+	struct device_node *ddc_phandle;
+	struct tfp410 *hdmi;
+	int ret;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "device-tree data is missing\n");
+		return -ENXIO;
+	}
+
+	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	if (!hdmi)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, hdmi);
+
+	ddc_phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c", 0);
+	if (ddc_phandle) {
+		hdmi->ddc = of_get_i2c_adapter_by_node(ddc_phandle);
+		of_node_put(ddc_phandle);
+		if (!hdmi->ddc)
+			return -EPROBE_DEFER;
+	} else {
+		dev_info(&pdev->dev,
+			 "No ddc i2c bus, disabling EDID readout\n");
+	}
+
+	hdmi->bridge.funcs = &tfp410_bridge_funcs;
+	hdmi->bridge.of_node = pdev->dev.of_node;
+	hdmi->dev = &pdev->dev;
+
+	ret = drm_bridge_add(&hdmi->bridge);
+	if (ret) {
+		dev_err(&pdev->dev, "drm_bridge_add() failed: %d\n", ret);
+		goto fail;
+	}
+
+	return 0;
+fail:
+	i2c_put_adapter(hdmi->ddc);
+	return ret;
+}
+
+static int tfp410_remove(struct platform_device *pdev)
+{
+	struct tfp410 *hdmi = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&hdmi->bridge);
+
+	if (hdmi->ddc)
+		i2c_put_adapter(hdmi->ddc);
+
+	return 0;
+}
+
+static const struct of_device_id tfp410_match[] = {
+	{ .compatible = "ti,tfp410" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tfp410_match);
+
+struct platform_driver tfp410_driver = {
+	.probe	= tfp410_probe,
+	.remove	= tfp410_remove,
+	.driver	= {
+		.name		= "tfp410-bridge",
+		.of_match_table	= tfp410_match,
+	},
+};
+module_platform_driver(tfp410_driver);
+
+MODULE_AUTHOR("Jyri Sarha <jsarha@ti.com>");
+MODULE_DESCRIPTION("TI TFP410 HDMI bridge driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

[Jyri Sarha]

Adds drm bride support for attaching drm bridge drivers to tilcdc. The
decision whether a video port leads to an external encoder or bridge
is made simply based on remote device's compatible string. The code
has been tested with BeagleBone-Black with and without BeagleBone
DVI-D Cape Rev A3 using ti-tfp410 driver.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../devicetree/bindings/display/tilcdc/tilcdc.txt  |   7 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |   7 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   2 +
 drivers/gpu/drm/tilcdc/tilcdc_external.c           | 140 ++++++++++++++++++---
 drivers/gpu/drm/tilcdc/tilcdc_external.h           |   1 +
 5 files changed, 135 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
index 6fddb4f..ae7a942 100644
--- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
+++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
@@ -34,9 +34,10 @@ Optional properties:
 
 Optional nodes:
 
- - port/ports: to describe a connection to an external encoder. The
-   binding follows Documentation/devicetree/bindings/graph.txt and
-   suppors a single port with a single endpoint.
+ - port/ports: to describe a connection to an external encoder or
+   bridge. The binding follows
+   Documentation/devicetree/bindings/graph.txt and suppors a single
+   port with a single endpoint.
 
  - See also Documentation/devicetree/bindings/display/tilcdc/panel.txt and
    Documentation/devicetree/bindings/display/tilcdc/tfp410.txt for connecting
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index dcc9621..ec22576 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -384,9 +384,14 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 		ret = tilcdc_add_external_encoders(ddev);
 		if (ret < 0)
 			goto init_failed;
+	} else {
+		ret = tilcdc_attach_remote_device(ddev);
+		if (ret)
+			goto init_failed;
 	}
 
-	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
+	if (!priv->remote_encoder &&
+	    ((priv->num_encoders == 0) || (priv->num_connectors == 0))) {
 		dev_err(dev, "no encoders/connectors found\n");
 		ret = -ENXIO;
 		goto init_failed;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index d31fe5d..283ff28 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -90,6 +90,8 @@ struct tilcdc_drm_private {
 	struct drm_connector *connectors[8];
 	const struct drm_connector_helper_funcs *connector_funcs[8];
 
+	struct drm_encoder *remote_encoder;
+
 	bool is_registered;
 	bool is_componentized;
 };
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
index 06a4c58..bcb1324 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -28,6 +28,18 @@
 		.raster_order           = 0,
 };
 
+static const struct tilcdc_panel_info panel_info_default = {
+		.ac_bias                = 255,
+		.ac_bias_intrpt         = 0,
+		.dma_burst_sz           = 16,
+		.bpp                    = 16,
+		.fdd                    = 0x80,
+		.tft_alt_mode           = 0,
+		.sync_edge              = 0,
+		.sync_ctrl              = 1,
+		.raster_order           = 0,
+};
+
 static int tilcdc_external_mode_valid(struct drm_connector *connector,
 				      struct drm_display_mode *mode)
 {
@@ -130,6 +142,101 @@ void tilcdc_remove_external_encoders(struct drm_device *dev)
 						 priv->connector_funcs[i]);
 }
 
+static struct drm_encoder_funcs tilcdc_remote_encoder_funcs = {
+	.destroy	= drm_encoder_cleanup,
+};
+
+static
+int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge *bridge)
+{
+	struct tilcdc_drm_private *priv = ddev->dev_private;
+	int ret;
+
+	priv->remote_encoder->possible_crtcs = BIT(0);
+	priv->remote_encoder->bridge = bridge;
+	bridge->encoder = priv->remote_encoder;
+
+	ret = drm_bridge_attach(ddev, bridge);
+	if (ret) {
+		dev_err(ddev->dev, "drm_bridge_attach() failed %d\n", ret);
+		return ret;
+	}
+
+	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_default);
+
+	return 0;
+}
+
+static int tilcdc_node_has_port(struct device_node *dev_node)
+{
+	struct device_node *node;
+
+	node = of_get_child_by_name(dev_node, "ports");
+	if (!node)
+		node = of_get_child_by_name(dev_node, "port");
+	if (!node)
+		return 0;
+	of_node_put(node);
+
+	return 1;
+}
+
+static
+struct device_node *tilcdc_get_remote_node(struct device_node *node)
+{
+	struct device_node *ep;
+	struct device_node *parent;
+
+	if (!tilcdc_node_has_port(node))
+		return NULL;
+
+	ep = of_graph_get_next_endpoint(node, NULL);
+	if (!ep)
+		return NULL;
+
+	parent = of_graph_get_remote_port_parent(ep);
+	of_node_put(ep);
+
+	return parent;
+}
+
+int tilcdc_attach_remote_device(struct drm_device *ddev)
+{
+	struct tilcdc_drm_private *priv = ddev->dev_private;
+	struct device_node *remote_node;
+	struct drm_bridge *bridge;
+	int ret;
+
+	remote_node = tilcdc_get_remote_node(ddev->dev->of_node);
+	if (!remote_node)
+		return 0;
+
+	bridge = of_drm_find_bridge(remote_node);
+	of_node_put(remote_node);
+	if (!bridge)
+		return -EPROBE_DEFER;
+
+	priv->remote_encoder = devm_kzalloc(ddev->dev,
+					    sizeof(*priv->remote_encoder),
+					    GFP_KERNEL);
+	if (!priv->remote_encoder)
+		return -ENOMEM;
+
+	ret = drm_encoder_init(ddev, priv->remote_encoder,
+			       &tilcdc_remote_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret) {
+		dev_err(ddev->dev, "drm_encoder_init() failed %d\n", ret);
+		return ret;
+	}
+
+	ret = tilcdc_attach_bridge(ddev, bridge);
+	if (ret)
+		drm_encoder_cleanup(priv->remote_encoder);
+
+	return ret;
+}
+
 static int dev_match_of(struct device *dev, void *data)
 {
 	return dev->of_node == data;
@@ -141,16 +248,10 @@ int tilcdc_get_external_components(struct device *dev,
 	struct device_node *node;
 	struct device_node *ep = NULL;
 	int count = 0;
+	int ret = 0;
 
-	/* Avoid error print by of_graph_get_next_endpoint() if there
-	 * is no ports present.
-	 */
-	node = of_get_child_by_name(dev->of_node, "ports");
-	if (!node)
-		node = of_get_child_by_name(dev->of_node, "port");
-	if (!node)
+	if (!tilcdc_node_has_port(dev->of_node))
 		return 0;
-	of_node_put(node);
 
 	while ((ep = of_graph_get_next_endpoint(dev->of_node, ep))) {
 		node = of_graph_get_remote_port_parent(ep);
@@ -160,17 +261,20 @@ int tilcdc_get_external_components(struct device *dev,
 		}
 
 		dev_dbg(dev, "Subdevice node '%s' found\n", node->name);
-		if (match)
-			drm_of_component_match_add(dev, match, dev_match_of,
-						   node);
-		of_node_put(node);
-		count++;
-	}
 
-	if (count > 1) {
-		dev_err(dev, "Only one external encoder is supported\n");
-		return -EINVAL;
+		if (of_device_is_compatible(node, "nxp,tda998x")) {
+			if (match)
+				drm_of_component_match_add(dev, match,
+							   dev_match_of, node);
+			ret = 1;
+		}
+
+		of_node_put(node);
+		if (count++ > 1) {
+			dev_err(dev, "Only one port is supported\n");
+			return -EINVAL;
+		}
 	}
 
-	return count;
+	return ret;
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.h b/drivers/gpu/drm/tilcdc/tilcdc_external.h
index c700e0c..a27c365 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.h
@@ -22,4 +22,5 @@
 void tilcdc_remove_external_encoders(struct drm_device *dev);
 int tilcdc_get_external_components(struct device *dev,
 				   struct component_match **match);
+int tilcdc_attach_remote_device(struct drm_device *ddev);
 #endif /* __TILCDC_SLAVE_H__ */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 150 bytes --]

On 02/11/16 18:32, Jyri Sarha wrote:
> Add very basic ti-ftp410 HDMI transmitter driver. The only feature

It's DVI encoder, not HDMI.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Wednesday 02 Nov 2016 18:32:16 Jyri Sarha wrote:
> Add very basic ti-ftp410 HDMI transmitter driver. The only feature
> separating this from a completely dummy bridge is the DDC i2c
> support. However, other HW specific features may be added later when
> needed. For instance there is a set of registers behind i2c if it is
> connected. The implementations is tested against my new tilcdc bridge
> support and works with BeagleBone DVI-D Cape Rev A3. A DT binding
> document is also added.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../bindings/display/bridge/ti,tfp410.txt          |  30 ++++
>  drivers/gpu/drm/bridge/Kconfig                     |   7 +
>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>  drivers/gpu/drm/bridge/ti-tfp410.c                 | 199 ++++++++++++++++++
>  4 files changed, 237 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt create mode
> 100644 drivers/gpu/drm/bridge/ti-tfp410.c
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt new file
> mode 100644
> index 0000000..dc93713
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> @@ -0,0 +1,30 @@
> +TFP410 HDMI/DVI bridge bindings

I'd name the document "TI TFP410 DVI Transmitter". DVI bridge doesn't tell 
whether the device is a receiver or transmitter.

> +Required properties:
> +	- compatible: "ti,tfp410"

The device is an I2C slave, it should have a reg property. Given that the chip 
can be used without being controlled through I2C, the reg property should be 
optional. You should document this clearly, and explain how the DT node can be 
instantiated as a child of an I2C controller when the I2C interface is used, 
or in other parts of the device tree otherwise.

> +Optional properties:
> +	- ddc-i2c: phandle of an I2C controller used for DDC EDID probing

The TFP410 doesn't handle DDC, this property should be part of the connector 
node.

> +Optional subnodes:
> +	- video input: this subnode can contain a video input port node
> +	  to connect the bridge to a display controller output (See this
> +	  documentation [1]).

You also need an output port for the DVI output. Those two ports should be 
required, not optional.

> +[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +Example:
> +	hdmi-bridge {
> +		compatible = "ti,tfp410";
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				bridge_in: endpoint {
> +					remote-endpoint = <&dc_out>;
> +				};
> +			};
> +		};
> +	};


> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index bd6acc8..a424e03 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -81,6 +81,13 @@ config DRM_TOSHIBA_TC358767
>  	---help---
>  	  Toshiba TC358767 eDP bridge chip driver.
> 
> +config DRM_TI_TFP410
> +	tristate "TI TFP410 DVI/HDMI bridge"
> +	depends on OF
> +	select DRM_KMS_HELPER
> +	---help---
> +	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
> +
>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
> 
>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> diff --git a/drivers/gpu/drm/bridge/Makefile
> b/drivers/gpu/drm/bridge/Makefile index 97ed1a5..8b065d9 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> +obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> b/drivers/gpu/drm/bridge/ti-tfp410.c new file mode 100644
> index 0000000..b0753d2
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 Texas Instruments
> + * Author: Jyri Sarha <jsarha@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by + * the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_graph.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +struct tfp410 {
> +	struct drm_bridge	bridge;
> +	struct drm_connector	connector;
> +
> +	struct i2c_adapter	*ddc;
> +
> +	struct device *dev;
> +};
> +
> +static inline struct tfp410 *
> +drm_bridge_to_tfp410(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct tfp410, bridge);
> +}
> +
> +static inline struct tfp410 *
> +drm_connector_to_tfp410(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct tfp410, connector);
> +}
> +
> +static int tfp410_get_modes(struct drm_connector *connector)
> +{
> +	struct tfp410 *hdmi = drm_connector_to_tfp410(connector);
> +	struct edid *edid;
> +	int ret;
> +
> +	if (!hdmi->ddc)
> +		goto fallback;
> +
> +	edid = drm_get_edid(connector, hdmi->ddc);
> +	if (!edid) {
> +		DRM_INFO("EDID read failed. Fallback to standard modes\n");
> +		goto fallback;
> +	}
> +
> +	drm_mode_connector_update_edid_property(connector, edid);
> +
> +	return drm_add_edid_modes(connector, edid);
> +fallback:
> +	/* No EDID, fallback on the XGA standard modes */
> +	ret = drm_add_modes_noedid(connector, 1920, 1200);
> +
> +	/* And prefer a mode pretty much anything can handle */
> +	drm_set_preferred_mode(connector, 1024, 768);
> +
> +	return ret;
> +}
> +
> +static const struct drm_connector_helper_funcs tfp410_con_helper_funcs = {
> +	.get_modes	= tfp410_get_modes,
> +};
> +
> +static enum drm_connector_status
> +tfp410_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct tfp410 *hdmi = drm_connector_to_tfp410(connector);
> +
> +	if (hdmi->ddc) {
> +		if (drm_probe_ddc(hdmi->ddc))
> +			return connector_status_connected;
> +		else
> +			return connector_status_disconnected;
> +	}
> +
> +	return connector_status_unknown;
> +}
> +
> +static const struct drm_connector_funcs tfp410_con_funcs = {
> +	.dpms			= drm_atomic_helper_connector_dpms,
> +	.detect			= tfp410_connector_detect,
> +	.fill_modes		= drm_helper_probe_single_connector_modes,
> +	.destroy		= drm_connector_cleanup,
> +	.reset			= drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int tfp410_attach(struct drm_bridge *bridge)
> +{
> +	struct tfp410 *hdmi = drm_bridge_to_tfp410(bridge);
> +	int ret;
> +
> +	if (!bridge->encoder) {
> +		dev_err(hdmi->dev, "Missing encoder\n");
> +		return -ENODEV;
> +	}
> +
> +	drm_connector_helper_add(&hdmi->connector,
> +				 &tfp410_con_helper_funcs);
> +	ret = drm_connector_init(bridge->dev, &hdmi->connector,
> +				 &tfp410_con_funcs, DRM_MODE_CONNECTOR_HDMIA);
> +	if (ret) {
> +		dev_err(hdmi->dev, "drm_connector_init() failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_mode_connector_attach_encoder(&hdmi->connector,
> +					  bridge->encoder);
> +
> +	return 0;
> +}
> +
> +static const struct drm_bridge_funcs tfp410_bridge_funcs = {
> +	.attach		= tfp410_attach,
> +};
> +
> +static int tfp410_probe(struct platform_device *pdev)
> +{
> +	struct device_node *ddc_phandle;
> +	struct tfp410 *hdmi;
> +	int ret;
> +
> +	if (!pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "device-tree data is missing\n");
> +		return -ENXIO;
> +	}
> +
> +	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> +	if (!hdmi)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, hdmi);
> +
> +	ddc_phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c", 0);
> +	if (ddc_phandle) {
> +		hdmi->ddc = of_get_i2c_adapter_by_node(ddc_phandle);
> +		of_node_put(ddc_phandle);
> +		if (!hdmi->ddc)
> +			return -EPROBE_DEFER;
> +	} else {
> +		dev_info(&pdev->dev,
> +			 "No ddc i2c bus, disabling EDID readout\n");
> +	}
> +
> +	hdmi->bridge.funcs = &tfp410_bridge_funcs;
> +	hdmi->bridge.of_node = pdev->dev.of_node;
> +	hdmi->dev = &pdev->dev;
> +
> +	ret = drm_bridge_add(&hdmi->bridge);
> +	if (ret) {
> +		dev_err(&pdev->dev, "drm_bridge_add() failed: %d\n", ret);
> +		goto fail;
> +	}
> +
> +	return 0;
> +fail:
> +	i2c_put_adapter(hdmi->ddc);
> +	return ret;
> +}
> +
> +static int tfp410_remove(struct platform_device *pdev)
> +{
> +	struct tfp410 *hdmi = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&hdmi->bridge);
> +
> +	if (hdmi->ddc)
> +		i2c_put_adapter(hdmi->ddc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tfp410_match[] = {
> +	{ .compatible = "ti,tfp410" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, tfp410_match);
> +
> +struct platform_driver tfp410_driver = {
> +	.probe	= tfp410_probe,
> +	.remove	= tfp410_remove,
> +	.driver	= {
> +		.name		= "tfp410-bridge",
> +		.of_match_table	= tfp410_match,
> +	},
> +};
> +module_platform_driver(tfp410_driver);
> +
> +MODULE_AUTHOR("Jyri Sarha <jsarha@ti.com>");
> +MODULE_DESCRIPTION("TI TFP410 HDMI bridge driver");
> +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Wednesday 02 Nov 2016 18:32:17 Jyri Sarha wrote:
> Adds drm bride support for attaching drm bridge drivers to tilcdc. The
> decision whether a video port leads to an external encoder or bridge
> is made simply based on remote device's compatible string. The code
> has been tested with BeagleBone-Black with and without BeagleBone
> DVI-D Cape Rev A3 using ti-tfp410 driver.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../devicetree/bindings/display/tilcdc/tilcdc.txt  |   7 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c                |   7 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   2 +
>  drivers/gpu/drm/tilcdc/tilcdc_external.c           | 140 ++++++++++++++---
>  drivers/gpu/drm/tilcdc/tilcdc_external.h           |   1 +
>  5 files changed, 135 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt index
> 6fddb4f..ae7a942 100644
> --- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> +++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> @@ -34,9 +34,10 @@ Optional properties:
> 
>  Optional nodes:
> 
> - - port/ports: to describe a connection to an external encoder. The
> -   binding follows Documentation/devicetree/bindings/graph.txt and
> -   suppors a single port with a single endpoint.
> + - port/ports: to describe a connection to an external encoder or
> +   bridge. The binding follows

What's the difference between encoder and bridge here ? drm_bridge and 
drm_encoder are irrelevant to DT bindings, we should only care about the 
hardware view of the system here.

> +   Documentation/devicetree/bindings/graph.txt and suppors a single
> +   port with a single endpoint.
> 
>   - See also Documentation/devicetree/bindings/display/tilcdc/panel.txt and
>     Documentation/devicetree/bindings/display/tilcdc/tfp410.txt for
> connecting diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index dcc9621..ec22576 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -384,9 +384,14 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
> device *dev) ret = tilcdc_add_external_encoders(ddev);
>  		if (ret < 0)
>  			goto init_failed;
> +	} else {
> +		ret = tilcdc_attach_remote_device(ddev);
> +		if (ret)
> +			goto init_failed;
>  	}
> 
> -	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
> +	if (!priv->remote_encoder &&
> +	    ((priv->num_encoders == 0) || (priv->num_connectors == 0))) {
>  		dev_err(dev, "no encoders/connectors found\n");
>  		ret = -ENXIO;
>  		goto init_failed;
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index d31fe5d..283ff28 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -90,6 +90,8 @@ struct tilcdc_drm_private {
>  	struct drm_connector *connectors[8];
>  	const struct drm_connector_helper_funcs *connector_funcs[8];
> 
> +	struct drm_encoder *remote_encoder;
> +
>  	bool is_registered;
>  	bool is_componentized;
>  };
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c
> b/drivers/gpu/drm/tilcdc/tilcdc_external.c index 06a4c58..bcb1324 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
> @@ -28,6 +28,18 @@
>  		.raster_order           = 0,
>  };
> 
> +static const struct tilcdc_panel_info panel_info_default = {
> +		.ac_bias                = 255,
> +		.ac_bias_intrpt         = 0,
> +		.dma_burst_sz           = 16,
> +		.bpp                    = 16,
> +		.fdd                    = 0x80,
> +		.tft_alt_mode           = 0,
> +		.sync_edge              = 0,
> +		.sync_ctrl              = 1,
> +		.raster_order           = 0,
> +};
> +
>  static int tilcdc_external_mode_valid(struct drm_connector *connector,
>  				      struct drm_display_mode *mode)
>  {
> @@ -130,6 +142,101 @@ void tilcdc_remove_external_encoders(struct drm_device
> *dev) priv->connector_funcs[i]);
>  }
> 
> +static struct drm_encoder_funcs tilcdc_remote_encoder_funcs = {
> +	.destroy	= drm_encoder_cleanup,
> +};
> +
> +static
> +int tilcdc_attach_bridge(struct drm_device *ddev, struct drm_bridge
> *bridge) +{
> +	struct tilcdc_drm_private *priv = ddev->dev_private;
> +	int ret;
> +
> +	priv->remote_encoder->possible_crtcs = BIT(0);
> +	priv->remote_encoder->bridge = bridge;
> +	bridge->encoder = priv->remote_encoder;
> +
> +	ret = drm_bridge_attach(ddev, bridge);
> +	if (ret) {
> +		dev_err(ddev->dev, "drm_bridge_attach() failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	tilcdc_crtc_set_panel_info(priv->crtc, &panel_info_default);
> +
> +	return 0;
> +}
> +
> +static int tilcdc_node_has_port(struct device_node *dev_node)
> +{
> +	struct device_node *node;
> +
> +	node = of_get_child_by_name(dev_node, "ports");
> +	if (!node)
> +		node = of_get_child_by_name(dev_node, "port");
> +	if (!node)
> +		return 0;
> +	of_node_put(node);
> +
> +	return 1;
> +}
> +
> +static
> +struct device_node *tilcdc_get_remote_node(struct device_node *node)
> +{
> +	struct device_node *ep;
> +	struct device_node *parent;
> +
> +	if (!tilcdc_node_has_port(node))
> +		return NULL;
> +
> +	ep = of_graph_get_next_endpoint(node, NULL);
> +	if (!ep)
> +		return NULL;
> +
> +	parent = of_graph_get_remote_port_parent(ep);
> +	of_node_put(ep);
> +
> +	return parent;
> +}
> +
> +int tilcdc_attach_remote_device(struct drm_device *ddev)
> +{
> +	struct tilcdc_drm_private *priv = ddev->dev_private;
> +	struct device_node *remote_node;
> +	struct drm_bridge *bridge;
> +	int ret;
> +
> +	remote_node = tilcdc_get_remote_node(ddev->dev->of_node);
> +	if (!remote_node)
> +		return 0;
> +
> +	bridge = of_drm_find_bridge(remote_node);
> +	of_node_put(remote_node);
> +	if (!bridge)
> +		return -EPROBE_DEFER;
> +
> +	priv->remote_encoder = devm_kzalloc(ddev->dev,
> +					    sizeof(*priv->remote_encoder),
> +					    GFP_KERNEL);
> +	if (!priv->remote_encoder)
> +		return -ENOMEM;
> +
> +	ret = drm_encoder_init(ddev, priv->remote_encoder,
> +			       &tilcdc_remote_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret) {
> +		dev_err(ddev->dev, "drm_encoder_init() failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = tilcdc_attach_bridge(ddev, bridge);
> +	if (ret)
> +		drm_encoder_cleanup(priv->remote_encoder);
> +
> +	return ret;
> +}
> +
>  static int dev_match_of(struct device *dev, void *data)
>  {
>  	return dev->of_node == data;
> @@ -141,16 +248,10 @@ int tilcdc_get_external_components(struct device *dev,
> struct device_node *node;
>  	struct device_node *ep = NULL;
>  	int count = 0;
> +	int ret = 0;
> 
> -	/* Avoid error print by of_graph_get_next_endpoint() if there
> -	 * is no ports present.
> -	 */
> -	node = of_get_child_by_name(dev->of_node, "ports");
> -	if (!node)
> -		node = of_get_child_by_name(dev->of_node, "port");
> -	if (!node)
> +	if (!tilcdc_node_has_port(dev->of_node))
>  		return 0;
> -	of_node_put(node);
> 
>  	while ((ep = of_graph_get_next_endpoint(dev->of_node, ep))) {
>  		node = of_graph_get_remote_port_parent(ep);
> @@ -160,17 +261,20 @@ int tilcdc_get_external_components(struct device *dev,
> }
> 
>  		dev_dbg(dev, "Subdevice node '%s' found\n", node->name);
> -		if (match)
> -			drm_of_component_match_add(dev, match, dev_match_of,
> -						   node);
> -		of_node_put(node);
> -		count++;
> -	}
> 
> -	if (count > 1) {
> -		dev_err(dev, "Only one external encoder is supported\n");
> -		return -EINVAL;
> +		if (of_device_is_compatible(node, "nxp,tda998x")) {
> +			if (match)
> +				drm_of_component_match_add(dev, match,
> +							   dev_match_of, 
node);
> +			ret = 1;
> +		}
> +
> +		of_node_put(node);
> +		if (count++ > 1) {
> +			dev_err(dev, "Only one port is supported\n");
> +			return -EINVAL;
> +		}
>  	}
> 
> -	return count;
> +	return ret;
>  }
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.h
> b/drivers/gpu/drm/tilcdc/tilcdc_external.h index c700e0c..a27c365 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_external.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.h
> @@ -22,4 +22,5 @@
>  void tilcdc_remove_external_encoders(struct drm_device *dev);
>  int tilcdc_get_external_components(struct device *dev,
>  				   struct component_match **match);
> +int tilcdc_attach_remote_device(struct drm_device *ddev);
>  #endif /* __TILCDC_SLAVE_H__ */

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/tilcdc: Add drm bridge support for attaching drm bridge drivers

[Rob Herring]

On Wed, Nov 02, 2016 at 06:32:17PM +0200, Jyri Sarha wrote:
> Adds drm bride support for attaching drm bridge drivers to tilcdc. The
> decision whether a video port leads to an external encoder or bridge
> is made simply based on remote device's compatible string. The code
> has been tested with BeagleBone-Black with and without BeagleBone
> DVI-D Cape Rev A3 using ti-tfp410 driver.
> 
> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
> ---
>  .../devicetree/bindings/display/tilcdc/tilcdc.txt  |   7 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c                |   7 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h                |   2 +
>  drivers/gpu/drm/tilcdc/tilcdc_external.c           | 140 ++++++++++++++++++---
>  drivers/gpu/drm/tilcdc/tilcdc_external.h           |   1 +
>  5 files changed, 135 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> index 6fddb4f..ae7a942 100644
> --- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> +++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> @@ -34,9 +34,10 @@ Optional properties:
>  
>  Optional nodes:
>  
> - - port/ports: to describe a connection to an external encoder. The
> -   binding follows Documentation/devicetree/bindings/graph.txt and
> -   suppors a single port with a single endpoint.
> + - port/ports: to describe a connection to an external encoder or
> +   bridge. The binding follows
> +   Documentation/devicetree/bindings/graph.txt and suppors a single
> +   port with a single endpoint.

I don't think a DT change is needed here. Bridge vs. encoder is all DRM 
terminology.

>  
>   - See also Documentation/devicetree/bindings/display/tilcdc/panel.txt and
>     Documentation/devicetree/bindings/display/tilcdc/tfp410.txt for connecting
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Jyri Sarha]

On 11/03/16 19:46, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Wednesday 02 Nov 2016 18:32:16 Jyri Sarha wrote:
>> Add very basic ti-ftp410 HDMI transmitter driver. The only feature
>> separating this from a completely dummy bridge is the DDC i2c
>> support. However, other HW specific features may be added later when
>> needed. For instance there is a set of registers behind i2c if it is
>> connected. The implementations is tested against my new tilcdc bridge
>> support and works with BeagleBone DVI-D Cape Rev A3. A DT binding
>> document is also added.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  .../bindings/display/bridge/ti,tfp410.txt          |  30 ++++
>>  drivers/gpu/drm/bridge/Kconfig                     |   7 +
>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>  drivers/gpu/drm/bridge/ti-tfp410.c                 | 199 ++++++++++++++++++
>>  4 files changed, 237 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt create mode
>> 100644 drivers/gpu/drm/bridge/ti-tfp410.c
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>> b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt new file
>> mode 100644
>> index 0000000..dc93713
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>> @@ -0,0 +1,30 @@
>> +TFP410 HDMI/DVI bridge bindings
> 
> I'd name the document "TI TFP410 DVI Transmitter". DVI bridge doesn't tell 
> whether the device is a receiver or transmitter.
> 
>> +Required properties:
>> +	- compatible: "ti,tfp410"
> 
> The device is an I2C slave, it should have a reg property. Given that the chip 
> can be used without being controlled through I2C, the reg property should be 
> optional. You should document this clearly, and explain how the DT node can be 
> instantiated as a child of an I2C controller when the I2C interface is used, 
> or in other parts of the device tree otherwise.
> 
>> +Optional properties:
>> +	- ddc-i2c: phandle of an I2C controller used for DDC EDID probing
> 
> The TFP410 doesn't handle DDC, this property should be part of the connector 
> node.
>
>> +Optional subnodes:
>> +	- video input: this subnode can contain a video input port node
>> +	  to connect the bridge to a display controller output (See this
>> +	  documentation [1]).
> 
> You also need an output port for the DVI output. Those two ports should be 
> required, not optional.
> 

Ok. So I need another device node. Should I create some specific
compatible string for connectors behind tfp410, or a generic DVI/HDMI
connector with optional ddc-i2c phandle?

The implementation side is not so critical, because it more easily
changed, but should I create an independent generic platform-device
driver for such DVI/HDMI connector or just implement the connector side
within tfp410 driver?

>> +[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
>> +
>> +Example:
>> +	hdmi-bridge {
>> +		compatible = "ti,tfp410";
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +				bridge_in: endpoint {
>> +					remote-endpoint = <&dc_out>;
>> +				};
>> +			};
>> +		};
>> +	};
> 
> 
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index bd6acc8..a424e03 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -81,6 +81,13 @@ config DRM_TOSHIBA_TC358767
>>  	---help---
>>  	  Toshiba TC358767 eDP bridge chip driver.
>>
>> +config DRM_TI_TFP410
>> +	tristate "TI TFP410 DVI/HDMI bridge"
>> +	depends on OF
>> +	select DRM_KMS_HELPER
>> +	---help---
>> +	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
>> +
>>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>>
>>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
>> diff --git a/drivers/gpu/drm/bridge/Makefile
>> b/drivers/gpu/drm/bridge/Makefile index 97ed1a5..8b065d9 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -11,3 +11,4 @@ obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>> +obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
>> b/drivers/gpu/drm/bridge/ti-tfp410.c new file mode 100644
>> index 0000000..b0753d2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
>> @@ -0,0 +1,199 @@
>> +/*
>> + * Copyright (C) 2016 Texas Instruments
>> + * Author: Jyri Sarha <jsarha@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> by + * the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/of_graph.h>
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_crtc_helper.h>
>> +
>> +struct tfp410 {
>> +	struct drm_bridge	bridge;
>> +	struct drm_connector	connector;
>> +
>> +	struct i2c_adapter	*ddc;
>> +
>> +	struct device *dev;
>> +};
>> +
>> +static inline struct tfp410 *
>> +drm_bridge_to_tfp410(struct drm_bridge *bridge)
>> +{
>> +	return container_of(bridge, struct tfp410, bridge);
>> +}
>> +
>> +static inline struct tfp410 *
>> +drm_connector_to_tfp410(struct drm_connector *connector)
>> +{
>> +	return container_of(connector, struct tfp410, connector);
>> +}
>> +
>> +static int tfp410_get_modes(struct drm_connector *connector)
>> +{
>> +	struct tfp410 *hdmi = drm_connector_to_tfp410(connector);
>> +	struct edid *edid;
>> +	int ret;
>> +
>> +	if (!hdmi->ddc)
>> +		goto fallback;
>> +
>> +	edid = drm_get_edid(connector, hdmi->ddc);
>> +	if (!edid) {
>> +		DRM_INFO("EDID read failed. Fallback to standard modes\n");
>> +		goto fallback;
>> +	}
>> +
>> +	drm_mode_connector_update_edid_property(connector, edid);
>> +
>> +	return drm_add_edid_modes(connector, edid);
>> +fallback:
>> +	/* No EDID, fallback on the XGA standard modes */
>> +	ret = drm_add_modes_noedid(connector, 1920, 1200);
>> +
>> +	/* And prefer a mode pretty much anything can handle */
>> +	drm_set_preferred_mode(connector, 1024, 768);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct drm_connector_helper_funcs tfp410_con_helper_funcs = {
>> +	.get_modes	= tfp410_get_modes,
>> +};
>> +
>> +static enum drm_connector_status
>> +tfp410_connector_detect(struct drm_connector *connector, bool force)
>> +{
>> +	struct tfp410 *hdmi = drm_connector_to_tfp410(connector);
>> +
>> +	if (hdmi->ddc) {
>> +		if (drm_probe_ddc(hdmi->ddc))
>> +			return connector_status_connected;
>> +		else
>> +			return connector_status_disconnected;
>> +	}
>> +
>> +	return connector_status_unknown;
>> +}
>> +
>> +static const struct drm_connector_funcs tfp410_con_funcs = {
>> +	.dpms			= drm_atomic_helper_connector_dpms,
>> +	.detect			= tfp410_connector_detect,
>> +	.fill_modes		= drm_helper_probe_single_connector_modes,
>> +	.destroy		= drm_connector_cleanup,
>> +	.reset			= drm_atomic_helper_connector_reset,
>> +	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +static int tfp410_attach(struct drm_bridge *bridge)
>> +{
>> +	struct tfp410 *hdmi = drm_bridge_to_tfp410(bridge);
>> +	int ret;
>> +
>> +	if (!bridge->encoder) {
>> +		dev_err(hdmi->dev, "Missing encoder\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	drm_connector_helper_add(&hdmi->connector,
>> +				 &tfp410_con_helper_funcs);
>> +	ret = drm_connector_init(bridge->dev, &hdmi->connector,
>> +				 &tfp410_con_funcs, DRM_MODE_CONNECTOR_HDMIA);
>> +	if (ret) {
>> +		dev_err(hdmi->dev, "drm_connector_init() failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	drm_mode_connector_attach_encoder(&hdmi->connector,
>> +					  bridge->encoder);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct drm_bridge_funcs tfp410_bridge_funcs = {
>> +	.attach		= tfp410_attach,
>> +};
>> +
>> +static int tfp410_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *ddc_phandle;
>> +	struct tfp410 *hdmi;
>> +	int ret;
>> +
>> +	if (!pdev->dev.of_node) {
>> +		dev_err(&pdev->dev, "device-tree data is missing\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>> +	if (!hdmi)
>> +		return -ENOMEM;
>> +	platform_set_drvdata(pdev, hdmi);
>> +
>> +	ddc_phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c", 0);
>> +	if (ddc_phandle) {
>> +		hdmi->ddc = of_get_i2c_adapter_by_node(ddc_phandle);
>> +		of_node_put(ddc_phandle);
>> +		if (!hdmi->ddc)
>> +			return -EPROBE_DEFER;
>> +	} else {
>> +		dev_info(&pdev->dev,
>> +			 "No ddc i2c bus, disabling EDID readout\n");
>> +	}
>> +
>> +	hdmi->bridge.funcs = &tfp410_bridge_funcs;
>> +	hdmi->bridge.of_node = pdev->dev.of_node;
>> +	hdmi->dev = &pdev->dev;
>> +
>> +	ret = drm_bridge_add(&hdmi->bridge);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "drm_bridge_add() failed: %d\n", ret);
>> +		goto fail;
>> +	}
>> +
>> +	return 0;
>> +fail:
>> +	i2c_put_adapter(hdmi->ddc);
>> +	return ret;
>> +}
>> +
>> +static int tfp410_remove(struct platform_device *pdev)
>> +{
>> +	struct tfp410 *hdmi = platform_get_drvdata(pdev);
>> +
>> +	drm_bridge_remove(&hdmi->bridge);
>> +
>> +	if (hdmi->ddc)
>> +		i2c_put_adapter(hdmi->ddc);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id tfp410_match[] = {
>> +	{ .compatible = "ti,tfp410" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, tfp410_match);
>> +
>> +struct platform_driver tfp410_driver = {
>> +	.probe	= tfp410_probe,
>> +	.remove	= tfp410_remove,
>> +	.driver	= {
>> +		.name		= "tfp410-bridge",
>> +		.of_match_table	= tfp410_match,
>> +	},
>> +};
>> +module_platform_driver(tfp410_driver);
>> +
>> +MODULE_AUTHOR("Jyri Sarha <jsarha@ti.com>");
>> +MODULE_DESCRIPTION("TI TFP410 HDMI bridge driver");
>> +MODULE_LICENSE("GPL");
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Laurent Pinchart]

Hi Jyri,

On Thursday 10 Nov 2016 11:16:53 Jyri Sarha wrote:
> On 11/03/16 19:46, Laurent Pinchart wrote:
> > On Wednesday 02 Nov 2016 18:32:16 Jyri Sarha wrote:
> >> Add very basic ti-ftp410 HDMI transmitter driver. The only feature
> >> separating this from a completely dummy bridge is the DDC i2c
> >> support. However, other HW specific features may be added later when
> >> needed. For instance there is a set of registers behind i2c if it is
> >> connected. The implementations is tested against my new tilcdc bridge
> >> support and works with BeagleBone DVI-D Cape Rev A3. A DT binding
> >> document is also added.
> >> 
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >> 
> >> .../bindings/display/bridge/ti,tfp410.txt          |  30 ++++
> >> drivers/gpu/drm/bridge/Kconfig                     |   7 +
> >> drivers/gpu/drm/bridge/Makefile                    |   1 +
> >> drivers/gpu/drm/bridge/ti-tfp410.c                 | 199 +++++++++++++++
> >> 4 files changed, 237 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> >> create mode 100644 drivers/gpu/drm/bridge/ti-tfp410.c
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> >> b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt new file
> >> mode 100644
> >> index 0000000..dc93713
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
> >> @@ -0,0 +1,30 @@
> >> +TFP410 HDMI/DVI bridge bindings
> > 
> > I'd name the document "TI TFP410 DVI Transmitter". DVI bridge doesn't tell
> > whether the device is a receiver or transmitter.
> > 
> >> +Required properties:
> >> +	- compatible: "ti,tfp410"
> > 
> > The device is an I2C slave, it should have a reg property. Given that the
> > chip can be used without being controlled through I2C, the reg property
> > should be optional. You should document this clearly, and explain how the
> > DT node can be instantiated as a child of an I2C controller when the I2C
> > interface is used, or in other parts of the device tree otherwise.
> > 
> >> +Optional properties:
> >> +	- ddc-i2c: phandle of an I2C controller used for DDC EDID probing
> > 
> > The TFP410 doesn't handle DDC, this property should be part of the
> > connector node.
> > 
> >> +Optional subnodes:
> >> +	- video input: this subnode can contain a video input port node
> >> +	  to connect the bridge to a display controller output (See this
> >> +	  documentation [1]).
> > 
> > You also need an output port for the DVI output. Those two ports should be
> > required, not optional.
> 
> Ok. So I need another device node. Should I create some specific
> compatible string for connectors behind tfp410, or a generic DVI/HDMI
> connector with optional ddc-i2c phandle?

The generic DVI/HDMI connector bindings should work fine.

> The implementation side is not so critical, because it more easily
> changed, but should I create an independent generic platform-device
> driver for such DVI/HDMI connector or just implement the connector side
> within tfp410 driver?

Longer term I'd like to go for connector drivers, but it might take a bit of 
infrastructure work. If you can give it a try it would be great ! Otherwise 
I'm fine with handling that in the tfp410 driver for now.

> >> +[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
> >> +
> >> +Example:
> >> +	hdmi-bridge {
> >> +		compatible = "ti,tfp410";
> >> +		ports {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >> +
> >> +			port@0 {
> >> +				reg = <0>;
> >> +				bridge_in: endpoint {
> >> +					remote-endpoint = <&dc_out>;
> >> +				};
> >> +			};
> >> +		};
> >> +	};

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 3032 bytes --]

On 10/11/16 11:16, Jyri Sarha wrote:
> On 11/03/16 19:46, Laurent Pinchart wrote:
>> Hi Jyri,
>>
>> Thank you for the patch.
>>
>> On Wednesday 02 Nov 2016 18:32:16 Jyri Sarha wrote:
>>> Add very basic ti-ftp410 HDMI transmitter driver. The only feature
>>> separating this from a completely dummy bridge is the DDC i2c
>>> support. However, other HW specific features may be added later when
>>> needed. For instance there is a set of registers behind i2c if it is
>>> connected. The implementations is tested against my new tilcdc bridge
>>> support and works with BeagleBone DVI-D Cape Rev A3. A DT binding
>>> document is also added.
>>>
>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>> ---
>>>  .../bindings/display/bridge/ti,tfp410.txt          |  30 ++++
>>>  drivers/gpu/drm/bridge/Kconfig                     |   7 +
>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>  drivers/gpu/drm/bridge/ti-tfp410.c                 | 199 ++++++++++++++++++
>>>  4 files changed, 237 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt create mode
>>> 100644 drivers/gpu/drm/bridge/ti-tfp410.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>>> b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt new file
>>> mode 100644
>>> index 0000000..dc93713
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt
>>> @@ -0,0 +1,30 @@
>>> +TFP410 HDMI/DVI bridge bindings
>>
>> I'd name the document "TI TFP410 DVI Transmitter". DVI bridge doesn't tell 
>> whether the device is a receiver or transmitter.
>>
>>> +Required properties:
>>> +	- compatible: "ti,tfp410"
>>
>> The device is an I2C slave, it should have a reg property. Given that the chip 
>> can be used without being controlled through I2C, the reg property should be 
>> optional. You should document this clearly, and explain how the DT node can be 
>> instantiated as a child of an I2C controller when the I2C interface is used, 
>> or in other parts of the device tree otherwise.
>>
>>> +Optional properties:
>>> +	- ddc-i2c: phandle of an I2C controller used for DDC EDID probing
>>
>> The TFP410 doesn't handle DDC, this property should be part of the connector 
>> node.
>>
>>> +Optional subnodes:
>>> +	- video input: this subnode can contain a video input port node
>>> +	  to connect the bridge to a display controller output (See this
>>> +	  documentation [1]).
>>
>> You also need an output port for the DVI output. Those two ports should be 
>> required, not optional.
>>
> 
> Ok. So I need another device node. Should I create some specific
> compatible string for connectors behind tfp410, or a generic DVI/HDMI
> connector with optional ddc-i2c phandle?

omapdrm uses connector nodes. See, for example, pandaboard's dts files.
It has TFP410 and a connector node. With omap specific drivers, for now,
but that's another matter.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Jyri Sarha]

On 11/03/16 19:46, Laurent Pinchart wrote:
>> +Required properties:
>> > +	- compatible: "ti,tfp410"
> The device is an I2C slave, it should have a reg property. Given that the chip 
> can be used without being controlled through I2C, the reg property should be 
> optional. You should document this clearly, and explain how the DT node can be 
> instantiated as a child of an I2C controller when the I2C interface is used, 
> or in other parts of the device tree otherwise.
> 

Shouldn't I have two different compatible strings if want to make both
platform driver probe and i2c client probe to work?

Or can it be done with single compatible string? Would you know of an
example of such a driver?

Cheers,
Jyri
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Laurent Pinchart]

Hi Jyri,

On Monday 14 Nov 2016 10:49:43 Jyri Sarha wrote:
> On 11/03/16 19:46, Laurent Pinchart wrote:
> >> +Required properties:
> >> > +	- compatible: "ti,tfp410"
> > 
> > The device is an I2C slave, it should have a reg property. Given that the
> > chip can be used without being controlled through I2C, the reg property
> > should be optional. You should document this clearly, and explain how the
> > DT node can be instantiated as a child of an I2C controller when the I2C
> > interface is used, or in other parts of the device tree otherwise.
> 
> Shouldn't I have two different compatible strings if want to make both
> platform driver probe and i2c client probe to work?

I don't think so, it's still the same chip.

> Or can it be done with single compatible string? Would you know of an
> example of such a driver?

You will need to register both a i2c_driver and a platform_driver in the 
tfp410 driver. Both will advertise the same compatible string. As you'll have 
two probe functions, it should be easy to handle the differences between the 
two situations there, with common code shared in common functions. A quick 
grep points to at least drivers/power/bq27x00_battery.c as an example (albeit 
without DT support).

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1481 bytes --]

On 14/11/16 13:10, Laurent Pinchart wrote:
> Hi Jyri,
> 
> On Monday 14 Nov 2016 10:49:43 Jyri Sarha wrote:
>> On 11/03/16 19:46, Laurent Pinchart wrote:
>>>> +Required properties:
>>>>> +	- compatible: "ti,tfp410"
>>>
>>> The device is an I2C slave, it should have a reg property. Given that the
>>> chip can be used without being controlled through I2C, the reg property
>>> should be optional. You should document this clearly, and explain how the
>>> DT node can be instantiated as a child of an I2C controller when the I2C
>>> interface is used, or in other parts of the device tree otherwise.
>>
>> Shouldn't I have two different compatible strings if want to make both
>> platform driver probe and i2c client probe to work?
> 
> I don't think so, it's still the same chip.
> 
>> Or can it be done with single compatible string? Would you know of an
>> example of such a driver?
> 
> You will need to register both a i2c_driver and a platform_driver in the 
> tfp410 driver. Both will advertise the same compatible string. As you'll have 
> two probe functions, it should be easy to handle the differences between the 

If you have the same compatible string, won't both probes trigger? If
so, how does, e.g., the platform driver know this is actually i2c case,
and bail out? And if both probes don't trigger, why not? How does the
device probing machinery know that this DT node is actually an i2c node,
not a platform device node?

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver

[Laurent Pinchart]

Hi Tomi,

On Monday 14 Nov 2016 13:16:49 Tomi Valkeinen wrote:
> On 14/11/16 13:10, Laurent Pinchart wrote:
> > On Monday 14 Nov 2016 10:49:43 Jyri Sarha wrote:
> >> On 11/03/16 19:46, Laurent Pinchart wrote:
> >>>> +Required properties:
> >>>>> +	- compatible: "ti,tfp410"
> >>> 
> >>> The device is an I2C slave, it should have a reg property. Given that
> >>> the chip can be used without being controlled through I2C, the reg
> >>> property should be optional. You should document this clearly, and
> >>> explain how the DT node can be instantiated as a child of an I2C
> >>> controller when the I2C interface is used, or in other parts of the
> >>> device tree otherwise.
> >> 
> >> Shouldn't I have two different compatible strings if want to make both
> >> platform driver probe and i2c client probe to work?
> > 
> > I don't think so, it's still the same chip.
> > 
> >> Or can it be done with single compatible string? Would you know of an
> >> example of such a driver?
> > 
> > You will need to register both a i2c_driver and a platform_driver in the
> > tfp410 driver. Both will advertise the same compatible string. As you'll
> > have two probe functions, it should be easy to handle the differences
> > between the
>
> If you have the same compatible string, won't both probes trigger? If
> so, how does, e.g., the platform driver know this is actually i2c case,
> and bail out? And if both probes don't trigger, why not? How does the
> device probing machinery know that this DT node is actually an i2c node,
> not a platform device node?

The driver for the bus on which the device is sitting is responsible for 
instantiating a struct device corresponding to the bus type, and for binding 
the corresponding bus drivers. This will be a struct i2c_client for I2C buses, 
and a struct platform_device for platform buses. Only the corresponding driver 
type will be probed.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html