Da903x regulator driver. Bug?

Da903x regulator driver. Bug?

[Jonathan Cameron]

Firstly, is lkml the right place to ask questions about regulator drivers?

Secondly, though I can't track down any examples, I'm guessing the following
is a valid board config for the da903x reg etc.

static struct regulator_init_data stargate2_ld8_init_data = {
	.supply_regulator_dev = NULL,
	.constraints = {
		.name = "vdd_mica",
		.min_uV = 1800000,
		.max_uV = 1900000,
		.valid_modes_mask = REGULATOR_CHANGE_VOLTAGE,
	},
};

/* playing with this ld0 as it only goes to an external connector */
static struct da903x_subdev_info stargate2_da9030_subdevs[] = {
	{
		.name = "da903x-regulator",
		.id = DA9030_ID_LDO8,
		.platform_data = &stargate2_ld8_init_data,
	},
};

static struct da903x_platform_data stargate2_da9030_pdata = {
	.num_subdevs = ARRAY_SIZE(stargate2_da9030_subdevs),
	.subdevs = stargate2_da9030_subdevs,
};
static struct i2c_board_info __initdata stargate2_pwr_i2c_board_info [] = {
	{
		.type = "da9030",
		.addr = 0x49,
		.platform_data = &stargate2_da9030_pdata,
		.irq = gpio_to_irq(1),
	},
};

// and relevant registration code.


Now if this is now things are expected to be, there is a bug in
regulators/da903x.c in da903x_regulator_probe

rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);

should be

rdev = regulator_register(&ri->desc, &pdev->dev, ri);

or else you aren't going to get the constraint.  I think the
first will give you the device of the mfd, not the regulator.

Thanks,

Jonathan

Re: Da903x regulator driver. Bug?

[Jonathan Cameron]

Jonathan Cameron wrote:
> Firstly, is lkml the right place to ask questions about regulator drivers?
>
> Secondly, though I can't track down any examples, I'm guessing the following
> is a valid board config for the da903x reg etc.
>
> static struct regulator_init_data stargate2_ld8_init_data = {
> 	.supply_regulator_dev = NULL,
> 	.constraints = {
> 		.name = "vdd_mica",
> 		.min_uV = 1800000,
> 		.max_uV = 1900000,
> 		.valid_modes_mask = REGULATOR_CHANGE_VOLTAGE,
> 	},
> };
>
> /* playing with this ld0 as it only goes to an external connector */
> static struct da903x_subdev_info stargate2_da9030_subdevs[] = {
> 	{
> 		.name = "da903x-regulator",
> 		.id = DA9030_ID_LDO8,
> 		.platform_data = &stargate2_ld8_init_data,
> 	},
> };
>
> static struct da903x_platform_data stargate2_da9030_pdata = {
> 	.num_subdevs = ARRAY_SIZE(stargate2_da9030_subdevs),
> 	.subdevs = stargate2_da9030_subdevs,
> };
> static struct i2c_board_info __initdata stargate2_pwr_i2c_board_info [] = {
> 	{
> 		.type = "da9030",
> 		.addr = 0x49,
> 		.platform_data = &stargate2_da9030_pdata,
> 		.irq = gpio_to_irq(1),
> 	},
> };
>
> // and relevant registration code.
>
>
> Now if this is now things are expected to be, there is a bug in
> regulators/da903x.c in da903x_regulator_probe
>
> rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
>
> should be
>
> rdev = regulator_register(&ri->desc, &pdev->dev, ri);
>
>   
Unfortunately this fix causes other issues as now the i2c_client
is 2 layers down rather than one requiring quite a few changes
to   
 struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
from
    struct device *da9034_dev = rdev_get_dev(rdev)->parent;

So either a change to the regulator framework is needed to
allow mfd's or these extra ->parent lines need to go in in lots
of places.

Which do people prefer?

Jonathan

Re: Da903x regulator driver. Bug?

[Liam Girdwood]

On Fri, 2008-10-24 at 18:23 +0100, Jonathan Cameron wrote:
> Jonathan Cameron wrote:
> > Firstly, is lkml the right place to ask questions about regulator drivers?

Yes.

> >
> > Secondly, though I can't track down any examples, I'm guessing the following
> > is a valid board config for the da903x reg etc.
> >

I'm not sure if this is a valid config for this board. Eric will
probably know for sure. 

> > static struct regulator_init_data stargate2_ld8_init_data = {
> > 	.supply_regulator_dev = NULL,
> > 	.constraints = {
> > 		.name = "vdd_mica",
> > 		.min_uV = 1800000,
> > 		.max_uV = 1900000,
> > 		.valid_modes_mask = REGULATOR_CHANGE_VOLTAGE,
> > 	},
> > };
> >
> > /* playing with this ld0 as it only goes to an external connector */
> > static struct da903x_subdev_info stargate2_da9030_subdevs[] = {
> > 	{
> > 		.name = "da903x-regulator",
> > 		.id = DA9030_ID_LDO8,
> > 		.platform_data = &stargate2_ld8_init_data,
> > 	},
> > };
> >
> > static struct da903x_platform_data stargate2_da9030_pdata = {
> > 	.num_subdevs = ARRAY_SIZE(stargate2_da9030_subdevs),
> > 	.subdevs = stargate2_da9030_subdevs,
> > };
> > static struct i2c_board_info __initdata stargate2_pwr_i2c_board_info [] = {
> > 	{
> > 		.type = "da9030",
> > 		.addr = 0x49,
> > 		.platform_data = &stargate2_da9030_pdata,
> > 		.irq = gpio_to_irq(1),
> > 	},
> > };
> >
> > // and relevant registration code.
> >
> >
> > Now if this is now things are expected to be, there is a bug in
> > regulators/da903x.c in da903x_regulator_probe
> >
> > rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
> >
> > should be
> >
> > rdev = regulator_register(&ri->desc, &pdev->dev, ri);

wm8350 and wm8400 (other mfd regulators) both register using the bottom
case. 

> >
> >   
> Unfortunately this fix causes other issues as now the i2c_client
> is 2 layers down rather than one requiring quite a few changes
> to   
>  struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
> from
>     struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> 
> So either a change to the regulator framework is needed to
> allow mfd's or these extra ->parent lines need to go in in lots
> of places.
> 
> Which do people prefer?
> 

Could you fix in a similar method to the wm8350/wm8400. 

I would also move the da903x_regulator_info lookup into each regulator
function, rather than at probe(). This would free up the registration
private data. da903x_regulator_info is an array so we should be able to
use regulator->id as the index.  

Liam

Re: Da903x regulator driver. Bug?

[Jonathan Cameron]

> 
> I'm not sure if this is a valid config for this board. Eric will
> probably know for sure.  
>>> static struct regulator_init_data stargate2_ld8_init_data = {
>>> 	.supply_regulator_dev = NULL,
>>> 	.constraints = {
>>> 		.name = "vdd_mica",
>>> 		.min_uV = 1800000,
>>> 		.max_uV = 1900000,
>>> 		.valid_modes_mask = REGULATOR_CHANGE_VOLTAGE,
>>> 	},
>>> };
>>>
>>> /* playing with this ld0 as it only goes to an external connector */
>>> static struct da903x_subdev_info stargate2_da9030_subdevs[] = {
>>> 	{
>>> 		.name = "da903x-regulator",
>>> 		.id = DA9030_ID_LDO8,
>>> 		.platform_data = &stargate2_ld8_init_data,
>>> 	},
>>> };
>>>
>>> static struct da903x_platform_data stargate2_da9030_pdata = {
>>> 	.num_subdevs = ARRAY_SIZE(stargate2_da9030_subdevs),
>>> 	.subdevs = stargate2_da9030_subdevs,
>>> };
>>> static struct i2c_board_info __initdata stargate2_pwr_i2c_board_info [] = {
>>> 	{
>>> 		.type = "da9030",
>>> 		.addr = 0x49,
>>> 		.platform_data = &stargate2_da9030_pdata,
>>> 		.irq = gpio_to_irq(1),
>>> 	},
>>> };
>>>
>>> // and relevant registration code.
>>>
>>>
>>> Now if this is now things are expected to be, there is a bug in
>>> regulators/da903x.c in da903x_regulator_probe
>>>
>>> rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
>>>
>>> should be
>>>
>>> rdev = regulator_register(&ri->desc, &pdev->dev, ri);
> 
> wm8350 and wm8400 (other mfd regulators) both register using the bottom
> case. 

Yup, guessing that the constraints stuff got added without all devices
being tested (but then this is only rc1 ;)
 
>>>   
>> Unfortunately this fix causes other issues as now the i2c_client
>> is 2 layers down rather than one requiring quite a few changes
>> to   
>>  struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>> from
>>     struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>
>> So either a change to the regulator framework is needed to
>> allow mfd's or these extra ->parent lines need to go in in lots
>> of places.
>>
>> Which do people prefer?
>>
> 
> Could you fix in a similar method to the wm8350/wm8400. 
Based on a quick look, I think this involves carrying around
an additional copy of the device pointer inside the driver data.

If so that would indeed work.

> I would also move the da903x_regulator_info lookup into each regulator
> function, rather than at probe(). This would free up the registration
> private data. da903x_regulator_info is an array so we should be able to
> use regulator->id as the index.  
> 
Sounds sensible to me, but I'll leave decision on that for the guys who
wrote the driver.

I'm just getting my head around how the whole framework is used (have been
waiting for the da903x driver to merge for a while).

Looks very nice!

Thanks,

Jonathan

Re: Da903x regulator driver. Bug?

[Mark Brown]

On Fri, Oct 24, 2008 at 07:53:37PM +0100, Jonathan Cameron wrote:

> >> is 2 layers down rather than one requiring quite a few changes
> >> to   
> >>  struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
> >> from
> >>     struct device *da9034_dev = rdev_get_dev(rdev)->parent;

> >> So either a change to the regulator framework is needed to
> >> allow mfd's or these extra ->parent lines need to go in in lots
> >> of places.

> >> Which do people prefer?

> > Could you fix in a similar method to the wm8350/wm8400. 

> Based on a quick look, I think this involves carrying around
> an additional copy of the device pointer inside the driver data.

> If so that would indeed work.

Yes, I'm actively using these.  I had been considering going back and
removing the extra layer of platform device from the code for WM8350 and
WM8400 since there is now less benefit to it but I would probably still
continue to use the driver_data to store the pointer to the wm8350 data
since I find that gives clear code.

The current situation was a minimal adaption to avoid churn in the core
API close to release - previously the regulator had been forced to be a
platform device but this was changed since we have to allocate a new
device when registering the regulator in order to have the class work in
sysfs.

[PATCH] da903x regulator bug fix

[Jonathan Cameron]

Changes the device registration part of the probe function to supply the
regulator device rather than its parent (the mfd device) as this caused
problems when the regulator core attempted to find constraints associated
with the regulators.

Signed-of-by: Jonathan Cameron <jic23@cam.ac.uk>

--
This is quickest and simplest way to fix this bug. There may be better
ways of doing it, but they all require considerably more substantial
changes to the driver.

diff --git a/drivers/regulator/da903x.c b/drivers/regulator/da903x.c
index 3688e33..f928aa1 100644
--- a/drivers/regulator/da903x.c
+++ b/drivers/regulator/da903x.c
@@ -93,7 +93,7 @@ static int da903x_set_ldo_voltage(struct regulator_dev
*rdev,
                   int min_uV, int max_uV)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
     uint8_t val, mask;
 
     if (check_range(info, min_uV, max_uV)) {
@@ -111,7 +111,7 @@ static int da903x_set_ldo_voltage(struct
regulator_dev *rdev,
 static int da903x_get_voltage(struct regulator_dev *rdev)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
     uint8_t val, mask;
     int ret;
 
@@ -128,7 +128,7 @@ static int da903x_get_voltage(struct regulator_dev
*rdev)
 static int da903x_enable(struct regulator_dev *rdev)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
 
     return da903x_set_bits(da9034_dev, info->enable_reg,
                     1 << info->enable_bit);
@@ -137,7 +137,7 @@ static int da903x_enable(struct regulator_dev *rdev)
 static int da903x_disable(struct regulator_dev *rdev)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
 
     return da903x_clr_bits(da9034_dev, info->enable_reg,
                     1 << info->enable_bit);
@@ -146,7 +146,7 @@ static int da903x_disable(struct regulator_dev *rdev)
 static int da903x_is_enabled(struct regulator_dev *rdev)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
     uint8_t reg_val;
     int ret;
 
@@ -238,7 +238,7 @@ static int da9034_set_dvc_voltage(struct
regulator_dev *rdev,
                   int min_uV, int max_uV)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
     uint8_t val, mask;
     int ret;
 
@@ -264,7 +264,7 @@ static int da9034_set_ldo12_voltage(struct
regulator_dev *rdev,
                     int min_uV, int max_uV)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
     uint8_t val, mask;
 
     if (check_range(info, min_uV, max_uV)) {
@@ -283,7 +283,7 @@ static int da9034_set_ldo12_voltage(struct
regulator_dev *rdev,
 static int da9034_get_ldo12_voltage(struct regulator_dev *rdev)
 {
     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
     uint8_t val, mask;
     int ret;
 
@@ -466,7 +466,7 @@ static int __devinit da903x_regulator_probe(struct
platform_device *pdev)
     if (ri->desc.id == DA9030_ID_LDO1 || ri->desc.id == DA9030_ID_LDO15)
         ri->desc.ops = &da9030_regulator_ldo1_15_ops;
 
-    rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
+    rdev = regulator_register(&ri->desc, &pdev->dev, ri);
     if (IS_ERR(rdev)) {
         dev_err(&pdev->dev, "failed to register regulator %s\n",
                 ri->desc.name);

Re: [PATCH] da903x regulator bug fix

[Eric Miao]

I'd suggest making this a static inline function then,
e.g. to_da903x_dev(rdev), which will help both
readability and future modifications.

On Mon, Oct 27, 2008 at 11:29 PM, Jonathan Cameron
<Jonathan.Cameron@gmail.com> wrote:
> Changes the device registration part of the probe function to supply the
> regulator device rather than its parent (the mfd device) as this caused
> problems when the regulator core attempted to find constraints associated
> with the regulators.
>
> Signed-of-by: Jonathan Cameron <jic23@cam.ac.uk>
>
> --
> This is quickest and simplest way to fix this bug. There may be better
> ways of doing it, but they all require considerably more substantial
> changes to the driver.
>
> diff --git a/drivers/regulator/da903x.c b/drivers/regulator/da903x.c
> index 3688e33..f928aa1 100644
> --- a/drivers/regulator/da903x.c
> +++ b/drivers/regulator/da903x.c
> @@ -93,7 +93,7 @@ static int da903x_set_ldo_voltage(struct regulator_dev
> *rdev,
>                   int min_uV, int max_uV)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>     uint8_t val, mask;
>
>     if (check_range(info, min_uV, max_uV)) {
> @@ -111,7 +111,7 @@ static int da903x_set_ldo_voltage(struct
> regulator_dev *rdev,
>  static int da903x_get_voltage(struct regulator_dev *rdev)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>     uint8_t val, mask;
>     int ret;
>
> @@ -128,7 +128,7 @@ static int da903x_get_voltage(struct regulator_dev
> *rdev)
>  static int da903x_enable(struct regulator_dev *rdev)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>
>     return da903x_set_bits(da9034_dev, info->enable_reg,
>                     1 << info->enable_bit);
> @@ -137,7 +137,7 @@ static int da903x_enable(struct regulator_dev *rdev)
>  static int da903x_disable(struct regulator_dev *rdev)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>
>     return da903x_clr_bits(da9034_dev, info->enable_reg,
>                     1 << info->enable_bit);
> @@ -146,7 +146,7 @@ static int da903x_disable(struct regulator_dev *rdev)
>  static int da903x_is_enabled(struct regulator_dev *rdev)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>     uint8_t reg_val;
>     int ret;
>
> @@ -238,7 +238,7 @@ static int da9034_set_dvc_voltage(struct
> regulator_dev *rdev,
>                   int min_uV, int max_uV)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>     uint8_t val, mask;
>     int ret;
>
> @@ -264,7 +264,7 @@ static int da9034_set_ldo12_voltage(struct
> regulator_dev *rdev,
>                     int min_uV, int max_uV)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>     uint8_t val, mask;
>
>     if (check_range(info, min_uV, max_uV)) {
> @@ -283,7 +283,7 @@ static int da9034_set_ldo12_voltage(struct
> regulator_dev *rdev,
>  static int da9034_get_ldo12_voltage(struct regulator_dev *rdev)
>  {
>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>     uint8_t val, mask;
>     int ret;
>
> @@ -466,7 +466,7 @@ static int __devinit da903x_regulator_probe(struct
> platform_device *pdev)
>     if (ri->desc.id == DA9030_ID_LDO1 || ri->desc.id == DA9030_ID_LDO15)
>         ri->desc.ops = &da9030_regulator_ldo1_15_ops;
>
> -    rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
> +    rdev = regulator_register(&ri->desc, &pdev->dev, ri);
>     if (IS_ERR(rdev)) {
>         dev_err(&pdev->dev, "failed to register regulator %s\n",
>                 ri->desc.name);
>
>
>
>
>



-- 
Cheers
- eric

Re: [PATCH] da903x regulator bug fix

[Eric Miao]

(Resend with corrected e-mail address of Liam)

I'd suggest making this a static inline function then,
e.g. to_da903x_dev(rdev), which will help both
readability and future modifications.

> On Mon, Oct 27, 2008 at 11:29 PM, Jonathan Cameron
> <Jonathan.Cameron@gmail.com> wrote:
>> Changes the device registration part of the probe function to supply the
>> regulator device rather than its parent (the mfd device) as this caused
>> problems when the regulator core attempted to find constraints associated
>> with the regulators.
>>
>> Signed-of-by: Jonathan Cameron <jic23@cam.ac.uk>
>>
>> --
>> This is quickest and simplest way to fix this bug. There may be better
>> ways of doing it, but they all require considerably more substantial
>> changes to the driver.
>>
>> diff --git a/drivers/regulator/da903x.c b/drivers/regulator/da903x.c
>> index 3688e33..f928aa1 100644
>> --- a/drivers/regulator/da903x.c
>> +++ b/drivers/regulator/da903x.c
>> @@ -93,7 +93,7 @@ static int da903x_set_ldo_voltage(struct regulator_dev
>> *rdev,
>>                   int min_uV, int max_uV)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>     uint8_t val, mask;
>>
>>     if (check_range(info, min_uV, max_uV)) {
>> @@ -111,7 +111,7 @@ static int da903x_set_ldo_voltage(struct
>> regulator_dev *rdev,
>>  static int da903x_get_voltage(struct regulator_dev *rdev)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>     uint8_t val, mask;
>>     int ret;
>>
>> @@ -128,7 +128,7 @@ static int da903x_get_voltage(struct regulator_dev
>> *rdev)
>>  static int da903x_enable(struct regulator_dev *rdev)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>
>>     return da903x_set_bits(da9034_dev, info->enable_reg,
>>                     1 << info->enable_bit);
>> @@ -137,7 +137,7 @@ static int da903x_enable(struct regulator_dev *rdev)
>>  static int da903x_disable(struct regulator_dev *rdev)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>
>>     return da903x_clr_bits(da9034_dev, info->enable_reg,
>>                     1 << info->enable_bit);
>> @@ -146,7 +146,7 @@ static int da903x_disable(struct regulator_dev *rdev)
>>  static int da903x_is_enabled(struct regulator_dev *rdev)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>     uint8_t reg_val;
>>     int ret;
>>
>> @@ -238,7 +238,7 @@ static int da9034_set_dvc_voltage(struct
>> regulator_dev *rdev,
>>                   int min_uV, int max_uV)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>     uint8_t val, mask;
>>     int ret;
>>
>> @@ -264,7 +264,7 @@ static int da9034_set_ldo12_voltage(struct
>> regulator_dev *rdev,
>>                     int min_uV, int max_uV)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>     uint8_t val, mask;
>>
>>     if (check_range(info, min_uV, max_uV)) {
>> @@ -283,7 +283,7 @@ static int da9034_set_ldo12_voltage(struct
>> regulator_dev *rdev,
>>  static int da9034_get_ldo12_voltage(struct regulator_dev *rdev)
>>  {
>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>     uint8_t val, mask;
>>     int ret;
>>
>> @@ -466,7 +466,7 @@ static int __devinit da903x_regulator_probe(struct
>> platform_device *pdev)
>>     if (ri->desc.id == DA9030_ID_LDO1 || ri->desc.id == DA9030_ID_LDO15)
>>         ri->desc.ops = &da9030_regulator_ldo1_15_ops;
>>
>> -    rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
>> +    rdev = regulator_register(&ri->desc, &pdev->dev, ri);
>>     if (IS_ERR(rdev)) {
>>         dev_err(&pdev->dev, "failed to register regulator %s\n",
>>                 ri->desc.name);
>>
>>
>>
>>
>>
>
>
>
> --
> Cheers
> - eric
>



-- 
Cheers
- eric

Re: [PATCH] da903x regulator bug fix

[Jonathan Cameron]

Good idea, patch to follow.

Jonathan
> (Resend with corrected e-mail address of Liam)
> 
> I'd suggest making this a static inline function then,
> e.g. to_da903x_dev(rdev), which will help both
> readability and future modifications.
> 
>> On Mon, Oct 27, 2008 at 11:29 PM, Jonathan Cameron
>> <Jonathan.Cameron@gmail.com> wrote:
>>> Changes the device registration part of the probe function to supply the
>>> regulator device rather than its parent (the mfd device) as this caused
>>> problems when the regulator core attempted to find constraints associated
>>> with the regulators.
>>>
>>> Signed-of-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>
>>> --
>>> This is quickest and simplest way to fix this bug. There may be better
>>> ways of doing it, but they all require considerably more substantial
>>> changes to the driver.
>>>
>>> diff --git a/drivers/regulator/da903x.c b/drivers/regulator/da903x.c
>>> index 3688e33..f928aa1 100644
>>> --- a/drivers/regulator/da903x.c
>>> +++ b/drivers/regulator/da903x.c
>>> @@ -93,7 +93,7 @@ static int da903x_set_ldo_voltage(struct regulator_dev
>>> *rdev,
>>>                   int min_uV, int max_uV)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>     uint8_t val, mask;
>>>
>>>     if (check_range(info, min_uV, max_uV)) {
>>> @@ -111,7 +111,7 @@ static int da903x_set_ldo_voltage(struct
>>> regulator_dev *rdev,
>>>  static int da903x_get_voltage(struct regulator_dev *rdev)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>     uint8_t val, mask;
>>>     int ret;
>>>
>>> @@ -128,7 +128,7 @@ static int da903x_get_voltage(struct regulator_dev
>>> *rdev)
>>>  static int da903x_enable(struct regulator_dev *rdev)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>
>>>     return da903x_set_bits(da9034_dev, info->enable_reg,
>>>                     1 << info->enable_bit);
>>> @@ -137,7 +137,7 @@ static int da903x_enable(struct regulator_dev *rdev)
>>>  static int da903x_disable(struct regulator_dev *rdev)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>
>>>     return da903x_clr_bits(da9034_dev, info->enable_reg,
>>>                     1 << info->enable_bit);
>>> @@ -146,7 +146,7 @@ static int da903x_disable(struct regulator_dev *rdev)
>>>  static int da903x_is_enabled(struct regulator_dev *rdev)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>     uint8_t reg_val;
>>>     int ret;
>>>
>>> @@ -238,7 +238,7 @@ static int da9034_set_dvc_voltage(struct
>>> regulator_dev *rdev,
>>>                   int min_uV, int max_uV)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>     uint8_t val, mask;
>>>     int ret;
>>>
>>> @@ -264,7 +264,7 @@ static int da9034_set_ldo12_voltage(struct
>>> regulator_dev *rdev,
>>>                     int min_uV, int max_uV)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>     uint8_t val, mask;
>>>
>>>     if (check_range(info, min_uV, max_uV)) {
>>> @@ -283,7 +283,7 @@ static int da9034_set_ldo12_voltage(struct
>>> regulator_dev *rdev,
>>>  static int da9034_get_ldo12_voltage(struct regulator_dev *rdev)
>>>  {
>>>     struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
>>> -    struct device *da9034_dev = rdev_get_dev(rdev)->parent;
>>> +    struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
>>>     uint8_t val, mask;
>>>     int ret;
>>>
>>> @@ -466,7 +466,7 @@ static int __devinit da903x_regulator_probe(struct
>>> platform_device *pdev)
>>>     if (ri->desc.id == DA9030_ID_LDO1 || ri->desc.id == DA9030_ID_LDO15)
>>>         ri->desc.ops = &da9030_regulator_ldo1_15_ops;
>>>
>>> -    rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
>>> +    rdev = regulator_register(&ri->desc, &pdev->dev, ri);
>>>     if (IS_ERR(rdev)) {
>>>         dev_err(&pdev->dev, "failed to register regulator %s\n",
>>>                 ri->desc.name);
>>>
>>>
>>>
>>>
>>>
>>
>>
>> --
>> Cheers
>> - eric
>>
> 
> 
> 

Re: [PATCH] da903x regulator bug fix

[Jonathan Cameron]

Changes the device registration part of the probe function to supply the
regulator device rather than its parent (the mfd device) as this caused
problems when the regulator core attempted to find constraints associated
with the regulators.

Signed-of-by: Jonathan Cameron <jic23@cam.ac.uk>

--
Version 2 applying Eric Miao's suggestion about using an inline function.
Change made in couple of additional places I'd missed the first time.


diff --git a/drivers/regulator/da903x.c b/drivers/regulator/da903x.c
index 3688e33..773b29c 100644
--- a/drivers/regulator/da903x.c
+++ b/drivers/regulator/da903x.c
@@ -79,6 +79,11 @@ struct da903x_regulator_info {
 	int	enable_bit;
 };
 
+static inline struct device *to_da903x_dev(struct regulator_dev *rdev)
+{
+	return rdev_get_dev(rdev)->parent->parent;
+}
+
 static inline int check_range(struct da903x_regulator_info *info,
 				int min_uV, int max_uV)
 {
@@ -93,7 +98,7 @@ static int da903x_set_ldo_voltage(struct regulator_dev *rdev,
 				  int min_uV, int max_uV)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 
 	if (check_range(info, min_uV, max_uV)) {
@@ -111,7 +116,7 @@ static int da903x_set_ldo_voltage(struct regulator_dev *rdev,
 static int da903x_get_voltage(struct regulator_dev *rdev)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 	int ret;
 
@@ -128,7 +133,7 @@ static int da903x_get_voltage(struct regulator_dev *rdev)
 static int da903x_enable(struct regulator_dev *rdev)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 
 	return da903x_set_bits(da9034_dev, info->enable_reg,
 					1 << info->enable_bit);
@@ -137,7 +142,7 @@ static int da903x_enable(struct regulator_dev *rdev)
 static int da903x_disable(struct regulator_dev *rdev)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 
 	return da903x_clr_bits(da9034_dev, info->enable_reg,
 					1 << info->enable_bit);
@@ -146,7 +151,7 @@ static int da903x_disable(struct regulator_dev *rdev)
 static int da903x_is_enabled(struct regulator_dev *rdev)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 	uint8_t reg_val;
 	int ret;
 
@@ -162,7 +167,7 @@ static int da9030_set_ldo1_15_voltage(struct regulator_dev *rdev,
 				       int min_uV, int max_uV)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da903x_dev = rdev_get_dev(rdev)->parent;
+	struct device *da903x_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 	int ret;
 
@@ -189,7 +194,7 @@ static int da9030_set_ldo14_voltage(struct regulator_dev *rdev,
 				  int min_uV, int max_uV)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da903x_dev = rdev_get_dev(rdev)->parent;
+	struct device *da903x_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 	int thresh;
 
@@ -215,7 +220,7 @@ static int da9030_set_ldo14_voltage(struct regulator_dev *rdev,
 static int da9030_get_ldo14_voltage(struct regulator_dev *rdev)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da903x_dev = rdev_get_dev(rdev)->parent;
+	struct device *da903x_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 	int ret;
 
@@ -238,7 +243,7 @@ static int da9034_set_dvc_voltage(struct regulator_dev *rdev,
 				  int min_uV, int max_uV)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 	int ret;
 
@@ -264,7 +269,7 @@ static int da9034_set_ldo12_voltage(struct regulator_dev *rdev,
 				    int min_uV, int max_uV)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 
 	if (check_range(info, min_uV, max_uV)) {
@@ -283,7 +288,7 @@ static int da9034_set_ldo12_voltage(struct regulator_dev *rdev,
 static int da9034_get_ldo12_voltage(struct regulator_dev *rdev)
 {
 	struct da903x_regulator_info *info = rdev_get_drvdata(rdev);
-	struct device *da9034_dev = rdev_get_dev(rdev)->parent;
+	struct device *da9034_dev = to_da903x_dev(rdev);
 	uint8_t val, mask;
 	int ret;
 
@@ -466,7 +471,7 @@ static int __devinit da903x_regulator_probe(struct platform_device *pdev)
 	if (ri->desc.id == DA9030_ID_LDO1 || ri->desc.id == DA9030_ID_LDO15)
 		ri->desc.ops = &da9030_regulator_ldo1_15_ops;
 
-	rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
+	rdev = regulator_register(&ri->desc, &pdev->dev, ri);
 	if (IS_ERR(rdev)) {
 		dev_err(&pdev->dev, "failed to register regulator %s\n",
 				ri->desc.name);

Re: [PATCH] da903x regulator bug fix

[Liam Girdwood]

On Tue, 2008-10-28 at 11:03 +0000, Jonathan Cameron wrote:
> Changes the device registration part of the probe function to supply the
> regulator device rather than its parent (the mfd device) as this caused
> problems when the regulator core attempted to find constraints associated
> with the regulators.
> 
> Signed-of-by: Jonathan Cameron <jic23@cam.ac.uk>

Thanks, this is more preferable than the first patch. I would ultimately
prefer this regulator to be more like wm8350 in terms of mfd
registration but we are in rc2 so this will have to wait.

Eric, would you like to Ack ?

Thanks

Liam

Re: [PATCH] da903x regulator bug fix

[Eric Miao]

On Tue, Oct 28, 2008 at 7:19 PM, Liam Girdwood <lrg@kernel.org> wrote:
> On Tue, 2008-10-28 at 11:03 +0000, Jonathan Cameron wrote:
>> Changes the device registration part of the probe function to supply the
>> regulator device rather than its parent (the mfd device) as this caused
>> problems when the regulator core attempted to find constraints associated
>> with the regulators.
>>
>> Signed-of-by: Jonathan Cameron <jic23@cam.ac.uk>
>
> Thanks, this is more preferable than the first patch. I would ultimately
> prefer this regulator to be more like wm8350 in terms of mfd
> registration but we are in rc2 so this will have to wait.
>
> Eric, would you like to Ack ?
>

Yeah, I'm fine with this.

Acked-by: Eric Miao <eric.miao@marvell.com>

> Thanks
>
> Liam
>
>
>



-- 
Cheers
- eric