[PATCH v7 0/3] media: ov8856: Add devicetree support

[PATCH v7 0/3] media: ov8856: Add devicetree support

[Robert Foss]

This adds devicetree support to the ov8856 driver.
In order to to aid debugging and enable future sensor
modes to be supported, module revision detection is also added.


Dongchun Zhu (1):
  media: dt-bindings: ov8856: Document YAML bindings

Robert Foss (2):
  media: ov8856: Add devicetree support
  media: ov8856: Implement sensor module revision identification

 .../devicetree/bindings/media/i2c/ov8856.yaml | 142 +++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov8856.c                    | 190 ++++++++++++++++--
 3 files changed, 319 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

-- 
2.25.1

[PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Robert Foss]

From: Dongchun Zhu <dongchun.zhu@mediatek.com>

This patch adds documentation of device tree in YAML schema for the
OV8856 CMOS image sensor.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
Signed-off-by: Robert Foss <robert.foss@linaro.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
---

- Changes since v9:
  * Remove remote-endpoint property
  * Marco: Make port property required again
  * Sakari: Remove Ben as a maintainer
  * Sakari: Replace data-lanes with const items
  * Sakari: Remove clock-lanes property
  * Sakari & Rob Herring: Change type of link-frequency
    work around dt-schema bug

- Changes since v8:
  * Maxime: Added r-b

- Changes since v7:
  * Marco: Make 'port' property optional
  * Maxime & Sakari: Add 'link-frequencies' property to dt binding
  * robher: Improve description for 'port' property

- Changes since v6:
  * Marco: remove qcom specifics from DT example
   
- Changes since v5:
  * Add assigned-clocks and assigned-clock-rates
  * robher: dt-schema errors

- Changes since v4:
  * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
  * Add clock-lanes property to example
  * robher: Fix syntax error in devicetree example

- Changes since v3:
  * robher: Fix syntax error
  * robher: Removed maxItems
  * Fixes yaml 'make dt-binding-check' errors

- Changes since v2:

- Changes since v1:
  Fixes comments from Sakari, Tomasz
  * Add clock-frequency and link-frequencies in DT

 .../devicetree/bindings/media/i2c/ov8856.yaml | 142 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 143 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
new file mode 100644
index 000000000000..d6af685ad3ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2019 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
+
+maintainers:
+  - Dongchun Zhu <dongchun.zhu@mediatek.com>
+
+description: |-
+  The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS
+  image sensor that delivers 3264x2448 at 30fps. It provides full-frame,
+  sub-sampled, and windowed 10-bit MIPI images in various formats via the
+  Serial Camera Control Bus (SCCB) interface. This chip is programmable
+  through I2C and two-wire SCCB. The sensor output is available via CSI-2
+  serial data output (up to 4-lane).
+
+properties:
+  compatible:
+    const: ovti,ov8856
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description:
+      Input clock for the sensor.
+    items:
+      - const: xvclk
+
+  clock-frequency:
+    description:
+      Frequency of the xvclk clock in Hertz.
+
+  dovdd-supply:
+    description:
+      Definition of the regulator used as interface power supply.
+
+  avdd-supply:
+    description:
+      Definition of the regulator used as analog power supply.
+
+  dvdd-supply:
+    description:
+      Definition of the regulator used as digital power supply.
+
+  reset-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls sensor reset.
+      This corresponds to the hardware pin XSHUTDOWN which is physically
+      active low.
+
+  port:
+    type: object
+    additionalProperties: false
+    description:
+      A node containing an output port node with an endpoint definition
+      as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          data-lanes:
+            description: |-
+              The driver only supports four-lane operation.
+            items:
+              - const: 1
+              - const: 2
+              - const: 3
+              - const: 4
+
+          link-frequencies:
+            allOf:
+              - $ref: /schemas/types.yaml#/definitions/uint64-array
+            description:
+              Allowed data bus frequencies. 360000000, 180000000 Hz or both
+              are supported by the driver.
+
+
+        required:
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov8856: camera@10 {
+            compatible = "ovti,ov8856";
+            reg = <0x10>;
+
+            reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&clk_24m_cam>;
+
+            clocks = <&cam_osc>;
+            clock-names = "xvclk";
+            clock-frequency = <19200000>;
+
+            avdd-supply = <&mt6358_vcama2_reg>;
+            dvdd-supply = <&mt6358_vcamd_reg>;
+            dovdd-supply = <&mt6358_vcamio_reg>;
+
+            port {
+                wcam_out: endpoint {
+                    remote-endpoint = <&mipi_in_wcam>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <360000000>;
+                };
+            };
+        };
+    };
+...
\ No newline at end of file
diff --git a/MAINTAINERS b/MAINTAINERS
index 26f281d9f32a..84b262afd13d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12489,6 +12489,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/i2c/ov8856.c
+F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
 
 OMNIVISION OV9640 SENSOR DRIVER
 M:	Petr Cvek <petrcvekcz@gmail.com>
-- 
2.25.1

Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Dongchun Zhu]

Hi Robert,

Thanks for the update.

On Tue, 2020-05-05 at 12:01 +0200, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> 
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> ---
> 
> - Changes since v9:
>   * Remove remote-endpoint property
>   * Marco: Make port property required again
>   * Sakari: Remove Ben as a maintainer
>   * Sakari: Replace data-lanes with const items
>   * Sakari: Remove clock-lanes property
>   * Sakari & Rob Herring: Change type of link-frequency
>     work around dt-schema bug
> 
> - Changes since v8:
>   * Maxime: Added r-b
> 
> - Changes since v7:
>   * Marco: Make 'port' property optional
>   * Maxime & Sakari: Add 'link-frequencies' property to dt binding
>   * robher: Improve description for 'port' property
> 
> - Changes since v6:
>   * Marco: remove qcom specifics from DT example
>    
> - Changes since v5:
>   * Add assigned-clocks and assigned-clock-rates
>   * robher: dt-schema errors
> 
> - Changes since v4:
>   * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
>   * Add clock-lanes property to example
>   * robher: Fix syntax error in devicetree example
> 
> - Changes since v3:
>   * robher: Fix syntax error
>   * robher: Removed maxItems
>   * Fixes yaml 'make dt-binding-check' errors
> 
> - Changes since v2:
> 
> - Changes since v1:
>   Fixes comments from Sakari, Tomasz
>   * Add clock-frequency and link-frequencies in DT
> 
>  .../devicetree/bindings/media/i2c/ov8856.yaml | 142 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 143 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> new file mode 100644
> index 000000000000..d6af685ad3ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2019 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> +
> +description: |-
> +  The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS
> +  image sensor that delivers 3264x2448 at 30fps. It provides full-frame,
> +  sub-sampled, and windowed 10-bit MIPI images in various formats via the
> +  Serial Camera Control Bus (SCCB) interface. This chip is programmable
> +  through I2C and two-wire SCCB. The sensor output is available via CSI-2
> +  serial data output (up to 4-lane).
> +
> +properties:
> +  compatible:
> +    const: ovti,ov8856
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      Input clock for the sensor.
> +    items:
> +      - const: xvclk
> +
> +  clock-frequency:
> +    description:
> +      Frequency of the xvclk clock in Hertz.
> +
> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as interface power supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as analog power supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as digital power supply.
> +
> +  reset-gpios:
> +    description:
> +      The phandle and specifier for the GPIO that controls sensor reset.
> +      This corresponds to the hardware pin XSHUTDOWN which is physically
> +      active low.
> +
> +  port:
> +    type: object
> +    additionalProperties: false
> +    description:
> +      A node containing an output port node with an endpoint definition
> +      as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +    properties:
> +      endpoint:
> +        type: object
> +
> +        properties:
> +          data-lanes:
> +            description: |-
> +              The driver only supports four-lane operation.
> +            items:
> +              - const: 1
> +              - const: 2
> +              - const: 3
> +              - const: 4
> +
> +          link-frequencies:
> +            allOf:
> +              - $ref: /schemas/types.yaml#/definitions/uint64-array
> +            description:
> +              Allowed data bus frequencies. 360000000, 180000000 Hz or both
> +              are supported by the driver.
> +
> +
> +        required:
> +          - link-frequencies
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - dovdd-supply
> +  - avdd-supply
> +  - dvdd-supply
> +  - reset-gpios
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov8856: camera@10 {
> +            compatible = "ovti,ov8856";
> +            reg = <0x10>;
> +
> +            reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;

Apologies for missing to follow the earlier discussion related to this.
I noticed the GPIO flag para and __ov8856_power_on() are aligned using
ACTIVE_LOW.

But from the datasheet, XSHUTDN pin is active-high for OV8856.
It means devm_gpiod_get API (in probe func) should use GPIOD_OUT_LOW to
initialize the GPIO as output with a value of 0.
Otherwise it should use GPIOD_OUT_HIGH.

There is one case for GPIO_ACTIVE_LOW setting:
https://patchwork.linuxtv.org/patch/63460/
https://patchwork.linuxtv.org/patch/63461/

Sakari, Tomasz, am I right?

> +            pinctrl-names = "default";
> +            pinctrl-0 = <&clk_24m_cam>;
> +
> +            clocks = <&cam_osc>;
> +            clock-names = "xvclk";
> +            clock-frequency = <19200000>;
> +
> +            avdd-supply = <&mt6358_vcama2_reg>;
> +            dvdd-supply = <&mt6358_vcamd_reg>;
> +            dovdd-supply = <&mt6358_vcamio_reg>;
> +
> +            port {
> +                wcam_out: endpoint {
> +                    remote-endpoint = <&mipi_in_wcam>;
> +                    data-lanes = <1 2 3 4>;
> +                    link-frequencies = /bits/ 64 <360000000>;
> +                };
> +            };
> +        };
> +    };
> +...
> \ No newline at end of file
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 26f281d9f32a..84b262afd13d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12489,6 +12489,7 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  T:	git git://linuxtv.org/media_tree.git
>  F:	drivers/media/i2c/ov8856.c
> +F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
>  

Had you run parse-maintainers.pl?
The new item is supposed to be arranged in alphabetical order.

>  OMNIVISION OV9640 SENSOR DRIVER
>  M:	Petr Cvek <petrcvekcz@gmail.com>

Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Robert Foss]

Hey Dongchun,

Thanks for having a look at this series.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov8856: camera@10 {
> > +            compatible = "ovti,ov8856";
> > +            reg = <0x10>;
> > +
> > +            reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
>
> Apologies for missing to follow the earlier discussion related to this.
> I noticed the GPIO flag para and __ov8856_power_on() are aligned using
> ACTIVE_LOW.
>
> But from the datasheet, XSHUTDN pin is active-high for OV8856.
> It means devm_gpiod_get API (in probe func) should use GPIOD_OUT_LOW to
> initialize the GPIO as output with a value of 0.
> Otherwise it should use GPIOD_OUT_HIGH.
>
> There is one case for GPIO_ACTIVE_LOW setting:
> https://patchwork.linuxtv.org/patch/63460/
> https://patchwork.linuxtv.org/patch/63461/

We went back and forth about this a few times, and I switched to this
gpio setting after having worked through the device probing reset gpio
toggling. Semantically it seemed easier to understand in the driver,
since the gpio is called reset and not !shutdown.

Looking into devm_gpiod_get_optional(), the flag argument
GPIOD_OUT_LOW or HIGH for that matter is actually not used initialize
the output, but only used for an exclusivity check.
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-devres.c#L109

If you prefer, I can invert the logic again. To me making the reset
gpio active resulting in the device being actually reset seems like
the most intuitive and easy to understand option.
The different OmniVision drivers seem to have different approaches to
this. The ov9640 driver for example is doing what this series
currently is doing.

>
> Sakari, Tomasz, am I right?
>
> > +            pinctrl-names = "default";
> > +            pinctrl-0 = <&clk_24m_cam>;
> > +
> > +            clocks = <&cam_osc>;
> > +            clock-names = "xvclk";
> > +            clock-frequency = <19200000>;
> > +
> > +            avdd-supply = <&mt6358_vcama2_reg>;
> > +            dvdd-supply = <&mt6358_vcamd_reg>;
> > +            dovdd-supply = <&mt6358_vcamio_reg>;
> > +
> > +            port {
> > +                wcam_out: endpoint {
> > +                    remote-endpoint = <&mipi_in_wcam>;
> > +                    data-lanes = <1 2 3 4>;
> > +                    link-frequencies = /bits/ 64 <360000000>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +...
> > \ No newline at end of file
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 26f281d9f32a..84b262afd13d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12489,6 +12489,7 @@ L:    linux-media@vger.kernel.org
> >  S:   Maintained
> >  T:   git git://linuxtv.org/media_tree.git
> >  F:   drivers/media/i2c/ov8856.c
> > +F:   Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> >
>
> Had you run parse-maintainers.pl?
> The new item is supposed to be arranged in alphabetical order.

No, I have not. But upon running it now, it doesn't make suggest any
changes. But let me order the files manually in the next revision.

However, I noticed I removed the wrong person from the maintainers
file in this revision.
So, I'll correct that and add you Dongchun as the maintainer if that's ok.

>
> >  OMNIVISION OV9640 SENSOR DRIVER
> >  M:   Petr Cvek <petrcvekcz@gmail.com>
>

Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Marco Felsch]

On 20-05-06 13:29, Robert Foss wrote:
> Hey Dongchun,
> 
> Thanks for having a look at this series.
> 
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        ov8856: camera@10 {
> > > +            compatible = "ovti,ov8856";
> > > +            reg = <0x10>;
> > > +
> > > +            reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
> >
> > Apologies for missing to follow the earlier discussion related to this.
> > I noticed the GPIO flag para and __ov8856_power_on() are aligned using
> > ACTIVE_LOW.
> >
> > But from the datasheet, XSHUTDN pin is active-high for OV8856.
> > It means devm_gpiod_get API (in probe func) should use GPIOD_OUT_LOW to
> > initialize the GPIO as output with a value of 0.
> > Otherwise it should use GPIOD_OUT_HIGH.
> >
> > There is one case for GPIO_ACTIVE_LOW setting:
> > https://patchwork.linuxtv.org/patch/63460/
> > https://patchwork.linuxtv.org/patch/63461/
> 
> We went back and forth about this a few times, and I switched to this
> gpio setting after having worked through the device probing reset gpio
> toggling. Semantically it seemed easier to understand in the driver,
> since the gpio is called reset and not !shutdown.

IMHO you can keep your version. DTs are part of the system integration.
What if one system has a invert logic infront of the gpio input.. The
system integrator needs to read and to understand the schematic
correctly to pick the correct value.

> Looking into devm_gpiod_get_optional(), the flag argument
> GPIOD_OUT_LOW or HIGH for that matter is actually not used initialize

The good think about gpiod is that it care about logic values not
physical/electrical values. If you set GPIOD_OUT_HIGH then the reset is
asserted, whatever asserted means electrical.

Regards,
  Marco

> the output, but only used for an exclusivity check.
> https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-devres.c#L109
> 
> If you prefer, I can invert the logic again. To me making the reset
> gpio active resulting in the device being actually reset seems like
> the most intuitive and easy to understand option.
> The different OmniVision drivers seem to have different approaches to
> this. The ov9640 driver for example is doing what this series
> currently is doing.
> 
> >
> > Sakari, Tomasz, am I right?
> >
> > > +            pinctrl-names = "default";
> > > +            pinctrl-0 = <&clk_24m_cam>;
> > > +
> > > +            clocks = <&cam_osc>;
> > > +            clock-names = "xvclk";
> > > +            clock-frequency = <19200000>;
> > > +
> > > +            avdd-supply = <&mt6358_vcama2_reg>;
> > > +            dvdd-supply = <&mt6358_vcamd_reg>;
> > > +            dovdd-supply = <&mt6358_vcamio_reg>;
> > > +
> > > +            port {
> > > +                wcam_out: endpoint {
> > > +                    remote-endpoint = <&mipi_in_wcam>;
> > > +                    data-lanes = <1 2 3 4>;
> > > +                    link-frequencies = /bits/ 64 <360000000>;
> > > +                };
> > > +            };
> > > +        };
> > > +    };
> > > +...
> > > \ No newline at end of file
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 26f281d9f32a..84b262afd13d 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -12489,6 +12489,7 @@ L:    linux-media@vger.kernel.org
> > >  S:   Maintained
> > >  T:   git git://linuxtv.org/media_tree.git
> > >  F:   drivers/media/i2c/ov8856.c
> > > +F:   Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > >
> >
> > Had you run parse-maintainers.pl?
> > The new item is supposed to be arranged in alphabetical order.
> 
> No, I have not. But upon running it now, it doesn't make suggest any
> changes. But let me order the files manually in the next revision.
> 
> However, I noticed I removed the wrong person from the maintainers
> file in this revision.
> So, I'll correct that and add you Dongchun as the maintainer if that's ok.
> 
> >
> > >  OMNIVISION OV9640 SENSOR DRIVER
> > >  M:   Petr Cvek <petrcvekcz@gmail.com>
> >
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Rob Herring]

On Tue,  5 May 2020 12:01:29 +0200, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> 
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> ---
> 
> - Changes since v9:
>   * Remove remote-endpoint property
>   * Marco: Make port property required again
>   * Sakari: Remove Ben as a maintainer
>   * Sakari: Replace data-lanes with const items
>   * Sakari: Remove clock-lanes property
>   * Sakari & Rob Herring: Change type of link-frequency
>     work around dt-schema bug
> 
> - Changes since v8:
>   * Maxime: Added r-b
> 
> - Changes since v7:
>   * Marco: Make 'port' property optional
>   * Maxime & Sakari: Add 'link-frequencies' property to dt binding
>   * robher: Improve description for 'port' property
> 
> - Changes since v6:
>   * Marco: remove qcom specifics from DT example
>    
> - Changes since v5:
>   * Add assigned-clocks and assigned-clock-rates
>   * robher: dt-schema errors
> 
> - Changes since v4:
>   * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
>   * Add clock-lanes property to example
>   * robher: Fix syntax error in devicetree example
> 
> - Changes since v3:
>   * robher: Fix syntax error
>   * robher: Removed maxItems
>   * Fixes yaml 'make dt-binding-check' errors
> 
> - Changes since v2:
> 
> - Changes since v1:
>   Fixes comments from Sakari, Tomasz
>   * Add clock-frequency and link-frequencies in DT
> 
>  .../devicetree/bindings/media/i2c/ov8856.yaml | 142 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 143 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Rob Herring]

On Tue,  5 May 2020 12:01:29 +0200, Robert Foss wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> 
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> ---
> 
> - Changes since v9:
>   * Remove remote-endpoint property
>   * Marco: Make port property required again
>   * Sakari: Remove Ben as a maintainer
>   * Sakari: Replace data-lanes with const items
>   * Sakari: Remove clock-lanes property
>   * Sakari & Rob Herring: Change type of link-frequency
>     work around dt-schema bug
> 
> - Changes since v8:
>   * Maxime: Added r-b
> 
> - Changes since v7:
>   * Marco: Make 'port' property optional
>   * Maxime & Sakari: Add 'link-frequencies' property to dt binding
>   * robher: Improve description for 'port' property
> 
> - Changes since v6:
>   * Marco: remove qcom specifics from DT example
>    
> - Changes since v5:
>   * Add assigned-clocks and assigned-clock-rates
>   * robher: dt-schema errors
> 
> - Changes since v4:
>   * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
>   * Add clock-lanes property to example
>   * robher: Fix syntax error in devicetree example
> 
> - Changes since v3:
>   * robher: Fix syntax error
>   * robher: Removed maxItems
>   * Fixes yaml 'make dt-binding-check' errors
> 
> - Changes since v2:
> 
> - Changes since v1:
>   Fixes comments from Sakari, Tomasz
>   * Add clock-frequency and link-frequencies in DT
> 
>  .../devicetree/bindings/media/i2c/ov8856.yaml | 142 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 143 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Robert Foss]

Thanks Rob!

On Tue, 5 May 2020 at 17:49, Rob Herring <robh@kernel.org> wrote:
>
> On Tue,  5 May 2020 12:01:29 +0200, Robert Foss wrote:
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> >
> > This patch adds documentation of device tree in YAML schema for the
> > OV8856 CMOS image sensor.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >
> > - Changes since v9:
> >   * Remove remote-endpoint property
> >   * Marco: Make port property required again
> >   * Sakari: Remove Ben as a maintainer
> >   * Sakari: Replace data-lanes with const items
> >   * Sakari: Remove clock-lanes property
> >   * Sakari & Rob Herring: Change type of link-frequency
> >     work around dt-schema bug
> >
> > - Changes since v8:
> >   * Maxime: Added r-b
> >
> > - Changes since v7:
> >   * Marco: Make 'port' property optional
> >   * Maxime & Sakari: Add 'link-frequencies' property to dt binding
> >   * robher: Improve description for 'port' property
> >
> > - Changes since v6:
> >   * Marco: remove qcom specifics from DT example
> >
> > - Changes since v5:
> >   * Add assigned-clocks and assigned-clock-rates
> >   * robher: dt-schema errors
> >
> > - Changes since v4:
> >   * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description
> >   * Add clock-lanes property to example
> >   * robher: Fix syntax error in devicetree example
> >
> > - Changes since v3:
> >   * robher: Fix syntax error
> >   * robher: Removed maxItems
> >   * Fixes yaml 'make dt-binding-check' errors
> >
> > - Changes since v2:
> >
> > - Changes since v1:
> >   Fixes comments from Sakari, Tomasz
> >   * Add clock-frequency and link-frequencies in DT
> >
> >  .../devicetree/bindings/media/i2c/ov8856.yaml | 142 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 143 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v7 2/3] media: ov8856: Add devicetree support

[Robert Foss]

Add match table, enable ov8856_probe() to support
both ACPI and DT modes.

ACPI and DT modes are primarily distinguished from
by checking for ACPI mode and by having resource like
be NULL.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---

- Changes since v6:
  * Marco: Bail out of __ov8856_power_on earlier if ACPI mode

- Changes since v5:
  * Maxime & Sakari: Replaced clock tolerance check with warning

- Changes since v4:
  * Maxime & Sakari: Switch to clock-frequency

- Changes since v3:
  * Remove redundant {}-brackets
  * Compare xvclk_rate to 5% tolerance
  * Andy: Use dev_fwnode()
  * Andy: Use %pe instead of %ld + PTR_ERR()
  * Andy: Invert reset_gpio logic
  * Andy: Remove dev_dbg() from failing reset_gpio setup
  * Andy: Use dev_err for logging for failures
  * Andy: Remove dev_warn from EDEFER/regulator error path
  * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
  * Maxime & Sakari: Verify clock frequency from DT
  * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
  * Sakari: Remove duplicate ov8856->dev assignment

- Changes since v2:
  * Added "struct device *dev" member to struct ov8856
  * Andy: Switch to optional version of devm_gpiod_get
  * Andy: Switch to optional version of devm_clk_get
  * Fabio: Add reset sleep period
  * Sakari: Unify defines for 19.2Mhz
  * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
  * Sakari: Replace dev_info() with dev_dbg()
  * Sakari: Switch induction variable type to unsigned
  * Sakari: Don't wait for reset_gpio when in ACPI mode
  * Sakari: Pull reset GPIO high on power on failure
  * Sakari: Add power on/off to resume/suspend
  * Sakari: Fix indentation
  * Sakari: Power off during ov8856_remove()
  * Sakari: Don't sleep during power-on in ACPI mode
  * Sakari: Switch to getting xvclk from clk_get_rate

- Changes since v1:
  * Andy & Sakari: Make XVCLK optional since to not break ACPI
  * Fabio: Change n_shutdown_gpio name to reset_gpio
  * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
  * Fabio: Remove empty line
  * Fabio: Remove real error from devm_gpiod_get() failures
  * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
  * Sakari: Use XVCLK rate as provided by DT

 drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
 1 file changed, 123 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..e6418a79801e 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -3,10 +3,13 @@
 
 #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/module.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>
@@ -18,7 +21,7 @@
 #define OV8856_LINK_FREQ_360MHZ		360000000ULL
 #define OV8856_LINK_FREQ_180MHZ		180000000ULL
 #define OV8856_SCLK			144000000ULL
-#define OV8856_MCLK			19200000
+#define OV8856_XVCLK_19_2		19200000
 #define OV8856_DATA_LANES		4
 #define OV8856_RGB_DEPTH		10
 
@@ -64,6 +67,12 @@
 
 #define to_ov8856(_sd)			container_of(_sd, struct ov8856, sd)
 
+static const char * const ov8856_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 enum {
 	OV8856_LINK_FREQ_720MBPS,
 	OV8856_LINK_FREQ_360MBPS,
@@ -566,6 +575,11 @@ struct ov8856 {
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
 
+	struct device		*dev;
+	struct clk		*xvclk;
+	struct gpio_desc	*reset_gpio;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
+
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
@@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int __ov8856_power_on(struct ov8856 *ov8856)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
+	int ret;
+
+	if (is_acpi_node(dev_fwnode(ov8856->dev)))
+		return 0;
+
+	ret = clk_prepare_enable(ov8856->xvclk);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to enable xvclk\n");
+		return ret;
+	}
+
+	if (ov8856->reset_gpio) {
+		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+		usleep_range(1000, 2000);
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
+				    ov8856->supplies);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to enable regulators\n");
+		goto disable_clk;
+	}
+
+	gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
+	usleep_range(1500, 1800);
+
+	return 0;
+
+disable_clk:
+	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+	clk_disable_unprepare(ov8856->xvclk);
+
+	return ret;
+}
+
+static void __ov8856_power_off(struct ov8856 *ov8856)
+{
+	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
+			       ov8856->supplies);
+	clk_disable_unprepare(ov8856->xvclk);
+}
+
 static int __maybe_unused ov8856_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
 	if (ov8856->streaming)
 		ov8856_stop_streaming(ov8856);
 
+	__ov8856_power_off(ov8856);
 	mutex_unlock(&ov8856->mutex);
 
 	return 0;
@@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
 	int ret;
 
 	mutex_lock(&ov8856->mutex);
+
+	__ov8856_power_on(ov8856);
 	if (ov8856->streaming) {
 		ret = ov8856_start_streaming(ov8856);
 		if (ret) {
@@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
 	return 0;
 }
 
-static int ov8856_check_hwcfg(struct device *dev)
+static int ov8856_get_hwcfg(struct ov8856 *ov8856)
 {
+	struct device *dev = ov8856->dev;
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
-	u32 mclk;
+	u32 xvclk_rate;
 	int ret;
 	unsigned int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
 
-	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
+	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
+		&xvclk_rate);
 	if (ret)
 		return ret;
 
-	if (mclk != OV8856_MCLK) {
-		dev_err(dev, "external clock %d is not supported", mclk);
-		return -EINVAL;
+	if (!is_acpi_node(fwnode)) {
+		ov8856->xvclk = devm_clk_get(dev, "xvclk");
+		if (IS_ERR(ov8856->xvclk)) {
+			dev_err(dev, "could not get xvclk clock (%pe)\n",
+					ov8856->xvclk);
+			return PTR_ERR(ov8856->xvclk);
+		}
+
+		clk_set_rate(ov8856->xvclk, xvclk_rate);
+		xvclk_rate = clk_get_rate(ov8856->xvclk);
 	}
 
+	if (xvclk_rate != OV8856_XVCLK_19_2)
+		dev_warn(dev, "external clock rate %d is unsupported", xvclk_rate);
+
+	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+		GPIOD_OUT_LOW);
+	if (IS_ERR(ov8856->reset_gpio))
+		return PTR_ERR(ov8856->reset_gpio);
+
+	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
+		ov8856->supplies[i].supply = ov8856_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
+				      ov8856->supplies);
+	if (ret)
+		return ret;
+
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
 		return -ENXIO;
@@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	mutex_destroy(&ov8856->mutex);
 
+	__ov8856_power_off(ov8856);
+
 	return 0;
 }
 
@@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
 	struct ov8856 *ov8856;
 	int ret;
 
-	ret = ov8856_check_hwcfg(&client->dev);
+	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
+	if (!ov8856)
+		return -ENOMEM;
+
+	ov8856->dev = &client->dev;
+
+	ret = ov8856_get_hwcfg(ov8856);
 	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
+		dev_err(&client->dev, "failed to get HW configuration: %d",
 			ret);
 		return ret;
 	}
 
-	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
-	if (!ov8856)
-		return -ENOMEM;
-
 	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
+
+	ret = __ov8856_power_on(ov8856);
+	if (ret) {
+		dev_err(&client->dev, "failed to power on\n");
+		return ret;
+	}
+
 	ret = ov8856_identify_module(ov8856);
 	if (ret) {
 		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		return ret;
+		goto probe_power_off;
 	}
 
 	mutex_init(&ov8856->mutex);
@@ -1238,6 +1337,9 @@ static int ov8856_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
 	mutex_destroy(&ov8856->mutex);
 
+probe_power_off:
+	__ov8856_power_off(ov8856);
+
 	return ret;
 }
 
@@ -1254,11 +1356,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
 #endif
 
+static const struct of_device_id ov8856_of_match[] = {
+	{ .compatible = "ovti,ov8856" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov8856_of_match);
+
 static struct i2c_driver ov8856_i2c_driver = {
 	.driver = {
 		.name = "ov8856",
 		.pm = &ov8856_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
+		.of_match_table = ov8856_of_match,
 	},
 	.probe_new = ov8856_probe,
 	.remove = ov8856_remove,
-- 
2.25.1

Re: [PATCH v7 2/3] media: ov8856: Add devicetree support

[Marco Felsch]

Hi Robert,

On 20-05-05 12:01, Robert Foss wrote:
> Add match table, enable ov8856_probe() to support
> both ACPI and DT modes.
> 
> ACPI and DT modes are primarily distinguished from
> by checking for ACPI mode and by having resource like
> be NULL.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v6:
>   * Marco: Bail out of __ov8856_power_on earlier if ACPI mode
> 
> - Changes since v5:
>   * Maxime & Sakari: Replaced clock tolerance check with warning
> 
> - Changes since v4:
>   * Maxime & Sakari: Switch to clock-frequency
> 
> - Changes since v3:
>   * Remove redundant {}-brackets
>   * Compare xvclk_rate to 5% tolerance
>   * Andy: Use dev_fwnode()
>   * Andy: Use %pe instead of %ld + PTR_ERR()
>   * Andy: Invert reset_gpio logic
>   * Andy: Remove dev_dbg() from failing reset_gpio setup
>   * Andy: Use dev_err for logging for failures
>   * Andy: Remove dev_warn from EDEFER/regulator error path
>   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
>   * Maxime & Sakari: Verify clock frequency from DT
>   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
>   * Sakari: Remove duplicate ov8856->dev assignment
> 
> - Changes since v2:
>   * Added "struct device *dev" member to struct ov8856
>   * Andy: Switch to optional version of devm_gpiod_get
>   * Andy: Switch to optional version of devm_clk_get
>   * Fabio: Add reset sleep period
>   * Sakari: Unify defines for 19.2Mhz
>   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
>   * Sakari: Replace dev_info() with dev_dbg()
>   * Sakari: Switch induction variable type to unsigned
>   * Sakari: Don't wait for reset_gpio when in ACPI mode
>   * Sakari: Pull reset GPIO high on power on failure
>   * Sakari: Add power on/off to resume/suspend
>   * Sakari: Fix indentation
>   * Sakari: Power off during ov8856_remove()
>   * Sakari: Don't sleep during power-on in ACPI mode
>   * Sakari: Switch to getting xvclk from clk_get_rate
> 
> - Changes since v1:
>   * Andy & Sakari: Make XVCLK optional since to not break ACPI
>   * Fabio: Change n_shutdown_gpio name to reset_gpio
>   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
>   * Fabio: Remove empty line
>   * Fabio: Remove real error from devm_gpiod_get() failures
>   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
>   * Sakari: Use XVCLK rate as provided by DT
> 
>  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
>  1 file changed, 123 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..e6418a79801e 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -3,10 +3,13 @@
>  
>  #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/module.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>
> @@ -18,7 +21,7 @@
>  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
>  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
>  #define OV8856_SCLK			144000000ULL
> -#define OV8856_MCLK			19200000
> +#define OV8856_XVCLK_19_2		19200000
>  #define OV8856_DATA_LANES		4
>  #define OV8856_RGB_DEPTH		10
>  
> @@ -64,6 +67,12 @@
>  
>  #define to_ov8856(_sd)			container_of(_sd, struct ov8856, sd)
>  
> +static const char * const ov8856_supply_names[] = {
> +	"dovdd",	/* Digital I/O power */
> +	"avdd",		/* Analog power */
> +	"dvdd",		/* Digital core power */
> +};
> +
>  enum {
>  	OV8856_LINK_FREQ_720MBPS,
>  	OV8856_LINK_FREQ_360MBPS,
> @@ -566,6 +575,11 @@ struct ov8856 {
>  	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  
> +	struct device		*dev;

Why do we need this reference here? We already have all information we
need to retrieve the device.

> +	struct clk		*xvclk;
> +	struct gpio_desc	*reset_gpio;
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> +
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl *link_freq;
>  	struct v4l2_ctrl *pixel_rate;
> @@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> +	int ret;
> +
> +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> +		return 0;
> +
> +	ret = clk_prepare_enable(ov8856->xvclk);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable xvclk\n");
> +		return ret;
> +	}
> +
> +	if (ov8856->reset_gpio) {
> +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> +				    ov8856->supplies);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable regulators\n");
> +		goto disable_clk;
> +	}
> +
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> +	usleep_range(1500, 1800);
> +
> +	return 0;
> +
> +disable_clk:
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +	clk_disable_unprepare(ov8856->xvclk);
> +
> +	return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> +			       ov8856->supplies);

In case of ACPI this will still be a unbalanced regulator. Albeit it is a
dummy_regulator it will produce warnings in the ACPI case. Therefore I
said to add the check:

if (is_acpi_node(dev_fwnode(ov8856->dev)))
	return 0;

here to at the begin of this function.


> +	clk_disable_unprepare(ov8856->xvclk);
> +}
> +
>  static int __maybe_unused ov8856_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
>  	if (ov8856->streaming)
>  		ov8856_stop_streaming(ov8856);
>  
> +	__ov8856_power_off(ov8856);
>  	mutex_unlock(&ov8856->mutex);
>  
>  	return 0;
> @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
>  	int ret;
>  
>  	mutex_lock(&ov8856->mutex);
> +
> +	__ov8856_power_on(ov8856);
>  	if (ov8856->streaming) {
>  		ret = ov8856_start_streaming(ov8856);
>  		if (ret) {
> @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
>  	return 0;
>  }
>  
> -static int ov8856_check_hwcfg(struct device *dev)
> +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
>  {
> +	struct device *dev = ov8856->dev;
>  	struct fwnode_handle *ep;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct v4l2_fwnode_endpoint bus_cfg = {
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
> -	u32 mclk;
> +	u32 xvclk_rate;
>  	int ret;
>  	unsigned int i, j;
>  
>  	if (!fwnode)
>  		return -ENXIO;
>  
> -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> +		&xvclk_rate);
>  	if (ret)
>  		return ret;
>  
> -	if (mclk != OV8856_MCLK) {
> -		dev_err(dev, "external clock %d is not supported", mclk);
> -		return -EINVAL;
> +	if (!is_acpi_node(fwnode)) {
> +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> +		if (IS_ERR(ov8856->xvclk)) {
> +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> +					ov8856->xvclk);

I tought I commented this to. Pls align it :)

Regards,
  Marco

> +			return PTR_ERR(ov8856->xvclk);
> +		}
> +
> +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> +		xvclk_rate = clk_get_rate(ov8856->xvclk);
>  	}
>  
> +	if (xvclk_rate != OV8856_XVCLK_19_2)
> +		dev_warn(dev, "external clock rate %d is unsupported", xvclk_rate);
> +
> +	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +		GPIOD_OUT_LOW);
> +	if (IS_ERR(ov8856->reset_gpio))
> +		return PTR_ERR(ov8856->reset_gpio);
> +
> +	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> +		ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> +				      ov8856->supplies);
> +	if (ret)
> +		return ret;
> +
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep)
>  		return -ENXIO;
> @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
>  	pm_runtime_disable(&client->dev);
>  	mutex_destroy(&ov8856->mutex);
>  
> +	__ov8856_power_off(ov8856);
> +
>  	return 0;
>  }
>  
> @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
>  	struct ov8856 *ov8856;
>  	int ret;
>  
> -	ret = ov8856_check_hwcfg(&client->dev);
> +	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> +	if (!ov8856)
> +		return -ENOMEM;
> +
> +	ov8856->dev = &client->dev;
> +
> +	ret = ov8856_get_hwcfg(ov8856);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to check HW configuration: %d",
> +		dev_err(&client->dev, "failed to get HW configuration: %d",
>  			ret);
>  		return ret;
>  	}
>  
> -	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> -	if (!ov8856)
> -		return -ENOMEM;
> -
>  	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> +
> +	ret = __ov8856_power_on(ov8856);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to power on\n");
> +		return ret;
> +	}
> +
>  	ret = ov8856_identify_module(ov8856);
>  	if (ret) {
>  		dev_err(&client->dev, "failed to find sensor: %d", ret);
> -		return ret;
> +		goto probe_power_off;
>  	}
>  
>  	mutex_init(&ov8856->mutex);
> @@ -1238,6 +1337,9 @@ static int ov8856_probe(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
>  	mutex_destroy(&ov8856->mutex);
>  
> +probe_power_off:
> +	__ov8856_power_off(ov8856);
> +
>  	return ret;
>  }
>  
> @@ -1254,11 +1356,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
>  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
>  #endif
>  
> +static const struct of_device_id ov8856_of_match[] = {
> +	{ .compatible = "ovti,ov8856" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +
>  static struct i2c_driver ov8856_i2c_driver = {
>  	.driver = {
>  		.name = "ov8856",
>  		.pm = &ov8856_pm_ops,
>  		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> +		.of_match_table = ov8856_of_match,
>  	},
>  	.probe_new = ov8856_probe,
>  	.remove = ov8856_remove,
> -- 
> 2.25.1
> 
> 

Re: [PATCH v7 2/3] media: ov8856: Add devicetree support

[Robert Foss]

On Tue, 5 May 2020 at 12:16, Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi Robert,
>
> On 20-05-05 12:01, Robert Foss wrote:
> > Add match table, enable ov8856_probe() to support
> > both ACPI and DT modes.
> >
> > ACPI and DT modes are primarily distinguished from
> > by checking for ACPI mode and by having resource like
> > be NULL.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >
> > - Changes since v6:
> >   * Marco: Bail out of __ov8856_power_on earlier if ACPI mode
> >
> > - Changes since v5:
> >   * Maxime & Sakari: Replaced clock tolerance check with warning
> >
> > - Changes since v4:
> >   * Maxime & Sakari: Switch to clock-frequency
> >
> > - Changes since v3:
> >   * Remove redundant {}-brackets
> >   * Compare xvclk_rate to 5% tolerance
> >   * Andy: Use dev_fwnode()
> >   * Andy: Use %pe instead of %ld + PTR_ERR()
> >   * Andy: Invert reset_gpio logic
> >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> >   * Andy: Use dev_err for logging for failures
> >   * Andy: Remove dev_warn from EDEFER/regulator error path
> >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> >   * Maxime & Sakari: Verify clock frequency from DT
> >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> >   * Sakari: Remove duplicate ov8856->dev assignment
> >
> > - Changes since v2:
> >   * Added "struct device *dev" member to struct ov8856
> >   * Andy: Switch to optional version of devm_gpiod_get
> >   * Andy: Switch to optional version of devm_clk_get
> >   * Fabio: Add reset sleep period
> >   * Sakari: Unify defines for 19.2Mhz
> >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> >   * Sakari: Replace dev_info() with dev_dbg()
> >   * Sakari: Switch induction variable type to unsigned
> >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> >   * Sakari: Pull reset GPIO high on power on failure
> >   * Sakari: Add power on/off to resume/suspend
> >   * Sakari: Fix indentation
> >   * Sakari: Power off during ov8856_remove()
> >   * Sakari: Don't sleep during power-on in ACPI mode
> >   * Sakari: Switch to getting xvclk from clk_get_rate
> >
> > - Changes since v1:
> >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> >   * Fabio: Change n_shutdown_gpio name to reset_gpio
> >   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
> >   * Fabio: Remove empty line
> >   * Fabio: Remove real error from devm_gpiod_get() failures
> >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> >   * Sakari: Use XVCLK rate as provided by DT
> >
> >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 123 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index 8655842af275..e6418a79801e 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -3,10 +3,13 @@
> >
> >  #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/module.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>
> > @@ -18,7 +21,7 @@
> >  #define OV8856_LINK_FREQ_360MHZ              360000000ULL
> >  #define OV8856_LINK_FREQ_180MHZ              180000000ULL
> >  #define OV8856_SCLK                  144000000ULL
> > -#define OV8856_MCLK                  19200000
> > +#define OV8856_XVCLK_19_2            19200000
> >  #define OV8856_DATA_LANES            4
> >  #define OV8856_RGB_DEPTH             10
> >
> > @@ -64,6 +67,12 @@
> >
> >  #define to_ov8856(_sd)                       container_of(_sd, struct ov8856, sd)
> >
> > +static const char * const ov8856_supply_names[] = {
> > +     "dovdd",        /* Digital I/O power */
> > +     "avdd",         /* Analog power */
> > +     "dvdd",         /* Digital core power */
> > +};
> > +
> >  enum {
> >       OV8856_LINK_FREQ_720MBPS,
> >       OV8856_LINK_FREQ_360MBPS,
> > @@ -566,6 +575,11 @@ struct ov8856 {
> >       struct media_pad pad;
> >       struct v4l2_ctrl_handler ctrl_handler;
> >
> > +     struct device           *dev;
>
> Why do we need this reference here? We already have all information we
> need to retrieve the device.

Ack, I'll remove it in the next revision.

>
> > +     struct clk              *xvclk;
> > +     struct gpio_desc        *reset_gpio;
> > +     struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> > +
> >       /* V4L2 Controls */
> >       struct v4l2_ctrl *link_freq;
> >       struct v4l2_ctrl *pixel_rate;
> > @@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> >       return ret;
> >  }
> >
> > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > +     int ret;
> > +
> > +     if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > +             return 0;
> > +
> > +     ret = clk_prepare_enable(ov8856->xvclk);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to enable xvclk\n");
> > +             return ret;
> > +     }
> > +
> > +     if (ov8856->reset_gpio) {
> > +             gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +             usleep_range(1000, 2000);
> > +     }
> > +
> > +     ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > +                                 ov8856->supplies);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to enable regulators\n");
> > +             goto disable_clk;
> > +     }
> > +
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> > +     usleep_range(1500, 1800);
> > +
> > +     return 0;
> > +
> > +disable_clk:
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +     clk_disable_unprepare(ov8856->xvclk);
> > +
> > +     return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > +{
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +     regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > +                            ov8856->supplies);
>
> In case of ACPI this will still be a unbalanced regulator. Albeit it is a
> dummy_regulator it will produce warnings in the ACPI case. Therefore I
> said to add the check:
>
> if (is_acpi_node(dev_fwnode(ov8856->dev)))
>         return 0;
>
> here to at the begin of this function.

Ah, I thought the dummy regulators wouldn't generate an error. I'll
add the early escape for ACPI.

>
>
> > +     clk_disable_unprepare(ov8856->xvclk);
> > +}
> > +
> >  static int __maybe_unused ov8856_suspend(struct device *dev)
> >  {
> >       struct i2c_client *client = to_i2c_client(dev);
> > @@ -918,6 +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> >       if (ov8856->streaming)
> >               ov8856_stop_streaming(ov8856);
> >
> > +     __ov8856_power_off(ov8856);
> >       mutex_unlock(&ov8856->mutex);
> >
> >       return 0;
> > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct device *dev)
> >       int ret;
> >
> >       mutex_lock(&ov8856->mutex);
> > +
> > +     __ov8856_power_on(ov8856);
> >       if (ov8856->streaming) {
> >               ret = ov8856_start_streaming(ov8856);
> >               if (ret) {
> > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> >       return 0;
> >  }
> >
> > -static int ov8856_check_hwcfg(struct device *dev)
> > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> >  {
> > +     struct device *dev = ov8856->dev;
> >       struct fwnode_handle *ep;
> >       struct fwnode_handle *fwnode = dev_fwnode(dev);
> >       struct v4l2_fwnode_endpoint bus_cfg = {
> >               .bus_type = V4L2_MBUS_CSI2_DPHY
> >       };
> > -     u32 mclk;
> > +     u32 xvclk_rate;
> >       int ret;
> >       unsigned int i, j;
> >
> >       if (!fwnode)
> >               return -ENXIO;
> >
> > -     ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > +     ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > +             &xvclk_rate);
> >       if (ret)
> >               return ret;
> >
> > -     if (mclk != OV8856_MCLK) {
> > -             dev_err(dev, "external clock %d is not supported", mclk);
> > -             return -EINVAL;
> > +     if (!is_acpi_node(fwnode)) {
> > +             ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > +             if (IS_ERR(ov8856->xvclk)) {
> > +                     dev_err(dev, "could not get xvclk clock (%pe)\n",
> > +                                     ov8856->xvclk);
>
> I tought I commented this to. Pls align it :)

Done!

>
> Regards,
>   Marco
>

RE: [PATCH v7 2/3] media: ov8856: Add devicetree support

[Kao, Ben]

Hi Robert,

On 20-05-05 12:01, Robert Foss wrote:
> Add match table, enable ov8856_probe() to support both ACPI and DT modes.
> 
> ACPI and DT modes are primarily distinguished from by checking for ACPI mode
> and by having resource like be NULL.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v6:
>   * Marco: Bail out of __ov8856_power_on earlier if ACPI mode
> 
> - Changes since v5:
>   * Maxime & Sakari: Replaced clock tolerance check with warning
> 
> - Changes since v4:
>   * Maxime & Sakari: Switch to clock-frequency
> 
> - Changes since v3:
>   * Remove redundant {}-brackets
>   * Compare xvclk_rate to 5% tolerance
>   * Andy: Use dev_fwnode()
>   * Andy: Use %pe instead of %ld + PTR_ERR()
>   * Andy: Invert reset_gpio logic
>   * Andy: Remove dev_dbg() from failing reset_gpio setup
>   * Andy: Use dev_err for logging for failures
>   * Andy: Remove dev_warn from EDEFER/regulator error path
>   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
>   * Maxime & Sakari: Verify clock frequency from DT
>   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
>   * Sakari: Remove duplicate ov8856->dev assignment
> 
> - Changes since v2:
>   * Added "struct device *dev" member to struct ov8856
>   * Andy: Switch to optional version of devm_gpiod_get
>   * Andy: Switch to optional version of devm_clk_get
>   * Fabio: Add reset sleep period
>   * Sakari: Unify defines for 19.2Mhz
>   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
>   * Sakari: Replace dev_info() with dev_dbg()
>   * Sakari: Switch induction variable type to unsigned
>   * Sakari: Don't wait for reset_gpio when in ACPI mode
>   * Sakari: Pull reset GPIO high on power on failure
>   * Sakari: Add power on/off to resume/suspend
>   * Sakari: Fix indentation
>   * Sakari: Power off during ov8856_remove()
>   * Sakari: Don't sleep during power-on in ACPI mode
>   * Sakari: Switch to getting xvclk from clk_get_rate
> 
> - Changes since v1:
>   * Andy & Sakari: Make XVCLK optional since to not break ACPI
>   * Fabio: Change n_shutdown_gpio name to reset_gpio
>   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW
> change
>   * Fabio: Remove empty line
>   * Fabio: Remove real error from devm_gpiod_get() failures
>   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
>   * Sakari: Use XVCLK rate as provided by DT
> 
>  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
>  1 file changed, 123 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index
> 8655842af275..e6418a79801e 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -3,10 +3,13 @@
> 
>  #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/module.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>
> @@ -18,7 +21,7 @@
>  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
>  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
>  #define OV8856_SCLK			144000000ULL
> -#define OV8856_MCLK			19200000
> +#define OV8856_XVCLK_19_2		19200000
>  #define OV8856_DATA_LANES		4
>  #define OV8856_RGB_DEPTH		10
> 
> @@ -64,6 +67,12 @@
> 
>  #define to_ov8856(_sd)			container_of(_sd, struct ov8856, sd)
> 
> +static const char * const ov8856_supply_names[] = {
> +	"dovdd",	/* Digital I/O power */
> +	"avdd",		/* Analog power */
> +	"dvdd",		/* Digital core power */
> +};
> +
>  enum {
>  	OV8856_LINK_FREQ_720MBPS,
>  	OV8856_LINK_FREQ_360MBPS,
> @@ -566,6 +575,11 @@ struct ov8856 {
>  	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
> 
> +	struct device		*dev;
> +	struct clk		*xvclk;
> +	struct gpio_desc	*reset_gpio;
> +	struct regulator_bulk_data
> supplies[ARRAY_SIZE(ov8856_supply_names)];
> +
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl *link_freq;
>  	struct v4l2_ctrl *pixel_rate;
> @@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev *sd,
> int enable)
>  	return ret;
>  }
> 
> +static int __ov8856_power_on(struct ov8856 *ov8856) {
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> +	int ret;
> +
> +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> +		return 0;
> +
> +	ret = clk_prepare_enable(ov8856->xvclk);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable xvclk\n");
> +		return ret;
> +	}
> +
> +	if (ov8856->reset_gpio) {
> +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> +				    ov8856->supplies);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable regulators\n");
> +		goto disable_clk;
> +	}
> +
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> +	usleep_range(1500, 1800);
> +
> +	return 0;
> +
> +disable_clk:
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +	clk_disable_unprepare(ov8856->xvclk);
> +
> +	return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856) {
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> +			       ov8856->supplies);
> +	clk_disable_unprepare(ov8856->xvclk);
> +}
> +
>  static int __maybe_unused ov8856_suspend(struct device *dev)  {
>  	struct i2c_client *client = to_i2c_client(dev); @@ -918,6 +978,7 @@
> static int __maybe_unused ov8856_suspend(struct device *dev)
>  	if (ov8856->streaming)
>  		ov8856_stop_streaming(ov8856);
> 
> +	__ov8856_power_off(ov8856);
>  	mutex_unlock(&ov8856->mutex);
> 
>  	return 0;
> @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct
> device *dev)
>  	int ret;
> 
>  	mutex_lock(&ov8856->mutex);
> +
> +	__ov8856_power_on(ov8856);
>  	if (ov8856->streaming) {
>  		ret = ov8856_start_streaming(ov8856);
>  		if (ret) {
> @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856
> *ov8856)
>  	return 0;
>  }
> 
> -static int ov8856_check_hwcfg(struct device *dev)
> +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
>  {
> +	struct device *dev = ov8856->dev;
>  	struct fwnode_handle *ep;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct v4l2_fwnode_endpoint bus_cfg = {
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
> -	u32 mclk;
> +	u32 xvclk_rate;
>  	int ret;
>  	unsigned int i, j;
> 
>  	if (!fwnode)
>  		return -ENXIO;
> 
> -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> +		&xvclk_rate);
>  	if (ret)
>  		return ret;
> 
> -	if (mclk != OV8856_MCLK) {
> -		dev_err(dev, "external clock %d is not supported", mclk);
> -		return -EINVAL;
> +	if (!is_acpi_node(fwnode)) {
> +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> +		if (IS_ERR(ov8856->xvclk)) {
> +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> +					ov8856->xvclk);
> +			return PTR_ERR(ov8856->xvclk);
> +		}
> +
> +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> +		xvclk_rate = clk_get_rate(ov8856->xvclk);
>  	}
> 
> +	if (xvclk_rate != OV8856_XVCLK_19_2)
> +		dev_warn(dev, "external clock rate %d is unsupported",
> xvclk_rate);
> +
> +	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +		GPIOD_OUT_LOW);
> +	if (IS_ERR(ov8856->reset_gpio))
> +		return PTR_ERR(ov8856->reset_gpio);
> +
> +	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> +		ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev,
> ARRAY_SIZE(ov8856_supply_names),
> +				      ov8856->supplies);
> +	if (ret)
> +		return ret;
> +

In case of ACPI this cannot get ov8856->reset_gpio  and ov8856->supplies, please add the check for ACPI case:
if (!is_acpi_node(fwnode)) { }

Thanks.
Ben

>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep)
>  		return -ENXIO;
> @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
>  	pm_runtime_disable(&client->dev);
>  	mutex_destroy(&ov8856->mutex);
> 
> +	__ov8856_power_off(ov8856);
> +
>  	return 0;
>  }
> 
> @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
>  	struct ov8856 *ov8856;
>  	int ret;
> 
> -	ret = ov8856_check_hwcfg(&client->dev);
> +	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> +	if (!ov8856)
> +		return -ENOMEM;
> +
> +	ov8856->dev = &client->dev;
> +
> +	ret = ov8856_get_hwcfg(ov8856);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to check HW configuration: %d",
> +		dev_err(&client->dev, "failed to get HW configuration: %d",
>  			ret);
>  		return ret;
>  	}
> 
> -	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> -	if (!ov8856)
> -		return -ENOMEM;
> -
>  	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> +
> +	ret = __ov8856_power_on(ov8856);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to power on\n");
> +		return ret;
> +	}
> +
>  	ret = ov8856_identify_module(ov8856);
>  	if (ret) {
>  		dev_err(&client->dev, "failed to find sensor: %d", ret);
> -		return ret;
> +		goto probe_power_off;
>  	}
> 
>  	mutex_init(&ov8856->mutex);
> @@ -1238,6 +1337,9 @@ static int ov8856_probe(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
>  	mutex_destroy(&ov8856->mutex);
> 
> +probe_power_off:
> +	__ov8856_power_off(ov8856);
> +
>  	return ret;
>  }
> 
> @@ -1254,11 +1356,18 @@ static const struct acpi_device_id ov8856_acpi_ids[]
> = {  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);  #endif
> 
> +static const struct of_device_id ov8856_of_match[] = {
> +	{ .compatible = "ovti,ov8856" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +
>  static struct i2c_driver ov8856_i2c_driver = {
>  	.driver = {
>  		.name = "ov8856",
>  		.pm = &ov8856_pm_ops,
>  		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> +		.of_match_table = ov8856_of_match,
>  	},
>  	.probe_new = ov8856_probe,
>  	.remove = ov8856_remove,
> --
> 2.25.1

Re: [PATCH v7 2/3] media: ov8856: Add devicetree support

[Marco Felsch]

Hi Ben,

On 20-05-07 08:06, Kao, Ben wrote:
> Hi Robert,
> 
> On 20-05-05 12:01, Robert Foss wrote:
> > Add match table, enable ov8856_probe() to support both ACPI and DT modes.
> > 
> > ACPI and DT modes are primarily distinguished from by checking for ACPI mode
> > and by having resource like be NULL.
> > 
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> > 
> > - Changes since v6:
> >   * Marco: Bail out of __ov8856_power_on earlier if ACPI mode
> > 
> > - Changes since v5:
> >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > 
> > - Changes since v4:
> >   * Maxime & Sakari: Switch to clock-frequency
> > 
> > - Changes since v3:
> >   * Remove redundant {}-brackets
> >   * Compare xvclk_rate to 5% tolerance
> >   * Andy: Use dev_fwnode()
> >   * Andy: Use %pe instead of %ld + PTR_ERR()
> >   * Andy: Invert reset_gpio logic
> >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> >   * Andy: Use dev_err for logging for failures
> >   * Andy: Remove dev_warn from EDEFER/regulator error path
> >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> >   * Maxime & Sakari: Verify clock frequency from DT
> >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> >   * Sakari: Remove duplicate ov8856->dev assignment
> > 
> > - Changes since v2:
> >   * Added "struct device *dev" member to struct ov8856
> >   * Andy: Switch to optional version of devm_gpiod_get
> >   * Andy: Switch to optional version of devm_clk_get
> >   * Fabio: Add reset sleep period
> >   * Sakari: Unify defines for 19.2Mhz
> >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> >   * Sakari: Replace dev_info() with dev_dbg()
> >   * Sakari: Switch induction variable type to unsigned
> >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> >   * Sakari: Pull reset GPIO high on power on failure
> >   * Sakari: Add power on/off to resume/suspend
> >   * Sakari: Fix indentation
> >   * Sakari: Power off during ov8856_remove()
> >   * Sakari: Don't sleep during power-on in ACPI mode
> >   * Sakari: Switch to getting xvclk from clk_get_rate
> > 
> > - Changes since v1:
> >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> >   * Fabio: Change n_shutdown_gpio name to reset_gpio
> >   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW
> > change
> >   * Fabio: Remove empty line
> >   * Fabio: Remove real error from devm_gpiod_get() failures
> >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> >   * Sakari: Use XVCLK rate as provided by DT
> > 
> >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 123 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index
> > 8655842af275..e6418a79801e 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -3,10 +3,13 @@
> > 
> >  #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/module.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>
> > @@ -18,7 +21,7 @@
> >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> >  #define OV8856_SCLK			144000000ULL
> > -#define OV8856_MCLK			19200000
> > +#define OV8856_XVCLK_19_2		19200000
> >  #define OV8856_DATA_LANES		4
> >  #define OV8856_RGB_DEPTH		10
> > 
> > @@ -64,6 +67,12 @@
> > 
> >  #define to_ov8856(_sd)			container_of(_sd, struct ov8856, sd)
> > 
> > +static const char * const ov8856_supply_names[] = {
> > +	"dovdd",	/* Digital I/O power */
> > +	"avdd",		/* Analog power */
> > +	"dvdd",		/* Digital core power */
> > +};
> > +
> >  enum {
> >  	OV8856_LINK_FREQ_720MBPS,
> >  	OV8856_LINK_FREQ_360MBPS,
> > @@ -566,6 +575,11 @@ struct ov8856 {
> >  	struct media_pad pad;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> > 
> > +	struct device		*dev;
> > +	struct clk		*xvclk;
> > +	struct gpio_desc	*reset_gpio;
> > +	struct regulator_bulk_data
> > supplies[ARRAY_SIZE(ov8856_supply_names)];
> > +
> >  	/* V4L2 Controls */
> >  	struct v4l2_ctrl *link_freq;
> >  	struct v4l2_ctrl *pixel_rate;
> > @@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev *sd,
> > int enable)
> >  	return ret;
> >  }
> > 
> > +static int __ov8856_power_on(struct ov8856 *ov8856) {
> > +	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > +	int ret;
> > +
> > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > +		return 0;
> > +
> > +	ret = clk_prepare_enable(ov8856->xvclk);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > +		return ret;
> > +	}
> > +
> > +	if (ov8856->reset_gpio) {
> > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +		usleep_range(1000, 2000);
> > +	}
> > +
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > +				    ov8856->supplies);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to enable regulators\n");
> > +		goto disable_clk;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> > +	usleep_range(1500, 1800);
> > +
> > +	return 0;
> > +
> > +disable_clk:
> > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +	clk_disable_unprepare(ov8856->xvclk);
> > +
> > +	return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856) {
> > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > +			       ov8856->supplies);
> > +	clk_disable_unprepare(ov8856->xvclk);
> > +}
> > +
> >  static int __maybe_unused ov8856_suspend(struct device *dev)  {
> >  	struct i2c_client *client = to_i2c_client(dev); @@ -918,6 +978,7 @@
> > static int __maybe_unused ov8856_suspend(struct device *dev)
> >  	if (ov8856->streaming)
> >  		ov8856_stop_streaming(ov8856);
> > 
> > +	__ov8856_power_off(ov8856);
> >  	mutex_unlock(&ov8856->mutex);
> > 
> >  	return 0;
> > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct
> > device *dev)
> >  	int ret;
> > 
> >  	mutex_lock(&ov8856->mutex);
> > +
> > +	__ov8856_power_on(ov8856);
> >  	if (ov8856->streaming) {
> >  		ret = ov8856_start_streaming(ov8856);
> >  		if (ret) {
> > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856
> > *ov8856)
> >  	return 0;
> >  }
> > 
> > -static int ov8856_check_hwcfg(struct device *dev)
> > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> >  {
> > +	struct device *dev = ov8856->dev;
> >  	struct fwnode_handle *ep;
> >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> >  	struct v4l2_fwnode_endpoint bus_cfg = {
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> >  	};
> > -	u32 mclk;
> > +	u32 xvclk_rate;
> >  	int ret;
> >  	unsigned int i, j;
> > 
> >  	if (!fwnode)
> >  		return -ENXIO;
> > 
> > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > +		&xvclk_rate);
> >  	if (ret)
> >  		return ret;
> > 
> > -	if (mclk != OV8856_MCLK) {
> > -		dev_err(dev, "external clock %d is not supported", mclk);
> > -		return -EINVAL;
> > +	if (!is_acpi_node(fwnode)) {
> > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > +		if (IS_ERR(ov8856->xvclk)) {
> > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > +					ov8856->xvclk);
> > +			return PTR_ERR(ov8856->xvclk);
> > +		}
> > +
> > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> >  	}
> > 
> > +	if (xvclk_rate != OV8856_XVCLK_19_2)
> > +		dev_warn(dev, "external clock rate %d is unsupported",
> > xvclk_rate);
> > +
> > +	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +		GPIOD_OUT_LOW);
> > +	if (IS_ERR(ov8856->reset_gpio))
> > +		return PTR_ERR(ov8856->reset_gpio);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> > +		ov8856->supplies[i].supply = ov8856_supply_names[i];
> > +
> > +	ret = devm_regulator_bulk_get(dev,
> > ARRAY_SIZE(ov8856_supply_names),
> > +				      ov8856->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> 
> In case of ACPI this cannot get ov8856->reset_gpio  and ov8856->supplies, please add the check for ACPI case:
> if (!is_acpi_node(fwnode)) { }

I tought that dummy-regulators will be created in case of ACPI?

Regards,
  Marco

> Thanks.
> Ben
> 
> >  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> >  	if (!ep)
> >  		return -ENXIO;
> > @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
> >  	pm_runtime_disable(&client->dev);
> >  	mutex_destroy(&ov8856->mutex);
> > 
> > +	__ov8856_power_off(ov8856);
> > +
> >  	return 0;
> >  }
> > 
> > @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
> >  	struct ov8856 *ov8856;
> >  	int ret;
> > 
> > -	ret = ov8856_check_hwcfg(&client->dev);
> > +	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > +	if (!ov8856)
> > +		return -ENOMEM;
> > +
> > +	ov8856->dev = &client->dev;
> > +
> > +	ret = ov8856_get_hwcfg(ov8856);
> >  	if (ret) {
> > -		dev_err(&client->dev, "failed to check HW configuration: %d",
> > +		dev_err(&client->dev, "failed to get HW configuration: %d",
> >  			ret);
> >  		return ret;
> >  	}
> > 
> > -	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > -	if (!ov8856)
> > -		return -ENOMEM;
> > -
> >  	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > +
> > +	ret = __ov8856_power_on(ov8856);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to power on\n");
> > +		return ret;
> > +	}
> > +
> >  	ret = ov8856_identify_module(ov8856);
> >  	if (ret) {
> >  		dev_err(&client->dev, "failed to find sensor: %d", ret);
> > -		return ret;
> > +		goto probe_power_off;
> >  	}
> > 
> >  	mutex_init(&ov8856->mutex);
> > @@ -1238,6 +1337,9 @@ static int ov8856_probe(struct i2c_client *client)
> >  	v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> >  	mutex_destroy(&ov8856->mutex);
> > 
> > +probe_power_off:
> > +	__ov8856_power_off(ov8856);
> > +
> >  	return ret;
> >  }
> > 
> > @@ -1254,11 +1356,18 @@ static const struct acpi_device_id ov8856_acpi_ids[]
> > = {  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);  #endif
> > 
> > +static const struct of_device_id ov8856_of_match[] = {
> > +	{ .compatible = "ovti,ov8856" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > +
> >  static struct i2c_driver ov8856_i2c_driver = {
> >  	.driver = {
> >  		.name = "ov8856",
> >  		.pm = &ov8856_pm_ops,
> >  		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > +		.of_match_table = ov8856_of_match,
> >  	},
> >  	.probe_new = ov8856_probe,
> >  	.remove = ov8856_remove,
> > --
> > 2.25.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v7 2/3] media: ov8856: Add devicetree support

[Kao, Ben]

Hi Marco Felsch,

> 20-05-08 04:22, Marco Felsch wrote:  
> Hi Ben,
> 
> On 20-05-07 08:06, Kao, Ben wrote:
> > Hi Robert,
> >
> > On 20-05-05 12:01, Robert Foss wrote:
> > > Add match table, enable ov8856_probe() to support both ACPI and DT modes.
> > >
> > > ACPI and DT modes are primarily distinguished from by checking for
> > > ACPI mode and by having resource like be NULL.
> > >
> > > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > > ---
> > >
> > > - Changes since v6:
> > >   * Marco: Bail out of __ov8856_power_on earlier if ACPI mode
> > >
> > > - Changes since v5:
> > >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > >
> > > - Changes since v4:
> > >   * Maxime & Sakari: Switch to clock-frequency
> > >
> > > - Changes since v3:
> > >   * Remove redundant {}-brackets
> > >   * Compare xvclk_rate to 5% tolerance
> > >   * Andy: Use dev_fwnode()
> > >   * Andy: Use %pe instead of %ld + PTR_ERR()
> > >   * Andy: Invert reset_gpio logic
> > >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> > >   * Andy: Use dev_err for logging for failures
> > >   * Andy: Remove dev_warn from EDEFER/regulator error path
> > >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> > >   * Maxime & Sakari: Verify clock frequency from DT
> > >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> > >   * Sakari: Remove duplicate ov8856->dev assignment
> > >
> > > - Changes since v2:
> > >   * Added "struct device *dev" member to struct ov8856
> > >   * Andy: Switch to optional version of devm_gpiod_get
> > >   * Andy: Switch to optional version of devm_clk_get
> > >   * Fabio: Add reset sleep period
> > >   * Sakari: Unify defines for 19.2Mhz
> > >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> > >   * Sakari: Replace dev_info() with dev_dbg()
> > >   * Sakari: Switch induction variable type to unsigned
> > >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> > >   * Sakari: Pull reset GPIO high on power on failure
> > >   * Sakari: Add power on/off to resume/suspend
> > >   * Sakari: Fix indentation
> > >   * Sakari: Power off during ov8856_remove()
> > >   * Sakari: Don't sleep during power-on in ACPI mode
> > >   * Sakari: Switch to getting xvclk from clk_get_rate
> > >
> > > - Changes since v1:
> > >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > >   * Fabio: Change n_shutdown_gpio name to reset_gpio
> > >   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH ->
> > > GPIO_ACTIVE_LOW change
> > >   * Fabio: Remove empty line
> > >   * Fabio: Remove real error from devm_gpiod_get() failures
> > >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > >   * Sakari: Use XVCLK rate as provided by DT
> > >
> > >  drivers/media/i2c/ov8856.c | 137
> > > +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 123 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > index 8655842af275..e6418a79801e 100644
> > > --- a/drivers/media/i2c/ov8856.c
> > > +++ b/drivers/media/i2c/ov8856.c
> > > @@ -3,10 +3,13 @@
> > >
> > >  #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/module.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>
> > > @@ -18,7 +21,7 @@
> > >  #define OV8856_LINK_FREQ_360MHZ		360000000ULL
> > >  #define OV8856_LINK_FREQ_180MHZ		180000000ULL
> > >  #define OV8856_SCLK			144000000ULL
> > > -#define OV8856_MCLK			19200000
> > > +#define OV8856_XVCLK_19_2		19200000
> > >  #define OV8856_DATA_LANES		4
> > >  #define OV8856_RGB_DEPTH		10
> > >
> > > @@ -64,6 +67,12 @@
> > >
> > >  #define to_ov8856(_sd)			container_of(_sd, struct
> ov8856, sd)
> > >
> > > +static const char * const ov8856_supply_names[] = {
> > > +	"dovdd",	/* Digital I/O power */
> > > +	"avdd",		/* Analog power */
> > > +	"dvdd",		/* Digital core power */
> > > +};
> > > +
> > >  enum {
> > >  	OV8856_LINK_FREQ_720MBPS,
> > >  	OV8856_LINK_FREQ_360MBPS,
> > > @@ -566,6 +575,11 @@ struct ov8856 {
> > >  	struct media_pad pad;
> > >  	struct v4l2_ctrl_handler ctrl_handler;
> > >
> > > +	struct device		*dev;
> > > +	struct clk		*xvclk;
> > > +	struct gpio_desc	*reset_gpio;
> > > +	struct regulator_bulk_data
> > > supplies[ARRAY_SIZE(ov8856_supply_names)];
> > > +
> > >  	/* V4L2 Controls */
> > >  	struct v4l2_ctrl *link_freq;
> > >  	struct v4l2_ctrl *pixel_rate;
> > > @@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev
> > > *sd, int enable)
> > >  	return ret;
> > >  }
> > >
> > > +static int __ov8856_power_on(struct ov8856 *ov8856) {
> > > +	struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > +	int ret;
> > > +
> > > +	if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > > +		return 0;
> > > +
> > > +	ret = clk_prepare_enable(ov8856->xvclk);
> > > +	if (ret < 0) {
> > > +		dev_err(&client->dev, "failed to enable xvclk\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (ov8856->reset_gpio) {
> > > +		gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +		usleep_range(1000, 2000);
> > > +	}
> > > +
> > > +	ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > > +				    ov8856->supplies);
> > > +	if (ret < 0) {
> > > +		dev_err(&client->dev, "failed to enable regulators\n");
> > > +		goto disable_clk;
> > > +	}
> > > +
> > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> > > +	usleep_range(1500, 1800);
> > > +
> > > +	return 0;
> > > +
> > > +disable_clk:
> > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +	clk_disable_unprepare(ov8856->xvclk);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void __ov8856_power_off(struct ov8856 *ov8856) {
> > > +	gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +	regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > > +			       ov8856->supplies);
> > > +	clk_disable_unprepare(ov8856->xvclk);
> > > +}
> > > +
> > >  static int __maybe_unused ov8856_suspend(struct device *dev)  {
> > >  	struct i2c_client *client = to_i2c_client(dev); @@ -918,6 +978,7
> > > @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> > >  	if (ov8856->streaming)
> > >  		ov8856_stop_streaming(ov8856);
> > >
> > > +	__ov8856_power_off(ov8856);
> > >  	mutex_unlock(&ov8856->mutex);
> > >
> > >  	return 0;
> > > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct
> > > device *dev)
> > >  	int ret;
> > >
> > >  	mutex_lock(&ov8856->mutex);
> > > +
> > > +	__ov8856_power_on(ov8856);
> > >  	if (ov8856->streaming) {
> > >  		ret = ov8856_start_streaming(ov8856);
> > >  		if (ret) {
> > > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct
> > > ov8856
> > > *ov8856)
> > >  	return 0;
> > >  }
> > >
> > > -static int ov8856_check_hwcfg(struct device *dev)
> > > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> > >  {
> > > +	struct device *dev = ov8856->dev;
> > >  	struct fwnode_handle *ep;
> > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > >  	struct v4l2_fwnode_endpoint bus_cfg = {
> > >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> > >  	};
> > > -	u32 mclk;
> > > +	u32 xvclk_rate;
> > >  	int ret;
> > >  	unsigned int i, j;
> > >
> > >  	if (!fwnode)
> > >  		return -ENXIO;
> > >
> > > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > > +		&xvclk_rate);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > -	if (mclk != OV8856_MCLK) {
> > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > -		return -EINVAL;
> > > +	if (!is_acpi_node(fwnode)) {
> > > +		ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > +		if (IS_ERR(ov8856->xvclk)) {
> > > +			dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > +					ov8856->xvclk);
> > > +			return PTR_ERR(ov8856->xvclk);
> > > +		}
> > > +
> > > +		clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> > >  	}
> > >
> > > +	if (xvclk_rate != OV8856_XVCLK_19_2)
> > > +		dev_warn(dev, "external clock rate %d is unsupported",
> > > xvclk_rate);
> > > +
> > > +	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > +		GPIOD_OUT_LOW);
> > > +	if (IS_ERR(ov8856->reset_gpio))
> > > +		return PTR_ERR(ov8856->reset_gpio);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> > > +		ov8856->supplies[i].supply = ov8856_supply_names[i];
> > > +
> > > +	ret = devm_regulator_bulk_get(dev,
> > > ARRAY_SIZE(ov8856_supply_names),
> > > +				      ov8856->supplies);
> > > +	if (ret)
> > > +		return ret;
> > > +
> >
> > In case of ACPI this cannot get ov8856->reset_gpio  and ov8856->supplies,
> please add the check for ACPI case:
> > if (!is_acpi_node(fwnode)) { }
> 
> I tought that dummy-regulators will be created in case of ACPI?
You are right, the check is unnecessary.

Ben
> 
> Regards,
>   Marco
> 
> > Thanks.
> > Ben
> >
> > >  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > >  	if (!ep)
> > >  		return -ENXIO;
> > > @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
> > >  	pm_runtime_disable(&client->dev);
> > >  	mutex_destroy(&ov8856->mutex);
> > >
> > > +	__ov8856_power_off(ov8856);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
> > >  	struct ov8856 *ov8856;
> > >  	int ret;
> > >
> > > -	ret = ov8856_check_hwcfg(&client->dev);
> > > +	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > > +	if (!ov8856)
> > > +		return -ENOMEM;
> > > +
> > > +	ov8856->dev = &client->dev;
> > > +
> > > +	ret = ov8856_get_hwcfg(ov8856);
> > >  	if (ret) {
> > > -		dev_err(&client->dev, "failed to check HW configuration: %d",
> > > +		dev_err(&client->dev, "failed to get HW configuration: %d",
> > >  			ret);
> > >  		return ret;
> > >  	}
> > >
> > > -	ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > > -	if (!ov8856)
> > > -		return -ENOMEM;
> > > -
> > >  	v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > > +
> > > +	ret = __ov8856_power_on(ov8856);
> > > +	if (ret) {
> > > +		dev_err(&client->dev, "failed to power on\n");
> > > +		return ret;
> > > +	}
> > > +
> > >  	ret = ov8856_identify_module(ov8856);
> > >  	if (ret) {
> > >  		dev_err(&client->dev, "failed to find sensor: %d", ret);
> > > -		return ret;
> > > +		goto probe_power_off;
> > >  	}
> > >
> > >  	mutex_init(&ov8856->mutex);
> > > @@ -1238,6 +1337,9 @@ static int ov8856_probe(struct i2c_client *client)
> > >  	v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> > >  	mutex_destroy(&ov8856->mutex);
> > >
> > > +probe_power_off:
> > > +	__ov8856_power_off(ov8856);
> > > +
> > >  	return ret;
> > >  }
> > >
> > > @@ -1254,11 +1356,18 @@ static const struct acpi_device_id
> > > ov8856_acpi_ids[] = {  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> > > #endif
> > >
> > > +static const struct of_device_id ov8856_of_match[] = {
> > > +	{ .compatible = "ovti,ov8856" },
> > > +	{ /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > > +
> > >  static struct i2c_driver ov8856_i2c_driver = {
> > >  	.driver = {
> > >  		.name = "ov8856",
> > >  		.pm = &ov8856_pm_ops,
> > >  		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > > +		.of_match_table = ov8856_of_match,
> > >  	},
> > >  	.probe_new = ov8856_probe,
> > >  	.remove = ov8856_remove,
> > > --
> > > 2.25.1
> >
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v7 2/3] media: ov8856: Add devicetree support

[Robert Foss]

On Thu, 7 May 2020 at 10:06, Kao, Ben <ben.kao@intel.com> wrote:
>
> Hi Robert,
>
> On 20-05-05 12:01, Robert Foss wrote:
> > Add match table, enable ov8856_probe() to support both ACPI and DT modes.
> >
> > ACPI and DT modes are primarily distinguished from by checking for ACPI mode
> > and by having resource like be NULL.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >
> > - Changes since v6:
> >   * Marco: Bail out of __ov8856_power_on earlier if ACPI mode
> >
> > - Changes since v5:
> >   * Maxime & Sakari: Replaced clock tolerance check with warning
> >
> > - Changes since v4:
> >   * Maxime & Sakari: Switch to clock-frequency
> >
> > - Changes since v3:
> >   * Remove redundant {}-brackets
> >   * Compare xvclk_rate to 5% tolerance
> >   * Andy: Use dev_fwnode()
> >   * Andy: Use %pe instead of %ld + PTR_ERR()
> >   * Andy: Invert reset_gpio logic
> >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> >   * Andy: Use dev_err for logging for failures
> >   * Andy: Remove dev_warn from EDEFER/regulator error path
> >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> >   * Maxime & Sakari: Verify clock frequency from DT
> >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> >   * Sakari: Remove duplicate ov8856->dev assignment
> >
> > - Changes since v2:
> >   * Added "struct device *dev" member to struct ov8856
> >   * Andy: Switch to optional version of devm_gpiod_get
> >   * Andy: Switch to optional version of devm_clk_get
> >   * Fabio: Add reset sleep period
> >   * Sakari: Unify defines for 19.2Mhz
> >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> >   * Sakari: Replace dev_info() with dev_dbg()
> >   * Sakari: Switch induction variable type to unsigned
> >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> >   * Sakari: Pull reset GPIO high on power on failure
> >   * Sakari: Add power on/off to resume/suspend
> >   * Sakari: Fix indentation
> >   * Sakari: Power off during ov8856_remove()
> >   * Sakari: Don't sleep during power-on in ACPI mode
> >   * Sakari: Switch to getting xvclk from clk_get_rate
> >
> > - Changes since v1:
> >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> >   * Fabio: Change n_shutdown_gpio name to reset_gpio
> >   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW
> > change
> >   * Fabio: Remove empty line
> >   * Fabio: Remove real error from devm_gpiod_get() failures
> >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> >   * Sakari: Use XVCLK rate as provided by DT
> >
> >  drivers/media/i2c/ov8856.c | 137 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 123 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c index
> > 8655842af275..e6418a79801e 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -3,10 +3,13 @@
> >
> >  #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/module.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>
> > @@ -18,7 +21,7 @@
> >  #define OV8856_LINK_FREQ_360MHZ              360000000ULL
> >  #define OV8856_LINK_FREQ_180MHZ              180000000ULL
> >  #define OV8856_SCLK                  144000000ULL
> > -#define OV8856_MCLK                  19200000
> > +#define OV8856_XVCLK_19_2            19200000
> >  #define OV8856_DATA_LANES            4
> >  #define OV8856_RGB_DEPTH             10
> >
> > @@ -64,6 +67,12 @@
> >
> >  #define to_ov8856(_sd)                       container_of(_sd, struct ov8856, sd)
> >
> > +static const char * const ov8856_supply_names[] = {
> > +     "dovdd",        /* Digital I/O power */
> > +     "avdd",         /* Analog power */
> > +     "dvdd",         /* Digital core power */
> > +};
> > +
> >  enum {
> >       OV8856_LINK_FREQ_720MBPS,
> >       OV8856_LINK_FREQ_360MBPS,
> > @@ -566,6 +575,11 @@ struct ov8856 {
> >       struct media_pad pad;
> >       struct v4l2_ctrl_handler ctrl_handler;
> >
> > +     struct device           *dev;
> > +     struct clk              *xvclk;
> > +     struct gpio_desc        *reset_gpio;
> > +     struct regulator_bulk_data
> > supplies[ARRAY_SIZE(ov8856_supply_names)];
> > +
> >       /* V4L2 Controls */
> >       struct v4l2_ctrl *link_freq;
> >       struct v4l2_ctrl *pixel_rate;
> > @@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev *sd,
> > int enable)
> >       return ret;
> >  }
> >
> > +static int __ov8856_power_on(struct ov8856 *ov8856) {
> > +     struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > +     int ret;
> > +
> > +     if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > +             return 0;
> > +
> > +     ret = clk_prepare_enable(ov8856->xvclk);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to enable xvclk\n");
> > +             return ret;
> > +     }
> > +
> > +     if (ov8856->reset_gpio) {
> > +             gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +             usleep_range(1000, 2000);
> > +     }
> > +
> > +     ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > +                                 ov8856->supplies);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "failed to enable regulators\n");
> > +             goto disable_clk;
> > +     }
> > +
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> > +     usleep_range(1500, 1800);
> > +
> > +     return 0;
> > +
> > +disable_clk:
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +     clk_disable_unprepare(ov8856->xvclk);
> > +
> > +     return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856) {
> > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > +     regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > +                            ov8856->supplies);
> > +     clk_disable_unprepare(ov8856->xvclk);
> > +}
> > +
> >  static int __maybe_unused ov8856_suspend(struct device *dev)  {
> >       struct i2c_client *client = to_i2c_client(dev); @@ -918,6 +978,7 @@
> > static int __maybe_unused ov8856_suspend(struct device *dev)
> >       if (ov8856->streaming)
> >               ov8856_stop_streaming(ov8856);
> >
> > +     __ov8856_power_off(ov8856);
> >       mutex_unlock(&ov8856->mutex);
> >
> >       return 0;
> > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct
> > device *dev)
> >       int ret;
> >
> >       mutex_lock(&ov8856->mutex);
> > +
> > +     __ov8856_power_on(ov8856);
> >       if (ov8856->streaming) {
> >               ret = ov8856_start_streaming(ov8856);
> >               if (ret) {
> > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct ov8856
> > *ov8856)
> >       return 0;
> >  }
> >
> > -static int ov8856_check_hwcfg(struct device *dev)
> > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> >  {
> > +     struct device *dev = ov8856->dev;
> >       struct fwnode_handle *ep;
> >       struct fwnode_handle *fwnode = dev_fwnode(dev);
> >       struct v4l2_fwnode_endpoint bus_cfg = {
> >               .bus_type = V4L2_MBUS_CSI2_DPHY
> >       };
> > -     u32 mclk;
> > +     u32 xvclk_rate;
> >       int ret;
> >       unsigned int i, j;
> >
> >       if (!fwnode)
> >               return -ENXIO;
> >
> > -     ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > +     ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > +             &xvclk_rate);
> >       if (ret)
> >               return ret;
> >
> > -     if (mclk != OV8856_MCLK) {
> > -             dev_err(dev, "external clock %d is not supported", mclk);
> > -             return -EINVAL;
> > +     if (!is_acpi_node(fwnode)) {
> > +             ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > +             if (IS_ERR(ov8856->xvclk)) {
> > +                     dev_err(dev, "could not get xvclk clock (%pe)\n",
> > +                                     ov8856->xvclk);
> > +                     return PTR_ERR(ov8856->xvclk);
> > +             }
> > +
> > +             clk_set_rate(ov8856->xvclk, xvclk_rate);
> > +             xvclk_rate = clk_get_rate(ov8856->xvclk);
> >       }
> >
> > +     if (xvclk_rate != OV8856_XVCLK_19_2)
> > +             dev_warn(dev, "external clock rate %d is unsupported",
> > xvclk_rate);
> > +
> > +     ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +             GPIOD_OUT_LOW);
> > +     if (IS_ERR(ov8856->reset_gpio))
> > +             return PTR_ERR(ov8856->reset_gpio);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> > +             ov8856->supplies[i].supply = ov8856_supply_names[i];
> > +
> > +     ret = devm_regulator_bulk_get(dev,
> > ARRAY_SIZE(ov8856_supply_names),
> > +                                   ov8856->supplies);
> > +     if (ret)
> > +             return ret;
> > +
>
> In case of ACPI this cannot get ov8856->reset_gpio  and ov8856->supplies, please add the check for ACPI case:
> if (!is_acpi_node(fwnode)) { }

Do you prefer the explicit check for clarity reasons or for functional reasons?

As far as I understand it, reset_gpio will be NULL and supplies will
be populated with dummy supplies in the ACPI case.
Both can be treated as if they were initialized fully during later
calls to their respective APIs.

>
> Thanks.
> Ben
>
> >       ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> >       if (!ep)
> >               return -ENXIO;
> > @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
> >       pm_runtime_disable(&client->dev);
> >       mutex_destroy(&ov8856->mutex);
> >
> > +     __ov8856_power_off(ov8856);
> > +
> >       return 0;
> >  }
> >
> > @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
> >       struct ov8856 *ov8856;
> >       int ret;
> >
> > -     ret = ov8856_check_hwcfg(&client->dev);
> > +     ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > +     if (!ov8856)
> > +             return -ENOMEM;
> > +
> > +     ov8856->dev = &client->dev;
> > +
> > +     ret = ov8856_get_hwcfg(ov8856);
> >       if (ret) {
> > -             dev_err(&client->dev, "failed to check HW configuration: %d",
> > +             dev_err(&client->dev, "failed to get HW configuration: %d",
> >                       ret);
> >               return ret;
> >       }
> >
> > -     ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > -     if (!ov8856)
> > -             return -ENOMEM;
> > -
> >       v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > +
> > +     ret = __ov8856_power_on(ov8856);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to power on\n");
> > +             return ret;
> > +     }
> > +
> >       ret = ov8856_identify_module(ov8856);
> >       if (ret) {
> >               dev_err(&client->dev, "failed to find sensor: %d", ret);
> > -             return ret;
> > +             goto probe_power_off;
> >       }
> >
> >       mutex_init(&ov8856->mutex);
> > @@ -1238,6 +1337,9 @@ static int ov8856_probe(struct i2c_client *client)
> >       v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> >       mutex_destroy(&ov8856->mutex);
> >
> > +probe_power_off:
> > +     __ov8856_power_off(ov8856);
> > +
> >       return ret;
> >  }
> >
> > @@ -1254,11 +1356,18 @@ static const struct acpi_device_id ov8856_acpi_ids[]
> > = {  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);  #endif
> >
> > +static const struct of_device_id ov8856_of_match[] = {
> > +     { .compatible = "ovti,ov8856" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > +
> >  static struct i2c_driver ov8856_i2c_driver = {
> >       .driver = {
> >               .name = "ov8856",
> >               .pm = &ov8856_pm_ops,
> >               .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > +             .of_match_table = ov8856_of_match,
> >       },
> >       .probe_new = ov8856_probe,
> >       .remove = ov8856_remove,
> > --
> > 2.25.1
>

RE: [PATCH v7 2/3] media: ov8856: Add devicetree support

[Kao, Ben]

Hi Robert,

On Thu, May 7, 2020 16:39 Robert Foss wrote:
> On Thu, 7 May 2020 at 10:06, Kao, Ben <ben.kao@intel.com> wrote:
> >
> > Hi Robert,
> >
> > On 20-05-05 12:01, Robert Foss wrote:
> > > Add match table, enable ov8856_probe() to support both ACPI and DT modes.
> > >
> > > ACPI and DT modes are primarily distinguished from by checking for
> > > ACPI mode and by having resource like be NULL.
> > >
> > > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > > ---
> > >
> > > - Changes since v6:
> > >   * Marco: Bail out of __ov8856_power_on earlier if ACPI mode
> > >
> > > - Changes since v5:
> > >   * Maxime & Sakari: Replaced clock tolerance check with warning
> > >
> > > - Changes since v4:
> > >   * Maxime & Sakari: Switch to clock-frequency
> > >
> > > - Changes since v3:
> > >   * Remove redundant {}-brackets
> > >   * Compare xvclk_rate to 5% tolerance
> > >   * Andy: Use dev_fwnode()
> > >   * Andy: Use %pe instead of %ld + PTR_ERR()
> > >   * Andy: Invert reset_gpio logic
> > >   * Andy: Remove dev_dbg() from failing reset_gpio setup
> > >   * Andy: Use dev_err for logging for failures
> > >   * Andy: Remove dev_warn from EDEFER/regulator error path
> > >   * Andy & Sakari: Replaced GPIOD_OUT_XXX with 0/1
> > >   * Maxime & Sakari: Verify clock frequency from DT
> > >   * Sakari: Verify the 'xvclk_rate' is set correctly for ACPI/DT devices
> > >   * Sakari: Remove duplicate ov8856->dev assignment
> > >
> > > - Changes since v2:
> > >   * Added "struct device *dev" member to struct ov8856
> > >   * Andy: Switch to optional version of devm_gpiod_get
> > >   * Andy: Switch to optional version of devm_clk_get
> > >   * Fabio: Add reset sleep period
> > >   * Sakari: Unify defines for 19.2Mhz
> > >   * Sakari: Remove 24Mhz clock, since it isn't needed for supported modes
> > >   * Sakari: Replace dev_info() with dev_dbg()
> > >   * Sakari: Switch induction variable type to unsigned
> > >   * Sakari: Don't wait for reset_gpio when in ACPI mode
> > >   * Sakari: Pull reset GPIO high on power on failure
> > >   * Sakari: Add power on/off to resume/suspend
> > >   * Sakari: Fix indentation
> > >   * Sakari: Power off during ov8856_remove()
> > >   * Sakari: Don't sleep during power-on in ACPI mode
> > >   * Sakari: Switch to getting xvclk from clk_get_rate
> > >
> > > - Changes since v1:
> > >   * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > >   * Fabio: Change n_shutdown_gpio name to reset_gpio
> > >   * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH ->
> > > GPIO_ACTIVE_LOW change
> > >   * Fabio: Remove empty line
> > >   * Fabio: Remove real error from devm_gpiod_get() failures
> > >   * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > >   * Sakari: Use XVCLK rate as provided by DT
> > >
> > >  drivers/media/i2c/ov8856.c | 137
> > > +++++++++++++++++++++++++++++++++----
> > >  1 file changed, 123 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > > index 8655842af275..e6418a79801e 100644
> > > --- a/drivers/media/i2c/ov8856.c
> > > +++ b/drivers/media/i2c/ov8856.c
> > > @@ -3,10 +3,13 @@
> > >
> > >  #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/module.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>
> > > @@ -18,7 +21,7 @@
> > >  #define OV8856_LINK_FREQ_360MHZ              360000000ULL
> > >  #define OV8856_LINK_FREQ_180MHZ              180000000ULL
> > >  #define OV8856_SCLK                  144000000ULL
> > > -#define OV8856_MCLK                  19200000
> > > +#define OV8856_XVCLK_19_2            19200000
> > >  #define OV8856_DATA_LANES            4
> > >  #define OV8856_RGB_DEPTH             10
> > >
> > > @@ -64,6 +67,12 @@
> > >
> > >  #define to_ov8856(_sd)                       container_of(_sd, struct ov8856, sd)
> > >
> > > +static const char * const ov8856_supply_names[] = {
> > > +     "dovdd",        /* Digital I/O power */
> > > +     "avdd",         /* Analog power */
> > > +     "dvdd",         /* Digital core power */
> > > +};
> > > +
> > >  enum {
> > >       OV8856_LINK_FREQ_720MBPS,
> > >       OV8856_LINK_FREQ_360MBPS,
> > > @@ -566,6 +575,11 @@ struct ov8856 {
> > >       struct media_pad pad;
> > >       struct v4l2_ctrl_handler ctrl_handler;
> > >
> > > +     struct device           *dev;
> > > +     struct clk              *xvclk;
> > > +     struct gpio_desc        *reset_gpio;
> > > +     struct regulator_bulk_data
> > > supplies[ARRAY_SIZE(ov8856_supply_names)];
> > > +
> > >       /* V4L2 Controls */
> > >       struct v4l2_ctrl *link_freq;
> > >       struct v4l2_ctrl *pixel_rate;
> > > @@ -908,6 +922,52 @@ static int ov8856_set_stream(struct v4l2_subdev
> > > *sd, int enable)
> > >       return ret;
> > >  }
> > >
> > > +static int __ov8856_power_on(struct ov8856 *ov8856) {
> > > +     struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > > +     int ret;
> > > +
> > > +     if (is_acpi_node(dev_fwnode(ov8856->dev)))
> > > +             return 0;
> > > +
> > > +     ret = clk_prepare_enable(ov8856->xvclk);
> > > +     if (ret < 0) {
> > > +             dev_err(&client->dev, "failed to enable xvclk\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     if (ov8856->reset_gpio) {
> > > +             gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +             usleep_range(1000, 2000);
> > > +     }
> > > +
> > > +     ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > > +                                 ov8856->supplies);
> > > +     if (ret < 0) {
> > > +             dev_err(&client->dev, "failed to enable regulators\n");
> > > +             goto disable_clk;
> > > +     }
> > > +
> > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> > > +     usleep_range(1500, 1800);
> > > +
> > > +     return 0;
> > > +
> > > +disable_clk:
> > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +     clk_disable_unprepare(ov8856->xvclk);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static void __ov8856_power_off(struct ov8856 *ov8856) {
> > > +     gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> > > +     regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > > +                            ov8856->supplies);
> > > +     clk_disable_unprepare(ov8856->xvclk);
> > > +}
> > > +
> > >  static int __maybe_unused ov8856_suspend(struct device *dev)  {
> > >       struct i2c_client *client = to_i2c_client(dev); @@ -918,6
> > > +978,7 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> > >       if (ov8856->streaming)
> > >               ov8856_stop_streaming(ov8856);
> > >
> > > +     __ov8856_power_off(ov8856);
> > >       mutex_unlock(&ov8856->mutex);
> > >
> > >       return 0;
> > > @@ -931,6 +992,8 @@ static int __maybe_unused ov8856_resume(struct
> > > device *dev)
> > >       int ret;
> > >
> > >       mutex_lock(&ov8856->mutex);
> > > +
> > > +     __ov8856_power_on(ov8856);
> > >       if (ov8856->streaming) {
> > >               ret = ov8856_start_streaming(ov8856);
> > >               if (ret) {
> > > @@ -1092,29 +1155,54 @@ static int ov8856_identify_module(struct
> > > ov8856
> > > *ov8856)
> > >       return 0;
> > >  }
> > >
> > > -static int ov8856_check_hwcfg(struct device *dev)
> > > +static int ov8856_get_hwcfg(struct ov8856 *ov8856)
> > >  {
> > > +     struct device *dev = ov8856->dev;
> > >       struct fwnode_handle *ep;
> > >       struct fwnode_handle *fwnode = dev_fwnode(dev);
> > >       struct v4l2_fwnode_endpoint bus_cfg = {
> > >               .bus_type = V4L2_MBUS_CSI2_DPHY
> > >       };
> > > -     u32 mclk;
> > > +     u32 xvclk_rate;
> > >       int ret;
> > >       unsigned int i, j;
> > >
> > >       if (!fwnode)
> > >               return -ENXIO;
> > >
> > > -     ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > +     ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > > +             &xvclk_rate);
> > >       if (ret)
> > >               return ret;
> > >
> > > -     if (mclk != OV8856_MCLK) {
> > > -             dev_err(dev, "external clock %d is not supported", mclk);
> > > -             return -EINVAL;
> > > +     if (!is_acpi_node(fwnode)) {
> > > +             ov8856->xvclk = devm_clk_get(dev, "xvclk");
> > > +             if (IS_ERR(ov8856->xvclk)) {
> > > +                     dev_err(dev, "could not get xvclk clock (%pe)\n",
> > > +                                     ov8856->xvclk);
> > > +                     return PTR_ERR(ov8856->xvclk);
> > > +             }
> > > +
> > > +             clk_set_rate(ov8856->xvclk, xvclk_rate);
> > > +             xvclk_rate = clk_get_rate(ov8856->xvclk);
> > >       }
> > >
> > > +     if (xvclk_rate != OV8856_XVCLK_19_2)
> > > +             dev_warn(dev, "external clock rate %d is unsupported",
> > > xvclk_rate);
> > > +
> > > +     ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > +             GPIOD_OUT_LOW);
> > > +     if (IS_ERR(ov8856->reset_gpio))
> > > +             return PTR_ERR(ov8856->reset_gpio);
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> > > +             ov8856->supplies[i].supply = ov8856_supply_names[i];
> > > +
> > > +     ret = devm_regulator_bulk_get(dev,
> > > ARRAY_SIZE(ov8856_supply_names),
> > > +                                   ov8856->supplies);
> > > +     if (ret)
> > > +             return ret;
> > > +
> >
> > In case of ACPI this cannot get ov8856->reset_gpio  and ov8856->supplies,
> please add the check for ACPI case:
> > if (!is_acpi_node(fwnode)) { }
> 
> Do you prefer the explicit check for clarity reasons or for functional reasons?
> 
> As far as I understand it, reset_gpio will be NULL and supplies will be populated
> with dummy supplies in the ACPI case.
> Both can be treated as if they were initialized fully during later calls to their
> respective APIs.
Understood, the check is unnecessary.

Ben.
> 
> >
> > Thanks.
> > Ben
> >
> > >       ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > >       if (!ep)
> > >               return -ENXIO;
> > > @@ -1169,6 +1257,8 @@ static int ov8856_remove(struct i2c_client *client)
> > >       pm_runtime_disable(&client->dev);
> > >       mutex_destroy(&ov8856->mutex);
> > >
> > > +     __ov8856_power_off(ov8856);
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -1177,22 +1267,31 @@ static int ov8856_probe(struct i2c_client *client)
> > >       struct ov8856 *ov8856;
> > >       int ret;
> > >
> > > -     ret = ov8856_check_hwcfg(&client->dev);
> > > +     ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > > +     if (!ov8856)
> > > +             return -ENOMEM;
> > > +
> > > +     ov8856->dev = &client->dev;
> > > +
> > > +     ret = ov8856_get_hwcfg(ov8856);
> > >       if (ret) {
> > > -             dev_err(&client->dev, "failed to check HW configuration: %d",
> > > +             dev_err(&client->dev, "failed to get HW configuration:
> > > + %d",
> > >                       ret);
> > >               return ret;
> > >       }
> > >
> > > -     ov8856 = devm_kzalloc(&client->dev, sizeof(*ov8856), GFP_KERNEL);
> > > -     if (!ov8856)
> > > -             return -ENOMEM;
> > > -
> > >       v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > > +
> > > +     ret = __ov8856_power_on(ov8856);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "failed to power on\n");
> > > +             return ret;
> > > +     }
> > > +
> > >       ret = ov8856_identify_module(ov8856);
> > >       if (ret) {
> > >               dev_err(&client->dev, "failed to find sensor: %d", ret);
> > > -             return ret;
> > > +             goto probe_power_off;
> > >       }
> > >
> > >       mutex_init(&ov8856->mutex);
> > > @@ -1238,6 +1337,9 @@ static int ov8856_probe(struct i2c_client *client)
> > >       v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> > >       mutex_destroy(&ov8856->mutex);
> > >
> > > +probe_power_off:
> > > +     __ov8856_power_off(ov8856);
> > > +
> > >       return ret;
> > >  }
> > >
> > > @@ -1254,11 +1356,18 @@ static const struct acpi_device_id
> > > ov8856_acpi_ids[] = {  MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> > > #endif
> > >
> > > +static const struct of_device_id ov8856_of_match[] = {
> > > +     { .compatible = "ovti,ov8856" },
> > > +     { /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > > +
> > >  static struct i2c_driver ov8856_i2c_driver = {
> > >       .driver = {
> > >               .name = "ov8856",
> > >               .pm = &ov8856_pm_ops,
> > >               .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > > +             .of_match_table = ov8856_of_match,
> > >       },
> > >       .probe_new = ov8856_probe,
> > >       .remove = ov8856_remove,
> > > --
> > > 2.25.1
> >

[PATCH v7 3/3] media: ov8856: Implement sensor module revision identification

[Robert Foss]

Query the sensor for its module revision, and compare it
to known revisions.

Currently 2A and 1B revision indentification is supported.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---

- Changes since v3:
  * Actually add module revision 2A

- Changes since v2:
  * Add module revision 2A
  * Sakari: Remove ov8856_check_revision()
  * Sakari: Stop EEPROM streaming mode

 drivers/media/i2c/ov8856.c | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index e6418a79801e..3c82c3e588d7 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -32,6 +32,19 @@
 #define OV8856_MODE_STANDBY		0x00
 #define OV8856_MODE_STREAMING		0x01
 
+/* module revisions */
+#define OV8856_2A_MODULE		0x01
+#define OV8856_1B_MODULE		0x02
+
+/* the OTP read-out buffer is at 0x7000 and 0xf is the offset
+ * of the byte in the OTP that means the module revision
+ */
+#define OV8856_MODULE_REVISION		0x700f
+#define OV8856_OTP_MODE_CTRL		0x3d84
+#define OV8856_OTP_LOAD_CTRL		0x3d81
+#define OV8856_OTP_MODE_AUTO		0x00
+#define OV8856_OTP_LOAD_CTRL_ENABLE	BIT(0)
+
 /* vertical-timings from sensor */
 #define OV8856_REG_VTS			0x380e
 #define OV8856_VTS_MAX			0x7fff
@@ -1152,6 +1165,46 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
 		return -ENXIO;
 	}
 
+	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
+			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
+	if (ret)
+		return ret;
+
+	ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL,
+			       OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
+	if (ret) {
+		dev_err(&client->dev, "failed to set otp mode");
+		return ret;
+	}
+
+	ret = ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
+			       OV8856_REG_VALUE_08BIT,
+			       OV8856_OTP_LOAD_CTRL_ENABLE);
+	if (ret) {
+		dev_err(&client->dev, "failed to enable load control");
+		return ret;
+	}
+
+	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
+			      OV8856_REG_VALUE_08BIT, &val);
+	if (ret) {
+		dev_err(&client->dev, "failed to read module revision");
+		return ret;
+	}
+
+	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
+		val,
+		val == OV8856_2A_MODULE ? "2A" :
+		val == OV8856_1B_MODULE ? "1B" : "unknown revision",
+		client->addr);
+
+	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
+			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
+	if (ret) {
+		dev_err(&client->dev, "failed to exit streaming mode");
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v7 3/3] media: ov8856: Implement sensor module revision identification

[Marco Felsch]

Hi Robert,

On 20-05-05 12:01, Robert Foss wrote:
> Query the sensor for its module revision, and compare it
> to known revisions.
> 
> Currently 2A and 1B revision indentification is supported.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - Changes since v3:
>   * Actually add module revision 2A
> 
> - Changes since v2:
>   * Add module revision 2A
>   * Sakari: Remove ov8856_check_revision()
>   * Sakari: Stop EEPROM streaming mode
> 
>  drivers/media/i2c/ov8856.c | 53 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index e6418a79801e..3c82c3e588d7 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -32,6 +32,19 @@
>  #define OV8856_MODE_STANDBY		0x00
>  #define OV8856_MODE_STREAMING		0x01
>  
> +/* module revisions */
> +#define OV8856_2A_MODULE		0x01
> +#define OV8856_1B_MODULE		0x02
> +
> +/* the OTP read-out buffer is at 0x7000 and 0xf is the offset
> + * of the byte in the OTP that means the module revision
> + */
> +#define OV8856_MODULE_REVISION		0x700f
> +#define OV8856_OTP_MODE_CTRL		0x3d84
> +#define OV8856_OTP_LOAD_CTRL		0x3d81
> +#define OV8856_OTP_MODE_AUTO		0x00
> +#define OV8856_OTP_LOAD_CTRL_ENABLE	BIT(0)
> +
>  /* vertical-timings from sensor */
>  #define OV8856_REG_VTS			0x380e
>  #define OV8856_VTS_MAX			0x7fff
> @@ -1152,6 +1165,46 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
>  		return -ENXIO;
>  	}
>  
> +	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
> +			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL,
> +			       OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to set otp mode");
> +		return ret;
> +	}
> +
> +	ret = ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
> +			       OV8856_REG_VALUE_08BIT,
> +			       OV8856_OTP_LOAD_CTRL_ENABLE);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable load control");
> +		return ret;
> +	}
> +
> +	ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
> +			      OV8856_REG_VALUE_08BIT, &val);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to read module revision");
> +		return ret;
> +	}
> +
> +	dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
> +		val,
> +		val == OV8856_2A_MODULE ? "2A" :
> +		val == OV8856_1B_MODULE ? "1B" : "unknown revision",
> +		client->addr);

Pls check the alignment here too.

Regards,
  Marco

> +
> +	ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
> +			       OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to exit streaming mode");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.25.1

Re: [PATCH v7 3/3] media: ov8856: Implement sensor module revision identification

[Robert Foss]

Hey Marco,

On Tue, 5 May 2020 at 12:17, Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi Robert,
>
> On 20-05-05 12:01, Robert Foss wrote:
> > Query the sensor for its module revision, and compare it
> > to known revisions.
> >
> > Currently 2A and 1B revision indentification is supported.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >
> > - Changes since v3:
> >   * Actually add module revision 2A
> >
> > - Changes since v2:
> >   * Add module revision 2A
> >   * Sakari: Remove ov8856_check_revision()
> >   * Sakari: Stop EEPROM streaming mode
> >
> >  drivers/media/i2c/ov8856.c | 53 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index e6418a79801e..3c82c3e588d7 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -32,6 +32,19 @@
> >  #define OV8856_MODE_STANDBY          0x00
> >  #define OV8856_MODE_STREAMING                0x01
> >
> > +/* module revisions */
> > +#define OV8856_2A_MODULE             0x01
> > +#define OV8856_1B_MODULE             0x02
> > +
> > +/* the OTP read-out buffer is at 0x7000 and 0xf is the offset
> > + * of the byte in the OTP that means the module revision
> > + */
> > +#define OV8856_MODULE_REVISION               0x700f
> > +#define OV8856_OTP_MODE_CTRL         0x3d84
> > +#define OV8856_OTP_LOAD_CTRL         0x3d81
> > +#define OV8856_OTP_MODE_AUTO         0x00
> > +#define OV8856_OTP_LOAD_CTRL_ENABLE  BIT(0)
> > +
> >  /* vertical-timings from sensor */
> >  #define OV8856_REG_VTS                       0x380e
> >  #define OV8856_VTS_MAX                       0x7fff
> > @@ -1152,6 +1165,46 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> >               return -ENXIO;
> >       }
> >
> > +     ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
> > +                            OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ov8856_write_reg(ov8856, OV8856_OTP_MODE_CTRL,
> > +                            OV8856_REG_VALUE_08BIT, OV8856_OTP_MODE_AUTO);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to set otp mode");
> > +             return ret;
> > +     }
> > +
> > +     ret = ov8856_write_reg(ov8856, OV8856_OTP_LOAD_CTRL,
> > +                            OV8856_REG_VALUE_08BIT,
> > +                            OV8856_OTP_LOAD_CTRL_ENABLE);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to enable load control");
> > +             return ret;
> > +     }
> > +
> > +     ret = ov8856_read_reg(ov8856, OV8856_MODULE_REVISION,
> > +                           OV8856_REG_VALUE_08BIT, &val);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to read module revision");
> > +             return ret;
> > +     }
> > +
> > +     dev_info(&client->dev, "OV8856 revision %x (%s) at address 0x%02x\n",
> > +             val,
> > +             val == OV8856_2A_MODULE ? "2A" :
> > +             val == OV8856_1B_MODULE ? "1B" : "unknown revision",
> > +             client->addr);
>
> Pls check the alignment here too.

Ack.

>
> Regards,
>   Marco
>
> > +
> > +     ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
> > +                            OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to exit streaming mode");
> > +             return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > --
> > 2.25.1