Devicetree

* Re: [PATCH 2/3] drm/bridge: Add ti-ftp410 HDMI transmitter driver
From: Laurent Pinchart @ 2016-11-03 17:46 UTC (permalink / raw)
  To: Jyri Sarha
  Cc: devicetree, bcousson, khilman, dri-devel, bgolaszewski,
	tomi.valkeinen
In-Reply-To: <9b6a6a4405de344cb9fbea735fea37adee1d726e.1478103726.git.jsarha@ti.com>

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

^ permalink raw reply