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);
>
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
