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", ®);
> > + 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
