subject:"Re\: \[PATCH v5 2\/2\] media\: Add a driver for the ov5645 camera sensor."

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

2016-09-21 Thread Sakari Ailus

Hi Todor,

Todor Tomov wrote:
> Hi Sakari,
> 
> One more question below:
> 
> On 08/25/2016 10:18 AM, Sakari Ailus wrote:
> +
> +static struct v4l2_subdev_core_ops ov5645_core_ops = {
> + .s_power = ov5645_s_power,
> +};
> +
> +static struct v4l2_subdev_video_ops ov5645_video_ops = {
> + .s_stream = ov5645_s_stream,
> +};
> +
> +static struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> + .enum_mbus_code = ov5645_enum_mbus_code,
> + .enum_frame_size = ov5645_enum_frame_size,
> + .get_fmt = ov5645_get_format,
> + .set_fmt = ov5645_set_format,
> + .get_selection = ov5645_get_selection,

 Could you add init_cfg() pad op to initialise the try format and 
 selections?
>>>
>>> Yes, I'll do that.
> 
> If I follow the code correctly this init_cfg() is called whenever the device 
> is opened, right?
> This means that the format will be reset to default values every time the 
> userspace opens the device.
> So I wonder if this is what we really want, or do we want to keep the last 
> set format instead. For
> example if we use media-ctl only to get the current format, this is already 
> enough - it opens the
> subdev node and resets the format to default.

What's getting set in init_cfg() called through open system call is the
try format, not the active format.

Whether to choose the default or current format for the try format for a
file handle was discussed some years ago and we indeed settled to use
the default one, since that's independent of whatever configuration
happened to be set --- quite possibly by a different application. AFAIR.

-- 
Kind regards,

Sakari Ailus
sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

2016-09-21 Thread Todor Tomov

Hi Sakari,

One more question below:

On 08/25/2016 10:18 AM, Sakari Ailus wrote:
 +
 +static struct v4l2_subdev_core_ops ov5645_core_ops = {
 +  .s_power = ov5645_s_power,
 +};
 +
 +static struct v4l2_subdev_video_ops ov5645_video_ops = {
 +  .s_stream = ov5645_s_stream,
 +};
 +
 +static struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 +  .enum_mbus_code = ov5645_enum_mbus_code,
 +  .enum_frame_size = ov5645_enum_frame_size,
 +  .get_fmt = ov5645_get_format,
 +  .set_fmt = ov5645_set_format,
 +  .get_selection = ov5645_get_selection,
>>>
>>> Could you add init_cfg() pad op to initialise the try format and selections?
>>
>> Yes, I'll do that.

If I follow the code correctly this init_cfg() is called whenever the device is 
opened, right?
This means that the format will be reset to default values every time the 
userspace opens the device.
So I wonder if this is what we really want, or do we want to keep the last set 
format instead. For
example if we use media-ctl only to get the current format, this is already 
enough - it opens the
subdev node and resets the format to default.

-- 
Best regards,
Todor Tomov
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

2016-08-25 Thread Sakari Ailus

Hi Todor,

On Thu, Aug 25, 2016 at 04:30:33PM +0300, Todor Tomov wrote:
> Hi Sakari, Rob,
> 
> On 08/25/2016 10:18 AM, Sakari Ailus wrote:
> > Hi Todor,
> > 
> > On Wed, Aug 24, 2016 at 06:24:31PM +0300, Todor Tomov wrote:
> >> Hi Sakari,
> >>
> >> Thanks a lot for the time spent to review the driver!
> > 
> > You're welcome! :-)
> > 
> >> I have a few responses bellow.
> >>
> >>
> >> On 08/24/2016 01:17 PM, Sakari Ailus wrote:
> >>> Hi Todor,
> >>>
> >>> Thank you for the patch. Please see my comments below.
> >>>
> >>> On Fri, Jul 08, 2016 at 05:54:39PM +0300, Todor Tomov wrote:
>  The ov5645 sensor from Omnivision supports up to 2592x1944
>  and CSI2 interface.
> 
>  The driver adds support for the following modes:
>  - 1280x960
>  - 1920x1080
>  - 2592x1944
> 
>  Output format is packed 8bit UYVY.
> 
>  Signed-off-by: Todor Tomov 
>  ---
>   drivers/media/i2c/Kconfig  |   12 +
>   drivers/media/i2c/Makefile |1 +
>   drivers/media/i2c/ov5645.c | 1371 
>  
>   3 files changed, 1384 insertions(+)
>   create mode 100644 drivers/media/i2c/ov5645.c
> 
>  diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>  index 993dc50..0cee05b 100644
>  --- a/drivers/media/i2c/Kconfig
>  +++ b/drivers/media/i2c/Kconfig
>  @@ -500,6 +500,18 @@ config VIDEO_OV2659
> To compile this driver as a module, choose M here: the
> module will be called ov2659.
>   
>  +config VIDEO_OV5645
>  +tristate "OmniVision OV5645 sensor support"
>  +depends on OF
>  +depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>  +depends on MEDIA_CAMERA_SUPPORT
>  +---help---
>  +  This is a Video4Linux2 sensor-level driver for the OmniVision
>  +  OV5645 camera.
>  +
>  +  To compile this driver as a module, choose M here: the
>  +  module will be called ov5645.
>  +
>   config VIDEO_OV7640
>   tristate "OmniVision OV7640 sensor support"
>   depends on I2C && VIDEO_V4L2
>  diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>  index 94f2c99..2485aed 100644
>  --- a/drivers/media/i2c/Makefile
>  +++ b/drivers/media/i2c/Makefile
>  @@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
>   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_OV5645) += ov5645.o
>   obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
>   obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>   obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
>  diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
>  new file mode 100644
>  index 000..ec96d10
>  --- /dev/null
>  +++ b/drivers/media/i2c/ov5645.c
>  @@ -0,0 +1,1371 @@
>  +/*
>  + * Driver for the OV5645 camera sensor.
>  + *
>  + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
>  + * Copyright (C) 2015 By Tech Design S.L. All Rights Reserved.
>  + * Copyright (C) 2012-2013 Freescale Semiconductor, Inc. All Rights 
>  Reserved.
>  + *
>  + * Based on:
>  + * - the OV5645 driver from QC msm-3.10 kernel on codeaurora.org:
>  + *   
>  https://us.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/
>  + *   
>  media/platform/msm/camera_v2/sensor/ov5645.c?h=LA.BR.1.2.4_rb1.41
>  + * - the OV5640 driver posted on linux-media:
>  + *   
>  https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
>  + */
>  +
>  +/*
>  + * 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.
>  +
>  + * This program is distributed in the hope that it will be useful,
>  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  + * GNU General Public License for more details.
>  + */
>  +
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +#include 
>  +
>  +#define OV5645_VOLTAGE_ANALOG   280
>  +#define OV5645_VOLTAGE_DIGITAL_CORE 150
>  +#define OV5645_VOLTAGE_DIGITAL_IO   180
>  +
>  +#define OV5645_XCLK 2388
> >>>
> >>> Is this really a

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

2016-08-25 Thread Todor Tomov

Hi Sakari, Rob,

On 08/25/2016 10:18 AM, Sakari Ailus wrote:
> Hi Todor,
> 
> On Wed, Aug 24, 2016 at 06:24:31PM +0300, Todor Tomov wrote:
>> Hi Sakari,
>>
>> Thanks a lot for the time spent to review the driver!
> 
> You're welcome! :-)
> 
>> I have a few responses bellow.
>>
>>
>> On 08/24/2016 01:17 PM, Sakari Ailus wrote:
>>> Hi Todor,
>>>
>>> Thank you for the patch. Please see my comments below.
>>>
>>> On Fri, Jul 08, 2016 at 05:54:39PM +0300, Todor Tomov wrote:
 The ov5645 sensor from Omnivision supports up to 2592x1944
 and CSI2 interface.

 The driver adds support for the following modes:
 - 1280x960
 - 1920x1080
 - 2592x1944

 Output format is packed 8bit UYVY.

 Signed-off-by: Todor Tomov 
 ---
  drivers/media/i2c/Kconfig  |   12 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/ov5645.c | 1371 
 
  3 files changed, 1384 insertions(+)
  create mode 100644 drivers/media/i2c/ov5645.c

 diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
 index 993dc50..0cee05b 100644
 --- a/drivers/media/i2c/Kconfig
 +++ b/drivers/media/i2c/Kconfig
 @@ -500,6 +500,18 @@ config VIDEO_OV2659
  To compile this driver as a module, choose M here: the
  module will be called ov2659.
  
 +config VIDEO_OV5645
 +  tristate "OmniVision OV5645 sensor support"
 +  depends on OF
 +  depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
 +  depends on MEDIA_CAMERA_SUPPORT
 +  ---help---
 +This is a Video4Linux2 sensor-level driver for the OmniVision
 +OV5645 camera.
 +
 +To compile this driver as a module, choose M here: the
 +module will be called ov5645.
 +
  config VIDEO_OV7640
tristate "OmniVision OV7640 sensor support"
depends on I2C && VIDEO_V4L2
 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
 index 94f2c99..2485aed 100644
 --- a/drivers/media/i2c/Makefile
 +++ b/drivers/media/i2c/Makefile
 @@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
  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_OV5645) += ov5645.o
  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
 diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
 new file mode 100644
 index 000..ec96d10
 --- /dev/null
 +++ b/drivers/media/i2c/ov5645.c
 @@ -0,0 +1,1371 @@
 +/*
 + * Driver for the OV5645 camera sensor.
 + *
 + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
 + * Copyright (C) 2015 By Tech Design S.L. All Rights Reserved.
 + * Copyright (C) 2012-2013 Freescale Semiconductor, Inc. All Rights 
 Reserved.
 + *
 + * Based on:
 + * - the OV5645 driver from QC msm-3.10 kernel on codeaurora.org:
 + *   https://us.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/
 + *   media/platform/msm/camera_v2/sensor/ov5645.c?h=LA.BR.1.2.4_rb1.41
 + * - the OV5640 driver posted on linux-media:
 + *   
 https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
 + */
 +
 +/*
 + * 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.
 +
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +
 +#define OV5645_VOLTAGE_ANALOG   280
 +#define OV5645_VOLTAGE_DIGITAL_CORE 150
 +#define OV5645_VOLTAGE_DIGITAL_IO   180
 +
 +#define OV5645_XCLK   2388
>>>
>>> Is this really a property of the sensor itself? Shouldn't this go to the DT
>>> instead? And 23,88 MHz seems pretty unusual for an external clock frequency.
>>>
>>> Even if your driver only could use this frequency for now, the DT still
>>> should contain the real board specific frequency.
>>
>> Yes, 23.88MHz is the value of the external clock frequency.
>> The sensor mode settings (the big sensor register settings arrays below)
>> are calculated over

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

2016-08-25 Thread Sakari Ailus

Hi Todor,

On Wed, Aug 24, 2016 at 06:24:31PM +0300, Todor Tomov wrote:
> Hi Sakari,
> 
> Thanks a lot for the time spent to review the driver!

You're welcome! :-)

> I have a few responses bellow.
> 
> 
> On 08/24/2016 01:17 PM, Sakari Ailus wrote:
> > Hi Todor,
> > 
> > Thank you for the patch. Please see my comments below.
> > 
> > On Fri, Jul 08, 2016 at 05:54:39PM +0300, Todor Tomov wrote:
> >> The ov5645 sensor from Omnivision supports up to 2592x1944
> >> and CSI2 interface.
> >>
> >> The driver adds support for the following modes:
> >> - 1280x960
> >> - 1920x1080
> >> - 2592x1944
> >>
> >> Output format is packed 8bit UYVY.
> >>
> >> Signed-off-by: Todor Tomov 
> >> ---
> >>  drivers/media/i2c/Kconfig  |   12 +
> >>  drivers/media/i2c/Makefile |1 +
> >>  drivers/media/i2c/ov5645.c | 1371 
> >> 
> >>  3 files changed, 1384 insertions(+)
> >>  create mode 100644 drivers/media/i2c/ov5645.c
> >>
> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> >> index 993dc50..0cee05b 100644
> >> --- a/drivers/media/i2c/Kconfig
> >> +++ b/drivers/media/i2c/Kconfig
> >> @@ -500,6 +500,18 @@ config VIDEO_OV2659
> >>  To compile this driver as a module, choose M here: the
> >>  module will be called ov2659.
> >>  
> >> +config VIDEO_OV5645
> >> +  tristate "OmniVision OV5645 sensor support"
> >> +  depends on OF
> >> +  depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> >> +  depends on MEDIA_CAMERA_SUPPORT
> >> +  ---help---
> >> +This is a Video4Linux2 sensor-level driver for the OmniVision
> >> +OV5645 camera.
> >> +
> >> +To compile this driver as a module, choose M here: the
> >> +module will be called ov5645.
> >> +
> >>  config VIDEO_OV7640
> >>tristate "OmniVision OV7640 sensor support"
> >>depends on I2C && VIDEO_V4L2
> >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> >> index 94f2c99..2485aed 100644
> >> --- a/drivers/media/i2c/Makefile
> >> +++ b/drivers/media/i2c/Makefile
> >> @@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
> >>  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_OV5645) += ov5645.o
> >>  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
> >>  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
> >>  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
> >> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> >> new file mode 100644
> >> index 000..ec96d10
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/ov5645.c
> >> @@ -0,0 +1,1371 @@
> >> +/*
> >> + * Driver for the OV5645 camera sensor.
> >> + *
> >> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
> >> + * Copyright (C) 2015 By Tech Design S.L. All Rights Reserved.
> >> + * Copyright (C) 2012-2013 Freescale Semiconductor, Inc. All Rights 
> >> Reserved.
> >> + *
> >> + * Based on:
> >> + * - the OV5645 driver from QC msm-3.10 kernel on codeaurora.org:
> >> + *   https://us.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/
> >> + *   media/platform/msm/camera_v2/sensor/ov5645.c?h=LA.BR.1.2.4_rb1.41
> >> + * - the OV5640 driver posted on linux-media:
> >> + *   
> >> https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
> >> + */
> >> +
> >> +/*
> >> + * 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.
> >> +
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +#define OV5645_VOLTAGE_ANALOG   280
> >> +#define OV5645_VOLTAGE_DIGITAL_CORE 150
> >> +#define OV5645_VOLTAGE_DIGITAL_IO   180
> >> +
> >> +#define OV5645_XCLK   2388
> > 
> > Is this really a property of the sensor itself? Shouldn't this go to the DT
> > instead? And 23,88 MHz seems pretty unusual for an external clock frequency.
> > 
> > Even if your driver only could use this frequency for now, the DT still
> > should contain the real board specific frequency.
> 
> Yes, 23.88MHz is the value of the external clock frequency.
> The sensor mode settings (the big sensor register settings arrays below)
> are calculated over this value. Changing the external clock frequency
> implies different sensor

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

2016-08-24 Thread Todor Tomov

Hi Sakari,

Thanks a lot for the time spent to review the driver!
I have a few responses bellow.


On 08/24/2016 01:17 PM, Sakari Ailus wrote:
> Hi Todor,
> 
> Thank you for the patch. Please see my comments below.
> 
> On Fri, Jul 08, 2016 at 05:54:39PM +0300, Todor Tomov wrote:
>> The ov5645 sensor from Omnivision supports up to 2592x1944
>> and CSI2 interface.
>>
>> The driver adds support for the following modes:
>> - 1280x960
>> - 1920x1080
>> - 2592x1944
>>
>> Output format is packed 8bit UYVY.
>>
>> Signed-off-by: Todor Tomov 
>> ---
>>  drivers/media/i2c/Kconfig  |   12 +
>>  drivers/media/i2c/Makefile |1 +
>>  drivers/media/i2c/ov5645.c | 1371 
>> 
>>  3 files changed, 1384 insertions(+)
>>  create mode 100644 drivers/media/i2c/ov5645.c
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 993dc50..0cee05b 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -500,6 +500,18 @@ config VIDEO_OV2659
>>To compile this driver as a module, choose M here: the
>>module will be called ov2659.
>>  
>> +config VIDEO_OV5645
>> +tristate "OmniVision OV5645 sensor support"
>> +depends on OF
>> +depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> +depends on MEDIA_CAMERA_SUPPORT
>> +---help---
>> +  This is a Video4Linux2 sensor-level driver for the OmniVision
>> +  OV5645 camera.
>> +
>> +  To compile this driver as a module, choose M here: the
>> +  module will be called ov5645.
>> +
>>  config VIDEO_OV7640
>>  tristate "OmniVision OV7640 sensor support"
>>  depends on I2C && VIDEO_V4L2
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 94f2c99..2485aed 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
>>  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_OV5645) += ov5645.o
>>  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
>>  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>>  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
>> new file mode 100644
>> index 000..ec96d10
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov5645.c
>> @@ -0,0 +1,1371 @@
>> +/*
>> + * Driver for the OV5645 camera sensor.
>> + *
>> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
>> + * Copyright (C) 2015 By Tech Design S.L. All Rights Reserved.
>> + * Copyright (C) 2012-2013 Freescale Semiconductor, Inc. All Rights 
>> Reserved.
>> + *
>> + * Based on:
>> + * - the OV5645 driver from QC msm-3.10 kernel on codeaurora.org:
>> + *   https://us.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/
>> + *   media/platform/msm/camera_v2/sensor/ov5645.c?h=LA.BR.1.2.4_rb1.41
>> + * - the OV5640 driver posted on linux-media:
>> + *   
>> https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
>> + */
>> +
>> +/*
>> + * 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.
>> +
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define OV5645_VOLTAGE_ANALOG   280
>> +#define OV5645_VOLTAGE_DIGITAL_CORE 150
>> +#define OV5645_VOLTAGE_DIGITAL_IO   180
>> +
>> +#define OV5645_XCLK 2388
> 
> Is this really a property of the sensor itself? Shouldn't this go to the DT
> instead? And 23,88 MHz seems pretty unusual for an external clock frequency.
> 
> Even if your driver only could use this frequency for now, the DT still
> should contain the real board specific frequency.

Yes, 23.88MHz is the value of the external clock frequency.
The sensor mode settings (the big sensor register settings arrays below)
are calculated over this value. Changing the external clock frequency
implies different sensor mode settings. However, the sensor mode settings
come from the reference driver by QC so we don't actually have a way to
change them and I doubt that we will ever have.

So both the external clock frequency and the sensor mode settings are
hardcoded in the driver. I have also discussed this with Rob Herring
when he reviewed the 1/2 patch

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

2016-08-24 Thread Sakari Ailus

Hi Todor,

Thank you for the patch. Please see my comments below.

On Fri, Jul 08, 2016 at 05:54:39PM +0300, Todor Tomov wrote:
> The ov5645 sensor from Omnivision supports up to 2592x1944
> and CSI2 interface.
> 
> The driver adds support for the following modes:
> - 1280x960
> - 1920x1080
> - 2592x1944
> 
> Output format is packed 8bit UYVY.
> 
> Signed-off-by: Todor Tomov 
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov5645.c | 1371 
> 
>  3 files changed, 1384 insertions(+)
>  create mode 100644 drivers/media/i2c/ov5645.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 993dc50..0cee05b 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -500,6 +500,18 @@ config VIDEO_OV2659
> To compile this driver as a module, choose M here: the
> module will be called ov2659.
>  
> +config VIDEO_OV5645
> + tristate "OmniVision OV5645 sensor support"
> + depends on OF
> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on MEDIA_CAMERA_SUPPORT
> + ---help---
> +   This is a Video4Linux2 sensor-level driver for the OmniVision
> +   OV5645 camera.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ov5645.
> +
>  config VIDEO_OV7640
>   tristate "OmniVision OV7640 sensor support"
>   depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 94f2c99..2485aed 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
>  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_OV5645) += ov5645.o
>  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
>  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> new file mode 100644
> index 000..ec96d10
> --- /dev/null
> +++ b/drivers/media/i2c/ov5645.c
> @@ -0,0 +1,1371 @@
> +/*
> + * Driver for the OV5645 camera sensor.
> + *
> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2015 By Tech Design S.L. All Rights Reserved.
> + * Copyright (C) 2012-2013 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * Based on:
> + * - the OV5645 driver from QC msm-3.10 kernel on codeaurora.org:
> + *   https://us.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/
> + *   media/platform/msm/camera_v2/sensor/ov5645.c?h=LA.BR.1.2.4_rb1.41
> + * - the OV5640 driver posted on linux-media:
> + *   https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
> + */
> +
> +/*
> + * 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.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define OV5645_VOLTAGE_ANALOG   280
> +#define OV5645_VOLTAGE_DIGITAL_CORE 150
> +#define OV5645_VOLTAGE_DIGITAL_IO   180
> +
> +#define OV5645_XCLK  2388

Is this really a property of the sensor itself? Shouldn't this go to the DT
instead? And 23,88 MHz seems pretty unusual for an external clock frequency.

Even if your driver only could use this frequency for now, the DT still
should contain the real board specific frequency.

> +
> +#define OV5645_SYSTEM_CTRL0  0x3008

Some of the definitions here are obviously register addresses but only some
have "_REG" suffix. I'd add that to all of them. Up to you.

> +#define  OV5645_SYSTEM_CTRL0_START   0x02
> +#define  OV5645_SYSTEM_CTRL0_STOP0x42
> +#define OV5645_CHIP_ID_HIGH_REG  0x300A
> +#define  OV5645_CHIP_ID_HIGH 0x56
> +#define OV5645_CHIP_ID_LOW_REG   0x300B
> +#define  OV5645_CHIP_ID_LOW  0x45
> +#define OV5645_AWB_MANUAL_CONTROL0x3406
> +#define  OV5645_AWB_MANUAL_ENABLEBIT(0)
> +#define OV5645_AEC_PK_MANUAL 0x3503
> +#define  OV5645_AEC_MANUAL_ENABLEBIT(0)
> +#define  OV5645_AGC_MANUAL_ENABLEBIT(1)
>

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

Re: [PATCH v5 2/2] media: Add a driver for the ov5645 camera sensor.

7 matches

Site Navigation

Mail list logo

Footer information