Re: [PATCH V3 2/3] OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function

On Fri, Sep 28, 2012 at 4:52 PM, Tomi Valkeinen [off-list ref] wrote:

On Fri, 2012-09-28 at 15:53 +0530, Chandrabhanu Mahapatra wrote:

quoted

The printk in DSSDBG function definition is replaced with dynamic debug enabled
pr_debug(). The use of dynamic debugging provides more flexibility as each debug
statement can be enabled or disabled dynamically on basis of source filename,
line number, module name etc. by writing to a control file in debugfs
filesystem. For better understanding please refer to
Documentation/dynamic-debug-howto.txt.

The DSSDBGF() differs from DSSDBG() by providing function name. However,
function name, line number, module name and thread ID can be printed through
dynamic debug by setting appropriate flags 'f','l','m' and 't' in the debugfs
control file. So, DSSDBGF instances are replaced with DSSDBG.

Signed-off-by: Chandrabhanu Mahapatra <redacted>
---
 drivers/video/omap2/dss/apply.c |    8 ++++----
 drivers/video/omap2/dss/dsi.c   |   12 ++++++------
 drivers/video/omap2/dss/dss.h   |   34 ++++++++--------------------------
 3 files changed, 18 insertions(+), 36 deletions(-)

quoted

-     DSSDBGF("%d", channel);
+     DSSDBG("Initial Config of Virtual Channel %d", channel);

A Bit Too Much Capital Letters here for my taste =). Use just one at the
beginning of the print, or none at all.

quoted

      r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel));

@@ -2814,7 +2814,7 @@ static int dsi_vc_config_source(struct platform_device *dsidev, int channel,
      if (dsi->vc[channel].source = source)
              return 0;

-     DSSDBGF("%d", channel);
+     DSSDBG("Source Config of Virtual Channel %d", channel);

Here also.

Ok.

quoted

      dsi_sync_vc(dsidev, channel);

@@ -3572,7 +3572,7 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
      int r, i;
      unsigned mask;

-     DSSDBGF();
+     DSSDBG("Entering ULPS");

      WARN_ON(!dsi_bus_is_locked(dsidev));

@@ -4276,7 +4276,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("Setting DSI clocks: 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 ffbba7e..5ef4e17 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h

@@ -25,38 +25,20 @@

 #ifdef DEBUG
 extern bool dss_debug;
-#ifdef DSS_SUBSYS_NAME
-#define DSSDBG(format, ...) \
-     if (dss_debug) \
-             printk(KERN_DEBUG "omapdss " DSS_SUBSYS_NAME ": " format, \
-             ## __VA_ARGS__)
-#else
-#define DSSDBG(format, ...) \
-     if (dss_debug) \
-             printk(KERN_DEBUG "omapdss: " format, ## __VA_ARGS__)
 #endif

-#ifdef DSS_SUBSYS_NAME
-#define DSSDBGF(format, ...) \
-     if (dss_debug) \
-             printk(KERN_DEBUG "omapdss " DSS_SUBSYS_NAME \
-                             ": %s(" format ")\n", \
-                             __func__, \
-                             ## __VA_ARGS__)
-#else
-#define DSSDBGF(format, ...) \
-     if (dss_debug) \
-             printk(KERN_DEBUG "omapdss: " \
-                             ": %s(" format ")\n", \
-                             __func__, \
-                             ## __VA_ARGS__)
+#ifdef pr_fmt
+#undef pr_fmt
 #endif

-#else /* DEBUG */
-#define DSSDBG(format, ...)
-#define DSSDBGF(format, ...)
+#ifdef DSS_SUBSYS_NAME
+#define pr_fmt(fmt) DSS_SUBSYS_NAME ": " fmt
+#else
+#define pr_fmt(fmt) fmt
 #endif

I think you could just do:

#ifdef DSS_SUBSYS_NAME
#ifdef pr_fmt
#undef pr_fmt
#endif
#define pr_fmt(fmt) DSS_SUBSYS_NAME ": " fmt
#endif

For the case where there's no DSS_SUBSYS_NAME, there's no need to undef
pr_fmt, only to redefine it again back to the original.

 Tomi

Ok. But I thought it could be more clearer if the definition of pr_fmt
is more clearer in both the cases, when DSS_SUBSYS_NAME is defined and
when not.

-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.