2017-09-19 00:46:42

by Wenyou Yang

[permalink] [raw]
Subject: [PATCH v5 0/3] media: ov7670: Add entity init and power operation

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

Changes in v5:
- Fix the build warning on the declaration *mbus_fmt(ISO C90 forbids mixed).

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 | 130 ++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 123 insertions(+), 7 deletions(-)

--
2.13.0


2017-09-19 00:47:06

by Wenyou Yang

[permalink] [raw]
Subject: [PATCH v5 1/3] media: ov7670: Add entity pads initialization

Add the media entity pads initialization.

Signed-off-by: Wenyou Yang <[email protected]>
---

Changes in v5: None
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

2017-09-19 00:47:33

by Wenyou Yang

[permalink] [raw]
Subject: [PATCH v5 2/3] media: ov7670: Add the get_fmt callback

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 <[email protected]>
---

Changes in v5:
- Fix the build warning on the declaration *mbus_fmt(ISO C90 forbids mixed).

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 | 77 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 553945d4ca28..73ceec63a8ca 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;
}

@@ -988,6 +992,9 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
struct ov7670_format_struct *ovfmt;
struct ov7670_win_size *wsize;
struct ov7670_info *info = to_state(sd);
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+ struct v4l2_mbus_framefmt *mbus_fmt;
+#endif
unsigned char com7;
int ret;

@@ -998,8 +1005,13 @@ 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
+ 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 +1053,30 @@ 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);
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+ struct v4l2_mbus_framefmt *mbus_fmt;
+#endif
+
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+ 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 +1544,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 +1588,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 +1598,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 +1656,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 +1717,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

2017-09-19 00:48:01

by Wenyou Yang

[permalink] [raw]
Subject: [PATCH v5 3/3] media: ov7670: Add the s_power operation

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 <[email protected]>
---

Changes in v5: None
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 73ceec63a8ca..8e86479d8a24 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1544,6 +1544,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)
{
@@ -1577,6 +1593,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 = {
@@ -1694,23 +1711,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);
@@ -1789,6 +1808,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;
@@ -1806,6 +1827,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

2017-09-19 07:36:47

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v5 3/3] media: ov7670: Add the s_power operation

Hi Wenyou,

On Tue, Sep 19, 2017 at 08:45:09AM +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.

This is still broken: accessing controls through the sub-device node won't
work when the device is powered off. The options are what I suggested the
previous time: keep it powered at all times, fix it or convert to runtime
PM.

>
> Signed-off-by: Wenyou Yang <[email protected]>
> ---
>
> Changes in v5: None
> 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 73ceec63a8ca..8e86479d8a24 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -1544,6 +1544,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)
> {
> @@ -1577,6 +1593,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 = {
> @@ -1694,23 +1711,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);
> @@ -1789,6 +1808,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;
> @@ -1806,6 +1827,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
>

--
Regards,

Sakari Ailus
e-mail: [email protected]