[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Sven Schwermer]

Fixed regulators don't have a set_value method. Therefore,
regulator_set_value will return -ENOSYS when called from
regulator_autoset.

Accepting this return value allows autosetting fixed regulators.

Signed-off-by: Sven Schwermer <sven@svenschwermer.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Peng Fan <peng.fan@nxp.com>
---
 drivers/power/regulator/regulator-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
index 9118b8eb39..0b99c262ac 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -243,7 +243,7 @@ int regulator_autoset(struct udevice *dev)
 	if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
 		ret = regulator_set_current(dev, uc_pdata->min_uA);
 
-	if (!ret)
+	if (!ret || ret == -ENOSYS)
 		ret = regulator_set_enable(dev, true);
 
 	return ret;
-- 
2.17.1

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Peng Fan]

Hi Sven,

> Subject: [PATCH] regulator: Allow autosetting fixed regulators
> 
> Fixed regulators don't have a set_value method. Therefore,
> regulator_set_value will return -ENOSYS when called from regulator_autoset.
> 
> Accepting this return value allows autosetting fixed regulators.
> 
> Signed-off-by: Sven Schwermer <sven@svenschwermer.de>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/power/regulator/regulator-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/regulator/regulator-uclass.c
> b/drivers/power/regulator/regulator-uclass.c
> index 9118b8eb39..0b99c262ac 100644
> --- a/drivers/power/regulator/regulator-uclass.c
> +++ b/drivers/power/regulator/regulator-uclass.c
> @@ -243,7 +243,7 @@ int regulator_autoset(struct udevice *dev)
>  	if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
>  		ret = regulator_set_current(dev, uc_pdata->min_uA);
> 
> -	if (!ret)
> +	if (!ret || ret == -ENOSYS)


According to code:
        if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
                ret = regulator_set_value(dev, uc_pdata->min_uV);
        if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
                ret = regulator_set_current(dev, uc_pdata->min_uA);

So you get -ENOSYS from the upper code?

Regards,
Peng.

>  		ret = regulator_set_enable(dev, true);
> 
>  	return ret;
> --
> 2.17.1

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Sven Schwermer]

Hi Peng,

> According to code:
>        if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
>                ret = regulator_set_value(dev, uc_pdata->min_uV);
>        if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
>                ret = regulator_set_current(dev, uc_pdata->min_uA);
> 
> So you get -ENOSYS from the upper code?

Yes, for fixed regulators, the following if clause evaluates to true in regulator_pre_probe:

	/* Those values are optional (-ENODATA if unset) */
	if ((uc_pdata->min_uV != -ENODATA) &&
	    (uc_pdata->max_uV != -ENODATA) &&
	    (uc_pdata->min_uV == uc_pdata->max_uV))
		uc_pdata->flags |= REGULATOR_FLAG_AUTOSET_UV;

However, in regulator_set_value, there is this section:

	if (!ops || !ops->set_value)
		return -ENOSYS;

So for fixed regulators, which don’t have a set_value, we’ll always get a -ENOSYS.

Sven

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Peng Fan]

> Subject: Re: [PATCH] regulator: Allow autosetting fixed regulators
> 
> Hi Peng,
> 
> > According to code:
> >        if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
> >                ret = regulator_set_value(dev, uc_pdata->min_uV);
> >        if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
> >                ret = regulator_set_current(dev, uc_pdata->min_uA);
> >
> > So you get -ENOSYS from the upper code?
> 
> Yes, for fixed regulators, the following if clause evaluates to true in
> regulator_pre_probe:
> 
> 	/* Those values are optional (-ENODATA if unset) */
> 	if ((uc_pdata->min_uV != -ENODATA) &&
> 	    (uc_pdata->max_uV != -ENODATA) &&
> 	    (uc_pdata->min_uV == uc_pdata->max_uV))
> 		uc_pdata->flags |= REGULATOR_FLAG_AUTOSET_UV;

For fixed regulator, will min_uV/max_uV be set to value other
than -ENODATA? I think no.

Regards,
Peng.

> 
> However, in regulator_set_value, there is this section:
> 
> 	if (!ops || !ops->set_value)
> 		return -ENOSYS;
> 
> So for fixed regulators, which don’t have a set_value, we’ll always get a
> -ENOSYS.
> 
> Sven

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Sven Schwermer]

> For fixed regulator, will min_uV/max_uV be set to value other
> than -ENODATA? I think no.

I see this commonly done (see Linux device trees as well as the u-boot device trees) and the kernel documentation seems to encourage it:
https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/fixed-regulator.txt <https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/fixed-regulator.txt>

Sven

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Peng Fan]

> Subject: [PATCH] regulator: Allow autosetting fixed regulators
> 
> Fixed regulators don't have a set_value method. Therefore,
> regulator_set_value will return -ENOSYS when called from regulator_autoset.
> 
> Accepting this return value allows autosetting fixed regulators.
> 
> Signed-off-by: Sven Schwermer <sven@svenschwermer.de>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/power/regulator/regulator-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/regulator/regulator-uclass.c
> b/drivers/power/regulator/regulator-uclass.c
> index 9118b8eb39..0b99c262ac 100644
> --- a/drivers/power/regulator/regulator-uclass.c
> +++ b/drivers/power/regulator/regulator-uclass.c
> @@ -243,7 +243,7 @@ int regulator_autoset(struct udevice *dev)
>  	if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
>  		ret = regulator_set_current(dev, uc_pdata->min_uA);
> 
> -	if (!ret)
> +	if (!ret || ret == -ENOSYS)
>  		ret = regulator_set_enable(dev, true);

How about the following patch? not tested

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
index 9118b8eb39..76be95bcd1 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -238,6 +238,9 @@ int regulator_autoset(struct udevice *dev)
        if (!uc_pdata->always_on && !uc_pdata->boot_on)
                return -EMEDIUMTYPE;

+       if (uc_pdata->type == REGULATOR_TYPE_FIXED)
+               return regulator_set_enable(dev, true);
+
        if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
                ret = regulator_set_value(dev, uc_pdata->min_uV);
        if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))

Regards,
Peng.

> 
>  	return ret;
> --
> 2.17.1

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Sven Schwermer]

> How about the following patch? not tested
> 
> diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
> index 9118b8eb39..76be95bcd1 100644
> --- a/drivers/power/regulator/regulator-uclass.c
> +++ b/drivers/power/regulator/regulator-uclass.c
> @@ -238,6 +238,9 @@ int regulator_autoset(struct udevice *dev)
>        if (!uc_pdata->always_on && !uc_pdata->boot_on)
>                return -EMEDIUMTYPE;
> 
> +       if (uc_pdata->type == REGULATOR_TYPE_FIXED)
> +               return regulator_set_enable(dev, true);
> +
>        if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
>                ret = regulator_set_value(dev, uc_pdata->min_uV);
>        if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))

That will probably work as well. It does, however, feel a little like a hack. What if there are other regulator types that require a similar workaround?

Sven

[U-Boot] [PATCH] regulator: Allow autosetting fixed regulators

[Peng Fan]

> -----Original Message-----
> From: Sven Schwermer [mailto:sven at svenschwermer.de]
> Sent: 2019年5月31日 18:44
> To: Peng Fan <peng.fan@nxp.com>
> Cc: u-boot at lists.denx.de; Jaehoon Chung <jh80.chung@samsung.com>
> Subject: Re: [PATCH] regulator: Allow autosetting fixed regulators
> 
> > How about the following patch? not tested
> >
> > diff --git a/drivers/power/regulator/regulator-uclass.c
> b/drivers/power/regulator/regulator-uclass.c
> > index 9118b8eb39..76be95bcd1 100644
> > --- a/drivers/power/regulator/regulator-uclass.c
> > +++ b/drivers/power/regulator/regulator-uclass.c
> > @@ -238,6 +238,9 @@ int regulator_autoset(struct udevice *dev)
> >        if (!uc_pdata->always_on && !uc_pdata->boot_on)
> >                return -EMEDIUMTYPE;
> >
> > +       if (uc_pdata->type == REGULATOR_TYPE_FIXED)
> > +               return regulator_set_enable(dev, true);
> > +
> >        if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
> >                ret = regulator_set_value(dev, uc_pdata->min_uV);
> >        if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
> 
> That will probably work as well. It does, however, feel a little like a hack.
> What if there are other regulator types that require a similar workaround?

When ret is -ENOSYS, it might be a fixed regulator. So directly use FIXED
regulator check make more sense to me.

Regards,
Peng.

> 
> Sven

[U-Boot] [PATCH v2] regulator: Allow autosetting fixed regulators

[Sven Schwermer]

Fixed regulators don't have a set_value method. Therefore, trying to
set their value will always return -ENOSYS.

Signed-off-by: Sven Schwermer <sven@svenschwermer.de>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Peng Fan <peng.fan@nxp.com>
---
 drivers/power/regulator/regulator-uclass.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
index 9118b8eb39..76be95bcd1 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -238,6 +238,9 @@ int regulator_autoset(struct udevice *dev)
 	if (!uc_pdata->always_on && !uc_pdata->boot_on)
 		return -EMEDIUMTYPE;
 
+	if (uc_pdata->type == REGULATOR_TYPE_FIXED)
+		return regulator_set_enable(dev, true);
+
 	if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
 		ret = regulator_set_value(dev, uc_pdata->min_uV);
 	if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
-- 
2.17.1

[U-Boot] [PATCH v2] regulator: Allow autosetting fixed regulators

[Simon Glass]

On Wed, 12 Jun 2019 at 07:32, Sven Schwermer <sven@svenschwermer.de> wrote:
>
> Fixed regulators don't have a set_value method. Therefore, trying to
> set their value will always return -ENOSYS.
>
> Signed-off-by: Sven Schwermer <sven@svenschwermer.de>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/power/regulator/regulator-uclass.c | 3 +++
>  1 file changed, 3 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

It should be possible to create a simple test for this in test/dm/regulator.c

[U-Boot] [PATCH v2] regulator: Allow autosetting fixed regulators

[Tom Rini]

On Wed, Jun 12, 2019 at 08:32:38AM +0200, Sven Schwermer wrote:

> Fixed regulators don't have a set_value method. Therefore, trying to
> set their value will always return -ENOSYS.
> 
> Signed-off-by: Sven Schwermer <sven@svenschwermer.de>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190714/8bf3a103/attachment.sig>