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@xxxxxx> >> --- >> .../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@xxxxxx> >> + * >> + * 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@xxxxxx>"); >> +MODULE_DESCRIPTION("TI TFP410 HDMI bridge driver"); >> +MODULE_LICENSE("GPL"); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html