| Message ID | 1394011373-4057-3-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> S2MPS14 regulators support suspend mode where their status is controlled > by PWREN coming from SoC. This patch implements the set_suspend_disable > for S2MPS14 regulators. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/regulator/s2mps11.c | 26 ++++++++++++++++++++++++++ > include/linux/mfd/samsung/s2mps14.h | 2 ++ > 2 files changed, 28 insertions(+) For the MFD changes: Acked-by: Lee Jones <lee.jones@linaro.org>
On Wed, Mar 05, 2014 at 10:22:52AM +0100, Krzysztof Kozlowski wrote: > + ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &data); > + if (ret < 0) > + return ret; > + > + /* > + * Don't enable suspend mode if regulator is already disabled because > + * this would effectively for a short time turn on the regulator after > + * resuming. > + */ > + if (!(data & rdev->desc->enable_mask)) > + return 0; > + > + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > + rdev->desc->enable_mask, S2MPS14_ENABLE_SUSPEND); > +} What happens if the regulator gets enabled between this being called and the device suspending? I'd expect this to be storing the state and then changing what gets written during enable and disable operations (if the hardware does what I think it does).
On Thu, 2014-03-06 at 17:38 +0800, Mark Brown wrote: > On Wed, Mar 05, 2014 at 10:22:52AM +0100, Krzysztof Kozlowski wrote: > > > + ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &data); > > + if (ret < 0) > > + return ret; > > + > > + /* > > + * Don't enable suspend mode if regulator is already disabled because > > + * this would effectively for a short time turn on the regulator after > > + * resuming. > > + */ > > + if (!(data & rdev->desc->enable_mask)) > > + return 0; > > + > > + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > > + rdev->desc->enable_mask, S2MPS14_ENABLE_SUSPEND); > > +} > > What happens if the regulator gets enabled between this being called > and the device suspending? I'd expect this to be storing the state and > then changing what gets written during enable and disable operations (if > the hardware does what I think it does). Hmmm... you're right. I'll fix this. Anyway can you look at other two patches and apply them if they are OK? They don't depend on this. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-03-06 at 17:38 +0800, Mark Brown wrote: > On Wed, Mar 05, 2014 at 10:22:52AM +0100, Krzysztof Kozlowski wrote: > > > + ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &data); > > + if (ret < 0) > > + return ret; > > + > > + /* > > + * Don't enable suspend mode if regulator is already disabled because > > + * this would effectively for a short time turn on the regulator after > > + * resuming. > > + */ > > + if (!(data & rdev->desc->enable_mask)) > > + return 0; > > + > > + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > > + rdev->desc->enable_mask, S2MPS14_ENABLE_SUSPEND); > > +} > > What happens if the regulator gets enabled between this being called > and the device suspending? I'd expect this to be storing the state and > then changing what gets written during enable and disable operations (if > the hardware does what I think it does). It seems that none of the regulators implementing set_suspend_disable() work that way. They just write the suspend value to device. Of course this is not an issue - the S2MPS14 may be the first :). However in that case the driver won't be able later to change that value back to "normal enable" (enable_mask). Consider such flow: 1. System is going to suspend. 2. Some regulator has "rstate->disabled" so set_suspend_disable() is called on it. 3. The "suspend" value is written to the device for given regulator and it is stored as "enable" value. 4. If regulator is enabled during here then the same "suspend" value will be written. 5. System is suspended. 6. After resuming regulator_suspend_finish() calls _regulator_do_enable() on the regulator... which will write the "suspend" value because the driver cannot differentiate between this enable and previous. I assume that this may not be a problem because: 1. Regulator will be still turned on (the "suspend" value tells PMIC to enable the regulator when SoC enables power). 2. The first disable of regulator may bring back "enable" value back to normal mode. Am I thinking here correctly? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 06, 2014 at 10:48:54AM +0100, Krzysztof Kozlowski wrote: > Anyway can you look at other two patches and apply them if they are OK? > They don't depend on this. There appears to be some external dependency for the definition of macros like S2MPS14X?
On Thu, Mar 06, 2014 at 03:42:22PM +0100, Krzysztof Kozlowski wrote: > However in that case the driver won't be able later to change that value > back to "normal enable" (enable_mask). Consider such flow: > 1. System is going to suspend. > 2. Some regulator has "rstate->disabled" so set_suspend_disable() is > called on it. > 3. The "suspend" value is written to the device for given regulator and > it is stored as "enable" value. > 4. If regulator is enabled during here then the same "suspend" value > will be written. > 5. System is suspended. > 6. After resuming regulator_suspend_finish() calls > _regulator_do_enable() on the regulator... which will write the > "suspend" value because the driver cannot differentiate between this > enable and previous. > I assume that this may not be a problem because: > 1. Regulator will be still turned on (the "suspend" value tells PMIC to > enable the regulator when SoC enables power). > 2. The first disable of regulator may bring back "enable" value back to > normal mode. > Am I thinking here correctly? I'm not entirely sure I follow here. Why would a disable reset the enable value? My understanding is that this is a bitfield with several values, off, on always and on when they system is active. The suspend state is being tracked with a variable so I'm not sure why disabling would reset it? There is a bit of an issue if the regulator is disabled during runtime but enabled in suspend but that's hard to resolve and I'm not sure that it's a realistic issue.
On Fri, 2014-03-07 at 09:51 +0800, Mark Brown wrote: > On Thu, Mar 06, 2014 at 10:48:54AM +0100, Krzysztof Kozlowski wrote: > > > Anyway can you look at other two patches and apply them if they are OK? > > They don't depend on this. > > There appears to be some external dependency for the definition of > macros like S2MPS14X? The patchset depends on S2MPS14 support for MFD driver. This was applied by Lee here: https://git.linaro.org/people/lee.jones/mfd.git/shortlog/refs/heads/for-mfd-next and merged into linux-next. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-03-07 at 10:37 +0800, Mark Brown wrote: > On Thu, Mar 06, 2014 at 03:42:22PM +0100, Krzysztof Kozlowski wrote: > > > However in that case the driver won't be able later to change that value > > back to "normal enable" (enable_mask). Consider such flow: > > 1. System is going to suspend. > > 2. Some regulator has "rstate->disabled" so set_suspend_disable() is > > called on it. > > 3. The "suspend" value is written to the device for given regulator and > > it is stored as "enable" value. > > 4. If regulator is enabled during here then the same "suspend" value > > will be written. > > 5. System is suspended. > > 6. After resuming regulator_suspend_finish() calls > > _regulator_do_enable() on the regulator... which will write the > > "suspend" value because the driver cannot differentiate between this > > enable and previous. > > > I assume that this may not be a problem because: > > 1. Regulator will be still turned on (the "suspend" value tells PMIC to > > enable the regulator when SoC enables power). > > 2. The first disable of regulator may bring back "enable" value back to > > normal mode. > > > Am I thinking here correctly? > > I'm not entirely sure I follow here. Why would a disable reset the > enable value? My understanding is that this is a bitfield with several > values, off, on always and on when they system is active. The suspend > state is being tracked with a variable so I'm not sure why disabling > would reset it? > > There is a bit of an issue if the regulator is disabled during runtime > but enabled in suspend but that's hard to resolve and I'm not sure that > it's a realistic issue. OK, I think I understand it... I sent v2 of the set_suspend_disable patch. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Anyway can you look at other two patches and apply them if they are OK? > > > They don't depend on this. > > > > There appears to be some external dependency for the definition of > > macros like S2MPS14X? > The patchset depends on S2MPS14 support for MFD driver. This was applied > by Lee here: > https://git.linaro.org/people/lee.jones/mfd.git/shortlog/refs/heads/for-mfd-next > and merged into linux-next. Right, so when this set obtains all its Acks, I'll apply them to the MFD branch.
diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c index 1a732e54cd70..d3209acefd79 100644 --- a/drivers/regulator/s2mps11.c +++ b/drivers/regulator/s2mps11.c @@ -399,6 +399,31 @@ static const struct regulator_desc s2mps11_regulators[] = { regulator_desc_s2mps11_buck10, }; +static int s2mps14_set_suspend_disable(struct regulator_dev *rdev) +{ + int ret; + unsigned int data; + + /* LDO3 should be always on and does not support suspend mode */ + if (rdev_get_id(rdev) == S2MPS14_LDO3) + return 0; + + ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &data); + if (ret < 0) + return ret; + + /* + * Don't enable suspend mode if regulator is already disabled because + * this would effectively for a short time turn on the regulator after + * resuming. + */ + if (!(data & rdev->desc->enable_mask)) + return 0; + + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, + rdev->desc->enable_mask, S2MPS14_ENABLE_SUSPEND); +} + static struct regulator_ops s2mps14_reg_ops = { .list_voltage = regulator_list_voltage_linear, .map_voltage = regulator_map_voltage_linear, @@ -408,6 +433,7 @@ static struct regulator_ops s2mps14_reg_ops = { .get_voltage_sel = regulator_get_voltage_sel_regmap, .set_voltage_sel = regulator_set_voltage_sel_regmap, .set_voltage_time_sel = regulator_set_voltage_time_sel, + .set_suspend_disable = s2mps14_set_suspend_disable, }; #define regulator_desc_s2mps14_ldo1(num) { \ diff --git a/include/linux/mfd/samsung/s2mps14.h b/include/linux/mfd/samsung/s2mps14.h index ec1e0857ddde..4b449b8ac548 100644 --- a/include/linux/mfd/samsung/s2mps14.h +++ b/include/linux/mfd/samsung/s2mps14.h @@ -146,6 +146,8 @@ enum s2mps14_regulators { #define S2MPS14_BUCK_VSEL_MASK 0xFF #define S2MPS14_ENABLE_MASK (0x03 << S2MPS14_ENABLE_SHIFT) #define S2MPS14_ENABLE_SHIFT 6 +/* On/Off controlled by PWREN */ +#define S2MPS14_ENABLE_SUSPEND (0x01 << S2MPS14_ENABLE_SHIFT) #define S2MPS14_LDO_N_VOLTAGES (S2MPS14_LDO_VSEL_MASK + 1) #define S2MPS14_BUCK_N_VOLTAGES (S2MPS14_BUCK_VSEL_MASK + 1)
S2MPS14 regulators support suspend mode where their status is controlled by PWREN coming from SoC. This patch implements the set_suspend_disable for S2MPS14 regulators. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/regulator/s2mps11.c | 26 ++++++++++++++++++++++++++ include/linux/mfd/samsung/s2mps14.h | 2 ++ 2 files changed, 28 insertions(+)