* [PATCH 0/2] media: i2c: mt9v111 sensor driver
@ 2018-06-11 15:17 Jacopo Mondi
2018-06-11 15:17 ` [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings Jacopo Mondi
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Jacopo Mondi @ 2018-06-11 15:17 UTC (permalink / raw)
To: mchehab; +Cc: Jacopo Mondi, linux-media, linux-kernel, linux-renesas-soc
Hello,
this is a sensor level driver for Aptina MT9V111.
The driver supports YUYV_2X8 output formats and VGA,QVGA,QQVGA,CIF and QCIF
resolution.
The driver allows control of frame rate through s_frame_interval or
V4L2_CID_H/VBLANK control. In order to allow manual frame control, the chip
is initialized with auto-exposure control, auto white balancing and flickering
control disabled.
Tested VGA, QVGA QQVGA resolutions at 5, 10, 15 and 30 frame per second.
Thanks
j
Jacopo Mondi (2):
dt-bindings: media: i2c: Document MT9V111 bindings
media: i2c: Add driver for Aptina MT9V111
.../bindings/media/i2c/aptina,mt9v111.txt | 46 +
MAINTAINERS | 8 +
drivers/media/i2c/Kconfig | 12 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/mt9v111.c | 1297 ++++++++++++++++++++
5 files changed, 1364 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt
create mode 100644 drivers/media/i2c/mt9v111.c
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings 2018-06-11 15:17 [PATCH 0/2] media: i2c: mt9v111 sensor driver Jacopo Mondi @ 2018-06-11 15:17 ` Jacopo Mondi 2018-06-12 22:11 ` Rob Herring 2018-07-23 14:29 ` Sakari Ailus 2018-06-11 15:17 ` [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 Jacopo Mondi 2018-07-04 9:01 ` [PATCH 0/2] media: i2c: mt9v111 sensor driver jacopo mondi 2 siblings, 2 replies; 12+ messages in thread From: Jacopo Mondi @ 2018-06-11 15:17 UTC (permalink / raw) To: mchehab, robh, devicetree Cc: Jacopo Mondi, linux-media, linux-kernel, linux-renesas-soc Add documentation for Aptina MT9V111 image sensor. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- .../bindings/media/i2c/aptina,mt9v111.txt | 46 ++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt diff --git a/Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt b/Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt new file mode 100644 index 0000000..bac4bf0 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt @@ -0,0 +1,46 @@ +* Aptina MT9V111 CMOS sensor +---------------------------- + +The Aptina MT9V111 is a 1/4-Inch VGA-format digital image sensor with a core +based on Aptina MT9V011 sensor and an integrated Image Flow Processor (IFP). + +The sensor has an active pixel array of 649x489 pixels and can output a number +of image resolution and formats controllable through a simple two-wires +interface. + +Required properties: +-------------------- + +- compatible: shall be "aptina,mt9v111". +- clocks: reference to the system clock input provider. + +Optional properties: +-------------------- + +- enable-gpios: output enable signal, pin name "OE#". Active low. +- standby-gpios: low power state control signal, pin name "STANDBY". + Active high. +- reset-gpios: chip reset signal, pin name "RESET#". Active low. + +The device node must contain one 'port' child node with one 'endpoint' child +sub-node for its digital output video port, in accordance with the video +interface bindings defined in: +Documentation/devicetree/bindings/media/video-interfaces.txt + +Example: +-------- + + &i2c1 { + camera@48 { + compatible = "aptina,mt9v111"; + reg = <0x48>; + + clocks = <&camera_clk>; + + port { + mt9v111_out: endpoint { + remote-endpoint = <&ceu_in>; + }; + }; + }; + }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings 2018-06-11 15:17 ` [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings Jacopo Mondi @ 2018-06-12 22:11 ` Rob Herring 2018-07-23 14:29 ` Sakari Ailus 1 sibling, 0 replies; 12+ messages in thread From: Rob Herring @ 2018-06-12 22:11 UTC (permalink / raw) To: Jacopo Mondi Cc: mchehab, devicetree, linux-media, linux-kernel, linux-renesas-soc On Mon, Jun 11, 2018 at 05:17:32PM +0200, Jacopo Mondi wrote: > Add documentation for Aptina MT9V111 image sensor. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > .../bindings/media/i2c/aptina,mt9v111.txt | 46 ++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings 2018-06-11 15:17 ` [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings Jacopo Mondi 2018-06-12 22:11 ` Rob Herring @ 2018-07-23 14:29 ` Sakari Ailus 1 sibling, 0 replies; 12+ messages in thread From: Sakari Ailus @ 2018-07-23 14:29 UTC (permalink / raw) To: Jacopo Mondi Cc: mchehab, robh, devicetree, linux-media, linux-kernel, linux-renesas-soc Hi Jacopo, On Mon, Jun 11, 2018 at 05:17:32PM +0200, Jacopo Mondi wrote: > Add documentation for Aptina MT9V111 image sensor. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > .../bindings/media/i2c/aptina,mt9v111.txt | 46 ++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > > diff --git a/Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt b/Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > new file mode 100644 > index 0000000..bac4bf0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > @@ -0,0 +1,46 @@ > +* Aptina MT9V111 CMOS sensor > +---------------------------- > + > +The Aptina MT9V111 is a 1/4-Inch VGA-format digital image sensor with a core > +based on Aptina MT9V011 sensor and an integrated Image Flow Processor (IFP). > + > +The sensor has an active pixel array of 649x489 pixels and can output a number 640x480 ? > +of image resolution and formats controllable through a simple two-wires > +interface. > + > +Required properties: > +-------------------- > + > +- compatible: shall be "aptina,mt9v111". > +- clocks: reference to the system clock input provider. > + > +Optional properties: > +-------------------- > + > +- enable-gpios: output enable signal, pin name "OE#". Active low. > +- standby-gpios: low power state control signal, pin name "STANDBY". > + Active high. > +- reset-gpios: chip reset signal, pin name "RESET#". Active low. > + > +The device node must contain one 'port' child node with one 'endpoint' child > +sub-node for its digital output video port, in accordance with the video > +interface bindings defined in: > +Documentation/devicetree/bindings/media/video-interfaces.txt > + > +Example: > +-------- > + > + &i2c1 { > + camera@48 { > + compatible = "aptina,mt9v111"; > + reg = <0x48>; > + > + clocks = <&camera_clk>; > + > + port { > + mt9v111_out: endpoint { > + remote-endpoint = <&ceu_in>; > + }; > + }; > + }; > + }; -- Sakari Ailus e-mail: sakari.ailus@iki.fi ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 2018-06-11 15:17 [PATCH 0/2] media: i2c: mt9v111 sensor driver Jacopo Mondi 2018-06-11 15:17 ` [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings Jacopo Mondi @ 2018-06-11 15:17 ` Jacopo Mondi 2018-06-11 16:44 ` kbuild test robot ` (2 more replies) 2018-07-04 9:01 ` [PATCH 0/2] media: i2c: mt9v111 sensor driver jacopo mondi 2 siblings, 3 replies; 12+ messages in thread From: Jacopo Mondi @ 2018-06-11 15:17 UTC (permalink / raw) To: mchehab; +Cc: Jacopo Mondi, linux-media, linux-kernel, linux-renesas-soc Add V4L2 sensor driver for Aptina MT9V111 CMOS image sensor. The MT9V111 is a 1/4-Inch CMOS image sensor based on MT9V011 with an integrated Image Flow Processor. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- MAINTAINERS | 8 + drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/mt9v111.c | 1297 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1318 insertions(+) create mode 100644 drivers/media/i2c/mt9v111.c diff --git a/MAINTAINERS b/MAINTAINERS index a38e24a..2c2fe60 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9523,6 +9523,14 @@ F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt F: drivers/media/i2c/mt9v032.c F: include/media/i2c/mt9v032.h +MT9V111 APTINA CAMERA SENSOR +M: Jacopo Mondi <jacopo@jmondi.org> +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt +F: drivers/media/i2c/mt9v111.c + MULTIFUNCTION DEVICES (MFD) M: Lee Jones <lee.jones@linaro.org> T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 341452f..0bd867d 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -841,6 +841,18 @@ config VIDEO_MT9V032 This is a Video4Linux2 sensor-level driver for the Micron MT9V032 752x480 CMOS sensor. +config VIDEO_MT9V111 + tristate "Aptina MT9V111 sensor support" + depends on I2C && VIDEO_V4L2 + depends on MEDIA_CAMERA_SUPPORT + depends on OF + help + This is a Video4Linux2 sensor-level driver for the Aptina/Micron + MT9V111 sensor. + + To compile this driver as a module, choose M here: the + module will be called mt9v111. + config VIDEO_SR030PC30 tristate "Siliconfile SR030PC30 sensor support" depends on I2C && VIDEO_V4L2 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index d679d57..f0e8618 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o obj-$(CONFIG_VIDEO_MT9T112) += mt9t112.o obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o +obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c new file mode 100644 index 0000000..36e7424 --- /dev/null +++ b/drivers/media/i2c/mt9v111.c @@ -0,0 +1,1297 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * V4L2 sensor driver for Aptina MT9V111 image sensor + * Copyright (C) 2018 Jacopo Mondi <jacopo@jmondi.org> + * + * Based on mt9v032 driver + * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com> + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> + * + * Base on mt9v011 driver + * Copyright (c) 2009 Mauro Carvalho Chehab <mchehab@kernel.org> + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/videodev2.h> +#include <linux/v4l2-mediabus.h> +#include <linux/module.h> + +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> +#include <media/v4l2-fwnode.h> +#include <media/v4l2-image-sizes.h> +#include <media/v4l2-subdev.h> + +/* + * MT9V111 is a 1/4-Inch CMOS digital image sensor with an integrated + * Image Flow Processing (IFP) engine and a sensor core loosely based on + * MT9V011. + * + * The IFP can produce several output image formats from the sensor core + * output, this driver currently supports only YUYV format permutations. + * + * As the auto exposure algorithms controls exposure time and modifies the + * frame rate, this driver disables auto exposure control, auto white balancing + * and auto flickering avoidance to allow manual frame rate control through + * s_frame_interval subdev operation or V4L2_CID_V/HBLANK controls. + * + * While it seems possible to instruct the auto-exposure control algorithm to + * respect a programmed frame rate when adjusting the pixel integration time, + * registers controlling this feature are not documented in the public + * available sensor manual used to develop this driver (09005aef80e90084, + * MT9V111_1.fm - Rev. G 1/05 EN). + */ + +#define MT9V111_CHIP_ID_HIGH 0x82 +#define MT9V111_CHIP_ID_LOW 0x3a + +#define MT9V111_R01_ADDR_SPACE 0x01 +#define MT9V111_R01_IFP 0x01 +#define MT9V111_R01_CORE 0x04 + +#define MT9V111_IFP_R06_OPMODE_CTRL 0x06 +#define MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN BIT(1) +#define MT9V111_IFP_R06_OPMODE_CTRL_AE_EN BIT(14) +#define MT9V111_IFP_R07_IFP_RESET 0x07 +#define MT9V111_IFP_R07_IFP_RESET_MASK BIT(0) +#define MT9V111_IFP_R08_OUTFMT_CTRL 0x08 +#define MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER BIT(11) +#define MT9V111_IFP_R08_OUTFMT_CTRL_PCLK BIT(5) +#define MT9V111_IFP_R3A_OUTFMT_CTRL2 0x3a +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR BIT(0) +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC BIT(1) +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK GENMASK(2, 0) +#define MT9V111_IFP_RA5_HPAN 0xa5 +#define MT9V111_IFP_RA6_HZOOM 0xa6 +#define MT9V111_IFP_RA7_HOUT 0xa7 +#define MT9V111_IFP_RA8_VPAN 0xa8 +#define MT9V111_IFP_RA9_VZOOM 0xa9 +#define MT9V111_IFP_RAA_VOUT 0xaa +#define MT9V111_IFP_DECIMATION_MASK GENMASK(9, 0) +#define MT9V111_IFP_DECIMATION_FREEZE BIT(15) + +#define MT9V111_CORE_R03_WIN_HEIGHT 0x03 +#define MT9V111_CORE_R03_WIN_V_OFFS 2 +#define MT9V111_CORE_R04_WIN_WIDTH 0x04 +#define MT9V111_CORE_R04_WIN_H_OFFS 114 +#define MT9V111_CORE_R05_HBLANK 0x05 +#define MT9V111_CORE_R05_MIN_HBLANK 0x09 +#define MT9V111_CORE_R05_MAX_HBLANK GENMASK(9, 0) +#define MT9V111_CORE_R05_DEF_HBLANK 0x26 +#define MT9V111_CORE_R06_VBLANK 0x06 +#define MT9V111_CORE_R06_MIN_VBLANK 0x03 +#define MT9V111_CORE_R06_MAX_VBLANK GENMASK(11, 0) +#define MT9V111_CORE_R06_DEF_VBLANK 0x04 +#define MT9V111_CORE_R07_OUT_CTRL 0x07 +#define MT9V111_CORE_R07_OUT_CTRL_SAMPLE BIT(4) +#define MT9V111_CORE_R09_PIXEL_INT 0x09 +#define MT9V111_CORE_R09_PIXEL_INT_MASK GENMASK(11, 0) +#define MT9V111_CORE_R0D_CORE_RESET 0x0d +#define MT9V111_CORE_R0D_CORE_RESET_MASK BIT(0) +#define MT9V111_CORE_RFF_CHIP_VER 0xff + +#define MT9V111_PIXEL_ARRAY_WIDTH 640 +#define MT9V111_PIXEL_ARRAY_HEIGHT 480 + +#define MT9V111_MAX_CLKIN 27000000 + +struct mt9v111_dev { + struct device *dev; + struct i2c_client *client; + + /* Protects address space change operations. */ + spinlock_t addr_lock; + u8 addr_space; + + struct v4l2_subdev sd; +#if defined(CONFIG_MEDIA_CONTROLLER) + struct media_pad pad; +#endif + + struct v4l2_ctrl *hblank; + struct v4l2_ctrl *vblank; + struct v4l2_ctrl_handler ctrls; + + /* Output image format and sizes. */ + struct v4l2_mbus_framefmt fmt; + unsigned int fps; + + /* Protects power up/down sequences. */ + struct mutex pwr_mutex; + int pwr_count; + + /* Protects stream on/off sequences. */ + struct mutex stream_mutex; + bool streaming; + + /* Flags to mark HW settings as not yet applied. */ + bool pending; + + /* Clock provider and system clock frequency. */ + struct clk *clk; + u32 sysclk; + + struct gpio_desc *oe; + struct gpio_desc *standby; + struct gpio_desc *reset; +}; + +#define sd_to_mt9v111(__sd) container_of(__sd, struct mt9v111_dev, sd) + +/* + * mt9v111_mbus_fmt - List all media bus formats supported by the driver. + * + * Only list the media bus code here. The image sizes are freely configurable + * in the pixel array sizes range. + * + * The frame desired frame interval, in the supported frame interval range, is + * obtained by configuring blanking as the sensor does not have a PLL but + * only a fixed clock divider that generates the output pixel clock. + */ +static struct mt9v111_mbus_fmt { + u32 code; +} mt9v111_formats[] = { + { + .code = MEDIA_BUS_FMT_UYVY8_2X8, + }, + { + .code = MEDIA_BUS_FMT_YUYV8_2X8, + }, + { + .code = MEDIA_BUS_FMT_VYUY8_2X8, + }, + { + .code = MEDIA_BUS_FMT_YVYU8_2X8, + }, +}; + +static u32 mt9v111_frame_intervals[] = {5, 10, 15, 20, 30}; + +/* + * mt9v111_frame_sizes - List sensor's supported resolutions. + * + * Resolution generated through decimation in the IFP block from the + * full VGA pixel array. + */ +static struct v4l2_rect mt9v111_frame_sizes[] = { + { + .width = 640, + .height = 480, + }, + { + .width = 352, + .height = 288 + }, + { + .width = 320, + .height = 240, + }, + { + .width = 176, + .height = 144, + }, + { + .width = 160, + .height = 120, + }, +}; + +/* Initial register configuration. */ +static struct mt9v111_reg_list { + u8 space; + u8 reg; + u16 mask; + u16 val; +} mt9v111_init_config[] = { + { + /* Disable AE and AWB to enable manual frame rate control. */ + .space = MT9V111_R01_IFP, + .reg = MT9V111_IFP_R06_OPMODE_CTRL, + .mask = MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN | + MT9V111_IFP_R06_OPMODE_CTRL_AE_EN, + .val = 0, + }, + { + /* Disable anti-flicker to enable manual frame rate control. */ + .space = MT9V111_R01_IFP, + .reg = MT9V111_IFP_R08_OUTFMT_CTRL, + .mask = MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER, + .val = 0, + }, + + /* TODO: AE and AWB manual configuration. */ +}; + +/* --- Device I/O access --- */ + +static int __mt9v111_read(struct i2c_client *c, u8 reg, u16 *val) +{ + struct i2c_msg msg[2]; + __be16 buf; + int ret; + + msg[0].addr = c->addr; + msg[0].flags = 0; + msg[0].len = 1; + msg[0].buf = ® + + msg[1].addr = c->addr; + msg[1].flags = I2C_M_RD; + msg[1].len = 2; + msg[1].buf = (char *)&buf; + + ret = i2c_transfer(c->adapter, msg, 2); + if (ret < 0) { + dev_err(&c->dev, "i2c read transfer error: %d\n", ret); + return ret; + } + + *val = be16_to_cpu(buf); + + dev_info(&c->dev, "%s: %x=%x\n", __func__, reg, *val); + + return 0; +} + +static int __mt9v111_write(struct i2c_client *c, u8 reg, u16 val) +{ + struct i2c_msg msg; + u8 buf[3] = { 0 }; + int ret; + + buf[0] = reg; + buf[1] = val >> 8; + buf[2] = val & 0xff; + + msg.addr = c->addr; + msg.flags = 0; + msg.len = 3; + msg.buf = (char *)buf; + + dev_info(&c->dev, "%s: %x = %x%x\n", __func__, reg, buf[1], buf[2]); + + ret = i2c_transfer(c->adapter, &msg, 1); + if (ret < 0) { + dev_err(&c->dev, "i2c write transfer error: %d\n", ret); + return ret; + } + + return 0; +} + +static int __mt9v111_addr_space_select(struct i2c_client *c, u16 addr_space) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(c); + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + u16 val; + int ret; + + spin_lock(&mt9v111->addr_lock); + if (mt9v111->addr_space == addr_space) { + spin_unlock(&mt9v111->addr_lock); + return 0; + } + spin_unlock(&mt9v111->addr_lock); + + ret = __mt9v111_write(c, MT9V111_R01_ADDR_SPACE, addr_space); + if (ret) + return ret; + + /* Verify address space has been updated */ + ret = __mt9v111_read(c, MT9V111_R01_ADDR_SPACE, &val); + if (ret) + return ret; + + if (val != addr_space) + return -EINVAL; + + mt9v111->addr_space = addr_space; + + return 0; +} + +static int mt9v111_read(struct i2c_client *c, u8 addr_space, u8 reg, u16 *val) +{ + int ret; + + /* Select register address space first. */ + ret = __mt9v111_addr_space_select(c, addr_space); + if (ret) + return ret; + + ret = __mt9v111_read(c, reg, val); + if (ret) + return ret; + + return 0; +} + +static int mt9v111_write(struct i2c_client *c, u8 addr_space, u8 reg, u16 val) +{ + int ret; + + /* Select register address space first. */ + ret = __mt9v111_addr_space_select(c, addr_space); + if (ret) + return ret; + + ret = __mt9v111_write(c, reg, val); + if (ret) + return ret; + + return 0; +} + +static int mt9v111_update(struct i2c_client *c, u8 addr_space, u8 reg, + u16 mask, u16 val) +{ + u16 current_val; + int ret; + + /* Select register address space first. */ + ret = __mt9v111_addr_space_select(c, addr_space); + if (ret) + return ret; + + /* Read the current register value, then update it. */ + ret = __mt9v111_read(c, reg, ¤t_val); + if (ret) + return ret; + + current_val &= ~mask; + current_val |= (val & mask); + ret = __mt9v111_write(c, reg, current_val); + if (ret) + return ret; + + return 0; +} + +static int mt9v111_update_reglist(struct i2c_client *c, + struct mt9v111_reg_list *reg_list, + unsigned int n_regs) +{ + unsigned int i; + int ret; + + for (i = 0; i < n_regs; i++) { + struct mt9v111_reg_list *reg = ®_list[i]; + + ret = mt9v111_update(c, reg->space, reg->reg, reg->mask, + reg->val); + if (ret) + return ret; + } + + return 0; +} + +/* --- Sensor HW operations --- */ + +static int __mt9v111_power_on(struct v4l2_subdev *sd) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + int ret; + + ret = clk_prepare_enable(mt9v111->clk); + if (ret) + return ret; + + clk_set_rate(mt9v111->clk, mt9v111->sysclk); + + gpiod_set_value(mt9v111->standby, 0); + usleep_range(500, 1000); + + gpiod_set_value(mt9v111->oe, 1); + usleep_range(500, 1000); + + return 0; +} + +static int __mt9v111_power_off(struct v4l2_subdev *sd) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + + gpiod_set_value(mt9v111->oe, 0); + usleep_range(500, 1000); + + gpiod_set_value(mt9v111->standby, 1); + usleep_range(500, 1000); + + clk_disable_unprepare(mt9v111->clk); + + return 0; +} + +static int __mt9v111_hw_reset(struct mt9v111_dev *mt9v111) +{ + if (!mt9v111->reset) + return -EINVAL; + + gpiod_set_value(mt9v111->reset, 1); + usleep_range(500, 1000); + + gpiod_set_value(mt9v111->reset, 0); + usleep_range(500, 1000); + + return 0; +} + +static int __mt9v111_sw_reset(struct mt9v111_dev *mt9v111) +{ + struct i2c_client *c = mt9v111->client; + unsigned int ret; + + /* Software reset core and IFP blocks. */ + + ret = mt9v111_update(c, MT9V111_R01_CORE, + MT9V111_CORE_R0D_CORE_RESET, + MT9V111_CORE_R0D_CORE_RESET_MASK, 1); + if (ret) + return ret; + usleep_range(500, 1000); + + ret = mt9v111_update(c, MT9V111_R01_CORE, + MT9V111_CORE_R0D_CORE_RESET, + MT9V111_CORE_R0D_CORE_RESET_MASK, 0); + if (ret) + return ret; + usleep_range(500, 1000); + + ret = mt9v111_update(c, MT9V111_R01_IFP, + MT9V111_IFP_R07_IFP_RESET, + MT9V111_IFP_R07_IFP_RESET_MASK, 1); + if (ret) + return ret; + usleep_range(500, 1000); + + ret = mt9v111_update(c, MT9V111_R01_IFP, + MT9V111_IFP_R07_IFP_RESET, + MT9V111_IFP_R07_IFP_RESET_MASK, 0); + if (ret) + return ret; + usleep_range(500, 1000); + + return 0; +} + +static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111, + struct v4l2_fract *tpf) +{ + unsigned int fps = tpf->numerator ? + tpf->denominator / tpf->numerator : + tpf->denominator; + unsigned int best_diff; + unsigned int frm_cols; + unsigned int row_pclk; + unsigned int best_fps; + unsigned int pclk; + unsigned int diff; + unsigned int idx; + unsigned int hb; + unsigned int vb; + unsigned int i; + int ret; + + /* Approximate to the closest supported frame interval. */ + best_diff = ~0L; + for (i = 0, idx = 0; i < ARRAY_SIZE(mt9v111_frame_intervals); i++) { + diff = abs(fps - mt9v111_frame_intervals[i]); + if (diff < best_diff) { + idx = i; + best_diff = diff; + } + } + fps = mt9v111_frame_intervals[idx]; + + /* + * The sensor does not provide a PLL circuitry and pixel clock is + * generated dividing by two the master clock source. + * + * Trow = (W + Hblank + 114) * 2 * (1 / SYSCLK) + * TFrame = Trow * (H + Vblank + 2) + * + * FPS = (SYSCLK / 2) / (Trow * (H + Vblank + 2)) + * + * This boils down to tune H and V blanks to best approximate the + * above equation. + * + * Test all available H/V blank values, until we reach the + * desired frame rate. + */ + best_fps = vb = hb = 0; + pclk = DIV_ROUND_CLOSEST(mt9v111->sysclk, 2); + row_pclk = MT9V111_PIXEL_ARRAY_WIDTH + 7 + MT9V111_CORE_R04_WIN_H_OFFS; + frm_cols = MT9V111_PIXEL_ARRAY_HEIGHT + 7 + MT9V111_CORE_R03_WIN_V_OFFS; + + best_diff = ~0L; + for (vb = MT9V111_CORE_R06_MIN_VBLANK; + vb < MT9V111_CORE_R06_MAX_VBLANK; vb++) { + for (hb = MT9V111_CORE_R05_MIN_HBLANK; + hb < MT9V111_CORE_R05_MAX_HBLANK; hb += 10) { + unsigned int t_frame = (row_pclk + hb) * + (frm_cols + vb); + unsigned int t_fps = DIV_ROUND_CLOSEST(pclk, t_frame); + + diff = abs(fps - t_fps); + if (diff < best_diff) { + best_diff = diff; + best_fps = t_fps; + + if (diff == 0) + break; + } + } + + if (diff == 0) + break; + } + + dev_info(mt9v111->dev, "FPS: %u, hb = %u, vb = %u\n", fps, hb, vb); + + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->hblank, hb); + if (ret) + return ret; + + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->vblank, vb); + if (ret) + return ret; + + tpf->numerator = 1; + tpf->denominator = best_fps; + + return 0; +} + +static int mt9v111_hw_config(struct mt9v111_dev *mt9v111) +{ + struct i2c_client *c = mt9v111->client; + unsigned int ret; + u16 outfmtctrl2; + + /* Force device reset. */ + ret = __mt9v111_hw_reset(mt9v111); + if (ret == -EINVAL) + ret = __mt9v111_sw_reset(mt9v111); + if (ret) + return ret; + + /* + * Disable AE, AWB and anti-flickering to allow manual frame rate + * control. + */ + ret = mt9v111_update_reglist(c, mt9v111_init_config, + ARRAY_SIZE(mt9v111_init_config)); + if (ret) + return ret; + + /* Configure internal clock sample rate. */ + ret = mt9v111->sysclk < DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? + mt9v111_update(c, MT9V111_R01_CORE, + MT9V111_CORE_R07_OUT_CTRL, + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 1) : + mt9v111_update(c, MT9V111_R01_CORE, + MT9V111_CORE_R07_OUT_CTRL, + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 0); + if (ret) + return ret; + + /* + * Configure output image format components ordering. + * + * TODO: IFP block can also output several RGB permutations, we only + * support YUYV permutations at the moment. + */ + switch (mt9v111->fmt.code) { + case MEDIA_BUS_FMT_YUYV8_2X8: + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC; + break; + case MEDIA_BUS_FMT_VYUY8_2X8: + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; + break; + case MEDIA_BUS_FMT_YVYU8_2X8: + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC | + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; + break; + case MEDIA_BUS_FMT_UYVY8_2X8: + default: + outfmtctrl2 = 0; + break; + } + + ret = mt9v111_update(c, MT9V111_R01_IFP, MT9V111_IFP_R3A_OUTFMT_CTRL2, + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK, + outfmtctrl2); + if (ret) + return ret; + + /* + * Do not change default sensor's core configuration: + * output the whole 640x480 pixel array, skip 18 columns and 6 rows. + * + * Instead, control the output image size through IFP block. + * + * TODO: No zoom&pan support. Currently we control the output image + * size only through decimation, with no zoom support. + */ + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA5_HPAN, + MT9V111_IFP_DECIMATION_FREEZE); + if (ret) + return ret; + + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA8_VPAN, + MT9V111_IFP_DECIMATION_FREEZE); + if (ret) + return ret; + + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA6_HZOOM, + MT9V111_IFP_DECIMATION_FREEZE | + MT9V111_PIXEL_ARRAY_WIDTH); + if (ret) + return ret; + + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA9_VZOOM, + MT9V111_IFP_DECIMATION_FREEZE | + MT9V111_PIXEL_ARRAY_HEIGHT); + if (ret) + return ret; + + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA7_HOUT, + MT9V111_IFP_DECIMATION_FREEZE | + mt9v111->fmt.width); + if (ret) + return ret; + + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RAA_VOUT, + mt9v111->fmt.height); + if (ret) + return ret; + + /* Update blankings to match the programmed frame rate. */ + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R05_HBLANK, + MT9V111_CORE_R05_MAX_HBLANK, + mt9v111->hblank->val); + if (ret) + return ret; + + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R06_VBLANK, + MT9V111_CORE_R06_MAX_VBLANK, + mt9v111->vblank->val); + if (ret) + return ret; + + /* + * Set pixel integration time to the whole frame time. + * This value controls the the shutter delay when running with AE + * disabled. If longer than frame time, it affects the output + * frame rate. + */ + ret = mt9v111_write(c, MT9V111_R01_CORE, MT9V111_CORE_R09_PIXEL_INT, + MT9V111_PIXEL_ARRAY_HEIGHT); + if (ret) + return ret; + + return 0; +} + +/* --- V4L2 subdev operations --- */ + +static int mt9v111_s_power(struct v4l2_subdev *sd, int on) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + int pwr_count; + int ret = 0; + + mutex_lock(&mt9v111->pwr_mutex); + + /* + * Make sure we're transitioning from 0 to 1, or viceversa, only + * before actually changing the power state. + */ + pwr_count = mt9v111->pwr_count; + pwr_count += on ? 1 : -1; + if (pwr_count == !!on) { + ret = on ? __mt9v111_power_on(sd) : + __mt9v111_power_off(sd); + if (!ret) + /* All went well, updated power counter. */ + mt9v111->pwr_count = pwr_count; + + mutex_unlock(&mt9v111->pwr_mutex); + + return ret; + } + + /* + * Update power counter to keep track of how many nested calls we + * received. + */ + WARN_ON(pwr_count < 0 || pwr_count > 1); + mt9v111->pwr_count = pwr_count; + + mutex_unlock(&mt9v111->pwr_mutex); + + return ret; +} + +static int mt9v111_s_stream(struct v4l2_subdev *subdev, int enable) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); + int ret; + + mutex_lock(&mt9v111->stream_mutex); + + if (mt9v111->streaming == enable) { + mutex_unlock(&mt9v111->stream_mutex); + return 0; + } + + ret = mt9v111_s_power(subdev, enable); + if (ret) + goto error_unlock; + + if (enable && mt9v111->pending) { + ret = mt9v111_hw_config(mt9v111); + if (ret) + goto error_unlock; + + /* + * No need to update control here as far as only H/VBLANK are + * supported and immediately programmed to registers in .s_ctrl + */ + + mt9v111->pending = false; + } + + mt9v111->streaming = enable ? true : false; + mutex_unlock(&mt9v111->stream_mutex); + + return 0; + +error_unlock: + mutex_unlock(&mt9v111->stream_mutex); + + return ret; +} + +static int mt9v111_s_frame_interval(struct v4l2_subdev *sd, + struct v4l2_subdev_frame_interval *ival) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + struct v4l2_fract *tpf = &ival->interval; + unsigned int fps = tpf->numerator ? + tpf->denominator / tpf->numerator : + tpf->denominator; + unsigned int max_fps; + + mutex_lock(&mt9v111->stream_mutex); + if (mt9v111->streaming) { + mutex_unlock(&mt9v111->stream_mutex); + return -EBUSY; + } + mutex_unlock(&mt9v111->stream_mutex); + + if (mt9v111->fps == fps) + return 0; + + /* Make sure frame rate/image sizes constraints are respected. */ + if (mt9v111->fmt.width < QVGA_WIDTH && + mt9v111->fmt.height < QVGA_HEIGHT) + max_fps = 90; + else if (mt9v111->fmt.width < CIF_WIDTH && + mt9v111->fmt.height < CIF_HEIGHT) + max_fps = 60; + else + max_fps = mt9v111->sysclk < + DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? 15 : + 30; + + if (fps > max_fps) + return -EINVAL; + + mt9v111_calc_frame_rate(mt9v111, tpf); + + mt9v111->fps = fps; + mt9v111->pending = true; + + return 0; +} + +static int mt9v111_g_frame_interval(struct v4l2_subdev *sd, + struct v4l2_subdev_frame_interval *ival) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + struct v4l2_fract *tpf = &ival->interval; + + tpf->numerator = 1; + tpf->denominator = mt9v111->fps; + + return 0; +} + +static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format( + struct mt9v111_dev *mt9v111, + struct v4l2_subdev_pad_config *cfg, + unsigned int pad, + enum v4l2_subdev_format_whence which) +{ + switch (which) { + case V4L2_SUBDEV_FORMAT_TRY: +#ifdef CONFIG_MEDIA_CONTROLLER + return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad); +#else + return &cfg->try_fmt; +#endif + case V4L2_SUBDEV_FORMAT_ACTIVE: + return &mt9v111->fmt; + default: + return NULL; + } +} + +static int mt9v111_enum_mbus_code(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_mbus_code_enum *code) +{ + if (code->pad || code->index > ARRAY_SIZE(mt9v111_formats) - 1) + return -EINVAL; + + code->code = mt9v111_formats[code->index].code; + + return 0; +} + +static int mt9v111_enum_frame_interval(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_frame_interval_enum *fie) +{ + unsigned int i; + + if (fie->pad || fie->index >= ARRAY_SIZE(mt9v111_frame_intervals)) + return -EINVAL; + + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) + if (fie->width == mt9v111_frame_sizes[i].width && + fie->height == mt9v111_frame_sizes[i].height) + break; + + if (i == ARRAY_SIZE(mt9v111_frame_sizes)) + return -EINVAL; + + fie->interval.numerator = 1; + fie->interval.denominator = mt9v111_frame_intervals[fie->index]; + + return 0; +} + +static int mt9v111_enum_frame_size(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_frame_size_enum *fse) +{ + if (fse->pad || fse->index > ARRAY_SIZE(mt9v111_frame_sizes)) + return -EINVAL; + + fse->min_width = mt9v111_frame_sizes[fse->index].width; + fse->max_width = mt9v111_frame_sizes[fse->index].width; + fse->min_height = mt9v111_frame_sizes[fse->index].height; + fse->max_height = mt9v111_frame_sizes[fse->index].height; + + return 0; +} + +static int mt9v111_get_format(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *format) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); + + if (format->pad) + return -EINVAL; + + format->format = *__mt9v111_get_pad_format(mt9v111, cfg, format->pad, + format->which); + return 0; +} + +static int mt9v111_set_format(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *format) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); + struct v4l2_mbus_framefmt new_fmt; + struct v4l2_mbus_framefmt *__fmt; + unsigned int best_fit = ~0L; + unsigned int idx; + unsigned int i; + + mutex_lock(&mt9v111->stream_mutex); + if (mt9v111->streaming) { + mutex_unlock(&mt9v111->stream_mutex); + return -EBUSY; + } + mutex_unlock(&mt9v111->stream_mutex); + + if (format->pad) + return -EINVAL; + + /* Update mbus format code and sizes. */ + for (i = 0; i < ARRAY_SIZE(mt9v111_formats); i++) { + if (format->format.code == mt9v111_formats[i].code) { + new_fmt.code = mt9v111_formats[i].code; + break; + } + } + if (i == ARRAY_SIZE(mt9v111_formats)) + new_fmt.code = mt9v111_formats[0].code; + + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) { + unsigned int fit = abs(mt9v111_frame_sizes[i].width - + format->format.width) + + abs(mt9v111_frame_sizes[i].height - + format->format.height); + if (fit < best_fit) { + best_fit = fit; + idx = i; + + if (fit == 0) + break; + } + } + new_fmt.width = mt9v111_frame_sizes[idx].width; + new_fmt.height = mt9v111_frame_sizes[idx].height; + + /* Update the device (or pad) format if it has changed. */ + __fmt = __mt9v111_get_pad_format(mt9v111, cfg, format->pad, + format->which); + + /* Format hasn't changed, stop here. */ + if (__fmt->code == new_fmt.code && + __fmt->width == new_fmt.width && + __fmt->height == new_fmt.height) + goto done; + + /* Update the format and sizes, then mark changes as pending. */ + __fmt->code = new_fmt.code; + __fmt->width = new_fmt.width; + __fmt->height = new_fmt.height; + + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) + mt9v111->pending = true; + + dev_info(mt9v111->dev, "%s: mbus_code: %x - (%ux%u)\n", + __func__, __fmt->code, __fmt->width, __fmt->height); + +done: + format->format = *__fmt; + + return 0; +} + +static const struct v4l2_subdev_core_ops mt9v111_core_ops = { + .s_power = mt9v111_s_power, +}; + +static const struct v4l2_subdev_video_ops mt9v111_video_ops = { + .s_stream = mt9v111_s_stream, + .s_frame_interval = mt9v111_s_frame_interval, + .g_frame_interval = mt9v111_g_frame_interval, +}; + +static const struct v4l2_subdev_pad_ops mt9v111_pad_ops = { + .enum_mbus_code = mt9v111_enum_mbus_code, + .enum_frame_size = mt9v111_enum_frame_size, + .enum_frame_interval = mt9v111_enum_frame_interval, + .get_fmt = mt9v111_get_format, + .set_fmt = mt9v111_set_format, +}; + +static const struct v4l2_subdev_ops mt9v111_ops = { + .core = &mt9v111_core_ops, + .video = &mt9v111_video_ops, + .pad = &mt9v111_pad_ops, +}; + +#ifdef CONFIG_MEDIA_CONTROLLER +static const struct media_entity_operations mt9v111_subdev_entity_ops = { + .link_validate = v4l2_subdev_link_validate, +}; + +static int mt9v111_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) +{ + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + struct v4l2_mbus_framefmt *__fmt = + v4l2_subdev_get_try_format(sd, fh->pad, 0); + + /* Initialize try format: copy the device format. */ + *__fmt = mt9v111->fmt; + + return 0; +} + +static const struct v4l2_subdev_internal_ops mt9v111_internal_ops = { + .open = mt9v111_open, +}; +#endif + +/* --- V4L2 ctrl --- */ +static int mt9v111_s_ctrl(struct v4l2_ctrl *ctrl) +{ + struct mt9v111_dev *mt9v111 = container_of(ctrl->handler, + struct mt9v111_dev, + ctrls); + int ret = 0; + + mutex_lock(&mt9v111->pwr_mutex); + /* + * If sensor is powered down, just cache new control values, + * no actual register access. + */ + if (!mt9v111->pwr_count) { + mutex_unlock(&mt9v111->pwr_mutex); + return 0; + } + mutex_unlock(&mt9v111->pwr_mutex); + + switch (ctrl->id) { + case V4L2_CID_HBLANK: + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, + MT9V111_CORE_R05_HBLANK, + MT9V111_CORE_R05_MAX_HBLANK, + mt9v111->hblank->val); + break; + case V4L2_CID_VBLANK: + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, + MT9V111_CORE_R06_VBLANK, + MT9V111_CORE_R06_MAX_VBLANK, + mt9v111->vblank->val); + break; + } + + return ret; +} + +static const struct v4l2_ctrl_ops mt9v111_ctrl_ops = { + .s_ctrl = mt9v111_s_ctrl, +}; + +static int mt9v111_chip_probe(struct mt9v111_dev *mt9v111) +{ + int ret; + u16 val; + + ret = __mt9v111_power_on(&mt9v111->sd); + if (ret) + return ret; + + ret = mt9v111_read(mt9v111->client, MT9V111_R01_CORE, + MT9V111_CORE_RFF_CHIP_VER, &val); + if (ret) + return ret; + + if ((val >> 8) != MT9V111_CHIP_ID_HIGH && + (val & 0xff) != MT9V111_CHIP_ID_LOW) { + dev_err(mt9v111->dev, + "Unable to identify MT9V111 chip: 0x%2x%2x\n", + val >> 8, val & 0xff); + return ret; + } + + ret = __mt9v111_power_off(&mt9v111->sd); + if (ret) + return ret; + + dev_info(mt9v111->dev, "Chip identified: 0x%2x%2x\n", + val >> 8, val & 0xff); + + return 0; +} + +static int mt9v111_probe(struct i2c_client *client) +{ + struct mt9v111_dev *mt9v111; + struct v4l2_fract tpf; + int ret; + + mt9v111 = devm_kzalloc(&client->dev, sizeof(*mt9v111), GFP_KERNEL); + if (!mt9v111) + return -ENOMEM; + + mt9v111->dev = &client->dev; + mt9v111->client = client; + + mt9v111->clk = devm_clk_get(&client->dev, NULL); + if (IS_ERR(mt9v111->clk)) + return PTR_ERR(mt9v111->clk); + + mt9v111->sysclk = clk_get_rate(mt9v111->clk); + if (mt9v111->sysclk > MT9V111_MAX_CLKIN) + return -EINVAL; + + mt9v111->oe = devm_gpiod_get_optional(&client->dev, "enable", + GPIOD_OUT_LOW); + if (IS_ERR(mt9v111->oe)) { + dev_err(&client->dev, "Unable to get GPIO \"enable\": %ld\n", + PTR_ERR(mt9v111->oe)); + return PTR_ERR(mt9v111->oe); + } + + mt9v111->standby = devm_gpiod_get_optional(&client->dev, "standby", + GPIOD_OUT_HIGH); + if (IS_ERR(mt9v111->standby)) { + dev_err(&client->dev, "Unable to get GPIO \"standby\": %ld\n", + PTR_ERR(mt9v111->standby)); + return PTR_ERR(mt9v111->standby); + } + + mt9v111->reset = devm_gpiod_get_optional(&client->dev, "reset", + GPIOD_OUT_LOW); + if (IS_ERR(mt9v111->reset)) { + dev_err(&client->dev, "Unable to get GPIO \"reset\": %ld\n", + PTR_ERR(mt9v111->reset)); + return PTR_ERR(mt9v111->reset); + } + + mutex_init(&mt9v111->pwr_mutex); + mutex_init(&mt9v111->stream_mutex); + spin_lock_init(&mt9v111->addr_lock); + + v4l2_ctrl_handler_init(&mt9v111->ctrls, 3); + + mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, + V4L2_CID_HBLANK, + MT9V111_CORE_R05_MIN_HBLANK, + MT9V111_CORE_R05_MAX_HBLANK, 1, + MT9V111_CORE_R05_DEF_HBLANK); + if (IS_ERR_OR_NULL(mt9v111->hblank)) { + ret = PTR_ERR(mt9v111->hblank); + goto error_free_ctrls; + } + + mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, + V4L2_CID_VBLANK, + MT9V111_CORE_R06_MIN_VBLANK, + MT9V111_CORE_R06_MAX_VBLANK, 1, + MT9V111_CORE_R06_DEF_VBLANK); + if (IS_ERR_OR_NULL(mt9v111->vblank)) { + ret = PTR_ERR(mt9v111->vblank); + goto error_free_ctrls; + } + + /* PIXEL_RATE is fixed: just expose it to user space. */ + v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, + V4L2_CID_PIXEL_RATE, 0, + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2), 1, + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2)); + + mt9v111->sd.ctrl_handler = &mt9v111->ctrls; + + /* Start with default configuration: 640x480 UYVY. */ + mt9v111->fmt.width = 640; + mt9v111->fmt.height = 480; + mt9v111->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; + + /* These are fixed for all supported formats. */ + mt9v111->fmt.field = V4L2_FIELD_NONE; + mt9v111->fmt.colorspace = V4L2_COLORSPACE_SRGB; + mt9v111->fmt.ycbcr_enc = V4L2_YCBCR_ENC_601; + mt9v111->fmt.quantization = V4L2_QUANTIZATION_LIM_RANGE; + mt9v111->fmt.xfer_func = V4L2_XFER_FUNC_SRGB; + + /* Re-calculate blankings for 640x480@15fps. */ + mt9v111->fps = 15; + tpf.numerator = 1; + tpf.denominator = mt9v111->fps; + ret = mt9v111_calc_frame_rate(mt9v111, &tpf); + if (ret) + goto error_free_ctrls; + + mt9v111->pwr_count = 0; + mt9v111->addr_space = MT9V111_R01_IFP; + mt9v111->pending = true; + + v4l2_i2c_subdev_init(&mt9v111->sd, client, &mt9v111_ops); + +#ifdef CONFIG_MEDIA_CONTROLLER + mt9v111->sd.internal_ops = &mt9v111_internal_ops; + mt9v111->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + mt9v111->sd.entity.ops = &mt9v111_subdev_entity_ops; + mt9v111->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; + + mt9v111->pad.flags = MEDIA_PAD_FL_SOURCE; + ret = media_entity_pads_init(&mt9v111->sd.entity, 1, &mt9v111->pad); + if (ret) + goto error_free_ctrls; +#endif + + ret = mt9v111_chip_probe(mt9v111); + if (ret) + goto error_free_ctrls; + + ret = v4l2_async_register_subdev(&mt9v111->sd); + if (ret) + goto error_free_ctrls; + + return 0; + +error_free_ctrls: + v4l2_ctrl_handler_free(&mt9v111->ctrls); + +#ifdef CONFIG_MEDIA_CONTROLLER + media_entity_cleanup(&mt9v111->sd.entity); +#endif + + mutex_destroy(&mt9v111->pwr_mutex); + mutex_destroy(&mt9v111->stream_mutex); + + return ret; +} + +static int mt9v111_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); + + v4l2_async_unregister_subdev(sd); + + v4l2_ctrl_handler_free(&mt9v111->ctrls); + +#ifdef CONFIG_MEDIA_CONTROLLER + media_entity_cleanup(&sd->entity); +#endif + + mutex_destroy(&mt9v111->pwr_mutex); + mutex_destroy(&mt9v111->stream_mutex); + + devm_gpiod_put(mt9v111->dev, mt9v111->oe); + devm_gpiod_put(mt9v111->dev, mt9v111->standby); + devm_gpiod_put(mt9v111->dev, mt9v111->reset); + + devm_clk_put(mt9v111->dev, mt9v111->clk); + + return 0; +} + +static const struct of_device_id mt9v111_of_match[] = { + { .compatible = "aptina,mt9v111", }, + { /* sentinel */ }, +}; + +static struct i2c_driver mt9v111_driver = { + .driver = { + .name = "mt9v111", + .of_match_table = of_match_ptr(mt9v111_of_match), + }, + .probe_new = mt9v111_probe, + .remove = mt9v111_remove, +}; + +module_i2c_driver(mt9v111_driver); + +MODULE_DESCRIPTION("V4L2 sensor driver for Aptina MT9V111"); +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>"); +MODULE_LICENSE("GPL v2"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 2018-06-11 15:17 ` [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 Jacopo Mondi @ 2018-06-11 16:44 ` kbuild test robot 2018-07-17 9:48 ` Kieran Bingham 2018-07-23 14:28 ` Sakari Ailus 2 siblings, 0 replies; 12+ messages in thread From: kbuild test robot @ 2018-06-11 16:44 UTC (permalink / raw) To: Jacopo Mondi Cc: kbuild-all, mchehab, Jacopo Mondi, linux-media, linux-kernel, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 3871 bytes --] Hi Jacopo, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on v4.17 next-20180608] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-i2c-mt9v111-sensor-driver/20180611-233038 base: git://linuxtv.org/media_tree.git master config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=sh Note: it may well be a FALSE warning. FWIW you are at least aware of it now. http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings All warnings (new ones prefixed by >>): drivers/media/i2c/mt9v111.c: In function 'mt9v111_set_format': >> drivers/media/i2c/mt9v111.c:929:15: warning: 'idx' may be used uninitialized in this function [-Wmaybe-uninitialized] unsigned int idx; ^~~ vim +/idx +929 drivers/media/i2c/mt9v111.c 920 921 static int mt9v111_set_format(struct v4l2_subdev *subdev, 922 struct v4l2_subdev_pad_config *cfg, 923 struct v4l2_subdev_format *format) 924 { 925 struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); 926 struct v4l2_mbus_framefmt new_fmt; 927 struct v4l2_mbus_framefmt *__fmt; 928 unsigned int best_fit = ~0L; > 929 unsigned int idx; 930 unsigned int i; 931 932 mutex_lock(&mt9v111->stream_mutex); 933 if (mt9v111->streaming) { 934 mutex_unlock(&mt9v111->stream_mutex); 935 return -EBUSY; 936 } 937 mutex_unlock(&mt9v111->stream_mutex); 938 939 if (format->pad) 940 return -EINVAL; 941 942 /* Update mbus format code and sizes. */ 943 for (i = 0; i < ARRAY_SIZE(mt9v111_formats); i++) { 944 if (format->format.code == mt9v111_formats[i].code) { 945 new_fmt.code = mt9v111_formats[i].code; 946 break; 947 } 948 } 949 if (i == ARRAY_SIZE(mt9v111_formats)) 950 new_fmt.code = mt9v111_formats[0].code; 951 952 for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) { 953 unsigned int fit = abs(mt9v111_frame_sizes[i].width - 954 format->format.width) + 955 abs(mt9v111_frame_sizes[i].height - 956 format->format.height); 957 if (fit < best_fit) { 958 best_fit = fit; 959 idx = i; 960 961 if (fit == 0) 962 break; 963 } 964 } 965 new_fmt.width = mt9v111_frame_sizes[idx].width; 966 new_fmt.height = mt9v111_frame_sizes[idx].height; 967 968 /* Update the device (or pad) format if it has changed. */ 969 __fmt = __mt9v111_get_pad_format(mt9v111, cfg, format->pad, 970 format->which); 971 972 /* Format hasn't changed, stop here. */ 973 if (__fmt->code == new_fmt.code && 974 __fmt->width == new_fmt.width && 975 __fmt->height == new_fmt.height) 976 goto done; 977 978 /* Update the format and sizes, then mark changes as pending. */ 979 __fmt->code = new_fmt.code; 980 __fmt->width = new_fmt.width; 981 __fmt->height = new_fmt.height; 982 983 if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) 984 mt9v111->pending = true; 985 986 dev_info(mt9v111->dev, "%s: mbus_code: %x - (%ux%u)\n", 987 __func__, __fmt->code, __fmt->width, __fmt->height); 988 989 done: 990 format->format = *__fmt; 991 992 return 0; 993 } 994 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 48245 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 2018-06-11 15:17 ` [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 Jacopo Mondi 2018-06-11 16:44 ` kbuild test robot @ 2018-07-17 9:48 ` Kieran Bingham 2018-07-23 14:08 ` jacopo mondi 2018-07-23 14:28 ` Sakari Ailus 2 siblings, 1 reply; 12+ messages in thread From: Kieran Bingham @ 2018-07-17 9:48 UTC (permalink / raw) To: Jacopo Mondi, mchehab; +Cc: linux-media, linux-kernel, linux-renesas-soc Hi Jacopo, Thank you for the patch, Some comments inline. Feel free to add my Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> when relevant comments are addressed. Regards Kieran On 11/06/18 16:17, Jacopo Mondi wrote: > Add V4L2 sensor driver for Aptina MT9V111 CMOS image sensor. > > The MT9V111 is a 1/4-Inch CMOS image sensor based on MT9V011 with an > integrated Image Flow Processor. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > MAINTAINERS | 8 + > drivers/media/i2c/Kconfig | 12 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/mt9v111.c | 1297 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1318 insertions(+) > create mode 100644 drivers/media/i2c/mt9v111.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a38e24a..2c2fe60 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9523,6 +9523,14 @@ F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt > F: drivers/media/i2c/mt9v032.c > F: include/media/i2c/mt9v032.h > > +MT9V111 APTINA CAMERA SENSOR > +M: Jacopo Mondi <jacopo@jmondi.org> > +L: linux-media@vger.kernel.org > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > +F: drivers/media/i2c/mt9v111.c > + > MULTIFUNCTION DEVICES (MFD) > M: Lee Jones <lee.jones@linaro.org> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 341452f..0bd867d 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -841,6 +841,18 @@ config VIDEO_MT9V032 > This is a Video4Linux2 sensor-level driver for the Micron > MT9V032 752x480 CMOS sensor. > > +config VIDEO_MT9V111 > + tristate "Aptina MT9V111 sensor support" > + depends on I2C && VIDEO_V4L2 > + depends on MEDIA_CAMERA_SUPPORT > + depends on OF > + help > + This is a Video4Linux2 sensor-level driver for the Aptina/Micron > + MT9V111 sensor. > + > + To compile this driver as a module, choose M here: the > + module will be called mt9v111. > + > config VIDEO_SR030PC30 > tristate "Siliconfile SR030PC30 sensor support" > depends on I2C && VIDEO_V4L2 > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index d679d57..f0e8618 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o > obj-$(CONFIG_VIDEO_MT9T112) += mt9t112.o > obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o > obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o > +obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o > obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o > obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o > obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > new file mode 100644 > index 0000000..36e7424 > --- /dev/null > +++ b/drivers/media/i2c/mt9v111.c > @@ -0,0 +1,1297 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * V4L2 sensor driver for Aptina MT9V111 image sensor > + * Copyright (C) 2018 Jacopo Mondi <jacopo@jmondi.org> > + * > + * Based on mt9v032 driver > + * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> > + * > + * Base on mt9v011 driver I realise it's probably a copy-paste from another driver and not your text, but s/Base/Based/ > + * Copyright (c) 2009 Mauro Carvalho Chehab <mchehab@kernel.org> > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +#include <linux/videodev2.h> > +#include <linux/v4l2-mediabus.h> > +#include <linux/module.h> > + > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-fwnode.h> > +#include <media/v4l2-image-sizes.h> > +#include <media/v4l2-subdev.h> > + > +/* > + * MT9V111 is a 1/4-Inch CMOS digital image sensor with an integrated > + * Image Flow Processing (IFP) engine and a sensor core loosely based on > + * MT9V011. > + * > + * The IFP can produce several output image formats from the sensor core > + * output, this driver currently supports only YUYV format permutations. s/output, this/output. This/ > + * > + * As the auto exposure algorithms controls exposure time and modifies the > + * frame rate, this driver disables auto exposure control, auto white balancing > + * and auto flickering avoidance to allow manual frame rate control through > + * s_frame_interval subdev operation or V4L2_CID_V/HBLANK controls. Should/Could this be an option perhaps (in the future) to allow someone to accept that the framerate will be a bit 'variable' but allow algorithms to run? > + * > + * While it seems possible to instruct the auto-exposure control algorithm to > + * respect a programmed frame rate when adjusting the pixel integration time, > + * registers controlling this feature are not documented in the public > + * available sensor manual used to develop this driver (09005aef80e90084, > + * MT9V111_1.fm - Rev. G 1/05 EN). Aha - Ok - so the 'right' way is to fix the rate, but we don't have the documentation to be able to do so. :( > + */ > + > +#define MT9V111_CHIP_ID_HIGH 0x82 > +#define MT9V111_CHIP_ID_LOW 0x3a > + > +#define MT9V111_R01_ADDR_SPACE 0x01 > +#define MT9V111_R01_IFP 0x01 Two identical values (0x01)... there's not a copy-paste error there is there ? (I don't yet have the doc to check) Edit: I assume it's correct actually - as the first is the register space, and then IFP is probably the first register in that space... > +#define MT9V111_R01_CORE 0x04 > + > +#define MT9V111_IFP_R06_OPMODE_CTRL 0x06 > +#define MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN BIT(1) > +#define MT9V111_IFP_R06_OPMODE_CTRL_AE_EN BIT(14) > +#define MT9V111_IFP_R07_IFP_RESET 0x07 > +#define MT9V111_IFP_R07_IFP_RESET_MASK BIT(0) > +#define MT9V111_IFP_R08_OUTFMT_CTRL 0x08 > +#define MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER BIT(11) > +#define MT9V111_IFP_R08_OUTFMT_CTRL_PCLK BIT(5) > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2 0x3a > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR BIT(0) > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC BIT(1) > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK GENMASK(2, 0) > +#define MT9V111_IFP_RA5_HPAN 0xa5 > +#define MT9V111_IFP_RA6_HZOOM 0xa6 > +#define MT9V111_IFP_RA7_HOUT 0xa7 > +#define MT9V111_IFP_RA8_VPAN 0xa8 > +#define MT9V111_IFP_RA9_VZOOM 0xa9 > +#define MT9V111_IFP_RAA_VOUT 0xaa > +#define MT9V111_IFP_DECIMATION_MASK GENMASK(9, 0) > +#define MT9V111_IFP_DECIMATION_FREEZE BIT(15) > + > +#define MT9V111_CORE_R03_WIN_HEIGHT 0x03 > +#define MT9V111_CORE_R03_WIN_V_OFFS 2 > +#define MT9V111_CORE_R04_WIN_WIDTH 0x04 > +#define MT9V111_CORE_R04_WIN_H_OFFS 114 > +#define MT9V111_CORE_R05_HBLANK 0x05 > +#define MT9V111_CORE_R05_MIN_HBLANK 0x09 > +#define MT9V111_CORE_R05_MAX_HBLANK GENMASK(9, 0) > +#define MT9V111_CORE_R05_DEF_HBLANK 0x26 > +#define MT9V111_CORE_R06_VBLANK 0x06 > +#define MT9V111_CORE_R06_MIN_VBLANK 0x03 > +#define MT9V111_CORE_R06_MAX_VBLANK GENMASK(11, 0) > +#define MT9V111_CORE_R06_DEF_VBLANK 0x04 > +#define MT9V111_CORE_R07_OUT_CTRL 0x07 > +#define MT9V111_CORE_R07_OUT_CTRL_SAMPLE BIT(4) > +#define MT9V111_CORE_R09_PIXEL_INT 0x09 > +#define MT9V111_CORE_R09_PIXEL_INT_MASK GENMASK(11, 0) > +#define MT9V111_CORE_R0D_CORE_RESET 0x0d > +#define MT9V111_CORE_R0D_CORE_RESET_MASK BIT(0) > +#define MT9V111_CORE_RFF_CHIP_VER 0xff > + > +#define MT9V111_PIXEL_ARRAY_WIDTH 640 > +#define MT9V111_PIXEL_ARRAY_HEIGHT 480 > + > +#define MT9V111_MAX_CLKIN 27000000 > + > +struct mt9v111_dev { > + struct device *dev; > + struct i2c_client *client; > + > + /* Protects address space change operations. */ > + spinlock_t addr_lock; > + u8 addr_space; > + > + struct v4l2_subdev sd; > +#if defined(CONFIG_MEDIA_CONTROLLER) I wonder if one day we'll have better integration so that we don't have to be conditional on CONFIG_MEDIA_CONTROLLER. That said, would it be better to use?: #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER) > + struct media_pad pad; > +#endif > + > + struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *vblank; > + struct v4l2_ctrl_handler ctrls; > + > + /* Output image format and sizes. */ > + struct v4l2_mbus_framefmt fmt; > + unsigned int fps; > + > + /* Protects power up/down sequences. */ > + struct mutex pwr_mutex; > + int pwr_count; > + > + /* Protects stream on/off sequences. */ > + struct mutex stream_mutex; > + bool streaming; > + > + /* Flags to mark HW settings as not yet applied. */ > + bool pending; > + > + /* Clock provider and system clock frequency. */ > + struct clk *clk; > + u32 sysclk; > + > + struct gpio_desc *oe; > + struct gpio_desc *standby; > + struct gpio_desc *reset; > +}; > + > +#define sd_to_mt9v111(__sd) container_of(__sd, struct mt9v111_dev, sd) I believe it's recommended to bracket macro parameters: #define sd_to_mt9v111(__sd) container_of((__sd), struct mt9v111_dev, sd) > + > +/* > + * mt9v111_mbus_fmt - List all media bus formats supported by the driver. > + * > + * Only list the media bus code here. The image sizes are freely configurable > + * in the pixel array sizes range. > + * > + * The frame desired frame interval, in the supported frame interval range, is "The frame desired frame interval,"? Did you mean "The desired frame interval,"? > + * obtained by configuring blanking as the sensor does not have a PLL but > + * only a fixed clock divider that generates the output pixel clock. > + */ > +static struct mt9v111_mbus_fmt { > + u32 code; > +} mt9v111_formats[] = { > + { > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > + }, > + { > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > + }, > + { > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > + }, > + { > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > + }, > +}; > + > +static u32 mt9v111_frame_intervals[] = {5, 10, 15, 20, 30}; > + > +/* > + * mt9v111_frame_sizes - List sensor's supported resolutions. > + * > + * Resolution generated through decimation in the IFP block from the > + * full VGA pixel array. > + */ > +static struct v4l2_rect mt9v111_frame_sizes[] = { > + { > + .width = 640, > + .height = 480, > + }, > + { > + .width = 352, > + .height = 288 > + }, > + { > + .width = 320, > + .height = 240, > + }, > + { > + .width = 176, > + .height = 144, > + }, > + { > + .width = 160, > + .height = 120, > + }, > +}; > + > +/* Initial register configuration. */ > +static struct mt9v111_reg_list { > + u8 space; > + u8 reg; > + u16 mask; > + u16 val; > +} mt9v111_init_config[] = { > + { > + /* Disable AE and AWB to enable manual frame rate control. */ > + .space = MT9V111_R01_IFP, > + .reg = MT9V111_IFP_R06_OPMODE_CTRL, > + .mask = MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN | > + MT9V111_IFP_R06_OPMODE_CTRL_AE_EN, > + .val = 0, > + }, > + { > + /* Disable anti-flicker to enable manual frame rate control. */ > + .space = MT9V111_R01_IFP, > + .reg = MT9V111_IFP_R08_OUTFMT_CTRL, > + .mask = MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER, > + .val = 0, > + }, > + > + /* TODO: AE and AWB manual configuration. */ > +}; > + > +/* --- Device I/O access --- */ > + > +static int __mt9v111_read(struct i2c_client *c, u8 reg, u16 *val) > +{ > + struct i2c_msg msg[2]; > + __be16 buf; > + int ret; > + > + msg[0].addr = c->addr; > + msg[0].flags = 0; > + msg[0].len = 1; > + msg[0].buf = ® > + > + msg[1].addr = c->addr; > + msg[1].flags = I2C_M_RD; > + msg[1].len = 2; > + msg[1].buf = (char *)&buf; > + > + ret = i2c_transfer(c->adapter, msg, 2); > + if (ret < 0) { > + dev_err(&c->dev, "i2c read transfer error: %d\n", ret); > + return ret; > + } > + > + *val = be16_to_cpu(buf); > + > + dev_info(&c->dev, "%s: %x=%x\n", __func__, reg, *val); dev_info seems a bit verbose to be dumping all register writes ... shouldn't this be dev_dbg? > + > + return 0; > +} > + > +static int __mt9v111_write(struct i2c_client *c, u8 reg, u16 val) > +{ > + struct i2c_msg msg; > + u8 buf[3] = { 0 }; > + int ret; > + > + buf[0] = reg; > + buf[1] = val >> 8; > + buf[2] = val & 0xff; > + > + msg.addr = c->addr; > + msg.flags = 0; > + msg.len = 3; > + msg.buf = (char *)buf; > + > + dev_info(&c->dev, "%s: %x = %x%x\n", __func__, reg, buf[1], buf[2]); same here ... should this be dev_dbg? > + > + ret = i2c_transfer(c->adapter, &msg, 1); > + if (ret < 0) { > + dev_err(&c->dev, "i2c write transfer error: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int __mt9v111_addr_space_select(struct i2c_client *c, u16 addr_space) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(c); > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + u16 val; > + int ret; > + > + spin_lock(&mt9v111->addr_lock); > + if (mt9v111->addr_space == addr_space) { > + spin_unlock(&mt9v111->addr_lock); > + return 0; > + } > + spin_unlock(&mt9v111->addr_lock); > + > + ret = __mt9v111_write(c, MT9V111_R01_ADDR_SPACE, addr_space); > + if (ret) > + return ret; > + > + /* Verify address space has been updated */ > + ret = __mt9v111_read(c, MT9V111_R01_ADDR_SPACE, &val); > + if (ret) > + return ret; > + > + if (val != addr_space) > + return -EINVAL; > + > + mt9v111->addr_space = addr_space; Should this write to the mt9v111->addr_space be locked, considering the read at the beginning of the function is locked? > + > + return 0; > +} > + > +static int mt9v111_read(struct i2c_client *c, u8 addr_space, u8 reg, u16 *val) > +{ > + int ret; > + > + /* Select register address space first. */ > + ret = __mt9v111_addr_space_select(c, addr_space); > + if (ret) > + return ret; > + > + ret = __mt9v111_read(c, reg, val); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int mt9v111_write(struct i2c_client *c, u8 addr_space, u8 reg, u16 val) > +{ > + int ret; > + > + /* Select register address space first. */ > + ret = __mt9v111_addr_space_select(c, addr_space); > + if (ret) > + return ret; > + > + ret = __mt9v111_write(c, reg, val); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int mt9v111_update(struct i2c_client *c, u8 addr_space, u8 reg, > + u16 mask, u16 val) > +{ > + u16 current_val; > + int ret; > + > + /* Select register address space first. */ > + ret = __mt9v111_addr_space_select(c, addr_space); > + if (ret) > + return ret; > + > + /* Read the current register value, then update it. */ > + ret = __mt9v111_read(c, reg, ¤t_val); > + if (ret) > + return ret; > + > + current_val &= ~mask; > + current_val |= (val & mask); > + ret = __mt9v111_write(c, reg, current_val); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int mt9v111_update_reglist(struct i2c_client *c, > + struct mt9v111_reg_list *reg_list, > + unsigned int n_regs) > +{ > + unsigned int i; > + int ret; > + > + for (i = 0; i < n_regs; i++) { > + struct mt9v111_reg_list *reg = ®_list[i]; > + > + ret = mt9v111_update(c, reg->space, reg->reg, reg->mask, > + reg->val); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +/* --- Sensor HW operations --- */ > + > +static int __mt9v111_power_on(struct v4l2_subdev *sd) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + int ret; > + > + ret = clk_prepare_enable(mt9v111->clk); > + if (ret) > + return ret; > + > + clk_set_rate(mt9v111->clk, mt9v111->sysclk); > + > + gpiod_set_value(mt9v111->standby, 0); > + usleep_range(500, 1000); > + > + gpiod_set_value(mt9v111->oe, 1); > + usleep_range(500, 1000); > + > + return 0; > +} > + > +static int __mt9v111_power_off(struct v4l2_subdev *sd) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + > + gpiod_set_value(mt9v111->oe, 0); > + usleep_range(500, 1000); > + > + gpiod_set_value(mt9v111->standby, 1); > + usleep_range(500, 1000); > + > + clk_disable_unprepare(mt9v111->clk); > + > + return 0; > +} > + > +static int __mt9v111_hw_reset(struct mt9v111_dev *mt9v111) > +{ > + if (!mt9v111->reset) > + return -EINVAL; > + > + gpiod_set_value(mt9v111->reset, 1); > + usleep_range(500, 1000); > + > + gpiod_set_value(mt9v111->reset, 0); > + usleep_range(500, 1000); > + > + return 0; > +} > + > +static int __mt9v111_sw_reset(struct mt9v111_dev *mt9v111) > +{ > + struct i2c_client *c = mt9v111->client; > + unsigned int ret; But the return type of the function is not unsigned? > + > + /* Software reset core and IFP blocks. */ > + > + ret = mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R0D_CORE_RESET, > + MT9V111_CORE_R0D_CORE_RESET_MASK, 1); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + ret = mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R0D_CORE_RESET, > + MT9V111_CORE_R0D_CORE_RESET_MASK, 0); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + ret = mt9v111_update(c, MT9V111_R01_IFP, > + MT9V111_IFP_R07_IFP_RESET, > + MT9V111_IFP_R07_IFP_RESET_MASK, 1); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + ret = mt9v111_update(c, MT9V111_R01_IFP, > + MT9V111_IFP_R07_IFP_RESET, > + MT9V111_IFP_R07_IFP_RESET_MASK, 0); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + return 0; > +} > + > +static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111, > + struct v4l2_fract *tpf) > +{ > + unsigned int fps = tpf->numerator ? > + tpf->denominator / tpf->numerator : > + tpf->denominator; > + unsigned int best_diff; > + unsigned int frm_cols; > + unsigned int row_pclk; > + unsigned int best_fps; > + unsigned int pclk; > + unsigned int diff; > + unsigned int idx; > + unsigned int hb; > + unsigned int vb; > + unsigned int i; > + int ret; > + > + /* Approximate to the closest supported frame interval. */ > + best_diff = ~0L;> + for (i = 0, idx = 0; i < ARRAY_SIZE(mt9v111_frame_intervals); i++) { > + diff = abs(fps - mt9v111_frame_intervals[i]); > + if (diff < best_diff) {> + idx = i; > + best_diff = diff; > + } > + } > + fps = mt9v111_frame_intervals[idx]; > + > + /* > + * The sensor does not provide a PLL circuitry and pixel clock is > + * generated dividing by two the master clock source. "The sensor does not provide a PLL circuitry and the pixel clock is generated by dividing the master clock source by two." ? > + * > + * Trow = (W + Hblank + 114) * 2 * (1 / SYSCLK) > + * TFrame = Trow * (H + Vblank + 2) > + * > + * FPS = (SYSCLK / 2) / (Trow * (H + Vblank + 2)) > + * > + * This boils down to tune H and V blanks to best approximate the > + * above equation. > + * > + * Test all available H/V blank values, until we reach the > + * desired frame rate. > + */ > + best_fps = vb = hb = 0; > + pclk = DIV_ROUND_CLOSEST(mt9v111->sysclk, 2); > + row_pclk = MT9V111_PIXEL_ARRAY_WIDTH + 7 + MT9V111_CORE_R04_WIN_H_OFFS; > + frm_cols = MT9V111_PIXEL_ARRAY_HEIGHT + 7 + MT9V111_CORE_R03_WIN_V_OFFS; > + > + best_diff = ~0L; > + for (vb = MT9V111_CORE_R06_MIN_VBLANK; > + vb < MT9V111_CORE_R06_MAX_VBLANK; vb++) { > + for (hb = MT9V111_CORE_R05_MIN_HBLANK; > + hb < MT9V111_CORE_R05_MAX_HBLANK; hb += 10) { > + unsigned int t_frame = (row_pclk + hb) * > + (frm_cols + vb); > + unsigned int t_fps = DIV_ROUND_CLOSEST(pclk, t_frame); > + > + diff = abs(fps - t_fps); > + if (diff < best_diff) { > + best_diff = diff; > + best_fps = t_fps; > + > + if (diff == 0) > + break; > + } > + } > + > + if (diff == 0) > + break; > + } > + > + dev_info(mt9v111->dev, "FPS: %u, hb = %u, vb = %u\n", fps, hb, vb); > + > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->hblank, hb); > + if (ret) > + return ret; > + > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->vblank, vb); > + if (ret) > + return ret; > + > + tpf->numerator = 1; > + tpf->denominator = best_fps; > + > + return 0; > +} > + > +static int mt9v111_hw_config(struct mt9v111_dev *mt9v111) > +{ > + struct i2c_client *c = mt9v111->client; > + unsigned int ret; > + u16 outfmtctrl2; > + > + /* Force device reset. */ > + ret = __mt9v111_hw_reset(mt9v111); > + if (ret == -EINVAL) > + ret = __mt9v111_sw_reset(mt9v111); > + if (ret) > + return ret; > + > + /* > + * Disable AE, AWB and anti-flickering to allow manual frame rate > + * control. > + */ > + ret = mt9v111_update_reglist(c, mt9v111_init_config, > + ARRAY_SIZE(mt9v111_init_config)); > + if (ret) > + return ret; > + > + /* Configure internal clock sample rate. */ > + ret = mt9v111->sysclk < DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? > + mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R07_OUT_CTRL, > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 1) : > + mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R07_OUT_CTRL, > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 0); > + if (ret) > + return ret; > + > + /* > + * Configure output image format components ordering. > + * > + * TODO: IFP block can also output several RGB permutations, we only > + * support YUYV permutations at the moment. > + */ > + switch (mt9v111->fmt.code) { > + case MEDIA_BUS_FMT_YUYV8_2X8: > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC; > + break; > + case MEDIA_BUS_FMT_VYUY8_2X8: > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > + break; > + case MEDIA_BUS_FMT_YVYU8_2X8: > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC | > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > + break; > + case MEDIA_BUS_FMT_UYVY8_2X8: > + default: > + outfmtctrl2 = 0; > + break; > + } > + > + ret = mt9v111_update(c, MT9V111_R01_IFP, MT9V111_IFP_R3A_OUTFMT_CTRL2, > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK, > + outfmtctrl2); > + if (ret) > + return ret; > + > + /* > + * Do not change default sensor's core configuration: > + * output the whole 640x480 pixel array, skip 18 columns and 6 rows. I'm not sure I understand why we skip 18 columns and 6 rows? (or does it not matter, and it's just necessary) > + * > + * Instead, control the output image size through IFP block. > + * > + * TODO: No zoom&pan support. Currently we control the output image > + * size only through decimation, with no zoom support. > + */ > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA5_HPAN, > + MT9V111_IFP_DECIMATION_FREEZE); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA8_VPAN, > + MT9V111_IFP_DECIMATION_FREEZE); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA6_HZOOM, > + MT9V111_IFP_DECIMATION_FREEZE | > + MT9V111_PIXEL_ARRAY_WIDTH); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA9_VZOOM, > + MT9V111_IFP_DECIMATION_FREEZE | > + MT9V111_PIXEL_ARRAY_HEIGHT); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA7_HOUT, > + MT9V111_IFP_DECIMATION_FREEZE | > + mt9v111->fmt.width); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RAA_VOUT, > + mt9v111->fmt.height); > + if (ret) > + return ret; > + > + /* Update blankings to match the programmed frame rate. */ > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R05_HBLANK, > + MT9V111_CORE_R05_MAX_HBLANK, > + mt9v111->hblank->val); > + if (ret) > + return ret; > + > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R06_VBLANK, > + MT9V111_CORE_R06_MAX_VBLANK, > + mt9v111->vblank->val); > + if (ret) > + return ret; > + > + /* > + * Set pixel integration time to the whole frame time. > + * This value controls the the shutter delay when running with AE > + * disabled. If longer than frame time, it affects the output > + * frame rate. > + */ > + ret = mt9v111_write(c, MT9V111_R01_CORE, MT9V111_CORE_R09_PIXEL_INT, > + MT9V111_PIXEL_ARRAY_HEIGHT); > + if (ret) > + return ret; > + > + return 0; > +} > + > +/* --- V4L2 subdev operations --- */ > + > +static int mt9v111_s_power(struct v4l2_subdev *sd, int on) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + int pwr_count; > + int ret = 0; > + > + mutex_lock(&mt9v111->pwr_mutex); > + > + /* > + * Make sure we're transitioning from 0 to 1, or viceversa, only > + * before actually changing the power state. > + */ > + pwr_count = mt9v111->pwr_count; > + pwr_count += on ? 1 : -1; > + if (pwr_count == !!on) { > + ret = on ? __mt9v111_power_on(sd) : > + __mt9v111_power_off(sd); > + if (!ret) > + /* All went well, updated power counter. */ > + mt9v111->pwr_count = pwr_count; > + > + mutex_unlock(&mt9v111->pwr_mutex); > + > + return ret; > + } > + > + /* > + * Update power counter to keep track of how many nested calls we > + * received. > + */ > + WARN_ON(pwr_count < 0 || pwr_count > 1); > + mt9v111->pwr_count = pwr_count; > + > + mutex_unlock(&mt9v111->pwr_mutex); > + > + return ret; > +} > + > +static int mt9v111_s_stream(struct v4l2_subdev *subdev, int enable) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > + int ret; > + > + mutex_lock(&mt9v111->stream_mutex); > + > + if (mt9v111->streaming == enable) { > + mutex_unlock(&mt9v111->stream_mutex); > + return 0; > + } > + > + ret = mt9v111_s_power(subdev, enable); > + if (ret) > + goto error_unlock; > + > + if (enable && mt9v111->pending) { > + ret = mt9v111_hw_config(mt9v111); > + if (ret) > + goto error_unlock; > + > + /* > + * No need to update control here as far as only H/VBLANK are > + * supported and immediately programmed to registers in .s_ctrl > + */ > + > + mt9v111->pending = false; > + } > + > + mt9v111->streaming = enable ? true : false; > + mutex_unlock(&mt9v111->stream_mutex); > + > + return 0; > + > +error_unlock: > + mutex_unlock(&mt9v111->stream_mutex); > + > + return ret; > +} > + > +static int mt9v111_s_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *ival) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + struct v4l2_fract *tpf = &ival->interval; > + unsigned int fps = tpf->numerator ? > + tpf->denominator / tpf->numerator : > + tpf->denominator; > + unsigned int max_fps; > + > + mutex_lock(&mt9v111->stream_mutex); > + if (mt9v111->streaming) { > + mutex_unlock(&mt9v111->stream_mutex); > + return -EBUSY; > + } > + mutex_unlock(&mt9v111->stream_mutex); > + > + if (mt9v111->fps == fps) > + return 0; > + > + /* Make sure frame rate/image sizes constraints are respected. */ > + if (mt9v111->fmt.width < QVGA_WIDTH && > + mt9v111->fmt.height < QVGA_HEIGHT) > + max_fps = 90; > + else if (mt9v111->fmt.width < CIF_WIDTH && > + mt9v111->fmt.height < CIF_HEIGHT) > + max_fps = 60; > + else > + max_fps = mt9v111->sysclk < > + DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? 15 : > + 30; > + > + if (fps > max_fps) > + return -EINVAL; > + > + mt9v111_calc_frame_rate(mt9v111, tpf); > + > + mt9v111->fps = fps; > + mt9v111->pending = true; > + > + return 0; > +} > + > +static int mt9v111_g_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *ival) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + struct v4l2_fract *tpf = &ival->interval; > + > + tpf->numerator = 1; > + tpf->denominator = mt9v111->fps; > + > + return 0; > +} > + > +static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format( > + struct mt9v111_dev *mt9v111, > + struct v4l2_subdev_pad_config *cfg, > + unsigned int pad, > + enum v4l2_subdev_format_whence which) > +{ > + switch (which) { > + case V4L2_SUBDEV_FORMAT_TRY: > +#ifdef CONFIG_MEDIA_CONTROLLER #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? > + return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad); > +#else > + return &cfg->try_fmt; > +#endif > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &mt9v111->fmt; > + default: > + return NULL; > + } > +} > + > +static int mt9v111_enum_mbus_code(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + if (code->pad || code->index > ARRAY_SIZE(mt9v111_formats) - 1) > + return -EINVAL; > + > + code->code = mt9v111_formats[code->index].code; > + > + return 0; > +} > + > +static int mt9v111_enum_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_interval_enum *fie) > +{ > + unsigned int i; > + > + if (fie->pad || fie->index >= ARRAY_SIZE(mt9v111_frame_intervals)) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) > + if (fie->width == mt9v111_frame_sizes[i].width && > + fie->height == mt9v111_frame_sizes[i].height) > + break; > + > + if (i == ARRAY_SIZE(mt9v111_frame_sizes)) > + return -EINVAL; > + > + fie->interval.numerator = 1; > + fie->interval.denominator = mt9v111_frame_intervals[fie->index]; > + > + return 0; > +} > + > +static int mt9v111_enum_frame_size(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->pad || fse->index > ARRAY_SIZE(mt9v111_frame_sizes)) > + return -EINVAL; > + > + fse->min_width = mt9v111_frame_sizes[fse->index].width; > + fse->max_width = mt9v111_frame_sizes[fse->index].width; > + fse->min_height = mt9v111_frame_sizes[fse->index].height; > + fse->max_height = mt9v111_frame_sizes[fse->index].height; > + > + return 0; > +} > + > +static int mt9v111_get_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > + > + if (format->pad) > + return -EINVAL; > + > + format->format = *__mt9v111_get_pad_format(mt9v111, cfg, format->pad, > + format->which); > + return 0; > +} > + > +static int mt9v111_set_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > + struct v4l2_mbus_framefmt new_fmt; > + struct v4l2_mbus_framefmt *__fmt; > + unsigned int best_fit = ~0L; > + unsigned int idx; > + unsigned int i; > + > + mutex_lock(&mt9v111->stream_mutex); > + if (mt9v111->streaming) { > + mutex_unlock(&mt9v111->stream_mutex); > + return -EBUSY; > + } > + mutex_unlock(&mt9v111->stream_mutex); > + > + if (format->pad) > + return -EINVAL; > + > + /* Update mbus format code and sizes. */ > + for (i = 0; i < ARRAY_SIZE(mt9v111_formats); i++) { > + if (format->format.code == mt9v111_formats[i].code) { > + new_fmt.code = mt9v111_formats[i].code; > + break; > + } > + } > + if (i == ARRAY_SIZE(mt9v111_formats)) > + new_fmt.code = mt9v111_formats[0].code; > + > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) { > + unsigned int fit = abs(mt9v111_frame_sizes[i].width - > + format->format.width) + > + abs(mt9v111_frame_sizes[i].height - > + format->format.height); > + if (fit < best_fit) { > + best_fit = fit; > + idx = i; > + > + if (fit == 0) > + break; > + } > + } > + new_fmt.width = mt9v111_frame_sizes[idx].width; > + new_fmt.height = mt9v111_frame_sizes[idx].height; > + > + /* Update the device (or pad) format if it has changed. */ > + __fmt = __mt9v111_get_pad_format(mt9v111, cfg, format->pad, > + format->which); > + > + /* Format hasn't changed, stop here. */ > + if (__fmt->code == new_fmt.code && > + __fmt->width == new_fmt.width && > + __fmt->height == new_fmt.height) > + goto done; > + > + /* Update the format and sizes, then mark changes as pending. */ > + __fmt->code = new_fmt.code; > + __fmt->width = new_fmt.width; > + __fmt->height = new_fmt.height; > + > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + mt9v111->pending = true; > + > + dev_info(mt9v111->dev, "%s: mbus_code: %x - (%ux%u)\n", > + __func__, __fmt->code, __fmt->width, __fmt->height); > + > +done: > + format->format = *__fmt; > + > + return 0; > +} > + > +static const struct v4l2_subdev_core_ops mt9v111_core_ops = { > + .s_power = mt9v111_s_power, > +}; > + > +static const struct v4l2_subdev_video_ops mt9v111_video_ops = { > + .s_stream = mt9v111_s_stream, > + .s_frame_interval = mt9v111_s_frame_interval, > + .g_frame_interval = mt9v111_g_frame_interval, > +}; > + > +static const struct v4l2_subdev_pad_ops mt9v111_pad_ops = { > + .enum_mbus_code = mt9v111_enum_mbus_code, > + .enum_frame_size = mt9v111_enum_frame_size, > + .enum_frame_interval = mt9v111_enum_frame_interval, > + .get_fmt = mt9v111_get_format, > + .set_fmt = mt9v111_set_format, > +}; > + > +static const struct v4l2_subdev_ops mt9v111_ops = { > + .core = &mt9v111_core_ops, > + .video = &mt9v111_video_ops, > + .pad = &mt9v111_pad_ops, > +}; > + > +#ifdef CONFIG_MEDIA_CONTROLLER #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? > +static const struct media_entity_operations mt9v111_subdev_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static int mt9v111_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + struct v4l2_mbus_framefmt *__fmt = > + v4l2_subdev_get_try_format(sd, fh->pad, 0); > + > + /* Initialize try format: copy the device format. */ > + *__fmt = mt9v111->fmt; > + > + return 0; > +} > + > +static const struct v4l2_subdev_internal_ops mt9v111_internal_ops = { > + .open = mt9v111_open, > +}; > +#endif > + > +/* --- V4L2 ctrl --- */ > +static int mt9v111_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct mt9v111_dev *mt9v111 = container_of(ctrl->handler, > + struct mt9v111_dev, > + ctrls); > + int ret = 0; > + > + mutex_lock(&mt9v111->pwr_mutex); > + /* > + * If sensor is powered down, just cache new control values, > + * no actual register access. > + */ > + if (!mt9v111->pwr_count) { > + mutex_unlock(&mt9v111->pwr_mutex); > + return 0; > + } > + mutex_unlock(&mt9v111->pwr_mutex); > + > + switch (ctrl->id) { > + case V4L2_CID_HBLANK: > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > + MT9V111_CORE_R05_HBLANK, > + MT9V111_CORE_R05_MAX_HBLANK, > + mt9v111->hblank->val); > + break; > + case V4L2_CID_VBLANK: > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > + MT9V111_CORE_R06_VBLANK, > + MT9V111_CORE_R06_MAX_VBLANK, > + mt9v111->vblank->val); > + break; > + } > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops mt9v111_ctrl_ops = { > + .s_ctrl = mt9v111_s_ctrl, > +}; > + > +static int mt9v111_chip_probe(struct mt9v111_dev *mt9v111) > +{ > + int ret; > + u16 val; > + > + ret = __mt9v111_power_on(&mt9v111->sd); > + if (ret) > + return ret; > + > + ret = mt9v111_read(mt9v111->client, MT9V111_R01_CORE, > + MT9V111_CORE_RFF_CHIP_VER, &val); > + if (ret) > + return ret; > + > + if ((val >> 8) != MT9V111_CHIP_ID_HIGH && > + (val & 0xff) != MT9V111_CHIP_ID_LOW) { > + dev_err(mt9v111->dev, > + "Unable to identify MT9V111 chip: 0x%2x%2x\n", > + val >> 8, val & 0xff); > + return ret; > + } > + > + ret = __mt9v111_power_off(&mt9v111->sd); > + if (ret) > + return ret; > + > + dev_info(mt9v111->dev, "Chip identified: 0x%2x%2x\n", > + val >> 8, val & 0xff); > + > + return 0; > +} > + > +static int mt9v111_probe(struct i2c_client *client) > +{ > + struct mt9v111_dev *mt9v111; > + struct v4l2_fract tpf; > + int ret; > + > + mt9v111 = devm_kzalloc(&client->dev, sizeof(*mt9v111), GFP_KERNEL); > + if (!mt9v111) > + return -ENOMEM; > + > + mt9v111->dev = &client->dev; > + mt9v111->client = client; > + > + mt9v111->clk = devm_clk_get(&client->dev, NULL); > + if (IS_ERR(mt9v111->clk)) > + return PTR_ERR(mt9v111->clk); > + > + mt9v111->sysclk = clk_get_rate(mt9v111->clk); > + if (mt9v111->sysclk > MT9V111_MAX_CLKIN) > + return -EINVAL; > + > + mt9v111->oe = devm_gpiod_get_optional(&client->dev, "enable", > + GPIOD_OUT_LOW); > + if (IS_ERR(mt9v111->oe)) { > + dev_err(&client->dev, "Unable to get GPIO \"enable\": %ld\n", > + PTR_ERR(mt9v111->oe)); > + return PTR_ERR(mt9v111->oe); > + } > + > + mt9v111->standby = devm_gpiod_get_optional(&client->dev, "standby", > + GPIOD_OUT_HIGH); > + if (IS_ERR(mt9v111->standby)) { > + dev_err(&client->dev, "Unable to get GPIO \"standby\": %ld\n", > + PTR_ERR(mt9v111->standby)); > + return PTR_ERR(mt9v111->standby); > + } > + > + mt9v111->reset = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_OUT_LOW); > + if (IS_ERR(mt9v111->reset)) { > + dev_err(&client->dev, "Unable to get GPIO \"reset\": %ld\n", > + PTR_ERR(mt9v111->reset)); > + return PTR_ERR(mt9v111->reset); > + } > + > + mutex_init(&mt9v111->pwr_mutex); > + mutex_init(&mt9v111->stream_mutex); > + spin_lock_init(&mt9v111->addr_lock); > + > + v4l2_ctrl_handler_init(&mt9v111->ctrls, 3); > + > + mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > + V4L2_CID_HBLANK, > + MT9V111_CORE_R05_MIN_HBLANK, > + MT9V111_CORE_R05_MAX_HBLANK, 1, > + MT9V111_CORE_R05_DEF_HBLANK); > + if (IS_ERR_OR_NULL(mt9v111->hblank)) { > + ret = PTR_ERR(mt9v111->hblank); > + goto error_free_ctrls; > + } > + > + mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > + V4L2_CID_VBLANK, > + MT9V111_CORE_R06_MIN_VBLANK, > + MT9V111_CORE_R06_MAX_VBLANK, 1, > + MT9V111_CORE_R06_DEF_VBLANK); > + if (IS_ERR_OR_NULL(mt9v111->vblank)) { > + ret = PTR_ERR(mt9v111->vblank); > + goto error_free_ctrls; > + } > + > + /* PIXEL_RATE is fixed: just expose it to user space. */ > + v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > + V4L2_CID_PIXEL_RATE, 0, > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2), 1, > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2)); > + > + mt9v111->sd.ctrl_handler = &mt9v111->ctrls; > + > + /* Start with default configuration: 640x480 UYVY. */ > + mt9v111->fmt.width = 640; > + mt9v111->fmt.height = 480; > + mt9v111->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; > + > + /* These are fixed for all supported formats. */ > + mt9v111->fmt.field = V4L2_FIELD_NONE; > + mt9v111->fmt.colorspace = V4L2_COLORSPACE_SRGB; > + mt9v111->fmt.ycbcr_enc = V4L2_YCBCR_ENC_601; > + mt9v111->fmt.quantization = V4L2_QUANTIZATION_LIM_RANGE; > + mt9v111->fmt.xfer_func = V4L2_XFER_FUNC_SRGB; > + > + /* Re-calculate blankings for 640x480@15fps. */ > + mt9v111->fps = 15; > + tpf.numerator = 1; > + tpf.denominator = mt9v111->fps; > + ret = mt9v111_calc_frame_rate(mt9v111, &tpf); > + if (ret) > + goto error_free_ctrls; > + > + mt9v111->pwr_count = 0; > + mt9v111->addr_space = MT9V111_R01_IFP; > + mt9v111->pending = true; > + > + v4l2_i2c_subdev_init(&mt9v111->sd, client, &mt9v111_ops); > + > +#ifdef CONFIG_MEDIA_CONTROLLER #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? > + mt9v111->sd.internal_ops = &mt9v111_internal_ops; > + mt9v111->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + mt9v111->sd.entity.ops = &mt9v111_subdev_entity_ops; > + mt9v111->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + > + mt9v111->pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&mt9v111->sd.entity, 1, &mt9v111->pad); > + if (ret) > + goto error_free_ctrls; > +#endif > + > + ret = mt9v111_chip_probe(mt9v111); > + if (ret) > + goto error_free_ctrls; > + > + ret = v4l2_async_register_subdev(&mt9v111->sd); > + if (ret) > + goto error_free_ctrls; > + > + return 0; > + > +error_free_ctrls: > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > + > +#ifdef CONFIG_MEDIA_CONTROLLER > + media_entity_cleanup(&mt9v111->sd.entity); > +#endif media_entity_cleanup() has #if IS_ENABLED(MC) barriers around it with a null implementation so we could drop the #ifdef CONFIG_ here ... but perhaps while we have to have a non-MC supported implementation it keeps the distinction clear... Otherwise, #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? > + > + mutex_destroy(&mt9v111->pwr_mutex); > + mutex_destroy(&mt9v111->stream_mutex); > + > + return ret; > +} > + > +static int mt9v111_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + > + v4l2_async_unregister_subdev(sd); > + > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > + > +#ifdef CONFIG_MEDIA_CONTROLLER > + media_entity_cleanup(&sd->entity); > +#endif and here... > + > + mutex_destroy(&mt9v111->pwr_mutex); > + mutex_destroy(&mt9v111->stream_mutex); > + > + devm_gpiod_put(mt9v111->dev, mt9v111->oe); > + devm_gpiod_put(mt9v111->dev, mt9v111->standby); > + devm_gpiod_put(mt9v111->dev, mt9v111->reset); > + > + devm_clk_put(mt9v111->dev, mt9v111->clk); > + > + return 0; > +} > + > +static const struct of_device_id mt9v111_of_match[] = { > + { .compatible = "aptina,mt9v111", }, > + { /* sentinel */ }, > +}; > + > +static struct i2c_driver mt9v111_driver = { > + .driver = { > + .name = "mt9v111", > + .of_match_table = of_match_ptr(mt9v111_of_match), > + }, > + .probe_new = mt9v111_probe, > + .remove = mt9v111_remove, > +}; > + > +module_i2c_driver(mt9v111_driver); > + > +MODULE_DESCRIPTION("V4L2 sensor driver for Aptina MT9V111"); > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>"); > +MODULE_LICENSE("GPL v2"); -- Regards -- Kieran ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 2018-07-17 9:48 ` Kieran Bingham @ 2018-07-23 14:08 ` jacopo mondi 0 siblings, 0 replies; 12+ messages in thread From: jacopo mondi @ 2018-07-23 14:08 UTC (permalink / raw) To: Kieran Bingham Cc: Jacopo Mondi, mchehab, linux-media, linux-kernel, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 45880 bytes --] Hi Kieran, thanks for comments On Tue, Jul 17, 2018 at 10:48:33AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > Thank you for the patch, > > Some comments inline. > > Feel free to add my > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > when relevant comments are addressed. > > Regards > > Kieran > > > On 11/06/18 16:17, Jacopo Mondi wrote: > > Add V4L2 sensor driver for Aptina MT9V111 CMOS image sensor. > > > > The MT9V111 is a 1/4-Inch CMOS image sensor based on MT9V011 with an > > integrated Image Flow Processor. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > MAINTAINERS | 8 + > > drivers/media/i2c/Kconfig | 12 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/mt9v111.c | 1297 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1318 insertions(+) > > create mode 100644 drivers/media/i2c/mt9v111.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index a38e24a..2c2fe60 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9523,6 +9523,14 @@ F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > F: drivers/media/i2c/mt9v032.c > > F: include/media/i2c/mt9v032.h > > > > +MT9V111 APTINA CAMERA SENSOR > > +M: Jacopo Mondi <jacopo@jmondi.org> > > +L: linux-media@vger.kernel.org > > +T: git git://linuxtv.org/media_tree.git > > +S: Maintained > > +F: Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > > +F: drivers/media/i2c/mt9v111.c > > + > > MULTIFUNCTION DEVICES (MFD) > > M: Lee Jones <lee.jones@linaro.org> > > T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 341452f..0bd867d 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -841,6 +841,18 @@ config VIDEO_MT9V032 > > This is a Video4Linux2 sensor-level driver for the Micron > > MT9V032 752x480 CMOS sensor. > > > > +config VIDEO_MT9V111 > > + tristate "Aptina MT9V111 sensor support" > > + depends on I2C && VIDEO_V4L2 > > + depends on MEDIA_CAMERA_SUPPORT > > + depends on OF > > + help > > + This is a Video4Linux2 sensor-level driver for the Aptina/Micron > > + MT9V111 sensor. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called mt9v111. > > + > > config VIDEO_SR030PC30 > > tristate "Siliconfile SR030PC30 sensor support" > > depends on I2C && VIDEO_V4L2 > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > > index d679d57..f0e8618 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o > > obj-$(CONFIG_VIDEO_MT9T112) += mt9t112.o > > obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o > > obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o > > +obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o > > obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o > > obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o > > obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o > > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > > new file mode 100644 > > index 0000000..36e7424 > > --- /dev/null > > +++ b/drivers/media/i2c/mt9v111.c > > @@ -0,0 +1,1297 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * V4L2 sensor driver for Aptina MT9V111 image sensor > > + * Copyright (C) 2018 Jacopo Mondi <jacopo@jmondi.org> > > + * > > + * Based on mt9v032 driver > > + * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> > > + * > > + * Base on mt9v011 driver > > I realise it's probably a copy-paste from another driver and not your > text, but s/Base/Based/ > > > + * Copyright (c) 2009 Mauro Carvalho Chehab <mchehab@kernel.org> > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/of.h> > > +#include <linux/slab.h> > > +#include <linux/videodev2.h> > > +#include <linux/v4l2-mediabus.h> > > +#include <linux/module.h> > > + > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-device.h> > > +#include <media/v4l2-fwnode.h> > > +#include <media/v4l2-image-sizes.h> > > +#include <media/v4l2-subdev.h> > > + > > +/* > > + * MT9V111 is a 1/4-Inch CMOS digital image sensor with an integrated > > + * Image Flow Processing (IFP) engine and a sensor core loosely based on > > + * MT9V011. > > + * > > + * The IFP can produce several output image formats from the sensor core > > + * output, this driver currently supports only YUYV format permutations. > > s/output, this/output. This/ > > > + * > > + * As the auto exposure algorithms controls exposure time and modifies the > > + * frame rate, this driver disables auto exposure control, auto white balancing > > + * and auto flickering avoidance to allow manual frame rate control through > > + * s_frame_interval subdev operation or V4L2_CID_V/HBLANK controls. > > Should/Could this be an option perhaps (in the future) to allow someone > to accept that the framerate will be a bit 'variable' but allow > algorithms to run? > > > + * > > + * While it seems possible to instruct the auto-exposure control algorithm to > > + * respect a programmed frame rate when adjusting the pixel integration time, > > + * registers controlling this feature are not documented in the public > > + * available sensor manual used to develop this driver (09005aef80e90084, > > + * MT9V111_1.fm - Rev. G 1/05 EN). > > Aha - Ok - so the 'right' way is to fix the rate, but we don't have the > documentation to be able to do so. :( > That's my understanding, yes. I went with auto-exposure disabled by default, but it actually might be disabled if and only if frame rate is explicitely modified. It is not worth doing that in my opinion, but I should add a control to handle auto-exposure enabling/disabling probably. > > > + */ > > + > > +#define MT9V111_CHIP_ID_HIGH 0x82 > > +#define MT9V111_CHIP_ID_LOW 0x3a > > + > > +#define MT9V111_R01_ADDR_SPACE 0x01 > > +#define MT9V111_R01_IFP 0x01 > > Two identical values (0x01)... there's not a copy-paste error there is > there ? (I don't yet have the doc to check) > > Edit: I assume it's correct actually - as the first is the register > space, and then IFP is probably the first register in that space... It is correct, but the first is the register address (MT9V111_R01_ADDR_SPACE) the second one (and the third one here blow) are the register values to select the address space where to write to. > > > > +#define MT9V111_R01_CORE 0x04 > > + > > +#define MT9V111_IFP_R06_OPMODE_CTRL 0x06 > > +#define MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN BIT(1) > > +#define MT9V111_IFP_R06_OPMODE_CTRL_AE_EN BIT(14) > > +#define MT9V111_IFP_R07_IFP_RESET 0x07 > > +#define MT9V111_IFP_R07_IFP_RESET_MASK BIT(0) > > +#define MT9V111_IFP_R08_OUTFMT_CTRL 0x08 > > +#define MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER BIT(11) > > +#define MT9V111_IFP_R08_OUTFMT_CTRL_PCLK BIT(5) > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2 0x3a > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR BIT(0) > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC BIT(1) > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK GENMASK(2, 0) > > +#define MT9V111_IFP_RA5_HPAN 0xa5 > > +#define MT9V111_IFP_RA6_HZOOM 0xa6 > > +#define MT9V111_IFP_RA7_HOUT 0xa7 > > +#define MT9V111_IFP_RA8_VPAN 0xa8 > > +#define MT9V111_IFP_RA9_VZOOM 0xa9 > > +#define MT9V111_IFP_RAA_VOUT 0xaa > > +#define MT9V111_IFP_DECIMATION_MASK GENMASK(9, 0) > > +#define MT9V111_IFP_DECIMATION_FREEZE BIT(15) > > + > > +#define MT9V111_CORE_R03_WIN_HEIGHT 0x03 > > +#define MT9V111_CORE_R03_WIN_V_OFFS 2 > > +#define MT9V111_CORE_R04_WIN_WIDTH 0x04 > > +#define MT9V111_CORE_R04_WIN_H_OFFS 114 > > +#define MT9V111_CORE_R05_HBLANK 0x05 > > +#define MT9V111_CORE_R05_MIN_HBLANK 0x09 > > +#define MT9V111_CORE_R05_MAX_HBLANK GENMASK(9, 0) > > +#define MT9V111_CORE_R05_DEF_HBLANK 0x26 > > +#define MT9V111_CORE_R06_VBLANK 0x06 > > +#define MT9V111_CORE_R06_MIN_VBLANK 0x03 > > +#define MT9V111_CORE_R06_MAX_VBLANK GENMASK(11, 0) > > +#define MT9V111_CORE_R06_DEF_VBLANK 0x04 > > +#define MT9V111_CORE_R07_OUT_CTRL 0x07 > > +#define MT9V111_CORE_R07_OUT_CTRL_SAMPLE BIT(4) > > +#define MT9V111_CORE_R09_PIXEL_INT 0x09 > > +#define MT9V111_CORE_R09_PIXEL_INT_MASK GENMASK(11, 0) > > +#define MT9V111_CORE_R0D_CORE_RESET 0x0d > > +#define MT9V111_CORE_R0D_CORE_RESET_MASK BIT(0) > > +#define MT9V111_CORE_RFF_CHIP_VER 0xff > > + > > +#define MT9V111_PIXEL_ARRAY_WIDTH 640 > > +#define MT9V111_PIXEL_ARRAY_HEIGHT 480 > > + > > +#define MT9V111_MAX_CLKIN 27000000 > > + > > +struct mt9v111_dev { > > + struct device *dev; > > + struct i2c_client *client; > > + > > + /* Protects address space change operations. */ > > + spinlock_t addr_lock; > > + u8 addr_space; > > + > > + struct v4l2_subdev sd; > > +#if defined(CONFIG_MEDIA_CONTROLLER) > > I wonder if one day we'll have better integration so that we don't have > to be conditional on CONFIG_MEDIA_CONTROLLER. > > That said, would it be better to use?: > #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER) My understanding is that they work the same, but yes, IS_ENABLED is nicer. > > > > + struct media_pad pad; > > +#endif > > + > > + struct v4l2_ctrl *hblank; > > + struct v4l2_ctrl *vblank; > > + struct v4l2_ctrl_handler ctrls; > > + > > + /* Output image format and sizes. */ > > + struct v4l2_mbus_framefmt fmt; > > + unsigned int fps; > > + > > + /* Protects power up/down sequences. */ > > + struct mutex pwr_mutex; > > + int pwr_count; > > + > > + /* Protects stream on/off sequences. */ > > + struct mutex stream_mutex; > > + bool streaming; > > + > > + /* Flags to mark HW settings as not yet applied. */ > > + bool pending; > > + > > + /* Clock provider and system clock frequency. */ > > + struct clk *clk; > > + u32 sysclk; > > + > > + struct gpio_desc *oe; > > + struct gpio_desc *standby; > > + struct gpio_desc *reset; > > +}; > > + > > +#define sd_to_mt9v111(__sd) container_of(__sd, struct mt9v111_dev, sd) > > I believe it's recommended to bracket macro parameters: > #define sd_to_mt9v111(__sd) container_of((__sd), struct mt9v111_dev, sd) > Correct, I'll fix it. > > + > > +/* > > + * mt9v111_mbus_fmt - List all media bus formats supported by the driver. > > + * > > + * Only list the media bus code here. The image sizes are freely configurable > > + * in the pixel array sizes range. > > + * > > + * The frame desired frame interval, in the supported frame interval range, is > > "The frame desired frame interval,"? > > Did you mean "The desired frame interval,"? Yup, thanks for spotting it. > > > > + * obtained by configuring blanking as the sensor does not have a PLL but > > + * only a fixed clock divider that generates the output pixel clock. > > + */ > > +static struct mt9v111_mbus_fmt { > > + u32 code; > > +} mt9v111_formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > + }, > > +}; > > + > > +static u32 mt9v111_frame_intervals[] = {5, 10, 15, 20, 30}; > > + > > +/* > > + * mt9v111_frame_sizes - List sensor's supported resolutions. > > + * > > + * Resolution generated through decimation in the IFP block from the > > + * full VGA pixel array. > > + */ > > +static struct v4l2_rect mt9v111_frame_sizes[] = { > > + { > > + .width = 640, > > + .height = 480, > > + }, > > + { > > + .width = 352, > > + .height = 288 > > + }, > > + { > > + .width = 320, > > + .height = 240, > > + }, > > + { > > + .width = 176, > > + .height = 144, > > + }, > > + { > > + .width = 160, > > + .height = 120, > > + }, > > +}; > > + > > +/* Initial register configuration. */ > > +static struct mt9v111_reg_list { > > + u8 space; > > + u8 reg; > > + u16 mask; > > + u16 val; > > +} mt9v111_init_config[] = { > > + { > > + /* Disable AE and AWB to enable manual frame rate control. */ > > + .space = MT9V111_R01_IFP, > > + .reg = MT9V111_IFP_R06_OPMODE_CTRL, > > + .mask = MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN | > > + MT9V111_IFP_R06_OPMODE_CTRL_AE_EN, > > + .val = 0, > > + }, > > + { > > + /* Disable anti-flicker to enable manual frame rate control. */ > > + .space = MT9V111_R01_IFP, > > + .reg = MT9V111_IFP_R08_OUTFMT_CTRL, > > + .mask = MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER, > > + .val = 0, > > + }, > > + > > + /* TODO: AE and AWB manual configuration. */ > > +}; > > + > > +/* --- Device I/O access --- */ > > + > > +static int __mt9v111_read(struct i2c_client *c, u8 reg, u16 *val) > > +{ > > + struct i2c_msg msg[2]; > > + __be16 buf; > > + int ret; > > + > > + msg[0].addr = c->addr; > > + msg[0].flags = 0; > > + msg[0].len = 1; > > + msg[0].buf = ® > > + > > + msg[1].addr = c->addr; > > + msg[1].flags = I2C_M_RD; > > + msg[1].len = 2; > > + msg[1].buf = (char *)&buf; > > + > > + ret = i2c_transfer(c->adapter, msg, 2); > > + if (ret < 0) { > > + dev_err(&c->dev, "i2c read transfer error: %d\n", ret); > > + return ret; > > + } > > + > > + *val = be16_to_cpu(buf); > > + > > + dev_info(&c->dev, "%s: %x=%x\n", __func__, reg, *val); > > dev_info seems a bit verbose to be dumping all register writes ... > shouldn't this be dev_dbg? Oh look: #define KERN_INFO KERN_SOH "6" /* informational */ #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */ #define KERN_DEFAULT KERN_SOH "d" /* the default kernel loglevel */ I was sure it was the other way around (informational sound less urgent than debug to me :/ ) > > > + > > + return 0; > > +} > > + > > +static int __mt9v111_write(struct i2c_client *c, u8 reg, u16 val) > > +{ > > + struct i2c_msg msg; > > + u8 buf[3] = { 0 }; > > + int ret; > > + > > + buf[0] = reg; > > + buf[1] = val >> 8; > > + buf[2] = val & 0xff; > > + > > + msg.addr = c->addr; > > + msg.flags = 0; > > + msg.len = 3; > > + msg.buf = (char *)buf; > > + > > + dev_info(&c->dev, "%s: %x = %x%x\n", __func__, reg, buf[1], buf[2]); > > same here ... should this be dev_dbg? > > > > + > > + ret = i2c_transfer(c->adapter, &msg, 1); > > + if (ret < 0) { > > + dev_err(&c->dev, "i2c write transfer error: %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int __mt9v111_addr_space_select(struct i2c_client *c, u16 addr_space) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(c); > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + u16 val; > > + int ret; > > + > > + spin_lock(&mt9v111->addr_lock); > > + if (mt9v111->addr_space == addr_space) { > > + spin_unlock(&mt9v111->addr_lock); > > + return 0; > > + } > > + spin_unlock(&mt9v111->addr_lock); > > + > > + ret = __mt9v111_write(c, MT9V111_R01_ADDR_SPACE, addr_space); > > + if (ret) > > + return ret; > > + > > + /* Verify address space has been updated */ > > + ret = __mt9v111_read(c, MT9V111_R01_ADDR_SPACE, &val); > > + if (ret) > > + return ret; > > + > > + if (val != addr_space) > > + return -EINVAL; > > + > > + mt9v111->addr_space = addr_space; > > Should this write to the mt9v111->addr_space be locked, considering the > read at the beginning of the function is locked? > It should. Or either or the whole function should be mutex protected, or neither the single per-driver variable should be. Probably I should just drop the spinlock and that's it.. Concurrent read/writes on a driver with a single devnode shouldn't be an issue actually. > > + > > + return 0; > > +} > > + > > +static int mt9v111_read(struct i2c_client *c, u8 addr_space, u8 reg, u16 *val) > > +{ > > + int ret; > > + > > + /* Select register address space first. */ > > + ret = __mt9v111_addr_space_select(c, addr_space); > > + if (ret) > > + return ret; > > + > > + ret = __mt9v111_read(c, reg, val); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int mt9v111_write(struct i2c_client *c, u8 addr_space, u8 reg, u16 val) > > +{ > > + int ret; > > + > > + /* Select register address space first. */ > > + ret = __mt9v111_addr_space_select(c, addr_space); > > + if (ret) > > + return ret; > > + > > + ret = __mt9v111_write(c, reg, val); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int mt9v111_update(struct i2c_client *c, u8 addr_space, u8 reg, > > + u16 mask, u16 val) > > +{ > > + u16 current_val; > > + int ret; > > + > > + /* Select register address space first. */ > > + ret = __mt9v111_addr_space_select(c, addr_space); > > + if (ret) > > + return ret; > > + > > + /* Read the current register value, then update it. */ > > + ret = __mt9v111_read(c, reg, ¤t_val); > > + if (ret) > > + return ret; > > + > > + current_val &= ~mask; > > + current_val |= (val & mask); > > + ret = __mt9v111_write(c, reg, current_val); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int mt9v111_update_reglist(struct i2c_client *c, > > + struct mt9v111_reg_list *reg_list, > > + unsigned int n_regs) > > +{ > > + unsigned int i; > > + int ret; > > + > > + for (i = 0; i < n_regs; i++) { > > + struct mt9v111_reg_list *reg = ®_list[i]; > > + > > + ret = mt9v111_update(c, reg->space, reg->reg, reg->mask, > > + reg->val); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +/* --- Sensor HW operations --- */ > > + > > +static int __mt9v111_power_on(struct v4l2_subdev *sd) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + int ret; > > + > > + ret = clk_prepare_enable(mt9v111->clk); > > + if (ret) > > + return ret; > > + > > + clk_set_rate(mt9v111->clk, mt9v111->sysclk); > > + > > + gpiod_set_value(mt9v111->standby, 0); > > + usleep_range(500, 1000); > > + > > + gpiod_set_value(mt9v111->oe, 1); > > + usleep_range(500, 1000); > > + > > + return 0; > > +} > > + > > +static int __mt9v111_power_off(struct v4l2_subdev *sd) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + > > + gpiod_set_value(mt9v111->oe, 0); > > + usleep_range(500, 1000); > > + > > + gpiod_set_value(mt9v111->standby, 1); > > + usleep_range(500, 1000); > > + > > + clk_disable_unprepare(mt9v111->clk); > > + > > + return 0; > > +} > > + > > +static int __mt9v111_hw_reset(struct mt9v111_dev *mt9v111) > > +{ > > + if (!mt9v111->reset) > > + return -EINVAL; > > + > > + gpiod_set_value(mt9v111->reset, 1); > > + usleep_range(500, 1000); > > + > > + gpiod_set_value(mt9v111->reset, 0); > > + usleep_range(500, 1000); > > + > > + return 0; > > +} > > + > > +static int __mt9v111_sw_reset(struct mt9v111_dev *mt9v111) > > +{ > > + struct i2c_client *c = mt9v111->client; > > + unsigned int ret; > > But the return type of the function is not unsigned? More than that, the return value from mt9v111_update() is case of errors is one coming from i2c_transfer(), so it must be signed. Sorry, this was a trivial mistake. > > > + > > + /* Software reset core and IFP blocks. */ > > + > > + ret = mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R0D_CORE_RESET, > > + MT9V111_CORE_R0D_CORE_RESET_MASK, 1); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + ret = mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R0D_CORE_RESET, > > + MT9V111_CORE_R0D_CORE_RESET_MASK, 0); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + ret = mt9v111_update(c, MT9V111_R01_IFP, > > + MT9V111_IFP_R07_IFP_RESET, > > + MT9V111_IFP_R07_IFP_RESET_MASK, 1); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + ret = mt9v111_update(c, MT9V111_R01_IFP, > > + MT9V111_IFP_R07_IFP_RESET, > > + MT9V111_IFP_R07_IFP_RESET_MASK, 0); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + return 0; > > +} > > + > > +static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111, > > + struct v4l2_fract *tpf) > > +{ > > + unsigned int fps = tpf->numerator ? > > + tpf->denominator / tpf->numerator : > > + tpf->denominator; > > + unsigned int best_diff; > > + unsigned int frm_cols; > > + unsigned int row_pclk; > > + unsigned int best_fps; > > + unsigned int pclk; > > + unsigned int diff; > > + unsigned int idx; > > + unsigned int hb; > > + unsigned int vb; > > + unsigned int i; > > + int ret; > > + > > + /* Approximate to the closest supported frame interval. */ > > + best_diff = ~0L;> + for (i = 0, idx = 0; i < ARRAY_SIZE(mt9v111_frame_intervals); i++) { > > + diff = abs(fps - mt9v111_frame_intervals[i]); > > + if (diff < best_diff) {> + idx = i; > > + best_diff = diff; > > + } > > + } > > + fps = mt9v111_frame_intervals[idx]; > > + > > + /* > > + * The sensor does not provide a PLL circuitry and pixel clock is > > + * generated dividing by two the master clock source. > > "The sensor does not provide a PLL circuitry and the pixel clock is > generated by dividing the master clock source by two." ? > One day I'll get english language syntax properly... > > + * > > + * Trow = (W + Hblank + 114) * 2 * (1 / SYSCLK) > > + * TFrame = Trow * (H + Vblank + 2) > > + * > > + * FPS = (SYSCLK / 2) / (Trow * (H + Vblank + 2)) > > + * > > + * This boils down to tune H and V blanks to best approximate the > > + * above equation. > > + * > > + * Test all available H/V blank values, until we reach the > > + * desired frame rate. > > + */ > > + best_fps = vb = hb = 0; > > + pclk = DIV_ROUND_CLOSEST(mt9v111->sysclk, 2); > > + row_pclk = MT9V111_PIXEL_ARRAY_WIDTH + 7 + MT9V111_CORE_R04_WIN_H_OFFS; > > + frm_cols = MT9V111_PIXEL_ARRAY_HEIGHT + 7 + MT9V111_CORE_R03_WIN_V_OFFS; > > + > > + best_diff = ~0L; > > + for (vb = MT9V111_CORE_R06_MIN_VBLANK; > > + vb < MT9V111_CORE_R06_MAX_VBLANK; vb++) { > > + for (hb = MT9V111_CORE_R05_MIN_HBLANK; > > + hb < MT9V111_CORE_R05_MAX_HBLANK; hb += 10) { > > + unsigned int t_frame = (row_pclk + hb) * > > + (frm_cols + vb); > > + unsigned int t_fps = DIV_ROUND_CLOSEST(pclk, t_frame); > > + > > + diff = abs(fps - t_fps); > > + if (diff < best_diff) { > > + best_diff = diff; > > + best_fps = t_fps; > > + > > + if (diff == 0) > > + break; > > + } > > + } > > + > > + if (diff == 0) > > + break; > > + } > > + > > + dev_info(mt9v111->dev, "FPS: %u, hb = %u, vb = %u\n", fps, hb, vb); > > + > > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->hblank, hb); > > + if (ret) > > + return ret; > > + > > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->vblank, vb); > > + if (ret) > > + return ret; > > + > > + tpf->numerator = 1; > > + tpf->denominator = best_fps; > > + > > + return 0; > > +} > > + > > +static int mt9v111_hw_config(struct mt9v111_dev *mt9v111) > > +{ > > + struct i2c_client *c = mt9v111->client; > > + unsigned int ret; > > + u16 outfmtctrl2; > > + > > + /* Force device reset. */ > > + ret = __mt9v111_hw_reset(mt9v111); > > + if (ret == -EINVAL) > > + ret = __mt9v111_sw_reset(mt9v111); > > + if (ret) > > + return ret; > > + > > + /* > > + * Disable AE, AWB and anti-flickering to allow manual frame rate > > + * control. > > + */ > > + ret = mt9v111_update_reglist(c, mt9v111_init_config, > > + ARRAY_SIZE(mt9v111_init_config)); > > + if (ret) > > + return ret; > > + > > + /* Configure internal clock sample rate. */ > > + ret = mt9v111->sysclk < DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? > > + mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R07_OUT_CTRL, > > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 1) : > > + mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R07_OUT_CTRL, > > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 0); > > + if (ret) > > + return ret; > > + > > + /* > > + * Configure output image format components ordering. > > + * > > + * TODO: IFP block can also output several RGB permutations, we only > > + * support YUYV permutations at the moment. > > + */ > > + switch (mt9v111->fmt.code) { > > + case MEDIA_BUS_FMT_YUYV8_2X8: > > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC; > > + break; > > + case MEDIA_BUS_FMT_VYUY8_2X8: > > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > > + break; > > + case MEDIA_BUS_FMT_YVYU8_2X8: > > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC | > > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > > + break; > > + case MEDIA_BUS_FMT_UYVY8_2X8: > > + default: > > + outfmtctrl2 = 0; > > + break; > > + } > > + > > + ret = mt9v111_update(c, MT9V111_R01_IFP, MT9V111_IFP_R3A_OUTFMT_CTRL2, > > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK, > > + outfmtctrl2); > > + if (ret) > > + return ret; > > + > > + /* > > + * Do not change default sensor's core configuration: > > + * output the whole 640x480 pixel array, skip 18 columns and 6 rows. > > I'm not sure I understand why we skip 18 columns and 6 rows? > (or does it not matter, and it's just necessary) Sensor usually a variable number of columns and rows which are blank or non active and are used for eg. black level calibration. > > > + * > > + * Instead, control the output image size through IFP block. > > + * > > + * TODO: No zoom&pan support. Currently we control the output image > > + * size only through decimation, with no zoom support. > > + */ > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA5_HPAN, > > + MT9V111_IFP_DECIMATION_FREEZE); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA8_VPAN, > > + MT9V111_IFP_DECIMATION_FREEZE); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA6_HZOOM, > > + MT9V111_IFP_DECIMATION_FREEZE | > > + MT9V111_PIXEL_ARRAY_WIDTH); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA9_VZOOM, > > + MT9V111_IFP_DECIMATION_FREEZE | > > + MT9V111_PIXEL_ARRAY_HEIGHT); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA7_HOUT, > > + MT9V111_IFP_DECIMATION_FREEZE | > > + mt9v111->fmt.width); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RAA_VOUT, > > + mt9v111->fmt.height); > > + if (ret) > > + return ret; > > + > > + /* Update blankings to match the programmed frame rate. */ > > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R05_HBLANK, > > + MT9V111_CORE_R05_MAX_HBLANK, > > + mt9v111->hblank->val); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R06_VBLANK, > > + MT9V111_CORE_R06_MAX_VBLANK, > > + mt9v111->vblank->val); > > + if (ret) > > + return ret; > > + > > + /* > > + * Set pixel integration time to the whole frame time. > > + * This value controls the the shutter delay when running with AE > > + * disabled. If longer than frame time, it affects the output > > + * frame rate. > > + */ > > + ret = mt9v111_write(c, MT9V111_R01_CORE, MT9V111_CORE_R09_PIXEL_INT, > > + MT9V111_PIXEL_ARRAY_HEIGHT); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +/* --- V4L2 subdev operations --- */ > > + > > +static int mt9v111_s_power(struct v4l2_subdev *sd, int on) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + int pwr_count; > > + int ret = 0; > > + > > + mutex_lock(&mt9v111->pwr_mutex); > > + > > + /* > > + * Make sure we're transitioning from 0 to 1, or viceversa, only > > + * before actually changing the power state. > > + */ > > + pwr_count = mt9v111->pwr_count; > > + pwr_count += on ? 1 : -1; > > + if (pwr_count == !!on) { > > + ret = on ? __mt9v111_power_on(sd) : > > + __mt9v111_power_off(sd); > > + if (!ret) > > + /* All went well, updated power counter. */ > > + mt9v111->pwr_count = pwr_count; > > + > > + mutex_unlock(&mt9v111->pwr_mutex); > > + > > + return ret; > > + } > > + > > + /* > > + * Update power counter to keep track of how many nested calls we > > + * received. > > + */ > > + WARN_ON(pwr_count < 0 || pwr_count > 1); > > + mt9v111->pwr_count = pwr_count; > > + > > + mutex_unlock(&mt9v111->pwr_mutex); > > + > > + return ret; > > +} > > + > > +static int mt9v111_s_stream(struct v4l2_subdev *subdev, int enable) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > > + int ret; > > + > > + mutex_lock(&mt9v111->stream_mutex); > > + > > + if (mt9v111->streaming == enable) { > > + mutex_unlock(&mt9v111->stream_mutex); > > + return 0; > > + } > > + > > + ret = mt9v111_s_power(subdev, enable); > > + if (ret) > > + goto error_unlock; > > + > > + if (enable && mt9v111->pending) { > > + ret = mt9v111_hw_config(mt9v111); > > + if (ret) > > + goto error_unlock; > > + > > + /* > > + * No need to update control here as far as only H/VBLANK are > > + * supported and immediately programmed to registers in .s_ctrl > > + */ > > + > > + mt9v111->pending = false; > > + } > > + > > + mt9v111->streaming = enable ? true : false; > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + return 0; > > + > > +error_unlock: > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + return ret; > > +} > > + > > +static int mt9v111_s_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_frame_interval *ival) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + struct v4l2_fract *tpf = &ival->interval; > > + unsigned int fps = tpf->numerator ? > > + tpf->denominator / tpf->numerator : > > + tpf->denominator; > > + unsigned int max_fps; > > + > > + mutex_lock(&mt9v111->stream_mutex); > > + if (mt9v111->streaming) { > > + mutex_unlock(&mt9v111->stream_mutex); > > + return -EBUSY; > > + } > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + if (mt9v111->fps == fps) > > + return 0; > > + > > + /* Make sure frame rate/image sizes constraints are respected. */ > > + if (mt9v111->fmt.width < QVGA_WIDTH && > > + mt9v111->fmt.height < QVGA_HEIGHT) > > + max_fps = 90; > > + else if (mt9v111->fmt.width < CIF_WIDTH && > > + mt9v111->fmt.height < CIF_HEIGHT) > > + max_fps = 60; > > + else > > + max_fps = mt9v111->sysclk < > > + DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? 15 : > > + 30; > > + > > + if (fps > max_fps) > > + return -EINVAL; > > + > > + mt9v111_calc_frame_rate(mt9v111, tpf); > > + > > + mt9v111->fps = fps; > > + mt9v111->pending = true; > > + > > + return 0; > > +} > > + > > +static int mt9v111_g_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_frame_interval *ival) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + struct v4l2_fract *tpf = &ival->interval; > > + > > + tpf->numerator = 1; > > + tpf->denominator = mt9v111->fps; > > + > > + return 0; > > +} > > + > > +static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format( > > + struct mt9v111_dev *mt9v111, > > + struct v4l2_subdev_pad_config *cfg, > > + unsigned int pad, > > + enum v4l2_subdev_format_whence which) > > +{ > > + switch (which) { > > + case V4L2_SUBDEV_FORMAT_TRY: > > +#ifdef CONFIG_MEDIA_CONTROLLER > > #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? > > > + return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad); > > +#else > > + return &cfg->try_fmt; > > +#endif > > + case V4L2_SUBDEV_FORMAT_ACTIVE: > > + return &mt9v111->fmt; > > + default: > > + return NULL; > > + } > > +} > > + > > +static int mt9v111_enum_mbus_code(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + if (code->pad || code->index > ARRAY_SIZE(mt9v111_formats) - 1) > > + return -EINVAL; > > + > > + code->code = mt9v111_formats[code->index].code; > > + > > + return 0; > > +} > > + > > +static int mt9v111_enum_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_interval_enum *fie) > > +{ > > + unsigned int i; > > + > > + if (fie->pad || fie->index >= ARRAY_SIZE(mt9v111_frame_intervals)) > > + return -EINVAL; > > + > > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) > > + if (fie->width == mt9v111_frame_sizes[i].width && > > + fie->height == mt9v111_frame_sizes[i].height) > > + break; > > + > > + if (i == ARRAY_SIZE(mt9v111_frame_sizes)) > > + return -EINVAL; > > + > > + fie->interval.numerator = 1; > > + fie->interval.denominator = mt9v111_frame_intervals[fie->index]; > > + > > + return 0; > > +} > > + > > +static int mt9v111_enum_frame_size(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + if (fse->pad || fse->index > ARRAY_SIZE(mt9v111_frame_sizes)) > > + return -EINVAL; > > + > > + fse->min_width = mt9v111_frame_sizes[fse->index].width; > > + fse->max_width = mt9v111_frame_sizes[fse->index].width; > > + fse->min_height = mt9v111_frame_sizes[fse->index].height; > > + fse->max_height = mt9v111_frame_sizes[fse->index].height; > > + > > + return 0; > > +} > > + > > +static int mt9v111_get_format(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *format) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > > + > > + if (format->pad) > > + return -EINVAL; > > + > > + format->format = *__mt9v111_get_pad_format(mt9v111, cfg, format->pad, > > + format->which); > > + return 0; > > +} > > + > > +static int mt9v111_set_format(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *format) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > > + struct v4l2_mbus_framefmt new_fmt; > > + struct v4l2_mbus_framefmt *__fmt; > > + unsigned int best_fit = ~0L; > > + unsigned int idx; > > + unsigned int i; > > + > > + mutex_lock(&mt9v111->stream_mutex); > > + if (mt9v111->streaming) { > > + mutex_unlock(&mt9v111->stream_mutex); > > + return -EBUSY; > > + } > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + if (format->pad) > > + return -EINVAL; > > + > > + /* Update mbus format code and sizes. */ > > + for (i = 0; i < ARRAY_SIZE(mt9v111_formats); i++) { > > + if (format->format.code == mt9v111_formats[i].code) { > > + new_fmt.code = mt9v111_formats[i].code; > > + break; > > + } > > + } > > + if (i == ARRAY_SIZE(mt9v111_formats)) > > + new_fmt.code = mt9v111_formats[0].code; > > + > > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) { > > + unsigned int fit = abs(mt9v111_frame_sizes[i].width - > > + format->format.width) + > > + abs(mt9v111_frame_sizes[i].height - > > + format->format.height); > > + if (fit < best_fit) { > > + best_fit = fit; > > + idx = i; > > + > > + if (fit == 0) > > + break; > > + } > > + } > > + new_fmt.width = mt9v111_frame_sizes[idx].width; > > + new_fmt.height = mt9v111_frame_sizes[idx].height; > > + > > + /* Update the device (or pad) format if it has changed. */ > > + __fmt = __mt9v111_get_pad_format(mt9v111, cfg, format->pad, > > + format->which); > > + > > + /* Format hasn't changed, stop here. */ > > + if (__fmt->code == new_fmt.code && > > + __fmt->width == new_fmt.width && > > + __fmt->height == new_fmt.height) > > + goto done; > > + > > + /* Update the format and sizes, then mark changes as pending. */ > > + __fmt->code = new_fmt.code; > > + __fmt->width = new_fmt.width; > > + __fmt->height = new_fmt.height; > > + > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > + mt9v111->pending = true; > > + > > + dev_info(mt9v111->dev, "%s: mbus_code: %x - (%ux%u)\n", > > + __func__, __fmt->code, __fmt->width, __fmt->height); > > + > > +done: > > + format->format = *__fmt; > > + > > + return 0; > > +} > > + > > +static const struct v4l2_subdev_core_ops mt9v111_core_ops = { > > + .s_power = mt9v111_s_power, > > +}; > > + > > +static const struct v4l2_subdev_video_ops mt9v111_video_ops = { > > + .s_stream = mt9v111_s_stream, > > + .s_frame_interval = mt9v111_s_frame_interval, > > + .g_frame_interval = mt9v111_g_frame_interval, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops mt9v111_pad_ops = { > > + .enum_mbus_code = mt9v111_enum_mbus_code, > > + .enum_frame_size = mt9v111_enum_frame_size, > > + .enum_frame_interval = mt9v111_enum_frame_interval, > > + .get_fmt = mt9v111_get_format, > > + .set_fmt = mt9v111_set_format, > > +}; > > + > > +static const struct v4l2_subdev_ops mt9v111_ops = { > > + .core = &mt9v111_core_ops, > > + .video = &mt9v111_video_ops, > > + .pad = &mt9v111_pad_ops, > > +}; > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? > > > +static const struct media_entity_operations mt9v111_subdev_entity_ops = { > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > +static int mt9v111_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + struct v4l2_mbus_framefmt *__fmt = > > + v4l2_subdev_get_try_format(sd, fh->pad, 0); > > + > > + /* Initialize try format: copy the device format. */ > > + *__fmt = mt9v111->fmt; > > + > > + return 0; > > +} > > + > > +static const struct v4l2_subdev_internal_ops mt9v111_internal_ops = { > > + .open = mt9v111_open, > > +}; > > +#endif > > + > > +/* --- V4L2 ctrl --- */ > > +static int mt9v111_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct mt9v111_dev *mt9v111 = container_of(ctrl->handler, > > + struct mt9v111_dev, > > + ctrls); > > + int ret = 0; > > + > > + mutex_lock(&mt9v111->pwr_mutex); > > + /* > > + * If sensor is powered down, just cache new control values, > > + * no actual register access. > > + */ > > + if (!mt9v111->pwr_count) { > > + mutex_unlock(&mt9v111->pwr_mutex); > > + return 0; > > + } > > + mutex_unlock(&mt9v111->pwr_mutex); > > + > > + switch (ctrl->id) { > > + case V4L2_CID_HBLANK: > > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > > + MT9V111_CORE_R05_HBLANK, > > + MT9V111_CORE_R05_MAX_HBLANK, > > + mt9v111->hblank->val); > > + break; > > + case V4L2_CID_VBLANK: > > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > > + MT9V111_CORE_R06_VBLANK, > > + MT9V111_CORE_R06_MAX_VBLANK, > > + mt9v111->vblank->val); > > + break; > > + } > > + > > + return ret; > > +} > > + > > +static const struct v4l2_ctrl_ops mt9v111_ctrl_ops = { > > + .s_ctrl = mt9v111_s_ctrl, > > +}; > > + > > +static int mt9v111_chip_probe(struct mt9v111_dev *mt9v111) > > +{ > > + int ret; > > + u16 val; > > + > > + ret = __mt9v111_power_on(&mt9v111->sd); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_read(mt9v111->client, MT9V111_R01_CORE, > > + MT9V111_CORE_RFF_CHIP_VER, &val); > > + if (ret) > > + return ret; > > + > > + if ((val >> 8) != MT9V111_CHIP_ID_HIGH && > > + (val & 0xff) != MT9V111_CHIP_ID_LOW) { > > + dev_err(mt9v111->dev, > > + "Unable to identify MT9V111 chip: 0x%2x%2x\n", > > + val >> 8, val & 0xff); > > + return ret; > > + } > > + > > + ret = __mt9v111_power_off(&mt9v111->sd); > > + if (ret) > > + return ret; > > + > > + dev_info(mt9v111->dev, "Chip identified: 0x%2x%2x\n", > > + val >> 8, val & 0xff); > > + > > + return 0; > > +} > > + > > +static int mt9v111_probe(struct i2c_client *client) > > +{ > > + struct mt9v111_dev *mt9v111; > > + struct v4l2_fract tpf; > > + int ret; > > + > > + mt9v111 = devm_kzalloc(&client->dev, sizeof(*mt9v111), GFP_KERNEL); > > + if (!mt9v111) > > + return -ENOMEM; > > + > > + mt9v111->dev = &client->dev; > > + mt9v111->client = client; > > + > > + mt9v111->clk = devm_clk_get(&client->dev, NULL); > > + if (IS_ERR(mt9v111->clk)) > > + return PTR_ERR(mt9v111->clk); > > + > > + mt9v111->sysclk = clk_get_rate(mt9v111->clk); > > + if (mt9v111->sysclk > MT9V111_MAX_CLKIN) > > + return -EINVAL; > > + > > + mt9v111->oe = devm_gpiod_get_optional(&client->dev, "enable", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(mt9v111->oe)) { > > + dev_err(&client->dev, "Unable to get GPIO \"enable\": %ld\n", > > + PTR_ERR(mt9v111->oe)); > > + return PTR_ERR(mt9v111->oe); > > + } > > + > > + mt9v111->standby = devm_gpiod_get_optional(&client->dev, "standby", > > + GPIOD_OUT_HIGH); > > + if (IS_ERR(mt9v111->standby)) { > > + dev_err(&client->dev, "Unable to get GPIO \"standby\": %ld\n", > > + PTR_ERR(mt9v111->standby)); > > + return PTR_ERR(mt9v111->standby); > > + } > > + > > + mt9v111->reset = devm_gpiod_get_optional(&client->dev, "reset", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(mt9v111->reset)) { > > + dev_err(&client->dev, "Unable to get GPIO \"reset\": %ld\n", > > + PTR_ERR(mt9v111->reset)); > > + return PTR_ERR(mt9v111->reset); > > + } > > + > > + mutex_init(&mt9v111->pwr_mutex); > > + mutex_init(&mt9v111->stream_mutex); > > + spin_lock_init(&mt9v111->addr_lock); > > + > > + v4l2_ctrl_handler_init(&mt9v111->ctrls, 3); > > + > > + mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > > + V4L2_CID_HBLANK, > > + MT9V111_CORE_R05_MIN_HBLANK, > > + MT9V111_CORE_R05_MAX_HBLANK, 1, > > + MT9V111_CORE_R05_DEF_HBLANK); > > + if (IS_ERR_OR_NULL(mt9v111->hblank)) { > > + ret = PTR_ERR(mt9v111->hblank); > > + goto error_free_ctrls; > > + } > > + > > + mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > > + V4L2_CID_VBLANK, > > + MT9V111_CORE_R06_MIN_VBLANK, > > + MT9V111_CORE_R06_MAX_VBLANK, 1, > > + MT9V111_CORE_R06_DEF_VBLANK); > > + if (IS_ERR_OR_NULL(mt9v111->vblank)) { > > + ret = PTR_ERR(mt9v111->vblank); > > + goto error_free_ctrls; > > + } > > + > > + /* PIXEL_RATE is fixed: just expose it to user space. */ > > + v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > > + V4L2_CID_PIXEL_RATE, 0, > > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2), 1, > > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2)); > > + > > + mt9v111->sd.ctrl_handler = &mt9v111->ctrls; > > + > > + /* Start with default configuration: 640x480 UYVY. */ > > + mt9v111->fmt.width = 640; > > + mt9v111->fmt.height = 480; > > + mt9v111->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; > > + > > + /* These are fixed for all supported formats. */ > > + mt9v111->fmt.field = V4L2_FIELD_NONE; > > + mt9v111->fmt.colorspace = V4L2_COLORSPACE_SRGB; > > + mt9v111->fmt.ycbcr_enc = V4L2_YCBCR_ENC_601; > > + mt9v111->fmt.quantization = V4L2_QUANTIZATION_LIM_RANGE; > > + mt9v111->fmt.xfer_func = V4L2_XFER_FUNC_SRGB; > > + > > + /* Re-calculate blankings for 640x480@15fps. */ > > + mt9v111->fps = 15; > > + tpf.numerator = 1; > > + tpf.denominator = mt9v111->fps; > > + ret = mt9v111_calc_frame_rate(mt9v111, &tpf); > > + if (ret) > > + goto error_free_ctrls; > > + > > + mt9v111->pwr_count = 0; > > + mt9v111->addr_space = MT9V111_R01_IFP; > > + mt9v111->pending = true; > > + > > + v4l2_i2c_subdev_init(&mt9v111->sd, client, &mt9v111_ops); > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? > > > + mt9v111->sd.internal_ops = &mt9v111_internal_ops; > > + mt9v111->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + mt9v111->sd.entity.ops = &mt9v111_subdev_entity_ops; > > + mt9v111->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + > > + mt9v111->pad.flags = MEDIA_PAD_FL_SOURCE; > > + ret = media_entity_pads_init(&mt9v111->sd.entity, 1, &mt9v111->pad); > > + if (ret) > > + goto error_free_ctrls; > > +#endif > > + > > + ret = mt9v111_chip_probe(mt9v111); > > + if (ret) > > + goto error_free_ctrls; > > + > > + ret = v4l2_async_register_subdev(&mt9v111->sd); > > + if (ret) > > + goto error_free_ctrls; > > + > > + return 0; > > + > > +error_free_ctrls: > > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + media_entity_cleanup(&mt9v111->sd.entity); > > +#endif > > > media_entity_cleanup() has #if IS_ENABLED(MC) barriers around it with a > null implementation so we could drop the #ifdef CONFIG_ here ... but > perhaps while we have to have a non-MC supported implementation it keeps > the distinction clear... > > Otherwise, > #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)? Ah, I see. Actually I would prefer to keep it explicit since it makes the code clearer. Thanks for your comments j > > > > + > > + mutex_destroy(&mt9v111->pwr_mutex); > > + mutex_destroy(&mt9v111->stream_mutex); > > + > > + return ret; > > +} > > + > > +static int mt9v111_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + > > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + media_entity_cleanup(&sd->entity); > > +#endif > > and here... > > > + > > + mutex_destroy(&mt9v111->pwr_mutex); > > + mutex_destroy(&mt9v111->stream_mutex); > > + > > + devm_gpiod_put(mt9v111->dev, mt9v111->oe); > > + devm_gpiod_put(mt9v111->dev, mt9v111->standby); > > + devm_gpiod_put(mt9v111->dev, mt9v111->reset); > > + > > + devm_clk_put(mt9v111->dev, mt9v111->clk); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id mt9v111_of_match[] = { > > + { .compatible = "aptina,mt9v111", }, > > + { /* sentinel */ }, > > +}; > > + > > +static struct i2c_driver mt9v111_driver = { > > + .driver = { > > + .name = "mt9v111", > > + .of_match_table = of_match_ptr(mt9v111_of_match), > > + }, > > + .probe_new = mt9v111_probe, > > + .remove = mt9v111_remove, > > +}; > > + > > +module_i2c_driver(mt9v111_driver); > > + > > +MODULE_DESCRIPTION("V4L2 sensor driver for Aptina MT9V111"); > > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>"); > > +MODULE_LICENSE("GPL v2"); > > > -- > Regards > -- > Kieran [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 2018-06-11 15:17 ` [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 Jacopo Mondi 2018-06-11 16:44 ` kbuild test robot 2018-07-17 9:48 ` Kieran Bingham @ 2018-07-23 14:28 ` Sakari Ailus 2018-07-24 13:08 ` jacopo mondi 2 siblings, 1 reply; 12+ messages in thread From: Sakari Ailus @ 2018-07-23 14:28 UTC (permalink / raw) To: Jacopo Mondi; +Cc: mchehab, linux-media, linux-kernel, linux-renesas-soc Hi Jacopo, Apologies for the late review. Please see my comments below. On Mon, Jun 11, 2018 at 05:17:33PM +0200, Jacopo Mondi wrote: > Add V4L2 sensor driver for Aptina MT9V111 CMOS image sensor. > > The MT9V111 is a 1/4-Inch CMOS image sensor based on MT9V011 with an > integrated Image Flow Processor. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > MAINTAINERS | 8 + Could you move the MAINTAINERS change to the DT binding documentation patch? Currently I presume adding a binding file which isn't accounted for produces a checkpatch.pl warning. > drivers/media/i2c/Kconfig | 12 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/mt9v111.c | 1297 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1318 insertions(+) > create mode 100644 drivers/media/i2c/mt9v111.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a38e24a..2c2fe60 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9523,6 +9523,14 @@ F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt > F: drivers/media/i2c/mt9v032.c > F: include/media/i2c/mt9v032.h > > +MT9V111 APTINA CAMERA SENSOR > +M: Jacopo Mondi <jacopo@jmondi.org> > +L: linux-media@vger.kernel.org > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > +F: drivers/media/i2c/mt9v111.c > + > MULTIFUNCTION DEVICES (MFD) > M: Lee Jones <lee.jones@linaro.org> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 341452f..0bd867d 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -841,6 +841,18 @@ config VIDEO_MT9V032 > This is a Video4Linux2 sensor-level driver for the Micron > MT9V032 752x480 CMOS sensor. > > +config VIDEO_MT9V111 > + tristate "Aptina MT9V111 sensor support" > + depends on I2C && VIDEO_V4L2 > + depends on MEDIA_CAMERA_SUPPORT > + depends on OF Why OF? I see no effective OF dependencies in the driver. > + help > + This is a Video4Linux2 sensor-level driver for the Aptina/Micron > + MT9V111 sensor. > + > + To compile this driver as a module, choose M here: the > + module will be called mt9v111. > + > config VIDEO_SR030PC30 > tristate "Siliconfile SR030PC30 sensor support" > depends on I2C && VIDEO_V4L2 > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index d679d57..f0e8618 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o > obj-$(CONFIG_VIDEO_MT9T112) += mt9t112.o > obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o > obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o > +obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o > obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o > obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o > obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > new file mode 100644 > index 0000000..36e7424 > --- /dev/null > +++ b/drivers/media/i2c/mt9v111.c > @@ -0,0 +1,1297 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * V4L2 sensor driver for Aptina MT9V111 image sensor > + * Copyright (C) 2018 Jacopo Mondi <jacopo@jmondi.org> > + * > + * Based on mt9v032 driver > + * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> > + * > + * Base on mt9v011 driver > + * Copyright (c) 2009 Mauro Carvalho Chehab <mchehab@kernel.org> > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +#include <linux/videodev2.h> > +#include <linux/v4l2-mediabus.h> > +#include <linux/module.h> > + > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-fwnode.h> > +#include <media/v4l2-image-sizes.h> > +#include <media/v4l2-subdev.h> > + > +/* > + * MT9V111 is a 1/4-Inch CMOS digital image sensor with an integrated > + * Image Flow Processing (IFP) engine and a sensor core loosely based on > + * MT9V011. > + * > + * The IFP can produce several output image formats from the sensor core > + * output, this driver currently supports only YUYV format permutations. > + * > + * As the auto exposure algorithms controls exposure time and modifies the > + * frame rate, this driver disables auto exposure control, auto white balancing > + * and auto flickering avoidance to allow manual frame rate control through > + * s_frame_interval subdev operation or V4L2_CID_V/HBLANK controls. > + * > + * While it seems possible to instruct the auto-exposure control algorithm to > + * respect a programmed frame rate when adjusting the pixel integration time, > + * registers controlling this feature are not documented in the public > + * available sensor manual used to develop this driver (09005aef80e90084, > + * MT9V111_1.fm - Rev. G 1/05 EN). > + */ > + > +#define MT9V111_CHIP_ID_HIGH 0x82 > +#define MT9V111_CHIP_ID_LOW 0x3a > + > +#define MT9V111_R01_ADDR_SPACE 0x01 > +#define MT9V111_R01_IFP 0x01 > +#define MT9V111_R01_CORE 0x04 > + > +#define MT9V111_IFP_R06_OPMODE_CTRL 0x06 > +#define MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN BIT(1) > +#define MT9V111_IFP_R06_OPMODE_CTRL_AE_EN BIT(14) > +#define MT9V111_IFP_R07_IFP_RESET 0x07 > +#define MT9V111_IFP_R07_IFP_RESET_MASK BIT(0) > +#define MT9V111_IFP_R08_OUTFMT_CTRL 0x08 > +#define MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER BIT(11) > +#define MT9V111_IFP_R08_OUTFMT_CTRL_PCLK BIT(5) > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2 0x3a > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR BIT(0) > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC BIT(1) > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK GENMASK(2, 0) > +#define MT9V111_IFP_RA5_HPAN 0xa5 > +#define MT9V111_IFP_RA6_HZOOM 0xa6 > +#define MT9V111_IFP_RA7_HOUT 0xa7 > +#define MT9V111_IFP_RA8_VPAN 0xa8 > +#define MT9V111_IFP_RA9_VZOOM 0xa9 > +#define MT9V111_IFP_RAA_VOUT 0xaa > +#define MT9V111_IFP_DECIMATION_MASK GENMASK(9, 0) > +#define MT9V111_IFP_DECIMATION_FREEZE BIT(15) > + > +#define MT9V111_CORE_R03_WIN_HEIGHT 0x03 > +#define MT9V111_CORE_R03_WIN_V_OFFS 2 > +#define MT9V111_CORE_R04_WIN_WIDTH 0x04 > +#define MT9V111_CORE_R04_WIN_H_OFFS 114 > +#define MT9V111_CORE_R05_HBLANK 0x05 > +#define MT9V111_CORE_R05_MIN_HBLANK 0x09 > +#define MT9V111_CORE_R05_MAX_HBLANK GENMASK(9, 0) > +#define MT9V111_CORE_R05_DEF_HBLANK 0x26 > +#define MT9V111_CORE_R06_VBLANK 0x06 > +#define MT9V111_CORE_R06_MIN_VBLANK 0x03 > +#define MT9V111_CORE_R06_MAX_VBLANK GENMASK(11, 0) > +#define MT9V111_CORE_R06_DEF_VBLANK 0x04 > +#define MT9V111_CORE_R07_OUT_CTRL 0x07 > +#define MT9V111_CORE_R07_OUT_CTRL_SAMPLE BIT(4) > +#define MT9V111_CORE_R09_PIXEL_INT 0x09 > +#define MT9V111_CORE_R09_PIXEL_INT_MASK GENMASK(11, 0) > +#define MT9V111_CORE_R0D_CORE_RESET 0x0d > +#define MT9V111_CORE_R0D_CORE_RESET_MASK BIT(0) > +#define MT9V111_CORE_RFF_CHIP_VER 0xff > + > +#define MT9V111_PIXEL_ARRAY_WIDTH 640 > +#define MT9V111_PIXEL_ARRAY_HEIGHT 480 > + > +#define MT9V111_MAX_CLKIN 27000000 > + > +struct mt9v111_dev { > + struct device *dev; > + struct i2c_client *client; > + > + /* Protects address space change operations. */ > + spinlock_t addr_lock; > + u8 addr_space; > + > + struct v4l2_subdev sd; > +#if defined(CONFIG_MEDIA_CONTROLLER) > + struct media_pad pad; > +#endif > + > + struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *vblank; > + struct v4l2_ctrl_handler ctrls; > + > + /* Output image format and sizes. */ > + struct v4l2_mbus_framefmt fmt; > + unsigned int fps; > + > + /* Protects power up/down sequences. */ > + struct mutex pwr_mutex; > + int pwr_count; > + > + /* Protects stream on/off sequences. */ > + struct mutex stream_mutex; > + bool streaming; > + > + /* Flags to mark HW settings as not yet applied. */ > + bool pending; > + > + /* Clock provider and system clock frequency. */ > + struct clk *clk; > + u32 sysclk; > + > + struct gpio_desc *oe; > + struct gpio_desc *standby; > + struct gpio_desc *reset; > +}; > + > +#define sd_to_mt9v111(__sd) container_of(__sd, struct mt9v111_dev, sd) > + > +/* > + * mt9v111_mbus_fmt - List all media bus formats supported by the driver. > + * > + * Only list the media bus code here. The image sizes are freely configurable > + * in the pixel array sizes range. > + * > + * The frame desired frame interval, in the supported frame interval range, is > + * obtained by configuring blanking as the sensor does not have a PLL but > + * only a fixed clock divider that generates the output pixel clock. > + */ > +static struct mt9v111_mbus_fmt { > + u32 code; > +} mt9v111_formats[] = { > + { > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > + }, > + { > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > + }, > + { > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > + }, > + { > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > + }, > +}; > + > +static u32 mt9v111_frame_intervals[] = {5, 10, 15, 20, 30}; > + > +/* > + * mt9v111_frame_sizes - List sensor's supported resolutions. > + * > + * Resolution generated through decimation in the IFP block from the > + * full VGA pixel array. > + */ > +static struct v4l2_rect mt9v111_frame_sizes[] = { > + { > + .width = 640, > + .height = 480, > + }, > + { > + .width = 352, > + .height = 288 > + }, > + { > + .width = 320, > + .height = 240, > + }, > + { > + .width = 176, > + .height = 144, > + }, > + { > + .width = 160, > + .height = 120, > + }, > +}; > + > +/* Initial register configuration. */ > +static struct mt9v111_reg_list { > + u8 space; > + u8 reg; > + u16 mask; > + u16 val; > +} mt9v111_init_config[] = { > + { > + /* Disable AE and AWB to enable manual frame rate control. */ > + .space = MT9V111_R01_IFP, > + .reg = MT9V111_IFP_R06_OPMODE_CTRL, > + .mask = MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN | > + MT9V111_IFP_R06_OPMODE_CTRL_AE_EN, > + .val = 0, > + }, > + { > + /* Disable anti-flicker to enable manual frame rate control. */ > + .space = MT9V111_R01_IFP, > + .reg = MT9V111_IFP_R08_OUTFMT_CTRL, > + .mask = MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER, > + .val = 0, > + }, > + > + /* TODO: AE and AWB manual configuration. */ > +}; > + > +/* --- Device I/O access --- */ > + > +static int __mt9v111_read(struct i2c_client *c, u8 reg, u16 *val) > +{ > + struct i2c_msg msg[2]; > + __be16 buf; > + int ret; > + > + msg[0].addr = c->addr; > + msg[0].flags = 0; > + msg[0].len = 1; > + msg[0].buf = ® > + > + msg[1].addr = c->addr; > + msg[1].flags = I2C_M_RD; > + msg[1].len = 2; > + msg[1].buf = (char *)&buf; > + > + ret = i2c_transfer(c->adapter, msg, 2); > + if (ret < 0) { > + dev_err(&c->dev, "i2c read transfer error: %d\n", ret); > + return ret; > + } > + > + *val = be16_to_cpu(buf); > + > + dev_info(&c->dev, "%s: %x=%x\n", __func__, reg, *val); > + > + return 0; > +} > + > +static int __mt9v111_write(struct i2c_client *c, u8 reg, u16 val) > +{ > + struct i2c_msg msg; > + u8 buf[3] = { 0 }; > + int ret; > + > + buf[0] = reg; > + buf[1] = val >> 8; > + buf[2] = val & 0xff; > + > + msg.addr = c->addr; > + msg.flags = 0; > + msg.len = 3; > + msg.buf = (char *)buf; > + > + dev_info(&c->dev, "%s: %x = %x%x\n", __func__, reg, buf[1], buf[2]); > + > + ret = i2c_transfer(c->adapter, &msg, 1); > + if (ret < 0) { > + dev_err(&c->dev, "i2c write transfer error: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int __mt9v111_addr_space_select(struct i2c_client *c, u16 addr_space) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(c); > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + u16 val; > + int ret; > + > + spin_lock(&mt9v111->addr_lock); > + if (mt9v111->addr_space == addr_space) { > + spin_unlock(&mt9v111->addr_lock); > + return 0; > + } > + spin_unlock(&mt9v111->addr_lock); > + > + ret = __mt9v111_write(c, MT9V111_R01_ADDR_SPACE, addr_space); > + if (ret) > + return ret; > + > + /* Verify address space has been updated */ > + ret = __mt9v111_read(c, MT9V111_R01_ADDR_SPACE, &val); > + if (ret) > + return ret; > + > + if (val != addr_space) > + return -EINVAL; > + > + mt9v111->addr_space = addr_space; > + > + return 0; > +} > + > +static int mt9v111_read(struct i2c_client *c, u8 addr_space, u8 reg, u16 *val) > +{ > + int ret; > + > + /* Select register address space first. */ > + ret = __mt9v111_addr_space_select(c, addr_space); > + if (ret) > + return ret; > + > + ret = __mt9v111_read(c, reg, val); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int mt9v111_write(struct i2c_client *c, u8 addr_space, u8 reg, u16 val) > +{ > + int ret; > + > + /* Select register address space first. */ > + ret = __mt9v111_addr_space_select(c, addr_space); > + if (ret) > + return ret; > + > + ret = __mt9v111_write(c, reg, val); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int mt9v111_update(struct i2c_client *c, u8 addr_space, u8 reg, > + u16 mask, u16 val) > +{ > + u16 current_val; > + int ret; > + > + /* Select register address space first. */ > + ret = __mt9v111_addr_space_select(c, addr_space); > + if (ret) > + return ret; > + > + /* Read the current register value, then update it. */ > + ret = __mt9v111_read(c, reg, ¤t_val); > + if (ret) > + return ret; > + > + current_val &= ~mask; > + current_val |= (val & mask); > + ret = __mt9v111_write(c, reg, current_val); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int mt9v111_update_reglist(struct i2c_client *c, > + struct mt9v111_reg_list *reg_list, > + unsigned int n_regs) > +{ > + unsigned int i; > + int ret; > + > + for (i = 0; i < n_regs; i++) { > + struct mt9v111_reg_list *reg = ®_list[i]; > + > + ret = mt9v111_update(c, reg->space, reg->reg, reg->mask, > + reg->val); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +/* --- Sensor HW operations --- */ > + > +static int __mt9v111_power_on(struct v4l2_subdev *sd) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + int ret; > + > + ret = clk_prepare_enable(mt9v111->clk); > + if (ret) > + return ret; > + > + clk_set_rate(mt9v111->clk, mt9v111->sysclk); > + > + gpiod_set_value(mt9v111->standby, 0); > + usleep_range(500, 1000); > + > + gpiod_set_value(mt9v111->oe, 1); > + usleep_range(500, 1000); > + > + return 0; > +} > + > +static int __mt9v111_power_off(struct v4l2_subdev *sd) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + > + gpiod_set_value(mt9v111->oe, 0); > + usleep_range(500, 1000); > + > + gpiod_set_value(mt9v111->standby, 1); > + usleep_range(500, 1000); > + > + clk_disable_unprepare(mt9v111->clk); > + > + return 0; > +} > + > +static int __mt9v111_hw_reset(struct mt9v111_dev *mt9v111) > +{ > + if (!mt9v111->reset) > + return -EINVAL; > + > + gpiod_set_value(mt9v111->reset, 1); > + usleep_range(500, 1000); > + > + gpiod_set_value(mt9v111->reset, 0); > + usleep_range(500, 1000); > + > + return 0; > +} > + > +static int __mt9v111_sw_reset(struct mt9v111_dev *mt9v111) > +{ > + struct i2c_client *c = mt9v111->client; > + unsigned int ret; > + > + /* Software reset core and IFP blocks. */ > + > + ret = mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R0D_CORE_RESET, > + MT9V111_CORE_R0D_CORE_RESET_MASK, 1); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + ret = mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R0D_CORE_RESET, > + MT9V111_CORE_R0D_CORE_RESET_MASK, 0); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + ret = mt9v111_update(c, MT9V111_R01_IFP, > + MT9V111_IFP_R07_IFP_RESET, > + MT9V111_IFP_R07_IFP_RESET_MASK, 1); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + ret = mt9v111_update(c, MT9V111_R01_IFP, > + MT9V111_IFP_R07_IFP_RESET, > + MT9V111_IFP_R07_IFP_RESET_MASK, 0); > + if (ret) > + return ret; > + usleep_range(500, 1000); > + > + return 0; > +} > + > +static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111, > + struct v4l2_fract *tpf) > +{ > + unsigned int fps = tpf->numerator ? > + tpf->denominator / tpf->numerator : > + tpf->denominator; > + unsigned int best_diff; > + unsigned int frm_cols; > + unsigned int row_pclk; > + unsigned int best_fps; > + unsigned int pclk; > + unsigned int diff; > + unsigned int idx; > + unsigned int hb; > + unsigned int vb; > + unsigned int i; > + int ret; > + > + /* Approximate to the closest supported frame interval. */ > + best_diff = ~0L; > + for (i = 0, idx = 0; i < ARRAY_SIZE(mt9v111_frame_intervals); i++) { > + diff = abs(fps - mt9v111_frame_intervals[i]); > + if (diff < best_diff) { > + idx = i; > + best_diff = diff; > + } > + } > + fps = mt9v111_frame_intervals[idx]; > + > + /* > + * The sensor does not provide a PLL circuitry and pixel clock is > + * generated dividing by two the master clock source. > + * > + * Trow = (W + Hblank + 114) * 2 * (1 / SYSCLK) > + * TFrame = Trow * (H + Vblank + 2) > + * > + * FPS = (SYSCLK / 2) / (Trow * (H + Vblank + 2)) > + * > + * This boils down to tune H and V blanks to best approximate the > + * above equation. > + * > + * Test all available H/V blank values, until we reach the > + * desired frame rate. > + */ > + best_fps = vb = hb = 0; > + pclk = DIV_ROUND_CLOSEST(mt9v111->sysclk, 2); > + row_pclk = MT9V111_PIXEL_ARRAY_WIDTH + 7 + MT9V111_CORE_R04_WIN_H_OFFS; > + frm_cols = MT9V111_PIXEL_ARRAY_HEIGHT + 7 + MT9V111_CORE_R03_WIN_V_OFFS; > + > + best_diff = ~0L; > + for (vb = MT9V111_CORE_R06_MIN_VBLANK; > + vb < MT9V111_CORE_R06_MAX_VBLANK; vb++) { > + for (hb = MT9V111_CORE_R05_MIN_HBLANK; > + hb < MT9V111_CORE_R05_MAX_HBLANK; hb += 10) { > + unsigned int t_frame = (row_pclk + hb) * > + (frm_cols + vb); > + unsigned int t_fps = DIV_ROUND_CLOSEST(pclk, t_frame); > + > + diff = abs(fps - t_fps); > + if (diff < best_diff) { > + best_diff = diff; > + best_fps = t_fps; > + > + if (diff == 0) > + break; > + } > + } > + > + if (diff == 0) > + break; > + } > + > + dev_info(mt9v111->dev, "FPS: %u, hb = %u, vb = %u\n", fps, hb, vb); > + > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->hblank, hb); > + if (ret) > + return ret; > + > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->vblank, vb); > + if (ret) > + return ret; > + > + tpf->numerator = 1; > + tpf->denominator = best_fps; > + > + return 0; > +} > + > +static int mt9v111_hw_config(struct mt9v111_dev *mt9v111) > +{ > + struct i2c_client *c = mt9v111->client; > + unsigned int ret; > + u16 outfmtctrl2; > + > + /* Force device reset. */ > + ret = __mt9v111_hw_reset(mt9v111); > + if (ret == -EINVAL) > + ret = __mt9v111_sw_reset(mt9v111); > + if (ret) > + return ret; > + > + /* > + * Disable AE, AWB and anti-flickering to allow manual frame rate > + * control. > + */ > + ret = mt9v111_update_reglist(c, mt9v111_init_config, > + ARRAY_SIZE(mt9v111_init_config)); > + if (ret) > + return ret; > + > + /* Configure internal clock sample rate. */ > + ret = mt9v111->sysclk < DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? > + mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R07_OUT_CTRL, > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 1) : > + mt9v111_update(c, MT9V111_R01_CORE, > + MT9V111_CORE_R07_OUT_CTRL, > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 0); > + if (ret) > + return ret; > + > + /* > + * Configure output image format components ordering. > + * > + * TODO: IFP block can also output several RGB permutations, we only > + * support YUYV permutations at the moment. > + */ > + switch (mt9v111->fmt.code) { > + case MEDIA_BUS_FMT_YUYV8_2X8: > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC; > + break; > + case MEDIA_BUS_FMT_VYUY8_2X8: > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > + break; > + case MEDIA_BUS_FMT_YVYU8_2X8: > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC | > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > + break; > + case MEDIA_BUS_FMT_UYVY8_2X8: > + default: > + outfmtctrl2 = 0; > + break; > + } > + > + ret = mt9v111_update(c, MT9V111_R01_IFP, MT9V111_IFP_R3A_OUTFMT_CTRL2, > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK, > + outfmtctrl2); > + if (ret) > + return ret; > + > + /* > + * Do not change default sensor's core configuration: > + * output the whole 640x480 pixel array, skip 18 columns and 6 rows. > + * > + * Instead, control the output image size through IFP block. > + * > + * TODO: No zoom&pan support. Currently we control the output image > + * size only through decimation, with no zoom support. > + */ > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA5_HPAN, > + MT9V111_IFP_DECIMATION_FREEZE); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA8_VPAN, > + MT9V111_IFP_DECIMATION_FREEZE); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA6_HZOOM, > + MT9V111_IFP_DECIMATION_FREEZE | > + MT9V111_PIXEL_ARRAY_WIDTH); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA9_VZOOM, > + MT9V111_IFP_DECIMATION_FREEZE | > + MT9V111_PIXEL_ARRAY_HEIGHT); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA7_HOUT, > + MT9V111_IFP_DECIMATION_FREEZE | > + mt9v111->fmt.width); > + if (ret) > + return ret; > + > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RAA_VOUT, > + mt9v111->fmt.height); > + if (ret) > + return ret; > + > + /* Update blankings to match the programmed frame rate. */ > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R05_HBLANK, > + MT9V111_CORE_R05_MAX_HBLANK, > + mt9v111->hblank->val); > + if (ret) > + return ret; > + > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R06_VBLANK, > + MT9V111_CORE_R06_MAX_VBLANK, > + mt9v111->vblank->val); > + if (ret) > + return ret; > + > + /* > + * Set pixel integration time to the whole frame time. > + * This value controls the the shutter delay when running with AE > + * disabled. If longer than frame time, it affects the output > + * frame rate. > + */ > + ret = mt9v111_write(c, MT9V111_R01_CORE, MT9V111_CORE_R09_PIXEL_INT, > + MT9V111_PIXEL_ARRAY_HEIGHT); return mt9v111_write(...); > + if (ret) > + return ret; > + > + return 0; > +} > + > +/* --- V4L2 subdev operations --- */ > + > +static int mt9v111_s_power(struct v4l2_subdev *sd, int on) Using runtime PM would be nicer. See e.g. the smiapp driver for an example. This could be done as a separate change later on as well. > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + int pwr_count; > + int ret = 0; > + > + mutex_lock(&mt9v111->pwr_mutex); > + > + /* > + * Make sure we're transitioning from 0 to 1, or viceversa, only > + * before actually changing the power state. > + */ > + pwr_count = mt9v111->pwr_count; > + pwr_count += on ? 1 : -1; > + if (pwr_count == !!on) { > + ret = on ? __mt9v111_power_on(sd) : > + __mt9v111_power_off(sd); > + if (!ret) > + /* All went well, updated power counter. */ > + mt9v111->pwr_count = pwr_count; > + > + mutex_unlock(&mt9v111->pwr_mutex); > + > + return ret; > + } > + > + /* > + * Update power counter to keep track of how many nested calls we > + * received. > + */ > + WARN_ON(pwr_count < 0 || pwr_count > 1); > + mt9v111->pwr_count = pwr_count; > + > + mutex_unlock(&mt9v111->pwr_mutex); > + > + return ret; > +} > + > +static int mt9v111_s_stream(struct v4l2_subdev *subdev, int enable) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > + int ret; > + > + mutex_lock(&mt9v111->stream_mutex); > + > + if (mt9v111->streaming == enable) { > + mutex_unlock(&mt9v111->stream_mutex); > + return 0; > + } > + > + ret = mt9v111_s_power(subdev, enable); > + if (ret) > + goto error_unlock; > + > + if (enable && mt9v111->pending) { > + ret = mt9v111_hw_config(mt9v111); > + if (ret) > + goto error_unlock; > + > + /* > + * No need to update control here as far as only H/VBLANK are > + * supported and immediately programmed to registers in .s_ctrl > + */ > + > + mt9v111->pending = false; > + } > + > + mt9v111->streaming = enable ? true : false; > + mutex_unlock(&mt9v111->stream_mutex); > + > + return 0; > + > +error_unlock: > + mutex_unlock(&mt9v111->stream_mutex); > + > + return ret; > +} > + > +static int mt9v111_s_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *ival) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + struct v4l2_fract *tpf = &ival->interval; > + unsigned int fps = tpf->numerator ? > + tpf->denominator / tpf->numerator : > + tpf->denominator; > + unsigned int max_fps; > + > + mutex_lock(&mt9v111->stream_mutex); > + if (mt9v111->streaming) { > + mutex_unlock(&mt9v111->stream_mutex); > + return -EBUSY; > + } > + mutex_unlock(&mt9v111->stream_mutex); > + > + if (mt9v111->fps == fps) > + return 0; > + > + /* Make sure frame rate/image sizes constraints are respected. */ > + if (mt9v111->fmt.width < QVGA_WIDTH && > + mt9v111->fmt.height < QVGA_HEIGHT) > + max_fps = 90; > + else if (mt9v111->fmt.width < CIF_WIDTH && > + mt9v111->fmt.height < CIF_HEIGHT) > + max_fps = 60; > + else > + max_fps = mt9v111->sysclk < > + DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? 15 : > + 30; > + > + if (fps > max_fps) > + return -EINVAL; > + > + mt9v111_calc_frame_rate(mt9v111, tpf); > + > + mt9v111->fps = fps; > + mt9v111->pending = true; I think you'd need to keep the mutex until here. > + > + return 0; > +} > + > +static int mt9v111_g_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *ival) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + struct v4l2_fract *tpf = &ival->interval; > + > + tpf->numerator = 1; > + tpf->denominator = mt9v111->fps; Same here --- access to fps is serialised by the mutex. > + > + return 0; > +} > + > +static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format( > + struct mt9v111_dev *mt9v111, > + struct v4l2_subdev_pad_config *cfg, > + unsigned int pad, > + enum v4l2_subdev_format_whence which) > +{ > + switch (which) { > + case V4L2_SUBDEV_FORMAT_TRY: > +#ifdef CONFIG_MEDIA_CONTROLLER > + return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad); > +#else > + return &cfg->try_fmt; > +#endif > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &mt9v111->fmt; > + default: > + return NULL; > + } > +} > + > +static int mt9v111_enum_mbus_code(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + if (code->pad || code->index > ARRAY_SIZE(mt9v111_formats) - 1) > + return -EINVAL; > + > + code->code = mt9v111_formats[code->index].code; > + > + return 0; > +} > + > +static int mt9v111_enum_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_interval_enum *fie) > +{ > + unsigned int i; > + > + if (fie->pad || fie->index >= ARRAY_SIZE(mt9v111_frame_intervals)) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) > + if (fie->width == mt9v111_frame_sizes[i].width && > + fie->height == mt9v111_frame_sizes[i].height) > + break; > + > + if (i == ARRAY_SIZE(mt9v111_frame_sizes)) > + return -EINVAL; > + > + fie->interval.numerator = 1; > + fie->interval.denominator = mt9v111_frame_intervals[fie->index]; > + > + return 0; > +} > + > +static int mt9v111_enum_frame_size(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->pad || fse->index > ARRAY_SIZE(mt9v111_frame_sizes)) > + return -EINVAL; > + > + fse->min_width = mt9v111_frame_sizes[fse->index].width; > + fse->max_width = mt9v111_frame_sizes[fse->index].width; > + fse->min_height = mt9v111_frame_sizes[fse->index].height; > + fse->max_height = mt9v111_frame_sizes[fse->index].height; > + > + return 0; > +} > + > +static int mt9v111_get_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > + > + if (format->pad) > + return -EINVAL; > + > + format->format = *__mt9v111_get_pad_format(mt9v111, cfg, format->pad, > + format->which); Ditto. > + return 0; > +} > + > +static int mt9v111_set_format(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > + struct v4l2_mbus_framefmt new_fmt; > + struct v4l2_mbus_framefmt *__fmt; > + unsigned int best_fit = ~0L; > + unsigned int idx; > + unsigned int i; > + > + mutex_lock(&mt9v111->stream_mutex); > + if (mt9v111->streaming) { > + mutex_unlock(&mt9v111->stream_mutex); > + return -EBUSY; > + } > + mutex_unlock(&mt9v111->stream_mutex); > + > + if (format->pad) > + return -EINVAL; > + > + /* Update mbus format code and sizes. */ > + for (i = 0; i < ARRAY_SIZE(mt9v111_formats); i++) { > + if (format->format.code == mt9v111_formats[i].code) { > + new_fmt.code = mt9v111_formats[i].code; > + break; > + } > + } > + if (i == ARRAY_SIZE(mt9v111_formats)) > + new_fmt.code = mt9v111_formats[0].code; > + > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) { > + unsigned int fit = abs(mt9v111_frame_sizes[i].width - > + format->format.width) + > + abs(mt9v111_frame_sizes[i].height - > + format->format.height); > + if (fit < best_fit) { > + best_fit = fit; > + idx = i; > + > + if (fit == 0) > + break; > + } > + } > + new_fmt.width = mt9v111_frame_sizes[idx].width; > + new_fmt.height = mt9v111_frame_sizes[idx].height; > + > + /* Update the device (or pad) format if it has changed. */ > + __fmt = __mt9v111_get_pad_format(mt9v111, cfg, format->pad, > + format->which); > + > + /* Format hasn't changed, stop here. */ > + if (__fmt->code == new_fmt.code && > + __fmt->width == new_fmt.width && > + __fmt->height == new_fmt.height) > + goto done; > + > + /* Update the format and sizes, then mark changes as pending. */ > + __fmt->code = new_fmt.code; > + __fmt->width = new_fmt.width; > + __fmt->height = new_fmt.height; > + > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + mt9v111->pending = true; > + > + dev_info(mt9v111->dev, "%s: mbus_code: %x - (%ux%u)\n", > + __func__, __fmt->code, __fmt->width, __fmt->height); > + > +done: > + format->format = *__fmt; Same here. > + > + return 0; > +} > + > +static const struct v4l2_subdev_core_ops mt9v111_core_ops = { > + .s_power = mt9v111_s_power, > +}; > + > +static const struct v4l2_subdev_video_ops mt9v111_video_ops = { > + .s_stream = mt9v111_s_stream, > + .s_frame_interval = mt9v111_s_frame_interval, > + .g_frame_interval = mt9v111_g_frame_interval, > +}; > + > +static const struct v4l2_subdev_pad_ops mt9v111_pad_ops = { > + .enum_mbus_code = mt9v111_enum_mbus_code, > + .enum_frame_size = mt9v111_enum_frame_size, > + .enum_frame_interval = mt9v111_enum_frame_interval, > + .get_fmt = mt9v111_get_format, > + .set_fmt = mt9v111_set_format, > +}; > + > +static const struct v4l2_subdev_ops mt9v111_ops = { > + .core = &mt9v111_core_ops, > + .video = &mt9v111_video_ops, > + .pad = &mt9v111_pad_ops, > +}; > + > +#ifdef CONFIG_MEDIA_CONTROLLER > +static const struct media_entity_operations mt9v111_subdev_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static int mt9v111_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + struct v4l2_mbus_framefmt *__fmt = > + v4l2_subdev_get_try_format(sd, fh->pad, 0); > + > + /* Initialize try format: copy the device format. */ > + *__fmt = mt9v111->fmt; Please use default configuration, not the current configuration. If this is all you need in open, I'd suggest to implement the init_cfg pad op instead. > + > + return 0; > +} > + > +static const struct v4l2_subdev_internal_ops mt9v111_internal_ops = { > + .open = mt9v111_open, > +}; > +#endif > + > +/* --- V4L2 ctrl --- */ > +static int mt9v111_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct mt9v111_dev *mt9v111 = container_of(ctrl->handler, > + struct mt9v111_dev, > + ctrls); > + int ret = 0; -EINVAL? That way any controls not handled by the function won't be missed. > + > + mutex_lock(&mt9v111->pwr_mutex); > + /* > + * If sensor is powered down, just cache new control values, > + * no actual register access. > + */ > + if (!mt9v111->pwr_count) { > + mutex_unlock(&mt9v111->pwr_mutex); > + return 0; > + } > + mutex_unlock(&mt9v111->pwr_mutex); > + > + switch (ctrl->id) { > + case V4L2_CID_HBLANK: > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > + MT9V111_CORE_R05_HBLANK, > + MT9V111_CORE_R05_MAX_HBLANK, > + mt9v111->hblank->val); > + break; > + case V4L2_CID_VBLANK: > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > + MT9V111_CORE_R06_VBLANK, > + MT9V111_CORE_R06_MAX_VBLANK, > + mt9v111->vblank->val); > + break; > + } > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops mt9v111_ctrl_ops = { > + .s_ctrl = mt9v111_s_ctrl, > +}; > + > +static int mt9v111_chip_probe(struct mt9v111_dev *mt9v111) > +{ > + int ret; > + u16 val; > + > + ret = __mt9v111_power_on(&mt9v111->sd); > + if (ret) > + return ret; > + > + ret = mt9v111_read(mt9v111->client, MT9V111_R01_CORE, > + MT9V111_CORE_RFF_CHIP_VER, &val); > + if (ret) > + return ret; Power off the device on failure? Same below. > + > + if ((val >> 8) != MT9V111_CHIP_ID_HIGH && > + (val & 0xff) != MT9V111_CHIP_ID_LOW) { > + dev_err(mt9v111->dev, > + "Unable to identify MT9V111 chip: 0x%2x%2x\n", > + val >> 8, val & 0xff); > + return ret; > + } > + > + ret = __mt9v111_power_off(&mt9v111->sd); > + if (ret) > + return ret; > + > + dev_info(mt9v111->dev, "Chip identified: 0x%2x%2x\n", > + val >> 8, val & 0xff); > + > + return 0; > +} > + > +static int mt9v111_probe(struct i2c_client *client) > +{ > + struct mt9v111_dev *mt9v111; > + struct v4l2_fract tpf; > + int ret; > + > + mt9v111 = devm_kzalloc(&client->dev, sizeof(*mt9v111), GFP_KERNEL); > + if (!mt9v111) > + return -ENOMEM; > + > + mt9v111->dev = &client->dev; > + mt9v111->client = client; > + > + mt9v111->clk = devm_clk_get(&client->dev, NULL); > + if (IS_ERR(mt9v111->clk)) > + return PTR_ERR(mt9v111->clk); > + > + mt9v111->sysclk = clk_get_rate(mt9v111->clk); > + if (mt9v111->sysclk > MT9V111_MAX_CLKIN) > + return -EINVAL; > + > + mt9v111->oe = devm_gpiod_get_optional(&client->dev, "enable", > + GPIOD_OUT_LOW); > + if (IS_ERR(mt9v111->oe)) { > + dev_err(&client->dev, "Unable to get GPIO \"enable\": %ld\n", > + PTR_ERR(mt9v111->oe)); > + return PTR_ERR(mt9v111->oe); > + } > + > + mt9v111->standby = devm_gpiod_get_optional(&client->dev, "standby", > + GPIOD_OUT_HIGH); > + if (IS_ERR(mt9v111->standby)) { > + dev_err(&client->dev, "Unable to get GPIO \"standby\": %ld\n", > + PTR_ERR(mt9v111->standby)); > + return PTR_ERR(mt9v111->standby); > + } > + > + mt9v111->reset = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_OUT_LOW); > + if (IS_ERR(mt9v111->reset)) { > + dev_err(&client->dev, "Unable to get GPIO \"reset\": %ld\n", > + PTR_ERR(mt9v111->reset)); > + return PTR_ERR(mt9v111->reset); > + } > + > + mutex_init(&mt9v111->pwr_mutex); > + mutex_init(&mt9v111->stream_mutex); > + spin_lock_init(&mt9v111->addr_lock); > + > + v4l2_ctrl_handler_init(&mt9v111->ctrls, 3); > + > + mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > + V4L2_CID_HBLANK, > + MT9V111_CORE_R05_MIN_HBLANK, > + MT9V111_CORE_R05_MAX_HBLANK, 1, > + MT9V111_CORE_R05_DEF_HBLANK); > + if (IS_ERR_OR_NULL(mt9v111->hblank)) { > + ret = PTR_ERR(mt9v111->hblank); > + goto error_free_ctrls; > + } > + > + mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > + V4L2_CID_VBLANK, > + MT9V111_CORE_R06_MIN_VBLANK, > + MT9V111_CORE_R06_MAX_VBLANK, 1, > + MT9V111_CORE_R06_DEF_VBLANK); > + if (IS_ERR_OR_NULL(mt9v111->vblank)) { > + ret = PTR_ERR(mt9v111->vblank); > + goto error_free_ctrls; > + } > + > + /* PIXEL_RATE is fixed: just expose it to user space. */ > + v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > + V4L2_CID_PIXEL_RATE, 0, > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2), 1, > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2)); > + > + mt9v111->sd.ctrl_handler = &mt9v111->ctrls; > + > + /* Start with default configuration: 640x480 UYVY. */ > + mt9v111->fmt.width = 640; > + mt9v111->fmt.height = 480; > + mt9v111->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; > + > + /* These are fixed for all supported formats. */ > + mt9v111->fmt.field = V4L2_FIELD_NONE; > + mt9v111->fmt.colorspace = V4L2_COLORSPACE_SRGB; > + mt9v111->fmt.ycbcr_enc = V4L2_YCBCR_ENC_601; > + mt9v111->fmt.quantization = V4L2_QUANTIZATION_LIM_RANGE; > + mt9v111->fmt.xfer_func = V4L2_XFER_FUNC_SRGB; > + > + /* Re-calculate blankings for 640x480@15fps. */ > + mt9v111->fps = 15; > + tpf.numerator = 1; > + tpf.denominator = mt9v111->fps; > + ret = mt9v111_calc_frame_rate(mt9v111, &tpf); > + if (ret) > + goto error_free_ctrls; > + > + mt9v111->pwr_count = 0; > + mt9v111->addr_space = MT9V111_R01_IFP; > + mt9v111->pending = true; > + > + v4l2_i2c_subdev_init(&mt9v111->sd, client, &mt9v111_ops); > + > +#ifdef CONFIG_MEDIA_CONTROLLER > + mt9v111->sd.internal_ops = &mt9v111_internal_ops; > + mt9v111->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + mt9v111->sd.entity.ops = &mt9v111_subdev_entity_ops; > + mt9v111->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + > + mt9v111->pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&mt9v111->sd.entity, 1, &mt9v111->pad); > + if (ret) > + goto error_free_ctrls; > +#endif > + > + ret = mt9v111_chip_probe(mt9v111); > + if (ret) > + goto error_free_ctrls; > + > + ret = v4l2_async_register_subdev(&mt9v111->sd); > + if (ret) > + goto error_free_ctrls; > + > + return 0; > + > +error_free_ctrls: > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > + > +#ifdef CONFIG_MEDIA_CONTROLLER > + media_entity_cleanup(&mt9v111->sd.entity); > +#endif > + > + mutex_destroy(&mt9v111->pwr_mutex); > + mutex_destroy(&mt9v111->stream_mutex); > + > + return ret; > +} > + > +static int mt9v111_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > + > + v4l2_async_unregister_subdev(sd); > + > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > + > +#ifdef CONFIG_MEDIA_CONTROLLER > + media_entity_cleanup(&sd->entity); > +#endif > + > + mutex_destroy(&mt9v111->pwr_mutex); > + mutex_destroy(&mt9v111->stream_mutex); > + > + devm_gpiod_put(mt9v111->dev, mt9v111->oe); > + devm_gpiod_put(mt9v111->dev, mt9v111->standby); > + devm_gpiod_put(mt9v111->dev, mt9v111->reset); > + > + devm_clk_put(mt9v111->dev, mt9v111->clk); > + > + return 0; > +} > + > +static const struct of_device_id mt9v111_of_match[] = { > + { .compatible = "aptina,mt9v111", }, > + { /* sentinel */ }, > +}; > + > +static struct i2c_driver mt9v111_driver = { > + .driver = { > + .name = "mt9v111", > + .of_match_table = of_match_ptr(mt9v111_of_match), Please assign this without of_match_ptr(). > + }, > + .probe_new = mt9v111_probe, > + .remove = mt9v111_remove, > +}; > + > +module_i2c_driver(mt9v111_driver); > + > +MODULE_DESCRIPTION("V4L2 sensor driver for Aptina MT9V111"); > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>"); > +MODULE_LICENSE("GPL v2"); -- Kind regards, Sakari Ailus e-mail: sakari.ailus@iki.fi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 2018-07-23 14:28 ` Sakari Ailus @ 2018-07-24 13:08 ` jacopo mondi 2018-07-24 13:23 ` Sakari Ailus 0 siblings, 1 reply; 12+ messages in thread From: jacopo mondi @ 2018-07-24 13:08 UTC (permalink / raw) To: Sakari Ailus Cc: Jacopo Mondi, mchehab, linux-media, linux-kernel, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 43357 bytes --] Hello Sakari, thanks for the comments.. On Mon, Jul 23, 2018 at 05:28:35PM +0300, Sakari Ailus wrote: > Hi Jacopo, > > Apologies for the late review. Please see my comments below. > > On Mon, Jun 11, 2018 at 05:17:33PM +0200, Jacopo Mondi wrote: > > Add V4L2 sensor driver for Aptina MT9V111 CMOS image sensor. > > > > The MT9V111 is a 1/4-Inch CMOS image sensor based on MT9V011 with an > > integrated Image Flow Processor. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > MAINTAINERS | 8 + > > Could you move the MAINTAINERS change to the DT binding documentation > patch? Currently I presume adding a binding file which isn't accounted for > produces a checkpatch.pl warning. > Yup! > > drivers/media/i2c/Kconfig | 12 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/mt9v111.c | 1297 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1318 insertions(+) > > create mode 100644 drivers/media/i2c/mt9v111.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index a38e24a..2c2fe60 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9523,6 +9523,14 @@ F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > F: drivers/media/i2c/mt9v032.c > > F: include/media/i2c/mt9v032.h > > > > +MT9V111 APTINA CAMERA SENSOR > > +M: Jacopo Mondi <jacopo@jmondi.org> > > +L: linux-media@vger.kernel.org > > +T: git git://linuxtv.org/media_tree.git > > +S: Maintained > > +F: Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > > +F: drivers/media/i2c/mt9v111.c > > + > > MULTIFUNCTION DEVICES (MFD) > > M: Lee Jones <lee.jones@linaro.org> > > T: git git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 341452f..0bd867d 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -841,6 +841,18 @@ config VIDEO_MT9V032 > > This is a Video4Linux2 sensor-level driver for the Micron > > MT9V032 752x480 CMOS sensor. > > > > +config VIDEO_MT9V111 > > + tristate "Aptina MT9V111 sensor support" > > + depends on I2C && VIDEO_V4L2 > > + depends on MEDIA_CAMERA_SUPPORT > > + depends on OF > > Why OF? I see no effective OF dependencies in the driver. > Isn't the driver probing through OF? > > + help > > + This is a Video4Linux2 sensor-level driver for the Aptina/Micron I'll change sensor-level to sensor, has you've been doing lately on all other Kconfig entries > > + MT9V111 sensor. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called mt9v111. > > + > > config VIDEO_SR030PC30 > > tristate "Siliconfile SR030PC30 sensor support" > > depends on I2C && VIDEO_V4L2 > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > > index d679d57..f0e8618 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o > > obj-$(CONFIG_VIDEO_MT9T112) += mt9t112.o > > obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o > > obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o > > +obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o > > obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o > > obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o > > obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o > > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > > new file mode 100644 > > index 0000000..36e7424 > > --- /dev/null > > +++ b/drivers/media/i2c/mt9v111.c > > @@ -0,0 +1,1297 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * V4L2 sensor driver for Aptina MT9V111 image sensor > > + * Copyright (C) 2018 Jacopo Mondi <jacopo@jmondi.org> > > + * > > + * Based on mt9v032 driver > > + * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de> > > + * > > + * Base on mt9v011 driver > > + * Copyright (c) 2009 Mauro Carvalho Chehab <mchehab@kernel.org> > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/of.h> > > +#include <linux/slab.h> > > +#include <linux/videodev2.h> > > +#include <linux/v4l2-mediabus.h> > > +#include <linux/module.h> > > + > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-device.h> > > +#include <media/v4l2-fwnode.h> > > +#include <media/v4l2-image-sizes.h> > > +#include <media/v4l2-subdev.h> > > + > > +/* > > + * MT9V111 is a 1/4-Inch CMOS digital image sensor with an integrated > > + * Image Flow Processing (IFP) engine and a sensor core loosely based on > > + * MT9V011. > > + * > > + * The IFP can produce several output image formats from the sensor core > > + * output, this driver currently supports only YUYV format permutations. > > + * > > + * As the auto exposure algorithms controls exposure time and modifies the > > + * frame rate, this driver disables auto exposure control, auto white balancing > > + * and auto flickering avoidance to allow manual frame rate control through > > + * s_frame_interval subdev operation or V4L2_CID_V/HBLANK controls. > > + * > > + * While it seems possible to instruct the auto-exposure control algorithm to > > + * respect a programmed frame rate when adjusting the pixel integration time, > > + * registers controlling this feature are not documented in the public > > + * available sensor manual used to develop this driver (09005aef80e90084, > > + * MT9V111_1.fm - Rev. G 1/05 EN). > > + */ > > + > > +#define MT9V111_CHIP_ID_HIGH 0x82 > > +#define MT9V111_CHIP_ID_LOW 0x3a > > + > > +#define MT9V111_R01_ADDR_SPACE 0x01 > > +#define MT9V111_R01_IFP 0x01 > > +#define MT9V111_R01_CORE 0x04 > > + > > +#define MT9V111_IFP_R06_OPMODE_CTRL 0x06 > > +#define MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN BIT(1) > > +#define MT9V111_IFP_R06_OPMODE_CTRL_AE_EN BIT(14) > > +#define MT9V111_IFP_R07_IFP_RESET 0x07 > > +#define MT9V111_IFP_R07_IFP_RESET_MASK BIT(0) > > +#define MT9V111_IFP_R08_OUTFMT_CTRL 0x08 > > +#define MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER BIT(11) > > +#define MT9V111_IFP_R08_OUTFMT_CTRL_PCLK BIT(5) > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2 0x3a > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR BIT(0) > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC BIT(1) > > +#define MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK GENMASK(2, 0) > > +#define MT9V111_IFP_RA5_HPAN 0xa5 > > +#define MT9V111_IFP_RA6_HZOOM 0xa6 > > +#define MT9V111_IFP_RA7_HOUT 0xa7 > > +#define MT9V111_IFP_RA8_VPAN 0xa8 > > +#define MT9V111_IFP_RA9_VZOOM 0xa9 > > +#define MT9V111_IFP_RAA_VOUT 0xaa > > +#define MT9V111_IFP_DECIMATION_MASK GENMASK(9, 0) > > +#define MT9V111_IFP_DECIMATION_FREEZE BIT(15) > > + > > +#define MT9V111_CORE_R03_WIN_HEIGHT 0x03 > > +#define MT9V111_CORE_R03_WIN_V_OFFS 2 > > +#define MT9V111_CORE_R04_WIN_WIDTH 0x04 > > +#define MT9V111_CORE_R04_WIN_H_OFFS 114 > > +#define MT9V111_CORE_R05_HBLANK 0x05 > > +#define MT9V111_CORE_R05_MIN_HBLANK 0x09 > > +#define MT9V111_CORE_R05_MAX_HBLANK GENMASK(9, 0) > > +#define MT9V111_CORE_R05_DEF_HBLANK 0x26 > > +#define MT9V111_CORE_R06_VBLANK 0x06 > > +#define MT9V111_CORE_R06_MIN_VBLANK 0x03 > > +#define MT9V111_CORE_R06_MAX_VBLANK GENMASK(11, 0) > > +#define MT9V111_CORE_R06_DEF_VBLANK 0x04 > > +#define MT9V111_CORE_R07_OUT_CTRL 0x07 > > +#define MT9V111_CORE_R07_OUT_CTRL_SAMPLE BIT(4) > > +#define MT9V111_CORE_R09_PIXEL_INT 0x09 > > +#define MT9V111_CORE_R09_PIXEL_INT_MASK GENMASK(11, 0) > > +#define MT9V111_CORE_R0D_CORE_RESET 0x0d > > +#define MT9V111_CORE_R0D_CORE_RESET_MASK BIT(0) > > +#define MT9V111_CORE_RFF_CHIP_VER 0xff > > + > > +#define MT9V111_PIXEL_ARRAY_WIDTH 640 > > +#define MT9V111_PIXEL_ARRAY_HEIGHT 480 > > + > > +#define MT9V111_MAX_CLKIN 27000000 > > + > > +struct mt9v111_dev { > > + struct device *dev; > > + struct i2c_client *client; > > + > > + /* Protects address space change operations. */ > > + spinlock_t addr_lock; > > + u8 addr_space; > > + > > + struct v4l2_subdev sd; > > +#if defined(CONFIG_MEDIA_CONTROLLER) > > + struct media_pad pad; > > +#endif > > + > > + struct v4l2_ctrl *hblank; > > + struct v4l2_ctrl *vblank; > > + struct v4l2_ctrl_handler ctrls; > > + > > + /* Output image format and sizes. */ > > + struct v4l2_mbus_framefmt fmt; > > + unsigned int fps; > > + > > + /* Protects power up/down sequences. */ > > + struct mutex pwr_mutex; > > + int pwr_count; > > + > > + /* Protects stream on/off sequences. */ > > + struct mutex stream_mutex; > > + bool streaming; > > + > > + /* Flags to mark HW settings as not yet applied. */ > > + bool pending; > > + > > + /* Clock provider and system clock frequency. */ > > + struct clk *clk; > > + u32 sysclk; > > + > > + struct gpio_desc *oe; > > + struct gpio_desc *standby; > > + struct gpio_desc *reset; > > +}; > > + > > +#define sd_to_mt9v111(__sd) container_of(__sd, struct mt9v111_dev, sd) > > + > > +/* > > + * mt9v111_mbus_fmt - List all media bus formats supported by the driver. > > + * > > + * Only list the media bus code here. The image sizes are freely configurable > > + * in the pixel array sizes range. > > + * > > + * The frame desired frame interval, in the supported frame interval range, is > > + * obtained by configuring blanking as the sensor does not have a PLL but > > + * only a fixed clock divider that generates the output pixel clock. > > + */ > > +static struct mt9v111_mbus_fmt { > > + u32 code; > > +} mt9v111_formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > + }, > > +}; > > + > > +static u32 mt9v111_frame_intervals[] = {5, 10, 15, 20, 30}; > > + > > +/* > > + * mt9v111_frame_sizes - List sensor's supported resolutions. > > + * > > + * Resolution generated through decimation in the IFP block from the > > + * full VGA pixel array. > > + */ > > +static struct v4l2_rect mt9v111_frame_sizes[] = { > > + { > > + .width = 640, > > + .height = 480, > > + }, > > + { > > + .width = 352, > > + .height = 288 > > + }, > > + { > > + .width = 320, > > + .height = 240, > > + }, > > + { > > + .width = 176, > > + .height = 144, > > + }, > > + { > > + .width = 160, > > + .height = 120, > > + }, > > +}; > > + > > +/* Initial register configuration. */ > > +static struct mt9v111_reg_list { > > + u8 space; > > + u8 reg; > > + u16 mask; > > + u16 val; > > +} mt9v111_init_config[] = { > > + { > > + /* Disable AE and AWB to enable manual frame rate control. */ > > + .space = MT9V111_R01_IFP, > > + .reg = MT9V111_IFP_R06_OPMODE_CTRL, > > + .mask = MT9V111_IFP_R06_OPMODE_CTRL_AWB_EN | > > + MT9V111_IFP_R06_OPMODE_CTRL_AE_EN, > > + .val = 0, > > + }, > > + { > > + /* Disable anti-flicker to enable manual frame rate control. */ > > + .space = MT9V111_R01_IFP, > > + .reg = MT9V111_IFP_R08_OUTFMT_CTRL, > > + .mask = MT9V111_IFP_R08_OUTFMT_CTRL_FLICKER, > > + .val = 0, > > + }, > > + > > + /* TODO: AE and AWB manual configuration. */ > > +}; > > + > > +/* --- Device I/O access --- */ > > + > > +static int __mt9v111_read(struct i2c_client *c, u8 reg, u16 *val) > > +{ > > + struct i2c_msg msg[2]; > > + __be16 buf; > > + int ret; > > + > > + msg[0].addr = c->addr; > > + msg[0].flags = 0; > > + msg[0].len = 1; > > + msg[0].buf = ® > > + > > + msg[1].addr = c->addr; > > + msg[1].flags = I2C_M_RD; > > + msg[1].len = 2; > > + msg[1].buf = (char *)&buf; > > + > > + ret = i2c_transfer(c->adapter, msg, 2); > > + if (ret < 0) { > > + dev_err(&c->dev, "i2c read transfer error: %d\n", ret); > > + return ret; > > + } > > + > > + *val = be16_to_cpu(buf); > > + > > + dev_info(&c->dev, "%s: %x=%x\n", __func__, reg, *val); > > + > > + return 0; > > +} > > + > > +static int __mt9v111_write(struct i2c_client *c, u8 reg, u16 val) > > +{ > > + struct i2c_msg msg; > > + u8 buf[3] = { 0 }; > > + int ret; > > + > > + buf[0] = reg; > > + buf[1] = val >> 8; > > + buf[2] = val & 0xff; > > + > > + msg.addr = c->addr; > > + msg.flags = 0; > > + msg.len = 3; > > + msg.buf = (char *)buf; > > + > > + dev_info(&c->dev, "%s: %x = %x%x\n", __func__, reg, buf[1], buf[2]); > > + > > + ret = i2c_transfer(c->adapter, &msg, 1); > > + if (ret < 0) { > > + dev_err(&c->dev, "i2c write transfer error: %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int __mt9v111_addr_space_select(struct i2c_client *c, u16 addr_space) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(c); > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + u16 val; > > + int ret; > > + > > + spin_lock(&mt9v111->addr_lock); > > + if (mt9v111->addr_space == addr_space) { > > + spin_unlock(&mt9v111->addr_lock); > > + return 0; > > + } > > + spin_unlock(&mt9v111->addr_lock); > > + > > + ret = __mt9v111_write(c, MT9V111_R01_ADDR_SPACE, addr_space); > > + if (ret) > > + return ret; > > + > > + /* Verify address space has been updated */ > > + ret = __mt9v111_read(c, MT9V111_R01_ADDR_SPACE, &val); > > + if (ret) > > + return ret; > > + > > + if (val != addr_space) > > + return -EINVAL; > > + > > + mt9v111->addr_space = addr_space; > > + > > + return 0; > > +} > > + > > +static int mt9v111_read(struct i2c_client *c, u8 addr_space, u8 reg, u16 *val) > > +{ > > + int ret; > > + > > + /* Select register address space first. */ > > + ret = __mt9v111_addr_space_select(c, addr_space); > > + if (ret) > > + return ret; > > + > > + ret = __mt9v111_read(c, reg, val); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int mt9v111_write(struct i2c_client *c, u8 addr_space, u8 reg, u16 val) > > +{ > > + int ret; > > + > > + /* Select register address space first. */ > > + ret = __mt9v111_addr_space_select(c, addr_space); > > + if (ret) > > + return ret; > > + > > + ret = __mt9v111_write(c, reg, val); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int mt9v111_update(struct i2c_client *c, u8 addr_space, u8 reg, > > + u16 mask, u16 val) > > +{ > > + u16 current_val; > > + int ret; > > + > > + /* Select register address space first. */ > > + ret = __mt9v111_addr_space_select(c, addr_space); > > + if (ret) > > + return ret; > > + > > + /* Read the current register value, then update it. */ > > + ret = __mt9v111_read(c, reg, ¤t_val); > > + if (ret) > > + return ret; > > + > > + current_val &= ~mask; > > + current_val |= (val & mask); > > + ret = __mt9v111_write(c, reg, current_val); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int mt9v111_update_reglist(struct i2c_client *c, > > + struct mt9v111_reg_list *reg_list, > > + unsigned int n_regs) > > +{ > > + unsigned int i; > > + int ret; > > + > > + for (i = 0; i < n_regs; i++) { > > + struct mt9v111_reg_list *reg = ®_list[i]; > > + > > + ret = mt9v111_update(c, reg->space, reg->reg, reg->mask, > > + reg->val); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +/* --- Sensor HW operations --- */ > > + > > +static int __mt9v111_power_on(struct v4l2_subdev *sd) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + int ret; > > + > > + ret = clk_prepare_enable(mt9v111->clk); > > + if (ret) > > + return ret; > > + > > + clk_set_rate(mt9v111->clk, mt9v111->sysclk); > > + > > + gpiod_set_value(mt9v111->standby, 0); > > + usleep_range(500, 1000); > > + > > + gpiod_set_value(mt9v111->oe, 1); > > + usleep_range(500, 1000); > > + > > + return 0; > > +} > > + > > +static int __mt9v111_power_off(struct v4l2_subdev *sd) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + > > + gpiod_set_value(mt9v111->oe, 0); > > + usleep_range(500, 1000); > > + > > + gpiod_set_value(mt9v111->standby, 1); > > + usleep_range(500, 1000); > > + > > + clk_disable_unprepare(mt9v111->clk); > > + > > + return 0; > > +} > > + > > +static int __mt9v111_hw_reset(struct mt9v111_dev *mt9v111) > > +{ > > + if (!mt9v111->reset) > > + return -EINVAL; > > + > > + gpiod_set_value(mt9v111->reset, 1); > > + usleep_range(500, 1000); > > + > > + gpiod_set_value(mt9v111->reset, 0); > > + usleep_range(500, 1000); > > + > > + return 0; > > +} > > + > > +static int __mt9v111_sw_reset(struct mt9v111_dev *mt9v111) > > +{ > > + struct i2c_client *c = mt9v111->client; > > + unsigned int ret; > > + > > + /* Software reset core and IFP blocks. */ > > + > > + ret = mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R0D_CORE_RESET, > > + MT9V111_CORE_R0D_CORE_RESET_MASK, 1); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + ret = mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R0D_CORE_RESET, > > + MT9V111_CORE_R0D_CORE_RESET_MASK, 0); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + ret = mt9v111_update(c, MT9V111_R01_IFP, > > + MT9V111_IFP_R07_IFP_RESET, > > + MT9V111_IFP_R07_IFP_RESET_MASK, 1); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + ret = mt9v111_update(c, MT9V111_R01_IFP, > > + MT9V111_IFP_R07_IFP_RESET, > > + MT9V111_IFP_R07_IFP_RESET_MASK, 0); > > + if (ret) > > + return ret; > > + usleep_range(500, 1000); > > + > > + return 0; > > +} > > + > > +static int mt9v111_calc_frame_rate(struct mt9v111_dev *mt9v111, > > + struct v4l2_fract *tpf) > > +{ > > + unsigned int fps = tpf->numerator ? > > + tpf->denominator / tpf->numerator : > > + tpf->denominator; > > + unsigned int best_diff; > > + unsigned int frm_cols; > > + unsigned int row_pclk; > > + unsigned int best_fps; > > + unsigned int pclk; > > + unsigned int diff; > > + unsigned int idx; > > + unsigned int hb; > > + unsigned int vb; > > + unsigned int i; > > + int ret; > > + > > + /* Approximate to the closest supported frame interval. */ > > + best_diff = ~0L; > > + for (i = 0, idx = 0; i < ARRAY_SIZE(mt9v111_frame_intervals); i++) { > > + diff = abs(fps - mt9v111_frame_intervals[i]); > > + if (diff < best_diff) { > > + idx = i; > > + best_diff = diff; > > + } > > + } > > + fps = mt9v111_frame_intervals[idx]; > > + > > + /* > > + * The sensor does not provide a PLL circuitry and pixel clock is > > + * generated dividing by two the master clock source. > > + * > > + * Trow = (W + Hblank + 114) * 2 * (1 / SYSCLK) > > + * TFrame = Trow * (H + Vblank + 2) > > + * > > + * FPS = (SYSCLK / 2) / (Trow * (H + Vblank + 2)) > > + * > > + * This boils down to tune H and V blanks to best approximate the > > + * above equation. > > + * > > + * Test all available H/V blank values, until we reach the > > + * desired frame rate. > > + */ > > + best_fps = vb = hb = 0; > > + pclk = DIV_ROUND_CLOSEST(mt9v111->sysclk, 2); > > + row_pclk = MT9V111_PIXEL_ARRAY_WIDTH + 7 + MT9V111_CORE_R04_WIN_H_OFFS; > > + frm_cols = MT9V111_PIXEL_ARRAY_HEIGHT + 7 + MT9V111_CORE_R03_WIN_V_OFFS; > > + > > + best_diff = ~0L; > > + for (vb = MT9V111_CORE_R06_MIN_VBLANK; > > + vb < MT9V111_CORE_R06_MAX_VBLANK; vb++) { > > + for (hb = MT9V111_CORE_R05_MIN_HBLANK; > > + hb < MT9V111_CORE_R05_MAX_HBLANK; hb += 10) { > > + unsigned int t_frame = (row_pclk + hb) * > > + (frm_cols + vb); > > + unsigned int t_fps = DIV_ROUND_CLOSEST(pclk, t_frame); > > + > > + diff = abs(fps - t_fps); > > + if (diff < best_diff) { > > + best_diff = diff; > > + best_fps = t_fps; > > + > > + if (diff == 0) > > + break; > > + } > > + } > > + > > + if (diff == 0) > > + break; > > + } > > + > > + dev_info(mt9v111->dev, "FPS: %u, hb = %u, vb = %u\n", fps, hb, vb); > > + > > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->hblank, hb); > > + if (ret) > > + return ret; > > + > > + ret = v4l2_ctrl_s_ctrl_int64(mt9v111->vblank, vb); > > + if (ret) > > + return ret; > > + > > + tpf->numerator = 1; > > + tpf->denominator = best_fps; > > + > > + return 0; > > +} > > + > > +static int mt9v111_hw_config(struct mt9v111_dev *mt9v111) > > +{ > > + struct i2c_client *c = mt9v111->client; > > + unsigned int ret; > > + u16 outfmtctrl2; > > + > > + /* Force device reset. */ > > + ret = __mt9v111_hw_reset(mt9v111); > > + if (ret == -EINVAL) > > + ret = __mt9v111_sw_reset(mt9v111); > > + if (ret) > > + return ret; > > + > > + /* > > + * Disable AE, AWB and anti-flickering to allow manual frame rate > > + * control. > > + */ > > + ret = mt9v111_update_reglist(c, mt9v111_init_config, > > + ARRAY_SIZE(mt9v111_init_config)); > > + if (ret) > > + return ret; > > + > > + /* Configure internal clock sample rate. */ > > + ret = mt9v111->sysclk < DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? > > + mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R07_OUT_CTRL, > > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 1) : > > + mt9v111_update(c, MT9V111_R01_CORE, > > + MT9V111_CORE_R07_OUT_CTRL, > > + MT9V111_CORE_R07_OUT_CTRL_SAMPLE, 0); > > + if (ret) > > + return ret; > > + > > + /* > > + * Configure output image format components ordering. > > + * > > + * TODO: IFP block can also output several RGB permutations, we only > > + * support YUYV permutations at the moment. > > + */ > > + switch (mt9v111->fmt.code) { > > + case MEDIA_BUS_FMT_YUYV8_2X8: > > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC; > > + break; > > + case MEDIA_BUS_FMT_VYUY8_2X8: > > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > > + break; > > + case MEDIA_BUS_FMT_YVYU8_2X8: > > + outfmtctrl2 = MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_YC | > > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_CBCR; > > + break; > > + case MEDIA_BUS_FMT_UYVY8_2X8: > > + default: > > + outfmtctrl2 = 0; > > + break; > > + } > > + > > + ret = mt9v111_update(c, MT9V111_R01_IFP, MT9V111_IFP_R3A_OUTFMT_CTRL2, > > + MT9V111_IFP_R3A_OUTFMT_CTRL2_SWAP_MASK, > > + outfmtctrl2); > > + if (ret) > > + return ret; > > + > > + /* > > + * Do not change default sensor's core configuration: > > + * output the whole 640x480 pixel array, skip 18 columns and 6 rows. > > + * > > + * Instead, control the output image size through IFP block. > > + * > > + * TODO: No zoom&pan support. Currently we control the output image > > + * size only through decimation, with no zoom support. > > + */ > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA5_HPAN, > > + MT9V111_IFP_DECIMATION_FREEZE); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA8_VPAN, > > + MT9V111_IFP_DECIMATION_FREEZE); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA6_HZOOM, > > + MT9V111_IFP_DECIMATION_FREEZE | > > + MT9V111_PIXEL_ARRAY_WIDTH); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA9_VZOOM, > > + MT9V111_IFP_DECIMATION_FREEZE | > > + MT9V111_PIXEL_ARRAY_HEIGHT); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RA7_HOUT, > > + MT9V111_IFP_DECIMATION_FREEZE | > > + mt9v111->fmt.width); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_write(c, MT9V111_R01_IFP, MT9V111_IFP_RAA_VOUT, > > + mt9v111->fmt.height); > > + if (ret) > > + return ret; > > + > > + /* Update blankings to match the programmed frame rate. */ > > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R05_HBLANK, > > + MT9V111_CORE_R05_MAX_HBLANK, > > + mt9v111->hblank->val); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_update(c, MT9V111_R01_CORE, MT9V111_CORE_R06_VBLANK, > > + MT9V111_CORE_R06_MAX_VBLANK, > > + mt9v111->vblank->val); > > + if (ret) > > + return ret; > > + > > + /* > > + * Set pixel integration time to the whole frame time. > > + * This value controls the the shutter delay when running with AE > > + * disabled. If longer than frame time, it affects the output > > + * frame rate. > > + */ > > + ret = mt9v111_write(c, MT9V111_R01_CORE, MT9V111_CORE_R09_PIXEL_INT, > > + MT9V111_PIXEL_ARRAY_HEIGHT); > > return mt9v111_write(...); > > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +/* --- V4L2 subdev operations --- */ > > + > > +static int mt9v111_s_power(struct v4l2_subdev *sd, int on) > > Using runtime PM would be nicer. See e.g. the smiapp driver for an example. > > This could be done as a separate change later on as well. I might post-pone this. > > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + int pwr_count; > > + int ret = 0; > > + > > + mutex_lock(&mt9v111->pwr_mutex); > > + > > + /* > > + * Make sure we're transitioning from 0 to 1, or viceversa, only > > + * before actually changing the power state. > > + */ > > + pwr_count = mt9v111->pwr_count; > > + pwr_count += on ? 1 : -1; > > + if (pwr_count == !!on) { > > + ret = on ? __mt9v111_power_on(sd) : > > + __mt9v111_power_off(sd); > > + if (!ret) > > + /* All went well, updated power counter. */ > > + mt9v111->pwr_count = pwr_count; > > + > > + mutex_unlock(&mt9v111->pwr_mutex); > > + > > + return ret; > > + } > > + > > + /* > > + * Update power counter to keep track of how many nested calls we > > + * received. > > + */ > > + WARN_ON(pwr_count < 0 || pwr_count > 1); > > + mt9v111->pwr_count = pwr_count; > > + > > + mutex_unlock(&mt9v111->pwr_mutex); > > + > > + return ret; > > +} > > + > > +static int mt9v111_s_stream(struct v4l2_subdev *subdev, int enable) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > > + int ret; > > + > > + mutex_lock(&mt9v111->stream_mutex); > > + > > + if (mt9v111->streaming == enable) { > > + mutex_unlock(&mt9v111->stream_mutex); > > + return 0; > > + } > > + > > + ret = mt9v111_s_power(subdev, enable); > > + if (ret) > > + goto error_unlock; > > + > > + if (enable && mt9v111->pending) { > > + ret = mt9v111_hw_config(mt9v111); > > + if (ret) > > + goto error_unlock; > > + > > + /* > > + * No need to update control here as far as only H/VBLANK are > > + * supported and immediately programmed to registers in .s_ctrl > > + */ > > + > > + mt9v111->pending = false; > > + } > > + > > + mt9v111->streaming = enable ? true : false; > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + return 0; > > + > > +error_unlock: > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + return ret; > > +} > > + > > +static int mt9v111_s_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_frame_interval *ival) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + struct v4l2_fract *tpf = &ival->interval; > > + unsigned int fps = tpf->numerator ? > > + tpf->denominator / tpf->numerator : > > + tpf->denominator; > > + unsigned int max_fps; > > + > > + mutex_lock(&mt9v111->stream_mutex); > > + if (mt9v111->streaming) { > > + mutex_unlock(&mt9v111->stream_mutex); > > + return -EBUSY; > > + } > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + if (mt9v111->fps == fps) > > + return 0; > > + > > + /* Make sure frame rate/image sizes constraints are respected. */ > > + if (mt9v111->fmt.width < QVGA_WIDTH && > > + mt9v111->fmt.height < QVGA_HEIGHT) > > + max_fps = 90; > > + else if (mt9v111->fmt.width < CIF_WIDTH && > > + mt9v111->fmt.height < CIF_HEIGHT) > > + max_fps = 60; > > + else > > + max_fps = mt9v111->sysclk < > > + DIV_ROUND_CLOSEST(MT9V111_MAX_CLKIN, 2) ? 15 : > > + 30; > > + > > + if (fps > max_fps) > > + return -EINVAL; > > + > > + mt9v111_calc_frame_rate(mt9v111, tpf); > > + > > + mt9v111->fps = fps; > > + mt9v111->pending = true; > > I think you'd need to keep the mutex until here. > > > + > > + return 0; > > +} > > + > > +static int mt9v111_g_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_frame_interval *ival) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + struct v4l2_fract *tpf = &ival->interval; > > + > > + tpf->numerator = 1; > > + tpf->denominator = mt9v111->fps; > > Same here --- access to fps is serialised by the mutex. > > > + > > + return 0; > > +} > > + > > +static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format( > > + struct mt9v111_dev *mt9v111, > > + struct v4l2_subdev_pad_config *cfg, > > + unsigned int pad, > > + enum v4l2_subdev_format_whence which) > > +{ > > + switch (which) { > > + case V4L2_SUBDEV_FORMAT_TRY: > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + return v4l2_subdev_get_try_format(&mt9v111->sd, cfg, pad); > > +#else > > + return &cfg->try_fmt; > > +#endif > > + case V4L2_SUBDEV_FORMAT_ACTIVE: > > + return &mt9v111->fmt; > > + default: > > + return NULL; > > + } > > +} > > + > > +static int mt9v111_enum_mbus_code(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + if (code->pad || code->index > ARRAY_SIZE(mt9v111_formats) - 1) > > + return -EINVAL; > > + > > + code->code = mt9v111_formats[code->index].code; > > + > > + return 0; > > +} > > + > > +static int mt9v111_enum_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_interval_enum *fie) > > +{ > > + unsigned int i; > > + > > + if (fie->pad || fie->index >= ARRAY_SIZE(mt9v111_frame_intervals)) > > + return -EINVAL; > > + > > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) > > + if (fie->width == mt9v111_frame_sizes[i].width && > > + fie->height == mt9v111_frame_sizes[i].height) > > + break; > > + > > + if (i == ARRAY_SIZE(mt9v111_frame_sizes)) > > + return -EINVAL; > > + > > + fie->interval.numerator = 1; > > + fie->interval.denominator = mt9v111_frame_intervals[fie->index]; > > + > > + return 0; > > +} > > + > > +static int mt9v111_enum_frame_size(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + if (fse->pad || fse->index > ARRAY_SIZE(mt9v111_frame_sizes)) > > + return -EINVAL; > > + > > + fse->min_width = mt9v111_frame_sizes[fse->index].width; > > + fse->max_width = mt9v111_frame_sizes[fse->index].width; > > + fse->min_height = mt9v111_frame_sizes[fse->index].height; > > + fse->max_height = mt9v111_frame_sizes[fse->index].height; > > + > > + return 0; > > +} > > + > > +static int mt9v111_get_format(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *format) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > > + > > + if (format->pad) > > + return -EINVAL; > > + > > + format->format = *__mt9v111_get_pad_format(mt9v111, cfg, format->pad, > > + format->which); > > Ditto. > > > + return 0; > > +} > > + > > +static int mt9v111_set_format(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *format) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(subdev); > > + struct v4l2_mbus_framefmt new_fmt; > > + struct v4l2_mbus_framefmt *__fmt; > > + unsigned int best_fit = ~0L; > > + unsigned int idx; > > + unsigned int i; > > + > > + mutex_lock(&mt9v111->stream_mutex); > > + if (mt9v111->streaming) { > > + mutex_unlock(&mt9v111->stream_mutex); > > + return -EBUSY; > > + } > > + mutex_unlock(&mt9v111->stream_mutex); > > + > > + if (format->pad) > > + return -EINVAL; > > + > > + /* Update mbus format code and sizes. */ > > + for (i = 0; i < ARRAY_SIZE(mt9v111_formats); i++) { > > + if (format->format.code == mt9v111_formats[i].code) { > > + new_fmt.code = mt9v111_formats[i].code; > > + break; > > + } > > + } > > + if (i == ARRAY_SIZE(mt9v111_formats)) > > + new_fmt.code = mt9v111_formats[0].code; > > + > > + for (i = 0; i < ARRAY_SIZE(mt9v111_frame_sizes); i++) { > > + unsigned int fit = abs(mt9v111_frame_sizes[i].width - > > + format->format.width) + > > + abs(mt9v111_frame_sizes[i].height - > > + format->format.height); > > + if (fit < best_fit) { > > + best_fit = fit; > > + idx = i; > > + > > + if (fit == 0) > > + break; > > + } > > + } > > + new_fmt.width = mt9v111_frame_sizes[idx].width; > > + new_fmt.height = mt9v111_frame_sizes[idx].height; > > + > > + /* Update the device (or pad) format if it has changed. */ > > + __fmt = __mt9v111_get_pad_format(mt9v111, cfg, format->pad, > > + format->which); > > + > > + /* Format hasn't changed, stop here. */ > > + if (__fmt->code == new_fmt.code && > > + __fmt->width == new_fmt.width && > > + __fmt->height == new_fmt.height) > > + goto done; > > + > > + /* Update the format and sizes, then mark changes as pending. */ > > + __fmt->code = new_fmt.code; > > + __fmt->width = new_fmt.width; > > + __fmt->height = new_fmt.height; > > + > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > + mt9v111->pending = true; > > + > > + dev_info(mt9v111->dev, "%s: mbus_code: %x - (%ux%u)\n", > > + __func__, __fmt->code, __fmt->width, __fmt->height); > > + > > +done: > > + format->format = *__fmt; > > Same here. Ok, I've protected with mutexes on the "is_streaming?" condition, not the values the set/get functions are accessing (fps, formats and so on). I will extend the mutex coverage to the end of each function. > > > + > > + return 0; > > +} > > + > > +static const struct v4l2_subdev_core_ops mt9v111_core_ops = { > > + .s_power = mt9v111_s_power, > > +}; > > + > > +static const struct v4l2_subdev_video_ops mt9v111_video_ops = { > > + .s_stream = mt9v111_s_stream, > > + .s_frame_interval = mt9v111_s_frame_interval, > > + .g_frame_interval = mt9v111_g_frame_interval, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops mt9v111_pad_ops = { > > + .enum_mbus_code = mt9v111_enum_mbus_code, > > + .enum_frame_size = mt9v111_enum_frame_size, > > + .enum_frame_interval = mt9v111_enum_frame_interval, > > + .get_fmt = mt9v111_get_format, > > + .set_fmt = mt9v111_set_format, > > +}; > > + > > +static const struct v4l2_subdev_ops mt9v111_ops = { > > + .core = &mt9v111_core_ops, > > + .video = &mt9v111_video_ops, > > + .pad = &mt9v111_pad_ops, > > +}; > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > +static const struct media_entity_operations mt9v111_subdev_entity_ops = { > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > +static int mt9v111_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > > +{ > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + struct v4l2_mbus_framefmt *__fmt = > > + v4l2_subdev_get_try_format(sd, fh->pad, 0); > > + > > + /* Initialize try format: copy the device format. */ > > + *__fmt = mt9v111->fmt; > > Please use default configuration, not the current configuration. If this is > all you need in open, I'd suggest to implement the init_cfg pad op instead. > Didn't know about that. I'll look into it. > > + > > + return 0; > > +} > > + > > +static const struct v4l2_subdev_internal_ops mt9v111_internal_ops = { > > + .open = mt9v111_open, > > +}; > > +#endif > > + > > +/* --- V4L2 ctrl --- */ > > +static int mt9v111_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct mt9v111_dev *mt9v111 = container_of(ctrl->handler, > > + struct mt9v111_dev, > > + ctrls); > > + int ret = 0; > > -EINVAL? That way any controls not handled by the function won't be missed. > > > + > > + mutex_lock(&mt9v111->pwr_mutex); > > + /* > > + * If sensor is powered down, just cache new control values, > > + * no actual register access. > > + */ > > + if (!mt9v111->pwr_count) { > > + mutex_unlock(&mt9v111->pwr_mutex); > > + return 0; > > + } > > + mutex_unlock(&mt9v111->pwr_mutex); > > + > > + switch (ctrl->id) { > > + case V4L2_CID_HBLANK: > > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > > + MT9V111_CORE_R05_HBLANK, > > + MT9V111_CORE_R05_MAX_HBLANK, > > + mt9v111->hblank->val); > > + break; > > + case V4L2_CID_VBLANK: > > + ret = mt9v111_update(mt9v111->client, MT9V111_R01_CORE, > > + MT9V111_CORE_R06_VBLANK, > > + MT9V111_CORE_R06_MAX_VBLANK, > > + mt9v111->vblank->val); > > + break; > > + } > > + > > + return ret; > > +} > > + > > +static const struct v4l2_ctrl_ops mt9v111_ctrl_ops = { > > + .s_ctrl = mt9v111_s_ctrl, > > +}; > > + > > +static int mt9v111_chip_probe(struct mt9v111_dev *mt9v111) > > +{ > > + int ret; > > + u16 val; > > + > > + ret = __mt9v111_power_on(&mt9v111->sd); > > + if (ret) > > + return ret; > > + > > + ret = mt9v111_read(mt9v111->client, MT9V111_R01_CORE, > > + MT9V111_CORE_RFF_CHIP_VER, &val); > > + if (ret) > > + return ret; > > Power off the device on failure? Same below. > > > + > > + if ((val >> 8) != MT9V111_CHIP_ID_HIGH && > > + (val & 0xff) != MT9V111_CHIP_ID_LOW) { > > + dev_err(mt9v111->dev, > > + "Unable to identify MT9V111 chip: 0x%2x%2x\n", > > + val >> 8, val & 0xff); > > + return ret; > > + } > > + > > + ret = __mt9v111_power_off(&mt9v111->sd); > > + if (ret) > > + return ret; > > + > > + dev_info(mt9v111->dev, "Chip identified: 0x%2x%2x\n", > > + val >> 8, val & 0xff); > > + > > + return 0; > > +} > > + > > +static int mt9v111_probe(struct i2c_client *client) > > +{ > > + struct mt9v111_dev *mt9v111; > > + struct v4l2_fract tpf; > > + int ret; > > + > > + mt9v111 = devm_kzalloc(&client->dev, sizeof(*mt9v111), GFP_KERNEL); > > + if (!mt9v111) > > + return -ENOMEM; > > + > > + mt9v111->dev = &client->dev; > > + mt9v111->client = client; > > + > > + mt9v111->clk = devm_clk_get(&client->dev, NULL); > > + if (IS_ERR(mt9v111->clk)) > > + return PTR_ERR(mt9v111->clk); > > + > > + mt9v111->sysclk = clk_get_rate(mt9v111->clk); > > + if (mt9v111->sysclk > MT9V111_MAX_CLKIN) > > + return -EINVAL; > > + > > + mt9v111->oe = devm_gpiod_get_optional(&client->dev, "enable", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(mt9v111->oe)) { > > + dev_err(&client->dev, "Unable to get GPIO \"enable\": %ld\n", > > + PTR_ERR(mt9v111->oe)); > > + return PTR_ERR(mt9v111->oe); > > + } > > + > > + mt9v111->standby = devm_gpiod_get_optional(&client->dev, "standby", > > + GPIOD_OUT_HIGH); > > + if (IS_ERR(mt9v111->standby)) { > > + dev_err(&client->dev, "Unable to get GPIO \"standby\": %ld\n", > > + PTR_ERR(mt9v111->standby)); > > + return PTR_ERR(mt9v111->standby); > > + } > > + > > + mt9v111->reset = devm_gpiod_get_optional(&client->dev, "reset", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(mt9v111->reset)) { > > + dev_err(&client->dev, "Unable to get GPIO \"reset\": %ld\n", > > + PTR_ERR(mt9v111->reset)); > > + return PTR_ERR(mt9v111->reset); > > + } > > + > > + mutex_init(&mt9v111->pwr_mutex); > > + mutex_init(&mt9v111->stream_mutex); > > + spin_lock_init(&mt9v111->addr_lock); > > + > > + v4l2_ctrl_handler_init(&mt9v111->ctrls, 3); > > + > > + mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > > + V4L2_CID_HBLANK, > > + MT9V111_CORE_R05_MIN_HBLANK, > > + MT9V111_CORE_R05_MAX_HBLANK, 1, > > + MT9V111_CORE_R05_DEF_HBLANK); > > + if (IS_ERR_OR_NULL(mt9v111->hblank)) { > > + ret = PTR_ERR(mt9v111->hblank); > > + goto error_free_ctrls; > > + } > > + > > + mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > > + V4L2_CID_VBLANK, > > + MT9V111_CORE_R06_MIN_VBLANK, > > + MT9V111_CORE_R06_MAX_VBLANK, 1, > > + MT9V111_CORE_R06_DEF_VBLANK); > > + if (IS_ERR_OR_NULL(mt9v111->vblank)) { > > + ret = PTR_ERR(mt9v111->vblank); > > + goto error_free_ctrls; > > + } > > + > > + /* PIXEL_RATE is fixed: just expose it to user space. */ > > + v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops, > > + V4L2_CID_PIXEL_RATE, 0, > > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2), 1, > > + DIV_ROUND_CLOSEST(mt9v111->sysclk, 2)); > > + > > + mt9v111->sd.ctrl_handler = &mt9v111->ctrls; > > + > > + /* Start with default configuration: 640x480 UYVY. */ > > + mt9v111->fmt.width = 640; > > + mt9v111->fmt.height = 480; > > + mt9v111->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; > > + > > + /* These are fixed for all supported formats. */ > > + mt9v111->fmt.field = V4L2_FIELD_NONE; > > + mt9v111->fmt.colorspace = V4L2_COLORSPACE_SRGB; > > + mt9v111->fmt.ycbcr_enc = V4L2_YCBCR_ENC_601; > > + mt9v111->fmt.quantization = V4L2_QUANTIZATION_LIM_RANGE; > > + mt9v111->fmt.xfer_func = V4L2_XFER_FUNC_SRGB; > > + > > + /* Re-calculate blankings for 640x480@15fps. */ > > + mt9v111->fps = 15; > > + tpf.numerator = 1; > > + tpf.denominator = mt9v111->fps; > > + ret = mt9v111_calc_frame_rate(mt9v111, &tpf); > > + if (ret) > > + goto error_free_ctrls; > > + > > + mt9v111->pwr_count = 0; > > + mt9v111->addr_space = MT9V111_R01_IFP; > > + mt9v111->pending = true; > > + > > + v4l2_i2c_subdev_init(&mt9v111->sd, client, &mt9v111_ops); > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + mt9v111->sd.internal_ops = &mt9v111_internal_ops; > > + mt9v111->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + mt9v111->sd.entity.ops = &mt9v111_subdev_entity_ops; > > + mt9v111->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + > > + mt9v111->pad.flags = MEDIA_PAD_FL_SOURCE; > > + ret = media_entity_pads_init(&mt9v111->sd.entity, 1, &mt9v111->pad); > > + if (ret) > > + goto error_free_ctrls; > > +#endif > > + > > + ret = mt9v111_chip_probe(mt9v111); > > + if (ret) > > + goto error_free_ctrls; > > + > > + ret = v4l2_async_register_subdev(&mt9v111->sd); > > + if (ret) > > + goto error_free_ctrls; > > + > > + return 0; > > + > > +error_free_ctrls: > > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + media_entity_cleanup(&mt9v111->sd.entity); > > +#endif > > + > > + mutex_destroy(&mt9v111->pwr_mutex); > > + mutex_destroy(&mt9v111->stream_mutex); > > + > > + return ret; > > +} > > + > > +static int mt9v111_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct mt9v111_dev *mt9v111 = sd_to_mt9v111(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + > > + v4l2_ctrl_handler_free(&mt9v111->ctrls); > > + > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + media_entity_cleanup(&sd->entity); > > +#endif > > + > > + mutex_destroy(&mt9v111->pwr_mutex); > > + mutex_destroy(&mt9v111->stream_mutex); > > + > > + devm_gpiod_put(mt9v111->dev, mt9v111->oe); > > + devm_gpiod_put(mt9v111->dev, mt9v111->standby); > > + devm_gpiod_put(mt9v111->dev, mt9v111->reset); > > + > > + devm_clk_put(mt9v111->dev, mt9v111->clk); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id mt9v111_of_match[] = { > > + { .compatible = "aptina,mt9v111", }, > > + { /* sentinel */ }, > > +}; > > + > > +static struct i2c_driver mt9v111_driver = { > > + .driver = { > > + .name = "mt9v111", > > + .of_match_table = of_match_ptr(mt9v111_of_match), > > Please assign this without of_match_ptr(). > Ack to all the above comments. I'll send v2 soon. Thanks j > > + }, > > + .probe_new = mt9v111_probe, > > + .remove = mt9v111_remove, > > +}; > > + > > +module_i2c_driver(mt9v111_driver); > > + > > +MODULE_DESCRIPTION("V4L2 sensor driver for Aptina MT9V111"); > > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>"); > > +MODULE_LICENSE("GPL v2"); > > -- > Kind regards, > > Sakari Ailus > e-mail: sakari.ailus@iki.fi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 2018-07-24 13:08 ` jacopo mondi @ 2018-07-24 13:23 ` Sakari Ailus 0 siblings, 0 replies; 12+ messages in thread From: Sakari Ailus @ 2018-07-24 13:23 UTC (permalink / raw) To: jacopo mondi Cc: Jacopo Mondi, mchehab, linux-media, linux-kernel, linux-renesas-soc Hi Jacopo, On Tue, Jul 24, 2018 at 03:08:45PM +0200, jacopo mondi wrote: > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > > index 341452f..0bd867d 100644 > > > --- a/drivers/media/i2c/Kconfig > > > +++ b/drivers/media/i2c/Kconfig > > > @@ -841,6 +841,18 @@ config VIDEO_MT9V032 > > > This is a Video4Linux2 sensor-level driver for the Micron > > > MT9V032 752x480 CMOS sensor. > > > > > > +config VIDEO_MT9V111 > > > + tristate "Aptina MT9V111 sensor support" > > > + depends on I2C && VIDEO_V4L2 > > > + depends on MEDIA_CAMERA_SUPPORT > > > + depends on OF > > > > Why OF? I see no effective OF dependencies in the driver. > > > > Isn't the driver probing through OF? It may, but the driver is otherwise not technically limited to OF. ACPI could be used as well, for instance. > > > > + help > > > + This is a Video4Linux2 sensor-level driver for the Aptina/Micron > > I'll change sensor-level to sensor, has you've been doing lately on > all other Kconfig entries Ack. -- Sakari Ailus e-mail: sakari.ailus@iki.fi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] media: i2c: mt9v111 sensor driver 2018-06-11 15:17 [PATCH 0/2] media: i2c: mt9v111 sensor driver Jacopo Mondi 2018-06-11 15:17 ` [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings Jacopo Mondi 2018-06-11 15:17 ` [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 Jacopo Mondi @ 2018-07-04 9:01 ` jacopo mondi 2 siblings, 0 replies; 12+ messages in thread From: jacopo mondi @ 2018-07-04 9:01 UTC (permalink / raw) To: Jacopo Mondi; +Cc: mchehab, linux-media, linux-kernel, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 1378 bytes --] Hello linux-media list, On Mon, Jun 11, 2018 at 05:17:31PM +0200, Jacopo Mondi wrote: > Hello, > this is a sensor level driver for Aptina MT9V111. > > The driver supports YUYV_2X8 output formats and VGA,QVGA,QQVGA,CIF and QCIF > resolution. > > The driver allows control of frame rate through s_frame_interval or > V4L2_CID_H/VBLANK control. In order to allow manual frame control, the chip > is initialized with auto-exposure control, auto white balancing and flickering > control disabled. > > Tested VGA, QVGA QQVGA resolutions at 5, 10, 15 and 30 frame per second. Gentle ping for this driver, as I have now sent a patch to add a user for it https://patchwork.kernel.org/patch/10505175/ Thanks j > > Thanks > j > > Jacopo Mondi (2): > dt-bindings: media: i2c: Document MT9V111 bindings > media: i2c: Add driver for Aptina MT9V111 > > .../bindings/media/i2c/aptina,mt9v111.txt | 46 + > MAINTAINERS | 8 + > drivers/media/i2c/Kconfig | 12 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/mt9v111.c | 1297 ++++++++++++++++++++ > 5 files changed, 1364 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.txt > create mode 100644 drivers/media/i2c/mt9v111.c > > -- > 2.7.4 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-07-24 13:23 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-11 15:17 [PATCH 0/2] media: i2c: mt9v111 sensor driver Jacopo Mondi 2018-06-11 15:17 ` [PATCH 1/2] dt-bindings: media: i2c: Document MT9V111 bindings Jacopo Mondi 2018-06-12 22:11 ` Rob Herring 2018-07-23 14:29 ` Sakari Ailus 2018-06-11 15:17 ` [PATCH 2/2] media: i2c: Add driver for Aptina MT9V111 Jacopo Mondi 2018-06-11 16:44 ` kbuild test robot 2018-07-17 9:48 ` Kieran Bingham 2018-07-23 14:08 ` jacopo mondi 2018-07-23 14:28 ` Sakari Ailus 2018-07-24 13:08 ` jacopo mondi 2018-07-24 13:23 ` Sakari Ailus 2018-07-04 9:01 ` [PATCH 0/2] media: i2c: mt9v111 sensor driver jacopo mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).