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
