[PATCH 0/3] media: i2c: og01a1b: Add OF support to OmniVision OG01A1B

[PATCH 0/3] media: i2c: og01a1b: Add OF support to OmniVision OG01A1B

[Vladimir Zapolskiy]

The change adds basic support of OmniVision OG01A1B image sensor on
OF platforms.

Vladimir Zapolskiy (3):
  media: dt-bindings: Add description of OmniVision OG01A1B image sensor
  media: i2c: og01a1b: Add OF support to the image sensor driver
  media: i2c: og01a1b: Add support of an input system clock

 .../bindings/media/i2c/ovti,og01a1b.yaml      | 108 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/og01a1b.c                   |  50 ++++++--
 3 files changed, 150 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml

-- 
2.45.2

[PATCH 1/3] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Vladimir Zapolskiy]

Add device tree bindings documentation for OmniVision OG01A1B image
sensor.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 .../bindings/media/i2c/ovti,og01a1b.yaml      | 108 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
new file mode 100644
index 000000000000..5f4c9c4c1ee5
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023-2024 Linaro Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,og01a1b.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OmniVision OG01A1B Image Sensor
+
+maintainers:
+  - Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
+
+description: |-
+  The OmniVision OG01A1B is black and white CMOS 1.3 Megapixel (1280x1024)
+  image sensor controlled over an I2C-compatible SCCB bus.
+  The sensor transmits images on a MIPI CSI-2 output interface with one or
+  two data lanes.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: ovti,og01a1b
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: External clock connected to the sensor system clock.
+    maxItems: 1
+
+  clock-frequency:
+    description: Frequency of the external clock to the sensor in Hz.
+
+  reset-gpios:
+    description: Active low signal GPIO to control sensor.
+    maxItems: 1
+
+  strobe-gpios:
+    description: GPIO connected to the sensor strobe pad.
+    maxItems: 1
+
+  avdd-supply:
+    description: Analogue circuit voltage supply.
+
+  dovdd-supply:
+    description: I/O circuit voltage supply.
+
+  dvdd-supply:
+    description: Digital circuit voltage supply.
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+    description:
+      Output port node, single endpoint describing the CSI-2 transmitter.
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            minItems: 1
+            maxItems: 2
+
+          link-frequencies: true
+
+        required:
+          - data-lanes
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@60 {
+            compatible = "ovti,og01a1b";
+            reg = <0x60>;
+            clocks = <&clk 0>;
+            reset-gpios = <&gpio 117 GPIO_ACTIVE_LOW>;
+            avdd-supply = <&vreg_3v3>;
+            dovdd-supply = <&vreg_1p8>;
+            dvdd-supply = <&vreg_1p2>;
+
+            port {
+                og01a1b_ep: endpoint {
+                    remote-endpoint = <&csiphy_ep>;
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <500000000>;
+                };
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..eff16776a287 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16509,6 +16509,7 @@ OMNIVISION OG01A1B SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/og01a1b.yaml
 F:	drivers/media/i2c/og01a1b.c
 
 OMNIVISION OV01A10 SENSOR DRIVER
-- 
2.45.2

Re: [PATCH 1/3] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Krzysztof Kozlowski]

On 20/06/2024 14:47, Vladimir Zapolskiy wrote:
> Add device tree bindings documentation for OmniVision OG01A1B image
> sensor.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>


> +
> +maintainers:
> +  - Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> +
> +description: |-

Do not need '|-' unless you need to preserve formatting.


> +  The OmniVision OG01A1B is black and white CMOS 1.3 Megapixel (1280x1024)
> +  image sensor controlled over an I2C-compatible SCCB bus.
> +  The sensor transmits images on a MIPI CSI-2 output interface with one or
> +  two data lanes.
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: ovti,og01a1b
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: External clock connected to the sensor system clock.

Drop description, obvious.

> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: Frequency of the external clock to the sensor in Hz.

Drop property. Frequency comes from clock.

> +
> +  reset-gpios:
> +    description: Active low signal GPIO to control sensor.
> +    maxItems: 1
> +
> +  strobe-gpios:
> +    description: GPIO connected to the sensor strobe pad.
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: Analogue circuit voltage supply.
> +
> +  dovdd-supply:
> +    description: I/O circuit voltage supply.
> +
> +  dvdd-supply:
> +    description: Digital circuit voltage supply.
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +    description:
> +      Output port node, single endpoint describing the CSI-2 transmitter.
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes:
> +            minItems: 1
> +            maxItems: 2
> +
> +          link-frequencies: true

This should not be needed.

> +
> +        required:
> +          - data-lanes
> +


Best regards,
Krzysztof

Re: [PATCH 1/3] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Sakari Ailus]

Hi Krzysztof,

On Thu, Jun 20, 2024 at 05:33:15PM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2024 14:47, Vladimir Zapolskiy wrote:
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          data-lanes:
> > +            minItems: 1
> > +            maxItems: 2
> > +
> > +          link-frequencies: true
> 
> This should not be needed.

Why?

Please see
<URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/3] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Krzysztof Kozlowski]

On 20/06/2024 22:04, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Thu, Jun 20, 2024 at 05:33:15PM +0200, Krzysztof Kozlowski wrote:
>> On 20/06/2024 14:47, Vladimir Zapolskiy wrote:
>>> +    properties:
>>> +      endpoint:
>>> +        $ref: /schemas/media/video-interfaces.yaml#
>>> +        unevaluatedProperties: false
>>> +
>>> +        properties:
>>> +          data-lanes:
>>> +            minItems: 1
>>> +            maxItems: 2
>>> +
>>> +          link-frequencies: true
>>
>> This should not be needed.
> 
> Why?

Because it is a no-op. Changes nothing, absolutely nothing. Property is
allowed from video-interfaces via unevaluatedProperties.

> 
> Please see
> <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.

This doe snot cover empty link-frequencies or I could not spot what you
are referring to.

Unless you are suggesting that this should be fixed into proper
frequencies? Then yes, Ack. Keep it with proper values.


Best regards,
Krzysztof

Re: [PATCH 1/3] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Sakari Ailus]

Hi Krzysztof,

On Thu, Jun 20, 2024 at 10:28:03PM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2024 22:04, Sakari Ailus wrote:
> > Hi Krzysztof,
> > 
> > On Thu, Jun 20, 2024 at 05:33:15PM +0200, Krzysztof Kozlowski wrote:
> >> On 20/06/2024 14:47, Vladimir Zapolskiy wrote:
> >>> +    properties:
> >>> +      endpoint:
> >>> +        $ref: /schemas/media/video-interfaces.yaml#
> >>> +        unevaluatedProperties: false
> >>> +
> >>> +        properties:
> >>> +          data-lanes:
> >>> +            minItems: 1
> >>> +            maxItems: 2
> >>> +
> >>> +          link-frequencies: true
> >>
> >> This should not be needed.
> > 
> > Why?
> 
> Because it is a no-op. Changes nothing, absolutely nothing. Property is
> allowed from video-interfaces via unevaluatedProperties.

Right, indeed.

This should be listeed as required instead.

> 
> > 
> > Please see
> > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.
> 
> This doe snot cover empty link-frequencies or I could not spot what you
> are referring to.
> 
> Unless you are suggesting that this should be fixed into proper
> frequencies? Then yes, Ack. Keep it with proper values.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/3] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[kernel test robot]

Hi Vladimir,

kernel test robot noticed the following build warnings:

[auto build test WARNING on media-tree/master]
[also build test WARNING on sailus-media-tree/master krzk-dt/for-next robh/for-next linus/master v6.10-rc5 next-20240621]
[cannot apply to sailus-media-tree/streams]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Vladimir-Zapolskiy/media-dt-bindings-Add-description-of-OmniVision-OG01A1B-image-sensor/20240624-161554
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20240620124745.1265011-2-vladimir.zapolskiy%40linaro.org
patch subject: [PATCH 1/3] media: dt-bindings: Add description of OmniVision OG01A1B image sensor
reproduce: (https://download.01.org/0day-ci/archive/20240624/202406242352.HSKkjAv2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406242352.HSKkjAv2-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/devicetree/bindings/sound/fsl-asoc-card.txt references a file that doesn't exist: Documentation/devicetree/bindings/sound/fsl,asrc.txt
   Warning: Documentation/gpu/amdgpu/display/display-contributing.rst references a file that doesn't exist: Documentation/GPU/amdgpu/display/mpo-overview.rst
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/media/i2c/og01a1b.yaml
   Using alabaster theme

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/3] media: i2c: og01a1b: Add OF support to the image sensor driver

[Vladimir Zapolskiy]

The OmniVision  OG01A1B image sensor driver currently supports probing
only on ACPI platforms, the changes adds support of OF platforms to
the driver.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/i2c/og01a1b.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
index bac9597faf68..9e756c1c47df 100644
--- a/drivers/media/i2c/og01a1b.c
+++ b/drivers/media/i2c/og01a1b.c
@@ -1057,10 +1057,17 @@ static const struct acpi_device_id og01a1b_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, og01a1b_acpi_ids);
 #endif
 
+static const struct of_device_id og01a1b_of_match[] = {
+	{ .compatible = "ovti,og01a1b" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, og01a1b_of_match);
+
 static struct i2c_driver og01a1b_i2c_driver = {
 	.driver = {
 		.name = "og01a1b",
 		.acpi_match_table = ACPI_PTR(og01a1b_acpi_ids),
+		.of_match_table = og01a1b_of_match,
 	},
 	.probe = og01a1b_probe,
 	.remove = og01a1b_remove,
-- 
2.45.2

[PATCH 3/3] media: i2c: og01a1b: Add support of an input system clock

[Vladimir Zapolskiy]

To be properly configured at probe time the sensor module contains
a required reference to xvclk clock on OF platforms, which is now
enabled on probe and checked for a supported frequency rate.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/i2c/og01a1b.c | 43 +++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
index 9e756c1c47df..929a7e32bd6a 100644
--- a/drivers/media/i2c/og01a1b.c
+++ b/drivers/media/i2c/og01a1b.c
@@ -3,6 +3,7 @@
 
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
@@ -418,6 +419,9 @@ static const struct og01a1b_mode supported_modes[] = {
 };
 
 struct og01a1b {
+	struct device *dev;
+	struct clk *xvclk;
+
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
@@ -898,8 +902,9 @@ static int og01a1b_identify_module(struct og01a1b *og01a1b)
 	return 0;
 }
 
-static int og01a1b_check_hwcfg(struct device *dev)
+static int og01a1b_check_hwcfg(struct og01a1b *og01a1b)
 {
+	struct device *dev = og01a1b->dev;
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
@@ -913,10 +918,13 @@ static int og01a1b_check_hwcfg(struct device *dev)
 		return -ENXIO;
 
 	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
-
 	if (ret) {
-		dev_err(dev, "can't get clock frequency");
-		return ret;
+		if (!og01a1b->xvclk) {
+			dev_err(dev, "can't get clock frequency");
+			return ret;
+		}
+
+		mclk = clk_get_rate(og01a1b->xvclk);
 	}
 
 	if (mclk != OG01A1B_MCLK) {
@@ -977,6 +985,7 @@ static void og01a1b_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	pm_runtime_disable(&client->dev);
 	mutex_destroy(&og01a1b->mutex);
+	clk_disable_unprepare(og01a1b->xvclk);
 }
 
 static int og01a1b_probe(struct i2c_client *client)
@@ -984,22 +993,35 @@ static int og01a1b_probe(struct i2c_client *client)
 	struct og01a1b *og01a1b;
 	int ret;
 
-	ret = og01a1b_check_hwcfg(&client->dev);
+	og01a1b = devm_kzalloc(&client->dev, sizeof(*og01a1b), GFP_KERNEL);
+	if (!og01a1b)
+		return -ENOMEM;
+
+	og01a1b->dev = &client->dev;
+
+	og01a1b->xvclk = devm_clk_get_optional(&client->dev, NULL);
+	if (IS_ERR(og01a1b->xvclk)) {
+		ret = PTR_ERR(og01a1b->xvclk);
+		dev_err(&client->dev, "failed to get clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = og01a1b_check_hwcfg(og01a1b);
 	if (ret) {
 		dev_err(&client->dev, "failed to check HW configuration: %d",
 			ret);
 		return ret;
 	}
 
-	og01a1b = devm_kzalloc(&client->dev, sizeof(*og01a1b), GFP_KERNEL);
-	if (!og01a1b)
-		return -ENOMEM;
+	ret = clk_prepare_enable(og01a1b->xvclk);
+	if (ret)
+		return ret;
 
 	v4l2_i2c_subdev_init(&og01a1b->sd, client, &og01a1b_subdev_ops);
 	ret = og01a1b_identify_module(og01a1b);
 	if (ret) {
 		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		return ret;
+		goto disable_clk;
 	}
 
 	mutex_init(&og01a1b->mutex);
@@ -1045,6 +1067,9 @@ static int og01a1b_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(og01a1b->sd.ctrl_handler);
 	mutex_destroy(&og01a1b->mutex);
 
+disable_clk:
+	clk_disable_unprepare(og01a1b->xvclk);
+
 	return ret;
 }
 
-- 
2.45.2