[PATCH v3 00/11] media: ov772x: support media controller, device tree probing, etc.

[PATCH v3 00/11] media: ov772x: support media controller, device tree probing, etc.

[Akinobu Mita]

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

* v3 (thanks to Sakari Ailus and Jacopo Mondi)
- Reorder the patches
- Add Reviewed-by: lines
- Remove I2C_CLIENT_SCCB flag set as it isn't needed anymore
- Fix typo in the commit log
- Return without resetting if ov772x_edgectrl() failed
- Update the error message for missing platform data
- Rename mutex name from power_lock to lock
- Add warning for duplicated s_power call
- Add newlines before labels
- Remove __v4l2_ctrl_handler_setup in s_power() as it causes duplicated
  register settings
- Make set_fmt() return -EBUSY while streaming (New)

* v2 (thanks to Jacopo Mondi)
- Replace the implementation of ov772x_read() instead of adding an
  alternative method
- Assign the ov772x_read() return value to pid and ver directly
- Do the same for MIDH and MIDL
- Move video_probe() before the entity initialization and remove the #ifdef
  around the media_entity_cleanup()
- Use generic names for reset and powerdown gpios (New)
- Add "dt-bindings:" in the subject
- Add a brief description of the sensor
- Update the GPIO names
- Indicate the GPIO active level
- Add missing NULL checks for priv->info
- Leave the check for the missing platform data if legacy platform data
  probe is used.
- Handle nested s_power() calls (New)
- Reconstruct s_frame_interval() (New)
- Avoid accessing registers

Akinobu Mita (11):
  media: dt-bindings: ov772x: add device tree binding
  media: ov772x: allow i2c controllers without
    I2C_FUNC_PROTOCOL_MANGLING
  media: ov772x: add checks for register read errors
  media: ov772x: add media controller support
  media: ov772x: use generic names for reset and powerdown gpios
  media: ov772x: support device tree probing
  media: ov772x: handle nested s_power() calls
  media: ov772x: reconstruct s_frame_interval()
  media: ov772x: avoid accessing registers under power saving mode
  media: ov772x: make set_fmt() return -EBUSY while streaming
  media: ov772x: create subdevice device node

 .../devicetree/bindings/media/i2c/ov772x.txt       |  42 +++
 MAINTAINERS                                        |   1 +
 arch/sh/boards/mach-migor/setup.c                  |   5 +-
 drivers/media/i2c/ov772x.c                         | 313 ++++++++++++++++-----
 4 files changed, 284 insertions(+), 77 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 v3 01/11] media: dt-bindings: 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>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Add Reviewed-by: lines

 .../devicetree/bindings/media/i2c/ov772x.txt       | 42 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 43 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..b045503
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
@@ -0,0 +1,42 @@
+* Omnivision OV7720/OV7725 CMOS sensor
+
+The Omnivision OV7720/OV7725 sensor supports multiple resolutions output,
+such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also can
+support the YUV422, RGB565/555/444, GRB422 or raw RGB output formats.
+
+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:
+- reset-gpios: reference to the GPIO connected to the RSTB pin which is
+  active low, if any.
+- powerdown-gpios: reference to the GPIO connected to the PWDN pin which is
+  active high, 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>;
+		reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
+		powerdown-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 5ae51d0..1cc5fb1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10353,6 +10353,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 v3 02/11] 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 changes the reading ov772x register method so that it doesn't
require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.

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>
---
* v3
- Remove I2C_CLIENT_SCCB flag set as it isn't needed anymore

 drivers/media/i2c/ov772x.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..2ae730f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -542,9 +542,19 @@ 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)
 {
-	return i2c_smbus_read_byte_data(client, 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)
@@ -1255,13 +1265,11 @@ static int ov772x_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
-					      I2C_FUNC_PROTOCOL_MANGLING)) {
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
 		dev_err(&adapter->dev,
-			"I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
+			"I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
 		return -EIO;
 	}
-	client->flags |= I2C_CLIENT_SCCB;
 
 	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
-- 
2.7.4

[PATCH v3 03/11] 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Add a Reviewed-by: line

 drivers/media/i2c/ov772x.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2ae730f..f25150d 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1146,7 +1146,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 static int ov772x_video_probe(struct ov772x_priv *priv)
 {
 	struct i2c_client  *client = v4l2_get_subdevdata(&priv->subdev);
-	u8                  pid, ver;
+	int		    pid, ver, midh, midl;
 	const char         *devname;
 	int		    ret;
 
@@ -1156,7 +1156,11 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 
 	/* Check and show product ID and manufacturer ID. */
 	pid = ov772x_read(client, PID);
+	if (pid < 0)
+		return pid;
 	ver = ov772x_read(client, VER);
+	if (ver < 0)
+		return ver;
 
 	switch (VERSION(pid, ver)) {
 	case OV7720:
@@ -1172,13 +1176,17 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 		goto done;
 	}
 
+	midh = ov772x_read(client, MIDH);
+	if (midh < 0)
+		return midh;
+	midl = ov772x_read(client, MIDL);
+	if (midl < 0)
+		return midl;
+
 	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));
+		 devname, pid, ver, midh, midl);
+
 	ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
 done:
-- 
2.7.4

[PATCH v3 04/11] 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Add a Reviewed-by: line

 drivers/media/i2c/ov772x.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index f25150d..18a5ed6 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,6 +424,9 @@ struct ov772x_priv {
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
 	unsigned short                    band_filter;
 	unsigned int			  fps;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_pad pad;
+#endif
 };
 
 /*
@@ -1316,16 +1319,26 @@ static int ov772x_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto error_gpio_put;
 
+#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
+
 	priv->cfmt = &ov772x_cfmts[0];
 	priv->win = &ov772x_win_sizes[0];
 	priv->fps = 15;
 
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret)
-		goto error_gpio_put;
+		goto error_entity_cleanup;
 
 	return 0;
 
+error_entity_cleanup:
+	media_entity_cleanup(&priv->subdev.entity);
 error_gpio_put:
 	if (priv->pwdn_gpio)
 		gpiod_put(priv->pwdn_gpio);
@@ -1341,6 +1354,7 @@ static int ov772x_remove(struct i2c_client *client)
 {
 	struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
 
+	media_entity_cleanup(&priv->subdev.entity);
 	clk_put(priv->clk);
 	if (priv->pwdn_gpio)
 		gpiod_put(priv->pwdn_gpio);
-- 
2.7.4

[PATCH v3 05/11] media: ov772x: use generic names for reset and powerdown gpios

[Akinobu Mita]

The ov772x driver uses "rstb-gpios" and "pwdn-gpios" for reset and
powerdown pins.  However, using generic names for these gpios is
preferred.  ("reset-gpios" and "powerdown-gpios" respectively)

There is only one mainline user for these gpios, so rename to generic
names.

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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Fix typo in the commit log
- Add a Reviewed-by: line

 arch/sh/boards/mach-migor/setup.c | 5 +++--
 drivers/media/i2c/ov772x.c        | 8 ++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
index 271dfc2..73b9ee4 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -351,8 +351,9 @@ static struct platform_device migor_ceu_device = {
 static struct gpiod_lookup_table ov7725_gpios = {
 	.dev_id		= "0-0021",
 	.table		= {
-		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "pwdn", GPIO_ACTIVE_HIGH),
-		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "powerdown",
+			    GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "reset", GPIO_ACTIVE_LOW),
 	},
 };
 
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 18a5ed6..8cac206 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -837,10 +837,10 @@ static int ov772x_power_on(struct ov772x_priv *priv)
 	 * available to handle this cleanly, request the GPIO temporarily
 	 * to avoid conflicts.
 	 */
-	priv->rstb_gpio = gpiod_get_optional(&client->dev, "rstb",
+	priv->rstb_gpio = gpiod_get_optional(&client->dev, "reset",
 					     GPIOD_OUT_LOW);
 	if (IS_ERR(priv->rstb_gpio)) {
-		dev_info(&client->dev, "Unable to get GPIO \"rstb\"");
+		dev_info(&client->dev, "Unable to get GPIO \"reset\"");
 		return PTR_ERR(priv->rstb_gpio);
 	}
 
@@ -1307,10 +1307,10 @@ static int ov772x_probe(struct i2c_client *client,
 		goto error_ctrl_free;
 	}
 
-	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "pwdn",
+	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "powerdown",
 					     GPIOD_OUT_LOW);
 	if (IS_ERR(priv->pwdn_gpio)) {
-		dev_info(&client->dev, "Unable to get GPIO \"pwdn\"");
+		dev_info(&client->dev, "Unable to get GPIO \"powerdown\"");
 		ret = PTR_ERR(priv->pwdn_gpio);
 		goto error_clk_put;
 	}
-- 
2.7.4

[PATCH v3 06/11] 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Add a Reviewed-by: line
- Return without resetting if ov772x_edgectrl() failed
- Update the error message for missing platform data

 drivers/media/i2c/ov772x.c | 64 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 8cac206..95c1c95d 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -749,13 +749,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:
@@ -914,19 +914,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.
@@ -937,19 +932,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) {
 		/*
@@ -961,15 +956,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)
+		return ret;
+
 	/* Format and window size. */
 	ret = ov772x_write(client, HSTART, win->rect.left >> 2);
 	if (ret < 0)
@@ -1020,9 +1035,9 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
 	/* Set COM3. */
 	val = cfmt->com3;
-	if (priv->info->flags & OV772X_FLAG_VFLIP)
+	if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
 		val |= VFLIP_IMG;
-	if (priv->info->flags & OV772X_FLAG_HFLIP)
+	if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
 		val |= HFLIP_IMG;
 	if (priv->flag_vflip)
 		val ^= VFLIP_IMG;
@@ -1271,8 +1286,9 @@ 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");
+	if (!client->dev.of_node && !client->dev.platform_data) {
+		dev_err(&client->dev,
+			"Missing ov772x platform data for non-DT device\n");
 		return -EINVAL;
 	}
 
@@ -1370,9 +1386,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

[PATCH v3 07/11] media: ov772x: handle nested s_power() calls

[Akinobu Mita]

Depending on the v4l2 driver, calling s_power() could be nested.  So the
actual transitions between power saving mode and normal operation mode
should only happen at the first power on and the last power off.

This adds an s_power() nesting counter and updates the power state if the
counter is modified from 0 to != 0 or from != 0 to 0.

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>
---
* v3
- Rename mutex name from power_lock to lock
- Add warning for duplicated s_power call

 drivers/media/i2c/ov772x.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 95c1c95d..8c0b850 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,6 +424,9 @@ struct ov772x_priv {
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
 	unsigned short                    band_filter;
 	unsigned int			  fps;
+	/* lock to protect power_count */
+	struct mutex			  lock;
+	int				  power_count;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pad;
 #endif
@@ -871,9 +874,26 @@ static int ov772x_power_off(struct ov772x_priv *priv)
 static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct ov772x_priv *priv = to_ov772x(sd);
+	int ret = 0;
+
+	mutex_lock(&priv->lock);
+
+	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (priv->power_count == !on)
+		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
+
+	if (!ret) {
+		/* Update the power count. */
+		priv->power_count += on ? 1 : -1;
+		WARN(priv->power_count < 0, "Unbalanced power count\n");
+		WARN(priv->power_count > 1, "Duplicated s_power call\n");
+	}
+
+	mutex_unlock(&priv->lock);
 
-	return on ? ov772x_power_on(priv) :
-		    ov772x_power_off(priv);
+	return ret;
 }
 
 static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
@@ -1303,6 +1323,7 @@ static int ov772x_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	priv->info = client->dev.platform_data;
+	mutex_init(&priv->lock);
 
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
 	v4l2_ctrl_handler_init(&priv->hdl, 3);
@@ -1313,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 			  V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
 	priv->subdev.ctrl_handler = &priv->hdl;
-	if (priv->hdl.error)
-		return priv->hdl.error;
+	if (priv->hdl.error) {
+		ret = priv->hdl.error;
+		goto error_mutex_destroy;
+	}
 
 	priv->clk = clk_get(&client->dev, "xclk");
 	if (IS_ERR(priv->clk)) {
@@ -1362,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
 	clk_put(priv->clk);
 error_ctrl_free:
 	v4l2_ctrl_handler_free(&priv->hdl);
+error_mutex_destroy:
+	mutex_destroy(&priv->lock);
 
 	return ret;
 }
@@ -1376,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
 		gpiod_put(priv->pwdn_gpio);
 	v4l2_async_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
+	mutex_destroy(&priv->lock);
 
 	return 0;
 }
-- 
2.7.4

[PATCH v3 08/11] media: ov772x: reconstruct s_frame_interval()

[Akinobu Mita]

This splits the s_frame_interval() in subdev video ops into selecting the
frame interval and setting up the registers.

This is a preparatory change to avoid accessing registers under power
saving mode.

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>
---
* v3
- No changes

 drivers/media/i2c/ov772x.c | 56 +++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 8c0b850..452406c 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -617,25 +617,16 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
-static int ov772x_set_frame_rate(struct ov772x_priv *priv,
-				 struct v4l2_fract *tpf,
-				 const struct ov772x_color_format *cfmt,
-				 const struct ov772x_win_size *win)
+static unsigned int ov772x_select_fps(struct ov772x_priv *priv,
+				 struct v4l2_fract *tpf)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
-	unsigned long fin = clk_get_rate(priv->clk);
 	unsigned int fps = tpf->numerator ?
 			   tpf->denominator / tpf->numerator :
 			   tpf->denominator;
 	unsigned int best_diff;
-	unsigned int fsize;
-	unsigned int pclk;
 	unsigned int diff;
 	unsigned int idx;
 	unsigned int i;
-	u8 clkrc = 0;
-	u8 com4 = 0;
-	int ret;
 
 	/* Approximate to the closest supported frame interval. */
 	best_diff = ~0L;
@@ -646,7 +637,25 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
 			best_diff = diff;
 		}
 	}
-	fps = ov772x_frame_intervals[idx];
+
+	return ov772x_frame_intervals[idx];
+}
+
+static int ov772x_set_frame_rate(struct ov772x_priv *priv,
+				 unsigned int fps,
+				 const struct ov772x_color_format *cfmt,
+				 const struct ov772x_win_size *win)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
+	unsigned long fin = clk_get_rate(priv->clk);
+	unsigned int fsize;
+	unsigned int pclk;
+	unsigned int best_diff;
+	unsigned int diff;
+	unsigned int i;
+	u8 clkrc = 0;
+	u8 com4 = 0;
+	int ret;
 
 	/* Use image size (with blankings) to calculate desired pixel clock. */
 	switch (cfmt->com7 & OFMT_MASK) {
@@ -711,10 +720,6 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
 	if (ret < 0)
 		return ret;
 
-	tpf->numerator = 1;
-	tpf->denominator = fps;
-	priv->fps = tpf->denominator;
-
 	return 0;
 }
 
@@ -735,8 +740,20 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
 {
 	struct ov772x_priv *priv = to_ov772x(sd);
 	struct v4l2_fract *tpf = &ival->interval;
+	unsigned int fps;
+	int ret;
+
+	fps = ov772x_select_fps(priv, tpf);
+
+	ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
+	if (ret)
+		return ret;
 
-	return ov772x_set_frame_rate(priv, tpf, priv->cfmt, priv->win);
+	tpf->numerator = 1;
+	tpf->denominator = fps;
+	priv->fps = fps;
+
+	return 0;
 }
 
 static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -993,7 +1010,6 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 			     const struct ov772x_win_size *win)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
-	struct v4l2_fract tpf;
 	int ret;
 	u8  val;
 
@@ -1075,9 +1091,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 		goto ov772x_set_fmt_error;
 
 	/* COM4, CLKRC: Set pixel clock and framerate. */
-	tpf.numerator = 1;
-	tpf.denominator = priv->fps;
-	ret = ov772x_set_frame_rate(priv, &tpf, cfmt, win);
+	ret = ov772x_set_frame_rate(priv, priv->fps, cfmt, win);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
 
-- 
2.7.4

[PATCH v3 09/11] media: ov772x: avoid accessing registers under power saving mode

[Akinobu Mita]

The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
and the s_frame_interval() in subdev video ops could be called when the
device is under power saving mode.  These callbacks for ov772x driver
cause updating H/W registers that will fail under power saving mode.

This avoids it by not apply any changes to H/W if the device is not powered
up.  Instead the changes will be restored right after power-up.

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>
---
* v3
- Add newlines before labels
- Remove __v4l2_ctrl_handler_setup in s_power() as it causes duplicated
  register settings

 drivers/media/i2c/ov772x.c | 79 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 452406c..96dd37a 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -741,19 +741,30 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
 	struct ov772x_priv *priv = to_ov772x(sd);
 	struct v4l2_fract *tpf = &ival->interval;
 	unsigned int fps;
-	int ret;
+	int ret = 0;
 
 	fps = ov772x_select_fps(priv, tpf);
 
-	ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
-	if (ret)
-		return ret;
+	mutex_lock(&priv->lock);
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any changes to H/W at this time. Instead
+	 * the frame rate will be restored right after power-up.
+	 */
+	if (priv->power_count > 0) {
+		ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
+		if (ret)
+			goto error;
+	}
 
 	tpf->numerator = 1;
 	tpf->denominator = fps;
 	priv->fps = fps;
 
-	return 0;
+error:
+	mutex_unlock(&priv->lock);
+
+	return ret;
 }
 
 static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -765,6 +776,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 	int ret = 0;
 	u8 val;
 
+	/* v4l2_ctrl_lock() locks our own mutex */
+
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any controls to H/W at this time. Instead
+	 * the controls will be restored right after power-up.
+	 */
+	if (priv->power_count == 0)
+		return 0;
+
 	switch (ctrl->id) {
 	case V4L2_CID_VFLIP:
 		val = ctrl->val ? VFLIP_IMG : 0x00;
@@ -888,6 +909,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
 	return 0;
 }
 
+static int ov772x_set_params(struct ov772x_priv *priv,
+			     const struct ov772x_color_format *cfmt,
+			     const struct ov772x_win_size *win);
+
 static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct ov772x_priv *priv = to_ov772x(sd);
@@ -898,8 +923,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
 	 * update the power state.
 	 */
-	if (priv->power_count == !on)
-		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
+	if (priv->power_count == !on) {
+		if (on) {
+			ret = ov772x_power_on(priv);
+			/*
+			 * Restore the format, the frame rate, and
+			 * the controls
+			 */
+			if (!ret)
+				ret = ov772x_set_params(priv, priv->cfmt,
+							priv->win);
+		} else {
+			ret = ov772x_power_off(priv);
+		}
+	}
 
 	if (!ret) {
 		/* Update the power count. */
@@ -1164,7 +1201,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *mf = &format->format;
 	const struct ov772x_color_format *cfmt;
 	const struct ov772x_win_size *win;
-	int ret;
+	int ret = 0;
 
 	if (format->pad)
 		return -EINVAL;
@@ -1185,14 +1222,24 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 		return 0;
 	}
 
-	ret = ov772x_set_params(priv, cfmt, win);
-	if (ret < 0)
-		return ret;
-
+	mutex_lock(&priv->lock);
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any changes to H/W at this time. Instead
+	 * the format will be restored right after power-up.
+	 */
+	if (priv->power_count > 0) {
+		ret = ov772x_set_params(priv, cfmt, win);
+		if (ret < 0)
+			goto error;
+	}
 	priv->win = win;
 	priv->cfmt = cfmt;
 
-	return 0;
+error:
+	mutex_unlock(&priv->lock);
+
+	return ret;
 }
 
 static int ov772x_video_probe(struct ov772x_priv *priv)
@@ -1202,7 +1249,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 	const char         *devname;
 	int		    ret;
 
-	ret = ov772x_s_power(&priv->subdev, 1);
+	ret = ov772x_power_on(priv);
 	if (ret < 0)
 		return ret;
 
@@ -1242,7 +1289,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 	ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
 done:
-	ov772x_s_power(&priv->subdev, 0);
+	ov772x_power_off(priv);
 
 	return ret;
 }
@@ -1341,6 +1388,8 @@ static int ov772x_probe(struct i2c_client *client,
 
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
 	v4l2_ctrl_handler_init(&priv->hdl, 3);
+	/* Use our mutex for the controls */
+	priv->hdl.lock = &priv->lock;
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 			  V4L2_CID_VFLIP, 0, 1, 1, 0);
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
-- 
2.7.4

[PATCH v3 10/11] media: ov772x: make set_fmt() return -EBUSY while streaming

[Akinobu Mita]

The ov772x driver is going to offer a V4L2 sub-device interface, so
changing the output data format on this sub-device can be made anytime.
However, the request is preferred to fail while the video stream on the
device is active.

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>
---
* v3
- New patch

 drivers/media/i2c/ov772x.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 96dd37a..c9fdc67 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,9 +424,10 @@ struct ov772x_priv {
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
 	unsigned short                    band_filter;
 	unsigned int			  fps;
-	/* lock to protect power_count */
+	/* lock to protect power_count and streaming */
 	struct mutex			  lock;
 	int				  power_count;
+	int				  streaming;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pad;
 #endif
@@ -603,18 +604,28 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov772x_priv *priv = to_ov772x(sd);
+	int ret = 0;
 
-	if (!enable) {
-		ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE, SOFT_SLEEP_MODE);
-		return 0;
-	}
+	mutex_lock(&priv->lock);
 
-	ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE, 0);
+	if (priv->streaming == enable)
+		goto done;
 
-	dev_dbg(&client->dev, "format %d, win %s\n",
-		priv->cfmt->code, priv->win->name);
+	ret = ov772x_mask_set(client, COM2, SOFT_SLEEP_MODE,
+			      enable ? 0 : SOFT_SLEEP_MODE);
+	if (ret)
+		goto done;
 
-	return 0;
+	if (enable) {
+		dev_dbg(&client->dev, "format %d, win %s\n",
+			priv->cfmt->code, priv->win->name);
+	}
+	priv->streaming = enable;
+
+done:
+	mutex_unlock(&priv->lock);
+
+	return ret;
 }
 
 static unsigned int ov772x_select_fps(struct ov772x_priv *priv,
@@ -1223,6 +1234,12 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	mutex_lock(&priv->lock);
+
+	if (priv->streaming) {
+		ret = -EBUSY;
+		goto error;
+	}
+
 	/*
 	 * If the device is not powered up by the host driver do
 	 * not apply any changes to H/W at this time. Instead
-- 
2.7.4

[PATCH v3 11/11] 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>
---
* v3
- No changes

 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 c9fdc67..a41c9d3 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1404,6 +1404,7 @@ static int ov772x_probe(struct i2c_client *client,
 	mutex_init(&priv->lock);
 
 	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);
 	/* Use our mutex for the controls */
 	priv->hdl.lock = &priv->lock;
-- 
2.7.4

Re: [PATCH v3 07/11] media: ov772x: handle nested s_power() calls

[jacopo mondi]

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

Hi Akinobu,
   thanks for v3

On Mon, Apr 23, 2018 at 12:56:13AM +0900, Akinobu Mita wrote:
> Depending on the v4l2 driver, calling s_power() could be nested.  So the
> actual transitions between power saving mode and normal operation mode
> should only happen at the first power on and the last power off.
>
> This adds an s_power() nesting counter and updates the power state if the
> counter is modified from 0 to != 0 or from != 0 to 0.
>
> 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>
> ---
> * v3
> - Rename mutex name from power_lock to lock
> - Add warning for duplicated s_power call
>
>  drivers/media/i2c/ov772x.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 95c1c95d..8c0b850 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -424,6 +424,9 @@ struct ov772x_priv {
>  	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>  	unsigned short                    band_filter;
>  	unsigned int			  fps;
> +	/* lock to protect power_count */
> +	struct mutex			  lock;
> +	int				  power_count;
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_pad pad;
>  #endif
> @@ -871,9 +874,26 @@ static int ov772x_power_off(struct ov772x_priv *priv)
>  static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct ov772x_priv *priv = to_ov772x(sd);
> +	int ret = 0;
> +
> +	mutex_lock(&priv->lock);
> +
> +	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> +	 * update the power state.
> +	 */
> +	if (priv->power_count == !on)
> +		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
> +
> +	if (!ret) {
> +		/* Update the power count. */
> +		priv->power_count += on ? 1 : -1;
> +		WARN(priv->power_count < 0, "Unbalanced power count\n");
> +		WARN(priv->power_count > 1, "Duplicated s_power call\n");


The first time you receive a power on (on == 1, power_count == 0)
you'll have:

        if (0 == !1)
                ret = ov772x_power_on()
        if (!0)
                power_count += 1;

If power management is 'nested' you could receive a second,
unexpected, power on (on == 1, power_count == 1)
        if (1 == !1)
        if (!0)
                power_count += 1;

Now power_count == 2, if you now receive a power off, you're going to
ignore it:

        if (2 == !0)
                ret = ov772x_power_off();
        if (!0)
                power_count += -1;

Again you now receive a new power off, and you're going to issue that
one.

        if (1 == !0)
                ret = ov772x_power_off();
        if (!0)
                power_count += -1;

So this seems correct to me!

If I were you, I would simplify this as:

        power_count += on ? 1 : -1;
        if (power_count == !!on)
                return on ? ov772x_power_on(priv) :
                            ov772x_power_off(priv);
        WARN_ON(power_count < 0 || power_count > 1);

        return 0;

which, if I'm not wrong expands to:

power_count = 0 ; on = 1
        power_count += 1;
        if (1 == !!1)
                return ov772x_power_on(priv);

power_count = 1 ; on = 1
        power_count += 1;
        if (2 == !!1)
        WARN_ON(2 > 1)

power_count = 2 ; on = 0
        power_count -= 1;
        if (1 == !!0)
        if (1 > 1 || 1 < 0)

power_count = 1; on = 0
        power_count -= 1;
        if (0 == !!0)
                return ov772_power_off(priv);

power_count = 0 ; on = 0
        power_count -= 1;
        if (-1 == !!0)
        WARN_ON(-1 < 0)

But if you have seen that pattern in other drivers, and maybe already
tested your one, I'm fine with either of them.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> +	}
> +
> +	mutex_unlock(&priv->lock);
>
> -	return on ? ov772x_power_on(priv) :
> -		    ov772x_power_off(priv);
> +	return ret;
>  }
>
>  static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
> @@ -1303,6 +1323,7 @@ static int ov772x_probe(struct i2c_client *client,
>  		return -ENOMEM;
>
>  	priv->info = client->dev.platform_data;
> +	mutex_init(&priv->lock);
>
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>  	v4l2_ctrl_handler_init(&priv->hdl, 3);
> @@ -1313,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>  			  V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
>  	priv->subdev.ctrl_handler = &priv->hdl;
> -	if (priv->hdl.error)
> -		return priv->hdl.error;
> +	if (priv->hdl.error) {
> +		ret = priv->hdl.error;
> +		goto error_mutex_destroy;
> +	}
>
>  	priv->clk = clk_get(&client->dev, "xclk");
>  	if (IS_ERR(priv->clk)) {
> @@ -1362,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
>  	clk_put(priv->clk);
>  error_ctrl_free:
>  	v4l2_ctrl_handler_free(&priv->hdl);
> +error_mutex_destroy:
> +	mutex_destroy(&priv->lock);
>
>  	return ret;
>  }
> @@ -1376,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
>  		gpiod_put(priv->pwdn_gpio);
>  	v4l2_async_unregister_subdev(&priv->subdev);
>  	v4l2_ctrl_handler_free(&priv->hdl);
> +	mutex_destroy(&priv->lock);
>
>  	return 0;
>  }
> --
> 2.7.4
>

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

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Laurent Pinchart]

Hi Mita-san,

On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
> 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>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v3
> - Add Reviewed-by: lines
> 
>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 +++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 43 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..b045503
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> @@ -0,0 +1,42 @@
> +* Omnivision OV7720/OV7725 CMOS sensor
> +
> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions output,
> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also can
> +support the YUV422, RGB565/555/444, GRB422 or raw RGB output formats.
> +
> +Required Properties:
> +- compatible: shall be one of
> +	"ovti,ov7720"
> +	"ovti,ov7725"
> +- clocks: reference to the xclk input clock.
> +- clock-names: shall be "xclk".

As there's a single clock we could omit clock-names, couldn't we ?

The rest of the patch looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +Optional Properties:
> +- reset-gpios: reference to the GPIO connected to the RSTB pin which is
> +  active low, if any.
> +- powerdown-gpios: reference to the GPIO connected to the PWDN pin which is
> +  active high, 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>;
> +		reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
> +		powerdown-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 5ae51d0..1cc5fb1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10353,6 +10353,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>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Laurent Pinchart]

Hi Mita-san,

On Sunday, 22 April 2018 18:56:08 EEST 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 changes the reading ov772x register method so that it doesn't
> require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.

As commented in a reply to v1, given that this implementation is in no way 
specific to the ov772x driver, I'd prefer implementing the fallback in the I2C 
core instead.

> 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>
> ---
> * v3
> - Remove I2C_CLIENT_SCCB flag set as it isn't needed anymore
> 
>  drivers/media/i2c/ov772x.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index b62860c..2ae730f 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -542,9 +542,19 @@ 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)
>  {
> -	return i2c_smbus_read_byte_data(client, 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) @@ -1255,13 +1265,11 @@ static int ov772x_probe(struct i2c_client
> *client, return -EINVAL;
>  	}
> 
> -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> -					      I2C_FUNC_PROTOCOL_MANGLING)) {
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
>  		dev_err(&adapter->dev,
> -			"I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING
\n");
> +			"I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
>  		return -EIO;
>  	}
> -	client->flags |= I2C_CLIENT_SCCB;
> 
>  	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)


-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Akinobu Mita]

2018-04-23 18:17 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Mita-san,
>
> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
>> 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>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>> * v3
>> - Add Reviewed-by: lines
>>
>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 +++++++++++++++++++
>>  MAINTAINERS                                        |  1 +
>>  2 files changed, 43 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..b045503
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> @@ -0,0 +1,42 @@
>> +* Omnivision OV7720/OV7725 CMOS sensor
>> +
>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions output,
>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also can
>> +support the YUV422, RGB565/555/444, GRB422 or raw RGB output formats.
>> +
>> +Required Properties:
>> +- compatible: shall be one of
>> +     "ovti,ov7720"
>> +     "ovti,ov7725"
>> +- clocks: reference to the xclk input clock.
>> +- clock-names: shall be "xclk".
>
> As there's a single clock we could omit clock-names, couldn't we ?

Sounds good.

I'll prepare another patch that replaces the clock consumer ID argument
of clk_get() from "xclk" to NULL, and remove the above line in this
bindings.

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Akinobu Mita]

2018-04-23 18:18 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Mita-san,
>
> On Sunday, 22 April 2018 18:56:08 EEST 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 changes the reading ov772x register method so that it doesn't
>> require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
>
> As commented in a reply to v1, given that this implementation is in no way
> specific to the ov772x driver, I'd prefer implementing the fallback in the I2C
> core instead.

Do you have any ideas how to implement in the I2C core?
I wonder if I can modify i2c_smbus_read_byte_data() or i2c_transfer()
for I2C_CLIENT_SCCB.

>> 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>
>> ---
>> * v3
>> - Remove I2C_CLIENT_SCCB flag set as it isn't needed anymore
>>
>>  drivers/media/i2c/ov772x.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index b62860c..2ae730f 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -542,9 +542,19 @@ 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)
>>  {
>> -     return i2c_smbus_read_byte_data(client, 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) @@ -1255,13 +1265,11 @@ static int ov772x_probe(struct i2c_client
>> *client, return -EINVAL;
>>       }
>>
>> -     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>> -                                           I2C_FUNC_PROTOCOL_MANGLING)) {
>> +     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
>>               dev_err(&adapter->dev,
>> -                     "I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING
> \n");
>> +                     "I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
>>               return -EIO;
>>       }
>> -     client->flags |= I2C_CLIENT_SCCB;
>>
>>       priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>       if (!priv)
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Laurent Pinchart]

Hi Mita-san,

(CC'ing Wolfram Sang)

On Monday, 23 April 2018 18:55:20 EEST Akinobu Mita wrote:
> 2018-04-23 18:18 GMT+09:00 Laurent Pinchart:
> > On Sunday, 22 April 2018 18:56:08 EEST 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 changes the reading ov772x register method so that it doesn't
> >> require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
> > 
> > As commented in a reply to v1, given that this implementation is in no way
> > specific to the ov772x driver, I'd prefer implementing the fallback in the
> > I2C core instead.
> 
> Do you have any ideas how to implement in the I2C core?
> I wonder if I can modify i2c_smbus_read_byte_data() or i2c_transfer()
> for I2C_CLIENT_SCCB.

How about i2c_smbus_xfer_emulated() ? The tricky part will be to handle the 
I2C adapters that implement .smbus_xfer(), as those won't go through 
i2c_smbus_xfer_emulated(). i2c_smbus_xfer_emulated() relies on i2c_transfer(), 
which itself relies on the I2C adapter's .master_xfer() operation. We're thus 
only concerned about the drivers that implement both .smbus_xfer() and 
master_xfer(), and there's only 4 of them (i2c-opal, i2c-pasemi, i2c-powermac 
and i2c-zx2967). Maybe the simplest solution would be to force the emulation 
path if I2C_CLIENT_SCCB && !I2C_FUNC_PROTOCOL_MANGLING && ->master_xfer != 
NULL ?

Wolfram, what do you think ?

> >> 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>
> >> ---
> >> * v3
> >> - Remove I2C_CLIENT_SCCB flag set as it isn't needed anymore
> >> 
> >>  drivers/media/i2c/ov772x.c | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> >> index b62860c..2ae730f 100644
> >> --- a/drivers/media/i2c/ov772x.c
> >> +++ b/drivers/media/i2c/ov772x.c
> >> @@ -542,9 +542,19 @@ 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)
> >>  {
> >> -     return i2c_smbus_read_byte_data(client, 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)
> >> @@ -1255,13 +1265,11 @@ static int ov772x_probe(struct i2c_client
> >> *client,
> >>  			return -EINVAL;
> >>       }
> >> 
> >> -     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> >> -                                           I2C_FUNC_PROTOCOL_MANGLING))
> >> {
> >> +     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> >>               dev_err(&adapter->dev,
> >> -                     "I2C-Adapter doesn't support SMBUS_BYTE_DATA or
> >> PROTOCOL_MANGLING\n");
> >> +                     "I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
> >>               return -EIO;
> >>       }
> >> 
> >> -     client->flags |= I2C_CLIENT_SCCB;
> >> 
> >>       priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> >>       if (!priv)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

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


> How about i2c_smbus_xfer_emulated() ? The tricky part will be to handle the 
> I2C adapters that implement .smbus_xfer(), as those won't go through 
> i2c_smbus_xfer_emulated(). i2c_smbus_xfer_emulated() relies on i2c_transfer(), 
> which itself relies on the I2C adapter's .master_xfer() operation. We're thus 
> only concerned about the drivers that implement both .smbus_xfer() and 
> master_xfer(), and there's only 4 of them (i2c-opal, i2c-pasemi, i2c-powermac 
> and i2c-zx2967). Maybe the simplest solution would be to force the emulation 
> path if I2C_CLIENT_SCCB && !I2C_FUNC_PROTOCOL_MANGLING && ->master_xfer != 
> NULL ?
> 
> Wolfram, what do you think ?

I think it is a mess :)

Further: I don't think we will ever see an SMBus controller which allows
mangling. SMBus is way more precisely defined than I2C, so HW can then
do much more things automatically. They will always do a REP_START, so I
don't think you can connect SCCB devices to SMBus.

As a result, we shouldn't do SMBus calls for SCCB. Maybe we should
introduce sccb_byte_read? SCCB didn't specify much more than byte read
IIRC, or? The implementation here with two seperate messages makes much
sense to me then.

I could argue that the sccb_* helpers should live in drivers/media since
it is probably only Omnivision trying to work around I2C licensing here?

But if it is not too heavy, maybe we could take it into i2c as well.

Makes sense or did I miss something?


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

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Laurent Pinchart]

Hi Wolfram,

On Monday, 23 April 2018 23:11:21 EEST Wolfram Sang wrote:
> > How about i2c_smbus_xfer_emulated() ? The tricky part will be to handle
> > the I2C adapters that implement .smbus_xfer(), as those won't go through
> > i2c_smbus_xfer_emulated(). i2c_smbus_xfer_emulated() relies on
> > i2c_transfer(), which itself relies on the I2C adapter's .master_xfer()
> > operation. We're thus only concerned about the drivers that implement
> > both .smbus_xfer() and master_xfer(), and there's only 4 of them
> > (i2c-opal, i2c-pasemi, i2c-powermac and i2c-zx2967). Maybe the simplest
> > solution would be to force the emulation path if I2C_CLIENT_SCCB &&
> > !I2C_FUNC_PROTOCOL_MANGLING && ->master_xfer != NULL ?
> > 
> > Wolfram, what do you think ?
> 
> I think it is a mess :)
> 
> Further: I don't think we will ever see an SMBus controller which allows
> mangling. SMBus is way more precisely defined than I2C, so HW can then
> do much more things automatically. They will always do a REP_START, so I
> don't think you can connect SCCB devices to SMBus.
> 
> As a result, we shouldn't do SMBus calls for SCCB. Maybe we should
> introduce sccb_byte_read? SCCB didn't specify much more than byte read
> IIRC, or? The implementation here with two seperate messages makes much
> sense to me then.
> 
> I could argue that the sccb_* helpers should live in drivers/media since
> it is probably only Omnivision trying to work around I2C licensing here?
> 
> But if it is not too heavy, maybe we could take it into i2c as well.
> 
> Makes sense or did I miss something?

SCCB helpers would work too. It would be easy to just move the functions 
defined in this patch to helpers and be done with it. However, there are I2C 
adapters that have native SCCB support, so to take advantage of that feature 
we need to forward native SCCB calls all the way down the stack in that case. 
That's why I thought an implementation in the I2C subsystem would be better. 
Furthermore, as SCCB is really a slightly mangled version of I2C, I think the 
I2C subsystem would be a natural location for the implementation. It shouldn't 
be too much work, it's just a matter of deciding what the call stacks should 
be for the native vs. emulated cases.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

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


> SCCB helpers would work too. It would be easy to just move the functions 
> defined in this patch to helpers and be done with it. However, there are I2C 
> adapters that have native SCCB support, so to take advantage of that feature 

Ah, didn't notice that so far. Can't find it in drivers/i2c/busses.
Where are those?

> we need to forward native SCCB calls all the way down the stack in that case. 

And how is it done currently?

> That's why I thought an implementation in the I2C subsystem would be better. 
> Furthermore, as SCCB is really a slightly mangled version of I2C, I think the 
> I2C subsystem would be a natural location for the implementation. It shouldn't 

Can be argued. But it can also be argues that it sits on top of I2C and
doesn't need to live in i2c-folders itself (like PMBus). The
implementation given in this patch looks a bit like the latter. However,
this is not the main question currently.

> be too much work, it's just a matter of deciding what the call stacks should 
> be for the native vs. emulated cases.

I don't like it. We shouldn't use SMBus calls for SCCB because SMBus
will very likely never support it. Or do you know of such a case? I
think I really want sccb helpers. So, people immediately see that SCCB
is used and not SMBus or I2C. And there, we can handle native support
vs. I2C-SCCB-emulation. And maybe SMBus-SCCB emulation but I doubt we
will ever need it.


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

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Laurent Pinchart]

Hi Wolfram,

On Monday, 23 April 2018 23:36:16 EEST Wolfram Sang wrote:
> > SCCB helpers would work too. It would be easy to just move the functions
> > defined in this patch to helpers and be done with it. However, there are
> > I2C adapters that have native SCCB support, so to take advantage of that
> > feature
> 
> Ah, didn't notice that so far. Can't find it in drivers/i2c/busses.
> Where are those?

IIRC the OMAP I2C adapter supports SCCB natively. I'm not sure the driver 
implements that though.

> > we need to forward native SCCB calls all the way down the stack in that
> > case.
> 
> And how is it done currently?

Currently we go down to .master_xfer(), and adapters can then decide to use 
the hardware SCCB support. Again, it might not be implemented :-)

> > That's why I thought an implementation in the I2C subsystem would be
> > better. Furthermore, as SCCB is really a slightly mangled version of I2C,
> > I think the I2C subsystem would be a natural location for the
> > implementation. It shouldn't
> 
> Can be argued. But it can also be argues that it sits on top of I2C and
> doesn't need to live in i2c-folders itself (like PMBus). The
> implementation given in this patch looks a bit like the latter. However,
> this is not the main question currently.
> 
> > be too much work, it's just a matter of deciding what the call stacks
> > should be for the native vs. emulated cases.
> 
> I don't like it. We shouldn't use SMBus calls for SCCB because SMBus
> will very likely never support it. Or do you know of such a case? I
> think I really want sccb helpers. So, people immediately see that SCCB
> is used and not SMBus or I2C. And there, we can handle native support
> vs. I2C-SCCB-emulation. And maybe SMBus-SCCB emulation but I doubt we
> will ever need it.

I'm fine with SCCB helpers. Please note, however, that SCCB differs from SMBus 
in two ways: NACKs shall be ignored by the master (even though most SCCB 
devices generate an ack, so we could likely ignore this), and write-read 
sequences shouldn't use a repeated start. Apart from that register reads and 
register writes are identical to SMBus, which prompted the reuse (or abuse) of 
the SMBus API. If we end up implementing SCCB helpers, they will likely look 
very, very similar to the SMBus implementation, including the SMBus emulated 
transfer helper.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Sakari Ailus]

Hi guys,

On Mon, Apr 23, 2018 at 11:51:30PM +0300, Laurent Pinchart wrote:
> Hi Wolfram,
> 
> On Monday, 23 April 2018 23:36:16 EEST Wolfram Sang wrote:
> > > SCCB helpers would work too. It would be easy to just move the functions
> > > defined in this patch to helpers and be done with it. However, there are
> > > I2C adapters that have native SCCB support, so to take advantage of that
> > > feature
> > 
> > Ah, didn't notice that so far. Can't find it in drivers/i2c/busses.
> > Where are those?
> 
> IIRC the OMAP I2C adapter supports SCCB natively. I'm not sure the driver 
> implements that though.
> 
> > > we need to forward native SCCB calls all the way down the stack in that
> > > case.
> > 
> > And how is it done currently?
> 
> Currently we go down to .master_xfer(), and adapters can then decide to use 
> the hardware SCCB support. Again, it might not be implemented :-)
> 
> > > That's why I thought an implementation in the I2C subsystem would be
> > > better. Furthermore, as SCCB is really a slightly mangled version of I2C,
> > > I think the I2C subsystem would be a natural location for the
> > > implementation. It shouldn't
> > 
> > Can be argued. But it can also be argues that it sits on top of I2C and
> > doesn't need to live in i2c-folders itself (like PMBus). The
> > implementation given in this patch looks a bit like the latter. However,
> > this is not the main question currently.
> > 
> > > be too much work, it's just a matter of deciding what the call stacks
> > > should be for the native vs. emulated cases.
> > 
> > I don't like it. We shouldn't use SMBus calls for SCCB because SMBus
> > will very likely never support it. Or do you know of such a case? I
> > think I really want sccb helpers. So, people immediately see that SCCB
> > is used and not SMBus or I2C. And there, we can handle native support
> > vs. I2C-SCCB-emulation. And maybe SMBus-SCCB emulation but I doubt we
> > will ever need it.
> 
> I'm fine with SCCB helpers. Please note, however, that SCCB differs from SMBus 
> in two ways: NACKs shall be ignored by the master (even though most SCCB 
> devices generate an ack, so we could likely ignore this), and write-read 
> sequences shouldn't use a repeated start. Apart from that register reads and 
> register writes are identical to SMBus, which prompted the reuse (or abuse) of 
> the SMBus API. If we end up implementing SCCB helpers, they will likely look 
> very, very similar to the SMBus implementation, including the SMBus emulated 
> transfer helper.

Sounds like that there would be a bit of work to do here but AFAIU the
patchset should be fine to go in (with the other comments addressed) and
this could be addressed separately. Let me know if you have concerns.

Thanks.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Akinobu Mita]

2018-04-24 0:54 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
> 2018-04-23 18:17 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>> Hi Mita-san,
>>
>> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
>>> 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>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>>> ---
>>> * v3
>>> - Add Reviewed-by: lines
>>>
>>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 +++++++++++++++++++
>>>  MAINTAINERS                                        |  1 +
>>>  2 files changed, 43 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..b045503
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>>> @@ -0,0 +1,42 @@
>>> +* Omnivision OV7720/OV7725 CMOS sensor
>>> +
>>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions output,
>>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also can
>>> +support the YUV422, RGB565/555/444, GRB422 or raw RGB output formats.
>>> +
>>> +Required Properties:
>>> +- compatible: shall be one of
>>> +     "ovti,ov7720"
>>> +     "ovti,ov7725"
>>> +- clocks: reference to the xclk input clock.
>>> +- clock-names: shall be "xclk".
>>
>> As there's a single clock we could omit clock-names, couldn't we ?
>
> Sounds good.
>
> I'll prepare another patch that replaces the clock consumer ID argument
> of clk_get() from "xclk" to NULL, and remove the above line in this
> bindings.

I thought it's easy to do.  However, there is a non-DT user
(arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.

This can be resolved by retrying clk_get() with NULL if no entry
with "xclk".  But should we do so or leave as is?

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Laurent Pinchart]

Hi Mita-san,

On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
> 2018-04-24 0:54 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
> >>> 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>
> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >>> ---
> >>> * v3
> >>> - Add Reviewed-by: lines
> >>> 
> >>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 +++++++++++++++
> >>>  MAINTAINERS                                        |  1 +
> >>>  2 files changed, 43 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..b045503
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >>> @@ -0,0 +1,42 @@
> >>> +* Omnivision OV7720/OV7725 CMOS sensor
> >>> +
> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
> >>> output,
> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also
> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
> >>> formats. +
> >>> +Required Properties:
> >>> +- compatible: shall be one of
> >>> +     "ovti,ov7720"
> >>> +     "ovti,ov7725"
> >>> +- clocks: reference to the xclk input clock.
> >>> +- clock-names: shall be "xclk".
> >> 
> >> As there's a single clock we could omit clock-names, couldn't we ?
> > 
> > Sounds good.
> > 
> > I'll prepare another patch that replaces the clock consumer ID argument
> > of clk_get() from "xclk" to NULL, and remove the above line in this
> > bindings.
> 
> I thought it's easy to do.  However, there is a non-DT user
> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
> 
> This can be resolved by retrying clk_get() with NULL if no entry
> with "xclk".  But should we do so or leave as is?

How about patching the board code to register the clock alias with

	clk_add_alias(NULL, "0-0021", "video_clk", NULL);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/11] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Wolfram Sang]

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


> > Ah, didn't notice that so far. Can't find it in drivers/i2c/busses.
> > Where are those?
> 
> IIRC the OMAP I2C adapter supports SCCB natively. I'm not sure the driver 
> implements that though.

It doesn't currently. And seeing how long it sits in HW without a driver
for it, I don't have much expectations.

> > > we need to forward native SCCB calls all the way down the stack in that
> > > case.
> > 
> > And how is it done currently?
> 
> Currently we go down to .master_xfer(), and adapters can then decide to use 
> the hardware SCCB support. Again, it might not be implemented :-)

To sum it up: hardware-driven SCCB support is a very rare exception not
implemented anywhere in all those years. From a pragmatic point of view,
I'd say: we should be open for it, but we don't need to design around
it.

> I'm fine with SCCB helpers. Please note, however, that SCCB differs from SMBus 
> in two ways: NACKs shall be ignored by the master (even though most SCCB 
> devices generate an ack, so we could likely ignore this), and write-read 
> sequences shouldn't use a repeated start. Apart from that register reads and 

Especially the latter is a huge difference to SMBus, and so I think it
will be much safer to not abuse SMBus calls for SCCB.

> register writes are identical to SMBus, which prompted the reuse (or abuse) of 
> the SMBus API. If we end up implementing SCCB helpers, they will likely look 
> very, very similar to the SMBus implementation, including the SMBus emulated 
> transfer helper.

I don't think so. SCCB has much less transaction types than SMBus. Also, the
fallback as sketched in this patch (one master write, then a master
read) would be impossible on SMBus.

I have an idea in my mind. Maybe it is better to implement an RFC first,
so we can talk over code (even if only in prototype stage). I already
found this in ov7670.c, so I am proven wrong on this one already:

 472  * Note that there are two versions of these.  On the XO 1, the
 473  * i2c controller only does SMBUS, so that's what we use.  The
 474  * ov7670 is not really an SMBUS device, though, so the communication
 475  * is not always entirely reliable.

Sigh...


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

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Akinobu Mita]

2018-04-26 7:40 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Mita-san,
>
> On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
>> 2018-04-24 0:54 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
>> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
>> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
>> >>> 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>
>> >>> Reviewed-by: Rob Herring <robh@kernel.org>
>> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> >>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> >>> ---
>> >>> * v3
>> >>> - Add Reviewed-by: lines
>> >>>
>> >>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 +++++++++++++++
>> >>>  MAINTAINERS                                        |  1 +
>> >>>  2 files changed, 43 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..b045503
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> >>> @@ -0,0 +1,42 @@
>> >>> +* Omnivision OV7720/OV7725 CMOS sensor
>> >>> +
>> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
>> >>> output,
>> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also
>> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
>> >>> formats. +
>> >>> +Required Properties:
>> >>> +- compatible: shall be one of
>> >>> +     "ovti,ov7720"
>> >>> +     "ovti,ov7725"
>> >>> +- clocks: reference to the xclk input clock.
>> >>> +- clock-names: shall be "xclk".
>> >>
>> >> As there's a single clock we could omit clock-names, couldn't we ?
>> >
>> > Sounds good.
>> >
>> > I'll prepare another patch that replaces the clock consumer ID argument
>> > of clk_get() from "xclk" to NULL, and remove the above line in this
>> > bindings.
>>
>> I thought it's easy to do.  However, there is a non-DT user
>> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
>>
>> This can be resolved by retrying clk_get() with NULL if no entry
>> with "xclk".  But should we do so or leave as is?
>
> How about patching the board code to register the clock alias with
>
>         clk_add_alias(NULL, "0-0021", "video_clk", NULL);

Sounds good.

But I'm a bit worried about whether clk_add_alias() can be called with
alias == NULL.  I couldn't find such use case.

Probably Jacopo can verify whether it works or not with v4 patchset.

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[jacopo mondi]

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

Hello Mita-san,

On Fri, Apr 27, 2018 at 01:17:55AM +0900, Akinobu Mita wrote:
> 2018-04-26 7:40 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > Hi Mita-san,
> >
> > On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
> >> 2018-04-24 0:54 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
> >> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
> >> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
> >> >>> 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>
> >> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >> >>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> >>> ---
> >> >>> * v3
> >> >>> - Add Reviewed-by: lines
> >> >>>
> >> >>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 +++++++++++++++
> >> >>>  MAINTAINERS                                        |  1 +
> >> >>>  2 files changed, 43 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..b045503
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> @@ -0,0 +1,42 @@
> >> >>> +* Omnivision OV7720/OV7725 CMOS sensor
> >> >>> +
> >> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
> >> >>> output,
> >> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also
> >> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
> >> >>> formats. +
> >> >>> +Required Properties:
> >> >>> +- compatible: shall be one of
> >> >>> +     "ovti,ov7720"
> >> >>> +     "ovti,ov7725"
> >> >>> +- clocks: reference to the xclk input clock.
> >> >>> +- clock-names: shall be "xclk".
> >> >>
> >> >> As there's a single clock we could omit clock-names, couldn't we ?
> >> >
> >> > Sounds good.
> >> >
> >> > I'll prepare another patch that replaces the clock consumer ID argument
> >> > of clk_get() from "xclk" to NULL, and remove the above line in this
> >> > bindings.
> >>
> >> I thought it's easy to do.  However, there is a non-DT user
> >> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
> >>
> >> This can be resolved by retrying clk_get() with NULL if no entry
> >> with "xclk".  But should we do so or leave as is?
> >
> > How about patching the board code to register the clock alias with
> >
> >         clk_add_alias(NULL, "0-0021", "video_clk", NULL);
>
> Sounds good.
>
> But I'm a bit worried about whether clk_add_alias() can be called with
> alias == NULL.  I couldn't find such use case.
>
> Probably Jacopo can verify whether it works or not with v4 patchset.

Yes, you can use NULL to register a clock alias. Just make sure to drop the
clock name in ov772x driver (I have just verified the following works :)

diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
index 3d7d004..2deee53 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -592,7 +592,7 @@ static int __init migor_devices_setup(void)
        }

        /* Add a clock alias for ov7725 xclk source. */
-       clk_add_alias("xclk", "0-0021", "video_clk", NULL);
+       clk_add_alias(NULL, "0-0021", "video_clk", NULL);

        /* Register GPIOs for video sources. */
        gpiod_add_lookup_table(&ov7725_gpios);
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..e1f4076 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1281,7 +1281,7 @@ static int ov772x_probe(struct i2c_client *client,
        if (priv->hdl.error)
                return priv->hdl.error;

-       priv->clk = clk_get(&client->dev, "xclk");
+       priv->clk = clk_get(&client->dev, NULL);
        if (IS_ERR(priv->clk)) {
                dev_err(&client->dev, "Unable
Thanks
   j

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

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Laurent Pinchart]

Hi Mita-san,

On Thursday, 26 April 2018 19:17:55 EEST Akinobu Mita wrote:
> 2018-04-26 7:40 GMT+09:00 Laurent Pinchart:
> > On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
> >> 2018-04-24 0:54 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
> >> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
> >> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
> >> >>> 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>
> >> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >> >>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> >>> ---
> >> >>> * v3
> >> >>> - Add Reviewed-by: lines
> >> >>> 
> >> >>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42
> >> >>>  +++++++++++++++
> >> >>>  MAINTAINERS                                        |  1 +
> >> >>>  2 files changed, 43 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..b045503
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> >> >>> @@ -0,0 +1,42 @@
> >> >>> +* Omnivision OV7720/OV7725 CMOS sensor
> >> >>> +
> >> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
> >> >>> output,
> >> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It
> >> >>> also
> >> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
> >> >>> formats. +
> >> >>> +Required Properties:
> >> >>> +- compatible: shall be one of
> >> >>> +     "ovti,ov7720"
> >> >>> +     "ovti,ov7725"
> >> >>> +- clocks: reference to the xclk input clock.
> >> >>> +- clock-names: shall be "xclk".
> >> >> 
> >> >> As there's a single clock we could omit clock-names, couldn't we ?
> >> > 
> >> > Sounds good.
> >> > 
> >> > I'll prepare another patch that replaces the clock consumer ID argument
> >> > of clk_get() from "xclk" to NULL, and remove the above line in this
> >> > bindings.
> >> 
> >> I thought it's easy to do.  However, there is a non-DT user
> >> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
> >> 
> >> This can be resolved by retrying clk_get() with NULL if no entry
> >> with "xclk".  But should we do so or leave as is?
> > 
> > How about patching the board code to register the clock alias with
> > 
> >         clk_add_alias(NULL, "0-0021", "video_clk", NULL);
> 
> Sounds good.
> 
> But I'm a bit worried about whether clk_add_alias() can be called with
> alias == NULL.  I couldn't find such use case.

There aren't many occurrences, but

$ find . -type f -exec grep -l 'clk_add_alias(NULL' {} \;
/drivers/clk/ti/clk.c
/drivers/clk/ti/fixed-factor.c
/drivers/clk/ti/clk-dra7-atl.c
/drivers/clk/ti/composite.c

A quick code analysis also shows me that this should be supported.

> Probably Jacopo can verify whether it works or not with v4 patchset.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 01/11] media: dt-bindings: ov772x: add device tree binding

[Akinobu Mita]

2018-04-27 6:34 GMT+09:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Mita-san,
>
> On Thursday, 26 April 2018 19:17:55 EEST Akinobu Mita wrote:
>> 2018-04-26 7:40 GMT+09:00 Laurent Pinchart:
>> > On Wednesday, 25 April 2018 19:19:11 EEST Akinobu Mita wrote:
>> >> 2018-04-24 0:54 GMT+09:00 Akinobu Mita <akinobu.mita@gmail.com>:
>> >> > 2018-04-23 18:17 GMT+09:00 Laurent Pinchart:
>> >> >> On Sunday, 22 April 2018 18:56:07 EEST Akinobu Mita wrote:
>> >> >>> 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>
>> >> >>> Reviewed-by: Rob Herring <robh@kernel.org>
>> >> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> >> >>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> >> >>> ---
>> >> >>> * v3
>> >> >>> - Add Reviewed-by: lines
>> >> >>>
>> >> >>>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42
>> >> >>>  +++++++++++++++
>> >> >>>  MAINTAINERS                                        |  1 +
>> >> >>>  2 files changed, 43 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..b045503
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
>> >> >>> @@ -0,0 +1,42 @@
>> >> >>> +* Omnivision OV7720/OV7725 CMOS sensor
>> >> >>> +
>> >> >>> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions
>> >> >>> output,
>> >> >>> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It
>> >> >>> also
>> >> >>> can +support the YUV422, RGB565/555/444, GRB422 or raw RGB output
>> >> >>> formats. +
>> >> >>> +Required Properties:
>> >> >>> +- compatible: shall be one of
>> >> >>> +     "ovti,ov7720"
>> >> >>> +     "ovti,ov7725"
>> >> >>> +- clocks: reference to the xclk input clock.
>> >> >>> +- clock-names: shall be "xclk".
>> >> >>
>> >> >> As there's a single clock we could omit clock-names, couldn't we ?
>> >> >
>> >> > Sounds good.
>> >> >
>> >> > I'll prepare another patch that replaces the clock consumer ID argument
>> >> > of clk_get() from "xclk" to NULL, and remove the above line in this
>> >> > bindings.
>> >>
>> >> I thought it's easy to do.  However, there is a non-DT user
>> >> (arch/sh/boards/mach-migor/setup.c) that defines a clock with "xclk" ID.
>> >>
>> >> This can be resolved by retrying clk_get() with NULL if no entry
>> >> with "xclk".  But should we do so or leave as is?
>> >
>> > How about patching the board code to register the clock alias with
>> >
>> >         clk_add_alias(NULL, "0-0021", "video_clk", NULL);
>>
>> Sounds good.
>>
>> But I'm a bit worried about whether clk_add_alias() can be called with
>> alias == NULL.  I couldn't find such use case.
>
> There aren't many occurrences, but
>
> $ find . -type f -exec grep -l 'clk_add_alias(NULL' {} \;
> /drivers/clk/ti/clk.c
> /drivers/clk/ti/fixed-factor.c
> /drivers/clk/ti/clk-dra7-atl.c
> /drivers/clk/ti/composite.c
>
> A quick code analysis also shows me that this should be supported.

This hits ti_clk_add_alias() only.  The function name is very similar,
but the first argument is struct device *.

Anyway, I'll add the change you suggested as Jacopo tested it.

Re: [PATCH v3 07/11] media: ov772x: handle nested s_power() calls

[Akinobu Mita]

2018-04-23 17:35 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>    thanks for v3
>
> On Mon, Apr 23, 2018 at 12:56:13AM +0900, Akinobu Mita wrote:
>> Depending on the v4l2 driver, calling s_power() could be nested.  So the
>> actual transitions between power saving mode and normal operation mode
>> should only happen at the first power on and the last power off.
>>
>> This adds an s_power() nesting counter and updates the power state if the
>> counter is modified from 0 to != 0 or from != 0 to 0.
>>
>> 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>
>> ---
>> * v3
>> - Rename mutex name from power_lock to lock
>> - Add warning for duplicated s_power call
>>
>>  drivers/media/i2c/ov772x.c | 34 ++++++++++++++++++++++++++++++----
>>  1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 95c1c95d..8c0b850 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -424,6 +424,9 @@ struct ov772x_priv {
>>       /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>>       unsigned short                    band_filter;
>>       unsigned int                      fps;
>> +     /* lock to protect power_count */
>> +     struct mutex                      lock;
>> +     int                               power_count;
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>       struct media_pad pad;
>>  #endif
>> @@ -871,9 +874,26 @@ static int ov772x_power_off(struct ov772x_priv *priv)
>>  static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>>  {
>>       struct ov772x_priv *priv = to_ov772x(sd);
>> +     int ret = 0;
>> +
>> +     mutex_lock(&priv->lock);
>> +
>> +     /* If the power count is modified from 0 to != 0 or from != 0 to 0,
>> +      * update the power state.
>> +      */
>> +     if (priv->power_count == !on)
>> +             ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
>> +
>> +     if (!ret) {
>> +             /* Update the power count. */
>> +             priv->power_count += on ? 1 : -1;
>> +             WARN(priv->power_count < 0, "Unbalanced power count\n");
>> +             WARN(priv->power_count > 1, "Duplicated s_power call\n");
>
>
> The first time you receive a power on (on == 1, power_count == 0)
> you'll have:
>
>         if (0 == !1)
>                 ret = ov772x_power_on()
>         if (!0)
>                 power_count += 1;
>
> If power management is 'nested' you could receive a second,
> unexpected, power on (on == 1, power_count == 1)
>         if (1 == !1)
>         if (!0)
>                 power_count += 1;
>
> Now power_count == 2, if you now receive a power off, you're going to
> ignore it:
>
>         if (2 == !0)
>                 ret = ov772x_power_off();
>         if (!0)
>                 power_count += -1;
>
> Again you now receive a new power off, and you're going to issue that
> one.
>
>         if (1 == !0)
>                 ret = ov772x_power_off();
>         if (!0)
>                 power_count += -1;
>
> So this seems correct to me!
>
> If I were you, I would simplify this as:
>
>         power_count += on ? 1 : -1;
>         if (power_count == !!on)
>                 return on ? ov772x_power_on(priv) :
>                             ov772x_power_off(priv);
>         WARN_ON(power_count < 0 || power_count > 1);
>
>         return 0;

The only drawback is that the power_count is updated even when the
ov772x_power_on/off failed.

> which, if I'm not wrong expands to:
>
> power_count = 0 ; on = 1
>         power_count += 1;
>         if (1 == !!1)
>                 return ov772x_power_on(priv);
>
> power_count = 1 ; on = 1
>         power_count += 1;
>         if (2 == !!1)
>         WARN_ON(2 > 1)
>
> power_count = 2 ; on = 0
>         power_count -= 1;
>         if (1 == !!0)
>         if (1 > 1 || 1 < 0)
>
> power_count = 1; on = 0
>         power_count -= 1;
>         if (0 == !!0)
>                 return ov772_power_off(priv);
>
> power_count = 0 ; on = 0
>         power_count -= 1;
>         if (-1 == !!0)
>         WARN_ON(-1 < 0)
>
> But if you have seen that pattern in other drivers, and maybe already
> tested your one, I'm fine with either of them.

For now, I leave this patch unchanged. If we can guarantee that s_power()
is not nested, we can simplify it by removing power_count or moving to
common field.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
>
>> +     }
>> +
>> +     mutex_unlock(&priv->lock);
>>
>> -     return on ? ov772x_power_on(priv) :
>> -                 ov772x_power_off(priv);
>> +     return ret;
>>  }
>>
>>  static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
>> @@ -1303,6 +1323,7 @@ static int ov772x_probe(struct i2c_client *client,
>>               return -ENOMEM;
>>
>>       priv->info = client->dev.platform_data;
>> +     mutex_init(&priv->lock);
>>
>>       v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>>       v4l2_ctrl_handler_init(&priv->hdl, 3);
>> @@ -1313,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
>>       v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>>                         V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
>>       priv->subdev.ctrl_handler = &priv->hdl;
>> -     if (priv->hdl.error)
>> -             return priv->hdl.error;
>> +     if (priv->hdl.error) {
>> +             ret = priv->hdl.error;
>> +             goto error_mutex_destroy;
>> +     }
>>
>>       priv->clk = clk_get(&client->dev, "xclk");
>>       if (IS_ERR(priv->clk)) {
>> @@ -1362,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
>>       clk_put(priv->clk);
>>  error_ctrl_free:
>>       v4l2_ctrl_handler_free(&priv->hdl);
>> +error_mutex_destroy:
>> +     mutex_destroy(&priv->lock);
>>
>>       return ret;
>>  }
>> @@ -1376,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
>>               gpiod_put(priv->pwdn_gpio);
>>       v4l2_async_unregister_subdev(&priv->subdev);
>>       v4l2_ctrl_handler_free(&priv->hdl);
>> +     mutex_destroy(&priv->lock);
>>
>>       return 0;
>>  }
>> --
>> 2.7.4
>>