From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 34BF93D6D; Fri, 26 Aug 2022 22:39:54 +0000 (UTC) Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D925230B; Sat, 27 Aug 2022 00:39:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1661553592; bh=R8qukP2RffLfxf6EBUyFnNoBjpObxeBRg1U0SiCvU9E=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eYUKTzF6QSKxmelG0IGUX5fRApy+oFTa6yaxqckT7jFur3bMN2VPKUHjCfiA1D7Dl s7m6NBji3WfpvsbirYWASI+L4MTJBY7CcsEX7FwALMxXpad6hEN4t2JjLH+Zy5h+Hr W1y03MKpyhsE4sK5Wn/oU4StLWJ8WPIGG+scy2vo= Date: Sat, 27 Aug 2022 01:39:44 +0300 From: Laurent Pinchart To: Paul Kocialkowski Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Sakari Ailus , Hans Verkuil , Maxime Ripard , Thomas Petazzoni Subject: Re: [PATCH v6 5/6] media: sun6i-csi: Detect the availability of the ISP Message-ID: References: <20220826184144.605605-1-paul.kocialkowski@bootlin.com> <20220826184144.605605-6-paul.kocialkowski@bootlin.com> Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220826184144.605605-6-paul.kocialkowski@bootlin.com> Hi Paul, Thank you for the patch. On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > Add a helper to detect whether the ISP is available and connected > and store the indication in a driver-wide variable. This sounds like it would be a global variable, while it's stored in the driver-specific device structure. > > Signed-off-by: Paul Kocialkowski > --- > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index 00521f966cee..b16166cba2ef 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -24,6 +24,35 @@ > #include "sun6i_csi_capture.h" > #include "sun6i_csi_reg.h" > > +/* ISP */ > + > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > +{ > + struct device *dev = csi_dev->dev; > + struct fwnode_handle *handle = NULL; No need to initialize this to NULL. > + > + /* ISP is not available if disabled in kernel config. */ > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > + return 0; Hmmm... The ISP driver may be disabled when compiling the sun6i-csi driver, but later enabled and deployed. Disabling ISP support silently like this could be confusing. Could it be better to move this check after the graph check, and print a warning message in this case ? > + > + /* > + * ISP is not available if not connected via fwnode graph. > + * This weill also check that the remote parent node is available. s/weill/will/ * ISP is not available if not connected via fwnode graph. This will * also check that the remote parent node is available. > + */ > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > + SUN6I_CSI_PORT_ISP, 0, > + FWNODE_GRAPH_ENDPOINT_NEXT); > + if (!handle) > + return 0; > + > + fwnode_handle_put(handle); > + > + dev_info(dev, "ISP link is available\n"); You could make that a debug message, it's not crucial information that needs to be printed when the driver is loaded. If you prefer keeping an info message, then I'd move it to the probe function and print that the CSI has been probed, and indicate in that message if the ISP is available. > + csi_dev->isp_available = true; > + > + return 0; > +} > + > /* Media */ > > static const struct media_device_ops sun6i_csi_media_ops = { > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > if (ret) > return ret; > > + ret = sun6i_csi_isp_detect(csi_dev); > + if (ret) > + goto error_resources; > + > ret = sun6i_csi_v4l2_setup(csi_dev); > if (ret) > goto error_resources; > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > index e611bdd6e9b2..8e232cd91ebe 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > @@ -21,6 +21,7 @@ > enum sun6i_csi_port { > SUN6I_CSI_PORT_PARALLEL = 0, > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > + SUN6I_CSI_PORT_ISP = 2, > }; > > struct sun6i_csi_buffer { > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > struct clk *clock_mod; > struct clk *clock_ram; > struct reset_control *reset; > + > + bool isp_available; > }; > > struct sun6i_csi_variant { -- Regards, Laurent Pinchart 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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 19BCCECAAD4 for ; Fri, 26 Aug 2022 22:41:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=84sF9vCMVpRm9SXTDvjqFo2YvZtGk75AZvXjdYw4HUM=; b=lYUHcJ1zKxI1Eo wasDTRdY0HiNjeJ0bRTRtvBLH+vd20PEtvV4/77WetFsNah1LCfXVEk/5J5JM/sZvA4nc3aYEmYsQ QDTAoS99Fh2zA5F2C1CD9Sg5i1DlIa4geEooS36i9c0PIrHxfQvi8FUgBuz9AFYc58BrXRNVMMxyk r0vA+mlgMBUcxPHbHiPYr4ZNf2w20usm4qG3DEiw12Y+HJ/dClLVDKGzLByxMcrU5JIQzNaYKgA0L 5jqbjAr5nFdprOZhJkgkvbXTI6u74J/ZMrV5o4NLMMiOFGLpjzX8DNyCQ5CHXbYXXUQfrIQ1oorh5 A8Ls+uscpoBxaX3nSucA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oRhzY-00ERyD-7M; Fri, 26 Aug 2022 22:39:56 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oRhzV-00ERxp-P8 for linux-arm-kernel@lists.infradead.org; Fri, 26 Aug 2022 22:39:55 +0000 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D925230B; Sat, 27 Aug 2022 00:39:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1661553592; bh=R8qukP2RffLfxf6EBUyFnNoBjpObxeBRg1U0SiCvU9E=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eYUKTzF6QSKxmelG0IGUX5fRApy+oFTa6yaxqckT7jFur3bMN2VPKUHjCfiA1D7Dl s7m6NBji3WfpvsbirYWASI+L4MTJBY7CcsEX7FwALMxXpad6hEN4t2JjLH+Zy5h+Hr W1y03MKpyhsE4sK5Wn/oU4StLWJ8WPIGG+scy2vo= Date: Sat, 27 Aug 2022 01:39:44 +0300 From: Laurent Pinchart To: Paul Kocialkowski Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Sakari Ailus , Hans Verkuil , Maxime Ripard , Thomas Petazzoni Subject: Re: [PATCH v6 5/6] media: sun6i-csi: Detect the availability of the ISP Message-ID: References: <20220826184144.605605-1-paul.kocialkowski@bootlin.com> <20220826184144.605605-6-paul.kocialkowski@bootlin.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220826184144.605605-6-paul.kocialkowski@bootlin.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220826_153953_993739_A2F37C60 X-CRM114-Status: GOOD ( 31.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Paul, Thank you for the patch. On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > Add a helper to detect whether the ISP is available and connected > and store the indication in a driver-wide variable. This sounds like it would be a global variable, while it's stored in the driver-specific device structure. > > Signed-off-by: Paul Kocialkowski > --- > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index 00521f966cee..b16166cba2ef 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -24,6 +24,35 @@ > #include "sun6i_csi_capture.h" > #include "sun6i_csi_reg.h" > > +/* ISP */ > + > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > +{ > + struct device *dev = csi_dev->dev; > + struct fwnode_handle *handle = NULL; No need to initialize this to NULL. > + > + /* ISP is not available if disabled in kernel config. */ > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > + return 0; Hmmm... The ISP driver may be disabled when compiling the sun6i-csi driver, but later enabled and deployed. Disabling ISP support silently like this could be confusing. Could it be better to move this check after the graph check, and print a warning message in this case ? > + > + /* > + * ISP is not available if not connected via fwnode graph. > + * This weill also check that the remote parent node is available. s/weill/will/ * ISP is not available if not connected via fwnode graph. This will * also check that the remote parent node is available. > + */ > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > + SUN6I_CSI_PORT_ISP, 0, > + FWNODE_GRAPH_ENDPOINT_NEXT); > + if (!handle) > + return 0; > + > + fwnode_handle_put(handle); > + > + dev_info(dev, "ISP link is available\n"); You could make that a debug message, it's not crucial information that needs to be printed when the driver is loaded. If you prefer keeping an info message, then I'd move it to the probe function and print that the CSI has been probed, and indicate in that message if the ISP is available. > + csi_dev->isp_available = true; > + > + return 0; > +} > + > /* Media */ > > static const struct media_device_ops sun6i_csi_media_ops = { > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > if (ret) > return ret; > > + ret = sun6i_csi_isp_detect(csi_dev); > + if (ret) > + goto error_resources; > + > ret = sun6i_csi_v4l2_setup(csi_dev); > if (ret) > goto error_resources; > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > index e611bdd6e9b2..8e232cd91ebe 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > @@ -21,6 +21,7 @@ > enum sun6i_csi_port { > SUN6I_CSI_PORT_PARALLEL = 0, > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > + SUN6I_CSI_PORT_ISP = 2, > }; > > struct sun6i_csi_buffer { > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > struct clk *clock_mod; > struct clk *clock_ram; > struct reset_control *reset; > + > + bool isp_available; > }; > > struct sun6i_csi_variant { -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel