Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <244225d6-5391-9f31-5065-c44318d7e235@gmail.com>
Date:   Mon, 24 Apr 2023 16:30:35 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US, en-GB
To:     Benjamin Bara <bbara93@gmail.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>, support.opensource@diasemi.com
Cc:     DLG-Adam.Ward.opensource@dm.renesas.com,
        linux-kernel@vger.kernel.org,
        Benjamin Bara <benjamin.bara@skidata.com>
References: <20230419-dynamic-vmon-v2-0-c303bcc75ebc@skidata.com>
 <20230419-dynamic-vmon-v2-1-c303bcc75ebc@skidata.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH RFC v2 1/2] regulator: add properties to disable
 monitoring on actions
In-Reply-To: <20230419-dynamic-vmon-v2-1-c303bcc75ebc@skidata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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! ~~