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.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,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 054E2C433DF for ; Wed, 22 Jul 2020 16:04:30 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A6516206F5 for ; Wed, 22 Jul 2020 16:04:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="XDWuhgyq"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="iQQer+2W" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A6516206F5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=V9X/wpKds4CwnXvaSrgELT0MwEZG5+5+whwoMSc8dvw=; b=XDWuhgyqpMwqAXX41BkLdbCIn 6lrRgKDURqAgXheOzfIfL3iUYJFqMWBTaD8dvkgNE/k3LJ+BeGSBVzA03xxVQC16h4YZXSOlb9KJz k6hX9dIXxs7ol4YBsmIU9XCxvYm/j6j6PwVYlIkb+oHu7wk9j072xSxIGo3ChUSjkFjIlSGNOT3w9 o2TzAMb29Xr0vdONoyo5IImff6o7bd6sSuHFtvSvnVpsL9d1d7IvEmIJwpRUXUClleCRNDBza8OrI Eq5oAp86nD8LRImlDJpo4rAlf65Gb71LCt9hxVyc1SL1yTWu0/pRS68zXmmjZV4ULDMkO8XAprdbT WNSlC5AhA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyHC6-00042Q-Px; Wed, 22 Jul 2020 16:02:10 +0000 Received: from mail-wr1-x443.google.com ([2a00:1450:4864:20::443]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyHBz-0003yu-W7 for linux-arm-kernel@lists.infradead.org; Wed, 22 Jul 2020 16:02:08 +0000 Received: by mail-wr1-x443.google.com with SMTP id b6so2404053wrs.11 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=PYuchlfg7ZW+sp2Ng0jUUPzDCOmnAAnj7l4VGq8jjsSlhwVzunYxOPfdmEOHJWQ3mz STMqRUR1Qux6a4EstggZUI88Oc7EBpAYJmJtl7L8Xt/MsgA5v+RDjiuV+lDTMKKPy9o5 GMEkZzqK4WqQlSMtGOXl1G2wwxJR+ArqN5HMcKV+4ZXMHdNRH7zn1Xmv4e59hAEysQs2 eUQ7n2x/Cb8MAgwcosGQI+iui+8SNDg6aq41nPwgdzg5tuOmF8OYOddRP+rqekTery/K SxNS7xaaACAYKEmbM7QOSshM0uM8vdm/EB4CrXg/AtAxfwTXtudj/7EyyW0GshXYgB4K g1bA== X-Gm-Message-State: AOAM530RoUvtRKvJyAIfEOF+qLl1A0CY5YP5UIuEKwmo/BlSSx08IaHX nLBJ/X7ZqwsPNknfIp6CS0pnZyL1R1kmnw== 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 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-Disposition: inline In-Reply-To: <477cb6ec-1d78-b30c-f84d-388c1bde2567@collabora.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200722_120204_247954_9793040D X-CRM114-Status: GOOD ( 42.23 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, eddie.cai.linux@gmail.com, kernel@collabora.com, heiko@sntech.de, jacob2.chen@rock-chips.com, jeffy.chen@rock-chips.com, zyc@rock-chips.com, linux-kernel@vger.kernel.org, Allon Huang , linux-rockchip@lists.infradead.org, Helen Koike , Jacob Chen , hans.verkuil@cisco.com, Laurent Pinchart , sakari.ailus@linux.intel.com, zhengsq@rock-chips.com, mchehab@kernel.org, ezequiel@collabora.com, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Archived-At: List-Archive: 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 t= he > > > > hack in mipidphy_g_mbus_config() where information from the sensor = was > > > > being propagated through the topology. > > > > - From the old dphy: > > > > =A0=A0=A0=A0=A0=A0=A0=A0 * cache get_remote_sensor() in s_stream > > > > =A0=A0=A0=A0=A0=A0=A0=A0 * 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 s= tream) > > > > - 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 ign= ored) > > > > - s/intput/input > > > > - remove #define sd_to_isp_sd(_sd), add a static inline as it will = be > > > > reused by the capture > > > > = > > > > =A0 drivers/media/platform/rockchip/isp1/rkisp1.c | 1286 ++++++++++= +++++++ > > > > =A0 drivers/media/platform/rockchip/isp1/rkisp1.h |=A0 111 ++ > > > > =A0 2 files changed, 1397 insertions(+) > > > > =A0 create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.c > > > > =A0 create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.h > > > > = > > > > diff --git a/drivers/media/platform/rockchip/isp1/rkisp1.c b/driver= s/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 > > > = > > > =A0 * 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 b= it > > > annoying when reviewing patches in order :-S > > > = > > > > + > > > > +#define CIF_ISP_INPUT_W_MAX=A0=A0=A0=A0=A0=A0=A0 4032 > > > > +#define CIF_ISP_INPUT_H_MAX=A0=A0=A0=A0=A0=A0=A0 3024 > > > > +#define CIF_ISP_INPUT_W_MIN=A0=A0=A0=A0=A0=A0=A0 32 > > > > +#define CIF_ISP_INPUT_H_MIN=A0=A0=A0=A0=A0=A0=A0 32 > > > > +#define CIF_ISP_OUTPUT_W_MAX=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_INPUT_W_= MAX > > > > +#define CIF_ISP_OUTPUT_H_MAX=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_INPUT_H_= MAX > > > > +#define CIF_ISP_OUTPUT_W_MIN=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_INPUT_W_= MIN > > > > +#define CIF_ISP_OUTPUT_H_MIN=A0=A0=A0=A0=A0=A0=A0 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 siz= e,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=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 | > > > > + * | +---------------------------------------------------+=A0=A0 | > > > > + * | | ISP_ACQ (for black level)=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0=A0 | > > > > + * | | in_frm=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 |=A0=A0 | > > > > + * | | +--------------------------------------------+=A0=A0=A0 |= =A0=A0 | > > > > + * | | |=A0=A0=A0 ISP_OUT=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0=A0=A0 |=A0= =A0 | > > > > + * | | |=A0=A0=A0 in_crop=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0=A0=A0 |=A0= =A0 | > > > = > > > 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= ? > > > = > > > > + * | | |=A0=A0=A0 +---------------------------------+=A0=A0=A0=A0 = |=A0=A0=A0 |=A0=A0 | > > > > + * | | |=A0=A0=A0 |=A0=A0 ISP_IS=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0=A0=A0=A0 |=A0=A0=A0 |=A0=A0 | > > > > + * | | |=A0=A0=A0 |=A0=A0 rkisp1_isp_subdev: out_crop=A0=A0 |=A0= =A0=A0=A0 |=A0=A0=A0 |=A0=A0 | > > > > + * | | |=A0=A0=A0 +---------------------------------+=A0=A0=A0=A0 = |=A0=A0=A0 |=A0=A0 | > > > > + * | | +--------------------------------------------+=A0=A0=A0 |= =A0=A0 | > > > > + * | +---------------------------------------------------+=A0=A0 | > > > > + * +---------------------------------------------------------+ > > > > + */ > > > > + > > > > +static inline struct rkisp1_device *sd_to_isp_dev(struct v4l2_subd= ev *sd) > > > > +{ > > > > +=A0=A0=A0 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 *s= d) > > > > +{ > > > > +=A0=A0=A0 struct media_pad *local, *remote; > > > > +=A0=A0=A0 struct media_entity *sensor_me; > > > > + > > > > +=A0=A0=A0 local =3D &sd->entity.pads[RKISP1_ISP_PAD_SINK]; > > > > +=A0=A0=A0 remote =3D media_entity_remote_pad(local); > > > > +=A0=A0=A0 if (!remote) { > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_warn(sd, "No link between isp and senso= r\n"); > > > > +=A0=A0=A0=A0=A0=A0=A0 return NULL; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 sensor_me =3D media_entity_remote_pad(local)->entity; > > > > +=A0=A0=A0 return media_entity_to_v4l2_subdev(sensor_me); > > > > +} > > > > + > > > > +static struct rkisp1_sensor *sd_to_sensor(struct rkisp1_device *de= v, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0 struct v4l2_subdev *sd) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_sensor *sensor; > > > > + > > > > +=A0=A0=A0 list_for_each_entry(sensor, &dev->sensors, list) > > > > +=A0=A0=A0=A0=A0=A0=A0 if (sensor->sd =3D=3D sd) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return sensor; > > > > + > > > > +=A0=A0=A0 return NULL; > > > > +} > > > > + > > > > +/****************=A0 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) > > > > +{ > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 struct v4l2_rect *out_crop =3D &dev->isp_sdev.out_crop; > > > > +=A0=A0=A0 u32 val; > > > > + > > > > +=A0=A0=A0 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. > > > = > > > > +=A0=A0=A0 writel(0, base + CIF_ISP_IS_MAX_DX); > > > > +=A0=A0=A0 writel(0, base + CIF_ISP_IS_MAX_DY); > > > > +=A0=A0=A0 writel(0, base + CIF_ISP_IS_DISPLACE); > > > > +=A0=A0=A0 writel(out_crop->left, base + CIF_ISP_IS_H_OFFS); > > > > +=A0=A0=A0 writel(out_crop->top, base + CIF_ISP_IS_V_OFFS); > > > > +=A0=A0=A0 writel(out_crop->width, base + CIF_ISP_IS_H_SIZE); > > > > +=A0=A0=A0 writel(out_crop->height, base + CIF_ISP_IS_V_SIZE); > > > > + > > > > +=A0=A0=A0 /* IS(Image Stabilization) is always on, working as outp= ut crop */ > > > > +=A0=A0=A0 writel(1, base + CIF_ISP_IS_CTRL); > > > > +=A0=A0=A0 val =3D readl(base + CIF_ISP_CTRL); > > > > +=A0=A0=A0 val |=3D CIF_ISP_CTRL_ISP_CFG_UPD; > > > > +=A0=A0=A0 writel(val, base + CIF_ISP_CTRL); > > > > +} > > > > + > > > > +/* > > > > + * configure isp blocks with input format, size...... > > > > + */ > > > > +static int rkisp1_config_isp(struct rkisp1_device *dev) > > > > +{ > > > > +=A0=A0=A0 u32 isp_ctrl =3D 0, irq_mask =3D 0, acq_mult =3D 0, sign= al =3D 0; > > > > +=A0=A0=A0 struct v4l2_rect *out_crop, *in_crop; > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 struct v4l2_mbus_framefmt *in_frm; > > > > +=A0=A0=A0 struct ispsd_out_fmt *out_fmt; > > > > +=A0=A0=A0 struct rkisp1_sensor *sensor; > > > > +=A0=A0=A0 struct ispsd_in_fmt *in_fmt; > > > > + > > > > +=A0=A0=A0 sensor =3D dev->active_sensor; > > > > +=A0=A0=A0 in_frm =3D &dev->isp_sdev.in_frm; > > > > +=A0=A0=A0 in_fmt =3D &dev->isp_sdev.in_fmt; > > > > +=A0=A0=A0 out_fmt =3D &dev->isp_sdev.out_fmt; > > > > +=A0=A0=A0 out_crop =3D &dev->isp_sdev.out_crop; > > > > +=A0=A0=A0 in_crop =3D &dev->isp_sdev.in_crop; > > > > + > > > > +=A0=A0=A0 if (in_fmt->fmt_type =3D=3D FMT_BAYER) { > > > > +=A0=A0=A0=A0=A0=A0=A0 acq_mult =3D 1; > > > > +=A0=A0=A0=A0=A0=A0=A0 if (out_fmt->fmt_type =3D=3D FMT_BAYER) { > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L= 2_MBUS_BT656) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_ctrl =3D > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_= CTRL_ISP_MODE_RAW_PICT_ITU656; > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_ctrl =3D > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_= CTRL_ISP_MODE_RAW_PICT; > > > > +=A0=A0=A0=A0=A0=A0=A0 } else { > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 writel(CIF_ISP_DEMOSAIC_TH(0xc), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 base + CIF_= ISP_DEMOSAIC); > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L= 2_MBUS_BT656) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_ctrl =3D CIF_ISP= _CTRL_ISP_MODE_BAYER_ITU656; > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_ctrl =3D CIF_ISP= _CTRL_ISP_MODE_BAYER_ITU601; > > > > +=A0=A0=A0=A0=A0=A0=A0 } > > > > +=A0=A0=A0 } else if (in_fmt->fmt_type =3D=3D FMT_YUV) { > > > > +=A0=A0=A0=A0=A0=A0=A0 acq_mult =3D 2; > > > > +=A0=A0=A0=A0=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L2_MBUS_CSI2_= DPHY) { > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_ctrl =3D CIF_ISP_CTRL_ISP_MO= DE_ITU601; > > > > +=A0=A0=A0=A0=A0=A0=A0 } else { > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L= 2_MBUS_BT656) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_ctrl =3D CIF_ISP= _CTRL_ISP_MODE_ITU656; > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_ctrl =3D CIF_ISP= _CTRL_ISP_MODE_ITU601; > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 irq_mask |=3D CIF_ISP_DATA_LOSS; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 /* Set up input acquisition properties */ > > > > +=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L2_MBUS_BT656 || > > > > +=A0=A0=A0=A0=A0=A0=A0 sensor->mbus.type =3D=3D V4L2_MBUS_PARALLEL)= { > > > > +=A0=A0=A0=A0=A0=A0=A0 if (sensor->mbus.flags & V4L2_MBUS_PCLK_SAMP= LE_RISING) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 signal =3D CIF_ISP_ACQ_PROP_POS_= EDGE; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L2_MBUS_PARALLEL) { > > > > +=A0=A0=A0=A0=A0=A0=A0 if (sensor->mbus.flags & V4L2_MBUS_VSYNC_ACT= IVE_LOW) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 signal |=3D CIF_ISP_ACQ_PROP_VSY= NC_LOW; > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 if (sensor->mbus.flags & V4L2_MBUS_HSYNC_ACT= IVE_LOW) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 signal |=3D CIF_ISP_ACQ_PROP_HSY= NC_LOW; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 writel(isp_ctrl, base + CIF_ISP_CTRL); > > > > +=A0=A0=A0 writel(signal | in_fmt->yuv_seq | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_ACQ_PROP_BAYER_PAT(in_fmt->= bayer_pat) | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_ACQ_PROP_FIELD_SEL_ALL, bas= e + CIF_ISP_ACQ_PROP); > > > > +=A0=A0=A0 writel(0, base + CIF_ISP_ACQ_NR_FRAMES); > > > > + > > > > +=A0=A0=A0 /* Acquisition Size */ > > > > +=A0=A0=A0 writel(0, base + CIF_ISP_ACQ_H_OFFS); > > > > +=A0=A0=A0 writel(0, base + CIF_ISP_ACQ_V_OFFS); > > > > +=A0=A0=A0 writel(acq_mult * in_frm->width, base + CIF_ISP_ACQ_H_SI= ZE); > > > > +=A0=A0=A0 writel(in_frm->height, base + CIF_ISP_ACQ_V_SIZE); > > > > + > > > > +=A0=A0=A0 /* ISP Out Area */ > > > > +=A0=A0=A0 writel(in_crop->left, base + CIF_ISP_OUT_H_OFFS); > > > > +=A0=A0=A0 writel(in_crop->top, base + CIF_ISP_OUT_V_OFFS); > > > > +=A0=A0=A0 writel(in_crop->width, base + CIF_ISP_OUT_H_SIZE); > > > > +=A0=A0=A0 writel(in_crop->height, base + CIF_ISP_OUT_V_SIZE); > > > > + > > > > +=A0=A0=A0 /* interrupt mask */ > > > > +=A0=A0=A0 irq_mask |=3D CIF_ISP_FRAME | CIF_ISP_V_START | CIF_ISP_= PIC_SIZE_ERROR | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_FRAME_IN; > > > > +=A0=A0=A0 writel(irq_mask, base + CIF_ISP_IMSC); > > > > + > > > > +=A0=A0=A0 if (out_fmt->fmt_type =3D=3D FMT_BAYER) > > > > +=A0=A0=A0=A0=A0=A0=A0 rkisp1_params_disable_isp(&dev->params_vdev); > > > > +=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0 rkisp1_params_configure_isp(&dev->params_vde= v, in_fmt, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 dev->isp_sdev.quantization); > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static int rkisp1_config_dvp(struct rkisp1_device *dev) > > > > +{ > > > > +=A0=A0=A0 struct ispsd_in_fmt *in_fmt =3D &dev->isp_sdev.in_fmt; > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 u32 val, input_sel; > > > > + > > > > +=A0=A0=A0 switch (in_fmt->bus_width) { > > > > +=A0=A0=A0 case 8: > > > > +=A0=A0=A0=A0=A0=A0=A0 input_sel =3D CIF_ISP_ACQ_PROP_IN_SEL_8B_ZER= O; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 case 10: > > > > +=A0=A0=A0=A0=A0=A0=A0 input_sel =3D CIF_ISP_ACQ_PROP_IN_SEL_10B_ZE= RO; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 case 12: > > > > +=A0=A0=A0=A0=A0=A0=A0 input_sel =3D CIF_ISP_ACQ_PROP_IN_SEL_12B; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 default: > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_err(&dev->v4l2_dev, "Invalid bus width\= n"); > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 val =3D readl(base + CIF_ISP_ACQ_PROP); > > > > +=A0=A0=A0 writel(val | input_sel, base + CIF_ISP_ACQ_PROP); > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static int rkisp1_config_mipi(struct rkisp1_device *dev) > > > > +{ > > > > +=A0=A0=A0 struct ispsd_in_fmt *in_fmt =3D &dev->isp_sdev.in_fmt; > > > > +=A0=A0=A0 struct rkisp1_sensor *sensor =3D dev->active_sensor; > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 unsigned int lanes; > > > > +=A0=A0=A0 u32 mipi_ctrl; > > > > + > > > > +=A0=A0=A0 /* > > > > +=A0=A0=A0=A0 * sensor->mbus is set in isp or d-phy notifier_bound = function > > > > +=A0=A0=A0=A0 */ > > > > +=A0=A0=A0 switch (sensor->mbus.flags & V4L2_MBUS_CSI2_LANES) { > > > > +=A0=A0=A0 case V4L2_MBUS_CSI2_4_LANE: > > > > +=A0=A0=A0=A0=A0=A0=A0 lanes =3D 4; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 case V4L2_MBUS_CSI2_3_LANE: > > > > +=A0=A0=A0=A0=A0=A0=A0 lanes =3D 3; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 case V4L2_MBUS_CSI2_2_LANE: > > > > +=A0=A0=A0=A0=A0=A0=A0 lanes =3D 2; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 case V4L2_MBUS_CSI2_1_LANE: > > > > +=A0=A0=A0=A0=A0=A0=A0 lanes =3D 1; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 default: > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 mipi_ctrl =3D CIF_MIPI_CTRL_NUM_LANES(lanes - 1) | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_MIPI_CTRL_SHUTDOWNLANES(0xf)= | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_MIPI_CTRL_ERR_SOT_SYNC_HS_SK= IP | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_MIPI_CTRL_CLOCKLANE_ENA; > > > > + > > > > +=A0=A0=A0 writel(mipi_ctrl, base + CIF_MIPI_CTRL); > > > > + > > > > +=A0=A0=A0 /* Configure Data Type and Virtual Channel */ > > > > +=A0=A0=A0 writel(CIF_MIPI_DATA_SEL_DT(in_fmt->mipi_dt) | CIF_MIPI_= DATA_SEL_VC(0), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 base + CIF_MIPI_IMG_DATA_SEL); > > > > + > > > > +=A0=A0=A0 /* Clear MIPI interrupts */ > > > > +=A0=A0=A0 writel(~0, base + CIF_MIPI_ICR); > > > > +=A0=A0=A0 /* > > > > +=A0=A0=A0=A0 * Disable CIF_MIPI_ERR_DPHY interrupt here temporary = for > > > > +=A0=A0=A0=A0 * isp bus may be dead when switch isp. > > > > +=A0=A0=A0=A0 */ > > > > +=A0=A0=A0 writel(CIF_MIPI_FRAME_END | CIF_MIPI_ERR_CSI | CIF_MIPI_= ERR_DPHY | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_MIPI_SYNC_FIFO_OVFLW(0x03) | CI= F_MIPI_ADD_DATA_OVFLW, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 base + CIF_MIPI_IMSC); > > > > + > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, "\n=A0 MIPI_CT= RL 0x%08x\n" > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "=A0 MIPI_IMG_DATA_SEL 0x%08x\n" > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "=A0 MIPI_STATUS 0x%08x\n" > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "=A0 MIPI_IMSC 0x%08x\n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MIPI_CTRL), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MIPI_IMG_DATA_SEL), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MIPI_STATUS), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MIPI_IMSC)); > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +/* Configure MUX */ > > > > +static int rkisp1_config_path(struct rkisp1_device *dev) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_sensor *sensor =3D dev->active_sensor; > > > > +=A0=A0=A0 u32 dpcl =3D readl(dev->base_addr + CIF_VI_DPCL); > > > > +=A0=A0=A0 int ret =3D 0; > > > > + > > > > +=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L2_MBUS_BT656 || > > > > +=A0=A0=A0=A0=A0=A0=A0 sensor->mbus.type =3D=3D V4L2_MBUS_PARALLEL)= { > > > > +=A0=A0=A0=A0=A0=A0=A0 ret =3D rkisp1_config_dvp(dev); > > > > +=A0=A0=A0=A0=A0=A0=A0 dpcl |=3D CIF_VI_DPCL_IF_SEL_PARALLEL; > > > > +=A0=A0=A0 } else if (sensor->mbus.type =3D=3D V4L2_MBUS_CSI2_DPHY)= { > > > > +=A0=A0=A0=A0=A0=A0=A0 ret =3D rkisp1_config_mipi(dev); > > > > +=A0=A0=A0=A0=A0=A0=A0 dpcl |=3D CIF_VI_DPCL_IF_SEL_MIPI; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 writel(dpcl, dev->base_addr + CIF_VI_DPCL); > > > > + > > > > +=A0=A0=A0 return ret; > > > > +} > > > > + > > > > +/* Hareware configure Entry */ > > > = > > > s/Hareware/Hardware/ > > > = > > > > +static int rkisp1_config_cif(struct rkisp1_device *dev) > > > > +{ > > > > +=A0=A0=A0 u32 cif_id; > > > > +=A0=A0=A0 int ret; > > > > + > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "SP streaming =3D %d, MP streaming =3D %d= \n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_SP].streaming, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_MP].streaming); > > > > + > > > > +=A0=A0=A0 cif_id =3D readl(dev->base_addr + CIF_VI_ID); > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, "CIF_ID 0x%08x= \n", cif_id); > > > > + > > > > +=A0=A0=A0 ret =3D rkisp1_config_isp(dev); > > > > +=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > +=A0=A0=A0 ret =3D rkisp1_config_path(dev); > > > > +=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > +=A0=A0=A0 rkisp1_config_ism(dev); > > > > + > > > > +=A0=A0=A0 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) > > > > +{ > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 u32 val; > > > > + > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "SP streaming =3D %d, MP streaming =3D %d= \n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_SP].streaming, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_MP].streaming); > > > > + > > > > +=A0=A0=A0 /* > > > > +=A0=A0=A0=A0 * ISP(mi) stop in mi frame end -> Stop ISP(mipi) -> > > > > +=A0=A0=A0=A0 * Stop ISP(isp) ->wait for ISP isp off > > > > +=A0=A0=A0=A0 */ > > > > +=A0=A0=A0 /* stop and clear MI, MIPI, and ISP interrupts */ > > > > +=A0=A0=A0 writel(0, base + CIF_MIPI_IMSC); > > > > +=A0=A0=A0 writel(~0, base + CIF_MIPI_ICR); > > > > + > > > > +=A0=A0=A0 writel(0, base + CIF_ISP_IMSC); > > > > +=A0=A0=A0 writel(~0, base + CIF_ISP_ICR); > > > > + > > > > +=A0=A0=A0 writel(0, base + CIF_MI_IMSC); > > > > +=A0=A0=A0 writel(~0, base + CIF_MI_ICR); > > > > +=A0=A0=A0 val =3D readl(base + CIF_MIPI_CTRL); > > > > +=A0=A0=A0 writel(val & (~CIF_MIPI_CTRL_OUTPUT_ENA), base + CIF_MIP= I_CTRL); > > > > +=A0=A0=A0 /* stop ISP */ > > > > +=A0=A0=A0 val =3D readl(base + CIF_ISP_CTRL); > > > > +=A0=A0=A0 val &=3D ~(CIF_ISP_CTRL_ISP_INFORM_ENABLE | CIF_ISP_CTRL= _ISP_ENABLE); > > > > +=A0=A0=A0 writel(val, base + CIF_ISP_CTRL); > > > > + > > > > +=A0=A0=A0 val =3D readl(base + CIF_ISP_CTRL); > > > > +=A0=A0=A0 writel(val | CIF_ISP_CTRL_ISP_CFG_UPD, base + CIF_ISP_CT= RL); > > > > + > > > > +=A0=A0=A0 readx_poll_timeout(readl, base + CIF_ISP_RIS, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 val, val & CIF_ISP_OFF,= 20, 100); > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0 "streaming(MP:%d, SP:%d), MI_CTRL:%x, ISP_CT= RL:%x, MIPI_CTRL:%x\n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_SP].streaming, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_MP].streaming, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MI_CTRL), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_ISP_CTRL), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MIPI_CTRL)); > > > > + > > > > +=A0=A0=A0 writel(CIF_IRCL_MIPI_SW_RST | CIF_IRCL_ISP_SW_RST, base = + CIF_IRCL); > > > > +=A0=A0=A0 writel(0x0, base + CIF_IRCL); > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +/* Mess register operations to start isp */ > > > > +static int rkisp1_isp_start(struct rkisp1_device *dev) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_sensor *sensor =3D dev->active_sensor; > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 u32 val; > > > > + > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "SP streaming =3D %d, MP streaming =3D %d= \n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_SP].streaming, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_MP].streaming); > > > > + > > > > +=A0=A0=A0 /* Activate MIPI */ > > > > +=A0=A0=A0 if (sensor->mbus.type =3D=3D V4L2_MBUS_CSI2_DPHY) { > > > > +=A0=A0=A0=A0=A0=A0=A0 val =3D readl(base + CIF_MIPI_CTRL); > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(val | CIF_MIPI_CTRL_OUTPUT_ENA, base = + CIF_MIPI_CTRL); > > > > +=A0=A0=A0 } > > > > +=A0=A0=A0 /* Activate ISP */ > > > > +=A0=A0=A0 val =3D readl(base + CIF_ISP_CTRL); > > > > +=A0=A0=A0 val |=3D CIF_ISP_CTRL_ISP_CFG_UPD | CIF_ISP_CTRL_ISP_ENA= BLE | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_CTRL_ISP_INFORM_ENABLE; > > > > +=A0=A0=A0 writel(val, base + CIF_ISP_CTRL); > > > > + > > > > +=A0=A0=A0 /* XXX: Is the 1000us too long? > > > > +=A0=A0=A0=A0 * CIF spec says to wait for sufficient time after ena= bling > > > > +=A0=A0=A0=A0 * the MIPI interface and before starting the sensor o= utput. > > > > +=A0=A0=A0=A0 */ > > > > +=A0=A0=A0 usleep_range(1000, 1200); > > > > + > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "SP streaming =3D %d, MP streaming =3D %d= MI_CTRL 0x%08x\n" > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "=A0 ISP_CTRL 0x%08x MIPI_CTRL 0x%08x\n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_SP].streaming, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 dev->stream[RKISP1_STREAM_MP].streaming, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MI_CTRL), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_ISP_CTRL), > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 readl(base + CIF_MIPI_CTRL)); > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static void rkisp1_config_clk(struct rkisp1_device *dev) > > > > +{ > > > > +=A0=A0=A0 u32 val =3D CIF_ICCL_ISP_CLK | CIF_ICCL_CP_CLK | CIF_ICC= L_MRSZ_CLK | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ICCL_SRSZ_CLK | CIF_ICCL_JPEG_CLK = | CIF_ICCL_MI_CLK | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ICCL_IE_CLK | CIF_ICCL_MIPI_CLK | = CIF_ICCL_DCROP_CLK; > > > > + > > > > +=A0=A0=A0 writel(val, dev->base_addr + CIF_ICCL); > > > > +} > > > > + > > > > +/***************************** isp sub-devs **********************= *********/ > > > > + > > > > +static const struct ispsd_in_fmt rkisp1_isp_input_formats[] =3D { > > > > +=A0=A0=A0 { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SBGGR1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_BGGR, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 10, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SRGGB1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_RGGB, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 10, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGBRG1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_GBRG, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 10, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGRBG1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_GRBG, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 10, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SRGGB1= 2_1X12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_RGGB, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 12, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SBGGR1= 2_1X12, > > > = > > > Is there a reason why the 10-bit Bayer format start with BGGR while t= he > > > 12-bit formats start with RGGB ? Not a big deal, just OCD kicking in = :-) > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_BGGR, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 12, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGBRG1= 2_1X12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_GBRG, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 12, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGRBG1= 2_1X12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_GRBG, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 12, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SRGGB8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_RGGB, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 8, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SBGGR8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_BGGR, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 8, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGBRG8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_GBRG, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 8, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGRBG8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_RAW8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bayer_pat=A0=A0=A0 =3D RAW_GRBG, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 8, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_YUYV8_= 1X16, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_YUV, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_YUV422_8b, > > > > +=A0=A0=A0=A0=A0=A0=A0 .yuv_seq=A0=A0=A0 =3D CIF_ISP_ACQ_PROP_YCBYC= R, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 16, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_YVYU8_= 1X16, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_YUV, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_YUV422_8b, > > > > +=A0=A0=A0=A0=A0=A0=A0 .yuv_seq=A0=A0=A0 =3D CIF_ISP_ACQ_PROP_YCRYC= B, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 16, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_UYVY8_= 1X16, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_YUV, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_YUV422_8b, > > > > +=A0=A0=A0=A0=A0=A0=A0 .yuv_seq=A0=A0=A0 =3D CIF_ISP_ACQ_PROP_CBYCR= Y, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 16, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_VYUY8_= 1X16, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_YUV, > > > > +=A0=A0=A0=A0=A0=A0=A0 .mipi_dt=A0=A0=A0 =3D CIF_CSI2_DT_YUV422_8b, > > > > +=A0=A0=A0=A0=A0=A0=A0 .yuv_seq=A0=A0=A0 =3D CIF_ISP_ACQ_PROP_CRYCB= Y, > > > > +=A0=A0=A0=A0=A0=A0=A0 .bus_width=A0=A0=A0 =3D 16, > > > > +=A0=A0=A0 }, > > > > +}; > > > > + > > > > +static const struct ispsd_out_fmt rkisp1_isp_output_formats[] =3D { > > > > +=A0=A0=A0 { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_YUYV8_= 2X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_YUV, > > > > +=A0=A0=A0 }, { > > > = > > > 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 comple= x. > > > 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. > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SRGGB1= 2_1X12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SBGGR1= 2_1X12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGBRG1= 2_1X12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGRBG1= 2_1X12, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SRGGB1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SBGGR1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGBRG1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGRBG1= 0_1X10, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SRGGB8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SBGGR8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGBRG8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, { > > > > +=A0=A0=A0=A0=A0=A0=A0 .mbus_code=A0=A0=A0 =3D MEDIA_BUS_FMT_SGRBG8= _1X8, > > > > +=A0=A0=A0=A0=A0=A0=A0 .fmt_type=A0=A0=A0 =3D FMT_BAYER, > > > > +=A0=A0=A0 }, > > > > +}; > > > > + > > > > +static const struct ispsd_in_fmt *find_in_fmt(u32 mbus_code) > > > > +{ > > > > +=A0=A0=A0 unsigned int i, array_size =3D ARRAY_SIZE(rkisp1_isp_inp= ut_formats); > > > > +=A0=A0=A0 const struct ispsd_in_fmt *fmt; > > > = > > > You can move this variable inside the loop. > > > = > > > > + > > > > +=A0=A0=A0 for (i =3D 0; i < array_size; i++) { > > > = > > > I would remove the array_size local variable, it doesn't improve > > > readability. Same for the next function. > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt =3D &rkisp1_isp_input_formats[i]; > > > > +=A0=A0=A0=A0=A0=A0=A0 if (fmt->mbus_code =3D=3D mbus_code) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return fmt; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 return NULL; > > > > +} > > > > + > > > > +static const struct ispsd_out_fmt *find_out_fmt(u32 mbus_code) > > > > +{ > > > > +=A0=A0=A0 unsigned int i, array_size =3D ARRAY_SIZE(rkisp1_isp_out= put_formats); > > > > +=A0=A0=A0 const struct ispsd_out_fmt *fmt; > > > > + > > > > +=A0=A0=A0 for (i =3D 0; i < array_size; i++) { > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt =3D &rkisp1_isp_output_formats[i]; > > > > +=A0=A0=A0=A0=A0=A0=A0 if (fmt->mbus_code =3D=3D mbus_code) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return fmt; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 return NULL; > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_enum_mbus_code(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v= 4l2_subdev_pad_config *cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v= 4l2_subdev_mbus_code_enum *code) > > > > +{ > > > > +=A0=A0=A0 unsigned int i =3D code->index; > > > > + > > > > +=A0=A0=A0 if ((code->pad !=3D RKISP1_ISP_PAD_SINK) && > > > > +=A0=A0=A0=A0=A0=A0=A0 (code->pad !=3D RKISP1_ISP_PAD_SOURCE_PATH))= { > > > > +=A0=A0=A0=A0=A0=A0=A0 if (i > 0) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0=A0=A0=A0=A0 code->code =3D MEDIA_BUS_FMT_FIXED; > > > > +=A0=A0=A0=A0=A0=A0=A0 return 0; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 if (code->pad =3D=3D RKISP1_ISP_PAD_SINK) { > > > > +=A0=A0=A0=A0=A0=A0=A0 if (i >=3D ARRAY_SIZE(rkisp1_isp_input_forma= ts)) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0=A0=A0=A0=A0 code->code =3D rkisp1_isp_input_formats[i].m= bus_code; > > > > +=A0=A0=A0 } else { > > > > +=A0=A0=A0=A0=A0=A0=A0 if (i >=3D ARRAY_SIZE(rkisp1_isp_output_form= ats)) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0=A0=A0=A0=A0 code->code =3D rkisp1_isp_output_formats[i].= mbus_code; > > > > +=A0=A0=A0 } > > > = > > > On the other hand, merging the two tables above into one, you could > > > merge the two branches here and only consider formats indicated as va= lid > > > for to the pad you want. Maybe the code would be cleaner in the end. > > > = > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_init_config(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struc= t v4l2_subdev_pad_config *cfg) > > > > +{ > > > > +=A0=A0=A0 struct v4l2_rect *mf_in_crop, *mf_out_crop; > > > > +=A0=A0=A0 struct v4l2_mbus_framefmt *mf_in, *mf_out; > > > > + > > > > +=A0=A0=A0 mf_in =3D v4l2_subdev_get_try_format(sd, cfg, RKISP1_ISP= _PAD_SINK); > > > > +=A0=A0=A0 mf_in->width =3D RKISP1_DEFAULT_WIDTH; > > > > +=A0=A0=A0 mf_in->height =3D RKISP1_DEFAULT_HEIGHT; > > > > +=A0=A0=A0 mf_in->field =3D V4L2_FIELD_NONE; > > > > +=A0=A0=A0 mf_in->code =3D rkisp1_isp_input_formats[0].mbus_code; > > > > + > > > > +=A0=A0=A0 mf_in_crop =3D v4l2_subdev_get_try_crop(sd, cfg, RKISP1_= ISP_PAD_SINK); > > > > +=A0=A0=A0 mf_in_crop->width =3D RKISP1_DEFAULT_WIDTH; > > > > +=A0=A0=A0 mf_in_crop->height =3D RKISP1_DEFAULT_HEIGHT; > > > > +=A0=A0=A0 mf_in_crop->left =3D 0; > > > > +=A0=A0=A0 mf_in_crop->top =3D 0; > > > > + > > > > +=A0=A0=A0 mf_out =3D v4l2_subdev_get_try_format(sd, cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 RKISP1_ISP_PAD_SOURCE_PATH); > > > > +=A0=A0=A0 *mf_out =3D *mf_in; > > > > +=A0=A0=A0 mf_out->code =3D rkisp1_isp_output_formats[0].mbus_code; > > > > +=A0=A0=A0 mf_out->quantization =3D V4L2_QUANTIZATION_FULL_RANGE; > > > > + > > > > +=A0=A0=A0 mf_out_crop =3D v4l2_subdev_get_try_crop(sd, cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0 RKISP1_ISP_PAD_SOURCE_PATH); > > > > +=A0=A0=A0 *mf_out_crop =3D *mf_in_crop; > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_get_fmt(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_subde= v_pad_config *cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_subde= v_format *fmt) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_isp_subdev *isp_sd =3D sd_to_isp_sd(sd); > > > > +=A0=A0=A0 struct v4l2_mbus_framefmt *mf =3D &fmt->format; > > > > + > > > > +=A0=A0=A0 if ((fmt->pad !=3D RKISP1_ISP_PAD_SINK) && > > > > +=A0=A0=A0=A0=A0=A0=A0 (fmt->pad !=3D RKISP1_ISP_PAD_SOURCE_PATH)) { > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->format.code =3D MEDIA_BUS_FMT_FIXED; > > > > +=A0=A0=A0=A0=A0=A0=A0 /* > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 * NOTE: setting a format here doesn't mak= e much sense > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 * but v4l2-compliance complains > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 */ > > > = > > > 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. > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->format.width =3D RKISP1_DEFAULT_WIDTH; > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->format.height =3D RKISP1_DEFAULT_HEIGHT; > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->format.field =3D V4L2_FIELD_NONE; > > > > +=A0=A0=A0=A0=A0=A0=A0 return 0; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 if (fmt->which =3D=3D V4L2_SUBDEV_FORMAT_TRY) { > > > > +=A0=A0=A0=A0=A0=A0=A0 mf =3D v4l2_subdev_get_try_format(sd, cfg, f= mt->pad); > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->format =3D *mf; > > > > +=A0=A0=A0=A0=A0=A0=A0 return 0; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 if (fmt->pad =3D=3D RKISP1_ISP_PAD_SINK) { > > > > +=A0=A0=A0=A0=A0=A0=A0 *mf =3D isp_sd->in_frm; > > > > +=A0=A0=A0 } else if (fmt->pad =3D=3D RKISP1_ISP_PAD_SOURCE_PATH) { > > > > +=A0=A0=A0=A0=A0=A0=A0 /* format of source pad */ > > > > +=A0=A0=A0=A0=A0=A0=A0 *mf =3D isp_sd->in_frm; > > > > +=A0=A0=A0=A0=A0=A0=A0 mf->code =3D isp_sd->out_fmt.mbus_code; > > > > +=A0=A0=A0=A0=A0=A0=A0 /* window size of source pad */ > > > > +=A0=A0=A0=A0=A0=A0=A0 mf->width =3D isp_sd->out_crop.width; > > > > +=A0=A0=A0=A0=A0=A0=A0 mf->height =3D isp_sd->out_crop.height; > > > > +=A0=A0=A0=A0=A0=A0=A0 mf->quantization =3D isp_sd->quantization; > > > > +=A0=A0=A0 } > > > > +=A0=A0=A0 mf->field =3D V4L2_FIELD_NONE; > > > = > > > This can be simplified, please read through, or jump to the review of > > > rkisp1_isp_subdev at the end. > > > = > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static void rkisp1_isp_sd_try_fmt(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 unsigned int p= ad, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_mb= us_framefmt *fmt) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_device *isp_dev =3D sd_to_isp_dev(sd); > > > > +=A0=A0=A0 struct rkisp1_isp_subdev *isp_sd =3D &isp_dev->isp_sdev; > > > > +=A0=A0=A0 const struct ispsd_out_fmt *out_fmt; > > > > +=A0=A0=A0 const struct ispsd_in_fmt *in_fmt; > > > > + > > > > +=A0=A0=A0 switch (pad) { > > > > +=A0=A0=A0 case RKISP1_ISP_PAD_SINK: > > > > +=A0=A0=A0=A0=A0=A0=A0 in_fmt =3D find_in_fmt(fmt->code); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (in_fmt) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 fmt->code =3D in_fmt->mbus_code; > > > > +=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 fmt->code =3D MEDIA_BUS_FMT_SRGG= B10_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]), w= ith > > > 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. > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->width=A0 =3D clamp_t(u32, fmt->width, C= IF_ISP_INPUT_W_MIN, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CI= F_ISP_INPUT_W_MAX); > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->height =3D clamp_t(u32, fmt->height, CI= F_ISP_INPUT_H_MIN, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CI= F_ISP_INPUT_H_MAX); > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 case RKISP1_ISP_PAD_SOURCE_PATH: > > > > +=A0=A0=A0=A0=A0=A0=A0 out_fmt =3D find_out_fmt(fmt->code); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (out_fmt) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 fmt->code =3D out_fmt->mbus_code; > > > > +=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 fmt->code =3D rkisp1_isp_output_= formats[0].mbus_code; > > > > +=A0=A0=A0=A0=A0=A0=A0 /* window size is set in s_selection */ > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->width=A0 =3D isp_sd->out_crop.width; > > > > +=A0=A0=A0=A0=A0=A0=A0 fmt->height =3D 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 :-) > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 /* full range by default */ > > > > +=A0=A0=A0=A0=A0=A0=A0 if (!fmt->quantization) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 fmt->quantization =3D V4L2_QUANT= IZATION_FULL_RANGE; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 fmt->field =3D V4L2_FIELD_NONE; > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_set_fmt(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_subde= v_pad_config *cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_subde= v_format *fmt) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_device *isp_dev =3D sd_to_isp_dev(sd); > > > > +=A0=A0=A0 struct rkisp1_isp_subdev *isp_sd =3D &isp_dev->isp_sdev; > > > > +=A0=A0=A0 struct v4l2_mbus_framefmt *mf =3D &fmt->format; > > > > + > > > > +=A0=A0=A0 if ((fmt->pad !=3D RKISP1_ISP_PAD_SINK) && > > > > +=A0=A0=A0=A0=A0=A0=A0 (fmt->pad !=3D RKISP1_ISP_PAD_SOURCE_PATH)) > > > > +=A0=A0=A0=A0=A0=A0=A0 return rkisp1_isp_sd_get_fmt(sd, cfg, fmt); > > > > + > > > > +=A0=A0=A0 rkisp1_isp_sd_try_fmt(sd, fmt->pad, mf); > > > > + > > > > +=A0=A0=A0 if (fmt->which =3D=3D V4L2_SUBDEV_FORMAT_TRY) { > > > > +=A0=A0=A0=A0=A0=A0=A0 struct v4l2_mbus_framefmt *try_mf; > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 try_mf =3D v4l2_subdev_get_try_format(sd, cf= g, fmt->pad); > > > > +=A0=A0=A0=A0=A0=A0=A0 *try_mf =3D *mf; > > > > +=A0=A0=A0=A0=A0=A0=A0 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). > > > = > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 if (fmt->pad =3D=3D RKISP1_ISP_PAD_SINK) { > > > > +=A0=A0=A0=A0=A0=A0=A0 const struct ispsd_in_fmt *in_fmt; > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 in_fmt =3D find_in_fmt(mf->code); > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_sd->in_fmt =3D *in_fmt; > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_sd->in_frm =3D *mf; > > > > +=A0=A0=A0 } else if (fmt->pad =3D=3D RKISP1_ISP_PAD_SOURCE_PATH) { > > > > +=A0=A0=A0=A0=A0=A0=A0 const struct ispsd_out_fmt *out_fmt; > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 /* Ignore width/height */ > > > > +=A0=A0=A0=A0=A0=A0=A0 out_fmt =3D find_out_fmt(mf->code); > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_sd->out_fmt =3D *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. > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 /* > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 * It is quantization for output, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 * isp use bt601 limit-range in internal > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 */ > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_sd->quantization =3D mf->quantization; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static void rkisp1_isp_sd_try_crop(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_su= bdev_pad_config *cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_su= bdev_selection *sel) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_isp_subdev *isp_sd =3D sd_to_isp_sd(sd); > > > > +=A0=A0=A0 struct v4l2_mbus_framefmt in_frm =3D isp_sd->in_frm; > > > > +=A0=A0=A0 struct v4l2_rect in_crop =3D isp_sd->in_crop; > > > > +=A0=A0=A0 struct v4l2_rect *input =3D &sel->r; > > > > + > > > > +=A0=A0=A0 if (sel->which =3D=3D V4L2_SUBDEV_FORMAT_TRY) { > > > > +=A0=A0=A0=A0=A0=A0=A0 in_frm =3D *v4l2_subdev_get_try_format(sd, c= fg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 RKISP1_ISP_PAD_SINK); > > > > +=A0=A0=A0=A0=A0=A0=A0 in_crop =3D *v4l2_subdev_get_try_crop(sd, cf= g, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 RKISP1_ISP_PAD_SINK); > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 input->left =3D ALIGN(input->left, 2); > > > > +=A0=A0=A0 input->width =3D ALIGN(input->width, 2); > > > > + > > > > +=A0=A0=A0 if (sel->pad =3D=3D RKISP1_ISP_PAD_SINK) { > > > > +=A0=A0=A0=A0=A0=A0=A0 input->left =3D clamp_t(u32, input->left, 0,= in_frm.width); > > > > +=A0=A0=A0=A0=A0=A0=A0 input->top =3D clamp_t(u32, input->top, 0, i= n_frm.height); > > > > +=A0=A0=A0=A0=A0=A0=A0 input->width =3D clamp_t(u32, input->width, = CIF_ISP_INPUT_W_MIN, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= in_frm.width - input->left); > > > > +=A0=A0=A0=A0=A0=A0=A0 input->height =3D clamp_t(u32, input->height, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_= INPUT_H_MIN, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 in_frm.h= eight - input->top); > > > > +=A0=A0=A0 } else if (sel->pad =3D=3D RKISP1_ISP_PAD_SOURCE_PATH) { > > > > +=A0=A0=A0=A0=A0=A0=A0 input->left =3D clamp_t(u32, input->left, 0,= in_crop.width); > > > > +=A0=A0=A0=A0=A0=A0=A0 input->top =3D clamp_t(u32, input->top, 0, i= n_crop.height); > > > > +=A0=A0=A0=A0=A0=A0=A0 input->width =3D clamp_t(u32, input->width, = CIF_ISP_OUTPUT_W_MIN, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= in_crop.width - input->left); > > > > +=A0=A0=A0=A0=A0=A0=A0 input->height =3D clamp_t(u32, input->height, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_= OUTPUT_H_MIN, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 in_crop.= height - input->top); > > > > +=A0=A0=A0 } > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_get_selection(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= struct v4l2_subdev_pad_config *cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= struct v4l2_subdev_selection *sel) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_isp_subdev *isp_sd =3D sd_to_isp_sd(sd); > > > > +=A0=A0=A0 struct v4l2_mbus_framefmt *frm; > > > > +=A0=A0=A0 struct v4l2_rect *rect; > > > > + > > > > +=A0=A0=A0 if (sel->pad !=3D RKISP1_ISP_PAD_SOURCE_PATH && > > > > +=A0=A0=A0=A0=A0=A0=A0 sel->pad !=3D RKISP1_ISP_PAD_SINK) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > + > > > > +=A0=A0=A0 switch (sel->target) { > > > > +=A0=A0=A0 case V4L2_SEL_TGT_CROP_BOUNDS: > > > > +=A0=A0=A0=A0=A0=A0=A0 if (sel->pad =3D=3D RKISP1_ISP_PAD_SINK) { > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (sel->which =3D=3D V4L2_SUBDE= V_FORMAT_TRY) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 frm =3D v4l2_subdev_= get_try_format(sd, cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sel->pad); > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 frm =3D &isp_sd->in_= frm; > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sel->r.height =3D frm->height; > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sel->r.width =3D frm->width; > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sel->r.left =3D 0; > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sel->r.top =3D 0; > > > > +=A0=A0=A0=A0=A0=A0=A0 } else { > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (sel->which =3D=3D V4L2_SUBDE= V_FORMAT_TRY) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rect =3D v4l2_subdev= _get_try_crop(sd, cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0 RKISP1_ISP_PAD_SINK); > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rect =3D &isp_sd->in= _crop; > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 sel->r =3D *rect; > > > > +=A0=A0=A0=A0=A0=A0=A0 } > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 case V4L2_SEL_TGT_CROP: > > > > +=A0=A0=A0=A0=A0=A0=A0 if (sel->which =3D=3D V4L2_SUBDEV_FORMAT_TRY) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rect =3D v4l2_subdev_get_try_cro= p(sd, cfg, sel->pad); > > > > +=A0=A0=A0=A0=A0=A0=A0 else if (sel->pad =3D=3D RKISP1_ISP_PAD_SINK) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rect =3D &isp_sd->in_crop; > > > > +=A0=A0=A0=A0=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rect =3D &isp_sd->out_crop; > > > > +=A0=A0=A0=A0=A0=A0=A0 sel->r =3D *rect; > > > > +=A0=A0=A0=A0=A0=A0=A0 break; > > > > +=A0=A0=A0 default: > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_set_selection(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= struct v4l2_subdev_pad_config *cfg, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= struct v4l2_subdev_selection *sel) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_isp_subdev *isp_sd =3D sd_to_isp_sd(sd); > > > > +=A0=A0=A0 struct rkisp1_device *dev =3D sd_to_isp_dev(sd); > > > > + > > > > +=A0=A0=A0 if (sel->pad !=3D RKISP1_ISP_PAD_SOURCE_PATH && > > > > +=A0=A0=A0=A0=A0=A0=A0 sel->pad !=3D RKISP1_ISP_PAD_SINK) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0 if (sel->target !=3D V4L2_SEL_TGT_CROP) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > + > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 "%s: pad: %d sel(%d,%d)/%dx%d\n", __func_= _, sel->pad, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 sel->r.left, sel->r.top, sel->r.width, se= l->r.height); > > > > +=A0=A0=A0 rkisp1_isp_sd_try_crop(sd, cfg, sel); > > > > + > > > > +=A0=A0=A0 if (sel->which =3D=3D V4L2_SUBDEV_FORMAT_TRY) { > > > > +=A0=A0=A0=A0=A0=A0=A0 struct v4l2_rect *try_sel =3D > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 v4l2_subdev_get_try_crop(sd, cfg= , sel->pad); > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 *try_sel =3D sel->r; > > > > +=A0=A0=A0=A0=A0=A0=A0 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 afr= aid > > > this means that we'll need separate subdevs for the resizers :-S > > > = > > > [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.ht= ml#order-of-configuration-and-format-propagation > > > = > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 if (sel->pad =3D=3D RKISP1_ISP_PAD_SINK) > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_sd->in_crop =3D sel->r; > > > > +=A0=A0=A0 else > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_sd->out_crop =3D sel->r; > > > > + > > > > +=A0=A0=A0 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, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct r= kisp1_sensor *sensor) > > > > +{ > > > > +=A0=A0=A0 union phy_configure_opts opts =3D { 0 }; > > > > +=A0=A0=A0 struct phy_configure_opts_mipi_dphy *cfg =3D &opts.mipi_= dphy; > > > > +=A0=A0=A0 struct v4l2_ctrl *pixel_rate; > > > > +=A0=A0=A0 s64 pixel_clock; > > > > + > > > > +=A0=A0=A0 pixel_rate =3D v4l2_ctrl_find(sensor->sd->ctrl_handler, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 V4L2_CID= _PIXEL_RATE); > > > > +=A0=A0=A0 if (!pixel_rate) { > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_warn(sensor->sd, "No pixel rate control= in subdev\n"); > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EPIPE; > > > > +=A0=A0=A0 } > > > = > > > Would it make sense to retrieve (and cache) the control pointer (not = its > > > value of course) at probe time already ? > > > = > > > > + > > > > +=A0=A0=A0 pixel_clock =3D v4l2_ctrl_g_ctrl_int64(pixel_rate); > > > > +=A0=A0=A0 if (!pixel_clock) { > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_err(sensor->sd, "Invalid pixel rate val= ue\n"); > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 phy_mipi_dphy_get_default_config(pixel_clock, isp_sd->in= _fmt.bus_width, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 senso= r->lanes, cfg); > > > > +=A0=A0=A0 phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY); > > > > +=A0=A0=A0 phy_configure(sensor->dphy, &opts); > > > > +=A0=A0=A0 phy_power_on(sensor->dphy); > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static void mipi_csi2_s_stream_stop(struct rkisp1_sensor *sensor) > > > > +{ > > > > +=A0=A0=A0 phy_power_off(sensor->dphy); > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_s_stream(struct v4l2_subdev *sd, int on) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_device *isp_dev =3D sd_to_isp_dev(sd); > > > > +=A0=A0=A0 struct v4l2_subdev *sensor_sd; > > > > +=A0=A0=A0 int ret =3D 0; > > > > + > > > > +=A0=A0=A0 if (!on) { > > > > +=A0=A0=A0=A0=A0=A0=A0 ret =3D rkisp1_isp_stop(isp_dev); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > +=A0=A0=A0=A0=A0=A0=A0 mipi_csi2_s_stream_stop(isp_dev->active_sens= or); > > > > +=A0=A0=A0=A0=A0=A0=A0 return 0; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 sensor_sd =3D get_remote_sensor(sd); > > > > +=A0=A0=A0 if (!sensor_sd) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -ENODEV; > > > > + > > > > +=A0=A0=A0 isp_dev->active_sensor =3D sd_to_sensor(isp_dev, sensor_= sd); > > > > +=A0=A0=A0 if (!isp_dev->active_sensor) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -ENODEV; > > > > + > > > > +=A0=A0=A0 atomic_set(&isp_dev->isp_sdev.frm_sync_seq, 0); > > > > +=A0=A0=A0 ret =3D rkisp1_config_cif(isp_dev); > > > > +=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > + > > > > +=A0=A0=A0 /* TODO: support other interfaces */ > > > > +=A0=A0=A0 if (isp_dev->active_sensor->mbus.type !=3D V4L2_MBUS_CSI= 2_DPHY) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > + > > > > +=A0=A0=A0 ret =3D mipi_csi2_s_stream_start(&isp_dev->isp_sdev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= isp_dev->active_sensor); > > > > +=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > + > > > > +=A0=A0=A0 ret =3D rkisp1_isp_start(isp_dev); > > > > +=A0=A0=A0 if (ret) > > > > +=A0=A0=A0=A0=A0=A0=A0 mipi_csi2_s_stream_stop(isp_dev->active_sens= or); > > > > + > > > > +=A0=A0=A0 return ret; > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_device *isp_dev =3D sd_to_isp_dev(sd); > > > > +=A0=A0=A0 int ret; > > > > + > > > > +=A0=A0=A0 v4l2_dbg(1, rkisp1_debug, &isp_dev->v4l2_dev, "s_power: = %d\n", on); > > > > + > > > > +=A0=A0=A0 if (on) { > > > > +=A0=A0=A0=A0=A0=A0=A0 ret =3D pm_runtime_get_sync(isp_dev->dev); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 rkisp1_config_clk(isp_dev); > > > > +=A0=A0=A0 } else { > > > > +=A0=A0=A0=A0=A0=A0=A0 ret =3D pm_runtime_put(isp_dev->dev); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static int rkisp1_subdev_link_validate(struct media_link *link) > > > > +{ > > > > +=A0=A0=A0 if (link->source->index =3D=3D RKISP1_ISP_PAD_SINK_PARAM= S) > > > > +=A0=A0=A0=A0=A0=A0=A0 return 0; > > > > + > > > > +=A0=A0=A0 return v4l2_subdev_link_validate(link); > > > > +} > > > > + > > > > +static int rkisp1_subdev_fmt_link_validate(struct v4l2_subdev *sd, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct m= edia_link *link, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v= 4l2_subdev_format *source_fmt, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v= 4l2_subdev_format *sink_fmt) > > > > +{ > > > > +=A0=A0=A0 if (source_fmt->format.code !=3D sink_fmt->format.code) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > + > > > > +=A0=A0=A0 /* Crop is available */ > > > > +=A0=A0=A0 if (source_fmt->format.width < sink_fmt->format.width || > > > > +=A0=A0=A0=A0=A0=A0=A0 source_fmt->format.height < sink_fmt->format= .height) > > > > +=A0=A0=A0=A0=A0=A0=A0 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. > > > = > > > > + > > > > +=A0=A0=A0 return 0; > > > > +} > > > > + > > > > +static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *i= sp) > > > > +{ > > > > +=A0=A0=A0 struct v4l2_event event =3D { > > > > +=A0=A0=A0=A0=A0=A0=A0 .type =3D V4L2_EVENT_FRAME_SYNC, > > > > +=A0=A0=A0=A0=A0=A0=A0 .u.frame_sync.frame_sequence =3D > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 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 possi= ble > > > 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 f= rom > > > 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': > > = > > =A0=A0=A0=A0/* > > =A0=A0=A0=A0=A0=A0=A0=A0 * If there's no next buffer assigned, queue t= his buffer directly > > =A0=A0=A0=A0=A0=A0=A0=A0 * as the next buffer, and update the memory i= nterface. > > =A0=A0=A0=A0=A0=A0=A0=A0 */ > > =A0=A0=A0=A0=A0=A0=A0 if (cap->is_streaming && !cap->buf.next && > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 atomic_read(&cap->rkisp1->isp.frame_= sequence) =3D=3D -1) { > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 cap->buf.next =3D ispbuf; > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rkisp1_set_next_buf(cap); > > =A0=A0=A0=A0=A0=A0=A0 } else { > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 list_add_tail(&ispbuf->q= ueue, &cap->buf.queue); > > =A0=A0=A0=A0=A0=A0=A0 } > > 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->is= p.frame_sequence) =3D=3D -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. > > = > > = > > = > > = > > = > > > = > > > > +=A0=A0=A0 }; > > > > +=A0=A0=A0 v4l2_event_queue(isp->sd.devnode, &event); > > > > +} > > > > + > > > > +static int rkisp1_isp_sd_subs_evt(struct v4l2_subdev *sd, struct v= 4l2_fh *fh, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2_ev= ent_subscription *sub) > > > > +{ > > > > +=A0=A0=A0 if (sub->type !=3D V4L2_EVENT_FRAME_SYNC) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > + > > > > +=A0=A0=A0 /* 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. > > > = > > > > +=A0=A0=A0 if (sub->id !=3D 0) > > > > +=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > > > > + > > > > +=A0=A0=A0 return v4l2_event_subscribe(fh, sub, 0, NULL); > > > > +} > > > > + > > > > +static const struct v4l2_subdev_pad_ops rkisp1_isp_sd_pad_ops =3D { > > > > +=A0=A0=A0 .enum_mbus_code =3D rkisp1_isp_sd_enum_mbus_code, > > > > +=A0=A0=A0 .get_selection =3D rkisp1_isp_sd_get_selection, > > > > +=A0=A0=A0 .set_selection =3D rkisp1_isp_sd_set_selection, > > > > +=A0=A0=A0 .init_cfg =3D rkisp1_isp_sd_init_config, > > > > +=A0=A0=A0 .get_fmt =3D rkisp1_isp_sd_get_fmt, > > > > +=A0=A0=A0 .set_fmt =3D rkisp1_isp_sd_set_fmt, > > > > +=A0=A0=A0 .link_validate =3D rkisp1_subdev_fmt_link_validate, > > > > +}; > > > > + > > > > +static const struct media_entity_operations rkisp1_isp_sd_media_op= s =3D { > > > > +=A0=A0=A0 .link_validate =3D rkisp1_subdev_link_validate, > > > > +}; > > > > + > > > > +static const struct v4l2_subdev_video_ops rkisp1_isp_sd_video_ops = =3D { > > > > +=A0=A0=A0 .s_stream =3D rkisp1_isp_sd_s_stream, > > > > +}; > > > > + > > > > +static const struct v4l2_subdev_core_ops rkisp1_isp_core_ops =3D { > > > > +=A0=A0=A0 .subscribe_event =3D rkisp1_isp_sd_subs_evt, > > > > +=A0=A0=A0 .unsubscribe_event =3D v4l2_event_subdev_unsubscribe, > > > > +=A0=A0=A0 .s_power =3D rkisp1_isp_sd_s_power, > > > > +}; > > > > + > > > > +static struct v4l2_subdev_ops rkisp1_isp_sd_ops =3D { > > > = > > > static const > > > = > > > > +=A0=A0=A0 .core =3D &rkisp1_isp_core_ops, > > > > +=A0=A0=A0 .video =3D &rkisp1_isp_sd_video_ops, > > > > +=A0=A0=A0 .pad =3D &rkisp1_isp_sd_pad_ops, > > > > +}; > > > > + > > > > +static void rkisp1_isp_sd_init_default_fmt(struct rkisp1_isp_subde= v *isp_sd) > > > > +{ > > > > +=A0=A0=A0 struct v4l2_mbus_framefmt *in_frm =3D &isp_sd->in_frm; > > > > +=A0=A0=A0 struct v4l2_rect *in_crop =3D &isp_sd->in_crop; > > > > +=A0=A0=A0 struct v4l2_rect *out_crop =3D &isp_sd->out_crop; > > > > +=A0=A0=A0 struct ispsd_in_fmt *in_fmt =3D &isp_sd->in_fmt; > > > > +=A0=A0=A0 struct ispsd_out_fmt *out_fmt =3D &isp_sd->out_fmt; > > > > + > > > > +=A0=A0=A0 *in_fmt =3D rkisp1_isp_input_formats[0]; > > > > +=A0=A0=A0 in_frm->width =3D RKISP1_DEFAULT_WIDTH; > > > > +=A0=A0=A0 in_frm->height =3D RKISP1_DEFAULT_HEIGHT; > > > > +=A0=A0=A0 in_frm->code =3D in_fmt->mbus_code; > > > > + > > > > +=A0=A0=A0 in_crop->width =3D in_frm->width; > > > > +=A0=A0=A0 in_crop->height =3D in_frm->height; > > > > +=A0=A0=A0 in_crop->left =3D 0; > > > > +=A0=A0=A0 in_crop->top =3D 0; > > > > + > > > > +=A0=A0=A0 /* propagate to source */ > > > > +=A0=A0=A0 *out_crop =3D *in_crop; > > > > +=A0=A0=A0 *out_fmt =3D rkisp1_isp_output_formats[0]; > > > > +=A0=A0=A0 isp_sd->quantization =3D 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, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct v4l2= _device *v4l2_dev) > > > > +{ > > > > +=A0=A0=A0 struct rkisp1_isp_subdev *isp_sdev =3D &isp_dev->isp_sde= v; > > > > +=A0=A0=A0 struct v4l2_subdev *sd =3D &isp_sdev->sd; > > > > +=A0=A0=A0 int ret; > > > > + > > > > +=A0=A0=A0 v4l2_subdev_init(sd, &rkisp1_isp_sd_ops); > > > > +=A0=A0=A0 sd->flags |=3D V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_= FL_HAS_EVENTS; > > > > +=A0=A0=A0 sd->entity.ops =3D &rkisp1_isp_sd_media_ops; > > > > +=A0=A0=A0 snprintf(sd->name, sizeof(sd->name), "rkisp1-isp-subdev"= ); > > > > + > > > > +=A0=A0=A0 isp_sdev->pads[RKISP1_ISP_PAD_SINK].flags =3D > > > > +=A0=A0=A0=A0=A0=A0=A0 MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNEC= T; > > > > +=A0=A0=A0 isp_sdev->pads[RKISP1_ISP_PAD_SINK_PARAMS].flags =3D MED= IA_PAD_FL_SINK; > > > > +=A0=A0=A0 isp_sdev->pads[RKISP1_ISP_PAD_SOURCE_PATH].flags =3D MED= IA_PAD_FL_SOURCE; > > > > +=A0=A0=A0 isp_sdev->pads[RKISP1_ISP_PAD_SOURCE_STATS].flags =3D ME= DIA_PAD_FL_SOURCE; > > > > +=A0=A0=A0 sd->entity.function =3D MEDIA_ENT_F_PROC_VIDEO_PIXEL_FOR= MATTER; > > > > +=A0=A0=A0 ret =3D media_entity_pads_init(&sd->entity, RKISP1_ISP_P= AD_MAX, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_s= dev->pads); > > > > +=A0=A0=A0 if (ret < 0) > > > > +=A0=A0=A0=A0=A0=A0=A0 return ret; > > > > + > > > > +=A0=A0=A0 sd->owner =3D THIS_MODULE; > > > > +=A0=A0=A0 v4l2_set_subdevdata(sd, isp_dev); > > > > + > > > > +=A0=A0=A0 sd->grp_id =3D GRP_ID_ISP; > > > = > > > I think you can drop this, as well as all the GRP_ID_* macros, they're > > > not used. > > > = > > > > +=A0=A0=A0 ret =3D v4l2_device_register_subdev(v4l2_dev, sd); > > > > +=A0=A0=A0 if (ret < 0) { > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_err(sd, "Failed to register isp subdev\= n"); > > > > +=A0=A0=A0=A0=A0=A0=A0 goto err_cleanup_media_entity; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 rkisp1_isp_sd_init_default_fmt(isp_sdev); > > > > + > > > > +=A0=A0=A0 return 0; > > > > +err_cleanup_media_entity: > > > > +=A0=A0=A0 media_entity_cleanup(&sd->entity); > > > > +=A0=A0=A0 return ret; > > > > +} > > > > + > > > > +void rkisp1_unregister_isp_subdev(struct rkisp1_device *isp_dev) > > > > +{ > > > > +=A0=A0=A0 struct v4l2_subdev *sd =3D &isp_dev->isp_sdev.sd; > > > > + > > > > +=A0=A0=A0 v4l2_device_unregister_subdev(sd); > > > > +=A0=A0=A0 media_entity_cleanup(&sd->entity); > > > > +} > > > > + > > > > +/****************=A0 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. Sho= uld > > > it also be renamed to status ? Same for rkisp1_isp_isr() below. > > > = > > > > +{ > > > > +=A0=A0=A0 struct v4l2_device *v4l2_dev =3D &dev->v4l2_dev; > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 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. > > > = > > > > +=A0=A0=A0 writel(~0, base + CIF_MIPI_ICR); > > > > + > > > > +=A0=A0=A0 /* > > > > +=A0=A0=A0=A0 * Disable DPHY errctrl interrupt, because this dphy > > > > +=A0=A0=A0=A0 * erctrl signal is asserted until the next changes > > > > +=A0=A0=A0=A0 * of line state. This time is may be too long and cpu > > > > +=A0=A0=A0=A0 * is hold in this interrupt. > > > > +=A0=A0=A0=A0 */ > > > > +=A0=A0=A0 if (mis & CIF_MIPI_ERR_CTRL(0x0f)) { > > > > +=A0=A0=A0=A0=A0=A0=A0 val =3D readl(base + CIF_MIPI_IMSC); > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(val & ~CIF_MIPI_ERR_CTRL(0x0f), base = + CIF_MIPI_IMSC); > > > > +=A0=A0=A0=A0=A0=A0=A0 dev->isp_sdev.dphy_errctrl_disabled =3D true; > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 /* > > > > +=A0=A0=A0=A0 * Enable DPHY errctrl interrupt again, if mipi have r= eceive > > > > +=A0=A0=A0=A0 * the whole frame without any error. > > > > +=A0=A0=A0=A0 */ > > > > +=A0=A0=A0 if (mis =3D=3D CIF_MIPI_FRAME_END) { > > > > +=A0=A0=A0=A0=A0=A0=A0 /* > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 * Enable DPHY errctrl interrupt again, if= mipi have receive > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 * the whole frame without any error. > > > > +=A0=A0=A0=A0=A0=A0=A0=A0 */ > > > > +=A0=A0=A0=A0=A0=A0=A0 if (dev->isp_sdev.dphy_errctrl_disabled) { > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 val =3D readl(base + CIF_MIPI_IM= SC); > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 val |=3D CIF_MIPI_ERR_CTRL(0x0f); > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 writel(val, base + CIF_MIPI_IMSC= ); > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 dev->isp_sdev.dphy_errctrl_disab= led =3D false; > > > > +=A0=A0=A0=A0=A0=A0=A0 } > > > > +=A0=A0=A0 } else { > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_warn(v4l2_dev, "MIPI mis error: 0x%08x\= n", mis); > > > > +=A0=A0=A0 } > > > > +} > > > > + > > > > +void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *de= v) > > > > +{ > > > > +=A0=A0=A0 void __iomem *base =3D dev->base_addr; > > > > +=A0=A0=A0 unsigned int isp_mis_tmp =3D 0; > > > = > > > _tmp are never good names :-S > > > = > > > > +=A0=A0=A0 unsigned int isp_err =3D 0; > > > = > > > Neither of these variable need to be initialised to 0. > > > = > > > > + > > > > +=A0=A0=A0 /* start edge of v_sync */ > > > > +=A0=A0=A0 if (isp_mis & CIF_ISP_V_START) { > > > > +=A0=A0=A0=A0=A0=A0=A0 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 t= wo? > > How can this be solved? I wonder if any v-start signal has a later sign= al > > 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? > > = > > > = > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 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 ? > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_mis_tmp =3D readl(base + CIF_ISP_MIS); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (isp_mis_tmp & CIF_ISP_V_START) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 v4l2_err(&dev->v4l2_dev, "isp ic= r v_statr err: 0x%x\n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 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. > > > = > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) { > > > > +=A0=A0=A0=A0=A0=A0=A0 /* Clear pic_size_error */ > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_IS= P_ICR); > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_err =3D readl(base + CIF_ISP_ERR); > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_err(&dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 "CIF_ISP_PIC_SIZE_ERROR (0x%0= 8x)", isp_err); > > > = > > > What does this mean ? > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(isp_err, base + CIF_ISP_ERR_CLR); > > > > +=A0=A0=A0 } else if ((isp_mis & CIF_ISP_DATA_LOSS)) { > > > = > > > Are CIF_ISP_PIC_SIZE_ERROR and CIF_ISP_DATA_LOSS mutually exclusive ? > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 /* Clear data_loss */ > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR= ); > > > > +=A0=A0=A0=A0=A0=A0=A0 v4l2_err(&dev->v4l2_dev, "CIF_ISP_DATA_LOSS\= n"); > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR= ); > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 /* sampled input frame is complete */ > > > > +=A0=A0=A0 if (isp_mis & CIF_ISP_FRAME_IN) { > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(CIF_ISP_FRAME_IN, base + CIF_ISP_ICR); > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_mis_tmp =3D readl(base + CIF_ISP_MIS); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (isp_mis_tmp & CIF_ISP_FRAME_IN) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 v4l2_err(&dev->v4l2_dev, "isp ic= r frame_in err: 0x%x\n", > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 isp_mis_tmp); > > > > +=A0=A0=A0 } > > > > + > > > > +=A0=A0=A0 /* frame was completely put out */ > > > = > > > "put out" ? :-) What's the difference between ISP_FRAME_IN and ISP_FR= AME > > > ? 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. > > = > > = > > > = > > > > +=A0=A0=A0 if (isp_mis & CIF_ISP_FRAME) { > > > > +=A0=A0=A0=A0=A0=A0=A0 u32 isp_ris =3D 0; > > > = > > > No need to initialise this to 0. > > > = > > > > +=A0=A0=A0=A0=A0=A0=A0 /* Clear Frame In (ISP) */ > > > > +=A0=A0=A0=A0=A0=A0=A0 writel(CIF_ISP_FRAME, base + CIF_ISP_ICR); > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_mis_tmp =3D readl(base + CIF_ISP_MIS); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (isp_mis_tmp & CIF_ISP_FRAME) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 v4l2_err(&dev->v4l2_dev, > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 "isp icr frame en= d err: 0x%x\n", isp_mis_tmp); > > > > + > > > > +=A0=A0=A0=A0=A0=A0=A0 isp_ris =3D readl(base + CIF_ISP_RIS); > > > > +=A0=A0=A0=A0=A0=A0=A0 if (isp_ris & (CIF_ISP_AWB_DONE | CIF_ISP_AF= M_FIN | > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 CIF_ISP_EXP= _END | CIF_ISP_HIST_MEASURE_RDY)) > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rkisp1_stats_isr(&dev->stats_vde= v, 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 re= ad. > > I think that if only part of the statistics are ready, we can already s= end 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_s= equence with > > the following explanation: > > = > > =A0=A0=A0=A0/* > > =A0=A0=A0=A0=A0=A0=A0=A0 * Call rkisp1_capture_isr() first to handle t= he frame that > > =A0=A0=A0=A0=A0=A0=A0=A0 * potentially completed using the current fra= me_sequence number before > > =A0=A0=A0=A0=A0=A0=A0=A0 * it is potentially incremented by rkisp1_isp= _isr() in the vertical > > =A0=A0=A0=A0=A0=A0=A0=A0 * sync. > > =A0=A0=A0=A0=A0=A0=A0=A0 */ > > = > > I think reading the stats/params should also be done before calling rki= sp1_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 parame= ters 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 ini= tial 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel