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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D9E81C433F5 for ; Fri, 11 Mar 2022 17:00:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1350412AbiCKRBy (ORCPT ); Fri, 11 Mar 2022 12:01:54 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35754 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237446AbiCKRBy (ORCPT ); Fri, 11 Mar 2022 12:01:54 -0500 Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::228]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1F4414EA19 for ; Fri, 11 Mar 2022 09:00:49 -0800 (PST) Received: (Authenticated sender: jacopo@jmondi.org) by mail.gandi.net (Postfix) with ESMTPSA id 3D0E81BF205; Fri, 11 Mar 2022 17:00:46 +0000 (UTC) Date: Fri, 11 Mar 2022 18:00:45 +0100 From: Jacopo Mondi To: Laurent Pinchart Cc: linux-media@vger.kernel.org, Rui Miguel Silva , kernel@pengutronix.de, linux-imx@nxp.com, Paul Elder , Sakari Ailus Subject: Re: [PATCH 4/4] media: imx: imx-mipi-csis: Simplify runtime PM implementation Message-ID: <20220311170045.k2dmhbwupfjqws22@uno.localdomain> References: <20220311135535.30108-1-laurent.pinchart@ideasonboard.com> <20220311135535.30108-5-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220311135535.30108-5-laurent.pinchart@ideasonboard.com> Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Laurent, On Fri, Mar 11, 2022 at 03:55:35PM +0200, Laurent Pinchart wrote: > The runtime PM resume handler is guaranteed to be called on a suspended > device, and the suspend handler on a resumed device. The implementation > can thus be simplified. > > While at it, rename the mipi_csis_device state field to powered, as the > now state contains a single flag only. Can we instead rely on pm_runtime_get_if_in_use() instead of manual tracking the power state ? After all, the powered flag is only used in: static int mipi_csis_log_status(struct v4l2_subdev *sd) { struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd); mutex_lock(&csis->lock); mipi_csis_log_counters(csis, true); if (csis->debug.enable && csis->powered) mipi_csis_dump_regs(csis); mutex_unlock(&csis->lock); return 0; } which could be simplified as static int mipi_csis_log_status(struct v4l2_subdev *sd) { struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd); mipi_csis_log_counters(csis, true); if (!csis->debug.enable) return 0; mutex_lock(&csis->lock); if (!pm_runtime_get_if_in_use()) goto unlock; mipi_csis_dump_regs(csis); pm_runtime_put(); unlock: mutex_unlock(&csis->lock); return 0; } Thanks j > > Signed-off-by: Laurent Pinchart > --- > drivers/media/platform/imx/imx-mipi-csis.c | 38 ++++++++++------------ > 1 file changed, 17 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > index d656b8bfcc33..f6ff8d50843c 100644 > --- a/drivers/media/platform/imx/imx-mipi-csis.c > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > @@ -248,10 +248,6 @@ > #define MIPI_CSI2_DATA_TYPE_RAW14 0x2d > #define MIPI_CSI2_DATA_TYPE_USER(x) (0x30 + (x)) > > -enum { > - ST_POWERED = 1, > -}; > - > struct mipi_csis_event { > bool debug; > u32 mask; > @@ -331,10 +327,10 @@ struct mipi_csis_device { > u32 hs_settle; > u32 clk_settle; > > - struct mutex lock; /* Protect csis_fmt, format_mbus and state */ > + struct mutex lock; /* Protect csis_fmt, format_mbus and powered */ > const struct csis_pix_format *csis_fmt; > struct v4l2_mbus_framefmt format_mbus[CSIS_PADS_NUM]; > - u32 state; > + bool powered; > > spinlock_t slock; /* Protect events */ > struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS]; > @@ -1193,7 +1189,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd) > > mutex_lock(&csis->lock); > mipi_csis_log_counters(csis, true); > - if (csis->debug.enable && (csis->state & ST_POWERED)) > + if (csis->debug.enable && csis->powered) > mipi_csis_dump_regs(csis); > mutex_unlock(&csis->lock); > > @@ -1354,13 +1350,14 @@ static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev) > int ret = 0; > > mutex_lock(&csis->lock); > - if (csis->state & ST_POWERED) { > - ret = mipi_csis_phy_disable(csis); > - if (ret) > - goto unlock; > - mipi_csis_clk_disable(csis); > - csis->state &= ~ST_POWERED; > - } > + > + ret = mipi_csis_phy_disable(csis); > + if (ret) > + goto unlock; > + > + mipi_csis_clk_disable(csis); > + > + csis->powered = false; > > unlock: > mutex_unlock(&csis->lock); > @@ -1376,14 +1373,13 @@ static int __maybe_unused mipi_csis_runtime_resume(struct device *dev) > > mutex_lock(&csis->lock); > > - if (!(csis->state & ST_POWERED)) { > - ret = mipi_csis_phy_enable(csis); > - if (ret) > - goto unlock; > + ret = mipi_csis_phy_enable(csis); > + if (ret) > + goto unlock; > > - csis->state |= ST_POWERED; > - mipi_csis_clk_enable(csis); > - } > + mipi_csis_clk_enable(csis); > + > + csis->powered = true; > > unlock: > mutex_unlock(&csis->lock); > -- > Regards, > > Laurent Pinchart >