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

[PATCH v2 0/8] 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 :)

v1->v2:
- Address Krzysztof comments on bindings
- 2/8: new patch to use the common clock framework
- Address Lauren's comment on runtime_pm function names
- 7/8: new patch to implement init_cfg as suggested by Laurent
- Rework 8/8 which was incorrect as reported by Laurent

Thanks
   j

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

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

 .../bindings/media/i2c/ovti,ov5670.yaml       |  99 ++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov5670.c                    | 281 +++++++++++++++---
 3 files changed, 340 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml

--
2.35.1

[PATCH v2 1/8] 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>

---
v1->v2 (comments from Krzysztof)

- Rename to include manufacturer name
- Add entry to MAINTAINERS
- Add maxItems: to -gpios properties
- Use common clock properties
- Use enum: [1, 2] for data lanes
- Fix whitespace issue in example
---

 .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
new file mode 100644
index 000000000000..73cf72203f17
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,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
+
+  assigned-clocks: true
+  assigned-clock-parents: true
+  assigned-clock-rates: true
+
+  clocks:
+    description: System clock. From 6 to 27 MHz.
+    maxItems: 1
+
+  pwdn-gpios:
+    description: Reference to the GPIO connected to the PWDNB pin. Active low.
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
+    maxItems: 1
+
+  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:
+              enum: [1, 2]
+
+          clock-noncontinuous: true
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5670: sensor@36 {
+            compatible = "ovti,ov5670";
+            reg = <0x36>;
+            clocks = <&sensor_xclk>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&csi_ep>;
+                    data-lanes = <1 2>;
+                    clock-noncontinuous;
+                };
+            };
+        };
+    };
+
+...
+
diff --git a/MAINTAINERS b/MAINTAINERS
index 83d27b57016f..6aa32609a3cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14259,6 +14259,7 @@ M:	Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
 F:	drivers/media/i2c/ov5670.c

 OMNIVISION OV5675 SENSOR DRIVER
--
2.35.1

[PATCH v2 2/8] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 02f75c18e480..721441024598 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>
@@ -2585,11 +2587,20 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
 #endif
 
+#ifdef 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 = {
 	.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 v2 3/8] media: i2c: ov5670: Probe clocks with OF

[Jacopo Mondi]

Add support for probing the main system clock using the common clock
framework and its OF bindings.

Maintain ACPI compatibility by falling back to parse 'clock-frequency'
if the no clock device reference is available.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 721441024598..25d792794fc7 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2,6 +2,7 @@
 // Copyright (c) 2017 Intel Corporation.
 
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -1819,6 +1820,8 @@ struct ov5670 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
+	struct clk *clk;
+
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
@@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client)
 	bool full_power;
 	int ret;
 
-	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
-	if (input_clk != 19200000)
-		return -EINVAL;
-
 	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
 	if (!ov5670) {
 		ret = -ENOMEM;
@@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
+	ov5670->clk = devm_clk_get_optional(&client->dev, NULL);
+	if (IS_ERR(ov5670->clk))
+		return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk),
+				     "error getting clock\n");
+
+	if (ov5670->clk)
+		input_clk = clk_get_rate(ov5670->clk);
+	else
+		device_property_read_u32(&client->dev, "clock-frequency",
+					 &input_clk);
+	if (input_clk != 19200000)
+		return -EINVAL;
+
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
-- 
2.35.1

[PATCH v2 4/8] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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 25d792794fc7..832355f65e52 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -8,6 +8,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>
@@ -86,6 +87,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;
@@ -1833,6 +1842,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;
 
@@ -2473,6 +2485,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;
@@ -2505,6 +2529,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 v2 5/8] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 832355f65e52..1ecc3e549737 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -3,6 +3,7 @@
 
 #include <linux/acpi.h>
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -1845,6 +1846,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;
 
@@ -2497,6 +2502,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;
@@ -2535,6 +2557,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 v2 6/8] 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 | 58 ++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 1ecc3e549737..c3f773524d5f 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -4,6 +4,7 @@
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -2426,6 +2427,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int __maybe_unused ov5670_runtime_resume(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;
+
+	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_runtime_suspend(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);
@@ -2563,14 +2597,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);
@@ -2607,11 +2652,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;
 
@@ -2624,6 +2665,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);
 
@@ -2640,6 +2684,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;
@@ -2647,6 +2692,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_runtime_suspend, ov5670_runtime_resume, NULL)
 };
 
 #ifdef CONFIG_ACPI
-- 
2.35.1

[PATCH v2 7/8] media: i2c: ov5670: Implement init_cfg

[Jacopo Mondi]

Implement the .init_cfg() pad operation and initialize the default
format.

With .init_cfg() pad operation implemented the deprecated .open()
internal operation can now be dropped.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index c3f773524d5f..9aa82774f8a6 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1958,27 +1958,6 @@ static int ov5670_write_reg_list(struct ov5670 *ov5670,
 	return ov5670_write_regs(ov5670, r_list->regs, r_list->num_of_regs);
 }
 
-/* Open sub-device */
-static int ov5670_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
-{
-	struct ov5670 *ov5670 = to_ov5670(sd);
-	struct v4l2_mbus_framefmt *try_fmt =
-				v4l2_subdev_get_try_format(sd, fh->state, 0);
-
-	mutex_lock(&ov5670->mutex);
-
-	/* Initialize try_fmt */
-	try_fmt->width = ov5670->cur_mode->width;
-	try_fmt->height = ov5670->cur_mode->height;
-	try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
-	try_fmt->field = V4L2_FIELD_NONE;
-
-	/* No crop or compose */
-	mutex_unlock(&ov5670->mutex);
-
-	return 0;
-}
-
 static int ov5670_update_digital_gain(struct ov5670 *ov5670, u32 d_gain)
 {
 	int ret;
@@ -2178,6 +2157,25 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 	return ret;
 }
 
+static int ov5670_init_cfg(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_state *state)
+{
+	struct v4l2_mbus_framefmt *fmt =
+				v4l2_subdev_get_try_format(sd, state, 0);
+	const struct ov5670_mode *default_mode = &supported_modes[0];
+
+	fmt->width = default_mode->width;
+	fmt->height = default_mode->height;
+	fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB);
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB);
+
+	return 0;
+}
+
 static int ov5670_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -2499,6 +2497,7 @@ static const struct v4l2_subdev_video_ops ov5670_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
+	.init_cfg = ov5670_init_cfg,
 	.enum_mbus_code = ov5670_enum_mbus_code,
 	.get_fmt = ov5670_get_pad_format,
 	.set_fmt = ov5670_set_pad_format,
@@ -2520,10 +2519,6 @@ static const struct media_entity_operations ov5670_subdev_entity_ops = {
 	.link_validate = v4l2_subdev_link_validate,
 };
 
-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);
@@ -2629,7 +2624,6 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_mutex_destroy;
 	}
 
-	ov5670->sd.internal_ops = &ov5670_internal_ops;
 	ov5670->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov5670->sd.entity.ops = &ov5670_subdev_entity_ops;
-- 
2.35.1

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

[Jacopo Mondi]

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

Add support for the .get_selection() pad operation to the ov5670 sensor
driver.

Report the native sensor size (pixel array), the crop bounds (readable
pixel array area) and the current and default analog crop rectangles.

Currently all driver's mode use an analog crop rectangle of size
[12, 4, 2600, 1952]. Instead of hardcoding the value in the operation
implementation, ad an .analog_crop field to the sensor's mode
definitions, to make sure that if any mode gets added, its crop
rectangle will be defined as well.

While at it re-sort the mode's field definition order to match the
declaration order and initialize the crop rectangle in init_cfg().

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 9aa82774f8a6..67897dabb712 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -71,6 +71,10 @@
 #define OV5670_REG_VALUE_16BIT		2
 #define OV5670_REG_VALUE_24BIT		3
 
+/* Pixel Array */
+#define OV5670_NATIVE_WIDTH		2624
+#define OV5670_NATIVE_HEIGHT		1980
+
 /* Initial number of frames to skip to avoid possible garbage */
 #define OV5670_NUM_OF_SKIP_FRAMES	2
 
@@ -113,10 +117,25 @@ struct ov5670_mode {
 	/* Link frequency needed for this resolution */
 	u32 link_freq_index;
 
+	/* Analog crop rectangle */
+	const struct v4l2_rect analog_crop;
+
 	/* Sensor register settings for this resolution */
 	const struct ov5670_reg_list reg_list;
 };
 
+/*
+ * All the modes supported by the driver are obtained by subsampling the
+ * full pixel array. The below values are reflected in registers from
+ * 03800-0x3807 in the modes register-value tables.
+ */
+static const struct v4l2_rect ov5670_analog_crop = {
+	.left	= 12,
+	.top	= 4,
+	.width	= 2600,
+	.height	= 1952,
+};
+
 static const struct ov5670_reg mipi_data_rate_840mbps[] = {
 	{0x0300, 0x04},
 	{0x0301, 0x00},
@@ -1764,66 +1783,73 @@ static const struct ov5670_mode supported_modes[] = {
 		.height = 1944,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = OV5670_VTS_30FPS,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
 			.regs = mode_2592x1944_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 1296,
 		.height = 972,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 996,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1296x972_regs),
 			.regs = mode_1296x972_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 648,
 		.height = 486,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 516,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_648x486_regs),
 			.regs = mode_648x486_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 2560,
 		.height = 1440,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = OV5670_VTS_30FPS,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_2560x1440_regs),
 			.regs = mode_2560x1440_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 1280,
 		.height = 720,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 1020,
+
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
 			.regs = mode_1280x720_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 640,
 		.height = 360,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 510,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_640x360_regs),
 			.regs = mode_640x360_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	}
 };
 
@@ -2163,6 +2189,7 @@ static int ov5670_init_cfg(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *fmt =
 				v4l2_subdev_get_try_format(sd, state, 0);
 	const struct ov5670_mode *default_mode = &supported_modes[0];
+	struct v4l2_rect *crop = v4l2_subdev_get_try_crop(sd, state, 0);
 
 	fmt->width = default_mode->width;
 	fmt->height = default_mode->height;
@@ -2173,6 +2200,8 @@ static int ov5670_init_cfg(struct v4l2_subdev *sd,
 	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB);
 
+	*crop = default_mode->analog_crop;
+
 	return 0;
 }
 
@@ -2492,6 +2521,52 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
+static const struct v4l2_rect *
+__ov5670_get_pad_crop(struct ov5670 *sensor, struct v4l2_subdev_state *state,
+		      unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	const struct ov5670_mode *mode = sensor->cur_mode;
+
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &mode->analog_crop;
+	}
+
+	return NULL;
+}
+
+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);
+		sel->r = *__ov5670_get_pad_crop(sensor, state, sel->pad,
+						sel->which);
+		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 = ov5670_analog_crop;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_subdev_video_ops ov5670_video_ops = {
 	.s_stream = ov5670_set_stream,
 };
@@ -2502,6 +2577,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 v2 8/8] media: i2c: ov5670: Add .get_selection() support

[kernel test robot]

Hi Jacopo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v5.17-rc8 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/20220315-003034
base:   git://linuxtv.org/media_tree.git master
config: i386-randconfig-a012-20220314 (https://download.01.org/0day-ci/archive/20220315/202203151238.pCKNarov-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 3e4950d7fa78ac83f33bbf1658e2f49a73719236)
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/c619a8eee6477517dfaa05511344d0bddc4e1c55
        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/20220315-003034
        git checkout c619a8eee6477517dfaa05511344d0bddc4e1c55
        # 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:1787:18: error: initializer element is not a compile-time constant
                   .analog_crop = ov5670_analog_crop,
                                  ^~~~~~~~~~~~~~~~~~
   1 error generated.


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

  1774	
  1775	/*
  1776	 * OV5670 sensor supports following resolutions with full FOV:
  1777	 * 4:3  ==> {2592x1944, 1296x972, 648x486}
  1778	 * 16:9 ==> {2560x1440, 1280x720, 640x360}
  1779	 */
  1780	static const struct ov5670_mode supported_modes[] = {
  1781		{
  1782			.width = 2592,
  1783			.height = 1944,
  1784			.vts_def = OV5670_VTS_30FPS,
  1785			.vts_min = OV5670_VTS_30FPS,
  1786			.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
> 1787			.analog_crop = ov5670_analog_crop,
  1788			.reg_list = {
  1789				.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
  1790				.regs = mode_2592x1944_regs,
  1791			},
  1792		},
  1793		{
  1794			.width = 1296,
  1795			.height = 972,
  1796			.vts_def = OV5670_VTS_30FPS,
  1797			.vts_min = 996,
  1798			.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
  1799			.analog_crop = ov5670_analog_crop,
  1800			.reg_list = {
  1801				.num_of_regs = ARRAY_SIZE(mode_1296x972_regs),
  1802				.regs = mode_1296x972_regs,
  1803			},
  1804		},
  1805		{
  1806			.width = 648,
  1807			.height = 486,
  1808			.vts_def = OV5670_VTS_30FPS,
  1809			.vts_min = 516,
  1810			.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
  1811			.analog_crop = ov5670_analog_crop,
  1812			.reg_list = {
  1813				.num_of_regs = ARRAY_SIZE(mode_648x486_regs),
  1814				.regs = mode_648x486_regs,
  1815			},
  1816		},
  1817		{
  1818			.width = 2560,
  1819			.height = 1440,
  1820			.vts_def = OV5670_VTS_30FPS,
  1821			.vts_min = OV5670_VTS_30FPS,
  1822			.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
  1823			.analog_crop = ov5670_analog_crop,
  1824			.reg_list = {
  1825				.num_of_regs = ARRAY_SIZE(mode_2560x1440_regs),
  1826				.regs = mode_2560x1440_regs,
  1827			},
  1828		},
  1829		{
  1830			.width = 1280,
  1831			.height = 720,
  1832			.vts_def = OV5670_VTS_30FPS,
  1833			.vts_min = 1020,
  1834	
  1835			.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
  1836			.analog_crop = ov5670_analog_crop,
  1837			.reg_list = {
  1838				.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
  1839				.regs = mode_1280x720_regs,
  1840			},
  1841		},
  1842		{
  1843			.width = 640,
  1844			.height = 360,
  1845			.vts_def = OV5670_VTS_30FPS,
  1846			.vts_min = 510,
  1847			.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
  1848			.analog_crop = ov5670_analog_crop,
  1849			.reg_list = {
  1850				.num_of_regs = ARRAY_SIZE(mode_640x360_regs),
  1851				.regs = mode_640x360_regs,
  1852			},
  1853		}
  1854	};
  1855	

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

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

[Krzysztof Kozlowski]

On 14/03/2022 17:27, Jacopo Mondi wrote:
> Provide the bindings documentation for Omnivision OV5670 image sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> ---
> v1->v2 (comments from Krzysztof)
> 
> - Rename to include manufacturer name
> - Add entry to MAINTAINERS
> - Add maxItems: to -gpios properties
> - Use common clock properties
> - Use enum: [1, 2] for data lanes
> - Fix whitespace issue in example
> ---
> 
>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> new file mode 100644
> index 000000000000..73cf72203f17
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,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
> +
> +  assigned-clocks: true
> +  assigned-clock-parents: true
> +  assigned-clock-rates: true

You should not need these. These are coming with schema. You can add
these to example schema below and double-check.

> +
> +  clocks:
> +    description: System clock. From 6 to 27 MHz.
> +    maxItems: 1
> +
> +  pwdn-gpios:
> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.

This does not look like a standard property, so you need a vendor prefix.

> +    maxItems: 1
> +

Best regards,
Krzysztof

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

[Sakari Ailus]

Hi Krzysztof, Jacopo,

On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > Provide the bindings documentation for Omnivision OV5670 image sensor.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > ---
> > v1->v2 (comments from Krzysztof)
> > 
> > - Rename to include manufacturer name
> > - Add entry to MAINTAINERS
> > - Add maxItems: to -gpios properties
> > - Use common clock properties
> > - Use enum: [1, 2] for data lanes
> > - Fix whitespace issue in example
> > ---
> > 
> >  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 100 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > new file mode 100644
> > index 000000000000..73cf72203f17
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,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
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
> 
> You should not need these. These are coming with schema. You can add
> these to example schema below and double-check.

They should probably be required actually.

> 
> > +
> > +  clocks:
> > +    description: System clock. From 6 to 27 MHz.
> > +    maxItems: 1
> > +
> > +  pwdn-gpios:
> > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> 
> This does not look like a standard property, so you need a vendor prefix.

The similarly named property exists elsewhere. I wouldn't use a vendor
prefix, also for the reason that the functionality is quite common. I guess
alternative name would be possible, too --- "shutdown" seems to be more
common.

-- 
Regards,

Sakari Ailus

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

[Krzysztof Kozlowski]

On 15/03/2022 08:59, Sakari Ailus wrote:
> Hi Krzysztof, Jacopo,
> 
> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
>> On 14/03/2022 17:27, Jacopo Mondi wrote:
>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>
>>> ---
>>> v1->v2 (comments from Krzysztof)
>>>
>>> - Rename to include manufacturer name
>>> - Add entry to MAINTAINERS
>>> - Add maxItems: to -gpios properties
>>> - Use common clock properties
>>> - Use enum: [1, 2] for data lanes
>>> - Fix whitespace issue in example
>>> ---
>>>
>>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
>>>  MAINTAINERS                                   |  1 +
>>>  2 files changed, 100 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>> new file mode 100644
>>> index 000000000000..73cf72203f17
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>> @@ -0,0 +1,99 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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
>>> +
>>> +  assigned-clocks: true
>>> +  assigned-clock-parents: true
>>> +  assigned-clock-rates: true
>>
>> You should not need these. These are coming with schema. You can add
>> these to example schema below and double-check.
> 
> They should probably be required actually.

Why required? The hardware can work with different clocks, get their
rate and configure internal PLLs/clocks to new value. Having it required
might have sense for current implementation of driver but this is
independent of bindings. Bindings do not describe driver, but hardware.

>>
>>> +
>>> +  clocks:
>>> +    description: System clock. From 6 to 27 MHz.
>>> +    maxItems: 1
>>> +
>>> +  pwdn-gpios:
>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>
>> This does not look like a standard property, so you need a vendor prefix.
> 
> The similarly named property exists elsewhere. I wouldn't use a vendor
> prefix, also for the reason that the functionality is quite common. I guess
> alternative name would be possible, too --- "shutdown" seems to be more
> common.

It exists in three bindings, so it is not a standard property...
although something closer to standard is "powerdown-gpios" so maybe just
use that one?

Best regards,
Krzysztof

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

[Laurent Pinchart]

Hi Sakari,

On Tue, Mar 15, 2022 at 09:59:17AM +0200, Sakari Ailus wrote:
> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> > On 14/03/2022 17:27, Jacopo Mondi wrote:
> > > Provide the bindings documentation for Omnivision OV5670 image sensor.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > 
> > > ---
> > > v1->v2 (comments from Krzysztof)
> > > 
> > > - Rename to include manufacturer name
> > > - Add entry to MAINTAINERS
> > > - Add maxItems: to -gpios properties
> > > - Use common clock properties
> > > - Use enum: [1, 2] for data lanes
> > > - Fix whitespace issue in example
> > > ---
> > > 
> > >  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> > >  MAINTAINERS                                   |  1 +
> > >  2 files changed, 100 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > new file mode 100644
> > > index 000000000000..73cf72203f17
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > @@ -0,0 +1,99 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ovti,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
> > > +
> > > +  assigned-clocks: true
> > > +  assigned-clock-parents: true
> > > +  assigned-clock-rates: true
> > 
> > You should not need these. These are coming with schema. You can add
> > these to example schema below and double-check.
> 
> They should probably be required actually.

Why so ?

> > > +
> > > +  clocks:
> > > +    description: System clock. From 6 to 27 MHz.
> > > +    maxItems: 1
> > > +
> > > +  pwdn-gpios:
> > > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> > 
> > This does not look like a standard property, so you need a vendor prefix.
> 
> The similarly named property exists elsewhere. I wouldn't use a vendor
> prefix, also for the reason that the functionality is quite common. I guess
> alternative name would be possible, too --- "shutdown" seems to be more
> common.

There's a desire to standardize GPIO names ("reset" and "enable" being
the two most common candidates), but I'm not aware of an official list
of standard names. Have I missed it ?

In this case, I'd use powerdown-gpios, as it's more common than
pwdn-gpios (used in 21 bindings, compared to 2 for pwdn-gpios).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/8] media: i2c: ov5670: Probe clocks with OF

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote:
> Add support for probing the main system clock using the common clock
> framework and its OF bindings.
> 
> Maintain ACPI compatibility by falling back to parse 'clock-frequency'
> if the no clock device reference is available.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 721441024598..25d792794fc7 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2,6 +2,7 @@
>  // Copyright (c) 2017 Intel Corporation.
>  
>  #include <linux/acpi.h>
> +#include <linux/clk.h>
>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -1819,6 +1820,8 @@ struct ov5670 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  
> +	struct clk *clk;
> +
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl *link_freq;
> @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client)
>  	bool full_power;
>  	int ret;
>  
> -	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> -	if (input_clk != 19200000)
> -		return -EINVAL;
> -
>  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
>  	if (!ov5670) {
>  		ret = -ENOMEM;
> @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
> +	ov5670->clk = devm_clk_get_optional(&client->dev, NULL);
> +	if (IS_ERR(ov5670->clk))
> +		return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk),
> +				     "error getting clock\n");
> +
> +	if (ov5670->clk)
> +		input_clk = clk_get_rate(ov5670->clk);
> +	else
> +		device_property_read_u32(&client->dev, "clock-frequency",
> +					 &input_clk);

This will try to use the clock-frequency property on OF-based systems if
no clock is specified. Could we instead have

	if (probed through OF) {
		use clock
	} else {
		use clock-frequency
	}

?

> +	if (input_clk != 19200000)
> +		return -EINVAL;
> +
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 7/8] media: i2c: ov5670: Implement init_cfg

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 05:27:13PM +0100, Jacopo Mondi wrote:
> Implement the .init_cfg() pad operation and initialize the default
> format.
> 
> With .init_cfg() pad operation implemented the deprecated .open()
> internal operation can now be dropped.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  drivers/media/i2c/ov5670.c | 46 +++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index c3f773524d5f..9aa82774f8a6 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -1958,27 +1958,6 @@ static int ov5670_write_reg_list(struct ov5670 *ov5670,
>  	return ov5670_write_regs(ov5670, r_list->regs, r_list->num_of_regs);
>  }
>  
> -/* Open sub-device */
> -static int ov5670_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> -{
> -	struct ov5670 *ov5670 = to_ov5670(sd);
> -	struct v4l2_mbus_framefmt *try_fmt =
> -				v4l2_subdev_get_try_format(sd, fh->state, 0);
> -
> -	mutex_lock(&ov5670->mutex);
> -
> -	/* Initialize try_fmt */
> -	try_fmt->width = ov5670->cur_mode->width;
> -	try_fmt->height = ov5670->cur_mode->height;
> -	try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> -	try_fmt->field = V4L2_FIELD_NONE;
> -
> -	/* No crop or compose */
> -	mutex_unlock(&ov5670->mutex);
> -
> -	return 0;
> -}
> -
>  static int ov5670_update_digital_gain(struct ov5670 *ov5670, u32 d_gain)
>  {
>  	int ret;
> @@ -2178,6 +2157,25 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
>  	return ret;
>  }
>  
> +static int ov5670_init_cfg(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_mbus_framefmt *fmt =
> +				v4l2_subdev_get_try_format(sd, state, 0);
> +	const struct ov5670_mode *default_mode = &supported_modes[0];
> +
> +	fmt->width = default_mode->width;
> +	fmt->height = default_mode->height;
> +	fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +	fmt->field = V4L2_FIELD_NONE;
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB);
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB);
> +
> +	return 0;
> +}
> +
>  static int ov5670_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -2499,6 +2497,7 @@ static const struct v4l2_subdev_video_ops ov5670_video_ops = {
>  };
>  
>  static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
> +	.init_cfg = ov5670_init_cfg,
>  	.enum_mbus_code = ov5670_enum_mbus_code,
>  	.get_fmt = ov5670_get_pad_format,
>  	.set_fmt = ov5670_set_pad_format,
> @@ -2520,10 +2519,6 @@ static const struct media_entity_operations ov5670_subdev_entity_ops = {
>  	.link_validate = v4l2_subdev_link_validate,
>  };
>  
> -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);
> @@ -2629,7 +2624,6 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_mutex_destroy;
>  	}
>  
> -	ov5670->sd.internal_ops = &ov5670_internal_ops;
>  	ov5670->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>  			    V4L2_SUBDEV_FL_HAS_EVENTS;
>  	ov5670->sd.entity.ops = &ov5670_subdev_entity_ops;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 14, 2022 at 05:27:14PM +0100, Jacopo Mondi wrote:
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> Add support for the .get_selection() pad operation to the ov5670 sensor
> driver.
> 
> Report the native sensor size (pixel array), the crop bounds (readable
> pixel array area) and the current and default analog crop rectangles.
> 
> Currently all driver's mode use an analog crop rectangle of size

s/mode/modes/

> [12, 4, 2600, 1952]. Instead of hardcoding the value in the operation
> implementation, ad an .analog_crop field to the sensor's mode
> definitions, to make sure that if any mode gets added, its crop
> rectangle will be defined as well.
> 
> While at it re-sort the mode's field definition order to match the
> declaration order and initialize the crop rectangle in init_cfg().
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 89 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 83 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 9aa82774f8a6..67897dabb712 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -71,6 +71,10 @@
>  #define OV5670_REG_VALUE_16BIT		2
>  #define OV5670_REG_VALUE_24BIT		3
>  
> +/* Pixel Array */
> +#define OV5670_NATIVE_WIDTH		2624
> +#define OV5670_NATIVE_HEIGHT		1980
> +
>  /* Initial number of frames to skip to avoid possible garbage */
>  #define OV5670_NUM_OF_SKIP_FRAMES	2
>  
> @@ -113,10 +117,25 @@ struct ov5670_mode {
>  	/* Link frequency needed for this resolution */
>  	u32 link_freq_index;
>  
> +	/* Analog crop rectangle */
> +	const struct v4l2_rect analog_crop;
> +
>  	/* Sensor register settings for this resolution */
>  	const struct ov5670_reg_list reg_list;
>  };
>  
> +/*
> + * All the modes supported by the driver are obtained by subsampling the
> + * full pixel array. The below values are reflected in registers from
> + * 03800-0x3807 in the modes register-value tables.
> + */
> +static const struct v4l2_rect ov5670_analog_crop = {
> +	.left	= 12,
> +	.top	= 4,
> +	.width	= 2600,
> +	.height	= 1952,
> +};
> +
>  static const struct ov5670_reg mipi_data_rate_840mbps[] = {
>  	{0x0300, 0x04},
>  	{0x0301, 0x00},
> @@ -1764,66 +1783,73 @@ static const struct ov5670_mode supported_modes[] = {
>  		.height = 1944,
>  		.vts_def = OV5670_VTS_30FPS,
>  		.vts_min = OV5670_VTS_30FPS,
> +		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
> +		.analog_crop = ov5670_analog_crop,

The buildbot reported issues here.

You could turn analog_crop into a pointer to fix this.

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

>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
>  			.regs = mode_2592x1944_regs,
>  		},
> -		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
>  	},
>  	{
>  		.width = 1296,
>  		.height = 972,
>  		.vts_def = OV5670_VTS_30FPS,
>  		.vts_min = 996,
> +		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
> +		.analog_crop = ov5670_analog_crop,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_1296x972_regs),
>  			.regs = mode_1296x972_regs,
>  		},
> -		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
>  	},
>  	{
>  		.width = 648,
>  		.height = 486,
>  		.vts_def = OV5670_VTS_30FPS,
>  		.vts_min = 516,
> +		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
> +		.analog_crop = ov5670_analog_crop,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_648x486_regs),
>  			.regs = mode_648x486_regs,
>  		},
> -		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
>  	},
>  	{
>  		.width = 2560,
>  		.height = 1440,
>  		.vts_def = OV5670_VTS_30FPS,
>  		.vts_min = OV5670_VTS_30FPS,
> +		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
> +		.analog_crop = ov5670_analog_crop,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_2560x1440_regs),
>  			.regs = mode_2560x1440_regs,
>  		},
> -		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
>  	},
>  	{
>  		.width = 1280,
>  		.height = 720,
>  		.vts_def = OV5670_VTS_30FPS,
>  		.vts_min = 1020,
> +
> +		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
> +		.analog_crop = ov5670_analog_crop,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
>  			.regs = mode_1280x720_regs,
>  		},
> -		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
>  	},
>  	{
>  		.width = 640,
>  		.height = 360,
>  		.vts_def = OV5670_VTS_30FPS,
>  		.vts_min = 510,
> +		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
> +		.analog_crop = ov5670_analog_crop,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_640x360_regs),
>  			.regs = mode_640x360_regs,
>  		},
> -		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
>  	}
>  };
>  
> @@ -2163,6 +2189,7 @@ static int ov5670_init_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *fmt =
>  				v4l2_subdev_get_try_format(sd, state, 0);
>  	const struct ov5670_mode *default_mode = &supported_modes[0];
> +	struct v4l2_rect *crop = v4l2_subdev_get_try_crop(sd, state, 0);
>  
>  	fmt->width = default_mode->width;
>  	fmt->height = default_mode->height;
> @@ -2173,6 +2200,8 @@ static int ov5670_init_cfg(struct v4l2_subdev *sd,
>  	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB);
>  
> +	*crop = default_mode->analog_crop;
> +
>  	return 0;
>  }
>  
> @@ -2492,6 +2521,52 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  };
>  
> +static const struct v4l2_rect *
> +__ov5670_get_pad_crop(struct ov5670 *sensor, struct v4l2_subdev_state *state,
> +		      unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> +	const struct ov5670_mode *mode = sensor->cur_mode;
> +
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &mode->analog_crop;
> +	}
> +
> +	return NULL;
> +}
> +
> +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);
> +		sel->r = *__ov5670_get_pad_crop(sensor, state, sel->pad,
> +						sel->which);
> +		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 = ov5670_analog_crop;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_video_ops ov5670_video_ops = {
>  	.s_stream = ov5670_set_stream,
>  };
> @@ -2502,6 +2577,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 v2 1/8] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Hi Krzysztof,

On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > Provide the bindings documentation for Omnivision OV5670 image sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > ---
> > v1->v2 (comments from Krzysztof)
> >
> > - Rename to include manufacturer name
> > - Add entry to MAINTAINERS
> > - Add maxItems: to -gpios properties
> > - Use common clock properties
> > - Use enum: [1, 2] for data lanes
> > - Fix whitespace issue in example
> > ---
> >
> >  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 100 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > new file mode 100644
> > index 000000000000..73cf72203f17
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,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
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
>
> You should not need these. These are coming with schema. You can add
> these to example schema below and double-check.
>

Thanks, I'll try removing and see how it works

> > +
> > +  clocks:
> > +    description: System clock. From 6 to 27 MHz.
> > +    maxItems: 1
> > +
> > +  pwdn-gpios:
> > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>
> This does not look like a standard property, so you need a vendor prefix.
>

$ git grep powerdown-gpio arch/arm*/boot/dts | wc -l
48

$ git grep pwdn-gpio arch/arm*/boot/dts | wc -l
5

I'll go with the majority.

I wonder however if 'standard' proeprties are documented somewhere.

Thanks
  j


> > +    maxItems: 1
> > +
>
> Best regards,
> Krzysztof

Re: [PATCH v2 3/8] media: i2c: ov5670: Probe clocks with OF

[Jacopo Mondi]

Hi Laurent,

On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote:
> > Add support for probing the main system clock using the common clock
> > framework and its OF bindings.
> >
> > Maintain ACPI compatibility by falling back to parse 'clock-frequency'
> > if the no clock device reference is available.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5670.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index 721441024598..25d792794fc7 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -2,6 +2,7 @@
> >  // Copyright (c) 2017 Intel Corporation.
> >
> >  #include <linux/acpi.h>
> > +#include <linux/clk.h>
> >  #include <linux/i2c.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > @@ -1819,6 +1820,8 @@ struct ov5670 {
> >  	struct v4l2_subdev sd;
> >  	struct media_pad pad;
> >
> > +	struct clk *clk;
> > +
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  	/* V4L2 Controls */
> >  	struct v4l2_ctrl *link_freq;
> > @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client)
> >  	bool full_power;
> >  	int ret;
> >
> > -	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > -	if (input_clk != 19200000)
> > -		return -EINVAL;
> > -
> >  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
> >  	if (!ov5670) {
> >  		ret = -ENOMEM;
> > @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client)
> >  		goto error_print;
> >  	}
> >
> > +	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
> > +	ov5670->clk = devm_clk_get_optional(&client->dev, NULL);
> > +	if (IS_ERR(ov5670->clk))
> > +		return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk),
> > +				     "error getting clock\n");
> > +
> > +	if (ov5670->clk)
> > +		input_clk = clk_get_rate(ov5670->clk);
> > +	else
> > +		device_property_read_u32(&client->dev, "clock-frequency",
> > +					 &input_clk);
>
> This will try to use the clock-frequency property on OF-based systems if
> no clock is specified. Could we instead have
>

'clocks' is listed as a required property in the OF bindings and my
understanding is that driver assume DTS are correct.

> 	if (probed through OF) {

Otherwise yes, I can check for dev->of_node
But again, I don't think drivers should have to work-around broken DTS

> 		use clock
> 	} else {
> 		use clock-frequency
> 	}
>
> ?
>
> > +	if (input_clk != 19200000)
> > +		return -EINVAL;
> > +
> >  	/* Initialize subdev */
> >  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 3/8] media: i2c: ov5670: Probe clocks with OF

[Krzysztof Kozlowski]

On 15/03/2022 09:46, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch.
>>
>> On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote:
>>> Add support for probing the main system clock using the common clock
>>> framework and its OF bindings.
>>>
>>> Maintain ACPI compatibility by falling back to parse 'clock-frequency'
>>> if the no clock device reference is available.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  drivers/media/i2c/ov5670.c | 21 +++++++++++++++++----
>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
>>> index 721441024598..25d792794fc7 100644
>>> --- a/drivers/media/i2c/ov5670.c
>>> +++ b/drivers/media/i2c/ov5670.c
>>> @@ -2,6 +2,7 @@
>>>  // Copyright (c) 2017 Intel Corporation.
>>>
>>>  #include <linux/acpi.h>
>>> +#include <linux/clk.h>
>>>  #include <linux/i2c.h>
>>>  #include <linux/mod_devicetable.h>
>>>  #include <linux/module.h>
>>> @@ -1819,6 +1820,8 @@ struct ov5670 {
>>>  	struct v4l2_subdev sd;
>>>  	struct media_pad pad;
>>>
>>> +	struct clk *clk;
>>> +
>>>  	struct v4l2_ctrl_handler ctrl_handler;
>>>  	/* V4L2 Controls */
>>>  	struct v4l2_ctrl *link_freq;
>>> @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client)
>>>  	bool full_power;
>>>  	int ret;
>>>
>>> -	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
>>> -	if (input_clk != 19200000)
>>> -		return -EINVAL;
>>> -
>>>  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
>>>  	if (!ov5670) {
>>>  		ret = -ENOMEM;
>>> @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client)
>>>  		goto error_print;
>>>  	}
>>>
>>> +	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
>>> +	ov5670->clk = devm_clk_get_optional(&client->dev, NULL);
>>> +	if (IS_ERR(ov5670->clk))
>>> +		return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk),
>>> +				     "error getting clock\n");
>>> +
>>> +	if (ov5670->clk)
>>> +		input_clk = clk_get_rate(ov5670->clk);
>>> +	else
>>> +		device_property_read_u32(&client->dev, "clock-frequency",
>>> +					 &input_clk);
>>
>> This will try to use the clock-frequency property on OF-based systems if
>> no clock is specified. Could we instead have
>>
> 
> 'clocks' is listed as a required property in the OF bindings and my
> understanding is that driver assume DTS are correct.
> 
>> 	if (probed through OF) {
> 
> Otherwise yes, I can check for dev->of_node
> But again, I don't think drivers should have to work-around broken DTS
> 

Driver should expect broken DTS and handle it. You're introducing new
feature (OF support) so you can be strict about DTS and reject the ones
which do not provide clock.

Best regards,
Krzysztof

Re: [PATCH v2 3/8] media: i2c: ov5670: Probe clocks with OF

[Laurent Pinchart]

Hi Jacopo,

On Tue, Mar 15, 2022 at 09:46:18AM +0100, Jacopo Mondi wrote:
> On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote:
> > On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote:
> > > Add support for probing the main system clock using the common clock
> > > framework and its OF bindings.
> > >
> > > Maintain ACPI compatibility by falling back to parse 'clock-frequency'
> > > if the no clock device reference is available.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ov5670.c | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > index 721441024598..25d792794fc7 100644
> > > --- a/drivers/media/i2c/ov5670.c
> > > +++ b/drivers/media/i2c/ov5670.c
> > > @@ -2,6 +2,7 @@
> > >  // Copyright (c) 2017 Intel Corporation.
> > >
> > >  #include <linux/acpi.h>
> > > +#include <linux/clk.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > > @@ -1819,6 +1820,8 @@ struct ov5670 {
> > >  	struct v4l2_subdev sd;
> > >  	struct media_pad pad;
> > >
> > > +	struct clk *clk;
> > > +
> > >  	struct v4l2_ctrl_handler ctrl_handler;
> > >  	/* V4L2 Controls */
> > >  	struct v4l2_ctrl *link_freq;
> > > @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client)
> > >  	bool full_power;
> > >  	int ret;
> > >
> > > -	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > > -	if (input_clk != 19200000)
> > > -		return -EINVAL;
> > > -
> > >  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
> > >  	if (!ov5670) {
> > >  		ret = -ENOMEM;
> > > @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client)
> > >  		goto error_print;
> > >  	}
> > >
> > > +	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
> > > +	ov5670->clk = devm_clk_get_optional(&client->dev, NULL);
> > > +	if (IS_ERR(ov5670->clk))
> > > +		return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk),
> > > +				     "error getting clock\n");
> > > +
> > > +	if (ov5670->clk)
> > > +		input_clk = clk_get_rate(ov5670->clk);
> > > +	else
> > > +		device_property_read_u32(&client->dev, "clock-frequency",
> > > +					 &input_clk);
> >
> > This will try to use the clock-frequency property on OF-based systems if
> > no clock is specified. Could we instead have
> 
> 'clocks' is listed as a required property in the OF bindings and my
> understanding is that driver assume DTS are correct.
> 
> > 	if (probed through OF) {
> 
> Otherwise yes, I can check for dev->of_node
> But again, I don't think drivers should have to work-around broken DTS

Not working around, but if we fail when DT is broken, it can help
avoiding broken DT spreading in the wild, which we would then need to
support forever.

> > 		use clock
> > 	} else {
> > 		use clock-frequency
> > 	}
> >
> > ?
> >
> > > +	if (input_clk != 19200000)
> > > +		return -EINVAL;
> > > +
> > >  	/* Initialize subdev */
> > >  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> > >

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2022 08:59, Sakari Ailus wrote:
> > Hi Krzysztof, Jacopo,
> > 
> > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> >> On 14/03/2022 17:27, Jacopo Mondi wrote:
> >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>
> >>> ---
> >>> v1->v2 (comments from Krzysztof)
> >>>
> >>> - Rename to include manufacturer name
> >>> - Add entry to MAINTAINERS
> >>> - Add maxItems: to -gpios properties
> >>> - Use common clock properties
> >>> - Use enum: [1, 2] for data lanes
> >>> - Fix whitespace issue in example
> >>> ---
> >>>
> >>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> >>>  MAINTAINERS                                   |  1 +
> >>>  2 files changed, 100 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >>> new file mode 100644
> >>> index 000000000000..73cf72203f17
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >>> @@ -0,0 +1,99 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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
> >>> +
> >>> +  assigned-clocks: true
> >>> +  assigned-clock-parents: true
> >>> +  assigned-clock-rates: true
> >>
> >> You should not need these. These are coming with schema. You can add
> >> these to example schema below and double-check.
> > 
> > They should probably be required actually.
> 
> Why required? The hardware can work with different clocks, get their
> rate and configure internal PLLs/clocks to new value. Having it required
> might have sense for current implementation of driver but this is
> independent of bindings. Bindings do not describe driver, but hardware.

We've had this discussion before and the result of that was this (see
"Handling clocks"):

Documentation/driver-api/media/camera-sensor.rst

> 
> >>
> >>> +
> >>> +  clocks:
> >>> +    description: System clock. From 6 to 27 MHz.
> >>> +    maxItems: 1
> >>> +
> >>> +  pwdn-gpios:
> >>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> >>
> >> This does not look like a standard property, so you need a vendor prefix.
> > 
> > The similarly named property exists elsewhere. I wouldn't use a vendor
> > prefix, also for the reason that the functionality is quite common. I guess
> > alternative name would be possible, too --- "shutdown" seems to be more
> > common.
> 
> It exists in three bindings, so it is not a standard property...
> although something closer to standard is "powerdown-gpios" so maybe just
> use that one?

Seems like a better choice.

-- 
Sakari Ailus

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

[Krzysztof Kozlowski]

On 15/03/2022 13:47, Sakari Ailus wrote:
> On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
>> On 15/03/2022 08:59, Sakari Ailus wrote:
>>> Hi Krzysztof, Jacopo,
>>>
>>> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
>>>> On 14/03/2022 17:27, Jacopo Mondi wrote:
>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>
>>>>> ---
>>>>> v1->v2 (comments from Krzysztof)
>>>>>
>>>>> - Rename to include manufacturer name
>>>>> - Add entry to MAINTAINERS
>>>>> - Add maxItems: to -gpios properties
>>>>> - Use common clock properties
>>>>> - Use enum: [1, 2] for data lanes
>>>>> - Fix whitespace issue in example
>>>>> ---
>>>>>
>>>>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
>>>>>  MAINTAINERS                                   |  1 +
>>>>>  2 files changed, 100 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..73cf72203f17
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>>> @@ -0,0 +1,99 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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
>>>>> +
>>>>> +  assigned-clocks: true
>>>>> +  assigned-clock-parents: true
>>>>> +  assigned-clock-rates: true
>>>>
>>>> You should not need these. These are coming with schema. You can add
>>>> these to example schema below and double-check.
>>>
>>> They should probably be required actually.
>>
>> Why required? The hardware can work with different clocks, get their
>> rate and configure internal PLLs/clocks to new value. Having it required
>> might have sense for current implementation of driver but this is
>> independent of bindings. Bindings do not describe driver, but hardware.
> 
> We've had this discussion before and the result of that was this (see
> "Handling clocks"):
> 
> Documentation/driver-api/media/camera-sensor.rst

... and the "Devicetree" chapter explains usage of assigned-clock-xxx.
None of these explain why it should be in the bindings. The chapter you
referred explicitly mentions that "clock tree is generally configured by
the driver", so it has nothing to do with the bindings.

Bindings describe hardware, not Linux driver implementation. The
hardware can work with multiple frequencies and can support changing
these frequencies, probably combined with some reset sequence. Therefore
hardware does not require the clock frequency to be always fixed and
defined.

Driver requires assigned-clock-xxx, not bindings. Do not put Linux
implementation specifics into the bindings.

Best regards,
Krzysztof

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

[Laurent Pinchart]

Hi Sakari,

On Tue, Mar 15, 2022 at 02:47:43PM +0200, Sakari Ailus wrote:
> On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
> > On 15/03/2022 08:59, Sakari Ailus wrote:
> > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> > >> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> > >>>
> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>
> > >>> ---
> > >>> v1->v2 (comments from Krzysztof)
> > >>>
> > >>> - Rename to include manufacturer name
> > >>> - Add entry to MAINTAINERS
> > >>> - Add maxItems: to -gpios properties
> > >>> - Use common clock properties
> > >>> - Use enum: [1, 2] for data lanes
> > >>> - Fix whitespace issue in example
> > >>> ---
> > >>>
> > >>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> > >>>  MAINTAINERS                                   |  1 +
> > >>>  2 files changed, 100 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > >>> new file mode 100644
> > >>> index 000000000000..73cf72203f17
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > >>> @@ -0,0 +1,99 @@
> > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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
> > >>> +
> > >>> +  assigned-clocks: true
> > >>> +  assigned-clock-parents: true
> > >>> +  assigned-clock-rates: true
> > >>
> > >> You should not need these. These are coming with schema. You can add
> > >> these to example schema below and double-check.
> > > 
> > > They should probably be required actually.
> > 
> > Why required? The hardware can work with different clocks, get their
> > rate and configure internal PLLs/clocks to new value. Having it required
> > might have sense for current implementation of driver but this is
> > independent of bindings. Bindings do not describe driver, but hardware.
> 
> We've had this discussion before and the result of that was this (see
> "Handling clocks"):
> 
> Documentation/driver-api/media/camera-sensor.rst

I don't think those properties should be required in the sensor
bindings. There are platforms where the clock provided to the sensor
comes from a fixed-frequency oscillator, assigning a rate or parent
makes no sense for those (assigning a parent would actually be
impossible).

Assigning a parent or rate is fine when applicable, but as it can't be
required, there's also no point in listing the properties here.

> > >>> +
> > >>> +  clocks:
> > >>> +    description: System clock. From 6 to 27 MHz.
> > >>> +    maxItems: 1
> > >>> +
> > >>> +  pwdn-gpios:
> > >>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> > >>
> > >> This does not look like a standard property, so you need a vendor prefix.
> > > 
> > > The similarly named property exists elsewhere. I wouldn't use a vendor
> > > prefix, also for the reason that the functionality is quite common. I guess
> > > alternative name would be possible, too --- "shutdown" seems to be more
> > > common.
> > 
> > It exists in three bindings, so it is not a standard property...
> > although something closer to standard is "powerdown-gpios" so maybe just
> > use that one?
> 
> Seems like a better choice.

-- 
Regards,

Laurent Pinchart

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

[Nathan Chancellor]

On Tue, Mar 15, 2022 at 12:18:48PM +0800, kernel test robot wrote:
> Hi Jacopo,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on media-tree/master]
> [also build test ERROR on linus/master v5.17-rc8 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/20220315-003034
> base:   git://linuxtv.org/media_tree.git master
> config: i386-randconfig-a012-20220314 (https://download.01.org/0day-ci/archive/20220315/202203151238.pCKNarov-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 3e4950d7fa78ac83f33bbf1658e2f49a73719236)
> 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/c619a8eee6477517dfaa05511344d0bddc4e1c55
>         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/20220315-003034
>         git checkout c619a8eee6477517dfaa05511344d0bddc4e1c55
>         # 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:1787:18: error: initializer element is not a compile-time constant
>                    .analog_crop = ov5670_analog_crop,
>                                   ^~~~~~~~~~~~~~~~~~
>    1 error generated.

GCC versions prior to 8 will complain about this as well:

$ gcc --version | head -1
gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0

$ make -j"$(nproc)" mrproper allmodconfig drivers/media/i2c/ov5670.o
drivers/media/i2c/ov5670.c:1787:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1787:18: note: (near initialization for ‘supported_modes[0].analog_crop’)
drivers/media/i2c/ov5670.c:1799:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1799:18: note: (near initialization for ‘supported_modes[1].analog_crop’)
drivers/media/i2c/ov5670.c:1811:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1811:18: note: (near initialization for ‘supported_modes[2].analog_crop’)
drivers/media/i2c/ov5670.c:1823:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1823:18: note: (near initialization for ‘supported_modes[3].analog_crop’)
drivers/media/i2c/ov5670.c:1836:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1836:18: note: (near initialization for ‘supported_modes[4].analog_crop’)
drivers/media/i2c/ov5670.c:1848:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1848:18: note: (near initialization for ‘supported_modes[5].analog_crop’)
scripts/Makefile.build:288: recipe for target 'drivers/media/i2c/ov5670.o' failed

clang may eventually support this: https://reviews.llvm.org/D76096

Cheers,
Nathan

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

[Nathan Chancellor]

[-- Attachment #1: Type: text/plain, Size: 3977 bytes --]

On Tue, Mar 15, 2022 at 12:18:48PM +0800, kernel test robot wrote:
> Hi Jacopo,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on media-tree/master]
> [also build test ERROR on linus/master v5.17-rc8 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/20220315-003034
> base:   git://linuxtv.org/media_tree.git master
> config: i386-randconfig-a012-20220314 (https://download.01.org/0day-ci/archive/20220315/202203151238.pCKNarov-lkp(a)intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 3e4950d7fa78ac83f33bbf1658e2f49a73719236)
> 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/c619a8eee6477517dfaa05511344d0bddc4e1c55
>         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/20220315-003034
>         git checkout c619a8eee6477517dfaa05511344d0bddc4e1c55
>         # 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:1787:18: error: initializer element is not a compile-time constant
>                    .analog_crop = ov5670_analog_crop,
>                                   ^~~~~~~~~~~~~~~~~~
>    1 error generated.

GCC versions prior to 8 will complain about this as well:

$ gcc --version | head -1
gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0

$ make -j"$(nproc)" mrproper allmodconfig drivers/media/i2c/ov5670.o
drivers/media/i2c/ov5670.c:1787:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1787:18: note: (near initialization for ‘supported_modes[0].analog_crop’)
drivers/media/i2c/ov5670.c:1799:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1799:18: note: (near initialization for ‘supported_modes[1].analog_crop’)
drivers/media/i2c/ov5670.c:1811:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1811:18: note: (near initialization for ‘supported_modes[2].analog_crop’)
drivers/media/i2c/ov5670.c:1823:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1823:18: note: (near initialization for ‘supported_modes[3].analog_crop’)
drivers/media/i2c/ov5670.c:1836:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1836:18: note: (near initialization for ‘supported_modes[4].analog_crop’)
drivers/media/i2c/ov5670.c:1848:18: error: initializer element is not constant
   .analog_crop = ov5670_analog_crop,
                  ^~~~~~~~~~~~~~~~~~
drivers/media/i2c/ov5670.c:1848:18: note: (near initialization for ‘supported_modes[5].analog_crop’)
scripts/Makefile.build:288: recipe for target 'drivers/media/i2c/ov5670.o' failed

clang may eventually support this: https://reviews.llvm.org/D76096

Cheers,
Nathan

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

[Sakari Ailus]

Hi Laurent,

On Tue, Mar 15, 2022 at 03:01:35PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Mar 15, 2022 at 02:47:43PM +0200, Sakari Ailus wrote:
> > On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
> > > On 15/03/2022 08:59, Sakari Ailus wrote:
> > > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> > > >> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > > >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> > > >>>
> > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >>>
> > > >>> ---
> > > >>> v1->v2 (comments from Krzysztof)
> > > >>>
> > > >>> - Rename to include manufacturer name
> > > >>> - Add entry to MAINTAINERS
> > > >>> - Add maxItems: to -gpios properties
> > > >>> - Use common clock properties
> > > >>> - Use enum: [1, 2] for data lanes
> > > >>> - Fix whitespace issue in example
> > > >>> ---
> > > >>>
> > > >>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> > > >>>  MAINTAINERS                                   |  1 +
> > > >>>  2 files changed, 100 insertions(+)
> > > >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > >>>
> > > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > >>> new file mode 100644
> > > >>> index 000000000000..73cf72203f17
> > > >>> --- /dev/null
> > > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > >>> @@ -0,0 +1,99 @@
> > > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > >>> +%YAML 1.2
> > > >>> +---
> > > >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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
> > > >>> +
> > > >>> +  assigned-clocks: true
> > > >>> +  assigned-clock-parents: true
> > > >>> +  assigned-clock-rates: true
> > > >>
> > > >> You should not need these. These are coming with schema. You can add
> > > >> these to example schema below and double-check.
> > > > 
> > > > They should probably be required actually.
> > > 
> > > Why required? The hardware can work with different clocks, get their
> > > rate and configure internal PLLs/clocks to new value. Having it required
> > > might have sense for current implementation of driver but this is
> > > independent of bindings. Bindings do not describe driver, but hardware.
> > 
> > We've had this discussion before and the result of that was this (see
> > "Handling clocks"):
> > 
> > Documentation/driver-api/media/camera-sensor.rst
> 
> I don't think those properties should be required in the sensor
> bindings. There are platforms where the clock provided to the sensor
> comes from a fixed-frequency oscillator, assigning a rate or parent
> makes no sense for those (assigning a parent would actually be
> impossible).
> 
> Assigning a parent or rate is fine when applicable, but as it can't be
> required, there's also no point in listing the properties here.

The cases where the clock is fixed are quite rare but admittedly they
exist.

It's just easy to get this wrong.

-- 
Sakari Ailus

Re: [PATCH v2 3/8] media: i2c: ov5670: Probe clocks with OF

[Sakari Ailus]

Hi Laurent, Jacopo,

On Tue, Mar 15, 2022 at 10:56:41AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Tue, Mar 15, 2022 at 09:46:18AM +0100, Jacopo Mondi wrote:
> > On Tue, Mar 15, 2022 at 10:11:46AM +0200, Laurent Pinchart wrote:
> > > On Mon, Mar 14, 2022 at 05:27:09PM +0100, Jacopo Mondi wrote:
> > > > Add support for probing the main system clock using the common clock
> > > > framework and its OF bindings.
> > > >
> > > > Maintain ACPI compatibility by falling back to parse 'clock-frequency'
> > > > if the no clock device reference is available.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  drivers/media/i2c/ov5670.c | 21 +++++++++++++++++----
> > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > > index 721441024598..25d792794fc7 100644
> > > > --- a/drivers/media/i2c/ov5670.c
> > > > +++ b/drivers/media/i2c/ov5670.c
> > > > @@ -2,6 +2,7 @@
> > > >  // Copyright (c) 2017 Intel Corporation.
> > > >
> > > >  #include <linux/acpi.h>
> > > > +#include <linux/clk.h>
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/mod_devicetable.h>
> > > >  #include <linux/module.h>
> > > > @@ -1819,6 +1820,8 @@ struct ov5670 {
> > > >  	struct v4l2_subdev sd;
> > > >  	struct media_pad pad;
> > > >
> > > > +	struct clk *clk;
> > > > +
> > > >  	struct v4l2_ctrl_handler ctrl_handler;
> > > >  	/* V4L2 Controls */
> > > >  	struct v4l2_ctrl *link_freq;
> > > > @@ -2478,10 +2481,6 @@ static int ov5670_probe(struct i2c_client *client)
> > > >  	bool full_power;
> > > >  	int ret;
> > > >
> > > > -	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > > > -	if (input_clk != 19200000)
> > > > -		return -EINVAL;
> > > > -
> > > >  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
> > > >  	if (!ov5670) {
> > > >  		ret = -ENOMEM;
> > > > @@ -2489,6 +2488,20 @@ static int ov5670_probe(struct i2c_client *client)
> > > >  		goto error_print;
> > > >  	}
> > > >
> > > > +	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
> > > > +	ov5670->clk = devm_clk_get_optional(&client->dev, NULL);
> > > > +	if (IS_ERR(ov5670->clk))
> > > > +		return dev_err_probe(&client->dev, PTR_ERR(ov5670->clk),
> > > > +				     "error getting clock\n");
> > > > +
> > > > +	if (ov5670->clk)
> > > > +		input_clk = clk_get_rate(ov5670->clk);
> > > > +	else
> > > > +		device_property_read_u32(&client->dev, "clock-frequency",
> > > > +					 &input_clk);
> > >
> > > This will try to use the clock-frequency property on OF-based systems if
> > > no clock is specified. Could we instead have
> > 
> > 'clocks' is listed as a required property in the OF bindings and my
> > understanding is that driver assume DTS are correct.
> > 
> > > 	if (probed through OF) {
> > 
> > Otherwise yes, I can check for dev->of_node
> > But again, I don't think drivers should have to work-around broken DTS
> 
> Not working around, but if we fail when DT is broken, it can help
> avoiding broken DT spreading in the wild, which we would then need to
> support forever.

If you think this is necessary (I'm not all that sure), then please check
this using is_of_node().

There are other drivers that could take clock-frequency from DT (grep for
reading clock-frequency with more indentation than one tab stop), too, even
if the bindings don't document that. I wouldn't expect that anyway since
clock control won't work there anyway.

-- 
Regards,

Sakari Ailus