RE: [PATCH v10 3/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver

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

 



Hello Hans,

Thank you for your review.

> -----Original Message-----
> From: Hans Verkuil <hverkuil@xxxxxxxxx>
> Sent: Friday, May 31, 2024 7:51 PM
> To: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>; Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Rafael J .
> Wysocki <rafael.j.wysocki@xxxxxxxxx>; iwamatsu nobuhiro(岩松 信洋 ○DI
> TC□DIT○OST) <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 3/6] media: platform: visconti: Add Toshiba Visconti
> Video Input Interface driver
> 
> Hi Yuji,
> 
> Some comments below:
> 
> On 24/04/2024 04:42, Yuji Ishikawa wrote:
> > Add support to Video Input Interface on Toshiba Visconti ARM SoCs.
> > The interface device includes CSI2 Receiver,
> > frame grabber, video DMAC and image signal processor.
> >
> > A driver instance provides three /dev/videoX device files;
> > one for RGB image capture, another one for optional RGB capture
> > with different parameters and the last one for RAW capture.
> >
> > Through the device files, the driver provides streaming interface.
> > Both DMABUF and MMAP operations are supported.
> > A userland application should feed phisically continuous
> > DMA-BUF instances as capture buffers.
> >
> > The driver is based on media controller framework.
> > Its operations are roughly mapped to three subdrivers;
> > CSI2 receiver subdevice, ISP subdevice and capture devices.
> >
> > The Video DMACs have 32bit address space
> > and currently corresponding IOMMU driver is not provided.
> > Therefore, memory-block address for captured image is 32bit IOVA
> > which is equal to 32bit-truncated phisical address.
> > When the Visconti IOMMU driver (currently under development) is accepted,
> > the hardware layer will use 32bit IOVA mapped by the attached IOMMU.
> >
> > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@xxxxxxxxxxxxx>
> > ---
> > Changelog v2:
> > - Resend v1 because a patch exceeds size limit.
> >
> > Changelog v3:
> > - Adapted to media control framework
> > - Introduced ISP subdevice, capture device
> > - Remove private IOCTLs and add vendor specific V4L2 controls
> > - Change function name avoiding camelcase and uppercase letters
> >
> > Changelog v4:
> > - fix style problems at the v3 patch
> > - remove "index" member
> > - update example
> > - Split patches because the v3 patch exceeds size limit
> > - Stop using ID number to identify driver instance:
> >   - Use dynamically allocated structure to hold driver's context,
> >     instead of static one indexed by ID number.
> >   - internal functions accept context structure instead of ID number.
> > - Use pm_runtime to trigger initialization of HW
> >   along with open/close of device files.
> >
> > Changelog v5:
> > - Fix coding style problems in viif.c
> >
> > Changelog v6:
> > - update dependency description of Kconfig
> > - bugfix: usage of buffer pointed with dma_active
> > - remove unused macros
> > - add viif_common.c for commonly used register buffer control routine
> > - add initialization of Bus Controller (HWAIF) and Memory Protection Unit
> > - removed hwd_ and HWD_ prefix
> > - update source code documentation
> > - Suggestion from Hans Verkuil
> >   - pointer to userland memory is removed from uAPI arguments
> >     - style of structure is now "nested" instead of "chained by pointer";
> >   - use div64_u64 for 64bit division
> >   - define Visconti specific control IDs in v4l2-controls.h
> >   - set proper initial size to v4l2_ctrl_handler_init()
> >   - set all buffers to QUEUED state on an error at start_streaming
> >   - use vb2_is_busy() instead of vb2_is_streaming()
> >   - add parameter check for s->type and s->target in get_selection()
> >   - remove ioctls related to DV format and EDID
> >   - release v4l2 fh instance on and error at opening device file
> >   - support VB2_MMAP mode for streaming operation
> >   - add initial value to each vendor specific control
> >   - GET_LAST_CAPTURE_STATUS control is updated asyncnously from
> workqueue
> >   - applied v4l2-compliance
> > - Suggestion from Sakari Ailus
> >   - use div64_u64 for 64bit division
> >   - update copyright's year
> >   - use common definition of MIPI CSI2 DataTypes
> >   - remove redandunt cast
> >   - use bool instead of HWD_VIIF_ENABLE/DISABLE
> >   - simplify comparison to 0
> >   - simplify statements with trigram operator
> >   - remove redundant local variables
> >   - simplify timeout loop
> >   - use general integer types instead of u32/s32
> > - Suggestion from Laurent Pinchart
> >   - moved VIIF driver to driver/platform/toshiba/visconti
> >   - add CSI2RX subdevice
> >   - change register access: struct-style to macro-style
> >   - use common definition of MIPI CSI2 DataTypes
> >   - Kconfig: add SPDX header, add V4L2_ASYNC
> >   - remove unused type definitions
> >   - define enums instead of successive macro constants
> >   - remove redundant parenthesis of macro constant
> >   - embed struct hwd_res into struct viif_device
> >   - turn switch-case into table lookup
> >   - use xxx_dma instead of xxx_paddr for variable names of IOVA
> >   - literal value: just 0 instead of 0x0
> >   - use literal 1 or 0 instead of HWD_VIIF_ENABLE, DISABLE for register
> access
> >   - use true or false instead of HWD_VIIF_ENABLE, DISABLE for function
> calls
> >   - remove ioctl request handlers which refers subdevices
> >
> > Changelog v7:
> > - change compatible string to visconti5-viif
> > - remove unused variables
> > - set static to internal functions
> > - Suggestion from kernel test robot <lkp@xxxxxxxxx>
> >   - update references to headers
> >
> > Changelog v8:
> > - bugfix: handling return value of visconti_viiif_parse_dt()
> > - add visconti_viif_subdev_notifier_register() to gather
> >   all operations around v4l2_async_notifier
> > - update for v6.6-rc2
> >   - use v4l2_async_connection instead of v4l2_async_subdev
> >   - aid for devices using subdev active state
> > - add __maybe_unused for runtime_pm callbacks
> > - Suggestion from Krzysztof Kozlowski
> >   - use static initialization of local variable
> >   - use dev_err_probe()
> >   - remove error message for DMA memory allocation failure
> >   - remove unused comment messages
> >   - add error handling at fail of workqueue_create()
> >   - remove redundant mutex for pm_runtime callback routines
> > - Suggestion from Hans Verkuil
> >   - remove pr_info() calls
> >   - build check with media_stage.git
> >   - some lacks for kerneldoc description
> >
> > Changelog v9:
> > - applied sparse checker
> >   - add static qualifier to a file scoped local variable
> >   - expand functions for acquiring/releasing locks
> > - bugfix: use NULL (instead of 0) for pad::get_fmt subdevice API
> > - fix warnings for cast between ptr and dma_addr_t
> > - call div64_u64 for 64bit division
> > - rebase to media_staging tree; update Visconti specific control IDs
> >
> > Changelog v10:
> > - remove vendor specific compound controls
> > - remove "rawpack mode" flag
> >   - RAW16, RAW18, RAW20 (to be implemented and tested) should be used
> instead
> > - catch up to v6.9-rc4
> >
> 
> <snip>
> 
> > diff --git a/drivers/media/platform/toshiba/visconti/viif_capture.c
> b/drivers/media/platform/toshiba/visconti/viif_capture.c
> > new file mode 100644
> > index 0000000000..221b9a1ba3
> > --- /dev/null
> > +++ b/drivers/media/platform/toshiba/visconti/viif_capture.c
> > @@ -0,0 +1,1472 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > +/* Toshiba Visconti Video Capture Support
> > + *
> > + * (C) Copyright 2023 TOSHIBA CORPORATION
> > + * (C) Copyright 2023 Toshiba Electronic Devices & Storage Corporation
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/pm_runtime.h>
> > +#include <media/v4l2-common.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include "viif.h"
> > +#include "viif_capture.h"
> > +#include "viif_common.h"
> > +#include "viif_isp.h"
> > +#include "viif_regs.h"
> > +
> > +/* single plane for RGB/Grayscale types, 3 planes for YUV types */
> > +#define VIIF_MAX_PLANE_NUM 3
> > +
> > +/* maximum horizontal/vertical position/dimension of CROP with ISP */
> > +#define VIIF_CROP_MAX_X_ISP 8062U
> > +#define VIIF_CROP_MAX_Y_ISP 3966U
> > +#define VIIF_CROP_MAX_W_ISP 8190U
> > +#define VIIF_CROP_MAX_H_ISP 4094U
> > +
> > +/* minimum horizontal/vertical dimension of CROP */
> > +#define VIIF_CROP_MIN_W 128U
> > +#define VIIF_CROP_MIN_H 128U
> > +
> > +/* maximum output size with ISP */
> > +#define VIIF_MAX_OUTPUT_IMG_WIDTH_ISP  5760U
> > +#define VIIF_MAX_OUTPUT_IMG_HEIGHT_ISP 3240U
> > +#define VIIF_MAX_PITCH_ISP	       32704U
> > +
> > +/* maximum output size for SUB path */
> > +#define VIIF_MAX_OUTPUT_IMG_WIDTH_SUB  4096U
> > +#define VIIF_MAX_OUTPUT_IMG_HEIGHT_SUB 2160U
> > +#define VIIF_MAX_PITCH		       65536U
> > +
> > +/* minimum output size */
> > +#define VIIF_MIN_OUTPUT_IMG_WIDTH  128U
> > +#define VIIF_MIN_OUTPUT_IMG_HEIGHT 128U
> > +
> > +/* DMA settings for SUB path */
> > +#define VDMAC_SRAM_BASE_ADDR_W03 0x440U
> > +#define SRAM_SIZE_W_PORT	 0x200
> > +
> > +enum viif_color_format {
> > +	VIIF_YCBCR422_8_PACKED = 0,
> > +	VIIF_RGB888_PACKED = 1U,
> > +	VIIF_ARGB8888_PACKED = 3U,
> > +	VIIF_YCBCR422_8_PLANAR = 8U,
> > +	VIIF_RGB888_YCBCR444_8_PLANAR = 9U,
> > +	VIIF_ONE_COLOR_8 = 11U,
> > +	VIIF_YCBCR422_16_PLANAR = 12U,
> > +	VIIF_RGB161616_YCBCR444_16_PLANAR = 13U,
> > +	VIIF_ONE_COLOR_16 = 15U
> > +};
> > +
> > +/**
> > + * struct viif_csc_param - color conversion information
> > + * @r_cr_in_offset: input offset of R/Cr
> > + * @g_y_in_offset: input offset of G/Y
> > + * @b_cb_in_offset: input offset of B/Cb
> > + * @coef: coefficient of matrix.
> > + * @r_cr_out_offset: output offset of R/Cr
> > + * @g_y_out_offset: output offset of G/Y
> > + * @b_cb_out_offset: output offset of B/Cb
> > + *
> > + * Range of parameters is:
> > + *
> > + * - {r_cr,g_y,b_cb}_{in,out}_offset
> > + *
> > + *   - Range: [0x0..0x1FFFF]
> > + *
> > + * - coef
> > + *
> > + *   - Range: [0x0..0xFFFF]
> > + *   - [0] : c00(YG_YG), [1] : c01(UB_YG), [2] : c02(VR_YG),
> > + *   - [3] : c10(YG_UB), [4] : c11(UB_UB), [5] : c12(VR_UB),
> > + *   - [6] : c20(YG_VR), [7] : c21(UB_VR), [8] : c22(VR_VR)
> > + */
> > +struct viif_csc_param {
> > +	u32 r_cr_in_offset;
> > +	u32 g_y_in_offset;
> > +	u32 b_cb_in_offset;
> > +	u32 coef[9];
> > +	u32 r_cr_out_offset;
> > +	u32 g_y_out_offset;
> > +	u32 b_cb_out_offset;
> > +};
> > +
> > +/**
> > + * struct viif_pixelmap - pixelmap information
> > + * @pmap_dma: start address of pixel data(DMA address). 4byte alignment.
> > + * @pitch: pitch size of pixel map [unit: byte]
> > + *
> > + * Condition of pitch in case of L2ISP output is as below.
> > + *
> > + * * max: 32704
> > + * * min: max (active width of image * k / r, 128)
> > + * * alignment: 64
> > + *
> > + * Condition of pitch in the other cases is as below.
> > + *
> > + * * max: 65536
> > + * * min: active width of image * k / r
> > + * * alignment: 4
> > + *
> > + * k is the size of 1 pixel and the value is as below.
> > + *
> > + * * VIIF_YCBCR422_8_PACKED: 2
> > + * * VIIF_RGB888_PACKED: 3
> > + * * VIIF_ARGB8888_PACKED: 4
> > + * * VIIF_YCBCR422_8_PLANAR: 1
> > + * * VIIF_RGB888_YCBCR444_8_PLANAR: 1
> > + * * VIIF_ONE_COLOR_8: 1
> > + * * VIIF_YCBCR422_16_PLANAR: 2
> > + * * VIIF_RGB161616_YCBCR444_16_PLANAR: 2
> > + * * VIIF_ONE_COLOR_16: 2
> > + *
> > + * r is the correction factor for Cb or Cr of YCbCr422 planar and the value is
> as below.
> > + *
> > + * * YCbCr422 Cb-planar: 2
> > + * * YCbCr422 Cr-planar: 2
> > + * * others: 1
> > + */
> > +struct viif_pixelmap {
> > +	dma_addr_t pmap_dma;
> > +	u32 pitch;
> > +};
> > +
> > +/**
> > + * struct viif_img - image information
> > + * @width: active width of image [unit: pixel]
> > + * * Range: [128..5760](output from L2ISP)
> > + * * Range: [128..4096](output from SUB unit)
> > + * * The value should be even.
> > + *
> > + * @height: active height of image[line]
> > + * * Range: [128..3240](output from L2ISP)
> > + * * Range: [128..2160](output from SUB unit)
> > + * * The value should be even.
> > + *
> > + * @format: viif_color_format "color format"
> > + * * Below color formats are supported for input and output of MAIN unit
> > + * * VIIF_YCBCR422_8_PACKED
> > + * * VIIF_RGB888_PACKED
> > + * * VIIF_ARGB8888_PACKED
> > + * * VIIF_YCBCR422_8_PLANAR
> > + * * VIIF_RGB888_YCBCR444_8_PLANAR
> > + * * VIIF_ONE_COLOR_8
> > + * * VIIF_YCBCR422_16_PLANAR
> > + * * VIIF_RGB161616_YCBCR444_16_PLANAR
> > + * * VIIF_ONE_COLOR_16
> > + * * Below color formats are supported for output of SUB unit
> > + * * VIIF_ONE_COLOR_8
> > + * * VIIF_ONE_COLOR_16
> > + *
> > + * @pixelmap: pixelmap information
> > + * * [0]: Y/G-planar, packed/Y/RAW
> > + * * [1]: Cb/B-planar
> > + * * [2]: Cr/R-planar
> > + */
> > +struct viif_img {
> > +	u32 width;
> > +	u32 height;
> > +	enum viif_color_format format;
> > +	struct viif_pixelmap pixelmap[VIIF_MAX_PLANE_NUM];
> > +};
> > +
> >
> +/*=============================================*/
> > +/* Low Layer Implementation */
> >
> +/*=============================================*/
> > +/**
> > + * viif_l2_set_output_csc() - Set output CSC parameters of L2ISP
> > + *
> > + * @viif_dev: the VIIF device
> > + * @post_id: POST ID. Range: [0..1]
> > + * @param: Pointer to output csc parameters of L2ISP
> > + */
> > +static int viif_l2_set_output_csc(struct viif_device *viif_dev, u32 post_id,
> > +				  const struct viif_csc_param *param)
> > +{
> > +	/* disable csc matrix when param is NULL */
> > +	if (!param) {
> > +		viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB(post_id), 0);
> > +		return 0;
> > +	}
> > +
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_YG_OFFSETI(post_id),
> > +			   param->g_y_in_offset);
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_YG1(post_id),
> > +			   FIELD_CSC_MTB_LOWER(param->coef[0]));
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_YG2(post_id),
> > +			   FIELD_CSC_MTB_UPPER(param->coef[1]) |
> > +
> FIELD_CSC_MTB_LOWER(param->coef[2]));
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_YG_OFFSETO(post_id),
> > +			   param->g_y_out_offset);
> > +
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CB_OFFSETI(post_id),
> > +			   param->b_cb_in_offset);
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CB1(post_id),
> > +			   FIELD_CSC_MTB_LOWER(param->coef[3]));
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CB2(post_id),
> > +			   FIELD_CSC_MTB_UPPER(param->coef[4]) |
> > +
> FIELD_CSC_MTB_LOWER(param->coef[5]));
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CB_OFFSETO(post_id),
> > +			   param->b_cb_out_offset);
> > +
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CR_OFFSETI(post_id),
> > +			   param->r_cr_in_offset);
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CR1(post_id),
> > +			   FIELD_CSC_MTB_LOWER(param->coef[6]));
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CR2(post_id),
> > +			   FIELD_CSC_MTB_UPPER(param->coef[7]) |
> > +
> FIELD_CSC_MTB_LOWER(param->coef[8]));
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_CSC_MTB_CR_OFFSETO(post_id),
> > +			   param->r_cr_out_offset);
> > +
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_CSC_MTB(post_id), 1);
> > +
> > +	return 0;
> > +}
> > +
> > +struct viif_out_format_spec {
> > +	int num_planes;
> > +	int bytes_per_px;
> > +	int pitch_align;
> > +	int skips_px[3];
> > +};
> > +
> > +static struct viif_out_format_spec out_format_spec[] = {
> > +	[VIIF_YCBCR422_8_PACKED] = {
> > +		.num_planes = 1,
> > +		.bytes_per_px = 2,
> > +		.pitch_align = 256,
> > +		.skips_px = {1},
> > +	},
> > +	[VIIF_RGB888_PACKED] = {
> > +		.num_planes = 1,
> > +		.bytes_per_px = 3,
> > +		.pitch_align = 384,
> > +		.skips_px = {1},
> > +	},
> > +	[VIIF_ARGB8888_PACKED] = {
> > +		.num_planes = 1,
> > +		.bytes_per_px = 4,
> > +		.pitch_align = 512,
> > +		.skips_px = {1},
> > +	},
> > +	[VIIF_ONE_COLOR_8] = {
> > +		.num_planes = 1,
> > +		.bytes_per_px = 1,
> > +		.pitch_align = 128,
> > +		.skips_px = {1},
> > +	},
> > +	[VIIF_ONE_COLOR_16] = {
> > +		.num_planes = 1,
> > +		.bytes_per_px = 2,
> > +		.pitch_align = 128,
> > +		.skips_px = {1},
> > +	},
> > +	[VIIF_YCBCR422_8_PLANAR] = {
> > +		.num_planes = 3,
> > +		.bytes_per_px = 1,
> > +		.pitch_align = 128,
> > +		.skips_px = {1, 2, 2},
> > +	},
> > +	[VIIF_RGB888_YCBCR444_8_PLANAR] = {
> > +		.num_planes = 3,
> > +		.bytes_per_px = 1,
> > +		.pitch_align = 128,
> > +		.skips_px = {1, 1, 1},
> > +	},
> > +	[VIIF_YCBCR422_16_PLANAR] = {
> > +		.num_planes = 3,
> > +		.bytes_per_px = 2,
> > +		.pitch_align = 128,
> > +		.skips_px = {1, 2, 2},
> > +	},
> > +	[VIIF_RGB161616_YCBCR444_16_PLANAR] = {
> > +		.num_planes = 3,
> > +		.bytes_per_px = 2,
> > +		.pitch_align = 128,
> > +		.skips_px = {1, 1, 1}
> > +	}
> > +};
> > +
> > +/**
> > + * viif_l2_set_img_transmission() - Set image transfer condition of L2ISP
> > + *
> > + * @viif_dev: the VIIF device
> > + * @post_id: POST ID. Range: [0..1]
> > + * @enable: set True to enable image transfer of MAIN unit.
> > + * @src: Pointer to crop area information
> > + * @out_process: Pointer to output process information
> > + * @img: Pointer to output image information
> > + *
> > + * see also: #viif_l2_set_roi_path
> > + */
> > +static int viif_l2_set_img_transmission(struct viif_device *viif_dev, u32
> post_id, bool enable,
> > +					const struct viif_img_area *src,
> > +					const struct viif_out_process
> *out_process,
> > +					const struct viif_img *img)
> > +{
> > +	dma_addr_t img_start_addr[VIIF_MAX_PLANE_NUM];
> > +	u32 pitch[VIIF_MAX_PLANE_NUM];
> > +	struct viif_out_format_spec *spec;
> > +	unsigned int i;
> > +
> > +	/* pitch alignment for planar or one color format */
> > +	if (post_id >= VIIF_MAX_POST_NUM || (enable && (!src
> || !out_process)) ||
> > +	    (!enable && (src || out_process))) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* DISABLE: no DMA transmission setup, set minimum crop rectangle
> */
> > +	if (!enable) {
> > +		viif_dev->l2_roi_path_info.post_enable_flag[post_id] = false;
> > +		viif_dev->l2_roi_path_info.post_crop_x[post_id] = 0U;
> > +		viif_dev->l2_roi_path_info.post_crop_y[post_id] = 0U;
> > +		viif_dev->l2_roi_path_info.post_crop_w[post_id] =
> VIIF_CROP_MIN_W;
> > +		viif_dev->l2_roi_path_info.post_crop_h[post_id] =
> VIIF_CROP_MIN_H;
> > +		visconti_viif_l2_set_roi_path(viif_dev);
> > +
> > +		return 0;
> > +	}
> > +
> > +	if (out_process->select_color != VIIF_COLOR_Y_G &&
> > +	    out_process->select_color != VIIF_COLOR_U_B &&
> > +	    out_process->select_color != VIIF_COLOR_V_R &&
> > +	    out_process->select_color != VIIF_COLOR_YUV_RGB) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (img->format != VIIF_ARGB8888_PACKED && out_process->alpha)
> > +		return -EINVAL;
> > +
> > +	if ((img->width % 2U) || (img->height % 2U) || img->width <
> VIIF_MIN_OUTPUT_IMG_WIDTH ||
> > +	    img->height < VIIF_MIN_OUTPUT_IMG_HEIGHT ||
> > +	    img->width > VIIF_MAX_OUTPUT_IMG_WIDTH_ISP ||
> > +	    img->height > VIIF_MAX_OUTPUT_IMG_HEIGHT_ISP) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (src->x > VIIF_CROP_MAX_X_ISP || src->y >
> VIIF_CROP_MAX_Y_ISP ||
> > +	    src->w < VIIF_CROP_MIN_W || src->w > VIIF_CROP_MAX_W_ISP
> || src->h < VIIF_CROP_MIN_H ||
> > +	    src->h > VIIF_CROP_MAX_H_ISP) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (out_process->half_scale) {
> > +		if ((src->w != (img->width * 2U)) || (src->h != (img->height *
> 2U)))
> > +			return -EINVAL;
> > +	} else {
> > +		if (src->w != img->width || src->h != img->height)
> > +			return -EINVAL;
> > +	}
> > +
> > +	if (out_process->select_color == VIIF_COLOR_Y_G ||
> > +	    out_process->select_color == VIIF_COLOR_U_B ||
> > +	    out_process->select_color == VIIF_COLOR_V_R) {
> > +		if (img->format != VIIF_ONE_COLOR_8 && img->format !=
> VIIF_ONE_COLOR_16)
> > +			return -EINVAL;
> > +	}
> > +
> > +	spec = &out_format_spec[img->format];
> > +	if (!spec->num_planes)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < spec->num_planes; i++) {
> > +		img_start_addr[i] = (u32)img->pixelmap[i].pmap_dma;
> > +		pitch[i] = img->pixelmap[i].pitch;
> > +	}
> > +
> > +	for (i = 0; i < spec->num_planes; i++) {
> > +		u32 pitch_req = max(((img->width * spec->bytes_per_px) /
> spec->skips_px[i]), 128U);
> > +
> > +		if (pitch[i] < pitch_req || pitch[i] > VIIF_MAX_PITCH_ISP ||
> > +		    (pitch[i] % spec->pitch_align) || (img_start_addr[i] % 4U))
> {
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_OUT_STADR_G(post_id),
> (u32)img_start_addr[0]);
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_OUT_PITCH_G(post_id),
> pitch[0]);
> > +	if (spec->num_planes == 3) {
> > +		viif_capture_write(viif_dev,
> REG_L2_POST_X_OUT_STADR_B(post_id),
> > +				   (u32)img_start_addr[1]);
> > +		viif_capture_write(viif_dev,
> REG_L2_POST_X_OUT_STADR_R(post_id),
> > +				   (u32)img_start_addr[2]);
> > +		viif_capture_write(viif_dev,
> REG_L2_POST_X_OUT_PITCH_B(post_id), pitch[1]);
> > +		viif_capture_write(viif_dev,
> REG_L2_POST_X_OUT_PITCH_R(post_id), pitch[2]);
> > +	}
> > +
> > +	/* Set CROP */
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_CAP_OFFSET(post_id),
> (src->y << 16U) | src->x);
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_CAP_SIZE(post_id),
> (src->h << 16U) | src->w);
> > +
> > +	/* Set output process */
> > +	viif_capture_write(viif_dev,
> REG_L2_POST_X_HALF_SCALE_EN(post_id),
> > +			   out_process->half_scale ? 1 : 0);
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_C_SELECT(post_id),
> out_process->select_color);
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_OPORTALP(post_id),
> (u32)out_process->alpha);
> > +	viif_capture_write(viif_dev, REG_L2_POST_X_OPORTFMT(post_id),
> img->format);
> > +
> > +	/* Update ROI area and input to each POST */
> > +	viif_dev->l2_roi_path_info.post_enable_flag[post_id] = true;
> > +	viif_dev->l2_roi_path_info.post_crop_x[post_id] = src->x;
> > +	viif_dev->l2_roi_path_info.post_crop_y[post_id] = src->y;
> > +	viif_dev->l2_roi_path_info.post_crop_w[post_id] = src->w;
> > +	viif_dev->l2_roi_path_info.post_crop_h[post_id] = src->h;
> > +	visconti_viif_l2_set_roi_path(viif_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * viif_sub_set_img_transmission() - Set image transfer condition of SUB
> unit
> > + *
> > + * @viif_dev: the VIIF device
> > + * @img: Pointer to output image information
> > + */
> > +static int viif_sub_set_img_transmission(struct viif_device *viif_dev, const
> struct viif_img *img)
> > +{
> > +	dma_addr_t img_start_addr, img_end_addr;
> > +	u32 data_width, pitch, height;
> > +	u32 bytes_px, port_control;
> > +
> > +	/* disable VDMAC when img is NULL */
> > +	if (!img) {
> > +		viif_capture_write(viif_dev, REG_IPORTS_IMGEN, 0);
> > +		port_control = ~((u32)1U << 3U) & viif_capture_read(viif_dev,
> REG_VDM_W_ENABLE);
> > +		viif_capture_write(viif_dev, REG_VDM_W_ENABLE,
> port_control);
> > +		return 0;
> > +	}
> > +
> > +	if ((img->width % 2U) || (img->height % 2U))
> > +		return -EINVAL;
> > +
> > +	if (img->width < VIIF_MIN_OUTPUT_IMG_WIDTH || img->height <
> VIIF_MIN_OUTPUT_IMG_HEIGHT ||
> > +	    img->width > VIIF_MAX_OUTPUT_IMG_WIDTH_SUB ||
> > +	    img->height > VIIF_MAX_OUTPUT_IMG_HEIGHT_SUB) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	img_start_addr = (u32)img->pixelmap[0].pmap_dma;
> > +	pitch = img->pixelmap[0].pitch;
> > +	height = img->height;
> > +
> > +	switch (img->format) {
> > +	case VIIF_ONE_COLOR_8:
> > +		data_width = 0U;
> > +		img_end_addr = img_start_addr + img->width - 1U;
> > +		bytes_px = 1;
> > +		break;
> > +	case VIIF_ONE_COLOR_16:
> > +		data_width = 1U;
> > +		img_end_addr = img_start_addr + (img->width * 2U) - 1U;
> > +		bytes_px = 2;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (img_start_addr % 4U)
> > +		return -EINVAL;
> > +
> > +	if ((pitch < (img->width * bytes_px)) || pitch > VIIF_MAX_PITCH ||
> (pitch % 4U))
> > +		return -EINVAL;
> > +
> > +	viif_capture_write(viif_dev,
> REG_VDM_WPORT_X_W_SRAM_BASE(IDX_WPORT_SUB_IMG),
> > +			   VDMAC_SRAM_BASE_ADDR_W03);
> > +	viif_capture_write(viif_dev,
> REG_VDM_WPORT_X_W_SRAM_SIZE(IDX_WPORT_SUB_IMG),
> > +			   SRAM_SIZE_W_PORT);
> > +	viif_capture_write(viif_dev,
> REG_VDM_WPORT_X_W_STADR(IDX_WPORT_SUB_IMG),
> > +			   (u32)img_start_addr);
> > +	viif_capture_write(viif_dev,
> REG_VDM_WPORT_X_W_ENDADR(IDX_WPORT_SUB_IMG),
> > +			   (u32)img_end_addr);
> > +	viif_capture_write(viif_dev,
> REG_VDM_WPORT_X_W_HEIGHT(IDX_WPORT_SUB_IMG), height);
> > +	viif_capture_write(viif_dev,
> REG_VDM_WPORT_X_W_PITCH(IDX_WPORT_SUB_IMG), pitch);
> > +	viif_capture_write(viif_dev,
> REG_VDM_WPORT_X_W_CFG0(IDX_WPORT_SUB_IMG), data_width << 8U);
> > +	port_control = BIT(3) | viif_capture_read(viif_dev,
> REG_VDM_W_ENABLE);
> > +	viif_capture_write(viif_dev, REG_VDM_W_ENABLE, port_control);
> > +	viif_capture_write(viif_dev, REG_IPORTS_IMGEN, 1);
> > +
> > +	return 0;
> > +}
> > +
> >
> +/*=============================================*/
> > +/* handling V4L2 framework */
> >
> +/*=============================================*/
> > +struct viif_buffer {
> > +	struct vb2_v4l2_buffer vb;
> > +	struct list_head queue;
> > +};
> > +
> > +static inline struct viif_buffer *vb2_to_viif(struct vb2_v4l2_buffer *vbuf)
> > +{
> > +	return container_of(vbuf, struct viif_buffer, vb);
> > +}
> > +
> > +static inline struct cap_dev *video_drvdata_to_capdev(struct file *file)
> > +{
> > +	return (struct cap_dev *)video_drvdata(file);
> > +}
> > +
> > +static inline struct cap_dev *vb2queue_to_capdev(struct vb2_queue *vq)
> > +{
> > +	return (struct cap_dev *)vb2_get_drv_priv(vq);
> > +}
> > +
> > +/* ----- ISRs and VB2 Operations ----- */
> > +static int viif_set_img(struct cap_dev *cap_dev, struct vb2_buffer *vb)
> > +{
> > +	struct v4l2_pix_format_mplane *pix = &cap_dev->v4l2_pix;
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	struct viif_img next_out_img;
> > +	int i, ret = 0;
> > +
> > +	next_out_img.width = pix->width;
> > +	next_out_img.height = pix->height;
> > +	next_out_img.format = cap_dev->out_format;
> > +
> > +	for (i = 0; i < pix->num_planes; i++) {
> > +		next_out_img.pixelmap[i].pitch =
> pix->plane_fmt[i].bytesperline;
> > +		next_out_img.pixelmap[i].pmap_dma =
> vb2_dma_contig_plane_dma_addr(vb, i);
> > +	}
> > +
> > +	if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST0) {
> > +		spin_lock(&viif_dev->regbuf_lock);
> > +		hwd_viif_isp_guard_start(viif_dev);
> > +		ret = viif_l2_set_img_transmission(viif_dev,
> VIIF_L2ISP_POST_0, true,
> > +						   &cap_dev->img_area,
> &cap_dev->out_process,
> > +						   &next_out_img);
> > +		hwd_viif_isp_guard_end(viif_dev);
> > +		spin_unlock(&viif_dev->regbuf_lock);
> > +		if (ret)
> > +			dev_err(viif_dev->dev, "set img error. %d\n", ret);
> > +	} else if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST1) {
> > +		spin_lock(&viif_dev->regbuf_lock);
> > +		hwd_viif_isp_guard_start(viif_dev);
> > +		ret = viif_l2_set_img_transmission(viif_dev,
> VIIF_L2ISP_POST_1, true,
> > +						   &cap_dev->img_area,
> &cap_dev->out_process,
> > +						   &next_out_img);
> > +		hwd_viif_isp_guard_end(viif_dev);
> > +		spin_unlock(&viif_dev->regbuf_lock);
> > +		if (ret)
> > +			dev_err(viif_dev->dev, "set img error. %d\n", ret);
> > +	} else if (cap_dev->pathid == CAPTURE_PATH_SUB) {
> > +		spin_lock(&viif_dev->regbuf_lock);
> > +		hwd_viif_isp_guard_start(viif_dev);
> > +		ret = viif_sub_set_img_transmission(viif_dev, &next_out_img);
> > +		hwd_viif_isp_guard_end(viif_dev);
> > +		spin_unlock(&viif_dev->regbuf_lock);
> > +		if (ret)
> > +			dev_err(viif_dev->dev, "set img error. %d\n", ret);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * viif_capture_switch_buffer() is called from interrupt service routine
> > + * triggered by VSync with some fixed delay.
> > + * The function may switch DMA target buffer by calling viif_set_img().
> > + * The VIIF DMA HW captures the destination address at next VSync
> > + * and completes transfer at one more after.
> > + * Therefore, filled buffer is available at the one after next ISR.
> > + *
> > + * To avoid DMA HW getting stucked, we always need to set valid
> destination address.
> > + * If a prepared buffer is not available, we reuse the buffer currently being
> transferred to.
> > + *
> > + * The cap_dev structure has two pointers and a queue to handle video
> buffers;
> > + + Description of each item at the entry of this function:
> > + * * buf_queue:  holds prepared buffers, set by vb2_queue()
> > + * * active:     pointing at address captured (and to be filled) by DMA HW
> > + * * dma_active: pointing at buffer filled by DMA HW
> > + *
> > + * Rules to update items:
> > + * * when buf_queue is not empty, "active" buffer goes "dma_active"
> > + * * when buf_queue is empty:
> > + *   * "active" buffer stays the same (DMA HW fills the same buffer for
> coming two frames)
> > + *   * "dma_active" gets NULL (filled buffer will be reused; should not go
> "DONE" at next ISR)
> > + *
> > + * Simulation:
> > + * | buf_queue   | active  | dma_active | note |
> > + * | X           | NULL    | NULL       |      |
> > + * <QBUF BUF0>
> > + * | X           | BUF0    | NULL       | BUF0 stays |
> > + * | X           | BUF0    | NULL       | BUF0 stays |
> > + * <QBUF BUF1>
> > + * <QBUF BUF2>
> > + * | BUF2 BUF1   | BUF0    | NULL       |      |
> > + * | BUF2        | BUF1    | BUF0       | BUF0 goes DONE |
> > + * | X           | BUF2    | BUF1       | BUF1 goes DONE, BUF2 stays |
> > + * | X           | BUF2    | NULL       | BUF2 stays |
> > + */
> > +void visconti_viif_capture_switch_buffer(struct cap_dev *cap_dev, u32
> status_err,
> > +					 u32 l2_transfer_status, u64
> timestamp)
> > +{
> > +	spin_lock(&cap_dev->buf_lock);
> > +
> > +	if (cap_dev->dma_active) {
> > +		/* DMA has completed and another framebuffer instance is
> set */
> > +		struct vb2_v4l2_buffer *vbuf = cap_dev->dma_active;
> > +		enum vb2_buffer_state state;
> > +
> > +		cap_dev->buf_cnt--;
> > +		vbuf->vb2_buf.timestamp = timestamp;
> > +		vbuf->sequence = cap_dev->sequence++;
> > +		vbuf->field = V4L2_FIELD_NONE;
> > +		if (status_err || l2_transfer_status)
> > +			state = VB2_BUF_STATE_ERROR;
> > +		else
> > +			state = VB2_BUF_STATE_DONE;
> > +
> > +		vb2_buffer_done(&vbuf->vb2_buf, state);
> > +	}
> > +
> > +	/* QUEUE pop to register an instance as next DMA target; if empty,
> reuse current instance */
> > +	if (!list_empty(&cap_dev->buf_queue)) {
> > +		struct viif_buffer *buf =
> > +			list_entry(cap_dev->buf_queue.next, struct
> viif_buffer, queue);
> > +		list_del_init(&buf->queue);
> > +		viif_set_img(cap_dev, &buf->vb.vb2_buf);
> > +		cap_dev->dma_active = cap_dev->active;
> > +		cap_dev->active = &buf->vb;
> > +	} else {
> > +		cap_dev->dma_active = NULL;
> > +	}
> > +
> > +	spin_unlock(&cap_dev->buf_lock);
> > +}
> > +
> > +/* --- Capture buffer control --- */
> > +static int viif_vb2_setup(struct vb2_queue *vq, unsigned int *count,
> unsigned int *num_planes,
> > +			  unsigned int sizes[], struct device *alloc_devs[])
> > +{
> > +	struct cap_dev *cap_dev = vb2queue_to_capdev(vq);
> > +	struct v4l2_pix_format_mplane *pix = &cap_dev->v4l2_pix;
> > +	unsigned int i;
> > +
> > +	/* num_planes is set: just check plane sizes. */
> > +	if (*num_planes) {
> > +		for (i = 0; i < pix->num_planes; i++)
> > +			if (sizes[i] < pix->plane_fmt[i].sizeimage)
> > +				return -EINVAL;
> > +
> > +		return 0;
> > +	}
> > +
> > +	/* num_planes not set: called from REQBUFS, just set plane sizes. */
> > +	*num_planes = pix->num_planes;
> > +	for (i = 0; i < pix->num_planes; i++)
> > +		sizes[i] = pix->plane_fmt[i].sizeimage;
> > +
> > +	cap_dev->buf_cnt = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +static void viif_vb2_queue(struct vb2_buffer *vb)
> > +{
> > +	struct cap_dev *cap_dev = vb2queue_to_capdev(vb->vb2_queue);
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct viif_buffer *buf = vb2_to_viif(vbuf);
> > +	unsigned long irqflags;
> > +
> > +	spin_lock_irqsave(&cap_dev->buf_lock, irqflags);
> > +
> > +	list_add_tail(&buf->queue, &cap_dev->buf_queue);
> > +	cap_dev->buf_cnt++;
> > +
> > +	spin_unlock_irqrestore(&cap_dev->buf_lock, irqflags);
> > +}
> > +
> > +static int viif_vb2_prepare(struct vb2_buffer *vb)
> > +{
> > +	struct cap_dev *cap_dev = vb2queue_to_capdev(vb->vb2_queue);
> > +	struct v4l2_pix_format_mplane *pix = &cap_dev->v4l2_pix;
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < pix->num_planes; i++) {
> > +		if (vb2_plane_size(vb, i) < pix->plane_fmt[i].sizeimage) {
> > +			dev_err(viif_dev->dev, "Plane size too small (%lu
> < %u)\n",
> > +				vb2_plane_size(vb, i),
> pix->plane_fmt[i].sizeimage);
> 
> Make this dev_info. It is a user-space problem, not a driver/hw error.
> 

I'll use dev_info for this message.

> > +			return -EINVAL;
> > +		}
> > +
> > +		vb2_set_plane_payload(vb, i, pix->plane_fmt[i].sizeimage);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void viif_return_all_buffers(struct cap_dev *cap_dev, enum
> vb2_buffer_state state)
> > +{
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	struct viif_buffer *buf;
> > +	unsigned long irqflags;
> > +
> > +	spin_lock_irqsave(&cap_dev->buf_lock, irqflags);
> > +
> > +	/* buffer control */
> > +	if (cap_dev->active) {
> > +		vb2_buffer_done(&cap_dev->active->vb2_buf, state);
> > +		cap_dev->buf_cnt--;
> > +		cap_dev->active = NULL;
> > +	}
> > +	if (cap_dev->dma_active) {
> > +		vb2_buffer_done(&cap_dev->dma_active->vb2_buf, state);
> > +		cap_dev->buf_cnt--;
> > +		cap_dev->dma_active = NULL;
> > +	}
> > +
> > +	/* Release all queued buffers. */
> > +	list_for_each_entry(buf, &cap_dev->buf_queue, queue) {
> > +		vb2_buffer_done(&buf->vb.vb2_buf,
> VB2_BUF_STATE_ERROR);
> > +		cap_dev->buf_cnt--;
> > +	}
> > +	INIT_LIST_HEAD(&cap_dev->buf_queue);
> > +	if (cap_dev->buf_cnt)
> > +		dev_err(viif_dev->dev, "Buffer count error %d\n",
> cap_dev->buf_cnt);
> > +
> > +	spin_unlock_irqrestore(&cap_dev->buf_lock, irqflags);
> > +}
> > +
> > +static int viif_l2_set_format(struct cap_dev *cap_dev);
> > +static int viif_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > +	struct cap_dev *cap_dev = vb2queue_to_capdev(vq);
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&viif_dev->stream_lock);
> > +
> > +	/* note that pipe is shared among paths; see pipe.streaming_count
> member variable */
> > +	ret = video_device_pipeline_start(&cap_dev->vdev, &viif_dev->pipe);
> > +	if (ret) {
> > +		dev_err(viif_dev->dev, "start pipeline failed %d\n", ret);
> > +		goto release_lock;
> > +	}
> > +
> > +	/* buffer control */
> > +	cap_dev->sequence = 0;
> > +
> > +	if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST0) {
> > +		/* Currently, only path0 (MAIN POST0) initializes ISP and
> Camera */
> > +		/* Possibly, initialization can be done when
> pipe.streaming_count==0 */
> > +		ret = visconti_viif_isp_main_set_unit(viif_dev);
> > +		if (ret) {
> > +			dev_err(viif_dev->dev, "Setting up main path0 L1ISP
> failed %d\n", ret);
> > +			goto config_path_end;
> > +		}
> > +		ret = viif_l2_set_format(cap_dev);
> > +		if (ret) {
> > +			dev_err(viif_dev->dev, "Setting up main path0 L2VDM
> failed %d\n", ret);
> > +			goto config_path_end;
> > +		}
> > +		ret = v4l2_subdev_call(&viif_dev->isp_subdev.sd, video,
> s_stream, true);
> > +		if (ret)
> > +			dev_err(viif_dev->dev, "Start isp subdevice stream
> failed. %d\n", ret);
> > +	} else if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST1) {
> > +		ret = viif_l2_set_format(cap_dev);
> > +		if (ret)
> > +			dev_err(viif_dev->dev, "Setting up main path1 L2VDM
> failed %d\n", ret);
> > +	} else {
> > +		cap_dev->out_format = (cap_dev->v4l2_pix.pixelformat ==
> V4L2_PIX_FMT_SRGGB8) ?
> > +						    VIIF_ONE_COLOR_8 :
> > +						    VIIF_ONE_COLOR_16;
> > +		ret = visconti_viif_isp_sub_set_unit(viif_dev);
> > +		if (ret)
> > +			dev_err(viif_dev->dev, "Setting up sub path
> failed %d\n", ret);
> > +	}
> > +config_path_end:
> > +	if (ret) {
> > +		viif_return_all_buffers(cap_dev, VB2_BUF_STATE_QUEUED);
> > +		video_device_pipeline_stop(&cap_dev->vdev);
> > +		ret = -EPIPE;
> > +	}
> > +release_lock:
> > +	mutex_unlock(&viif_dev->stream_lock);
> > +	return ret;
> > +}
> > +
> > +static void viif_stop_streaming(struct vb2_queue *vq)
> > +{
> > +	struct cap_dev *cap_dev = vb2queue_to_capdev(vq);
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	int ret;
> > +
> > +	mutex_lock(&viif_dev->stream_lock);
> > +
> > +	/* Currently, only path0 (MAIN POST0) stops ISP and Camera */
> > +	/* Possibly, teardown can be done when pipe.streaming_count==0 */
> > +	if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST0) {
> > +		ret = v4l2_subdev_call(&viif_dev->isp_subdev.sd, video,
> s_stream, false);
> > +		if (ret)
> > +			dev_err(viif_dev->dev, "Stop isp subdevice stream
> failed %d\n", ret);
> > +	}
> > +
> > +	viif_return_all_buffers(cap_dev, VB2_BUF_STATE_ERROR);
> > +	video_device_pipeline_stop(&cap_dev->vdev);
> > +	mutex_unlock(&viif_dev->stream_lock);
> > +}
> > +
> > +static const struct vb2_ops viif_vb2_ops = {
> > +	.queue_setup = viif_vb2_setup,
> > +	.buf_queue = viif_vb2_queue,
> > +	.buf_prepare = viif_vb2_prepare,
> > +	.wait_prepare = vb2_ops_wait_prepare,
> > +	.wait_finish = vb2_ops_wait_finish,
> > +	.start_streaming = viif_start_streaming,
> > +	.stop_streaming = viif_stop_streaming,
> > +};
> > +
> > +/* --- VIIF hardware settings --- */
> > +/* L2ISP output csc setting for YUV to RGB(ITU-R BT.709) */
> > +static const struct viif_csc_param viif_csc_yuv2rgb = {
> > +	.r_cr_in_offset = 0x18000,
> > +	.g_y_in_offset = 0x1f000,
> > +	.b_cb_in_offset = 0x18000,
> > +	.coef = {
> > +			[0] = 0x1000,
> > +			[1] = 0xfd12,
> > +			[2] = 0xf8ad,
> > +			[3] = 0x1000,
> > +			[4] = 0x1d07,
> > +			[5] = 0x0000,
> > +			[6] = 0x1000,
> > +			[7] = 0x0000,
> > +			[8] = 0x18a2,
> > +		},
> > +	.r_cr_out_offset = 0x1000,
> > +	.g_y_out_offset = 0x1000,
> > +	.b_cb_out_offset = 0x1000,
> > +};
> > +
> > +/* L2ISP output csc setting for RGB to YUV(ITU-R BT.709) */
> > +static const struct viif_csc_param viif_csc_rgb2yuv = {
> > +	.r_cr_in_offset = 0x1f000,
> > +	.g_y_in_offset = 0x1f000,
> > +	.b_cb_in_offset = 0x1f000,
> > +	.coef = {
> > +			[0] = 0x0b71,
> > +			[1] = 0x0128,
> > +			[2] = 0x0367,
> > +			[3] = 0xf9b1,
> > +			[4] = 0x082f,
> > +			[5] = 0xfe20,
> > +			[6] = 0xf891,
> > +			[7] = 0xff40,
> > +			[8] = 0x082f,
> > +		},
> > +	.r_cr_out_offset = 0x8000,
> > +	.g_y_out_offset = 0x1000,
> > +	.b_cb_out_offset = 0x8000,
> > +};
> > +
> > +static int viif_l2_set_format(struct cap_dev *cap_dev)
> > +{
> > +	struct v4l2_pix_format_mplane *pix = &cap_dev->v4l2_pix;
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	const struct viif_csc_param *csc_param = NULL;
> > +	struct v4l2_subdev_selection sel = {
> > +		.target = V4L2_SEL_TGT_CROP,
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +	};
> > +	struct v4l2_subdev_format fmt = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +	};
> > +	bool inp_is_rgb = false;
> > +	bool out_is_rgb = false;
> > +	u32 postid;
> > +	int ret;
> > +
> > +	/* check path id */
> > +	if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST0) {
> > +		sel.pad = VIIF_ISP_PAD_SRC_PATH0;
> > +		fmt.pad = VIIF_ISP_PAD_SRC_PATH0;
> > +		postid = VIIF_L2ISP_POST_0;
> > +	} else if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST1) {
> > +		sel.pad = VIIF_ISP_PAD_SRC_PATH1;
> > +		fmt.pad = VIIF_ISP_PAD_SRC_PATH1;
> > +		postid = VIIF_L2ISP_POST_1;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	cap_dev->out_process.half_scale = false;
> > +	cap_dev->out_process.select_color = VIIF_COLOR_YUV_RGB;
> > +	cap_dev->out_process.alpha = 0;
> > +
> > +	ret = v4l2_subdev_call(&viif_dev->isp_subdev.sd, pad, get_selection,
> NULL, &sel);
> > +	if (ret) {
> > +		cap_dev->img_area.x = 0;
> > +		cap_dev->img_area.y = 0;
> > +		cap_dev->img_area.w = pix->width;
> > +		cap_dev->img_area.h = pix->height;
> > +	} else {
> > +		cap_dev->img_area.x = sel.r.left;
> > +		cap_dev->img_area.y = sel.r.top;
> > +		cap_dev->img_area.w = sel.r.width;
> > +		cap_dev->img_area.h = sel.r.height;
> > +	}
> > +
> > +	ret = v4l2_subdev_call(&viif_dev->isp_subdev.sd, pad, get_fmt, NULL,
> &fmt);
> > +	if (!ret)
> > +		inp_is_rgb = (fmt.format.code ==
> MEDIA_BUS_FMT_RGB888_1X24);
> > +
> > +	switch (pix->pixelformat) {
> > +	case V4L2_PIX_FMT_RGB24:
> > +		cap_dev->out_format = VIIF_RGB888_PACKED;
> > +		out_is_rgb = true;
> > +		break;
> > +	case V4L2_PIX_FMT_ABGR32:
> > +		cap_dev->out_format = VIIF_ARGB8888_PACKED;
> > +		cap_dev->out_process.alpha = 0xff;
> > +		out_is_rgb = true;
> > +		break;
> > +	case V4L2_PIX_FMT_YUV422M:
> > +		cap_dev->out_format = VIIF_YCBCR422_8_PLANAR;
> > +		break;
> > +	case V4L2_PIX_FMT_YUV444M:
> > +		cap_dev->out_format = VIIF_RGB888_YCBCR444_8_PLANAR;
> > +		break;
> > +	case V4L2_PIX_FMT_Y16:
> > +		cap_dev->out_format = VIIF_ONE_COLOR_16;
> > +		cap_dev->out_process.select_color = VIIF_COLOR_Y_G;
> > +		break;
> > +	}
> > +
> > +	if (!inp_is_rgb && out_is_rgb)
> > +		csc_param = &viif_csc_yuv2rgb; /* YUV -> RGB */
> > +	else if (inp_is_rgb && !out_is_rgb)
> > +		csc_param = &viif_csc_rgb2yuv; /* RGB -> YUV */
> > +
> > +	return viif_l2_set_output_csc(viif_dev, postid, csc_param);
> > +}
> > +
> > +/* --- IOCTL Operations --- */
> > +static const struct viif_fmt viif_capture_fmt_list_mainpath[] = {
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_RGB24,
> > +		.bpp = { 24, 0, 0 },
> > +		.num_planes = 1,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +		.pitch_align = 384,
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_ABGR32,
> > +		.bpp = { 32, 0, 0 },
> > +		.num_planes = 1,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +		.pitch_align = 512,
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_YUV422M,
> > +		.bpp = { 8, 4, 4 },
> > +		.num_planes = 3,
> > +		.colorspace = V4L2_COLORSPACE_REC709,
> > +		.pitch_align = 128,
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_YUV444M,
> > +		.bpp = { 8, 8, 8 },
> > +		.num_planes = 3,
> > +		.colorspace = V4L2_COLORSPACE_REC709,
> > +		.pitch_align = 128,
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_Y16,
> > +		.bpp = { 16, 0, 0 },
> > +		.num_planes = 1,
> > +		.colorspace = V4L2_COLORSPACE_REC709,
> > +		.pitch_align = 128,
> > +	},
> > +};
> > +
> > +static const struct viif_fmt viif_capture_fmt_list_subpath[] = {
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_SRGGB8,
> > +		.bpp = { 8, 0, 0 },
> > +		.num_planes = 1,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +		.pitch_align = 256,
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_SRGGB10,
> > +		.bpp = { 16, 0, 0 },
> > +		.num_planes = 1,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +		.pitch_align = 256,
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_SRGGB12,
> > +		.bpp = { 16, 0, 0 },
> > +		.num_planes = 1,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +		.pitch_align = 256,
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_SRGGB14,
> > +		.bpp = { 16, 0, 0 },
> > +		.num_planes = 1,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +		.pitch_align = 256,
> > +	},
> > +};
> > +
> > +static const struct viif_fmt *get_viif_fmt_from_fourcc(struct cap_dev
> *cap_dev, unsigned int fourcc)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < cap_dev->fmt_size; i++) {
> > +		const struct viif_fmt *fmt = &cap_dev->fmts[i];
> > +
> > +		if (fmt->fourcc == fourcc)
> > +			return fmt;
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static u32 get_pixelformat_from_fourcc(struct cap_dev *cap_dev, unsigned
> int fourcc)
> > +{
> > +	const struct viif_fmt *fmt = get_viif_fmt_from_fourcc(cap_dev, fourcc);
> > +
> > +	return fmt ? fmt->fourcc : cap_dev->fmts[0].fourcc;
> > +}
> > +
> > +static u32 get_pixelformat_from_mbus_code(struct cap_dev *cap_dev,
> unsigned int mbus_code)
> > +{
> > +	const struct viif_fmt *fmt;
> > +	unsigned int fourcc;
> > +
> > +	switch (mbus_code) {
> > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +		fourcc = V4L2_PIX_FMT_SRGGB8;
> > +		break;
> > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +		fourcc = V4L2_PIX_FMT_SRGGB10;
> > +		break;
> > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> > +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> > +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> > +		fourcc = V4L2_PIX_FMT_SRGGB12;
> > +		break;
> > +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> > +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> > +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> > +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> > +		fourcc = V4L2_PIX_FMT_SRGGB14;
> > +		break;
> > +	default:
> > +		return cap_dev->fmts[0].fourcc;
> > +	}
> > +
> > +	fmt = get_viif_fmt_from_fourcc(cap_dev, fourcc);
> > +	return fmt ? fmt->fourcc : cap_dev->fmts[0].fourcc;
> > +}
> > +
> > +static void viif_calc_plane_sizes(struct cap_dev *cap_dev, struct
> v4l2_pix_format_mplane *pix)
> > +{
> > +	const struct viif_fmt *viif_fmt = get_viif_fmt_from_fourcc(cap_dev,
> pix->pixelformat);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < viif_fmt->num_planes; i++) {
> > +		struct v4l2_plane_pix_format *plane_i = &pix->plane_fmt[i];
> > +		unsigned int bpl;
> > +
> > +		memset(plane_i, 0, sizeof(*plane_i));
> > +		bpl = roundup(pix->width * viif_fmt->bpp[i] / 8,
> viif_fmt->pitch_align);
> > +
> > +		plane_i->bytesperline = bpl;
> > +		plane_i->sizeimage = pix->height * bpl;
> > +	}
> > +	pix->num_planes = viif_fmt->num_planes;
> > +}
> > +
> > +static int viif_querycap(struct file *file, void *priv, struct v4l2_capability
> *cap)
> > +{
> > +	struct viif_device *viif_dev = video_drvdata_to_capdev(file)->viif_dev;
> > +
> > +	strscpy(cap->driver, VIIF_DRIVER_NAME, sizeof(cap->driver));
> > +	snprintf(cap->card, sizeof(cap->card), "%s-%s", VIIF_DRIVER_NAME,
> dev_name(viif_dev->dev));
> > +	/* TODO: platform:visconti-viif-0,1,2,3 for each VIIF driver instance */
> > +	snprintf(cap->bus_info, sizeof(cap->bus_info), "%s-0",
> VIIF_BUS_INFO_BASE);
> > +
> > +	return 0;
> > +}
> > +
> > +static int viif_enum_fmt_vid_cap(struct file *file, void *priv, struct
> v4l2_fmtdesc *f)
> > +{
> > +	struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> > +
> > +	if (f->index >= cap_dev->fmt_size)
> > +		return -EINVAL;
> > +
> > +	f->pixelformat = cap_dev->fmts[f->index].fourcc;
> > +	return 0;
> > +}
> > +
> > +static void viif_try_fmt(struct cap_dev *cap_dev, struct
> v4l2_pix_format_mplane *pix)
> > +{
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	struct v4l2_subdev_format format = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +	};
> > +	int ret;
> > +
> > +	/* check path id */
> > +	if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST0)
> > +		format.pad = VIIF_ISP_PAD_SRC_PATH0;
> > +	else if (cap_dev->pathid == CAPTURE_PATH_MAIN_POST1)
> > +		format.pad = VIIF_ISP_PAD_SRC_PATH1;
> > +	else
> > +		format.pad = VIIF_ISP_PAD_SRC_PATH2;
> > +
> > +	pix->field = V4L2_FIELD_NONE;
> > +	pix->colorspace = V4L2_COLORSPACE_DEFAULT;
> > +	pix->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +	pix->quantization = V4L2_QUANTIZATION_DEFAULT;
> > +
> > +	ret = v4l2_subdev_call(&viif_dev->isp_subdev.sd, pad, get_fmt, NULL,
> &format);
> > +	if (ret) {
> > +		/* minimal default format */
> > +		pix->width = VIIF_MIN_OUTPUT_IMG_WIDTH;
> > +		pix->height = VIIF_MIN_OUTPUT_IMG_HEIGHT;
> > +		pix->pixelformat = (cap_dev->pathid ==
> CAPTURE_PATH_SUB) ? V4L2_PIX_FMT_SRGGB8 :
> > +
> 	 V4L2_PIX_FMT_RGB24;
> > +		viif_calc_plane_sizes(cap_dev, pix);
> > +		return;
> > +	}
> > +
> > +	pix->width = format.format.width;
> > +	pix->height = format.format.height;
> > +
> > +	/* check output format */
> > +	if (cap_dev->pathid == CAPTURE_PATH_SUB)
> > +		pix->pixelformat = get_pixelformat_from_mbus_code(cap_dev,
> format.format.code);
> > +	else
> > +		pix->pixelformat = get_pixelformat_from_fourcc(cap_dev,
> pix->pixelformat);
> > +
> > +	/* update derived parameters, such as bpp */
> > +	viif_calc_plane_sizes(cap_dev, pix);
> > +}
> > +
> > +static int viif_try_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format
> *f)
> > +{
> > +	struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> > +
> > +	viif_try_fmt(cap_dev, &f->fmt.pix_mp);
> > +	return 0;
> > +}
> > +
> > +static int viif_s_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format
> *f)
> > +{
> > +	struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> > +
> > +	if (vb2_is_busy(&cap_dev->vb2_vq))
> > +		return -EBUSY;
> > +
> > +	if (f->type != cap_dev->vb2_vq.type)
> > +		return -EINVAL;
> 
> I don't believe this test is needed.
> 

I'll remove this test.

> > +
> > +	viif_try_fmt(cap_dev, &f->fmt.pix_mp);
> > +	cap_dev->v4l2_pix = f->fmt.pix_mp;
> > +
> > +	return 0;
> > +}
> > +
> > +static int viif_g_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format
> *f)
> > +{
> > +	struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> > +
> > +	f->fmt.pix_mp = cap_dev->v4l2_pix;
> > +
> > +	return 0;
> > +}
> > +
> > +static int viif_enum_framesizes(struct file *file, void *fh, struct
> v4l2_frmsizeenum *fsize)
> > +{
> > +	struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> > +
> > +	if (fsize->index)
> > +		return -EINVAL;
> > +
> > +	if (!get_viif_fmt_from_fourcc(cap_dev, fsize->pixel_format))
> > +		return -EINVAL;
> > +
> > +	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> > +	fsize->stepwise.min_width = VIIF_MIN_OUTPUT_IMG_WIDTH;
> > +	fsize->stepwise.max_width = (cap_dev->pathid ==
> CAPTURE_PATH_SUB) ?
> > +
> VIIF_MAX_OUTPUT_IMG_WIDTH_SUB :
> > +
> VIIF_MAX_OUTPUT_IMG_WIDTH_ISP;
> > +	fsize->stepwise.min_height = VIIF_MIN_OUTPUT_IMG_HEIGHT;
> > +	fsize->stepwise.max_height = (cap_dev->pathid ==
> CAPTURE_PATH_SUB) ?
> > +
> VIIF_MAX_OUTPUT_IMG_HEIGHT_SUB :
> > +
> VIIF_MAX_OUTPUT_IMG_HEIGHT_ISP;
> > +	fsize->stepwise.step_width = 1;
> > +	fsize->stepwise.step_height = 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ioctl_ops viif_ioctl_ops = {
> > +	.vidioc_querycap = viif_querycap,
> > +
> > +	.vidioc_enum_fmt_vid_cap = viif_enum_fmt_vid_cap,
> > +	.vidioc_try_fmt_vid_cap_mplane = viif_try_fmt_vid_cap,
> > +	.vidioc_s_fmt_vid_cap_mplane = viif_s_fmt_vid_cap,
> > +	.vidioc_g_fmt_vid_cap_mplane = viif_g_fmt_vid_cap,
> > +
> > +	.vidioc_enum_framesizes = viif_enum_framesizes,
> > +
> > +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> > +	.vidioc_querybuf = vb2_ioctl_querybuf,
> > +	.vidioc_qbuf = vb2_ioctl_qbuf,
> > +	.vidioc_expbuf = vb2_ioctl_expbuf,
> > +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> > +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> > +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> > +	.vidioc_streamon = vb2_ioctl_streamon,
> > +	.vidioc_streamoff = vb2_ioctl_streamoff,
> > +
> > +	.vidioc_log_status = v4l2_ctrl_log_status,
> > +	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > +};
> > +
> > +/* --- File Operations --- */
> > +static const struct v4l2_pix_format_mplane pixm_default[3] = {
> > +	{
> > +		.pixelformat = V4L2_PIX_FMT_RGB24,
> > +		.width = 1920,
> > +		.height = 1080,
> > +		.field = V4L2_FIELD_NONE,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +	},
> > +	{
> > +		.pixelformat = V4L2_PIX_FMT_RGB24,
> > +		.width = 1920,
> > +		.height = 1080,
> > +		.field = V4L2_FIELD_NONE,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +	},
> > +	{
> > +		.pixelformat = V4L2_PIX_FMT_SRGGB8,
> > +		.width = 1920,
> > +		.height = 1080,
> > +		.field = V4L2_FIELD_NONE,
> > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > +	}
> > +};
> > +
> > +static int viif_capture_open(struct file *file)
> > +{
> > +	struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	int ret;
> > +
> > +	ret = v4l2_fh_open(file);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = pm_runtime_resume_and_get(viif_dev->dev);
> > +	if (ret) {
> > +		v4l2_fh_release(file);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int viif_capture_release(struct file *file)
> > +{
> > +	struct cap_dev *cap_dev = video_drvdata_to_capdev(file);
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +
> > +	vb2_fop_release(file);
> > +	pm_runtime_put(viif_dev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations viif_fops = {
> > +	.owner = THIS_MODULE,
> > +	.open = viif_capture_open,
> > +	.release = viif_capture_release,
> > +	.unlocked_ioctl = video_ioctl2,
> > +	.mmap = vb2_fop_mmap,
> > +	.poll = vb2_fop_poll,
> > +};
> > +
> > +/* ----- media control callbacks ----- */
> > +static int viif_capture_link_validate(struct media_link *link)
> > +{
> > +	/* link validation at start-stream */
> > +	return 0;
> > +}
> > +
> > +static const struct media_entity_operations viif_media_ops = {
> > +	.link_validate = viif_capture_link_validate,
> > +};
> > +
> > +/* ----- attach ctrl callbacck handler ----- */
> 
> callbacck -> callback
> 

I'll fix this and run spell checker again.

> > +int visconti_viif_capture_register_ctrl_handlers(struct viif_device *viif_dev)
> > +{
> > +	struct v4l2_subdev *sensor_sd = viif_dev->sensor_sd;
> > +	int ret;
> > +
> > +	if (!sensor_sd)
> > +		return -EINVAL;
> > +
> > +	/* MAIN POST0: merge controls of ISP and sensor */
> > +	ret = v4l2_ctrl_add_handler(&viif_dev->cap_dev0.ctrl_handler,
> sensor_sd->ctrl_handler, NULL,
> > +				    true);
> > +	if (ret) {
> > +		dev_err(viif_dev->dev, "Failed to add sensor ctrl_handler");
> > +		return ret;
> > +	}
> > +
> > +	/* MAIN POST1: merge controls of ISP and sensor */
> > +	ret = v4l2_ctrl_add_handler(&viif_dev->cap_dev1.ctrl_handler,
> sensor_sd->ctrl_handler, NULL,
> > +				    true);
> > +	if (ret) {
> > +		dev_err(viif_dev->dev, "Failed to add sensor ctrl_handler");
> > +		return ret;
> > +	}
> 
> For a Media Controller device you shouldn't merge the sensor controls into the
> main driver. The application software (libcamera) will handle the sensor
> controls
> directly through the v4l-subdevX device.
> 

I understand sensor's control should not be merged.
I'll remove calls of v4l2_ctrl_add_handler().

> > +
> > +	/* SUB: no control is exported */
> > +
> > +	return 0;
> > +}
> > +
> > +/* ----- register/remove capture device node ----- */
> > +static int visconti_viif_capture_register_node(struct cap_dev *cap_dev)
> > +{
> > +	struct viif_device *viif_dev = cap_dev->viif_dev;
> > +	struct v4l2_device *v4l2_dev = &viif_dev->v4l2_dev;
> > +	struct video_device *vdev = &cap_dev->vdev;
> > +	struct vb2_queue *q = &cap_dev->vb2_vq;
> > +	static const char *const node_name[] = {
> > +		"viif_capture_post0",
> > +		"viif_capture_post1",
> > +		"viif_capture_sub",
> > +	};
> > +	struct v4l2_pix_format_mplane pixm;
> > +	int ret;
> > +
> > +	INIT_LIST_HEAD(&cap_dev->buf_queue);
> > +
> > +	mutex_init(&cap_dev->vlock);
> > +	spin_lock_init(&cap_dev->buf_lock);
> > +
> > +	/* Initialize image format */
> > +	pixm = pixm_default[cap_dev->pathid];
> > +	viif_try_fmt(cap_dev, &pixm);
> > +	cap_dev->v4l2_pix = pixm;
> > +
> > +	/* Initialize vb2 queue. */
> > +	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > +	q->io_modes = VB2_MMAP | VB2_DMABUF;
> > +	q->min_queued_buffers = 3;
> > +	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > +	q->ops = &viif_vb2_ops;
> > +	q->mem_ops = &vb2_dma_contig_memops;
> > +	q->drv_priv = cap_dev;
> > +	q->buf_struct_size = sizeof(struct viif_buffer);
> > +	q->lock = &cap_dev->vlock;
> > +	q->dev = viif_dev->v4l2_dev.dev;
> > +
> > +	ret = vb2_queue_init(q);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Register the video device. */
> > +	strscpy(vdev->name, node_name[cap_dev->pathid],
> sizeof(vdev->name));
> > +	vdev->v4l2_dev = v4l2_dev;
> > +	vdev->lock = &cap_dev->vlock;
> > +	vdev->queue = &cap_dev->vb2_vq;
> > +	vdev->ctrl_handler = NULL;
> > +	vdev->fops = &viif_fops;
> > +	vdev->ioctl_ops = &viif_ioctl_ops;
> > +	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> V4L2_CAP_STREAMING;
> > +	vdev->device_caps |= V4L2_CAP_IO_MC;
> > +	vdev->entity.ops = &viif_media_ops;
> > +	vdev->release = video_device_release_empty;
> > +	video_set_drvdata(vdev, cap_dev);
> > +	vdev->vfl_dir = VFL_DIR_RX;
> > +	cap_dev->capture_pad.flags = MEDIA_PAD_FL_SINK;
> > +
> > +	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > +	if (ret < 0) {
> > +		dev_err(v4l2_dev->dev, "video_register_device failed: %d\n",
> ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = media_entity_pads_init(&vdev->entity, 1,
> &cap_dev->capture_pad);
> > +	if (ret) {
> > +		video_unregister_device(vdev);
> > +		return ret;
> > +	}
> > +
> > +	ret = v4l2_ctrl_handler_init(&cap_dev->ctrl_handler, 30);
> > +	if (ret)
> > +		return -ENOMEM;
> > +
> > +	cap_dev->vdev.ctrl_handler = &cap_dev->ctrl_handler;
> > +
> > +	return 0;
> > +}
> > +
> > +int visconti_viif_capture_register(struct viif_device *viif_dev)
> > +{
> > +	int ret;
> > +
> > +	/* register MAIN POST0 (primary RGB)*/
> > +	viif_dev->cap_dev0.pathid = CAPTURE_PATH_MAIN_POST0;
> > +	viif_dev->cap_dev0.viif_dev = viif_dev;
> > +	viif_dev->cap_dev0.fmts = viif_capture_fmt_list_mainpath;
> > +	viif_dev->cap_dev0.fmt_size =
> ARRAY_SIZE(viif_capture_fmt_list_mainpath);
> > +	ret = visconti_viif_capture_register_node(&viif_dev->cap_dev0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* register MAIN POST1 (additional RGB)*/
> > +	viif_dev->cap_dev1.pathid = CAPTURE_PATH_MAIN_POST1;
> > +	viif_dev->cap_dev1.viif_dev = viif_dev;
> > +	viif_dev->cap_dev1.fmts = viif_capture_fmt_list_mainpath;
> > +	viif_dev->cap_dev1.fmt_size =
> ARRAY_SIZE(viif_capture_fmt_list_mainpath);
> > +	ret = visconti_viif_capture_register_node(&viif_dev->cap_dev1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* register SUB (RAW) */
> > +	viif_dev->cap_dev2.pathid = CAPTURE_PATH_SUB;
> > +	viif_dev->cap_dev2.viif_dev = viif_dev;
> > +	viif_dev->cap_dev2.fmts = viif_capture_fmt_list_subpath;
> > +	viif_dev->cap_dev2.fmt_size =
> ARRAY_SIZE(viif_capture_fmt_list_subpath);
> > +	ret = visconti_viif_capture_register_node(&viif_dev->cap_dev2);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static void visconti_viif_capture_unregister_node(struct cap_dev *cap_dev)
> > +{
> > +	media_entity_cleanup(&cap_dev->vdev.entity);
> > +	v4l2_ctrl_handler_free(&cap_dev->ctrl_handler);
> > +	vb2_video_unregister_device(&cap_dev->vdev);
> > +	mutex_destroy(&cap_dev->vlock);
> > +}
> > +
> > +void visconti_viif_capture_unregister(struct viif_device *viif_dev)
> > +{
> > +	visconti_viif_capture_unregister_node(&viif_dev->cap_dev0);
> > +	visconti_viif_capture_unregister_node(&viif_dev->cap_dev1);
> > +	visconti_viif_capture_unregister_node(&viif_dev->cap_dev2);
> > +}
> 
> <snip>
> 
> Regards,
> 
> 	Hans

Best regards,
Yuji




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux