Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1672724 > unrolled thread

[PATCH v1 0/6] Add support of OV9655 camera

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 0/6] Add support of OV9655 camera Hugues Fruchet <hugues.fruchet@st.com> - 2017-06-22 17:10 +0200
    [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Hugues Fruchet <hugues.fruchet@st.com> - 2017-06-22 17:10 +0200
      Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-23 12:30 +0200
        Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Andreas Färber <afaerber@suse.de> - 2017-06-23 12:50 +0200
          Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-23 13:10 +0200
            Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-06-23 14:00 +0200
              Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-23 17:00 +0200
                Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Andreas Färber <afaerber@suse.de> - 2017-06-23 17:00 +0200
                  Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-23 17:30 +0200
                    Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Suman Anna <s-anna@ti.com> - 2017-06-23 20:10 +0200
                      Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-23 21:10 +0200
                        Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Suman Anna <s-anna@ti.com> - 2017-06-24 00:30 +0200
                          Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-26 08:10 +0200
        Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Hugues FRUCHET <hugues.fruchet@st.com> - 2017-06-26 12:40 +0200
          Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Sylwester Nawrocki <snawrocki@kernel.org> - 2017-06-26 22:10 +0200
            Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-27 07:50 +0200
              Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Sylwester Nawrocki <snawrocki@kernel.org> - 2017-06-28 01:00 +0200
                Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-28 11:20 +0200
                  Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera  module Sylwester Nawrocki <s.nawrocki@samsung.com> - 2017-06-28 13:00 +0200
                    Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-28 13:30 +0200
                      Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Hugues FRUCHET <hugues.fruchet@st.com> - 2017-06-28 14:30 +0200
        Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Rob Herring <robh@kernel.org> - 2017-06-26 21:00 +0200
      Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module Rob Herring <robh@kernel.org> - 2017-06-26 21:00 +0200
    [PATCH v1 2/6] [media] ov9650: add device tree support Hugues Fruchet <hugues.fruchet@st.com> - 2017-06-22 17:10 +0200
      Re: [PATCH v1 2/6] [media] ov9650: add device tree support Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-26 18:40 +0200
        Re: [PATCH v1 2/6] [media] ov9650: add device tree support "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-26 19:50 +0200
          Re: [PATCH v1 2/6] [media] ov9650: add device tree support Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-27 07:40 +0200
            Re: [PATCH v1 2/6] [media] ov9650: add device tree support Hugues FRUCHET <hugues.fruchet@st.com> - 2017-06-27 12:20 +0200
    [PATCH v1 4/6] [media] ov9650: use write_array() for resolution sequences Hugues Fruchet <hugues.fruchet@st.com> - 2017-06-22 17:10 +0200
      Re: [PATCH v1 4/6] [media] ov9650: use write_array() for resolution  sequences Sakari Ailus <sakari.ailus@iki.fi> - 2017-06-26 18:40 +0200
        Re: [PATCH v1 4/6] [media] ov9650: use write_array() for resolution  sequences Hugues FRUCHET <hugues.fruchet@st.com> - 2017-06-29 16:10 +0200
    [PATCH v1 3/6] [media] ov9650: select the nearest higher resolution Hugues Fruchet <hugues.fruchet@st.com> - 2017-06-22 17:10 +0200
    [PATCH v1 6/6] [media] ov9650: add support of OV9655 variant Hugues Fruchet <hugues.fruchet@st.com> - 2017-06-22 17:10 +0200
      Re: [PATCH v1 6/6] [media] ov9650: add support of OV9655 variant kbuild test robot <lkp@intel.com> - 2017-06-25 18:10 +0200
      [PATCH] ov9650: fix semicolon.cocci warnings kbuild test robot <lkp@intel.com> - 2017-06-25 18:10 +0200
      Re: [PATCH v1 6/6] [media] ov9650: add support of OV9655 variant "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-26 08:10 +0200
    Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-22 17:50 +0200
      Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-23 12:30 +0200
        omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655 camera Pavel Machek <pavel@ucw.cz> - 2017-06-25 11:20 +0200
          Re: omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-26 08:10 +0200
            Re: omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655  camera Pavel Machek <pavel@ucw.cz> - 2017-06-26 10:40 +0200
              Re: omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-26 12:00 +0200
                Re: omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655  camera Pavel Machek <pavel@ucw.cz> - 2017-06-26 13:20 +0200
                  Re: omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-27 08:00 +0200
              Re: omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655  camera Hugues FRUCHET <hugues.fruchet@st.com> - 2017-06-26 15:30 +0200
                Re: omap3isp camera was Re: [PATCH v1 0/6] Add support of OV9655  camera Hugues FRUCHET <hugues.fruchet@st.com> - 2017-06-27 10:00 +0200
                  Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-07-01 23:10 +0200
                    Re: [PATCH v1 0/6] Add support of OV9655 camera Hugues FRUCHET <hugues.fruchet@st.com> - 2017-07-03 10:20 +0200
                      Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-07-03 11:20 +0200
                        Re: [PATCH v1 0/6] Add support of OV9655 camera Hugues FRUCHET <hugues.fruchet@st.com> - 2017-07-03 14:10 +0200
                          Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-07-03 14:30 +0200
        Re: [PATCH v1 0/6] Add support of OV9655 camera "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-26 08:10 +0200
      Re: [PATCH v1 0/6] Add support of OV9655 camera Hugues FRUCHET <hugues.fruchet@st.com> - 2017-06-26 12:10 +0200

Page 1 of 3  [1] 2 3  Next page →

#1672724 — [PATCH v1 0/6] Add support of OV9655 camera

This patchset enables OV9655 camera support.

OV9655 support has been tested using STM32F4DIS-CAM extension board
plugged on connector P1 of STM32F746G-DISCO board.
Due to lack of OV9650/52 hardware support, the modified related code
could not have been checked for non-regression.

First patches upgrade current support of OV9650/52 to prepare then
introduction of OV9655 variant patch.
Because of OV9655 register set slightly different from OV9650/9652,
not all of the driver features are supported (controls). Supported
resolutions are limited to VGA, QVGA, QQVGA.
Supported format is limited to RGB565.
Controls are limited to color bar test pattern for test purpose.

OV9655 initial support is based on a driver written by H. Nikolaus Schaller [1].
OV9655 registers sequences come from STM32CubeF7 embedded software [2].

[1] http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
[2] https://developer.mbed.org/teams/ST/code/BSP_DISCO_F746NG/file/e1d9da7fe856/Drivers/BSP/Components/ov9655/ov9655.c

===========
= history =
===========
version 1:
  - Initial submission.

H. Nikolaus Schaller (1):
  DT bindings: add bindings for ov965x camera module

Hugues Fruchet (5):
  [media] ov9650: add device tree support
  [media] ov9650: select the nearest higher resolution
  [media] ov9650: use write_array() for resolution sequences
  [media] ov9650: add multiple variant support
  [media] ov9650: add support of OV9655 variant

 .../devicetree/bindings/media/i2c/ov965x.txt       |  37 +
 drivers/media/i2c/Kconfig                          |   6 +-
 drivers/media/i2c/ov9650.c                         | 792 +++++++++++++++++----
 3 files changed, 704 insertions(+), 131 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

-- 
1.9.1

[toc] | [next] | [standalone]

#1672725 — [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

From: "H. Nikolaus Schaller" <hns@goldelico.com>

This adds documentation of device tree bindings
for the OV965X family camera sensor module.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 .../devicetree/bindings/media/i2c/ov965x.txt       | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
new file mode 100644
index 0000000..0e0de1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
@@ -0,0 +1,37 @@
+* Omnivision OV9650/9652/9655 CMOS sensor
+
+The Omnivision OV965x sensor support multiple resolutions output, such as
+CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
+output format.
+
+Required Properties:
+- compatible: should be one of
+	"ovti,ov9650"
+	"ovti,ov9652"
+	"ovti,ov9655"
+- clocks: reference to the mclk input clock.
+
+Optional Properties:
+- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
+- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
+
+The device node must contain one 'port' child node for its digital output
+video port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+&i2c2 {
+	ov9655: camera@30 {
+		compatible = "ovti,ov9655";
+		reg = <0x30>;
+		pwdn-gpios = <&gpioh 13 GPIO_ACTIVE_HIGH>;
+		clocks = <&clk_ext_camera>;
+
+		port {
+			ov9655: endpoint {
+				remote-endpoint = <&dcmi_0>;
+			};
+		};
+	};
+};
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1673461 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Hi Hugues,

> Am 22.06.2017 um 17:05 schrieb Hugues Fruchet <hugues.fruchet@st.com>:
> 
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
> 
> This adds documentation of device tree bindings
> for the OV965X family camera sensor module.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
> .../devicetree/bindings/media/i2c/ov965x.txt       | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
> new file mode 100644
> index 0000000..0e0de1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
> @@ -0,0 +1,37 @@
> +* Omnivision OV9650/9652/9655 CMOS sensor
> +
> +The Omnivision OV965x sensor support multiple resolutions output, such as
> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> +output format.
> +
> +Required Properties:
> +- compatible: should be one of
> +	"ovti,ov9650"
> +	"ovti,ov9652"
> +	"ovti,ov9655"
> +- clocks: reference to the mclk input clock.

I wonder why you have removed the clock-frequency property?

In some situations the camera driver must be able to tell the clock source
which frequency it wants to see.

For example we connect the camera to an OMAP3-ISP (image signal processor) and
there it is assumed that camera modules know the frequency and set the clock, e.g.:

http://elixir.free-electrons.com/linux/v4.4/source/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt#L52
http://elixir.free-electrons.com/linux/v3.14/source/Documentation/devicetree/bindings/media/i2c/mt9p031.txt

If your clock is constant and defined elsewhere we should make this
property optional instead of required. But it should not be missing.

Here is a hack to get it into your code:

http://git.goldelico.com/?p=gta04-kernel.git;a=blobdiff;f=drivers/media/i2c/ov9650.c;h=b7ab46c775b9e40087e427ae0777e9f7c283694a;hp=1846bcbb19ae71ce686dade320aa06ce2e429ca4;hb=ca85196f6fd9a77e5a0f796aeaf7aa2cde60ce91;hpb=8a71f21b75543a6d99102be1ae4677b28c478ac9

> +
> +Optional Properties:
> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.

Here I wonder why you did split that up into two gpios. Each "*-gpios" can have
multiple entries and if one is not used, a 0 can be specified to make it being ignored.

But it is up to DT maintainers what they prefer: separate single gpios or a single gpio array.


What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
So the driver does not power up the camera module when needed and therefore probing fails.

  - vana-supply: a regulator to power up the camera module.

Driver code is not complex to add:

http://git.goldelico.com/?p=gta04-kernel.git;a=blobdiff;f=drivers/media/i2c/ov9650.c;h=1846bcbb19ae71ce686dade320aa06ce2e429ca4;hp=c0819afdcefcb19da351741d51dad00aaf909254;hb=8a71f21b75543a6d99102be1ae4677b28c478ac9;hpb=6db55fc472eea2ec6db03833df027aecf6649f88

> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> +&i2c2 {
> +	ov9655: camera@30 {
> +		compatible = "ovti,ov9655";
> +		reg = <0x30>;
> +		pwdn-gpios = <&gpioh 13 GPIO_ACTIVE_HIGH>;
> +		clocks = <&clk_ext_camera>;
> +
> +		port {
> +			ov9655: endpoint {
> +				remote-endpoint = <&dcmi_0>;
> +			};
> +		};
> +	};
> +};
> -- 
> 1.9.1
> 

BR and thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1673472 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Hi,

Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>> new file mode 100644
>> index 0000000..0e0de1f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>> @@ -0,0 +1,37 @@
>> +* Omnivision OV9650/9652/9655 CMOS sensor
>> +
>> +The Omnivision OV965x sensor support multiple resolutions output, such as
>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>> +output format.
>> +
>> +Required Properties:
>> +- compatible: should be one of
>> +	"ovti,ov9650"
>> +	"ovti,ov9652"
>> +	"ovti,ov9655"
>> +- clocks: reference to the mclk input clock.
> 
> I wonder why you have removed the clock-frequency property?
> 
> In some situations the camera driver must be able to tell the clock source
> which frequency it wants to see.

That's what assigned-clock-rates property is for:

https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-bindings.txt

AFAIU clock-frequency on devices is deprecated and equivalent to having
a clocks property pointing to a fixed-clock, which is different from a
clock with varying rate.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1673476 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

> Am 23.06.2017 um 12:46 schrieb Andreas Färber <afaerber@suse.de>:
> 
> Hi,
> 
> Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>> new file mode 100644
>>> index 0000000..0e0de1f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>> @@ -0,0 +1,37 @@
>>> +* Omnivision OV9650/9652/9655 CMOS sensor
>>> +
>>> +The Omnivision OV965x sensor support multiple resolutions output, such as
>>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>>> +output format.
>>> +
>>> +Required Properties:
>>> +- compatible: should be one of
>>> +	"ovti,ov9650"
>>> +	"ovti,ov9652"
>>> +	"ovti,ov9655"
>>> +- clocks: reference to the mclk input clock.
>> 
>> I wonder why you have removed the clock-frequency property?
>> 
>> In some situations the camera driver must be able to tell the clock source
>> which frequency it wants to see.
> 
> That's what assigned-clock-rates property is for:
> 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-bindings.txt
> 
> AFAIU clock-frequency on devices is deprecated and equivalent to having
> a clocks property pointing to a fixed-clock, which is different from a
> clock with varying rate.

I am not sure if that helps here. The OMAP3-ISP does not have a fixed clock rate
so we can only have the driver define what it wants to see.

And common practise for OMAP3-ISP based camera modules (e.g. N900, N9) is that they do it in the driver.

Maybe ISP developers can comment?

BR,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1673497 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Hi Nikolaus,

On Friday 23 Jun 2017 12:59:24 H. Nikolaus Schaller wrote:
> Am 23.06.2017 um 12:46 schrieb Andreas Färber <afaerber@suse.de>:
> > Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt
> >>> b/Documentation/devicetree/bindings/media/i2c/ov965x.txt new file mode
> >>> 100644
> >>> index 0000000..0e0de1f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
> >>> @@ -0,0 +1,37 @@
> >>> +* Omnivision OV9650/9652/9655 CMOS sensor
> >>> +
> >>> +The Omnivision OV965x sensor support multiple resolutions output, such
> >>> as
> >>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> >>> +output format.
> >>> +
> >>> +Required Properties:
> >>> +- compatible: should be one of
> >>> +	"ovti,ov9650"
> >>> +	"ovti,ov9652"
> >>> +	"ovti,ov9655"
> >>> +- clocks: reference to the mclk input clock.
> >> 
> >> I wonder why you have removed the clock-frequency property?
> >> 
> >> In some situations the camera driver must be able to tell the clock
> >> source which frequency it wants to see.
> > 
> > That's what assigned-clock-rates property is for:
> > 
> > https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-b
> > indings.txt
> > 
> > AFAIU clock-frequency on devices is deprecated and equivalent to having
> > a clocks property pointing to a fixed-clock, which is different from a
> > clock with varying rate.
> 
> I am not sure if that helps here. The OMAP3-ISP does not have a fixed clock
> rate so we can only have the driver define what it wants to see.
> 
> And common practise for OMAP3-ISP based camera modules (e.g. N900, N9) is
> that they do it in the driver.
> 
> Maybe ISP developers can comment?

The OMAP3 ISP is a variable-frequency clock provider. The clock frequency is 
controlled by the clock consumer. As such, it's up to the consumer to decide 
whether to compute and request the clock rate dynamically at runtime, or use 
the assigned-clock-rates property in DT.

Some ISPs include a clock generator, others don't. It should make no 
difference whether the clock is provided by the ISP, by a dedicated clock 
source in the SoC or by a discrete on-board adjustable clock source.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1673619 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Hi Laurent,

> Am 23.06.2017 um 13:58 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> Hi Nikolaus,
> 
> On Friday 23 Jun 2017 12:59:24 H. Nikolaus Schaller wrote:
>> Am 23.06.2017 um 12:46 schrieb Andreas Färber <afaerber@suse.de>:
>>> Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>> b/Documentation/devicetree/bindings/media/i2c/ov965x.txt new file mode
>>>>> 100644
>>>>> index 0000000..0e0de1f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>> @@ -0,0 +1,37 @@
>>>>> +* Omnivision OV9650/9652/9655 CMOS sensor
>>>>> +
>>>>> +The Omnivision OV965x sensor support multiple resolutions output, such
>>>>> as
>>>>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>>>>> +output format.
>>>>> +
>>>>> +Required Properties:
>>>>> +- compatible: should be one of
>>>>> +	"ovti,ov9650"
>>>>> +	"ovti,ov9652"
>>>>> +	"ovti,ov9655"
>>>>> +- clocks: reference to the mclk input clock.
>>>> 
>>>> I wonder why you have removed the clock-frequency property?
>>>> 
>>>> In some situations the camera driver must be able to tell the clock
>>>> source which frequency it wants to see.
>>> 
>>> That's what assigned-clock-rates property is for:
>>> 
>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-b
>>> indings.txt
>>> 
>>> AFAIU clock-frequency on devices is deprecated and equivalent to having
>>> a clocks property pointing to a fixed-clock, which is different from a
>>> clock with varying rate.
>> 
>> I am not sure if that helps here. The OMAP3-ISP does not have a fixed clock
>> rate so we can only have the driver define what it wants to see.
>> 
>> And common practise for OMAP3-ISP based camera modules (e.g. N900, N9) is
>> that they do it in the driver.
>> 
>> Maybe ISP developers can comment?
> 
> The OMAP3 ISP is a variable-frequency clock provider. The clock frequency is 
> controlled by the clock consumer. As such, it's up to the consumer to decide 
> whether to compute and request the clock rate dynamically at runtime, or use 
> the assigned-clock-rates property in DT.
> 
> Some ISPs include a clock generator, others don't. It should make no 
> difference whether the clock is provided by the ISP, by a dedicated clock 
> source in the SoC or by a discrete on-board adjustable clock source.

Thanks for explaining the background.

Do you have an hint or example how to use the assigned-clock-rates property in
a DT for a camera module connected to the omap3isp?

Or does it just mean that it defines the property name?

BR,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1673620 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Am 23.06.2017 um 16:53 schrieb H. Nikolaus Schaller:
> Hi Laurent,
> 
>> Am 23.06.2017 um 13:58 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>>
>> Hi Nikolaus,
>>
>> On Friday 23 Jun 2017 12:59:24 H. Nikolaus Schaller wrote:
>>> Am 23.06.2017 um 12:46 schrieb Andreas Färber <afaerber@suse.de>:
>>>> Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>>> b/Documentation/devicetree/bindings/media/i2c/ov965x.txt new file mode
>>>>>> 100644
>>>>>> index 0000000..0e0de1f
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>>> @@ -0,0 +1,37 @@
>>>>>> +* Omnivision OV9650/9652/9655 CMOS sensor
>>>>>> +
>>>>>> +The Omnivision OV965x sensor support multiple resolutions output, such
>>>>>> as
>>>>>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>>>>>> +output format.
>>>>>> +
>>>>>> +Required Properties:
>>>>>> +- compatible: should be one of
>>>>>> +	"ovti,ov9650"
>>>>>> +	"ovti,ov9652"
>>>>>> +	"ovti,ov9655"
>>>>>> +- clocks: reference to the mclk input clock.
>>>>>
>>>>> I wonder why you have removed the clock-frequency property?
>>>>>
>>>>> In some situations the camera driver must be able to tell the clock
>>>>> source which frequency it wants to see.
>>>>
>>>> That's what assigned-clock-rates property is for:
>>>>
>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-b
>>>> indings.txt
>>>>
>>>> AFAIU clock-frequency on devices is deprecated and equivalent to having
>>>> a clocks property pointing to a fixed-clock, which is different from a
>>>> clock with varying rate.
>>>
>>> I am not sure if that helps here. The OMAP3-ISP does not have a fixed clock
>>> rate so we can only have the driver define what it wants to see.
>>>
>>> And common practise for OMAP3-ISP based camera modules (e.g. N900, N9) is
>>> that they do it in the driver.
>>>
>>> Maybe ISP developers can comment?
>>
>> The OMAP3 ISP is a variable-frequency clock provider. The clock frequency is 
>> controlled by the clock consumer. As such, it's up to the consumer to decide 
>> whether to compute and request the clock rate dynamically at runtime, or use 
>> the assigned-clock-rates property in DT.
>>
>> Some ISPs include a clock generator, others don't. It should make no 
>> difference whether the clock is provided by the ISP, by a dedicated clock 
>> source in the SoC or by a discrete on-board adjustable clock source.
> 
> Thanks for explaining the background.
> 
> Do you have an hint or example how to use the assigned-clock-rates property in
> a DT for a camera module connected to the omap3isp?
> 
> Or does it just mean that it defines the property name?

Please read the documentation link I sent - it's in the very bottom and
should have an example.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1673636 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Hi,

> Am 23.06.2017 um 16:57 schrieb Andreas Färber <afaerber@suse.de>:
> 
> Am 23.06.2017 um 16:53 schrieb H. Nikolaus Schaller:
>> Hi Laurent,
>> 
>>> Am 23.06.2017 um 13:58 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> On Friday 23 Jun 2017 12:59:24 H. Nikolaus Schaller wrote:
>>>> Am 23.06.2017 um 12:46 schrieb Andreas Färber <afaerber@suse.de>:
>>>>> Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>>>> b/Documentation/devicetree/bindings/media/i2c/ov965x.txt new file mode
>>>>>>> 100644
>>>>>>> index 0000000..0e0de1f
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>>>> @@ -0,0 +1,37 @@
>>>>>>> +* Omnivision OV9650/9652/9655 CMOS sensor
>>>>>>> +
>>>>>>> +The Omnivision OV965x sensor support multiple resolutions output, such
>>>>>>> as
>>>>>>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>>>>>>> +output format.
>>>>>>> +
>>>>>>> +Required Properties:
>>>>>>> +- compatible: should be one of
>>>>>>> +	"ovti,ov9650"
>>>>>>> +	"ovti,ov9652"
>>>>>>> +	"ovti,ov9655"
>>>>>>> +- clocks: reference to the mclk input clock.
>>>>>> 
>>>>>> I wonder why you have removed the clock-frequency property?
>>>>>> 
>>>>>> In some situations the camera driver must be able to tell the clock
>>>>>> source which frequency it wants to see.
>>>>> 
>>>>> That's what assigned-clock-rates property is for:
>>>>> 
>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-b
>>>>> indings.txt
>>>>> 
>>>>> AFAIU clock-frequency on devices is deprecated and equivalent to having
>>>>> a clocks property pointing to a fixed-clock, which is different from a
>>>>> clock with varying rate.
>>>> 
>>>> I am not sure if that helps here. The OMAP3-ISP does not have a fixed clock
>>>> rate so we can only have the driver define what it wants to see.
>>>> 
>>>> And common practise for OMAP3-ISP based camera modules (e.g. N900, N9) is
>>>> that they do it in the driver.
>>>> 
>>>> Maybe ISP developers can comment?
>>> 
>>> The OMAP3 ISP is a variable-frequency clock provider. The clock frequency is 
>>> controlled by the clock consumer. As such, it's up to the consumer to decide 
>>> whether to compute and request the clock rate dynamically at runtime, or use 
>>> the assigned-clock-rates property in DT.
>>> 
>>> Some ISPs include a clock generator, others don't. It should make no 
>>> difference whether the clock is provided by the ISP, by a dedicated clock 
>>> source in the SoC or by a discrete on-board adjustable clock source.
>> 
>> Thanks for explaining the background.
>> 
>> Do you have an hint or example how to use the assigned-clock-rates property in
>> a DT for a camera module connected to the omap3isp?
>> 
>> Or does it just mean that it defines the property name?
> 
> Please read the documentation link I sent - it's in the very bottom and
> should have an example.

I have seen it but it does not give me a good clue how to translate that into
correct omap3isp node setup in a specific DT. Rather it raises more questions.
Maybe because I don't understand completely what it is talking about.

The fundamental question is if this "assigned-clock-rates" is already
handled by ov965x->clk = devm_clk_get(&client->dev, NULL); ?

Or should we define that for the omap3isp node?

Then of course we need no new code and just use the right property names.
And N900, N9 camera DTs should be updated.

BR and thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1673749 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Hi Nikolaus,

On 06/23/2017 10:22 AM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 23.06.2017 um 16:57 schrieb Andreas Färber <afaerber@suse.de>:
>>
>> Am 23.06.2017 um 16:53 schrieb H. Nikolaus Schaller:
>>> Hi Laurent,
>>>
>>>> Am 23.06.2017 um 13:58 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> On Friday 23 Jun 2017 12:59:24 H. Nikolaus Schaller wrote:
>>>>> Am 23.06.2017 um 12:46 schrieb Andreas Färber <afaerber@suse.de>:
>>>>>> Am 23.06.2017 um 12:25 schrieb H. Nikolaus Schaller:
>>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>>>>> b/Documentation/devicetree/bindings/media/i2c/ov965x.txt new file mode
>>>>>>>> 100644
>>>>>>>> index 0000000..0e0de1f
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>> +* Omnivision OV9650/9652/9655 CMOS sensor
>>>>>>>> +
>>>>>>>> +The Omnivision OV965x sensor support multiple resolutions output, such
>>>>>>>> as
>>>>>>>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>>>>>>>> +output format.
>>>>>>>> +
>>>>>>>> +Required Properties:
>>>>>>>> +- compatible: should be one of
>>>>>>>> +	"ovti,ov9650"
>>>>>>>> +	"ovti,ov9652"
>>>>>>>> +	"ovti,ov9655"
>>>>>>>> +- clocks: reference to the mclk input clock.
>>>>>>>
>>>>>>> I wonder why you have removed the clock-frequency property?
>>>>>>>
>>>>>>> In some situations the camera driver must be able to tell the clock
>>>>>>> source which frequency it wants to see.
>>>>>>
>>>>>> That's what assigned-clock-rates property is for:
>>>>>>
>>>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/clock-b
>>>>>> indings.txt
>>>>>>
>>>>>> AFAIU clock-frequency on devices is deprecated and equivalent to having
>>>>>> a clocks property pointing to a fixed-clock, which is different from a
>>>>>> clock with varying rate.
>>>>>
>>>>> I am not sure if that helps here. The OMAP3-ISP does not have a fixed clock
>>>>> rate so we can only have the driver define what it wants to see.
>>>>>
>>>>> And common practise for OMAP3-ISP based camera modules (e.g. N900, N9) is
>>>>> that they do it in the driver.
>>>>>
>>>>> Maybe ISP developers can comment?
>>>>
>>>> The OMAP3 ISP is a variable-frequency clock provider. The clock frequency is 
>>>> controlled by the clock consumer. As such, it's up to the consumer to decide 
>>>> whether to compute and request the clock rate dynamically at runtime, or use 
>>>> the assigned-clock-rates property in DT.
>>>>
>>>> Some ISPs include a clock generator, others don't. It should make no 
>>>> difference whether the clock is provided by the ISP, by a dedicated clock 
>>>> source in the SoC or by a discrete on-board adjustable clock source.
>>>
>>> Thanks for explaining the background.
>>>
>>> Do you have an hint or example how to use the assigned-clock-rates property in
>>> a DT for a camera module connected to the omap3isp?
>>>
>>> Or does it just mean that it defines the property name?
>>
>> Please read the documentation link I sent - it's in the very bottom and
>> should have an example.
> 
> I have seen it but it does not give me a good clue how to translate that into
> correct omap3isp node setup in a specific DT. Rather it raises more questions.
> Maybe because I don't understand completely what it is talking about.
> 
> The fundamental question is if this "assigned-clock-rates" is already
> handled by ov965x->clk = devm_clk_get(&client->dev, NULL); ?
> 
> Or should we define that for the omap3isp node?
> 
> Then of course we need no new code and just use the right property names.
> And N900, N9 camera DTs should be updated.

Look up of_clk_set_defaults() function in drivers/clk/clk-conf.c. This
function gets invoked usually during clock registration, and also gets
called in platform_drv_probe(), so the parents and clocks do get
configured before your driver gets probed. So, this provides a default
configuration if these properties are supplied (in either clock nodes or
actual device nodes), and if your driver needs to change the rates at
runtime, then you would have to do that in the driver itself.

regards
Suman

[toc] | [prev] | [next] | [standalone]

#1673788 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

Hi Suman,

> Am 23.06.2017 um 20:05 schrieb Suman Anna <s-anna@ti.com>:
> 
>>>> 
>>>> Or does it just mean that it defines the property name?
>>> 
>>> Please read the documentation link I sent - it's in the very bottom and
>>> should have an example.
>> 
>> I have seen it but it does not give me a good clue how to translate that into
>> correct omap3isp node setup in a specific DT. Rather it raises more questions.
>> Maybe because I don't understand completely what it is talking about.
>> 
>> The fundamental question is if this "assigned-clock-rates" is already
>> handled by ov965x->clk = devm_clk_get(&client->dev, NULL); ?
>> 
>> Or should we define that for the omap3isp node?
>> 
>> Then of course we need no new code and just use the right property names.
>> And N900, N9 camera DTs should be updated.
> 
> Look up of_clk_set_defaults() function in drivers/clk/clk-conf.c. This
> function gets invoked usually during clock registration, and also gets
> called in platform_drv_probe(), so the parents and clocks do get
> configured before your driver gets probed. So, this provides a default
> configuration if these properties are supplied (in either clock nodes or
> actual device nodes), and if your driver needs to change the rates at
> runtime, then you would have to do that in the driver itself.

Ok, now I understand. Thanks!

Quite hidden, but nice feature. I would never have thought that it exists.
Especially as there are no examples around omap3isp cameras...

And an fgrep assigned-clock-rates shows not many use cases outside CPU/SoC
include files.

But interestingly arch/arm/boot/dts/at91sam9g25ek.dts uses it for an ovti,ov2640 camera...

So it seems that we just have to write:

	ov9655@30 {
		compatible = "ovti,ov9655";
		reg = <0x30>;
		clocks = <&isp 0>;	/* cam_clka */
		assigned-clocks = <&isp 0>;
		assigned-clock-rates = <24000000>;
	};

instead of introducing a new clock-frequency property and code to handle it.

Or do I misinterpret what "parents" and "clocks" are in this context?

BR and thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1673926 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

On 06/23/2017 01:59 PM, H. Nikolaus Schaller wrote:
> Hi Suman,
> 
>> Am 23.06.2017 um 20:05 schrieb Suman Anna <s-anna@ti.com>:
>>
>>>>>
>>>>> Or does it just mean that it defines the property name?
>>>>
>>>> Please read the documentation link I sent - it's in the very bottom and
>>>> should have an example.
>>>
>>> I have seen it but it does not give me a good clue how to translate that into
>>> correct omap3isp node setup in a specific DT. Rather it raises more questions.
>>> Maybe because I don't understand completely what it is talking about.
>>>
>>> The fundamental question is if this "assigned-clock-rates" is already
>>> handled by ov965x->clk = devm_clk_get(&client->dev, NULL); ?
>>>
>>> Or should we define that for the omap3isp node?
>>>
>>> Then of course we need no new code and just use the right property names.
>>> And N900, N9 camera DTs should be updated.
>>
>> Look up of_clk_set_defaults() function in drivers/clk/clk-conf.c. This
>> function gets invoked usually during clock registration, and also gets
>> called in platform_drv_probe(), so the parents and clocks do get
>> configured before your driver gets probed. So, this provides a default
>> configuration if these properties are supplied (in either clock nodes or
>> actual device nodes), and if your driver needs to change the rates at
>> runtime, then you would have to do that in the driver itself.
> 
> Ok, now I understand. Thanks!
> 
> Quite hidden, but nice feature. I would never have thought that it exists.
> Especially as there are no examples around omap3isp cameras...
> 
> And an fgrep assigned-clock-rates shows not many use cases outside CPU/SoC
> include files.
> 
> But interestingly arch/arm/boot/dts/at91sam9g25ek.dts uses it for an ovti,ov2640 camera...
> 
> So it seems that we just have to write:
> 
> 	ov9655@30 {
> 		compatible = "ovti,ov9655";
> 		reg = <0x30>;
> 		clocks = <&isp 0>;	/* cam_clka */
> 		assigned-clocks = <&isp 0>;
> 		assigned-clock-rates = <24000000>;
> 	};

Yeah, that looks alright and should work.

regards
Suman

> 
> instead of introducing a new clock-frequency property and code to handle it.
> 
> Or do I misinterpret what "parents" and "clocks" are in this context?

[toc] | [prev] | [next] | [standalone]

#1674449 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

> Am 24.06.2017 um 00:24 schrieb Suman Anna <s-anna@ti.com>:
> 
> On 06/23/2017 01:59 PM, H. Nikolaus Schaller wrote:
>> Hi Suman,
>> 
>>> Am 23.06.2017 um 20:05 schrieb Suman Anna <s-anna@ti.com>:
>>> 
>>>>>> 
>>>>>> Or does it just mean that it defines the property name?
>>>>> 
>>>>> Please read the documentation link I sent - it's in the very bottom and
>>>>> should have an example.
>>>> 
>>>> I have seen it but it does not give me a good clue how to translate that into
>>>> correct omap3isp node setup in a specific DT. Rather it raises more questions.
>>>> Maybe because I don't understand completely what it is talking about.
>>>> 
>>>> The fundamental question is if this "assigned-clock-rates" is already
>>>> handled by ov965x->clk = devm_clk_get(&client->dev, NULL); ?
>>>> 
>>>> Or should we define that for the omap3isp node?
>>>> 
>>>> Then of course we need no new code and just use the right property names.
>>>> And N900, N9 camera DTs should be updated.
>>> 
>>> Look up of_clk_set_defaults() function in drivers/clk/clk-conf.c. This
>>> function gets invoked usually during clock registration, and also gets
>>> called in platform_drv_probe(), so the parents and clocks do get
>>> configured before your driver gets probed. So, this provides a default
>>> configuration if these properties are supplied (in either clock nodes or
>>> actual device nodes), and if your driver needs to change the rates at
>>> runtime, then you would have to do that in the driver itself.
>> 
>> Ok, now I understand. Thanks!
>> 
>> Quite hidden, but nice feature. I would never have thought that it exists.
>> Especially as there are no examples around omap3isp cameras...
>> 
>> And an fgrep assigned-clock-rates shows not many use cases outside CPU/SoC
>> include files.
>> 
>> But interestingly arch/arm/boot/dts/at91sam9g25ek.dts uses it for an ovti,ov2640 camera...
>> 
>> So it seems that we just have to write:
>> 
>> 	ov9655@30 {
>> 		compatible = "ovti,ov9655";
>> 		reg = <0x30>;
>> 		clocks = <&isp 0>;	/* cam_clka */
>> 		assigned-clocks = <&isp 0>;
>> 		assigned-clock-rates = <24000000>;
>> 	};
> 
> Yeah, that looks alright and should work.

I have tested and it works that way.

Thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1674631 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

On 06/23/2017 12:25 PM, H. Nikolaus Schaller wrote:
> Hi Hugues,
> 
>> Am 22.06.2017 um 17:05 schrieb Hugues Fruchet <hugues.fruchet@st.com>:
>>
>> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>>
>> This adds documentation of device tree bindings
>> for the OV965X family camera sensor module.
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>> .../devicetree/bindings/media/i2c/ov965x.txt       | 37 ++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>> new file mode 100644
>> index 0000000..0e0de1f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>> @@ -0,0 +1,37 @@
>> +* Omnivision OV9650/9652/9655 CMOS sensor
>> +
>> +The Omnivision OV965x sensor support multiple resolutions output, such as
>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>> +output format.
>> +
>> +Required Properties:
>> +- compatible: should be one of
>> +	"ovti,ov9650"
>> +	"ovti,ov9652"
>> +	"ovti,ov9655"
>> +- clocks: reference to the mclk input clock.
> 
> I wonder why you have removed the clock-frequency property?
> 
> In some situations the camera driver must be able to tell the clock source
> which frequency it wants to see.
> 
> For example we connect the camera to an OMAP3-ISP (image signal processor) and
> there it is assumed that camera modules know the frequency and set the clock, e.g.:
> 
> http://elixir.free-electrons.com/linux/v4.4/source/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt#L52
> http://elixir.free-electrons.com/linux/v3.14/source/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> 
> If your clock is constant and defined elsewhere we should make this
> property optional instead of required. But it should not be missing.
> 
> Here is a hack to get it into your code:
> 
> http://git.goldelico.com/?p=gta04-kernel.git;a=blobdiff;f=drivers/media/i2c/ov9650.c;h=b7ab46c775b9e40087e427ae0777e9f7c283694a;hp=1846bcbb19ae71ce686dade320aa06ce2e429ca4;hb=ca85196f6fd9a77e5a0f796aeaf7aa2cde60ce91;hpb=8a71f21b75543a6d99102be1ae4677b28c478ac9
> 

Here is how it is used on my DT, the camera clock is a fixed crystal 24M 
clock:

+	clocks {
+		clk_ext_camera: clk-ext-camera {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <24000000>;
+		};
+	};
[...]
+	ov9655: camera@30 {
+		compatible = "ovti,ov9655";
+		reg = <0x30>;
+		pwdn-gpios = <&gpioh 13 GPIO_ACTIVE_HIGH>;
+		clocks = <&clk_ext_camera>;
+		status = "okay";
+
+		port {
+			ov9655_0: endpoint {
+				remote-endpoint = <&dcmi_0>;
+			};
+		};
+	};


>> +
>> +Optional Properties:
>> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
>> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
> 
> Here I wonder why you did split that up into two gpios. Each "*-gpios" can have
> multiple entries and if one is not used, a 0 can be specified to make it being ignored.
> 
> But it is up to DT maintainers what they prefer: separate single gpios or a single gpio array.

I have followed the ov2640 binding, which have the same pins naming 
(resetb/pwdn).
As far as I see, separate single gpios are commonly used in
Documentation/devicetree/bindings/media/i2c/

> 
> 
> What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
> So the driver does not power up the camera module when needed and therefore probing fails.
> 
>    - vana-supply: a regulator to power up the camera module.
> 
> Driver code is not complex to add:
> 
> http://git.goldelico.com/?p=gta04-kernel.git;a=blobdiff;f=drivers/media/i2c/ov9650.c;h=1846bcbb19ae71ce686dade320aa06ce2e429ca4;hp=c0819afdcefcb19da351741d51dad00aaf909254;hb=8a71f21b75543a6d99102be1ae4677b28c478ac9;hpb=6db55fc472eea2ec6db03833df027aecf6649f88

Yes, I saw it in your code, but as I don't have any programmable power 
supply on my setup, I have not pushed this commit.
And I also don't have a clock to enable/disable -fixed clock-, I need to 
check the behaviour when disabling/enabling a fixed clock, I will give 
it a try.

> 
>> +
>> +The device node must contain one 'port' child node for its digital output
>> +video port, in accordance with the video interface bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +
>> +&i2c2 {
>> +	ov9655: camera@30 {
>> +		compatible = "ovti,ov9655";
>> +		reg = <0x30>;
>> +		pwdn-gpios = <&gpioh 13 GPIO_ACTIVE_HIGH>;
>> +		clocks = <&clk_ext_camera>;
>> +
>> +		port {
>> +			ov9655: endpoint {
>> +				remote-endpoint = <&dcmi_0>;
>> +			};
>> +		};
>> +	};
>> +};
>> -- 
>> 1.9.1
>>
> 
> BR and thanks,
> Nikolaus
> 

[toc] | [prev] | [next] | [standalone]

#1675074 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>> What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
>> So the driver does not power up the camera module when needed and therefore probing fails.
>>
>>     - vana-supply: a regulator to power up the camera module.
>>
>> Driver code is not complex to add:

> Yes, I saw it in your code, but as I don't have any programmable power
> supply on my setup, I have not pushed this commit.

Since you are about to add voltage supplies to the DT binding I'd suggest
to include all three voltage supplies of the sensor chip. Looking at the OV9650
and the OV9655 datasheet there are following names used for the voltage supply
pins:

AVDD - Analog power supply,
DVDD - Power supply for digital core logic,
DOVDD - Digital power supply for I/O.

I doubt the sensor can work without any of these voltage supplies, thus
regulator_get_optional() should not be used. I would just use the regulator
bulk API to handle all three power supplies.

--
Regards,
Sylwester

[toc] | [prev] | [next] | [standalone]

#1675305 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
> 
> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>>> What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
>>> So the driver does not power up the camera module when needed and therefore probing fails.
>>> 
>>>    - vana-supply: a regulator to power up the camera module.
>>> 
>>> Driver code is not complex to add:
> 
>> Yes, I saw it in your code, but as I don't have any programmable power
>> supply on my setup, I have not pushed this commit.
> 
> Since you are about to add voltage supplies to the DT binding I'd suggest
> to include all three voltage supplies of the sensor chip. Looking at the OV9650
> and the OV9655 datasheet there are following names used for the voltage supply
> pins:
> 
> AVDD - Analog power supply,
> DVDD - Power supply for digital core logic,
> DOVDD - Digital power supply for I/O.

The latter two are usually not independently switchable from the SoC power
the module is connected to.

And sometimes DVDD and DOVDD are connected together.

So the driver can't make much use of knowing or requesting them because the
1.8V supply is always active, even during suspend.

> 
> I doubt the sensor can work without any of these voltage supplies, thus
> regulator_get_optional() should not be used. I would just use the regulator
> bulk API to handle all three power supplies.

The digital part works with AVDD turned off. So the LDO supplying AVDD should
be switchable to save power (&vaux3 on the GTA04 device).

But not all designs can switch it off. Hence the idea to define it as an
/optional/ regulator. If it is not defined by DT, the driver simply assumes
it is always powered on.

So in summary we only need AVDD switched for the GTA04 - but it does not
matter if the others are optional properties. We would not use them.

It does matter if they are mandatory because it adds DT complexity (size
and processing) without added function.

BR and thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1676243 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote:
>> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
>>
>> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>>>> What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
>>>> So the driver does not power up the camera module when needed and therefore probing fails.
>>>>
>>>>     - vana-supply: a regulator to power up the camera module.
>>>>
>>>> Driver code is not complex to add:
>>
>>> Yes, I saw it in your code, but as I don't have any programmable power
>>> supply on my setup, I have not pushed this commit.
>>
>> Since you are about to add voltage supplies to the DT binding I'd suggest
>> to include all three voltage supplies of the sensor chip. Looking at the OV9650
>> and the OV9655 datasheet there are following names used for the voltage supply
>> pins:
>>
>> AVDD - Analog power supply,
>> DVDD - Power supply for digital core logic,
>> DOVDD - Digital power supply for I/O.
> 
> The latter two are usually not independently switchable from the SoC power
> the module is connected to.
> 
> And sometimes DVDD and DOVDD are connected together.
> 
> So the driver can't make much use of knowing or requesting them because the
> 1.8V supply is always active, even during suspend.
> 
>>
>> I doubt the sensor can work without any of these voltage supplies, thus
>> regulator_get_optional() should not be used. I would just use the regulator
>> bulk API to handle all three power supplies.
> 
> The digital part works with AVDD turned off. So the LDO supplying AVDD should
> be switchable to save power (&vaux3 on the GTA04 device).>
> But not all designs can switch it off. Hence the idea to define it as an
> /optional/ regulator. If it is not defined by DT, the driver simply assumes
> it is always powered on.

I didn't say we can't define regulator supply properties as optional in the DT 
binding.  If we define them as such and any of these *-supply properties is 
missing in DT with regulator_get() the regulator core will use dummy regulator 
for that particular voltage supply.  While with regulator_get_optional() 
-ENODEV is returned when the regulator cannot be found. 

> So in summary we only need AVDD switched for the GTA04 - but it does not
> matter if the others are optional properties. We would not use them.
> 
> It does matter if they are mandatory because it adds DT complexity (size
> and processing) without added function.
 
We should not be defining DT binding only with selected use cases/board
designs in mind.  IMO all three voltage supplies should be listed in the
binding, presumably all can be made optional, with an assumption that when
the property is missing selected pin is hooked up to a fixed regulator.

--
Thanks,
Sylwester

[toc] | [prev] | [next] | [standalone]

#1676464 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

> Am 28.06.2017 um 00:57 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
> 
> On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote:
>>> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
>>> 
>>> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>>>>> What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
>>>>> So the driver does not power up the camera module when needed and therefore probing fails.
>>>>> 
>>>>>    - vana-supply: a regulator to power up the camera module.
>>>>> 
>>>>> Driver code is not complex to add:
>>> 
>>>> Yes, I saw it in your code, but as I don't have any programmable power
>>>> supply on my setup, I have not pushed this commit.
>>> 
>>> Since you are about to add voltage supplies to the DT binding I'd suggest
>>> to include all three voltage supplies of the sensor chip. Looking at the OV9650
>>> and the OV9655 datasheet there are following names used for the voltage supply
>>> pins:
>>> 
>>> AVDD - Analog power supply,
>>> DVDD - Power supply for digital core logic,
>>> DOVDD - Digital power supply for I/O.
>> 
>> The latter two are usually not independently switchable from the SoC power
>> the module is connected to.
>> 
>> And sometimes DVDD and DOVDD are connected together.
>> 
>> So the driver can't make much use of knowing or requesting them because the
>> 1.8V supply is always active, even during suspend.
>> 
>>> 
>>> I doubt the sensor can work without any of these voltage supplies, thus
>>> regulator_get_optional() should not be used. I would just use the regulator
>>> bulk API to handle all three power supplies.
>> 
>> The digital part works with AVDD turned off. So the LDO supplying AVDD should
>> be switchable to save power (&vaux3 on the GTA04 device).>
>> But not all designs can switch it off. Hence the idea to define it as an
>> /optional/ regulator. If it is not defined by DT, the driver simply assumes
>> it is always powered on.
> 
> I didn't say we can't define regulator supply properties as optional in the DT 
> binding.  If we define them as such and any of these *-supply properties is 
> missing in DT with regulator_get() the regulator core will use dummy regulator 
> for that particular voltage supply.  While with regulator_get_optional() 
> -ENODEV is returned when the regulator cannot be found. 

Ah, ok. I see.

I had thought that it is the right thing to do like devm_gpiod_get_optional().

That one it is described as:

"* This is equivalent to gpiod_get(), except that when no GPIO was assigned to
 * the requested function it will return NULL. This is convenient for drivers
 * that need to handle optional GPIOs."

Seems to be inconsistent definition of what "optional" means.

So we indeed should use devm_regulator_get() in this case. Thanks for pointing out!

> 
>> So in summary we only need AVDD switched for the GTA04 - but it does not
>> matter if the others are optional properties. We would not use them.
>> 
>> It does matter if they are mandatory because it adds DT complexity (size
>> and processing) without added function.
> 
> We should not be defining DT binding only with selected use cases/board
> designs in mind.  IMO all three voltage supplies should be listed in the
> binding, presumably all can be made optional, with an assumption that when
> the property is missing selected pin is hooked up to a fixed regulator.

Ok, then it should just be defined in the bindings but not used by the driver?

BR and thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

#1676551 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

On 06/28/2017 11:12 AM, H. Nikolaus Schaller wrote:
>> Am 28.06.2017 um 00:57 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
>> On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote:
>>>> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
>>>> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>>>>>> What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
>>>>>> So the driver does not power up the camera module when needed and therefore probing fails.
>>>>>>
>>>>>>     - vana-supply: a regulator to power up the camera module.
>>>>>>
>>>>>> Driver code is not complex to add:
>>>>
>>>>> Yes, I saw it in your code, but as I don't have any programmable power
>>>>> supply on my setup, I have not pushed this commit.
>>>>
>>>> Since you are about to add voltage supplies to the DT binding I'd suggest
>>>> to include all three voltage supplies of the sensor chip. Looking at the OV9650
>>>> and the OV9655 datasheet there are following names used for the voltage supply
>>>> pins:
>>>>
>>>> AVDD - Analog power supply,
>>>> DVDD - Power supply for digital core logic,
>>>> DOVDD - Digital power supply for I/O.
>>>
>>> The latter two are usually not independently switchable from the SoC power
>>> the module is connected to.
>>>
>>> And sometimes DVDD and DOVDD are connected together.
>>>
>>> So the driver can't make much use of knowing or requesting them because the
>>> 1.8V supply is always active, even during suspend.
>>>
>>>>
>>>> I doubt the sensor can work without any of these voltage supplies, thus
>>>> regulator_get_optional() should not be used. I would just use the regulator
>>>> bulk API to handle all three power supplies.
>>>
>>> The digital part works with AVDD turned off. So the LDO supplying AVDD should
>>> be switchable to save power (&vaux3 on the GTA04 device).>
>>> But not all designs can switch it off. Hence the idea to define it as an
>>> /optional/ regulator. If it is not defined by DT, the driver simply assumes
>>> it is always powered on.
>>
>> I didn't say we can't define regulator supply properties as optional in the DT
>> binding.  If we define them as such and any of these *-supply properties is
>> missing in DT with regulator_get() the regulator core will use dummy regulator
>> for that particular voltage supply.  While with regulator_get_optional()
>> -ENODEV is returned when the regulator cannot be found.
> 
> Ah, ok. I see.
> 
> I had thought that it is the right thing to do like devm_gpiod_get_optional().
> 
> That one it is described as:
> 
> "* This is equivalent to gpiod_get(), except that when no GPIO was assigned to
>   * the requested function it will return NULL. This is convenient for drivers
>   * that need to handle optional GPIOs."
> 
> Seems to be inconsistent definition of what "optional" means.

Indeed, this commit explains it further:

commit de1dd9fd2156874b45803299b3b27e65d5defdd9
regulator: core: Provide hints to the core about optional supplies

> So we indeed should use devm_regulator_get() in this case. Thanks for > pointing out!

>>> So in summary we only need AVDD switched for the GTA04 - but it does not
>>> matter if the others are optional properties. We would not use them.
>>>
>>> It does matter if they are mandatory because it adds DT complexity (size
>>> and processing) without added function.
>>
>> We should not be defining DT binding only with selected use cases/board
>> designs in mind.  IMO all three voltage supplies should be listed in the
>> binding, presumably all can be made optional, with an assumption that when
>> the property is missing selected pin is hooked up to a fixed regulator.
> 
> Ok, then it should just be defined in the bindings but not used by 
> the driver?

Yes, I think so. So we have a possibly complete binding right from the 
beginning. I someone needs handling other supplies than AVDD they could
update the driver in future.


Regards,
Sylwester

[toc] | [prev] | [next] | [standalone]

#1676563 — Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

> Am 28.06.2017 um 12:50 schrieb Sylwester Nawrocki <s.nawrocki@samsung.com>:
> 
> On 06/28/2017 11:12 AM, H. Nikolaus Schaller wrote:
>>> Am 28.06.2017 um 00:57 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
>>> On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote:
>>>>> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki <snawrocki@kernel.org>:
>>>>> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>>>>>>> What I am missing to support the GTA04 camera is the control of the optional "vana-supply".
>>>>>>> So the driver does not power up the camera module when needed and therefore probing fails.
>>>>>>> 
>>>>>>>    - vana-supply: a regulator to power up the camera module.
>>>>>>> 
>>>>>>> Driver code is not complex to add:
>>>>> 
>>>>>> Yes, I saw it in your code, but as I don't have any programmable power
>>>>>> supply on my setup, I have not pushed this commit.
>>>>> 
>>>>> Since you are about to add voltage supplies to the DT binding I'd suggest
>>>>> to include all three voltage supplies of the sensor chip. Looking at the OV9650
>>>>> and the OV9655 datasheet there are following names used for the voltage supply
>>>>> pins:
>>>>> 
>>>>> AVDD - Analog power supply,
>>>>> DVDD - Power supply for digital core logic,
>>>>> DOVDD - Digital power supply for I/O.
>>>> 
>>>> The latter two are usually not independently switchable from the SoC power
>>>> the module is connected to.
>>>> 
>>>> And sometimes DVDD and DOVDD are connected together.
>>>> 
>>>> So the driver can't make much use of knowing or requesting them because the
>>>> 1.8V supply is always active, even during suspend.
>>>> 
>>>>> 
>>>>> I doubt the sensor can work without any of these voltage supplies, thus
>>>>> regulator_get_optional() should not be used. I would just use the regulator
>>>>> bulk API to handle all three power supplies.
>>>> 
>>>> The digital part works with AVDD turned off. So the LDO supplying AVDD should
>>>> be switchable to save power (&vaux3 on the GTA04 device).>
>>>> But not all designs can switch it off. Hence the idea to define it as an
>>>> /optional/ regulator. If it is not defined by DT, the driver simply assumes
>>>> it is always powered on.
>>> 
>>> I didn't say we can't define regulator supply properties as optional in the DT
>>> binding.  If we define them as such and any of these *-supply properties is
>>> missing in DT with regulator_get() the regulator core will use dummy regulator
>>> for that particular voltage supply.  While with regulator_get_optional()
>>> -ENODEV is returned when the regulator cannot be found.
>> 
>> Ah, ok. I see.
>> 
>> I had thought that it is the right thing to do like devm_gpiod_get_optional().
>> 
>> That one it is described as:
>> 
>> "* This is equivalent to gpiod_get(), except that when no GPIO was assigned to
>>  * the requested function it will return NULL. This is convenient for drivers
>>  * that need to handle optional GPIOs."
>> 
>> Seems to be inconsistent definition of what "optional" means.
> 
> Indeed, this commit explains it further:
> 
> commit de1dd9fd2156874b45803299b3b27e65d5defdd9
> regulator: core: Provide hints to the core about optional supplies
> 
>> So we indeed should use devm_regulator_get() in this case. Thanks for > pointing out!
> 
>>>> So in summary we only need AVDD switched for the GTA04 - but it does not
>>>> matter if the others are optional properties. We would not use them.
>>>> 
>>>> It does matter if they are mandatory because it adds DT complexity (size
>>>> and processing) without added function.
>>> 
>>> We should not be defining DT binding only with selected use cases/board
>>> designs in mind.  IMO all three voltage supplies should be listed in the
>>> binding, presumably all can be made optional, with an assumption that when
>>> the property is missing selected pin is hooked up to a fixed regulator.
>> 
>> Ok, then it should just be defined in the bindings but not used by 
>> the driver?
> 
> Yes, I think so. So we have a possibly complete binding right from the 
> beginning. I someone needs handling other supplies than AVDD they could
> update the driver in future.

Fine! I have sent some patches to Hughues so that he can integrate it in
his next version of the patch series.

BR and thanks,
Nikolaus

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web