Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754755Ab3JCScg (ORCPT ); Thu, 3 Oct 2013 14:32:36 -0400 Received: from mail-ie0-f180.google.com ([209.85.223.180]:59627 "EHLO mail-ie0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753893Ab3JCScc (ORCPT ); Thu, 3 Oct 2013 14:32:32 -0400 MIME-Version: 1.0 In-Reply-To: References: <1380670860-17621-1-git-send-email-seanpaul@chromium.org> <1380670860-17621-4-git-send-email-seanpaul@chromium.org> From: Sean Paul Date: Thu, 3 Oct 2013 14:32:09 -0400 Message-ID: Subject: Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver To: Inki Dae Cc: "devicetree@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , DRI mailing list , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 25549 Lines: 653 On Thu, Oct 3, 2013 at 2:23 PM, Inki Dae wrote: > 2013/10/4 Sean Paul : >> On Thu, Oct 3, 2013 at 1:39 PM, Inki Dae wrote: >>> 2013/10/3 Sean Paul : >>>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae wrote: >>>>> Hi, thank you for your contribution and the below is my short comments, >>>>> >>>>> 2013/10/2 Sean Paul : >>>>>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS >>>>>> bridge chip. >>>>>> >>>>>> Signed-off-by: Sean Paul >>>>>> --- >>>>>> .../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 >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +#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. 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? 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@lists.infradead.org >>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/