[PATCH v2 0/7] media: ov5693: cleanup code and add dts support

[PATCH v2 0/7] media: ov5693: cleanup code and add dts support

[Tommaso Merciai]

Hi All,
This series cleanup code on ov5693 driver and bring up dts support, also add
documentation for ov5693 camera sensor

Inspired by recently Quentin series:

 - https://patchwork.kernel.org/project/linux-media/list/?series=64807

Tommaso Merciai (7):
  media: ov5693: count num_supplies using array_size
  media: ov5693: add dvdd into ov5693_supply_names array
  media: ov5693: rename clk into xvclk
  media: ov5693: move hw cfg functions into ov5693_check_hwcfg
  media: ov5693: rename ov5693_check_hwcfg into ov5693_get_hwcfg
  media: ov5693: add ov5693_of_match, dts support
  media: dt-bindings: ov5693: document YAML binding

 .../bindings/media/i2c/ovti,ov5693.yaml       | 108 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov5693.c                    |  86 ++++++++------
 3 files changed, 161 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml

-- 
2.25.1

[PATCH v2 1/7] media: ov5693: count num_supplies using array_size

[Tommaso Merciai]

Instead of hardcode OV5693_NUM_SUPPLIES in a define is better use
ARRAY_SIZE function to count the number of supplies from
ov5693_supply_names array

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 drivers/media/i2c/ov5693.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 117ff5403312..f410333c4c67 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -127,11 +127,15 @@
 #define OV5693_LINK_FREQ_419_2MHZ		419200000
 #define OV5693_PIXEL_RATE			167680000
 
-/* Miscellaneous */
-#define OV5693_NUM_SUPPLIES			2
-
 #define to_ov5693_sensor(x) container_of(x, struct ov5693_device, sd)
 
+static const char * const ov5693_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dovdd",	/* Digital I/O power */
+};
+
+#define OV5693_NUM_SUPPLIES	ARRAY_SIZE(ov5693_supply_names)
+
 struct ov5693_reg {
 	u32 reg;
 	u8 val;
@@ -352,11 +356,6 @@ static const s64 link_freq_menu_items[] = {
 	OV5693_LINK_FREQ_419_2MHZ
 };
 
-static const char * const ov5693_supply_names[] = {
-	"avdd",
-	"dovdd",
-};
-
 static const char * const ov5693_test_pattern_menu[] = {
 	"Disabled",
 	"Random Data",
-- 
2.25.1

[PATCH v2 2/7] media: ov5693: add dvdd into ov5693_supply_names array

[Tommaso Merciai]

Add missing "dvdd" (Digital circuit power) entry into ov5693_supply_names
array

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 drivers/media/i2c/ov5693.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index f410333c4c67..9e84468d920e 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -132,6 +132,7 @@
 static const char * const ov5693_supply_names[] = {
 	"avdd",		/* Analog power */
 	"dovdd",	/* Digital I/O power */
+	"dvdd",		/* Digital circuit power */
 };
 
 #define OV5693_NUM_SUPPLIES	ARRAY_SIZE(ov5693_supply_names)
-- 
2.25.1

[PATCH v2 3/7] media: ov5693: rename clk into xvclk

[Tommaso Merciai]

Rename clk pdata pointer into xvclk (system clock input).
Same for clk_rate into xvclk_rate. This is more explicit

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 drivers/media/i2c/ov5693.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 9e84468d920e..d2adc5513a21 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -157,7 +157,7 @@ struct ov5693_device {
 	struct gpio_desc *reset;
 	struct gpio_desc *powerdown;
 	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
-	struct clk *clk;
+	struct clk *xvclk;
 
 	struct ov5693_mode {
 		struct v4l2_rect crop;
@@ -794,7 +794,7 @@ static void ov5693_sensor_powerdown(struct ov5693_device *ov5693)
 
 	regulator_bulk_disable(OV5693_NUM_SUPPLIES, ov5693->supplies);
 
-	clk_disable_unprepare(ov5693->clk);
+	clk_disable_unprepare(ov5693->xvclk);
 }
 
 static int ov5693_sensor_powerup(struct ov5693_device *ov5693)
@@ -804,7 +804,7 @@ static int ov5693_sensor_powerup(struct ov5693_device *ov5693)
 	gpiod_set_value_cansleep(ov5693->reset, 1);
 	gpiod_set_value_cansleep(ov5693->powerdown, 1);
 
-	ret = clk_prepare_enable(ov5693->clk);
+	ret = clk_prepare_enable(ov5693->xvclk);
 	if (ret) {
 		dev_err(ov5693->dev, "Failed to enable clk\n");
 		goto fail_power;
@@ -1390,7 +1390,7 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
 static int ov5693_probe(struct i2c_client *client)
 {
 	struct ov5693_device *ov5693;
-	u32 clk_rate;
+	u32 xvclk_rate;
 	int ret = 0;
 
 	ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
@@ -1408,16 +1408,16 @@ static int ov5693_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
 
-	ov5693->clk = devm_clk_get(&client->dev, "xvclk");
-	if (IS_ERR(ov5693->clk)) {
+	ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
+	if (IS_ERR(ov5693->xvclk)) {
 		dev_err(&client->dev, "Error getting clock\n");
-		return PTR_ERR(ov5693->clk);
+		return PTR_ERR(ov5693->xvclk);
 	}
 
-	clk_rate = clk_get_rate(ov5693->clk);
-	if (clk_rate != OV5693_XVCLK_FREQ)
+	xvclk_rate = clk_get_rate(ov5693->xvclk);
+	if (xvclk_rate != OV5693_XVCLK_FREQ)
 		dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
-			 clk_rate, OV5693_XVCLK_FREQ);
+			 xvclk_rate, OV5693_XVCLK_FREQ);
 
 	ret = ov5693_configure_gpios(ov5693);
 	if (ret)
-- 
2.25.1

[PATCH v2 4/7] media: ov5693: move hw cfg functions into ov5693_check_hwcfg

[Tommaso Merciai]

Move hw configuration functions into ov5693_check_hwcfg. This is done to
separe the code that handle the hw cfg from probe in a clean way

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index d2adc5513a21..d5a934ace597 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
 	struct fwnode_handle *endpoint;
 	unsigned int i;
 	int ret;
+	u32 xvclk_rate;
+
+	ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");
+	if (IS_ERR(ov5693->xvclk))
+		return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
+				     "failed to get xvclk: %ld\n",
+				     PTR_ERR(ov5693->xvclk));
+
+	if (ov5693->xvclk) {
+		xvclk_rate = clk_get_rate(ov5693->xvclk);
+	} else {
+		ret = fwnode_property_read_u32(fwnode, "clock-frequency",
+					       &xvclk_rate);
+
+		if (ret) {
+			dev_err(ov5693->dev, "can't get clock frequency");
+			return ret;
+		}
+	}
+
+	if (xvclk_rate != OV5693_XVCLK_FREQ)
+		dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
+			 xvclk_rate, OV5693_XVCLK_FREQ);
+
+	ret = ov5693_configure_gpios(ov5693);
+	if (ret)
+		return ret;
+
+	ret = ov5693_get_regulators(ov5693);
+	if (ret)
+		return dev_err_probe(ov5693->dev, ret,
+				     "Error fetching regulators\n");
 
 	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!endpoint)
@@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
 static int ov5693_probe(struct i2c_client *client)
 {
 	struct ov5693_device *ov5693;
-	u32 xvclk_rate;
 	int ret = 0;
 
 	ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
@@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
 
-	ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
-	if (IS_ERR(ov5693->xvclk)) {
-		dev_err(&client->dev, "Error getting clock\n");
-		return PTR_ERR(ov5693->xvclk);
-	}
-
-	xvclk_rate = clk_get_rate(ov5693->xvclk);
-	if (xvclk_rate != OV5693_XVCLK_FREQ)
-		dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
-			 xvclk_rate, OV5693_XVCLK_FREQ);
-
-	ret = ov5693_configure_gpios(ov5693);
-	if (ret)
-		return ret;
-
-	ret = ov5693_get_regulators(ov5693);
-	if (ret)
-		return dev_err_probe(&client->dev, ret,
-				     "Error fetching regulators\n");
-
 	ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-- 
2.25.1

[PATCH v2 5/7] media: ov5693: rename ov5693_check_hwcfg into ov5693_get_hwcfg

[Tommaso Merciai]

Function ov5693_check_hwcfg don't only check the hw configuration but
also get the hw configuration. Is better better to rename this into ov5693_get_hwcfg

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 drivers/media/i2c/ov5693.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index d5a934ace597..273caef467fe 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1339,7 +1339,7 @@ static int ov5693_get_regulators(struct ov5693_device *ov5693)
 				       ov5693->supplies);
 }
 
-static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
+static int ov5693_get_hwcfg(struct ov5693_device *ov5693)
 {
 	struct fwnode_handle *fwnode = dev_fwnode(ov5693->dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
@@ -1431,7 +1431,7 @@ static int ov5693_probe(struct i2c_client *client)
 	ov5693->client = client;
 	ov5693->dev = &client->dev;
 
-	ret = ov5693_check_hwcfg(ov5693);
+	ret = ov5693_get_hwcfg(ov5693);
 	if (ret)
 		return ret;
 
-- 
2.25.1

[PATCH v2 6/7] media: ov5693: add ov5693_of_match, dts support

[Tommaso Merciai]

Add ov5693_of_match. Device tree support

Reviewed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---
 drivers/media/i2c/ov5693.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 273caef467fe..0854226dc140 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1532,10 +1532,17 @@ static const struct acpi_device_id ov5693_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
 
+static const struct of_device_id ov5693_of_match[] = {
+	{ .compatible = "ovti,ov5693", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov5693_of_match);
+
 static struct i2c_driver ov5693_driver = {
 	.driver = {
 		.name = "ov5693",
 		.acpi_match_table = ov5693_acpi_match,
+		.of_match_table = ov5693_of_match,
 		.pm = &ov5693_pm_ops,
 	},
 	.probe_new = ov5693_probe,
-- 
2.25.1

[PATCH v2 7/7] media: dt-bindings: ov5693: document YAML binding

[Tommaso Merciai]

This patch adds documentation of device tree in YAML schema for the
OV5693 CMOS image sensor from Omnivision

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
Changes since v1:
 - Fix allOf position as suggested by Krzysztof
 - Remove port description as suggested by Krzysztof
 - Fix EOF as suggested by Krzysztof

 .../bindings/media/i2c/ovti,ov5693.yaml       | 108 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
new file mode 100644
index 000000000000..e8e2776efc08
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2022 Amarulasolutions
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5693.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5693 CMOS Sensor
+
+maintainers:
+  - Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
+
+description: |
+  The Omnivision OV5693 is a high performance, 1/4-inch, 5 megapixel, CMOS
+  image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
+  sub-sampled, and windowed 10-bit MIPI images in various formats via the
+  Serial Camera Control Bus (SCCB) interface.
+
+  Supports images sizes: 5 Mpixel, EIS1080p, 1080p, 720p, VGA, QVGA
+
+  OV5693 is programmable through I2C and two-wire Serial Camera Control Bus (SCCB).
+  The sensor output is available via CSI-2 serial data output (up to 2-lane).
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: ovti,ov5693
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description:
+      System input clock (aka XVCLK). From 6 to 27 MHz.
+    maxItems: 1
+
+  dovdd-supply:
+    description:
+      Digital I/O voltage supply, 1.8V.
+
+  avdd-supply:
+    description:
+      Analog voltage supply, 2.8V.
+
+  dvdd-supply:
+    description:
+      Digital core voltage supply, 1.2V.
+
+  reset-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls sensor reset.
+      This corresponds to the hardware pin XSHUTDN which is physically
+      active low.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/px30-cru.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/pinctrl/rockchip.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5693: camera@36 {
+            compatible = "ovti,ov5693";
+            reg = <0x36>;
+
+            reset-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&cif_clkout_m0>;
+
+            clocks = <&cru SCLK_CIF_OUT>;
+            assigned-clocks = <&cru SCLK_CIF_OUT>;
+            assigned-clock-rates = <19200000>;
+
+            avdd-supply = <&vcc_1v8>;
+            dvdd-supply = <&vcc_1v2>;
+            dovdd-supply = <&vcc_2v8>;
+
+            rotation = <90>;
+            orientation = <0>;
+
+            port {
+                ucam_out: endpoint {
+                    remote-endpoint = <&mipi_in_ucam>;
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <450000000>;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 1fc9ead83d2a..844307cb20c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14719,6 +14719,7 @@ M:	Daniel Scally <djrscally@gmail.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
 F:	drivers/media/i2c/ov5693.c
 
 OMNIVISION OV5695 SENSOR DRIVER
-- 
2.25.1

Re: [PATCH v2 1/7] media: ov5693: count num_supplies using array_size

[Jacopo Mondi]

Hi Tommaso

On Mon, Jun 27, 2022 at 05:04:47PM +0200, Tommaso Merciai wrote:
> Instead of hardcode OV5693_NUM_SUPPLIES in a define is better use
> ARRAY_SIZE function to count the number of supplies from
> ov5693_supply_names array
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>

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

thanks
  j

> ---
>  drivers/media/i2c/ov5693.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 117ff5403312..f410333c4c67 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -127,11 +127,15 @@
>  #define OV5693_LINK_FREQ_419_2MHZ		419200000
>  #define OV5693_PIXEL_RATE			167680000
>
> -/* Miscellaneous */
> -#define OV5693_NUM_SUPPLIES			2
> -
>  #define to_ov5693_sensor(x) container_of(x, struct ov5693_device, sd)
>
> +static const char * const ov5693_supply_names[] = {
> +	"avdd",		/* Analog power */
> +	"dovdd",	/* Digital I/O power */
> +};
> +
> +#define OV5693_NUM_SUPPLIES	ARRAY_SIZE(ov5693_supply_names)
> +
>  struct ov5693_reg {
>  	u32 reg;
>  	u8 val;
> @@ -352,11 +356,6 @@ static const s64 link_freq_menu_items[] = {
>  	OV5693_LINK_FREQ_419_2MHZ
>  };
>
> -static const char * const ov5693_supply_names[] = {
> -	"avdd",
> -	"dovdd",
> -};
> -
>  static const char * const ov5693_test_pattern_menu[] = {
>  	"Disabled",
>  	"Random Data",
> --
> 2.25.1
>

Re: [PATCH v2 2/7] media: ov5693: add dvdd into ov5693_supply_names array

[Jacopo Mondi]

Hi Tommaso

On Mon, Jun 27, 2022 at 05:04:48PM +0200, Tommaso Merciai wrote:
> Add missing "dvdd" (Digital circuit power) entry into ov5693_supply_names
> array
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>

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

Thanks
   j
> ---
>  drivers/media/i2c/ov5693.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index f410333c4c67..9e84468d920e 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -132,6 +132,7 @@
>  static const char * const ov5693_supply_names[] = {
>  	"avdd",		/* Analog power */
>  	"dovdd",	/* Digital I/O power */
> +	"dvdd",		/* Digital circuit power */
>  };
>
>  #define OV5693_NUM_SUPPLIES	ARRAY_SIZE(ov5693_supply_names)
> --
> 2.25.1
>

Re: [PATCH v2 3/7] media: ov5693: rename clk into xvclk

[Jacopo Mondi]

Hi Tommaso,

On Mon, Jun 27, 2022 at 05:04:49PM +0200, Tommaso Merciai wrote:
> Rename clk pdata pointer into xvclk (system clock input).
> Same for clk_rate into xvclk_rate. This is more explicit
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>

Not sure this is really better, but I guess it doesn't hurt

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

Thanks
  j

> ---
>  drivers/media/i2c/ov5693.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 9e84468d920e..d2adc5513a21 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -157,7 +157,7 @@ struct ov5693_device {
>  	struct gpio_desc *reset;
>  	struct gpio_desc *powerdown;
>  	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
> -	struct clk *clk;
> +	struct clk *xvclk;
>
>  	struct ov5693_mode {
>  		struct v4l2_rect crop;
> @@ -794,7 +794,7 @@ static void ov5693_sensor_powerdown(struct ov5693_device *ov5693)
>
>  	regulator_bulk_disable(OV5693_NUM_SUPPLIES, ov5693->supplies);
>
> -	clk_disable_unprepare(ov5693->clk);
> +	clk_disable_unprepare(ov5693->xvclk);
>  }
>
>  static int ov5693_sensor_powerup(struct ov5693_device *ov5693)
> @@ -804,7 +804,7 @@ static int ov5693_sensor_powerup(struct ov5693_device *ov5693)
>  	gpiod_set_value_cansleep(ov5693->reset, 1);
>  	gpiod_set_value_cansleep(ov5693->powerdown, 1);
>
> -	ret = clk_prepare_enable(ov5693->clk);
> +	ret = clk_prepare_enable(ov5693->xvclk);
>  	if (ret) {
>  		dev_err(ov5693->dev, "Failed to enable clk\n");
>  		goto fail_power;
> @@ -1390,7 +1390,7 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>  static int ov5693_probe(struct i2c_client *client)
>  {
>  	struct ov5693_device *ov5693;
> -	u32 clk_rate;
> +	u32 xvclk_rate;
>  	int ret = 0;
>
>  	ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
> @@ -1408,16 +1408,16 @@ static int ov5693_probe(struct i2c_client *client)
>
>  	v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
>
> -	ov5693->clk = devm_clk_get(&client->dev, "xvclk");
> -	if (IS_ERR(ov5693->clk)) {
> +	ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
> +	if (IS_ERR(ov5693->xvclk)) {
>  		dev_err(&client->dev, "Error getting clock\n");
> -		return PTR_ERR(ov5693->clk);
> +		return PTR_ERR(ov5693->xvclk);
>  	}
>
> -	clk_rate = clk_get_rate(ov5693->clk);
> -	if (clk_rate != OV5693_XVCLK_FREQ)
> +	xvclk_rate = clk_get_rate(ov5693->xvclk);
> +	if (xvclk_rate != OV5693_XVCLK_FREQ)
>  		dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
> -			 clk_rate, OV5693_XVCLK_FREQ);
> +			 xvclk_rate, OV5693_XVCLK_FREQ);
>
>  	ret = ov5693_configure_gpios(ov5693);
>  	if (ret)
> --
> 2.25.1
>

Re: [PATCH v2 5/7] media: ov5693: rename ov5693_check_hwcfg into ov5693_get_hwcfg

[Jacopo Mondi]

On Mon, Jun 27, 2022 at 05:04:51PM +0200, Tommaso Merciai wrote:
> Function ov5693_check_hwcfg don't only check the hw configuration but

s/don't/doesn't/ ?

> also get the hw configuration. Is better better to rename this into ov5693_get_hwcfg

The concept of "getting" the hw configuration vs "checking" it is
debatable. The function does many other things as far as I can tell.

However, this is also a little arbitrary but I guess it doesn't hurt ?

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

Thanks
  j

>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
>  drivers/media/i2c/ov5693.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index d5a934ace597..273caef467fe 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1339,7 +1339,7 @@ static int ov5693_get_regulators(struct ov5693_device *ov5693)
>  				       ov5693->supplies);
>  }
>
> -static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
> +static int ov5693_get_hwcfg(struct ov5693_device *ov5693)
>  {
>  	struct fwnode_handle *fwnode = dev_fwnode(ov5693->dev);
>  	struct v4l2_fwnode_endpoint bus_cfg = {
> @@ -1431,7 +1431,7 @@ static int ov5693_probe(struct i2c_client *client)
>  	ov5693->client = client;
>  	ov5693->dev = &client->dev;
>
> -	ret = ov5693_check_hwcfg(ov5693);
> +	ret = ov5693_get_hwcfg(ov5693);
>  	if (ret)
>  		return ret;
>
> --
> 2.25.1
>

Re: [PATCH v2 4/7] media: ov5693: move hw cfg functions into ov5693_check_hwcfg

[Jacopo Mondi]

Hi Tommaso,

On Mon, Jun 27, 2022 at 05:04:50PM +0200, Tommaso Merciai wrote:
> Move hw configuration functions into ov5693_check_hwcfg. This is done to
> separe the code that handle the hw cfg from probe in a clean way

s/separe/separate/

You also seem to change the logic of the clk handling, please mention
this in the commit message, otherwise one could be fooled into
thinking you're only moving code around with no functional changes...

>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
>  drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index d2adc5513a21..d5a934ace597 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>  	struct fwnode_handle *endpoint;
>  	unsigned int i;
>  	int ret;
> +	u32 xvclk_rate;

nit: move it up to maintain reverse-xmas-tree order (I know, it's an
annoying comment, but since variables are already declared in this order..)

> +
> +	ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");

Isn't this broken ?

if you use ov5693->xvclk to identify the ACPI vs OF use case shouldn't
you use the get_optionl() version ? Otherwise in the ACPI case you will have
-ENOENT if there's not 'xvclk' property and bail out.

Unless my understanding is wrong on ACPI we have "clock-frequency" and
on OF "xvclk" with an "assigned-clock-rates",

Dan you upstreamed this driver and I assume it was tested on ACPI ?
Can you clarify how this worked for you, as it seems the original code
wanted a mandatory "xvclk" ? Are there ACPI tables with an actual
'xvclk' property ?

> +	if (IS_ERR(ov5693->xvclk))
> +		return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
> +				     "failed to get xvclk: %ld\n",
> +				     PTR_ERR(ov5693->xvclk));
> +
> +	if (ov5693->xvclk) {
> +		xvclk_rate = clk_get_rate(ov5693->xvclk);
> +	} else {
> +		ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> +					       &xvclk_rate);
> +
> +		if (ret) {
> +			dev_err(ov5693->dev, "can't get clock frequency");
> +			return ret;
> +		}
> +	}
> +
> +	if (xvclk_rate != OV5693_XVCLK_FREQ)
> +		dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
> +			 xvclk_rate, OV5693_XVCLK_FREQ);
> +
> +	ret = ov5693_configure_gpios(ov5693);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5693_get_regulators(ov5693);
> +	if (ret)
> +		return dev_err_probe(ov5693->dev, ret,
> +				     "Error fetching regulators\n");
>
>  	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!endpoint)
> @@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>  static int ov5693_probe(struct i2c_client *client)
>  {
>  	struct ov5693_device *ov5693;
> -	u32 xvclk_rate;
>  	int ret = 0;
>
>  	ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
> @@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
>
>  	v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
>
> -	ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
> -	if (IS_ERR(ov5693->xvclk)) {
> -		dev_err(&client->dev, "Error getting clock\n");
> -		return PTR_ERR(ov5693->xvclk);
> -	}
> -
> -	xvclk_rate = clk_get_rate(ov5693->xvclk);
> -	if (xvclk_rate != OV5693_XVCLK_FREQ)
> -		dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
> -			 xvclk_rate, OV5693_XVCLK_FREQ);
> -
> -	ret = ov5693_configure_gpios(ov5693);
> -	if (ret)
> -		return ret;
> -
> -	ret = ov5693_get_regulators(ov5693);
> -	if (ret)
> -		return dev_err_probe(&client->dev, ret,
> -				     "Error fetching regulators\n");
> -
>  	ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> --
> 2.25.1
>

Re: [PATCH v2 6/7] media: ov5693: add ov5693_of_match, dts support

[Jacopo Mondi]

Hi Tommaso,

On Mon, Jun 27, 2022 at 05:04:52PM +0200, Tommaso Merciai wrote:
> Add ov5693_of_match. Device tree support
>
> Reviewed-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

Missing Signed-off-by

> ---
>  drivers/media/i2c/ov5693.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 273caef467fe..0854226dc140 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1532,10 +1532,17 @@ static const struct acpi_device_id ov5693_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
>
> +static const struct of_device_id ov5693_of_match[] = {
> +	{ .compatible = "ovti,ov5693", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov5693_of_match);
> +
>  static struct i2c_driver ov5693_driver = {
>  	.driver = {
>  		.name = "ov5693",
>  		.acpi_match_table = ov5693_acpi_match,
> +		.of_match_table = ov5693_of_match,
>  		.pm = &ov5693_pm_ops,
>  	},
>  	.probe_new = ov5693_probe,
> --
> 2.25.1
>

Re: [PATCH v2 4/7] media: ov5693: move hw cfg functions into ov5693_check_hwcfg

[Tommaso Merciai]

Hi Jacopo,

On Wed, Jun 29, 2022 at 10:16:35AM +0200, Jacopo Mondi wrote:
> Hi Tommaso,
> 
> On Mon, Jun 27, 2022 at 05:04:50PM +0200, Tommaso Merciai wrote:
> > Move hw configuration functions into ov5693_check_hwcfg. This is done to
> > separe the code that handle the hw cfg from probe in a clean way
> 
> s/separe/separate/
> 
> You also seem to change the logic of the clk handling, please mention
> this in the commit message, otherwise one could be fooled into
> thinking you're only moving code around with no functional changes...

Right. I'll add some comments on support to get clock-frequency using
fwnode_property_read_u32 in v3

> 
> >
> > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > ---
> >  drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
> >  1 file changed, 32 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> > index d2adc5513a21..d5a934ace597 100644
> > --- a/drivers/media/i2c/ov5693.c
> > +++ b/drivers/media/i2c/ov5693.c
> > @@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
> >  	struct fwnode_handle *endpoint;
> >  	unsigned int i;
> >  	int ret;
> > +	u32 xvclk_rate;
> 
> nit: move it up to maintain reverse-xmas-tree order (I know, it's an
> annoying comment, but since variables are already declared in this order..)

No problem :)
I'll do it in v3.

> 
> > +
> > +	ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");
> 
> Isn't this broken ?
> 
> if you use ov5693->xvclk to identify the ACPI vs OF use case shouldn't
> you use the get_optionl() version ? Otherwise in the ACPI case you will have
> -ENOENT if there's not 'xvclk' property and bail out.

You are right, devm_clk_get_optional is the correct way.

Thanks,
Tommaso

> 
> Unless my understanding is wrong on ACPI we have "clock-frequency" and
> on OF "xvclk" with an "assigned-clock-rates",
> 
> Dan you upstreamed this driver and I assume it was tested on ACPI ?
> Can you clarify how this worked for you, as it seems the original code
> wanted a mandatory "xvclk" ? Are there ACPI tables with an actual
> 'xvclk' property ?
> 
> > +	if (IS_ERR(ov5693->xvclk))
> > +		return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
> > +				     "failed to get xvclk: %ld\n",
> > +				     PTR_ERR(ov5693->xvclk));
> > +
> > +	if (ov5693->xvclk) {
> > +		xvclk_rate = clk_get_rate(ov5693->xvclk);
> > +	} else {
> > +		ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > +					       &xvclk_rate);
> > +
> > +		if (ret) {
> > +			dev_err(ov5693->dev, "can't get clock frequency");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (xvclk_rate != OV5693_XVCLK_FREQ)
> > +		dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
> > +			 xvclk_rate, OV5693_XVCLK_FREQ);
> > +
> > +	ret = ov5693_configure_gpios(ov5693);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ov5693_get_regulators(ov5693);
> > +	if (ret)
> > +		return dev_err_probe(ov5693->dev, ret,
> > +				     "Error fetching regulators\n");
> >
> >  	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> >  	if (!endpoint)
> > @@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
> >  static int ov5693_probe(struct i2c_client *client)
> >  {
> >  	struct ov5693_device *ov5693;
> > -	u32 xvclk_rate;
> >  	int ret = 0;
> >
> >  	ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
> > @@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
> >
> >  	v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
> >
> > -	ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
> > -	if (IS_ERR(ov5693->xvclk)) {
> > -		dev_err(&client->dev, "Error getting clock\n");
> > -		return PTR_ERR(ov5693->xvclk);
> > -	}
> > -
> > -	xvclk_rate = clk_get_rate(ov5693->xvclk);
> > -	if (xvclk_rate != OV5693_XVCLK_FREQ)
> > -		dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
> > -			 xvclk_rate, OV5693_XVCLK_FREQ);
> > -
> > -	ret = ov5693_configure_gpios(ov5693);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = ov5693_get_regulators(ov5693);
> > -	if (ret)
> > -		return dev_err_probe(&client->dev, ret,
> > -				     "Error fetching regulators\n");
> > -
> >  	ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >  	ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
> >  	ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > --
> > 2.25.1
> >

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH v2 7/7] media: dt-bindings: ov5693: document YAML binding

[Krzysztof Kozlowski]

On 27/06/2022 17:04, Tommaso Merciai wrote:
> This patch adds documentation of device tree in YAML schema for the
> OV5693 CMOS image sensor from Omnivision

You could remove "This patch", but it's a nit-pick
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH v2 7/7] media: dt-bindings: ov5693: document YAML binding

[Tommaso Merciai]

Hi Krzysztof,

On Wed, Jun 29, 2022 at 12:42:10PM +0200, Krzysztof Kozlowski wrote:
> On 27/06/2022 17:04, Tommaso Merciai wrote:
> > This patch adds documentation of device tree in YAML schema for the
> > OV5693 CMOS image sensor from Omnivision
> 
> You could remove "This patch", but it's a nit-pick
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

No issue :)

I'll fix this in v3.
Thanks for the review.

Regards,
Tommaso

> 
> > 
> > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> 
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> 
> Best regards,
> Krzysztof

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH v2 4/7] media: ov5693: move hw cfg functions into ov5693_check_hwcfg

[Daniel Scally]

Hey

On 29/06/2022 09:16, Jacopo Mondi wrote:
> Hi Tommaso,
>
> On Mon, Jun 27, 2022 at 05:04:50PM +0200, Tommaso Merciai wrote:
>> Move hw configuration functions into ov5693_check_hwcfg. This is done to
>> separe the code that handle the hw cfg from probe in a clean way
> s/separe/separate/
>
> You also seem to change the logic of the clk handling, please mention
> this in the commit message, otherwise one could be fooled into
> thinking you're only moving code around with no functional changes...
>
>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
>> ---
>>  drivers/media/i2c/ov5693.c | 53 +++++++++++++++++++++++---------------
>>  1 file changed, 32 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
>> index d2adc5513a21..d5a934ace597 100644
>> --- a/drivers/media/i2c/ov5693.c
>> +++ b/drivers/media/i2c/ov5693.c
>> @@ -1348,6 +1348,38 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>>  	struct fwnode_handle *endpoint;
>>  	unsigned int i;
>>  	int ret;
>> +	u32 xvclk_rate;
> nit: move it up to maintain reverse-xmas-tree order (I know, it's an
> annoying comment, but since variables are already declared in this order..)
>
>> +
>> +	ov5693->xvclk = devm_clk_get(ov5693->dev, "xvclk");
> Isn't this broken ?
>
> if you use ov5693->xvclk to identify the ACPI vs OF use case shouldn't
> you use the get_optionl() version ? Otherwise in the ACPI case you will have
> -ENOENT if there's not 'xvclk' property and bail out.
>
> Unless my understanding is wrong on ACPI we have "clock-frequency" and
> on OF "xvclk" with an "assigned-clock-rates",
>
> Dan you upstreamed this driver and I assume it was tested on ACPI ?
> Can you clarify how this worked for you, as it seems the original code
> wanted a mandatory "xvclk" ? Are there ACPI tables with an actual
> 'xvclk' property ?


Sorry - late answer, but when I wrote this although it's ostensibly for
an ACPI platform, it's actually only tested with the IPU3 platforms
which work in a _weird_ way. The fix we eventually came to was to create
through the int3472-discrete driver clocks and regulators through the
normal frameworks that a dt platform would expect to consume, so even
though the devices are enumerated through ACPI, the clock/regulator
parts really work more like a dt platform.


You're right that it needs to be get_optional() here, but with that
added I think this is fine - I tested it last night and it works ok for me.

>
>> +	if (IS_ERR(ov5693->xvclk))
>> +		return dev_err_probe(ov5693->dev, PTR_ERR(ov5693->xvclk),
>> +				     "failed to get xvclk: %ld\n",
>> +				     PTR_ERR(ov5693->xvclk));
>> +
>> +	if (ov5693->xvclk) {
>> +		xvclk_rate = clk_get_rate(ov5693->xvclk);
>> +	} else {
>> +		ret = fwnode_property_read_u32(fwnode, "clock-frequency",
>> +					       &xvclk_rate);
>> +
>> +		if (ret) {
>> +			dev_err(ov5693->dev, "can't get clock frequency");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (xvclk_rate != OV5693_XVCLK_FREQ)
>> +		dev_warn(ov5693->dev, "Found clk freq %u, expected %u\n",
>> +			 xvclk_rate, OV5693_XVCLK_FREQ);
>> +
>> +	ret = ov5693_configure_gpios(ov5693);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ov5693_get_regulators(ov5693);
>> +	if (ret)
>> +		return dev_err_probe(ov5693->dev, ret,
>> +				     "Error fetching regulators\n");
>>
>>  	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
>>  	if (!endpoint)
>> @@ -1390,7 +1422,6 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>>  static int ov5693_probe(struct i2c_client *client)
>>  {
>>  	struct ov5693_device *ov5693;
>> -	u32 xvclk_rate;
>>  	int ret = 0;
>>
>>  	ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
>> @@ -1408,26 +1439,6 @@ static int ov5693_probe(struct i2c_client *client)
>>
>>  	v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
>>
>> -	ov5693->xvclk = devm_clk_get(&client->dev, "xvclk");
>> -	if (IS_ERR(ov5693->xvclk)) {
>> -		dev_err(&client->dev, "Error getting clock\n");
>> -		return PTR_ERR(ov5693->xvclk);
>> -	}
>> -
>> -	xvclk_rate = clk_get_rate(ov5693->xvclk);
>> -	if (xvclk_rate != OV5693_XVCLK_FREQ)
>> -		dev_warn(&client->dev, "Found clk freq %u, expected %u\n",
>> -			 xvclk_rate, OV5693_XVCLK_FREQ);
>> -
>> -	ret = ov5693_configure_gpios(ov5693);
>> -	if (ret)
>> -		return ret;
>> -
>> -	ret = ov5693_get_regulators(ov5693);
>> -	if (ret)
>> -		return dev_err_probe(&client->dev, ret,
>> -				     "Error fetching regulators\n");
>> -
>>  	ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>  	ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
>>  	ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>> --
>> 2.25.1
>>