Hi Sanjeev, On Wed, Mar 9, 2011 at 8:19 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: Wednesday, March 09, 2011 5:15 PM >> To: linux-omap@xxxxxxxxxxxxxxx; Valkeinen, Tomi >> Cc: K, Mythri P >> Subject: [PATCH v4 3/9] OMAP4 : DSS2 : HDMI: HDMI dispc gamma table >> disable. > [sp] HDMI seems repetitive in the subject in this and some of the subsequent > Patches. >> >> Adding changes to set gamma table bit for TV interface to make sure it is >> disabled > > [sp] Is this bit expected to be "enabled" by default? If no, how can/would > this bit get enabled. > > The subject and description of the patch doesn't match the contents. > The patch is adding a function that can *both* enable/disable gamma > table - not just disable. >We have seen several examples of the reset bit being changed across Silicon versions , So this is to set the gamma enable to 0 Effectively a disable., so i have added a comment that this still needs to add enable functionality. >> >> Signed-off-by: Mythri P K <mythripk@xxxxxx> >> --- >> drivers/video/omap2/dss/dispc.c | 10 ++++++++++ >> drivers/video/omap2/dss/dss.h | 1 + >> 2 files changed, 11 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/video/omap2/dss/dispc.c >> b/drivers/video/omap2/dss/dispc.c >> index 4a368c4..0defe16 100644 >> --- a/drivers/video/omap2/dss/dispc.c >> +++ b/drivers/video/omap2/dss/dispc.c >> @@ -1008,6 +1008,16 @@ void dispc_set_burst_size(enum omap_plane plane, >> enable_clocks(0); >> } >> >> +void dispc_enable_gamma_table(bool enable) >> +{ >> + /* This is partially implemented to support only >> + * disabling of the gamma table. >> + */ >> + BUG_ON(enable); > [sp] Is BUG_ON really needed. Can't this be a WARN or equiv? > >> + >> + REG_FLD_MOD(DISPC_CONFIG, enable, 9, 9); > > [sp] I don't understand HDMI much, but if enabling gamma causes BUG_ON, > then why would we need implementation to disable it. > > Instead, shouldn't the function be empty - if it can't be avoided > altogether? > >> +} >> + >> static void _dispc_set_vid_color_conv(enum omap_plane plane, bool enable) >> { >> u32 val; >> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h >> index 29f31fd..008cbf4 100644 >> --- a/drivers/video/omap2/dss/dss.h >> +++ b/drivers/video/omap2/dss/dss.h >> @@ -363,6 +363,7 @@ void dispc_set_plane_size(enum omap_plane plane, u16 >> width, u16 height); >> void dispc_set_channel_out(enum omap_plane plane, >> enum omap_channel channel_out); >> >> +void dispc_enable_gamma_table(bool enable); > > [sp] Is this patch really standalone? I suggest merging it with the patch > where the API is actually used. > > In effect, nothing gets disabled in this patch. > >> int dispc_setup_plane(enum omap_plane plane, >> u32 paddr, u16 screen_width, >> u16 pos_x, u16 pos_y, >> -- >> 1.5.6.3 >> >> -- >> 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 > -- 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
