Re: [PATCH 1/2] media: i2c: Add ON Semiconductor AP1302 ISP driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sakari,

On Fri, Jun 04, 2021 at 12:23:28AM +0300, Sakari Ailus wrote:
> Hi Anil, Laurent,
> 
> Thanks for the set. Also my apologies for reviewing it so late...
> 
> Some trivial comments but some questions, too, below.
> 
> Has the firmware been submitted for the linux-firmware repository?

It hasn't, partly because there will be one firmware per sensor
connected to the AP1302, and we're trying to figure out how to best
provide them.

> On Mon, Mar 22, 2021 at 10:36:32PM +0530, Anil Kumar Mamidala wrote:
> > The AP1302 is a standalone ISP for ON Semiconductor sensors.
> > AP1302 ISP supports single and dual sensor inputs. The driver
> > code supports AR1335, AR0144 and AR0330 sensors with single and
> > dual mode by loading the corresponding firmware.
> > 
> > Signed-off-by: Anil Kumar Mamidala <anil.mamidala@xxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Stefan Hladnik <stefan.hladnik@xxxxxxxxx>
> > Signed-off-by: Florian Rebaudo <frebaudo@xxxxxxxxxxx>
> > ---
> >  MAINTAINERS                |    9 +
> >  drivers/media/i2c/Kconfig  |   12 +
> >  drivers/media/i2c/Makefile |    1 +
> >  drivers/media/i2c/ap1302.c | 2632 ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 2654 insertions(+)
> >  create mode 100644 drivers/media/i2c/ap1302.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 16ada1a..2017614 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1206,6 +1206,15 @@ L:	alsa-devel@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
> >  S:	Maintained
> >  F:	sound/aoa/
> >  
> > +AP1302 ON SEMICONDUCTOR CAMERA ISP
> > +M:	Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > +M:	Anil Kumar Mamidala <anil.mamidala@xxxxxxxxxx>
> > +L:	linux-media@xxxxxxxxxxxxxxx
> > +S:	Maintained
> > +T:	git git://linuxtv.org/media_tree.git
> > +F:	Documentation/devicetree/bindings/media/i2c/onnn,ap1302.yaml
> > +F:	drivers/media/i2c/ap1302.c
> > +
> >  APEX EMBEDDED SYSTEMS STX104 IIO DRIVER
> >  M:	William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> >  L:	linux-iio@xxxxxxxxxxxxxxx
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 462c0e0..b8f671f 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -627,6 +627,18 @@ config VIDEO_UPD64083
> >  
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called upd64083.
> > +
> > +config VIDEO_AP1302
> > +	tristate "ON Semiconductor AP1302 support"
> > +	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> > +	select V4L2_FWNODE
> 
> You'll need to update this (things have changed since).
> 
> > +	help
> > +	  This is a Video4Linux2 sensor-level driver for the ON Semiconductor
> > +	  AP1302 ISP.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called ap1302.
> > +
> >  endmenu
> >  
> >  menu "Audio/Video compression chips"
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 0c067be..8c74024 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -64,6 +64,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
> >  obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
> >  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
> >  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
> > +obj-$(CONFIG_VIDEO_AP1302) += ap1302.o
> >  obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
> >  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
> >  obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
> > diff --git a/drivers/media/i2c/ap1302.c b/drivers/media/i2c/ap1302.c
> > new file mode 100644
> > index 0000000..271ec8f
> > --- /dev/null
> > +++ b/drivers/media/i2c/ap1302.c
> > @@ -0,0 +1,2632 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Driver for the AP1302 external camera ISP from ON Semiconductor
> > + *
> > + * Copyright (C) 2021, Witekio, Inc.
> > + * Copyright (C) 2021, Xilinx, Inc.
> > + * Copyright (C) 2021, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/firmware.h>
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/media.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
> > +
> > +#define DRIVER_NAME "ap1302"
> > +
> > +#define AP1302_FW_WINDOW_SIZE			0x2000
> > +#define AP1302_FW_WINDOW_OFFSET			0x8000
> > +
> > +#define AP1302_MIN_WIDTH			24U
> > +#define AP1302_MIN_HEIGHT			16U
> > +#define AP1302_MAX_WIDTH			4224U
> > +#define AP1302_MAX_HEIGHT			4092U
> > +
> > +#define AP1302_REG_16BIT(n)			((2 << 24) | (n))
> > +#define AP1302_REG_32BIT(n)			((4 << 24) | (n))
> > +#define AP1302_REG_SIZE(n)			((n) >> 24)
> > +#define AP1302_REG_ADDR(n)			((n) & 0x0000ffff)
> > +#define AP1302_REG_PAGE(n)			((n) & 0x00ff0000)
> > +#define AP1302_REG_PAGE_MASK			0x00ff0000
> > +
> > +/* Info Registers */
> > +#define AP1302_CHIP_VERSION			AP1302_REG_16BIT(0x0000)
> > +#define AP1302_CHIP_ID				0x0265
> > +#define AP1302_FRAME_CNT			AP1302_REG_16BIT(0x0002)
> > +#define AP1302_ERROR				AP1302_REG_16BIT(0x0006)
> > +#define AP1302_ERR_FILE				AP1302_REG_32BIT(0x0008)
> > +#define AP1302_ERR_LINE				AP1302_REG_16BIT(0x000c)
> > +#define AP1302_SIPM_ERR_0			AP1302_REG_16BIT(0x0014)
> > +#define AP1302_SIPM_ERR_1			AP1302_REG_16BIT(0x0016)
> > +#define AP1302_CHIP_REV				AP1302_REG_16BIT(0x0050)
> > +#define AP1302_CON_BUF(n)			AP1302_REG_16BIT(0x0a2c + (n))
> > +#define AP1302_CON_BUF_SIZE			512
> > +
> > +/* Control Registers */
> > +#define AP1302_DZ_TGT_FCT			AP1302_REG_16BIT(0x1010)
> > +#define AP1302_SFX_MODE				AP1302_REG_16BIT(0x1016)
> > +#define AP1302_SFX_MODE_SFX_NORMAL		(0U << 0)
> > +#define AP1302_SFX_MODE_SFX_ALIEN		(1U << 0)
> > +#define AP1302_SFX_MODE_SFX_ANTIQUE		(2U << 0)
> > +#define AP1302_SFX_MODE_SFX_BW			(3U << 0)
> > +#define AP1302_SFX_MODE_SFX_EMBOSS		(4U << 0)
> > +#define AP1302_SFX_MODE_SFX_EMBOSS_COLORED	(5U << 0)
> > +#define AP1302_SFX_MODE_SFX_GRAYSCALE		(6U << 0)
> > +#define AP1302_SFX_MODE_SFX_NEGATIVE		(7U << 0)
> > +#define AP1302_SFX_MODE_SFX_BLUISH		(8U << 0)
> > +#define AP1302_SFX_MODE_SFX_GREENISH		(9U << 0)
> > +#define AP1302_SFX_MODE_SFX_REDISH		(10U << 0)
> > +#define AP1302_SFX_MODE_SFX_POSTERIZE1		(11U << 0)
> > +#define AP1302_SFX_MODE_SFX_POSTERIZE2		(12U << 0)
> > +#define AP1302_SFX_MODE_SFX_SEPIA1		(13U << 0)
> > +#define AP1302_SFX_MODE_SFX_SEPIA2		(14U << 0)
> > +#define AP1302_SFX_MODE_SFX_SKETCH		(15U << 0)
> > +#define AP1302_SFX_MODE_SFX_SOLARIZE		(16U << 0)
> > +#define AP1302_SFX_MODE_SFX_FOGGY		(17U << 0)
> > +#define AP1302_BUBBLE_OUT_FMT			AP1302_REG_16BIT(0x1164)
> > +#define AP1302_BUBBLE_OUT_FMT_FT_YUV		(3U << 4)
> > +#define AP1302_BUBBLE_OUT_FMT_FT_RGB		(4U << 4)
> > +#define AP1302_BUBBLE_OUT_FMT_FT_YUV_JFIF	(5U << 4)
> > +#define AP1302_BUBBLE_OUT_FMT_FST_RGB_888	(0U << 0)
> > +#define AP1302_BUBBLE_OUT_FMT_FST_RGB_565	(1U << 0)
> > +#define AP1302_BUBBLE_OUT_FMT_FST_RGB_555M	(2U << 0)
> > +#define AP1302_BUBBLE_OUT_FMT_FST_RGB_555L	(3U << 0)
> > +#define AP1302_BUBBLE_OUT_FMT_FST_YUV_422	(0U << 0)
> > +#define AP1302_BUBBLE_OUT_FMT_FST_YUV_420	(1U << 0)
> > +#define AP1302_BUBBLE_OUT_FMT_FST_YUV_400	(2U << 0)
> > +#define AP1302_ATOMIC				AP1302_REG_16BIT(0x1184)
> > +#define AP1302_ATOMIC_MODE			BIT(2)
> > +#define AP1302_ATOMIC_FINISH			BIT(1)
> > +#define AP1302_ATOMIC_RECORD			BIT(0)
> > +
> > +/*
> > + * Preview Context Registers (preview_*). AP1302 supports 3 "contexts"
> > + * (Preview, Snapshot, Video). These can be programmed for different size,
> > + * format, FPS, etc. There is no functional difference between the contexts,
> > + * so the only potential benefit of using them is reduced number of register
> > + * writes when switching output modes (if your concern is atomicity, see
> > + * "atomic" register).
> > + * So there's virtually no benefit in using contexts for this driver and it
> > + * would significantly increase complexity. Let's use preview context only.
> > + */
> > +#define AP1302_PREVIEW_WIDTH			AP1302_REG_16BIT(0x2000)
> > +#define AP1302_PREVIEW_HEIGHT			AP1302_REG_16BIT(0x2002)
> > +#define AP1302_PREVIEW_ROI_X0			AP1302_REG_16BIT(0x2004)
> > +#define AP1302_PREVIEW_ROI_Y0			AP1302_REG_16BIT(0x2006)
> > +#define AP1302_PREVIEW_ROI_X1			AP1302_REG_16BIT(0x2008)
> > +#define AP1302_PREVIEW_ROI_Y1			AP1302_REG_16BIT(0x200a)
> > +#define AP1302_PREVIEW_OUT_FMT			AP1302_REG_16BIT(0x2012)
> > +#define AP1302_PREVIEW_OUT_FMT_IPIPE_BYPASS	BIT(13)
> > +#define AP1302_PREVIEW_OUT_FMT_SS		BIT(12)
> > +#define AP1302_PREVIEW_OUT_FMT_FAKE_EN		BIT(11)
> > +#define AP1302_PREVIEW_OUT_FMT_ST_EN		BIT(10)
> > +#define AP1302_PREVIEW_OUT_FMT_IIS_NONE		(0U << 8)
> > +#define AP1302_PREVIEW_OUT_FMT_IIS_POST_VIEW	(1U << 8)
> > +#define AP1302_PREVIEW_OUT_FMT_IIS_VIDEO	(2U << 8)
> > +#define AP1302_PREVIEW_OUT_FMT_IIS_BUBBLE	(3U << 8)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_JPEG_422	(0U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_JPEG_420	(1U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_YUV		(3U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_RGB		(4U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_YUV_JFIF	(5U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_RAW8		(8U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_RAW10		(9U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_RAW12		(10U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_RAW16		(11U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_DNG8		(12U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_DNG10		(13U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_DNG12		(14U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FT_DNG16		(15U << 4)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_JPEG_ROTATE	BIT(2)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_JPEG_SCAN	(0U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_JPEG_JFIF	(1U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_JPEG_EXIF	(2U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RGB_888	(0U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RGB_565	(1U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RGB_555M	(2U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RGB_555L	(3U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_YUV_422	(0U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_YUV_420	(1U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_YUV_400	(2U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_SENSOR	(0U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_CAPTURE	(1U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_CP	(2U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_BPC	(3U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_IHDR	(4U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_PP	(5U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_DENSH	(6U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_PM	(7U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_GC	(8U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_CURVE	(9U << 0)
> > +#define AP1302_PREVIEW_OUT_FMT_FST_RAW_CCONV	(10U << 0)
> > +#define AP1302_PREVIEW_S1_SENSOR_MODE		AP1302_REG_16BIT(0x202e)
> > +#define AP1302_PREVIEW_HINF_CTRL		AP1302_REG_16BIT(0x2030)
> > +#define AP1302_PREVIEW_HINF_CTRL_BT656_LE	BIT(15)
> > +#define AP1302_PREVIEW_HINF_CTRL_BT656_16BIT	BIT(14)
> > +#define AP1302_PREVIEW_HINF_CTRL_MUX_DELAY(n)	((n) << 8)
> > +#define AP1302_PREVIEW_HINF_CTRL_LV_POL		BIT(7)
> > +#define AP1302_PREVIEW_HINF_CTRL_FV_POL		BIT(6)
> > +#define AP1302_PREVIEW_HINF_CTRL_MIPI_CONT_CLK	BIT(5)
> > +#define AP1302_PREVIEW_HINF_CTRL_SPOOF		BIT(4)
> > +#define AP1302_PREVIEW_HINF_CTRL_MIPI_MODE	BIT(3)
> > +#define AP1302_PREVIEW_HINF_CTRL_MIPI_LANES(n)	((n) << 0)
> > +
> > +/* IQ Registers */
> > +#define AP1302_AE_BV_OFF			AP1302_REG_16BIT(0x5014)
> > +#define AP1302_AWB_CTRL				AP1302_REG_16BIT(0x5100)
> > +#define AP1302_AWB_CTRL_RECALC			BIT(13)
> > +#define AP1302_AWB_CTRL_POSTGAIN		BIT(12)
> > +#define AP1302_AWB_CTRL_UNGAIN			BIT(11)
> > +#define AP1302_AWB_CTRL_CLIP			BIT(10)
> > +#define AP1302_AWB_CTRL_SKY			BIT(9)
> > +#define AP1302_AWB_CTRL_FLASH			BIT(8)
> > +#define AP1302_AWB_CTRL_FACE_OFF		(0U << 6)
> > +#define AP1302_AWB_CTRL_FACE_IGNORE		(1U << 6)
> > +#define AP1302_AWB_CTRL_FACE_CONSTRAINED	(2U << 6)
> > +#define AP1302_AWB_CTRL_FACE_ONLY		(3U << 6)
> > +#define AP1302_AWB_CTRL_IMM			BIT(5)
> > +#define AP1302_AWB_CTRL_IMM1			BIT(4)
> > +#define AP1302_AWB_CTRL_MODE_OFF		(0U << 0)
> > +#define AP1302_AWB_CTRL_MODE_HORIZON		(1U << 0)
> > +#define AP1302_AWB_CTRL_MODE_A			(2U << 0)
> > +#define AP1302_AWB_CTRL_MODE_CWF		(3U << 0)
> > +#define AP1302_AWB_CTRL_MODE_D50		(4U << 0)
> > +#define AP1302_AWB_CTRL_MODE_D65		(5U << 0)
> > +#define AP1302_AWB_CTRL_MODE_D75		(6U << 0)
> > +#define AP1302_AWB_CTRL_MODE_MANUAL		(7U << 0)
> > +#define AP1302_AWB_CTRL_MODE_MEASURE		(8U << 0)
> > +#define AP1302_AWB_CTRL_MODE_AUTO		(15U << 0)
> > +#define AP1302_AWB_CTRL_MODE_MASK		0x000f
> > +#define AP1302_FLICK_CTRL			AP1302_REG_16BIT(0x5440)
> > +#define AP1302_FLICK_CTRL_FREQ(n)		((n) << 8)
> > +#define AP1302_FLICK_CTRL_ETC_IHDR_UP		BIT(6)
> > +#define AP1302_FLICK_CTRL_ETC_DIS		BIT(5)
> > +#define AP1302_FLICK_CTRL_FRC_OVERRIDE_MAX_ET	BIT(4)
> > +#define AP1302_FLICK_CTRL_FRC_OVERRIDE_UPPER_ET	BIT(3)
> > +#define AP1302_FLICK_CTRL_FRC_EN		BIT(2)
> > +#define AP1302_FLICK_CTRL_MODE_DISABLED		(0U << 0)
> > +#define AP1302_FLICK_CTRL_MODE_MANUAL		(1U << 0)
> > +#define AP1302_FLICK_CTRL_MODE_AUTO		(2U << 0)
> > +#define AP1302_SCENE_CTRL			AP1302_REG_16BIT(0x5454)
> > +#define AP1302_SCENE_CTRL_MODE_NORMAL		(0U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_PORTRAIT		(1U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_LANDSCAPE	(2U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_SPORT		(3U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_CLOSE_UP		(4U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_NIGHT		(5U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_TWILIGHT		(6U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_BACKLIGHT	(7U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_HIGH_SENSITIVE	(8U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_NIGHT_PORTRAIT	(9U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_BEACH		(10U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_DOCUMENT		(11U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_PARTY		(12U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_FIREWORKS	(13U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_SUNSET		(14U << 0)
> > +#define AP1302_SCENE_CTRL_MODE_AUTO		(0xffU << 0)
> > +
> > +/* System Registers */
> > +#define AP1302_BOOTDATA_STAGE			AP1302_REG_16BIT(0x6002)
> > +#define AP1302_WARNING(n)			AP1302_REG_16BIT(0x6004 + (n) * 2)
> > +#define AP1302_SENSOR_SELECT			AP1302_REG_16BIT(0x600c)
> > +#define AP1302_SENSOR_SELECT_TP_MODE(n)		((n) << 8)
> > +#define AP1302_SENSOR_SELECT_PATTERN_ON		BIT(7)
> > +#define AP1302_SENSOR_SELECT_MODE_3D_ON		BIT(6)
> > +#define AP1302_SENSOR_SELECT_CLOCK		BIT(5)
> > +#define AP1302_SENSOR_SELECT_SINF_MIPI		BIT(4)
> > +#define AP1302_SENSOR_SELECT_YUV		BIT(2)
> > +#define AP1302_SENSOR_SELECT_SENSOR_TP		(0U << 0)
> > +#define AP1302_SENSOR_SELECT_SENSOR(n)		(((n) + 1) << 0)
> > +#define AP1302_SYS_START			AP1302_REG_16BIT(0x601a)
> > +#define AP1302_SYS_START_PLL_LOCK		BIT(15)
> > +#define AP1302_SYS_START_LOAD_OTP		BIT(12)
> > +#define AP1302_SYS_START_RESTART_ERROR		BIT(11)
> > +#define AP1302_SYS_START_STALL_STATUS		BIT(9)
> > +#define AP1302_SYS_START_STALL_EN		BIT(8)
> > +#define AP1302_SYS_START_STALL_MODE_FRAME	(0U << 6)
> > +#define AP1302_SYS_START_STALL_MODE_DISABLED	(1U << 6)
> > +#define AP1302_SYS_START_STALL_MODE_POWER_DOWN	(2U << 6)
> > +#define AP1302_SYS_START_GO			BIT(4)
> > +#define AP1302_SYS_START_PATCH_FUN		BIT(1)
> > +#define AP1302_SYS_START_PLL_INIT		BIT(0)
> > +#define AP1302_DMA_SRC				AP1302_REG_32BIT(0x60a0)
> > +#define AP1302_DMA_DST				AP1302_REG_32BIT(0x60a4)
> > +#define AP1302_DMA_SIP_SIPM(n)			((n) << 26)
> > +#define AP1302_DMA_SIP_DATA_16_BIT		BIT(25)
> > +#define AP1302_DMA_SIP_ADDR_16_BIT		BIT(24)
> > +#define AP1302_DMA_SIP_ID(n)			((n) << 17)
> > +#define AP1302_DMA_SIP_REG(n)			((n) << 0)
> > +#define AP1302_DMA_SIZE				AP1302_REG_32BIT(0x60a8)
> > +#define AP1302_DMA_CTRL				AP1302_REG_16BIT(0x60ac)
> > +#define AP1302_DMA_CTRL_SCH_NORMAL		(0 << 12)
> > +#define AP1302_DMA_CTRL_SCH_NEXT		(1 << 12)
> > +#define AP1302_DMA_CTRL_SCH_NOW			(2 << 12)
> > +#define AP1302_DMA_CTRL_DST_REG			(0 << 8)
> > +#define AP1302_DMA_CTRL_DST_SRAM		(1 << 8)
> > +#define AP1302_DMA_CTRL_DST_SPI			(2 << 8)
> > +#define AP1302_DMA_CTRL_DST_SIP			(3 << 8)
> > +#define AP1302_DMA_CTRL_SRC_REG			(0 << 4)
> > +#define AP1302_DMA_CTRL_SRC_SRAM		(1 << 4)
> > +#define AP1302_DMA_CTRL_SRC_SPI			(2 << 4)
> > +#define AP1302_DMA_CTRL_SRC_SIP			(3 << 4)
> > +#define AP1302_DMA_CTRL_MODE_32_BIT		BIT(3)
> > +#define AP1302_DMA_CTRL_MODE_MASK		(7 << 0)
> > +#define AP1302_DMA_CTRL_MODE_IDLE		(0 << 0)
> > +#define AP1302_DMA_CTRL_MODE_SET		(1 << 0)
> > +#define AP1302_DMA_CTRL_MODE_COPY		(2 << 0)
> > +#define AP1302_DMA_CTRL_MODE_MAP		(3 << 0)
> > +#define AP1302_DMA_CTRL_MODE_UNPACK		(4 << 0)
> > +#define AP1302_DMA_CTRL_MODE_OTP_READ		(5 << 0)
> > +#define AP1302_DMA_CTRL_MODE_SIP_PROBE		(6 << 0)
> > +
> > +/* Misc Registers */
> > +#define AP1302_REG_ADV_START			0xe000
> > +#define AP1302_ADVANCED_BASE			AP1302_REG_32BIT(0xf038)
> > +#define AP1302_SIP_CRC				AP1302_REG_16BIT(0xf052)
> > +
> > +/* Advanced System Registers */
> > +#define AP1302_ADV_IRQ_SYS_INTE			AP1302_REG_32BIT(0x00230000)
> > +#define AP1302_ADV_IRQ_SYS_INTE_TEST_COUNT	BIT(25)
> > +#define AP1302_ADV_IRQ_SYS_INTE_HINF_1		BIT(24)
> > +#define AP1302_ADV_IRQ_SYS_INTE_HINF_0		BIT(23)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SINF_B_MIPI_L	(7U << 20)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SINF_B_MIPI	BIT(19)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SINF_A_MIPI_L	(15U << 14)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SINF_A_MIPI	BIT(13)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SINF		BIT(12)
> > +#define AP1302_ADV_IRQ_SYS_INTE_IPIPE_S		BIT(11)
> > +#define AP1302_ADV_IRQ_SYS_INTE_IPIPE_B		BIT(10)
> > +#define AP1302_ADV_IRQ_SYS_INTE_IPIPE_A		BIT(9)
> > +#define AP1302_ADV_IRQ_SYS_INTE_IP		BIT(8)
> > +#define AP1302_ADV_IRQ_SYS_INTE_TIMER		BIT(7)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SIPM		(3U << 6)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SIPS_ADR_RANGE	BIT(5)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SIPS_DIRECT_WRITE	BIT(4)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SIPS_FIFO_WRITE	BIT(3)
> > +#define AP1302_ADV_IRQ_SYS_INTE_SPI		BIT(2)
> > +#define AP1302_ADV_IRQ_SYS_INTE_GPIO_CNT	BIT(1)
> > +#define AP1302_ADV_IRQ_SYS_INTE_GPIO_PIN	BIT(0)
> > +
> > +/* Advanced Slave MIPI Registers */
> > +#define AP1302_ADV_SINF_MIPI_INTERNAL_p_LANE_n_STAT(p, n) \
> > +	AP1302_REG_32BIT(0x00420008 + (p) * 0x50000 + (n) * 0x20)
> > +#define AP1302_LANE_ERR_LP_VAL(n)		(((n) >> 30) & 3)
> > +#define AP1302_LANE_ERR_STATE(n)		(((n) >> 24) & 0xf)
> > +#define AP1302_LANE_ERR				BIT(18)
> > +#define AP1302_LANE_ABORT			BIT(17)
> > +#define AP1302_LANE_LP_VAL(n)			(((n) >> 6) & 3)
> > +#define AP1302_LANE_STATE(n)			((n) & 0xf)
> > +#define AP1302_LANE_STATE_STOP_S		0x0
> > +#define AP1302_LANE_STATE_HS_REQ_S		0x1
> > +#define AP1302_LANE_STATE_LP_REQ_S		0x2
> > +#define AP1302_LANE_STATE_HS_S			0x3
> > +#define AP1302_LANE_STATE_LP_S			0x4
> > +#define AP1302_LANE_STATE_ESC_REQ_S		0x5
> > +#define AP1302_LANE_STATE_TURN_REQ_S		0x6
> > +#define AP1302_LANE_STATE_ESC_S			0x7
> > +#define AP1302_LANE_STATE_ESC_0			0x8
> > +#define AP1302_LANE_STATE_ESC_1			0x9
> > +#define AP1302_LANE_STATE_TURN_S		0xa
> > +#define AP1302_LANE_STATE_TURN_MARK		0xb
> > +#define AP1302_LANE_STATE_ERROR_S		0xc
> > +
> > +#define AP1302_ADV_CAPTURE_A_FV_CNT		AP1302_REG_32BIT(0x00490040)
> > +
> > +struct ap1302_device;
> > +
> > +enum {
> > +	AP1302_PAD_SINK_0,
> > +	AP1302_PAD_SINK_1,
> > +	AP1302_PAD_SOURCE,
> > +	AP1302_PAD_MAX,
> > +};
> > +
> > +struct ap1302_format_info {
> > +	unsigned int code;
> > +	u16 out_fmt;
> > +};
> > +
> > +struct ap1302_format {
> > +	struct v4l2_mbus_framefmt format;
> > +	const struct ap1302_format_info *info;
> > +};
> > +
> > +struct ap1302_size {
> > +	unsigned int width;
> > +	unsigned int height;
> > +};
> > +
> > +struct ap1302_sensor_supply {
> > +	const char *name;
> > +	unsigned int post_delay_us;
> > +};
> > +
> > +struct ap1302_sensor_info {
> > +	const char *model;
> > +	const char *name;
> > +	unsigned int i2c_addr;
> > +	struct ap1302_size resolution;
> > +	u32 format;
> > +	const struct ap1302_sensor_supply *supplies;
> > +};
> > +
> > +struct ap1302_sensor {
> > +	struct ap1302_device *ap1302;
> > +	unsigned int index;
> > +
> > +	struct device_node *of_node;
> > +	struct device *dev;
> > +	unsigned int num_supplies;
> > +	struct regulator_bulk_data *supplies;
> > +
> > +	struct v4l2_subdev sd;
> > +	struct media_pad pad;
> > +};
> > +
> > +static inline struct ap1302_sensor *to_ap1302_sensor(struct v4l2_subdev *sd)
> > +{
> > +	return container_of(sd, struct ap1302_sensor, sd);
> > +}
> > +
> > +struct ap1302_device {
> > +	struct device *dev;
> > +	struct i2c_client *client;
> > +
> > +	struct gpio_desc *reset_gpio;
> > +	struct gpio_desc *standby_gpio;
> > +	struct clk *clock;
> > +	struct regmap *regmap16;
> > +	struct regmap *regmap32;
> > +	u32 reg_page;
> > +
> > +	const struct firmware *fw;
> > +
> > +	struct v4l2_fwnode_endpoint bus_cfg;
> > +
> > +	struct mutex lock;	/* Protects formats */
> > +
> > +	struct v4l2_subdev sd;
> > +	struct media_pad pads[AP1302_PAD_MAX];
> > +	struct ap1302_format formats[AP1302_PAD_MAX];
> > +	unsigned int width_factor;
> > +
> > +	struct v4l2_ctrl_handler ctrls;
> > +
> > +	const struct ap1302_sensor_info *sensor_info;
> > +	struct ap1302_sensor sensors[2];
> > +
> > +	struct {
> > +		struct dentry *dir;
> > +		struct mutex lock;
> > +		u32 sipm_addr;
> > +	} debugfs;
> > +};
> > +
> > +static inline struct ap1302_device *to_ap1302(struct v4l2_subdev *sd)
> > +{
> > +	return container_of(sd, struct ap1302_device, sd);
> > +}
> > +
> > +struct ap1302_firmware_header {
> > +	u16 pll_init_size;
> > +	u16 crc;
> > +} __packed;
> > +
> > +#define MAX_FW_LOAD_RETRIES 3
> 
> I'd just move this to where the retrying happens.
> 
> > +
> > +static const struct ap1302_format_info supported_video_formats[] = {
> > +	{
> > +		.code = MEDIA_BUS_FMT_UYVY8_1X16,
> > +		.out_fmt = AP1302_PREVIEW_OUT_FMT_FT_YUV_JFIF
> > +			 | AP1302_PREVIEW_OUT_FMT_FST_YUV_422,
> > +	}, {
> > +		.code = MEDIA_BUS_FMT_UYYVYY8_0_5X24,
> > +		.out_fmt = AP1302_PREVIEW_OUT_FMT_FT_YUV_JFIF
> > +			 | AP1302_PREVIEW_OUT_FMT_FST_YUV_420,
> > +	},
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Sensor Info
> > + */
> > +
> > +static const struct ap1302_sensor_info ap1302_sensor_info[] = {
> > +	{
> > +		.model = "onnn,ar0144",
> > +		.name = "ar0144",
> > +		.i2c_addr = 0x10,
> 
> Is this also known to the firmware? Or should it be in DT? Just
> wondering...

It's known to the firmware, the address is fixed (in theory it could be
modified, and the firmware could be informed, but that would be a bit
pointless as the sensor is on a dedicated I2C bus).

> Does the firmware control both of the sensors simultaneously?

The AP1302 has a dedicated I2C master port for each sensor, although one
could connect both to the same port if a corresponding firmware was
loaded. I'm not aware of this option being supported.

> It might be configurable on sensors' side but OTOH it might be quite a
> useless feature here.
> 
> > +		.resolution = { 1280, 800 },
> > +		.format = MEDIA_BUS_FMT_SGRBG12_1X12,
> > +		.supplies = (const struct ap1302_sensor_supply[]) {
> > +			{ "vaa", 0 },
> > +			{ "vddio", 0 },
> > +			{ "vdd", 0 },
> > +			{ NULL, 0 },
> > +		},
> > +	}, {
> > +		.model = "onnn,ar0330",
> > +		.name = "ar0330",
> > +		.i2c_addr = 0x10,
> > +		.resolution = { 2304, 1536 },
> > +		.format = MEDIA_BUS_FMT_SGRBG12_1X12,
> > +		.supplies = (const struct ap1302_sensor_supply[]) {
> > +			{ "vddpll", 0 },
> > +			{ "vaa", 0 },
> > +			{ "vdd", 0 },
> > +			{ "vddio", 0 },
> > +			{ NULL, 0 },
> > +		},
> > +	}, {
> > +		.model = "onnn,ar1335",
> > +		.name = "ar1335",
> > +		.i2c_addr = 0x36,
> > +		.resolution = { 4208, 3120 },
> > +		.format = MEDIA_BUS_FMT_SGRBG10_1X10,
> > +		.supplies = (const struct ap1302_sensor_supply[]) {
> > +			{ "vaa", 0 },
> > +			{ "vddio", 0 },
> > +			{ "vdd", 0 },
> > +			{ NULL, 0 },
> > +		},
> > +	},
> > +};
> > +
> > +static const struct ap1302_sensor_info ap1302_sensor_info_tpg = {
> > +	.model = "",
> > +	.name = "tpg",
> > +	.resolution = { 1920, 1080 },
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Register Configuration
> > + */
> 
> /*
>  * Multi-line
>  * comment
>  */

This one is a common pattern in many drivers though (albeit it probably
comes from me in most cases :-)).

> > +
> > +static const struct regmap_config ap1302_reg16_config = {
> > +	.reg_bits = 16,
> > +	.val_bits = 16,
> > +	.reg_stride = 2,
> > +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> > +	.val_format_endian = REGMAP_ENDIAN_BIG,
> > +	.cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static const struct regmap_config ap1302_reg32_config = {
> > +	.reg_bits = 16,
> > +	.val_bits = 32,
> > +	.reg_stride = 4,
> > +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> > +	.val_format_endian = REGMAP_ENDIAN_BIG,
> > +	.cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static int __ap1302_write(struct ap1302_device *ap1302, u32 reg, u32 val)
> > +{
> > +	unsigned int size = AP1302_REG_SIZE(reg);
> > +	u16 addr = AP1302_REG_ADDR(reg);
> > +	int ret;
> > +
> > +	switch (size) {
> > +	case 2:
> > +		ret = regmap_write(ap1302->regmap16, addr, val);
> > +		break;
> > +	case 4:
> > +		ret = regmap_write(ap1302->regmap32, addr, val);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ret) {
> > +		dev_err(ap1302->dev, "%s: register 0x%04x %s failed: %d\n",
> > +			__func__, addr, "write", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_write(struct ap1302_device *ap1302, u32 reg, u32 val,
> > +			int *err)
> > +{
> > +	u32 page = AP1302_REG_PAGE(reg);
> > +	int ret;
> > +
> > +	if (err && *err)
> > +		return *err;
> > +
> > +	if (page) {
> > +		if (ap1302->reg_page != page) {
> > +			ret = __ap1302_write(ap1302, AP1302_ADVANCED_BASE,
> > +					     page);
> > +			if (ret < 0)
> > +				goto done;
> > +
> > +			ap1302->reg_page = page;
> > +		}
> > +
> > +		reg &= ~AP1302_REG_PAGE_MASK;
> > +		reg += AP1302_REG_ADV_START;
> > +	}
> > +
> > +	ret = __ap1302_write(ap1302, reg, val);
> > +
> > +done:
> > +	if (err && ret)
> > +		*err = ret;
> > +
> > +	return ret;
> > +}
> > +
> > +static int __ap1302_read(struct ap1302_device *ap1302, u32 reg, u32 *val)
> > +{
> > +	unsigned int size = AP1302_REG_SIZE(reg);
> > +	u16 addr = AP1302_REG_ADDR(reg);
> > +	int ret;
> > +
> > +	switch (size) {
> > +	case 2:
> > +		ret = regmap_read(ap1302->regmap16, addr, val);
> > +		break;
> > +	case 4:
> > +		ret = regmap_read(ap1302->regmap32, addr, val);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ret) {
> > +		dev_err(ap1302->dev, "%s: register 0x%04x %s failed: %d\n",
> > +			__func__, addr, "read", ret);
> > +		return ret;
> > +	}
> > +
> > +	dev_dbg(ap1302->dev, "%s: R0x%04x = 0x%0*x\n", __func__,
> > +		addr, size * 2, *val);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_read(struct ap1302_device *ap1302, u32 reg, u32 *val)
> > +{
> > +	u32 page = AP1302_REG_PAGE(reg);
> > +	int ret;
> > +
> > +	if (page) {
> > +		if (ap1302->reg_page != page) {
> > +			ret = __ap1302_write(ap1302, AP1302_ADVANCED_BASE,
> > +					     page);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			ap1302->reg_page = page;
> > +		}
> > +
> > +		reg &= ~AP1302_REG_PAGE_MASK;
> > +		reg += AP1302_REG_ADV_START;
> > +	}
> > +
> > +	return __ap1302_read(ap1302, reg, val);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Sensor Registers Access
> > + *
> > + * Read and write sensor registers through the AP1302 DMA interface.
> > + */
> > +
> > +static int ap1302_dma_wait_idle(struct ap1302_device *ap1302)
> > +{
> > +	unsigned int i;
> > +	u32 ctrl;
> > +	int ret;
> > +
> > +	for (i = 50; i > 0; i--) {
> > +		ret = ap1302_read(ap1302, AP1302_DMA_CTRL, &ctrl);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if ((ctrl & AP1302_DMA_CTRL_MODE_MASK) ==
> > +		    AP1302_DMA_CTRL_MODE_IDLE)
> > +			break;
> > +
> > +		usleep_range(1000, 1500);
> > +	}
> > +
> > +	if (!i) {
> > +		dev_err(ap1302->dev, "DMA timeout\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_sipm_read(struct ap1302_device *ap1302, unsigned int port,
> > +			    u32 reg, u32 *val)
> > +{
> > +	unsigned int size = AP1302_REG_SIZE(reg);
> > +	u32 src;
> > +	int ret;
> > +
> > +	if (size > 2)
> > +		return -EINVAL;
> > +
> > +	ret = ap1302_dma_wait_idle(ap1302);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ap1302_write(ap1302, AP1302_DMA_SIZE, size, &ret);
> > +	src = AP1302_DMA_SIP_SIPM(port)
> > +	    | (size == 2 ? AP1302_DMA_SIP_DATA_16_BIT : 0)
> > +	    | AP1302_DMA_SIP_ADDR_16_BIT
> > +	    | AP1302_DMA_SIP_ID(ap1302->sensor_info->i2c_addr)
> > +	    | AP1302_DMA_SIP_REG(AP1302_REG_ADDR(reg));
> > +	ap1302_write(ap1302, AP1302_DMA_SRC, src, &ret);
> > +
> > +	/*
> > +	 * Use the AP1302_DMA_DST register as both the destination address, and
> > +	 * the scratch pad to store the read value.
> > +	 */
> > +	ap1302_write(ap1302, AP1302_DMA_DST, AP1302_REG_ADDR(AP1302_DMA_DST),
> > +		     &ret);
> > +
> > +	ap1302_write(ap1302, AP1302_DMA_CTRL,
> > +		     AP1302_DMA_CTRL_SCH_NORMAL |
> > +		     AP1302_DMA_CTRL_DST_REG |
> > +		     AP1302_DMA_CTRL_SRC_SIP |
> > +		     AP1302_DMA_CTRL_MODE_COPY, &ret);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ap1302_dma_wait_idle(ap1302);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_DMA_DST, val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/*
> > +	 * The value is stored in big-endian at the DMA_DST address. The regmap
> > +	 * uses big-endian, so 8-bit values are stored in bits 31:24 and 16-bit
> > +	 * values in bits 23:16.
> > +	 */
> > +	*val >>= 32 - size * 8;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_sipm_write(struct ap1302_device *ap1302, unsigned int port,
> > +			     u32 reg, u32 val)
> > +{
> > +	unsigned int size = AP1302_REG_SIZE(reg);
> > +	u32 dst;
> > +	int ret;
> > +
> > +	if (size > 2)
> > +		return -EINVAL;
> > +
> > +	ret = ap1302_dma_wait_idle(ap1302);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ap1302_write(ap1302, AP1302_DMA_SIZE, size, &ret);
> > +
> > +	/*
> > +	 * Use the AP1302_DMA_SRC register as both the source address, and the
> > +	 * scratch pad to store the write value.
> > +	 *
> > +	 * As the AP1302 uses big endian, to store the value at address DMA_SRC
> > +	 * it must be written in the high order bits of the registers. However,
> > +	 * 8-bit values seem to be incorrectly handled by the AP1302, which
> > +	 * expects them to be stored at DMA_SRC + 1 instead of DMA_SRC. The
> > +	 * value is thus unconditionally shifted by 16 bits, unlike for DMA
> > +	 * reads.
> > +	 */
> > +	ap1302_write(ap1302, AP1302_DMA_SRC,
> > +		     (val << 16) | AP1302_REG_ADDR(AP1302_DMA_SRC), &ret);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dst = AP1302_DMA_SIP_SIPM(port)
> > +	    | (size == 2 ? AP1302_DMA_SIP_DATA_16_BIT : 0)
> > +	    | AP1302_DMA_SIP_ADDR_16_BIT
> > +	    | AP1302_DMA_SIP_ID(ap1302->sensor_info->i2c_addr)
> > +	    | AP1302_DMA_SIP_REG(AP1302_REG_ADDR(reg));
> > +	ap1302_write(ap1302, AP1302_DMA_DST, dst, &ret);
> > +
> > +	ap1302_write(ap1302, AP1302_DMA_CTRL,
> > +		     AP1302_DMA_CTRL_SCH_NORMAL |
> > +		     AP1302_DMA_CTRL_DST_SIP |
> > +		     AP1302_DMA_CTRL_SRC_REG |
> > +		     AP1302_DMA_CTRL_MODE_COPY, &ret);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ap1302_dma_wait_idle(ap1302);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Debugfs
> > + */
> > +
> > +static int ap1302_sipm_addr_get(void *arg, u64 *val)
> > +{
> > +	struct ap1302_device *ap1302 = arg;
> > +
> > +	mutex_lock(&ap1302->debugfs.lock);
> > +	*val = ap1302->debugfs.sipm_addr;
> > +	mutex_unlock(&ap1302->debugfs.lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_sipm_addr_set(void *arg, u64 val)
> > +{
> > +	struct ap1302_device *ap1302 = arg;
> > +
> > +	if (val & ~0x8700ffff)
> > +		return -EINVAL;
> > +
> > +	switch ((val >> 24) & 7) {
> > +	case 1:
> > +	case 2:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	mutex_lock(&ap1302->debugfs.lock);
> > +	ap1302->debugfs.sipm_addr = val;
> > +	mutex_unlock(&ap1302->debugfs.lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_sipm_data_get(void *arg, u64 *val)
> > +{
> > +	struct ap1302_device *ap1302 = arg;
> > +	u32 value;
> > +	u32 addr;
> > +	int ret;
> > +
> > +	mutex_lock(&ap1302->debugfs.lock);
> > +
> > +	addr = ap1302->debugfs.sipm_addr;
> > +	if (!addr) {
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	ret = ap1302_sipm_read(ap1302, addr >> 30, addr & ~BIT(31),
> > +			       &value);
> > +	if (!ret)
> > +		*val = value;
> > +
> > +unlock:
> > +	mutex_unlock(&ap1302->debugfs.lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ap1302_sipm_data_set(void *arg, u64 val)
> > +{
> > +	struct ap1302_device *ap1302 = arg;
> > +	u32 addr;
> > +	int ret;
> > +
> > +	mutex_lock(&ap1302->debugfs.lock);
> > +
> > +	addr = ap1302->debugfs.sipm_addr;
> > +	if (!addr) {
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	ret = ap1302_sipm_write(ap1302, addr >> 30, addr & ~BIT(31),
> > +				val);
> > +
> > +unlock:
> > +	mutex_unlock(&ap1302->debugfs.lock);
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * The sipm_addr and sipm_data attributes expose access to the sensor I2C bus.
> > + *
> > + * To read or write a register, sipm_addr has to first be written with the
> > + * register address. The address is a 32-bit integer formatted as follows.
> > + *
> > + * I000 0SSS 0000 0000 RRRR RRRR RRRR RRRR
> > + *
> > + * I: SIPM index (0 or 1)
> > + * S: Size (1: 8-bit, 2: 16-bit)
> > + * R: Register address (16-bit)
> > + *
> > + * The sipm_data attribute can then be read to read the register value, or
> > + * written to write it.
> > + */
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ap1302_sipm_addr_fops, ap1302_sipm_addr_get,
> > +			 ap1302_sipm_addr_set, "0x%08llx\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(ap1302_sipm_data_fops, ap1302_sipm_data_get,
> > +			 ap1302_sipm_data_set, "0x%08llx\n");
> > +
> > +static void ap1302_debugfs_init(struct ap1302_device *ap1302)
> > +{
> > +	struct dentry *dir;
> > +	char name[16];
> > +
> > +	mutex_init(&ap1302->debugfs.lock);
> > +
> > +	snprintf(name, sizeof(name), "ap1302.%s", dev_name(ap1302->dev));
> > +
> > +	dir = debugfs_create_dir(name, NULL);
> > +	if (IS_ERR(dir))
> > +		return;
> > +
> > +	ap1302->debugfs.dir = dir;
> > +
> > +	debugfs_create_file_unsafe("sipm_addr", 0600, ap1302->debugfs.dir,
> > +				   ap1302, &ap1302_sipm_addr_fops);
> > +	debugfs_create_file_unsafe("sipm_data", 0600, ap1302->debugfs.dir,
> > +				   ap1302, &ap1302_sipm_data_fops);
> > +}
> > +
> > +static void ap1302_debugfs_cleanup(struct ap1302_device *ap1302)
> > +{
> > +	debugfs_remove_recursive(ap1302->debugfs.dir);
> > +	mutex_destroy(&ap1302->debugfs.lock);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Power Handling
> > + */
> > +
> > +static int ap1302_power_on_sensors(struct ap1302_device *ap1302)
> > +{
> > +	struct ap1302_sensor *sensor;
> > +	unsigned int i, j;
> > +	int ret;
> > +
> > +	if (!ap1302->sensor_info->supplies)
> > +		return 0;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->sensors); ++i) {
> > +		sensor = &ap1302->sensors[i];
> > +		ret = 0;
> > +
> > +		for (j = 0; j < sensor->num_supplies; ++j) {
> > +			unsigned int delay;
> > +
> > +			/*
> > +			 * We can't use regulator_bulk_enable() as it would
> > +			 * enable all supplies in parallel, breaking the sensor
> > +			 * power sequencing constraints.
> > +			 */
> > +			ret = regulator_enable(sensor->supplies[j].consumer);
> > +			if (ret < 0) {
> > +				dev_err(ap1302->dev,
> > +					"Failed to enable supply %u for sensor %u\n",
> > +					j, i);
> > +				goto error;
> > +			}
> > +
> > +			delay = ap1302->sensor_info->supplies[j].post_delay_us;
> > +			usleep_range(delay, delay + 100);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	for (; j > 0; --j)
> > +		regulator_disable(sensor->supplies[j - 1].consumer);
> > +
> > +	for (; i > 0; --i) {
> > +		sensor = &ap1302->sensors[i - 1];
> > +		regulator_bulk_disable(sensor->num_supplies, sensor->supplies);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void ap1302_power_off_sensors(struct ap1302_device *ap1302)
> > +{
> > +	unsigned int i;
> > +
> > +	if (!ap1302->sensor_info->supplies)
> > +		return;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->sensors); ++i) {
> > +		struct ap1302_sensor *sensor = &ap1302->sensors[i];
> > +
> > +		regulator_bulk_disable(sensor->num_supplies, sensor->supplies);
> > +	}
> > +}
> > +
> > +static int ap1302_power_on(struct ap1302_device *ap1302)
> > +{
> > +	int ret;
> > +
> > +	/* 0. RESET was asserted when getting the GPIO. */
> > +
> > +	/* 1. Assert STANDBY. */
> > +	if (ap1302->standby_gpio) {
> > +		gpiod_set_value(ap1302->standby_gpio, 1);
> > +		usleep_range(200, 1000);
> > +	}
> > +
> > +	/* 2. Power up the regulators. To be implemented. */
> > +
> > +	/* 3. De-assert STANDBY. */
> > +	if (ap1302->standby_gpio) {
> > +		gpiod_set_value(ap1302->standby_gpio, 0);
> > +		usleep_range(200, 1000);
> > +	}
> > +
> > +	/* 4. Turn the clock on. */
> > +	ret = clk_prepare_enable(ap1302->clock);
> > +	if (ret < 0) {
> > +		dev_err(ap1302->dev, "Failed to enable clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* 5. De-assert RESET. */
> > +	gpiod_set_value(ap1302->reset_gpio, 0);
> > +
> > +	/*
> > +	 * 6. Wait for the AP1302 to initialize. The datasheet doesn't specify
> > +	 * how long this takes.
> > +	 */
> > +	usleep_range(10000, 11000);
> > +
> > +	return 0;
> > +}
> > +
> > +static void ap1302_power_off(struct ap1302_device *ap1302)
> > +{
> > +	/* 1. Assert RESET. */
> > +	gpiod_set_value(ap1302->reset_gpio, 1);
> > +
> > +	/* 2. Turn the clock off. */
> > +	clk_disable_unprepare(ap1302->clock);
> > +
> > +	/* 3. Assert STANDBY. */
> > +	if (ap1302->standby_gpio) {
> > +		gpiod_set_value(ap1302->standby_gpio, 1);
> > +		usleep_range(200, 1000);
> > +	}
> > +
> > +	/* 4. Power down the regulators. To be implemented. */
> > +
> > +	/* 5. De-assert STANDBY. */
> > +	if (ap1302->standby_gpio) {
> > +		usleep_range(200, 1000);
> > +		gpiod_set_value(ap1302->standby_gpio, 0);
> > +	}
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Hardware Configuration
> > + */
> > +
> > +static int ap1302_dump_console(struct ap1302_device *ap1302)
> > +{
> > +	u8 *buffer;
> > +	u8 *endp;
> > +	u8 *p;
> > +	int ret;
> > +
> > +	buffer = kmalloc(AP1302_CON_BUF_SIZE + 1, GFP_KERNEL);
> > +	if (!buffer)
> > +		return -ENOMEM;
> > +
> > +	ret = regmap_raw_read(ap1302->regmap16, AP1302_CON_BUF(0), buffer,
> > +			      AP1302_CON_BUF_SIZE);
> > +	if (ret < 0) {
> > +		dev_err(ap1302->dev, "Failed to read console buffer: %d\n",
> > +			ret);
> > +		goto done;
> > +	}
> > +
> > +	print_hex_dump(KERN_INFO, "console ", DUMP_PREFIX_OFFSET, 16, 1, buffer,
> > +		       AP1302_CON_BUF_SIZE, true);
> > +
> > +	buffer[AP1302_CON_BUF_SIZE] = '\0';
> > +
> > +	for (p = buffer; p < buffer + AP1302_CON_BUF_SIZE && *p; p = endp + 1) {
> > +		endp = strchrnul(p, '\n');
> > +		*endp = '\0';
> > +
> > +		pr_info("console %s\n", p);
> > +	}
> > +
> > +	ret = 0;
> > +
> > +done:
> > +	kfree(buffer);
> > +	return ret;
> > +}
> > +
> > +static int ap1302_configure(struct ap1302_device *ap1302)
> > +{
> > +	const struct ap1302_format *format = &ap1302->formats[AP1302_PAD_SOURCE];
> > +	unsigned int data_lanes = ap1302->bus_cfg.bus.mipi_csi2.num_data_lanes;
> > +	int ret = 0;
> > +
> > +	ap1302_write(ap1302, AP1302_PREVIEW_HINF_CTRL,
> > +		     AP1302_PREVIEW_HINF_CTRL_SPOOF |
> > +		     AP1302_PREVIEW_HINF_CTRL_MIPI_LANES(data_lanes), &ret);
> > +
> > +	ap1302_write(ap1302, AP1302_PREVIEW_WIDTH,
> > +		     format->format.width / ap1302->width_factor, &ret);
> > +	ap1302_write(ap1302, AP1302_PREVIEW_HEIGHT,
> > +		     format->format.height, &ret);
> > +	ap1302_write(ap1302, AP1302_PREVIEW_OUT_FMT,
> > +		     format->info->out_fmt, &ret);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	__v4l2_ctrl_handler_setup(&ap1302->ctrls);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_stall(struct ap1302_device *ap1302, bool stall)
> > +{
> > +	int ret = 0;
> > +
> > +	if (stall) {
> > +		ap1302_write(ap1302, AP1302_SYS_START,
> > +			     AP1302_SYS_START_PLL_LOCK |
> > +			     AP1302_SYS_START_STALL_MODE_DISABLED, &ret);
> > +		ap1302_write(ap1302, AP1302_SYS_START,
> > +			     AP1302_SYS_START_PLL_LOCK |
> > +			     AP1302_SYS_START_STALL_EN |
> > +			     AP1302_SYS_START_STALL_MODE_DISABLED, &ret);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		msleep(200);
> > +
> > +		ap1302_write(ap1302, AP1302_ADV_IRQ_SYS_INTE,
> > +			     AP1302_ADV_IRQ_SYS_INTE_SIPM |
> > +			     AP1302_ADV_IRQ_SYS_INTE_SIPS_FIFO_WRITE, &ret);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		return 0;
> > +	} else {
> > +		return ap1302_write(ap1302, AP1302_SYS_START,
> > +				    AP1302_SYS_START_PLL_LOCK |
> > +				    AP1302_SYS_START_STALL_STATUS |
> > +				    AP1302_SYS_START_STALL_EN |
> > +				    AP1302_SYS_START_STALL_MODE_DISABLED, NULL);
> > +	}
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * V4L2 Controls
> > + */
> > +
> > +static u16 ap1302_wb_values[] = {
> > +	AP1302_AWB_CTRL_MODE_OFF,	/* V4L2_WHITE_BALANCE_MANUAL */
> > +	AP1302_AWB_CTRL_MODE_AUTO,	/* V4L2_WHITE_BALANCE_AUTO */
> > +	AP1302_AWB_CTRL_MODE_A,		/* V4L2_WHITE_BALANCE_INCANDESCENT */
> > +	AP1302_AWB_CTRL_MODE_D50,	/* V4L2_WHITE_BALANCE_FLUORESCENT */
> > +	AP1302_AWB_CTRL_MODE_D65,	/* V4L2_WHITE_BALANCE_FLUORESCENT_H */
> > +	AP1302_AWB_CTRL_MODE_HORIZON,	/* V4L2_WHITE_BALANCE_HORIZON */
> > +	AP1302_AWB_CTRL_MODE_D65,	/* V4L2_WHITE_BALANCE_DAYLIGHT */
> > +	AP1302_AWB_CTRL_MODE_AUTO,	/* V4L2_WHITE_BALANCE_FLASH */
> > +	AP1302_AWB_CTRL_MODE_D75,	/* V4L2_WHITE_BALANCE_CLOUDY */
> > +	AP1302_AWB_CTRL_MODE_D75,	/* V4L2_WHITE_BALANCE_SHADE */
> > +};
> > +
> > +static int ap1302_set_wb_mode(struct ap1302_device *ap1302, s32 mode)
> > +{
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_AWB_CTRL, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val &= ~AP1302_AWB_CTRL_MODE_MASK;
> > +	val |= ap1302_wb_values[mode];
> > +
> > +	if (mode == V4L2_WHITE_BALANCE_FLASH)
> > +		val |= AP1302_AWB_CTRL_FLASH;
> > +	else
> > +		val &= ~AP1302_AWB_CTRL_FLASH;
> > +
> > +	return ap1302_write(ap1302, AP1302_AWB_CTRL, val, NULL);
> > +}
> > +
> > +static int ap1302_set_zoom(struct ap1302_device *ap1302, s32 val)
> > +{
> > +	return ap1302_write(ap1302, AP1302_DZ_TGT_FCT, val, NULL);
> > +}
> > +
> > +static u16 ap1302_sfx_values[] = {
> > +	AP1302_SFX_MODE_SFX_NORMAL,	/* V4L2_COLORFX_NONE */
> > +	AP1302_SFX_MODE_SFX_BW,		/* V4L2_COLORFX_BW */
> > +	AP1302_SFX_MODE_SFX_SEPIA1,	/* V4L2_COLORFX_SEPIA */
> > +	AP1302_SFX_MODE_SFX_NEGATIVE,	/* V4L2_COLORFX_NEGATIVE */
> > +	AP1302_SFX_MODE_SFX_EMBOSS,	/* V4L2_COLORFX_EMBOSS */
> > +	AP1302_SFX_MODE_SFX_SKETCH,	/* V4L2_COLORFX_SKETCH */
> > +	AP1302_SFX_MODE_SFX_BLUISH,	/* V4L2_COLORFX_SKY_BLUE */
> > +	AP1302_SFX_MODE_SFX_GREENISH,	/* V4L2_COLORFX_GRASS_GREEN */
> > +	AP1302_SFX_MODE_SFX_REDISH,	/* V4L2_COLORFX_SKIN_WHITEN */
> > +	AP1302_SFX_MODE_SFX_NORMAL,	/* V4L2_COLORFX_VIVID */
> > +	AP1302_SFX_MODE_SFX_NORMAL,	/* V4L2_COLORFX_AQUA */
> > +	AP1302_SFX_MODE_SFX_NORMAL,	/* V4L2_COLORFX_ART_FREEZE */
> > +	AP1302_SFX_MODE_SFX_NORMAL,	/* V4L2_COLORFX_SILHOUETTE */
> > +	AP1302_SFX_MODE_SFX_SOLARIZE, /* V4L2_COLORFX_SOLARIZATION */
> > +	AP1302_SFX_MODE_SFX_ANTIQUE, /* V4L2_COLORFX_ANTIQUE */
> > +	AP1302_SFX_MODE_SFX_NORMAL,	/* V4L2_COLORFX_SET_CBCR */
> > +};
> > +
> > +static int ap1302_set_special_effect(struct ap1302_device *ap1302, s32 val)
> > +{
> > +	return ap1302_write(ap1302, AP1302_SFX_MODE, ap1302_sfx_values[val],
> > +			    NULL);
> > +}
> > +
> > +static u16 ap1302_scene_mode_values[] = {
> > +	AP1302_SCENE_CTRL_MODE_NORMAL,		/* V4L2_SCENE_MODE_NONE */
> > +	AP1302_SCENE_CTRL_MODE_BACKLIGHT,	/* V4L2_SCENE_MODE_BACKLIGHT */
> > +	AP1302_SCENE_CTRL_MODE_BEACH,		/* V4L2_SCENE_MODE_BEACH_SNOW */
> > +	AP1302_SCENE_CTRL_MODE_TWILIGHT,	/* V4L2_SCENE_MODE_CANDLE_LIGHT */
> > +	AP1302_SCENE_CTRL_MODE_NORMAL,		/* V4L2_SCENE_MODE_DAWN_DUSK */
> > +	AP1302_SCENE_CTRL_MODE_NORMAL,		/* V4L2_SCENE_MODE_FALL_COLORS */
> > +	AP1302_SCENE_CTRL_MODE_FIREWORKS,	/* V4L2_SCENE_MODE_FIREWORKS */
> > +	AP1302_SCENE_CTRL_MODE_LANDSCAPE,	/* V4L2_SCENE_MODE_LANDSCAPE */
> > +	AP1302_SCENE_CTRL_MODE_NIGHT,		/* V4L2_SCENE_MODE_NIGHT */
> > +	AP1302_SCENE_CTRL_MODE_PARTY,		/* V4L2_SCENE_MODE_PARTY_INDOOR */
> > +	AP1302_SCENE_CTRL_MODE_PORTRAIT,	/* V4L2_SCENE_MODE_PORTRAIT */
> > +	AP1302_SCENE_CTRL_MODE_SPORT,		/* V4L2_SCENE_MODE_SPORTS */
> > +	AP1302_SCENE_CTRL_MODE_SUNSET,		/* V4L2_SCENE_MODE_SUNSET */
> > +	AP1302_SCENE_CTRL_MODE_DOCUMENT,	/* V4L2_SCENE_MODE_TEXT */
> > +};
> > +
> > +static int ap1302_set_scene_mode(struct ap1302_device *ap1302, s32 val)
> > +{
> > +	return ap1302_write(ap1302, AP1302_SCENE_CTRL,
> > +			    ap1302_scene_mode_values[val], NULL);
> > +}
> > +
> > +static const u16 ap1302_flicker_values[] = {
> > +	AP1302_FLICK_CTRL_MODE_DISABLED,
> > +	AP1302_FLICK_CTRL_FREQ(50) | AP1302_FLICK_CTRL_MODE_MANUAL,
> > +	AP1302_FLICK_CTRL_FREQ(60) | AP1302_FLICK_CTRL_MODE_MANUAL,
> > +	AP1302_FLICK_CTRL_MODE_AUTO,
> > +};
> > +
> > +static int ap1302_set_flicker_freq(struct ap1302_device *ap1302, s32 val)
> > +{
> > +	return ap1302_write(ap1302, AP1302_FLICK_CTRL,
> > +			    ap1302_flicker_values[val], NULL);
> > +}
> > +
> > +static int ap1302_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct ap1302_device *ap1302 =
> > +		container_of(ctrl->handler, struct ap1302_device, ctrls);
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE:
> > +		return ap1302_set_wb_mode(ap1302, ctrl->val);
> > +
> > +	case V4L2_CID_ZOOM_ABSOLUTE:
> > +		return ap1302_set_zoom(ap1302, ctrl->val);
> > +
> > +	case V4L2_CID_COLORFX:
> > +		return ap1302_set_special_effect(ap1302, ctrl->val);
> > +
> > +	case V4L2_CID_SCENE_MODE:
> > +		return ap1302_set_scene_mode(ap1302, ctrl->val);
> > +
> > +	case V4L2_CID_POWER_LINE_FREQUENCY:
> > +		return ap1302_set_flicker_freq(ap1302, ctrl->val);
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct v4l2_ctrl_ops ap1302_ctrl_ops = {
> > +	.s_ctrl = ap1302_s_ctrl,
> > +};
> > +
> > +static const struct v4l2_ctrl_config ap1302_ctrls[] = {
> > +	{
> > +		.ops = &ap1302_ctrl_ops,
> > +		.id = V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE,
> > +		.min = 0,
> > +		.max = 9,
> > +		.def = 1,
> > +	}, {
> > +		.ops = &ap1302_ctrl_ops,
> > +		.id = V4L2_CID_ZOOM_ABSOLUTE,
> > +		.min = 0x0100,
> > +		.max = 0x1000,
> > +		.step = 1,
> > +		.def = 0x0100,
> > +	}, {
> > +		.ops = &ap1302_ctrl_ops,
> > +		.id = V4L2_CID_COLORFX,
> > +		.min = 0,
> > +		.max = 15,
> > +		.def = 0,
> > +		.menu_skip_mask = BIT(15) | BIT(12) | BIT(11) | BIT(10) | BIT(9),
> > +	}, {
> > +		.ops = &ap1302_ctrl_ops,
> > +		.id = V4L2_CID_SCENE_MODE,
> > +		.min = 0,
> > +		.max = 13,
> > +		.def = 0,
> > +		.menu_skip_mask = BIT(5) | BIT(4),
> > +	}, {
> > +		.ops = &ap1302_ctrl_ops,
> > +		.id = V4L2_CID_POWER_LINE_FREQUENCY,
> > +		.min = 0,
> > +		.max = 3,
> > +		.def = 3,
> > +	},
> > +};
> > +
> > +static int ap1302_ctrls_init(struct ap1302_device *ap1302)
> > +{
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	ret = v4l2_ctrl_handler_init(&ap1302->ctrls, ARRAY_SIZE(ap1302_ctrls));
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302_ctrls); i++)
> > +		v4l2_ctrl_new_custom(&ap1302->ctrls, &ap1302_ctrls[i], NULL);
> > +
> > +	if (ap1302->ctrls.error) {
> > +		ret = ap1302->ctrls.error;
> > +		v4l2_ctrl_handler_free(&ap1302->ctrls);
> > +		return ret;
> > +	}
> > +
> > +	/* Use same lock for controls as for everything else. */
> > +	ap1302->ctrls.lock = &ap1302->lock;
> > +	ap1302->sd.ctrl_handler = &ap1302->ctrls;
> > +
> > +	return 0;
> > +}
> > +
> > +static void ap1302_ctrls_cleanup(struct ap1302_device *ap1302)
> > +{
> > +	v4l2_ctrl_handler_free(&ap1302->ctrls);
> 
> You could omit the entire function and just call v4l2_ctrl_handler_free().

Sure, but I like the symmetry between ap1302_ctrls_init() and
ap1302_ctrls_cleanup(). It's cleaner from a caller's point of view, and
would minimize the required changes if more operations were needed at
cleanup time. The compiler will likely inline the function anyway.

> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * V4L2 Subdev Operations
> > + */
> > +
> > +static struct v4l2_mbus_framefmt *
> > +ap1302_get_pad_format(struct ap1302_device *ap1302,
> > +		      struct v4l2_subdev_pad_config *cfg,
> > +		      unsigned int pad, u32 which)
> > +{
> > +	switch (which) {
> > +	case V4L2_SUBDEV_FORMAT_TRY:
> > +		return v4l2_subdev_get_try_format(&ap1302->sd, cfg, pad);
> > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +		return &ap1302->formats[pad].format;
> > +	default:
> > +		return NULL;
> > +	}
> > +}
> > +
> > +static int ap1302_init_cfg(struct v4l2_subdev *sd,
> > +			   struct v4l2_subdev_pad_config *cfg)
> > +{
> > +	u32 which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	const struct ap1302_sensor_info *info = ap1302->sensor_info;
> > +	unsigned int pad;
> > +
> > +	for (pad = 0; pad < ARRAY_SIZE(ap1302->formats); ++pad) {
> > +		struct v4l2_mbus_framefmt *format =
> > +			ap1302_get_pad_format(ap1302, cfg, pad, which);
> > +
> > +		format->width = info->resolution.width;
> > +		format->height = info->resolution.height;
> > +
> > +		/*
> > +		 * The source pad combines images side by side in multi-sensor
> > +		 * setup.
> > +		 */
> > +		if (pad == AP1302_PAD_SOURCE) {
> > +			format->width *= ap1302->width_factor;
> > +			format->code = ap1302->formats[pad].info->code;
> > +		} else {
> > +			format->code = info->format;
> > +		}
> > +
> > +		format->field = V4L2_FIELD_NONE;
> > +		format->colorspace = V4L2_COLORSPACE_SRGB;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_enum_mbus_code(struct v4l2_subdev *sd,
> > +				 struct v4l2_subdev_pad_config *cfg,
> > +				 struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +
> > +	if (code->pad != AP1302_PAD_SOURCE) {
> > +		/*
> > +		 * On the sink pads, only the format produced by the sensor is
> > +		 * supported.
> > +		 */
> > +		if (code->index)
> > +			return -EINVAL;
> > +
> > +		code->code = ap1302->sensor_info->format;
> > +	} else {
> > +		/* On the source pad, multiple formats are supported. */
> > +		if (code->index >= ARRAY_SIZE(supported_video_formats))
> > +			return -EINVAL;
> > +
> > +		code->code = supported_video_formats[code->index].code;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_enum_frame_size(struct v4l2_subdev *sd,
> > +				  struct v4l2_subdev_pad_config *cfg,
> > +				  struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	unsigned int i;
> > +
> > +	if (fse->index)
> > +		return -EINVAL;
> > +
> > +	if (fse->pad != AP1302_PAD_SOURCE) {
> > +		/*
> > +		 * On the sink pads, only the size produced by the sensor is
> > +		 * supported.
> > +		 */
> > +		if (fse->code != ap1302->sensor_info->format)
> > +			return -EINVAL;
> > +
> > +		fse->min_width = ap1302->sensor_info->resolution.width;
> > +		fse->min_height = ap1302->sensor_info->resolution.height;
> > +		fse->max_width = ap1302->sensor_info->resolution.width;
> > +		fse->max_height = ap1302->sensor_info->resolution.height;
> > +	} else {
> > +		/*
> > +		 * On the source pad, the AP1302 can freely scale within the
> > +		 * scaler's limits.
> > +		 */
> > +		for (i = 0; i < ARRAY_SIZE(supported_video_formats); i++) {
> > +			if (supported_video_formats[i].code == fse->code)
> > +				break;
> > +		}
> > +
> > +		if (i >= ARRAY_SIZE(supported_video_formats))
> > +			return -EINVAL;
> > +
> > +		fse->min_width = AP1302_MIN_WIDTH * ap1302->width_factor;
> > +		fse->min_height = AP1302_MIN_HEIGHT;
> > +		fse->max_width = AP1302_MAX_WIDTH;
> > +		fse->max_height = AP1302_MAX_HEIGHT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_get_fmt(struct v4l2_subdev *sd,
> > +			  struct v4l2_subdev_pad_config *cfg,
> > +			  struct v4l2_subdev_format *fmt)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	const struct v4l2_mbus_framefmt *format;
> > +
> > +	format = ap1302_get_pad_format(ap1302, cfg, fmt->pad, fmt->which);
> > +
> > +	mutex_lock(&ap1302->lock);
> > +	fmt->format = *format;
> > +	mutex_unlock(&ap1302->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_set_fmt(struct v4l2_subdev *sd,
> > +			  struct v4l2_subdev_pad_config *cfg,
> > +			  struct v4l2_subdev_format *fmt)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	const struct ap1302_format_info *info = NULL;
> > +	struct v4l2_mbus_framefmt *format;
> > +	unsigned int i;
> > +
> > +	/* Formats on the sink pads can't be changed. */
> > +	if (fmt->pad != AP1302_PAD_SOURCE)
> > +		return ap1302_get_fmt(sd, cfg, fmt);
> > +
> > +	format = ap1302_get_pad_format(ap1302, cfg, fmt->pad, fmt->which);
> > +
> > +	/* Validate the media bus code, default to the first supported value. */
> > +	for (i = 0; i < ARRAY_SIZE(supported_video_formats); i++) {
> > +		if (supported_video_formats[i].code == fmt->format.code) {
> > +			info = &supported_video_formats[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!info)
> > +		info = &supported_video_formats[0];
> > +
> > +	/*
> > +	 * Clamp the size. The width must be a multiple of 4 (or 8 in the
> > +	 * dual-sensor case) and the height a multiple of 2.
> > +	 */
> > +	fmt->format.width = clamp(ALIGN_DOWN(fmt->format.width,
> > +					     4 * ap1302->width_factor),
> > +				  AP1302_MIN_WIDTH * ap1302->width_factor,
> > +				  AP1302_MAX_WIDTH);
> > +	fmt->format.height = clamp(ALIGN_DOWN(fmt->format.height, 2),
> > +				   AP1302_MIN_HEIGHT, AP1302_MAX_HEIGHT);
> > +
> > +	mutex_lock(&ap1302->lock);
> > +
> > +	format->width = fmt->format.width;
> > +	format->height = fmt->format.height;
> > +	format->code = info->code;
> > +
> > +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		ap1302->formats[fmt->pad].info = info;
> > +
> > +	mutex_unlock(&ap1302->lock);
> > +
> > +	fmt->format = *format;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_get_selection(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_pad_config *cfg,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	const struct ap1302_size *resolution = &ap1302->sensor_info->resolution;
> > +
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP:
> > +		sel->r.left = 0;
> > +		sel->r.top = 0;
> > +		sel->r.width = resolution->width * ap1302->width_factor;
> > +		sel->r.height = resolution->height;
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	int ret;
> > +
> > +	mutex_lock(&ap1302->lock);
> > +
> > +	if (enable) {
> > +		ret = ap1302_configure(ap1302);
> > +		if (ret < 0)
> > +			goto done;
> > +
> > +		ret = ap1302_stall(ap1302, false);
> > +	} else {
> > +		ret = ap1302_stall(ap1302, true);
> > +	}
> > +
> > +done:
> > +	mutex_unlock(&ap1302->lock);
> > +
> > +	if (ret < 0)
> > +		dev_err(ap1302->dev, "Failed to %s stream: %d\n",
> > +			enable ? "start" : "stop", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static const char * const ap1302_warnings[] = {
> > +	"HINF_BANDWIDTH",
> > +	"FLICKER_DETECTION",
> > +	"FACED_NE",
> > +	"SMILED_NE",
> > +	"HINF_OVERRUN",
> > +	NULL,
> > +	"FRAME_TOO_SMALL",
> > +	"MISSING_PHASES",
> > +	"SPOOF_UNDERRUN",
> > +	"JPEG_NOLAST",
> > +	"NO_IN_FREQ_SPEC",
> > +	"SINF0",
> > +	"SINF1",
> > +	"CAPTURE0",
> > +	"CAPTURE1",
> > +	"ISR_UNHANDLED",
> > +	"INTERLEAVE_SPOOF",
> > +	"INTERLEAVE_BUF",
> > +	"COORD_OUT_OF_RANGE",
> > +	"ICP_CLOCKING",
> > +	"SENSOR_CLOCKING",
> > +	"SENSOR_NO_IHDR",
> > +	"DIVIDE_BY_ZERO",
> > +	"INT0_UNDERRUN",
> > +	"INT1_UNDERRUN",
> > +	"SCRATCHPAD_TOO_BIG",
> > +	"OTP_RECORD_READ",
> > +	"NO_LSC_IN_OTP",
> > +	"GPIO_INT_LOST",
> > +	"NO_PDAF_DATA",
> > +	"FAR_PDAF_ACCESS_SKIP",
> > +	"PDAF_ERROR",
> > +	"ATM_TVI_BOUNDS",
> > +	"SIPM_0_RTY",
> > +	"SIPM_1_TRY",
> > +	"SIPM_0_NO_ACK",
> > +	"SIPM_1_NO_ACK",
> > +	"SMILE_DIS",
> > +	"DVS_DIS",
> > +	"TEST_DIS",
> > +	"SENSOR_LV2LV",
> > +	"SENSOR_FV2FV",
> > +	"FRAME_LOST",
> > +};
> > +
> > +static const char * const ap1302_lane_states[] = {
> > +	"stop_s",
> > +	"hs_req_s",
> > +	"lp_req_s",
> > +	"hs_s",
> > +	"lp_s",
> > +	"esc_req_s",
> > +	"turn_req_s",
> > +	"esc_s",
> > +	"esc_0",
> > +	"esc_1",
> > +	"turn_s",
> > +	"turn_mark",
> > +	"error_s",
> > +};
> > +
> > +static void ap1302_log_lane_state(struct ap1302_sensor *sensor,
> > +				  unsigned int index)
> > +{
> > +	static const char * const lp_states[] = {
> > +		"00", "10", "01", "11",
> > +	};
> > +
> 
> Extra newline.
> 
> > +	unsigned int counts[4][ARRAY_SIZE(ap1302_lane_states)];
> 
> How about replacing 4 by a macro called NUM_LANES or MAX_LANES?
> 
> > +	unsigned int samples = 0;
> > +	unsigned int lane;
> > +	unsigned int i;
> > +	u32 first[4] = { 0, };
> > +	u32 last[4] = { 0, };
> > +	int ret;
> > +
> > +	memset(counts, 0, sizeof(counts));
> > +
> > +	for (i = 0; i < 1000; ++i) {
> > +		u32 values[4];
> > +
> > +		/*
> > +		 * Read the state of all lanes and skip read errors and invalid
> > +		 * values.
> > +		 */
> > +		for (lane = 0; lane < 4; ++lane) {
> > +			ret = ap1302_read(sensor->ap1302,
> > +					  AP1302_ADV_SINF_MIPI_INTERNAL_p_LANE_n_STAT(index, lane),
> > +					  &values[lane]);
> > +			if (ret < 0)
> > +				break;
> > +
> > +			if (AP1302_LANE_STATE(values[lane]) >=
> > +			    ARRAY_SIZE(ap1302_lane_states)) {
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		/* Accumulate the samples and save the first and last states. */
> > +		for (lane = 0; lane < 4; ++lane)
> > +			counts[lane][AP1302_LANE_STATE(values[lane])]++;
> > +
> > +		if (!samples)
> > +			memcpy(first, values, sizeof(first));
> > +		memcpy(last, values, sizeof(last));
> > +
> > +		samples++;
> > +	}
> > +
> > +	if (!samples)
> > +		return;
> > +
> > +	/*
> > +	 * Print the LP state from the first sample, the error state from the
> > +	 * last sample, and the states accumulators for each lane.
> > +	 */
> > +	for (lane = 0; lane < 4; ++lane) {
> > +		u32 state = last[lane];
> > +		char error_msg[25] = "";
> > +
> > +		if (state & (AP1302_LANE_ERR | AP1302_LANE_ABORT)) {
> > +			unsigned int err = AP1302_LANE_ERR_STATE(state);
> > +			const char *err_state = NULL;
> > +
> > +			err_state = err < ARRAY_SIZE(ap1302_lane_states)
> > +				  ? ap1302_lane_states[err] : "INVALID";
> > +
> > +			snprintf(error_msg, sizeof(error_msg), "ERR (%s%s) %s LP%s",
> > +				 state & AP1302_LANE_ERR ? "E" : "",
> > +				 state & AP1302_LANE_ABORT ? "A" : "",
> > +				 err_state,
> > +				 lp_states[AP1302_LANE_ERR_LP_VAL(state)]);
> > +		}
> > +
> > +		dev_info(sensor->ap1302->dev, "SINF%u L%u state: LP%s %s",
> > +			 index, lane, lp_states[AP1302_LANE_LP_VAL(first[lane])],
> > +			 error_msg);
> > +
> > +		for (i = 0; i < ARRAY_SIZE(ap1302_lane_states); ++i) {
> > +			if (counts[lane][i])
> > +				pr_cont(" %s:%u",
> > +				       ap1302_lane_states[i],
> > +				       counts[lane][i]);
> > +		}
> > +		pr_cont("\n");
> > +	}
> > +
> > +	/* Reset the error flags. */
> > +	for (lane = 0; lane < 4; ++lane)
> > +		ap1302_write(sensor->ap1302,
> > +			     AP1302_ADV_SINF_MIPI_INTERNAL_p_LANE_n_STAT(index, lane),
> > +			     AP1302_LANE_ERR | AP1302_LANE_ABORT, NULL);
> > +}
> > +
> > +static int ap1302_log_status(struct v4l2_subdev *sd)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	u16 frame_count_icp;
> > +	u16 frame_count_brac;
> > +	u16 frame_count_hinf;
> > +	u32 warning[4];
> > +	u32 error[3];
> > +	unsigned int i;
> > +	u32 value;
> > +	int ret;
> > +
> > +	/* Dump the console buffer. */
> > +	ret = ap1302_dump_console(ap1302);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Print errors. */
> > +	ret = ap1302_read(ap1302, AP1302_ERROR, &error[0]);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_ERR_FILE, &error[1]);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_ERR_LINE, &error[2]);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev_info(ap1302->dev, "ERROR: 0x%04x (file 0x%08x:%u)\n",
> > +		 error[0], error[1], error[2]);
> > +
> > +	ret = ap1302_read(ap1302, AP1302_SIPM_ERR_0, &error[0]);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_SIPM_ERR_1, &error[1]);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev_info(ap1302->dev, "SIPM_ERR [0] 0x%04x [1] 0x%04x\n",
> > +		 error[0], error[1]);
> > +
> > +	/* Print warnings. */
> > +	for (i = 0; i < ARRAY_SIZE(warning); ++i) {
> > +		ret = ap1302_read(ap1302, AP1302_WARNING(i), &warning[i]);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	dev_info(ap1302->dev,
> > +		 "WARNING [0] 0x%04x [1] 0x%04x [2] 0x%04x [3] 0x%04x\n",
> > +		 warning[0], warning[1], warning[2], warning[3]);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302_warnings); ++i) {
> > +		if ((warning[i / 16] & BIT(i % 16)) &&
> > +		    ap1302_warnings[i])
> > +			dev_info(ap1302->dev, "- WARN_%s\n",
> > +				 ap1302_warnings[i]);
> > +	}
> > +
> > +	/* Print the frame counter. */
> > +	ret = ap1302_read(ap1302, AP1302_FRAME_CNT, &value);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	frame_count_hinf = value >> 8;
> > +	frame_count_brac = value & 0xff;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_ADV_CAPTURE_A_FV_CNT, &value);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	frame_count_icp = value & 0xffff;
> > +
> > +	dev_info(ap1302->dev, "Frame counters: ICP %u, HINF %u, BRAC %u\n",
> > +		 frame_count_icp, frame_count_hinf, frame_count_brac);
> > +
> > +	/* Sample the lane state. */
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->sensors); ++i) {
> > +		struct ap1302_sensor *sensor = &ap1302->sensors[i];
> > +
> > +		if (!sensor->ap1302)
> > +			continue;
> > +
> > +		ap1302_log_lane_state(sensor, i);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_subdev_registered(struct v4l2_subdev *sd)
> > +{
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->sensors); ++i) {
> > +		struct ap1302_sensor *sensor = &ap1302->sensors[i];
> > +
> > +		if (!sensor->dev)
> > +			continue;
> > +
> > +		dev_dbg(ap1302->dev, "registering sensor %u\n", i);
> > +
> > +		ret = v4l2_device_register_subdev(sd->v4l2_dev, &sensor->sd);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = media_create_pad_link(&sensor->sd.entity, 0,
> > +					    &sd->entity, i,
> > +					    MEDIA_LNK_FL_IMMUTABLE |
> > +					    MEDIA_LNK_FL_ENABLED);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct media_entity_operations ap1302_media_ops = {
> > +	.link_validate = v4l2_subdev_link_validate
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops ap1302_pad_ops = {
> > +	.init_cfg = ap1302_init_cfg,
> > +	.enum_mbus_code = ap1302_enum_mbus_code,
> > +	.enum_frame_size = ap1302_enum_frame_size,
> > +	.get_fmt = ap1302_get_fmt,
> > +	.set_fmt = ap1302_set_fmt,
> > +	.get_selection = ap1302_get_selection,
> > +	.set_selection = ap1302_get_selection,
> > +};
> > +
> > +static const struct v4l2_subdev_video_ops ap1302_video_ops = {
> > +	.s_stream = ap1302_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_core_ops ap1302_core_ops = {
> > +	.log_status = ap1302_log_status,
> > +};
> > +
> > +static const struct v4l2_subdev_ops ap1302_subdev_ops = {
> > +	.core = &ap1302_core_ops,
> > +	.video = &ap1302_video_ops,
> > +	.pad = &ap1302_pad_ops,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops ap1302_subdev_internal_ops = {
> > +	.registered = ap1302_subdev_registered,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Sensor
> > + */
> > +
> > +static int ap1302_sensor_enum_mbus_code(struct v4l2_subdev *sd,
> > +					struct v4l2_subdev_pad_config *cfg,
> > +					struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +	struct ap1302_sensor *sensor = to_ap1302_sensor(sd);
> > +	const struct ap1302_sensor_info *info = sensor->ap1302->sensor_info;
> > +
> > +	if (code->index)
> > +		return -EINVAL;
> > +
> > +	code->code = info->format;
> 
> A newline here would be nice.

Should I take the one you don't like in ap1302_log_lane_state() ? ;-)

> > +	return 0;
> > +}
> > +
> > +static int ap1302_sensor_enum_frame_size(struct v4l2_subdev *sd,
> > +					 struct v4l2_subdev_pad_config *cfg,
> > +					 struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +	struct ap1302_sensor *sensor = to_ap1302_sensor(sd);
> > +	const struct ap1302_sensor_info *info = sensor->ap1302->sensor_info;
> > +
> > +	if (fse->index)
> > +		return -EINVAL;
> > +
> > +	if (fse->code != info->format)
> > +		return -EINVAL;
> > +
> > +	fse->min_width = info->resolution.width;
> > +	fse->min_height = info->resolution.height;
> > +	fse->max_width = info->resolution.width;
> > +	fse->max_height = info->resolution.height;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_sensor_get_fmt(struct v4l2_subdev *sd,
> > +				 struct v4l2_subdev_pad_config *cfg,
> > +				 struct v4l2_subdev_format *fmt)
> > +{
> > +	struct ap1302_sensor *sensor = to_ap1302_sensor(sd);
> > +	const struct ap1302_sensor_info *info = sensor->ap1302->sensor_info;
> > +
> > +	memset(&fmt->format, 0, sizeof(fmt->format));
> > +
> > +	fmt->format.width = info->resolution.width;
> > +	fmt->format.height = info->resolution.height;
> > +	fmt->format.field = V4L2_FIELD_NONE;
> > +	fmt->format.code = info->format;
> > +	fmt->format.colorspace = V4L2_COLORSPACE_SRGB;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_pad_ops ap1302_sensor_pad_ops = {
> > +	.enum_mbus_code = ap1302_sensor_enum_mbus_code,
> > +	.enum_frame_size = ap1302_sensor_enum_frame_size,
> > +	.get_fmt = ap1302_sensor_get_fmt,
> > +	.set_fmt = ap1302_sensor_get_fmt,
> > +};
> > +
> > +static const struct v4l2_subdev_ops ap1302_sensor_subdev_ops = {
> > +	.pad = &ap1302_sensor_pad_ops,
> > +};
> > +
> > +static int ap1302_sensor_parse_of(struct ap1302_device *ap1302,
> > +				  struct device_node *node)
> > +{
> > +	struct ap1302_sensor *sensor;
> > +	u32 reg;
> > +	int ret;
> > +
> > +	/* Retrieve the sensor index from the reg property. */
> > +	ret = of_property_read_u32(node, "reg", &reg);
> > +	if (ret < 0) {
> > +		dev_warn(ap1302->dev,
> > +			 "'reg' property missing in sensor node\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (reg >= ARRAY_SIZE(ap1302->sensors)) {
> > +		dev_warn(ap1302->dev, "Out-of-bounds 'reg' value %u\n",
> > +			 reg);
> > +		return -EINVAL;
> > +	}
> > +
> > +	sensor = &ap1302->sensors[reg];
> > +	if (sensor->ap1302) {
> > +		dev_warn(ap1302->dev, "Duplicate entry for sensor %u\n", reg);
> > +		return -EINVAL;
> > +	}
> > +
> > +	sensor->ap1302 = ap1302;
> > +	sensor->of_node = of_node_get(node);
> > +
> > +	return 0;
> > +}
> > +
> > +static void ap1302_sensor_dev_release(struct device *dev)
> > +{
> > +	of_node_put(dev->of_node);
> > +	kfree(dev);
> > +}
> > +
> > +static int ap1302_sensor_init(struct ap1302_sensor *sensor, unsigned int index)
> > +{
> > +	struct ap1302_device *ap1302 = sensor->ap1302;
> > +	struct v4l2_subdev *sd = &sensor->sd;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	sensor->index = index;
> > +
> > +	/*
> > +	 * Register a device for the sensor, to support usage of the regulator
> > +	 * API.
> > +	 */
> > +	sensor->dev = kzalloc(sizeof(*sensor->dev), GFP_KERNEL);
> > +	if (!sensor->dev)
> > +		return -ENOMEM;
> > +
> > +	sensor->dev->parent = ap1302->dev;
> > +	sensor->dev->of_node = of_node_get(sensor->of_node);
> > +	sensor->dev->release = &ap1302_sensor_dev_release;
> > +	dev_set_name(sensor->dev, "%s-%s.%u", dev_name(ap1302->dev),
> > +		     ap1302->sensor_info->name, index);
> > +
> > +	ret = device_register(sensor->dev);
> > +	if (ret < 0) {
> > +		dev_err(ap1302->dev,
> > +			"Failed to register device for sensor %u\n", index);
> > +		goto error;
> > +	}
> > +
> > +	/* Retrieve the power supplies for the sensor, if any. */
> > +	if (ap1302->sensor_info->supplies) {
> > +		const struct ap1302_sensor_supply *supplies =
> > +			ap1302->sensor_info->supplies;
> > +		unsigned int num_supplies;
> > +
> > +		for (num_supplies = 0; supplies[num_supplies].name; ++num_supplies)
> 
> This would be nice to wrap on two lines.

It's 3 characters over the 80 columns limit, which in this case I think
is reasonable given the increased readability. I won't make it a casus
belli though.

> > +			;
> > +
> > +		sensor->supplies = devm_kcalloc(ap1302->dev, num_supplies,
> > +						sizeof(*sensor->supplies),
> > +						GFP_KERNEL);
> > +		if (!sensor->supplies) {
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> > +
> > +		for (i = 0; i < num_supplies; ++i)
> > +			sensor->supplies[i].supply = supplies[i].name;
> > +
> > +		ret = regulator_bulk_get(sensor->dev, num_supplies,
> > +					 sensor->supplies);
> > +		if (ret < 0) {
> > +			dev_err(ap1302->dev,
> > +				"Failed to get supplies for sensor %u\n", index);
> 
> Ditto.
> 
> > +			goto error;
> > +		}
> > +
> > +		sensor->num_supplies = i;
> > +	}
> > +
> > +	sd->dev = sensor->dev;
> > +	v4l2_subdev_init(sd, &ap1302_sensor_subdev_ops);
> > +
> > +	snprintf(sd->name, sizeof(sd->name), "%s %u",
> > +		 ap1302->sensor_info->name, index);
> > +
> > +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +	ret = media_entity_pads_init(&sd->entity, 1, &sensor->pad);
> > +	if (ret < 0) {
> > +		dev_err(ap1302->dev,
> > +			"failed to initialize media entity for sensor %u: %d\n",
> > +			index, ret);
> > +		goto error;
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	put_device(sensor->dev);
> > +	return ret;
> > +}
> > +
> > +static void ap1302_sensor_cleanup(struct ap1302_sensor *sensor)
> > +{
> > +	media_entity_cleanup(&sensor->sd.entity);
> > +
> > +	if (sensor->num_supplies)
> > +		regulator_bulk_free(sensor->num_supplies, sensor->supplies);
> > +
> > +	put_device(sensor->dev);
> > +	of_node_put(sensor->of_node);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Boot & Firmware Handling
> > + */
> > +
> > +static int ap1302_request_firmware(struct ap1302_device *ap1302)
> > +{
> > +	static const char * const suffixes[] = {
> > +		"",
> > +		"_single",
> > +		"_dual",
> > +	};
> > +
> > +	const struct ap1302_firmware_header *fw_hdr;
> > +	unsigned int num_sensors;
> > +	unsigned int fw_size;
> > +	unsigned int i;
> > +	char name[64];
> > +	int ret;
> > +
> > +	for (i = 0, num_sensors = 0; i < ARRAY_SIZE(ap1302->sensors); ++i) {
> > +		if (ap1302->sensors[i].dev)
> > +			num_sensors++;
> > +	}
> > +
> > +	ret = snprintf(name, sizeof(name), "ap1302_%s%s_fw.bin",
> > +		       ap1302->sensor_info->name, suffixes[num_sensors]);
> 
> What's the difference between the three different firmware images?
> 
> I guess you're using two sensors at a time if two are found, with other
> possibilities left as further development if needed and firmware allows?

The _single and _dual versions are used when a single or two sensors are
connected respectively (these two cases require different firmwares). In
the dual sensor case, the AP1302 will output the two images
side-by-side.

The firmware version without any suffix is used when no sensor is
connected, and is a special firmware that enables the TPG only.

> > +	if (ret >= sizeof(name)) {
> > +		dev_err(ap1302->dev, "Firmware name too long\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(ap1302->dev, "Requesting firmware %s\n", name);
> > +
> > +	ret = request_firmware(&ap1302->fw, name, ap1302->dev);
> > +	if (ret) {
> > +		dev_err(ap1302->dev, "Failed to request firmware: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * The firmware binary contains a header defined by the
> > +	 * ap1302_firmware_header structure. The firmware itself (also referred
> > +	 * to as bootdata) follows the header. Perform sanity checks to ensure
> > +	 * the firmware is valid.
> > +	 */
> > +	fw_hdr = (const struct ap1302_firmware_header *)ap1302->fw->data;
> > +	fw_size = ap1302->fw->size - sizeof(*fw_hdr);
> > +
> > +	if (fw_hdr->pll_init_size > fw_size) {
> > +		dev_err(ap1302->dev,
> > +			"Invalid firmware: PLL init size too large\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * ap1302_write_fw_window() - Write a piece of firmware to the AP1302
> > + * @win_pos: Firmware load window current position
> > + * @buf: Firmware data buffer
> > + * @len: Firmware data length
> > + *
> > + * The firmware is loaded through a window in the registers space. Writes are
> > + * sequential starting at address 0x8000, and must wrap around when reaching
> > + * 0x9fff. This function write the firmware data stored in @buf to the AP1302,
> > + * keeping track of the window position in the @win_pos argument.
> > + */
> > +static int ap1302_write_fw_window(struct ap1302_device *ap1302, const u8 *buf,
> > +				  u32 len, unsigned int *win_pos)
> > +{
> > +	while (len > 0) {
> > +		unsigned int write_addr;
> > +		unsigned int write_size;
> > +		int ret;
> > +
> > +		/*
> > +		 * Write at most len bytes, from the current position to the
> > +		 * end of the window.
> > +		 */
> > +		write_addr = *win_pos + AP1302_FW_WINDOW_OFFSET;
> > +		write_size = min(len, AP1302_FW_WINDOW_SIZE - *win_pos);
> > +
> > +		ret = regmap_raw_write(ap1302->regmap16, write_addr, buf,
> > +				       write_size);
> > +		if (ret)
> > +			return ret;
> > +
> > +		buf += write_size;
> > +		len -= write_size;
> > +
> > +		*win_pos += write_size;
> > +		if (*win_pos >= AP1302_FW_WINDOW_SIZE)
> > +			*win_pos = 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_load_firmware(struct ap1302_device *ap1302)
> > +{
> > +	const struct ap1302_firmware_header *fw_hdr;
> > +	unsigned int fw_size;
> > +	const u8 *fw_data;
> > +	unsigned int win_pos = 0;
> > +	unsigned int crc;
> > +	int ret;
> > +
> > +	fw_hdr = (const struct ap1302_firmware_header *)ap1302->fw->data;
> 
> Can the firmware be smaller than the struct??

Good point, this needs to be checked in ap1302_request_firmware().

> > +	fw_data = (u8 *)&fw_hdr[1];
> > +	fw_size = ap1302->fw->size - sizeof(*fw_hdr);
> > +
> > +	/* Clear the CRC register. */
> > +	ret = ap1302_write(ap1302, AP1302_SIP_CRC, 0xffff, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Load the PLL initialization settings, set the bootdata stage to 2 to
> > +	 * apply the basic_init_hp settings, and wait 1ms for the PLL to lock.
> > +	 */
> > +	ret = ap1302_write_fw_window(ap1302, fw_data, fw_hdr->pll_init_size,
> > +				     &win_pos);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ap1302_write(ap1302, AP1302_BOOTDATA_STAGE, 0x0002, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	usleep_range(1000, 2000);
> > +
> > +	/* Load the rest of the bootdata content and verify the CRC. */
> > +	ret = ap1302_write_fw_window(ap1302, fw_data + fw_hdr->pll_init_size,
> > +				     fw_size - fw_hdr->pll_init_size, &win_pos);
> > +	if (ret)
> > +		return ret;
> > +
> > +	msleep(40);
> > +
> > +	ret = ap1302_read(ap1302, AP1302_SIP_CRC, &crc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (crc != fw_hdr->crc) {
> > +		dev_warn(ap1302->dev,
> > +			 "CRC mismatch: expected 0x%04x, got 0x%04x\n",
> > +			 fw_hdr->crc, crc);
> > +		return -EAGAIN;
> > +	}
> > +
> > +	/*
> > +	 * Write 0xffff to the bootdata_stage register to indicate to the
> > +	 * AP1302 that the whole bootdata content has been loaded.
> > +	 */
> > +	ret = ap1302_write(ap1302, AP1302_BOOTDATA_STAGE, 0xffff, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* The AP1302 starts outputting frames right after boot, stop it. */
> > +	return ap1302_stall(ap1302, true);
> > +}
> > +
> > +static int ap1302_detect_chip(struct ap1302_device *ap1302)
> > +{
> > +	unsigned int version;
> > +	unsigned int revision;
> > +	int ret;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_CHIP_VERSION, &version);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ap1302_read(ap1302, AP1302_CHIP_REV, &revision);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (version != AP1302_CHIP_ID) {
> > +		dev_err(ap1302->dev,
> > +			"Invalid chip version, expected 0x%04x, got 0x%04x\n",
> > +			AP1302_CHIP_ID, version);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_info(ap1302->dev, "AP1302 revision %u.%u.%u detected\n",
> > +		 (revision & 0xf000) >> 12, (revision & 0x0f00) >> 8,
> > +		 revision & 0x00ff);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ap1302_hw_init(struct ap1302_device *ap1302)
> > +{
> > +	unsigned int retries;
> > +	int ret;
> > +
> > +	/* Request and validate the firmware. */
> > +	ret = ap1302_request_firmware(ap1302);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Power the sensors first, as the firmware will access them once it
> > +	 * gets loaded.
> > +	 */
> > +	ret = ap1302_power_on_sensors(ap1302);
> > +	if (ret < 0)
> > +		goto error_firmware;
> > +
> > +	/*
> > +	 * Load the firmware, retrying in case of CRC errors. The AP1302 is
> > +	 * reset with a full power cycle between each attempt.
> > +	 */
> > +	for (retries = 0; retries < MAX_FW_LOAD_RETRIES; ++retries) {
> > +		ret = ap1302_power_on(ap1302);
> > +		if (ret < 0)
> > +			goto error_power_sensors;
> > +
> > +		ret = ap1302_detect_chip(ap1302);
> > +		if (ret)
> > +			goto error_power;
> > +
> > +		ret = ap1302_load_firmware(ap1302);
> > +		if (!ret)
> > +			break;
> > +
> > +		if (ret != -EAGAIN)
> > +			goto error_power;
> > +
> > +		ap1302_power_off(ap1302);
> > +	}
> > +
> > +	if (retries == MAX_FW_LOAD_RETRIES) {
> > +		dev_err(ap1302->dev,
> > +			"Firmware load retries exceeded, aborting\n");
> > +		ret = -ETIMEDOUT;
> > +		goto error_power_sensors;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_power:
> > +	ap1302_power_off(ap1302);
> > +error_power_sensors:
> > +	ap1302_power_off_sensors(ap1302);
> > +error_firmware:
> > +	release_firmware(ap1302->fw);
> > +
> > +	return ret;
> > +}
> > +
> > +static void ap1302_hw_cleanup(struct ap1302_device *ap1302)
> > +{
> > +	ap1302_power_off(ap1302);
> > +	ap1302_power_off_sensors(ap1302);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Probe & Remove
> > + */
> > +
> > +static int ap1302_config_v4l2(struct ap1302_device *ap1302)
> > +{
> > +	struct v4l2_subdev *sd;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	sd = &ap1302->sd;
> > +	sd->dev = ap1302->dev;
> > +	v4l2_i2c_subdev_init(sd, ap1302->client, &ap1302_subdev_ops);
> > +
> > +	strscpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> > +	strlcat(sd->name, ".", sizeof(sd->name));
> > +	strlcat(sd->name, dev_name(ap1302->dev), sizeof(sd->name));
> > +	dev_dbg(ap1302->dev, "name %s\n", sd->name);
> > +
> > +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> > +	sd->internal_ops = &ap1302_subdev_internal_ops;
> > +	sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_ISP;
> > +	sd->entity.ops = &ap1302_media_ops;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->pads); ++i)
> > +		ap1302->pads[i].flags = i == AP1302_PAD_SOURCE
> > +				      ? MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
> > +
> > +	ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(ap1302->pads),
> > +				     ap1302->pads);
> > +	if (ret < 0) {
> > +		dev_err(ap1302->dev, "media_entity_init failed %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->formats); ++i)
> > +		ap1302->formats[i].info = &supported_video_formats[0];
> > +
> > +	ret = ap1302_init_cfg(sd, NULL);
> > +	if (ret < 0)
> > +		goto error_media;
> > +
> > +	ret = ap1302_ctrls_init(ap1302);
> > +	if (ret < 0)
> > +		goto error_media;
> > +
> > +	ret = v4l2_async_register_subdev(sd);
> > +	if (ret < 0) {
> > +		dev_err(ap1302->dev, "v4l2_async_register_subdev failed %d\n", ret);
> > +		goto error_ctrls;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_ctrls:
> > +	ap1302_ctrls_cleanup(ap1302);
> > +error_media:
> > +	media_entity_cleanup(&sd->entity);
> > +	return ret;
> > +}
> > +
> > +static int ap1302_parse_of(struct ap1302_device *ap1302)
> > +{
> > +	struct device_node *sensors;
> > +	struct device_node *node;
> > +	struct fwnode_handle *ep;
> > +	unsigned int num_sensors = 0;
> > +	const char *model;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/* Clock */
> > +	ap1302->clock = devm_clk_get(ap1302->dev, NULL);
> > +	if (IS_ERR(ap1302->clock)) {
> > +		dev_err(ap1302->dev, "Failed to get clock: %ld\n",
> > +			PTR_ERR(ap1302->clock));
> > +		return PTR_ERR(ap1302->clock);
> > +	}
> > +
> > +	/* GPIOs */
> > +	ap1302->reset_gpio = devm_gpiod_get(ap1302->dev, "reset",
> > +					    GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ap1302->reset_gpio)) {
> > +		dev_err(ap1302->dev, "Can't get reset GPIO: %ld\n",
> > +			PTR_ERR(ap1302->reset_gpio));
> > +		return PTR_ERR(ap1302->reset_gpio);
> > +	}
> > +
> > +	ap1302->standby_gpio = devm_gpiod_get_optional(ap1302->dev, "standby",
> > +						       GPIOD_OUT_LOW);
> > +	if (IS_ERR(ap1302->standby_gpio)) {
> > +		dev_err(ap1302->dev, "Can't get standby GPIO: %ld\n",
> > +			PTR_ERR(ap1302->standby_gpio));
> > +		return PTR_ERR(ap1302->standby_gpio);
> > +	}
> > +
> > +	/* Bus configuration */
> > +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(ap1302->dev), NULL);
> 
> Could you use fwnode_graph_get_endpoint_by_id()? That way you can specify
> the port, too.
> 
> > +	if (!ep)
> > +		return -EINVAL;
> > +
> > +	ap1302->bus_cfg.bus_type = V4L2_MBUS_CSI2_DPHY;
> > +
> > +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &ap1302->bus_cfg);
> > +	if (ret < 0) {
> > +		dev_err(ap1302->dev, "Failed to parse bus configuration\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Sensors */
> > +	sensors = of_get_child_by_name(ap1302->dev->of_node, "sensors");
> 
> You could use dev_of_node() here.
> 
> > +	if (!sensors) {
> > +		dev_err(ap1302->dev, "'sensors' child node not found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = of_property_read_string(sensors, "onnn,model", &model);
> > +	if (ret < 0) {
> > +		/*
> > +		 * If no sensor is connected, we can still support operation
> > +		 * with the test pattern generator.
> > +		 */
> > +		ap1302->sensor_info = &ap1302_sensor_info_tpg;
> > +		ap1302->width_factor = 1;
> > +		ret = 0;
> > +		goto done;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302_sensor_info); ++i) {
> > +		const struct ap1302_sensor_info *info =
> > +			&ap1302_sensor_info[i];
> > +
> > +		if (!strcmp(info->model, model)) {
> > +			ap1302->sensor_info = info;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!ap1302->sensor_info) {
> > +		dev_warn(ap1302->dev, "Unsupported sensor model %s\n", model);
> > +		ret = -EINVAL;
> > +		goto done;
> > +	}
> > +
> > +	for_each_child_of_node(sensors, node) {
> > +		if (of_node_name_eq(node, "sensor")) {
> > +			if (!ap1302_sensor_parse_of(ap1302, node))
> > +				num_sensors++;
> > +		}
> > +	}
> > +
> > +	if (!num_sensors) {
> > +		dev_err(ap1302->dev, "No sensor found\n");
> > +		ret = -EINVAL;
> > +		goto done;
> > +	}
> > +
> > +	ap1302->width_factor = num_sensors;
> > +
> > +done:
> > +	of_node_put(sensors);
> > +	return ret;
> > +}
> > +
> > +static void ap1302_cleanup(struct ap1302_device *ap1302)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->sensors); ++i) {
> > +		struct ap1302_sensor *sensor = &ap1302->sensors[i];
> > +
> > +		if (!sensor->ap1302)
> > +			continue;
> > +
> > +		ap1302_sensor_cleanup(sensor);
> > +	}
> > +
> > +	v4l2_fwnode_endpoint_free(&ap1302->bus_cfg);
> > +
> > +	mutex_destroy(&ap1302->lock);
> > +}
> > +
> > +static int ap1302_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > +{
> > +	struct ap1302_device *ap1302;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	ap1302 = devm_kzalloc(&client->dev, sizeof(*ap1302), GFP_KERNEL);
> > +	if (!ap1302)
> > +		return -ENOMEM;
> > +
> > +	ap1302->dev = &client->dev;
> > +	ap1302->client = client;
> > +
> > +	mutex_init(&ap1302->lock);
> > +
> > +	ap1302->regmap16 = devm_regmap_init_i2c(client, &ap1302_reg16_config);
> > +	if (IS_ERR(ap1302->regmap16)) {
> > +		dev_err(ap1302->dev, "regmap16 init failed: %ld\n",
> > +			PTR_ERR(ap1302->regmap16));
> > +		ret = -ENODEV;
> > +		goto error;
> > +	}
> > +
> > +	ap1302->regmap32 = devm_regmap_init_i2c(client, &ap1302_reg32_config);
> > +	if (IS_ERR(ap1302->regmap32)) {
> > +		dev_err(ap1302->dev, "regmap32 init failed: %ld\n",
> > +			PTR_ERR(ap1302->regmap32));
> > +		ret = -ENODEV;
> > +		goto error;
> > +	}
> > +
> > +	ret = ap1302_parse_of(ap1302);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ap1302->sensors); ++i) {
> > +		struct ap1302_sensor *sensor = &ap1302->sensors[i];
> > +
> > +		if (!sensor->ap1302)
> > +			continue;
> > +
> > +		ret = ap1302_sensor_init(sensor, i);
> > +		if (ret < 0)
> > +			goto error;
> > +	}
> > +
> > +	ret = ap1302_hw_init(ap1302);
> > +	if (ret)
> > +		goto error;
> > +
> > +	ap1302_debugfs_init(ap1302);
> > +
> > +	ret = ap1302_config_v4l2(ap1302);
> > +	if (ret)
> > +		goto error_hw_cleanup;
> > +
> > +	return 0;
> > +
> > +error_hw_cleanup:
> > +	ap1302_hw_cleanup(ap1302);
> > +error:
> > +	ap1302_cleanup(ap1302);
> > +	return ret;
> > +}
> > +
> > +static int ap1302_remove(struct i2c_client *client)
> > +{
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ap1302_device *ap1302 = to_ap1302(sd);
> > +
> > +	ap1302_debugfs_cleanup(ap1302);
> > +
> > +	ap1302_hw_cleanup(ap1302);
> > +
> > +	release_firmware(ap1302->fw);
> > +
> > +	v4l2_async_unregister_subdev(sd);
> > +	media_entity_cleanup(&sd->entity);
> > +
> > +	ap1302_ctrls_cleanup(ap1302);
> > +
> > +	ap1302_cleanup(ap1302);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ap1302_of_id_table[] = {
> > +	{ .compatible = "onnn,ap1302" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ap1302_of_id_table);
> > +
> > +static struct i2c_driver ap1302_i2c_driver = {
> > +	.driver = {
> > +		.name	= DRIVER_NAME,
> > +		.of_match_table	= ap1302_of_id_table,
> > +	},
> > +	.probe		= ap1302_probe,
> > +	.remove		= ap1302_remove,
> > +};
> > +
> > +module_i2c_driver(ap1302_i2c_driver);
> > +
> > +MODULE_AUTHOR("Florian Rebaudo <frebaudo@xxxxxxxxxxx>");
> > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Anil Kumar M <anil.mamidala@xxxxxxxxxx>");
> > +
> > +MODULE_DESCRIPTION("ON Semiconductor AP1302 ISP driver");
> > +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux