Messages in this thread | | | Date | Mon, 26 Jun 2023 17:04:58 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCH RFC v4 08/13] regulator: move monitor handling into own function |
| |
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 ? ®_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 ? ®_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 ? ®_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 ? ®_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! ~~
| |