From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752848AbdBUVtD (ORCPT ); Tue, 21 Feb 2017 16:49:03 -0500 Received: from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:39358 "EHLO hillosipuli.retiisi.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751745AbdBUVsy (ORCPT ); Tue, 21 Feb 2017 16:48:54 -0500 Date: Tue, 21 Feb 2017 23:48:14 +0200 From: Sakari Ailus To: Vladimir Zapolskiy Cc: Ramiro Oliveira , linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, devicetree@vger.kernel.org, CARLOS.PALMINHA@synopsys.com, Arnd Bergmann , "David S. Miller" , Geert Uytterhoeven , Greg Kroah-Hartman , Guenter Roeck , Hans Verkuil , Lars-Peter Clausen , Mark Rutland , Mauro Carvalho Chehab , Pali =?iso-8859-1?Q?Roh=E1r?= , Pavel Machek , Robert Jarzmik , Rob Herring , Sakari Ailus , Steve Longerbeam Subject: Re: [PATCH v9 1/2] Add OV5647 device tree documentation Message-ID: <20170221214813.GN16975@valkosipuli.retiisi.org.uk> References: <5a93352142495528dd56d5e281a8ed8ad6404a05.1487334912.git.roliveir@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Vladimir! How do you do? :-) On Tue, Feb 21, 2017 at 10:48:09PM +0200, Vladimir Zapolskiy wrote: > Hi Ramiro, > > On 02/21/2017 10:13 PM, Ramiro Oliveira wrote: > > Hi Vladimir, > > > > Thank you for your feedback > > > > On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote: > >> Hi Ramiro, > >> > >> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote: > >>> Create device tree bindings documentation. > >>> > >>> Signed-off-by: Ramiro Oliveira > >>> Acked-by: Rob Herring > >>> --- > >>> .../devicetree/bindings/media/i2c/ov5647.txt | 35 ++++++++++++++++++++++ > >>> 1 file changed, 35 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt > >>> new file mode 100644 > >>> index 000000000000..31956426d3b9 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt > >>> @@ -0,0 +1,35 @@ > >>> +Omnivision OV5647 raw image sensor > >>> +--------------------------------- > >>> + > >>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces > >>> +and CCI (I2C compatible) control bus. > >>> + > >>> +Required properties: > >>> + > >>> +- compatible : "ovti,ov5647". > >>> +- reg : I2C slave address of the sensor. > >>> +- clocks : Reference to the xclk clock. > >> > >> Is "xclk" clock a pixel clock or something else? > >> > > > > It's an external oscillator. > > hmm, I suppose a clock of any type could serve as a clock for the sensor. > It can be an external oscillator on a particular board, or it can be > something else on another board. Any clock source could be used I presume. > > Can you please describe what for does ov5647 sensor need this clock, what > is its function? Camera modules (sensors) quite commonly require an external clock as they do not have an oscillator on their own. A lot of devices under Documentation/devicetree/bindings/media/i2c/ have similar properties. > > > > >>> +- clock-names : Should be "xclk". > >> > >> You can remove this property, because there is only one source clock. > >> > > > > Ok. > > > >>> +- clock-frequency : Frequency of the xclk clock. > >> > >> And after the last updates in the driver this property can be removed as well. > >> > > > > But I'm still using clk_get_rate in the driver, if I remove the frequency here > > the probing will fail. > > > > I doubt it, there should be no connection between a custom "clock-frequency" > device tree property in a clock consumer device node and clk_get_rate() function > from the CCF, which takes a clock provider as its argument. The purpose is to make sure the clock frequency is really usable for the device, in this particular case the driver can work with one particular frequency. That said, the driver does not appear to use the property at the moment. It should. It'd be good to verify that the rate matches: clk_set_rate() is not guaranteed to produce the requested clock rate, and the driver could conceivably be updated with support for more frequencies. There are typically a few frequencies that a SoC such a sensor is connected can support, and 25 MHz is not one of the common frequencies. With this property, the frequency would be always there explicitly. > > >>> + > >>> +The common video interfaces bindings (see video-interfaces.txt) should be > >>> +used to specify link to the image data receiver. The OV5647 device > >>> +node should contain one 'port' child node with an 'endpoint' subnode. > >>> + > >>> +Example: > >>> + > >>> + i2c@2000 { > >>> + ... > >>> + ov: camera@36 { > >>> + compatible = "ovti,ov5647"; > >>> + reg = <0x36>; > >>> + clocks = <&camera_clk>; > >>> + clock-names = "xclk"; > >>> + clock-frequency = <25000000>; > >> > >> When you remove two unused properties, please don't forget to update the > >> example. > >> > > > > Ok. > > > >>> + port { > >>> + camera_1: endpoint { > >>> + remote-endpoint = <&csi1_ep1>; > >>> + }; > >>> + }; > >>> + }; > >>> + }; > >>> > >> > > -- ^ There's a space missing here. > With best wishes, > Vladimir -- Kind regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sakari Ailus Subject: Re: [PATCH v9 1/2] Add OV5647 device tree documentation Date: Tue, 21 Feb 2017 23:48:14 +0200 Message-ID: <20170221214813.GN16975@valkosipuli.retiisi.org.uk> References: <5a93352142495528dd56d5e281a8ed8ad6404a05.1487334912.git.roliveir@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Vladimir Zapolskiy Cc: Ramiro Oliveira , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w@public.gmane.org, Arnd Bergmann , "David S. Miller" , Geert Uytterhoeven , Greg Kroah-Hartman , Guenter Roeck , Hans Verkuil , Lars-Peter Clausen , Mark Rutland , Mauro Carvalho Chehab , Pali =?iso-8859-1?Q?Roh=E1r?= , Pavel Machek , Robert Jarzmik , Rob Herring , Sakari Ailus , Steve Longerbeam List-Id: devicetree@vger.kernel.org Hi, Vladimir! How do you do? :-) On Tue, Feb 21, 2017 at 10:48:09PM +0200, Vladimir Zapolskiy wrote: > Hi Ramiro, > > On 02/21/2017 10:13 PM, Ramiro Oliveira wrote: > > Hi Vladimir, > > > > Thank you for your feedback > > > > On 2/21/2017 3:58 PM, Vladimir Zapolskiy wrote: > >> Hi Ramiro, > >> > >> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote: > >>> Create device tree bindings documentation. > >>> > >>> Signed-off-by: Ramiro Oliveira > >>> Acked-by: Rob Herring > >>> --- > >>> .../devicetree/bindings/media/i2c/ov5647.txt | 35 ++++++++++++++++++++++ > >>> 1 file changed, 35 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt > >>> new file mode 100644 > >>> index 000000000000..31956426d3b9 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt > >>> @@ -0,0 +1,35 @@ > >>> +Omnivision OV5647 raw image sensor > >>> +--------------------------------- > >>> + > >>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces > >>> +and CCI (I2C compatible) control bus. > >>> + > >>> +Required properties: > >>> + > >>> +- compatible : "ovti,ov5647". > >>> +- reg : I2C slave address of the sensor. > >>> +- clocks : Reference to the xclk clock. > >> > >> Is "xclk" clock a pixel clock or something else? > >> > > > > It's an external oscillator. > > hmm, I suppose a clock of any type could serve as a clock for the sensor. > It can be an external oscillator on a particular board, or it can be > something else on another board. Any clock source could be used I presume. > > Can you please describe what for does ov5647 sensor need this clock, what > is its function? Camera modules (sensors) quite commonly require an external clock as they do not have an oscillator on their own. A lot of devices under Documentation/devicetree/bindings/media/i2c/ have similar properties. > > > > >>> +- clock-names : Should be "xclk". > >> > >> You can remove this property, because there is only one source clock. > >> > > > > Ok. > > > >>> +- clock-frequency : Frequency of the xclk clock. > >> > >> And after the last updates in the driver this property can be removed as well. > >> > > > > But I'm still using clk_get_rate in the driver, if I remove the frequency here > > the probing will fail. > > > > I doubt it, there should be no connection between a custom "clock-frequency" > device tree property in a clock consumer device node and clk_get_rate() function > from the CCF, which takes a clock provider as its argument. The purpose is to make sure the clock frequency is really usable for the device, in this particular case the driver can work with one particular frequency. That said, the driver does not appear to use the property at the moment. It should. It'd be good to verify that the rate matches: clk_set_rate() is not guaranteed to produce the requested clock rate, and the driver could conceivably be updated with support for more frequencies. There are typically a few frequencies that a SoC such a sensor is connected can support, and 25 MHz is not one of the common frequencies. With this property, the frequency would be always there explicitly. > > >>> + > >>> +The common video interfaces bindings (see video-interfaces.txt) should be > >>> +used to specify link to the image data receiver. The OV5647 device > >>> +node should contain one 'port' child node with an 'endpoint' subnode. > >>> + > >>> +Example: > >>> + > >>> + i2c@2000 { > >>> + ... > >>> + ov: camera@36 { > >>> + compatible = "ovti,ov5647"; > >>> + reg = <0x36>; > >>> + clocks = <&camera_clk>; > >>> + clock-names = "xclk"; > >>> + clock-frequency = <25000000>; > >> > >> When you remove two unused properties, please don't forget to update the > >> example. > >> > > > > Ok. > > > >>> + port { > >>> + camera_1: endpoint { > >>> + remote-endpoint = <&csi1_ep1>; > >>> + }; > >>> + }; > >>> + }; > >>> + }; > >>> > >> > > -- ^ There's a space missing here. > With best wishes, > Vladimir -- Kind regards, Sakari Ailus e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html