Hi!
This series targets the "automatic" state handling of monitors when the
state of the monitored regulator is changed. This is e.g. necessary for
the da9063, which reaches an invalid state (!PWR_OK) if the voltage
monitor is not disabled before the regulator is disabled. The problem
could also be tackled inside of the driver's "state change ops"
(.enable(), .disable(), ...) but I thought it might be a good idea to
have a "common framework" independent of the driver's implementation.
After feedback from Matti, the new approach doesn't disable the monitors
of disabled regulators anymore (except the respective workaround
property is set). Additionally, the core differs between initialization
and "workaround handling" when it comes to -EOPNOTSUPP.
1/13 is temporary implemented by me now and fixes a bug found by Martin
Fuzzey [1] which can be removed once a follow-up is received.
2/13 introduces a new op to read out the active monitors.
3/13 implements the new op for the da9063.
4/13 implements the new op for the bd718x7 (untested).
5/13 introduces the new "workaround properties".
6/13 ensure that the required regulator ops are implemented.
7/13 find all active regulator monitors the DT is not aware of.
8/13 factors out the existing monitor handling into an own function.
{9,10,11}/13 implements the workaround properties in the core.
12/13 implements mon_disable_reg_disabled for da9063.
13/13 implements mon_disable_reg_set_higher for bd718x7 (untested).
As far as I could tell from the implementations, the other two PMICs
with voltage protection support (max597x, bd9576) don't require
workarounds.
Thanks & best regards,
Benjamin
[1] https://lore.kernel.org/all/[email protected]/
---
Changes in v4:
- introduce helper to handle the monitor state according to the workarounds
- split up commits per workaround implementation
- don't disable monitors of disabled regulators anymore
- implement monitor getter for the da9063
- workarounds are now per-monitor instead of "global"
- require defined ops for workarounds
- ensure that active monitoring is known on regulators with workarounds
- re-enable monitors only if they were disabled
- Link to v3: https://lore.kernel.org/r/[email protected]
---
Benjamin Bara (13):
regulator: da9063: fix null pointer deref with partial DT config
regulator: add getter for active monitors
regulator: da9063: implement get_active_protections()
regulator: bd718x7: implement get_active_protections()
regulator: introduce properties for monitoring workarounds
regulator: set required ops for monitoring workarounds
regulator: find active protections during initialization
regulator: move monitor handling into own function
regulator: implement mon_disable_reg_disabled
regulator: implement mon_disable_reg_set_{higher,lower}
regulator: implement mon_unsupported_reg_modes
regulator: da9063: let the core handle the monitors
regulator: bd718x7: let the core handle the monitors
drivers/regulator/bd718x7-regulator.c | 210 +++++++------------
drivers/regulator/core.c | 370 ++++++++++++++++++++++++++++------
drivers/regulator/da9063-regulator.c | 33 ++-
include/linux/regulator/driver.h | 28 +++
4 files changed, 439 insertions(+), 202 deletions(-)
---
base-commit: f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6
change-id: 20230419-dynamic-vmon-e08daa0ac7ad
Best regards,
--
Benjamin Bara <[email protected]>
From: Benjamin Bara <[email protected]>
The monitors of the da9063 must be disabled while the respective
regulator is disabled. Use the new property '.mon_disable_reg_disabled'
to activate the handling in the core.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/da9063-regulator.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 071b368f154f..0e28ed15ebc3 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -1055,6 +1055,10 @@ static int da9063_regulator_probe(struct platform_device *pdev)
return ret;
}
+ /* da9063 requires disabled monitors while reg is disabled. */
+ regl->desc.mon_disable_reg_disabled = REGULATOR_MONITOR_OVER_VOLTAGE |
+ REGULATOR_MONITOR_UNDER_VOLTAGE;
+
regl->rdev = devm_regulator_register(&pdev->dev, ®l->desc,
&config);
if (IS_ERR(regl->rdev)) {
--
2.34.1
From: Benjamin Bara <[email protected]>
These are useful when the state of the regulator might change during
runtime, but the monitors state (in hardware) are not implicitly changed
with the change of the regulator state or mode (in hardware). Also, when
the monitors should be disabled while ramping after a set_value() or
when the regulator can enter a certain mode in which the monitoring does
not result in a valid state.
Signed-off-by: Benjamin Bara <[email protected]>
---
include/linux/regulator/driver.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 9a9163cae769..ce204ecd20e1 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -367,6 +367,19 @@ enum regulator_type {
* the regulator was actually enabled. Max upto enable_time.
*
* @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
+ *
+ * @mon_disable_reg_disabled: Disables the regulator's monitors while it is
+ * disabled. Affected REGULATOR_MONITOR_* are OR'ed.
+ * @mon_disable_reg_set_higher: Disables regulator's monitors while it is
+ * changing its value to a higher one. Affected
+ * REGULATOR_MONITOR_* are OR'ed.
+ * @mon_disable_reg_set_lower: Disables regulator's monitors while it is
+ * changing its value to a lower one. Affected
+ * REGULATOR_MONITOR_* are OR'ed.
+ * @mon_unsupported_reg_modes: Disables regulator's monitors before an
+ * unsupported mode is entered. REGULATOR_MODE_* are
+ * OR'ed. REGULATOR_MODE_INVALID means all modes can
+ * be monitored.
*/
struct regulator_desc {
const char *name;
@@ -441,6 +454,11 @@ struct regulator_desc {
unsigned int poll_enabled_time;
unsigned int (*of_map_mode)(unsigned int mode);
+
+ unsigned int mon_disable_reg_disabled;
+ unsigned int mon_disable_reg_set_higher;
+ unsigned int mon_disable_reg_set_lower;
+ unsigned int mon_unsupported_reg_modes;
};
/**
--
2.34.1
From: Benjamin Bara <[email protected]>
The mon_disable_reg_disabled property disables all dt-enabled monitors
before a regulator is disabled. If an error occurs while disabling the
regulator, the monitors are enabled again.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/core.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 873e53633698..b37dcafff407 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
trace_regulator_enable_complete(rdev_get_name(rdev));
- return 0;
+ return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
}
/**
@@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
static int _regulator_do_disable(struct regulator_dev *rdev)
{
+ const struct regulator_desc *desc = rdev->desc;
int ret;
+ ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
+ if (ret)
+ return ret;
+
trace_regulator_disable(rdev_get_name(rdev));
if (rdev->ena_pin) {
@@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
rdev->ena_gpio_state = 0;
}
- } else if (rdev->desc->ops->disable) {
- ret = rdev->desc->ops->disable(rdev);
+ } else if (desc->ops->disable) {
+ ret = desc->ops->disable(rdev);
if (ret != 0)
return ret;
}
- if (rdev->desc->off_on_delay)
+ if (desc->off_on_delay)
rdev->last_off = ktime_get_boottime();
trace_regulator_disable_complete(rdev_get_name(rdev));
@@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
_notifier_call_chain(rdev,
REGULATOR_EVENT_ABORT_DISABLE,
NULL);
+ monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
return ret;
}
_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
@@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
_notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
REGULATOR_EVENT_ABORT_DISABLE, NULL);
+ monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
return ret;
}
@@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
*/
rdev_info(rdev, "disabling\n");
ret = _regulator_do_disable(rdev);
- if (ret != 0)
+ if (ret != 0) {
rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
+ monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
+ }
} else {
/* The intention is that in future we will
* assume that full constraints are provided
--
2.34.1
From: Benjamin Bara <[email protected]>
Add an op to get all active monitors of a regulator. This is useful to
find out if any monitor is turned on, of which the device-tree is not
aware of (e.g. by bootloader or OTP).
Signed-off-by: Benjamin Bara <[email protected]>
---
include/linux/regulator/driver.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d3b4a3d4514a..9a9163cae769 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -25,6 +25,13 @@ struct regulator_config;
struct regulator_init_data;
struct regulator_enable_gpio;
+#define REGULATOR_MONITOR_NONE 0x0
+#define REGULATOR_MONITOR_OVER_CURRENT 0x1
+#define REGULATOR_MONITOR_OVER_VOLTAGE 0x2
+#define REGULATOR_MONITOR_UNDER_VOLTAGE 0x4
+#define REGULATOR_MONITOR_OVER_TEMPERATURE 0x8
+#define REGULATOR_MONITOR_ALL 0xF
+
enum regulator_status {
REGULATOR_STATUS_OFF,
REGULATOR_STATUS_ON,
@@ -112,6 +119,8 @@ enum regulator_detection_severity {
* @set_thermal_protection: Support enabling of and setting limits for over
* temperature situation detection.Detection can be configured for same
* severities as over current protection. Units of degree Kelvin.
+ * @get_active_protections: Get all enabled monitors of a regulator. OR'ed val
+ * of REGULATOR_MONITOR_*.
*
* @set_active_discharge: Set active discharge enable/disable of regulators.
*
@@ -183,6 +192,7 @@ struct regulator_ops {
int severity, bool enable);
int (*set_thermal_protection)(struct regulator_dev *, int lim,
int severity, bool enable);
+ int (*get_active_protections)(struct regulator_dev *dev, unsigned int *state);
int (*set_active_discharge)(struct regulator_dev *, bool enable);
/* enable/disable regulator */
--
2.34.1
From: Benjamin Bara <[email protected]>
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 <[email protected]>
---
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.1
On 6/20/23 23:02, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> Add an op to get all active monitors of a regulator. This is useful to
> find out if any monitor is turned on, of which the device-tree is not
> aware of (e.g. by bootloader or OTP).
>
> Signed-off-by: Benjamin Bara <[email protected]>
Just a couple of minor remarks. Feel free to change those if you end up
respinning.
Reviewed-by: Matti Vaittinen <[email protected]>
> ---
> include/linux/regulator/driver.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index d3b4a3d4514a..9a9163cae769 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -25,6 +25,13 @@ struct regulator_config;
> struct regulator_init_data;
> struct regulator_enable_gpio;
>
> +#define REGULATOR_MONITOR_NONE 0x0
> +#define REGULATOR_MONITOR_OVER_CURRENT 0x1
> +#define REGULATOR_MONITOR_OVER_VOLTAGE 0x2
> +#define REGULATOR_MONITOR_UNDER_VOLTAGE 0x4
> +#define REGULATOR_MONITOR_OVER_TEMPERATURE 0x8
> +#define REGULATOR_MONITOR_ALL 0xF
Not a big thing but maybe use BIT() to underline this is a bitmask?
> +
> enum regulator_status {
> REGULATOR_STATUS_OFF,
> REGULATOR_STATUS_ON,
> @@ -112,6 +119,8 @@ enum regulator_detection_severity {
> * @set_thermal_protection: Support enabling of and setting limits for over
> * temperature situation detection.Detection can be configured for same
> * severities as over current protection. Units of degree Kelvin.
> + * @get_active_protections: Get all enabled monitors of a regulator. OR'ed val
> + * of REGULATOR_MONITOR_*.
I think it wouldn't hurt to have doc stating in which case populating
this call-back is needed? I haven't read rest of the patches yet but I
guess this callback is going to be used internally by the regulator core
and maybe it is not obvious for driver author that this is needed by
core to be able to support automatic protection handling.
> *
> * @set_active_discharge: Set active discharge enable/disable of regulators.
> *
> @@ -183,6 +192,7 @@ struct regulator_ops {
> int severity, bool enable);
> int (*set_thermal_protection)(struct regulator_dev *, int lim,
> int severity, bool enable);
> + int (*get_active_protections)(struct regulator_dev *dev, unsigned int *state);
> int (*set_active_discharge)(struct regulator_dev *, bool enable);
>
> /* enable/disable regulator */
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 6/20/23 23:02, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> These are useful when the state of the regulator might change during
> runtime, but the monitors state (in hardware) are not implicitly changed
> with the change of the regulator state or mode (in hardware). Also, when
> the monitors should be disabled while ramping after a set_value() or
> when the regulator can enter a certain mode in which the monitoring does
> not result in a valid state.
>
> Signed-off-by: Benjamin Bara <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Mon, Jun 26, 2023 at 04:43:49PM +0300, Matti Vaittinen wrote:
> On 6/20/23 23:02, Benjamin Bara wrote:
> > + * @get_active_protections: Get all enabled monitors of a regulator. OR'ed val
> > + * of REGULATOR_MONITOR_*.
> I think it wouldn't hurt to have doc stating in which case populating this
> call-back is needed? I haven't read rest of the patches yet but I guess this
> callback is going to be used internally by the regulator core and maybe it
> is not obvious for driver author that this is needed by core to be able to
> support automatic protection handling.
I think this is true for pretty much all callbacks - broadly it's just
the case that if the hardware has a feature the best practice is that
the driver should implement all relevant operations.
Hi deeee Ho Benjamin,
I hope your train back to home was not delayed too much ;)
On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> The mon_disable_reg_disabled
The name of this always makes me to scratch my head a bit. (or, maybe it
is just the sunburns at my bald).
Do you think making it:
mon_disable_at_reg_disable or mon_disable_when_reg_disabled would be too
long?
> property disables all dt-enabled monitors
> before a regulator is disabled. If an error occurs while disabling the
> regulator, the monitors are enabled again.
>
> Signed-off-by: Benjamin Bara <[email protected]>
> ---
> drivers/regulator/core.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 873e53633698..b37dcafff407 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
>
> trace_regulator_enable_complete(rdev_get_name(rdev));
>
> - return 0;
> + return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
As I wrote in my comment to previous patch, I might find the logic a bit
more clear if the condition check was done here. Eg:
if (rdev->desc->mon_disable_when_reg_disabled)
return monitors_reenable(...);
return 0;
> }
>
> /**
> @@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
>
> static int _regulator_do_disable(struct regulator_dev *rdev)
> {
> + const struct regulator_desc *desc = rdev->desc;
> int ret;
>
> + ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
> + if (ret)
> + return ret;
Similarly, for me the logic would be easier to follow if this was:
if (desc->mon_disable_when_reg_disabled)
monitors_disable(...);
> +
> trace_regulator_disable(rdev_get_name(rdev));
>
> if (rdev->ena_pin) {
> @@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
> rdev->ena_gpio_state = 0;
> }
>
> - } else if (rdev->desc->ops->disable) {
> - ret = rdev->desc->ops->disable(rdev);
> + } else if (desc->ops->disable) {
> + ret = desc->ops->disable(rdev);
> if (ret != 0)
> return ret;
> }
>
> - if (rdev->desc->off_on_delay)
> + if (desc->off_on_delay)
> rdev->last_off = ktime_get_boottime();
>
> trace_regulator_disable_complete(rdev_get_name(rdev));
> @@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
> _notifier_call_chain(rdev,
> REGULATOR_EVENT_ABORT_DISABLE,
> NULL);
> + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
same here,
> return ret;
> }
> _notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> @@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
> rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
> _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
> REGULATOR_EVENT_ABORT_DISABLE, NULL);
> + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
here...
> return ret;
> }
>
> @@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
> */
> rdev_info(rdev, "disabling\n");
> ret = _regulator_do_disable(rdev);
> - if (ret != 0)
> + if (ret != 0) {
> rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
> + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
... and here.
> + }
> } else {
> /* The intention is that in future we will
> * assume that full constraints are provided
>
These were just very minor things. Mostly looks good for me.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> 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 <[email protected]>
This looks great to me. Eg,
Acked-by: Matti Vaittinen <[email protected]>
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! ~~
Hi Matti!
On Mon, 3 Jul 2023 at 12:31, Matti Vaittinen <[email protected]> wrote:
> I hope your train back to home was not delayed too much ;)
Yes, much better this time :)
> On 6/20/23 23:03, Benjamin Bara wrote:
> > From: Benjamin Bara <[email protected]>
> >
> > The mon_disable_reg_disabled
>
> The name of this always makes me to scratch my head a bit. (or, maybe it
> is just the sunburns at my bald).
>
> Do you think making it:
> mon_disable_at_reg_disable or mon_disable_when_reg_disabled would be too
> long?
mons_to_disable_while_reg_off maybe fits better, or mons_off_while_reg_off.
Still not satisfied though, I will think about it - maybe something
better comes to
my mind.
> > property disables all dt-enabled monitors
> > before a regulator is disabled. If an error occurs while disabling the
> > regulator, the monitors are enabled again.
> >
> > Signed-off-by: Benjamin Bara <[email protected]>
> > ---
> > drivers/regulator/core.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 873e53633698..b37dcafff407 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
> >
> > trace_regulator_enable_complete(rdev_get_name(rdev));
> >
> > - return 0;
> > + return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> As I wrote in my comment to previous patch, I might find the logic a bit
> more clear if the condition check was done here. Eg:
>
> if (rdev->desc->mon_disable_when_reg_disabled)
> return monitors_reenable(...);
>
> return 0;
Yes, thanks. I applied this to all the mentioned occasions.
> > }
> >
> > /**
> > @@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
> >
> > static int _regulator_do_disable(struct regulator_dev *rdev)
> > {
> > + const struct regulator_desc *desc = rdev->desc;
> > int ret;
> >
> > + ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
> > + if (ret)
> > + return ret;
>
> Similarly, for me the logic would be easier to follow if this was:
>
> if (desc->mon_disable_when_reg_disabled)
> monitors_disable(...);
>
> > +
> > trace_regulator_disable(rdev_get_name(rdev));
> >
> > if (rdev->ena_pin) {
> > @@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
> > rdev->ena_gpio_state = 0;
> > }
> >
> > - } else if (rdev->desc->ops->disable) {
> > - ret = rdev->desc->ops->disable(rdev);
> > + } else if (desc->ops->disable) {
> > + ret = desc->ops->disable(rdev);
> > if (ret != 0)
> > return ret;
> > }
> >
> > - if (rdev->desc->off_on_delay)
> > + if (desc->off_on_delay)
> > rdev->last_off = ktime_get_boottime();
> >
> > trace_regulator_disable_complete(rdev_get_name(rdev));
> > @@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
> > _notifier_call_chain(rdev,
> > REGULATOR_EVENT_ABORT_DISABLE,
> > NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> same here,
>
> > return ret;
> > }
> > _notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> > @@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
> > rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
> > _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
> > REGULATOR_EVENT_ABORT_DISABLE, NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> here...
>
> > return ret;
> > }
> >
> > @@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
> > */
> > rdev_info(rdev, "disabling\n");
> > ret = _regulator_do_disable(rdev);
> > - if (ret != 0)
> > + if (ret != 0) {
> > rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> ... and here.
> > + }
> > } else {
> > /* The intention is that in future we will
> > * assume that full constraints are provided
> >
>
> These were just very minor things. Mostly looks good for me.
Thanks!
Benjamin
> Yours,
> -- Matti
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~
>
On Mon, 3 Jul 2023 at 12:31, Matti Vaittinen <[email protected]> wrote:
>
> Hi deeee Ho Benjamin,
>
> I hope your train back to home was not delayed too much ;)
>
> On 6/20/23 23:03, Benjamin Bara wrote:
> > From: Benjamin Bara <[email protected]>
> >
> > The mon_disable_reg_disabled
>
> The name of this always makes me to scratch my head a bit. (or, maybe it
> is just the sunburns at my bald).
>
> Do you think making it:
> mon_disable_at_reg_disable or mon_disable_when_reg_disabled would be too
> long?
>
> > property disables all dt-enabled monitors
> > before a regulator is disabled. If an error occurs while disabling the
> > regulator, the monitors are enabled again.
> >
> > Signed-off-by: Benjamin Bara <[email protected]>
> > ---
> > drivers/regulator/core.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 873e53633698..b37dcafff407 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
> >
> > trace_regulator_enable_complete(rdev_get_name(rdev));
> >
> > - return 0;
> > + return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> As I wrote in my comment to previous patch, I might find the logic a bit
> more clear if the condition check was done here. Eg:
>
> if (rdev->desc->mon_disable_when_reg_disabled)
> return monitors_reenable(...);
>
> return 0;
>
> > }
> >
> > /**
> > @@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
> >
> > static int _regulator_do_disable(struct regulator_dev *rdev)
> > {
> > + const struct regulator_desc *desc = rdev->desc;
> > int ret;
> >
> > + ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
> > + if (ret)
> > + return ret;
>
> Similarly, for me the logic would be easier to follow if this was:
>
> if (desc->mon_disable_when_reg_disabled)
> monitors_disable(...);
>
> > +
> > trace_regulator_disable(rdev_get_name(rdev));
> >
> > if (rdev->ena_pin) {
> > @@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
> > rdev->ena_gpio_state = 0;
> > }
> >
> > - } else if (rdev->desc->ops->disable) {
> > - ret = rdev->desc->ops->disable(rdev);
> > + } else if (desc->ops->disable) {
> > + ret = desc->ops->disable(rdev);
> > if (ret != 0)
> > return ret;
> > }
> >
> > - if (rdev->desc->off_on_delay)
> > + if (desc->off_on_delay)
> > rdev->last_off = ktime_get_boottime();
> >
> > trace_regulator_disable_complete(rdev_get_name(rdev));
> > @@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
> > _notifier_call_chain(rdev,
> > REGULATOR_EVENT_ABORT_DISABLE,
> > NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> same here,
>
> > return ret;
> > }
> > _notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> > @@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
> > rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
> > _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
> > REGULATOR_EVENT_ABORT_DISABLE, NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> here...
>
> > return ret;
> > }
> >
> > @@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
> > */
> > rdev_info(rdev, "disabling\n");
> > ret = _regulator_do_disable(rdev);
> > - if (ret != 0)
> > + if (ret != 0) {
> > rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> ... and here.
> > + }
> > } else {
> > /* The intention is that in future we will
> > * assume that full constraints are provided
> >
>
> These were just very minor things. Mostly looks good for me.
>
>
> Yours,
> -- Matti
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~
>