Hi Sanjeev, On Wed, Mar 9, 2011 at 9:20 PM, Premi, Sanjeev <premi@xxxxxx> wrote: >> -----Original Message----- >> From: linux-omap-owner@xxxxxxxxxxxxxxx [mailto:linux-omap- >> owner@xxxxxxxxxxxxxxx] On Behalf Of K, Mythri P >> Sent: Wednesday, March 09, 2011 5:15 PM >> To: linux-omap@xxxxxxxxxxxxxxx; Valkeinen, Tomi >> Cc: K, Mythri P >> Subject: [PATCH v4 5/9] OMAP4 : DSS2 : HDMI: HDMI driver addition in the >> DSS >> >> Adding the hdmi interface driver(hdmi.c) to the dss driver. >> It configures the audio and video portion of HDMI based on >> functionality called by the panel driver. >> >> Signed-off-by: Mythri P K <mythripk@xxxxxx> >> Yong Zhi <y-zhi@xxxxxx> >> --- >> drivers/video/omap2/dss/Kconfig | 8 + >> drivers/video/omap2/dss/Makefile | 1 + >> drivers/video/omap2/dss/display.c | 3 + >> drivers/video/omap2/dss/dss.h | 43 ++ >> drivers/video/omap2/dss/hdmi.c | 1315 >> +++++++++++++++++++++++++++++++++++++ >> 5 files changed, 1370 insertions(+), 0 deletions(-) >> create mode 100644 drivers/video/omap2/dss/hdmi.c >> >> diff --git a/drivers/video/omap2/dss/Kconfig >> b/drivers/video/omap2/dss/Kconfig >> index db01473..7749ddb 100644 >> --- a/drivers/video/omap2/dss/Kconfig >> +++ b/drivers/video/omap2/dss/Kconfig >> @@ -60,6 +60,14 @@ config OMAP2_DSS_VENC >> help >> OMAP Video Encoder support for S-Video and composite TV-out. >> >> +config OMAP2_DSS_HDMI >> + bool "HDMI support" >> + depends on ARCH_OMAP4 >> + default y >> + help >> + HDMI Interface. This adds the High Definition Multimedia >> Interface. >> + See http://www.hdmi.org/ for HDMI specification. >> + > [sp] This may have been discussed earlier... but if the interface is > supported only on OMAP4, wouldn't it be better to rename > OMAP2_DSS_HDMI as OMAP4_DSS_HDMI? > Or, the implementation can be used by OMAP3 as well. If so, you > may want to update the depends. I shall change it to OMAP4. > >> config OMAP2_DSS_SDI >> bool "SDI support" >> depends on ARCH_OMAP3 >> diff --git a/drivers/video/omap2/dss/Makefile >> b/drivers/video/omap2/dss/Makefile >> index 7db17b5..5998b69 100644 >> --- a/drivers/video/omap2/dss/Makefile >> +++ b/drivers/video/omap2/dss/Makefile >> @@ -5,3 +5,4 @@ omapdss-$(CONFIG_OMAP2_DSS_RFBI) += rfbi.o >> omapdss-$(CONFIG_OMAP2_DSS_VENC) += venc.o >> omapdss-$(CONFIG_OMAP2_DSS_SDI) += sdi.o >> omapdss-$(CONFIG_OMAP2_DSS_DSI) += dsi.o >> +omapdss-$(CONFIG_OMAP2_DSS_HDMI) += hdmi.o >> diff --git a/drivers/video/omap2/dss/display.c >> b/drivers/video/omap2/dss/display.c >> index c40bcbd..a85a6f3 100644 >> --- a/drivers/video/omap2/dss/display.c >> +++ b/drivers/video/omap2/dss/display.c >> @@ -418,6 +418,9 @@ void dss_init_device(struct platform_device *pdev, >> r = dsi_init_display(dssdev); >> break; >> #endif >> + case OMAP_DISPLAY_TYPE_HDMI: >> + r = hdmi_init_display(dssdev); >> + break; >> default: >> DSSERR("Support for display '%s' not compiled in.\n", >> dssdev->name); >> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h >> index 008cbf4..3eb2661 100644 >> --- a/drivers/video/omap2/dss/dss.h >> +++ b/drivers/video/omap2/dss/dss.h >> @@ -174,6 +174,16 @@ struct dsi_clock_info { >> bool use_sys_clk; >> }; >> >> +/* HDMI PLL structure */ >> +struct hdmi_pll_info { >> + u16 regn; >> + u16 regm; >> + u32 regmf; >> + u16 regm2; >> + u16 regsd; >> + u16 dcofreq; >> +}; >> + >> struct seq_file; >> struct platform_device; >> >> @@ -437,6 +447,39 @@ static inline void venc_uninit_platform_driver(void) >> } >> #endif >> >> +/* HDMI */ >> +#ifdef CONFIG_OMAP2_DSS_HDMI >> +int hdmi_init_platform_driver(void); >> +void hdmi_uninit_platform_driver(void); >> +int hdmi_init_display(struct omap_dss_device *dssdev); >> +#else >> +static inline int hdmi_init_display(struct omap_dss_device *dssdev) >> +{ >> + return 0; >> +} >> +static inline int hdmi_init_platform_driver(void) >> +{ >> + return 0; >> +} >> +static inline void hdmi_uninit_platform_driver(void) >> +{ >> +} >> +#endif >> +int omapdss_hdmi_display_enable(struct omap_dss_device *dssdev); >> +void omapdss_hdmi_display_disable(struct omap_dss_device *dssdev); >> +#ifdef CONFIG_OMAP4_PANEL_HDMI >> +int hdmi_panel_init(void); >> +void hdmi_panel_exit(void); >> +#else >> +static inline int hdmi_panel_init(void) >> +{ >> + return 0; >> +} >> +static inline void hdmi_panel_exit(void) >> +{ >> +} >> +#endif >> + >> /* RFBI */ >> #ifdef CONFIG_OMAP2_DSS_RFBI >> int rfbi_init_platform_driver(void); >> diff --git a/drivers/video/omap2/dss/hdmi.c >> b/drivers/video/omap2/dss/hdmi.c >> new file mode 100644 >> index 0000000..6852843 >> --- /dev/null >> +++ b/drivers/video/omap2/dss/hdmi.c >> @@ -0,0 +1,1315 @@ >> +/* >> + * hdmi.c >> + * >> + * HDMI interface DSS driver setting for TI's OMAP4 family of processor. >> + * Copyright (C) 2010-2011 Texas Instruments Incorporated - >> http://www.ti.com/ >> + * Authors: Yong Zhi >> + * Mythri pk <mythripk@xxxxxx> >> + * >> + * > > [sp] Extra blank lines here > >> + * 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. >> + * >> + * This program is distributed in the hope that it will be useful, but >> WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License >> along with >> + * this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#define DSS_SUBSYS_NAME "HDMI" >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/err.h> >> +#include <linux/io.h> >> +#include <linux/interrupt.h> >> +#include <linux/mutex.h> >> +#include <linux/delay.h> >> +#include <linux/string.h> >> +#include <plat/display.h> >> + >> +#include "dss.h" >> +#include "hdmi.h" >> + >> +static struct { >> + struct mutex lock; >> + struct omap_display_platform_data *pdata; >> + struct platform_device *pdev; >> + void __iomem *base_wp; /* HDMI wrapper */ >> + int code; >> + int mode; >> + u8 edid[HDMI_EDID_MAX_LENGTH]; >> + u8 edid_set; >> + struct hdmi_config cfg; >> +} hdmi; >> + >> +/* >> + * Logic for the below structure >> + * user enters the CEA or VESA timings by specifying >> + * the hdmicode which corresponds to CEA/VESA timings >> + * please refer to section 6.3 in HDMI 1.3 specification for timing code. >> + * There is a correspondence between CEA/VESA timing and code. >> + * In the below structure, cea_vesa_timings corresponds to all >> + * The OMAP4 supported timing CEA and VESA timing values. >> + * code_cea corresponds to the CEA code entered by the user, >> + * The use of it is to get the timing from the cea_vesa_timing array. >> + * Similarly for code_vesa. >> + * code_index is backmapping, Once EDID is read from the TV >> + * EDID is parsed to find the timing values to map it back to the >> + * corresponding CEA or VESA index this structure is used. >> + */ > > [sp] Is the comment intentionally formatted as it is? > There seems to be an opportunity to improve the text and > readability here. > I shall try to indent. >> + >> +/* >> + * This is the structure which has all supported timing >> + * values that OMAP4 supports >> + */ > > [sp] Seems to be repeat of the comment just above. > >> +static const struct hdmi_timings cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = >> { >> + { {640, 480, 25200, 96, 16, 48, 2, 10, 33} , 0 , 0}, >> + { {1280, 720, 74250, 40, 440, 220, 5, 5, 20}, 1, 1}, >> + { {1280, 720, 74250, 40, 110, 220, 5, 5, 20}, 1, 1}, >> + { {720, 480, 27027, 62, 16, 60, 6, 9, 30}, 0, 0}, >> + { {2880, 576, 108000, 256, 48, 272, 5, 5, 39}, 0, 0}, >> + { {1440, 240, 27027, 124, 38, 114, 3, 4, 15}, 0, 0}, >> + { {1440, 288, 27000, 126, 24, 138, 3, 2, 19}, 0, 0}, >> + { {1920, 540, 74250, 44, 528, 148, 5, 2, 15}, 1, 1}, >> + { {1920, 540, 74250, 44, 88, 148, 5, 2, 15}, 1, 1}, >> + { {1920, 1080, 148500, 44, 88, 148, 5, 4, 36}, 1, 1}, >> + { {720, 576, 27000, 64, 12, 68, 5, 5, 39}, 0, 0}, >> + { {1440, 576, 54000, 128, 24, 136, 5, 5, 39}, 0, 0}, >> + { {1920, 1080, 148500, 44, 528, 148, 5, 4, 36}, 1, 1}, >> + { {2880, 480, 108108, 248, 64, 240, 6, 9, 30}, 0, 0}, >> + { {1920, 1080, 74250, 44, 638, 148, 5, 4, 36}, 1, 1}, >> + /* VESA From Here */ >> + { {640, 480, 25175, 96, 16, 48, 2 , 11, 31}, 0, 0}, >> + { {800, 600, 40000, 128, 40, 88, 4 , 1, 23}, 1, 1}, >> + { {848, 480, 33750, 112, 16, 112, 8 , 6, 23}, 1, 1}, >> + { {1280, 768, 79500, 128, 64, 192, 7 , 3, 20}, 1, 0}, >> + { {1280, 800, 83500, 128, 72, 200, 6 , 3, 22}, 1, 0}, >> + { {1360, 768, 85500, 112, 64, 256, 6 , 3, 18}, 1, 1}, >> + { {1280, 960, 108000, 112, 96, 312, 3 , 1, 36}, 1, 1}, >> + { {1280, 1024, 108000, 112, 48, 248, 3 , 1, 38}, 1, 1}, >> + { {1024, 768, 65000, 136, 24, 160, 6, 3, 29}, 0, 0}, >> + { {1400, 1050, 121750, 144, 88, 232, 4, 3, 32}, 1, 0}, >> + { {1440, 900, 106500, 152, 80, 232, 6, 3, 25}, 1, 0}, >> + { {1680, 1050, 146250, 176 , 104, 280, 6, 3, 30}, 1, 0}, >> + { {1366, 768, 85500, 143, 70, 213, 3, 3, 24}, 1, 1}, >> + { {1920, 1080, 148500, 44, 148, 80, 5, 4, 36}, 1, 1}, >> + { {1280, 768, 68250, 32, 48, 80, 7, 3, 12}, 0, 1}, >> + { {1400, 1050, 101000, 32, 48, 80, 4, 3, 23}, 0, 1}, >> + { {1680, 1050, 119000, 32, 48, 80, 6, 3, 21}, 0, 1}, >> + { {1280, 800, 79500, 32, 48, 80, 6, 3, 14}, 0, 1}, >> + { {1280, 720, 74250, 40, 110, 220, 5, 5, 20}, 1, 1} >> +}; >> + >> +/* >> + * This is a static mapping array which maps the timing values >> + * with corresponding CEA / VESA code >> + */ >> +static const int code_index[OMAP_HDMI_TIMINGS_NB] = { >> + 1, 19, 4, 2, 37, 6, 21, 20, 5, 16, 17, 29, 31, 35, 32, >> + /* <--15 CEA 17--> vesa*/ >> + 4, 9, 0xE, 0x17, 0x1C, 0x27, 0x20, 0x23, 0x10, 0x2A, >> + 0X2F, 0x3A, 0X51, 0X52, 0x16, 0x29, 0x39, 0x1B >> +}; >> + >> +/* >> + * This is reverse static mapping which maps the CEA / VESA code >> + * to the corresponding timing values >> + */ >> +static const int code_cea[39] = { >> + -1, 0, 3, 3, 2, 8, 5, 5, -1, -1, >> + -1, -1, -1, -1, -1, -1, 9, 10, 10, 1, >> + 7, 6, 6, -1, -1, -1, -1, -1, -1, 11, >> + 11, 12, 14, -1, -1, 13, 13, 4, 4 >> +}; >> + >> +static const int code_vesa[85] = { >> + -1, -1, -1, -1, 15, -1, -1, -1, -1, 16, >> + -1, -1, -1, -1, 17, -1, 23, -1, -1, -1, >> + -1, -1, 29, 18, -1, -1, -1, 32, 19, -1, >> + -1, -1, 21, -1, -1, 22, -1, -1, -1, 20, >> + -1, 30, 24, -1, -1, -1, -1, 25, -1, -1, >> + -1, -1, -1, -1, -1, -1, -1, 31, 26, -1, >> + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, >> + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, >> + -1, 27, 28, -1, 33}; >> + >> +static const u8 edid_header[8] = {0x0, 0xff, 0xff, 0xff, 0xff, 0xff, >> 0xff, 0x0}; >> + >> +static inline void hdmi_write_reg(const struct hdmi_reg idx, u32 val) >> +{ >> + __raw_writel(val, hdmi.base_wp + idx.idx); >> +} >> + >> +static inline u32 hdmi_read_reg(const struct hdmi_reg idx) >> +{ >> + return __raw_readl(hdmi.base_wp + idx.idx); >> +} > > [sp] Suggestion - reg.idx reads better than idx.idx esp since the argument > type is "struct hdmi_reg".\ I have named it similar to other DSS register read functions. > >> + >> +static inline int hdmi_wait_for_bit_change(const struct hdmi_reg idx, >> + int b2, int b1, u32 val) >> +{ >> + u32 t = 0; >> + while (val != REG_GET(idx, b2, b1)) { >> + udelay(1); >> + if (t++ > 10000) >> + return !val; > > [sp] Just wanted to confirm that "!val" is really safe to return from here. yes , again similar to other DSS function. > >> + } >> + return val; >> +} >> + >> +int hdmi_init_display(struct omap_dss_device *dssdev) >> +{ >> + DSSDBG("init_display\n"); >> + >> + return 0; >> +} >> + >> +static int hdmi_pll_init(enum hdmi_clk_refsel refsel, int dcofreq, >> + struct hdmi_pll_info *fmt, u16 sd) >> +{ >> + u32 r; >> + >> + /* PLL start always use manual mode */ >> + REG_FLD_MOD(PLLCTRL_PLL_CONTROL, 0x0, 0, 0); >> + >> + r = hdmi_read_reg(PLLCTRL_CFG1); >> + r = FLD_MOD(r, fmt->regm, 20, 9); /* CFG1_PLL_REGM */ >> + r = FLD_MOD(r, fmt->regn, 8, 1); /* CFG1_PLL_REGN */ >> + >> + hdmi_write_reg(PLLCTRL_CFG1, r); >> + >> + r = hdmi_read_reg(PLLCTRL_CFG2); >> + >> + r = FLD_MOD(r, 0x0, 12, 12); /* PLL_HIGHFREQ divide by 2 */ >> + r = FLD_MOD(r, 0x1, 13, 13); /* PLL_REFEN */ >> + r = FLD_MOD(r, 0x0, 14, 14); /* PHY_CLKINEN de-assert during locking >> */ >> + >> + if (dcofreq) { >> + /* divider programming for frequency beyond 1000Mhz */ >> + REG_FLD_MOD(PLLCTRL_CFG3, sd, 17, 10); >> + r = FLD_MOD(r, 0x4, 3, 1); /* 1000MHz and 2000MHz */ >> + } else { >> + r = FLD_MOD(r, 0x2, 3, 1); /* 500MHz and 1000MHz */ >> + } >> + >> + hdmi_write_reg(PLLCTRL_CFG2, r); >> + >> + r = hdmi_read_reg(PLLCTRL_CFG4); >> + r = FLD_MOD(r, fmt->regm2, 24, 18); >> + r = FLD_MOD(r, fmt->regmf, 17, 0); >> + >> + hdmi_write_reg(PLLCTRL_CFG4, r); >> + >> + /* go now */ >> + REG_FLD_MOD(PLLCTRL_PLL_GO, 0x1, 0, 0); >> + >> + /* wait for bit change */ >> + if (hdmi_wait_for_bit_change(PLLCTRL_PLL_GO, 0, 0, 1) != 1) { >> + DSSERR("PLL GO bit not set\n"); >> + return -ETIMEDOUT; >> + } >> + >> + /* Wait till the lock bit is set in PLL status */ >> + if (hdmi_wait_for_bit_change(PLLCTRL_PLL_STATUS, 1, 1, 1) != 1) { >> + DSSWARN("cannot lock PLL\n"); >> + DSSWARN("CFG1 0x%x\n", >> + hdmi_read_reg(PLLCTRL_CFG1)); >> + DSSWARN("CFG2 0x%x\n", >> + hdmi_read_reg(PLLCTRL_CFG2)); >> + DSSWARN("CFG4 0x%x\n", >> + hdmi_read_reg(PLLCTRL_CFG4)); >> + return -ETIMEDOUT; >> + } >> + >> + DSSDBG("PLL locked!\n"); >> + >> + return 0; >> +} >> + >> +/* PHY_PWR_CMD */ >> +static int hdmi_set_phy_pwr(enum hdmi_phy_pwr val) >> +{ >> + /* Command for power control of HDMI PHY */ >> + REG_FLD_MOD(HDMI_WP_PWR_CTRL, val, 7, 6); >> + >> + /* Status of the power control of HDMI PHY */ >> + if (hdmi_wait_for_bit_change(HDMI_WP_PWR_CTRL, 5, 4, val) != val) { >> + DSSERR("Failed to set PHY power mode to %d\n", val); >> + return -ENODEV; > > [sp] Is -ENODEV appropriate. Going by error message, function seems to be > setting the power mode? yes , but as we are waiting on device to set the status, probably i can set a TIMEDOUT. > >> + } >> + >> + return 0; >> +} >> + >> +/* PLL_PWR_CMD */ >> +static int hdmi_set_pll_pwr(enum hdmi_pll_pwr val) >> +{ >> + /* Command for power control of HDMI PLL */ >> + REG_FLD_MOD(HDMI_WP_PWR_CTRL, val, 3, 2); >> + >> + /* wait till PHY_PWR_STATUS is set */ >> + if (hdmi_wait_for_bit_change(HDMI_WP_PWR_CTRL, 1, 0, val) != val) { >> + DSSERR("Failed to set PHY_PWR_STATUS\n"); >> + return -ENODEV; > > [sp] same comment here. > >> + } >> + >> + return 0; >> +} >> + >> +static int hdmi_pll_reset(void) >> +{ >> + /* SYSRESET controlled by power FSM */ >> + REG_FLD_MOD(PLLCTRL_PLL_CONTROL, 0x0, 3, 3); >> + >> + /* READ 0x0 reset is in progress */ >> + if (hdmi_wait_for_bit_change(PLLCTRL_PLL_STATUS, 0, 0, 1) != 1) { >> + DSSERR("Failed to sysreset PLL\n"); >> + return -ETIMEDOUT; >> + } >> + >> + return 0; >> +} >> + >> +static int hdmi_phy_init(void) >> +{ >> + u16 r = 0; >> + >> + r = hdmi_set_phy_pwr(HDMI_PHYPWRCMD_LDOON); >> + if (r) >> + return r; >> + >> + r = hdmi_set_phy_pwr(HDMI_PHYPWRCMD_TXON); >> + if (r) >> + return r; > > [sp] When we return from here, HDMI_PHYPWRCMD_LDOON was > successful. Is it okay to leave the state as is? yes > >> + >> + /* >> + * Read address 0 in order to get the SCP reset done completed >> + * Dummy access performed to make sure reset is done >> + */ >> + hdmi_read_reg(HDMI_TXPHY_TX_CTRL); > > [sp] Is this known behavior/feature - or a workaround? It is a known behavior. > >> + >> + /* >> + * Write to phy address 0 to configure the clock >> + * use HFBITCLK write HDMI_TXPHY_TX_CONTROL_FREQOUT field >> + */ >> + REG_FLD_MOD(HDMI_TXPHY_TX_CTRL, 0x1, 31, 30); >> + >> + /* Write to phy address 1 to start HDMI line (TXVALID and TMDSCLKEN) >> */ >> + hdmi_write_reg(HDMI_TXPHY_DIGITAL_CTRL, 0xF0000000); > > [sp] Can REG_FLD_MOD be used here as well? - comment suggests only > 2 fields being updated. It is used to configure TXVALID and TMDSCLKEN .2 other bits to say use the configured value. > >> + >> + /* Setup max LDO voltage */ >> + REG_FLD_MOD(HDMI_TXPHY_POWER_CTRL, 0xB, 3, 0); >> + >> + /* Write to phy address 3 to change the polarity control */ >> + REG_FLD_MOD(HDMI_TXPHY_PAD_CFG_CTRL, 0x1, 27, 27); >> + >> + return 0; >> +} >> + >> +static int hdmi_wait_softreset(void) >> +{ >> + /* reset W1 */ >> + REG_FLD_MOD(HDMI_WP_SYSCONFIG, 0x1, 0, 0); >> + >> + /* wait till SOFTRESET == 0 */ >> + if (hdmi_wait_for_bit_change(HDMI_WP_SYSCONFIG, 0, 0, 0) != 0) { >> + DSSERR("sysconfig reset failed\n"); >> + return -ENODEV; > > [sp] Is -ENODEV appropriate? Will replace with TIMEDOUT. > >> + } >> + >> + return 0; >> +} >> + >> +static int hdmi_pll_program(struct hdmi_pll_info *fmt) >> +{ >> + u16 r = 0; >> + enum hdmi_clk_refsel refsel; >> + >> + /* wait for wrapper reset */ >> + hdmi_wait_softreset(); > > [sp] Return value isn;t being checked here. If -ENODEV was returned, > is it safe to move ahead? > >> + >> + r = hdmi_set_pll_pwr(HDMI_PLLPWRCMD_ALLOFF); >> + if (r) >> + return r; >> + >> + r = hdmi_set_pll_pwr(HDMI_PLLPWRCMD_BOTHON_ALLCLKS); >> + if (r) >> + return r; >> + >> + hdmi_pll_reset(); > > [sp] Return value is again ignored here. This function can return > -ETIMEDOUT. Is it okay to ignore it? > >> + >> + refsel = HDMI_REFSEL_SYSCLK; >> + >> + r = hdmi_pll_init(refsel, fmt->dcofreq, fmt, fmt->regsd); >> + if (r) >> + return r; > > [sp] When we return from this error location, much of clock and > power config seems to have been done. Can we leave them in > same state? yes , because on error we would anyway be cutting clocks , and that would be over written while configuring next. > >> + >> + return 0; >> +} >> + >> +static void hdmi_phy_off(void) >> +{ >> + hdmi_set_phy_pwr(HDMI_PHYPWRCMD_OFF); >> +} >> + >> +static int hdmi_core_ddc_edid(u8 *pEDID, int ext) > > [sp] Variable pEDID seems bit different. Suggest using lowercase for it. > >> +{ >> + u32 i, j; >> + char checksum = 0; >> + u32 offset = 0; >> + >> + /* Turn on CLK for DDC */ >> + REG_FLD_MOD(HDMI_CORE_AV_DPD, 0x7, 2, 0); >> + >> + /* >> + * SW HACK : Without the Delay DDC(i2c bus) reads 0 values / >> + * right shifted values( The behavior is not consistent and seen >> only >> + * with some TV's) >> + */ >> + usleep_range(800, 1000); >> + >> + if (!ext) { >> + /* Clk SCL Devices */ >> + REG_FLD_MOD(HDMI_CORE_DDC_CMD, 0xA, 3, 0); >> + >> + /* HDMI_CORE_DDC_STATUS_IN_PROG No timer needed */ > > [sp] What is significance of "No timer needed"? > >> + if (hdmi_wait_for_bit_change(HDMI_CORE_DDC_STATUS, >> + 4, 4, 0) != 0) { >> + DSSERR("Failed to program DDC\n"); >> + return -ETIMEDOUT; >> + } >> + >> + /* Clear FIFO */ >> + REG_FLD_MOD(HDMI_CORE_DDC_CMD, 0x9, 3, 0); >> + >> + /* HDMI_CORE_DDC_STATUS_IN_PROG */ >> + if (hdmi_wait_for_bit_change(HDMI_CORE_DDC_STATUS, >> + 4, 4, 0) != 0) { >> + DSSERR("Failed to program DDC\n"); >> + return -ETIMEDOUT; >> + } >> + >> + } else { >> + if (ext % 2 != 0) >> + offset = 0x80; >> + } >> + >> + /* Load Segment Address Register */ >> + REG_FLD_MOD(HDMI_CORE_DDC_SEGM, ext/2, 7, 0); >> + >> + /* Load Slave Address Register */ >> + REG_FLD_MOD(HDMI_CORE_DDC_ADDR, 0xA0 >> 1, 7, 1); >> + >> + /* Load Offset Address Register */ >> + REG_FLD_MOD(HDMI_CORE_DDC_OFFSET, offset, 7, 0); >> + >> + /* Load Byte Count */ >> + REG_FLD_MOD(HDMI_CORE_DDC_COUNT1, 0x80, 7, 0); >> + REG_FLD_MOD(HDMI_CORE_DDC_COUNT2, 0x0, 1, 0); >> + >> + /* Set DDC_CMD */ >> + if (ext) >> + REG_FLD_MOD(HDMI_CORE_DDC_CMD, 0x4, 3, 0); >> + else >> + REG_FLD_MOD(HDMI_CORE_DDC_CMD, 0x2, 3, 0); >> + >> + /* >> + * Do not optimize this part of the code, seems >> + * DDC bus needs some time to get stabilized >> + */ >> + >> + /* HDMI_CORE_DDC_STATUS_BUS_LOW */ >> + if (REG_GET(HDMI_CORE_DDC_STATUS, 6, 6) == 1) { >> + DSSWARN("I2C Bus Low?\n"); >> + return -EIO; >> + } >> + /* HDMI_CORE_DDC_STATUS_NO_ACK */ >> + if (REG_GET(HDMI_CORE_DDC_STATUS, 5, 5) == 1) { >> + DSSWARN("I2C No Ack\n"); >> + return -EIO; >> + } >> + > [sp] Is -EIO appropriate error here? I2c is not responding , so i think EIO would be appropriate, Do you have any other suggestions? > >> + i = ext * 128; >> + j = 0; >> + while (((REG_GET(HDMI_CORE_DDC_STATUS, 4, 4) == 1) || >> + (REG_GET(HDMI_CORE_DDC_STATUS, 2, 2) == 0)) && >> + j < 128) { >> + >> + if (REG_GET(HDMI_CORE_DDC_STATUS, 2, 2) == 0) { >> + /* FIFO not empty */ >> + pEDID[i++] = REG_GET(HDMI_CORE_DDC_DATA, 7, 0); >> + j++; >> + } >> + } >> + >> + for (j = 0; j < 128; j++) >> + checksum += pEDID[j]; >> + >> + if (checksum != 0) { >> + DSSERR("E-EDID checksum failed!!\n"); >> + return -EIO; >> + } >> + >> + return 0; >> +} >> + >> +static int read_edid(u8 *pEDID, u16 max_length) >> +{ >> + int r = 0, n = 0, i = 0; >> + int max_ext_blocks = (max_length / 128) - 1; >> + >> + r = hdmi_core_ddc_edid(pEDID, 0); >> + if (r) { >> + return -EIO; > > [sp] Same query on -EIO here. Again it is because of i2c fail , so I think EIO is appropriate. > >> + } else { >> + n = pEDID[0x7e]; >> + >> + /* >> + * README: need to comply with max_length set by the caller. >> + * Better implementation should be to allocate necessary >> + * memory to store EDID according to nb_block field found >> + * in first block >> + */ > [sp] Shouldn't this info go to the function header instead? It is for the parameter passed , so i added it while checking for the parameter. > >> + > [sp] extra line >> + if (n > max_ext_blocks) >> + n = max_ext_blocks; >> + >> + for (i = 1; i <= n; i++) { >> + r = hdmi_core_ddc_edid(pEDID, i); >> + if (r) >> + return -EIO; >> + } >> + } >> + return 0; >> +} >> + >> +static int get_timings_index(void) >> +{ >> + int code; >> + >> + if (hdmi.mode == 0) >> + code = code_vesa[hdmi.code]; >> + else >> + code = code_cea[hdmi.code]; >> + >> + if (code == -1) { >> + code = 9; >> + /* HDMI code 16 corresponds to 1920 * 1080 */ >> + hdmi.code = 16; >> + /* HDMI mode 1 corresponds to HDMI 0 to DVI */ >> + hdmi.mode = HDMI_HDMI; >> + } >> + return code; >> +} >> + >> +static struct hdmi_cm hdmi_get_code(struct omap_video_timings *timing) >> +{ >> + int i = 0, code = -1, temp_vsync = 0, temp_hsync = 0; >> + int timing_vsync = 0, timing_hsync = 0; >> + struct omap_video_timings temp; >> + struct hdmi_cm cm = {-1}; >> + DSSDBG("hdmi_get_code\n"); >> + >> + for (i = 0; i < OMAP_HDMI_TIMINGS_NB; i++) { >> + temp = cea_vesa_timings[i].timings; >> + if ((temp.pixel_clock == timing->pixel_clock) && >> + (temp.x_res == timing->x_res) && >> + (temp.y_res == timing->y_res)) { >> + >> + temp_hsync = temp.hfp + temp.hsw + temp.hbp; >> + timing_hsync = timing->hfp + timing->hsw + timing->hbp; >> + temp_vsync = temp.vfp + temp.vsw + temp.vbp; >> + timing_vsync = timing->vfp + timing->vsw + timing->vbp; >> + >> + DSSDBG("temp_hsync = %d , temp_vsync = %d" >> + "timing_hsync = %d, timing_vsync = %d\n", >> + temp_hsync, temp_hsync, >> + timing_hsync, timing_vsync); >> + >> + if ((temp_hsync == timing_hsync) && >> + (temp_vsync == timing_vsync)) { >> + code = i; >> + cm.code = code_index[i]; >> + if (code < 14) >> + cm.mode = HDMI_HDMI; >> + else >> + cm.mode = HDMI_DVI; >> + DSSDBG("Hdmi_code = %d mode = %d\n", >> + cm.code, cm.mode); >> + break; >> + } >> + } >> + } >> + >> + return cm; >> +} >> + >> +static void get_horz_vert_timing_info(int current_descriptor_addrs, u8 >> *edid , >> + struct omap_video_timings *timings) >> +{ >> + /* X and Y resolution */ >> + timings->x_res = (((edid[current_descriptor_addrs + 4] & 0xF0) << 4) >> | >> + edid[current_descriptor_addrs + 2]); >> + timings->y_res = (((edid[current_descriptor_addrs + 7] & 0xF0) << 4) >> | >> + edid[current_descriptor_addrs + 5]); >> + >> + timings->pixel_clock = ((edid[current_descriptor_addrs + 1] << 8) | >> + edid[current_descriptor_addrs]); >> + >> + timings->pixel_clock = 10 * timings->pixel_clock; >> + >> + /* HORIZONTAL FRONT PORCH */ >> + timings->hfp = edid[current_descriptor_addrs + 8] | >> + ((edid[current_descriptor_addrs + 11] & 0xc0) << 2); >> + /* HORIZONTAL SYNC WIDTH */ >> + timings->hsw = edid[current_descriptor_addrs + 9] | >> + ((edid[current_descriptor_addrs + 11] & 0x30) << 4); >> + /* HORIZONTAL BACK PORCH */ >> + timings->hbp = (((edid[current_descriptor_addrs + 4] & 0x0F) << 8) | >> + edid[current_descriptor_addrs + 3]) - >> + (timings->hfp + timings->hsw); >> + /* VERTICAL FRONT PORCH */ >> + timings->vfp = ((edid[current_descriptor_addrs + 10] & 0xF0) >> 4) | >> + ((edid[current_descriptor_addrs + 11] & 0x0f) << 2); >> + /* VERTICAL SYNC WIDTH */ >> + timings->vsw = (edid[current_descriptor_addrs + 10] & 0x0F) | >> + ((edid[current_descriptor_addrs + 11] & 0x03) << 4); >> + /* VERTICAL BACK PORCH */ >> + timings->vbp = (((edid[current_descriptor_addrs + 7] & 0x0F) << 8) | >> + edid[current_descriptor_addrs + 6]) - >> + (timings->vfp + timings->vsw); >> + >> +} >> + >> +/* Description : This function gets the resolution information from EDID >> */ > [sp] Suggest using kernel documentation style. > >> +static void get_edid_timing_data(u8 *edid) > > [sp] Contrast use of *edid vs *pEDID few functions above. > >> +{ >> + u8 count; >> + u16 current_descriptor_addrs; >> + struct hdmi_cm cm; >> + struct omap_video_timings edid_timings; >> + >> + /* seach block 0, there are 4 DTDs arranged in priority order */ >> + for (count = 0; count < EDID_SIZE_BLOCK0_TIMING_DESCRIPTOR; count++) >> { >> + current_descriptor_addrs = >> + EDID_DESCRIPTOR_BLOCK0_ADDRESS + >> + count * EDID_TIMING_DESCRIPTOR_SIZE; >> + get_horz_vert_timing_info(current_descriptor_addrs, >> + edid, &edid_timings); >> + cm = hdmi_get_code(&edid_timings); >> + DSSDBG("Block0[%d] value matches code = %d , mode = %d\n", >> + count, cm.code, cm.mode); >> + if (cm.code == -1) { >> + continue; >> + } else { >> + hdmi.code = cm.code; >> + hdmi.mode = cm.mode; >> + DSSDBG("code = %d , mode = %d\n", >> + hdmi.code, hdmi.mode); >> + return; >> + } >> + } >> + if (edid[0x7e] != 0x00) { >> + for (count = 0; count < EDID_SIZE_BLOCK1_TIMING_DESCRIPTOR; >> + count++) { >> + current_descriptor_addrs = >> + EDID_DESCRIPTOR_BLOCK1_ADDRESS + >> + count * EDID_TIMING_DESCRIPTOR_SIZE; >> + get_horz_vert_timing_info(current_descriptor_addrs, >> + edid, &edid_timings); >> + cm = hdmi_get_code(&edid_timings); >> + DSSDBG("Block1[%d] value matches code = %d, mode = %d", >> + count, cm.code, cm.mode); >> + if (cm.code == -1) { >> + continue; >> + } else { >> + hdmi.code = cm.code; >> + hdmi.mode = cm.mode; >> + DSSDBG("code = %d , mode = %d\n", >> + hdmi.code, hdmi.mode); >> + return; >> + } >> + } >> + } >> + >> + DSSINFO("no valid timing found , falling back to VGA\n"); >> + hdmi.code = 4; /* setting default value of 640 480 VGA */ >> + hdmi.mode = HDMI_DVI; >> +} >> + >> +static int hdmi_read_edid(struct omap_video_timings *dp) >> +{ >> + int ret = 0, code; >> + >> + memset(hdmi.edid, 0, HDMI_EDID_MAX_LENGTH); >> + >> + if (!hdmi.edid_set) >> + ret = read_edid(hdmi.edid, HDMI_EDID_MAX_LENGTH); >> + >> + if (ret != 0) { > > [sp] Shouldn't we check for success case first? > >> + DSSWARN("failed to read E-EDID\n"); >> + } else { >> + if (!memcmp(hdmi.edid, edid_header, sizeof(edid_header))) { >> + /* search for timings of default resolution */ >> + get_edid_timing_data(hdmi.edid); >> + hdmi.edid_set = true; >> + } >> + } >> + >> + if (!hdmi.edid_set) { >> + DSSINFO("fallback to VGA\n"); >> + hdmi.code = 4; /* setting default value of 640 480 VGA */ >> + hdmi.mode = HDMI_DVI; >> + } >> + >> + code = get_timings_index(); >> + >> + *dp = cea_vesa_timings[code].timings; >> + >> + return 0; > > [sp] Function will always return 0. Shouldn't it be void? will change to void. > Thanks and regards, Mythri. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
