From: Benjamin Bara <benjamin.bara@skidata.com>
The monitors of the bd718x7 must be disabled while the respective
regulator is switching to a higher voltage. Use the new property
'.mon_disable_reg_set_higher' to activate the handling in the core.
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
drivers/regulator/bd718x7-regulator.c | 136 +++-------------------------------
1 file changed, 10 insertions(+), 126 deletions(-)
diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index fbf609d219fc..251d098d088c 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -128,128 +128,6 @@ static int bd71837_get_buck34_enable_hwctrl(struct regulator_dev *rdev)
return !!(BD718XX_BUCK_RUN_ON & val);
}
-static void voltage_change_done(struct regulator_dev *rdev, unsigned int sel,
- unsigned int *mask)
-{
- int ret;
-
- if (*mask) {
- /*
- * Let's allow scheduling as we use I2C anyways. We just need to
- * guarantee minimum of 1ms sleep - it shouldn't matter if we
- * exceed it due to the scheduling.
- */
- msleep(1);
-
- ret = regmap_clear_bits(rdev->regmap, BD718XX_REG_MVRFLTMASK2,
- *mask);
- if (ret)
- dev_err(&rdev->dev,
- "Failed to re-enable voltage monitoring (%d)\n",
- ret);
- }
-}
-
-static int voltage_change_prepare(struct regulator_dev *rdev, unsigned int sel,
- unsigned int *mask)
-{
- int ret;
-
- *mask = 0;
- if (rdev->desc->ops->is_enabled(rdev)) {
- int now, new;
-
- now = rdev->desc->ops->get_voltage_sel(rdev);
- if (now < 0)
- return now;
-
- now = rdev->desc->ops->list_voltage(rdev, now);
- if (now < 0)
- return now;
-
- new = rdev->desc->ops->list_voltage(rdev, sel);
- if (new < 0)
- return new;
-
- /*
- * If we increase LDO voltage when LDO is enabled we need to
- * disable the power-good detection until voltage has reached
- * the new level. According to HW colleagues the maximum time
- * it takes is 1000us. I assume that on systems with light load
- * this might be less - and we could probably use DT to give
- * system specific delay value if performance matters.
- *
- * Well, knowing we use I2C here and can add scheduling delays
- * I don't think it is worth the hassle and I just add fixed
- * 1ms sleep here (and allow scheduling). If this turns out to
- * be a problem we can change it to delay and make the delay
- * time configurable.
- */
- if (new > now) {
- int tmp;
- int prot_bit;
- int ldo_offset = rdev->desc->id - BD718XX_LDO1;
-
- prot_bit = BD718XX_LDO1_VRMON80 << ldo_offset;
- ret = regmap_read(rdev->regmap, BD718XX_REG_MVRFLTMASK2,
- &tmp);
- if (ret) {
- dev_err(&rdev->dev,
- "Failed to read voltage monitoring state\n");
- return ret;
- }
-
- if (!(tmp & prot_bit)) {
- /* We disable protection if it was enabled... */
- ret = regmap_set_bits(rdev->regmap,
- BD718XX_REG_MVRFLTMASK2,
- prot_bit);
- /* ...and we also want to re-enable it */
- *mask = prot_bit;
- }
- if (ret) {
- dev_err(&rdev->dev,
- "Failed to stop voltage monitoring\n");
- return ret;
- }
- }
- }
-
- return 0;
-}
-
-static int bd718xx_set_voltage_sel_restricted(struct regulator_dev *rdev,
- unsigned int sel)
-{
- int ret;
- int mask;
-
- ret = voltage_change_prepare(rdev, sel, &mask);
- if (ret)
- return ret;
-
- ret = regulator_set_voltage_sel_regmap(rdev, sel);
- voltage_change_done(rdev, sel, &mask);
-
- return ret;
-}
-
-static int bd718xx_set_voltage_sel_pickable_restricted(
- struct regulator_dev *rdev, unsigned int sel)
-{
- int ret;
- int mask;
-
- ret = voltage_change_prepare(rdev, sel, &mask);
- if (ret)
- return ret;
-
- ret = regulator_set_voltage_sel_pickable_regmap(rdev, sel);
- voltage_change_done(rdev, sel, &mask);
-
- return ret;
-}
-
static int bd71837_set_voltage_sel_pickable_restricted(
struct regulator_dev *rdev, unsigned int sel)
{
@@ -610,7 +488,7 @@ static int bd718x7_set_buck_ovp(struct regulator_dev *rdev, int lim_uV,
*/
BD718XX_OPS(bd718xx_pickable_range_ldo_ops,
regulator_list_voltage_pickable_linear_range, NULL,
- bd718xx_set_voltage_sel_pickable_restricted,
+ regulator_set_voltage_sel_pickable_regmap,
regulator_get_voltage_sel_pickable_regmap, NULL, NULL,
bd718x7_set_ldo_uvp, NULL, bd717x7_get_ldo_prot);
@@ -618,7 +496,7 @@ BD718XX_OPS(bd718xx_pickable_range_ldo_ops,
static const struct regulator_ops bd718xx_ldo5_ops_hwstate = {
.is_enabled = never_enabled_by_hwstate,
.list_voltage = regulator_list_voltage_pickable_linear_range,
- .set_voltage_sel = bd718xx_set_voltage_sel_pickable_restricted,
+ .set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,
.get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,
.set_under_voltage_protection = bd718x7_set_ldo_uvp,
};
@@ -631,12 +509,12 @@ BD718XX_OPS(bd718xx_pickable_range_buck_ops,
bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
BD718XX_OPS(bd718xx_ldo_regulator_ops, regulator_list_voltage_linear_range,
- NULL, bd718xx_set_voltage_sel_restricted,
+ NULL, regulator_set_voltage_sel_regmap,
regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
NULL, bd717x7_get_ldo_prot);
BD718XX_OPS(bd718xx_ldo_regulator_nolinear_ops, regulator_list_voltage_table,
- NULL, bd718xx_set_voltage_sel_restricted,
+ NULL, regulator_set_voltage_sel_regmap,
regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
NULL, bd717x7_get_ldo_prot);
@@ -1818,6 +1696,12 @@ static int bd718xx_probe(struct platform_device *pdev)
else
desc->ops = swops[i];
+ /*
+ * bd718x7 requires to disable a regulator's over voltage
+ * protection while it changes to a higher value.
+ */
+ desc->mon_disable_reg_set_higher = REGULATOR_MONITOR_OVER_VOLTAGE;
+
rdev = devm_regulator_register(&pdev->dev, desc, &config);
if (IS_ERR(rdev))
return dev_err_probe(&pdev->dev, PTR_ERR(rdev),
--
2.34.1On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> The monitors of the bd718x7 must be disabled while the respective
> regulator is switching to a higher voltage. Use the new property
> '.mon_disable_reg_set_higher' to activate the handling in the core.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
This looks great to me. Eg,
Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
Just a thing crossed my mind if you want to go an extra mile... (Yeah,
we usually do like to do a bit more work, right?)
I guess that enabling / disabling a monitor is in many cases a matter of
setting/clearing a single bit in a monitoring register - or maybe in
some cases setting a limit value to zero.
Do you think it might be worth to add a 'monitor_reg_enable_uv,
monitor_reg_enable_ov, monitor_reg_enable_oc, monitor_reg_enable_temp'
and 'monitor_mask_enable_uv, monitor_mask_enable_ov,
monitor_mask_enable_oc, monitor_mask_enable_temp' in the regulator_desc
+ provide helpers for the drivers which do not need any more complex stuff?
Just a thought. Again, thanks for working on this!
> ---
> drivers/regulator/bd718x7-regulator.c | 136 +++-------------------------------
> 1 file changed, 10 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
> index fbf609d219fc..251d098d088c 100644
> --- a/drivers/regulator/bd718x7-regulator.c
> +++ b/drivers/regulator/bd718x7-regulator.c
> @@ -128,128 +128,6 @@ static int bd71837_get_buck34_enable_hwctrl(struct regulator_dev *rdev)
> return !!(BD718XX_BUCK_RUN_ON & val);
> }
>
> -static void voltage_change_done(struct regulator_dev *rdev, unsigned int sel,
> - unsigned int *mask)
> -{
> - int ret;
> -
> - if (*mask) {
> - /*
> - * Let's allow scheduling as we use I2C anyways. We just need to
> - * guarantee minimum of 1ms sleep - it shouldn't matter if we
> - * exceed it due to the scheduling.
> - */
> - msleep(1);
> -
> - ret = regmap_clear_bits(rdev->regmap, BD718XX_REG_MVRFLTMASK2,
> - *mask);
> - if (ret)
> - dev_err(&rdev->dev,
> - "Failed to re-enable voltage monitoring (%d)\n",
> - ret);
> - }
> -}
> -
> -static int voltage_change_prepare(struct regulator_dev *rdev, unsigned int sel,
> - unsigned int *mask)
> -{
> - int ret;
> -
> - *mask = 0;
> - if (rdev->desc->ops->is_enabled(rdev)) {
> - int now, new;
> -
> - now = rdev->desc->ops->get_voltage_sel(rdev);
> - if (now < 0)
> - return now;
> -
> - now = rdev->desc->ops->list_voltage(rdev, now);
> - if (now < 0)
> - return now;
> -
> - new = rdev->desc->ops->list_voltage(rdev, sel);
> - if (new < 0)
> - return new;
> -
> - /*
> - * If we increase LDO voltage when LDO is enabled we need to
> - * disable the power-good detection until voltage has reached
> - * the new level. According to HW colleagues the maximum time
> - * it takes is 1000us. I assume that on systems with light load
> - * this might be less - and we could probably use DT to give
> - * system specific delay value if performance matters.
> - *
> - * Well, knowing we use I2C here and can add scheduling delays
> - * I don't think it is worth the hassle and I just add fixed
> - * 1ms sleep here (and allow scheduling). If this turns out to
> - * be a problem we can change it to delay and make the delay
> - * time configurable.
> - */
> - if (new > now) {
> - int tmp;
> - int prot_bit;
> - int ldo_offset = rdev->desc->id - BD718XX_LDO1;
> -
> - prot_bit = BD718XX_LDO1_VRMON80 << ldo_offset;
> - ret = regmap_read(rdev->regmap, BD718XX_REG_MVRFLTMASK2,
> - &tmp);
> - if (ret) {
> - dev_err(&rdev->dev,
> - "Failed to read voltage monitoring state\n");
> - return ret;
> - }
> -
> - if (!(tmp & prot_bit)) {
> - /* We disable protection if it was enabled... */
> - ret = regmap_set_bits(rdev->regmap,
> - BD718XX_REG_MVRFLTMASK2,
> - prot_bit);
> - /* ...and we also want to re-enable it */
> - *mask = prot_bit;
> - }
> - if (ret) {
> - dev_err(&rdev->dev,
> - "Failed to stop voltage monitoring\n");
> - return ret;
> - }
> - }
> - }
> -
> - return 0;
> -}
> -
> -static int bd718xx_set_voltage_sel_restricted(struct regulator_dev *rdev,
> - unsigned int sel)
> -{
> - int ret;
> - int mask;
> -
> - ret = voltage_change_prepare(rdev, sel, &mask);
> - if (ret)
> - return ret;
> -
> - ret = regulator_set_voltage_sel_regmap(rdev, sel);
> - voltage_change_done(rdev, sel, &mask);
> -
> - return ret;
> -}
> -
> -static int bd718xx_set_voltage_sel_pickable_restricted(
> - struct regulator_dev *rdev, unsigned int sel)
> -{
> - int ret;
> - int mask;
> -
> - ret = voltage_change_prepare(rdev, sel, &mask);
> - if (ret)
> - return ret;
> -
> - ret = regulator_set_voltage_sel_pickable_regmap(rdev, sel);
> - voltage_change_done(rdev, sel, &mask);
> -
> - return ret;
> -}
> -
> static int bd71837_set_voltage_sel_pickable_restricted(
> struct regulator_dev *rdev, unsigned int sel)
> {
> @@ -610,7 +488,7 @@ static int bd718x7_set_buck_ovp(struct regulator_dev *rdev, int lim_uV,
> */
> BD718XX_OPS(bd718xx_pickable_range_ldo_ops,
> regulator_list_voltage_pickable_linear_range, NULL,
> - bd718xx_set_voltage_sel_pickable_restricted,
> + regulator_set_voltage_sel_pickable_regmap,
> regulator_get_voltage_sel_pickable_regmap, NULL, NULL,
> bd718x7_set_ldo_uvp, NULL, bd717x7_get_ldo_prot);
>
> @@ -618,7 +496,7 @@ BD718XX_OPS(bd718xx_pickable_range_ldo_ops,
> static const struct regulator_ops bd718xx_ldo5_ops_hwstate = {
> .is_enabled = never_enabled_by_hwstate,
> .list_voltage = regulator_list_voltage_pickable_linear_range,
> - .set_voltage_sel = bd718xx_set_voltage_sel_pickable_restricted,
> + .set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,
> .get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,
> .set_under_voltage_protection = bd718x7_set_ldo_uvp,
> };
> @@ -631,12 +509,12 @@ BD718XX_OPS(bd718xx_pickable_range_buck_ops,
> bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
>
> BD718XX_OPS(bd718xx_ldo_regulator_ops, regulator_list_voltage_linear_range,
> - NULL, bd718xx_set_voltage_sel_restricted,
> + NULL, regulator_set_voltage_sel_regmap,
> regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
> NULL, bd717x7_get_ldo_prot);
>
> BD718XX_OPS(bd718xx_ldo_regulator_nolinear_ops, regulator_list_voltage_table,
> - NULL, bd718xx_set_voltage_sel_restricted,
> + NULL, regulator_set_voltage_sel_regmap,
> regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
> NULL, bd717x7_get_ldo_prot);
>
> @@ -1818,6 +1696,12 @@ static int bd718xx_probe(struct platform_device *pdev)
> else
> desc->ops = swops[i];
>
> + /*
> + * bd718x7 requires to disable a regulator's over voltage
> + * protection while it changes to a higher value.
> + */
> + desc->mon_disable_reg_set_higher = REGULATOR_MONITOR_OVER_VOLTAGE;
> +
> rdev = devm_regulator_register(&pdev->dev, desc, &config);
> if (IS_ERR(rdev))
> return dev_err_probe(&pdev->dev, PTR_ERR(rdev),
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
© 2016 - 2025 Red Hat, Inc.