Re: [PATCH RESEND v5 1/8] reset: Fix devm bulk optional exclusive control getter

From: Serge Semin <fancer.lancer@gmail.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>,
	Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Dmitry Osipenko <digetx@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	linux-clk@vger.kernel.org, linux-mips@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH RESEND v5 1/8] reset: Fix devm bulk optional exclusive control getter
Date: Wed, 29 Jun 2022 18:06:59 +0300	[thread overview]
Message-ID: <20220629150659.4mutbylylukmoduq@mobilestation> (raw)
In-Reply-To: <c6794e78b9425c84f090d3058b93319bbb17e10f.camel@pengutronix.de>

On Wed, Jun 29, 2022 at 04:02:43PM +0200, Philipp Zabel wrote:
> Hi Serge,
> 
> On Fr, 2022-06-24 at 17:18 +0300, Serge Semin wrote:
> > Most likely due to copy-paste mistake the device managed version of the
> > denoted reset control getter has been implemented with invalid semantic,
> > which can be immediately spotted by having "WARN_ON(shared && acquired)"
> > warning in the system log as soon as the method is called. Anyway let's
> > fix it by altering the boolean arguments passed to the
> > __devm_reset_control_bulk_get() method from
> > - shared = true, optional = false, acquired = true
> > to
> > + shared = false, optional = true, acquired = true
> > That's what they were supposed to be in the first place (see the non-devm
> > version of the same method: reset_control_bulk_get_optional_exclusive()).
> > 
> > Fixes: 48d71395896d ("reset: Add reset_control_bulk API")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > Changelog v4:
> > - This is a new patch added on v4 lap of the series.
> > ---
> >  include/linux/reset.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > index 8a21b5756c3e..514ddf003efc 100644
> > --- a/include/linux/reset.h
> > +++ b/include/linux/reset.h
> > @@ -731,7 +731,7 @@ static inline int __must_check
> >  devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs,
> >  					       struct reset_control_bulk_data *rstcs)
> >  {
> > -	return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true);
> > +	return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true);
> >  }
> 

> You are right, thank you. I'll take this as a sign that the internal
> functions should probably switch from collecting ever more boolean
> arguments to a flags parameter, similar to enum gpiod_flags.
> 
> I'll pick this up separately into the reset/fixes branch.

Ok. Thanks. Meanwhile could you please take a look at the rest of the
reset-related patches of the series? They concern the reset-control
functionality updates of the Baikal-T1 CCU common-clock driver.

[PATCH RESEND v5 6/8] clk: baikal-t1: Move reset-controls code into a dedicated module
Link: https://lore.kernel.org/linux-clk/20220624141853.7417-7-Sergey.Semin@baikalelectronics.ru/
[PATCH RESEND v5 7/8] clk: baikal-t1: Add DDR/PCIe directly controlled resets support
Link: https://lore.kernel.org/linux-clk/20220624141853.7417-8-Sergey.Semin@baikalelectronics.ru/

The functionality has been placed in the clocks subsystem because the
reset-control is tightly coupled with the clock-control part of CCU.
It's a normal approach for the SoC CCU/RCU devices, which normally
have both clocks and reset controls intermixed. Anyway the main goal
of the series was to add the directly controlled resets of the
DDR and PCIe SoC sub-domains provided in the framework of the patch
7/8. Please see the patches log for details.

-Sergey

> 
> regards
> Philipp