[PATCH 0/3] Convert SR030PC30 driver to use the control framework and the regulator API

[PATCH 0/3] Convert SR030PC30 driver to use the control framework and the regulator API

[Sylwester Nawrocki]

Hello,

the following patch set is an update for the sr030pc30 sensor driver.
The second patch converts the driver to use the control framework and
adds two new controls as it required relatively little effort to have 
them exported.
The third patch replaces the set_power callback with regulator framework
calls, the RESET and STANDBY gpios are passed as platform data and are
now directly managed in the driver, rather than having the power handling
code repeated in each board setup file. 


The patch series contains:

[PATCH 1/3] sr030pc30: Remove empty s_stream op
[PATCH 2/3] sr030pc30: Use the control framework
[PATCH 3/3] sr030pc30: Add regulator framework support


Regards,
Sylwester



--
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH 1/3] sr030pc30: Remove empty s_stream op

[Sylwester Nawrocki]

s_stream does nothing in current form so remove it.

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

diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
index c901721..e1eced1 100644
--- a/drivers/media/video/sr030pc30.c
+++ b/drivers/media/video/sr030pc30.c
@@ -714,11 +714,6 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
 	return ret;
 }
 
-static int sr030pc30_s_stream(struct v4l2_subdev *sd, int enable)
-{
-	return 0;
-}
-
 static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -761,7 +756,6 @@ static const struct v4l2_subdev_core_ops sr030pc30_core_ops = {
 };
 
 static const struct v4l2_subdev_video_ops sr030pc30_video_ops = {
-	.s_stream	= sr030pc30_s_stream,
 	.g_mbus_fmt	= sr030pc30_g_fmt,
 	.s_mbus_fmt	= sr030pc30_s_fmt,
 	.try_mbus_fmt	= sr030pc30_try_fmt,
-- 
1.7.0.4

Re: [PATCH 1/3] sr030pc30: Remove empty s_stream op

[Mauro Carvalho Chehab]

Em 19-01-2011 23:44, Sylwester Nawrocki escreveu:
> s_stream does nothing in current form so remove it.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungin.park@samsung.com>
> ---
>  drivers/media/video/sr030pc30.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
> index c901721..e1eced1 100644
> --- a/drivers/media/video/sr030pc30.c
> +++ b/drivers/media/video/sr030pc30.c
> @@ -714,11 +714,6 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
>  	return ret;
>  }
>  
> -static int sr030pc30_s_stream(struct v4l2_subdev *sd, int enable)
> -{
> -	return 0;
> -}
> -
>  static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> @@ -761,7 +756,6 @@ static const struct v4l2_subdev_core_ops sr030pc30_core_ops = {
>  };
>  
>  static const struct v4l2_subdev_video_ops sr030pc30_video_ops = {
> -	.s_stream	= sr030pc30_s_stream,
>  	.g_mbus_fmt	= sr030pc30_g_fmt,
>  	.s_mbus_fmt	= sr030pc30_s_fmt,
>  	.try_mbus_fmt	= sr030pc30_try_fmt,

Hmm...
this patch[1] were never merged. It seems a cleanup, though.

Care to review it?

Thanks!
Mauro

[1] http://patchwork.linuxtv.org/patch/5631/

Re: [PATCH 1/3] sr030pc30: Remove empty s_stream op

[Sylwester Nawrocki]

On 09/21/2011 05:22 PM, Mauro Carvalho Chehab wrote:
> Em 19-01-2011 23:44, Sylwester Nawrocki escreveu:
>> s_stream does nothing in current form so remove it.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungin.park@samsung.com>
>> ---
>>  drivers/media/video/sr030pc30.c |    6 ------
>>  1 files changed, 0 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
>> index c901721..e1eced1 100644
>> --- a/drivers/media/video/sr030pc30.c
>> +++ b/drivers/media/video/sr030pc30.c
>> @@ -714,11 +714,6 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
>>  	return ret;
>>  }
>>  
>> -static int sr030pc30_s_stream(struct v4l2_subdev *sd, int enable)
>> -{
>> -	return 0;
>> -}
>> -
>>  static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
>>  {
>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> @@ -761,7 +756,6 @@ static const struct v4l2_subdev_core_ops sr030pc30_core_ops = {
>>  };
>>  
>>  static const struct v4l2_subdev_video_ops sr030pc30_video_ops = {
>> -	.s_stream	= sr030pc30_s_stream,
>>  	.g_mbus_fmt	= sr030pc30_g_fmt,
>>  	.s_mbus_fmt	= sr030pc30_s_fmt,
>>  	.try_mbus_fmt	= sr030pc30_try_fmt,
> 
> Hmm...
> this patch[1] were never merged. It seems a cleanup, though.
> 
> Care to review it?

Indeed, it is still worth to apply. There was some ongoing work
at the control framework and other patches form this series need some
more modifications. But this one alone can be merged.

> 
> Thanks!
> Mauro
> 
> [1] http://patchwork.linuxtv.org/patch/5631/
> 
> 

Thank you,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

[PATCH 2/3] sr030pc30: Use the control framework

[Sylwester Nawrocki]

Implement controls using the control framework.
Add horizontal/vertical flip controls, minor cleanup.

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

diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
index e1eced1..1a195f0 100644
--- a/drivers/media/video/sr030pc30.c
+++ b/drivers/media/video/sr030pc30.c
@@ -19,6 +19,8 @@
 #include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 #include <media/v4l2-mediabus.h>
@@ -141,17 +143,24 @@ module_param(debug, int, 0644);
 
 struct sr030pc30_info {
 	struct v4l2_subdev sd;
+	struct v4l2_ctrl_handler hdl;
+	struct {
+		/* exposure/auto-exposure cluster */
+		struct v4l2_ctrl *autoexposure;
+		struct v4l2_ctrl *exposure;
+	};
+	struct {
+		/* blue/red/autowhitebalance cluster */
+		struct v4l2_ctrl *autowb;
+		struct v4l2_ctrl *blue;
+		struct v4l2_ctrl *red;
+	};
+	struct v4l2_ctrl *hflip;
+	struct v4l2_ctrl *vflip;
 	const struct sr030pc30_platform_data *pdata;
 	const struct sr030pc30_format *curr_fmt;
 	const struct sr030pc30_frmsize *curr_win;
-	unsigned int auto_wb:1;
-	unsigned int auto_exp:1;
-	unsigned int hflip:1;
-	unsigned int vflip:1;
 	unsigned int sleep:1;
-	unsigned int exposure;
-	u8 blue_balance;
-	u8 red_balance;
 	u8 i2c_reg_page;
 };
 
@@ -172,52 +181,6 @@ struct i2c_regval {
 	u16 val;
 };
 
-static const struct v4l2_queryctrl sr030pc30_ctrl[] = {
-	{
-		.id		= V4L2_CID_AUTO_WHITE_BALANCE,
-		.type		= V4L2_CTRL_TYPE_BOOLEAN,
-		.name		= "Auto White Balance",
-		.minimum	= 0,
-		.maximum	= 1,
-		.step		= 1,
-		.default_value	= 1,
-	}, {
-		.id		= V4L2_CID_RED_BALANCE,
-		.type		= V4L2_CTRL_TYPE_INTEGER,
-		.name		= "Red Balance",
-		.minimum	= 0,
-		.maximum	= 127,
-		.step		= 1,
-		.default_value	= 64,
-		.flags		= 0,
-	}, {
-		.id		= V4L2_CID_BLUE_BALANCE,
-		.type		= V4L2_CTRL_TYPE_INTEGER,
-		.name		= "Blue Balance",
-		.minimum	= 0,
-		.maximum	= 127,
-		.step		= 1,
-		.default_value	= 64,
-	}, {
-		.id		= V4L2_CID_EXPOSURE_AUTO,
-		.type		= V4L2_CTRL_TYPE_INTEGER,
-		.name		= "Auto Exposure",
-		.minimum	= 0,
-		.maximum	= 1,
-		.step		= 1,
-		.default_value	= 1,
-	}, {
-		.id		= V4L2_CID_EXPOSURE,
-		.type		= V4L2_CTRL_TYPE_INTEGER,
-		.name		= "Exposure",
-		.minimum	= EXPOS_MIN_MS,
-		.maximum	= EXPOS_MAX_MS,
-		.step		= 1,
-		.default_value	= 1,
-	}, {
-	}
-};
-
 /* supported resolutions */
 static const struct sr030pc30_frmsize sr030pc30_sizes[] = {
 	{
@@ -323,6 +286,11 @@ static inline struct sr030pc30_info *to_sr030pc30(struct v4l2_subdev *sd)
 	return container_of(sd, struct sr030pc30_info, sd);
 }
 
+static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
+{
+	return &container_of(ctrl->handler, struct sr030pc30_info, hdl)->sd;
+}
+
 static inline int set_i2c_page(struct sr030pc30_info *info,
 			       struct i2c_client *client, unsigned int reg)
 {
@@ -395,59 +363,56 @@ static int sr030pc30_pwr_ctrl(struct v4l2_subdev *sd,
 
 static inline int sr030pc30_enable_autoexposure(struct v4l2_subdev *sd, int on)
 {
-	struct sr030pc30_info *info = to_sr030pc30(sd);
 	/* auto anti-flicker is also enabled here */
-	int ret = cam_i2c_write(sd, AE_CTL1_REG, on ? 0xDC : 0x0C);
-	if (!ret)
-		info->auto_exp = on;
-	return ret;
+	return cam_i2c_write(sd, AE_CTL1_REG, on ? 0xDC : 0x0C);
 }
 
 static int sr030pc30_set_exposure(struct v4l2_subdev *sd, int value)
 {
 	struct sr030pc30_info *info = to_sr030pc30(sd);
-
 	unsigned long expos = value * info->pdata->clk_rate / (8 * 1000);
+	int ret;
 
-	int ret = cam_i2c_write(sd, EXP_TIMEH_REG, expos >> 16 & 0xFF);
+	ret = cam_i2c_write(sd, EXP_TIMEH_REG, expos >> 16 & 0xFF);
 	if (!ret)
 		ret = cam_i2c_write(sd, EXP_TIMEM_REG, expos >> 8 & 0xFF);
 	if (!ret)
 		ret = cam_i2c_write(sd, EXP_TIMEL_REG, expos & 0xFF);
-	if (!ret) { /* Turn off AE */
-		info->exposure = value;
+	if (!ret) /* Turn off AE */
 		ret = sr030pc30_enable_autoexposure(sd, 0);
-	}
+
 	return ret;
 }
 
 /* Automatic white balance control */
 static int sr030pc30_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
 {
-	struct sr030pc30_info *info = to_sr030pc30(sd);
+	int ret;
 
-	int ret = cam_i2c_write(sd, AWB_CTL2_REG, on ? 0x2E : 0x2F);
+	ret = cam_i2c_write(sd, AWB_CTL2_REG, on ? 0x2E : 0x2F);
 	if (!ret)
 		ret = cam_i2c_write(sd, AWB_CTL1_REG, on ? 0xFB : 0x7B);
-	if (!ret)
-		info->auto_wb = on;
 
 	return ret;
 }
 
-static int sr030pc30_set_flip(struct v4l2_subdev *sd)
+/**
+ * sr030pc30_set_flip - set image flipping
+ * @sd: a pointer to the subdev to apply the seetings to
+ * @hflip: 1 to enable or 0 to disable horizontal flip
+ * @vflip: as above but for vertical flip
+ */
+static int sr030pc30_set_flip(struct v4l2_subdev *sd, u32 hflip, u32 vflip)
 {
-	struct sr030pc30_info *info = to_sr030pc30(sd);
-
 	s32 reg = cam_i2c_read(sd, VDO_CTL2_REG);
+
 	if (reg < 0)
 		return reg;
 
 	reg &= 0x7C;
-	if (info->hflip)
-		reg |= 0x01;
-	if (info->vflip)
-		reg |= 0x02;
+	reg |= ((hflip & 0x1) << 0);
+	reg |= ((vflip & 0x1) << 1);
+
 	return cam_i2c_write(sd, VDO_CTL2_REG, reg | 0x80);
 }
 
@@ -468,8 +433,8 @@ static int sr030pc30_set_params(struct v4l2_subdev *sd)
 		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
 				info->curr_fmt->ispctl1_reg);
 	if (!ret)
-		ret = sr030pc30_set_flip(sd);
-
+		ret = sr030pc30_set_flip(sd, info->hflip->val,
+					 info->vflip->val);
 	return ret;
 }
 
@@ -497,108 +462,48 @@ static int sr030pc30_try_frame_size(struct v4l2_mbus_framefmt *mf)
 	return -EINVAL;
 }
 
-static int sr030pc30_queryctrl(struct v4l2_subdev *sd,
-			       struct v4l2_queryctrl *qc)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(sr030pc30_ctrl); i++)
-		if (qc->id == sr030pc30_ctrl[i].id) {
-			*qc = sr030pc30_ctrl[i];
-			v4l2_dbg(1, debug, sd, "%s id: %d\n",
-				 __func__, qc->id);
-			return 0;
-		}
-
-	return -EINVAL;
-}
-
-static inline int sr030pc30_set_bluebalance(struct v4l2_subdev *sd, int value)
-{
-	int ret = cam_i2c_write(sd, MWB_BGAIN_REG, value);
-	if (!ret)
-		to_sr030pc30(sd)->blue_balance = value;
-	return ret;
-}
-
-static inline int sr030pc30_set_redbalance(struct v4l2_subdev *sd, int value)
-{
-	int ret = cam_i2c_write(sd, MWB_RGAIN_REG, value);
-	if (!ret)
-		to_sr030pc30(sd)->red_balance = value;
-	return ret;
-}
-
-static int sr030pc30_s_ctrl(struct v4l2_subdev *sd,
-			    struct v4l2_control *ctrl)
+static int sr030pc30_s_ctrl(struct v4l2_ctrl *ctrl)
 {
-	int i, ret = 0;
-
-	for (i = 0; i < ARRAY_SIZE(sr030pc30_ctrl); i++)
-		if (ctrl->id == sr030pc30_ctrl[i].id)
-			break;
-
-	if (i == ARRAY_SIZE(sr030pc30_ctrl))
-		return -EINVAL;
-
-	if (ctrl->value < sr030pc30_ctrl[i].minimum ||
-		ctrl->value > sr030pc30_ctrl[i].maximum)
-			return -ERANGE;
+	struct v4l2_subdev *sd = to_sd(ctrl);
+	struct sr030pc30_info *info = to_sr030pc30(sd);
+	int ret = 0;
 
-	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
-			 __func__, ctrl->id, ctrl->value);
+	v4l2_dbg(1, debug, sd, "%s: ctrl id: %d, value: %d\n",
+			 __func__, ctrl->id, ctrl->val);
 
 	switch (ctrl->id) {
 	case V4L2_CID_AUTO_WHITE_BALANCE:
-		sr030pc30_enable_autowhitebalance(sd, ctrl->value);
-		break;
-	case V4L2_CID_BLUE_BALANCE:
-		ret = sr030pc30_set_bluebalance(sd, ctrl->value);
-		break;
-	case V4L2_CID_RED_BALANCE:
-		ret = sr030pc30_set_redbalance(sd, ctrl->value);
-		break;
-	case V4L2_CID_EXPOSURE_AUTO:
-		sr030pc30_enable_autoexposure(sd,
-			ctrl->value == V4L2_EXPOSURE_AUTO);
-		break;
-	case V4L2_CID_EXPOSURE:
-		ret = sr030pc30_set_exposure(sd, ctrl->value);
-		break;
-	default:
-		return -EINVAL;
-	}
+		if (!ctrl->has_new)
+			ctrl->val = 0;
 
-	return ret;
-}
-
-static int sr030pc30_g_ctrl(struct v4l2_subdev *sd,
-			    struct v4l2_control *ctrl)
-{
-	struct sr030pc30_info *info = to_sr030pc30(sd);
+		ret = sr030pc30_enable_autowhitebalance(sd, ctrl->val);
 
-	v4l2_dbg(1, debug, sd, "%s: id: %d\n", __func__, ctrl->id);
+		if (!ret && !ctrl->val) {
+			ret = cam_i2c_write(sd, MWB_BGAIN_REG, info->blue->val);
+			if (!ret)
+				ret = cam_i2c_write(sd, MWB_RGAIN_REG,
+						    info->red->val);
+		}
+		return ret;
 
-	switch (ctrl->id) {
-	case V4L2_CID_AUTO_WHITE_BALANCE:
-		ctrl->value = info->auto_wb;
-		break;
-	case V4L2_CID_BLUE_BALANCE:
-		ctrl->value = info->blue_balance;
-		break;
-	case V4L2_CID_RED_BALANCE:
-		ctrl->value = info->red_balance;
-		break;
 	case V4L2_CID_EXPOSURE_AUTO:
-		ctrl->value = info->auto_exp;
-		break;
-	case V4L2_CID_EXPOSURE:
-		ctrl->value = info->exposure;
-		break;
+		if (!ctrl->has_new)
+			ctrl->val = V4L2_EXPOSURE_MANUAL;
+
+		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
+			return sr030pc30_set_exposure(sd, info->exposure->val);
+		else
+			return sr030pc30_enable_autoexposure(sd, 1);
+
+	case V4L2_CID_HFLIP:
+		return sr030pc30_set_flip(sd, ctrl->val,
+					  info->vflip->val);
+	case V4L2_CID_VFLIP:
+		return sr030pc30_set_flip(sd, info->hflip->val,
+					  ctrl->val);
 	default:
 		return -EINVAL;
 	}
-	return 0;
 }
 
 static int sr030pc30_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
@@ -700,7 +605,7 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
 	v4l2_dbg(1, debug, sd, "%s: expmin= %lx, expmax= %lx", __func__,
 		 expmin, expmax);
 
-	/* Setting up manual exposure time range */
+	/* Setting up manual exposure time range. */
 	ret = cam_i2c_write(sd, EXP_MMINH_REG, expmin >> 8 & 0xFF);
 	if (!ret)
 		ret = cam_i2c_write(sd, EXP_MMINL_REG, expmin & 0xFF);
@@ -710,8 +615,11 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
 		ret = cam_i2c_write(sd, EXP_MMAXM_REG, expmax >> 8 & 0xFF);
 	if (!ret)
 		ret = cam_i2c_write(sd, EXP_MMAXL_REG, expmax & 0xFF);
+	if (ret)
+		return ret;
 
-	return ret;
+	/* Sync the handler and the registers state. */
+	return v4l2_ctrl_handler_setup(&info->hdl);
 }
 
 static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
@@ -748,11 +656,28 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
+static int sr030pc30_log_status(struct v4l2_subdev *sd)
+{
+	struct sr030pc30_info *info = to_sr030pc30(sd);
+
+	v4l2_ctrl_handler_log_status(&info->hdl, sd->name);
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops sr030pc30_ctrl_ops = {
+	.s_ctrl = sr030pc30_s_ctrl,
+};
+
 static const struct v4l2_subdev_core_ops sr030pc30_core_ops = {
 	.s_power	= sr030pc30_s_power,
-	.queryctrl	= sr030pc30_queryctrl,
-	.s_ctrl		= sr030pc30_s_ctrl,
-	.g_ctrl		= sr030pc30_g_ctrl,
+	.g_ctrl		= v4l2_subdev_g_ctrl,
+	.s_ctrl		= v4l2_subdev_s_ctrl,
+	.queryctrl	= v4l2_subdev_queryctrl,
+	.querymenu	= v4l2_subdev_querymenu,
+	.g_ext_ctrls	= v4l2_subdev_g_ext_ctrls,
+	.try_ext_ctrls	= v4l2_subdev_try_ext_ctrls,
+	.s_ext_ctrls	= v4l2_subdev_s_ext_ctrls,
+	.log_status	= sr030pc30_log_status,
 };
 
 static const struct v4l2_subdev_video_ops sr030pc30_video_ops = {
@@ -797,7 +722,6 @@ static int sr030pc30_detect(struct i2c_client *client)
 	return ret == SR030PC30_ID ? 0 : -ENODEV;
 }
 
-
 static int sr030pc30_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
@@ -808,7 +732,7 @@ static int sr030pc30_probe(struct i2c_client *client,
 	int ret;
 
 	if (!pdata) {
-		dev_err(&client->dev, "No platform data!");
+		dev_err(&client->dev, "No platform data!\n");
 		return -EIO;
 	}
 
@@ -820,17 +744,45 @@ static int sr030pc30_probe(struct i2c_client *client,
 	if (!info)
 		return -ENOMEM;
 
+	info->i2c_reg_page = -1;
+
 	sd = &info->sd;
-	strcpy(sd->name, MODULE_NAME);
+	strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
 	info->pdata = client->dev.platform_data;
 
 	v4l2_i2c_subdev_init(sd, client, &sr030pc30_ops);
+	v4l2_ctrl_handler_init(&info->hdl, 6);
+
+	info->autowb = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
+				V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+	info->red = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
+				V4L2_CID_RED_BALANCE, 0, 127, 1, 64);
+	info->blue = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
+				V4L2_CID_BLUE_BALANCE, 0, 127, 1, 64);
+
+	info->hflip = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
+				V4L2_CID_HFLIP, 0, 1, 1, 0);
+	info->vflip = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
+				V4L2_CID_VFLIP, 0, 1, 1, 0);
+
+	info->exposure = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
+				V4L2_CID_EXPOSURE, EXPOS_MIN_MS,
+				EXPOS_MAX_MS, 1, 30);
+
+	info->autoexposure = v4l2_ctrl_new_std_menu(&info->hdl,
+				&sr030pc30_ctrl_ops, V4L2_CID_EXPOSURE_AUTO,
+				1, 0, V4L2_EXPOSURE_AUTO);
+
+	sd->ctrl_handler = &info->hdl;
+
+	if (info->hdl.error) {
+		v4l2_ctrl_handler_free(&info->hdl);
+		kfree(info);
+		return info->hdl.error;
+	}
 
-	info->i2c_reg_page	= -1;
-	info->hflip		= 1;
-	info->auto_exp		= 1;
-	info->exposure		= 30;
-
+	v4l2_ctrl_cluster(2, &info->autoexposure);
+	v4l2_ctrl_cluster(3, &info->autowb);
 	return 0;
 }
 
@@ -840,6 +792,7 @@ static int sr030pc30_remove(struct i2c_client *client)
 	struct sr030pc30_info *info = to_sr030pc30(sd);
 
 	v4l2_device_unregister_subdev(sd);
+	v4l2_ctrl_handler_free(&info->hdl);
 	kfree(info);
 	return 0;
 }
-- 
1.7.0.4

Re: [PATCH 2/3] sr030pc30: Use the control framework

[Hans Verkuil]

Hi Sylwester!

I have some review comments below...

On Thursday, January 20, 2011 02:44:01 Sylwester Nawrocki wrote:
> Implement controls using the control framework.
> Add horizontal/vertical flip controls, minor cleanup.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/sr030pc30.c |  311 +++++++++++++++++----------------------
>  1 files changed, 132 insertions(+), 179 deletions(-)
> 
> diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
> index e1eced1..1a195f0 100644
> --- a/drivers/media/video/sr030pc30.c
> +++ b/drivers/media/video/sr030pc30.c
> @@ -19,6 +19,8 @@
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-subdev.h>
>  #include <media/v4l2-mediabus.h>
> @@ -141,17 +143,24 @@ module_param(debug, int, 0644);
>  
>  struct sr030pc30_info {
>  	struct v4l2_subdev sd;
> +	struct v4l2_ctrl_handler hdl;
> +	struct {
> +		/* exposure/auto-exposure cluster */
> +		struct v4l2_ctrl *autoexposure;
> +		struct v4l2_ctrl *exposure;
> +	};
> +	struct {
> +		/* blue/red/autowhitebalance cluster */
> +		struct v4l2_ctrl *autowb;
> +		struct v4l2_ctrl *blue;
> +		struct v4l2_ctrl *red;
> +	};
> +	struct v4l2_ctrl *hflip;
> +	struct v4l2_ctrl *vflip;
>  	const struct sr030pc30_platform_data *pdata;
>  	const struct sr030pc30_format *curr_fmt;
>  	const struct sr030pc30_frmsize *curr_win;
> -	unsigned int auto_wb:1;
> -	unsigned int auto_exp:1;
> -	unsigned int hflip:1;
> -	unsigned int vflip:1;
>  	unsigned int sleep:1;
> -	unsigned int exposure;
> -	u8 blue_balance;
> -	u8 red_balance;
>  	u8 i2c_reg_page;
>  };
>  
> @@ -172,52 +181,6 @@ struct i2c_regval {
>  	u16 val;
>  };
>  
> -static const struct v4l2_queryctrl sr030pc30_ctrl[] = {
> -	{
> -		.id		= V4L2_CID_AUTO_WHITE_BALANCE,
> -		.type		= V4L2_CTRL_TYPE_BOOLEAN,
> -		.name		= "Auto White Balance",
> -		.minimum	= 0,
> -		.maximum	= 1,
> -		.step		= 1,
> -		.default_value	= 1,
> -	}, {
> -		.id		= V4L2_CID_RED_BALANCE,
> -		.type		= V4L2_CTRL_TYPE_INTEGER,
> -		.name		= "Red Balance",
> -		.minimum	= 0,
> -		.maximum	= 127,
> -		.step		= 1,
> -		.default_value	= 64,
> -		.flags		= 0,
> -	}, {
> -		.id		= V4L2_CID_BLUE_BALANCE,
> -		.type		= V4L2_CTRL_TYPE_INTEGER,
> -		.name		= "Blue Balance",
> -		.minimum	= 0,
> -		.maximum	= 127,
> -		.step		= 1,
> -		.default_value	= 64,
> -	}, {
> -		.id		= V4L2_CID_EXPOSURE_AUTO,
> -		.type		= V4L2_CTRL_TYPE_INTEGER,
> -		.name		= "Auto Exposure",
> -		.minimum	= 0,
> -		.maximum	= 1,
> -		.step		= 1,
> -		.default_value	= 1,
> -	}, {
> -		.id		= V4L2_CID_EXPOSURE,
> -		.type		= V4L2_CTRL_TYPE_INTEGER,
> -		.name		= "Exposure",
> -		.minimum	= EXPOS_MIN_MS,
> -		.maximum	= EXPOS_MAX_MS,
> -		.step		= 1,
> -		.default_value	= 1,
> -	}, {
> -	}
> -};
> -
>  /* supported resolutions */
>  static const struct sr030pc30_frmsize sr030pc30_sizes[] = {
>  	{
> @@ -323,6 +286,11 @@ static inline struct sr030pc30_info *to_sr030pc30(struct v4l2_subdev *sd)
>  	return container_of(sd, struct sr030pc30_info, sd);
>  }
>  
> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of(ctrl->handler, struct sr030pc30_info, hdl)->sd;
> +}
> +
>  static inline int set_i2c_page(struct sr030pc30_info *info,
>  			       struct i2c_client *client, unsigned int reg)
>  {
> @@ -395,59 +363,56 @@ static int sr030pc30_pwr_ctrl(struct v4l2_subdev *sd,
>  
>  static inline int sr030pc30_enable_autoexposure(struct v4l2_subdev *sd, int on)
>  {
> -	struct sr030pc30_info *info = to_sr030pc30(sd);
>  	/* auto anti-flicker is also enabled here */
> -	int ret = cam_i2c_write(sd, AE_CTL1_REG, on ? 0xDC : 0x0C);
> -	if (!ret)
> -		info->auto_exp = on;
> -	return ret;
> +	return cam_i2c_write(sd, AE_CTL1_REG, on ? 0xDC : 0x0C);
>  }
>  
>  static int sr030pc30_set_exposure(struct v4l2_subdev *sd, int value)
>  {
>  	struct sr030pc30_info *info = to_sr030pc30(sd);
> -
>  	unsigned long expos = value * info->pdata->clk_rate / (8 * 1000);
> +	int ret;
>  
> -	int ret = cam_i2c_write(sd, EXP_TIMEH_REG, expos >> 16 & 0xFF);
> +	ret = cam_i2c_write(sd, EXP_TIMEH_REG, expos >> 16 & 0xFF);
>  	if (!ret)
>  		ret = cam_i2c_write(sd, EXP_TIMEM_REG, expos >> 8 & 0xFF);
>  	if (!ret)
>  		ret = cam_i2c_write(sd, EXP_TIMEL_REG, expos & 0xFF);
> -	if (!ret) { /* Turn off AE */
> -		info->exposure = value;
> +	if (!ret) /* Turn off AE */
>  		ret = sr030pc30_enable_autoexposure(sd, 0);
> -	}
> +
>  	return ret;
>  }
>  
>  /* Automatic white balance control */
>  static int sr030pc30_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
>  {
> -	struct sr030pc30_info *info = to_sr030pc30(sd);
> +	int ret;
>  
> -	int ret = cam_i2c_write(sd, AWB_CTL2_REG, on ? 0x2E : 0x2F);
> +	ret = cam_i2c_write(sd, AWB_CTL2_REG, on ? 0x2E : 0x2F);
>  	if (!ret)
>  		ret = cam_i2c_write(sd, AWB_CTL1_REG, on ? 0xFB : 0x7B);
> -	if (!ret)
> -		info->auto_wb = on;
>  
>  	return ret;
>  }
>  
> -static int sr030pc30_set_flip(struct v4l2_subdev *sd)
> +/**
> + * sr030pc30_set_flip - set image flipping
> + * @sd: a pointer to the subdev to apply the seetings to
> + * @hflip: 1 to enable or 0 to disable horizontal flip
> + * @vflip: as above but for vertical flip
> + */
> +static int sr030pc30_set_flip(struct v4l2_subdev *sd, u32 hflip, u32 vflip)
>  {
> -	struct sr030pc30_info *info = to_sr030pc30(sd);
> -
>  	s32 reg = cam_i2c_read(sd, VDO_CTL2_REG);
> +
>  	if (reg < 0)
>  		return reg;
>  
>  	reg &= 0x7C;
> -	if (info->hflip)
> -		reg |= 0x01;
> -	if (info->vflip)
> -		reg |= 0x02;
> +	reg |= ((hflip & 0x1) << 0);
> +	reg |= ((vflip & 0x1) << 1);
> +
>  	return cam_i2c_write(sd, VDO_CTL2_REG, reg | 0x80);
>  }

These functions are now very small, so my suggestion is to move the code directly
into the s_ctrl function.

>  
> @@ -468,8 +433,8 @@ static int sr030pc30_set_params(struct v4l2_subdev *sd)
>  		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
>  				info->curr_fmt->ispctl1_reg);
>  	if (!ret)
> -		ret = sr030pc30_set_flip(sd);
> -
> +		ret = sr030pc30_set_flip(sd, info->hflip->val,
> +					 info->vflip->val);

Why is flip being set here? It seems out of place to me.

>  	return ret;
>  }
>  
> @@ -497,108 +462,48 @@ static int sr030pc30_try_frame_size(struct v4l2_mbus_framefmt *mf)
>  	return -EINVAL;
>  }
>  
> -static int sr030pc30_queryctrl(struct v4l2_subdev *sd,
> -			       struct v4l2_queryctrl *qc)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(sr030pc30_ctrl); i++)
> -		if (qc->id == sr030pc30_ctrl[i].id) {
> -			*qc = sr030pc30_ctrl[i];
> -			v4l2_dbg(1, debug, sd, "%s id: %d\n",
> -				 __func__, qc->id);
> -			return 0;
> -		}
> -
> -	return -EINVAL;
> -}
> -
> -static inline int sr030pc30_set_bluebalance(struct v4l2_subdev *sd, int value)
> -{
> -	int ret = cam_i2c_write(sd, MWB_BGAIN_REG, value);
> -	if (!ret)
> -		to_sr030pc30(sd)->blue_balance = value;
> -	return ret;
> -}
> -
> -static inline int sr030pc30_set_redbalance(struct v4l2_subdev *sd, int value)
> -{
> -	int ret = cam_i2c_write(sd, MWB_RGAIN_REG, value);
> -	if (!ret)
> -		to_sr030pc30(sd)->red_balance = value;
> -	return ret;
> -}
> -
> -static int sr030pc30_s_ctrl(struct v4l2_subdev *sd,
> -			    struct v4l2_control *ctrl)
> +static int sr030pc30_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
> -	int i, ret = 0;
> -
> -	for (i = 0; i < ARRAY_SIZE(sr030pc30_ctrl); i++)
> -		if (ctrl->id == sr030pc30_ctrl[i].id)
> -			break;
> -
> -	if (i == ARRAY_SIZE(sr030pc30_ctrl))
> -		return -EINVAL;
> -
> -	if (ctrl->value < sr030pc30_ctrl[i].minimum ||
> -		ctrl->value > sr030pc30_ctrl[i].maximum)
> -			return -ERANGE;
> +	struct v4l2_subdev *sd = to_sd(ctrl);
> +	struct sr030pc30_info *info = to_sr030pc30(sd);
> +	int ret = 0;
>  
> -	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
> -			 __func__, ctrl->id, ctrl->value);
> +	v4l2_dbg(1, debug, sd, "%s: ctrl id: %d, value: %d\n",
> +			 __func__, ctrl->id, ctrl->val);
>  
>  	switch (ctrl->id) {
>  	case V4L2_CID_AUTO_WHITE_BALANCE:
> -		sr030pc30_enable_autowhitebalance(sd, ctrl->value);
> -		break;
> -	case V4L2_CID_BLUE_BALANCE:
> -		ret = sr030pc30_set_bluebalance(sd, ctrl->value);
> -		break;
> -	case V4L2_CID_RED_BALANCE:
> -		ret = sr030pc30_set_redbalance(sd, ctrl->value);
> -		break;
> -	case V4L2_CID_EXPOSURE_AUTO:
> -		sr030pc30_enable_autoexposure(sd,
> -			ctrl->value == V4L2_EXPOSURE_AUTO);
> -		break;
> -	case V4L2_CID_EXPOSURE:
> -		ret = sr030pc30_set_exposure(sd, ctrl->value);
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +		if (!ctrl->has_new)
> +			ctrl->val = 0;
>  
> -	return ret;
> -}
> -
> -static int sr030pc30_g_ctrl(struct v4l2_subdev *sd,
> -			    struct v4l2_control *ctrl)
> -{
> -	struct sr030pc30_info *info = to_sr030pc30(sd);
> +		ret = sr030pc30_enable_autowhitebalance(sd, ctrl->val);
>  
> -	v4l2_dbg(1, debug, sd, "%s: id: %d\n", __func__, ctrl->id);
> +		if (!ret && !ctrl->val) {
> +			ret = cam_i2c_write(sd, MWB_BGAIN_REG, info->blue->val);
> +			if (!ret)
> +				ret = cam_i2c_write(sd, MWB_RGAIN_REG,
> +						    info->red->val);
> +		}
> +		return ret;
>  
> -	switch (ctrl->id) {
> -	case V4L2_CID_AUTO_WHITE_BALANCE:
> -		ctrl->value = info->auto_wb;
> -		break;
> -	case V4L2_CID_BLUE_BALANCE:
> -		ctrl->value = info->blue_balance;
> -		break;
> -	case V4L2_CID_RED_BALANCE:
> -		ctrl->value = info->red_balance;
> -		break;
>  	case V4L2_CID_EXPOSURE_AUTO:
> -		ctrl->value = info->auto_exp;
> -		break;
> -	case V4L2_CID_EXPOSURE:
> -		ctrl->value = info->exposure;
> -		break;
> +		if (!ctrl->has_new)
> +			ctrl->val = V4L2_EXPOSURE_MANUAL;
> +
> +		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
> +			return sr030pc30_set_exposure(sd, info->exposure->val);
> +		else
> +			return sr030pc30_enable_autoexposure(sd, 1);

Setting up auto<foo> and <foo> controls like this is a bit tricky. I've
converted several drivers recently and had to do quite a few of these. Based
on that experience I will be posting a RFC patch this weekend adding special
support for this to the control framework that simplifies life a bit.

So I'd appreciate it if you could test those changes with this driver and if
all is OK, then I'll make a pull request for that and you can base your
changes on top of that.

> +
> +	case V4L2_CID_HFLIP:
> +		return sr030pc30_set_flip(sd, ctrl->val,
> +					  info->vflip->val);
> +	case V4L2_CID_VFLIP:
> +		return sr030pc30_set_flip(sd, info->hflip->val,
> +					  ctrl->val);

Since hflip and vflip are set in one register, you might want to make them a
cluster.

>  	default:
>  		return -EINVAL;
>  	}
> -	return 0;
>  }
>  
>  static int sr030pc30_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
> @@ -700,7 +605,7 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
>  	v4l2_dbg(1, debug, sd, "%s: expmin= %lx, expmax= %lx", __func__,
>  		 expmin, expmax);
>  
> -	/* Setting up manual exposure time range */
> +	/* Setting up manual exposure time range. */
>  	ret = cam_i2c_write(sd, EXP_MMINH_REG, expmin >> 8 & 0xFF);
>  	if (!ret)
>  		ret = cam_i2c_write(sd, EXP_MMINL_REG, expmin & 0xFF);
> @@ -710,8 +615,11 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
>  		ret = cam_i2c_write(sd, EXP_MMAXM_REG, expmax >> 8 & 0xFF);
>  	if (!ret)
>  		ret = cam_i2c_write(sd, EXP_MMAXL_REG, expmax & 0xFF);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	/* Sync the handler and the registers state. */
> +	return v4l2_ctrl_handler_setup(&info->hdl);
>  }
>  
>  static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
> @@ -748,11 +656,28 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
>  	return ret;
>  }
>  
> +static int sr030pc30_log_status(struct v4l2_subdev *sd)
> +{
> +	struct sr030pc30_info *info = to_sr030pc30(sd);
> +
> +	v4l2_ctrl_handler_log_status(&info->hdl, sd->name);
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops sr030pc30_ctrl_ops = {
> +	.s_ctrl = sr030pc30_s_ctrl,
> +};
> +
>  static const struct v4l2_subdev_core_ops sr030pc30_core_ops = {
>  	.s_power	= sr030pc30_s_power,
> -	.queryctrl	= sr030pc30_queryctrl,
> -	.s_ctrl		= sr030pc30_s_ctrl,
> -	.g_ctrl		= sr030pc30_g_ctrl,
> +	.g_ctrl		= v4l2_subdev_g_ctrl,
> +	.s_ctrl		= v4l2_subdev_s_ctrl,
> +	.queryctrl	= v4l2_subdev_queryctrl,
> +	.querymenu	= v4l2_subdev_querymenu,
> +	.g_ext_ctrls	= v4l2_subdev_g_ext_ctrls,
> +	.try_ext_ctrls	= v4l2_subdev_try_ext_ctrls,
> +	.s_ext_ctrls	= v4l2_subdev_s_ext_ctrls,
> +	.log_status	= sr030pc30_log_status,
>  };
>  
>  static const struct v4l2_subdev_video_ops sr030pc30_video_ops = {
> @@ -797,7 +722,6 @@ static int sr030pc30_detect(struct i2c_client *client)
>  	return ret == SR030PC30_ID ? 0 : -ENODEV;
>  }
>  
> -
>  static int sr030pc30_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
> @@ -808,7 +732,7 @@ static int sr030pc30_probe(struct i2c_client *client,
>  	int ret;
>  
>  	if (!pdata) {
> -		dev_err(&client->dev, "No platform data!");
> +		dev_err(&client->dev, "No platform data!\n");
>  		return -EIO;
>  	}
>  
> @@ -820,17 +744,45 @@ static int sr030pc30_probe(struct i2c_client *client,
>  	if (!info)
>  		return -ENOMEM;
>  
> +	info->i2c_reg_page = -1;
> +
>  	sd = &info->sd;
> -	strcpy(sd->name, MODULE_NAME);
> +	strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
>  	info->pdata = client->dev.platform_data;
>  
>  	v4l2_i2c_subdev_init(sd, client, &sr030pc30_ops);
> +	v4l2_ctrl_handler_init(&info->hdl, 6);
> +
> +	info->autowb = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
> +				V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
> +	info->red = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
> +				V4L2_CID_RED_BALANCE, 0, 127, 1, 64);
> +	info->blue = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
> +				V4L2_CID_BLUE_BALANCE, 0, 127, 1, 64);
> +
> +	info->hflip = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
> +				V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	info->vflip = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
> +				V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> +	info->exposure = v4l2_ctrl_new_std(&info->hdl, &sr030pc30_ctrl_ops,
> +				V4L2_CID_EXPOSURE, EXPOS_MIN_MS,
> +				EXPOS_MAX_MS, 1, 30);
> +
> +	info->autoexposure = v4l2_ctrl_new_std_menu(&info->hdl,
> +				&sr030pc30_ctrl_ops, V4L2_CID_EXPOSURE_AUTO,
> +				1, 0, V4L2_EXPOSURE_AUTO);
> +
> +	sd->ctrl_handler = &info->hdl;
> +
> +	if (info->hdl.error) {
> +		v4l2_ctrl_handler_free(&info->hdl);
> +		kfree(info);
> +		return info->hdl.error;
> +	}
>  
> -	info->i2c_reg_page	= -1;
> -	info->hflip		= 1;
> -	info->auto_exp		= 1;
> -	info->exposure		= 30;
> -
> +	v4l2_ctrl_cluster(2, &info->autoexposure);
> +	v4l2_ctrl_cluster(3, &info->autowb);
>  	return 0;
>  }
>  
> @@ -840,6 +792,7 @@ static int sr030pc30_remove(struct i2c_client *client)
>  	struct sr030pc30_info *info = to_sr030pc30(sd);
>  
>  	v4l2_device_unregister_subdev(sd);
> +	v4l2_ctrl_handler_free(&info->hdl);
>  	kfree(info);
>  	return 0;
>  }
> 

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [PATCH 2/3] sr030pc30: Use the control framework

[Sylwester Nawrocki]

Hello Hans,

On 2011-01-20 20:09, Hans Verkuil wrote:
> Hi Sylwester!
> 
> I have some review comments below...

Thank you for taking time to look at the patches.
> 
> On Thursday, January 20, 2011 02:44:01 Sylwester Nawrocki wrote:
>> Implement controls using the control framework.
>> Add horizontal/vertical flip controls, minor cleanup.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/media/video/sr030pc30.c |  311 +++++++++++++++++----------------------
>>  1 files changed, 132 insertions(+), 179 deletions(-)
>>
>> diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
>> index e1eced1..1a195f0 100644
>> --- a/drivers/media/video/sr030pc30.c
>> +++ b/drivers/media/video/sr030pc30.c
>> @@ -19,6 +19,8 @@
>>  #include <linux/i2c.h>
>>  #include <linux/delay.h>
>>  #include <linux/slab.h>
>> +#include <media/v4l2-chip-ident.h>
>> +#include <media/v4l2-ctrls.h>
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-subdev.h>
>>  #include <media/v4l2-mediabus.h>
>> @@ -141,17 +143,24 @@ module_param(debug, int, 0644);
>>  
>>  struct sr030pc30_info {
>>  	struct v4l2_subdev sd;
>> +	struct v4l2_ctrl_handler hdl;
>> +	struct {
>> +		/* exposure/auto-exposure cluster */
>> +		struct v4l2_ctrl *autoexposure;
>> +		struct v4l2_ctrl *exposure;
>> +	};
>> +	struct {
>> +		/* blue/red/autowhitebalance cluster */
>> +		struct v4l2_ctrl *autowb;
>> +		struct v4l2_ctrl *blue;
>> +		struct v4l2_ctrl *red;
>> +	};
>> +	struct v4l2_ctrl *hflip;
>> +	struct v4l2_ctrl *vflip;
>>  	const struct sr030pc30_platform_data *pdata;
>>  	const struct sr030pc30_format *curr_fmt;
>>  	const struct sr030pc30_frmsize *curr_win;
>> -	unsigned int auto_wb:1;
>> -	unsigned int auto_exp:1;
>> -	unsigned int hflip:1;
>> -	unsigned int vflip:1;
>>  	unsigned int sleep:1;
>> -	unsigned int exposure;
>> -	u8 blue_balance;
>> -	u8 red_balance;
>>  	u8 i2c_reg_page;
>>  };
>>  
>> @@ -172,52 +181,6 @@ struct i2c_regval {
>>  	u16 val;
>>  };
>>  
...
>> +
>>  static inline int set_i2c_page(struct sr030pc30_info *info,
>>  			       struct i2c_client *client, unsigned int reg)
>>  {
>> @@ -395,59 +363,56 @@ static int sr030pc30_pwr_ctrl(struct v4l2_subdev *sd,
>>  
>>  static inline int sr030pc30_enable_autoexposure(struct v4l2_subdev *sd, int on)
>>  {
>> -	struct sr030pc30_info *info = to_sr030pc30(sd);
>>  	/* auto anti-flicker is also enabled here */
>> -	int ret = cam_i2c_write(sd, AE_CTL1_REG, on ? 0xDC : 0x0C);
>> -	if (!ret)
>> -		info->auto_exp = on;
>> -	return ret;
>> +	return cam_i2c_write(sd, AE_CTL1_REG, on ? 0xDC : 0x0C);
>>  }
>>  
>>  static int sr030pc30_set_exposure(struct v4l2_subdev *sd, int value)
>>  {
>>  	struct sr030pc30_info *info = to_sr030pc30(sd);
>> -
>>  	unsigned long expos = value * info->pdata->clk_rate / (8 * 1000);
>> +	int ret;
>>  
>> -	int ret = cam_i2c_write(sd, EXP_TIMEH_REG, expos >> 16 & 0xFF);
>> +	ret = cam_i2c_write(sd, EXP_TIMEH_REG, expos >> 16 & 0xFF);
>>  	if (!ret)
>>  		ret = cam_i2c_write(sd, EXP_TIMEM_REG, expos >> 8 & 0xFF);
>>  	if (!ret)
>>  		ret = cam_i2c_write(sd, EXP_TIMEL_REG, expos & 0xFF);
>> -	if (!ret) { /* Turn off AE */
>> -		info->exposure = value;
>> +	if (!ret) /* Turn off AE */
>>  		ret = sr030pc30_enable_autoexposure(sd, 0);
>> -	}
>> +
>>  	return ret;
>>  }
>>  
>>  /* Automatic white balance control */
>>  static int sr030pc30_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
>>  {
>> -	struct sr030pc30_info *info = to_sr030pc30(sd);
>> +	int ret;
>>  
>> -	int ret = cam_i2c_write(sd, AWB_CTL2_REG, on ? 0x2E : 0x2F);
>> +	ret = cam_i2c_write(sd, AWB_CTL2_REG, on ? 0x2E : 0x2F);
>>  	if (!ret)
>>  		ret = cam_i2c_write(sd, AWB_CTL1_REG, on ? 0xFB : 0x7B);
>> -	if (!ret)
>> -		info->auto_wb = on;
>>  
>>  	return ret;
>>  }
>>  
>> -static int sr030pc30_set_flip(struct v4l2_subdev *sd)
>> +/**
>> + * sr030pc30_set_flip - set image flipping
>> + * @sd: a pointer to the subdev to apply the seetings to
>> + * @hflip: 1 to enable or 0 to disable horizontal flip
>> + * @vflip: as above but for vertical flip
>> + */
>> +static int sr030pc30_set_flip(struct v4l2_subdev *sd, u32 hflip, u32 vflip)
>>  {
>> -	struct sr030pc30_info *info = to_sr030pc30(sd);
>> -
>>  	s32 reg = cam_i2c_read(sd, VDO_CTL2_REG);
>> +
>>  	if (reg < 0)
>>  		return reg;
>>  
>>  	reg &= 0x7C;
>> -	if (info->hflip)
>> -		reg |= 0x01;
>> -	if (info->vflip)
>> -		reg |= 0x02;
>> +	reg |= ((hflip & 0x1) << 0);
>> +	reg |= ((vflip & 0x1) << 1);
>> +
>>  	return cam_i2c_write(sd, VDO_CTL2_REG, reg | 0x80);
>>  }
> 
> These functions are now very small, so my suggestion is to move the code directly
> into the s_ctrl function.

yeah, that seems reasonable
> 
>>  
>> @@ -468,8 +433,8 @@ static int sr030pc30_set_params(struct v4l2_subdev *sd)
>>  		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
>>  				info->curr_fmt->ispctl1_reg);
>>  	if (!ret)
>> -		ret = sr030pc30_set_flip(sd);
>> -
>> +		ret = sr030pc30_set_flip(sd, info->hflip->val,
>> +					 info->vflip->val);
> 
> Why is flip being set here? It seems out of place to me.

Indeed, there is no good reason for setting the flip register here.

> 
>>  	return ret;
>>  }
>>  
>> @@ -497,108 +462,48 @@ static int sr030pc30_try_frame_size(struct v4l2_mbus_framefmt *mf)
>>  	return -EINVAL;
>>  }
>>  
...
>>  	switch (ctrl->id) {
>>  	case V4L2_CID_AUTO_WHITE_BALANCE:
>> -		sr030pc30_enable_autowhitebalance(sd, ctrl->value);
>> -		break;
>> -	case V4L2_CID_BLUE_BALANCE:
>> -		ret = sr030pc30_set_bluebalance(sd, ctrl->value);
>> -		break;
>> -	case V4L2_CID_RED_BALANCE:
>> -		ret = sr030pc30_set_redbalance(sd, ctrl->value);
>> -		break;
>> -	case V4L2_CID_EXPOSURE_AUTO:
>> -		sr030pc30_enable_autoexposure(sd,
>> -			ctrl->value == V4L2_EXPOSURE_AUTO);
>> -		break;
>> -	case V4L2_CID_EXPOSURE:
>> -		ret = sr030pc30_set_exposure(sd, ctrl->value);
>> -		break;
>> -	default:
>> -		return -EINVAL;
>> -	}
>> +		if (!ctrl->has_new)
>> +			ctrl->val = 0;

Hmm, looks like I merged the changes renaming "has_new" to "is_new"
to the wrong patch. Need to fix that too..
When there is a full support for one of our S5PV210 SoC based reference
boards in the mainline kernel the testing should get easier..

>>  
>> -	return ret;
>> -}
>> -
>> -static int sr030pc30_g_ctrl(struct v4l2_subdev *sd,
>> -			    struct v4l2_control *ctrl)
>> -{
>> -	struct sr030pc30_info *info = to_sr030pc30(sd);
>> +		ret = sr030pc30_enable_autowhitebalance(sd, ctrl->val);
>>  
>> -	v4l2_dbg(1, debug, sd, "%s: id: %d\n", __func__, ctrl->id);
>> +		if (!ret && !ctrl->val) {
>> +			ret = cam_i2c_write(sd, MWB_BGAIN_REG, info->blue->val);
>> +			if (!ret)
>> +				ret = cam_i2c_write(sd, MWB_RGAIN_REG,
>> +						    info->red->val);
>> +		}
>> +		return ret;
>>  
>> -	switch (ctrl->id) {
>> -	case V4L2_CID_AUTO_WHITE_BALANCE:
>> -		ctrl->value = info->auto_wb;
>> -		break;
>> -	case V4L2_CID_BLUE_BALANCE:
>> -		ctrl->value = info->blue_balance;
>> -		break;
>> -	case V4L2_CID_RED_BALANCE:
>> -		ctrl->value = info->red_balance;
>> -		break;
>>  	case V4L2_CID_EXPOSURE_AUTO:
>> -		ctrl->value = info->auto_exp;
>> -		break;
>> -	case V4L2_CID_EXPOSURE:
>> -		ctrl->value = info->exposure;
>> -		break;
>> +		if (!ctrl->has_new)
>> +			ctrl->val = V4L2_EXPOSURE_MANUAL;
>> +
>> +		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
>> +			return sr030pc30_set_exposure(sd, info->exposure->val);
>> +		else
>> +			return sr030pc30_enable_autoexposure(sd, 1);
> 
> Setting up auto<foo> and <foo> controls like this is a bit tricky. I've
> converted several drivers recently and had to do quite a few of these. Based
> on that experience I will be posting a RFC patch this weekend adding special
> support for this to the control framework that simplifies life a bit.
> 
> So I'd appreciate it if you could test those changes with this driver and if
> all is OK, then I'll make a pull request for that and you can base your
> changes on top of that.

OK, I'll be happy to test your extension to the control framework,
hopefully I can do that next week, or at the latest the week after.
I've just noticed patches for the control framework on the mailing list.
I've reviewed them and didn't find, so far ;), any problems.

> 
>> +
>> +	case V4L2_CID_HFLIP:
>> +		return sr030pc30_set_flip(sd, ctrl->val,
>> +					  info->vflip->val);
>> +	case V4L2_CID_VFLIP:
>> +		return sr030pc30_set_flip(sd, info->hflip->val,
>> +					  ctrl->val);
> 
> Since hflip and vflip are set in one register, you might want to make them a
> cluster.

Yeah, that sounds like a good idea. I'll rework it that way.


Cheers,
Sylwester

Re: [PATCH 2/3] sr030pc30: Use the control framework

[Sylwester Nawrocki]

On 01/21/2011 04:09 AM, Hans Verkuil wrote:
> Hi Sylwester!
> 
> I have some review comments below...
> 
> On Thursday, January 20, 2011 02:44:01 Sylwester Nawrocki wrote:
>> Implement controls using the control framework.
>> Add horizontal/vertical flip controls, minor cleanup.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   drivers/media/video/sr030pc30.c |  311 +++++++++++++++++----------------------
>>   1 files changed, 132 insertions(+), 179 deletions(-)
>>
...
>> -
>> -static int sr030pc30_s_ctrl(struct v4l2_subdev *sd,
>> -			    struct v4l2_control *ctrl)
>> +static int sr030pc30_s_ctrl(struct v4l2_ctrl *ctrl)
>>   {
>> -	int i, ret = 0;
>> -
>> -	for (i = 0; i<  ARRAY_SIZE(sr030pc30_ctrl); i++)
>> -		if (ctrl->id == sr030pc30_ctrl[i].id)
>> -			break;
>> -
>> -	if (i == ARRAY_SIZE(sr030pc30_ctrl))
>> -		return -EINVAL;
>> -
>> -	if (ctrl->value<  sr030pc30_ctrl[i].minimum ||
>> -		ctrl->value>  sr030pc30_ctrl[i].maximum)
>> -			return -ERANGE;
>> +	struct v4l2_subdev *sd = to_sd(ctrl);
>> +	struct sr030pc30_info *info = to_sr030pc30(sd);
>> +	int ret = 0;
>>
>> -	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
>> -			 __func__, ctrl->id, ctrl->value);
>> +	v4l2_dbg(1, debug, sd, "%s: ctrl id: %d, value: %d\n",
>> +			 __func__, ctrl->id, ctrl->val);
>>
>>   	switch (ctrl->id) {
>>   	case V4L2_CID_AUTO_WHITE_BALANCE:
>> -		sr030pc30_enable_autowhitebalance(sd, ctrl->value);
>> -		break;
>> -	case V4L2_CID_BLUE_BALANCE:
>> -		ret = sr030pc30_set_bluebalance(sd, ctrl->value);
>> -		break;
>> -	case V4L2_CID_RED_BALANCE:
>> -		ret = sr030pc30_set_redbalance(sd, ctrl->value);
>> -		break;
>> -	case V4L2_CID_EXPOSURE_AUTO:
>> -		sr030pc30_enable_autoexposure(sd,
>> -			ctrl->value == V4L2_EXPOSURE_AUTO);
>> -		break;
>> -	case V4L2_CID_EXPOSURE:
>> -		ret = sr030pc30_set_exposure(sd, ctrl->value);
>> -		break;
>> -	default:
>> -		return -EINVAL;
>> -	}
>> +		if (!ctrl->has_new)
>> +			ctrl->val = 0;
>>
>> -	return ret;
>> -}
>> -
>> -static int sr030pc30_g_ctrl(struct v4l2_subdev *sd,
>> -			    struct v4l2_control *ctrl)
>> -{
>> -	struct sr030pc30_info *info = to_sr030pc30(sd);
>> +		ret = sr030pc30_enable_autowhitebalance(sd, ctrl->val);
>>
>> -	v4l2_dbg(1, debug, sd, "%s: id: %d\n", __func__, ctrl->id);
>> +		if (!ret&&  !ctrl->val) {
>> +			ret = cam_i2c_write(sd, MWB_BGAIN_REG, info->blue->val);
>> +			if (!ret)
>> +				ret = cam_i2c_write(sd, MWB_RGAIN_REG,
>> +						    info->red->val);
>> +		}
>> +		return ret;
>>
>> -	switch (ctrl->id) {
>> -	case V4L2_CID_AUTO_WHITE_BALANCE:
>> -		ctrl->value = info->auto_wb;
>> -		break;
>> -	case V4L2_CID_BLUE_BALANCE:
>> -		ctrl->value = info->blue_balance;
>> -		break;
>> -	case V4L2_CID_RED_BALANCE:
>> -		ctrl->value = info->red_balance;
>> -		break;
>>   	case V4L2_CID_EXPOSURE_AUTO:
>> -		ctrl->value = info->auto_exp;
>> -		break;
>> -	case V4L2_CID_EXPOSURE:
>> -		ctrl->value = info->exposure;
>> -		break;
>> +		if (!ctrl->has_new)
>> +			ctrl->val = V4L2_EXPOSURE_MANUAL;
>> +
>> +		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
>> +			return sr030pc30_set_exposure(sd, info->exposure->val);
>> +		else
>> +			return sr030pc30_enable_autoexposure(sd, 1);
> 
> Setting up auto<foo>  and<foo>  controls like this is a bit tricky. I've
> converted several drivers recently and had to do quite a few of these. Based
> on that experience I will be posting a RFC patch this weekend adding special
> support for this to the control framework that simplifies life a bit.
> 
> So I'd appreciate it if you could test those changes with this driver and if
> all is OK, then I'll make a pull request for that and you can base your
> changes on top of that.
> 

I have converted sr030pc30 already just need to do testing when I get my hands
on the hardware. With your foo/auto-foo for V4L2_CID_EXPOSURE_AUTO I did:

	case V4L2_CID_EXPOSURE_AUTO:
	        if (ctrl->val == V4L2_EXPOSURE_MANUAL)
                        ret = sr030pc30_set_exposure(sd, info->exposure->val);

                /* Set autoexposure and auto anti-flicker. */
                if (!ret)
                        return cam_i2c_write(sd, AE_CTL1_REG,
                                             ctrl->val == V4L2_EXPOSURE_MANUAL ?
                                             0xDC : 0x0C);
                return ret;
 
Is the change with foo/auto-foo controls support in v4l2 core only about
that the "is_new" flag don't have to be checked in s_ctrl for each control
and ctrl->val reassigned there?


Regards,
Sylwester

[PATCH 3/3] sr030pc30: Add regulator framework support

[Sylwester Nawrocki]

Use the regulator API instead of the set_power callback.
Handle RESET and STANDBY gpio in the driver when needed.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/sr030pc30.c |  193 ++++++++++++++++++++++++++++++---------
 include/media/sr030pc30.h       |   14 ++-
 2 files changed, 162 insertions(+), 45 deletions(-)

diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
index 1a195f0..940ac37 100644
--- a/drivers/media/video/sr030pc30.c
+++ b/drivers/media/video/sr030pc30.c
@@ -18,6 +18,8 @@
 
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <media/v4l2-chip-ident.h>
 #include <media/v4l2-ctrls.h>
@@ -141,6 +143,12 @@ module_param(debug, int, 0644);
 #define EXPOS_MIN_MS		1
 #define EXPOS_MAX_MS		125
 
+static const char * const sr030pc30_supply_name[] = {
+	"vdd_core", "vddio", "vdda"
+};
+
+#define SR030PC30_NUM_SUPPLIES ARRAY_SIZE(sr030pc30_supply_name)
+
 struct sr030pc30_info {
 	struct v4l2_subdev sd;
 	struct v4l2_ctrl_handler hdl;
@@ -160,7 +168,11 @@ struct sr030pc30_info {
 	const struct sr030pc30_platform_data *pdata;
 	const struct sr030pc30_format *curr_fmt;
 	const struct sr030pc30_frmsize *curr_win;
+	unsigned int power:1;
 	unsigned int sleep:1;
+	struct regulator_bulk_data supply[SR030PC30_NUM_SUPPLIES];
+	u32 gpio_nreset;
+	u32 gpio_nstby;
 	u8 i2c_reg_page;
 };
 
@@ -473,7 +485,7 @@ static int sr030pc30_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_AUTO_WHITE_BALANCE:
-		if (!ctrl->has_new)
+		if (!ctrl->is_new)
 			ctrl->val = 0;
 
 		ret = sr030pc30_enable_autowhitebalance(sd, ctrl->val);
@@ -487,7 +499,7 @@ static int sr030pc30_s_ctrl(struct v4l2_ctrl *ctrl)
 		return ret;
 
 	case V4L2_CID_EXPOSURE_AUTO:
-		if (!ctrl->has_new)
+		if (!ctrl->is_new)
 			ctrl->val = V4L2_EXPOSURE_MANUAL;
 
 		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
@@ -622,9 +634,66 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
 	return v4l2_ctrl_handler_setup(&info->hdl);
 }
 
+static int sr030pc30_power_enable(struct sr030pc30_info *info)
+{
+	int ret;
+
+	if (info->power)
+		return 0;
+
+	if (gpio_is_valid(info->gpio_nstby))
+		gpio_set_value(info->gpio_nstby, 0);
+
+	if (gpio_is_valid(info->gpio_nreset))
+		gpio_set_value(info->gpio_nreset, 0);
+
+	ret = regulator_bulk_enable(SR030PC30_NUM_SUPPLIES, info->supply);
+	if (ret)
+		return ret;
+
+	if (gpio_is_valid(info->gpio_nreset)) {
+		msleep(50);
+		gpio_set_value(info->gpio_nreset, 1);
+	}
+	if (gpio_is_valid(info->gpio_nstby)) {
+		udelay(1000);
+		gpio_set_value(info->gpio_nstby, 1);
+	}
+	if (gpio_is_valid(info->gpio_nreset)) {
+		udelay(1000);
+		gpio_set_value(info->gpio_nreset, 0);
+		msleep(100);
+		gpio_set_value(info->gpio_nreset, 1);
+		msleep(20);
+	}
+
+	info->power = 1;
+	return 0;
+}
+
+static int sr030pc30_power_disable(struct sr030pc30_info *info)
+{
+	int ret;
+
+	if (!info->power)
+		return 0;
+
+	ret = regulator_bulk_disable(SR030PC30_NUM_SUPPLIES, info->supply);
+	if (ret)
+		return ret;
+
+	if (gpio_is_valid(info->gpio_nstby))
+		gpio_set_value(info->gpio_nstby, 0);
+
+	if (gpio_is_valid(info->gpio_nreset))
+		gpio_set_value(info->gpio_nreset, 0);
+
+	info->power = 0;
+	return 0;
+}
+
 static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct sr030pc30_info *info = to_sr030pc30(sd);
 	const struct sr030pc30_platform_data *pdata = info->pdata;
 	int ret;
@@ -632,23 +701,15 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
 	if (WARN(pdata == NULL, "No platform data!\n"))
 		return -ENOMEM;
 
-	/*
-	 * Put sensor into power sleep mode before switching off
-	 * power and disabling MCLK.
-	 */
-	if (!on)
-		sr030pc30_pwr_ctrl(sd, false, true);
-
-	/* set_power controls sensor's power and clock */
-	if (pdata->set_power) {
-		ret = pdata->set_power(&client->dev, on);
+	/* Put sensor into power sleep mode before switching supply off. */
+	if (on) {
+		ret = sr030pc30_power_enable(info);
 		if (ret)
 			return ret;
-	}
-
-	if (on) {
 		ret = sr030pc30_base_config(sd);
 	} else {
+		sr030pc30_pwr_ctrl(sd, false, true);
+		ret = sr030pc30_power_disable(info);
 		info->curr_win = NULL;
 		info->curr_fmt = NULL;
 	}
@@ -696,30 +757,40 @@ static const struct v4l2_subdev_ops sr030pc30_ops = {
  * Detect sensor type. Return 0 if SR030PC30 was detected
  * or -ENODEV otherwise.
  */
-static int sr030pc30_detect(struct i2c_client *client)
+static int sr030pc30_detect(struct i2c_client *client, struct sr030pc30_info *info)
 {
-	const struct sr030pc30_platform_data *pdata
-		= client->dev.platform_data;
 	int ret;
 
-	/* Enable sensor's power and clock */
-	if (pdata->set_power) {
-		ret = pdata->set_power(&client->dev, 1);
-		if (ret)
-			return ret;
-	}
+	ret = sr030pc30_power_enable(info);
+	if (ret)
+		return ret;
 
 	ret = i2c_smbus_read_byte_data(client, DEVICE_ID_REG);
+	if (ret < 0)
+		dev_err(&client->dev, "%s: I2C read failed\n", __func__);
 
-	if (pdata->set_power)
-		pdata->set_power(&client->dev, 0);
+	sr030pc30_power_disable(info);
 
-	if (ret < 0) {
-		dev_err(&client->dev, "%s: I2C read failed\n", __func__);
+	return ret == SR030PC30_ID ? 0 : -ENODEV;
+}
+
+static int sr030pc30_configure_gpio(struct v4l2_subdev *sd,
+				    int nr, const char *name)
+{
+	int ret = 0;
+
+	if (!gpio_is_valid(nr))
 		return ret;
-	}
 
-	return ret == SR030PC30_ID ? 0 : -ENODEV;
+	ret = gpio_request(nr, name);
+	if (!ret)
+		ret = gpio_direction_output(nr, 0);
+	if (!ret)
+		ret = gpio_export(nr, 0);
+	if (ret)
+		v4l2_err(sd, "gpio configuration error: %d\n", ret);
+
+	return ret;
 }
 
 static int sr030pc30_probe(struct i2c_client *client,
@@ -729,17 +800,13 @@ static int sr030pc30_probe(struct i2c_client *client,
 	struct v4l2_subdev *sd;
 	const struct sr030pc30_platform_data *pdata
 		= client->dev.platform_data;
-	int ret;
+	int i, ret;
 
 	if (!pdata) {
 		dev_err(&client->dev, "No platform data!\n");
 		return -EIO;
 	}
 
-	ret = sr030pc30_detect(client);
-	if (ret)
-		return ret;
-
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
@@ -774,16 +841,49 @@ static int sr030pc30_probe(struct i2c_client *client,
 				1, 0, V4L2_EXPOSURE_AUTO);
 
 	sd->ctrl_handler = &info->hdl;
+	ret = info->hdl.error;
 
-	if (info->hdl.error) {
-		v4l2_ctrl_handler_free(&info->hdl);
-		kfree(info);
-		return info->hdl.error;
-	}
+	if (ret)
+		goto sp_err;
 
 	v4l2_ctrl_cluster(2, &info->autoexposure);
 	v4l2_ctrl_cluster(3, &info->autowb);
-	return 0;
+
+	ret = sr030pc30_configure_gpio(sd, pdata->gpio_nreset,
+				       "SR030PC30_NRST");
+	if (ret)
+		goto sp_err;
+	info->gpio_nreset = pdata->gpio_nreset;
+
+	ret = sr030pc30_configure_gpio(sd, pdata->gpio_nstby,
+				       "SR030PC30_NSTBY");
+	if (ret)
+		goto sp_gpio_err;
+	info->gpio_nstby = pdata->gpio_nstby;
+
+	for (i = 0; i < SR030PC30_NUM_SUPPLIES; i++)
+		info->supply[i].supply = sr030pc30_supply_name[i];
+
+	ret = regulator_bulk_get(&client->dev, SR030PC30_NUM_SUPPLIES,
+				 info->supply);
+	if (ret)
+		goto sp_reg_err;
+
+	ret = sr030pc30_detect(client, info);
+	if (!ret)
+		return 0;
+
+	regulator_bulk_free(SR030PC30_NUM_SUPPLIES, info->supply);
+sp_reg_err:
+	if (gpio_is_valid(info->gpio_nstby))
+		gpio_free(info->gpio_nstby);
+sp_gpio_err:
+	if (gpio_is_valid(info->gpio_nreset))
+		gpio_free(info->gpio_nreset);
+sp_err:
+	v4l2_ctrl_handler_free(&info->hdl);
+	kfree(info);
+	return ret;
 }
 
 static int sr030pc30_remove(struct i2c_client *client)
@@ -793,6 +893,15 @@ static int sr030pc30_remove(struct i2c_client *client)
 
 	v4l2_device_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&info->hdl);
+
+	regulator_bulk_free(SR030PC30_NUM_SUPPLIES, info->supply);
+
+	if (gpio_is_valid(info->gpio_nreset))
+		gpio_free(info->gpio_nreset);
+
+	if (gpio_is_valid(info->gpio_nstby))
+		gpio_free(info->gpio_nstby);
+
 	kfree(info);
 	return 0;
 }
diff --git a/include/media/sr030pc30.h b/include/media/sr030pc30.h
index 6f901a6..3689769 100644
--- a/include/media/sr030pc30.h
+++ b/include/media/sr030pc30.h
@@ -13,9 +13,17 @@
 #ifndef SR030PC30_H
 #define SR030PC30_H
 
+/**
+ * @clk_rate: the sensor's master clock frequency in Hz
+ * @gpio_nreset: GPIO driving nRESET pin
+ * @gpio_nstby: GPIO driving nSTBY pin
+ *
+ * When the gpio pins are not used gpio_nreset
+ * and gpio_nstby must be set to -EINVAL.
+ */
 struct sr030pc30_platform_data {
-	unsigned long clk_rate;	/* master clock frequency in Hz */
-	int (*set_power)(struct device *dev, int on);
+	unsigned long clk_rate;
+	int gpio_nreset;
+	int gpio_nstby;
 };
-
 #endif /* SR030PC30_H */
-- 
1.7.0.4