From mboxrd@z Thu Jan 1 00:00:00 1970 From: slongerbeam@gmail.com (Steve Longerbeam) Date: Mon, 29 May 2017 14:50:34 -0700 Subject: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver In-Reply-To: <20170529155511.GI29527@valkosipuli.retiisi.org.uk> References: <1495672189-29164-1-git-send-email-steve_longerbeam@mentor.com> <1495672189-29164-17-git-send-email-steve_longerbeam@mentor.com> <20170529155511.GI29527@valkosipuli.retiisi.org.uk> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 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 >> +#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 = &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. > > >> + >> +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. > >> + >> +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? > > >> + >> +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