Re: [PATCH 2/2] media: i2c: Add driver for Sony IMX219 sensor

Andrey Konovalov <andrey.konovalov@xxxxxxxxxx> · Thu, 19 Dec 2019 01:29:36 +0300

Hi Ezequiel,

Thank you for reviewing the patch!

On 14.12.2019 08:52, Ezequiel Garcia wrote:
Hi Andrey, Dave:

I'm really happy to see this. Thanks for your upstreaming effort :)
I mostly have minor comments, the driver looks quite good.

On Wed, 2019-12-11 at 14:54 +0300, Andrey Konovalov wrote:
From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>

Adds a driver for the 8MPix Sony IMX219 CSI2 sensor.
Whilst the sensor supports 2 or 4 CSI2 data lanes, this driver
currently only supports 2 lanes.
8MPix @ 15fps, 1080P @ 30fps (cropped FOV), and 1640x1232 (2x2 binned)
@ 30fps are currently supported.

Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
Signed-off-by: Andrey Konovalov <andrey.konovalov@xxxxxxxxxx>
---
  drivers/media/i2c/Kconfig  |   12 +
  drivers/media/i2c/Makefile |    1 +
  drivers/media/i2c/imx219.c | 1248 ++++++++++++++++++++++++++++++++++++
  3 files changed, 1261 insertions(+)
  create mode 100644 drivers/media/i2c/imx219.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c68e002d26ea..6fa5af1f72b9 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -591,6 +591,18 @@ config VIDEO_IMX214
  	  To compile this driver as a module, choose M here: the
  	  module will be called imx214.
  
+config VIDEO_IMX219
+	tristate "Sony IMX219 sensor support"
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	depends on MEDIA_CAMERA_SUPPORT
+	select V4L2_FWNODE
+	help
+	  This is a Video4Linux2 sensor driver for the Sony
+	  IMX219 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called imx219.
+
  config VIDEO_IMX258
  	tristate "Sony IMX258 sensor support"
  	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c147bb9d28db..77bf7d0b691f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -111,6 +111,7 @@ obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
  obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
  obj-$(CONFIG_VIDEO_HI556)	+= hi556.o
  obj-$(CONFIG_VIDEO_IMX214)	+= imx214.o
+obj-$(CONFIG_VIDEO_IMX219)	+= imx219.o
  obj-$(CONFIG_VIDEO_IMX258)	+= imx258.o
  obj-$(CONFIG_VIDEO_IMX274)	+= imx274.o
  obj-$(CONFIG_VIDEO_IMX290)	+= imx290.o
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
new file mode 100644
index 000000000000..a76e071279f2
--- /dev/null
+++ b/drivers/media/i2c/imx219.c
@@ -0,0 +1,1248 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A V4L2 driver for Sony IMX219 cameras.
+ * Copyright (C) 2019, Raspberry Pi (Trading) Ltd
+ *
+ * Based on Sony imx258 camera driver
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * DT / fwnode changes, and regulator / GPIO control taken from imx214 driver
+ * Copyright 2018 Qtechnology A/S
+ *
+ * Flip handling taken from the Sony IMX319 driver.
+ * Copyright (C) 2018 Intel Corporation
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.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-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mediabus.h>
+#include <asm/unaligned.h>
+
+#define IMX219_REG_VALUE_08BIT		1
+#define IMX219_REG_VALUE_16BIT		2
+
+#define IMX219_REG_MODE_SELECT		0x0100
+#define IMX219_MODE_STANDBY		0x00
+#define IMX219_MODE_STREAMING		0x01
+
+/* Chip ID */
+#define IMX219_REG_CHIP_ID		0x0000
+#define IMX219_CHIP_ID			0x0219
+
+/* pixel rate is fixed at 182.4M for all the modes */
+#define IMX219_PIXEL_RATE		182400000
+
+/* V_TIMING internal */
+#define IMX219_REG_VTS			0x0160
+#define IMX219_VTS_15FPS		0x0dc6
+#define IMX219_VTS_30FPS_1080P		0x06e3
+#define IMX219_VTS_30FPS_BINNED		0x06e3
+#define IMX219_VTS_MAX			0xffff
+
+#define IMX219_VBLANK_MIN		4
+
+/*Frame Length Line*/
+#define IMX219_FLL_MIN			0x08a6
+#define IMX219_FLL_MAX			0xffff
+#define IMX219_FLL_STEP			1
+#define IMX219_FLL_DEFAULT		0x0c98
+
+/* HBLANK control - read only */
+#define IMX219_PPL_DEFAULT		3448
+
+/* Exposure control */
+#define IMX219_REG_EXPOSURE		0x015a
+#define IMX219_EXPOSURE_MIN		4
+#define IMX219_EXPOSURE_STEP		1
+#define IMX219_EXPOSURE_DEFAULT		0x640
+#define IMX219_EXPOSURE_MAX		65535
+
+/* Analog gain control */
+#define IMX219_REG_ANALOG_GAIN		0x0157
+#define IMX219_ANA_GAIN_MIN		0
+#define IMX219_ANA_GAIN_MAX		232
+#define IMX219_ANA_GAIN_STEP		1
+#define IMX219_ANA_GAIN_DEFAULT		0x0
+
+/* Digital gain control */
+#define IMX219_REG_DIGITAL_GAIN		0x0158
+#define IMX219_DGTL_GAIN_MIN		0x0100
+#define IMX219_DGTL_GAIN_MAX		0x0fff
+#define IMX219_DGTL_GAIN_DEFAULT	0x0100
+#define IMX219_DGTL_GAIN_STEP		1
+
+#define IMX219_REG_ORIENTATION		0x0172
+
+/* Test Pattern Control */
+#define IMX219_REG_TEST_PATTERN		0x0600
+#define IMX219_TEST_PATTERN_DISABLE	0
+#define IMX219_TEST_PATTERN_SOLID_COLOR	1
+#define IMX219_TEST_PATTERN_COLOR_BARS	2
+#define IMX219_TEST_PATTERN_GREY_COLOR	3
+#define IMX219_TEST_PATTERN_PN9		4
+
+/* Test pattern colour components */
+#define IMX219_REG_TESTP_RED		0x0602
+#define IMX219_REG_TESTP_GREENR		0x0604
+#define IMX219_REG_TESTP_BLUE		0x0606
+#define IMX219_REG_TESTP_GREENB		0x0608
+#define IMX219_TESTP_COLOUR_MIN		0
+#define IMX219_TESTP_COLOUR_MAX		0x03ff
+#define IMX219_TESTP_COLOUR_STEP	1
+#define IMX219_TESTP_RED_DEFAULT	IMX219_TESTP_COLOUR_MAX
+#define IMX219_TESTP_GREENR_DEFAULT	0
+#define IMX219_TESTP_BLUE_DEFAULT	0
+#define IMX219_TESTP_GREENB_DEFAULT	0
+
+struct imx219_reg {
+	u16 address;
+	u8 val;
+};
+
+struct imx219_reg_list {
+	u32 num_of_regs;

s/u32/unsigned int? unless there is a need
for this to have a fixed width.

Will fix in v2. There is no need to use u32 here.

Same goes for other u32 variables.

The v4l2_ctrl_new_*() functions take s32 or u32 as their integer arguments,
and those u32 variables are used to pass the args.
Also there are drivers (e.g. imx319.c) which use u32 quite a lot.

But OK. I can change most of the u32 and s32 to unsigned int and int.
Will do that in v2 of the patch.

+	const struct imx219_reg *regs;
+};
+
+/* Mode : resolution and related config&values */
+struct imx219_mode {
+	/* Frame width */

This is probably a too much of a nitpick: the driver is full of these
type of comments "Frame width", for a width field.

Now, I really don't want to get dragged into a commenting-anti-pattern
debate (the internet's trucks are already filled with those),
yet I can't resist
mentioning that I see an excess of comments
for stuff whose meaning is pretty obvious and self-explained.

This looks excessive indeed, but I would prefer to keep these particular "Frame width" and
"Frame height" comments. Though this is quite obvious from the context the width/height fields
are used in, but there are few other widths and heights in the driver so..

Same goes for functions whose name is self-explanatory,
where no comment is needed:

     /* Check module identity */
     ret = imx219_identify_module(imx219);

     /* Initialize subdev */
     v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);

... and others similar.

Agreed. I'll remove these and the similar ones in v2.

I guess my view is aligned with that of the coding style doc:

"""
Comments are good, but there is also a danger of over-commenting.  NEVER
try to explain HOW your code works in a comment: it's much better to
write the code so that the **working** is obvious, and it's a waste of
time to explain badly written code.
"""

I fully agree. Just I prefer to overcomment the data structures than to undercomment them.

+	u32 width;
+	/* Frame height */
+	u32 height;
+
+	/* V-timing */
+	u32 vts_def;
+
+	/* Default register values */
+	struct imx219_reg_list reg_list;
+};
+
+/*
+ * Register sets lifted off the i2C interface from the Raspberry Pi firmware
+ * driver.
+ * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
+ */
+static const struct imx219_reg mode_3280x2464_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x0c},
+	{0x30eb, 0x05},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x00},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0164, 0x00},
+	{0x0165, 0x00},
+	{0x0166, 0x0c},
+	{0x0167, 0xcf},
+	{0x0168, 0x00},
+	{0x0169, 0x00},
+	{0x016a, 0x09},
+	{0x016b, 0x9f},
+	{0x016c, 0x0c},
+	{0x016d, 0xd0},
+	{0x016e, 0x09},
+	{0x016f, 0xa0},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0174, 0x00},
+	{0x0175, 0x00},
+	{0x018c, 0x0a},
+	{0x018d, 0x0a},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x0309, 0x0a},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x0c},
+	{0x0625, 0xd0},
+	{0x0626, 0x09},
+	{0x0627, 0xa0},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+

I bet this extra line has a meaning. Perhaps you can add a comment here.

Well,

+	{0x0172, 0x00},

- this sets IMX219_REG_ORIENTATION to defaut (no horisontal or vertical flips).

+	{0x0162, 0x0d},
+	{0x0163, 0x78},

0x0d78 equals to IMX219_PPL_DEFAULT (pixels per line, right?), and this is the fixed value
in this driver (even though the frame width is not the same in different modes; there is a comment
on that in imx219_set_pad_format()).

I don't know if setting these two parameters last in the row has a special meaning.

Maybe Dave can add something here,
but otherwise I am OK with either having this extra line, or deleting it.

+};
+
+static const struct imx219_reg mode_1920_1080_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x05},
+	{0x30eb, 0x0c},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x00},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+	{0x0164, 0x02},
+	{0x0165, 0xa8},
+	{0x0166, 0x0a},
+	{0x0167, 0x27},
+	{0x0168, 0x02},
+	{0x0169, 0xb4},
+	{0x016a, 0x06},
+	{0x016b, 0xeb},
+	{0x016c, 0x07},
+	{0x016d, 0x80},
+	{0x016e, 0x04},
+	{0x016f, 0x38},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0174, 0x00},
+	{0x0175, 0x00},
+	{0x018c, 0x0a},
+	{0x018d, 0x0a},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x0309, 0x0a},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x07},
+	{0x0625, 0x80},
+	{0x0626, 0x04},
+	{0x0627, 0x38},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+

Ditto.

+	{0x0172, 0x00},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+};
+
+static const struct imx219_reg mode_1640_1232_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x0c},
+	{0x30eb, 0x05},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x00},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0164, 0x00},
+	{0x0165, 0x00},
+	{0x0166, 0x0c},
+	{0x0167, 0xcf},
+	{0x0168, 0x00},
+	{0x0169, 0x00},
+	{0x016a, 0x09},
+	{0x016b, 0x9f},
+	{0x016c, 0x06},
+	{0x016d, 0x68},
+	{0x016e, 0x04},
+	{0x016f, 0xd0},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0174, 0x01},
+	{0x0175, 0x01},
+	{0x018c, 0x0a},
+	{0x018d, 0x0a},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x0309, 0x0a},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x06},
+	{0x0625, 0x68},
+	{0x0626, 0x04},
+	{0x0627, 0xd0},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+
+	{0x0172, 0x00},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+};
+
+static const char * const imx219_test_pattern_menu[] = {
+	"Disabled",
+	"Color Bars",
+	"Solid Color",
+	"Grey Color Bars",
+	"PN9"
+};
+
+static const int imx219_test_pattern_val[] = {
+	IMX219_TEST_PATTERN_DISABLE,
+	IMX219_TEST_PATTERN_COLOR_BARS,
+	IMX219_TEST_PATTERN_SOLID_COLOR,
+	IMX219_TEST_PATTERN_GREY_COLOR,
+	IMX219_TEST_PATTERN_PN9,
+};
+
+/* regulator supplies */
+static const char * const imx219_supply_name[] = {
+	/* Supplies can be enabled in any order */
+	"VANA",  /* Analog (2.8V) supply */
+	"VDIG",  /* Digital Core (1.8V) supply */
+	"VDDL",  /* IF (1.2V) supply */
+};
+
+#define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
+
+#define IMX219_XCLR_DELAY_MS 10	/* Initialisation delay after XCLR low->high */
+
+/* Mode configs */
+static const struct imx219_mode supported_modes[] = {
+	{
+		/* 8MPix 15fps mode */
+		.width = 3280,
+		.height = 2464,
+		.vts_def = IMX219_VTS_15FPS,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
+			.regs = mode_3280x2464_regs,
+		},
+	},
+	{
+		/* 1080P 30fps cropped */
+		.width = 1920,
+		.height = 1080,
+		.vts_def = IMX219_VTS_30FPS_1080P,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920_1080_regs),
+			.regs = mode_1920_1080_regs,
+		},
+	},
+	{
+		/* 2x2 binned 30fps mode */
+		.width = 1640,
+		.height = 1232,
+		.vts_def = IMX219_VTS_30FPS_BINNED,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1640_1232_regs),
+			.regs = mode_1640_1232_regs,
+		},
+	},
+};
+
+struct imx219 {
+	struct device *dev;
+
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+
+	struct v4l2_fwnode_endpoint ep; /* the parsed DT endpoint info */
+	struct clk *xclk; /* system clock to IMX219 */
+	u32 xclk_freq;
+
+	struct gpio_desc *xclr_gpio;
+	struct regulator_bulk_data supplies[IMX219_NUM_SUPPLIES];
+
+	struct v4l2_ctrl_handler ctrl_handler;
+	/* V4L2 Controls */
+	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *vflip;
+	struct v4l2_ctrl *hflip;
+	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *hblank;
+
+	/* Current mode */
+	const struct imx219_mode *mode;
+
+	/*
+	 * Mutex for serialized access:
+	 * Protect sensor module set pad format and start/stop streaming safely.
+	 */
+	struct mutex mutex;
+
+	/* Streaming on/off */
+	bool streaming;
+};
+
+static inline struct imx219 *to_imx219(struct v4l2_subdev *_sd)
+{
+	return container_of(_sd, struct imx219, sd);
+}
+
+/* Read registers up to 2 at a time */
+static int imx219_read_reg(struct imx219 *imx219, u16 reg, u32 len, u32 *val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	struct i2c_msg msgs[2];
+	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
+	u8 data_buf[4] = { 0, };
+	int ret;
+
+	if (len > 4)
+		return -EINVAL;
+
+	/* Write register address */
+	msgs[0].addr = client->addr;
+	msgs[0].flags = 0;
+	msgs[0].len = ARRAY_SIZE(addr_buf);
+	msgs[0].buf = addr_buf;
+
+	/* Read data from register */
+	msgs[1].addr = client->addr;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].len = len;
+	msgs[1].buf = &data_buf[4 - len];
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret != ARRAY_SIZE(msgs))
+		return -EIO;
+
+	*val = get_unaligned_be32(data_buf);
+
+	return 0;
+}
+
+/* Write registers up to 2 at a time */
+static int imx219_write_reg(struct imx219 *imx219, u16 reg, u32 len, u32 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	u8 buf[6];
+
+	if (len > 4)
+		return -EINVAL;
+
+	put_unaligned_be16(reg, buf);
+	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
+	if (i2c_master_send(client, buf, len + 2) != len + 2)
+		return -EIO;
+
+	return 0;
+}
+
+/* Write a list of registers */
+static int imx219_write_regs(struct imx219 *imx219,
+			     const struct imx219_reg *regs, u32 len)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < len; i++) {
+		ret = imx219_write_reg(imx219, regs[i].address, 1, regs[i].val);
+		if (ret) {
+			dev_err_ratelimited(&client->dev,
+					    "Failed to write reg 0x%4.4x. error = %d\n",
+					    regs[i].address, ret);
+
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/* Get bayer order based on flip setting. */
+static u32 imx219_get_format_code(struct imx219 *imx219)
+{
+	/*
+	 * Only one bayer order is supported.
+	 * It depends on the flip settings.
+	 */
+	u32 code;
+	static const u32 codes[2][2] = {
+		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
+		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
+	}
+
+	lockdep_assert_held(&imx219->mutex);
+	code = codes[imx219->vflip->val][imx219->hflip->val];
+

Nitpick: returning codes[..]; seems straighter, and thus more readable.

Will fix in v2.

+	return code;
+}
+
+static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_mbus_framefmt *try_fmt =
+		v4l2_subdev_get_try_format(sd, fh->pad, 0);
+
+	mutex_lock(&imx219->mutex);
+
+	/* Initialize try_fmt */
+	try_fmt->width = supported_modes[0].width;
+	try_fmt->height = supported_modes[0].height;
+	try_fmt->code = imx219_get_format_code(imx219);
+	try_fmt->field = V4L2_FIELD_NONE;
+
+	mutex_unlock(&imx219->mutex);
+
+	return 0;
+}
+
+static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct imx219 *imx219 =
+		container_of(ctrl->handler, struct imx219, ctrl_handler);
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret = 0;
+

Don't think you need to assign 'ret'.

Right. Will fix in v2 of the patch.

+	if (ctrl->id == V4L2_CID_VBLANK) {
+		s64 exposure_max, exposure_def;
+
+		/* Update max exposure while meeting expected vblanking */
+		exposure_max = imx219->mode->height + ctrl->val - 4;
+		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+			exposure_max : IMX219_EXPOSURE_DEFAULT;
+		__v4l2_ctrl_modify_range(imx219->exposure,
+					 imx219->exposure->minimum,
+					 exposure_max, imx219->exposure->step,
+					 exposure_def);
+	}
+
+	/*
+	 * Applying V4L2 control value only happens
+	 * when power is up for streaming
+	 */
+	if (pm_runtime_get_if_in_use(&client->dev) == 0)
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_ANALOGUE_GAIN:
+		ret = imx219_write_reg(imx219, IMX219_REG_ANALOG_GAIN,
+				       IMX219_REG_VALUE_08BIT, ctrl->val);
+		break;
+	case V4L2_CID_EXPOSURE:
+		ret = imx219_write_reg(imx219, IMX219_REG_EXPOSURE,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_DIGITAL_GAIN:
+		ret = imx219_write_reg(imx219, IMX219_REG_DIGITAL_GAIN,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = imx219_write_reg(imx219, IMX219_REG_TEST_PATTERN,
+				       IMX219_REG_VALUE_16BIT,
+				       imx219_test_pattern_val[ctrl->val]);
+		break;
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+		ret = imx219_write_reg(imx219, IMX219_REG_ORIENTATION, 1,
+				       imx219->hflip->val |
+				       imx219->vflip->val << 1);
+		break;
+	case V4L2_CID_VBLANK:
+		ret = imx219_write_reg(imx219, IMX219_REG_VTS,
+				       IMX219_REG_VALUE_16BIT,
+				       imx219->mode->height + ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_RED:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_RED,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_GREENR:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENR,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_BLUE:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_BLUE,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN_GREENB:
+		ret = imx219_write_reg(imx219, IMX219_REG_TESTP_GREENB,
+				       IMX219_REG_VALUE_16BIT, ctrl->val);
+		break;
+	default:
+		dev_info(&client->dev,
+			 "ctrl(id:0x%x,val:0x%x) is not handled\n",
+			 ctrl->id, ctrl->val);
+		ret = -EINVAL;
+		break;
+	}
+
+	pm_runtime_put(&client->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
+	.s_ctrl = imx219_set_ctrl,
+};
+
+static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+
+	/* Only one bayer order(GRBG) is supported */
+	if (code->index > 0)
+		return -EINVAL;
+
+	code->code = imx219_get_format_code(imx219);
+
+	return 0;
+}
+
+static int imx219_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+
+	if (fse->index >= ARRAY_SIZE(supported_modes))
+		return -EINVAL;
+
+	if (fse->code != imx219_get_format_code(imx219))
+		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 void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							  fmt->colorspace,
+							  fmt->ycbcr_enc);
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+}
+
+static void imx219_update_pad_format(struct imx219 *imx219,
+				     const struct imx219_mode *mode,
+				     struct v4l2_subdev_format *fmt)
+{
+	fmt->format.width = mode->width;
+	fmt->format.height = mode->height;
+	fmt->format.code = imx219_get_format_code(imx219);
+	fmt->format.field = V4L2_FIELD_NONE;
+
+	imx219_reset_colorspace(&fmt->format);
+}
+
+static int __imx219_get_pad_format(struct imx219 *imx219,
+				   struct v4l2_subdev_pad_config *cfg,
+				   struct v4l2_subdev_format *fmt)
+{
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		fmt->format = *v4l2_subdev_get_try_format(&imx219->sd, cfg,
+							  fmt->pad);
+	else
+		imx219_update_pad_format(imx219, imx219->mode, fmt);
+
+	return 0;
+}
+
+static int imx219_get_pad_format(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	int ret;
+
+	mutex_lock(&imx219->mutex);
+	ret = __imx219_get_pad_format(imx219, cfg, fmt);
+	mutex_unlock(&imx219->mutex);
+
+	return ret;
+}
+
+static int imx219_set_pad_format(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_format *fmt)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	const struct imx219_mode *mode;
+	struct v4l2_mbus_framefmt *framefmt;
+	s64 exposure_max, exposure_def, hblank;
+
+	mutex_lock(&imx219->mutex);
+
+	/* Bayer order varies with flips */
+	fmt->format.code = imx219_get_format_code(imx219);
+
+	mode = v4l2_find_nearest_size(supported_modes,
+				      ARRAY_SIZE(supported_modes),
+				      width, height,
+				      fmt->format.width, fmt->format.height);
+	imx219_update_pad_format(imx219, mode, fmt);
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+		*framefmt = fmt->format;
+	} else if (imx219->mode != mode) {
+		imx219->mode = mode;
+		/* Update limits and set FPS to default */
+		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
+					 IMX219_VTS_MAX - mode->height, 1,
+					 mode->vts_def - mode->height);
+		__v4l2_ctrl_s_ctrl(imx219->vblank,
+				   mode->vts_def - mode->height);
+		/* Update max exposure while meeting expected vblanking */
+		exposure_max = mode->vts_def - 4;
+		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+			exposure_max : IMX219_EXPOSURE_DEFAULT;
+		__v4l2_ctrl_modify_range(imx219->exposure,
+					 imx219->exposure->minimum,
+					 exposure_max, imx219->exposure->step,
+					 exposure_def);
+		/*
+		 * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank
+		 * depends on mode->width only, and is not changeble in any
+		 * way other than changing the mode.
+		 */
+		hblank = IMX219_PPL_DEFAULT - mode->width;
+		__v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
+					 hblank);
+	}
+
+	mutex_unlock(&imx219->mutex);
+
+	return 0;
+}
+
+/* Start streaming */
+static int imx219_start_streaming(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	const struct imx219_reg_list *reg_list;
+	int ret;
+
+	/* Apply default values of current mode */
+	reg_list = &imx219->mode->reg_list;
+	ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to set mode\n", __func__);
+		return ret;
+	}
+
+	/* Apply customized values from user */
+	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
+	if (ret)
+		return ret;
+
+	/* set stream on register */
+	return imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+				IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
+}
+
+/* Stop streaming */
+static int imx219_stop_streaming(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+
+	/* set stream off register */
+	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
+	if (ret)
+		dev_err(&client->dev, "%s failed to set stream\n", __func__);
+
+	/*
+	 * Return success even if it was an error, as there is nothing the
+	 * caller can do about it.
+	 */

Just change this function return to void, instead?

Maybe something like that (functionally the same, but probably more self-explaining):

-----8<-----
@@ -798,11 +796,7 @@ static int imx219_stop_streaming(struct imx219 *imx219)
        if (ret)
                dev_err(&client->dev, "%s failed to set stream\n", __func__);

-       /*
-        * Return success even if it was an error, as there is nothing the
-        * caller can do about it.
-        */
-       return 0;
+       return ret;
 }

 static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
@@ -832,7 +826,7 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
                if (ret)
                        goto err_rpm_put;
        } else {
-               imx219_stop_streaming(imx219);
+               (void)imx219_stop_streaming(imx219);
                pm_runtime_put(&client->dev);
        }
-----8<-----

?

+	return 0;
+}
+
+static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	mutex_lock(&imx219->mutex);
+	if (imx219->streaming == enable) {
+		mutex_unlock(&imx219->mutex);
+		return 0;
+	}
+
+	if (enable) {
+		ret = pm_runtime_get_sync(&client->dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(&client->dev);
+			goto err_unlock;
+		}
+
+		/*
+		 * Apply default & customized values
+		 * and then start streaming.
+		 */
+		ret = imx219_start_streaming(imx219);
+		if (ret)
+			goto err_rpm_put;
+	} else {
+		imx219_stop_streaming(imx219);
+		pm_runtime_put(&client->dev);
+	}
+
+	imx219->streaming = enable;
+
+	/* vflip and hflip cannot change during streaming */
+	__v4l2_ctrl_grab(imx219->vflip, enable);
+	__v4l2_ctrl_grab(imx219->hflip, enable);
+
+	mutex_unlock(&imx219->mutex);
+
+	return ret;
+
+err_rpm_put:
+	pm_runtime_put(&client->dev);
+err_unlock:
+	mutex_unlock(&imx219->mutex);
+
+	return ret;
+}
+
+/* Power/clock management functions */
+static int imx219_power_on(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
+				    imx219->supplies);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable regulators\n",
+			__func__);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(imx219->xclk);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable clock\n",
+			__func__);
+		goto reg_off;
+	}
+
+	gpiod_set_value_cansleep(imx219->xclr_gpio, 1);
+	msleep(IMX219_XCLR_DELAY_MS);
+
+	return 0;
+reg_off:
+	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
+	return ret;
+}
+
+static int imx219_power_off(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+
+	gpiod_set_value_cansleep(imx219->xclr_gpio, 0);
+	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
+	clk_disable_unprepare(imx219->xclk);
+
+	return 0;
+}
+
+static int __maybe_unused imx219_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+
+	if (imx219->streaming)
+		imx219_stop_streaming(imx219);
+
+	return 0;
+}
+
+static int __maybe_unused imx219_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+	int ret;
+
+	if (imx219->streaming) {
+		ret = imx219_start_streaming(imx219);
+		if (ret)
+			goto error;
+	}
+
+	return 0;
+
+error:
+	imx219_stop_streaming(imx219);
+	imx219->streaming = 0;
+	return ret;
+}
+
+static int imx219_get_regulators(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int i;
+
+	for (i = 0; i < IMX219_NUM_SUPPLIES; i++)
+		imx219->supplies[i].supply = imx219_supply_name[i];
+
+	return devm_regulator_bulk_get(&client->dev,
+				       IMX219_NUM_SUPPLIES,
+				       imx219->supplies);
+}
+
+/* Verify chip ID */
+static int imx219_identify_module(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	int ret;
+	u32 val;
+
+	ret = imx219_power_on(imx219->dev);
+	if (ret)
+		return ret;
+
+	ret = imx219_read_reg(imx219, IMX219_REG_CHIP_ID,
+			      IMX219_REG_VALUE_16BIT, &val);
+	if (ret) {
+		dev_err(&client->dev, "failed to read chip id %x\n",
+			IMX219_CHIP_ID);
+		goto power_off;
+	}
+
+	if (val != IMX219_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+			IMX219_CHIP_ID, val);
+		ret = -EIO;
+	}
+

I wonder if this is not a bit obscure: it's not obvious
from a first read that the device is left powered on successful
identification.

Perhaps you can have:

     return 0;

And then goto err_power_off on the error paths.
This way, it's clear that powering off is only
to be done on error.

OK. Makes sense. Will fix in v2.

OTOH, why do we need to leave the device powered on probe?

Let me try what happens if I leave powering the sensor on and off
to dev_pm_ops. (Seems like it *should* work in theory, but maybe
there were some hidden problems.)
(Also I would only be able to check if the sensor works or not - can't
do the electrical signals or power consumption measurements etc.)

Anyway, leaving the sensor powered on shouldn't hurt, as the sensor
is kept in lower power mode when it is not streaming (MIPI signals
are passive - the indication on that is the "clock-noncontinuous"
property in the DT bindings; also there is no code in the driver
to change that, then the imx219 sensor must always "turn off"
MIPI lines when it is not streaming - with the reg setting currently
used at least).

+power_off:

(and with the above, s/power_off/err_power_off)

Yes, OK.

+	if (ret)
+		imx219_power_off(imx219->dev);
+	return ret;
+}
+
+static const struct v4l2_subdev_core_ops imx219_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_video_ops imx219_video_ops = {
+	.s_stream = imx219_set_stream,
+};
+
+static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
+	.enum_mbus_code = imx219_enum_mbus_code,
+	.get_fmt = imx219_get_pad_format,
+	.set_fmt = imx219_set_pad_format,
+	.enum_frame_size = imx219_enum_frame_size,
+};
+
+static const struct v4l2_subdev_ops imx219_subdev_ops = {
+	.core = &imx219_core_ops,
+	.video = &imx219_video_ops,
+	.pad = &imx219_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
+	.open = imx219_open,
+};
+
+/* Initialize control handlers */
+static int imx219_init_controls(struct imx219 *imx219)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	struct v4l2_ctrl_handler *ctrl_hdlr;
+	u32 height = imx219->mode->height;
+	s64 hblank, exposure_max, exposure_def;
+	int i, ret;
+
+	ctrl_hdlr = &imx219->ctrl_handler;
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
+	if (ret)
+		return ret;
+
+	mutex_init(&imx219->mutex);
+	ctrl_hdlr->lock = &imx219->mutex;
+
+	/* By default, PIXEL_RATE is read only */
+	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       IMX219_PIXEL_RATE,
+					       IMX219_PIXEL_RATE, 1,
+					       IMX219_PIXEL_RATE);
+
+	/* Initial vblank/hblank/exposure parameters based on current mode */
+	imx219->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					   V4L2_CID_VBLANK, IMX219_VBLANK_MIN,
+					   IMX219_VTS_MAX - height, 1,
+					   imx219->mode->vts_def - height);
+	hblank = IMX219_PPL_DEFAULT - imx219->mode->width;
+	imx219->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					   V4L2_CID_HBLANK, hblank, hblank,
+					   1, hblank);
+	if (imx219->hblank)
+		imx219->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;

Certainly not part of this patch, but I wonder if we shouldn't
have something like v4l2_ctrl_new_std_with_flags.

+	exposure_max = imx219->mode->vts_def - 4;
+	exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
+		exposure_max : IMX219_EXPOSURE_DEFAULT;
+	imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					     V4L2_CID_EXPOSURE,
+					     IMX219_EXPOSURE_MIN, exposure_max,
+					     IMX219_EXPOSURE_STEP,
+					     exposure_def);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
+			  IMX219_ANA_GAIN_MIN, IMX219_ANA_GAIN_MAX,
+			  IMX219_ANA_GAIN_STEP, IMX219_ANA_GAIN_DEFAULT);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+			  IMX219_DGTL_GAIN_MIN, IMX219_DGTL_GAIN_MAX,
+			  IMX219_DGTL_GAIN_STEP, IMX219_DGTL_GAIN_DEFAULT);
+
+	imx219->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (imx219->hflip)
+		imx219->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	imx219->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (imx219->vflip)
+		imx219->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx219_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx219_test_pattern_menu) - 1,
+				     0, 0, imx219_test_pattern_menu);
+	for (i = 0; i < 4; i++) {
+		/*
+		 * The assumption is that
+		 * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
+		 * V4L2_CID_TEST_PATTERN_BLUE   == V4L2_CID_TEST_PATTERN_RED + 2
+		 * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
+		 */
+		v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
+				  V4L2_CID_TEST_PATTERN_RED + i,
+				  IMX219_TESTP_COLOUR_MIN,
+				  IMX219_TESTP_COLOUR_MAX,
+				  IMX219_TESTP_COLOUR_STEP,
+				  IMX219_TESTP_COLOUR_MAX);
+		/* The "Solid color" pattern is white by default */
+	}
+
+	if (ctrl_hdlr->error) {
+		ret = ctrl_hdlr->error;
+		dev_err(&client->dev, "%s control init failed (%d)\n",
+			__func__, ret);
+		goto error;
+	}
+
+	imx219->sd.ctrl_handler = ctrl_hdlr;
+
+	return 0;
+
+error:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+	mutex_destroy(&imx219->mutex);
+
+	return ret;
+}
+
+static void imx219_free_controls(struct imx219 *imx219)
+{
+	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
+	mutex_destroy(&imx219->mutex);
+}
+
+static int imx219_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct fwnode_handle *endpoint;
+	struct imx219 *imx219;
+	int ret;
+
+	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
+	if (!imx219)
+		return -ENOMEM;
+
+	imx219->dev = dev;
+
+	/* Initialize subdev */
+	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
+
+	/* Get CSI2 bus config */
+	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
+						  NULL);
+	if (!endpoint) {
+		dev_err(dev, "endpoint node not found\n");
+		return -EINVAL;
+	}
+
+	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
+	fwnode_handle_put(endpoint);
+	if (ret) {
+		dev_err(dev, "Could not parse endpoint\n");
+		return ret;
+	}
+
+	/* Get system clock (xclk) */
+	imx219->xclk = devm_clk_get(dev, "xclk");
+	if (IS_ERR(imx219->xclk)) {
+		dev_err(dev, "failed to get xclk\n");
+		return PTR_ERR(imx219->xclk);
+	}
+
+	imx219->xclk_freq = clk_get_rate(imx219->xclk);
+	if (imx219->xclk_freq != 24000000) {

Use a macro for the value?

Will fix in v2 of the patch.

+		dev_err(dev, "xclk frequency not supported: %d Hz\n",
+			imx219->xclk_freq);
+		return -EINVAL;
+	}
+
+	ret = imx219_get_regulators(imx219);
+	if (ret)
+		return ret;
+
+	/* request optional power down pin */
+	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",

This could be named powerdown instead of xclr, and it'd
be self-explanatory.

"xclr" is most probably the sensor pin name. Then I would better keep it
as is - not to confuse those who can read the datasheet.

+						    GPIOD_OUT_HIGH);
+
+	/* Check module identity */
+	ret = imx219_identify_module(imx219);
+	if (ret)
+		return ret;
+

As I mentioned above, why do we leave the device powered?
If that is really needed, I'd put a comment about it here.

Right. Answered above (will check if it works OK w/o leaving
the sensor powered).

+	/* Set default mode to max resolution */
+	imx219->mode = &supported_modes[0];
+
+	ret = imx219_init_controls(imx219);
+	if (ret)
+		return ret;
+
+	/* Initialize subdev */
+	imx219->sd.internal_ops = &imx219_internal_ops;
+	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	/* Initialize source pad */
+	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
+
+	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
+	if (ret)
+		goto error_handler_free;
+
+	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
+	if (ret < 0)
+		goto error_media_entity;
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_idle(dev);
+
+	return 0;
+
+error_media_entity:
+	media_entity_cleanup(&imx219->sd.entity);
+
+error_handler_free:
+	imx219_free_controls(imx219);
+
+	return ret;
+}
+
+static int imx219_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx219 *imx219 = to_imx219(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+	imx219_free_controls(imx219);
+
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
+	return 0;
+}
+
+static const struct of_device_id imx219_dt_ids[] = {
+	{ .compatible = "sony,imx219" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx219_dt_ids);
+
+static const struct dev_pm_ops imx219_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
+	SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
+};
+
+static struct i2c_driver imx219_i2c_driver = {
+	.driver = {
+		.name = "imx219",
+		.of_match_table	= imx219_dt_ids,
+		.pm = &imx219_pm_ops,
+	},
+	.probe = imx219_probe,
+	.remove = imx219_remove,
+};
+
+module_i2c_driver(imx219_i2c_driver);
+
+MODULE_AUTHOR("Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx");
+MODULE_DESCRIPTION("Sony IMX219 sensor driver");
+MODULE_LICENSE("GPL v2");

Thanks,
Ezequiel

Thanks,
Andrey






