[v2,2/2] media: i2c: Add driver for Sony IMX219 sensor

  From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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@raspberrypi.com>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/Kconfig  |   12 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
 3 files changed, 1253 insertions(+)
 create mode 100644 drivers/media/i2c/imx219.c
  

Hi Andrey,

On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> 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@raspberrypi.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 1253 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

You can safely drop this line.

> +	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..28b55c61cd77
> --- /dev/null
> +++ b/drivers/media/i2c/imx219.c
> @@ -0,0 +1,1240 @@
> +// 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
> +
> +/* External clock frequency is 24.0M */
> +#define IMX219_XCLK_FREQ		24000000
> +
> +/* 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 {
> +	unsigned int num_of_regs;
> +	const struct imx219_reg *regs;
> +};
> +
> +/* Mode : resolution and related config&values */
> +struct imx219_mode {
> +	/* Frame width */
> +	unsigned int width;
> +	/* Frame height */
> +	unsigned int height;
> +
> +	/* V-timing */
> +	unsigned int 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},
> +	{0x0162, 0x0d},
> +	{0x0163, 0x78},
> +};
> +
> +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},
> +	{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},
> +	{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.
> +	 */
> +	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);
> +	return codes[imx219->vflip->val][imx219->hflip->val];
> +}
> +
> +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);

Please assign this when getting the format (subdev pad get_fmt callback).

> +	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;
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		int 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 is supported (though it depends on the flip
> +	 * settings)
> +	 */
> +	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;
> +	int 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;
> +}
> +
> +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);
> +}
> +
> +static void 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__);
> +}
> +
> +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);

I guess 10 ms is ok albeit it'd be nicer if you calculated the required
delay instead.

> +
> +	return 0;

Empty line here, please.

> +reg_off:
> +	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);

And here.

> +	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;

Please add an empty line here.

> +	return ret;
> +}
> +
> +static int imx219_get_regulators(struct imx219 *imx219)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	int i;

unsigned int

> +
> +	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_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);
> +		return ret;
> +	}
> +
> +	if (val != IMX219_CHIP_ID) {
> +		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
> +			IMX219_CHIP_ID, val);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +	unsigned int height = imx219->mode->height;
> +	int exposure_max, exposure_def, hblank;
> +	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;
> +	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;

Instead of putting a dev field to struct imx219, you can find the device in
struct i2c_client.dev, which you can get by:

	v4l2_get_subdevdata(imx219->sd)

> +
> +	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 != IMX219_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			imx219->xclk_freq);
> +		return -EINVAL;
> +	}

Could you also check the link frequencies (the link-frequencies property
that also should be added to DT bindings) matches with what is possible
with the given xclk frequency? Please see e.g. imx319 driver for an
example.

> +
> +	ret = imx219_get_regulators(imx219);
> +	if (ret)
> +		return ret;
> +
> +	/* Request optional enable pin */
> +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> +						    GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for imx219_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = imx219_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx219_identify_module(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode to max resolution */
> +	imx219->mode = &supported_modes[0];
> +
> +	ret = imx219_init_controls(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* 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;
> +
> +	/* Enable runtime PM and turn off the device */
> +	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);
> +
> +error_power_off:
> +	imx219_power_off(dev);
> +
> +	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);

imx219_power_off(), if the sensor is powered on here. Please see e.g. the
smiapp driver regarding this.

> +
> +	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,

Could you use .probe_new, and remove the i2c_device_id argument?

> +	.remove = imx219_remove,
> +};
> +
> +module_i2c_driver(imx219_i2c_driver);
> +
> +MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
> +MODULE_DESCRIPTION("Sony IMX219 sensor driver");
> +MODULE_LICENSE("GPL v2");
  

On Fri, 2019-12-27 at 15:21 +0300, Andrey Konovalov wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> 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@raspberrypi.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 1253 insertions(+)
>  create mode 100644 drivers/media/i2c/imx219.c
> 
> 
[..]
> +
> +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;
> +
> +	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 != IMX219_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			imx219->xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = imx219_get_regulators(imx219);
> +	if (ret)
> +		return ret;
> +

I think printing an error if you can't regulators
(as the core will stay silent), is a good idea.

Just got bitten by this while trying the driver with
a RPI camera module v2, and the RKISP1 patches that
we recently posted on this mailing list.

(did _very_ limited testing, but working nicely so far).

> +	/* Request optional enable pin */
> +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> +						    GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for imx219_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = imx219_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx219_identify_module(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode to max resolution */
> +	imx219->mode = &supported_modes[0];
> +
> +	ret = imx219_init_controls(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* 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;
> +

Ditto on these last two, it's possible to get silent
failures here, which are a bit annoying to debug.

Thanks,
Ezequiel
  

Hi Sakari,

Sorry for delayed reply..
(your email got into a wrong folder, and I might not find it there if Ezequiel
did not warn me that I didn't address the comments from your review)

On 27.12.2019 17:55, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> 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@raspberrypi.com>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/media/i2c/Kconfig  |   12 +
>>   drivers/media/i2c/Makefile |    1 +
>>   drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 1253 insertions(+)
>>   create mode 100644 drivers/media/i2c/imx219.c

<snip>

>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
>> new file mode 100644
>> index 000000000000..28b55c61cd77
>> --- /dev/null
>> +++ b/drivers/media/i2c/imx219.c

<snip>

>> +/* 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);
> 
> I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> delay instead.

I think this 10 ms delay actually serves two purposes here. It is
not only the delay after XCLR pin is set high (reset de-asserted),
but it also lets the camera power supplies voltages to settle after
regulator_bulk_enable() called few lines above.

So I would make the delay a sum of 1) the value which depends on
input clock frequency (the driver currently supports 24MHz only)
and 2) a fixed value of 8 ms or so to account for the power supplies
settle time. So that the sum would be the same 10 ms for 24MHz input
clock.
Does it makes sense?

<snip>

>> +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;
> 
> Instead of putting a dev field to struct imx219, you can find the device in
> struct i2c_client.dev, which you can get by:
> 
> 	v4l2_get_subdevdata(imx219->sd)
> 
>> +
>> +	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 != IMX219_XCLK_FREQ) {
>> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
>> +			imx219->xclk_freq);
>> +		return -EINVAL;
>> +	}
> 
> Could you also check the link frequencies (the link-frequencies property
> that also should be added to DT bindings) matches with what is possible
> with the given xclk frequency? Please see e.g. imx319 driver for an
> example.

The driver only supports single xclk frequency and single link-frequency
(hardcoded in the registers value tables). So the check will be more like
the one in imx290 driver (error out if the link-frequency property isn't
equal to the pre#define-d default value).

>> +
>> +	ret = imx219_get_regulators(imx219);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Request optional enable pin */
>> +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
>> +						    GPIOD_OUT_HIGH);
>> +
>> +	/*
>> +	 * The sensor must be powered for imx219_identify_module()
>> +	 * to be able to read the CHIP_ID register
>> +	 */
>> +	ret = imx219_power_on(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = imx219_identify_module(imx219);
>> +	if (ret)
>> +		goto error_power_off;
>> +
>> +	/* Set default mode to max resolution */
>> +	imx219->mode = &supported_modes[0];
>> +
>> +	ret = imx219_init_controls(imx219);
>> +	if (ret)
>> +		goto error_power_off;
>> +
>> +	/* 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;
>> +
>> +	/* Enable runtime PM and turn off the device */
>> +	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);
>> +
>> +error_power_off:
>> +	imx219_power_off(dev);
>> +
>> +	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);
> 
> imx219_power_off(), if the sensor is powered on here. Please see e.g. the
> smiapp driver regarding this.

It should be powered off here.

The sensor is powered on when streaming is started, and is powered off when
it is stopped: if the enable argument is false, imx219_set_stream() calls
pm_runtime_put().
IOW, it follows the imx319 driver as the example of power management.

>> +
>> +	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,
> 
> Could you use .probe_new, and remove the i2c_device_id argument?

I'll fix this and all the other issues I didn't comment on in this email
in the v4 of the patch set.

Thanks,
Andrey

>> +	.remove = imx219_remove,
>> +};
>> +
>> +module_i2c_driver(imx219_i2c_driver);
>> +
>> +MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
>> +MODULE_DESCRIPTION("Sony IMX219 sensor driver");
>> +MODULE_LICENSE("GPL v2");
>
  

Hi Andrey & Sakari

On Mon, 13 Jan 2020 at 19:16, Andrey Konovalov
<andrey.konovalov@linaro.org> wrote:
>
> Hi Sakari,
>
> Sorry for delayed reply..
> (your email got into a wrong folder, and I might not find it there if Ezequiel
> did not warn me that I didn't address the comments from your review)

For some reason I missed getting Sakari's email too - apologies.

> On 27.12.2019 17:55, Sakari Ailus wrote:
> > Hi Andrey,
> >
> > On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
> >> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>
> >> 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@raspberrypi.com>
> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> >> ---
> >>   drivers/media/i2c/Kconfig  |   12 +
> >>   drivers/media/i2c/Makefile |    1 +
> >>   drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 1253 insertions(+)
> >>   create mode 100644 drivers/media/i2c/imx219.c
>
> <snip>
>
> >> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> >> new file mode 100644
> >> index 000000000000..28b55c61cd77
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/imx219.c
>
> <snip>
>
> >> +/* 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);
> >
> > I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> > delay instead.
>
> I think this 10 ms delay actually serves two purposes here. It is
> not only the delay after XCLR pin is set high (reset de-asserted),
> but it also lets the camera power supplies voltages to settle after
> regulator_bulk_enable() called few lines above.
>
> So I would make the delay a sum of 1) the value which depends on
> input clock frequency (the driver currently supports 24MHz only)
> and 2) a fixed value of 8 ms or so to account for the power supplies
> settle time. So that the sum would be the same 10 ms for 24MHz input
> clock.
> Does it makes sense?

Regulator settling times shouldn't really be included here - that
should be taken care of via the regulator framework (in the case of DT
for regulator-fixed you define the startup-delay-us property).

What level do you end up computing it to?

This delay covers t4, t5 and t6 from figure 38 on page 77 of the datasheet[1]
t4 is max 200usecs.
t5 is 6ms.
t6 is 32000 cycles of INCK. As you say INCK=24MHz is the only
supported clock frequency at present, so 1.3ms. Minimum clock is 6MHz
which will make this 5.3ms.

t6 is in parallel with t5, but it is smaller than t5 even at the
minimum clock frequency.
Yes we can be programming the sensor over I2C after t5 but before t6,
but you'd now want to be computing the number of I2C writes required,
the speed of the I2C bus, and probably a few other parameters to
ensure you don't violate t5. The sensor supports 1MHz CCI if INCK is
>11.4MHz. A quick check says we have around 60 register writes to
initialise the sensor. 38 clocks (4 * (8 data bits + ACK) + start +
end) to write each register, which I make 2.4ms. It is therefore
possible to violate t5.

Is it worth that level of computation, or do you just take t4+t5 at 6.2ms?

I have been a bit naughty up until now in not setting startup-delay-us
on the regulator definition and relying on this delay instead. The
driver ought to do the right thing though and I'll fix my
configuration.

  Dave

[1] https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS

> <snip>
>
> >> +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;
> >
> > Instead of putting a dev field to struct imx219, you can find the device in
> > struct i2c_client.dev, which you can get by:
> >
> >       v4l2_get_subdevdata(imx219->sd)
> >
> >> +
> >> +    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 != IMX219_XCLK_FREQ) {
> >> +            dev_err(dev, "xclk frequency not supported: %d Hz\n",
> >> +                    imx219->xclk_freq);
> >> +            return -EINVAL;
> >> +    }
> >
> > Could you also check the link frequencies (the link-frequencies property
> > that also should be added to DT bindings) matches with what is possible
> > with the given xclk frequency? Please see e.g. imx319 driver for an
> > example.
>
> The driver only supports single xclk frequency and single link-frequency
> (hardcoded in the registers value tables). So the check will be more like
> the one in imx290 driver (error out if the link-frequency property isn't
> equal to the pre#define-d default value).
>
> >> +
> >> +    ret = imx219_get_regulators(imx219);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    /* Request optional enable pin */
> >> +    imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> >> +                                                GPIOD_OUT_HIGH);
> >> +
> >> +    /*
> >> +     * The sensor must be powered for imx219_identify_module()
> >> +     * to be able to read the CHIP_ID register
> >> +     */
> >> +    ret = imx219_power_on(dev);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    ret = imx219_identify_module(imx219);
> >> +    if (ret)
> >> +            goto error_power_off;
> >> +
> >> +    /* Set default mode to max resolution */
> >> +    imx219->mode = &supported_modes[0];
> >> +
> >> +    ret = imx219_init_controls(imx219);
> >> +    if (ret)
> >> +            goto error_power_off;
> >> +
> >> +    /* 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;
> >> +
> >> +    /* Enable runtime PM and turn off the device */
> >> +    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);
> >> +
> >> +error_power_off:
> >> +    imx219_power_off(dev);
> >> +
> >> +    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);
> >
> > imx219_power_off(), if the sensor is powered on here. Please see e.g. the
> > smiapp driver regarding this.
>
> It should be powered off here.
>
> The sensor is powered on when streaming is started, and is powered off when
> it is stopped: if the enable argument is false, imx219_set_stream() calls
> pm_runtime_put().
> IOW, it follows the imx319 driver as the example of power management.
>
> >> +
> >> +    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,
> >
> > Could you use .probe_new, and remove the i2c_device_id argument?
>
> I'll fix this and all the other issues I didn't comment on in this email
> in the v4 of the patch set.
>
> Thanks,
> Andrey
>
> >> +    .remove = imx219_remove,
> >> +};
> >> +
> >> +module_i2c_driver(imx219_i2c_driver);
> >> +
> >> +MODULE_AUTHOR("Dave Stevenson <dave.stevenson@raspberrypi.com");
> >> +MODULE_DESCRIPTION("Sony IMX219 sensor driver");
> >> +MODULE_LICENSE("GPL v2");
> >
  

Hi Andrey,

On Mon, Jan 13, 2020 at 10:16:21PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> Sorry for delayed reply..
> (your email got into a wrong folder, and I might not find it there if Ezequiel
> did not warn me that I didn't address the comments from your review)

No worries.

> 
> On 27.12.2019 17:55, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > On Fri, Dec 27, 2019 at 03:21:14PM +0300, Andrey Konovalov wrote:
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > 
> > > 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@raspberrypi.com>
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > ---
> > >   drivers/media/i2c/Kconfig  |   12 +
> > >   drivers/media/i2c/Makefile |    1 +
> > >   drivers/media/i2c/imx219.c | 1240 ++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 1253 insertions(+)
> > >   create mode 100644 drivers/media/i2c/imx219.c
> 
> <snip>
> 
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > new file mode 100644
> > > index 000000000000..28b55c61cd77
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/imx219.c
> 
> <snip>
> 
> > > +/* 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);
> > 
> > I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> > delay instead.
> 
> I think this 10 ms delay actually serves two purposes here. It is
> not only the delay after XCLR pin is set high (reset de-asserted),
> but it also lets the camera power supplies voltages to settle after
> regulator_bulk_enable() called few lines above.
> 
> So I would make the delay a sum of 1) the value which depends on
> input clock frequency (the driver currently supports 24MHz only)
> and 2) a fixed value of 8 ms or so to account for the power supplies
> settle time. So that the sum would be the same 10 ms for 24MHz input
> clock.
> Does it makes sense?

It does, agreed.

> 
> <snip>
> 
> > > +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;
> > 
> > Instead of putting a dev field to struct imx219, you can find the device in
> > struct i2c_client.dev, which you can get by:
> > 
> > 	v4l2_get_subdevdata(imx219->sd)
> > 
> > > +
> > > +	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 != IMX219_XCLK_FREQ) {
> > > +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> > > +			imx219->xclk_freq);
> > > +		return -EINVAL;
> > > +	}
> > 
> > Could you also check the link frequencies (the link-frequencies property
> > that also should be added to DT bindings) matches with what is possible
> > with the given xclk frequency? Please see e.g. imx319 driver for an
> > example.
> 
> The driver only supports single xclk frequency and single link-frequency
> (hardcoded in the registers value tables). So the check will be more like
> the one in imx290 driver (error out if the link-frequency property isn't
> equal to the pre#define-d default value).

Right. The frequencies don't seem to be in DT bindings either; they should
be added there. The sensor supports multiple frequencies as in practice
every other sensor does.

The driver does not necessarily need to check for them if it supports a
single one only.

> 
> > > +
> > > +	ret = imx219_get_regulators(imx219);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Request optional enable pin */
> > > +	imx219->xclr_gpio = devm_gpiod_get_optional(dev, "xclr",
> > > +						    GPIOD_OUT_HIGH);
> > > +
> > > +	/*
> > > +	 * The sensor must be powered for imx219_identify_module()
> > > +	 * to be able to read the CHIP_ID register
> > > +	 */
> > > +	ret = imx219_power_on(dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = imx219_identify_module(imx219);
> > > +	if (ret)
> > > +		goto error_power_off;
> > > +
> > > +	/* Set default mode to max resolution */
> > > +	imx219->mode = &supported_modes[0];
> > > +
> > > +	ret = imx219_init_controls(imx219);
> > > +	if (ret)
> > > +		goto error_power_off;
> > > +
> > > +	/* 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;
> > > +
> > > +	/* Enable runtime PM and turn off the device */
> > > +	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);
> > > +
> > > +error_power_off:
> > > +	imx219_power_off(dev);
> > > +
> > > +	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);
> > 
> > imx219_power_off(), if the sensor is powered on here. Please see e.g. the
> > smiapp driver regarding this.
> 
> It should be powered off here.
> 
> The sensor is powered on when streaming is started, and is powered off when
> it is stopped: if the enable argument is false, imx219_set_stream() calls
> pm_runtime_put().
> IOW, it follows the imx319 driver as the example of power management.

Well, yes, or rather maybe.

If you have a non-ACPI system and runtime PM is disabled, then the sensor
is powered on here.

The imx319 driver only works on ACPI based systems so it's not something
you can compare to directly. Check e.g. the smiapp driver which works on
both DT and ACPI systems.

> 
> > > +
> > > +	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,
> > 
> > Could you use .probe_new, and remove the i2c_device_id argument?
> 
> I'll fix this and all the other issues I didn't comment on in this email
> in the v4 of the patch set.

Thanks!
  

Hi Dave,

On Tue, Jan 14, 2020 at 11:34:46AM +0000, Dave Stevenson wrote:

...

> > >> +    msleep(IMX219_XCLR_DELAY_MS);
> > >
> > > I guess 10 ms is ok albeit it'd be nicer if you calculated the required
> > > delay instead.
> >
> > I think this 10 ms delay actually serves two purposes here. It is
> > not only the delay after XCLR pin is set high (reset de-asserted),
> > but it also lets the camera power supplies voltages to settle after
> > regulator_bulk_enable() called few lines above.
> >
> > So I would make the delay a sum of 1) the value which depends on
> > input clock frequency (the driver currently supports 24MHz only)
> > and 2) a fixed value of 8 ms or so to account for the power supplies
> > settle time. So that the sum would be the same 10 ms for 24MHz input
> > clock.
> > Does it makes sense?
> 
> Regulator settling times shouldn't really be included here - that
> should be taken care of via the regulator framework (in the case of DT
> for regulator-fixed you define the startup-delay-us property).
> 
> What level do you end up computing it to?
> 
> This delay covers t4, t5 and t6 from figure 38 on page 77 of the datasheet[1]
> t4 is max 200usecs.
> t5 is 6ms.
> t6 is 32000 cycles of INCK. As you say INCK=24MHz is the only
> supported clock frequency at present, so 1.3ms. Minimum clock is 6MHz
> which will make this 5.3ms.

It's nicer to calculate that still. Someone may well add support for other
frequencies and it's easy to miss changing this.

> 
> t6 is in parallel with t5, but it is smaller than t5 even at the
> minimum clock frequency.
> Yes we can be programming the sensor over I2C after t5 but before t6,
> but you'd now want to be computing the number of I2C writes required,
> the speed of the I2C bus, and probably a few other parameters to
> ensure you don't violate t5. The sensor supports 1MHz CCI if INCK is
> >11.4MHz. A quick check says we have around 60 register writes to
> initialise the sensor. 38 clocks (4 * (8 data bits + ACK) + start +
> end) to write each register, which I make 2.4ms. It is therefore
> possible to violate t5.
> 
> Is it worth that level of computation, or do you just take t4+t5 at 6.2ms?

I'd just ignore the I²C part and wait for long enough. This is not *that*
much time after all. Of course, if someone feels like optimising this, why
not, but it looks like something that doesn't really pay off.

> 
> I have been a bit naughty up until now in not setting startup-delay-us
> on the regulator definition and relying on this delay instead. The
> driver ought to do the right thing though and I'll fix my
> configuration.

Agreed.