* [PATCH v4 1/3] media: ov7670: Add entity pads initialization
2017-09-18 6:45 [PATCH v4 0/3] media: ov7670: Add entity init and power operation Wenyou Yang
@ 2017-09-18 6:45 ` Wenyou Yang
2017-09-18 6:45 ` [PATCH v4 2/3] media: ov7670: Add the get_fmt callback Wenyou Yang
2017-09-18 6:45 ` [PATCH v4 3/3] media: ov7670: Add the s_power operation Wenyou Yang
2 siblings, 0 replies; 10+ messages in thread
From: Wenyou Yang @ 2017-09-18 6:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4 2/3] media: ov7670: Add the get_fmt callback
2017-09-18 6:45 [PATCH v4 0/3] media: ov7670: Add entity init and power operation Wenyou Yang
2017-09-18 6:45 ` [PATCH v4 1/3] media: ov7670: Add entity pads initialization Wenyou Yang
@ 2017-09-18 6:45 ` Wenyou Yang
2017-09-18 7:35 ` kbuild test robot
2017-09-18 7:36 ` Sakari Ailus
2017-09-18 6:45 ` [PATCH v4 3/3] media: ov7670: Add the s_power operation Wenyou Yang
2 siblings, 2 replies; 10+ messages in thread
From: Wenyou Yang @ 2017-09-18 6:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4 2/3] media: ov7670: Add the get_fmt callback
2017-09-18 6:45 ` [PATCH v4 2/3] media: ov7670: Add the get_fmt callback Wenyou Yang
@ 2017-09-18 7:35 ` kbuild test robot
2017-09-18 7:36 ` Sakari Ailus
1 sibling, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2017-09-18 7:35 UTC (permalink / raw)
To: linux-arm-kernel
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 31190 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170918/5d393aa1/attachment-0001.gz>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4 2/3] media: ov7670: Add the get_fmt callback
2017-09-18 6:45 ` [PATCH v4 2/3] media: ov7670: Add the get_fmt callback Wenyou Yang
2017-09-18 7:35 ` kbuild test robot
@ 2017-09-18 7:36 ` Sakari Ailus
2017-09-18 9:25 ` Yang, Wenyou
1 sibling, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2017-09-18 7:36 UTC (permalink / raw)
To: linux-arm-kernel
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 at iki.fi
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/3] media: ov7670: Add the get_fmt callback
2017-09-18 7:36 ` Sakari Ailus
@ 2017-09-18 9:25 ` Yang, Wenyou
0 siblings, 0 replies; 10+ messages in thread
From: Yang, Wenyou @ 2017-09-18 9:25 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 3/3] media: ov7670: Add the s_power operation
2017-09-18 6:45 [PATCH v4 0/3] media: ov7670: Add entity init and power operation Wenyou Yang
2017-09-18 6:45 ` [PATCH v4 1/3] media: ov7670: Add entity pads initialization Wenyou Yang
2017-09-18 6:45 ` [PATCH v4 2/3] media: ov7670: Add the get_fmt callback Wenyou Yang
@ 2017-09-18 6:45 ` Wenyou Yang
2017-09-18 7:35 ` Sakari Ailus
2 siblings, 1 reply; 10+ messages in thread
From: Wenyou Yang @ 2017-09-18 6:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4 3/3] media: ov7670: Add the s_power operation
2017-09-18 6:45 ` [PATCH v4 3/3] media: ov7670: Add the s_power operation Wenyou Yang
@ 2017-09-18 7:35 ` Sakari Ailus
2017-09-18 9:26 ` Yang, Wenyou
0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2017-09-18 7:35 UTC (permalink / raw)
To: linux-arm-kernel
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 at iki.fi
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4 3/3] media: ov7670: Add the s_power operation
2017-09-18 7:35 ` Sakari Ailus
@ 2017-09-18 9:26 ` Yang, Wenyou
2017-09-18 13:26 ` Sakari Ailus
0 siblings, 1 reply; 10+ messages in thread
From: Yang, Wenyou @ 2017-09-18 9:26 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4 3/3] media: ov7670: Add the s_power operation
2017-09-18 9:26 ` Yang, Wenyou
@ 2017-09-18 13:26 ` Sakari Ailus
0 siblings, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2017-09-18 13:26 UTC (permalink / raw)
To: linux-arm-kernel
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 at iki.fi
^ permalink raw reply [flat|nested] 10+ messages in thread