[PATCH v3 0/4] ov08x40: Enable use of ov08x40 on Qualcomm X1E80100 CRD

[PATCH v3 0/4] ov08x40: Enable use of ov08x40 on Qualcomm X1E80100 CRD

[Bryan O'Donoghue]

Changes in v3:
- Drops assigned-clock-* from description retains in example - Sakari,
  Krzysztof
- Updates example fake clock names to ov08x40_* instead of copy/paste
  ov9282_clk -> ov08x40_clk, ov9282_clk_parent -> ov08x40_clk_parent - bod
- Link to v2: https://lore.kernel.org/r/20241001-b4-master-24-11-25-ov08x40-v2-0-e478976b20c1@linaro.org

Changes in v2:
- Drops "-" in ovti,ov08x40.yaml after description: - Rob
- Adds ":" after first line of description text - Rob
- dts -> DT in commit log - Rob
- Removes dependency on 'xvclk' as a name in yaml
  and driver - Sakari
- Uses assigned-clock, assigned-clock-parents and assigned-clock-rates -
  Sakari
- Drops clock-frequency - Sakarai, Krzysztof
- Drops dovdd-supply, avdd-supply, dvdd-supply and reset-gpios
  as required, its perfectly possible not to have the reset GPIO or the
  power rails under control of the SoC. - bod

- Link to v1: https://lore.kernel.org/r/20240926-b4-master-24-11-25-ov08x40-v1-0-e4d5fbd3b58a@linaro.org

V1:
This series brings fixes and updates to ov08x40 which allows for use of
this sensor on the Qualcomm x1e80100 CRD but also on any other dts based
system.

Firstly there's a fix for the pseudo burst mode code that was added in
8f667d202384 ("media: ov08x40: Reduce start streaming time"). Not every I2C
controller can handle an arbitrary sized write, this is the case on
Qualcomm CAMSS/CCI I2C sensor interfaces which limit the transaction size
and communicate this limit via I2C quirks. A simple fix to optionally break
up the large submitted burst into chunks not exceeding adapter->quirk size
fixes.

Secondly then is addition of a yaml description for the ov08x40 and
extension of the driver to support OF probe and powering on of the power
rails from the driver instead of from ACPI.

Once done the sensor works without further modification on the Qualcomm
x1e80100 CRD.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
Bryan O'Donoghue (4):
      media: ov08x40: Fix burst write sequence
      media: dt-bindings: Add OmniVision OV08X40
      media: ov08x40: Rename ext_clk to xvclk
      media: ov08x40: Add OF probe support

 .../bindings/media/i2c/ovti,ov08x40.yaml           | 116 +++++++++++++
 drivers/media/i2c/ov08x40.c                        | 179 ++++++++++++++++++---
 2 files changed, 272 insertions(+), 23 deletions(-)
---
base-commit: 2b7275670032a98cba266bd1b8905f755b3e650f
change-id: 20240926-b4-master-24-11-25-ov08x40-c6f477aaa6a4

Best regards,
-- 
Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH v3 1/4] media: ov08x40: Fix burst write sequence

[Bryan O'Donoghue]

It is necessary to account for I2C quirks in the burst mode path of this
driver. Not all I2C controllers can accept arbitrarily long writes and this
is represented in the quirks field of the adapter structure.

Prior to this patch the following error message is seen on a Qualcomm
X1E80100 CRD.

[   38.773524] i2c i2c-2: adapter quirk: msg too long (addr 0x0036, size 290, write)
[   38.781454] ov08x40 2-0036: Failed regs transferred: -95
[   38.787076] ov08x40 2-0036: ov08x40_start_streaming failed to set regs

Fix the error by breaking up the write sequence into the advertised maximum
write size of the quirks field if the quirks field is populated.

Fixes: 8f667d202384 ("media: ov08x40: Reduce start streaming time")
Cc: stable@vger.kernel.org # v6.9+
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e80100-crd
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/i2c/ov08x40.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
index 48df077522ad0bb2b5f64a6def8844c02af6a193..be25e45175b1322145dca428e845242d8fea2698 100644
--- a/drivers/media/i2c/ov08x40.c
+++ b/drivers/media/i2c/ov08x40.c
@@ -1339,15 +1339,13 @@ static int ov08x40_read_reg(struct ov08x40 *ov08x,
 	return 0;
 }
 
-static int ov08x40_burst_fill_regs(struct ov08x40 *ov08x, u16 first_reg,
-				   u16 last_reg,  u8 val)
+static int __ov08x40_burst_fill_regs(struct i2c_client *client, u16 first_reg,
+				     u16 last_reg, size_t num_regs, u8 val)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov08x->sd);
 	struct i2c_msg msgs;
-	size_t i, num_regs;
+	size_t i;
 	int ret;
 
-	num_regs = last_reg - first_reg + 1;
 	msgs.addr = client->addr;
 	msgs.flags = 0;
 	msgs.len = 2 + num_regs;
@@ -1373,6 +1371,31 @@ static int ov08x40_burst_fill_regs(struct ov08x40 *ov08x, u16 first_reg,
 	return 0;
 }
 
+static int ov08x40_burst_fill_regs(struct ov08x40 *ov08x, u16 first_reg,
+				   u16 last_reg,  u8 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov08x->sd);
+	size_t num_regs, num_write_regs;
+	int ret;
+
+	num_regs = last_reg - first_reg + 1;
+	num_write_regs = num_regs;
+
+	if (client->adapter->quirks && client->adapter->quirks->max_write_len)
+		num_write_regs = client->adapter->quirks->max_write_len - 2;
+
+	while (first_reg < last_reg) {
+		ret = __ov08x40_burst_fill_regs(client, first_reg, last_reg,
+						num_write_regs, val);
+		if (ret)
+			return ret;
+
+		first_reg += num_write_regs;
+	}
+
+	return 0;
+}
+
 /* Write registers up to 4 at a time */
 static int ov08x40_write_reg(struct ov08x40 *ov08x,
 			     u16 reg, u32 len, u32 __val)

-- 
2.46.2

[PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Bryan O'Donoghue]

Add bindings for the already upstream OV08X40 to enable usage of this
sensor on DTS based systems.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../bindings/media/i2c/ovti,ov08x40.yaml           | 116 +++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov08x40.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov08x40.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..74b33a083efbe91db0fa4e7e7bb6008a95e4e4d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov08x40.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2024 Linaro Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov08x40.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV08X40 CMOS Sensor
+
+maintainers:
+  - Bryan O'Donoghue <bryan.odonoghue@linaro.org>
+
+description: |
+  The Omnivision OV08X40 is a 9.2 megapixel, CMOS image sensor which supports:
+  - Automatic black level calibration (ABLC)
+  - Programmable controls for frame rate, mirror and flip, binning, cropping
+    and windowing
+  - Output formats 10-bit 4C RGB RAW, 10-bit Bayer RAW
+  - 4-lane MIPI D-PHY TX @ 1 Gbps per lane
+  - 2-lane MPIP D-PHY TX @ 2 Gbps per lane
+  - Dynamic defect pixel cancellation
+  - Standard SCCB command interface
+
+properties:
+  compatible:
+    const: ovti,ov08x40
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  avdd-supply:
+    description: Analogue circuit voltage supply.
+
+  dovdd-supply:
+    description: I/O circuit voltage supply.
+
+  dvdd-supply:
+    description: Digital circuit voltage supply.
+
+  reset-gpios:
+    description: Active low GPIO connected to XSHUTDOWN pad of the sensor.
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            oneOf:
+              - items:
+                  - const: 1
+                  - const: 2
+              - items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+
+          link-frequencies: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov08x40: camera@36 {
+            compatible = "ovti,ov08x40";
+            reg = <0x36>;
+
+            reset-gpios = <&tlmm 111 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&cam_rgb_defaultt>;
+
+            clocks = <&ov08x40_clk>;
+
+            assigned-clocks = <&ov08x40_clk>;
+            assigned-clock-parents = <&ov08x40_clk_parent>;
+            assigned-clock-rates = <19200000>;
+
+            avdd-supply = <&vreg_l7b_2p8>;
+            dvdd-supply = <&vreg_l7b_1p8>;
+            dovdd-supply = <&vreg_l3m_1p8>;
+
+            port {
+                ov08x40_ep: endpoint {
+                    remote-endpoint = <&csiphy4_ep>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <400000000>;
+                };
+            };
+        };
+    };
+...

-- 
2.46.2

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Krzysztof Kozlowski]

On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
> +        properties:
> +          data-lanes:
> +            oneOf:
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +                  - const: 3
> +                  - const: 4
> +
> +          link-frequencies: true

Not much changed here and you did not continued discussion about it.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Bryan O'Donoghue]

On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
> On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
>> +        properties:
>> +          data-lanes:
>> +            oneOf:
>> +              - items:
>> +                  - const: 1
>> +                  - const: 2
>> +              - items:
>> +                  - const: 1
>> +                  - const: 2
>> +                  - const: 3
>> +                  - const: 4
>> +
>> +          link-frequencies: true
> 
> Not much changed here and you did not continued discussion about it.
> 
> Best regards,
> Krzysztof
> 

Ah my mistake, I didn't read the bit at the bottom of your email

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Krzysztof Kozlowski]

On 03/10/2024 10:33, Bryan O'Donoghue wrote:
> On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
>> On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
>>> +        properties:
>>> +          data-lanes:
>>> +            oneOf:
>>> +              - items:
>>> +                  - const: 1
>>> +                  - const: 2
>>> +              - items:
>>> +                  - const: 1
>>> +                  - const: 2
>>> +                  - const: 3
>>> +                  - const: 4
>>> +
>>> +          link-frequencies: true
>>
>> Not much changed here and you did not continued discussion about it.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Ah my mistake, I didn't read the bit at the bottom of your email

Unlike some other folks, I almost never leave useless quote below my
messages, because it wastes a lot of time on reader side, so if there is
a quote it means there is something further.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Bryan O'Donoghue]

On 03/10/2024 09:33, Bryan O'Donoghue wrote:
> On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
>> On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
>>> +        properties:
>>> +          data-lanes:
>>> +            oneOf:
>>> +              - items:
>>> +                  - const: 1
>>> +                  - const: 2
>>> +              - items:
>>> +                  - const: 1
>>> +                  - const: 2
>>> +                  - const: 3
>>> +                  - const: 4
>>> +
>>> +          link-frequencies: true
>>
>> Not much changed here and you did not continued discussion about it.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Ah my mistake, I didn't read the bit at the bottom of your email

I'll do this

Documentation/devicetree/bindings/media/i2c/thine,thp7312.yaml

           data-lanes:
             description:
               This property is for lane reordering between the THP7312
               and the SoC. The sensor supports either two-lane, or
               four-lane operation.
               If this property is omitted four-lane operation is
               assumed. For two-lane operation the property must be
               set to <1 2>.
             minItems: 2
             maxItems: 4
             items:
               maximum: 4

This captures what I'm after.

---
bod

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Krzysztof Kozlowski]

On 03/10/2024 10:38, Bryan O'Donoghue wrote:
> On 03/10/2024 09:33, Bryan O'Donoghue wrote:
>> On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
>>> On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
>>>> +        properties:
>>>> +          data-lanes:
>>>> +            oneOf:
>>>> +              - items:
>>>> +                  - const: 1
>>>> +                  - const: 2
>>>> +              - items:
>>>> +                  - const: 1
>>>> +                  - const: 2
>>>> +                  - const: 3
>>>> +                  - const: 4
>>>> +
>>>> +          link-frequencies: true
>>>
>>> Not much changed here and you did not continued discussion about it.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Ah my mistake, I didn't read the bit at the bottom of your email
> 
> I'll do this
> 
> Documentation/devicetree/bindings/media/i2c/thine,thp7312.yaml
> 
>            data-lanes:
>              description:
>                This property is for lane reordering between the THP7312
>                and the SoC. The sensor supports either two-lane, or
>                four-lane operation.
>                If this property is omitted four-lane operation is
>                assumed. For two-lane operation the property must be
>                set to <1 2>.
>              minItems: 2
>              maxItems: 4
>              items:
>                maximum: 4
> 
> This captures what I'm after.

I commented on link-frequencies.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Bryan O'Donoghue]

On 03/10/2024 11:17, Krzysztof Kozlowski wrote:
> On 03/10/2024 10:38, Bryan O'Donoghue wrote:
>> On 03/10/2024 09:33, Bryan O'Donoghue wrote:
>>> On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
>>>> On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
>>>>> +        properties:
>>>>> +          data-lanes:
>>>>> +            oneOf:
>>>>> +              - items:
>>>>> +                  - const: 1
>>>>> +                  - const: 2
>>>>> +              - items:
>>>>> +                  - const: 1
>>>>> +                  - const: 2
>>>>> +                  - const: 3
>>>>> +                  - const: 4
>>>>> +
>>>>> +          link-frequencies: true
>>>>
>>>> Not much changed here and you did not continued discussion about it.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Ah my mistake, I didn't read the bit at the bottom of your email
>>
>> I'll do this
>>
>> Documentation/devicetree/bindings/media/i2c/thine,thp7312.yaml
>>
>>             data-lanes:
>>               description:
>>                 This property is for lane reordering between the THP7312
>>                 and the SoC. The sensor supports either two-lane, or
>>                 four-lane operation.
>>                 If this property is omitted four-lane operation is
>>                 assumed. For two-lane operation the property must be
>>                 set to <1 2>.
>>               minItems: 2
>>               maxItems: 4
>>               items:
>>                 maximum: 4
>>
>> This captures what I'm after.
> 
> I commented on link-frequencies.
> 
> Best regards,
> Krzysztof
> 

Ah I understand you.

You're saying the link-frequencies we have in 
Documentation/devicetree/bindings/media/i2c/* are redundant absent 
hardware specific link frequencies being enumerated.

I'll either enumerate the acceptable set or drop this.

---
bod

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Sakari Ailus]

Hi Bryan, Krzysztof,

On Thu, Oct 03, 2024 at 12:54:41PM +0100, Bryan O'Donoghue wrote:
> On 03/10/2024 11:17, Krzysztof Kozlowski wrote:
> > On 03/10/2024 10:38, Bryan O'Donoghue wrote:
> > > On 03/10/2024 09:33, Bryan O'Donoghue wrote:
> > > > On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
> > > > > On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
> > > > > > +        properties:
> > > > > > +          data-lanes:
> > > > > > +            oneOf:
> > > > > > +              - items:
> > > > > > +                  - const: 1
> > > > > > +                  - const: 2
> > > > > > +              - items:
> > > > > > +                  - const: 1
> > > > > > +                  - const: 2
> > > > > > +                  - const: 3
> > > > > > +                  - const: 4
> > > > > > +
> > > > > > +          link-frequencies: true
> > > > > 
> > > > > Not much changed here and you did not continued discussion about it.
> > > > > 
> > > > > Best regards,
> > > > > Krzysztof
> > > > > 
> > > > 
> > > > Ah my mistake, I didn't read the bit at the bottom of your email
> > > 
> > > I'll do this
> > > 
> > > Documentation/devicetree/bindings/media/i2c/thine,thp7312.yaml
> > > 
> > >             data-lanes:
> > >               description:
> > >                 This property is for lane reordering between the THP7312
> > >                 and the SoC. The sensor supports either two-lane, or
> > >                 four-lane operation.
> > >                 If this property is omitted four-lane operation is
> > >                 assumed. For two-lane operation the property must be
> > >                 set to <1 2>.
> > >               minItems: 2
> > >               maxItems: 4
> > >               items:
> > >                 maximum: 4
> > > 
> > > This captures what I'm after.
> > 
> > I commented on link-frequencies.
> > 
> > Best regards,
> > Krzysztof
> > 
> 
> Ah I understand you.
> 
> You're saying the link-frequencies we have in
> Documentation/devicetree/bindings/media/i2c/* are redundant absent hardware
> specific link frequencies being enumerated.
> 
> I'll either enumerate the acceptable set or drop this.

link-frequencies should remain mandatory in bindings, whether there are
hardware specific limits in bindings or not.
<URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Krzysztof Kozlowski]

On 03/10/2024 14:17, Sakari Ailus wrote:
> Hi Bryan, Krzysztof,
> 
> On Thu, Oct 03, 2024 at 12:54:41PM +0100, Bryan O'Donoghue wrote:
>> On 03/10/2024 11:17, Krzysztof Kozlowski wrote:
>>> On 03/10/2024 10:38, Bryan O'Donoghue wrote:
>>>> On 03/10/2024 09:33, Bryan O'Donoghue wrote:
>>>>> On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
>>>>>> On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
>>>>>>> +        properties:
>>>>>>> +          data-lanes:
>>>>>>> +            oneOf:
>>>>>>> +              - items:
>>>>>>> +                  - const: 1
>>>>>>> +                  - const: 2
>>>>>>> +              - items:
>>>>>>> +                  - const: 1
>>>>>>> +                  - const: 2
>>>>>>> +                  - const: 3
>>>>>>> +                  - const: 4
>>>>>>> +
>>>>>>> +          link-frequencies: true
>>>>>>
>>>>>> Not much changed here and you did not continued discussion about it.
>>>>>>
>>>>>> Best regards,
>>>>>> Krzysztof
>>>>>>
>>>>>
>>>>> Ah my mistake, I didn't read the bit at the bottom of your email
>>>>
>>>> I'll do this
>>>>
>>>> Documentation/devicetree/bindings/media/i2c/thine,thp7312.yaml
>>>>
>>>>             data-lanes:
>>>>               description:
>>>>                 This property is for lane reordering between the THP7312
>>>>                 and the SoC. The sensor supports either two-lane, or
>>>>                 four-lane operation.
>>>>                 If this property is omitted four-lane operation is
>>>>                 assumed. For two-lane operation the property must be
>>>>                 set to <1 2>.
>>>>               minItems: 2
>>>>               maxItems: 4
>>>>               items:
>>>>                 maximum: 4
>>>>
>>>> This captures what I'm after.
>>>
>>> I commented on link-frequencies.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Ah I understand you.
>>
>> You're saying the link-frequencies we have in
>> Documentation/devicetree/bindings/media/i2c/* are redundant absent hardware
>> specific link frequencies being enumerated.
>>
>> I'll either enumerate the acceptable set or drop this.
> 
> link-frequencies should remain mandatory in bindings, whether there are
> hardware specific limits in bindings or not.
> <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>

Yep and my comment was not under required field. Why all this discussion
is taken out of context? No wonder everyone interprets it differently.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Bryan O'Donoghue]

On 03/10/2024 13:31, Krzysztof Kozlowski wrote:
>>> Ah I understand you.
>>>
>>> You're saying the link-frequencies we have in
>>> Documentation/devicetree/bindings/media/i2c/* are redundant absent hardware
>>> specific link frequencies being enumerated.
>>>
>>> I'll either enumerate the acceptable set or drop this.
>> link-frequencies should remain mandatory in bindings, whether there are
>> hardware specific limits in bindings or not.
>> <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera- 
>> sensor.html#handling-clocks>
> Yep and my comment was not under required field. Why all this discussion
> is taken out of context? No wonder everyone interprets it differently.
> 
> Best regards,

Just so I'm 100% clear.

required:
     link-frequencies

is required but

properties:
     link-frequencies: true

is not, and presumably should be dropped from other yaml descriptions 
upstream.

---
bod

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Sakari Ailus]

Hi Bryan,

On Thu, Oct 03, 2024 at 01:40:34PM +0100, Bryan O'Donoghue wrote:
> On 03/10/2024 13:31, Krzysztof Kozlowski wrote:
> > > > Ah I understand you.
> > > > 
> > > > You're saying the link-frequencies we have in
> > > > Documentation/devicetree/bindings/media/i2c/* are redundant absent hardware
> > > > specific link frequencies being enumerated.
> > > > 
> > > > I'll either enumerate the acceptable set or drop this.
> > > link-frequencies should remain mandatory in bindings, whether there are
> > > hardware specific limits in bindings or not.
> > > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-
> > > sensor.html#handling-clocks>
> > Yep and my comment was not under required field. Why all this discussion
> > is taken out of context? No wonder everyone interprets it differently.
> > 
> > Best regards,
> 
> Just so I'm 100% clear.
> 
> required:
>     link-frequencies
> 
> is required but
> 
> properties:
>     link-frequencies: true
> 
> is not, and presumably should be dropped from other yaml descriptions
> upstream.

Seems good to me.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Bryan O'Donoghue]

On 03/10/2024 13:47, Sakari Ailus wrote:
> Hi Bryan,
> 
> On Thu, Oct 03, 2024 at 01:40:34PM +0100, Bryan O'Donoghue wrote:
>> On 03/10/2024 13:31, Krzysztof Kozlowski wrote:
>>>>> Ah I understand you.
>>>>>
>>>>> You're saying the link-frequencies we have in
>>>>> Documentation/devicetree/bindings/media/i2c/* are redundant absent hardware
>>>>> specific link frequencies being enumerated.
>>>>>
>>>>> I'll either enumerate the acceptable set or drop this.
>>>> link-frequencies should remain mandatory in bindings, whether there are
>>>> hardware specific limits in bindings or not.
>>>> <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-
>>>> sensor.html#handling-clocks>
>>> Yep and my comment was not under required field. Why all this discussion
>>> is taken out of context? No wonder everyone interprets it differently.
>>>
>>> Best regards,
>>
>> Just so I'm 100% clear.
>>
>> required:
>>      link-frequencies
>>
>> is required but
>>
>> properties:
>>      link-frequencies: true
>>
>> is not, and presumably should be dropped from other yaml descriptions
>> upstream.
> 
> Seems good to me.
> 

heh so I'll v4 this and since I'm a masochist, I'll send out a fixup 
series for the rest ...

---
bod

Re: [PATCH v3 2/4] media: dt-bindings: Add OmniVision OV08X40

[Sakari Ailus]

Hi Krzyzstof,

On Thu, Oct 03, 2024 at 02:31:17PM +0200, Krzysztof Kozlowski wrote:
> On 03/10/2024 14:17, Sakari Ailus wrote:
> > Hi Bryan, Krzysztof,
> > 
> > On Thu, Oct 03, 2024 at 12:54:41PM +0100, Bryan O'Donoghue wrote:
> >> On 03/10/2024 11:17, Krzysztof Kozlowski wrote:
> >>> On 03/10/2024 10:38, Bryan O'Donoghue wrote:
> >>>> On 03/10/2024 09:33, Bryan O'Donoghue wrote:
> >>>>> On 03/10/2024 09:29, Krzysztof Kozlowski wrote:
> >>>>>> On Wed, Oct 02, 2024 at 02:58:44PM +0100, Bryan O'Donoghue wrote:
> >>>>>>> +        properties:
> >>>>>>> +          data-lanes:
> >>>>>>> +            oneOf:
> >>>>>>> +              - items:
> >>>>>>> +                  - const: 1
> >>>>>>> +                  - const: 2
> >>>>>>> +              - items:
> >>>>>>> +                  - const: 1
> >>>>>>> +                  - const: 2
> >>>>>>> +                  - const: 3
> >>>>>>> +                  - const: 4
> >>>>>>> +
> >>>>>>> +          link-frequencies: true
> >>>>>>
> >>>>>> Not much changed here and you did not continued discussion about it.
> >>>>>>
> >>>>>> Best regards,
> >>>>>> Krzysztof
> >>>>>>
> >>>>>
> >>>>> Ah my mistake, I didn't read the bit at the bottom of your email
> >>>>
> >>>> I'll do this
> >>>>
> >>>> Documentation/devicetree/bindings/media/i2c/thine,thp7312.yaml
> >>>>
> >>>>             data-lanes:
> >>>>               description:
> >>>>                 This property is for lane reordering between the THP7312
> >>>>                 and the SoC. The sensor supports either two-lane, or
> >>>>                 four-lane operation.
> >>>>                 If this property is omitted four-lane operation is
> >>>>                 assumed. For two-lane operation the property must be
> >>>>                 set to <1 2>.
> >>>>               minItems: 2
> >>>>               maxItems: 4
> >>>>               items:
> >>>>                 maximum: 4
> >>>>
> >>>> This captures what I'm after.
> >>>
> >>> I commented on link-frequencies.
> >>>
> >>> Best regards,
> >>> Krzysztof
> >>>
> >>
> >> Ah I understand you.
> >>
> >> You're saying the link-frequencies we have in
> >> Documentation/devicetree/bindings/media/i2c/* are redundant absent hardware
> >> specific link frequencies being enumerated.
> >>
> >> I'll either enumerate the acceptable set or drop this.
> > 
> > link-frequencies should remain mandatory in bindings, whether there are
> > hardware specific limits in bindings or not.
> > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>
> 
> Yep and my comment was not under required field. Why all this discussion
> is taken out of context? No wonder everyone interprets it differently.

I wanted to just add that so we wouldn't have a need for v5. :-)

-- 
Kind regards,

Sakari Ailus

[PATCH v3 3/4] media: ov08x40: Rename ext_clk to xvclk

[Bryan O'Donoghue]

The data-sheet and documentation for this part uses the name xvclk not
ext_clk for the input reference clock. Rename the variables and defines in
this driver to align with the data-sheet name.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/i2c/ov08x40.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
index be25e45175b1322145dca428e845242d8fea2698..3ab8b51df157af78fcccc1aaef73aedb2ae759c9 100644
--- a/drivers/media/i2c/ov08x40.c
+++ b/drivers/media/i2c/ov08x40.c
@@ -1215,7 +1215,7 @@ static const char * const ov08x40_test_pattern_menu[] = {
 /* Configurations for supported link frequencies */
 #define OV08X40_LINK_FREQ_400MHZ	400000000ULL
 #define OV08X40_SCLK_96MHZ		96000000ULL
-#define OV08X40_EXT_CLK			19200000
+#define OV08X40_XVCLK			19200000
 #define OV08X40_DATA_LANES		4
 
 /*
@@ -2081,21 +2081,21 @@ static int ov08x40_check_hwcfg(struct device *dev)
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	unsigned int i, j;
 	int ret;
-	u32 ext_clk;
+	u32 xvclk_rate;
 
 	if (!fwnode)
 		return -ENXIO;
 
 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
-				       &ext_clk);
+				       &xvclk_rate);
 	if (ret) {
 		dev_err(dev, "can't get clock frequency");
 		return ret;
 	}
 
-	if (ext_clk != OV08X40_EXT_CLK) {
+	if (xvclk_rate != OV08X40_XVCLK) {
 		dev_err(dev, "external clock %d is not supported",
-			ext_clk);
+			xvclk_rate);
 		return -EINVAL;
 	}
 

-- 
2.46.2

[PATCH v3 4/4] media: ov08x40: Add OF probe support

[Bryan O'Donoghue]

The ACPI version of this driver "just works" on dts based systems with a
few extensions to facilitate.

- Add support for DT based probing
- Add support for taking the part out of reset via a GPIO reset pin
- Add in regulator bulk on/off logic for the power rails.

Once done this sensor works nicely on a Qualcomm X1E80100 CRD.

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e80100-crd
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/i2c/ov08x40.c | 138 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 124 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
index 3ab8b51df157af78fcccc1aaef73aedb2ae759c9..821102287580acecd544402254cfe0fb5c8dc299 100644
--- a/drivers/media/i2c/ov08x40.c
+++ b/drivers/media/i2c/ov08x40.c
@@ -3,10 +3,13 @@
 
 #include <asm-generic/unaligned.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/i2c.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -1279,6 +1282,12 @@ static const struct ov08x40_mode supported_modes[] = {
 	},
 };
 
+static const char * const ov08x40_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 struct ov08x40 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -1291,6 +1300,10 @@ struct ov08x40 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *exposure;
 
+	struct clk		*xvclk;
+	struct gpio_desc	*reset_gpio;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(ov08x40_supply_names)];
+
 	/* Current mode */
 	const struct ov08x40_mode *cur_mode;
 
@@ -1303,6 +1316,61 @@ struct ov08x40 {
 
 #define to_ov08x40(_sd)	container_of(_sd, struct ov08x40, sd)
 
+static int ov08x40_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov08x40 *ov08x = to_ov08x40(sd);
+	int ret;
+
+	if (is_acpi_node(dev_fwnode(dev)))
+		return 0;
+
+	ret = clk_prepare_enable(ov08x->xvclk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable xvclk\n");
+		return ret;
+	}
+
+	if (ov08x->reset_gpio) {
+		gpiod_set_value_cansleep(ov08x->reset_gpio, 1);
+		usleep_range(1000, 2000);
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov08x40_supply_names),
+				    ov08x->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable regulators\n");
+		goto disable_clk;
+	}
+
+	gpiod_set_value_cansleep(ov08x->reset_gpio, 0);
+	usleep_range(1500, 1800);
+
+	return 0;
+
+disable_clk:
+	gpiod_set_value_cansleep(ov08x->reset_gpio, 1);
+	clk_disable_unprepare(ov08x->xvclk);
+
+	return ret;
+}
+
+static int ov08x40_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov08x40 *ov08x = to_ov08x40(sd);
+
+	if (is_acpi_node(dev_fwnode(dev)))
+		return 0;
+
+	gpiod_set_value_cansleep(ov08x->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(ov08x40_supply_names),
+			       ov08x->supplies);
+	clk_disable_unprepare(ov08x->xvclk);
+
+	return 0;
+}
+
 /* Read registers up to 4 at a time */
 static int ov08x40_read_reg(struct ov08x40 *ov08x,
 			    u16 reg, u32 len, u32 *val)
@@ -2072,7 +2140,7 @@ static void ov08x40_free_controls(struct ov08x40 *ov08x)
 	mutex_destroy(&ov08x->mutex);
 }
 
-static int ov08x40_check_hwcfg(struct device *dev)
+static int ov08x40_check_hwcfg(struct ov08x40 *ov08x, struct device *dev)
 {
 	struct v4l2_fwnode_endpoint bus_cfg = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
@@ -2086,11 +2154,36 @@ static int ov08x40_check_hwcfg(struct device *dev)
 	if (!fwnode)
 		return -ENXIO;
 
-	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
-				       &xvclk_rate);
-	if (ret) {
-		dev_err(dev, "can't get clock frequency");
-		return ret;
+	if (!is_acpi_node(fwnode)) {
+		ov08x->xvclk = devm_clk_get(dev, NULL);
+		if (IS_ERR(ov08x->xvclk)) {
+			dev_err(dev, "could not get xvclk clock (%pe)\n",
+				ov08x->xvclk);
+			return PTR_ERR(ov08x->xvclk);
+		}
+
+		xvclk_rate = clk_get_rate(ov08x->xvclk);
+
+		ov08x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+							    GPIOD_OUT_LOW);
+		if (IS_ERR(ov08x->reset_gpio))
+			return PTR_ERR(ov08x->reset_gpio);
+
+		for (i = 0; i < ARRAY_SIZE(ov08x40_supply_names); i++)
+			ov08x->supplies[i].supply = ov08x40_supply_names[i];
+
+		ret = devm_regulator_bulk_get(dev,
+					      ARRAY_SIZE(ov08x40_supply_names),
+					      ov08x->supplies);
+		if (ret)
+			return ret;
+	} else {
+		ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
+					       &xvclk_rate);
+		if (ret) {
+			dev_err(dev, "can't get clock frequency");
+			return ret;
+		}
 	}
 
 	if (xvclk_rate != OV08X40_XVCLK) {
@@ -2143,32 +2236,37 @@ static int ov08x40_check_hwcfg(struct device *dev)
 }
 
 static int ov08x40_probe(struct i2c_client *client)
-{
-	struct ov08x40 *ov08x;
+{	struct ov08x40 *ov08x;
 	int ret;
 	bool full_power;
 
+	ov08x = devm_kzalloc(&client->dev, sizeof(*ov08x), GFP_KERNEL);
+	if (!ov08x)
+		return -ENOMEM;
+
 	/* Check HW config */
-	ret = ov08x40_check_hwcfg(&client->dev);
+	ret = ov08x40_check_hwcfg(ov08x, &client->dev);
 	if (ret) {
 		dev_err(&client->dev, "failed to check hwcfg: %d", ret);
 		return ret;
 	}
 
-	ov08x = devm_kzalloc(&client->dev, sizeof(*ov08x), GFP_KERNEL);
-	if (!ov08x)
-		return -ENOMEM;
-
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov08x->sd, client, &ov08x40_subdev_ops);
 
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
+		ret = ov08x40_power_on(&client->dev);
+		if (ret) {
+			dev_err(&client->dev, "failed to power on\n");
+			return ret;
+		}
+
 		/* Check module identity */
 		ret = ov08x40_identify_module(ov08x);
 		if (ret) {
 			dev_err(&client->dev, "failed to find sensor: %d\n", ret);
-			return ret;
+			goto probe_power_off;
 		}
 	}
 
@@ -2210,6 +2308,9 @@ static int ov08x40_probe(struct i2c_client *client)
 error_handler_free:
 	ov08x40_free_controls(ov08x);
 
+probe_power_off:
+	ov08x40_power_off(&client->dev);
+
 	return ret;
 }
 
@@ -2224,6 +2325,8 @@ static void ov08x40_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
+
+	ov08x40_power_off(&client->dev);
 }
 
 #ifdef CONFIG_ACPI
@@ -2235,10 +2338,17 @@ static const struct acpi_device_id ov08x40_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov08x40_acpi_ids);
 #endif
 
+static const struct of_device_id ov08x40_of_match[] = {
+	{ .compatible = "ovti,ov08x40" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov08x40_of_match);
+
 static struct i2c_driver ov08x40_i2c_driver = {
 	.driver = {
 		.name = "ov08x40",
 		.acpi_match_table = ACPI_PTR(ov08x40_acpi_ids),
+		.of_match_table = ov08x40_of_match,
 	},
 	.probe = ov08x40_probe,
 	.remove = ov08x40_remove,

-- 
2.46.2