Hi!
This series targets the "automatic" state handling of voltage 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.
Not sure if a good idea, therefore RFC.
For now, 1/2 implements so-called "monitoring constraints", but for now
only for under- and overvoltage monitoring.
2/2 depends on [1], which implements (static) voltage monitoring for the
da9063. It shows a basic example how to use these constraints.
What's not targeted (for now) are possibly required delay times between
monitor state change and regulator state change. For the da9063, these
are not required but I can imagine there are other regulators where they
might be needed?
Possible next step:
"regulators-{uv,ov}-{warn,error,protection}-enable" dt property on chip
level, with either 1 or 0, to en-/disable the dynamic voltage monitoring
for every regulator of the chip. This would require the regulator's
set_{over,under}_voltage_protection() to work with limit = 1.
Thanks & best regards,
Benjamin
[1] https://lore.kernel.org/all/[email protected]/
---
Benjamin Bara (2):
regulator: introduce regulator monitoring constraints
regulator: da9063: disable monitoring while regulator is off
drivers/regulator/core.c | 155 +++++++++++++++++++++++++++++++----
drivers/regulator/da9063-regulator.c | 17 +++-
include/linux/regulator/machine.h | 34 ++++++++
3 files changed, 190 insertions(+), 16 deletions(-)
---
base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
change-id: 20230419-dynamic-vmon-e08daa0ac7ad
Best regards,
--
Benjamin Bara <[email protected]>
From: Benjamin Bara <[email protected]>
Add constraints for regulator monitoring. These are useful when the
state of the regulator might change during runtime, but the monitor
state (in hardware) is not implicitly changed with the change of the
regulator state or mode (in hardware).
When used, the core takes care of handling the monitor state. This could
ensure that a monitor does not stay active when its regulator is
disabled.
TODO: depending on the initial state of the regulator, the initial state
of the monitor might not be guaranteed to be correct for now.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/core.c | 155 ++++++++++++++++++++++++++++++++++----
include/linux/regulator/machine.h | 34 +++++++++
2 files changed, 175 insertions(+), 14 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4fcd36055b02..3ec34c05bda2 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1360,7 +1360,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;
@@ -1385,6 +1385,106 @@ 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 inline bool should_disable_monitor(const struct monitoring_constraints *mon_c, bool pre,
+ bool enable, bool change, unsigned int mode)
+{
+ bool disable = !enable && !change && mode == REGULATOR_MODE_INVALID;
+
+ if (!pre)
+ return false;
+ return (mon_c->mon_disable_during_reg_set && change) ||
+ (mon_c->mon_disable_during_reg_off && disable) ||
+ (mon_c->mon_disable_pre_reg_idle && mode == REGULATOR_MODE_IDLE) ||
+ (mon_c->mon_disable_pre_reg_standby && mode == REGULATOR_MODE_STANDBY);
+}
+
+static inline bool should_enable_monitor(const struct monitoring_constraints *mon_c, bool pre,
+ bool enable, bool change, unsigned int mode)
+{
+ if (pre)
+ return false;
+ return (mon_c->mon_disable_during_reg_set && change) ||
+ (mon_c->mon_disable_during_reg_off && enable) ||
+ (mon_c->mon_enable_post_reg_normal && mode == REGULATOR_MODE_NORMAL) ||
+ (mon_c->mon_enable_post_reg_fast && mode == REGULATOR_MODE_FAST);
+}
+
+static int handle_monitors(struct regulator_dev *rdev, bool pre, bool enable, bool change,
+ unsigned int mode)
+{
+ const struct regulator_ops *ops = rdev->desc->ops;
+ const struct regulation_constraints *reg_c = rdev->constraints;
+
+ /*
+ * ensure that voltage monitoring is explicitly enabled in the device tree and that the
+ * driver has monitoring constraints and protection ops.
+ */
+ bool handle_ov = reg_c->over_voltage_detection && reg_c->ov_constraints &&
+ ops->set_over_voltage_protection;
+ bool handle_uv = reg_c->under_voltage_detection && reg_c->uv_constraints &&
+ ops->set_under_voltage_protection;
+ int ret = 0;
+
+ if (!handle_ov && !handle_uv)
+ return 0;
+
+ dev_dbg(&rdev->dev, "%s: pre: %d, en: %d, ch: %d, mode: %u\n", __func__, pre, enable,
+ change, mode);
+ if ((enable + change + !!mode) > 1) {
+ dev_err(&rdev->dev, "%s: invalid combination!\n", __func__);
+ return -EINVAL;
+ }
+
+ if (handle_ov) {
+ if (should_disable_monitor(reg_c->ov_constraints, pre, enable, change, mode))
+ ret = handle_notify_limits(rdev, ops->set_over_voltage_protection,
+ &disable_limits);
+ else if (should_enable_monitor(reg_c->ov_constraints, pre, enable, change, mode))
+ ret = handle_notify_limits(rdev, ops->set_over_voltage_protection,
+ ®_c->over_voltage_limits);
+ }
+ if (ret)
+ return ret;
+
+ if (handle_uv) {
+ if (should_disable_monitor(reg_c->uv_constraints, pre, enable, change, mode))
+ ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
+ &disable_limits);
+ else if (should_enable_monitor(reg_c->uv_constraints, pre, enable, change, mode))
+ ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
+ ®_c->under_voltage_limits);
+ }
+
+ return ret;
+}
+
+static inline int handle_monitors_disable(struct regulator_dev *rdev)
+{
+ return handle_monitors(rdev, true, false, false, REGULATOR_MODE_INVALID);
+}
+
+static inline int handle_monitors_enable(struct regulator_dev *rdev)
+{
+ return handle_monitors(rdev, false, true, false, REGULATOR_MODE_INVALID);
+}
+
+static inline int handle_monitors_set(struct regulator_dev *rdev, bool pre)
+{
+ return handle_monitors(rdev, pre, false, true, REGULATOR_MODE_INVALID);
+}
+
+static inline int handle_monitors_mode(struct regulator_dev *rdev, bool pre, unsigned int mode)
+{
+ return handle_monitors(rdev, pre, false, false, mode);
+}
+
/**
* set_machine_constraints - sets regulator constraints
* @rdev: regulator source
@@ -1512,7 +1612,8 @@ static int set_machine_constraints(struct regulator_dev *rdev)
"IC does not support requested over-current limits\n");
}
- if (rdev->constraints->over_voltage_detection)
+ /* only if we have static monitoring. with dynamic, it will be set according to state. */
+ if (rdev->constraints->over_voltage_detection && !rdev->constraints->ov_constraints)
ret = handle_notify_limits(rdev,
ops->set_over_voltage_protection,
&rdev->constraints->over_voltage_limits);
@@ -1526,7 +1627,8 @@ static int set_machine_constraints(struct regulator_dev *rdev)
"IC does not support requested over voltage limits\n");
}
- if (rdev->constraints->under_voltage_detection)
+ /* only if we have static monitoring. with dynamic, it will be set according to state. */
+ if (rdev->constraints->under_voltage_detection && !rdev->constraints->uv_constraints)
ret = handle_notify_limits(rdev,
ops->set_under_voltage_protection,
&rdev->constraints->under_voltage_limits);
@@ -2734,7 +2836,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
trace_regulator_enable_complete(rdev_get_name(rdev));
- return 0;
+ return handle_monitors_enable(rdev);
}
/**
@@ -2895,6 +2997,10 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
{
int ret;
+ ret = handle_monitors_disable(rdev);
+ if (ret)
+ return ret;
+
trace_regulator_disable(rdev_get_name(rdev));
if (rdev->ena_pin) {
@@ -3406,7 +3512,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
unsigned *selector)
{
struct pre_voltage_change_data data;
- int ret;
+ int ret, err = 0;
data.old_uV = regulator_get_voltage_rdev(rdev);
data.min_uV = min_uV;
@@ -3416,12 +3522,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 (ret >= 0)
+ ret = handle_monitors_set(rdev, true);
+ if (ret)
return ret;
- _notifier_call_chain(rdev, REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE,
- (void *)data.old_uV);
+ ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
+ if (ret >= 0)
+ err = handle_monitors_set(rdev, false);
+ else
+ _notifier_call_chain(rdev, REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE,
+ (void *)data.old_uV);
+ if (err)
+ return err;
return ret;
}
@@ -3430,7 +3542,7 @@ static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
int uV, unsigned selector)
{
struct pre_voltage_change_data data;
- int ret;
+ int ret, err = 0;
data.old_uV = regulator_get_voltage_rdev(rdev);
data.min_uV = uV;
@@ -3440,12 +3552,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 (ret >= 0)
+ ret = handle_monitors_set(rdev, true);
+ if (ret)
return ret;
- _notifier_call_chain(rdev, REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE,
- (void *)data.old_uV);
+ ret = rdev->desc->ops->set_voltage_sel(rdev, selector);
+ if (ret >= 0)
+ err = handle_monitors_set(rdev, false);
+ else
+ _notifier_call_chain(rdev, REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE,
+ (void *)data.old_uV);
+ if (err)
+ return err;
return ret;
}
@@ -4545,7 +4663,16 @@ int regulator_set_mode(struct regulator *regulator, unsigned int mode)
if (ret < 0)
goto out;
+ ret = handle_monitors_mode(rdev, true, mode);
+ if (ret)
+ goto out;
+
ret = rdev->desc->ops->set_mode(rdev, mode);
+ if (ret)
+ goto out;
+
+ ret = handle_monitors_mode(rdev, false, mode);
+
out:
regulator_unlock(rdev);
return ret;
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 621b7f4a3639..1cfd10ec13a5 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -83,6 +83,36 @@ struct regulator_state {
bool changeable;
};
+/**
+ * struct monitoring_constraints - regulator monitoring constraints.
+ *
+ * This struct describes monitoring specific constraints.
+ *
+ * The constraints should be set by a driver if an enable/disable or regulator MODE switch does not
+ * change the state of the monitor implicitly. When used, the core handles the monitoring of a
+ * dynamic regulator implicitly on state/mode change, based on this configuration. This should
+ * avoid that the monitor reaches an invalid state.
+ *
+ * @mon_disable_during_reg_set: Monitor should be disabled before and enabled after the regulators'
+ * value is changed
+ * @mon_disable_during_reg_off: Monitor should be disabled before a regulator disable and enabled
+ * after a regulator enable
+ *
+ * @mon_disable_pre_reg_idle: Monitor should be disabled before a switch to MODE_IDLE
+ * @mon_disable_pre_reg_standby: Monitor should be disabled before a switch to MODE_STANDBY
+ * @mon_enable_post_reg_normal: Monitor should be enabled after a switch to MODE_NORMAL
+ * @mon_enable_post_reg_fast: Monitor should be enabled after a switch to MODE_FAST
+ */
+struct monitoring_constraints {
+ unsigned mon_disable_during_reg_set:1;
+ unsigned mon_disable_during_reg_off:1;
+
+ unsigned mon_disable_pre_reg_idle:1;
+ unsigned mon_disable_pre_reg_standby:1;
+ unsigned mon_enable_post_reg_normal:1;
+ unsigned mon_enable_post_reg_fast:1;
+};
+
#define REGULATOR_NOTIF_LIMIT_DISABLE -1
#define REGULATOR_NOTIF_LIMIT_ENABLE -2
struct notification_limit {
@@ -207,6 +237,10 @@ struct regulation_constraints {
unsigned int active_discharge;
+ /* monitoring constraints */
+ const struct monitoring_constraints *ov_constraints;
+ const struct monitoring_constraints *uv_constraints;
+
/* constraint flags */
unsigned always_on:1; /* regulator never off when system is on */
unsigned boot_on:1; /* bootloader/firmware enabled regulator */
--
2.34.1
From: Benjamin Bara <[email protected]>
The PWR_OK state of the da9063 indicates whether all monitored voltages
are within the expected range. When a regulator is disabled without
disabling its voltage monitor before, the PWR_OK state becomes false. On
our board, this invalid state leads to a hard reset.
Therefore, prevent the invalid state by disabling the monitor before the
regulator is disabled.
This still requires to explicitly enable the voltage monitor in the
device tree and has no impact otherwise.
TODO: clarify if a MODE change has impact on the voltage monitor
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/da9063-regulator.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 82f52a2a031a..0fa320abad93 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -713,14 +713,27 @@ static irqreturn_t da9063_ldo_lim_event(int irq, void *data)
/*
* Probing and Initialisation functions
*/
+
+/* prevent !PWR_OK state by disabling monitoring while regulator is off. */
+static const struct monitoring_constraints da9063_vmon_constraints = {
+ .mon_disable_during_reg_off = 1,
+};
+
static const struct regulator_init_data *da9063_get_regulator_initdata(
const struct da9063_regulators_pdata *regl_pdata, int id)
{
+ struct regulator_init_data *initdata;
+ struct regulation_constraints *reg_constr;
int i;
for (i = 0; i < regl_pdata->n_regulators; i++) {
- if (id == regl_pdata->regulator_data[i].id)
- return regl_pdata->regulator_data[i].initdata;
+ if (id == regl_pdata->regulator_data[i].id) {
+ initdata = regl_pdata->regulator_data[i].initdata;
+ reg_constr = &initdata->constraints;
+ reg_constr->ov_constraints = &da9063_vmon_constraints;
+ reg_constr->uv_constraints = &da9063_vmon_constraints;
+ return initdata;
+ }
}
return NULL;
--
2.34.1
Hi Benjamin,
On 4/20/23 13:29, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> Add constraints for regulator monitoring. These are useful when the
> state of the regulator might change during runtime, but the monitor
> state (in hardware) is not implicitly changed with the change of the
> regulator state or mode (in hardware).
>
> When used, the core takes care of handling the monitor state. This could
> ensure that a monitor does not stay active when its regulator is
> disabled.
I think this could be useful feature. AFAIR for example the bd718x7
regulator driver needs to disable the monitoring for the duration of a
voltage change.
> +static int handle_monitors(struct regulator_dev *rdev, bool pre, bool enable, bool change,
> + unsigned int mode)
> +{
> + const struct regulator_ops *ops = rdev->desc->ops;
> + const struct regulation_constraints *reg_c = rdev->constraints;
> +
> + /*
> + * ensure that voltage monitoring is explicitly enabled in the device tree and that the
> + * driver has monitoring constraints and protection ops.
> + */
> + bool handle_ov = reg_c->over_voltage_detection && reg_c->ov_constraints &&
> + ops->set_over_voltage_protection;
> + bool handle_uv = reg_c->under_voltage_detection && reg_c->uv_constraints &&
> + ops->set_under_voltage_protection;
Hm. Is there a reason why we need to perform these checks for each of
the calls? Could we do checks when regulator is registered? Also, could
we just directly have function pointers to monitoring disabling which
would be populated by the driver. Core would then call these callbacks
if they were populated (instead of these flags and unconditional call to
handling). This might have other benefits as well (please see below).
> + int ret = 0;
> +
> + if (!handle_ov && !handle_uv)
> + return 0;
> +
> + dev_dbg(&rdev->dev, "%s: pre: %d, en: %d, ch: %d, mode: %u\n", __func__, pre, enable,
> + change, mode);
> + if ((enable + change + !!mode) > 1) {
> + dev_err(&rdev->dev, "%s: invalid combination!\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (handle_ov) {
> + if (should_disable_monitor(reg_c->ov_constraints, pre, enable, change, mode))
> + ret = handle_notify_limits(rdev, ops->set_over_voltage_protection,
> + &disable_limits);
> + else if (should_enable_monitor(reg_c->ov_constraints, pre, enable, change, mode))
> + ret = handle_notify_limits(rdev, ops->set_over_voltage_protection,
> + ®_c->over_voltage_limits);
> + }
> + if (ret)
> + return ret;
> +
> + if (handle_uv) {
> + if (should_disable_monitor(reg_c->uv_constraints, pre, enable, change, mode))
> + ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
> + &disable_limits);
> + else if (should_enable_monitor(reg_c->uv_constraints, pre, enable, change, mode))
> + ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
> + ®_c->under_voltage_limits);
> + }
> +
> + return ret;
> +}
> +
> +static inline int handle_monitors_disable(struct regulator_dev *rdev)
> +{
> + return handle_monitors(rdev, true, false, false, REGULATOR_MODE_INVALID);
> +}
> +
> +static inline int handle_monitors_enable(struct regulator_dev *rdev)
> +{
> + return handle_monitors(rdev, false, true, false, REGULATOR_MODE_INVALID);
> +}
> +
> +static inline int handle_monitors_set(struct regulator_dev *rdev, bool pre)
> +{
> + return handle_monitors(rdev, pre, false, true, REGULATOR_MODE_INVALID);
> +}
> +
> +static inline int handle_monitors_mode(struct regulator_dev *rdev, bool pre, unsigned int mode)
> +{
> + return handle_monitors(rdev, pre, false, false, mode);
> +}
> +
> /**
> * set_machine_constraints - sets regulator constraints
> * @rdev: regulator source
> @@ -1512,7 +1612,8 @@ static int set_machine_constraints(struct regulator_dev *rdev)
> "IC does not support requested over-current limits\n");
> }
>
> - if (rdev->constraints->over_voltage_detection)
> + /* only if we have static monitoring. with dynamic, it will be set according to state. */
> + if (rdev->constraints->over_voltage_detection && !rdev->constraints->ov_constraints)
> ret = handle_notify_limits(rdev,
> ops->set_over_voltage_protection,
> &rdev->constraints->over_voltage_limits);
> @@ -1526,7 +1627,8 @@ static int set_machine_constraints(struct regulator_dev *rdev)
> "IC does not support requested over voltage limits\n");
> }
>
> - if (rdev->constraints->under_voltage_detection)
> + /* only if we have static monitoring. with dynamic, it will be set according to state. */
> + if (rdev->constraints->under_voltage_detection && !rdev->constraints->uv_constraints)
> ret = handle_notify_limits(rdev,
> ops->set_under_voltage_protection,
> &rdev->constraints->under_voltage_limits);
> @@ -2734,7 +2836,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
>
> trace_regulator_enable_complete(rdev_get_name(rdev));
>
> - return 0;
> + return handle_monitors_enable(rdev);
Eg, instead of unconditional call to handle_monitors_enable() could we
here check if driver has given us a pointer to monitoring disabling
function? Or, do you think it would be worth making this a tiny bit more
generic by just doing some 'pre_enable' and 'post _disable' callbacks
which driver can populate? Maybe we could also add 'ready-to-use'
default helpers which drivers (who just want to propagate the call to
ops->set_over_voltage_protection et al) could use.
I think that some PMICs I've seen had separate 'disable/enable all
monitors' bit which needed to be used when monitoring was
disabled/enabled due to an operation [voltage change/disable/enable].
If we allowed driver to populate the disabling callbacks, then this
would support also those ICs which need to disable multiple monitors for
the duration of an operation (or do some other strange stuff). It would
also allow drivers to add delays to the function(s) re-enabling of
monitors when needed - at least the bd718x7 had to wait for new voltage
to stabilize prior re-enabling the monitors.
>
> +/**
> + * struct monitoring_constraints - regulator monitoring constraints.
> + *
> + * This struct describes monitoring specific constraints.
> + *
> + * The constraints should be set by a driver if an enable/disable or regulator MODE switch does not
> + * change the state of the monitor implicitly. When used, the core handles the monitoring of a
> + * dynamic regulator implicitly on state/mode change, based on this configuration. This should
> + * avoid that the monitor reaches an invalid state.
> + *
> + * @mon_disable_during_reg_set: Monitor should be disabled before and enabled after the regulators'
> + * value is changed
> + * @mon_disable_during_reg_off: Monitor should be disabled before a regulator disable and enabled
> + * after a regulator enable
> + *
> + * @mon_disable_pre_reg_idle: Monitor should be disabled before a switch to MODE_IDLE
> + * @mon_disable_pre_reg_standby: Monitor should be disabled before a switch to MODE_STANDBY
> + * @mon_enable_post_reg_normal: Monitor should be enabled after a switch to MODE_NORMAL
> + * @mon_enable_post_reg_fast: Monitor should be enabled after a switch to MODE_FAST
> + */
> +struct monitoring_constraints {
> + unsigned mon_disable_during_reg_set:1;
> + unsigned mon_disable_during_reg_off:1;
> +
> + unsigned mon_disable_pre_reg_idle:1;
> + unsigned mon_disable_pre_reg_standby:1;
> + unsigned mon_enable_post_reg_normal:1;
> + unsigned mon_enable_post_reg_fast:1;
> +};
So, could we perhaps have function pointers for these in the ops instead
of flags? Core could then call these if set? Do you think that would work?
> +
> #define REGULATOR_NOTIF_LIMIT_DISABLE -1
> #define REGULATOR_NOTIF_LIMIT_ENABLE -2
> struct notification_limit {
> @@ -207,6 +237,10 @@ struct regulation_constraints {
>
> unsigned int active_discharge;
>
> + /* monitoring constraints */
> + const struct monitoring_constraints *ov_constraints;
> + const struct monitoring_constraints *uv_constraints;
> +
> /* constraint flags */
> unsigned always_on:1; /* regulator never off when system is on */
> unsigned boot_on:1; /* bootloader/firmware enabled regulator */
>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Thu, Apr 20, 2023 at 12:29:20PM +0200, Benjamin Bara wrote:
> Add constraints for regulator monitoring. These are useful when the
> state of the regulator might change during runtime, but the monitor
> state (in hardware) is not implicitly changed with the change of the
> regulator state or mode (in hardware).
> When used, the core takes care of handling the monitor state. This could
> ensure that a monitor does not stay active when its regulator is
> disabled.
Are these constraints (ie, board specific limits) or are these more just
properties of the regulator device? It does feel useful to factor out
this stuff, but it's not clear to me that these are things that should
be configured on a per board basis.
> + * @mon_disable_during_reg_set: Monitor should be disabled before and enabled after the regulators'
> + * value is changed
> + * @mon_disable_during_reg_off: Monitor should be disabled before a regulator disable and enabled
> + * after a regulator enable
> + * @mon_disable_pre_reg_idle: Monitor should be disabled before a switch to MODE_IDLE
> + * @mon_disable_pre_reg_standby: Monitor should be disabled before a switch to MODE_STANDBY
> + * @mon_enable_post_reg_normal: Monitor should be enabled after a switch to MODE_NORMAL
> + * @mon_enable_post_reg_fast: Monitor should be enabled after a switch to MODE_FAST
These all sound like things where the regulator device is simply not
going to support having monitoring enabled when doing the relevant
actions no matter what situation we're in. If that's the case we should
just have the regulator driver set things up.
For the modes might it be clearer to mark a set of modes as not
supporting monitoring? I think that's the intended effect here.
Thanks for the feedback!
On Thu, 20 Apr 2023 at 13:33, Matti Vaittinen <[email protected]> wrote:
> Hm. Is there a reason why we need to perform these checks for each of
> the calls?
No, I guess there might be a more efficient way to set a bit somewhere
during registration instead. But I thought it might be "clearer" to have
it all in one function to clarify what is required that something
actually happens.
> Also, could we just directly have function pointers to monitoring
> disabling which would be populated by the driver.
> So, could we perhaps have function pointers for these in the ops
> instead of flags? Core could then call these if set? Do you think that
> would work?
One goal was to reuse the existing set_{under,over}_voltage_protection()
methods for voltage monitoring. For me, disabling is basically the same
as setting the limit to REGULATOR_NOTIF_LIMIT_DISABLE. Of course, we
could also introduce new "disable_monitor_on_*()" ops, but I think it
should do the same job as set_*_protection(disable_limits), and
therefore only leads to duplicated code in every driver that wants to
use "dynamic monitoring". Also, I think we might need at least 6 new ops
methods to cover the different state changes, except if we do some kind
of "old state -> new state" function to identify if turning off or on
the monitor is required (which is basically now done in the core).
I am not sure if it improves things, but I could modify the approach to
be more "driver-centric". What do you think?
> Or, do you think it would be worth making this a tiny bit more generic
> by just doing some 'pre_enable' and 'post _disable' callbacks which
> driver can populate?
To be honest, I am not sure. For the da9063, it might not be worth it.
For others, it might simplify the usage of voltage monitoring and move
the "mental complexity" of handling all the possible cases, where the
voltage monitoring must be turned off, to the core.
The driver must just set the monitoring constraints to "please turn off
while the regulator is turned off, turn off while the voltage is changed
and while the regulator is in STANDBY mode".
For me, it would also be fine to let the core turn off voltage monitoring on
all defined cases (while regulator is off, while voltage is changed, while in
IDLE, STANDBY mode). The constraints would just let the driver decide more
specifically when it is really necessary and skip the other cases.
> I think that some PMICs I've seen had separate 'disable/enable all
> monitors' bit which needed to be used when monitoring was
> disabled/enabled due to an operation [voltage change/disable/enable].
I think this case could be handled by my "possible next step" in a very
simple way.
> If we allowed driver to populate the disabling callbacks, then this
> would support also those ICs which need to disable multiple monitors
> for the duration of an operation (or do some other strange stuff)
I think that these should also be handled in the case of
set_*_protection(), but I am not 100% sure here.
> It would also allow drivers to add delays to the function(s)
> re-enabling of monitors when needed - at least the bd718x7 had to wait
> for new voltage to stabilize prior re-enabling the monitors.
Also not 100% sure about this one, but I think these cases could be
covered by a mandatory regulator-*-ramp-delay, when necessary?
I can provide a RFC v2 with the stuff handled from the driver instead.
Thanks and best regards,
Benjamin
Thanks for the feedback!
On Thu, 20 Apr 2023 at 14:17, Mark Brown <[email protected]> wrote:
> Are these constraints (ie, board specific limits) or are these more
> just properties of the regulator device? It does feel useful to
> factor out this stuff, but it's not clear to me that these are things
> that should be configured on a per board basis.
These are actually properties of the regulator device. However, the
properties are only "active" if the voltage monitoring is wanted, which
is currently a per-board decision. Not sure if there might be reasons to
not activate it.
> These all sound like things where the regulator device is simply not
> going to support having monitoring enabled when doing the relevant
> actions no matter what situation we're in. If that's the case we
> should just have the regulator driver set things up.
I think this would be feasible if the driver decides whether monitoring
is on or off (which might be a way to go). I think if the decision is
done per-board, it might simplify things to have the whole "should I
turn the monitor off now?" overhead not duplicated in every driver that
supports monitoring. What do you think?
> For the modes might it be clearer to mark a set of modes as not
> supporting monitoring? I think that's the intended effect here.
Yes, that's true. I will change that.
Thanks and best regards,
Benjamin
On Thu, Apr 20, 2023 at 04:30:45PM +0200, Benjamin Bara wrote:
> On Thu, 20 Apr 2023 at 14:17, Mark Brown <[email protected]> wrote:
> > Are these constraints (ie, board specific limits) or are these more
> > just properties of the regulator device? It does feel useful to
> > factor out this stuff, but it's not clear to me that these are things
> > that should be configured on a per board basis.
> These are actually properties of the regulator device. However, the
> properties are only "active" if the voltage monitoring is wanted, which
> is currently a per-board decision. Not sure if there might be reasons to
> not activate it.
Right, but in any case where the monitoring is enabled then these
properties would also be needed so there's no point in separately
configuring it.
> > These all sound like things where the regulator device is simply not
> > going to support having monitoring enabled when doing the relevant
> > actions no matter what situation we're in. If that's the case we
> > should just have the regulator driver set things up.
> I think this would be feasible if the driver decides whether monitoring
> is on or off (which might be a way to go). I think if the decision is
> done per-board, it might simplify things to have the whole "should I
> turn the monitor off now?" overhead not duplicated in every driver that
> supports monitoring. What do you think?
The driver can supply flags to tell the core to do things like it
already does for a whole range of other things, there's no need to force
things to be configured per system in order to factor things out. It's
just a question of where the core gets information from.
On Thu, 20 Apr 2023 at 16:37, Mark Brown <[email protected]> wrote:
> Right, but in any case where the monitoring is enabled then these
> properties would also be needed so there's no point in separately
> configuring it.
> The driver can supply flags to tell the core to do things like it
> already does for a whole range of other things, there's no need to
> force things to be configured per system in order to factor things
> out. It's just a question of where the core gets information from.
Ah, got it. Thanks for clarification.
On Thu, Apr 20, 2023 at 04:29:24PM +0200, Benjamin Bara wrote:
> On Thu, 20 Apr 2023 at 13:33, Matti Vaittinen <[email protected]> wrote:
> > It would also allow drivers to add delays to the function(s)
> > re-enabling of monitors when needed - at least the bd718x7 had to wait
> > for new voltage to stabilize prior re-enabling the monitors.
> Also not 100% sure about this one, but I think these cases could be
> covered by a mandatory regulator-*-ramp-delay, when necessary?
If the voltage is out of spec there should be a ramp delay anyway, yes.