From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5AAD2C43459 for ; Wed, 22 Jul 2020 16:02:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1EA3E206F5 for ; Wed, 22 Jul 2020 16:02:14 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="iQQer+2W" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728194AbgGVQCN (ORCPT ); Wed, 22 Jul 2020 12:02:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38030 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727044AbgGVQCD (ORCPT ); Wed, 22 Jul 2020 12:02:03 -0400 Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C0BAFC0619E0 for ; Wed, 22 Jul 2020 09:02:02 -0700 (PDT) Received: by mail-wr1-x441.google.com with SMTP id f1so1879635wro.2 for ; Wed, 22 Jul 2020 09:02:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=mXXliQ0NzdJ99v1+T4OT/Ul5swu8pwO0o8XdxbyN0kM=; b=iQQer+2WkVuA1oP3PamadlOz9WuQW3NJnq06Qr6+QjXjQpzVFzir7OsaC0AhYSaWtP eN3kMurAfbgskdMfpLKQMqFOehTXbdj2Q5xpU1sB1dhF3I0jiodFntkXxC6Wgh18cpSi xKwAPIjvE0zLvP326eeKlYa+hsqAcfr76w42k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=mXXliQ0NzdJ99v1+T4OT/Ul5swu8pwO0o8XdxbyN0kM=; b=LamsX/lqaGdFkFgpE8HGWMJt6ZZCuGd+n1u9rLi4TCsjfX5TUPRePmxMS8BDHMpvci LzGVKnfS8qeYeD0Rs6XKkxuRu9HO+1kFg/dZXFInT6USlJLVw0lf/0eatlmPIrHv97sf s5wv6GM6R++OQNIFfGAbpBNwypl9vWMwPOdt/iNjt5V034gvsG2gc53DN/fNpfJ6WvNx bY0Nva4coFA+u87bWAqgr2mLRAo5fCtqzLi3mWgeXA74nkfaStyYLrhuaBm2ysIzLZ8s NMcYPLEDSXmLTFWTp4jblcQliz1ML5LAQMatCOkixIkN7sAzvEcGhJamud91mLNeFlVT Karg== X-Gm-Message-State: AOAM533aoZpFHmTTPL4itMgbv28z1Ka6RloHqxwOac6TTsFz08jeMvnV GNoSa/pyoZYoWOwrqcfJP2U6Sg== X-Google-Smtp-Source: ABdhPJy8cleKYzNQyOiQCvDg4VKzmgk1AHumPSxlX7ljlxKG/tXrwUQEaDu4I19lQxS7c/qzmlAZ4Q== X-Received: by 2002:a5d:544f:: with SMTP id w15mr254749wrv.208.1595433720722; Wed, 22 Jul 2020 09:02:00 -0700 (PDT) Received: from chromium.org (205.215.190.35.bc.googleusercontent.com. [35.190.215.205]) by smtp.gmail.com with ESMTPSA id 65sm506824wre.6.2020.07.22.09.01.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jul 2020 09:01:59 -0700 (PDT) Date: Wed, 22 Jul 2020 16:01:58 +0000 From: Tomasz Figa To: Dafna Hirschfeld Cc: Laurent Pinchart , Helen Koike , 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 , Allon Huang 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-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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 > > > > > > > > Add the subdev driver for rockchip isp1. > > > > > > > > Signed-off-by: Jacob Chen > > > > Signed-off-by: Shunqian Zheng > > > > Signed-off-by: Yichong Zhong > > > > Signed-off-by: Jacob Chen > > > > Signed-off-by: Eddie Cai > > > > Signed-off-by: Jeffy Chen > > > > Signed-off-by: Allon Huang > > > > Signed-off-by: Tomasz Figa > > > > [fixed unknown entity type / switched to PIXEL_RATE] > > > > Signed-off-by: Ezequiel Garcia > > > > [update for upstream] > > > > Signed-off-by: Helen Koike > > > > > > > > --- > > > > > > > > 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 > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#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