From: Lad Prabhakar <[email protected]>
Hi All,
The main aim of this series is to add Runtime PM support to the sensor
driver alongside some cleanups and fixes.
v2 -> v3
- Included patch#1 [2] as part of this series
- Patched all in-tree DTS for dropping the clock preoperty to
avoid dt warnings
- Fixed review comments pointed by Sakari and Laurent for the Runtime
PM patch
- Now sending the error code of first error while stream off.
- Included RB tags from Laurent
v1-> v2
- patch #1 is infact a v3 [1] no changes
- patch #2 fixed review comments pointed by Sakari
- patch #3 [0] no changes
- patches #4 and #5 are new
[0] https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
[1] https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
[2] https://patchwork.kernel.org/project/linux-media/patch/[email protected]/
Lad Prabhakar (9):
media: i2c: ov5645: Drop fetching the clk reference by name
ARM: dts: imx6qdl-pico: Drop clock-names property
ARM: dts: imx6qdl-wandboard: Drop clock-names property
arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop clock-names
property
media: dt-bindings: ov5645: Convert OV5645 binding to a schema
media: i2c: ov5645: Use runtime PM
media: i2c: ov5645: Drop empty comment
media: i2c: ov5645: Don't return early on failures for s_stream(0)
media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering
the subdev
.../devicetree/bindings/media/i2c/ov5645.txt | 54 ------
.../bindings/media/i2c/ovti,ov5645.yaml | 104 ++++++++++++
arch/arm/boot/dts/imx6qdl-pico.dtsi | 1 -
arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 -
.../aistarvision-mipi-adapter-2.1.dtsi | 1 -
drivers/media/i2c/Kconfig | 2 +-
drivers/media/i2c/ov5645.c | 157 +++++++++---------
7 files changed, 186 insertions(+), 134 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
--
2.25.1
From: Lad Prabhakar <[email protected]>
Convert the simple OV5645 Device Tree binding to json-schema.
The previous binding marked the below properties as required which was a
driver requirement and not the device requirement so just drop them from
the required list during the conversion.
- clock-frequency
- enable-gpios
- reset-gpios
Also drop the "clock-names" property as we have a single clock source for
the sensor and the driver has been updated to drop the clk referencing by
name.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
Resend v3:
* No change
v2 -> v3
* Dropped clock-names property
* Marked power supplies as mandatory
* Dropped the comment for voltage power supplies
* Included RB tag from Laurent
* Driver change to drop clock-names [0]
[0] https://lore.kernel.org/linux-media/Yyh%[email protected]/T/#t
v1 -> v2
* Dropped ref to video-interface-devices.yaml#
* Dropped driver specific required items from the list
* Updated commit message
* Dropped clock-lanes and bus-type from the port and example node
* Marked data-lanes as required in port node
---
.../devicetree/bindings/media/i2c/ov5645.txt | 54 ---------
.../bindings/media/i2c/ovti,ov5645.yaml | 104 ++++++++++++++++++
2 files changed, 104 insertions(+), 54 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
deleted file mode 100644
index 72ad992f77be..000000000000
--- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
+++ /dev/null
@@ -1,54 +0,0 @@
-* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
-
-The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
-an active array size of 2592H x 1944V. It is programmable through a serial I2C
-interface.
-
-Required Properties:
-- compatible: Value should be "ovti,ov5645".
-- clocks: Reference to the xclk clock.
-- clock-names: Should be "xclk".
-- clock-frequency: Frequency of the xclk clock.
-- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
- to the hardware pin PWDNB which is physically active low.
-- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
- the hardware pin RESETB.
-- vdddo-supply: Chip digital IO regulator.
-- vdda-supply: Chip analog regulator.
-- vddd-supply: Chip digital core regulator.
-
-The device node must contain one 'port' child node for its digital output
-video port, in accordance with the video interface bindings defined in
-Documentation/devicetree/bindings/media/video-interfaces.txt.
-
-Example:
-
- &i2c1 {
- ...
-
- ov5645: ov5645@3c {
- compatible = "ovti,ov5645";
- reg = <0x3c>;
-
- enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
- reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
- pinctrl-names = "default";
- pinctrl-0 = <&camera_rear_default>;
-
- clocks = <&clks 200>;
- clock-names = "xclk";
- clock-frequency = <24000000>;
-
- vdddo-supply = <&camera_dovdd_1v8>;
- vdda-supply = <&camera_avdd_2v8>;
- vddd-supply = <&camera_dvdd_1v2>;
-
- port {
- ov5645_ep: endpoint {
- clock-lanes = <1>;
- data-lanes = <0 2>;
- remote-endpoint = <&csi0_ep>;
- };
- };
- };
- };
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
new file mode 100644
index 000000000000..0b10483cd267
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5645.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OmniVision OV5645 Image Sensor Device Tree Bindings
+
+maintainers:
+ - Lad Prabhakar <[email protected]>
+
+properties:
+ compatible:
+ const: ovti,ov5645
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ description: XCLK Input Clock
+
+ clock-frequency:
+ description: Frequency of the xclk clock in Hz.
+
+ vdda-supply:
+ description: Analog voltage supply, 2.8 volts
+
+ vddd-supply:
+ description: Digital core voltage supply, 1.5 volts
+
+ vdddo-supply:
+ description: Digital I/O voltage supply, 1.8 volts
+
+ enable-gpios:
+ maxItems: 1
+ description:
+ Reference to the GPIO connected to the PWDNB pin, if any.
+
+ reset-gpios:
+ maxItems: 1
+ description:
+ Reference to the GPIO connected to the RESETB pin, if any.
+
+ port:
+ description: Digital Output Port
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ additionalProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ data-lanes:
+ minItems: 1
+ maxItems: 2
+ items:
+ enum: [1, 2]
+
+ required:
+ - data-lanes
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - vdddo-supply
+ - vdda-supply
+ - vddd-supply
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ camera@3c {
+ compatible = "ovti,ov5645";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ov5645>;
+ reg = <0x3c>;
+ clocks = <&clks 1>;
+ clock-frequency = <24000000>;
+ vdddo-supply = <&ov5645_vdddo_1v8>;
+ vdda-supply = <&ov5645_vdda_2v8>;
+ vddd-supply = <&ov5645_vddd_1v5>;
+ enable-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
+
+ port {
+ ov5645_ep: endpoint {
+ remote-endpoint = <&csi0_ep>;
+ data-lanes = <1 2>;
+ };
+ };
+ };
+ };
+...
--
2.25.1
From: Lad Prabhakar <[email protected]>
Now that the driver has been updated to drop fetching the clk reference by
name we no longer need the clock-names property in the ov5645 sensor node.
This is in preparation for removal for clock-names property from the DT
binding.
Signed-off-by: Lad Prabhakar <[email protected]>
---
v3
* New patch
---
arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
index ec6fba5ee8fd..e4f63423d8ee 100644
--- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
@@ -131,7 +131,6 @@ camera@3c {
pinctrl-0 = <&pinctrl_ov5645>;
reg = <0x3c>;
clocks = <&clks IMX6QDL_CLK_CKO2>;
- clock-names = "xclk";
clock-frequency = <24000000>;
vdddo-supply = <®_1p8v>;
vdda-supply = <®_2p8v>;
--
2.25.1
From: Lad Prabhakar <[email protected]>
Make sure we dont stop the code flow in case of errors while stopping
the stream and return the error code of the first error case if any.
v4l2-core takes care of warning the user so no need to add a warning
message in the driver.
Suggested-by: Sakari Ailus <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
v2->v3
* Now propagating the first error code in case of failure.
v1->v2
* New patch
---
drivers/media/i2c/ov5645.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index eea3067ddc8b..5702a55607fc 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
if (ret < 0)
goto err_rpm_put;
} else {
+ int stream_off_ret = 0;
+
ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
if (ret < 0)
- return ret;
+ stream_off_ret = ret;
ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
OV5645_SYSTEM_CTRL0_STOP);
- if (ret < 0)
- return ret;
+ if (ret < 0 && !stream_off_ret)
+ stream_off_ret = ret;
pm_runtime_mark_last_busy(ov5645->dev);
pm_runtime_put_autosuspend(ov5645->dev);
+
+ if (stream_off_ret)
+ return stream_off_ret;
}
return 0;
--
2.25.1
From: Lad Prabhakar <[email protected]>
Make sure we call ov5645_entity_init_cfg() before registering the subdev
to make sure default formats are set up.
Suggested-by: Sakari Ailus <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
v2->v3
* Included RB tag from Laurent.
v1->v2
* New patch
---
drivers/media/i2c/ov5645.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 5702a55607fc..73cf6716f3ed 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1223,6 +1223,8 @@ static int ov5645_probe(struct i2c_client *client)
goto err_pm_runtime;
}
+ ov5645_entity_init_cfg(&ov5645->sd, NULL);
+
ret = v4l2_async_register_subdev(&ov5645->sd);
if (ret < 0) {
dev_err(dev, "could not register v4l2 device\n");
@@ -1234,8 +1236,6 @@ static int ov5645_probe(struct i2c_client *client)
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
- ov5645_entity_init_cfg(&ov5645->sd, NULL);
-
return 0;
err_pm_runtime:
--
2.25.1
From: Lad Prabhakar <[email protected]>
Now that the driver has been updated to drop fetching the clk reference by
name we no longer need the clock-names property in the ov5645 sensor node.
This is in preparation for removal for clock-names property from the DT
binding.
Signed-off-by: Lad Prabhakar <[email protected]>
---
v3
* New patch
---
arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi b/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi
index 7ce986f0a06f..7cb5c958aece 100644
--- a/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi
+++ b/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi
@@ -65,7 +65,6 @@ &MIPI_OV5645_PARENT_I2C {
ov5645: ov5645@3c {
compatible = "ovti,ov5645";
reg = <0x3c>;
- clock-names = "xclk";
clocks = <&osc25250_clk>;
clock-frequency = <24000000>;
vdddo-supply = <&ov5645_vdddo_1v8>;
--
2.25.1
From: Lad Prabhakar <[email protected]>
Switch to using runtime PM for power management.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
v2->v3
* Jumped to err_pm_runtime label in case of sd register failure
* Now calling pm_runtime_mark_last_busy() before pm_runtime_put_autosuspend()
call
* Now calling pm_runtime_put_sync() in case s_stream(1) fails
* In s_stream(0) no calling pm_runtime_mark_last_busy() and
pm_runtime_put_autosuspend()
* Included RB tag from Laurent.
v1->v2
* Moved pm_runtime_*_autosuspend() calls after registering the subdev.
---
drivers/media/i2c/Kconfig | 2 +-
drivers/media/i2c/ov5645.c | 141 +++++++++++++++++++------------------
2 files changed, 73 insertions(+), 70 deletions(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 7806d4b81716..c0edd1017fe8 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -459,7 +459,7 @@ config VIDEO_OV5640
config VIDEO_OV5645
tristate "OmniVision OV5645 sensor support"
depends on OF
- depends on I2C && VIDEO_DEV
+ depends on I2C && PM && VIDEO_DEV
select MEDIA_CONTROLLER
select VIDEO_V4L2_SUBDEV_API
select V4L2_FWNODE
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 47451238ca05..4438dc91b82a 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -27,6 +27,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_graph.h>
+#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/types.h>
@@ -108,7 +109,6 @@ struct ov5645 {
u8 timing_tc_reg21;
struct mutex power_lock; /* lock to protect power state */
- int power_count;
struct gpio_desc *enable_gpio;
struct gpio_desc *rst_gpio;
@@ -635,8 +635,24 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
return 0;
}
-static int ov5645_set_power_on(struct ov5645 *ov5645)
+static int ov5645_set_power_off(struct device *dev)
{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ov5645 *ov5645 = to_ov5645(sd);
+
+ ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
+ gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
+ gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
+ clk_disable_unprepare(ov5645->xclk);
+ regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
+
+ return 0;
+}
+
+static int ov5645_set_power_on(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ov5645 *ov5645 = to_ov5645(sd);
int ret;
ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
@@ -658,57 +674,19 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
msleep(20);
- return 0;
-}
-
-static void ov5645_set_power_off(struct ov5645 *ov5645)
-{
- gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
- gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
- clk_disable_unprepare(ov5645->xclk);
- regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
-}
-
-static int ov5645_s_power(struct v4l2_subdev *sd, int on)
-{
- struct ov5645 *ov5645 = to_ov5645(sd);
- int ret = 0;
-
- mutex_lock(&ov5645->power_lock);
-
- /* If the power count is modified from 0 to != 0 or from != 0 to 0,
- * update the power state.
- */
- if (ov5645->power_count == !on) {
- if (on) {
- ret = ov5645_set_power_on(ov5645);
- if (ret < 0)
- goto exit;
-
- ret = ov5645_set_register_array(ov5645,
- ov5645_global_init_setting,
+ ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
ARRAY_SIZE(ov5645_global_init_setting));
- if (ret < 0) {
- dev_err(ov5645->dev,
- "could not set init registers\n");
- ov5645_set_power_off(ov5645);
- goto exit;
- }
-
- usleep_range(500, 1000);
- } else {
- ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
- ov5645_set_power_off(ov5645);
- }
+ if (ret < 0) {
+ dev_err(ov5645->dev, "could not set init registers\n");
+ goto exit;
}
- /* Update the power count. */
- ov5645->power_count += on ? 1 : -1;
- WARN_ON(ov5645->power_count < 0);
+ usleep_range(500, 1000);
-exit:
- mutex_unlock(&ov5645->power_lock);
+ return 0;
+exit:
+ ov5645_set_power_off(dev);
return ret;
}
@@ -795,7 +773,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
int ret;
mutex_lock(&ov5645->power_lock);
- if (!ov5645->power_count) {
+ if (!pm_runtime_get_if_in_use(ov5645->dev)) {
mutex_unlock(&ov5645->power_lock);
return 0;
}
@@ -827,6 +805,8 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
break;
}
+ pm_runtime_mark_last_busy(ov5645->dev);
+ pm_runtime_put_autosuspend(ov5645->dev);
mutex_unlock(&ov5645->power_lock);
return ret;
@@ -991,6 +971,10 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
int ret;
if (enable) {
+ ret = pm_runtime_resume_and_get(ov5645->dev);
+ if (ret < 0)
+ return ret;
+
ret = ov5645_set_register_array(ov5645,
ov5645->current_mode->data,
ov5645->current_mode->data_size);
@@ -998,22 +982,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
dev_err(ov5645->dev, "could not set mode %dx%d\n",
ov5645->current_mode->width,
ov5645->current_mode->height);
- return ret;
+ goto err_rpm_put;
}
ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
if (ret < 0) {
dev_err(ov5645->dev, "could not sync v4l2 controls\n");
- return ret;
+ goto err_rpm_put;
}
ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
if (ret < 0)
- return ret;
+ goto err_rpm_put;
ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
OV5645_SYSTEM_CTRL0_START);
if (ret < 0)
- return ret;
+ goto err_rpm_put;
} else {
ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
if (ret < 0)
@@ -1023,14 +1007,17 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
OV5645_SYSTEM_CTRL0_STOP);
if (ret < 0)
return ret;
+
+ pm_runtime_mark_last_busy(ov5645->dev);
+ pm_runtime_put_autosuspend(ov5645->dev);
}
return 0;
-}
-static const struct v4l2_subdev_core_ops ov5645_core_ops = {
- .s_power = ov5645_s_power,
-};
+err_rpm_put:
+ pm_runtime_put_sync(ov5645->dev);
+ return ret;
+}
static const struct v4l2_subdev_video_ops ov5645_video_ops = {
.s_stream = ov5645_s_stream,
@@ -1046,7 +1033,6 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
};
static const struct v4l2_subdev_ops ov5645_subdev_ops = {
- .core = &ov5645_core_ops,
.video = &ov5645_video_ops,
.pad = &ov5645_subdev_pad_ops,
};
@@ -1188,11 +1174,9 @@ static int ov5645_probe(struct i2c_client *client)
goto free_ctrl;
}
- ret = ov5645_s_power(&ov5645->sd, true);
- if (ret < 0) {
- dev_err(dev, "could not power up OV5645\n");
+ ret = ov5645_set_power_on(dev);
+ if (ret)
goto free_entity;
- }
ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
@@ -1209,12 +1193,16 @@ static int ov5645_probe(struct i2c_client *client)
dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
+ pm_runtime_set_active(dev);
+ pm_runtime_get_noresume(dev);
+ pm_runtime_enable(dev);
+
ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
&ov5645->aec_pk_manual);
if (ret < 0) {
dev_err(dev, "could not read AEC/AGC mode\n");
ret = -ENODEV;
- goto power_down;
+ goto err_pm_runtime;
}
ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
@@ -1222,7 +1210,7 @@ static int ov5645_probe(struct i2c_client *client)
if (ret < 0) {
dev_err(dev, "could not read vflip value\n");
ret = -ENODEV;
- goto power_down;
+ goto err_pm_runtime;
}
ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
@@ -1230,23 +1218,29 @@ static int ov5645_probe(struct i2c_client *client)
if (ret < 0) {
dev_err(dev, "could not read hflip value\n");
ret = -ENODEV;
- goto power_down;
+ goto err_pm_runtime;
}
- ov5645_s_power(&ov5645->sd, false);
-
ret = v4l2_async_register_subdev(&ov5645->sd);
if (ret < 0) {
dev_err(dev, "could not register v4l2 device\n");
- goto free_entity;
+ goto err_pm_runtime;
}
+ pm_runtime_set_autosuspend_delay(dev, 1000);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
ov5645_entity_init_cfg(&ov5645->sd, NULL);
return 0;
+err_pm_runtime:
+ pm_runtime_disable(dev);
+ pm_runtime_put_noidle(dev);
power_down:
- ov5645_s_power(&ov5645->sd, false);
+ ov5645_set_power_off(dev);
free_entity:
media_entity_cleanup(&ov5645->sd.entity);
free_ctrl:
@@ -1264,6 +1258,10 @@ static void ov5645_remove(struct i2c_client *client)
v4l2_async_unregister_subdev(&ov5645->sd);
media_entity_cleanup(&ov5645->sd.entity);
v4l2_ctrl_handler_free(&ov5645->ctrls);
+ pm_runtime_disable(ov5645->dev);
+ if (!pm_runtime_status_suspended(ov5645->dev))
+ ov5645_set_power_off(ov5645->dev);
+ pm_runtime_set_suspended(ov5645->dev);
mutex_destroy(&ov5645->power_lock);
}
@@ -1279,10 +1277,15 @@ static const struct of_device_id ov5645_of_match[] = {
};
MODULE_DEVICE_TABLE(of, ov5645_of_match);
+static const struct dev_pm_ops ov5645_pm_ops = {
+ SET_RUNTIME_PM_OPS(ov5645_set_power_off, ov5645_set_power_on, NULL)
+};
+
static struct i2c_driver ov5645_i2c_driver = {
.driver = {
.of_match_table = ov5645_of_match,
.name = "ov5645",
+ .pm = &ov5645_pm_ops,
},
.probe_new = ov5645_probe,
.remove = ov5645_remove,
--
2.25.1
From: Lad Prabhakar <[email protected]>
Now that the driver has been updated to drop fetching the clk reference by
name we no longer need the clock-names property in the ov5645 sensor node.
This is in preparation for removal for clock-names property from the
DT binding.
Signed-off-by: Lad Prabhakar <[email protected]>
---
v3
* New patch
---
arch/arm/boot/dts/imx6qdl-pico.dtsi | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6qdl-pico.dtsi b/arch/arm/boot/dts/imx6qdl-pico.dtsi
index f7a56d6b160c..c39a9ebdaba1 100644
--- a/arch/arm/boot/dts/imx6qdl-pico.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-pico.dtsi
@@ -233,7 +233,6 @@ camera@3c {
pinctrl-0 = <&pinctrl_ov5645>;
reg = <0x3c>;
clocks = <&clks IMX6QDL_CLK_CKO2>;
- clock-names = "xclk";
clock-frequency = <24000000>;
vdddo-supply = <®_1p8v>;
vdda-supply = <®_2p8v>;
--
2.25.1
On 22-10-26, Lad, Prabhakar wrote:
> Hi Marco,
>
> Thank you for the review.
>
> On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch <[email protected]> wrote:
> >
> > Hi Prabhakar,
> >
> > thanks for the patch, please see below my comments.
> >
> > On 22-10-26, Prabhakar wrote:
> > > From: Lad Prabhakar <[email protected]>
> > >
> > > Make sure we dont stop the code flow in case of errors while stopping
> > > the stream and return the error code of the first error case if any.
> > >
> > > v4l2-core takes care of warning the user so no need to add a warning
> > > message in the driver.
> > >
> > > Suggested-by: Sakari Ailus <[email protected]>
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > ---
> > > v2->v3
> > > * Now propagating the first error code in case of failure.
> > >
> > > v1->v2
> > > * New patch
> > > ---
> > > drivers/media/i2c/ov5645.c | 11 ++++++++---
> > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index eea3067ddc8b..5702a55607fc 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > > if (ret < 0)
> > > goto err_rpm_put;
> > > } else {
> > > + int stream_off_ret = 0;
> > > +
> > > ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> >
> > If this write failed..
> >
> > > if (ret < 0)
> > > - return ret;
> > > + stream_off_ret = ret;
> > >
> > > ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > > OV5645_SYSTEM_CTRL0_STOP);
> >
> > why should this write be successful?
> >
> Indeed that will fail unless I have a lucky day ;-)
>
> But it seemed to be an overkill for adding an additional check to see
> if the previous write succeeded. Do you think this will create an
> issue?
Why not just say?
ret = ov5645_write_reg();
if (ret < 0)
goto out;
...
out:
dev_pm_xxx();
return ret;
Regards,
Marco
Hi Prabhakar,
thanks for the patch, please see below my comments.
On 22-10-26, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> Make sure we dont stop the code flow in case of errors while stopping
> the stream and return the error code of the first error case if any.
>
> v4l2-core takes care of warning the user so no need to add a warning
> message in the driver.
>
> Suggested-by: Sakari Ailus <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> v2->v3
> * Now propagating the first error code in case of failure.
>
> v1->v2
> * New patch
> ---
> drivers/media/i2c/ov5645.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index eea3067ddc8b..5702a55607fc 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> if (ret < 0)
> goto err_rpm_put;
> } else {
> + int stream_off_ret = 0;
> +
> ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
If this write failed..
> if (ret < 0)
> - return ret;
> + stream_off_ret = ret;
>
> ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> OV5645_SYSTEM_CTRL0_STOP);
why should this write be successful?
Regards,
Marco
> - if (ret < 0)
> - return ret;
> + if (ret < 0 && !stream_off_ret)
> + stream_off_ret = ret;
>
> pm_runtime_mark_last_busy(ov5645->dev);
> pm_runtime_put_autosuspend(ov5645->dev);
> +
> + if (stream_off_ret)
> + return stream_off_ret;
> }
>
> return 0;
> --
> 2.25.1
>
>
>
Hi Marco,
Thank you for the review.
On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch <[email protected]> wrote:
>
> Hi Prabhakar,
>
> thanks for the patch, please see below my comments.
>
> On 22-10-26, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Make sure we dont stop the code flow in case of errors while stopping
> > the stream and return the error code of the first error case if any.
> >
> > v4l2-core takes care of warning the user so no need to add a warning
> > message in the driver.
> >
> > Suggested-by: Sakari Ailus <[email protected]>
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > v2->v3
> > * Now propagating the first error code in case of failure.
> >
> > v1->v2
> > * New patch
> > ---
> > drivers/media/i2c/ov5645.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index eea3067ddc8b..5702a55607fc 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > if (ret < 0)
> > goto err_rpm_put;
> > } else {
> > + int stream_off_ret = 0;
> > +
> > ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
>
> If this write failed..
>
> > if (ret < 0)
> > - return ret;
> > + stream_off_ret = ret;
> >
> > ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > OV5645_SYSTEM_CTRL0_STOP);
>
> why should this write be successful?
>
Indeed that will fail unless I have a lucky day ;-)
But it seemed to be an overkill for adding an additional check to see
if the previous write succeeded. Do you think this will create an
issue?
Cheers,
Prabhakar
Hi Marco,
On Wed, Oct 26, 2022 at 6:35 PM Marco Felsch <[email protected]> wrote:
>
> On 22-10-26, Lad, Prabhakar wrote:
> > Hi Marco,
> >
> > Thank you for the review.
> >
> > On Wed, Oct 26, 2022 at 6:17 PM Marco Felsch <[email protected]> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > thanks for the patch, please see below my comments.
> > >
> > > On 22-10-26, Prabhakar wrote:
> > > > From: Lad Prabhakar <[email protected]>
> > > >
> > > > Make sure we dont stop the code flow in case of errors while stopping
> > > > the stream and return the error code of the first error case if any.
> > > >
> > > > v4l2-core takes care of warning the user so no need to add a warning
> > > > message in the driver.
> > > >
> > > > Suggested-by: Sakari Ailus <[email protected]>
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > > ---
> > > > v2->v3
> > > > * Now propagating the first error code in case of failure.
> > > >
> > > > v1->v2
> > > > * New patch
> > > > ---
> > > > drivers/media/i2c/ov5645.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > > index eea3067ddc8b..5702a55607fc 100644
> > > > --- a/drivers/media/i2c/ov5645.c
> > > > +++ b/drivers/media/i2c/ov5645.c
> > > > @@ -996,17 +996,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > > > if (ret < 0)
> > > > goto err_rpm_put;
> > > > } else {
> > > > + int stream_off_ret = 0;
> > > > +
> > > > ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > >
> > > If this write failed..
> > >
> > > > if (ret < 0)
> > > > - return ret;
> > > > + stream_off_ret = ret;
> > > >
> > > > ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > > > OV5645_SYSTEM_CTRL0_STOP);
> > >
> > > why should this write be successful?
> > >
> > Indeed that will fail unless I have a lucky day ;-)
> >
> > But it seemed to be an overkill for adding an additional check to see
> > if the previous write succeeded. Do you think this will create an
> > issue?
>
> Why not just say?
>
> ret = ov5645_write_reg();
> if (ret < 0)
> goto out;
>
> ...
>
> out:
>
> dev_pm_xxx();
>
> return ret;
>
Thanks for the suggestion, I will rework this in the next version.
Cheers,
Prabhakar
On 26/10/2022 09:06, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> Convert the simple OV5645 Device Tree binding to json-schema.
>
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - vdddo-supply
> + - vdda-supply
> + - vddd-supply
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
Use 4 spaces for example indentation.
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + camera@3c {
> + compatible = "ovti,ov5645";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ov5645>;
> + reg = <0x3c>;
reg goes after compatible.
> + clocks = <&clks 1>;
Best regards,
Krzysztof
Hi Prabhakar,
Thanks for the update.
On Wed, Oct 26, 2022 at 02:06:55PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> Switch to using runtime PM for power management.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> v2->v3
> * Jumped to err_pm_runtime label in case of sd register failure
> * Now calling pm_runtime_mark_last_busy() before pm_runtime_put_autosuspend()
> call
> * Now calling pm_runtime_put_sync() in case s_stream(1) fails
> * In s_stream(0) no calling pm_runtime_mark_last_busy() and
> pm_runtime_put_autosuspend()
> * Included RB tag from Laurent.
>
> v1->v2
> * Moved pm_runtime_*_autosuspend() calls after registering the subdev.
> ---
> drivers/media/i2c/Kconfig | 2 +-
> drivers/media/i2c/ov5645.c | 141 +++++++++++++++++++------------------
> 2 files changed, 73 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 7806d4b81716..c0edd1017fe8 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -459,7 +459,7 @@ config VIDEO_OV5640
> config VIDEO_OV5645
> tristate "OmniVision OV5645 sensor support"
> depends on OF
> - depends on I2C && VIDEO_DEV
> + depends on I2C && PM && VIDEO_DEV
I think you can drop the PM dependency --- the driver will work fine
without CONFIG_PM.
Although one could question why do we have CONFIG_PM. Some systems won't
boot without it and who would want to consume more power than necessary?
Could this be removed altogether? Or perhaps we could add CONFIG_PM
dependency to V4L2 and DVB? :-)
Certainly out of scope of this set though.
--
Kind regards,
Sakari Ailus
Hi Sakari,
On Thu, Oct 27, 2022 at 10:22 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> Thanks for the update.
>
> On Wed, Oct 26, 2022 at 02:06:55PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Switch to using runtime PM for power management.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > ---
> > v2->v3
> > * Jumped to err_pm_runtime label in case of sd register failure
> > * Now calling pm_runtime_mark_last_busy() before pm_runtime_put_autosuspend()
> > call
> > * Now calling pm_runtime_put_sync() in case s_stream(1) fails
> > * In s_stream(0) no calling pm_runtime_mark_last_busy() and
> > pm_runtime_put_autosuspend()
> > * Included RB tag from Laurent.
> >
> > v1->v2
> > * Moved pm_runtime_*_autosuspend() calls after registering the subdev.
> > ---
> > drivers/media/i2c/Kconfig | 2 +-
> > drivers/media/i2c/ov5645.c | 141 +++++++++++++++++++------------------
> > 2 files changed, 73 insertions(+), 70 deletions(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 7806d4b81716..c0edd1017fe8 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -459,7 +459,7 @@ config VIDEO_OV5640
> > config VIDEO_OV5645
> > tristate "OmniVision OV5645 sensor support"
> > depends on OF
> > - depends on I2C && VIDEO_DEV
> > + depends on I2C && PM && VIDEO_DEV
>
> I think you can drop the PM dependency --- the driver will work fine
> without CONFIG_PM.
>
Agreed, I'll send a new version dropping this and fixing the comments
on patch #5 and patch #8.
> Although one could question why do we have CONFIG_PM. Some systems won't
> boot without it and who would want to consume more power than necessary?
>
> Could this be removed altogether? Or perhaps we could add CONFIG_PM
> dependency to V4L2 and DVB? :-)
>
Or rather this option should be selected by the platform itself rather
than subsystem?
Cheers,
Prabhakar
Hello,
On Wed, Oct 26, 2022 at 02:06:49PM +0100, Prabhakar wrote:
> ARM: dts: imx6qdl-pico: Drop clock-names property
> ARM: dts: imx6qdl-wandboard: Drop clock-names property
> arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop clock-names
> property
Are Freescale/Renesas arch maintainers fine with me taking these patches
or should they be merged through another tree?
Thanks.
--
Kind regards,
Sakari Ailus
On Wed, Oct 26, 2022 at 3:07 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> Now that the driver has been updated to drop fetching the clk reference by
> name we no longer need the clock-names property in the ov5645 sensor node.
>
> This is in preparation for removal for clock-names property from the DT
> binding.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> v3
> * New patch
Reviewed-by: Geert Uytterhoeven <[email protected]>
Acked-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Oct 27, 2022 at 09:32:20AM +0000, Sakari Ailus wrote:
> Hello,
>
> On Wed, Oct 26, 2022 at 02:06:49PM +0100, Prabhakar wrote:
> > ARM: dts: imx6qdl-pico: Drop clock-names property
> > ARM: dts: imx6qdl-wandboard: Drop clock-names property
> > arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop clock-names
> > property
>
> Are Freescale/Renesas arch maintainers fine with me taking these patches
> or should they be merged through another tree?
Go ahead to take i.MX DTS patches:
Acked-by: Shawn Guo <[email protected]>
On Sat, Oct 29, 2022 at 08:54:12PM +0800, Shawn Guo wrote:
> On Thu, Oct 27, 2022 at 09:32:20AM +0000, Sakari Ailus wrote:
> > Hello,
> >
> > On Wed, Oct 26, 2022 at 02:06:49PM +0100, Prabhakar wrote:
> > > ARM: dts: imx6qdl-pico: Drop clock-names property
> > > ARM: dts: imx6qdl-wandboard: Drop clock-names property
> > > arm64: dts: renesas: aistarvision-mipi-adapter-2.1: Drop clock-names
> > > property
> >
> > Are Freescale/Renesas arch maintainers fine with me taking these patches
> > or should they be merged through another tree?
>
> Go ahead to take i.MX DTS patches:
>
> Acked-by: Shawn Guo <[email protected]>
Thanks, Shawn and Geert!
--
Sakari Ailus