[PATCH v3 0/6] Fix rtq2208 BUCK ramp_delay and LDO dvs setting

[PATCH v3 0/6] Fix rtq2208 BUCK ramp_delay and LDO dvs setting

[Alina Yu]

Hi,

This series patches is for hardware modification of RTQ2208.
ramp_delay range of BUCK is changed.
The maximum ramp up and down range of BUCK are shorten
 from 64mVstep/us to 16mVstep/us.
The LDO's Vout is adjustable if the hardware setting allow it,
and it can be set either 1800mv or 3300mv.
Additionally, the discharge register has been moved to another position.
In this version, a software bug has been fixed.
rtq2208_ldo_match is no longer a local variable.

Thanks,
Alina yu
---
Change in v3:
- In Patch 1/2:
	- As discussing in v2 series, 
	  add 'richtek,fixed-microvolt' property to specify the fixed voltage
- In Patch 2/2:
	- Move Fixes to the start of the series
	- Seperate LDO vsel and discharge change to seperate patches
	- Add "richtek,fixed-microvolt" to specify LDO fixed voltage
	- Check specified desc->fixed_uV matches with constraints->min_uV and constraints->max_uV
---
Alina Yu (6):
  regulator: rtq2208: Fix invalid memory access when
    devm_of_regulator_put_matches is called
  regulator: rtq2208: Fix LDO vsel setting
  regulator: rtq2208: Fix LDO discharge register
  regulator: rtq2208: Fix the BUCK ramp_delay range to maximum of
    16mVstep/us
  regulator: rtq2208: Fix LDO to be compatible with both fixed and
    adjustable vout
  regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is
    adjustable or not

 .../bindings/regulator/richtek,rtq2208.yaml        |   8 ++
 drivers/regulator/rtq2208-regulator.c              | 151 ++++++++++++++-------
 2 files changed, 110 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH v3 1/6] regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called

[Alina Yu]

In this patch, a software bug has been fixed.
rtq2208_ldo_match is no longer a local variable.
It prevents invalid memory access when devm_of_regulator_put_matches
 is called.

Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
v3
- Move Fixes to the start of the series
---
 drivers/regulator/rtq2208-regulator.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 2d54844..dfa293a 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -224,6 +224,11 @@ static const struct regulator_ops rtq2208_regulator_ldo_ops = {
 	.set_suspend_disable = rtq2208_set_suspend_disable,
 };
 
+static struct of_regulator_match rtq2208_ldo_match[] = {
+	{.name = "ldo2", },
+	{.name = "ldo1", },
+};
+
 static unsigned int rtq2208_of_map_mode(unsigned int mode)
 {
 	switch (mode) {
@@ -335,8 +340,7 @@ static const struct linear_range rtq2208_vout_range[] = {
 	REGULATOR_LINEAR_RANGE(1310000, 181, 255, 10000),
 };
 
-static int rtq2208_of_get_fixed_voltage(struct device *dev,
-					struct of_regulator_match *rtq2208_ldo_match, int n_fixed)
+static int rtq2208_of_get_ldo_dvs_ability(struct device *dev)
 {
 	struct device_node *np;
 	struct of_regulator_match *match;
@@ -351,14 +355,14 @@ static int rtq2208_of_get_fixed_voltage(struct device *dev,
 	if (!np)
 		np = dev->of_node;
 
-	ret = of_regulator_match(dev, np, rtq2208_ldo_match, n_fixed);
+	ret = of_regulator_match(dev, np, rtq2208_ldo_match, ARRAY_SIZE(rtq2208_ldo_match));
 
 	of_node_put(np);
 
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < n_fixed; i++) {
+	for (i = 0; i < ARRAY_SIZE(rtq2208_ldo_match); i++) {
 		match = rtq2208_ldo_match + i;
 		init_data = match->init_data;
 		rdesc = (struct rtq2208_regulator_desc *)match->driver_data;
@@ -373,8 +377,7 @@ static int rtq2208_of_get_fixed_voltage(struct device *dev,
 	return 0;
 }
 
-static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, int mtp_sel,
-					int idx, struct of_regulator_match *rtq2208_ldo_match, int *ldo_idx)
+static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, int mtp_sel, int idx)
 {
 	struct regulator_desc *desc;
 	static const struct {
@@ -432,10 +435,6 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 		desc->n_voltages = 1;
 		desc->active_discharge_reg = LDO_RG_SHIFT(curr_info->base, 2);
 
-		rtq2208_ldo_match[*ldo_idx].name = desc->name;
-		rtq2208_ldo_match[*ldo_idx].driver_data = rdesc;
-		rtq2208_ldo_match[(*ldo_idx)++].desc = desc;
-
 		rdesc->suspend_config_reg = curr_info->base;
 		rdesc->suspend_enable_mask = RTQ2208_LDO_EN_STR_MASK;
 	}
@@ -444,8 +443,7 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *regulator_idx_table,
 		struct rtq2208_regulator_desc *rdesc[RTQ2208_LDO_MAX], struct device *dev)
 {
-	struct of_regulator_match rtq2208_ldo_match[2];
-	int mtp_sel, ret, i, idx, ldo_idx = 0;
+	int mtp_sel, i, idx, ret;
 
 	/* get mtp_sel0 or mtp_sel1 */
 	mtp_sel = device_property_read_bool(dev, "richtek,mtp-sel-high");
@@ -457,11 +455,15 @@ static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *
 		if (!rdesc[i])
 			return -ENOMEM;
 
-		rtq2208_init_regulator_desc(rdesc[i], mtp_sel, idx, rtq2208_ldo_match, &ldo_idx);
+		rtq2208_init_regulator_desc(rdesc[i], mtp_sel, idx);
+
+		/* init ldo dvs ability */
+		if (idx >= RTQ2208_LDO2)
+			rtq2208_ldo_match[idx - RTQ2208_LDO2].desc = &rdesc[i]->desc;
 	}
 
 	/* init ldo fixed_uV */
-	ret = rtq2208_of_get_fixed_voltage(dev, rtq2208_ldo_match, ldo_idx);
+	ret = rtq2208_of_get_ldo_dvs_ability(dev);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to get ldo fixed_uV\n");
 
-- 
2.7.4

Re: [PATCH v3 1/6] regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called

[Mark Brown]

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

On Fri, May 10, 2024 at 08:06:20PM +0800, Alina Yu wrote:
> In this patch, a software bug has been fixed.
> rtq2208_ldo_match is no longer a local variable.
> It prevents invalid memory access when devm_of_regulator_put_matches
>  is called.

This doesn't apply against current code, please check and resend (on
Linus' tree rather than mine at this point given the merge window).

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

Re: [PATCH v3 1/6] regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called

[Alina Yu]

On Wed, May 15, 2024 at 04:47:37PM +0100, Mark Brown wrote:
> On Fri, May 10, 2024 at 08:06:20PM +0800, Alina Yu wrote:
> > In this patch, a software bug has been fixed.
> > rtq2208_ldo_match is no longer a local variable.
> > It prevents invalid memory access when devm_of_regulator_put_matches
> >  is called.
> 
> This doesn't apply against current code, please check and resend (on
> Linus' tree rather than mine at this point given the merge window).

Hi,
Mark

I apologize for the interruption.

I've resubmitted the new series for review at the following link: 
https://lore.kernel.org/all/7c28d2e61d2fc13066ba4814d1ecfab8f344aaad.1715846612.git.alina_yu@richtek.com/

This series is based on the previous submission found at:
https://lore.kernel.org/all/5d56b79c94de63fc86b5a70b7e374da4240fee8b.1714467553.git.alina_yu@richtek.com/

I would greatly appreciate it if you could let me know if this will be merged in the future branch,
or if there are any mistakes that I need to address.


Thanks,
Alina

[PATCH v3 2/6] regulator: rtq2208: Fix LDO vsel setting

[Alina Yu]

The LDO's Vout is adjustable if the hardware setting allows it,
 and it can be set either 1800mv or 3300mv.

Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
v3
- Seperate LDO vsel and discharge change to seperate patches
---
 drivers/regulator/rtq2208-regulator.c | 56 ++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index dfa293a..00da787 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -26,6 +26,7 @@
 #define RTQ2208_REG_BUCK_H_CFG0			0xA2
 #define RTQ2208_REG_LDO1_CFG			0xB1
 #define RTQ2208_REG_LDO2_CFG			0xC1
+#define RTQ2208_REG_LDO_DVS_CTRL		0xD0
 
 /* Mask */
 #define RTQ2208_BUCK_NR_MTP_SEL_MASK		GENMASK(7, 0)
@@ -40,6 +41,8 @@
 #define RTQ2208_EN_DIS_MASK			BIT(0)
 #define RTQ2208_BUCK_RAMP_SEL_MASK		GENMASK(2, 0)
 #define RTQ2208_HD_INT_MASK			BIT(0)
+#define RTQ2208_LDO1_VOSEL_SD_MASK		BIT(5)
+#define RTQ2208_LDO2_VOSEL_SD_MASK		BIT(7)
 
 /* Size */
 #define RTQ2208_VOUT_MAXNUM			256
@@ -323,14 +326,23 @@ static irqreturn_t rtq2208_irq_handler(int irqno, void *devid)
 	return IRQ_HANDLED;
 }
 
-#define RTQ2208_REGULATOR_INFO(_name, _base) \
-{ \
-	.name = #_name, \
-	.base = _base, \
+#define BUCK_INFO(_name, _id)						\
+{									\
+	.name = _name,							\
+	.base = RTQ2208_REG_BUCK_##_id##_CFG0,				\
+	.enable_reg = BUCK_RG_SHIFT(RTQ2208_REG_BUCK_##_id##_CFG0, 2),	\
+	.dis_reg = RTQ2208_REG_BUCK_##_id##_CFG0,			\
 }
-#define BUCK_RG_BASE(_id)	RTQ2208_REG_BUCK_##_id##_CFG0
+
+#define LDO_INFO(_name, _id)						\
+{									\
+	.name = _name,							\
+	.base = RTQ2208_REG_LDO##_id##_CFG,				\
+	.enable_reg = RTQ2208_REG_LDO##_id##_CFG,			\
+	.vsel_mask = RTQ2208_LDO##_id##_VOSEL_SD_MASK,			\
+}
+
 #define BUCK_RG_SHIFT(_base, _shift)	(_base + _shift)
-#define LDO_RG_BASE(_id)	RTQ2208_REG_LDO##_id##_CFG
 #define LDO_RG_SHIFT(_base, _shift)	(_base + _shift)
 #define	VSEL_SHIFT(_sel)	(_sel ? 3 : 1)
 #define MTP_SEL_MASK(_sel)	RTQ2208_BUCK_EN_NR_MTP_SEL##_sel##_MASK
@@ -383,17 +395,22 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 	static const struct {
 		char *name;
 		int base;
+		int enable_reg;
+		int dis_reg;
+		int dis_mask;
+		int dis_on;
+		int vsel_mask;
 	} regulator_info[] = {
-		RTQ2208_REGULATOR_INFO(buck-b, BUCK_RG_BASE(B)),
-		RTQ2208_REGULATOR_INFO(buck-c, BUCK_RG_BASE(C)),
-		RTQ2208_REGULATOR_INFO(buck-d, BUCK_RG_BASE(D)),
-		RTQ2208_REGULATOR_INFO(buck-a, BUCK_RG_BASE(A)),
-		RTQ2208_REGULATOR_INFO(buck-f, BUCK_RG_BASE(F)),
-		RTQ2208_REGULATOR_INFO(buck-g, BUCK_RG_BASE(G)),
-		RTQ2208_REGULATOR_INFO(buck-h, BUCK_RG_BASE(H)),
-		RTQ2208_REGULATOR_INFO(buck-e, BUCK_RG_BASE(E)),
-		RTQ2208_REGULATOR_INFO(ldo2, LDO_RG_BASE(2)),
-		RTQ2208_REGULATOR_INFO(ldo1, LDO_RG_BASE(1)),
+		BUCK_INFO("buck-b", B),
+		BUCK_INFO("buck-c", C),
+		BUCK_INFO("buck-d", D),
+		BUCK_INFO("buck-a", A),
+		BUCK_INFO("buck-f", F),
+		BUCK_INFO("buck-g", G),
+		BUCK_INFO("buck-h", H),
+		BUCK_INFO("buck-e", E),
+		LDO_INFO("ldo2", 2),
+		LDO_INFO("ldo1", 1),
 	}, *curr_info;
 
 	curr_info = regulator_info + idx;
@@ -408,12 +425,12 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 	desc->active_discharge_on = RTQ2208_EN_DIS_MASK;
 	desc->active_discharge_off = 0;
 	desc->active_discharge_mask = RTQ2208_EN_DIS_MASK;
+	desc->enable_reg = curr_info->enable_reg;
 
 	rdesc->mode_mask = RTQ2208_BUCK_NRMODE_MASK;
 
 	if (idx >= RTQ2208_BUCK_B && idx <= RTQ2208_BUCK_E) {
 		/* init buck desc */
-		desc->enable_reg = BUCK_RG_SHIFT(curr_info->base, 2);
 		desc->ops = &rtq2208_regulator_buck_ops;
 		desc->vsel_reg = curr_info->base + VSEL_SHIFT(mtp_sel);
 		desc->vsel_mask = RTQ2208_BUCK_NR_MTP_SEL_MASK;
@@ -430,10 +447,9 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 		rdesc->suspend_mode_mask = RTQ2208_BUCK_STRMODE_MASK;
 	} else {
 		/* init ldo desc */
-		desc->enable_reg = curr_info->base;
-		desc->ops = &rtq2208_regulator_ldo_ops;
-		desc->n_voltages = 1;
 		desc->active_discharge_reg = LDO_RG_SHIFT(curr_info->base, 2);
+		desc->vsel_reg = RTQ2208_REG_LDO_DVS_CTRL;
+		desc->vsel_mask = curr_info->vsel_mask;
 
 		rdesc->suspend_config_reg = curr_info->base;
 		rdesc->suspend_enable_mask = RTQ2208_LDO_EN_STR_MASK;
-- 
2.7.4

[PATCH v3 3/6] regulator: rtq2208: Fix LDO discharge register

[Alina Yu]

Since the discharge register has been moved to another position,
the modification is to fit the new register setting.

Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
v3
- Seperate LDO vsel and discharge change to seperate patches
---
 drivers/regulator/rtq2208-regulator.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 00da787..cea8e77 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -41,7 +41,9 @@
 #define RTQ2208_EN_DIS_MASK			BIT(0)
 #define RTQ2208_BUCK_RAMP_SEL_MASK		GENMASK(2, 0)
 #define RTQ2208_HD_INT_MASK			BIT(0)
+#define RTQ2208_LDO1_DISCHG_EN_MASK		BIT(4)
 #define RTQ2208_LDO1_VOSEL_SD_MASK		BIT(5)
+#define RTQ2208_LDO2_DISCHG_EN_MASK		BIT(6)
 #define RTQ2208_LDO2_VOSEL_SD_MASK		BIT(7)
 
 /* Size */
@@ -339,11 +341,12 @@ static irqreturn_t rtq2208_irq_handler(int irqno, void *devid)
 	.name = _name,							\
 	.base = RTQ2208_REG_LDO##_id##_CFG,				\
 	.enable_reg = RTQ2208_REG_LDO##_id##_CFG,			\
+	.dis_mask = RTQ2208_LDO##_id##_DISCHG_EN_MASK,			\
+	.dis_on = RTQ2208_LDO##_id##_DISCHG_EN_MASK,			\
 	.vsel_mask = RTQ2208_LDO##_id##_VOSEL_SD_MASK,			\
 }
 
 #define BUCK_RG_SHIFT(_base, _shift)	(_base + _shift)
-#define LDO_RG_SHIFT(_base, _shift)	(_base + _shift)
 #define	VSEL_SHIFT(_sel)	(_sel ? 3 : 1)
 #define MTP_SEL_MASK(_sel)	RTQ2208_BUCK_EN_NR_MTP_SEL##_sel##_MASK
 
@@ -422,9 +425,7 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 	desc->owner = THIS_MODULE;
 	desc->type = REGULATOR_VOLTAGE;
 	desc->enable_mask = mtp_sel ? MTP_SEL_MASK(1) : MTP_SEL_MASK(0);
-	desc->active_discharge_on = RTQ2208_EN_DIS_MASK;
 	desc->active_discharge_off = 0;
-	desc->active_discharge_mask = RTQ2208_EN_DIS_MASK;
 	desc->enable_reg = curr_info->enable_reg;
 
 	rdesc->mode_mask = RTQ2208_BUCK_NRMODE_MASK;
@@ -438,8 +439,10 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 		desc->linear_ranges = rtq2208_vout_range;
 		desc->n_linear_ranges = ARRAY_SIZE(rtq2208_vout_range);
 		desc->ramp_reg = BUCK_RG_SHIFT(curr_info->base, 5);
-		desc->active_discharge_reg = curr_info->base;
 		desc->of_map_mode = rtq2208_of_map_mode;
+		desc->active_discharge_reg = curr_info->dis_reg;
+		desc->active_discharge_on = RTQ2208_EN_DIS_MASK;
+		desc->active_discharge_mask = RTQ2208_EN_DIS_MASK;
 
 		rdesc->mode_reg = BUCK_RG_SHIFT(curr_info->base, 2);
 		rdesc->suspend_config_reg = BUCK_RG_SHIFT(curr_info->base, 4);
@@ -447,9 +450,11 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
 		rdesc->suspend_mode_mask = RTQ2208_BUCK_STRMODE_MASK;
 	} else {
 		/* init ldo desc */
-		desc->active_discharge_reg = LDO_RG_SHIFT(curr_info->base, 2);
 		desc->vsel_reg = RTQ2208_REG_LDO_DVS_CTRL;
 		desc->vsel_mask = curr_info->vsel_mask;
+		desc->active_discharge_reg = RTQ2208_REG_LDO_DVS_CTRL;
+		desc->active_discharge_on = curr_info->dis_on;
+		desc->active_discharge_mask = curr_info->dis_mask;
 
 		rdesc->suspend_config_reg = curr_info->base;
 		rdesc->suspend_enable_mask = RTQ2208_LDO_EN_STR_MASK;
-- 
2.7.4

[PATCH v3 4/6] regulator: rtq2208: Fix the BUCK ramp_delay range to maximum of 16mVstep/us

[Alina Yu]

The maximum ramp up and down range of BUCK are shorten
 from 64mVstep/us to 16mVstep/us.
Therefore, the RTQ2208_RAMP_VALUE_MAX_uV is modified
 to 16000uV in this version.

Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
 drivers/regulator/rtq2208-regulator.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index cea8e77..2e9387f 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -53,7 +53,7 @@
 
 /* Value */
 #define RTQ2208_RAMP_VALUE_MIN_uV		500
-#define RTQ2208_RAMP_VALUE_MAX_uV		64000
+#define RTQ2208_RAMP_VALUE_MAX_uV		16000
 
 #define RTQ2208_BUCK_MASK(uv_irq, ov_irq)	(1 << ((uv_irq) % 8) | 1 << ((ov_irq) % 8))
 
@@ -147,12 +147,11 @@ static int rtq2208_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 	 * Because the relation of seleltion and value is like that
 	 *
 	 * seletion: value
-	 * 000: 64mv
-	 * 001: 32mv
+	 * 010: 16mv
 	 * ...
 	 * 111: 0.5mv
 	 *
-	 * For example, if I would like to select 64mv, the fls(ramp_delay) - 1 will be 0b111,
+	 * For example, if I would like to select 16mv, the fls(ramp_delay) - 1 will be 0b010,
 	 * and I need to use 0b111 - sel to do the shifting
 	 */
 
-- 
2.7.4

[PATCH v3 5/6] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

[Alina Yu]

In this patch, LDO's adjustable and fixed Vout settings are compatible.
The LDO Vout ability depends on "richtek,fixed-microvolt".
If adjustable, the Vout can be set to either 1800mV or 3300mV.

Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
v3
- Add "richtek,fixed-microvolt" to specify LDO fixed voltage
- Check specified desc->fixed_uV matches with constraints->min_uV and constraints->max_uV
---
 drivers/regulator/rtq2208-regulator.c | 43 ++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 2e9387f..c2c1689 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -219,7 +219,7 @@ static const struct regulator_ops rtq2208_regulator_buck_ops = {
 	.set_suspend_mode = rtq2208_set_suspend_mode,
 };
 
-static const struct regulator_ops rtq2208_regulator_ldo_ops = {
+static const struct regulator_ops rtq2208_regulator_ldo_fix_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
@@ -228,6 +228,23 @@ static const struct regulator_ops rtq2208_regulator_ldo_ops = {
 	.set_suspend_disable = rtq2208_set_suspend_disable,
 };
 
+static const struct regulator_ops rtq2208_regulator_ldo_adj_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_table,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_active_discharge = regulator_set_active_discharge_regmap,
+	.set_suspend_enable = rtq2208_set_suspend_enable,
+	.set_suspend_disable = rtq2208_set_suspend_disable,
+};
+
+static const unsigned int rtq2208_ldo_volt_table[] = {
+	1800000,
+	3300000,
+};
+
 static struct of_regulator_match rtq2208_ldo_match[] = {
 	{.name = "ldo2", },
 	{.name = "ldo1", },
@@ -358,8 +375,9 @@ static int rtq2208_of_get_ldo_dvs_ability(struct device *dev)
 {
 	struct device_node *np;
 	struct of_regulator_match *match;
-	struct rtq2208_regulator_desc *rdesc;
+	struct regulator_desc *desc;
 	struct regulator_init_data *init_data;
+	u32 fixed_uV;
 	int ret, i;
 
 	if (!dev->of_node)
@@ -379,13 +397,26 @@ static int rtq2208_of_get_ldo_dvs_ability(struct device *dev)
 	for (i = 0; i < ARRAY_SIZE(rtq2208_ldo_match); i++) {
 		match = rtq2208_ldo_match + i;
 		init_data = match->init_data;
-		rdesc = (struct rtq2208_regulator_desc *)match->driver_data;
+		desc = (struct regulator_desc *)match->desc;
 
-		if (!init_data || !rdesc)
+		if (!init_data || !desc)
 			continue;
 
-		if (init_data->constraints.min_uV == init_data->constraints.max_uV)
-			rdesc->desc.fixed_uV = init_data->constraints.min_uV;
+		/* specify working fixed voltage if the propery exists */
+		ret = of_property_read_u32(match->of_node, "richtek,fixed-microvolt", &fixed_uV);
+
+		if (!ret) {
+			if (fixed_uV != init_data->constraints.min_uV ||
+				fixed_uV != init_data->constraints.max_uV)
+				return -EINVAL;
+			desc->n_voltages = 1;
+			desc->fixed_uV = fixed_uV;
+			desc->ops = &rtq2208_regulator_ldo_fix_ops;
+		} else {
+			desc->n_voltages = ARRAY_SIZE(rtq2208_ldo_volt_table);
+			desc->volt_table = rtq2208_ldo_volt_table;
+			desc->ops = &rtq2208_regulator_ldo_adj_ops;
+		}
 	}
 
 	return 0;
-- 
2.7.4

[PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Alina Yu]

Since there is no way to check is ldo is adjustable or not.
As discussing in v2 series, 'richtek,fixed-microvolt' is added for that.
user is supposed to know whether vout of ldo is adjustable.

Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
 Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
index 609c066..6212f44 100644
--- a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
+++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
@@ -75,6 +75,13 @@ properties:
         description:
           regulator description for ldo[1-2].
 
+        properties:
+          richtek,fixed-microvolt:
+            description: |
+              If it exists, the voltage is unadjustable.
+              There is no risk-free method for software to determine whether the ldo vout is fixed or not.
+              Therefore, it can only be done in this way.
+
 required:
   - compatible
   - reg
@@ -177,6 +184,7 @@ examples:
             };
           };
           ldo1 {
+            richtek,fixed-microvolt = <1200000>;
             regulator-min-microvolt = <1200000>;
             regulator-max-microvolt = <1200000>;
             regulator-always-on;
-- 
2.7.4

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Conor Dooley]

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

On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote:
> Since there is no way to check is ldo is adjustable or not.
> As discussing in v2 series, 'richtek,fixed-microvolt' is added for that.
> user is supposed to know whether vout of ldo is adjustable.
> 
> Signed-off-by: Alina Yu <alina_yu@richtek.com>
> ---
>  Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> index 609c066..6212f44 100644
> --- a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> @@ -75,6 +75,13 @@ properties:
>          description:
>            regulator description for ldo[1-2].
>  
> +        properties:
> +          richtek,fixed-microvolt:
> +            description: |
> +              If it exists, the voltage is unadjustable.
> +              There is no risk-free method for software to determine whether the ldo vout is fixed or not.
> +              Therefore, it can only be done in this way.
> +
>  required:
>    - compatible
>    - reg
> @@ -177,6 +184,7 @@ examples:
>              };
>            };
>            ldo1 {

> +            richtek,fixed-microvolt = <1200000>;
>              regulator-min-microvolt = <1200000>;
>              regulator-max-microvolt = <1200000>;

I'm dumb and this example seemed odd to me. Can you explain to me why
it is not sufficient to set min-microvolt == max-microvolt to achieve
the same thing?

Cheers,
Conor.

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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Mark Brown]

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

On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote:
> On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote:

> > +            richtek,fixed-microvolt = <1200000>;
> >              regulator-min-microvolt = <1200000>;
> >              regulator-max-microvolt = <1200000>;

> I'm dumb and this example seemed odd to me. Can you explain to me why
> it is not sufficient to set min-microvolt == max-microvolt to achieve
> the same thing?

This is for a special mode where the voltage being configured is out of
the range usually supported by the regulator, requiring a hardware
design change to achieve.  The separate property is because otherwise we
can't distinguish the case where the mode is in use from the case where
the constraints are nonsense, and we need to handle setting a fixed
voltage on a configurable regulator differently to there being a
hardware fixed voltage on a normally configurable regulator.

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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Conor Dooley]

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

On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote:
> On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote:
> > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote:
> 
> > > +            richtek,fixed-microvolt = <1200000>;
> > >              regulator-min-microvolt = <1200000>;
> > >              regulator-max-microvolt = <1200000>;
> 
> > I'm dumb and this example seemed odd to me. Can you explain to me why
> > it is not sufficient to set min-microvolt == max-microvolt to achieve
> > the same thing?
> 
> This is for a special mode where the voltage being configured is out of
> the range usually supported by the regulator, requiring a hardware
> design change to achieve.  The separate property is because otherwise we
> can't distinguish the case where the mode is in use from the case where
> the constraints are nonsense, and we need to handle setting a fixed
> voltage on a configurable regulator differently to there being a
> hardware fixed voltage on a normally configurable regulator.

Cool, I think an improved comment message and description would be
helpful then to describe the desired behaviour that you mention here.
The commit message in particular isn't great:
| Since there is no way to check is ldo is adjustable or not.
| As discussing in v2 series, 'richtek,fixed-microvolt' is added for that.
| user is supposed to know whether vout of ldo is adjustable.

It also doesn't seem like this sort of behaviour would be limited to
Richtek either, should this actually be a common property in
regulator.yaml w/o the vendor prefix?

Cheers,
Conor.

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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Alina Yu]

On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote:
> On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote:
> > On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote:
> > > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote:
> > 
> > > > +            richtek,fixed-microvolt = <1200000>;
> > > >              regulator-min-microvolt = <1200000>;
> > > >              regulator-max-microvolt = <1200000>;
> > 
> > > I'm dumb and this example seemed odd to me. Can you explain to me why
> > > it is not sufficient to set min-microvolt == max-microvolt to achieve
> > > the same thing?
> > 
> > This is for a special mode where the voltage being configured is out of
> > the range usually supported by the regulator, requiring a hardware
> > design change to achieve.  The separate property is because otherwise we
> > can't distinguish the case where the mode is in use from the case where
> > the constraints are nonsense, and we need to handle setting a fixed
> > voltage on a configurable regulator differently to there being a
> > hardware fixed voltage on a normally configurable regulator.
> 
> Cool, I think an improved comment message and description would be
> helpful then to describe the desired behaviour that you mention here.
> The commit message in particular isn't great:
> | Since there is no way to check is ldo is adjustable or not.
> | As discussing in v2 series, 'richtek,fixed-microvolt' is added for that.
> | user is supposed to know whether vout of ldo is adjustable.
> 
> It also doesn't seem like this sort of behaviour would be limited to
> Richtek either, should this actually be a common property in
> regulator.yaml w/o the vendor prefix?
> 
> Cheers,
> Conor.


Hi Conor,


Should I update v4 to fix the commit message ?
I will modify it as follows.


There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode.

As the fixed voltage for the LDO is outside the range of the adjustable voltage mode,
the constraints for this scenario are not suitable to represent both modes.

In version 3, a property has been added to specify the fixed voltage.


Thanks,
Alina

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Conor Dooley]

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

On Wed, May 15, 2024 at 03:38:30PM +0800, Alina Yu wrote:
> On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote:
> > On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote:
> > > On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote:
> > > > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote:
> > > 
> > > > > +            richtek,fixed-microvolt = <1200000>;
> > > > >              regulator-min-microvolt = <1200000>;
> > > > >              regulator-max-microvolt = <1200000>;
> > > 
> > > > I'm dumb and this example seemed odd to me. Can you explain to me why
> > > > it is not sufficient to set min-microvolt == max-microvolt to achieve
> > > > the same thing?
> > > 
> > > This is for a special mode where the voltage being configured is out of
> > > the range usually supported by the regulator, requiring a hardware
> > > design change to achieve.  The separate property is because otherwise we
> > > can't distinguish the case where the mode is in use from the case where
> > > the constraints are nonsense, and we need to handle setting a fixed
> > > voltage on a configurable regulator differently to there being a
> > > hardware fixed voltage on a normally configurable regulator.
> > 
> > Cool, I think an improved comment message and description would be
> > helpful then to describe the desired behaviour that you mention here.
> > The commit message in particular isn't great:
> > | Since there is no way to check is ldo is adjustable or not.
> > | As discussing in v2 series, 'richtek,fixed-microvolt' is added for that.
> > | user is supposed to know whether vout of ldo is adjustable.
> > 
> > It also doesn't seem like this sort of behaviour would be limited to
> > Richtek either, should this actually be a common property in
> > regulator.yaml w/o the vendor prefix?
> > 
> > Cheers,
> > Conor.
> 
> 
> Hi Conor,
> 
> 
> Should I update v4 to fix the commit message ?
> I will modify it as follows.
> 
> 
> There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode.
> 
> As the fixed voltage for the LDO is outside the range of the adjustable voltage mode,
> the constraints for this scenario are not suitable to represent both modes.

That's definitely an improvement, yes. The property description could
also do with an update to explain that this is for a situation where the
fixed voltage is out of the adjustable range, it doesn't mention that at
all right now.

> In version 3, a property has been added to specify the fixed voltage.

Don't refer to previous versions of the patchset in your commit message,
that doesn't help people reading a commit log in the future etc. If
there's some relevant information in a previous version patchset, put it
in the commit message directly.

Cheers,
Conor.

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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Alina Yu]

On Wed, May 15, 2024 at 09:06:06AM +0100, Conor Dooley wrote:
> On Wed, May 15, 2024 at 03:38:30PM +0800, Alina Yu wrote:
> > On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote:
> > > On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote:
> > > > On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote:
> > > > > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote:
> > > > 
> > > > > > +            richtek,fixed-microvolt = <1200000>;
> > > > > >              regulator-min-microvolt = <1200000>;
> > > > > >              regulator-max-microvolt = <1200000>;
> > > > 
> > > > > I'm dumb and this example seemed odd to me. Can you explain to me why
> > > > > it is not sufficient to set min-microvolt == max-microvolt to achieve
> > > > > the same thing?
> > > > 
> > > > This is for a special mode where the voltage being configured is out of
> > > > the range usually supported by the regulator, requiring a hardware
> > > > design change to achieve.  The separate property is because otherwise we
> > > > can't distinguish the case where the mode is in use from the case where
> > > > the constraints are nonsense, and we need to handle setting a fixed
> > > > voltage on a configurable regulator differently to there being a
> > > > hardware fixed voltage on a normally configurable regulator.
> > > 
> > > Cool, I think an improved comment message and description would be
> > > helpful then to describe the desired behaviour that you mention here.
> > > The commit message in particular isn't great:
> > > | Since there is no way to check is ldo is adjustable or not.
> > > | As discussing in v2 series, 'richtek,fixed-microvolt' is added for that.
> > > | user is supposed to know whether vout of ldo is adjustable.
> > > 
> > > It also doesn't seem like this sort of behaviour would be limited to
> > > Richtek either, should this actually be a common property in
> > > regulator.yaml w/o the vendor prefix?
> > > 
> > > Cheers,
> > > Conor.
> > 
> > 
> > Hi Conor,
> > 
> > 
> > Should I update v4 to fix the commit message ?
> > I will modify it as follows.
> > 
> > 
> > There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode.
> > 
> > As the fixed voltage for the LDO is outside the range of the adjustable voltage mode,
> > the constraints for this scenario are not suitable to represent both modes.
> 
> That's definitely an improvement, yes. The property description could
> also do with an update to explain that this is for a situation where the
> fixed voltage is out of the adjustable range, it doesn't mention that at
> all right now.
> 
> > In version 3, a property has been added to specify the fixed voltage.
> 
> Don't refer to previous versions of the patchset in your commit message,
> that doesn't help people reading a commit log in the future etc. If
> there's some relevant information in a previous version patchset, put it
> in the commit message directly.
> 
> Cheers,
> Conor.

Hi Conor,

May I modify the description as follows ?

        properties:
          richtek,fixed-microvolt:
            description: |
-              If it exists, the voltage is unadjustable.
-              There is no risk-free method for software to determine whether the ldo vout is fixed or not.
-              Therefore, it can only be done in this way.
+
+              There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode.
+              For fixed voltage mode, the working voltage is out side the range of the adjustable mode.
+              The constraints are unsuitable to describe both modes.
+              Therefore, this property is added to specify the fixed LDO VOUT.

Thanks,
Alina

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Conor Dooley]

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

On Wed, May 15, 2024 at 05:02:29PM +0800, Alina Yu wrote:
>         properties:
>           richtek,fixed-microvolt:
>             description: |
> -              If it exists, the voltage is unadjustable.
> -              There is no risk-free method for software to determine whether the ldo vout is fixed or not.
> -              Therefore, it can only be done in this way.
> +
> +              There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode.
> +              For fixed voltage mode, the working voltage is out side the range of the adjustable mode.
> +              The constraints are unsuitable to describe both modes.
> +              Therefore, this property is added to specify the fixed LDO VOUT.

I think you could do something as simple as:
	This property can be used to set a fixed operating voltage that lies
	outside of the range of the regulator's adjustable mode.

BTW, you should probably change the example so that the voltage you add
there is actually outside of the range, rather than identical to one of
the range's constraints :)

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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Mark Brown]

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

On Wed, May 15, 2024 at 04:51:30PM +0100, Conor Dooley wrote:

> BTW, you should probably change the example so that the voltage you add
> there is actually outside of the range, rather than identical to one of
> the range's constraints :)

No, that'd be invalid - the constraints need to include a value offered
by the regulator, in this case the one fixed voltage.  For a fixed
voltage regulator it's probably better to just not specify a voltage
range since it can't be changed anyway.

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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Conor Dooley]

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

On Wed, May 15, 2024 at 05:04:32PM +0100, Mark Brown wrote:
> On Wed, May 15, 2024 at 04:51:30PM +0100, Conor Dooley wrote:
> 
> > BTW, you should probably change the example so that the voltage you add
> > there is actually outside of the range, rather than identical to one of
> > the range's constraints :)
> 
> No, that'd be invalid - the constraints need to include a value offered
> by the regulator, in this case the one fixed voltage.  For a fixed
> voltage regulator it's probably better to just not specify a voltage
> range since it can't be changed anyway.

I see. Clearly I got your previous explanation of why the property was
needed arseways.



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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Mark Brown]

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

On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote:

> It also doesn't seem like this sort of behaviour would be limited to
> Richtek either, should this actually be a common property in
> regulator.yaml w/o the vendor prefix?

It's a pretty weird thing for hardware to do - usually if the regulator
is controllable it'll be qualified for use within whatever range it's
variable over and not some other completely disjoint value.

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

Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Conor Dooley]

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

On Wed, May 15, 2024 at 01:10:31PM +0100, Mark Brown wrote:
> On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote:
> 
> > It also doesn't seem like this sort of behaviour would be limited to
> > Richtek either, should this actually be a common property in
> > regulator.yaml w/o the vendor prefix?
> 
> It's a pretty weird thing for hardware to do - usually if the regulator
> is controllable it'll be qualified for use within whatever range it's
> variable over and not some other completely disjoint value.

Okay cool, just the commit message/description to be changed then.
Thanks for the info

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

Re: (subset) [PATCH v3 0/6] Fix rtq2208 BUCK ramp_delay and LDO dvs setting

[Mark Brown]

On Fri, 10 May 2024 20:06:19 +0800, Alina Yu wrote:
> This series patches is for hardware modification of RTQ2208.
> ramp_delay range of BUCK is changed.
> The maximum ramp up and down range of BUCK are shorten
>  from 64mVstep/us to 16mVstep/us.
> The LDO's Vout is adjustable if the hardware setting allow it,
> and it can be set either 1800mv or 3300mv.
> Additionally, the discharge register has been moved to another position.
> In this version, a software bug has been fixed.
> rtq2208_ldo_match is no longer a local variable.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/6] regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called
      commit: 72b6a2d6506843375c7b91197f49ef38ca0c6d0f

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark