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

[PATCH v5 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.

* v5 (thanks to Jacopo Mondi)
- Add Acked-by: line
- Add Reviewed-by: line
- Remove unnecessary space
- Align arguments to open parenthesis
- Sort variable declarations
- Make s_frame_interval() return -EBUSY while streaming

* 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() and s_frame_interval() 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                         | 357 +++++++++++++++------
 4 files changed, 308 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 v5 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>
---
* v5
- No changes

 .../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 df6e9bb..cd4c270 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10356,6 +10356,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 v5 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>
Acked-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v5
- Add Acked-by: line

 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 v5 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>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v5
- Add 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 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 v5 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>
---
* v5
- 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 v5 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>
---
* v5
- 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 v5 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>
---
* v5
- 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 v5 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>
---
* v5
- No changes

 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 v5 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>
---
* v5
- Remove unnecessary space

 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..2b02411 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 v5 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>
---
* v5
- No changes

 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 2b02411..6c0c792 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 v5 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>
---
* v5
- Align arguments to open parenthesis
- Sort variable declarations

 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 6c0c792..92ad13f 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 best_diff;
+	unsigned int fsize;
+	unsigned int pclk;
+	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 v5 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>
---
* v5
- No changes

 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 92ad13f..9292a18 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 v5 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>
---
* v5
- 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 9292a18..262a7e5 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 v5 13/14] media: ov772x: make set_fmt() and s_frame_interval() return -EBUSY while streaming

[Akinobu Mita]

The ov772x driver is going to offer a V4L2 sub-device interface, so
changing the output data format and the frame interval on this sub-device
can be made anytime.  However, these requests are 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>
---
* v5:
- Make s_frame_interval() return -EBUSY while streaming

 drivers/media/i2c/ov772x.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 262a7e5..4b479f9 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,
@@ -743,9 +754,15 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
 	unsigned int fps;
 	int ret = 0;
 
+	mutex_lock(&priv->lock);
+
+	if (priv->streaming) {
+		ret = -EBUSY;
+		goto error;
+	}
+
 	fps = ov772x_select_fps(priv, tpf);
 
-	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
@@ -1222,6 +1239,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 v5 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>
---
* v5
- 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 4b479f9..f7f4fe6 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1409,6 +1409,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 v5 08/14] media: ov772x: support device tree probing

[Sakari Ailus]

Dear Mita-san,

On Sun, May 06, 2018 at 11:19:23PM +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>
> ---
> * v5
> - Remove unnecessary space
> 
>  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..2b02411 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

No need for #ifdef's nor of_match_ptr below.

If no other changes would be needed for the set (pending Jacopo's review),
I can also drop these bits if you're ok for that.

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

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Akinobu Mita]

2018-05-07 18:26 GMT+09:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
> Dear Mita-san,
>
> On Sun, May 06, 2018 at 11:19:23PM +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>
>> ---
>> * v5
>> - Remove unnecessary space
>>
>>  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..2b02411 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
>
> No need for #ifdef's nor of_match_ptr below.
>
> If no other changes would be needed for the set (pending Jacopo's review),
> I can also drop these bits if you're ok for that.

I thought #ifdef was required to compile without CONFIG_OF, but it
actually wasn't required.  So the change is fine with me.

>> +
>>  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,
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

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

[jacopo mondi]

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

Hi Akinobu,
   thanks for resending

On Sun, May 06, 2018 at 11:19:25PM +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>

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

Thanks
  j

> ---
> * v5
> - Align arguments to open parenthesis
> - Sort variable declarations
>
>  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 6c0c792..92ad13f 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 best_diff;
> +	unsigned int fsize;
> +	unsigned int pclk;
> +	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
>

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

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

[jacopo mondi]

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

Hi Akinobu,

   a small nit below

On Sun, May 06, 2018 at 11:19:27PM +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>
> ---
> * v5
> - 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 9292a18..262a7e5 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;

Isn't this unrelated?

Apart from that,

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

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 v5 13/14] media: ov772x: make set_fmt() and s_frame_interval() return -EBUSY while streaming

[jacopo mondi]

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

Hi Akinobu,
   thanks for the patch

On Sun, May 06, 2018 at 11:19:28PM +0900, Akinobu Mita wrote:
> The ov772x driver is going to offer a V4L2 sub-device interface, so
> changing the output data format and the frame interval on this sub-device
> can be made anytime.  However, these requests are 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>

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

Thanks
   j
> ---
> * v5:
> - Make s_frame_interval() return -EBUSY while streaming
>
>  drivers/media/i2c/ov772x.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 262a7e5..4b479f9 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,
> @@ -743,9 +754,15 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
>  	unsigned int fps;
>  	int ret = 0;
>
> +	mutex_lock(&priv->lock);
> +
> +	if (priv->streaming) {
> +		ret = -EBUSY;
> +		goto error;
> +	}
> +
>  	fps = ov772x_select_fps(priv, tpf);
>
> -	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
> @@ -1222,6 +1239,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
>

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

Re: [PATCH v5 14/14] media: ov772x: create subdevice device node

[jacopo mondi]

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

Hi Akinobu,
   thanks for the patch.

On Sun, May 06, 2018 at 11:19:29PM +0900, Akinobu Mita wrote:
> 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>

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

Thanks
   j

> ---
> * v5
> - 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 4b479f9..f7f4fe6 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1409,6 +1409,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
>

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

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

[jacopo mondi]

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

Hi Sakari,

On Mon, May 07, 2018 at 12:26:41PM +0300, Sakari Ailus wrote:
> Dear Mita-san,
>
> On Sun, May 06, 2018 at 11:19:23PM +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>
> > ---
> > * v5
> > - Remove unnecessary space
> >
> >  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..2b02411 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
>
> No need for #ifdef's nor of_match_ptr below.
>
> If no other changes would be needed for the set (pending Jacopo's review),
> I can also drop these bits if you're ok for that.

I've now reviewed all the patches in the series, 11/14 apart where I
would apreciate if you could have a look at (nothing complex, just me
not knowing enough of controls to feel confident enough to give my ack
there).

From my side then, the series is now fine. Thank Mita-san for your
work.

>
> > +
> >  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,
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

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

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

[Sakari Ailus]

Hi Jacopo,

On Mon, May 14, 2018 at 11:06:46AM +0200, jacopo mondi wrote:
> Hi Akinobu,
> 
>    a small nit below
> 
> On Sun, May 06, 2018 at 11:19:27PM +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>
> > ---
> > * v5
> > - 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 9292a18..262a7e5 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;
> 
> Isn't this unrelated?

AFAICT not, since the access to power count is serialised using the
driver's mutex. The power count is used in the s_ctrl callback as well.

> 
> Apart from that,
> 
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> 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
> >



-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[jacopo mondi]

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

Hi Sakari,
   thanks for the clarification, please ignore my comment then.

Thanks
   j

On Mon, May 14, 2018 at 12:49:19PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, May 14, 2018 at 11:06:46AM +0200, jacopo mondi wrote:
> > Hi Akinobu,
> >
> >    a small nit below
> >
> > On Sun, May 06, 2018 at 11:19:27PM +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>
> > > ---
> > > * v5
> > > - 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 9292a18..262a7e5 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;
> >
> > Isn't this unrelated?
>
> AFAICT not, since the access to power count is serialised using the
> driver's mutex. The power count is used in the s_ctrl callback as well.
>
> >
> > Apart from that,
> >
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > 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
> > >
>
>
>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com

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

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

[Sakari Ailus]

On Mon, May 14, 2018 at 11:16:42AM +0200, jacopo mondi wrote:
> Hi Sakari,
> 
> On Mon, May 07, 2018 at 12:26:41PM +0300, Sakari Ailus wrote:
> > Dear Mita-san,
> >
> > On Sun, May 06, 2018 at 11:19:23PM +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>
> > > ---
> > > * v5
> > > - Remove unnecessary space
> > >
> > >  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..2b02411 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
> >
> > No need for #ifdef's nor of_match_ptr below.
> >
> > If no other changes would be needed for the set (pending Jacopo's review),
> > I can also drop these bits if you're ok for that.
> 
> I've now reviewed all the patches in the series, 11/14 apart where I
> would apreciate if you could have a look at (nothing complex, just me
> not knowing enough of controls to feel confident enough to give my ack
> there).
> 
> From my side then, the series is now fine. Thank Mita-san for your
> work.

Thanks. The diff is here:

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2b02411e9a5e..b5ae88e69b94 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1386,19 +1386,17 @@ 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),
+		.of_match_table = ov772x_of_match,
 	},
 	.probe    = ov772x_probe,
 	.remove   = ov772x_remove,

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Mauro Carvalho Chehab]

Em Sun,  6 May 2018 23:19:18 +0900
Akinobu Mita <akinobu.mita@gmail.com> escreveu:

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

I had a Déjà vu when I looked on this patch, as I saw the same
issue I pointed to another patch you submitted ;-)

So, I ended by not replying to this one. Just to be clear: the same
comment I did to the SCCB helpers apply here:

	https://www.mail-archive.com/linux-media@vger.kernel.org/msg130868.html

It is a very bad idea to replace an i2c xfer by a pair of i2c
send()/recv(), as, if are there any other device at the bus managed
by an independent driver, you may end by mangling i2c transfers and
eventually cause device malfunctions.

I've seen it a lot on media devices that have devices that need
constant polling via I2C (e. g. hardware with remote controllers,
for example).

So, IMO, the best is to push the patch you proposed that adds a
new I2C flag:

	https://patchwork.linuxtv.org/patch/49396/
 
> 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>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v5
> - Add 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 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)



Thanks,
Mauro

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

[Wolfram Sang]

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


> It is a very bad idea to replace an i2c xfer by a pair of i2c
> send()/recv(), as, if are there any other device at the bus managed
> by an independent driver, you may end by mangling i2c transfers and
> eventually cause device malfunctions.

For I2C, this is true and a very important detail. Yet, we are talking
not I2C but SCCB here and SCCB demands a STOP between messages. So,
technically, to avoid what you describe one shouldn't mix I2C and SCCB
devices. I am quite aware the reality is very different, but still...

My preference would be to stop acting as SCCB was I2C but give it its
own set of functions so it becomes clear for everyone what protocol is
used for what device.

> So, IMO, the best is to push the patch you proposed that adds a
> new I2C flag:
> 
> 	https://patchwork.linuxtv.org/patch/49396/

Sorry, but I don't like it. This makes the I2C core code very
unreadable. This is why I think SCCB should be exported to its own
realm. Which may live in i2c-core-sccb.c, no need for a seperate
subsystem.


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

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

[Mauro Carvalho Chehab]

Em Tue, 29 May 2018 15:29:29 +0200
Wolfram Sang <wsa@the-dreams.de> escreveu:

> > It is a very bad idea to replace an i2c xfer by a pair of i2c
> > send()/recv(), as, if are there any other device at the bus managed
> > by an independent driver, you may end by mangling i2c transfers and
> > eventually cause device malfunctions.  
> 
> For I2C, this is true and a very important detail. Yet, we are talking
> not I2C but SCCB here and SCCB demands a STOP between messages. So,
> technically, to avoid what you describe one shouldn't mix I2C and SCCB
> devices. I am quite aware the reality is very different, but still...
> 
> My preference would be to stop acting as SCCB was I2C but give it its
> own set of functions so it becomes clear for everyone what protocol is
> used for what device.
> 
> > So, IMO, the best is to push the patch you proposed that adds a
> > new I2C flag:
> > 
> > 	https://patchwork.linuxtv.org/patch/49396/  
> 
> Sorry, but I don't like it. This makes the I2C core code very
> unreadable. This is why I think SCCB should be exported to its own
> realm. Which may live in i2c-core-sccb.c, no need for a seperate
> subsystem.

We actually have the same issue with pure I2C devices on media.
There are several I2C devices that don't accept repeat start.

The solution given there was hackish and varies from driver to driver.
The most common solution were to patch the I2C xfer callback 
function of the I2C master code in order to prevent them to do
I2C repeated start ops, usually checking for some specific I2C
addresses where repeated start is known to not working.

So, IMHO, a flag like that would be an improvement not only for
SCCB but also for other I2C devices. Probably there would be
other ways to do it without making the I2C core code harder to
read.

Thanks,
Mauro