From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751194AbdE2Vuk (ORCPT ); Mon, 29 May 2017 17:50:40 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:34407 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750951AbdE2Vui (ORCPT ); Mon, 29 May 2017 17:50:38 -0400 From: Steve Longerbeam Subject: Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver To: Sakari Ailus Cc: robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org, kernel@pengutronix.de, fabio.estevam@nxp.com, linux@armlinux.org.uk, mchehab@kernel.org, hverkuil@xs4all.nl, nick@shmanahar.org, markus.heiser@darmarIT.de, p.zabel@pengutronix.de, laurent.pinchart+renesas@ideasonboard.com, bparrot@ti.com, geert@linux-m68k.org, arnd@arndb.de, sudipm.mukherjee@gmail.com, minghsiu.tsai@mediatek.com, tiffany.lin@mediatek.com, jean-christophe.trotin@st.com, horms+renesas@verge.net.au, niklas.soderlund+renesas@ragnatech.se, robert.jarzmik@free.fr, songjun.wu@microchip.com, andrew-ct.chen@mediatek.com, gregkh@linuxfoundation.org, shuah@kernel.org, sakari.ailus@linux.intel.com, pavel@ucw.cz, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, devel@driverdev.osuosl.org, Steve Longerbeam 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: Date: Mon, 29 May 2017 14:50:34 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170529155511.GI29527@valkosipuli.retiisi.org.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Longerbeam Subject: Re: [PATCH v7 16/34] [media] add Omnivision OV5640 sensor driver Date: Mon, 29 May 2017 14:50:34 -0700 Message-ID: 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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170529155511.GI29527@valkosipuli.retiisi.org.uk> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Sakari Ailus Cc: mark.rutland@arm.com, andrew-ct.chen@mediatek.com, minghsiu.tsai@mediatek.com, sakari.ailus@linux.intel.com, nick@shmanahar.org, songjun.wu@microchip.com, hverkuil@xs4all.nl, Steve Longerbeam , pavel@ucw.cz, robert.jarzmik@free.fr, devel@driverdev.osuosl.org, markus.heiser@darmarIT.de, laurent.pinchart+renesas@ideasonboard.com, shuah@kernel.org, linux@armlinux.org.uk, geert@linux-m68k.org, linux-media@vger.kernel.org, devicetree@vger.kernel.org, kernel@pengutronix.de, arnd@arndb.de, mchehab@kernel.org, bparrot@ti.com, robh+dt@kernel.org, horms+renesas@verge.net.au, tiffany.lin@mediatek.com, linux-arm-kernel@lists.infradead.org, niklas.soderlund+renesas@ragnatech.se, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, jean-christophe.trotin@st.com, p.zabel@pengutronix.de, fabio.estevam@nxp.com, shawnguo@kernel.org, sudipm.mukherjee@gmail List-Id: devicetree@vger.kernel.org SGkgU2FrYXJpLAoKCk9uIDA1LzI5LzIwMTcgMDg6NTUgQU0sIFNha2FyaSBBaWx1cyB3cm90ZToK PiBIaSBTdGV2ZSwKPgo+IEEgZmV3IGNvbW1lbnRzIGJlbG93Lgo+Cj4gT24gV2VkLCBNYXkgMjQs IDIwMTcgYXQgMDU6Mjk6MzFQTSAtMDcwMCwgU3RldmUgTG9uZ2VyYmVhbSB3cm90ZToKPj4gVGhp cyBkcml2ZXIgaXMgYmFzZWQgb24gb3Y1NjQwX21pcGkuYyBmcm9tIEZyZWVzY2FsZSBpbXhfMy4x MC4xN18xLjAuMF9iZXRhCj4+IGJyYW5jaCwgbW9kaWZpZWQgaGVhdmlseSB0byBicmluZyBmb3J3 YXJkIHRvIGxhdGVzdCBpbnRlcmZhY2VzIGFuZCBjb2RlCj4+IGNsZWFudXAuCj4+Cj4+IFNpZ25l ZC1vZmYtYnk6IFN0ZXZlIExvbmdlcmJlYW08c3RldmVfbG9uZ2VyYmVhbUBtZW50b3IuY29tPgo+ PiAtLS0KPj4gICBkcml2ZXJzL21lZGlhL2kyYy9LY29uZmlnICB8ICAgIDkgKwo+PiAgIGRyaXZl cnMvbWVkaWEvaTJjL01ha2VmaWxlIHwgICAgMSArCj4+ICAgZHJpdmVycy9tZWRpYS9pMmMvb3Y1 NjQwLmMgfCAyMjI0ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr Cj4+ICAgMyBmaWxlcyBjaGFuZ2VkLCAyMjM0IGluc2VydGlvbnMoKykKPj4gICBjcmVhdGUgbW9k ZSAxMDA2NDQgZHJpdmVycy9tZWRpYS9pMmMvb3Y1NjQwLmMKPj4KPj4gZGlmZiAtLWdpdCBhL2Ry aXZlcnMvbWVkaWEvaTJjL0tjb25maWcgYi9kcml2ZXJzL21lZGlhL2kyYy9LY29uZmlnCj4+IGlu ZGV4IGZkMTgxYzkuLmZmMDgyYTcgMTAwNjQ0Cj4+IC0tLSBhL2RyaXZlcnMvbWVkaWEvaTJjL0tj b25maWcKPj4gKysrIGIvZHJpdmVycy9tZWRpYS9pMmMvS2NvbmZpZwo+PiBAQCAtNTM5LDYgKzUz OSwxNSBAQCBjb25maWcgVklERU9fT1YyNjU5Cj4+ICAgCSAgVG8gY29tcGlsZSB0aGlzIGRyaXZl ciBhcyBhIG1vZHVsZSwgY2hvb3NlIE0gaGVyZTogdGhlCj4+ICAgCSAgbW9kdWxlIHdpbGwgYmUg Y2FsbGVkIG92MjY1OS4KPj4gICAKPj4gK2NvbmZpZyBWSURFT19PVjU2NDAKPj4gKwl0cmlzdGF0 ZSAiT21uaVZpc2lvbiBPVjU2NDAgc2Vuc29yIHN1cHBvcnQiCj4+ICsJZGVwZW5kcyBvbiBPRgo+ PiArCWRlcGVuZHMgb24gR1BJT0xJQiAmJiBWSURFT19WNEwyICYmIEkyQyAmJiBWSURFT19WNEwy X1NVQkRFVl9BUEkKPj4gKwlkZXBlbmRzIG9uIE1FRElBX0NBTUVSQV9TVVBQT1JUCj4+ICsJLS0t aGVscC0tLQo+PiArCSAgVGhpcyBpcyBhIFZpZGVvNExpbnV4MiBzZW5zb3ItbGV2ZWwgZHJpdmVy IGZvciB0aGUgT21uaXZpc2lvbgo+PiArCSAgT1Y1NjQwIGNhbWVyYSBzZW5zb3Igd2l0aCBhIE1J UEkgQ1NJLTIgaW50ZXJmYWNlLgo+PiArCj4+ICAgY29uZmlnIFZJREVPX09WNTY0NQo+PiAgIAl0 cmlzdGF0ZSAiT21uaVZpc2lvbiBPVjU2NDUgc2Vuc29yIHN1cHBvcnQiCj4+ICAgCWRlcGVuZHMg b24gT0YKPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbWVkaWEvaTJjL01ha2VmaWxlIGIvZHJpdmVy cy9tZWRpYS9pMmMvTWFrZWZpbGUKPj4gaW5kZXggNjIzMjNlYy4uZGM2YjBjNCAxMDA2NDQKPj4g LS0tIGEvZHJpdmVycy9tZWRpYS9pMmMvTWFrZWZpbGUKPj4gKysrIGIvZHJpdmVycy9tZWRpYS9p MmMvTWFrZWZpbGUKPj4gQEAgLTU4LDYgKzU4LDcgQEAgb2JqLSQoQ09ORklHX1ZJREVPX1NPTllf QlRGX01QWCkgKz0gc29ueS1idGYtbXB4Lm8KPj4gICBvYmotJChDT05GSUdfVklERU9fVVBENjQw MzFBKSArPSB1cGQ2NDAzMWEubwo+PiAgIG9iai0kKENPTkZJR19WSURFT19VUEQ2NDA4MykgKz0g dXBkNjQwODMubwo+PiAgIG9iai0kKENPTkZJR19WSURFT19PVjI2NDApICs9IG92MjY0MC5vCj4+ ICtvYmotJChDT05GSUdfVklERU9fT1Y1NjQwKSArPSBvdjU2NDAubwo+PiAgIG9iai0kKENPTkZJ R19WSURFT19PVjU2NDUpICs9IG92NTY0NS5vCj4+ICAgb2JqLSQoQ09ORklHX1ZJREVPX09WNTY0 NykgKz0gb3Y1NjQ3Lm8KPj4gICBvYmotJChDT05GSUdfVklERU9fT1Y3NjQwKSArPSBvdjc2NDAu bwo+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9tZWRpYS9pMmMvb3Y1NjQwLmMgYi9kcml2ZXJzL21l ZGlhL2kyYy9vdjU2NDAuYwo+PiBuZXcgZmlsZSBtb2RlIDEwMDY0NAo+PiBpbmRleCAwMDAwMDAw Li4yYTAzMmJjCj4+IC0tLSAvZGV2L251bGwKPj4gKysrIGIvZHJpdmVycy9tZWRpYS9pMmMvb3Y1 NjQwLmMKPj4gQEAgLTAsMCArMSwyMjI0IEBACj4+ICsvKgo+PiArICogQ29weXJpZ2h0IChDKSAy MDExLTIwMTMgRnJlZXNjYWxlIFNlbWljb25kdWN0b3IsIEluYy4gQWxsIFJpZ2h0cyBSZXNlcnZl ZC4KPj4gKyAqIENvcHlyaWdodCAoQykgMjAxNC0yMDE3IE1lbnRvciBHcmFwaGljcyBJbmMuCj4+ ICsgKgo+PiArICogVGhpcyBwcm9ncmFtIGlzIGZyZWUgc29mdHdhcmU7IHlvdSBjYW4gcmVkaXN0 cmlidXRlIGl0IGFuZC9vciBtb2RpZnkKPj4gKyAqIGl0IHVuZGVyIHRoZSB0ZXJtcyBvZiB0aGUg R05VIEdlbmVyYWwgUHVibGljIExpY2Vuc2UgYXMgcHVibGlzaGVkIGJ5Cj4+ICsgKiB0aGUgRnJl ZSBTb2Z0d2FyZSBGb3VuZGF0aW9uOyBlaXRoZXIgdmVyc2lvbiAyIG9mIHRoZSBMaWNlbnNlLCBv cgo+PiArICogKGF0IHlvdXIgb3B0aW9uKSBhbnkgbGF0ZXIgdmVyc2lvbi4KPj4gKyAqLwo+PiAr Cj4+ICsjaW5jbHVkZSA8bGludXgvY2xrLmg+Cj4+ICsjaW5jbHVkZSA8bGludXgvY2xrLXByb3Zp ZGVyLmg+Cj4+ICsjaW5jbHVkZSA8bGludXgvY2xrZGV2Lmg+Cj4+ICsjaW5jbHVkZSA8bGludXgv Y3R5cGUuaD4KPj4gKyNpbmNsdWRlIDxsaW51eC9kZWxheS5oPgo+PiArI2luY2x1ZGUgPGxpbnV4 L2RldmljZS5oPgo+PiArI2luY2x1ZGUgPGxpbnV4L2kyYy5oPgo+PiArI2luY2x1ZGUgPGxpbnV4 L2luaXQuaD4KPj4gKyNpbmNsdWRlIDxsaW51eC9tb2R1bGUuaD4KPj4gKyNpbmNsdWRlIDxsaW51 eC9vZl9kZXZpY2UuaD4KPj4gKyNpbmNsdWRlIDxsaW51eC9zbGFiLmg+Cj4+ICsjaW5jbHVkZSA8 bGludXgvdHlwZXMuaD4KPj4gKyNpbmNsdWRlIDxsaW51eC9ncGlvL2NvbnN1bWVyLmg+Cj4+ICsj aW5jbHVkZSA8bGludXgvcmVndWxhdG9yL2NvbnN1bWVyLmg+Cj4+ICsjaW5jbHVkZSA8bWVkaWEv djRsMi1hc3luYy5oPgo+PiArI2luY2x1ZGUgPG1lZGlhL3Y0bDItY3RybHMuaD4KPj4gKyNpbmNs dWRlIDxtZWRpYS92NGwyLWRldmljZS5oPgo+PiArI2luY2x1ZGUgPG1lZGlhL3Y0bDItb2YuaD4K PiBDb3VsZCB5b3UgcmViYXNlIHRoaXMgb24gdGhlIFY0TDIgZndub2RlIHBhdGNoc2V0IGhlcmUs IHBsZWFzZT8KPgo+IDxVUkw6aHR0cHM6Ly9naXQubGludXh0di5vcmcvc2FpbHVzL21lZGlhX3Ry ZWUuZ2l0L2xvZy8/aD12NGwyLWFjcGk+CgpPbmNlIHRoZSBmd25vZGUgcGF0Y2hzZXQgaGl0cyBt ZWRpYXRyZWUsIHRoZW4geWVzIGl0IGNhbiBiZQpjb252ZXJ0ZWQgYWxvbmcgd2l0aCBhbGwgdGhl IG90aGVycyB1bmRlciBtZWRpYS9pMmMuCgo+IDxzbmlwPgo+Cj4+ICsKPj4gK3N0YXRpYyBpbnQg b3Y1NjQwX3dyaXRlX3JlZzE2KHN0cnVjdCBvdjU2NDBfZGV2ICpzZW5zb3IsIHUxNiByZWcsIHUx NiB2YWwpCj4+ICt7Cj4+ICsJaW50IHJldDsKPj4gKwo+PiArCXJldCA9IG92NTY0MF93cml0ZV9y ZWcoc2Vuc29yLCByZWcsIHZhbCA+PiA4KTsKPj4gKwlpZiAocmV0KQo+PiArCQlyZXR1cm4gcmV0 Owo+PiArCj4+ICsJcmV0dXJuIG92NTY0MF93cml0ZV9yZWcoc2Vuc29yLCByZWcgKyAxLCB2YWwg JiAweGZmKTsKPiBEb2VzIHRoZSBzZW5zb3IgZGF0YXNoZWV0IHN1Z2dlc3QgZG9pbmcgdGhpcz8K CldoeSB3b3VsZCB0aGUgZGF0YXNoZWV0IHN1Z2dlc3Qgb3Igbm90IHN1Z2dlc3Qgc3VjaCB0aGlu Z3M/CkNvZGluZyBkZXRhaWxzIGxpa2UgdGhpcyBkb24ndCBiZWxvbmcgaW4gdGhlIGRhdGFzaGVl dC4KCj4gICBNYWtpbmcgdGhlIHdyaXRlIGluIHR3bwo+IHRyYW5zYWN0aW9ucyB3aWxsIG1ha2Ug aXQgbm9uLWF0b21pYyB0aGF0IGNvdWxkIGJlIGFuIGlzc3VlIGluIHNvbWUgY29ybmVyCj4gY2Fz ZXMuCgpJdCdzIGNhbGxlZCBldmVyeXdoZXJlIHVuZGVyIHRoZSBzYW1lIGRldmljZSBtdXRleC4K Cgo+IDxzbmlwPgo+PiArCj4+ICtzdGF0aWMgaW50IG92NTY0MF9zZXRfZ2FpbihzdHJ1Y3Qgb3Y1 NjQwX2RldiAqc2Vuc29yLCBpbnQgYXV0b19nYWluKQo+PiArewo+PiArCXN0cnVjdCBvdjU2NDBf Y3RybHMgKmN0cmxzID0gJnNlbnNvci0+Y3RybHM7Cj4+ICsKPj4gKwlpZiAoY3RybHMtPmF1dG9f Z2Fpbi0+aXNfbmV3KSB7Cj4+ICsJCW92NTY0MF9tb2RfcmVnKHNlbnNvciwgT1Y1NjQwX1JFR19B RUNfUEtfTUFOVUFMLAo+PiArCQkJICAgICAgIEJJVCgxKSwgY3RybHMtPmF1dG9fZ2Fpbi0+dmFs ID8gMCA6IEJJVCgxKSk7Cj4gWW91J3JlIGdlbmVyYWxseSBzaWxlbnRseSBpZ25vcmluZyBhbGwg ScKyQyBhY2Nlc3MgZXJyb3JzLiBJcyB0aGF0Cj4gaW50ZW50aW9uYWw/CgpZZWFoLCB0aGlzIGRy aXZlciBpcyBtdWNoIGNsZWFuZWQgdXAgZnJvbSB0aGUgb3JpZ2luYWwsIGJ1dCB0aGVyZSBhcmUK c3RpbGwgc29tZSBpc3N1ZXMgbGlrZSB0aGlzLiBUaGUgcmVnaXN0ZXIgYWNjZXNzIGVycm9ycyBh cmUgcmVhbGx5IG9ubHkKYmVpbmcgcGFpZCBhdHRlbnRpb24gdG8gZHVyaW5nIHNfcG93ZXIoKSB3 aGVuIGxvYWRpbmcgdGhlIGluaXRpYWwKcmVnaXN0ZXIgc2V0LCB3aGljaCBpcyBlbm91Z2ggYXQg bGVhc3QgdG8gY2F0Y2ggYSBub24tZXhpc3RlbnQgY2hpcApvciBiYXNpYyBpMmMgYnVzIG9yIG90 aGVyIGhhcmR3YXJlIGlzc3Vlcy4gQnV0IEkgc2hvdWxkIHdvcmsgb24KY2F0Y2hpbmcgYWxsIGFj Y2VzcyBlcnJvcnMuIFRoaXMgaXMgc29tZXRoaW5nIEkgZGlkIGluIGFuIGVhcmxpZXIgcmV2CmJ1 dCBJIHVzZWQgYSBxdWVzdGlvbmFibGUgc2hvcnQtY3V0IHRvIG1ha2UgaXQgZWFzaWVyIHRvIGlt cGxlbWVudC4KSSdsbCBqdXN0IGhhdmUgdG8gY2F0Y2ggZXZlcnkgY2FzZSBvbmUgYnkgb25lLgoK Cj4gPHNuaXA+Cj4KPj4gKwo+PiArc3RhdGljIGludCBvdjU2NDBfc19jdHJsKHN0cnVjdCB2NGwy X2N0cmwgKmN0cmwpCj4+ICt7Cj4+ICsJc3RydWN0IHY0bDJfc3ViZGV2ICpzZCA9IGN0cmxfdG9f c2QoY3RybCk7Cj4+ICsJc3RydWN0IG92NTY0MF9kZXYgKnNlbnNvciA9IHRvX292NTY0MF9kZXYo c2QpOwo+PiArCWludCByZXQgPSAwOwo+PiArCj4+ICsJbXV0ZXhfbG9jaygmc2Vuc29yLT5sb2Nr KTsKPiBDb3VsZCB5b3UgdXNlIHRoZSBzYW1lIGxvY2sgZm9yIHRoZSBjb250cm9scyBhcyB5b3Ug dXNlIGZvciB0aGUgcmVzdD8gSnVzdAo+IHNldHRpbmcgaGFuZGxlci0+bG9jayBhZnRlciBoYW5k bGVyIGluaXQgZG9lcyB0aGUgdHJpY2suCgpDYW4geW91IHBsZWFzZSByZXBocmFzZSwgSSBkb24n dCBmb2xsb3cuICJzYW1lIGxvY2sgZm9yIHRoZSBjb250cm9scyBhcwp5b3UgdXNlIGZvciB0aGUg cmVzdCIgLSB0aGVyZSdzIG9ubHkgb25lIGRldmljZSBsb2NrIG93bmVkIGJ5IHRoaXMgZHJpdmVy CmFuZCBJIGFtIGFscmVhZHkgdXNpbmcgdGhhdCBzYW1lIGxvY2suCgoKPiA8c25pcD4KPj4gKwo+ PiArc3RhdGljIGludCBvdjU2NDBfc19zdHJlYW0oc3RydWN0IHY0bDJfc3ViZGV2ICpzZCwgaW50 IGVuYWJsZSkKPj4gK3sKPj4gKwlzdHJ1Y3Qgb3Y1NjQwX2RldiAqc2Vuc29yID0gdG9fb3Y1NjQw X2RldihzZCk7Cj4+ICsJaW50IHJldCA9IDA7Cj4+ICsKPj4gKwltdXRleF9sb2NrKCZzZW5zb3It PmxvY2spOwo+PiArCj4+ICsjaWYgZGVmaW5lZChDT05GSUdfTUVESUFfQ09OVFJPTExFUikKPj4g KwlpZiAoc2QtPmVudGl0eS5zdHJlYW1fY291bnQgPiAxKQo+IFRoZSBlbnRpdHkgc3RyZWFtX2Nv dW50IGlzbid0IGNvbm5lY3RlZCB0byB0aGUgbnVtYmVyIG9mIHRpbWVzIHNfc3RyZWFtKHNkLAo+ IHRydWUpIGlzIGNhbGxlZC4gUGxlYXNlIHJlbW92ZSB0aGUgY2hlY2suCgpJdCdzIGluY3JlbWVu dGVkIGJ5IG1lZGlhX3BpcGVsaW5lX3N0YXJ0KCksIGV2ZW4gaWYgdGhlIGVudGl0eSBpcyBhbHJl YWR5CmEgbWVtYmVyIG9mIHRoZSBnaXZlbiBwaXBlbGluZS4KCkkgYWRkZWQgdGhpcyBjaGVjayBi ZWNhdXNlIGluIGlteC1tZWRpYSwgdGhlIG92NTY0MCBjYW4gYmUgc3RyZWFtaW5nCmNvbmN1cnJl bnRseSB0byBtdWx0aXBsZSB2aWRlbyBjYXB0dXJlIGRldmljZXMsIGFuZCBlYWNoIGNhcHR1cmUg ZGV2aWNlIApjYWxscwptZWRpYV9waXBlbGluZV9zdGFydCgpIGF0IHN0cmVhbSBvbiwgd2hpY2gg aW5jcmVtZW50cyB0aGUgZW50aXR5IHN0cmVhbSAKY291bnQuCgpTbyBpZiBvbmUgY2FwdHVyZSBk ZXZpY2UgaXNzdWVzIGEgc3RyZWFtIG9mZiB3aGlsZSBvdGhlcnMgYXJlIHN0aWxsIApzdHJlYW1p bmcsCm92NTY0MCBzaG91bGQgcmVtYWluIGF0IHN0cmVhbSBvbi4gU28gdGhlIGVudGl0eSBzdHJl YW0gY291bnQgaXMgYmVpbmcKdXNlZCBhcyBhIHN0cmVhbWluZyB1c2FnZSBjb3VudGVyLiBJcyB0 aGVyZSBhIGJldHRlciB3YXkgdG8gZG8gdGhpcz8gU2hvdWxkCkkgdXNlIGEgcHJpdmF0ZSBzdHJl YW0gdXNlIGNvdW50ZXIgaW5zdGVhZD8KCgoKPiA8c25pcD4KPgo+PiArCj4+ICtmcmVlX2N0cmxz Ogo+PiArCXY0bDJfY3RybF9oYW5kbGVyX2ZyZWUoJnNlbnNvci0+Y3RybHMuaGFuZGxlcik7Cj4+ ICtlbnRpdHlfY2xlYW51cDoKPj4gKwltdXRleF9kZXN0cm95KCZzZW5zb3ItPmxvY2spOwo+PiAr CW1lZGlhX2VudGl0eV9jbGVhbnVwKCZzZW5zb3ItPnNkLmVudGl0eSk7Cj4+ICsJcmVndWxhdG9y X2J1bGtfZGlzYWJsZShPVjU2NDBfTlVNX1NVUFBMSUVTLCBzZW5zb3ItPnN1cHBsaWVzKTsKPiBT aG91bGQgdGhpcyBzdGlsbCBiZSBoZXJlPwo+Cj4+ICsJcmV0dXJuIHJldDsKPj4gK30KPj4gKwo+ PiArc3RhdGljIGludCBvdjU2NDBfcmVtb3ZlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpCj4+ ICt7Cj4+ICsJc3RydWN0IHY0bDJfc3ViZGV2ICpzZCA9IGkyY19nZXRfY2xpZW50ZGF0YShjbGll bnQpOwo+PiArCXN0cnVjdCBvdjU2NDBfZGV2ICpzZW5zb3IgPSB0b19vdjU2NDBfZGV2KHNkKTsK Pj4gKwo+PiArCXJlZ3VsYXRvcl9idWxrX2Rpc2FibGUoT1Y1NjQwX05VTV9TVVBQTElFUywgc2Vu c29yLT5zdXBwbGllcyk7Cj4gRGl0dG8uCgpJIGRvbid0IHVuZGVyc3RhbmQuIHJlZ3VsYXRvcl9i dWxrX2Rpc2FibGUoKSBpcyBzdGlsbCBuZWVkZWQsIGFtIEkgbWlzc2luZwpzb21ldGhpbmc/CgpT dGV2ZQoKCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpk ZXZlbCBtYWlsaW5nIGxpc3QKZGV2ZWxAbGludXhkcml2ZXJwcm9qZWN0Lm9yZwpodHRwOi8vZHJp dmVyZGV2LmxpbnV4ZHJpdmVycHJvamVjdC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcml2ZXJkZXYt ZGV2ZWwK 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