Skip site navigation (1)Skip section navigation (2)
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 (&regnode->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>