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.
Also added a patch to check and propagate errors on s_stream invocation.
Finally, we update the licensing statement to use SPDX licensing.
Changes since v3:
- Addressed review comment from Sakari about s_ctrl i2c access whne
powered off
Changes since v2:
- Addressed review comment from Sakari
- Reworked the "media: i2c: ov2659: Add powerdown/reset gpio handling"
to use pm_runtime if it is available but not to make it depend on it.
- Cleaned up the gpio related calls to remove the unnecessary NULL check
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 (8):
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: i2c: ov2659: fix s_stream return value
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 | 138 +++++++++++++-----
3 files changed, 112 insertions(+), 37 deletions(-)
--
2.17.1
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 <[email protected]>
Signed-off-by: Jyri Sarha <[email protected]>
---
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
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 <[email protected]>
---
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 7d0baa386644..720310e0725d 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
Add powerdown-gpios and reset-gpios to the list of optional properties
for the OV2659 camera sensor.
Signed-off-by: Benoit Parrot <[email protected]>
---
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
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 <[email protected]>
---
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
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 when it is available otherwise the sensor will be
powered on at module insertion/probe and powered off at module removal.
Now originally the driver assumed that the sensor would always stay
powered and keep its register setting. We cannot assume 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 <[email protected]>
---
drivers/media/i2c/Kconfig | 2 +-
drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
2 files changed, 87 insertions(+), 3 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 cd4625432264..7d0baa386644 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,10 +1191,19 @@ 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 = ov2659_set_pixel_clock(ov2659);
+ ret = pm_runtime_get_sync(&client->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&client->dev);
+ goto unlock;
+ }
+
+ ret = ov2659_init(sd, 0);
+ if (!ret)
+ ret = ov2659_set_pixel_clock(ov2659);
if (!ret)
ret = ov2659_set_frame_size(ov2659);
if (!ret)
@@ -1229,6 +1245,9 @@ static int ov2659_s_ctrl(struct v4l2_ctrl *ctrl)
struct ov2659 *ov2659 =
container_of(ctrl->handler, struct ov2659, ctrls);
+ if (!ov2659->on)
+ return 0;
+
switch (ctrl->id) {
case V4L2_CID_TEST_PATTERN:
return ov2659_set_test_pattern(ov2659, ctrl->val);
@@ -1246,6 +1265,43 @@ static const char * const ov2659_test_pattern_menu[] = {
"Vertical Color Bars",
};
+static int ov2659_power_off(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);
+
+ dev_dbg(&client->dev, "%s:\n", __func__);
+
+ gpiod_set_value(ov2659->pwdn_gpio, 1);
+
+ ov2659->on = false;
+
+ return 0;
+}
+
+static int ov2659_power_on(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);
+
+ dev_dbg(&client->dev, "%s:\n", __func__);
+
+ gpiod_set_value(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);
+ }
+
+ ov2659->on = true;
+
+ return 0;
+}
+
/* -----------------------------------------------------------------------------
* V4L2 subdev internal operations
*/
@@ -1326,7 +1382,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);
}
}
@@ -1403,6 +1458,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,
@@ -1448,6 +1515,8 @@ static int ov2659_probe(struct i2c_client *client)
ov2659->frame_size = &ov2659_framesizes[2];
ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
+ ov2659_power_on(&client->dev);
+
ret = ov2659_detect(sd);
if (ret < 0)
goto error;
@@ -1461,10 +1530,15 @@ static int ov2659_probe(struct i2c_client *client)
dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
+ pm_runtime_set_active(&client->dev);
+ pm_runtime_enable(&client->dev);
+ pm_runtime_idle(&client->dev);
+
return 0;
error:
v4l2_ctrl_handler_free(&ov2659->ctrls);
+ ov2659_power_off(&client->dev);
media_entity_cleanup(&sd->entity);
mutex_destroy(&ov2659->lock);
return ret;
@@ -1480,9 +1554,18 @@ static int ov2659_remove(struct i2c_client *client)
media_entity_cleanup(&sd->entity);
mutex_destroy(&ov2659->lock);
+ pm_runtime_disable(&client->dev);
+ if (!pm_runtime_status_suspended(&client->dev))
+ ov2659_power_off(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+
return 0;
}
+static const struct dev_pm_ops ov2659_pm_ops = {
+ SET_RUNTIME_PM_OPS(ov2659_power_off, ov2659_power_on, NULL)
+};
+
static const struct i2c_device_id ov2659_id[] = {
{ "ov2659", 0 },
{ /* sentinel */ },
@@ -1500,6 +1583,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
On Fri, Sep 27, 2019 at 7:47 PM Benoit Parrot <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> drivers/media/i2c/ov2659.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
Acked-by: Lad, Prabhakar <[email protected]>
Cheers,
--Prabhakar Lad
> 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
>
On Fri, Sep 27, 2019 at 7:47 PM Benoit Parrot <[email protected]> wrote:
>
> 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 <[email protected]>
> Signed-off-by: Jyri Sarha <[email protected]>
> ---
> drivers/media/i2c/ov2659.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
Acked-by: Lad, Prabhakar <[email protected]>
Cheers,
--Prabhakar Lad
> 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
>
On Fri, Sep 27, 2019 at 7:47 PM Benoit Parrot <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> drivers/media/i2c/ov2659.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
Acked-by: Lad, Prabhakar <[email protected]>
Cheers,
--Prabhakar Lad
> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> index 7d0baa386644..720310e0725d 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
>
On Fri, Sep 27, 2019 at 7:47 PM Benoit Parrot <[email protected]> wrote:
>
> Add powerdown-gpios and reset-gpios to the list of optional properties
> for the OV2659 camera sensor.
>
> Signed-off-by: Benoit Parrot <[email protected]>
> ---
> Documentation/devicetree/bindings/media/i2c/ov2659.txt | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
Acked-by: Lad, Prabhakar <[email protected]>
Cheers,
--Prabhakar Lad
> 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
>
Hi Benoit,
thank you for the patch.
On Fri, Sep 27, 2019 at 7:49 PM Benoit Parrot <[email protected]> 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 when it is available otherwise the sensor will be
> powered on at module insertion/probe and powered off at module removal.
>
> Now originally the driver assumed that the sensor would always stay
> powered and keep its register setting. We cannot assume 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 <[email protected]>
> ---
> drivers/media/i2c/Kconfig | 2 +-
> drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
> 2 files changed, 87 insertions(+), 3 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 cd4625432264..7d0baa386644 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,10 +1191,19 @@ 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 = ov2659_set_pixel_clock(ov2659);
> + ret = pm_runtime_get_sync(&client->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(&client->dev);
> + goto unlock;
> + }
> +
> + ret = ov2659_init(sd, 0);
> + if (!ret)
> + ret = ov2659_set_pixel_clock(ov2659);
> if (!ret)
> ret = ov2659_set_frame_size(ov2659);
> if (!ret)
> @@ -1229,6 +1245,9 @@ static int ov2659_s_ctrl(struct v4l2_ctrl *ctrl)
> struct ov2659 *ov2659 =
> container_of(ctrl->handler, struct ov2659, ctrls);
>
> + if (!ov2659->on)
> + return 0;
> +
why is this check only in s_ctrl() ? I think we could just drop this variable.
Cheers,
--Prabhakar
> switch (ctrl->id) {
> case V4L2_CID_TEST_PATTERN:
> return ov2659_set_test_pattern(ov2659, ctrl->val);
> @@ -1246,6 +1265,43 @@ static const char * const ov2659_test_pattern_menu[] = {
> "Vertical Color Bars",
> };
>
> +static int ov2659_power_off(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);
> +
> + dev_dbg(&client->dev, "%s:\n", __func__);
> +
> + gpiod_set_value(ov2659->pwdn_gpio, 1);
> +
> + ov2659->on = false;
> +
> + return 0;
> +}
> +
> +static int ov2659_power_on(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);
> +
> + dev_dbg(&client->dev, "%s:\n", __func__);
> +
> + gpiod_set_value(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);
> + }
> +
> + ov2659->on = true;
> +
> + return 0;
> +}
> +
> /* -----------------------------------------------------------------------------
> * V4L2 subdev internal operations
> */
> @@ -1326,7 +1382,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);
> }
> }
>
> @@ -1403,6 +1458,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,
> @@ -1448,6 +1515,8 @@ static int ov2659_probe(struct i2c_client *client)
> ov2659->frame_size = &ov2659_framesizes[2];
> ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
>
> + ov2659_power_on(&client->dev);
> +
> ret = ov2659_detect(sd);
> if (ret < 0)
> goto error;
> @@ -1461,10 +1530,15 @@ static int ov2659_probe(struct i2c_client *client)
>
> dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
>
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_enable(&client->dev);
> + pm_runtime_idle(&client->dev);
> +
> return 0;
>
> error:
> v4l2_ctrl_handler_free(&ov2659->ctrls);
> + ov2659_power_off(&client->dev);
> media_entity_cleanup(&sd->entity);
> mutex_destroy(&ov2659->lock);
> return ret;
> @@ -1480,9 +1554,18 @@ static int ov2659_remove(struct i2c_client *client)
> media_entity_cleanup(&sd->entity);
> mutex_destroy(&ov2659->lock);
>
> + pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev))
> + ov2659_power_off(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> +
> return 0;
> }
>
> +static const struct dev_pm_ops ov2659_pm_ops = {
> + SET_RUNTIME_PM_OPS(ov2659_power_off, ov2659_power_on, NULL)
> +};
> +
> static const struct i2c_device_id ov2659_id[] = {
> { "ov2659", 0 },
> { /* sentinel */ },
> @@ -1500,6 +1583,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
>
Hi Benoit,
On Fri, Sep 27, 2019 at 01:47:20PM -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 when it is available otherwise the sensor will be
> powered on at module insertion/probe and powered off at module removal.
>
> Now originally the driver assumed that the sensor would always stay
> powered and keep its register setting. We cannot assume 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 <[email protected]>
> ---
> drivers/media/i2c/Kconfig | 2 +-
> drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
> 2 files changed, 87 insertions(+), 3 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 cd4625432264..7d0baa386644 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;
Please use runtime PM for this instead. It's hard to get this right
otherwise.
Access to "on" is not serialised with the power state changes. In this case
e.g. drivers/media/i2c/ov5670.c is a good example. I think I'll see how I
could improve smiapp as well.
--
Sakari Ailus
[email protected]
Lad, Prabhakar <[email protected]> wrote on Fri [2019-Sep-27 22:09:45 +0100]:
> Hi Benoit,
>
> thank you for the patch.
>
> On Fri, Sep 27, 2019 at 7:49 PM Benoit Parrot <[email protected]> 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 when it is available otherwise the sensor will be
> > powered on at module insertion/probe and powered off at module removal.
> >
> > Now originally the driver assumed that the sensor would always stay
> > powered and keep its register setting. We cannot assume 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 <[email protected]>
> > ---
> > drivers/media/i2c/Kconfig | 2 +-
> > drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 87 insertions(+), 3 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 cd4625432264..7d0baa386644 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,10 +1191,19 @@ 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 = ov2659_set_pixel_clock(ov2659);
> > + ret = pm_runtime_get_sync(&client->dev);
> > + if (ret < 0) {
> > + pm_runtime_put_noidle(&client->dev);
> > + goto unlock;
> > + }
> > +
> > + ret = ov2659_init(sd, 0);
> > + if (!ret)
> > + ret = ov2659_set_pixel_clock(ov2659);
> > if (!ret)
> > ret = ov2659_set_frame_size(ov2659);
> > if (!ret)
> > @@ -1229,6 +1245,9 @@ static int ov2659_s_ctrl(struct v4l2_ctrl *ctrl)
> > struct ov2659 *ov2659 =
> > container_of(ctrl->handler, struct ov2659, ctrls);
> >
> > + if (!ov2659->on)
> > + return 0;
> > +
> why is this check only in s_ctrl() ? I think we could just drop this variable.
Well, it is the only place which would yield i2c access without
explicitly turning on power, so we need to make sure it is already on.
See Sakari's comment in the previous iteration.
Benoit
>
> Cheers,
> --Prabhakar
>
> > switch (ctrl->id) {
> > case V4L2_CID_TEST_PATTERN:
> > return ov2659_set_test_pattern(ov2659, ctrl->val);
> > @@ -1246,6 +1265,43 @@ static const char * const ov2659_test_pattern_menu[] = {
> > "Vertical Color Bars",
> > };
> >
> > +static int ov2659_power_off(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);
> > +
> > + dev_dbg(&client->dev, "%s:\n", __func__);
> > +
> > + gpiod_set_value(ov2659->pwdn_gpio, 1);
> > +
> > + ov2659->on = false;
> > +
> > + return 0;
> > +}
> > +
> > +static int ov2659_power_on(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);
> > +
> > + dev_dbg(&client->dev, "%s:\n", __func__);
> > +
> > + gpiod_set_value(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);
> > + }
> > +
> > + ov2659->on = true;
> > +
> > + return 0;
> > +}
> > +
> > /* -----------------------------------------------------------------------------
> > * V4L2 subdev internal operations
> > */
> > @@ -1326,7 +1382,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);
> > }
> > }
> >
> > @@ -1403,6 +1458,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,
> > @@ -1448,6 +1515,8 @@ static int ov2659_probe(struct i2c_client *client)
> > ov2659->frame_size = &ov2659_framesizes[2];
> > ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> >
> > + ov2659_power_on(&client->dev);
> > +
> > ret = ov2659_detect(sd);
> > if (ret < 0)
> > goto error;
> > @@ -1461,10 +1530,15 @@ static int ov2659_probe(struct i2c_client *client)
> >
> > dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
> >
> > + pm_runtime_set_active(&client->dev);
> > + pm_runtime_enable(&client->dev);
> > + pm_runtime_idle(&client->dev);
> > +
> > return 0;
> >
> > error:
> > v4l2_ctrl_handler_free(&ov2659->ctrls);
> > + ov2659_power_off(&client->dev);
> > media_entity_cleanup(&sd->entity);
> > mutex_destroy(&ov2659->lock);
> > return ret;
> > @@ -1480,9 +1554,18 @@ static int ov2659_remove(struct i2c_client *client)
> > media_entity_cleanup(&sd->entity);
> > mutex_destroy(&ov2659->lock);
> >
> > + pm_runtime_disable(&client->dev);
> > + if (!pm_runtime_status_suspended(&client->dev))
> > + ov2659_power_off(&client->dev);
> > + pm_runtime_set_suspended(&client->dev);
> > +
> > return 0;
> > }
> >
> > +static const struct dev_pm_ops ov2659_pm_ops = {
> > + SET_RUNTIME_PM_OPS(ov2659_power_off, ov2659_power_on, NULL)
> > +};
> > +
> > static const struct i2c_device_id ov2659_id[] = {
> > { "ov2659", 0 },
> > { /* sentinel */ },
> > @@ -1500,6 +1583,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
> >
Sakari Ailus <[email protected]> wrote on Mon [2019-Sep-30 10:28:57 +0300]:
> Hi Benoit,
>
> On Fri, Sep 27, 2019 at 01:47:20PM -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 when it is available otherwise the sensor will be
> > powered on at module insertion/probe and powered off at module removal.
> >
> > Now originally the driver assumed that the sensor would always stay
> > powered and keep its register setting. We cannot assume 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 <[email protected]>
> > ---
> > drivers/media/i2c/Kconfig | 2 +-
> > drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 87 insertions(+), 3 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 cd4625432264..7d0baa386644 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;
>
> Please use runtime PM for this instead. It's hard to get this right
> otherwise.
So you mean, I should use the "if (!pm_runtime_get_if_in_use(dev))"
construct?
Ok I'll switch it to that, just using "on" was a little simple but I
already had it :).
>
> Access to "on" is not serialised with the power state changes. In this case
> e.g. drivers/media/i2c/ov5670.c is a good example. I think I'll see how I
> could improve smiapp as well.
Ok so that driver uses this pm method but does not actually power up or
down the sensor.
Benoit
>
> --
> Sakari Ailus
> [email protected]
On Mon, Sep 30, 2019 at 07:29:46AM -0500, Benoit Parrot wrote:
> Sakari Ailus <[email protected]> wrote on Mon [2019-Sep-30 10:28:57 +0300]:
> > Hi Benoit,
> >
> > On Fri, Sep 27, 2019 at 01:47:20PM -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 when it is available otherwise the sensor will be
> > > powered on at module insertion/probe and powered off at module removal.
> > >
> > > Now originally the driver assumed that the sensor would always stay
> > > powered and keep its register setting. We cannot assume 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 <[email protected]>
> > > ---
> > > drivers/media/i2c/Kconfig | 2 +-
> > > drivers/media/i2c/ov2659.c | 88 +++++++++++++++++++++++++++++++++++++-
> > > 2 files changed, 87 insertions(+), 3 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 cd4625432264..7d0baa386644 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;
> >
> > Please use runtime PM for this instead. It's hard to get this right
> > otherwise.
>
> So you mean, I should use the "if (!pm_runtime_get_if_in_use(dev))"
> construct?
Yes, please.
>
> Ok I'll switch it to that, just using "on" was a little simple but I
> already had it :).
>
>
> >
> > Access to "on" is not serialised with the power state changes. In this case
> > e.g. drivers/media/i2c/ov5670.c is a good example. I think I'll see how I
> > could improve smiapp as well.
>
> Ok so that driver uses this pm method but does not actually power up or
> down the sensor.
Well, not explicitly. The driver supports ACPI based systems only right
now.
--
Sakari Ailus
[email protected]