On Tue, 2012-09-25 at 15:00 +0530, Mahapatra, Chandrabhanu wrote:
> > This debug message is even less meaningful than the overlay number =).
> > Again, I think either keep the DSSDBGF, or print something sensible,
> > like "entering ULPS".
> >
>
> I dont think it would be wise enough to update code for one and keep
> the older version for another when both DSSDBG and DSSDBGF are almost
> one and the same. Its better to add something meaningful to the prints
> as you have mentioned like "writing ovl %d regs" and "DSI entering
> ULPS".
I didn't mean to leave DSSDBGF unchanged. I meant that it should work as
it works now, printing the func name, but using pr_debug.
> >>
> >> WARN_ON(!dsi_bus_is_locked(dsidev));
> >>
> >> @@ -4184,7 +4180,7 @@ int omapdss_dsi_set_clocks(struct omap_dss_device *dssdev,
> >> unsigned long pck;
> >> int r;
> >>
> >> - DSSDBGF("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk);
> >> + DSSDBG("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk);
> >>
> >> mutex_lock(&dsi->lock);
> >>
> >> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> >> index 5e9fd769..3a2caab 100644
> >> --- a/drivers/video/omap2/dss/dss.h
> >> +++ b/drivers/video/omap2/dss/dss.h
> >> @@ -29,38 +29,20 @@
> >>
> >> #ifdef DEBUG
> >> extern bool dss_debug;
> >
> > You still left the dss_debug option here, even if it's not used by the
> > DSSDBG anymore. What's your plan about this?
> >
> > Tomi
> >
>
> dss_debug and DEBUG need to remain here as it is being used by
> functions omap_dispc_irq_handler() and _dsi_print_reset_status() in
It would be good to clean all this in one patch series.
> dispc.c and dsi.c. I am little bit unsure of how to deal with it.
> There could be a single print in omap_dispc_irq_handler() but it is a
> bit tricky in _dsi_print_reset_status().
>
> May be a macro like this one can be used in _dsi_print_reset_status()
>
> #define DSI_FLD_GET(fld, start, end)\
> FLD_GET(dsi_read_reg(dsidev, DSI_##fld), start, end);
>
> pr_debug("PLL (%d) CIO (%d) \n PHY (%x%x%x, %d, %d, %d) \n",
> DSI_FLD_GET(PLL_STATUS, 0, 0),
> DSI_FLD_GET(COMPLEXIO_CFG1, 29, 29),
> DSI_FLD_GET(DSIPHY_CFG5, bo, bo),
> DSI_FLD_GET(DSIPHY_CFG5, b1, b1),
> ..................................................);
> This could be defined at the beginning of the function and later at its end.
Yes, I think something like that could work. I don't see any problem
with having temporary helper macros to help creating the debug message.
> As you had previously mentioned a print like
>
> #define PIS(x) (status & DSI_IRQ_##x) ? (#x " ") : ""
>
> pr_debug("DSI IRQ: 0x%x: %s%s%s",
> status,
> PIS(WAKEUP),
> PIS(RESYNC),
> PIS(PLL_LOCK));
>
> could help in print_irq_status() but I am still unsure how to deal
> with conditional statements in print_irq_status() like
> if (dss_has_feature(FEAT_MGR_LCD3))
> PIS(SYNC_LOST3);
> Should we use approach like
>
> pr_debug("DSI IRQ: 0x%x: %s%s%s%s...",
> status,
> PIS(WAKEUP),
> PIS(RESYNC),
> PIS(PLL_LOCK)
> dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : ""
> ...................................... );
Yes, that looks fine to me.
Tomi
Attachment:
signature.asc
Description: This is a digitally signed message part
