Messages in this thread | | | Date | Mon, 24 Apr 2023 16:30:35 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCH RFC v2 1/2] regulator: add properties to disable monitoring on actions |
| |
On 4/21/23 12:13, 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(). > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com> > --- > drivers/regulator/core.c | 64 ++++++++++++++++++++++++++++++++++++---- > include/linux/regulator/driver.h | 10 +++++++ > 2 files changed, 68 insertions(+), 6 deletions(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 4fcd36055b02..5052e1da85a7 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,29 @@ static int handle_notify_limits(struct regulator_dev *rdev, > > return ret; > } > + > +static const struct notification_limit disable_limits = { > + .prot = REGULATOR_NOTIF_LIMIT_DISABLE, > + .err = REGULATOR_NOTIF_LIMIT_DISABLE, > + .warn = REGULATOR_NOTIF_LIMIT_DISABLE, > +}; > + > +static int monitors_set_state(struct regulator_dev *rdev, bool enable) > +{ > + const struct regulation_constraints *reg_c = rdev->constraints; > + const struct regulator_ops *ops = rdev->desc->ops; > + int ret = 0; > + > + /* only set the state if monitoring is activated in the device-tree. */ > + if (reg_c->over_voltage_detection) > + ret = handle_notify_limits(rdev, ops->set_over_voltage_protection, > + enable ? ®_c->over_voltage_limits : &disable_limits); > + if (!ret && reg_c->under_voltage_detection) > + ret = handle_notify_limits(rdev, ops->set_under_voltage_protection, > + enable ? ®_c->under_voltage_limits : &disable_limits);
I still think forcing the use of the set_over_voltage_protection() / set_under_voltage_protection() (and omitting over-current protection) instead of allowing the driver to populate potentially more suitable callbacks is somewhat limiting.
For example, the only _in-tree_ regulator driver that I know is disabling monitors at voltage change is the bd718x7. There we have extra conditions for disabling - the power must be enabled (which could probably be a generic condition for disabling monitoring at voltage change), and also the new voltage must be greater than the old voltage. This logic is naturally implemented in set_under_voltage_protection - which should unconditionally disable the monitoring if it is requested via device-tree.
After that being said - I am leaving weighing pros and cons to You and Mark - I don't feel I am in a position where I should dictate the design here. I'll just say that if the new generic disabling unconditionally uses set_under_voltage_protection - then at least the bd718x7 can not benefit from it w/o relaxing the monitoring.
Finally, thanks Benjamin for improving things here!
Yours, -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |