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 5C564ECAAD1 for ; Thu, 1 Sep 2022 18:18:48 +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=+6m5stGeLoXqwoKfDU6E5uJher86cZk32ALdgtExhcc=; b=CQozZQHfckILP/ yQOJLen+WW2oBg7F6as9oGcI6x0w9sJd0L8eJyTDtEyb0ii6hSJTolY7Pr233v4MOTQ++OjcpmqL4 WswDM+AHXEwEfFML3K2Y5s4h+LW8vQ3hvY0gsuh0n2BvEVLRjee1MvQ7l9YsBPHfG2WQaYFfpv6Bd 1q8j6jOX5kFd4kFoeA4pKj51ICbO+/M1e0wJyChfZp+7ccw4vLK/SwKgq7j7HOf2KeSrc+zyaVD7+ Dv/+MrB7MtNcs4anD9KpWk5/JreXepoGECsmOtGfn3K1mM+S4UGB/wQSPG7Uu8Kj5Asyp8IHaN+/B tq+oa9DOU3yvFH2M0OeQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oToky-00DsaH-7I; Thu, 01 Sep 2022 18:17:37 +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 1oToiU-00DrJi-DE for linux-arm-kernel@lists.infradead.org; Thu, 01 Sep 2022 18:15:04 +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 96F5C6CD; Thu, 1 Sep 2022 20:15:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1662056100; bh=zRBz0GUyRGTxcADlcmJSiGE78jaeWQ5MWIZCKWdpg9o=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=uxeZCtoherm0RTZ5pf1+4qwa+MUrepmLXoCB4lbxK8Wqftsdg9ZErsiZmXcfidAlt mqrkIYfjgKajGwXoC067yGf7ao9xRIZURJHkRYGU8gsr3OXozm8XCPbR/TQsjb8r4L rS9b0Eud84d3MrbXXnsgu3/JQW+JYTnPbNaJptgo= Date: Thu, 1 Sep 2022 21:14:49 +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: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220901_111502_703554_C022755F X-CRM114-Status: GOOD ( 45.00 ) 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, On Thu, Sep 01, 2022 at 05:11:00PM +0200, Paul Kocialkowski wrote: > On Sat 27 Aug 22, 01:39, Laurent Pinchart wrote: > > 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. > > Okay I can clarify the commit message here. > > > > > > > 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. > > Indeed. > > > > + > > > + /* 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 ? > > Yeah I'm not too surprised corner cases like this can exist. > Agreed that printing a warning message would be good, but I don't follow the > point of moving the check later on. Do you have something in mind there? I meant that the warning should only be printed of there's an endpoint connected to the ISP, so I'd first check if the endpoint is present, return 0 if it isn't, and then check if the ISP driver is enabled and print a warning if it isn't. > > > + > > > + /* > > > + * ISP is not available if not connected via fwnode graph. > > > + * This weill also check that the remote parent node is available. > > > > s/weill/will/ > > Good catch, thanks! > > > * 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. > > You're right, let's make this debug. It's more the opposite case that is worth > a warning message. > > > > + 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