Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter

From: Philipp Zabel <p.zabel@pengutronix.de>
To: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Rob Herring <robh+dt@kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Magnus Damm <magnus.damm@gmail.com>,
	Alexandru Ardelean <aardelean@deviqon.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH v3 2/3] iio: adc: Add driver for Renesas RZ/G2L A/D converter
Date: Tue, 27 Jul 2021 10:13:25 +0200	[thread overview]
Message-ID: <145309b88353d4127c659dfabd374252cb2afc48.camel@pengutronix.de> (raw)
In-Reply-To: <CA+V-a8shgfxffdOTj0cyxz36XVxGxUkq1obPJNOSc94BKUWung@mail.gmail.com>

On Tue, 2021-07-27 at 09:02 +0100, Lad, Prabhakar wrote:
[...]
> > > +     ret = devm_add_action_or_reset(&pdev->dev,
> > > +                                    rzg2l_adc_reset_assert, adc->adrstn);
> > > +     if (ret) {
> > > +             dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n",
> > > +                     ret);
> > > +             return ret;
> > > +     }
> > 
> > This is the wrong way around. Installing devres actions should be done
> > after the thing they are supposed to revert in case of error. You should
> > move this down below the reset_control_deassert(adc->adrstn).
> > 
> Ouch my understanding was, there won't be any harm in asserting the
> reset line. Agree with will move this below
> reset_control_deassert(adc->adrstn).

You are probably right, but it's still better do it correctly. Just
imagine one of the reset lines turns out to be shared later, or somebody
else will look at this driver for inspiration.

regards
Philipp