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
