From: m.chehab@samsung.com (Mauro Carvalho Chehab)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
Date: Tue, 24 Sep 2013 10:02:40 -0300 [thread overview]
Message-ID: <20130924100240.211dcaa3@samsung.com> (raw)
In-Reply-To: <1583471.Ys4OY3iSLA@amdc1032>
Em Tue, 24 Sep 2013 12:33:34 +0200
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> escreveu:
>
> Hi Tomasz,
>
> On Tuesday, September 24, 2013 11:43:53 AM Tomasz Stanislawski wrote:
> > Hi,
> >
> > On 09/23/2013 05:48 PM, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Monday, September 23, 2013 04:50:01 PM Tomasz Stanislawski wrote:
> > >> Hello,
> > >> May I ask what is the rationale for this patch?
> > >> To reduce a few lines of code?
> > >
> > > This patch makes source code more generic-like and easier to follow (mxd_r*
> >
> > more generic(-like?) - NOT. Using mxr_ macros is a more generic way to produce logs
>
> Using mxr_* macros is not more generic, don't be silly. :)
Let me give my 2 cents on it: it used to be common on media drivers to
define their own print macros. The rationale for that is because the
existing macros, on that time (kernel 2.2), weren't good enough[1].
Other drivers just followed it due to cut-and-paste.
However, nowadays, most developers are just sticking with the existing
common debug infrastructure (either dev_* or pr_* - being the last more
used). By using them, Kernel janitors can do a better job, as they may
have scripts already prepared to check issues there.
I prefer the usage of pr_* macros, as they allow debug messages to be
selectively enabled/disabled, if dynamic printk's are enabled. So,
a change like that actually improves the debug capability on a given
driver.
So, IMHO, the better would be to change the patch to use pr_* macros,
and follow Joe's suggestions to improve it.
Regards,
Mauro
[1] on other drivers that create their own macros, the usual prefix is the
name of the driver (so, on em28xx, it is em28xx_dbg, and so on). In this
case, the driver is s5p_tv. So, a macro following the old way should be
called as s5p_tv_dbg. Yet, it is takes just a fraction of time to identify
them as printk-alike macros, as all those macros are similar.
Cheers,
Mauro
WARNING: multiple messages have this Message-ID (diff)
From: Mauro Carvalho Chehab <m.chehab@samsung.com>
To: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Tomasz Stanislawski <t.stanislaws@samsung.com>,
Mateusz Krawczuk <m.krawczuk@partner.samsung.com>,
t.figa@samsung.com, kyungmin.park@samsung.com,
s.nawrocki@samsung.com, linux-arm-kernel@lists.infradead.org,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
Date: Tue, 24 Sep 2013 10:02:40 -0300 [thread overview]
Message-ID: <20130924100240.211dcaa3@samsung.com> (raw)
In-Reply-To: <1583471.Ys4OY3iSLA@amdc1032>
Em Tue, 24 Sep 2013 12:33:34 +0200
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> escreveu:
>
> Hi Tomasz,
>
> On Tuesday, September 24, 2013 11:43:53 AM Tomasz Stanislawski wrote:
> > Hi,
> >
> > On 09/23/2013 05:48 PM, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Monday, September 23, 2013 04:50:01 PM Tomasz Stanislawski wrote:
> > >> Hello,
> > >> May I ask what is the rationale for this patch?
> > >> To reduce a few lines of code?
> > >
> > > This patch makes source code more generic-like and easier to follow (mxd_r*
> >
> > more generic(-like?) - NOT. Using mxr_ macros is a more generic way to produce logs
>
> Using mxr_* macros is not more generic, don't be silly. :)
Let me give my 2 cents on it: it used to be common on media drivers to
define their own print macros. The rationale for that is because the
existing macros, on that time (kernel 2.2), weren't good enough[1].
Other drivers just followed it due to cut-and-paste.
However, nowadays, most developers are just sticking with the existing
common debug infrastructure (either dev_* or pr_* - being the last more
used). By using them, Kernel janitors can do a better job, as they may
have scripts already prepared to check issues there.
I prefer the usage of pr_* macros, as they allow debug messages to be
selectively enabled/disabled, if dynamic printk's are enabled. So,
a change like that actually improves the debug capability on a given
driver.
So, IMHO, the better would be to change the patch to use pr_* macros,
and follow Joe's suggestions to improve it.
Regards,
Mauro
[1] on other drivers that create their own macros, the usual prefix is the
name of the driver (so, on em28xx, it is em28xx_dbg, and so on). In this
case, the driver is s5p_tv. So, a macro following the old way should be
called as s5p_tv_dbg. Yet, it is takes just a fraction of time to identify
them as printk-alike macros, as all those macros are similar.
Cheers,
Mauro
next prev parent reply other threads:[~2013-09-24 13:02 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-21 15:00 [PATCH v5 0/4] media: s5p-tv: clean-up and fixes Mateusz Krawczuk
2013-09-21 15:00 ` Mateusz Krawczuk
2013-09-21 15:00 ` [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_ Mateusz Krawczuk
2013-09-21 15:00 ` Mateusz Krawczuk
2013-09-23 14:50 ` Tomasz Stanislawski
2013-09-23 14:50 ` Tomasz Stanislawski
2013-09-23 15:48 ` Bartlomiej Zolnierkiewicz
2013-09-23 15:48 ` Bartlomiej Zolnierkiewicz
2013-09-23 17:44 ` Joe Perches
2013-09-23 17:44 ` Joe Perches
2013-09-24 12:52 ` Tomasz Stanislawski
2013-09-24 12:52 ` Tomasz Stanislawski
2013-09-24 15:35 ` Bartlomiej Zolnierkiewicz
2013-09-24 15:35 ` Bartlomiej Zolnierkiewicz
2013-09-24 16:24 ` Joe Perches
2013-09-24 16:24 ` Joe Perches
2013-09-24 9:43 ` Tomasz Stanislawski
2013-09-24 9:43 ` Tomasz Stanislawski
2013-09-24 10:33 ` Bartlomiej Zolnierkiewicz
2013-09-24 10:33 ` Bartlomiej Zolnierkiewicz
2013-09-24 13:02 ` Mauro Carvalho Chehab [this message]
2013-09-24 13:02 ` Mauro Carvalho Chehab
2013-09-21 15:00 ` [PATCH v5 2/4] media: s5p-tv: Restore vpll clock rate Mateusz Krawczuk
2013-09-21 15:00 ` Mateusz Krawczuk
2013-09-25 15:46 ` Tomasz Stanislawski
2013-09-25 15:46 ` Tomasz Stanislawski
2013-10-12 10:06 ` Sylwester Nawrocki
2013-10-12 10:06 ` Sylwester Nawrocki
2013-09-21 15:00 ` [PATCH v5 3/4] media: s5p-tv: Fix sdo driver to work with CCF Mateusz Krawczuk
2013-09-21 15:00 ` Mateusz Krawczuk
2013-09-25 15:59 ` Tomasz Stanislawski
2013-09-25 15:59 ` Tomasz Stanislawski
2013-10-12 10:25 ` Sylwester Nawrocki
2013-10-12 10:25 ` Sylwester Nawrocki
2013-09-21 15:00 ` [PATCH v5 4/4] media: s5p-tv: Fix mixer " Mateusz Krawczuk
2013-09-21 15:00 ` Mateusz Krawczuk
2013-09-23 12:44 ` Sylwester Nawrocki
2013-09-25 16:10 ` Tomasz Stanislawski
2013-09-25 16:09 ` Tomasz Stanislawski
2013-09-25 16:09 ` Tomasz Stanislawski
2013-10-12 10:27 ` Sylwester Nawrocki
2013-10-12 10:27 ` Sylwester Nawrocki
2013-09-23 3:52 ` [PATCH v5 0/4] media: s5p-tv: clean-up and fixes Sachin Kamat
2013-09-23 3:52 ` Sachin Kamat
2013-09-23 12:26 ` Tomasz Figa
2013-09-23 12:26 ` Tomasz Figa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130924100240.211dcaa3@samsung.com \
--to=m.chehab@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.