[Patch v2 0/7] media: i2c: ov2659: maintenance series

[Patch v2 0/7] media: i2c: ov2659: maintenance series

[Benoit Parrot]

This patch series is a collection of patches we have been carrying for a
while.

It includes a few sensor register fixes which would cause visual
artifacts at lower resolution and also at 720p.

Also on some board the 'powerdown' and /or 'reset' pins are not tied so
we need to add support for optional gpios to handle these.

Since these camera are removable on some board we also need the driver
to actually fail when there is no hardware present so the driver is
actually removed.

Finally, we update the licensing statement to use SPDX licensing.

Changes since v1:
- Addressed review comment from Prabhakar
- Added support for reset-gpios
- Rework the power setting to use pm_runtime instead of s_power
  as based on discussion with Sakari it would be the prefered method
- Added a patch to reduce the number explicit include files to the
  minimum necessary instead of the previous kitchen sink approach

Benoit Parrot (7):
  media: i2c: ov2659: Fix for image wrap-around in lower resolution
  media: i2c: ov2659: Fix sensor detection to actually fail when device
    is not present
  media: i2c: ov2659: Cleanup include file list
  media: dt-bindings: ov2659: add powerdown/reset-gpios optional
    property
  media: i2c: ov2659: Add powerdown/reset gpio handling
  media: i2c: ov2659: Fix missing 720p register config
  media: i2c: ov2659: Switch to SPDX Licensing

 .../devicetree/bindings/media/i2c/ov2659.txt  |   9 ++
 drivers/media/i2c/Kconfig                     |   2 +-
 drivers/media/i2c/ov2659.c                    | 129 +++++++++++++-----
 3 files changed, 107 insertions(+), 33 deletions(-)

-- 
2.17.1

[Patch v2 1/7] media: i2c: ov2659: Fix for image wrap-around in lower resolution

[Benoit Parrot]

Based on recently found sensor configuration examples, it was
discovered that when scaling and binning are used for the lower
resolutions (i.e. 640x480, 320x240) the read offset has to be
increased otherwise the image appears to be wrapped around.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/media/i2c/ov2659.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index f4ded0669ff9..17573257097d 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -661,7 +661,7 @@ static struct sensor_register ov2659_vga[] = {
 	{ REG_TIMING_HORIZ_FORMAT, 0x01 },
 	{ 0x370a, 0x52 },
 	{ REG_VFIFO_READ_START_H, 0x00 },
-	{ REG_VFIFO_READ_START_L, 0x80 },
+	{ REG_VFIFO_READ_START_L, 0xa0 },
 	{ REG_ISP_CTRL02, 0x10 },
 	{ REG_NULL, 0x00 },
 };
@@ -709,7 +709,7 @@ static  struct sensor_register ov2659_qvga[] = {
 	{ REG_TIMING_HORIZ_FORMAT, 0x01 },
 	{ 0x370a, 0x52 },
 	{ REG_VFIFO_READ_START_H, 0x00 },
-	{ REG_VFIFO_READ_START_L, 0x80 },
+	{ REG_VFIFO_READ_START_L, 0xa0 },
 	{ REG_ISP_CTRL02, 0x10 },
 	{ REG_NULL, 0x00 },
 };
-- 
2.17.1

[Patch v2 2/7] media: i2c: ov2659: Fix sensor detection to actually fail when device is not present

[Benoit Parrot]

Make sure that if the expected sensor device id register
is not recognized properly the failure is propagated
up so devices are not left partially initialized.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/media/i2c/ov2659.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index 17573257097d..efbe6dc720e2 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -1330,11 +1330,12 @@ static int ov2659_detect(struct v4l2_subdev *sd)
 		unsigned short id;
 
 		id = OV265X_ID(pid, ver);
-		if (id != OV2659_ID)
+		if (id != OV2659_ID) {
 			dev_err(&client->dev,
 				"Sensor detection failed (%04X, %d)\n",
 				id, ret);
-		else {
+			ret = -ENODEV;
+		} else {
 			dev_info(&client->dev, "Found OV%04X sensor\n", id);
 			ret = ov2659_init(sd, 0);
 		}
-- 
2.17.1

[Patch v2 3/7] media: i2c: ov2659: Cleanup include file list

[Benoit Parrot]

Several of include files listed are not explicitly needed.
If they are need then they are implicitly included.

Reduce the list of includes to an easier to manage list.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/i2c/ov2659.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index efbe6dc720e2..f77320e8a60d 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -22,29 +22,15 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
-#include <linux/err.h>
-#include <linux/init.h>
-#include <linux/interrupt.h>
-#include <linux/io.h>
 #include <linux/i2c.h>
-#include <linux/kernel.h>
-#include <linux/media.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/of_graph.h>
-#include <linux/slab.h>
-#include <linux/uaccess.h>
-#include <linux/videodev2.h>
 
-#include <media/media-entity.h>
 #include <media/i2c/ov2659.h>
-#include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
-#include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-image-sizes.h>
-#include <media/v4l2-mediabus.h>
 #include <media/v4l2-subdev.h>
 
 #define DRIVER_NAME "ov2659"
-- 
2.17.1

[Patch v2 4/7] media: dt-bindings: ov2659: add powerdown/reset-gpios optional property

[Benoit Parrot]

Add powerdown-gpios and reset-gpios to the list of optional properties
for the OV2659 camera sensor.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 Documentation/devicetree/bindings/media/i2c/ov2659.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
index cabc7d827dfb..92989a619f29 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
@@ -12,6 +12,12 @@ Required Properties:
 - clock-names: should be "xvclk".
 - link-frequencies: target pixel clock frequency.
 
+Optional Properties:
+- powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
+  Active high with internal pull down resistor.
+- reset-gpios: reference to the GPIO connected to the resetb pin, if any.
+  Active low with internal pull up resistor.
+
 For further reading on port node refer to
 Documentation/devicetree/bindings/media/video-interfaces.txt.
 
@@ -27,6 +33,9 @@ Example:
 			clocks = <&clk_ov2659 0>;
 			clock-names = "xvclk";
 
+			powerdown-gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>;
+			reset-gpios = <&gpio6 15 GPIO_ACTIVE_LOW>;
+
 			port {
 				ov2659_0: endpoint {
 					remote-endpoint = <&vpfe_ep>;
-- 
2.17.1

[Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio handling

[Benoit Parrot]

On some board it is possible that the sensor 'powerdown' and or 'reset'
pin might be controlled by gpio instead of being tied.

To implement we add pm_runtime support which will handle the power
up/down sequence.

Now originally the driver assumed tat the sensor would always stay
powered and there keep its register setting. We cannot assume that this
anymore, so every time we "power up" we need to re-program the initial
registers configuration first. This was previously done only at probe
time.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 7eee1812bba3..315c1d8bdb7b 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -634,7 +634,7 @@ config VIDEO_OV2640
 config VIDEO_OV2659
 	tristate "OmniVision OV2659 sensor support"
 	depends on VIDEO_V4L2 && I2C
-	depends on MEDIA_CAMERA_SUPPORT
+	depends on MEDIA_CAMERA_SUPPORT && GPIOLIB
 	select V4L2_FWNODE
 	help
 	  This is a Video4Linux2 sensor driver for the OmniVision
diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index f77320e8a60d..170f80a1a51f 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -22,9 +22,11 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
+#include <linux/pm_runtime.h>
 
 #include <media/i2c/ov2659.h>
 #include <media/v4l2-ctrls.h>
@@ -218,6 +220,11 @@ struct ov2659 {
 	struct sensor_register *format_ctrl_regs;
 	struct ov2659_pll_ctrl pll;
 	int streaming;
+	/* used to control the sensor PWDN pin */
+	struct gpio_desc *pwdn_gpio;
+	/* used to control the sensor RESETB pin */
+	struct gpio_desc *resetb_gpio;
+	int on;
 };
 
 static const struct sensor_register ov2659_init_regs[] = {
@@ -1184,9 +1191,17 @@ static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
 		/* Stop Streaming Sequence */
 		ov2659_set_streaming(ov2659, 0);
 		ov2659->streaming = on;
+		pm_runtime_put(&client->dev);
 		goto unlock;
 	}
 
+	ret = pm_runtime_get_sync(&client->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&client->dev);
+		goto unlock;
+	}
+
+	ov2659_init(sd, 0);
 	ov2659_set_pixel_clock(ov2659);
 	ov2659_set_frame_size(ov2659);
 	ov2659_set_format(ov2659);
@@ -1243,6 +1258,32 @@ static const char * const ov2659_test_pattern_menu[] = {
 	"Vertical Color Bars",
 };
 
+static int ov2659_set_power(struct ov2659 *ov2659, int on)
+{
+	struct i2c_client *client = ov2659->client;
+
+	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
+
+	if (on) {
+		if (ov2659->pwdn_gpio)
+			gpiod_direction_output(ov2659->pwdn_gpio, 0);
+
+		if (ov2659->resetb_gpio) {
+			gpiod_set_value(ov2659->resetb_gpio, 1);
+			usleep_range(500, 1000);
+			gpiod_set_value(ov2659->resetb_gpio, 0);
+			usleep_range(3000, 5000);
+		}
+	} else {
+		if (ov2659->pwdn_gpio)
+			gpiod_direction_output(ov2659->pwdn_gpio, 1);
+	}
+
+	ov2659->on = on;
+
+	return 0;
+}
+
 /* -----------------------------------------------------------------------------
  * V4L2 subdev internal operations
  */
@@ -1323,7 +1364,6 @@ static int ov2659_detect(struct v4l2_subdev *sd)
 			ret = -ENODEV;
 		} else {
 			dev_info(&client->dev, "Found OV%04X sensor\n", id);
-			ret = ov2659_init(sd, 0);
 		}
 	}
 
@@ -1400,6 +1440,18 @@ static int ov2659_probe(struct i2c_client *client)
 	    ov2659->xvclk_frequency > 27000000)
 		return -EINVAL;
 
+	/* Optional gpio don't fail if not present */
+	ov2659->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(ov2659->pwdn_gpio))
+		return PTR_ERR(ov2659->pwdn_gpio);
+
+	/* Optional gpio don't fail if not present */
+	ov2659->resetb_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+						      GPIOD_OUT_HIGH);
+	if (IS_ERR(ov2659->resetb_gpio))
+		return PTR_ERR(ov2659->resetb_gpio);
+
 	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
 	ov2659->link_frequency =
 			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
@@ -1445,6 +1497,9 @@ static int ov2659_probe(struct i2c_client *client)
 	ov2659->frame_size = &ov2659_framesizes[2];
 	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
 
+	pm_runtime_enable(&client->dev);
+	pm_runtime_get_sync(&client->dev);
+
 	ret = ov2659_detect(sd);
 	if (ret < 0)
 		goto error;
@@ -1458,10 +1513,14 @@ static int ov2659_probe(struct i2c_client *client)
 
 	dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
 
+	pm_runtime_put(&client->dev);
+
 	return 0;
 
 error:
 	v4l2_ctrl_handler_free(&ov2659->ctrls);
+	pm_runtime_put(&client->dev);
+	pm_runtime_disable(&client->dev);
 	media_entity_cleanup(&sd->entity);
 	mutex_destroy(&ov2659->lock);
 	return ret;
@@ -1477,9 +1536,35 @@ static int ov2659_remove(struct i2c_client *client)
 	media_entity_cleanup(&sd->entity);
 	mutex_destroy(&ov2659->lock);
 
+	pm_runtime_disable(&client->dev);
+
 	return 0;
 }
 
+static int ov2659_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov2659 *ov2659 = to_ov2659(sd);
+
+	ov2659_set_power(ov2659, 0);
+
+	return 0;
+}
+
+static int ov2659_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov2659 *ov2659 = to_ov2659(sd);
+
+	return ov2659_set_power(ov2659, 1);
+}
+
+static const struct dev_pm_ops ov2659_pm_ops = {
+	SET_RUNTIME_PM_OPS(ov2659_runtime_suspend, ov2659_runtime_resume, NULL)
+};
+
 static const struct i2c_device_id ov2659_id[] = {
 	{ "ov2659", 0 },
 	{ /* sentinel */ },
@@ -1497,6 +1582,7 @@ MODULE_DEVICE_TABLE(of, ov2659_of_match);
 static struct i2c_driver ov2659_i2c_driver = {
 	.driver = {
 		.name	= DRIVER_NAME,
+		.pm	= &ov2659_pm_ops,
 		.of_match_table = of_match_ptr(ov2659_of_match),
 	},
 	.probe_new	= ov2659_probe,
-- 
2.17.1

[Patch v2 6/7] media: i2c: ov2659: Fix missing 720p register config

[Benoit Parrot]

The initial registers sequence is only loaded at probe
time. Afterward only the resolution and format specific
register are modified. Care must be taken to make sure
registers modified by one resolution setting are reverted
back when another resolution is programmed.

This was not done properly for the 720p case.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/i2c/ov2659.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index 170f80a1a51f..d4341204207d 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -412,10 +412,14 @@ static struct sensor_register ov2659_720p[] = {
 	{ REG_TIMING_YINC, 0x11 },
 	{ REG_TIMING_VERT_FORMAT, 0x80 },
 	{ REG_TIMING_HORIZ_FORMAT, 0x00 },
+	{ 0x370a, 0x12 },
 	{ 0x3a03, 0xe8 },
 	{ 0x3a09, 0x6f },
 	{ 0x3a0b, 0x5d },
 	{ 0x3a15, 0x9a },
+	{ REG_VFIFO_READ_START_H, 0x00 },
+	{ REG_VFIFO_READ_START_L, 0x80 },
+	{ REG_ISP_CTRL02, 0x00 },
 	{ REG_NULL, 0x00 },
 };
 
-- 
2.17.1

[Patch v2 7/7] media: i2c: ov2659: Switch to SPDX Licensing

[Benoit Parrot]

Switch to SPDX licensing and drop the redundant GPL text.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/i2c/ov2659.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
index d4341204207d..8c7cdd6d8e56 100644
--- a/drivers/media/i2c/ov2659.c
+++ b/drivers/media/i2c/ov2659.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Omnivision OV2659 CMOS Image Sensor driver
  *
@@ -5,19 +6,6 @@
  *
  * Benoit Parrot <bparrot@ti.com>
  * Lad, Prabhakar <prabhakar.csengg@gmail.com>
- *
- * This program is free software; you may redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; version 2 of the License.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
  */
 
 #include <linux/clk.h>
-- 
2.17.1

Re: [Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio handling

[Sakari Ailus]

Hi Benoit,

Thanks for the update.

On Thu, Sep 19, 2019 at 03:39:53PM -0500, Benoit Parrot wrote:
> On some board it is possible that the sensor 'powerdown' and or 'reset'
> pin might be controlled by gpio instead of being tied.
> 
> To implement we add pm_runtime support which will handle the power
> up/down sequence.
> 
> Now originally the driver assumed tat the sensor would always stay
> powered and there keep its register setting. We cannot assume that this
> anymore, so every time we "power up" we need to re-program the initial
> registers configuration first. This was previously done only at probe
> time.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/i2c/Kconfig  |  2 +-
>  drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 7eee1812bba3..315c1d8bdb7b 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -634,7 +634,7 @@ config VIDEO_OV2640
>  config VIDEO_OV2659
>  	tristate "OmniVision OV2659 sensor support"
>  	depends on VIDEO_V4L2 && I2C
> -	depends on MEDIA_CAMERA_SUPPORT
> +	depends on MEDIA_CAMERA_SUPPORT && GPIOLIB
>  	select V4L2_FWNODE
>  	help
>  	  This is a Video4Linux2 sensor driver for the OmniVision
> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> index f77320e8a60d..170f80a1a51f 100644
> --- a/drivers/media/i2c/ov2659.c
> +++ b/drivers/media/i2c/ov2659.c
> @@ -22,9 +22,11 @@
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <media/i2c/ov2659.h>
>  #include <media/v4l2-ctrls.h>
> @@ -218,6 +220,11 @@ struct ov2659 {
>  	struct sensor_register *format_ctrl_regs;
>  	struct ov2659_pll_ctrl pll;
>  	int streaming;
> +	/* used to control the sensor PWDN pin */
> +	struct gpio_desc *pwdn_gpio;
> +	/* used to control the sensor RESETB pin */
> +	struct gpio_desc *resetb_gpio;
> +	int on;
>  };
>  
>  static const struct sensor_register ov2659_init_regs[] = {
> @@ -1184,9 +1191,17 @@ static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
>  		/* Stop Streaming Sequence */
>  		ov2659_set_streaming(ov2659, 0);
>  		ov2659->streaming = on;
> +		pm_runtime_put(&client->dev);
>  		goto unlock;
>  	}
>  
> +	ret = pm_runtime_get_sync(&client->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&client->dev);
> +		goto unlock;
> +	}
> +
> +	ov2659_init(sd, 0);
>  	ov2659_set_pixel_clock(ov2659);
>  	ov2659_set_frame_size(ov2659);
>  	ov2659_set_format(ov2659);
> @@ -1243,6 +1258,32 @@ static const char * const ov2659_test_pattern_menu[] = {
>  	"Vertical Color Bars",
>  };
>  
> +static int ov2659_set_power(struct ov2659 *ov2659, int on)
> +{
> +	struct i2c_client *client = ov2659->client;
> +
> +	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
> +
> +	if (on) {
> +		if (ov2659->pwdn_gpio)
> +			gpiod_direction_output(ov2659->pwdn_gpio, 0);
> +
> +		if (ov2659->resetb_gpio) {
> +			gpiod_set_value(ov2659->resetb_gpio, 1);
> +			usleep_range(500, 1000);
> +			gpiod_set_value(ov2659->resetb_gpio, 0);
> +			usleep_range(3000, 5000);
> +		}

Please move the code to the runtime PM callbacks.

> +	} else {
> +		if (ov2659->pwdn_gpio)
> +			gpiod_direction_output(ov2659->pwdn_gpio, 1);

Gpiod API works with NULL GPIOs, too, so no need to check here.

Isn't the direction supposed to be already output, so set_value would be
more appropriate here, and above.

> +	}
> +
> +	ov2659->on = on;
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 subdev internal operations
>   */
> @@ -1323,7 +1364,6 @@ static int ov2659_detect(struct v4l2_subdev *sd)
>  			ret = -ENODEV;
>  		} else {
>  			dev_info(&client->dev, "Found OV%04X sensor\n", id);
> -			ret = ov2659_init(sd, 0);
>  		}
>  	}
>  
> @@ -1400,6 +1440,18 @@ static int ov2659_probe(struct i2c_client *client)
>  	    ov2659->xvclk_frequency > 27000000)
>  		return -EINVAL;
>  
> +	/* Optional gpio don't fail if not present */
> +	ov2659->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> +						    GPIOD_OUT_LOW);
> +	if (IS_ERR(ov2659->pwdn_gpio))
> +		return PTR_ERR(ov2659->pwdn_gpio);
> +
> +	/* Optional gpio don't fail if not present */
> +	ov2659->resetb_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +						      GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov2659->resetb_gpio))
> +		return PTR_ERR(ov2659->resetb_gpio);
> +
>  	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
>  	ov2659->link_frequency =
>  			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> @@ -1445,6 +1497,9 @@ static int ov2659_probe(struct i2c_client *client)
>  	ov2659->frame_size = &ov2659_framesizes[2];
>  	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
>  
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_get_sync(&client->dev);

This makes the driver depend on runtime PM.

See e.g. the smiapp driver for an example how to make it work without. It
wasn't trivial. :I You won't need autosuspend.

> +
>  	ret = ov2659_detect(sd);
>  	if (ret < 0)
>  		goto error;
> @@ -1458,10 +1513,14 @@ static int ov2659_probe(struct i2c_client *client)
>  
>  	dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
>  
> +	pm_runtime_put(&client->dev);
> +
>  	return 0;
>  
>  error:
>  	v4l2_ctrl_handler_free(&ov2659->ctrls);
> +	pm_runtime_put(&client->dev);
> +	pm_runtime_disable(&client->dev);
>  	media_entity_cleanup(&sd->entity);
>  	mutex_destroy(&ov2659->lock);
>  	return ret;
> @@ -1477,9 +1536,35 @@ static int ov2659_remove(struct i2c_client *client)
>  	media_entity_cleanup(&sd->entity);
>  	mutex_destroy(&ov2659->lock);
>  
> +	pm_runtime_disable(&client->dev);
> +
>  	return 0;
>  }
>  
> +static int ov2659_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov2659 *ov2659 = to_ov2659(sd);
> +
> +	ov2659_set_power(ov2659, 0);
> +
> +	return 0;
> +}
> +
> +static int ov2659_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov2659 *ov2659 = to_ov2659(sd);
> +
> +	return ov2659_set_power(ov2659, 1);
> +}
> +
> +static const struct dev_pm_ops ov2659_pm_ops = {
> +	SET_RUNTIME_PM_OPS(ov2659_runtime_suspend, ov2659_runtime_resume, NULL)
> +};
> +
>  static const struct i2c_device_id ov2659_id[] = {
>  	{ "ov2659", 0 },
>  	{ /* sentinel */ },
> @@ -1497,6 +1582,7 @@ MODULE_DEVICE_TABLE(of, ov2659_of_match);
>  static struct i2c_driver ov2659_i2c_driver = {
>  	.driver = {
>  		.name	= DRIVER_NAME,
> +		.pm	= &ov2659_pm_ops,
>  		.of_match_table = of_match_ptr(ov2659_of_match),
>  	},
>  	.probe_new	= ov2659_probe,

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio handling

[Benoit Parrot]

Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Fri [2019-Sep-20 13:17:06 +0300]:
> Hi Benoit,
> 
> Thanks for the update.
> 
> On Thu, Sep 19, 2019 at 03:39:53PM -0500, Benoit Parrot wrote:
> > On some board it is possible that the sensor 'powerdown' and or 'reset'
> > pin might be controlled by gpio instead of being tied.
> > 
> > To implement we add pm_runtime support which will handle the power
> > up/down sequence.
> > 
> > Now originally the driver assumed tat the sensor would always stay
> > powered and there keep its register setting. We cannot assume that this
> > anymore, so every time we "power up" we need to re-program the initial
> > registers configuration first. This was previously done only at probe
> > time.
> > 
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/media/i2c/Kconfig  |  2 +-
> >  drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 88 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 7eee1812bba3..315c1d8bdb7b 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -634,7 +634,7 @@ config VIDEO_OV2640
> >  config VIDEO_OV2659
> >  	tristate "OmniVision OV2659 sensor support"
> >  	depends on VIDEO_V4L2 && I2C
> > -	depends on MEDIA_CAMERA_SUPPORT
> > +	depends on MEDIA_CAMERA_SUPPORT && GPIOLIB
> >  	select V4L2_FWNODE
> >  	help
> >  	  This is a Video4Linux2 sensor driver for the OmniVision
> > diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> > index f77320e8a60d..170f80a1a51f 100644
> > --- a/drivers/media/i2c/ov2659.c
> > +++ b/drivers/media/i2c/ov2659.c
> > @@ -22,9 +22,11 @@
> >  
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> >  #include <linux/of_graph.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #include <media/i2c/ov2659.h>
> >  #include <media/v4l2-ctrls.h>
> > @@ -218,6 +220,11 @@ struct ov2659 {
> >  	struct sensor_register *format_ctrl_regs;
> >  	struct ov2659_pll_ctrl pll;
> >  	int streaming;
> > +	/* used to control the sensor PWDN pin */
> > +	struct gpio_desc *pwdn_gpio;
> > +	/* used to control the sensor RESETB pin */
> > +	struct gpio_desc *resetb_gpio;
> > +	int on;
> >  };
> >  
> >  static const struct sensor_register ov2659_init_regs[] = {
> > @@ -1184,9 +1191,17 @@ static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
> >  		/* Stop Streaming Sequence */
> >  		ov2659_set_streaming(ov2659, 0);
> >  		ov2659->streaming = on;
> > +		pm_runtime_put(&client->dev);
> >  		goto unlock;
> >  	}
> >  
> > +	ret = pm_runtime_get_sync(&client->dev);
> > +	if (ret < 0) {
> > +		pm_runtime_put_noidle(&client->dev);
> > +		goto unlock;
> > +	}
> > +
> > +	ov2659_init(sd, 0);
> >  	ov2659_set_pixel_clock(ov2659);
> >  	ov2659_set_frame_size(ov2659);
> >  	ov2659_set_format(ov2659);
> > @@ -1243,6 +1258,32 @@ static const char * const ov2659_test_pattern_menu[] = {
> >  	"Vertical Color Bars",
> >  };
> >  
> > +static int ov2659_set_power(struct ov2659 *ov2659, int on)
> > +{
> > +	struct i2c_client *client = ov2659->client;
> > +
> > +	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
> > +
> > +	if (on) {
> > +		if (ov2659->pwdn_gpio)
> > +			gpiod_direction_output(ov2659->pwdn_gpio, 0);
> > +
> > +		if (ov2659->resetb_gpio) {
> > +			gpiod_set_value(ov2659->resetb_gpio, 1);
> > +			usleep_range(500, 1000);
> > +			gpiod_set_value(ov2659->resetb_gpio, 0);
> > +			usleep_range(3000, 5000);
> > +		}
> 
> Please move the code to the runtime PM callbacks.
> 
> > +	} else {
> > +		if (ov2659->pwdn_gpio)
> > +			gpiod_direction_output(ov2659->pwdn_gpio, 1);
> 
> Gpiod API works with NULL GPIOs, too, so no need to check here.

Didn't realize that. I'll check.

> 
> Isn't the direction supposed to be already output, so set_value would be
> more appropriate here, and above.

Oh yeah the direction should already be set.

> 
> > +	}
> > +
> > +	ov2659->on = on;
> > +
> > +	return 0;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 subdev internal operations
> >   */
> > @@ -1323,7 +1364,6 @@ static int ov2659_detect(struct v4l2_subdev *sd)
> >  			ret = -ENODEV;
> >  		} else {
> >  			dev_info(&client->dev, "Found OV%04X sensor\n", id);
> > -			ret = ov2659_init(sd, 0);
> >  		}
> >  	}
> >  
> > @@ -1400,6 +1440,18 @@ static int ov2659_probe(struct i2c_client *client)
> >  	    ov2659->xvclk_frequency > 27000000)
> >  		return -EINVAL;
> >  
> > +	/* Optional gpio don't fail if not present */
> > +	ov2659->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > +						    GPIOD_OUT_LOW);
> > +	if (IS_ERR(ov2659->pwdn_gpio))
> > +		return PTR_ERR(ov2659->pwdn_gpio);
> > +
> > +	/* Optional gpio don't fail if not present */
> > +	ov2659->resetb_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > +						      GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ov2659->resetb_gpio))
> > +		return PTR_ERR(ov2659->resetb_gpio);
> > +
> >  	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> >  	ov2659->link_frequency =
> >  			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> > @@ -1445,6 +1497,9 @@ static int ov2659_probe(struct i2c_client *client)
> >  	ov2659->frame_size = &ov2659_framesizes[2];
> >  	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> >  
> > +	pm_runtime_enable(&client->dev);
> > +	pm_runtime_get_sync(&client->dev);
> 
> This makes the driver depend on runtime PM.

Obviously.
Why? Is that bad?

> 
> See e.g. the smiapp driver for an example how to make it work without. It
> wasn't trivial. :I You won't need autosuspend.

I took a look at that driver, but I don't get your reference to being able
to work without runtime pm!
That driver looks pretty similar to ov7740.c which I used as a reference
for this.

> 
> > +
> >  	ret = ov2659_detect(sd);
> >  	if (ret < 0)
> >  		goto error;
> > @@ -1458,10 +1513,14 @@ static int ov2659_probe(struct i2c_client *client)
> >  
> >  	dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
> >  
> > +	pm_runtime_put(&client->dev);
> > +
> >  	return 0;
> >  
> >  error:
> >  	v4l2_ctrl_handler_free(&ov2659->ctrls);
> > +	pm_runtime_put(&client->dev);
> > +	pm_runtime_disable(&client->dev);
> >  	media_entity_cleanup(&sd->entity);
> >  	mutex_destroy(&ov2659->lock);
> >  	return ret;
> > @@ -1477,9 +1536,35 @@ static int ov2659_remove(struct i2c_client *client)
> >  	media_entity_cleanup(&sd->entity);
> >  	mutex_destroy(&ov2659->lock);
> >  
> > +	pm_runtime_disable(&client->dev);
> > +
> >  	return 0;
> >  }
> >  
> > +static int ov2659_runtime_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ov2659 *ov2659 = to_ov2659(sd);
> > +
> > +	ov2659_set_power(ov2659, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ov2659_runtime_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ov2659 *ov2659 = to_ov2659(sd);
> > +
> > +	return ov2659_set_power(ov2659, 1);
> > +}
> > +
> > +static const struct dev_pm_ops ov2659_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(ov2659_runtime_suspend, ov2659_runtime_resume, NULL)
> > +};
> > +
> >  static const struct i2c_device_id ov2659_id[] = {
> >  	{ "ov2659", 0 },
> >  	{ /* sentinel */ },
> > @@ -1497,6 +1582,7 @@ MODULE_DEVICE_TABLE(of, ov2659_of_match);
> >  static struct i2c_driver ov2659_i2c_driver = {
> >  	.driver = {
> >  		.name	= DRIVER_NAME,
> > +		.pm	= &ov2659_pm_ops,
> >  		.of_match_table = of_match_ptr(ov2659_of_match),
> >  	},
> >  	.probe_new	= ov2659_probe,
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com

Re: [Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio handling

[Sakari Ailus]

Hi Benoit,

On Fri, Sep 20, 2019 at 11:55:29AM -0500, Benoit Parrot wrote:
...
> > > @@ -1400,6 +1440,18 @@ static int ov2659_probe(struct i2c_client *client)
> > >  	    ov2659->xvclk_frequency > 27000000)
> > >  		return -EINVAL;
> > >  
> > > +	/* Optional gpio don't fail if not present */
> > > +	ov2659->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > > +						    GPIOD_OUT_LOW);
> > > +	if (IS_ERR(ov2659->pwdn_gpio))
> > > +		return PTR_ERR(ov2659->pwdn_gpio);
> > > +
> > > +	/* Optional gpio don't fail if not present */
> > > +	ov2659->resetb_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > +						      GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(ov2659->resetb_gpio))
> > > +		return PTR_ERR(ov2659->resetb_gpio);
> > > +
> > >  	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> > >  	ov2659->link_frequency =
> > >  			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> > > @@ -1445,6 +1497,9 @@ static int ov2659_probe(struct i2c_client *client)
> > >  	ov2659->frame_size = &ov2659_framesizes[2];
> > >  	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> > >  
> > > +	pm_runtime_enable(&client->dev);
> > > +	pm_runtime_get_sync(&client->dev);
> > 
> > This makes the driver depend on runtime PM.
> 
> Obviously.
> Why? Is that bad?

Well, if it is, then it should be listed in driver's dependencies in
Kconfig.

> 
> > 
> > See e.g. the smiapp driver for an example how to make it work without. It
> > wasn't trivial. :I You won't need autosuspend.
> 
> I took a look at that driver, but I don't get your reference to being able
> to work without runtime pm!

The driver didn't need runtime PM, so it'd be nice to continue work
without.

What smiapp does is that it powers the sensor on first *without* runtime
PM, and then proceeds to set up runtime PM if it's available. The sensor
will only be powered off when the device is unbound with runtime PM
disabled.

Regarding the smiapp driver, you can replace pm_runtime_get_noresume() and
all the autoidle lines with pm_runtime_idle() call after
pm_runtime_enable() in the ov2659 driver.

> That driver looks pretty similar to ov7740.c which I used as a reference
> for this.

I guess in practice many sensor drivers don't work without it on DT-based
systems I'm afraid. :-( They should be fixed.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio handling

[Benoit Parrot]

Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Mon [2019-Sep-23 09:17:32 +0300]:
> Hi Benoit,
> 
> On Fri, Sep 20, 2019 at 11:55:29AM -0500, Benoit Parrot wrote:
> ...
> > > > @@ -1400,6 +1440,18 @@ static int ov2659_probe(struct i2c_client *client)
> > > >  	    ov2659->xvclk_frequency > 27000000)
> > > >  		return -EINVAL;
> > > >  
> > > > +	/* Optional gpio don't fail if not present */
> > > > +	ov2659->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > > > +						    GPIOD_OUT_LOW);
> > > > +	if (IS_ERR(ov2659->pwdn_gpio))
> > > > +		return PTR_ERR(ov2659->pwdn_gpio);
> > > > +
> > > > +	/* Optional gpio don't fail if not present */
> > > > +	ov2659->resetb_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > > +						      GPIOD_OUT_HIGH);
> > > > +	if (IS_ERR(ov2659->resetb_gpio))
> > > > +		return PTR_ERR(ov2659->resetb_gpio);
> > > > +
> > > >  	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> > > >  	ov2659->link_frequency =
> > > >  			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> > > > @@ -1445,6 +1497,9 @@ static int ov2659_probe(struct i2c_client *client)
> > > >  	ov2659->frame_size = &ov2659_framesizes[2];
> > > >  	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> > > >  
> > > > +	pm_runtime_enable(&client->dev);
> > > > +	pm_runtime_get_sync(&client->dev);
> > > 
> > > This makes the driver depend on runtime PM.
> > 
> > Obviously.
> > Why? Is that bad?
> 
> Well, if it is, then it should be listed in driver's dependencies in
> Kconfig.

And I see that most camera driver using pm_runtime calls don't mark it as a
dependency in their own Kconfig. There is actually only one that does.

> 
> > 
> > > 
> > > See e.g. the smiapp driver for an example how to make it work without. It
> > > wasn't trivial. :I You won't need autosuspend.
> > 
> > I took a look at that driver, but I don't get your reference to being able
> > to work without runtime pm!
> 
> The driver didn't need runtime PM, so it'd be nice to continue work
> without.

Ok.

> 
> What smiapp does is that it powers the sensor on first *without* runtime
> PM, and then proceeds to set up runtime PM if it's available. The sensor
> will only be powered off when the device is unbound with runtime PM
> disabled.

In that case in your original reply you suggested that I move the content
of ov2659_set_power() into the pm_runtime callback, but based on this
comments then I should keep ov2659_set_power() as is as I  would need to
use it for this specific purpose.

> 
> Regarding the smiapp driver, you can replace pm_runtime_get_noresume() and
> all the autoidle lines with pm_runtime_idle() call after
> pm_runtime_enable() in the ov2659 driver.

Ok, I'll study this mechanics a little more, as this is not immediately
clear.

Benoit

> 
> > That driver looks pretty similar to ov7740.c which I used as a reference
> > for this.
> 
> I guess in practice many sensor drivers don't work without it on DT-based
> systems I'm afraid. :-( They should be fixed.
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com

Re: [Patch v2 5/7] media: i2c: ov2659: Add powerdown/reset gpio handling

[Sakari Ailus]

On Mon, Sep 23, 2019 at 12:05:57PM -0500, Benoit Parrot wrote:
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Mon [2019-Sep-23 09:17:32 +0300]:
> > Hi Benoit,
> > 
> > On Fri, Sep 20, 2019 at 11:55:29AM -0500, Benoit Parrot wrote:
> > ...
> > > > > @@ -1400,6 +1440,18 @@ static int ov2659_probe(struct i2c_client *client)
> > > > >  	    ov2659->xvclk_frequency > 27000000)
> > > > >  		return -EINVAL;
> > > > >  
> > > > > +	/* Optional gpio don't fail if not present */
> > > > > +	ov2659->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > > > > +						    GPIOD_OUT_LOW);
> > > > > +	if (IS_ERR(ov2659->pwdn_gpio))
> > > > > +		return PTR_ERR(ov2659->pwdn_gpio);
> > > > > +
> > > > > +	/* Optional gpio don't fail if not present */
> > > > > +	ov2659->resetb_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > > > +						      GPIOD_OUT_HIGH);
> > > > > +	if (IS_ERR(ov2659->resetb_gpio))
> > > > > +		return PTR_ERR(ov2659->resetb_gpio);
> > > > > +
> > > > >  	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> > > > >  	ov2659->link_frequency =
> > > > >  			v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> > > > > @@ -1445,6 +1497,9 @@ static int ov2659_probe(struct i2c_client *client)
> > > > >  	ov2659->frame_size = &ov2659_framesizes[2];
> > > > >  	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> > > > >  
> > > > > +	pm_runtime_enable(&client->dev);
> > > > > +	pm_runtime_get_sync(&client->dev);
> > > > 
> > > > This makes the driver depend on runtime PM.
> > > 
> > > Obviously.
> > > Why? Is that bad?
> > 
> > Well, if it is, then it should be listed in driver's dependencies in
> > Kconfig.
> 
> And I see that most camera driver using pm_runtime calls don't mark it as a
> dependency in their own Kconfig. There is actually only one that does.

Drivers should also work without runtime PM even if they can use it.

> 
> > 
> > > 
> > > > 
> > > > See e.g. the smiapp driver for an example how to make it work without. It
> > > > wasn't trivial. :I You won't need autosuspend.
> > > 
> > > I took a look at that driver, but I don't get your reference to being able
> > > to work without runtime pm!
> > 
> > The driver didn't need runtime PM, so it'd be nice to continue work
> > without.
> 
> Ok.
> 
> > 
> > What smiapp does is that it powers the sensor on first *without* runtime
> > PM, and then proceeds to set up runtime PM if it's available. The sensor
> > will only be powered off when the device is unbound with runtime PM
> > disabled.
> 
> In that case in your original reply you suggested that I move the content
> of ov2659_set_power() into the pm_runtime callback, but based on this
> comments then I should keep ov2659_set_power() as is as I  would need to
> use it for this specific purpose.

No need to; you can call the same function elsewhere in the driver (just
like the smiapp driver does).

> 
> > 
> > Regarding the smiapp driver, you can replace pm_runtime_get_noresume() and
> > all the autoidle lines with pm_runtime_idle() call after
> > pm_runtime_enable() in the ov2659 driver.
> 
> Ok, I'll study this mechanics a little more, as this is not immediately
> clear.
> 
> Benoit
> 
> > 
> > > That driver looks pretty similar to ov7740.c which I used as a reference
> > > for this.
> > 
> > I guess in practice many sensor drivers don't work without it on DT-based
> > systems I'm afraid. :-( They should be fixed.
> > 
> > -- 
> > Kind regards,
> > 
> > Sakari Ailus
> > sakari.ailus@linux.intel.com

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