regulator/fixed: set constraints.apply_uV to 0

PATCH Superseded Richard Zhao 3 participants 18 messages Dec 30, 2011 – Jan 6, 2012 Voltage and current regulator framework

lore

Share & tools

lore

Richard Zhao

author

2011-12-30 06:06 UTC #0 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

From: Richard Zhao <[email protected]>

Fix fixed regulator failed to call regulator_register.

Fixed regulator does not support set voltage operation, so we don't need apply_uV.

Signed-off-by Richard Zhao <[email protected]>

---
 drivers/regulator/fixed.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/regulator/fixed.c +1 -0

index a44a017..fa0c426 100644
@@ -69,6 +69,7 @@ of_get_fixed_voltage_config(struct device *dev)
 		return NULL;
 
 	init_data = config->init_data;
+	init_data->constraints.apply_uV = 0;
 
 	config->supply_name = init_data->constraints.name;
 	if (init_data->constraints.min_uV == init_data->constraints.max_uV) {

Signature · 1 line

1.7.5.4

Mark Brown

2011-12-30 10:30 UTC #1 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Quoted 6 lines · Richard Zhao

From: Richard Zhao <[email protected]>
Fix fixed regulator failed to call regulator_register.
Fixed regulator does not support set voltage operation, so we don't need apply_uV.

No, the regulator drivers shouldn't be fiddling around with their constraints. Generally the answer is that users shouldn't set constraints that won't work, or perhaps the API needs to be more forgiving.

Richard Zhao

2011-12-30 10:38 UTC #2 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Quoted 5 lines · Mark Brown

Quoted 4 lines · Richard Zhao
From: Richard Zhao <[email protected]>
Fix fixed regulator failed to call regulator_register.

Quoted 7 lines

Quoted 2 lines
Fixed regulator does not support set voltage operation, so we don't need apply_uV.
No, the regulator drivers shouldn't be fiddling around with their constraints. Generally the answer is that users shouldn't set constraints that won't work, or perhaps the API needs to be more forgiving.

So what's your suggestion of the issue? Below fixed regulator failed to register.

reg_2P5V: regulator-2P5V {
        compatible = "regulator-fixed";
        regulator-name = "2P5V";
        regulator-min-microvolt = <2500000>;
        regulator-max-microvolt = <2500000>;
        regulator-always-on;
}

Richard

Mark Brown

2011-12-30 10:48 UTC #3 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Quoted 2 lines · Richard Zhao

So what's your suggestion of the issue? Below fixed regulator failed to register.

It shouldn't actually be passing on the constraint setting the voltage when it constructs the constraints.

Richard Zhao

2011-12-30 11:08 UTC #4 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Quoted 7 lines · Mark Brown

Quoted 2 lines · Richard Zhao
So what's your suggestion of the issue? Below fixed regulator failed to register.
It shouldn't actually be passing on the constraint setting the voltage when it constructs the constraints.

drivers/regulator/of_regulator.c +0 -3

@@ -34,9 +34,6 @@ static void of_get_regulation_constraints(struct device_node *np,
        /* Voltage change possible? */
        if (constraints->min_uV != constraints->max_uV)
                constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
-       /* Only one voltage?  Then make sure it's set. */
-       if (constraints->min_uV == constraints->max_uV)
-               constraints->apply_uV = true;
 
        uV_offset = of_get_property(np, "regulator-microvolt-offset", NULL);
        if (uV_offset)

Make sense?

Thanks
Richard

Mark Brown

2011-12-31 01:02 UTC #5 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Quoted 8 lines · Richard Zhao

Quoted 2 lines · Mark Brown
It shouldn't actually be passing on the constraint setting the voltage when it constructs the constraints.

drivers/regulator/of_regulator.c static void of_get_regulation_constraints(struct device_node *np, · line 39

Quoted 6 lines

drivers/regulator/of_regulator.c +0 -3

@@ -34,9 +34,6 @@ static void of_get_regulation_constraints(struct device_node *np,
        /* Voltage change possible? */
        if (constraints->min_uV != constraints->max_uV)
                constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
-       /* Only one voltage?  Then make sure it's set. */
-       if (constraints->min_uV == constraints->max_uV)
-               constraints->apply_uV = true;

No! That's going to break *all* other users.

Richard Zhao

2011-12-31 01:26 UTC #6 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Hi Mark,

Quoted 5 lines · Mark Brown

Quoted 4 lines · Richard Zhao
Quoted 2 lines · Mark Brown
It shouldn't actually be passing on the constraint setting the voltage when it constructs the constraints.

The below code is where fixed regulator construct the constraints.

Quoted 12 lines

drivers/regulator/of_regulator.c static void of_get_regulation_constraints(struct device_node *np, · line 39

Quoted 10 lines

drivers/regulator/of_regulator.c +0 -3

@@ -34,9 +34,6 @@ static void of_get_regulation_constraints(struct device_node *np,
        /* Voltage change possible? */
        if (constraints->min_uV != constraints->max_uV)
                constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
-       /* Only one voltage?  Then make sure it's set. */
-       if (constraints->min_uV == constraints->max_uV)
-               constraints->apply_uV = true;

No! That's going to break *all* other users.

The above code follows your idea. why you still say no? Could you please past some code to show your suggested solution, or you send out a patch to fix it?

Thanks
Richard

Mark Brown

2011-12-31 21:59 UTC #7 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Quoted 6 lines · Richard Zhao

Quoted 2 lines · Mark Brown
Quoted 2 lines
Quoted 2 lines
It shouldn't actually be passing on the constraint setting the voltage when it constructs the constraints.
The below code is where fixed regulator construct the constraints.

It's not code which is specific to the fixed regulator, it's generic code that's used by all regulators.

Quoted 7 lines

Quoted 5 lines
drivers/regulator/of_regulator.c static void of_get_regulation_constraints(struct device_node *np, · line 39
Quoted 3 lines
drivers/regulator/of_regulator.c +0 -3
-       /* Only one voltage?  Then make sure it's set. */
-       if (constraints->min_uV == constraints->max_uV)
-               constraints->apply_uV = true;
No! That's going to break *all* other users.
The above code follows your idea. why you still say no?

You're just randomly inserting code for a specific driver into the generic regulator API with no consideration for the effect on other users.

Quoted 2 lines

Could you please past some code to show your suggested solution, or you send out a patch to fix it?

Not in the immediate future.

Richard Zhao

author

2012-01-02 13:46 UTC #8 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Hi Mark

Quoted 10 lines · Mark Brown

Quoted 6 lines · Richard Zhao
Quoted 2 lines · Mark Brown
Quoted 2 lines
Quoted 2 lines
It shouldn't actually be passing on the constraint setting the voltage when it constructs the constraints.
The below code is where fixed regulator construct the constraints.
It's not code which is specific to the fixed regulator, it's generic code that's used by all regulators.

The original patch is in fixed regulator driver.

Quoted 9 lines

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
- ? ? ? /* Only one voltage? ?Then make sure it's set. */
- ? ? ? if (constraints->min_uV == constraints->max_uV)
- ? ? ? ? ? ? ? constraints->apply_uV = true;
No! ?That's going to break *all* other users.
The above code follows your idea. why you still say no?

Quoted 8 lines

You're just randomly inserting code for a specific driver into the generic regulator API with no consideration for the effect on other users.
Quoted 2 lines
Could you please past some code to show your suggested solution, or you send out a patch to fix it?
Not in the immediate future.

It's a __bug__ blocking others to use fixed regulator with dt binding.

Thanks
Richard

Mark Brown

2012-01-02 14:39 UTC #9 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Quoted 6 lines · Richard Zhao

Quoted 2 lines · Mark Brown
It's not code which is specific to the fixed regulator, it's generic code that's used by all regulators.
The original patch is in fixed regulator driver.

Your original patch made no mention of device tree, it just had the driver modify its constraints which is clearly not good - aside from the design issue what happens if we make const data read only? Any boards that have their constraints marked const will start oopsing. Your new patch makes it impossible to apply a voltage to *any* regulator on a device tree which is going to break any system that needs to apply a voltage.

You really need to take a step back, think about the consequences of the changes you're making and make changes that also make sense at a design level. You can't think only about the one board that you're currently working on, it's important that you consider the wider effects of your change.

To repeat my previous suggestions you could either make the system more forgiving of silly constraints where that makes sense (and remember that one of the reasons the core rejects silly constraints is that it wants to make absolutely clear that the user wrote what they meant) or make sure that silly constraints aren't written in the first place. For example, why not make this a device tree specific change?

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Could you please past some code to show your suggested solution, or you send out a patch to fix it?
Not in the immediate future.
It's a __bug__ blocking others to use fixed regulator with dt binding.

There's some fairly clear and obvious directions to look at and simply demanding that I drop everything and implement them for you too isn't really inspiring me to do so.

Richard Zhao

2012-01-04 02:47 UTC #10 lore raw

PATCH 1/1 regulator/fixed: set constraints.apply_uV to 0

Hi Mark,

Quoted 11 lines · Mark Brown

Quoted 6 lines · Richard Zhao
Quoted 2 lines · Mark Brown
It's not code which is specific to the fixed regulator, it's generic code that's used by all regulators.
The original patch is in fixed regulator driver.
Your original patch made no mention of device tree, it just had the driver modify its constraints which is clearly not good

The fix is in function of_get_fixed_voltage_config. I'll add more comments in the commit message.

Quoted 6 lines

- aside from the
design issue what happens if we make const data read only? Any boards that have their constraints marked const will start oopsing. Your new patch makes it impossible to apply a voltage to *any* regulator on a device tree which is going to break any system that needs to apply a voltage.

The code is in dt specific function and for dt constraints is not const.

Quoted 6 lines

You really need to take a step back, think about the consequences of the changes you're making and make changes that also make sense at a design level. You can't think only about the one board that you're currently working on, it's important that you consider the wider effects of your change.

I was thinking at the point of fixed regulator DT binding user.

Quoted 7 lines

To repeat my previous suggestions you could either make the system more forgiving of silly constraints where that makes sense (and remember that one of the reasons the core rejects silly constraints is that it wants to make absolutely clear that the user wrote what they meant) or make sure that silly constraints aren't written in the first place. For example, why not make this a device tree specific change?

It is.

Thanks
Richard

Quoted 11 lines

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Could you please past some code to show your suggested solution, or you send out a patch to fix it?
Not in the immediate future.
It's a __bug__ blocking others to use fixed regulator with dt binding.
There's some fairly clear and obvious directions to look at and simply demanding that I drop everything and implement them for you too isn't really inspiring me to do so.

Richard Zhao

author

2011-12-31 12:51 UTC #11 lore raw

PATCH 1/1 regulator/fixed: pass regulator_register of_node

In reply to Richard Zhao

regulator_get needs of_node to find right regulator.

Signed-off-by Richard Zhao <[email protected]>

---
 drivers/regulator/fixed.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/regulator/fixed.c +2 -1

index fa0c426..e24e3a1 100644
@@ -247,7 +247,8 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	}
 
 	drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
-					  config->init_data, drvdata, NULL);
+					  config->init_data, drvdata,
+					  pdev->dev.of_node);
 	if (IS_ERR(drvdata->dev)) {
 		ret = PTR_ERR(drvdata->dev);
 		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);

Signature · 1 line

1.7.5.4

Mark Brown

Applied

2012-01-02 12:46 UTC #12 lore raw

PATCH 1/1 regulator/fixed: pass regulator_register of_node

Quoted 3 lines · Richard Zhao

regulator_get needs of_node to find right regulator.
Signed-off-by Richard Zhao <[email protected]>

Applied, thanks, but please always use subject lines that match the subsystem you're submitting against.

Richard Zhao

author

2012-01-02 13:22 UTC #13 lore raw

PATCH 1/1 regulator/fixed: pass regulator_register of_node

Quoted 7 lines · Mark Brown

Quoted 3 lines · Richard Zhao
regulator_get needs of_node to find right regulator.
Signed-off-by Richard Zhao <[email protected]>
Applied, thanks, but please always use subject lines that match the subsystem you're submitting against.

I thought it is more brief. I will follow the convention.

Thanks
Richard

Richard Zhao

author

2012-01-04 03:07 UTC #14 lore raw

PATCH v2 1/1 regulator: set constraints.apply_uV to 0 in of_get_fixed_voltage_config

In reply to Richard Zhao

Fix fixed regulator using DT failed to call regulator_register.

of_get_regulator_init_data set apply_uV to 1, but fixed regulator doesn't need it. Set it back to 0.

Signed-off-by Richard Zhao <[email protected]>

---
 drivers/regulator/fixed.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/regulator/fixed.c +1 -0

index 393f039..e24e3a1 100644
@@ -69,6 +69,7 @@ of_get_fixed_voltage_config(struct device *dev)
 		return NULL;
 
 	init_data = config->init_data;
+	init_data->constraints.apply_uV = 0;
 
 	config->supply_name = init_data->constraints.name;
 	if (init_data->constraints.min_uV == init_data->constraints.max_uV) {

Signature · 1 line

1.7.5.4

Richard Zhao

2012-01-06 09:51 UTC #15 lore raw

Re: [PATCH v2 1/1] regulator: set constraints.apply_uV to 0 in of_get_fixed_voltage_config

Hi Mark,

What do you think of the patch?

Thanks
Richard

Mark Brown

2012-01-06 18:43 UTC #16 lore raw

Re: [PATCH v2 1/1] regulator: set constraints.apply_uV to 0 in of_get_fixed_voltage_config

Quoted 1 line · Richard Zhao

What do you think of the patch?

I think context is important when sending e-mails, as is not burying your mail in the middle of old threads.

Mark Brown

Applied

2012-01-06 19:01 UTC #17 lore raw

Re: [PATCH v2 1/1] regulator: set constraints.apply_uV to 0 in of_get_fixed_voltage_config

In reply to Richard Zhao

Quoted 4 lines · Richard Zhao

Fix fixed regulator using DT failed to call regulator_register.
of_get_regulator_init_data set apply_uV to 1, but fixed regulator doesn't need it. Set it back to 0.

Applied, thanks.