lkml.org 
[lkml]   [2023]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 ? &reg_c->over_voltage_limits : &disable_limits);
> + if (!ret && reg_c->under_voltage_detection)
> + ret = handle_notify_limits(rdev, ops->set_under_voltage_protection,
> + enable ? &reg_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! ~~

\
 
 \ /
  Last update: 2023-04-24 15:32    [W:0.048 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site