Date: Fri, 15 Jun 2018 08:13:06 +0200 From: Michal Meloun <melounmichal@gmail.com> To: Kyle Evans <kevans@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r335173 - head/sys/dev/extres/regulator Message-ID: <ea2e4ada-bf7e-c395-7e01-afa482425f5a@freebsd.org> In-Reply-To: <201806142037.w5EKbP6S096772@repo.freebsd.org> References: <201806142037.w5EKbP6S096772@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 14.06.2018 22:37, Kyle Evans wrote: > Author: kevans > Date: Thu Jun 14 20:37:25 2018 > New Revision: 335173 > URL: https://svnweb.freebsd.org/changeset/base/335173 > > Log: > extres/regulator: Properly refcount gpio regulators > > regnode::enable_cnt is generally used to refcount regulator nodes. For > GPIOs, the refcount was done on the gpio_entry since more than one regulator > can share a GPIO. > > GPIO regulators were not taking part in the node refcount, since they had > their own mechanism. This caused some fallout after manu started disabling > everybody's unused regulators in r331989. > > Refcount it. This is, imho, wrong solution. The regnode::enable_cnt counts number of enable request from consumer and regulator framework calls regnode_enable() method only when first consumer wants enable it or last consumer disable it. regnode::enable_cnt doesn't reflect actual status (enabled/disabled) of given regulator. The gpio_entry::enable_cnt track number of enabled regulators on same gpio line and is unrelated to sum of regnode::enable_cnt or actual state of enable pin. Just look whats happen for regulator without boot_on or always_on property after standard regulator_enable() then regulator_disable() sequence. For regulator_enable(), the framework increments regnode::enable_cnt then invoke regnode_enable(..., true, ...) (because regnode::enable_cnt was zero). The regnode_enable() also increments regnode::enable_cnt. So final value of counter is 2. Subsequent regulator_disable() decrement regnode::enable_cnt and because it is not zero, it returns without any action. The real bug is that regulator framework transforms regulator_stop() (not refcounting method) to regnode_enable(..., false, ...) (refcounting method) and thus regnode_fixed_enable() cannot handle gpio_entry::enable_cnt properly. Can you, please, revert this? > > Glanced over by: manu > > Modified: > head/sys/dev/extres/regulator/regulator.c > head/sys/dev/extres/regulator/regulator.h > head/sys/dev/extres/regulator/regulator_fixed.c > > Modified: head/sys/dev/extres/regulator/regulator.c > ============================================================================== > --- head/sys/dev/extres/regulator/regulator.c Thu Jun 14 20:36:55 2018 (r335172) > +++ head/sys/dev/extres/regulator/regulator.c Thu Jun 14 20:37:25 2018 (r335173) > @@ -507,6 +507,20 @@ struct regnode_std_param *regnode_get_stdparam(struct > return (®node->std_param); > } > > +void > +regnode_enable_cnt_inc(struct regnode *regnode) > +{ > + > + regnode->enable_cnt++; > +} > + > +void > +regnode_enable_cnt_dec(struct regnode *regnode) > +{ > + > + regnode->enable_cnt--; > +} > + > void regnode_topo_unlock(void) > { > > > Modified: head/sys/dev/extres/regulator/regulator.h > ============================================================================== > --- head/sys/dev/extres/regulator/regulator.h Thu Jun 14 20:36:55 2018 (r335172) > +++ head/sys/dev/extres/regulator/regulator.h Thu Jun 14 20:37:25 2018 (r335173) > @@ -106,6 +106,8 @@ int regnode_get_flags(struct regnode *regnode); > void *regnode_get_softc(struct regnode *regnode); > device_t regnode_get_device(struct regnode *regnode); > struct regnode_std_param *regnode_get_stdparam(struct regnode *regnode); > +void regnode_enable_cnt_inc(struct regnode *regnode); > +void regnode_enable_cnt_dec(struct regnode *regnode); > void regnode_topo_unlock(void); > void regnode_topo_xlock(void); > void regnode_topo_slock(void); > > Modified: head/sys/dev/extres/regulator/regulator_fixed.c > ============================================================================== > --- head/sys/dev/extres/regulator/regulator_fixed.c Thu Jun 14 20:36:55 2018 (r335172) > +++ head/sys/dev/extres/regulator/regulator_fixed.c Thu Jun 14 20:37:25 2018 (r335173) > @@ -156,6 +156,8 @@ regnode_fixed_init(struct regnode *regnode) > if (sc->gpio_open_drain) > flags |= GPIO_PIN_OPENDRAIN; > enable = sc->param->boot_on || sc->param->always_on; > + if (enable) > + regnode_enable_cnt_inc(regnode); > if (!sc->param->enable_active_high) > enable = !enable; > rv = GPIO_PIN_SET(pin->dev, pin->pin, enable); > @@ -194,12 +196,14 @@ regnode_fixed_enable(struct regnode *regnode, bool ena > return (0); > pin = &sc->gpio_entry->gpio_pin; > if (enable) { > + regnode_enable_cnt_inc(regnode); > sc->gpio_entry->enable_cnt++; > if (sc->gpio_entry->enable_cnt > 1) > return (0); > } else { > KASSERT(sc->gpio_entry->enable_cnt > 0, > ("Invalid enable count")); > + regnode_enable_cnt_dec(regnode); > sc->gpio_entry->enable_cnt--; > if (sc->gpio_entry->enable_cnt >= 1) > return (0); >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?ea2e4ada-bf7e-c395-7e01-afa482425f5a>