Messages in this thread | | | Date | Mon, 3 Jul 2023 14:02:35 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCH RFC v4 13/13] regulator: bd718x7: let the core handle the monitors |
| |
On 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! ~~
| |