Skip to contentSkip to conversation

Fix rtq2208 BUCK ramp_delay and LDO dvs setting

PATCH In review Alina Yu 4 participants 21 messages May 10, 2024 – May 29, 2024

Subsystems (2)
Device tree bindings Voltage and current regulator framework
lore
Share & tools
lore

Alina Yu

author
#0 lore raw

v3 cover Fix rtq2208 BUCK ramp_delay and LDO dvs setting

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(-)
Signature · 1 line
2.7.4
author
#1 lore raw

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

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 <[email protected]>

---
v3
- Move Fixes to the start of the series

---
 drivers/regulator/rtq2208-regulator.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)
drivers/regulator/rtq2208-regulator.c +16 -14
index 2d54844..dfa293a 100644
@@ -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");
Signature · 1 line
2.7.4
maintainer
#2 lore raw

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

Quoted 4 lines · 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.

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

author
#3 lore raw

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

Quoted 8 lines · Mark Brown
Quoted 4 lines · 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.

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

author
#4 lore raw

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

In reply to 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 <[email protected]>

---
v3
- Seperate LDO vsel and discharge change to seperate patches

---
 drivers/regulator/rtq2208-regulator.c | 56 ++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 20 deletions(-)
drivers/regulator/rtq2208-regulator.c +36 -20
index dfa293a..00da787 100644
@@ -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;
Signature · 1 line
2.7.4
author
#5 lore raw

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

In reply to 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 <[email protected]>

---
v3
- Seperate LDO vsel and discharge change to seperate patches

---
 drivers/regulator/rtq2208-regulator.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/regulator/rtq2208-regulator.c +10 -5
index 00da787..cea8e77 100644
@@ -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;
Signature · 1 line
2.7.4
author
#6 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/regulator/rtq2208-regulator.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/regulator/rtq2208-regulator.c +3 -4
index cea8e77..2e9387f 100644
@@ -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
 	 */
Signature · 1 line
2.7.4
author
#7 lore raw

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

In reply to 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 <[email protected]>

---
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(-)
drivers/regulator/rtq2208-regulator.c +37 -6
index 2e9387f..c2c1689 100644
@@ -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;
Signature · 1 line
2.7.4
author
#8 lore raw

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

In reply to 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 <[email protected]>
---
 Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml +8 -0
index 609c066..6212f44 100644
@@ -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;
Signature · 1 line
2.7.4
#9 lore raw

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

Quoted 28 lines · 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 <[email protected]>
---
 Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml +7 -0
index 609c066..6212f44 100644
@@ -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

Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml examples: · line 190

Quoted 7 lines
Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml +1 -0
@@ -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.

maintainer
#10 lore raw

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

Quoted 9 lines · Conor Dooley
Quoted 3 lines · Alina Yu
+            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.

#11 lore raw

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

Quoted 11 lines · Mark Brown
Quoted 9 lines · Conor Dooley
Quoted 3 lines · Alina Yu
+            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?

Quoted 7 lines

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.

author
#12 lore raw

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

Quoted 27 lines · Conor Dooley
Quoted 11 lines · Mark Brown
Quoted 9 lines · Conor Dooley
Quoted 3 lines · Alina Yu
+            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?

Quoted 7 lines

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.
Quoted 6 lines

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

#13 lore raw

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

Quoted 39 lines · Alina Yu
Quoted 27 lines · Conor Dooley
Quoted 11 lines · Mark Brown
Quoted 9 lines · Conor Dooley
Quoted 3 lines · Alina Yu
+            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?

Quoted 7 lines

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.
Quoted 6 lines

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,

Quoted 8 lines

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.

Quoted 1 line

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.

author
#14 lore raw

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

Quoted 56 lines · Conor Dooley
Quoted 39 lines · Alina Yu
Quoted 27 lines · Conor Dooley
Quoted 11 lines · Mark Brown
Quoted 9 lines · Conor Dooley
Quoted 3 lines · Alina Yu
+            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?

Quoted 7 lines

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.
Quoted 6 lines

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,

Quoted 8 lines

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.

Quoted 1 line

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

Quoted 7 lines

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

#15 lore raw

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

Review

Quoted 11 lines · Alina Yu

properties:
richtek,fixed-microvolt:
description: |

(quoted diff) +5 -3
-              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 :)

maintainer
#16 lore raw

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

Quoted 3 lines · Conor Dooley

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.

#17 lore raw

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

Quoted 10 lines · Mark Brown
Quoted 3 lines · Conor Dooley

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.

maintainer
#18 lore raw

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

In reply to Conor Dooley
Quoted 3 lines · Conor Dooley

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.

#19 lore raw

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

Quoted 9 lines · Mark Brown
Quoted 3 lines · Conor Dooley

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

maintainer
#20 lore raw

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

In reply to Alina Yu
Quoted 11 lines · Alina Yu

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