Fix rtq2208 BUCK ramp_delay and LDO dvs setting

PATCH Superseded Alina Yu 2 participants 20 messages Apr 30, 2024 – May 10, 2024 Voltage and current regulator framework

lore

Share & tools

lore

Alina Yu

author

2024-04-30 09:58 UTC #0 lore raw

v2 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.
It prevents invalid memory access when devm_of_regulator_put_matches is called.

Alina Yu (4):
regulator: rtq2208: Fix LDO discharge register and add vsel setting
regulator: rtq2208: Fix LDO to be compatible with both fixed and
adjustable vout
regulator: rtq2208: Fix invalid memory access when
devm_of_regulator_put_matches is called
regulator: rtq2208: Fix the BUCK ramp_delay range to maximum of

    16mVstep/us

 drivers/regulator/rtq2208-regulator.c | 166 ++++++++++++++++++++++------------
 1 file changed, 106 insertions(+), 60 deletions(-)

Signature · 1 line

2.7.4

Alina Yu

author

2024-04-30 09:58 UTC #1 lore raw

PATCH v2 1/4 regulator: rtq2208: Fix LDO discharge register and add vsel setting

The LDO's Vout is adjustable if the hardware setting allows it, and it can be set either 1800mv or 3300mv.
Additionally, the discharge register has been moved to another position.

Signed-off-by Alina Yu <[email protected]>

---
 drivers/regulator/rtq2208-regulator.c | 100 +++++++++++++++++++++-------------
 1 file changed, 61 insertions(+), 39 deletions(-)

drivers/regulator/rtq2208-regulator.c +61 ~~-39~~

index 2d54844..63d4037 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,10 @@
 #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 */
 #define RTQ2208_VOUT_MAXNUM			256

@@ -318,23 +323,6 @@ 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_RG_BASE(_id)	RTQ2208_REG_BUCK_##_id##_CFG0
-#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
-
-static const struct linear_range rtq2208_vout_range[] = {
-	REGULATOR_LINEAR_RANGE(400000, 0, 180, 5000),
-	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)
 {

@@ -373,6 +361,34 @@ static int rtq2208_of_get_fixed_voltage(struct device *dev,
 	return 0;
 }
 
+
+#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 LDO_INFO(_name, _id)						\
+{									\
+	.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	VSEL_SHIFT(_sel)	(_sel ? 3 : 1)
+#define MTP_SEL_MASK(_sel)	RTQ2208_BUCK_EN_NR_MTP_SEL##_sel##_MASK
+
+static const struct linear_range rtq2208_vout_range[] = {
+	REGULATOR_LINEAR_RANGE(400000, 0, 180, 5000),
+	REGULATOR_LINEAR_RANGE(1310000, 181, 255, 10000),
+};
+
 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)
 {

@@ -380,17 +396,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;

@@ -402,15 +423,13 @@ 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->enable_reg = curr_info->enable_reg;
 	desc->active_discharge_off = 0;
-	desc->active_discharge_mask = RTQ2208_EN_DIS_MASK;
 
 	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;

@@ -418,8 +437,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);

@@ -427,14 +448,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->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);
-
-		rtq2208_ldo_match[*ldo_idx].name = desc->name;
-		rtq2208_ldo_match[*ldo_idx].driver_data = rdesc;
-		rtq2208_ldo_match[(*ldo_idx)++].desc = desc;
+		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;
+		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;

@@ -458,6 +476,10 @@ static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *
 			return -ENOMEM;
 
 		rtq2208_init_regulator_desc(rdesc[i], mtp_sel, idx, rtq2208_ldo_match, &ldo_idx);
+
+		/* init ldo dvs ability */
+		if (idx >= RTQ2208_LDO2)
+			rtq2208_ldo_match[idx - RTQ2208_LDO2].desc = &rdesc[i]->desc;
 	}
 
 	/* init ldo fixed_uV */

Signature · 1 line

2.7.4

Mark Brown

maintainer

2024-05-01 14:14 UTC #2 lore raw

Re: [PATCH v2 1/4] regulator: rtq2208: Fix LDO discharge register and add vsel setting

Quoted 3 lines · Alina Yu

The LDO's Vout is adjustable if the hardware setting allows it, and it can be set either 1800mv or 3300mv.
Additionally, the discharge register has been moved to another position.

The discharge register change should really be a separate patch since it's a bugfix, though it's probably not a super urgent one.

Alina Yu

author

2024-04-30 09:58 UTC #3 lore raw

PATCH v2 2/4 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 the init_data->constraints. If adjustable, the Vout can be set to either 1800mV or 3300mV.

Signed-off-by Alina Yu <[email protected]>

---
 drivers/regulator/rtq2208-regulator.c | 41 +++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 9 deletions(-)

drivers/regulator/rtq2208-regulator.c +32 -9

index 63d4037..80a4d2f 100644
@@ -220,7 +220,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,

@@ -229,6 +229,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 unsigned int rtq2208_of_map_mode(unsigned int mode)
 {
 	switch (mode) {

@@ -323,12 +340,12 @@ static irqreturn_t rtq2208_irq_handler(int irqno, void *devid)
 	return IRQ_HANDLED;
 }
 
-static int rtq2208_of_get_fixed_voltage(struct device *dev,
+static int rtq2208_of_get_ldo_dvs_ability(struct device *dev,
 					struct of_regulator_match *rtq2208_ldo_match, int n_fixed)
 {
 	struct device_node *np;
 	struct of_regulator_match *match;
-	struct rtq2208_regulator_desc *rdesc;
+	struct regulator_desc *desc;
 	struct regulator_init_data *init_data;
 	int ret, i;

@@ -349,13 +366,20 @@ static int rtq2208_of_get_fixed_voltage(struct device *dev,
 	for (i = 0; i < n_fixed; 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;
+		if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+			desc->n_voltages = 1;
+			desc->fixed_uV = init_data->constraints.min_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;

@@ -482,8 +506,7 @@ static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *
 			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, rtq2208_ldo_match, ldo_idx);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to get ldo fixed_uV\n");

Signature · 1 line

2.7.4

Mark Brown

maintainer

2024-05-01 02:19 UTC #4 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

drivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382

Quoted 13 lines · Alina Yu

In this patch, LDO's adjustable and fixed Vout settings are compatible. The LDO Vout ability depends on the init_data->constraints. If adjustable, the Vout can be set to either 1800mV or 3300mV.
drivers/regulator/rtq2208-regulator.c +9 -0
+		if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+			desc->n_voltages = 1;
+			desc->fixed_uV = init_data->constraints.min_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;
+		}

Why are you making this change? The operations supported by the regulator don't change depending on if the system is going to chnage the voltage.

Alina Yu

author

2024-05-02 07:30 UTC #5 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 6 lines · Mark Brown

Quoted 3 lines · Alina Yu
In this patch, LDO's adjustable and fixed Vout settings are compatible. The LDO Vout ability depends on the init_data->constraints. If adjustable, the Vout can be set to either 1800mV or 3300mV.

Quoted 13 lines

drivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382
Quoted 9 lines
drivers/regulator/rtq2208-regulator.c +9 -0
+		if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+			desc->n_voltages = 1;
+			desc->fixed_uV = init_data->constraints.min_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;
+		}
Why are you making this change? The operations supported by the regulator don't change depending on if the system is going to chnage the voltage.

The change is necessary due to the requirement of the SD card for high/default and ultra-high-speed modes. The system needs to adjust the LDO Vout accordingly. In ultra-high-speed mode, the LDO Vout needs to be adjusted to 1.8V; otherwise, it will remain at 1.8V.

Thanks,
Alina

Alina Yu

author

2024-05-02 09:26 UTC #6 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 21 lines · Alina Yu

Quoted 6 lines · Mark Brown
Quoted 3 lines · Alina Yu
In this patch, LDO's adjustable and fixed Vout settings are compatible. The LDO Vout ability depends on the init_data->constraints. If adjustable, the Vout can be set to either 1800mV or 3300mV.
Quoted 13 lines
drivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382
Quoted 9 lines
drivers/regulator/rtq2208-regulator.c +9 -0
+		if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+			desc->n_voltages = 1;
+			desc->fixed_uV = init_data->constraints.min_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;
+		}
Why are you making this change? The operations supported by the regulator don't change depending on if the system is going to chnage the voltage.

Quoted 4 lines

The change is necessary due to the requirement of the SD card for high/default and ultra-high-speed modes. The system needs to adjust the LDO Vout accordingly. In ultra-high-speed mode, the LDO Vout needs to be adjusted to 1.8V; otherwise, it will remain at 1.8V.

Apologies for the misunderstanding. Let me provide further clarification regarding the LDO Vout. For the fixed LDO Vout, it will be set to either 0.9V or 1.2V, which are outside the range of 1.8V to 3.3V. The determination of whether it is fixed or adjustable lies solely with the user.
This modification aims to ensure compatibility with the user's application.

Thanks,
Alina

Quoted 2 lines

Mark Brown

maintainer

2024-05-03 01:41 UTC #7 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 3 lines · Alina Yu

For the fixed LDO Vout, it will be set to either 0.9V or 1.2V, which are outside the range of 1.8V to 3.3V. The determination of whether it is fixed or adjustable lies solely with the user.
This modification aims to ensure compatibility with the user's application.

That's a substantail reconfiguration of the regulator, it would be better to have an explicit property for these non-standard fixed voltages rather than trying to do this using constraints, or at the very least have validation that the values being set are supported by the hardware. The code should also be very clear about what is going on.

Alina Yu

author

2024-05-03 07:35 UTC #8 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 11 lines · Mark Brown

Quoted 3 lines · Alina Yu
For the fixed LDO Vout, it will be set to either 0.9V or 1.2V, which are outside the range of 1.8V to 3.3V. The determination of whether it is fixed or adjustable lies solely with the user.
This modification aims to ensure compatibility with the user's application.
That's a substantail reconfiguration of the regulator, it would be better to have an explicit property for these non-standard fixed voltages rather than trying to do this using constraints, or at the very least have validation that the values being set are supported by the hardware. The code should also be very clear about what is going on.

May I add the 'richtek,use-fix-dvs' property back ?
which is the proerty I add in v1 patch series to let the user determines whether the LDO Vout is fixed or not.

+ fixed_uV = of_property_read_bool(match->of_node, "richtek,use-fix-dvs");
+
+ if (fixed_uV) {
+ desc->n_voltages = 1;
+ desc->fixed_uV = init_data->constraints.min_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;
+ }

Thanks,
Alina

Mark Brown

maintainer

2024-05-06 14:58 UTC #9 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 9 lines · Alina Yu

Quoted 5 lines · Mark Brown
That's a substantail reconfiguration of the regulator, it would be better to have an explicit property for these non-standard fixed voltages rather than trying to do this using constraints, or at the very least have validation that the values being set are supported by the hardware. The code should also be very clear about what is going on.
May I add the 'richtek,use-fix-dvs' property back ?

It sounds like it might be better to add a property specifying the specific fixed voltage rather than overloading the constraints for this purpose.

Alina Yu

author

2024-05-08 06:54 UTC #10 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 9 lines · Mark Brown

Quoted 7 lines · Alina Yu
Quoted 5 lines · Mark Brown
That's a substantail reconfiguration of the regulator, it would be better to have an explicit property for these non-standard fixed voltages rather than trying to do this using constraints, or at the very least have validation that the values being set are supported by the hardware. The code should also be very clear about what is going on.

Quoted 5 lines

Quoted 1 line
May I add the 'richtek,use-fix-dvs' property back ?
It sounds like it might be better to add a property specifying the specific fixed voltage rather than overloading the constraints for this purpose.

May I modify the code into this ?
I'll add 'richtek,fixed-microvolt' property in dtsi; remove 'regulator-min-microvolt' and 'regulator-max-microvolt' to prevent fail caused by constraints->apply_uV.

+       u32 fixed_uV;
        int ret, i;

- if (init_data->constraints.min_uV == init_data->constraints.max_uV) {

+               if (!of_property_read_u32(match->of_node, "richtek,fixed-microvolt", &fixed_uV)) {
			desc->n_voltages = 1;
-                       desc->fixed_uV = init_data->constraints.min_uV;
+                       desc->fixed_uV = fixed_uV;
			desc->ops = &rtq2208_regulator_ldo_fix_ops;

...

Thanks,
Alina

Mark Brown

maintainer

2024-05-08 11:53 UTC #11 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 3 lines · Alina Yu

May I modify the code into this ?
I'll add 'richtek,fixed-microvolt' property in dtsi; remove 'regulator-min-microvolt' and 'regulator-max-microvolt' to prevent fail caused by constraints->apply_uV.

Adding the new property seems fine. You still need to permit the min/max microvolt properties for the case where the regulator is in normal mode and can vary, you could write rules that ensure that the constraints line up in the case where a fixed voltage is specified but I'm not sure it's worth the effort.

Alina Yu

author

2024-05-09 09:15 UTC #12 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 11 lines · Mark Brown

Quoted 3 lines · Alina Yu
May I modify the code into this ?
I'll add 'richtek,fixed-microvolt' property in dtsi; remove 'regulator-min-microvolt' and 'regulator-max-microvolt' to prevent fail caused by constraints->apply_uV.
Adding the new property seems fine. You still need to permit the min/max microvolt properties for the case where the regulator is in normal mode and can vary, you could write rules that ensure that the constraints line up in the case where a fixed voltage is specified but I'm not sure it's worth the effort.

Or may I add the following condition to check the constraints.min_uV and constraints.max_uV match the specified fixed voltage ?

+       u32 fixed_uV;
        int ret, i;

...

+ /* 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;

...

Thanks,
Alina

Mark Brown

maintainer

2024-05-09 15:40 UTC #13 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 1 line · Alina Yu

Or may I add the following condition to check the constraints.min_uV and constraints.max_uV match the specified fixed voltage ?

That seems like a reasonable check, though I think we should just do that in the core any time we have a fixed voltage regulator rather than doing it in a single driver.

Alina Yu

author

2024-05-10 12:13 UTC #14 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

Quoted 7 lines · Mark Brown

Quoted 1 line · Alina Yu
Or may I add the following condition to check the constraints.min_uV and constraints.max_uV match the specified fixed voltage ?
That seems like a reasonable check, though I think we should just do that in the core any time we have a fixed voltage regulator rather than doing it in a single driver.

Hi,
Mark

Thank you so much for your review.
I have compiled all of your suggestions and implemented the modifications for the v3 series.

Best regards,
Alina

Mark Brown

maintainer

2024-05-03 01:37 UTC #15 lore raw

Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout

In reply to Alina Yu

Quoted 17 lines · Alina Yu

Quoted 15 lines · Mark Brown
drivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382
Quoted 9 lines · Alina Yu
drivers/regulator/rtq2208-regulator.c +9 -0
+		if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+			desc->n_voltages = 1;
+			desc->fixed_uV = init_data->constraints.min_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;
+		}
Why are you making this change? The operations supported by the regulator don't change depending on if the system is going to chnage the voltage.

Quoted 2 lines

The change is necessary due to the requirement of the SD card for high/default and ultra-high-speed modes. The system needs to adjust the LDO Vout accordingly. In ultra-high-speed mode, the LDO Vout needs to be adjusted to 1.8V; otherwise, it will remain at 1.8V.

That just sounds like normal constraints usage, and something that applies to a wide variety of systems not just this specific regulator. If there are limits on what voltages can be configured the constraints will enforce them.

Alina Yu

author

2024-04-30 09:58 UTC #16 lore raw

PATCH v2 3/4 regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called

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

---
 drivers/regulator/rtq2208-regulator.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

drivers/regulator/rtq2208-regulator.c +12 ~~-10~~

index 80a4d2f..97a97a4 100644
@@ -246,6 +246,11 @@ static const unsigned int rtq2208_ldo_volt_table[] = {
 	3300000,
 };
 
+static struct of_regulator_match rtq2208_ldo_match[] = {
+	{.name = "ldo2", },
+	{.name = "ldo1", },
+};
+
 static unsigned int rtq2208_of_map_mode(unsigned int mode)
 {
 	switch (mode) {

@@ -340,8 +345,7 @@ static irqreturn_t rtq2208_irq_handler(int irqno, void *devid)
 	return IRQ_HANDLED;
 }
 
-static int rtq2208_of_get_ldo_dvs_ability(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;

@@ -356,14 +360,14 @@ static int rtq2208_of_get_ldo_dvs_ability(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;
 		desc = (struct regulator_desc *)match->desc;

@@ -413,8 +417,7 @@ static const struct linear_range rtq2208_vout_range[] = {
 	REGULATOR_LINEAR_RANGE(1310000, 181, 255, 10000),
 };
 
-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 {

@@ -486,8 +489,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");

@@ -499,14 +501,14 @@ 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;
 	}
 
-	ret = rtq2208_of_get_ldo_dvs_ability(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

Mark Brown

maintainer

2024-05-01 02:19 UTC #17 lore raw

Re: [PATCH v2 3/4] 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.

Fixes should generally go at the start of the series, that way they can be merged without any spurious dependencies on later work.

Alina Yu

author

2024-04-30 09:58 UTC #18 lore raw

PATCH v2 4/4 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 97a97a4..39dde4e 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

Mark Brown

maintainer

2024-05-03 01:32 UTC #19 lore raw

Re: (subset) [PATCH v2 0/4] 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.
It prevents invalid memory access when devm_of_regulator_put_matches is called.

Quoted 1 line

[...]

Applied to

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

Thanks!

[1/4] regulator: rtq2208: Fix LDO discharge register and add vsel setting

      commit: 38bcec0e7cbbd6566c12ae4f2b7a48bd50cd215c
[3/4] regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called
      (no commit info)
[4/4] regulator: rtq2208: Fix the BUCK ramp_delay range to maximum of 16mVstep/us
      commit: d1ef160b45a0010d1f1b3d601230457243a8f3e8

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