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 <todor.tomov@xxxxxxxxxx> > >>>> --- > >>>> 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 0000000..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 <linux/bitops.h> > >>>> +#include <linux/clk.h> > >>>> +#include <linux/delay.h> > >>>> +#include <linux/device.h> > >>>> +#include <linux/gpio/consumer.h> > >>>> +#include <linux/i2c.h> > >>>> +#include <linux/init.h> > >>>> +#include <linux/module.h> > >>>> +#include <linux/of.h> > >>>> +#include <linux/of_graph.h> > >>>> +#include <linux/regulator/consumer.h> > >>>> +#include <linux/slab.h> > >>>> +#include <linux/types.h> > >>>> +#include <media/v4l2-ctrls.h> > >>>> +#include <media/v4l2-of.h> > >>>> +#include <media/v4l2-subdev.h> > >>>> + > >>>> +#define OV5645_VOLTAGE_ANALOG 2800000 > >>>> +#define OV5645_VOLTAGE_DIGITAL_CORE 1500000 > >>>> +#define OV5645_VOLTAGE_DIGITAL_IO 1800000 > >>>> + > >>>> +#define OV5645_XCLK 23880000 > >>> > >>> 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 and we came to this conclusion. > > > > It still isn't a property of the sensor. I'd put the frequency to the DT, so > > that if support for more frequencies is added, no hacks will be needed. > > Ok, I can add a property in th DT for the external clock frequency but also > will add a comment in the driver that the frequency currently supported is > 23.88MHz only. Sounds good to me. > > However I'd like to hear also Rob's opinion on this as he is the one > who already acknowledged the dt binding. > > > <snip> > > >>>> +static int ov5645_s_power(struct v4l2_subdev *sd, int on) > >>>> +{ > >>>> + struct ov5645 *ov5645 = to_ov5645(sd); > >>>> + int ret = 0; > >>>> + > >>>> + dev_dbg(ov5645->dev, "%s: on = %d\n", __func__, on); > >>>> + > >>>> + mutex_lock(&ov5645->power_lock); > >>>> + > >>>> + if (ov5645->power == !on) { > >>>> + /* Power state changes. */ > >>>> + if (on) { > >>>> + ret = ov5645_set_power_on(ov5645); > >>>> + if (ret < 0) { > >>>> + dev_err(ov5645->dev, "could not set power %s\n", > >>>> + on ? "on" : "off"); > >>>> + goto exit; > >>>> + } > >>>> + > >>>> + ret = ov5645_init(ov5645); > >>>> + if (ret < 0) { > >>>> + dev_err(ov5645->dev, > >>>> + "could not set init registers\n"); > >>>> + ov5645_set_power_off(ov5645); > >>>> + goto exit; > >>>> + } > >>>> + > >>>> + ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > >>>> + OV5645_SYSTEM_CTRL0_STOP); > >>> > >>> Is there a change that the sensor was streaming at this point? > >> > >> I'm not sure, but this is the startup sequence which is used in the reference driver > >> from QC so I've followed it. > > > > I suppose that'd only be the case if streaming was enabled by the register > > list. That'd be a bug most probably. > > I cannot say whether it is a bug or not. > This init settings usually come from the camera sensor vendor and you just follow > them - I guess this is what QC did when preparing their driver. But they have put > this stop command after the init explicitly so I believe they were told to do it > that way. Ok. > > > > >> > >>> > >>>> + if (ret < 0) { > >>>> + ov5645_set_power_off(ov5645); > >>>> + goto exit; > >>>> + } > >>>> + } else { > >>>> + ov5645_set_power_off(ov5645); > >>>> + } > >>>> + > >>>> + /* Update the power state. */ > >>>> + ov5645->power = on ? true : false; > >>>> + } > >>>> + > >>>> +exit: > >>>> + mutex_unlock(&ov5645->power_lock); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> + > > <snip> > > >>>> +static int ov5645_set_format(struct v4l2_subdev *sd, > >>>> + struct v4l2_subdev_pad_config *cfg, > >>>> + struct v4l2_subdev_format *format) > >>>> +{ > >>>> + struct ov5645 *ov5645 = to_ov5645(sd); > >>>> + struct v4l2_mbus_framefmt *__format; > >>>> + struct v4l2_rect *__crop; > >>>> + enum ov5645_mode new_mode; > >>>> + > >>>> + __crop = __ov5645_get_pad_crop(ov5645, cfg, format->pad, > >>>> + format->which); > >>>> + > >>>> + new_mode = ov5645_find_nearest_mode(ov5645, > >>>> + format->format.width, format->format.height); > >>> > >>> Do you think you could you use v4l2_find_nearest_format() instead of making > >>> your own function for the purpose? > >> > >> v4l2_find_nearest_format() doesn't quite fit with the current implementation > >> which stores the current mode in "enum ov5645_mode current_mode". > >> Is it recommended to use it? > > > > I guess if you can't reasonably use it then don't, but then it raises the > > question whether there's something wrong with the function. I think so. It > > should be able to associate driver specific data to each resolution. > > I'm not sure that I understand what you mean here. The newly found mode is > set in ov5645->current_mode which is actually an index in the ov5645_mode_info_data > array, which contains the settings for this resolution/mode. The idea is that v4l2_find_nearest_format() should be usable for drivers to search the closest suitable configuration for a given size. If the function isn't up to the task, there probably is something wrong with it. It'd be nice to fix that, if there's something wrong with the implementation. > > > > >> > >>> > >>>> + __crop->width = ov5645_mode_info_data[new_mode].width; > >>>> + __crop->height = ov5645_mode_info_data[new_mode].height; > >>>> + > >>>> + ov5645->current_mode = new_mode; > >>>> + > >>>> + __format = __ov5645_get_pad_format(ov5645, cfg, format->pad, > >>>> + format->which); > >>>> + __format->width = __crop->width; > >>>> + __format->height = __crop->height; > >>>> + > >>>> + format->format = *__format; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > > <snip> -- Regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
