Re: [PATCH v2 2/8] OMAP4 : DSS2 : HDMI: HDMI specific display controller and dss change.

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

 



Hi Tomi,
I just rechecked the TRM.
The TRM description of the bit is VENC_HDMI_switch , Wouldnt it be
appropriate to have the same
name for the enum and the variables as suggested by you.

Thanks and regards,
Mythri.

On Thu, Mar 3, 2011 at 9:57 AM, K, Mythri P <mythripk@xxxxxx> wrote:
> Hi Tomi,
>
> On Tue, Mar 1, 2011 at 10:31 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
>> On Tue, 2011-03-01 at 08:16 -0600, K, Mythri P wrote:
>>> Adding changes to set gamma table bit for TV interface and function to select
>>> between VENC and HDMI.
>>>
>>> Signed-off-by: Mythri P K <mythripk@xxxxxx>
>>> ---
>>>  drivers/video/omap2/dss/dispc.c |    5 +++++
>>>  drivers/video/omap2/dss/dss.c   |    5 +++++
>>>  drivers/video/omap2/dss/dss.h   |    7 +++++++
>>>  3 files changed, 17 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>>> index 69e1e9d..cf385f8 100644
>>> --- a/drivers/video/omap2/dss/dispc.c
>>> +++ b/drivers/video/omap2/dss/dispc.c
>>> @@ -1224,6 +1224,11 @@ void dispc_enable_zorder(enum omap_plane plane, bool enable)
>>>       dispc_write_reg(dispc_reg_att[plane], val);
>>>  }
>>>
>>> +void dispc_enable_gamma_table(bool enable)
>>> +{
>>> +     REG_FLD_MOD(DISPC_CONFIG, enable, 9, 9);
>>> +}
>>
>> I would separate the gamma function out from this patch. These two
>> things are not related in any way.
>>
>> And I'm not quite happy with this partial gamma implementation, as only
>> the enable function is implemented. At the minimum, put BUG_ON(enable);
>> there, and a comment that this can currently only be used to disable
>> gamma support.
> I shall separate these out.
>>
>>> +
>>>  static void _dispc_set_vid_color_conv(enum omap_plane plane, bool enable)
>>>  {
>>>       u32 val;
>>> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
>>> index 99de4e1..ca76e68 100644
>>> --- a/drivers/video/omap2/dss/dss.c
>>> +++ b/drivers/video/omap2/dss/dss.c
>>> @@ -559,6 +559,11 @@ void dss_set_dac_pwrdn_bgz(bool enable)
>>>       REG_FLD_MOD(DSS_CONTROL, enable, 5, 5); /* DAC Power-Down Control */
>>>  }
>>>
>>> +void dss_select_hdmi_venc(enum dss_hdmi_venc_select hdmi)
>>> +{
>>> +     REG_FLD_MOD(DSS_CONTROL, hdmi, 15, 15); /* 0x1 for HDMI, 0x0 VENC */
>>
>> That comment is not needed, but you could put "VENC_HDMI_SWITCH" there
>> in the comments.
>>
>>> +}
>>> +
>>>  static int dss_init(bool skip_init)
>>>  {
>>>       int r;
>>> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
>>> index b6f27fe..5b5b90c 100644
>>> --- a/drivers/video/omap2/dss/dss.h
>>> +++ b/drivers/video/omap2/dss/dss.h
>>> @@ -123,6 +123,11 @@ enum dss_clk_source {
>>>       DSS_SRC_DSS1_ALWON_FCLK,
>>>  };
>>>
>>> +enum dss_hdmi_venc_select {
>>> +     DSS_VENC_SELECT,        /* Select VENC */
>>> +     DSS_HDMI_SELECT,        /* Select HDMI */
>>> +};
>>
>> If the integer value of the enum is important, like in this case, you
>> need to define the value: DSS_VENC_SELECT = 0.
>>
>> Also, the comments are not needed, they just say the same thing as the
>> enum itself.
>>
>> And only now I realized that this is a clock source switch. The enum and
>> the function should be named appropriately, for example enum
>> dss_tv_clk_out_source. And the sources are TV_CLK and HDMI_M_PCLK, so
>> the the enum values should use those as part of their name.
>>
> ok i shall name them appropriately.
>>  Tomi
>>
>>
>>
>
--
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