Hi Sakari,
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 <steve_longerbeam@mentor.com> --- 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 0000000..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 <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/ctype.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h> +#include <media/v4l2-async.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> +#include <media/v4l2-of.h>Could you rebase this on the V4L2 fwnode patchset here, please? <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi>
<snip>+ +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?
Making the write in two transactions will make it non-atomic that could be an issue in some corner cases.
<snip>+ +static int ov5640_set_gain(struct ov5640_dev *sensor, int auto_gain) +{ + struct ov5640_ctrls *ctrls = &sensor->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?
<snip>+ +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(&sensor->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.
<snip>+ +static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) +{ + struct ov5640_dev *sensor = to_ov5640_dev(sd); + int ret = 0; + + mutex_lock(&sensor->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.
<snip>+ +free_ctrls: + v4l2_ctrl_handler_free(&sensor->ctrls.handler); +entity_cleanup: + mutex_destroy(&sensor->lock); + media_entity_cleanup(&sensor->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.