Re: [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable()

[Bjorn Andersson]

On Tue 02 Jun 03:09 PDT 2020, Sumit Semwal wrote:

> Some regulators might need to verify that they have indeed been enabled
> after the enable() call is made and enable_time delay has passed.
> 
> This is implemented by repeatedly checking is_enabled() upto
> poll_enabled_time, waiting for the already calculated enable delay in
> each iteration.
> 
> Signed-off-by: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> --
> v2: Address review comments, including swapping enable_time and poll_enabled_time.
> ---
>  drivers/regulator/core.c         | 58 +++++++++++++++++++++++++++++++-
>  include/linux/regulator/driver.h |  5 +++
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 7486f6e4e613..d9ab888da95f 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2347,6 +2347,32 @@ static void _regulator_enable_delay(unsigned int delay)
>  		udelay(us);
>  }
>  
> +/* _regulator_check_status_enabled

Please make all your kerneldoc follow:
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

> + *
> + * returns:
> + *          1 if status shows regulator is in enabled state
> + *          0 if not enabled state
> + *          else, error value as received from ops->get_status()
> + */
> +static inline int _regulator_check_status_enabled(struct regulator_dev *rdev)
> +{
> +	int ret = rdev->desc->ops->get_status(rdev);
> +
> +	if (ret < 0) {
> +		rdev_info(rdev, "get_status returned error: %d\n", ret);
> +		return ret;
> +	}
> +
> +	switch (ret) {
> +	case REGULATOR_STATUS_OFF:
> +	case REGULATOR_STATUS_ERROR:
> +	case REGULATOR_STATUS_UNDEFINED:
> +		return 0;
> +	default:
> +		return 1;
> +	}
> +}
> +
>  static int _regulator_do_enable(struct regulator_dev *rdev)
>  {
>  	int ret, delay;
> @@ -2407,7 +2433,37 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
>  	 * together.  */
>  	trace_regulator_enable_delay(rdev_get_name(rdev));
>  
> -	_regulator_enable_delay(delay);
> +	/* If poll_enabled_time is set, poll upto the delay calculated
> +	 * above, delaying poll_enabled_time uS to check if the regulator
> +	 * actually got enabled.
> +	 * If the regulator isn't enabled after enable_delay has
> +	 * expired, return -ETIMEDOUT.
> +	 */
> +	if (rdev->desc->poll_enabled_time) {
> +		unsigned int time_remaining = delay;
> +
> +		while (time_remaining > 0) {
> +			_regulator_enable_delay(rdev->desc->poll_enabled_time);
> +
> +			if (rdev->desc->ops->get_status) {
> +				ret = _regulator_check_status_enabled(rdev);
> +				if (ret < 0)
> +					return ret;
> +				else if (ret)
> +					break;
> +			} else if (rdev->desc->ops->is_enabled(rdev))
> +				break;
> +
> +			time_remaining -= rdev->desc->poll_enabled_time;
> +		}
> +
> +		if (time_remaining <= 0) {
> +			rdev_err(rdev, "Enabled check failed.\n");
> +			return -ETIMEDOUT;
> +		}
> +	} else {
> +		_regulator_enable_delay(delay);
> +	}
>  
>  	trace_regulator_enable_complete(rdev_get_name(rdev));
>  
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index 29d920516e0b..bb50e943010f 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -322,6 +322,9 @@ enum regulator_type {
>   * @enable_time: Time taken for initial enable of regulator (in uS).
>   * @off_on_delay: guard time (in uS), before re-enabling a regulator
>   *
> + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is
> + *                          actually enabled, after enable() call

I read this as "how long should we stay in the poll loop". I think it
would be better describes as something like "The polling interval to use
while checking that the regulator was actually enabled".

Regards,
Bjorn

> + *
>   * @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
>   */
>  struct regulator_desc {
> @@ -389,6 +392,8 @@ struct regulator_desc {
>  
>  	unsigned int off_on_delay;
>  
> +	unsigned int poll_enabled_time;
> +
>  	unsigned int (*of_map_mode)(unsigned int mode);
>  };
>  
> -- 
> 2.26.2
>