| Message ID | 1343779019-10611-1-git-send-email-ricardo.neri@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2012-07-31 at 18:56 -0500, Ricardo Neri wrote: > DSS code wrongly assumes that VENC is always available as source for the external > sync signal for the display controller DIGIT channel. One cannot blindly write/read > the value of DSS_CONTROL[15] as in certain processors (e.g., OMAP5) this operation > may not be valid. If the the sync source is not read correctly, the callers of > dss_get_hdmi_venc_clk_source might make wrong assumptions about, for instance, > video timings. > > Logic is added to correctly get the sync signal based on the available displays > in the DIGIT channel. The source is set only if both VENC and HDMI are supported. > > Signed-off-by: Ricardo Neri <ricardo.neri@ti.com> > --- > v3: instead of BUG_ON calls, select only if both VENC and HDMI are available. I think they do make sense. For example, if somebody calls this function and tries to set the switch to VENC on an OMAP that doesn't have VENC, something is wrong. Your new version just skips the register write in that case, which is ok, but I think we should also yell that something is wrong. I'd have the BUG_ONs there, but change them to WARN_ON. I think we should generally try to have less BUGs and more WARNs. Also, please keep the comment about the bits being written (VENC_HDMI_SWITCH), it helps to understand the code. Tomi
On 08/01/2012 01:28 AM, Tomi Valkeinen wrote: > On Tue, 2012-07-31 at 18:56 -0500, Ricardo Neri wrote: >> DSS code wrongly assumes that VENC is always available as source for the external >> sync signal for the display controller DIGIT channel. One cannot blindly write/read >> the value of DSS_CONTROL[15] as in certain processors (e.g., OMAP5) this operation >> may not be valid. If the the sync source is not read correctly, the callers of >> dss_get_hdmi_venc_clk_source might make wrong assumptions about, for instance, >> video timings. >> >> Logic is added to correctly get the sync signal based on the available displays >> in the DIGIT channel. The source is set only if both VENC and HDMI are supported. >> >> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com> >> --- >> v3: instead of BUG_ON calls, select only if both VENC and HDMI are available. > > I think they do make sense. For example, if somebody calls this function > and tries to set the switch to VENC on an OMAP that doesn't have VENC, > something is wrong. Your new version just skips the register write in > that case, which is ok, but I think we should also yell that something > is wrong. > > I'd have the BUG_ONs there, but change them to WARN_ON. I think we > should generally try to have less BUGs and more WARNs. Also, please keep > the comment about the bits being written (VENC_HDMI_SWITCH), it helps to > understand the code. I will resubmit based on these comments. > > Tomi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c index 04b4586..29e677e 100644 --- a/drivers/video/omap2/dss/dss.c +++ b/drivers/video/omap2/dss/dss.c @@ -648,9 +648,14 @@ 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_clk_source(enum dss_hdmi_venc_clk_source_select hdmi) +void dss_select_hdmi_venc_clk_source(enum dss_hdmi_venc_clk_source_select src) { - REG_FLD_MOD(DSS_CONTROL, hdmi, 15, 15); /* VENC_HDMI_SWITCH */ + enum omap_display_type dp; + dp = dss_feat_get_supported_displays(OMAP_DSS_CHANNEL_DIGIT); + + /* Select only if we have options */ + if ((dp & OMAP_DISPLAY_TYPE_VENC) && (dp & OMAP_DISPLAY_TYPE_HDMI)) + REG_FLD_MOD(DSS_CONTROL, src, 15, 15); } enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void) @@ -661,6 +666,9 @@ enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void) if ((displays & OMAP_DISPLAY_TYPE_HDMI) == 0) return DSS_VENC_TV_CLK; + if ((displays & OMAP_DISPLAY_TYPE_VENC) == 0) + return DSS_HDMI_M_PCLK; + return REG_GET(DSS_CONTROL, 15, 15); }
DSS code wrongly assumes that VENC is always available as source for the external sync signal for the display controller DIGIT channel. One cannot blindly write/read the value of DSS_CONTROL[15] as in certain processors (e.g., OMAP5) this operation may not be valid. If the the sync source is not read correctly, the callers of dss_get_hdmi_venc_clk_source might make wrong assumptions about, for instance, video timings. Logic is added to correctly get the sync signal based on the available displays in the DIGIT channel. The source is set only if both VENC and HDMI are supported. Signed-off-by: Ricardo Neri <ricardo.neri@ti.com> --- v3: instead of BUG_ON calls, select only if both VENC and HDMI are available. v2: use BUG_ON() to simplify handling of invalid cases. drivers/video/omap2/dss/dss.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-)