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

[PATCH v4 00/14] 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.

* v4 (thanks to Laurent Pinchart)
- Add Reviewed-by: lines
- Correct setting of banding filter (New)
- Omit consumer ID when getting clock reference (New)
- Use v4l2_ctrl to get current control value (New)
- Correctly restore the controls changed under power saving mode

* 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 (14):
  media: dt-bindings: ov772x: add device tree binding
  media: ov772x: correct setting of banding filter
  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: omit consumer ID when getting clock reference
  media: ov772x: support device tree probing
  media: ov772x: handle nested s_power() calls
  media: ov772x: reconstruct s_frame_interval()
  media: ov772x: use v4l2_ctrl to get current control value
  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       |  40 +++
 MAINTAINERS                                        |   1 +
 arch/sh/boards/mach-migor/setup.c                  |   7 +-
 drivers/media/i2c/ov772x.c                         | 351 +++++++++++++++------
 4 files changed, 302 insertions(+), 97 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>
Cc: Wolfram Sang <wsa@the-dreams.de>
-- 
2.7.4

[PATCH v4 01/14] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v4
- Add Reviewed-by: line
- Omit clock-names property

 .../devicetree/bindings/media/i2c/ov772x.txt       | 40 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 41 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..0b3ede5
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
@@ -0,0 +1,40 @@
+* 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.
+
+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>;
+
+		port {
+			ov772x_0: endpoint {
+				remote-endpoint = <&vcap1_in0>;
+			};
+		};
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index db2bc3f..f39d78b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10363,6 +10363,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 v4 02/14] media: ov772x: correct setting of banding filter

[Akinobu Mita]

The banding filter ON/OFF is controlled via bit 5 of COM8 register.  It
is attempted to be enabled in ov772x_set_params() by the following line.

	ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);

But this unexpectedly results disabling the banding filter, because the
mask and set bits are exclusive.

On the other hand, ov772x_s_ctrl() correctly sets the bit by:

	ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);

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

 drivers/media/i2c/ov772x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..e255070 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1035,7 +1035,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
 	/* Set COM8. */
 	if (priv->band_filter) {
-		ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);
+		ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
 		if (!ret)
 			ret = ov772x_mask_set(client, BDBASE,
 					      0xff, 256 - priv->band_filter);
-- 
2.7.4

[PATCH v4 03/14] 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>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v4
- No changes

 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 e255070..b6223bf 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 v4 04/14] 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>
---
* v4
- No changes

 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 b6223bf..3fdbe64 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 v4 05/14] 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>
---
* v4
- No changes

 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 3fdbe64..bb5327f 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 v4 06/14] 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>
---
* v4
- No changes

 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 bb5327f..97a65ce 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 v4 07/14] media: ov772x: omit consumer ID when getting clock reference

[Akinobu Mita]

Currently the ov772x driver obtains a clock with a specific consumer ID.
As there's a single clock for this driver, we could omit clock-names
property in device tree by passing NULL as a consumer ID to clk_get().

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>
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v4
- New patch

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

diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
index 73b9ee4..11f8001 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -593,7 +593,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 97a65ce..f939e28 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1300,7 +1300,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 to get xclk clock\n");
 		ret = PTR_ERR(priv->clk);
-- 
2.7.4

[PATCH v4 08/14] 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>
---
* v4
- No changes

 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 f939e28..621149a 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 v4 09/14] 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v4
- Add Reviewed-by: line

 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 621149a..edc013d 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, NULL);
 	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 v4 10/14] 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>
---
* v4
- 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 edc013d..7ea157e 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 v4 11/14] media: ov772x: use v4l2_ctrl to get current control value

[Akinobu Mita]

The ov772x driver provides three V4L2 controls and the current value of
each control is saved as a variable in the private data structure.

We don't need to keep track of the current value by ourself, if we use
v4l2_ctrl returned from v4l2_ctrl_new_std() instead.

This is a preparatory change to avoid accessing registers under power
saving mode.  This simplifies s_ctrl() by making it just return without
saving the current control value in private area when it is called 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>
---
* v4
- New patch

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

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 7ea157e..3e6ca98 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -419,10 +419,10 @@ struct ov772x_priv {
 	struct gpio_desc		 *rstb_gpio;
 	const struct ov772x_color_format *cfmt;
 	const struct ov772x_win_size     *win;
-	unsigned short                    flag_vflip:1;
-	unsigned short                    flag_hflip:1;
+	struct v4l2_ctrl		 *vflip_ctrl;
+	struct v4l2_ctrl		 *hflip_ctrl;
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
-	unsigned short                    band_filter;
+	struct v4l2_ctrl		 *band_filter_ctrl;
 	unsigned int			  fps;
 	/* lock to protect power_count */
 	struct mutex			  lock;
@@ -768,13 +768,11 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 	switch (ctrl->id) {
 	case V4L2_CID_VFLIP:
 		val = ctrl->val ? VFLIP_IMG : 0x00;
-		priv->flag_vflip = ctrl->val;
 		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 && (priv->info->flags & OV772X_FLAG_HFLIP))
 			val ^= HFLIP_IMG;
 		return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
@@ -794,8 +792,7 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 				ret = ov772x_mask_set(client, BDBASE,
 						      0xff, val);
 		}
-		if (!ret)
-			priv->band_filter = ctrl->val;
+
 		return ret;
 	}
 
@@ -1075,9 +1072,9 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 		val |= VFLIP_IMG;
 	if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
 		val |= HFLIP_IMG;
-	if (priv->flag_vflip)
+	if (priv->vflip_ctrl->val)
 		val ^= VFLIP_IMG;
-	if (priv->flag_hflip)
+	if (priv->hflip_ctrl->val)
 		val ^= HFLIP_IMG;
 
 	ret = ov772x_mask_set(client,
@@ -1096,11 +1093,13 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 		goto ov772x_set_fmt_error;
 
 	/* Set COM8. */
-	if (priv->band_filter) {
+	if (priv->band_filter_ctrl->val) {
+		unsigned short band_filter = priv->band_filter_ctrl->val;
+
 		ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
 		if (!ret)
 			ret = ov772x_mask_set(client, BDBASE,
-					      0xff, 256 - priv->band_filter);
+					      0xff, 256 - band_filter);
 		if (ret < 0)
 			goto ov772x_set_fmt_error;
 	}
@@ -1341,12 +1340,13 @@ 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);
-	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,
-			  V4L2_CID_HFLIP, 0, 1, 1, 0);
-	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
-			  V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
+	priv->vflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
+					     V4L2_CID_VFLIP, 0, 1, 1, 0);
+	priv->hflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
+					     V4L2_CID_HFLIP, 0, 1, 1, 0);
+	priv->band_filter_ctrl = 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) {
 		ret = priv->hdl.error;
-- 
2.7.4

[PATCH v4 12/14] 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>
---
* v4
- No changes

 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 3e6ca98..bd37169 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;
@@ -885,6 +906,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);
@@ -895,8 +920,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. */
@@ -1163,7 +1200,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;
@@ -1184,14 +1221,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)
@@ -1201,7 +1248,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;
 
@@ -1241,7 +1288,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;
 }
@@ -1340,6 +1387,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;
 	priv->vflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 					     V4L2_CID_VFLIP, 0, 1, 1, 0);
 	priv->hflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
-- 
2.7.4

[PATCH v4 13/14] 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>
---
* v4
- No changes

 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 bd37169..f3c4f78 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 */
 	struct v4l2_ctrl		 *band_filter_ctrl;
 	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,
@@ -1222,6 +1233,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 v4 14/14] 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>
---
* v4
- 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 f3c4f78..ec45eed 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1403,6 +1403,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 v4 02/14] media: ov772x: correct setting of banding filter

[jacopo mondi]

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

Hi Akinobu,
   thanks for the patch

On Mon, Apr 30, 2018 at 02:13:01AM +0900, Akinobu Mita wrote:
> The banding filter ON/OFF is controlled via bit 5 of COM8 register.  It
> is attempted to be enabled in ov772x_set_params() by the following line.
>
> 	ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);
>
> But this unexpectedly results disabling the banding filter, because the
> mask and set bits are exclusive.
>
> On the other hand, ov772x_s_ctrl() correctly sets the bit by:
>
> 	ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
>
> 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>

Acked-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

One unrelated note: the fixes you have added in v4 are very welcome.
For another time, maybe you want to send incremental patches instead
of adding them to a series already in review, as increasing the series
size may slow down its inclusion due to review latencies.
V1 was 6 patches, v2 and v3 10, and this is one 14. This is fine, but
to speed up things, maybe send fixes like this one separately and
clearly state they have some dependency on an already sent series.
That said, I'm not collecting patches, so that's just how I see that,
maybe Sakari, who usually picks sensor driver contributions prefers the way
you sent this.

Thanks
   j

> ---
> * v4
> - New patch
>
>  drivers/media/i2c/ov772x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index b62860c..e255070 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1035,7 +1035,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>
>  	/* Set COM8. */
>  	if (priv->band_filter) {
> -		ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);
> +		ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
>  		if (!ret)
>  			ret = ov772x_mask_set(client, BDBASE,
>  					      0xff, 256 - priv->band_filter);
> --
> 2.7.4
>

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

Re: [PATCH v4 03/14] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[jacopo mondi]

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

Hi Akinobu,

On Mon, Apr 30, 2018 at 02:13:02AM +0900, Akinobu Mita wrote:
> The ov772x driver only works when the i2c controller have
> I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> support it.
>
> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> it doesn't support repeated starts.
>
> This 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>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

As you have sent a separate series to move the SCCB case handling to
the i2c core, and I feel like it may take some time to get there, as
Sakari suggested, I'm fine with this change in the driver code.

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

Thanks
  j

> ---
> * v4
> - No changes
>
>  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 e255070..b6223bf 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
>

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

Re: [PATCH v4 08/14] media: ov772x: support device tree probing

[jacopo mondi]

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

Akinobu,
    a very minor thing, please consider this only if you have to
resend.

On Mon, Apr 30, 2018 at 02:13:07AM +0900, Akinobu Mita wrote:
> The ov772x driver currently only supports legacy platform data probe.
> This change enables device tree probing.
>
> Note that the platform data probe can select auto or manual edge control
> mode, but the device tree probling can only select auto edge control mode
> for now.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v4
> - No changes
>
>  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 f939e28..621149a 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);

You have two spaces before 'ov772x_edgectrl(priv)'

Thanks
   j


> +	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
>

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

Re: [PATCH v4 10/14] media: ov772x: reconstruct s_frame_interval()

[jacopo mondi]

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

Hi Akinobu,
   thank you for the patch

On Mon, Apr 30, 2018 at 02:13:09AM +0900, Akinobu Mita wrote:
> 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>
> ---
> * v4
> - 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 edc013d..7ea157e 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)

Please align to the open brace, as all the other functions in the
driver.

Also, is it worth making a function out of this? It is called from one
place only... see below.

>  {
> -	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;

Please keep variable declarations sorted as in other functions in the
driver.

> +	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);

That's the only caller of this function. I'm fine if you want to keep
it as it is though.

Thanks
   j


> +
> +	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
>

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

Re: [PATCH v4 12/14] media: ov772x: avoid accessing registers under power saving mode

[jacopo mondi]

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

Hi Akinobu,
  let me see if I got this right...

On Mon, Apr 30, 2018 at 02:13:11AM +0900, Akinobu Mita wrote:
> 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>
> ---
> * v4
> - No changes
>
>  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 3e6ca98..bd37169 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) {

If I'm not wrong this is not protected when the device is
streaming.

Since Hans' series frame control is called by g/s_parm (until a new
ioctl is not introduced) and I've looked at the call stack and it
seems to me it does not check for active streaming[1]. I -think-
this is even worse when this is called on the subdev, as there is
no shared notion of active streaming. Am I wrong?

If you have to check for active streaming here (I see some other
drivers doing that, eg ov5640), then I see two ways, or you just
return -EBUSY, or you save the desired FPS for later, but then it gets
messy as you won't be able to just restore paramters at power_on()
time, but you would need to do that also at start streaming time.

My opinion is that you should check for streaming active (as you're
doing for the set_fmt() function in [13/14], do you agree?

[1] This calls for a device wise 'streaming' state handler. Maybe it's
there but I failed to find checks for that.

> +		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;
> @@ -885,6 +906,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);
> @@ -895,8 +920,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. */
> @@ -1163,7 +1200,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;
> @@ -1184,14 +1221,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)
> @@ -1201,7 +1248,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;
>
> @@ -1241,7 +1288,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;
>  }
> @@ -1340,6 +1387,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;

This is unrelated and should probably go in the previous patch (which
I would move before the one where you split s_frame_interval up, for
clearness)

Thanks
   j

>  	priv->vflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>  					     V4L2_CID_VFLIP, 0, 1, 1, 0);
>  	priv->hflip_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
> --
> 2.7.4
>

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

Re: [PATCH v4 12/14] media: ov772x: avoid accessing registers under power saving mode

[Akinobu Mita]

2018-05-04 6:03 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>   let me see if I got this right...
>
> On Mon, Apr 30, 2018 at 02:13:11AM +0900, Akinobu Mita wrote:
>> 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>
>> ---
>> * v4
>> - No changes
>>
>>  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 3e6ca98..bd37169 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) {
>
> If I'm not wrong this is not protected when the device is
> streaming.
>
> Since Hans' series frame control is called by g/s_parm (until a new
> ioctl is not introduced) and I've looked at the call stack and it
> seems to me it does not check for active streaming[1]. I -think-
> this is even worse when this is called on the subdev, as there is
> no shared notion of active streaming. Am I wrong?
>
> If you have to check for active streaming here (I see some other
> drivers doing that, eg ov5640), then I see two ways, or you just
> return -EBUSY, or you save the desired FPS for later, but then it gets
> messy as you won't be able to just restore paramters at power_on()
> time, but you would need to do that also at start streaming time.
>
> My opinion is that you should check for streaming active (as you're
> doing for the set_fmt() function in [13/14], do you agree?

I agree.  I would like to make ov772x_s_frame_interval() return -EBUSY
without saving the desired FPS for later when the device is streaming.

I'm going to prepare v5 patches that address the above and other issues
you found in v4 unless you prefer the incremental patch series.

> [1] This calls for a device wise 'streaming' state handler. Maybe it's
> there but I failed to find checks for that.

Re: [PATCH v4 12/14] media: ov772x: avoid accessing registers under power saving mode

[jacopo mondi]

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

Hi Akinobu,

On Fri, May 04, 2018 at 11:50:39PM +0900, Akinobu Mita wrote:
> 2018-05-04 6:03 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> > Hi Akinobu,
> >   let me see if I got this right...
> >
> > On Mon, Apr 30, 2018 at 02:13:11AM +0900, Akinobu Mita wrote:
> >> 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>
> >> ---
> >> * v4
> >> - No changes
> >>
> >>  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 3e6ca98..bd37169 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) {
> >
> > If I'm not wrong this is not protected when the device is
> > streaming.
> >
> > Since Hans' series frame control is called by g/s_parm (until a new
> > ioctl is not introduced) and I've looked at the call stack and it
> > seems to me it does not check for active streaming[1]. I -think-
> > this is even worse when this is called on the subdev, as there is
> > no shared notion of active streaming. Am I wrong?
> >
> > If you have to check for active streaming here (I see some other
> > drivers doing that, eg ov5640), then I see two ways, or you just
> > return -EBUSY, or you save the desired FPS for later, but then it gets
> > messy as you won't be able to just restore paramters at power_on()
> > time, but you would need to do that also at start streaming time.
> >
> > My opinion is that you should check for streaming active (as you're
> > doing for the set_fmt() function in [13/14], do you agree?
>
> I agree.  I would like to make ov772x_s_frame_interval() return -EBUSY
> without saving the desired FPS for later when the device is streaming.
>
> I'm going to prepare v5 patches that address the above and other issues
> you found in v4 unless you prefer the incremental patch series.

Thank you. Please send v5, the incremental patches thing only applies
to new developments and fixes not already part of this series.

Thanks
   j

>
> > [1] This calls for a device wise 'streaming' state handler. Maybe it's
> > there but I failed to find checks for that.

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

Re: [PATCH v4 10/14] media: ov772x: reconstruct s_frame_interval()

[Akinobu Mita]

2018-05-04 5:29 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>    thank you for the patch
>
> On Mon, Apr 30, 2018 at 02:13:09AM +0900, Akinobu Mita wrote:
>> 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>
>> ---
>> * v4
>> - 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 edc013d..7ea157e 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)
>
> Please align to the open brace, as all the other functions in the
> driver.

OK.

> Also, is it worth making a function out of this? It is called from one
> place only... see below.
>
>>  {
>> -     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;
>
> Please keep variable declarations sorted as in other functions in the
> driver.

OK.

>> +     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);
>
> That's the only caller of this function. I'm fine if you want to keep
> it as it is though.

I would like to keep ov772x_select_fps() because the function name is
self descriptive and imples it doesn't change HW registers.  So I feel
it helps readability a bit.

> Thanks
>    j
>
>
>> +
>> +     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
>>