2013/10/4 Sean Paul <seanpaul@xxxxxxxxxxxx>: > On Thu, Oct 3, 2013 at 2:23 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >> 2013/10/4 Sean Paul <seanpaul@xxxxxxxxxxxx>: >>> On Thu, Oct 3, 2013 at 1:39 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >>>> 2013/10/3 Sean Paul <seanpaul@xxxxxxxxxxxx>: >>>>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >>>>>> Hi, thank you for your contribution and the below is my short comments, >>>>>> >>>>>> 2013/10/2 Sean Paul <seanpaul@xxxxxxxxxxxx>: >>>>>>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS >>>>>>> bridge chip. >>>>>>> >>>>>>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> >>>>>>> --- >>>>>>> .../devicetree/bindings/drm/bridge/ptn3460.txt | 27 ++ >>>>>>> drivers/gpu/drm/Kconfig | 2 + >>>>>>> drivers/gpu/drm/Makefile | 1 + >>>>>>> drivers/gpu/drm/bridge/Kconfig | 4 + >>>>>>> drivers/gpu/drm/bridge/Makefile | 3 + >>>>>>> drivers/gpu/drm/bridge/ptn3460.c | 349 +++++++++++++++++++++ >>>>>>> include/drm/bridge/ptn3460.h | 36 +++ >>>>>>> 7 files changed, 422 insertions(+) >>>>>>> create mode 100644 Documentation/devicetree/bindings/drm/bridge/ptn3460.txt >>>>>>> create mode 100644 drivers/gpu/drm/bridge/Kconfig >>>>>>> create mode 100644 drivers/gpu/drm/bridge/Makefile >>>>>>> create mode 100644 drivers/gpu/drm/bridge/ptn3460.c >>>>>>> create mode 100644 include/drm/bridge/ptn3460.h >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/drm/bridge/ptn3460.txt b/Documentation/devicetree/bindings/drm/bridge/ptn3460.txt >>>>>>> new file mode 100644 >>>>>>> index 0000000..c1cd329 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/drm/bridge/ptn3460.txt >>>>>>> @@ -0,0 +1,27 @@ >>>>>>> +ptn3460-bridge bindings >>>>>>> + >>>>>>> +Required properties: >>>>>>> + - compatible: "nxp,ptn3460" >>>>>>> + - reg: i2c address of the bridge >>>>>>> + - powerdown-gpio: OF device-tree gpio specification >>>>>> >>>>>> Can a regulator be used instead of gpio in other board case? >>>>>> >>>>> >>>>> No, not to my knowledge. >>>>> >>>> >>>> Hm.. plz check it out again. the gpio pin is specific to board, and >>>> the the gpio be used as power source trigger could be replaced with a >>>> regulator according to board design. So you should consider all >>>> possibilities even though there are no other cases yet: other board >>>> could use a regulator instead. >>>> >>>>> >>>>>>> + - reset-gpio: OF device-tree gpio specification >>>>>>> + - edid-emulation: The EDID emulation entry to use >>>>>>> + +-------+------------+------------------+ >>>>>>> + | Value | Resolution | Description | >>>>>>> + | 0 | 1024x768 | NXP Generic | >>>>>>> + | 1 | 1920x1080 | NXP Generic | >>>>>>> + | 2 | 1920x1080 | NXP Generic | >>>>>>> + | 3 | 1600x900 | Samsung LTM200KT | >>>>>>> + | 4 | 1920x1080 | Samsung LTM230HT | >>>>>>> + | 5 | 1366x768 | NXP Generic | >>>>>>> + | 6 | 1600x900 | ChiMei M215HGE | >>>>>>> + +-------+------------+------------------+ >>>>>>> + >>>>>>> +Example: >>>>>>> + ptn3460-bridge@20 { >>>>>>> + compatible = "nxp,ptn3460"; >>>>>>> + reg = <0x20>; >>>>>>> + powerdown-gpio = <&gpy2 5 1 0 0>; >>>>>>> + reset-gpio = <&gpx1 5 1 0 0>; >>>>>>> + edid-emulation = <5>; >>>>>>> + }; >>>>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>>>>>> index 955555d..cd7bfb3 100644 >>>>>>> --- a/drivers/gpu/drm/Kconfig >>>>>>> +++ b/drivers/gpu/drm/Kconfig >>>>>>> @@ -236,3 +236,5 @@ source "drivers/gpu/drm/tilcdc/Kconfig" >>>>>>> source "drivers/gpu/drm/qxl/Kconfig" >>>>>>> >>>>>>> source "drivers/gpu/drm/msm/Kconfig" >>>>>>> + >>>>>>> +source "drivers/gpu/drm/bridge/Kconfig" >>>>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >>>>>>> index f089adf..9234253 100644 >>>>>>> --- a/drivers/gpu/drm/Makefile >>>>>>> +++ b/drivers/gpu/drm/Makefile >>>>>>> @@ -56,3 +56,4 @@ obj-$(CONFIG_DRM_TILCDC) += tilcdc/ >>>>>>> obj-$(CONFIG_DRM_QXL) += qxl/ >>>>>>> obj-$(CONFIG_DRM_MSM) += msm/ >>>>>>> obj-y += i2c/ >>>>>>> +obj-y += bridge/ >>>>>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >>>>>>> new file mode 100644 >>>>>>> index 0000000..f8db069 >>>>>>> --- /dev/null >>>>>>> +++ b/drivers/gpu/drm/bridge/Kconfig >>>>>>> @@ -0,0 +1,4 @@ >>>>>>> +config DRM_PTN3460 >>>>>>> + tristate "PTN3460 DP/LVDS bridge" >>>>>>> + depends on DRM && I2C >>>>>>> + ---help--- >>>>>>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile >>>>>>> new file mode 100644 >>>>>>> index 0000000..b4733e1 >>>>>>> --- /dev/null >>>>>>> +++ b/drivers/gpu/drm/bridge/Makefile >>>>>>> @@ -0,0 +1,3 @@ >>>>>>> +ccflags-y := -Iinclude/drm >>>>>>> + >>>>>>> +obj-$(CONFIG_DRM_PTN3460) += ptn3460.o >>>>>>> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c >>>>>>> new file mode 100644 >>>>>>> index 0000000..a9e5c1a >>>>>>> --- /dev/null >>>>>>> +++ b/drivers/gpu/drm/bridge/ptn3460.c >>>>>>> @@ -0,0 +1,349 @@ >>>>>>> +/* >>>>>>> + * NXP PTN3460 DP/LVDS bridge driver >>>>>>> + * >>>>>>> + * Copyright (C) 2013 Google, Inc. >>>>>>> + * >>>>>>> + * This software is licensed under the terms of the GNU General Public >>>>>>> + * License version 2, as published by the Free Software Foundation, and >>>>>>> + * may be copied, distributed, and modified under those terms. >>>>>>> + * >>>>>>> + * This program is distributed in the hope that it will be useful, >>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>>>> + * GNU General Public License for more details. >>>>>>> + */ >>>>>>> + >>>>>>> +#include <linux/module.h> >>>>>>> +#include <linux/of.h> >>>>>>> +#include <linux/of_gpio.h> >>>>>>> +#include <linux/i2c.h> >>>>>>> +#include <linux/gpio.h> >>>>>>> +#include <linux/delay.h> >>>>>>> + >>>>>>> +#include "drmP.h" >>>>>>> +#include "drm_edid.h" >>>>>>> +#include "drm_crtc.h" >>>>>>> +#include "drm_crtc_helper.h" >>>>>>> + >>>>>>> +#include "bridge/ptn3460.h" >>>>>>> + >>>>>>> +#define PTN3460_EDID_ADDR 0x0 >>>>>>> +#define PTN3460_EDID_EMULATION_ADDR 0x84 >>>>>>> +#define PTN3460_EDID_ENABLE_EMULATION 0 >>>>>>> +#define PTN3460_EDID_EMULATION_SELECTION 1 >>>>>>> +#define PTN3460_EDID_SRAM_LOAD_ADDR 0x85 >>>>>>> + >>>>>>> +struct ptn3460_bridge { >>>>>>> + struct drm_connector connector; >>>>>>> + struct i2c_client *client; >>>>>>> + struct drm_encoder *encoder; >>>>>>> + struct drm_bridge *bridge; >>>>>>> + struct edid *edid; >>>>>>> + int gpio_pd_n; >>>>>>> + int gpio_rst_n; >>>>>>> + u32 edid_emulation; >>>>>>> + bool enabled; >>>>>>> +}; >>>>>>> + >>>>>>> +static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, >>>>>>> + u8 *buf, int len) >>>>>>> +{ >>>>>>> + int ret; >>>>>>> + >>>>>>> + ret = i2c_master_send(ptn_bridge->client, &addr, 1); >>>>>>> + if (ret <= 0) { >>>>>>> + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + >>>>>>> + ret = i2c_master_recv(ptn_bridge->client, buf, len); >>>>>>> + if (ret <= 0) { >>>>>>> + DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, >>>>>>> + char val) >>>>>>> +{ >>>>>>> + int ret; >>>>>>> + char buf[2]; >>>>>>> + >>>>>>> + buf[0] = addr; >>>>>>> + buf[1] = val; >>>>>>> + >>>>>>> + ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); >>>>>>> + if (ret <= 0) { >>>>>>> + DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) >>>>>>> +{ >>>>>>> + int ret; >>>>>>> + char val; >>>>>>> + >>>>>>> + /* Load the selected edid into SRAM (accessed at PTN3460_EDID_ADDR) */ >>>>>>> + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_SRAM_LOAD_ADDR, >>>>>>> + ptn_bridge->edid_emulation); >>>>>>> + if (ret) { >>>>>>> + DRM_ERROR("Failed to transfer edid to sram, ret=%d\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + >>>>>>> + /* Enable EDID emulation and select the desired EDID */ >>>>>>> + val = 1 << PTN3460_EDID_ENABLE_EMULATION | >>>>>>> + ptn_bridge->edid_emulation << PTN3460_EDID_EMULATION_SELECTION; >>>>>>> + >>>>>>> + ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_EMULATION_ADDR, val); >>>>>>> + if (ret) { >>>>>>> + DRM_ERROR("Failed to write edid value, ret=%d\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +static void ptn3460_pre_enable(struct drm_bridge *bridge) >>>>>>> +{ >>>>>>> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; >>>>>>> + int ret; >>>>>>> + >>>>>>> + if (ptn_bridge->enabled) >>>>>>> + return; >>>>>>> + >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_pd_n)) >>>>>>> + gpio_set_value(ptn_bridge->gpio_pd_n, 1); >>>>>> >>>>>> Ditto. >>>>>> >>>>>>> + >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_rst_n)) { >>>>>>> + gpio_set_value(ptn_bridge->gpio_rst_n, 0); >>>>>>> + udelay(10); >>>>>>> + gpio_set_value(ptn_bridge->gpio_rst_n, 1); >>>>>>> + } >>>>>>> + >>>>>>> + /* >>>>>>> + * There's a bug in the PTN chip where it falsely asserts hotplug before >>>>>>> + * it is fully functional. We're forced to wait for the maximum start up >>>>>>> + * time specified in the chip's datasheet to make sure we're really up. >>>>>>> + */ >>>>>>> + msleep(90); >>>>>>> + >>>>>>> + ret = ptn3460_select_edid(ptn_bridge); >>>>>>> + if (ret) >>>>>>> + DRM_ERROR("Select edid failed ret=%d\n", ret); >>>>>>> + >>>>>>> + ptn_bridge->enabled = true; >>>>>>> +} >>>>>>> + >>>>>>> +static void ptn3460_enable(struct drm_bridge *bridge) >>>>>>> +{ >>>>>>> +} >>>>>>> + >>>>>>> +static void ptn3460_disable(struct drm_bridge *bridge) >>>>>>> +{ >>>>>>> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; >>>>>>> + >>>>>>> + if (!ptn_bridge->enabled) >>>>>>> + return; >>>>>>> + >>>>>>> + ptn_bridge->enabled = false; >>>>>>> + >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_rst_n)) >>>>>>> + gpio_set_value(ptn_bridge->gpio_rst_n, 1); >>>>>>> + >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_pd_n)) >>>>>>> + gpio_set_value(ptn_bridge->gpio_pd_n, 0); >>>>>> >>>>>> Ditto. >>>>>> >>>>>>> +} >>>>>>> + >>>>>>> +static void ptn3460_post_disable(struct drm_bridge *bridge) >>>>>>> +{ >>>>>>> +} >>>>>>> + >>>>>>> +void ptn3460_bridge_destroy(struct drm_bridge *bridge) >>>>>>> +{ >>>>>>> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; >>>>>>> + >>>>>>> + drm_bridge_cleanup(bridge); >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_pd_n)) >>>>>>> + gpio_free(ptn_bridge->gpio_pd_n); >>>>>> >>>>>> Ditto. >>>>>> >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_rst_n)) >>>>>>> + gpio_free(ptn_bridge->gpio_rst_n); >>>>>>> + /* Nothing else to free, we've got devm allocated memory */ >>>>>>> +} >>>>>>> + >>>>>>> +struct drm_bridge_funcs ptn3460_bridge_funcs = { >>>>>>> + .pre_enable = ptn3460_pre_enable, >>>>>>> + .enable = ptn3460_enable, >>>>>>> + .disable = ptn3460_disable, >>>>>>> + .post_disable = ptn3460_post_disable, >>>>>>> + .destroy = ptn3460_bridge_destroy, >>>>>>> +}; >>>>>>> + >>>>>>> +int ptn3460_get_modes(struct drm_connector *connector) >>>>>>> +{ >>>>>>> + struct ptn3460_bridge *ptn_bridge; >>>>>>> + u8 *edid; >>>>>>> + int ret, num_modes; >>>>>>> + bool power_off; >>>>>>> + >>>>>>> + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); >>>>>>> + >>>>>>> + if (ptn_bridge->edid) >>>>>>> + return drm_add_edid_modes(connector, ptn_bridge->edid); >>>>>>> + >>>>>>> + power_off = !ptn_bridge->enabled; >>>>>>> + ptn3460_pre_enable(ptn_bridge->bridge); >>>>>>> + >>>>>>> + edid = kmalloc(EDID_LENGTH, GFP_KERNEL); >>>>>>> + if (!edid) { >>>>>>> + DRM_ERROR("Failed to allocate edid\n"); >>>>>>> + return 0; >>>>>>> + } >>>>>>> + >>>>>>> + ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid, >>>>>>> + EDID_LENGTH); >>>>>>> + if (ret) { >>>>>>> + kfree(edid); >>>>>>> + num_modes = 0; >>>>>>> + goto out; >>>>>>> + } >>>>>>> + >>>>>>> + ptn_bridge->edid = (struct edid *)edid; >>>>>>> + drm_mode_connector_update_edid_property(connector, ptn_bridge->edid); >>>>>>> + >>>>>>> + num_modes = drm_add_edid_modes(connector, ptn_bridge->edid); >>>>>>> + >>>>>>> +out: >>>>>>> + if (power_off) >>>>>>> + ptn3460_disable(ptn_bridge->bridge); >>>>>>> + >>>>>>> + return num_modes; >>>>>>> +} >>>>>>> + >>>>>>> +static int ptn3460_mode_valid(struct drm_connector *connector, >>>>>>> + struct drm_display_mode *mode) >>>>>>> +{ >>>>>>> + return MODE_OK; >>>>>>> +} >>>>>>> + >>>>>>> +struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector) >>>>>>> +{ >>>>>>> + struct ptn3460_bridge *ptn_bridge; >>>>>>> + >>>>>>> + ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); >>>>>>> + >>>>>>> + return ptn_bridge->encoder; >>>>>>> +} >>>>>>> + >>>>>>> +struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = { >>>>>>> + .get_modes = ptn3460_get_modes, >>>>>>> + .mode_valid = ptn3460_mode_valid, >>>>>>> + .best_encoder = ptn3460_best_encoder, >>>>>>> +}; >>>>>>> + >>>>>>> +enum drm_connector_status ptn3460_detect(struct drm_connector *connector, >>>>>>> + bool force) >>>>>>> +{ >>>>>>> + return connector_status_connected; >>>>>>> +} >>>>>>> + >>>>>>> +void ptn3460_connector_destroy(struct drm_connector *connector) >>>>>>> +{ >>>>>>> + drm_connector_cleanup(connector); >>>>>>> +} >>>>>>> + >>>>>>> +struct drm_connector_funcs ptn3460_connector_funcs = { >>>>>>> + .dpms = drm_helper_connector_dpms, >>>>>>> + .fill_modes = drm_helper_probe_single_connector_modes, >>>>>>> + .detect = ptn3460_detect, >>>>>>> + .destroy = ptn3460_connector_destroy, >>>>>>> +}; >>>>>> >>>>>> Why do you try to add a new connector here? We already have the >>>>>> connector for LCD, and also provides some callbacks for it. For this, >>>>>> please see exynos_drm_display_ops of exynos_drm_fimd driver, and you >>>>>> can add new callbacks to there such as init callback for bridge device >>>>>> initialization if needed. >>>>>> >>>>> >>>>> We add a new connector for 2 reasons: >>>>> >>>>> 1) We need to override the drm detect() callback to always return true >>>>> since the DP driver will presumably return its hotplug status which >>>>> will always be low when the ptn chip is turned off. >>>>> 2) We want the ability to control the result of get_modes(). >>>>> >>>>> I've got a patch set almost ready to tear the display ops out of fimd >>>>> and put them in the DP driver. >>>> >>>> Really? if so, that is ideal something we want and we should go. But >>>> isn't the DP driver placed in drivers/video/exynos? How did you take >>>> care of that? >>> >>> git mv :) >>> >> >> :) >> >> >>>> Actually, for this, we planned to use CDF(Common Display >>>> Framework) if the framework is merged to mainline somehow. >>>> >>> >>> Right. I think CDF will end up being a series of improvements to drm, >>> as opposed to its own framework (at least this was the conclusion I >>> came to after speaking with Laurent at the plumbers conference). I >>> don't think it makes sense to have the DP driver outside of drm. The >>> HDMI driver is already inside drm, the DP driver should be too. >> >> See the below, >> >> Application >> -------------------------------------------------------------- >> v4l2 drm kms >> hdmi driver hdmi driver >> ------------------------------------------------------------- >> hdmi hw >> >> HDMI is a controller can be controlled by user application, and for >> this some frameworks such as v4l2 and drm kms interfaces are used. But >> DP, MIPI-DSI, LVDS, and so on aren't controlled by user application. >> These are just display bus between scanout devices(hdmi, fimd) and lcd >> panel. So the above your example doesn't make sense. >> > > I think we've probably gone far enough off-topic. Let's discuss this > when you've had an opportunity to see the patchset. I hope the code > will speak for itself. > Ok, let's have a discussion after looking into the codes. > Aside from Olof's suggestion about changing the dts binding to > lvds-bridge (which I'll upload a new patch for), do you have any > suggestions for this patch? > lvds-bridge is better. It seems reasonable to me. > Sean > > >>> Regardless, this conversation is only tangentially related to this >>> patch and can probably be deferred. >>> >>> Sean >>> >>>>> The display ops are better suited >>>>> there, since it's the actual encoder/connector. I hope to get that >>>>> posted this week. >>>>> >>>> >>>> I will look forward to that posting. :) >>>> >>>>> >>>>>>> + >>>>>>> +int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, >>>>>>> + struct i2c_client *client, struct device_node *node) >>>>>>> +{ >>>>>>> + int ret; >>>>>>> + struct drm_bridge *bridge; >>>>>>> + struct ptn3460_bridge *ptn_bridge; >>>>>>> + >>>>>>> + bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL); >>>>>>> + if (!bridge) { >>>>>>> + DRM_ERROR("Failed to allocate drm bridge\n"); >>>>>>> + return -ENOMEM; >>>>>>> + } >>>>>>> + >>>>>>> + ptn_bridge = devm_kzalloc(dev->dev, sizeof(*ptn_bridge), GFP_KERNEL); >>>>>>> + if (!ptn_bridge) { >>>>>>> + DRM_ERROR("Failed to allocate ptn bridge\n"); >>>>>>> + return -ENOMEM; >>>>>>> + } >>>>>>> + >>>>>>> + ptn_bridge->client = client; >>>>>>> + ptn_bridge->encoder = encoder; >>>>>>> + ptn_bridge->bridge = bridge; >>>>>>> + ptn_bridge->gpio_pd_n = of_get_named_gpio(node, "powerdown-gpio", 0); >>>>>> >>>>>> Also, if a regulator is used instead? >>>>>> >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_pd_n)) { >>>>>>> + ret = gpio_request_one(ptn_bridge->gpio_pd_n, >>>>>>> + GPIOF_OUT_INIT_HIGH, "PTN3460_PD_N"); >>>>>>> + if (ret) { >>>>>>> + DRM_ERROR("Request powerdown-gpio failed (%d)\n", ret); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + } >>>>>>> + >>>>>>> + ptn_bridge->gpio_rst_n = of_get_named_gpio(node, "reset-gpio", 0); >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_rst_n)) { >>>>>>> + /* >>>>>>> + * Request the reset pin low to avoid the bridge being >>>>>>> + * initialized prematurely >>>>>>> + */ >>>>>>> + ret = gpio_request_one(ptn_bridge->gpio_rst_n, >>>>>>> + GPIOF_OUT_INIT_LOW, "PTN3460_RST_N"); >>>>>>> + if (ret) { >>>>>>> + DRM_ERROR("Request reset-gpio failed (%d)\n", ret); >>>>>>> + gpio_free(ptn_bridge->gpio_pd_n); >>>>>>> + return ret; >>>>>>> + } >>>>>>> + } >>>>>>> + >>>>>>> + ret = of_property_read_u32(node, "edid-emulation", >>>>>>> + &ptn_bridge->edid_emulation); >>>>>>> + if (ret) { >>>>>>> + DRM_ERROR("Can't read edid emulation value\n"); >>>>>>> + goto err; >>>>>>> + } >>>>>>> + >>>>>>> + ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs); >>>>>>> + if (ret) { >>>>>>> + DRM_ERROR("Failed to initialize bridge with drm\n"); >>>>>>> + goto err; >>>>>>> + } >>>>>>> + >>>>>>> + bridge->driver_private = ptn_bridge; >>>>>>> + encoder->bridge = bridge; >>>>>>> + >>>>>>> + ret = drm_connector_init(dev, &ptn_bridge->connector, >>>>>>> + &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS); >>>>>> >>>>>> So it seems that here's not right place to call drm_connector_init function. >>>>>> >>>>>> Display pipeline path could be one of, >>>>>> Display Controller Display bus >>>>>> --------------------------------------------------------------------------- >>>>>> FIMD---------------------LVDS--------------------LCD, >>>>>> or >>>>>> FIMD----------------------eDP---------------------LCD, >>>>>> or >>>>>> FIMD------------------MIPI-DSI------------------LCD, >>>>>> or >>>>>> FIMD-------------------------------------------------LCD >>>>>> >>>>>> And also in case using image enhancement chip, >>>>>> mDNIe-------------FIMD-LITE between Display Controller and Display >>>>>> bus. >>>>>> >>>>>> So, wouldn't the right place below FIMD driver? :) >>>>>> >>>>> >>>>> Well, this driver should be considered outside of exynos context since >>>>> it could be used by any drm driver. >>>>> >>>>> In the exynos context, the right place to implement it would be in the >>>>> dp driver, actually. However, the exynos driver has a level of >>>>> abstraction on top of the crtcs/encoders such that we need to >>>>> initialize it in the exynos_drm_core. The patchset I mentioned above >>>>> should help move things in a direction where fimd/mixer implement >>>>> drm_crtc directly and hdmi/dp implement drm_encoder/drm_connector >>>>> directly. In that world, DP would initialize the ptn driver. >>>>> >>>>>> >>>>>>> + if (ret) { >>>>>>> + DRM_ERROR("Failed to initialize connector with drm\n"); >>>>>>> + goto err; >>>>>>> + } >>>>>>> + drm_connector_helper_add(&ptn_bridge->connector, >>>>>>> + &ptn3460_connector_helper_funcs); >>>>>>> + drm_sysfs_connector_add(&ptn_bridge->connector); >>>>>>> + drm_mode_connector_attach_encoder(&ptn_bridge->connector, encoder); >>>>>>> + >>>>>>> + return 0; >>>>>>> + >>>>>>> +err: >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_pd_n)) >>>>>>> + gpio_free(ptn_bridge->gpio_pd_n); >>>>>>> + if (gpio_is_valid(ptn_bridge->gpio_rst_n)) >>>>>>> + gpio_free(ptn_bridge->gpio_rst_n); >>>>>>> + return ret; >>>>>>> +} >>>>>>> diff --git a/include/drm/bridge/ptn3460.h b/include/drm/bridge/ptn3460.h >>>>>>> new file mode 100644 >>>>>>> index 0000000..157ffa1 >>>>>>> --- /dev/null >>>>>>> +++ b/include/drm/bridge/ptn3460.h >>>>>>> @@ -0,0 +1,36 @@ >>>>>>> +/* >>>>>>> + * Copyright (C) 2013 Google, Inc. >>>>>>> + * >>>>>>> + * This software is licensed under the terms of the GNU General Public >>>>>>> + * License version 2, as published by the Free Software Foundation, and >>>>>>> + * may be copied, distributed, and modified under those terms. >>>>>>> + * >>>>>>> + * This program is distributed in the hope that it will be useful, >>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>>>> + * GNU General Public License for more details. >>>>>>> + */ >>>>>>> + >>>>>>> +#ifndef _DRM_BRIDGE_PTN3460_H_ >>>>>>> +#define _DRM_BRIDGE_PTN3460_H_ >>>>>>> + >>>>>>> +struct drm_device; >>>>>>> +struct drm_encoder; >>>>>>> +struct i2c_client; >>>>>>> +struct device_node; >>>>>>> + >>>>>>> +#ifdef CONFIG_DRM_PTN3460 >>>>>>> + >>>>>>> +int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, >>>>>>> + struct i2c_client *client, struct device_node *node); >>>>>>> +#else >>>>>>> + >>>>>>> +int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, >>>>>>> + struct i2c_client *client, struct device_node *node) >>>>>>> +{ >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +#endif >>>>>>> + >>>>>>> +#endif >>>>>>> -- >>>>>>> 1.8.4 >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> linux-arm-kernel mailing list >>>>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx >>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@xxxxxxxxxxxxxxxxxxxxx >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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