[PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[Charles Keepax]

From: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>

For regulators that support HI_PWR we need to ensure that switching to
1.8v allows time for the regulator to reach that voltage.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/regulator/arizona-ldo1.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index a1d07d3..9474597 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -113,6 +113,17 @@ static int arizona_ldo1_hc_get_voltage_sel(struct regulator_dev *rdev)
 	return (val & ARIZONA_LDO1_VSEL_MASK) >> ARIZONA_LDO1_VSEL_SHIFT;
 }
 
+static int arizona_ldo1_hc_set_voltage_time_sel(struct regulator_dev *rdev,
+						unsigned int old_selector,
+						unsigned int new_selector)
+{
+	/* if moving to 1.8v allow time for it to reach voltage */
+	if (new_selector == rdev->desc->n_voltages - 1)
+		return 25;
+	else
+		return 0;
+}
+
 static struct regulator_ops arizona_ldo1_hc_ops = {
 	.list_voltage = arizona_ldo1_hc_list_voltage,
 	.map_voltage = arizona_ldo1_hc_map_voltage,
@@ -120,6 +131,7 @@ static struct regulator_ops arizona_ldo1_hc_ops = {
 	.set_voltage_sel = arizona_ldo1_hc_set_voltage_sel,
 	.get_bypass = regulator_get_bypass_regmap,
 	.set_bypass = regulator_set_bypass_regmap,
+	.set_voltage_time_sel = arizona_ldo1_hc_set_voltage_time_sel,
 };
 
 static const struct regulator_desc arizona_ldo1_hc = {
-- 
1.7.2.5

Re: [PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Tue, Mar 24, 2015 at 02:27:56PM +0000, Charles Keepax wrote:

> +static int arizona_ldo1_hc_set_voltage_time_sel(struct regulator_dev *rdev,
> +						unsigned int old_selector,
> +						unsigned int new_selector)
> +{
> +	/* if moving to 1.8v allow time for it to reach voltage */
> +	if (new_selector == rdev->desc->n_voltages - 1)
> +		return 25;
> +	else
> +		return 0;
> +}

So changes to move to the top voltage always take constant time while
all other voltage changes are instantaneous?  That doesn't seem right.
I'd expect something more like a calculation based on some number of
miliseconds per milivolt.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[Charles Keepax]

On Tue, Mar 24, 2015 at 09:06:09AM -0700, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 02:27:56PM +0000, Charles Keepax wrote:
> 
> > +static int arizona_ldo1_hc_set_voltage_time_sel(struct regulator_dev *rdev,
> > +						unsigned int old_selector,
> > +						unsigned int new_selector)
> > +{
> > +	/* if moving to 1.8v allow time for it to reach voltage */
> > +	if (new_selector == rdev->desc->n_voltages - 1)
> > +		return 25;
> > +	else
> > +		return 0;
> > +}
> 
> So changes to move to the top voltage always take constant time while
> all other voltage changes are instantaneous?  That doesn't seem right.
> I'd expect something more like a calculation based on some number of
> miliseconds per milivolt.

Its more just that this is the only case that we really care
about. The reg only ever gets used at 1.2V and 1.8V, and the
only case where there is a problem is if we ask for 1.8V and
we don't have it yet.

I don't think we really have the data to give for other cases. I
could expand the comment perhaps? Or TBH it is fast enough it is
unlikely to ever be a problem in practice so we could just drop
the patch.

Thanks,
Charles

Re: [PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[Charles Keepax]

On Tue, Mar 24, 2015 at 04:40:50PM +0000, Charles Keepax wrote:
> On Tue, Mar 24, 2015 at 09:06:09AM -0700, Mark Brown wrote:
> > On Tue, Mar 24, 2015 at 02:27:56PM +0000, Charles Keepax wrote:
> > 
> > > +static int arizona_ldo1_hc_set_voltage_time_sel(struct regulator_dev *rdev,
> > > +						unsigned int old_selector,
> > > +						unsigned int new_selector)
> > > +{
> > > +	/* if moving to 1.8v allow time for it to reach voltage */
> > > +	if (new_selector == rdev->desc->n_voltages - 1)
> > > +		return 25;
> > > +	else
> > > +		return 0;
> > > +}
> > 
> > So changes to move to the top voltage always take constant time while
> > all other voltage changes are instantaneous?  That doesn't seem right.
> > I'd expect something more like a calculation based on some number of
> > miliseconds per milivolt.
> 
> Its more just that this is the only case that we really care
> about. The reg only ever gets used at 1.2V and 1.8V, and the
> only case where there is a problem is if we ask for 1.8V and
> we don't have it yet.
> 
> I don't think we really have the data to give for other cases. I
> could expand the comment perhaps? Or TBH it is fast enough it is
> unlikely to ever be a problem in practice so we could just drop
> the patch.

Oh, I had one other thought, what about if we just returned 25uS
as a worse case for the transition. Perhaps that would be nicer,
and the 1.2V -> 1.8V transition is almost certainly the slowest
of the options available.

Thanks,
Charles

Re: [PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

On Tue, Mar 24, 2015 at 05:05:34PM +0000, Charles Keepax wrote:

> Oh, I had one other thought, what about if we just returned 25uS
> as a worse case for the transition. Perhaps that would be nicer,
> and the 1.2V -> 1.8V transition is almost certainly the slowest
> of the options available.

That works too.  The problem is returning zero.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On Tue, Mar 24, 2015 at 04:40:50PM +0000, Charles Keepax wrote:
> On Tue, Mar 24, 2015 at 09:06:09AM -0700, Mark Brown wrote:

> > So changes to move to the top voltage always take constant time while
> > all other voltage changes are instantaneous?  That doesn't seem right.
> > I'd expect something more like a calculation based on some number of
> > miliseconds per milivolt.

> Its more just that this is the only case that we really care
> about. The reg only ever gets used at 1.2V and 1.8V, and the
> only case where there is a problem is if we ask for 1.8V and
> we don't have it yet.

> I don't think we really have the data to give for other cases. I
> could expand the comment perhaps? Or TBH it is fast enough it is
> unlikely to ever be a problem in practice so we could just drop
> the patch.

You'll probably find that the ramp is pretty much linear so just
changing to something that multiplies out to the expected value will be
good enough.  I'd rather not have bad examples that other drivers might
copy.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: arizona-ldo1: Add ramp time for HI_PWR

[Charles Keepax]

On Tue, Mar 24, 2015 at 10:05:54AM -0700, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 04:40:50PM +0000, Charles Keepax wrote:
> > On Tue, Mar 24, 2015 at 09:06:09AM -0700, Mark Brown wrote:
> 
> > > So changes to move to the top voltage always take constant time while
> > > all other voltage changes are instantaneous?  That doesn't seem right.
> > > I'd expect something more like a calculation based on some number of
> > > miliseconds per milivolt.
> 
> > Its more just that this is the only case that we really care
> > about. The reg only ever gets used at 1.2V and 1.8V, and the
> > only case where there is a problem is if we ask for 1.8V and
> > we don't have it yet.
> 
> > I don't think we really have the data to give for other cases. I
> > could expand the comment perhaps? Or TBH it is fast enough it is
> > unlikely to ever be a problem in practice so we could just drop
> > the patch.
> 
> You'll probably find that the ramp is pretty much linear so just
> changing to something that multiplies out to the expected value will be
> good enough.  I'd rather not have bad examples that other drivers might
> copy.

Cool, I will respin.

Thanks,
Charles