Da903x regulator driver. Bug?

PATCH In review Jonathan Cameron 5 participants 12 messages Oct 24, 2008 – Oct 28, 2008 Voltage and current regulator framework

lore

Share & tools

lore

Jonathan Cameron

author

2008-10-24 16:23 UTC #0 lore raw

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

Jonathan Cameron

2008-10-24 17:23 UTC #1 lore raw

Quoted 40 lines · 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.

Quoted 10 lines

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

Liam Girdwood

2008-10-24 18:24 UTC #2 lore raw

Quoted 2 lines · Jonathan Cameron

Quoted 1 line · Jonathan Cameron
Firstly, is lkml the right place to ask questions about regulator drivers?

Yes.

Quoted 4 lines

Quoted 4 lines
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.

Quoted 35 lines

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.

Quoted 8 lines

Quoted 8 lines
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.

Quoted 7 lines

Quoted 2 lines
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

Quoted 8 lines

    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

Jonathan Cameron

author

2008-10-24 18:53 UTC #3 lore raw

Quoted 42 lines

I'm not sure if this is a valid config for this board. Eric will probably know for sure.
Quoted 38 lines
Quoted 38 lines
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

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
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 ;)

Quoted 8 lines

Quoted 8 lines
Quoted 1 line
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;

Quoted 8 lines

Quoted 6 lines
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.

Quoted 5 lines

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

Mark Brown

2008-10-24 19:44 UTC #4 lore raw

Quoted 10 lines · Jonathan Cameron

Quoted 9 lines
Quoted 9 lines
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.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
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.

Jonathan Cameron

2008-10-27 15:28 UTC #5 lore raw

PATCH 1/1 da903x regulator bug fix

In reply to 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 <[email protected]>

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

drivers/regulator/da903x.c +0 -0

index 3688e33..f928aa1 100644
@@ -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);

Eric Miao

2008-10-28 01:02 UTC #6 lore raw

Re: [PATCH] da903x regulator bug fix

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

Quoted 109 lines · Jonathan Cameron

Signed-of-by Jonathan Cameron <[email protected]>

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

drivers/regulator/da903x.c +1 -1

index 3688e33..f928aa1 100644
@@ -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)) {

drivers/regulator/da903x.c +2 -2

@@ -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);

drivers/regulator/da903x.c +4 -4

@@ -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)) {

drivers/regulator/da903x.c +2 -2

@@ -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);

Signature · 2 lines

Cheers
- eric

Eric Miao

2008-10-28 01:03 UTC #7 lore raw

Re: [PATCH] da903x regulator bug fix

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

Quoted 118 lines

Quoted 103 lines · Jonathan Cameron

Signed-of-by Jonathan Cameron <[email protected]>

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

drivers/regulator/da903x.c +1 -1

index 3688e33..f928aa1 100644
@@ -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)) {

drivers/regulator/da903x.c +2 -2

@@ -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);

drivers/regulator/da903x.c +4 -4

@@ -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)) {

drivers/regulator/da903x.c +2 -2

@@ -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",

Quoted 6 lines

                ri->desc.name);

--
Cheers
- eric

Signature · 2 lines

Cheers
- eric

Jonathan Cameron

author

2008-10-28 10:40 UTC #8 lore raw

Re: [PATCH] da903x regulator bug fix

Good idea, patch to follow.

Jonathan

Quoted 126 lines

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

Quoted 117 lines

Quoted 103 lines · Jonathan Cameron

Signed-of-by Jonathan Cameron <[email protected]>

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

drivers/regulator/da903x.c +1 -1

index 3688e33..f928aa1 100644
@@ -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)) {

drivers/regulator/da903x.c +2 -2

@@ -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);

drivers/regulator/da903x.c +4 -4

@@ -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)) {

drivers/regulator/da903x.c +2 -2

@@ -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",

Quoted 6 lines

                ri->desc.name);

--
Cheers
- eric

Jonathan Cameron

author

2008-10-28 11:03 UTC #9 lore raw

Re: [PATCH] da903x regulator bug fix

In reply to Eric Miao

Signed-of-by Jonathan Cameron <[email protected]>

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

drivers/regulator/da903x.c +17 ~~-12~~

index 3688e33..773b29c 100644
@@ -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);

Liam Girdwood

2008-10-28 11:19 UTC #10 lore raw

Re: [PATCH] da903x regulator bug fix

Quoted 6 lines · 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 <[email protected]>

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

Eric Miao

Acked-by

2008-10-28 12:22 UTC #11 lore raw

Re: [PATCH] da903x regulator bug fix

Quoted 6 lines · Liam Girdwood

Quoted 5 lines · 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.

Quoted 8 lines

Quoted 1 line
Signed-of-by Jonathan Cameron <[email protected]>
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 <[email protected]>

Quoted 6 lines

Thanks
Liam

Signature · 2 lines

Cheers
- eric