* [V3, 0/2] media: Add support for OV02A10 sensor @ 2019-08-19 3:43 dongchun.zhu 2019-08-19 3:43 ` [V3, 1/2] media: dt-bindings: media: i2c: Add bindings for OV02A10 dongchun.zhu 2019-08-19 3:43 ` [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver dongchun.zhu 0 siblings, 2 replies; 16+ messages in thread From: dongchun.zhu @ 2019-08-19 3:43 UTC (permalink / raw) To: mchehab, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao Cc: devicetree, srv_heupstream, shengnan.wang, sj.huang, linux-mediatek, dongchun.zhu, louis.kuo, linux-arm-kernel, linux-media From: Dongchun Zhu <dongchun.zhu@mediatek.com> Hello, This patch adds driver and bindings for Omnivision's OV02A10 2 megapixel CMOS 1/5" sensor, which has a single MIPI lane interface and output format of 10-bit Raw. The driver is implemented wth V4L2 framework. 1. Async registered as a V4L2 I2C sub-device. 2. A media entity that can provide several source pads and sink pads to link with other device like Seninf, ISP one after another to create a default overall camera topology, image frame or meta-data from sensor can flow through particular path to output preview or capture image or 3A info. Changes of v3 are mainly addressing comments from Rob, Sakari, Bingbu. - Fix coding style errors in dt-bindings - Use macro flag to describle basic line 1224 when updating v-blanking - Remove unnecessary debug log in driver Mainly changes of v2 are addressing the comments from Nicolas, Bingbu, Sakari, Rob, including, - Put dt binding before driver in series - Add MAINTAINERS entries - Squash the MAINTAINERS entry and Kconfig to driver patch - Add rotation support for driver - Fix other reviewed issues in v1 Dongchun Zhu (2): media: dt-bindings: media: i2c: Add bindings for OV02A10 media: i2c: Add Omnivision OV02A10 camera sensor driver .../devicetree/bindings/media/i2c/ov02a10.txt | 54 ++ MAINTAINERS | 8 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov02a10.c | 1018 ++++++++++++++++++++ 5 files changed, 1092 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt create mode 100644 drivers/media/i2c/ov02a10.c -- 2.9.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [V3, 1/2] media: dt-bindings: media: i2c: Add bindings for OV02A10 2019-08-19 3:43 [V3, 0/2] media: Add support for OV02A10 sensor dongchun.zhu @ 2019-08-19 3:43 ` dongchun.zhu 2019-08-27 16:48 ` Rob Herring 2019-08-19 3:43 ` [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver dongchun.zhu 1 sibling, 1 reply; 16+ messages in thread From: dongchun.zhu @ 2019-08-19 3:43 UTC (permalink / raw) To: mchehab, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao Cc: devicetree, srv_heupstream, shengnan.wang, sj.huang, linux-mediatek, dongchun.zhu, louis.kuo, linux-arm-kernel, linux-media From: Dongchun Zhu <dongchun.zhu@mediatek.com> Add device tree binding documentation for the OV02A10 camera sensor. Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> --- .../devicetree/bindings/media/i2c/ov02a10.txt | 54 ++++++++++++++++++++++ MAINTAINERS | 7 +++ 2 files changed, 61 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt new file mode 100644 index 0000000..4a83a38 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt @@ -0,0 +1,54 @@ +* Omnivision OV02A10 MIPI CSI-2 sensor + +Required Properties: +- compatible: shall be "ovti,ov02a10" +- clocks: reference to the xvclk input clock +- clock-names: shall be "xvclk" +- avdd-supply: Analog voltage supply, 2.8 volts +- dovdd-supply: Digital I/O voltage supply, 1.8 volts +- dvdd-supply: Digital core voltage supply, 1.8 volts +- reset-gpios: reference to the GPIO connected to the reset pin, if any. + This is an active high signal to the OV02A10. +- powerdown-gpios: reference to the GPIO connected to the powerdown pin, + if any. This is an active low signal to the OV02A10. + +Optional Properties: +- rotation: as defined in + Documentation/devicetree/bindings/media/video-interfaces.txt, + valid values are 0 (sensor mounted upright) and 180 (sensor + mounted upside down). + +The device node shall contain one 'port' child node with an +'endpoint' subnode for its digital output video port, +in accordance with the video interface bindings defined in +Documentation/devicetree/bindings/media/video-interfaces.txt. +The endpoint optional property 'data-lanes' shall be "<1>". + +Example: +&i2c4 { + ov02a10: camera-sensor@3d { + compatible = "ovti,ov02a10"; + reg = <0x3d>; + pinctrl-names = "default"; + pinctrl-0 = <&camera_pins_cam1_mclk_on>; + + clocks = <&topckgen CLK_TOP_MUX_CAMTG2>, + <&topckgen CLK_TOP_UNIVP_192M_D8>; + clock-names = "xvclk", "freq_mux"; + + avdd-supply = <&mt6358_vcama1_reg>; + dvdd-supply = <&mt6358_vcn18_reg>; + dovdd-supply = <&mt6358_vcamio_reg>; + reset-gpios = <&pio 109 1>; + powerdown-gpios = <&pio 107 1>; + rotation = <180>; + + port { + /* MIPI CSI-2 bus endpoint */ + ov02a10_core: endpoint { + remote-endpoint = <&ov02a10_0>; + data-lanes = <1>; + }; + }; + }; +}; diff --git a/MAINTAINERS b/MAINTAINERS index 783569e..41734fb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11816,6 +11816,13 @@ T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov13858.c +OMNIVISION OV02A10 SENSOR DRIVER +M: Dongchun Zhu <dongchun.zhu@mediatek.com> +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: Documentation/devicetree/bindings/media/i2c/ov02a10.txt + OMNIVISION OV2680 SENSOR DRIVER M: Rui Miguel Silva <rmfrfs@gmail.com> L: linux-media@vger.kernel.org -- 2.9.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [V3, 1/2] media: dt-bindings: media: i2c: Add bindings for OV02A10 2019-08-19 3:43 ` [V3, 1/2] media: dt-bindings: media: i2c: Add bindings for OV02A10 dongchun.zhu @ 2019-08-27 16:48 ` Rob Herring 0 siblings, 0 replies; 16+ messages in thread From: Rob Herring @ 2019-08-27 16:48 UTC (permalink / raw) To: dongchun.zhu Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang, tfiga, louis.kuo, sj.huang, robh+dt, linux-mediatek, dongchun.zhu, sakari.ailus, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media On Mon, 19 Aug 2019 11:43:30 +0800, <dongchun.zhu@mediatek.com> wrote: > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > Add device tree binding documentation for the OV02A10 camera sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > .../devicetree/bindings/media/i2c/ov02a10.txt | 54 ++++++++++++++++++++++ > MAINTAINERS | 7 +++ > 2 files changed, 61 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt > Reviewed-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-08-19 3:43 [V3, 0/2] media: Add support for OV02A10 sensor dongchun.zhu 2019-08-19 3:43 ` [V3, 1/2] media: dt-bindings: media: i2c: Add bindings for OV02A10 dongchun.zhu @ 2019-08-19 3:43 ` dongchun.zhu 2019-08-19 8:30 ` Sakari Ailus 2019-08-21 10:30 ` Tomasz Figa 1 sibling, 2 replies; 16+ messages in thread From: dongchun.zhu @ 2019-08-19 3:43 UTC (permalink / raw) To: mchehab, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao Cc: devicetree, srv_heupstream, shengnan.wang, sj.huang, linux-mediatek, dongchun.zhu, louis.kuo, linux-arm-kernel, linux-media From: Dongchun Zhu <dongchun.zhu@mediatek.com> This patch adds a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is a 1/5" CMOS sensor from Omnivision, which supports output format: 10-bit Raw. The OV02A10 has a single MIPI lane interface and use the I2C bus for control and the CSI-2 bus for data. Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> --- MAINTAINERS | 1 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov02a10.c | 1018 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1031 insertions(+) create mode 100644 drivers/media/i2c/ov02a10.c diff --git a/MAINTAINERS b/MAINTAINERS index 41734fb..4b714a2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11821,6 +11821,7 @@ M: Dongchun Zhu <dongchun.zhu@mediatek.com> L: linux-media@vger.kernel.org T: git git://linuxtv.org/media_tree.git S: Maintained +F: drivers/media/i2c/ov02a10.c F: Documentation/devicetree/bindings/media/i2c/ov02a10.txt OMNIVISION OV2680 SENSOR DRIVER diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 79ce9ec..d063a82 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -617,6 +617,17 @@ config VIDEO_IMX355 To compile this driver as a module, choose M here: the module will be called imx355. +config VIDEO_OV02A10 + tristate "OmniVision OV02A10 sensor support" + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + help + This is a Video4Linux2 sensor driver for the OmniVision + OV02A10 camera. + + To compile this driver as a module, choose M here: the + module will be called ov02a10. + config VIDEO_OV2640 tristate "OmniVision OV2640 sensor support" depends on VIDEO_V4L2 && I2C diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index fd4ea86..f34a7ac 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -71,6 +71,7 @@ obj-$(CONFIG_VIDEO_OV5645) += ov5645.o obj-$(CONFIG_VIDEO_OV5647) += ov5647.o obj-$(CONFIG_VIDEO_OV5670) += ov5670.o obj-$(CONFIG_VIDEO_OV5695) += ov5695.o +obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o obj-$(CONFIG_VIDEO_OV6650) += ov6650.o obj-$(CONFIG_VIDEO_OV7251) += ov7251.o obj-$(CONFIG_VIDEO_OV7640) += ov7640.o diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c new file mode 100644 index 0000000..ff5460a --- /dev/null +++ b/drivers/media/i2c/ov02a10.c @@ -0,0 +1,1018 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018 MediaTek Inc. + */ + +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> +#include <media/media-entity.h> +#include <media/v4l2-async.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-subdev.h> + +#define CHIP_ID 0x2509 +#define OV02A10_REG_CHIP_ID_H 0x02 +#define OV02A10_REG_CHIP_ID_L 0x03 +#define OV02A10_ID(_msb, _lsb) ((_msb) << 8 | (_lsb)) + +/* Bit[1] vertical upside down */ +/* Bit[0] horizontal mirror */ +#define REG_MIRROR_FLIP_CONTROL 0x3f + +/* Orientation */ +#define REG_CONFIG_MIRROR_FLIP 0x03 + +#define REG_PAGE_SWITCH 0xfd +#define REG_GLOBAL_EFFECTIVE 0x01 +#define REG_ENABLE BIT(0) + +#define REG_SC_CTRL_MODE 0xac +#define SC_CTRL_MODE_STANDBY 0x00 +#define SC_CTRL_MODE_STREAMING 0x01 + +#define OV02A10_REG_EXPOSURE_H 0x03 +#define OV02A10_REG_EXPOSURE_L 0x04 +#define OV02A10_EXPOSURE_MIN 4 +#define OV02A10_EXPOSURE_STEP 1 + +#define OV02A10_REG_VTS_H 0x05 +#define OV02A10_REG_VTS_L 0x06 +#define OV02A10_VTS_MAX 0x209f +#define OV02A10_VTS_MIN 0x04cf +#define OV02A10_BASIC_LINE 1224 + +#define OV02A10_REG_GAIN 0x24 +#define OV02A10_GAIN_MIN 0x10 +#define OV02A10_GAIN_MAX 0xf8 +#define OV02A10_GAIN_STEP 0x01 +#define OV02A10_GAIN_DEFAULT 0x40 + +#define REG_NULL 0xff + +#define OV02A10_LANES 1 +#define OV02A10_BITS_PER_SAMPLE 10 + +static const char * const ov02a10_supply_names[] = { + "dovdd", /* Digital I/O power */ + "avdd", /* Analog power */ + "dvdd", /* Digital core power */ +}; + +#define OV02A10_NUM_SUPPLIES ARRAY_SIZE(ov02a10_supply_names) + +struct regval { + u16 addr; + u8 val; +}; + +struct ov02a10_mode { + u32 width; + u32 height; + u32 exp_def; + u32 hts_def; + u32 vts_def; + const struct regval *reg_list; +}; + +struct ov02a10 { + struct clk *xvclk; + struct gpio_desc *powerdown_gpio; + struct gpio_desc *reset_gpio; + struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES]; + + bool streaming; + bool upside_down; + + /* + * Serialize control access, get/set format, get selection + * and start streaming. + */ + struct mutex mutex; + struct v4l2_subdev subdev; + struct media_pad pad; + struct v4l2_ctrl *anal_gain; + struct v4l2_ctrl *exposure; + struct v4l2_ctrl *hblank; + struct v4l2_ctrl *vblank; + struct v4l2_ctrl *hflip; + struct v4l2_ctrl *vflip; + struct v4l2_ctrl *test_pattern; + struct v4l2_mbus_framefmt fmt; + struct v4l2_ctrl_handler ctrl_handler; + + const struct ov02a10_mode *cur_mode; +}; + +#define to_ov02a10(sd) container_of(sd, struct ov02a10, subdev) + +static inline void msleep_range(unsigned int delay_base) +{ + usleep_range(delay_base * 1000, delay_base * 1000 + 500); +} + +/* MIPI color bar enable output */ +static const struct regval ov02a10_test_pattern_enable_regs[] = { + {0xfd, 0x01}, + {0x0d, 0x00}, + {0xb6, 0x01}, + {0x01, 0x01}, + {0xfd, 0x01}, + {0xac, 0x01}, + {REG_NULL, 0x00} +}; + +/* MIPI color bar disable output */ +static const struct regval ov02a10_test_pattern_disable_regs[] = { + {0xfd, 0x01}, + {0x0d, 0x00}, + {0xb6, 0x00}, + {0x01, 0x01}, + {0xfd, 0x01}, + {0xac, 0x01}, + {REG_NULL, 0x00} +}; + +/* + * xvclk 24Mhz + * pclk 39Mhz + * linelength 934(0x3a6) + * framelength 1390(0x56e) + * grabwindow_width 1600 + * grabwindow_height 1200 + * max_framerate 30fps + * mipi_datarate per lane 780Mbps + */ +static const struct regval ov02a10_1600x1200_regs[] = { + {0xfd, 0x01}, + {0xac, 0x00}, + {0xfd, 0x00}, + {0x2f, 0x29}, + {0x34, 0x00}, + {0x35, 0x21}, + {0x30, 0x15}, + {0x33, 0x01}, + {0xfd, 0x01}, + {0x44, 0x00}, + {0x2a, 0x4c}, + {0x2b, 0x1e}, + {0x2c, 0x60}, + {0x25, 0x11}, + {0x03, 0x01}, + {0x04, 0xae}, + {0x09, 0x00}, + {0x0a, 0x02}, + {0x06, 0xa6}, + {0x31, 0x00}, + {0x24, 0x40}, + {0x01, 0x01}, + {0xfb, 0x73}, + {0xfd, 0x01}, + {0x16, 0x04}, + {0x1c, 0x09}, + {0x21, 0x42}, + {0x12, 0x04}, + {0x13, 0x10}, + {0x11, 0x40}, + {0x33, 0x81}, + {0xd0, 0x00}, + {0xd1, 0x01}, + {0xd2, 0x00}, + {0x50, 0x10}, + {0x51, 0x23}, + {0x52, 0x20}, + {0x53, 0x10}, + {0x54, 0x02}, + {0x55, 0x20}, + {0x56, 0x02}, + {0x58, 0x48}, + {0x5d, 0x15}, + {0x5e, 0x05}, + {0x66, 0x66}, + {0x68, 0x68}, + {0x6b, 0x00}, + {0x6c, 0x00}, + {0x6f, 0x40}, + {0x70, 0x40}, + {0x71, 0x0a}, + {0x72, 0xf0}, + {0x73, 0x10}, + {0x75, 0x80}, + {0x76, 0x10}, + {0x84, 0x00}, + {0x85, 0x10}, + {0x86, 0x10}, + {0x87, 0x00}, + {0x8a, 0x22}, + {0x8b, 0x22}, + {0x19, 0xf1}, + {0x29, 0x01}, + {0xfd, 0x01}, + {0x9d, 0xd6}, + {0xa0, 0x29}, + {0xa1, 0x03}, + {0xad, 0x62}, + {0xae, 0x00}, + {0xaf, 0x85}, + {0xb1, 0x01}, + {0x8e, 0x06}, + {0x8f, 0x40}, + {0x90, 0x04}, + {0x91, 0xb0}, + {0x45, 0x01}, + {0x46, 0x00}, + {0x47, 0x6c}, + {0x48, 0x03}, + {0x49, 0x8b}, + {0x4a, 0x00}, + {0x4b, 0x07}, + {0x4c, 0x04}, + {0x4d, 0xb7}, + {0xf0, 0x40}, + {0xf1, 0x40}, + {0xf2, 0x40}, + {0xf3, 0x40}, + {0x3f, 0x00}, + {0xfd, 0x01}, + {0x05, 0x00}, + {0x06, 0xa6}, + {0xfd, 0x01}, + {REG_NULL, 0x00} +}; + +#define OV02A10_LINK_FREQ_390MHZ 390000000 +static const s64 link_freq_menu_items[] = { + OV02A10_LINK_FREQ_390MHZ +}; + +static const char * const ov02a10_test_pattern_menu[] = { + "Disabled", + "Color Bar", +}; + +static const struct ov02a10_mode supported_modes[] = { + { + .width = 1600, + .height = 1200, + .exp_def = 0x01ae, + .hts_def = 0x03a6, + .vts_def = 0x056e, + .reg_list = ov02a10_1600x1200_regs, + }, +}; + +/* Write a register */ +static int ov02a10_write_reg(struct ov02a10 *ov02a10, u8 addr, u8 val) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + u8 buf[2] = {addr, val}; + int ret; + + ret = i2c_master_send(client, buf, 2); + + if (ret != 2) { + dev_err(&client->dev, "%s: error: reg=%x, val=%x\n", + __func__, addr, val); + return -EIO; + } + + return 0; +} + +static int ov02a10_write_array(struct ov02a10 *ov02a10, + const struct regval *regs) +{ + u32 i; + int ret; + + for (i = 0; regs[i].addr != REG_NULL; i++) { + ret = ov02a10_write_reg(ov02a10, regs[i].addr, regs[i].val); + if (ret < 0) + return ret; + } + + return 0; +} + +/* Read a register */ +static int ov02a10_read_reg(struct ov02a10 *ov02a10, u8 reg, u8 *val) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + u8 data = reg; + struct i2c_msg msg = { + .addr = client->addr, + .flags = 0, + .len = 1, + .buf = &data, + }; + int ret; + + ret = i2c_transfer(client->adapter, &msg, 1); + if (ret < 0) + goto err_wr; + + msg.flags = I2C_M_RD; + ret = i2c_transfer(client->adapter, &msg, 1); + if (ret < 0) + goto err_rd; + + *val = data; + return 0; + +err_rd: + dev_err(&client->dev, "i2c_transfer --I2C_M_RD failed\n"); +err_wr: + dev_err(&client->dev, "read error: reg=0x%02x: %d\n", reg, ret); + return ret; +} + +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode, + struct v4l2_mbus_framefmt *fmt) +{ + fmt->width = mode->width; + fmt->height = mode->height; + fmt->field = V4L2_FIELD_NONE; +} + +static int ov02a10_set_fmt(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt) +{ + struct ov02a10 *ov02a10 = to_ov02a10(sd); + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; + + mutex_lock(&ov02a10->mutex); + + if (ov02a10->streaming) { + mutex_unlock(&ov02a10->mutex); + return -EBUSY; + } + + /* Only one sensor mode supported */ + mbus_fmt->code = ov02a10->fmt.code; + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); + ov02a10->fmt = fmt->format; + + mutex_unlock(&ov02a10->mutex); + + return 0; +} + +static int ov02a10_get_fmt(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt) +{ + struct ov02a10 *ov02a10 = to_ov02a10(sd); + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; + + mutex_lock(&ov02a10->mutex); + + fmt->format = ov02a10->fmt; + mbus_fmt->code = ov02a10->fmt.code; + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); + + mutex_unlock(&ov02a10->mutex); + + return 0; +} + +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_mbus_code_enum *code) +{ + struct ov02a10 *ov02a10 = to_ov02a10(sd); + + if (code->index >= ARRAY_SIZE(supported_modes) || !(code->index)) + return -EINVAL; + + code->code = ov02a10->fmt.code; + + return 0; +} + +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_frame_size_enum *fse) +{ + if (fse->index >= ARRAY_SIZE(supported_modes) || !(fse->index)) + return -EINVAL; + + fse->min_width = supported_modes[fse->index].width; + fse->max_width = supported_modes[fse->index].width; + fse->max_height = supported_modes[fse->index].height; + fse->min_height = supported_modes[fse->index].height; + + return 0; +} + +static int __ov02a10_power_on(struct ov02a10 *ov02a10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + struct device *dev = &client->dev; + int ret; + + ret = clk_prepare_enable(ov02a10->xvclk); + if (ret < 0) { + dev_err(dev, "Failed to enable xvclk\n"); + return ret; + } + + /* Note: set 0 is high, set 1 is low */ + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 0); + + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); + if (ret < 0) { + dev_err(dev, "Failed to enable regulators\n"); + goto disable_clk; + } + msleep_range(7); + + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); + msleep_range(10); + + gpiod_set_value_cansleep(ov02a10->reset_gpio, 0); + msleep_range(10); + + return 0; + +disable_clk: + clk_disable_unprepare(ov02a10->xvclk); + + return ret; +} + +static void __ov02a10_power_off(struct ov02a10 *ov02a10) +{ + clk_disable_unprepare(ov02a10->xvclk); + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); +} + +static int __ov02a10_start_stream(struct ov02a10 *ov02a10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + int ret; + + /* Apply default values of current mode */ + ret = ov02a10_write_array(ov02a10, ov02a10->cur_mode->reg_list); + if (ret) + return ret; + + /* Apply customized values from user */ + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler); + if (ret) + return ret; + + /* Set orientation to 180 degree */ + if (ov02a10->upside_down) { + ret = ov02a10_write_reg(ov02a10, REG_MIRROR_FLIP_CONTROL, + REG_CONFIG_MIRROR_FLIP); + if (ret) { + dev_err(&client->dev, "%s failed to set orientation\n", + __func__); + return ret; + } + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, + REG_ENABLE); + if (ret < 0) + return ret; + } + + /* Set stream on register */ + return ov02a10_write_reg(ov02a10, + REG_SC_CTRL_MODE, SC_CTRL_MODE_STREAMING); +} + +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10) +{ + return ov02a10_write_reg(ov02a10, + REG_SC_CTRL_MODE, SC_CTRL_MODE_STANDBY); +} + +static int ov02a10_entity_init_cfg(struct v4l2_subdev *subdev, + struct v4l2_subdev_pad_config *cfg) +{ + struct v4l2_subdev_format fmt = { 0 }; + + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; + fmt.format.width = 1600; + fmt.format.height = 1200; + + ov02a10_set_fmt(subdev, cfg, &fmt); + + return 0; +} + +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) +{ + struct ov02a10 *ov02a10 = to_ov02a10(sd); + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + int ret = 0; + + mutex_lock(&ov02a10->mutex); + + if (ov02a10->streaming == on) + goto unlock_and_return; + + if (on) { + ret = pm_runtime_get_sync(&client->dev); + if (ret < 0) { + pm_runtime_put_noidle(&client->dev); + goto unlock_and_return; + } + + ret = __ov02a10_start_stream(ov02a10); + if (ret) { + __ov02a10_stop_stream(ov02a10); + ov02a10->streaming = !on; + goto err_rpm_put; + } + } else { + __ov02a10_stop_stream(ov02a10); + pm_runtime_put(&client->dev); + } + + ov02a10->streaming = on; + mutex_unlock(&ov02a10->mutex); + + return ret; + +err_rpm_put: + pm_runtime_put(&client->dev); +unlock_and_return: + mutex_unlock(&ov02a10->mutex); + + return ret; +} + +static int ov02a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) +{ + struct ov02a10 *ov02a10 = to_ov02a10(sd); + struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_get_try_format(sd, + fh->pad, + 0); + + mutex_lock(&ov02a10->mutex); + /* Initialize try_fmt */ + try_fmt->code = ov02a10->fmt.code; + ov02a10_fill_fmt(&supported_modes[0], try_fmt); + + mutex_unlock(&ov02a10->mutex); + + return 0; +} + +static int __maybe_unused ov02a10_runtime_resume(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ov02a10 *ov02a10 = to_ov02a10(sd); + + return __ov02a10_power_on(ov02a10); +} + +static int __maybe_unused ov02a10_runtime_suspend(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ov02a10 *ov02a10 = to_ov02a10(sd); + + __ov02a10_power_off(ov02a10); + + return 0; +} + +static const struct dev_pm_ops ov02a10_pm_ops = { + SET_RUNTIME_PM_OPS(ov02a10_runtime_suspend, + ov02a10_runtime_resume, NULL) +}; + +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, s32 value) +{ + if (value) + return ov02a10_write_array(ov02a10, + ov02a10_test_pattern_enable_regs); + + return ov02a10_write_array(ov02a10, + ov02a10_test_pattern_disable_regs); +} + +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct ov02a10 *ov02a10 = container_of(ctrl->handler, + struct ov02a10, ctrl_handler); + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + s64 max_expo; + int ret; + + /* Propagate change of current control to all related controls */ + if (ctrl->id == V4L2_CID_VBLANK) { + /* Update max exposure while meeting expected vblanking */ + max_expo = ov02a10->cur_mode->height + ctrl->val - 4; + __v4l2_ctrl_modify_range(ov02a10->exposure, + ov02a10->exposure->minimum, max_expo, + ov02a10->exposure->step, + ov02a10->exposure->default_value); + } + + /* V4L2 controls values will be applied only when power is already up */ + if (!pm_runtime_get_if_in_use(&client->dev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); + if (ret < 0) + return ret; + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_H, + ((ctrl->val >> 8) & 0xFF)); + if (!ret) { + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_L, + (ctrl->val & 0xFF)); + if (ret < 0) + return ret; + } + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, + REG_ENABLE); + if (ret < 0) + return ret; + break; + case V4L2_CID_ANALOGUE_GAIN: + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); + if (ret < 0) + return ret; + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_GAIN, + (ctrl->val & 0xFF)); + if (ret < 0) + return ret; + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, + REG_ENABLE); + if (ret < 0) + return ret; + break; + case V4L2_CID_VBLANK: + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); + if (ret < 0) + return ret; + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_H, + (((ctrl->val + + ov02a10->cur_mode->height - + OV02A10_BASIC_LINE) >> 8) + & 0xFF)); + if (!ret) { + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_L, + ((ctrl->val + + ov02a10->cur_mode->height - + OV02A10_BASIC_LINE) & 0xFF)); + if (ret < 0) + return ret; + } + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, + REG_ENABLE); + if (ret < 0) + return ret; + break; + case V4L2_CID_TEST_PATTERN: + ret = ov02a10_set_test_pattern(ov02a10, ctrl->val); + if (ret < 0) + return ret; + break; + default: + dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n", + __func__, ctrl->id, ctrl->val); + ret = -EINVAL; + break; + }; + + pm_runtime_put(&client->dev); + + return ret; +} + +static const struct v4l2_subdev_video_ops ov02a10_video_ops = { + .s_stream = ov02a10_s_stream, +}; + +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = { + .init_cfg = ov02a10_entity_init_cfg, + .enum_mbus_code = ov02a10_enum_mbus_code, + .enum_frame_size = ov02a10_enum_frame_sizes, + .get_fmt = ov02a10_get_fmt, + .set_fmt = ov02a10_set_fmt, +}; + +static const struct v4l2_subdev_ops ov02a10_subdev_ops = { + .video = &ov02a10_video_ops, + .pad = &ov02a10_pad_ops, +}; + +static const struct media_entity_operations ov02a10_subdev_entity_ops = { + .link_validate = v4l2_subdev_link_validate, +}; + +static const struct v4l2_subdev_internal_ops ov02a10_internal_ops = { + .open = ov02a10_open, +}; + +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = { + .s_ctrl = ov02a10_set_ctrl, +}; + +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + const struct ov02a10_mode *mode; + struct v4l2_ctrl_handler *handler; + struct v4l2_ctrl *ctrl; + u64 exposure_max; + u32 pixel_rate, h_blank; + int ret; + + handler = &ov02a10->ctrl_handler; + mode = ov02a10->cur_mode; + ret = v4l2_ctrl_handler_init(handler, 10); + if (ret) + return ret; + handler->lock = &ov02a10->mutex; + + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, + 0, 0, link_freq_menu_items); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + pixel_rate = (link_freq_menu_items[0] * 2 * OV02A10_LANES) / + OV02A10_BITS_PER_SAMPLE; + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, + 0, pixel_rate, 1, pixel_rate); + + h_blank = mode->hts_def - mode->width; + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK, + h_blank, h_blank, 1, h_blank); + if (ov02a10->hblank) + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, + V4L2_CID_VBLANK, mode->vts_def - + mode->height, + OV02A10_VTS_MAX - mode->height, 1, + mode->vts_def - mode->height); + + exposure_max = mode->vts_def - 4; + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, + V4L2_CID_EXPOSURE, + OV02A10_EXPOSURE_MIN, + exposure_max, + OV02A10_EXPOSURE_STEP, + mode->exp_def); + + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, + V4L2_CID_ANALOGUE_GAIN, + OV02A10_GAIN_MIN, + OV02A10_GAIN_MAX, + OV02A10_GAIN_STEP, + OV02A10_GAIN_DEFAULT); + + ov02a10->test_pattern = + v4l2_ctrl_new_std_menu_items(handler, + &ov02a10_ctrl_ops, + V4L2_CID_TEST_PATTERN, + ARRAY_SIZE(ov02a10_test_pattern_menu) - + 1, 0, 0, ov02a10_test_pattern_menu); + + if (handler->error) { + ret = handler->error; + dev_err(&client->dev, + "Failed to init controls(%d)\n", ret); + goto err_free_handler; + } + + ov02a10->subdev.ctrl_handler = handler; + + return 0; + +err_free_handler: + v4l2_ctrl_handler_free(handler); + + return ret; +} + +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + u16 id; + u8 pid = 0; + u8 ver = 0; + int ret; + + /* Check sensor revision */ + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_H, &pid); + if (ret) + return ret; + + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_L, &ver); + if (ret) + return ret; + + id = OV02A10_ID(pid, ver); + if (id != CHIP_ID) { + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id); + return ret; + } + + dev_info(&client->dev, "Detected OV%04X sensor\n", id); + + return 0; +} + +static int ov02a10_configure_regulators(struct ov02a10 *ov02a10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); + unsigned int i; + + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++) + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; + + return devm_regulator_bulk_get(&client->dev, + OV02A10_NUM_SUPPLIES, + ov02a10->supplies); +} + +static int ov02a10_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct ov02a10 *ov02a10; + u32 rotation; + u32 xclk_freq; + int ret; + + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); + if (!ov02a10) + return -ENOMEM; + + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; + + /* Optional indication of physical rotation of sensor */ + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", + &rotation); + if (!ret) { + switch (rotation) { + case 180: + ov02a10->upside_down = true; + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; + break; + case 0: + break; + default: + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", + rotation); + } + } + + /* Get system clock (xvclk) */ + ov02a10->xvclk = devm_clk_get(dev, "xvclk"); + if (IS_ERR(ov02a10->xvclk)) { + dev_err(dev, "Failed to get xvclk\n"); + return -EINVAL; + } + + ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); + if (ret) { + dev_err(dev, "Failed to get xclk frequency\n"); + return ret; + } + + /* External clock must be 24MHz, allow 1% tolerance */ + if (xclk_freq < 23760000 || xclk_freq > 24240000) { + dev_err(dev, "external clock frequency %u is not supported\n", + xclk_freq); + return -EINVAL; + } + dev_dbg(dev, "external clock frequency %u\n", xclk_freq); + + ret = clk_set_rate(ov02a10->xvclk, xclk_freq); + if (ret) { + dev_err(dev, "Failed to set xvclk frequency (24MHz)\n"); + return ret; + } + + ov02a10->powerdown_gpio = devm_gpiod_get(dev, "powerdown", + GPIOD_OUT_LOW); + if (IS_ERR(ov02a10->powerdown_gpio)) { + dev_err(dev, "Failed to get powerdown-gpios\n"); + return -EINVAL; + } + + ov02a10->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(ov02a10->reset_gpio)) { + dev_err(dev, "Failed to get reset-gpios\n"); + return -EINVAL; + } + + ret = ov02a10_configure_regulators(ov02a10); + if (ret) { + dev_err(dev, "Failed to get power regulators\n"); + return ret; + } + + mutex_init(&ov02a10->mutex); + ov02a10->cur_mode = &supported_modes[0]; + ret = ov02a10_initialize_controls(ov02a10); + if (ret) { + dev_err(dev, "Failed to initialize controls\n"); + goto err_destroy_mutex; + } + + ret = __ov02a10_power_on(ov02a10); + if (ret) + goto err_free_handler; + + ret = ov02a10_check_sensor_id(ov02a10); + if (ret) + goto err_power_off; + + ov02a10->subdev.internal_ops = &ov02a10_internal_ops; + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); + if (ret < 0) { + dev_err(dev, "failed to init entity pads: %d", ret); + goto err_power_off; + } + + ret = v4l2_async_register_subdev(&ov02a10->subdev); + if (ret) { + dev_err(dev, "failed to register V4L2 subdev: %d", + ret); + goto err_clean_entity; + } + + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + pm_runtime_idle(dev); + + dev_info(dev, "ov02a10 probe --\n"); + return 0; + +err_clean_entity: + media_entity_cleanup(&ov02a10->subdev.entity); +err_power_off: + __ov02a10_power_off(ov02a10); +err_free_handler: + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); +err_destroy_mutex: + mutex_destroy(&ov02a10->mutex); + + return ret; +} + +static int ov02a10_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ov02a10 *ov02a10 = to_ov02a10(sd); + + v4l2_async_unregister_subdev(sd); + media_entity_cleanup(&sd->entity); + v4l2_ctrl_handler_free(sd->ctrl_handler); + pm_runtime_disable(&client->dev); + if (!pm_runtime_status_suspended(&client->dev)) + __ov02a10_power_off(ov02a10); + pm_runtime_set_suspended(&client->dev); + mutex_destroy(&ov02a10->mutex); + + return 0; +} + +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id ov02a10_of_match[] = { + { .compatible = "ovti,ov02a10" }, + {}, +}; +MODULE_DEVICE_TABLE(of, ov02a10_of_match); +#endif + +static struct i2c_driver ov02a10_i2c_driver = { + .driver = { + .name = "ov02a10", + .pm = &ov02a10_pm_ops, + .of_match_table = ov02a10_of_match, + }, + .probe_new = &ov02a10_probe, + .remove = &ov02a10_remove, +}; + +module_i2c_driver(ov02a10_i2c_driver); + +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>"); +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver"); +MODULE_LICENSE("GPL v2"); + -- 2.9.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-08-19 3:43 ` [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver dongchun.zhu @ 2019-08-19 8:30 ` Sakari Ailus 2019-09-05 9:41 ` Dongchun Zhu 2019-08-21 10:30 ` Tomasz Figa 1 sibling, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2019-08-19 8:30 UTC (permalink / raw) To: dongchun.zhu Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang, tfiga, louis.kuo, sj.huang, robh+dt, linux-mediatek, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media Hi Dongchun, Thanks for the update. On Mon, Aug 19, 2019 at 11:43:31AM +0800, dongchun.zhu@mediatek.com wrote: > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > This patch adds a V4L2 sub-device driver for OV02A10 image sensor. > The OV02A10 is a 1/5" CMOS sensor from Omnivision, > which supports output format: 10-bit Raw. > The OV02A10 has a single MIPI lane interface and use the I2C bus > for control and the CSI-2 bus for data. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > MAINTAINERS | 1 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov02a10.c | 1018 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1031 insertions(+) > create mode 100644 drivers/media/i2c/ov02a10.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 41734fb..4b714a2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11821,6 +11821,7 @@ M: Dongchun Zhu <dongchun.zhu@mediatek.com> > L: linux-media@vger.kernel.org > T: git git://linuxtv.org/media_tree.git > S: Maintained > +F: drivers/media/i2c/ov02a10.c > F: Documentation/devicetree/bindings/media/i2c/ov02a10.txt > > OMNIVISION OV2680 SENSOR DRIVER > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 79ce9ec..d063a82 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -617,6 +617,17 @@ config VIDEO_IMX355 > To compile this driver as a module, choose M here: the > module will be called imx355. > > +config VIDEO_OV02A10 > + tristate "OmniVision OV02A10 sensor support" > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + depends on MEDIA_CAMERA_SUPPORT > + help > + This is a Video4Linux2 sensor driver for the OmniVision > + OV02A10 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called ov02a10. > + > config VIDEO_OV2640 > tristate "OmniVision OV2640 sensor support" > depends on VIDEO_V4L2 && I2C > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index fd4ea86..f34a7ac 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -71,6 +71,7 @@ obj-$(CONFIG_VIDEO_OV5645) += ov5645.o > obj-$(CONFIG_VIDEO_OV5647) += ov5647.o > obj-$(CONFIG_VIDEO_OV5670) += ov5670.o > obj-$(CONFIG_VIDEO_OV5695) += ov5695.o > +obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o > obj-$(CONFIG_VIDEO_OV6650) += ov6650.o > obj-$(CONFIG_VIDEO_OV7251) += ov7251.o > obj-$(CONFIG_VIDEO_OV7640) += ov7640.o > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > new file mode 100644 > index 0000000..ff5460a > --- /dev/null > +++ b/drivers/media/i2c/ov02a10.c > @@ -0,0 +1,1018 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + */ > + > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > +#include <media/media-entity.h> > +#include <media/v4l2-async.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-subdev.h> > + > +#define CHIP_ID 0x2509 > +#define OV02A10_REG_CHIP_ID_H 0x02 > +#define OV02A10_REG_CHIP_ID_L 0x03 > +#define OV02A10_ID(_msb, _lsb) ((_msb) << 8 | (_lsb)) > + > +/* Bit[1] vertical upside down */ > +/* Bit[0] horizontal mirror */ > +#define REG_MIRROR_FLIP_CONTROL 0x3f > + > +/* Orientation */ > +#define REG_CONFIG_MIRROR_FLIP 0x03 > + > +#define REG_PAGE_SWITCH 0xfd > +#define REG_GLOBAL_EFFECTIVE 0x01 > +#define REG_ENABLE BIT(0) > + > +#define REG_SC_CTRL_MODE 0xac > +#define SC_CTRL_MODE_STANDBY 0x00 > +#define SC_CTRL_MODE_STREAMING 0x01 > + > +#define OV02A10_REG_EXPOSURE_H 0x03 > +#define OV02A10_REG_EXPOSURE_L 0x04 > +#define OV02A10_EXPOSURE_MIN 4 > +#define OV02A10_EXPOSURE_STEP 1 > + > +#define OV02A10_REG_VTS_H 0x05 > +#define OV02A10_REG_VTS_L 0x06 > +#define OV02A10_VTS_MAX 0x209f > +#define OV02A10_VTS_MIN 0x04cf > +#define OV02A10_BASIC_LINE 1224 > + > +#define OV02A10_REG_GAIN 0x24 > +#define OV02A10_GAIN_MIN 0x10 > +#define OV02A10_GAIN_MAX 0xf8 > +#define OV02A10_GAIN_STEP 0x01 > +#define OV02A10_GAIN_DEFAULT 0x40 > + > +#define REG_NULL 0xff > + > +#define OV02A10_LANES 1 > +#define OV02A10_BITS_PER_SAMPLE 10 > + > +static const char * const ov02a10_supply_names[] = { > + "dovdd", /* Digital I/O power */ > + "avdd", /* Analog power */ > + "dvdd", /* Digital core power */ > +}; > + > +#define OV02A10_NUM_SUPPLIES ARRAY_SIZE(ov02a10_supply_names) > + > +struct regval { > + u16 addr; > + u8 val; > +}; > + > +struct ov02a10_mode { > + u32 width; > + u32 height; > + u32 exp_def; > + u32 hts_def; > + u32 vts_def; > + const struct regval *reg_list; > +}; > + > +struct ov02a10 { > + struct clk *xvclk; > + struct gpio_desc *powerdown_gpio; > + struct gpio_desc *reset_gpio; > + struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES]; > + > + bool streaming; > + bool upside_down; > + > + /* > + * Serialize control access, get/set format, get selection > + * and start streaming. > + */ > + struct mutex mutex; > + struct v4l2_subdev subdev; > + struct media_pad pad; > + struct v4l2_ctrl *anal_gain; > + struct v4l2_ctrl *exposure; > + struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *vblank; > + struct v4l2_ctrl *hflip; > + struct v4l2_ctrl *vflip; > + struct v4l2_ctrl *test_pattern; > + struct v4l2_mbus_framefmt fmt; > + struct v4l2_ctrl_handler ctrl_handler; > + > + const struct ov02a10_mode *cur_mode; > +}; > + > +#define to_ov02a10(sd) container_of(sd, struct ov02a10, subdev) > + > +static inline void msleep_range(unsigned int delay_base) > +{ > + usleep_range(delay_base * 1000, delay_base * 1000 + 500); > +} > + > +/* MIPI color bar enable output */ > +static const struct regval ov02a10_test_pattern_enable_regs[] = { > + {0xfd, 0x01}, > + {0x0d, 0x00}, > + {0xb6, 0x01}, > + {0x01, 0x01}, > + {0xfd, 0x01}, > + {0xac, 0x01}, > + {REG_NULL, 0x00} > +}; > + > +/* MIPI color bar disable output */ > +static const struct regval ov02a10_test_pattern_disable_regs[] = { > + {0xfd, 0x01}, > + {0x0d, 0x00}, > + {0xb6, 0x00}, > + {0x01, 0x01}, > + {0xfd, 0x01}, > + {0xac, 0x01}, > + {REG_NULL, 0x00} > +}; > + > +/* > + * xvclk 24Mhz > + * pclk 39Mhz > + * linelength 934(0x3a6) > + * framelength 1390(0x56e) > + * grabwindow_width 1600 > + * grabwindow_height 1200 > + * max_framerate 30fps > + * mipi_datarate per lane 780Mbps > + */ > +static const struct regval ov02a10_1600x1200_regs[] = { > + {0xfd, 0x01}, > + {0xac, 0x00}, > + {0xfd, 0x00}, > + {0x2f, 0x29}, > + {0x34, 0x00}, > + {0x35, 0x21}, > + {0x30, 0x15}, > + {0x33, 0x01}, > + {0xfd, 0x01}, > + {0x44, 0x00}, > + {0x2a, 0x4c}, > + {0x2b, 0x1e}, > + {0x2c, 0x60}, > + {0x25, 0x11}, > + {0x03, 0x01}, > + {0x04, 0xae}, > + {0x09, 0x00}, > + {0x0a, 0x02}, > + {0x06, 0xa6}, > + {0x31, 0x00}, > + {0x24, 0x40}, > + {0x01, 0x01}, > + {0xfb, 0x73}, > + {0xfd, 0x01}, > + {0x16, 0x04}, > + {0x1c, 0x09}, > + {0x21, 0x42}, > + {0x12, 0x04}, > + {0x13, 0x10}, > + {0x11, 0x40}, > + {0x33, 0x81}, > + {0xd0, 0x00}, > + {0xd1, 0x01}, > + {0xd2, 0x00}, > + {0x50, 0x10}, > + {0x51, 0x23}, > + {0x52, 0x20}, > + {0x53, 0x10}, > + {0x54, 0x02}, > + {0x55, 0x20}, > + {0x56, 0x02}, > + {0x58, 0x48}, > + {0x5d, 0x15}, > + {0x5e, 0x05}, > + {0x66, 0x66}, > + {0x68, 0x68}, > + {0x6b, 0x00}, > + {0x6c, 0x00}, > + {0x6f, 0x40}, > + {0x70, 0x40}, > + {0x71, 0x0a}, > + {0x72, 0xf0}, > + {0x73, 0x10}, > + {0x75, 0x80}, > + {0x76, 0x10}, > + {0x84, 0x00}, > + {0x85, 0x10}, > + {0x86, 0x10}, > + {0x87, 0x00}, > + {0x8a, 0x22}, > + {0x8b, 0x22}, > + {0x19, 0xf1}, > + {0x29, 0x01}, > + {0xfd, 0x01}, > + {0x9d, 0xd6}, > + {0xa0, 0x29}, > + {0xa1, 0x03}, > + {0xad, 0x62}, > + {0xae, 0x00}, > + {0xaf, 0x85}, > + {0xb1, 0x01}, > + {0x8e, 0x06}, > + {0x8f, 0x40}, > + {0x90, 0x04}, > + {0x91, 0xb0}, > + {0x45, 0x01}, > + {0x46, 0x00}, > + {0x47, 0x6c}, > + {0x48, 0x03}, > + {0x49, 0x8b}, > + {0x4a, 0x00}, > + {0x4b, 0x07}, > + {0x4c, 0x04}, > + {0x4d, 0xb7}, > + {0xf0, 0x40}, > + {0xf1, 0x40}, > + {0xf2, 0x40}, > + {0xf3, 0x40}, > + {0x3f, 0x00}, > + {0xfd, 0x01}, > + {0x05, 0x00}, > + {0x06, 0xa6}, > + {0xfd, 0x01}, > + {REG_NULL, 0x00} > +}; > + > +#define OV02A10_LINK_FREQ_390MHZ 390000000 > +static const s64 link_freq_menu_items[] = { > + OV02A10_LINK_FREQ_390MHZ > +}; > + > +static const char * const ov02a10_test_pattern_menu[] = { > + "Disabled", > + "Color Bar", > +}; > + > +static const struct ov02a10_mode supported_modes[] = { > + { > + .width = 1600, > + .height = 1200, > + .exp_def = 0x01ae, > + .hts_def = 0x03a6, > + .vts_def = 0x056e, > + .reg_list = ov02a10_1600x1200_regs, > + }, > +}; > + > +/* Write a register */ > +static int ov02a10_write_reg(struct ov02a10 *ov02a10, u8 addr, u8 val) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + u8 buf[2] = {addr, val}; > + int ret; > + > + ret = i2c_master_send(client, buf, 2); > + > + if (ret != 2) { > + dev_err(&client->dev, "%s: error: reg=%x, val=%x\n", > + __func__, addr, val); > + return -EIO; > + } > + > + return 0; > +} > + > +static int ov02a10_write_array(struct ov02a10 *ov02a10, > + const struct regval *regs) > +{ > + u32 i; unsigned int, please. > + int ret; > + > + for (i = 0; regs[i].addr != REG_NULL; i++) { > + ret = ov02a10_write_reg(ov02a10, regs[i].addr, regs[i].val); > + if (ret < 0) > + return ret; > + } > + > + return 0; > +} > + > +/* Read a register */ > +static int ov02a10_read_reg(struct ov02a10 *ov02a10, u8 reg, u8 *val) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + u8 data = reg; > + struct i2c_msg msg = { > + .addr = client->addr, > + .flags = 0, > + .len = 1, > + .buf = &data, > + }; > + int ret; > + > + ret = i2c_transfer(client->adapter, &msg, 1); > + if (ret < 0) > + goto err_wr; > + > + msg.flags = I2C_M_RD; > + ret = i2c_transfer(client->adapter, &msg, 1); > + if (ret < 0) > + goto err_rd; > + > + *val = data; > + return 0; > + > +err_rd: > + dev_err(&client->dev, "i2c_transfer --I2C_M_RD failed\n"); > +err_wr: > + dev_err(&client->dev, "read error: reg=0x%02x: %d\n", reg, ret); > + return ret; > +} > + > +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode, > + struct v4l2_mbus_framefmt *fmt) > +{ > + fmt->width = mode->width; > + fmt->height = mode->height; > + fmt->field = V4L2_FIELD_NONE; > +} > + > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + mutex_lock(&ov02a10->mutex); > + > + if (ov02a10->streaming) { > + mutex_unlock(&ov02a10->mutex); > + return -EBUSY; > + } > + > + /* Only one sensor mode supported */ > + mbus_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > + ov02a10->fmt = fmt->format; > + > + mutex_unlock(&ov02a10->mutex); > + > + return 0; > +} > + > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + mutex_lock(&ov02a10->mutex); > + > + fmt->format = ov02a10->fmt; > + mbus_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > + > + mutex_unlock(&ov02a10->mutex); > + > + return 0; > +} > + > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + if (code->index >= ARRAY_SIZE(supported_modes) || !(code->index)) This doesn't make sense. The supported modes, should more be added in the future, are presumably using the same mbus code. If you only have one, then non-zero code->index would result in -EINVAL (and not zero code->index). > + return -EINVAL; > + > + code->code = ov02a10->fmt.code; > + > + return 0; > +} > + > +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->index >= ARRAY_SIZE(supported_modes) || !(fse->index)) > + return -EINVAL; > + > + fse->min_width = supported_modes[fse->index].width; > + fse->max_width = supported_modes[fse->index].width; > + fse->max_height = supported_modes[fse->index].height; > + fse->min_height = supported_modes[fse->index].height; > + > + return 0; > +} > + > +static int __ov02a10_power_on(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + struct device *dev = &client->dev; > + int ret; > + > + ret = clk_prepare_enable(ov02a10->xvclk); > + if (ret < 0) { > + dev_err(dev, "Failed to enable xvclk\n"); > + return ret; > + } > + > + /* Note: set 0 is high, set 1 is low */ > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 0); > + > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > + if (ret < 0) { > + dev_err(dev, "Failed to enable regulators\n"); > + goto disable_clk; > + } > + msleep_range(7); This has some potential of clashing with more generic functions in the future. Please use usleep_range directly, or msleep. > + > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > + msleep_range(10); > + > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 0); > + msleep_range(10); > + > + return 0; > + > +disable_clk: > + clk_disable_unprepare(ov02a10->xvclk); > + > + return ret; > +} > + > +static void __ov02a10_power_off(struct ov02a10 *ov02a10) > +{ > + clk_disable_unprepare(ov02a10->xvclk); > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > +} > + > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + int ret; > + > + /* Apply default values of current mode */ > + ret = ov02a10_write_array(ov02a10, ov02a10->cur_mode->reg_list); > + if (ret) > + return ret; > + > + /* Apply customized values from user */ > + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler); > + if (ret) > + return ret; > + > + /* Set orientation to 180 degree */ > + if (ov02a10->upside_down) { > + ret = ov02a10_write_reg(ov02a10, REG_MIRROR_FLIP_CONTROL, > + REG_CONFIG_MIRROR_FLIP); > + if (ret) { > + dev_err(&client->dev, "%s failed to set orientation\n", > + __func__); > + return ret; > + } > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + } > + > + /* Set stream on register */ > + return ov02a10_write_reg(ov02a10, > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STREAMING); > +} > + > +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10) > +{ > + return ov02a10_write_reg(ov02a10, > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STANDBY); > +} > + > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg) > +{ > + struct v4l2_subdev_format fmt = { 0 }; > + > + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; > + fmt.format.width = 1600; > + fmt.format.height = 1200; > + > + ov02a10_set_fmt(subdev, cfg, &fmt); > + > + return 0; > +} > + > +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + int ret = 0; > + > + mutex_lock(&ov02a10->mutex); > + > + if (ov02a10->streaming == on) > + goto unlock_and_return; > + > + if (on) { > + ret = pm_runtime_get_sync(&client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&client->dev); > + goto unlock_and_return; > + } > + > + ret = __ov02a10_start_stream(ov02a10); > + if (ret) { > + __ov02a10_stop_stream(ov02a10); > + ov02a10->streaming = !on; > + goto err_rpm_put; > + } > + } else { > + __ov02a10_stop_stream(ov02a10); > + pm_runtime_put(&client->dev); > + } > + > + ov02a10->streaming = on; > + mutex_unlock(&ov02a10->mutex); > + > + return ret; > + > +err_rpm_put: > + pm_runtime_put(&client->dev); > +unlock_and_return: > + mutex_unlock(&ov02a10->mutex); > + > + return ret; > +} > + > +static int ov02a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_get_try_format(sd, > + fh->pad, > + 0); > + > + mutex_lock(&ov02a10->mutex); > + /* Initialize try_fmt */ > + try_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(&supported_modes[0], try_fmt); > + > + mutex_unlock(&ov02a10->mutex); No need for the mutex: the supported values are static and there is no parallel access to try_fmt. Btw. you don't need to do this here as you have init_cfg implemented, i.e. you can omit the open callback. > + > + return 0; > +} > + > +static int __maybe_unused ov02a10_runtime_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + return __ov02a10_power_on(ov02a10); > +} > + > +static int __maybe_unused ov02a10_runtime_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + __ov02a10_power_off(ov02a10); > + > + return 0; > +} > + > +static const struct dev_pm_ops ov02a10_pm_ops = { > + SET_RUNTIME_PM_OPS(ov02a10_runtime_suspend, > + ov02a10_runtime_resume, NULL) > +}; > + > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, s32 value) > +{ > + if (value) > + return ov02a10_write_array(ov02a10, > + ov02a10_test_pattern_enable_regs); > + > + return ov02a10_write_array(ov02a10, > + ov02a10_test_pattern_disable_regs); Fits on previous line. > +} > + > +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct ov02a10 *ov02a10 = container_of(ctrl->handler, > + struct ov02a10, ctrl_handler); > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + s64 max_expo; > + int ret; > + > + /* Propagate change of current control to all related controls */ > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure while meeting expected vblanking */ > + max_expo = ov02a10->cur_mode->height + ctrl->val - 4; > + __v4l2_ctrl_modify_range(ov02a10->exposure, > + ov02a10->exposure->minimum, max_expo, > + ov02a10->exposure->step, > + ov02a10->exposure->default_value); > + } > + > + /* V4L2 controls values will be applied only when power is already up */ > + if (!pm_runtime_get_if_in_use(&client->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_EXPOSURE: > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_H, > + ((ctrl->val >> 8) & 0xFF)); > + if (!ret) { > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_L, > + (ctrl->val & 0xFF)); > + if (ret < 0) > + return ret; > + } > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + break; > + case V4L2_CID_ANALOGUE_GAIN: > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_GAIN, > + (ctrl->val & 0xFF)); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + break; > + case V4L2_CID_VBLANK: > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_H, > + (((ctrl->val + > + ov02a10->cur_mode->height - > + OV02A10_BASIC_LINE) >> 8) > + & 0xFF)); > + if (!ret) { > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_L, > + ((ctrl->val + > + ov02a10->cur_mode->height - > + OV02A10_BASIC_LINE) & 0xFF)); > + if (ret < 0) > + return ret; > + } > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + break; > + case V4L2_CID_TEST_PATTERN: > + ret = ov02a10_set_test_pattern(ov02a10, ctrl->val); > + if (ret < 0) > + return ret; > + break; > + default: > + dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n", > + __func__, ctrl->id, ctrl->val); > + ret = -EINVAL; > + break; > + }; > + > + pm_runtime_put(&client->dev); > + > + return ret; > +} > + > +static const struct v4l2_subdev_video_ops ov02a10_video_ops = { > + .s_stream = ov02a10_s_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = { > + .init_cfg = ov02a10_entity_init_cfg, > + .enum_mbus_code = ov02a10_enum_mbus_code, > + .enum_frame_size = ov02a10_enum_frame_sizes, > + .get_fmt = ov02a10_get_fmt, > + .set_fmt = ov02a10_set_fmt, > +}; > + > +static const struct v4l2_subdev_ops ov02a10_subdev_ops = { > + .video = &ov02a10_video_ops, > + .pad = &ov02a10_pad_ops, > +}; > + > +static const struct media_entity_operations ov02a10_subdev_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static const struct v4l2_subdev_internal_ops ov02a10_internal_ops = { > + .open = ov02a10_open, > +}; > + > +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = { > + .s_ctrl = ov02a10_set_ctrl, > +}; > + > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + const struct ov02a10_mode *mode; > + struct v4l2_ctrl_handler *handler; > + struct v4l2_ctrl *ctrl; > + u64 exposure_max; > + u32 pixel_rate, h_blank; > + int ret; > + > + handler = &ov02a10->ctrl_handler; > + mode = ov02a10->cur_mode; > + ret = v4l2_ctrl_handler_init(handler, 10); > + if (ret) > + return ret; An extra newline here, please. > + handler->lock = &ov02a10->mutex; > + > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, > + 0, 0, link_freq_menu_items); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + pixel_rate = (link_freq_menu_items[0] * 2 * OV02A10_LANES) / > + OV02A10_BITS_PER_SAMPLE; Please use do_div(). The values are static so the compiler may not emit the instructions for division but better be safe than sorry. > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, > + 0, pixel_rate, 1, pixel_rate); > + > + h_blank = mode->hts_def - mode->width; > + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK, > + h_blank, h_blank, 1, h_blank); > + if (ov02a10->hblank) > + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_VBLANK, mode->vts_def - > + mode->height, > + OV02A10_VTS_MAX - mode->height, 1, > + mode->vts_def - mode->height); > + > + exposure_max = mode->vts_def - 4; > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_EXPOSURE, > + OV02A10_EXPOSURE_MIN, > + exposure_max, > + OV02A10_EXPOSURE_STEP, > + mode->exp_def); > + > + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, > + OV02A10_GAIN_MIN, > + OV02A10_GAIN_MAX, > + OV02A10_GAIN_STEP, > + OV02A10_GAIN_DEFAULT); > + > + ov02a10->test_pattern = > + v4l2_ctrl_new_std_menu_items(handler, Indentation. If some of the arguments slip over 80 I guess we can live with that. > + &ov02a10_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(ov02a10_test_pattern_menu) - > + 1, 0, 0, ov02a10_test_pattern_menu); > + > + if (handler->error) { > + ret = handler->error; > + dev_err(&client->dev, > + "Failed to init controls(%d)\n", ret); Fits on the previous line. > + goto err_free_handler; > + } > + > + ov02a10->subdev.ctrl_handler = handler; > + > + return 0; > + > +err_free_handler: > + v4l2_ctrl_handler_free(handler); > + > + return ret; > +} > + > +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + u16 id; > + u8 pid = 0; > + u8 ver = 0; > + int ret; > + > + /* Check sensor revision */ > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_H, &pid); > + if (ret) > + return ret; > + > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_L, &ver); > + if (ret) > + return ret; > + > + id = OV02A10_ID(pid, ver); > + if (id != CHIP_ID) { > + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id); > + return ret; > + } > + > + dev_info(&client->dev, "Detected OV%04X sensor\n", id); > + > + return 0; > +} > + > +static int ov02a10_configure_regulators(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + unsigned int i; > + > + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++) > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > + > + return devm_regulator_bulk_get(&client->dev, > + OV02A10_NUM_SUPPLIES, > + ov02a10->supplies); > +} > + > +static int ov02a10_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct ov02a10 *ov02a10; > + u32 rotation; > + u32 xclk_freq; > + int ret; > + > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > + if (!ov02a10) > + return -ENOMEM; > + > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > + > + /* Optional indication of physical rotation of sensor */ > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", > + &rotation); > + if (!ret) { > + switch (rotation) { > + case 180: > + ov02a10->upside_down = true; > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > + break; > + case 0: > + break; > + default: > + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", > + rotation); > + } > + } > + Please also check that the link frequencies in DT match with what the driver expects. See e.g. drivers/media/i2c/ov8856.c driver for an example. > + /* Get system clock (xvclk) */ > + ov02a10->xvclk = devm_clk_get(dev, "xvclk"); > + if (IS_ERR(ov02a10->xvclk)) { > + dev_err(dev, "Failed to get xvclk\n"); > + return -EINVAL; > + } > + > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > + if (ret) { > + dev_err(dev, "Failed to get xclk frequency\n"); > + return ret; > + } > + > + /* External clock must be 24MHz, allow 1% tolerance */ > + if (xclk_freq < 23760000 || xclk_freq > 24240000) { > + dev_err(dev, "external clock frequency %u is not supported\n", > + xclk_freq); > + return -EINVAL; > + } > + dev_dbg(dev, "external clock frequency %u\n", xclk_freq); > + > + ret = clk_set_rate(ov02a10->xvclk, xclk_freq); > + if (ret) { > + dev_err(dev, "Failed to set xvclk frequency (24MHz)\n"); > + return ret; > + } > + > + ov02a10->powerdown_gpio = devm_gpiod_get(dev, "powerdown", > + GPIOD_OUT_LOW); > + if (IS_ERR(ov02a10->powerdown_gpio)) { > + dev_err(dev, "Failed to get powerdown-gpios\n"); > + return -EINVAL; > + } > + > + ov02a10->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(ov02a10->reset_gpio)) { > + dev_err(dev, "Failed to get reset-gpios\n"); > + return -EINVAL; > + } > + > + ret = ov02a10_configure_regulators(ov02a10); > + if (ret) { > + dev_err(dev, "Failed to get power regulators\n"); > + return ret; > + } > + > + mutex_init(&ov02a10->mutex); > + ov02a10->cur_mode = &supported_modes[0]; > + ret = ov02a10_initialize_controls(ov02a10); > + if (ret) { > + dev_err(dev, "Failed to initialize controls\n"); > + goto err_destroy_mutex; > + } > + > + ret = __ov02a10_power_on(ov02a10); > + if (ret) > + goto err_free_handler; > + > + ret = ov02a10_check_sensor_id(ov02a10); > + if (ret) > + goto err_power_off; > + > + ov02a10->subdev.internal_ops = &ov02a10_internal_ops; > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > + if (ret < 0) { > + dev_err(dev, "failed to init entity pads: %d", ret); > + goto err_power_off; > + } > + > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > + if (ret) { > + dev_err(dev, "failed to register V4L2 subdev: %d", > + ret); > + goto err_clean_entity; > + } > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_idle(dev); > + > + dev_info(dev, "ov02a10 probe --\n"); This isn't very informative. The driver already prints errors when probing fails and otherwise it succeeds. Please remove. > + return 0; > + > +err_clean_entity: > + media_entity_cleanup(&ov02a10->subdev.entity); > +err_power_off: > + __ov02a10_power_off(ov02a10); > +err_free_handler: > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > +err_destroy_mutex: > + mutex_destroy(&ov02a10->mutex); > + > + return ret; > +} > + > +static int ov02a10_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + v4l2_async_unregister_subdev(sd); > + media_entity_cleanup(&sd->entity); > + v4l2_ctrl_handler_free(sd->ctrl_handler); > + pm_runtime_disable(&client->dev); > + if (!pm_runtime_status_suspended(&client->dev)) > + __ov02a10_power_off(ov02a10); > + pm_runtime_set_suspended(&client->dev); > + mutex_destroy(&ov02a10->mutex); > + > + return 0; > +} > + > +#if IS_ENABLED(CONFIG_OF) > +static const struct of_device_id ov02a10_of_match[] = { > + { .compatible = "ovti,ov02a10" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > +#endif > + > +static struct i2c_driver ov02a10_i2c_driver = { > + .driver = { > + .name = "ov02a10", > + .pm = &ov02a10_pm_ops, > + .of_match_table = ov02a10_of_match, > + }, > + .probe_new = &ov02a10_probe, > + .remove = &ov02a10_remove, > +}; > + > +module_i2c_driver(ov02a10_i2c_driver); > + > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>"); > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver"); > +MODULE_LICENSE("GPL v2"); -- Kind regards, Sakari Ailus sakari.ailus@linux.intel.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-08-19 8:30 ` Sakari Ailus @ 2019-09-05 9:41 ` Dongchun Zhu 2019-09-05 10:45 ` Sakari Ailus 0 siblings, 1 reply; 16+ messages in thread From: Dongchun Zhu @ 2019-09-05 9:41 UTC (permalink / raw) To: Sakari Ailus Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang, tfiga, louis.kuo, sj.huang, robh+dt, linux-mediatek, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media Hi Sakari, On Mon, 2019-08-19 at 11:30 +0300, Sakari Ailus wrote: > Hi Dongchun, > > Thanks for the update. > > On Mon, Aug 19, 2019 at 11:43:31AM +0800, dongchun.zhu@mediatek.com wrote: > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > This patch adds a V4L2 sub-device driver for OV02A10 image sensor. > > The OV02A10 is a 1/5" CMOS sensor from Omnivision, > > which supports output format: 10-bit Raw. > > The OV02A10 has a single MIPI lane interface and use the I2C bus > > for control and the CSI-2 bus for data. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 11 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1018 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1031 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 41734fb..4b714a2 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -11821,6 +11821,7 @@ M: Dongchun Zhu <dongchun.zhu@mediatek.com> > > L: linux-media@vger.kernel.org > > T: git git://linuxtv.org/media_tree.git > > S: Maintained > > +F: drivers/media/i2c/ov02a10.c > > F: Documentation/devicetree/bindings/media/i2c/ov02a10.txt > > > > OMNIVISION OV2680 SENSOR DRIVER > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 79ce9ec..d063a82 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -617,6 +617,17 @@ config VIDEO_IMX355 > > To compile this driver as a module, choose M here: the > > module will be called imx355. > > > > +config VIDEO_OV02A10 > > + tristate "OmniVision OV02A10 sensor support" > > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > > + depends on MEDIA_CAMERA_SUPPORT > > + help > > + This is a Video4Linux2 sensor driver for the OmniVision > > + OV02A10 camera. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called ov02a10. > > + > > config VIDEO_OV2640 > > tristate "OmniVision OV2640 sensor support" > > depends on VIDEO_V4L2 && I2C > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > > index fd4ea86..f34a7ac 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -71,6 +71,7 @@ obj-$(CONFIG_VIDEO_OV5645) += ov5645.o > > obj-$(CONFIG_VIDEO_OV5647) += ov5647.o > > obj-$(CONFIG_VIDEO_OV5670) += ov5670.o > > obj-$(CONFIG_VIDEO_OV5695) += ov5695.o > > +obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o > > obj-$(CONFIG_VIDEO_OV6650) += ov6650.o > > obj-$(CONFIG_VIDEO_OV7251) += ov7251.o > > obj-$(CONFIG_VIDEO_OV7640) += ov7640.o > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > new file mode 100644 > > index 0000000..ff5460a > > --- /dev/null > > +++ b/drivers/media/i2c/ov02a10.c > > @@ -0,0 +1,1018 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/device.h> > > +#include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regulator/consumer.h> > > +#include <media/media-entity.h> > > +#include <media/v4l2-async.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-subdev.h> > > + > > +#define CHIP_ID 0x2509 > > +#define OV02A10_REG_CHIP_ID_H 0x02 > > +#define OV02A10_REG_CHIP_ID_L 0x03 > > +#define OV02A10_ID(_msb, _lsb) ((_msb) << 8 | (_lsb)) > > + > > +/* Bit[1] vertical upside down */ > > +/* Bit[0] horizontal mirror */ > > +#define REG_MIRROR_FLIP_CONTROL 0x3f > > + > > +/* Orientation */ > > +#define REG_CONFIG_MIRROR_FLIP 0x03 > > + > > +#define REG_PAGE_SWITCH 0xfd > > +#define REG_GLOBAL_EFFECTIVE 0x01 > > +#define REG_ENABLE BIT(0) > > + > > +#define REG_SC_CTRL_MODE 0xac > > +#define SC_CTRL_MODE_STANDBY 0x00 > > +#define SC_CTRL_MODE_STREAMING 0x01 > > + > > +#define OV02A10_REG_EXPOSURE_H 0x03 > > +#define OV02A10_REG_EXPOSURE_L 0x04 > > +#define OV02A10_EXPOSURE_MIN 4 > > +#define OV02A10_EXPOSURE_STEP 1 > > + > > +#define OV02A10_REG_VTS_H 0x05 > > +#define OV02A10_REG_VTS_L 0x06 > > +#define OV02A10_VTS_MAX 0x209f > > +#define OV02A10_VTS_MIN 0x04cf > > +#define OV02A10_BASIC_LINE 1224 > > + > > +#define OV02A10_REG_GAIN 0x24 > > +#define OV02A10_GAIN_MIN 0x10 > > +#define OV02A10_GAIN_MAX 0xf8 > > +#define OV02A10_GAIN_STEP 0x01 > > +#define OV02A10_GAIN_DEFAULT 0x40 > > + > > +#define REG_NULL 0xff > > + > > +#define OV02A10_LANES 1 > > +#define OV02A10_BITS_PER_SAMPLE 10 > > + > > +static const char * const ov02a10_supply_names[] = { > > + "dovdd", /* Digital I/O power */ > > + "avdd", /* Analog power */ > > + "dvdd", /* Digital core power */ > > +}; > > + > > +#define OV02A10_NUM_SUPPLIES ARRAY_SIZE(ov02a10_supply_names) > > + > > +struct regval { > > + u16 addr; > > + u8 val; > > +}; > > + > > +struct ov02a10_mode { > > + u32 width; > > + u32 height; > > + u32 exp_def; > > + u32 hts_def; > > + u32 vts_def; > > + const struct regval *reg_list; > > +}; > > + > > +struct ov02a10 { > > + struct clk *xvclk; > > + struct gpio_desc *powerdown_gpio; > > + struct gpio_desc *reset_gpio; > > + struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES]; > > + > > + bool streaming; > > + bool upside_down; > > + > > + /* > > + * Serialize control access, get/set format, get selection > > + * and start streaming. > > + */ > > + struct mutex mutex; > > + struct v4l2_subdev subdev; > > + struct media_pad pad; > > + struct v4l2_ctrl *anal_gain; > > + struct v4l2_ctrl *exposure; > > + struct v4l2_ctrl *hblank; > > + struct v4l2_ctrl *vblank; > > + struct v4l2_ctrl *hflip; > > + struct v4l2_ctrl *vflip; > > + struct v4l2_ctrl *test_pattern; > > + struct v4l2_mbus_framefmt fmt; > > + struct v4l2_ctrl_handler ctrl_handler; > > + > > + const struct ov02a10_mode *cur_mode; > > +}; > > + > > +#define to_ov02a10(sd) container_of(sd, struct ov02a10, subdev) > > + > > +static inline void msleep_range(unsigned int delay_base) > > +{ > > + usleep_range(delay_base * 1000, delay_base * 1000 + 500); > > +} > > + > > +/* MIPI color bar enable output */ > > +static const struct regval ov02a10_test_pattern_enable_regs[] = { > > + {0xfd, 0x01}, > > + {0x0d, 0x00}, > > + {0xb6, 0x01}, > > + {0x01, 0x01}, > > + {0xfd, 0x01}, > > + {0xac, 0x01}, > > + {REG_NULL, 0x00} > > +}; > > + > > +/* MIPI color bar disable output */ > > +static const struct regval ov02a10_test_pattern_disable_regs[] = { > > + {0xfd, 0x01}, > > + {0x0d, 0x00}, > > + {0xb6, 0x00}, > > + {0x01, 0x01}, > > + {0xfd, 0x01}, > > + {0xac, 0x01}, > > + {REG_NULL, 0x00} > > +}; > > + > > +/* > > + * xvclk 24Mhz > > + * pclk 39Mhz > > + * linelength 934(0x3a6) > > + * framelength 1390(0x56e) > > + * grabwindow_width 1600 > > + * grabwindow_height 1200 > > + * max_framerate 30fps > > + * mipi_datarate per lane 780Mbps > > + */ > > +static const struct regval ov02a10_1600x1200_regs[] = { > > + {0xfd, 0x01}, > > + {0xac, 0x00}, > > + {0xfd, 0x00}, > > + {0x2f, 0x29}, > > + {0x34, 0x00}, > > + {0x35, 0x21}, > > + {0x30, 0x15}, > > + {0x33, 0x01}, > > + {0xfd, 0x01}, > > + {0x44, 0x00}, > > + {0x2a, 0x4c}, > > + {0x2b, 0x1e}, > > + {0x2c, 0x60}, > > + {0x25, 0x11}, > > + {0x03, 0x01}, > > + {0x04, 0xae}, > > + {0x09, 0x00}, > > + {0x0a, 0x02}, > > + {0x06, 0xa6}, > > + {0x31, 0x00}, > > + {0x24, 0x40}, > > + {0x01, 0x01}, > > + {0xfb, 0x73}, > > + {0xfd, 0x01}, > > + {0x16, 0x04}, > > + {0x1c, 0x09}, > > + {0x21, 0x42}, > > + {0x12, 0x04}, > > + {0x13, 0x10}, > > + {0x11, 0x40}, > > + {0x33, 0x81}, > > + {0xd0, 0x00}, > > + {0xd1, 0x01}, > > + {0xd2, 0x00}, > > + {0x50, 0x10}, > > + {0x51, 0x23}, > > + {0x52, 0x20}, > > + {0x53, 0x10}, > > + {0x54, 0x02}, > > + {0x55, 0x20}, > > + {0x56, 0x02}, > > + {0x58, 0x48}, > > + {0x5d, 0x15}, > > + {0x5e, 0x05}, > > + {0x66, 0x66}, > > + {0x68, 0x68}, > > + {0x6b, 0x00}, > > + {0x6c, 0x00}, > > + {0x6f, 0x40}, > > + {0x70, 0x40}, > > + {0x71, 0x0a}, > > + {0x72, 0xf0}, > > + {0x73, 0x10}, > > + {0x75, 0x80}, > > + {0x76, 0x10}, > > + {0x84, 0x00}, > > + {0x85, 0x10}, > > + {0x86, 0x10}, > > + {0x87, 0x00}, > > + {0x8a, 0x22}, > > + {0x8b, 0x22}, > > + {0x19, 0xf1}, > > + {0x29, 0x01}, > > + {0xfd, 0x01}, > > + {0x9d, 0xd6}, > > + {0xa0, 0x29}, > > + {0xa1, 0x03}, > > + {0xad, 0x62}, > > + {0xae, 0x00}, > > + {0xaf, 0x85}, > > + {0xb1, 0x01}, > > + {0x8e, 0x06}, > > + {0x8f, 0x40}, > > + {0x90, 0x04}, > > + {0x91, 0xb0}, > > + {0x45, 0x01}, > > + {0x46, 0x00}, > > + {0x47, 0x6c}, > > + {0x48, 0x03}, > > + {0x49, 0x8b}, > > + {0x4a, 0x00}, > > + {0x4b, 0x07}, > > + {0x4c, 0x04}, > > + {0x4d, 0xb7}, > > + {0xf0, 0x40}, > > + {0xf1, 0x40}, > > + {0xf2, 0x40}, > > + {0xf3, 0x40}, > > + {0x3f, 0x00}, > > + {0xfd, 0x01}, > > + {0x05, 0x00}, > > + {0x06, 0xa6}, > > + {0xfd, 0x01}, > > + {REG_NULL, 0x00} > > +}; > > + > > +#define OV02A10_LINK_FREQ_390MHZ 390000000 > > +static const s64 link_freq_menu_items[] = { > > + OV02A10_LINK_FREQ_390MHZ > > +}; > > + > > +static const char * const ov02a10_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bar", > > +}; > > + > > +static const struct ov02a10_mode supported_modes[] = { > > + { > > + .width = 1600, > > + .height = 1200, > > + .exp_def = 0x01ae, > > + .hts_def = 0x03a6, > > + .vts_def = 0x056e, > > + .reg_list = ov02a10_1600x1200_regs, > > + }, > > +}; > > + > > +/* Write a register */ > > +static int ov02a10_write_reg(struct ov02a10 *ov02a10, u8 addr, u8 val) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + u8 buf[2] = {addr, val}; > > + int ret; > > + > > + ret = i2c_master_send(client, buf, 2); > > + > > + if (ret != 2) { > > + dev_err(&client->dev, "%s: error: reg=%x, val=%x\n", > > + __func__, addr, val); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > + > > +static int ov02a10_write_array(struct ov02a10 *ov02a10, > > + const struct regval *regs) > > +{ > > + u32 i; > > unsigned int, please. > Fixed in next release. > > + int ret; > > + > > + for (i = 0; regs[i].addr != REG_NULL; i++) { > > + ret = ov02a10_write_reg(ov02a10, regs[i].addr, regs[i].val); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +/* Read a register */ > > +static int ov02a10_read_reg(struct ov02a10 *ov02a10, u8 reg, u8 *val) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + u8 data = reg; > > + struct i2c_msg msg = { > > + .addr = client->addr, > > + .flags = 0, > > + .len = 1, > > + .buf = &data, > > + }; > > + int ret; > > + > > + ret = i2c_transfer(client->adapter, &msg, 1); > > + if (ret < 0) > > + goto err_wr; > > + > > + msg.flags = I2C_M_RD; > > + ret = i2c_transfer(client->adapter, &msg, 1); > > + if (ret < 0) > > + goto err_rd; > > + > > + *val = data; > > + return 0; > > + > > +err_rd: > > + dev_err(&client->dev, "i2c_transfer --I2C_M_RD failed\n"); > > +err_wr: > > + dev_err(&client->dev, "read error: reg=0x%02x: %d\n", reg, ret); > > + return ret; > > +} > > + > > +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode, > > + struct v4l2_mbus_framefmt *fmt) > > +{ > > + fmt->width = mode->width; > > + fmt->height = mode->height; > > + fmt->field = V4L2_FIELD_NONE; > > +} > > + > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + if (ov02a10->streaming) { > > + mutex_unlock(&ov02a10->mutex); > > + return -EBUSY; > > + } > > + > > + /* Only one sensor mode supported */ > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + ov02a10->fmt = fmt->format; > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + fmt->format = ov02a10->fmt; > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + if (code->index >= ARRAY_SIZE(supported_modes) || !(code->index)) > > This doesn't make sense. The supported modes, should more be added in the > future, are presumably using the same mbus code. If you only have one, then > non-zero code->index would result in -EINVAL (and not zero code->index). > Thanks for kind reminder. !(code->index) would be removed in next release. > > + return -EINVAL; > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + if (fse->index >= ARRAY_SIZE(supported_modes) || !(fse->index)) > > + return -EINVAL; > > + > > + fse->min_width = supported_modes[fse->index].width; > > + fse->max_width = supported_modes[fse->index].width; > > + fse->max_height = supported_modes[fse->index].height; > > + fse->min_height = supported_modes[fse->index].height; > > + > > + return 0; > > +} > > + > > +static int __ov02a10_power_on(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + struct device *dev = &client->dev; > > + int ret; > > + > > + ret = clk_prepare_enable(ov02a10->xvclk); > > + if (ret < 0) { > > + dev_err(dev, "Failed to enable xvclk\n"); > > + return ret; > > + } > > + > > + /* Note: set 0 is high, set 1 is low */ > > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 0); > > + > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > + if (ret < 0) { > > + dev_err(dev, "Failed to enable regulators\n"); > > + goto disable_clk; > > + } > > + msleep_range(7); > > This has some potential of clashing with more generic functions in the > future. Please use usleep_range directly, or msleep. > Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1? https://patchwork.kernel.org/patch/10957225/ > > + > > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > > + msleep_range(10); > > + > > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 0); > > + msleep_range(10); > > + > > + return 0; > > + > > +disable_clk: > > + clk_disable_unprepare(ov02a10->xvclk); > > + > > + return ret; > > +} > > + > > +static void __ov02a10_power_off(struct ov02a10 *ov02a10) > > +{ > > + clk_disable_unprepare(ov02a10->xvclk); > > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > > + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > +} > > + > > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + int ret; > > + > > + /* Apply default values of current mode */ > > + ret = ov02a10_write_array(ov02a10, ov02a10->cur_mode->reg_list); > > + if (ret) > > + return ret; > > + > > + /* Apply customized values from user */ > > + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler); > > + if (ret) > > + return ret; > > + > > + /* Set orientation to 180 degree */ > > + if (ov02a10->upside_down) { > > + ret = ov02a10_write_reg(ov02a10, REG_MIRROR_FLIP_CONTROL, > > + REG_CONFIG_MIRROR_FLIP); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to set orientation\n", > > + __func__); > > + return ret; > > + } > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + } > > + > > + /* Set stream on register */ > > + return ov02a10_write_reg(ov02a10, > > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STREAMING); > > +} > > + > > +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10) > > +{ > > + return ov02a10_write_reg(ov02a10, > > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STANDBY); > > +} > > + > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg) > > +{ > > + struct v4l2_subdev_format fmt = { 0 }; > > + > > + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; > > + fmt.format.width = 1600; > > + fmt.format.height = 1200; > > + > > + ov02a10_set_fmt(subdev, cfg, &fmt); > > + > > + return 0; > > +} > > + > > +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + int ret = 0; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + if (ov02a10->streaming == on) > > + goto unlock_and_return; > > + > > + if (on) { > > + ret = pm_runtime_get_sync(&client->dev); > > + if (ret < 0) { > > + pm_runtime_put_noidle(&client->dev); > > + goto unlock_and_return; > > + } > > + > > + ret = __ov02a10_start_stream(ov02a10); > > + if (ret) { > > + __ov02a10_stop_stream(ov02a10); > > + ov02a10->streaming = !on; > > + goto err_rpm_put; > > + } > > + } else { > > + __ov02a10_stop_stream(ov02a10); > > + pm_runtime_put(&client->dev); > > + } > > + > > + ov02a10->streaming = on; > > + mutex_unlock(&ov02a10->mutex); > > + > > + return ret; > > + > > +err_rpm_put: > > + pm_runtime_put(&client->dev); > > +unlock_and_return: > > + mutex_unlock(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > +static int ov02a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_get_try_format(sd, > > + fh->pad, > > + 0); > > + > > + mutex_lock(&ov02a10->mutex); > > + /* Initialize try_fmt */ > > + try_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(&supported_modes[0], try_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > No need for the mutex: the supported values are static and there is no > parallel access to try_fmt. > > Btw. you don't need to do this here as you have init_cfg implemented, i.e. > you can omit the open callback. > Understood. mutex here would be removed in next release. > > + > > + return 0; > > +} > > + > > +static int __maybe_unused ov02a10_runtime_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + return __ov02a10_power_on(ov02a10); > > +} > > + > > +static int __maybe_unused ov02a10_runtime_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + __ov02a10_power_off(ov02a10); > > + > > + return 0; > > +} > > + > > +static const struct dev_pm_ops ov02a10_pm_ops = { > > + SET_RUNTIME_PM_OPS(ov02a10_runtime_suspend, > > + ov02a10_runtime_resume, NULL) > > +}; > > + > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, s32 value) > > +{ > > + if (value) > > + return ov02a10_write_array(ov02a10, > > + ov02a10_test_pattern_enable_regs); > > + > > + return ov02a10_write_array(ov02a10, > > + ov02a10_test_pattern_disable_regs); > > Fits on previous line. > Sorry for the typo. Fixed in next release. > > +} > > + > > +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct ov02a10 *ov02a10 = container_of(ctrl->handler, > > + struct ov02a10, ctrl_handler); > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + s64 max_expo; > > + int ret; > > + > > + /* Propagate change of current control to all related controls */ > > + if (ctrl->id == V4L2_CID_VBLANK) { > > + /* Update max exposure while meeting expected vblanking */ > > + max_expo = ov02a10->cur_mode->height + ctrl->val - 4; > > + __v4l2_ctrl_modify_range(ov02a10->exposure, > > + ov02a10->exposure->minimum, max_expo, > > + ov02a10->exposure->step, > > + ov02a10->exposure->default_value); > > + } > > + > > + /* V4L2 controls values will be applied only when power is already up */ > > + if (!pm_runtime_get_if_in_use(&client->dev)) > > + return 0; > > + > > + switch (ctrl->id) { > > + case V4L2_CID_EXPOSURE: > > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_H, > > + ((ctrl->val >> 8) & 0xFF)); > > + if (!ret) { > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_L, > > + (ctrl->val & 0xFF)); > > + if (ret < 0) > > + return ret; > > + } > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + break; > > + case V4L2_CID_ANALOGUE_GAIN: > > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_GAIN, > > + (ctrl->val & 0xFF)); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + break; > > + case V4L2_CID_VBLANK: > > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_H, > > + (((ctrl->val + > > + ov02a10->cur_mode->height - > > + OV02A10_BASIC_LINE) >> 8) > > + & 0xFF)); > > + if (!ret) { > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_L, > > + ((ctrl->val + > > + ov02a10->cur_mode->height - > > + OV02A10_BASIC_LINE) & 0xFF)); > > + if (ret < 0) > > + return ret; > > + } > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + break; > > + case V4L2_CID_TEST_PATTERN: > > + ret = ov02a10_set_test_pattern(ov02a10, ctrl->val); > > + if (ret < 0) > > + return ret; > > + break; > > + default: > > + dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n", > > + __func__, ctrl->id, ctrl->val); > > + ret = -EINVAL; > > + break; > > + }; > > + > > + pm_runtime_put(&client->dev); > > + > > + return ret; > > +} > > + > > +static const struct v4l2_subdev_video_ops ov02a10_video_ops = { > > + .s_stream = ov02a10_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = { > > + .init_cfg = ov02a10_entity_init_cfg, > > + .enum_mbus_code = ov02a10_enum_mbus_code, > > + .enum_frame_size = ov02a10_enum_frame_sizes, > > + .get_fmt = ov02a10_get_fmt, > > + .set_fmt = ov02a10_set_fmt, > > +}; > > + > > +static const struct v4l2_subdev_ops ov02a10_subdev_ops = { > > + .video = &ov02a10_video_ops, > > + .pad = &ov02a10_pad_ops, > > +}; > > + > > +static const struct media_entity_operations ov02a10_subdev_entity_ops = { > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > +static const struct v4l2_subdev_internal_ops ov02a10_internal_ops = { > > + .open = ov02a10_open, > > +}; > > + > > +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = { > > + .s_ctrl = ov02a10_set_ctrl, > > +}; > > + > > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + const struct ov02a10_mode *mode; > > + struct v4l2_ctrl_handler *handler; > > + struct v4l2_ctrl *ctrl; > > + u64 exposure_max; > > + u32 pixel_rate, h_blank; > > + int ret; > > + > > + handler = &ov02a10->ctrl_handler; > > + mode = ov02a10->cur_mode; > > + ret = v4l2_ctrl_handler_init(handler, 10); > > + if (ret) > > + return ret; > > An extra newline here, please. > Sorry for the typo. Fixed in next release. > > + handler->lock = &ov02a10->mutex; > > + > > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, > > + 0, 0, link_freq_menu_items); > > + if (ctrl) > > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + pixel_rate = (link_freq_menu_items[0] * 2 * OV02A10_LANES) / > > + OV02A10_BITS_PER_SAMPLE; > > Please use do_div(). The values are static so the compiler may not emit the > instructions for division but better be safe than sorry. > Fixed in next release. > > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, > > + 0, pixel_rate, 1, pixel_rate); > > + > > + h_blank = mode->hts_def - mode->width; > > + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK, > > + h_blank, h_blank, 1, h_blank); > > + if (ov02a10->hblank) > > + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_VBLANK, mode->vts_def - > > + mode->height, > > + OV02A10_VTS_MAX - mode->height, 1, > > + mode->vts_def - mode->height); > > + > > + exposure_max = mode->vts_def - 4; > > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_EXPOSURE, > > + OV02A10_EXPOSURE_MIN, > > + exposure_max, > > + OV02A10_EXPOSURE_STEP, > > + mode->exp_def); > > + > > + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, > > + OV02A10_GAIN_MIN, > > + OV02A10_GAIN_MAX, > > + OV02A10_GAIN_STEP, > > + OV02A10_GAIN_DEFAULT); > > + > > + ov02a10->test_pattern = > > + v4l2_ctrl_new_std_menu_items(handler, > > Indentation. If some of the arguments slip over 80 I guess we can live with > that. > Got it. Fixed in next release. > > + &ov02a10_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov02a10_test_pattern_menu) - > > + 1, 0, 0, ov02a10_test_pattern_menu); > > + > > + if (handler->error) { > > + ret = handler->error; > > + dev_err(&client->dev, > > + "Failed to init controls(%d)\n", ret); > > Fits on the previous line. > Fixed in next release. > > + goto err_free_handler; > > + } > > + > > + ov02a10->subdev.ctrl_handler = handler; > > + > > + return 0; > > + > > +err_free_handler: > > + v4l2_ctrl_handler_free(handler); > > + > > + return ret; > > +} > > + > > +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + u16 id; > > + u8 pid = 0; > > + u8 ver = 0; > > + int ret; > > + > > + /* Check sensor revision */ > > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_H, &pid); > > + if (ret) > > + return ret; > > + > > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_L, &ver); > > + if (ret) > > + return ret; > > + > > + id = OV02A10_ID(pid, ver); > > + if (id != CHIP_ID) { > > + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id); > > + return ret; > > + } > > + > > + dev_info(&client->dev, "Detected OV%04X sensor\n", id); > > + > > + return 0; > > +} > > + > > +static int ov02a10_configure_regulators(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + unsigned int i; > > + > > + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++) > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > + > > + return devm_regulator_bulk_get(&client->dev, > > + OV02A10_NUM_SUPPLIES, > > + ov02a10->supplies); > > +} > > + > > +static int ov02a10_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct ov02a10 *ov02a10; > > + u32 rotation; > > + u32 xclk_freq; > > + int ret; > > + > > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > > + if (!ov02a10) > > + return -ENOMEM; > > + > > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > > + > > + /* Optional indication of physical rotation of sensor */ > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", > > + &rotation); > > + if (!ret) { > > + switch (rotation) { > > + case 180: > > + ov02a10->upside_down = true; > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > + break; > > + case 0: > > + break; > > + default: > > + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", > > + rotation); > > + } > > + } > > + > > Please also check that the link frequencies in DT match with what the > driver expects. See e.g. drivers/media/i2c/ov8856.c driver for an example. > I would have a try and set one check rule for link frequencies. > > + /* Get system clock (xvclk) */ > > + ov02a10->xvclk = devm_clk_get(dev, "xvclk"); > > + if (IS_ERR(ov02a10->xvclk)) { > > + dev_err(dev, "Failed to get xvclk\n"); > > + return -EINVAL; > > + } > > + > > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > > + if (ret) { > > + dev_err(dev, "Failed to get xclk frequency\n"); > > + return ret; > > + } > > + > > + /* External clock must be 24MHz, allow 1% tolerance */ > > + if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > + dev_err(dev, "external clock frequency %u is not supported\n", > > + xclk_freq); > > + return -EINVAL; > > + } > > + dev_dbg(dev, "external clock frequency %u\n", xclk_freq); > > + > > + ret = clk_set_rate(ov02a10->xvclk, xclk_freq); > > + if (ret) { > > + dev_err(dev, "Failed to set xvclk frequency (24MHz)\n"); > > + return ret; > > + } > > + > > + ov02a10->powerdown_gpio = devm_gpiod_get(dev, "powerdown", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(ov02a10->powerdown_gpio)) { > > + dev_err(dev, "Failed to get powerdown-gpios\n"); > > + return -EINVAL; > > + } > > + > > + ov02a10->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > > + if (IS_ERR(ov02a10->reset_gpio)) { > > + dev_err(dev, "Failed to get reset-gpios\n"); > > + return -EINVAL; > > + } > > + > > + ret = ov02a10_configure_regulators(ov02a10); > > + if (ret) { > > + dev_err(dev, "Failed to get power regulators\n"); > > + return ret; > > + } > > + > > + mutex_init(&ov02a10->mutex); > > + ov02a10->cur_mode = &supported_modes[0]; > > + ret = ov02a10_initialize_controls(ov02a10); > > + if (ret) { > > + dev_err(dev, "Failed to initialize controls\n"); > > + goto err_destroy_mutex; > > + } > > + > > + ret = __ov02a10_power_on(ov02a10); > > + if (ret) > > + goto err_free_handler; > > + > > + ret = ov02a10_check_sensor_id(ov02a10); > > + if (ret) > > + goto err_power_off; > > + > > + ov02a10->subdev.internal_ops = &ov02a10_internal_ops; > > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > > + if (ret < 0) { > > + dev_err(dev, "failed to init entity pads: %d", ret); > > + goto err_power_off; > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", > > + ret); > > + goto err_clean_entity; > > + } > > + > > + pm_runtime_set_active(dev); > > + pm_runtime_enable(dev); > > + pm_runtime_idle(dev); > > + > > + dev_info(dev, "ov02a10 probe --\n"); > > This isn't very informative. The driver already prints errors when probing > fails and otherwise it succeeds. Please remove. > This would be removed in next release. > > + return 0; > > + > > +err_clean_entity: > > + media_entity_cleanup(&ov02a10->subdev.entity); > > +err_power_off: > > + __ov02a10_power_off(ov02a10); > > +err_free_handler: > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > +err_destroy_mutex: > > + mutex_destroy(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > +static int ov02a10_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + media_entity_cleanup(&sd->entity); > > + v4l2_ctrl_handler_free(sd->ctrl_handler); > > + pm_runtime_disable(&client->dev); > > + if (!pm_runtime_status_suspended(&client->dev)) > > + __ov02a10_power_off(ov02a10); > > + pm_runtime_set_suspended(&client->dev); > > + mutex_destroy(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +#if IS_ENABLED(CONFIG_OF) > > +static const struct of_device_id ov02a10_of_match[] = { > > + { .compatible = "ovti,ov02a10" }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > > +#endif > > + > > +static struct i2c_driver ov02a10_i2c_driver = { > > + .driver = { > > + .name = "ov02a10", > > + .pm = &ov02a10_pm_ops, > > + .of_match_table = ov02a10_of_match, > > + }, > > + .probe_new = &ov02a10_probe, > > + .remove = &ov02a10_remove, > > +}; > > + > > +module_i2c_driver(ov02a10_i2c_driver); > > + > > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>"); > > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver"); > > +MODULE_LICENSE("GPL v2"); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-09-05 9:41 ` Dongchun Zhu @ 2019-09-05 10:45 ` Sakari Ailus 2019-09-05 10:53 ` Tomasz Figa 0 siblings, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2019-09-05 10:45 UTC (permalink / raw) To: Dongchun Zhu Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang, tfiga, louis.kuo, sj.huang, robh+dt, linux-mediatek, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media Hi Dongchun, On Thu, Sep 05, 2019 at 05:41:05PM +0800, Dongchun Zhu wrote: ... > > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > > + if (ret < 0) { > > > + dev_err(dev, "Failed to enable regulators\n"); > > > + goto disable_clk; > > > + } > > > + msleep_range(7); > > > > This has some potential of clashing with more generic functions in the > > future. Please use usleep_range directly, or msleep. > > > > Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1? > https://patchwork.kernel.org/patch/10957225/ Yes, please. -- Sakari Ailus sakari.ailus@linux.intel.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-09-05 10:45 ` Sakari Ailus @ 2019-09-05 10:53 ` Tomasz Figa 2019-09-05 16:05 ` Sakari Ailus 0 siblings, 1 reply; 16+ messages in thread From: Tomasz Figa @ 2019-09-05 10:53 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, Nicolas Boichat, srv_heupstream, shengnan.wang, Louis Kuo, Sj Huang, Rob Herring, moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, , Linux Media Mailing List On Thu, Sep 5, 2019 at 7:45 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Dongchun, > > On Thu, Sep 05, 2019 at 05:41:05PM +0800, Dongchun Zhu wrote: > > ... > > > > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > > > + if (ret < 0) { > > > > + dev_err(dev, "Failed to enable regulators\n"); > > > > + goto disable_clk; > > > > + } > > > > + msleep_range(7); > > > > > > This has some potential of clashing with more generic functions in the > > > future. Please use usleep_range directly, or msleep. > > > > > > > Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1? > > https://patchwork.kernel.org/patch/10957225/ > > Yes, please. Why not just msleep()? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-09-05 10:53 ` Tomasz Figa @ 2019-09-05 16:05 ` Sakari Ailus 2019-09-05 22:58 ` Nicolas Boichat 0 siblings, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2019-09-05 16:05 UTC (permalink / raw) To: Tomasz Figa Cc: Mark Rutland, devicetree, Nicolas Boichat, srv_heupstream, shengnan.wang, Louis Kuo, Sj Huang, Rob Herring, moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, , Linux Media Mailing List On Thu, Sep 05, 2019 at 07:53:37PM +0900, Tomasz Figa wrote: > On Thu, Sep 5, 2019 at 7:45 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Dongchun, > > > > On Thu, Sep 05, 2019 at 05:41:05PM +0800, Dongchun Zhu wrote: > > > > ... > > > > > > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > > > > + if (ret < 0) { > > > > > + dev_err(dev, "Failed to enable regulators\n"); > > > > > + goto disable_clk; > > > > > + } > > > > > + msleep_range(7); > > > > > > > > This has some potential of clashing with more generic functions in the > > > > future. Please use usleep_range directly, or msleep. > > > > > > > > > > Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1? > > > https://patchwork.kernel.org/patch/10957225/ > > > > Yes, please. > > Why not just msleep()? msleep() is usually less accurate. I'm not sure it makes a big different in this case. Perhaps, if someone wants that the sensor is powered on and streaming as soon as possible. -- Sakari Ailus sakari.ailus@linux.intel.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-09-05 16:05 ` Sakari Ailus @ 2019-09-05 22:58 ` Nicolas Boichat 2019-09-06 1:33 ` Dongchun Zhu 0 siblings, 1 reply; 16+ messages in thread From: Nicolas Boichat @ 2019-09-05 22:58 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, srv_heupstream, shengnan.wang, Tomasz Figa, Louis Kuo, Sj Huang, Rob Herring, moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, , Linux Media Mailing List On Fri, Sep 6, 2019 at 12:05 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > On Thu, Sep 05, 2019 at 07:53:37PM +0900, Tomasz Figa wrote: > > On Thu, Sep 5, 2019 at 7:45 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Dongchun, > > > > > > On Thu, Sep 05, 2019 at 05:41:05PM +0800, Dongchun Zhu wrote: > > > > > > ... > > > > > > > > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > > > > > + if (ret < 0) { > > > > > > + dev_err(dev, "Failed to enable regulators\n"); > > > > > > + goto disable_clk; > > > > > > + } > > > > > > + msleep_range(7); > > > > > > > > > > This has some potential of clashing with more generic functions in the > > > > > future. Please use usleep_range directly, or msleep. > > > > > > > > > > > > > Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1? > > > > https://patchwork.kernel.org/patch/10957225/ > > > > > > Yes, please. > > > > Why not just msleep()? > > msleep() is usually less accurate. I'm not sure it makes a big different in > this case. Perhaps, if someone wants that the sensor is powered on and > streaming as soon as possible. https://elixir.bootlin.com/linux/latest/source/Documentation/timers/timers-howto.txt#L70 Use usleep_range for delays up to 20ms (at least that's what the documentation (still) says?) > -- > Sakari Ailus > sakari.ailus@linux.intel.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-09-05 22:58 ` Nicolas Boichat @ 2019-09-06 1:33 ` Dongchun Zhu 2019-09-07 3:51 ` Tomasz Figa 0 siblings, 1 reply; 16+ messages in thread From: Dongchun Zhu @ 2019-09-06 1:33 UTC (permalink / raw) To: Nicolas Boichat Cc: Mark Rutland, devicetree, srv_heupstream, shengnan.wang, Tomasz Figa, Louis Kuo, Sj Huang, Rob Herring, moderated list:ARM/Mediatek SoC support, Sakari Ailus, Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, , Linux Media Mailing List On Fri, 2019-09-06 at 06:58 +0800, Nicolas Boichat wrote: > On Fri, Sep 6, 2019 at 12:05 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > On Thu, Sep 05, 2019 at 07:53:37PM +0900, Tomasz Figa wrote: > > > On Thu, Sep 5, 2019 at 7:45 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Hi Dongchun, > > > > > > > > On Thu, Sep 05, 2019 at 05:41:05PM +0800, Dongchun Zhu wrote: > > > > > > > > ... > > > > > > > > > > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > > > > > > + if (ret < 0) { > > > > > > > + dev_err(dev, "Failed to enable regulators\n"); > > > > > > > + goto disable_clk; > > > > > > > + } > > > > > > > + msleep_range(7); > > > > > > > > > > > > This has some potential of clashing with more generic functions in the > > > > > > future. Please use usleep_range directly, or msleep. > > > > > > > > > > > > > > > > Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1? > > > > > https://patchwork.kernel.org/patch/10957225/ > > > > > > > > Yes, please. > > > > > > Why not just msleep()? > > > > msleep() is usually less accurate. I'm not sure it makes a big different in > > this case. Perhaps, if someone wants that the sensor is powered on and > > streaming as soon as possible. > > https://elixir.bootlin.com/linux/latest/source/Documentation/timers/timers-howto.txt#L70 > > Use usleep_range for delays up to 20ms (at least that's what the > documentation (still) says?) > Thank you for your clarifications. From the doc, "msleep(1~20) may not do what the caller intends, and will often sleep longer (~20 ms actual sleep for any value given in the 1~20ms range). In many cases this is not the desired behavior." So, it is supposed to use usleep_range in shorter sleep case, such as 5ms. > > -- > > Sakari Ailus > > sakari.ailus@linux.intel.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-09-06 1:33 ` Dongchun Zhu @ 2019-09-07 3:51 ` Tomasz Figa 0 siblings, 0 replies; 16+ messages in thread From: Tomasz Figa @ 2019-09-07 3:51 UTC (permalink / raw) To: Dongchun Zhu Cc: Mark Rutland, devicetree, Nicolas Boichat, srv_heupstream, shengnan.wang, Louis Kuo, Sj Huang, Rob Herring, moderated list:ARM/Mediatek SoC support, Sakari Ailus, Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, , Linux Media Mailing List On Fri, Sep 6, 2019 at 10:33 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > On Fri, 2019-09-06 at 06:58 +0800, Nicolas Boichat wrote: > > On Fri, Sep 6, 2019 at 12:05 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > On Thu, Sep 05, 2019 at 07:53:37PM +0900, Tomasz Figa wrote: > > > > On Thu, Sep 5, 2019 at 7:45 PM Sakari Ailus > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > Hi Dongchun, > > > > > > > > > > On Thu, Sep 05, 2019 at 05:41:05PM +0800, Dongchun Zhu wrote: > > > > > > > > > > ... > > > > > > > > > > > > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > > > > > > > + if (ret < 0) { > > > > > > > > + dev_err(dev, "Failed to enable regulators\n"); > > > > > > > > + goto disable_clk; > > > > > > > > + } > > > > > > > > + msleep_range(7); > > > > > > > > > > > > > > This has some potential of clashing with more generic functions in the > > > > > > > future. Please use usleep_range directly, or msleep. > > > > > > > > > > > > > > > > > > > Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1? > > > > > > https://patchwork.kernel.org/patch/10957225/ > > > > > > > > > > Yes, please. > > > > > > > > Why not just msleep()? > > > > > > msleep() is usually less accurate. I'm not sure it makes a big different in > > > this case. Perhaps, if someone wants that the sensor is powered on and > > > streaming as soon as possible. > > > > https://elixir.bootlin.com/linux/latest/source/Documentation/timers/timers-howto.txt#L70 > > > > Use usleep_range for delays up to 20ms (at least that's what the > > documentation (still) says?) > > > > Thank you for your clarifications. > From the doc, > "msleep(1~20) may not do what the caller intends, and > will often sleep longer (~20 ms actual sleep for any > value given in the 1~20ms range). In many cases this > is not the desired behavior." > > So, it is supposed to use usleep_range in shorter sleep case, > such as 5ms. Thanks for double checking. usleep_range() sounds good then. Sorry for the noise. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-08-19 3:43 ` [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver dongchun.zhu 2019-08-19 8:30 ` Sakari Ailus @ 2019-08-21 10:30 ` Tomasz Figa 2019-08-21 11:05 ` Sakari Ailus 2019-09-05 15:54 ` Dongchun Zhu 1 sibling, 2 replies; 16+ messages in thread From: Tomasz Figa @ 2019-08-21 10:30 UTC (permalink / raw) To: dongchun.zhu Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang, louis.kuo, sj.huang, robh+dt, linux-mediatek, sakari.ailus, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media Hi Dongchun, On Mon, Aug 19, 2019 at 11:43:31AM +0800, dongchun.zhu@mediatek.com wrote: > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > This patch adds a V4L2 sub-device driver for OV02A10 image sensor. > The OV02A10 is a 1/5" CMOS sensor from Omnivision, > which supports output format: 10-bit Raw. > The OV02A10 has a single MIPI lane interface and use the I2C bus > for control and the CSI-2 bus for data. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > MAINTAINERS | 1 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov02a10.c | 1018 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1031 insertions(+) > create mode 100644 drivers/media/i2c/ov02a10.c > Thanks for the patch! Please see my comments inline. [snip] > +#define CHIP_ID 0x2509 > +#define OV02A10_REG_CHIP_ID_H 0x02 > +#define OV02A10_REG_CHIP_ID_L 0x03 > +#define OV02A10_ID(_msb, _lsb) ((_msb) << 8 | (_lsb)) > + > +/* Bit[1] vertical upside down */ > +/* Bit[0] horizontal mirror */ > +#define REG_MIRROR_FLIP_CONTROL 0x3f > + > +/* Orientation */ > +#define REG_CONFIG_MIRROR_FLIP 0x03 > + > +#define REG_PAGE_SWITCH 0xfd > +#define REG_GLOBAL_EFFECTIVE 0x01 > +#define REG_ENABLE BIT(0) > + > +#define REG_SC_CTRL_MODE 0xac > +#define SC_CTRL_MODE_STANDBY 0x00 > +#define SC_CTRL_MODE_STREAMING 0x01 > + > +#define OV02A10_REG_EXPOSURE_H 0x03 > +#define OV02A10_REG_EXPOSURE_L 0x04 > +#define OV02A10_EXPOSURE_MIN 4 > +#define OV02A10_EXPOSURE_STEP 1 > + > +#define OV02A10_REG_VTS_H 0x05 > +#define OV02A10_REG_VTS_L 0x06 > +#define OV02A10_VTS_MAX 0x209f > +#define OV02A10_VTS_MIN 0x04cf > +#define OV02A10_BASIC_LINE 1224 > + > +#define OV02A10_REG_GAIN 0x24 > +#define OV02A10_GAIN_MIN 0x10 > +#define OV02A10_GAIN_MAX 0xf8 > +#define OV02A10_GAIN_STEP 0x01 > +#define OV02A10_GAIN_DEFAULT 0x40 > + > +#define REG_NULL 0xff > + > +#define OV02A10_LANES 1 > +#define OV02A10_BITS_PER_SAMPLE 10 > + I think there is something wrong with the indentation in the code above. Please use tabs wherever possible. [snip] > + > +#define to_ov02a10(sd) container_of(sd, struct ov02a10, subdev) Please use a static inline function for added compile-time type checks. > + > +static inline void msleep_range(unsigned int delay_base) > +{ > + usleep_range(delay_base * 1000, delay_base * 1000 + 500); > +} Why not just use msleep()? > + > +/* MIPI color bar enable output */ > +static const struct regval ov02a10_test_pattern_enable_regs[] = { > + {0xfd, 0x01}, > + {0x0d, 0x00}, > + {0xb6, 0x01}, > + {0x01, 0x01}, > + {0xfd, 0x01}, > + {0xac, 0x01}, > + {REG_NULL, 0x00} > +}; > + > +/* MIPI color bar disable output */ > +static const struct regval ov02a10_test_pattern_disable_regs[] = { > + {0xfd, 0x01}, > + {0x0d, 0x00}, > + {0xb6, 0x00}, > + {0x01, 0x01}, > + {0xfd, 0x01}, > + {0xac, 0x01}, > + {REG_NULL, 0x00} > +}; Hmm, only the register 0xb6 seems to here. Could we just set it directly, without these arrays? > + > +/* > + * xvclk 24Mhz This seems to assume 24MHz, but the driver allows a range in probe. Is that correct? [snip]] > +/* Write a register */ > +static int ov02a10_write_reg(struct ov02a10 *ov02a10, u8 addr, u8 val) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + u8 buf[2] = {addr, val}; > + int ret; > + > + ret = i2c_master_send(client, buf, 2); > + > + if (ret != 2) { > + dev_err(&client->dev, "%s: error: reg=%x, val=%x\n", > + __func__, addr, val); > + return -EIO; > + } > + > + return 0; > +} Could this be replaced with i2c_smbus_write_byte_data()? > + > +static int ov02a10_write_array(struct ov02a10 *ov02a10, > + const struct regval *regs) > +{ > + u32 i; > + int ret; > + > + for (i = 0; regs[i].addr != REG_NULL; i++) { > + ret = ov02a10_write_reg(ov02a10, regs[i].addr, regs[i].val); > + if (ret < 0) > + return ret; > + } > + > + return 0; > +} > + > +/* Read a register */ > +static int ov02a10_read_reg(struct ov02a10 *ov02a10, u8 reg, u8 *val) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + u8 data = reg; > + struct i2c_msg msg = { > + .addr = client->addr, > + .flags = 0, > + .len = 1, > + .buf = &data, > + }; > + int ret; > + > + ret = i2c_transfer(client->adapter, &msg, 1); > + if (ret < 0) > + goto err_wr; > + > + msg.flags = I2C_M_RD; > + ret = i2c_transfer(client->adapter, &msg, 1); > + if (ret < 0) > + goto err_rd; Could we just have 2 messages in an array and just call i2c_transfer() once for both write and read? Or actually it sounds like the i2c_smbus_read_byte_data() helper could work here. > + > + *val = data; > + return 0; > + > +err_rd: > + dev_err(&client->dev, "i2c_transfer --I2C_M_RD failed\n"); > +err_wr: > + dev_err(&client->dev, "read error: reg=0x%02x: %d\n", reg, ret); > + return ret; > +} > + > +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode, > + struct v4l2_mbus_framefmt *fmt) > +{ > + fmt->width = mode->width; > + fmt->height = mode->height; > + fmt->field = V4L2_FIELD_NONE; > +} > + > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + mutex_lock(&ov02a10->mutex); > + > + if (ov02a10->streaming) { > + mutex_unlock(&ov02a10->mutex); > + return -EBUSY; > + } > + > + /* Only one sensor mode supported */ > + mbus_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > + ov02a10->fmt = fmt->format; > + > + mutex_unlock(&ov02a10->mutex); > + > + return 0; > +} > + > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + mutex_lock(&ov02a10->mutex); > + > + fmt->format = ov02a10->fmt; > + mbus_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > + > + mutex_unlock(&ov02a10->mutex); > + > + return 0; > +} > + > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + if (code->index >= ARRAY_SIZE(supported_modes) || !(code->index)) Hmm, ARRAY_SIZE(supported_modes) is 1 and we don't allow code->index to be 0 either. Is there a code->index value that wouldn't return an error here? > + return -EINVAL; > + > + code->code = ov02a10->fmt.code; > + > + return 0; > +} > + > +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->index >= ARRAY_SIZE(supported_modes) || !(fse->index)) Same here. > + return -EINVAL; > + > + fse->min_width = supported_modes[fse->index].width; > + fse->max_width = supported_modes[fse->index].width; > + fse->max_height = supported_modes[fse->index].height; > + fse->min_height = supported_modes[fse->index].height; > + > + return 0; > +} > + > +static int __ov02a10_power_on(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + struct device *dev = &client->dev; > + int ret; > + > + ret = clk_prepare_enable(ov02a10->xvclk); > + if (ret < 0) { > + dev_err(dev, "Failed to enable xvclk\n"); > + return ret; > + } Is it really correct to enable the clock before the regulators? According to the datasheet, it should be: - PD pin HIGH, - nRST pin LOW, - DVDDIO and AVDD28 power up and stabilize, - clock enabled, - min 5 ms delay, - PD pin LOW, - min 4 ms delay, - nRST pin HIGH, - min 5 ms delay, - I2C interface ready. > + > + /* Note: set 0 is high, set 1 is low */ Why is that? If there is some inverter on the way that should be handled outside of this driver. (GPIO DT bindings have flags for this purpose. If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should call them like this in the driver too (+/- the lowercase and underscore convention). According to the datasheet, the reset pin is called RST and inverted, so we should call it n_rst, but the powerdown signal, called PD, is not inverted, so pd would be the right name. > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 0); > + > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > + if (ret < 0) { > + dev_err(dev, "Failed to enable regulators\n"); > + goto disable_clk; > + } > + msleep_range(7); > + > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > + msleep_range(10); > + > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 0); > + msleep_range(10); > + > + return 0; > + > +disable_clk: > + clk_disable_unprepare(ov02a10->xvclk); > + > + return ret; > +} > + > +static void __ov02a10_power_off(struct ov02a10 *ov02a10) > +{ > + clk_disable_unprepare(ov02a10->xvclk); > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); This also doesn't seem to match my datasheet. The sequence there is: - nRST goes LOW, - clock stops, - PD goes HIGH, - regulators are powerd down. > +} > + > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + int ret; > + > + /* Apply default values of current mode */ > + ret = ov02a10_write_array(ov02a10, ov02a10->cur_mode->reg_list); > + if (ret) > + return ret; > + > + /* Apply customized values from user */ > + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler); > + if (ret) > + return ret; > + > + /* Set orientation to 180 degree */ > + if (ov02a10->upside_down) { > + ret = ov02a10_write_reg(ov02a10, REG_MIRROR_FLIP_CONTROL, > + REG_CONFIG_MIRROR_FLIP); > + if (ret) { > + dev_err(&client->dev, "%s failed to set orientation\n", > + __func__); > + return ret; > + } > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + } > + > + /* Set stream on register */ > + return ov02a10_write_reg(ov02a10, > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STREAMING); > +} > + > +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10) > +{ > + return ov02a10_write_reg(ov02a10, > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STANDBY); > +} > + > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg) > +{ > + struct v4l2_subdev_format fmt = { 0 }; > + > + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; > + fmt.format.width = 1600; > + fmt.format.height = 1200; Where do these values come from? Should we have some macros for them? > + > + ov02a10_set_fmt(subdev, cfg, &fmt); > + > + return 0; > +} > + > +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + int ret = 0; > + > + mutex_lock(&ov02a10->mutex); > + > + if (ov02a10->streaming == on) > + goto unlock_and_return; > + > + if (on) { > + ret = pm_runtime_get_sync(&client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&client->dev); > + goto unlock_and_return; > + } > + > + ret = __ov02a10_start_stream(ov02a10); > + if (ret) { > + __ov02a10_stop_stream(ov02a10); > + ov02a10->streaming = !on; > + goto err_rpm_put; > + } > + } else { > + __ov02a10_stop_stream(ov02a10); > + pm_runtime_put(&client->dev); > + } > + > + ov02a10->streaming = on; > + mutex_unlock(&ov02a10->mutex); > + > + return ret; > + > +err_rpm_put: > + pm_runtime_put(&client->dev); > +unlock_and_return: > + mutex_unlock(&ov02a10->mutex); > + > + return ret; > +} > + > +static int ov02a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_get_try_format(sd, > + fh->pad, > + 0); Please separate the initialization from the declaration, because there isn't just enough space on this line anymore. > + > + mutex_lock(&ov02a10->mutex); > + /* Initialize try_fmt */ > + try_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(&supported_modes[0], try_fmt); > + > + mutex_unlock(&ov02a10->mutex); > + > + return 0; > +} > + > +static int __maybe_unused ov02a10_runtime_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + return __ov02a10_power_on(ov02a10); > +} > + > +static int __maybe_unused ov02a10_runtime_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + __ov02a10_power_off(ov02a10); > + > + return 0; > +} > + > +static const struct dev_pm_ops ov02a10_pm_ops = { > + SET_RUNTIME_PM_OPS(ov02a10_runtime_suspend, > + ov02a10_runtime_resume, NULL) Don't we need to implement and provide system PM ops too? > +}; > + > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, s32 value) > +{ > + if (value) > + return ov02a10_write_array(ov02a10, > + ov02a10_test_pattern_enable_regs); > + > + return ov02a10_write_array(ov02a10, > + ov02a10_test_pattern_disable_regs); > +} > + > +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct ov02a10 *ov02a10 = container_of(ctrl->handler, > + struct ov02a10, ctrl_handler); > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + s64 max_expo; > + int ret; > + > + /* Propagate change of current control to all related controls */ > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure while meeting expected vblanking */ > + max_expo = ov02a10->cur_mode->height + ctrl->val - 4; > + __v4l2_ctrl_modify_range(ov02a10->exposure, > + ov02a10->exposure->minimum, max_expo, > + ov02a10->exposure->step, > + ov02a10->exposure->default_value); > + } > + > + /* V4L2 controls values will be applied only when power is already up */ > + if (!pm_runtime_get_if_in_use(&client->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_EXPOSURE: > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_H, > + ((ctrl->val >> 8) & 0xFF)); > + if (!ret) { > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_L, > + (ctrl->val & 0xFF)); > + if (ret < 0) > + return ret; > + } > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + break; > + case V4L2_CID_ANALOGUE_GAIN: > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_GAIN, > + (ctrl->val & 0xFF)); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + break; > + case V4L2_CID_VBLANK: > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > + if (ret < 0) > + return ret; > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_H, > + (((ctrl->val + > + ov02a10->cur_mode->height - > + OV02A10_BASIC_LINE) >> 8) > + & 0xFF)); > + if (!ret) { > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_L, > + ((ctrl->val + > + ov02a10->cur_mode->height - > + OV02A10_BASIC_LINE) & 0xFF)); > + if (ret < 0) > + return ret; > + } > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + break; > + case V4L2_CID_TEST_PATTERN: > + ret = ov02a10_set_test_pattern(ov02a10, ctrl->val); > + if (ret < 0) > + return ret; > + break; > + default: > + dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n", > + __func__, ctrl->id, ctrl->val); > + ret = -EINVAL; > + break; We shouldn't need to handle this, as the control framework wouldn't call us with a control that we didn't register explicitly. > + }; > + > + pm_runtime_put(&client->dev); > + > + return ret; > +} > + > +static const struct v4l2_subdev_video_ops ov02a10_video_ops = { > + .s_stream = ov02a10_s_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = { > + .init_cfg = ov02a10_entity_init_cfg, > + .enum_mbus_code = ov02a10_enum_mbus_code, > + .enum_frame_size = ov02a10_enum_frame_sizes, > + .get_fmt = ov02a10_get_fmt, > + .set_fmt = ov02a10_set_fmt, > +}; > + > +static const struct v4l2_subdev_ops ov02a10_subdev_ops = { > + .video = &ov02a10_video_ops, > + .pad = &ov02a10_pad_ops, > +}; > + > +static const struct media_entity_operations ov02a10_subdev_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static const struct v4l2_subdev_internal_ops ov02a10_internal_ops = { > + .open = ov02a10_open, > +}; > + > +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = { > + .s_ctrl = ov02a10_set_ctrl, > +}; > + > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + const struct ov02a10_mode *mode; > + struct v4l2_ctrl_handler *handler; > + struct v4l2_ctrl *ctrl; > + u64 exposure_max; > + u32 pixel_rate, h_blank; > + int ret; > + > + handler = &ov02a10->ctrl_handler; > + mode = ov02a10->cur_mode; > + ret = v4l2_ctrl_handler_init(handler, 10); I can see 6 controls registered below. > + if (ret) > + return ret; > + handler->lock = &ov02a10->mutex; > + > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, > + 0, 0, link_freq_menu_items); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + pixel_rate = (link_freq_menu_items[0] * 2 * OV02A10_LANES) / > + OV02A10_BITS_PER_SAMPLE; > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, > + 0, pixel_rate, 1, pixel_rate); > + > + h_blank = mode->hts_def - mode->width; > + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK, > + h_blank, h_blank, 1, h_blank); > + if (ov02a10->hblank) > + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_VBLANK, mode->vts_def - > + mode->height, > + OV02A10_VTS_MAX - mode->height, 1, > + mode->vts_def - mode->height); > + > + exposure_max = mode->vts_def - 4; > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_EXPOSURE, > + OV02A10_EXPOSURE_MIN, > + exposure_max, > + OV02A10_EXPOSURE_STEP, > + mode->exp_def); > + > + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, > + OV02A10_GAIN_MIN, > + OV02A10_GAIN_MAX, > + OV02A10_GAIN_STEP, > + OV02A10_GAIN_DEFAULT); > + > + ov02a10->test_pattern = > + v4l2_ctrl_new_std_menu_items(handler, > + &ov02a10_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(ov02a10_test_pattern_menu) - > + 1, 0, 0, ov02a10_test_pattern_menu); > + > + if (handler->error) { > + ret = handler->error; > + dev_err(&client->dev, > + "Failed to init controls(%d)\n", ret); > + goto err_free_handler; > + } > + > + ov02a10->subdev.ctrl_handler = handler; > + > + return 0; > + > +err_free_handler: > + v4l2_ctrl_handler_free(handler); > + > + return ret; > +} > + > +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + u16 id; > + u8 pid = 0; > + u8 ver = 0; > + int ret; > + > + /* Check sensor revision */ > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_H, &pid); > + if (ret) > + return ret; > + > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_L, &ver); > + if (ret) > + return ret; > + > + id = OV02A10_ID(pid, ver); > + if (id != CHIP_ID) { > + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id); > + return ret; > + } > + > + dev_info(&client->dev, "Detected OV%04X sensor\n", id); > + > + return 0; > +} > + > +static int ov02a10_configure_regulators(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + unsigned int i; > + > + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++) > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > + > + return devm_regulator_bulk_get(&client->dev, > + OV02A10_NUM_SUPPLIES, > + ov02a10->supplies); > +} I think we can just have this directly inside probe. > + > +static int ov02a10_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct ov02a10 *ov02a10; > + u32 rotation; > + u32 xclk_freq; > + int ret; > + > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > + if (!ov02a10) > + return -ENOMEM; > + > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > + > + /* Optional indication of physical rotation of sensor */ > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", > + &rotation); > + if (!ret) { > + switch (rotation) { > + case 180: > + ov02a10->upside_down = true; > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > + break; > + case 0: > + break; > + default: > + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", > + rotation); > + } > + } > + > + /* Get system clock (xvclk) */ > + ov02a10->xvclk = devm_clk_get(dev, "xvclk"); > + if (IS_ERR(ov02a10->xvclk)) { > + dev_err(dev, "Failed to get xvclk\n"); > + return -EINVAL; > + } Hmm, it's called eclk in my datasheet. > + > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > + if (ret) { > + dev_err(dev, "Failed to get xclk frequency\n"); > + return ret; > + } > + > + /* External clock must be 24MHz, allow 1% tolerance */ > + if (xclk_freq < 23760000 || xclk_freq > 24240000) { How do we support a range of frequencies? I don't see the driver calculate any register values based on this frequency. Are you sure that the register arrays don't assume one specific frequency? > + dev_err(dev, "external clock frequency %u is not supported\n", > + xclk_freq); > + return -EINVAL; > + } > + dev_dbg(dev, "external clock frequency %u\n", xclk_freq); > + > + ret = clk_set_rate(ov02a10->xvclk, xclk_freq); > + if (ret) { > + dev_err(dev, "Failed to set xvclk frequency (24MHz)\n"); > + return ret; > + } > + > + ov02a10->powerdown_gpio = devm_gpiod_get(dev, "powerdown", > + GPIOD_OUT_LOW); Hmm, shouldn't this be HIGH? At least the datasheet has it so for the powered down state. > + if (IS_ERR(ov02a10->powerdown_gpio)) { > + dev_err(dev, "Failed to get powerdown-gpios\n"); > + return -EINVAL; > + } > + > + ov02a10->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); Also LOW here, similarly to the above. > + if (IS_ERR(ov02a10->reset_gpio)) { > + dev_err(dev, "Failed to get reset-gpios\n"); > + return -EINVAL; > + } > + > + ret = ov02a10_configure_regulators(ov02a10); > + if (ret) { > + dev_err(dev, "Failed to get power regulators\n"); > + return ret; > + } > + > + mutex_init(&ov02a10->mutex); > + ov02a10->cur_mode = &supported_modes[0]; > + ret = ov02a10_initialize_controls(ov02a10); > + if (ret) { > + dev_err(dev, "Failed to initialize controls\n"); > + goto err_destroy_mutex; > + } > + > + ret = __ov02a10_power_on(ov02a10); > + if (ret) > + goto err_free_handler; > + > + ret = ov02a10_check_sensor_id(ov02a10); > + if (ret) > + goto err_power_off; > + > + ov02a10->subdev.internal_ops = &ov02a10_internal_ops; > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > + if (ret < 0) { > + dev_err(dev, "failed to init entity pads: %d", ret); > + goto err_power_off; > + } > + > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > + if (ret) { > + dev_err(dev, "failed to register V4L2 subdev: %d", > + ret); > + goto err_clean_entity; > + } > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_idle(dev); > + > + dev_info(dev, "ov02a10 probe --\n"); Please remove this. > + return 0; > + > +err_clean_entity: > + media_entity_cleanup(&ov02a10->subdev.entity); > +err_power_off: > + __ov02a10_power_off(ov02a10); > +err_free_handler: > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > +err_destroy_mutex: > + mutex_destroy(&ov02a10->mutex); > + > + return ret; > +} > + > +static int ov02a10_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + v4l2_async_unregister_subdev(sd); > + media_entity_cleanup(&sd->entity); > + v4l2_ctrl_handler_free(sd->ctrl_handler); > + pm_runtime_disable(&client->dev); > + if (!pm_runtime_status_suspended(&client->dev)) > + __ov02a10_power_off(ov02a10); > + pm_runtime_set_suspended(&client->dev); > + mutex_destroy(&ov02a10->mutex); > + > + return 0; > +} > + > +#if IS_ENABLED(CONFIG_OF) > +static const struct of_device_id ov02a10_of_match[] = { > + { .compatible = "ovti,ov02a10" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > +#endif > + > +static struct i2c_driver ov02a10_i2c_driver = { > + .driver = { > + .name = "ov02a10", > + .pm = &ov02a10_pm_ops, > + .of_match_table = ov02a10_of_match, Please use of_match_ptr() wrapper. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-08-21 10:30 ` Tomasz Figa @ 2019-08-21 11:05 ` Sakari Ailus 2019-08-26 6:54 ` Tomasz Figa 2019-09-05 15:54 ` Dongchun Zhu 1 sibling, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2019-08-21 11:05 UTC (permalink / raw) To: Tomasz Figa Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang, louis.kuo, sj.huang, robh+dt, linux-mediatek, dongchun.zhu, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media Hi Tomasz, On Wed, Aug 21, 2019 at 07:30:38PM +0900, Tomasz Figa wrote: ... > > + > > +/* > > + * xvclk 24Mhz > > This seems to assume 24MHz, but the driver allows a range in probe. Is that > correct? I think it'd be better to check for an exact frequency: this is board specific and its exact value is known. ... > > +static int __ov02a10_power_on(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + struct device *dev = &client->dev; > > + int ret; > > + > > + ret = clk_prepare_enable(ov02a10->xvclk); > > + if (ret < 0) { > > + dev_err(dev, "Failed to enable xvclk\n"); > > + return ret; > > + } > > Is it really correct to enable the clock before the regulators? > > According to the datasheet, it should be: > - PD pin HIGH, > - nRST pin LOW, > - DVDDIO and AVDD28 power up and stabilize, > - clock enabled, > - min 5 ms delay, > - PD pin LOW, > - min 4 ms delay, > - nRST pin HIGH, > - min 5 ms delay, > - I2C interface ready. > > > + > > + /* Note: set 0 is high, set 1 is low */ > > Why is that? If there is some inverter on the way that should be handled > outside of this driver. (GPIO DT bindings have flags for this purpose. > > If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should > call them like this in the driver too (+/- the lowercase and underscore > convention). > > According to the datasheet, the reset pin is called RST and inverted, so we should > call it n_rst, but the powerdown signal, called PD, is not inverted, so pd > would be the right name. For what it's worth sensors generally have xshutdown (or reset) pin that is active high. Looking at the code, it is not the case here. It's a bit odd since the usual arrangement saves power when the camera is not in use; it's not a lot but still. Oh well. ... > > +static struct i2c_driver ov02a10_i2c_driver = { > > + .driver = { > > + .name = "ov02a10", > > + .pm = &ov02a10_pm_ops, > > + .of_match_table = ov02a10_of_match, > > Please use of_match_ptr() wrapper. Not really needed; the driver does expect regulators, GPIOs etc., but by leaving out of_match_ptr(), the driver will also probe on ACPI based systems. -- Regards, Sakari Ailus sakari.ailus@linux.intel.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-08-21 11:05 ` Sakari Ailus @ 2019-08-26 6:54 ` Tomasz Figa 0 siblings, 0 replies; 16+ messages in thread From: Tomasz Figa @ 2019-08-26 6:54 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, Nicolas Boichat, srv_heupstream, shengnan.wang, Louis Kuo, Sj Huang, Rob Herring, moderated list:ARM/Mediatek SoC support, dongchun.zhu, Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, , Linux Media Mailing List On Wed, Aug 21, 2019 at 8:05 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Tomasz, > > On Wed, Aug 21, 2019 at 07:30:38PM +0900, Tomasz Figa wrote: [snip] > > Is it really correct to enable the clock before the regulators? > > > > According to the datasheet, it should be: > > - PD pin HIGH, > > - nRST pin LOW, > > - DVDDIO and AVDD28 power up and stabilize, > > - clock enabled, > > - min 5 ms delay, > > - PD pin LOW, > > - min 4 ms delay, > > - nRST pin HIGH, > > - min 5 ms delay, > > - I2C interface ready. > > > > > + > > > + /* Note: set 0 is high, set 1 is low */ > > > > Why is that? If there is some inverter on the way that should be handled > > outside of this driver. (GPIO DT bindings have flags for this purpose. > > > > If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should > > call them like this in the driver too (+/- the lowercase and underscore > > convention). > > > > According to the datasheet, the reset pin is called RST and inverted, so we should > > call it n_rst, but the powerdown signal, called PD, is not inverted, so pd > > would be the right name. > > For what it's worth sensors generally have xshutdown (or reset) pin that is > active high. Looking at the code, it is not the case here. It's a bit odd > since the usual arrangement saves power when the camera is not in use; it's > not a lot but still. Oh well. > I guess we could drive powerdown low after disabling the regulators and clocks, but that wouldn't work for the cases where the regulators are actually shared with something else, especially if that is not related to the same camera module. > ... > > > > +static struct i2c_driver ov02a10_i2c_driver = { > > > + .driver = { > > > + .name = "ov02a10", > > > + .pm = &ov02a10_pm_ops, > > > + .of_match_table = ov02a10_of_match, > > > > Please use of_match_ptr() wrapper. > > Not really needed; the driver does expect regulators, GPIOs etc., but by > leaving out of_match_ptr(), the driver will also probe on ACPI based > systems. Good point, I always keep forgetting about the ability to probe OF drivers from ACPI. Then we also need to remove the #if IS_ENABLED(CONFIG_OF) from ov02a10_of_match. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver 2019-08-21 10:30 ` Tomasz Figa 2019-08-21 11:05 ` Sakari Ailus @ 2019-09-05 15:54 ` Dongchun Zhu 1 sibling, 0 replies; 16+ messages in thread From: Dongchun Zhu @ 2019-09-05 15:54 UTC (permalink / raw) To: Tomasz Figa Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang, louis.kuo, sj.huang, robh+dt, linux-mediatek, sakari.ailus, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media Hi Tomasz, Thanks very much for your careful review. Reply to the comments see belows. Any missing please let me know. On Wed, 2019-08-21 at 19:30 +0900, Tomasz Figa wrote: > Hi Dongchun, > > On Mon, Aug 19, 2019 at 11:43:31AM +0800, dongchun.zhu@mediatek.com wrote: > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > This patch adds a V4L2 sub-device driver for OV02A10 image sensor. > > The OV02A10 is a 1/5" CMOS sensor from Omnivision, > > which supports output format: 10-bit Raw. > > The OV02A10 has a single MIPI lane interface and use the I2C bus > > for control and the CSI-2 bus for data. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 11 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1018 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1031 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > Thanks for the patch! Please see my comments inline. > > [snip] > > > +#define CHIP_ID 0x2509 > > +#define OV02A10_REG_CHIP_ID_H 0x02 > > +#define OV02A10_REG_CHIP_ID_L 0x03 > > +#define OV02A10_ID(_msb, _lsb) ((_msb) << 8 | (_lsb)) > > + > > +/* Bit[1] vertical upside down */ > > +/* Bit[0] horizontal mirror */ > > +#define REG_MIRROR_FLIP_CONTROL 0x3f > > + > > +/* Orientation */ > > +#define REG_CONFIG_MIRROR_FLIP 0x03 > > + > > +#define REG_PAGE_SWITCH 0xfd > > +#define REG_GLOBAL_EFFECTIVE 0x01 > > +#define REG_ENABLE BIT(0) > > + > > +#define REG_SC_CTRL_MODE 0xac > > +#define SC_CTRL_MODE_STANDBY 0x00 > > +#define SC_CTRL_MODE_STREAMING 0x01 > > + > > +#define OV02A10_REG_EXPOSURE_H 0x03 > > +#define OV02A10_REG_EXPOSURE_L 0x04 > > +#define OV02A10_EXPOSURE_MIN 4 > > +#define OV02A10_EXPOSURE_STEP 1 > > + > > +#define OV02A10_REG_VTS_H 0x05 > > +#define OV02A10_REG_VTS_L 0x06 > > +#define OV02A10_VTS_MAX 0x209f > > +#define OV02A10_VTS_MIN 0x04cf > > +#define OV02A10_BASIC_LINE 1224 > > + > > +#define OV02A10_REG_GAIN 0x24 > > +#define OV02A10_GAIN_MIN 0x10 > > +#define OV02A10_GAIN_MAX 0xf8 > > +#define OV02A10_GAIN_STEP 0x01 > > +#define OV02A10_GAIN_DEFAULT 0x40 > > + > > +#define REG_NULL 0xff > > + > > +#define OV02A10_LANES 1 > > +#define OV02A10_BITS_PER_SAMPLE 10 > > + > > I think there is something wrong with the indentation in the code above. > Please use tabs wherever possible. > Sorry for the typo. Fixed in next release. > [snip] > > > + > > +#define to_ov02a10(sd) container_of(sd, struct ov02a10, subdev) > > Please use a static inline function for added compile-time type checks. > Fixed in next release. > > + > > +static inline void msleep_range(unsigned int delay_base) > > +{ > > + usleep_range(delay_base * 1000, delay_base * 1000 + 500); > > +} > > Why not just use msleep()? > Generally for shorter sleep(e.g.less than 10ms), usleep_range is preferred. We would use msleep for bigger sleep. > > + > > +/* MIPI color bar enable output */ > > +static const struct regval ov02a10_test_pattern_enable_regs[] = { > > + {0xfd, 0x01}, > > + {0x0d, 0x00}, > > + {0xb6, 0x01}, > > + {0x01, 0x01}, > > + {0xfd, 0x01}, > > + {0xac, 0x01}, > > + {REG_NULL, 0x00} > > +}; > > + > > +/* MIPI color bar disable output */ > > +static const struct regval ov02a10_test_pattern_disable_regs[] = { > > + {0xfd, 0x01}, > > + {0x0d, 0x00}, > > + {0xb6, 0x00}, > > + {0x01, 0x01}, > > + {0xfd, 0x01}, > > + {0xac, 0x01}, > > + {REG_NULL, 0x00} > > +}; > > Hmm, only the register 0xb6 seems to here. Could we just set it directly, > without these arrays? > R0x0d is another color bar control register. But R0x01 and R0xac both are essential, which could make color bar register writing behavior global effective. > > + > > +/* > > + * xvclk 24Mhz > > This seems to assume 24MHz, but the driver allows a range in probe. Is that > correct? > That's OK. As the input clock for sensor, the frequency of Master Clock(xvclk) is regularly 24MHz. But actually the sensor xvclk could allow some little tolerance. For instance, 1%. > [snip]] > > > +/* Write a register */ > > +static int ov02a10_write_reg(struct ov02a10 *ov02a10, u8 addr, u8 val) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + u8 buf[2] = {addr, val}; > > + int ret; > > + > > + ret = i2c_master_send(client, buf, 2); > > + > > + if (ret != 2) { > > + dev_err(&client->dev, "%s: error: reg=%x, val=%x\n", > > + __func__, addr, val); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > Could this be replaced with i2c_smbus_write_byte_data()? > We would have a try using this generic API. > > + > > +static int ov02a10_write_array(struct ov02a10 *ov02a10, > > + const struct regval *regs) > > +{ > > + u32 i; > > + int ret; > > + > > + for (i = 0; regs[i].addr != REG_NULL; i++) { > > + ret = ov02a10_write_reg(ov02a10, regs[i].addr, regs[i].val); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +/* Read a register */ > > +static int ov02a10_read_reg(struct ov02a10 *ov02a10, u8 reg, u8 *val) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + u8 data = reg; > > + struct i2c_msg msg = { > > + .addr = client->addr, > > + .flags = 0, > > + .len = 1, > > + .buf = &data, > > + }; > > + int ret; > > + > > + ret = i2c_transfer(client->adapter, &msg, 1); > > + if (ret < 0) > > + goto err_wr; > > + > > + msg.flags = I2C_M_RD; > > + ret = i2c_transfer(client->adapter, &msg, 1); > > + if (ret < 0) > > + goto err_rd; > > Could we just have 2 messages in an array and just call i2c_transfer() once > for both write and read? > > Or actually it sounds like the i2c_smbus_read_byte_data() helper could work > here. > Understood. We would have a try, too. > > + > > + *val = data; > > + return 0; > > + > > +err_rd: > > + dev_err(&client->dev, "i2c_transfer --I2C_M_RD failed\n"); > > +err_wr: > > + dev_err(&client->dev, "read error: reg=0x%02x: %d\n", reg, ret); > > + return ret; > > +} > > + > > +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode, > > + struct v4l2_mbus_framefmt *fmt) > > +{ > > + fmt->width = mode->width; > > + fmt->height = mode->height; > > + fmt->field = V4L2_FIELD_NONE; > > +} > > + > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + if (ov02a10->streaming) { > > + mutex_unlock(&ov02a10->mutex); > > + return -EBUSY; > > + } > > + > > + /* Only one sensor mode supported */ > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + ov02a10->fmt = fmt->format; > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + fmt->format = ov02a10->fmt; > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + if (code->index >= ARRAY_SIZE(supported_modes) || !(code->index)) > > Hmm, ARRAY_SIZE(supported_modes) is 1 and we don't allow code->index to be > 0 either. Is there a code->index value that wouldn't return an error here? > Fixed in next release. > > + return -EINVAL; > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + if (fse->index >= ARRAY_SIZE(supported_modes) || !(fse->index)) > > Same here. > Fixed in next release. > > + return -EINVAL; > > + > > + fse->min_width = supported_modes[fse->index].width; > > + fse->max_width = supported_modes[fse->index].width; > > + fse->max_height = supported_modes[fse->index].height; > > + fse->min_height = supported_modes[fse->index].height; > > + > > + return 0; > > +} > > + > > +static int __ov02a10_power_on(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + struct device *dev = &client->dev; > > + int ret; > > + > > + ret = clk_prepare_enable(ov02a10->xvclk); > > + if (ret < 0) { > > + dev_err(dev, "Failed to enable xvclk\n"); > > + return ret; > > + } > > Is it really correct to enable the clock before the regulators? > > According to the datasheet, it should be: > - PD pin HIGH, > - nRST pin LOW, > - DVDDIO and AVDD28 power up and stabilize, > - clock enabled, > - min 5 ms delay, > - PD pin LOW, > - min 4 ms delay, > - nRST pin HIGH, > - min 5 ms delay, > - I2C interface ready. > xvclk as the clock source of sensor, we could enable it initially to let PLL ready. In fact, the power up sequence mainly focus on PD, RST, and IOVDD/AVDD28/DVDD pins. > > + > > + /* Note: set 0 is high, set 1 is low */ > > Why is that? If there is some inverter on the way that should be handled > outside of this driver. (GPIO DT bindings have flags for this purpose. > > If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should > call them like this in the driver too (+/- the lowercase and underscore > convention). > > According to the datasheet, the reset pin is called RST and inverted, so we should > call it n_rst, but the powerdown signal, called PD, is not inverted, so pd > would be the right name. > Sorry. Could make some more explanations about this? > > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 0); > > + > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > + if (ret < 0) { > > + dev_err(dev, "Failed to enable regulators\n"); > > + goto disable_clk; > > + } > > + msleep_range(7); > > + > > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > > + msleep_range(10); > > + > > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 0); > > + msleep_range(10); > > + > > + return 0; > > + > > +disable_clk: > > + clk_disable_unprepare(ov02a10->xvclk); > > + > > + return ret; > > +} > > + > > +static void __ov02a10_power_off(struct ov02a10 *ov02a10) > > +{ > > + clk_disable_unprepare(ov02a10->xvclk); > > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1); > > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1); > > + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies); > > This also doesn't seem to match my datasheet. The sequence there is: > - nRST goes LOW, > - clock stops, > - PD goes HIGH, > - regulators are powerd down. > Got it. This would be fixed in next release. > > +} > > + > > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + int ret; > > + > > + /* Apply default values of current mode */ > > + ret = ov02a10_write_array(ov02a10, ov02a10->cur_mode->reg_list); > > + if (ret) > > + return ret; > > + > > + /* Apply customized values from user */ > > + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler); > > + if (ret) > > + return ret; > > + > > + /* Set orientation to 180 degree */ > > + if (ov02a10->upside_down) { > > + ret = ov02a10_write_reg(ov02a10, REG_MIRROR_FLIP_CONTROL, > > + REG_CONFIG_MIRROR_FLIP); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to set orientation\n", > > + __func__); > > + return ret; > > + } > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + } > > + > > + /* Set stream on register */ > > + return ov02a10_write_reg(ov02a10, > > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STREAMING); > > +} > > + > > +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10) > > +{ > > + return ov02a10_write_reg(ov02a10, > > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STANDBY); > > +} > > + > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg) > > +{ > > + struct v4l2_subdev_format fmt = { 0 }; > > + > > + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; > > + fmt.format.width = 1600; > > + fmt.format.height = 1200; > > Where do these values come from? Should we have some macros for them? > Fixed in next release. > > + > > + ov02a10_set_fmt(subdev, cfg, &fmt); > > + > > + return 0; > > +} > > + > > +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + int ret = 0; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + if (ov02a10->streaming == on) > > + goto unlock_and_return; > > + > > + if (on) { > > + ret = pm_runtime_get_sync(&client->dev); > > + if (ret < 0) { > > + pm_runtime_put_noidle(&client->dev); > > + goto unlock_and_return; > > + } > > + > > + ret = __ov02a10_start_stream(ov02a10); > > + if (ret) { > > + __ov02a10_stop_stream(ov02a10); > > + ov02a10->streaming = !on; > > + goto err_rpm_put; > > + } > > + } else { > > + __ov02a10_stop_stream(ov02a10); > > + pm_runtime_put(&client->dev); > > + } > > + > > + ov02a10->streaming = on; > > + mutex_unlock(&ov02a10->mutex); > > + > > + return ret; > > + > > +err_rpm_put: > > + pm_runtime_put(&client->dev); > > +unlock_and_return: > > + mutex_unlock(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > +static int ov02a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_get_try_format(sd, > > + fh->pad, > > + 0); > > Please separate the initialization from the declaration, because there > isn't just enough space on this line anymore. > As Sakari pointed out, indeed this function is repeated executed since init_cfg is implemented already. Thus we would omit the open callback in next release. > > + > > + mutex_lock(&ov02a10->mutex); > > + /* Initialize try_fmt */ > > + try_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(&supported_modes[0], try_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused ov02a10_runtime_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + return __ov02a10_power_on(ov02a10); > > +} > > + > > +static int __maybe_unused ov02a10_runtime_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + __ov02a10_power_off(ov02a10); > > + > > + return 0; > > +} > > + > > +static const struct dev_pm_ops ov02a10_pm_ops = { > > + SET_RUNTIME_PM_OPS(ov02a10_runtime_suspend, > > + ov02a10_runtime_resume, NULL) > > Don't we need to implement and provide system PM ops too? > Fixed in next release. > > +}; > > + > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, s32 value) > > +{ > > + if (value) > > + return ov02a10_write_array(ov02a10, > > + ov02a10_test_pattern_enable_regs); > > + > > + return ov02a10_write_array(ov02a10, > > + ov02a10_test_pattern_disable_regs); > > +} > > + > > +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct ov02a10 *ov02a10 = container_of(ctrl->handler, > > + struct ov02a10, ctrl_handler); > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + s64 max_expo; > > + int ret; > > + > > + /* Propagate change of current control to all related controls */ > > + if (ctrl->id == V4L2_CID_VBLANK) { > > + /* Update max exposure while meeting expected vblanking */ > > + max_expo = ov02a10->cur_mode->height + ctrl->val - 4; > > + __v4l2_ctrl_modify_range(ov02a10->exposure, > > + ov02a10->exposure->minimum, max_expo, > > + ov02a10->exposure->step, > > + ov02a10->exposure->default_value); > > + } > > + > > + /* V4L2 controls values will be applied only when power is already up */ > > + if (!pm_runtime_get_if_in_use(&client->dev)) > > + return 0; > > + > > + switch (ctrl->id) { > > + case V4L2_CID_EXPOSURE: > > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_H, > > + ((ctrl->val >> 8) & 0xFF)); > > + if (!ret) { > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_L, > > + (ctrl->val & 0xFF)); > > + if (ret < 0) > > + return ret; > > + } > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + break; > > + case V4L2_CID_ANALOGUE_GAIN: > > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_GAIN, > > + (ctrl->val & 0xFF)); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + break; > > + case V4L2_CID_VBLANK: > > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_H, > > + (((ctrl->val + > > + ov02a10->cur_mode->height - > > + OV02A10_BASIC_LINE) >> 8) > > + & 0xFF)); > > + if (!ret) { > > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_L, > > + ((ctrl->val + > > + ov02a10->cur_mode->height - > > + OV02A10_BASIC_LINE) & 0xFF)); > > + if (ret < 0) > > + return ret; > > + } > > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + break; > > + case V4L2_CID_TEST_PATTERN: > > + ret = ov02a10_set_test_pattern(ov02a10, ctrl->val); > > + if (ret < 0) > > + return ret; > > + break; > > + default: > > + dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n", > > + __func__, ctrl->id, ctrl->val); > > + ret = -EINVAL; > > + break; > > We shouldn't need to handle this, as the control framework wouldn't call us > with a control that we didn't register explicitly. > I see other sensors like ov5645 and ov8856 also have this error ctrl id handle. Did you mean that "default:..." could be omitted? > > + }; > > + > > + pm_runtime_put(&client->dev); > > + > > + return ret; > > +} > > + > > +static const struct v4l2_subdev_video_ops ov02a10_video_ops = { > > + .s_stream = ov02a10_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = { > > + .init_cfg = ov02a10_entity_init_cfg, > > + .enum_mbus_code = ov02a10_enum_mbus_code, > > + .enum_frame_size = ov02a10_enum_frame_sizes, > > + .get_fmt = ov02a10_get_fmt, > > + .set_fmt = ov02a10_set_fmt, > > +}; > > + > > +static const struct v4l2_subdev_ops ov02a10_subdev_ops = { > > + .video = &ov02a10_video_ops, > > + .pad = &ov02a10_pad_ops, > > +}; > > + > > +static const struct media_entity_operations ov02a10_subdev_entity_ops = { > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > +static const struct v4l2_subdev_internal_ops ov02a10_internal_ops = { > > + .open = ov02a10_open, > > +}; > > + > > +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = { > > + .s_ctrl = ov02a10_set_ctrl, > > +}; > > + > > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + const struct ov02a10_mode *mode; > > + struct v4l2_ctrl_handler *handler; > > + struct v4l2_ctrl *ctrl; > > + u64 exposure_max; > > + u32 pixel_rate, h_blank; > > + int ret; > > + > > + handler = &ov02a10->ctrl_handler; > > + mode = ov02a10->cur_mode; > > + ret = v4l2_ctrl_handler_init(handler, 10); > > I can see 6 controls registered below. > I think handler num could be little more than the implemented num. But as you suggested, this would be fixed in next release. > > + if (ret) > > + return ret; > > + handler->lock = &ov02a10->mutex; > > + > > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, > > + 0, 0, link_freq_menu_items); > > + if (ctrl) > > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + pixel_rate = (link_freq_menu_items[0] * 2 * OV02A10_LANES) / > > + OV02A10_BITS_PER_SAMPLE; > > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, > > + 0, pixel_rate, 1, pixel_rate); > > + > > + h_blank = mode->hts_def - mode->width; > > + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK, > > + h_blank, h_blank, 1, h_blank); > > + if (ov02a10->hblank) > > + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_VBLANK, mode->vts_def - > > + mode->height, > > + OV02A10_VTS_MAX - mode->height, 1, > > + mode->vts_def - mode->height); > > + > > + exposure_max = mode->vts_def - 4; > > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_EXPOSURE, > > + OV02A10_EXPOSURE_MIN, > > + exposure_max, > > + OV02A10_EXPOSURE_STEP, > > + mode->exp_def); > > + > > + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, > > + OV02A10_GAIN_MIN, > > + OV02A10_GAIN_MAX, > > + OV02A10_GAIN_STEP, > > + OV02A10_GAIN_DEFAULT); > > + > > + ov02a10->test_pattern = > > + v4l2_ctrl_new_std_menu_items(handler, > > + &ov02a10_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov02a10_test_pattern_menu) - > > + 1, 0, 0, ov02a10_test_pattern_menu); > > + > > + if (handler->error) { > > + ret = handler->error; > > + dev_err(&client->dev, > > + "Failed to init controls(%d)\n", ret); > > + goto err_free_handler; > > + } > > + > > + ov02a10->subdev.ctrl_handler = handler; > > + > > + return 0; > > + > > +err_free_handler: > > + v4l2_ctrl_handler_free(handler); > > + > > + return ret; > > +} > > + > > +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + u16 id; > > + u8 pid = 0; > > + u8 ver = 0; > > + int ret; > > + > > + /* Check sensor revision */ > > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_H, &pid); > > + if (ret) > > + return ret; > > + > > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_L, &ver); > > + if (ret) > > + return ret; > > + > > + id = OV02A10_ID(pid, ver); > > + if (id != CHIP_ID) { > > + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id); > > + return ret; > > + } > > + > > + dev_info(&client->dev, "Detected OV%04X sensor\n", id); > > + > > + return 0; > > +} > > + > > +static int ov02a10_configure_regulators(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + unsigned int i; > > + > > + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++) > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > + > > + return devm_regulator_bulk_get(&client->dev, > > + OV02A10_NUM_SUPPLIES, > > + ov02a10->supplies); > > +} > > I think we can just have this directly inside probe. > Understood. We would have a try. > > + > > +static int ov02a10_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct ov02a10 *ov02a10; > > + u32 rotation; > > + u32 xclk_freq; > > + int ret; > > + > > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > > + if (!ov02a10) > > + return -ENOMEM; > > + > > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > > + > > + /* Optional indication of physical rotation of sensor */ > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", > > + &rotation); > > + if (!ret) { > > + switch (rotation) { > > + case 180: > > + ov02a10->upside_down = true; > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > + break; > > + case 0: > > + break; > > + default: > > + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", > > + rotation); > > + } > > + } > > + > > + /* Get system clock (xvclk) */ > > + ov02a10->xvclk = devm_clk_get(dev, "xvclk"); > > + if (IS_ERR(ov02a10->xvclk)) { > > + dev_err(dev, "Failed to get xvclk\n"); > > + return -EINVAL; > > + } > > Hmm, it's called eclk in my datasheet. > Does this really matter? Should we follow the common naming, refer to other sensors? > > + > > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > > + if (ret) { > > + dev_err(dev, "Failed to get xclk frequency\n"); > > + return ret; > > + } > > + > > + /* External clock must be 24MHz, allow 1% tolerance */ > > + if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > How do we support a range of frequencies? I don't see the driver calculate > any register values based on this frequency. Are you sure that the register > arrays don't assume one specific frequency? > In fact, clock-frequency defined in DT is always 24MHz, which is the clock frequency that sensor requires to keep itself work normally. > > + dev_err(dev, "external clock frequency %u is not supported\n", > > + xclk_freq); > > + return -EINVAL; > > + } > > + dev_dbg(dev, "external clock frequency %u\n", xclk_freq); > > + > > + ret = clk_set_rate(ov02a10->xvclk, xclk_freq); > > + if (ret) { > > + dev_err(dev, "Failed to set xvclk frequency (24MHz)\n"); > > + return ret; > > + } > > + > > + ov02a10->powerdown_gpio = devm_gpiod_get(dev, "powerdown", > > + GPIOD_OUT_LOW); > > Hmm, shouldn't this be HIGH? At least the datasheet has it so for the > powered down state. > The gpio state for this API callback seems to be inverse. > > + if (IS_ERR(ov02a10->powerdown_gpio)) { > > + dev_err(dev, "Failed to get powerdown-gpios\n"); > > + return -EINVAL; > > + } > > + > > + ov02a10->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > > Also LOW here, similarly to the above. > Ditto. > > + if (IS_ERR(ov02a10->reset_gpio)) { > > + dev_err(dev, "Failed to get reset-gpios\n"); > > + return -EINVAL; > > + } > > + > > + ret = ov02a10_configure_regulators(ov02a10); > > + if (ret) { > > + dev_err(dev, "Failed to get power regulators\n"); > > + return ret; > > + } > > + > > + mutex_init(&ov02a10->mutex); > > + ov02a10->cur_mode = &supported_modes[0]; > > + ret = ov02a10_initialize_controls(ov02a10); > > + if (ret) { > > + dev_err(dev, "Failed to initialize controls\n"); > > + goto err_destroy_mutex; > > + } > > + > > + ret = __ov02a10_power_on(ov02a10); > > + if (ret) > > + goto err_free_handler; > > + > > + ret = ov02a10_check_sensor_id(ov02a10); > > + if (ret) > > + goto err_power_off; > > + > > + ov02a10->subdev.internal_ops = &ov02a10_internal_ops; > > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > > + if (ret < 0) { > > + dev_err(dev, "failed to init entity pads: %d", ret); > > + goto err_power_off; > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", > > + ret); > > + goto err_clean_entity; > > + } > > + > > + pm_runtime_set_active(dev); > > + pm_runtime_enable(dev); > > + pm_runtime_idle(dev); > > + > > + dev_info(dev, "ov02a10 probe --\n"); > > Please remove this. > Fixed in next release. > > + return 0; > > + > > +err_clean_entity: > > + media_entity_cleanup(&ov02a10->subdev.entity); > > +err_power_off: > > + __ov02a10_power_off(ov02a10); > > +err_free_handler: > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > +err_destroy_mutex: > > + mutex_destroy(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > +static int ov02a10_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + media_entity_cleanup(&sd->entity); > > + v4l2_ctrl_handler_free(sd->ctrl_handler); > > + pm_runtime_disable(&client->dev); > > + if (!pm_runtime_status_suspended(&client->dev)) > > + __ov02a10_power_off(ov02a10); > > + pm_runtime_set_suspended(&client->dev); > > + mutex_destroy(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +#if IS_ENABLED(CONFIG_OF) > > +static const struct of_device_id ov02a10_of_match[] = { > > + { .compatible = "ovti,ov02a10" }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > > +#endif > > + > > +static struct i2c_driver ov02a10_i2c_driver = { > > + .driver = { > > + .name = "ov02a10", > > + .pm = &ov02a10_pm_ops, > > + .of_match_table = ov02a10_of_match, > > Please use of_match_ptr() wrapper. > Sorry. I am a little confused now. It seems that Sakari had one different ideas about this. https://patchwork.kernel.org/patch/10957225/ > Best regards, > Tomasz > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-09-07 3:58 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-19 3:43 [V3, 0/2] media: Add support for OV02A10 sensor dongchun.zhu 2019-08-19 3:43 ` [V3, 1/2] media: dt-bindings: media: i2c: Add bindings for OV02A10 dongchun.zhu 2019-08-27 16:48 ` Rob Herring 2019-08-19 3:43 ` [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver dongchun.zhu 2019-08-19 8:30 ` Sakari Ailus 2019-09-05 9:41 ` Dongchun Zhu 2019-09-05 10:45 ` Sakari Ailus 2019-09-05 10:53 ` Tomasz Figa 2019-09-05 16:05 ` Sakari Ailus 2019-09-05 22:58 ` Nicolas Boichat 2019-09-06 1:33 ` Dongchun Zhu 2019-09-07 3:51 ` Tomasz Figa 2019-08-21 10:30 ` Tomasz Figa 2019-08-21 11:05 ` Sakari Ailus 2019-08-26 6:54 ` Tomasz Figa 2019-09-05 15:54 ` Dongchun Zhu
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).