| Message ID | 20190213164648.26579-8-krzk@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
| Series | arm: exynos: Fix reboot on Odroid HC1 | expand |
On Wed, 13 Feb 2019 17:46:46 +0100 Krzysztof Kozlowski <krzk@kernel.org> wrote: > According to datasheet, the output on LDO regulators will start > appearing after 10-15 us. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/regulator/s2mps11_regulator.c > b/drivers/power/regulator/s2mps11_regulator.c index > 723d27f67c9a..1f1581852ee2 100644 --- > a/drivers/power/regulator/s2mps11_regulator.c +++ > b/drivers/power/regulator/s2mps11_regulator.c @@ -551,7 +551,14 @@ > static int ldo_get_enable(struct udevice *dev) > static int ldo_set_enable(struct udevice *dev, bool enable) > { > - return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > + int ret; > + > + ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > + > + /* Wait the "enable delay" for voltage to start to rise */ > + udelay(15); Isn't the enable delay provided/read from dts? Or is it too early to have dtb parsed? The udelay(15) seems a bit "magic" value (or is it specified in the PMIC manual?). > + > + return ret; > } > > static int ldo_get_mode(struct udevice *dev) Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, 15 Feb 2019 at 08:04, Lukasz Majewski <lukma@denx.de> wrote: > > On Wed, 13 Feb 2019 17:46:46 +0100 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > According to datasheet, the output on LDO regulators will start > > appearing after 10-15 us. > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/power/regulator/s2mps11_regulator.c > > b/drivers/power/regulator/s2mps11_regulator.c index > > 723d27f67c9a..1f1581852ee2 100644 --- > > a/drivers/power/regulator/s2mps11_regulator.c +++ > > b/drivers/power/regulator/s2mps11_regulator.c @@ -551,7 +551,14 @@ > > static int ldo_get_enable(struct udevice *dev) > > static int ldo_set_enable(struct udevice *dev, bool enable) > > { > > - return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > > + int ret; > > + > > + ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > > + > > + /* Wait the "enable delay" for voltage to start to rise */ > > + udelay(15); > > Isn't the enable delay provided/read from dts? > Or is it too early to have dtb parsed? We could read it from DTB... but I would need to add new property just for that. I can... just more commits for simple stuff :) > The udelay(15) seems a bit "magic" value (or is it specified in the > PMIC manual?). Yeah, it is magic value mentioned in PMIC manual (actually - 10-15 us). It is the same as ramp delay - PMIC specific value. Best regards, Krzysztof
Hi Krzysztof, > On Fri, 15 Feb 2019 at 08:04, Lukasz Majewski <lukma@denx.de> wrote: > > > > On Wed, 13 Feb 2019 17:46:46 +0100 > > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > According to datasheet, the output on LDO regulators will start > > > appearing after 10-15 us. > > > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > > --- > > > drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/power/regulator/s2mps11_regulator.c > > > b/drivers/power/regulator/s2mps11_regulator.c index > > > 723d27f67c9a..1f1581852ee2 100644 --- > > > a/drivers/power/regulator/s2mps11_regulator.c +++ > > > b/drivers/power/regulator/s2mps11_regulator.c @@ -551,7 +551,14 @@ > > > static int ldo_get_enable(struct udevice *dev) > > > static int ldo_set_enable(struct udevice *dev, bool enable) > > > { > > > - return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > > > + int ret; > > > + > > > + ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > > > + > > > + /* Wait the "enable delay" for voltage to start to rise */ > > > + udelay(15); > > > > Isn't the enable delay provided/read from dts? > > Or is it too early to have dtb parsed? > > We could read it from DTB... but I would need to add new property just > for that. I can... just more commits for simple stuff :) No, lets keep simple things simple :-). No need for extra DTB property. > > > The udelay(15) seems a bit "magic" value (or is it specified in the > > PMIC manual?). > > Yeah, it is magic value mentioned in PMIC manual (actually - 10-15 > us). It is the same as ramp delay - PMIC specific value. Ok, If it is in the manual then we shall stick to it (and vendor just stated - delay for 15ms). > > Best regards, > Krzysztof Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Krzysztof, On Wed, 13 Feb 2019 at 17:47, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > According to datasheet, the output on LDO regulators will start > appearing after 10-15 us. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/regulator/s2mps11_regulator.c b/drivers/power/regulator/s2mps11_regulator.c > index 723d27f67c9a..1f1581852ee2 100644 > --- a/drivers/power/regulator/s2mps11_regulator.c > +++ b/drivers/power/regulator/s2mps11_regulator.c > @@ -551,7 +551,14 @@ static int ldo_get_enable(struct udevice *dev) > > static int ldo_set_enable(struct udevice *dev, bool enable) > { > - return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > + int ret; > + > + ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); How about: if (ret) return ret; > + > + /* Wait the "enable delay" for voltage to start to rise */ > + udelay(15); > + > + return ret; return 0; > } > > static int ldo_get_mode(struct udevice *dev) > -- > 2.17.1 > Regards, Simon
On Fri, Feb 15, 2019 at 06:11:34PM +0100, Simon Glass wrote: > Hi Krzysztof, > > On Wed, 13 Feb 2019 at 17:47, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > According to datasheet, the output on LDO regulators will start > > appearing after 10-15 us. > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/power/regulator/s2mps11_regulator.c b/drivers/power/regulator/s2mps11_regulator.c > > index 723d27f67c9a..1f1581852ee2 100644 > > --- a/drivers/power/regulator/s2mps11_regulator.c > > +++ b/drivers/power/regulator/s2mps11_regulator.c > > @@ -551,7 +551,14 @@ static int ldo_get_enable(struct udevice *dev) > > > > static int ldo_set_enable(struct udevice *dev, bool enable) > > { > > - return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > > + int ret; > > + > > + ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); > > How about: > > if (ret) > return ret; > Sure, good idea, thanks! Best regards, Krzysztof
diff --git a/drivers/power/regulator/s2mps11_regulator.c b/drivers/power/regulator/s2mps11_regulator.c index 723d27f67c9a..1f1581852ee2 100644 --- a/drivers/power/regulator/s2mps11_regulator.c +++ b/drivers/power/regulator/s2mps11_regulator.c @@ -551,7 +551,14 @@ static int ldo_get_enable(struct udevice *dev) static int ldo_set_enable(struct udevice *dev, bool enable) { - return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); + int ret; + + ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable); + + /* Wait the "enable delay" for voltage to start to rise */ + udelay(15); + + return ret; } static int ldo_get_mode(struct udevice *dev)
According to datasheet, the output on LDO regulators will start appearing after 10-15 us. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)