From: Matti Vaittinen <mazziesaccount@gmail.com>
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>
Subject: Re: [PATCH RFC v4 08/13] regulator: move monitor handling into own function
Date: Mon, 26 Jun 2023 17:04:58 +0300 [thread overview]
Message-ID: <2938c236-1144-d49e-f02e-2c2e99ce17af@gmail.com> (raw)
In-Reply-To: <20230419-dynamic-vmon-v4-8-4d3734e62ada@skidata.com>
On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> Extract the current monitor handling into an own function and create
> helper for initialization, disabling and re-enabling of monitors.
> For reenabling the monitors, the current state and mode is considered to
> avoid entering an invalid state on regulators with enabled workarounds.
>
> Additionally, monitors of disabled regulators are not disabled before
> changing state. The mon_disable_reg_disabled property is still respected
> in this case, because turning off the monitor happens when the regulator
> is still enabled.
>
> Differ between initialization and normal "workaround handling" when an
> EOPNOTSUPP is returned.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> drivers/regulator/core.c | 234 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 178 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9bddab17450e..873e53633698 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1426,7 +1426,7 @@ static int notif_set_limit(struct regulator_dev *rdev,
>
> static int handle_notify_limits(struct regulator_dev *rdev,
> int (*set)(struct regulator_dev *, int, int, bool),
> - struct notification_limit *limits)
> + const struct notification_limit *limits)
> {
> int ret = 0;
>
> @@ -1451,6 +1451,180 @@ static int handle_notify_limits(struct regulator_dev *rdev,
>
> return ret;
> }
> +
> +static const struct notification_limit disable_limits = {
> + .prot = REGULATOR_NOTIF_LIMIT_DISABLE,
> + .err = REGULATOR_NOTIF_LIMIT_DISABLE,
> + .warn = REGULATOR_NOTIF_LIMIT_DISABLE,
> +};
> +
> +static int monitors_set_state(struct regulator_dev *rdev, bool enable,
> + unsigned int mons)
> +{
> + const struct regulation_constraints *reg_c = rdev->constraints;
> + const struct regulator_ops *ops = rdev->desc->ops;
> + int tmp, ret = 0;
> +
> + rdev_dbg(rdev, "%s: en: %d, mons: %x\n", __func__, enable, mons);
> +
> + /* only set the state if monitoring is activated in the device-tree. */
> + if ((mons & REGULATOR_MONITOR_OVER_VOLTAGE) && reg_c->over_voltage_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_over_voltage_protection,
> + enable ? ®_c->over_voltage_limits
> + : &disable_limits);
> + if (tmp) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set over voltage limits %pe\n",
> + ERR_PTR(tmp));
> + return tmp;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested over voltage limits\n");
> + ret = tmp;
> + }
> + }
> + if ((mons & REGULATOR_MONITOR_UNDER_VOLTAGE) && reg_c->under_voltage_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_under_voltage_protection,
> + enable ? ®_c->under_voltage_limits
> + : &disable_limits);
> + if (tmp) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set under voltage limits %pe\n",
> + ERR_PTR(tmp));
> + return ret;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested under voltage limits\n");
> + ret = tmp;
> + }
> + }
> + if ((mons & REGULATOR_MONITOR_OVER_CURRENT) && reg_c->over_current_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_over_current_protection,
> + enable ? ®_c->over_curr_limits
> + : &disable_limits);
> + if (ret) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set over current limits: %pe\n",
> + ERR_PTR(tmp));
> + return tmp;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested over-current limits\n");
> + ret = tmp;
> + }
> + }
> + if ((mons & REGULATOR_MONITOR_OVER_TEMPERATURE) && reg_c->over_temp_detection) {
> + tmp = handle_notify_limits(rdev, ops->set_thermal_protection,
> + enable ? ®_c->temp_limits
> + : &disable_limits);
> + if (tmp) {
> + if (tmp != -EOPNOTSUPP) {
> + rdev_err(rdev, "failed to set temperature limits %pe\n",
> + ERR_PTR(tmp));
> + return tmp;
> + }
> + rdev_warn(rdev,
> + "IC does not support requested temperature limits\n");
> + ret = tmp;
> + }
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * monitors_disable - disables given monitors if the regulator is enabled
> + * @rdev: regulator source
> + * @mons: monitors to enable
> + */
> +static int monitors_disable(struct regulator_dev *rdev, unsigned int mons)
> +{
> + int reg_enabled;
> +
> + if (!mons)
> + return 0;
Just a minor thing but can we do this check already at the caller side?
I think that would show the logic more clearly already in the functions
implementing the actual action requested by the user.
(disable/enable/change voltage). Eg, the logic in those functions would
be clear:
if (flag_to_do_magic_monitor_toggling)
monitors_disable();
and similarly for the monitors_reenable()...
> +
> + reg_enabled = _regulator_is_enabled(rdev);
> + if (reg_enabled <= 0)
> + return reg_enabled;
> +
> + return monitors_set_state(rdev, false, mons);
> +}
> +
> +/**
> + * monitors_enable - enables given monitors
> + * @rdev: regulator source
> + * @mons: monitors to enable
> + *
> + * Enables monitors based on their workaround properties and the current state
> + * or mode.
> + */
> +static int monitors_enable(struct regulator_dev *rdev, unsigned int mons)
> +{
> + const struct regulator_desc *desc = rdev->desc;
> + const struct regulator_ops *ops = desc->ops;
> +
> + /* don't enable monitors if regulator is in unsupported mode. */
> + if (desc->mon_unsupported_reg_modes &&
> + (desc->mon_unsupported_reg_modes & ops->get_mode(rdev)))
> + return 0;
> +
> + /* don't enable monitor on disabled regulator with workaround active. */
> + if (mons & desc->mon_disable_reg_disabled) {
> + int reg_enabled = _regulator_is_enabled(rdev);
> +
> + if (reg_enabled < 0)
> + return reg_enabled;
> + if (!reg_enabled)
> + mons &= ~desc->mon_disable_reg_disabled;
> + }
> +
> + return monitors_set_state(rdev, true, mons);
> +}
> +
> +static int monitors_init(struct regulator_dev *rdev)
> +{
> + unsigned int mons = REGULATOR_MONITOR_NONE;
> + int reg_enabled = _regulator_is_enabled(rdev);
> + int ret;
> +
> + /*
> + * Ensure that monitors of disabled regulators with respective
> + * workaround active are disabled during initialization.
> + */
> + if (reg_enabled < 0)
> + return reg_enabled;
> + if (!reg_enabled && rdev->desc->mon_disable_reg_disabled) {
> + mons = rdev->desc->mon_disable_reg_disabled;
> + ret = monitors_set_state(rdev, false, mons);
> + }
> +
> + /* Ignore EOPNOTSUPP at initialization, but not during workarounds. */
> + ret = monitors_enable(rdev, ~mons);
> + if (ret && ret != -EOPNOTSUPP)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int monitors_reenable(struct regulator_dev *rdev, unsigned int mons)
> +{
> + int reg_enabled;
> +
> + if (!mons)
> + return 0;
> +
...here.
> + /*
> + * Monitors of disabled regulators are not turned off, therefore skip
> + * turning on.
> + */
> + reg_enabled = _regulator_is_enabled(rdev);
> + if (reg_enabled <= 0)
> + return reg_enabled;
> +
> + return monitors_enable(rdev, mons);
> +}
> +
Sorry but my flight landed so I need to stop reviewing for now... This
will be a busy week for me. It may be I can't go through rest of the
patches until later :/
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
next prev parent reply other threads:[~2023-06-26 14:05 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-20 20:02 [PATCH RFC v4 00/13] regulator: dynamic voltage monitoring support Benjamin Bara
2023-06-20 20:02 ` [PATCH RFC v4 01/13] regulator: da9063: fix null pointer deref with partial DT config Benjamin Bara
2023-06-20 20:02 ` [PATCH RFC v4 02/13] regulator: add getter for active monitors Benjamin Bara
2023-06-26 13:43 ` Matti Vaittinen
2023-06-26 16:37 ` Mark Brown
2023-06-20 20:02 ` [PATCH RFC v4 03/13] regulator: da9063: implement get_active_protections() Benjamin Bara
2023-06-20 20:02 ` [PATCH RFC v4 04/13] regulator: bd718x7: " Benjamin Bara
2023-06-26 13:45 ` Matti Vaittinen
2023-06-20 20:02 ` [PATCH RFC v4 05/13] regulator: introduce properties for monitoring workarounds Benjamin Bara
2023-06-26 13:47 ` Matti Vaittinen
2023-06-20 20:02 ` [PATCH RFC v4 06/13] regulator: set required ops " Benjamin Bara
2023-06-26 13:49 ` Matti Vaittinen
2023-06-20 20:03 ` [PATCH RFC v4 07/13] regulator: find active protections during initialization Benjamin Bara
2023-06-26 13:56 ` Matti Vaittinen
2023-06-26 16:49 ` Mark Brown
2023-07-03 18:43 ` Benjamin Bara
2023-07-04 13:49 ` Mark Brown
2023-06-20 20:03 ` [PATCH RFC v4 08/13] regulator: move monitor handling into own function Benjamin Bara
2023-06-26 14:04 ` Matti Vaittinen [this message]
2023-06-20 20:03 ` [PATCH RFC v4 09/13] regulator: implement mon_disable_reg_disabled Benjamin Bara
2023-07-03 10:31 ` Matti Vaittinen
2023-07-03 18:50 ` Benjamin Bara
2023-06-20 20:03 ` [PATCH RFC v4 10/13] regulator: implement mon_disable_reg_set_{higher,lower} Benjamin Bara
2023-07-03 10:45 ` Matti Vaittinen
2023-06-20 20:03 ` [PATCH RFC v4 11/13] regulator: implement mon_unsupported_reg_modes Benjamin Bara
2023-07-03 10:49 ` Matti Vaittinen
2023-06-20 20:03 ` [PATCH RFC v4 12/13] regulator: da9063: let the core handle the monitors Benjamin Bara
2023-06-20 20:03 ` [PATCH RFC v4 13/13] regulator: bd718x7: " Benjamin Bara
2023-07-03 11:02 ` Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2938c236-1144-d49e-f02e-2c2e99ce17af@gmail.com \
--to=mazziesaccount@gmail.com \
--cc=DLG-Adam.Ward.opensource@dm.renesas.com \
--cc=bbara93@gmail.com \
--cc=benjamin.bara@skidata.com \
--cc=broonie@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.fuzzey@flowbird.group \
--cc=support.opensource@diasemi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).