[PATCH v2 0/4] Fix rtq2208 BUCK ramp_delay and LDO dvs setting

[PATCH v2 0/4] 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.
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(-)

-- 
2.7.4

[PATCH v2 1/4] regulator: rtq2208: Fix LDO discharge register and add 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.
Additionally, the discharge register has been moved to another position.

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

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 2d54844..63d4037 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,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 */
-- 
2.7.4

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

[Mark Brown]

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

On Tue, Apr 30, 2024 at 05:58:24PM +0800, Alina Yu wrote:
> 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.

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

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

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

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 63d4037..80a4d2f 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -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");
 
-- 
2.7.4

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

[Mark Brown]

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

On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:

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

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

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

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

[Alina Yu]

On Wed, May 01, 2024 at 11:19:05AM +0900, Mark Brown wrote:
> On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:
> 
> > 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.
> 
> > +		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

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

[Alina Yu]

On Thu, May 02, 2024 at 03:30:29PM +0800, Alina Yu wrote:
> On Wed, May 01, 2024 at 11:19:05AM +0900, Mark Brown wrote:
> > On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:
> > 
> > > 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.
> > 
> > > +		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.
> 
> 

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

> 
> 

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

[Mark Brown]

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

On Thu, May 02, 2024 at 05:26:14PM +0800, Alina Yu wrote:

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

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

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

[Alina Yu]

On Fri, May 03, 2024 at 10:41:04AM +0900, Mark Brown wrote:
> On Thu, May 02, 2024 at 05:26:14PM +0800, Alina Yu wrote:
> 
> > 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

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

[Mark Brown]

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

On Fri, May 03, 2024 at 03:35:36PM +0800, Alina Yu wrote:
> On Fri, May 03, 2024 at 10:41:04AM +0900, Mark Brown wrote:

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

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

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

[Alina Yu]

On Mon, May 06, 2024 at 11:58:29PM +0900, Mark Brown wrote:
> On Fri, May 03, 2024 at 03:35:36PM +0800, Alina Yu wrote:
> > On Fri, May 03, 2024 at 10:41:04AM +0900, Mark Brown wrote:
> 
> > > 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.

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

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

[Mark Brown]

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

On Wed, May 08, 2024 at 02:54:02PM +0800, Alina Yu wrote:

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

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

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

[Alina Yu]

On Wed, May 08, 2024 at 08:53:34PM +0900, Mark Brown wrote:
> On Wed, May 08, 2024 at 02:54:02PM +0800, Alina Yu wrote:
> 
> > 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

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

[Mark Brown]

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

On Thu, May 09, 2024 at 05:15:03PM +0800, Alina Yu wrote:

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

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

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

[Alina Yu]

On Thu, May 09, 2024 at 05:40:19PM +0200, Mark Brown wrote:
> On Thu, May 09, 2024 at 05:15:03PM +0800, Alina Yu wrote:
> 
> > 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

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

[Mark Brown]

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

On Thu, May 02, 2024 at 03:30:29PM +0800, Alina Yu wrote:
> On Wed, May 01, 2024 at 11:19:05AM +0900, Mark Brown wrote:
> > On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:

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

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.

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

[PATCH v2 3/4] 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>
---
 drivers/regulator/rtq2208-regulator.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 80a4d2f..97a97a4 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -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");
 
-- 
2.7.4

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

[Mark Brown]

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

On Tue, Apr 30, 2024 at 05:58:26PM +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.

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

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

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

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

[Mark Brown]

On Tue, 30 Apr 2024 17:58:23 +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.
> It prevents invalid memory access when devm_of_regulator_put_matches is called.
> 
> [...]

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