[PATCH v3 0/3] Conversion of the NOON010PC30 sensor driver to media controller API

[PATCH v3 0/3] Conversion of the NOON010PC30 sensor driver to media controller API

[Sylwester Nawrocki]

Hello,

The following patch set converts noon010pc30 camera sensor driver to the 
subdev pad level operations and user-space V4L2 subdev API. 
In addition it implements s_stream operation, removes the now unneeded
g_chip_ident op and tags the driver as experimental.

Changes since v1:
- fixed subdev's flags initialization
- corrected s_stream handler so the sensor's sleep mode is used
  for suspending/resuming the output signal
- removed g_chip_indent operation handler

Changes since v2:
- added subdev internal open() operation
- moved registers access to s_power or s_stream so the user space
 calls are supported before/without enabling sensor's power

Sylwester Nawrocki (3):
  noon010pc30: Conversion to the media controller API
  noon010pc30: Improve s_power operation handling
  noon010pc30: Remove g_chip_ident operation handler

 drivers/media/video/Kconfig       |    2 +-
 drivers/media/video/noon010pc30.c |  295 ++++++++++++++++++++++---------------
 include/media/v4l2-chip-ident.h   |    3 -
 3 files changed, 177 insertions(+), 123 deletions(-)


Regards,
--
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH v3 1/3] noon010pc30: Conversion to the media controller API

[Sylwester Nawrocki]

Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations.
Add media entity initialization and set subdev flags so the host driver
creates a subdev device node for the driver.
A mutex was added for serializing the subdev operations. When setting
format is attempted during streaming an (EBUSY) error will be returned.

After the device is powered up it will now remain in "power sleep"
mode until s_stream(1) is called. The "power sleep" mode is used
to suspend/resume frame generation at the sensor's output through
s_stream op.

While at here simplify the colorspace parameter handling.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/Kconfig       |    2 +-
 drivers/media/video/noon010pc30.c |  253 ++++++++++++++++++++++++-------------
 2 files changed, 164 insertions(+), 91 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 6279663..75bb46f 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -755,7 +755,7 @@ config VIDEO_VIA_CAMERA
 
 config VIDEO_NOON010PC30
 	tristate "NOON010PC30 CIF camera sensor support"
-	depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2 && EXPERIMENTAL && VIDEO_V4L2_SUBDEV_API
 	---help---
 	  This driver supports NOON010PC30 CIF camera from Siliconfile
 
diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
index 35f722a..115d976 100644
--- a/drivers/media/video/noon010pc30.c
+++ b/drivers/media/video/noon010pc30.c
@@ -1,7 +1,7 @@
 /*
  * Driver for SiliconFile NOON010PC30 CIF (1/11") Image Sensor with ISP
  *
- * Copyright (C) 2010 Samsung Electronics
+ * Copyright (C) 2010 - 2011 Samsung Electronics Co., Ltd.
  * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com>
  *
  * Initial register configuration based on a driver authored by
@@ -10,7 +10,7 @@
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later vergsion.
+ * (at your option) any later version.
  */
 
 #include <linux/delay.h>
@@ -113,7 +113,6 @@ MODULE_PARM_DESC(debug, "Enable module debug trace. Set to 1 to enable.");
 
 struct noon010_format {
 	enum v4l2_mbus_pixelcode code;
-	enum v4l2_colorspace colorspace;
 	u16 ispctl1_reg;
 };
 
@@ -131,17 +130,24 @@ static const char * const noon010_supply_name[] = {
 
 struct noon010_info {
 	struct v4l2_subdev sd;
+	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;
 	const struct noon010pc30_platform_data *pdata;
+	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
+	u32 gpio_nreset;
+	u32 gpio_nstby;
+
+	/* Protects the struct members below */
+	struct mutex lock;
+
 	const struct noon010_format *curr_fmt;
 	const struct noon010_frmsize *curr_win;
+	unsigned int apply_new_cfg:1;
+	unsigned int streaming:1;
 	unsigned int hflip:1;
 	unsigned int vflip:1;
 	unsigned int power:1;
 	u8 i2c_reg_page;
-	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
-	u32 gpio_nreset;
-	u32 gpio_nstby;
 };
 
 struct i2c_regval {
@@ -168,27 +174,11 @@ static const struct noon010_frmsize noon010_sizes[] = {
 
 /* Supported pixel formats. */
 static const struct noon010_format noon010_formats[] = {
-	{
-		.code		= V4L2_MBUS_FMT_YUYV8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
-		.ispctl1_reg	= 0x03,
-	}, {
-		.code		= V4L2_MBUS_FMT_YVYU8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
-		.ispctl1_reg	= 0x02,
-	}, {
-		.code		= V4L2_MBUS_FMT_VYUY8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
-		.ispctl1_reg	= 0,
-	}, {
-		.code		= V4L2_MBUS_FMT_UYVY8_2X8,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
-		.ispctl1_reg	= 0x01,
-	}, {
-		.code		= V4L2_MBUS_FMT_RGB565_2X8_BE,
-		.colorspace	= V4L2_COLORSPACE_JPEG,
-		.ispctl1_reg	= 0x40,
-	},
+	{ V4L2_MBUS_FMT_YUYV8_2X8, 0x03 },
+	{ V4L2_MBUS_FMT_YVYU8_2X8, 0x02 },
+	{ V4L2_MBUS_FMT_VYUY8_2X8, 0 },
+	{ V4L2_MBUS_FMT_UYVY8_2X8, 0x01 },
+	{ V4L2_MBUS_FMT_RGB565_2X8_BE, 0x40},
 };
 
 static const struct i2c_regval noon010_base_regs[] = {
@@ -313,6 +303,7 @@ static int noon010_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+/* Called with struct noon010_info.lock mutex held */
 static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
 {
 	struct noon010_info *info = to_noon010(sd);
@@ -340,21 +331,18 @@ static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
 static int noon010_set_params(struct v4l2_subdev *sd)
 {
 	struct noon010_info *info = to_noon010(sd);
-	int ret;
-
-	if (!info->curr_win)
-		return -EINVAL;
-
-	ret = cam_i2c_write(sd, VDO_CTL_REG(0), info->curr_win->vid_ctl1);
 
-	if (!ret && info->curr_fmt)
-		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
-				info->curr_fmt->ispctl1_reg);
-	return ret;
+	int ret = cam_i2c_write(sd, VDO_CTL_REG(0),
+				info->curr_win->vid_ctl1);
+	if (ret)
+		return ret;
+	return cam_i2c_write(sd, ISP_CTL_REG(0),
+			     info->curr_fmt->ispctl1_reg);
 }
 
 /* Find nearest matching image pixel size. */
-static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
+static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf,
+				  const struct noon010_frmsize **size)
 {
 	unsigned int min_err = ~0;
 	int i = ARRAY_SIZE(noon010_sizes);
@@ -374,11 +362,14 @@ static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
 	if (match) {
 		mf->width  = match->width;
 		mf->height = match->height;
+		if (size)
+			*size = match;
 		return 0;
 	}
 	return -EINVAL;
 }
 
+/* Called with info.lock mutex held */
 static int power_enable(struct noon010_info *info)
 {
 	int ret;
@@ -419,6 +410,7 @@ static int power_enable(struct noon010_info *info)
 	return 0;
 }
 
+/* Called with info.lock mutex held */
 static int power_disable(struct noon010_info *info)
 {
 	int ret;
@@ -448,93 +440,125 @@ static int power_disable(struct noon010_info *info)
 static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = to_sd(ctrl);
+	struct noon010_info *info = to_noon010(sd);
+	int ret = 0;
 
 	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
 		 __func__, ctrl->id, ctrl->val);
 
+	mutex_lock(&info->lock);
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any controls to H/W at this time. Instead
+	 * the controls will be restored right after power-up.
+	 */
+	if (!info->power)
+		goto unlock;
+
 	switch (ctrl->id) {
 	case V4L2_CID_AUTO_WHITE_BALANCE:
-		return noon010_enable_autowhitebalance(sd, ctrl->val);
+		ret = noon010_enable_autowhitebalance(sd, ctrl->val);
+		break;
 	case V4L2_CID_BLUE_BALANCE:
-		return cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
+		ret = cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
+		break;
 	case V4L2_CID_RED_BALANCE:
-		return cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
+		ret =  cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+unlock:
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
-static int noon010_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
-			    enum v4l2_mbus_pixelcode *code)
+static int noon010_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (!code || index >= ARRAY_SIZE(noon010_formats))
+	if (code->index >= ARRAY_SIZE(noon010_formats))
 		return -EINVAL;
 
-	*code = noon010_formats[index].code;
+	code->code = noon010_formats[code->index].code;
 	return 0;
 }
 
-static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
+static void noon010_get_current_fmt(struct noon010_info *info,
+				    struct v4l2_mbus_framefmt *mf)
 {
-	struct noon010_info *info = to_noon010(sd);
-	int ret;
-
-	if (!mf)
-		return -EINVAL;
-
-	if (!info->curr_win || !info->curr_fmt) {
-		ret = noon010_set_params(sd);
-		if (ret)
-			return ret;
-	}
-
 	mf->width	= info->curr_win->width;
 	mf->height	= info->curr_win->height;
 	mf->code	= info->curr_fmt->code;
-	mf->colorspace	= info->curr_fmt->colorspace;
 	mf->field	= V4L2_FIELD_NONE;
+	mf->colorspace	= V4L2_COLORSPACE_JPEG;
+}
+
+static int noon010_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			   struct v4l2_subdev_format *fmt)
+{
+	struct noon010_info *info = to_noon010(sd);
+	struct v4l2_mbus_framefmt *mf;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (fh) {
+			mf = v4l2_subdev_get_try_format(fh, 0);
+			fmt->format = *mf;
+		}
+		return 0;
+	}
+
+	mutex_lock(&info->lock);
+	noon010_get_current_fmt(info, &fmt->format);
+	mutex_unlock(&info->lock);
 
 	return 0;
 }
 
 /* Return nearest media bus frame format. */
-static const struct noon010_format *try_fmt(struct v4l2_subdev *sd,
+static const struct noon010_format *noon010_try_fmt(struct v4l2_subdev *sd,
 					    struct v4l2_mbus_framefmt *mf)
 {
 	int i = ARRAY_SIZE(noon010_formats);
 
-	noon010_try_frame_size(mf);
-
-	while (i--)
+	while (--i)
 		if (mf->code == noon010_formats[i].code)
 			break;
-
 	mf->code = noon010_formats[i].code;
 
 	return &noon010_formats[i];
 }
 
-static int noon010_try_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_mbus_framefmt *mf)
-{
-	if (!sd || !mf)
-		return -EINVAL;
-
-	try_fmt(sd, mf);
-	return 0;
-}
-
-static int noon010_s_fmt(struct v4l2_subdev *sd,
-			 struct v4l2_mbus_framefmt *mf)
+static int noon010_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			   struct v4l2_subdev_format *fmt)
 {
 	struct noon010_info *info = to_noon010(sd);
+	const struct noon010_frmsize *size = NULL;
+	const struct noon010_format *nf;
+	struct v4l2_mbus_framefmt *mf;
+	int ret = 0;
 
-	if (!sd || !mf)
-		return -EINVAL;
-
-	info->curr_fmt = try_fmt(sd, mf);
+	nf = noon010_try_fmt(sd, &fmt->format);
+	noon010_try_frame_size(&fmt->format, &size);
+	fmt->format.colorspace = V4L2_COLORSPACE_JPEG;
 
-	return noon010_set_params(sd);
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (fh) {
+			mf = v4l2_subdev_get_try_format(fh, 0);
+			*mf = fmt->format;
+		}
+		return 0;
+	}
+	mutex_lock(&info->lock);
+	if (!info->streaming) {
+		info->apply_new_cfg = 1;
+		info->curr_fmt = nf;
+		info->curr_win = size;
+	} else {
+		ret = -EBUSY;
+	}
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
 static int noon010_base_config(struct v4l2_subdev *sd)
@@ -550,8 +574,6 @@ static int noon010_base_config(struct v4l2_subdev *sd)
 	}
 	if (!ret)
 		ret = noon010_set_flip(sd, 1, 0);
-	if (!ret)
-		ret = noon010_power_ctrl(sd, false, false);
 
 	/* sync the handler and the registers state */
 	v4l2_ctrl_handler_setup(&to_noon010(sd)->hdl);
@@ -582,6 +604,26 @@ static int noon010_s_power(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+static int noon010_s_stream(struct v4l2_subdev *sd, int on)
+{
+	struct noon010_info *info = to_noon010(sd);
+	int ret = 0;
+
+	mutex_lock(&info->lock);
+	if (!info->streaming != !on) {
+		ret = noon010_power_ctrl(sd, false, !on);
+		if (!ret)
+			info->streaming = on;
+	}
+	if (!ret && on && info->apply_new_cfg) {
+		ret = noon010_set_params(sd);
+		if (!ret)
+			info->apply_new_cfg = 0;
+	}
+	mutex_unlock(&info->lock);
+	return ret;
+}
+
 static int noon010_g_chip_ident(struct v4l2_subdev *sd,
 				struct v4l2_dbg_chip_ident *chip)
 {
@@ -599,6 +641,22 @@ static int noon010_log_status(struct v4l2_subdev *sd)
 	return 0;
 }
 
+static int noon010_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(fh, 0);
+	struct noon010_info *info = to_noon010(sd);
+
+	mutex_lock(&info->lock);
+	noon010_get_current_fmt(to_noon010(sd), mf);
+
+	mutex_unlock(&info->lock);
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops noon010_subdev_internal_ops = {
+	.open = noon010_open,
+};
+
 static const struct v4l2_ctrl_ops noon010_ctrl_ops = {
 	.s_ctrl = noon010_s_ctrl,
 };
@@ -616,15 +674,19 @@ static const struct v4l2_subdev_core_ops noon010_core_ops = {
 	.log_status	= noon010_log_status,
 };
 
-static const struct v4l2_subdev_video_ops noon010_video_ops = {
-	.g_mbus_fmt	= noon010_g_fmt,
-	.s_mbus_fmt	= noon010_s_fmt,
-	.try_mbus_fmt	= noon010_try_fmt,
-	.enum_mbus_fmt	= noon010_enum_fmt,
+static struct v4l2_subdev_pad_ops noon010_pad_ops = {
+	.enum_mbus_code	= noon010_enum_mbus_code,
+	.get_fmt	= noon010_get_fmt,
+	.set_fmt	= noon010_set_fmt,
+};
+
+static struct v4l2_subdev_video_ops noon010_video_ops = {
+	.s_stream	= noon010_s_stream,
 };
 
 static const struct v4l2_subdev_ops noon010_ops = {
 	.core	= &noon010_core_ops,
+	.pad	= &noon010_pad_ops,
 	.video	= &noon010_video_ops,
 };
 
@@ -665,10 +727,14 @@ static int noon010_probe(struct i2c_client *client,
 	if (!info)
 		return -ENOMEM;
 
+	mutex_init(&info->lock);
 	sd = &info->sd;
 	strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
 	v4l2_i2c_subdev_init(sd, client, &noon010_ops);
 
+	sd->internal_ops = &noon010_subdev_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
 	v4l2_ctrl_handler_init(&info->hdl, 3);
 
 	v4l2_ctrl_new_std(&info->hdl, &noon010_ctrl_ops,
@@ -719,11 +785,17 @@ static int noon010_probe(struct i2c_client *client,
 	if (ret)
 		goto np_reg_err;
 
+	info->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
+	ret = media_entity_init(&sd->entity, 1, &info->pad, 0);
+	if (ret < 0)
+		goto np_me_err;
+
 	ret = noon010_detect(client, info);
 	if (!ret)
 		return 0;
 
-	/* the sensor detection failed */
+np_me_err:
 	regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply);
 np_reg_err:
 	if (gpio_is_valid(info->gpio_nstby))
@@ -754,6 +826,7 @@ static int noon010_remove(struct i2c_client *client)
 	if (gpio_is_valid(info->gpio_nstby))
 		gpio_free(info->gpio_nstby);
 
+	media_entity_cleanup(&sd->entity);
 	kfree(info);
 	return 0;
 }
-- 
1.7.6

[PATCH v3 2/3] noon010pc30: Improve s_power operation handling

[Sylwester Nawrocki]

Remove the now unneeded check for the platform data in s_power
handler and the platform data pointer in struct noon010_info.
Also do not reset the configured output resolution and pixel
format when cycling sensor's power.
Add small delay for proper reset signal shape.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/noon010pc30.c |   40 +++++++++++++++---------------------
 1 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
index 115d976..436b1ee 100644
--- a/drivers/media/video/noon010pc30.c
+++ b/drivers/media/video/noon010pc30.c
@@ -132,7 +132,6 @@ struct noon010_info {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;
-	const struct noon010pc30_platform_data *pdata;
 	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
 	u32 gpio_nreset;
 	u32 gpio_nstby;
@@ -282,8 +281,10 @@ static int noon010_power_ctrl(struct v4l2_subdev *sd, bool reset, bool sleep)
 	u8 reg = sleep ? 0xF1 : 0xF0;
 	int ret = 0;
 
-	if (reset)
+	if (reset) {
 		ret = cam_i2c_write(sd, POWER_CTRL_REG, reg | 0x02);
+		udelay(20);
+	}
 	if (!ret) {
 		ret = cam_i2c_write(sd, POWER_CTRL_REG, reg);
 		if (reset && !ret)
@@ -561,45 +562,37 @@ static int noon010_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	return ret;
 }
 
+/* Called with struct noon010_info.lock mutex held */
 static int noon010_base_config(struct v4l2_subdev *sd)
 {
-	struct noon010_info *info = to_noon010(sd);
-	int ret;
-
-	ret = noon010_bulk_write_reg(sd, noon010_base_regs);
-	if (!ret) {
-		info->curr_fmt = &noon010_formats[0];
-		info->curr_win = &noon010_sizes[0];
+	int ret = noon010_bulk_write_reg(sd, noon010_base_regs);
+	if (!ret)
 		ret = noon010_set_params(sd);
-	}
 	if (!ret)
 		ret = noon010_set_flip(sd, 1, 0);
 
-	/* sync the handler and the registers state */
-	v4l2_ctrl_handler_setup(&to_noon010(sd)->hdl);
 	return ret;
 }
 
 static int noon010_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct noon010_info *info = to_noon010(sd);
-	const struct noon010pc30_platform_data *pdata = info->pdata;
-	int ret = 0;
-
-	if (WARN(pdata == NULL, "No platform data!\n"))
-		return -ENOMEM;
+	int ret;
 
+	mutex_lock(&info->lock);
 	if (on) {
 		ret = power_enable(info);
-		if (ret)
-			return ret;
-		ret = noon010_base_config(sd);
+		if (!ret)
+			ret = noon010_base_config(sd);
 	} else {
 		noon010_power_ctrl(sd, false, true);
 		ret = power_disable(info);
-		info->curr_win = NULL;
-		info->curr_fmt = NULL;
 	}
+	mutex_unlock(&info->lock);
+
+	/* Restore the controls state */
+	if (!ret && on)
+		ret = v4l2_ctrl_handler_setup(&info->hdl);
 
 	return ret;
 }
@@ -750,10 +743,11 @@ static int noon010_probe(struct i2c_client *client,
 	if (ret)
 		goto np_err;
 
-	info->pdata		= client->dev.platform_data;
 	info->i2c_reg_page	= -1;
 	info->gpio_nreset	= -EINVAL;
 	info->gpio_nstby	= -EINVAL;
+	info->curr_fmt		= &noon010_formats[0];
+	info->curr_win		= &noon010_sizes[0];
 
 	if (gpio_is_valid(pdata->gpio_nreset)) {
 		ret = gpio_request(pdata->gpio_nreset, "NOON010PC30 NRST");
-- 
1.7.6

[PATCH 3/3 (resend)] noon010pc30: Remove g_chip_ident operation handler

[Sylwester Nawrocki]

It is now not needed as the sensor identification is done
through the media controller API.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/noon010pc30.c |   10 ----------
 include/media/v4l2-chip-ident.h   |    3 ---
 2 files changed, 0 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
index 436b1ee..d38b4d4 100644
--- a/drivers/media/video/noon010pc30.c
+++ b/drivers/media/video/noon010pc30.c
@@ -617,15 +617,6 @@ static int noon010_s_stream(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
-static int noon010_g_chip_ident(struct v4l2_subdev *sd,
-				struct v4l2_dbg_chip_ident *chip)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-
-	return v4l2_chip_ident_i2c_client(client, chip,
-					  V4L2_IDENT_NOON010PC30, 0);
-}
-
 static int noon010_log_status(struct v4l2_subdev *sd)
 {
 	struct noon010_info *info = to_noon010(sd);
@@ -655,7 +646,6 @@ static const struct v4l2_ctrl_ops noon010_ctrl_ops = {
 };
 
 static const struct v4l2_subdev_core_ops noon010_core_ops = {
-	.g_chip_ident	= noon010_g_chip_ident,
 	.s_power	= noon010_s_power,
 	.g_ctrl		= v4l2_subdev_g_ctrl,
 	.s_ctrl		= v4l2_subdev_s_ctrl,
diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index 63fd9d3..810a209 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -212,9 +212,6 @@ enum {
 	/* module sn9c20x: just ident 10000 */
 	V4L2_IDENT_SN9C20X = 10000,
 
-	/* Siliconfile sensors: reserved range 10100 - 10199 */
-	V4L2_IDENT_NOON010PC30	= 10100,
-
 	/* module cx231xx and cx25840 */
 	V4L2_IDENT_CX2310X_AV = 23099, /* Integrated A/V decoder; not in '100 */
 	V4L2_IDENT_CX23100    = 23100,
-- 
1.7.6

[PATCH v3 1/3] noon010pc30: Conversion to the media controller API

[Sylwester Nawrocki]

Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations.
Add media entity initialization and set subdev flags so the host driver
creates a subdev device node for the driver.
A mutex was added for serializing the subdev operations. When setting
format is attempted during streaming an (EBUSY) error will be returned.

After the device is powered up it will now remain in "power sleep"
mode until s_stream(1) is called. The "power sleep" mode is used
to suspend/resume frame generation at the sensor's output through
s_stream op.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

This is just a resend as I just previously sent wrong version.
Sorry about the mistake.

---
 drivers/media/video/Kconfig       |    2 +-
 drivers/media/video/noon010pc30.c |  224 ++++++++++++++++++++++++++-----------
 2 files changed, 158 insertions(+), 68 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 6279663..75bb46f 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -755,7 +755,7 @@ config VIDEO_VIA_CAMERA
 
 config VIDEO_NOON010PC30
 	tristate "NOON010PC30 CIF camera sensor support"
-	depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2 && EXPERIMENTAL && VIDEO_V4L2_SUBDEV_API
 	---help---
 	  This driver supports NOON010PC30 CIF camera from Siliconfile
 
diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
index 35f722a..f91ae14 100644
--- a/drivers/media/video/noon010pc30.c
+++ b/drivers/media/video/noon010pc30.c
@@ -1,7 +1,7 @@
 /*
  * Driver for SiliconFile NOON010PC30 CIF (1/11") Image Sensor with ISP
  *
- * Copyright (C) 2010 Samsung Electronics
+ * Copyright (C) 2010 - 2011 Samsung Electronics Co., Ltd.
  * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com>
  *
  * Initial register configuration based on a driver authored by
@@ -10,7 +10,7 @@
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later vergsion.
+ * (at your option) any later version.
  */
 
 #include <linux/delay.h>
@@ -131,17 +131,24 @@ static const char * const noon010_supply_name[] = {
 
 struct noon010_info {
 	struct v4l2_subdev sd;
+	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;
 	const struct noon010pc30_platform_data *pdata;
+	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
+	u32 gpio_nreset;
+	u32 gpio_nstby;
+
+	/* Protects the struct members below */
+	struct mutex lock;
+
 	const struct noon010_format *curr_fmt;
 	const struct noon010_frmsize *curr_win;
+	unsigned int apply_new_cfg:1;
+	unsigned int streaming:1;
 	unsigned int hflip:1;
 	unsigned int vflip:1;
 	unsigned int power:1;
 	u8 i2c_reg_page;
-	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
-	u32 gpio_nreset;
-	u32 gpio_nstby;
 };
 
 struct i2c_regval {
@@ -313,6 +320,7 @@ static int noon010_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+/* Called with struct noon010_info.lock mutex held */
 static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
 {
 	struct noon010_info *info = to_noon010(sd);
@@ -340,21 +348,18 @@ static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
 static int noon010_set_params(struct v4l2_subdev *sd)
 {
 	struct noon010_info *info = to_noon010(sd);
-	int ret;
-
-	if (!info->curr_win)
-		return -EINVAL;
 
-	ret = cam_i2c_write(sd, VDO_CTL_REG(0), info->curr_win->vid_ctl1);
-
-	if (!ret && info->curr_fmt)
-		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
-				info->curr_fmt->ispctl1_reg);
-	return ret;
+	int ret = cam_i2c_write(sd, VDO_CTL_REG(0),
+				info->curr_win->vid_ctl1);
+	if (ret)
+		return ret;
+	return cam_i2c_write(sd, ISP_CTL_REG(0),
+			     info->curr_fmt->ispctl1_reg);
 }
 
 /* Find nearest matching image pixel size. */
-static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
+static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf,
+				  const struct noon010_frmsize **size)
 {
 	unsigned int min_err = ~0;
 	int i = ARRAY_SIZE(noon010_sizes);
@@ -374,11 +379,14 @@ static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
 	if (match) {
 		mf->width  = match->width;
 		mf->height = match->height;
+		if (size)
+			*size = match;
 		return 0;
 	}
 	return -EINVAL;
 }
 
+/* Called with info.lock mutex held */
 static int power_enable(struct noon010_info *info)
 {
 	int ret;
@@ -419,6 +427,7 @@ static int power_enable(struct noon010_info *info)
 	return 0;
 }
 
+/* Called with info.lock mutex held */
 static int power_disable(struct noon010_info *info)
 {
 	int ret;
@@ -448,93 +457,125 @@ static int power_disable(struct noon010_info *info)
 static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = to_sd(ctrl);
+	struct noon010_info *info = to_noon010(sd);
+	int ret = 0;
 
 	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
 		 __func__, ctrl->id, ctrl->val);
 
+	mutex_lock(&info->lock);
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any controls to H/W at this time. Instead
+	 * the controls will be restored right after power-up.
+	 */
+	if (!info->power)
+		goto unlock;
+
 	switch (ctrl->id) {
 	case V4L2_CID_AUTO_WHITE_BALANCE:
-		return noon010_enable_autowhitebalance(sd, ctrl->val);
+		ret = noon010_enable_autowhitebalance(sd, ctrl->val);
+		break;
 	case V4L2_CID_BLUE_BALANCE:
-		return cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
+		ret = cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
+		break;
 	case V4L2_CID_RED_BALANCE:
-		return cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
+		ret =  cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+unlock:
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
-static int noon010_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
-			    enum v4l2_mbus_pixelcode *code)
+static int noon010_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (!code || index >= ARRAY_SIZE(noon010_formats))
+	if (code->index >= ARRAY_SIZE(noon010_formats))
 		return -EINVAL;
 
-	*code = noon010_formats[index].code;
+	code->code = noon010_formats[code->index].code;
 	return 0;
 }
 
-static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
+static void noon010_get_current_fmt(struct noon010_info *info,
+				    struct v4l2_mbus_framefmt *mf)
 {
-	struct noon010_info *info = to_noon010(sd);
-	int ret;
-
-	if (!mf)
-		return -EINVAL;
-
-	if (!info->curr_win || !info->curr_fmt) {
-		ret = noon010_set_params(sd);
-		if (ret)
-			return ret;
-	}
-
 	mf->width	= info->curr_win->width;
 	mf->height	= info->curr_win->height;
 	mf->code	= info->curr_fmt->code;
 	mf->colorspace	= info->curr_fmt->colorspace;
 	mf->field	= V4L2_FIELD_NONE;
+}
+
+static int noon010_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			   struct v4l2_subdev_format *fmt)
+{
+	struct noon010_info *info = to_noon010(sd);
+	struct v4l2_mbus_framefmt *mf;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (fh) {
+			mf = v4l2_subdev_get_try_format(fh, 0);
+			fmt->format = *mf;
+		}
+		return 0;
+	}
+
+	mutex_lock(&info->lock);
+	noon010_get_current_fmt(info, &fmt->format);
+	mutex_unlock(&info->lock);
 
 	return 0;
 }
 
 /* Return nearest media bus frame format. */
-static const struct noon010_format *try_fmt(struct v4l2_subdev *sd,
+static const struct noon010_format *noon010_try_fmt(struct v4l2_subdev *sd,
 					    struct v4l2_mbus_framefmt *mf)
 {
 	int i = ARRAY_SIZE(noon010_formats);
 
-	noon010_try_frame_size(mf);
-
-	while (i--)
+	while (--i)
 		if (mf->code == noon010_formats[i].code)
 			break;
-
 	mf->code = noon010_formats[i].code;
 
 	return &noon010_formats[i];
 }
 
-static int noon010_try_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_mbus_framefmt *mf)
-{
-	if (!sd || !mf)
-		return -EINVAL;
-
-	try_fmt(sd, mf);
-	return 0;
-}
-
-static int noon010_s_fmt(struct v4l2_subdev *sd,
-			 struct v4l2_mbus_framefmt *mf)
+static int noon010_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			   struct v4l2_subdev_format *fmt)
 {
 	struct noon010_info *info = to_noon010(sd);
+	const struct noon010_frmsize *size = NULL;
+	const struct noon010_format *nf;
+	struct v4l2_mbus_framefmt *mf;
+	int ret = 0;
 
-	if (!sd || !mf)
-		return -EINVAL;
-
-	info->curr_fmt = try_fmt(sd, mf);
+	nf = noon010_try_fmt(sd, &fmt->format);
+	noon010_try_frame_size(&fmt->format, &size);
+	fmt->format.colorspace = V4L2_COLORSPACE_JPEG;
 
-	return noon010_set_params(sd);
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (fh) {
+			mf = v4l2_subdev_get_try_format(fh, 0);
+			*mf = fmt->format;
+		}
+		return 0;
+	}
+	mutex_lock(&info->lock);
+	if (!info->streaming) {
+		info->apply_new_cfg = 1;
+		info->curr_fmt = nf;
+		info->curr_win = size;
+	} else {
+		ret = -EBUSY;
+	}
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
 static int noon010_base_config(struct v4l2_subdev *sd)
@@ -550,8 +591,6 @@ static int noon010_base_config(struct v4l2_subdev *sd)
 	}
 	if (!ret)
 		ret = noon010_set_flip(sd, 1, 0);
-	if (!ret)
-		ret = noon010_power_ctrl(sd, false, false);
 
 	/* sync the handler and the registers state */
 	v4l2_ctrl_handler_setup(&to_noon010(sd)->hdl);
@@ -582,6 +621,26 @@ static int noon010_s_power(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+static int noon010_s_stream(struct v4l2_subdev *sd, int on)
+{
+	struct noon010_info *info = to_noon010(sd);
+	int ret = 0;
+
+	mutex_lock(&info->lock);
+	if (!info->streaming != !on) {
+		ret = noon010_power_ctrl(sd, false, !on);
+		if (!ret)
+			info->streaming = on;
+	}
+	if (!ret && on && info->apply_new_cfg) {
+		ret = noon010_set_params(sd);
+		if (!ret)
+			info->apply_new_cfg = 0;
+	}
+	mutex_unlock(&info->lock);
+	return ret;
+}
+
 static int noon010_g_chip_ident(struct v4l2_subdev *sd,
 				struct v4l2_dbg_chip_ident *chip)
 {
@@ -599,6 +658,22 @@ static int noon010_log_status(struct v4l2_subdev *sd)
 	return 0;
 }
 
+static int noon010_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(fh, 0);
+	struct noon010_info *info = to_noon010(sd);
+
+	mutex_lock(&info->lock);
+	noon010_get_current_fmt(to_noon010(sd), mf);
+
+	mutex_unlock(&info->lock);
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops noon010_subdev_internal_ops = {
+	.open = noon010_open,
+};
+
 static const struct v4l2_ctrl_ops noon010_ctrl_ops = {
 	.s_ctrl = noon010_s_ctrl,
 };
@@ -616,15 +691,19 @@ static const struct v4l2_subdev_core_ops noon010_core_ops = {
 	.log_status	= noon010_log_status,
 };
 
-static const struct v4l2_subdev_video_ops noon010_video_ops = {
-	.g_mbus_fmt	= noon010_g_fmt,
-	.s_mbus_fmt	= noon010_s_fmt,
-	.try_mbus_fmt	= noon010_try_fmt,
-	.enum_mbus_fmt	= noon010_enum_fmt,
+static struct v4l2_subdev_pad_ops noon010_pad_ops = {
+	.enum_mbus_code	= noon010_enum_mbus_code,
+	.get_fmt	= noon010_get_fmt,
+	.set_fmt	= noon010_set_fmt,
+};
+
+static struct v4l2_subdev_video_ops noon010_video_ops = {
+	.s_stream	= noon010_s_stream,
 };
 
 static const struct v4l2_subdev_ops noon010_ops = {
 	.core	= &noon010_core_ops,
+	.pad	= &noon010_pad_ops,
 	.video	= &noon010_video_ops,
 };
 
@@ -665,10 +744,14 @@ static int noon010_probe(struct i2c_client *client,
 	if (!info)
 		return -ENOMEM;
 
+	mutex_init(&info->lock);
 	sd = &info->sd;
 	strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
 	v4l2_i2c_subdev_init(sd, client, &noon010_ops);
 
+	sd->internal_ops = &noon010_subdev_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
 	v4l2_ctrl_handler_init(&info->hdl, 3);
 
 	v4l2_ctrl_new_std(&info->hdl, &noon010_ctrl_ops,
@@ -719,11 +802,17 @@ static int noon010_probe(struct i2c_client *client,
 	if (ret)
 		goto np_reg_err;
 
+	info->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
+	ret = media_entity_init(&sd->entity, 1, &info->pad, 0);
+	if (ret < 0)
+		goto np_me_err;
+
 	ret = noon010_detect(client, info);
 	if (!ret)
 		return 0;
 
-	/* the sensor detection failed */
+np_me_err:
 	regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply);
 np_reg_err:
 	if (gpio_is_valid(info->gpio_nstby))
@@ -754,6 +843,7 @@ static int noon010_remove(struct i2c_client *client)
 	if (gpio_is_valid(info->gpio_nstby))
 		gpio_free(info->gpio_nstby);
 
+	media_entity_cleanup(&sd->entity);
 	kfree(info);
 	return 0;
 }
-- 
1.7.6

Re: [PATCH v3 1/3] noon010pc30: Conversion to the media controller API

[Laurent Pinchart]

Hi Sylwester,

Thanks for the patch.

On Friday 16 September 2011 17:59:54 Sylwester Nawrocki wrote:
> Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations.
> Add media entity initialization and set subdev flags so the host driver
> creates a subdev device node for the driver.
> A mutex was added for serializing the subdev operations. When setting
> format is attempted during streaming an (EBUSY) error will be returned.
> 
> After the device is powered up it will now remain in "power sleep"
> mode until s_stream(1) is called. The "power sleep" mode is used
> to suspend/resume frame generation at the sensor's output through
> s_stream op.
> 
> While at here simplify the colorspace parameter handling.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

[snip]

> diff --git a/drivers/media/video/noon010pc30.c
> b/drivers/media/video/noon010pc30.c index 35f722a..115d976 100644
> --- a/drivers/media/video/noon010pc30.c
> +++ b/drivers/media/video/noon010pc30.c

[snip]

> @@ -599,6 +641,22 @@ static int noon010_log_status(struct v4l2_subdev *sd)
>  	return 0;
>  }
> 
> +static int noon010_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(fh, 0);
> +	struct noon010_info *info = to_noon010(sd);
> +
> +	mutex_lock(&info->lock);
> +	noon010_get_current_fmt(to_noon010(sd), mf);

Should you initialize mf with a constant default format instead of retrieving 
the current format from the sensor ? A non-constant default would probably 
confuse userspace application.

> +
> +	mutex_unlock(&info->lock);
> +	return 0;
> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/3] noon010pc30: Conversion to the media controller API

[Sylwester Nawrocki]

Hi Laurent,

thanks for the review.

On 09/21/2011 12:18 AM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> Thanks for the patch.
> 
> On Friday 16 September 2011 17:59:54 Sylwester Nawrocki wrote:
>> Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations.
>> Add media entity initialization and set subdev flags so the host driver
>> creates a subdev device node for the driver.
>> A mutex was added for serializing the subdev operations. When setting
>> format is attempted during streaming an (EBUSY) error will be returned.
>>
>> After the device is powered up it will now remain in "power sleep"
>> mode until s_stream(1) is called. The "power sleep" mode is used
>> to suspend/resume frame generation at the sensor's output through
>> s_stream op.
>>
>> While at here simplify the colorspace parameter handling.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> [snip]
> 
>> diff --git a/drivers/media/video/noon010pc30.c
>> b/drivers/media/video/noon010pc30.c index 35f722a..115d976 100644
>> --- a/drivers/media/video/noon010pc30.c
>> +++ b/drivers/media/video/noon010pc30.c
> 
> [snip]
> 
>> @@ -599,6 +641,22 @@ static int noon010_log_status(struct v4l2_subdev *sd)
>>  	return 0;
>>  }
>>
>> +static int noon010_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> +	struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(fh, 0);
>> +	struct noon010_info *info = to_noon010(sd);
>> +
>> +	mutex_lock(&info->lock);
>> +	noon010_get_current_fmt(to_noon010(sd), mf);
> 
> Should you initialize mf with a constant default format instead of retrieving 
> the current format from the sensor ? A non-constant default would probably 
> confuse userspace application.

Sure, I could change to it. But I don't quite see the problem, the applications
should call set_fmt(TRY) anyway, rather than relying on the format gotten right
after opening the device, shouldn't they ? Anyway I guess it's better to have
all drivers behaving consistently.


Regards,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH v4] noon010pc30: Conversion to the media controller API

[Sylwester Nawrocki]

Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations.
Add media entity initialization and set subdev flags so the host driver
creates a subdev device node for the driver.
A mutex was added for serializing the subdev operations. When setting
format is attempted during streaming an (EBUSY) error will be returned.

After the device is powered up it will now remain in "power sleep"
mode until s_stream(1) is called. The "power sleep" mode is used
to suspend/resume frame generation at the sensor's output through
s_stream op.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Addresing Laurent's comments, changes since v3:
 - set constant default TRY format in subdev open() internal op
   rather than the currently set format
---
 drivers/media/video/Kconfig       |    2 +-
 drivers/media/video/noon010pc30.c |  221 +++++++++++++++++++++++++-----------
 2 files changed, 154 insertions(+), 69 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 6279663..75bb46f 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -755,7 +755,7 @@ config VIDEO_VIA_CAMERA
 
 config VIDEO_NOON010PC30
 	tristate "NOON010PC30 CIF camera sensor support"
-	depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2 && EXPERIMENTAL && VIDEO_V4L2_SUBDEV_API
 	---help---
 	  This driver supports NOON010PC30 CIF camera from Siliconfile
 
diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
index 35f722a..1a874ec 100644
--- a/drivers/media/video/noon010pc30.c
+++ b/drivers/media/video/noon010pc30.c
@@ -1,7 +1,7 @@
 /*
  * Driver for SiliconFile NOON010PC30 CIF (1/11") Image Sensor with ISP
  *
- * Copyright (C) 2010 Samsung Electronics
+ * Copyright (C) 2010 - 2011 Samsung Electronics Co., Ltd.
  * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com>
  *
  * Initial register configuration based on a driver authored by
@@ -10,7 +10,7 @@
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later vergsion.
+ * (at your option) any later version.
  */
 
 #include <linux/delay.h>
@@ -131,17 +131,24 @@ static const char * const noon010_supply_name[] = {
 
 struct noon010_info {
 	struct v4l2_subdev sd;
+	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;
 	const struct noon010pc30_platform_data *pdata;
+	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
+	u32 gpio_nreset;
+	u32 gpio_nstby;
+
+	/* Protects the struct members below */
+	struct mutex lock;
+
 	const struct noon010_format *curr_fmt;
 	const struct noon010_frmsize *curr_win;
+	unsigned int apply_new_cfg:1;
+	unsigned int streaming:1;
 	unsigned int hflip:1;
 	unsigned int vflip:1;
 	unsigned int power:1;
 	u8 i2c_reg_page;
-	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
-	u32 gpio_nreset;
-	u32 gpio_nstby;
 };
 
 struct i2c_regval {
@@ -313,6 +320,7 @@ static int noon010_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+/* Called with struct noon010_info.lock mutex held */
 static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
 {
 	struct noon010_info *info = to_noon010(sd);
@@ -340,21 +348,18 @@ static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
 static int noon010_set_params(struct v4l2_subdev *sd)
 {
 	struct noon010_info *info = to_noon010(sd);
-	int ret;
 
-	if (!info->curr_win)
-		return -EINVAL;
-
-	ret = cam_i2c_write(sd, VDO_CTL_REG(0), info->curr_win->vid_ctl1);
-
-	if (!ret && info->curr_fmt)
-		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
-				info->curr_fmt->ispctl1_reg);
-	return ret;
+	int ret = cam_i2c_write(sd, VDO_CTL_REG(0),
+				info->curr_win->vid_ctl1);
+	if (ret)
+		return ret;
+	return cam_i2c_write(sd, ISP_CTL_REG(0),
+			     info->curr_fmt->ispctl1_reg);
 }
 
 /* Find nearest matching image pixel size. */
-static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
+static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf,
+				  const struct noon010_frmsize **size)
 {
 	unsigned int min_err = ~0;
 	int i = ARRAY_SIZE(noon010_sizes);
@@ -374,11 +379,14 @@ static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
 	if (match) {
 		mf->width  = match->width;
 		mf->height = match->height;
+		if (size)
+			*size = match;
 		return 0;
 	}
 	return -EINVAL;
 }
 
+/* Called with info.lock mutex held */
 static int power_enable(struct noon010_info *info)
 {
 	int ret;
@@ -419,6 +427,7 @@ static int power_enable(struct noon010_info *info)
 	return 0;
 }
 
+/* Called with info.lock mutex held */
 static int power_disable(struct noon010_info *info)
 {
 	int ret;
@@ -448,93 +457,120 @@ static int power_disable(struct noon010_info *info)
 static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = to_sd(ctrl);
+	struct noon010_info *info = to_noon010(sd);
+	int ret = 0;
 
 	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
 		 __func__, ctrl->id, ctrl->val);
 
+	mutex_lock(&info->lock);
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any controls to H/W at this time. Instead
+	 * the controls will be restored right after power-up.
+	 */
+	if (!info->power)
+		goto unlock;
+
 	switch (ctrl->id) {
 	case V4L2_CID_AUTO_WHITE_BALANCE:
-		return noon010_enable_autowhitebalance(sd, ctrl->val);
+		ret = noon010_enable_autowhitebalance(sd, ctrl->val);
+		break;
 	case V4L2_CID_BLUE_BALANCE:
-		return cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
+		ret = cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
+		break;
 	case V4L2_CID_RED_BALANCE:
-		return cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
+		ret =  cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+unlock:
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
-static int noon010_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
-			    enum v4l2_mbus_pixelcode *code)
+static int noon010_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (!code || index >= ARRAY_SIZE(noon010_formats))
+	if (code->index >= ARRAY_SIZE(noon010_formats))
 		return -EINVAL;
 
-	*code = noon010_formats[index].code;
+	code->code = noon010_formats[code->index].code;
 	return 0;
 }
 
-static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
+static int noon010_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			   struct v4l2_subdev_format *fmt)
 {
 	struct noon010_info *info = to_noon010(sd);
-	int ret;
-
-	if (!mf)
-		return -EINVAL;
+	struct v4l2_mbus_framefmt *mf;
 
-	if (!info->curr_win || !info->curr_fmt) {
-		ret = noon010_set_params(sd);
-		if (ret)
-			return ret;
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (fh) {
+			mf = v4l2_subdev_get_try_format(fh, 0);
+			fmt->format = *mf;
+		}
+		return 0;
 	}
+	mf = &fmt->format;
 
-	mf->width	= info->curr_win->width;
-	mf->height	= info->curr_win->height;
-	mf->code	= info->curr_fmt->code;
-	mf->colorspace	= info->curr_fmt->colorspace;
-	mf->field	= V4L2_FIELD_NONE;
+	mutex_lock(&info->lock);
+	mf->width = info->curr_win->width;
+	mf->height = info->curr_win->height;
+	mf->code = info->curr_fmt->code;
+	mf->colorspace = info->curr_fmt->colorspace;
+	mf->field = V4L2_FIELD_NONE;
 
+	mutex_unlock(&info->lock);
 	return 0;
 }
 
 /* Return nearest media bus frame format. */
-static const struct noon010_format *try_fmt(struct v4l2_subdev *sd,
+static const struct noon010_format *noon010_try_fmt(struct v4l2_subdev *sd,
 					    struct v4l2_mbus_framefmt *mf)
 {
 	int i = ARRAY_SIZE(noon010_formats);
 
-	noon010_try_frame_size(mf);
-
-	while (i--)
+	while (--i)
 		if (mf->code == noon010_formats[i].code)
 			break;
-
 	mf->code = noon010_formats[i].code;
 
 	return &noon010_formats[i];
 }
 
-static int noon010_try_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_mbus_framefmt *mf)
-{
-	if (!sd || !mf)
-		return -EINVAL;
-
-	try_fmt(sd, mf);
-	return 0;
-}
-
-static int noon010_s_fmt(struct v4l2_subdev *sd,
-			 struct v4l2_mbus_framefmt *mf)
+static int noon010_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			   struct v4l2_subdev_format *fmt)
 {
 	struct noon010_info *info = to_noon010(sd);
+	const struct noon010_frmsize *size = NULL;
+	const struct noon010_format *nf;
+	struct v4l2_mbus_framefmt *mf;
+	int ret = 0;
 
-	if (!sd || !mf)
-		return -EINVAL;
-
-	info->curr_fmt = try_fmt(sd, mf);
+	nf = noon010_try_fmt(sd, &fmt->format);
+	noon010_try_frame_size(&fmt->format, &size);
+	fmt->format.colorspace = V4L2_COLORSPACE_JPEG;
 
-	return noon010_set_params(sd);
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (fh) {
+			mf = v4l2_subdev_get_try_format(fh, 0);
+			*mf = fmt->format;
+		}
+		return 0;
+	}
+	mutex_lock(&info->lock);
+	if (!info->streaming) {
+		info->apply_new_cfg = 1;
+		info->curr_fmt = nf;
+		info->curr_win = size;
+	} else {
+		ret = -EBUSY;
+	}
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
 static int noon010_base_config(struct v4l2_subdev *sd)
@@ -550,8 +586,6 @@ static int noon010_base_config(struct v4l2_subdev *sd)
 	}
 	if (!ret)
 		ret = noon010_set_flip(sd, 1, 0);
-	if (!ret)
-		ret = noon010_power_ctrl(sd, false, false);
 
 	/* sync the handler and the registers state */
 	v4l2_ctrl_handler_setup(&to_noon010(sd)->hdl);
@@ -582,6 +616,26 @@ static int noon010_s_power(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+static int noon010_s_stream(struct v4l2_subdev *sd, int on)
+{
+	struct noon010_info *info = to_noon010(sd);
+	int ret = 0;
+
+	mutex_lock(&info->lock);
+	if (!info->streaming != !on) {
+		ret = noon010_power_ctrl(sd, false, !on);
+		if (!ret)
+			info->streaming = on;
+	}
+	if (!ret && on && info->apply_new_cfg) {
+		ret = noon010_set_params(sd);
+		if (!ret)
+			info->apply_new_cfg = 0;
+	}
+	mutex_unlock(&info->lock);
+	return ret;
+}
+
 static int noon010_g_chip_ident(struct v4l2_subdev *sd,
 				struct v4l2_dbg_chip_ident *chip)
 {
@@ -599,6 +653,22 @@ static int noon010_log_status(struct v4l2_subdev *sd)
 	return 0;
 }
 
+static int noon010_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(fh, 0);
+
+	mf->width = noon010_sizes[0].width;
+	mf->height = noon010_sizes[0].height;
+	mf->code = noon010_formats[0].code;
+	mf->colorspace = V4L2_COLORSPACE_JPEG;
+	mf->field = V4L2_FIELD_NONE;
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops noon010_subdev_internal_ops = {
+	.open = noon010_open,
+};
+
 static const struct v4l2_ctrl_ops noon010_ctrl_ops = {
 	.s_ctrl = noon010_s_ctrl,
 };
@@ -616,15 +686,19 @@ static const struct v4l2_subdev_core_ops noon010_core_ops = {
 	.log_status	= noon010_log_status,
 };
 
-static const struct v4l2_subdev_video_ops noon010_video_ops = {
-	.g_mbus_fmt	= noon010_g_fmt,
-	.s_mbus_fmt	= noon010_s_fmt,
-	.try_mbus_fmt	= noon010_try_fmt,
-	.enum_mbus_fmt	= noon010_enum_fmt,
+static struct v4l2_subdev_pad_ops noon010_pad_ops = {
+	.enum_mbus_code	= noon010_enum_mbus_code,
+	.get_fmt	= noon010_get_fmt,
+	.set_fmt	= noon010_set_fmt,
+};
+
+static struct v4l2_subdev_video_ops noon010_video_ops = {
+	.s_stream	= noon010_s_stream,
 };
 
 static const struct v4l2_subdev_ops noon010_ops = {
 	.core	= &noon010_core_ops,
+	.pad	= &noon010_pad_ops,
 	.video	= &noon010_video_ops,
 };
 
@@ -665,10 +739,14 @@ static int noon010_probe(struct i2c_client *client,
 	if (!info)
 		return -ENOMEM;
 
+	mutex_init(&info->lock);
 	sd = &info->sd;
 	strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
 	v4l2_i2c_subdev_init(sd, client, &noon010_ops);
 
+	sd->internal_ops = &noon010_subdev_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
 	v4l2_ctrl_handler_init(&info->hdl, 3);
 
 	v4l2_ctrl_new_std(&info->hdl, &noon010_ctrl_ops,
@@ -719,11 +797,17 @@ static int noon010_probe(struct i2c_client *client,
 	if (ret)
 		goto np_reg_err;
 
+	info->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
+	ret = media_entity_init(&sd->entity, 1, &info->pad, 0);
+	if (ret < 0)
+		goto np_me_err;
+
 	ret = noon010_detect(client, info);
 	if (!ret)
 		return 0;
 
-	/* the sensor detection failed */
+np_me_err:
 	regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply);
 np_reg_err:
 	if (gpio_is_valid(info->gpio_nstby))
@@ -754,6 +838,7 @@ static int noon010_remove(struct i2c_client *client)
 	if (gpio_is_valid(info->gpio_nstby))
 		gpio_free(info->gpio_nstby);
 
+	media_entity_cleanup(&sd->entity);
 	kfree(info);
 	return 0;
 }
-- 
1.7.6.3

Re: [PATCH v4] noon010pc30: Conversion to the media controller API

[Laurent Pinchart]

Hi Sylwester,

Thanks for the patch.

On Wednesday 21 September 2011 16:26:00 Sylwester Nawrocki wrote:
> Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations.
> Add media entity initialization and set subdev flags so the host driver
> creates a subdev device node for the driver.
> A mutex was added for serializing the subdev operations. When setting
> format is attempted during streaming an (EBUSY) error will be returned.
> 
> After the device is powered up it will now remain in "power sleep"
> mode until s_stream(1) is called. The "power sleep" mode is used
> to suspend/resume frame generation at the sensor's output through
> s_stream op.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart