* [PATCH v3 0/2] media: i2c: Add OmniVision OV6211 image sensor driver @ 2025-08-12 21:30 Vladimir Zapolskiy 2025-08-12 21:30 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add OmniVision OV6211 image sensor Vladimir Zapolskiy 2025-08-12 21:30 ` [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy 0 siblings, 2 replies; 8+ messages in thread From: Vladimir Zapolskiy @ 2025-08-12 21:30 UTC (permalink / raw) To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil, Tarang Raval Cc: linux-media, devicetree OmniVision OV6211 is a monochrome image sensor, which produces frames in 8/10-bit raw output format and supports 400x400, 200x200 and 100x100 output image resolution modes. At the moment the only supported resolution in the device driver is 400x400@120fps (Y8). The driver version is based on top of the series, which adds a new devm_v4l2_sensor_clk_get() helper [1]. Link to v2 of the OV6211 camera sensor device driver: * https://lore.kernel.org/all/20250729231454.242748-1-vladimir.zapolskiy@linaro.org/ Changes from v2 to v3: * added Reviewed-by: tag to the device tree binding (Rob), * simplifed access to the associated struct device (Tarang), * ov6211_get_format() replaced by v4l2_subdev_state_get_format() (Tarang), * removed private .cur_mode, since just one mode is supported (Tarang), * removed a custom serialization mutex (Tarang), * set power on before registering the sensor in V4L2 (Tarang), * v4l2_subdev_internal_ops: replaced .open with .init_state, * updated signature of ov6211_update_pad_format() to generalize it and to simplify adding more modes in future, * replaced a custom .s_stream from v4l2_subdev_video_ops by .enable_streams and .disable_streams callbacks in v4l2_subdev_pad_ops, * add support of PM runtime autosuspend into the driver, * taking into account a similarity of OV6211 PLL1 configuration to the known OV7251 PLL1 correct the set MIPI link frequency value. Link to v1 of the OV6211 camera sensor device driver: * https://lore.kernel.org/linux-media/20250717124001.108486-1-vladimir.zapolskiy@linaro.org/ Changes from v1 to v2: 1. restrict endpoint unevaluated properties (Krzysztof), 2. changed dev_err() to dev_err_probe() whenever it's applicable (Krzysztof), 3. removed in-driver I2C operations in favour of V4L2 CCI interface (Kieran), 4. added hblank, vblank, pixel rate and rotation/orientation V4L2 ro controls (Kieran, Dave), 5. due to unselectable data lanes property removed data_lanes handling (Dave), 6. replaced devm_clk_get_optional() with devm_v4l2_sensor_clk_get() (Dave, Mehdi), 7. minor cosmetic updates (reported error messages, goto label names etc.). [1] https://lore.kernel.org/linux-media/8ecbcafbd91b25ad5e188dbe127b921a1643027e.1750942967.git.mehdi.djait@linux.intel.com/ Vladimir Zapolskiy (2): dt-bindings: media: i2c: Add OmniVision OV6211 image sensor media: i2c: Add OmniVision OV6211 image sensor driver .../bindings/media/i2c/ovti,ov6211.yaml | 96 +++ MAINTAINERS | 8 + drivers/media/i2c/Kconfig | 10 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov6211.c | 809 ++++++++++++++++++ 5 files changed, 924 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov6211.yaml create mode 100644 drivers/media/i2c/ov6211.c -- 2.49.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] dt-bindings: media: i2c: Add OmniVision OV6211 image sensor 2025-08-12 21:30 [PATCH v3 0/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy @ 2025-08-12 21:30 ` Vladimir Zapolskiy 2025-08-12 21:30 ` [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy 1 sibling, 0 replies; 8+ messages in thread From: Vladimir Zapolskiy @ 2025-08-12 21:30 UTC (permalink / raw) To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil, Tarang Raval Cc: linux-media, devicetree Add device tree bindings documentation for OmniVision OV6211 image sensor. Reviewed-by: Rob Herring (Arm) <robh@kernel.org> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> --- Some minor changes from v1 to v2: * removed dvdd-supply voltage level from its description, it is not fixed, * changed a MIPI link frequency in the example to 480MHz. .../bindings/media/i2c/ovti,ov6211.yaml | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov6211.yaml diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov6211.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov6211.yaml new file mode 100644 index 000000000000..5a857fa2f371 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov6211.yaml @@ -0,0 +1,96 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ovti,ov6211.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: OmniVision OV6211 Image Sensor + +description: + OmniVision OV6211 image sensor is a high performance monochrome image + sensor. The sensor is controlled over a serial camera control bus + protocol (SCCB), the widest supported output image frame size is 400x400 + at 120 frames per second rate, data output format is 8/10-bit RAW + transferred over one-lane MIPI D-PHY interface. + +maintainers: + - Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> + +allOf: + - $ref: /schemas/media/video-interface-devices.yaml# + +properties: + compatible: + const: ovti,ov6211 + + reg: + maxItems: 1 + + clocks: + description: XVCLK supply clock, 6MHz to 27MHz frequency. + maxItems: 1 + + reset-gpios: + description: Active low GPIO connected to XSHUTDOWN pad of the sensor. + maxItems: 1 + + strobe-gpios: + description: Input GPIO connected to strobe pad of the sensor. + maxItems: 1 + + avdd-supply: + description: Analogue voltage supply, 2.6 to 3.0 volts. + + dovdd-supply: + description: Digital I/O voltage supply, 1.8 volts. + + dvdd-supply: + description: Digital core voltage supply. + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + required: + - link-frequencies + +required: + - compatible + - reg + - port + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + camera@60 { + compatible = "ovti,ov6211"; + reg = <0x60>; + clocks = <&camera_clk 0>; + assigned-clocks = <&camera_clk 0>; + assigned-clock-rates = <24000000>; + reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>; + avdd-supply = <&vreg_2p8>; + dovdd-supply = <&vreg_1p8>; + dvdd-supply = <&vreg_1p2>; + + port { + endpoint { + link-frequencies = /bits/ 64 <480000000>; + remote-endpoint = <&mipi_csi2_ep>; + }; + }; + }; + }; +... -- 2.49.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver 2025-08-12 21:30 [PATCH v3 0/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy 2025-08-12 21:30 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add OmniVision OV6211 image sensor Vladimir Zapolskiy @ 2025-08-12 21:30 ` Vladimir Zapolskiy 2025-08-13 6:25 ` Tarang Raval 2025-08-18 12:08 ` Sakari Ailus 1 sibling, 2 replies; 8+ messages in thread From: Vladimir Zapolskiy @ 2025-08-12 21:30 UTC (permalink / raw) To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sakari Ailus, Hans Verkuil, Tarang Raval Cc: linux-media, devicetree OmniVision OV6211 is a monochrome image sensor, which produces frames in 8/10-bit raw output format and supports 400x400, 200x200 and 100x100 output image resolution modes. Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> --- MAINTAINERS | 8 + drivers/media/i2c/Kconfig | 10 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov6211.c | 809 +++++++++++++++++++++++++++++++++++++ 4 files changed, 828 insertions(+) create mode 100644 drivers/media/i2c/ov6211.c diff --git a/MAINTAINERS b/MAINTAINERS index fe168477caa4..f2af41bd19b0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18722,6 +18722,14 @@ S: Maintained T: git git://linuxtv.org/media.git F: drivers/media/i2c/ov5695.c +OMNIVISION OV6211 SENSOR DRIVER +M: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> +L: linux-media@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/ovti,ov6211.yaml +F: drivers/media/i2c/ov6211.c + OMNIVISION OV64A40 SENSOR DRIVER M: Jacopo Mondi <jacopo.mondi@ideasonboard.com> L: linux-media@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 6237fe804a5c..13ff3c153117 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -542,6 +542,16 @@ config VIDEO_OV5695 To compile this driver as a module, choose M here: the module will be called ov5695. +config VIDEO_OV6211 + tristate "OmniVision OV6211 sensor support" + select V4L2_CCI_I2C + help + This is a Video4Linux2 sensor driver for the OmniVision + OV6211 camera. + + To compile this driver as a module, choose M here: the + module will be called ov6211. + config VIDEO_OV64A40 tristate "OmniVision OV64A40 sensor support" select V4L2_CCI_I2C diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 5873d29433ee..03814fc8e2f7 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -103,6 +103,7 @@ obj-$(CONFIG_VIDEO_OV5670) += ov5670.o obj-$(CONFIG_VIDEO_OV5675) += ov5675.o obj-$(CONFIG_VIDEO_OV5693) += ov5693.o obj-$(CONFIG_VIDEO_OV5695) += ov5695.o +obj-$(CONFIG_VIDEO_OV6211) += ov6211.o obj-$(CONFIG_VIDEO_OV64A40) += ov64a40.o obj-$(CONFIG_VIDEO_OV6650) += ov6650.o obj-$(CONFIG_VIDEO_OV7251) += ov7251.o diff --git a/drivers/media/i2c/ov6211.c b/drivers/media/i2c/ov6211.c new file mode 100644 index 000000000000..d48b13399b2e --- /dev/null +++ b/drivers/media/i2c/ov6211.c @@ -0,0 +1,809 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2024-2025 Linaro Ltd + +#include <linux/clk.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/v4l2-cci.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> +#include <media/v4l2-fwnode.h> + +#define OV6211_LINK_FREQ_480MHZ 480000000ULL +#define OV6211_MCLK_FREQ_24MHZ 24000000 + +#define OV6211_REG_CHIP_ID CCI_REG16(0x300a) +#define OV6211_CHIP_ID 0x6710 + +#define OV6211_REG_MODE_SELECT CCI_REG8(0x0100) +#define OV6211_MODE_STANDBY 0x00 +#define OV6211_MODE_STREAMING 0x01 + +#define OV6211_REG_SOFTWARE_RST CCI_REG8(0x0103) +#define OV6211_SOFTWARE_RST 0x01 + +/* Exposure controls from sensor */ +#define OV6211_REG_EXPOSURE CCI_REG24(0x3500) +#define OV6211_EXPOSURE_MIN 1 +#define OV6211_EXPOSURE_MAX_MARGIN 4 +#define OV6211_EXPOSURE_STEP 1 +#define OV6211_EXPOSURE_DEFAULT 210 + +/* Analogue gain controls from sensor */ +#define OV6211_REG_ANALOGUE_GAIN CCI_REG16(0x350a) +#define OV6211_ANALOGUE_GAIN_MIN 1 +#define OV6211_ANALOGUE_GAIN_MAX 0x3ff +#define OV6211_ANALOGUE_GAIN_STEP 1 +#define OV6211_ANALOGUE_GAIN_DEFAULT 0xa0 + +#define to_ov6211(_sd) container_of(_sd, struct ov6211, sd) + +struct ov6211_reg_list { + const struct cci_reg_sequence *regs; + unsigned int num_regs; +}; + +struct ov6211_mode { + u32 width; /* Frame width in pixels */ + u32 height; /* Frame height in pixels */ + u32 hts; /* Horizontal timing size */ + u32 vts; /* Default vertical timing size */ + u32 bpp; /* Bits per pixel */ + + const struct ov6211_reg_list reg_list; /* Sensor register setting */ +}; + +static const s64 link_freq_menu_items[] = { + OV6211_LINK_FREQ_480MHZ, +}; + +static const struct cci_reg_sequence ov6211_400x400_120fps_mode[] = { + { CCI_REG8(0x3005), 0x00 }, + { CCI_REG8(0x3013), 0x12 }, + { CCI_REG8(0x3014), 0x04 }, + { CCI_REG8(0x3016), 0x10 }, + { CCI_REG8(0x3017), 0x00 }, + { CCI_REG8(0x3018), 0x00 }, + { CCI_REG8(0x301a), 0x00 }, + { CCI_REG8(0x301b), 0x00 }, + { CCI_REG8(0x301c), 0x00 }, + { CCI_REG8(0x3037), 0xf0 }, + { CCI_REG8(0x3080), 0x01 }, + { CCI_REG8(0x3081), 0x00 }, + { CCI_REG8(0x3082), 0x01 }, + { CCI_REG8(0x3098), 0x04 }, + { CCI_REG8(0x3099), 0x28 }, + { CCI_REG8(0x309a), 0x06 }, + { CCI_REG8(0x309b), 0x04 }, + { CCI_REG8(0x309c), 0x00 }, + { CCI_REG8(0x309d), 0x00 }, + { CCI_REG8(0x309e), 0x01 }, + { CCI_REG8(0x309f), 0x00 }, + { CCI_REG8(0x30b0), 0x08 }, + { CCI_REG8(0x30b1), 0x02 }, + { CCI_REG8(0x30b2), 0x00 }, + { CCI_REG8(0x30b3), 0x28 }, + { CCI_REG8(0x30b4), 0x02 }, + { CCI_REG8(0x30b5), 0x00 }, + { CCI_REG8(0x3106), 0xd9 }, + { CCI_REG8(0x3503), 0x07 }, + { CCI_REG8(0x3509), 0x10 }, + { CCI_REG8(0x3600), 0xfc }, + { CCI_REG8(0x3620), 0xb7 }, + { CCI_REG8(0x3621), 0x05 }, + { CCI_REG8(0x3626), 0x31 }, + { CCI_REG8(0x3627), 0x40 }, + { CCI_REG8(0x3632), 0xa3 }, + { CCI_REG8(0x3633), 0x34 }, + { CCI_REG8(0x3634), 0x40 }, + { CCI_REG8(0x3636), 0x00 }, + { CCI_REG8(0x3660), 0x80 }, + { CCI_REG8(0x3662), 0x03 }, + { CCI_REG8(0x3664), 0xf0 }, + { CCI_REG8(0x366a), 0x10 }, + { CCI_REG8(0x366b), 0x06 }, + { CCI_REG8(0x3680), 0xf4 }, + { CCI_REG8(0x3681), 0x50 }, + { CCI_REG8(0x3682), 0x00 }, + { CCI_REG8(0x3708), 0x20 }, + { CCI_REG8(0x3709), 0x40 }, + { CCI_REG8(0x370d), 0x03 }, + { CCI_REG8(0x373b), 0x02 }, + { CCI_REG8(0x373c), 0x08 }, + { CCI_REG8(0x3742), 0x00 }, + { CCI_REG8(0x3744), 0x16 }, + { CCI_REG8(0x3745), 0x08 }, + { CCI_REG8(0x3781), 0xfc }, + { CCI_REG8(0x3788), 0x00 }, + { CCI_REG8(0x3800), 0x00 }, + { CCI_REG8(0x3801), 0x04 }, + { CCI_REG8(0x3802), 0x00 }, + { CCI_REG8(0x3803), 0x04 }, + { CCI_REG8(0x3804), 0x01 }, + { CCI_REG8(0x3805), 0x9b }, + { CCI_REG8(0x3806), 0x01 }, + { CCI_REG8(0x3807), 0x9b }, + { CCI_REG8(0x3808), 0x01 }, + { CCI_REG8(0x3809), 0x90 }, + { CCI_REG8(0x380a), 0x01 }, + { CCI_REG8(0x380b), 0x90 }, + { CCI_REG8(0x380c), 0x05 }, + { CCI_REG8(0x380d), 0xf2 }, + { CCI_REG8(0x380e), 0x01 }, + { CCI_REG8(0x380f), 0xb6 }, + { CCI_REG8(0x3810), 0x00 }, + { CCI_REG8(0x3811), 0x04 }, + { CCI_REG8(0x3812), 0x00 }, + { CCI_REG8(0x3813), 0x04 }, + { CCI_REG8(0x3814), 0x11 }, + { CCI_REG8(0x3815), 0x11 }, + { CCI_REG8(0x3820), 0x00 }, + { CCI_REG8(0x3821), 0x00 }, + { CCI_REG8(0x382b), 0xfa }, + { CCI_REG8(0x382f), 0x04 }, + { CCI_REG8(0x3832), 0x00 }, + { CCI_REG8(0x3833), 0x05 }, + { CCI_REG8(0x3834), 0x00 }, + { CCI_REG8(0x3835), 0x05 }, + { CCI_REG8(0x3882), 0x04 }, + { CCI_REG8(0x3883), 0x00 }, + { CCI_REG8(0x38a4), 0x10 }, + { CCI_REG8(0x38a5), 0x00 }, + { CCI_REG8(0x38b1), 0x03 }, + { CCI_REG8(0x3b80), 0x00 }, + { CCI_REG8(0x3b81), 0xff }, + { CCI_REG8(0x3b82), 0x10 }, + { CCI_REG8(0x3b83), 0x00 }, + { CCI_REG8(0x3b84), 0x08 }, + { CCI_REG8(0x3b85), 0x00 }, + { CCI_REG8(0x3b86), 0x01 }, + { CCI_REG8(0x3b87), 0x00 }, + { CCI_REG8(0x3b88), 0x00 }, + { CCI_REG8(0x3b89), 0x00 }, + { CCI_REG8(0x3b8a), 0x00 }, + { CCI_REG8(0x3b8b), 0x05 }, + { CCI_REG8(0x3b8c), 0x00 }, + { CCI_REG8(0x3b8d), 0x00 }, + { CCI_REG8(0x3b8e), 0x01 }, + { CCI_REG8(0x3b8f), 0xb2 }, + { CCI_REG8(0x3b94), 0x05 }, + { CCI_REG8(0x3b95), 0xf2 }, + { CCI_REG8(0x3b96), 0xc0 }, + { CCI_REG8(0x4004), 0x04 }, + { CCI_REG8(0x404e), 0x01 }, + { CCI_REG8(0x4801), 0x0f }, + { CCI_REG8(0x4806), 0x0f }, + { CCI_REG8(0x4837), 0x43 }, + { CCI_REG8(0x5a08), 0x00 }, + { CCI_REG8(0x5a01), 0x00 }, + { CCI_REG8(0x5a03), 0x00 }, + { CCI_REG8(0x5a04), 0x10 }, + { CCI_REG8(0x5a05), 0xa0 }, + { CCI_REG8(0x5a06), 0x0c }, + { CCI_REG8(0x5a07), 0x78 }, +}; + +static const struct ov6211_mode supported_modes[] = { + { + .width = 400, + .height = 400, + .hts = 1522, + .vts = 438, + .bpp = 8, + .reg_list = { + .regs = ov6211_400x400_120fps_mode, + .num_regs = ARRAY_SIZE(ov6211_400x400_120fps_mode), + }, + }, +}; + +struct ov6211 { + struct device *dev; + struct regmap *regmap; + struct clk *xvclk; + struct gpio_desc *reset_gpio; + struct regulator *avdd; + struct regulator *dovdd; + struct regulator *dvdd; + + struct v4l2_subdev sd; + struct media_pad pad; + + struct v4l2_ctrl_handler ctrl_handler; +}; + +static int ov6211_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct ov6211 *ov6211 = container_of(ctrl->handler, struct ov6211, + ctrl_handler); + int ret; + + /* V4L2 controls values will be applied only when power is already up */ + if (!pm_runtime_get_if_in_use(ov6211->dev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_ANALOGUE_GAIN: + ret = cci_write(ov6211->regmap, OV6211_REG_ANALOGUE_GAIN, + ctrl->val, NULL); + break; + case V4L2_CID_EXPOSURE: + ret = cci_write(ov6211->regmap, OV6211_REG_EXPOSURE, + ctrl->val << 4, NULL); + break; + default: + ret = -EINVAL; + break; + } + + pm_runtime_put(ov6211->dev); + + return ret; +} + +static const struct v4l2_ctrl_ops ov6211_ctrl_ops = { + .s_ctrl = ov6211_set_ctrl, +}; + +static int ov6211_init_controls(struct ov6211 *ov6211) +{ + struct v4l2_ctrl_handler *ctrl_hdlr = &ov6211->ctrl_handler; + const struct ov6211_mode *mode = &supported_modes[0]; + struct v4l2_fwnode_device_properties props; + s64 exposure_max, pixel_rate, h_blank; + struct v4l2_ctrl *ctrl; + int ret; + + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); + if (ret) + return ret; + + ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov6211_ctrl_ops, + V4L2_CID_LINK_FREQ, + ARRAY_SIZE(link_freq_menu_items) - 1, + 0, link_freq_menu_items); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + pixel_rate = link_freq_menu_items[0] / mode->bpp; + v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_PIXEL_RATE, + 0, pixel_rate, 1, pixel_rate); + + h_blank = mode->hts - mode->width; + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_HBLANK, + h_blank, h_blank, 1, h_blank); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_VBLANK, + mode->vts - mode->height, + mode->vts - mode->height, 1, + mode->vts - mode->height); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, + OV6211_ANALOGUE_GAIN_MIN, OV6211_ANALOGUE_GAIN_MAX, + OV6211_ANALOGUE_GAIN_STEP, + OV6211_ANALOGUE_GAIN_DEFAULT); + + exposure_max = (mode->vts - OV6211_EXPOSURE_MAX_MARGIN); + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, + V4L2_CID_EXPOSURE, + OV6211_EXPOSURE_MIN, exposure_max, + OV6211_EXPOSURE_STEP, + OV6211_EXPOSURE_DEFAULT); + + if (ctrl_hdlr->error) + return ctrl_hdlr->error; + + ret = v4l2_fwnode_device_parse(ov6211->dev, &props); + if (ret) + goto error_free_hdlr; + + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov6211_ctrl_ops, + &props); + if (ret) + goto error_free_hdlr; + + ov6211->sd.ctrl_handler = ctrl_hdlr; + + return 0; + +error_free_hdlr: + v4l2_ctrl_handler_free(ctrl_hdlr); + + return ret; +} + +static void ov6211_update_pad_format(const struct ov6211_mode *mode, + struct v4l2_mbus_framefmt *fmt) +{ + fmt->code = MEDIA_BUS_FMT_Y8_1X8; + fmt->width = mode->width; + fmt->height = mode->height; + fmt->field = V4L2_FIELD_NONE; + fmt->colorspace = V4L2_COLORSPACE_RAW; + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->xfer_func = V4L2_XFER_FUNC_NONE; +} + +static int ov6211_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + const struct ov6211_reg_list *reg_list = &supported_modes[0].reg_list; + struct ov6211 *ov6211 = to_ov6211(sd); + int ret; + + ret = pm_runtime_resume_and_get(ov6211->dev); + if (ret) + return ret; + + /* Skip a step of explicit entering into the standby mode */ + ret = cci_write(ov6211->regmap, OV6211_REG_SOFTWARE_RST, + OV6211_SOFTWARE_RST, NULL); + if (ret) { + dev_err(ov6211->dev, "failed to software reset: %d\n", ret); + goto error; + } + + ret = cci_multi_reg_write(ov6211->regmap, reg_list->regs, + reg_list->num_regs, NULL); + if (ret) { + dev_err(ov6211->dev, "failed to set mode: %d\n", ret); + goto error; + } + + ret = __v4l2_ctrl_handler_setup(ov6211->sd.ctrl_handler); + if (ret) + goto error; + + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, + OV6211_MODE_STREAMING, NULL); + if (ret) { + dev_err(ov6211->dev, "failed to start streaming: %d\n", ret); + goto error; + } + + return 0; + +error: + pm_runtime_mark_last_busy(ov6211->dev); + pm_runtime_put_autosuspend(ov6211->dev); + + return ret; +} + +static int ov6211_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct ov6211 *ov6211 = to_ov6211(sd); + int ret; + + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, + OV6211_MODE_STANDBY, NULL); + if (ret) + dev_err(ov6211->dev, "failed to stop streaming: %d\n", ret); + + pm_runtime_mark_last_busy(ov6211->dev); + pm_runtime_put_autosuspend(ov6211->dev); + + return ret; +} + +static int ov6211_set_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, + struct v4l2_subdev_format *fmt) +{ + struct v4l2_mbus_framefmt *format; + const struct ov6211_mode *mode; + + format = v4l2_subdev_state_get_format(state, 0); + + mode = v4l2_find_nearest_size(supported_modes, + ARRAY_SIZE(supported_modes), + width, height, + fmt->format.width, + fmt->format.height); + + ov6211_update_pad_format(mode, &fmt->format); + *format = fmt->format; + + return 0; +} + +static int ov6211_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + if (code->index > 0) + return -EINVAL; + + code->code = MEDIA_BUS_FMT_Y8_1X8; + + return 0; +} + +static int ov6211_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + if (fse->index >= ARRAY_SIZE(supported_modes)) + return -EINVAL; + + if (fse->code != MEDIA_BUS_FMT_Y8_1X8) + return -EINVAL; + + fse->min_width = supported_modes[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = supported_modes[fse->index].height; + fse->max_height = fse->min_height; + + return 0; +} + +static int ov6211_init_state(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state) +{ + struct v4l2_subdev_format fmt = { + .which = V4L2_SUBDEV_FORMAT_TRY, + .pad = 0, + .format = { + .code = MEDIA_BUS_FMT_Y8_1X8, + .width = supported_modes[0].width, + .height = supported_modes[0].height, + }, + }; + + ov6211_set_pad_format(sd, state, &fmt); + + return 0; +} + +static const struct v4l2_subdev_video_ops ov6211_video_ops = { + .s_stream = v4l2_subdev_s_stream_helper, +}; + +static const struct v4l2_subdev_pad_ops ov6211_pad_ops = { + .set_fmt = ov6211_set_pad_format, + .get_fmt = v4l2_subdev_get_fmt, + .enum_mbus_code = ov6211_enum_mbus_code, + .enum_frame_size = ov6211_enum_frame_size, + .enable_streams = ov6211_enable_streams, + .disable_streams = ov6211_disable_streams, +}; + +static const struct v4l2_subdev_ops ov6211_subdev_ops = { + .video = &ov6211_video_ops, + .pad = &ov6211_pad_ops, +}; + +static const struct media_entity_operations ov6211_subdev_entity_ops = { + .link_validate = v4l2_subdev_link_validate, +}; + +static const struct v4l2_subdev_internal_ops ov6211_internal_ops = { + .init_state = ov6211_init_state, +}; + +static int ov6211_identify_module(struct ov6211 *ov6211) +{ + u64 val; + int ret; + + ret = cci_read(ov6211->regmap, OV6211_REG_CHIP_ID, &val, NULL); + if (ret) { + dev_err(ov6211->dev, "failed to read chip id: %d\n", ret); + return ret; + } + + if (val != OV6211_CHIP_ID) { + dev_err(ov6211->dev, "chip id mismatch: %x!=%llx\n", + OV6211_CHIP_ID, val); + return -ENODEV; + } + + return 0; +} + +static int ov6211_check_hwcfg(struct ov6211 *ov6211) +{ + struct fwnode_handle *fwnode = dev_fwnode(ov6211->dev), *ep; + struct v4l2_fwnode_endpoint bus_cfg = { + .bus_type = V4L2_MBUS_CSI2_DPHY, + }; + int ret; + + if (!fwnode) + return -ENODEV; + + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); + if (!ep) + return -EINVAL; + + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); + fwnode_handle_put(ep); + if (ret) + return ret; + + if (!bus_cfg.nr_of_link_frequencies) { + dev_err(ov6211->dev, "link-frequency property is not found\n"); + ret = -EINVAL; + goto error; + } + + if (bus_cfg.nr_of_link_frequencies != 1 || + bus_cfg.link_frequencies[0] != link_freq_menu_items[0]) { + dev_err(ov6211->dev, "Unsupported MIPI CSI2 link frequency\n"); + ret = -EINVAL; + goto error; + } + +error: + v4l2_fwnode_endpoint_free(&bus_cfg); + + return ret; +} + +static int ov6211_power_on(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct ov6211 *ov6211 = to_ov6211(sd); + int ret; + + if (ov6211->avdd) { + ret = regulator_enable(ov6211->avdd); + if (ret) + return ret; + } + + if (ov6211->dovdd) { + ret = regulator_enable(ov6211->dovdd); + if (ret) + goto avdd_disable; + } + + if (ov6211->dvdd) { + ret = regulator_enable(ov6211->dvdd); + if (ret) + goto dovdd_disable; + } + + gpiod_set_value_cansleep(ov6211->reset_gpio, 0); + usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC); + + ret = clk_prepare_enable(ov6211->xvclk); + if (ret) + goto reset_gpio; + + return 0; + +reset_gpio: + gpiod_set_value_cansleep(ov6211->reset_gpio, 1); + + if (ov6211->dvdd) + regulator_disable(ov6211->dvdd); +dovdd_disable: + if (ov6211->dovdd) + regulator_disable(ov6211->dovdd); +avdd_disable: + if (ov6211->avdd) + regulator_disable(ov6211->avdd); + + return ret; +} + +static int ov6211_power_off(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct ov6211 *ov6211 = to_ov6211(sd); + + clk_disable_unprepare(ov6211->xvclk); + + gpiod_set_value_cansleep(ov6211->reset_gpio, 1); + + if (ov6211->dvdd) + regulator_disable(ov6211->dvdd); + + if (ov6211->dovdd) + regulator_disable(ov6211->dovdd); + + if (ov6211->avdd) + regulator_disable(ov6211->avdd); + + return 0; +} + +static int ov6211_probe(struct i2c_client *client) +{ + struct ov6211 *ov6211; + unsigned long freq; + int ret; + + ov6211 = devm_kzalloc(&client->dev, sizeof(*ov6211), GFP_KERNEL); + if (!ov6211) + return -ENOMEM; + + ov6211->dev = &client->dev; + + v4l2_i2c_subdev_init(&ov6211->sd, client, &ov6211_subdev_ops); + ov6211->sd.dev = ov6211->dev; + + ov6211->regmap = devm_cci_regmap_init_i2c(client, 16); + if (IS_ERR(ov6211->regmap)) + return dev_err_probe(ov6211->dev, PTR_ERR(ov6211->regmap), + "failed to init CCI\n"); + + ov6211->xvclk = devm_v4l2_sensor_clk_get(ov6211->dev, NULL); + if (IS_ERR(ov6211->xvclk)) + return dev_err_probe(ov6211->dev, PTR_ERR(ov6211->xvclk), + "failed to get XVCLK clock\n"); + + freq = clk_get_rate(ov6211->xvclk); + if (freq && freq != OV6211_MCLK_FREQ_24MHZ) + return dev_err_probe(ov6211->dev, -EINVAL, + "XVCLK clock frequency %lu is not supported\n", + freq); + + ret = ov6211_check_hwcfg(ov6211); + if (ret) + return dev_err_probe(ov6211->dev, ret, + "failed to check HW configuration\n"); + + ov6211->reset_gpio = devm_gpiod_get_optional(ov6211->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(ov6211->reset_gpio)) + return dev_err_probe(ov6211->dev, PTR_ERR(ov6211->reset_gpio), + "cannot get reset GPIO\n"); + + ov6211->avdd = devm_regulator_get_optional(ov6211->dev, "avdd"); + if (IS_ERR(ov6211->avdd)) { + ret = PTR_ERR(ov6211->avdd); + if (ret != -ENODEV) + return dev_err_probe(ov6211->dev, ret, + "Failed to get avdd regulator\n"); + + ov6211->avdd = NULL; + } + + ov6211->dovdd = devm_regulator_get_optional(ov6211->dev, "dovdd"); + if (IS_ERR(ov6211->dovdd)) { + ret = PTR_ERR(ov6211->dovdd); + if (ret != -ENODEV) + return dev_err_probe(ov6211->dev, ret, + "Failed to get dovdd regulator\n"); + + ov6211->dovdd = NULL; + } + + ov6211->dvdd = devm_regulator_get_optional(ov6211->dev, "dvdd"); + if (IS_ERR(ov6211->dvdd)) { + ret = PTR_ERR(ov6211->dvdd); + if (ret != -ENODEV) + return dev_err_probe(ov6211->dev, ret, + "Failed to get dvdd regulator\n"); + + ov6211->dvdd = NULL; + } + + /* The sensor must be powered on to read the CHIP_ID register */ + ret = ov6211_power_on(ov6211->dev); + if (ret) + return ret; + + ret = ov6211_identify_module(ov6211); + if (ret) { + dev_err_probe(ov6211->dev, ret, "failed to find sensor\n"); + goto power_off; + } + + ret = ov6211_init_controls(ov6211); + if (ret) { + dev_err_probe(ov6211->dev, ret, "failed to init controls\n"); + goto power_off; + } + + ov6211->sd.state_lock = ov6211->ctrl_handler.lock; + ov6211->sd.internal_ops = &ov6211_internal_ops; + ov6211->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + ov6211->sd.entity.ops = &ov6211_subdev_entity_ops; + ov6211->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; + ov6211->pad.flags = MEDIA_PAD_FL_SOURCE; + + ret = media_entity_pads_init(&ov6211->sd.entity, 1, &ov6211->pad); + if (ret) { + dev_err_probe(ov6211->dev, ret, + "failed to init media entity pads\n"); + goto v4l2_ctrl_handler_free; + } + + ret = v4l2_subdev_init_finalize(&ov6211->sd); + if (ret < 0) { + dev_err_probe(ov6211->dev, ret, + "failed to init media entity pads\n"); + goto media_entity_cleanup; + } + + pm_runtime_set_active(ov6211->dev); + pm_runtime_enable(ov6211->dev); + + ret = v4l2_async_register_subdev_sensor(&ov6211->sd); + if (ret < 0) { + dev_err_probe(ov6211->dev, ret, + "failed to register V4L2 subdev\n"); + goto subdev_cleanup; + } + + /* Enable runtime PM and turn off the device */ + pm_runtime_idle(ov6211->dev); + pm_runtime_set_autosuspend_delay(ov6211->dev, 1000); + pm_runtime_use_autosuspend(ov6211->dev); + + return 0; + +subdev_cleanup: + v4l2_subdev_cleanup(&ov6211->sd); + pm_runtime_disable(ov6211->dev); + pm_runtime_set_suspended(ov6211->dev); + +media_entity_cleanup: + media_entity_cleanup(&ov6211->sd.entity); + +v4l2_ctrl_handler_free: + v4l2_ctrl_handler_free(ov6211->sd.ctrl_handler); + +power_off: + ov6211_power_off(ov6211->dev); + + return ret; +} + +static void ov6211_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ov6211 *ov6211 = to_ov6211(sd); + + v4l2_async_unregister_subdev(sd); + v4l2_subdev_cleanup(sd); + media_entity_cleanup(&sd->entity); + v4l2_ctrl_handler_free(sd->ctrl_handler); + pm_runtime_disable(ov6211->dev); + + if (!pm_runtime_status_suspended(ov6211->dev)) { + ov6211_power_off(ov6211->dev); + pm_runtime_set_suspended(ov6211->dev); + } +} + +static const struct dev_pm_ops ov6211_pm_ops = { + SET_RUNTIME_PM_OPS(ov6211_power_off, ov6211_power_on, NULL) +}; + +static const struct of_device_id ov6211_of_match[] = { + { .compatible = "ovti,ov6211" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ov6211_of_match); + +static struct i2c_driver ov6211_i2c_driver = { + .driver = { + .name = "ov6211", + .pm = &ov6211_pm_ops, + .of_match_table = ov6211_of_match, + }, + .probe = ov6211_probe, + .remove = ov6211_remove, +}; + +module_i2c_driver(ov6211_i2c_driver); + +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>"); +MODULE_DESCRIPTION("OmniVision OV6211 sensor driver"); +MODULE_LICENSE("GPL"); -- 2.49.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver 2025-08-12 21:30 ` [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy @ 2025-08-13 6:25 ` Tarang Raval 2025-08-18 12:08 ` Sakari Ailus 1 sibling, 0 replies; 8+ messages in thread From: Tarang Raval @ 2025-08-13 6:25 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Conor Dooley, Sakari Ailus, Mauro Carvalho Chehab, Krzysztof Kozlowski, linux-media@vger.kernel.org, Rob Herring, devicetree@vger.kernel.org, Hans Verkuil Hi Vladimir, I have very small comments below, you can check them. Since these are minor, you may want to wait for others review before making changes. With the below changes, it looks good to me. > OmniVision OV6211 is a monochrome image sensor, which produces frames in > 8/10-bit raw output format and supports 400x400, 200x200 and 100x100 > output image resolution modes. > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > --- > MAINTAINERS | 8 + > drivers/media/i2c/Kconfig | 10 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov6211.c | 809 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 828 insertions(+) > create mode 100644 drivers/media/i2c/ov6211.c ... > +static int ov6211_init_controls(struct ov6211 *ov6211) > +{ > + struct v4l2_ctrl_handler *ctrl_hdlr = &ov6211->ctrl_handler; > + const struct ov6211_mode *mode = &supported_modes[0]; > + struct v4l2_fwnode_device_properties props; > + s64 exposure_max, pixel_rate, h_blank; > + struct v4l2_ctrl *ctrl; > + int ret; > + > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); > + if (ret) > + return ret; No need to explicitly check the return value of v4l2_ctrl_handler_init() here, so this can be dropped. > + ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov6211_ctrl_ops, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(link_freq_menu_items) - 1, > + 0, link_freq_menu_items); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + pixel_rate = link_freq_menu_items[0] / mode->bpp; > + v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_PIXEL_RATE, > + 0, pixel_rate, 1, pixel_rate); > + > + h_blank = mode->hts - mode->width; > + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_HBLANK, > + h_blank, h_blank, 1, h_blank); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_VBLANK, > + mode->vts - mode->height, > + mode->vts - mode->height, 1, > + mode->vts - mode->height); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, > + OV6211_ANALOGUE_GAIN_MIN, OV6211_ANALOGUE_GAIN_MAX, > + OV6211_ANALOGUE_GAIN_STEP, > + OV6211_ANALOGUE_GAIN_DEFAULT); > + > + exposure_max = (mode->vts - OV6211_EXPOSURE_MAX_MARGIN); > + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, > + V4L2_CID_EXPOSURE, > + OV6211_EXPOSURE_MIN, exposure_max, > + OV6211_EXPOSURE_STEP, > + OV6211_EXPOSURE_DEFAULT); > + > + if (ctrl_hdlr->error) > + return ctrl_hdlr->error; > + > + ret = v4l2_fwnode_device_parse(ov6211->dev, &props); > + if (ret) > + goto error_free_hdlr; > + > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov6211_ctrl_ops, > + &props); > + if (ret) > + goto error_free_hdlr; > + > + ov6211->sd.ctrl_handler = ctrl_hdlr; > + > + return 0; > + > +error_free_hdlr: > + v4l2_ctrl_handler_free(ctrl_hdlr); > + > + return ret; > +} ... > +static int ov6211_enable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + const struct ov6211_reg_list *reg_list = &supported_modes[0].reg_list; > + struct ov6211 *ov6211 = to_ov6211(sd); > + int ret; > + > + ret = pm_runtime_resume_and_get(ov6211->dev); > + if (ret) > + return ret; > + > + /* Skip a step of explicit entering into the standby mode */ > + ret = cci_write(ov6211->regmap, OV6211_REG_SOFTWARE_RST, > + OV6211_SOFTWARE_RST, NULL); > + if (ret) { > + dev_err(ov6211->dev, "failed to software reset: %d\n", ret); > + goto error; > + } > + > + ret = cci_multi_reg_write(ov6211->regmap, reg_list->regs, > + reg_list->num_regs, NULL); > + if (ret) { > + dev_err(ov6211->dev, "failed to set mode: %d\n", ret); > + goto error; > + } > + > + ret = __v4l2_ctrl_handler_setup(ov6211->sd.ctrl_handler); > + if (ret) > + goto error; > + > + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, > + OV6211_MODE_STREAMING, NULL); > + if (ret) { > + dev_err(ov6211->dev, "failed to start streaming: %d\n", ret); > + goto error; > + } > + > + return 0; > + > +error: > + pm_runtime_mark_last_busy(ov6211->dev); > + pm_runtime_put_autosuspend(ov6211->dev); The call to pm_runtime_mark_last_busy() is now redundant, as it has been merged into pm_runtime_put_autosuspend(). see: https://lore.kernel.org/all/20250616061212.2286741-3-sakari.ailus@linux.intel.com/ > + return ret; > +} > + > +static int ov6211_disable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + struct ov6211 *ov6211 = to_ov6211(sd); > + int ret; > + > + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, > + OV6211_MODE_STANDBY, NULL); > + if (ret) > + dev_err(ov6211->dev, "failed to stop streaming: %d\n", ret); > + > + pm_runtime_mark_last_busy(ov6211->dev); > + pm_runtime_put_autosuspend(ov6211->dev); same here > + return ret; > +} > + ... > +static const struct of_device_id ov6211_of_match[] = { > + { .compatible = "ovti,ov6211" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, ov6211_of_match); > + > +static struct i2c_driver ov6211_i2c_driver = { > + .driver = { > + .name = "ov6211", > + .pm = &ov6211_pm_ops, > + .of_match_table = ov6211_of_match, > + }, > + .probe = ov6211_probe, > + .remove = ov6211_remove, > +}; > + > +module_i2c_driver(ov6211_i2c_driver); > + > +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>"); > +MODULE_DESCRIPTION("OmniVision OV6211 sensor driver"); > +MODULE_LICENSE("GPL"); > -- > 2.49.0 Best Regards, Tarang ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver 2025-08-12 21:30 ` [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy 2025-08-13 6:25 ` Tarang Raval @ 2025-08-18 12:08 ` Sakari Ailus 2025-08-19 22:57 ` Vladimir Zapolskiy 1 sibling, 1 reply; 8+ messages in thread From: Sakari Ailus @ 2025-08-18 12:08 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, Tarang Raval, linux-media, devicetree Hi Vladimir, Thanks for the update. On Wed, Aug 13, 2025 at 12:30:24AM +0300, Vladimir Zapolskiy wrote: > OmniVision OV6211 is a monochrome image sensor, which produces frames in > 8/10-bit raw output format and supports 400x400, 200x200 and 100x100 > output image resolution modes. > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > --- > MAINTAINERS | 8 + > drivers/media/i2c/Kconfig | 10 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov6211.c | 809 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 828 insertions(+) > create mode 100644 drivers/media/i2c/ov6211.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index fe168477caa4..f2af41bd19b0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -18722,6 +18722,14 @@ S: Maintained > T: git git://linuxtv.org/media.git > F: drivers/media/i2c/ov5695.c > > +OMNIVISION OV6211 SENSOR DRIVER > +M: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git > +F: Documentation/devicetree/bindings/media/i2c/ovti,ov6211.yaml > +F: drivers/media/i2c/ov6211.c > + > OMNIVISION OV64A40 SENSOR DRIVER > M: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > L: linux-media@vger.kernel.org > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 6237fe804a5c..13ff3c153117 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -542,6 +542,16 @@ config VIDEO_OV5695 > To compile this driver as a module, choose M here: the > module will be called ov5695. > > +config VIDEO_OV6211 > + tristate "OmniVision OV6211 sensor support" > + select V4L2_CCI_I2C > + help > + This is a Video4Linux2 sensor driver for the OmniVision > + OV6211 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called ov6211. > + > config VIDEO_OV64A40 > tristate "OmniVision OV64A40 sensor support" > select V4L2_CCI_I2C > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index 5873d29433ee..03814fc8e2f7 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -103,6 +103,7 @@ obj-$(CONFIG_VIDEO_OV5670) += ov5670.o > obj-$(CONFIG_VIDEO_OV5675) += ov5675.o > obj-$(CONFIG_VIDEO_OV5693) += ov5693.o > obj-$(CONFIG_VIDEO_OV5695) += ov5695.o > +obj-$(CONFIG_VIDEO_OV6211) += ov6211.o > obj-$(CONFIG_VIDEO_OV64A40) += ov64a40.o > obj-$(CONFIG_VIDEO_OV6650) += ov6650.o > obj-$(CONFIG_VIDEO_OV7251) += ov7251.o > diff --git a/drivers/media/i2c/ov6211.c b/drivers/media/i2c/ov6211.c > new file mode 100644 > index 000000000000..d48b13399b2e > --- /dev/null > +++ b/drivers/media/i2c/ov6211.c > @@ -0,0 +1,809 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2024-2025 Linaro Ltd > + > +#include <linux/clk.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/v4l2-cci.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-fwnode.h> > + > +#define OV6211_LINK_FREQ_480MHZ 480000000ULL > +#define OV6211_MCLK_FREQ_24MHZ 24000000 > + > +#define OV6211_REG_CHIP_ID CCI_REG16(0x300a) > +#define OV6211_CHIP_ID 0x6710 > + > +#define OV6211_REG_MODE_SELECT CCI_REG8(0x0100) > +#define OV6211_MODE_STANDBY 0x00 > +#define OV6211_MODE_STREAMING 0x01 > + > +#define OV6211_REG_SOFTWARE_RST CCI_REG8(0x0103) > +#define OV6211_SOFTWARE_RST 0x01 > + > +/* Exposure controls from sensor */ > +#define OV6211_REG_EXPOSURE CCI_REG24(0x3500) > +#define OV6211_EXPOSURE_MIN 1 > +#define OV6211_EXPOSURE_MAX_MARGIN 4 > +#define OV6211_EXPOSURE_STEP 1 > +#define OV6211_EXPOSURE_DEFAULT 210 > + > +/* Analogue gain controls from sensor */ > +#define OV6211_REG_ANALOGUE_GAIN CCI_REG16(0x350a) > +#define OV6211_ANALOGUE_GAIN_MIN 1 > +#define OV6211_ANALOGUE_GAIN_MAX 0x3ff > +#define OV6211_ANALOGUE_GAIN_STEP 1 > +#define OV6211_ANALOGUE_GAIN_DEFAULT 0xa0 > + > +#define to_ov6211(_sd) container_of(_sd, struct ov6211, sd) > + > +struct ov6211_reg_list { > + const struct cci_reg_sequence *regs; > + unsigned int num_regs; > +}; > + > +struct ov6211_mode { > + u32 width; /* Frame width in pixels */ > + u32 height; /* Frame height in pixels */ > + u32 hts; /* Horizontal timing size */ > + u32 vts; /* Default vertical timing size */ > + u32 bpp; /* Bits per pixel */ > + > + const struct ov6211_reg_list reg_list; /* Sensor register setting */ > +}; > + > +static const s64 link_freq_menu_items[] = { > + OV6211_LINK_FREQ_480MHZ, > +}; > + > +static const struct cci_reg_sequence ov6211_400x400_120fps_mode[] = { > + { CCI_REG8(0x3005), 0x00 }, > + { CCI_REG8(0x3013), 0x12 }, > + { CCI_REG8(0x3014), 0x04 }, > + { CCI_REG8(0x3016), 0x10 }, > + { CCI_REG8(0x3017), 0x00 }, > + { CCI_REG8(0x3018), 0x00 }, > + { CCI_REG8(0x301a), 0x00 }, > + { CCI_REG8(0x301b), 0x00 }, > + { CCI_REG8(0x301c), 0x00 }, > + { CCI_REG8(0x3037), 0xf0 }, > + { CCI_REG8(0x3080), 0x01 }, > + { CCI_REG8(0x3081), 0x00 }, > + { CCI_REG8(0x3082), 0x01 }, > + { CCI_REG8(0x3098), 0x04 }, > + { CCI_REG8(0x3099), 0x28 }, > + { CCI_REG8(0x309a), 0x06 }, > + { CCI_REG8(0x309b), 0x04 }, > + { CCI_REG8(0x309c), 0x00 }, > + { CCI_REG8(0x309d), 0x00 }, > + { CCI_REG8(0x309e), 0x01 }, > + { CCI_REG8(0x309f), 0x00 }, > + { CCI_REG8(0x30b0), 0x08 }, > + { CCI_REG8(0x30b1), 0x02 }, > + { CCI_REG8(0x30b2), 0x00 }, > + { CCI_REG8(0x30b3), 0x28 }, > + { CCI_REG8(0x30b4), 0x02 }, > + { CCI_REG8(0x30b5), 0x00 }, > + { CCI_REG8(0x3106), 0xd9 }, > + { CCI_REG8(0x3503), 0x07 }, > + { CCI_REG8(0x3509), 0x10 }, > + { CCI_REG8(0x3600), 0xfc }, > + { CCI_REG8(0x3620), 0xb7 }, > + { CCI_REG8(0x3621), 0x05 }, > + { CCI_REG8(0x3626), 0x31 }, > + { CCI_REG8(0x3627), 0x40 }, > + { CCI_REG8(0x3632), 0xa3 }, > + { CCI_REG8(0x3633), 0x34 }, > + { CCI_REG8(0x3634), 0x40 }, > + { CCI_REG8(0x3636), 0x00 }, > + { CCI_REG8(0x3660), 0x80 }, > + { CCI_REG8(0x3662), 0x03 }, > + { CCI_REG8(0x3664), 0xf0 }, > + { CCI_REG8(0x366a), 0x10 }, > + { CCI_REG8(0x366b), 0x06 }, > + { CCI_REG8(0x3680), 0xf4 }, > + { CCI_REG8(0x3681), 0x50 }, > + { CCI_REG8(0x3682), 0x00 }, > + { CCI_REG8(0x3708), 0x20 }, > + { CCI_REG8(0x3709), 0x40 }, > + { CCI_REG8(0x370d), 0x03 }, > + { CCI_REG8(0x373b), 0x02 }, > + { CCI_REG8(0x373c), 0x08 }, > + { CCI_REG8(0x3742), 0x00 }, > + { CCI_REG8(0x3744), 0x16 }, > + { CCI_REG8(0x3745), 0x08 }, > + { CCI_REG8(0x3781), 0xfc }, > + { CCI_REG8(0x3788), 0x00 }, > + { CCI_REG8(0x3800), 0x00 }, > + { CCI_REG8(0x3801), 0x04 }, > + { CCI_REG8(0x3802), 0x00 }, > + { CCI_REG8(0x3803), 0x04 }, > + { CCI_REG8(0x3804), 0x01 }, > + { CCI_REG8(0x3805), 0x9b }, > + { CCI_REG8(0x3806), 0x01 }, > + { CCI_REG8(0x3807), 0x9b }, > + { CCI_REG8(0x3808), 0x01 }, > + { CCI_REG8(0x3809), 0x90 }, > + { CCI_REG8(0x380a), 0x01 }, > + { CCI_REG8(0x380b), 0x90 }, > + { CCI_REG8(0x380c), 0x05 }, > + { CCI_REG8(0x380d), 0xf2 }, > + { CCI_REG8(0x380e), 0x01 }, > + { CCI_REG8(0x380f), 0xb6 }, > + { CCI_REG8(0x3810), 0x00 }, > + { CCI_REG8(0x3811), 0x04 }, > + { CCI_REG8(0x3812), 0x00 }, > + { CCI_REG8(0x3813), 0x04 }, > + { CCI_REG8(0x3814), 0x11 }, > + { CCI_REG8(0x3815), 0x11 }, > + { CCI_REG8(0x3820), 0x00 }, > + { CCI_REG8(0x3821), 0x00 }, > + { CCI_REG8(0x382b), 0xfa }, > + { CCI_REG8(0x382f), 0x04 }, > + { CCI_REG8(0x3832), 0x00 }, > + { CCI_REG8(0x3833), 0x05 }, > + { CCI_REG8(0x3834), 0x00 }, > + { CCI_REG8(0x3835), 0x05 }, > + { CCI_REG8(0x3882), 0x04 }, > + { CCI_REG8(0x3883), 0x00 }, > + { CCI_REG8(0x38a4), 0x10 }, > + { CCI_REG8(0x38a5), 0x00 }, > + { CCI_REG8(0x38b1), 0x03 }, > + { CCI_REG8(0x3b80), 0x00 }, > + { CCI_REG8(0x3b81), 0xff }, > + { CCI_REG8(0x3b82), 0x10 }, > + { CCI_REG8(0x3b83), 0x00 }, > + { CCI_REG8(0x3b84), 0x08 }, > + { CCI_REG8(0x3b85), 0x00 }, > + { CCI_REG8(0x3b86), 0x01 }, > + { CCI_REG8(0x3b87), 0x00 }, > + { CCI_REG8(0x3b88), 0x00 }, > + { CCI_REG8(0x3b89), 0x00 }, > + { CCI_REG8(0x3b8a), 0x00 }, > + { CCI_REG8(0x3b8b), 0x05 }, > + { CCI_REG8(0x3b8c), 0x00 }, > + { CCI_REG8(0x3b8d), 0x00 }, > + { CCI_REG8(0x3b8e), 0x01 }, > + { CCI_REG8(0x3b8f), 0xb2 }, > + { CCI_REG8(0x3b94), 0x05 }, > + { CCI_REG8(0x3b95), 0xf2 }, > + { CCI_REG8(0x3b96), 0xc0 }, > + { CCI_REG8(0x4004), 0x04 }, > + { CCI_REG8(0x404e), 0x01 }, > + { CCI_REG8(0x4801), 0x0f }, > + { CCI_REG8(0x4806), 0x0f }, > + { CCI_REG8(0x4837), 0x43 }, > + { CCI_REG8(0x5a08), 0x00 }, > + { CCI_REG8(0x5a01), 0x00 }, > + { CCI_REG8(0x5a03), 0x00 }, > + { CCI_REG8(0x5a04), 0x10 }, > + { CCI_REG8(0x5a05), 0xa0 }, > + { CCI_REG8(0x5a06), 0x0c }, > + { CCI_REG8(0x5a07), 0x78 }, > +}; > + > +static const struct ov6211_mode supported_modes[] = { > + { > + .width = 400, > + .height = 400, > + .hts = 1522, > + .vts = 438, > + .bpp = 8, > + .reg_list = { > + .regs = ov6211_400x400_120fps_mode, > + .num_regs = ARRAY_SIZE(ov6211_400x400_120fps_mode), > + }, > + }, > +}; > + > +struct ov6211 { > + struct device *dev; > + struct regmap *regmap; > + struct clk *xvclk; > + struct gpio_desc *reset_gpio; > + struct regulator *avdd; > + struct regulator *dovdd; > + struct regulator *dvdd; > + > + struct v4l2_subdev sd; > + struct media_pad pad; > + > + struct v4l2_ctrl_handler ctrl_handler; > +}; > + > +static int ov6211_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct ov6211 *ov6211 = container_of(ctrl->handler, struct ov6211, > + ctrl_handler); > + int ret; > + > + /* V4L2 controls values will be applied only when power is already up */ > + if (!pm_runtime_get_if_in_use(ov6211->dev)) I think this should be pm_runtime_get_if_active() as no writes will now take place even if the sensor is powered on. > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_ANALOGUE_GAIN: > + ret = cci_write(ov6211->regmap, OV6211_REG_ANALOGUE_GAIN, > + ctrl->val, NULL); > + break; > + case V4L2_CID_EXPOSURE: > + ret = cci_write(ov6211->regmap, OV6211_REG_EXPOSURE, > + ctrl->val << 4, NULL); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + pm_runtime_put(ov6211->dev); > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops ov6211_ctrl_ops = { > + .s_ctrl = ov6211_set_ctrl, > +}; > + > +static int ov6211_init_controls(struct ov6211 *ov6211) > +{ > + struct v4l2_ctrl_handler *ctrl_hdlr = &ov6211->ctrl_handler; > + const struct ov6211_mode *mode = &supported_modes[0]; > + struct v4l2_fwnode_device_properties props; > + s64 exposure_max, pixel_rate, h_blank; > + struct v4l2_ctrl *ctrl; > + int ret; > + > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); > + if (ret) > + return ret; This check can be omitted. Up to you. > + > + ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov6211_ctrl_ops, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(link_freq_menu_items) - 1, > + 0, link_freq_menu_items); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + pixel_rate = link_freq_menu_items[0] / mode->bpp; > + v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_PIXEL_RATE, > + 0, pixel_rate, 1, pixel_rate); > + > + h_blank = mode->hts - mode->width; > + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_HBLANK, > + h_blank, h_blank, 1, h_blank); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_VBLANK, > + mode->vts - mode->height, > + mode->vts - mode->height, 1, > + mode->vts - mode->height); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, > + OV6211_ANALOGUE_GAIN_MIN, OV6211_ANALOGUE_GAIN_MAX, > + OV6211_ANALOGUE_GAIN_STEP, > + OV6211_ANALOGUE_GAIN_DEFAULT); > + > + exposure_max = (mode->vts - OV6211_EXPOSURE_MAX_MARGIN); > + ctrl = v4l2_ctrl_new_std(ctrl_hdlr, &ov6211_ctrl_ops, > + V4L2_CID_EXPOSURE, > + OV6211_EXPOSURE_MIN, exposure_max, > + OV6211_EXPOSURE_STEP, > + OV6211_EXPOSURE_DEFAULT); > + > + if (ctrl_hdlr->error) > + return ctrl_hdlr->error; > + > + ret = v4l2_fwnode_device_parse(ov6211->dev, &props); > + if (ret) > + goto error_free_hdlr; > + > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov6211_ctrl_ops, > + &props); > + if (ret) > + goto error_free_hdlr; > + > + ov6211->sd.ctrl_handler = ctrl_hdlr; > + > + return 0; > + > +error_free_hdlr: > + v4l2_ctrl_handler_free(ctrl_hdlr); > + > + return ret; > +} > + > +static void ov6211_update_pad_format(const struct ov6211_mode *mode, > + struct v4l2_mbus_framefmt *fmt) > +{ > + fmt->code = MEDIA_BUS_FMT_Y8_1X8; > + fmt->width = mode->width; > + fmt->height = mode->height; > + fmt->field = V4L2_FIELD_NONE; > + fmt->colorspace = V4L2_COLORSPACE_RAW; > + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > + fmt->xfer_func = V4L2_XFER_FUNC_NONE; > +} > + > +static int ov6211_enable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + const struct ov6211_reg_list *reg_list = &supported_modes[0].reg_list; > + struct ov6211 *ov6211 = to_ov6211(sd); > + int ret; > + > + ret = pm_runtime_resume_and_get(ov6211->dev); > + if (ret) > + return ret; > + > + /* Skip a step of explicit entering into the standby mode */ > + ret = cci_write(ov6211->regmap, OV6211_REG_SOFTWARE_RST, > + OV6211_SOFTWARE_RST, NULL); > + if (ret) { > + dev_err(ov6211->dev, "failed to software reset: %d\n", ret); > + goto error; > + } > + > + ret = cci_multi_reg_write(ov6211->regmap, reg_list->regs, > + reg_list->num_regs, NULL); > + if (ret) { > + dev_err(ov6211->dev, "failed to set mode: %d\n", ret); > + goto error; > + } > + > + ret = __v4l2_ctrl_handler_setup(ov6211->sd.ctrl_handler); > + if (ret) > + goto error; > + > + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, > + OV6211_MODE_STREAMING, NULL); > + if (ret) { > + dev_err(ov6211->dev, "failed to start streaming: %d\n", ret); > + goto error; > + } > + > + return 0; > + > +error: > + pm_runtime_mark_last_busy(ov6211->dev); Please omit direct calls to pm_runtime_mark_last_busy(); this is now called via the runtime PM put autosuspend etc. functions. > + pm_runtime_put_autosuspend(ov6211->dev); > + > + return ret; > +} > + > +static int ov6211_disable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + struct ov6211 *ov6211 = to_ov6211(sd); > + int ret; > + > + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, > + OV6211_MODE_STANDBY, NULL); > + if (ret) > + dev_err(ov6211->dev, "failed to stop streaming: %d\n", ret); > + > + pm_runtime_mark_last_busy(ov6211->dev); > + pm_runtime_put_autosuspend(ov6211->dev); > + > + return ret; > +} > + > +static int ov6211_set_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *fmt) > +{ > + struct v4l2_mbus_framefmt *format; > + const struct ov6211_mode *mode; > + > + format = v4l2_subdev_state_get_format(state, 0); > + > + mode = v4l2_find_nearest_size(supported_modes, > + ARRAY_SIZE(supported_modes), > + width, height, > + fmt->format.width, > + fmt->format.height); > + > + ov6211_update_pad_format(mode, &fmt->format); > + *format = fmt->format; > + > + return 0; > +} > + > +static int ov6211_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + if (code->index > 0) > + return -EINVAL; > + > + code->code = MEDIA_BUS_FMT_Y8_1X8; > + > + return 0; > +} > + > +static int ov6211_enum_frame_size(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + if (fse->index >= ARRAY_SIZE(supported_modes)) > + return -EINVAL; > + > + if (fse->code != MEDIA_BUS_FMT_Y8_1X8) > + return -EINVAL; > + > + fse->min_width = supported_modes[fse->index].width; > + fse->max_width = fse->min_width; > + fse->min_height = supported_modes[fse->index].height; > + fse->max_height = fse->min_height; > + > + return 0; > +} > + > +static int ov6211_init_state(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_format fmt = { > + .which = V4L2_SUBDEV_FORMAT_TRY, > + .pad = 0, > + .format = { > + .code = MEDIA_BUS_FMT_Y8_1X8, > + .width = supported_modes[0].width, > + .height = supported_modes[0].height, > + }, > + }; > + > + ov6211_set_pad_format(sd, state, &fmt); > + > + return 0; > +} > + > +static const struct v4l2_subdev_video_ops ov6211_video_ops = { > + .s_stream = v4l2_subdev_s_stream_helper, > +}; > + > +static const struct v4l2_subdev_pad_ops ov6211_pad_ops = { > + .set_fmt = ov6211_set_pad_format, > + .get_fmt = v4l2_subdev_get_fmt, > + .enum_mbus_code = ov6211_enum_mbus_code, > + .enum_frame_size = ov6211_enum_frame_size, > + .enable_streams = ov6211_enable_streams, > + .disable_streams = ov6211_disable_streams, > +}; > + > +static const struct v4l2_subdev_ops ov6211_subdev_ops = { > + .video = &ov6211_video_ops, > + .pad = &ov6211_pad_ops, > +}; > + > +static const struct media_entity_operations ov6211_subdev_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static const struct v4l2_subdev_internal_ops ov6211_internal_ops = { > + .init_state = ov6211_init_state, > +}; > + > +static int ov6211_identify_module(struct ov6211 *ov6211) > +{ > + u64 val; > + int ret; > + > + ret = cci_read(ov6211->regmap, OV6211_REG_CHIP_ID, &val, NULL); > + if (ret) { > + dev_err(ov6211->dev, "failed to read chip id: %d\n", ret); > + return ret; > + } > + > + if (val != OV6211_CHIP_ID) { > + dev_err(ov6211->dev, "chip id mismatch: %x!=%llx\n", > + OV6211_CHIP_ID, val); > + return -ENODEV; > + } > + > + return 0; > +} > + > +static int ov6211_check_hwcfg(struct ov6211 *ov6211) > +{ > + struct fwnode_handle *fwnode = dev_fwnode(ov6211->dev), *ep; > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_CSI2_DPHY, > + }; > + int ret; > + > + if (!fwnode) > + return -ENODEV; > + > + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > + if (!ep) > + return -EINVAL; > + > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > + fwnode_handle_put(ep); > + if (ret) > + return ret; > + > + if (!bus_cfg.nr_of_link_frequencies) { > + dev_err(ov6211->dev, "link-frequency property is not found\n"); > + ret = -EINVAL; > + goto error; > + } > + > + if (bus_cfg.nr_of_link_frequencies != 1 || > + bus_cfg.link_frequencies[0] != link_freq_menu_items[0]) { Could you use v4l2_link_freq_to_bitmap()? I think it'd simplify the function and possibly error handling, too. > + dev_err(ov6211->dev, "Unsupported MIPI CSI2 link frequency\n"); > + ret = -EINVAL; > + goto error; > + } > + > +error: > + v4l2_fwnode_endpoint_free(&bus_cfg); > + > + return ret; > +} > + > +static int ov6211_power_on(struct device *dev) > +{ > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct ov6211 *ov6211 = to_ov6211(sd); > + int ret; > + > + if (ov6211->avdd) { > + ret = regulator_enable(ov6211->avdd); > + if (ret) > + return ret; > + } > + > + if (ov6211->dovdd) { > + ret = regulator_enable(ov6211->dovdd); > + if (ret) > + goto avdd_disable; > + } > + > + if (ov6211->dvdd) { > + ret = regulator_enable(ov6211->dvdd); > + if (ret) > + goto dovdd_disable; > + } Can you use the regulator bulk functions? Or if the order is required, add a comment about it? Otherwise someone will provide a "fix" soon afterwards. :-) > + > + gpiod_set_value_cansleep(ov6211->reset_gpio, 0); > + usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC); > + > + ret = clk_prepare_enable(ov6211->xvclk); Is the clock really supposed to be enabled here, and not e.g. before lifting reset? > + if (ret) > + goto reset_gpio; > + > + return 0; > + > +reset_gpio: > + gpiod_set_value_cansleep(ov6211->reset_gpio, 1); > + > + if (ov6211->dvdd) > + regulator_disable(ov6211->dvdd); > +dovdd_disable: > + if (ov6211->dovdd) > + regulator_disable(ov6211->dovdd); > +avdd_disable: > + if (ov6211->avdd) > + regulator_disable(ov6211->avdd); > + > + return ret; > +} > + > +static int ov6211_power_off(struct device *dev) > +{ > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct ov6211 *ov6211 = to_ov6211(sd); > + > + clk_disable_unprepare(ov6211->xvclk); > + > + gpiod_set_value_cansleep(ov6211->reset_gpio, 1); > + > + if (ov6211->dvdd) > + regulator_disable(ov6211->dvdd); > + > + if (ov6211->dovdd) > + regulator_disable(ov6211->dovdd); > + > + if (ov6211->avdd) > + regulator_disable(ov6211->avdd); > + > + return 0; > +} > + > +static int ov6211_probe(struct i2c_client *client) > +{ > + struct ov6211 *ov6211; > + unsigned long freq; > + int ret; > + > + ov6211 = devm_kzalloc(&client->dev, sizeof(*ov6211), GFP_KERNEL); > + if (!ov6211) > + return -ENOMEM; > + > + ov6211->dev = &client->dev; > + > + v4l2_i2c_subdev_init(&ov6211->sd, client, &ov6211_subdev_ops); > + ov6211->sd.dev = ov6211->dev; > + > + ov6211->regmap = devm_cci_regmap_init_i2c(client, 16); > + if (IS_ERR(ov6211->regmap)) > + return dev_err_probe(ov6211->dev, PTR_ERR(ov6211->regmap), > + "failed to init CCI\n"); > + > + ov6211->xvclk = devm_v4l2_sensor_clk_get(ov6211->dev, NULL); > + if (IS_ERR(ov6211->xvclk)) > + return dev_err_probe(ov6211->dev, PTR_ERR(ov6211->xvclk), > + "failed to get XVCLK clock\n"); > + > + freq = clk_get_rate(ov6211->xvclk); > + if (freq && freq != OV6211_MCLK_FREQ_24MHZ) > + return dev_err_probe(ov6211->dev, -EINVAL, > + "XVCLK clock frequency %lu is not supported\n", > + freq); > + > + ret = ov6211_check_hwcfg(ov6211); > + if (ret) > + return dev_err_probe(ov6211->dev, ret, > + "failed to check HW configuration\n"); > + > + ov6211->reset_gpio = devm_gpiod_get_optional(ov6211->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(ov6211->reset_gpio)) > + return dev_err_probe(ov6211->dev, PTR_ERR(ov6211->reset_gpio), > + "cannot get reset GPIO\n"); > + > + ov6211->avdd = devm_regulator_get_optional(ov6211->dev, "avdd"); > + if (IS_ERR(ov6211->avdd)) { > + ret = PTR_ERR(ov6211->avdd); > + if (ret != -ENODEV) > + return dev_err_probe(ov6211->dev, ret, > + "Failed to get avdd regulator\n"); > + > + ov6211->avdd = NULL; > + } > + > + ov6211->dovdd = devm_regulator_get_optional(ov6211->dev, "dovdd"); > + if (IS_ERR(ov6211->dovdd)) { > + ret = PTR_ERR(ov6211->dovdd); > + if (ret != -ENODEV) > + return dev_err_probe(ov6211->dev, ret, > + "Failed to get dovdd regulator\n"); > + > + ov6211->dovdd = NULL; > + } > + > + ov6211->dvdd = devm_regulator_get_optional(ov6211->dev, "dvdd"); > + if (IS_ERR(ov6211->dvdd)) { > + ret = PTR_ERR(ov6211->dvdd); > + if (ret != -ENODEV) > + return dev_err_probe(ov6211->dev, ret, > + "Failed to get dvdd regulator\n"); > + > + ov6211->dvdd = NULL; > + } > + > + /* The sensor must be powered on to read the CHIP_ID register */ > + ret = ov6211_power_on(ov6211->dev); > + if (ret) > + return ret; > + > + ret = ov6211_identify_module(ov6211); > + if (ret) { > + dev_err_probe(ov6211->dev, ret, "failed to find sensor\n"); > + goto power_off; > + } > + > + ret = ov6211_init_controls(ov6211); > + if (ret) { > + dev_err_probe(ov6211->dev, ret, "failed to init controls\n"); > + goto power_off; > + } > + > + ov6211->sd.state_lock = ov6211->ctrl_handler.lock; > + ov6211->sd.internal_ops = &ov6211_internal_ops; > + ov6211->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + ov6211->sd.entity.ops = &ov6211_subdev_entity_ops; > + ov6211->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + ov6211->pad.flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&ov6211->sd.entity, 1, &ov6211->pad); > + if (ret) { > + dev_err_probe(ov6211->dev, ret, > + "failed to init media entity pads\n"); > + goto v4l2_ctrl_handler_free; > + } > + > + ret = v4l2_subdev_init_finalize(&ov6211->sd); > + if (ret < 0) { > + dev_err_probe(ov6211->dev, ret, > + "failed to init media entity pads\n"); > + goto media_entity_cleanup; > + } > + > + pm_runtime_set_active(ov6211->dev); > + pm_runtime_enable(ov6211->dev); > + > + ret = v4l2_async_register_subdev_sensor(&ov6211->sd); > + if (ret < 0) { > + dev_err_probe(ov6211->dev, ret, > + "failed to register V4L2 subdev\n"); > + goto subdev_cleanup; > + } > + > + /* Enable runtime PM and turn off the device */ > + pm_runtime_idle(ov6211->dev); > + pm_runtime_set_autosuspend_delay(ov6211->dev, 1000); > + pm_runtime_use_autosuspend(ov6211->dev); > + > + return 0; > + > +subdev_cleanup: > + v4l2_subdev_cleanup(&ov6211->sd); > + pm_runtime_disable(ov6211->dev); > + pm_runtime_set_suspended(ov6211->dev); > + > +media_entity_cleanup: > + media_entity_cleanup(&ov6211->sd.entity); > + > +v4l2_ctrl_handler_free: > + v4l2_ctrl_handler_free(ov6211->sd.ctrl_handler); > + > +power_off: > + ov6211_power_off(ov6211->dev); > + > + return ret; > +} > + > +static void ov6211_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov6211 *ov6211 = to_ov6211(sd); > + > + v4l2_async_unregister_subdev(sd); > + v4l2_subdev_cleanup(sd); > + media_entity_cleanup(&sd->entity); > + v4l2_ctrl_handler_free(sd->ctrl_handler); > + pm_runtime_disable(ov6211->dev); > + > + if (!pm_runtime_status_suspended(ov6211->dev)) { > + ov6211_power_off(ov6211->dev); > + pm_runtime_set_suspended(ov6211->dev); > + } > +} > + > +static const struct dev_pm_ops ov6211_pm_ops = { > + SET_RUNTIME_PM_OPS(ov6211_power_off, ov6211_power_on, NULL) > +}; > + > +static const struct of_device_id ov6211_of_match[] = { > + { .compatible = "ovti,ov6211" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, ov6211_of_match); > + > +static struct i2c_driver ov6211_i2c_driver = { > + .driver = { > + .name = "ov6211", > + .pm = &ov6211_pm_ops, > + .of_match_table = ov6211_of_match, > + }, > + .probe = ov6211_probe, > + .remove = ov6211_remove, > +}; > + > +module_i2c_driver(ov6211_i2c_driver); > + > +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>"); > +MODULE_DESCRIPTION("OmniVision OV6211 sensor driver"); > +MODULE_LICENSE("GPL"); -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver 2025-08-18 12:08 ` Sakari Ailus @ 2025-08-19 22:57 ` Vladimir Zapolskiy 2025-08-21 9:18 ` Sakari Ailus 0 siblings, 1 reply; 8+ messages in thread From: Vladimir Zapolskiy @ 2025-08-19 22:57 UTC (permalink / raw) To: Sakari Ailus Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, Tarang Raval, linux-media, devicetree Hi Sakari, thank you so much for review. Please find a few comments below. On 8/18/25 15:08, Sakari Ailus wrote: > Hi Vladimir, > > Thanks for the update. > > On Wed, Aug 13, 2025 at 12:30:24AM +0300, Vladimir Zapolskiy wrote: >> OmniVision OV6211 is a monochrome image sensor, which produces frames in >> 8/10-bit raw output format and supports 400x400, 200x200 and 100x100 >> output image resolution modes. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> <snip> >> +static int ov6211_set_ctrl(struct v4l2_ctrl *ctrl) >> +{ >> + struct ov6211 *ov6211 = container_of(ctrl->handler, struct ov6211, >> + ctrl_handler); >> + int ret; >> + >> + /* V4L2 controls values will be applied only when power is already up */ >> + if (!pm_runtime_get_if_in_use(ov6211->dev)) > > I think this should be pm_runtime_get_if_active() as no writes will now > take place even if the sensor is powered on. Ack. >> + return 0; >> + >> + switch (ctrl->id) { >> + case V4L2_CID_ANALOGUE_GAIN: >> + ret = cci_write(ov6211->regmap, OV6211_REG_ANALOGUE_GAIN, >> + ctrl->val, NULL); >> + break; >> + case V4L2_CID_EXPOSURE: >> + ret = cci_write(ov6211->regmap, OV6211_REG_EXPOSURE, >> + ctrl->val << 4, NULL); >> + break; >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + pm_runtime_put(ov6211->dev); >> + >> + return ret; >> +} >> + >> +static const struct v4l2_ctrl_ops ov6211_ctrl_ops = { >> + .s_ctrl = ov6211_set_ctrl, >> +}; >> + >> +static int ov6211_init_controls(struct ov6211 *ov6211) >> +{ >> + struct v4l2_ctrl_handler *ctrl_hdlr = &ov6211->ctrl_handler; >> + const struct ov6211_mode *mode = &supported_modes[0]; >> + struct v4l2_fwnode_device_properties props; >> + s64 exposure_max, pixel_rate, h_blank; >> + struct v4l2_ctrl *ctrl; >> + int ret; >> + >> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); >> + if (ret) >> + return ret; > > This check can be omitted. Up to you. This check I would prefer to keep, otherwise I have a feeling that under ENOMEM the following code would make an attempt to insert the controls data over an NULL pointer... I'll mock it to check it explicitly though, but I'm inclined to keep it. <snip> >> + >> + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, >> + OV6211_MODE_STREAMING, NULL); >> + if (ret) { >> + dev_err(ov6211->dev, "failed to start streaming: %d\n", ret); >> + goto error; >> + } >> + >> + return 0; >> + >> +error: >> + pm_runtime_mark_last_busy(ov6211->dev); > > Please omit direct calls to pm_runtime_mark_last_busy(); this is now called > via the runtime PM put autosuspend etc. functions. > Ack. <snip> >> + if (bus_cfg.nr_of_link_frequencies != 1 || >> + bus_cfg.link_frequencies[0] != link_freq_menu_items[0]) { > > Could you use v4l2_link_freq_to_bitmap()? I think it'd simplify the > function and possibly error handling, too. > Ack, let it be so. >> + dev_err(ov6211->dev, "Unsupported MIPI CSI2 link frequency\n"); >> + ret = -EINVAL; >> + goto error; >> + } >> + >> +error: >> + v4l2_fwnode_endpoint_free(&bus_cfg); >> + >> + return ret; >> +} >> + >> +static int ov6211_power_on(struct device *dev) >> +{ >> + struct v4l2_subdev *sd = dev_get_drvdata(dev); >> + struct ov6211 *ov6211 = to_ov6211(sd); >> + int ret; >> + >> + if (ov6211->avdd) { >> + ret = regulator_enable(ov6211->avdd); >> + if (ret) >> + return ret; >> + } >> + >> + if (ov6211->dovdd) { >> + ret = regulator_enable(ov6211->dovdd); >> + if (ret) >> + goto avdd_disable; >> + } >> + >> + if (ov6211->dvdd) { >> + ret = regulator_enable(ov6211->dvdd); >> + if (ret) >> + goto dovdd_disable; >> + } > > Can you use the regulator bulk functions? Or if the order is required, add > a comment about it? Otherwise someone will provide a "fix" soon afterwards. > :-) I've already responded to a similar review comment from Krzysztof on v1, here the rationale is to follow a deliberately selected model of having some of regulators as optional. Moreover the "digital core" DVDD regulator is truly optional, there is a working sensor configuration to omit it. Unfortunately so far there is no bulk regulator helper, which deals properly with optional regulators, so I would prefer to keep it registered this way. >> + >> + gpiod_set_value_cansleep(ov6211->reset_gpio, 0); >> + usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC); >> + >> + ret = clk_prepare_enable(ov6211->xvclk); > > Is the clock really supposed to be enabled here, and not e.g. before > lifting reset? > The so called "gated" mode of XVCLK clock says that is should be enabled after releasing XSHUTDOWN pin, so I don't see a mistake here. Please note, it's basically a common part with other OmniVision sensors, for instance you may get a reference from OG01A1B product specification etc. Thank you for review. Shortly I'll send another new but very similar image sensor driver acknowledging your given comments. -- Best wishes, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver 2025-08-19 22:57 ` Vladimir Zapolskiy @ 2025-08-21 9:18 ` Sakari Ailus 2025-08-22 8:22 ` Vladimir Zapolskiy 0 siblings, 1 reply; 8+ messages in thread From: Sakari Ailus @ 2025-08-21 9:18 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, Tarang Raval, linux-media, devicetree Hi Vladimir, On Wed, Aug 20, 2025 at 01:57:52AM +0300, Vladimir Zapolskiy wrote: > Hi Sakari, > > thank you so much for review. Please find a few comments below. > > On 8/18/25 15:08, Sakari Ailus wrote: > > Hi Vladimir, > > > > Thanks for the update. > > > > On Wed, Aug 13, 2025 at 12:30:24AM +0300, Vladimir Zapolskiy wrote: > > > OmniVision OV6211 is a monochrome image sensor, which produces frames in > > > 8/10-bit raw output format and supports 400x400, 200x200 and 100x100 > > > output image resolution modes. > > > > > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > > <snip> > > > > +static int ov6211_set_ctrl(struct v4l2_ctrl *ctrl) > > > +{ > > > + struct ov6211 *ov6211 = container_of(ctrl->handler, struct ov6211, > > > + ctrl_handler); > > > + int ret; > > > + > > > + /* V4L2 controls values will be applied only when power is already up */ > > > + if (!pm_runtime_get_if_in_use(ov6211->dev)) > > > > I think this should be pm_runtime_get_if_active() as no writes will now > > take place even if the sensor is powered on. > > Ack. > > > > + return 0; > > > + > > > + switch (ctrl->id) { > > > + case V4L2_CID_ANALOGUE_GAIN: > > > + ret = cci_write(ov6211->regmap, OV6211_REG_ANALOGUE_GAIN, > > > + ctrl->val, NULL); > > > + break; > > > + case V4L2_CID_EXPOSURE: > > > + ret = cci_write(ov6211->regmap, OV6211_REG_EXPOSURE, > > > + ctrl->val << 4, NULL); > > > + break; > > > + default: > > > + ret = -EINVAL; > > > + break; > > > + } > > > + > > > + pm_runtime_put(ov6211->dev); > > > + > > > + return ret; > > > +} > > > + > > > +static const struct v4l2_ctrl_ops ov6211_ctrl_ops = { > > > + .s_ctrl = ov6211_set_ctrl, > > > +}; > > > + > > > +static int ov6211_init_controls(struct ov6211 *ov6211) > > > +{ > > > + struct v4l2_ctrl_handler *ctrl_hdlr = &ov6211->ctrl_handler; > > > + const struct ov6211_mode *mode = &supported_modes[0]; > > > + struct v4l2_fwnode_device_properties props; > > > + s64 exposure_max, pixel_rate, h_blank; > > > + struct v4l2_ctrl *ctrl; > > > + int ret; > > > + > > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); > > > + if (ret) > > > + return ret; > > > > This check can be omitted. Up to you. > > This check I would prefer to keep, otherwise I have a feeling that under > ENOMEM the following code would make an attempt to insert the controls > data over an NULL pointer... > > I'll mock it to check it explicitly though, but I'm inclined to keep it. The control framework will handle that internally. > > <snip> > > > > + > > > + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, > > > + OV6211_MODE_STREAMING, NULL); > > > + if (ret) { > > > + dev_err(ov6211->dev, "failed to start streaming: %d\n", ret); > > > + goto error; > > > + } > > > + > > > + return 0; > > > + > > > +error: > > > + pm_runtime_mark_last_busy(ov6211->dev); > > > > Please omit direct calls to pm_runtime_mark_last_busy(); this is now called > > via the runtime PM put autosuspend etc. functions. > > > > Ack. > > <snip> > > > > + if (bus_cfg.nr_of_link_frequencies != 1 || > > > + bus_cfg.link_frequencies[0] != link_freq_menu_items[0]) { > > > > Could you use v4l2_link_freq_to_bitmap()? I think it'd simplify the > > function and possibly error handling, too. > > > > Ack, let it be so. > > > > + dev_err(ov6211->dev, "Unsupported MIPI CSI2 link frequency\n"); > > > + ret = -EINVAL; > > > + goto error; > > > + } > > > + > > > +error: > > > + v4l2_fwnode_endpoint_free(&bus_cfg); > > > + > > > + return ret; > > > +} > > > + > > > +static int ov6211_power_on(struct device *dev) > > > +{ > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > > + struct ov6211 *ov6211 = to_ov6211(sd); > > > + int ret; > > > + > > > + if (ov6211->avdd) { > > > + ret = regulator_enable(ov6211->avdd); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + if (ov6211->dovdd) { > > > + ret = regulator_enable(ov6211->dovdd); > > > + if (ret) > > > + goto avdd_disable; > > > + } > > > + > > > + if (ov6211->dvdd) { > > > + ret = regulator_enable(ov6211->dvdd); > > > + if (ret) > > > + goto dovdd_disable; > > > + } > > > > Can you use the regulator bulk functions? Or if the order is required, add > > a comment about it? Otherwise someone will provide a "fix" soon afterwards. > > :-) > > I've already responded to a similar review comment from Krzysztof on v1, > here the rationale is to follow a deliberately selected model of having > some of regulators as optional. Moreover the "digital core" DVDD regulator > is truly optional, there is a working sensor configuration to omit it. > > Unfortunately so far there is no bulk regulator helper, which deals properly > with optional regulators, so I would prefer to keep it registered this way. Won't the regulator framework provide you a dummy regulator if a given regulator isn't assigned to the device in DT? E.g. the CCS driver unconditionally requests regulators and fails on error. It still works on ACPI platforms without any regulators (as in terms of the regulator framework). > > > + gpiod_set_value_cansleep(ov6211->reset_gpio, 0); > > > + usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC); > > > + > > > + ret = clk_prepare_enable(ov6211->xvclk); > > > > Is the clock really supposed to be enabled here, and not e.g. before > > lifting reset? > > > > The so called "gated" mode of XVCLK clock says that is should be enabled > after releasing XSHUTDOWN pin, so I don't see a mistake here. > > Please note, it's basically a common part with other OmniVision sensors, > for instance you may get a reference from OG01A1B product specification etc. > > Thank you for review. Shortly I'll send another new but very similar > image sensor driver acknowledging your given comments. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver 2025-08-21 9:18 ` Sakari Ailus @ 2025-08-22 8:22 ` Vladimir Zapolskiy 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Zapolskiy @ 2025-08-22 8:22 UTC (permalink / raw) To: Sakari Ailus Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, Tarang Raval, linux-media, devicetree Hi Sakari. On 8/21/25 12:18, Sakari Ailus wrote: > Hi Vladimir, > > On Wed, Aug 20, 2025 at 01:57:52AM +0300, Vladimir Zapolskiy wrote: >> Hi Sakari, >> >> thank you so much for review. Please find a few comments below. >> >> On 8/18/25 15:08, Sakari Ailus wrote: >>> Hi Vladimir, >>> >>> Thanks for the update. >>> >>> On Wed, Aug 13, 2025 at 12:30:24AM +0300, Vladimir Zapolskiy wrote: >>>> OmniVision OV6211 is a monochrome image sensor, which produces frames in >>>> 8/10-bit raw output format and supports 400x400, 200x200 and 100x100 >>>> output image resolution modes. >>>> >>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> >> >> <snip> >> >>>> +static int ov6211_set_ctrl(struct v4l2_ctrl *ctrl) >>>> +{ >>>> + struct ov6211 *ov6211 = container_of(ctrl->handler, struct ov6211, >>>> + ctrl_handler); >>>> + int ret; >>>> + >>>> + /* V4L2 controls values will be applied only when power is already up */ >>>> + if (!pm_runtime_get_if_in_use(ov6211->dev)) >>> >>> I think this should be pm_runtime_get_if_active() as no writes will now >>> take place even if the sensor is powered on. >> >> Ack. >> >>>> + return 0; >>>> + >>>> + switch (ctrl->id) { >>>> + case V4L2_CID_ANALOGUE_GAIN: >>>> + ret = cci_write(ov6211->regmap, OV6211_REG_ANALOGUE_GAIN, >>>> + ctrl->val, NULL); >>>> + break; >>>> + case V4L2_CID_EXPOSURE: >>>> + ret = cci_write(ov6211->regmap, OV6211_REG_EXPOSURE, >>>> + ctrl->val << 4, NULL); >>>> + break; >>>> + default: >>>> + ret = -EINVAL; >>>> + break; >>>> + } >>>> + >>>> + pm_runtime_put(ov6211->dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct v4l2_ctrl_ops ov6211_ctrl_ops = { >>>> + .s_ctrl = ov6211_set_ctrl, >>>> +}; >>>> + >>>> +static int ov6211_init_controls(struct ov6211 *ov6211) >>>> +{ >>>> + struct v4l2_ctrl_handler *ctrl_hdlr = &ov6211->ctrl_handler; >>>> + const struct ov6211_mode *mode = &supported_modes[0]; >>>> + struct v4l2_fwnode_device_properties props; >>>> + s64 exposure_max, pixel_rate, h_blank; >>>> + struct v4l2_ctrl *ctrl; >>>> + int ret; >>>> + >>>> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); >>>> + if (ret) >>>> + return ret; >>> >>> This check can be omitted. Up to you. >> >> This check I would prefer to keep, otherwise I have a feeling that under >> ENOMEM the following code would make an attempt to insert the controls >> data over an NULL pointer... >> >> I'll mock it to check it explicitly though, but I'm inclined to keep it. > > The control framework will handle that internally. > Right, I've tested a mocked version before sending v4, and it worked expectedly with no issues, the check for a return value has been removed. >> >> <snip> >> >>>> + >>>> + ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT, >>>> + OV6211_MODE_STREAMING, NULL); >>>> + if (ret) { >>>> + dev_err(ov6211->dev, "failed to start streaming: %d\n", ret); >>>> + goto error; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> +error: >>>> + pm_runtime_mark_last_busy(ov6211->dev); >>> >>> Please omit direct calls to pm_runtime_mark_last_busy(); this is now called >>> via the runtime PM put autosuspend etc. functions. >>> >> >> Ack. >> >> <snip> >> >>>> + if (bus_cfg.nr_of_link_frequencies != 1 || >>>> + bus_cfg.link_frequencies[0] != link_freq_menu_items[0]) { >>> >>> Could you use v4l2_link_freq_to_bitmap()? I think it'd simplify the >>> function and possibly error handling, too. >>> >> >> Ack, let it be so. >> >>>> + dev_err(ov6211->dev, "Unsupported MIPI CSI2 link frequency\n"); >>>> + ret = -EINVAL; >>>> + goto error; >>>> + } >>>> + >>>> +error: >>>> + v4l2_fwnode_endpoint_free(&bus_cfg); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int ov6211_power_on(struct device *dev) >>>> +{ >>>> + struct v4l2_subdev *sd = dev_get_drvdata(dev); >>>> + struct ov6211 *ov6211 = to_ov6211(sd); >>>> + int ret; >>>> + >>>> + if (ov6211->avdd) { >>>> + ret = regulator_enable(ov6211->avdd); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + if (ov6211->dovdd) { >>>> + ret = regulator_enable(ov6211->dovdd); >>>> + if (ret) >>>> + goto avdd_disable; >>>> + } >>>> + >>>> + if (ov6211->dvdd) { >>>> + ret = regulator_enable(ov6211->dvdd); >>>> + if (ret) >>>> + goto dovdd_disable; >>>> + } >>> >>> Can you use the regulator bulk functions? Or if the order is required, add >>> a comment about it? Otherwise someone will provide a "fix" soon afterwards. >>> :-) >> >> I've already responded to a similar review comment from Krzysztof on v1, >> here the rationale is to follow a deliberately selected model of having >> some of regulators as optional. Moreover the "digital core" DVDD regulator >> is truly optional, there is a working sensor configuration to omit it. >> >> Unfortunately so far there is no bulk regulator helper, which deals properly >> with optional regulators, so I would prefer to keep it registered this way. > > Won't the regulator framework provide you a dummy regulator if a given > regulator isn't assigned to the device in DT? > > E.g. the CCS driver unconditionally requests regulators and fails on error. > It still works on ACPI platforms without any regulators (as in terms of the > regulator framework). > You are right here, bulk version of regulator API falls back to a dummy regulator usage, I've tested that it works totally fine, and therefore I'll change to it in the next version of the driver. >>>> + gpiod_set_value_cansleep(ov6211->reset_gpio, 0); >>>> + usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC); >>>> + >>>> + ret = clk_prepare_enable(ov6211->xvclk); >>> >>> Is the clock really supposed to be enabled here, and not e.g. before >>> lifting reset? >>> >> >> The so called "gated" mode of XVCLK clock says that is should be enabled >> after releasing XSHUTDOWN pin, so I don't see a mistake here. >> >> Please note, it's basically a common part with other OmniVision sensors, >> for instance you may get a reference from OG01A1B product specification etc. >> >> Thank you for review. Shortly I'll send another new but very similar >> image sensor driver acknowledging your given comments. > -- Best wishes, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-08-22 8:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-12 21:30 [PATCH v3 0/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy 2025-08-12 21:30 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add OmniVision OV6211 image sensor Vladimir Zapolskiy 2025-08-12 21:30 ` [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver Vladimir Zapolskiy 2025-08-13 6:25 ` Tarang Raval 2025-08-18 12:08 ` Sakari Ailus 2025-08-19 22:57 ` Vladimir Zapolskiy 2025-08-21 9:18 ` Sakari Ailus 2025-08-22 8:22 ` Vladimir Zapolskiy
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).