> -----Original Message----- > From: Rahul Sharma [mailto:r.sh.open@xxxxxxxxx] > 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@xxxxxxxxxxxx > 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@xxxxxxxxxxxx> wrote: > > A few comments. > > > > On Fri, Aug 30, 2013 at 2:59 AM, Rahul Sharma <rahul.sharma@xxxxxxxxxxx> > 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@xxxxxxxxxxx> > >> --- > >> .../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? 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@xxxxxxxxxxxxxxxx/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 > >> _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
