[PATCH v2 0/3] regulator: da9063: disable unused voltage monitors

[PATCH v2 0/3] regulator: da9063: disable unused voltage monitors

[Benjamin Bara]

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/cover.1622628333.git.matti.vaittinen@fi.rohmeurope.com/

v1: https://lore.kernel.org/all/20220713124958.3094505-1-bbara93@gmail.com/

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 <benjamin.bara@skidata.com>

[PATCH v2 1/3] regulator: da9063: add voltage monitoring registers

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Add the definitions for the registers responsible for voltage
monitoring. Add a voltage monitor enable bitfield per regulator.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 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

Re: [PATCH v2 1/3] regulator: da9063: add voltage monitoring registers

[Matti Vaittinen]

Hi Benjamin,

On 4/5/23 08:29, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Add the definitions for the registers responsible for voltage
> monitoring. Add a voltage monitor enable bitfield per regulator.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>   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 <mazziesaccount@gmail.com>

> +			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! ~~

Re: [PATCH v2 1/3] regulator: da9063: add voltage monitoring registers

[Benjamin Bara]

Thank you for the feedback!

On Wed, 5 Apr 2023 at 09:29, Matti Vaittinen <mazziesaccount@gmail.com> 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.

[PATCH v2 2/3] regulator: da9063: implement basic XVP setter

[Benjamin Bara]

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, &regl->desc,
 						     &config);
 		if (IS_ERR(regl->rdev)) {

-- 
2.34.1

Re: [PATCH v2 2/3] regulator: da9063: implement basic XVP setter

[Matti Vaittinen]

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! ~~

Re: [PATCH v2 2/3] regulator: da9063: implement basic XVP setter

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

On 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] regulator: da9063: implement basic XVP setter

[Benjamin Bara]

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.

[PATCH v2 3/3] dt-bindings: mfd: dlg,da9063: document XVP

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

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 <benjamin.bara@skidata.com>
---
 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

Re: [PATCH v2 3/3] dt-bindings: mfd: dlg,da9063: document XVP

[Krzysztof Kozlowski]

On 05/04/2023 07:29, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> 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 <benjamin.bara@skidata.com>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: mfd: dlg,da9063: document XVP

[Matti Vaittinen]

On 4/5/23 08:29, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> 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 <benjamin.bara@skidata.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   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! ~~

Re: [PATCH v2 0/3] regulator: da9063: disable unused voltage monitors

[Lee Jones]

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/cover.1622628333.git.matti.vaittinen@fi.rohmeurope.com/
>
> v1: https://lore.kernel.org/all/20220713124958.3094505-1-bbara93@gmail.com/
>
> 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 <benjamin.bara@skidata.com>
>

--
Lee Jones [李琼斯]