[PATCH 0/6] media: ov772x: support media controller, device tree probing, etc.

[PATCH 0/6] media: ov772x: support media controller, device tree probing, etc.

[Akinobu Mita]

This patchset includes support media controller, device tree probing and
other miscellanuous changes for ov772x driver.

Akinobu Mita (6):
  media: ov772x: allow i2c controllers without
    I2C_FUNC_PROTOCOL_MANGLING
  media: ov772x: add checks for register read errors
  media: ov772x: create subdevice device node
  media: ov772x: add media controller support
  media: ov772x: add device tree binding
  media: ov772x: support device tree probing

 .../devicetree/bindings/media/i2c/ov772x.txt       |  36 ++++++
 MAINTAINERS                                        |   1 +
 drivers/media/i2c/ov772x.c                         | 136 ++++++++++++++++-----
 3 files changed, 140 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: Rob Herring <robh+dt@kernel.org>
-- 
2.7.4

[PATCH 1/6] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Akinobu Mita]

The ov772x driver only works when the i2c controller have
I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
support it.

The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
it doesn't support repeated starts.

This change adds an alternative method for reading from ov772x register
which uses two separated i2c messages for the i2c controllers without
I2C_FUNC_PROTOCOL_MANGLING.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/ov772x.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..283ae2c 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,6 +424,7 @@ struct ov772x_priv {
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
 	unsigned short                    band_filter;
 	unsigned int			  fps;
+	int (*reg_read)(struct i2c_client *client, u8 addr);
 };
 
 /*
@@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov772x_priv, subdev);
 }
 
-static inline int ov772x_read(struct i2c_client *client, u8 addr)
+static int ov772x_read(struct i2c_client *client, u8 addr)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov772x_priv *priv = to_ov772x(sd);
+
+	return priv->reg_read(client, addr);
+}
+
+static int ov772x_reg_read(struct i2c_client *client, u8 addr)
 {
 	return i2c_smbus_read_byte_data(client, addr);
 }
 
+static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr)
+{
+	int ret;
+	u8 val;
+
+	ret = i2c_master_send(client, &addr, 1);
+	if (ret < 0)
+		return ret;
+	ret = i2c_master_recv(client, &val, 1);
+	if (ret < 0)
+		return ret;
+
+	return val;
+}
+
 static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
 {
 	return i2c_smbus_write_byte_data(client, addr, value);
@@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
-					      I2C_FUNC_PROTOCOL_MANGLING)) {
-		dev_err(&adapter->dev,
-			"I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
-		return -EIO;
-	}
-	client->flags |= I2C_CLIENT_SCCB;
-
 	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	priv->info = client->dev.platform_data;
 
+	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+					     I2C_FUNC_PROTOCOL_MANGLING))
+		priv->reg_read = ov772x_reg_read;
+	else
+		priv->reg_read = ov772x_reg_read_fallback;
+
+	client->flags |= I2C_CLIENT_SCCB;
+
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
 	v4l2_ctrl_handler_init(&priv->hdl, 3);
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
-- 
2.7.4

[PATCH 2/6] media: ov772x: add checks for register read errors

[Akinobu Mita]

This change adds checks for register read errors and returns correct
error code.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/ov772x.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 283ae2c..c56f910 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1169,8 +1169,15 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 		return ret;
 
 	/* Check and show product ID and manufacturer ID. */
-	pid = ov772x_read(client, PID);
-	ver = ov772x_read(client, VER);
+	ret = ov772x_read(client, PID);
+	if (ret < 0)
+		return ret;
+	pid = ret;
+
+	ret = ov772x_read(client, VER);
+	if (ret < 0)
+		return ret;
+	ver = ret;
 
 	switch (VERSION(pid, ver)) {
 	case OV7720:
-- 
2.7.4

[PATCH 3/6] media: ov772x: create subdevice device node

[Akinobu Mita]

Set the V4L2_SUBDEV_FL_HAS_DEVNODE flag for the subdevice so that the
subdevice device node is created.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/ov772x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index c56f910..4bb81ff 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1301,6 +1301,7 @@ static int ov772x_probe(struct i2c_client *client,
 	client->flags |= I2C_CLIENT_SCCB;
 
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
+	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	v4l2_ctrl_handler_init(&priv->hdl, 3);
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 			  V4L2_CID_VFLIP, 0, 1, 1, 0);
-- 
2.7.4

[PATCH 4/6] media: ov772x: add media controller support

[Akinobu Mita]

Create a source pad and set the media controller type to the sensor.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/ov772x.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 4bb81ff..5e91fa1 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -425,6 +425,9 @@ struct ov772x_priv {
 	unsigned short                    band_filter;
 	unsigned int			  fps;
 	int (*reg_read)(struct i2c_client *client, u8 addr);
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_pad pad;
+#endif
 };
 
 /*
@@ -1328,9 +1331,17 @@ static int ov772x_probe(struct i2c_client *client,
 		goto error_clk_put;
 	}
 
-	ret = ov772x_video_probe(priv);
+#ifdef CONFIG_MEDIA_CONTROLLER
+	priv->pad.flags = MEDIA_PAD_FL_SOURCE;
+	priv->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&priv->subdev.entity, 1, &priv->pad);
 	if (ret < 0)
 		goto error_gpio_put;
+#endif
+
+	ret = ov772x_video_probe(priv);
+	if (ret < 0)
+		goto error_entity_cleanup;
 
 	priv->cfmt = &ov772x_cfmts[0];
 	priv->win = &ov772x_win_sizes[0];
@@ -1338,11 +1349,15 @@ static int ov772x_probe(struct i2c_client *client,
 
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret)
-		goto error_gpio_put;
+		goto error_entity_cleanup;
 
 	return 0;
 
+error_entity_cleanup:
+#ifdef CONFIG_MEDIA_CONTROLLER
+	media_entity_cleanup(&priv->subdev.entity);
 error_gpio_put:
+#endif
 	if (priv->pwdn_gpio)
 		gpiod_put(priv->pwdn_gpio);
 error_clk_put:
@@ -1357,6 +1372,9 @@ static int ov772x_remove(struct i2c_client *client)
 {
 	struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+	media_entity_cleanup(&priv->subdev.entity);
+#endif
 	clk_put(priv->clk);
 	if (priv->pwdn_gpio)
 		gpiod_put(priv->pwdn_gpio);
-- 
2.7.4

[PATCH 5/6] media: ov772x: add device tree binding

[Akinobu Mita]

This adds a device tree binding documentation for OV7720/OV7725 sensor.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 .../devicetree/bindings/media/i2c/ov772x.txt       | 36 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
new file mode 100644
index 0000000..9b0df3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
@@ -0,0 +1,36 @@
+* Omnivision OV7720/OV7725 CMOS sensor
+
+Required Properties:
+- compatible: shall be one of
+	"ovti,ov7720"
+	"ovti,ov7725"
+- clocks: reference to the xclk input clock.
+- clock-names: shall be "xclk".
+
+Optional Properties:
+- rstb-gpios: reference to the GPIO connected to the RSTB pin, if any.
+- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any.
+
+The device node shall contain one 'port' child node with one child 'endpoint'
+subnode for its digital output video port, in accordance with the video
+interface bindings defined in Documentation/devicetree/bindings/media/
+video-interfaces.txt.
+
+Example:
+
+&i2c0 {
+	ov772x: camera@21 {
+		compatible = "ovti,ov7725";
+		reg = <0x21>;
+		rstb-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
+		pwdn-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
+		clocks = <&xclk>;
+		clock-names = "xclk";
+
+		port {
+			ov772x_0: endpoint {
+				remote-endpoint = <&vcap1_in0>;
+			};
+		};
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 7e48624..3e0224a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10295,6 +10295,7 @@ T:	git git://linuxtv.org/media_tree.git
 S:	Odd fixes
 F:	drivers/media/i2c/ov772x.c
 F:	include/media/i2c/ov772x.h
+F:	Documentation/devicetree/bindings/media/i2c/ov772x.txt
 
 OMNIVISION OV7740 SENSOR DRIVER
 M:	Wenyou Yang <wenyou.yang@microchip.com>
-- 
2.7.4

[PATCH 6/6] media: ov772x: support device tree probing

[Akinobu Mita]

The ov772x driver currently only supports legacy platform data probe.
This change enables device tree probing.

Note that the platform data probe can select auto or manual edge control
mode, but the device tree probling can only select auto edge control mode
for now.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/ov772x.c | 60 ++++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 5e91fa1..e67ec37 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -763,13 +763,13 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_VFLIP:
 		val = ctrl->val ? VFLIP_IMG : 0x00;
 		priv->flag_vflip = ctrl->val;
-		if (priv->info->flags & OV772X_FLAG_VFLIP)
+		if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
 			val ^= VFLIP_IMG;
 		return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
 	case V4L2_CID_HFLIP:
 		val = ctrl->val ? HFLIP_IMG : 0x00;
 		priv->flag_hflip = ctrl->val;
-		if (priv->info->flags & OV772X_FLAG_HFLIP)
+		if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
 			val ^= HFLIP_IMG;
 		return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
 	case V4L2_CID_BAND_STOP_FILTER:
@@ -928,19 +928,14 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
 	*win = ov772x_select_win(mf->width, mf->height);
 }
 
-static int ov772x_set_params(struct ov772x_priv *priv,
-			     const struct ov772x_color_format *cfmt,
-			     const struct ov772x_win_size *win)
+static int ov772x_edgectrl(struct ov772x_priv *priv)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
-	struct v4l2_fract tpf;
 	int ret;
-	u8  val;
 
-	/* Reset hardware. */
-	ov772x_reset(client);
+	if (!priv->info)
+		return 0;
 
-	/* Edge Ctrl. */
 	if (priv->info->edgectrl.strength & OV772X_MANUAL_EDGE_CTRL) {
 		/*
 		 * Manual Edge Control Mode.
@@ -951,19 +946,19 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
 		ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 		ret = ov772x_mask_set(client,
 				      EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
 				      priv->info->edgectrl.threshold);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 		ret = ov772x_mask_set(client,
 				      EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
 				      priv->info->edgectrl.strength);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 	} else if (priv->info->edgectrl.upper > priv->info->edgectrl.lower) {
 		/*
@@ -975,15 +970,35 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 				      EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
 				      priv->info->edgectrl.upper);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 		ret = ov772x_mask_set(client,
 				      EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
 				      priv->info->edgectrl.lower);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 	}
 
+	return 0;
+}
+
+static int ov772x_set_params(struct ov772x_priv *priv,
+			     const struct ov772x_color_format *cfmt,
+			     const struct ov772x_win_size *win)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
+	struct v4l2_fract tpf;
+	int ret;
+	u8  val;
+
+	/* Reset hardware. */
+	ov772x_reset(client);
+
+	/* Edge Ctrl. */
+	ret =  ov772x_edgectrl(priv);
+	if (ret < 0)
+		goto ov772x_set_fmt_error;
+
 	/* Format and window size. */
 	ret = ov772x_write(client, HSTART, win->rect.left >> 2);
 	if (ret < 0)
@@ -1284,11 +1299,6 @@ static int ov772x_probe(struct i2c_client *client,
 	struct i2c_adapter	*adapter = client->adapter;
 	int			ret;
 
-	if (!client->dev.platform_data) {
-		dev_err(&client->dev, "Missing ov772x platform data\n");
-		return -EINVAL;
-	}
-
 	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -1390,9 +1400,19 @@ static const struct i2c_device_id ov772x_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ov772x_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id ov772x_of_match[] = {
+	{ .compatible = "ovti,ov7725", },
+	{ .compatible = "ovti,ov7720", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov772x_of_match);
+#endif
+
 static struct i2c_driver ov772x_i2c_driver = {
 	.driver = {
 		.name = "ov772x",
+		.of_match_table = of_match_ptr(ov772x_of_match),
 	},
 	.probe    = ov772x_probe,
 	.remove   = ov772x_remove,
-- 
2.7.4

Re: [PATCH 1/6] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[jacopo mondi]

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

Hello Akinobu,
    thank you for the patch.

On which platform have you tested the series (just curious) ?

On Sun, Apr 08, 2018 at 12:48:05AM +0900, Akinobu Mita wrote:
> The ov772x driver only works when the i2c controller have
> I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> support it.
>
> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> it doesn't support repeated starts.
>
> This change adds an alternative method for reading from ov772x register
> which uses two separated i2c messages for the i2c controllers without
> I2C_FUNC_PROTOCOL_MANGLING.

Actually, and please correct me if I'm wrong, what I see here is that
an i2c_master_send+i2c_master_recv sequence is equivalent to a mangled
smbus transaction:

i2c_smbus_read_byte_data | I2C_CLIENT_SCCB:
S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P

i2c_master_send() + i2c_master_recv():
S Addr Wr [A] Data [A] P
S Addr Rd [A] [Data] NA P

I wonder if it is not worth to ditch the existing read() function in
favour of your new proposed one completely.

I have tested it on the Migo-R board where I have an ov772x installed
and it works fine.

Although I would like to have a confirmation this is fine by people
how has seen more i2c adapters in action than me :)

Thanks
   j

>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/ov772x.c | 42 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index b62860c..283ae2c 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -424,6 +424,7 @@ struct ov772x_priv {
>  	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>  	unsigned short                    band_filter;
>  	unsigned int			  fps;
> +	int (*reg_read)(struct i2c_client *client, u8 addr);
>  };
>
>  /*
> @@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
>  	return container_of(sd, struct ov772x_priv, subdev);
>  }
>
> -static inline int ov772x_read(struct i2c_client *client, u8 addr)
> +static int ov772x_read(struct i2c_client *client, u8 addr)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov772x_priv *priv = to_ov772x(sd);
> +
> +	return priv->reg_read(client, addr);
> +}
> +
> +static int ov772x_reg_read(struct i2c_client *client, u8 addr)
>  {
>  	return i2c_smbus_read_byte_data(client, addr);
>  }
>
> +static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = i2c_master_send(client, &addr, 1);
> +	if (ret < 0)
> +		return ret;
> +	ret = i2c_master_recv(client, &val, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return val;
> +}
> +
>  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
>  {
>  	return i2c_smbus_write_byte_data(client, addr, value);
> @@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>
> -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> -					      I2C_FUNC_PROTOCOL_MANGLING)) {
> -		dev_err(&adapter->dev,
> -			"I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
> -		return -EIO;
> -	}
> -	client->flags |= I2C_CLIENT_SCCB;
> -
>  	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>
>  	priv->info = client->dev.platform_data;
>
> +	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +					     I2C_FUNC_PROTOCOL_MANGLING))
> +		priv->reg_read = ov772x_reg_read;
> +	else
> +		priv->reg_read = ov772x_reg_read_fallback;
> +
> +	client->flags |= I2C_CLIENT_SCCB;
> +
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>  	v4l2_ctrl_handler_init(&priv->hdl, 3);
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/6] media: ov772x: add checks for register read errors

[jacopo mondi]

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

Hi Akinobu,

On Sun, Apr 08, 2018 at 12:48:06AM +0900, Akinobu Mita wrote:
> This change adds checks for register read errors and returns correct
> error code.
>

I feel like error conditions are anyway captured by the switch()
default case, but I understand there may be merits in returning the
actual error code.

> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/ov772x.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 283ae2c..c56f910 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1169,8 +1169,15 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  		return ret;
>
>  	/* Check and show product ID and manufacturer ID. */
> -	pid = ov772x_read(client, PID);
> -	ver = ov772x_read(client, VER);
> +	ret = ov772x_read(client, PID);
> +	if (ret < 0)
> +		return ret;
> +	pid = ret;
> +
> +	ret = ov772x_read(client, VER);
> +	if (ret < 0)
> +		return ret;
> +	ver = ret;

You can assign the ov772x_read() return value to pid and ver directly
and save two assignments.

>
>  	switch (VERSION(pid, ver)) {
>  	case OV7720:

If we want to check for return values here, which is always a good
thing, could you do the same for MIDH and MIDL below?

Nit: You can also fix the dev_info() parameters alignment to span to
the whole line length while at there. Ie.

	dev_info(&client->dev,
		 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
		 devname, pid, ver, ov772x_read(client, MIDH),
		 ov772x_read(client, MIDL));

Thanks
   j


> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/6] media: ov772x: add media controller support

[jacopo mondi]

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

Hi Akinobu,

On Sun, Apr 08, 2018 at 12:48:08AM +0900, Akinobu Mita wrote:
> Create a source pad and set the media controller type to the sensor.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/ov772x.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 4bb81ff..5e91fa1 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -425,6 +425,9 @@ struct ov772x_priv {
>  	unsigned short                    band_filter;
>  	unsigned int			  fps;
>  	int (*reg_read)(struct i2c_client *client, u8 addr);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	struct media_pad pad;
> +#endif
>  };
>
>  /*
> @@ -1328,9 +1331,17 @@ static int ov772x_probe(struct i2c_client *client,
>  		goto error_clk_put;
>  	}
>
> -	ret = ov772x_video_probe(priv);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	priv->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	priv->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	ret = media_entity_pads_init(&priv->subdev.entity, 1, &priv->pad);
>  	if (ret < 0)
>  		goto error_gpio_put;
> +#endif
> +
> +	ret = ov772x_video_probe(priv);
> +	if (ret < 0)
> +		goto error_entity_cleanup;

If you remove the #ifdef around the media_entity_cleanup() below, I
suggest moving video_probe() before the entity intialization so you
don't have to #ifdef around the error_gpio_put: label, which otherwise
the compiler complains for being defined but not used.

>
>  	priv->cfmt = &ov772x_cfmts[0];
>  	priv->win = &ov772x_win_sizes[0];
> @@ -1338,11 +1349,15 @@ static int ov772x_probe(struct i2c_client *client,
>
>  	ret = v4l2_async_register_subdev(&priv->subdev);
>  	if (ret)
> -		goto error_gpio_put;
> +		goto error_entity_cleanup;
>
>  	return 0;
>
> +error_entity_cleanup:
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	media_entity_cleanup(&priv->subdev.entity);

media_entity_cleanup() resolves to a nop if CONFIG_MEDIA_CONTROLLER is
not defined

>  error_gpio_put:
> +#endif
>  	if (priv->pwdn_gpio)
>  		gpiod_put(priv->pwdn_gpio);
>  error_clk_put:
> @@ -1357,6 +1372,9 @@ static int ov772x_remove(struct i2c_client *client)
>  {
>  	struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
>
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	media_entity_cleanup(&priv->subdev.entity);

ditto

Thanks
   j

> +#endif
>  	clk_put(priv->clk);
>  	if (priv->pwdn_gpio)
>  		gpiod_put(priv->pwdn_gpio);
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/6] media: ov772x: add device tree binding

[jacopo mondi]

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

Hi Akinobu,

On Sun, Apr 08, 2018 at 12:48:09AM +0900, Akinobu Mita wrote:
> This adds a device tree binding documentation for OV7720/OV7725 sensor.

Please use as patch subject
media: dt-bindings:

>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  .../devicetree/bindings/media/i2c/ov772x.txt       | 36 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> new file mode 100644
> index 0000000..9b0df3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> @@ -0,0 +1,36 @@
> +* Omnivision OV7720/OV7725 CMOS sensor
> +

Could you please provide a brief description of the sensor (supported
resolution and formats is ok)

> +Required Properties:
> +- compatible: shall be one of
> +	"ovti,ov7720"
> +	"ovti,ov7725"
> +- clocks: reference to the xclk input clock.
> +- clock-names: shall be "xclk".
> +
> +Optional Properties:
> +- rstb-gpios: reference to the GPIO connected to the RSTB pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any.

As a general note:
This is debated, and I'm not enforcing it, but please consider using
generic names for GPIOs with common functions. In this case
"reset-gpios" and "powerdown-gpios". Also please indicate the GPIO
active level in bindings description.

For this specific driver:
The probe routine already looks for a GPIO named 'pwdn', so I guess
the DT bindings should use the same name. Unless you're willing to
change it in the board files that register it (Migo-R only in mainline) and
use the generic 'powerdown' name for both. Either is fine with me.

There is no support for the reset GPIO in the driver code, it
supports soft reset only. Either ditch it from bindings or add support
for it in driver's code.

Thanks
   j

> +
> +The device node shall contain one 'port' child node with one child 'endpoint'
> +subnode for its digital output video port, in accordance with the video
> +interface bindings defined in Documentation/devicetree/bindings/media/
> +video-interfaces.txt.
> +
> +Example:
> +
> +&i2c0 {
> +	ov772x: camera@21 {
> +		compatible = "ovti,ov7725";
> +		reg = <0x21>;
> +		rstb-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
> +		pwdn-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
> +		clocks = <&xclk>;
> +		clock-names = "xclk";
> +
> +		port {
> +			ov772x_0: endpoint {
> +				remote-endpoint = <&vcap1_in0>;
> +			};
> +		};
> +	};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7e48624..3e0224a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10295,6 +10295,7 @@ T:	git git://linuxtv.org/media_tree.git
>  S:	Odd fixes
>  F:	drivers/media/i2c/ov772x.c
>  F:	include/media/i2c/ov772x.h
> +F:	Documentation/devicetree/bindings/media/i2c/ov772x.txt
>
>  OMNIVISION OV7740 SENSOR DRIVER
>  M:	Wenyou Yang <wenyou.yang@microchip.com>
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/6] media: ov772x: support device tree probing

[jacopo mondi]

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

Hi Akinobu,

On Sun, Apr 08, 2018 at 12:48:10AM +0900, Akinobu Mita wrote:
> The ov772x driver currently only supports legacy platform data probe.
> This change enables device tree probing.
>
> Note that the platform data probe can select auto or manual edge control
> mode, but the device tree probling can only select auto edge control mode
> for now.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/ov772x.c | 60 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 5e91fa1..e67ec37 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -763,13 +763,13 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_VFLIP:
>  		val = ctrl->val ? VFLIP_IMG : 0x00;
>  		priv->flag_vflip = ctrl->val;
> -		if (priv->info->flags & OV772X_FLAG_VFLIP)
> +		if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
>  			val ^= VFLIP_IMG;
>  		return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
>  	case V4L2_CID_HFLIP:
>  		val = ctrl->val ? HFLIP_IMG : 0x00;
>  		priv->flag_hflip = ctrl->val;
> -		if (priv->info->flags & OV772X_FLAG_HFLIP)
> +		if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
>  			val ^= HFLIP_IMG;
>  		return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
>  	case V4L2_CID_BAND_STOP_FILTER:
> @@ -928,19 +928,14 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
>  	*win = ov772x_select_win(mf->width, mf->height);
>  }
>
> -static int ov772x_set_params(struct ov772x_priv *priv,
> -			     const struct ov772x_color_format *cfmt,
> -			     const struct ov772x_win_size *win)
> +static int ov772x_edgectrl(struct ov772x_priv *priv)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> -	struct v4l2_fract tpf;
>  	int ret;
> -	u8  val;
>
> -	/* Reset hardware. */
> -	ov772x_reset(client);
> +	if (!priv->info)
> +		return 0;
>
> -	/* Edge Ctrl. */
>  	if (priv->info->edgectrl.strength & OV772X_MANUAL_EDGE_CTRL) {
>  		/*
>  		 * Manual Edge Control Mode.
> @@ -951,19 +946,19 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>
>  		ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  		ret = ov772x_mask_set(client,
>  				      EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
>  				      priv->info->edgectrl.threshold);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  		ret = ov772x_mask_set(client,
>  				      EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
>  				      priv->info->edgectrl.strength);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  	} else if (priv->info->edgectrl.upper > priv->info->edgectrl.lower) {
>  		/*
> @@ -975,15 +970,35 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  				      EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
>  				      priv->info->edgectrl.upper);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  		ret = ov772x_mask_set(client,
>  				      EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
>  				      priv->info->edgectrl.lower);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>  	}
>
> +	return 0;
> +}
> +
> +static int ov772x_set_params(struct ov772x_priv *priv,
> +			     const struct ov772x_color_format *cfmt,
> +			     const struct ov772x_win_size *win)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> +	struct v4l2_fract tpf;
> +	int ret;
> +	u8  val;
> +
> +	/* Reset hardware. */
> +	ov772x_reset(client);
> +
> +	/* Edge Ctrl. */
> +	ret =  ov772x_edgectrl(priv);
> +	if (ret < 0)
> +		goto ov772x_set_fmt_error;
> +
>  	/* Format and window size. */
>  	ret = ov772x_write(client, HSTART, win->rect.left >> 2);
>  	if (ret < 0)
> @@ -1284,11 +1299,6 @@ static int ov772x_probe(struct i2c_client *client,
>  	struct i2c_adapter	*adapter = client->adapter;
>  	int			ret;
>
> -	if (!client->dev.platform_data) {

Not sure this has to be removed completely. I'm debated, as in
general, for mainline code, we should make sure every user of this
driver shall provide platform_data during review, and this check is
not requried. But in general, I still think it may have value.

What about:
        if (!client->dev.of_node && !client->dev.platform_data) {
               dev_err(&client->dev, "Missing ov772x platform data\n");
               return -EINVAL;
        }

Thanks
   j

> -		dev_err(&client->dev, "Missing ov772x platform data\n");
> -		return -EINVAL;
> -	}
> -
>  	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> @@ -1390,9 +1400,19 @@ static const struct i2c_device_id ov772x_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ov772x_id);
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov772x_of_match[] = {
> +	{ .compatible = "ovti,ov7725", },
> +	{ .compatible = "ovti,ov7720", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov772x_of_match);
> +#endif
> +
>  static struct i2c_driver ov772x_i2c_driver = {
>  	.driver = {
>  		.name = "ov772x",
> +		.of_match_table = of_match_ptr(ov772x_of_match),
>  	},
>  	.probe    = ov772x_probe,
>  	.remove   = ov772x_remove,
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Laurent Pinchart]

Hello,

On Monday, 9 April 2018 09:58:12 EEST jacopo mondi wrote:
> Hello Akinobu,
>     thank you for the patch.
> 
> On which platform have you tested the series (just curious) ?
> 
> On Sun, Apr 08, 2018 at 12:48:05AM +0900, Akinobu Mita wrote:
> > The ov772x driver only works when the i2c controller have
> > I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> > support it.
> > 
> > The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> > it doesn't support repeated starts.
> > 
> > This change adds an alternative method for reading from ov772x register
> > which uses two separated i2c messages for the i2c controllers without
> > I2C_FUNC_PROTOCOL_MANGLING.
> 
> Actually, and please correct me if I'm wrong, what I see here is that
> an i2c_master_send+i2c_master_recv sequence is equivalent to a mangled
> smbus transaction:
> 
> i2c_smbus_read_byte_data | I2C_CLIENT_SCCB:
> S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P
> 
> i2c_master_send() + i2c_master_recv():
> S Addr Wr [A] Data [A] P
> S Addr Rd [A] [Data] NA P
> 
> I wonder if it is not worth to ditch the existing read() function in
> favour of your new proposed one completely.

Given that this implementation is in no way specific to the ov772x driver, 
wouldn't it be better to handle this fallback in the I2C core instead ?

> I have tested it on the Migo-R board where I have an ov772x installed
> and it works fine.
> 
> Although I would like to have a confirmation this is fine by people
> how has seen more i2c adapters in action than me :)
> 
> > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> > 
> >  drivers/media/i2c/ov772x.c | 42 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index b62860c..283ae2c 100644
> > --- a/drivers/media/i2c/ov772x.c
> > +++ b/drivers/media/i2c/ov772x.c
> > @@ -424,6 +424,7 @@ struct ov772x_priv {
> >  	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
> >  	unsigned short                    band_filter;
> >  	unsigned int			  fps;
> > +	int (*reg_read)(struct i2c_client *client, u8 addr);
> >  };
> >  
> >  /*
> > @@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct
> > v4l2_subdev *sd)
> >  	return container_of(sd, struct ov772x_priv, subdev);
> >  }
> > 
> > -static inline int ov772x_read(struct i2c_client *client, u8 addr)
> > +static int ov772x_read(struct i2c_client *client, u8 addr)
> > +{
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ov772x_priv *priv = to_ov772x(sd);
> > +
> > +	return priv->reg_read(client, addr);
> > +}
> > +
> > +static int ov772x_reg_read(struct i2c_client *client, u8 addr)
> >  {
> >  	return i2c_smbus_read_byte_data(client, addr);
> >  }
> > 
> > +static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr)
> > +{
> > +	int ret;
> > +	u8 val;
> > +
> > +	ret = i2c_master_send(client, &addr, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +	ret = i2c_master_recv(client, &val, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return val;
> > +}
> > +
> > 
> >  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8
> >  value) {
> >  	return i2c_smbus_write_byte_data(client, addr, value);
> > @@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client,
> >  		return -EINVAL;
> >  	}
> > 
> > -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> > -					      I2C_FUNC_PROTOCOL_MANGLING)) {
> > -		dev_err(&adapter->dev,
> > -			"I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING
\n");
> > -		return -EIO;
> > -	}
> > -	client->flags |= I2C_CLIENT_SCCB;
> > -
> >  	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> >  		return -ENOMEM;
> >  	
> >  	priv->info = client->dev.platform_data;
> > 
> > +	if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> > +					     I2C_FUNC_PROTOCOL_MANGLING))
> > +		priv->reg_read = ov772x_reg_read;
> > +	else
> > +		priv->reg_read = ov772x_reg_read_fallback;
> > +
> > +	client->flags |= I2C_CLIENT_SCCB;
> > +
> >  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
> >  	v4l2_ctrl_handler_init(&priv->hdl, 3);
> >  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: ov772x: add checks for register read errors

[Akinobu Mita]

2018-04-09 16:36 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>
> On Sun, Apr 08, 2018 at 12:48:06AM +0900, Akinobu Mita wrote:
>> This change adds checks for register read errors and returns correct
>> error code.
>>
>
> I feel like error conditions are anyway captured by the switch()
> default case, but I understand there may be merits in returning the
> actual error code.
>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>  drivers/media/i2c/ov772x.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 283ae2c..c56f910 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -1169,8 +1169,15 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>>               return ret;
>>
>>       /* Check and show product ID and manufacturer ID. */
>> -     pid = ov772x_read(client, PID);
>> -     ver = ov772x_read(client, VER);
>> +     ret = ov772x_read(client, PID);
>> +     if (ret < 0)
>> +             return ret;
>> +     pid = ret;
>> +
>> +     ret = ov772x_read(client, VER);
>> +     if (ret < 0)
>> +             return ret;
>> +     ver = ret;
>
> You can assign the ov772x_read() return value to pid and ver directly
> and save two assignments.

OK. This needs to change the data types of pid and ver from 'u8' to 'int'.

>>
>>       switch (VERSION(pid, ver)) {
>>       case OV7720:
>
> If we want to check for return values here, which is always a good
> thing, could you do the same for MIDH and MIDL below?

Sounds good.

> Nit: You can also fix the dev_info() parameters alignment to span to
> the whole line length while at there. Ie.
>
>         dev_info(&client->dev,
>                  "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
>                  devname, pid, ver, ov772x_read(client, MIDH),
>                  ov772x_read(client, MIDL));
>
> Thanks
>    j
>
>
>> --
>> 2.7.4
>>

Re: [PATCH 4/6] media: ov772x: add media controller support

[Akinobu Mita]

2018-04-09 17:32 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>
> On Sun, Apr 08, 2018 at 12:48:08AM +0900, Akinobu Mita wrote:
>> Create a source pad and set the media controller type to the sensor.
>>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>  drivers/media/i2c/ov772x.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 4bb81ff..5e91fa1 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -425,6 +425,9 @@ struct ov772x_priv {
>>       unsigned short                    band_filter;
>>       unsigned int                      fps;
>>       int (*reg_read)(struct i2c_client *client, u8 addr);
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +     struct media_pad pad;
>> +#endif
>>  };
>>
>>  /*
>> @@ -1328,9 +1331,17 @@ static int ov772x_probe(struct i2c_client *client,
>>               goto error_clk_put;
>>       }
>>
>> -     ret = ov772x_video_probe(priv);
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +     priv->pad.flags = MEDIA_PAD_FL_SOURCE;
>> +     priv->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>> +     ret = media_entity_pads_init(&priv->subdev.entity, 1, &priv->pad);
>>       if (ret < 0)
>>               goto error_gpio_put;
>> +#endif
>> +
>> +     ret = ov772x_video_probe(priv);
>> +     if (ret < 0)
>> +             goto error_entity_cleanup;
>
> If you remove the #ifdef around the media_entity_cleanup() below, I
> suggest moving video_probe() before the entity intialization so you
> don't have to #ifdef around the error_gpio_put: label, which otherwise
> the compiler complains for being defined but not used.

I see.

Re: [PATCH 5/6] media: ov772x: add device tree binding

[Akinobu Mita]

2018-04-09 18:06 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>
> On Sun, Apr 08, 2018 at 12:48:09AM +0900, Akinobu Mita wrote:
>> This adds a device tree binding documentation for OV7720/OV7725 sensor.
>
> Please use as patch subject
> media: dt-bindings:

OK.

>>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 36 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  1 +
>>  2 files changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> new file mode 100644
>> index 0000000..9b0df3b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> @@ -0,0 +1,36 @@
>> +* Omnivision OV7720/OV7725 CMOS sensor
>> +
>
> Could you please provide a brief description of the sensor (supported
> resolution and formats is ok)

OK.

>> +Required Properties:
>> +- compatible: shall be one of
>> +     "ovti,ov7720"
>> +     "ovti,ov7725"
>> +- clocks: reference to the xclk input clock.
>> +- clock-names: shall be "xclk".
>> +
>> +Optional Properties:
>> +- rstb-gpios: reference to the GPIO connected to the RSTB pin, if any.
>> +- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any.
>
> As a general note:
> This is debated, and I'm not enforcing it, but please consider using
> generic names for GPIOs with common functions. In this case
> "reset-gpios" and "powerdown-gpios". Also please indicate the GPIO
> active level in bindings description.
>
> For this specific driver:
> The probe routine already looks for a GPIO named 'pwdn', so I guess
> the DT bindings should use the same name. Unless you're willing to
> change it in the board files that register it (Migo-R only in mainline) and
> use the generic 'powerdown' name for both. Either is fine with me.

I'll prepare anothre patch that renames the GPIO names to generic one in
this driver and Mingo-R board file.

> There is no support for the reset GPIO in the driver code, it
> supports soft reset only. Either ditch it from bindings or add support
> for it in driver's code.

Doesn't that reset GPIO exist in current ov772x driver code, does it?

Re: [PATCH 6/6] media: ov772x: support device tree probing

[Akinobu Mita]

2018-04-09 18:27 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>
> On Sun, Apr 08, 2018 at 12:48:10AM +0900, Akinobu Mita wrote:
>> The ov772x driver currently only supports legacy platform data probe.
>> This change enables device tree probing.
>>
>> Note that the platform data probe can select auto or manual edge control
>> mode, but the device tree probling can only select auto edge control mode
>> for now.
>>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>  drivers/media/i2c/ov772x.c | 60 ++++++++++++++++++++++++++++++----------------
>>  1 file changed, 40 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 5e91fa1..e67ec37 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -763,13 +763,13 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>>       case V4L2_CID_VFLIP:
>>               val = ctrl->val ? VFLIP_IMG : 0x00;
>>               priv->flag_vflip = ctrl->val;
>> -             if (priv->info->flags & OV772X_FLAG_VFLIP)
>> +             if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
>>                       val ^= VFLIP_IMG;
>>               return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
>>       case V4L2_CID_HFLIP:
>>               val = ctrl->val ? HFLIP_IMG : 0x00;
>>               priv->flag_hflip = ctrl->val;
>> -             if (priv->info->flags & OV772X_FLAG_HFLIP)
>> +             if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
>>                       val ^= HFLIP_IMG;
>>               return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
>>       case V4L2_CID_BAND_STOP_FILTER:
>> @@ -928,19 +928,14 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
>>       *win = ov772x_select_win(mf->width, mf->height);
>>  }
>>
>> -static int ov772x_set_params(struct ov772x_priv *priv,
>> -                          const struct ov772x_color_format *cfmt,
>> -                          const struct ov772x_win_size *win)
>> +static int ov772x_edgectrl(struct ov772x_priv *priv)
>>  {
>>       struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
>> -     struct v4l2_fract tpf;
>>       int ret;
>> -     u8  val;
>>
>> -     /* Reset hardware. */
>> -     ov772x_reset(client);
>> +     if (!priv->info)
>> +             return 0;
>>
>> -     /* Edge Ctrl. */
>>       if (priv->info->edgectrl.strength & OV772X_MANUAL_EDGE_CTRL) {
>>               /*
>>                * Manual Edge Control Mode.
>> @@ -951,19 +946,19 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>>
>>               ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
>>               if (ret < 0)
>> -                     goto ov772x_set_fmt_error;
>> +                     return ret;
>>
>>               ret = ov772x_mask_set(client,
>>                                     EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
>>                                     priv->info->edgectrl.threshold);
>>               if (ret < 0)
>> -                     goto ov772x_set_fmt_error;
>> +                     return ret;
>>
>>               ret = ov772x_mask_set(client,
>>                                     EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
>>                                     priv->info->edgectrl.strength);
>>               if (ret < 0)
>> -                     goto ov772x_set_fmt_error;
>> +                     return ret;
>>
>>       } else if (priv->info->edgectrl.upper > priv->info->edgectrl.lower) {
>>               /*
>> @@ -975,15 +970,35 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>>                                     EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
>>                                     priv->info->edgectrl.upper);
>>               if (ret < 0)
>> -                     goto ov772x_set_fmt_error;
>> +                     return ret;
>>
>>               ret = ov772x_mask_set(client,
>>                                     EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
>>                                     priv->info->edgectrl.lower);
>>               if (ret < 0)
>> -                     goto ov772x_set_fmt_error;
>> +                     return ret;
>>       }
>>
>> +     return 0;
>> +}
>> +
>> +static int ov772x_set_params(struct ov772x_priv *priv,
>> +                          const struct ov772x_color_format *cfmt,
>> +                          const struct ov772x_win_size *win)
>> +{
>> +     struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
>> +     struct v4l2_fract tpf;
>> +     int ret;
>> +     u8  val;
>> +
>> +     /* Reset hardware. */
>> +     ov772x_reset(client);
>> +
>> +     /* Edge Ctrl. */
>> +     ret =  ov772x_edgectrl(priv);
>> +     if (ret < 0)
>> +             goto ov772x_set_fmt_error;
>> +
>>       /* Format and window size. */
>>       ret = ov772x_write(client, HSTART, win->rect.left >> 2);
>>       if (ret < 0)
>> @@ -1284,11 +1299,6 @@ static int ov772x_probe(struct i2c_client *client,
>>       struct i2c_adapter      *adapter = client->adapter;
>>       int                     ret;
>>
>> -     if (!client->dev.platform_data) {
>
> Not sure this has to be removed completely. I'm debated, as in
> general, for mainline code, we should make sure every user of this
> driver shall provide platform_data during review, and this check is
> not requried. But in general, I still think it may have value.
>
> What about:
>         if (!client->dev.of_node && !client->dev.platform_data) {
>                dev_err(&client->dev, "Missing ov772x platform data\n");
>                return -EINVAL;
>         }

OK. I'll take this.

Re: [PATCH 1/6] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Akinobu Mita]

2018-04-09 15:58 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hello Akinobu,
>     thank you for the patch.
>
> On which platform have you tested the series (just curious) ?

I use Zynq-7000 development board with Xilinx Video IP driver and
custom video pipeline design based on the example reference project.

> On Sun, Apr 08, 2018 at 12:48:05AM +0900, Akinobu Mita wrote:
>> The ov772x driver only works when the i2c controller have
>> I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
>> support it.
>>
>> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
>> it doesn't support repeated starts.
>>
>> This change adds an alternative method for reading from ov772x register
>> which uses two separated i2c messages for the i2c controllers without
>> I2C_FUNC_PROTOCOL_MANGLING.
>
> Actually, and please correct me if I'm wrong, what I see here is that
> an i2c_master_send+i2c_master_recv sequence is equivalent to a mangled
> smbus transaction:
>
> i2c_smbus_read_byte_data | I2C_CLIENT_SCCB:
> S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P
>
> i2c_master_send() + i2c_master_recv():
> S Addr Wr [A] Data [A] P
> S Addr Rd [A] [Data] NA P
>
> I wonder if it is not worth to ditch the existing read() function in
> favour of your new proposed one completely.
>
> I have tested it on the Migo-R board where I have an ov772x installed
> and it works fine.

I'll replace the read() function to the new implementation in the next
version of this patchset. Although handling in the I2C core is fascinating,
I feel the area of influence is a bit large.

> Although I would like to have a confirmation this is fine by people
> how has seen more i2c adapters in action than me :)
>
> Thanks
>    j
>
>>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>  drivers/media/i2c/ov772x.c | 42 +++++++++++++++++++++++++++++++++---------
>>  1 file changed, 33 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index b62860c..283ae2c 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -424,6 +424,7 @@ struct ov772x_priv {
>>       /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>>       unsigned short                    band_filter;
>>       unsigned int                      fps;
>> +     int (*reg_read)(struct i2c_client *client, u8 addr);
>>  };
>>
>>  /*
>> @@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
>>       return container_of(sd, struct ov772x_priv, subdev);
>>  }
>>
>> -static inline int ov772x_read(struct i2c_client *client, u8 addr)
>> +static int ov772x_read(struct i2c_client *client, u8 addr)
>> +{
>> +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +     struct ov772x_priv *priv = to_ov772x(sd);
>> +
>> +     return priv->reg_read(client, addr);
>> +}
>> +
>> +static int ov772x_reg_read(struct i2c_client *client, u8 addr)
>>  {
>>       return i2c_smbus_read_byte_data(client, addr);
>>  }
>>
>> +static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr)
>> +{
>> +     int ret;
>> +     u8 val;
>> +
>> +     ret = i2c_master_send(client, &addr, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +     ret = i2c_master_recv(client, &val, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return val;
>> +}
>> +
>>  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
>>  {
>>       return i2c_smbus_write_byte_data(client, addr, value);
>> @@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client,
>>               return -EINVAL;
>>       }
>>
>> -     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>> -                                           I2C_FUNC_PROTOCOL_MANGLING)) {
>> -             dev_err(&adapter->dev,
>> -                     "I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
>> -             return -EIO;
>> -     }
>> -     client->flags |= I2C_CLIENT_SCCB;
>> -
>>       priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>       if (!priv)
>>               return -ENOMEM;
>>
>>       priv->info = client->dev.platform_data;
>>
>> +     if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>> +                                          I2C_FUNC_PROTOCOL_MANGLING))
>> +             priv->reg_read = ov772x_reg_read;
>> +     else
>> +             priv->reg_read = ov772x_reg_read_fallback;
>> +
>> +     client->flags |= I2C_CLIENT_SCCB;
>> +
>>       v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>>       v4l2_ctrl_handler_init(&priv->hdl, 3);
>>       v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>> --
>> 2.7.4
>>

Re: [PATCH 1/6] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[jacopo mondi]

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

Hi Akinobu,

On Wed, Apr 11, 2018 at 01:37:03AM +0900, Akinobu Mita wrote:
> 2018-04-09 15:58 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> > Hello Akinobu,
> >     thank you for the patch.
> >
> > On which platform have you tested the series (just curious) ?
>
> I use Zynq-7000 development board with Xilinx Video IP driver and
> custom video pipeline design based on the example reference project.
>

That's interesting! I was just wondering if there were other
development boards around that ship with this sensor...

> > On Sun, Apr 08, 2018 at 12:48:05AM +0900, Akinobu Mita wrote:
> >> The ov772x driver only works when the i2c controller have
> >> I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> >> support it.
> >>
> >> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> >> it doesn't support repeated starts.
> >>
> >> This change adds an alternative method for reading from ov772x register
> >> which uses two separated i2c messages for the i2c controllers without
> >> I2C_FUNC_PROTOCOL_MANGLING.
> >
> > Actually, and please correct me if I'm wrong, what I see here is that
> > an i2c_master_send+i2c_master_recv sequence is equivalent to a mangled
> > smbus transaction:
> >
> > i2c_smbus_read_byte_data | I2C_CLIENT_SCCB:
> > S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P
> >
> > i2c_master_send() + i2c_master_recv():
> > S Addr Wr [A] Data [A] P
> > S Addr Rd [A] [Data] NA P
> >
> > I wonder if it is not worth to ditch the existing read() function in
> > favour of your new proposed one completely.
> >
> > I have tested it on the Migo-R board where I have an ov772x installed
> > and it works fine.
>
> I'll replace the read() function to the new implementation in the next
> version of this patchset. Although handling in the I2C core is fascinating,
> I feel the area of influence is a bit large.
>

Yep, i2c is huge and complex but fascinating, and the SCCB use case
should probably be handled with a master_send+master_read instead of
relying on the i2c adapter ability to send an additional stop bit
in between an smbus transaction, as Laurent suggested. I wonder why it
has not been implemented this way from day 1. I'm sure there are
reasons :)

I'll wait for v2.
Thanks
   j

> > Although I would like to have a confirmation this is fine by people
> > how has seen more i2c adapters in action than me :)
> >
> > Thanks
> >    j
> >
> >>
> >> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> ---
> >>  drivers/media/i2c/ov772x.c | 42 +++++++++++++++++++++++++++++++++---------
> >>  1 file changed, 33 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> >> index b62860c..283ae2c 100644
> >> --- a/drivers/media/i2c/ov772x.c
> >> +++ b/drivers/media/i2c/ov772x.c
> >> @@ -424,6 +424,7 @@ struct ov772x_priv {
> >>       /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
> >>       unsigned short                    band_filter;
> >>       unsigned int                      fps;
> >> +     int (*reg_read)(struct i2c_client *client, u8 addr);
> >>  };
> >>
> >>  /*
> >> @@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
> >>       return container_of(sd, struct ov772x_priv, subdev);
> >>  }
> >>
> >> -static inline int ov772x_read(struct i2c_client *client, u8 addr)
> >> +static int ov772x_read(struct i2c_client *client, u8 addr)
> >> +{
> >> +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> +     struct ov772x_priv *priv = to_ov772x(sd);
> >> +
> >> +     return priv->reg_read(client, addr);
> >> +}
> >> +
> >> +static int ov772x_reg_read(struct i2c_client *client, u8 addr)
> >>  {
> >>       return i2c_smbus_read_byte_data(client, addr);
> >>  }
> >>
> >> +static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr)
> >> +{
> >> +     int ret;
> >> +     u8 val;
> >> +
> >> +     ret = i2c_master_send(client, &addr, 1);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +     ret = i2c_master_recv(client, &val, 1);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     return val;
> >> +}
> >> +
> >>  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
> >>  {
> >>       return i2c_smbus_write_byte_data(client, addr, value);
> >> @@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client,
> >>               return -EINVAL;
> >>       }
> >>
> >> -     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> >> -                                           I2C_FUNC_PROTOCOL_MANGLING)) {
> >> -             dev_err(&adapter->dev,
> >> -                     "I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
> >> -             return -EIO;
> >> -     }
> >> -     client->flags |= I2C_CLIENT_SCCB;
> >> -
> >>       priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> >>       if (!priv)
> >>               return -ENOMEM;
> >>
> >>       priv->info = client->dev.platform_data;
> >>
> >> +     if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> >> +                                          I2C_FUNC_PROTOCOL_MANGLING))
> >> +             priv->reg_read = ov772x_reg_read;
> >> +     else
> >> +             priv->reg_read = ov772x_reg_read_fallback;
> >> +
> >> +     client->flags |= I2C_CLIENT_SCCB;
> >> +
> >>       v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
> >>       v4l2_ctrl_handler_init(&priv->hdl, 3);
> >>       v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
> >> --
> >> 2.7.4
> >>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]