Re: [PATCH RFC v2 1/2] regulator: add properties to disable monitoring on actions

[Matti Vaittinen]

On 4/21/23 12:13, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
>   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! ~~