[v2,2/2] media: i2c: IMX296 camera sensor driver

The IMX296LLR is a monochrome 1.60MP CMOS sensor from Sony. The driver
supports cropping and binning (but not both at the same time due to
hardware limitations) and exposure, gain, vertical blanking and test
pattern controls.

Preliminary support is also included for the color IMX296LQR sensor.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Use help instead of ---help--- in Kconfig
- Print error message on I2C write failure
- Fix number of controls in v4l2_ctrl_handler_init() call
- Replace hardcoded number with ARRAY_SIZE()
- Initialize structures at declaration time
- Use .probe_new() and remove I2C device table
- Drop of_match_ptr()
- Fix module description
- Fix test pattern configuration
- Fix power on
- Read sensor temperature at probe time
- Set opaque background in test patterns
- Initialize format field and colorspace
- Reorder subdev pad operations
- Create controls for fwnode properties
- Expose crop bounds and defaults
- Add V4L2_CID_HBLANK control
- Replace gain control with analogue gain
- Rename local variable 'imx' to 'sensor'
- Protect format and crop with a mutex
- Consistently access struct device through sensor->dev
- Use runtime PM
- Use three power supplies
- Fix number of controls when initializing handler
- Fix error message for reset GPIO get failure
- Switch to V4L2 subdev state API
- Use dev_err_probe()
---
 MAINTAINERS                |    1 +
 drivers/media/i2c/Kconfig  |   12 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/imx296.c | 1200 ++++++++++++++++++++++++++++++++++++
 4 files changed, 1214 insertions(+)
 create mode 100644 drivers/media/i2c/imx296.c

Hi Sakari,

On Tue, Dec 21, 2021 at 02:54:48PM +0200, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thanks for the patchset. A few relatively minor comments below.
> 
> On Mon, Dec 20, 2021 at 12:09:48AM +0200, Laurent Pinchart wrote:
> > The IMX296LLR is a monochrome 1.60MP CMOS sensor from Sony. The driver
> > supports cropping and binning (but not both at the same time due to
> > hardware limitations) and exposure, gain, vertical blanking and test
> > pattern controls.
> > 
> > Preliminary support is also included for the color IMX296LQR sensor.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Use help instead of ---help--- in Kconfig
> > - Print error message on I2C write failure
> > - Fix number of controls in v4l2_ctrl_handler_init() call
> > - Replace hardcoded number with ARRAY_SIZE()
> > - Initialize structures at declaration time
> > - Use .probe_new() and remove I2C device table
> > - Drop of_match_ptr()
> > - Fix module description
> > - Fix test pattern configuration
> > - Fix power on
> > - Read sensor temperature at probe time
> > - Set opaque background in test patterns
> > - Initialize format field and colorspace
> > - Reorder subdev pad operations
> > - Create controls for fwnode properties
> > - Expose crop bounds and defaults
> > - Add V4L2_CID_HBLANK control
> > - Replace gain control with analogue gain
> > - Rename local variable 'imx' to 'sensor'
> > - Protect format and crop with a mutex
> > - Consistently access struct device through sensor->dev
> > - Use runtime PM
> > - Use three power supplies
> > - Fix number of controls when initializing handler
> > - Fix error message for reset GPIO get failure
> > - Switch to V4L2 subdev state API
> > - Use dev_err_probe()
> > ---
> >  MAINTAINERS                |    1 +
> >  drivers/media/i2c/Kconfig  |   12 +
> >  drivers/media/i2c/Makefile |    1 +
> >  drivers/media/i2c/imx296.c | 1200 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1214 insertions(+)
> >  create mode 100644 drivers/media/i2c/imx296.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1b20f2b90aec..e49049e8ee36 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17759,6 +17759,7 @@ L:	linux-media@vger.kernel.org
> >  S:	Maintained
> >  T:	git git://linuxtv.org/media_tree.git
> >  F:	Documentation/devicetree/bindings/media/i2c/sony,imx296.yaml
> > +F:	drivers/media/i2c/imx296.c
> >  
> >  SONY IMX319 SENSOR DRIVER
> >  M:	Bingbu Cao <bingbu.cao@intel.com>
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index d6a5d4ca439a..8cc23019459d 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -832,6 +832,18 @@ config VIDEO_IMX290
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called imx290.
> >  
> > +config VIDEO_IMX296
> > +	tristate "Sony IMX296 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
> > +	  IMX296 camera.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called imx296.
> > +
> >  config VIDEO_IMX319
> >  	tristate "Sony IMX319 sensor support"
> >  	depends on I2C && VIDEO_V4L2
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 4d4fe08d7a6a..aba3bd7dd337 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -125,6 +125,7 @@ obj-$(CONFIG_VIDEO_IMX219)	+= imx219.o
> >  obj-$(CONFIG_VIDEO_IMX258)	+= imx258.o
> >  obj-$(CONFIG_VIDEO_IMX274)	+= imx274.o
> >  obj-$(CONFIG_VIDEO_IMX290)	+= imx290.o
> > +obj-$(CONFIG_VIDEO_IMX296)	+= imx296.o
> >  obj-$(CONFIG_VIDEO_IMX319)	+= imx319.o
> >  obj-$(CONFIG_VIDEO_IMX334)	+= imx334.o
> >  obj-$(CONFIG_VIDEO_IMX335)	+= imx335.o
> > diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c
> > new file mode 100644
> > index 000000000000..4be03180deac
> > --- /dev/null
> > +++ b/drivers/media/i2c/imx296.c
> > @@ -0,0 +1,1200 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for IMX296 CMOS Image Sensor from Sony
> > + *
> > + * Copyright 2019 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/videodev2.h>
> > +
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-fwnode.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#define IMX296_PIXEL_ARRAY_WIDTH			1456
> > +#define IMX296_PIXEL_ARRAY_HEIGHT			1088
> > +
> > +#define IMX296_REG_8BIT(n)				((1 << 16) | (n))
> > +#define IMX296_REG_16BIT(n)				((2 << 16) | (n))
> > +#define IMX296_REG_24BIT(n)				((3 << 16) | (n))
> > +#define IMX296_REG_SIZE_SHIFT				16
> > +#define IMX296_REG_ADDR_MASK				0xffff
> > +
> > +#define IMX296_CTRL00					IMX296_REG_8BIT(0x3000)
> > +#define IMX296_CTRL00_STANDBY				BIT(0)
> > +#define IMX296_CTRL08					IMX296_REG_8BIT(0x3008)
> > +#define IMX296_CTRL08_REGHOLD				BIT(0)
> > +#define IMX296_CTRL0A					IMX296_REG_8BIT(0x300a)
> > +#define IMX296_CTRL0A_XMSTA				BIT(0)
> > +#define IMX296_CTRL0B					IMX296_REG_8BIT(0x300b)
> > +#define IMX296_CTRL0B_TRIGEN				BIT(0)
> > +#define IMX296_CTRL0D					IMX296_REG_8BIT(0x300d)
> > +#define IMX296_CTRL0D_WINMODE_ALL			(0 << 0)
> > +#define IMX296_CTRL0D_WINMODE_FD_BINNING		(2 << 0)
> > +#define IMX296_CTRL0D_HADD_ON_BINNING			BIT(5)
> > +#define IMX296_CTRL0D_SAT_CNT				BIT(6)
> > +#define IMX296_CTRL0E					IMX296_REG_8BIT(0x300e)
> > +#define IMX296_CTRL0E_VREVERSE				BIT(0)
> > +#define IMX296_CTRL0E_HREVERSE				BIT(1)
> > +#define IMX296_VMAX					IMX296_REG_24BIT(0x3010)
> > +#define IMX296_HMAX					IMX296_REG_16BIT(0x3014)
> > +#define IMX296_TMDCTRL					IMX296_REG_8BIT(0x301d)
> > +#define IMX296_TMDCTRL_LATCH				BIT(0)
> > +#define IMX296_TMDOUT					IMX296_REG_16BIT(0x301e)
> > +#define IMX296_TMDOUT_MASK				0x3ff
> > +#define IMX296_WDSEL					IMX296_REG_8BIT(0x3021)
> > +#define IMX296_WDSEL_NORMAL				(0 << 0)
> > +#define IMX296_WDSEL_MULTI_2				(1 << 0)
> > +#define IMX296_WDSEL_MULTI_4				(3 << 0)
> > +#define IMX296_BLKLEVELAUTO				IMX296_REG_8BIT(0x3022)
> > +#define IMX296_BLKLEVELAUTO_ON				0x01
> > +#define IMX296_BLKLEVELAUTO_OFF				0xf0
> > +#define IMX296_SST					IMX296_REG_8BIT(0x3024)
> > +#define IMX296_SST_EN					BIT(0)
> > +#define IMX296_CTRLTOUT					IMX296_REG_8BIT(0x3026)
> > +#define IMX296_CTRLTOUT_TOUT1SEL_LOW			(0 << 0)
> > +#define IMX296_CTRLTOUT_TOUT1SEL_PULSE			(3 << 0)
> > +#define IMX296_CTRLTOUT_TOUT2SEL_LOW			(0 << 2)
> > +#define IMX296_CTRLTOUT_TOUT2SEL_PULSE			(3 << 2)
> > +#define IMX296_CTRLTRIG					IMX296_REG_8BIT(0x3029)
> > +#define IMX296_CTRLTRIG_TOUT1_SEL_LOW			(0 << 0)
> > +#define IMX296_CTRLTRIG_TOUT1_SEL_PULSE1		(1 << 0)
> > +#define IMX296_CTRLTRIG_TOUT2_SEL_LOW			(0 << 4)
> > +#define IMX296_CTRLTRIG_TOUT2_SEL_PULSE2		(2 << 4)
> > +#define IMX296_SYNCSEL					IMX296_REG_8BIT(0x3036)
> > +#define IMX296_SYNCSEL_NORMAL				0xc0
> > +#define IMX296_SYNCSEL_HIZ				0xf0
> > +#define IMX296_PULSE1					IMX296_REG_8BIT(0x306d)
> > +#define IMX296_PULSE1_EN_NOR				BIT(0)
> > +#define IMX296_PULSE1_EN_TRIG				BIT(1)
> > +#define IMX296_PULSE1_POL_HIGH				(0 << 2)
> > +#define IMX296_PULSE1_POL_LOW				(1 << 2)
> > +#define IMX296_PULSE1_UP				IMX296_REG_24BIT(0x3070)
> > +#define IMX296_PULSE1_DN				IMX296_REG_24BIT(0x3074)
> > +#define IMX296_PULSE2					IMX296_REG_8BIT(0x3079)
> > +#define IMX296_PULSE2_EN_NOR				BIT(0)
> > +#define IMX296_PULSE2_EN_TRIG				BIT(1)
> > +#define IMX296_PULSE2_POL_HIGH				(0 << 2)
> > +#define IMX296_PULSE2_POL_LOW				(1 << 2)
> > +#define IMX296_PULSE2_UP				IMX296_REG_24BIT(0x307c)
> > +#define IMX296_PULSE2_DN				IMX296_REG_24BIT(0x3080)
> > +#define IMX296_INCKSEL(n)				IMX296_REG_8BIT(0x3089 + (n))
> > +#define IMX296_SHS1					IMX296_REG_24BIT(0x308d)
> > +#define IMX296_SHS2					IMX296_REG_24BIT(0x3090)
> > +#define IMX296_SHS3					IMX296_REG_24BIT(0x3094)
> > +#define IMX296_SHS4					IMX296_REG_24BIT(0x3098)
> > +#define IMX296_VBLANKLP					IMX296_REG_8BIT(0x309c)
> > +#define IMX296_VBLANKLP_NORMAL				0x04
> > +#define IMX296_VBLANKLP_LOW_POWER			0x2c
> > +#define IMX296_EXP_CNT					IMX296_REG_8BIT(0x30a3)
> > +#define IMX296_EXP_CNT_RESET				BIT(0)
> > +#define IMX296_EXP_MAX					IMX296_REG_16BIT(0x30a6)
> > +#define IMX296_VINT					IMX296_REG_8BIT(0x30aa)
> > +#define IMX296_VINT_EN					BIT(0)
> > +#define IMX296_LOWLAGTRG				IMX296_REG_8BIT(0x30ae)
> > +#define IMX296_LOWLAGTRG_FAST				BIT(0)
> > +#define IMX296_I2CCTRL					IMX296_REG_8BIT(0x30ef)
> > +#define IMX296_I2CCTRL_I2CACKEN				BIT(0)
> > +
> > +#define IMX296_SENSOR_INFO				IMX296_REG_16BIT(0x3148)
> > +#define IMX296_SENSOR_INFO_MONO				BIT(15)
> > +#define IMX296_S_SHSA					IMX296_REG_16BIT(0x31ca)
> > +#define IMX296_S_SHSB					IMX296_REG_16BIT(0x31d2)
> > +/*
> > + * Registers 0x31c8 to 0x31cd, 0x31d0 to 0x31d5, 0x31e2, 0x31e3, 0x31ea and
> > + * 0x31eb are related to exposure mode but otherwise not documented.
> > + */
> > +
> > +#define IMX296_GAINCTRL					IMX296_REG_8BIT(0x3200)
> > +#define IMX296_GAINCTRL_WD_GAIN_MODE_NORMAL		0x01
> > +#define IMX296_GAINCTRL_WD_GAIN_MODE_MULTI		0x41
> > +#define IMX296_GAIN					IMX296_REG_16BIT(0x3204)
> > +#define IMX296_GAIN_MIN					0
> > +#define IMX296_GAIN_MAX					480
> > +#define IMX296_GAIN1					IMX296_REG_16BIT(0x3208)
> > +#define IMX296_GAIN2					IMX296_REG_16BIT(0x320c)
> > +#define IMX296_GAIN3					IMX296_REG_16BIT(0x3210)
> > +#define IMX296_GAINDLY					IMX296_REG_8BIT(0x3212)
> > +#define IMX296_GAINDLY_NONE				0x08
> > +#define IMX296_GAINDLY_1FRAME				0x09
> > +#define IMX296_PGCTRL					IMX296_REG_8BIT(0x3238)
> > +#define IMX296_PGCTRL_REGEN				BIT(0)
> > +#define IMX296_PGCTRL_THRU				BIT(1)
> > +#define IMX296_PGCTRL_CLKEN				BIT(2)
> > +#define IMX296_PGCTRL_MODE(n)				((n) << 3)
> > +#define IMX296_PGHPOS					IMX296_REG_16BIT(0x3239)
> > +#define IMX296_PGVPOS					IMX296_REG_16BIT(0x323c)
> > +#define IMX296_PGHPSTEP					IMX296_REG_8BIT(0x323e)
> > +#define IMX296_PGVPSTEP					IMX296_REG_8BIT(0x323f)
> > +#define IMX296_PGHPNUM					IMX296_REG_8BIT(0x3240)
> > +#define IMX296_PGVPNUM					IMX296_REG_8BIT(0x3241)
> > +#define IMX296_PGDATA1					IMX296_REG_16BIT(0x3244)
> > +#define IMX296_PGDATA2					IMX296_REG_16BIT(0x3246)
> > +#define IMX296_PGHGSTEP					IMX296_REG_8BIT(0x3249)
> > +#define IMX296_BLKLEVEL					IMX296_REG_16BIT(0x3254)
> > +
> > +#define IMX296_FID0_ROI					IMX296_REG_8BIT(0x3300)
> > +#define IMX296_FID0_ROIH1ON				BIT(0)
> > +#define IMX296_FID0_ROIV1ON				BIT(1)
> > +#define IMX296_FID0_ROIPH1				IMX296_REG_16BIT(0x3310)
> > +#define IMX296_FID0_ROIPV1				IMX296_REG_16BIT(0x3312)
> > +#define IMX296_FID0_ROIWH1				IMX296_REG_16BIT(0x3314)
> > +#define IMX296_FID0_ROIWH1_MIN				80
> > +#define IMX296_FID0_ROIWV1				IMX296_REG_16BIT(0x3316)
> > +#define IMX296_FID0_ROIWV1_MIN				4
> > +
> > +#define IMX296_CM_HSST_STARTTMG				IMX296_REG_16BIT(0x4018)
> > +#define IMX296_CM_HSST_ENDTMG				IMX296_REG_16BIT(0x401a)
> > +#define IMX296_DA_HSST_STARTTMG				IMX296_REG_16BIT(0x404d)
> > +#define IMX296_DA_HSST_ENDTMG				IMX296_REG_16BIT(0x4050)
> > +#define IMX296_LM_HSST_STARTTMG				IMX296_REG_16BIT(0x4094)
> > +#define IMX296_LM_HSST_ENDTMG				IMX296_REG_16BIT(0x4096)
> > +#define IMX296_SST_SIEASTA1_SET				IMX296_REG_8BIT(0x40c9)
> > +#define IMX296_SST_SIEASTA1PRE_1U			IMX296_REG_16BIT(0x40cc)
> > +#define IMX296_SST_SIEASTA1PRE_1D			IMX296_REG_16BIT(0x40ce)
> > +#define IMX296_SST_SIEASTA1PRE_2U			IMX296_REG_16BIT(0x40d0)
> > +#define IMX296_SST_SIEASTA1PRE_2D			IMX296_REG_16BIT(0x40d2)
> > +#define IMX296_HSST					IMX296_REG_8BIT(0x40dc)
> > +#define IMX296_HSST_EN					BIT(2)
> > +
> > +#define IMX296_CKREQSEL					IMX296_REG_8BIT(0x4101)
> > +#define IMX296_CKREQSEL_HS				BIT(2)
> > +#define IMX296_GTTABLENUM				IMX296_REG_8BIT(0x4114)
> > +#define IMX296_CTRL418C					IMX296_REG_8BIT(0x418c)
> > +
> > +struct imx296_clk_params {
> > +	unsigned int freq;
> > +	u8 incksel[4];
> > +	u8 ctrl418c;
> > +};
> > +
> > +static const struct imx296_clk_params imx296_clk_params[] = {
> > +	{ 37125000, { 0x80, 0x0b, 0x80, 0x08 }, 116 },
> > +	{ 54000000, { 0xb0, 0x0f, 0xb0, 0x0c }, 168 },
> > +	{ 74250000, { 0x80, 0x0f, 0x80, 0x0c }, 232 },
> > +};
> > +
> > +static const char * const imx296_supply_names[] = {
> > +	"dvdd",
> > +	"ovdd",
> > +	"avdd",
> > +};
> > +
> > +struct imx296 {
> > +	struct device *dev;
> > +	struct clk *clk;
> > +	struct regulator_bulk_data supplies[ARRAY_SIZE(imx296_supply_names)];
> > +	struct gpio_desc *reset;
> > +	struct regmap *regmap;
> > +
> > +	const struct imx296_clk_params *clk_params;
> > +	bool mono;
> > +
> > +	bool streaming;			/* Protected by ctrls.lock */
> > +
> > +	struct v4l2_subdev subdev;
> > +	struct media_pad pad;
> > +
> > +	struct v4l2_ctrl_handler ctrls;
> > +	struct v4l2_ctrl *hblank;
> > +	struct v4l2_ctrl *vblank;
> > +
> > +	struct mutex lock;		/* Protects format and crop */
> > +	struct v4l2_mbus_framefmt format;
> > +	struct v4l2_rect crop;
> > +};
> > +
> > +static inline struct imx296 *to_imx296(struct v4l2_subdev *sd)
> > +{
> > +	return container_of(sd, struct imx296, subdev);
> > +}
> > +
> > +static int imx296_read(struct imx296 *sensor, u32 addr)
> > +{
> > +	u8 data[3] = { 0, 0, 0 };
> > +	int ret;
> > +
> > +	ret = regmap_raw_read(sensor->regmap, addr & IMX296_REG_ADDR_MASK, data,
> > +			      (addr >> IMX296_REG_SIZE_SHIFT) & 3);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return (data[2] << 16) | (data[1] << 8) | data[0];
> > +}
> > +
> > +static int imx296_write(struct imx296 *sensor, u32 addr, u32 value, int *err)
> > +{
> > +	u8 data[3] = { value & 0xff, (value >> 8) & 0xff, value >> 16 };
> > +	int ret;
> > +
> > +	if (err && *err)
> > +		return *err;
> > +
> > +	ret = regmap_raw_write(sensor->regmap, addr & IMX296_REG_ADDR_MASK,
> > +			       data, (addr >> IMX296_REG_SIZE_SHIFT) & 3);
> > +	if (ret < 0) {
> > +		dev_err(sensor->dev, "%u-bit write to 0x%04x failed: %d\n",
> > +			((addr >> IMX296_REG_SIZE_SHIFT) & 3) * 8,
> > +			addr & IMX296_REG_ADDR_MASK, ret);
> > +		if (err)
> > +			*err = ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Controls
> > + */
> > +
> > +static const char * const imx296_test_pattern_menu[] = {
> > +	"Disabled",
> > +	"Multiple Pixels",
> > +	"Sequence 1",
> > +	"Sequence 2",
> > +	"Gradient",
> > +	"Row",
> > +	"Column",
> > +	"Cross",
> > +	"Stripe",
> > +	"Checks",
> 
> Interesting pattern name.
> 
> > +};
> > +
> > +static int imx296_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct imx296 *sensor = container_of(ctrl->handler, struct imx296, ctrls);
> > +	unsigned int vmax;
> > +	int ret = 0;
> > +
> > +	if (!sensor->streaming)
> > +		return 0;
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_EXPOSURE:
> > +		/* Clamp the exposure value to VMAX. */
> > +		vmax = sensor->format.height + sensor->vblank->cur.val;
> > +		ctrl->val = min_t(int, ctrl->val, vmax);
> > +		imx296_write(sensor, IMX296_SHS1, vmax - ctrl->val, &ret);
> > +		break;
> > +
> > +	case V4L2_CID_ANALOGUE_GAIN:
> > +		imx296_write(sensor, IMX296_GAIN, ctrl->val, &ret);
> > +		break;
> > +
> > +	case V4L2_CID_VBLANK:
> > +		imx296_write(sensor, IMX296_VMAX,
> > +			     sensor->format.height + ctrl->val, &ret);
> > +		break;
> > +
> > +	case V4L2_CID_TEST_PATTERN:
> > +		if (ctrl->val) {
> > +			imx296_write(sensor, IMX296_PGHPOS, 8, &ret);
> > +			imx296_write(sensor, IMX296_PGVPOS, 8, &ret);
> > +			imx296_write(sensor, IMX296_PGHPSTEP, 8, &ret);
> > +			imx296_write(sensor, IMX296_PGVPSTEP, 8, &ret);
> > +			imx296_write(sensor, IMX296_PGHPNUM, 100, &ret);
> > +			imx296_write(sensor, IMX296_PGVPNUM, 100, &ret);
> > +			imx296_write(sensor, IMX296_PGDATA1, 0x300, &ret);
> > +			imx296_write(sensor, IMX296_PGDATA2, 0x100, &ret);
> > +			imx296_write(sensor, IMX296_PGHGSTEP, 0, &ret);
> > +			imx296_write(sensor, IMX296_BLKLEVEL, 0, &ret);
> > +			imx296_write(sensor, IMX296_BLKLEVELAUTO,
> > +				     IMX296_BLKLEVELAUTO_OFF, &ret);
> > +			imx296_write(sensor, IMX296_PGCTRL,
> > +				     IMX296_PGCTRL_REGEN |
> > +				     IMX296_PGCTRL_CLKEN |
> > +				     IMX296_PGCTRL_MODE(ctrl->val - 1), &ret);
> > +		} else {
> > +			imx296_write(sensor, IMX296_PGCTRL,
> > +				     IMX296_PGCTRL_CLKEN, &ret);
> > +			imx296_write(sensor, IMX296_BLKLEVEL, 0x3c, &ret);
> > +			imx296_write(sensor, IMX296_BLKLEVELAUTO,
> > +				     IMX296_BLKLEVELAUTO_ON, &ret);
> > +		}
> > +		break;
> > +
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops imx296_ctrl_ops = {
> > +	.s_ctrl = imx296_s_ctrl,
> > +};
> > +
> > +static int imx296_ctrls_init(struct imx296 *sensor)
> > +{
> > +	struct v4l2_fwnode_device_properties props;
> > +	unsigned int hblank;
> > +	int ret;
> > +
> > +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > +
> > +	v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > +			  V4L2_CID_EXPOSURE, 1, 1048575, 1, 1104);
> > +	v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > +			  V4L2_CID_ANALOGUE_GAIN, IMX296_GAIN_MIN,
> > +			  IMX296_GAIN_MAX, 1, IMX296_GAIN_MIN);
> > +
> > +	/*
> > +	 * Horizontal blanking is controlled through the HMAX register, which
> > +	 * contains a line length in INCK clock units. The INCK frequency is
> > +	 * fixed to 74.25 MHz. The HMAX value is currently fixed to 1100,
> 
> It seems the driver supports other values, too. Shouldn't this be the
> actual frequency?

That's not clear to me from the documentation I have access to :-( It's
quite convoluted, there are a few examples from which I tried to infer
what was going on, but no clear explanation. My board uses a fixed clock
frequency of 37.125MHz so I can't test other values.

Can we start with this and update it later if we can figure out more
(assuming there's an issue, it may actually be correct already) ?

> > +	 * convert it to a number of pixels based on the nominal pixel rate.
> > +	 */
> > +	hblank = 1100 * 1188000000ULL / 10 / 74250000
> > +	       - IMX296_PIXEL_ARRAY_WIDTH;
> > +	sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > +					   V4L2_CID_HBLANK, hblank, hblank, 1,
> > +					   hblank);
> > +	if (sensor->hblank)
> > +		sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +	sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > +					   V4L2_CID_VBLANK, 30,
> > +					   1048575 - IMX296_PIXEL_ARRAY_HEIGHT,
> > +					   1, 30);
> > +	/*
> > +	 * The sensor calculates the MIPI timings internally to achieve a bit
> > +	 * rate between 1122 and 1198 Mbps. The exact value is unfortunately not
> > +	 * reported, at least according to the documentation. Report a nominal
> > +	 * rate of 1188 Mbps as that is used by the datasheet in multiple
> > +	 * examples.
> > +	 */
> > +	v4l2_ctrl_new_std(&sensor->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> > +			  1122000000 / 10, 1198000000 / 10, 1, 1188000000 / 10);
> 
> What about the link frequency?
> 
> Is this value constant for the sensor? Or should there be a list of
> hardware supported link frequencies?

It seems to be constant, but again the documentation is fairly unclear.

> > +	v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &imx296_ctrl_ops,
> > +				     V4L2_CID_TEST_PATTERN,
> > +				     ARRAY_SIZE(imx296_test_pattern_menu) - 1,
> > +				     0, 0, imx296_test_pattern_menu);
> > +
> > +	v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &imx296_ctrl_ops,
> > +					&props);
> > +
> > +	if (sensor->ctrls.error) {
> > +		dev_err(sensor->dev, "failed to add controls (%d)\n",
> > +			sensor->ctrls.error);
> > +		v4l2_ctrl_handler_free(&sensor->ctrls);
> > +		return sensor->ctrls.error;
> > +	}
> > +
> > +	sensor->subdev.ctrl_handler = &sensor->ctrls;
> > +
> > +	return 0;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * V4L2 Subdev Operations
> > + */
> > +
> > +/*
> > + * This table is extracted from vendor data that is entirely undocumented. The
> > + * first register write is required to activate the CSI-2 output. The other
> > + * entries may or may not be optional?
> > + */
> > +static const struct {
> > +	unsigned int reg;
> > +	unsigned int value;
> > +} imx296_init_table[] = {
> > +	{ IMX296_REG_8BIT(0x3005), 0xf0 },
> > +	{ IMX296_REG_8BIT(0x309e), 0x04 },
> > +	{ IMX296_REG_8BIT(0x30a0), 0x04 },
> > +	{ IMX296_REG_8BIT(0x30a1), 0x3c },
> > +	{ IMX296_REG_8BIT(0x30a4), 0x5f },
> > +	{ IMX296_REG_8BIT(0x30a8), 0x91 },
> > +	{ IMX296_REG_8BIT(0x30ac), 0x28 },
> > +	{ IMX296_REG_8BIT(0x30af), 0x09 },
> > +	{ IMX296_REG_8BIT(0x30df), 0x00 },
> > +	{ IMX296_REG_8BIT(0x3165), 0x00 },
> > +	{ IMX296_REG_8BIT(0x3169), 0x10 },
> > +	{ IMX296_REG_8BIT(0x316a), 0x02 },
> > +	{ IMX296_REG_8BIT(0x31c8), 0xf3 },	/* Exposure-related */
> > +	{ IMX296_REG_8BIT(0x31d0), 0xf4 },	/* Exposure-related */
> > +	{ IMX296_REG_8BIT(0x321a), 0x00 },
> > +	{ IMX296_REG_8BIT(0x3226), 0x02 },
> > +	{ IMX296_REG_8BIT(0x3256), 0x01 },
> > +	{ IMX296_REG_8BIT(0x3541), 0x72 },
> > +	{ IMX296_REG_8BIT(0x3516), 0x77 },
> > +	{ IMX296_REG_8BIT(0x350b), 0x7f },
> > +	{ IMX296_REG_8BIT(0x3758), 0xa3 },
> > +	{ IMX296_REG_8BIT(0x3759), 0x00 },
> > +	{ IMX296_REG_8BIT(0x375a), 0x85 },
> > +	{ IMX296_REG_8BIT(0x375b), 0x00 },
> > +	{ IMX296_REG_8BIT(0x3832), 0xf5 },
> > +	{ IMX296_REG_8BIT(0x3833), 0x00 },
> > +	{ IMX296_REG_8BIT(0x38a2), 0xf6 },
> > +	{ IMX296_REG_8BIT(0x38a3), 0x00 },
> > +	{ IMX296_REG_8BIT(0x3a00), 0x80 },
> > +	{ IMX296_REG_8BIT(0x3d48), 0xa3 },
> > +	{ IMX296_REG_8BIT(0x3d49), 0x00 },
> > +	{ IMX296_REG_8BIT(0x3d4a), 0x85 },
> > +	{ IMX296_REG_8BIT(0x3d4b), 0x00 },
> > +	{ IMX296_REG_8BIT(0x400e), 0x58 },
> > +	{ IMX296_REG_8BIT(0x4014), 0x1c },
> > +	{ IMX296_REG_8BIT(0x4041), 0x2a },
> > +	{ IMX296_REG_8BIT(0x40a2), 0x06 },
> > +	{ IMX296_REG_8BIT(0x40c1), 0xf6 },
> > +	{ IMX296_REG_8BIT(0x40c7), 0x0f },
> > +	{ IMX296_REG_8BIT(0x40c8), 0x00 },
> > +	{ IMX296_REG_8BIT(0x4174), 0x00 },
> > +};
> > +
> > +static int imx296_setup(struct imx296 *sensor)
> > +{
> > +	const struct v4l2_mbus_framefmt *format = &sensor->format;
> > +	const struct v4l2_rect *crop = &sensor->crop;
> > +	unsigned int i;
> > +	int ret = 0;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(imx296_init_table); ++i)
> > +		imx296_write(sensor, imx296_init_table[i].reg,
> > +			     imx296_init_table[i].value, &ret);
> > +
> > +	if (crop->width != IMX296_PIXEL_ARRAY_WIDTH ||
> > +	    crop->height != IMX296_PIXEL_ARRAY_HEIGHT) {
> > +		imx296_write(sensor, IMX296_FID0_ROI,
> > +			     IMX296_FID0_ROIH1ON | IMX296_FID0_ROIV1ON, &ret);
> > +		imx296_write(sensor, IMX296_FID0_ROIPH1, crop->left, &ret);
> > +		imx296_write(sensor, IMX296_FID0_ROIPV1, crop->top, &ret);
> > +		imx296_write(sensor, IMX296_FID0_ROIWH1, crop->width, &ret);
> > +		imx296_write(sensor, IMX296_FID0_ROIWV1, crop->height, &ret);
> > +	} else {
> > +		imx296_write(sensor, IMX296_FID0_ROI, 0, &ret);
> > +	}
> > +
> > +	imx296_write(sensor, IMX296_CTRL0D,
> > +		     (crop->width != format->width ?
> > +		      IMX296_CTRL0D_HADD_ON_BINNING : 0) |
> > +		     (crop->height != format->height ?
> > +		      IMX296_CTRL0D_WINMODE_FD_BINNING : 0),
> > +		     &ret);
> > +
> > +	/*
> > +	 * HMAX and VMAX configure horizontal and vertical blanking by
> > +	 * specifying the total line time and frame time respectively. The line
> > +	 * time is specified in operational clock units (which appears to be the
> > +	 * output of an internal PLL, fixed at 74.25 MHz regardless of the
> > +	 * exernal clock frequency), while the frame time is specified as a
> > +	 * number of lines.
> > +	 *
> > +	 * In the vertical direction the sensor outputs the following:
> > +	 *
> > +	 * - one line for the FS packet
> > +	 * - two lines of embedded data (DT 0x12)
> > +	 * - six null lines (DT 0x10)
> > +	 * - four lines of vertical effective optical black (DT 0x37)
> > +	 * - 8 to 1088 lines of active image data (RAW10, DT 0x2b)
> > +	 * - one line for the FE packet
> > +	 * - 16 or more lines of vertical blanking
> > +	 */
> > +	imx296_write(sensor, IMX296_HMAX, 1100, &ret);
> > +	imx296_write(sensor, IMX296_VMAX,
> > +		     format->height + sensor->vblank->cur.val, &ret);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(sensor->clk_params->incksel); ++i)
> > +		imx296_write(sensor, IMX296_INCKSEL(i),
> > +			     sensor->clk_params->incksel[i], &ret);
> > +	imx296_write(sensor, IMX296_GTTABLENUM, 0xc5, &ret);
> > +	imx296_write(sensor, IMX296_CTRL418C, sensor->clk_params->ctrl418c,
> > +		     &ret);
> > +
> > +	imx296_write(sensor, IMX296_GAINDLY, IMX296_GAINDLY_NONE, &ret);
> > +	imx296_write(sensor, IMX296_BLKLEVEL, 0x03c, &ret);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return __v4l2_ctrl_handler_setup(&sensor->ctrls);
> > +}
> > +
> > +static int imx296_stream_on(struct imx296 *sensor)
> > +{
> > +	int ret = 0;
> > +
> > +	imx296_write(sensor, IMX296_CTRL00, 0, &ret);
> > +	usleep_range(2000, 5000);
> > +	imx296_write(sensor, IMX296_CTRL0A, 0, &ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx296_stream_off(struct imx296 *sensor)
> > +{
> > +	int ret = 0;
> > +
> > +	imx296_write(sensor, IMX296_CTRL0A, IMX296_CTRL0A_XMSTA, &ret);
> > +	imx296_write(sensor, IMX296_CTRL00, IMX296_CTRL00_STANDBY, &ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx296_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct imx296 *sensor = to_imx296(sd);
> > +	int ret;
> > +
> > +	if (!enable) {
> > +		ret = imx296_stream_off(sensor);
> > +
> > +		pm_runtime_mark_last_busy(sensor->dev);
> > +		pm_runtime_put_autosuspend(sensor->dev);
> > +
> > +		mutex_lock(sensor->ctrls.lock);
> > +		sensor->streaming = false;
> > +		mutex_unlock(sensor->ctrls.lock);
> > +
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(sensor->ctrls.lock);
> > +
> > +	ret = pm_runtime_get_sync(sensor->dev);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	ret = imx296_setup(sensor);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	/*
> > +	 * Set streaming to true to ensure __v4l2_ctrl_handler_setup() will set
> > +	 * the controls. The flag is reset to false further down if an error
> > +	 * occurs.
> > +	 */
> > +	sensor->streaming = true;
> > +
> > +	ret = __v4l2_ctrl_handler_setup(&sensor->ctrls);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	ret = imx296_stream_on(sensor);
> > +
> > +done:
> > +	if (ret < 0) {
> > +		/*
> > +		 * In case of error, turn the power off synchronously as the
> > +		 * device likely has no other chance to recover.
> > +		 */
> > +		pm_runtime_put_sync(sensor->dev);
> > +		sensor->streaming = false;
> > +	}
> > +
> > +	mutex_unlock(sensor->ctrls.lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx296_enum_mbus_code(struct v4l2_subdev *sd,
> > +				 struct v4l2_subdev_state *state,
> > +				 struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +	struct imx296 *sensor = to_imx296(sd);
> > +
> > +	if (code->index != 0)
> > +		return -EINVAL;
> > +
> > +	code->code = sensor->mono ? MEDIA_BUS_FMT_Y10_1X10
> > +		   : MEDIA_BUS_FMT_SBGGR10_1X10;
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx296_enum_frame_size(struct v4l2_subdev *sd,
> > +				  struct v4l2_subdev_state *state,
> > +				  struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +	struct imx296 *sensor = to_imx296(sd);
> > +
> > +	if (fse->index >= 2 || fse->code != sensor->format.code)
> > +		return -EINVAL;
> > +
> > +	fse->min_width = IMX296_PIXEL_ARRAY_WIDTH / (fse->index + 1);
> > +	fse->max_width = fse->min_width;
> > +	fse->min_height = IMX296_PIXEL_ARRAY_HEIGHT / (fse->index + 1);
> > +	fse->max_height = fse->min_height;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct v4l2_mbus_framefmt *
> > +imx296_get_pad_format(struct imx296 *sensor, struct v4l2_subdev_state *state,
> > +		      unsigned int pad, u32 which)
> > +{
> > +	switch (which) {
> > +	case V4L2_SUBDEV_FORMAT_TRY:
> > +		return v4l2_subdev_get_try_format(&sensor->subdev, state, pad);
> > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +		return &sensor->format;
> > +	default:
> > +		return NULL;
> > +	}
> > +}
> > +
> > +static struct v4l2_rect *
> > +imx296_get_pad_crop(struct imx296 *sensor, struct v4l2_subdev_state *state,
> > +		    unsigned int pad, u32 which)
> > +{
> > +	switch (which) {
> > +	case V4L2_SUBDEV_FORMAT_TRY:
> > +		return v4l2_subdev_get_try_crop(&sensor->subdev, state, pad);
> > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +		return &sensor->crop;
> > +	default:
> > +		return NULL;
> > +	}
> > +}
> > +
> > +static int imx296_get_format(struct v4l2_subdev *sd,
> > +			     struct v4l2_subdev_state *state,
> > +			     struct v4l2_subdev_format *fmt)
> > +{
> > +	struct imx296 *sensor = to_imx296(sd);
> > +
> > +	mutex_lock(&sensor->lock);
> > +	fmt->format = *imx296_get_pad_format(sensor, state, fmt->pad,
> > +					     fmt->which);
> > +	mutex_unlock(&sensor->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx296_set_format(struct v4l2_subdev *sd,
> > +			     struct v4l2_subdev_state *state,
> > +			     struct v4l2_subdev_format *fmt)
> > +{
> > +	struct imx296 *sensor = to_imx296(sd);
> > +	struct v4l2_mbus_framefmt *format;
> > +	struct v4l2_rect *crop;
> > +
> > +	crop = imx296_get_pad_crop(sensor, state, fmt->pad, fmt->which);
> > +	format = imx296_get_pad_format(sensor, state, fmt->pad, fmt->which);
> > +
> > +	mutex_lock(&sensor->lock);
> > +
> > +	/*
> > +	 * Binning is only allowed when cropping is disabled according to the
> > +	 * documentation. This should be double-checked.
> > +	 */
> > +	if (crop->width == IMX296_PIXEL_ARRAY_WIDTH &&
> > +	    crop->height == IMX296_PIXEL_ARRAY_HEIGHT) {
> > +		unsigned int width;
> > +		unsigned int height;
> > +		unsigned int hratio;
> > +		unsigned int vratio;
> > +
> > +		/* Clamp the width and height to avoid dividing by zero. */
> > +		width = clamp_t(unsigned int, fmt->format.width,
> > +				crop->width / 2, crop->width);
> > +		height = clamp_t(unsigned int, fmt->format.height,
> > +				 crop->height / 2, crop->height);
> > +
> > +		hratio = DIV_ROUND_CLOSEST(crop->width, width);
> > +		vratio = DIV_ROUND_CLOSEST(crop->height, height);
> > +
> > +		format->width = crop->width / hratio;
> > +		format->height = crop->height / vratio;
> > +	} else {
> > +		format->width = crop->width;
> > +		format->height = crop->height;
> > +	}
> > +
> > +	format->code = sensor->mono ? MEDIA_BUS_FMT_Y10_1X10
> > +		     : MEDIA_BUS_FMT_SBGGR10_1X10;
> > +	format->field = V4L2_FIELD_NONE;
> > +	format->colorspace = V4L2_COLORSPACE_RAW;
> > +	format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	format->xfer_func = V4L2_XFER_FUNC_NONE;
> > +
> > +	fmt->format = *format;
> > +
> > +	mutex_unlock(&sensor->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx296_get_selection(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_state *state,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	struct imx296 *sensor = to_imx296(sd);
> > +
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +		mutex_lock(&sensor->lock);
> > +		sel->r = *imx296_get_pad_crop(sensor, state, sel->pad,
> > +					      sel->which);
> > +		mutex_unlock(&sensor->lock);
> > +		break;
> > +
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > +		sel->r.left = 0;
> > +		sel->r.top = 0;
> > +		sel->r.width = IMX296_PIXEL_ARRAY_WIDTH;
> > +		sel->r.height = IMX296_PIXEL_ARRAY_HEIGHT;
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx296_set_selection(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_state *state,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	struct imx296 *sensor = to_imx296(sd);
> > +	struct v4l2_mbus_framefmt *format;
> > +	struct v4l2_rect *crop;
> > +	struct v4l2_rect rect;
> > +
> > +	if (sel->target != V4L2_SEL_TGT_CROP)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Clamp the crop rectangle boundaries and align them to a multiple of 4
> > +	 * pixels to satisfy hardware requirements.
> > +	 */
> > +	rect.left = clamp(ALIGN(sel->r.left, 4), 0,
> > +			  IMX296_PIXEL_ARRAY_WIDTH - IMX296_FID0_ROIWH1_MIN);
> > +	rect.top = clamp(ALIGN(sel->r.top, 4), 0,
> > +			  IMX296_PIXEL_ARRAY_HEIGHT - IMX296_FID0_ROIWV1_MIN);
> > +	rect.width = clamp_t(unsigned int, ALIGN(sel->r.width, 4),
> > +			     IMX296_FID0_ROIWH1_MIN, IMX296_PIXEL_ARRAY_WIDTH);
> > +	rect.height = clamp_t(unsigned int, ALIGN(sel->r.height, 4),
> > +			      IMX296_FID0_ROIWV1_MIN, IMX296_PIXEL_ARRAY_HEIGHT);
> > +
> > +	rect.width = min_t(unsigned int, rect.width,
> > +			   IMX296_PIXEL_ARRAY_WIDTH - rect.left);
> > +	rect.height = min_t(unsigned int, rect.height,
> > +			    IMX296_PIXEL_ARRAY_HEIGHT - rect.top);
> > +
> > +	crop = imx296_get_pad_crop(sensor, state, sel->pad, sel->which);
> > +
> > +	mutex_lock(&sensor->lock);
> > +
> > +	if (rect.width != crop->width || rect.height != crop->height) {
> > +		/*
> > +		 * Reset the output image size if the crop rectangle size has
> > +		 * been modified.
> > +		 */
> > +		format = imx296_get_pad_format(sensor, state, sel->pad,
> > +					       sel->which);
> > +		format->width = rect.width;
> > +		format->height = rect.height;
> > +	}
> > +
> > +	*crop = rect;
> > +	sel->r = rect;
> > +
> > +	mutex_unlock(&sensor->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx296_init_cfg(struct v4l2_subdev *sd,
> > +			   struct v4l2_subdev_state *state)
> > +{
> > +	struct v4l2_subdev_selection sel = {
> > +		.target = V4L2_SEL_TGT_CROP,
> > +		.which = state ? V4L2_SUBDEV_FORMAT_TRY
> > +		       : V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		.r.width = IMX296_PIXEL_ARRAY_WIDTH,
> > +		.r.height = IMX296_PIXEL_ARRAY_HEIGHT,
> > +	};
> > +	struct v4l2_subdev_format format = {
> > +		.which = state ? V4L2_SUBDEV_FORMAT_TRY
> > +		       : V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		.format = {
> > +			.width = IMX296_PIXEL_ARRAY_WIDTH,
> > +			.height = IMX296_PIXEL_ARRAY_HEIGHT,
> > +		},
> > +	};
> > +
> > +	imx296_set_selection(sd, state, &sel);
> > +	imx296_set_format(sd, state, &format);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops imx296_subdev_video_ops = {
> > +	.s_stream = imx296_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops imx296_subdev_pad_ops = {
> > +	.enum_mbus_code = imx296_enum_mbus_code,
> > +	.enum_frame_size = imx296_enum_frame_size,
> > +	.get_fmt = imx296_get_format,
> > +	.set_fmt = imx296_set_format,
> > +	.get_selection = imx296_get_selection,
> > +	.set_selection = imx296_set_selection,
> > +	.init_cfg = imx296_init_cfg,
> > +};
> > +
> > +static const struct v4l2_subdev_ops imx296_subdev_ops = {
> > +	.video = &imx296_subdev_video_ops,
> > +	.pad = &imx296_subdev_pad_ops,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Power management
> > + */
> > +
> > +static int imx296_power_on(struct imx296 *sensor)
> > +{
> > +	int ret;
> > +
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(sensor->supplies),
> > +				    sensor->supplies);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	udelay(1);
> 
> This is a very small delay.
> 
> > +
> > +	ret = gpiod_direction_output(sensor->reset, 0);
> > +	if (ret < 0)
> > +		goto err_supply;
> > +
> > +	udelay(1);
> 
> This one as well.

Those are what the datasheet reports as required. The exact constraint
for the first of the two delays is actually >+ 0.5µs, I've rounded it up
:-)

> > +
> > +	ret = clk_prepare_enable(sensor->clk);
> > +	if (ret < 0)
> > +		goto err_reset;
> > +
> > +	/*
> > +	 * The documentation doesn't explicitly say how much time is required
> > +	 * after providing a clock and before starting I2C communication. It
> > +	 * mentions a delay of 20µs in 4-wire mode, but tests showed that a
> > +	 * delay of 100µs resulted in I2C communication failures, while 500µs
> > +	 * seems to be enough. Be conservative.
> > +	 */
> > +	usleep_range(1000, 2000);
> > +
> > +	return 0;
> > +
> > +err_reset:
> > +	gpiod_direction_output(sensor->reset, 1);
> > +err_supply:
> > +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
> > +	return ret;
> > +}
> > +
> > +static void imx296_power_off(struct imx296 *sensor)
> > +{
> > +	clk_disable_unprepare(sensor->clk);
> > +	gpiod_direction_output(sensor->reset, 1);
> > +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
> > +}
> > +
> > +static int __maybe_unused imx296_runtime_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > +	struct imx296 *sensor = to_imx296(subdev);
> > +
> > +	return imx296_power_on(sensor);
> > +}
> > +
> > +static int __maybe_unused imx296_runtime_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > +	struct imx296 *sensor = to_imx296(subdev);
> > +
> > +	imx296_power_off(sensor);
> > +
> > +	return 0;
> 
> I'd merge these two with imx296_power_o{n,ff}.

That would require calling imx296_runtime_resume() and
imx296_runtime_suspend() in probe() and remove(), which I don't really
like. I'd prefer keeping the functions separate.

> > +}
> > +
> > +static const struct dev_pm_ops imx296_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(imx296_runtime_suspend, imx296_runtime_resume, NULL)
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Probe & Remove
> > + */
> > +
> > +static int imx296_read_temperature(struct imx296 *sensor, int *temp)
> > +{
> > +	int tmdout;
> > +	int ret;
> > +
> > +	ret = imx296_write(sensor, IMX296_TMDCTRL, IMX296_TMDCTRL_LATCH, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	tmdout = imx296_read(sensor, IMX296_TMDOUT) & IMX296_TMDOUT_MASK;
> > +	if (tmdout < 0)
> > +		return tmdout;
> > +
> > +	/* T(°C) = 246.312 - 0.304 * TMDOUT */;
> > +	*temp = 246312 - 304 * tmdout;
> > +
> > +	return imx296_write(sensor, IMX296_TMDCTRL, 0, NULL);
> > +}
> > +
> > +static int imx296_identify_model(struct imx296 *sensor)
> > +{
> > +	unsigned int model;
> > +	int temp = 0;
> > +	int ret;
> > +
> > +	/*
> > +	 * While most registers can be read when the sensor is in standby, this
> > +	 * is not the case of the sensor info register :-(
> > +	 */
> > +	ret = imx296_write(sensor, IMX296_CTRL00, 0, NULL);
> > +	if (ret < 0) {
> > +		dev_err(sensor->dev,
> > +			"failed to get sensor out of standby (%d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = imx296_read(sensor, IMX296_SENSOR_INFO);
> > +	if (ret < 0) {
> > +		dev_err(sensor->dev, "failed to read sensor information (%d)\n",
> > +			ret);
> > +		goto done;
> > +	}
> > +
> > +	model = (ret >> 6) & 0x1ff;
> > +
> > +	switch (model) {
> > +	case 296:
> > +		sensor->mono = ret & IMX296_SENSOR_INFO_MONO;
> > +		break;
> > +	/*
> > +	 * The IMX297 seems to share features with the IMX296, it may be
> > +	 * possible to support it in the same driver.
> > +	 */
> > +	case 297:
> > +	default:
> > +		dev_err(sensor->dev, "invalid device model 0x%04x\n", ret);
> > +		ret = -ENODEV;
> > +		goto done;
> > +	}
> > +
> > +	ret = imx296_read_temperature(sensor, &temp);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	dev_info(sensor->dev, "found IMX%u%s (%u.%uC)\n", model,
> > +		 sensor->mono ? "LL" : "LQ", temp / 1000, (temp / 100) % 10);
> > +
> > +done:
> > +	imx296_write(sensor, IMX296_CTRL00, IMX296_CTRL00_STANDBY, NULL);
> > +	return ret;
> > +}
> > +
> > +static const struct regmap_config imx296_regmap_config = {
> > +	.reg_bits = 16,
> > +	.val_bits = 8,
> > +
> > +	.wr_table = &(const struct regmap_access_table) {
> > +		.no_ranges = (const struct regmap_range[]) {
> > +			{
> > +				.range_min = IMX296_SENSOR_INFO & 0xffff,
> > +				.range_max = (IMX296_SENSOR_INFO & 0xffff) + 1,
> > +			},
> > +		},
> > +		.n_no_ranges = 1,
> > +	},
> > +};
> > +
> > +static int imx296_probe(struct i2c_client *client)
> > +{
> > +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > +	unsigned long clk_rate;
> > +	struct imx296 *sensor;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > +		dev_warn(&adapter->dev,
> > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE\n");
> > +		return -EIO;
> > +	}
> > +
> > +	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> > +	if (!sensor)
> > +		return -ENOMEM;
> > +
> > +	sensor->dev = &client->dev;
> > +
> > +	mutex_init(&sensor->lock);
> 
> You could simplify error handling a little by moving mutex init later. Up
> to you.

That's right, but if you don't mind I'd prefer keeping it here, to have
ass the "static" initialization of "generic" members at the top.

> > +
> > +	/* Acquire resources. */
> > +	for (i = 0; i < ARRAY_SIZE(sensor->supplies); ++i)
> > +		sensor->supplies[i].supply = imx296_supply_names[i];
> > +
> > +	ret = devm_regulator_bulk_get(sensor->dev, ARRAY_SIZE(sensor->supplies),
> > +				      sensor->supplies);
> > +	if (ret) {
> > +		dev_err_probe(sensor->dev, ret, "failed to get supplies\n");
> > +		goto err_mutex;
> > +	}
> > +
> > +	sensor->reset = devm_gpiod_get_optional(sensor->dev, "reset",
> > +						GPIOD_OUT_HIGH);
> > +	if (IS_ERR(sensor->reset)) {
> > +		ret = PTR_ERR(sensor->reset);
> > +		dev_err_probe(sensor->dev, ret, "failed to get reset GPIO\n");
> > +		goto err_mutex;
> > +	}
> > +
> > +	sensor->clk = devm_clk_get(sensor->dev, "inck");
> > +	if (IS_ERR(sensor->clk)) {
> > +		ret = PTR_ERR(sensor->clk);
> > +		dev_err_probe(sensor->dev, ret, "failed to get clock\n");
> > +		goto err_mutex;
> > +	}
> > +
> > +	clk_rate = clk_get_rate(sensor->clk);
> > +	for (i = 0; i < ARRAY_SIZE(imx296_clk_params); ++i) {
> > +		if (clk_rate == imx296_clk_params[i].freq) {
> > +			sensor->clk_params = &imx296_clk_params[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!sensor->clk_params) {
> > +		dev_err(sensor->dev, "unsupported clock rate %lu\n", clk_rate);
> > +		ret = -EINVAL;
> > +		goto err_mutex;
> > +	}
> > +
> > +	sensor->regmap = devm_regmap_init_i2c(client, &imx296_regmap_config);
> > +	if (IS_ERR(sensor->regmap)) {
> > +		ret = PTR_ERR(sensor->regmap);
> > +		goto err_mutex;
> > +	}
> > +
> > +	/*
> > +	 * Enable power management. The driver supports runtime PM, but needs to
> > +	 * work when runtime PM is disabled in the kernel. To that end, power
> > +	 * the sensor on manually here, identify it, and fully initialize it.
> > +	 */
> > +	ret = imx296_power_on(sensor);
> > +	if (ret < 0)
> > +		goto err_mutex;
> > +
> > +	ret = imx296_identify_model(sensor);
> > +	if (ret < 0)
> > +		goto err_power;
> > +
> > +	/* Initialize the V4L2 subdev, controls and media entity. */
> > +	v4l2_i2c_subdev_init(&sensor->subdev, client, &imx296_subdev_ops);
> > +
> > +	ret = imx296_ctrls_init(sensor);
> > +	if (ret < 0)
> > +		goto err_power;
> > +
> > +	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +	sensor->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +	ret = media_entity_pads_init(&sensor->subdev.entity, 1, &sensor->pad);
> > +	if (ret < 0)
> > +		goto err_ctrls;
> > +
> > +	imx296_init_cfg(&sensor->subdev, NULL);
> > +
> > +	/*
> > +	 * Enable runtime PM. As the device has been powered manually, mark it
> > +	 * as active, and increase the usage count without resuming the device.
> > +	 */
> > +	pm_runtime_set_active(sensor->dev);
> > +	pm_runtime_get_noresume(sensor->dev);
> > +	pm_runtime_enable(sensor->dev);
> > +
> > +	ret = v4l2_async_register_subdev(&sensor->subdev);
> > +	if (ret < 0)
> > +		goto err_pm;
> > +
> > +	/*
> > +	 * Finally, enable autosuspend and decrease the usage count. The device
> > +	 * will get suspended after the autosuspend delay, turning the power
> > +	 * off.
> > +	 */
> > +	pm_runtime_set_autosuspend_delay(sensor->dev, 1000);
> > +	pm_runtime_use_autosuspend(sensor->dev);
> > +	pm_runtime_put_autosuspend(sensor->dev);
> > +
> > +	return 0;
> > +
> > +err_pm:
> > +	pm_runtime_disable(sensor->dev);
> > +	pm_runtime_put_noidle(sensor->dev);
> > +	media_entity_cleanup(&sensor->subdev.entity);
> > +err_ctrls:
> > +	v4l2_ctrl_handler_free(&sensor->ctrls);
> > +err_power:
> > +	imx296_power_off(sensor);
> > +err_mutex:
> > +	mutex_destroy(&sensor->lock);
> > +	return ret;
> > +}
> > +
> > +static int imx296_remove(struct i2c_client *client)
> > +{
> > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > +	struct imx296 *sensor = to_imx296(subdev);
> > +
> > +	v4l2_async_unregister_subdev(subdev);
> > +	media_entity_cleanup(&subdev->entity);
> > +	v4l2_ctrl_handler_free(&sensor->ctrls);
> > +
> > +	/*
> > +	 * Disable runtime PM. In case runtime PM is disabled in the kernel,
> > +	 * make sure to turn power off manually.
> > +	 */
> > +	pm_runtime_disable(sensor->dev);
> > +	if (!pm_runtime_status_suspended(sensor->dev))
> > +		imx296_power_off(sensor);
> > +	pm_runtime_set_suspended(sensor->dev);
> > +
> > +	mutex_destroy(&sensor->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id imx296_of_match[] = {
> > +	{ .compatible = "sony,imx296" },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, imx296_of_match);
> > +
> > +static struct i2c_driver imx296_i2c_driver = {
> > +	.driver = {
> > +		.of_match_table = imx296_of_match,
> > +		.name = "imx296",
> > +		.pm = &imx296_pm_ops
> > +	},
> > +	.probe_new = imx296_probe,
> > +	.remove = imx296_remove,
> > +};
> > +
> > +module_i2c_driver(imx296_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("Sony IMX296 Camera driver");
> > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > +MODULE_LICENSE("GPL v2");

Hi Laurent,

On Tue, Dec 21, 2021 at 05:56:54PM +0200, Laurent Pinchart wrote:

,,,

> > > +static int imx296_ctrls_init(struct imx296 *sensor)
> > > +{
> > > +	struct v4l2_fwnode_device_properties props;
> > > +	unsigned int hblank;
> > > +	int ret;
> > > +
> > > +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > > +
> > > +	v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > +			  V4L2_CID_EXPOSURE, 1, 1048575, 1, 1104);
> > > +	v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > +			  V4L2_CID_ANALOGUE_GAIN, IMX296_GAIN_MIN,
> > > +			  IMX296_GAIN_MAX, 1, IMX296_GAIN_MIN);
> > > +
> > > +	/*
> > > +	 * Horizontal blanking is controlled through the HMAX register, which
> > > +	 * contains a line length in INCK clock units. The INCK frequency is
> > > +	 * fixed to 74.25 MHz. The HMAX value is currently fixed to 1100,
> > 
> > It seems the driver supports other values, too. Shouldn't this be the
> > actual frequency?
> 
> That's not clear to me from the documentation I have access to :-( It's
> quite convoluted, there are a few examples from which I tried to infer
> what was going on, but no clear explanation. My board uses a fixed clock
> frequency of 37.125MHz so I can't test other values.
> 
> Can we start with this and update it later if we can figure out more
> (assuming there's an issue, it may actually be correct already) ?

Sounds reasonable. I was just wondering.

> 
> > > +	 * convert it to a number of pixels based on the nominal pixel rate.
> > > +	 */
> > > +	hblank = 1100 * 1188000000ULL / 10 / 74250000
> > > +	       - IMX296_PIXEL_ARRAY_WIDTH;
> > > +	sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > +					   V4L2_CID_HBLANK, hblank, hblank, 1,
> > > +					   hblank);
> > > +	if (sensor->hblank)
> > > +		sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > +	sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > +					   V4L2_CID_VBLANK, 30,
> > > +					   1048575 - IMX296_PIXEL_ARRAY_HEIGHT,
> > > +					   1, 30);
> > > +	/*
> > > +	 * The sensor calculates the MIPI timings internally to achieve a bit
> > > +	 * rate between 1122 and 1198 Mbps. The exact value is unfortunately not
> > > +	 * reported, at least according to the documentation. Report a nominal
> > > +	 * rate of 1188 Mbps as that is used by the datasheet in multiple
> > > +	 * examples.
> > > +	 */
> > > +	v4l2_ctrl_new_std(&sensor->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> > > +			  1122000000 / 10, 1198000000 / 10, 1, 1188000000 / 10);
> > 
> > What about the link frequency?
> > 
> > Is this value constant for the sensor? Or should there be a list of
> > hardware supported link frequencies?
> 
> It seems to be constant, but again the documentation is fairly unclear.

Ack.

...

> > > +static int __maybe_unused imx296_runtime_resume(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > +	struct imx296 *sensor = to_imx296(subdev);
> > > +
> > > +	return imx296_power_on(sensor);
> > > +}
> > > +
> > > +static int __maybe_unused imx296_runtime_suspend(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > +	struct imx296 *sensor = to_imx296(subdev);
> > > +
> > > +	imx296_power_off(sensor);
> > > +
> > > +	return 0;
> > 
> > I'd merge these two with imx296_power_o{n,ff}.
> 
> That would require calling imx296_runtime_resume() and
> imx296_runtime_suspend() in probe() and remove(), which I don't really
> like. I'd prefer keeping the functions separate.

You could keep calling the functions imx296_power_o{n,ff}. There's really
no need for two pairs of functions doing the same things.

...

> > > +		dev_warn(&adapter->dev,
> > > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE\n");
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> > > +	if (!sensor)
> > > +		return -ENOMEM;
> > > +
> > > +	sensor->dev = &client->dev;
> > > +
> > > +	mutex_init(&sensor->lock);
> > 
> > You could simplify error handling a little by moving mutex init later. Up
> > to you.
> 
> That's right, but if you don't mind I'd prefer keeping it here, to have
> ass the "static" initialization of "generic" members at the top.

Sure.

Hi Sakari,

On Wed, Dec 22, 2021 at 11:31:06AM +0200, Sakari Ailus wrote:
> On Wed, Dec 22, 2021 at 12:53:27AM +0200, Laurent Pinchart wrote:
> > On Wed, Dec 22, 2021 at 12:41:08AM +0200, Sakari Ailus wrote:
> > > On Tue, Dec 21, 2021 at 05:56:54PM +0200, Laurent Pinchart wrote:
> > > 
> > > ,,,
> > > 
> > > > > > +static int imx296_ctrls_init(struct imx296 *sensor)
> > > > > > +{
> > > > > > +	struct v4l2_fwnode_device_properties props;
> > > > > > +	unsigned int hblank;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> > > > > > +	if (ret < 0)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > > > > > +
> > > > > > +	v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > > > > +			  V4L2_CID_EXPOSURE, 1, 1048575, 1, 1104);
> > > > > > +	v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > > > > +			  V4L2_CID_ANALOGUE_GAIN, IMX296_GAIN_MIN,
> > > > > > +			  IMX296_GAIN_MAX, 1, IMX296_GAIN_MIN);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Horizontal blanking is controlled through the HMAX register, which
> > > > > > +	 * contains a line length in INCK clock units. The INCK frequency is
> > > > > > +	 * fixed to 74.25 MHz. The HMAX value is currently fixed to 1100,
> > > > > 
> > > > > It seems the driver supports other values, too. Shouldn't this be the
> > > > > actual frequency?
> > > > 
> > > > That's not clear to me from the documentation I have access to :-( It's
> > > > quite convoluted, there are a few examples from which I tried to infer
> > > > what was going on, but no clear explanation. My board uses a fixed clock
> > > > frequency of 37.125MHz so I can't test other values.
> > > > 
> > > > Can we start with this and update it later if we can figure out more
> > > > (assuming there's an issue, it may actually be correct already) ?
> > > 
> > > Sounds reasonable. I was just wondering.
> > > 
> > > > > > +	 * convert it to a number of pixels based on the nominal pixel rate.
> > > > > > +	 */
> > > > > > +	hblank = 1100 * 1188000000ULL / 10 / 74250000
> > > > > > +	       - IMX296_PIXEL_ARRAY_WIDTH;
> > > > > > +	sensor->hblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > > > > +					   V4L2_CID_HBLANK, hblank, hblank, 1,
> > > > > > +					   hblank);
> > > > > > +	if (sensor->hblank)
> > > > > > +		sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > +
> > > > > > +	sensor->vblank = v4l2_ctrl_new_std(&sensor->ctrls, &imx296_ctrl_ops,
> > > > > > +					   V4L2_CID_VBLANK, 30,
> > > > > > +					   1048575 - IMX296_PIXEL_ARRAY_HEIGHT,
> > > > > > +					   1, 30);
> > > > > > +	/*
> > > > > > +	 * The sensor calculates the MIPI timings internally to achieve a bit
> > > > > > +	 * rate between 1122 and 1198 Mbps. The exact value is unfortunately not
> > > > > > +	 * reported, at least according to the documentation. Report a nominal
> > > > > > +	 * rate of 1188 Mbps as that is used by the datasheet in multiple
> > > > > > +	 * examples.
> > > > > > +	 */
> > > > > > +	v4l2_ctrl_new_std(&sensor->ctrls, NULL, V4L2_CID_PIXEL_RATE,
> > > > > > +			  1122000000 / 10, 1198000000 / 10, 1, 1188000000 / 10);
> > > > > 
> > > > > What about the link frequency?
> > > > > 
> > > > > Is this value constant for the sensor? Or should there be a list of
> > > > > hardware supported link frequencies?
> > > > 
> > > > It seems to be constant, but again the documentation is fairly unclear.
> > > 
> > > Ack.
> > > 
> > > ...
> > > 
> > > > > > +static int __maybe_unused imx296_runtime_resume(struct device *dev)
> > > > > > +{
> > > > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > > > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > > > > +	struct imx296 *sensor = to_imx296(subdev);
> > > > > > +
> > > > > > +	return imx296_power_on(sensor);
> > > > > > +}
> > > > > > +
> > > > > > +static int __maybe_unused imx296_runtime_suspend(struct device *dev)
> > > > > > +{
> > > > > > +	struct i2c_client *client = to_i2c_client(dev);
> > > > > > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > > > > > +	struct imx296 *sensor = to_imx296(subdev);
> > > > > > +
> > > > > > +	imx296_power_off(sensor);
> > > > > > +
> > > > > > +	return 0;
> > > > > 
> > > > > I'd merge these two with imx296_power_o{n,ff}.
> > > > 
> > > > That would require calling imx296_runtime_resume() and
> > > > imx296_runtime_suspend() in probe() and remove(), which I don't really
> > > > like. I'd prefer keeping the functions separate.
> > > 
> > > You could keep calling the functions imx296_power_o{n,ff}. There's really
> > > no need for two pairs of functions doing the same things.
> > 
> > imx296_power_on() is called in probe() before the subdev is initialized,
> > so the i2c_get_clientdata() call in imx296_runtime_resume() would fail.
> > It may be possible to refactor the probe() function to fix this, but I
> > think that explicit power on/off calls in probe() are clearer than
> > calling the pm runtime resume and suspend handlers.
> 
> They're doing exactly the same things, powering the device on and off. The
> fact that it's used as a runtime PM callback as well as called directly
> doesn't change its nature.
> 
> Most other drivers do it this way actually. What it'd take is a call to
> v4l2_i2c_subdev_init() before powering the sensor on.

I still really dislike it, sorry. I thought I could find a middle ground
by using the runtime PM functions to power up the sensor in probe(), but
that won't work if runtime PM is disabled.

It's fairly common for drivers to have internal functions called by
wrappers whose only purpose is to convert the arguments, I don't see why
it would be a problem here. I think it's actually cleaner and conveys
the purpose better.