Re: [PATCH 1/8] OMAP4: DSS: HDMI: HDMI clean up to pass base_address

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sanjeev,

On Mon, Jun 20, 2011 at 7:03 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: Friday, June 17, 2011 1:47 PM
>> To: linux-omap@xxxxxxxxxxxxxxx; 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 <mythripk@xxxxxx>
>> ---
>>  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux