[PATCH RFC v4 00/13] regulator: dynamic voltage monitoring support

[PATCH RFC v4 00/13] 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.

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/20230616143736.2946173-1-martin.fuzzey@flowbird.group/

---
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/20230419-dynamic-vmon-v3-0-4179b586d8a1@skidata.com

---
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 <benjamin.bara@skidata.com>

[PATCH RFC v4 01/13] regulator: da9063: fix null pointer deref with partial DT config

[Benjamin Bara]

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

Skip checking the XVP constraints if the regulator has no DT node.
This fixes a null pointer dereference.

Fixes: b8717a80e6ee ("regulator: da9063: implement setter for voltage monitoring")
Reported-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
Closes: https://lore.kernel.org/all/20230616143736.2946173-1-martin.fuzzey@flowbird.group/
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/regulator/da9063-regulator.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index c5dd77be558b..308ad63fcf78 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -1028,9 +1028,12 @@ static int da9063_regulator_probe(struct platform_device *pdev)
 			config.of_node = da9063_reg_matches[id].of_node;
 		config.regmap = da9063->regmap;
 
-		ret = da9063_check_xvp_constraints(&config);
-		if (ret)
-			return ret;
+		/* Only check constraints if DT node is available. */
+		if (config.init_data) {
+			ret = da9063_check_xvp_constraints(&config);
+			if (ret)
+				return ret;
+		}
 
 		regl->rdev = devm_regulator_register(&pdev->dev, &regl->desc,
 						     &config);

-- 
2.34.1

[PATCH RFC v4 02/13] 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. 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 <benjamin.bara@skidata.com>
---
 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

[PATCH RFC v4 03/13] regulator: da9063: implement get_active_protections()

[Benjamin Bara]

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

Implement the new get_active_protections() op. This helps to find out if
a monitor is enabled and the DT is unaware of it. This could result to
an invalid state, as the !PWR_OK state might be triggered if the
regulator is turned off while the monitor stays active.

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

diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 308ad63fcf78..071b368f154f 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -207,6 +207,24 @@ static const unsigned int da9063_bmem_bio_merged_limits[] = {
 	4600000, 4800000, 5000000, 5200000, 5400000, 5600000, 5800000, 6000000
 };
 
+static int da9063_get_xvp(struct regulator_dev *rdev, unsigned int *state)
+{
+	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_field_read(regl->vmon, &val);
+	if (ret)
+		return ret;
+
+	*state = REGULATOR_MONITOR_NONE;
+	if (val)
+		*state = REGULATOR_MONITOR_OVER_VOLTAGE |
+			 REGULATOR_MONITOR_UNDER_VOLTAGE;
+
+	return 0;
+}
+
 static int da9063_set_xvp(struct regulator_dev *rdev, int lim_uV, int severity, bool enable)
 {
 	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
@@ -580,6 +598,7 @@ static const struct regulator_ops da9063_buck_ops = {
 	.set_suspend_mode		= da9063_buck_set_suspend_mode,
 	.set_over_voltage_protection	= da9063_set_xvp,
 	.set_under_voltage_protection	= da9063_set_xvp,
+	.get_active_protections		= da9063_get_xvp,
 };
 
 static const struct regulator_ops da9063_ldo_ops = {
@@ -598,6 +617,7 @@ static const struct regulator_ops da9063_ldo_ops = {
 	.set_suspend_mode		= da9063_ldo_set_suspend_mode,
 	.set_over_voltage_protection	= da9063_set_xvp,
 	.set_under_voltage_protection	= da9063_set_xvp,
+	.get_active_protections		= da9063_get_xvp,
 };
 
 /* Info of regulators for DA9063 */

-- 
2.34.1

[PATCH RFC v4 04/13] regulator: bd718x7: implement get_active_protections()

[Benjamin Bara]

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

This is required as the mon_disable_reg_set_higher workaround property
requires the ops to find out if it the monitor is set without the
device-tree being aware of it.

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

diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index b0b9938c20a1..fbf609d219fc 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),			\
 }								\
 
 /*
@@ -457,6 +459,23 @@ 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 *state)
+{
+	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;
+
+	*state = REGULATOR_MONITOR_NONE;
+	if (ret)
+		*state = 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 +538,33 @@ 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 *state)
+{
+	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;
+
+	*state = REGULATOR_MONITOR_NONE;
+	if (ret)
+		*state = 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)
+		*state |= REGULATOR_MONITOR_OVER_VOLTAGE;
+
+	return 0;
+}
+
 static int bd718x7_set_buck_uvp(struct regulator_dev *rdev, int lim_uV,
 				int severity, bool enable)
 {
@@ -566,7 +612,7 @@ BD718XX_OPS(bd718xx_pickable_range_ldo_ops,
 	    regulator_list_voltage_pickable_linear_range, NULL,
 	    bd718xx_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);
 
 /* BD71847 and BD71850 LDO 5 is by default OFF at RUN state */
 static const struct regulator_ops bd718xx_ldo5_ops_hwstate = {
@@ -582,27 +628,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,
 	    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,
 	    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 +657,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 +708,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);
 
 
 

-- 
2.34.1

[PATCH RFC v4 05/13] regulator: introduce properties for monitoring workarounds

[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() 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 <benjamin.bara@skidata.com>
---
 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

[PATCH RFC v4 06/13] regulator: set required ops for monitoring workarounds

[Benjamin Bara]

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

If the core should be able to handle the monitoring workarounds, certain
regulator ops are required:
- is_enabled() to decide whether a monitor should be turned off or not.
- get_active_protections() to find out if the device-tree is missing
  active protections.
- get_mode() if the regulator is in a mode where monitoring is not
  supported.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index dc741ac156c3..ca5d6ba889dc 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5540,6 +5540,40 @@ regulator_register(struct device *dev,
 		goto rinse;
 	}
 
+	/* monitor workaround properties require ops to ensure functionality. */
+	if (regulator_desc->mon_disable_reg_disabled ||
+	    regulator_desc->mon_disable_reg_set_higher ||
+	    regulator_desc->mon_disable_reg_set_lower ||
+	    regulator_desc->mon_unsupported_reg_modes) {
+		/*
+		 * is_enabled() to make sure the monitors aren't disabled on
+		 * disabled regulators.
+		 */
+		if (!regulator_desc->ops->is_enabled) {
+			ret = -EINVAL;
+			goto rinse;
+		}
+
+		/*
+		 * get_active_protections() to know if a regulator is monitored
+		 * without the device-tree being aware of it.
+		 */
+		if (!regulator_desc->ops->get_active_protections) {
+			ret = -EINVAL;
+			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;
+		}
+	}
+
 	rdev = kzalloc(sizeof(struct regulator_dev), GFP_KERNEL);
 	if (rdev == NULL) {
 		ret = -ENOMEM;

-- 
2.34.1

[PATCH RFC v4 07/13] regulator: find active protections during initialization

[Benjamin Bara]

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

It can happen that monitors are activated before the kernel is started,
e.g. by bootloader or by OTP. If monitoring workarounds are active on a
regulator, the core shouldn't perform the state changes without applying
the workaround to the regulator. Therefore, warn the user already during
initialization that the device-tree should be adapted.

Warning can be fixed by enabling (or disabling) monitoring in the DT,
e.g.:
regulator-uv-protection-microvolt = <1>;
or
regulator-ov-error-microvolt = <0>;

Constraints regarding the monitoring of a regulator can usually be found
in the docu.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ca5d6ba889dc..9bddab17450e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1564,6 +1564,47 @@ static int set_machine_constraints(struct regulator_dev *rdev)
 		}
 	}
 
+	/*
+	 * When the core is in charge of handling monitoring workarounds,
+	 * it is essential to know if a monitor is already active during
+	 * initialization.
+	 */
+	if (rdev->desc->mon_disable_reg_disabled ||
+	    rdev->desc->mon_disable_reg_set_higher ||
+	    rdev->desc->mon_disable_reg_set_lower ||
+	    rdev->desc->mon_unsupported_reg_modes) {
+		unsigned int monitor_state = REGULATOR_MONITOR_NONE;
+
+		ret = ops->get_active_protections(rdev, &monitor_state);
+		if (ret)
+			return ret;
+
+		if (!rdev->constraints->over_voltage_detection &&
+		    (monitor_state & REGULATOR_MONITOR_OVER_VOLTAGE)) {
+			rdev_err(rdev, "dt unaware of active %s monitor!\n",
+				       "over-voltage");
+			return -EINVAL;
+		}
+		if (!rdev->constraints->under_voltage_detection &&
+		    (monitor_state & REGULATOR_MONITOR_UNDER_VOLTAGE)) {
+			rdev_err(rdev, "dt unaware of active %s monitor!\n",
+				       "under-voltage");
+			return -EINVAL;
+		}
+		if (!rdev->constraints->over_current_detection &&
+		    (monitor_state & REGULATOR_MONITOR_OVER_CURRENT)) {
+			rdev_err(rdev, "dt unaware of active %s monitor!\n",
+				       "over-current");
+			return -EINVAL;
+		}
+		if (!rdev->constraints->over_temp_detection &&
+		    (monitor_state & REGULATOR_MONITOR_OVER_TEMPERATURE)) {
+			rdev_err(rdev, "dt unaware of active %s monitor!\n",
+				       "over-temperature");
+			return -EINVAL;
+		}
+	}
+
 	if (rdev->constraints->over_current_detection)
 		ret = handle_notify_limits(rdev,
 					   ops->set_over_current_protection,

-- 
2.34.1

[PATCH RFC v4 08/13] regulator: move monitor handling into own function

[Benjamin Bara]

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

Extract the current monitor handling into an own function and create
helper for initialization, disabling and re-enabling of monitors.
For reenabling the monitors, the current state and mode is considered to
avoid entering an invalid state on regulators with enabled workarounds.

Additionally, monitors of disabled regulators are not disabled before
changing state. The mon_disable_reg_disabled property is still respected
in this case, because turning off the monitor happens when the regulator
is still enabled.

Differ between initialization and normal "workaround handling" when an
EOPNOTSUPP is returned.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9bddab17450e..873e53633698 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,180 @@ 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,
+			      unsigned int mons)
+{
+	const struct regulation_constraints *reg_c = rdev->constraints;
+	const struct regulator_ops *ops = rdev->desc->ops;
+	int tmp, ret = 0;
+
+	rdev_dbg(rdev, "%s: en: %d, mons: %x\n", __func__, enable, mons);
+
+	/* only set the state if monitoring is activated in the device-tree. */
+	if ((mons & REGULATOR_MONITOR_OVER_VOLTAGE) && reg_c->over_voltage_detection) {
+		tmp = handle_notify_limits(rdev, ops->set_over_voltage_protection,
+					   enable ? &reg_c->over_voltage_limits
+					   : &disable_limits);
+		if (tmp) {
+			if (tmp != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set over voltage limits %pe\n",
+					 ERR_PTR(tmp));
+				return tmp;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested over voltage limits\n");
+			ret = tmp;
+		}
+	}
+	if ((mons & REGULATOR_MONITOR_UNDER_VOLTAGE) && reg_c->under_voltage_detection) {
+		tmp = handle_notify_limits(rdev, ops->set_under_voltage_protection,
+					   enable ? &reg_c->under_voltage_limits
+					   : &disable_limits);
+		if (tmp) {
+			if (tmp != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set under voltage limits %pe\n",
+					 ERR_PTR(tmp));
+				return ret;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested under voltage limits\n");
+			ret = tmp;
+		}
+	}
+	if ((mons & REGULATOR_MONITOR_OVER_CURRENT) && reg_c->over_current_detection) {
+		tmp = handle_notify_limits(rdev, ops->set_over_current_protection,
+					   enable ? &reg_c->over_curr_limits
+					   : &disable_limits);
+		if (ret) {
+			if (tmp != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set over current limits: %pe\n",
+					 ERR_PTR(tmp));
+				return tmp;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested over-current limits\n");
+			ret = tmp;
+		}
+	}
+	if ((mons & REGULATOR_MONITOR_OVER_TEMPERATURE) && reg_c->over_temp_detection) {
+		tmp = handle_notify_limits(rdev, ops->set_thermal_protection,
+					   enable ? &reg_c->temp_limits
+					   : &disable_limits);
+		if (tmp) {
+			if (tmp != -EOPNOTSUPP) {
+				rdev_err(rdev, "failed to set temperature limits %pe\n",
+					 ERR_PTR(tmp));
+				return tmp;
+			}
+			rdev_warn(rdev,
+				  "IC does not support requested temperature limits\n");
+			ret = tmp;
+		}
+	}
+
+	return ret;
+}
+
+/**
+ * monitors_disable - disables given monitors if the regulator is enabled
+ * @rdev: regulator source
+ * @mons: monitors to enable
+ */
+static int monitors_disable(struct regulator_dev *rdev, unsigned int mons)
+{
+	int reg_enabled;
+
+	if (!mons)
+		return 0;
+
+	reg_enabled = _regulator_is_enabled(rdev);
+	if (reg_enabled <= 0)
+		return reg_enabled;
+
+	return monitors_set_state(rdev, false, mons);
+}
+
+/**
+ * monitors_enable - enables given monitors
+ * @rdev: regulator source
+ * @mons: monitors to enable
+ *
+ * Enables monitors based on their workaround properties and the current state
+ * or mode.
+ */
+static int monitors_enable(struct regulator_dev *rdev, unsigned int mons)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	const struct regulator_ops *ops = desc->ops;
+
+	/* don't enable monitors if regulator is in unsupported mode. */
+	if (desc->mon_unsupported_reg_modes &&
+	    (desc->mon_unsupported_reg_modes & ops->get_mode(rdev)))
+		return 0;
+
+	/* don't enable monitor on disabled regulator with workaround active. */
+	if (mons & desc->mon_disable_reg_disabled) {
+		int reg_enabled = _regulator_is_enabled(rdev);
+
+		if (reg_enabled < 0)
+			return reg_enabled;
+		if (!reg_enabled)
+			mons &= ~desc->mon_disable_reg_disabled;
+	}
+
+	return monitors_set_state(rdev, true, mons);
+}
+
+static int monitors_init(struct regulator_dev *rdev)
+{
+	unsigned int mons = REGULATOR_MONITOR_NONE;
+	int reg_enabled = _regulator_is_enabled(rdev);
+	int ret;
+
+	/*
+	 * Ensure that monitors of disabled regulators with respective
+	 * workaround active are disabled during initialization.
+	 */
+	if (reg_enabled < 0)
+		return reg_enabled;
+	if (!reg_enabled && rdev->desc->mon_disable_reg_disabled) {
+		mons = rdev->desc->mon_disable_reg_disabled;
+		ret = monitors_set_state(rdev, false, mons);
+	}
+
+	/* Ignore EOPNOTSUPP at initialization, but not during workarounds. */
+	ret = monitors_enable(rdev, ~mons);
+	if (ret && ret != -EOPNOTSUPP)
+		return ret;
+
+	return 0;
+}
+
+static int monitors_reenable(struct regulator_dev *rdev, unsigned int mons)
+{
+	int reg_enabled;
+
+	if (!mons)
+		return 0;
+
+	/*
+	 * Monitors of disabled regulators are not turned off, therefore skip
+	 * turning on.
+	 */
+	reg_enabled = _regulator_is_enabled(rdev);
+	if (reg_enabled <= 0)
+		return reg_enabled;
+
+	return monitors_enable(rdev, mons);
+}
+
 /**
  * set_machine_constraints - sets regulator constraints
  * @rdev: regulator source
@@ -1605,61 +1779,9 @@ 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));
-			return ret;
-		}
-		rdev_warn(rdev,
-			  "IC does not support requested temperature limits\n");
-	}
+	ret = monitors_init(rdev);
+	if (ret)
+		return ret;
 
 	if (rdev->constraints->active_discharge && ops->set_active_discharge) {
 		bool ad_state = (rdev->constraints->active_discharge ==

-- 
2.34.1

[PATCH RFC v4 09/13] regulator: implement mon_disable_reg_disabled

[Benjamin Bara]

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

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 <benjamin.bara@skidata.com>
---
 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

[PATCH RFC v4 10/13] regulator: implement mon_disable_reg_set_{higher,lower}

[Benjamin Bara]

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

The mon_disable_reg_set_{higher,lower} properties disable all dt-enabled
monitors when the value of the regulator is changed to a higher or lower
one.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b37dcafff407..74b9c12d38e9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3643,6 +3643,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
 				       int min_uV, int max_uV,
 				       unsigned *selector)
 {
+	const struct regulator_desc *desc = rdev->desc;
 	struct pre_voltage_change_data data;
 	int ret;
 
@@ -3654,7 +3655,18 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
 	if (ret & NOTIFY_STOP_MASK)
 		return -EINVAL;
 
-	ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
+	if (min_uV > data.old_uV || max_uV > data.old_uV) {
+		ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
+		if (ret)
+			return ret;
+	}
+	if (min_uV < data.old_uV || max_uV < data.old_uV) {
+		ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
+		if (ret)
+			return ret;
+	}
+
+	ret = desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
 	if (ret >= 0)
 		return ret;
 
@@ -3667,6 +3679,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
 static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
 					   int uV, unsigned selector)
 {
+	const struct regulator_desc *desc = rdev->desc;
 	struct pre_voltage_change_data data;
 	int ret;
 
@@ -3678,7 +3691,18 @@ static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
 	if (ret & NOTIFY_STOP_MASK)
 		return -EINVAL;
 
-	ret = rdev->desc->ops->set_voltage_sel(rdev, selector);
+	if (uV > data.old_uV) {
+		ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
+		if (ret)
+			return ret;
+	}
+	if (uV < data.old_uV) {
+		ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
+		if (ret)
+			return ret;
+	}
+
+	ret = desc->ops->set_voltage_sel(rdev, selector);
 	if (ret >= 0)
 		return ret;
 
@@ -3780,7 +3804,8 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	int best_val = 0;
 	unsigned int selector;
 	int old_selector = -1;
-	const struct regulator_ops *ops = rdev->desc->ops;
+	const struct regulator_desc *desc = rdev->desc;
+	const struct regulator_ops *ops = desc->ops;
 	int old_uV = regulator_get_voltage_rdev(rdev);
 
 	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
@@ -3819,7 +3844,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 				selector = ret;
 				if (old_selector == selector)
 					ret = 0;
-				else if (rdev->desc->vsel_step)
+				else if (desc->vsel_step)
 					ret = _regulator_set_voltage_sel_step(
 						rdev, best_val, selector);
 				else
@@ -3874,6 +3899,14 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 out:
 	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
 
+	/* if setting voltage failed, ignore monitoring error. */
+	if (ret)
+		monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
+					desc->mon_disable_reg_set_lower);
+	else
+		ret = monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
+					      desc->mon_disable_reg_set_lower);
+
 	return ret;
 }
 

-- 
2.34.1

[PATCH RFC v4 11/13] regulator: implement mon_unsupported_reg_modes

[Benjamin Bara]

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

The mon_unsupported_reg_modes property disables all dt-enabled monitors
when the mode of the regulator is changed to an "unsupported" one.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 74b9c12d38e9..ca768d0ddb1e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4816,8 +4816,21 @@ 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_disable(rdev, REGULATOR_MONITOR_ALL);
+		if (ret)
+			goto out;
+	}
+
 	ret = rdev->desc->ops->set_mode(rdev, mode);
+
 out:
+	/* if changing mode failed, ignore monitoring error. */
+	if (ret)
+		monitors_reenable(rdev, REGULATOR_MONITOR_ALL);
+	else
+		ret = monitors_reenable(rdev, REGULATOR_MONITOR_ALL);
+
 	regulator_unlock(rdev);
 	return ret;
 }

-- 
2.34.1

[PATCH RFC v4 12/13] regulator: da9063: let the core handle the monitors

[Benjamin Bara]

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

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 <benjamin.bara@skidata.com>
---
 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, &regl->desc,
 						     &config);
 		if (IS_ERR(regl->rdev)) {

-- 
2.34.1

[PATCH RFC v4 13/13] 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.

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

diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index fbf609d219fc..251d098d088c 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -128,128 +128,6 @@ static int bd71837_get_buck34_enable_hwctrl(struct regulator_dev *rdev)
 	return !!(BD718XX_BUCK_RUN_ON & val);
 }
 
-static void voltage_change_done(struct regulator_dev *rdev, unsigned int sel,
-				unsigned int *mask)
-{
-	int ret;
-
-	if (*mask) {
-		/*
-		 * Let's allow scheduling as we use I2C anyways. We just need to
-		 * guarantee minimum of 1ms sleep - it shouldn't matter if we
-		 * exceed it due to the scheduling.
-		 */
-		msleep(1);
-
-		ret = regmap_clear_bits(rdev->regmap, BD718XX_REG_MVRFLTMASK2,
-					 *mask);
-		if (ret)
-			dev_err(&rdev->dev,
-				"Failed to re-enable voltage monitoring (%d)\n",
-				ret);
-	}
-}
-
-static int voltage_change_prepare(struct regulator_dev *rdev, unsigned int sel,
-				  unsigned int *mask)
-{
-	int ret;
-
-	*mask = 0;
-	if (rdev->desc->ops->is_enabled(rdev)) {
-		int now, new;
-
-		now = rdev->desc->ops->get_voltage_sel(rdev);
-		if (now < 0)
-			return now;
-
-		now = rdev->desc->ops->list_voltage(rdev, now);
-		if (now < 0)
-			return now;
-
-		new = rdev->desc->ops->list_voltage(rdev, sel);
-		if (new < 0)
-			return new;
-
-		/*
-		 * If we increase LDO voltage when LDO is enabled we need to
-		 * disable the power-good detection until voltage has reached
-		 * the new level. According to HW colleagues the maximum time
-		 * it takes is 1000us. I assume that on systems with light load
-		 * this might be less - and we could probably use DT to give
-		 * system specific delay value if performance matters.
-		 *
-		 * Well, knowing we use I2C here and can add scheduling delays
-		 * I don't think it is worth the hassle and I just add fixed
-		 * 1ms sleep here (and allow scheduling). If this turns out to
-		 * be a problem we can change it to delay and make the delay
-		 * time configurable.
-		 */
-		if (new > now) {
-			int tmp;
-			int prot_bit;
-			int ldo_offset = rdev->desc->id - BD718XX_LDO1;
-
-			prot_bit = BD718XX_LDO1_VRMON80 << ldo_offset;
-			ret = regmap_read(rdev->regmap, BD718XX_REG_MVRFLTMASK2,
-					  &tmp);
-			if (ret) {
-				dev_err(&rdev->dev,
-					"Failed to read voltage monitoring state\n");
-				return ret;
-			}
-
-			if (!(tmp & prot_bit)) {
-				/* We disable protection if it was enabled... */
-				ret = regmap_set_bits(rdev->regmap,
-						      BD718XX_REG_MVRFLTMASK2,
-						      prot_bit);
-				/* ...and we also want to re-enable it */
-				*mask = prot_bit;
-			}
-			if (ret) {
-				dev_err(&rdev->dev,
-					"Failed to stop voltage monitoring\n");
-				return ret;
-			}
-		}
-	}
-
-	return 0;
-}
-
-static int bd718xx_set_voltage_sel_restricted(struct regulator_dev *rdev,
-						    unsigned int sel)
-{
-	int ret;
-	int mask;
-
-	ret = voltage_change_prepare(rdev, sel, &mask);
-	if (ret)
-		return ret;
-
-	ret = regulator_set_voltage_sel_regmap(rdev, sel);
-	voltage_change_done(rdev, sel, &mask);
-
-	return ret;
-}
-
-static int bd718xx_set_voltage_sel_pickable_restricted(
-		struct regulator_dev *rdev, unsigned int sel)
-{
-	int ret;
-	int mask;
-
-	ret = voltage_change_prepare(rdev, sel, &mask);
-	if (ret)
-		return ret;
-
-	ret = regulator_set_voltage_sel_pickable_regmap(rdev, sel);
-	voltage_change_done(rdev, sel, &mask);
-
-	return ret;
-}
-
 static int bd71837_set_voltage_sel_pickable_restricted(
 		struct regulator_dev *rdev, unsigned int sel)
 {
@@ -610,7 +488,7 @@ static int bd718x7_set_buck_ovp(struct regulator_dev *rdev, int lim_uV,
  */
 BD718XX_OPS(bd718xx_pickable_range_ldo_ops,
 	    regulator_list_voltage_pickable_linear_range, NULL,
-	    bd718xx_set_voltage_sel_pickable_restricted,
+	    regulator_set_voltage_sel_pickable_regmap,
 	    regulator_get_voltage_sel_pickable_regmap, NULL, NULL,
 	    bd718x7_set_ldo_uvp, NULL, bd717x7_get_ldo_prot);
 
@@ -618,7 +496,7 @@ BD718XX_OPS(bd718xx_pickable_range_ldo_ops,
 static const struct regulator_ops bd718xx_ldo5_ops_hwstate = {
 	.is_enabled = never_enabled_by_hwstate,
 	.list_voltage = regulator_list_voltage_pickable_linear_range,
-	.set_voltage_sel = bd718xx_set_voltage_sel_pickable_restricted,
+	.set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,
 	.get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,
 	.set_under_voltage_protection = bd718x7_set_ldo_uvp,
 };
@@ -631,12 +509,12 @@ BD718XX_OPS(bd718xx_pickable_range_buck_ops,
 	    bd718x7_set_buck_ovp, bd717x7_get_buck_prot);
 
 BD718XX_OPS(bd718xx_ldo_regulator_ops, regulator_list_voltage_linear_range,
-	    NULL, bd718xx_set_voltage_sel_restricted,
+	    NULL, regulator_set_voltage_sel_regmap,
 	    regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
 	    NULL, bd717x7_get_ldo_prot);
 
 BD718XX_OPS(bd718xx_ldo_regulator_nolinear_ops, regulator_list_voltage_table,
-	    NULL, bd718xx_set_voltage_sel_restricted,
+	    NULL, regulator_set_voltage_sel_regmap,
 	    regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
 	    NULL, bd717x7_get_ldo_prot);
 
@@ -1818,6 +1696,12 @@ static int bd718xx_probe(struct platform_device *pdev)
 		else
 			desc->ops = swops[i];
 
+		/*
+		 * bd718x7 requires to disable a regulator's over voltage
+		 * protection while it changes to a higher value.
+		 */
+		desc->mon_disable_reg_set_higher = REGULATOR_MONITOR_OVER_VOLTAGE;
+
 		rdev = devm_regulator_register(&pdev->dev, desc, &config);
 		if (IS_ERR(rdev))
 			return dev_err_probe(&pdev->dev, PTR_ERR(rdev),

-- 
2.34.1

Re: [PATCH RFC v4 02/13] regulator: add getter for active monitors

[Matti Vaittinen]

On 6/20/23 23:02, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> 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 <benjamin.bara@skidata.com>

Just a couple of minor remarks. Feel free to change those if you end up 
respinning.

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   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! ~~

Re: [PATCH RFC v4 04/13] regulator: bd718x7: implement get_active_protections()

[Matti Vaittinen]

On 6/20/23 23:02, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> This is required as the mon_disable_reg_set_higher workaround property
> requires the ops to find out if it the monitor is set without the
> device-tree being aware of it.
>  > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

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

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

Re: [PATCH RFC v4 05/13] regulator: introduce properties for monitoring workarounds

[Matti Vaittinen]

On 6/20/23 23:02, Benjamin Bara wrote:
> 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() 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 <benjamin.bara@skidata.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>


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

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

Re: [PATCH RFC v4 06/13] regulator: set required ops for monitoring workarounds

[Matti Vaittinen]

On 6/20/23 23:02, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> If the core should be able to handle the monitoring workarounds, certain
> regulator ops are required:
> - is_enabled() to decide whether a monitor should be turned off or not.
> - get_active_protections() to find out if the device-tree is missing
>    active protections.
> - get_mode() if the regulator is in a mode where monitoring is not
>    supported.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   drivers/regulator/core.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index dc741ac156c3..ca5d6ba889dc 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5540,6 +5540,40 @@ regulator_register(struct device *dev,
>   		goto rinse;
>   	}
>   
> +	/* monitor workaround properties require ops to ensure functionality. */
> +	if (regulator_desc->mon_disable_reg_disabled ||
> +	    regulator_desc->mon_disable_reg_set_higher ||
> +	    regulator_desc->mon_disable_reg_set_lower ||
> +	    regulator_desc->mon_unsupported_reg_modes) {
> +		/*
> +		 * is_enabled() to make sure the monitors aren't disabled on
> +		 * disabled regulators.
> +		 */
> +		if (!regulator_desc->ops->is_enabled) {
> +			ret = -EINVAL;
> +			goto rinse;
> +		}
> +
> +		/*
> +		 * get_active_protections() to know if a regulator is monitored
> +		 * without the device-tree being aware of it.
> +		 */
> +		if (!regulator_desc->ops->get_active_protections) {
> +			ret = -EINVAL;
> +			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;
> +		}
> +	}
> +
>   	rdev = kzalloc(sizeof(struct regulator_dev), GFP_KERNEL);
>   	if (rdev == NULL) {
>   		ret = -ENOMEM;
> 

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

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

Re: [PATCH RFC v4 07/13] regulator: find active protections during initialization

[Matti Vaittinen]

On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> It can happen that monitors are activated before the kernel is started,
> e.g. by bootloader or by OTP. If monitoring workarounds are active on a
> regulator, the core shouldn't perform the state changes without applying
> the workaround to the regulator. Therefore, warn the user already during
> initialization that the device-tree should be adapted.
> 
> Warning can be fixed by enabling (or disabling) monitoring in the DT,
> e.g.:
> regulator-uv-protection-microvolt = <1>;
> or
> regulator-ov-error-microvolt = <0>;
> 
> Constraints regarding the monitoring of a regulator can usually be found
> in the docu.

I am not entirely sure if this is the right thing to do. Should we 
expect the hardware state to be what is described in DT at Linux boot-up 
- or, should we silently accept the fact that for example boot can alter 
things.

 From the 'code pov' I have no complaints though. I just can't say if 
warning is the right idea. I'll leave this for bigger brains to decide :)


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

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

Re: [PATCH RFC v4 08/13] regulator: move monitor handling into own function

[Matti Vaittinen]

On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Extract the current monitor handling into an own function and create
> helper for initialization, disabling and re-enabling of monitors.
> For reenabling the monitors, the current state and mode is considered to
> avoid entering an invalid state on regulators with enabled workarounds.
> 
> Additionally, monitors of disabled regulators are not disabled before
> changing state. The mon_disable_reg_disabled property is still respected
> in this case, because turning off the monitor happens when the regulator
> is still enabled.
> 
> Differ between initialization and normal "workaround handling" when an
> EOPNOTSUPP is returned.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>   drivers/regulator/core.c | 234 +++++++++++++++++++++++++++++++++++------------
>   1 file changed, 178 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9bddab17450e..873e53633698 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,180 @@ 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,
> +			      unsigned int mons)
> +{
> +	const struct regulation_constraints *reg_c = rdev->constraints;
> +	const struct regulator_ops *ops = rdev->desc->ops;
> +	int tmp, ret = 0;
> +
> +	rdev_dbg(rdev, "%s: en: %d, mons: %x\n", __func__, enable, mons);
> +
> +	/* only set the state if monitoring is activated in the device-tree. */
> +	if ((mons & REGULATOR_MONITOR_OVER_VOLTAGE) && reg_c->over_voltage_detection) {
> +		tmp = handle_notify_limits(rdev, ops->set_over_voltage_protection,
> +					   enable ? &reg_c->over_voltage_limits
> +					   : &disable_limits);
> +		if (tmp) {
> +			if (tmp != -EOPNOTSUPP) {
> +				rdev_err(rdev, "failed to set over voltage limits %pe\n",
> +					 ERR_PTR(tmp));
> +				return tmp;
> +			}
> +			rdev_warn(rdev,
> +				  "IC does not support requested over voltage limits\n");
> +			ret = tmp;
> +		}
> +	}
> +	if ((mons & REGULATOR_MONITOR_UNDER_VOLTAGE) && reg_c->under_voltage_detection) {
> +		tmp = handle_notify_limits(rdev, ops->set_under_voltage_protection,
> +					   enable ? &reg_c->under_voltage_limits
> +					   : &disable_limits);
> +		if (tmp) {
> +			if (tmp != -EOPNOTSUPP) {
> +				rdev_err(rdev, "failed to set under voltage limits %pe\n",
> +					 ERR_PTR(tmp));
> +				return ret;
> +			}
> +			rdev_warn(rdev,
> +				  "IC does not support requested under voltage limits\n");
> +			ret = tmp;
> +		}
> +	}
> +	if ((mons & REGULATOR_MONITOR_OVER_CURRENT) && reg_c->over_current_detection) {
> +		tmp = handle_notify_limits(rdev, ops->set_over_current_protection,
> +					   enable ? &reg_c->over_curr_limits
> +					   : &disable_limits);
> +		if (ret) {
> +			if (tmp != -EOPNOTSUPP) {
> +				rdev_err(rdev, "failed to set over current limits: %pe\n",
> +					 ERR_PTR(tmp));
> +				return tmp;
> +			}
> +			rdev_warn(rdev,
> +				  "IC does not support requested over-current limits\n");
> +			ret = tmp;
> +		}
> +	}
> +	if ((mons & REGULATOR_MONITOR_OVER_TEMPERATURE) && reg_c->over_temp_detection) {
> +		tmp = handle_notify_limits(rdev, ops->set_thermal_protection,
> +					   enable ? &reg_c->temp_limits
> +					   : &disable_limits);
> +		if (tmp) {
> +			if (tmp != -EOPNOTSUPP) {
> +				rdev_err(rdev, "failed to set temperature limits %pe\n",
> +					 ERR_PTR(tmp));
> +				return tmp;
> +			}
> +			rdev_warn(rdev,
> +				  "IC does not support requested temperature limits\n");
> +			ret = tmp;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * monitors_disable - disables given monitors if the regulator is enabled
> + * @rdev: regulator source
> + * @mons: monitors to enable
> + */
> +static int monitors_disable(struct regulator_dev *rdev, unsigned int mons)
> +{
> +	int reg_enabled;
> +
> +	if (!mons)
> +		return 0;

Just a minor thing but can we do this check already at the caller side? 
I think that would show the logic more clearly already in the functions 
implementing the actual action requested by the user. 
(disable/enable/change voltage). Eg, the logic in those functions would 
be clear:

if (flag_to_do_magic_monitor_toggling)
	monitors_disable();

and similarly for the monitors_reenable()...

> +
> +	reg_enabled = _regulator_is_enabled(rdev);
> +	if (reg_enabled <= 0)
> +		return reg_enabled;
> +
> +	return monitors_set_state(rdev, false, mons);
> +}
> +
> +/**
> + * monitors_enable - enables given monitors
> + * @rdev: regulator source
> + * @mons: monitors to enable
> + *
> + * Enables monitors based on their workaround properties and the current state
> + * or mode.
> + */
> +static int monitors_enable(struct regulator_dev *rdev, unsigned int mons)
> +{
> +	const struct regulator_desc *desc = rdev->desc;
> +	const struct regulator_ops *ops = desc->ops;
> +
> +	/* don't enable monitors if regulator is in unsupported mode. */
> +	if (desc->mon_unsupported_reg_modes &&
> +	    (desc->mon_unsupported_reg_modes & ops->get_mode(rdev)))
> +		return 0;
> +
> +	/* don't enable monitor on disabled regulator with workaround active. */
> +	if (mons & desc->mon_disable_reg_disabled) {
> +		int reg_enabled = _regulator_is_enabled(rdev);
> +
> +		if (reg_enabled < 0)
> +			return reg_enabled;
> +		if (!reg_enabled)
> +			mons &= ~desc->mon_disable_reg_disabled;
> +	}
> +
> +	return monitors_set_state(rdev, true, mons);
> +}
> +
> +static int monitors_init(struct regulator_dev *rdev)
> +{
> +	unsigned int mons = REGULATOR_MONITOR_NONE;
> +	int reg_enabled = _regulator_is_enabled(rdev);
> +	int ret;
> +
> +	/*
> +	 * Ensure that monitors of disabled regulators with respective
> +	 * workaround active are disabled during initialization.
> +	 */
> +	if (reg_enabled < 0)
> +		return reg_enabled;
> +	if (!reg_enabled && rdev->desc->mon_disable_reg_disabled) {
> +		mons = rdev->desc->mon_disable_reg_disabled;
> +		ret = monitors_set_state(rdev, false, mons);
> +	}
> +
> +	/* Ignore EOPNOTSUPP at initialization, but not during workarounds. */
> +	ret = monitors_enable(rdev, ~mons);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int monitors_reenable(struct regulator_dev *rdev, unsigned int mons)
> +{
> +	int reg_enabled;
> +
> +	if (!mons)
> +		return 0;
> +

...here.

> +	/*
> +	 * Monitors of disabled regulators are not turned off, therefore skip
> +	 * turning on.
> +	 */
> +	reg_enabled = _regulator_is_enabled(rdev);
> +	if (reg_enabled <= 0)
> +		return reg_enabled;
> +
> +	return monitors_enable(rdev, mons);
> +}
> +

Sorry but my flight landed so I need to stop reviewing for now... This 
will be a busy week for me. It may be I can't go through rest of the 
patches until later :/

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 v4 02/13] regulator: add getter for active monitors

[Mark Brown]

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

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.

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

Re: [PATCH RFC v4 07/13] regulator: find active protections during initialization

[Mark Brown]

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

On Mon, Jun 26, 2023 at 04:56:21PM +0300, Matti Vaittinen wrote:
> On 6/20/23 23:03, Benjamin Bara wrote:

> > Warning can be fixed by enabling (or disabling) monitoring in the DT,
> > e.g.:
> > regulator-uv-protection-microvolt = <1>;
> > or
> > regulator-ov-error-microvolt = <0>;
> > 
> > Constraints regarding the monitoring of a regulator can usually be found
> > in the docu.

> I am not entirely sure if this is the right thing to do. Should we expect
> the hardware state to be what is described in DT at Linux boot-up - or,
> should we silently accept the fact that for example boot can alter things.

> From the 'code pov' I have no complaints though. I just can't say if warning
> is the right idea. I'll leave this for bigger brains to decide :)

Yes, this isn't really the idiom we normally adopt - the default thing
is to just leave the hardware untouched, that should not usually be
regarded as a problem.

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

Re: [PATCH RFC v4 09/13] regulator: implement mon_disable_reg_disabled

[Matti Vaittinen]

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 <benjamin.bara@skidata.com>
> 
> 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 <benjamin.bara@skidata.com>
> ---
>   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! ~~

Re: [PATCH RFC v4 10/13] regulator: implement mon_disable_reg_set_{higher,lower}

[Matti Vaittinen]

On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> The mon_disable_reg_set_{higher,lower} properties disable all dt-enabled
> monitors when the value of the regulator is changed to a higher or lower
> one.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>   drivers/regulator/core.c | 41 +++++++++++++++++++++++++++++++++++++----
>   1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b37dcafff407..74b9c12d38e9 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3643,6 +3643,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
>   				       int min_uV, int max_uV,
>   				       unsigned *selector)
>   {
> +	const struct regulator_desc *desc = rdev->desc;
>   	struct pre_voltage_change_data data;
>   	int ret;
>   
> @@ -3654,7 +3655,18 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
>   	if (ret & NOTIFY_STOP_MASK)
>   		return -EINVAL;
>   
> -	ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
> +	if (min_uV > data.old_uV || max_uV > data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
> +		if (ret)
> +			return ret;

Here, as per comments to previous patches, the logic would be more 
obvious for me if this was:
	if (desc->mon_disable_reg_set_higher &&
	   (min_uV > data.old_uV || max_uV > data.old_uV)) {
		ret = monitors_disable(...)

> +	}
> +	if (min_uV < data.old_uV || max_uV < data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
> +		if (ret)
> +			return ret;
> +	}

I guess you guess what I think of the above by now :)

> +
> +	ret = desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
>   	if (ret >= 0)
>   		return ret;
>   
> @@ -3667,6 +3679,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
>   static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
>   					   int uV, unsigned selector)
>   {
> +	const struct regulator_desc *desc = rdev->desc;
>   	struct pre_voltage_change_data data;
>   	int ret;
>   
> @@ -3678,7 +3691,18 @@ static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
>   	if (ret & NOTIFY_STOP_MASK)
>   		return -EINVAL;
>   
> -	ret = rdev->desc->ops->set_voltage_sel(rdev, selector);
> +	if (uV > data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
> +		if (ret)
> +			return ret;
> +	}
> +	if (uV < data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
> +		if (ret)
> +			return ret;
> +	}

Here I would also pull the check from monitors_disable() to these 
callers just to explicitly show the logic.

> +
> +	ret = desc->ops->set_voltage_sel(rdev, selector);
>   	if (ret >= 0)
>   		return ret;
>   
> @@ -3780,7 +3804,8 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>   	int best_val = 0;
>   	unsigned int selector;
>   	int old_selector = -1;
> -	const struct regulator_ops *ops = rdev->desc->ops;
> +	const struct regulator_desc *desc = rdev->desc;
> +	const struct regulator_ops *ops = desc->ops;
>   	int old_uV = regulator_get_voltage_rdev(rdev);
>   
>   	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> @@ -3819,7 +3844,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>   				selector = ret;
>   				if (old_selector == selector)
>   					ret = 0;
> -				else if (rdev->desc->vsel_step)
> +				else if (desc->vsel_step)
>   					ret = _regulator_set_voltage_sel_step(
>   						rdev, best_val, selector);
>   				else
> @@ -3874,6 +3899,14 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>   out:
>   	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
>   
> +	/* if setting voltage failed, ignore monitoring error. */
> +	if (ret)
> +		monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
> +					desc->mon_disable_reg_set_lower);
> +	else
> +		ret = monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
> +					      desc->mon_disable_reg_set_lower);

Here as well.

> +
>   	return ret;
>   }

Well, pulling the check from monitors_*() to callers will increase line 
count quite a bit. Still, my personal take on this is that the logic is 
easier to follow that way. I, however, am fine also with the way it is 
done in these patches if you think the line count matters more.

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 v4 11/13] regulator: implement mon_unsupported_reg_modes

[Matti Vaittinen]

On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> The mon_unsupported_reg_modes property disables all dt-enabled monitors

How do you feel about
mon_disable_when/if/at_unsupported_mode (or unsupported_*_reg_mode)? Or 
is it again getting too long?

This is not a point I would like to stress though :) So,

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> when the mode of the regulator is changed to an "unsupported" one.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>   drivers/regulator/core.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 74b9c12d38e9..ca768d0ddb1e 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -4816,8 +4816,21 @@ 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_disable(rdev, REGULATOR_MONITOR_ALL);
> +		if (ret)
> +			goto out;
> +	}
> +
>   	ret = rdev->desc->ops->set_mode(rdev, mode);
> +
>   out:
> +	/* if changing mode failed, ignore monitoring error. */
> +	if (ret)
> +		monitors_reenable(rdev, REGULATOR_MONITOR_ALL);
> +	else
> +		ret = monitors_reenable(rdev, REGULATOR_MONITOR_ALL);
> +
>   	regulator_unlock(rdev);
>   	return ret;
>   }
> 

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

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

Re: [PATCH RFC v4 13/13] regulator: bd718x7: let the core handle the monitors

[Matti Vaittinen]

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! ~~

Re: [PATCH RFC v4 07/13] regulator: find active protections during initialization

[Benjamin Bara]

Hi Matti & Mark,

thank you for the feedback!

On Mon, 26 Jun 2023 at 18:49, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jun 26, 2023 at 04:56:21PM +0300, Matti Vaittinen wrote:
> > On 6/20/23 23:03, Benjamin Bara wrote:
> > > Warning can be fixed by enabling (or disabling) monitoring in the DT,
> > > e.g.:
> > > regulator-uv-protection-microvolt = <1>;
> > > or
> > > regulator-ov-error-microvolt = <0>;
> > >
> > > Constraints regarding the monitoring of a regulator can usually be found
> > > in the docu.
>
> > I am not entirely sure if this is the right thing to do. Should we expect
> > the hardware state to be what is described in DT at Linux boot-up - or,
> > should we silently accept the fact that for example boot can alter things.
>
> > From the 'code pov' I have no complaints though. I just can't say if warning
> > is the right idea. I'll leave this for bigger brains to decide :)
>
> Yes, this isn't really the idiom we normally adopt - the default thing
> is to just leave the hardware untouched, that should not usually be
> regarded as a problem.

Thanks for clarifying. I will now activate the constraint instead of erroring
out. This guarantees that the workaround will still be applied, so basically
similar to the current bd718x7 implementation. I would still keep the message as
a warn, or should I drop it too? My idea is to let the user know that there is
some kind of monitoring going on but the device-tree is not aware of it.

Re: [PATCH RFC v4 09/13] regulator: implement mon_disable_reg_disabled

[Benjamin Bara]

Hi Matti!

On Mon, 3 Jul 2023 at 12:31, Matti Vaittinen <mazziesaccount@gmail.com> 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 <benjamin.bara@skidata.com>
> >
> > 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 <benjamin.bara@skidata.com>
> > ---
> >   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 <mazziesaccount@gmail.com> 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 <benjamin.bara@skidata.com>
> >
> > 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 <benjamin.bara@skidata.com>
> > ---
> >   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! ~~
>

Re: [PATCH RFC v4 07/13] regulator: find active protections during initialization

[Mark Brown]

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

On Mon, Jul 03, 2023 at 08:43:47PM +0200, Benjamin Bara wrote:
> On Mon, 26 Jun 2023 at 18:49, Mark Brown <broonie@kernel.org> wrote:

> > Yes, this isn't really the idiom we normally adopt - the default thing
> > is to just leave the hardware untouched, that should not usually be
> > regarded as a problem.

> Thanks for clarifying. I will now activate the constraint instead of erroring
> out. This guarantees that the workaround will still be applied, so basically
> similar to the current bd718x7 implementation. I would still keep the message as
> a warn, or should I drop it too? My idea is to let the user know that there is
> some kind of monitoring going on but the device-tree is not aware of it.

I would leave the warning off, I'd say it's more unusual that it might
be possible to disable the montioring than that it's being enabled - a
lot of devices either have fixed limits or only allow the limit to be
configured without allowing it to be completely disabled.

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