[PATCH v4 0/6] media: ov5693: cleanup code and add dts support

[PATCH v4 0/6] 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 (6):
  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_hwcfg
  media: dt-bindings: ov5693: document YAML binding
  media: ov5693: add ov5693_of_match, dts support

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

-- 
2.25.1

[PATCH v4 1/6] 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes since v3:
 - Add reviewed-by tag, suggested by Jacopo, Krzysztof

 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 v4 1/6] media: ov5693: count num_supplies using array_size

[Daniel Scally]

Hello - sorry, hadn't had much time to look  at this series.

On 30/06/2022 08:45, 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>


Reviewed-by: Daniel Scally <djrscally@gmail.com>

> ---
> Changes since v3:
>  - Add reviewed-by tag, suggested by Jacopo, Krzysztof
>
>  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",

[PATCH v4 2/6] 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes since v3:
 - Add reviewed-by tag, suggested by Jacopo, Krzysztof

 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 v4 2/6] media: ov5693: add dvdd into ov5693_supply_names array

[Daniel Scally]

Hello


On 30/06/2022 08:45, 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>


Reviewed-by: Daniel Scally <djrscally@gmail.com>

> ---
> Changes since v3:
>  - Add reviewed-by tag, suggested by Jacopo, Krzysztof
>
>  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)

[PATCH v4 3/6] 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes since v3:
 - Add reviewed-by tag, suggested by Jacopo, Krzysztof

 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 v4 4/6] media: ov5693: move hw cfg functions into ov5693_hwcfg

[Tommaso Merciai]

Move hw configuration functions into ov5693_hwcfg. This is done to
separate the code that handle the hw cfg from probe in a clean way.
Add support for ACPI-based platforms that specify the clock frequency by
using the "clock-frequency" property instead of specifying a clock
provider reference

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
---
Changes since v2:
 - Fix commit body as suggested by Sakari, Jacopo
 - Add details to commit body as suggested by Jacopo
 - Move ov5693_check_hwcfg into ov5693_hwcfg
 - Fix xvclk_rate position as suggested by Jacopo
 - Use devm_clk_get_optional instead of devm_clk_get as suggested Jacopo

Changes since v3:
 - Fix commit body as suggested by Jacopo
 - Add reviewed-by tags, suggested by Jacopo, Krzysztof

 drivers/media/i2c/ov5693.c | 57 +++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index d2adc5513a21..3c805a5a5181 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1339,16 +1339,48 @@ static int ov5693_get_regulators(struct ov5693_device *ov5693)
 				       ov5693->supplies);
 }
 
-static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
+static int ov5693_hwcfg(struct ov5693_device *ov5693)
 {
 	struct fwnode_handle *fwnode = dev_fwnode(ov5693->dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY,
 	};
 	struct fwnode_handle *endpoint;
+	u32 xvclk_rate;
 	unsigned int i;
 	int ret;
 
+	ov5693->xvclk = devm_clk_get_optional(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)
 		return -EPROBE_DEFER; /* Could be provided by cio2-bridge */
@@ -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);
@@ -1400,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_hwcfg(ov5693);
 	if (ret)
 		return ret;
 
@@ -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 v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Tommaso Merciai]

Add documentation of device tree in YAML schema for the OV5693
CMOS image sensor from Omnivision

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
---
Changes since v1:
 - Fix allOf position as suggested by Krzysztof
 - Remove port description as suggested by Krzysztof
 - Fix EOF as suggested by Krzysztof

Changes since v2:
 - Fix commit body as suggested by Krzysztof

Changes since v3:
 - Add reviewed-by tags, ssuggested by Jacopo, Krzysztof

 .../bindings/media/i2c/ovti,ov5693.yaml       | 106 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 107 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..b83c9fc04023
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
@@ -0,0 +1,106 @@
+# 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.
+
+  OV5693 is controlled via 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 v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Krzysztof Kozlowski]

On 30/06/2022 09:45, Tommaso Merciai wrote:
> Add documentation of device tree in YAML schema for the OV5693
> CMOS image sensor from Omnivision
> 
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>

How Sakari's tag appeared here? There was no email from him.

Best regards,
Krzysztof

Re: [PATCH v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Tommaso Merciai]

On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2022 09:45, Tommaso Merciai wrote:
> > Add documentation of device tree in YAML schema for the OV5693
> > CMOS image sensor from Omnivision
> > 
> > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> How Sakari's tag appeared here? There was no email from him.

Sakari made me some review on v2, but I think he forgot to add the mailing
list in cc. ( I suppose :) )

Let me know if I need to remove this.

Regards,
Tommaso


> 
> 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 v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Sakari Ailus]

On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > Add documentation of device tree in YAML schema for the OV5693
> > > CMOS image sensor from Omnivision
> > > 
> > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > How Sakari's tag appeared here? There was no email from him.
> 
> Sakari made me some review on v2, but I think he forgot to add the mailing
> list in cc. ( I suppose :) )
> 
> Let me know if I need to remove this.

You're only supposed to put these tags into patches if you get them in
written form as part of the review, signalling acceptance of the patch in
various forms. Just commenting a patch does not imply this.

Please also see Documentation/process/submitting-patches.rst for more
information on how to use the tags.

-- 
Sakari Ailus

Re: [PATCH v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Tommaso Merciai]

Hi Sakari,

On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > > Add documentation of device tree in YAML schema for the OV5693
> > > > CMOS image sensor from Omnivision
> > > > 
> > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > 
> > > How Sakari's tag appeared here? There was no email from him.
> > 
> > Sakari made me some review on v2, but I think he forgot to add the mailing
> > list in cc. ( I suppose :) )
> > 
> > Let me know if I need to remove this.
> 
> You're only supposed to put these tags into patches if you get them in
> written form as part of the review, signalling acceptance of the patch in
> various forms. Just commenting a patch does not imply this.
> 
> Please also see Documentation/process/submitting-patches.rst for more
> information on how to use the tags.

Thanks for sharing this. My bad.
I remove your tags.

Regards,
Tommaso

> 
> -- 
> Sakari Ailus

-- 
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 v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Sakari Ailus]

Hi Tommaso,

On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> Hi Sakari,
> 
> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> > On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > > On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > > > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > > > Add documentation of device tree in YAML schema for the OV5693
> > > > > CMOS image sensor from Omnivision
> > > > > 
> > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > 
> > > > How Sakari's tag appeared here? There was no email from him.
> > > 
> > > Sakari made me some review on v2, but I think he forgot to add the mailing
> > > list in cc. ( I suppose :) )
> > > 
> > > Let me know if I need to remove this.
> > 
> > You're only supposed to put these tags into patches if you get them in
> > written form as part of the review, signalling acceptance of the patch in
> > various forms. Just commenting a patch does not imply this.
> > 
> > Please also see Documentation/process/submitting-patches.rst for more
> > information on how to use the tags.
> 
> Thanks for sharing this. My bad.
> I remove your tags.

The patches themselves seem fine. I'd just drop the 4th patch or at least
come up with a better name for ov5693_hwcfg() --- you're acquiring
resources there, and that generally fits well for probe. The code is fine
already.

-- 
Sakari Ailus

Re: [PATCH v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Tommaso Merciai]

Hi Sakari,

On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
> Hi Tommaso,
> 
> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> > Hi Sakari,
> > 
> > On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> > > On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > > > On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > > > > On 30/06/2022 09:45, Tommaso Merciai wrote:
> > > > > > Add documentation of device tree in YAML schema for the OV5693
> > > > > > CMOS image sensor from Omnivision
> > > > > > 
> > > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > > Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > 
> > > > > How Sakari's tag appeared here? There was no email from him.
> > > > 
> > > > Sakari made me some review on v2, but I think he forgot to add the mailing
> > > > list in cc. ( I suppose :) )
> > > > 
> > > > Let me know if I need to remove this.
> > > 
> > > You're only supposed to put these tags into patches if you get them in
> > > written form as part of the review, signalling acceptance of the patch in
> > > various forms. Just commenting a patch does not imply this.
> > > 
> > > Please also see Documentation/process/submitting-patches.rst for more
> > > information on how to use the tags.
> > 
> > Thanks for sharing this. My bad.
> > I remove your tags.
> 
> The patches themselves seem fine. I'd just drop the 4th patch or at least
> come up with a better name for ov5693_hwcfg() --- you're acquiring
> resources there, and that generally fits well for probe. The code is fine
> already.

Then we don't need v5 with your reviewed tags removed?

I think the patch4 is needed to add dts support properly.
Also this contains devm_clk_get_optional fix suggested by Jacopo and
support for ACPI-based platforms that specify the clock frequency by
using the "clock-frequency" property instead of specifying a clock
provider reference.

Some suggestion on the function name?

Thanks,
Tommaso

> 
> -- 
> Sakari Ailus

-- 
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 v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Daniel Scally]

Hello

On 30/06/2022 11:09, Tommaso Merciai wrote:
> Hi Sakari,
>
> On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
>> Hi Tommaso,
>>
>> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
>>> Hi Sakari,
>>>
>>> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
>>>> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
>>>>> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 30/06/2022 09:45, Tommaso Merciai wrote:
>>>>>>> Add documentation of device tree in YAML schema for the OV5693
>>>>>>> CMOS image sensor from Omnivision
>>>>>>>
>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>>> Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
>>>>>> How Sakari's tag appeared here? There was no email from him.
>>>>> Sakari made me some review on v2, but I think he forgot to add the mailing
>>>>> list in cc. ( I suppose :) )
>>>>>
>>>>> Let me know if I need to remove this.
>>>> You're only supposed to put these tags into patches if you get them in
>>>> written form as part of the review, signalling acceptance of the patch in
>>>> various forms. Just commenting a patch does not imply this.
>>>>
>>>> Please also see Documentation/process/submitting-patches.rst for more
>>>> information on how to use the tags.
>>> Thanks for sharing this. My bad.
>>> I remove your tags.
>> The patches themselves seem fine. I'd just drop the 4th patch or at least
>> come up with a better name for ov5693_hwcfg() --- you're acquiring
>> resources there, and that generally fits well for probe. The code is fine
>> already.
> Then we don't need v5 with your reviewed tags removed?
>
> I think the patch4 is needed to add dts support properly.
> Also this contains devm_clk_get_optional fix suggested by Jacopo and
> support for ACPI-based platforms that specify the clock frequency by
> using the "clock-frequency" property instead of specifying a clock
> provider reference.


I agree patch 4 in some form is needed - I didn't do the clock handling
particularly well in this driver, and though it's ostensibly an ACPI
driver it wouldn't actually work with a "normal" ACPI, but just with the
cio2-bridge-repaired style. So the changes to the clock handling logic
are welcome and needed I think. whether it needs to go into a separate
function I don't particularly mind either way.

>
> Some suggestion on the function name?
>
> Thanks,
> Tommaso
>
>> -- 
>> Sakari Ailus

Re: [PATCH v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Sakari Ailus]

On Thu, Jun 30, 2022 at 11:15:40AM +0100, Daniel Scally wrote:
> Hello
> 
> On 30/06/2022 11:09, Tommaso Merciai wrote:
> > Hi Sakari,
> >
> > On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
> >> Hi Tommaso,
> >>
> >> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> >>> Hi Sakari,
> >>>
> >>> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> >>>> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> >>>>> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On 30/06/2022 09:45, Tommaso Merciai wrote:
> >>>>>>> Add documentation of device tree in YAML schema for the OV5693
> >>>>>>> CMOS image sensor from Omnivision
> >>>>>>>
> >>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> >>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>>>>> Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
> >>>>>> How Sakari's tag appeared here? There was no email from him.
> >>>>> Sakari made me some review on v2, but I think he forgot to add the mailing
> >>>>> list in cc. ( I suppose :) )
> >>>>>
> >>>>> Let me know if I need to remove this.
> >>>> You're only supposed to put these tags into patches if you get them in
> >>>> written form as part of the review, signalling acceptance of the patch in
> >>>> various forms. Just commenting a patch does not imply this.
> >>>>
> >>>> Please also see Documentation/process/submitting-patches.rst for more
> >>>> information on how to use the tags.
> >>> Thanks for sharing this. My bad.
> >>> I remove your tags.
> >> The patches themselves seem fine. I'd just drop the 4th patch or at least
> >> come up with a better name for ov5693_hwcfg() --- you're acquiring
> >> resources there, and that generally fits well for probe. The code is fine
> >> already.
> > Then we don't need v5 with your reviewed tags removed?
> >
> > I think the patch4 is needed to add dts support properly.
> > Also this contains devm_clk_get_optional fix suggested by Jacopo and
> > support for ACPI-based platforms that specify the clock frequency by
> > using the "clock-frequency" property instead of specifying a clock
> > provider reference.
> 
> 
> I agree patch 4 in some form is needed - I didn't do the clock handling
> particularly well in this driver, and though it's ostensibly an ACPI
> driver it wouldn't actually work with a "normal" ACPI, but just with the
> cio2-bridge-repaired style. So the changes to the clock handling logic
> are welcome and needed I think. whether it needs to go into a separate
> function I don't particularly mind either way.

Yes, the clock handling needs to be changed. But I'd keep it in probe.

-- 
Sakari Ailus

Re: [PATCH v4 5/6] media: dt-bindings: ov5693: document YAML binding

[Tommaso Merciai]

Hi Sakari,

On Thu, Jun 30, 2022 at 02:21:04PM +0300, Sakari Ailus wrote:
> On Thu, Jun 30, 2022 at 11:15:40AM +0100, Daniel Scally wrote:
> > Hello
> > 
> > On 30/06/2022 11:09, Tommaso Merciai wrote:
> > > Hi Sakari,
> > >
> > > On Thu, Jun 30, 2022 at 12:50:05PM +0300, Sakari Ailus wrote:
> > >> Hi Tommaso,
> > >>
> > >> On Thu, Jun 30, 2022 at 11:16:13AM +0200, Tommaso Merciai wrote:
> > >>> Hi Sakari,
> > >>>
> > >>> On Thu, Jun 30, 2022 at 12:12:47PM +0300, Sakari Ailus wrote:
> > >>>> On Thu, Jun 30, 2022 at 11:02:32AM +0200, Tommaso Merciai wrote:
> > >>>>> On Thu, Jun 30, 2022 at 10:07:19AM +0200, Krzysztof Kozlowski wrote:
> > >>>>>> On 30/06/2022 09:45, Tommaso Merciai wrote:
> > >>>>>>> Add documentation of device tree in YAML schema for the OV5693
> > >>>>>>> CMOS image sensor from Omnivision
> > >>>>>>>
> > >>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > >>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > >>>>>>> Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
> > >>>>>> How Sakari's tag appeared here? There was no email from him.
> > >>>>> Sakari made me some review on v2, but I think he forgot to add the mailing
> > >>>>> list in cc. ( I suppose :) )
> > >>>>>
> > >>>>> Let me know if I need to remove this.
> > >>>> You're only supposed to put these tags into patches if you get them in
> > >>>> written form as part of the review, signalling acceptance of the patch in
> > >>>> various forms. Just commenting a patch does not imply this.
> > >>>>
> > >>>> Please also see Documentation/process/submitting-patches.rst for more
> > >>>> information on how to use the tags.
> > >>> Thanks for sharing this. My bad.
> > >>> I remove your tags.
> > >> The patches themselves seem fine. I'd just drop the 4th patch or at least
> > >> come up with a better name for ov5693_hwcfg() --- you're acquiring
> > >> resources there, and that generally fits well for probe. The code is fine
> > >> already.
> > > Then we don't need v5 with your reviewed tags removed?
> > >
> > > I think the patch4 is needed to add dts support properly.
> > > Also this contains devm_clk_get_optional fix suggested by Jacopo and
> > > support for ACPI-based platforms that specify the clock frequency by
> > > using the "clock-frequency" property instead of specifying a clock
> > > provider reference.
> > 
> > 
> > I agree patch 4 in some form is needed - I didn't do the clock handling
> > particularly well in this driver, and though it's ostensibly an ACPI
> > driver it wouldn't actually work with a "normal" ACPI, but just with the
> > cio2-bridge-repaired style. So the changes to the clock handling logic
> > are welcome and needed I think. whether it needs to go into a separate
> > function I don't particularly mind either way.
> 
> Yes, the clock handling needs to be changed. But I'd keep it in probe.

Fixed in v5, as you suggest.
Thanks all for your time.

Regards,
Tommaso

> 
> -- 
> Sakari Ailus

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

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

[Tommaso Merciai]

Add ov5693_of_match. Device tree support

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
---
Changes since v2:
 - Change patch order 7 -> 6 as suggested by Sakari
 - Add missing signed-off as suggested by Jacopo

Changes since v3:
 - Add reviewed-by tags, suggested by Jacopo, Krzysztof

 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 3c805a5a5181..5930207193c7 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 v4 6/6] media: ov5693: add ov5693_of_match, dts support

[Daniel Scally]

Hello

On 30/06/2022 08:45, Tommaso Merciai wrote:
> Add ov5693_of_match. Device tree support
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>


Reviewed-by: Daniel Scally <djrscally@gmail.com>

> ---
> Changes since v2:
>  - Change patch order 7 -> 6 as suggested by Sakari
>  - Add missing signed-off as suggested by Jacopo
>
> Changes since v3:
>  - Add reviewed-by tags, suggested by Jacopo, Krzysztof
>
>  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 3c805a5a5181..5930207193c7 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,