Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 22 Jul 2020 16:01:58 +0000
From:   Tomasz Figa <tfiga@chromium.org>
To:     Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Helen Koike <helen.koike@collabora.com>,
        linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
        eddie.cai.linux@gmail.com, mchehab@kernel.org, heiko@sntech.de,
        jacob2.chen@rock-chips.com, jeffy.chen@rock-chips.com,
        zyc@rock-chips.com, linux-kernel@vger.kernel.org,
        hans.verkuil@cisco.com, sakari.ailus@linux.intel.com,
        kernel@collabora.com, ezequiel@collabora.com,
        linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        zhengsq@rock-chips.com, Jacob Chen <cc@rock-chips.com>,
        Allon Huang <allon.huang@rock-chips.com>
Subject: Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
Message-ID: <20200722160158.GD1828171@chromium.org>
References: <20190730184256.30338-1-helen.koike@collabora.com>
 <20190730184256.30338-6-helen.koike@collabora.com>
 <20190816001323.GF5011@pendragon.ideasonboard.com>
 <30b6367d-9088-d755-d041-904ff2a48130@collabora.com>
 <477cb6ec-1d78-b30c-f84d-388c1bde2567@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <477cb6ec-1d78-b30c-f84d-388c1bde2567@collabora.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 17, 2020 at 09:46:24AM +0200, Dafna Hirschfeld wrote:
> Hi,
> 
> 
> On 11.07.20 13:04, Dafna Hirschfeld wrote:
> > Hi Laurent,
> > 
> > On 16.08.19 02:13, Laurent Pinchart wrote:
> > > Hello Helen,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote:
> > > > From: Jacob Chen <jacob2.chen@rock-chips.com>
> > > > 
> > > > Add the subdev driver for rockchip isp1.
> > > > 
> > > > Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
> > > > Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> > > > Signed-off-by: Yichong Zhong <zyc@rock-chips.com>
> > > > Signed-off-by: Jacob Chen <cc@rock-chips.com>
> > > > Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com>
> > > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > > > Signed-off-by: Allon Huang <allon.huang@rock-chips.com>
> > > > Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> > > > [fixed unknown entity type / switched to PIXEL_RATE]
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > [update for upstream]
> > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > 
> > > > ---
> > > > 
> > > > Changes in v8: None
> > > > Changes in v7:
> > > > - fixed warning because of unknown entity type
> > > > - fixed v4l2-compliance errors regarding rkisp1 formats, try formats
> > > > and default values
> > > > - fix typo riksp1/rkisp1
> > > > - redesign: remove mipi/csi subdevice, sensors connect directly to the
> > > > isp subdevice in the media topology now. As a consequence, remove the
> > > > hack in mipidphy_g_mbus_config() where information from the sensor was
> > > > being propagated through the topology.
> > > > - From the old dphy:
> > > > ???????? * cache get_remote_sensor() in s_stream
> > > > ???????? * use V4L2_CID_PIXEL_RATE instead of V4L2_CID_LINK_FREQ
> > > > - Replace stream state with a boolean
> > > > - code styling and checkpatch fixes
> > > > - fix stop_stream (return after calling stop, do not reenable the stream)
> > > > - fix rkisp1_isp_sd_get_selection when V4L2_SUBDEV_FORMAT_TRY is set
> > > > - fix get format in output (isp_sd->out_fmt.mbus_code was being ignored)
> > > > - s/intput/input
> > > > - remove #define sd_to_isp_sd(_sd), add a static inline as it will be
> > > > reused by the capture
> > > > 
> > > > ? drivers/media/platform/rockchip/isp1/rkisp1.c | 1286 +++++++++++++++++
> > > > ? drivers/media/platform/rockchip/isp1/rkisp1.h |? 111 ++
> > > > ? 2 files changed, 1397 insertions(+)
> > > > ? create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.c
> > > > ? create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.h
> > > > 
> > > > diff --git a/drivers/media/platform/rockchip/isp1/rkisp1.c b/drivers/media/platform/rockchip/isp1/rkisp1.c
> > > > new file mode 100644
> > > > index 000000000000..6d0c0ffb5e03
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/rockchip/isp1/rkisp1.c
> > > > @@ -0,0 +1,1286 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > +/*
> > > > + * Rockchip isp1 driver
> > > 
> > > Shouldn't each file describe what it contains ? Maybe
> > > 
> > > ? * Rockchip ISP1 Driver - ISP Core
> > > 
> > > for this one ? Same for other .c or .h files.
> > > 
> > > > + *
> > > > + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > > > + */
> > > > +
> > > > +#include <linux/iopoll.h>
> > > > +#include <linux/phy/phy.h>
> > > > +#include <linux/phy/phy-mipi-dphy.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/videodev2.h>
> > > > +#include <linux/vmalloc.h>
> > > > +#include <media/v4l2-event.h>
> > > > +
> > > > +#include "common.h"
> > > > +#include "regs.h"
> > > 
> > > common.h and regs.h aren't available yet. This won't break bisection as
> > > this file isn't referenced from the makefile yet, but it makes it a bit
> > > annoying when reviewing patches in order :-S
> > > 
> > > > +
> > > > +#define CIF_ISP_INPUT_W_MAX??????? 4032
> > > > +#define CIF_ISP_INPUT_H_MAX??????? 3024
> > > > +#define CIF_ISP_INPUT_W_MIN??????? 32
> > > > +#define CIF_ISP_INPUT_H_MIN??????? 32
> > > > +#define CIF_ISP_OUTPUT_W_MAX??????? CIF_ISP_INPUT_W_MAX
> > > > +#define CIF_ISP_OUTPUT_H_MAX??????? CIF_ISP_INPUT_H_MAX
> > > > +#define CIF_ISP_OUTPUT_W_MIN??????? CIF_ISP_INPUT_W_MIN
> > > > +#define CIF_ISP_OUTPUT_H_MIN??????? CIF_ISP_INPUT_H_MIN
> > > > +
> > > > +/*
> > > > + * NOTE: MIPI controller and input MUX are also configured in this file,
> > > > + * because ISP Subdev is not only describe ISP submodule(input size,format,
> > > > + * output size, format), but also a virtual route device.
> > > > + */
> > > > +
> > > > +/*
> > > > + * There are many variables named with format/frame in below code,
> > > > + * please see here for their meaning.
> > > > + *
> > > > + * Cropping regions of ISP
> > > > + *
> > > > + * +---------------------------------------------------------+
> > > > + * | Sensor image??????????????????????????????????????????? |
> > > > + * | +---------------------------------------------------+?? |
> > > > + * | | ISP_ACQ (for black level)???????????????????????? |?? |
> > > > + * | | in_frm??????????????????????????????????????????? |?? |
> > > > + * | | +--------------------------------------------+??? |?? |
> > > > + * | | |??? ISP_OUT???????????????????????????????? |??? |?? |
> > > > + * | | |??? in_crop???????????????????????????????? |??? |?? |
> > > 
> > > in_crop at the ISP output ? That seems a bit weird. I'm guessing that
> > > this is really the ISP output, while ISP_IS is related to the resizer ?
> > > 
> > > > + * | | |??? +---------------------------------+???? |??? |?? |
> > > > + * | | |??? |?? ISP_IS??????????????????????? |???? |??? |?? |
> > > > + * | | |??? |?? rkisp1_isp_subdev: out_crop?? |???? |??? |?? |
> > > > + * | | |??? +---------------------------------+???? |??? |?? |
> > > > + * | | +--------------------------------------------+??? |?? |
> > > > + * | +---------------------------------------------------+?? |
> > > > + * +---------------------------------------------------------+
> > > > + */
> > > > +
> > > > +static inline struct rkisp1_device *sd_to_isp_dev(struct v4l2_subdev *sd)
> > > > +{
> > > > +??? return container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
> > > > +}
> > > > +
> > > > +/* Get sensor by enabled media link */
> > > > +static struct v4l2_subdev *get_remote_sensor(struct v4l2_subdev *sd)
> > > > +{
> > > > +??? struct media_pad *local, *remote;
> > > > +??? struct media_entity *sensor_me;
> > > > +
> > > > +??? local = &sd->entity.pads[RKISP1_ISP_PAD_SINK];
> > > > +??? remote = media_entity_remote_pad(local);
> > > > +??? if (!remote) {
> > > > +??????? v4l2_warn(sd, "No link between isp and sensor\n");
> > > > +??????? return NULL;
> > > > +??? }
> > > > +
> > > > +??? sensor_me = media_entity_remote_pad(local)->entity;
> > > > +??? return media_entity_to_v4l2_subdev(sensor_me);
> > > > +}
> > > > +
> > > > +static struct rkisp1_sensor *sd_to_sensor(struct rkisp1_device *dev,
> > > > +?????????????????????????? struct v4l2_subdev *sd)
> > > > +{
> > > > +??? struct rkisp1_sensor *sensor;
> > > > +
> > > > +??? list_for_each_entry(sensor, &dev->sensors, list)
> > > > +??????? if (sensor->sd == sd)
> > > > +??????????? return sensor;
> > > > +
> > > > +??? return NULL;
> > > > +}
> > > > +
> > > > +/****************? register operations ****************/
> > > > +
> > > > +/*
> > > > + * Image Stabilization.
> > > > + * This should only be called when configuring CIF
> > > > + * or at the frame end interrupt
> > > > + */
> > > > +static void rkisp1_config_ism(struct rkisp1_device *dev)
> > > > +{
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? struct v4l2_rect *out_crop = &dev->isp_sdev.out_crop;
> > > > +??? u32 val;
> > > > +
> > > > +??? writel(0, base + CIF_ISP_IS_RECENTER);
> > > 
> > > How about read/write wrappers that take a rkisp1_device pointer, a
> > > register offset and a value (for the write wrapper) and compute
> > > dev->base_addr + offset internally ? That would make the code easier to
> > > read.
> > > 
> > > > +??? writel(0, base + CIF_ISP_IS_MAX_DX);
> > > > +??? writel(0, base + CIF_ISP_IS_MAX_DY);
> > > > +??? writel(0, base + CIF_ISP_IS_DISPLACE);
> > > > +??? writel(out_crop->left, base + CIF_ISP_IS_H_OFFS);
> > > > +??? writel(out_crop->top, base + CIF_ISP_IS_V_OFFS);
> > > > +??? writel(out_crop->width, base + CIF_ISP_IS_H_SIZE);
> > > > +??? writel(out_crop->height, base + CIF_ISP_IS_V_SIZE);
> > > > +
> > > > +??? /* IS(Image Stabilization) is always on, working as output crop */
> > > > +??? writel(1, base + CIF_ISP_IS_CTRL);
> > > > +??? val = readl(base + CIF_ISP_CTRL);
> > > > +??? val |= CIF_ISP_CTRL_ISP_CFG_UPD;
> > > > +??? writel(val, base + CIF_ISP_CTRL);
> > > > +}
> > > > +
> > > > +/*
> > > > + * configure isp blocks with input format, size......
> > > > + */
> > > > +static int rkisp1_config_isp(struct rkisp1_device *dev)
> > > > +{
> > > > +??? u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, signal = 0;
> > > > +??? struct v4l2_rect *out_crop, *in_crop;
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? struct v4l2_mbus_framefmt *in_frm;
> > > > +??? struct ispsd_out_fmt *out_fmt;
> > > > +??? struct rkisp1_sensor *sensor;
> > > > +??? struct ispsd_in_fmt *in_fmt;
> > > > +
> > > > +??? sensor = dev->active_sensor;
> > > > +??? in_frm = &dev->isp_sdev.in_frm;
> > > > +??? in_fmt = &dev->isp_sdev.in_fmt;
> > > > +??? out_fmt = &dev->isp_sdev.out_fmt;
> > > > +??? out_crop = &dev->isp_sdev.out_crop;
> > > > +??? in_crop = &dev->isp_sdev.in_crop;
> > > > +
> > > > +??? if (in_fmt->fmt_type == FMT_BAYER) {
> > > > +??????? acq_mult = 1;
> > > > +??????? if (out_fmt->fmt_type == FMT_BAYER) {
> > > > +??????????? if (sensor->mbus.type == V4L2_MBUS_BT656)
> > > > +??????????????? isp_ctrl =
> > > > +??????????????????? CIF_ISP_CTRL_ISP_MODE_RAW_PICT_ITU656;
> > > > +??????????? else
> > > > +??????????????? isp_ctrl =
> > > > +??????????????????? CIF_ISP_CTRL_ISP_MODE_RAW_PICT;
> > > > +??????? } else {
> > > > +??????????? writel(CIF_ISP_DEMOSAIC_TH(0xc),
> > > > +?????????????????? base + CIF_ISP_DEMOSAIC);
> > > > +
> > > > +??????????? if (sensor->mbus.type == V4L2_MBUS_BT656)
> > > > +??????????????? isp_ctrl = CIF_ISP_CTRL_ISP_MODE_BAYER_ITU656;
> > > > +??????????? else
> > > > +??????????????? isp_ctrl = CIF_ISP_CTRL_ISP_MODE_BAYER_ITU601;
> > > > +??????? }
> > > > +??? } else if (in_fmt->fmt_type == FMT_YUV) {
> > > > +??????? acq_mult = 2;
> > > > +??????? if (sensor->mbus.type == V4L2_MBUS_CSI2_DPHY) {
> > > > +??????????? isp_ctrl = CIF_ISP_CTRL_ISP_MODE_ITU601;
> > > > +??????? } else {
> > > > +??????????? if (sensor->mbus.type == V4L2_MBUS_BT656)
> > > > +??????????????? isp_ctrl = CIF_ISP_CTRL_ISP_MODE_ITU656;
> > > > +??????????? else
> > > > +??????????????? isp_ctrl = CIF_ISP_CTRL_ISP_MODE_ITU601;
> > > > +
> > > > +??????? }
> > > > +
> > > > +??????? irq_mask |= CIF_ISP_DATA_LOSS;
> > > > +??? }
> > > > +
> > > > +??? /* Set up input acquisition properties */
> > > > +??? if (sensor->mbus.type == V4L2_MBUS_BT656 ||
> > > > +??????? sensor->mbus.type == V4L2_MBUS_PARALLEL) {
> > > > +??????? if (sensor->mbus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > +??????????? signal = CIF_ISP_ACQ_PROP_POS_EDGE;
> > > > +??? }
> > > > +
> > > > +??? if (sensor->mbus.type == V4L2_MBUS_PARALLEL) {
> > > > +??????? if (sensor->mbus.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > +??????????? signal |= CIF_ISP_ACQ_PROP_VSYNC_LOW;
> > > > +
> > > > +??????? if (sensor->mbus.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > > > +??????????? signal |= CIF_ISP_ACQ_PROP_HSYNC_LOW;
> > > > +??? }
> > > > +
> > > > +??? writel(isp_ctrl, base + CIF_ISP_CTRL);
> > > > +??? writel(signal | in_fmt->yuv_seq |
> > > > +?????????? CIF_ISP_ACQ_PROP_BAYER_PAT(in_fmt->bayer_pat) |
> > > > +?????????? CIF_ISP_ACQ_PROP_FIELD_SEL_ALL, base + CIF_ISP_ACQ_PROP);
> > > > +??? writel(0, base + CIF_ISP_ACQ_NR_FRAMES);
> > > > +
> > > > +??? /* Acquisition Size */
> > > > +??? writel(0, base + CIF_ISP_ACQ_H_OFFS);
> > > > +??? writel(0, base + CIF_ISP_ACQ_V_OFFS);
> > > > +??? writel(acq_mult * in_frm->width, base + CIF_ISP_ACQ_H_SIZE);
> > > > +??? writel(in_frm->height, base + CIF_ISP_ACQ_V_SIZE);
> > > > +
> > > > +??? /* ISP Out Area */
> > > > +??? writel(in_crop->left, base + CIF_ISP_OUT_H_OFFS);
> > > > +??? writel(in_crop->top, base + CIF_ISP_OUT_V_OFFS);
> > > > +??? writel(in_crop->width, base + CIF_ISP_OUT_H_SIZE);
> > > > +??? writel(in_crop->height, base + CIF_ISP_OUT_V_SIZE);
> > > > +
> > > > +??? /* interrupt mask */
> > > > +??? irq_mask |= CIF_ISP_FRAME | CIF_ISP_V_START | CIF_ISP_PIC_SIZE_ERROR |
> > > > +??????????? CIF_ISP_FRAME_IN;
> > > > +??? writel(irq_mask, base + CIF_ISP_IMSC);
> > > > +
> > > > +??? if (out_fmt->fmt_type == FMT_BAYER)
> > > > +??????? rkisp1_params_disable_isp(&dev->params_vdev);
> > > > +??? else
> > > > +??????? rkisp1_params_configure_isp(&dev->params_vdev, in_fmt,
> > > > +??????????????????????? dev->isp_sdev.quantization);
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static int rkisp1_config_dvp(struct rkisp1_device *dev)
> > > > +{
> > > > +??? struct ispsd_in_fmt *in_fmt = &dev->isp_sdev.in_fmt;
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? u32 val, input_sel;
> > > > +
> > > > +??? switch (in_fmt->bus_width) {
> > > > +??? case 8:
> > > > +??????? input_sel = CIF_ISP_ACQ_PROP_IN_SEL_8B_ZERO;
> > > > +??????? break;
> > > > +??? case 10:
> > > > +??????? input_sel = CIF_ISP_ACQ_PROP_IN_SEL_10B_ZERO;
> > > > +??????? break;
> > > > +??? case 12:
> > > > +??????? input_sel = CIF_ISP_ACQ_PROP_IN_SEL_12B;
> > > > +??????? break;
> > > > +??? default:
> > > > +??????? v4l2_err(&dev->v4l2_dev, "Invalid bus width\n");
> > > > +??????? return -EINVAL;
> > > > +??? }
> > > > +
> > > > +??? val = readl(base + CIF_ISP_ACQ_PROP);
> > > > +??? writel(val | input_sel, base + CIF_ISP_ACQ_PROP);
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static int rkisp1_config_mipi(struct rkisp1_device *dev)
> > > > +{
> > > > +??? struct ispsd_in_fmt *in_fmt = &dev->isp_sdev.in_fmt;
> > > > +??? struct rkisp1_sensor *sensor = dev->active_sensor;
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? unsigned int lanes;
> > > > +??? u32 mipi_ctrl;
> > > > +
> > > > +??? /*
> > > > +???? * sensor->mbus is set in isp or d-phy notifier_bound function
> > > > +???? */
> > > > +??? switch (sensor->mbus.flags & V4L2_MBUS_CSI2_LANES) {
> > > > +??? case V4L2_MBUS_CSI2_4_LANE:
> > > > +??????? lanes = 4;
> > > > +??????? break;
> > > > +??? case V4L2_MBUS_CSI2_3_LANE:
> > > > +??????? lanes = 3;
> > > > +??????? break;
> > > > +??? case V4L2_MBUS_CSI2_2_LANE:
> > > > +??????? lanes = 2;
> > > > +??????? break;
> > > > +??? case V4L2_MBUS_CSI2_1_LANE:
> > > > +??????? lanes = 1;
> > > > +??????? break;
> > > > +??? default:
> > > > +??????? return -EINVAL;
> > > > +??? }
> > > > +
> > > > +??? mipi_ctrl = CIF_MIPI_CTRL_NUM_LANES(lanes - 1) |
> > > > +??????????? CIF_MIPI_CTRL_SHUTDOWNLANES(0xf) |
> > > > +??????????? CIF_MIPI_CTRL_ERR_SOT_SYNC_HS_SKIP |
> > > > +??????????? CIF_MIPI_CTRL_CLOCKLANE_ENA;
> > > > +
> > > > +??? writel(mipi_ctrl, base + CIF_MIPI_CTRL);
> > > > +
> > > > +??? /* Configure Data Type and Virtual Channel */
> > > > +??? writel(CIF_MIPI_DATA_SEL_DT(in_fmt->mipi_dt) | CIF_MIPI_DATA_SEL_VC(0),
> > > > +?????????? base + CIF_MIPI_IMG_DATA_SEL);
> > > > +
> > > > +??? /* Clear MIPI interrupts */
> > > > +??? writel(~0, base + CIF_MIPI_ICR);
> > > > +??? /*
> > > > +???? * Disable CIF_MIPI_ERR_DPHY interrupt here temporary for
> > > > +???? * isp bus may be dead when switch isp.
> > > > +???? */
> > > > +??? writel(CIF_MIPI_FRAME_END | CIF_MIPI_ERR_CSI | CIF_MIPI_ERR_DPHY |
> > > > +?????????? CIF_MIPI_SYNC_FIFO_OVFLW(0x03) | CIF_MIPI_ADD_DATA_OVFLW,
> > > > +?????????? base + CIF_MIPI_IMSC);
> > > > +
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, "\n? MIPI_CTRL 0x%08x\n"
> > > > +???????? "? MIPI_IMG_DATA_SEL 0x%08x\n"
> > > > +???????? "? MIPI_STATUS 0x%08x\n"
> > > > +???????? "? MIPI_IMSC 0x%08x\n",
> > > > +???????? readl(base + CIF_MIPI_CTRL),
> > > > +???????? readl(base + CIF_MIPI_IMG_DATA_SEL),
> > > > +???????? readl(base + CIF_MIPI_STATUS),
> > > > +???????? readl(base + CIF_MIPI_IMSC));
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +/* Configure MUX */
> > > > +static int rkisp1_config_path(struct rkisp1_device *dev)
> > > > +{
> > > > +??? struct rkisp1_sensor *sensor = dev->active_sensor;
> > > > +??? u32 dpcl = readl(dev->base_addr + CIF_VI_DPCL);
> > > > +??? int ret = 0;
> > > > +
> > > > +??? if (sensor->mbus.type == V4L2_MBUS_BT656 ||
> > > > +??????? sensor->mbus.type == V4L2_MBUS_PARALLEL) {
> > > > +??????? ret = rkisp1_config_dvp(dev);
> > > > +??????? dpcl |= CIF_VI_DPCL_IF_SEL_PARALLEL;
> > > > +??? } else if (sensor->mbus.type == V4L2_MBUS_CSI2_DPHY) {
> > > > +??????? ret = rkisp1_config_mipi(dev);
> > > > +??????? dpcl |= CIF_VI_DPCL_IF_SEL_MIPI;
> > > > +??? }
> > > > +
> > > > +??? writel(dpcl, dev->base_addr + CIF_VI_DPCL);
> > > > +
> > > > +??? return ret;
> > > > +}
> > > > +
> > > > +/* Hareware configure Entry */
> > > 
> > > s/Hareware/Hardware/
> > > 
> > > > +static int rkisp1_config_cif(struct rkisp1_device *dev)
> > > > +{
> > > > +??? u32 cif_id;
> > > > +??? int ret;
> > > > +
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
> > > > +???????? "SP streaming = %d, MP streaming = %d\n",
> > > > +???????? dev->stream[RKISP1_STREAM_SP].streaming,
> > > > +???????? dev->stream[RKISP1_STREAM_MP].streaming);
> > > > +
> > > > +??? cif_id = readl(dev->base_addr + CIF_VI_ID);
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, "CIF_ID 0x%08x\n", cif_id);
> > > > +
> > > > +??? ret = rkisp1_config_isp(dev);
> > > > +??? if (ret < 0)
> > > > +??????? return ret;
> > > > +??? ret = rkisp1_config_path(dev);
> > > > +??? if (ret < 0)
> > > > +??????? return ret;
> > > > +??? rkisp1_config_ism(dev);
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +/* Mess register operations to stop isp */
> > > 
> > > Is it such a mess ? :-)
> > > 
> > > I would capitalise ISP in all comments.
> > > 
> > > > +static int rkisp1_isp_stop(struct rkisp1_device *dev)
> > > > +{
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? u32 val;
> > > > +
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
> > > > +???????? "SP streaming = %d, MP streaming = %d\n",
> > > > +???????? dev->stream[RKISP1_STREAM_SP].streaming,
> > > > +???????? dev->stream[RKISP1_STREAM_MP].streaming);
> > > > +
> > > > +??? /*
> > > > +???? * ISP(mi) stop in mi frame end -> Stop ISP(mipi) ->
> > > > +???? * Stop ISP(isp) ->wait for ISP isp off
> > > > +???? */
> > > > +??? /* stop and clear MI, MIPI, and ISP interrupts */
> > > > +??? writel(0, base + CIF_MIPI_IMSC);
> > > > +??? writel(~0, base + CIF_MIPI_ICR);
> > > > +
> > > > +??? writel(0, base + CIF_ISP_IMSC);
> > > > +??? writel(~0, base + CIF_ISP_ICR);
> > > > +
> > > > +??? writel(0, base + CIF_MI_IMSC);
> > > > +??? writel(~0, base + CIF_MI_ICR);
> > > > +??? val = readl(base + CIF_MIPI_CTRL);
> > > > +??? writel(val & (~CIF_MIPI_CTRL_OUTPUT_ENA), base + CIF_MIPI_CTRL);
> > > > +??? /* stop ISP */
> > > > +??? val = readl(base + CIF_ISP_CTRL);
> > > > +??? val &= ~(CIF_ISP_CTRL_ISP_INFORM_ENABLE | CIF_ISP_CTRL_ISP_ENABLE);
> > > > +??? writel(val, base + CIF_ISP_CTRL);
> > > > +
> > > > +??? val = readl(base + CIF_ISP_CTRL);
> > > > +??? writel(val | CIF_ISP_CTRL_ISP_CFG_UPD, base + CIF_ISP_CTRL);
> > > > +
> > > > +??? readx_poll_timeout(readl, base + CIF_ISP_RIS,
> > > > +?????????????? val, val & CIF_ISP_OFF, 20, 100);
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
> > > > +??????? "streaming(MP:%d, SP:%d), MI_CTRL:%x, ISP_CTRL:%x, MIPI_CTRL:%x\n",
> > > > +???????? dev->stream[RKISP1_STREAM_SP].streaming,
> > > > +???????? dev->stream[RKISP1_STREAM_MP].streaming,
> > > > +???????? readl(base + CIF_MI_CTRL),
> > > > +???????? readl(base + CIF_ISP_CTRL),
> > > > +???????? readl(base + CIF_MIPI_CTRL));
> > > > +
> > > > +??? writel(CIF_IRCL_MIPI_SW_RST | CIF_IRCL_ISP_SW_RST, base + CIF_IRCL);
> > > > +??? writel(0x0, base + CIF_IRCL);
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +/* Mess register operations to start isp */
> > > > +static int rkisp1_isp_start(struct rkisp1_device *dev)
> > > > +{
> > > > +??? struct rkisp1_sensor *sensor = dev->active_sensor;
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? u32 val;
> > > > +
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
> > > > +???????? "SP streaming = %d, MP streaming = %d\n",
> > > > +???????? dev->stream[RKISP1_STREAM_SP].streaming,
> > > > +???????? dev->stream[RKISP1_STREAM_MP].streaming);
> > > > +
> > > > +??? /* Activate MIPI */
> > > > +??? if (sensor->mbus.type == V4L2_MBUS_CSI2_DPHY) {
> > > > +??????? val = readl(base + CIF_MIPI_CTRL);
> > > > +??????? writel(val | CIF_MIPI_CTRL_OUTPUT_ENA, base + CIF_MIPI_CTRL);
> > > > +??? }
> > > > +??? /* Activate ISP */
> > > > +??? val = readl(base + CIF_ISP_CTRL);
> > > > +??? val |= CIF_ISP_CTRL_ISP_CFG_UPD | CIF_ISP_CTRL_ISP_ENABLE |
> > > > +?????????? CIF_ISP_CTRL_ISP_INFORM_ENABLE;
> > > > +??? writel(val, base + CIF_ISP_CTRL);
> > > > +
> > > > +??? /* XXX: Is the 1000us too long?
> > > > +???? * CIF spec says to wait for sufficient time after enabling
> > > > +???? * the MIPI interface and before starting the sensor output.
> > > > +???? */
> > > > +??? usleep_range(1000, 1200);
> > > > +
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
> > > > +???????? "SP streaming = %d, MP streaming = %d MI_CTRL 0x%08x\n"
> > > > +???????? "? ISP_CTRL 0x%08x MIPI_CTRL 0x%08x\n",
> > > > +???????? dev->stream[RKISP1_STREAM_SP].streaming,
> > > > +???????? dev->stream[RKISP1_STREAM_MP].streaming,
> > > > +???????? readl(base + CIF_MI_CTRL),
> > > > +???????? readl(base + CIF_ISP_CTRL),
> > > > +???????? readl(base + CIF_MIPI_CTRL));
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static void rkisp1_config_clk(struct rkisp1_device *dev)
> > > > +{
> > > > +??? u32 val = CIF_ICCL_ISP_CLK | CIF_ICCL_CP_CLK | CIF_ICCL_MRSZ_CLK |
> > > > +????????? CIF_ICCL_SRSZ_CLK | CIF_ICCL_JPEG_CLK | CIF_ICCL_MI_CLK |
> > > > +????????? CIF_ICCL_IE_CLK | CIF_ICCL_MIPI_CLK | CIF_ICCL_DCROP_CLK;
> > > > +
> > > > +??? writel(val, dev->base_addr + CIF_ICCL);
> > > > +}
> > > > +
> > > > +/***************************** isp sub-devs *******************************/
> > > > +
> > > > +static const struct ispsd_in_fmt rkisp1_isp_input_formats[] = {
> > > > +??? {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW10,
> > > > +??????? .bayer_pat??? = RAW_BGGR,
> > > > +??????? .bus_width??? = 10,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW10,
> > > > +??????? .bayer_pat??? = RAW_RGGB,
> > > > +??????? .bus_width??? = 10,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW10,
> > > > +??????? .bayer_pat??? = RAW_GBRG,
> > > > +??????? .bus_width??? = 10,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW10,
> > > > +??????? .bayer_pat??? = RAW_GRBG,
> > > > +??????? .bus_width??? = 10,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW12,
> > > > +??????? .bayer_pat??? = RAW_RGGB,
> > > > +??????? .bus_width??? = 12,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > 
> > > Is there a reason why the 10-bit Bayer format start with BGGR while the
> > > 12-bit formats start with RGGB ? Not a big deal, just OCD kicking in :-)
> > > 
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW12,
> > > > +??????? .bayer_pat??? = RAW_BGGR,
> > > > +??????? .bus_width??? = 12,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW12,
> > > > +??????? .bayer_pat??? = RAW_GBRG,
> > > > +??????? .bus_width??? = 12,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW12,
> > > > +??????? .bayer_pat??? = RAW_GRBG,
> > > > +??????? .bus_width??? = 12,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW8,
> > > > +??????? .bayer_pat??? = RAW_RGGB,
> > > > +??????? .bus_width??? = 8,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW8,
> > > > +??????? .bayer_pat??? = RAW_BGGR,
> > > > +??????? .bus_width??? = 8,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW8,
> > > > +??????? .bayer_pat??? = RAW_GBRG,
> > > > +??????? .bus_width??? = 8,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_RAW8,
> > > > +??????? .bayer_pat??? = RAW_GRBG,
> > > > +??????? .bus_width??? = 8,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_YUYV8_1X16,
> > > > +??????? .fmt_type??? = FMT_YUV,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_YUV422_8b,
> > > > +??????? .yuv_seq??? = CIF_ISP_ACQ_PROP_YCBYCR,
> > > > +??????? .bus_width??? = 16,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_YVYU8_1X16,
> > > > +??????? .fmt_type??? = FMT_YUV,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_YUV422_8b,
> > > > +??????? .yuv_seq??? = CIF_ISP_ACQ_PROP_YCRYCB,
> > > > +??????? .bus_width??? = 16,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > +??????? .fmt_type??? = FMT_YUV,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_YUV422_8b,
> > > > +??????? .yuv_seq??? = CIF_ISP_ACQ_PROP_CBYCRY,
> > > > +??????? .bus_width??? = 16,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_VYUY8_1X16,
> > > > +??????? .fmt_type??? = FMT_YUV,
> > > > +??????? .mipi_dt??? = CIF_CSI2_DT_YUV422_8b,
> > > > +??????? .yuv_seq??? = CIF_ISP_ACQ_PROP_CRYCBY,
> > > > +??????? .bus_width??? = 16,
> > > > +??? },
> > > > +};
> > > > +
> > > > +static const struct ispsd_out_fmt rkisp1_isp_output_formats[] = {
> > > > +??? {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_YUYV8_2X8,
> > > > +??????? .fmt_type??? = FMT_YUV,
> > > > +??? }, {
> > > 
> > > This is the only entry not present in the previous table, so I'm
> > > wondering if it would make sense to merge the two tables and rename
> > > ispsd_in_fmt to rkisp1_format_info. You would need to add a field that
> > > tells, for each format, if it's valid as an input format, and output
> > > format, or both. Hmmmm and also make the enum logic a bit more complex.
> > > Maybe it's not worth it after all, but it bothers me a bit to have two
> > > tables :-) I'll let you decide what's best.
> > > 
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? }, {
> > > > +??????? .mbus_code??? = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > +??????? .fmt_type??? = FMT_BAYER,
> > > > +??? },
> > > > +};
> > > > +
> > > > +static const struct ispsd_in_fmt *find_in_fmt(u32 mbus_code)
> > > > +{
> > > > +??? unsigned int i, array_size = ARRAY_SIZE(rkisp1_isp_input_formats);
> > > > +??? const struct ispsd_in_fmt *fmt;
> > > 
> > > You can move this variable inside the loop.
> > > 
> > > > +
> > > > +??? for (i = 0; i < array_size; i++) {
> > > 
> > > I would remove the array_size local variable, it doesn't improve
> > > readability. Same for the next function.
> > > 
> > > > +??????? fmt = &rkisp1_isp_input_formats[i];
> > > > +??????? if (fmt->mbus_code == mbus_code)
> > > > +??????????? return fmt;
> > > > +??? }
> > > > +
> > > > +??? return NULL;
> > > > +}
> > > > +
> > > > +static const struct ispsd_out_fmt *find_out_fmt(u32 mbus_code)
> > > > +{
> > > > +??? unsigned int i, array_size = ARRAY_SIZE(rkisp1_isp_output_formats);
> > > > +??? const struct ispsd_out_fmt *fmt;
> > > > +
> > > > +??? for (i = 0; i < array_size; i++) {
> > > > +??????? fmt = &rkisp1_isp_output_formats[i];
> > > > +??????? if (fmt->mbus_code == mbus_code)
> > > > +??????????? return fmt;
> > > > +??? }
> > > > +
> > > > +??? return NULL;
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_enum_mbus_code(struct v4l2_subdev *sd,
> > > > +??????????????????? struct v4l2_subdev_pad_config *cfg,
> > > > +??????????????????? struct v4l2_subdev_mbus_code_enum *code)
> > > > +{
> > > > +??? unsigned int i = code->index;
> > > > +
> > > > +??? if ((code->pad != RKISP1_ISP_PAD_SINK) &&
> > > > +??????? (code->pad != RKISP1_ISP_PAD_SOURCE_PATH)) {
> > > > +??????? if (i > 0)
> > > > +??????????? return -EINVAL;
> > > > +??????? code->code = MEDIA_BUS_FMT_FIXED;
> > > > +??????? return 0;
> > > > +??? }
> > > > +
> > > > +??? if (code->pad == RKISP1_ISP_PAD_SINK) {
> > > > +??????? if (i >= ARRAY_SIZE(rkisp1_isp_input_formats))
> > > > +??????????? return -EINVAL;
> > > > +??????? code->code = rkisp1_isp_input_formats[i].mbus_code;
> > > > +??? } else {
> > > > +??????? if (i >= ARRAY_SIZE(rkisp1_isp_output_formats))
> > > > +??????????? return -EINVAL;
> > > > +??????? code->code = rkisp1_isp_output_formats[i].mbus_code;
> > > > +??? }
> > > 
> > > On the other hand, merging the two tables above into one, you could
> > > merge the two branches here and only consider formats indicated as valid
> > > for to the pad you want. Maybe the code would be cleaner in the end.
> > > 
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_init_config(struct v4l2_subdev *sd,
> > > > +???????????????????? struct v4l2_subdev_pad_config *cfg)
> > > > +{
> > > > +??? struct v4l2_rect *mf_in_crop, *mf_out_crop;
> > > > +??? struct v4l2_mbus_framefmt *mf_in, *mf_out;
> > > > +
> > > > +??? mf_in = v4l2_subdev_get_try_format(sd, cfg, RKISP1_ISP_PAD_SINK);
> > > > +??? mf_in->width = RKISP1_DEFAULT_WIDTH;
> > > > +??? mf_in->height = RKISP1_DEFAULT_HEIGHT;
> > > > +??? mf_in->field = V4L2_FIELD_NONE;
> > > > +??? mf_in->code = rkisp1_isp_input_formats[0].mbus_code;
> > > > +
> > > > +??? mf_in_crop = v4l2_subdev_get_try_crop(sd, cfg, RKISP1_ISP_PAD_SINK);
> > > > +??? mf_in_crop->width = RKISP1_DEFAULT_WIDTH;
> > > > +??? mf_in_crop->height = RKISP1_DEFAULT_HEIGHT;
> > > > +??? mf_in_crop->left = 0;
> > > > +??? mf_in_crop->top = 0;
> > > > +
> > > > +??? mf_out = v4l2_subdev_get_try_format(sd, cfg,
> > > > +??????????????????????? RKISP1_ISP_PAD_SOURCE_PATH);
> > > > +??? *mf_out = *mf_in;
> > > > +??? mf_out->code = rkisp1_isp_output_formats[0].mbus_code;
> > > > +??? mf_out->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > > +
> > > > +??? mf_out_crop = v4l2_subdev_get_try_crop(sd, cfg,
> > > > +?????????????????????????? RKISP1_ISP_PAD_SOURCE_PATH);
> > > > +??? *mf_out_crop = *mf_in_crop;
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_get_fmt(struct v4l2_subdev *sd,
> > > > +???????????????? struct v4l2_subdev_pad_config *cfg,
> > > > +???????????????? struct v4l2_subdev_format *fmt)
> > > > +{
> > > > +??? struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
> > > > +??? struct v4l2_mbus_framefmt *mf = &fmt->format;
> > > > +
> > > > +??? if ((fmt->pad != RKISP1_ISP_PAD_SINK) &&
> > > > +??????? (fmt->pad != RKISP1_ISP_PAD_SOURCE_PATH)) {
> > > > +??????? fmt->format.code = MEDIA_BUS_FMT_FIXED;
> > > > +??????? /*
> > > > +???????? * NOTE: setting a format here doesn't make much sense
> > > > +???????? * but v4l2-compliance complains
> > > > +???????? */
> > > 
> > > For the params pad I agreed it makes no sense, and I think
> > > v4l2-compliance is at fault, so I'd set width and height to 0. For the
> > > stats pad we *could* use the size of the image from which stats are
> > > computed, but because v4l2_meta_format has no width/height, I think 0
> > > would also be appropriate.
> > > 
> > > > +??????? fmt->format.width = RKISP1_DEFAULT_WIDTH;
> > > > +??????? fmt->format.height = RKISP1_DEFAULT_HEIGHT;
> > > > +??????? fmt->format.field = V4L2_FIELD_NONE;
> > > > +??????? return 0;
> > > > +??? }
> > > > +
> > > > +??? if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > > +??????? mf = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> > > > +??????? fmt->format = *mf;
> > > > +??????? return 0;
> > > > +??? }
> > > > +
> > > > +??? if (fmt->pad == RKISP1_ISP_PAD_SINK) {
> > > > +??????? *mf = isp_sd->in_frm;
> > > > +??? } else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_PATH) {
> > > > +??????? /* format of source pad */
> > > > +??????? *mf = isp_sd->in_frm;
> > > > +??????? mf->code = isp_sd->out_fmt.mbus_code;
> > > > +??????? /* window size of source pad */
> > > > +??????? mf->width = isp_sd->out_crop.width;
> > > > +??????? mf->height = isp_sd->out_crop.height;
> > > > +??????? mf->quantization = isp_sd->quantization;
> > > > +??? }
> > > > +??? mf->field = V4L2_FIELD_NONE;
> > > 
> > > This can be simplified, please read through, or jump to the review of
> > > rkisp1_isp_subdev at the end.
> > > 
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static void rkisp1_isp_sd_try_fmt(struct v4l2_subdev *sd,
> > > > +????????????????? unsigned int pad,
> > > > +????????????????? struct v4l2_mbus_framefmt *fmt)
> > > > +{
> > > > +??? struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > > +??? struct rkisp1_isp_subdev *isp_sd = &isp_dev->isp_sdev;
> > > > +??? const struct ispsd_out_fmt *out_fmt;
> > > > +??? const struct ispsd_in_fmt *in_fmt;
> > > > +
> > > > +??? switch (pad) {
> > > > +??? case RKISP1_ISP_PAD_SINK:
> > > > +??????? in_fmt = find_in_fmt(fmt->code);
> > > > +??????? if (in_fmt)
> > > > +??????????? fmt->code = in_fmt->mbus_code;
> > > > +??????? else
> > > > +??????????? fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > > 
> > > You write MEDIA_BUS_FMT_SRGGB10_1X10 explicitly here, while you use
> > > rkisp1_isp_output_formats[0].mbus_code below (and in other places). I
> > > would standardise on one of the two (explicit formats or array[0]), with
> > > a preference for the first as that would allow merging the input and
> > > output arrays more easily. I would then create two #define,
> > > RKISP1_DEF_INPUT_FORMAT and RKISP2_DEF_OUTPUT_FORMAT (or similar).
> > > Similar macros for the default width and height could also be useful, to
> > > make it easier to change them.
> > > 
> > > > +??????? fmt->width? = clamp_t(u32, fmt->width, CIF_ISP_INPUT_W_MIN,
> > > > +????????????????????? CIF_ISP_INPUT_W_MAX);
> > > > +??????? fmt->height = clamp_t(u32, fmt->height, CIF_ISP_INPUT_H_MIN,
> > > > +????????????????????? CIF_ISP_INPUT_H_MAX);
> > > > +??????? break;
> > > > +??? case RKISP1_ISP_PAD_SOURCE_PATH:
> > > > +??????? out_fmt = find_out_fmt(fmt->code);
> > > > +??????? if (out_fmt)
> > > > +??????????? fmt->code = out_fmt->mbus_code;
> > > > +??????? else
> > > > +??????????? fmt->code = rkisp1_isp_output_formats[0].mbus_code;
> > > > +??????? /* window size is set in s_selection */
> > > > +??????? fmt->width? = isp_sd->out_crop.width;
> > > > +??????? fmt->height = isp_sd->out_crop.height;
> > > 
> > > This function operates on the TRY configuration too, in which case you
> > > should use the TRY crop rectangle here, not the ACTIVE one. If you've
> > > already jumped to the review of rkisp1_isp_subdev you know my proposal
> > > to simplify this. Otherwise now may be a good time to do so :-)
> > > 
> > > > +??????? /* full range by default */
> > > > +??????? if (!fmt->quantization)
> > > > +??????????? fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > > +??????? break;
> > > > +??? }
> > > > +
> > > > +??? fmt->field = V4L2_FIELD_NONE;
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_set_fmt(struct v4l2_subdev *sd,
> > > > +???????????????? struct v4l2_subdev_pad_config *cfg,
> > > > +???????????????? struct v4l2_subdev_format *fmt)
> > > > +{
> > > > +??? struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > > +??? struct rkisp1_isp_subdev *isp_sd = &isp_dev->isp_sdev;
> > > > +??? struct v4l2_mbus_framefmt *mf = &fmt->format;
> > > > +
> > > > +??? if ((fmt->pad != RKISP1_ISP_PAD_SINK) &&
> > > > +??????? (fmt->pad != RKISP1_ISP_PAD_SOURCE_PATH))
> > > > +??????? return rkisp1_isp_sd_get_fmt(sd, cfg, fmt);
> > > > +
> > > > +??? rkisp1_isp_sd_try_fmt(sd, fmt->pad, mf);
> > > > +
> > > > +??? if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > > +??????? struct v4l2_mbus_framefmt *try_mf;
> > > > +
> > > > +??????? try_mf = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> > > > +??????? *try_mf = *mf;
> > > > +??????? return 0;
> > > 
> > > When setting the format on the sink pad the crop rectangles need to be
> > > reset (here, and for the ACTIVE format below too).
> > > 
> > > > +??? }
> > > > +
> > > > +??? if (fmt->pad == RKISP1_ISP_PAD_SINK) {
> > > > +??????? const struct ispsd_in_fmt *in_fmt;
> > > > +
> > > > +??????? in_fmt = find_in_fmt(mf->code);
> > > > +??????? isp_sd->in_fmt = *in_fmt;
> > > > +??????? isp_sd->in_frm = *mf;
> > > > +??? } else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_PATH) {
> > > > +??????? const struct ispsd_out_fmt *out_fmt;
> > > > +
> > > > +??????? /* Ignore width/height */
> > > > +??????? out_fmt = find_out_fmt(mf->code);
> > > > +??????? isp_sd->out_fmt = *out_fmt;
> > > 
> > > I would return the in_fmt and out_fmt from rkisp1_isp_sd_try_fmt() as it
> > > already looks them up. If you merge the input and output tables, you'll
> > > have a single format info structure type, and rkisp1_isp_sd_try_fmt()
> > > could return the entry for the pad it operates on.
> > > 
> > > > +??????? /*
> > > > +???????? * It is quantization for output,
> > > > +???????? * isp use bt601 limit-range in internal
> > > > +???????? */
> > > > +??????? isp_sd->quantization = mf->quantization;
> > > > +??? }
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static void rkisp1_isp_sd_try_crop(struct v4l2_subdev *sd,
> > > > +????????????????? struct v4l2_subdev_pad_config *cfg,
> > > > +????????????????? struct v4l2_subdev_selection *sel)
> > > > +{
> > > > +??? struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
> > > > +??? struct v4l2_mbus_framefmt in_frm = isp_sd->in_frm;
> > > > +??? struct v4l2_rect in_crop = isp_sd->in_crop;
> > > > +??? struct v4l2_rect *input = &sel->r;
> > > > +
> > > > +??? if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > > +??????? in_frm = *v4l2_subdev_get_try_format(sd, cfg,
> > > > +???????????????????????????? RKISP1_ISP_PAD_SINK);
> > > > +??????? in_crop = *v4l2_subdev_get_try_crop(sd, cfg,
> > > > +??????????????????????????? RKISP1_ISP_PAD_SINK);
> > > > +??? }
> > > > +
> > > > +??? input->left = ALIGN(input->left, 2);
> > > > +??? input->width = ALIGN(input->width, 2);
> > > > +
> > > > +??? if (sel->pad == RKISP1_ISP_PAD_SINK) {
> > > > +??????? input->left = clamp_t(u32, input->left, 0, in_frm.width);
> > > > +??????? input->top = clamp_t(u32, input->top, 0, in_frm.height);
> > > > +??????? input->width = clamp_t(u32, input->width, CIF_ISP_INPUT_W_MIN,
> > > > +?????????????????????? in_frm.width - input->left);
> > > > +??????? input->height = clamp_t(u32, input->height,
> > > > +??????????????????? CIF_ISP_INPUT_H_MIN,
> > > > +??????????????????? in_frm.height - input->top);
> > > > +??? } else if (sel->pad == RKISP1_ISP_PAD_SOURCE_PATH) {
> > > > +??????? input->left = clamp_t(u32, input->left, 0, in_crop.width);
> > > > +??????? input->top = clamp_t(u32, input->top, 0, in_crop.height);
> > > > +??????? input->width = clamp_t(u32, input->width, CIF_ISP_OUTPUT_W_MIN,
> > > > +?????????????????????? in_crop.width - input->left);
> > > > +??????? input->height = clamp_t(u32, input->height,
> > > > +??????????????????? CIF_ISP_OUTPUT_H_MIN,
> > > > +??????????????????? in_crop.height - input->top);
> > > > +??? }
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_get_selection(struct v4l2_subdev *sd,
> > > > +?????????????????????? struct v4l2_subdev_pad_config *cfg,
> > > > +?????????????????????? struct v4l2_subdev_selection *sel)
> > > > +{
> > > > +??? struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
> > > > +??? struct v4l2_mbus_framefmt *frm;
> > > > +??? struct v4l2_rect *rect;
> > > > +
> > > > +??? if (sel->pad != RKISP1_ISP_PAD_SOURCE_PATH &&
> > > > +??????? sel->pad != RKISP1_ISP_PAD_SINK)
> > > > +??????? return -EINVAL;
> > > > +
> > > > +??? switch (sel->target) {
> > > > +??? case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > +??????? if (sel->pad == RKISP1_ISP_PAD_SINK) {
> > > > +??????????? if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> > > > +??????????????? frm = v4l2_subdev_get_try_format(sd, cfg,
> > > > +???????????????????????????????? sel->pad);
> > > > +??????????? else
> > > > +??????????????? frm = &isp_sd->in_frm;
> > > > +
> > > > +??????????? sel->r.height = frm->height;
> > > > +??????????? sel->r.width = frm->width;
> > > > +??????????? sel->r.left = 0;
> > > > +??????????? sel->r.top = 0;
> > > > +??????? } else {
> > > > +??????????? if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> > > > +??????????????? rect = v4l2_subdev_get_try_crop(sd, cfg,
> > > > +??????????????????????????? RKISP1_ISP_PAD_SINK);
> > > > +??????????? else
> > > > +??????????????? rect = &isp_sd->in_crop;
> > > > +??????????? sel->r = *rect;
> > > > +??????? }
> > > > +??????? break;
> > > > +??? case V4L2_SEL_TGT_CROP:
> > > > +??????? if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> > > > +??????????? rect = v4l2_subdev_get_try_crop(sd, cfg, sel->pad);
> > > > +??????? else if (sel->pad == RKISP1_ISP_PAD_SINK)
> > > > +??????????? rect = &isp_sd->in_crop;
> > > > +??????? else
> > > > +??????????? rect = &isp_sd->out_crop;
> > > > +??????? sel->r = *rect;
> > > > +??????? break;
> > > > +??? default:
> > > > +??????? return -EINVAL;
> > > > +??? }
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_set_selection(struct v4l2_subdev *sd,
> > > > +?????????????????????? struct v4l2_subdev_pad_config *cfg,
> > > > +?????????????????????? struct v4l2_subdev_selection *sel)
> > > > +{
> > > > +??? struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
> > > > +??? struct rkisp1_device *dev = sd_to_isp_dev(sd);
> > > > +
> > > > +??? if (sel->pad != RKISP1_ISP_PAD_SOURCE_PATH &&
> > > > +??????? sel->pad != RKISP1_ISP_PAD_SINK)
> > > > +??????? return -EINVAL;
> > > > +??? if (sel->target != V4L2_SEL_TGT_CROP)
> > > > +??????? return -EINVAL;
> > > > +
> > > > +??? v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
> > > > +???????? "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__, sel->pad,
> > > > +???????? sel->r.left, sel->r.top, sel->r.width, sel->r.height);
> > > > +??? rkisp1_isp_sd_try_crop(sd, cfg, sel);
> > > > +
> > > > +??? if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > > +??????? struct v4l2_rect *try_sel =
> > > > +??????????? v4l2_subdev_get_try_crop(sd, cfg, sel->pad);
> > > > +
> > > > +??????? *try_sel = sel->r;
> > > > +??????? return 0;
> > > 
> > > When setting the crop rectangle on the sink pad the rectangle on the
> > > source pad should be reset (and below for the ACTIVE format too).
> > > 
> > > The resizer logic, through selection rectangles, seems to be missing. I
> > > assume it's configured through the video nodes, but that's not correct
> > > I'm afraid, scaling should be configured on subdevs, see [1]. I'm afraid
> > > this means that we'll need separate subdevs for the resizers :-S
> > > 
> > > [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.html#order-of-configuration-and-format-propagation
> > > 
> > > > +??? }
> > > > +
> > > > +??? if (sel->pad == RKISP1_ISP_PAD_SINK)
> > > > +??????? isp_sd->in_crop = sel->r;
> > > > +??? else
> > > > +??????? isp_sd->out_crop = sel->r;
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > 
> > > I'll skip the part related to power on/off below as Sakari requested it
> > > to be handled through runtime PM.
> > > 
> > > > +static int mipi_csi2_s_stream_start(struct rkisp1_isp_subdev *isp_sd,
> > > > +??????????????????? struct rkisp1_sensor *sensor)
> > > > +{
> > > > +??? union phy_configure_opts opts = { 0 };
> > > > +??? struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
> > > > +??? struct v4l2_ctrl *pixel_rate;
> > > > +??? s64 pixel_clock;
> > > > +
> > > > +??? pixel_rate = v4l2_ctrl_find(sensor->sd->ctrl_handler,
> > > > +??????????????????? V4L2_CID_PIXEL_RATE);
> > > > +??? if (!pixel_rate) {
> > > > +??????? v4l2_warn(sensor->sd, "No pixel rate control in subdev\n");
> > > > +??????? return -EPIPE;
> > > > +??? }
> > > 
> > > Would it make sense to retrieve (and cache) the control pointer (not its
> > > value of course) at probe time already ?
> > > 
> > > > +
> > > > +??? pixel_clock = v4l2_ctrl_g_ctrl_int64(pixel_rate);
> > > > +??? if (!pixel_clock) {
> > > > +??????? v4l2_err(sensor->sd, "Invalid pixel rate value\n");
> > > > +??????? return -EINVAL;
> > > > +??? }
> > > > +
> > > > +??? phy_mipi_dphy_get_default_config(pixel_clock, isp_sd->in_fmt.bus_width,
> > > > +???????????????????? sensor->lanes, cfg);
> > > > +??? phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY);
> > > > +??? phy_configure(sensor->dphy, &opts);
> > > > +??? phy_power_on(sensor->dphy);
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static void mipi_csi2_s_stream_stop(struct rkisp1_sensor *sensor)
> > > > +{
> > > > +??? phy_power_off(sensor->dphy);
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_s_stream(struct v4l2_subdev *sd, int on)
> > > > +{
> > > > +??? struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > > +??? struct v4l2_subdev *sensor_sd;
> > > > +??? int ret = 0;
> > > > +
> > > > +??? if (!on) {
> > > > +??????? ret = rkisp1_isp_stop(isp_dev);
> > > > +??????? if (ret < 0)
> > > > +??????????? return ret;
> > > > +??????? mipi_csi2_s_stream_stop(isp_dev->active_sensor);
> > > > +??????? return 0;
> > > > +??? }
> > > > +
> > > > +??? sensor_sd = get_remote_sensor(sd);
> > > > +??? if (!sensor_sd)
> > > > +??????? return -ENODEV;
> > > > +
> > > > +??? isp_dev->active_sensor = sd_to_sensor(isp_dev, sensor_sd);
> > > > +??? if (!isp_dev->active_sensor)
> > > > +??????? return -ENODEV;
> > > > +
> > > > +??? atomic_set(&isp_dev->isp_sdev.frm_sync_seq, 0);
> > > > +??? ret = rkisp1_config_cif(isp_dev);
> > > > +??? if (ret < 0)
> > > > +??????? return ret;
> > > > +
> > > > +??? /* TODO: support other interfaces */
> > > > +??? if (isp_dev->active_sensor->mbus.type != V4L2_MBUS_CSI2_DPHY)
> > > > +??????? return -EINVAL;
> > > > +
> > > > +??? ret = mipi_csi2_s_stream_start(&isp_dev->isp_sdev,
> > > > +?????????????????????? isp_dev->active_sensor);
> > > > +??? if (ret < 0)
> > > > +??????? return ret;
> > > > +
> > > > +??? ret = rkisp1_isp_start(isp_dev);
> > > > +??? if (ret)
> > > > +??????? mipi_csi2_s_stream_stop(isp_dev->active_sensor);
> > > > +
> > > > +??? return ret;
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > > > +{
> > > > +??? struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > > +??? int ret;
> > > > +
> > > > +??? v4l2_dbg(1, rkisp1_debug, &isp_dev->v4l2_dev, "s_power: %d\n", on);
> > > > +
> > > > +??? if (on) {
> > > > +??????? ret = pm_runtime_get_sync(isp_dev->dev);
> > > > +??????? if (ret < 0)
> > > > +??????????? return ret;
> > > > +
> > > > +??????? rkisp1_config_clk(isp_dev);
> > > > +??? } else {
> > > > +??????? ret = pm_runtime_put(isp_dev->dev);
> > > > +??????? if (ret < 0)
> > > > +??????????? return ret;
> > > > +??? }
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static int rkisp1_subdev_link_validate(struct media_link *link)
> > > > +{
> > > > +??? if (link->source->index == RKISP1_ISP_PAD_SINK_PARAMS)
> > > > +??????? return 0;
> > > > +
> > > > +??? return v4l2_subdev_link_validate(link);
> > > > +}
> > > > +
> > > > +static int rkisp1_subdev_fmt_link_validate(struct v4l2_subdev *sd,
> > > > +??????????????????? struct media_link *link,
> > > > +??????????????????? struct v4l2_subdev_format *source_fmt,
> > > > +??????????????????? struct v4l2_subdev_format *sink_fmt)
> > > > +{
> > > > +??? if (source_fmt->format.code != sink_fmt->format.code)
> > > > +??????? return -EINVAL;
> > > > +
> > > > +??? /* Crop is available */
> > > > +??? if (source_fmt->format.width < sink_fmt->format.width ||
> > > > +??????? source_fmt->format.height < sink_fmt->format.height)
> > > > +??????? return -EINVAL;
> > > 
> > > Crop should be handled on pads, not on links. The size at the ends of
> > > the link should match. This will likely require a bit of a redesign of
> > > the format and selection rectangles handling, but I think it's also an
> > > opportunity to simplify the code.
> > > 
> > > > +
> > > > +??? return 0;
> > > > +}
> > > > +
> > > > +static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *isp)
> > > > +{
> > > > +??? struct v4l2_event event = {
> > > > +??????? .type = V4L2_EVENT_FRAME_SYNC,
> > > > +??????? .u.frame_sync.frame_sequence =
> > > > +??????????? atomic_inc_return(&isp->frm_sync_seq) - 1,
> > > 
> > > I would move the increment to the caller, hiding it in this function is
> > > error-prone (and if you look at the caller I'm pointing out one possible
> > > error :-)).
> > > 
> > > In general usage of frm_sync_seq through the driver seems to be very
> > > race-prone. It's read in various IRQ handling functions, all coming from
> > > the same IRQ, so that part is fine (and wouldn't require an atomic
> > > variable), but when read from the buffer queue handlers I really get a
> > > red light flashing in my head. I'll try to investigate more when
> > > reviewing the next patches.
> > 
> > I see that the only place were 'frame_sequence' is read outside of the irq
> > handlers is in the capture in 'rkisp1_vb2_buf_queue':
> > 
> >  ????/*
> >  ???????? * If there's no next buffer assigned, queue this buffer directly
> >  ???????? * as the next buffer, and update the memory interface.
> >  ???????? */
> >  ??????? if (cap->is_streaming && !cap->buf.next &&
> >  ??????????? atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) {
> >  ??????????????? cap->buf.next = ispbuf;
> >  ??????????????? rkisp1_set_next_buf(cap);
> >  ??????? } else {
> >  ??????????????? list_add_tail(&ispbuf->queue, &cap->buf.queue);
> >  ??????? }
> > This "if" condition seems very specific, a case where we already stream but v-start was not yet received.
> > I think it is possible to remove the test 'atomic_read(&cap->rkisp1->isp.frame_sequence) == -1'
> > from the above condition so that the next buffer is updated in case it is null not just before the first
> > v-start signal.
> > 
> > 
> > 
> > 
> > 
> > > 
> > > > +??? };
> > > > +??? v4l2_event_queue(isp->sd.devnode, &event);
> > > > +}
> > > > +
> > > > +static int rkisp1_isp_sd_subs_evt(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> > > > +????????????????? struct v4l2_event_subscription *sub)
> > > > +{
> > > > +??? if (sub->type != V4L2_EVENT_FRAME_SYNC)
> > > > +??????? return -EINVAL;
> > > > +
> > > > +??? /* Line number. For now only zero accepted. */
> > > 
> > > Is the id a line number for V4L2_EVENT_FRAME_SYNC ? It's not mentioned
> > > for V4L2_EVENT_FRAME_SYNC in the V4L2 spec, so I think this check is
> > > correct, but the comment doesn't seem to be.
> > > 
> > > > +??? if (sub->id != 0)
> > > > +??????? return -EINVAL;
> > > > +
> > > > +??? return v4l2_event_subscribe(fh, sub, 0, NULL);
> > > > +}
> > > > +
> > > > +static const struct v4l2_subdev_pad_ops rkisp1_isp_sd_pad_ops = {
> > > > +??? .enum_mbus_code = rkisp1_isp_sd_enum_mbus_code,
> > > > +??? .get_selection = rkisp1_isp_sd_get_selection,
> > > > +??? .set_selection = rkisp1_isp_sd_set_selection,
> > > > +??? .init_cfg = rkisp1_isp_sd_init_config,
> > > > +??? .get_fmt = rkisp1_isp_sd_get_fmt,
> > > > +??? .set_fmt = rkisp1_isp_sd_set_fmt,
> > > > +??? .link_validate = rkisp1_subdev_fmt_link_validate,
> > > > +};
> > > > +
> > > > +static const struct media_entity_operations rkisp1_isp_sd_media_ops = {
> > > > +??? .link_validate = rkisp1_subdev_link_validate,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_video_ops rkisp1_isp_sd_video_ops = {
> > > > +??? .s_stream = rkisp1_isp_sd_s_stream,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_core_ops rkisp1_isp_core_ops = {
> > > > +??? .subscribe_event = rkisp1_isp_sd_subs_evt,
> > > > +??? .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > > +??? .s_power = rkisp1_isp_sd_s_power,
> > > > +};
> > > > +
> > > > +static struct v4l2_subdev_ops rkisp1_isp_sd_ops = {
> > > 
> > > static const
> > > 
> > > > +??? .core = &rkisp1_isp_core_ops,
> > > > +??? .video = &rkisp1_isp_sd_video_ops,
> > > > +??? .pad = &rkisp1_isp_sd_pad_ops,
> > > > +};
> > > > +
> > > > +static void rkisp1_isp_sd_init_default_fmt(struct rkisp1_isp_subdev *isp_sd)
> > > > +{
> > > > +??? struct v4l2_mbus_framefmt *in_frm = &isp_sd->in_frm;
> > > > +??? struct v4l2_rect *in_crop = &isp_sd->in_crop;
> > > > +??? struct v4l2_rect *out_crop = &isp_sd->out_crop;
> > > > +??? struct ispsd_in_fmt *in_fmt = &isp_sd->in_fmt;
> > > > +??? struct ispsd_out_fmt *out_fmt = &isp_sd->out_fmt;
> > > > +
> > > > +??? *in_fmt = rkisp1_isp_input_formats[0];
> > > > +??? in_frm->width = RKISP1_DEFAULT_WIDTH;
> > > > +??? in_frm->height = RKISP1_DEFAULT_HEIGHT;
> > > > +??? in_frm->code = in_fmt->mbus_code;
> > > > +
> > > > +??? in_crop->width = in_frm->width;
> > > > +??? in_crop->height = in_frm->height;
> > > > +??? in_crop->left = 0;
> > > > +??? in_crop->top = 0;
> > > > +
> > > > +??? /* propagate to source */
> > > > +??? *out_crop = *in_crop;
> > > > +??? *out_fmt = rkisp1_isp_output_formats[0];
> > > > +??? isp_sd->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > 
> > > I wonder, could this be implemented by sharing code with .init_cfg() if
> > > you store the active configuration in v4l2_subdev_pad_config instances
> > > (please see the comments about the rkisp1_isp_subdev structure below) ?
> > > 
> > > > +}
> > > > +
> > > > +int rkisp1_register_isp_subdev(struct rkisp1_device *isp_dev,
> > > > +?????????????????? struct v4l2_device *v4l2_dev)
> > > > +{
> > > > +??? struct rkisp1_isp_subdev *isp_sdev = &isp_dev->isp_sdev;
> > > > +??? struct v4l2_subdev *sd = &isp_sdev->sd;
> > > > +??? int ret;
> > > > +
> > > > +??? v4l2_subdev_init(sd, &rkisp1_isp_sd_ops);
> > > > +??? sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > +??? sd->entity.ops = &rkisp1_isp_sd_media_ops;
> > > > +??? snprintf(sd->name, sizeof(sd->name), "rkisp1-isp-subdev");
> > > > +
> > > > +??? isp_sdev->pads[RKISP1_ISP_PAD_SINK].flags =
> > > > +??????? MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> > > > +??? isp_sdev->pads[RKISP1_ISP_PAD_SINK_PARAMS].flags = MEDIA_PAD_FL_SINK;
> > > > +??? isp_sdev->pads[RKISP1_ISP_PAD_SOURCE_PATH].flags = MEDIA_PAD_FL_SOURCE;
> > > > +??? isp_sdev->pads[RKISP1_ISP_PAD_SOURCE_STATS].flags = MEDIA_PAD_FL_SOURCE;
> > > > +??? sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > > > +??? ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX,
> > > > +???????????????????? isp_sdev->pads);
> > > > +??? if (ret < 0)
> > > > +??????? return ret;
> > > > +
> > > > +??? sd->owner = THIS_MODULE;
> > > > +??? v4l2_set_subdevdata(sd, isp_dev);
> > > > +
> > > > +??? sd->grp_id = GRP_ID_ISP;
> > > 
> > > I think you can drop this, as well as all the GRP_ID_* macros, they're
> > > not used.
> > > 
> > > > +??? ret = v4l2_device_register_subdev(v4l2_dev, sd);
> > > > +??? if (ret < 0) {
> > > > +??????? v4l2_err(sd, "Failed to register isp subdev\n");
> > > > +??????? goto err_cleanup_media_entity;
> > > > +??? }
> > > > +
> > > > +??? rkisp1_isp_sd_init_default_fmt(isp_sdev);
> > > > +
> > > > +??? return 0;
> > > > +err_cleanup_media_entity:
> > > > +??? media_entity_cleanup(&sd->entity);
> > > > +??? return ret;
> > > > +}
> > > > +
> > > > +void rkisp1_unregister_isp_subdev(struct rkisp1_device *isp_dev)
> > > > +{
> > > > +??? struct v4l2_subdev *sd = &isp_dev->isp_sdev.sd;
> > > > +
> > > > +??? v4l2_device_unregister_subdev(sd);
> > > > +??? media_entity_cleanup(&sd->entity);
> > > > +}
> > > > +
> > > > +/****************? Interrupter Handler ****************/
> > > 
> > > s/Handler/Handlers/
> > > 
> > > > +
> > > > +void rkisp1_mipi_isr(unsigned int mis, struct rkisp1_device *dev)
> > > 
> > > I would make the mis field an u32 as it contains register values. Should
> > > it also be renamed to status ? Same for rkisp1_isp_isr() below.
> > > 
> > > > +{
> > > > +??? struct v4l2_device *v4l2_dev = &dev->v4l2_dev;
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? u32 val;
> > > > +
> > > 
> > > How about moving the CIF_MIPI_MIS read here and removing the mis
> > > argument to the function ? It would be more logical to group all
> > > register access related to interrupts in a single place. Same for the
> > > other interrupt handler functions.
> > > 
> > > > +??? writel(~0, base + CIF_MIPI_ICR);
> > > > +
> > > > +??? /*
> > > > +???? * Disable DPHY errctrl interrupt, because this dphy
> > > > +???? * erctrl signal is asserted until the next changes
> > > > +???? * of line state. This time is may be too long and cpu
> > > > +???? * is hold in this interrupt.
> > > > +???? */
> > > > +??? if (mis & CIF_MIPI_ERR_CTRL(0x0f)) {
> > > > +??????? val = readl(base + CIF_MIPI_IMSC);
> > > > +??????? writel(val & ~CIF_MIPI_ERR_CTRL(0x0f), base + CIF_MIPI_IMSC);
> > > > +??????? dev->isp_sdev.dphy_errctrl_disabled = true;
> > > > +??? }
> > > > +
> > > > +??? /*
> > > > +???? * Enable DPHY errctrl interrupt again, if mipi have receive
> > > > +???? * the whole frame without any error.
> > > > +???? */
> > > > +??? if (mis == CIF_MIPI_FRAME_END) {
> > > > +??????? /*
> > > > +???????? * Enable DPHY errctrl interrupt again, if mipi have receive
> > > > +???????? * the whole frame without any error.
> > > > +???????? */
> > > > +??????? if (dev->isp_sdev.dphy_errctrl_disabled) {
> > > > +??????????? val = readl(base + CIF_MIPI_IMSC);
> > > > +??????????? val |= CIF_MIPI_ERR_CTRL(0x0f);
> > > > +??????????? writel(val, base + CIF_MIPI_IMSC);
> > > > +??????????? dev->isp_sdev.dphy_errctrl_disabled = false;
> > > > +??????? }
> > > > +??? } else {
> > > > +??????? v4l2_warn(v4l2_dev, "MIPI mis error: 0x%08x\n", mis);
> > > > +??? }
> > > > +}
> > > > +
> > > > +void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev)
> > > > +{
> > > > +??? void __iomem *base = dev->base_addr;
> > > > +??? unsigned int isp_mis_tmp = 0;
> > > 
> > > _tmp are never good names :-S
> > > 
> > > > +??? unsigned int isp_err = 0;
> > > 
> > > Neither of these variable need to be initialised to 0.
> > > 
> > > > +
> > > > +??? /* start edge of v_sync */
> > > > +??? if (isp_mis & CIF_ISP_V_START) {
> > > > +??????? rkisp1_isp_queue_event_sof(&dev->isp_sdev);
> > > 
> > > This will increment the frame sequence number. What if the interrupt is
> > > slightly delayed and the next frame starts before we get a change to
> > > copy the sequence number to the buffers (before they will complete
> > > below) ?
> > 
> > Do you mean that we get two sequental v-start signals and then the next
> > frame-end signal in MI_MIS belongs to the first v-start signal of the two?
> > How can this be solved? I wonder if any v-start signal has a later signal
> > that correspond to the same frame so that we can follow it?
> > 
> > Maybe we should have one counter that is incremented on v-start signal,
> > and another counter that is incremented uppon some other signal?
> > 
> > > 
> > > > +
> > > > +??????? writel(CIF_ISP_V_START, base + CIF_ISP_ICR);
> > > 
> > > Do you need to clear all interrupt bits individually, can't you write
> > > isp_mis to CIF_ISP_ICR at the beginning of the function to clear them
> > > all in one go ?
> > > 
> > > > +??????? isp_mis_tmp = readl(base + CIF_ISP_MIS);
> > > > +??????? if (isp_mis_tmp & CIF_ISP_V_START)
> > > > +??????????? v4l2_err(&dev->v4l2_dev, "isp icr v_statr err: 0x%x\n",
> > > > +???????????????? isp_mis_tmp);
> > > 
> > > This require some explanation. It looks like a naive way to protect
> > > against something, but I think it could trigger under normal
> > > circumstances if IRQ handling is delayed, and wouldn't do much anyway.
> > > Same for the similar constructs below.
> > > 
> > > > +??? }
> > > > +
> > > > +??? if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) {
> > > > +??????? /* Clear pic_size_error */
> > > > +??????? writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_ISP_ICR);
> > > > +??????? isp_err = readl(base + CIF_ISP_ERR);
> > > > +??????? v4l2_err(&dev->v4l2_dev,
> > > > +???????????? "CIF_ISP_PIC_SIZE_ERROR (0x%08x)", isp_err);
> > > 
> > > What does this mean ?
> > > 
> > > > +??????? writel(isp_err, base + CIF_ISP_ERR_CLR);
> > > > +??? } else if ((isp_mis & CIF_ISP_DATA_LOSS)) {
> > > 
> > > Are CIF_ISP_PIC_SIZE_ERROR and CIF_ISP_DATA_LOSS mutually exclusive ?
> > > 
> > > > +??????? /* Clear data_loss */
> > > > +??????? writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR);
> > > > +??????? v4l2_err(&dev->v4l2_dev, "CIF_ISP_DATA_LOSS\n");
> > > > +??????? writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR);
> > > > +??? }
> > > > +
> > > > +??? /* sampled input frame is complete */
> > > > +??? if (isp_mis & CIF_ISP_FRAME_IN) {
> > > > +??????? writel(CIF_ISP_FRAME_IN, base + CIF_ISP_ICR);
> > > > +??????? isp_mis_tmp = readl(base + CIF_ISP_MIS);
> > > > +??????? if (isp_mis_tmp & CIF_ISP_FRAME_IN)
> > > > +??????????? v4l2_err(&dev->v4l2_dev, "isp icr frame_in err: 0x%x\n",
> > > > +???????????????? isp_mis_tmp);
> > > > +??? }
> > > > +
> > > > +??? /* frame was completely put out */
> > > 
> > > "put out" ? :-) What's the difference between ISP_FRAME_IN and ISP_FRAME
> > > ? The two comments could do with a bit of brush up, and I think the
> > > ISP_FRAME_IN interrupt could be disabled as it doesn't perform any
> > > action.
> > 
> > Those two oneline comments are just copy-paste from the datasheet.
> > 
> > ""
> > 5 MIS_FRAME_IN sampled input frame is complete
> > 1 MIS_FRAME frame was completely put out
> > ""
> > 
> > Unfrotunately, the datasheet does not add any further explanation about those signals.
> > 
> > 
> > > 
> > > > +??? if (isp_mis & CIF_ISP_FRAME) {
> > > > +??????? u32 isp_ris = 0;
> > > 
> > > No need to initialise this to 0.
> > > 
> > > > +??????? /* Clear Frame In (ISP) */
> > > > +??????? writel(CIF_ISP_FRAME, base + CIF_ISP_ICR);
> > > > +??????? isp_mis_tmp = readl(base + CIF_ISP_MIS);
> > > > +??????? if (isp_mis_tmp & CIF_ISP_FRAME)
> > > > +??????????? v4l2_err(&dev->v4l2_dev,
> > > > +???????????????? "isp icr frame end err: 0x%x\n", isp_mis_tmp);
> > > > +
> > > > +??????? isp_ris = readl(base + CIF_ISP_RIS);
> > > > +??????? if (isp_ris & (CIF_ISP_AWB_DONE | CIF_ISP_AFM_FIN |
> > > > +?????????????????? CIF_ISP_EXP_END | CIF_ISP_HIST_MEASURE_RDY))
> > > > +??????????? rkisp1_stats_isr(&dev->stats_vdev, isp_ris);
> > > 
> > > Is there a guarantee that the statistics will be fully written out
> > > before the video frame itself ? And doesn't this test if any of the
> > > statistics is complete, not all of them ? I think the logic is wrong, it
> > 
> > The datasheet does not add any explanation of what is expected to come first.
> > Should we wait until all statistics measurements are done? In the struct
> > sent to userspace there is a bitmaks for which of the statistics are read.
> > I think that if only part of the statistics are ready, we can already send the once
> > that are ready to userspace.
> > 
> > > seems it should be moved out of the CIF_ISP_FRAME test, to a test of its
> > > own. It's hard to tell for sure without extra information though (for
> > > instance why are the stats-related bits read from CIF_ISP_RIS, when
> > > they seem to be documented as valid in CIF_ISP_ISR), but this should be
> > > validated, and most probably fixed. Care should be taken to keep
> > > synchronisation of sequence number between the different queues.
> > 
> > I see that the capture buffers are done before incrementing the frame_sequence with
> > the following explanation:
> > 
> >  ????/*
> >  ???????? * Call rkisp1_capture_isr() first to handle the frame that
> >  ???????? * potentially completed using the current frame_sequence number before
> >  ???????? * it is potentially incremented by rkisp1_isp_isr() in the vertical
> >  ???????? * sync.
> >  ???????? */
> > 
> > I think reading the stats/params should also be done before calling rkisp1_capture_isr
> > for the same reason. (so to match the correct frame_sequence)
> > 
> > Thanks,
> > Dafna
> 
> 
> I am not sure what should be the meaning of the sequence field for parameters buffers.
> Currently the rkisp1-params entity returns the first buffer queued right away with the frame_sequence that it reads.
> If streaming did not yet started then the first buffer is used as the initial configuration that overrides the driver's default.
> In that case the first params buffer is not associated with any captured frame and has a sequence value '-1'
> Is this a valid behavior?

-1 certainly doesn't sound like a valid sequence number. I think it
should correspond to the frame that the parameters were (or are about to
be) applied to.

Best regards,
Tomasz