Re: [PATCH] OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3425 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]