Hi Svyatoslav,
On Thu, 20 Jul 2023 at 23:23, Svyatoslav Ryhel <clamo...@gmail.com> wrote:
>
> чт, 20 лип. 2023 р. о 22:43 Simon Glass <s...@chromium.org> пише:
> >
> > Hi Svyatoslav,
> >
> > On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel <clamo...@gmail.com> wrote:
> > >
> > > Commit is based on 4fcba5d ("regulator: implement basic reference
> > > counter") but expands the idea to all regulators instead of just
> > > fixed/gpio regulators.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamo...@gmail.com>
> > > ---
> > > drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++
> > > drivers/power/regulator/regulator_common.c | 22 ------------
> > > drivers/power/regulator/regulator_common.h | 21 -----------
> > > include/power/regulator.h | 2 ++
> > > 4 files changed, 43 insertions(+), 43 deletions(-)
> >
> > Reviewed-by: Simon Glass <s...@chromium.org>
> >
> > nit below
> >
> > >
> > > diff --git a/drivers/power/regulator/regulator-uclass.c
> > > b/drivers/power/regulator/regulator-uclass.c
> > > index 3a6ba69f6d..fc7a4631b4 100644
> > > --- a/drivers/power/regulator/regulator-uclass.c
> > > +++ b/drivers/power/regulator/regulator-uclass.c
> > > @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev)
> > > return ops->get_enable(dev);
> > > }
> > >
> > > +/*
> > > + * Enable or Disable a regulator
> > > + *
> > > + * This is a reentrant function and subsequent calls that enable will
> > > + * increase an internal counter, and disable calls will decrease the
> > > counter.
> > > + * The actual resource will be enabled when the counter gets to 1 coming
> > > from 0,
> > > + * and disabled when it reaches 0 coming from 1.
> > > + *
> > > + * @dev: regulator device
> > > + * @enable: bool indicating whether to enable or disable the regulator
> > > + * @return:
> > > + * 0 on Success
> > > + * -EBUSY if the regulator cannot be disabled because it's requested by
> > > + * another device
> > > + * -EALREADY if the regulator has already been enabled or has already
> > > been
> > > + * disabled
> > > + * -EACCES if there is no possibility to enable/disable the regulator
> > > + * -ve on different error situation
> > > + */
> > > int regulator_set_enable(struct udevice *dev, bool enable)
> > > {
> > > const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
> > > @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool
> > > enable)
> > > if (!enable && uc_pdata->always_on)
> > > return -EACCES;
> > >
> > > + /* If previously enabled, increase count */
> > > + if (enable && uc_pdata->enable_count > 0) {
> > > + uc_pdata->enable_count++;
> > > + return -EALREADY;
> > > + }
> > > +
> > > + if (!enable) {
> > > + if (uc_pdata->enable_count > 1) {
> > > + /* If enabled multiple times, decrease count */
> > > + uc_pdata->enable_count--;
> > > + return -EBUSY;
> > > + } else if (!uc_pdata->enable_count) {
> > > + /* If already disabled, do nothing */
> > > + return -EALREADY;
> > > + }
> > > + }
> > > +
> > > if (uc_pdata->ramp_delay)
> > > old_enable = regulator_get_enable(dev);
> > >
> > > @@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool
> > > enable)
> > > }
> > > }
> > >
> > > + if (enable)
> > > + uc_pdata->enable_count++;
> > > + else
> > > + uc_pdata->enable_count--;
> > > +
> > > return ret;
> > > }
> > >
> > > diff --git a/drivers/power/regulator/regulator_common.c
> > > b/drivers/power/regulator/regulator_common.c
> > > index e26f5ebec3..d88bc6f6de 100644
> > > --- a/drivers/power/regulator/regulator_common.c
> > > +++ b/drivers/power/regulator/regulator_common.c
> > > @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice
> > > *dev,
> > > return 0;
> > > }
> > >
> > > - /* If previously enabled, increase count */
> > > - if (enable && plat->enable_count > 0) {
> > > - plat->enable_count++;
> > > - return -EALREADY;
> > > - }
> > > -
> > > - if (!enable) {
> > > - if (plat->enable_count > 1) {
> > > - /* If enabled multiple times, decrease count */
> > > - plat->enable_count--;
> > > - return -EBUSY;
> > > - } else if (!plat->enable_count) {
> > > - /* If already disabled, do nothing */
> > > - return -EALREADY;
> > > - }
> > > - }
> > > -
> > > ret = dm_gpio_set_value(&plat->gpio, enable);
> > > if (ret) {
> > > pr_err("Can't set regulator : %s gpio to: %d\n",
> > > dev->name,
> > > @@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice
> > > *dev,
> > > if (!enable && plat->off_on_delay_us)
> > > udelay(plat->off_on_delay_us);
> > >
> > > - if (enable)
> > > - plat->enable_count++;
> > > - else
> > > - plat->enable_count--;
> > > -
> > > return 0;
> > > }
> > > diff --git a/drivers/power/regulator/regulator_common.h
> > > b/drivers/power/regulator/regulator_common.h
> > > index d4962899d8..15f1fa4c93 100644
> > > --- a/drivers/power/regulator/regulator_common.h
> > > +++ b/drivers/power/regulator/regulator_common.h
> > > @@ -13,7 +13,6 @@ struct regulator_common_plat {
> > > struct gpio_desc gpio; /* GPIO for regulator enable control */
> > > unsigned int startup_delay_us;
> > > unsigned int off_on_delay_us;
> > > - unsigned int enable_count;
> > > };
> > >
> > > int regulator_common_of_to_plat(struct udevice *dev,
> > > @@ -21,26 +20,6 @@ int regulator_common_of_to_plat(struct udevice *dev,
> > > char *enable_gpio_name);
> > > int regulator_common_get_enable(const struct udevice *dev,
> > > struct regulator_common_plat *plat);
> > > -/*
> > > - * Enable or Disable a regulator
> > > - *
> > > - * This is a reentrant function and subsequent calls that enable will
> > > - * increase an internal counter, and disable calls will decrease the
> > > counter.
> > > - * The actual resource will be enabled when the counter gets to 1 coming
> > > from 0,
> > > - * and disabled when it reaches 0 coming from 1.
> > > - *
> > > - * @dev: regulator device
> > > - * @plat: Platform data
> > > - * @enable: bool indicating whether to enable or disable the regulator
> > > - * @return:
> > > - * 0 on Success
> > > - * -EBUSY if the regulator cannot be disabled because it's requested by
> > > - * another device
> > > - * -EALREADY if the regulator has already been enabled or has already
> > > been
> > > - * disabled
> > > - * -EACCES if there is no possibility to enable/disable the regulator
> > > - * -ve on different error situation
> > > - */
> > > int regulator_common_set_enable(const struct udevice *dev,
> > > struct regulator_common_plat *plat, bool enable);
> > >
> > > diff --git a/include/power/regulator.h b/include/power/regulator.h
> > > index ff1bfc2435..727776a8cf 100644
> > > --- a/include/power/regulator.h
> > > +++ b/include/power/regulator.h
> > > @@ -158,6 +158,7 @@ enum regulator_flag {
> > > * @name** - fdt regulator name - should be taken from the device
> > > tree
> > > * ctrl_reg: - Control register offset used to enable/disable regulator
> > > * volt_reg: - register offset for writing voltage vsel values
> > > + * enable_count - counter of enable calls for this regulator
> > > *
> > > * Note:
> > > * * - set automatically on device probe by the uclass's '.pre_probe'
> > > method.
> > > @@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat {
> > > u8 volt_reg;
> > > bool suspend_on;
> > > u32 suspend_uV;
> > > + u32 enable_count;
> >
> > Please add a comment for this
> >
>
> Description included in struct description here:
> + * enable_count - counter of enable calls for this regulator
>
OK thanks, I wonder why it says it is part of 'enum regulator_flag' ?
Regards,
Simon
