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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 28, 2012 at 4:52 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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@xxxxxx>
>> ---
>>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux