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

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

[Richard Zhao]

From: Richard Zhao <richard.zhao@freescale.com>

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 <richard.zhao@freescale.com>
Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 drivers/regulator/fixed.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index a44a017..fa0c426 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -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) {
-- 
1.7.5.4

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

[Mark Brown]

On Fri, Dec 30, 2011 at 02:06:53PM +0800, Richard Zhao wrote:
> From: Richard Zhao <richard.zhao@freescale.com>
> 
> 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.

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

[Richard Zhao]

On Fri, Dec 30, 2011 at 10:30:27AM +0000, Mark Brown wrote:
> On Fri, Dec 30, 2011 at 02:06:53PM +0800, Richard Zhao wrote:
> > From: Richard Zhao <richard.zhao@freescale.com>
> > 
> > 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.
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

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

[Mark Brown]

On Fri, Dec 30, 2011 at 06:38:31PM +0800, Richard Zhao wrote:

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

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

[Richard Zhao]

On Fri, Dec 30, 2011 at 10:48:50AM +0000, Mark Brown wrote:
> On Fri, Dec 30, 2011 at 06:38:31PM +0800, Richard Zhao wrote:
> 
> > 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.
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -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

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

[Mark Brown]

On Fri, Dec 30, 2011 at 07:08:43PM +0800, Richard Zhao wrote:
> On Fri, Dec 30, 2011 at 10:48:50AM +0000, Mark Brown wrote:

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

> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -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.

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

[Richard Zhao]

Hi Mark,

On Sat, Dec 31, 2011 at 01:02:40AM +0000, Mark Brown wrote:
> On Fri, Dec 30, 2011 at 07:08:43PM +0800, Richard Zhao wrote:
> > On Fri, Dec 30, 2011 at 10:48:50AM +0000, Mark Brown wrote:
> 
> > > 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.
> 
> > --- a/drivers/regulator/of_regulator.c
> > +++ b/drivers/regulator/of_regulator.c
> > @@ -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

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

[Mark Brown]

On Sat, Dec 31, 2011 at 09:26:30AM +0800, Richard Zhao wrote:
> On Sat, Dec 31, 2011 at 01:02:40AM +0000, Mark Brown wrote:

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

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

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

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

[Richard Zhao]

Hi Mark
On 1 January 2012 05:59, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Dec 31, 2011 at 09:26:30AM +0800, Richard Zhao wrote:
>> On Sat, Dec 31, 2011 at 01:02:40AM +0000, Mark Brown wrote:
>
>> > > > 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.
>
>> > > - ? ? ? /* 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.
>
>> 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

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

[Mark Brown]

On Mon, Jan 02, 2012 at 09:46:01PM +0800, Richard Zhao wrote:

> On 1 January 2012 05:59, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

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

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

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

[Richard Zhao]

Hi Mark,

On Mon, Jan 02, 2012 at 02:39:05PM +0000, Mark Brown wrote:
> On Mon, Jan 02, 2012 at 09:46:01PM +0800, Richard Zhao wrote:
> 
> > On 1 January 2012 05:59, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > > 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.
>  - 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.
> 
> 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.
> 
> 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
> 
> > >> 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.

[PATCH 1/1] regulator/fixed: pass regulator_register of_node

[Richard Zhao]

regulator_get needs of_node to find right regulator.

Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 drivers/regulator/fixed.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index fa0c426..e24e3a1 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -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);
-- 
1.7.5.4

[PATCH 1/1] regulator/fixed: pass regulator_register of_node

[Mark Brown]

On Sat, Dec 31, 2011 at 08:51:16PM +0800, Richard Zhao wrote:
> regulator_get needs of_node to find right regulator.
> 
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>

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

[PATCH 1/1] regulator/fixed: pass regulator_register of_node

[Richard Zhao]

Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

>On Sat, Dec 31, 2011 at 08:51:16PM +0800, Richard Zhao wrote:
>> regulator_get needs of_node to find right regulator.
>> 
>> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
>
>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

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

[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 <richard.zhao@linaro.org>
---
 drivers/regulator/fixed.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 393f039..e24e3a1 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -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) {
-- 
1.7.5.4

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

[Richard Zhao]

Hi Mark,

What do you think of the patch?

Thanks
Richard 

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

[Mark Brown]

On Fri, Jan 06, 2012 at 05:51:18PM +0800, Richard Zhao wrote:

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

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

[Mark Brown]

On Wed, Jan 04, 2012 at 11:07:29AM +0800, Richard Zhao wrote:
> 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.