On Tue, 2012-09-25 at 15:00 +0530, Mahapatra, Chandrabhanu wrote:

quoted

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.

quoted

quoted

      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