Hi Vladimir On 2/21/2017 10:37 PM, Vladimir Zapolskiy wrote: > Hi Sakari, > > On 02/21/2017 11:48 PM, Sakari Ailus wrote: >> Hi, Vladimir! >> >> How do you do? :-) > > deferring execution of boring tasks by doing code review :) > >> 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 <roliveir@xxxxxxxxxxxx> >>>>>> Acked-by: Rob Herring <robh@xxxxxxxxxx> >>>>>> --- >>>>>> .../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. >> > > That's exactly my point, and it is a reason to rename "xclk" to something > more generic. > xclk it's the name being used in the camera datasheet, but I can change it to something more generic >>> >>> 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. >> > > So, what should be a better replacement of "xclk" in the description above? > > E.g. > > - clocks : Phandle to a device supply clock. > >>> >>>> >>>>>> +- clock-names : Should be "xclk". > > We got an agreement that "clock-names" property is removed, nevertheless > if it is added back, is should not 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. >> > > I can provide my arguments given at v8 review time again, since I don't > see a contradiction with my older comments. > > Briefly "clock-frequency" as a device tree property on a consumer side > can be considered as redundant, because there is a mechanism to specify > a wanted clock frequency on a clock provider side right in a board DTB. > > So, the clock frequency set up is delegated to CCF, and when any other > than 25 MHz frequencies are got supported, that's only the matter of > driver updates, DTBs won't be touched. > In the driver, I'm using this piece of code to check that the frequency is 25Mhz xclk_freq = clk_get_rate(sensor->xclk); if (xclk_freq != 25000000) { dev_err(dev, "Unsupported clock frequency: %u\n", xclk_freq); return -EINVAL; } So if I don't define it here the probing will fail. Do you have another suggestion for this? -- Best Regards Ramiro Oliveira Ramiro.Oliveira@xxxxxxxxxxxx