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 <[email protected]>
> --
> 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
> 

Reply via email to