Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-07 Thread Sakari Ailus 
On Sat, Jun 03, 2017 at 09:46:36PM -0700, Steve Longerbeam wrote:
> 
> 
> On 06/03/2017 02:57 PM, Sakari Ailus wrote:
> >On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote:
> >>Hi!
> >>
> >>+   /* Auto/manual exposure */
> >>+   ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> >>+V4L2_CID_EXPOSURE_AUTO,
> >>+V4L2_EXPOSURE_MANUAL, 
> >>0,
> >>+V4L2_EXPOSURE_AUTO);
> >>+   ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> >>+   V4L2_CID_EXPOSURE_ABSOLUTE,
> >>+   0, 65535, 1, 0);
> >
> >Is exposure_absolute supposed to be in microseconds...?
> 
> Yes.
> >>>
> >>>According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> >>>
> >>>  OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> >>>
> >>>Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> >>>taking 100 usec units, so unit-less is better.
> >>
> >>Thanks. If you know the units, it would be of course better to use
> >>right units...
> >
> >Steve: what's the unit in this case? Is it lines or something else?
> 
> Yes, the register interface for exposure takes lines*16.
> 
> Maybe converting from seconds to lines is as simple as
> framerate * height * seconds. But I'm not sure about that.

The smiapp and a few other drivers are using lines. One option could be to
use lines as the unit and have step as 16.

Then the hblank + vblank controls will be needed, too, to enable the user to
at least figure out the conversion to Si units.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-07 Thread Sakari Ailus 
On Sun, Jun 04, 2017 at 11:00:14AM -0700, Steve Longerbeam wrote:
> 
> 
> On 06/03/2017 11:02 AM, Steve Longerbeam wrote:
> >Hi Sakari,
> >
> >
> >On 05/29/2017 11:56 PM, Sakari Ailus wrote:
> >>Hi Steve,
> >>
> >>On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:
> 
> 
> >+
> >+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >+{
> >+struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> >+struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >+int ret = 0;
> >+
> >+mutex_lock(>lock);
> Could you use the same lock for the controls as you use for the
> rest? Just
> setting handler->lock after handler init does the trick.
> >>>
> >>>Can you please rephrase, I don't follow. "same lock for the controls as
> >>>you use for the rest" - there's only one device lock owned by this
> >>>driver
> >>>and I am already using that same lock.
> >>
> >>There's another in the control handler. You could use your own lock for
> >>the
> >>control handler as well.
> >
> >I still don't understand.
> >
> 
> Hi Sakari, sorry I see what you are referring to now. The lock
> in 'struct v4l2_ctrl_handler' can be overridden by a caller's own
> lock. Yes that's a good idea, I'll do that.

Ack, good! :-)

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-04 Thread Steve Longerbeam 



On 06/03/2017 11:02 AM, Steve Longerbeam wrote:

Hi Sakari,


On 05/29/2017 11:56 PM, Sakari Ailus wrote:

Hi Steve,

On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:




+
+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+struct ov5640_dev *sensor = to_ov5640_dev(sd);
+int ret = 0;
+
+mutex_lock(>lock);
Could you use the same lock for the controls as you use for the 
rest? Just

setting handler->lock after handler init does the trick.


Can you please rephrase, I don't follow. "same lock for the controls as
you use for the rest" - there's only one device lock owned by this 
driver

and I am already using that same lock.


There's another in the control handler. You could use your own lock 
for the

control handler as well.


I still don't understand.



Hi Sakari, sorry I see what you are referring to now. The lock
in 'struct v4l2_ctrl_handler' can be overridden by a caller's own
lock. Yes that's a good idea, I'll do that.

Steve

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-03 Thread Steve Longerbeam 



On 06/03/2017 02:57 PM, Sakari Ailus wrote:

On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote:

Hi!


+   /* Auto/manual exposure */
+   ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
+V4L2_CID_EXPOSURE_AUTO,
+V4L2_EXPOSURE_MANUAL, 0,
+V4L2_EXPOSURE_AUTO);
+   ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
+   V4L2_CID_EXPOSURE_ABSOLUTE,
+   0, 65535, 1, 0);


Is exposure_absolute supposed to be in microseconds...?


Yes.


According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.

  OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.

Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.


Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
taking 100 usec units, so unit-less is better.


Thanks. If you know the units, it would be of course better to use
right units...


Steve: what's the unit in this case? Is it lines or something else?


Yes, the register interface for exposure takes lines*16.

Maybe converting from seconds to lines is as simple as
framerate * height * seconds. But I'm not sure about that.

Steve



Pavel: we do need to make sure the user space will be able to know the unit,
too. It's rather a case with a number of controls: the unit is known but
there's no API to convey it to the user.

The exposure is a bit special, too: granularity matters a lot on small
values. On most other controls it does not.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-03 Thread Pavel Machek 
Hi!

> > > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> > > 
> > >  OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> > > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> > > 
> > > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> > > taking 100 usec units, so unit-less is better.
> > 
> > Thanks. If you know the units, it would be of course better to use
> > right units...
> 
> Steve: what's the unit in this case? Is it lines or something else?
> 
> Pavel: we do need to make sure the user space will be able to know the unit,
> too. It's rather a case with a number of controls: the unit is known but
> there's no API to convey it to the user.
> 
> The exposure is a bit special, too: granularity matters a lot on small
> values. On most other controls it does not.

Yeah. Basically problem with exposure is that the control is
logarithmic; by using linear scale we got too much resolution at long
times and too little resolution at short times.

(Plus, 100 usec ... n900 can do times _way_ shorter than that.)

Anyway, even u32 gives us enough range, but I so the linear/log
confusion does not matter. But it would be nicer if values were in 10
usec or usec, not 100 usec... 

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-03 Thread Sakari Ailus 
On Sat, Jun 03, 2017 at 09:51:39PM +0200, Pavel Machek wrote:
> Hi!
> 
> > >>>+/* Auto/manual exposure */
> > >>>+ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > >>>+ V4L2_CID_EXPOSURE_AUTO,
> > >>>+ V4L2_EXPOSURE_MANUAL, 
> > >>>0,
> > >>>+ V4L2_EXPOSURE_AUTO);
> > >>>+ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > >>>+V4L2_CID_EXPOSURE_ABSOLUTE,
> > >>>+0, 65535, 1, 0);
> > >>
> > >>Is exposure_absolute supposed to be in microseconds...?
> > >
> > >Yes.
> > 
> > According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> > 
> >  OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> > >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> > 
> > Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> > taking 100 usec units, so unit-less is better.
> 
> Thanks. If you know the units, it would be of course better to use
> right units...

Steve: what's the unit in this case? Is it lines or something else?

Pavel: we do need to make sure the user space will be able to know the unit,
too. It's rather a case with a number of controls: the unit is known but
there's no API to convey it to the user.

The exposure is a bit special, too: granularity matters a lot on small
values. On most other controls it does not.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-03 Thread Pavel Machek 
Hi!

> >>>+  /* Auto/manual exposure */
> >>>+  ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> >>>+   V4L2_CID_EXPOSURE_AUTO,
> >>>+   V4L2_EXPOSURE_MANUAL, 0,
> >>>+   V4L2_EXPOSURE_AUTO);
> >>>+  ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> >>>+  V4L2_CID_EXPOSURE_ABSOLUTE,
> >>>+  0, 65535, 1, 0);
> >>
> >>Is exposure_absolute supposed to be in microseconds...?
> >
> >Yes.
> 
> According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.
> 
>  OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> >Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> 
> Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
> taking 100 usec units, so unit-less is better.

Thanks. If you know the units, it would be of course better to use
right units...

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-03 Thread Steve Longerbeam 



On 06/01/2017 01:26 AM, Sakari Ailus wrote:

Hi Pavel,

On Wed, May 31, 2017 at 09:58:21PM +0200, Pavel Machek wrote:

Hi!


+/* min/typical/max system clock (xclk) frequencies */
+#define OV5640_XCLK_MIN  600
+#define OV5640_XCLK_MAX 2400
+
+/*
+ * FIXME: there is no subdev API to set the MIPI CSI-2
+ * virtual channel yet, so this is hardcoded for now.
+ */
+#define OV5640_MIPI_VC 1


Can the FIXME be fixed?


Yes, but it's quite a bit of work. It makes sense to use a static virtual
channel for now. A patchset which is however incomplete can be found here:



For what it's worth, all other devices use virtual channel zero for image
data and so should this one.



Actually no. The CSI2IPU gasket in i.MX6 quad sends virtual channel 0
streams to IPU1-CSI0. But input to IPU1-CSI0 is also muxed with parallel
bus cameras. So if vc0 were chosen instead, platforms that support
parallel cameras to IPU1-CSI0 (SabreLite, SabreSD) would not be able
to use them concurrently with a MIPI CSI-2 source to IPU1-CSI1. So I
prefer to use static channel 1 to support those platforms.

I could convert this to a module parameter however, until a virtual
channel selection subdev API becomes available, at which point that
would have to be stripped.







+/*
+ * image size under 1280 * 960 are SUBSAMPLING


-> Image


+ * image size upper 1280 * 960 are SCALING


above?




done.


+/*
+ * FIXME: all of these register tables are likely filled with
+ * entries that set the register to their power-on default values,
+ * and which are otherwise not touched by this driver. Those entries
+ * should be identified and removed to speed register load time
+ * over i2c.
+ */


load->loading? Can the FIXME be fixed?


That's a lot of work, and risky work at that. If someone could take
this on (strip out power-on default values from the tables), I'd
be grateful, but I don't have the time. For now at least, these
registers sets work fine.





+   /* Auto/manual exposure */
+   ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
+V4L2_CID_EXPOSURE_AUTO,
+V4L2_EXPOSURE_MANUAL, 0,
+V4L2_EXPOSURE_AUTO);
+   ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
+   V4L2_CID_EXPOSURE_ABSOLUTE,
+   0, 65535, 1, 0);


Is exposure_absolute supposed to be in microseconds...?


Yes.


According to the docs V4L2_CID_EXPOSURE_ABSOLUTE is in 100 usec units.

 OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.

Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.


Done, switched to V4L2_CID_EXPOSURE. It's true, this control is not
taking 100 usec units, so unit-less is better.


Steve
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-03 Thread Steve Longerbeam 

Hi Sakari,


On 05/29/2017 11:56 PM, Sakari Ailus wrote:

Hi Steve,

On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:




+
+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+   struct ov5640_dev *sensor = to_ov5640_dev(sd);
+   int ret = 0;
+
+   mutex_lock(>lock);

Could you use the same lock for the controls as you use for the rest? Just
setting handler->lock after handler init does the trick.


Can you please rephrase, I don't follow. "same lock for the controls as
you use for the rest" - there's only one device lock owned by this driver
and I am already using that same lock.


There's another in the control handler. You could use your own lock for the
control handler as well.


I still don't understand.









+
+static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
+{
+   struct ov5640_dev *sensor = to_ov5640_dev(sd);
+   int ret = 0;
+
+   mutex_lock(>lock);
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+   if (sd->entity.stream_count > 1)

The entity stream_count isn't connected to the number of times s_stream(sd,
true) is called. Please remove the check.


It's incremented by media_pipeline_start(), even if the entity is already
a member of the given pipeline.

I added this check because in imx-media, the ov5640 can be streaming
concurrently to multiple video capture devices, and each capture device
calls
media_pipeline_start() at stream on, which increments the entity stream
count.

So if one capture device issues a stream off while others are still
streaming,
ov5640 should remain at stream on. So the entity stream count is being
used as a streaming usage counter. Is there a better way to do this? Should
I use a private stream use counter instead?


Different drivers may use media_pipeline_start() in different ways. Stream
control shouldn't depend on that count. This could cause issues in using the
driver with other ISP / receiver drivers.

I think it should be enough to move the check to the imx driver in this
case.



I will remove this check.



+
+static int ov5640_remove(struct i2c_client *client)
+{
+   struct v4l2_subdev *sd = i2c_get_clientdata(client);
+   struct ov5640_dev *sensor = to_ov5640_dev(sd);
+
+   regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);

Ditto.


I don't understand. regulator_bulk_disable() is still needed, am I missing
something?


You still need to enable it first. I don't see that being done in probe. As
the driver implements the s_power() op, I don't see a need for powering the
device on at probe time (and conversely off at remove time).


Oh you're right, it must have been left over from a previous revision
I guess. Yes, regulator_bulk_enable|disable() is only called in
ov5640_set_power(). I'll remove regulator_bulk_disable() from
probe/remove.

Steve


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

exposure vs. exposure_absolute was Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-01 Thread Pavel Machek 
Hi!

> > > + /* Auto/manual exposure */
> > > + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > > +  V4L2_CID_EXPOSURE_AUTO,
> > > +  V4L2_EXPOSURE_MANUAL, 0,
> > > +  V4L2_EXPOSURE_AUTO);
> > > + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > > + V4L2_CID_EXPOSURE_ABSOLUTE,
> > > + 0, 65535, 1, 0);
> > 
> > Is exposure_absolute supposed to be in microseconds...?
> 
> Yes. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
> Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.
> 
> Ideally we should have only one control for exposure.

No. N-o. No no no. NO! No. N-o. NONONO. No. NooOOO!

Sorry, no.

Userspace needs to know exposure times. It is not so important for a
webcam, but it is mandatory for digital camera. As it gets darker,
autogain wants to scale exposure to cca 1/100 sec, then it wants to
scale gain up to maximum, and only then it wants to continue scaling
exposure. (Threshold will be shorter in "sports" mode, perhaps
1/300sec?)

Plus, we want user to be able to manually set exposure parameters.

So... _this_ driver probably should use V4L2_CID_EXPOSURE. (If the
units are not known). But in general we'd prefer drivers using
V4L2_CID_EXPOSURE_ABSOLUTE. Your car has speedometer calibrated in
km/h or mph, not in "% of max", right?

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-06-01 Thread Sakari Ailus 
Hi Pavel,

On Wed, May 31, 2017 at 09:58:21PM +0200, Pavel Machek wrote:
> Hi!
> 
> > +/* min/typical/max system clock (xclk) frequencies */
> > +#define OV5640_XCLK_MIN  600
> > +#define OV5640_XCLK_MAX 2400
> > +
> > +/*
> > + * FIXME: there is no subdev API to set the MIPI CSI-2
> > + * virtual channel yet, so this is hardcoded for now.
> > + */
> > +#define OV5640_MIPI_VC 1
> 
> Can the FIXME be fixed?

Yes, but it's quite a bit of work. It makes sense to use a static virtual
channel for now. A patchset which is however incomplete can be found here:



For what it's worth, all other devices use virtual channel zero for image
data and so should this one.

> 
> > +/*
> > + * image size under 1280 * 960 are SUBSAMPLING
> 
> -> Image
> 
> > + * image size upper 1280 * 960 are SCALING
> 
> above?
> 
> > +/*
> > + * FIXME: all of these register tables are likely filled with
> > + * entries that set the register to their power-on default values,
> > + * and which are otherwise not touched by this driver. Those entries
> > + * should be identified and removed to speed register load time
> > + * over i2c.
> > + */
> 
> load->loading? Can the FIXME be fixed?
> 
> > +   /* Auto/manual exposure */
> > +   ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> > +V4L2_CID_EXPOSURE_AUTO,
> > +V4L2_EXPOSURE_MANUAL, 0,
> > +V4L2_EXPOSURE_AUTO);
> > +   ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> > +   V4L2_CID_EXPOSURE_ABSOLUTE,
> > +   0, 65535, 1, 0);
> 
> Is exposure_absolute supposed to be in microseconds...?

Yes. OTOH V4L2_CID_EXPOSURE has no defined unit, so it's a better fit IMO.
Way more drivers appear to be using EXPOSURE than EXPOSURE_ABSOLUTE, too.

Ideally we should have only one control for exposure.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-05-31 Thread Pavel Machek 
Hi!

> +/* min/typical/max system clock (xclk) frequencies */
> +#define OV5640_XCLK_MIN  600
> +#define OV5640_XCLK_MAX 2400
> +
> +/*
> + * FIXME: there is no subdev API to set the MIPI CSI-2
> + * virtual channel yet, so this is hardcoded for now.
> + */
> +#define OV5640_MIPI_VC   1

Can the FIXME be fixed?

> +/*
> + * image size under 1280 * 960 are SUBSAMPLING

-> Image

> + * image size upper 1280 * 960 are SCALING

above?

> +/*
> + * FIXME: all of these register tables are likely filled with
> + * entries that set the register to their power-on default values,
> + * and which are otherwise not touched by this driver. Those entries
> + * should be identified and removed to speed register load time
> + * over i2c.
> + */

load->loading? Can the FIXME be fixed?

> + /* Auto/manual exposure */
> + ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> +  V4L2_CID_EXPOSURE_AUTO,
> +  V4L2_EXPOSURE_MANUAL, 0,
> +  V4L2_EXPOSURE_AUTO);
> + ctrls->exposure = v4l2_ctrl_new_std(hdl, ops,
> + V4L2_CID_EXPOSURE_ABSOLUTE,
> + 0, 65535, 1, 0);

Is exposure_absolute supposed to be in microseconds...?


> + /* Auto/manual gain */
> + ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
> +  0, 1, 1, 1);
> + ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
> + 0, 1023, 1, 0);
> +
> + ctrls->saturation = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION,
> +   0, 255, 1, 64);
> + ctrls->hue = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HUE,
> +0, 359, 1, 0);
> + ctrls->contrast = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST,
> + 0, 255, 1, 0);
> + ctrls->test_pattern =
> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> +  ARRAY_SIZE(test_pattern_menu) - 1,
> +  0, 0, test_pattern_menu);
> +

It is good to see sensor that has autogain/etc. I'm emulating them in
v4l-utils, and hardware that supports it is a good argument. 

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-05-30 Thread Sakari Ailus 
Hi Steve,

On Mon, May 29, 2017 at 02:50:34PM -0700, Steve Longerbeam wrote:
> >
> >
> >>+
> >>+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >>+{
> >>+   struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> >>+   struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >>+   int ret = 0;
> >>+
> >>+   mutex_lock(>lock);
> >Could you use the same lock for the controls as you use for the rest? Just
> >setting handler->lock after handler init does the trick.
> 
> Can you please rephrase, I don't follow. "same lock for the controls as
> you use for the rest" - there's only one device lock owned by this driver
> and I am already using that same lock.

There's another in the control handler. You could use your own lock for the
control handler as well.

> 
> 
> >
> >>+
> >>+static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
> >>+{
> >>+   struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >>+   int ret = 0;
> >>+
> >>+   mutex_lock(>lock);
> >>+
> >>+#if defined(CONFIG_MEDIA_CONTROLLER)
> >>+   if (sd->entity.stream_count > 1)
> >The entity stream_count isn't connected to the number of times s_stream(sd,
> >true) is called. Please remove the check.
> 
> It's incremented by media_pipeline_start(), even if the entity is already
> a member of the given pipeline.
> 
> I added this check because in imx-media, the ov5640 can be streaming
> concurrently to multiple video capture devices, and each capture device
> calls
> media_pipeline_start() at stream on, which increments the entity stream
> count.
> 
> So if one capture device issues a stream off while others are still
> streaming,
> ov5640 should remain at stream on. So the entity stream count is being
> used as a streaming usage counter. Is there a better way to do this? Should
> I use a private stream use counter instead?

Different drivers may use media_pipeline_start() in different ways. Stream
control shouldn't depend on that count. This could cause issues in using the
driver with other ISP / receiver drivers.

I think it should be enough to move the check to the imx driver in this
case.

> 
> 
> 
> >
> >
> >>+
> >>+free_ctrls:
> >>+   v4l2_ctrl_handler_free(>ctrls.handler);
> >>+entity_cleanup:
> >>+   mutex_destroy(>lock);
> >>+   media_entity_cleanup(>sd.entity);
> >>+   regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
> >Should this still be here?
> >
> >>+   return ret;
> >>+}
> >>+
> >>+static int ov5640_remove(struct i2c_client *client)
> >>+{
> >>+   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >>+   struct ov5640_dev *sensor = to_ov5640_dev(sd);
> >>+
> >>+   regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
> >Ditto.
> 
> I don't understand. regulator_bulk_disable() is still needed, am I missing
> something?

You still need to enable it first. I don't see that being done in probe. As
the driver implements the s_power() op, I don't see a need for powering the
device on at probe time (and conversely off at remove time).

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-05-29 Thread Steve Longerbeam 

Hi Sakari,


On 05/29/2017 08:55 AM, Sakari Ailus wrote:

Hi Steve,

A few comments below.

On Wed, May 24, 2017 at 05:29:31PM -0700, Steve Longerbeam wrote:

This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
branch, modified heavily to bring forward to latest interfaces and code
cleanup.

Signed-off-by: Steve Longerbeam
---
  drivers/media/i2c/Kconfig  |9 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov5640.c | 2224 
  3 files changed, 2234 insertions(+)
  create mode 100644 drivers/media/i2c/ov5640.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd181c9..ff082a7 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -539,6 +539,15 @@ config VIDEO_OV2659
  To compile this driver as a module, choose M here: the
  module will be called ov2659.
  
+config VIDEO_OV5640

+   tristate "OmniVision OV5640 sensor support"
+   depends on OF
+   depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the Omnivision
+ OV5640 camera sensor with a MIPI CSI-2 interface.
+
  config VIDEO_OV5645
tristate "OmniVision OV5645 sensor support"
depends on OF
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 62323ec..dc6b0c4 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
+obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
  obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
  obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
new file mode 100644
index 000..2a032bc
--- /dev/null
+++ b/drivers/media/i2c/ov5640.c
@@ -0,0 +1,2224 @@
+/*
+ * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright (C) 2014-2017 Mentor Graphics Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 

Could you rebase this on the V4L2 fwnode patchset here, please?




Once the fwnode patchset hits mediatree, then yes it can be
converted along with all the others under media/i2c.





+
+static int ov5640_write_reg16(struct ov5640_dev *sensor, u16 reg, u16 val)
+{
+   int ret;
+
+   ret = ov5640_write_reg(sensor, reg, val >> 8);
+   if (ret)
+   return ret;
+
+   return ov5640_write_reg(sensor, reg + 1, val & 0xff);

Does the sensor datasheet suggest doing this?


Why would the datasheet suggest or not suggest such things?
Coding details like this don't belong in the datasheet.


  Making the write in two
transactions will make it non-atomic that could be an issue in some corner
cases.


It's called everywhere under the same device mutex.





+
+static int ov5640_set_gain(struct ov5640_dev *sensor, int auto_gain)
+{
+   struct ov5640_ctrls *ctrls = >ctrls;
+
+   if (ctrls->auto_gain->is_new) {
+   ov5640_mod_reg(sensor, OV5640_REG_AEC_PK_MANUAL,
+  BIT(1), ctrls->auto_gain->val ? 0 : BIT(1));

You're generally silently ignoring all I²C access errors. Is that
intentional?


Yeah, this driver is much cleaned up from the original, but there are
still some issues like this. The register access errors are really only
being paid attention to during s_power() when loading the initial
register set, which is enough at least to catch a non-existent chip
or basic i2c bus or other hardware issues. But I should work on
catching all access errors. This is something I did in an earlier rev
but I used a questionable short-cut to make it easier to implement.
I'll just have to catch every case one by one.






+
+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+   struct ov5640_dev *sensor = to_ov5640_dev(sd);
+   int ret = 0;
+
+   mutex_lock(>lock);

Could you use the same lock for the controls as you use for the rest? Just
setting handler->lock after handler init does the trick.


Can you please rephrase, I don't follow. "same lock for the controls as
you use for the rest" - there's only one device lock owned by this driver
and I am already using that same

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-05-29 Thread Sakari Ailus 
Hi Steve,

A few comments below.

On Wed, May 24, 2017 at 05:29:31PM -0700, Steve Longerbeam wrote:
> This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
> branch, modified heavily to bring forward to latest interfaces and code
> cleanup.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/media/i2c/Kconfig  |9 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov5640.c | 2224 
> 
>  3 files changed, 2234 insertions(+)
>  create mode 100644 drivers/media/i2c/ov5640.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index fd181c9..ff082a7 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -539,6 +539,15 @@ config VIDEO_OV2659
> To compile this driver as a module, choose M here: the
> module will be called ov2659.
>  
> +config VIDEO_OV5640
> + tristate "OmniVision OV5640 sensor support"
> + depends on OF
> + depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> + depends on MEDIA_CAMERA_SUPPORT
> + ---help---
> +   This is a Video4Linux2 sensor-level driver for the Omnivision
> +   OV5640 camera sensor with a MIPI CSI-2 interface.
> +
>  config VIDEO_OV5645
>   tristate "OmniVision OV5645 sensor support"
>   depends on OF
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 62323ec..dc6b0c4 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
>  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
>  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
> +obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
>  obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
>  obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
>  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> new file mode 100644
> index 000..2a032bc
> --- /dev/null
> +++ b/drivers/media/i2c/ov5640.c
> @@ -0,0 +1,2224 @@
> +/*
> + * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
> + * Copyright (C) 2014-2017 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Could you rebase this on the V4L2 fwnode patchset here, please?



> +#include 
> +
> +/* min/typical/max system clock (xclk) frequencies */
> +#define OV5640_XCLK_MIN  600
> +#define OV5640_XCLK_MAX 2400
> +
> +/*
> + * FIXME: there is no subdev API to set the MIPI CSI-2
> + * virtual channel yet, so this is hardcoded for now.
> + */
> +#define OV5640_MIPI_VC   1
> +
> +#define OV5640_DEFAULT_SLAVE_ID 0x3c
> +
> +#define OV5640_REG_CHIP_ID   0x300a
> +#define OV5640_REG_PAD_OUTPUT00  0x3019
> +#define OV5640_REG_SC_PLL_CTRL0  0x3034
> +#define OV5640_REG_SC_PLL_CTRL1  0x3035
> +#define OV5640_REG_SC_PLL_CTRL2  0x3036
> +#define OV5640_REG_SC_PLL_CTRL3  0x3037
> +#define OV5640_REG_SLAVE_ID  0x3100
> +#define OV5640_REG_SYS_ROOT_DIVIDER  0x3108
> +#define OV5640_REG_AWB_R_GAIN0x3400
> +#define OV5640_REG_AWB_G_GAIN0x3402
> +#define OV5640_REG_AWB_B_GAIN0x3404
> +#define OV5640_REG_AWB_MANUAL_CTRL   0x3406
> +#define OV5640_REG_AEC_PK_EXPOSURE_HI0x3500
> +#define OV5640_REG_AEC_PK_EXPOSURE_MED   0x3501
> +#define OV5640_REG_AEC_PK_EXPOSURE_LO0x3502
> +#define OV5640_REG_AEC_PK_MANUAL 0x3503
> +#define OV5640_REG_AEC_PK_REAL_GAIN  0x350a
> +#define OV5640_REG_AEC_PK_VTS0x350c
> +#define OV5640_REG_TIMING_HTS0x380c
> +#define OV5640_REG_TIMING_VTS0x380e
> +#define OV5640_REG_TIMING_TC_REG21   0x3821
> +#define OV5640_REG_AEC_CTRL000x3a00
> +#define OV5640_REG_AEC_B50_STEP  0x3a08
> +#define OV5640_REG_AEC_B60_STEP  0x3a0a
> +#define OV5640_REG_AEC_CTRL0D0x3a0d
> +#define OV5640_REG_AEC_CTRL0E0x3a0e
> +#define OV5640_REG_AEC_CTRL0F0x3a0f
> +#define OV5640_REG_AEC_CTRL100x3a10
> +#define OV5640_REG_AEC_CTRL110x3a11
> +#define OV5640_REG_AEC_CTRL1B0x3a1b
> +#define OV5640_REG_AEC_CTRL1E0x3a1e
> +#define OV5640_REG_AEC_CTRL1F0x3a1f
> +#define

Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-05-29 Thread Hans Verkuil 

On 05/25/2017 02:29 AM, Steve Longerbeam wrote:

This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
branch, modified heavily to bring forward to latest interfaces and code
cleanup.

Signed-off-by: Steve Longerbeam 


Acked-by: Hans Verkuil 

Thanks,

Hans


---
  drivers/media/i2c/Kconfig  |9 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov5640.c | 2224 
  3 files changed, 2234 insertions(+)
  create mode 100644 drivers/media/i2c/ov5640.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd181c9..ff082a7 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -539,6 +539,15 @@ config VIDEO_OV2659
  To compile this driver as a module, choose M here: the
  module will be called ov2659.
  
+config VIDEO_OV5640

+   tristate "OmniVision OV5640 sensor support"
+   depends on OF
+   depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the Omnivision
+ OV5640 camera sensor with a MIPI CSI-2 interface.
+
  config VIDEO_OV5645
tristate "OmniVision OV5645 sensor support"
depends on OF
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 62323ec..dc6b0c4 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
+obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
  obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
  obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
new file mode 100644
index 000..2a032bc
--- /dev/null
+++ b/drivers/media/i2c/ov5640.c
@@ -0,0 +1,2224 @@
+/*
+ * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright (C) 2014-2017 Mentor Graphics Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* min/typical/max system clock (xclk) frequencies */
+#define OV5640_XCLK_MIN  600
+#define OV5640_XCLK_MAX 2400
+
+/*
+ * FIXME: there is no subdev API to set the MIPI CSI-2
+ * virtual channel yet, so this is hardcoded for now.
+ */
+#define OV5640_MIPI_VC 1
+
+#define OV5640_DEFAULT_SLAVE_ID 0x3c
+
+#define OV5640_REG_CHIP_ID 0x300a
+#define OV5640_REG_PAD_OUTPUT000x3019
+#define OV5640_REG_SC_PLL_CTRL00x3034
+#define OV5640_REG_SC_PLL_CTRL10x3035
+#define OV5640_REG_SC_PLL_CTRL20x3036
+#define OV5640_REG_SC_PLL_CTRL30x3037
+#define OV5640_REG_SLAVE_ID0x3100
+#define OV5640_REG_SYS_ROOT_DIVIDER0x3108
+#define OV5640_REG_AWB_R_GAIN  0x3400
+#define OV5640_REG_AWB_G_GAIN  0x3402
+#define OV5640_REG_AWB_B_GAIN  0x3404
+#define OV5640_REG_AWB_MANUAL_CTRL 0x3406
+#define OV5640_REG_AEC_PK_EXPOSURE_HI  0x3500
+#define OV5640_REG_AEC_PK_EXPOSURE_MED 0x3501
+#define OV5640_REG_AEC_PK_EXPOSURE_LO  0x3502
+#define OV5640_REG_AEC_PK_MANUAL   0x3503
+#define OV5640_REG_AEC_PK_REAL_GAIN0x350a
+#define OV5640_REG_AEC_PK_VTS  0x350c
+#define OV5640_REG_TIMING_HTS  0x380c
+#define OV5640_REG_TIMING_VTS  0x380e
+#define OV5640_REG_TIMING_TC_REG21 0x3821
+#define OV5640_REG_AEC_CTRL00  0x3a00
+#define OV5640_REG_AEC_B50_STEP0x3a08
+#define OV5640_REG_AEC_B60_STEP0x3a0a
+#define OV5640_REG_AEC_CTRL0D  0x3a0d
+#define OV5640_REG_AEC_CTRL0E  0x3a0e
+#define OV5640_REG_AEC_CTRL0F  0x3a0f
+#define OV5640_REG_AEC_CTRL10  0x3a10
+#define OV5640_REG_AEC_CTRL11  0x3a11
+#define OV5640_REG_AEC_CTRL1B  0x3a1b
+#define OV5640_REG_AEC_CTRL1E  0x3a1e
+#define OV5640_REG_AEC_CTRL1F  0x3a1f
+#define OV5640_REG_HZ5060_CTRL00   0x3c00
+#define OV5640_REG_HZ5060_CTRL01   0x3c01
+#define OV5640_REG_SIGMADELTA_CTRL0C   0x3c0c
+#define OV5640_REG_FRAME_CTRL010x4202
+#define OV5640_REG_MIPI_CTRL00 0x4800
+#define OV5640_REG_DEBUG_MODE  0x4814
+#define OV5640_REG_PRE_ISP_TEST_SET1   0x503d
+#define OV5640_REG_SDE_CTRL0   0x5580
+#define OV5640_REG_SDE_CTRL1   0x5581
+#define

[PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver

2017-05-24 Thread Steve Longerbeam 
This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
branch, modified heavily to bring forward to latest interfaces and code
cleanup.

Signed-off-by: Steve Longerbeam 
---
 drivers/media/i2c/Kconfig  |9 +
 drivers/media/i2c/Makefile |1 +
 drivers/media/i2c/ov5640.c | 2224 
 3 files changed, 2234 insertions(+)
 create mode 100644 drivers/media/i2c/ov5640.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd181c9..ff082a7 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -539,6 +539,15 @@ config VIDEO_OV2659
  To compile this driver as a module, choose M here: the
  module will be called ov2659.
 
+config VIDEO_OV5640
+   tristate "OmniVision OV5640 sensor support"
+   depends on OF
+   depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the Omnivision
+ OV5640 camera sensor with a MIPI CSI-2 interface.
+
 config VIDEO_OV5645
tristate "OmniVision OV5645 sensor support"
depends on OF
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 62323ec..dc6b0c4 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
 obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
 obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
 obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
+obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
 obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
 obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
 obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
new file mode 100644
index 000..2a032bc
--- /dev/null
+++ b/drivers/media/i2c/ov5640.c
@@ -0,0 +1,2224 @@
+/*
+ * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright (C) 2014-2017 Mentor Graphics Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* min/typical/max system clock (xclk) frequencies */
+#define OV5640_XCLK_MIN  600
+#define OV5640_XCLK_MAX 2400
+
+/*
+ * FIXME: there is no subdev API to set the MIPI CSI-2
+ * virtual channel yet, so this is hardcoded for now.
+ */
+#define OV5640_MIPI_VC 1
+
+#define OV5640_DEFAULT_SLAVE_ID 0x3c
+
+#define OV5640_REG_CHIP_ID 0x300a
+#define OV5640_REG_PAD_OUTPUT000x3019
+#define OV5640_REG_SC_PLL_CTRL00x3034
+#define OV5640_REG_SC_PLL_CTRL10x3035
+#define OV5640_REG_SC_PLL_CTRL20x3036
+#define OV5640_REG_SC_PLL_CTRL30x3037
+#define OV5640_REG_SLAVE_ID0x3100
+#define OV5640_REG_SYS_ROOT_DIVIDER0x3108
+#define OV5640_REG_AWB_R_GAIN  0x3400
+#define OV5640_REG_AWB_G_GAIN  0x3402
+#define OV5640_REG_AWB_B_GAIN  0x3404
+#define OV5640_REG_AWB_MANUAL_CTRL 0x3406
+#define OV5640_REG_AEC_PK_EXPOSURE_HI  0x3500
+#define OV5640_REG_AEC_PK_EXPOSURE_MED 0x3501
+#define OV5640_REG_AEC_PK_EXPOSURE_LO  0x3502
+#define OV5640_REG_AEC_PK_MANUAL   0x3503
+#define OV5640_REG_AEC_PK_REAL_GAIN0x350a
+#define OV5640_REG_AEC_PK_VTS  0x350c
+#define OV5640_REG_TIMING_HTS  0x380c
+#define OV5640_REG_TIMING_VTS  0x380e
+#define OV5640_REG_TIMING_TC_REG21 0x3821
+#define OV5640_REG_AEC_CTRL00  0x3a00
+#define OV5640_REG_AEC_B50_STEP0x3a08
+#define OV5640_REG_AEC_B60_STEP0x3a0a
+#define OV5640_REG_AEC_CTRL0D  0x3a0d
+#define OV5640_REG_AEC_CTRL0E  0x3a0e
+#define OV5640_REG_AEC_CTRL0F  0x3a0f
+#define OV5640_REG_AEC_CTRL10  0x3a10
+#define OV5640_REG_AEC_CTRL11  0x3a11
+#define OV5640_REG_AEC_CTRL1B  0x3a1b
+#define OV5640_REG_AEC_CTRL1E  0x3a1e
+#define OV5640_REG_AEC_CTRL1F  0x3a1f
+#define OV5640_REG_HZ5060_CTRL00   0x3c00
+#define OV5640_REG_HZ5060_CTRL01   0x3c01
+#define OV5640_REG_SIGMADELTA_CTRL0C   0x3c0c
+#define OV5640_REG_FRAME_CTRL010x4202
+#define OV5640_REG_MIPI_CTRL00 0x4800
+#define OV5640_REG_DEBUG_MODE  0x4814
+#define OV5640_REG_PRE_ISP_TEST_SET1   0x503d
+#define OV5640_REG_SDE_CTRL0   0x5580
+#define OV5640_REG_SDE_CTRL1   0x5581
+#define OV5640_REG_SDE_CTRL3   0x5583
+#define OV5640_REG_SDE_CTRL4   0x5584
+#define OV5640_REG_SDE_CTRL5   0x5585
+#define