Hi!
Follow-up for my last patch regarding the disabling of unrequired
voltage monitors. We use the PWR_OK functionality, which asserts GP_FB2
if every monitored voltage is in range. This patch should provide the
possibility to deactivate a voltage monitor from the DT if the regulator
might be disabled during run time. For this purpose, the regulator
notification support is used:
https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
v2:
- reworked solution, based on Adam Thomson's feedback
---
Benjamin Bara (3):
regulator: da9063: add voltage monitoring registers
regulator: da9063: implement basic XVP setter
dt-bindings: mfd: dlg,da9063: document XVP
.../devicetree/bindings/mfd/dlg,da9063.yaml | 16 ++-
drivers/regulator/da9063-regulator.c | 129 ++++++++++++++++-----
include/linux/mfd/da9063/registers.h | 23 ++++
3 files changed, 138 insertions(+), 30 deletions(-)
---
base-commit: 7e364e56293bb98cae1b55fd835f5991c4e96e7d
change-id: 20230403-da9063-disable-unused-15836e2f4539
Best regards,
--
Benjamin Bara <[email protected]>
From: Benjamin Bara <[email protected]>
Add the definitions for the registers responsible for voltage
monitoring. Add a voltage monitor enable bitfield per regulator.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/da9063-regulator.c | 29 +++++++++++++++++++++++++++++
include/linux/mfd/da9063/registers.h | 23 +++++++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 82f52a2a031a..1c720fc595b3 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -83,6 +83,9 @@ struct da9063_regulator_info {
/* DA9063 event detection bit */
struct reg_field oc_event;
+
+ /* DA9063 voltage monitor bit */
+ struct reg_field vmon;
};
/* Macros for LDO */
@@ -148,6 +151,7 @@ struct da9063_regulator {
struct regmap_field *suspend;
struct regmap_field *sleep;
struct regmap_field *suspend_sleep;
+ struct regmap_field *vmon;
};
/* Encapsulates all information for the regulators driver */
@@ -581,36 +585,42 @@ static const struct da9063_regulator_info da9063_regulator_info[] = {
da9063_buck_a_limits,
DA9063_REG_BUCK_ILIM_C, DA9063_BCORE1_ILIM_MASK),
DA9063_BUCK_COMMON_FIELDS(BCORE1),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BCORE1_MON_EN),
},
{
DA9063_BUCK(DA9063, BCORE2, 300, 10, 1570,
da9063_buck_a_limits,
DA9063_REG_BUCK_ILIM_C, DA9063_BCORE2_ILIM_MASK),
DA9063_BUCK_COMMON_FIELDS(BCORE2),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BCORE2_MON_EN),
},
{
DA9063_BUCK(DA9063, BPRO, 530, 10, 1800,
da9063_buck_a_limits,
DA9063_REG_BUCK_ILIM_B, DA9063_BPRO_ILIM_MASK),
DA9063_BUCK_COMMON_FIELDS(BPRO),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BPRO_MON_EN),
},
{
DA9063_BUCK(DA9063, BMEM, 800, 20, 3340,
da9063_buck_b_limits,
DA9063_REG_BUCK_ILIM_A, DA9063_BMEM_ILIM_MASK),
DA9063_BUCK_COMMON_FIELDS(BMEM),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BMEM_MON_EN),
},
{
DA9063_BUCK(DA9063, BIO, 800, 20, 3340,
da9063_buck_b_limits,
DA9063_REG_BUCK_ILIM_A, DA9063_BIO_ILIM_MASK),
DA9063_BUCK_COMMON_FIELDS(BIO),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BIO_MON_EN),
},
{
DA9063_BUCK(DA9063, BPERI, 800, 20, 3340,
da9063_buck_b_limits,
DA9063_REG_BUCK_ILIM_B, DA9063_BPERI_ILIM_MASK),
DA9063_BUCK_COMMON_FIELDS(BPERI),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BPERI_MON_EN),
},
{
DA9063_BUCK(DA9063, BCORES_MERGED, 300, 10, 1570,
@@ -618,6 +628,7 @@ static const struct da9063_regulator_info da9063_regulator_info[] = {
DA9063_REG_BUCK_ILIM_C, DA9063_BCORE1_ILIM_MASK),
/* BCORES_MERGED uses the same register fields as BCORE1 */
DA9063_BUCK_COMMON_FIELDS(BCORE1),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BCORE1_MON_EN),
},
{
DA9063_BUCK(DA9063, BMEM_BIO_MERGED, 800, 20, 3340,
@@ -625,47 +636,59 @@ static const struct da9063_regulator_info da9063_regulator_info[] = {
DA9063_REG_BUCK_ILIM_A, DA9063_BMEM_ILIM_MASK),
/* BMEM_BIO_MERGED uses the same register fields as BMEM */
DA9063_BUCK_COMMON_FIELDS(BMEM),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_4, DA9063_BMEM_MON_EN),
},
{
DA9063_LDO(DA9063, LDO3, 900, 20, 3440),
.oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO3_LIM),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO3_MON_EN),
},
{
DA9063_LDO(DA9063, LDO7, 900, 50, 3600),
.oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO7_LIM),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO7_MON_EN),
},
{
DA9063_LDO(DA9063, LDO8, 900, 50, 3600),
.oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO8_LIM),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO8_MON_EN),
},
{
DA9063_LDO(DA9063, LDO9, 950, 50, 3600),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_3, DA9063_LDO9_MON_EN),
},
{
DA9063_LDO(DA9063, LDO11, 900, 50, 3600),
.oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO11_LIM),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_3, DA9063_LDO11_MON_EN),
},
/* The following LDOs are present only on DA9063, not on DA9063L */
{
DA9063_LDO(DA9063, LDO1, 600, 20, 1860),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO1_MON_EN),
},
{
DA9063_LDO(DA9063, LDO2, 600, 20, 1860),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO2_MON_EN),
},
{
DA9063_LDO(DA9063, LDO4, 900, 20, 3440),
.oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO4_LIM),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO4_MON_EN),
},
{
DA9063_LDO(DA9063, LDO5, 900, 50, 3600),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO5_MON_EN),
},
{
DA9063_LDO(DA9063, LDO6, 900, 50, 3600),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_2, DA9063_LDO6_MON_EN),
},
{
DA9063_LDO(DA9063, LDO10, 900, 50, 3600),
+ .vmon = BFIELD(DA9063_BB_REG_MON_REG_3, DA9063_LDO10_MON_EN),
},
};
@@ -932,6 +955,12 @@ static int da9063_regulator_probe(struct platform_device *pdev)
if (IS_ERR(regl->suspend_sleep))
return PTR_ERR(regl->suspend_sleep);
}
+ if (regl->info->vmon.reg) {
+ regl->vmon = devm_regmap_field_alloc(&pdev->dev,
+ da9063->regmap, regl->info->vmon);
+ if (IS_ERR(regl->vmon))
+ return PTR_ERR(regl->vmon);
+ }
/* Register regulator */
memset(&config, 0, sizeof(config));
diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h
index 6e0f66a2e727..7b8364bd08a0 100644
--- a/include/linux/mfd/da9063/registers.h
+++ b/include/linux/mfd/da9063/registers.h
@@ -1040,6 +1040,29 @@
/* DA9063_REG_CONFIG_J (addr=0x10F) */
#define DA9063_TWOWIRE_TO 0x40
+/* DA9063_REG_MON_REG_2 (addr=0x115) */
+#define DA9063_LDO1_MON_EN 0x01
+#define DA9063_LDO2_MON_EN 0x02
+#define DA9063_LDO3_MON_EN 0x04
+#define DA9063_LDO4_MON_EN 0x08
+#define DA9063_LDO5_MON_EN 0x10
+#define DA9063_LDO6_MON_EN 0x20
+#define DA9063_LDO7_MON_EN 0x40
+#define DA9063_LDO8_MON_EN 0x80
+
+/* DA9063_REG_MON_REG_3 (addr=0x116) */
+#define DA9063_LDO9_MON_EN 0x01
+#define DA9063_LDO10_MON_EN 0x02
+#define DA9063_LDO11_MON_EN 0x04
+
+/* DA9063_REG_MON_REG_4 (addr=0x117) */
+#define DA9063_BCORE1_MON_EN 0x04
+#define DA9063_BCORE2_MON_EN 0x08
+#define DA9063_BPRO_MON_EN 0x10
+#define DA9063_BIO_MON_EN 0x20
+#define DA9063_BMEM_MON_EN 0x40
+#define DA9063_BPERI_MON_EN 0x80
+
/* DA9063_REG_MON_REG_5 (addr=0x116) */
#define DA9063_MON_A8_IDX_MASK 0x07
#define DA9063_MON_A8_IDX_NONE 0x00
--
2.34.1
From: Benjamin Bara <[email protected]>
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 <[email protected]>
---
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.1
From: Benjamin Bara <[email protected]>
Document that the da9063 only provides UVP *and* OVP in one, and
therefore requires both configured. Add an example for clarification.
Signed-off-by: Benjamin Bara <[email protected]>
---
Documentation/devicetree/bindings/mfd/dlg,da9063.yaml | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml b/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
index e8e74e91070c..e9d5ab418dd2 100644
--- a/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
+++ b/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
@@ -12,6 +12,10 @@ maintainers:
description: |
For device-tree bindings of other sub-modules refer to the binding documents
under the respective sub-system directories.
+ Using regulator-uv-X-microvolt and regulator-ov-X-microvolt requires special
+ handling: First, when GP_FB2 is used, it must be ensured that there is no
+ moment where all voltage monitors are disabled. Next, as da9063 only supports
+ UV *and* OV monitoring, both must be set.
properties:
compatible:
@@ -121,11 +125,19 @@ examples:
regulator-max-microamp = <2000000>;
regulator-boot-on;
};
+ ldo6 {
+ /* UNUSED */
+ regulator-name = "LDO_6";
+ regulator-uv-protection-microvolt = <0>;
+ regulator-ov-protection-microvolt = <0>;
+ };
ldo11 {
regulator-name = "LDO_11";
regulator-min-microvolt = <900000>;
- regulator-max-microvolt = <3600000>;
- regulator-boot-on;
+ regulator-max-microvolt = <900000>;
+ regulator-uv-protection-microvolt = <1>;
+ regulator-ov-protection-microvolt = <1>;
+ regulator-always-on;
};
};
};
--
2.34.1
On 05/04/2023 07:29, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> Document that the da9063 only provides UVP *and* OVP in one, and
> therefore requires both configured. Add an example for clarification.
>
> Signed-off-by: Benjamin Bara <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hi Benjamin,
On 4/5/23 08:29, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> Add the definitions for the registers responsible for voltage
> monitoring. Add a voltage monitor enable bitfield per regulator.
>
> Signed-off-by: Benjamin Bara <[email protected]>
> ---
> drivers/regulator/da9063-regulator.c | 29 +++++++++++++++++++++++++++++
> include/linux/mfd/da9063/registers.h | 23 +++++++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
> index 82f52a2a031a..1c720fc595b3 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
...
>
> @@ -932,6 +955,12 @@ static int da9063_regulator_probe(struct platform_device *pdev)
> if (IS_ERR(regl->suspend_sleep))
> return PTR_ERR(regl->suspend_sleep);
> }
> + if (regl->info->vmon.reg) {
Just a very minor thing - wouldn't this check be better as:
if (regl->info->vmon.mask) ?
We may have device(s) where 0 is a valid reg. However, mask 0 is
probably not making sense - unless I misunderstand something?
Well, I guess the reg 0 is not valid for vmon on currently supported
ICs, and it probably is unlikely that would happen on a future device
either. Still, treating register 0 as 'not initialized' always feels a
tad bad for me if it can be avoided. So, perhaps consider this if you
re-spin for some other reason - but I don't think this is by any means
crucial.
FWIW:
Reviewed-by: Matti Vaittinen <[email protected]>
> + regl->vmon = devm_regmap_field_alloc(&pdev->dev,
> + da9063->regmap, regl->info->vmon);
> + if (IS_ERR(regl->vmon))
> + return PTR_ERR(regl->vmon);
> + }
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 4/5/23 08:29, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> Document that the da9063 only provides UVP *and* OVP in one, and
> therefore requires both configured. Add an example for clarification.
>
> Signed-off-by: Benjamin Bara <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/dlg,da9063.yaml | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 4/5/23 08:29, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Thank you for the feedback!
On Wed, 5 Apr 2023 at 09:29, Matti Vaittinen <[email protected]> wrote:
> Just a very minor thing - wouldn't this check be better as:
> if (regl->info->vmon.mask) ?
> We may have device(s) where 0 is a valid reg. However, mask 0 is
> probably not making sense - unless I misunderstand something?
This config is specific to the da9063. On this IC, register 0 is used for
PAGE_CON (control register). The registers relevant for voltage monitoring are
on 0x115-0x117. So IMHO this should be fine.
On Wed, Apr 05, 2023 at 07:29:08AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> 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 <[email protected]> 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 Wed, 05 Apr 2023, Benjamin Bara wrote:
> Hi!
>
> Follow-up for my last patch regarding the disabling of unrequired
> voltage monitors. We use the PWR_OK functionality, which asserts GP_FB2
> if every monitored voltage is in range. This patch should provide the
> possibility to deactivate a voltage monitor from the DT if the regulator
> might be disabled during run time. For this purpose, the regulator
> notification support is used:
> https://lore.kernel.org/all/[email protected]/
>
> v1: https://lore.kernel.org/all/[email protected]/
>
> v2:
> - reworked solution, based on Adam Thomson's feedback
>
> ---
> Benjamin Bara (3):
> regulator: da9063: add voltage monitoring registers
> regulator: da9063: implement basic XVP setter
> dt-bindings: mfd: dlg,da9063: document XVP
>
> .../devicetree/bindings/mfd/dlg,da9063.yaml | 16 ++-
> drivers/regulator/da9063-regulator.c | 129 ++++++++++++++++-----
> include/linux/mfd/da9063/registers.h | 23 ++++
> 3 files changed, 138 insertions(+), 30 deletions(-)
I'll handle this set once Mark is happy.
> ---
> base-commit: 7e364e56293bb98cae1b55fd835f5991c4e96e7d
> change-id: 20230403-da9063-disable-unused-15836e2f4539
>
> Best regards,
> --
> Benjamin Bara <[email protected]>
>
--
Lee Jones [李琼斯]