| Message ID | 20200602100924.26256-2-sumit.semwal@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Qualcomm labibb regulator driver | expand |
On Tue, Jun 02, 2020 at 03:39:20PM +0530, Sumit Semwal wrote: > + > + if (time_remaining <= 0) { > + rdev_err(rdev, "Enabled check failed.\n"); > + return -ETIMEDOUT; s/failed/timed out/ > + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is > + * actually enabled, after enable() call > + * This comment needs updating to reflect the new implementation.
Hi Mark, Thanks for the review! On Tue, 2 Jun 2020 at 16:54, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Jun 02, 2020 at 03:39:20PM +0530, Sumit Semwal wrote: > > > + > > + if (time_remaining <= 0) { > > + rdev_err(rdev, "Enabled check failed.\n"); > > + return -ETIMEDOUT; > > s/failed/timed out/ Ack > > > + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is > > + * actually enabled, after enable() call > > + * > > This comment needs updating to reflect the new implementation. Yes, I will update. Best, Sumit.
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@linaro.org> > -- > 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 >
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 + * + * 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 + * * @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); };
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@linaro.org> -- 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(-)