Re: [PATCH 9/9] remoteproc/wkup_m3: Use reset control driver if available

From: Tony Lindgren <tony@atomide.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: linux-omap@vger.kernel.org, "Andrew F . Davis" <afd@ti.com>,
	Dave Gerlach <d-gerlach@ti.com>, Faiz Abbas <faiz_abbas@ti.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	Keerthy <j-keerthy@ti.com>, Nishanth Menon <nm@ti.com>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Roger Quadros <rogerq@ti.com>, Suman Anna <s-anna@ti.com>,
	Tero Kristo <t-kristo@ti.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-remoteproc@vger.kernel.org,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Santosh Shilimkar <ssantosh@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	linux-clk@vger.kernel.org
Subject: Re: [PATCH 9/9] remoteproc/wkup_m3: Use reset control driver if available
Date: Mon, 26 Oct 2020 15:02:38 +0200	[thread overview]
Message-ID: <20201026130238.GA5639@atomide.com> (raw)
In-Reply-To: <c8ea80c2eb79f80539911f3563398957beedaa41.camel@pengutronix.de>

Hi,

* Philipp Zabel <p.zabel@pengutronix.de> [201026 11:35]:
> On Mon, 2020-10-26 at 13:10 +0200, Tony Lindgren wrote:
> > @@ -57,6 +60,9 @@ static int wkup_m3_rproc_start(struct rproc *rproc)
> >  	struct device *dev = &pdev->dev;
> >  	struct wkup_m3_platform_data *pdata = dev_get_platdata(dev);
> >  
> > +	if (wkupm3->rsts)
> 
> No need for this check, reset_control_deassert() just returns 0 if the
> rstc parameter is NULL.
> 
> > +		return reset_control_deassert(wkupm3->rsts);
> > +
> >  	if (pdata->deassert_reset(pdev, pdata->reset_name)) {
> >  		dev_err(dev, "Unable to reset wkup_m3!\n");
> >  		return -ENODEV;
> > @@ -72,6 +78,9 @@ static int wkup_m3_rproc_stop(struct rproc *rproc)
> >  	struct device *dev = &pdev->dev;
> >  	struct wkup_m3_platform_data *pdata = dev_get_platdata(dev);
> >  
> > +	if (wkupm3->rsts)
> 
> Same as above.

OK great.

> > +		return reset_control_assert(wkupm3->rsts);
> > +
> >  	if (pdata->assert_reset(pdev, pdata->reset_name)) {
> >  		dev_err(dev, "Unable to assert reset of wkup_m3!\n");
> >  		return -ENODEV;
> > @@ -132,12 +141,6 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
> >  	int ret;
> >  	int i;
> >  
> > -	if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
> > -	      pdata->reset_name)) {
> > -		dev_err(dev, "Platform data missing!\n");
> > -		return -ENODEV;
> > -	}
> > -
> >  	ret = of_property_read_string(dev->of_node, "ti,pm-firmware",
> >  				      &fw_name);
> >  	if (ret) {
> > @@ -165,6 +168,17 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
> >  	wkupm3->rproc = rproc;
> >  	wkupm3->pdev = pdev;
> >  
> > +	wkupm3->rsts = devm_reset_control_get_optional_shared(dev, "rstctrl");
> > +	if (PTR_ERR_OR_ZERO(wkupm3->rsts)) {
> 
> Please properly return errors. rsts will be NULL if the optional rstctrl
> reset is not specified:
> 
> 	if (IS_ERR(wkump3->rsts))
> 		return PTR_ERR(wkump3->rsts);

OK thanks will do.

> 	if (!wkump3->rsts) {
> > +		if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
> > +		      pdata->reset_name)) {
> > +			dev_err(dev, "Platform data missing!\n");
> > +			ret = -ENODEV;
> > +			goto err_put_rproc;
> > +		}
> > +		wkupm3->rsts = NULL;
> 
> I assume this will later be dropped with the platform data support?

Yes once also am437x has been updated to probe with dts data only we
can drop the custom callbacks.

Regards,

Tony