[PATCH v5 0/4] media: s5p-tv: clean-up and fixes

[PATCH v5 0/4] media: s5p-tv: clean-up and fixes

[Mateusz Krawczuk]

This patch series add restoring previous vpll rate after driver offs stream 
or recives error.
It also replace mxr_info, mxr_dbg, mxr_warn and mxr_err macro 
by generic solution.

Mateusz Krawczuk (4):
  media: s5p-tv: Replace mxr_ macro by default dev_
  media: s5p-tv: Restore vpll clock rate
  media: s5p-tv: Fix sdo driver to work with CCF
  media: s5p-tv: Fix mixer driver to work with CCF

 drivers/media/platform/s5p-tv/mixer.h           |  12 ---
 drivers/media/platform/s5p-tv/mixer_drv.c       |  81 ++++++++++++-------
 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 +-
 drivers/media/platform/s5p-tv/sdo_drv.c         |  39 +++++++--
 7 files changed, 137 insertions(+), 105 deletions(-)

-- 
1.8.1.2

[PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Mateusz Krawczuk]

Replace mxr_dbg, mxr_info and mxr_warn by generic solution.

Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@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);
 	if (ret) {
-		mxr_err(mdev, "request interrupt failed.\n");
+		dev_err(mdev->dev, "request interrupt failed.\n");
 		goto fail_vp_regs;
 	}
 	mdev->res.irq = res->start;
@@ -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");
 		goto fail;
 	}
 	res->vp = clk_get(dev, "vp");
 	if (IS_ERR(res->vp)) {
-		mxr_err(mdev, "failed to get clock 'vp'\n");
+		dev_err(mdev->dev, "failed to get clock 'vp'\n");
 		goto fail;
 	}
 	res->sclk_mixer = clk_get(dev, "sclk_mixer");
 	if (IS_ERR(res->sclk_mixer)) {
-		mxr_err(mdev, "failed to get clock 'sclk_mixer'\n");
+		dev_err(mdev->dev, "failed to get clock 'sclk_mixer'\n");
 		goto fail;
 	}
 	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
 	if (IS_ERR(res->sclk_hdmi)) {
-		mxr_err(mdev, "failed to get clock 'sclk_hdmi'\n");
+		dev_err(mdev->dev, "failed to get clock 'sclk_hdmi'\n");
 		goto fail;
 	}
 	res->sclk_dac = clk_get(dev, "sclk_dac");
 	if (IS_ERR(res->sclk_dac)) {
-		mxr_err(mdev, "failed to get clock 'sclk_dac'\n");
+		dev_err(mdev->dev, "failed to get clock 'sclk_dac'\n");
 		goto fail;
 	}
 
@@ -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");
 	return 0;
 
 fail_plat:
 	mxr_release_plat_resources(mdev);
 fail:
-	mxr_err(mdev, "resources acquire failed\n");
+	dev_err(mdev->dev, "resources acquire failed\n");
 	return ret;
 }
 
@@ -330,7 +330,7 @@ static int mxr_acquire_layers(struct mxr_device *mdev,
 	mdev->layer[2] = mxr_vp_layer_create(mdev, 0);
 
 	if (!mdev->layer[0] || !mdev->layer[1] || !mdev->layer[2]) {
-		mxr_err(mdev, "failed to acquire layers\n");
+		dev_err(mdev->dev, "failed to acquire layers\n");
 		goto fail;
 	}
 
@@ -348,7 +348,7 @@ static int mxr_runtime_resume(struct device *dev)
 	struct mxr_device *mdev = to_mdev(dev);
 	struct mxr_resources *res = &mdev->res;
 
-	mxr_dbg(mdev, "resume - start\n");
+	dev_dbg(mdev->dev, "resume - start\n");
 	mutex_lock(&mdev->mutex);
 	/* turn clocks on */
 	clk_enable(res->mixer);
@@ -356,7 +356,7 @@ static int mxr_runtime_resume(struct device *dev)
 	clk_enable(res->sclk_mixer);
 	/* apply default configuration */
 	mxr_reg_reset(mdev);
-	mxr_dbg(mdev, "resume - finished\n");
+	dev_dbg(mdev->dev, "resume - finished\n");
 
 	mutex_unlock(&mdev->mutex);
 	return 0;
@@ -366,14 +366,14 @@ static int mxr_runtime_suspend(struct device *dev)
 {
 	struct mxr_device *mdev = to_mdev(dev);
 	struct mxr_resources *res = &mdev->res;
-	mxr_dbg(mdev, "suspend - start\n");
+	dev_dbg(mdev->dev, "suspend - start\n");
 	mutex_lock(&mdev->mutex);
 	/* turn clocks off */
 	clk_disable(res->sclk_mixer);
 	clk_disable(res->vp);
 	clk_disable(res->mixer);
 	mutex_unlock(&mdev->mutex);
-	mxr_dbg(mdev, "suspend - finished\n");
+	dev_dbg(mdev->dev, "suspend - finished\n");
 	return 0;
 }
 
@@ -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");
 
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
@@ -426,7 +425,7 @@ static int mxr_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
-	mxr_info(mdev, "probe successful\n");
+	dev_info(mdev->dev, "probe successful\n");
 	return 0;
 
 fail_video:
diff --git a/drivers/media/platform/s5p-tv/mixer_grp_layer.c b/drivers/media/platform/s5p-tv/mixer_grp_layer.c
index b93a21f..24a2355 100644
--- a/drivers/media/platform/s5p-tv/mixer_grp_layer.c
+++ b/drivers/media/platform/s5p-tv/mixer_grp_layer.c
@@ -248,7 +248,7 @@ struct mxr_layer *mxr_graph_layer_create(struct mxr_device *mdev, int idx)
 
 	layer = mxr_base_layer_create(mdev, idx, name, &ops);
 	if (layer == NULL) {
-		mxr_err(mdev, "failed to initialize layer(%d) base\n", idx);
+		dev_err(mdev->dev, "failed to initialize layer(%d) base\n", idx);
 		goto fail;
 	}
 
diff --git a/drivers/media/platform/s5p-tv/mixer_reg.c b/drivers/media/platform/s5p-tv/mixer_reg.c
index b713403..37b0221 100644
--- a/drivers/media/platform/s5p-tv/mixer_reg.c
+++ b/drivers/media/platform/s5p-tv/mixer_reg.c
@@ -368,7 +368,7 @@ int mxr_reg_wait4vsync(struct mxr_device *mdev)
 		return 0;
 	if (ret < 0)
 		return ret;
-	mxr_warn(mdev, "no vsync detected - timeout\n");
+	dev_warn(mdev->dev, "no vsync detected - timeout\n");
 	return -ETIME;
 }
 
@@ -481,7 +481,7 @@ static void mxr_reg_mxr_dump(struct mxr_device *mdev)
 {
 #define DUMPREG(reg_id) \
 do { \
-	mxr_dbg(mdev, #reg_id " = %08x\n", \
+	dev_dbg(mdev->dev, #reg_id " = %08x\n", \
 		(u32)readl(mdev->res.mxr_regs + reg_id)); \
 } while (0)
 
@@ -513,7 +513,7 @@ static void mxr_reg_vp_dump(struct mxr_device *mdev)
 {
 #define DUMPREG(reg_id) \
 do { \
-	mxr_dbg(mdev, #reg_id " = %08x\n", \
+	dev_dbg(mdev->dev, #reg_id " = %08x\n", \
 		(u32) readl(mdev->res.vp_regs + reg_id)); \
 } while (0)
 
diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index 641b1f0..726bdcb 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -42,20 +42,20 @@ static struct v4l2_subdev *find_and_register_subdev(
 	/* TODO: add waiting until probe is finished */
 	drv = driver_find(module_name, &platform_bus_type);
 	if (!drv) {
-		mxr_warn(mdev, "module %s is missing\n", module_name);
+		dev_warn(mdev->dev, "module %s is missing\n", module_name);
 		return NULL;
 	}
 	/* driver refcnt is increased, it is safe to iterate over devices */
 	ret = driver_for_each_device(drv, NULL, &sd, find_reg_callback);
 	/* ret == 0 means that find_reg_callback was never executed */
 	if (sd == NULL) {
-		mxr_warn(mdev, "module %s provides no subdev!\n", module_name);
+		dev_warn(mdev->dev, "module %s provides no subdev!\n", module_name);
 		goto done;
 	}
 	/* v4l2_device_register_subdev detects if sd is NULL */
 	ret = v4l2_device_register_subdev(&mdev->v4l2_dev, sd);
 	if (ret) {
-		mxr_warn(mdev, "failed to register subdev %s\n", sd->name);
+		dev_warn(mdev->dev, "failed to register subdev %s\n", sd->name);
 		sd = NULL;
 	}
 
@@ -76,13 +76,13 @@ int mxr_acquire_video(struct mxr_device *mdev,
 	/* prepare context for V4L2 device */
 	ret = v4l2_device_register(dev, v4l2_dev);
 	if (ret) {
-		mxr_err(mdev, "could not register v4l2 device.\n");
+		dev_err(mdev->dev, "could not register v4l2 device.\n");
 		goto fail;
 	}
 
 	mdev->alloc_ctx = vb2_dma_contig_init_ctx(mdev->dev);
 	if (IS_ERR(mdev->alloc_ctx)) {
-		mxr_err(mdev, "could not acquire vb2 allocator\n");
+		dev_err(mdev->dev, "could not acquire vb2 allocator\n");
 		ret = PTR_ERR(mdev->alloc_ctx);
 		goto fail_v4l2_dev;
 	}
@@ -99,7 +99,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
 			continue;
 		out = kzalloc(sizeof(*out), GFP_KERNEL);
 		if (out == NULL) {
-			mxr_err(mdev, "no memory for '%s'\n",
+			dev_err(mdev->dev, "no memory for '%s'\n",
 				conf->output_name);
 			ret = -ENOMEM;
 			/* registered subdevs are removed in fail_v4l2_dev */
@@ -109,7 +109,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
 		out->sd = sd;
 		out->cookie = conf->cookie;
 		mdev->output[mdev->output_cnt++] = out;
-		mxr_info(mdev, "added output '%s' from module '%s'\n",
+		dev_info(mdev->dev, "added output '%s' from module '%s'\n",
 			conf->output_name, conf->module_name);
 		/* checking if maximal number of outputs is reached */
 		if (mdev->output_cnt >= MXR_MAX_OUTPUTS)
@@ -117,7 +117,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
 	}
 
 	if (mdev->output_cnt == 0) {
-		mxr_err(mdev, "failed to register any output\n");
+		dev_err(mdev->dev, "failed to register any output\n");
 		ret = -ENODEV;
 		/* skipping fail_output because there is nothing to free */
 		goto fail_vb2_allocator;
@@ -160,7 +160,7 @@ static int mxr_querycap(struct file *file, void *priv,
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 
 	strlcpy(cap->driver, MXR_DRIVER_NAME, sizeof(cap->driver));
 	strlcpy(cap->card, layer->vfd.name, sizeof(cap->card));
@@ -173,19 +173,19 @@ static int mxr_querycap(struct file *file, void *priv,
 
 static void mxr_geometry_dump(struct mxr_device *mdev, struct mxr_geometry *geo)
 {
-	mxr_dbg(mdev, "src.full_size = (%u, %u)\n",
+	dev_dbg(mdev->dev, "src.full_size = (%u, %u)\n",
 		geo->src.full_width, geo->src.full_height);
-	mxr_dbg(mdev, "src.size = (%u, %u)\n",
+	dev_dbg(mdev->dev, "src.size = (%u, %u)\n",
 		geo->src.width, geo->src.height);
-	mxr_dbg(mdev, "src.offset = (%u, %u)\n",
+	dev_dbg(mdev->dev, "src.offset = (%u, %u)\n",
 		geo->src.x_offset, geo->src.y_offset);
-	mxr_dbg(mdev, "dst.full_size = (%u, %u)\n",
+	dev_dbg(mdev->dev, "dst.full_size = (%u, %u)\n",
 		geo->dst.full_width, geo->dst.full_height);
-	mxr_dbg(mdev, "dst.size = (%u, %u)\n",
+	dev_dbg(mdev->dev, "dst.size = (%u, %u)\n",
 		geo->dst.width, geo->dst.height);
-	mxr_dbg(mdev, "dst.offset = (%u, %u)\n",
+	dev_dbg(mdev->dev, "dst.offset = (%u, %u)\n",
 		geo->dst.x_offset, geo->dst.y_offset);
-	mxr_dbg(mdev, "ratio = (%u, %u)\n",
+	dev_dbg(mdev->dev, "ratio = (%u, %u)\n",
 		geo->x_ratio, geo->y_ratio);
 }
 
@@ -245,7 +245,7 @@ static int mxr_enum_fmt(struct file *file, void  *priv,
 	struct mxr_device *mdev = layer->mdev;
 	const struct mxr_format *fmt;
 
-	mxr_dbg(mdev, "%s\n", __func__);
+	dev_dbg(mdev->dev, "%s\n", __func__);
 	fmt = find_format_by_index(layer, f->index);
 	if (fmt == NULL)
 		return -EINVAL;
@@ -300,7 +300,7 @@ static int mxr_g_fmt(struct file *file, void *priv,
 	struct mxr_layer *layer = video_drvdata(file);
 	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 
 	pix->width = layer->geo.src.full_width;
 	pix->height = layer->geo.src.full_height;
@@ -321,12 +321,12 @@ static int mxr_s_fmt(struct file *file, void *priv,
 	struct mxr_device *mdev = layer->mdev;
 	struct mxr_geometry *geo = &layer->geo;
 
-	mxr_dbg(mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(mdev->dev, "%s:%d\n", __func__, __LINE__);
 
 	pix = &f->fmt.pix_mp;
 	fmt = find_format_by_fourcc(layer, pix->pixelformat);
 	if (fmt == NULL) {
-		mxr_warn(mdev, "not recognized fourcc: %08x\n",
+		dev_warn(mdev->dev, "not recognized fourcc: %08x\n",
 			pix->pixelformat);
 		return -EINVAL;
 	}
@@ -362,7 +362,7 @@ static int mxr_g_selection(struct file *file, void *fh,
 	struct mxr_layer *layer = video_drvdata(file);
 	struct mxr_geometry *geo = &layer->geo;
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 
 	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
 		s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
@@ -429,7 +429,7 @@ static int mxr_s_selection(struct file *file, void *fh,
 
 	memset(&res, 0, sizeof(res));
 
-	mxr_dbg(layer->mdev, "%s: rect: %dx%d@%d,%d\n", __func__,
+	dev_dbg(layer->mdev->dev, "%s: rect: %dx%d@%d,%d\n", __func__,
 		s->r.width, s->r.height, s->r.left, s->r.top);
 
 	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
@@ -664,7 +664,7 @@ static int mxr_s_output(struct file *file, void *fh, unsigned int i)
 	/* update layers geometry */
 	mxr_layer_update_output(layer);
 
-	mxr_dbg(mdev, "tvnorms = %08llx\n", vfd->tvnorms);
+	dev_dbg(mdev->dev, "tvnorms = %08llx\n", vfd->tvnorms);
 
 	return 0;
 }
@@ -686,7 +686,7 @@ static int mxr_reqbufs(struct file *file, void *priv,
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 	return vb2_reqbufs(&layer->vb_queue, p);
 }
 
@@ -694,7 +694,7 @@ static int mxr_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 	return vb2_querybuf(&layer->vb_queue, p);
 }
 
@@ -702,7 +702,7 @@ static int mxr_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d(%d)\n", __func__, __LINE__, p->index);
+	dev_dbg(layer->mdev->dev, "%s:%d(%d)\n", __func__, __LINE__, p->index);
 	return vb2_qbuf(&layer->vb_queue, p);
 }
 
@@ -710,7 +710,7 @@ static int mxr_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 	return vb2_dqbuf(&layer->vb_queue, p, file->f_flags & O_NONBLOCK);
 }
 
@@ -719,7 +719,7 @@ static int mxr_expbuf(struct file *file, void *priv,
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 	return vb2_expbuf(&layer->vb_queue, eb);
 }
 
@@ -727,7 +727,7 @@ static int mxr_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 	return vb2_streamon(&layer->vb_queue, i);
 }
 
@@ -735,7 +735,7 @@ static int mxr_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 	return vb2_streamoff(&layer->vb_queue, i);
 }
 
@@ -777,7 +777,7 @@ static int mxr_video_open(struct file *file)
 	struct mxr_device *mdev = layer->mdev;
 	int ret = 0;
 
-	mxr_dbg(mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(mdev->dev, "%s:%d\n", __func__, __LINE__);
 	if (mutex_lock_interruptible(&layer->mutex))
 		return -ERESTARTSYS;
 	/* assure device probe is finished */
@@ -785,7 +785,7 @@ static int mxr_video_open(struct file *file)
 	/* creating context for file descriptor */
 	ret = v4l2_fh_open(file);
 	if (ret) {
-		mxr_err(mdev, "v4l2_fh_open failed\n");
+		dev_err(mdev->dev, "v4l2_fh_open failed\n");
 		goto unlock;
 	}
 
@@ -796,13 +796,13 @@ static int mxr_video_open(struct file *file)
 	/* FIXME: should power be enabled on open? */
 	ret = mxr_power_get(mdev);
 	if (ret) {
-		mxr_err(mdev, "power on failed\n");
+		dev_err(mdev->dev, "power on failed\n");
 		goto fail_fh_open;
 	}
 
 	ret = vb2_queue_init(&layer->vb_queue);
 	if (ret != 0) {
-		mxr_err(mdev, "failed to initialize vb2 queue\n");
+		dev_err(mdev->dev, "failed to initialize vb2 queue\n");
 		goto fail_power;
 	}
 	/* set default format, first on the list */
@@ -831,7 +831,7 @@ mxr_video_poll(struct file *file, struct poll_table_struct *wait)
 	struct mxr_layer *layer = video_drvdata(file);
 	unsigned int res;
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 
 	mutex_lock(&layer->mutex);
 	res = vb2_poll(&layer->vb_queue, file, wait);
@@ -844,7 +844,7 @@ static int mxr_video_mmap(struct file *file, struct vm_area_struct *vma)
 	struct mxr_layer *layer = video_drvdata(file);
 	int ret;
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 
 	if (mutex_lock_interruptible(&layer->mutex))
 		return -ERESTARTSYS;
@@ -857,7 +857,7 @@ static int mxr_video_release(struct file *file)
 {
 	struct mxr_layer *layer = video_drvdata(file);
 
-	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
 	mutex_lock(&layer->mutex);
 	if (v4l2_fh_is_singular_file(file)) {
 		vb2_queue_release(&layer->vb_queue);
@@ -887,11 +887,11 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
 	struct mxr_device *mdev = layer->mdev;
 	struct v4l2_plane_pix_format planes[3];
 
-	mxr_dbg(mdev, "%s\n", __func__);
+	dev_dbg(mdev->dev, "%s\n", __func__);
 	/* checking if format was configured */
 	if (fmt == NULL)
 		return -EINVAL;
-	mxr_dbg(mdev, "fmt = %s\n", fmt->name);
+	dev_dbg(mdev->dev, "fmt = %s\n", fmt->name);
 	mxr_mplane_fill(planes, fmt, layer->geo.src.full_width,
 		layer->geo.src.full_height);
 
@@ -899,7 +899,7 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
 	for (i = 0; i < fmt->num_subframes; ++i) {
 		alloc_ctxs[i] = layer->mdev->alloc_ctx;
 		sizes[i] = planes[i].sizeimage;
-		mxr_dbg(mdev, "size[%d] = %08x\n", i, sizes[i]);
+		dev_dbg(mdev->dev, "size[%d] = %08x\n", i, sizes[i]);
 	}
 
 	if (*nbuffers == 0)
@@ -919,14 +919,14 @@ static void buf_queue(struct vb2_buffer *vb)
 	list_add_tail(&buffer->list, &layer->enq_list);
 	spin_unlock_irqrestore(&layer->enq_slock, flags);
 
-	mxr_dbg(mdev, "queuing buffer\n");
+	dev_dbg(mdev->dev, "queuing buffer\n");
 }
 
 static void wait_lock(struct vb2_queue *vq)
 {
 	struct mxr_layer *layer = vb2_get_drv_priv(vq);
 
-	mxr_dbg(layer->mdev, "%s\n", __func__);
+	dev_dbg(layer->mdev->dev, "%s\n", __func__);
 	mutex_lock(&layer->mutex);
 }
 
@@ -934,7 +934,7 @@ static void wait_unlock(struct vb2_queue *vq)
 {
 	struct mxr_layer *layer = vb2_get_drv_priv(vq);
 
-	mxr_dbg(layer->mdev, "%s\n", __func__);
+	dev_dbg(layer->mdev->dev, "%s\n", __func__);
 	mutex_unlock(&layer->mutex);
 }
 
@@ -944,10 +944,10 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct mxr_device *mdev = layer->mdev;
 	unsigned long flags;
 
-	mxr_dbg(mdev, "%s\n", __func__);
+	dev_dbg(mdev->dev, "%s\n", __func__);
 
 	if (count == 0) {
-		mxr_dbg(mdev, "no output buffers queued\n");
+		dev_dbg(mdev->dev, "no output buffers queued\n");
 		return -EINVAL;
 	}
 
@@ -973,7 +973,7 @@ static void mxr_watchdog(unsigned long arg)
 	struct mxr_device *mdev = layer->mdev;
 	unsigned long flags;
 
-	mxr_err(mdev, "watchdog fired for layer %s\n", layer->vfd.name);
+	dev_err(mdev->dev, "watchdog fired for layer %s\n", layer->vfd.name);
 
 	spin_lock_irqsave(&layer->enq_slock, flags);
 
@@ -998,7 +998,7 @@ static int stop_streaming(struct vb2_queue *vq)
 	struct timer_list watchdog;
 	struct mxr_buffer *buf, *buf_tmp;
 
-	mxr_dbg(mdev, "%s\n", __func__);
+	dev_dbg(mdev->dev, "%s\n", __func__);
 
 	spin_lock_irqsave(&layer->enq_slock, flags);
 
@@ -1056,9 +1056,9 @@ int mxr_base_layer_register(struct mxr_layer *layer)
 
 	ret = video_register_device(&layer->vfd, VFL_TYPE_GRABBER, -1);
 	if (ret)
-		mxr_err(mdev, "failed to register video device\n");
+		dev_err(mdev->dev, "failed to register video device\n");
 	else
-		mxr_info(mdev, "registered layer %s as /dev/video%d\n",
+		dev_info(mdev->dev, "registered layer %s as /dev/video%d\n",
 			layer->vfd.name, layer->vfd.num);
 	return ret;
 }
@@ -1091,7 +1091,7 @@ struct mxr_layer *mxr_base_layer_create(struct mxr_device *mdev,
 
 	layer = kzalloc(sizeof(*layer), GFP_KERNEL);
 	if (layer == NULL) {
-		mxr_err(mdev, "not enough memory for layer.\n");
+		dev_err(mdev->dev, "not enough memory for layer.\n");
 		goto fail;
 	}
 
diff --git a/drivers/media/platform/s5p-tv/mixer_vp_layer.c b/drivers/media/platform/s5p-tv/mixer_vp_layer.c
index 3d13a63..38b216e 100644
--- a/drivers/media/platform/s5p-tv/mixer_vp_layer.c
+++ b/drivers/media/platform/s5p-tv/mixer_vp_layer.c
@@ -219,7 +219,7 @@ struct mxr_layer *mxr_vp_layer_create(struct mxr_device *mdev, int idx)
 
 	layer = mxr_base_layer_create(mdev, idx, name, &ops);
 	if (layer == NULL) {
-		mxr_err(mdev, "failed to initialize layer(%d) base\n", idx);
+		dev_err(mdev->dev, "failed to initialize layer(%d) base\n", idx);
 		goto fail;
 	}
 
-- 
1.8.1.2

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Tomasz Stanislawski]

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.krawczuk@partner.samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@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);
>  	if (ret) {
> -		mxr_err(mdev, "request interrupt failed.\n");
> +		dev_err(mdev->dev, "request interrupt failed.\n");
>  		goto fail_vp_regs;
>  	}
>  	mdev->res.irq = res->start;
> @@ -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");
>  		goto fail;
>  	}
>  	res->vp = clk_get(dev, "vp");
>  	if (IS_ERR(res->vp)) {
> -		mxr_err(mdev, "failed to get clock 'vp'\n");
> +		dev_err(mdev->dev, "failed to get clock 'vp'\n");
>  		goto fail;
>  	}
>  	res->sclk_mixer = clk_get(dev, "sclk_mixer");
>  	if (IS_ERR(res->sclk_mixer)) {
> -		mxr_err(mdev, "failed to get clock 'sclk_mixer'\n");
> +		dev_err(mdev->dev, "failed to get clock 'sclk_mixer'\n");
>  		goto fail;
>  	}
>  	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
>  	if (IS_ERR(res->sclk_hdmi)) {
> -		mxr_err(mdev, "failed to get clock 'sclk_hdmi'\n");
> +		dev_err(mdev->dev, "failed to get clock 'sclk_hdmi'\n");
>  		goto fail;
>  	}
>  	res->sclk_dac = clk_get(dev, "sclk_dac");
>  	if (IS_ERR(res->sclk_dac)) {
> -		mxr_err(mdev, "failed to get clock 'sclk_dac'\n");
> +		dev_err(mdev->dev, "failed to get clock 'sclk_dac'\n");
>  		goto fail;
>  	}
>  
> @@ -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");
>  	return 0;
>  
>  fail_plat:
>  	mxr_release_plat_resources(mdev);
>  fail:
> -	mxr_err(mdev, "resources acquire failed\n");
> +	dev_err(mdev->dev, "resources acquire failed\n");
>  	return ret;
>  }
>  
> @@ -330,7 +330,7 @@ static int mxr_acquire_layers(struct mxr_device *mdev,
>  	mdev->layer[2] = mxr_vp_layer_create(mdev, 0);
>  
>  	if (!mdev->layer[0] || !mdev->layer[1] || !mdev->layer[2]) {
> -		mxr_err(mdev, "failed to acquire layers\n");
> +		dev_err(mdev->dev, "failed to acquire layers\n");
>  		goto fail;
>  	}
>  
> @@ -348,7 +348,7 @@ static int mxr_runtime_resume(struct device *dev)
>  	struct mxr_device *mdev = to_mdev(dev);
>  	struct mxr_resources *res = &mdev->res;
>  
> -	mxr_dbg(mdev, "resume - start\n");
> +	dev_dbg(mdev->dev, "resume - start\n");
>  	mutex_lock(&mdev->mutex);
>  	/* turn clocks on */
>  	clk_enable(res->mixer);
> @@ -356,7 +356,7 @@ static int mxr_runtime_resume(struct device *dev)
>  	clk_enable(res->sclk_mixer);
>  	/* apply default configuration */
>  	mxr_reg_reset(mdev);
> -	mxr_dbg(mdev, "resume - finished\n");
> +	dev_dbg(mdev->dev, "resume - finished\n");
>  
>  	mutex_unlock(&mdev->mutex);
>  	return 0;
> @@ -366,14 +366,14 @@ static int mxr_runtime_suspend(struct device *dev)
>  {
>  	struct mxr_device *mdev = to_mdev(dev);
>  	struct mxr_resources *res = &mdev->res;
> -	mxr_dbg(mdev, "suspend - start\n");
> +	dev_dbg(mdev->dev, "suspend - start\n");
>  	mutex_lock(&mdev->mutex);
>  	/* turn clocks off */
>  	clk_disable(res->sclk_mixer);
>  	clk_disable(res->vp);
>  	clk_disable(res->mixer);
>  	mutex_unlock(&mdev->mutex);
> -	mxr_dbg(mdev, "suspend - finished\n");
> +	dev_dbg(mdev->dev, "suspend - finished\n");
>  	return 0;
>  }
>  
> @@ -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");
>  
>  	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> @@ -426,7 +425,7 @@ static int mxr_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(dev);
>  
> -	mxr_info(mdev, "probe successful\n");
> +	dev_info(mdev->dev, "probe successful\n");
>  	return 0;
>  
>  fail_video:
> diff --git a/drivers/media/platform/s5p-tv/mixer_grp_layer.c b/drivers/media/platform/s5p-tv/mixer_grp_layer.c
> index b93a21f..24a2355 100644
> --- a/drivers/media/platform/s5p-tv/mixer_grp_layer.c
> +++ b/drivers/media/platform/s5p-tv/mixer_grp_layer.c
> @@ -248,7 +248,7 @@ struct mxr_layer *mxr_graph_layer_create(struct mxr_device *mdev, int idx)
>  
>  	layer = mxr_base_layer_create(mdev, idx, name, &ops);
>  	if (layer == NULL) {
> -		mxr_err(mdev, "failed to initialize layer(%d) base\n", idx);
> +		dev_err(mdev->dev, "failed to initialize layer(%d) base\n", idx);
>  		goto fail;
>  	}
>  
> diff --git a/drivers/media/platform/s5p-tv/mixer_reg.c b/drivers/media/platform/s5p-tv/mixer_reg.c
> index b713403..37b0221 100644
> --- a/drivers/media/platform/s5p-tv/mixer_reg.c
> +++ b/drivers/media/platform/s5p-tv/mixer_reg.c
> @@ -368,7 +368,7 @@ int mxr_reg_wait4vsync(struct mxr_device *mdev)
>  		return 0;
>  	if (ret < 0)
>  		return ret;
> -	mxr_warn(mdev, "no vsync detected - timeout\n");
> +	dev_warn(mdev->dev, "no vsync detected - timeout\n");
>  	return -ETIME;
>  }
>  
> @@ -481,7 +481,7 @@ static void mxr_reg_mxr_dump(struct mxr_device *mdev)
>  {
>  #define DUMPREG(reg_id) \
>  do { \
> -	mxr_dbg(mdev, #reg_id " = %08x\n", \
> +	dev_dbg(mdev->dev, #reg_id " = %08x\n", \
>  		(u32)readl(mdev->res.mxr_regs + reg_id)); \
>  } while (0)
>  
> @@ -513,7 +513,7 @@ static void mxr_reg_vp_dump(struct mxr_device *mdev)
>  {
>  #define DUMPREG(reg_id) \
>  do { \
> -	mxr_dbg(mdev, #reg_id " = %08x\n", \
> +	dev_dbg(mdev->dev, #reg_id " = %08x\n", \
>  		(u32) readl(mdev->res.vp_regs + reg_id)); \
>  } while (0)
>  
> diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
> index 641b1f0..726bdcb 100644
> --- a/drivers/media/platform/s5p-tv/mixer_video.c
> +++ b/drivers/media/platform/s5p-tv/mixer_video.c
> @@ -42,20 +42,20 @@ static struct v4l2_subdev *find_and_register_subdev(
>  	/* TODO: add waiting until probe is finished */
>  	drv = driver_find(module_name, &platform_bus_type);
>  	if (!drv) {
> -		mxr_warn(mdev, "module %s is missing\n", module_name);
> +		dev_warn(mdev->dev, "module %s is missing\n", module_name);
>  		return NULL;
>  	}
>  	/* driver refcnt is increased, it is safe to iterate over devices */
>  	ret = driver_for_each_device(drv, NULL, &sd, find_reg_callback);
>  	/* ret == 0 means that find_reg_callback was never executed */
>  	if (sd == NULL) {
> -		mxr_warn(mdev, "module %s provides no subdev!\n", module_name);
> +		dev_warn(mdev->dev, "module %s provides no subdev!\n", module_name);
>  		goto done;
>  	}
>  	/* v4l2_device_register_subdev detects if sd is NULL */
>  	ret = v4l2_device_register_subdev(&mdev->v4l2_dev, sd);
>  	if (ret) {
> -		mxr_warn(mdev, "failed to register subdev %s\n", sd->name);
> +		dev_warn(mdev->dev, "failed to register subdev %s\n", sd->name);
>  		sd = NULL;
>  	}
>  
> @@ -76,13 +76,13 @@ int mxr_acquire_video(struct mxr_device *mdev,
>  	/* prepare context for V4L2 device */
>  	ret = v4l2_device_register(dev, v4l2_dev);
>  	if (ret) {
> -		mxr_err(mdev, "could not register v4l2 device.\n");
> +		dev_err(mdev->dev, "could not register v4l2 device.\n");
>  		goto fail;
>  	}
>  
>  	mdev->alloc_ctx = vb2_dma_contig_init_ctx(mdev->dev);
>  	if (IS_ERR(mdev->alloc_ctx)) {
> -		mxr_err(mdev, "could not acquire vb2 allocator\n");
> +		dev_err(mdev->dev, "could not acquire vb2 allocator\n");
>  		ret = PTR_ERR(mdev->alloc_ctx);
>  		goto fail_v4l2_dev;
>  	}
> @@ -99,7 +99,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
>  			continue;
>  		out = kzalloc(sizeof(*out), GFP_KERNEL);
>  		if (out == NULL) {
> -			mxr_err(mdev, "no memory for '%s'\n",
> +			dev_err(mdev->dev, "no memory for '%s'\n",
>  				conf->output_name);
>  			ret = -ENOMEM;
>  			/* registered subdevs are removed in fail_v4l2_dev */
> @@ -109,7 +109,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
>  		out->sd = sd;
>  		out->cookie = conf->cookie;
>  		mdev->output[mdev->output_cnt++] = out;
> -		mxr_info(mdev, "added output '%s' from module '%s'\n",
> +		dev_info(mdev->dev, "added output '%s' from module '%s'\n",
>  			conf->output_name, conf->module_name);
>  		/* checking if maximal number of outputs is reached */
>  		if (mdev->output_cnt >= MXR_MAX_OUTPUTS)
> @@ -117,7 +117,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
>  	}
>  
>  	if (mdev->output_cnt == 0) {
> -		mxr_err(mdev, "failed to register any output\n");
> +		dev_err(mdev->dev, "failed to register any output\n");
>  		ret = -ENODEV;
>  		/* skipping fail_output because there is nothing to free */
>  		goto fail_vb2_allocator;
> @@ -160,7 +160,7 @@ static int mxr_querycap(struct file *file, void *priv,
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  
>  	strlcpy(cap->driver, MXR_DRIVER_NAME, sizeof(cap->driver));
>  	strlcpy(cap->card, layer->vfd.name, sizeof(cap->card));
> @@ -173,19 +173,19 @@ static int mxr_querycap(struct file *file, void *priv,
>  
>  static void mxr_geometry_dump(struct mxr_device *mdev, struct mxr_geometry *geo)
>  {
> -	mxr_dbg(mdev, "src.full_size = (%u, %u)\n",
> +	dev_dbg(mdev->dev, "src.full_size = (%u, %u)\n",
>  		geo->src.full_width, geo->src.full_height);
> -	mxr_dbg(mdev, "src.size = (%u, %u)\n",
> +	dev_dbg(mdev->dev, "src.size = (%u, %u)\n",
>  		geo->src.width, geo->src.height);
> -	mxr_dbg(mdev, "src.offset = (%u, %u)\n",
> +	dev_dbg(mdev->dev, "src.offset = (%u, %u)\n",
>  		geo->src.x_offset, geo->src.y_offset);
> -	mxr_dbg(mdev, "dst.full_size = (%u, %u)\n",
> +	dev_dbg(mdev->dev, "dst.full_size = (%u, %u)\n",
>  		geo->dst.full_width, geo->dst.full_height);
> -	mxr_dbg(mdev, "dst.size = (%u, %u)\n",
> +	dev_dbg(mdev->dev, "dst.size = (%u, %u)\n",
>  		geo->dst.width, geo->dst.height);
> -	mxr_dbg(mdev, "dst.offset = (%u, %u)\n",
> +	dev_dbg(mdev->dev, "dst.offset = (%u, %u)\n",
>  		geo->dst.x_offset, geo->dst.y_offset);
> -	mxr_dbg(mdev, "ratio = (%u, %u)\n",
> +	dev_dbg(mdev->dev, "ratio = (%u, %u)\n",
>  		geo->x_ratio, geo->y_ratio);
>  }
>  
> @@ -245,7 +245,7 @@ static int mxr_enum_fmt(struct file *file, void  *priv,
>  	struct mxr_device *mdev = layer->mdev;
>  	const struct mxr_format *fmt;
>  
> -	mxr_dbg(mdev, "%s\n", __func__);
> +	dev_dbg(mdev->dev, "%s\n", __func__);
>  	fmt = find_format_by_index(layer, f->index);
>  	if (fmt == NULL)
>  		return -EINVAL;
> @@ -300,7 +300,7 @@ static int mxr_g_fmt(struct file *file, void *priv,
>  	struct mxr_layer *layer = video_drvdata(file);
>  	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  
>  	pix->width = layer->geo.src.full_width;
>  	pix->height = layer->geo.src.full_height;
> @@ -321,12 +321,12 @@ static int mxr_s_fmt(struct file *file, void *priv,
>  	struct mxr_device *mdev = layer->mdev;
>  	struct mxr_geometry *geo = &layer->geo;
>  
> -	mxr_dbg(mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(mdev->dev, "%s:%d\n", __func__, __LINE__);
>  
>  	pix = &f->fmt.pix_mp;
>  	fmt = find_format_by_fourcc(layer, pix->pixelformat);
>  	if (fmt == NULL) {
> -		mxr_warn(mdev, "not recognized fourcc: %08x\n",
> +		dev_warn(mdev->dev, "not recognized fourcc: %08x\n",
>  			pix->pixelformat);
>  		return -EINVAL;
>  	}
> @@ -362,7 +362,7 @@ static int mxr_g_selection(struct file *file, void *fh,
>  	struct mxr_layer *layer = video_drvdata(file);
>  	struct mxr_geometry *geo = &layer->geo;
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  
>  	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
>  		s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> @@ -429,7 +429,7 @@ static int mxr_s_selection(struct file *file, void *fh,
>  
>  	memset(&res, 0, sizeof(res));
>  
> -	mxr_dbg(layer->mdev, "%s: rect: %dx%d@%d,%d\n", __func__,
> +	dev_dbg(layer->mdev->dev, "%s: rect: %dx%d@%d,%d\n", __func__,
>  		s->r.width, s->r.height, s->r.left, s->r.top);
>  
>  	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> @@ -664,7 +664,7 @@ static int mxr_s_output(struct file *file, void *fh, unsigned int i)
>  	/* update layers geometry */
>  	mxr_layer_update_output(layer);
>  
> -	mxr_dbg(mdev, "tvnorms = %08llx\n", vfd->tvnorms);
> +	dev_dbg(mdev->dev, "tvnorms = %08llx\n", vfd->tvnorms);
>  
>  	return 0;
>  }
> @@ -686,7 +686,7 @@ static int mxr_reqbufs(struct file *file, void *priv,
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	return vb2_reqbufs(&layer->vb_queue, p);
>  }
>  
> @@ -694,7 +694,7 @@ static int mxr_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	return vb2_querybuf(&layer->vb_queue, p);
>  }
>  
> @@ -702,7 +702,7 @@ static int mxr_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d(%d)\n", __func__, __LINE__, p->index);
> +	dev_dbg(layer->mdev->dev, "%s:%d(%d)\n", __func__, __LINE__, p->index);
>  	return vb2_qbuf(&layer->vb_queue, p);
>  }
>  
> @@ -710,7 +710,7 @@ static int mxr_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	return vb2_dqbuf(&layer->vb_queue, p, file->f_flags & O_NONBLOCK);
>  }
>  
> @@ -719,7 +719,7 @@ static int mxr_expbuf(struct file *file, void *priv,
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	return vb2_expbuf(&layer->vb_queue, eb);
>  }
>  
> @@ -727,7 +727,7 @@ static int mxr_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	return vb2_streamon(&layer->vb_queue, i);
>  }
>  
> @@ -735,7 +735,7 @@ static int mxr_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	return vb2_streamoff(&layer->vb_queue, i);
>  }
>  
> @@ -777,7 +777,7 @@ static int mxr_video_open(struct file *file)
>  	struct mxr_device *mdev = layer->mdev;
>  	int ret = 0;
>  
> -	mxr_dbg(mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	if (mutex_lock_interruptible(&layer->mutex))
>  		return -ERESTARTSYS;
>  	/* assure device probe is finished */
> @@ -785,7 +785,7 @@ static int mxr_video_open(struct file *file)
>  	/* creating context for file descriptor */
>  	ret = v4l2_fh_open(file);
>  	if (ret) {
> -		mxr_err(mdev, "v4l2_fh_open failed\n");
> +		dev_err(mdev->dev, "v4l2_fh_open failed\n");
>  		goto unlock;
>  	}
>  
> @@ -796,13 +796,13 @@ static int mxr_video_open(struct file *file)
>  	/* FIXME: should power be enabled on open? */
>  	ret = mxr_power_get(mdev);
>  	if (ret) {
> -		mxr_err(mdev, "power on failed\n");
> +		dev_err(mdev->dev, "power on failed\n");
>  		goto fail_fh_open;
>  	}
>  
>  	ret = vb2_queue_init(&layer->vb_queue);
>  	if (ret != 0) {
> -		mxr_err(mdev, "failed to initialize vb2 queue\n");
> +		dev_err(mdev->dev, "failed to initialize vb2 queue\n");
>  		goto fail_power;
>  	}
>  	/* set default format, first on the list */
> @@ -831,7 +831,7 @@ mxr_video_poll(struct file *file, struct poll_table_struct *wait)
>  	struct mxr_layer *layer = video_drvdata(file);
>  	unsigned int res;
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  
>  	mutex_lock(&layer->mutex);
>  	res = vb2_poll(&layer->vb_queue, file, wait);
> @@ -844,7 +844,7 @@ static int mxr_video_mmap(struct file *file, struct vm_area_struct *vma)
>  	struct mxr_layer *layer = video_drvdata(file);
>  	int ret;
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  
>  	if (mutex_lock_interruptible(&layer->mutex))
>  		return -ERESTARTSYS;
> @@ -857,7 +857,7 @@ static int mxr_video_release(struct file *file)
>  {
>  	struct mxr_layer *layer = video_drvdata(file);
>  
> -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
>  	mutex_lock(&layer->mutex);
>  	if (v4l2_fh_is_singular_file(file)) {
>  		vb2_queue_release(&layer->vb_queue);
> @@ -887,11 +887,11 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
>  	struct mxr_device *mdev = layer->mdev;
>  	struct v4l2_plane_pix_format planes[3];
>  
> -	mxr_dbg(mdev, "%s\n", __func__);
> +	dev_dbg(mdev->dev, "%s\n", __func__);
>  	/* checking if format was configured */
>  	if (fmt == NULL)
>  		return -EINVAL;
> -	mxr_dbg(mdev, "fmt = %s\n", fmt->name);
> +	dev_dbg(mdev->dev, "fmt = %s\n", fmt->name);
>  	mxr_mplane_fill(planes, fmt, layer->geo.src.full_width,
>  		layer->geo.src.full_height);
>  
> @@ -899,7 +899,7 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
>  	for (i = 0; i < fmt->num_subframes; ++i) {
>  		alloc_ctxs[i] = layer->mdev->alloc_ctx;
>  		sizes[i] = planes[i].sizeimage;
> -		mxr_dbg(mdev, "size[%d] = %08x\n", i, sizes[i]);
> +		dev_dbg(mdev->dev, "size[%d] = %08x\n", i, sizes[i]);
>  	}
>  
>  	if (*nbuffers == 0)
> @@ -919,14 +919,14 @@ static void buf_queue(struct vb2_buffer *vb)
>  	list_add_tail(&buffer->list, &layer->enq_list);
>  	spin_unlock_irqrestore(&layer->enq_slock, flags);
>  
> -	mxr_dbg(mdev, "queuing buffer\n");
> +	dev_dbg(mdev->dev, "queuing buffer\n");
>  }
>  
>  static void wait_lock(struct vb2_queue *vq)
>  {
>  	struct mxr_layer *layer = vb2_get_drv_priv(vq);
>  
> -	mxr_dbg(layer->mdev, "%s\n", __func__);
> +	dev_dbg(layer->mdev->dev, "%s\n", __func__);
>  	mutex_lock(&layer->mutex);
>  }
>  
> @@ -934,7 +934,7 @@ static void wait_unlock(struct vb2_queue *vq)
>  {
>  	struct mxr_layer *layer = vb2_get_drv_priv(vq);
>  
> -	mxr_dbg(layer->mdev, "%s\n", __func__);
> +	dev_dbg(layer->mdev->dev, "%s\n", __func__);
>  	mutex_unlock(&layer->mutex);
>  }
>  
> @@ -944,10 +944,10 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	struct mxr_device *mdev = layer->mdev;
>  	unsigned long flags;
>  
> -	mxr_dbg(mdev, "%s\n", __func__);
> +	dev_dbg(mdev->dev, "%s\n", __func__);
>  
>  	if (count == 0) {
> -		mxr_dbg(mdev, "no output buffers queued\n");
> +		dev_dbg(mdev->dev, "no output buffers queued\n");
>  		return -EINVAL;
>  	}
>  
> @@ -973,7 +973,7 @@ static void mxr_watchdog(unsigned long arg)
>  	struct mxr_device *mdev = layer->mdev;
>  	unsigned long flags;
>  
> -	mxr_err(mdev, "watchdog fired for layer %s\n", layer->vfd.name);
> +	dev_err(mdev->dev, "watchdog fired for layer %s\n", layer->vfd.name);
>  
>  	spin_lock_irqsave(&layer->enq_slock, flags);
>  
> @@ -998,7 +998,7 @@ static int stop_streaming(struct vb2_queue *vq)
>  	struct timer_list watchdog;
>  	struct mxr_buffer *buf, *buf_tmp;
>  
> -	mxr_dbg(mdev, "%s\n", __func__);
> +	dev_dbg(mdev->dev, "%s\n", __func__);
>  
>  	spin_lock_irqsave(&layer->enq_slock, flags);
>  
> @@ -1056,9 +1056,9 @@ int mxr_base_layer_register(struct mxr_layer *layer)
>  
>  	ret = video_register_device(&layer->vfd, VFL_TYPE_GRABBER, -1);
>  	if (ret)
> -		mxr_err(mdev, "failed to register video device\n");
> +		dev_err(mdev->dev, "failed to register video device\n");
>  	else
> -		mxr_info(mdev, "registered layer %s as /dev/video%d\n",
> +		dev_info(mdev->dev, "registered layer %s as /dev/video%d\n",
>  			layer->vfd.name, layer->vfd.num);
>  	return ret;
>  }
> @@ -1091,7 +1091,7 @@ struct mxr_layer *mxr_base_layer_create(struct mxr_device *mdev,
>  
>  	layer = kzalloc(sizeof(*layer), GFP_KERNEL);
>  	if (layer == NULL) {
> -		mxr_err(mdev, "not enough memory for layer.\n");
> +		dev_err(mdev->dev, "not enough memory for layer.\n");
>  		goto fail;
>  	}
>  
> diff --git a/drivers/media/platform/s5p-tv/mixer_vp_layer.c b/drivers/media/platform/s5p-tv/mixer_vp_layer.c
> index 3d13a63..38b216e 100644
> --- a/drivers/media/platform/s5p-tv/mixer_vp_layer.c
> +++ b/drivers/media/platform/s5p-tv/mixer_vp_layer.c
> @@ -219,7 +219,7 @@ struct mxr_layer *mxr_vp_layer_create(struct mxr_device *mdev, int idx)
>  
>  	layer = mxr_base_layer_create(mdev, idx, name, &ops);
>  	if (layer == NULL) {
> -		mxr_err(mdev, "failed to initialize layer(%d) base\n", idx);
> +		dev_err(mdev->dev, "failed to initialize layer(%d) base\n", idx);
>  		goto fail;
>  	}
>  
> 

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Bartlomiej Zolnierkiewicz]

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 R&D 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.krawczuk@partner.samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@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);
> >  	if (ret) {
> > -		mxr_err(mdev, "request interrupt failed.\n");
> > +		dev_err(mdev->dev, "request interrupt failed.\n");
> >  		goto fail_vp_regs;
> >  	}
> >  	mdev->res.irq = res->start;
> > @@ -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");
> >  		goto fail;
> >  	}
> >  	res->vp = clk_get(dev, "vp");
> >  	if (IS_ERR(res->vp)) {
> > -		mxr_err(mdev, "failed to get clock 'vp'\n");
> > +		dev_err(mdev->dev, "failed to get clock 'vp'\n");
> >  		goto fail;
> >  	}
> >  	res->sclk_mixer = clk_get(dev, "sclk_mixer");
> >  	if (IS_ERR(res->sclk_mixer)) {
> > -		mxr_err(mdev, "failed to get clock 'sclk_mixer'\n");
> > +		dev_err(mdev->dev, "failed to get clock 'sclk_mixer'\n");
> >  		goto fail;
> >  	}
> >  	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
> >  	if (IS_ERR(res->sclk_hdmi)) {
> > -		mxr_err(mdev, "failed to get clock 'sclk_hdmi'\n");
> > +		dev_err(mdev->dev, "failed to get clock 'sclk_hdmi'\n");
> >  		goto fail;
> >  	}
> >  	res->sclk_dac = clk_get(dev, "sclk_dac");
> >  	if (IS_ERR(res->sclk_dac)) {
> > -		mxr_err(mdev, "failed to get clock 'sclk_dac'\n");
> > +		dev_err(mdev->dev, "failed to get clock 'sclk_dac'\n");
> >  		goto fail;
> >  	}
> >  
> > @@ -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");
> >  	return 0;
> >  
> >  fail_plat:
> >  	mxr_release_plat_resources(mdev);
> >  fail:
> > -	mxr_err(mdev, "resources acquire failed\n");
> > +	dev_err(mdev->dev, "resources acquire failed\n");
> >  	return ret;
> >  }
> >  
> > @@ -330,7 +330,7 @@ static int mxr_acquire_layers(struct mxr_device *mdev,
> >  	mdev->layer[2] = mxr_vp_layer_create(mdev, 0);
> >  
> >  	if (!mdev->layer[0] || !mdev->layer[1] || !mdev->layer[2]) {
> > -		mxr_err(mdev, "failed to acquire layers\n");
> > +		dev_err(mdev->dev, "failed to acquire layers\n");
> >  		goto fail;
> >  	}
> >  
> > @@ -348,7 +348,7 @@ static int mxr_runtime_resume(struct device *dev)
> >  	struct mxr_device *mdev = to_mdev(dev);
> >  	struct mxr_resources *res = &mdev->res;
> >  
> > -	mxr_dbg(mdev, "resume - start\n");
> > +	dev_dbg(mdev->dev, "resume - start\n");
> >  	mutex_lock(&mdev->mutex);
> >  	/* turn clocks on */
> >  	clk_enable(res->mixer);
> > @@ -356,7 +356,7 @@ static int mxr_runtime_resume(struct device *dev)
> >  	clk_enable(res->sclk_mixer);
> >  	/* apply default configuration */
> >  	mxr_reg_reset(mdev);
> > -	mxr_dbg(mdev, "resume - finished\n");
> > +	dev_dbg(mdev->dev, "resume - finished\n");
> >  
> >  	mutex_unlock(&mdev->mutex);
> >  	return 0;
> > @@ -366,14 +366,14 @@ static int mxr_runtime_suspend(struct device *dev)
> >  {
> >  	struct mxr_device *mdev = to_mdev(dev);
> >  	struct mxr_resources *res = &mdev->res;
> > -	mxr_dbg(mdev, "suspend - start\n");
> > +	dev_dbg(mdev->dev, "suspend - start\n");
> >  	mutex_lock(&mdev->mutex);
> >  	/* turn clocks off */
> >  	clk_disable(res->sclk_mixer);
> >  	clk_disable(res->vp);
> >  	clk_disable(res->mixer);
> >  	mutex_unlock(&mdev->mutex);
> > -	mxr_dbg(mdev, "suspend - finished\n");
> > +	dev_dbg(mdev->dev, "suspend - finished\n");
> >  	return 0;
> >  }
> >  
> > @@ -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");
> >  
> >  	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > @@ -426,7 +425,7 @@ static int mxr_probe(struct platform_device *pdev)
> >  
> >  	pm_runtime_enable(dev);
> >  
> > -	mxr_info(mdev, "probe successful\n");
> > +	dev_info(mdev->dev, "probe successful\n");
> >  	return 0;
> >  
> >  fail_video:
> > diff --git a/drivers/media/platform/s5p-tv/mixer_grp_layer.c b/drivers/media/platform/s5p-tv/mixer_grp_layer.c
> > index b93a21f..24a2355 100644
> > --- a/drivers/media/platform/s5p-tv/mixer_grp_layer.c
> > +++ b/drivers/media/platform/s5p-tv/mixer_grp_layer.c
> > @@ -248,7 +248,7 @@ struct mxr_layer *mxr_graph_layer_create(struct mxr_device *mdev, int idx)
> >  
> >  	layer = mxr_base_layer_create(mdev, idx, name, &ops);
> >  	if (layer == NULL) {
> > -		mxr_err(mdev, "failed to initialize layer(%d) base\n", idx);
> > +		dev_err(mdev->dev, "failed to initialize layer(%d) base\n", idx);
> >  		goto fail;
> >  	}
> >  
> > diff --git a/drivers/media/platform/s5p-tv/mixer_reg.c b/drivers/media/platform/s5p-tv/mixer_reg.c
> > index b713403..37b0221 100644
> > --- a/drivers/media/platform/s5p-tv/mixer_reg.c
> > +++ b/drivers/media/platform/s5p-tv/mixer_reg.c
> > @@ -368,7 +368,7 @@ int mxr_reg_wait4vsync(struct mxr_device *mdev)
> >  		return 0;
> >  	if (ret < 0)
> >  		return ret;
> > -	mxr_warn(mdev, "no vsync detected - timeout\n");
> > +	dev_warn(mdev->dev, "no vsync detected - timeout\n");
> >  	return -ETIME;
> >  }
> >  
> > @@ -481,7 +481,7 @@ static void mxr_reg_mxr_dump(struct mxr_device *mdev)
> >  {
> >  #define DUMPREG(reg_id) \
> >  do { \
> > -	mxr_dbg(mdev, #reg_id " = %08x\n", \
> > +	dev_dbg(mdev->dev, #reg_id " = %08x\n", \
> >  		(u32)readl(mdev->res.mxr_regs + reg_id)); \
> >  } while (0)
> >  
> > @@ -513,7 +513,7 @@ static void mxr_reg_vp_dump(struct mxr_device *mdev)
> >  {
> >  #define DUMPREG(reg_id) \
> >  do { \
> > -	mxr_dbg(mdev, #reg_id " = %08x\n", \
> > +	dev_dbg(mdev->dev, #reg_id " = %08x\n", \
> >  		(u32) readl(mdev->res.vp_regs + reg_id)); \
> >  } while (0)
> >  
> > diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
> > index 641b1f0..726bdcb 100644
> > --- a/drivers/media/platform/s5p-tv/mixer_video.c
> > +++ b/drivers/media/platform/s5p-tv/mixer_video.c
> > @@ -42,20 +42,20 @@ static struct v4l2_subdev *find_and_register_subdev(
> >  	/* TODO: add waiting until probe is finished */
> >  	drv = driver_find(module_name, &platform_bus_type);
> >  	if (!drv) {
> > -		mxr_warn(mdev, "module %s is missing\n", module_name);
> > +		dev_warn(mdev->dev, "module %s is missing\n", module_name);
> >  		return NULL;
> >  	}
> >  	/* driver refcnt is increased, it is safe to iterate over devices */
> >  	ret = driver_for_each_device(drv, NULL, &sd, find_reg_callback);
> >  	/* ret == 0 means that find_reg_callback was never executed */
> >  	if (sd == NULL) {
> > -		mxr_warn(mdev, "module %s provides no subdev!\n", module_name);
> > +		dev_warn(mdev->dev, "module %s provides no subdev!\n", module_name);
> >  		goto done;
> >  	}
> >  	/* v4l2_device_register_subdev detects if sd is NULL */
> >  	ret = v4l2_device_register_subdev(&mdev->v4l2_dev, sd);
> >  	if (ret) {
> > -		mxr_warn(mdev, "failed to register subdev %s\n", sd->name);
> > +		dev_warn(mdev->dev, "failed to register subdev %s\n", sd->name);
> >  		sd = NULL;
> >  	}
> >  
> > @@ -76,13 +76,13 @@ int mxr_acquire_video(struct mxr_device *mdev,
> >  	/* prepare context for V4L2 device */
> >  	ret = v4l2_device_register(dev, v4l2_dev);
> >  	if (ret) {
> > -		mxr_err(mdev, "could not register v4l2 device.\n");
> > +		dev_err(mdev->dev, "could not register v4l2 device.\n");
> >  		goto fail;
> >  	}
> >  
> >  	mdev->alloc_ctx = vb2_dma_contig_init_ctx(mdev->dev);
> >  	if (IS_ERR(mdev->alloc_ctx)) {
> > -		mxr_err(mdev, "could not acquire vb2 allocator\n");
> > +		dev_err(mdev->dev, "could not acquire vb2 allocator\n");
> >  		ret = PTR_ERR(mdev->alloc_ctx);
> >  		goto fail_v4l2_dev;
> >  	}
> > @@ -99,7 +99,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
> >  			continue;
> >  		out = kzalloc(sizeof(*out), GFP_KERNEL);
> >  		if (out == NULL) {
> > -			mxr_err(mdev, "no memory for '%s'\n",
> > +			dev_err(mdev->dev, "no memory for '%s'\n",
> >  				conf->output_name);
> >  			ret = -ENOMEM;
> >  			/* registered subdevs are removed in fail_v4l2_dev */
> > @@ -109,7 +109,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
> >  		out->sd = sd;
> >  		out->cookie = conf->cookie;
> >  		mdev->output[mdev->output_cnt++] = out;
> > -		mxr_info(mdev, "added output '%s' from module '%s'\n",
> > +		dev_info(mdev->dev, "added output '%s' from module '%s'\n",
> >  			conf->output_name, conf->module_name);
> >  		/* checking if maximal number of outputs is reached */
> >  		if (mdev->output_cnt >= MXR_MAX_OUTPUTS)
> > @@ -117,7 +117,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
> >  	}
> >  
> >  	if (mdev->output_cnt == 0) {
> > -		mxr_err(mdev, "failed to register any output\n");
> > +		dev_err(mdev->dev, "failed to register any output\n");
> >  		ret = -ENODEV;
> >  		/* skipping fail_output because there is nothing to free */
> >  		goto fail_vb2_allocator;
> > @@ -160,7 +160,7 @@ static int mxr_querycap(struct file *file, void *priv,
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  
> >  	strlcpy(cap->driver, MXR_DRIVER_NAME, sizeof(cap->driver));
> >  	strlcpy(cap->card, layer->vfd.name, sizeof(cap->card));
> > @@ -173,19 +173,19 @@ static int mxr_querycap(struct file *file, void *priv,
> >  
> >  static void mxr_geometry_dump(struct mxr_device *mdev, struct mxr_geometry *geo)
> >  {
> > -	mxr_dbg(mdev, "src.full_size = (%u, %u)\n",
> > +	dev_dbg(mdev->dev, "src.full_size = (%u, %u)\n",
> >  		geo->src.full_width, geo->src.full_height);
> > -	mxr_dbg(mdev, "src.size = (%u, %u)\n",
> > +	dev_dbg(mdev->dev, "src.size = (%u, %u)\n",
> >  		geo->src.width, geo->src.height);
> > -	mxr_dbg(mdev, "src.offset = (%u, %u)\n",
> > +	dev_dbg(mdev->dev, "src.offset = (%u, %u)\n",
> >  		geo->src.x_offset, geo->src.y_offset);
> > -	mxr_dbg(mdev, "dst.full_size = (%u, %u)\n",
> > +	dev_dbg(mdev->dev, "dst.full_size = (%u, %u)\n",
> >  		geo->dst.full_width, geo->dst.full_height);
> > -	mxr_dbg(mdev, "dst.size = (%u, %u)\n",
> > +	dev_dbg(mdev->dev, "dst.size = (%u, %u)\n",
> >  		geo->dst.width, geo->dst.height);
> > -	mxr_dbg(mdev, "dst.offset = (%u, %u)\n",
> > +	dev_dbg(mdev->dev, "dst.offset = (%u, %u)\n",
> >  		geo->dst.x_offset, geo->dst.y_offset);
> > -	mxr_dbg(mdev, "ratio = (%u, %u)\n",
> > +	dev_dbg(mdev->dev, "ratio = (%u, %u)\n",
> >  		geo->x_ratio, geo->y_ratio);
> >  }
> >  
> > @@ -245,7 +245,7 @@ static int mxr_enum_fmt(struct file *file, void  *priv,
> >  	struct mxr_device *mdev = layer->mdev;
> >  	const struct mxr_format *fmt;
> >  
> > -	mxr_dbg(mdev, "%s\n", __func__);
> > +	dev_dbg(mdev->dev, "%s\n", __func__);
> >  	fmt = find_format_by_index(layer, f->index);
> >  	if (fmt == NULL)
> >  		return -EINVAL;
> > @@ -300,7 +300,7 @@ static int mxr_g_fmt(struct file *file, void *priv,
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  	struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  
> >  	pix->width = layer->geo.src.full_width;
> >  	pix->height = layer->geo.src.full_height;
> > @@ -321,12 +321,12 @@ static int mxr_s_fmt(struct file *file, void *priv,
> >  	struct mxr_device *mdev = layer->mdev;
> >  	struct mxr_geometry *geo = &layer->geo;
> >  
> > -	mxr_dbg(mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  
> >  	pix = &f->fmt.pix_mp;
> >  	fmt = find_format_by_fourcc(layer, pix->pixelformat);
> >  	if (fmt == NULL) {
> > -		mxr_warn(mdev, "not recognized fourcc: %08x\n",
> > +		dev_warn(mdev->dev, "not recognized fourcc: %08x\n",
> >  			pix->pixelformat);
> >  		return -EINVAL;
> >  	}
> > @@ -362,7 +362,7 @@ static int mxr_g_selection(struct file *file, void *fh,
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  	struct mxr_geometry *geo = &layer->geo;
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  
> >  	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> >  		s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> > @@ -429,7 +429,7 @@ static int mxr_s_selection(struct file *file, void *fh,
> >  
> >  	memset(&res, 0, sizeof(res));
> >  
> > -	mxr_dbg(layer->mdev, "%s: rect: %dx%d@%d,%d\n", __func__,
> > +	dev_dbg(layer->mdev->dev, "%s: rect: %dx%d@%d,%d\n", __func__,
> >  		s->r.width, s->r.height, s->r.left, s->r.top);
> >  
> >  	if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> > @@ -664,7 +664,7 @@ static int mxr_s_output(struct file *file, void *fh, unsigned int i)
> >  	/* update layers geometry */
> >  	mxr_layer_update_output(layer);
> >  
> > -	mxr_dbg(mdev, "tvnorms = %08llx\n", vfd->tvnorms);
> > +	dev_dbg(mdev->dev, "tvnorms = %08llx\n", vfd->tvnorms);
> >  
> >  	return 0;
> >  }
> > @@ -686,7 +686,7 @@ static int mxr_reqbufs(struct file *file, void *priv,
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	return vb2_reqbufs(&layer->vb_queue, p);
> >  }
> >  
> > @@ -694,7 +694,7 @@ static int mxr_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	return vb2_querybuf(&layer->vb_queue, p);
> >  }
> >  
> > @@ -702,7 +702,7 @@ static int mxr_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d(%d)\n", __func__, __LINE__, p->index);
> > +	dev_dbg(layer->mdev->dev, "%s:%d(%d)\n", __func__, __LINE__, p->index);
> >  	return vb2_qbuf(&layer->vb_queue, p);
> >  }
> >  
> > @@ -710,7 +710,7 @@ static int mxr_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	return vb2_dqbuf(&layer->vb_queue, p, file->f_flags & O_NONBLOCK);
> >  }
> >  
> > @@ -719,7 +719,7 @@ static int mxr_expbuf(struct file *file, void *priv,
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	return vb2_expbuf(&layer->vb_queue, eb);
> >  }
> >  
> > @@ -727,7 +727,7 @@ static int mxr_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	return vb2_streamon(&layer->vb_queue, i);
> >  }
> >  
> > @@ -735,7 +735,7 @@ static int mxr_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	return vb2_streamoff(&layer->vb_queue, i);
> >  }
> >  
> > @@ -777,7 +777,7 @@ static int mxr_video_open(struct file *file)
> >  	struct mxr_device *mdev = layer->mdev;
> >  	int ret = 0;
> >  
> > -	mxr_dbg(mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	if (mutex_lock_interruptible(&layer->mutex))
> >  		return -ERESTARTSYS;
> >  	/* assure device probe is finished */
> > @@ -785,7 +785,7 @@ static int mxr_video_open(struct file *file)
> >  	/* creating context for file descriptor */
> >  	ret = v4l2_fh_open(file);
> >  	if (ret) {
> > -		mxr_err(mdev, "v4l2_fh_open failed\n");
> > +		dev_err(mdev->dev, "v4l2_fh_open failed\n");
> >  		goto unlock;
> >  	}
> >  
> > @@ -796,13 +796,13 @@ static int mxr_video_open(struct file *file)
> >  	/* FIXME: should power be enabled on open? */
> >  	ret = mxr_power_get(mdev);
> >  	if (ret) {
> > -		mxr_err(mdev, "power on failed\n");
> > +		dev_err(mdev->dev, "power on failed\n");
> >  		goto fail_fh_open;
> >  	}
> >  
> >  	ret = vb2_queue_init(&layer->vb_queue);
> >  	if (ret != 0) {
> > -		mxr_err(mdev, "failed to initialize vb2 queue\n");
> > +		dev_err(mdev->dev, "failed to initialize vb2 queue\n");
> >  		goto fail_power;
> >  	}
> >  	/* set default format, first on the list */
> > @@ -831,7 +831,7 @@ mxr_video_poll(struct file *file, struct poll_table_struct *wait)
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  	unsigned int res;
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  
> >  	mutex_lock(&layer->mutex);
> >  	res = vb2_poll(&layer->vb_queue, file, wait);
> > @@ -844,7 +844,7 @@ static int mxr_video_mmap(struct file *file, struct vm_area_struct *vma)
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  	int ret;
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  
> >  	if (mutex_lock_interruptible(&layer->mutex))
> >  		return -ERESTARTSYS;
> > @@ -857,7 +857,7 @@ static int mxr_video_release(struct file *file)
> >  {
> >  	struct mxr_layer *layer = video_drvdata(file);
> >  
> > -	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
> > +	dev_dbg(layer->mdev->dev, "%s:%d\n", __func__, __LINE__);
> >  	mutex_lock(&layer->mutex);
> >  	if (v4l2_fh_is_singular_file(file)) {
> >  		vb2_queue_release(&layer->vb_queue);
> > @@ -887,11 +887,11 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
> >  	struct mxr_device *mdev = layer->mdev;
> >  	struct v4l2_plane_pix_format planes[3];
> >  
> > -	mxr_dbg(mdev, "%s\n", __func__);
> > +	dev_dbg(mdev->dev, "%s\n", __func__);
> >  	/* checking if format was configured */
> >  	if (fmt == NULL)
> >  		return -EINVAL;
> > -	mxr_dbg(mdev, "fmt = %s\n", fmt->name);
> > +	dev_dbg(mdev->dev, "fmt = %s\n", fmt->name);
> >  	mxr_mplane_fill(planes, fmt, layer->geo.src.full_width,
> >  		layer->geo.src.full_height);
> >  
> > @@ -899,7 +899,7 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *pfmt,
> >  	for (i = 0; i < fmt->num_subframes; ++i) {
> >  		alloc_ctxs[i] = layer->mdev->alloc_ctx;
> >  		sizes[i] = planes[i].sizeimage;
> > -		mxr_dbg(mdev, "size[%d] = %08x\n", i, sizes[i]);
> > +		dev_dbg(mdev->dev, "size[%d] = %08x\n", i, sizes[i]);
> >  	}
> >  
> >  	if (*nbuffers == 0)
> > @@ -919,14 +919,14 @@ static void buf_queue(struct vb2_buffer *vb)
> >  	list_add_tail(&buffer->list, &layer->enq_list);
> >  	spin_unlock_irqrestore(&layer->enq_slock, flags);
> >  
> > -	mxr_dbg(mdev, "queuing buffer\n");
> > +	dev_dbg(mdev->dev, "queuing buffer\n");
> >  }
> >  
> >  static void wait_lock(struct vb2_queue *vq)
> >  {
> >  	struct mxr_layer *layer = vb2_get_drv_priv(vq);
> >  
> > -	mxr_dbg(layer->mdev, "%s\n", __func__);
> > +	dev_dbg(layer->mdev->dev, "%s\n", __func__);
> >  	mutex_lock(&layer->mutex);
> >  }
> >  
> > @@ -934,7 +934,7 @@ static void wait_unlock(struct vb2_queue *vq)
> >  {
> >  	struct mxr_layer *layer = vb2_get_drv_priv(vq);
> >  
> > -	mxr_dbg(layer->mdev, "%s\n", __func__);
> > +	dev_dbg(layer->mdev->dev, "%s\n", __func__);
> >  	mutex_unlock(&layer->mutex);
> >  }
> >  
> > @@ -944,10 +944,10 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >  	struct mxr_device *mdev = layer->mdev;
> >  	unsigned long flags;
> >  
> > -	mxr_dbg(mdev, "%s\n", __func__);
> > +	dev_dbg(mdev->dev, "%s\n", __func__);
> >  
> >  	if (count == 0) {
> > -		mxr_dbg(mdev, "no output buffers queued\n");
> > +		dev_dbg(mdev->dev, "no output buffers queued\n");
> >  		return -EINVAL;
> >  	}
> >  
> > @@ -973,7 +973,7 @@ static void mxr_watchdog(unsigned long arg)
> >  	struct mxr_device *mdev = layer->mdev;
> >  	unsigned long flags;
> >  
> > -	mxr_err(mdev, "watchdog fired for layer %s\n", layer->vfd.name);
> > +	dev_err(mdev->dev, "watchdog fired for layer %s\n", layer->vfd.name);
> >  
> >  	spin_lock_irqsave(&layer->enq_slock, flags);
> >  
> > @@ -998,7 +998,7 @@ static int stop_streaming(struct vb2_queue *vq)
> >  	struct timer_list watchdog;
> >  	struct mxr_buffer *buf, *buf_tmp;
> >  
> > -	mxr_dbg(mdev, "%s\n", __func__);
> > +	dev_dbg(mdev->dev, "%s\n", __func__);
> >  
> >  	spin_lock_irqsave(&layer->enq_slock, flags);
> >  
> > @@ -1056,9 +1056,9 @@ int mxr_base_layer_register(struct mxr_layer *layer)
> >  
> >  	ret = video_register_device(&layer->vfd, VFL_TYPE_GRABBER, -1);
> >  	if (ret)
> > -		mxr_err(mdev, "failed to register video device\n");
> > +		dev_err(mdev->dev, "failed to register video device\n");
> >  	else
> > -		mxr_info(mdev, "registered layer %s as /dev/video%d\n",
> > +		dev_info(mdev->dev, "registered layer %s as /dev/video%d\n",
> >  			layer->vfd.name, layer->vfd.num);
> >  	return ret;
> >  }
> > @@ -1091,7 +1091,7 @@ struct mxr_layer *mxr_base_layer_create(struct mxr_device *mdev,
> >  
> >  	layer = kzalloc(sizeof(*layer), GFP_KERNEL);
> >  	if (layer == NULL) {
> > -		mxr_err(mdev, "not enough memory for layer.\n");
> > +		dev_err(mdev->dev, "not enough memory for layer.\n");
> >  		goto fail;
> >  	}
> >  
> > diff --git a/drivers/media/platform/s5p-tv/mixer_vp_layer.c b/drivers/media/platform/s5p-tv/mixer_vp_layer.c
> > index 3d13a63..38b216e 100644
> > --- a/drivers/media/platform/s5p-tv/mixer_vp_layer.c
> > +++ b/drivers/media/platform/s5p-tv/mixer_vp_layer.c
> > @@ -219,7 +219,7 @@ struct mxr_layer *mxr_vp_layer_create(struct mxr_device *mdev, int idx)
> >  
> >  	layer = mxr_base_layer_create(mdev, idx, name, &ops);
> >  	if (layer == NULL) {
> > -		mxr_err(mdev, "failed to initialize layer(%d) base\n", idx);
> > +		dev_err(mdev->dev, "failed to initialize layer(%d) base\n", idx);
> >  		goto fail;
> >  	}

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Joe Perches]

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.

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Tomasz Stanislawski]

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

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Bartlomiej Zolnierkiewicz]

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 you I know that you can handle patches effectively ;).
Lets also not forget that the mixer driver is almost dead when it comes to
new developments..

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

These sound like a good improvements.

> I ask Mateusz to modify the 'cleanup' patch to remove only useless
> mxr_dbg() and mxr_info() but to keep mxr_* macros intact.

As you wish, you're the maintainer of this driver and the mxr_* macros
issue is not worth anymore of my time..

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Joe Perches]

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

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Tomasz Stanislawski]

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 R&D 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.krawczuk@partner.samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@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(-)
>>>

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Bartlomiej Zolnierkiewicz]

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 R&D 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 R&D 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.krawczuk@partner.samsung.com>
> >>> Signed-off-by: Kyungmin Park <kyungmin.park@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(-)
> >>>

Re: [PATCH v5 1/4] media: s5p-tv: Replace mxr_ macro by default dev_

[Mauro Carvalho Chehab]

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

[PATCH v5 2/4] media: s5p-tv: Restore vpll clock rate

[Mateusz Krawczuk]

Restore vpll clock rate if start stream fail or stream is off.

Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/sdo_drv.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/sdo_drv.c b/drivers/media/platform/s5p-tv/sdo_drv.c
index 0afa90f..e49ac6c 100644
--- a/drivers/media/platform/s5p-tv/sdo_drv.c
+++ b/drivers/media/platform/s5p-tv/sdo_drv.c
@@ -55,6 +55,8 @@ struct sdo_device {
 	struct clk *dacphy;
 	/** clock for control of VPLL */
 	struct clk *fout_vpll;
+	/** vpll rate before sdo stream was on */
+	unsigned long vpll_rate;
 	/** regulator for SDO IP power */
 	struct regulator *vdac;
 	/** regulator for SDO plug detection */
@@ -193,17 +195,33 @@ static int sdo_s_power(struct v4l2_subdev *sd, int on)
 
 static int sdo_streamon(struct sdo_device *sdev)
 {
+	int ret;
+
 	/* set proper clock for Timing Generator */
-	clk_set_rate(sdev->fout_vpll, 54000000);
+	sdev->vpll_rate = clk_get_rate(sdev->fout_vpll);
+	ret = clk_set_rate(sdev->fout_vpll, 54000000);
+	if (ret < 0) {
+		dev_err(sdev->dev, "Failed to set vpll rate\n");
+		return ret;
+	}
 	dev_info(sdev->dev, "fout_vpll.rate = %lu\n",
 	clk_get_rate(sdev->fout_vpll));
 	/* enable clock in SDO */
 	sdo_write_mask(sdev, SDO_CLKCON, ~0, SDO_TVOUT_CLOCK_ON);
-	clk_enable(sdev->dacphy);
+	ret = clk_enable(sdev->dacphy);
+	if (ret < 0) {
+		dev_err(sdev->dev, "clk_enable(dacphy) failed\n");
+		goto fail;
+	}
 	/* enable DAC */
 	sdo_write_mask(sdev, SDO_DAC, ~0, SDO_POWER_ON_DAC);
 	sdo_reg_debug(sdev);
 	return 0;
+	
+fail:
+	sdo_write_mask(sdev, SDO_CLKCON, 0, SDO_TVOUT_CLOCK_ON);
+	clk_set_rate(sdev->fout_vpll, sdev->vpll_rate);
+	return ret;
 }
 
 static int sdo_streamoff(struct sdo_device *sdev)
@@ -220,6 +238,7 @@ static int sdo_streamoff(struct sdo_device *sdev)
 	}
 	if (tries == 0)
 		dev_err(sdev->dev, "failed to stop streaming\n");
+	clk_set_rate(sdev->fout_vpll, sdev->vpll_rate);
 	return tries ? 0 : -EIO;
 }
 
-- 
1.8.1.2

Re: [PATCH v5 2/4] media: s5p-tv: Restore vpll clock rate

[Tomasz Stanislawski]

Hi,

As you can see sdo, hdmi and mixer are saparate drivers that are
parts of s5p-tv drivers set. Could you rename commit name to
'media: s5p-tv: sdo: Restore vpll clock rate after streamoff'


On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote:
> Restore vpll clock rate if start stream fail or stream is off.
> 
> Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/platform/s5p-tv/sdo_drv.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-tv/sdo_drv.c b/drivers/media/platform/s5p-tv/sdo_drv.c
> index 0afa90f..e49ac6c 100644
> --- a/drivers/media/platform/s5p-tv/sdo_drv.c
> +++ b/drivers/media/platform/s5p-tv/sdo_drv.c
> @@ -55,6 +55,8 @@ struct sdo_device {
>  	struct clk *dacphy;
>  	/** clock for control of VPLL */
>  	struct clk *fout_vpll;
> +	/** vpll rate before sdo stream was on */
> +	unsigned long vpll_rate;
>  	/** regulator for SDO IP power */
>  	struct regulator *vdac;
>  	/** regulator for SDO plug detection */
> @@ -193,17 +195,33 @@ static int sdo_s_power(struct v4l2_subdev *sd, int on)
>  
>  static int sdo_streamon(struct sdo_device *sdev)
>  {
> +	int ret;
> +
>  	/* set proper clock for Timing Generator */
> -	clk_set_rate(sdev->fout_vpll, 54000000);
> +	sdev->vpll_rate = clk_get_rate(sdev->fout_vpll);
> +	ret = clk_set_rate(sdev->fout_vpll, 54000000);
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "Failed to set vpll rate\n");
> +		return ret;
> +	}
>  	dev_info(sdev->dev, "fout_vpll.rate = %lu\n",

Could you also remove the line below in a new 'sdo: cleanup' patch?
Change dev_info to dev_dbg in the line above.

>  	clk_get_rate(sdev->fout_vpll));
>  	/* enable clock in SDO */
>  	sdo_write_mask(sdev, SDO_CLKCON, ~0, SDO_TVOUT_CLOCK_ON);
> -	clk_enable(sdev->dacphy);
> +	ret = clk_enable(sdev->dacphy);
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "clk_enable(dacphy) failed\n");
> +		goto fail;
> +	}
>  	/* enable DAC */
>  	sdo_write_mask(sdev, SDO_DAC, ~0, SDO_POWER_ON_DAC);
>  	sdo_reg_debug(sdev);
>  	return 0;
> +	
> +fail:
> +	sdo_write_mask(sdev, SDO_CLKCON, 0, SDO_TVOUT_CLOCK_ON);
> +	clk_set_rate(sdev->fout_vpll, sdev->vpll_rate);
> +	return ret;
>  }
>  
>  static int sdo_streamoff(struct sdo_device *sdev)
> @@ -220,6 +238,7 @@ static int sdo_streamoff(struct sdo_device *sdev)
>  	}
>  	if (tries == 0)
>  		dev_err(sdev->dev, "failed to stop streaming\n");

Produce some warning if the line below fails.

> +	clk_set_rate(sdev->fout_vpll, sdev->vpll_rate);
>  	return tries ? 0 : -EIO;
>  }
>  
> 

Re: [PATCH v5 2/4] media: s5p-tv: Restore vpll clock rate

[Sylwester Nawrocki]

On 09/25/2013 05:46 PM, Tomasz Stanislawski wrote:
> Hi,
>
> As you can see sdo, hdmi and mixer are saparate drivers that are
> parts of s5p-tv drivers set. Could you rename commit name to
> 'media: s5p-tv: sdo: Restore vpll clock rate after streamoff'

Patch applied with the above commit summary line and checkpatch error
as below fixed. Please always run scripts/checkpatch.pl before submitting
patches.

$ scripts/checkpatch.pl v5-2-4-media-s5p-tv-Restore-vpll-clock-rate.patch
ERROR: trailing whitespace
#67: FILE: drivers/media/platform/s5p-tv/sdo_drv.c:220:
+^I$

total: 1 errors, 0 warnings, 50 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
       scripts/cleanfile

v5-2-4-media-s5p-tv-Restore-vpll-clock-rate.patch has style problems, 
please review.

If any of these errors are false positives, please report

--
Thanks,
Sylwester

[PATCH v5 3/4] media: s5p-tv: Fix sdo driver to work with CCF

[Mateusz Krawczuk]

Replace clk_enable by clock_enable_prepare and clk_disable with clk_disable_unprepare.
Clock prepare is required by Clock Common Framework, and old clock driver didn`t support it.
Without it Common Clock Framework prints a warning.

Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/sdo_drv.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/sdo_drv.c b/drivers/media/platform/s5p-tv/sdo_drv.c
index e49ac6c..17272e1 100644
--- a/drivers/media/platform/s5p-tv/sdo_drv.c
+++ b/drivers/media/platform/s5p-tv/sdo_drv.c
@@ -208,9 +208,9 @@ static int sdo_streamon(struct sdo_device *sdev)
 	clk_get_rate(sdev->fout_vpll));
 	/* enable clock in SDO */
 	sdo_write_mask(sdev, SDO_CLKCON, ~0, SDO_TVOUT_CLOCK_ON);
-	ret = clk_enable(sdev->dacphy);
+	ret = clk_prepare_enable(sdev->dacphy);
 	if (ret < 0) {
-		dev_err(sdev->dev, "clk_enable(dacphy) failed\n");
+		dev_err(sdev->dev, "clk_prepare_enable(dacphy) failed\n");
 		goto fail;
 	}
 	/* enable DAC */
@@ -229,7 +229,7 @@ static int sdo_streamoff(struct sdo_device *sdev)
 	int tries;
 
 	sdo_write_mask(sdev, SDO_DAC, 0, SDO_POWER_ON_DAC);
-	clk_disable(sdev->dacphy);
+	clk_disable_unprepare(sdev->dacphy);
 	sdo_write_mask(sdev, SDO_CLKCON, 0, SDO_TVOUT_CLOCK_ON);
 	for (tries = 100; tries; --tries) {
 		if (sdo_read(sdev, SDO_CLKCON) & SDO_TVOUT_CLOCK_READY)
@@ -273,7 +273,7 @@ static int sdo_runtime_suspend(struct device *dev)
 	dev_info(dev, "suspend\n");
 	regulator_disable(sdev->vdet);
 	regulator_disable(sdev->vdac);
-	clk_disable(sdev->sclk_dac);
+	clk_disable_unprepare(sdev->sclk_dac);
 	return 0;
 }
 
@@ -285,7 +285,7 @@ static int sdo_runtime_resume(struct device *dev)
 
 	dev_info(dev, "resume\n");
 
-	ret = clk_enable(sdev->sclk_dac);
+	ret = clk_prepare_enable(sdev->sclk_dac);
 	if (ret < 0)
 		return ret;
 
@@ -318,7 +318,7 @@ static int sdo_runtime_resume(struct device *dev)
 vdac_r_dis:
 	regulator_disable(sdev->vdac);
 dac_clk_dis:
-	clk_disable(sdev->sclk_dac);
+	clk_disable_unprepare(sdev->sclk_dac);
 	return ret;
 }
 
@@ -424,7 +424,11 @@ static int sdo_probe(struct platform_device *pdev)
 	}
 
 	/* enable gate for dac clock, because mixer uses it */
-	clk_enable(sdev->dac);
+	ret = clk_prepare_enable(sdev->dac);
+	if (ret < 0) {
+		dev_err(dev, "clk_prepare_enable(dac) failed\n");
+		goto fail_fout_vpll;
+	}
 
 	/* configure power management */
 	pm_runtime_enable(dev);
@@ -463,7 +467,7 @@ static int sdo_remove(struct platform_device *pdev)
 	struct sdo_device *sdev = sd_to_sdev(sd);
 
 	pm_runtime_disable(&pdev->dev);
-	clk_disable(sdev->dac);
+	clk_disable_unprepare(sdev->dac);
 	clk_put(sdev->fout_vpll);
 	clk_put(sdev->dacphy);
 	clk_put(sdev->dac);
-- 
1.8.1.2

Re: [PATCH v5 3/4] media: s5p-tv: Fix sdo driver to work with CCF

[Tomasz Stanislawski]

Rename to 'media: s5p-tv: sdo: integrate with CCF'

On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote:
> Replace clk_enable by clock_enable_prepare and clk_disable with clk_disable_unprepare.
> Clock prepare is required by Clock Common Framework, and old clock driver didn`t support it.
> Without it Common Clock Framework prints a warning.
> 
> Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/platform/s5p-tv/sdo_drv.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-tv/sdo_drv.c b/drivers/media/platform/s5p-tv/sdo_drv.c
> index e49ac6c..17272e1 100644
> --- a/drivers/media/platform/s5p-tv/sdo_drv.c
> +++ b/drivers/media/platform/s5p-tv/sdo_drv.c
> @@ -208,9 +208,9 @@ static int sdo_streamon(struct sdo_device *sdev)
>  	clk_get_rate(sdev->fout_vpll));
>  	/* enable clock in SDO */
>  	sdo_write_mask(sdev, SDO_CLKCON, ~0, SDO_TVOUT_CLOCK_ON);
> -	ret = clk_enable(sdev->dacphy);
> +	ret = clk_prepare_enable(sdev->dacphy);
>  	if (ret < 0) {
> -		dev_err(sdev->dev, "clk_enable(dacphy) failed\n");
> +		dev_err(sdev->dev, "clk_prepare_enable(dacphy) failed\n");
>  		goto fail;
>  	}
>  	/* enable DAC */
> @@ -229,7 +229,7 @@ static int sdo_streamoff(struct sdo_device *sdev)
>  	int tries;
>  
>  	sdo_write_mask(sdev, SDO_DAC, 0, SDO_POWER_ON_DAC);
> -	clk_disable(sdev->dacphy);
> +	clk_disable_unprepare(sdev->dacphy);
>  	sdo_write_mask(sdev, SDO_CLKCON, 0, SDO_TVOUT_CLOCK_ON);
>  	for (tries = 100; tries; --tries) {
>  		if (sdo_read(sdev, SDO_CLKCON) & SDO_TVOUT_CLOCK_READY)
> @@ -273,7 +273,7 @@ static int sdo_runtime_suspend(struct device *dev)
>  	dev_info(dev, "suspend\n");
>  	regulator_disable(sdev->vdet);
>  	regulator_disable(sdev->vdac);
> -	clk_disable(sdev->sclk_dac);
> +	clk_disable_unprepare(sdev->sclk_dac);
>  	return 0;
>  }
>  
> @@ -285,7 +285,7 @@ static int sdo_runtime_resume(struct device *dev)
>  
>  	dev_info(dev, "resume\n");
>  
> -	ret = clk_enable(sdev->sclk_dac);
> +	ret = clk_prepare_enable(sdev->sclk_dac);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -318,7 +318,7 @@ static int sdo_runtime_resume(struct device *dev)
>  vdac_r_dis:
>  	regulator_disable(sdev->vdac);
>  dac_clk_dis:
> -	clk_disable(sdev->sclk_dac);
> +	clk_disable_unprepare(sdev->sclk_dac);
>  	return ret;
>  }
>  
> @@ -424,7 +424,11 @@ static int sdo_probe(struct platform_device *pdev)
>  	}
>  
>  	/* enable gate for dac clock, because mixer uses it */
> -	clk_enable(sdev->dac);
> +	ret = clk_prepare_enable(sdev->dac);
> +	if (ret < 0) {
> +		dev_err(dev, "clk_prepare_enable(dac) failed\n");
> +		goto fail_fout_vpll;
> +	}
>  
>  	/* configure power management */
>  	pm_runtime_enable(dev);
> @@ -463,7 +467,7 @@ static int sdo_remove(struct platform_device *pdev)
>  	struct sdo_device *sdev = sd_to_sdev(sd);
>  
>  	pm_runtime_disable(&pdev->dev);
> -	clk_disable(sdev->dac);
> +	clk_disable_unprepare(sdev->dac);
>  	clk_put(sdev->fout_vpll);
>  	clk_put(sdev->dacphy);
>  	clk_put(sdev->dac);
> 

Re: [PATCH v5 3/4] media: s5p-tv: Fix sdo driver to work with CCF

[Sylwester Nawrocki]

On 09/25/2013 05:59 PM, Tomasz Stanislawski wrote:
> Rename to 'media: s5p-tv: sdo: integrate with CCF'
>
> On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote:
>> Replace clk_enable by clock_enable_prepare and clk_disable with clk_disable_unprepare.
>> Clock prepare is required by Clock Common Framework, and old clock driver didn`t support it.
>> Without it Common Clock Framework prints a warning.
>>
>> Signed-off-by: Mateusz Krawczuk<m.krawczuk@partner.samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>

Patch applied with following commit log:

commit 526ec3cc57a0751ff087e93acd1566e6d063fb17
Author: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
Date:   Sat Sep 21 14:00:49 2013 +0000

     s5p-tv: mixer: Prepare for common clock framework

     Replace clk_enable() by clock_enable_prepare() and clk_disable()
     with clk_disable_unprepare(). clk_{prepare/unprepare} calls are
     required by common clock framework and this driver was missed while
     converting all users of the Samsung original clocks driver to its
     new implementation based on the common clock API.

     Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
     Acked-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
     [s.nawrocki@samsung.com: edited commit description]
     Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

--
Regards,
Sylwester

[PATCH v5 4/4] media: s5p-tv: Fix mixer driver to work with CCF

[Mateusz Krawczuk]

Replace clk_enable by clock_enable_prepare and clk_disable with clk_disable_unprepare.
Clock prepare is required by Clock Common Framework, and old clock driver didn`t support it.
Without it Common Clock Framework prints a warning.

Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/mixer_drv.c | 34 +++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c b/drivers/media/platform/s5p-tv/mixer_drv.c
index 8ce7c3e..7eea286 100644
--- a/drivers/media/platform/s5p-tv/mixer_drv.c
+++ b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -347,19 +347,41 @@ static int mxr_runtime_resume(struct device *dev)
 {
 	struct mxr_device *mdev = to_mdev(dev);
 	struct mxr_resources *res = &mdev->res;
+	int ret;
 
 	dev_dbg(mdev->dev, "resume - start\n");
 	mutex_lock(&mdev->mutex);
 	/* turn clocks on */
-	clk_enable(res->mixer);
-	clk_enable(res->vp);
-	clk_enable(res->sclk_mixer);
+	ret = clk_prepare_enable(res->mixer);
+	if (ret < 0) {
+		dev_err(mdev->dev, "clk_prepare_enable(mixer) failed\n");
+		goto fail;
+	}
+	ret = clk_prepare_enable(res->vp);
+	if (ret < 0) {
+		dev_err(mdev->dev, "clk_prepare_enable(vp) failed\n");
+		goto fail_mixer;
+	}
+	ret = clk_prepare_enable(res->sclk_mixer);
+	if (ret < 0) {
+		dev_err(mdev->dev, "clk_prepare_enable(sclk_mixer) failed\n");
+		goto fail_vp;
+	}
 	/* apply default configuration */
 	mxr_reg_reset(mdev);
 	dev_dbg(mdev->dev, "resume - finished\n");
 
 	mutex_unlock(&mdev->mutex);
 	return 0;
+
+fail_vp:
+	clk_disable_unprepare(res->vp);
+fail_mixer:
+	clk_disable_unprepare(res->mixer);
+fail:
+	mutex_unlock(&mdev->mutex);
+	dev_err(mdev->dev, "resume failed\n");
+	return ret;
 }
 
 static int mxr_runtime_suspend(struct device *dev)
@@ -369,9 +391,9 @@ static int mxr_runtime_suspend(struct device *dev)
 	dev_dbg(mdev->dev, "suspend - start\n");
 	mutex_lock(&mdev->mutex);
 	/* turn clocks off */
-	clk_disable(res->sclk_mixer);
-	clk_disable(res->vp);
-	clk_disable(res->mixer);
+	clk_disable_unprepare(res->sclk_mixer);
+	clk_disable_unprepare(res->vp);
+	clk_disable_unprepare(res->mixer);
 	mutex_unlock(&mdev->mutex);
 	dev_dbg(mdev->dev, "suspend - finished\n");
 	return 0;
-- 
1.8.1.2

Re: [PATCH v5 4/4] media: s5p-tv: Fix mixer driver to work with CCF

[Tomasz Stanislawski]

rename to 'media: s5p-tv: mixer: integrate with CCF'

On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote:
> Replace clk_enable by clock_enable_prepare and clk_disable with clk_disable_unprepare.
> Clock prepare is required by Clock Common Framework, and old clock driver didn`t support it.
> Without it Common Clock Framework prints a warning.
> 
> Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Tomasz Stanislawski <t.stanislaws@samsung.com>

> ---
>  drivers/media/platform/s5p-tv/mixer_drv.c | 34 +++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c b/drivers/media/platform/s5p-tv/mixer_drv.c
> index 8ce7c3e..7eea286 100644
> --- a/drivers/media/platform/s5p-tv/mixer_drv.c
> +++ b/drivers/media/platform/s5p-tv/mixer_drv.c
> @@ -347,19 +347,41 @@ static int mxr_runtime_resume(struct device *dev)
>  {
>  	struct mxr_device *mdev = to_mdev(dev);
>  	struct mxr_resources *res = &mdev->res;
> +	int ret;
>  
>  	dev_dbg(mdev->dev, "resume - start\n");
>  	mutex_lock(&mdev->mutex);
>  	/* turn clocks on */
> -	clk_enable(res->mixer);
> -	clk_enable(res->vp);
> -	clk_enable(res->sclk_mixer);
> +	ret = clk_prepare_enable(res->mixer);
> +	if (ret < 0) {
> +		dev_err(mdev->dev, "clk_prepare_enable(mixer) failed\n");
> +		goto fail;
> +	}
> +	ret = clk_prepare_enable(res->vp);
> +	if (ret < 0) {
> +		dev_err(mdev->dev, "clk_prepare_enable(vp) failed\n");
> +		goto fail_mixer;
> +	}
> +	ret = clk_prepare_enable(res->sclk_mixer);
> +	if (ret < 0) {
> +		dev_err(mdev->dev, "clk_prepare_enable(sclk_mixer) failed\n");
> +		goto fail_vp;
> +	}
>  	/* apply default configuration */
>  	mxr_reg_reset(mdev);
>  	dev_dbg(mdev->dev, "resume - finished\n");
>  
>  	mutex_unlock(&mdev->mutex);
>  	return 0;
> +
> +fail_vp:
> +	clk_disable_unprepare(res->vp);
> +fail_mixer:
> +	clk_disable_unprepare(res->mixer);
> +fail:
> +	mutex_unlock(&mdev->mutex);
> +	dev_err(mdev->dev, "resume failed\n");
> +	return ret;
>  }
>  
>  static int mxr_runtime_suspend(struct device *dev)
> @@ -369,9 +391,9 @@ static int mxr_runtime_suspend(struct device *dev)
>  	dev_dbg(mdev->dev, "suspend - start\n");
>  	mutex_lock(&mdev->mutex);
>  	/* turn clocks off */
> -	clk_disable(res->sclk_mixer);
> -	clk_disable(res->vp);
> -	clk_disable(res->mixer);
> +	clk_disable_unprepare(res->sclk_mixer);
> +	clk_disable_unprepare(res->vp);
> +	clk_disable_unprepare(res->mixer);
>  	mutex_unlock(&mdev->mutex);
>  	dev_dbg(mdev->dev, "suspend - finished\n");
>  	return 0;
> 

Re: [PATCH v5 4/4] media: s5p-tv: Fix mixer driver to work with CCF

[Sylwester Nawrocki]

On 09/25/2013 06:09 PM, Tomasz Stanislawski wrote:
> rename to 'media: s5p-tv: mixer: integrate with CCF'
>
> On 09/21/2013 05:00 PM, Mateusz Krawczuk wrote:
>> >  Replace clk_enable by clock_enable_prepare and clk_disable with clk_disable_unprepare.
>> >  Clock prepare is required by Clock Common Framework, and old clock driver didn`t support it.
>> >  Without it Common Clock Framework prints a warning.
>> >
>> >  Signed-off-by: Mateusz Krawczuk<m.krawczuk@partner.samsung.com>
>> >  Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>
> Acked-by: Tomasz Stanislawski<t.stanislaws@samsung.com>

Patch applied with following commit log:

commit 526ec3cc57a0751ff087e93acd1566e6d063fb17
Author: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
Date:   Sat Sep 21 14:00:49 2013 +0000

     s5p-tv: mixer: Prepare for common clock framework

     Replace clk_enable() by clock_enable_prepare() and clk_disable()
     with clk_disable_unprepare(). clk_{prepare/unprepare} calls are
     required by common clock framework and this driver was missed while
     converting all users of the Samsung original clocks driver to its
     new implementation based on the common clock API.

     Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
     Acked-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
     [s.nawrocki@samsung.com: edited commit description]
     Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

--
Regards,
Sylwester

Re: [PATCH v5 0/4] media: s5p-tv: clean-up and fixes

[Sachin Kamat]

Hi Mateusz,

On 21 September 2013 20:30, Mateusz Krawczuk
<m.krawczuk@partner.samsung.com> wrote:
> This patch series add restoring previous vpll rate after driver offs stream
> or recives error.
> It also replace mxr_info, mxr_dbg, mxr_warn and mxr_err macro
> by generic solution.

It is a good practice to include revision history of the changes in
the patch set. This will help reviewers
check if the changes previously suggested have been incorporated or
not and to identify any new changes introduced by the author.


-- 
With warm regards,
Sachin

Re: [PATCH v5 0/4] media: s5p-tv: clean-up and fixes

[Tomasz Figa]

Hi Mateusz,

On Saturday 21 of September 2013 17:00:45 Mateusz Krawczuk wrote:
> This patch series add restoring previous vpll rate after driver offs
> stream or recives error.
> It also replace mxr_info, mxr_dbg, mxr_warn and mxr_err macro
> by generic solution.
> 
> Mateusz Krawczuk (4):
>   media: s5p-tv: Replace mxr_ macro by default dev_
>   media: s5p-tv: Restore vpll clock rate
>   media: s5p-tv: Fix sdo driver to work with CCF
>   media: s5p-tv: Fix mixer driver to work with CCF
> 
>  drivers/media/platform/s5p-tv/mixer.h           |  12 ---
>  drivers/media/platform/s5p-tv/mixer_drv.c       |  81
> ++++++++++++------- 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 +-
>  drivers/media/platform/s5p-tv/sdo_drv.c         |  39 +++++++--
>  7 files changed, 137 insertions(+), 105 deletions(-)

For the whole series:

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz