[PATCH 0/3] noon010pc30 driver conversion to the pad level operations

[PATCH 0/3] noon010pc30 driver conversion to the pad level operations

[Sylwester Nawrocki]

The following patch set converts noon010pc30 camera sensor driver to the 
subdev pad level operations. Additionally it cleans up some leftover after
conversion to the regulator framework and marks driver as experimental
as it now uses V4L2 sub-device API.


Sylwester Nawrocki (3):
  noon010pc30: Do not ignore errors in initial controls setup
  noon010pc30: Convert to the pad level ops
  noon010pc30: Clean up the s_power callback

 drivers/media/video/Kconfig       |    2 +-
 drivers/media/video/noon010pc30.c |  151 ++++++++++++++++++++++++-------------
 2 files changed, 98 insertions(+), 55 deletions(-)


--
Regards,
Sylwester

[PATCH 1/3] noon010pc30: Do not ignore errors in initial controls setup

[Sylwester Nawrocki]

Propagate return value from v4l2_ctrl_handler_setup as any
errors from it are now silently ignored.

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

diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
index 35f722a..50ca097 100644
--- a/drivers/media/video/noon010pc30.c
+++ b/drivers/media/video/noon010pc30.c
@@ -554,7 +554,8 @@ static int noon010_base_config(struct v4l2_subdev *sd)
 		ret = noon010_power_ctrl(sd, false, false);
 
 	/* sync the handler and the registers state */
-	v4l2_ctrl_handler_setup(&to_noon010(sd)->hdl);
+	if (!ret)
+		ret = v4l2_ctrl_handler_setup(&info->hdl);
 	return ret;
 }
 
-- 
1.7.5.4

[PATCH 2/3] noon010pc30: Convert to the pad level ops

[Sylwester Nawrocki]

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

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 |  130 +++++++++++++++++++++++++------------
 2 files changed, 90 insertions(+), 42 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 8de3476..b505120 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -746,7 +746,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 50ca097..6920cc4 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
  * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com>
  *
  * Initial register configuration based on a driver authored by
@@ -130,14 +130,19 @@ static const char * const noon010_supply_name[] = {
 #define NOON010_NUM_SUPPLIES ARRAY_SIZE(noon010_supply_name)
 
 struct noon010_info {
+	/* Mutex protecting this data structure and subdev operations */
+	struct mutex lock;
 	struct v4l2_subdev sd;
+	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;
 	const struct noon010pc30_platform_data *pdata;
 	const struct noon010_format *curr_fmt;
 	const struct noon010_frmsize *curr_win;
+	struct v4l2_mbus_framefmt format;
 	unsigned int hflip:1;
 	unsigned int vflip:1;
 	unsigned int power:1;
+	unsigned int streaming:1;
 	u8 i2c_reg_page;
 	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
 	u32 gpio_nreset;
@@ -342,7 +347,7 @@ static int noon010_set_params(struct v4l2_subdev *sd)
 	struct noon010_info *info = to_noon010(sd);
 	int ret;
 
-	if (!info->curr_win)
+	if (!info->curr_win || !info->power)
 		return -EINVAL;
 
 	ret = cam_i2c_write(sd, VDO_CTL_REG(0), info->curr_win->vid_ctl1);
@@ -354,7 +359,8 @@ static int noon010_set_params(struct v4l2_subdev *sd)
 }
 
 /* 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,6 +380,8 @@ 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;
@@ -464,36 +472,45 @@ static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)
 	}
 }
 
-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 || 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;
+	struct v4l2_mbus_framefmt *mf;
 
-	if (!mf)
+	if (fmt->pad != 0)
 		return -EINVAL;
 
-	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;
 	}
+	/* Active format */
+	mf = &fmt->format;
 
+	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);
 
+	mf->field	= V4L2_FIELD_NONE;
+	mf->colorspace	= V4L2_COLORSPACE_JPEG;
 	return 0;
 }
 
@@ -503,38 +520,47 @@ static const struct noon010_format *try_fmt(struct v4l2_subdev *sd,
 {
 	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;
 
-	if (!sd || !mf)
+	if (fmt->pad != 0)
 		return -EINVAL;
 
-	info->curr_fmt = try_fmt(sd, mf);
+	nf = 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) {
+		ret = -EBUSY;
+	} else {
+		info->curr_fmt = nf;
+		info->curr_win = size;
+		ret = noon010_set_params(sd);
+	}
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
 static int noon010_base_config(struct v4l2_subdev *sd)
@@ -583,6 +609,17 @@ 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);
+
+	mutex_lock(&info->lock);
+	info->streaming = on;
+	mutex_unlock(&info->lock);
+
+	return 0;
+}
+
 static int noon010_g_chip_ident(struct v4l2_subdev *sd,
 				struct v4l2_dbg_chip_ident *chip)
 {
@@ -617,15 +654,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,
 };
 
@@ -666,9 +707,11 @@ 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->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	v4l2_ctrl_handler_init(&info->hdl, 3);
 
@@ -720,11 +763,16 @@ static int noon010_probe(struct i2c_client *client,
 	if (ret)
 		goto np_reg_err;
 
+	info->pad.flags = MEDIA_PAD_FL_SOURCE;
+	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))
@@ -751,10 +799,10 @@ static int noon010_remove(struct i2c_client *client)
 
 	if (gpio_is_valid(info->gpio_nreset))
 		gpio_free(info->gpio_nreset);
-
 	if (gpio_is_valid(info->gpio_nstby))
 		gpio_free(info->gpio_nstby);
 
+	media_entity_cleanup(&sd->entity);
 	kfree(info);
 	return 0;
 }
-- 
1.7.5.4

[PATCH 3/3] noon010pc30: Clean up the s_power callback

[Sylwester Nawrocki]

Remove unneeded check for the platform data in s_power operation
and reorder the code to use early return path. There is no functional
changes in this patch.

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

diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
index 6920cc4..99b58d0 100644
--- a/drivers/media/video/noon010pc30.c
+++ b/drivers/media/video/noon010pc30.c
@@ -588,25 +588,19 @@ static int noon010_base_config(struct v4l2_subdev *sd)
 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;
 
 	if (on) {
 		ret = power_enable(info);
 		if (ret)
 			return 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;
+		return noon010_base_config(sd);
 	}
 
-	return ret;
+	noon010_power_ctrl(sd, false, true);
+	info->curr_win = NULL;
+	info->curr_fmt = NULL;
+	return power_disable(info);
 }
 
 static int noon010_s_stream(struct v4l2_subdev *sd, int on)
-- 
1.7.5.4

Re: [PATCH 2/3] noon010pc30: Convert to the pad level ops

[Laurent Pinchart]

Hi Sylwester,

Thanks for the patch. It's nice to see sensor drivers picking up the pad-level 
API :-)

On Wednesday 22 June 2011 17:44:29 Sylwester Nawrocki wrote:
> Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations.
> Add media entity initalization and set subdev flags so the host driver
> creates a v4l-subdev device node for the driver. A mutex is added for
> serializing operations on subdevice node. When setting format
> is attempted during streaming EBUSY error code will be returned.

[snip]

> @@ -130,14 +130,19 @@ static const char * const noon010_supply_name[] = {
>  #define NOON010_NUM_SUPPLIES ARRAY_SIZE(noon010_supply_name)
> 
>  struct noon010_info {
> +	/* Mutex protecting this data structure and subdev operations */
> +	struct mutex lock;

Locks protect data, not operations. You should describe which data members are 
protected by the lock.

>  	struct v4l2_subdev sd;
> +	struct media_pad pad;
>  	struct v4l2_ctrl_handler hdl;
>  	const struct noon010pc30_platform_data *pdata;
>  	const struct noon010_format *curr_fmt;
>  	const struct noon010_frmsize *curr_win;
> +	struct v4l2_mbus_framefmt format;
>  	unsigned int hflip:1;
>  	unsigned int vflip:1;
>  	unsigned int power:1;
> +	unsigned int streaming:1;
>  	u8 i2c_reg_page;
>  	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
>  	u32 gpio_nreset;

[snip]

> @@ -374,6 +380,8 @@ 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;
> @@ -464,36 +472,45 @@ static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)

[snip]

> -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;
> +	struct v4l2_mbus_framefmt *mf;
> 
> -	if (!mf)
> +	if (fmt->pad != 0)
>  		return -EINVAL;

subdev_do_ioctl() already validates fmt->pad.

> -	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;
>  	}
> +	/* Active format */
> +	mf = &fmt->format;
> 
> +	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);
> 
> +	mf->field	= V4L2_FIELD_NONE;
> +	mf->colorspace	= V4L2_COLORSPACE_JPEG;
>  	return 0;
>  }
> 

[snip]

> @@ -583,6 +609,17 @@ 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);
> +
> +	mutex_lock(&info->lock);
> +	info->streaming = on;
> +	mutex_unlock(&info->lock);

Does the sensor produce data continuously, without any way to stop it ?

> +
> +	return 0;
> +}
> +
>  static int noon010_g_chip_ident(struct v4l2_subdev *sd,
>  				struct v4l2_dbg_chip_ident *chip)

You can get rid of g_chip_ident as well.

>  {

[snip]

> @@ -666,9 +707,11 @@ 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->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;

You should |= V4L2_SUBDEV_FL_HAS_DEVNODE flag. v4l2_i2c_subdev_init() sets 
V4L2_SUBDEV_FL_IS_I2C.

>  	v4l2_ctrl_handler_init(&info->hdl, 3);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] noon010pc30: Convert to the pad level ops

[Sylwester Nawrocki]

Hi Laurent,

thanks for your review.

On 06/25/2011 02:08 AM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> Thanks for the patch. It's nice to see sensor drivers picking up the pad-level
> API :-)

This is somehow a consequence of converting our camera host driver
to the pad-level API. Nevertheless for some of our devices the pad-level API
just scales better than regular V4L2 interface. So my goal is to gradually
introduce it in our BSP where relevant.

> 
> On Wednesday 22 June 2011 17:44:29 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 v4l-subdev device node for the driver. A mutex is added for
>> serializing operations on subdevice node. When setting format
>> is attempted during streaming EBUSY error code will be returned.
> 
> [snip]
> 
>> @@ -130,14 +130,19 @@ static const char * const noon010_supply_name[] = {
>>   #define NOON010_NUM_SUPPLIES ARRAY_SIZE(noon010_supply_name)
>>
>>   struct noon010_info {
>> +	/* Mutex protecting this data structure and subdev operations */
>> +	struct mutex lock;
> 
> Locks protect data, not operations. You should describe which data members are
> protected by the lock.

OK, thanks for pointing this out. I'll try to be more precise in the next
patch version.:)

> 
>>   	struct v4l2_subdev sd;
>> +	struct media_pad pad;
>>   	struct v4l2_ctrl_handler hdl;
>>   	const struct noon010pc30_platform_data *pdata;
>>   	const struct noon010_format *curr_fmt;
>>   	const struct noon010_frmsize *curr_win;
>> +	struct v4l2_mbus_framefmt format;
>>   	unsigned int hflip:1;
>>   	unsigned int vflip:1;
>>   	unsigned int power:1;
>> +	unsigned int streaming:1;
>>   	u8 i2c_reg_page;
>>   	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
>>   	u32 gpio_nreset;
> 
> [snip]
> 
>> @@ -374,6 +380,8 @@ 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;
>> @@ -464,36 +472,45 @@ static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)
> 
> [snip]
> 
>> -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;
>> +	struct v4l2_mbus_framefmt *mf;
>>
>> -	if (!mf)
>> +	if (fmt->pad != 0)
>>   		return -EINVAL;
> 
> subdev_do_ioctl() already validates fmt->pad.

OK, I'll get rid of the check.

> 
>> -	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;
>>   	}
>> +	/* Active format */
>> +	mf =&fmt->format;
>>
>> +	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);
>>
>> +	mf->field	= V4L2_FIELD_NONE;
>> +	mf->colorspace	= V4L2_COLORSPACE_JPEG;
>>   	return 0;
>>   }
>>
> 
> [snip]
> 
>> @@ -583,6 +609,17 @@ 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);
>> +
>> +	mutex_lock(&info->lock);
>> +	info->streaming = on;
>> +	mutex_unlock(&info->lock);
> 
> Does the sensor produce data continuously, without any way to stop it ?

Hmm, looks like not enough attention to that from my side. The sensor has
a software "power sleep" mode in which it is supposed to not generate 
an output signal and it tri-states its output pins. 
All registers' state is retained and the normal I2C register access should
be possible. I'll look into details in the datasheet. I think the driver
could be leaving the sensor chip in such 'suspended' state after s_power(1)
and be switching it into normal operation within s_stream(1).

> 
>> +
>> +	return 0;
>> +}
>> +
>>   static int noon010_g_chip_ident(struct v4l2_subdev *sd,
>>   				struct v4l2_dbg_chip_ident *chip)
> 
> You can get rid of g_chip_ident as well.

All right, when I was originally writing this driver I thought
it was mandatory to implement g_chip_indent. In fact it was never
really used so far, so I'm going to do away with it in next iteration.

> 
>>   {
> 
> [snip]
> 
>> @@ -666,9 +707,11 @@ 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->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> 
> You should |= V4L2_SUBDEV_FL_HAS_DEVNODE flag. v4l2_i2c_subdev_init() sets
> V4L2_SUBDEV_FL_IS_I2C.

Oops, my bad. Thanks, I'll fix this.

> 
>>   	v4l2_ctrl_handler_init(&info->hdl, 3);
> 

--
Regards,
Sylwester