[REVIEW PATCH 0/6] s5p-tv: replace dv_preset by dv_timings

[REVIEW PATCH 0/6] s5p-tv: replace dv_preset by dv_timings

[Hans Verkuil]

Hi Tomasz,

Here is what I hope is the final patch series for this. I've incorporated
your suggestions and it's split off from the davinci/blackfin changes into
its own patch series to keep things better organized.

The changes since the previous version are:

- changed the order of the first three patches as per your suggestion.
- the patch named "[RFC PATCH 08/18] s5p-tv: add dv_timings support for
  mixer_video." had two changes that rightfully belonged to the 'add
  dv_timings support for mixer_video.' patch. Moved them accordingly.
- hdmiphy now also supports dv_timings_cap and sets the pixelclock range
  accordingly. The hdmi driver chains hdmiphy to get those values.
- updating the minimum width to 720.

I didn't add a comment to clarify the pixclk handling hdmiphy_s_dv_preset
because 1) I forgot, 2) it's not a bug, and 3) that whole function is
removed anyway a few patches later :-)

The only functional change is the handling of dv_timings_cap. Can you
verify that that works as it should?

Thanks!

	Hans

[REVIEW PATCH 1/6] s5p-tv: add dv_timings support for hdmiphy.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This just adds dv_timings support without modifying existing dv_preset
support, although I had to refactor a little bit in order to share
hdmiphy_find_conf() between the preset and timings code.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/hdmiphy_drv.c |   60 +++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/hdmiphy_drv.c b/drivers/media/platform/s5p-tv/hdmiphy_drv.c
index 80717ce..ef0d812 100644
--- a/drivers/media/platform/s5p-tv/hdmiphy_drv.c
+++ b/drivers/media/platform/s5p-tv/hdmiphy_drv.c
@@ -197,14 +197,9 @@ static unsigned long hdmiphy_preset_to_pixclk(u32 preset)
 		return 0;
 }
 
-static const u8 *hdmiphy_find_conf(u32 preset, const struct hdmiphy_conf *conf)
+static const u8 *hdmiphy_find_conf(unsigned long pixclk,
+		const struct hdmiphy_conf *conf)
 {
-	unsigned long pixclk;
-
-	pixclk = hdmiphy_preset_to_pixclk(preset);
-	if (!pixclk)
-		return NULL;
-
 	for (; conf->pixclk; ++conf)
 		if (conf->pixclk == pixclk)
 			return conf->data;
@@ -220,15 +215,49 @@ static int hdmiphy_s_power(struct v4l2_subdev *sd, int on)
 static int hdmiphy_s_dv_preset(struct v4l2_subdev *sd,
 	struct v4l2_dv_preset *preset)
 {
-	const u8 *data;
+	const u8 *data = NULL;
 	u8 buffer[32];
 	int ret;
 	struct hdmiphy_ctx *ctx = sd_to_ctx(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	unsigned long pixclk;
 	struct device *dev = &client->dev;
 
 	dev_info(dev, "s_dv_preset(preset = %d)\n", preset->preset);
-	data = hdmiphy_find_conf(preset->preset, ctx->conf_tab);
+
+	pixclk = hdmiphy_preset_to_pixclk(preset->preset);
+	data = hdmiphy_find_conf(pixclk, ctx->conf_tab);
+	if (!data) {
+		dev_err(dev, "format not supported\n");
+		return -EINVAL;
+	}
+
+	/* storing configuration to the device */
+	memcpy(buffer, data, 32);
+	ret = i2c_master_send(client, buffer, 32);
+	if (ret != 32) {
+		dev_err(dev, "failed to configure HDMIPHY via I2C\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int hdmiphy_s_dv_timings(struct v4l2_subdev *sd,
+	struct v4l2_dv_timings *timings)
+{
+	const u8 *data;
+	u8 buffer[32];
+	int ret;
+	struct hdmiphy_ctx *ctx = sd_to_ctx(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct device *dev = &client->dev;
+	unsigned long pixclk = timings->bt.pixelclock;
+
+	dev_info(dev, "s_dv_timings\n");
+	if ((timings->bt.flags & V4L2_DV_FL_REDUCED_FPS) && pixclk == 74250000)
+		pixclk = 74176000;
+	data = hdmiphy_find_conf(pixclk, ctx->conf_tab);
 	if (!data) {
 		dev_err(dev, "format not supported\n");
 		return -EINVAL;
@@ -245,6 +274,17 @@ static int hdmiphy_s_dv_preset(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int hdmiphy_dv_timings_cap(struct v4l2_subdev *sd,
+	struct v4l2_dv_timings_cap *cap)
+{
+	cap->type = V4L2_DV_BT_656_1120;
+	/* The phy only determines the pixelclock, leave the other values
+	 * at 0 to signify that we have no information for them. */
+	cap->bt.min_pixelclock = 27000000;
+	cap->bt.max_pixelclock = 148500000;
+	return 0;
+}
+
 static int hdmiphy_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -271,6 +311,8 @@ static const struct v4l2_subdev_core_ops hdmiphy_core_ops = {
 
 static const struct v4l2_subdev_video_ops hdmiphy_video_ops = {
 	.s_dv_preset = hdmiphy_s_dv_preset,
+	.s_dv_timings = hdmiphy_s_dv_timings,
+	.dv_timings_cap = hdmiphy_dv_timings_cap,
 	.s_stream =  hdmiphy_s_stream,
 };
 
-- 
1.7.10.4

[REVIEW PATCH 2/6] s5p-tv: add dv_timings support for hdmi.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This just adds dv_timings support without modifying existing dv_preset
support.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/hdmi_drv.c |   94 +++++++++++++++++++++++++-----
 1 file changed, 81 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/hdmi_drv.c b/drivers/media/platform/s5p-tv/hdmi_drv.c
index 8de1b3d..1376577 100644
--- a/drivers/media/platform/s5p-tv/hdmi_drv.c
+++ b/drivers/media/platform/s5p-tv/hdmi_drv.c
@@ -31,6 +31,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/regulator/consumer.h>
+#include <linux/v4l2-dv-timings.h>
 
 #include <media/s5p_hdmi.h>
 #include <media/v4l2-common.h>
@@ -93,6 +94,8 @@ struct hdmi_device {
 	int cur_conf_dirty;
 	/** current preset */
 	u32 cur_preset;
+	/** current timings */
+	struct v4l2_dv_timings cur_timings;
 	/** other resources */
 	struct hdmi_resources res;
 };
@@ -477,19 +480,21 @@ static const struct hdmi_timings hdmi_timings_1080p50 = {
 
 static const struct {
 	u32 preset;
-	const struct hdmi_timings *timings;
+	bool reduced_fps;
+	const struct v4l2_dv_timings dv_timings;
+	const struct hdmi_timings *hdmi_timings;
 } hdmi_timings[] = {
-	{ V4L2_DV_480P59_94, &hdmi_timings_480p },
-	{ V4L2_DV_576P50, &hdmi_timings_576p50 },
-	{ V4L2_DV_720P50, &hdmi_timings_720p50 },
-	{ V4L2_DV_720P59_94, &hdmi_timings_720p60 },
-	{ V4L2_DV_720P60, &hdmi_timings_720p60 },
-	{ V4L2_DV_1080P24, &hdmi_timings_1080p24 },
-	{ V4L2_DV_1080P30, &hdmi_timings_1080p60 },
-	{ V4L2_DV_1080P50, &hdmi_timings_1080p50 },
-	{ V4L2_DV_1080I50, &hdmi_timings_1080i50 },
-	{ V4L2_DV_1080I60, &hdmi_timings_1080i60 },
-	{ V4L2_DV_1080P60, &hdmi_timings_1080p60 },
+	{ V4L2_DV_480P59_94, false, V4L2_DV_BT_CEA_720X480P59_94, &hdmi_timings_480p },
+	{ V4L2_DV_576P50, false, V4L2_DV_BT_CEA_720X576P50, &hdmi_timings_576p50 },
+	{ V4L2_DV_720P50, false, V4L2_DV_BT_CEA_1280X720P50, &hdmi_timings_720p50 },
+	{ V4L2_DV_720P59_94, true, V4L2_DV_BT_CEA_1280X720P60, &hdmi_timings_720p60 },
+	{ V4L2_DV_720P60, false, V4L2_DV_BT_CEA_1280X720P60, &hdmi_timings_720p60 },
+	{ V4L2_DV_1080P24, false, V4L2_DV_BT_CEA_1920X1080P24, &hdmi_timings_1080p24 },
+	{ V4L2_DV_1080P30, false, V4L2_DV_BT_CEA_1920X1080P30, &hdmi_timings_1080p60 },
+	{ V4L2_DV_1080P50, false, V4L2_DV_BT_CEA_1920X1080P50, &hdmi_timings_1080p50 },
+	{ V4L2_DV_1080I50, false, V4L2_DV_BT_CEA_1920X1080I50, &hdmi_timings_1080i50 },
+	{ V4L2_DV_1080I60, false, V4L2_DV_BT_CEA_1920X1080I60, &hdmi_timings_1080i60 },
+	{ V4L2_DV_1080P60, false, V4L2_DV_BT_CEA_1920X1080P60, &hdmi_timings_1080p60 },
 };
 
 static const struct hdmi_timings *hdmi_preset2timings(u32 preset)
@@ -498,7 +503,7 @@ static const struct hdmi_timings *hdmi_preset2timings(u32 preset)
 
 	for (i = 0; i < ARRAY_SIZE(hdmi_timings); ++i)
 		if (hdmi_timings[i].preset == preset)
-			return  hdmi_timings[i].timings;
+			return  hdmi_timings[i].hdmi_timings;
 	return NULL;
 }
 
@@ -647,6 +652,36 @@ static int hdmi_g_dv_preset(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int hdmi_s_dv_timings(struct v4l2_subdev *sd,
+	struct v4l2_dv_timings *timings)
+{
+	struct hdmi_device *hdev = sd_to_hdmi_dev(sd);
+	struct device *dev = hdev->dev;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(hdmi_timings); i++)
+		if (v4l_match_dv_timings(&hdmi_timings[i].dv_timings,
+					timings, 0))
+			break;
+	if (i == ARRAY_SIZE(hdmi_timings)) {
+		dev_err(dev, "timings not supported\n");
+		return -EINVAL;
+	}
+	hdev->cur_conf = hdmi_timings[i].hdmi_timings;
+	hdev->cur_conf_dirty = 1;
+	hdev->cur_timings = *timings;
+	if (!hdmi_timings[i].reduced_fps)
+		hdev->cur_timings.bt.flags &= ~V4L2_DV_FL_CAN_REDUCE_FPS;
+	return 0;
+}
+
+static int hdmi_g_dv_timings(struct v4l2_subdev *sd,
+	struct v4l2_dv_timings *timings)
+{
+	*timings = sd_to_hdmi_dev(sd)->cur_timings;
+	return 0;
+}
+
 static int hdmi_g_mbus_fmt(struct v4l2_subdev *sd,
 	  struct v4l2_mbus_framefmt *fmt)
 {
@@ -679,6 +714,35 @@ static int hdmi_enum_dv_presets(struct v4l2_subdev *sd,
 		preset);
 }
 
+static int hdmi_enum_dv_timings(struct v4l2_subdev *sd,
+	struct v4l2_enum_dv_timings *timings)
+{
+	if (timings->index >= ARRAY_SIZE(hdmi_timings))
+		return -EINVAL;
+	timings->timings = hdmi_timings[timings->index].dv_timings;
+	if (!hdmi_timings[timings->index].reduced_fps)
+		timings->timings.bt.flags &= ~V4L2_DV_FL_CAN_REDUCE_FPS;
+	return 0;
+}
+
+static int hdmi_dv_timings_cap(struct v4l2_subdev *sd,
+	struct v4l2_dv_timings_cap *cap)
+{
+	struct hdmi_device *hdev = sd_to_hdmi_dev(sd);
+
+	/* Let the phy fill in the pixelclock range */
+	v4l2_subdev_call(hdev->phy_sd, video, dv_timings_cap, 0);
+	cap->type = V4L2_DV_BT_656_1120;
+	cap->bt.min_width = 720;
+	cap->bt.max_width = 1920;
+	cap->bt.min_height = 480;
+	cap->bt.max_height = 1080;
+	cap->bt.standards = V4L2_DV_BT_STD_CEA861;
+	cap->bt.capabilities = V4L2_DV_BT_CAP_INTERLACED |
+			       V4L2_DV_BT_CAP_PROGRESSIVE;
+	return 0;
+}
+
 static const struct v4l2_subdev_core_ops hdmi_sd_core_ops = {
 	.s_power = hdmi_s_power,
 };
@@ -687,6 +751,10 @@ static const struct v4l2_subdev_video_ops hdmi_sd_video_ops = {
 	.s_dv_preset = hdmi_s_dv_preset,
 	.g_dv_preset = hdmi_g_dv_preset,
 	.enum_dv_presets = hdmi_enum_dv_presets,
+	.s_dv_timings = hdmi_s_dv_timings,
+	.g_dv_timings = hdmi_g_dv_timings,
+	.enum_dv_timings = hdmi_enum_dv_timings,
+	.dv_timings_cap = hdmi_dv_timings_cap,
 	.g_mbus_fmt = hdmi_g_mbus_fmt,
 	.s_stream = hdmi_s_stream,
 };
-- 
1.7.10.4

[REVIEW PATCH 3/6] s5p-tv: add dv_timings support for mixer_video.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This just adds dv_timings support without modifying existing dv_preset
support.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/mixer_video.c |   80 +++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index 82142a2..24fb381 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -559,6 +559,79 @@ static int mxr_g_dv_preset(struct file *file, void *fh,
 	return ret ? -EINVAL : 0;
 }
 
+static int mxr_enum_dv_timings(struct file *file, void *fh,
+	struct v4l2_enum_dv_timings *timings)
+{
+	struct mxr_layer *layer = video_drvdata(file);
+	struct mxr_device *mdev = layer->mdev;
+	int ret;
+
+	/* lock protects from changing sd_out */
+	mutex_lock(&mdev->mutex);
+	ret = v4l2_subdev_call(to_outsd(mdev), video, enum_dv_timings, timings);
+	mutex_unlock(&mdev->mutex);
+
+	return ret ? -EINVAL : 0;
+}
+
+static int mxr_s_dv_timings(struct file *file, void *fh,
+	struct v4l2_dv_timings *timings)
+{
+	struct mxr_layer *layer = video_drvdata(file);
+	struct mxr_device *mdev = layer->mdev;
+	int ret;
+
+	/* lock protects from changing sd_out */
+	mutex_lock(&mdev->mutex);
+
+	/* timings change cannot be done while there is an entity
+	 * dependant on output configuration
+	 */
+	if (mdev->n_output > 0) {
+		mutex_unlock(&mdev->mutex);
+		return -EBUSY;
+	}
+
+	ret = v4l2_subdev_call(to_outsd(mdev), video, s_dv_timings, timings);
+
+	mutex_unlock(&mdev->mutex);
+
+	mxr_layer_update_output(layer);
+
+	/* any failure should return EINVAL according to V4L2 doc */
+	return ret ? -EINVAL : 0;
+}
+
+static int mxr_g_dv_timings(struct file *file, void *fh,
+	struct v4l2_dv_timings *timings)
+{
+	struct mxr_layer *layer = video_drvdata(file);
+	struct mxr_device *mdev = layer->mdev;
+	int ret;
+
+	/* lock protects from changing sd_out */
+	mutex_lock(&mdev->mutex);
+	ret = v4l2_subdev_call(to_outsd(mdev), video, g_dv_timings, timings);
+	mutex_unlock(&mdev->mutex);
+
+	return ret ? -EINVAL : 0;
+}
+
+static int mxr_dv_timings_cap(struct file *file, void *fh,
+	struct v4l2_dv_timings_cap *cap)
+{
+	struct mxr_layer *layer = video_drvdata(file);
+	struct mxr_device *mdev = layer->mdev;
+	int ret;
+
+	/* lock protects from changing sd_out */
+	mutex_lock(&mdev->mutex);
+	ret = v4l2_subdev_call(to_outsd(mdev), video, dv_timings_cap, cap);
+	mutex_unlock(&mdev->mutex);
+
+	return ret ? -EINVAL : 0;
+}
+
 static int mxr_s_std(struct file *file, void *fh, v4l2_std_id *norm)
 {
 	struct mxr_layer *layer = video_drvdata(file);
@@ -618,6 +691,8 @@ static int mxr_enum_output(struct file *file, void *fh, struct v4l2_output *a)
 	a->capabilities = 0;
 	if (sd->ops->video && sd->ops->video->s_dv_preset)
 		a->capabilities |= V4L2_OUT_CAP_PRESETS;
+	if (sd->ops->video && sd->ops->video->s_dv_timings)
+		a->capabilities |= V4L2_OUT_CAP_DV_TIMINGS;
 	if (sd->ops->video && sd->ops->video->s_std_output)
 		a->capabilities |= V4L2_OUT_CAP_STD;
 	a->type = V4L2_OUTPUT_TYPE_ANALOG;
@@ -742,6 +817,11 @@ static const struct v4l2_ioctl_ops mxr_ioctl_ops = {
 	.vidioc_enum_dv_presets = mxr_enum_dv_presets,
 	.vidioc_s_dv_preset = mxr_s_dv_preset,
 	.vidioc_g_dv_preset = mxr_g_dv_preset,
+	/* DV Timings functions */
+	.vidioc_enum_dv_timings = mxr_enum_dv_timings,
+	.vidioc_s_dv_timings = mxr_s_dv_timings,
+	.vidioc_g_dv_timings = mxr_g_dv_timings,
+	.vidioc_dv_timings_cap = mxr_dv_timings_cap,
 	/* analog TV standard functions */
 	.vidioc_s_std = mxr_s_std,
 	.vidioc_g_std = mxr_g_std,
-- 
1.7.10.4

[REVIEW PATCH 4/6] s5p-tv: remove dv_preset support from mixer_video.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The dv_preset API is deprecated and is replaced by the much improved dv_timings
API. Remove the dv_preset support from this driver as this will allow us to
remove the dv_preset API altogether (s5p-tv being the last user of this code).

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/mixer_video.c |   64 ---------------------------
 1 file changed, 64 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index 24fb381..9961e13 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -501,64 +501,6 @@ fail:
 	return -ERANGE;
 }
 
-static int mxr_enum_dv_presets(struct file *file, void *fh,
-	struct v4l2_dv_enum_preset *preset)
-{
-	struct mxr_layer *layer = video_drvdata(file);
-	struct mxr_device *mdev = layer->mdev;
-	int ret;
-
-	/* lock protects from changing sd_out */
-	mutex_lock(&mdev->mutex);
-	ret = v4l2_subdev_call(to_outsd(mdev), video, enum_dv_presets, preset);
-	mutex_unlock(&mdev->mutex);
-
-	return ret ? -EINVAL : 0;
-}
-
-static int mxr_s_dv_preset(struct file *file, void *fh,
-	struct v4l2_dv_preset *preset)
-{
-	struct mxr_layer *layer = video_drvdata(file);
-	struct mxr_device *mdev = layer->mdev;
-	int ret;
-
-	/* lock protects from changing sd_out */
-	mutex_lock(&mdev->mutex);
-
-	/* preset change cannot be done while there is an entity
-	 * dependant on output configuration
-	 */
-	if (mdev->n_output > 0) {
-		mutex_unlock(&mdev->mutex);
-		return -EBUSY;
-	}
-
-	ret = v4l2_subdev_call(to_outsd(mdev), video, s_dv_preset, preset);
-
-	mutex_unlock(&mdev->mutex);
-
-	mxr_layer_update_output(layer);
-
-	/* any failure should return EINVAL according to V4L2 doc */
-	return ret ? -EINVAL : 0;
-}
-
-static int mxr_g_dv_preset(struct file *file, void *fh,
-	struct v4l2_dv_preset *preset)
-{
-	struct mxr_layer *layer = video_drvdata(file);
-	struct mxr_device *mdev = layer->mdev;
-	int ret;
-
-	/* lock protects from changing sd_out */
-	mutex_lock(&mdev->mutex);
-	ret = v4l2_subdev_call(to_outsd(mdev), video, g_dv_preset, preset);
-	mutex_unlock(&mdev->mutex);
-
-	return ret ? -EINVAL : 0;
-}
-
 static int mxr_enum_dv_timings(struct file *file, void *fh,
 	struct v4l2_enum_dv_timings *timings)
 {
@@ -689,8 +631,6 @@ static int mxr_enum_output(struct file *file, void *fh, struct v4l2_output *a)
 	/* try to obtain supported tv norms */
 	v4l2_subdev_call(sd, video, g_tvnorms_output, &a->std);
 	a->capabilities = 0;
-	if (sd->ops->video && sd->ops->video->s_dv_preset)
-		a->capabilities |= V4L2_OUT_CAP_PRESETS;
 	if (sd->ops->video && sd->ops->video->s_dv_timings)
 		a->capabilities |= V4L2_OUT_CAP_DV_TIMINGS;
 	if (sd->ops->video && sd->ops->video->s_std_output)
@@ -813,10 +753,6 @@ static const struct v4l2_ioctl_ops mxr_ioctl_ops = {
 	/* Streaming control */
 	.vidioc_streamon = mxr_streamon,
 	.vidioc_streamoff = mxr_streamoff,
-	/* Preset functions */
-	.vidioc_enum_dv_presets = mxr_enum_dv_presets,
-	.vidioc_s_dv_preset = mxr_s_dv_preset,
-	.vidioc_g_dv_preset = mxr_g_dv_preset,
 	/* DV Timings functions */
 	.vidioc_enum_dv_timings = mxr_enum_dv_timings,
 	.vidioc_s_dv_timings = mxr_s_dv_timings,
-- 
1.7.10.4

[REVIEW PATCH 5/6] s5p-tv: remove the dv_preset API from hdmi.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The dv_preset API is deprecated and is replaced by the much improved dv_timings
API. Remove the dv_preset support from this driver as this will allow us to
remove the dv_preset API altogether (s5p-tv being the last user of this code).

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/hdmi_drv.c |   95 +++++++-----------------------
 1 file changed, 22 insertions(+), 73 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/hdmi_drv.c b/drivers/media/platform/s5p-tv/hdmi_drv.c
index 1376577..119603f 100644
--- a/drivers/media/platform/s5p-tv/hdmi_drv.c
+++ b/drivers/media/platform/s5p-tv/hdmi_drv.c
@@ -44,9 +44,6 @@ MODULE_AUTHOR("Tomasz Stanislawski, <t.stanislaws@samsung.com>");
 MODULE_DESCRIPTION("Samsung HDMI");
 MODULE_LICENSE("GPL");
 
-/* default preset configured on probe */
-#define HDMI_DEFAULT_PRESET V4L2_DV_480P59_94
-
 struct hdmi_pulse {
 	u32 beg;
 	u32 end;
@@ -92,8 +89,6 @@ struct hdmi_device {
 	const struct hdmi_timings *cur_conf;
 	/** flag indicating that timings are dirty */
 	int cur_conf_dirty;
-	/** current preset */
-	u32 cur_preset;
 	/** current timings */
 	struct v4l2_dv_timings cur_timings;
 	/** other resources */
@@ -255,7 +250,6 @@ static int hdmi_conf_apply(struct hdmi_device *hdmi_dev)
 {
 	struct device *dev = hdmi_dev->dev;
 	const struct hdmi_timings *conf = hdmi_dev->cur_conf;
-	struct v4l2_dv_preset preset;
 	int ret;
 
 	dev_dbg(dev, "%s\n", __func__);
@@ -270,11 +264,11 @@ static int hdmi_conf_apply(struct hdmi_device *hdmi_dev)
 	hdmi_write_mask(hdmi_dev, HDMI_PHY_RSTOUT,  0, HDMI_PHY_SW_RSTOUT);
 	mdelay(10);
 
-	/* configure presets */
-	preset.preset = hdmi_dev->cur_preset;
-	ret = v4l2_subdev_call(hdmi_dev->phy_sd, video, s_dv_preset, &preset);
+	/* configure timings */
+	ret = v4l2_subdev_call(hdmi_dev->phy_sd, video, s_dv_timings,
+				&hdmi_dev->cur_timings);
 	if (ret) {
-		dev_err(dev, "failed to set preset (%u)\n", preset.preset);
+		dev_err(dev, "failed to set timings\n");
 		return ret;
 	}
 
@@ -478,35 +472,26 @@ static const struct hdmi_timings hdmi_timings_1080p50 = {
 	.vsyn[0] = { .beg = 0 + 4, .end = 5 + 4},
 };
 
+/* default hdmi_timings index of the timings configured on probe */
+#define HDMI_DEFAULT_TIMINGS_IDX (0)
+
 static const struct {
-	u32 preset;
 	bool reduced_fps;
 	const struct v4l2_dv_timings dv_timings;
 	const struct hdmi_timings *hdmi_timings;
 } hdmi_timings[] = {
-	{ V4L2_DV_480P59_94, false, V4L2_DV_BT_CEA_720X480P59_94, &hdmi_timings_480p },
-	{ V4L2_DV_576P50, false, V4L2_DV_BT_CEA_720X576P50, &hdmi_timings_576p50 },
-	{ V4L2_DV_720P50, false, V4L2_DV_BT_CEA_1280X720P50, &hdmi_timings_720p50 },
-	{ V4L2_DV_720P59_94, true, V4L2_DV_BT_CEA_1280X720P60, &hdmi_timings_720p60 },
-	{ V4L2_DV_720P60, false, V4L2_DV_BT_CEA_1280X720P60, &hdmi_timings_720p60 },
-	{ V4L2_DV_1080P24, false, V4L2_DV_BT_CEA_1920X1080P24, &hdmi_timings_1080p24 },
-	{ V4L2_DV_1080P30, false, V4L2_DV_BT_CEA_1920X1080P30, &hdmi_timings_1080p60 },
-	{ V4L2_DV_1080P50, false, V4L2_DV_BT_CEA_1920X1080P50, &hdmi_timings_1080p50 },
-	{ V4L2_DV_1080I50, false, V4L2_DV_BT_CEA_1920X1080I50, &hdmi_timings_1080i50 },
-	{ V4L2_DV_1080I60, false, V4L2_DV_BT_CEA_1920X1080I60, &hdmi_timings_1080i60 },
-	{ V4L2_DV_1080P60, false, V4L2_DV_BT_CEA_1920X1080P60, &hdmi_timings_1080p60 },
+	{ false, V4L2_DV_BT_CEA_720X480P59_94, &hdmi_timings_480p    },
+	{ false, V4L2_DV_BT_CEA_720X576P50,    &hdmi_timings_576p50  },
+	{ false, V4L2_DV_BT_CEA_1280X720P50,   &hdmi_timings_720p50  },
+	{ true,  V4L2_DV_BT_CEA_1280X720P60,   &hdmi_timings_720p60  },
+	{ false, V4L2_DV_BT_CEA_1920X1080P24,  &hdmi_timings_1080p24 },
+	{ false, V4L2_DV_BT_CEA_1920X1080P30,  &hdmi_timings_1080p60 },
+	{ false, V4L2_DV_BT_CEA_1920X1080P50,  &hdmi_timings_1080p50 },
+	{ false, V4L2_DV_BT_CEA_1920X1080I50,  &hdmi_timings_1080i50 },
+	{ false, V4L2_DV_BT_CEA_1920X1080I60,  &hdmi_timings_1080i60 },
+	{ false, V4L2_DV_BT_CEA_1920X1080P60,  &hdmi_timings_1080p60 },
 };
 
-static const struct hdmi_timings *hdmi_preset2timings(u32 preset)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(hdmi_timings); ++i)
-		if (hdmi_timings[i].preset == preset)
-			return  hdmi_timings[i].hdmi_timings;
-	return NULL;
-}
-
 static int hdmi_streamon(struct hdmi_device *hdev)
 {
 	struct device *dev = hdev->dev;
@@ -626,32 +611,6 @@ static int hdmi_s_power(struct v4l2_subdev *sd, int on)
 	return IS_ERR_VALUE(ret) ? ret : 0;
 }
 
-static int hdmi_s_dv_preset(struct v4l2_subdev *sd,
-	struct v4l2_dv_preset *preset)
-{
-	struct hdmi_device *hdev = sd_to_hdmi_dev(sd);
-	struct device *dev = hdev->dev;
-	const struct hdmi_timings *conf;
-
-	conf = hdmi_preset2timings(preset->preset);
-	if (conf == NULL) {
-		dev_err(dev, "preset (%u) not supported\n", preset->preset);
-		return -EINVAL;
-	}
-	hdev->cur_conf = conf;
-	hdev->cur_conf_dirty = 1;
-	hdev->cur_preset = preset->preset;
-	return 0;
-}
-
-static int hdmi_g_dv_preset(struct v4l2_subdev *sd,
-	struct v4l2_dv_preset *preset)
-{
-	memset(preset, 0, sizeof(*preset));
-	preset->preset = sd_to_hdmi_dev(sd)->cur_preset;
-	return 0;
-}
-
 static int hdmi_s_dv_timings(struct v4l2_subdev *sd,
 	struct v4l2_dv_timings *timings)
 {
@@ -705,15 +664,6 @@ static int hdmi_g_mbus_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int hdmi_enum_dv_presets(struct v4l2_subdev *sd,
-	struct v4l2_dv_enum_preset *preset)
-{
-	if (preset->index >= ARRAY_SIZE(hdmi_timings))
-		return -EINVAL;
-	return v4l_fill_dv_preset_info(hdmi_timings[preset->index].preset,
-		preset);
-}
-
 static int hdmi_enum_dv_timings(struct v4l2_subdev *sd,
 	struct v4l2_enum_dv_timings *timings)
 {
@@ -748,9 +698,6 @@ static const struct v4l2_subdev_core_ops hdmi_sd_core_ops = {
 };
 
 static const struct v4l2_subdev_video_ops hdmi_sd_video_ops = {
-	.s_dv_preset = hdmi_s_dv_preset,
-	.g_dv_preset = hdmi_g_dv_preset,
-	.enum_dv_presets = hdmi_enum_dv_presets,
 	.s_dv_timings = hdmi_s_dv_timings,
 	.g_dv_timings = hdmi_g_dv_timings,
 	.enum_dv_timings = hdmi_enum_dv_timings,
@@ -1024,9 +971,11 @@ static int hdmi_probe(struct platform_device *pdev)
 	sd->owner = THIS_MODULE;
 
 	strlcpy(sd->name, "s5p-hdmi", sizeof(sd->name));
-	hdmi_dev->cur_preset = HDMI_DEFAULT_PRESET;
-	/* FIXME: missing fail preset is not supported */
-	hdmi_dev->cur_conf = hdmi_preset2timings(hdmi_dev->cur_preset);
+	hdmi_dev->cur_timings =
+		hdmi_timings[HDMI_DEFAULT_TIMINGS_IDX].dv_timings;
+	/* FIXME: missing fail timings is not supported */
+	hdmi_dev->cur_conf =
+		hdmi_timings[HDMI_DEFAULT_TIMINGS_IDX].hdmi_timings;
 	hdmi_dev->cur_conf_dirty = 1;
 
 	/* storing subdev for call that have only access to struct device */
-- 
1.7.10.4

[REVIEW PATCH 6/6] s5p-tv: remove the dv_preset API from hdmiphy.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The dv_preset API is deprecated and is replaced by the much improved dv_timings
API. Remove the dv_preset support from this driver as this will allow us to
remove the dv_preset API altogether (s5p-tv being the last user of this code).

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/hdmiphy_drv.c |   53 ---------------------------
 1 file changed, 53 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/hdmiphy_drv.c b/drivers/media/platform/s5p-tv/hdmiphy_drv.c
index ef0d812..e19a0af 100644
--- a/drivers/media/platform/s5p-tv/hdmiphy_drv.c
+++ b/drivers/media/platform/s5p-tv/hdmiphy_drv.c
@@ -176,27 +176,6 @@ static inline struct hdmiphy_ctx *sd_to_ctx(struct v4l2_subdev *sd)
 	return container_of(sd, struct hdmiphy_ctx, sd);
 }
 
-static unsigned long hdmiphy_preset_to_pixclk(u32 preset)
-{
-	static const unsigned long pixclk[] = {
-		[V4L2_DV_480P59_94] =  27000000,
-		[V4L2_DV_576P50]    =  27000000,
-		[V4L2_DV_720P59_94] =  74176000,
-		[V4L2_DV_720P50]    =  74250000,
-		[V4L2_DV_720P60]    =  74250000,
-		[V4L2_DV_1080P24]   =  74250000,
-		[V4L2_DV_1080P30]   =  74250000,
-		[V4L2_DV_1080I50]   =  74250000,
-		[V4L2_DV_1080I60]   =  74250000,
-		[V4L2_DV_1080P50]   = 148500000,
-		[V4L2_DV_1080P60]   = 148500000,
-	};
-	if (preset < ARRAY_SIZE(pixclk))
-		return pixclk[preset];
-	else
-		return 0;
-}
-
 static const u8 *hdmiphy_find_conf(unsigned long pixclk,
 		const struct hdmiphy_conf *conf)
 {
@@ -212,37 +191,6 @@ static int hdmiphy_s_power(struct v4l2_subdev *sd, int on)
 	return 0;
 }
 
-static int hdmiphy_s_dv_preset(struct v4l2_subdev *sd,
-	struct v4l2_dv_preset *preset)
-{
-	const u8 *data = NULL;
-	u8 buffer[32];
-	int ret;
-	struct hdmiphy_ctx *ctx = sd_to_ctx(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	unsigned long pixclk;
-	struct device *dev = &client->dev;
-
-	dev_info(dev, "s_dv_preset(preset = %d)\n", preset->preset);
-
-	pixclk = hdmiphy_preset_to_pixclk(preset->preset);
-	data = hdmiphy_find_conf(pixclk, ctx->conf_tab);
-	if (!data) {
-		dev_err(dev, "format not supported\n");
-		return -EINVAL;
-	}
-
-	/* storing configuration to the device */
-	memcpy(buffer, data, 32);
-	ret = i2c_master_send(client, buffer, 32);
-	if (ret != 32) {
-		dev_err(dev, "failed to configure HDMIPHY via I2C\n");
-		return -EIO;
-	}
-
-	return 0;
-}
-
 static int hdmiphy_s_dv_timings(struct v4l2_subdev *sd,
 	struct v4l2_dv_timings *timings)
 {
@@ -310,7 +258,6 @@ static const struct v4l2_subdev_core_ops hdmiphy_core_ops = {
 };
 
 static const struct v4l2_subdev_video_ops hdmiphy_video_ops = {
-	.s_dv_preset = hdmiphy_s_dv_preset,
 	.s_dv_timings = hdmiphy_s_dv_timings,
 	.dv_timings_cap = hdmiphy_dv_timings_cap,
 	.s_stream =  hdmiphy_s_stream,
-- 
1.7.10.4

Re: [REVIEW PATCH 0/6] s5p-tv: replace dv_preset by dv_timings

[Hans Verkuil]

On Mon March 4 2013 14:02:00 Hans Verkuil wrote:
> Hi Tomasz,
> 
> Here is what I hope is the final patch series for this. I've incorporated
> your suggestions and it's split off from the davinci/blackfin changes into
> its own patch series to keep things better organized.
> 
> The changes since the previous version are:
> 
> - changed the order of the first three patches as per your suggestion.
> - the patch named "[RFC PATCH 08/18] s5p-tv: add dv_timings support for
>   mixer_video." had two changes that rightfully belonged to the 'add
>   dv_timings support for mixer_video.' patch. Moved them accordingly.
> - hdmiphy now also supports dv_timings_cap and sets the pixelclock range
>   accordingly. The hdmi driver chains hdmiphy to get those values.
> - updating the minimum width to 720.
> 
> I didn't add a comment to clarify the pixclk handling hdmiphy_s_dv_preset
> because 1) I forgot, 2) it's not a bug, and 3) that whole function is
> removed anyway a few patches later :-)
> 
> The only functional change is the handling of dv_timings_cap. Can you
> verify that that works as it should?

Tomasz,

Should I wait for feedback from you, or can I go ahead and make a pull
request for this?

Regards,

	Hans

Re: [REVIEW PATCH 0/6] s5p-tv: replace dv_preset by dv_timings

[Tomasz Stanislawski]

Hi everyone,
After successful testing (after applying "use cap instead of 0" fix),
please add:

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

to the commit log.

Regards,
Tomasz Stanislawski

On 03/18/2013 03:24 PM, Hans Verkuil wrote:
> On Mon March 4 2013 14:02:00 Hans Verkuil wrote:
>> Hi Tomasz,
>>
>> Here is what I hope is the final patch series for this. I've incorporated
>> your suggestions and it's split off from the davinci/blackfin changes into
>> its own patch series to keep things better organized.
>>
>> The changes since the previous version are:
>>
>> - changed the order of the first three patches as per your suggestion.
>> - the patch named "[RFC PATCH 08/18] s5p-tv: add dv_timings support for
>>   mixer_video." had two changes that rightfully belonged to the 'add
>>   dv_timings support for mixer_video.' patch. Moved them accordingly.
>> - hdmiphy now also supports dv_timings_cap and sets the pixelclock range
>>   accordingly. The hdmi driver chains hdmiphy to get those values.
>> - updating the minimum width to 720.
>>
>> I didn't add a comment to clarify the pixclk handling hdmiphy_s_dv_preset
>> because 1) I forgot, 2) it's not a bug, and 3) that whole function is
>> removed anyway a few patches later :-)
>>
>> The only functional change is the handling of dv_timings_cap. Can you
>> verify that that works as it should?
> 
> Tomasz,
> 
> Should I wait for feedback from you, or can I go ahead and make a pull
> request for this?
> 
> Regards,
> 
> 	Hans
>