Received: by 2002:a25:b794:0:0:0:0:0 with SMTP id n20csp7454733ybh; Thu, 8 Aug 2019 16:22:14 -0700 (PDT) X-Google-Smtp-Source: APXvYqxlWVdJTz/WpiXzTWe0kgIFMz+8CMB1UtoXayGxoa9trKcK3aNQOh6aleYVrHBgfAnIewmy X-Received: by 2002:a17:90a:d814:: with SMTP id a20mr6470646pjv.48.1565306533950; Thu, 08 Aug 2019 16:22:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1565306533; cv=none; d=google.com; s=arc-20160816; b=pBXelFFF0Ng1ASw9+5fSvXM1qpcX+SsD4GONSN8tWnpl1v4C6jCgmWsfwXEiy0lSNY 3yv7iQ2pX8mNathOk+/ynESgLatYKmMXtiBPgYDQB7LsqzUgb+PS+bbiVfTnP7hw0oLQ IpWQfumW3mY8VymIecDvONhFdAwDK9bJUSo/lDYjmlBwHVTyGzs5jXk/JqADAtSsN9j3 yFSGAjweeYlD4jFqoFlUVD4sMP5JjJHnum7PTQTiXGL1uIU3ZjqPrrTvR1XZNKo0bmjZ eir1tHC6KzUsN4PHc3Q/rg3d+c1m7gwV6PtHUdEWBlBlrgI2Ce9A8HroN3+rONPC8oRC w4Hw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:autocrypt:openpgp:from:references:cc:to:subject; bh=DOuV/NbaxxxZ/ECFFxBuFA/SDozJR3E8A1QkzNV4rgM=; b=VikC5Wg4cQZMTgj9sKIm9B5XWR5rzPNID7nZUA4PJV+HETB+8tM4OhWZUWH6tkgkHu HvQMG8pKSIHfJcce9Py/xgFFNxqHAHXb8H1iVVUvLeH3xIUMOVY6ehmYDPvxhB8frHCg VT/mvyFAnz/1TWpTNlC3ufXaWN1IksK5HBR11leoz6s3vjbsr0TqwIi4iChbWSu4DjQA d45q3NFg3lH/Q3EHd+02QEXLpz3KH6VQmOCzSB0izjrT4Fwcpd26M8xL0jiWuj9d4+m6 NvAeIH3nzhV+FY7pfxHWja0yPpDf7mgPVc+JF6rFh76BGAhadbFX8ndYu9UPvg1O3dqe nuKQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id d1si53363703pgq.540.2019.08.08.16.21.56; Thu, 08 Aug 2019 16:22:13 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404499AbfHHWAA (ORCPT + 99 others); Thu, 8 Aug 2019 18:00:00 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:43876 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725535AbfHHWAA (ORCPT ); Thu, 8 Aug 2019 18:00:00 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: koike) with ESMTPSA id F360E2753BF Subject: Re: [PATCH v8 09/14] media: rkisp1: add rockchip isp1 core driver To: Sakari Ailus Cc: 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, tfiga@chromium.org, hans.verkuil@cisco.com, laurent.pinchart@ideasonboard.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 References: <20190730184256.30338-1-helen.koike@collabora.com> <20190730184256.30338-10-helen.koike@collabora.com> <20190807152727.GN21370@paasikivi.fi.intel.com> From: Helen Koike Openpgp: preference=signencrypt Autocrypt: addr=helen.koike@collabora.com; keydata= mQINBFmOMD4BEADb2nC8Oeyvklh+ataw2u/3mrl+hIHL4WSWtii4VxCapl9+zILuxFDrxw1p XgF3cfx7g9taWBrmLE9VEPwJA6MxaVnQuDL3GXxTxO/gqnOFgT3jT+skAt6qMvoWnhgurMGH wRaA3dO4cFrDlLsZIdDywTYcy7V2bou81ItR5Ed6c5UVX7uTTzeiD/tUi8oIf0XN4takyFuV Rf09nOhi24bn9fFN5xWHJooFaFf/k2Y+5UTkofANUp8nn4jhBUrIr6glOtmE0VT4pZMMLT63 hyRB+/s7b1zkOofUGW5LxUg+wqJXZcOAvjocqSq3VVHcgyxdm+Nv0g9Hdqo8bQHC2KBK86VK vB+R7tfv7NxVhG1sTW3CQ4gZb0ZugIWS32Mnr+V+0pxci7QpV3jrtVp5W2GA5HlXkOyC6C7H Ao7YhogtvFehnlUdG8NrkC3HhCTF8+nb08yGMVI4mMZ9v/KoIXKC6vT0Ykz434ed9Oc9pDow VUqaKi3ey96QczfE4NI029bmtCY4b5fucaB/aVqWYRH98Jh8oIQVwbt+pY7cL5PxS7dQ/Zuz 6yheqDsUGLev1O3E4R8RZ8jPcfCermL0txvoXXIA56t4ZjuHVcWEe2ERhLHFGq5Zw7KC6u12 kJoiZ6WDBYo4Dp+Gd7a81/WsA33Po0j3tk/8BWoiJCrjXzhtRwARAQABtCdIZWxlbiBLb2lr ZSA8aGVsZW4ua29pa2VAY29sbGFib3JhLmNvbT6JAlQEEwEKAD4CGwEFCwkIBwMFFQoJCAsF FgIDAQACHgECF4AWIQSofQA6zrItXEgHWTzAfqwo9yFiXQUCXEz3bwUJBKaPRQAKCRDAfqwo 9yFiXdUCD/4+WZr503hQ13KB4DijOW76ju8JDPp4p++qoPxtoAsld3yROoTI+VPWmt7ojHrr TZc7sTLxOFzaUC8HjGTb3r9ilIhIKf/M9KRLkpIJ+iLA+VoUbcSOMYWoVNfgLmbnqoezjPcy OHJwVw9dzEeYpvG6nkY6E4UktANySp27AniSXNuHOvYsOsXmUOqU1ScdsrQ9s732p/OGdTyw 1yd3gUMLZvCKFOBVHILH59HCRJgpwUPiws8G4dGMs4GTRvHT2s2mDQdQ0HEvcM9rvCRVixuC 5ZeOymZNi6lDIUIysgiZ+yzk6i5l/Ni6r7v20N3JppZvhPK6LqtaYceyAGyc3jjnOqoHT/qR kPjCwzmKiPtXjLw6HbRXtGgGtP5m3y8v6bfHH+66zd2vGCY0Z9EsqcnK4DCqRkLncFLPM2gn 9cZcCmO4ZqXUhTyn1nHM494kd5NX1Op4HO+t9ErnpufkVjoMUeBwESdQwwwHT3rjUueGmCrn VJK69/qhA4La72VTxHutl+3Z0Xy20HWsZS8Gsam39f95/LtPLzbBwnOOi5ZoXnm97tF8HrAZ 2h+kcRLMWw3BXy5q4gic+oFZMZP9oq1G9XTFld4FGgJ9ys8aGmhLM+uB1pFxb3XFtWQ2z4AJ iEp2VLl34quwfD6Gg4csiZe2KzvQHUe0w8SJ9LplrHPPprkCDQRZjjChARAAzISLQaHzaDOv ZxcoCNBk/hUGo2/gsmBW4KSj73pkStZ+pm3Yv2CRtOD4jBlycXjzhwBV7/70ZMH70/Y25dJa CnJKl/Y76dPPn2LDWrG/4EkqUzoJkhRIYFUTpkPdaVYznqLgsho19j7HpEbAum8r3jemYBE1 AIuVGg4bqY3UkvuHWLVRMuaHZNy55aYwnUvd46E64JH7O990mr6t/nu2a1aJ0BDdi8HZ0RMo Eg76Avah+YR9fZrhDFmBQSL+mcCVWEbdiOzHmGYFoToqzM52wsNEpo2aStH9KLk8zrCXGx68 ohJyQoALX4sS03RIWh1jFjnlw2FCbEdj/HDX0+U0i9COtanm54arYXiBTnAnx0F7LW7pv7sb 6tKMxsMLmprP/nWyV5AfFRi3jxs5tdwtDDk/ny8WH6KWeLR/zWDwpYgnXLBCdg8l97xUoPQO 0VkKSa4JEXUZWZx9q6kICzFGsuqApqf9gIFJZwUmirsxH80Fe04Tv+IqIAW7/djYpOqGjSyk oaEVNacwLLgZr+/j69/1ZwlbS8K+ChCtyBV4kEPzltSRZ4eU19v6sDND1JSTK9KSDtCcCcAt VGFlr4aE00AD/aOkHSylc93nPinBFO4AGhcs4WypZ3GGV6vGWCpJy9svfWsUDhSwI7GS/i/v UQ1+bswyYEY1Q3DjJqT7fXcAEQEAAYkEcgQYAQoAJgIbAhYhBKh9ADrOsi1cSAdZPMB+rCj3 IWJdBQJcTPfVBQkEpo7hAkDBdCAEGQEKAB0WIQSomGMEg78Cd/pMshveCRfNeJ05lgUCWY4w oQAKCRDeCRfNeJ05lp0gD/49i95kPKjpgjUbYeidjaWuINXMCA171KyaBAp+Jp2Qrun4sIJB Z6srMj6O/gC34AhZln2sXeQdxe88sNbg6HjlN+4AkhTd6DttjOfUwnamLDA7uw+YIapGgsgN lznjLnqOaQ9mtEwRbZMUOdyRf9osSuL14vHl4ia3bYNJ52WYre6gLMu4K+Ghd02og+ILgIio Q827h0spqIJYHrR3Ynnhxdlv5GPCobh+AKsQMdTIuCzR6JSCBk6GHkg33SiWScKMUzT8B/cn ypLfGnfV/LDZ9wS2TMzIlK/uv0Vd4C0OGDd/GCi5Gwu/Ot0aY7fzZo2CiRV+/nJBWPRRBTji bE4FG2rt7WSRLO/QmH2meIW4f0USDiHeNwznHkPei59vRdlMyQdsxrmgSRDuX9Y3UkERxbgd uscqC8Cpcy5kpF11EW91J8aGpcxASc+5Pa66/+7CrpBC2DnfcfACdMAje7yeMn9XlHrqXNlQ GaglEcnGN2qVqRcKgcjJX+ur8l56BVpBPFYQYkYkIdQAuhlPylxOvsMcqI6VoEWNt0iFF3dA //0MNb8fEqw5TlxDPOt6BDhDKowkxOGIA9LOcF4PkaR9Qkvwo2P4vA/8fhCnMqlSPom4xYdk Ev8P554zDoL/XMHl+s7A0MjIJzT253ejZKlWeO68pAbNy/z7QRn2lFDnjwkQwH6sKPchYl2f 0g//Yu3vDkqk8+mi2letP3XBl2hjv2eCZjTh34VvtgY5oeL2ROSJWNd18+7O6q3hECZ727EW gIb3LK9g4mKF6+Rch6Gwz1Y4fmC5554fd2Y2XbVzzz6AGUC6Y+ohNg7lTAVO4wu43+IyTB8u ip5rX/JDGFv7Y1sl6tQJKAVIKAJE+Z3Ncqh3doQr9wWHl0UiQYKbSR9HpH1lmC1C3EEbTpwK fUIpZd1eQNyNJl1jHsZZIBYFsAfVNH/u6lB1TU+9bSOsV5SepdIb88d0fm3oZ4KzjhRHLFQF RwNUNn3ha6x4fbxYcwbvu5ZCiiX6yRTPoage/LUNkgQNX2PtPcur6CdxK6Pqm8EAI7PmYLfN NY3y01XhKNRvaVZoH2FugfUkhsBITglTIpI+n6YU06nDAcbeINFo67TSE0iL6Pek5a6gUQQC 6w+hJCaMr8KYud0q3ccHyU3TlAPDe10En3GsVz7Y5Sa3ODGdbmkfjK8Af3ogGNBVmpV16Xl8 4rETFv7POSUB2eMtbpmBopd+wKqHCwUEy3fx1zDbM9mp+pcDoL73rRZmlgmNfW/4o4qBzxRf FYTQLE69wAFU2IFce9PjtUAlBdC+6r3X24h3uD+EC37s/vWhxuKj2glaU9ONrVJ/SPvlqXOO WR1Zqw57vHMKimLdG3c24l8PkSw1usudgAA5OyO5Ag0EWY4wyQEQAMVp0U38Le7d80Mu6AT+ 1dMes87iKn30TdMuLvSg2uYqJ1T2riRBF7zU6u74HF6zps0rPQviBXOgoSuKa1hnS6OwFb9x yQPlk76LY96SUB5jPWJ3fO78ZGSwkVbJFuG9gpD/41n8Unn1hXgDb2gUaxD0oXv/723EmTYC vSo3z6Y8A2aBQNr+PyhQAPDazvVQ+P7vnZYq1oK0w+D7aIix/Bp4mo4VbgAeAeMxXWSZs8N5 NQtXeTBgB7DqrfJP5wWwgCsROfeds6EoddcYgqhG0zVU9E54C8JcPOA0wKVs+9+gt2eyRNtx 0UhFbah7qXuJGhWy/0CLXvVoCoS+7qpWz070TBAlPZrg9D0o2gOw01trQgoKAYBKKgJhxaX/ 4gzi+5Ccm33LYH9lAVTdzdorejuV1xWdsnNyc8OAPeoXBf9RIIWfQVmbhVXBp2DAPjV6/kIJ Eml7MNJfEvqjV9zKsWF9AFlsqDWZDCyUdqR96ahTSD34pRwb6a9H99/GrjeowKaaL95DIVZT C6STvDNL6kpys4sOe2AMmQGv2MMcJB3aYLzH8f1sEQ9S0UMX7/6CifEG6JodG6Y/W/lLo1Vv DxeDA+u4Lgq6qxlksp8M78FjcmxFVlf4cpCi2ucbZxurhlBkjtZZ8MVAEde3hlqjcBl2Ah6Q D826FTxscOGlHEfNABEBAAGJAjwEGAEKACYCGwwWIQSofQA6zrItXEgHWTzAfqwo9yFiXQUC XEz31QUJBKaOuQAKCRDAfqwo9yFiXUvnEACBWe8wSnIvSX+9k4LxuLq6GQTOt+RNfliZQkCW 5lT3KL1IJyzzOm4x+/slHRBl8bF7KEZyOPinXQXyJ/vgIdgSYxDqoZ7YZn3SvuNe4aT6kGwL EYYEV8Ecj4ets15FR2jSUNnVv5YHWtZ7bP/oUzr2LT54fjRcstYxgwzoj8AREtHQ4EJWAWCO ZuEHTSm5clMFoi41CmG4DlJbzbo4YfilKYm69vwh50Y8WebcRN31jh0g8ufjOJnBldYYBLwN Obymhlfy/HKBDIbyCGBuwYoAkoJ6LR/cqzl/FuhwhuDocCGlXyYaJOwXgHaCvVXI3PLQPxWZ +vPsD+TSVHc9m/YWrOiYDnZn6aO0Uk1Zv/m9+BBkWAwsreLJ/evn3SsJV1omNBTITG+uxXcf JkgmmesIAw8mpI6EeLmReUJLasz8QkzhZIC7t5rGlQI94GQG3Jg2dC+kpaGWOaT5G4FVMcBj iR1nXfMxENVYnM5ag7mBZyD/kru5W1Uj34L6AFaDMXFPwedSCpzzqUiHb0f+nYkfOodf5xy0 46+3THy/NUS/ZZp/rI4F7Y77+MQPVg7vARfHHX1AxYUKfRVW5j88QUB70txn8Vgi1tDrOr4J eD+xr0CvIGa5lKqgQacQtGkpOpJ8zY4ObSvpNubey/qYUE3DCXD0n2Xxk4muTvqlkFpOYA== Message-ID: Date: Thu, 8 Aug 2019 18:59:47 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20190807152727.GN21370@paasikivi.fi.intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sakari, Thanks for your review, just some comments/questions below: On 8/7/19 12:27 PM, Sakari Ailus wrote: > Hi Helen, > > On Tue, Jul 30, 2019 at 03:42:51PM -0300, Helen Koike wrote: >> From: Jacob Chen >> >> Add the core 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 compilation and run time errors regarding new v4l2 async API] >> Signed-off-by: Laurent Pinchart >> [Add missing module device table] >> Signed-off-by: Ezequiel Garcia >> [update for upstream] >> Signed-off-by: Helen Koike >> >> --- >> >> Changes in v8: None >> Changes in v7: >> - VIDEO_ROCKCHIP_ISP1 selects VIDEOBUF2_VMALLOC >> - add PHY_ROCKCHIP_DPHY as a dependency for VIDEO_ROCKCHIP_ISP1 >> - Fix compilation and runtime errors due to bitrotting >> The code has bit-rotten since March 2018, fix compilation errors. >> The new V4L2 async notifier API requires notifiers to be initialized by >> a call to v4l2_async_notifier_init() before being used, do so. >> - Add missing module device table >> - use clk_bulk framework >> - add missing notifiers cleanups >> - s/strlcpy/strscpy >> - normalize bus_info name >> - fix s_stream error path, stream_cnt wans't being decremented properly >> - use devm_platform_ioremap_resource() helper >> - s/deice/device >> - redesign: remove mipi/csi subdevice, sensors connect directly to the >> isp subdevice in the media topology now. >> - remove "saved_state" member from rkisp1_stream struct >> - Reverse the order of MIs >> - Simplify MI interrupt handling >> Rather than adding unnecessary indirection, just use stream index to >> handle MI interrupt enable/disable/clear, since the stream index matches >> the order of bits now, thanks to previous patch. While at it, remove >> some dead code. >> - code styling and checkpatch fixes >> >> drivers/media/platform/Kconfig | 12 + >> drivers/media/platform/Makefile | 1 + >> drivers/media/platform/rockchip/isp1/Makefile | 7 + >> drivers/media/platform/rockchip/isp1/common.h | 101 +++ >> drivers/media/platform/rockchip/isp1/dev.c | 675 ++++++++++++++++++ >> drivers/media/platform/rockchip/isp1/dev.h | 97 +++ >> 6 files changed, 893 insertions(+) >> create mode 100644 drivers/media/platform/rockchip/isp1/Makefile >> create mode 100644 drivers/media/platform/rockchip/isp1/common.h >> create mode 100644 drivers/media/platform/rockchip/isp1/dev.c >> create mode 100644 drivers/media/platform/rockchip/isp1/dev.h >> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig >> index 89555f9a813f..e0e98937c565 100644 >> --- a/drivers/media/platform/Kconfig >> +++ b/drivers/media/platform/Kconfig >> @@ -106,6 +106,18 @@ config VIDEO_QCOM_CAMSS >> select VIDEOBUF2_DMA_SG >> select V4L2_FWNODE >> >> +config VIDEO_ROCKCHIP_ISP1 >> + tristate "Rockchip Image Signal Processing v1 Unit driver" >> + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API >> + depends on ARCH_ROCKCHIP || COMPILE_TEST >> + select VIDEOBUF2_DMA_CONTIG >> + select VIDEOBUF2_VMALLOC >> + select V4L2_FWNODE >> + select PHY_ROCKCHIP_DPHY >> + default n >> + ---help--- >> + Support for ISP1 on the rockchip SoC. >> + >> config VIDEO_S3C_CAMIF >> tristate "Samsung S3C24XX/S3C64XX SoC Camera Interface driver" >> depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API >> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile >> index 7cbbd925124c..f9fcf8e7c513 100644 >> --- a/drivers/media/platform/Makefile >> +++ b/drivers/media/platform/Makefile >> @@ -69,6 +69,7 @@ obj-$(CONFIG_VIDEO_RENESAS_FDP1) += rcar_fdp1.o >> obj-$(CONFIG_VIDEO_RENESAS_JPU) += rcar_jpu.o >> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/ >> >> +obj-$(CONFIG_VIDEO_ROCKCHIP_ISP1) += rockchip/isp1/ >> obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/ >> >> obj-y += omap/ >> diff --git a/drivers/media/platform/rockchip/isp1/Makefile b/drivers/media/platform/rockchip/isp1/Makefile >> new file mode 100644 >> index 000000000000..72706e80fc8b >> --- /dev/null >> +++ b/drivers/media/platform/rockchip/isp1/Makefile >> @@ -0,0 +1,7 @@ >> +obj-$(CONFIG_VIDEO_ROCKCHIP_ISP1) += rockchip-isp1.o >> +rockchip-isp1-objs += rkisp1.o \ >> + dev.o \ >> + regs.o \ >> + isp_stats.o \ >> + isp_params.o \ >> + capture.o >> diff --git a/drivers/media/platform/rockchip/isp1/common.h b/drivers/media/platform/rockchip/isp1/common.h >> new file mode 100644 >> index 000000000000..606ce2793546 >> --- /dev/null >> +++ b/drivers/media/platform/rockchip/isp1/common.h >> @@ -0,0 +1,101 @@ >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >> +/* >> + * Rockchip isp1 driver >> + * >> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd. >> + */ >> + >> +#ifndef _RKISP1_COMMON_H >> +#define _RKISP1_COMMON_H >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define RKISP1_DEFAULT_WIDTH 800 >> +#define RKISP1_DEFAULT_HEIGHT 600 >> + >> +#define RKISP1_MAX_STREAM 2 >> +#define RKISP1_STREAM_MP 0 >> +#define RKISP1_STREAM_SP 1 >> + >> +#define RKISP1_PLANE_Y 0 >> +#define RKISP1_PLANE_CB 1 >> +#define RKISP1_PLANE_CR 2 >> + >> +enum rkisp1_sd_type { >> + RKISP1_SD_SENSOR, >> + RKISP1_SD_PHY_CSI, >> + RKISP1_SD_VCM, >> + RKISP1_SD_FLASH, >> + RKISP1_SD_MAX, >> +}; > > I wonder if this is a leftover from the driver development time. Same goes > for the subdevs field in struct rkisp1_device. It is a left over, I'm removing them for the next version, thanks. > >> + >> +/* One structure per video node */ >> +struct rkisp1_vdev_node { >> + struct vb2_queue buf_queue; >> + /* vfd lock */ >> + struct mutex vlock; >> + struct video_device vdev; >> + struct media_pad pad; >> +}; >> + >> +enum rkisp1_fmt_pix_type { >> + FMT_YUV, >> + FMT_RGB, >> + FMT_BAYER, >> + FMT_JPEG, >> + FMT_MAX >> +}; >> + >> +enum rkisp1_fmt_raw_pat_type { >> + RAW_RGGB = 0, >> + RAW_GRBG, >> + RAW_GBRG, >> + RAW_BGGR, >> +}; >> + >> +struct rkisp1_buffer { >> + struct vb2_v4l2_buffer vb; >> + struct list_head queue; >> + union { >> + u32 buff_addr[VIDEO_MAX_PLANES]; >> + void *vaddr[VIDEO_MAX_PLANES]; >> + }; >> +}; >> + >> +struct rkisp1_dummy_buffer { >> + void *vaddr; >> + dma_addr_t dma_addr; >> + u32 size; >> +}; >> + >> +extern int rkisp1_debug; >> + >> +static inline >> +struct rkisp1_vdev_node *vdev_to_node(struct video_device *vdev) >> +{ >> + return container_of(vdev, struct rkisp1_vdev_node, vdev); >> +} >> + >> +static inline struct rkisp1_vdev_node *queue_to_node(struct vb2_queue *q) >> +{ >> + return container_of(q, struct rkisp1_vdev_node, buf_queue); >> +} >> + >> +static inline struct rkisp1_buffer *to_rkisp1_buffer(struct vb2_v4l2_buffer *vb) >> +{ >> + return container_of(vb, struct rkisp1_buffer, vb); >> +} >> + >> +static inline struct vb2_queue *to_vb2_queue(struct file *file) >> +{ >> + struct rkisp1_vdev_node *vnode = video_drvdata(file); >> + >> + return &vnode->buf_queue; >> +} >> + >> +#endif /* _RKISP1_COMMON_H */ >> diff --git a/drivers/media/platform/rockchip/isp1/dev.c b/drivers/media/platform/rockchip/isp1/dev.c >> new file mode 100644 >> index 000000000000..2b4a67e1a3b5 >> --- /dev/null >> +++ b/drivers/media/platform/rockchip/isp1/dev.c >> @@ -0,0 +1,675 @@ >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> +/* >> + * Rockchip isp1 driver >> + * >> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "common.h" >> +#include "regs.h" >> + >> +struct isp_match_data { >> + const char * const *clks; >> + int size; > > unsigned int ack > >> +}; >> + >> +struct sensor_async_subdev { >> + struct v4l2_async_subdev asd; >> + struct v4l2_mbus_config mbus; >> + unsigned int lanes; >> +}; >> + >> +int rkisp1_debug; >> +module_param_named(debug, rkisp1_debug, int, 0644); >> +MODULE_PARM_DESC(debug, "Debug level (0-1)"); > > Have you thought of using dynamic debug instead? right, this is being used in v4l2_dbg(), which I can replace by dev_dbg() that is already covered by dynamic debug iirc. Should I also replace v4l2_err() by dev_err() (I always get confused by which log function I should use). > >> + >> +/**************************** pipeline operations******************************/ >> + >> +static int __isp_pipeline_prepare(struct rkisp1_pipeline *p, >> + struct media_entity *me) >> +{ >> + struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe); >> + struct v4l2_subdev *sd; >> + unsigned int i; >> + >> + p->num_subdevs = 0; >> + memset(p->subdevs, 0, sizeof(p->subdevs)); >> + >> + while (1) { >> + struct media_pad *pad = NULL; >> + >> + /* Find remote source pad */ >> + for (i = 0; i < me->num_pads; i++) { >> + struct media_pad *spad = &me->pads[i]; >> + >> + if (!(spad->flags & MEDIA_PAD_FL_SINK)) >> + continue; >> + pad = media_entity_remote_pad(spad); >> + if (pad) >> + break; >> + } >> + >> + if (!pad) >> + break; >> + >> + sd = media_entity_to_v4l2_subdev(pad->entity); >> + if (sd != &dev->isp_sdev.sd) >> + p->subdevs[p->num_subdevs++] = sd; > > How do you make sure you don't overrun the array? Because the maximum path the topology can have is: sensor->rkisp->capture > > Instead, I'd avoid maintaining the array in the first place --- the same > information is available from the MC framework data structures --- see e.g. > the omap3isp driver. If I understand correctly, omap3isp navigates through the topology in the same way, but it just don't save in an array, but I reuse this information in other places, mostly for power up/down (see below why I don't use v4l2_pipeline_pm_use()) > >> + >> + me = &sd->entity; >> + if (me->num_pads == 1) >> + break; >> + } >> + return 0; >> +} >> + >> +static int __subdev_set_power(struct v4l2_subdev *sd, int on) >> +{ >> + int ret; >> + >> + if (!sd) >> + return -ENXIO; >> + >> + ret = v4l2_subdev_call(sd, core, s_power, on); >> + >> + return ret != -ENOIOCTLCMD ? ret : 0; >> +} >> + >> +static int __isp_pipeline_s_power(struct rkisp1_pipeline *p, bool on) > > Could you instead use v4l2_pipeline_pm_use()? Unless I misunderstood this (which is very likely), v4l2_pipeline_pm_use() calls pipeline_pm_power(), that applies power change to all entities in a pipeline, but if I have two sensors connected to the ISP, one with link enabled and the other with a disabled link, I don't want to power both sensors on, just the one participating in that stream. And if I call v4l2_pipeline_pm_use() it will power on both, which is not what I want. Does it make sense? > >> +{ >> + struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe); >> + int i, ret; >> + >> + if (on) { >> + __subdev_set_power(&dev->isp_sdev.sd, true); >> + >> + for (i = p->num_subdevs - 1; i >= 0; --i) { >> + ret = __subdev_set_power(p->subdevs[i], true); >> + if (ret < 0 && ret != -ENXIO) >> + goto err_power_off; >> + } >> + } else { >> + for (i = 0; i < p->num_subdevs; ++i) >> + __subdev_set_power(p->subdevs[i], false); >> + >> + __subdev_set_power(&dev->isp_sdev.sd, false); >> + } >> + >> + return 0; >> + >> +err_power_off: >> + for (++i; i < p->num_subdevs; ++i) >> + __subdev_set_power(p->subdevs[i], false); >> + __subdev_set_power(&dev->isp_sdev.sd, true); >> + return ret; >> +} >> + >> +static int rkisp1_pipeline_open(struct rkisp1_pipeline *p, >> + struct media_entity *me, >> + bool prepare) >> +{ >> + int ret; >> + >> + if (WARN_ON(!p || !me)) >> + return -EINVAL; >> + if (atomic_inc_return(&p->power_cnt) > 1) >> + return 0; >> + >> + /* go through media graphic and get subdevs */ >> + if (prepare) >> + __isp_pipeline_prepare(p, me); >> + >> + if (!p->num_subdevs) >> + return -EINVAL; >> + >> + ret = __isp_pipeline_s_power(p, 1); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int rkisp1_pipeline_close(struct rkisp1_pipeline *p) >> +{ >> + int ret; >> + >> + if (atomic_dec_return(&p->power_cnt) > 0) >> + return 0; >> + ret = __isp_pipeline_s_power(p, 0); >> + >> + return ret == -ENXIO ? 0 : ret; >> +} >> + >> +/* >> + * stream-on order: isp_subdev, mipi dphy, sensor >> + * stream-off order: mipi dphy, sensor, isp_subdev >> + */ >> +static int rkisp1_pipeline_set_stream(struct rkisp1_pipeline *p, bool on) >> +{ >> + struct rkisp1_device *dev = container_of(p, struct rkisp1_device, pipe); >> + int i, ret; >> + >> + if ((on && atomic_inc_return(&p->stream_cnt) > 1) || >> + (!on && atomic_dec_return(&p->stream_cnt) > 0)) >> + return 0; >> + >> + if (on) { >> + ret = v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream, >> + true); >> + if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) { >> + v4l2_err(&dev->v4l2_dev, >> + "s_stream failed on subdevice %s (%d)\n", >> + dev->isp_sdev.sd.name, >> + ret); >> + atomic_dec(&p->stream_cnt); >> + return ret; >> + } >> + } >> + >> + /* phy -> sensor */ >> + for (i = 0; i < p->num_subdevs; ++i) { >> + ret = v4l2_subdev_call(p->subdevs[i], video, s_stream, on); >> + if (on && ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV) >> + goto err_stream_off; > > You should stop after the first external sub-device. > > It seems even the omap3isp driver doesn't do that. It's not easy to spot > such issues indeed. I'm not sure I understand, what do you call an external sub-device? Is the sensor an external subdevice? > >> + } >> + >> + if (!on) >> + v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream, false); >> + >> + return 0; >> + >> +err_stream_off: >> + for (--i; i >= 0; --i) >> + v4l2_subdev_call(p->subdevs[i], video, s_stream, false); >> + v4l2_subdev_call(&dev->isp_sdev.sd, video, s_stream, false); >> + atomic_dec(&p->stream_cnt); >> + return ret; >> +} >> + >> +/***************************** media controller *******************************/ >> +/* See http://opensource.rock-chips.com/wiki_Rockchip-isp1 for Topology */ > > The host appears to be down, or there's a routing problem. Unless this is > fixed, having such a URL here doesn't do much good. :-I This is a left over, sorry about that. I can access the URL now. I'll try to get some of the docs and move to the kernel docs. > >> + >> +static int rkisp1_create_links(struct rkisp1_device *dev) >> +{ >> + struct media_entity *source, *sink; >> + struct rkisp1_sensor *sensor; >> + unsigned int flags, pad; >> + int ret; >> + >> + /* sensor links(or mipi-phy) */ >> + list_for_each_entry(sensor, &dev->sensors, list) { >> + for (pad = 0; pad < sensor->sd->entity.num_pads; pad++) >> + if (sensor->sd->entity.pads[pad].flags & >> + MEDIA_PAD_FL_SOURCE) >> + break; > > Could you use media_entity_get_fwnode_pad() instead? Yes, I didn't know about it actually, thanks for that, looks cleaner (I'll send in the next version). > >> + >> + if (pad == sensor->sd->entity.num_pads) { >> + dev_err(dev->dev, >> + "failed to find src pad for %s\n", >> + sensor->sd->name); >> + >> + return -ENXIO; >> + } >> + >> + ret = media_create_pad_link( >> + &sensor->sd->entity, pad, >> + &dev->isp_sdev.sd.entity, >> + RKISP1_ISP_PAD_SINK, >> + list_is_first(&sensor->list, &dev->sensors) ? >> + MEDIA_LNK_FL_ENABLED : 0); >> + if (ret) { >> + dev_err(dev->dev, >> + "failed to create link for %s\n", >> + sensor->sd->name); >> + return ret; >> + } >> + } >> + >> + /* params links */ >> + source = &dev->params_vdev.vnode.vdev.entity; >> + sink = &dev->isp_sdev.sd.entity; >> + flags = MEDIA_LNK_FL_ENABLED; >> + ret = media_create_pad_link(source, 0, sink, >> + RKISP1_ISP_PAD_SINK_PARAMS, flags); >> + if (ret < 0) >> + return ret; >> + >> + /* create isp internal links */ >> + /* SP links */ >> + source = &dev->isp_sdev.sd.entity; >> + sink = &dev->stream[RKISP1_STREAM_SP].vnode.vdev.entity; >> + ret = media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_PATH, >> + sink, 0, flags); >> + if (ret < 0) >> + return ret; >> + >> + /* MP links */ >> + source = &dev->isp_sdev.sd.entity; >> + sink = &dev->stream[RKISP1_STREAM_MP].vnode.vdev.entity; >> + ret = media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_PATH, >> + sink, 0, flags); >> + if (ret < 0) >> + return ret; >> + >> + /* 3A stats links */ >> + source = &dev->isp_sdev.sd.entity; >> + sink = &dev->stats_vdev.vnode.vdev.entity; >> + return media_create_pad_link(source, RKISP1_ISP_PAD_SOURCE_STATS, >> + sink, 0, flags); > > Indentation. Same for the calls to the same function above. ack > >> +} >> + >> +static int subdev_notifier_bound(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *sd, >> + struct v4l2_async_subdev *asd) >> +{ >> + struct rkisp1_device *isp_dev = container_of(notifier, >> + struct rkisp1_device, >> + notifier); >> + struct sensor_async_subdev *s_asd = container_of(asd, >> + struct sensor_async_subdev, asd); >> + struct rkisp1_sensor *sensor; >> + >> + sensor = devm_kzalloc(isp_dev->dev, sizeof(*sensor), GFP_KERNEL); >> + if (!sensor) >> + return -ENOMEM; >> + >> + sensor->lanes = s_asd->lanes; >> + sensor->mbus = s_asd->mbus; >> + sensor->sd = sd; >> + sensor->dphy = devm_phy_get(isp_dev->dev, "dphy"); >> + if (IS_ERR(sensor->dphy)) { >> + if (PTR_ERR(sensor->dphy) != -EPROBE_DEFER) >> + dev_err(isp_dev->dev, "Couldn't get the MIPI D-PHY\n"); >> + return PTR_ERR(sensor->dphy); >> + } >> + phy_init(sensor->dphy); >> + >> + list_add(&sensor->list, &isp_dev->sensors); > > In general, maintaining the information on the external subdevs on your own > adds complexity to the driver. You can get the information when you need it > from the data structures maintained by MC (see e.g. the omap3isp driver for > examples). > >> + >> + return 0; >> +} >> + >> +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; >> +} >> + >> +static void subdev_notifier_unbind(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *sd, >> + struct v4l2_async_subdev *asd) >> +{ >> + struct rkisp1_device *isp_dev = container_of(notifier, >> + struct rkisp1_device, >> + notifier); >> + struct rkisp1_sensor *sensor = sd_to_sensor(isp_dev, sd); >> + >> + /* TODO: check if a lock is required here */ >> + list_del(&sensor->list); >> + >> + phy_exit(sensor->dphy); >> +} >> + >> +static int subdev_notifier_complete(struct v4l2_async_notifier *notifier) >> +{ >> + struct rkisp1_device *dev = container_of(notifier, struct rkisp1_device, >> + notifier); >> + int ret; >> + >> + mutex_lock(&dev->media_dev.graph_mutex); >> + ret = rkisp1_create_links(dev); >> + if (ret < 0) >> + goto unlock; >> + ret = v4l2_device_register_subdev_nodes(&dev->v4l2_dev); >> + if (ret < 0) >> + goto unlock; >> + >> + v4l2_info(&dev->v4l2_dev, "Async subdev notifier completed\n"); >> + >> +unlock: >> + mutex_unlock(&dev->media_dev.graph_mutex); >> + return ret; >> +} >> + >> +static int rkisp1_fwnode_parse(struct device *dev, >> + struct v4l2_fwnode_endpoint *vep, >> + struct v4l2_async_subdev *asd) >> +{ >> + struct sensor_async_subdev *s_asd = >> + container_of(asd, struct sensor_async_subdev, asd); >> + >> + if (vep->bus_type != V4L2_MBUS_CSI2_DPHY) { >> + dev_err(dev, "Only CSI2 bus type is currently supported\n"); >> + return -EINVAL; >> + } >> + >> + if (vep->base.port != 0) { >> + dev_err(dev, "The ISP has only port 0\n"); >> + return -EINVAL; >> + } >> + >> + s_asd->mbus.type = vep->bus_type; >> + s_asd->mbus.flags = vep->bus.mipi_csi2.flags; >> + s_asd->lanes = vep->bus.mipi_csi2.num_data_lanes; >> + >> + switch (vep->bus.mipi_csi2.num_data_lanes) { >> + case 1: >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_1_LANE; >> + break; >> + case 2: >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_2_LANE; >> + break; >> + case 3: >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_3_LANE; >> + break; >> + case 4: >> + s_asd->mbus.flags |= V4L2_MBUS_CSI2_4_LANE; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = { >> + .bound = subdev_notifier_bound, >> + .unbind = subdev_notifier_unbind, >> + .complete = subdev_notifier_complete, >> +}; >> + >> +static int isp_subdev_notifier(struct rkisp1_device *isp_dev) >> +{ >> + struct v4l2_async_notifier *ntf = &isp_dev->notifier; >> + struct device *dev = isp_dev->dev; >> + int ret; >> + >> + v4l2_async_notifier_init(ntf); >> + >> + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( >> + dev, ntf, sizeof(struct sensor_async_subdev), 0, >> + rkisp1_fwnode_parse); >> + if (ret < 0) >> + return ret; >> + >> + if (list_empty(&ntf->asd_list)) >> + return -ENODEV; /* no endpoint */ >> + >> + ntf->ops = &subdev_notifier_ops; >> + >> + return v4l2_async_notifier_register(&isp_dev->v4l2_dev, ntf); >> +} >> + >> +/***************************** platform device *******************************/ >> + >> +static int rkisp1_register_platform_subdevs(struct rkisp1_device *dev) >> +{ >> + int ret; >> + >> + ret = rkisp1_register_isp_subdev(dev, &dev->v4l2_dev); >> + if (ret < 0) >> + return ret; >> + >> + ret = rkisp1_register_stream_vdevs(dev); >> + if (ret < 0) >> + goto err_unreg_isp_subdev; >> + >> + ret = rkisp1_register_stats_vdev(&dev->stats_vdev, &dev->v4l2_dev, dev); >> + if (ret < 0) >> + goto err_unreg_stream_vdev; >> + >> + ret = rkisp1_register_params_vdev(&dev->params_vdev, &dev->v4l2_dev, >> + dev); >> + if (ret < 0) >> + goto err_unreg_stats_vdev; >> + >> + ret = isp_subdev_notifier(dev); >> + if (ret < 0) { >> + v4l2_err(&dev->v4l2_dev, >> + "Failed to register subdev notifier(%d)\n", ret); >> + goto err_unreg_params_vdev; >> + } >> + >> + return 0; >> +err_unreg_params_vdev: >> + rkisp1_unregister_params_vdev(&dev->params_vdev); >> +err_unreg_stats_vdev: >> + rkisp1_unregister_stats_vdev(&dev->stats_vdev); >> +err_unreg_stream_vdev: >> + rkisp1_unregister_stream_vdevs(dev); >> +err_unreg_isp_subdev: >> + rkisp1_unregister_isp_subdev(dev); >> + return ret; >> +} >> + >> +static const char * const rk3399_isp_clks[] = { >> + "clk_isp", >> + "aclk_isp", >> + "hclk_isp", >> + "aclk_isp_wrap", >> + "hclk_isp_wrap", >> +}; >> + >> +static const char * const rk3288_isp_clks[] = { >> + "clk_isp", >> + "aclk_isp", >> + "hclk_isp", >> + "pclk_isp_in", >> + "sclk_isp_jpe", >> +}; >> + >> +static const struct isp_match_data rk3288_isp_clk_data = { >> + .clks = rk3288_isp_clks, >> + .size = ARRAY_SIZE(rk3288_isp_clks), >> +}; >> + >> +static const struct isp_match_data rk3399_isp_clk_data = { >> + .clks = rk3399_isp_clks, >> + .size = ARRAY_SIZE(rk3399_isp_clks), >> +}; >> + >> +static const struct of_device_id rkisp1_plat_of_match[] = { >> + { >> + .compatible = "rockchip,rk3288-cif-isp", >> + .data = &rk3288_isp_clk_data, >> + }, { >> + .compatible = "rockchip,rk3399-cif-isp", >> + .data = &rk3399_isp_clk_data, >> + }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, rkisp1_plat_of_match); >> + >> +static irqreturn_t rkisp1_irq_handler(int irq, void *ctx) >> +{ >> + struct device *dev = ctx; >> + struct rkisp1_device *rkisp1_dev = dev_get_drvdata(dev); >> + unsigned int mis_val; >> + >> + mis_val = readl(rkisp1_dev->base_addr + CIF_ISP_MIS); >> + if (mis_val) >> + rkisp1_isp_isr(mis_val, rkisp1_dev); >> + >> + mis_val = readl(rkisp1_dev->base_addr + CIF_MIPI_MIS); >> + if (mis_val) >> + rkisp1_mipi_isr(mis_val, rkisp1_dev); >> + >> + mis_val = readl(rkisp1_dev->base_addr + CIF_MI_MIS); >> + if (mis_val) >> + rkisp1_mi_isr(mis_val, rkisp1_dev); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int rkisp1_plat_probe(struct platform_device *pdev) >> +{ >> + struct device_node *node = pdev->dev.of_node; >> + const struct isp_match_data *clk_data; >> + const struct of_device_id *match; >> + struct device *dev = &pdev->dev; >> + struct rkisp1_device *isp_dev; >> + struct v4l2_device *v4l2_dev; >> + unsigned int i; >> + int ret, irq; >> + >> + match = of_match_node(rkisp1_plat_of_match, node); >> + isp_dev = devm_kzalloc(dev, sizeof(*isp_dev), GFP_KERNEL); >> + if (!isp_dev) >> + return -ENOMEM; >> + >> + INIT_LIST_HEAD(&isp_dev->sensors); >> + >> + dev_set_drvdata(dev, isp_dev); >> + isp_dev->dev = dev; >> + >> + isp_dev->base_addr = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(isp_dev->base_addr)) >> + return PTR_ERR(isp_dev->base_addr); >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return irq; >> + >> + ret = devm_request_irq(dev, irq, rkisp1_irq_handler, IRQF_SHARED, >> + dev_driver_string(dev), dev); >> + if (ret < 0) { >> + dev_err(dev, "request irq failed: %d\n", ret); >> + return ret; >> + } >> + >> + isp_dev->irq = irq; >> + clk_data = match->data; >> + >> + for (i = 0; i < clk_data->size; i++) >> + isp_dev->clks[i].id = clk_data->clks[i]; >> + ret = devm_clk_bulk_get(dev, clk_data->size, isp_dev->clks); >> + if (ret) >> + return ret; >> + isp_dev->clk_size = clk_data->size; >> + >> + atomic_set(&isp_dev->pipe.power_cnt, 0); >> + atomic_set(&isp_dev->pipe.stream_cnt, 0); >> + isp_dev->pipe.open = rkisp1_pipeline_open; >> + isp_dev->pipe.close = rkisp1_pipeline_close; >> + isp_dev->pipe.set_stream = rkisp1_pipeline_set_stream; >> + >> + rkisp1_stream_init(isp_dev, RKISP1_STREAM_SP); >> + rkisp1_stream_init(isp_dev, RKISP1_STREAM_MP); >> + >> + strscpy(isp_dev->media_dev.model, "rkisp1", >> + sizeof(isp_dev->media_dev.model)); >> + isp_dev->media_dev.dev = &pdev->dev; >> + strscpy(isp_dev->media_dev.bus_info, >> + "platform: " DRIVER_NAME, sizeof(isp_dev->media_dev.bus_info)); >> + media_device_init(&isp_dev->media_dev); >> + >> + v4l2_dev = &isp_dev->v4l2_dev; >> + v4l2_dev->mdev = &isp_dev->media_dev; >> + strscpy(v4l2_dev->name, "rkisp1", sizeof(v4l2_dev->name)); >> + v4l2_ctrl_handler_init(&isp_dev->ctrl_handler, 5); >> + v4l2_dev->ctrl_handler = &isp_dev->ctrl_handler; >> + >> + ret = v4l2_device_register(isp_dev->dev, &isp_dev->v4l2_dev); >> + if (ret < 0) > > Once you've initialised the control handler, you'll need to free it in case > of an error. I.e. add one more label for that purpose near the end. control handler is not required, I'll remove it for the next version. > >> + return ret; >> + >> + ret = media_device_register(&isp_dev->media_dev); >> + if (ret < 0) { >> + v4l2_err(v4l2_dev, "Failed to register media device: %d\n", >> + ret); >> + goto err_unreg_v4l2_dev; >> + } >> + >> + /* create & register platefom subdev (from of_node) */ >> + ret = rkisp1_register_platform_subdevs(isp_dev); >> + if (ret < 0) >> + goto err_unreg_media_dev; >> + >> + pm_runtime_enable(&pdev->dev); >> + >> + return 0; >> + >> +err_unreg_media_dev: >> + media_device_unregister(&isp_dev->media_dev); >> +err_unreg_v4l2_dev: >> + v4l2_device_unregister(&isp_dev->v4l2_dev); >> + return ret; >> +} >> + >> +static int rkisp1_plat_remove(struct platform_device *pdev) >> +{ >> + struct rkisp1_device *isp_dev = platform_get_drvdata(pdev); >> + >> + pm_runtime_disable(&pdev->dev); >> + media_device_unregister(&isp_dev->media_dev); >> + v4l2_async_notifier_unregister(&isp_dev->notifier); >> + v4l2_async_notifier_cleanup(&isp_dev->notifier); >> + v4l2_device_unregister(&isp_dev->v4l2_dev); >> + rkisp1_unregister_params_vdev(&isp_dev->params_vdev); >> + rkisp1_unregister_stats_vdev(&isp_dev->stats_vdev); >> + rkisp1_unregister_stream_vdevs(isp_dev); >> + rkisp1_unregister_isp_subdev(isp_dev); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev) >> +{ >> + struct rkisp1_device *isp_dev = dev_get_drvdata(dev); >> + >> + clk_bulk_disable_unprepare(isp_dev->clk_size, isp_dev->clks); >> + return pinctrl_pm_select_sleep_state(dev); >> +} >> + >> +static int __maybe_unused rkisp1_runtime_resume(struct device *dev) >> +{ >> + struct rkisp1_device *isp_dev = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = pinctrl_pm_select_default_state(dev); >> + if (ret < 0) >> + return ret; >> + ret = clk_bulk_prepare_enable(isp_dev->clk_size, isp_dev->clks); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops rkisp1_plat_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> + pm_runtime_force_resume) >> + SET_RUNTIME_PM_OPS(rkisp1_runtime_suspend, rkisp1_runtime_resume, NULL) >> +}; >> + >> +static struct platform_driver rkisp1_plat_drv = { >> + .driver = { >> + .name = DRIVER_NAME, >> + .of_match_table = of_match_ptr(rkisp1_plat_of_match), >> + .pm = &rkisp1_plat_pm_ops, >> + }, >> + .probe = rkisp1_plat_probe, >> + .remove = rkisp1_plat_remove, >> +}; >> + >> +module_platform_driver(rkisp1_plat_drv); >> +MODULE_AUTHOR("Rockchip Camera/ISP team"); >> +MODULE_DESCRIPTION("Rockchip ISP1 platform driver"); >> +MODULE_LICENSE("Dual BSD/GPL"); > > BSD or MIT? Thanks for spotting this, I'll verify. > >> diff --git a/drivers/media/platform/rockchip/isp1/dev.h b/drivers/media/platform/rockchip/isp1/dev.h >> new file mode 100644 >> index 000000000000..f7cbee316523 >> --- /dev/null >> +++ b/drivers/media/platform/rockchip/isp1/dev.h >> @@ -0,0 +1,97 @@ >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */ >> +/* >> + * Rockchip isp1 driver >> + * >> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd. >> + */ >> + >> +#ifndef _RKISP1_DEV_H >> +#define _RKISP1_DEV_H >> + >> +#include >> + >> +#include "capture.h" >> +#include "rkisp1.h" >> +#include "isp_params.h" >> +#include "isp_stats.h" >> + >> +#define DRIVER_NAME "rkisp1" >> +#define ISP_VDEV_NAME DRIVER_NAME "_ispdev" >> +#define SP_VDEV_NAME DRIVER_NAME "_selfpath" >> +#define MP_VDEV_NAME DRIVER_NAME "_mainpath" >> +#define DMA_VDEV_NAME DRIVER_NAME "_dmapath" >> + >> +#define GRP_ID_SENSOR BIT(0) >> +#define GRP_ID_MIPIPHY BIT(1) >> +#define GRP_ID_ISP BIT(2) >> +#define GRP_ID_ISP_MP BIT(3) >> +#define GRP_ID_ISP_SP BIT(4) >> + >> +#define RKISP1_MAX_BUS_CLK 8 >> +#define RKISP1_MAX_SENSOR 2 >> +#define RKISP1_MAX_PIPELINE 4 >> + >> +/* >> + * struct rkisp1_pipeline - An ISP hardware pipeline >> + * >> + * Capture device call other devices via pipeline >> + * >> + * @num_subdevs: number of linked subdevs >> + * @power_cnt: pipeline power count >> + * @stream_cnt: stream power count >> + */ >> +struct rkisp1_pipeline { >> + struct media_pipeline pipe; >> + int num_subdevs; >> + atomic_t power_cnt; >> + atomic_t stream_cnt; >> + struct v4l2_subdev *subdevs[RKISP1_MAX_PIPELINE]; >> + int (*open)(struct rkisp1_pipeline *p, >> + struct media_entity *me, bool prepare); >> + int (*close)(struct rkisp1_pipeline *p); >> + int (*set_stream)(struct rkisp1_pipeline *p, bool on); >> +}; >> + >> +/* >> + * struct rkisp1_sensor - Sensor information >> + * @mbus: media bus configuration >> + */ >> +struct rkisp1_sensor { >> + struct v4l2_subdev *sd; >> + struct v4l2_mbus_config mbus; >> + unsigned int lanes; >> + struct phy *dphy; >> + struct list_head list; >> +}; > > You seem to also have struct sensor_async_subdev that appears to contain > similar information. Would it be possible to unify the two? > > This would appear to allow you getting rid of functions such as > sd_to_sensor, for instance. ack, I managed to get rid of this, and I don't even need to keep them on a list, I'll submit in the next version. Thanks a lot for your review Helen > >> + >> +/* >> + * struct rkisp1_device - ISP platform device >> + * @base_addr: base register address >> + * @active_sensor: sensor in-use, set when streaming on >> + * @isp_sdev: ISP sub-device >> + * @rkisp1_stream: capture video device >> + * @stats_vdev: ISP statistics output device >> + * @params_vdev: ISP input parameters device >> + */ >> +struct rkisp1_device { >> + void __iomem *base_addr; >> + int irq; >> + struct device *dev; >> + unsigned int clk_size; >> + struct clk_bulk_data clks[RKISP1_MAX_BUS_CLK]; >> + struct v4l2_device v4l2_dev; >> + struct v4l2_ctrl_handler ctrl_handler; >> + struct media_device media_dev; >> + struct v4l2_async_notifier notifier; >> + struct v4l2_subdev *subdevs[RKISP1_SD_MAX]; >> + struct rkisp1_sensor *active_sensor; >> + struct list_head sensors; >> + struct rkisp1_isp_subdev isp_sdev; >> + struct rkisp1_stream stream[RKISP1_MAX_STREAM]; >> + struct rkisp1_isp_stats_vdev stats_vdev; >> + struct rkisp1_isp_params_vdev params_vdev; >> + struct rkisp1_pipeline pipe; >> + struct vb2_alloc_ctx *alloc_ctx; >> +}; >> + >> +#endif >