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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux