Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
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 because one can change only one line to change error format for the whole module. For example, in case of mxr_ family, a patch adding function name to debug message would require just a few lines patch. Using in case of dev_ family, one has to produce 200-lines of highly conflicting patch. 'easier to follow' - MAYBE. I agree that some people may prefer to see more directly what is happening, but tell me if you really consider line: mxr_dbg(mdev, this is debug\n); as a very confusing and obfuscated piece of code. COME ON! Regards, TS macros currently only obfuscate the code and make them harder to read for everybody, maybe besides the original driver author ;). Removal of few superfluous lines of code is just a bonus. Or to give up possibility of changing message format in just one place? For over two and half year (since s5p-tv driver introduction in Feb 2011) such change was not needed and it doesn't seem that keeping the code to allow such possibility is worth an added code obfuscation. Besides you can easily script a change to message format so in practice I don't see a real advantage of keeping non-standard messaging macros just for easing a potential message format conversion. I could see migrating from mxr_* to pr_* could seen as the fix, but not this. Such migration seems to be pointless as you would have to add an extra argument to pr_* to not lose the device information. There is something called pr_fmt. It may help with mentioned issue. Best regards, -- Bartlomiej Zolnierkiewicz Samsung RD Institute Poland Samsung Electronics Waiting for reply, Tomasz Stanislawski On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote: Replace mxr_dbg, mxr_info and mxr_warn by generic solution. Signed-off-by: Mateusz Krawczuk m.krawc...@partner.samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/media/platform/s5p-tv/mixer.h | 12 --- drivers/media/platform/s5p-tv/mixer_drv.c | 47 ++- drivers/media/platform/s5p-tv/mixer_grp_layer.c | 2 +- drivers/media/platform/s5p-tv/mixer_reg.c | 6 +- drivers/media/platform/s5p-tv/mixer_video.c | 100 drivers/media/platform/s5p-tv/mixer_vp_layer.c | 2 +- 6 files changed, 78 insertions(+), 91 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
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. :) because one can change only one line to change error format for the whole module. For example, in case of mxr_ family, a patch adding function name to debug message would require just a few lines patch. Using in case of dev_ family, one has to produce 200-lines of highly conflicting patch. * For over two and half year since the s5p-tv driver introduction there was no such need and it is very _unlikely_ that it will be one in the future. * Optimizing for the completely _theorethical_ future patches size is just over-enginneering IMHO. 'easier to follow' - MAYBE. I agree that some people may prefer to see more directly what is happening, but tell me if you really consider line: mxr_dbg(mdev, this is debug\n); as a very confusing and obfuscated piece of code. COME ON! It is confusing becuase you have to lookup what mxr_dbg() actually does, extra time needs to be spent on seeing that the mxr_* macros are just a needless wrappers. You also have to recall it from the memory or look it up again if you come back to the code some time later. Just stop arguing about this trivial change and Ack Mateusz's patch already. :) PS Please try to not top post, thanks. Best regards, -- Bartlomiej Zolnierkiewicz Samsung RD Institute Poland Samsung Electronics Regards, TS macros currently only obfuscate the code and make them harder to read for everybody, maybe besides the original driver author ;). Removal of few superfluous lines of code is just a bonus. Or to give up possibility of changing message format in just one place? For over two and half year (since s5p-tv driver introduction in Feb 2011) such change was not needed and it doesn't seem that keeping the code to allow such possibility is worth an added code obfuscation. Besides you can easily script a change to message format so in practice I don't see a real advantage of keeping non-standard messaging macros just for easing a potential message format conversion. I could see migrating from mxr_* to pr_* could seen as the fix, but not this. Such migration seems to be pointless as you would have to add an extra argument to pr_* to not lose the device information. There is something called pr_fmt. It may help with mentioned issue. Best regards, -- Bartlomiej Zolnierkiewicz Samsung RD Institute Poland Samsung Electronics Waiting for reply, Tomasz Stanislawski On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote: Replace mxr_dbg, mxr_info and mxr_warn by generic solution. Signed-off-by: Mateusz Krawczuk m.krawc...@partner.samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/media/platform/s5p-tv/mixer.h | 12 --- drivers/media/platform/s5p-tv/mixer_drv.c | 47 ++- drivers/media/platform/s5p-tv/mixer_grp_layer.c | 2 +- drivers/media/platform/s5p-tv/mixer_reg.c | 6 +- drivers/media/platform/s5p-tv/mixer_video.c | 100 drivers/media/platform/s5p-tv/mixer_vp_layer.c | 2 +- 6 files changed, 78 insertions(+), 91 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
On 09/23/2013 07:44 PM, Joe Perches wrote:
On Mon, 2013-09-23 at 17:48 +0200, Bartlomiej Zolnierkiewicz wrote:
On Monday, September 23, 2013 04:50:01 PM Tomasz Stanislawski wrote:
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*
macros currently only obfuscate the code and make them harder to read for
everybody, maybe besides the original driver author ;). Removal of few
superfluous lines of code is just a bonus.
I don't see any significant issue with this change.
Using generic mechanisms is good.
Hi Joe,
Sorry for flaming but please let me explain reasons of my opposition to this
patch.
1. It is true that there was no change in mixer messages for 2.5 year in
MAINLINE.
But sometimes I used modification of mxr_ macros while testing the driver.
Therefore those macros are useful for me.
2. The other problem with this patch is its high 'conflictness' during merging.
Unfortunately, sometimes I have to use s5p-tv on platform and configuration
that is only supported in older versions of the kernel + some integration
patches.
The s5p-tv differs from mainline version in those kernels. Therefore
I would need to keep two versions of patches, one for old and another one for
new kernel.
Or backport the 'cleanup patch' and all experimental patches above it.
3. As I understand the coding guidelines asked to use dev_* to ensure that all
error messages have information about the device. There is no change in format
of
errors after this patch. So they do not change anything from userland point of
view.
4. I looked for other files where macro for dev_err is used.
I tried following shell command on v3.12-rc2.
git grep -A1 _err( | grep -A1 '#define' | grep -B1 dev_err
then processing results using
grep -v ^-- | cut -d: -f 1 | sort -u | wc
produced 55 files. Among them, the files below makes use of a macro that is
directly expanded to dev_err(dev_ptr, fmt, ...) without any changes in format.
drivers/firewire/ohci.c
drivers/gpu/drm/i2c/ch7006_priv.h
drivers/gpu/drm/i2c/sil164_drv.c
drivers/infiniband/hw/mthca/mthca_dev.h
drivers/infiniband/hw/qib/qib.h
drivers/media/platform/marvell-ccic/cafe-driver.c
drivers/media/platform/marvell-ccic/mcam-core.c
drivers/media/platform/s5p-tv/mixer.h
drivers/media/platform/via-camera.c
drivers/mtd/devices/docg3.h
drivers/net/ethernet/broadcom/bgmac.h
drivers/net/ethernet/chelsio/cxgb3/common.h
drivers/net/ethernet/intel/e1000/e1000.h
drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
drivers/net/ethernet/mellanox/mlx4/mlx4.h
drivers/net/wireless/iwlegacy/common.h
drivers/pci/hotplug/pciehp.h
drivers/pci/hotplug/shpchp.h
drivers/remoteproc/ste_modem_rproc.c
drivers/scsi/csiostor/csio_hw.h
drivers/staging/fwserial/fwserial.c
drivers/usb/atm/usbatm.h
drivers/usb/host/ehci.h
drivers/usb/host/fhci.h
drivers/usb/host/fotg210-hcd.c
drivers/usb/host/fusbh200-hcd.c
drivers/usb/host/ohci.h
drivers/usb/host/oxu210hp-hcd.c
drivers/usb/host/xhci.h
include/linux/hid.h
include/net/cfg80211.h
Other files makes only cosmetic changes to format, so they might still be worth
to
be 'demacronized'. So I think we can consider that macros wrapping dev_* is
still
a widely used technique so I ask for a good reason before changing the driver.
If one still would like to continue a 'dev_* cleanup crusade' then I kindly
ask to create a big patchset that fixes all over mentioned files.
If most of their maintainers accepts the patches I promise to accept it in
s5p-tv.
Currently, due to mentioned reason the patch is not a cleanup-up for me.
And since I am still a maintainer of this god-forgotten driver I am
going NACK this patch because it makes my work more difficult and because
this patch provides only (if any) relative aesthetic gain.
However this patch can be saved a little. (see below)
Few trivial nits:
I'd remove the trailing periods from some of the messages
at the same time.
Function tracing is better done by the function tracing
mechanism built in to the kernel. Removing the
dev_dbg(dev, %s: enter\n, __func__)
lines would be good too.
Maybe look at the message levels of more of these
logging messages and determine which are actually
useful and what is mostly noise and should be dev_dbg
or deleted altogether.
I agree that most of debugs in form
mxr_dbg(layer-mdev, %s:%d\n, __func__, __LINE__);
are obfuscation. So removal of such lines is a cleanup
and provides some gain and I can ACK this kind of change.
Moreover, I agree that some mxr_info() should be changed mxr_dbg().
I ask Mateusz to modify the 'cleanup' patch to remove only useless
mxr_dbg() and mxr_info() but to keep mxr_* macros intact.
Regards,
Tomasz Stanislawski
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c
b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -59,7 +59,7 @@ void mxr_streamer_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_streamer;
-
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
Em Tue, 24 Sep 2013 12:33:34 +0200 Bartlomiej Zolnierkiewicz b.zolnier...@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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
Hi Tomasz, On Tuesday, September 24, 2013 02:52:44 PM Tomasz Stanislawski wrote: On 09/23/2013 07:44 PM, Joe Perches wrote: On Mon, 2013-09-23 at 17:48 +0200, Bartlomiej Zolnierkiewicz wrote: On Monday, September 23, 2013 04:50:01 PM Tomasz Stanislawski wrote: 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* macros currently only obfuscate the code and make them harder to read for everybody, maybe besides the original driver author ;). Removal of few superfluous lines of code is just a bonus. I don't see any significant issue with this change. Using generic mechanisms is good. Hi Joe, Sorry for flaming but please let me explain reasons of my opposition to this patch. 1. It is true that there was no change in mixer messages for 2.5 year in MAINLINE. But sometimes I used modification of mxr_ macros while testing the driver. Therefore those macros are useful for me. For debug you can also trivially do this with dev_*, just #undef them in your driver and define your versions. 2. The other problem with this patch is its high 'conflictness' during merging. Unfortunately, sometimes I have to use s5p-tv on platform and configuration that is only supported in older versions of the kernel + some integration patches. The s5p-tv differs from mainline version in those kernels. Therefore I would need to keep two versions of patches, one for old and another one for new kernel. You would need to do it or not, depending on the actual change. Anyway in the reality there is practically no development happening on this driver. During two and half year you only did two small fixes to the mixer driver (BTW your other drivers from s5p-tv directory are not using any custom macros): 3c44efd [media] v4l: s5p-tv: mixer: support for dmabuf exporting fa77521 [media] v4l: s5p-tv: mixer: support for dmabuf importing Or backport the 'cleanup patch' and all experimental patches above it. The cost to backport it if/when needed should be pretty small, it is not a big change: 6 files changed, 78 insertions(+), 91 deletions(-) 3. As I understand the coding guidelines asked to use dev_* to ensure that all error messages have information about the device. There is no change in format of errors after this patch. So they do not change anything from userland point of view. Which is actually a good thing (same functionality, less code). 4. I looked for other files where macro for dev_err is used. I tried following shell command on v3.12-rc2. git grep -A1 _err( | grep -A1 '#define' | grep -B1 dev_err then processing results using grep -v ^-- | cut -d: -f 1 | sort -u | wc produced 55 files. Among them, the files below makes use of a macro that is directly expanded to dev_err(dev_ptr, fmt, ...) without any changes in format. drivers/firewire/ohci.c drivers/gpu/drm/i2c/ch7006_priv.h drivers/gpu/drm/i2c/sil164_drv.c drivers/infiniband/hw/mthca/mthca_dev.h drivers/infiniband/hw/qib/qib.h drivers/media/platform/marvell-ccic/cafe-driver.c drivers/media/platform/marvell-ccic/mcam-core.c drivers/media/platform/s5p-tv/mixer.h drivers/media/platform/via-camera.c drivers/mtd/devices/docg3.h drivers/net/ethernet/broadcom/bgmac.h drivers/net/ethernet/chelsio/cxgb3/common.h drivers/net/ethernet/intel/e1000/e1000.h drivers/net/ethernet/intel/ixgbe/ixgbe_common.h drivers/net/ethernet/mellanox/mlx4/mlx4.h drivers/net/wireless/iwlegacy/common.h drivers/pci/hotplug/pciehp.h drivers/pci/hotplug/shpchp.h drivers/remoteproc/ste_modem_rproc.c drivers/scsi/csiostor/csio_hw.h drivers/staging/fwserial/fwserial.c drivers/usb/atm/usbatm.h drivers/usb/host/ehci.h drivers/usb/host/fhci.h drivers/usb/host/fotg210-hcd.c drivers/usb/host/fusbh200-hcd.c drivers/usb/host/ohci.h drivers/usb/host/oxu210hp-hcd.c drivers/usb/host/xhci.h include/linux/hid.h include/net/cfg80211.h Other files makes only cosmetic changes to format, so they might still be worth to be 'demacronized'. So I think we can consider that macros wrapping dev_* is still a widely used technique so I ask for a good reason before changing the driver. If one still would like to continue a 'dev_* cleanup crusade' then I kindly ask to create a big patchset that fixes all over mentioned files. If most of their maintainers accepts the patches I promise to accept it in s5p-tv. OK. Currently, due to mentioned reason the patch is not a cleanup-up for me. And since I am still a maintainer of this god-forgotten driver I am going NACK this patch because it makes my work more difficult and because this patch provides only (if any) relative aesthetic gain. I won't argue about this anymore because I find the whole discussion a bit silly. The change itself is obvious, trivial and cost to either port new patches over it or backport it to older private trees should be very small (as I sit next to
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
On Tue, 2013-09-24 at 14:52 +0200, Tomasz Stanislawski wrote:
On 09/23/2013 07:44 PM, Joe Perches wrote:
On Mon, 2013-09-23 at 17:48 +0200, Bartlomiej Zolnierkiewicz wrote:
On Monday, September 23, 2013 04:50:01 PM Tomasz Stanislawski wrote:
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*
macros currently only obfuscate the code and make them harder to read for
everybody, maybe besides the original driver author ;). Removal of few
superfluous lines of code is just a bonus.
I don't see any significant issue with this change.
Using generic mechanisms is good.
Hi Joe,
Hi Tomasz
Sorry for flaming
Sorry, but I didn't feel any heat.
Maybe I'm too far from the fire.
I'll get closer.
but please let me explain reasons of my opposition to this patch.
I don't have an issue either way.
I prefer using localized logging macros like mxd_level
but I don't much care if actual maintainers want to use
the generic style or a localized style.
4. I looked for other files where macro for dev_err is used.
I tried following shell command on v3.12-rc2.
git grep -A1 _err( | grep -A1 '#define' | grep -B1 dev_err
then processing results using
grep -v ^-- | cut -d: -f 1 | sort -u | wc
You'll have to look farther then 1 line for dev_err
There are uses like:
#define foo_err(pointer, fmt, ...) \
do {\
if (something) \
dev_err(pointer-something, ...); \
} while (0)
Currently, due to mentioned reason the patch is not a cleanup-up for me.
And since I am still a maintainer of this god-forgotten driver I am
going NACK this patch because it makes my work more difficult and because
this patch provides only (if any) relative aesthetic gain.
Good for you.
Maintainer preference trumps global style consistency.
cheers, Joe
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
Hello,
May I ask what is the rationale for this patch?
To reduce a few lines of code?
Or to give up possibility of changing message format in just one place?
I could see migrating from mxr_* to pr_* could seen as the fix, but not this.
Waiting for reply,
Tomasz Stanislawski
On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote:
Replace mxr_dbg, mxr_info and mxr_warn by generic solution.
Signed-off-by: Mateusz Krawczuk m.krawc...@partner.samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/media/platform/s5p-tv/mixer.h | 12 ---
drivers/media/platform/s5p-tv/mixer_drv.c | 47 ++-
drivers/media/platform/s5p-tv/mixer_grp_layer.c | 2 +-
drivers/media/platform/s5p-tv/mixer_reg.c | 6 +-
drivers/media/platform/s5p-tv/mixer_video.c | 100
drivers/media/platform/s5p-tv/mixer_vp_layer.c | 2 +-
6 files changed, 78 insertions(+), 91 deletions(-)
diff --git a/drivers/media/platform/s5p-tv/mixer.h
b/drivers/media/platform/s5p-tv/mixer.h
index 04e6490..c054106 100644
--- a/drivers/media/platform/s5p-tv/mixer.h
+++ b/drivers/media/platform/s5p-tv/mixer.h
@@ -327,18 +327,6 @@ void mxr_streamer_put(struct mxr_device *mdev);
void mxr_get_mbus_fmt(struct mxr_device *mdev,
struct v4l2_mbus_framefmt *mbus_fmt);
-/* Debug */
-
-#define mxr_err(mdev, fmt, ...) dev_err(mdev-dev, fmt, ##__VA_ARGS__)
-#define mxr_warn(mdev, fmt, ...) dev_warn(mdev-dev, fmt, ##__VA_ARGS__)
-#define mxr_info(mdev, fmt, ...) dev_info(mdev-dev, fmt, ##__VA_ARGS__)
-
-#ifdef CONFIG_VIDEO_SAMSUNG_S5P_MIXER_DEBUG
- #define mxr_dbg(mdev, fmt, ...) dev_dbg(mdev-dev, fmt, ##__VA_ARGS__)
-#else
- #define mxr_dbg(mdev, fmt, ...) do { (void) mdev; } while (0)
-#endif
-
/* accessing Mixer's and Video Processor's registers */
void mxr_vsync_set_update(struct mxr_device *mdev, int en);
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c
b/drivers/media/platform/s5p-tv/mixer_drv.c
index 51805a5..8ce7c3e 100644
--- a/drivers/media/platform/s5p-tv/mixer_drv.c
+++ b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -59,7 +59,7 @@ void mxr_streamer_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_streamer;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_streamer);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_streamer);
if (mdev-n_streamer == 1) {
struct v4l2_subdev *sd = to_outsd(mdev);
struct v4l2_mbus_framefmt mbus_fmt;
@@ -91,7 +91,7 @@ void mxr_streamer_put(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
--mdev-n_streamer;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_streamer);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_streamer);
if (mdev-n_streamer == 0) {
int ret;
struct v4l2_subdev *sd = to_outsd(mdev);
@@ -113,7 +113,7 @@ void mxr_output_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_output;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_output);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_output);
/* turn on auxiliary driver */
if (mdev-n_output == 1)
v4l2_subdev_call(to_outsd(mdev), core, s_power, 1);
@@ -124,7 +124,7 @@ void mxr_output_put(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
--mdev-n_output;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_output);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_output);
/* turn on auxiliary driver */
if (mdev-n_output == 0)
v4l2_subdev_call(to_outsd(mdev), core, s_power, 0);
@@ -159,42 +159,42 @@ static int mxr_acquire_plat_resources(struct mxr_device
*mdev,
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, mxr);
if (res == NULL) {
- mxr_err(mdev, get memory resource failed.\n);
+ dev_err(mdev-dev, get memory resource failed.\n);
ret = -ENXIO;
goto fail;
}
mdev-res.mxr_regs = ioremap(res-start, resource_size(res));
if (mdev-res.mxr_regs == NULL) {
- mxr_err(mdev, register mapping failed.\n);
+ dev_err(mdev-dev, register mapping failed.\n);
ret = -ENXIO;
goto fail;
}
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, vp);
if (res == NULL) {
- mxr_err(mdev, get memory resource failed.\n);
+ dev_err(mdev-dev, get memory resource failed.\n);
ret = -ENXIO;
goto fail_mxr_regs;
}
mdev-res.vp_regs = ioremap(res-start, resource_size(res));
if (mdev-res.vp_regs == NULL) {
- mxr_err(mdev, register mapping failed.\n);
+ dev_err(mdev-dev, register mapping failed.\n);
ret = -ENXIO;
goto fail_mxr_regs;
}
res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
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*
macros currently only obfuscate the code and make them harder to read for
everybody, maybe besides the original driver author ;). Removal of few
superfluous lines of code is just a bonus.
Or to give up possibility of changing message format in just one place?
For over two and half year (since s5p-tv driver introduction in Feb 2011)
such change was not needed and it doesn't seem that keeping the code to
allow such possibility is worth an added code obfuscation.
Besides you can easily script a change to message format so in practice
I don't see a real advantage of keeping non-standard messaging macros
just for easing a potential message format conversion.
I could see migrating from mxr_* to pr_* could seen as the fix, but not this.
Such migration seems to be pointless as you would have to add an extra
argument to pr_* to not lose the device information.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung RD Institute Poland
Samsung Electronics
Waiting for reply,
Tomasz Stanislawski
On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote:
Replace mxr_dbg, mxr_info and mxr_warn by generic solution.
Signed-off-by: Mateusz Krawczuk m.krawc...@partner.samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/media/platform/s5p-tv/mixer.h | 12 ---
drivers/media/platform/s5p-tv/mixer_drv.c | 47 ++-
drivers/media/platform/s5p-tv/mixer_grp_layer.c | 2 +-
drivers/media/platform/s5p-tv/mixer_reg.c | 6 +-
drivers/media/platform/s5p-tv/mixer_video.c | 100
drivers/media/platform/s5p-tv/mixer_vp_layer.c | 2 +-
6 files changed, 78 insertions(+), 91 deletions(-)
diff --git a/drivers/media/platform/s5p-tv/mixer.h
b/drivers/media/platform/s5p-tv/mixer.h
index 04e6490..c054106 100644
--- a/drivers/media/platform/s5p-tv/mixer.h
+++ b/drivers/media/platform/s5p-tv/mixer.h
@@ -327,18 +327,6 @@ void mxr_streamer_put(struct mxr_device *mdev);
void mxr_get_mbus_fmt(struct mxr_device *mdev,
struct v4l2_mbus_framefmt *mbus_fmt);
-/* Debug */
-
-#define mxr_err(mdev, fmt, ...) dev_err(mdev-dev, fmt, ##__VA_ARGS__)
-#define mxr_warn(mdev, fmt, ...) dev_warn(mdev-dev, fmt, ##__VA_ARGS__)
-#define mxr_info(mdev, fmt, ...) dev_info(mdev-dev, fmt, ##__VA_ARGS__)
-
-#ifdef CONFIG_VIDEO_SAMSUNG_S5P_MIXER_DEBUG
- #define mxr_dbg(mdev, fmt, ...) dev_dbg(mdev-dev, fmt, ##__VA_ARGS__)
-#else
- #define mxr_dbg(mdev, fmt, ...) do { (void) mdev; } while (0)
-#endif
-
/* accessing Mixer's and Video Processor's registers */
void mxr_vsync_set_update(struct mxr_device *mdev, int en);
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c
b/drivers/media/platform/s5p-tv/mixer_drv.c
index 51805a5..8ce7c3e 100644
--- a/drivers/media/platform/s5p-tv/mixer_drv.c
+++ b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -59,7 +59,7 @@ void mxr_streamer_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_streamer;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_streamer);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_streamer);
if (mdev-n_streamer == 1) {
struct v4l2_subdev *sd = to_outsd(mdev);
struct v4l2_mbus_framefmt mbus_fmt;
@@ -91,7 +91,7 @@ void mxr_streamer_put(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
--mdev-n_streamer;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_streamer);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_streamer);
if (mdev-n_streamer == 0) {
int ret;
struct v4l2_subdev *sd = to_outsd(mdev);
@@ -113,7 +113,7 @@ void mxr_output_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_output;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_output);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_output);
/* turn on auxiliary driver */
if (mdev-n_output == 1)
v4l2_subdev_call(to_outsd(mdev), core, s_power, 1);
@@ -124,7 +124,7 @@ void mxr_output_put(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
--mdev-n_output;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_output);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_output);
/* turn on auxiliary driver */
if (mdev-n_output == 0)
v4l2_subdev_call(to_outsd(mdev), core, s_power, 0);
@@ -159,42 +159,42 @@ static int mxr_acquire_plat_resources(struct
mxr_device *mdev,
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, mxr);
if (res == NULL) {
- mxr_err(mdev, get memory resource failed.\n);
+ dev_err(mdev-dev, get memory resource failed.\n);
ret = -ENXIO;
Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
On Mon, 2013-09-23 at 17:48 +0200, Bartlomiej Zolnierkiewicz wrote:
On Monday, September 23, 2013 04:50:01 PM Tomasz Stanislawski wrote:
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*
macros currently only obfuscate the code and make them harder to read for
everybody, maybe besides the original driver author ;). Removal of few
superfluous lines of code is just a bonus.
I don't see any significant issue with this change.
Using generic mechanisms is good.
Few trivial nits:
I'd remove the trailing periods from some of the messages
at the same time.
Function tracing is better done by the function tracing
mechanism built in to the kernel. Removing the
dev_dbg(dev, %s: enter\n, __func__)
lines would be good too.
Maybe look at the message levels of more of these
logging messages and determine which are actually
useful and what is mostly noise and should be dev_dbg
or deleted altogether.
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c
b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -59,7 +59,7 @@ void mxr_streamer_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_streamer;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_streamer);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_streamer);
not too useful
[]
@@ -159,42 +159,42 @@ static int mxr_acquire_plat_resources(struct
mxr_device *mdev,
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, mxr);
if (res == NULL) {
- mxr_err(mdev, get memory resource failed.\n);
+ dev_err(mdev-dev, get memory resource failed.\n);
dev_err(mdev-dev, get memory resource failed\n);
etc... because of:
@@ -252,27 +252,27 @@ static int mxr_acquire_clocks(struct mxr_device
*mdev)
res-mixer = clk_get(dev, mixer);
if (IS_ERR(res-mixer)) {
- mxr_err(mdev, failed to get clock 'mixer'\n);
+ dev_err(mdev-dev, failed to get clock 'mixer'\n);
Mixed use of messages with/without periods.
@@ -295,13 +295,13 @@ static int mxr_acquire_resources(struct mxr_device
*mdev,
if (ret)
goto fail_plat;
- mxr_info(mdev, resources acquired\n);
+ dev_info(mdev-dev, resources acquired\n);
This isn't really a useful message so I'd convert it
to dev_dbg
@@ -391,7 +391,6 @@ static int mxr_probe(struct platform_device *pdev)
struct mxr_device *mdev;
int ret;
- /* mdev does not exist yet so no mxr_dbg is used */
dev_info(dev, probe start\n);
Same with a lot of these...
Maybe in a separate patch.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_
Replace mxr_dbg, mxr_info and mxr_warn by generic solution.
Signed-off-by: Mateusz Krawczuk m.krawc...@partner.samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/media/platform/s5p-tv/mixer.h | 12 ---
drivers/media/platform/s5p-tv/mixer_drv.c | 47 ++-
drivers/media/platform/s5p-tv/mixer_grp_layer.c | 2 +-
drivers/media/platform/s5p-tv/mixer_reg.c | 6 +-
drivers/media/platform/s5p-tv/mixer_video.c | 100
drivers/media/platform/s5p-tv/mixer_vp_layer.c | 2 +-
6 files changed, 78 insertions(+), 91 deletions(-)
diff --git a/drivers/media/platform/s5p-tv/mixer.h
b/drivers/media/platform/s5p-tv/mixer.h
index 04e6490..c054106 100644
--- a/drivers/media/platform/s5p-tv/mixer.h
+++ b/drivers/media/platform/s5p-tv/mixer.h
@@ -327,18 +327,6 @@ void mxr_streamer_put(struct mxr_device *mdev);
void mxr_get_mbus_fmt(struct mxr_device *mdev,
struct v4l2_mbus_framefmt *mbus_fmt);
-/* Debug */
-
-#define mxr_err(mdev, fmt, ...) dev_err(mdev-dev, fmt, ##__VA_ARGS__)
-#define mxr_warn(mdev, fmt, ...) dev_warn(mdev-dev, fmt, ##__VA_ARGS__)
-#define mxr_info(mdev, fmt, ...) dev_info(mdev-dev, fmt, ##__VA_ARGS__)
-
-#ifdef CONFIG_VIDEO_SAMSUNG_S5P_MIXER_DEBUG
- #define mxr_dbg(mdev, fmt, ...) dev_dbg(mdev-dev, fmt, ##__VA_ARGS__)
-#else
- #define mxr_dbg(mdev, fmt, ...) do { (void) mdev; } while (0)
-#endif
-
/* accessing Mixer's and Video Processor's registers */
void mxr_vsync_set_update(struct mxr_device *mdev, int en);
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c
b/drivers/media/platform/s5p-tv/mixer_drv.c
index 51805a5..8ce7c3e 100644
--- a/drivers/media/platform/s5p-tv/mixer_drv.c
+++ b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -59,7 +59,7 @@ void mxr_streamer_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_streamer;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_streamer);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_streamer);
if (mdev-n_streamer == 1) {
struct v4l2_subdev *sd = to_outsd(mdev);
struct v4l2_mbus_framefmt mbus_fmt;
@@ -91,7 +91,7 @@ void mxr_streamer_put(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
--mdev-n_streamer;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_streamer);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_streamer);
if (mdev-n_streamer == 0) {
int ret;
struct v4l2_subdev *sd = to_outsd(mdev);
@@ -113,7 +113,7 @@ void mxr_output_get(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
++mdev-n_output;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_output);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_output);
/* turn on auxiliary driver */
if (mdev-n_output == 1)
v4l2_subdev_call(to_outsd(mdev), core, s_power, 1);
@@ -124,7 +124,7 @@ void mxr_output_put(struct mxr_device *mdev)
{
mutex_lock(mdev-mutex);
--mdev-n_output;
- mxr_dbg(mdev, %s(%d)\n, __func__, mdev-n_output);
+ dev_dbg(mdev-dev, %s(%d)\n, __func__, mdev-n_output);
/* turn on auxiliary driver */
if (mdev-n_output == 0)
v4l2_subdev_call(to_outsd(mdev), core, s_power, 0);
@@ -159,42 +159,42 @@ static int mxr_acquire_plat_resources(struct mxr_device
*mdev,
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, mxr);
if (res == NULL) {
- mxr_err(mdev, get memory resource failed.\n);
+ dev_err(mdev-dev, get memory resource failed.\n);
ret = -ENXIO;
goto fail;
}
mdev-res.mxr_regs = ioremap(res-start, resource_size(res));
if (mdev-res.mxr_regs == NULL) {
- mxr_err(mdev, register mapping failed.\n);
+ dev_err(mdev-dev, register mapping failed.\n);
ret = -ENXIO;
goto fail;
}
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, vp);
if (res == NULL) {
- mxr_err(mdev, get memory resource failed.\n);
+ dev_err(mdev-dev, get memory resource failed.\n);
ret = -ENXIO;
goto fail_mxr_regs;
}
mdev-res.vp_regs = ioremap(res-start, resource_size(res));
if (mdev-res.vp_regs == NULL) {
- mxr_err(mdev, register mapping failed.\n);
+ dev_err(mdev-dev, register mapping failed.\n);
ret = -ENXIO;
goto fail_mxr_regs;
}
res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, irq);
if (res == NULL) {
- mxr_err(mdev, get interrupt resource failed.\n);
+ dev_err(mdev-dev, get interrupt resource failed.\n);
ret = -ENXIO;
goto fail_vp_regs;
}
ret = request_irq(res-start, mxr_irq_handler, 0, s5p-mixer, mdev);
