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 at 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 at 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 at 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 at ti.com>");
>> +MODULE_DESCRIPTION("TI TFP410 HDMI bridge driver");
>> +MODULE_LICENSE("GPL");
>
