From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 D1D27168BA for ; Fri, 30 Jun 2023 17:29:43 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6600,9927,10757"; a="365026637" X-IronPort-AV: E=Sophos;i="6.01,171,1684825200"; d="scan'208";a="365026637" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jun 2023 10:28:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10757"; a="862359352" X-IronPort-AV: E=Sophos;i="6.01,171,1684825200"; d="scan'208";a="862359352" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP; 30 Jun 2023 10:28:57 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1qFHvT-001AKL-1n; Fri, 30 Jun 2023 20:28:55 +0300 Date: Fri, 30 Jun 2023 20:28:55 +0300 From: Andy Shevchenko To: Shenwei Wang Cc: Linus Walleij , Bartosz Golaszewski , "linux-gpio@vger.kernel.org" , "imx@lists.linux.dev" , dl-linux-imx Subject: Re: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support Message-ID: References: <20230629191903.2423243-1-shenwei.wang@nxp.com> Precedence: bulk X-Mailing-List: imx@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo On Fri, Jun 30, 2023 at 05:01:10PM +0000, Shenwei Wang wrote: > > From: Andy Shevchenko > > Sent: Friday, June 30, 2023 4:19 AM ... > > > + ret = pm_runtime_get_sync(chip->parent); > > > > reference count disbalance here. > > Seems we shouldn't check the return value here and simply return 0. > Or should it be changed to " pm_runtime_resume_and_get" ? It depends. I don't know the goal and what the case will be if PM fails and you still go with that. > > > + return ret < 0 ? ret : 0; ... > > But here is the question: does your controller support wake from IRQ? > > > > Have you tried to see if the lines that are used for IRQ with > > gpiod_to_irq() really work with this? > > Yes, the controller supports wake from IRQ. This patch has been > Verified with the SDCARD plug-in/out use case which use a GPIO line as CD PIN. Yes, but in that case it has been probably requested. What I'm telling is when the GPIO IRQ is went via IRQ chip and hence never been requested by GPIO. ... > > > + err = pm_runtime_get_sync(&pdev->dev); > > > + if (err < 0) > > > > reference count leak here. > > Change to pm_runtime_resume_and_get? No idea. You know it better than me. See above. > > > + goto out_pm_dis; ... > > Personal view on this is that it makes little sense to do and is prone to > > subtle bugs with wake sources or other, not so obvious, uses of the GPIO > > lines. Can you provide the numbers of the current dissipation if the > > controller is on and no line is requested? Also is there any real example > > of hardware (existing DTS) that has no GPIO in use? > > While putting the GPIO module itself into power saving mode may not have an > obvious impact on current dissipation, the function is necessary because the > GPIO module disables its clock when idle. This enables the system an > opportunity to power off the parent subsystem, and this conserves more power. > The typical i.MX8 SoC features up to 8 GPIO controllers, but most of the > controllers often remain unused. Maybe you need to summarize above in the commit message to improve your selling point. -- With Best Regards, Andy Shevchenko