[PATCH RFC v3 0/5] regulator: dynamic voltage monitoring support

[PATCH RFC v3 0/5] regulator: dynamic voltage monitoring support

[Benjamin Bara]

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.
The new approach takes Matti's feedback regarding the bd718x7 into
account.

1/5 factors out the current monitor handling, to be able to re-use it
during the state changes.
2/5 disables monitoring of disabled regulators, also something mentioned
by Matti as a "requirement" of the bd718x7 driver.
3/5 adds a new op to get the active monitors/protections of a regulator,
which should help users to add enabled protections to their device-tree.
4/5 adds new properties to let the core know on which state changes the
monitors of a regulator need to be en-/disabled.
5/5 is an example how the bd718x7 could be converted to use the new
property (untested, as no hw available).

Thanks & best regards,
Benjamin

---
Changes in v3:
- rebase to v6.4-rc2, to get voltage monitoring of da9063
- re-use the existing monitor handling
- set initial monitoring state to regulator state
- disable monitoring before disabling regulators (if protection is
  activated in dt)
- add new op to get the active protections of a regulator
- distinguish between switching to higher or lower voltage
- re-activate old monitoring state if regulator's state change fails
- convert bd718x7 monitor handling to use the state change property

Link to v2: https://lore.kernel.org/r/20230419-dynamic-vmon-v2-0-c303bcc75ebc@skidata.com

Changes in v2:
1/2:
- move from board-specific (machine.h) to driver-specific (driver.h)
- move from own struct to fields/properties in regulator_desc
- handle modes as one "unsupported modes" field
- factor out new monitors_set_state() to handle all (activated) monitors
- move re-enabling of monitor after ramp-delay
- add TODOs for error handling when the action fails (return error from
  actual action instead, return state of monitoring to pre-action).
- reword commit message
2/2:
- adapting change to the properties approach

Link to v1: https://lore.kernel.org/r/20230419-dynamic-vmon-v1-0-f48b7438e891@skidata.com

---
Benjamin Bara (5):
      regulator: move monitor handling into own function
      regulator: disable monitors when regulator is disabled
      regulator: add getter for active monitors
      regulator: add properties to handle monitoring on state change
      regulator: bd718x7: let the core handle the monitors

 drivers/regulator/bd718x7-regulator.c | 206 ++++++++++---------------------
 drivers/regulator/core.c              | 224 ++++++++++++++++++++++++++--------
 include/linux/regulator/driver.h      |  22 ++++
 3 files changed, 262 insertions(+), 190 deletions(-)
---
base-commit: f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6
change-id: 20230419-dynamic-vmon-e08daa0ac7ad

Best regards,
-- 
Benjamin Bara <benjamin.bara@skidata.com>

[PATCH RFC v3 1/5] regulator: move monitor handling into own function

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Similar to the existing implementation, the new function does not handle
EOPNOTSUPP as an error. The initial monitoring state is set to the
regulator state.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/regulator/core.c | 134 ++++++++++++++++++++++++++++-------------------
 1 file changed, 80 insertions(+), 54 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index dc741ac156c3..76f112817f9d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1426,7 +1426,7 @@ static int notif_set_limit(struct regulator_dev *rdev,
 
 static int handle_notify_limits(struct regulator_dev *rdev,
 			int (*set)(struct regulator_dev *, int, int, bool),
-			struct notification_limit *limits)
+			const struct notification_limit *limits)
 {
 	int ret = 0;
 
@@ -1451,6 +1451,80 @@ static int handle_notify_limits(struct regulator_dev *rdev,
 
 	return ret;
 }
+
+static const struct notification_limit disable_limits = {
+	.prot = REGULATOR_NOTIF_LIMIT_DISABLE,
+	.err = REGULATOR_NOTIF_LIMIT_DISABLE,
+	.warn = REGULATOR_NOTIF_LIMIT_DISABLE,
+};
+
+static int monitors_set_state(struct regulator_dev *rdev, bool enable)
+{
+	const struct regulation_constraints *reg_c = rdev->constraints;
+	const struct regulator_ops *ops = rdev->desc->ops;
+	int ret;
+
+	/* only set the state if monitoring is activated in the device-tree. */
+	if (reg_c->over_voltage_detection) {
+		ret = handle_notify_limits(rdev, ops->set_over_voltage_protection,
+					   enable ? &reg_c->over_voltage_limits
+					   : &disable_limits);
+		if (ret) {
+			if (ret != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set over voltage limits %pe\n",
+					 ERR_PTR(ret));
+				return ret;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested over voltage limits\n");
+		}
+	}
+	if (reg_c->under_voltage_detection) {
+		ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
+					   enable ? &reg_c->under_voltage_limits
+					   : &disable_limits);
+		if (ret) {
+			if (ret != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set under voltage limits %pe\n",
+					 ERR_PTR(ret));
+				return ret;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested under voltage limits\n");
+		}
+	}
+	if (reg_c->over_current_detection) {
+		ret = handle_notify_limits(rdev, ops->set_over_current_protection,
+					   enable ? &reg_c->over_curr_limits
+					   : &disable_limits);
+		if (ret) {
+			if (ret != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set over current limits: %pe\n",
+					 ERR_PTR(ret));
+				return ret;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested over-current limits\n");
+		}
+	}
+	if (reg_c->over_temp_detection) {
+		ret = handle_notify_limits(rdev, ops->set_thermal_protection,
+					   enable ? &reg_c->temp_limits
+					   : &disable_limits);
+		if (ret) {
+			if (ret != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set temperature limits %pe\n",
+					 ERR_PTR(ret));
+				return ret;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested temperature limits\n");
+		}
+	}
+
+	return 0;
+}
+
 /**
  * set_machine_constraints - sets regulator constraints
  * @rdev: regulator source
@@ -1564,60 +1638,12 @@ static int set_machine_constraints(struct regulator_dev *rdev)
 		}
 	}
 
-	if (rdev->constraints->over_current_detection)
-		ret = handle_notify_limits(rdev,
-					   ops->set_over_current_protection,
-					   &rdev->constraints->over_curr_limits);
-	if (ret) {
-		if (ret != -EOPNOTSUPP) {
-			rdev_err(rdev, "failed to set over current limits: %pe\n",
-				 ERR_PTR(ret));
-			return ret;
-		}
-		rdev_warn(rdev,
-			  "IC does not support requested over-current limits\n");
-	}
-
-	if (rdev->constraints->over_voltage_detection)
-		ret = handle_notify_limits(rdev,
-					   ops->set_over_voltage_protection,
-					   &rdev->constraints->over_voltage_limits);
-	if (ret) {
-		if (ret != -EOPNOTSUPP) {
-			rdev_err(rdev, "failed to set over voltage limits %pe\n",
-				 ERR_PTR(ret));
-			return ret;
-		}
-		rdev_warn(rdev,
-			  "IC does not support requested over voltage limits\n");
-	}
-
-	if (rdev->constraints->under_voltage_detection)
-		ret = handle_notify_limits(rdev,
-					   ops->set_under_voltage_protection,
-					   &rdev->constraints->under_voltage_limits);
-	if (ret) {
-		if (ret != -EOPNOTSUPP) {
-			rdev_err(rdev, "failed to set under voltage limits %pe\n",
-				 ERR_PTR(ret));
-			return ret;
-		}
-		rdev_warn(rdev,
-			  "IC does not support requested under voltage limits\n");
-	}
-
-	if (rdev->constraints->over_temp_detection)
-		ret = handle_notify_limits(rdev,
-					   ops->set_thermal_protection,
-					   &rdev->constraints->temp_limits);
-	if (ret) {
-		if (ret != -EOPNOTSUPP) {
-			rdev_err(rdev, "failed to set temperature limits %pe\n",
-				 ERR_PTR(ret));
+	/* set initial monitor state to current regulator state. */
+	ret = _regulator_is_enabled(rdev);
+	if (ret >= 0) {
+		ret = monitors_set_state(rdev, !!ret);
+		if (ret)
 			return ret;
-		}
-		rdev_warn(rdev,
-			  "IC does not support requested temperature limits\n");
 	}
 
 	if (rdev->constraints->active_discharge && ops->set_active_discharge) {

-- 
2.34.1

[PATCH RFC v3 2/5] regulator: disable monitors when regulator is disabled

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

This disables all enabled monitors before a regulator is disabled. This
only happens if an protection is enabled in the device-tree. If an error
occurs while disabling the regulator, the monitors are enabled again.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/regulator/core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 76f112817f9d..e59204920d6c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2828,7 +2828,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 
 	trace_regulator_enable_complete(rdev_get_name(rdev));
 
-	return 0;
+	return monitors_set_state(rdev, true);
 }
 
 /**
@@ -2989,6 +2989,10 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 {
 	int ret;
 
+	ret = monitors_set_state(rdev, false);
+	if (ret)
+		return ret;
+
 	trace_regulator_disable(rdev_get_name(rdev));
 
 	if (rdev->ena_pin) {
@@ -3043,6 +3047,7 @@ static int _regulator_disable(struct regulator *regulator)
 				_notifier_call_chain(rdev,
 						REGULATOR_EVENT_ABORT_DISABLE,
 						NULL);
+				monitors_set_state(rdev, true);
 				return ret;
 			}
 			_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
@@ -3109,6 +3114,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_set_state(rdev, true);
 		return ret;
 	}
 
@@ -6251,8 +6257,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_set_state(rdev, true);
+		}
 	} else {
 		/* The intention is that in future we will
 		 * assume that full constraints are provided

-- 
2.34.1

[PATCH RFC v3 3/5] regulator: add getter for active monitors

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Add an op to get all active monitors of a regulator.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 include/linux/regulator/driver.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d3b4a3d4514a..35547e9eca48 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -25,6 +25,12 @@ struct regulator_config;
 struct regulator_init_data;
 struct regulator_enable_gpio;
 
+#define REGULATOR_MONITOR_INVALID 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
+
 enum regulator_status {
 	REGULATOR_STATUS_OFF,
 	REGULATOR_STATUS_ON,
@@ -112,6 +118,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_*. REGULATOR_MONITOR_INVALID means no one active.
  *
  * @set_active_discharge: Set active discharge enable/disable of regulators.
  *
@@ -183,6 +191,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

[PATCH RFC v3 4/5] regulator: add properties to handle monitoring on state change

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

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().

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/regulator/core.c         | 90 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h | 13 ++++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e59204920d6c..98a9283a0322 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1638,6 +1638,34 @@ static int set_machine_constraints(struct regulator_dev *rdev)
 		}
 	}
 
+	/*
+	 * when the core is in charge of disabling monitors while the regulator
+	 * changes its value, it is essential to know if a monitor is active.
+	 */
+	if (rdev->desc->mon_disable_reg_set_higher ||
+	    rdev->desc->mon_disable_reg_set_lower) {
+		unsigned int monitor_state = REGULATOR_MONITOR_INVALID;
+
+		ret = ops->get_active_protections(rdev, &monitor_state);
+		if (ret)
+			return ret;
+
+		/*
+		 * when a monitor is active, without dt knowing, we have to
+		 * adapt the constraints to ensure functionality.
+		 */
+		if (!rdev->constraints->over_voltage_detection &&
+		    (monitor_state & REGULATOR_MONITOR_OVER_VOLTAGE)) {
+			rdev_warn(rdev, "dt unaware of over-voltage protection!\n");
+			rdev->constraints->over_voltage_detection = 1;
+		}
+		if (!rdev->constraints->under_voltage_detection &&
+		    (monitor_state & REGULATOR_MONITOR_UNDER_VOLTAGE)) {
+			rdev_warn(rdev, "dt unaware of under-voltage protection!\n");
+			rdev->constraints->under_voltage_detection = 1;
+		}
+	}
+
 	/* set initial monitor state to current regulator state. */
 	ret = _regulator_is_enabled(rdev);
 	if (ret >= 0) {
@@ -3516,6 +3544,15 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
 	if (ret & NOTIFY_STOP_MASK)
 		return -EINVAL;
 
+	if ((rdev->desc->mon_disable_reg_set_higher &&
+	    (min_uV > data.old_uV || max_uV > data.old_uV)) ||
+	    (rdev->desc->mon_disable_reg_set_lower &&
+	    (min_uV < data.old_uV || max_uV < data.old_uV))) {
+		ret = monitors_set_state(rdev, false);
+		if (ret)
+			return ret;
+	}
+
 	ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
 	if (ret >= 0)
 		return ret;
@@ -3540,6 +3577,13 @@ static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
 	if (ret & NOTIFY_STOP_MASK)
 		return -EINVAL;
 
+	if ((rdev->desc->mon_disable_reg_set_higher && uV > data.old_uV) ||
+	    (rdev->desc->mon_disable_reg_set_lower && uV < data.old_uV)) {
+		ret = monitors_set_state(rdev, false);
+		if (ret)
+			return ret;
+	}
+
 	ret = rdev->desc->ops->set_voltage_sel(rdev, selector);
 	if (ret >= 0)
 		return ret;
@@ -3736,6 +3780,15 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 out:
 	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
 
+	if ((rdev->desc->mon_disable_reg_set_higher || rdev->desc->mon_disable_reg_set_lower) &&
+	    _regulator_is_enabled(rdev) > 0) {
+		/* if setting voltage failed, ignore monitoring error. */
+		if (ret)
+			monitors_set_state(rdev, true);
+		else
+			ret = monitors_set_state(rdev, true);
+	}
+
 	return ret;
 }
 
@@ -4645,7 +4698,24 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
 	if (ret < 0)
 		goto out;
 
+	if (mode & rdev->desc->mon_unsupported_reg_modes) {
+		ret = monitors_set_state(rdev, false);
+		if (ret)
+			goto out;
+	}
+
 	ret = rdev->desc->ops->set_mode(rdev, mode);
+	if (ret) {
+		/* get_mode() is required: regulator_curr_mode should be valid. */
+		if ((regulator_curr_mode & ~rdev->desc->mon_unsupported_reg_modes) &&
+		    _regulator_is_enabled(rdev) > 0)
+			monitors_set_state(rdev, true);
+		goto out;
+	}
+
+	if ((mode & ~rdev->desc->mon_unsupported_reg_modes) && _regulator_is_enabled(rdev) > 0)
+		ret = monitors_set_state(rdev, true);
+
 out:
 	regulator_unlock(rdev);
 	return ret;
@@ -5572,6 +5642,26 @@ regulator_register(struct device *dev,
 		goto rinse;
 	}
 
+	/*
+	 * mon_unsupported_reg_modes property requires get_mode() to get the old
+	 * state in case a state switch is failing.
+	 */
+	if (regulator_desc->mon_unsupported_reg_modes &&
+	    !regulator_desc->ops->get_mode) {
+		ret = -EINVAL;
+		goto rinse;
+	}
+	/*
+	 * mon_disable_reg_set_* property requires get_active_protections() to
+	 * know if a regulator is monitored without the device-tree knowing it.
+	 */
+	if ((regulator_desc->mon_disable_reg_set_higher ||
+	    regulator_desc->mon_disable_reg_set_lower) &&
+	    !regulator_desc->ops->get_active_protections) {
+		ret = -EINVAL;
+		goto rinse;
+	}
+
 	rdev = kzalloc(sizeof(struct regulator_dev), GFP_KERNEL);
 	if (rdev == NULL) {
 		ret = -ENOMEM;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 35547e9eca48..bf42a5f452c5 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -366,6 +366,15 @@ 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_set_higher: Disables regulator's monitors while it is
+ *                              changing its value to a higher one.
+ * @mon_disable_reg_set_lower: Disables regulator's monitors while it is
+ *                             changing its value to a lower one.
+ * @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;
@@ -440,6 +449,10 @@ struct regulator_desc {
 	unsigned int poll_enabled_time;
 
 	unsigned int (*of_map_mode)(unsigned int mode);
+
+	unsigned int mon_disable_reg_set_higher;
+	unsigned int mon_disable_reg_set_lower;
+	unsigned int mon_unsupported_reg_modes;
 };
 
 /**

-- 
2.34.1

[PATCH RFC v3 5/5] regulator: bd718x7: let the core handle the monitors

[Benjamin Bara]

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.

.mon_disable_reg_set_higher requires get_active_protections() to find
out if a regulator is monitored without the device-tree knowing it.
Otherwise, this might lead to a failure as the core might not be aware
that a regulator is monitored and therefore would not disable it.
Therefore, implement it.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/regulator/bd718x7-regulator.c | 206 +++++++++++-----------------------
 1 file changed, 66 insertions(+), 140 deletions(-)

diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index b0b9938c20a1..897388d68949 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -56,7 +56,7 @@
 
 #define BD718XX_OPS(name, _list_voltage, _map_voltage, _set_voltage_sel, \
 		   _get_voltage_sel, _set_voltage_time_sel, _set_ramp_delay, \
-		   _set_uvp, _set_ovp)				\
+		   _set_uvp, _set_ovp, _get_prot)		\
 static const struct regulator_ops name = {			\
 	.enable = regulator_enable_regmap,			\
 	.disable = regulator_disable_regmap,			\
@@ -69,6 +69,7 @@ static const struct regulator_ops name = {			\
 	.set_ramp_delay = (_set_ramp_delay),			\
 	.set_under_voltage_protection = (_set_uvp),		\
 	.set_over_voltage_protection = (_set_ovp),		\
+	.get_active_protections = (_get_prot),			\
 };								\
 								\
 static const struct regulator_ops BD718XX_HWOPNAME(name) = {	\
@@ -81,6 +82,7 @@ static const struct regulator_ops BD718XX_HWOPNAME(name) = {	\
 	.set_ramp_delay = (_set_ramp_delay),			\
 	.set_under_voltage_protection = (_set_uvp),		\
 	.set_over_voltage_protection = (_set_ovp),		\
+	.get_active_protections = (_get_prot),			\
 }								\
 
 /*
@@ -126,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)
 {
@@ -457,6 +337,21 @@ static int bd718x7_xvp_sanity_check(struct regulator_dev *rdev, int lim_uV,
 	return 0;
 }
 
+static int bd717x7_get_ldo_prot(struct regulator_dev *rdev, unsigned int *val)
+{
+	int ldo_offset = rdev->desc->id - BD718XX_LDO1;
+	int prot_bit = BD718XX_LDO1_VRMON80 << ldo_offset;
+	int ret;
+
+	ret = regmap_test_bits(rdev->regmap, BD718XX_REG_MVRFLTMASK2, prot_bit);
+	if (ret < 0)
+		return ret;
+	if (ret)
+		*val |= REGULATOR_MONITOR_UNDER_VOLTAGE;
+
+	return 0;
+}
+
 static int bd718x7_set_ldo_uvp(struct regulator_dev *rdev, int lim_uV,
 			       int severity, bool enable)
 {
@@ -519,6 +414,31 @@ static int bd718x7_get_buck_uvp_info(int id, int *reg, int *bit)
 	return 0;
 }
 
+static int bd717x7_get_buck_prot(struct regulator_dev *rdev, unsigned int *val)
+{
+	int ret, reg, bit;
+
+	ret = bd718x7_get_buck_uvp_info(rdev->desc->id, &reg, &bit);
+	if (ret)
+		return ret;
+	ret = regmap_test_bits(rdev->regmap, reg, bit);
+	if (ret < 0)
+		return ret;
+	if (ret)
+		*val |= REGULATOR_MONITOR_UNDER_VOLTAGE;
+
+	ret = bd718x7_get_buck_ovp_info(rdev->desc->id, &reg, &bit);
+	if (ret)
+		return ret;
+	ret = regmap_test_bits(rdev->regmap, reg, bit);
+	if (ret < 0)
+		return ret;
+	if (ret)
+		*val |= REGULATOR_MONITOR_OVER_VOLTAGE;
+
+	return 0;
+}
+
 static int bd718x7_set_buck_uvp(struct regulator_dev *rdev, int lim_uV,
 				int severity, bool enable)
 {
@@ -564,15 +484,15 @@ 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);
+	    bd718x7_set_ldo_uvp, NULL, bd717x7_get_ldo_prot);
 
 /* BD71847 and BD71850 LDO 5 is by default OFF at RUN state */
 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,
 };
@@ -582,27 +502,27 @@ BD718XX_OPS(bd718xx_pickable_range_buck_ops,
 	    regulator_set_voltage_sel_pickable_regmap,
 	    regulator_get_voltage_sel_pickable_regmap,
 	    regulator_set_voltage_time_sel, NULL, bd718x7_set_buck_uvp,
-	    bd718x7_set_buck_ovp);
+	    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);
+	    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);
+	    NULL, bd717x7_get_ldo_prot);
 
 BD718XX_OPS(bd718xx_buck_regulator_ops, regulator_list_voltage_linear_range,
 	    NULL, regulator_set_voltage_sel_regmap,
 	    regulator_get_voltage_sel_regmap, regulator_set_voltage_time_sel,
-	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp);
+	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
 
 BD718XX_OPS(bd718xx_buck_regulator_nolinear_ops, regulator_list_voltage_table,
 	    regulator_map_voltage_ascend, regulator_set_voltage_sel_regmap,
 	    regulator_get_voltage_sel_regmap, regulator_set_voltage_time_sel,
-	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp);
+	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
 
 /*
  * OPS for BD71837
@@ -611,34 +531,34 @@ BD718XX_OPS(bd71837_pickable_range_ldo_ops,
 	    regulator_list_voltage_pickable_linear_range, NULL,
 	    bd71837_set_voltage_sel_pickable_restricted,
 	    regulator_get_voltage_sel_pickable_regmap, NULL, NULL,
-	    bd718x7_set_ldo_uvp, NULL);
+	    bd718x7_set_ldo_uvp, NULL, bd717x7_get_ldo_prot);
 
 BD718XX_OPS(bd71837_pickable_range_buck_ops,
 	    regulator_list_voltage_pickable_linear_range, NULL,
 	    bd71837_set_voltage_sel_pickable_restricted,
 	    regulator_get_voltage_sel_pickable_regmap,
 	    regulator_set_voltage_time_sel, NULL, bd718x7_set_buck_uvp,
-	    bd718x7_set_buck_ovp);
+	    bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
 
 BD718XX_OPS(bd71837_ldo_regulator_ops, regulator_list_voltage_linear_range,
 	    NULL, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
-	    NULL);
+	    NULL, bd717x7_get_ldo_prot);
 
 BD718XX_OPS(bd71837_ldo_regulator_nolinear_ops, regulator_list_voltage_table,
 	    NULL, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
-	    NULL);
+	    NULL, bd717x7_get_ldo_prot);
 
 BD718XX_OPS(bd71837_buck_regulator_ops, regulator_list_voltage_linear_range,
 	    NULL, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, regulator_set_voltage_time_sel,
-	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp);
+	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
 
 BD718XX_OPS(bd71837_buck_regulator_nolinear_ops, regulator_list_voltage_table,
 	    regulator_map_voltage_ascend, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, regulator_set_voltage_time_sel,
-	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp);
+	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
 /*
  * BD71837 bucks 3 and 4 support defining their enable/disable state also
  * when buck enable state is under HW state machine control. In that case the
@@ -662,7 +582,7 @@ BD718XX_OPS(bd718xx_dvs_buck_regulator_ops, regulator_list_voltage_linear_range,
 	    NULL, regulator_set_voltage_sel_regmap,
 	    regulator_get_voltage_sel_regmap, regulator_set_voltage_time_sel,
 	    regulator_set_ramp_delay_regmap, bd718x7_set_buck_uvp,
-	    bd718x7_set_buck_ovp);
+	    bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
 
 
 
@@ -1772,6 +1692,12 @@ static int bd718xx_probe(struct platform_device *pdev)
 		else
 			desc->ops = swops[i];
 
+		/*
+		 * bd718x7 requires to disable a regulator's monitors while it
+		 * changes to a higher value.
+		 */
+		desc->mon_disable_reg_set_higher = 1;
+
 		rdev = devm_regulator_register(&pdev->dev, desc, &config);
 		if (IS_ERR(rdev))
 			return dev_err_probe(&pdev->dev, PTR_ERR(rdev),

-- 
2.34.1

Re: [PATCH RFC v3 1/5] regulator: move monitor handling into own function

[Matti Vaittinen]

Hi Benjamin,

Thanks for working on this. :)

On 5/21/23 14:39, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Similar to the existing implementation, the new function does not handle
> EOPNOTSUPP as an error. The initial monitoring state is set to the
> regulator state.


As far as I see, this changes the existing logic. Previously the 
monitoring was unconditionally enabled for all regulators, now it gets 
only enabled for regulators which are marked as enabled.

Furthermore, if I am not reading this wrong, the code tries to disable 
all protections if regulator is not enabled at startup(?)

I am not saying this is wrong. I am just saying that things will change 
here and likely to break something.

There are PMICs like ROHM BD9576, where the protection can not be disabled.

For example, the bd9576_set_uvp() has:
         if (severity == REGULATOR_SEVERITY_PROT) {
                 if (!enable || lim_uV)
                         return -EINVAL;
                 return 0;
         }

I am unsure if we might also have cases where some regulator could 
really be enabled w/o core knowing it. There can also be a problem if we 
have hardware where monitoring is common for all regulators, eg either 
globally enabled / disabled.

Yours,
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH RFC v3 1/5] regulator: move monitor handling into own function

[Benjamin Bara]

Hi Matti,

thanks for the feedback!

On Tue, 23 May 2023 at 11:46, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> As far as I see, this changes the existing logic. Previously the
> monitoring was unconditionally enabled for all regulators, now it gets
> only enabled for regulators which are marked as enabled.
>
> Furthermore, if I am not reading this wrong, the code tries to disable
> all protections if regulator is not enabled at startup(?)
>
> I am not saying this is wrong. I am just saying that things will
> change here and likely to break something.
>
> There are PMICs like ROHM BD9576, where the protection can not be
> disabled.

Thanks for letting me know! I dropped my initial "disable monitor while
disabling the regulator" property, and activated it per default instead.
But this basically means something like that will be required. I guess
it might make sense to have a property which is called something like
"monitor always on", to let the driver inform the core that the monitors
cannot or should not be disabled, instead.
Except if you think there is a general problem with keeping monitors
disabled while the regulator is disabled, then I might have to do it
differently.


> I am unsure if we might also have cases where some regulator could
> really be enabled w/o core knowing it. 

Unfortunately, I am not 100% sure what you mean by that.
On the da9063, for example, it might be possible that a monitor is
activated by the OTP, without that the kernel actually activates it.
I think it is not recommended, but it is possible.


> There can also be a problem if we have hardware where monitoring is
> common for all regulators, eg either globally enabled / disabled.

Yes, but I think in this case it should be the responsibility of the
driver to ensure that either all or no regulator is monitored, because
the same requirement is valid for implementing the protection ops.

Best regards,
Benjamin

Re: [PATCH RFC v3 1/5] regulator: move monitor handling into own function

[Matti Vaittinen]

On 5/23/23 14:51, Benjamin Bara wrote:
> Hi Matti,
> 
> thanks for the feedback!
> 
> On Tue, 23 May 2023 at 11:46, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>> As far as I see, this changes the existing logic. Previously the
>> monitoring was unconditionally enabled for all regulators, now it gets
>> only enabled for regulators which are marked as enabled.
>>
>> Furthermore, if I am not reading this wrong, the code tries to disable
>> all protections if regulator is not enabled at startup(?)
>>
>> I am not saying this is wrong. I am just saying that things will
>> change here and likely to break something.
>>
>> There are PMICs like ROHM BD9576, where the protection can not be
>> disabled.
> 
> Thanks for letting me know! I dropped my initial "disable monitor while
> disabling the regulator" property, and activated it per default instead.
> But this basically means something like that will be required. I guess
> it might make sense to have a property which is called something like
> "monitor always on", to let the driver inform the core that the monitors
> cannot or should not be disabled, instead. > Except if you think there is a general problem with keeping monitors
> disabled while the regulator is disabled, then I might have to do it
> differently.

I am thinking that maybe the default should still be to not touch the 
monitoring unless explicitly requested. My thinking is that the hardware 
should by default be able to handle the voltage change / enable / 
disable etc while monitoring is enabled. Hardware which requires 
explicit monitoring disabling sounds (to me) like a 'design problem' and 
disabling the monitoring sounds (to me) like a workaround. I wouldn't 
make this workaround default. Furthermore, monitoring is a safety 
feature, and as such core should not autonomously disable it (unless 
such behaviour is requested). Well, experience has proven that my 
thinking is not _always_ right, so feel free to voice other opinions :)

>> I am unsure if we might also have cases where some regulator could
>> really be enabled w/o core knowing it.
> 
> Unfortunately, I am not 100% sure what you mean by that.

I was thinking of a case where regulator state is not readable - I'm not 
100% sure how core thinks of their state. Another case could be a 
regulator which is not registered to the core but shares monitoring with 
some other regulator. This falls under the common monitoring category 
mentioned below.

> On the da9063, for example, it might be possible that a monitor is
> activated by the OTP, without that the kernel actually activates it.
> I think it is not recommended, but it is possible.
> 
> 
>> There can also be a problem if we have hardware where monitoring is
>> common for all regulators, eg either globally enabled / disabled.
> 
> Yes, but I think in this case it should be the responsibility of the
> driver to ensure that either all or no regulator is monitored, because
> the same requirement is valid for implementing the protection ops.

If I didn't misread the code, the differences here are that existing 
"ideology" is to a) only touch the monitoring (enable/disable) when 
explicitly requested for a given level and b) knowing that all monitors 
that are requested to be enabled are enabled at the end of the probe.

In my eyes change a) is problematic. For example, if a board using 
BD9576 wants to have protection disabled via device-tree (let's assume 
there is a board where we know that some disturbance to voltages will 
occur under specific conditions) - it is very valid to complain 
disabling protection is not supported. Go fix your board design message 
needs to be given because protection can't be disabled. This is very 
different from case where we just try disabling monitoring because 
regulator is turned off. In latter case with BD9576 the failure to 
disable protection should just be silently ignored. When we use same 
callbacks for both the initial configuration and the runtime 
enable/disable/voltage-change handling the driver has no way knowing if 
this is an error or not. Writing this leads me back to thinking that the 
monitor configuration for enable/disable/voltage-change should be done 
via separate driver callback - that would allow driver to separate these 
use-cases. If this was change I wrote, I might try creating separate 
driver callbacks for 
enable/disable/voltage_change_start/voltage_change_done which get the 
initial monitor configuration (as was read from device-tree) as an 
argument. Do you think that could give the flexibility to handle all 
different hardware quirks?

The change b) does also have consequences. Some PMICs like the BD9576 do 
use same IRQ for indicating either ERROR or WARNING level problem. 
Whether to use WARNING or ERROR is selected at star-up when the 
device-tree flags are read. Eg, the .set_<XXX>_protection callbacks 
store the severity information (WARNING or ERROR) and complain if both 
are tried to be used. With the current approach we know the validity of 
this configuration is checked right when regulator is registered, not 
later at runtime when regulator is enabled.

Another example regarding design that uses the knowledge that all 
requested monitors are enabled when regulator is registered is BD96801 - 
which is not upstream (although I've had patches in my outbox for an 
year already waiting for permission from the HQ to actually send them... 
Don't ask...). This PMIC can configure fatality of the fault monitoring. 
This driver checks that all regulators did agree on whether to use 
PROTECTION or ERROR/WARNING level monitoring at the end of the probe - 
and toggles the IRQ fatality accordingly. I truly believe that 
out-of-tree drivers must not mandate upstream design - but I equally 
believe that we may see similar HW designs in upstream and considering 
this now makes sense :) Yes, in order to paper over b) a driver can for 
sure go and parse all the monitoring properties from device-tree itself 
and decide things based on that - but it might be quite a lot of 
duplicated code.

To sum up my view - I do definitely like the idea that core supports 
toggling the monitors for duration of enable/disable/voltage-change as 
this is needed by some real world ICs.

I, however, think drivers should be able to separate the "set the 
default monitoring config" request from the "change config to something 
we use for duration of this operation" - because the best monitoring 
config that is required for an operation may not be a "disable all". 
Hence, we should leave it for the driver to decide what config to set 
for the duration of an enable/disable/voltage_set-operation.

Furthermore, I believe the default should be "don't touch the 
monitoring" and not to try disable/enable it w/o explicit request.

Again, thank you for working on this and including me in the discussion :)

Yours,
	-- Matti



-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH RFC v3 1/5] regulator: move monitor handling into own function

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On Wed, May 24, 2023 at 10:28:10AM +0300, Matti Vaittinen wrote:

> I am thinking that maybe the default should still be to not touch the
> monitoring unless explicitly requested. My thinking is that the hardware

This is the general approach of the regulator API, we require explicit
permission to change any hardware setting since that way anything we do
that's unsafe for the hardware was the result of explicit permissions
rather than a software decision.

> > > I am unsure if we might also have cases where some regulator could
> > > really be enabled w/o core knowing it.

> > Unfortunately, I am not 100% sure what you mean by that.

> I was thinking of a case where regulator state is not readable - I'm not
> 100% sure how core thinks of their state. Another case could be a regulator
> which is not registered to the core but shares monitoring with some other
> regulator. This falls under the common monitoring category mentioned below.

I'd expect that a regulator which supports monitoring will have at least
the requested state readable so it wouldn't come up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC v3 1/5] regulator: move monitor handling into own function

[Benjamin Bara]

Hi Matti!

On Wed, 24 May 2023 at 09:28, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> I am thinking that maybe the default should still be to not touch the
> monitoring unless explicitly requested.

Got it - I will bring back the "mon_disable_reg_disabled" property. With
this property, the current behaviour will be in-place.

> My thinking is that the hardware should by default be able to handle
> the voltage change / enable / disable etc while monitoring is enabled.
> Hardware which requires explicit monitoring disabling sounds (to me)
> like a 'design problem' and disabling the monitoring sounds (to me)
> like a workaround. I wouldn't make this workaround default.
> Furthermore, monitoring is a safety feature, and as such core should
> not autonomously disable it (unless such behaviour is requested).

I totally agree with you here. However, there are regulators that
require such workarounds (e.g. bd718x7 and da9063) and that's the reason
for this series.

> I was thinking of a case where regulator state is not readable - I'm
> not 100% sure how core thinks of their state.

AFAIK, they would be considered as always on. But as Mark said, we could
add this as a requirement for having protection.

> Another case could be a regulator which is not registered to the core
> but shares monitoring with some other regulator. 

I think this case should be handled by the driver anyways. Activating a
shared protection on one regulator, without activating it on the other
regulator should be considered as an error in my opinion.

> If I didn't misread the code, the differences here are that existing
> "ideology" is to a) only touch the monitoring (enable/disable) when
> explicitly requested for a given level and b) knowing that all
> monitors that are requested to be enabled are enabled at the end of
> the probe.
>
> In my eyes change a) is problematic. For example, if a board using
> BD9576 wants to have protection disabled via device-tree (let's assume
> there is a board where we know that some disturbance to voltages will
> occur under specific conditions) - it is very valid to complain
> disabling protection is not supported. 

Yes, I think so too. I would not give the BD9576 any new "workaround
property", which would lead to the behaviour which is currently
implemented, meaning the monitoring is not touched after initialization.

> Go fix your board design message needs to be given because protection
> can't be disabled. This is very different from case where we just try
> disabling monitoring because regulator is turned off. In latter case
> with BD9576 the failure to disable protection should just be silently
> ignored. When we use same callbacks for both the initial configuration
> and the runtime enable/disable/voltage-change handling the driver has
> no way knowing if this is an error or not.

Got it. I am aware now that there are PMICs which do not allow to turn
off the monitor, therefore the default behaviour will be the same as
now. For now, only the da9063 (invalid state when monitoring a disabled
monitor) and the bd718x7 (invalid state when monitoring an enabled
regulator that switches to a higher voltage) are affected by the new
properties. The others which currently have {O,U}VP (max597x, bd9576)
should stay the same as now.

> Therefore, I will switch back to only do it when the monitor
> configuration for enable/disable/voltage-change should be done via
> separate driver callback - that would allow driver to separate these
> use-cases. If this was change I wrote, I might try creating separate
> driver callbacks for
> enable/disable/voltage_change_start/voltage_change_done which get the
> initial monitor configuration (as was read from device-tree) as an
> argument. Do you think that could give the flexibility to handle all
> different hardware quirks?

I think it would, yes. But I also think that it will lead to a lot of
duplicate code. However, instead of a simple "enable/disable" property,
we could reuse the "type of protection" too, to create some kind of
matrix. Example: Instead of setting mon_disable_reg_set_higher to 1 for
the bd718x7, we could set it to REGULATOR_MONITOR_OVER_VOLTAGE, meaning
just this protection should be disabled while switching to the higher
voltage. What do you think about that?

> The change b) does also have consequences. Some PMICs like the BD9576
> do use same IRQ for indicating either ERROR or WARNING level problem.
> Whether to use WARNING or ERROR is selected at star-up when the
> device-tree flags are read. Eg, the .set_<XXX>_protection callbacks
> store the severity information (WARNING or ERROR) and complain if both
> are tried to be used. With the current approach we know the validity
> of this configuration is checked right when regulator is registered,
> not later at runtime when regulator is enabled.

Not sure about that, but I think it would fail to register the
regulator? In this case, later it would not be able to enable it because
it is not registered, right?

> Another example regarding design that uses the knowledge that all
> requested monitors are enabled when regulator is registered is BD96801
> - which is not upstream (although I've had patches in my outbox for an
> year already waiting for permission from the HQ to actually send
> them... Don't ask...). This PMIC can configure fatality of the fault
> monitoring. This driver checks that all regulators did agree on
> whether to use PROTECTION or ERROR/WARNING level monitoring at the end
> of the probe - and toggles the IRQ fatality accordingly. I truly
> believe that out-of-tree drivers must not mandate upstream design -
> but I equally believe that we may see similar HW designs in upstream
> and considering this now makes sense :) Yes, in order to paper over b)
> a driver can for sure go and parse all the monitoring properties from
> device-tree itself and decide things based on that - but it might be
> quite a lot of duplicated code.

From my point of view, the behaviour will stay exactly the same! If they
don't agree on the same level, the probe should actually fail and the
regulators should not be registered. 

> To sum up my view - I do definitely like the idea that core supports
> toggling the monitors for duration of enable/disable/voltage-change as
> this is needed by some real world ICs.
>
> I, however, think drivers should be able to separate the "set the
> default monitoring config" request from the "change config to
> something we use for duration of this operation" - because the best
> monitoring config that is required for an operation may not be a
> "disable all". Hence, we should leave it for the driver to decide what
> config to set for the duration of an
> enable/disable/voltage_set-operation.
>
> Furthermore, I believe the default should be "don't touch the
> monitoring" and not to try disable/enable it w/o explicit request.

Yes, I will definitely keep that in mind and implement it like that in
the next version.

> Again, thank you for working on this and including me in the
> discussion :)

Thanks for your valuable feedback!

Best regards,
Benjamin