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: <4eeaa617-202b-a69e-9a91-0c955144b36a@gmail.com>
Date:   Mon, 3 Jul 2023 13:45:33 +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>
Cc:     support.opensource@diasemi.com,
        DLG-Adam.Ward.opensource@dm.renesas.com,
        Martin Fuzzey <martin.fuzzey@flowbird.group>,
        linux-kernel@vger.kernel.org,
        Benjamin Bara <benjamin.bara@skidata.com>
References: <20230419-dynamic-vmon-v4-0-4d3734e62ada@skidata.com>
 <20230419-dynamic-vmon-v4-10-4d3734e62ada@skidata.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH RFC v4 10/13] regulator: implement
 mon_disable_reg_set_{higher,lower}
In-Reply-To: <20230419-dynamic-vmon-v4-10-4d3734e62ada@skidata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> The mon_disable_reg_set_{higher,lower} properties disable all dt-enabled
> monitors when the value of the regulator is changed to a higher or lower
> one.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>   drivers/regulator/core.c | 41 +++++++++++++++++++++++++++++++++++++----
>   1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b37dcafff407..74b9c12d38e9 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3643,6 +3643,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
>   				       int min_uV, int max_uV,
>   				       unsigned *selector)
>   {
> +	const struct regulator_desc *desc = rdev->desc;
>   	struct pre_voltage_change_data data;
>   	int ret;
>   
> @@ -3654,7 +3655,18 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
>   	if (ret & NOTIFY_STOP_MASK)
>   		return -EINVAL;
>   
> -	ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
> +	if (min_uV > data.old_uV || max_uV > data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
> +		if (ret)
> +			return ret;

Here, as per comments to previous patches, the logic would be more 
obvious for me if this was:
	if (desc->mon_disable_reg_set_higher &&
	   (min_uV > data.old_uV || max_uV > data.old_uV)) {
		ret = monitors_disable(...)

> +	}
> +	if (min_uV < data.old_uV || max_uV < data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
> +		if (ret)
> +			return ret;
> +	}

I guess you guess what I think of the above by now :)

> +
> +	ret = desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
>   	if (ret >= 0)
>   		return ret;
>   
> @@ -3667,6 +3679,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
>   static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
>   					   int uV, unsigned selector)
>   {
> +	const struct regulator_desc *desc = rdev->desc;
>   	struct pre_voltage_change_data data;
>   	int ret;
>   
> @@ -3678,7 +3691,18 @@ static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
>   	if (ret & NOTIFY_STOP_MASK)
>   		return -EINVAL;
>   
> -	ret = rdev->desc->ops->set_voltage_sel(rdev, selector);
> +	if (uV > data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
> +		if (ret)
> +			return ret;
> +	}
> +	if (uV < data.old_uV) {
> +		ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
> +		if (ret)
> +			return ret;
> +	}

Here I would also pull the check from monitors_disable() to these 
callers just to explicitly show the logic.

> +
> +	ret = desc->ops->set_voltage_sel(rdev, selector);
>   	if (ret >= 0)
>   		return ret;
>   
> @@ -3780,7 +3804,8 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>   	int best_val = 0;
>   	unsigned int selector;
>   	int old_selector = -1;
> -	const struct regulator_ops *ops = rdev->desc->ops;
> +	const struct regulator_desc *desc = rdev->desc;
> +	const struct regulator_ops *ops = desc->ops;
>   	int old_uV = regulator_get_voltage_rdev(rdev);
>   
>   	trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> @@ -3819,7 +3844,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>   				selector = ret;
>   				if (old_selector == selector)
>   					ret = 0;
> -				else if (rdev->desc->vsel_step)
> +				else if (desc->vsel_step)
>   					ret = _regulator_set_voltage_sel_step(
>   						rdev, best_val, selector);
>   				else
> @@ -3874,6 +3899,14 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>   out:
>   	trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
>   
> +	/* if setting voltage failed, ignore monitoring error. */
> +	if (ret)
> +		monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
> +					desc->mon_disable_reg_set_lower);
> +	else
> +		ret = monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
> +					      desc->mon_disable_reg_set_lower);

Here as well.

> +
>   	return ret;
>   }

Well, pulling the check from monitors_*() to callers will increase line 
count quite a bit. Still, my personal take on this is that the logic is 
easier to follow that way. I, however, am fine also with the way it is 
done in these patches if you think the line count matters more.

Yours,
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~