Re: [PATCH v9 1/2] Add OV5647 device tree documentation
On Sat, Feb 25, 2017 at 04:50:53PM +0200, Sakari Ailus wrote: > Although, the driver could try to work on the actual obtained frequency. > This is unlikely to work though, but it won't be very easy to figure out > why the device isn't working. Having the frequency in DT accessible for the > driver to check makes failing early with a clear error message possible. The below point actually makes the above matter a non-issue: this driver will contain a list of supported frequencies. A SMIA driver that I typically work with, for instance, does not. So I think your suggestion of having the CCF handle setting the frequency is a good one. > > > > > 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. > > Indeed. The new supported frequencies in this case will be additional > single values; there won't be ranges or such. The register lists the > driver contains are more or less dependent on that frequency. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Vladimir, On Wed, Feb 22, 2017 at 12:37:51AM +0200, 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 :) X-) > > > 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 ..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. That's a sensor specific name, the one in the hardware datasheet should be used if there's one, shouldn't it? An alternative is not to use a name, as there's just a single clock. > > >> > >> 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. You can, but there's no guarantee that the frequency what you get is going to be what you asked for. Although, the driver could try to work on the actual obtained frequ
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Vladimir On 2/22/2017 11:39 AM, Vladimir Zapolskiy wrote: > Hi Ramiro, > > On 02/22/2017 12:57 PM, Ramiro Oliveira wrote: >> 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 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 ..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 >> > > Ah, if the name comes from the sensor datasheet, then it should be okay > to keep it. > > > 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
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Ramiro, On 02/22/2017 12:57 PM, Ramiro Oliveira wrote: > 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 >>> 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 ..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 > Ah, if the name comes from the sensor datasheet, then it should be okay to keep it. 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
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
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
>> 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 ..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 != 2500) {
dev_err(dev, "
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
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 > 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 ..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. >> >> 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. -- With best wishes, Vladimir
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
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 ..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 = <2500>;
> >>
> >> 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.ai...@iki.fi XMPP: sai...@retiisi.org.uk
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
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 ..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.
Can you please describe what for does ov5647 sensor need this clock, what
is its function?
>
>>> +- 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 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 = <2500>;
>>
>> When you remove two unused properties, please don't forget to update the
>> example.
>>
>
> Ok.
>
>>> + port {
>>> + camera_1: endpoint {
>>> + remote-endpoint = <&csi1_ep1>;
>>> + };
>>> + };
>>> + };
>>> + };
>>>
>>
--
With best wishes,
Vladimir
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
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 ..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.
>> +- 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.
>> +
>> +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 = <2500>;
>
> When you remove two unused properties, please don't forget to update the
> example.
>
Ok.
>> +port {
>> +camera_1: endpoint {
>> +remote-endpoint = <&csi1_ep1>;
>> +};
>> +};
>> +};
>> +};
>>
>
> --
> With best wishes,
> Vladimir
>
--
Best Regards
Ramiro Oliveira
ramiro.olive...@synopsys.com
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
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 ..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?
> +- clock-names: Should be "xclk".
You can remove this property, because there is only one source clock.
> +- clock-frequency: Frequency of the xclk clock.
And after the last updates in the driver this property can be removed as well.
> +
> +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 = <2500>;
When you remove two unused properties, please don't forget to update the
example.
> + port {
> + camera_1: endpoint {
> + remote-endpoint = <&csi1_ep1>;
> + };
> + };
> + };
> + };
>
--
With best wishes,
Vladimir
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Ramiro, On Tue, Feb 21, 2017 at 02:30:16PM +, Ramiro Oliveira wrote: > Hi Sakari, > > Thank you for your feedback. > > On 2/21/2017 11:57 AM, Sakari Ailus wrote: > > Hi Ramiro, > > > > On Fri, Feb 17, 2017 at 01:14:15PM +, 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 ..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. > >> +- clock-names : Should be "xclk". > >> +- clock-frequency : Frequency of the xclk clock. > >> + > >> +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. > > > > The remote-endpoint property in endpoint nodes should be mandatory, > > shouldn't it? Otherwise the sensor isn't connected to anything and hardly > > useful as such. The list of optional endpoint properties is a long one and > > it should be documented here which ones are recognised, either as optional > > or mandatory. > > > > I guess you're right, it should be mandatory, although at the moment I'm not > checking for it's presence in the driver so I'll add it to the driver. > > At the moment that's the only property I think it should be mandatory, and I > don't believe I need any optional one. > > Do you have a suggestion for any new property I should use? As you don't need any in the driver for the driver to work, you can omit them. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Sakari,
Thank you for your feedback.
On 2/21/2017 11:57 AM, Sakari Ailus wrote:
> Hi Ramiro,
>
> On Fri, Feb 17, 2017 at 01:14:15PM +, 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 ..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.
>> +- clock-names : Should be "xclk".
>> +- clock-frequency : Frequency of the xclk clock.
>> +
>> +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.
>
> The remote-endpoint property in endpoint nodes should be mandatory,
> shouldn't it? Otherwise the sensor isn't connected to anything and hardly
> useful as such. The list of optional endpoint properties is a long one and
> it should be documented here which ones are recognised, either as optional
> or mandatory.
>
I guess you're right, it should be mandatory, although at the moment I'm not
checking for it's presence in the driver so I'll add it to the driver.
At the moment that's the only property I think it should be mandatory, and I
don't believe I need any optional one.
Do you have a suggestion for any new property I should use?
>> +
>> +Example:
>> +
>> +i2c@2000 {
>> +...
>> +ov: camera@36 {
>> +compatible = "ovti,ov5647";
>> +reg = <0x36>;
>> +clocks = <&camera_clk>;
>> +clock-names = "xclk";
>> +clock-frequency = <2500>;
>> +port {
>> +camera_1: endpoint {
>> +remote-endpoint = <&csi1_ep1>;
>> +};
>> +};
>> +};
>> +};
>
--
Best Regards
Ramiro Oliveira
ramiro.olive...@synopsys.com
Re: [PATCH v9 1/2] Add OV5647 device tree documentation
Hi Ramiro,
On Fri, Feb 17, 2017 at 01:14:15PM +, 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 ..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.
> +- clock-names: Should be "xclk".
> +- clock-frequency: Frequency of the xclk clock.
> +
> +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.
The remote-endpoint property in endpoint nodes should be mandatory,
shouldn't it? Otherwise the sensor isn't connected to anything and hardly
useful as such. The list of optional endpoint properties is a long one and
it should be documented here which ones are recognised, either as optional
or mandatory.
> +
> +Example:
> +
> + i2c@2000 {
> + ...
> + ov: camera@36 {
> + compatible = "ovti,ov5647";
> + reg = <0x36>;
> + clocks = <&camera_clk>;
> + clock-names = "xclk";
> + clock-frequency = <2500>;
> + port {
> + camera_1: endpoint {
> + remote-endpoint = <&csi1_ep1>;
> + };
> + };
> + };
> + };
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
[PATCH v9 1/2] Add OV5647 device tree documentation
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 ..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.
+- clock-names : Should be "xclk".
+- clock-frequency : Frequency of the xclk clock.
+
+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 = <2500>;
+ port {
+ camera_1: endpoint {
+ remote-endpoint = <&csi1_ep1>;
+ };
+ };
+ };
+ };
--
2.11.0
