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

[PATCH v3 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 | 150 ++++++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov8856.c                    | 192 ++++++++++++++++--
 3 files changed, 327 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 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>
---

- 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:
  Fixes comments from from Andy, Tomasz, Sakari, Rob.
  * Convert text documentation to YAML schema.

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

 .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 151 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..beeddfbb8709
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
@@ -0,0 +1,150 @@
+# 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:
+  - Ben Kao <ben.kao@intel.com>
+  - 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.
+
+  assigned-clocks:
+    description:
+      Input clock for the sensor.
+
+  assigned-clock-rates:
+    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 input and output port nodes with endpoint definitions
+      as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          clock-lanes:
+            maxItems: 1
+
+          data-lanes:
+            maxItems: 1
+
+          remote-endpoint: true
+
+        required:
+          - clock-lanes
+          - data-lanes
+          - remote-endpoint
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - assigned-clocks
+  - assigned-clock-rates
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/clock/qcom,camcc-sdm845.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 = <&clock_camcc CAM_CC_MCLK0_CLK>;
+            clock-names = "xvclk";
+            clock-frequency = <19200000>;
+            assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
+            assigned-clock-rates = <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>;
+                    clock-lanes = <0>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <360000000 180000000>;
+                };
+            };
+        };
+    };
+...
\ No newline at end of file
diff --git a/MAINTAINERS b/MAINTAINERS
index a6fbdf354d34..0f99e863978a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12355,6 +12355,7 @@ L:	linux-media@vger.kernel.org
 T:	git git://linuxtv.org/media_tree.git
 S:	Maintained
 F:	drivers/media/i2c/ov8856.c
+F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
 
 OMNIVISION OV9650 SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

Hi Robert,

On 20-03-31 15:33, 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>
> ---
> 
> - 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:
>   Fixes comments from from Andy, Tomasz, Sakari, Rob.
>   * Convert text documentation to YAML schema.
> 
> - Changes since v1:
>   Fixes comments from Sakari, Tomasz
>   * Add clock-frequency and link-frequencies in DT
> 
>  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 151 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..beeddfbb8709
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> @@ -0,0 +1,150 @@
> +# 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:
> +  - Ben Kao <ben.kao@intel.com>
> +  - 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.

Why do we need this here?

> +  assigned-clocks:
> +    description:
> +      Input clock for the sensor.
> +
> +  assigned-clock-rates:
> +    description:
> +      Frequency of the xvclk clock in Hertz.

Also this isn't related to the chip. You need this because you are using
a qcom platform which provides the clock.

IMHO you only need to specify the clock. You can get the frequency with
the clk_get_rate() function.

> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as interface power supply.

Phandle to the interface power supply regulator?

> +
> +  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 input and output port nodes with endpoint definitions
> +      as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +    properties:
> +      endpoint:
> +        type: object
> +
> +        properties:
> +          clock-lanes:
> +            maxItems: 1
> +
> +          data-lanes:
> +            maxItems: 1
> +
> +          remote-endpoint: true
> +
> +        required:
> +          - clock-lanes
> +          - data-lanes
> +          - remote-endpoint
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - assigned-clocks
> +  - assigned-clock-rates
> +  - dovdd-supply
> +  - avdd-supply
> +  - dvdd-supply
> +  - reset-gpios
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/qcom,camcc-sdm845.h>

IMHO we should avoid examples with hardware specific includes.

> +
> +    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 = <&clock_camcc CAM_CC_MCLK0_CLK>;
> +            clock-names = "xvclk";
> +            clock-frequency = <19200000>;
> +            assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
> +            assigned-clock-rates = <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>;
> +                    clock-lanes = <0>;
> +                    data-lanes = <1 2 3 4>;
> +                    link-frequencies = /bits/ 64 <360000000 180000000>;

Should we add the link-frequencies as optional param?

Regards,
  Marco

> +                };
> +            };
> +        };
> +    };
> +...
> \ No newline at end of file
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a6fbdf354d34..0f99e863978a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12355,6 +12355,7 @@ L:	linux-media@vger.kernel.org
>  T:	git git://linuxtv.org/media_tree.git
>  S:	Maintained
>  F:	drivers/media/i2c/ov8856.c
> +F:	Documentation/devicetree/bindings/media/i2c/ov8856.yaml
>  
>  OMNIVISION OV9650 SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
> -- 
> 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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

'Hey Marco,

On Tue, 31 Mar 2020 at 17:13, Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi Robert,
>
> On 20-03-31 15:33, 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>
> > ---
> >
> > - 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:
> >   Fixes comments from from Andy, Tomasz, Sakari, Rob.
> >   * Convert text documentation to YAML schema.
> >
> > - Changes since v1:
> >   Fixes comments from Sakari, Tomasz
> >   * Add clock-frequency and link-frequencies in DT
> >
> >  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 151 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..beeddfbb8709
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > @@ -0,0 +1,150 @@
> > +# 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:
> > +  - Ben Kao <ben.kao@intel.com>
> > +  - 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.
>
> Why do we need this here?
>
> > +  assigned-clocks:
> > +    description:
> > +      Input clock for the sensor.
> > +
> > +  assigned-clock-rates:
> > +    description:
> > +      Frequency of the xvclk clock in Hertz.
>
> Also this isn't related to the chip. You need this because you are using
> a qcom platform which provides the clock.
>
> IMHO you only need to specify the clock. You can get the frequency with
> the clk_get_rate() function.

The way I understood this, was that clk_get_rate() would fetch the
clock rate as defined by the 'assigned-clock-rates'
Is this not the case? And if so, what rate would cllk_get_rate()
actually retrieve?

>
> > +  dovdd-supply:
> > +    description:
> > +      Definition of the regulator used as interface power supply.
>
> Phandle to the interface power supply regulator?
>
> > +
> > +  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 input and output port nodes with endpoint definitions
> > +      as documented in
> > +      Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +
> > +        properties:
> > +          clock-lanes:
> > +            maxItems: 1
> > +
> > +          data-lanes:
> > +            maxItems: 1
> > +
> > +          remote-endpoint: true
> > +
> > +        required:
> > +          - clock-lanes
> > +          - data-lanes
> > +          - remote-endpoint
> > +
> > +    required:
> > +      - endpoint
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - clock-frequency
> > +  - assigned-clocks
> > +  - assigned-clock-rates
> > +  - dovdd-supply
> > +  - avdd-supply
> > +  - dvdd-supply
> > +  - reset-gpios
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
>
> IMHO we should avoid examples with hardware specific includes.

The HW specific include is used for the clocks property.
clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;

Is there a non hw specific clock that would be better to use for examples?

>
> > +
> > +    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 = <&clock_camcc CAM_CC_MCLK0_CLK>;
> > +            clock-names = "xvclk";
> > +            clock-frequency = <19200000>;
> > +            assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
> > +            assigned-clock-rates = <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>;
> > +                    clock-lanes = <0>;
> > +                    data-lanes = <1 2 3 4>;
> > +                    link-frequencies = /bits/ 64 <360000000 180000000>;
>
> Should we add the link-frequencies as optional param?
>
> Regards,
>   Marco
>
> > +                };
> > +            };
> > +        };
> > +    };
> > +...
> > \ No newline at end of file
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a6fbdf354d34..0f99e863978a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12355,6 +12355,7 @@ L:    linux-media@vger.kernel.org
> >  T:   git git://linuxtv.org/media_tree.git
> >  S:   Maintained
> >  F:   drivers/media/i2c/ov8856.c
> > +F:   Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> >
> >  OMNIVISION OV9650 SENSOR DRIVER
> >  M:   Sakari Ailus <sakari.ailus@linux.intel.com>
> > --
> > 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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marco Felsch]

Hi Robert,

On 20-04-02 11:57, Robert Foss wrote:
> 'Hey Marco,
> 
> On Tue, 31 Mar 2020 at 17:13, Marco Felsch <m.felsch@pengutronix.de> wrote:

...

> > > +  assigned-clocks:
> > > +    description:
> > > +      Input clock for the sensor.
> > > +
> > > +  assigned-clock-rates:
> > > +    description:
> > > +      Frequency of the xvclk clock in Hertz.
> >
> > Also this isn't related to the chip. You need this because you are using
> > a qcom platform which provides the clock.
> >
> > IMHO you only need to specify the clock. You can get the frequency with
> > the clk_get_rate() function.
> 
> The way I understood this, was that clk_get_rate() would fetch the
> clock rate as defined by the 'assigned-clock-rates'
> Is this not the case? And if so, what rate would cllk_get_rate()
> actually retrieve?
> 

You're right clk_get_rate() should be used to get the current clk rate
but assigned-cock-rates only applies to your setup where the host
supplies the clk. This is not the case ff we connect a simple external
osc which provides a static (not adjustable) ckl.

...

> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
> >
> > IMHO we should avoid examples with hardware specific includes.
> 
> The HW specific include is used for the clocks property.
> clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
> 
> Is there a non hw specific clock that would be better to use for examples?

Yes, just use:

  clocks = <&cam_osc>;

The dt-validation provides dummy hooks for such phandles.

Regards,
  Marco

> 
> >
> > > +
> > > +    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 = <&clock_camcc CAM_CC_MCLK0_CLK>;
> > > +            clock-names = "xvclk";
> > > +            clock-frequency = <19200000>;
> > > +            assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
> > > +            assigned-clock-rates = <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>;
> > > +                    clock-lanes = <0>;
> > > +                    data-lanes = <1 2 3 4>;
> > > +                    link-frequencies = /bits/ 64 <360000000 180000000>;
> >
> > Should we add the link-frequencies as optional param?
> >
> > Regards,
> >   Marco
> >
> > > +                };
> > > +            };
> > > +        };
> > > +    };
> > > +...
> > > \ No newline at end of file
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index a6fbdf354d34..0f99e863978a 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -12355,6 +12355,7 @@ L:    linux-media@vger.kernel.org
> > >  T:   git git://linuxtv.org/media_tree.git
> > >  S:   Maintained
> > >  F:   drivers/media/i2c/ov8856.c
> > > +F:   Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > >
> > >  OMNIVISION OV9650 SENSOR DRIVER
> > >  M:   Sakari Ailus <sakari.ailus@linux.intel.com>
> > > --
> > > 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 |
> 

-- 
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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3709 bytes --]

Hi,

On Tue, Mar 31, 2020 at 03:33:44PM +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>
> ---
>
> - 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:
>   Fixes comments from from Andy, Tomasz, Sakari, Rob.
>   * Convert text documentation to YAML schema.
>
> - Changes since v1:
>   Fixes comments from Sakari, Tomasz
>   * Add clock-frequency and link-frequencies in DT
>
>  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 151 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..beeddfbb8709
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> @@ -0,0 +1,150 @@
> +# 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:
> +  - Ben Kao <ben.kao@intel.com>
> +  - 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.

We also had that discussion recently for another omnivision sensor
(ov5645 iirc), but what is clock-frequency useful for?

It seems that the sensor is passed in clocks, so if you need to
retrieve the clock rate you should use the clock API instead.

Looking at the driver, it looks like it first retrieves the clock, set
it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2
(19.2 MHz).

The datasheet says that the sensor can have any frequency in the 6 -
27 MHz range, so this is a driver limitation and should be set in the
driver using the clock API, and you can always bail out if it doesn't
provide a rate that is not acceptable for the drivers assumption.

In any case, you don't need clock-frequency here...

> +  assigned-clocks:
> +    description:
> +      Input clock for the sensor.
> +
> +  assigned-clock-rates:
> +    description:
> +      Frequency of the xvclk clock in Hertz.

And you don't need assigned-clock-rates either.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

Hey Maxime,

On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Tue, Mar 31, 2020 at 03:33:44PM +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>
> > ---
> >
> > - 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:
> >   Fixes comments from from Andy, Tomasz, Sakari, Rob.
> >   * Convert text documentation to YAML schema.
> >
> > - Changes since v1:
> >   Fixes comments from Sakari, Tomasz
> >   * Add clock-frequency and link-frequencies in DT
> >
> >  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 151 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..beeddfbb8709
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > @@ -0,0 +1,150 @@
> > +# 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:
> > +  - Ben Kao <ben.kao@intel.com>
> > +  - 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.
>
> We also had that discussion recently for another omnivision sensor
> (ov5645 iirc), but what is clock-frequency useful for?
>
> It seems that the sensor is passed in clocks, so if you need to
> retrieve the clock rate you should use the clock API instead.
>
> Looking at the driver, it looks like it first retrieves the clock, set
> it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2
> (19.2 MHz).

As far as I understand it, 19.2MHz is requirement for the sensor mode
that currently defaults to. Some modes require higher clock speeds
than this however.

>
> The datasheet says that the sensor can have any frequency in the 6 -
> 27 MHz range, so this is a driver limitation and should be set in the
> driver using the clock API, and you can always bail out if it doesn't
> provide a rate that is not acceptable for the drivers assumption.
>
> In any case, you don't need clock-frequency here...

So your suggestion is that we remove all clocks-rate properties, and
replace the clk_get_rate() calls in the driver with clk_set_rate()
calls for the desired frequencies?

>
> > +  assigned-clocks:
> > +    description:
> > +      Input clock for the sensor.
> > +
> > +  assigned-clock-rates:
> > +    description:
> > +      Frequency of the xvclk clock in Hertz.
>
> And you don't need assigned-clock-rates either.

Ack.

>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Robert,

On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote:
> Hey Maxime,
> 
> On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 31, 2020 at 03:33:44PM +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>
> > > ---
> > >
> > > - 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:
> > >   Fixes comments from from Andy, Tomasz, Sakari, Rob.
> > >   * Convert text documentation to YAML schema.
> > >
> > > - Changes since v1:
> > >   Fixes comments from Sakari, Tomasz
> > >   * Add clock-frequency and link-frequencies in DT
> > >
> > >  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
> > >  MAINTAINERS                                   |   1 +
> > >  2 files changed, 151 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..beeddfbb8709
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > @@ -0,0 +1,150 @@
> > > +# 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:
> > > +  - Ben Kao <ben.kao@intel.com>
> > > +  - 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.
> >
> > We also had that discussion recently for another omnivision sensor
> > (ov5645 iirc), but what is clock-frequency useful for?
> >
> > It seems that the sensor is passed in clocks, so if you need to
> > retrieve the clock rate you should use the clock API instead.
> >
> > Looking at the driver, it looks like it first retrieves the clock, set
> > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2
> > (19.2 MHz).
> 
> As far as I understand it, 19.2MHz is requirement for the sensor mode
> that currently defaults to. Some modes require higher clock speeds
> than this however.

It's very system specific. Either way, bindings should not assume a
particular driver implementation.

> 
> >
> > The datasheet says that the sensor can have any frequency in the 6 -
> > 27 MHz range, so this is a driver limitation and should be set in the
> > driver using the clock API, and you can always bail out if it doesn't
> > provide a rate that is not acceptable for the drivers assumption.
> >
> > In any case, you don't need clock-frequency here...
> 
> So your suggestion is that we remove all clocks-rate properties, and
> replace the clk_get_rate() calls in the driver with clk_set_rate()
> calls for the desired frequencies?

The driver shouldn't set the rate here unless it gets it from DT (but that
was not the intention). So the driver should get the frequency instead.

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 6267 bytes --]

Hi,

On Sat, Apr 04, 2020 at 02:27:36AM +0300, Sakari Ailus wrote:
> Hi Robert,
>
> On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote:
> > Hey Maxime,
> >
> > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Mar 31, 2020 at 03:33:44PM +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>
> > > > ---
> > > >
> > > > - 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:
> > > >   Fixes comments from from Andy, Tomasz, Sakari, Rob.
> > > >   * Convert text documentation to YAML schema.
> > > >
> > > > - Changes since v1:
> > > >   Fixes comments from Sakari, Tomasz
> > > >   * Add clock-frequency and link-frequencies in DT
> > > >
> > > >  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
> > > >  MAINTAINERS                                   |   1 +
> > > >  2 files changed, 151 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..beeddfbb8709
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > > @@ -0,0 +1,150 @@
> > > > +# 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:
> > > > +  - Ben Kao <ben.kao@intel.com>
> > > > +  - 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.
> > >
> > > We also had that discussion recently for another omnivision sensor
> > > (ov5645 iirc), but what is clock-frequency useful for?
> > >
> > > It seems that the sensor is passed in clocks, so if you need to
> > > retrieve the clock rate you should use the clock API instead.
> > >
> > > Looking at the driver, it looks like it first retrieves the clock, set
> > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2
> > > (19.2 MHz).
> >
> > As far as I understand it, 19.2MHz is requirement for the sensor mode
> > that currently defaults to. Some modes require higher clock speeds
> > than this however.
>
> It's very system specific. Either way, bindings should not assume a
> particular driver implementation.
>
> >
> > >
> > > The datasheet says that the sensor can have any frequency in the 6 -
> > > 27 MHz range, so this is a driver limitation and should be set in the
> > > driver using the clock API, and you can always bail out if it doesn't
> > > provide a rate that is not acceptable for the drivers assumption.
> > >
> > > In any case, you don't need clock-frequency here...
> >
> > So your suggestion is that we remove all clocks-rate properties, and
> > replace the clk_get_rate() calls in the driver with clk_set_rate()
> > calls for the desired frequencies?
>
> The driver shouldn't set the rate here unless it gets it from DT (but that
> was not the intention). So the driver should get the frequency instead.

I'm actually saying the opposite :)

Like you were saying, the binding (or DT, for that matter) shouldn't
assume a particular driver implementation.

So one corollary is that if the driver has some restrictions in Linux,
it shouldn't be part of the binding, right?

This binding uses multiple clock properties but as far as I can see,
the driver retrieves a clock using clocks and makes sure that its rate
match its limitation of 19.2MHz using clock-frequency (which is
redundant on a clk_get_rate on the clocks provided earlier).

I'm suspecting that the parent clock on multiple SoCs can be
configured and is not a fixed rate crystal, so assigned-clocks-rate is
here just to make sure we set the frequency at the one being checked
in the driver's probe so that it all works.

But that 19.2MHz is not a limitation of the device itself, it's a
limitation of our implementation, so we can instead implement
something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
sure that our parent clock is configured at the right rate) and the
clk_get_rate and compare that to 19.2MHz (to make sure that it's not
been rounded too far apart from the frequency we expect).

This is doing exactly the same thing, except that we don't encode our
implementation limitations in the DT, but in the driver instead.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

Hey Maxime,

On Sat, 4 Apr 2020 at 11:34, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Sat, Apr 04, 2020 at 02:27:36AM +0300, Sakari Ailus wrote:
> > Hi Robert,
> >
> > On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote:
> > > Hey Maxime,
> > >
> > > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Mar 31, 2020 at 03:33:44PM +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>
> > > > > ---
> > > > >
> > > > > - 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:
> > > > >   Fixes comments from from Andy, Tomasz, Sakari, Rob.
> > > > >   * Convert text documentation to YAML schema.
> > > > >
> > > > > - Changes since v1:
> > > > >   Fixes comments from Sakari, Tomasz
> > > > >   * Add clock-frequency and link-frequencies in DT
> > > > >
> > > > >  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
> > > > >  MAINTAINERS                                   |   1 +
> > > > >  2 files changed, 151 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..beeddfbb8709
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > > > @@ -0,0 +1,150 @@
> > > > > +# 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:
> > > > > +  - Ben Kao <ben.kao@intel.com>
> > > > > +  - 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.
> > > >
> > > > We also had that discussion recently for another omnivision sensor
> > > > (ov5645 iirc), but what is clock-frequency useful for?
> > > >
> > > > It seems that the sensor is passed in clocks, so if you need to
> > > > retrieve the clock rate you should use the clock API instead.
> > > >
> > > > Looking at the driver, it looks like it first retrieves the clock, set
> > > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2
> > > > (19.2 MHz).
> > >
> > > As far as I understand it, 19.2MHz is requirement for the sensor mode
> > > that currently defaults to. Some modes require higher clock speeds
> > > than this however.
> >
> > It's very system specific. Either way, bindings should not assume a
> > particular driver implementation.
> >
> > >
> > > >
> > > > The datasheet says that the sensor can have any frequency in the 6 -
> > > > 27 MHz range, so this is a driver limitation and should be set in the
> > > > driver using the clock API, and you can always bail out if it doesn't
> > > > provide a rate that is not acceptable for the drivers assumption.
> > > >
> > > > In any case, you don't need clock-frequency here...
> > >
> > > So your suggestion is that we remove all clocks-rate properties, and
> > > replace the clk_get_rate() calls in the driver with clk_set_rate()
> > > calls for the desired frequencies?
> >
> > The driver shouldn't set the rate here unless it gets it from DT (but that
> > was not the intention). So the driver should get the frequency instead.
>
> I'm actually saying the opposite :)
>
> Like you were saying, the binding (or DT, for that matter) shouldn't
> assume a particular driver implementation.
>
> So one corollary is that if the driver has some restrictions in Linux,
> it shouldn't be part of the binding, right?
>
> This binding uses multiple clock properties but as far as I can see,
> the driver retrieves a clock using clocks and makes sure that its rate
> match its limitation of 19.2MHz using clock-frequency (which is
> redundant on a clk_get_rate on the clocks provided earlier).
>
> I'm suspecting that the parent clock on multiple SoCs can be
> configured and is not a fixed rate crystal, so assigned-clocks-rate is
> here just to make sure we set the frequency at the one being checked
> in the driver's probe so that it all works.
>
> But that 19.2MHz is not a limitation of the device itself, it's a
> limitation of our implementation, so we can instead implement
> something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> sure that our parent clock is configured at the right rate) and the
> clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> been rounded too far apart from the frequency we expect).
>
> This is doing exactly the same thing, except that we don't encode our
> implementation limitations in the DT, but in the driver instead.
>

Thanks for taking the time to explain this.
I'll spin a new revision that moves the clock rate handling into the driver.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Maxime,

On Sat, Apr 04, 2020 at 11:34:46AM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Apr 04, 2020 at 02:27:36AM +0300, Sakari Ailus wrote:
> > Hi Robert,
> >
> > On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote:
> > > Hey Maxime,
> > >
> > > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Mar 31, 2020 at 03:33:44PM +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>
> > > > > ---
> > > > >
> > > > > - 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:
> > > > >   Fixes comments from from Andy, Tomasz, Sakari, Rob.
> > > > >   * Convert text documentation to YAML schema.
> > > > >
> > > > > - Changes since v1:
> > > > >   Fixes comments from Sakari, Tomasz
> > > > >   * Add clock-frequency and link-frequencies in DT
> > > > >
> > > > >  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
> > > > >  MAINTAINERS                                   |   1 +
> > > > >  2 files changed, 151 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..beeddfbb8709
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > > > @@ -0,0 +1,150 @@
> > > > > +# 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:
> > > > > +  - Ben Kao <ben.kao@intel.com>
> > > > > +  - 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.
> > > >
> > > > We also had that discussion recently for another omnivision sensor
> > > > (ov5645 iirc), but what is clock-frequency useful for?
> > > >
> > > > It seems that the sensor is passed in clocks, so if you need to
> > > > retrieve the clock rate you should use the clock API instead.
> > > >
> > > > Looking at the driver, it looks like it first retrieves the clock, set
> > > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2
> > > > (19.2 MHz).
> > >
> > > As far as I understand it, 19.2MHz is requirement for the sensor mode
> > > that currently defaults to. Some modes require higher clock speeds
> > > than this however.
> >
> > It's very system specific. Either way, bindings should not assume a
> > particular driver implementation.
> >
> > >
> > > >
> > > > The datasheet says that the sensor can have any frequency in the 6 -
> > > > 27 MHz range, so this is a driver limitation and should be set in the
> > > > driver using the clock API, and you can always bail out if it doesn't
> > > > provide a rate that is not acceptable for the drivers assumption.
> > > >
> > > > In any case, you don't need clock-frequency here...
> > >
> > > So your suggestion is that we remove all clocks-rate properties, and
> > > replace the clk_get_rate() calls in the driver with clk_set_rate()
> > > calls for the desired frequencies?
> >
> > The driver shouldn't set the rate here unless it gets it from DT (but that
> > was not the intention). So the driver should get the frequency instead.
> 
> I'm actually saying the opposite :)
> 
> Like you were saying, the binding (or DT, for that matter) shouldn't
> assume a particular driver implementation.
> 
> So one corollary is that if the driver has some restrictions in Linux,
> it shouldn't be part of the binding, right?

Correct.

> 
> This binding uses multiple clock properties but as far as I can see,
> the driver retrieves a clock using clocks and makes sure that its rate
> match its limitation of 19.2MHz using clock-frequency (which is
> redundant on a clk_get_rate on the clocks provided earlier).
> 
> I'm suspecting that the parent clock on multiple SoCs can be
> configured and is not a fixed rate crystal, so assigned-clocks-rate is
> here just to make sure we set the frequency at the one being checked
> in the driver's probe so that it all works.

Agreed.

> 
> But that 19.2MHz is not a limitation of the device itself, it's a
> limitation of our implementation, so we can instead implement
> something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> sure that our parent clock is configured at the right rate) and the
> clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> been rounded too far apart from the frequency we expect).
> 
> This is doing exactly the same thing, except that we don't encode our
> implementation limitations in the DT, but in the driver instead.

What I really wanted to say that a driver that doesn't get the clock
frequency from DT but still sets that frequency is broken.

This frequency is highly system specific, and in many cases only a certain
frequency is usable, for a few reasons: On many SoCs, not all common
frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
are being used as well), and then that frequency affects the usable CSI-2
bus frequencies directly --- and of those, only safe, known-good ones
should be used. IOW, getting the external clock frequency wrong typically
has an effect that that none of the known-good CSI-2 bus clock frequencies
are available.

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1667 bytes --]

Hi Sakari,

On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > But that 19.2MHz is not a limitation of the device itself, it's a
> > limitation of our implementation, so we can instead implement
> > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > sure that our parent clock is configured at the right rate) and the
> > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > been rounded too far apart from the frequency we expect).
> >
> > This is doing exactly the same thing, except that we don't encode our
> > implementation limitations in the DT, but in the driver instead.
>
> What I really wanted to say that a driver that doesn't get the clock
> frequency from DT but still sets that frequency is broken.
>
> This frequency is highly system specific, and in many cases only a certain
> frequency is usable, for a few reasons: On many SoCs, not all common
> frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> are being used as well), and then that frequency affects the usable CSI-2
> bus frequencies directly --- and of those, only safe, known-good ones
> should be used. IOW, getting the external clock frequency wrong typically
> has an effect that that none of the known-good CSI-2 bus clock frequencies
> are available.

So clock-frequency is not about the "Frequency of the xvclk clock in
Hertz", but the frequency at which that clock must run on this
particular SoC / board to be functional?

If so, then yeah, we should definitely keep it, but the documentation
of the binding should be made clearer as well.

assigned-clock-rates should still go away though.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

Hey Maixme & Sakari,

On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Sakari,
>
> On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > limitation of our implementation, so we can instead implement
> > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > sure that our parent clock is configured at the right rate) and the
> > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > been rounded too far apart from the frequency we expect).
> > >
> > > This is doing exactly the same thing, except that we don't encode our
> > > implementation limitations in the DT, but in the driver instead.
> >
> > What I really wanted to say that a driver that doesn't get the clock
> > frequency from DT but still sets that frequency is broken.
> >
> > This frequency is highly system specific, and in many cases only a certain
> > frequency is usable, for a few reasons: On many SoCs, not all common
> > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > are being used as well), and then that frequency affects the usable CSI-2
> > bus frequencies directly --- and of those, only safe, known-good ones
> > should be used. IOW, getting the external clock frequency wrong typically
> > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > are available.
>
> So clock-frequency is not about the "Frequency of the xvclk clock in
> Hertz", but the frequency at which that clock must run on this
> particular SoC / board to be functional?
>
> If so, then yeah, we should definitely keep it, but the documentation
> of the binding should be made clearer as well.
>

Alright so, let me summarise the desired approach then.

ACPI:
  - Fetch the "clock-frequency" property
  - Verify it to be 19.2Mhz

DT:
  - Fetch the "clock-frequency" property
  - Verify it to be 19.2Mhz
  - Get xvclk clock
  - Get xvclk clock rate
  - Verify xvclk clock rate to be 19.2Mhz

Since the xvclk clock isn't available under ACPI, this is how the two
cases would be distinguished between.
Does this sound about right?

> assigned-clock-rates should still go away though.

Ack.

>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2512 bytes --]

Hi Robert,

On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > limitation of our implementation, so we can instead implement
> > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > sure that our parent clock is configured at the right rate) and the
> > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > been rounded too far apart from the frequency we expect).
> > > >
> > > > This is doing exactly the same thing, except that we don't encode our
> > > > implementation limitations in the DT, but in the driver instead.
> > >
> > > What I really wanted to say that a driver that doesn't get the clock
> > > frequency from DT but still sets that frequency is broken.
> > >
> > > This frequency is highly system specific, and in many cases only a certain
> > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > are being used as well), and then that frequency affects the usable CSI-2
> > > bus frequencies directly --- and of those, only safe, known-good ones
> > > should be used. IOW, getting the external clock frequency wrong typically
> > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > are available.
> >
> > So clock-frequency is not about the "Frequency of the xvclk clock in
> > Hertz", but the frequency at which that clock must run on this
> > particular SoC / board to be functional?
> >
> > If so, then yeah, we should definitely keep it, but the documentation
> > of the binding should be made clearer as well.
>
> Alright so, let me summarise the desired approach then.

There's a separate discussion on the same topic here:
https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/

> ACPI:
>   - Fetch the "clock-frequency" property
>   - Verify it to be 19.2Mhz
>
> DT:
>   - Fetch the "clock-frequency" property
>   - Verify it to be 19.2Mhz
>   - Get xvclk clock
>   - Get xvclk clock rate
>   - Verify xvclk clock rate to be 19.2Mhz

The current status is that you should
's/clock-frequency/link-frequencies/', and in order to replace
assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
between steps 3 and 4

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Robert,
>
> On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > limitation of our implementation, so we can instead implement
> > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > sure that our parent clock is configured at the right rate) and the
> > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > been rounded too far apart from the frequency we expect).
> > > > >
> > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > implementation limitations in the DT, but in the driver instead.
> > > >
> > > > What I really wanted to say that a driver that doesn't get the clock
> > > > frequency from DT but still sets that frequency is broken.
> > > >
> > > > This frequency is highly system specific, and in many cases only a certain
> > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > are available.
> > >
> > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > Hertz", but the frequency at which that clock must run on this
> > > particular SoC / board to be functional?
> > >
> > > If so, then yeah, we should definitely keep it, but the documentation
> > > of the binding should be made clearer as well.
> >
> > Alright so, let me summarise the desired approach then.
>
> There's a separate discussion on the same topic here:
> https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/

Thanks for the link.

>
> > ACPI:
> >   - Fetch the "clock-frequency" property
> >   - Verify it to be 19.2Mhz
> >
> > DT:
> >   - Fetch the "clock-frequency" property
> >   - Verify it to be 19.2Mhz
> >   - Get xvclk clock
> >   - Get xvclk clock rate
> >   - Verify xvclk clock rate to be 19.2Mhz
>
> The current status is that you should
> 's/clock-frequency/link-frequencies/', and in order to replace
> assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> between steps 3 and 4

Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
I imagine that would cause some breakage.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Robert,
> >
> > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > limitation of our implementation, so we can instead implement
> > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > been rounded too far apart from the frequency we expect).
> > > > > >
> > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > implementation limitations in the DT, but in the driver instead.
> > > > >
> > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > frequency from DT but still sets that frequency is broken.
> > > > >
> > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > are available.
> > > >
> > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > Hertz", but the frequency at which that clock must run on this
> > > > particular SoC / board to be functional?
> > > >
> > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > of the binding should be made clearer as well.
> > >
> > > Alright so, let me summarise the desired approach then.
> >
> > There's a separate discussion on the same topic here:
> > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> 
> Thanks for the link.
> 
> >
> > > ACPI:
> > >   - Fetch the "clock-frequency" property
> > >   - Verify it to be 19.2Mhz
> > >
> > > DT:
> > >   - Fetch the "clock-frequency" property
> > >   - Verify it to be 19.2Mhz
> > >   - Get xvclk clock
> > >   - Get xvclk clock rate
> > >   - Verify xvclk clock rate to be 19.2Mhz
> >
> > The current status is that you should
> > 's/clock-frequency/link-frequencies/', and in order to replace
> > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > between steps 3 and 4
> 
> Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> I imagine that would cause some breakage.

It would, yes, and it would be no more correct on DT either.

There are basically two possibilities here; either use the clock-frequency
property and set the frequency, or rely on assigned-clock-rates, and get
the frequency instead.

The latter, while I understand it is generally preferred, comes with having
to figure out the register list set that closest matches the frequency
obtained. The former generally gets around this silently by the clock
driver setting the closest frequency it can support.

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tomasz Figa]

On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi Robert,
> > >
> > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > >
> > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > >
> > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > frequency from DT but still sets that frequency is broken.
> > > > > >
> > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > are available.
> > > > >
> > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > Hertz", but the frequency at which that clock must run on this
> > > > > particular SoC / board to be functional?
> > > > >
> > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > of the binding should be made clearer as well.
> > > >
> > > > Alright so, let me summarise the desired approach then.
> > >
> > > There's a separate discussion on the same topic here:
> > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> >
> > Thanks for the link.
> >
> > >
> > > > ACPI:
> > > >   - Fetch the "clock-frequency" property
> > > >   - Verify it to be 19.2Mhz
> > > >
> > > > DT:
> > > >   - Fetch the "clock-frequency" property
> > > >   - Verify it to be 19.2Mhz
> > > >   - Get xvclk clock
> > > >   - Get xvclk clock rate
> > > >   - Verify xvclk clock rate to be 19.2Mhz
> > >
> > > The current status is that you should
> > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > between steps 3 and 4
> >
> > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > I imagine that would cause some breakage.
>
> It would, yes, and it would be no more correct on DT either.
>
> There are basically two possibilities here; either use the clock-frequency
> property and set the frequency, or rely on assigned-clock-rates, and get
> the frequency instead.
>
> The latter, while I understand it is generally preferred, comes with having
> to figure out the register list set that closest matches the frequency
> obtained. The former generally gets around this silently by the clock
> driver setting the closest frequency it can support.

Wouldn't the former actually cause problems, because the closest
frequency the clock driver can support could be pretty far from the
one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
resulting frequency anyway.

Perhaps a simplified approach of rounding the result of clk_get_rate()
to a multiple of 100 KHz and comparing it to the hardcoded value would
be enough in practice?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Tomasz,

On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi Robert,
> > > >
> > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > >
> > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > >
> > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > >
> > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > are available.
> > > > > >
> > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > particular SoC / board to be functional?
> > > > > >
> > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > of the binding should be made clearer as well.
> > > > >
> > > > > Alright so, let me summarise the desired approach then.
> > > >
> > > > There's a separate discussion on the same topic here:
> > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > >
> > > Thanks for the link.
> > >
> > > >
> > > > > ACPI:
> > > > >   - Fetch the "clock-frequency" property
> > > > >   - Verify it to be 19.2Mhz
> > > > >
> > > > > DT:
> > > > >   - Fetch the "clock-frequency" property
> > > > >   - Verify it to be 19.2Mhz
> > > > >   - Get xvclk clock
> > > > >   - Get xvclk clock rate
> > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > >
> > > > The current status is that you should
> > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > between steps 3 and 4
> > >
> > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > I imagine that would cause some breakage.
> >
> > It would, yes, and it would be no more correct on DT either.
> >
> > There are basically two possibilities here; either use the clock-frequency
> > property and set the frequency, or rely on assigned-clock-rates, and get
> > the frequency instead.
> >
> > The latter, while I understand it is generally preferred, comes with having
> > to figure out the register list set that closest matches the frequency
> > obtained. The former generally gets around this silently by the clock
> > driver setting the closest frequency it can support.
> 
> Wouldn't the former actually cause problems, because the closest
> frequency the clock driver can support could be pretty far from the
> one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> resulting frequency anyway.

That's possible, yes; in this case there wouldn't be a guarantee the
frequency wouldn't be far off.

> 
> Perhaps a simplified approach of rounding the result of clk_get_rate()
> to a multiple of 100 KHz and comparing it to the hardcoded value would
> be enough in practice?

Then the question is: how much deviation from the expected value is
allowed? I think there was another case where such range was checked and
because the clock was just slightly more off, the probe failed because of
that.

I'd in that case check some fairly wide range, and print a warning if the
frequency isn't in that range, but still proceed. It's generally impossible
to set a precise limit on the tolerance.

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5112 bytes --]

On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >
> > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > Hi Robert,
> > > > >
> > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > >
> > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > >
> > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > >
> > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > are available.
> > > > > > >
> > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > particular SoC / board to be functional?
> > > > > > >
> > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > of the binding should be made clearer as well.
> > > > > >
> > > > > > Alright so, let me summarise the desired approach then.
> > > > >
> > > > > There's a separate discussion on the same topic here:
> > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > >
> > > > Thanks for the link.
> > > >
> > > > >
> > > > > > ACPI:
> > > > > >   - Fetch the "clock-frequency" property
> > > > > >   - Verify it to be 19.2Mhz
> > > > > >
> > > > > > DT:
> > > > > >   - Fetch the "clock-frequency" property
> > > > > >   - Verify it to be 19.2Mhz
> > > > > >   - Get xvclk clock
> > > > > >   - Get xvclk clock rate
> > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > >
> > > > > The current status is that you should
> > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > between steps 3 and 4
> > > >
> > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > I imagine that would cause some breakage.
> > >
> > > It would, yes, and it would be no more correct on DT either.
> > >
> > > There are basically two possibilities here; either use the clock-frequency
> > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > the frequency instead.
> > >
> > > The latter, while I understand it is generally preferred, comes with having
> > > to figure out the register list set that closest matches the frequency
> > > obtained. The former generally gets around this silently by the clock
> > > driver setting the closest frequency it can support.
> >
> > Wouldn't the former actually cause problems, because the closest
> > frequency the clock driver can support could be pretty far from the
> > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > resulting frequency anyway.
>
> That's possible, yes; in this case there wouldn't be a guarantee the
> frequency wouldn't be far off.

assigned-clock-rates is really fragile... There's zero guarantee on
how far the actual rate is going to be from the asked one, but more
importantly you have zero guarantee on the time frame that rate is
going to be enforced for.

It's simply going to change the rate as a one-off thing, and if
there's the next millisecond someone else is going to change its rate
one way or another, it's going to do so and you won't have any
notification.

And even semantically, they do not share the same meaning at all, so
we should really stop using assigned-clock-rates if we can, instead of
pushing it.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tomasz Figa]

On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > >
> > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > >
> > > > > > Hi Robert,
> > > > > >
> > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > >
> > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > >
> > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > >
> > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > are available.
> > > > > > > >
> > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > particular SoC / board to be functional?
> > > > > > > >
> > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > of the binding should be made clearer as well.
> > > > > > >
> > > > > > > Alright so, let me summarise the desired approach then.
> > > > > >
> > > > > > There's a separate discussion on the same topic here:
> > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > >
> > > > > Thanks for the link.
> > > > >
> > > > > >
> > > > > > > ACPI:
> > > > > > >   - Fetch the "clock-frequency" property
> > > > > > >   - Verify it to be 19.2Mhz
> > > > > > >
> > > > > > > DT:
> > > > > > >   - Fetch the "clock-frequency" property
> > > > > > >   - Verify it to be 19.2Mhz
> > > > > > >   - Get xvclk clock
> > > > > > >   - Get xvclk clock rate
> > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > >
> > > > > > The current status is that you should
> > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > between steps 3 and 4
> > > > >
> > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > I imagine that would cause some breakage.
> > > >
> > > > It would, yes, and it would be no more correct on DT either.
> > > >
> > > > There are basically two possibilities here; either use the clock-frequency
> > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > the frequency instead.
> > > >
> > > > The latter, while I understand it is generally preferred, comes with having
> > > > to figure out the register list set that closest matches the frequency
> > > > obtained. The former generally gets around this silently by the clock
> > > > driver setting the closest frequency it can support.
> > >
> > > Wouldn't the former actually cause problems, because the closest
> > > frequency the clock driver can support could be pretty far from the
> > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > resulting frequency anyway.
> >
> > That's possible, yes; in this case there wouldn't be a guarantee the
> > frequency wouldn't be far off.
>
> assigned-clock-rates is really fragile... There's zero guarantee on
> how far the actual rate is going to be from the asked one, but more
> importantly you have zero guarantee on the time frame that rate is
> going to be enforced for.
>

Is there such a guarantee if clk_set_rate() is called?

> It's simply going to change the rate as a one-off thing, and if
> there's the next millisecond someone else is going to change its rate
> one way or another, it's going to do so and you won't have any
> notification.
>
> And even semantically, they do not share the same meaning at all, so
> we should really stop using assigned-clock-rates if we can, instead of
> pushing it.
>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5668 bytes --]

On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > >
> > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > >
> > > > > > > Hi Robert,
> > > > > > >
> > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > >
> > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > >
> > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > >
> > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > are available.
> > > > > > > > >
> > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > particular SoC / board to be functional?
> > > > > > > > >
> > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > of the binding should be made clearer as well.
> > > > > > > >
> > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > >
> > > > > > > There's a separate discussion on the same topic here:
> > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > >
> > > > > > Thanks for the link.
> > > > > >
> > > > > > >
> > > > > > > > ACPI:
> > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > >
> > > > > > > > DT:
> > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > >   - Get xvclk clock
> > > > > > > >   - Get xvclk clock rate
> > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > >
> > > > > > > The current status is that you should
> > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > between steps 3 and 4
> > > > > >
> > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > I imagine that would cause some breakage.
> > > > >
> > > > > It would, yes, and it would be no more correct on DT either.
> > > > >
> > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > the frequency instead.
> > > > >
> > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > to figure out the register list set that closest matches the frequency
> > > > > obtained. The former generally gets around this silently by the clock
> > > > > driver setting the closest frequency it can support.
> > > >
> > > > Wouldn't the former actually cause problems, because the closest
> > > > frequency the clock driver can support could be pretty far from the
> > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > resulting frequency anyway.
> > >
> > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > frequency wouldn't be far off.
> >
> > assigned-clock-rates is really fragile... There's zero guarantee on
> > how far the actual rate is going to be from the asked one, but more
> > importantly you have zero guarantee on the time frame that rate is
> > going to be enforced for.
>
> Is there such a guarantee if clk_set_rate() is called?

with clk_set_rate itself, no, but...

> > It's simply going to change the rate as a one-off thing, and if
> > there's the next millisecond someone else is going to change its rate
> > one way or another, it's going to do so and you won't have any
> > notification.

You can get notified, and you can use clk_set_rate_exclusive if you
*really* want to enforce it.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Maxime,

On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:
> On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > > >
> > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > >
> > > > > > > > Hi Robert,
> > > > > > > >
> > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > > >
> > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > > >
> > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > > >
> > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > > are available.
> > > > > > > > > >
> > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > > particular SoC / board to be functional?
> > > > > > > > > >
> > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > > of the binding should be made clearer as well.
> > > > > > > > >
> > > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > > >
> > > > > > > > There's a separate discussion on the same topic here:
> > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > > >
> > > > > > > Thanks for the link.
> > > > > > >
> > > > > > > >
> > > > > > > > > ACPI:
> > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > >
> > > > > > > > > DT:
> > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > >   - Get xvclk clock
> > > > > > > > >   - Get xvclk clock rate
> > > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > > >
> > > > > > > > The current status is that you should
> > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > > between steps 3 and 4
> > > > > > >
> > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > > I imagine that would cause some breakage.
> > > > > >
> > > > > > It would, yes, and it would be no more correct on DT either.
> > > > > >
> > > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > > the frequency instead.
> > > > > >
> > > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > > to figure out the register list set that closest matches the frequency
> > > > > > obtained. The former generally gets around this silently by the clock
> > > > > > driver setting the closest frequency it can support.
> > > > >
> > > > > Wouldn't the former actually cause problems, because the closest
> > > > > frequency the clock driver can support could be pretty far from the
> > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > > resulting frequency anyway.
> > > >
> > > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > > frequency wouldn't be far off.
> > >
> > > assigned-clock-rates is really fragile... There's zero guarantee on
> > > how far the actual rate is going to be from the asked one, but more
> > > importantly you have zero guarantee on the time frame that rate is
> > > going to be enforced for.
> >
> > Is there such a guarantee if clk_set_rate() is called?
> 
> with clk_set_rate itself, no, but...
> 
> > > It's simply going to change the rate as a one-off thing, and if
> > > there's the next millisecond someone else is going to change its rate
> > > one way or another, it's going to do so and you won't have any
> > > notification.
> 
> You can get notified, and you can use clk_set_rate_exclusive if you
> *really* want to enforce it.

Is the conclusion then we should go back to relying on the clock-frequency
property?

This has been discussed multiple times over the years, and I don't really
disagree with the above. The frequency is typically indeed hand-picked for
the hardware, and no other frequency should be used in any circumstances.

No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
they should. The absence of practical problems has been probably because of
two factors; firstly, these are typically clocks dedicated to the sensors
and secondly, good luck.

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Cc'ing Laurent as well.

On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote:
> Hi Maxime,
> 
> On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:
> > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > >
> > > > > > > > > Hi Robert,
> > > > > > > > >
> > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > > > >
> > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > > > >
> > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > > > >
> > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > > > are available.
> > > > > > > > > > >
> > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > > > particular SoC / board to be functional?
> > > > > > > > > > >
> > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > > > of the binding should be made clearer as well.
> > > > > > > > > >
> > > > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > > > >
> > > > > > > > > There's a separate discussion on the same topic here:
> > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > > > >
> > > > > > > > Thanks for the link.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > ACPI:
> > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > >
> > > > > > > > > > DT:
> > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > >   - Get xvclk clock
> > > > > > > > > >   - Get xvclk clock rate
> > > > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > > > >
> > > > > > > > > The current status is that you should
> > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > > > between steps 3 and 4
> > > > > > > >
> > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > > > I imagine that would cause some breakage.
> > > > > > >
> > > > > > > It would, yes, and it would be no more correct on DT either.
> > > > > > >
> > > > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > > > the frequency instead.
> > > > > > >
> > > > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > > > to figure out the register list set that closest matches the frequency
> > > > > > > obtained. The former generally gets around this silently by the clock
> > > > > > > driver setting the closest frequency it can support.
> > > > > >
> > > > > > Wouldn't the former actually cause problems, because the closest
> > > > > > frequency the clock driver can support could be pretty far from the
> > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > > > resulting frequency anyway.
> > > > >
> > > > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > > > frequency wouldn't be far off.
> > > >
> > > > assigned-clock-rates is really fragile... There's zero guarantee on
> > > > how far the actual rate is going to be from the asked one, but more
> > > > importantly you have zero guarantee on the time frame that rate is
> > > > going to be enforced for.
> > >
> > > Is there such a guarantee if clk_set_rate() is called?
> > 
> > with clk_set_rate itself, no, but...
> > 
> > > > It's simply going to change the rate as a one-off thing, and if
> > > > there's the next millisecond someone else is going to change its rate
> > > > one way or another, it's going to do so and you won't have any
> > > > notification.
> > 
> > You can get notified, and you can use clk_set_rate_exclusive if you
> > *really* want to enforce it.
> 
> Is the conclusion then we should go back to relying on the clock-frequency
> property?
> 
> This has been discussed multiple times over the years, and I don't really
> disagree with the above. The frequency is typically indeed hand-picked for
> the hardware, and no other frequency should be used in any circumstances.
> 
> No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
> they should. The absence of practical problems has been probably because of
> two factors; firstly, these are typically clocks dedicated to the sensors
> and secondly, good luck.
> 
> -- 
> Regards,
> 
> Sakari Ailus

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote:
> On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote:
> > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:

...

> > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
> > they should. The absence of practical problems has been probably because of
> > two factors; firstly, these are typically clocks dedicated to the sensors
> > and secondly, good luck.

As I heard in another thread clk_*_exclusive() is quite a big hammer with a lot
of side effects and if it can be avoided, it must be avoided.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 7764 bytes --]

On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote:
> Cc'ing Laurent as well.
>
> On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote:
> > Hi Maxime,
> >
> > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:
> > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > > > > >
> > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Robert,
> > > > > > > > > >
> > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > > > > >
> > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > > > > >
> > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > > > > are available.
> > > > > > > > > > > >
> > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > > > > particular SoC / board to be functional?
> > > > > > > > > > > >
> > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > > > > of the binding should be made clearer as well.
> > > > > > > > > > >
> > > > > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > > > > >
> > > > > > > > > > There's a separate discussion on the same topic here:
> > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > > > > >
> > > > > > > > > Thanks for the link.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > ACPI:
> > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > >
> > > > > > > > > > > DT:
> > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > >   - Get xvclk clock
> > > > > > > > > > >   - Get xvclk clock rate
> > > > > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > > > > >
> > > > > > > > > > The current status is that you should
> > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > > > > between steps 3 and 4
> > > > > > > > >
> > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > > > > I imagine that would cause some breakage.
> > > > > > > >
> > > > > > > > It would, yes, and it would be no more correct on DT either.
> > > > > > > >
> > > > > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > > > > the frequency instead.
> > > > > > > >
> > > > > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > > > > to figure out the register list set that closest matches the frequency
> > > > > > > > obtained. The former generally gets around this silently by the clock
> > > > > > > > driver setting the closest frequency it can support.
> > > > > > >
> > > > > > > Wouldn't the former actually cause problems, because the closest
> > > > > > > frequency the clock driver can support could be pretty far from the
> > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > > > > resulting frequency anyway.
> > > > > >
> > > > > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > > > > frequency wouldn't be far off.
> > > > >
> > > > > assigned-clock-rates is really fragile... There's zero guarantee on
> > > > > how far the actual rate is going to be from the asked one, but more
> > > > > importantly you have zero guarantee on the time frame that rate is
> > > > > going to be enforced for.
> > > >
> > > > Is there such a guarantee if clk_set_rate() is called?
> > >
> > > with clk_set_rate itself, no, but...
> > >
> > > > > It's simply going to change the rate as a one-off thing, and if
> > > > > there's the next millisecond someone else is going to change its rate
> > > > > one way or another, it's going to do so and you won't have any
> > > > > notification.
> > >
> > > You can get notified, and you can use clk_set_rate_exclusive if you
> > > *really* want to enforce it.
> >
> > Is the conclusion then we should go back to relying on the clock-frequency
> > property?

clock-frequency or link-frequencies. link-frequencies seems to be a
better fit here, but we don't really have the choice for older
bindings.

> > This has been discussed multiple times over the years, and I don't really
> > disagree with the above. The frequency is typically indeed hand-picked for
> > the hardware, and no other frequency should be used in any circumstances.
> >
> > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
> > they should. The absence of practical problems has been probably because of
> > two factors; firstly, these are typically clocks dedicated to the sensors
> > and secondly, good luck.

My point was that at least with handling the clock rate within the
driver (as opposed to assigned-clock-rates) you have multiple options
in dealing with changing colck rates / parents (Modelling the sensor
clock as a clock itself, using clk_set_rate_exclusive, using a
notifier, etc).. Some are more intrusive to the rest of the system
than others (especially clk_set_rate_exclusive), so I'm not really
advocating for any here, but we should make sure we have them in the
first place.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

On Wed, 15 Apr 2020 at 12:18, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote:
> > Cc'ing Laurent as well.
> >
> > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote:
> > > Hi Maxime,
> > >
> > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:
> > > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> > > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Robert,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > > > > > are available.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > > > > > particular SoC / board to be functional?
> > > > > > > > > > > > >
> > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > > > > > of the binding should be made clearer as well.
> > > > > > > > > > > >
> > > > > > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > > > > > >
> > > > > > > > > > > There's a separate discussion on the same topic here:
> > > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > > > > > >
> > > > > > > > > > Thanks for the link.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > ACPI:
> > > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > > >
> > > > > > > > > > > > DT:
> > > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > > >   - Get xvclk clock
> > > > > > > > > > > >   - Get xvclk clock rate
> > > > > > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > > > > > >
> > > > > > > > > > > The current status is that you should
> > > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > > > > > between steps 3 and 4
> > > > > > > > > >
> > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > > > > > I imagine that would cause some breakage.
> > > > > > > > >
> > > > > > > > > It would, yes, and it would be no more correct on DT either.
> > > > > > > > >
> > > > > > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > > > > > the frequency instead.
> > > > > > > > >
> > > > > > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > > > > > to figure out the register list set that closest matches the frequency
> > > > > > > > > obtained. The former generally gets around this silently by the clock
> > > > > > > > > driver setting the closest frequency it can support.
> > > > > > > >
> > > > > > > > Wouldn't the former actually cause problems, because the closest
> > > > > > > > frequency the clock driver can support could be pretty far from the
> > > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > > > > > resulting frequency anyway.
> > > > > > >
> > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > > > > > frequency wouldn't be far off.
> > > > > >
> > > > > > assigned-clock-rates is really fragile... There's zero guarantee on
> > > > > > how far the actual rate is going to be from the asked one, but more
> > > > > > importantly you have zero guarantee on the time frame that rate is
> > > > > > going to be enforced for.
> > > > >
> > > > > Is there such a guarantee if clk_set_rate() is called?
> > > >
> > > > with clk_set_rate itself, no, but...
> > > >
> > > > > > It's simply going to change the rate as a one-off thing, and if
> > > > > > there's the next millisecond someone else is going to change its rate
> > > > > > one way or another, it's going to do so and you won't have any
> > > > > > notification.
> > > >
> > > > You can get notified, and you can use clk_set_rate_exclusive if you
> > > > *really* want to enforce it.
> > >
> > > Is the conclusion then we should go back to relying on the clock-frequency
> > > property?
>
> clock-frequency or link-frequencies. link-frequencies seems to be a
> better fit here, but we don't really have the choice for older
> bindings.

Alright, so since this is a new binding, let's aim for link-frequencies then.

I don't think I understand what they look like on the driver side.
Do you know an example of what a link-frequencies based driver would look like?

>
> > > This has been discussed multiple times over the years, and I don't really
> > > disagree with the above. The frequency is typically indeed hand-picked for
> > > the hardware, and no other frequency should be used in any circumstances.
> > >
> > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
> > > they should. The absence of practical problems has been probably because of
> > > two factors; firstly, these are typically clocks dedicated to the sensors
> > > and secondly, good luck.
>
> My point was that at least with handling the clock rate within the
> driver (as opposed to assigned-clock-rates) you have multiple options
> in dealing with changing colck rates / parents (Modelling the sensor
> clock as a clock itself, using clk_set_rate_exclusive, using a
> notifier, etc).. Some are more intrusive to the rest of the system
> than others (especially clk_set_rate_exclusive), so I'm not really
> advocating for any here, but we should make sure we have them in the
> first place.
>
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

On Wed, Apr 15, 2020 at 12:18:27PM +0200, Maxime Ripard wrote:
> On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote:
> > Cc'ing Laurent as well.
> >
> > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote:
> > > Hi Maxime,
> > >
> > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:
> > > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> > > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Robert,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > > > > > are available.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > > > > > particular SoC / board to be functional?
> > > > > > > > > > > > >
> > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > > > > > of the binding should be made clearer as well.
> > > > > > > > > > > >
> > > > > > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > > > > > >
> > > > > > > > > > > There's a separate discussion on the same topic here:
> > > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > > > > > >
> > > > > > > > > > Thanks for the link.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > ACPI:
> > > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > > >
> > > > > > > > > > > > DT:
> > > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > > >   - Get xvclk clock
> > > > > > > > > > > >   - Get xvclk clock rate
> > > > > > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > > > > > >
> > > > > > > > > > > The current status is that you should
> > > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > > > > > between steps 3 and 4
> > > > > > > > > >
> > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > > > > > I imagine that would cause some breakage.
> > > > > > > > >
> > > > > > > > > It would, yes, and it would be no more correct on DT either.
> > > > > > > > >
> > > > > > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > > > > > the frequency instead.
> > > > > > > > >
> > > > > > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > > > > > to figure out the register list set that closest matches the frequency
> > > > > > > > > obtained. The former generally gets around this silently by the clock
> > > > > > > > > driver setting the closest frequency it can support.
> > > > > > > >
> > > > > > > > Wouldn't the former actually cause problems, because the closest
> > > > > > > > frequency the clock driver can support could be pretty far from the
> > > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > > > > > resulting frequency anyway.
> > > > > > >
> > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > > > > > frequency wouldn't be far off.
> > > > > >
> > > > > > assigned-clock-rates is really fragile... There's zero guarantee on
> > > > > > how far the actual rate is going to be from the asked one, but more
> > > > > > importantly you have zero guarantee on the time frame that rate is
> > > > > > going to be enforced for.
> > > > >
> > > > > Is there such a guarantee if clk_set_rate() is called?
> > > >
> > > > with clk_set_rate itself, no, but...
> > > >
> > > > > > It's simply going to change the rate as a one-off thing, and if
> > > > > > there's the next millisecond someone else is going to change its rate
> > > > > > one way or another, it's going to do so and you won't have any
> > > > > > notification.
> > > >
> > > > You can get notified, and you can use clk_set_rate_exclusive if you
> > > > *really* want to enforce it.
> > >
> > > Is the conclusion then we should go back to relying on the clock-frequency
> > > property?
> 
> clock-frequency or link-frequencies. link-frequencies seems to be a
> better fit here, but we don't really have the choice for older
> bindings.

You can't replace one with the other as the two are different things. The
clock-frequency refers to the external clock frequency whereas the
link-frequencies refers to the frequencies allowed on the CSI-2 bus.

> 
> > > This has been discussed multiple times over the years, and I don't really
> > > disagree with the above. The frequency is typically indeed hand-picked for
> > > the hardware, and no other frequency should be used in any circumstances.
> > >
> > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
> > > they should. The absence of practical problems has been probably because of
> > > two factors; firstly, these are typically clocks dedicated to the sensors
> > > and secondly, good luck.
> 
> My point was that at least with handling the clock rate within the
> driver (as opposed to assigned-clock-rates) you have multiple options
> in dealing with changing colck rates / parents (Modelling the sensor
> clock as a clock itself, using clk_set_rate_exclusive, using a
> notifier, etc).. Some are more intrusive to the rest of the system
> than others (especially clk_set_rate_exclusive), so I'm not really
> advocating for any here, but we should make sure we have them in the
> first place.

Using a different frequency really should not be allowed. It may be
possible on a development system, hobbyist platform, but never in
production. Therefore the exclusive variant sounds like the right one to
me.

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 9331 bytes --]

On Wed, Apr 15, 2020 at 07:16:16PM +0300, Sakari Ailus wrote:
> On Wed, Apr 15, 2020 at 12:18:27PM +0200, Maxime Ripard wrote:
> > On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote:
> > > Cc'ing Laurent as well.
> > >
> > > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote:
> > > > Hi Maxime,
> > > >
> > > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:
> > > > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> > > > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Robert,
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > > > > > > are available.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > > > > > > particular SoC / board to be functional?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > > > > > > of the binding should be made clearer as well.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > > > > > > >
> > > > > > > > > > > > There's a separate discussion on the same topic here:
> > > > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > > > > > > >
> > > > > > > > > > > Thanks for the link.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > ACPI:
> > > > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > > > >
> > > > > > > > > > > > > DT:
> > > > > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > > > > >   - Get xvclk clock
> > > > > > > > > > > > >   - Get xvclk clock rate
> > > > > > > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > > > > > > >
> > > > > > > > > > > > The current status is that you should
> > > > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > > > > > > between steps 3 and 4
> > > > > > > > > > >
> > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > > > > > > I imagine that would cause some breakage.
> > > > > > > > > >
> > > > > > > > > > It would, yes, and it would be no more correct on DT either.
> > > > > > > > > >
> > > > > > > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > > > > > > the frequency instead.
> > > > > > > > > >
> > > > > > > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > > > > > > to figure out the register list set that closest matches the frequency
> > > > > > > > > > obtained. The former generally gets around this silently by the clock
> > > > > > > > > > driver setting the closest frequency it can support.
> > > > > > > > >
> > > > > > > > > Wouldn't the former actually cause problems, because the closest
> > > > > > > > > frequency the clock driver can support could be pretty far from the
> > > > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > > > > > > resulting frequency anyway.
> > > > > > > >
> > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > > > > > > frequency wouldn't be far off.
> > > > > > >
> > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on
> > > > > > > how far the actual rate is going to be from the asked one, but more
> > > > > > > importantly you have zero guarantee on the time frame that rate is
> > > > > > > going to be enforced for.
> > > > > >
> > > > > > Is there such a guarantee if clk_set_rate() is called?
> > > > >
> > > > > with clk_set_rate itself, no, but...
> > > > >
> > > > > > > It's simply going to change the rate as a one-off thing, and if
> > > > > > > there's the next millisecond someone else is going to change its rate
> > > > > > > one way or another, it's going to do so and you won't have any
> > > > > > > notification.
> > > > >
> > > > > You can get notified, and you can use clk_set_rate_exclusive if you
> > > > > *really* want to enforce it.
> > > >
> > > > Is the conclusion then we should go back to relying on the clock-frequency
> > > > property?
> > 
> > clock-frequency or link-frequencies. link-frequencies seems to be a
> > better fit here, but we don't really have the choice for older
> > bindings.
> 
> You can't replace one with the other as the two are different things. The
> clock-frequency refers to the external clock frequency whereas the
> link-frequencies refers to the frequencies allowed on the CSI-2 bus.

Ack.

> > > > This has been discussed multiple times over the years, and I don't really
> > > > disagree with the above. The frequency is typically indeed hand-picked for
> > > > the hardware, and no other frequency should be used in any circumstances.
> > > >
> > > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
> > > > they should. The absence of practical problems has been probably because of
> > > > two factors; firstly, these are typically clocks dedicated to the sensors
> > > > and secondly, good luck.
> > 
> > My point was that at least with handling the clock rate within the
> > driver (as opposed to assigned-clock-rates) you have multiple options
> > in dealing with changing colck rates / parents (Modelling the sensor
> > clock as a clock itself, using clk_set_rate_exclusive, using a
> > notifier, etc).. Some are more intrusive to the rest of the system
> > than others (especially clk_set_rate_exclusive), so I'm not really
> > advocating for any here, but we should make sure we have them in the
> > first place.
> 
> Using a different frequency really should not be allowed. It may be
> possible on a development system, hobbyist platform, but never in
> production. Therefore the exclusive variant sounds like the right one to
> me.

In all of those cases you would not allow a different frequency. The only
difference is whether you react to a rate change in your parent clock, or
prevent it from happening in the first place.

The latter is easier to do, but has a wider impact on the rest of the system
than the former.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

Hi Maxime,

On Wed, 8 Apr 2020 at 17:30, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Maxime,
>
> On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote:
> > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote:
> > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote:
> > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > >
> > > > > > > > > Hi Robert,
> > > > > > > > >
> > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > > > > > > > > limitation of our implementation, so we can instead implement
> > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > > > > > > > > been rounded too far apart from the frequency we expect).
> > > > > > > > > > > > >
> > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead.
> > > > > > > > > > > >
> > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > > > > > > > > frequency from DT but still sets that frequency is broken.
> > > > > > > > > > > >
> > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > > > > > > > > are available.
> > > > > > > > > > >
> > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > > > > > > > > Hertz", but the frequency at which that clock must run on this
> > > > > > > > > > > particular SoC / board to be functional?
> > > > > > > > > > >
> > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > > > > > > > > of the binding should be made clearer as well.
> > > > > > > > > >
> > > > > > > > > > Alright so, let me summarise the desired approach then.
> > > > > > > > >
> > > > > > > > > There's a separate discussion on the same topic here:
> > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> > > > > > > >
> > > > > > > > Thanks for the link.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > ACPI:
> > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > >
> > > > > > > > > > DT:
> > > > > > > > > >   - Fetch the "clock-frequency" property
> > > > > > > > > >   - Verify it to be 19.2Mhz
> > > > > > > > > >   - Get xvclk clock
> > > > > > > > > >   - Get xvclk clock rate
> > > > > > > > > >   - Verify xvclk clock rate to be 19.2Mhz
> > > > > > > > >
> > > > > > > > > The current status is that you should
> > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace
> > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > > > > > > > > between steps 3 and 4
> > > > > > > >
> > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> > > > > > > > I imagine that would cause some breakage.
> > > > > > >
> > > > > > > It would, yes, and it would be no more correct on DT either.
> > > > > > >
> > > > > > > There are basically two possibilities here; either use the clock-frequency
> > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get
> > > > > > > the frequency instead.
> > > > > > >
> > > > > > > The latter, while I understand it is generally preferred, comes with having
> > > > > > > to figure out the register list set that closest matches the frequency
> > > > > > > obtained. The former generally gets around this silently by the clock
> > > > > > > driver setting the closest frequency it can support.
> > > > > >
> > > > > > Wouldn't the former actually cause problems, because the closest
> > > > > > frequency the clock driver can support could be pretty far from the
> > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the
> > > > > > resulting frequency anyway.
> > > > >
> > > > > That's possible, yes; in this case there wouldn't be a guarantee the
> > > > > frequency wouldn't be far off.
> > > >
> > > > assigned-clock-rates is really fragile... There's zero guarantee on
> > > > how far the actual rate is going to be from the asked one, but more
> > > > importantly you have zero guarantee on the time frame that rate is
> > > > going to be enforced for.
> > >
> > > Is there such a guarantee if clk_set_rate() is called?
> >
> > with clk_set_rate itself, no, but...
> >
> > > > It's simply going to change the rate as a one-off thing, and if
> > > > there's the next millisecond someone else is going to change its rate
> > > > one way or another, it's going to do so and you won't have any
> > > > notification.
> >
> > You can get notified, and you can use clk_set_rate_exclusive if you
> > *really* want to enforce it.
>
> Is the conclusion then we should go back to relying on the clock-frequency
> property?

I too am curious about the conclusion, if we have arrived at one yet.

I sent out v4 of this series yesterday, which used the
'assigned-clock-rates' approach but I would like update the series
with whatever is deemed the best approach.

>
> This has been discussed multiple times over the years, and I don't really
> disagree with the above. The frequency is typically indeed hand-picked for
> the hardware, and no other frequency should be used in any circumstances.
>
> No sensor driver I've seen has used clk_set_rate_exclusive() but I guess
> they should. The absence of practical problems has been probably because of
> two factors; firstly, these are typically clocks dedicated to the sensors
> and secondly, good luck.
>
> --
> Regards,
>
> Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Maxime,

On Tue, Apr 07, 2020 at 02:32:32PM +0200, Maxime Ripard wrote:
> Hi Robert,
> 
> On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > limitation of our implementation, so we can instead implement
> > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > sure that our parent clock is configured at the right rate) and the
> > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > been rounded too far apart from the frequency we expect).
> > > > >
> > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > implementation limitations in the DT, but in the driver instead.
> > > >
> > > > What I really wanted to say that a driver that doesn't get the clock
> > > > frequency from DT but still sets that frequency is broken.
> > > >
> > > > This frequency is highly system specific, and in many cases only a certain
> > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > are available.
> > >
> > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > Hertz", but the frequency at which that clock must run on this
> > > particular SoC / board to be functional?
> > >
> > > If so, then yeah, we should definitely keep it, but the documentation
> > > of the binding should be made clearer as well.
> >
> > Alright so, let me summarise the desired approach then.
> 
> There's a separate discussion on the same topic here:
> https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> 
> > ACPI:
> >   - Fetch the "clock-frequency" property
> >   - Verify it to be 19.2Mhz
> >
> > DT:
> >   - Fetch the "clock-frequency" property
> >   - Verify it to be 19.2Mhz
> >   - Get xvclk clock
> >   - Get xvclk clock rate
> >   - Verify xvclk clock rate to be 19.2Mhz
> 
> The current status is that you should
> 's/clock-frequency/link-frequencies/', and in order to replace
> assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> between steps 3 and 4

The (CSI-2) link frequency is specified in the endpoint, and is already
being read by the V4L2 fwnode framework.

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4595 bytes --]

Hi Robert,

On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote:
> On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote:
> > On Tue, Mar 31, 2020 at 03:33:44PM +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>
> > > ---
> > >
> > > - 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:
> > >   Fixes comments from from Andy, Tomasz, Sakari, Rob.
> > >   * Convert text documentation to YAML schema.
> > >
> > > - Changes since v1:
> > >   Fixes comments from Sakari, Tomasz
> > >   * Add clock-frequency and link-frequencies in DT
> > >
> > >  .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++
> > >  MAINTAINERS                                   |   1 +
> > >  2 files changed, 151 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..beeddfbb8709
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > @@ -0,0 +1,150 @@
> > > +# 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:
> > > +  - Ben Kao <ben.kao@intel.com>
> > > +  - 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.
> >
> > We also had that discussion recently for another omnivision sensor
> > (ov5645 iirc), but what is clock-frequency useful for?
> >
> > It seems that the sensor is passed in clocks, so if you need to
> > retrieve the clock rate you should use the clock API instead.
> >
> > Looking at the driver, it looks like it first retrieves the clock, set
> > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2
> > (19.2 MHz).
>
> As far as I understand it, 19.2MHz is requirement for the sensor mode
> that currently defaults to. Some modes require higher clock speeds
> than this however.
>
> >
> > The datasheet says that the sensor can have any frequency in the 6 -
> > 27 MHz range, so this is a driver limitation and should be set in the
> > driver using the clock API, and you can always bail out if it doesn't
> > provide a rate that is not acceptable for the drivers assumption.
> >
> > In any case, you don't need clock-frequency here...
>
> So your suggestion is that we remove all clocks-rate properties, and
> replace the clk_get_rate() calls in the driver with clk_set_rate()
> calls for the desired frequencies?

Yep, and if you need to make sure that the frequency that your
provider rounded to is matching 19.2MHz like you were doing, then you
can throw a clk_get_rate after it. It seems overly-restrictive to me,
but the device might be picky

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

Add devicetree match table, and enable ov8856_probe()
to initialize power, clocks and reset pins.

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

- 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 | 141 ++++++++++++++++++++++++++++++++-----
 1 file changed, 125 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..260aaf332631 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;
+
+	ret = clk_prepare_enable(ov8856->xvclk);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to enable xvclk\n");
+		return ret;
+	}
+
+	if (is_acpi_node(ov8856->dev->fwnode))
+		return 0;
+
+	if (ov8856->reset_gpio) {
+		gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
+		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, GPIOD_OUT_LOW);
+	usleep_range(1500, 1800);
+
+	return 0;
+
+disable_clk:
+	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
+	clk_disable_unprepare(ov8856->xvclk);
+
+	return ret;
+}
+
+static void __ov8856_power_off(struct ov8856 *ov8856)
+{
+	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
+	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,27 +1155,52 @@ 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;
+	unsigned long xvclk_rate;
 	int ret;
 	unsigned int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
 
-	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
-	if (ret)
-		return ret;
+	ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
+	if (IS_ERR(ov8856->xvclk)) {
+		dev_err(dev, "could not get xvclk clock (%ld)\n",
+			PTR_ERR(ov8856->xvclk));
+		return PTR_ERR(ov8856->xvclk);
+	}
 
-	if (mclk != OV8856_MCLK) {
-		dev_err(dev, "external clock %d is not supported", mclk);
-		return -EINVAL;
+	if (ov8856->xvclk) {
+		xvclk_rate = clk_get_rate(ov8856->xvclk);
+		if (xvclk_rate != OV8856_XVCLK_19_2) {
+			dev_err(dev, "external clock %lu is not supported",
+				xvclk_rate);
+			return -EINVAL;
+		}
+	}
+
+	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+		GPIOD_OUT_HIGH);
+	if (IS_ERR(ov8856->reset_gpio)) {
+		dev_dbg(dev, "failed to get reset-gpio\n");
+		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) {
+		dev_warn(dev, "failed to get regulators\n");
+		return ret;
 	}
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
@@ -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;
+
+	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_warn(&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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote:
>
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.

...

> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> +       int ret;
> +
> +       ret = clk_prepare_enable(ov8856->xvclk);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "failed to enable xvclk\n");
> +               return ret;
> +       }
> +

> +       if (is_acpi_node(ov8856->dev->fwnode))

Use dev_fwnode().

> +               return 0;
> +
> +       if (ov8856->reset_gpio) {

> +               gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);

This is wrong. You have to fix it to use either 0 or 1.

> +               usleep_range(1000, 2000);
> +       }
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> +                                   ov8856->supplies);

> +       if (ret < 0) {

Do you need all ' < 0' parts all over the series?

> +               dev_err(&client->dev, "failed to enable regulators\n");
> +               goto disable_clk;
> +       }

...

> +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);

Ditto.

...

> +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);

Ditto.

...

> +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);

Ditto.

...

> -       ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> -       if (ret)
> -               return ret;

Where is it gone? Why?

> +       ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
> +       if (IS_ERR(ov8856->xvclk)) {

> +               dev_err(dev, "could not get xvclk clock (%ld)\n",
> +                       PTR_ERR(ov8856->xvclk));

Also you may use %pe here and in similar cases.

> +               return PTR_ERR(ov8856->xvclk);
> +       }

> +       ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +               GPIOD_OUT_HIGH);

Here parameter is correct. The question is, what the value should be
HIGH or LOW?
Basically HIGH means to assert the signal.

> +       if (IS_ERR(ov8856->reset_gpio)) {

> +               dev_dbg(dev, "failed to get reset-gpio\n");

Noise.
Enable GPIO debug to see this kind of messages.

> +               return PTR_ERR(ov8856->reset_gpio);
> +       }

...

> +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> +                                     ov8856->supplies);
> +       if (ret) {

> +               dev_warn(dev, "failed to get regulators\n");

If it's a warning, why we return from here?
Same question to all other places with same issue.

> +               return ret;
>         }

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

Hey Andy,

Thanks for the review, it is much appreciated!

On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote:
> >
> > Add devicetree match table, and enable ov8856_probe()
> > to initialize power, clocks and reset pins.
>
> ...
>
> > +static int __ov8856_power_on(struct ov8856 *ov8856)
> > +{
> > +       struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> > +       int ret;
> > +
> > +       ret = clk_prepare_enable(ov8856->xvclk);
> > +       if (ret < 0) {
> > +               dev_err(&client->dev, "failed to enable xvclk\n");
> > +               return ret;
> > +       }
> > +
>
> > +       if (is_acpi_node(ov8856->dev->fwnode))
>
> Use dev_fwnode().

Ack.

>
> > +               return 0;
> > +
> > +       if (ov8856->reset_gpio) {
>
> > +               gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
>
> This is wrong. You have to fix it to use either 0 or 1.

I've changed all gpiod_set_value_cansleep() calls to use 0/1.

>
> > +               usleep_range(1000, 2000);
> > +       }
> > +
> > +       ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names),
> > +                                   ov8856->supplies);
>
> > +       if (ret < 0) {
>
> Do you need all ' < 0' parts all over the series?

Some checks are needed due to ACPI and DT support co-existing.
Maybe it would be better to just split the probing into an ACPI path
and a DT path.

I'll have a look through the series for redundant retval checks.

>
> > +               dev_err(&client->dev, "failed to enable regulators\n");
> > +               goto disable_clk;
> > +       }
>
> ...
>
> > +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW);
>
> Ditto.

Ack.

>
> ...
>
> > +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
>
> Ditto.

Ack.

>
> ...
>
> > +       gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
>
> Ditto.

Ack.

>
> ...
>
> > -       ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > -       if (ret)
> > -               return ret;
>
> Where is it gone? Why?

It was replaced by a clk_get_rate call, which as Sakari pointed out,
isn't correct.
I'll rework the clock handling for v4.

>
> > +       ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
> > +       if (IS_ERR(ov8856->xvclk)) {
>
> > +               dev_err(dev, "could not get xvclk clock (%ld)\n",
> > +                       PTR_ERR(ov8856->xvclk));
>
> Also you may use %pe here and in similar cases.

Weirdly checkpatch complains about this.
But it builds and runs cleanly, so I'll add it in v4.

>
> > +               return PTR_ERR(ov8856->xvclk);
> > +       }
>
> > +       ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +               GPIOD_OUT_HIGH);
>
> Here parameter is correct. The question is, what the value should be
> HIGH or LOW?
> Basically HIGH means to assert the signal.

Ack, I'll invert the logic.

>
> > +       if (IS_ERR(ov8856->reset_gpio)) {
>
> > +               dev_dbg(dev, "failed to get reset-gpio\n");
>
> Noise.
> Enable GPIO debug to see this kind of messages.

Ack.

>
> > +               return PTR_ERR(ov8856->reset_gpio);
> > +       }
>
> ...
>
> > +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> > +                                     ov8856->supplies);
> > +       if (ret) {
>
> > +               dev_warn(dev, "failed to get regulators\n");
>
> If it's a warning, why we return from here?
> Same question to all other places with same issue.

The issue I was seeing was the driver having to return a EDEFER here,
so this warning sheds some light on which exact component is returning
an EDEFER.

[   15.962623] ov8856 16-0010: Dropping the link to regulator.29
[   15.968464] ov8856 16-0010: failed to get regulators
[   15.973493] ov8856 16-0010: failed to get HW configuration: -517
[   15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc
[   15.985855] ov8856 16-0010: genpd_remove_device()
[   15.990672] i2c 16-0010: Driver ov8856 requests probe deferral

Personally I found it helpful to speed up debugging, but I'll happily
remove it if you prefer no warning.

>
> > +               return ret;
> >         }
>
> --
> With Best Regards,
> Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Mon, Apr 06, 2020 at 03:37:24PM +0200, Robert Foss wrote:
> On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote:

...

> > > +       if (ret < 0) {
> >
> > Do you need all ' < 0' parts all over the series?
> 
> Some checks are needed due to ACPI and DT support co-existing.
> Maybe it would be better to just split the probing into an ACPI path
> and a DT path.
> 
> I'll have a look through the series for redundant retval checks.

Drop where it is redundant.

...

> > > -       ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > -       if (ret)
> > > -               return ret;
> >
> > Where is it gone? Why?
> 
> It was replaced by a clk_get_rate call, which as Sakari pointed out,
> isn't correct.
> I'll rework the clock handling for v4.

If it was in the driver it should stay -- properties is an ABI (between firmware and kernel).

> > > +       ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
> > > +       if (IS_ERR(ov8856->xvclk)) {
> >
> > > +               dev_err(dev, "could not get xvclk clock (%ld)\n",
> > > +                       PTR_ERR(ov8856->xvclk));
> >
> > Also you may use %pe here and in similar cases.
> 
> Weirdly checkpatch complains about this.
> But it builds and runs cleanly, so I'll add it in v4.

%pe requires pointer, PTR_ERR converts pointer to integer.

...

> > > +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> > > +                                     ov8856->supplies);
> > > +       if (ret) {
> >
> > > +               dev_warn(dev, "failed to get regulators\n");
> >
> > If it's a warning, why we return from here?
> > Same question to all other places with same issue.
> 
> The issue I was seeing was the driver having to return a EDEFER here,
> so this warning sheds some light on which exact component is returning
> an EDEFER.
> 
> [   15.962623] ov8856 16-0010: Dropping the link to regulator.29
> [   15.968464] ov8856 16-0010: failed to get regulators
> [   15.973493] ov8856 16-0010: failed to get HW configuration: -517
> [   15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc
> [   15.985855] ov8856 16-0010: genpd_remove_device()
> [   15.990672] i2c 16-0010: Driver ov8856 requests probe deferral
> 
> Personally I found it helpful to speed up debugging, but I'll happily
> remove it if you prefer no warning.

My point is that you have it in align:
 - if it is an error, print as an error and bail out, otherwise
 - if it is a warning, print it and continue.

> > > +               return ret;
> > >         }

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Robert Foss]

On Mon, 6 Apr 2020 at 17:06, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Mon, Apr 06, 2020 at 03:37:24PM +0200, Robert Foss wrote:
> > On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote:
>
> ...
>
> > > > +       if (ret < 0) {
> > >
> > > Do you need all ' < 0' parts all over the series?
> >
> > Some checks are needed due to ACPI and DT support co-existing.
> > Maybe it would be better to just split the probing into an ACPI path
> > and a DT path.
> >
> > I'll have a look through the series for redundant retval checks.
>
> Drop where it is redundant.
>
> ...
>
> > > > -       ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > > -       if (ret)
> > > > -               return ret;
> > >
> > > Where is it gone? Why?
> >
> > It was replaced by a clk_get_rate call, which as Sakari pointed out,
> > isn't correct.
> > I'll rework the clock handling for v4.
>
> If it was in the driver it should stay -- properties is an ABI (between firmware and kernel).

Ack.

>
> > > > +       ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
> > > > +       if (IS_ERR(ov8856->xvclk)) {
> > >
> > > > +               dev_err(dev, "could not get xvclk clock (%ld)\n",
> > > > +                       PTR_ERR(ov8856->xvclk));
> > >
> > > Also you may use %pe here and in similar cases.
> >
> > Weirdly checkpatch complains about this.
> > But it builds and runs cleanly, so I'll add it in v4.
>
> %pe requires pointer, PTR_ERR converts pointer to integer.

Ack.

>
> ...
>
> > > > +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> > > > +                                     ov8856->supplies);
> > > > +       if (ret) {
> > >
> > > > +               dev_warn(dev, "failed to get regulators\n");
> > >
> > > If it's a warning, why we return from here?
> > > Same question to all other places with same issue.
> >
> > The issue I was seeing was the driver having to return a EDEFER here,
> > so this warning sheds some light on which exact component is returning
> > an EDEFER.
> >
> > [   15.962623] ov8856 16-0010: Dropping the link to regulator.29
> > [   15.968464] ov8856 16-0010: failed to get regulators
> > [   15.973493] ov8856 16-0010: failed to get HW configuration: -517
> > [   15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc
> > [   15.985855] ov8856 16-0010: genpd_remove_device()
> > [   15.990672] i2c 16-0010: Driver ov8856 requests probe deferral
> >
> > Personally I found it helpful to speed up debugging, but I'll happily
> > remove it if you prefer no warning.
>
> My point is that you have it in align:
>  - if it is an error, print as an error and bail out, otherwise
>  - if it is a warning, print it and continue.

I see what you're saying now, let's remove it then :)
I guess in the specific case of EDEFER, it doesn't fit neatly into
either of those categories, in the sense that the way you continue is
to return and then try to probe again later.

There are some other locations where this is handled wrong, I'll align
them properly for v4.

>
> > > > +               return ret;
> > > >         }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Robert,

On Tue, Mar 31, 2020 at 03:33:45PM +0200, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
> 
> - 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 | 141 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 125 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..260aaf332631 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;
> +
> +	ret = clk_prepare_enable(ov8856->xvclk);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable xvclk\n");
> +		return ret;
> +	}
> +
> +	if (is_acpi_node(ov8856->dev->fwnode))
> +		return 0;
> +
> +	if (ov8856->reset_gpio) {
> +		gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> +		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, GPIOD_OUT_LOW);
> +	usleep_range(1500, 1800);
> +
> +	return 0;
> +
> +disable_clk:
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> +	clk_disable_unprepare(ov8856->xvclk);
> +
> +	return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> +	gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> +	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,27 +1155,52 @@ 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;
> +	unsigned long xvclk_rate;
>  	int ret;
>  	unsigned int i, j;
>  
>  	if (!fwnode)
>  		return -ENXIO;
>  
> -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> -	if (ret)
> -		return ret;

You'll still need to continue reading the clock-frequency property on ACPI
systems, and assume that frequency (as you won't have the clock object).

> +	ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
> +	if (IS_ERR(ov8856->xvclk)) {
> +		dev_err(dev, "could not get xvclk clock (%ld)\n",
> +			PTR_ERR(ov8856->xvclk));
> +		return PTR_ERR(ov8856->xvclk);
> +	}
>  
> -	if (mclk != OV8856_MCLK) {
> -		dev_err(dev, "external clock %d is not supported", mclk);
> -		return -EINVAL;
> +	if (ov8856->xvclk) {
> +		xvclk_rate = clk_get_rate(ov8856->xvclk);
> +		if (xvclk_rate != OV8856_XVCLK_19_2) {
> +			dev_err(dev, "external clock %lu is not supported",
> +				xvclk_rate);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +		GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov8856->reset_gpio)) {
> +		dev_dbg(dev, "failed to get reset-gpio\n");
> +		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) {
> +		dev_warn(dev, "failed to get regulators\n");
> +		return ret;
>  	}
>  
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> @@ -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;
> +
> +	ov8856->dev = &client->dev;

What happened here? :-)

I think this patch is starting to look quite reasonable.

> +	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_warn(&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,

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 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 only the '1B' revision has been added.

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

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


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

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 260aaf332631..c7551cee2bb0 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -32,6 +32,18 @@
 #define OV8856_MODE_STANDBY		0x00
 #define OV8856_MODE_STREAMING		0x01
 
+/* define 1B module revision */
+#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 +1164,45 @@ 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_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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel