[PATCH v4 0/3] media: ov7670: Add entity init and power operation

[PATCH v4 0/3] media: ov7670: Add entity init and power operation

[Wenyou Yang]

This patch set is to add the media entity pads initialization,
the s_power operation and get_fmt callback support.

Changes in v4:
 - Fix the build error when not enabling Media Controller API option.
 - Fix the build error when not enabling V4L2 sub-device userspace API option.

Changes in v3:
 - Keep tried format info in the try_fmt member of
   v4l2_subdev__pad_config struct.
 - Add the internal_ops callback to set default format.

Changes in v2:
 - Add the patch to support the get_fmt ops.
 - Remove the redundant invoking ov7670_init_gpio().

Wenyou Yang (3):
  media: ov7670: Add entity pads initialization
  media: ov7670: Add the get_fmt callback
  media: ov7670: Add the s_power operation

 drivers/media/i2c/ov7670.c | 128 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 121 insertions(+), 7 deletions(-)

-- 
2.13.0

[PATCH v4 1/3] media: ov7670: Add entity pads initialization

[Wenyou Yang]

Add the media entity pads initialization.

Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
---

Changes in v4:
 - Fix the build error when not enabling Media Controller API option.

Changes in v3: None
Changes in v2: None

 drivers/media/i2c/ov7670.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index e88549f0e704..553945d4ca28 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -213,6 +213,9 @@ struct ov7670_devtype {
 struct ov7670_format_struct;  /* coming later */
 struct ov7670_info {
 	struct v4l2_subdev sd;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	struct media_pad pad;
+#endif
 	struct v4l2_ctrl_handler hdl;
 	struct {
 		/* gain cluster */
@@ -1688,14 +1691,27 @@ static int ov7670_probe(struct i2c_client *client,
 	v4l2_ctrl_auto_cluster(2, &info->auto_exposure,
 			       V4L2_EXPOSURE_MANUAL, false);
 	v4l2_ctrl_cluster(2, &info->saturation);
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	info->pad.flags = MEDIA_PAD_FL_SOURCE;
+	info->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&info->sd.entity, 1, &info->pad);
+	if (ret < 0)
+		goto hdl_free;
+#endif
+
 	v4l2_ctrl_handler_setup(&info->hdl);
 
 	ret = v4l2_async_register_subdev(&info->sd);
 	if (ret < 0)
-		goto hdl_free;
+		goto entity_cleanup;
 
 	return 0;
 
+entity_cleanup:
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	media_entity_cleanup(&info->sd.entity);
+#endif
 hdl_free:
 	v4l2_ctrl_handler_free(&info->hdl);
 clk_disable:
@@ -1712,6 +1728,9 @@ static int ov7670_remove(struct i2c_client *client)
 	v4l2_device_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&info->hdl);
 	clk_disable_unprepare(info->clk);
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	media_entity_cleanup(&info->sd.entity);
+#endif
 	return 0;
 }
 
-- 
2.13.0

[PATCH v4 2/3] media: ov7670: Add the get_fmt callback

[Wenyou Yang]

Add the get_fmt callback, also enable V4L2_SUBDEV_FL_HAS_DEVNODE flag
to make this subdev has device node.

Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
---

Changes in v4:
 - Fix the build error when not enabling V4L2 sub-device userspace API option.

Changes in v3:
 - Keep tried format info in the try_fmt member of
   v4l2_subdev__pad_config struct.
 - Add the internal_ops callback to set default format.

Changes in v2: None

 drivers/media/i2c/ov7670.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 553945d4ca28..456f48057605 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -232,6 +232,7 @@ struct ov7670_info {
 		struct v4l2_ctrl *saturation;
 		struct v4l2_ctrl *hue;
 	};
+	struct v4l2_mbus_framefmt format;
 	struct ov7670_format_struct *fmt;  /* Current format */
 	struct clk *clk;
 	struct gpio_desc *resetb_gpio;
@@ -975,6 +976,9 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
 	fmt->width = wsize->width;
 	fmt->height = wsize->height;
 	fmt->colorspace = ov7670_formats[index].colorspace;
+
+	info->format = *fmt;
+
 	return 0;
 }
 
@@ -998,8 +1002,15 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
 		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
 		if (ret)
 			return ret;
-		cfg->try_fmt = format->format;
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+		struct v4l2_mbus_framefmt *mbus_fmt;
+
+		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
+		*mbus_fmt = format->format;
 		return 0;
+#else
+		return -ENOTTY;
+#endif
 	}
 
 	ret = ov7670_try_fmt_internal(sd, &format->format, &ovfmt, &wsize);
@@ -1041,6 +1052,29 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov7670_get_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *format)
+{
+	struct ov7670_info *info = to_state(sd);
+
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+		struct v4l2_mbus_framefmt *mbus_fmt;
+
+		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
+		format->format = *mbus_fmt;
+		return 0;
+#else
+		return -ENOTTY;
+#endif
+	} else {
+		format->format = info->format;
+	}
+
+	return 0;
+}
+
 /*
  * Implement G/S_PARM.  There is a "high quality" mode we could try
  * to do someday; for now, we just do the frame rate tweak.
@@ -1508,6 +1542,30 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
 }
 #endif
 
+static void ov7670_get_default_format(struct v4l2_subdev *sd,
+				      struct v4l2_mbus_framefmt *format)
+{
+	struct ov7670_info *info = to_state(sd);
+
+	format->width = info->devtype->win_sizes[0].width;
+	format->height = info->devtype->win_sizes[0].height;
+	format->colorspace = info->fmt->colorspace;
+	format->code = info->fmt->mbus_code;
+	format->field = V4L2_FIELD_NONE;
+}
+
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+static int ov7670_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *format =
+				v4l2_subdev_get_try_format(sd, fh->pad, 0);
+
+	ov7670_get_default_format(sd, format);
+
+	return 0;
+}
+#endif
+
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_subdev_core_ops ov7670_core_ops = {
@@ -1528,6 +1586,7 @@ static const struct v4l2_subdev_pad_ops ov7670_pad_ops = {
 	.enum_frame_interval = ov7670_enum_frame_interval,
 	.enum_frame_size = ov7670_enum_frame_size,
 	.enum_mbus_code = ov7670_enum_mbus_code,
+	.get_fmt = ov7670_get_fmt,
 	.set_fmt = ov7670_set_fmt,
 };
 
@@ -1537,6 +1596,12 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 	.pad = &ov7670_pad_ops,
 };
 
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+static const struct v4l2_subdev_internal_ops ov7670_subdev_internal_ops = {
+	.open = ov7670_open,
+};
+#endif
+
 /* ----------------------------------------------------------------------- */
 
 static const struct ov7670_devtype ov7670_devdata[] = {
@@ -1589,6 +1654,11 @@ static int ov7670_probe(struct i2c_client *client,
 	sd = &info->sd;
 	v4l2_i2c_subdev_init(sd, client, &ov7670_ops);
 
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+	sd->internal_ops = &ov7670_subdev_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+#endif
+
 	info->clock_speed = 30; /* default: a guess */
 	if (client->dev.platform_data) {
 		struct ov7670_config *config = client->dev.platform_data;
@@ -1645,6 +1715,9 @@ static int ov7670_probe(struct i2c_client *client,
 
 	info->devtype = &ov7670_devdata[id->driver_data];
 	info->fmt = &ov7670_formats[0];
+
+	ov7670_get_default_format(sd, &info->format);
+
 	info->clkrc = 0;
 
 	/* Set default frame rate to 30 fps */
-- 
2.13.0

[PATCH v4 3/3] media: ov7670: Add the s_power operation

[Wenyou Yang]

Add the s_power operation which is responsible for manipulating the
power dowm mode through the PWDN pin and the reset operation through
the RESET pin.

Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
 - Add the patch to support the get_fmt ops.
 - Remove the redundant invoking ov7670_init_gpio().

 drivers/media/i2c/ov7670.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 456f48057605..304abc769a67 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1542,6 +1542,22 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
 }
 #endif
 
+static int ov7670_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct ov7670_info *info = to_state(sd);
+
+	if (info->pwdn_gpio)
+		gpiod_direction_output(info->pwdn_gpio, !on);
+	if (on && info->resetb_gpio) {
+		gpiod_set_value(info->resetb_gpio, 1);
+		usleep_range(500, 1000);
+		gpiod_set_value(info->resetb_gpio, 0);
+		usleep_range(3000, 5000);
+	}
+
+	return 0;
+}
+
 static void ov7670_get_default_format(struct v4l2_subdev *sd,
 				      struct v4l2_mbus_framefmt *format)
 {
@@ -1575,6 +1591,7 @@ static const struct v4l2_subdev_core_ops ov7670_core_ops = {
 	.g_register = ov7670_g_register,
 	.s_register = ov7670_s_register,
 #endif
+	.s_power = ov7670_s_power,
 };
 
 static const struct v4l2_subdev_video_ops ov7670_video_ops = {
@@ -1692,23 +1709,25 @@ static int ov7670_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	ret = ov7670_init_gpio(client, info);
-	if (ret)
-		goto clk_disable;
-
 	info->clock_speed = clk_get_rate(info->clk) / 1000000;
 	if (info->clock_speed < 10 || info->clock_speed > 48) {
 		ret = -EINVAL;
 		goto clk_disable;
 	}
 
+	ret = ov7670_init_gpio(client, info);
+	if (ret)
+		goto clk_disable;
+
+	ov7670_s_power(sd, 1);
+
 	/* Make sure it's an ov7670 */
 	ret = ov7670_detect(sd);
 	if (ret) {
 		v4l_dbg(1, debug, client,
 			"chip found @ 0x%x (%s) is not an ov7670 chip.\n",
 			client->addr << 1, client->adapter->name);
-		goto clk_disable;
+		goto power_off;
 	}
 	v4l_info(client, "chip found @ 0x%02x (%s)\n",
 			client->addr << 1, client->adapter->name);
@@ -1787,6 +1806,8 @@ static int ov7670_probe(struct i2c_client *client,
 #endif
 hdl_free:
 	v4l2_ctrl_handler_free(&info->hdl);
+power_off:
+	ov7670_s_power(sd, 0);
 clk_disable:
 	clk_disable_unprepare(info->clk);
 	return ret;
@@ -1804,6 +1825,7 @@ static int ov7670_remove(struct i2c_client *client)
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	media_entity_cleanup(&info->sd.entity);
 #endif
+	ov7670_s_power(sd, 0);
 	return 0;
 }
 
-- 
2.13.0

Re: [PATCH v4 2/3] media: ov7670: Add the get_fmt callback

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3414 bytes --]

Hi Wenyou,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.14-rc1 next-20170918]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wenyou-Yang/media-ov7670-Add-entity-init-and-power-operation/20170918-145527
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x003-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/media/i2c/ov7670.c: In function 'ov7670_set_fmt':
>> drivers/media/i2c/ov7670.c:1006:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
      struct v4l2_mbus_framefmt *mbus_fmt;
      ^~~~~~

vim +1006 drivers/media/i2c/ov7670.c

   984	
   985	/*
   986	 * Set a format.
   987	 */
   988	static int ov7670_set_fmt(struct v4l2_subdev *sd,
   989			struct v4l2_subdev_pad_config *cfg,
   990			struct v4l2_subdev_format *format)
   991	{
   992		struct ov7670_format_struct *ovfmt;
   993		struct ov7670_win_size *wsize;
   994		struct ov7670_info *info = to_state(sd);
   995		unsigned char com7;
   996		int ret;
   997	
   998		if (format->pad)
   999			return -EINVAL;
  1000	
  1001		if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
  1002			ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
  1003			if (ret)
  1004				return ret;
  1005	#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> 1006			struct v4l2_mbus_framefmt *mbus_fmt;
  1007	
  1008			mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
  1009			*mbus_fmt = format->format;
  1010			return 0;
  1011	#else
  1012			return -ENOTTY;
  1013	#endif
  1014		}
  1015	
  1016		ret = ov7670_try_fmt_internal(sd, &format->format, &ovfmt, &wsize);
  1017	
  1018		if (ret)
  1019			return ret;
  1020		/*
  1021		 * COM7 is a pain in the ass, it doesn't like to be read then
  1022		 * quickly written afterward.  But we have everything we need
  1023		 * to set it absolutely here, as long as the format-specific
  1024		 * register sets list it first.
  1025		 */
  1026		com7 = ovfmt->regs[0].value;
  1027		com7 |= wsize->com7_bit;
  1028		ov7670_write(sd, REG_COM7, com7);
  1029		/*
  1030		 * Now write the rest of the array.  Also store start/stops
  1031		 */
  1032		ov7670_write_array(sd, ovfmt->regs + 1);
  1033		ov7670_set_hw(sd, wsize->hstart, wsize->hstop, wsize->vstart,
  1034				wsize->vstop);
  1035		ret = 0;
  1036		if (wsize->regs)
  1037			ret = ov7670_write_array(sd, wsize->regs);
  1038		info->fmt = ovfmt;
  1039	
  1040		/*
  1041		 * If we're running RGB565, we must rewrite clkrc after setting
  1042		 * the other parameters or the image looks poor.  If we're *not*
  1043		 * doing RGB565, we must not rewrite clkrc or the image looks
  1044		 * *really* poor.
  1045		 *
  1046		 * (Update) Now that we retain clkrc state, we should be able
  1047		 * to write it unconditionally, and that will make the frame
  1048		 * rate persistent too.
  1049		 */
  1050		if (ret == 0)
  1051			ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
  1052		return 0;
  1053	}
  1054	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31190 bytes --]

Re: [PATCH v4 3/3] media: ov7670: Add the s_power operation

[Sakari Ailus]

Hi Wenyou,

Thanks for the update.

The driver exposes controls that are accessible through the sub-device node
even if the device hasn't been powered on.

Many ISP and bridge drivers will also power the sensor down once the last
user of the user space device nodes disappears. You could keep the device
powered at all times or change the behaviour so that controls can be
accessed when the power is off.

The best option would be to convert the driver to use runtime PM. An
example of this can be found in drivers/media/i2c/ov13858.c .

On Mon, Sep 18, 2017 at 02:45:14PM +0800, Wenyou Yang wrote:
> Add the s_power operation which is responsible for manipulating the
> power dowm mode through the PWDN pin and the reset operation through
> the RESET pin.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
>  - Add the patch to support the get_fmt ops.
>  - Remove the redundant invoking ov7670_init_gpio().
> 
>  drivers/media/i2c/ov7670.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 456f48057605..304abc769a67 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -1542,6 +1542,22 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
>  }
>  #endif
>  
> +static int ov7670_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct ov7670_info *info = to_state(sd);
> +
> +	if (info->pwdn_gpio)
> +		gpiod_direction_output(info->pwdn_gpio, !on);
> +	if (on && info->resetb_gpio) {
> +		gpiod_set_value(info->resetb_gpio, 1);
> +		usleep_range(500, 1000);
> +		gpiod_set_value(info->resetb_gpio, 0);
> +		usleep_range(3000, 5000);
> +	}
> +
> +	return 0;
> +}
> +
>  static void ov7670_get_default_format(struct v4l2_subdev *sd,
>  				      struct v4l2_mbus_framefmt *format)
>  {
> @@ -1575,6 +1591,7 @@ static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>  	.g_register = ov7670_g_register,
>  	.s_register = ov7670_s_register,
>  #endif
> +	.s_power = ov7670_s_power,
>  };
>  
>  static const struct v4l2_subdev_video_ops ov7670_video_ops = {
> @@ -1692,23 +1709,25 @@ static int ov7670_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> -	ret = ov7670_init_gpio(client, info);
> -	if (ret)
> -		goto clk_disable;
> -
>  	info->clock_speed = clk_get_rate(info->clk) / 1000000;
>  	if (info->clock_speed < 10 || info->clock_speed > 48) {
>  		ret = -EINVAL;
>  		goto clk_disable;
>  	}
>  
> +	ret = ov7670_init_gpio(client, info);
> +	if (ret)
> +		goto clk_disable;
> +
> +	ov7670_s_power(sd, 1);
> +
>  	/* Make sure it's an ov7670 */
>  	ret = ov7670_detect(sd);
>  	if (ret) {
>  		v4l_dbg(1, debug, client,
>  			"chip found @ 0x%x (%s) is not an ov7670 chip.\n",
>  			client->addr << 1, client->adapter->name);
> -		goto clk_disable;
> +		goto power_off;
>  	}
>  	v4l_info(client, "chip found @ 0x%02x (%s)\n",
>  			client->addr << 1, client->adapter->name);
> @@ -1787,6 +1806,8 @@ static int ov7670_probe(struct i2c_client *client,
>  #endif
>  hdl_free:
>  	v4l2_ctrl_handler_free(&info->hdl);
> +power_off:
> +	ov7670_s_power(sd, 0);
>  clk_disable:
>  	clk_disable_unprepare(info->clk);
>  	return ret;
> @@ -1804,6 +1825,7 @@ static int ov7670_remove(struct i2c_client *client)
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	media_entity_cleanup(&info->sd.entity);
>  #endif
> +	ov7670_s_power(sd, 0);
>  	return 0;
>  }
>  
> -- 
> 2.13.0
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v4 2/3] media: ov7670: Add the get_fmt callback

[Sakari Ailus]

Hi Wenyou,

On Mon, Sep 18, 2017 at 02:45:13PM +0800, Wenyou Yang wrote:
> @@ -998,8 +1002,15 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>  		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
>  		if (ret)
>  			return ret;
> -		cfg->try_fmt = format->format;
> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> +		struct v4l2_mbus_framefmt *mbus_fmt;

This will emit a compiler warning at least.

> +
> +		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
> +		*mbus_fmt = format->format;
>  		return 0;
> +#else
> +		return -ENOTTY;
> +#endif
>  	}
>  
>  	ret = ov7670_try_fmt_internal(sd, &format->format, &ovfmt, &wsize);

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v4 2/3] media: ov7670: Add the get_fmt callback

[Yang, Wenyou]

Hi Sakari,


On 2017/9/18 15:36, Sakari Ailus wrote:
> Hi Wenyou,
>
> On Mon, Sep 18, 2017 at 02:45:13PM +0800, Wenyou Yang wrote:
>> @@ -998,8 +1002,15 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>>   		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
>>   		if (ret)
>>   			return ret;
>> -		cfg->try_fmt = format->format;
>> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
>> +		struct v4l2_mbus_framefmt *mbus_fmt;
> This will emit a compiler warning at least.
Thank you for your review.
Will be fixed in next version.
>
>> +
>> +		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
>> +		*mbus_fmt = format->format;
>>   		return 0;
>> +#else
>> +		return -ENOTTY;
>> +#endif
>>   	}
>>   
>>   	ret = ov7670_try_fmt_internal(sd, &format->format, &ovfmt, &wsize);

Best Regards,
Wenyou Yang

Re: [PATCH v4 3/3] media: ov7670: Add the s_power operation

[Yang, Wenyou]

Hi Sakari,


On 2017/9/18 15:35, Sakari Ailus wrote:
> Hi Wenyou,
>
> Thanks for the update.
>
> The driver exposes controls that are accessible through the sub-device node
> even if the device hasn't been powered on.
>
> Many ISP and bridge drivers will also power the sensor down once the last
> user of the user space device nodes disappears. You could keep the device
> powered at all times or change the behaviour so that controls can be
> accessed when the power is off.
>
> The best option would be to convert the driver to use runtime PM.
Yes, I agree with you.
I also noticed there are a lot of things needed to improve except for 
it, such as the platform data via device tree.
I would like do it in another patch set. I will continue to work on it.
> An example of this can be found in drivers/media/i2c/ov13858.c .
A good example, thank you for your providing.
>
> On Mon, Sep 18, 2017 at 02:45:14PM +0800, Wenyou Yang wrote:
>> Add the s_power operation which is responsible for manipulating the
>> power dowm mode through the PWDN pin and the reset operation through
>> the RESET pin.
>>
>> Signed-off-by: Wenyou Yang <wenyou.yang@microchip.com>
>> ---
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2:
>>   - Add the patch to support the get_fmt ops.
>>   - Remove the redundant invoking ov7670_init_gpio().
>>
>>   drivers/media/i2c/ov7670.c | 32 +++++++++++++++++++++++++++-----
>>   1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
>> index 456f48057605..304abc769a67 100644
>> --- a/drivers/media/i2c/ov7670.c
>> +++ b/drivers/media/i2c/ov7670.c
>> @@ -1542,6 +1542,22 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
>>   }
>>   #endif
>>   
>> +static int ov7670_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +	struct ov7670_info *info = to_state(sd);
>> +
>> +	if (info->pwdn_gpio)
>> +		gpiod_direction_output(info->pwdn_gpio, !on);
>> +	if (on && info->resetb_gpio) {
>> +		gpiod_set_value(info->resetb_gpio, 1);
>> +		usleep_range(500, 1000);
>> +		gpiod_set_value(info->resetb_gpio, 0);
>> +		usleep_range(3000, 5000);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static void ov7670_get_default_format(struct v4l2_subdev *sd,
>>   				      struct v4l2_mbus_framefmt *format)
>>   {
>> @@ -1575,6 +1591,7 @@ static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>>   	.g_register = ov7670_g_register,
>>   	.s_register = ov7670_s_register,
>>   #endif
>> +	.s_power = ov7670_s_power,
>>   };
>>   
>>   static const struct v4l2_subdev_video_ops ov7670_video_ops = {
>> @@ -1692,23 +1709,25 @@ static int ov7670_probe(struct i2c_client *client,
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = ov7670_init_gpio(client, info);
>> -	if (ret)
>> -		goto clk_disable;
>> -
>>   	info->clock_speed = clk_get_rate(info->clk) / 1000000;
>>   	if (info->clock_speed < 10 || info->clock_speed > 48) {
>>   		ret = -EINVAL;
>>   		goto clk_disable;
>>   	}
>>   
>> +	ret = ov7670_init_gpio(client, info);
>> +	if (ret)
>> +		goto clk_disable;
>> +
>> +	ov7670_s_power(sd, 1);
>> +
>>   	/* Make sure it's an ov7670 */
>>   	ret = ov7670_detect(sd);
>>   	if (ret) {
>>   		v4l_dbg(1, debug, client,
>>   			"chip found @ 0x%x (%s) is not an ov7670 chip.\n",
>>   			client->addr << 1, client->adapter->name);
>> -		goto clk_disable;
>> +		goto power_off;
>>   	}
>>   	v4l_info(client, "chip found @ 0x%02x (%s)\n",
>>   			client->addr << 1, client->adapter->name);
>> @@ -1787,6 +1806,8 @@ static int ov7670_probe(struct i2c_client *client,
>>   #endif
>>   hdl_free:
>>   	v4l2_ctrl_handler_free(&info->hdl);
>> +power_off:
>> +	ov7670_s_power(sd, 0);
>>   clk_disable:
>>   	clk_disable_unprepare(info->clk);
>>   	return ret;
>> @@ -1804,6 +1825,7 @@ static int ov7670_remove(struct i2c_client *client)
>>   #if defined(CONFIG_MEDIA_CONTROLLER)
>>   	media_entity_cleanup(&info->sd.entity);
>>   #endif
>> +	ov7670_s_power(sd, 0);
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.13.0
>>

Best Regards,
Wenyou Yang

Re: [PATCH v4 3/3] media: ov7670: Add the s_power operation

[Sakari Ailus]

Hi Wenyou,

On Mon, Sep 18, 2017 at 05:26:16PM +0800, Yang, Wenyou wrote:
> Hi Sakari,
> 
> 
> On 2017/9/18 15:35, Sakari Ailus wrote:
> > Hi Wenyou,
> > 
> > Thanks for the update.
> > 
> > The driver exposes controls that are accessible through the sub-device node
> > even if the device hasn't been powered on.
> > 
> > Many ISP and bridge drivers will also power the sensor down once the last
> > user of the user space device nodes disappears. You could keep the device
> > powered at all times or change the behaviour so that controls can be
> > accessed when the power is off.
> > 
> > The best option would be to convert the driver to use runtime PM.
> Yes, I agree with you.
> I also noticed there are a lot of things needed to improve except for it,
> such as the platform data via device tree.
> I would like do it in another patch set. I will continue to work on it.

Adding runtime PM support later on sound good to me.

> > An example of this can be found in drivers/media/i2c/ov13858.c .
> A good example, thank you for your providing.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi