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

From: Steve Longerbeam
Date: Mon May 29 2017 - 17:50:48 EST

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<steve_longerbeam@xxxxxxxxxx>
---
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>

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

<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?

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.


<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?

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.


<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.

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.


<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.

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?



<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.

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

Steve