Fix issue when using devm_of_regulator_put_matches and add compatiblity for both fixed and adjustable LDO VOUT

PATCH RESEND In review Alina Yu 3 participants 8 messages May 28, 2024 – May 29, 2024

Subsystems (2)

Device tree bindings Voltage and current regulator framework

lore

Share & tools

lore

Alina Yu

author

2024-05-28 06:01 UTC #0 lore raw

cover Fix issue when using devm_of_regulator_put_matches and add compatiblity for both fixed and adjustable LDO VOUT

Hi,

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. Therefore, A property is added to specify the fixed LDO VOUT.

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 invalid memory access when
devm_of_regulator_put_matches is called
regulator: rtq2208: Fix LDO to be compatible with both fixed and
adjustable vout
regulator: rtq2208: Add fixed LDO VOUT property and check that matches
the constraints
regulator: dt-bindings: rtq2208: Add specified fixed LDO VOUT property

 .../bindings/regulator/richtek,rtq2208.yaml        | 11 +++-
 drivers/regulator/rtq2208-regulator.c              | 66 ++++++++++++++++------
 2 files changed, 60 insertions(+), 17 deletions(-)

Signature · 1 line

2.7.4

Alina Yu

author

2024-05-28 06:01 UTC #1 lore raw

PATCH RESEND 1/4 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]>

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

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

index b90e53d..c31b6dc 100644
@@ -228,6 +228,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) {

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

@@ -338,14 +342,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;

@@ -388,8 +392,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 {

@@ -461,8 +464,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");

@@ -474,7 +476,7 @@ 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)

@@ -482,7 +484,7 @@ static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *
 	}
 
 	/* 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

Alina Yu

author

2024-05-28 06:01 UTC #2 lore raw

PATCH RESEND 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 "richtek,fixed-microvolt". If adjustable, the Vout can be set to either 1800mV or 3300mV.

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

---
 drivers/regulator/rtq2208-regulator.c | 37 +++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

drivers/regulator/rtq2208-regulator.c +31 -6

index c31b6dc..f6afa4e 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", },

@@ -331,7 +348,7 @@ 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;
 	int ret, i;

@@ -352,13 +369,21 @@ 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;
+		if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+			desc->fixed_uV = init_data->constraints.min_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;

Signature · 1 line

2.7.4

Alina Yu

author

2024-05-28 06:01 UTC #3 lore raw

PATCH RESEND 3/4 regulator: rtq2208: Add fixed LDO VOUT property and check that matches the constraints

In reply to Alina Yu

A fixed LDO VOUT property has been added to specify the fixed_uV of the regulator_desc. Additionally, a check has been included in this version
to ensure that the fixed_uV matches the constraints.

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

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

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

index f6afa4e..a5c126af 100644
@@ -350,6 +350,7 @@ static int rtq2208_of_get_ldo_dvs_ability(struct device *dev)
 	struct of_regulator_match *match;
 	struct regulator_desc *desc;
 	struct regulator_init_data *init_data;
+	u32 fixed_uV;
 	int ret, i;
 
 	if (!dev->of_node)

@@ -374,9 +375,15 @@ static int rtq2208_of_get_ldo_dvs_ability(struct device *dev)
 		if (!init_data || !desc)
 			continue;
 
-		if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
-			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->fixed_uV = init_data->constraints.min_uV;
 			desc->ops = &rtq2208_regulator_ldo_fix_ops;
 		} else {

Signature · 1 line

2.7.4

Alina Yu

author

2024-05-28 06:01 UTC #4 lore raw

PATCH RESEND 4/4 regulator: dt-bindings: rtq2208: Add specified fixed LDO VOUT property

In reply to Alina Yu

Examples of fixed LDO VOUT and adjustable LDO VOUT is also added to this version.

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

---
 .../devicetree/bindings/regulator/richtek,rtq2208.yaml        | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml +10 -1

index 609c066..87accc6 100644
@@ -75,6 +75,12 @@ properties:
         description:
           regulator description for ldo[1-2].
 
+        properties:
+          richtek,fixed-microvolt:
+            description: |
+              This property can be used to set a fixed operating voltage that lies outside
+              the range of the regulator's adjustable mode.
+
 required:
   - compatible
   - reg

@@ -177,6 +183,8 @@ examples:
             };
           };
           ldo1 {
+            /* Fixed LDO VOUT */
+            richtek,fixed-microvolt = <1200000>;
             regulator-min-microvolt = <1200000>;
             regulator-max-microvolt = <1200000>;
             regulator-always-on;

@@ -185,7 +193,8 @@ examples:
             };
           };
           ldo2 {
-            regulator-min-microvolt = <3300000>;
+            /* Adjustable LDO VOUT */
+            regulator-min-microvolt = <1800000>;
             regulator-max-microvolt = <3300000>;
             regulator-always-on;
             regulator-state-mem {

Signature · 1 line

2.7.4

Krzysztof Kozlowski

2024-05-28 06:35 UTC #5 lore raw

Re: [RESEND 4/4] regulator: dt-bindings: rtq2208: Add specified fixed LDO VOUT property

Quoted 7 lines · Alina Yu

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. Therefore, A property is added to specify the fixed LDO VOUT.
Examples of fixed LDO VOUT and adjustable LDO VOUT is also added to this version.
Signed-off-by Alina Yu <[email protected]>

Nothing improved. Where is the changelog and versioning? I already asked about this.

Also,

This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions, under or above your Signed-off-by tag. Tag is "received", when provided in a message replied to you on the mailing list. Tools like b4 can help here. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for tags received on the version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.

Best regards,
Krzysztof

Mark Brown

maintainer

2024-05-29 11:21 UTC #6 lore raw

Re: (subset) [RESEND 0/4] Fix issue when using devm_of_regulator_put_matches and add compatiblity for both fixed and adjustable LDO VOUT

In reply to Alina Yu

Quoted 11 lines · Alina Yu

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. Therefore, A property is added to specify the fixed LDO VOUT.
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 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

Mark Brown

maintainer

2024-05-29 15:09 UTC #7 lore raw

In reply to Alina Yu

Quoted 11 lines · Alina Yu

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. Therefore, A property is added to specify the fixed LDO VOUT.
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 invalid memory access when devm_of_regulator_put_matches is called

      commit: 72b6a2d6506843375c7b91197f49ef38ca0c6d0f

[2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
(no commit info)
[3/4] regulator: rtq2208: Add fixed LDO VOUT property and check that matches the constraints
(no commit info)
[4/4] regulator: dt-bindings: rtq2208: Add specified fixed LDO VOUT property
(no commit info)

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