From: Benjamin Bara <benjamin.bara@skidata.com>
Allow to en- and disable voltage monitoring from the device tree.
Consider that the da9063 only monitors UV *and* OV together, so both
must be en- or disabled.
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
drivers/regulator/da9063-regulator.c | 100 +++++++++++++++++++++++++----------
1 file changed, 72 insertions(+), 28 deletions(-)
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 1c720fc595b3..000fa0daef18 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -207,6 +207,24 @@ static const unsigned int da9063_bmem_bio_merged_limits[] = {
4600000, 4800000, 5000000, 5200000, 5400000, 5600000, 5800000, 6000000
};
+static int da9063_set_xvp(struct regulator_dev *rdev, int lim_uV, int severity, bool enable)
+{
+ struct da9063_regulator *regl = rdev_get_drvdata(rdev);
+ struct device *dev = regl->hw->dev;
+
+ dev_dbg(dev, "%s: lim: %d, sev: %d, en: %d\n", regl->desc.name, lim_uV, severity, enable);
+
+ /*
+ * only support enable and disable.
+ * the da9063 offers a GPIO (GP_FB2) which is unasserted if an XV happens.
+ * therefore ignore severity here, as there might be handlers in hardware.
+ */
+ if (lim_uV)
+ return -EINVAL;
+
+ return regmap_field_write(regl->vmon, enable ? 1 : 0);
+}
+
static int da9063_buck_set_mode(struct regulator_dev *rdev, unsigned int mode)
{
struct da9063_regulator *regl = rdev_get_drvdata(rdev);
@@ -545,37 +563,41 @@ static int da9063_buck_get_current_limit(struct regulator_dev *rdev)
}
static const struct regulator_ops da9063_buck_ops = {
- .enable = regulator_enable_regmap,
- .disable = regulator_disable_regmap,
- .is_enabled = regulator_is_enabled_regmap,
- .get_voltage_sel = regulator_get_voltage_sel_regmap,
- .set_voltage_sel = regulator_set_voltage_sel_regmap,
- .list_voltage = regulator_list_voltage_linear,
- .set_current_limit = da9063_buck_set_current_limit,
- .get_current_limit = da9063_buck_get_current_limit,
- .set_mode = da9063_buck_set_mode,
- .get_mode = da9063_buck_get_mode,
- .get_status = da9063_buck_get_status,
- .set_suspend_voltage = da9063_set_suspend_voltage,
- .set_suspend_enable = da9063_suspend_enable,
- .set_suspend_disable = da9063_suspend_disable,
- .set_suspend_mode = da9063_buck_set_suspend_mode,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .list_voltage = regulator_list_voltage_linear,
+ .set_current_limit = da9063_buck_set_current_limit,
+ .get_current_limit = da9063_buck_get_current_limit,
+ .set_mode = da9063_buck_set_mode,
+ .get_mode = da9063_buck_get_mode,
+ .get_status = da9063_buck_get_status,
+ .set_suspend_voltage = da9063_set_suspend_voltage,
+ .set_suspend_enable = da9063_suspend_enable,
+ .set_suspend_disable = da9063_suspend_disable,
+ .set_suspend_mode = da9063_buck_set_suspend_mode,
+ .set_over_voltage_protection = da9063_set_xvp,
+ .set_under_voltage_protection = da9063_set_xvp,
};
static const struct regulator_ops da9063_ldo_ops = {
- .enable = regulator_enable_regmap,
- .disable = regulator_disable_regmap,
- .is_enabled = regulator_is_enabled_regmap,
- .get_voltage_sel = regulator_get_voltage_sel_regmap,
- .set_voltage_sel = regulator_set_voltage_sel_regmap,
- .list_voltage = regulator_list_voltage_linear,
- .set_mode = da9063_ldo_set_mode,
- .get_mode = da9063_ldo_get_mode,
- .get_status = da9063_ldo_get_status,
- .set_suspend_voltage = da9063_set_suspend_voltage,
- .set_suspend_enable = da9063_suspend_enable,
- .set_suspend_disable = da9063_suspend_disable,
- .set_suspend_mode = da9063_ldo_set_suspend_mode,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .list_voltage = regulator_list_voltage_linear,
+ .set_mode = da9063_ldo_set_mode,
+ .get_mode = da9063_ldo_get_mode,
+ .get_status = da9063_ldo_get_status,
+ .set_suspend_voltage = da9063_set_suspend_voltage,
+ .set_suspend_enable = da9063_suspend_enable,
+ .set_suspend_disable = da9063_suspend_disable,
+ .set_suspend_mode = da9063_ldo_set_suspend_mode,
+ .set_over_voltage_protection = da9063_set_xvp,
+ .set_under_voltage_protection = da9063_set_xvp,
};
/* Info of regulators for DA9063 */
@@ -749,6 +771,23 @@ static const struct regulator_init_data *da9063_get_regulator_initdata(
return NULL;
}
+static int da9063_check_xvp_constraints(struct regulator_config *config)
+{
+ struct da9063_regulator *regl = config->driver_data;
+ const struct regulation_constraints *constr = &config->init_data->constraints;
+ const struct notification_limit *uv_l = &constr->under_voltage_limits;
+ const struct notification_limit *ov_l = &constr->over_voltage_limits;
+
+ /* make sure that both UV/OV protections are either enabled or disabled */
+ if (uv_l->prot != ov_l->prot || uv_l->err != ov_l->err || uv_l->warn != ov_l->warn) {
+ dev_err(config->dev, "%s: regulator-uv-X-microvolt != regulator-ov-X-microvolt\n",
+ regl->desc.name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static struct of_regulator_match da9063_matches[] = {
[DA9063_ID_BCORE1] = { .name = "bcore1" },
[DA9063_ID_BCORE2] = { .name = "bcore2" },
@@ -970,6 +1009,11 @@ static int da9063_regulator_probe(struct platform_device *pdev)
if (da9063_reg_matches)
config.of_node = da9063_reg_matches[id].of_node;
config.regmap = da9063->regmap;
+
+ ret = da9063_check_xvp_constraints(&config);
+ if (ret)
+ return ret;
+
regl->rdev = devm_regulator_register(&pdev->dev, ®l->desc,
&config);
if (IS_ERR(regl->rdev)) {
--
2.34.1On Wed, Apr 05, 2023 at 07:29:08AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> Allow to en- and disable voltage monitoring from the device tree.
> Consider that the da9063 only monitors UV *and* OV together, so both
> must be en- or disabled.
I have no idea what a "basic XVP setter" is and this isn't super
enlightening. Is VP supposed to mean voltage protection or something?
> + /* make sure that both UV/OV protections are either enabled or disabled */
> + if (uv_l->prot != ov_l->prot || uv_l->err != ov_l->err || uv_l->warn != ov_l->warn) {
> + dev_err(config->dev, "%s: regulator-uv-X-microvolt != regulator-ov-X-microvolt\n",
> + regl->desc.name);
> + return -EINVAL;
I'm not sure that a user is going to figure out that this refers to the
protection levels, there's no hint as to what the X might be and the
error suggests that both the under and over voltage protection limits
must be have the same value, not just both be provided.
Thank you for the feedback! On Wed, 5 Apr 2023 at 12:52, Mark Brown <broonie@kernel.org> wrote: > I have no idea what a "basic XVP setter" is and this isn't super > enlightening. Is VP supposed to mean voltage protection or something? Yes, but basically this series handles just the monitoring part. The "protection part" is happening in hardware (at least on our board). So I will reword "XVP" to "voltage monitoring" in the next version. > I'm not sure that a user is going to figure out that this refers to the > protection levels, there's no hint as to what the X might be and the error > suggests that both the under and over voltage protection limits must be have > the same value, not just both be provided. I will split up the "catch-all" into an error per severity, like: "error-microvolt: value must be equal for uv and ov!" I will also ensure that there is only one severity set per regulator. Additionally, will also adapt the docu: if the voltage monitor should be changed, uv and ov must be set to the same severity and value.
On 4/5/23 08:29, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> Allow to en- and disable voltage monitoring from the device tree.
> Consider that the da9063 only monitors UV *and* OV together, so both
> must be en- or disabled.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> drivers/regulator/da9063-regulator.c | 100 +++++++++++++++++++++++++----------
> 1 file changed, 72 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
> index 1c720fc595b3..000fa0daef18 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
> @@ -207,6 +207,24 @@ static const unsigned int da9063_bmem_bio_merged_limits[] = {
> 4600000, 4800000, 5000000, 5200000, 5400000, 5600000, 5800000, 6000000
> };
>
> +static int da9063_set_xvp(struct regulator_dev *rdev, int lim_uV, int severity, bool enable)
> +{
> + struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> + struct device *dev = regl->hw->dev;
> +
> + dev_dbg(dev, "%s: lim: %d, sev: %d, en: %d\n", regl->desc.name, lim_uV, severity, enable);
> +
> + /*
> + * only support enable and disable.
> + * the da9063 offers a GPIO (GP_FB2) which is unasserted if an XV happens.
> + * therefore ignore severity here, as there might be handlers in hardware.
> + */
> + if (lim_uV)
> + return -EINVAL;
> +
> + return regmap_field_write(regl->vmon, enable ? 1 : 0);
> +}
> +
> static int da9063_buck_set_mode(struct regulator_dev *rdev, unsigned int mode)
> {
> struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> @@ -545,37 +563,41 @@ static int da9063_buck_get_current_limit(struct regulator_dev *rdev)
> }
>
> static const struct regulator_ops da9063_buck_ops = {
> - .enable = regulator_enable_regmap,
> - .disable = regulator_disable_regmap,
> - .is_enabled = regulator_is_enabled_regmap,
> - .get_voltage_sel = regulator_get_voltage_sel_regmap,
> - .set_voltage_sel = regulator_set_voltage_sel_regmap,
> - .list_voltage = regulator_list_voltage_linear,
> - .set_current_limit = da9063_buck_set_current_limit,
> - .get_current_limit = da9063_buck_get_current_limit,
> - .set_mode = da9063_buck_set_mode,
> - .get_mode = da9063_buck_get_mode,
> - .get_status = da9063_buck_get_status,
> - .set_suspend_voltage = da9063_set_suspend_voltage,
> - .set_suspend_enable = da9063_suspend_enable,
> - .set_suspend_disable = da9063_suspend_disable,
> - .set_suspend_mode = da9063_buck_set_suspend_mode,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .list_voltage = regulator_list_voltage_linear,
> + .set_current_limit = da9063_buck_set_current_limit,
> + .get_current_limit = da9063_buck_get_current_limit,
> + .set_mode = da9063_buck_set_mode,
> + .get_mode = da9063_buck_get_mode,
> + .get_status = da9063_buck_get_status,
> + .set_suspend_voltage = da9063_set_suspend_voltage,
> + .set_suspend_enable = da9063_suspend_enable,
> + .set_suspend_disable = da9063_suspend_disable,
> + .set_suspend_mode = da9063_buck_set_suspend_mode,
> + .set_over_voltage_protection = da9063_set_xvp,
> + .set_under_voltage_protection = da9063_set_xvp,
> };
>
> static const struct regulator_ops da9063_ldo_ops = {
> - .enable = regulator_enable_regmap,
> - .disable = regulator_disable_regmap,
> - .is_enabled = regulator_is_enabled_regmap,
> - .get_voltage_sel = regulator_get_voltage_sel_regmap,
> - .set_voltage_sel = regulator_set_voltage_sel_regmap,
> - .list_voltage = regulator_list_voltage_linear,
> - .set_mode = da9063_ldo_set_mode,
> - .get_mode = da9063_ldo_get_mode,
> - .get_status = da9063_ldo_get_status,
> - .set_suspend_voltage = da9063_set_suspend_voltage,
> - .set_suspend_enable = da9063_suspend_enable,
> - .set_suspend_disable = da9063_suspend_disable,
> - .set_suspend_mode = da9063_ldo_set_suspend_mode,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .list_voltage = regulator_list_voltage_linear,
> + .set_mode = da9063_ldo_set_mode,
> + .get_mode = da9063_ldo_get_mode,
> + .get_status = da9063_ldo_get_status,
> + .set_suspend_voltage = da9063_set_suspend_voltage,
> + .set_suspend_enable = da9063_suspend_enable,
> + .set_suspend_disable = da9063_suspend_disable,
> + .set_suspend_mode = da9063_ldo_set_suspend_mode,
> + .set_over_voltage_protection = da9063_set_xvp,
> + .set_under_voltage_protection = da9063_set_xvp,
> };
During my recent visit in the IIO territory I was told to by Jonathan to
drop the 'pretty indenting' of structs like this. I think this shows
well why - when longer members are added, it's hard to see from the diff
what actually changed. So, if you re-spin and unless Mark has another
opinion, maybe drop the tabs - in my eyes this does not do much for the
readability.
Well, IMO this is definitely not something that would require a re-spin
- and it may be others disagree with me on this. So, FWIW:
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
© 2016 - 2026 Red Hat, Inc.