[PATCH] OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function

[PATCH] OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function

[Chandrabhanu Mahapatra]

The printk in DSSDBG function definition is replaced with dynamic debug enabled
pr_debug(). The use of dynamic debugging provides more flexiblity 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 undertsanding 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 appropiate flags 'f','l','m' and 't' in the debugfs
control file. So, DSSDBGF instances are replaced with DSSDBG.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/apply.c |    8 ++++----
 drivers/video/omap2/dss/dsi.c   |   12 ++++--------
 drivers/video/omap2/dss/dss.h   |   34 ++++++++--------------------------
 3 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 6354bb8..cb86d94 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -559,7 +559,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
 	struct mgr_priv_data *mp;
 	int r;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("%d", ovl->id);
 
 	if (!op->enabled || !op->info_dirty)
 		return;
@@ -594,7 +594,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl)
 	struct ovl_priv_data *op = get_ovl_priv(ovl);
 	struct mgr_priv_data *mp;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("%d", ovl->id);
 
 	if (!op->extra_info_dirty)
 		return;
@@ -618,7 +618,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr)
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 	struct omap_overlay *ovl;
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("%d", mgr->id);
 
 	if (!mp->enabled)
 		return;
@@ -644,7 +644,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr)
 {
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("%d", mgr->id);
 
 	if (!mp->extra_info_dirty)
 		return;
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 8d815e3..8304cc6b 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1525,8 +1525,6 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev,
 	u8 regn_start, regn_end, regm_start, regm_end;
 	u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end;
 
-	DSSDBGF();
-
 	dsi->current_cinfo.clkin = cinfo->clkin;
 	dsi->current_cinfo.fint = cinfo->fint;
 	dsi->current_cinfo.clkin4ddr = cinfo->clkin4ddr;
@@ -2334,8 +2332,6 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
 	int r;
 	u32 l;
 
-	DSSDBGF();
-
 	r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev));
 	if (r)
 		return r;
@@ -2686,7 +2682,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel)
 {
 	u32 r;
 
-	DSSDBGF("%d", channel);
+	DSSDBG("%d", channel);
 
 	r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel));
 
@@ -2718,7 +2714,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("%d", channel);
 
 	dsi_sync_vc(dsidev, channel);
 
@@ -3475,7 +3471,7 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
 	int r, i;
 	unsigned mask;
 
-	DSSDBGF();
+	DSSDBG("");
 
 	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;
-#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
 
+#define DSSDBG(format, ...) \
+	pr_debug(format, ## __VA_ARGS__)
 
 #ifdef DSS_SUBSYS_NAME
 #define DSSERR(format, ...) \
-- 
1.7.10

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

[Tomi Valkeinen]

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

On Tue, 2012-09-25 at 11:33 +0530, Chandrabhanu Mahapatra wrote:
> The printk in DSSDBG function definition is replaced with dynamic debug enabled
> pr_debug(). The use of dynamic debugging provides more flexiblity 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 undertsanding 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 appropiate flags 'f','l','m' and 't' in the debugfs
> control file. So, DSSDBGF instances are replaced with DSSDBG.

Typos with: flexiblity, undertsanding, appropiate.

> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
>  drivers/video/omap2/dss/apply.c |    8 ++++----
>  drivers/video/omap2/dss/dsi.c   |   12 ++++--------
>  drivers/video/omap2/dss/dss.h   |   34 ++++++++--------------------------
>  3 files changed, 16 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 6354bb8..cb86d94 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -559,7 +559,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
>  	struct mgr_priv_data *mp;
>  	int r;
>  
> -	DSSDBGF("%d", ovl->id);
> +	DSSDBG("%d", ovl->id);

I don't think this is good. It's true that dyn-debug can print the
function name, but that's optional. The debug message should be somehow
sensible independently, but in this case only a number is printed which
is totally meaningless.

Either the messages should be modified to give a hint what's going on,
or the DSSDBGF could be kept for now. In the above case the debug
message could be something like "writing ovl %d regs".

However, I think it'd be easier just to keep the DSSDBGF for now, and
remove it gradually.

>  	if (!op->enabled || !op->info_dirty)
>  		return;
> @@ -594,7 +594,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl)
>  	struct ovl_priv_data *op = get_ovl_priv(ovl);
>  	struct mgr_priv_data *mp;
>  
> -	DSSDBGF("%d", ovl->id);
> +	DSSDBG("%d", ovl->id);
>  
>  	if (!op->extra_info_dirty)
>  		return;
> @@ -618,7 +618,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr)
>  	struct mgr_priv_data *mp = get_mgr_priv(mgr);
>  	struct omap_overlay *ovl;
>  
> -	DSSDBGF("%d", mgr->id);
> +	DSSDBG("%d", mgr->id);
>  
>  	if (!mp->enabled)
>  		return;
> @@ -644,7 +644,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr)
>  {
>  	struct mgr_priv_data *mp = get_mgr_priv(mgr);
>  
> -	DSSDBGF("%d", mgr->id);
> +	DSSDBG("%d", mgr->id);
>  
>  	if (!mp->extra_info_dirty)
>  		return;
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 8d815e3..8304cc6b 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -1525,8 +1525,6 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev,
>  	u8 regn_start, regn_end, regm_start, regm_end;
>  	u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end;
>  
> -	DSSDBGF();
> -
>  	dsi->current_cinfo.clkin = cinfo->clkin;
>  	dsi->current_cinfo.fint = cinfo->fint;
>  	dsi->current_cinfo.clkin4ddr = cinfo->clkin4ddr;
> @@ -2334,8 +2332,6 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
>  	int r;
>  	u32 l;
>  
> -	DSSDBGF();
> -
>  	r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev));
>  	if (r)
>  		return r;
> @@ -2686,7 +2682,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel)
>  {
>  	u32 r;
>  
> -	DSSDBGF("%d", channel);
> +	DSSDBG("%d", channel);
>  
>  	r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel));
>  
> @@ -2718,7 +2714,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("%d", channel);
>  
>  	dsi_sync_vc(dsidev, channel);
>  
> @@ -3475,7 +3471,7 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
>  	int r, i;
>  	unsigned mask;
>  
> -	DSSDBGF();
> +	DSSDBG("");

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".

>  
>  	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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mahapatra, Chandrabhanu]

>> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
>> index 6354bb8..cb86d94 100644
>> --- a/drivers/video/omap2/dss/apply.c
>> +++ b/drivers/video/omap2/dss/apply.c
>> @@ -559,7 +559,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
>>       struct mgr_priv_data *mp;
>>       int r;
>>
>> -     DSSDBGF("%d", ovl->id);
>> +     DSSDBG("%d", ovl->id);
>
> I don't think this is good. It's true that dyn-debug can print the
> function name, but that's optional. The debug message should be somehow
> sensible independently, but in this case only a number is printed which
> is totally meaningless.
>
> Either the messages should be modified to give a hint what's going on,
> or the DSSDBGF could be kept for now. In the above case the debug
> message could be something like "writing ovl %d regs".
>
> However, I think it'd be easier just to keep the DSSDBGF for now, and
> remove it gradually.
>
>>       if (!op->enabled || !op->info_dirty)
>>               return;
>> @@ -594,7 +594,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl)
>>       struct ovl_priv_data *op = get_ovl_priv(ovl);
>>       struct mgr_priv_data *mp;
>>
>> -     DSSDBGF("%d", ovl->id);
>> +     DSSDBG("%d", ovl->id);
>>
>>       if (!op->extra_info_dirty)
>>               return;
>> @@ -618,7 +618,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr)
>>       struct mgr_priv_data *mp = get_mgr_priv(mgr);
>>       struct omap_overlay *ovl;
>>
>> -     DSSDBGF("%d", mgr->id);
>> +     DSSDBG("%d", mgr->id);
>>
>>       if (!mp->enabled)
>>               return;
>> @@ -644,7 +644,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr)
>>  {
>>       struct mgr_priv_data *mp = get_mgr_priv(mgr);
>>
>> -     DSSDBGF("%d", mgr->id);
>> +     DSSDBG("%d", mgr->id);
>>
>>       if (!mp->extra_info_dirty)
>>               return;
>> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
>> index 8d815e3..8304cc6b 100644
>> --- a/drivers/video/omap2/dss/dsi.c
>> +++ b/drivers/video/omap2/dss/dsi.c
>> @@ -1525,8 +1525,6 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev,
>>       u8 regn_start, regn_end, regm_start, regm_end;
>>       u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end;
>>
>> -     DSSDBGF();
>> -
>>       dsi->current_cinfo.clkin = cinfo->clkin;
>>       dsi->current_cinfo.fint = cinfo->fint;
>>       dsi->current_cinfo.clkin4ddr = cinfo->clkin4ddr;
>> @@ -2334,8 +2332,6 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
>>       int r;
>>       u32 l;
>>
>> -     DSSDBGF();
>> -
>>       r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev));
>>       if (r)
>>               return r;
>> @@ -2686,7 +2682,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel)
>>  {
>>       u32 r;
>>
>> -     DSSDBGF("%d", channel);
>> +     DSSDBG("%d", channel);
>>
>>       r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel));
>>
>> @@ -2718,7 +2714,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("%d", channel);
>>
>>       dsi_sync_vc(dsidev, channel);
>>
>> @@ -3475,7 +3471,7 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
>>       int r, i;
>>       unsigned mask;
>>
>> -     DSSDBGF();
>> +     DSSDBG("");
>
> 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".

>>
>>       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
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.

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) : ""
  ...................................... );


-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.

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

[Tomi Valkeinen]

[-- 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 --]

[PATCH V2 0/2] OMAPDSS: Enable dynamic debug printing

[Chandrabhanu Mahapatra]

Hi everyone,
this patch series aims at cleaning up of DSS of printk()'s enabled with
dss_debug and replace them with generic dynamic debug printing.

The 1st patch 
 * replaces printk() in DSSDBG definition with pr_debug()
 * removes DSSDBGF definition and replaces its instances with DSSDBG() 
The 2nd patch
 * cleans up printk()'s in omap_dispc_unregister_isr() and 
   _dsi_print_reset_status() with pr_debug()
 * removes dss_debug variable

Changes with respect to V1:
 * added debug messages to DSSDBG calls replacing DSSDBGF
 * added patch "OMAPDSS: Remove dss_debug variable"

All your comments and suggestions are welcome.

Regards,
Chandrabhanu

Chandrabhanu Mahapatra (2):
  OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function
  OMAPDSS: Remove dss_debug variable

 drivers/video/omap2/dss/apply.c |    8 +++----
 drivers/video/omap2/dss/core.c  |    5 ----
 drivers/video/omap2/dss/dispc.c |   39 +++++++++++--------------------
 drivers/video/omap2/dss/dsi.c   |   49 ++++++++++++++++-----------------------
 drivers/video/omap2/dss/dss.h   |   34 +++++----------------------
 5 files changed, 44 insertions(+), 91 deletions(-)

-- 
1.7.10

[PATCH V2 1/2] OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function

[Chandrabhanu Mahapatra]

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 <cmahapatra@ti.com>
---
 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(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 6354bb8..e92f5fb 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -559,7 +559,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
 	struct mgr_priv_data *mp;
 	int r;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("writing ovl %d regs", ovl->id);
 
 	if (!op->enabled || !op->info_dirty)
 		return;
@@ -594,7 +594,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl)
 	struct ovl_priv_data *op = get_ovl_priv(ovl);
 	struct mgr_priv_data *mp;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("writing ovl %d regs extra", ovl->id);
 
 	if (!op->extra_info_dirty)
 		return;
@@ -618,7 +618,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr)
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 	struct omap_overlay *ovl;
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("writing mgr %d regs", mgr->id);
 
 	if (!mp->enabled)
 		return;
@@ -644,7 +644,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr)
 {
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("writing mgr %d regs extra", mgr->id);
 
 	if (!mp->extra_info_dirty)
 		return;
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 8d815e3..18834f3 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1525,7 +1525,7 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev,
 	u8 regn_start, regn_end, regm_start, regm_end;
 	u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end;
 
-	DSSDBGF();
+	DSSDBG("DSI PLL clock config starts");
 
 	dsi->current_cinfo.clkin = cinfo->clkin;
 	dsi->current_cinfo.fint = cinfo->fint;
@@ -2334,7 +2334,7 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
 	int r;
 	u32 l;
 
-	DSSDBGF();
+	DSSDBG("DSI CIO init starts");
 
 	r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dssdev));
 	if (r)
@@ -2686,7 +2686,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel)
 {
 	u32 r;
 
-	DSSDBGF("%d", channel);
+	DSSDBG("Initial Config of Virtual Channel %d", channel);
 
 	r = dsi_read_reg(dsidev, DSI_VC_CTRL(channel));
 
@@ -2718,7 +2718,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);
 
 	dsi_sync_vc(dsidev, channel);
 
@@ -3475,7 +3475,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));
 
@@ -4184,7 +4184,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 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;
-#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
 
+#define DSSDBG(format, ...) \
+	pr_debug(format, ## __VA_ARGS__)
 
 #ifdef DSS_SUBSYS_NAME
 #define DSSERR(format, ...) \
-- 
1.7.10

[PATCH V2 2/2] OMAPDSS: Remove dss_debug variable

[Chandrabhanu Mahapatra]

The debug prints in omap_dispc_unregister_isr() and _dsi_print_reset_status()
are replaced with dynamic debug enabled pr_debug(). So, as the final dependency
on dss_debug variable is replaced with dyndbg, the dss_debug variable is
removed.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/core.c  |    5 -----
 drivers/video/omap2/dss/dispc.c |   39 ++++++++++++++-------------------------
 drivers/video/omap2/dss/dsi.c   |   37 ++++++++++++++-----------------------
 drivers/video/omap2/dss/dss.h   |    4 ----
 4 files changed, 28 insertions(+), 57 deletions(-)

diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 58bd9c2..5d77c86 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -52,11 +52,6 @@ static char *def_disp_name;
 module_param_named(def_disp, def_disp_name, charp, 0);
 MODULE_PARM_DESC(def_disp, "default display name");
 
-#ifdef DEBUG
-bool dss_debug;
-module_param_named(debug, dss_debug, bool, 0644);
-#endif
-
 /* REGULATORS */
 
 struct regulator *dss_get_vdds_dsi(void)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 1121823..6c813a6 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3447,34 +3447,26 @@ int omap_dispc_unregister_isr(omap_dispc_isr_t isr, void *arg, u32 mask)
 }
 EXPORT_SYMBOL(omap_dispc_unregister_isr);
 
-#ifdef DEBUG
 static void print_irq_status(u32 status)
 {
 	if ((status & dispc.irq_error_mask) = 0)
 		return;
 
-	printk(KERN_DEBUG "DISPC IRQ: 0x%x: ", status);
-
-#define PIS(x) \
-	if (status & DISPC_IRQ_##x) \
-		printk(#x " ");
-	PIS(GFX_FIFO_UNDERFLOW);
-	PIS(OCP_ERR);
-	PIS(VID1_FIFO_UNDERFLOW);
-	PIS(VID2_FIFO_UNDERFLOW);
-	if (dss_feat_get_num_ovls() > 3)
-		PIS(VID3_FIFO_UNDERFLOW);
-	PIS(SYNC_LOST);
-	PIS(SYNC_LOST_DIGIT);
-	if (dss_has_feature(FEAT_MGR_LCD2))
-		PIS(SYNC_LOST2);
-	if (dss_has_feature(FEAT_MGR_LCD3))
-		PIS(SYNC_LOST3);
+#define PIS(x) (status & DISPC_IRQ_##x) ? (#x " ") : ""
+
+	pr_debug("DISPC IRQ: 0x%x: %s%s%s%s%s%s%s%s%s\n",
+		status,
+		PIS(OCP_ERR),
+		PIS(GFX_FIFO_UNDERFLOW),
+		PIS(VID1_FIFO_UNDERFLOW),
+		PIS(VID2_FIFO_UNDERFLOW),
+		dss_feat_get_num_ovls() > 3 ? PIS(VID3_FIFO_UNDERFLOW) : "",
+		PIS(SYNC_LOST),
+		PIS(SYNC_LOST_DIGIT),
+		dss_has_feature(FEAT_MGR_LCD2) ? PIS(SYNC_LOST2) : "",
+		dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "");
 #undef PIS
-
-	printk("\n");
 }
-#endif
 
 /* Called from dss.c. Note that we don't touch clocks here,
  * but we presume they are on because we got an IRQ. However,
@@ -3507,10 +3499,7 @@ static irqreturn_t omap_dispc_irq_handler(int irq, void *arg)
 	spin_unlock(&dispc.irq_stats_lock);
 #endif
 
-#ifdef DEBUG
-	if (dss_debug)
-		print_irq_status(irqstatus);
-#endif
+	print_irq_status(irqstatus);
 	/* Ack the interrupt. Do it here before clocks are possibly turned
 	 * off */
 	dispc_write_reg(DISPC_IRQSTATUS, irqstatus);
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 18834f3..ed7ff72 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1100,28 +1100,16 @@ static inline void dsi_enable_pll_clock(struct platform_device *dsidev,
 	}
 }
 
-#ifdef DEBUG
 static void _dsi_print_reset_status(struct platform_device *dsidev)
 {
 	u32 l;
 	int b0, b1, b2;
 
-	if (!dss_debug)
-		return;
-
 	/* A dummy read using the SCP interface to any DSIPHY register is
 	 * required after DSIPHY reset to complete the reset of the DSI complex
 	 * I/O. */
 	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
 
-	printk(KERN_DEBUG "DSI resets: ");
-
-	l = dsi_read_reg(dsidev, DSI_PLL_STATUS);
-	printk("PLL (%d) ", FLD_GET(l, 0, 0));
-
-	l = dsi_read_reg(dsidev, DSI_COMPLEXIO_CFG1);
-	printk("CIO (%d) ", FLD_GET(l, 29, 29));
-
 	if (dss_has_feature(FEAT_DSI_REVERSE_TXCLKESC)) {
 		b0 = 28;
 		b1 = 27;
@@ -1132,18 +1120,21 @@ static void _dsi_print_reset_status(struct platform_device *dsidev)
 		b2 = 26;
 	}
 
-	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
-	printk("PHY (%x%x%x, %d, %d, %d)\n",
-			FLD_GET(l, b0, b0),
-			FLD_GET(l, b1, b1),
-			FLD_GET(l, b2, b2),
-			FLD_GET(l, 29, 29),
-			FLD_GET(l, 30, 30),
-			FLD_GET(l, 31, 31));
+#define DSI_FLD_GET(fld, start, end)\
+	FLD_GET(dsi_read_reg(dsidev, DSI_##fld), start, end)
+
+	pr_debug("DSI resets: PLL (%d) CIO (%d) 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, b0, b0),
+		DSI_FLD_GET(DSIPHY_CFG5, b1, b1),
+		DSI_FLD_GET(DSIPHY_CFG5, b2, b2),
+		DSI_FLD_GET(DSIPHY_CFG5, 29, 29),
+		DSI_FLD_GET(DSIPHY_CFG5, 30, 30),
+		DSI_FLD_GET(DSIPHY_CFG5, 31, 31));
+
+#undef DSI_FLD_GET
 }
-#else
-#define _dsi_print_reset_status(x)
-#endif
 
 static inline int dsi_if_enable(struct platform_device *dsidev, bool enable)
 {
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 3a2caab..0015803 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -27,10 +27,6 @@
 #define DEBUG
 #endif
 
-#ifdef DEBUG
-extern bool dss_debug;
-#endif
-
 #ifdef pr_fmt
 #undef pr_fmt
 #endif
-- 
1.7.10

Re: [PATCH V2 0/2] OMAPDSS: Enable dynamic debug printing

[Tomi Valkeinen]

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

Hi,

On Wed, 2012-09-26 at 10:45 +0530, Chandrabhanu Mahapatra wrote:
> Hi everyone,
> this patch series aims at cleaning up of DSS of printk()'s enabled with
> dss_debug and replace them with generic dynamic debug printing.
> 
> The 1st patch 
>  * replaces printk() in DSSDBG definition with pr_debug()
>  * removes DSSDBGF definition and replaces its instances with DSSDBG() 
> The 2nd patch
>  * cleans up printk()'s in omap_dispc_unregister_isr() and 
>    _dsi_print_reset_status() with pr_debug()
>  * removes dss_debug variable
> 
> Changes with respect to V1:
>  * added debug messages to DSSDBG calls replacing DSSDBGF
>  * added patch "OMAPDSS: Remove dss_debug variable"
> 
> All your comments and suggestions are welcome.

This doesn't work quite correctly. The problem is in dss.h, where we
define DEBUG if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. The thing is,
DEBUG should be defined before including the kernel headers where the
pr_debug etc are defined.

So if you try the patches without dynamic debugging enabled, you won't
get any debug outputs at all, even if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is
set.

And for dynamic debug, the Kconfig help says:

If a source file is compiled with DEBUG flag set, any       
pr_debug() calls in it are enabled by default, but can be   
disabled at runtime as below.  Note that DEBUG flag is      
turned on by many CONFIG_*DEBUG* options.                   

So if we have CONFIG_OMAP2_DSS_DEBUG_SUPPORT set, all the pr_debugs
should be enabled by default, which is not the case, again because DEBUG
is defined too late.

I think setting DEBUG in dss.h should be removed, and instead DEBUG
should be set in the makefile if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 0/2] OMAPDSS: Enable dynamic debug printing

[Mahapatra, Chandrabhanu]

On Wed, Sep 26, 2012 at 7:59 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> This doesn't work quite correctly. The problem is in dss.h, where we
> define DEBUG if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. The thing is,
> DEBUG should be defined before including the kernel headers where the
> pr_debug etc are defined.
>
> So if you try the patches without dynamic debugging enabled, you won't
> get any debug outputs at all, even if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is
> set.
>
> And for dynamic debug, the Kconfig help says:
>
> If a source file is compiled with DEBUG flag set, any
> pr_debug() calls in it are enabled by default, but can be
> disabled at runtime as below.  Note that DEBUG flag is
> turned on by many CONFIG_*DEBUG* options.
>
> So if we have CONFIG_OMAP2_DSS_DEBUG_SUPPORT set, all the pr_debugs
> should be enabled by default, which is not the case, again because DEBUG
> is defined too late.
>
> I think setting DEBUG in dss.h should be removed, and instead DEBUG
> should be set in the makefile if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set.
>
>  Tomi
>

Well the documentation lags in describing about the DEBUG flag. I
should have checked DYNAMIC_DEBUG in Kconfig and pr_debug definition
in printk.h file.

#if defined(CONFIG_DYNAMIC_DEBUG)
/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
#define pr_debug(fmt, ...) \
        dynamic_pr_debug(fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define pr_debug(fmt, ...) \
        printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#else
#define pr_debug(fmt, ...) \
        no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#endif

As per the definition above pr_debug is dynamic with
CONFIG_DYNAMIC_DEBUG set or else with DEBUG set it is just a normal
kernel debug printk as you have mentioned.
I still don't get how even if DEBUG is set before DSSDBG() is defined
in dss.c pr_debug() fails to enable.
Well anyways, how to do the same in the Makefile? I tried adding
ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DEBUG
to makefile in dss directory but of no use.

-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.

Re: [PATCH V2 0/2] OMAPDSS: Enable dynamic debug printing

[Tomi Valkeinen]

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

On Thu, 2012-09-27 at 16:20 +0530, Mahapatra, Chandrabhanu wrote:
> On Wed, Sep 26, 2012 at 7:59 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 
> > This doesn't work quite correctly. The problem is in dss.h, where we
> > define DEBUG if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set. The thing is,
> > DEBUG should be defined before including the kernel headers where the
> > pr_debug etc are defined.
> >
> > So if you try the patches without dynamic debugging enabled, you won't
> > get any debug outputs at all, even if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is
> > set.
> >
> > And for dynamic debug, the Kconfig help says:
> >
> > If a source file is compiled with DEBUG flag set, any
> > pr_debug() calls in it are enabled by default, but can be
> > disabled at runtime as below.  Note that DEBUG flag is
> > turned on by many CONFIG_*DEBUG* options.
> >
> > So if we have CONFIG_OMAP2_DSS_DEBUG_SUPPORT set, all the pr_debugs
> > should be enabled by default, which is not the case, again because DEBUG
> > is defined too late.
> >
> > I think setting DEBUG in dss.h should be removed, and instead DEBUG
> > should be set in the makefile if CONFIG_OMAP2_DSS_DEBUG_SUPPORT is set.
> >
> >  Tomi
> >
> 
> Well the documentation lags in describing about the DEBUG flag. I
> should have checked DYNAMIC_DEBUG in Kconfig and pr_debug definition
> in printk.h file.
> 
> #if defined(CONFIG_DYNAMIC_DEBUG)
> /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
> #define pr_debug(fmt, ...) \
>         dynamic_pr_debug(fmt, ##__VA_ARGS__)
> #elif defined(DEBUG)
> #define pr_debug(fmt, ...) \
>         printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #else
> #define pr_debug(fmt, ...) \
>         no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #endif
> 
> As per the definition above pr_debug is dynamic with
> CONFIG_DYNAMIC_DEBUG set or else with DEBUG set it is just a normal
> kernel debug printk as you have mentioned.
> I still don't get how even if DEBUG is set before DSSDBG() is defined
> in dss.c pr_debug() fails to enable.

Because printk.h is included without DEBUG, thus pr_debug is defined as
no_printk.

> Well anyways, how to do the same in the Makefile? I tried adding
> ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DEBUG
> to makefile in dss directory but of no use.

-D option for the compiler is used to set defines. So it should be
-DDEBUG

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH V3 0/3] OMAPDSS: Enable dynamic debug printing

[Chandrabhanu Mahapatra]

Hi everyone,
this patch series aims at cleaning up of DSS of printk()'s enabled with
dss_debug and replace them with generic dynamic debug printing.

The 1st patch
 * moved DEBUG flag definition to Makefile
The 2nd patch 
 * replaces printk() in DSSDBG definition with pr_debug()
 * removes DSSDBGF definition and replaces its instances with DSSDBG() 
The 3rd patch
 * cleans up printk()'s in omap_dispc_unregister_isr() and 
   _dsi_print_reset_status() with pr_debug()
 * removes dss_debug variable

Changes from V1 to V2:
 * added debug messages to DSSDBG calls
 * added patch "OMAPDSS: Remove dss_debug variable"

Changes from V2 to V3
 * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile"

All your comments and suggestions are welcome.

Refenence Tree:
git://gitorious.org/linux-omap-dss2/chandrabhanus-linux.git dss_cleanup

Regards,
Chandrabhanu

Chandrabhanu Mahapatra (3):
  OMAPDSS: Move definition of DEBUG flag to Makefile
  OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function
  OMAPDSS: Remove dss_debug variable

 drivers/video/omap2/dss/Makefile |    1 +
 drivers/video/omap2/dss/apply.c  |    8 +++----
 drivers/video/omap2/dss/core.c   |    5 ----
 drivers/video/omap2/dss/dispc.c  |   39 +++++++++++-------------------
 drivers/video/omap2/dss/dsi.c    |   49 ++++++++++++++++----------------------
 drivers/video/omap2/dss/dss.h    |   38 +++++------------------------
 6 files changed, 45 insertions(+), 95 deletions(-)

-- 
1.7.10

[PATCH V3 1/3] OMAPDSS: Move definition of DEBUG flag to Makefile

[Chandrabhanu Mahapatra]

In OMAPDSS the DEBUG flag is set only after the OMAPDSS module is called, for
which the debugging capabilities are available only after its proper
initialization. As a result of which tracking of bugs prior to or during initial
process becomes difficult. So, the definition of DEBUG is being moved to the
corresponding Makefile.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/Makefile |    1 +
 drivers/video/omap2/dss/dss.h    |    4 ----
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/Makefile b/drivers/video/omap2/dss/Makefile
index 4549869..86493e3 100644
--- a/drivers/video/omap2/dss/Makefile
+++ b/drivers/video/omap2/dss/Makefile
@@ -8,3 +8,4 @@ omapdss-$(CONFIG_OMAP2_DSS_SDI) += sdi.o
 omapdss-$(CONFIG_OMAP2_DSS_DSI) += dsi.o
 omapdss-$(CONFIG_OMAP4_DSS_HDMI) += hdmi.o \
 				    hdmi_panel.o ti_hdmi_4xxx_ip.o
+ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DDEBUG
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 6728892..ffbba7e 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -23,10 +23,6 @@
 #ifndef __OMAP2_DSS_H
 #define __OMAP2_DSS_H
 
-#ifdef CONFIG_OMAP2_DSS_DEBUG_SUPPORT
-#define DEBUG
-#endif
-
 #ifdef DEBUG
 extern bool dss_debug;
 #ifdef DSS_SUBSYS_NAME
-- 
1.7.10

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

[Chandrabhanu Mahapatra]

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 <cmahapatra@ti.com>
---
 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(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 19d66f4..e923d9f 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -573,7 +573,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
 	struct mgr_priv_data *mp;
 	int r;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("writing ovl %d regs", ovl->id);
 
 	if (!op->enabled || !op->info_dirty)
 		return;
@@ -608,7 +608,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl)
 	struct ovl_priv_data *op = get_ovl_priv(ovl);
 	struct mgr_priv_data *mp;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("writing ovl %d regs extra", ovl->id);
 
 	if (!op->extra_info_dirty)
 		return;
@@ -632,7 +632,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr)
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 	struct omap_overlay *ovl;
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("writing mgr %d regs", mgr->id);
 
 	if (!mp->enabled)
 		return;
@@ -658,7 +658,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr)
 {
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("writing mgr %d regs extra", mgr->id);
 
 	if (!mp->extra_info_dirty)
 		return;
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index e37e6d8..3b524cd 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1612,7 +1612,7 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev,
 	u8 regn_start, regn_end, regm_start, regm_end;
 	u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end;
 
-	DSSDBGF();
+	DSSDBG("DSI PLL clock config starts");
 
 	dsi->current_cinfo.clkin = cinfo->clkin;
 	dsi->current_cinfo.fint = cinfo->fint;
@@ -2431,7 +2431,7 @@ static int dsi_cio_init(struct platform_device *dsidev)
 	int r;
 	u32 l;
 
-	DSSDBGF();
+	DSSDBG("DSI CIO init starts");
 
 	r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dsidev));
 	if (r)
@@ -2782,7 +2782,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel)
 {
 	u32 r;
 
-	DSSDBGF("%d", channel);
+	DSSDBG("Initial Config of Virtual Channel %d", channel);
 
 	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);
 
 	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
 
+#define DSSDBG(format, ...) \
+	pr_debug(format, ## __VA_ARGS__)
 
 #ifdef DSS_SUBSYS_NAME
 #define DSSERR(format, ...) \
-- 
1.7.10

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

[Tomi Valkeinen]

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

On Fri, 2012-09-28 at 15:53 +0530, Chandrabhanu Mahapatra wrote:
> 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 <cmahapatra@ti.com>
> ---
>  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(-)
> 

> -	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. 

>  
>  	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.

>  
>  	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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mahapatra, Chandrabhanu]

On Fri, Sep 28, 2012 at 4:52 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Fri, 2012-09-28 at 15:53 +0530, Chandrabhanu Mahapatra wrote:
>> 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 <cmahapatra@ti.com>
>> ---
>>  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(-)
>>
>
>> -     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.
>
>>
>>       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.

>>
>>       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.

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

[Tomi Valkeinen]

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

On Fri, 2012-09-28 at 17:00 +0530, Mahapatra, Chandrabhanu wrote:

> > 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.

Ok, then it's fine for me.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH V3 3/3] OMAPDSS: Remove dss_debug variable

[Chandrabhanu Mahapatra]

The debug prints in omap_dispc_unregister_isr() and _dsi_print_reset_status()
are replaced with dynamic debug enabled pr_debug(). So, as the final dependency
on dss_debug variable is replaced with dyndbg, the dss_debug variable is
removed.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/core.c  |    5 -----
 drivers/video/omap2/dss/dispc.c |   39 ++++++++++++++-------------------------
 drivers/video/omap2/dss/dsi.c   |   37 ++++++++++++++-----------------------
 drivers/video/omap2/dss/dss.h   |    4 ----
 4 files changed, 28 insertions(+), 57 deletions(-)

diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index b2af72d..e51bd84 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -53,11 +53,6 @@ static char *def_disp_name;
 module_param_named(def_disp, def_disp_name, charp, 0);
 MODULE_PARM_DESC(def_disp, "default display name");
 
-#ifdef DEBUG
-bool dss_debug;
-module_param_named(debug, dss_debug, bool, 0644);
-#endif
-
 const char *dss_get_default_display_name(void)
 {
 	return core.default_display_name;
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index a173a94..85d2128 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3669,34 +3669,26 @@ int omap_dispc_unregister_isr(omap_dispc_isr_t isr, void *arg, u32 mask)
 }
 EXPORT_SYMBOL(omap_dispc_unregister_isr);
 
-#ifdef DEBUG
 static void print_irq_status(u32 status)
 {
 	if ((status & dispc.irq_error_mask) = 0)
 		return;
 
-	printk(KERN_DEBUG "DISPC IRQ: 0x%x: ", status);
-
-#define PIS(x) \
-	if (status & DISPC_IRQ_##x) \
-		printk(#x " ");
-	PIS(GFX_FIFO_UNDERFLOW);
-	PIS(OCP_ERR);
-	PIS(VID1_FIFO_UNDERFLOW);
-	PIS(VID2_FIFO_UNDERFLOW);
-	if (dss_feat_get_num_ovls() > 3)
-		PIS(VID3_FIFO_UNDERFLOW);
-	PIS(SYNC_LOST);
-	PIS(SYNC_LOST_DIGIT);
-	if (dss_has_feature(FEAT_MGR_LCD2))
-		PIS(SYNC_LOST2);
-	if (dss_has_feature(FEAT_MGR_LCD3))
-		PIS(SYNC_LOST3);
+#define PIS(x) (status & DISPC_IRQ_##x) ? (#x " ") : ""
+
+	pr_debug("DISPC IRQ: 0x%x: %s%s%s%s%s%s%s%s%s\n",
+		status,
+		PIS(OCP_ERR),
+		PIS(GFX_FIFO_UNDERFLOW),
+		PIS(VID1_FIFO_UNDERFLOW),
+		PIS(VID2_FIFO_UNDERFLOW),
+		dss_feat_get_num_ovls() > 3 ? PIS(VID3_FIFO_UNDERFLOW) : "",
+		PIS(SYNC_LOST),
+		PIS(SYNC_LOST_DIGIT),
+		dss_has_feature(FEAT_MGR_LCD2) ? PIS(SYNC_LOST2) : "",
+		dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "");
 #undef PIS
-
-	printk("\n");
 }
-#endif
 
 /* Called from dss.c. Note that we don't touch clocks here,
  * but we presume they are on because we got an IRQ. However,
@@ -3729,10 +3721,7 @@ static irqreturn_t omap_dispc_irq_handler(int irq, void *arg)
 	spin_unlock(&dispc.irq_stats_lock);
 #endif
 
-#ifdef DEBUG
-	if (dss_debug)
-		print_irq_status(irqstatus);
-#endif
+	print_irq_status(irqstatus);
 	/* Ack the interrupt. Do it here before clocks are possibly turned
 	 * off */
 	dispc_write_reg(DISPC_IRQSTATUS, irqstatus);
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 3b524cd..70be86c 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1107,28 +1107,16 @@ static inline void dsi_enable_pll_clock(struct platform_device *dsidev,
 	}
 }
 
-#ifdef DEBUG
 static void _dsi_print_reset_status(struct platform_device *dsidev)
 {
 	u32 l;
 	int b0, b1, b2;
 
-	if (!dss_debug)
-		return;
-
 	/* A dummy read using the SCP interface to any DSIPHY register is
 	 * required after DSIPHY reset to complete the reset of the DSI complex
 	 * I/O. */
 	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
 
-	printk(KERN_DEBUG "DSI resets: ");
-
-	l = dsi_read_reg(dsidev, DSI_PLL_STATUS);
-	printk("PLL (%d) ", FLD_GET(l, 0, 0));
-
-	l = dsi_read_reg(dsidev, DSI_COMPLEXIO_CFG1);
-	printk("CIO (%d) ", FLD_GET(l, 29, 29));
-
 	if (dss_has_feature(FEAT_DSI_REVERSE_TXCLKESC)) {
 		b0 = 28;
 		b1 = 27;
@@ -1139,18 +1127,21 @@ static void _dsi_print_reset_status(struct platform_device *dsidev)
 		b2 = 26;
 	}
 
-	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
-	printk("PHY (%x%x%x, %d, %d, %d)\n",
-			FLD_GET(l, b0, b0),
-			FLD_GET(l, b1, b1),
-			FLD_GET(l, b2, b2),
-			FLD_GET(l, 29, 29),
-			FLD_GET(l, 30, 30),
-			FLD_GET(l, 31, 31));
+#define DSI_FLD_GET(fld, start, end)\
+	FLD_GET(dsi_read_reg(dsidev, DSI_##fld), start, end)
+
+	pr_debug("DSI resets: PLL (%d) CIO (%d) 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, b0, b0),
+		DSI_FLD_GET(DSIPHY_CFG5, b1, b1),
+		DSI_FLD_GET(DSIPHY_CFG5, b2, b2),
+		DSI_FLD_GET(DSIPHY_CFG5, 29, 29),
+		DSI_FLD_GET(DSIPHY_CFG5, 30, 30),
+		DSI_FLD_GET(DSIPHY_CFG5, 31, 31));
+
+#undef DSI_FLD_GET
 }
-#else
-#define _dsi_print_reset_status(x)
-#endif
 
 static inline int dsi_if_enable(struct platform_device *dsidev, bool enable)
 {
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 5ef4e17..1d0205c 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -23,10 +23,6 @@
 #ifndef __OMAP2_DSS_H
 #define __OMAP2_DSS_H
 
-#ifdef DEBUG
-extern bool dss_debug;
-#endif
-
 #ifdef pr_fmt
 #undef pr_fmt
 #endif
-- 
1.7.10

Re: [PATCH V3 0/3] OMAPDSS: Enable dynamic debug printing

[Tomi Valkeinen]

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

On Fri, 2012-09-28 at 15:53 +0530, Chandrabhanu Mahapatra wrote:
> Hi everyone,
> this patch series aims at cleaning up of DSS of printk()'s enabled with
> dss_debug and replace them with generic dynamic debug printing.
> 
> The 1st patch
>  * moved DEBUG flag definition to Makefile
> The 2nd patch 
>  * replaces printk() in DSSDBG definition with pr_debug()
>  * removes DSSDBGF definition and replaces its instances with DSSDBG() 
> The 3rd patch
>  * cleans up printk()'s in omap_dispc_unregister_isr() and 
>    _dsi_print_reset_status() with pr_debug()
>  * removes dss_debug variable
> 
> Changes from V1 to V2:
>  * added debug messages to DSSDBG calls
>  * added patch "OMAPDSS: Remove dss_debug variable"
> 
> Changes from V2 to V3
>  * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile"
> 
> All your comments and suggestions are welcome.

There's one thing that's not quite nice about omapdss's debug print
behavior after this series.

CONFIG_OMAP2_DSS_DEBUG_SUPPORT is marked "default y", and it's also been
safe to enable earlier as we had the dss_debug variable to prevent the
debug prints. But after this series, the debug prints are enabled, and
will spam the kernel log quite heavily.

And that happens with both dynamic debugging enabled and disabled.

How things should work:

For kernels with dynamic debugging disabled: by default the dss debugs
are not compiled, and the user needs to explicitly enable them in the
kernel config.

For kernels with dynamic debugging enabled: by default the dss debugs
are compiled in, but not enabled. A Kconfig option can be set to make
the debugs enabled by default.

In addition to those, we have the debugfs files. Those should be usable
regardless of the debug prints.

So I suggest the following:

- Remove CONFIG_OMAP2_DSS_DEBUG_SUPPORT. We can't re-use it, because it
may be enabled in user's kernel configs.
- Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUG. This will set DEBUG in
the makefile. This is off by default.
- Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUGFS. This will be use to
decide if debugfs functionality is compiled in or not. This is off by
default.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V3 0/3] OMAPDSS: Enable dynamic debug printing

[Mahapatra, Chandrabhanu]

On Fri, Sep 28, 2012 at 5:04 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Fri, 2012-09-28 at 15:53 +0530, Chandrabhanu Mahapatra wrote:
>> Hi everyone,
>> this patch series aims at cleaning up of DSS of printk()'s enabled with
>> dss_debug and replace them with generic dynamic debug printing.
>>
>> The 1st patch
>>  * moved DEBUG flag definition to Makefile
>> The 2nd patch
>>  * replaces printk() in DSSDBG definition with pr_debug()
>>  * removes DSSDBGF definition and replaces its instances with DSSDBG()
>> The 3rd patch
>>  * cleans up printk()'s in omap_dispc_unregister_isr() and
>>    _dsi_print_reset_status() with pr_debug()
>>  * removes dss_debug variable
>>
>> Changes from V1 to V2:
>>  * added debug messages to DSSDBG calls
>>  * added patch "OMAPDSS: Remove dss_debug variable"
>>
>> Changes from V2 to V3
>>  * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile"
>>
>> All your comments and suggestions are welcome.
>
> There's one thing that's not quite nice about omapdss's debug print
> behavior after this series.
>
> CONFIG_OMAP2_DSS_DEBUG_SUPPORT is marked "default y", and it's also been
> safe to enable earlier as we had the dss_debug variable to prevent the
> debug prints. But after this series, the debug prints are enabled, and
> will spam the kernel log quite heavily.
>
> And that happens with both dynamic debugging enabled and disabled.

Yes, I had noticed that but I thought a better way to disable debug
prints is to disable both dynamic debugging (CONFIG_DYNAMIC_DEBUG) and
CONFIG_OMAP2_DSS_DEBUG_SUPPORT. May be CONFIG_OMAP2_DSS_DEBUG_SUPPORT
should have been false by default.

> How things should work:
>
> For kernels with dynamic debugging disabled: by default the dss debugs
> are not compiled, and the user needs to explicitly enable them in the
> kernel config.
>
> For kernels with dynamic debugging enabled: by default the dss debugs
> are compiled in, but not enabled. A Kconfig option can be set to make
> the debugs enabled by default.
>
> In addition to those, we have the debugfs files. Those should be usable
> regardless of the debug prints.
>
> So I suggest the following:
>
> - Remove CONFIG_OMAP2_DSS_DEBUG_SUPPORT. We can't re-use it, because it
> may be enabled in user's kernel configs.
> - Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUG. This will set DEBUG in
> the makefile. This is off by default.
> - Add new Kconfig option: CONFIG_OMAP2_DSS_DEBUGFS. This will be use to
> decide if debugfs functionality is compiled in or not. This is off by
> default.
>
>  Tomi
>

Well, I had a different perception. If one needs to debug then both
debugfs and debug prints should be enabled. If one needs only debug
prints then CONFIG_DEBUG_FS can be disabled.
But above approach seems to provide more flexibilty.

-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.

[PATCH V4 0/5] OMAPDSS: Enable dynamic debug printing

[Chandrabhanu Mahapatra]

Hi everyone,
this patch series aims at cleaning up of DSS of printk()'s enabled with
dss_debug and replace them with generic dynamic debug printing.

The 1st patch
 * moved DEBUG flag definition to Makefile
The 2nd patch
 * created two debug config options OMAP2_DSS_DEBUG and OMAP2_DSS_DEBUGFS
The 3rd patch
 * replaces printk() in DSSDBG definition with pr_debug()
 * removes DSSDBGF definition and replaces its instances with DSSDBG() 
The 4th patch
 * cleans up printk()'s in omap_dispc_unregister_isr() and 
   _dsi_print_reset_status() with pr_debug()
The 5th patch
 * removes dss_debug variable

Changes from V1 to V2:
 * added debug messages to DSSDBG calls
 * added patch "OMAPDSS: Remove dss_debug variable"

Changes from V2 to V3
 * added patch "OMAPDSS: Move definition of DEBUG flag to Makefile"

Changes from V3 to V4:
 * added patch "OMAPDSS: Create new debug config options"
 * broke earlier patch "OMAPDSS: Remove dss_debug variable" into two parts as
   "OMAPDSS: Replace multi part debug prints with pr_debug" and
   "OMAPDSS: Remove dss_debug variable"

All your comments and suggestions are welcome.

Refenence Tree:
git://gitorious.org/linux-omap-dss2/chandrabhanus-linux.git dss_cleanup

Regards,
Chandrabhanu

Chandrabhanu Mahapatra (5):
  OMAPDSS: Move definition of DEBUG flag to Makefile
  OMAPDSS: Create new debug config options
  OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function
  OMAPDSS: Replace multi part debug prints with pr_debug
  OMAPDSS: Remove dss_debug variable

 drivers/video/omap2/dss/Kconfig  |   21 +++++++++++-----
 drivers/video/omap2/dss/Makefile |    1 +
 drivers/video/omap2/dss/apply.c  |    8 +++----
 drivers/video/omap2/dss/core.c   |   11 +++------
 drivers/video/omap2/dss/dispc.c  |   40 ++++++++++++-------------------
 drivers/video/omap2/dss/dsi.c    |   49 ++++++++++++++++----------------------
 drivers/video/omap2/dss/dss.c    |    2 +-
 drivers/video/omap2/dss/dss.h    |   40 ++++++-------------------------
 8 files changed, 66 insertions(+), 106 deletions(-)

-- 
1.7.10

[PATCH V4 1/5] OMAPDSS: Move definition of DEBUG flag to Makefile

[Chandrabhanu Mahapatra]

In OMAPDSS the DEBUG flag is set only after the OMAPDSS module is called, for
which the debugging capabilities are available only after its proper
initialization. As a result of which tracking of bugs prior to or during initial
process becomes difficult. So, the definition of DEBUG is being moved to the
corresponding Makefile.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/Makefile |    1 +
 drivers/video/omap2/dss/dss.h    |    4 ----
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/Makefile b/drivers/video/omap2/dss/Makefile
index 4549869..86493e3 100644
--- a/drivers/video/omap2/dss/Makefile
+++ b/drivers/video/omap2/dss/Makefile
@@ -8,3 +8,4 @@ omapdss-$(CONFIG_OMAP2_DSS_SDI) += sdi.o
 omapdss-$(CONFIG_OMAP2_DSS_DSI) += dsi.o
 omapdss-$(CONFIG_OMAP4_DSS_HDMI) += hdmi.o \
 				    hdmi_panel.o ti_hdmi_4xxx_ip.o
+ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DDEBUG
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 6728892..ffbba7e 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -23,10 +23,6 @@
 #ifndef __OMAP2_DSS_H
 #define __OMAP2_DSS_H
 
-#ifdef CONFIG_OMAP2_DSS_DEBUG_SUPPORT
-#define DEBUG
-#endif
-
 #ifdef DEBUG
 extern bool dss_debug;
 #ifdef DSS_SUBSYS_NAME
-- 
1.7.10

[PATCH V4 2/5] OMAPDSS: Create new debug config options

[Chandrabhanu Mahapatra]

The config option CONFIG_OMAP2_DSS_DEBUG_SUPPORT has been removed and replaced
with CONFIG_OMAP2_DSS_DEBUG and CONFIG_OMAP2_DSS_DEBUGFS. CONFIG_OMAP2_DSS_DEBUG
enables DEBUG flag and CONFIG_OMAP2_DSS_DEBUGFS enables creation of debugfs for
OMAPDSS. Both the config options are disabled by default and can be enabled
independently of one another as per convenience.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/Kconfig  |   21 +++++++++++++++------
 drivers/video/omap2/dss/Makefile |    2 +-
 drivers/video/omap2/dss/core.c   |    6 +++---
 drivers/video/omap2/dss/dss.c    |    2 +-
 drivers/video/omap2/dss/dss.h    |    2 +-
 5 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/video/omap2/dss/Kconfig b/drivers/video/omap2/dss/Kconfig
index 80f5390..866a563 100644
--- a/drivers/video/omap2/dss/Kconfig
+++ b/drivers/video/omap2/dss/Kconfig
@@ -18,16 +18,25 @@ config OMAP2_VRAM_SIZE
 	  You can also set this with "vram=<bytes>" kernel argument, or
 	  in the board file.
 
-config OMAP2_DSS_DEBUG_SUPPORT
-        bool "Debug support"
-	default y
+config OMAP2_DSS_DEBUG
+	bool "Debug support"
+	default n
+	help
+	  This enables printing of debug messages. Alternatively, debug messages
+	  can also be enabled by setting CONFIG_DYNAMIC_DEBUG and then setting
+	  appropriate flags in <debugfs>/dynamic_debug/control.
+
+config OMAP2_DSS_DEBUGFS
+	bool "Debugfs filesystem support"
+	default n
 	help
-	  This enables debug messages. You need to enable printing
-	  with 'debug' module parameter.
+	  This enables debugfs for OMAPDSS at <debugfs>/omapdss. This enables
+	  querying about clock configuration and register configuration of dss,
+	  dispc, dsi, hdmi and rfbi.
 
 config OMAP2_DSS_COLLECT_IRQ_STATS
 	bool "Collect DSS IRQ statistics"
-	depends on OMAP2_DSS_DEBUG_SUPPORT
+	depends on OMAP2_DSS_DEBUGFS
 	default n
 	help
 	  Collect DSS IRQ statistics, printable via debugfs.
diff --git a/drivers/video/omap2/dss/Makefile b/drivers/video/omap2/dss/Makefile
index 86493e3..4070191 100644
--- a/drivers/video/omap2/dss/Makefile
+++ b/drivers/video/omap2/dss/Makefile
@@ -8,4 +8,4 @@ omapdss-$(CONFIG_OMAP2_DSS_SDI) += sdi.o
 omapdss-$(CONFIG_OMAP2_DSS_DSI) += dsi.o
 omapdss-$(CONFIG_OMAP4_DSS_HDMI) += hdmi.o \
 				    hdmi_panel.o ti_hdmi_4xxx_ip.o
-ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DDEBUG
+ccflags-$(CONFIG_OMAP2_DSS_DEBUG) += -DDEBUG
diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index b2af72d..1e1f50f 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -138,7 +138,7 @@ int dss_set_min_bus_tput(struct device *dev, unsigned long tput)
 		return 0;
 }
 
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUGFS)
 static int dss_debug_show(struct seq_file *s, void *unused)
 {
 	void (*func)(struct seq_file *) = s->private;
@@ -193,7 +193,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
 
 	return 0;
 }
-#else /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
+#else /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUGFS */
 static inline int dss_initialize_debugfs(void)
 {
 	return 0;
@@ -205,7 +205,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
 {
 	return 0;
 }
-#endif /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
+#endif /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUGFS */
 
 /* PLATFORM DEVICE */
 static int omap_dss_pm_notif(struct notifier_block *b, unsigned long v, void *d)
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 2ab1c3e..fe30855 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -746,7 +746,7 @@ static void dss_runtime_put(void)
 }
 
 /* DEBUGFS */
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUGFS)
 void dss_debug_dump_clocks(struct seq_file *s)
 {
 	dss_dump_clocks(s);
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index ffbba7e..a8a1ab4 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -301,7 +301,7 @@ enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void);
 const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src);
 void dss_dump_clocks(struct seq_file *s);
 
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUGFS)
 void dss_debug_dump_clocks(struct seq_file *s);
 #endif
 
-- 
1.7.10

Re: [PATCH V4 2/5] OMAPDSS: Create new debug config options

[Sumit Semwal]

Hi Chandrabhanu,

On Saturday 29 September 2012 04:19 PM, Chandrabhanu Mahapatra wrote:
> The config option CONFIG_OMAP2_DSS_DEBUG_SUPPORT has been removed and replaced
> with CONFIG_OMAP2_DSS_DEBUG and CONFIG_OMAP2_DSS_DEBUGFS. CONFIG_OMAP2_DSS_DEBUG
> enables DEBUG flag and CONFIG_OMAP2_DSS_DEBUGFS enables creation of debugfs for
> OMAPDSS. Both the config options are disabled by default and can be enabled
> independently of one another as per convenience.
>
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
>   drivers/video/omap2/dss/Kconfig  |   21 +++++++++++++++------
>   drivers/video/omap2/dss/Makefile |    2 +-
>   drivers/video/omap2/dss/core.c   |    6 +++---
>   drivers/video/omap2/dss/dss.c    |    2 +-
>   drivers/video/omap2/dss/dss.h    |    2 +-
>   5 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/Kconfig b/drivers/video/omap2/dss/Kconfig
> index 80f5390..866a563 100644
> --- a/drivers/video/omap2/dss/Kconfig
> +++ b/drivers/video/omap2/dss/Kconfig
> @@ -18,16 +18,25 @@ config OMAP2_VRAM_SIZE
>   	  You can also set this with "vram=<bytes>" kernel argument, or
>   	  in the board file.
>
> -config OMAP2_DSS_DEBUG_SUPPORT
> -        bool "Debug support"
> -	default y
> +config OMAP2_DSS_DEBUG
> +	bool "Debug support"
> +	default n
> +	help
> +	  This enables printing of debug messages. Alternatively, debug messages
> +	  can also be enabled by setting CONFIG_DYNAMIC_DEBUG and then setting
> +	  appropriate flags in <debugfs>/dynamic_debug/control.
> +
> +config OMAP2_DSS_DEBUGFS
> +	bool "Debugfs filesystem support"
> +	default n
You can make it as 'depends on CONFIG_DEBUG_FS', so that your check 
below [1] becomes cleaner.
>   	help
> -	  This enables debug messages. You need to enable printing
> -	  with 'debug' module parameter.
> +	  This enables debugfs for OMAPDSS at <debugfs>/omapdss. This enables
> +	  querying about clock configuration and register configuration of dss,
> +	  dispc, dsi, hdmi and rfbi.
>
>   config OMAP2_DSS_COLLECT_IRQ_STATS
>   	bool "Collect DSS IRQ statistics"
> -	depends on OMAP2_DSS_DEBUG_SUPPORT
> +	depends on OMAP2_DSS_DEBUGFS
>   	default n
>   	help
>   	  Collect DSS IRQ statistics, printable via debugfs.
> diff --git a/drivers/video/omap2/dss/Makefile b/drivers/video/omap2/dss/Makefile
> index 86493e3..4070191 100644
> --- a/drivers/video/omap2/dss/Makefile
> +++ b/drivers/video/omap2/dss/Makefile
> @@ -8,4 +8,4 @@ omapdss-$(CONFIG_OMAP2_DSS_SDI) += sdi.o
>   omapdss-$(CONFIG_OMAP2_DSS_DSI) += dsi.o
>   omapdss-$(CONFIG_OMAP4_DSS_HDMI) += hdmi.o \
>   				    hdmi_panel.o ti_hdmi_4xxx_ip.o
> -ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DDEBUG
> +ccflags-$(CONFIG_OMAP2_DSS_DEBUG) += -DDEBUG
> diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
> index b2af72d..1e1f50f 100644
> --- a/drivers/video/omap2/dss/core.c
> +++ b/drivers/video/omap2/dss/core.c
> @@ -138,7 +138,7 @@ int dss_set_min_bus_tput(struct device *dev, unsigned long tput)
>   		return 0;
>   }
>
> -#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUGFS)
[1]: here.
>   static int dss_debug_show(struct seq_file *s, void *unused)
>   {
>   	void (*func)(struct seq_file *) = s->private;
> @@ -193,7 +193,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
>
>   	return 0;
>   }
> -#else /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
> +#else /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUGFS */
>   static inline int dss_initialize_debugfs(void)
>   {
>   	return 0;
> @@ -205,7 +205,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
>   {
>   	return 0;
>   }
> -#endif /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
> +#endif /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUGFS */
>
>   /* PLATFORM DEVICE */
>   static int omap_dss_pm_notif(struct notifier_block *b, unsigned long v, void *d)
> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> index 2ab1c3e..fe30855 100644
> --- a/drivers/video/omap2/dss/dss.c
> +++ b/drivers/video/omap2/dss/dss.c
> @@ -746,7 +746,7 @@ static void dss_runtime_put(void)
>   }
>
>   /* DEBUGFS */
> -#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUGFS)
>   void dss_debug_dump_clocks(struct seq_file *s)
>   {
>   	dss_dump_clocks(s);
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index ffbba7e..a8a1ab4 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -301,7 +301,7 @@ enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void);
>   const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src);
>   void dss_dump_clocks(struct seq_file *s);
>
> -#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUGFS)
>   void dss_debug_dump_clocks(struct seq_file *s);
>   #endif
>
>

[PATCH V5 2/5] OMAPDSS: Create new debug config options

[Chandrabhanu Mahapatra]

The config option CONFIG_OMAP2_DSS_DEBUG_SUPPORT has been removed and replaced
with CONFIG_OMAP2_DSS_DEBUG and CONFIG_OMAP2_DSS_DEBUGFS. CONFIG_OMAP2_DSS_DEBUG
enables DEBUG flag and CONFIG_OMAP2_DSS_DEBUGFS enables creation of debugfs for
OMAPDSS. Both the config options are disabled by default and can be enabled
independently of one another as per convenience.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
changes from V4 to V5
 * added "depends on DEBUG_FS" to OMAP2_DSS_DEBUGFS defintion in Kconfig so that
   CONFIG_OMAP2_DSS_DEBUGFS check become cleaner

 drivers/video/omap2/dss/Kconfig  |   22 ++++++++++++++++------
 drivers/video/omap2/dss/Makefile |    2 +-
 drivers/video/omap2/dss/core.c   |    6 +++---
 drivers/video/omap2/dss/dss.c    |    2 +-
 drivers/video/omap2/dss/dss.h    |    2 +-
 5 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/video/omap2/dss/Kconfig b/drivers/video/omap2/dss/Kconfig
index 80f5390..7052487 100644
--- a/drivers/video/omap2/dss/Kconfig
+++ b/drivers/video/omap2/dss/Kconfig
@@ -18,16 +18,26 @@ config OMAP2_VRAM_SIZE
 	  You can also set this with "vram=<bytes>" kernel argument, or
 	  in the board file.
 
-config OMAP2_DSS_DEBUG_SUPPORT
-        bool "Debug support"
-	default y
+config OMAP2_DSS_DEBUG
+	bool "Debug support"
+	default n
+	help
+	  This enables printing of debug messages. Alternatively, debug messages
+	  can also be enabled by setting CONFIG_DYNAMIC_DEBUG and then setting
+	  appropriate flags in <debugfs>/dynamic_debug/control.
+
+config OMAP2_DSS_DEBUGFS
+	bool "Debugfs filesystem support"
+	depends on DEBUG_FS
+	default n
 	help
-	  This enables debug messages. You need to enable printing
-	  with 'debug' module parameter.
+	  This enables debugfs for OMAPDSS at <debugfs>/omapdss. This enables
+	  querying about clock configuration and register configuration of dss,
+	  dispc, dsi, hdmi and rfbi.
 
 config OMAP2_DSS_COLLECT_IRQ_STATS
 	bool "Collect DSS IRQ statistics"
-	depends on OMAP2_DSS_DEBUG_SUPPORT
+	depends on OMAP2_DSS_DEBUGFS
 	default n
 	help
 	  Collect DSS IRQ statistics, printable via debugfs.
diff --git a/drivers/video/omap2/dss/Makefile b/drivers/video/omap2/dss/Makefile
index 86493e3..4070191 100644
--- a/drivers/video/omap2/dss/Makefile
+++ b/drivers/video/omap2/dss/Makefile
@@ -8,4 +8,4 @@ omapdss-$(CONFIG_OMAP2_DSS_SDI) += sdi.o
 omapdss-$(CONFIG_OMAP2_DSS_DSI) += dsi.o
 omapdss-$(CONFIG_OMAP4_DSS_HDMI) += hdmi.o \
 				    hdmi_panel.o ti_hdmi_4xxx_ip.o
-ccflags-$(CONFIG_OMAP2_DSS_DEBUG_SUPPORT) += -DDEBUG
+ccflags-$(CONFIG_OMAP2_DSS_DEBUG) += -DDEBUG
diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index b2af72d..826d64f 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -138,7 +138,7 @@ int dss_set_min_bus_tput(struct device *dev, unsigned long tput)
 		return 0;
 }
 
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
+#if defined(CONFIG_OMAP2_DSS_DEBUGFS)
 static int dss_debug_show(struct seq_file *s, void *unused)
 {
 	void (*func)(struct seq_file *) = s->private;
@@ -193,7 +193,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
 
 	return 0;
 }
-#else /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
+#else /* CONFIG_OMAP2_DSS_DEBUGFS */
 static inline int dss_initialize_debugfs(void)
 {
 	return 0;
@@ -205,7 +205,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
 {
 	return 0;
 }
-#endif /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
+#endif /* CONFIG_OMAP2_DSS_DEBUGFS */
 
 /* PLATFORM DEVICE */
 static int omap_dss_pm_notif(struct notifier_block *b, unsigned long v, void *d)
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 2ab1c3e..3954742 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -746,7 +746,7 @@ static void dss_runtime_put(void)
 }
 
 /* DEBUGFS */
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
+#if defined(CONFIG_OMAP2_DSS_DEBUGFS)
 void dss_debug_dump_clocks(struct seq_file *s)
 {
 	dss_dump_clocks(s);
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index ffbba7e..685275e 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -301,7 +301,7 @@ enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void);
 const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src);
 void dss_dump_clocks(struct seq_file *s);
 
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
+#if defined(CONFIG_OMAP2_DSS_DEBUGFS)
 void dss_debug_dump_clocks(struct seq_file *s);
 #endif
 
-- 
1.7.10

[PATCH V4 3/5] OMAPDSS: Cleanup DSSDBG with dynamic pr_debug function

[Chandrabhanu Mahapatra]

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 <cmahapatra@ti.com>
---
 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(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 19d66f4..e923d9f 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -573,7 +573,7 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
 	struct mgr_priv_data *mp;
 	int r;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("writing ovl %d regs", ovl->id);
 
 	if (!op->enabled || !op->info_dirty)
 		return;
@@ -608,7 +608,7 @@ static void dss_ovl_write_regs_extra(struct omap_overlay *ovl)
 	struct ovl_priv_data *op = get_ovl_priv(ovl);
 	struct mgr_priv_data *mp;
 
-	DSSDBGF("%d", ovl->id);
+	DSSDBG("writing ovl %d regs extra", ovl->id);
 
 	if (!op->extra_info_dirty)
 		return;
@@ -632,7 +632,7 @@ static void dss_mgr_write_regs(struct omap_overlay_manager *mgr)
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 	struct omap_overlay *ovl;
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("writing mgr %d regs", mgr->id);
 
 	if (!mp->enabled)
 		return;
@@ -658,7 +658,7 @@ static void dss_mgr_write_regs_extra(struct omap_overlay_manager *mgr)
 {
 	struct mgr_priv_data *mp = get_mgr_priv(mgr);
 
-	DSSDBGF("%d", mgr->id);
+	DSSDBG("writing mgr %d regs extra", mgr->id);
 
 	if (!mp->extra_info_dirty)
 		return;
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index e37e6d8..b0345f3 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1612,7 +1612,7 @@ int dsi_pll_set_clock_div(struct platform_device *dsidev,
 	u8 regn_start, regn_end, regm_start, regm_end;
 	u8 regm_dispc_start, regm_dispc_end, regm_dsi_start, regm_dsi_end;
 
-	DSSDBGF();
+	DSSDBG("DSI PLL clock config starts");
 
 	dsi->current_cinfo.clkin = cinfo->clkin;
 	dsi->current_cinfo.fint = cinfo->fint;
@@ -2431,7 +2431,7 @@ static int dsi_cio_init(struct platform_device *dsidev)
 	int r;
 	u32 l;
 
-	DSSDBGF();
+	DSSDBG("DSI CIO init starts");
 
 	r = dss_dsi_enable_pads(dsi->module_id, dsi_get_lane_mask(dsidev));
 	if (r)
@@ -2782,7 +2782,7 @@ static void dsi_vc_initial_config(struct platform_device *dsidev, int channel)
 {
 	u32 r;
 
-	DSSDBGF("%d", channel);
+	DSSDBG("Initial config of virtual channel %d", channel);
 
 	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);
 
 	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 a8a1ab4..8bef7ee 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
 
+#define DSSDBG(format, ...) \
+	pr_debug(format, ## __VA_ARGS__)
 
 #ifdef DSS_SUBSYS_NAME
 #define DSSERR(format, ...) \
-- 
1.7.10

[PATCH V4 4/5] OMAPDSS: Replace multi part debug prints with pr_debug

[Chandrabhanu Mahapatra]

The omap_dispc_unregister_isr() and _dsi_print_reset_status() consist of a
number of debug prints which need to be enabled all at once or none at all. So,
these debug prints in corresponding functions are replaced with one dynamic
debug enabled pr_debug() each.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/dispc.c |   32 +++++++++++++-------------------
 drivers/video/omap2/dss/dsi.c   |   30 ++++++++++++++----------------
 2 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index a173a94..67d9f3b 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3675,26 +3675,20 @@ static void print_irq_status(u32 status)
 	if ((status & dispc.irq_error_mask) = 0)
 		return;
 
-	printk(KERN_DEBUG "DISPC IRQ: 0x%x: ", status);
-
-#define PIS(x) \
-	if (status & DISPC_IRQ_##x) \
-		printk(#x " ");
-	PIS(GFX_FIFO_UNDERFLOW);
-	PIS(OCP_ERR);
-	PIS(VID1_FIFO_UNDERFLOW);
-	PIS(VID2_FIFO_UNDERFLOW);
-	if (dss_feat_get_num_ovls() > 3)
-		PIS(VID3_FIFO_UNDERFLOW);
-	PIS(SYNC_LOST);
-	PIS(SYNC_LOST_DIGIT);
-	if (dss_has_feature(FEAT_MGR_LCD2))
-		PIS(SYNC_LOST2);
-	if (dss_has_feature(FEAT_MGR_LCD3))
-		PIS(SYNC_LOST3);
+#define PIS(x) (status & DISPC_IRQ_##x) ? (#x " ") : ""
+
+	pr_debug("DISPC IRQ: 0x%x: %s%s%s%s%s%s%s%s%s\n",
+		status,
+		PIS(OCP_ERR),
+		PIS(GFX_FIFO_UNDERFLOW),
+		PIS(VID1_FIFO_UNDERFLOW),
+		PIS(VID2_FIFO_UNDERFLOW),
+		dss_feat_get_num_ovls() > 3 ? PIS(VID3_FIFO_UNDERFLOW) : "",
+		PIS(SYNC_LOST),
+		PIS(SYNC_LOST_DIGIT),
+		dss_has_feature(FEAT_MGR_LCD2) ? PIS(SYNC_LOST2) : "",
+		dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "");
 #undef PIS
-
-	printk("\n");
 }
 #endif
 
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index b0345f3..6dd073a 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1121,14 +1121,6 @@ static void _dsi_print_reset_status(struct platform_device *dsidev)
 	 * I/O. */
 	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
 
-	printk(KERN_DEBUG "DSI resets: ");
-
-	l = dsi_read_reg(dsidev, DSI_PLL_STATUS);
-	printk("PLL (%d) ", FLD_GET(l, 0, 0));
-
-	l = dsi_read_reg(dsidev, DSI_COMPLEXIO_CFG1);
-	printk("CIO (%d) ", FLD_GET(l, 29, 29));
-
 	if (dss_has_feature(FEAT_DSI_REVERSE_TXCLKESC)) {
 		b0 = 28;
 		b1 = 27;
@@ -1139,14 +1131,20 @@ static void _dsi_print_reset_status(struct platform_device *dsidev)
 		b2 = 26;
 	}
 
-	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
-	printk("PHY (%x%x%x, %d, %d, %d)\n",
-			FLD_GET(l, b0, b0),
-			FLD_GET(l, b1, b1),
-			FLD_GET(l, b2, b2),
-			FLD_GET(l, 29, 29),
-			FLD_GET(l, 30, 30),
-			FLD_GET(l, 31, 31));
+#define DSI_FLD_GET(fld, start, end)\
+	FLD_GET(dsi_read_reg(dsidev, DSI_##fld), start, end)
+
+	pr_debug("DSI resets: PLL (%d) CIO (%d) 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, b0, b0),
+		DSI_FLD_GET(DSIPHY_CFG5, b1, b1),
+		DSI_FLD_GET(DSIPHY_CFG5, b2, b2),
+		DSI_FLD_GET(DSIPHY_CFG5, 29, 29),
+		DSI_FLD_GET(DSIPHY_CFG5, 30, 30),
+		DSI_FLD_GET(DSIPHY_CFG5, 31, 31));
+
+#undef DSI_FLD_GET
 }
 #else
 #define _dsi_print_reset_status(x)
-- 
1.7.10

Re: [PATCH V4 4/5] OMAPDSS: Replace multi part debug prints with pr_debug

[Tomi Valkeinen]

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

On Sat, 2012-09-29 at 16:19 +0530, Chandrabhanu Mahapatra wrote:
> The omap_dispc_unregister_isr() and _dsi_print_reset_status() consist of a
> number of debug prints which need to be enabled all at once or none at all. So,
> these debug prints in corresponding functions are replaced with one dynamic
> debug enabled pr_debug() each.
> 
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
>  drivers/video/omap2/dss/dispc.c |   32 +++++++++++++-------------------
>  drivers/video/omap2/dss/dsi.c   |   30 ++++++++++++++----------------
>  2 files changed, 27 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index a173a94..67d9f3b 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -3675,26 +3675,20 @@ static void print_irq_status(u32 status)
>  	if ((status & dispc.irq_error_mask) == 0)
>  		return;
>  
> -	printk(KERN_DEBUG "DISPC IRQ: 0x%x: ", status);
> -
> -#define PIS(x) \
> -	if (status & DISPC_IRQ_##x) \
> -		printk(#x " ");
> -	PIS(GFX_FIFO_UNDERFLOW);
> -	PIS(OCP_ERR);
> -	PIS(VID1_FIFO_UNDERFLOW);
> -	PIS(VID2_FIFO_UNDERFLOW);
> -	if (dss_feat_get_num_ovls() > 3)
> -		PIS(VID3_FIFO_UNDERFLOW);
> -	PIS(SYNC_LOST);
> -	PIS(SYNC_LOST_DIGIT);
> -	if (dss_has_feature(FEAT_MGR_LCD2))
> -		PIS(SYNC_LOST2);
> -	if (dss_has_feature(FEAT_MGR_LCD3))
> -		PIS(SYNC_LOST3);
> +#define PIS(x) (status & DISPC_IRQ_##x) ? (#x " ") : ""
> +
> +	pr_debug("DISPC IRQ: 0x%x: %s%s%s%s%s%s%s%s%s\n",
> +		status,
> +		PIS(OCP_ERR),
> +		PIS(GFX_FIFO_UNDERFLOW),
> +		PIS(VID1_FIFO_UNDERFLOW),
> +		PIS(VID2_FIFO_UNDERFLOW),
> +		dss_feat_get_num_ovls() > 3 ? PIS(VID3_FIFO_UNDERFLOW) : "",
> +		PIS(SYNC_LOST),
> +		PIS(SYNC_LOST_DIGIT),
> +		dss_has_feature(FEAT_MGR_LCD2) ? PIS(SYNC_LOST2) : "",
> +		dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "");
>  #undef PIS
> -
> -	printk("\n");
>  }
>  #endif

There's similar irq printing code in dsi.c that should also be converted
to the above style.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH V5 4/5] OMAPDSS: Replace multi part debug prints with pr_debug

[Chandrabhanu Mahapatra]

The various functions in dispc and dsi such as print_irq_status(),
print_irq_status_vc(), print_irq_status_cio() and _dsi_print_reset_status()
consist of a number of debug prints which need to be enabled all at once or none
at all. So, these debug prints in corresponding functions are replaced with one
dynamic debug enabled pr_debug() each.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
Changes from V4 to V5:
 * replaced prints in dsi functions print_irq_status, print_irq_status_vc() and
   print_irq_status_cio() with single pr_debug()
 * replaced macro VERBOSE_IRQ with static variable verbose_irq

 drivers/video/omap2/dss/dispc.c |   32 +++-----
 drivers/video/omap2/dss/dsi.c   |  168 ++++++++++++++++++---------------------
 2 files changed, 90 insertions(+), 110 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index a173a94..67d9f3b 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3675,26 +3675,20 @@ static void print_irq_status(u32 status)
 	if ((status & dispc.irq_error_mask) = 0)
 		return;
 
-	printk(KERN_DEBUG "DISPC IRQ: 0x%x: ", status);
-
-#define PIS(x) \
-	if (status & DISPC_IRQ_##x) \
-		printk(#x " ");
-	PIS(GFX_FIFO_UNDERFLOW);
-	PIS(OCP_ERR);
-	PIS(VID1_FIFO_UNDERFLOW);
-	PIS(VID2_FIFO_UNDERFLOW);
-	if (dss_feat_get_num_ovls() > 3)
-		PIS(VID3_FIFO_UNDERFLOW);
-	PIS(SYNC_LOST);
-	PIS(SYNC_LOST_DIGIT);
-	if (dss_has_feature(FEAT_MGR_LCD2))
-		PIS(SYNC_LOST2);
-	if (dss_has_feature(FEAT_MGR_LCD3))
-		PIS(SYNC_LOST3);
+#define PIS(x) (status & DISPC_IRQ_##x) ? (#x " ") : ""
+
+	pr_debug("DISPC IRQ: 0x%x: %s%s%s%s%s%s%s%s%s\n",
+		status,
+		PIS(OCP_ERR),
+		PIS(GFX_FIFO_UNDERFLOW),
+		PIS(VID1_FIFO_UNDERFLOW),
+		PIS(VID2_FIFO_UNDERFLOW),
+		dss_feat_get_num_ovls() > 3 ? PIS(VID3_FIFO_UNDERFLOW) : "",
+		PIS(SYNC_LOST),
+		PIS(SYNC_LOST_DIGIT),
+		dss_has_feature(FEAT_MGR_LCD2) ? PIS(SYNC_LOST2) : "",
+		dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "");
 #undef PIS
-
-	printk("\n");
 }
 #endif
 
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index b0345f3..19daee9 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -45,7 +45,6 @@
 #include "dss.h"
 #include "dss_features.h"
 
-/*#define VERBOSE_IRQ*/
 #define DSI_CATCH_MISSING_TE
 
 struct dsi_reg { u16 idx; };
@@ -526,42 +525,38 @@ static inline void dsi_perf_show(struct platform_device *dsidev,
 }
 #endif
 
+static int verbose_irq;
+
 static void print_irq_status(u32 status)
 {
 	if (status = 0)
 		return;
 
-#ifndef VERBOSE_IRQ
-	if ((status & ~DSI_IRQ_CHANNEL_MASK) = 0)
+	if (!verbose_irq && (status & ~DSI_IRQ_CHANNEL_MASK) = 0)
 		return;
-#endif
-	printk(KERN_DEBUG "DSI IRQ: 0x%x: ", status);
 
-#define PIS(x) \
-	if (status & DSI_IRQ_##x) \
-		printk(#x " ");
-#ifdef VERBOSE_IRQ
-	PIS(VC0);
-	PIS(VC1);
-	PIS(VC2);
-	PIS(VC3);
-#endif
-	PIS(WAKEUP);
-	PIS(RESYNC);
-	PIS(PLL_LOCK);
-	PIS(PLL_UNLOCK);
-	PIS(PLL_RECALL);
-	PIS(COMPLEXIO_ERR);
-	PIS(HS_TX_TIMEOUT);
-	PIS(LP_RX_TIMEOUT);
-	PIS(TE_TRIGGER);
-	PIS(ACK_TRIGGER);
-	PIS(SYNC_LOST);
-	PIS(LDO_POWER_GOOD);
-	PIS(TA_TIMEOUT);
+#define PIS(x) (status & DSI_IRQ_##x) ? (#x " ") : ""
+
+	pr_debug("DSI IRQ: 0x%x: %s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
+		status,
+		verbose_irq ? PIS(VC0) : "",
+		verbose_irq ? PIS(VC1) : "",
+		verbose_irq ? PIS(VC2) : "",
+		verbose_irq ? PIS(VC3) : "",
+		PIS(WAKEUP),
+		PIS(RESYNC),
+		PIS(PLL_LOCK),
+		PIS(PLL_UNLOCK),
+		PIS(PLL_RECALL),
+		PIS(COMPLEXIO_ERR),
+		PIS(HS_TX_TIMEOUT),
+		PIS(LP_RX_TIMEOUT),
+		PIS(TE_TRIGGER),
+		PIS(ACK_TRIGGER),
+		PIS(SYNC_LOST),
+		PIS(LDO_POWER_GOOD),
+		PIS(TA_TIMEOUT));
 #undef PIS
-
-	printk("\n");
 }
 
 static void print_irq_status_vc(int channel, u32 status)
@@ -569,28 +564,24 @@ static void print_irq_status_vc(int channel, u32 status)
 	if (status = 0)
 		return;
 
-#ifndef VERBOSE_IRQ
-	if ((status & ~DSI_VC_IRQ_PACKET_SENT) = 0)
+	if (!verbose_irq && (status & ~DSI_VC_IRQ_PACKET_SENT) = 0)
 		return;
-#endif
-	printk(KERN_DEBUG "DSI VC(%d) IRQ 0x%x: ", channel, status);
 
-#define PIS(x) \
-	if (status & DSI_VC_IRQ_##x) \
-		printk(#x " ");
-	PIS(CS);
-	PIS(ECC_CORR);
-#ifdef VERBOSE_IRQ
-	PIS(PACKET_SENT);
-#endif
-	PIS(FIFO_TX_OVF);
-	PIS(FIFO_RX_OVF);
-	PIS(BTA);
-	PIS(ECC_NO_CORR);
-	PIS(FIFO_TX_UDF);
-	PIS(PP_BUSY_CHANGE);
+#define PIS(x) (status & DSI_VC_IRQ_##x) ? (#x " ") : ""
+
+	pr_debug("DSI VC(%d) IRQ 0x%x: %s%s%s%s%s%s%s%s%s\n",
+		channel,
+		status,
+		PIS(CS),
+		PIS(ECC_CORR),
+		PIS(ECC_NO_CORR),
+		verbose_irq ? PIS(PACKET_SENT) : "",
+		PIS(BTA),
+		PIS(FIFO_TX_OVF),
+		PIS(FIFO_RX_OVF),
+		PIS(FIFO_TX_UDF),
+		PIS(PP_BUSY_CHANGE));
 #undef PIS
-	printk("\n");
 }
 
 static void print_irq_status_cio(u32 status)
@@ -598,34 +589,31 @@ static void print_irq_status_cio(u32 status)
 	if (status = 0)
 		return;
 
-	printk(KERN_DEBUG "DSI CIO IRQ 0x%x: ", status);
-
-#define PIS(x) \
-	if (status & DSI_CIO_IRQ_##x) \
-		printk(#x " ");
-	PIS(ERRSYNCESC1);
-	PIS(ERRSYNCESC2);
-	PIS(ERRSYNCESC3);
-	PIS(ERRESC1);
-	PIS(ERRESC2);
-	PIS(ERRESC3);
-	PIS(ERRCONTROL1);
-	PIS(ERRCONTROL2);
-	PIS(ERRCONTROL3);
-	PIS(STATEULPS1);
-	PIS(STATEULPS2);
-	PIS(STATEULPS3);
-	PIS(ERRCONTENTIONLP0_1);
-	PIS(ERRCONTENTIONLP1_1);
-	PIS(ERRCONTENTIONLP0_2);
-	PIS(ERRCONTENTIONLP1_2);
-	PIS(ERRCONTENTIONLP0_3);
-	PIS(ERRCONTENTIONLP1_3);
-	PIS(ULPSACTIVENOT_ALL0);
-	PIS(ULPSACTIVENOT_ALL1);
+#define PIS(x) (status & DSI_CIO_IRQ_##x) ? (#x " ") : ""
+
+	pr_debug("DSI CIO IRQ 0x%x: %s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
+		status,
+		PIS(ERRSYNCESC1),
+		PIS(ERRSYNCESC2),
+		PIS(ERRSYNCESC3),
+		PIS(ERRESC1),
+		PIS(ERRESC2),
+		PIS(ERRESC3),
+		PIS(ERRCONTROL1),
+		PIS(ERRCONTROL2),
+		PIS(ERRCONTROL3),
+		PIS(STATEULPS1),
+		PIS(STATEULPS2),
+		PIS(STATEULPS3),
+		PIS(ERRCONTENTIONLP0_1),
+		PIS(ERRCONTENTIONLP1_1),
+		PIS(ERRCONTENTIONLP0_2),
+		PIS(ERRCONTENTIONLP1_2),
+		PIS(ERRCONTENTIONLP0_3),
+		PIS(ERRCONTENTIONLP1_3),
+		PIS(ULPSACTIVENOT_ALL0),
+		PIS(ULPSACTIVENOT_ALL1));
 #undef PIS
-
-	printk("\n");
 }
 
 #ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
@@ -1121,14 +1109,6 @@ static void _dsi_print_reset_status(struct platform_device *dsidev)
 	 * I/O. */
 	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
 
-	printk(KERN_DEBUG "DSI resets: ");
-
-	l = dsi_read_reg(dsidev, DSI_PLL_STATUS);
-	printk("PLL (%d) ", FLD_GET(l, 0, 0));
-
-	l = dsi_read_reg(dsidev, DSI_COMPLEXIO_CFG1);
-	printk("CIO (%d) ", FLD_GET(l, 29, 29));
-
 	if (dss_has_feature(FEAT_DSI_REVERSE_TXCLKESC)) {
 		b0 = 28;
 		b1 = 27;
@@ -1139,14 +1119,20 @@ static void _dsi_print_reset_status(struct platform_device *dsidev)
 		b2 = 26;
 	}
 
-	l = dsi_read_reg(dsidev, DSI_DSIPHY_CFG5);
-	printk("PHY (%x%x%x, %d, %d, %d)\n",
-			FLD_GET(l, b0, b0),
-			FLD_GET(l, b1, b1),
-			FLD_GET(l, b2, b2),
-			FLD_GET(l, 29, 29),
-			FLD_GET(l, 30, 30),
-			FLD_GET(l, 31, 31));
+#define DSI_FLD_GET(fld, start, end)\
+	FLD_GET(dsi_read_reg(dsidev, DSI_##fld), start, end)
+
+	pr_debug("DSI resets: PLL (%d) CIO (%d) 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, b0, b0),
+		DSI_FLD_GET(DSIPHY_CFG5, b1, b1),
+		DSI_FLD_GET(DSIPHY_CFG5, b2, b2),
+		DSI_FLD_GET(DSIPHY_CFG5, 29, 29),
+		DSI_FLD_GET(DSIPHY_CFG5, 30, 30),
+		DSI_FLD_GET(DSIPHY_CFG5, 31, 31));
+
+#undef DSI_FLD_GET
 }
 #else
 #define _dsi_print_reset_status(x)
-- 
1.7.10

[PATCH V4 5/5] OMAPDSS: Remove dss_debug variable

[Chandrabhanu Mahapatra]

All the debug prints have been replaced with pr_debug(). Thus, the dependency on
dss_debug variable is replaced with dyndbg in dynamic debugging mode and DEBUG
flag otherwise. So, the dss_debug variable is removed along with checks for
DEBUG flag.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/core.c  |    5 -----
 drivers/video/omap2/dss/dispc.c |    8 ++------
 drivers/video/omap2/dss/dsi.c   |    7 -------
 drivers/video/omap2/dss/dss.h   |    4 ----
 4 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 1e1f50f..9abcf43 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -53,11 +53,6 @@ static char *def_disp_name;
 module_param_named(def_disp, def_disp_name, charp, 0);
 MODULE_PARM_DESC(def_disp, "default display name");
 
-#ifdef DEBUG
-bool dss_debug;
-module_param_named(debug, dss_debug, bool, 0644);
-#endif
-
 const char *dss_get_default_display_name(void)
 {
 	return core.default_display_name;
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 67d9f3b..b5204b4 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3669,7 +3669,6 @@ int omap_dispc_unregister_isr(omap_dispc_isr_t isr, void *arg, u32 mask)
 }
 EXPORT_SYMBOL(omap_dispc_unregister_isr);
 
-#ifdef DEBUG
 static void print_irq_status(u32 status)
 {
 	if ((status & dispc.irq_error_mask) = 0)
@@ -3690,7 +3689,6 @@ static void print_irq_status(u32 status)
 		dss_has_feature(FEAT_MGR_LCD3) ? PIS(SYNC_LOST3) : "");
 #undef PIS
 }
-#endif
 
 /* Called from dss.c. Note that we don't touch clocks here,
  * but we presume they are on because we got an IRQ. However,
@@ -3723,10 +3721,8 @@ static irqreturn_t omap_dispc_irq_handler(int irq, void *arg)
 	spin_unlock(&dispc.irq_stats_lock);
 #endif
 
-#ifdef DEBUG
-	if (dss_debug)
-		print_irq_status(irqstatus);
-#endif
+	print_irq_status(irqstatus);
+
 	/* Ack the interrupt. Do it here before clocks are possibly turned
 	 * off */
 	dispc_write_reg(DISPC_IRQSTATUS, irqstatus);
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 6dd073a..4e41c7e 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1107,15 +1107,11 @@ static inline void dsi_enable_pll_clock(struct platform_device *dsidev,
 	}
 }
 
-#ifdef DEBUG
 static void _dsi_print_reset_status(struct platform_device *dsidev)
 {
 	u32 l;
 	int b0, b1, b2;
 
-	if (!dss_debug)
-		return;
-
 	/* A dummy read using the SCP interface to any DSIPHY register is
 	 * required after DSIPHY reset to complete the reset of the DSI complex
 	 * I/O. */
@@ -1146,9 +1142,6 @@ static void _dsi_print_reset_status(struct platform_device *dsidev)
 
 #undef DSI_FLD_GET
 }
-#else
-#define _dsi_print_reset_status(x)
-#endif
 
 static inline int dsi_if_enable(struct platform_device *dsidev, bool enable)
 {
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 8bef7ee..739cfeb 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -23,10 +23,6 @@
 #ifndef __OMAP2_DSS_H
 #define __OMAP2_DSS_H
 
-#ifdef DEBUG
-extern bool dss_debug;
-#endif
-
 #ifdef pr_fmt
 #undef pr_fmt
 #endif
-- 
1.7.10

Re: [PATCH V4 0/5] OMAPDSS: Enable dynamic debug printing

[Tomi Valkeinen]

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

On Sat, 2012-09-29 at 16:19 +0530, Chandrabhanu Mahapatra wrote:
> Hi everyone,
> this patch series aims at cleaning up of DSS of printk()'s enabled with
> dss_debug and replace them with generic dynamic debug printing.

Except for the missing debug print conversions in dsi.c this looks good.
Do you want me to apply the current series and you can send the dsi.c
patch later, or do you want to fix the dsi.c also before I apply?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V4 0/5] OMAPDSS: Enable dynamic debug printing

[Sumit Semwal]

Tomi, Chandrabhanu,

On Friday 05 October 2012 06:16 PM, Tomi Valkeinen wrote:
> On Sat, 2012-09-29 at 16:19 +0530, Chandrabhanu Mahapatra wrote:
>> Hi everyone,
>> this patch series aims at cleaning up of DSS of printk()'s enabled with
>> dss_debug and replace them with generic dynamic debug printing.
>
> Except for the missing debug print conversions in dsi.c this looks good.
> Do you want me to apply the current series and you can send the dsi.c
> patch later, or do you want to fix the dsi.c also before I apply?

With the change for DSI, please feel free to add

Reviewed-by: Sumit Semwal <sumit.semwal@ti.com>

BR,
~Sumit.
>
>   Tomi
>