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.
Same goes for other u32 variables.
> + 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.
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.
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.
"""
> + 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.
> + {0x0172, 0x00},
> + {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},
> +
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.
> + 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'.
> + 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?
> + 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.
OTOH, why do we need to leave the device powered on probe?
> +power_off:
(and with the above, s/power_off/err_power_off)
> + 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?
> + 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.
> + 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.
> + /* 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
