Re: [PATCH RFC v4 09/13] regulator: implement mon_disable_reg_disabled

[Matti Vaittinen]

Hi deeee Ho Benjamin,

I hope your train back to home was not delayed too much ;)

On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
>
> The mon_disable_reg_disabled

The name of this always makes me to scratch my head a bit. (or, maybe it 
is just the sunburns at my bald).

Do you think making it:
mon_disable_at_reg_disable or mon_disable_when_reg_disabled would be too 
long?

> property disables all dt-enabled monitors
> before a regulator is disabled. If an error occurs while disabling the
> regulator, the monitors are enabled again.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> ---
>   drivers/regulator/core.c | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 873e53633698..b37dcafff407 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
>   
>   	trace_regulator_enable_complete(rdev_get_name(rdev));
>   
> -	return 0;
> +	return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);

As I wrote in my comment to previous patch, I might find the logic a bit 
more clear if the condition check was done here. Eg:

	if (rdev->desc->mon_disable_when_reg_disabled)
		return monitors_reenable(...);

	return 0;

>   }
>   
>   /**
> @@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
>   
>   static int _regulator_do_disable(struct regulator_dev *rdev)
>   {
> +	const struct regulator_desc *desc = rdev->desc;
>   	int ret;
>   
> +	ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
> +	if (ret)
> +		return ret;

Similarly, for me the logic would be easier to follow if this was:

	if (desc->mon_disable_when_reg_disabled)
		monitors_disable(...);

> +
>   	trace_regulator_disable(rdev_get_name(rdev));
>   
>   	if (rdev->ena_pin) {
> @@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
>   			rdev->ena_gpio_state = 0;
>   		}
>   
> -	} else if (rdev->desc->ops->disable) {
> -		ret = rdev->desc->ops->disable(rdev);
> +	} else if (desc->ops->disable) {
> +		ret = desc->ops->disable(rdev);
>   		if (ret != 0)
>   			return ret;
>   	}
>   
> -	if (rdev->desc->off_on_delay)
> +	if (desc->off_on_delay)
>   		rdev->last_off = ktime_get_boottime();
>   
>   	trace_regulator_disable_complete(rdev_get_name(rdev));
> @@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
>   				_notifier_call_chain(rdev,
>   						REGULATOR_EVENT_ABORT_DISABLE,
>   						NULL);
> +				monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);

same here,

>   				return ret;
>   			}
>   			_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> @@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
>   		rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
>   		_notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
>   				REGULATOR_EVENT_ABORT_DISABLE, NULL);
> +		monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);

here...

>   		return ret;
>   	}
>   
> @@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
>   		 */
>   		rdev_info(rdev, "disabling\n");
>   		ret = _regulator_do_disable(rdev);
> -		if (ret != 0)
> +		if (ret != 0) {
>   			rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
> +			monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);

... and here.
> +		}
>   	} else {
>   		/* The intention is that in future we will
>   		 * assume that full constraints are provided

These were just very minor things. Mostly looks good for me.


Yours,
	-- Matti

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

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