[PATCH 0/6] media: i2c: ov5670: OF support, runtime_pm, regulators

[PATCH 0/6] media: i2c: ov5670: OF support, runtime_pm, regulators

[Jacopo Mondi]

Hello this small series introduces OF support for the ov5670 sensor and
adds support for regulators and GPIOs.

It also register runtime_pm callbacks and rework the powering sequence (cc
Paul(s) and Sakari for the discussion about the same topic on ov5640)

Tested on an OF system, ACPI should not be affected
(ofc, testing would be nice :)

Thanks
   j

Jacopo Mondi (5):
  media: dt-bindings: i2c: Document ov5670
  media: i2c: ov5670: Allow probing with OF
  media: i2c: ov5670: Probe regulators
  media: i2c: ov5670: Probe GPIOs
  media: i2c: ov5670: Add runtime_pm operations

Jean-Michel Hautbois (1):
  media: i2c: ov5670: Add .get_selection() support

 .../devicetree/bindings/media/i2c/ov5670.yaml |  93 +++++++++
 drivers/media/i2c/ov5670.c                    | 189 +++++++++++++++++-
 2 files changed, 276 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml

--
2.35.1

[PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Provide the bindings documentation for Omnivision OV5670 image sensor.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
new file mode 100644
index 000000000000..dc4a3297bf6f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5670 5 Megapixels raw image sensor
+
+maintainers:
+  - Jacopo Mondi <jacopo@jmondi.org>
+
+description: |-
+  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
+  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
+  controlled through an I2C compatible control bus.
+
+properties:
+  compatible:
+    const: ovti,ov5670
+
+  reg:
+    maxItems: 1
+
+  clock-frequency:
+    description: Frequency of the xclk clock.
+
+  pwdn-gpios:
+    description: Reference to the GPIO connected to the PWDNB pin. Active low.
+
+  reset-gpios:
+    description:
+      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
+
+  avdd-supply:
+    description: Analog circuit power. Typically 2.8V.
+
+  dvdd-supply:
+    description: Digital circuit power. Typically 1.2V.
+
+  dovdd-supply:
+    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            description: The sensor supports 1 or 2 data lanes operations.
+            minItems: 1
+            maxItems: 2
+            items:
+              maximum: 2
+
+          clock-noncontinuous: true
+
+required:
+  - compatible
+  - reg
+  - clock-frequency
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5670: sensor@36 {
+            compatible = "ovti,ov5670";
+            reg = <0x36>;
+
+            clock-frequency=<19200000>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&csi_ep>;
+                    data-lanes = <1 2>;
+                    clock-noncontinuous;
+                };
+            };
+        };
+    };
+
+...
+
--
2.35.1

[PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[Jacopo Mondi]

The ov5670 driver currently only supports probing using ACPI matching.
Add support for OF and add a missing header inclusion.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 02f75c18e480..39786f3c9489 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -3,7 +3,9 @@
 
 #include <linux/acpi.h>
 #include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -2583,6 +2585,12 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
 };
 
 MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
+#elif defined CONFIG_OF
+static const struct of_device_id ov5670_of_ids[] = {
+	{ .compatible = "ovti,ov5670" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov5670_of_ids);
 #endif
 
 static struct i2c_driver ov5670_i2c_driver = {
@@ -2590,6 +2598,7 @@ static struct i2c_driver ov5670_i2c_driver = {
 		.name = "ov5670",
 		.pm = &ov5670_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
+		.of_match_table = of_match_ptr(ov5670_of_ids),
 	},
 	.probe_new = ov5670_probe,
 	.remove = ov5670_remove,
-- 
2.35.1

[PATCH 3/6] media: i2c: ov5670: Probe regulators

[Jacopo Mondi]

The OV5670 has three power supplies (AVDD, DOVDD and DVDD).

Probe them in the driver to prepare controlling with runtime_pm
operations.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 39786f3c9489..cba310aec011 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -85,6 +86,14 @@ struct ov5670_link_freq_config {
 	const struct ov5670_reg_list reg_list;
 };
 
+static const char * const ov5670_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital power */
+	"dovdd",	/* Digital output power */
+};
+
+#define OV5670_NUM_SUPPLIES ARRAY_SIZE(ov5670_supply_names)
+
 struct ov5670_mode {
 	/* Frame width in pixels */
 	u32 width;
@@ -1830,6 +1839,9 @@ struct ov5670 {
 	/* Current mode */
 	const struct ov5670_mode *cur_mode;
 
+	/* Regulators */
+	struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES];
+
 	/* To serialize asynchronus callbacks */
 	struct mutex mutex;
 
@@ -2470,6 +2482,18 @@ static const struct v4l2_subdev_internal_ops ov5670_internal_ops = {
 	.open = ov5670_open,
 };
 
+static int ov5670_regulators_probe(struct ov5670 *ov5670)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+	unsigned int i;
+
+	for (i = 0; i < OV5670_NUM_SUPPLIES; i++)
+		ov5670->supplies[i].supply = ov5670_supply_names[i];
+
+	return devm_regulator_bulk_get(&client->dev, OV5670_NUM_SUPPLIES,
+				       ov5670->supplies);
+}
+
 static int ov5670_probe(struct i2c_client *client)
 {
 	struct ov5670 *ov5670;
@@ -2492,6 +2516,12 @@ static int ov5670_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
+	ret = ov5670_regulators_probe(ov5670);
+	if (ret) {
+		err_msg = "Regulators probe failed";
+		goto error_print;
+	}
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		/* Check module identity */
-- 
2.35.1

[PATCH 4/6] media: i2c: ov5670: Probe GPIOs

[Jacopo Mondi]

The OV5670 has a powerdown and reset pin, named respectively "PWDN" and
"XSHUTDOWN".

Optionally probe the gpios connected to the pins during the driver probe
routine.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index cba310aec011..ca5191d043ce 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1842,6 +1842,10 @@ struct ov5670 {
 	/* Regulators */
 	struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES];
 
+	/* Power-down and reset gpios. */
+	struct gpio_desc *pwdn_gpio; /* PWDNB pin. */
+	struct gpio_desc *reset_gpio; /* XSHUTDOWN pin. */
+
 	/* To serialize asynchronus callbacks */
 	struct mutex mutex;
 
@@ -2494,6 +2498,23 @@ static int ov5670_regulators_probe(struct ov5670 *ov5670)
 				       ov5670->supplies);
 }
 
+static int ov5670_gpio_probe(struct ov5670 *ov5670)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+
+	ov5670->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(ov5670->pwdn_gpio))
+		return PTR_ERR(ov5670->pwdn_gpio);
+
+	ov5670->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(ov5670->reset_gpio))
+		return PTR_ERR(ov5670->reset_gpio);
+
+	return 0;
+}
+
 static int ov5670_probe(struct i2c_client *client)
 {
 	struct ov5670 *ov5670;
@@ -2522,6 +2543,12 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	ret = ov5670_gpio_probe(ov5670);
+	if (ret) {
+		err_msg = "GPIO probe failed";
+		goto error_print;
+	}
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		/* Check module identity */
-- 
2.35.1

[PATCH 5/6] media: i2c: ov5670: Add runtime_pm operations

[Jacopo Mondi]

Implement the power up and power down routines and install them as
runtime_pm handler for runtime_suspend and runtime_resume operations.

Rework the runtime_pm enablement and the chip power handling during
probe, as calling pm_runtime_idle() in a driver that registers no
idle callback is a nop.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 59 ++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index ca5191d043ce..c9f69ffef210 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2,6 +2,8 @@
 // Copyright (c) 2017 Intel Corporation.
 
 #include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -2422,6 +2424,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int __maybe_unused ov5670_power_on(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5670 *ov5670 = to_ov5670(sd);
+	int ret = 0;
+
+	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
+	if (ret)
+		return ret;
+
+	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
+	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
+
+	/* 8192 * 2 clock pulses before the first SCCB transaction. */
+	usleep_range(1000, 1500);
+
+	return 0;
+}
+
+static int __maybe_unused ov5670_power_off(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5670 *ov5670 = to_ov5670(sd);
+
+	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
+	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
+	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
+
+	return 0;
+}
+
 static int __maybe_unused ov5670_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -2549,14 +2584,25 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	pm_runtime_enable(&client->dev);
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
+		ret = pm_runtime_resume_and_get(&client->dev);
+		if (ret) {
+			err_msg = "Failed to power on";
+			goto error_print;
+		}
+
 		/* Check module identity */
 		ret = ov5670_identify_module(ov5670);
 		if (ret) {
 			err_msg = "ov5670_identify_module() error";
-			goto error_print;
+			goto error_power_off;
 		}
+
+		/* Set the device's state to active if it's in D0 state. */
+		pm_runtime_set_active(&client->dev);
 	}
 
 	mutex_init(&ov5670->mutex);
@@ -2593,11 +2639,7 @@ static int ov5670_probe(struct i2c_client *client)
 
 	ov5670->streaming = false;
 
-	/* Set the device's state to active if it's in D0 state. */
-	if (full_power)
-		pm_runtime_set_active(&client->dev);
-	pm_runtime_enable(&client->dev);
-	pm_runtime_idle(&client->dev);
+	pm_runtime_suspend(&client->dev);
 
 	return 0;
 
@@ -2610,6 +2652,9 @@ static int ov5670_probe(struct i2c_client *client)
 error_mutex_destroy:
 	mutex_destroy(&ov5670->mutex);
 
+error_power_off:
+	pm_runtime_put(&client->dev);
+
 error_print:
 	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
 
@@ -2626,6 +2671,7 @@ static int ov5670_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	mutex_destroy(&ov5670->mutex);
 
+	pm_runtime_put(&client->dev);
 	pm_runtime_disable(&client->dev);
 
 	return 0;
@@ -2633,6 +2679,7 @@ static int ov5670_remove(struct i2c_client *client)
 
 static const struct dev_pm_ops ov5670_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
+	SET_RUNTIME_PM_OPS(ov5670_power_off, ov5670_power_on, NULL)
 };
 
 #ifdef CONFIG_ACPI
-- 
2.35.1

[PATCH 6/6] media: i2c: ov5670: Add .get_selection() support

[Jacopo Mondi]

From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

The ov5670 driver's v4l2_subdev_pad_ops currently does not include
.get_selection() - add support for that callback.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 64 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index c9f69ffef210..1fa0d632c536 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -70,6 +70,15 @@
 #define OV5670_REG_VALUE_16BIT		2
 #define OV5670_REG_VALUE_24BIT		3
 
+/* Pixel Array */
+
+#define OV5670_NATIVE_WIDTH		2624
+#define OV5670_NATIVE_HEIGHT		1980
+#define OV5670_ACTIVE_START_TOP		8
+#define OV5670_ACTIVE_START_LEFT	16
+#define OV5670_ACTIVE_WIDTH		2592
+#define OV5670_ACTIVE_HEIGHT		1944
+
 /* Initial number of frames to skip to avoid possible garbage */
 #define OV5670_NUM_OF_SKIP_FRAMES	2
 
@@ -2491,6 +2500,59 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
+static void
+__ov5670_get_pad_crop(struct ov5670 *sensor,
+		      struct v4l2_subdev_state *state, unsigned int pad,
+		      enum v4l2_subdev_format_whence which, struct v4l2_rect *r)
+{
+	const struct ov5670_mode *mode = sensor->cur_mode;
+
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		*r = *v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
+		break;
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		r->height = mode->height;
+		r->width = mode->width;
+		r->top = (OV5670_NATIVE_HEIGHT - mode->height) / 2;
+		r->left = (OV5670_NATIVE_WIDTH - mode->width) / 2;
+		break;
+	}
+}
+
+static int ov5670_get_selection(struct v4l2_subdev *subdev,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct ov5670 *sensor = to_ov5670(subdev);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		mutex_lock(&sensor->mutex);
+			__ov5670_get_pad_crop(sensor, state, sel->pad,
+					      sel->which, &sel->r);
+		mutex_unlock(&sensor->mutex);
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV5670_NATIVE_WIDTH;
+		sel->r.height = OV5670_NATIVE_HEIGHT;
+		break;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = OV5670_ACTIVE_START_TOP;
+		sel->r.left = OV5670_ACTIVE_START_LEFT;
+		sel->r.width = OV5670_ACTIVE_WIDTH;
+		sel->r.height = OV5670_ACTIVE_HEIGHT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_subdev_video_ops ov5670_video_ops = {
 	.s_stream = ov5670_set_stream,
 };
@@ -2500,6 +2562,8 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
 	.get_fmt = ov5670_get_pad_format,
 	.set_fmt = ov5670_set_pad_format,
 	.enum_frame_size = ov5670_enum_frame_size,
+	.get_selection = ov5670_get_selection,
+	.set_selection = ov5670_get_selection,
 };
 
 static const struct v4l2_subdev_sensor_ops ov5670_sensor_ops = {
-- 
2.35.1

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 10/03/2022 14:08, Jacopo Mondi wrote:
> Provide the bindings documentation for Omnivision OV5670 image sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++

Add the file to maintainers entry.

>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> new file mode 100644
> index 000000000000..dc4a3297bf6f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml

Missing vendor prefix in file name.

> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV5670 5 Megapixels raw image sensor
> +
> +maintainers:
> +  - Jacopo Mondi <jacopo@jmondi.org>

Please add also driver maintainer.

> +
> +description: |-
> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> +  controlled through an I2C compatible control bus.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov5670
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: Frequency of the xclk clock.

Is the xclk external clock coming to the sensor? If yes, there should be
a "clocks" property.

> +
> +  pwdn-gpios:
> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.

maxItems

> +
> +  reset-gpios:
> +    description:
> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.

maxItems

> +
> +  avdd-supply:
> +    description: Analog circuit power. Typically 2.8V.
> +
> +  dvdd-supply:
> +    description: Digital circuit power. Typically 1.2V.
> +
> +  dovdd-supply:
> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes:
> +            description: The sensor supports 1 or 2 data lanes operations.
> +            minItems: 1
> +            maxItems: 2
> +            items:
> +              maximum: 2

Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'

no clock-lanes?

> +
> +          clock-noncontinuous: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - clock-frequency
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov5670: sensor@36 {
> +            compatible = "ovti,ov5670";
> +            reg = <0x36>;
> +
> +            clock-frequency=<19200000>;

Missing spaces around '='.



Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Hi Krzysztof

On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2022 14:08, Jacopo Mondi wrote:
> > Provide the bindings documentation for Omnivision OV5670 image sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>
> Add the file to maintainers entry.
>

Right

> >  1 file changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> > new file mode 100644
> > index 000000000000..dc4a3297bf6f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>
> Missing vendor prefix in file name.
>

Right x2

> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV5670 5 Megapixels raw image sensor
> > +
> > +maintainers:
> > +  - Jacopo Mondi <jacopo@jmondi.org>
>
> Please add also driver maintainer.
>

I never got what the policy was, if the maintainer entries here only
refer to the binding file or to the driver too

> > +
> > +description: |-
> > +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > +  controlled through an I2C compatible control bus.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5670
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-frequency:
> > +    description: Frequency of the xclk clock.
>
> Is the xclk external clock coming to the sensor? If yes, there should be
> a "clocks" property.
>

To be honest I was not sure about this, as clock-frequency is already
used by the driver for the ACPI part, but it seems to in DT bindings
it is a property meant to be specified in the clock providers, even if
Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
really clarify this

Clock consumer should rather use 'clocks' and point to the provider's
phandle if my understanding is right.

> > +
> > +  pwdn-gpios:
> > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>
> maxItems
>

I thought it was not necessary with a single description: entry. But
looking at the dt-schema source I fail to find any commit mentioning
that.

> > +
> > +  reset-gpios:
> > +    description:
> > +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>
> maxItems
>
> > +
> > +  avdd-supply:
> > +    description: Analog circuit power. Typically 2.8V.
> > +
> > +  dvdd-supply:
> > +    description: Digital circuit power. Typically 1.2V.
> > +
> > +  dovdd-supply:
> > +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          data-lanes:
> > +            description: The sensor supports 1 or 2 data lanes operations.
> > +            minItems: 1
> > +            maxItems: 2
> > +            items:
> > +              maximum: 2
>
> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'

No 0 is not allowed, but the data-lanes properties should accept any
of the following combinations
        <1>
        <1 2>
        <2 1>

As the chip seems to support lane re-ordering.

using enum would allow to between <1> or <2> if I got it right?

as the data-lane property is defined in video-interfaces.yaml

  data-lanes:
    $ref: /schemas/types.yaml#/definitions/uint32-array
    minItems: 1
    maxItems: 8
    items:
      # Assume up to 9 physical lane indices
      maximum: 8
    description:
      An array of physical data lane indexes. Position of an entry determines
      the logical lane number, while the value of an entry indicates physical
      lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
      assuming the clock lane is on hardware lane 0. If the hardware does not
      support lane reordering, monotonically incremented values shall be used
      from 0 or 1 onwards, depending on whether or not there is also a clock
      lane. This property is valid for serial busses only (e.g. MIPI CSI-2).

I did the same but restricted the max number of items to 2, and the
maximum value to 2 as well

>
> no clock-lanes?
>

clock lane is fixed on lane #0 afaict
`
> > +
> > +          clock-noncontinuous: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clock-frequency
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov5670: sensor@36 {
> > +            compatible = "ovti,ov5670";
> > +            reg = <0x36>;
> > +
> > +            clock-frequency=<19200000>;
>
> Missing spaces around '='.

ouch, thanks for spotting

Thanks
  j

>
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 10/03/2022 18:16, Jacopo Mondi wrote:
> Hi Krzysztof
> 
> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
>> On 10/03/2022 14:08, Jacopo Mondi wrote:
>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>>
>> Add the file to maintainers entry.
>>
> 
> Right
> 
>>>  1 file changed, 93 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>> new file mode 100644
>>> index 000000000000..dc4a3297bf6f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>
>> Missing vendor prefix in file name.
>>
> 
> Right x2
> 
>>> @@ -0,0 +1,93 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>> +
>>> +maintainers:
>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>
>> Please add also driver maintainer.
>>
> 
> I never got what the policy was, if the maintainer entries here only
> refer to the binding file or to the driver too

It is a person responsible for the bindings, so indeed it might not feed
existing maintainer.

> 
>>> +
>>> +description: |-
>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>> +  controlled through an I2C compatible control bus.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ovti,ov5670
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-frequency:
>>> +    description: Frequency of the xclk clock.
>>
>> Is the xclk external clock coming to the sensor? If yes, there should be
>> a "clocks" property.
>>
> 
> To be honest I was not sure about this, as clock-frequency is already
> used by the driver for the ACPI part, but it seems to in DT bindings
> it is a property meant to be specified in the clock providers, even if
> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> really clarify this
> 
> Clock consumer should rather use 'clocks' and point to the provider's
> phandle if my understanding is right.

This is a clock-frequency, not clock reference. For external clocks, a
clock phandles + assigned-clock-rates should be rather used. However for
internal clocks, this is a perfectly valid property.

Therefore the question is - what is the "xclk"?

> 
>>> +
>>> +  pwdn-gpios:
>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>
>> maxItems
>>
> 
> I thought it was not necessary with a single description: entry. But
> looking at the dt-schema source I fail to find any commit mentioning
> that.

The purpose of maxItems is to constrain the number of GPIOs, so two
would be incorrect.

> 
>>> +
>>> +  reset-gpios:
>>> +    description:
>>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>>
>> maxItems
>>
>>> +
>>> +  avdd-supply:
>>> +    description: Analog circuit power. Typically 2.8V.
>>> +
>>> +  dvdd-supply:
>>> +    description: Digital circuit power. Typically 1.2V.
>>> +
>>> +  dovdd-supply:
>>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
>>> +
>>> +  port:
>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      endpoint:
>>> +        $ref: /schemas/media/video-interfaces.yaml#
>>> +        unevaluatedProperties: false
>>> +
>>> +        properties:
>>> +          data-lanes:
>>> +            description: The sensor supports 1 or 2 data lanes operations.
>>> +            minItems: 1
>>> +            maxItems: 2
>>> +            items:
>>> +              maximum: 2
>>
>> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
> 
> No 0 is not allowed, but the data-lanes properties should accept any
> of the following combinations
>         <1>
>         <1 2>
>         <2 1>
> 
> As the chip seems to support lane re-ordering.
> 
> using enum would allow to between <1> or <2> if I got it right?

Yeah, enum would be equivalent. I find it more readable, than min+max,
but it's not a strong preference.

> 
> as the data-lane property is defined in video-interfaces.yaml
> 
>   data-lanes:
>     $ref: /schemas/types.yaml#/definitions/uint32-array
>     minItems: 1
>     maxItems: 8
>     items:
>       # Assume up to 9 physical lane indices
>       maximum: 8
>     description:
>       An array of physical data lane indexes. Position of an entry determines
>       the logical lane number, while the value of an entry indicates physical
>       lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
>       assuming the clock lane is on hardware lane 0. If the hardware does not
>       support lane reordering, monotonically incremented values shall be used
>       from 0 or 1 onwards, depending on whether or not there is also a clock
>       lane. This property is valid for serial busses only (e.g. MIPI CSI-2).
> 
> I did the same but restricted the max number of items to 2, and the
> maximum value to 2 as well

Makes sense, but you should also restrict the minimum (so not 0). enum
solves this.

> 
>>
>> no clock-lanes?
>>
> 
> clock lane is fixed on lane #0 afaict

ok


Best regards,
Krzysztof

Re: [PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[kernel test robot]

Hi Jacopo,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.17-rc7 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-i2c-ov5670-OF-support-runtime_pm-regulators/20220310-211143
base:   git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (https://download.01.org/0day-ci/archive/20220311/202203110200.BVFJJTh4-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f5425877ffa93005c9f71ce9ce4185a787db66eb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jacopo-Mondi/media-i2c-ov5670-OF-support-runtime_pm-regulators/20220310-211143
        git checkout f5425877ffa93005c9f71ce9ce4185a787db66eb
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/media/i2c/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from drivers/media/i2c/ov5670.c:4:
>> drivers/media/i2c/ov5670.c:2601:48: error: 'ov5670_of_ids' undeclared here (not in a function); did you mean 'ov5670_acpi_ids'?
    2601 |                 .of_match_table = of_match_ptr(ov5670_of_ids),
         |                                                ^~~~~~~~~~~~~
   include/linux/of.h:411:34: note: in definition of macro 'of_match_ptr'
     411 | #define of_match_ptr(_ptr)      (_ptr)
         |                                  ^~~~


vim +2601 drivers/media/i2c/ov5670.c

  2595	
  2596	static struct i2c_driver ov5670_i2c_driver = {
  2597		.driver = {
  2598			.name = "ov5670",
  2599			.pm = &ov5670_pm_ops,
  2600			.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> 2601			.of_match_table = of_match_ptr(ov5670_of_ids),
  2602		},
  2603		.probe_new = ov5670_probe,
  2604		.remove = ov5670_remove,
  2605		.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
  2606	};
  2607	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[kernel test robot]

Hi Jacopo,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.17-rc7 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-i2c-ov5670-OF-support-runtime_pm-regulators/20220310-211143
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220311/202203110433.anTyFE5N-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 276ca87382b8f16a65bddac700202924228982f6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f5425877ffa93005c9f71ce9ce4185a787db66eb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jacopo-Mondi/media-i2c-ov5670-OF-support-runtime_pm-regulators/20220310-211143
        git checkout f5425877ffa93005c9f71ce9ce4185a787db66eb
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/media/i2c/ov5670.c:2601:34: error: use of undeclared identifier 'ov5670_of_ids'
                   .of_match_table = of_match_ptr(ov5670_of_ids),
                                                  ^
   1 error generated.


vim +/ov5670_of_ids +2601 drivers/media/i2c/ov5670.c

  2595	
  2596	static struct i2c_driver ov5670_i2c_driver = {
  2597		.driver = {
  2598			.name = "ov5670",
  2599			.pm = &ov5670_pm_ops,
  2600			.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> 2601			.of_match_table = of_match_ptr(ov5670_of_ids),
  2602		},
  2603		.probe_new = ov5670_probe,
  2604		.remove = ov5670_remove,
  2605		.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
  2606	};
  2607	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[kernel test robot]

Hi Jacopo,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.17-rc7 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-i2c-ov5670-OF-support-runtime_pm-regulators/20220310-211143
base:   git://linuxtv.org/media_tree.git master
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220311/202203110425.5nMUZpmG-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 276ca87382b8f16a65bddac700202924228982f6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f5425877ffa93005c9f71ce9ce4185a787db66eb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jacopo-Mondi/media-i2c-ov5670-OF-support-runtime_pm-regulators/20220310-211143
        git checkout f5425877ffa93005c9f71ce9ce4185a787db66eb
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/media/i2c/ov5670.c:2601:34: error: use of undeclared identifier 'ov5670_of_ids'
                   .of_match_table = of_match_ptr(ov5670_of_ids),
                                                  ^
   1 error generated.


vim +/ov5670_of_ids +2601 drivers/media/i2c/ov5670.c

  2595	
  2596	static struct i2c_driver ov5670_i2c_driver = {
  2597		.driver = {
  2598			.name = "ov5670",
  2599			.pm = &ov5670_pm_ops,
  2600			.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> 2601			.of_match_table = of_match_ptr(ov5670_of_ids),
  2602		},
  2603		.probe_new = ov5670_probe,
  2604		.remove = ov5670_remove,
  2605		.flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
  2606	};
  2607	

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Rob Herring]

On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2022 18:16, Jacopo Mondi wrote:
> > Hi Krzysztof
> > 
> > On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>
> >> Add the file to maintainers entry.
> >>
> > 
> > Right
> > 
> >>>  1 file changed, 93 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>> new file mode 100644
> >>> index 000000000000..dc4a3297bf6f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>
> >> Missing vendor prefix in file name.
> >>
> > 
> > Right x2
> > 
> >>> @@ -0,0 +1,93 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>> +
> >>> +maintainers:
> >>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Please add also driver maintainer.
> >>
> > 
> > I never got what the policy was, if the maintainer entries here only
> > refer to the binding file or to the driver too
> 
> It is a person responsible for the bindings, so indeed it might not feed
> existing maintainer.

No need for a MAINTAINERS entry as get_maintainers.pl will pick it up 
from here.

Rob

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Hi Krzysztof,

On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2022 18:16, Jacopo Mondi wrote:
> > Hi Krzysztof
> >
> > On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>
> >> Add the file to maintainers entry.
> >>
> >
> > Right
> >
> >>>  1 file changed, 93 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>> new file mode 100644
> >>> index 000000000000..dc4a3297bf6f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>
> >> Missing vendor prefix in file name.
> >>
> >
> > Right x2
> >
> >>> @@ -0,0 +1,93 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>> +
> >>> +maintainers:
> >>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Please add also driver maintainer.
> >>
> >
> > I never got what the policy was, if the maintainer entries here only
> > refer to the binding file or to the driver too
>
> It is a person responsible for the bindings, so indeed it might not feed
> existing maintainer.
>
> >
> >>> +
> >>> +description: |-
> >>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> >>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> >>> +  controlled through an I2C compatible control bus.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: ovti,ov5670
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  clock-frequency:
> >>> +    description: Frequency of the xclk clock.
> >>
> >> Is the xclk external clock coming to the sensor? If yes, there should be
> >> a "clocks" property.
> >>
> >
> > To be honest I was not sure about this, as clock-frequency is already
> > used by the driver for the ACPI part, but it seems to in DT bindings
> > it is a property meant to be specified in the clock providers, even if
> > Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> > really clarify this
> >
> > Clock consumer should rather use 'clocks' and point to the provider's
> > phandle if my understanding is right.
>
> This is a clock-frequency, not clock reference. For external clocks, a

Yes, I was suggesting to replace clock-frequency with clocks, that
accepts a phandle.

The thing is, the driver parses 'clock-frequency'
	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);

which I assume comes from ACPI (as the driver was developed for an
ACPI platform).

If in DTS we don't use it, I then need to

#ifdef CONFIG_ACPI

#elif defined CONFIG_OF

#endif

Which I would really like to avoid.

Anyone with ACPI experience that knows where clock-frequency comes
from ?

> clock phandles + assigned-clock-rates should be rather used. However for
> internal clocks, this is a perfectly valid property.
>
> Therefore the question is - what is the "xclk"?

xclk is the clock fed to the sensor, which which all its internal
clocks are generated, so it's indeed an 'external' clock. As I've
said, clock-frequency seems to be meant for clock providers, and
the image sensor is a clock consumer.

>
> >
> >>> +
> >>> +  pwdn-gpios:
> >>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> >>
> >> maxItems
> >>
> >
> > I thought it was not necessary with a single description: entry. But
> > looking at the dt-schema source I fail to find any commit mentioning
> > that.
>
> The purpose of maxItems is to constrain the number of GPIOs, so two
> would be incorrect.
>

I recall that with a single description entry then maxItems: 1 was
assumed by the dt-schema validator, but I cannot find references to
any commit, so I'll add it.

> >
> >>> +
> >>> +  reset-gpios:
> >>> +    description:
> >>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
> >>
> >> maxItems
> >>
> >>> +
> >>> +  avdd-supply:
> >>> +    description: Analog circuit power. Typically 2.8V.
> >>> +
> >>> +  dvdd-supply:
> >>> +    description: Digital circuit power. Typically 1.2V.
> >>> +
> >>> +  dovdd-supply:
> >>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> >>> +
> >>> +  port:
> >>> +    $ref: /schemas/graph.yaml#/$defs/port-base
> >>> +    additionalProperties: false
> >>> +
> >>> +    properties:
> >>> +      endpoint:
> >>> +        $ref: /schemas/media/video-interfaces.yaml#
> >>> +        unevaluatedProperties: false
> >>> +
> >>> +        properties:
> >>> +          data-lanes:
> >>> +            description: The sensor supports 1 or 2 data lanes operations.
> >>> +            minItems: 1
> >>> +            maxItems: 2
> >>> +            items:
> >>> +              maximum: 2
> >>
> >> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
> >
> > No 0 is not allowed, but the data-lanes properties should accept any
> > of the following combinations
> >         <1>
> >         <1 2>
> >         <2 1>
> >
> > As the chip seems to support lane re-ordering.
> >
> > using enum would allow to between <1> or <2> if I got it right?
>
> Yeah, enum would be equivalent. I find it more readable, than min+max,
> but it's not a strong preference.
>

I don't think enum is equivalent, as it specifies a set of valid values
a property can assume, but it does not support arrays.

https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.

enum
  The value of this keyword MUST be an array. This array SHOULD have
  at least one element. Elements in the array SHOULD be unique.

  An instance validates successfully against this keyword if its value
  is equal to one of the elements in this keyword's array value.

In facts:

--- a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
@@ -52,10 +52,7 @@ properties:
         properties:
           data-lanes:
             description: The sensor supports 1 or 2 data lanes operations.
-            minItems: 1
-            maxItems: 2
-            items:
-              maximum: 2
+            enum: [1, 2]


Results in:

Documentation/devicetree/bindings/media/i2c/ov5670.example.dt.yaml: sensor@36: port:endpoint:data-lanes:0: [1, 2] is too long

If instead

--- a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
@@ -54,8 +54,7 @@ properties:
             description: The sensor supports 1 or 2 data lanes operations.
             minItems: 1
             maxItems: 2
-            items:
-              maximum: 2
+            enum: [1, 2]

I get
Documentation/devicetree/bindings/media/i2c/ov5670.yaml:
properties:port:properties:endpoint:properties:data-lanes:
'enum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
	hint: Scalar and array keywords cannot be mixed
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

Thanks
   j

> >
> > as the data-lane property is defined in video-interfaces.yaml
> >
> >   data-lanes:
> >     $ref: /schemas/types.yaml#/definitions/uint32-array
> >     minItems: 1
> >     maxItems: 8
> >     items:
> >       # Assume up to 9 physical lane indices
> >       maximum: 8
> >     description:
> >       An array of physical data lane indexes. Position of an entry determines
> >       the logical lane number, while the value of an entry indicates physical
> >       lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
> >       assuming the clock lane is on hardware lane 0. If the hardware does not
> >       support lane reordering, monotonically incremented values shall be used
> >       from 0 or 1 onwards, depending on whether or not there is also a clock
> >       lane. This property is valid for serial busses only (e.g. MIPI CSI-2).
> >
> > I did the same but restricted the max number of items to 2, and the
> > maximum value to 2 as well
>
> Makes sense, but you should also restrict the minimum (so not 0). enum
> solves this.
>
> >
> >>
> >> no clock-lanes?
> >>
> >
> > clock lane is fixed on lane #0 afaict
>
> ok
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 11/03/2022 17:05, Jacopo Mondi wrote:
> Hi Krzysztof,
> 
> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
>> On 10/03/2022 18:16, Jacopo Mondi wrote:
>>> Hi Krzysztof
>>>
>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
>>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>>>>
>>>> Add the file to maintainers entry.
>>>>
>>>
>>> Right
>>>
>>>>>  1 file changed, 93 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..dc4a3297bf6f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>
>>>> Missing vendor prefix in file name.
>>>>
>>>
>>> Right x2
>>>
>>>>> @@ -0,0 +1,93 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>>>> +
>>>>> +maintainers:
>>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>>>
>>>> Please add also driver maintainer.
>>>>
>>>
>>> I never got what the policy was, if the maintainer entries here only
>>> refer to the binding file or to the driver too
>>
>> It is a person responsible for the bindings, so indeed it might not feed
>> existing maintainer.
>>
>>>
>>>>> +
>>>>> +description: |-
>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>>>> +  controlled through an I2C compatible control bus.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: ovti,ov5670
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clock-frequency:
>>>>> +    description: Frequency of the xclk clock.
>>>>
>>>> Is the xclk external clock coming to the sensor? If yes, there should be
>>>> a "clocks" property.
>>>>
>>>
>>> To be honest I was not sure about this, as clock-frequency is already
>>> used by the driver for the ACPI part, but it seems to in DT bindings
>>> it is a property meant to be specified in the clock providers, even if
>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
>>> really clarify this
>>>
>>> Clock consumer should rather use 'clocks' and point to the provider's
>>> phandle if my understanding is right.
>>
>> This is a clock-frequency, not clock reference. For external clocks, a
> 
> Yes, I was suggesting to replace clock-frequency with clocks, that
> accepts a phandle.
> 
> The thing is, the driver parses 'clock-frequency'
> 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> 
> which I assume comes from ACPI (as the driver was developed for an
> ACPI platform).
> 
> If in DTS we don't use it, I then need to
> 
> #ifdef CONFIG_ACPI
> 
> #elif defined CONFIG_OF
> 
> #endif
> 
> Which I would really like to avoid.
> 
> Anyone with ACPI experience that knows where clock-frequency comes
> from ?

I would assume that ACPI simply does not support common clock framework,
so it had to use clock-frequency. Several of such drivers were added by
folks from Intel which use ACPI, not Devicetree.

> 
>> clock phandles + assigned-clock-rates should be rather used. However for
>> internal clocks, this is a perfectly valid property.
>>
>> Therefore the question is - what is the "xclk"?
> 
> xclk is the clock fed to the sensor, which which all its internal
> clocks are generated, so it's indeed an 'external' clock. As I've
> said, clock-frequency seems to be meant for clock providers, and
> the image sensor is a clock consumer.

Regardless whether clock-frequency stays or not, you need the clocks
property in such case.

> 
>>
>>>
>>>>> +
>>>>> +  pwdn-gpios:
>>>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>>>
>>>> maxItems
>>>>
>>>
>>> I thought it was not necessary with a single description: entry. But
>>> looking at the dt-schema source I fail to find any commit mentioning
>>> that.
>>
>> The purpose of maxItems is to constrain the number of GPIOs, so two
>> would be incorrect.
>>
> 
> I recall that with a single description entry then maxItems: 1 was
> assumed by the dt-schema validator, but I cannot find references to
> any commit, so I'll add it.
> 
>>>
>>>>> +
>>>>> +  reset-gpios:
>>>>> +    description:
>>>>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>>>>
>>>> maxItems
>>>>
>>>>> +
>>>>> +  avdd-supply:
>>>>> +    description: Analog circuit power. Typically 2.8V.
>>>>> +
>>>>> +  dvdd-supply:
>>>>> +    description: Digital circuit power. Typically 1.2V.
>>>>> +
>>>>> +  dovdd-supply:
>>>>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
>>>>> +
>>>>> +  port:
>>>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>>>>> +    additionalProperties: false
>>>>> +
>>>>> +    properties:
>>>>> +      endpoint:
>>>>> +        $ref: /schemas/media/video-interfaces.yaml#
>>>>> +        unevaluatedProperties: false
>>>>> +
>>>>> +        properties:
>>>>> +          data-lanes:
>>>>> +            description: The sensor supports 1 or 2 data lanes operations.
>>>>> +            minItems: 1
>>>>> +            maxItems: 2
>>>>> +            items:
>>>>> +              maximum: 2
>>>>
>>>> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
>>>
>>> No 0 is not allowed, but the data-lanes properties should accept any
>>> of the following combinations
>>>         <1>
>>>         <1 2>
>>>         <2 1>
>>>
>>> As the chip seems to support lane re-ordering.
>>>
>>> using enum would allow to between <1> or <2> if I got it right?
>>
>> Yeah, enum would be equivalent. I find it more readable, than min+max,
>> but it's not a strong preference.
>>
> 
> I don't think enum is equivalent, as it specifies a set of valid values
> a property can assume, but it does not support arrays.

It is equivalent, just has to be used in equivalent way.

> 
> https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.
> 
> enum
>   The value of this keyword MUST be an array. This array SHOULD have
>   at least one element. Elements in the array SHOULD be unique.
> 
>   An instance validates successfully against this keyword if its value
>   is equal to one of the elements in this keyword's array value.
> > In facts:

That's not good usage. See for example:
Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml


Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Hi Krzysztof

On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote:
> On 11/03/2022 17:05, Jacopo Mondi wrote:
> > Hi Krzysztof,
> >
> > On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/03/2022 18:16, Jacopo Mondi wrote:
> >>> Hi Krzysztof
> >>>
> >>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>>>
> >>>> Add the file to maintainers entry.
> >>>>
> >>>
> >>> Right
> >>>
> >>>>>  1 file changed, 93 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..dc4a3297bf6f
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>
> >>>> Missing vendor prefix in file name.
> >>>>
> >>>
> >>> Right x2
> >>>
> >>>>> @@ -0,0 +1,93 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>>>
> >>>> Please add also driver maintainer.
> >>>>
> >>>
> >>> I never got what the policy was, if the maintainer entries here only
> >>> refer to the binding file or to the driver too
> >>
> >> It is a person responsible for the bindings, so indeed it might not feed
> >> existing maintainer.
> >>
> >>>
> >>>>> +
> >>>>> +description: |-
> >>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> >>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> >>>>> +  controlled through an I2C compatible control bus.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: ovti,ov5670
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  clock-frequency:
> >>>>> +    description: Frequency of the xclk clock.
> >>>>
> >>>> Is the xclk external clock coming to the sensor? If yes, there should be
> >>>> a "clocks" property.
> >>>>
> >>>
> >>> To be honest I was not sure about this, as clock-frequency is already
> >>> used by the driver for the ACPI part, but it seems to in DT bindings
> >>> it is a property meant to be specified in the clock providers, even if
> >>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> >>> really clarify this
> >>>
> >>> Clock consumer should rather use 'clocks' and point to the provider's
> >>> phandle if my understanding is right.
> >>
> >> This is a clock-frequency, not clock reference. For external clocks, a
> >
> > Yes, I was suggesting to replace clock-frequency with clocks, that
> > accepts a phandle.
> >
> > The thing is, the driver parses 'clock-frequency'
> > 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> >
> > which I assume comes from ACPI (as the driver was developed for an
> > ACPI platform).
> >
> > If in DTS we don't use it, I then need to
> >
> > #ifdef CONFIG_ACPI
> >
> > #elif defined CONFIG_OF
> >
> > #endif
> >
> > Which I would really like to avoid.
> >
> > Anyone with ACPI experience that knows where clock-frequency comes
> > from ?
>
> I would assume that ACPI simply does not support common clock framework,
> so it had to use clock-frequency. Several of such drivers were added by
> folks from Intel which use ACPI, not Devicetree.
>
> >
> >> clock phandles + assigned-clock-rates should be rather used. However for
> >> internal clocks, this is a perfectly valid property.
> >>
> >> Therefore the question is - what is the "xclk"?
> >
> > xclk is the clock fed to the sensor, which which all its internal
> > clocks are generated, so it's indeed an 'external' clock. As I've
> > said, clock-frequency seems to be meant for clock providers, and
> > the image sensor is a clock consumer.
>
> Regardless whether clock-frequency stays or not, you need the clocks
> property in such case.
>

Yes, I will have to ifdef in the driver if no better alternatives

> >
> >>
> >>>
> >>>>> +
> >>>>> +  pwdn-gpios:
> >>>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> >>>>
> >>>> maxItems
> >>>>
> >>>
> >>> I thought it was not necessary with a single description: entry. But
> >>> looking at the dt-schema source I fail to find any commit mentioning
> >>> that.
> >>
> >> The purpose of maxItems is to constrain the number of GPIOs, so two
> >> would be incorrect.
> >>
> >
> > I recall that with a single description entry then maxItems: 1 was
> > assumed by the dt-schema validator, but I cannot find references to
> > any commit, so I'll add it.
> >
> >>>
> >>>>> +
> >>>>> +  reset-gpios:
> >>>>> +    description:
> >>>>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
> >>>>
> >>>> maxItems
> >>>>
> >>>>> +
> >>>>> +  avdd-supply:
> >>>>> +    description: Analog circuit power. Typically 2.8V.
> >>>>> +
> >>>>> +  dvdd-supply:
> >>>>> +    description: Digital circuit power. Typically 1.2V.
> >>>>> +
> >>>>> +  dovdd-supply:
> >>>>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> >>>>> +
> >>>>> +  port:
> >>>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
> >>>>> +    additionalProperties: false
> >>>>> +
> >>>>> +    properties:
> >>>>> +      endpoint:
> >>>>> +        $ref: /schemas/media/video-interfaces.yaml#
> >>>>> +        unevaluatedProperties: false
> >>>>> +
> >>>>> +        properties:
> >>>>> +          data-lanes:
> >>>>> +            description: The sensor supports 1 or 2 data lanes operations.
> >>>>> +            minItems: 1
> >>>>> +            maxItems: 2
> >>>>> +            items:
> >>>>> +              maximum: 2
> >>>>
> >>>> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
> >>>
> >>> No 0 is not allowed, but the data-lanes properties should accept any
> >>> of the following combinations
> >>>         <1>
> >>>         <1 2>
> >>>         <2 1>
> >>>
> >>> As the chip seems to support lane re-ordering.
> >>>
> >>> using enum would allow to between <1> or <2> if I got it right?
> >>
> >> Yeah, enum would be equivalent. I find it more readable, than min+max,
> >> but it's not a strong preference.
> >>
> >
> > I don't think enum is equivalent, as it specifies a set of valid values
> > a property can assume, but it does not support arrays.
>
> It is equivalent, just has to be used in equivalent way.
>
> >
> > https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.
> >
> > enum
> >   The value of this keyword MUST be an array. This array SHOULD have
> >   at least one element. Elements in the array SHOULD be unique.
> >
> >   An instance validates successfully against this keyword if its value
> >   is equal to one of the elements in this keyword's array value.
> > > In facts:
>
> That's not good usage. See for example:
> Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml

Thanks, you're right.

        items:
          enum: [1, 2]

validates correctly.

Thanks for the suggestion!

>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 11/03/2022 19:00, Jacopo Mondi wrote:
> Hi Krzysztof
> 
> On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote:
>> On 11/03/2022 17:05, Jacopo Mondi wrote:
>>> Hi Krzysztof,
>>>
>>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
>>>> On 10/03/2022 18:16, Jacopo Mondi wrote:
>>>>> Hi Krzysztof
>>>>>
>>>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
>>>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>>>>>
>>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>>>>>>
>>>>>> Add the file to maintainers entry.
>>>>>>
>>>>>
>>>>> Right
>>>>>
>>>>>>>  1 file changed, 93 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..dc4a3297bf6f
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>>
>>>>>> Missing vendor prefix in file name.
>>>>>>
>>>>>
>>>>> Right x2
>>>>>
>>>>>>> @@ -0,0 +1,93 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>>>>>
>>>>>> Please add also driver maintainer.
>>>>>>
>>>>>
>>>>> I never got what the policy was, if the maintainer entries here only
>>>>> refer to the binding file or to the driver too
>>>>
>>>> It is a person responsible for the bindings, so indeed it might not feed
>>>> existing maintainer.
>>>>
>>>>>
>>>>>>> +
>>>>>>> +description: |-
>>>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>>>>>> +  controlled through an I2C compatible control bus.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: ovti,ov5670
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  clock-frequency:
>>>>>>> +    description: Frequency of the xclk clock.
>>>>>>
>>>>>> Is the xclk external clock coming to the sensor? If yes, there should be
>>>>>> a "clocks" property.
>>>>>>
>>>>>
>>>>> To be honest I was not sure about this, as clock-frequency is already
>>>>> used by the driver for the ACPI part, but it seems to in DT bindings
>>>>> it is a property meant to be specified in the clock providers, even if
>>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
>>>>> really clarify this
>>>>>
>>>>> Clock consumer should rather use 'clocks' and point to the provider's
>>>>> phandle if my understanding is right.
>>>>
>>>> This is a clock-frequency, not clock reference. For external clocks, a
>>>
>>> Yes, I was suggesting to replace clock-frequency with clocks, that
>>> accepts a phandle.
>>>
>>> The thing is, the driver parses 'clock-frequency'
>>> 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
>>>
>>> which I assume comes from ACPI (as the driver was developed for an
>>> ACPI platform).
>>>
>>> If in DTS we don't use it, I then need to
>>>
>>> #ifdef CONFIG_ACPI
>>>
>>> #elif defined CONFIG_OF
>>>
>>> #endif
>>>
>>> Which I would really like to avoid.
>>>
>>> Anyone with ACPI experience that knows where clock-frequency comes
>>> from ?
>>
>> I would assume that ACPI simply does not support common clock framework,
>> so it had to use clock-frequency. Several of such drivers were added by
>> folks from Intel which use ACPI, not Devicetree.
>>
>>>
>>>> clock phandles + assigned-clock-rates should be rather used. However for
>>>> internal clocks, this is a perfectly valid property.
>>>>
>>>> Therefore the question is - what is the "xclk"?
>>>
>>> xclk is the clock fed to the sensor, which which all its internal
>>> clocks are generated, so it's indeed an 'external' clock. As I've
>>> said, clock-frequency seems to be meant for clock providers, and
>>> the image sensor is a clock consumer.
>>
>> Regardless whether clock-frequency stays or not, you need the clocks
>> property in such case.
>>
> 
> Yes, I will have to ifdef in the driver if no better alternatives

I do not see the need of ifdefs... BTW, imx258 has exactly that case -
clock-frequency coming from ACPI world but not added to DT bindings.

Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Laurent Pinchart]

Hello,

On Sat, Mar 12, 2022 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
> On 11/03/2022 19:00, Jacopo Mondi wrote:
> > On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote:
> >> On 11/03/2022 17:05, Jacopo Mondi wrote:
> >>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 10/03/2022 18:16, Jacopo Mondi wrote:
> >>>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >>>>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>>>>>
> >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>>>>>
> >>>>>> Add the file to maintainers entry.
> >>>>>
> >>>>> Right
> >>>>>
> >>>>>>>  1 file changed, 93 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..dc4a3297bf6f
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>>
> >>>>>> Missing vendor prefix in file name.
> >>>>>
> >>>>> Right x2
> >>>>>
> >>>>>>> @@ -0,0 +1,93 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>
> >>>>>> Please add also driver maintainer.
> >>>>>
> >>>>> I never got what the policy was, if the maintainer entries here only
> >>>>> refer to the binding file or to the driver too
> >>>>
> >>>> It is a person responsible for the bindings, so indeed it might not feed
> >>>> existing maintainer.
> >>>>
> >>>>>>> +
> >>>>>>> +description: |-
> >>>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> >>>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> >>>>>>> +  controlled through an I2C compatible control bus.
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    const: ovti,ov5670
> >>>>>>> +
> >>>>>>> +  reg:
> >>>>>>> +    maxItems: 1
> >>>>>>> +
> >>>>>>> +  clock-frequency:
> >>>>>>> +    description: Frequency of the xclk clock.
> >>>>>>
> >>>>>> Is the xclk external clock coming to the sensor? If yes, there should be
> >>>>>> a "clocks" property.
> >>>>>
> >>>>> To be honest I was not sure about this, as clock-frequency is already
> >>>>> used by the driver for the ACPI part, but it seems to in DT bindings
> >>>>> it is a property meant to be specified in the clock providers, even if
> >>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> >>>>> really clarify this
> >>>>>
> >>>>> Clock consumer should rather use 'clocks' and point to the provider's
> >>>>> phandle if my understanding is right.
> >>>>
> >>>> This is a clock-frequency, not clock reference. For external clocks, a
> >>>
> >>> Yes, I was suggesting to replace clock-frequency with clocks, that
> >>> accepts a phandle.
> >>>
> >>> The thing is, the driver parses 'clock-frequency'
> >>> 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> >>>
> >>> which I assume comes from ACPI (as the driver was developed for an
> >>> ACPI platform).
> >>>
> >>> If in DTS we don't use it, I then need to
> >>>
> >>> #ifdef CONFIG_ACPI
> >>>
> >>> #elif defined CONFIG_OF
> >>>
> >>> #endif
> >>>
> >>> Which I would really like to avoid.
> >>>
> >>> Anyone with ACPI experience that knows where clock-frequency comes
> >>> from ?
> >>
> >> I would assume that ACPI simply does not support common clock framework,
> >> so it had to use clock-frequency. Several of such drivers were added by
> >> folks from Intel which use ACPI, not Devicetree.
> >>
> >>>> clock phandles + assigned-clock-rates should be rather used. However for
> >>>> internal clocks, this is a perfectly valid property.
> >>>>
> >>>> Therefore the question is - what is the "xclk"?
> >>>
> >>> xclk is the clock fed to the sensor, which which all its internal
> >>> clocks are generated, so it's indeed an 'external' clock. As I've
> >>> said, clock-frequency seems to be meant for clock providers, and
> >>> the image sensor is a clock consumer.
> >>
> >> Regardless whether clock-frequency stays or not, you need the clocks
> >> property in such case.
> > 
> > Yes, I will have to ifdef in the driver if no better alternatives
> 
> I do not see the need of ifdefs... BTW, imx258 has exactly that case -
> clock-frequency coming from ACPI world but not added to DT bindings.

The driver can call clk_get_rate() when a clock is provided, and use the
clock-frequency property otherwise. I also don't think conditional
compilation is needed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 10, 2022 at 02:08:25PM +0100, Jacopo Mondi wrote:
> The ov5670 driver currently only supports probing using ACPI matching.
> Add support for OF and add a missing header inclusion.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 02f75c18e480..39786f3c9489 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -3,7 +3,9 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/pm_runtime.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> @@ -2583,6 +2585,12 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
>  };
>  
>  MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
> +#elif defined CONFIG_OF

This should be

#ifdef CONFIG_OF
...
#endif

to support kernels compiled with both CONFIG_ACPI and CONFIG_OF.

With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +static const struct of_device_id ov5670_of_ids[] = {
> +	{ .compatible = "ovti,ov5670" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov5670_of_ids);
>  #endif
>  
>  static struct i2c_driver ov5670_i2c_driver = {
> @@ -2590,6 +2598,7 @@ static struct i2c_driver ov5670_i2c_driver = {
>  		.name = "ov5670",
>  		.pm = &ov5670_pm_ops,
>  		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> +		.of_match_table = of_match_ptr(ov5670_of_ids),
>  	},
>  	.probe_new = ov5670_probe,
>  	.remove = ov5670_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] media: i2c: ov5670: Probe regulators

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 10, 2022 at 02:08:26PM +0100, Jacopo Mondi wrote:
> The OV5670 has three power supplies (AVDD, DOVDD and DVDD).
> 
> Probe them in the driver to prepare controlling with runtime_pm
> operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 39786f3c9489..cba310aec011 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -7,6 +7,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -85,6 +86,14 @@ struct ov5670_link_freq_config {
>  	const struct ov5670_reg_list reg_list;
>  };
>  
> +static const char * const ov5670_supply_names[] = {
> +	"avdd",		/* Analog power */
> +	"dvdd",		/* Digital power */
> +	"dovdd",	/* Digital output power */
> +};
> +
> +#define OV5670_NUM_SUPPLIES ARRAY_SIZE(ov5670_supply_names)
> +
>  struct ov5670_mode {
>  	/* Frame width in pixels */
>  	u32 width;
> @@ -1830,6 +1839,9 @@ struct ov5670 {
>  	/* Current mode */
>  	const struct ov5670_mode *cur_mode;
>  
> +	/* Regulators */
> +	struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES];
> +
>  	/* To serialize asynchronus callbacks */
>  	struct mutex mutex;
>  
> @@ -2470,6 +2482,18 @@ static const struct v4l2_subdev_internal_ops ov5670_internal_ops = {
>  	.open = ov5670_open,
>  };
>  
> +static int ov5670_regulators_probe(struct ov5670 *ov5670)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
> +	unsigned int i;
> +
> +	for (i = 0; i < OV5670_NUM_SUPPLIES; i++)
> +		ov5670->supplies[i].supply = ov5670_supply_names[i];
> +
> +	return devm_regulator_bulk_get(&client->dev, OV5670_NUM_SUPPLIES,
> +				       ov5670->supplies);
> +}
> +
>  static int ov5670_probe(struct i2c_client *client)
>  {
>  	struct ov5670 *ov5670;
> @@ -2492,6 +2516,12 @@ static int ov5670_probe(struct i2c_client *client)
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
>  
> +	ret = ov5670_regulators_probe(ov5670);
> +	if (ret) {
> +		err_msg = "Regulators probe failed";

That's not common.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		goto error_print;
> +	}
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		/* Check module identity */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] media: i2c: ov5670: Probe GPIOs

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 10, 2022 at 02:08:27PM +0100, Jacopo Mondi wrote:
> The OV5670 has a powerdown and reset pin, named respectively "PWDN" and
> "XSHUTDOWN".
> 
> Optionally probe the gpios connected to the pins during the driver probe
> routine.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5670.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index cba310aec011..ca5191d043ce 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -1842,6 +1842,10 @@ struct ov5670 {
>  	/* Regulators */
>  	struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES];
>  
> +	/* Power-down and reset gpios. */
> +	struct gpio_desc *pwdn_gpio; /* PWDNB pin. */
> +	struct gpio_desc *reset_gpio; /* XSHUTDOWN pin. */
> +
>  	/* To serialize asynchronus callbacks */
>  	struct mutex mutex;
>  
> @@ -2494,6 +2498,23 @@ static int ov5670_regulators_probe(struct ov5670 *ov5670)
>  				       ov5670->supplies);
>  }
>  
> +static int ov5670_gpio_probe(struct ov5670 *ov5670)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
> +
> +	ov5670->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
> +						    GPIOD_OUT_LOW);
> +	if (IS_ERR(ov5670->pwdn_gpio))
> +		return PTR_ERR(ov5670->pwdn_gpio);
> +
> +	ov5670->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +						     GPIOD_OUT_LOW);
> +	if (IS_ERR(ov5670->reset_gpio))
> +		return PTR_ERR(ov5670->reset_gpio);
> +
> +	return 0;
> +}
> +
>  static int ov5670_probe(struct i2c_client *client)
>  {
>  	struct ov5670 *ov5670;
> @@ -2522,6 +2543,12 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	ret = ov5670_gpio_probe(ov5670);
> +	if (ret) {
> +		err_msg = "GPIO probe failed";
> +		goto error_print;
> +	}
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		/* Check module identity */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] media: i2c: ov5670: Add runtime_pm operations

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 10, 2022 at 02:08:28PM +0100, Jacopo Mondi wrote:
> Implement the power up and power down routines and install them as
> runtime_pm handler for runtime_suspend and runtime_resume operations.
> 
> Rework the runtime_pm enablement and the chip power handling during
> probe, as calling pm_runtime_idle() in a driver that registers no
> idle callback is a nop.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 59 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index ca5191d043ce..c9f69ffef210 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2,6 +2,8 @@
>  // Copyright (c) 2017 Intel Corporation.
>  
>  #include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>

Isn't this header needed in the previous patch ?

>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -2422,6 +2424,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int __maybe_unused ov5670_power_on(struct device *dev)

I'd name this ov5670_runtime_resume(), and the following function
ov5670_runtime_suspend().

> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +	int ret = 0;

No need to initialize ret.

> +
> +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> +
> +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> +	usleep_range(1000, 1500);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ov5670_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ov5670_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -2549,14 +2584,25 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	pm_runtime_enable(&client->dev);
> +
>  	full_power = acpi_dev_state_d0(&client->dev);

That's peculiar. Sakari, is this needed, or is there a way to rely on
runtime PM only ? Or on another API that would abstract the differences
between ACPI and OF ?

>  	if (full_power) {
> +		ret = pm_runtime_resume_and_get(&client->dev);
> +		if (ret) {
> +			err_msg = "Failed to power on";
> +			goto error_print;
> +		}
> +
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
>  		if (ret) {
>  			err_msg = "ov5670_identify_module() error";
> -			goto error_print;
> +			goto error_power_off;
>  		}
> +
> +		/* Set the device's state to active if it's in D0 state. */
> +		pm_runtime_set_active(&client->dev);
>  	}
>  
>  	mutex_init(&ov5670->mutex);
> @@ -2593,11 +2639,7 @@ static int ov5670_probe(struct i2c_client *client)
>  
>  	ov5670->streaming = false;
>  
> -	/* Set the device's state to active if it's in D0 state. */
> -	if (full_power)
> -		pm_runtime_set_active(&client->dev);
> -	pm_runtime_enable(&client->dev);
> -	pm_runtime_idle(&client->dev);
> +	pm_runtime_suspend(&client->dev);
>  
>  	return 0;
>  
> @@ -2610,6 +2652,9 @@ static int ov5670_probe(struct i2c_client *client)
>  error_mutex_destroy:
>  	mutex_destroy(&ov5670->mutex);
>  
> +error_power_off:
> +	pm_runtime_put(&client->dev);
> +
>  error_print:
>  	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
>  
> @@ -2626,6 +2671,7 @@ static int ov5670_remove(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  	mutex_destroy(&ov5670->mutex);
>  
> +	pm_runtime_put(&client->dev);
>  	pm_runtime_disable(&client->dev);
>  
>  	return 0;
> @@ -2633,6 +2679,7 @@ static int ov5670_remove(struct i2c_client *client)
>  
>  static const struct dev_pm_ops ov5670_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
> +	SET_RUNTIME_PM_OPS(ov5670_power_off, ov5670_power_on, NULL)
>  };
>  
>  #ifdef CONFIG_ACPI

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] media: i2c: ov5670: Add .get_selection() support

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 10, 2022 at 02:08:29PM +0100, Jacopo Mondi wrote:
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> The ov5670 driver's v4l2_subdev_pad_ops currently does not include
> .get_selection() - add support for that callback.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 64 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index c9f69ffef210..1fa0d632c536 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -70,6 +70,15 @@
>  #define OV5670_REG_VALUE_16BIT		2
>  #define OV5670_REG_VALUE_24BIT		3
>  
> +/* Pixel Array */
> +
> +#define OV5670_NATIVE_WIDTH		2624
> +#define OV5670_NATIVE_HEIGHT		1980
> +#define OV5670_ACTIVE_START_TOP		8
> +#define OV5670_ACTIVE_START_LEFT	16
> +#define OV5670_ACTIVE_WIDTH		2592
> +#define OV5670_ACTIVE_HEIGHT		1944
> +
>  /* Initial number of frames to skip to avoid possible garbage */
>  #define OV5670_NUM_OF_SKIP_FRAMES	2
>  
> @@ -2491,6 +2500,59 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  };
>  
> +static void
> +__ov5670_get_pad_crop(struct ov5670 *sensor,
> +		      struct v4l2_subdev_state *state, unsigned int pad,
> +		      enum v4l2_subdev_format_whence which, struct v4l2_rect *r)
> +{
> +	const struct ov5670_mode *mode = sensor->cur_mode;
> +
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		*r = *v4l2_subdev_get_try_crop(&sensor->sd, state, pad);

Where is the try crop rectangle initialized ?

> +		break;
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		r->height = mode->height;
> +		r->width = mode->width;
> +		r->top = (OV5670_NATIVE_HEIGHT - mode->height) / 2;
> +		r->left = (OV5670_NATIVE_WIDTH - mode->width) / 2;

There's a comment above stating

/*
 * OV5670 sensor supports following resolutions with full FOV:
 * 4:3  ==> {2592x1944, 1296x972, 648x486}
 * 16:9 ==> {2560x1440, 1280x720, 640x360}
 */

so this doesn't look right.

> +		break;
> +	}
> +}
> +
> +static int ov5670_get_selection(struct v4l2_subdev *subdev,
> +				struct v4l2_subdev_state *state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	struct ov5670 *sensor = to_ov5670(subdev);
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		mutex_lock(&sensor->mutex);
> +			__ov5670_get_pad_crop(sensor, state, sel->pad,
> +					      sel->which, &sel->r);

Wrong indentation.

> +		mutex_unlock(&sensor->mutex);
> +		break;
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = OV5670_NATIVE_WIDTH;
> +		sel->r.height = OV5670_NATIVE_HEIGHT;
> +		break;
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = OV5670_ACTIVE_START_TOP;
> +		sel->r.left = OV5670_ACTIVE_START_LEFT;
> +		sel->r.width = OV5670_ACTIVE_WIDTH;
> +		sel->r.height = OV5670_ACTIVE_HEIGHT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_video_ops ov5670_video_ops = {
>  	.s_stream = ov5670_set_stream,
>  };
> @@ -2500,6 +2562,8 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
>  	.get_fmt = ov5670_get_pad_format,
>  	.set_fmt = ov5670_set_pad_format,
>  	.enum_frame_size = ov5670_enum_frame_size,
> +	.get_selection = ov5670_get_selection,
> +	.set_selection = ov5670_get_selection,
>  };
>  
>  static const struct v4l2_subdev_sensor_ops ov5670_sensor_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[Jacopo Mondi]

Hi Laurent,

On Sun, Mar 13, 2022 at 04:33:12PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Mar 10, 2022 at 02:08:25PM +0100, Jacopo Mondi wrote:
> > The ov5670 driver currently only supports probing using ACPI matching.
> > Add support for OF and add a missing header inclusion.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5670.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index 02f75c18e480..39786f3c9489 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -3,7 +3,9 @@
> >
> >  #include <linux/acpi.h>
> >  #include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> >  #include <linux/pm_runtime.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> > @@ -2583,6 +2585,12 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
> >  };
> >
> >  MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
> > +#elif defined CONFIG_OF
>
> This should be
>
> #ifdef CONFIG_OF
> ...
> #endif
>
> to support kernels compiled with both CONFIG_ACPI and CONFIG_OF.

Actually, as kernel test robot reported, I should declare the id
tables unconditionally, and let of_match_ptr() and ACPI_PTR() macros
expand to NULL if the corresponding symbol is not defined

https://patchwork.linuxtv.org/project/linux-media/patch/20220310130829.96001-3-jacopo@jmondi.org/#135841

>
> With this fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks
   j

>
> > +static const struct of_device_id ov5670_of_ids[] = {
> > +	{ .compatible = "ovti,ov5670" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov5670_of_ids);
> >  #endif
> >
> >  static struct i2c_driver ov5670_i2c_driver = {
> > @@ -2590,6 +2598,7 @@ static struct i2c_driver ov5670_i2c_driver = {
> >  		.name = "ov5670",
> >  		.pm = &ov5670_pm_ops,
> >  		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> > +		.of_match_table = of_match_ptr(ov5670_of_ids),
> >  	},
> >  	.probe_new = ov5670_probe,
> >  	.remove = ov5670_remove,
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[Laurent Pinchart]

Hi Jacopo,

On Mon, Mar 14, 2022 at 09:42:08AM +0100, Jacopo Mondi wrote:
> On Sun, Mar 13, 2022 at 04:33:12PM +0200, Laurent Pinchart wrote:
> > On Thu, Mar 10, 2022 at 02:08:25PM +0100, Jacopo Mondi wrote:
> > > The ov5670 driver currently only supports probing using ACPI matching.
> > > Add support for OF and add a missing header inclusion.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ov5670.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > index 02f75c18e480..39786f3c9489 100644
> > > --- a/drivers/media/i2c/ov5670.c
> > > +++ b/drivers/media/i2c/ov5670.c
> > > @@ -3,7 +3,9 @@
> > >
> > >  #include <linux/acpi.h>
> > >  #include <linux/i2c.h>
> > > +#include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > > +#include <linux/of.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <media/v4l2-ctrls.h>
> > >  #include <media/v4l2-device.h>
> > > @@ -2583,6 +2585,12 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
> > >  };
> > >
> > >  MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
> > > +#elif defined CONFIG_OF
> >
> > This should be
> >
> > #ifdef CONFIG_OF
> > ...
> > #endif
> >
> > to support kernels compiled with both CONFIG_ACPI and CONFIG_OF.
> 
> Actually, as kernel test robot reported, I should declare the id
> tables unconditionally, and let of_match_ptr() and ACPI_PTR() macros
> expand to NULL if the corresponding symbol is not defined

With a __maybe_unused that should work too. I don't mind either way.

> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20220310130829.96001-3-jacopo@jmondi.org/#135841
> 
> > With this fixed,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks
> 
> > > +static const struct of_device_id ov5670_of_ids[] = {
> > > +	{ .compatible = "ovti,ov5670" },
> > > +	{ /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ov5670_of_ids);
> > >  #endif
> > >
> > >  static struct i2c_driver ov5670_i2c_driver = {
> > > @@ -2590,6 +2598,7 @@ static struct i2c_driver ov5670_i2c_driver = {
> > >  		.name = "ov5670",
> > >  		.pm = &ov5670_pm_ops,
> > >  		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> > > +		.of_match_table = of_match_ptr(ov5670_of_ids),
> > >  	},
> > >  	.probe_new = ov5670_probe,
> > >  	.remove = ov5670_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: i2c: ov5670: Allow probing with OF

[Laurent Pinchart]

On Mon, Mar 14, 2022 at 10:50:25AM +0200, Laurent Pinchart wrote:
> On Mon, Mar 14, 2022 at 09:42:08AM +0100, Jacopo Mondi wrote:
> > On Sun, Mar 13, 2022 at 04:33:12PM +0200, Laurent Pinchart wrote:
> > > On Thu, Mar 10, 2022 at 02:08:25PM +0100, Jacopo Mondi wrote:
> > > > The ov5670 driver currently only supports probing using ACPI matching.
> > > > Add support for OF and add a missing header inclusion.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/ov5670.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > > index 02f75c18e480..39786f3c9489 100644
> > > > --- a/drivers/media/i2c/ov5670.c
> > > > +++ b/drivers/media/i2c/ov5670.c
> > > > @@ -3,7 +3,9 @@
> > > >
> > > >  #include <linux/acpi.h>
> > > >  #include <linux/i2c.h>
> > > > +#include <linux/mod_devicetable.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/of.h>
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <media/v4l2-ctrls.h>
> > > >  #include <media/v4l2-device.h>
> > > > @@ -2583,6 +2585,12 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
> > > >  };
> > > >
> > > >  MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
> > > > +#elif defined CONFIG_OF
> > >
> > > This should be
> > >
> > > #ifdef CONFIG_OF
> > > ...
> > > #endif
> > >
> > > to support kernels compiled with both CONFIG_ACPI and CONFIG_OF.
> > 
> > Actually, as kernel test robot reported, I should declare the id
> > tables unconditionally, and let of_match_ptr() and ACPI_PTR() macros
> > expand to NULL if the corresponding symbol is not defined
> 
> With a __maybe_unused that should work too. I don't mind either way.

Actually, won't you always have the OF module device table then, even
when the kernel is compiled with !OF ? That may not hurt, but it's a
waste of space.

> > https://patchwork.linuxtv.org/project/linux-media/patch/20220310130829.96001-3-jacopo@jmondi.org/#135841
> > 
> > > With this fixed,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Thanks
> > 
> > > > +static const struct of_device_id ov5670_of_ids[] = {
> > > > +	{ .compatible = "ovti,ov5670" },
> > > > +	{ /* sentinel */ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, ov5670_of_ids);
> > > >  #endif
> > > >
> > > >  static struct i2c_driver ov5670_i2c_driver = {
> > > > @@ -2590,6 +2598,7 @@ static struct i2c_driver ov5670_i2c_driver = {
> > > >  		.name = "ov5670",
> > > >  		.pm = &ov5670_pm_ops,
> > > >  		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> > > > +		.of_match_table = of_match_ptr(ov5670_of_ids),
> > > >  	},
> > > >  	.probe_new = ov5670_probe,
> > > >  	.remove = ov5670_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] media: i2c: ov5670: Add .get_selection() support

[Jacopo Mondi]

On Sun, Mar 13, 2022 at 04:44:48PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Mar 10, 2022 at 02:08:29PM +0100, Jacopo Mondi wrote:
> > From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >
> > The ov5670 driver's v4l2_subdev_pad_ops currently does not include
> > .get_selection() - add support for that callback.
> >
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5670.c | 64 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index c9f69ffef210..1fa0d632c536 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -70,6 +70,15 @@
> >  #define OV5670_REG_VALUE_16BIT		2
> >  #define OV5670_REG_VALUE_24BIT		3
> >
> > +/* Pixel Array */
> > +
> > +#define OV5670_NATIVE_WIDTH		2624
> > +#define OV5670_NATIVE_HEIGHT		1980
> > +#define OV5670_ACTIVE_START_TOP		8
> > +#define OV5670_ACTIVE_START_LEFT	16
> > +#define OV5670_ACTIVE_WIDTH		2592
> > +#define OV5670_ACTIVE_HEIGHT		1944
> > +
> >  /* Initial number of frames to skip to avoid possible garbage */
> >  #define OV5670_NUM_OF_SKIP_FRAMES	2
> >
> > @@ -2491,6 +2500,59 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
> >  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> >  };
> >
> > +static void
> > +__ov5670_get_pad_crop(struct ov5670 *sensor,
> > +		      struct v4l2_subdev_state *state, unsigned int pad,
> > +		      enum v4l2_subdev_format_whence which, struct v4l2_rect *r)
> > +{
> > +	const struct ov5670_mode *mode = sensor->cur_mode;
> > +
> > +	switch (which) {
> > +	case V4L2_SUBDEV_FORMAT_TRY:
> > +		*r = *v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
>
> Where is the try crop rectangle initialized ?
>

I'll implement init_cfg

> > +		break;
> > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +		r->height = mode->height;
> > +		r->width = mode->width;
> > +		r->top = (OV5670_NATIVE_HEIGHT - mode->height) / 2;
> > +		r->left = (OV5670_NATIVE_WIDTH - mode->width) / 2;
>
> There's a comment above stating
>
> /*
>  * OV5670 sensor supports following resolutions with full FOV:
>  * 4:3  ==> {2592x1944, 1296x972, 648x486}
>  * 16:9 ==> {2560x1440, 1280x720, 640x360}
>  */
>
> so this doesn't look right.
>

You're right, all modes are obtained by subsampling the full pixel
array, so this is not right.

Using Figure 4.2 as a reference, all the modes in the driver use:

        H_crop_start = 0x0c
        V_crop_start = 0x04
        H_crop_end = 0xa33 (2611)
        V_crop_end = 0x733 (1843)

So I think the crop rectangle can be hardcoded for all modes to:

        .left = 12
        .top = 4
        .width = 2600
        .height = 1840

> > +		break;
> > +	}
> > +}
> > +
> > +static int ov5670_get_selection(struct v4l2_subdev *subdev,
> > +				struct v4l2_subdev_state *state,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	struct ov5670 *sensor = to_ov5670(subdev);
> > +
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +		mutex_lock(&sensor->mutex);
> > +			__ov5670_get_pad_crop(sensor, state, sel->pad,
> > +					      sel->which, &sel->r);
>
> Wrong indentation.
>

Thought that was intentional from Jean-Michel to highlight the
critical section, but I can re-indent back.

Thanks
   j

> > +		mutex_unlock(&sensor->mutex);
> > +		break;
> > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +		sel->r.top = 0;
> > +		sel->r.left = 0;
> > +		sel->r.width = OV5670_NATIVE_WIDTH;
> > +		sel->r.height = OV5670_NATIVE_HEIGHT;
> > +		break;
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +		sel->r.top = OV5670_ACTIVE_START_TOP;
> > +		sel->r.left = OV5670_ACTIVE_START_LEFT;
> > +		sel->r.width = OV5670_ACTIVE_WIDTH;
> > +		sel->r.height = OV5670_ACTIVE_HEIGHT;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct v4l2_subdev_video_ops ov5670_video_ops = {
> >  	.s_stream = ov5670_set_stream,
> >  };
> > @@ -2500,6 +2562,8 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
> >  	.get_fmt = ov5670_get_pad_format,
> >  	.set_fmt = ov5670_set_pad_format,
> >  	.enum_frame_size = ov5670_enum_frame_size,
> > +	.get_selection = ov5670_get_selection,
> > +	.set_selection = ov5670_get_selection,
> >  };
> >
> >  static const struct v4l2_subdev_sensor_ops ov5670_sensor_ops = {
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 6/6] media: i2c: ov5670: Add .get_selection() support

[Jacopo Mondi]

One correction

On Mon, Mar 14, 2022 at 02:30:17PM +0100, Jacopo Mondi wrote:
> On Sun, Mar 13, 2022 at 04:44:48PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Thu, Mar 10, 2022 at 02:08:29PM +0100, Jacopo Mondi wrote:
> > > From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > >
> > > The ov5670 driver's v4l2_subdev_pad_ops currently does not include
> > > .get_selection() - add support for that callback.
> > >
> > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ov5670.c | 64 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 64 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > index c9f69ffef210..1fa0d632c536 100644
> > > --- a/drivers/media/i2c/ov5670.c
> > > +++ b/drivers/media/i2c/ov5670.c
> > > @@ -70,6 +70,15 @@
> > >  #define OV5670_REG_VALUE_16BIT		2
> > >  #define OV5670_REG_VALUE_24BIT		3
> > >
> > > +/* Pixel Array */
> > > +
> > > +#define OV5670_NATIVE_WIDTH		2624
> > > +#define OV5670_NATIVE_HEIGHT		1980
> > > +#define OV5670_ACTIVE_START_TOP		8
> > > +#define OV5670_ACTIVE_START_LEFT	16
> > > +#define OV5670_ACTIVE_WIDTH		2592
> > > +#define OV5670_ACTIVE_HEIGHT		1944
> > > +
> > >  /* Initial number of frames to skip to avoid possible garbage */
> > >  #define OV5670_NUM_OF_SKIP_FRAMES	2
> > >
> > > @@ -2491,6 +2500,59 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
> > >  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > >  };
> > >
> > > +static void
> > > +__ov5670_get_pad_crop(struct ov5670 *sensor,
> > > +		      struct v4l2_subdev_state *state, unsigned int pad,
> > > +		      enum v4l2_subdev_format_whence which, struct v4l2_rect *r)
> > > +{
> > > +	const struct ov5670_mode *mode = sensor->cur_mode;
> > > +
> > > +	switch (which) {
> > > +	case V4L2_SUBDEV_FORMAT_TRY:
> > > +		*r = *v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
> >
> > Where is the try crop rectangle initialized ?
> >
>
> I'll implement init_cfg
>
> > > +		break;
> > > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > > +		r->height = mode->height;
> > > +		r->width = mode->width;
> > > +		r->top = (OV5670_NATIVE_HEIGHT - mode->height) / 2;
> > > +		r->left = (OV5670_NATIVE_WIDTH - mode->width) / 2;
> >
> > There's a comment above stating
> >
> > /*
> >  * OV5670 sensor supports following resolutions with full FOV:
> >  * 4:3  ==> {2592x1944, 1296x972, 648x486}
> >  * 16:9 ==> {2560x1440, 1280x720, 640x360}
> >  */
> >
> > so this doesn't look right.
> >
>
> You're right, all modes are obtained by subsampling the full pixel
> array, so this is not right.
>
> Using Figure 4.2 as a reference, all the modes in the driver use:
>
>         H_crop_start = 0x0c
>         V_crop_start = 0x04
>         H_crop_end = 0xa33 (2611)
>         V_crop_end = 0x733 (1843)

This should be
         V_crop_end = 0x7a3 (1955)

>
> So I think the crop rectangle can be hardcoded for all modes to:
>
>         .left = 12
>         .top = 4
>         .width = 2600
>         .height = 1840

          .height = 1952

Which seems more likely :)

>
> > > +		break;
> > > +	}
> > > +}
> > > +
> > > +static int ov5670_get_selection(struct v4l2_subdev *subdev,
> > > +				struct v4l2_subdev_state *state,
> > > +				struct v4l2_subdev_selection *sel)
> > > +{
> > > +	struct ov5670 *sensor = to_ov5670(subdev);
> > > +
> > > +	switch (sel->target) {
> > > +	case V4L2_SEL_TGT_CROP:
> > > +		mutex_lock(&sensor->mutex);
> > > +			__ov5670_get_pad_crop(sensor, state, sel->pad,
> > > +					      sel->which, &sel->r);
> >
> > Wrong indentation.
> >
>
> Thought that was intentional from Jean-Michel to highlight the
> critical section, but I can re-indent back.
>
> Thanks
>    j
>
> > > +		mutex_unlock(&sensor->mutex);
> > > +		break;
> > > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > > +		sel->r.top = 0;
> > > +		sel->r.left = 0;
> > > +		sel->r.width = OV5670_NATIVE_WIDTH;
> > > +		sel->r.height = OV5670_NATIVE_HEIGHT;
> > > +		break;
> > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +		sel->r.top = OV5670_ACTIVE_START_TOP;
> > > +		sel->r.left = OV5670_ACTIVE_START_LEFT;
> > > +		sel->r.width = OV5670_ACTIVE_WIDTH;
> > > +		sel->r.height = OV5670_ACTIVE_HEIGHT;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct v4l2_subdev_video_ops ov5670_video_ops = {
> > >  	.s_stream = ov5670_set_stream,
> > >  };
> > > @@ -2500,6 +2562,8 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
> > >  	.get_fmt = ov5670_get_pad_format,
> > >  	.set_fmt = ov5670_set_pad_format,
> > >  	.enum_frame_size = ov5670_enum_frame_size,
> > > +	.get_selection = ov5670_get_selection,
> > > +	.set_selection = ov5670_get_selection,
> > >  };
> > >
> > >  static const struct v4l2_subdev_sensor_ops ov5670_sensor_ops = {
> >
> > --
> > Regards,
> >
> > Laurent Pinchart