From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from metis.whiteo.stw.pengutronix.de (metis.whiteo.stw.pengutronix.de [185.203.201.7]) (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 0A3C42F7AC6 for ; Fri, 5 Sep 2025 15:30:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.203.201.7 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757086254; cv=none; b=mJpmAAZAwmMmFOu7fqEF3bU11Y3KY7PKLI8Z8Xp1EPMbec7Nk70Wx3G6Ec/LY1u2tAOi6w0Fh5XB2WXECoKcs8uM/1au8ylBSWT8cqbyIfuuTOziTV4JRsUZfr93c6GjEJlzrhFNU0+y2qrW4hkKpncJn66/hWDND53zNgtLgMg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757086254; c=relaxed/simple; bh=A4ic6ODTiBaDKNa1fC8D4vrvVB1rgld9zJXBeUf4mwc=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=a6PoDkZK6X/G1n/FjJnVXHIZrPjD0cpwzx+EdNxyVRrJATBo9a8M3jXzOiI2tTohUAHXVy59Qxs3WZ4AtezncHAP13MiB5ylYDmV1JQhml028nbn2nTeL3MhoEukvfUS8NZGiYFRTNfkOhpwETIyHNiCht8tiql6zkbkPJ3+tmY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de; spf=pass smtp.mailfrom=pengutronix.de; arc=none smtp.client-ip=185.203.201.7 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pengutronix.de Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.whiteo.stw.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1uuYOX-0004lC-50; Fri, 05 Sep 2025 17:30:33 +0200 Received: from lupine.office.stw.pengutronix.de ([2a0a:edc0:0:900:1d::4e] helo=lupine) by drehscheibe.grey.stw.pengutronix.de with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1uuYOV-004OU7-34; Fri, 05 Sep 2025 17:30:31 +0200 Received: from pza by lupine with local (Exim 4.96) (envelope-from ) id 1uuYOV-000YZb-2n; Fri, 05 Sep 2025 17:30:31 +0200 Message-ID: <7b610a06e17e1c816a0a760bb661dbfd20ec44f4.camel@pengutronix.de> Subject: Re: [PATCH 2/4] dmaengine: sh: rz-dmac: Use devm_add_action_or_reset() From: Philipp Zabel To: Tommaso Merciai Cc: tomm.merciai@gmail.com, linux-renesas-soc@vger.kernel.org, biju.das.jz@bp.renesas.com, Vinod Koul , Geert Uytterhoeven , Fabrizio Castro , Lad Prabhakar , Wolfram Sang , Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= , dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org Date: Fri, 05 Sep 2025 17:30:31 +0200 In-Reply-To: References: <20250905144427.1840684-1-tommaso.merciai.xr@bp.renesas.com> <20250905144427.1840684-3-tommaso.merciai.xr@bp.renesas.com> <1b2d2410dd9c300da1ffe015ed4835208416798b.camel@pengutronix.de> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.46.4-2 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.whiteo.stw.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org On Fr, 2025-09-05 at 17:22 +0200, Tommaso Merciai wrote: > Hi Philipp, > Thank you for your review! >=20 > On Fri, Sep 05, 2025 at 04:53:54PM +0200, Philipp Zabel wrote: > > On Fr, 2025-09-05 at 16:44 +0200, Tommaso Merciai wrote: > > > Slightly simplify rz_dmac_probe() by using devm_add_action_or_reset() > > > for reset cleanup. > > >=20 > > > Signed-off-by: Tommaso Merciai > > > --- > > > drivers/dma/sh/rz-dmac.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > >=20 > > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c > > > index 0b526cc4d24be..0bc11a6038383 100644 > > > --- a/drivers/dma/sh/rz-dmac.c > > > +++ b/drivers/dma/sh/rz-dmac.c > > > @@ -905,6 +905,11 @@ static int rz_dmac_parse_of(struct device *dev, = struct rz_dmac *dmac) > > > return rz_dmac_parse_of_icu(dev, dmac); > > > } > > > =20 > > > +static void rz_dmac_reset_control_assert(void *data) > > > +{ > > > + reset_control_assert(data); > > > +} > > > + > > > static int rz_dmac_probe(struct platform_device *pdev) > > > { > > > const char *irqname =3D "error"; > > > @@ -977,6 +982,12 @@ static int rz_dmac_probe(struct platform_device = *pdev) > > > if (ret) > > > goto err_pm_runtime_put; > > > =20 > > > + ret =3D devm_add_action_or_reset(&pdev->dev, > > > + rz_dmac_reset_control_assert, > > > + dmac->rstc); > > > + if (ret) > > > + goto err_pm_runtime_put; > > > + > > > for (i =3D 0; i < dmac->n_channels; i++) { > > > ret =3D rz_dmac_chan_probe(dmac, &dmac->channels[i], i); > > > if (ret < 0) > > > @@ -1031,7 +1042,6 @@ static int rz_dmac_probe(struct platform_device= *pdev) > > > channel->lmdesc.base_dma); > > > } > > > =20 > > > - reset_control_assert(dmac->rstc); > > > err_pm_runtime_put: > > > pm_runtime_put(&pdev->dev); > > > =20 > > > @@ -1053,7 +1063,6 @@ static void rz_dmac_remove(struct platform_devi= ce *pdev) > > > channel->lmdesc.base, > > > channel->lmdesc.base_dma); > > > } > > > - reset_control_assert(dmac->rstc); > >=20 > > This patch changes cleanup order by effectively moving the > > reset_control_assert() after pm_runtime_put(). The commit message does > > not explain that this is safe to do. >=20 > Agreed. Thanks. >=20 > >=20 > > If this is ok, I'd move the reset_control_assert() up before > > pm_runtime_enable/resume_and_get(). >=20 > You mean having in the end the following calls: >=20 > ... > dmac->rstc =3D devm_reset_control_array_get_optional_exclusive(&pdev->de= v); > if (IS_ERR(dmac->rstc)) > return dev_err_probe(&pdev->dev, PTR_ERR(dmac->rstc), > "failed to get resets\n"); >=20 > ret =3D reset_control_deassert(dmac->rstc); > if (ret) > return dev_err_probe(&pdev->dev, ret, > "failed to deassert resets\n"); >=20 > ret =3D devm_add_action_or_reset(&pdev->dev, > rz_dmac_reset_control_assert, > dmac->rstc); > if (ret) > return dev_err_probe(&pdev->dev, ret, > "failed to register reset cleanup action\n"); >=20 > ret =3D devm_pm_runtime_enable(&pdev->dev); > if (ret < 0) > return dev_err_probe(&pdev->dev, ret, > "Failed to enable runtime PM\n"); > ... >=20 > Right? Right. regards Philipp