Re: [PATCH v9 1/2] Add OV5647 device tree documentation

[Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>]

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@synopsys.com>
>>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>>> ---
>>>>>>  .../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@synopsys.com