Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver

From: Inki Dae
Date: Thu Oct 03 2013 - 14:23:35 EST

Next message: Tony Lindgren: "Re: [PATCH] ARM: omap1: fix incorrect placement of __initdata tag"
Previous message: Al Viro: "Re: [PATCH] fs: make sure we do not read beyond allocation"
In reply to: Sean Paul: "Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver"
Next in thread: Sean Paul: "Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Tony Lindgren: "Re: [PATCH] ARM: omap1: fix incorrect placement of __initdata tag"
Previous message: Al Viro: "Re: [PATCH] fs: make sure we do not read beyond allocation"
In reply to: Sean Paul: "Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver"
Next in thread: Sean Paul: "Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]