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

[PATCH v6 0/9] 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 clocks and GPIOs.

Last year I dropped the ball as I didn't have access to HW anymore.
Luca (in cc) has reported he has a sensor and might give this new version
a spin, thanks!

Cheers
  j

v5->v6:
- Rework clock parsing as suggested by Sakari
- Move runtime_pm enablement after async subdev registration
- Use DIV_ROUND_UP to round clock freq

v4->v5:
- Enable clock in ov5670_runtime_resume() as suggested by Luca
- Add a patch to handle HBLANK, PIXEL_RATE and LINK_FREQ in .set_ctrl()
  to fix a warning again reported by Luca

v3->v4:
- Rework power enablement in power up sequence to support !CONFIG_OF
- Minor changes as per Sakari's review

v2->v3:
- bindings:
  - Drop assigned-clock properties from schema (moved to example)
  - s/pwdn-gpios/powerdown-gpios/

- driver
  - Use is_of_node() to decide how to parse clocks
  - Fix:
    drivers/media/i2c/ov5670.c:1787:18: error: initializer element is not a compile-time constant
                   .analog_crop = ov5670_analog_crop,
                                  ^~~~~~~~~~~~~~~~~~

    reported by kernel test robot and Nathan Chancellor with
    clang15 and gcc < 8

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 (8):
  media: dt-bindings: Add OV5670
  media: i2c: ov5670: Allow probing with OF
  media: i2c: ov5670: Use common clock framework
  media: i2c: ov5670: Probe regulators
  media: i2c: ov5670: Probe GPIOs
  media: i2c: ov5670: Add runtime_pm operations
  media: i2c: ov5670: Implement init_cfg
  media: i2c: ov5670: Handle RO controls in set_ctrl

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

 .../bindings/media/i2c/ovti,ov5670.yaml       |  92 ++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov5670.c                    | 312 +++++++++++++++---
 3 files changed, 360 insertions(+), 45 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml

--
2.39.0

[PATCH v6 1/9] media: dt-bindings: Add OV5670

[Jacopo Mondi]

Add the bindings documentation for Omnivision OV5670 image sensor.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../bindings/media/i2c/ovti,ov5670.yaml       | 92 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 93 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..fa264255b5eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
@@ -0,0 +1,92 @@
+# 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.mondi@ideasonboard.com>
+
+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
+
+  clocks:
+    description: System clock. From 6 to 27 MHz.
+    maxItems: 1
+
+  powerdown-gpios:
+    description: Reference to the GPIO connected to the PWDNB pin. Active low.
+
+  reset-gpios:
+    description: Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
+    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:
+            minItems: 1
+            maxItems: 2
+            items:
+              enum: [1, 2]
+
+          clock-noncontinuous: true
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5670: sensor@36 {
+            compatible = "ovti,ov5670";
+            reg = <0x36>;
+
+            clocks = <&sensor_xclk>;
+
+            port {
+                ov5670_ep: endpoint {
+                    remote-endpoint = <&csi_ep>;
+                    data-lanes = <1 2>;
+                    clock-noncontinuous;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f61eb221415b..38d8d1d5d536 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15468,6 +15468,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.39.0

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski]

On 26/01/2023 17:59, Jacopo Mondi wrote:
> Add the bindings documentation for Omnivision OV5670 image sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---

(...)

> +
> +  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:
> +            minItems: 1
> +            maxItems: 2
> +            items:
> +              enum: [1, 2]
> +
> +          clock-noncontinuous: true

You do not need this. Drop.

> +

Best regards,
Krzysztof

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Jacopo Mondi]

Hi Krzysztof

On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
> On 26/01/2023 17:59, Jacopo Mondi wrote:
> > Add the bindings documentation for Omnivision OV5670 image sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
>
> (...)
>
> > +
> > +  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:
> > +            minItems: 1
> > +            maxItems: 2
> > +            items:
> > +              enum: [1, 2]
> > +
> > +          clock-noncontinuous: true
>
> You do not need this. Drop.
>

Is this due to "unevaluatedProperties: false" ?

I read you recent explanation to a similar question on the Visconti
bindings. Let me summarize my understanding:

For a given schema a property could be
- required
        required:
          - foo

- optional
        by default with "unevaluatedProperties: false"
        "foo: true" with "additionalProperties: false"

- forbidden
        "foo: false" with "unevaluatedProperties: false"
        by default wiht "additionalProperties: false"

clock-noncontinuous is defined in video-interfaces.yaml. as I specify
"unevaluatedProperties: false" does it mean
all the properties defined in video-interfaces.yaml are optionally
accepted ? If that's the case that's not what I want as
clock-noncontinuous is -the only- property from that file we want to
accept here (and data-lanes ofc).

Should I change "unevaluatedProperties: false" to
"additionalProperties: false" and keep "clock-noncontinuous: true"  ?

Thanks
   j

> > +
>
> Best regards,
> Krzysztof
>

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski]

On 27/01/2023 19:14, Jacopo Mondi wrote:
> Hi Krzysztof
> 
> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
>> On 26/01/2023 17:59, Jacopo Mondi wrote:
>>> Add the bindings documentation for Omnivision OV5670 image sensor.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> ---
>>
>> (...)
>>
>>> +
>>> +  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:
>>> +            minItems: 1
>>> +            maxItems: 2
>>> +            items:
>>> +              enum: [1, 2]
>>> +
>>> +          clock-noncontinuous: true
>>
>> You do not need this. Drop.
>>
> 
> Is this due to "unevaluatedProperties: false" ?
> 
> I read you recent explanation to a similar question on the Visconti
> bindings. Let me summarize my understanding:
> 
> For a given schema a property could be
> - required
>         required:
>           - foo
> 
> - optional
>         by default with "unevaluatedProperties: false"
>         "foo: true" with "additionalProperties: false"
> 
> - forbidden
>         "foo: false" with "unevaluatedProperties: false"
>         by default wiht "additionalProperties: false"
> 
> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
> "unevaluatedProperties: false" does it mean
> all the properties defined in video-interfaces.yaml are optionally
> accepted ? If that's the case that's not what I want as
> clock-noncontinuous is -the only- property from that file we want to
> accept here (and data-lanes ofc).
> 
> Should I change "unevaluatedProperties: false" to
> "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
>

Why would you disallow other properties? Just because driver does not
use them? That's not correct, driver change but bindings should stay the
same.

Best regards,
Krzysztof

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Sakari Ailus]

Hi Krzysztof,

On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
> On 27/01/2023 19:14, Jacopo Mondi wrote:
> > Hi Krzysztof
> > 
> > On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
> >> On 26/01/2023 17:59, Jacopo Mondi wrote:
> >>> Add the bindings documentation for Omnivision OV5670 image sensor.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >>> ---
> >>
> >> (...)
> >>
> >>> +
> >>> +  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:
> >>> +            minItems: 1
> >>> +            maxItems: 2
> >>> +            items:
> >>> +              enum: [1, 2]
> >>> +
> >>> +          clock-noncontinuous: true
> >>
> >> You do not need this. Drop.
> >>
> > 
> > Is this due to "unevaluatedProperties: false" ?
> > 
> > I read you recent explanation to a similar question on the Visconti
> > bindings. Let me summarize my understanding:
> > 
> > For a given schema a property could be
> > - required
> >         required:
> >           - foo
> > 
> > - optional
> >         by default with "unevaluatedProperties: false"
> >         "foo: true" with "additionalProperties: false"
> > 
> > - forbidden
> >         "foo: false" with "unevaluatedProperties: false"
> >         by default wiht "additionalProperties: false"
> > 
> > clock-noncontinuous is defined in video-interfaces.yaml. as I specify
> > "unevaluatedProperties: false" does it mean
> > all the properties defined in video-interfaces.yaml are optionally
> > accepted ? If that's the case that's not what I want as
> > clock-noncontinuous is -the only- property from that file we want to
> > accept here (and data-lanes ofc).
> > 
> > Should I change "unevaluatedProperties: false" to
> > "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
> >
> 
> Why would you disallow other properties? Just because driver does not
> use them? That's not correct, driver change but bindings should stay the
> same.

The clock-noncontinuous property is relevant for the hardware. There are
some properties not listed here that might be relevant (for all camera
sensors) but most properties in video-interfaces.yaml are not applicable to
this device.

I also think is be useful to say what is relevant in DT bindings, as the
other sources of information left are hardware datasheets (if you have
access to them) or the driver (which is supposed not to be relevant for the
bindings).

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski]

On 27/01/2023 21:38, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
>> On 27/01/2023 19:14, Jacopo Mondi wrote:
>>> Hi Krzysztof
>>>
>>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
>>>> On 26/01/2023 17:59, Jacopo Mondi wrote:
>>>>> Add the bindings documentation for Omnivision OV5670 image sensor.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>> ---
>>>>
>>>> (...)
>>>>
>>>>> +
>>>>> +  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:
>>>>> +            minItems: 1
>>>>> +            maxItems: 2
>>>>> +            items:
>>>>> +              enum: [1, 2]
>>>>> +
>>>>> +          clock-noncontinuous: true
>>>>
>>>> You do not need this. Drop.
>>>>
>>>
>>> Is this due to "unevaluatedProperties: false" ?
>>>
>>> I read you recent explanation to a similar question on the Visconti
>>> bindings. Let me summarize my understanding:
>>>
>>> For a given schema a property could be
>>> - required
>>>         required:
>>>           - foo
>>>
>>> - optional
>>>         by default with "unevaluatedProperties: false"
>>>         "foo: true" with "additionalProperties: false"
>>>
>>> - forbidden
>>>         "foo: false" with "unevaluatedProperties: false"
>>>         by default wiht "additionalProperties: false"
>>>
>>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
>>> "unevaluatedProperties: false" does it mean
>>> all the properties defined in video-interfaces.yaml are optionally
>>> accepted ? If that's the case that's not what I want as
>>> clock-noncontinuous is -the only- property from that file we want to
>>> accept here (and data-lanes ofc).
>>>
>>> Should I change "unevaluatedProperties: false" to
>>> "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
>>>
>>
>> Why would you disallow other properties? Just because driver does not
>> use them? That's not correct, driver change but bindings should stay the
>> same.
> 
> The clock-noncontinuous property is relevant for the hardware. There are
> some properties not listed here that might be relevant (for all camera
> sensors) but most properties in video-interfaces.yaml are not applicable to
> this device.
> 
> I also think is be useful to say what is relevant in DT bindings, as the
> other sources of information left are hardware datasheets (if you have
> access to them) or the driver (which is supposed not to be relevant for the
> bindings).
> 

Then it might be meaningful to list all allowed properties - even if not
currently supported by the driver - and use additionalProperties:false.
This has drawback - whenever video-interfaces gets something new, the
bindings here (and other such devices) will have to be explicitly enabled.

Best regards,
Krzysztof

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Jacopo Mondi]

Hi Krzysztof

On Fri, Jan 27, 2023 at 09:44:25PM +0100, Krzysztof Kozlowski wrote:
> On 27/01/2023 21:38, Sakari Ailus wrote:
> > Hi Krzysztof,
> >
> > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
> >> On 27/01/2023 19:14, Jacopo Mondi wrote:
> >>> Hi Krzysztof
> >>>
> >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 26/01/2023 17:59, Jacopo Mondi wrote:
> >>>>> Add the bindings documentation for Omnivision OV5670 image sensor.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >>>>> ---
> >>>>
> >>>> (...)
> >>>>
> >>>>> +
> >>>>> +  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:
> >>>>> +            minItems: 1
> >>>>> +            maxItems: 2
> >>>>> +            items:
> >>>>> +              enum: [1, 2]
> >>>>> +
> >>>>> +          clock-noncontinuous: true
> >>>>
> >>>> You do not need this. Drop.
> >>>>
> >>>
> >>> Is this due to "unevaluatedProperties: false" ?
> >>>
> >>> I read you recent explanation to a similar question on the Visconti
> >>> bindings. Let me summarize my understanding:
> >>>
> >>> For a given schema a property could be
> >>> - required
> >>>         required:
> >>>           - foo
> >>>
> >>> - optional
> >>>         by default with "unevaluatedProperties: false"
> >>>         "foo: true" with "additionalProperties: false"
> >>>
> >>> - forbidden
> >>>         "foo: false" with "unevaluatedProperties: false"
> >>>         by default wiht "additionalProperties: false"
> >>>
> >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
> >>> "unevaluatedProperties: false" does it mean
> >>> all the properties defined in video-interfaces.yaml are optionally
> >>> accepted ? If that's the case that's not what I want as
> >>> clock-noncontinuous is -the only- property from that file we want to
> >>> accept here (and data-lanes ofc).
> >>>
> >>> Should I change "unevaluatedProperties: false" to
> >>> "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
> >>>
> >>
> >> Why would you disallow other properties? Just because driver does not
> >> use them? That's not correct, driver change but bindings should stay the
> >> same.
> >
> > The clock-noncontinuous property is relevant for the hardware. There are
> > some properties not listed here that might be relevant (for all camera
> > sensors) but most properties in video-interfaces.yaml are not applicable to
> > this device.
> >
> > I also think is be useful to say what is relevant in DT bindings, as the
> > other sources of information left are hardware datasheets (if you have
> > access to them) or the driver (which is supposed not to be relevant for the
> > bindings).
> >
>
> Then it might be meaningful to list all allowed properties - even if not
> currently supported by the driver - and use additionalProperties:false.

Have a look at what properties video-interfaces.yaml lists. Some of
them only apply to CSI-2 sensors (data lanes, link-frequencies etc),
some of them only to parallel sensors (lines polarities, bus-width
etc).

I see most of the bindings in media/i2c reporting

        $ref: /schemas/media/video-interfaces.yaml#
        unevaluatedProperties: false

I think that's actually wrong as there's no way all the properties in
video-interfaces.yaml can apply to a single device (with the exception
of a few sensors that support both bus types).

> This has drawback - whenever video-interfaces gets something new, the
> bindings here (and other such devices) will have to be explicitly enabled.

video-interfaces is rarely updated, and when it happes it's to add
properties required by a newly supported device, so this doesn't
concern me much personally.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Sakari Ailus]

Hi Jacopo, Krzysztof,

On Sat, Jan 28, 2023 at 10:58:31AM +0100, Jacopo Mondi wrote:
> Hi Krzysztof
> 
> On Fri, Jan 27, 2023 at 09:44:25PM +0100, Krzysztof Kozlowski wrote:
> > On 27/01/2023 21:38, Sakari Ailus wrote:
> > > Hi Krzysztof,
> > >
> > > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
> > >> On 27/01/2023 19:14, Jacopo Mondi wrote:
> > >>> Hi Krzysztof
> > >>>
> > >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
> > >>>> On 26/01/2023 17:59, Jacopo Mondi wrote:
> > >>>>> Add the bindings documentation for Omnivision OV5670 image sensor.
> > >>>>>
> > >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >>>>> ---
> > >>>>
> > >>>> (...)
> > >>>>
> > >>>>> +
> > >>>>> +  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:
> > >>>>> +            minItems: 1
> > >>>>> +            maxItems: 2
> > >>>>> +            items:
> > >>>>> +              enum: [1, 2]
> > >>>>> +
> > >>>>> +          clock-noncontinuous: true
> > >>>>
> > >>>> You do not need this. Drop.
> > >>>>
> > >>>
> > >>> Is this due to "unevaluatedProperties: false" ?
> > >>>
> > >>> I read you recent explanation to a similar question on the Visconti
> > >>> bindings. Let me summarize my understanding:
> > >>>
> > >>> For a given schema a property could be
> > >>> - required
> > >>>         required:
> > >>>           - foo
> > >>>
> > >>> - optional
> > >>>         by default with "unevaluatedProperties: false"
> > >>>         "foo: true" with "additionalProperties: false"
> > >>>
> > >>> - forbidden
> > >>>         "foo: false" with "unevaluatedProperties: false"
> > >>>         by default wiht "additionalProperties: false"
> > >>>
> > >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
> > >>> "unevaluatedProperties: false" does it mean
> > >>> all the properties defined in video-interfaces.yaml are optionally
> > >>> accepted ? If that's the case that's not what I want as
> > >>> clock-noncontinuous is -the only- property from that file we want to
> > >>> accept here (and data-lanes ofc).
> > >>>
> > >>> Should I change "unevaluatedProperties: false" to
> > >>> "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
> > >>>
> > >>
> > >> Why would you disallow other properties? Just because driver does not
> > >> use them? That's not correct, driver change but bindings should stay the
> > >> same.
> > >
> > > The clock-noncontinuous property is relevant for the hardware. There are
> > > some properties not listed here that might be relevant (for all camera
> > > sensors) but most properties in video-interfaces.yaml are not applicable to
> > > this device.
> > >
> > > I also think is be useful to say what is relevant in DT bindings, as the
> > > other sources of information left are hardware datasheets (if you have
> > > access to them) or the driver (which is supposed not to be relevant for the
> > > bindings).
> > >
> >
> > Then it might be meaningful to list all allowed properties - even if not
> > currently supported by the driver - and use additionalProperties:false.
> 
> Have a look at what properties video-interfaces.yaml lists. Some of
> them only apply to CSI-2 sensors (data lanes, link-frequencies etc),
> some of them only to parallel sensors (lines polarities, bus-width
> etc).
> 
> I see most of the bindings in media/i2c reporting
> 
>         $ref: /schemas/media/video-interfaces.yaml#
>         unevaluatedProperties: false
> 
> I think that's actually wrong as there's no way all the properties in
> video-interfaces.yaml can apply to a single device (with the exception
> of a few sensors that support both bus types).

It's been in my plan to split this into multiple files so you could refer
to fewer than all the properties. I have no schedule for this though.

> 
> > This has drawback - whenever video-interfaces gets something new, the
> > bindings here (and other such devices) will have to be explicitly enabled.
> 
> video-interfaces is rarely updated, and when it happes it's to add
> properties required by a newly supported device, so this doesn't
> concern me much personally.

Me neither.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Jacopo Mondi]

Since I got the attention of both of you, let me point out another
issue I'm facing.

We also have video-interface-devices.yaml which lists properties for
the device node and not for the endpoints.

video-interface-devices lists properties that should be all optionally
accepted, as they can potentially apply to all sensors (things like
rotation, orientation, lens-focus, flash-leds are valid for all
devices)

Being properties for the device node they should be specified in the
schema top-level and I see a few schema that do that by

        allOf:
          - $ref: /schemas/media/video-interface-devices.yaml#

However top level schemas usually specify

        additionalProperties: false

Which means each sensor schema has to list the properties it accepts from
video-interface-devices.yaml. It's easy to verify this just by
adding "orientation" to the example in a schema that refers to
video-interface-devices.yaml and see that the bindings validation
fails (see below)

TL;DR is there a way to tell in a schema with a top-level
"additionalProperties: false" that all properties from a referenced
schema are accepted ?

I'll leave video-interface-devices.yaml this out from this series for
now and only resend this patch with the previous comments on the usage
of unevaluatedProperties fixed.

Thanks
  j


-----------------------------------------------------------------------------
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
@@ -109,6 +109,7 @@ examples:
               powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
               reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
               rotation = <180>;
+              orientation = <0>;

               port {
                   /* MIPI CSI-2 bus endpoint */


$ make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
  DTEX    Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dts
  DTC_CHK Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dtb
  /home/jmondi/linux-worktree/mainline/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dtb: camera@3c: 'orientation' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /home/jmondi/linux-worktree/mainline/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml

On Sat, Jan 28, 2023 at 12:07:44PM +0200, Sakari Ailus wrote:
> Hi Jacopo, Krzysztof,
>
> On Sat, Jan 28, 2023 at 10:58:31AM +0100, Jacopo Mondi wrote:
> > Hi Krzysztof
> >
> > On Fri, Jan 27, 2023 at 09:44:25PM +0100, Krzysztof Kozlowski wrote:
> > > On 27/01/2023 21:38, Sakari Ailus wrote:
> > > > Hi Krzysztof,
> > > >
> > > > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
> > > >> On 27/01/2023 19:14, Jacopo Mondi wrote:
> > > >>> Hi Krzysztof
> > > >>>
> > > >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
> > > >>>> On 26/01/2023 17:59, Jacopo Mondi wrote:
> > > >>>>> Add the bindings documentation for Omnivision OV5670 image sensor.
> > > >>>>>
> > > >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >>>>> ---
> > > >>>>
> > > >>>> (...)
> > > >>>>
> > > >>>>> +
> > > >>>>> +  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:
> > > >>>>> +            minItems: 1
> > > >>>>> +            maxItems: 2
> > > >>>>> +            items:
> > > >>>>> +              enum: [1, 2]
> > > >>>>> +
> > > >>>>> +          clock-noncontinuous: true
> > > >>>>
> > > >>>> You do not need this. Drop.
> > > >>>>
> > > >>>
> > > >>> Is this due to "unevaluatedProperties: false" ?
> > > >>>
> > > >>> I read you recent explanation to a similar question on the Visconti
> > > >>> bindings. Let me summarize my understanding:
> > > >>>
> > > >>> For a given schema a property could be
> > > >>> - required
> > > >>>         required:
> > > >>>           - foo
> > > >>>
> > > >>> - optional
> > > >>>         by default with "unevaluatedProperties: false"
> > > >>>         "foo: true" with "additionalProperties: false"
> > > >>>
> > > >>> - forbidden
> > > >>>         "foo: false" with "unevaluatedProperties: false"
> > > >>>         by default wiht "additionalProperties: false"
> > > >>>
> > > >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
> > > >>> "unevaluatedProperties: false" does it mean
> > > >>> all the properties defined in video-interfaces.yaml are optionally
> > > >>> accepted ? If that's the case that's not what I want as
> > > >>> clock-noncontinuous is -the only- property from that file we want to
> > > >>> accept here (and data-lanes ofc).
> > > >>>
> > > >>> Should I change "unevaluatedProperties: false" to
> > > >>> "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
> > > >>>
> > > >>
> > > >> Why would you disallow other properties? Just because driver does not
> > > >> use them? That's not correct, driver change but bindings should stay the
> > > >> same.
> > > >
> > > > The clock-noncontinuous property is relevant for the hardware. There are
> > > > some properties not listed here that might be relevant (for all camera
> > > > sensors) but most properties in video-interfaces.yaml are not applicable to
> > > > this device.
> > > >
> > > > I also think is be useful to say what is relevant in DT bindings, as the
> > > > other sources of information left are hardware datasheets (if you have
> > > > access to them) or the driver (which is supposed not to be relevant for the
> > > > bindings).
> > > >
> > >
> > > Then it might be meaningful to list all allowed properties - even if not
> > > currently supported by the driver - and use additionalProperties:false.
> >
> > Have a look at what properties video-interfaces.yaml lists. Some of
> > them only apply to CSI-2 sensors (data lanes, link-frequencies etc),
> > some of them only to parallel sensors (lines polarities, bus-width
> > etc).
> >
> > I see most of the bindings in media/i2c reporting
> >
> >         $ref: /schemas/media/video-interfaces.yaml#
> >         unevaluatedProperties: false
> >
> > I think that's actually wrong as there's no way all the properties in
> > video-interfaces.yaml can apply to a single device (with the exception
> > of a few sensors that support both bus types).
>
> It's been in my plan to split this into multiple files so you could refer
> to fewer than all the properties. I have no schedule for this though.
>
> >
> > > This has drawback - whenever video-interfaces gets something new, the
> > > bindings here (and other such devices) will have to be explicitly enabled.
> >
> > video-interfaces is rarely updated, and when it happes it's to add
> > properties required by a newly supported device, so this doesn't
> > concern me much personally.
>
> Me neither.
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski]

On 28/01/2023 12:03, Jacopo Mondi wrote:
> Since I got the attention of both of you, let me point out another
> issue I'm facing.
> 
> We also have video-interface-devices.yaml which lists properties for
> the device node and not for the endpoints.
> 
> video-interface-devices lists properties that should be all optionally
> accepted, as they can potentially apply to all sensors (things like
> rotation, orientation, lens-focus, flash-leds are valid for all
> devices)
> 
> Being properties for the device node they should be specified in the
> schema top-level and I see a few schema that do that by
> 
>         allOf:
>           - $ref: /schemas/media/video-interface-devices.yaml#
> 
> However top level schemas usually specify
> 
>         additionalProperties: false
> 
> Which means each sensor schema has to list the properties it accepts from
> video-interface-devices.yaml. It's easy to verify this just by
> adding "orientation" to the example in a schema that refers to
> video-interface-devices.yaml and see that the bindings validation
> fails (see below)
> 
> TL;DR is there a way to tell in a schema with a top-level
> "additionalProperties: false" that all properties from a referenced
> schema are accepted ?

No, because this would make it exactly the same as
unevaluatedProperties, so we would have two keywords with same meaning.

https://lore.kernel.org/all/c2740d66-b51f-efc2-6583-a69bde950c68@linaro.org/

Best regards,
Krzysztof

[PATCH v6.1] media: dt-bindings: Add OV5670

[Jacopo Mondi]

Add the bindings documentation for Omnivision OV5670 image sensor.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
v6->6.1
- Use additionalProperties: false for endpoint properties from
  video-interfaces.yaml
- List 'remote-endpoint' among the accepted endpoint properties
  now that we use additionalProperties: false
---
 .../bindings/media/i2c/ovti,ov5670.yaml       | 93 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 94 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..6e089fe1d613
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5670 5 Megapixels raw image sensor
+
+maintainers:
+  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+
+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
+
+  clocks:
+    description: System clock. From 6 to 27 MHz.
+    maxItems: 1
+
+  powerdown-gpios:
+    description: Reference to the GPIO connected to the PWDNB pin. Active low.
+
+  reset-gpios:
+    description: Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
+    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#
+        additionalProperties: false
+
+        properties:
+          data-lanes:
+            minItems: 1
+            maxItems: 2
+            items:
+              enum: [1, 2]
+
+          clock-noncontinuous: true
+          remote-endpoint: true
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5670: sensor@36 {
+            compatible = "ovti,ov5670";
+            reg = <0x36>;
+
+            clocks = <&sensor_xclk>;
+
+            port {
+                ov5670_ep: endpoint {
+                    remote-endpoint = <&csi_ep>;
+                    data-lanes = <1 2>;
+                    clock-noncontinuous;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f61eb221415b..38d8d1d5d536 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15468,6 +15468,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.39.0

Re: [PATCH v6.1] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski]

On 28/01/2023 12:27, Jacopo Mondi wrote:
> Add the bindings documentation for Omnivision OV5670 image sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> v6->6.1
> - Use additionalProperties: false for endpoint properties from
>   video-interfaces.yaml
> - List 'remote-endpoint' among the accepted endpoint properties
>   now that we use additionalProperties: false

b4 diff '20230128112736.8000-1-jacopo.mondi@ideasonboard.com'
Could not create fake-am range for lower series v1

Can you send patches in a way it does not break out workflows? Why
making our review process more difficult?

Best regards,
Krzysztof

Re: [PATCH v6.1] media: dt-bindings: Add OV5670

[Jacopo Mondi]

On Sun, Jan 29, 2023 at 12:40:03PM +0100, Krzysztof Kozlowski wrote:
> On 28/01/2023 12:27, Jacopo Mondi wrote:
> > Add the bindings documentation for Omnivision OV5670 image sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > v6->6.1
> > - Use additionalProperties: false for endpoint properties from
> >   video-interfaces.yaml
> > - List 'remote-endpoint' among the accepted endpoint properties
> >   now that we use additionalProperties: false
>
> b4 diff '20230128112736.8000-1-jacopo.mondi@ideasonboard.com'
> Could not create fake-am range for lower series v1
>
> Can you send patches in a way it does not break out workflows? Why
> making our review process more difficult?

Because it's a nit on a 10 patches series with no other changes
requested ?

What is difficult exactly ?

I see several patches in linux-media being sent inline to a previous
version for small fixes if the only required changed is a nit like
this one.


> Best regards,
> Krzysztof
>

Re: [PATCH v6.1] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski]

On 29/01/2023 13:11, Jacopo Mondi wrote:
> On Sun, Jan 29, 2023 at 12:40:03PM +0100, Krzysztof Kozlowski wrote:
>> On 28/01/2023 12:27, Jacopo Mondi wrote:
>>> Add the bindings documentation for Omnivision OV5670 image sensor.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> ---
>>> v6->6.1
>>> - Use additionalProperties: false for endpoint properties from
>>>   video-interfaces.yaml
>>> - List 'remote-endpoint' among the accepted endpoint properties
>>>   now that we use additionalProperties: false
>>
>> b4 diff '20230128112736.8000-1-jacopo.mondi@ideasonboard.com'
>> Could not create fake-am range for lower series v1
>>
>> Can you send patches in a way it does not break out workflows? Why
>> making our review process more difficult?
> 
> Because it's a nit on a 10 patches series with no other changes
> requested ?
> 
> What is difficult exactly ?

I wrote above what's difficult.

> 
> I see several patches in linux-media being sent inline to a previous
> version for small fixes if the only required changed is a nit like
> this one.

If you sent it as separate v7 would be fine, but:
1. Threading is wrong - it's buried in other patch.
2. Version is wrong - you did there changes, not nits. There are no
point versions...

Best regards,
Krzysztof

Re: [PATCH v6.1] media: dt-bindings: Add OV5670

[Rob Herring]

On Sun, Jan 29, 2023 at 01:11:32PM +0100, Jacopo Mondi wrote:
> On Sun, Jan 29, 2023 at 12:40:03PM +0100, Krzysztof Kozlowski wrote:
> > On 28/01/2023 12:27, Jacopo Mondi wrote:
> > > Add the bindings documentation for Omnivision OV5670 image sensor.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > > v6->6.1
> > > - Use additionalProperties: false for endpoint properties from
> > >   video-interfaces.yaml
> > > - List 'remote-endpoint' among the accepted endpoint properties
> > >   now that we use additionalProperties: false
> >
> > b4 diff '20230128112736.8000-1-jacopo.mondi@ideasonboard.com'
> > Could not create fake-am range for lower series v1
> >
> > Can you send patches in a way it does not break out workflows? Why
> > making our review process more difficult?
> 
> Because it's a nit on a 10 patches series with no other changes
> requested ?

So? Think of patch series as an 'email transport' for your git branches. 
If you rebase your branch, that's a whole new branch to send.

> What is difficult exactly ?

In addition to 'b4 diff', if a maintainer is applying this series, for a 
v7 they just do:

b4 shazam msgid-of-v7

For v6.1, they do:

b4 shazam msgid-of-v6
git rebase -i ...
<stop on patch 1>
git reset --hard HEAD^
b4 shazam msgid-of-v6.1
git rebase --continue

Which one makes the maintainer's life easier?

If it's a CI job trying to apply and test this, there's no way it's 
going to do the second case.

Rob

Re: [PATCH v6.1] media: dt-bindings: Add OV5670

[Jacopo Mondi]

On Mon, Jan 30, 2023 at 09:58:40AM -0600, Rob Herring wrote:
> On Sun, Jan 29, 2023 at 01:11:32PM +0100, Jacopo Mondi wrote:
> > On Sun, Jan 29, 2023 at 12:40:03PM +0100, Krzysztof Kozlowski wrote:
> > > On 28/01/2023 12:27, Jacopo Mondi wrote:
> > > > Add the bindings documentation for Omnivision OV5670 image sensor.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > > v6->6.1
> > > > - Use additionalProperties: false for endpoint properties from
> > > >   video-interfaces.yaml
> > > > - List 'remote-endpoint' among the accepted endpoint properties
> > > >   now that we use additionalProperties: false
> > >
> > > b4 diff '20230128112736.8000-1-jacopo.mondi@ideasonboard.com'
> > > Could not create fake-am range for lower series v1
> > >
> > > Can you send patches in a way it does not break out workflows? Why
> > > making our review process more difficult?
> >
> > Because it's a nit on a 10 patches series with no other changes
> > requested ?
>
> So? Think of patch series as an 'email transport' for your git branches.
> If you rebase your branch, that's a whole new branch to send.
>

So if a series has a single comment and could be then collected as it
is but one patch I saw it happening multiple times on the ML and I
thought it was an accepted practice.


> > What is difficult exactly ?
>
> In addition to 'b4 diff', if a maintainer is applying this series, for a
> v7 they just do:
>
> b4 shazam msgid-of-v7
>
> For v6.1, they do:
>
> b4 shazam msgid-of-v6
> git rebase -i ...
> <stop on patch 1>
> git reset --hard HEAD^
> b4 shazam msgid-of-v6.1
> git rebase --continue
>
> Which one makes the maintainer's life easier?
>

With b4 it now certainly makes a difference.

As I save patches from my mail client and apply them manually I never
really considered picking one patch over the other from the same
thread "more difficult". I should have noticed when Krzysztof
mentioned b4 in his first reply.

> If it's a CI job trying to apply and test this, there's no way it's
> going to do the second case.
>

That's another point yes.

Got your message, I'll stop :)

Don't think a v7 is needed for this on though (if not other
comments ofc)

> Rob

Re: [PATCH v6.1] media: dt-bindings: Add OV5670

[Krzysztof Kozlowski]

On 28/01/2023 12:27, Jacopo Mondi wrote:
> Add the bindings documentation for Omnivision OV5670 image sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> v6->6.1
> - Use additionalProperties: false for endpoint properties from
>   video-interfaces.yaml
> - List 'remote-endpoint' among the accepted endpoint properties
>   now that we use additionalProperties: false
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index bc9fc3bc90c2..07a396c8ab68 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -3,7 +3,9 @@
 
 #include <linux/acpi.h>
 #include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -2583,11 +2585,18 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
 #endif
 
+static const struct of_device_id ov5670_of_ids[] = {
+	{ .compatible = "ovti,ov5670" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov5670_of_ids);
+
 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 = ov5670_of_ids,
 	},
 	.probe_new = ov5670_probe,
 	.remove = ov5670_remove,
-- 
2.39.0

[PATCH v6 3/9] media: i2c: ov5670: Use common clock framework

[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'.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 07a396c8ab68..52b799a7491c 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>
@@ -12,6 +13,8 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 
+#define OV5670_XVCLK_FREQ		19200000
+
 #define OV5670_REG_CHIP_ID		0x300a
 #define OV5670_CHIP_ID			0x005670
 
@@ -1830,6 +1833,9 @@ struct ov5670 {
 	/* Current mode */
 	const struct ov5670_mode *cur_mode;
 
+	/* xvclk input clock */
+	struct clk *xvclk;
+
 	/* To serialize asynchronus callbacks */
 	struct mutex mutex;
 
@@ -2478,10 +2484,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 +2491,22 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	ov5670->xvclk = devm_clk_get(&client->dev, NULL);
+	if (!IS_ERR_OR_NULL(ov5670->xvclk))
+		input_clk = clk_get_rate(ov5670->xvclk);
+	else if (PTR_ERR(ov5670->xvclk) == -ENOENT)
+		device_property_read_u32(&client->dev, "clock-frequency",
+					 &input_clk);
+	else
+		return dev_err_probe(&client->dev, PTR_ERR(ov5670->xvclk),
+				     "error getting clock\n");
+
+	if (input_clk != OV5670_XVCLK_FREQ) {
+		dev_err(&client->dev,
+			"Unsupported clock frequency %u\n", input_clk);
+		return -EINVAL;
+	}
+
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
-- 
2.39.0

[PATCH v6 4/9] 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.mondi@ideasonboard.com>
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 52b799a7491c..e71f13360480 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>
@@ -88,6 +89,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;
@@ -1836,6 +1845,9 @@ struct ov5670 {
 	/* xvclk input clock */
 	struct clk *xvclk;
 
+	/* Regulators */
+	struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES];
+
 	/* To serialize asynchronus callbacks */
 	struct mutex mutex;
 
@@ -2476,6 +2488,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;
@@ -2510,6 +2534,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.39.0

[PATCH v6 5/9] 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.mondi@ideasonboard.com>
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 e71f13360480..0290f33f619d 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>
@@ -1848,6 +1849,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;
 
@@ -2500,6 +2505,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, "powerdown",
+						    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;
@@ -2540,6 +2562,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.39.0

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

[Jacopo Mondi]

Implement the runtime resume and suspend routines and install them as
runtime_pm handlers.

While at it rework the probe() sequence in order to enable runtime_pm
before registering the async subdevice.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 78 +++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 0290f33f619d..47fedbe37ced 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2017 Intel Corporation.
 
+#include <asm/unaligned.h>
 #include <linux/acpi.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
@@ -2429,6 +2431,49 @@ 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);
+	unsigned long delay_us;
+	int ret;
+
+	ret = clk_prepare_enable(ov5670->xvclk);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
+	if (ret) {
+		clk_disable_unprepare(ov5670->xvclk);
+		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. */
+	delay_us = DIV_ROUND_UP(8192 * 2 * 1000,
+				DIV_ROUND_UP(OV5670_XVCLK_FREQ, 1000));
+	fsleep(delay_us);
+
+	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);
+	clk_disable_unprepare(ov5670->xvclk);
+
+	return 0;
+}
+
 static int __maybe_unused ov5670_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -2570,11 +2615,17 @@ static int ov5670_probe(struct i2c_client *client)
 
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
+		ret = ov5670_runtime_resume(&client->dev);
+		if (ret) {
+			err_msg = "Power up failed";
+			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;
 		}
 	}
 
@@ -2603,24 +2654,27 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_handler_free;
 	}
 
-	/* Async register for subdev */
-	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
-	if (ret < 0) {
-		err_msg = "v4l2_async_register_subdev() error";
-		goto error_entity_cleanup;
-	}
-
 	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);
+
+	/* Async register for subdev */
+	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
+	if (ret < 0) {
+		err_msg = "v4l2_async_register_subdev() error";
+		goto error_pm_disable;
+	}
+
 	pm_runtime_idle(&client->dev);
 
 	return 0;
 
-error_entity_cleanup:
+error_pm_disable:
+	pm_runtime_disable(&client->dev);
+
 	media_entity_cleanup(&ov5670->sd.entity);
 
 error_handler_free:
@@ -2629,6 +2683,10 @@ static int ov5670_probe(struct i2c_client *client)
 error_mutex_destroy:
 	mutex_destroy(&ov5670->mutex);
 
+error_power_off:
+	if (full_power)
+		ov5670_runtime_suspend(&client->dev);
+
 error_print:
 	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
 
@@ -2646,10 +2704,12 @@ static void ov5670_remove(struct i2c_client *client)
 	mutex_destroy(&ov5670->mutex);
 
 	pm_runtime_disable(&client->dev);
+	ov5670_runtime_suspend(&client->dev);
 }
 
 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.39.0

[PATCH v6 7/9] media: i2c: ov5670: Implement init_cfg

[Jacopo Mondi]

Implement the .init_cfg() pad operation and initialize the default
format with the default full resolution mode 2592x1944.

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

Signed-off-by: Jacopo Mondi <jacopo.mondi@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 47fedbe37ced..898f564e0c3e 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1962,27 +1962,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;
@@ -2182,6 +2161,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)
@@ -2513,6 +2511,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,
@@ -2534,10 +2533,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);
@@ -2640,7 +2635,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.39.0

[PATCH v6 8/9] 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 modes 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 modes
definitions, to make sure that if any mode gets added, its crop
rectangle will be defined as well.

While at it re-sort the modes' 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.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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 898f564e0c3e..e2a3db7e4e20 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -74,6 +74,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
 
@@ -116,10 +120,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},
@@ -1767,66 +1786,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,
 	}
 };
 
@@ -2167,6 +2193,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;
@@ -2177,6 +2204,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;
 }
 
@@ -2506,6 +2535,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,
 };
@@ -2516,6 +2591,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.39.0

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

[Luca Weiss]

On Donnerstag, 26. Jänner 2023 17:59:08 CET 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 modes 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 modes
> definitions, to make sure that if any mode gets added, its crop
> rectangle will be defined as well.
> 
> While at it re-sort the modes' 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.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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 898f564e0c3e..e2a3db7e4e20 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -74,6 +74,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
> 
> @@ -116,10 +120,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.

nit: 03800 should be 0x3800

Regards
Luca

> + */
> +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},
> @@ -1767,66 +1786,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,
>  	}
>  };
> 
> @@ -2167,6 +2193,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;
> @@ -2177,6 +2204,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;
>  }
> 
> @@ -2506,6 +2535,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,
>  };
> @@ -2516,6 +2591,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 = {

[PATCH v6 9/9] media: i2c: ov5670: Handle RO controls in set_ctrl

[Jacopo Mondi]

The ov5670 driver registers three controls as read-only:
- V4L2_CID_PIXEL_RATE
- V4L2_CID_LINK_FREQ
- V4L2_CID_HBLANK

The driver updates the range of HBLANK with __v4l2_ctrl_modify_range()
and updates the values of PIXEL_RATE and LINK_FREQ with an
explicit call to __v4l2_ctrl_s_ctrl() in ov5670_set_pad_format() time.

This causes the .set_ctrl handler to be called on these controls
causing a non-fatal warning to be emitted:

	ov5670_set_ctrl Unhandled id:0x9e0902, val:0x824

This is currently only critical for HBLANK, as LINK_FREQ and PIXEL_RATE
currently only support a single value, and the v4l2-ctrl framework skips
calling .set_ctrl() if the current control value is not changed.

Expand the ov5670_set_ctrl() callback to handle the above controls
to remove the above warning and defend against future expansions
of the supported pixel rates and link frequencies.

Also be stricter and return an error value if a control is actually not
handled.

Reported-by: Luca Weiss <luca@z3ntu.xyz>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index e2a3db7e4e20..da02d01f2e37 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2038,7 +2038,7 @@ static int ov5670_set_ctrl(struct v4l2_ctrl *ctrl)
 					     struct ov5670, ctrl_handler);
 	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
 	s64 max;
-	int ret = 0;
+	int ret;
 
 	/* Propagate change of current control to all related controls */
 	switch (ctrl->id) {
@@ -2077,7 +2077,13 @@ static int ov5670_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_TEST_PATTERN:
 		ret = ov5670_enable_test_pattern(ov5670, ctrl->val);
 		break;
+	case V4L2_CID_HBLANK:
+	case V4L2_CID_LINK_FREQ:
+	case V4L2_CID_PIXEL_RATE:
+		ret = 0;
+		break;
 	default:
+		ret = -EINVAL;
 		dev_info(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n",
 			 __func__, ctrl->id, ctrl->val);
 		break;
-- 
2.39.0

Re: [PATCH v6 0/9] media: i2c: ov5670: OF support, runtime_pm, regulators

[Luca Weiss]

Hi Jacopo,

On Donnerstag, 26. Jänner 2023 17:59:00 CET Jacopo Mondi wrote:
> Hello
>   this small series introduces OF support for the ov5670 sensor and
> adds support for regulators clocks and GPIOs.
> 
> Last year I dropped the ball as I didn't have access to HW anymore.
> Luca (in cc) has reported he has a sensor and might give this new version
> a spin, thanks!

With this series the sensor initializes correctly for me on msm8974pro-
fairphone-fp2. I locally have essentially one more change that configures the 
sensor for single-lane operation (currently dual-lane is hardcoded) and with 
that I can get image that using libcamera.

Tested-by: Luca Weiss <luca@z3ntu.xyz>


Thanks for respinning this!

Regards
Luca

> 
> Cheers
>   j
> 
> v5->v6:
> - Rework clock parsing as suggested by Sakari
> - Move runtime_pm enablement after async subdev registration
> - Use DIV_ROUND_UP to round clock freq
> 
> v4->v5:
> - Enable clock in ov5670_runtime_resume() as suggested by Luca
> - Add a patch to handle HBLANK, PIXEL_RATE and LINK_FREQ in .set_ctrl()
>   to fix a warning again reported by Luca
> 
> v3->v4:
> - Rework power enablement in power up sequence to support !CONFIG_OF
> - Minor changes as per Sakari's review
> 
> v2->v3:
> - bindings:
>   - Drop assigned-clock properties from schema (moved to example)
>   - s/pwdn-gpios/powerdown-gpios/
> 
> - driver
>   - Use is_of_node() to decide how to parse clocks
>   - Fix:
>     drivers/media/i2c/ov5670.c:1787:18: error: initializer element is not a
> compile-time constant .analog_crop = ov5670_analog_crop,
>                                   ^~~~~~~~~~~~~~~~~~
> 
>     reported by kernel test robot and Nathan Chancellor with
>     clang15 and gcc < 8
> 
> 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 (8):
>   media: dt-bindings: Add OV5670
>   media: i2c: ov5670: Allow probing with OF
>   media: i2c: ov5670: Use common clock framework
>   media: i2c: ov5670: Probe regulators
>   media: i2c: ov5670: Probe GPIOs
>   media: i2c: ov5670: Add runtime_pm operations
>   media: i2c: ov5670: Implement init_cfg
>   media: i2c: ov5670: Handle RO controls in set_ctrl
> 
> Jean-Michel Hautbois (1):
>   media: i2c: ov5670: Add .get_selection() support
> 
>  .../bindings/media/i2c/ovti,ov5670.yaml       |  92 ++++++
>  MAINTAINERS                                   |   1 +
>  drivers/media/i2c/ov5670.c                    | 312 +++++++++++++++---
>  3 files changed, 360 insertions(+), 45 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> 
> --
> 2.39.0