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

Commit Message

Sean Paul Oct. 1, 2013, 11:40 p.m. UTC

This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
bridge chip.

Signed-off-by: Sean Paul <[e-mail]>
---
 .../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

Comments

Olof Johansson Oct. 2, 2013, 9:20 p.m. UTC | #1

Hi,


On Tue, Oct 1, 2013 at 4:40 PM, Sean Paul <[e-mail]> wrote:
> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
> bridge chip.
>
> Signed-off-by: Sean Paul <[e-mail]>
> ---

[...]

> +Example:
> +       ptn3460-bridge@20 {

Nit: Name is usually generic device name, i.e. "lvds-bridge" or
something like that.

> +               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/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)

This should be static inline int ptn3460_init(...)

> +{
> +       return 0;
> +}


-0Olof

대인기/Tizen Platform Lab(SR)/삼성전자 Oct. 3, 2013, 1:55 p.m. UTC | #2

Hi, thank you for your contribution and the below is my short comments,

2013/10/2 Sean Paul <[e-mail]>:
> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
> bridge chip.
>
> Signed-off-by: Sean Paul <[e-mail]>
> ---
>  .../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?

> +       - 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.

> +
> +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? :)


> +       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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Sean Paul Oct. 3, 2013, 2:57 p.m. UTC | #3

On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
> Hi, thank you for your contribution and the below is my short comments,
>
> 2013/10/2 Sean Paul <[e-mail]>:
>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
>> bridge chip.
>>
>> Signed-off-by: Sean Paul <[e-mail]>
>> ---
>>  .../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.


>> +       - 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. The display ops are better suited
there, since it's the actual encoder/connector. I hope to get that
posted this week.


>> +
>> +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
>> [e-mail]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

대인기/Tizen Platform Lab(SR)/삼성전자 Oct. 3, 2013, 5:39 p.m. UTC | #4

2013/10/3 Sean Paul <[e-mail]>:
> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
>> Hi, thank you for your contribution and the below is my short comments,
>>
>> 2013/10/2 Sean Paul <[e-mail]>:
>>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
>>> bridge chip.
>>>
>>> Signed-off-by: Sean Paul <[e-mail]>
>>> ---
>>>  .../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? Actually, for this, we planned to use CDF(Common Display
Framework) if the framework is merged to mainline somehow.

> 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
>>> [e-mail]
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Olof Johansson Oct. 3, 2013, 6:01 p.m. UTC | #5

On Thu, Oct 3, 2013 at 10:39 AM, Inki Dae <[e-mail]> wrote:
> 2013/10/3 Sean Paul <[e-mail]>:
>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
>>> 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.

Take a look at the data sheet, it is publicly available.

PD_N is not a power supply input, so modelling it as a regulator makes no sense:

"If PD_N is LOW, then the device is in Deep power-down completely,
even if supply rail is ON; for the device to be able to operate, the
PD_N pin must be HIGH."



-Olof

Sean Paul Oct. 3, 2013, 6:09 p.m. UTC | #6

On Thu, Oct 3, 2013 at 1:39 PM, Inki Dae <[e-mail]> wrote:
> 2013/10/3 Sean Paul <[e-mail]>:
>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
>>> Hi, thank you for your contribution and the below is my short comments,
>>>
>>> 2013/10/2 Sean Paul <[e-mail]>:
>>>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
>>>> bridge chip.
>>>>
>>>> Signed-off-by: Sean Paul <[e-mail]>
>>>> ---
>>>>  .../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.
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
>>>> [e-mail]
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

대인기/Tizen Platform Lab(SR)/삼성전자 Oct. 3, 2013, 6:23 p.m. UTC | #7

2013/10/4 Sean Paul <[e-mail]>:
> On Thu, Oct 3, 2013 at 1:39 PM, Inki Dae <[e-mail]> wrote:
>> 2013/10/3 Sean Paul <[e-mail]>:
>>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
>>>> Hi, thank you for your contribution and the below is my short comments,
>>>>
>>>> 2013/10/2 Sean Paul <[e-mail]>:
>>>>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
>>>>> bridge chip.
>>>>>
>>>>> Signed-off-by: Sean Paul <[e-mail]>
>>>>> ---
>>>>>  .../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
>>>>> [e-mail]
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> _______________________________________________
>>> dri-devel mailing list
>>> [e-mail]
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Sean Paul Oct. 3, 2013, 6:32 p.m. UTC | #8

On Thu, Oct 3, 2013 at 2:23 PM, Inki Dae <[e-mail]> wrote:
> 2013/10/4 Sean Paul <[e-mail]>:
>> On Thu, Oct 3, 2013 at 1:39 PM, Inki Dae <[e-mail]> wrote:
>>> 2013/10/3 Sean Paul <[e-mail]>:
>>>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
>>>>> Hi, thank you for your contribution and the below is my short comments,
>>>>>
>>>>> 2013/10/2 Sean Paul <[e-mail]>:
>>>>>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
>>>>>> bridge chip.
>>>>>>
>>>>>> Signed-off-by: Sean Paul <[e-mail]>
>>>>>> ---
>>>>>>  .../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.

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
>>>>>> [e-mail]
>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> [e-mail]
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

대인기/Tizen Platform Lab(SR)/삼성전자 Oct. 4, 2013, 1:59 a.m. UTC | #9

2013/10/4 Sean Paul <[e-mail]>:
> On Thu, Oct 3, 2013 at 2:23 PM, Inki Dae <[e-mail]> wrote:
>> 2013/10/4 Sean Paul <[e-mail]>:
>>> On Thu, Oct 3, 2013 at 1:39 PM, Inki Dae <[e-mail]> wrote:
>>>> 2013/10/3 Sean Paul <[e-mail]>:
>>>>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
>>>>>> Hi, thank you for your contribution and the below is my short comments,
>>>>>>
>>>>>> 2013/10/2 Sean Paul <[e-mail]>:
>>>>>>> This patch adds a drm_bridge driver for the PTN3460 DisplayPort to LVDS
>>>>>>> bridge chip.
>>>>>>>
>>>>>>> Signed-off-by: Sean Paul <[e-mail]>
>>>>>>> ---
>>>>>>>  .../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
>>>>>>> [e-mail]
>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> [e-mail]
>>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> _______________________________________________
>>> dri-devel mailing list
>>> [e-mail]
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

대인기/Tizen Platform Lab(SR)/삼성전자 Oct. 4, 2013, 2:05 a.m. UTC | #10

2013/10/4 Olof Johansson <[e-mail]>:
> On Thu, Oct 3, 2013 at 10:39 AM, Inki Dae <[e-mail]> wrote:
>> 2013/10/3 Sean Paul <[e-mail]>:
>>> On Thu, Oct 3, 2013 at 9:55 AM, Inki Dae <[e-mail]> wrote:
>>>> 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.
>
> Take a look at the data sheet, it is publicly available.
>
> PD_N is not a power supply input, so modelling it as a regulator makes no sense:
>
> "If PD_N is LOW, then the device is in Deep power-down completely,
> even if supply rail is ON; for the device to be able to operate, the
> PD_N pin must be HIGH."
>

I still think the pin could be replaced with a regulator. But
lvds-bridge node has "powerdown-gpio" property - it say this board
will use gpio pin - specific to board.  So it seems no problem.

>
>
> -Olof
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Brown Oct. 9, 2013, 6:29 p.m. UTC | #11

On Fri, Oct 04, 2013 at 11:05:48AM +0900, Inki Dae wrote:
> 2013/10/4 Olof Johansson <[e-mail]>:

> > "If PD_N is LOW, then the device is in Deep power-down completely,
> > even if supply rail is ON; for the device to be able to operate, the
> > PD_N pin must be HIGH."

> I still think the pin could be replaced with a regulator. But
> lvds-bridge node has "powerdown-gpio" property - it say this board
> will use gpio pin - specific to board.  So it seems no problem.

No, don't model things that aren't regulators as regulators - it's just
confusing from a usability standpoint and causes breakage when the pins
don't behave like regulators.

대인기/Tizen Platform Lab(SR)/삼성전자 Oct. 10, 2013, 4:18 a.m. UTC | #12

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Thursday, October 10, 2013 3:29 AM
> To: Inki Dae
> Cc: Olof Johansson; Sean Paul; [e-mail]; linux-samsung-
> [e-mail]; [e-mail]; linux-
> [e-mail]; DRI mailing list; linux-arm-
> [e-mail]
> Subject: Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver
> 
> On Fri, Oct 04, 2013 at 11:05:48AM +0900, Inki Dae wrote:
> > 2013/10/4 Olof Johansson <[e-mail]>:
> 
> > > "If PD_N is LOW, then the device is in Deep power-down completely,
> > > even if supply rail is ON; for the device to be able to operate, the
> > > PD_N pin must be HIGH."
> 
> > I still think the pin could be replaced with a regulator. But
> > lvds-bridge node has "powerdown-gpio" property - it say this board
> > will use gpio pin - specific to board.  So it seems no problem.
> 
> No, don't model things that aren't regulators as regulators - it's just
> confusing from a usability standpoint and causes breakage when the pins
> don't behave like regulators.

It seems that there was your missing point. That _is not_ what I mentioned.
I mean that other boards can use a regulator instead of gpio pin.

Mark Brown Oct. 10, 2013, 9:37 a.m. UTC | #13

On Thu, Oct 10, 2013 at 01:18:05PM +0900, Inki Dae wrote:

> > > I still think the pin could be replaced with a regulator. But
> > > lvds-bridge node has "powerdown-gpio" property - it say this board
> > > will use gpio pin - specific to board.  So it seems no problem.

> > No, don't model things that aren't regulators as regulators - it's just
> > confusing from a usability standpoint and causes breakage when the pins
> > don't behave like regulators.

> It seems that there was your missing point. That _is not_ what I mentioned.
> I mean that other boards can use a regulator instead of gpio pin.

What I'm saying is no boards should use a regulator to control that
GPIO pin, obviously if they're controlling the actual regulators that's
fine but the reset signal should not be controlled via the regulator
API (there are some unfortunate cases where people have done that
already but let's not have any more).

대인기/Tizen Platform Lab(SR)/삼성전자 Oct. 10, 2013, 11:40 a.m. UTC | #14

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Thursday, October 10, 2013 6:37 PM
> To: Inki Dae
> Cc: 'Olof Johansson'; 'Sean Paul'; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; 'DRI mailing list'; linux-arm-
> [e-mail]
> Subject: Re: [PATCH 3/5] drm/bridge: Add PTN3460 bridge driver
> 
> On Thu, Oct 10, 2013 at 01:18:05PM +0900, Inki Dae wrote:
> 
> > > > I still think the pin could be replaced with a regulator. But
> > > > lvds-bridge node has "powerdown-gpio" property - it say this board
> > > > will use gpio pin - specific to board.  So it seems no problem.
> 
> > > No, don't model things that aren't regulators as regulators - it's
> > > just confusing from a usability standpoint and causes breakage when
> > > the pins don't behave like regulators.
> 
> > It seems that there was your missing point. That _is not_ what I
> mentioned.
> > I mean that other boards can use a regulator instead of gpio pin.
> 
> What I'm saying is no boards should use a regulator to control that GPIO
> pin, obviously if they're controlling the actual regulators that's fine

That is what I mentioned. Some boards _could control_ the actual regulator
for lvds-bridge, and that would be depended on how HW engineer designs the
board. 

> but the reset signal should not be controlled via the regulator API (there
> are some unfortunate cases where people have done that already but let's
> not have any more).

Mark Brown Oct. 10, 2013, 12:23 p.m. UTC | #15

On Thu, Oct 10, 2013 at 08:40:38PM +0900, Inki Dae wrote:

> That is what I mentioned. Some boards _could control_ the actual regulator
> for lvds-bridge, and that would be depended on how HW engineer designs the
> board. 

For the driver this should be totally transparent - it should just
control the regulator all the time, the regulator API will just not do
anything if the regulator state can't actually be changed.

Patch

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
+	- 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);
+
+	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);
+}
+
+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);
+	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,
+};
+
+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);
+	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);
+	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