[PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Inki Dae
inki.dae at samsung.com
Wed Sep 4 21:16:47 PDT 2013
> -----Original Message-----
> From: linux-samsung-soc-owner at vger.kernel.org [mailto:linux-samsung-soc-
> owner at vger.kernel.org] On Behalf Of Sean Paul
> Sent: Wednesday, September 04, 2013 11:52 PM
> To: Inki Dae
> Cc: Rahul Sharma; Rahul Sharma; linux-samsung-soc; dri-devel; kgene.kim;
> sw0312.kim; Lucas Stach; Tomasz Figa; Sylwester Nawrocki; sunil joshi;
> Shirish S
> Subject: Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy
> driver
>
> On Wed, Sep 4, 2013 at 3:37 AM, Inki Dae <inki.dae at samsung.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Rahul Sharma [mailto:r.sh.open at gmail.com]
> >> Sent: Wednesday, September 04, 2013 2:48 PM
> >> To: Sean Paul
> >> Cc: Rahul Sharma; linux-samsung-soc; dri-devel; kgene.kim; sw0312.kim;
> >> InKi Dae; Lucas Stach; Tomasz Figa; Sylwester Nawrocki; sunil joshi;
> >> shirish at chromium.org
> >> Subject: Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to
> hdmiphy
> >> driver
> >>
> >> Thanks Sean,
> >>
> >> On 3 September 2013 20:15, Sean Paul <seanpaul at chromium.org> wrote:
> >> > A few comments.
> >> >
> >> > On Fri, Aug 30, 2013 at 2:59 AM, Rahul Sharma
> <rahul.sharma at samsung.com>
> >> wrote:
> >> >> Exynos hdmiphy operations and configs are kept inside
> >> >> the hdmi driver. Hdmiphy related code is tightly coupled
> >> >> with hdmi IP driver.
> >> >>
> >> >> This patche moves hdmiphy related code to hdmiphy driver.
> >> >
> >> > s/patche/patch
> >> >
> >> ok.
> >> >> It will help in cleanly supporting the hdmiphy variations
> >> >> in further SoCs.
> >> >>
> >> >> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
> >> >> ---
> >> >> .../devicetree/bindings/video/exynos_hdmi.txt | 2 +
> >> >> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 11 +
> >> >> drivers/gpu/drm/exynos/exynos_hdmi.c | 343
> > +++------------
> >> >> drivers/gpu/drm/exynos/exynos_hdmiphy.c | 438
> >> +++++++++++++++++++-
> >> >> drivers/gpu/drm/exynos/regs-hdmiphy.h | 35 ++
> >> >> 5 files changed, 533 insertions(+), 296 deletions(-)
> >> >> create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> >> b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> >> >> index 50decf8..240eca5 100644
> >> >> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> >> >> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> >> >> @@ -25,6 +25,7 @@ Required properties:
> >> >> sclk_pixel.
> >> >> - clock-names: aliases as per driver requirements for above clock
> IDs:
> >> >> "hdmi", "sclk_hdmi", "sclk_pixel", "sclk_hdmiphy" and
> > "mout_hdmi".
> >> >> +- phy: it points to hdmiphy dt node.
> >> >> Example:
> >> >>
> >> >> hdmi {
> >> >> @@ -32,4 +33,5 @@ Example:
> >> >> reg = <0x14530000 0x100000>;
> >> >> interrupts = <0 95 0>;
> >> >> hpd-gpio = <&gpx3 7 1>;
> >> >> + phy = <&hdmiphy>;
> >> >> };
> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> >> b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> >> >> index 724cab1..1c839f8 100644
> >> >> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> >> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> >> >> @@ -64,4 +64,15 @@ void exynos_hdmi_drv_attach(struct
> >> exynos_drm_hdmi_context *ctx);
> >> >> void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx);
> >> >> void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops);
> >> >> void exynos_mixer_ops_register(struct exynos_mixer_ops *ops);
> >> >> +
> >> >> +int exynos_hdmiphy_driver_register(void);
> >> >> +void exynos_hdmiphy_driver_unregister(void);
> >> >> +
> >> >> +void exynos_hdmiphy_enable(struct device *dev);
> >> >> +void exynos_hdmiphy_disable(struct device *dev);
> >> >> +int exynos_hdmiphy_check_mode(struct device *dev,
> >> >> + struct drm_display_mode *mode);
> >> >> +int exynos_hdmiphy_set_mode(struct device *dev,
> >> >> + struct drm_display_mode *mode);
> >> >> +int exynos_hdmiphy_conf_apply(struct device *dev);
> >> >> #endif
> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c
> >> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> >> >> index f67ffca..3af4e4c 100644
> >> >> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> >> >> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> >> >> @@ -34,6 +34,8 @@
> >> >> #include <linux/io.h>
> >> >> #include <linux/of.h>
> >> >> #include <linux/of_gpio.h>
> >> >> +#include <linux/of_i2c.h>
> >> >> +#include <linux/of_platform.h>
> >> >>
> >> >> #include <drm/exynos_drm.h>
> >> >>
> >> >> @@ -172,7 +174,6 @@ struct hdmi_v14_conf {
> >> >> };
> >> >>
> >> >> struct hdmi_conf_regs {
> >> >> - int pixel_clock;
> >> >> int cea_video_id;
> >> >> union {
> >> >> struct hdmi_v13_conf v13_conf;
> >> >> @@ -193,9 +194,9 @@ struct hdmi_context {
> >> >> int irq;
> >> >>
> >> >> struct i2c_client *ddc_port;
> >> >> - struct i2c_client *hdmiphy_port;
> >> >> + struct device *hdmiphy_dev;
> >> >>
> >> >> - /* current hdmiphy conf regs */
> >> >> + /* current hdmi ip configuration registers. */
> >> >> struct hdmi_conf_regs mode_conf;
> >> >>
> >> >> struct hdmi_resources res;
> >> >> @@ -205,180 +206,6 @@ struct hdmi_context {
> >> >> enum hdmi_type type;
> >> >> };
> >> >>
> >> >> -struct hdmiphy_config {
> >> >> - int pixel_clock;
> >> >> - u8 conf[32];
> >> >> -};
> >> >> -
> >> >> -/* list of phy config settings */
> >> >> -static const struct hdmiphy_config hdmiphy_v13_configs[] = {
> >> >> - {
> >> >> - .pixel_clock = 27000000,
> >> >> - .conf = {
> >> >> - 0x01, 0x05, 0x00, 0xD8, 0x10, 0x1C, 0x30,
0x40,
> >> >> - 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54,
0x87,
> >> >> - 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> - 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00,
0x00,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 27027000,
> >> >> - .conf = {
> >> >> - 0x01, 0x05, 0x00, 0xD4, 0x10, 0x9C, 0x09,
0x64,
> >> >> - 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54,
0x87,
> >> >> - 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> - 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00,
0x00,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 74176000,
> >> >> - .conf = {
> >> >> - 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xef,
0x5B,
> >> >> - 0x6D, 0x10, 0x01, 0x51, 0xef, 0xF3, 0x54,
0xb9,
> >> >> - 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> - 0x22, 0x40, 0xa5, 0x26, 0x01, 0x00, 0x00,
0x00,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 74250000,
> >> >> - .conf = {
> >> >> - 0x01, 0x05, 0x00, 0xd8, 0x10, 0x9c, 0xf8,
0x40,
> >> >> - 0x6a, 0x10, 0x01, 0x51, 0xff, 0xf1, 0x54,
0xba,
> >> >> - 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10,
0xe0,
> >> >> - 0x22, 0x40, 0xa4, 0x26, 0x01, 0x00, 0x00,
0x00,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 148500000,
> >> >> - .conf = {
> >> >> - 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xf8,
0x40,
> >> >> - 0x6A, 0x18, 0x00, 0x51, 0xff, 0xF1, 0x54,
0xba,
> >> >> - 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> - 0x22, 0x40, 0xa4, 0x26, 0x02, 0x00, 0x00,
0x00,
> >> >> - },
> >> >> - },
> >> >> -};
> >> >> -
> >> >> -static const struct hdmiphy_config hdmiphy_v14_configs[] = {
> >> >> - {
> >> >> - .pixel_clock = 25200000,
> >> >> - .conf = {
> >> >> - 0x01, 0x51, 0x2A, 0x75, 0x40, 0x01, 0x00,
0x08,
> >> >> - 0x82, 0x80, 0xfc, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xf4, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 27000000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x22, 0x51, 0x40, 0x08, 0xfc,
0x20,
> >> >> - 0x98, 0xa0, 0xcb, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x06, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xe4, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 27027000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x2d, 0x72, 0x40, 0x64, 0x12,
0x08,
> >> >> - 0x43, 0xa0, 0x0e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xe3, 0x24, 0x00, 0x00, 0x00, 0x01,
0x00,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 36000000,
> >> >> - .conf = {
> >> >> - 0x01, 0x51, 0x2d, 0x55, 0x40, 0x01, 0x00,
0x08,
> >> >> - 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xab, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 40000000,
> >> >> - .conf = {
> >> >> - 0x01, 0x51, 0x32, 0x55, 0x40, 0x01, 0x00,
0x08,
> >> >> - 0x82, 0x80, 0x2c, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0x9a, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 65000000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x36, 0x34, 0x40, 0x1e, 0x0a,
0x08,
> >> >> - 0x82, 0xa0, 0x45, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xbd, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 74176000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x3e, 0x35, 0x40, 0x5b, 0xde,
0x08,
> >> >> - 0x82, 0xa0, 0x73, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x56, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xa6, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 74250000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x1f, 0x10, 0x40, 0x40, 0xf8,
0x08,
> >> >> - 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xa5, 0x24, 0x01, 0x00, 0x00, 0x01,
0x00,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 83500000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x23, 0x11, 0x40, 0x0c, 0xfb,
0x08,
> >> >> - 0x85, 0xa0, 0xd1, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0x93, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 106500000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x2c, 0x12, 0x40, 0x0c, 0x09,
0x08,
> >> >> - 0x84, 0xa0, 0x0a, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0x73, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 108000000,
> >> >> - .conf = {
> >> >> - 0x01, 0x51, 0x2d, 0x15, 0x40, 0x01, 0x00,
0x08,
> >> >> - 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0xc7, 0x25, 0x03, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 146250000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x3d, 0x15, 0x40, 0x18, 0xfd,
0x08,
> >> >> - 0x83, 0xa0, 0x6e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0x50, 0x25, 0x03, 0x00, 0x00, 0x01,
0x80,
> >> >> - },
> >> >> - },
> >> >> - {
> >> >> - .pixel_clock = 148500000,
> >> >> - .conf = {
> >> >> - 0x01, 0xd1, 0x1f, 0x00, 0x40, 0x40, 0xf8,
0x08,
> >> >> - 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> - 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> - 0x54, 0x4b, 0x25, 0x03, 0x00, 0x00, 0x01,
0x00,
> >> >> - },
> >> >> - },
> >> >> -};
> >> >> -
> >> >> struct hdmi_infoframe {
> >> >> enum HDMI_PACKET_TYPE type;
> >> >> u8 ver;
> >> >> @@ -769,28 +596,6 @@ static struct edid *hdmi_get_edid(void *ctx,
> >> struct drm_connector *connector)
> >> >> return raw_edid;
> >> >> }
> >> >>
> >> >> -static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32
> >> pixel_clock)
> >> >> -{
> >> >> - const struct hdmiphy_config *confs;
> >> >> - int count, i;
> >> >> -
> >> >> - if (hdata->type == HDMI_TYPE13) {
> >> >> - confs = hdmiphy_v13_configs;
> >> >> - count = ARRAY_SIZE(hdmiphy_v13_configs);
> >> >> - } else if (hdata->type == HDMI_TYPE14) {
> >> >> - confs = hdmiphy_v14_configs;
> >> >> - count = ARRAY_SIZE(hdmiphy_v14_configs);
> >> >> - } else
> >> >> - return -EINVAL;
> >> >> -
> >> >> - for (i = 0; i < count; i++)
> >> >> - if (confs[i].pixel_clock == pixel_clock)
> >> >> - return i;
> >> >> -
> >> >> - DRM_DEBUG_KMS("Could not find phy config for %d\n",
> > pixel_clock);
> >> >> - return -EINVAL;
> >> >> -}
> >> >> -
> >> >> static int hdmi_check_mode(void *ctx, struct drm_display_mode
*mode)
> >> >> {
> >> >> struct hdmi_context *hdata = ctx;
> >> >> @@ -801,7 +606,7 @@ static int hdmi_check_mode(void *ctx, struct
> >> drm_display_mode *mode)
> >> >> (mode->flags & DRM_MODE_FLAG_INTERLACE) ? true :
> >> >> false, mode->clock * 1000);
> >> >>
> >> >> - ret = hdmi_find_phy_conf(hdata, mode->clock * 1000);
> >> >> + ret = exynos_hdmiphy_check_mode(hdata->hdmiphy_dev, mode);
> >> >> if (ret < 0)
> >> >> return ret;
> >> >> return 0;
> >> >> @@ -1302,19 +1107,13 @@ static void hdmi_mode_apply(struct
> hdmi_context
> >> *hdata)
> >> >>
> >> >> static void hdmiphy_conf_reset(struct hdmi_context *hdata)
> >> >> {
> >> >> - u8 buffer[2];
> >> >> u32 reg;
> >> >>
> >> >> clk_disable_unprepare(hdata->res.sclk_hdmi);
> >> >> clk_set_parent(hdata->res.mout_hdmi, hdata->res.sclk_pixel);
> >> >> clk_prepare_enable(hdata->res.sclk_hdmi);
> >> >>
> >> >> - /* operation mode */
> >> >> - buffer[0] = 0x1f;
> >> >> - buffer[1] = 0x00;
> >> >> -
> >> >> - if (hdata->hdmiphy_port)
> >> >> - i2c_master_send(hdata->hdmiphy_port, buffer, 2);
> >> >> + exynos_hdmiphy_disable(hdata->hdmiphy_dev);
> >> >>
> >> >> if (hdata->type == HDMI_TYPE13)
> >> >> reg = HDMI_V13_PHY_RSTOUT;
> >> >> @@ -1342,66 +1141,12 @@ static void hdmiphy_poweroff(struct
> >> hdmi_context *hdata)
> >> >> HDMI_PHY_POWER_OFF_EN);
> >> >> }
> >> >>
> >> >> -static void hdmiphy_conf_apply(struct hdmi_context *hdata)
> >> >> -{
> >> >> - const u8 *hdmiphy_data;
> >> >> - u8 buffer[32];
> >> >> - u8 operation[2];
> >> >> - u8 read_buffer[32] = {0, };
> >> >> - int ret;
> >> >> - int i;
> >> >> -
> >> >> - if (!hdata->hdmiphy_port) {
> >> >> - DRM_ERROR("hdmiphy is not attached\n");
> >> >> - return;
> >> >> - }
> >> >> -
> >> >> - /* pixel clock */
> >> >> - i = hdmi_find_phy_conf(hdata, hdata->mode_conf.pixel_clock);
> >> >> - if (i < 0) {
> >> >> - DRM_ERROR("failed to find hdmiphy conf\n");
> >> >> - return;
> >> >> - }
> >> >> -
> >> >> - if (hdata->type == HDMI_TYPE13)
> >> >> - hdmiphy_data = hdmiphy_v13_configs[i].conf;
> >> >> - else
> >> >> - hdmiphy_data = hdmiphy_v14_configs[i].conf;
> >> >> -
> >> >> - memcpy(buffer, hdmiphy_data, 32);
> >> >> - ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
> >> >> - if (ret != 32) {
> >> >> - DRM_ERROR("failed to configure HDMIPHY via I2C\n");
> >> >> - return;
> >> >> - }
> >> >> -
> >> >> - usleep_range(10000, 12000);
> >> >> -
> >> >> - /* operation mode */
> >> >> - operation[0] = 0x1f;
> >> >> - operation[1] = 0x80;
> >> >> -
> >> >> - ret = i2c_master_send(hdata->hdmiphy_port, operation, 2);
> >> >> - if (ret != 2) {
> >> >> - DRM_ERROR("failed to enable hdmiphy\n");
> >> >> - return;
> >> >> - }
> >> >> -
> >> >> - ret = i2c_master_recv(hdata->hdmiphy_port, read_buffer, 32);
> >> >> - if (ret < 0) {
> >> >> - DRM_ERROR("failed to read hdmiphy config\n");
> >> >> - return;
> >> >> - }
> >> >> -
> >> >> - for (i = 0; i < ret; i++)
> >> >> - DRM_DEBUG_KMS("hdmiphy[0x%02x] write[0x%02x] - "
> >> >> - "recv [0x%02x]\n", i, buffer[i],
> > read_buffer[i]);
> >> >> -}
> >> >> -
> >> >> static void hdmi_conf_apply(struct hdmi_context *hdata)
> >> >> {
> >> >> hdmiphy_conf_reset(hdata);
> >> >> - hdmiphy_conf_apply(hdata);
> >> >> + exynos_hdmiphy_conf_apply(hdata->hdmiphy_dev);
> >> >> + usleep_range(10000, 12000);
> >> >
> >> > This delay seems like something that should be in the phy driver,
> >> > instead of the hdmi driver (since it seems conceivable that certain
> >> > phys might not need a delay or might need a longer delay).
> >> >
> >>
> >> ok. I will move it there.
> >>
> >> >> + exynos_hdmiphy_enable(hdata->hdmiphy_dev);
> >> >>
> >> >> mutex_lock(&hdata->hdmi_mutex);
> >> >> hdmi_conf_reset(hdata);
> >> >> @@ -1434,7 +1179,6 @@ static void hdmi_v13_mode_set(struct
> hdmi_context
> >> *hdata,
> >> >>
> >> >> hdata->mode_conf.cea_video_id =
> >> >> drm_match_cea_mode((struct drm_display_mode *)m);
> >> >> - hdata->mode_conf.pixel_clock = m->clock * 1000;
> >> >>
> >> >> hdmi_set_reg(core->h_blank, 2, m->htotal - m->hdisplay);
> >> >> hdmi_set_reg(core->h_v_line, 3, (m->htotal << 12) |
m->vtotal);
> >> >> @@ -1530,7 +1274,6 @@ static void hdmi_v14_mode_set(struct
> hdmi_context
> >> *hdata,
> >> >>
> >> >> hdata->mode_conf.cea_video_id =
> >> >> drm_match_cea_mode((struct drm_display_mode *)m);
> >> >> - hdata->mode_conf.pixel_clock = m->clock * 1000;
> >> >>
> >> >> hdmi_set_reg(core->h_blank, 2, m->htotal - m->hdisplay);
> >> >> hdmi_set_reg(core->v_line, 2, m->vtotal);
> >> >> @@ -1646,6 +1389,8 @@ static void hdmi_mode_set(void *ctx, struct
> >> drm_display_mode *mode)
> >> >> hdmi_v13_mode_set(hdata, mode);
> >> >> else
> >> >> hdmi_v14_mode_set(hdata, mode);
> >> >> +
> >> >> + exynos_hdmiphy_set_mode(hdata->hdmiphy_dev, mode);
> >> >
> >> > Why set_mode when everything else is mode_set?
> >> >
> >>
> >> I will change it to mode_set.
> >>
> >> >> }
> >> >>
> >> >> static void hdmi_get_max_resol(void *ctx, unsigned int *width,
> >> >> @@ -1824,7 +1569,7 @@ fail:
> >> >> return -ENODEV;
> >> >> }
> >> >>
> >> >> -static struct i2c_client *hdmi_ddc, *hdmi_hdmiphy;
> >> >> +static struct i2c_client *hdmi_ddc;
> >> >>
> >> >> void hdmi_attach_ddc_client(struct i2c_client *ddc)
> >> >> {
> >> >> @@ -1832,12 +1577,6 @@ void hdmi_attach_ddc_client(struct i2c_client
> >> *ddc)
> >> >> hdmi_ddc = ddc;
> >> >> }
> >> >>
> >> >> -void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
> >> >> -{
> >> >> - if (hdmiphy)
> >> >> - hdmi_hdmiphy = hdmiphy;
> >> >> -}
> >> >> -
> >> >> static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
> >> >> (struct device *dev)
> >> >> {
> >> >> @@ -1862,6 +1601,50 @@ err_data:
> >> >> return NULL;
> >> >> }
> >> >>
> >> >> +static int hdmi_get_phy_device(struct hdmi_context *hdata)
> >> >
> >> > "get" is not the proper verb here, "initialize" would be more
> >> > appropriate (IMO, of course).
> >> >
> >>
> >> But we don't initialise the phy here. It is just to get the
> >> hdmiphy dev* through dt.
> >>
> >> >> +{
> >> >> + struct device_node *np;
> >> >> + struct i2c_client *client;
> >> >> + struct platform_device *pdev;
> >> >> + int ret;
> >> >> +
> >> >> + /* register hdmiphy driver */
> >> >> + ret = exynos_hdmiphy_driver_register();
> >> >> + if (ret) {
> >> >> + DRM_ERROR("failed to register hdmiphy driver\n");
> >> >
> >> > Printing ret would be useful here.
> >>
> >> ok.
> >> >
> >> >> + goto err;
> >> >> + }
> >> >> +
> >> >> + np = of_parse_phandle(hdata->dev->of_node, "phy", 0);
> >> >> + if (!np) {
> >> >> + DRM_ERROR("Could not find 'phy' property\n");
> >> >> + ret = -ENOENT;
> >> >> + goto err;
> >> >> + }
> >> >> +
> >> >> + /* find hdmi phy on i2c bus */
> >> >> + client = of_find_i2c_device_by_node(np);
> >> >> + if (client) {
> >> >> + hdata->hdmiphy_dev = &client->dev;
> >> >> + ret = 0;
> >> >> + goto exit;
> >> >> + }
> >> >> +
> >> >> + /* find hdmi phy on platform bus */
> >> >> + pdev = of_find_device_by_node(np);
> >> >> + if (pdev) {
> >> >> + hdata->hdmiphy_dev = &pdev->dev;
> >> >> + ret = 0;
> >> >> + goto exit;
> >> >> + }
> >> >> +
> >> >> +exit:
> >> >> + /* able to find hdmiphy on platform/i2c bus */
> >> >> +err:
> >> >
> >> > Probably just easier to have one "exit" or "out" label that just
> returns
> >> ret.
> >>
> >> ok.
> >> >
> >> >> + of_node_put(np);
> >> >> + return ret;
> >> >> +}
> >> >> +
> >> >> static struct of_device_id hdmi_match_types[] = {
> >> >> {
> >> >> .compatible = "samsung,exynos5-hdmi",
> >> >> @@ -1939,15 +1722,13 @@ static int hdmi_probe(struct platform_device
> >> *pdev)
> >> >>
> >> >> hdata->ddc_port = hdmi_ddc;
> >> >>
> >> >> - /* hdmiphy i2c driver */
> >> >> - if (i2c_add_driver(&hdmiphy_driver)) {
> >> >> - DRM_ERROR("failed to register hdmiphy i2c
driver\n");
> >> >> + ret = hdmi_get_phy_device(hdata);
> >> >> + if (ret) {
> >> >> + DRM_ERROR("failed to get hdmiphy device\n");
> >> >
> >> > Printing ret would be useful.
> >> >
> >> >
> >> >> ret = -ENOENT;
> >> >
> >> > Why change the return value here?
> >> >
> >> Yea. Correct. I shouldn't. I will change this.
> >>
> >> >> goto err_ddc;
> >> >> }
> >> >>
> >> >> - hdata->hdmiphy_port = hdmi_hdmiphy;
> >> >> -
> >> >> hdata->irq = gpio_to_irq(hdata->hpd_gpio);
> >> >> if (hdata->irq < 0) {
> >> >> DRM_ERROR("failed to get GPIO irq\n");
> >> >> @@ -1977,7 +1758,7 @@ static int hdmi_probe(struct platform_device
> >> *pdev)
> >> >> return 0;
> >> >>
> >> >> err_hdmiphy:
> >> >> - i2c_del_driver(&hdmiphy_driver);
> >> >> + exynos_hdmiphy_driver_unregister();
> >> >> err_ddc:
> >> >> i2c_del_driver(&ddc_driver);
> >> >> return ret;
> >> >> @@ -1989,8 +1770,8 @@ static int hdmi_remove(struct platform_device
> >> *pdev)
> >> >>
> >> >> pm_runtime_disable(dev);
> >> >>
> >> >> - /* hdmiphy i2c driver */
> >> >> - i2c_del_driver(&hdmiphy_driver);
> >> >> + /* hdmiphy driver */
> >> >> + exynos_hdmiphy_driver_unregister();
> >> >> /* DDC i2c driver */
> >> >> i2c_del_driver(&ddc_driver);
> >> >>
> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_hdmiphy.c
> >> b/drivers/gpu/drm/exynos/exynos_hdmiphy.c
> >> >> index 59abb14..82daa42 100644
> >> >> --- a/drivers/gpu/drm/exynos/exynos_hdmiphy.c
> >> >> +++ b/drivers/gpu/drm/exynos/exynos_hdmiphy.c
> >> >> @@ -15,51 +15,459 @@
> >> >>
> >> >> #include <linux/kernel.h>
> >> >> #include <linux/i2c.h>
> >> >> -#include <linux/of.h>
> >> >> +#include <linux/module.h>
> >> >> +#include <linux/pm_runtime.h>
> >> >> +#include <linux/clk.h>
> >> >>
> >> >> #include "exynos_drm_drv.h"
> >> >> #include "exynos_hdmi.h"
> >> >> +#include "exynos_drm_hdmi.h"
> >> >> +#include "regs-hdmiphy.h"
> >> >>
> >> >> +#define HDMIPHY_REG_COUNT (32)
> >> >
> >> > This probably belongs in regs-hdmiphy.h instead of here. Also remove
> the
> >> ()
> >> >
> >>
> >> Ok. I will move it there.
> >>
> >> >>
> >> >> -static int hdmiphy_probe(struct i2c_client *client,
> >> >> - const struct i2c_device_id *id)
> >> >> +struct hdmiphy_context {
> >> >> + struct device *dev;
> >> >> + struct hdmiphy_config *current_conf;
> >> >> +
> >> >> + struct hdmiphy_config *confs;
> >> >> + unsigned int nr_confs;
> >> >> +};
> >> >> +
> >> >> +struct hdmiphy_config {
> >> >> + int pixel_clock;
> >> >> + u8 conf[HDMIPHY_REG_COUNT];
> >> >> +};
> >> >> +
> >> >> +struct hdmiphy_drv_data {
> >> >> + struct hdmiphy_config *confs;
> >> >> + unsigned int count;
> >> >> +};
> >> >> +
> >> >> +/* list of all required phy config settings */
> >> >> +static struct hdmiphy_config hdmiphy_4212_configs[] = {
> >> >> + {
> >> >> + .pixel_clock = 25200000,
> >> >> + .conf = {
> >> >> + 0x01, 0x51, 0x2A, 0x75, 0x40, 0x01, 0x00,
0x08,
> >> >> + 0x82, 0x80, 0xfc, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xf4, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 27000000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x22, 0x51, 0x40, 0x08, 0xfc,
0x20,
> >> >> + 0x98, 0xa0, 0xcb, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x06, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xe4, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 27027000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x2d, 0x72, 0x40, 0x64, 0x12,
0x08,
> >> >> + 0x43, 0xa0, 0x0e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xe3, 0x24, 0x00, 0x00, 0x00, 0x01,
0x00,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 36000000,
> >> >> + .conf = {
> >> >> + 0x01, 0x51, 0x2d, 0x55, 0x40, 0x01, 0x00,
0x08,
> >> >> + 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xab, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 40000000,
> >> >> + .conf = {
> >> >> + 0x01, 0x51, 0x32, 0x55, 0x40, 0x01, 0x00,
0x08,
> >> >> + 0x82, 0x80, 0x2c, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0x9a, 0x24, 0x00, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 65000000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x36, 0x34, 0x40, 0x1e, 0x0a,
0x08,
> >> >> + 0x82, 0xa0, 0x45, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xbd, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 74176000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x3e, 0x35, 0x40, 0x5b, 0xde,
0x08,
> >> >> + 0x82, 0xa0, 0x73, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x56, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xa6, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 74250000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x1f, 0x10, 0x40, 0x40, 0xf8,
0x08,
> >> >> + 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xa5, 0x24, 0x01, 0x00, 0x00, 0x01,
0x00,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 83500000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x23, 0x11, 0x40, 0x0c, 0xfb,
0x08,
> >> >> + 0x85, 0xa0, 0xd1, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0x93, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 106500000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x2c, 0x12, 0x40, 0x0c, 0x09,
0x08,
> >> >> + 0x84, 0xa0, 0x0a, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0x73, 0x24, 0x01, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 108000000,
> >> >> + .conf = {
> >> >> + 0x01, 0x51, 0x2d, 0x15, 0x40, 0x01, 0x00,
0x08,
> >> >> + 0x82, 0x80, 0x0e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0xc7, 0x25, 0x03, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 146250000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x3d, 0x15, 0x40, 0x18, 0xfd,
0x08,
> >> >> + 0x83, 0xa0, 0x6e, 0xd9, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x08, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0x50, 0x25, 0x03, 0x00, 0x00, 0x01,
0x80,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 148500000,
> >> >> + .conf = {
> >> >> + 0x01, 0xd1, 0x1f, 0x00, 0x40, 0x40, 0xf8,
0x08,
> >> >> + 0x81, 0xa0, 0xba, 0xd8, 0x45, 0xa0, 0xac,
0x80,
> >> >> + 0x3c, 0x80, 0x11, 0x04, 0x02, 0x22, 0x44,
0x86,
> >> >> + 0x54, 0x4b, 0x25, 0x03, 0x00, 0x00, 0x01,
0x00,
> >> >> + },
> >> >> + },
> >> >> +};
> >> >> +
> >> >> +static struct hdmiphy_config hdmiphy_4210_configs[] = {
> >> >> + {
> >> >> + .pixel_clock = 27000000,
> >> >> + .conf = {
> >> >> + 0x01, 0x05, 0x00, 0xD8, 0x10, 0x1C, 0x30,
0x40,
> >> >> + 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54,
0x87,
> >> >> + 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> + 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00,
0x00,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 27027000,
> >> >> + .conf = {
> >> >> + 0x01, 0x05, 0x00, 0xD4, 0x10, 0x9C, 0x09,
0x64,
> >> >> + 0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54,
0x87,
> >> >> + 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> + 0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00,
0x00,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 74176000,
> >> >> + .conf = {
> >> >> + 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xef,
0x5B,
> >> >> + 0x6D, 0x10, 0x01, 0x51, 0xef, 0xF3, 0x54,
0xb9,
> >> >> + 0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> + 0x22, 0x40, 0xa5, 0x26, 0x01, 0x00, 0x00,
0x00,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 74250000,
> >> >> + .conf = {
> >> >> + 0x01, 0x05, 0x00, 0xd8, 0x10, 0x9c, 0xf8,
0x40,
> >> >> + 0x6a, 0x10, 0x01, 0x51, 0xff, 0xf1, 0x54,
0xba,
> >> >> + 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10,
0xe0,
> >> >> + 0x22, 0x40, 0xa4, 0x26, 0x01, 0x00, 0x00,
0x00,
> >> >> + },
> >> >> + },
> >> >> + {
> >> >> + .pixel_clock = 148500000,
> >> >> + .conf = {
> >> >> + 0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xf8,
0x40,
> >> >> + 0x6A, 0x18, 0x00, 0x51, 0xff, 0xF1, 0x54,
0xba,
> >> >> + 0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10,
0xE0,
> >> >> + 0x22, 0x40, 0xa4, 0x26, 0x02, 0x00, 0x00,
0x00,
> >> >> + },
> >> >> + },
> >> >> +};
> >> >> +
> >> >
> >> > Are you aware of the effort to move these to dt? Since these are
> >> > board-specific values, it seems incorrect to apply them universally.
> >> > Shirish has uploaded a patch to the chromium review site to push
> these
> >> > into dt (https://chromium-review.googlesource.com/#/c/65581). Maybe
> >> > you can work that into your patch set?
> >> >
> >
> > Are these really board-specific values?
>
> According to your hardware people:
>
> "If the signal pattern doesn't change for new board, the phy setting
> is same as the current board. But if changed, you need to confirm with
> measurement of signals, because it may vary slightly by resistance of
> board pattern"
>
Right. it seems that the phy configuration should be adjusted according to
PCB environment: OSC clock rate, 24MHz or 27MHz, could be decided by PCB
even though most PCBs use 27MHz.
> That indicates to me that we might need to tweak these on a per-board
> basis.
>
> In the 5420 datasheet, there are a few register descriptions available
> for the phy. 0x145D0004 is CLK_SEL which seems like it would be
> board-specific, same with TX_* registers.
>
And we can select HDMI Tx PHY internal PLL input clock by setting CLK_SEL.
Ok, Shirish's patch set is reasonable to me. However, that patch set should
be rebased on top of Rahul's patch set. Shirish and Rahul, please re-post
your patch set after discussing how to rebase these patch set.
Thanks,
Inki Dae
> Sean
>
>
>
> > I think they are SoC-specific
> > values, and it's better to define them in the driver. For this, I gave
> > already comments to Shirish why these constraints should be placed in
> > driver. For more detail, you can also refer to the below link,
> >
> > http://www.mail-archive.com/devicetree-
> discuss at lists.ozlabs.org/msg36691.htm
> > l
> >
> > Thanks,
> > Inki Dae
> >
> >>
> >> I have gone through these patches. I am adding Shirish here for further
> >> discussion on these changes. he can explain better.
> >>
> >> >
> >> >> +static struct hdmiphy_config *hdmiphy_find_conf(struct
> hdmiphy_context
> >> *hdata,
> >> >> + const struct drm_display_mode *mode)
> >> >> +{
> >> >> + int i;
> >> >> +
> >> >> + for (i = 0; i < hdata->nr_confs; i++)
> >> >> + if (hdata->confs[i].pixel_clock == (mode->clock *
> > 1000))
> >> >> + return &hdata->confs[i];
> >> >> +
> >> >> + return NULL;
> >> >> +}
> >> >> +
> >> >> +static int hdmiphy_reg_writeb(struct hdmiphy_context *hdata,
> >> >> + u32 reg_offset, u8 value)
> >> >> +{
> >> >> + if (reg_offset >= HDMIPHY_REG_COUNT)
> >> >> + return -EINVAL;
> >> >> +
> >> >> + if (i2c_verify_client(hdata->dev)) {
> >> >
> >> > Is this necessary?
> >>
> >> Based on this check, I was deciding that given dev is i2c or
> >> platform device. But now when we agreed to split drivers to
> >> phy and plf driver, I will remove this.
> >>
> >> >
> >> >> + u8 buffer[2];
> >> >> + int ret;
> >> >> +
> >> >> + buffer[0] = reg_offset;
> >> >> + buffer[1] = value;
> >> >> +
> >> >> + ret = i2c_master_send(to_i2c_client(hdata->dev),
> >> >> + buffer, 2);
> >> >> + if (ret == 2)
> >> >> + return 0;
> >> >> + return ret;
> >> >> + } else {
> >> >> + return -EINVAL;
> >> >> + }
> >> >> +}
> >> >> +
> >> >> +static int hdmiphy_reg_write_buf(struct hdmiphy_context *hdata,
> >> >> + u32 reg_offset, const u8 *buf, u32 len)
> >> >> +{
> >> >> + if ((reg_offset + len) > HDMIPHY_REG_COUNT)
> >> >> + return -EINVAL;
> >> >> +
> >> >> + if (i2c_verify_client(hdata->dev)) {
> >> >
> >> > Is this necessary?
> >> >
> >> >
> >> >> + int ret;
> >> >> +
> >> >> + ret = i2c_master_send(to_i2c_client(hdata->dev),
> >> >> + buf, len);
> >> >
> >> > Does this actually work? You don't seem to be using reg_offset
> anywhere.
> >> >
> >>
> >> reg_offset is not required here. I should remove this. This
> >> procedure is to write the complete buf starting from 0.
> >> hdmiphy_reg_writeb is supposed to be used for writing
> >> value from some offset.
> >>
> >> >> + if (ret == len)
> >> >> + return 0;
> >> >> + return ret;
> >> >> + } else {
> >> >> + return -EINVAL;
> >> >> + }
> >> >> +}
> >> >> +
> >> >> +int exynos_hdmiphy_check_mode(struct device *dev,
> >> >> + struct drm_display_mode *mode)
> >> >> {
> >> >> - hdmi_attach_hdmiphy_client(client);
> >> >> + struct hdmiphy_context *hdata = dev_get_drvdata(dev);
> >> >> + const struct hdmiphy_config *conf;
> >> >> +
> >> >> + DRM_DEBUG_KMS("[%d]\n", __LINE__);
> >> >
> >> > Maybe you can merge this debug message with the one below.
> >> >
> >>
> >> ok.
> >>
> >> >>
> >> >> - dev_info(&client->adapter->dev, "attached s5p_hdmiphy "
> >> >> - "into i2c adapter successfully\n");
> >> >> + if (!hdata) {
> >> >> + DRM_ERROR("Invalid arg: hdmiphy device pointer.\n");
> >> >> + return -EINVAL;
> >> >> + }
> >> >
> >> > Can this happen? I think the phy driver registration should be
> >> > synchronous with the hdmi driver registration. As such, it shouldn't
> >> > be possible to get a check_mode callback until it's all set up.
> >> >
> >>
> >> I will confirm on this.
> >>
> >> >>
> >> >> + DRM_DEBUG("xres=%d, yres=%d, refresh=%d, intl=%d
clock=%d\n",
> >> >> + mode->hdisplay, mode->vdisplay,
> >> >> + mode->vrefresh, (mode->flags &
DRM_MODE_FLAG_INTERLACE)
> >> >> + ? true : false, mode->clock * 1000);
> >> >> +
> >> >> + conf = hdmiphy_find_conf(hdata, mode);
> >> >> + if (!conf) {
> >> >> + DRM_DEBUG("Display Mode is not supported.\n");
> >> >> + return -EINVAL;
> >> >> + }
> >> >> return 0;
> >> >> }
> >> >>
> >> >> -static int hdmiphy_remove(struct i2c_client *client)
> >> >> +int exynos_hdmiphy_set_mode(struct device *dev,
> >> >> + struct drm_display_mode *mode)
> >> >> {
> >> >> - dev_info(&client->adapter->dev, "detached s5p_hdmiphy "
> >> >> - "from i2c adapter successfully\n");
> >> >> + struct hdmiphy_context *hdata = dev_get_drvdata(dev);
> >> >> +
> >> >> + DRM_DEBUG_KMS("[%d]\n", __LINE__);
> >> >> +
> >> >> + hdata->current_conf = hdmiphy_find_conf(hdata, mode);
> >> >> +
> >> >> + if (!hdata) {
> >> >> + DRM_ERROR("Invalid arg: hdmiphy device pointer.\n");
> >> >> + return -EINVAL;
> >> >> + }
> >> >
> >> > I really hope this check isn't necessary, since you've already
> >> > dereferenced it 2 lines up :) See my comment above, I don't think you
> >> > can ever hit this.
> >> >
> >> >>
> >> >> + if (!hdata->current_conf) {
> >> >> + DRM_ERROR("Display Mode is not supported.\n");
> >> >> + return -EINVAL;
> >> >> + }
> >> >> return 0;
> >> >> }
> >> >>
> >> >> -static struct of_device_id hdmiphy_match_types[] = {
> >> >> +void exynos_hdmiphy_enable(struct device *dev)
> >> >> +{
> >> >> + struct hdmiphy_context *hdata = dev_get_drvdata(dev);
> >> >> + int ret;
> >> >> +
> >> >> + DRM_DEBUG_KMS("[%d]\n", __LINE__);
> >> >> +
> >> >> + if (!hdata) {
> >> >> + DRM_ERROR("Invalid arg: hdmiphy device pointer.\n");
> >> >> + return;
> >> >> + }
> >> >
> >> > Same comment as above.
> >> >
> >> >> +
> >> >> + ret = hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE,
> >> >> + HDMIPHY_MODE_EN);
> >> >> + if (ret < 0) {
> >> >> + DRM_ERROR("failed to disable hdmiphy.\n");
> >> >> + return;
> >> >> + }
> >> >> +}
> >> >> +
> >> >> +void exynos_hdmiphy_disable(struct device *dev)
> >> >> +{
> >> >> + struct hdmiphy_context *hdata = dev_get_drvdata(dev);
> >> >> + int ret;
> >> >> +
> >> >> + DRM_DEBUG_KMS("[%d]\n", __LINE__);
> >> >> +
> >> >> + if (!hdata) {
> >> >> + DRM_ERROR("Invalid arg: hdmiphy device pointer.\n");
> >> >> + return;
> >> >> + }
> >> >> +
> >> >> + ret = hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE, 0);
> >> >> + if (ret < 0) {
> >> >> + DRM_ERROR("failed to enable hdmiphy.\n");
> >> >
> >> > If you're not going to return the error code, at least print it here.
> >> >
> >>
> >> ok.
> >>
> >> >> + return;
> >> >> + }
> >> >> +}
> >> >> +
> >> >> +int exynos_hdmiphy_conf_apply(struct device *dev)
> >> >> +{
> >> >> + struct hdmiphy_context *hdata = dev_get_drvdata(dev);
> >> >> + const u8 *hdmiphy_data;
> >> >> + int ret;
> >> >> +
> >> >> + DRM_DEBUG_KMS("[%d]\n", __LINE__);
> >> >> +
> >> >> + if (!hdata) {
> >> >> + DRM_ERROR("Invalid arg: hdmiphy device pointer.\n");
> >> >> + return -EINVAL;
> >> >> + }
> >> >> +
> >> >> + /* pixel clock */
> >> >
> >> > I don't understand this comment.
> >> >
> >>
> >> I will remove this.
> >>
> >> >> + if (hdata->current_conf)
> >> >> + hdmiphy_data = hdata->current_conf->conf;
> >> >> + else
> >> >> + return -EINVAL;
> >> >> +
> >> >> + ret = hdmiphy_reg_write_buf(hdata, 0, hdmiphy_data,
> >> >> + HDMIPHY_REG_COUNT);
> >> >
> >> > I think the only reason this works is b/c you're writing to register
> >> offset 0.
> >> >
> >> >> + if (ret) {
> >> >> + DRM_ERROR("failed to configure hdmiphy.\n");
> >> >> + return -EINVAL;
> >> >
> >> > Why don't you return ret?
> >> >
> >>
> >> ok.
> >>
> >> >> + }
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +static struct hdmiphy_drv_data exynos4212_hdmiphy_drv_data = {
> >> >> + .confs = hdmiphy_4212_configs,
> >> >> + .count = ARRAY_SIZE(hdmiphy_4212_configs)
> >> >> +};
> >> >> +
> >> >> +static struct hdmiphy_drv_data exynos4210_hdmiphy_drv_data = {
> >> >> + .confs = hdmiphy_4210_configs,
> >> >> + .count = ARRAY_SIZE(hdmiphy_4210_configs)
> >> >> +};
> >> >> +
> >> >> +static struct of_device_id hdmiphy_i2c_device_match_types[] = {
> >> >> {
> >> >> .compatible = "samsung,exynos5-hdmiphy",
> >> >> + .data = &exynos4212_hdmiphy_drv_data,
> >> >> }, {
> >> >> .compatible = "samsung,exynos4210-hdmiphy",
> >> >> + .data = &exynos4210_hdmiphy_drv_data,
> >> >> }, {
> >> >> .compatible = "samsung,exynos4212-hdmiphy",
> >> >> + .data = &exynos4212_hdmiphy_drv_data,
> >> >> }, {
> >> >> /* end node */
> >> >> }
> >> >> };
> >> >>
> >> >> -struct i2c_driver hdmiphy_driver = {
> >> >> +static int hdmiphy_i2c_device_probe(struct i2c_client *client,
> >> >> + const struct i2c_device_id *id)
> >> >> +{
> >> >> + struct device *dev = &client->dev;
> >> >> + struct hdmiphy_context *hdata;
> >> >> + struct hdmiphy_drv_data *drv;
> >> >> + const struct of_device_id *match;
> >> >> +
> >> >> + DRM_DEBUG_KMS("[%d]\n", __LINE__);
> >> >> +
> >> >> + hdata = devm_kzalloc(dev, sizeof(*hdata), GFP_KERNEL);
> >> >> + if (!hdata) {
> >> >> + DRM_ERROR("failed to allocate hdmiphy context.\n");
> >> >> + return -ENOMEM;
> >> >> + }
> >> >> +
> >> >> + match = of_match_node(of_match_ptr(
> >> >> + hdmiphy_i2c_device_match_types),
> >> >> + dev->of_node);
> >> >> +
> >> >> + if (match == NULL)
> >> >> + return -ENODEV;
> >> >> +
> >> >> + drv = (struct hdmiphy_drv_data *)match->data;
> >> >> +
> >> >> + hdata->dev = dev;
> >> >> + hdata->confs = drv->confs;
> >> >> + hdata->nr_confs = drv->count;
> >> >> +
> >> >> + i2c_set_clientdata(client, hdata);
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +static const struct i2c_device_id hdmiphy_id[] = {
> >> >> + { },
> >> >> +};
> >> >> +
> >> >> +struct i2c_driver hdmiphy_i2c_driver = {
> >> >> .driver = {
> >> >> .name = "exynos-hdmiphy",
> >> >> .owner = THIS_MODULE,
> >> >> - .of_match_table = hdmiphy_match_types,
> >> >> + .of_match_table = of_match_ptr(
> >> >> + hdmiphy_i2c_device_match_types),
> >> >> },
> >> >> - .probe = hdmiphy_probe,
> >> >> - .remove = hdmiphy_remove,
> >> >> + .id_table = hdmiphy_id,
> >> >> + .probe = hdmiphy_i2c_device_probe,
> >> >> .command = NULL,
> >> >> };
> >> >> -EXPORT_SYMBOL(hdmiphy_driver);
> >> >> +
> >> >> +int exynos_hdmiphy_driver_register(void)
> >> >> +{
> >> >> + int ret;
> >> >> +
> >> >> + ret = i2c_add_driver(&hdmiphy_i2c_driver);
> >> >> + if (ret)
> >> >> + return ret;
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +void exynos_hdmiphy_driver_unregister(void)
> >> >> +{
> >> >> + i2c_del_driver(&hdmiphy_i2c_driver);
> >> >> +}
> >> >> diff --git a/drivers/gpu/drm/exynos/regs-hdmiphy.h
> >> b/drivers/gpu/drm/exynos/regs-hdmiphy.h
> >> >> new file mode 100644
> >> >> index 0000000..d3f9d87
> >> >> --- /dev/null
> >> >> +++ b/drivers/gpu/drm/exynos/regs-hdmiphy.h
> >> >> @@ -0,0 +1,35 @@
> >> >> +/*
> >> >> + *
> >> >> + * regs-hdmiphy.h
> >> >> + *
> >> >> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> >> >> + * http://www.samsung.com/
> >> >> + *
> >> >> + * HDMI-PHY register header file for Samsung HDMI driver
> >> >> + *
> >> >> + * This program is free software; you can redistribute it and/or
> >> modify
> >> >> + * it under the terms of the GNU General Public License version 2
> as
> >> >> + * published by the Free Software Foundation.
> >> >> +*/
> >> >> +
> >> >> +#ifndef SAMSUNG_REGS_HDMIPHY_H
> >> >> +#define SAMSUNG_REGS_HDMIPHY_H
> >> >> +
> >> >> +/*
> >> >> + * Register part
> >> >> +*/
> >> >> +#define HDMIPHY_MODE_SET_DONE (0x1f)
> >> >> +
> >> >> +/*
> >> >> + * Bit definition part
> >> >> + */
> >> >> +
> >> >> +/* HDMIPHY_MODE_SET_DONE */
> >> >> +#define HDMIPHY_MODE_EN (1 << 7)
> >> >> +
> >> >> +/* hdmiphy pmu control bits */
> >> >> +#define PMU_HDMI_PHY_CONTROL_MASK (1 << 0)
> >> >> +#define PMU_HDMI_PHY_ENABLE (1)
> >> >> +#define PMU_HDMI_PHY_DISABLE (0)
> >> >
> >> >
> >> > You don't need the () around simple values
> >> >
> >>
> >> ok.
> >>
> >> We also discussed previously to keep independent i2c and
> >> platform phy drivers in exynos_hdmiphy_i2c.c and
> >> exynos_hdmiphy_platform.c respectively, which will duplicate
> >> the code to some extent.
> >>
> >> Second point is to implement exynos_i2c_hdmiphy_get_ops()
> >> and exynos_platform_hdmiphy_get_ops() to get access to all
> >> callbacks. This will limit the exposure of phy callbacks to the
> >> phy controller.
> >>
> >> Please share your views on this.
> >>
> >> Regards,
> >> Rahul Sharma.
> >>
> >> >> +
> >> >> +#endif /* SAMSUNG_REGS_HDMIPHY_H */
> >> >> --
> >> >> 1.7.10.4
> >> >>
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-
> soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the dri-devel
mailing list