[PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Sean Paul
seanpaul at chromium.org
Wed Sep 4 07:51:48 PDT 2013
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"
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.
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@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
>> >>
>
More information about the dri-devel
mailing list