Hi Sanjeev,
On Mon, Jun 20, 2011 at 7:03 PM, Premi, Sanjeev <pr...@ti.com> wrote:
>> -----Original Message-----
>> From: linux-omap-ow...@vger.kernel.org
>> [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of K, Mythri P
>> Sent: Friday, June 17, 2011 1:47 PM
>> To: linux-omap@vger.kernel.org; Valkeinen, Tomi
>> Cc: K, Mythri P
>> Subject: [PATCH 1/8] OMAP4: DSS: HDMI: HDMI clean up to pass
>> base_address
>
> [sp] HDMI appears twice on the subject. One of these can be removed.
> Also, it isn't clear (though implied) whose base address is being
> passed.
>
> The changes in the patch aren't limited to DSS alone.
> See updated to hdmi_core_audio_config(), hdmi_wp_audio_config_format(),
> hdmi_wp_audio_config_dma(), etc.
>
The changes are related to the HDMI driver which as of this patch is
in DSS. You see any concerns ? It is only later that those functions
are moved out .
>>
>> As the base_address of the HDMI might differ across SoC's,
>
> [sp] Is 'base_address' a variable? OR did you mean 'base address'?
>
>> offset of the HDMI
>> logical blocks and base address got from the platform data are passed
>> dynamically to the functions that modify HDMI IP registers.
>
> [sp] Overall, the sentence can be simplified.
>
>>
>> Signed-off-by: Mythri P K <mythr...@ti.com>
>> ---
>> drivers/video/omap2/dss/hdmi.c | 518
>> ++++++++++++++++++++++++----------------
>> drivers/video/omap2/dss/hdmi.h | 292 +++++++++++------------
>> 2 files changed, 452 insertions(+), 358 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/hdmi.c
>> b/drivers/video/omap2/dss/hdmi.c
>> index 5e5b98b..1d16ee2 100644
>> --- a/drivers/video/omap2/dss/hdmi.c
>> +++ b/drivers/video/omap2/dss/hdmi.c
>> @@ -43,11 +43,17 @@
>> #include "hdmi.h"
>> #include "dss_features.h"
>>
>> +#define HDMI_WP 0x0
>> +#define HDMI_CORE_SYS 0x400
>> +#define HDMI_CORE_AV 0x900
>> +#define HDMI_PLLCTRL 0x200
>> +#define HDMI_PHY 0x300
>> +
>> static struct {
>> struct mutex lock;
>> struct omap_display_platform_data *pdata;
>> struct platform_device *pdev;
>> - void __iomem *base_wp; /* HDMI wrapper */
>> + struct hdmi_ip_data hdmi_data;
>> int code;
>> int mode;
>> u8 edid[HDMI_EDID_MAX_LENGTH];
>> @@ -146,21 +152,51 @@ static const int code_vesa[85] = {
>>
>> 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)
>> +static inline void hdmi_write_reg(void __iomem *base_addr,
>> + const struct hdmi_reg idx, u32 val)
>> +{
>> + __raw_writel(val, base_addr + idx.idx);
>> +}
>> +
>> +static inline u32 hdmi_read_reg(void __iomem *base_addr,
>> + const struct hdmi_reg idx)
>> +{
>> + return __raw_readl(base_addr + idx.idx);
>> +}
>> +
>> +static inline void __iomem *hdmi_wp_base(struct hdmi_ip_data
>> *ip_data)
>> +{
>> + return (void __iomem *) (ip_data->base_wp);
>> +}
>> +
>> +static inline void __iomem *hdmi_phy_base(struct
>> hdmi_ip_data *ip_data)
>> {
>> - __raw_writel(val, hdmi.base_wp + idx.idx);
>> + return (void __iomem *) (ip_data->base_wp +
>> ip_data->hdmi_phy_offset);
>> }
>>
>> -static inline u32 hdmi_read_reg(const struct hdmi_reg idx)
>> +static inline void __iomem *hdmi_pll_base(struct
>> hdmi_ip_data *ip_data)
>> {
>> - return __raw_readl(hdmi.base_wp + idx.idx);
>> + return (void __iomem *) (ip_data->base_wp +
>> ip_data->hdmi_pll_offset);
>> }
>>
>> -static inline int hdmi_wait_for_bit_change(const struct hdmi_reg idx,
>> +static inline void __iomem *hdmi_av_base(struct hdmi_ip_data
>> *ip_data)
>> +{
>> + return (void __iomem *)
>> + (ip_data->base_wp +
>> ip_data->hdmi_core_av_offset);
>> +}
>> +
>> +static inline void __iomem *hdmi_core_sys_base(struct
>> hdmi_ip_data *ip_data)
>> +{
>> + return (void __iomem *)
>> + (ip_data->base_wp +
>> ip_data->hdmi_core_sys_offset);
>> +}
>> +
>> +static inline int hdmi_wait_for_bit_change(void __iomem *base_addr,
>> + const struct hdmi_reg idx,
>> int b2, int b1, u32 val)
>> {
>> u32 t = 0;
>> - while (val != REG_GET(idx, b2, b1)) {
>> + while (val != REG_GET(base_addr, idx, b2, b1)) {
>> udelay(1);
>> if (t++ > 10000)
>> return !val;
>> @@ -225,21 +261,22 @@ int hdmi_init_display(struct
>> omap_dss_device *dssdev)
>> return 0;
>> }
>>
>> -static int hdmi_pll_init(enum hdmi_clk_refsel refsel, int dcofreq,
>> +static int hdmi_pll_init(struct hdmi_ip_data *ip_data,
>> + 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);
>> + REG_FLD_MOD(hdmi_pll_base(ip_data),
>> PLLCTRL_PLL_CONTROL, 0x0, 0, 0);
>>
>> - r = hdmi_read_reg(PLLCTRL_CFG1);
>> + r = hdmi_read_reg(hdmi_pll_base(ip_data), 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);
>> + hdmi_write_reg(hdmi_pll_base(ip_data), PLLCTRL_CFG1, r);
>
> [sp] hdmi_pll_base(ip_data) is used at many places withing this
> function. Have you checked if saving the result in a local
> variable generates any better code than repetitive ADD operation
> in the function.
>
> Same comment applies to similar use in other functions
> modified in this patch.
>
I had this comment from Tomi as well , Yes in some places it makes
sense to have a local variable to save , but then in certain functions
where there are four writes with 3 different base address it doesn't
make sense to add 3 variables.
> [snip]...[snip]
>
>> -static int hdmi_core_ddc_edid(u8 *pedid, int ext)
>> +static int hdmi_core_ddc_edid(struct hdmi_ip_data *ip_data,
>> + u8 *pedid, int ext)
>> {
>> u32 i, j;
>> char checksum = 0;
>> u32 offset = 0;
>>
>> /* Turn on CLK for DDC */
>> - REG_FLD_MOD(HDMI_CORE_AV_DPD, 0x7, 2, 0);
>> + REG_FLD_MOD(hdmi_av_base(ip_data), HDMI_CORE_AV_DPD, 0x7, 2, 0);
>>
>> /*
>> * SW HACK : Without the Delay DDC(i2c bus) reads 0 values /
>> @@ -416,21 +462,23 @@ static int hdmi_core_ddc_edid(u8
>> *pedid, int ext)
>>
>> if (!ext) {
>> /* Clk SCL Devices */
>> - REG_FLD_MOD(HDMI_CORE_DDC_CMD, 0xA, 3, 0);
>> + REG_FLD_MOD(hdmi_core_sys_base(ip_data),
>> +
>> HDMI_CORE_DDC_CMD, 0xA, 3, 0);
>>
>> /* HDMI_CORE_DDC_STATUS_IN_PROG */
>> - if (hdmi_wait_for_bit_change(HDMI_CORE_DDC_STATUS,
>> - 4, 4, 0) != 0) {
>> + if
>> (hdmi_wait_for_bit_change(hdmi_core_sys_base(ip_data),
>> + 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);
>> + REG_FLD_MOD(hdmi_core_sys_base(ip_data)
>> + ,
>> HDMI_CORE_DDC_CMD, 0x9, 3, 0);
>
> [sp] Misplaced "," at beginning of the line.
>
Not a part of the patch i suppose?
> [snip]...[snip]
>
>> static void update_hdmi_timings(struct hdmi_config *cfg,
>> @@ -1166,16 +1231,16 @@ static int hdmi_power_on(struct
>> omap_dss_device *dssdev)
>>
>> hdmi_compute_pll(dssdev, phy, &pll_data);
>>
>> - hdmi_wp_video_start(0);
>> + hdmi_wp_video_start(&hdmi.hdmi_data, 0);
>>
>> - /* config the PLL and PHY first */
>> - r = hdmi_pll_program(&pll_data);
>> + /* config the PLL and PHY hdmi_set_pll_pwrfirst */
>
> [sp] Is the change in comment intended?
I shall remove this as it is unrelated to the patch
>
> [snip]...[snip]
>
> ~sanjeev
>
--
Thanks and regards,
Mythri.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
