Re: [PATCH v4] media: i2c: add support for omnivision's ov2659 sensor

[Sakari Ailus]

Hi Laurent,

On Wed, Mar 11, 2015 at 08:09:11PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday 11 March 2015 13:04:43 Sakari Ailus wrote:
> > On Sun, Mar 08, 2015 at 11:33:27AM +0000, Lad Prabhakar wrote:
> > > From: Benoit Parrot <bparrot@xxxxxx>
> > > 
> > > this patch adds support for omnivision's ov2659
> > > sensor, the driver supports following features:
> > > 1: Asynchronous probing
> > > 2: DT support
> > > 3: Media controller support
> > > 
> > > Signed-off-by: Benoit Parrot <bparrot@xxxxxx>
> > > Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
> > > ---
> > > 
> > >  Sorry quick new version, I need to get this merged for next
> > >  merge window.
> > >  
> > >  Changes for v4:
> > >  1: Renamed target frequency property to 'link-frequencies'
> > >     as per Sakari's suggestion.
> > >  
> > >  2: Changed the copyright to "GPL v2"
> > >  
> > >  v3: https://patchwork.kernel.org/patch/5959401/
> > >  v2: https://patchwork.kernel.org/patch/5859801/
> > >  v1: https://patchwork.linuxtv.org/patch/27919/
> > >  
> > >  .../devicetree/bindings/media/i2c/ov2659.txt       |   38 +
> > >  MAINTAINERS                                        |   10 +
> > >  drivers/media/i2c/Kconfig                          |   11 +
> > >  drivers/media/i2c/Makefile                         |    1 +
> > >  drivers/media/i2c/ov2659.c                         | 1439 +++++++++++++++
> > >  include/media/ov2659.h                             |   33 +
> > >  6 files changed, 1532 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > >  create mode 100644 drivers/media/i2c/ov2659.c
> > >  create mode 100644 include/media/ov2659.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > > b/Documentation/devicetree/bindings/media/i2c/ov2659.txt new file mode
> > > 100644
> > > index 0000000..a655500
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > > @@ -0,0 +1,38 @@
> > > +* OV2659 1/5-Inch 2Mp SOC Camera
> > > +
> > > +The Omnivision OV2659 is a 1/5-inch SOC camera, with an active array size
> > > of +1632H x 1212V. It is programmable through a SCCB. The OV2659 sensor
> > > supports +multiple resolutions output, such as UXGA, SVGA, 720p. It also
> > > can support +YUV422, RGB565/555 or raw RGB output formats.
> > > +
> > > +Required Properties:
> > > +- compatible: Must be "ovti,ov2659"
> > > +- reg: I2C slave address
> > > +- clocks: reference to the xvclk input clock.
> > > +- clock-names: should be "xvclk".
> > > +- link-frequencies: target pixel clock frequency.
> > > +
> > > +For further reading on port node refer to
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +
> > > +Example:
> > > +
> > > +	i2c0@1c22000 {
> > > +		...
> > > +		...
> > > +		 ov2659@30 {
> > > +			compatible = "ovti,ov2659";
> > > +			reg = <0x30>;
> > > +
> > > +			clocks = <&clk_ov2659 0>;
> > > +			clock-names = "xvclk";
> > > +
> > > +			port {
> > > +				ov2659_0: endpoint {
> > > +					remote-endpoint = <&vpfe_ep>;
> > > +					link-frequencies = <70000000>;
> > 
> > link-frequencies = /bits/ 64 <70000000>;
> > 
> > > +				};
> > > +			};
> > > +		};
> > > +		...
> > > +	};
> 
> [snip]
> 
> > > new file mode 100644
> > > index 0000000..487cb19
> > > --- /dev/null
> > > +++ b/include/media/ov2659.h
> > > @@ -0,0 +1,33 @@
> > > +/*
> > > + * Omnivision OV2659 CMOS Image Sensor driver
> > > + *
> > > + * Copyright (C) 2015 Texas Instruments, Inc.
> > > + *
> > > + * Benoit Parrot <bparrot@xxxxxx>
> > > + * Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
> > > + *
> > > + * This program is free software; you may redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; version 2 of the License.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > > + * SOFTWARE.
> > > + */
> > > +
> > > +#ifndef OV2659_H
> > > +#define OV2659_H
> > > +
> > > +/**
> > > + * struct ov2659_platform_data - ov2659 driver platform data
> > > + * @link_frequency: target pixel clock frequency
> > > + */
> > > +struct ov2659_platform_data {
> > > +	unsigned int link_frequency;
> > 
> > Shouldn't you have xvclk_frequency here as well? In most cases you need to
> > set explicitly to a certain value in order to achieve the required link
> > frequency.
> 
> I'm not sure about that. In the DT case setting the xvclk clock frequency is 
> done outside of the driver (through assigned-clock-rates for instance). 
> Wouldn't it make sense to apply the same logic for non-DT platforms and let 
> the platform configure the clock ?

Good point. However, there may be other users of the clock that choose
different frequencies. The assigned-clock-rates sets the *default* frequency
of the clock. It could be later on changed.

The PLL calculation is very sensitive to the external clock freqnency, so
whatever is done, the frequency must always be what was intended. Explicitly
setting it ensures this whereas relying on the default not having been
changed is risky at least.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/