Check name in initialization of max8925 regulator driver.
Signed-off-by: Haojian Zhuang <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
---
drivers/regulator/max8925-regulator.c | 39 ++++++++++++++++-----------------
1 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/regulator/max8925-regulator.c b/drivers/regulator/max8925-regulator.c
index e4dbd66..7ce8147 100644
--- a/drivers/regulator/max8925-regulator.c
+++ b/drivers/regulator/max8925-regulator.c
@@ -174,7 +174,7 @@ static struct regulator_ops max8925_regulator_ldo_ops = {
#define MAX8925_SDV(_id, min, max, step) \
{ \
.desc = { \
- .name = "SDV" #_id, \
+ .name = "SD" #_id, \
.ops = &max8925_regulator_sdv_ops, \
.type = REGULATOR_VOLTAGE, \
.id = MAX8925_ID_SD##_id, \
@@ -236,39 +236,38 @@ static struct max8925_regulator_info max8925_regulator_info[] = {
MAX8925_LDO(20, 750, 3900, 50),
};
-static struct max8925_regulator_info * __devinit find_regulator_info(int id)
+static int __devinit max8925_regulator_probe(struct platform_device *pdev)
{
- struct max8925_regulator_info *ri;
+ struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ struct max8925_regulator_info *ri = NULL;
+ struct regulator_init_data *pdata;
+ struct regulator_dev *rdev;
int i;
+ pdata = pdev->dev.platform_data;
+ if ((pdata == NULL) || (pdata->driver_data == NULL))
+ return -EINVAL;
+
for (i = 0; i < ARRAY_SIZE(max8925_regulator_info); i++) {
ri = &max8925_regulator_info[i];
- if (ri->desc.id == id)
- return ri;
+ /* pdata->driver_data stores the name of regulator */
+ if (!strcmp(ri->desc.name, pdata->driver_data))
+ break;
}
- return NULL;
-}
-
-static int __devinit max8925_regulator_probe(struct platform_device *pdev)
-{
- struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
- struct max8925_platform_data *pdata = chip->dev->platform_data;
- struct max8925_regulator_info *ri;
- struct regulator_dev *rdev;
-
- ri = find_regulator_info(pdev->id);
- if (ri == NULL) {
- dev_err(&pdev->dev, "invalid regulator ID specified\n");
+ if (i > ARRAY_SIZE(max8925_regulator_info)) {
+ dev_err(&pdev->dev, "Failed to find regulator %s\n",
+ pdata->constraints.name);
return -EINVAL;
}
ri->i2c = chip->i2c;
ri->chip = chip;
+ /* replace driver_data with ri */
rdev = regulator_register(&ri->desc, &pdev->dev,
- pdata->regulator[pdev->id], ri);
+ pdata, ri);
if (IS_ERR(rdev)) {
dev_err(&pdev->dev, "failed to register regulator %s\n",
- ri->desc.name);
+ ri->desc.name);
return PTR_ERR(rdev);
}
--
1.5.6.5
On Mon, Apr 18, 2011 at 10:04:10PM +0800, Haojian Zhuang wrote:
> Check name in initialization of max8925 regulator driver.
What name are we checking and why do we need to check it? I've no idea
what the patch is supposed to do which makes it hard to review.
> +++ b/drivers/regulator/max8925-regulator.c
> @@ -174,7 +174,7 @@ static struct regulator_ops max8925_regulator_ldo_ops = {
> #define MAX8925_SDV(_id, min, max, step) \
> { \
> .desc = { \
> - .name = "SDV" #_id, \
> + .name = "SD" #_id, \
The above isn't obviously correct - it changes the name that's assigned
from matching the macro used to something different.
On Mon, Apr 18, 2011 at 10:28 PM, Mark Brown
<[email protected]> wrote:
> On Mon, Apr 18, 2011 at 10:04:10PM +0800, Haojian Zhuang wrote:
>> Check name in initialization of max8925 regulator driver.
>
> What name are we checking and why do we need to check it? ?I've no idea
> what the patch is supposed to do which makes it hard to review.
>
Actually, I didn't submit any platform driver before. So I can't
attach the related patch on
platform driver. After 2.6.39, I'll upload patches on platform driver.
In this original design, regulator data is assigned in platform driver
separately with index.
If I missed to define regulator[0] in platform data, the regulator
driver will meet failure
because of checking in max8925-core.c. The regulator[0] always means Buck0.
So I want to avoid to use the index and check regulator[] one by one.
I use a pointer to link
all regulator data together. I just need to check whether the
regulator pointer is valid or not.
>> +++ b/drivers/regulator/max8925-regulator.c
>> @@ -174,7 +174,7 @@ static struct regulator_ops max8925_regulator_ldo_ops = {
>> ?#define MAX8925_SDV(_id, min, max, step) ? ? ? ? ? ? ? ? ? ? \
>> ?{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> ? ? ? .desc ? = { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>> - ? ? ? ? ? ? .name ? = "SDV" #_id, ? ? ? ? ? ? ? ? ? ? ? ? ? \
>> + ? ? ? ? ? ? .name ? = "SD" #_id, ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>
> The above isn't obviously correct - it changes the name that's assigned
> from matching the macro used to something different.
>
The name isn't used by others. So I change it to SDx in order to
compare regulator name.
On Mon, Apr 18, 2011 at 11:33:34PM +0800, Haojian Zhuang wrote:
> So I want to avoid to use the index and check regulator[] one by one.
> I use a pointer to link
> all regulator data together. I just need to check whether the
> regulator pointer is valid or not.
Why? I don't understand what the goal of this change is.
>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011??4??19?? 12:04 AM
>To: Haojian Zhuang
>Cc: Haojian Zhuang; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]
>Subject: Re: [PATCH 13/14] regulator: check name in initialization of
>max8925
>
>On Mon, Apr 18, 2011 at 11:33:34PM +0800, Haojian Zhuang wrote:
>
>> So I want to avoid to use the index and check regulator[] one by one.
>> I use a pointer to link
>> all regulator data together. I just need to check whether the
>> regulator pointer is valid or not.
>
>Why? I don't understand what the goal of this change is.
Original implementation:
Max8925_core.c:
If ((pdata == NULL) || (pdata->regulator[0] == NULL))
Return -EINVAL;
Machine driver:
Platform data:
.regulator[0] = xxx
.regulator[1] = xxx
The index of regulator array is id of buck and ldo.
There's issue in max8925_core.c since we can't assume regulator[0] always declared in machine driver.
Current implementation:
Max8925_core.c:
If ((pdata == NULL) || (pdata->regulator == NULL))
Return -EINVAL;
Machine driver:
Platform data:
Use two parameters: num_regulators and regulator pointer. The index of new regulator array isn't id of buck and ldo any more.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, Apr 18, 2011 at 07:42:09PM -0700, Haojian Zhuang wrote:
> Machine driver:
> Platform data:
> .regulator[0] = xxx
> .regulator[1] = xxx
> The index of regulator array is id of buck and ldo.
> There's issue in max8925_core.c since we can't assume regulator[0] always declared in machine driver.
What is the issue? It's trivial to skip the regulator if the data is
null.
>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011??4??19?? 4:09 PM
>To: Haojian Zhuang
>Cc: Haojian Zhuang; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]
>Subject: Re: [PATCH 13/14] regulator: check name in initialization of
>max8925
>
>On Mon, Apr 18, 2011 at 07:42:09PM -0700, Haojian Zhuang wrote:
>> Machine driver:
>> Platform data:
>> .regulator[0] = xxx
>> .regulator[1] = xxx
>
>> The index of regulator array is id of buck and ldo.
>> There's issue in max8925_core.c since we can't assume regulator[0]
>always declared in machine driver.
>
>What is the issue? It's trivial to skip the regulator if the data is
>null.
It skiped all regulators if regulator[0] isn't decleared.
(pdata == NULL) || (pdata->regulator[0] == NULL)
So it's a bug.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tue, Apr 19, 2011 at 01:28:23AM -0700, Haojian Zhuang wrote:
> >> There's issue in max8925_core.c since we can't assume regulator[0]
> >always declared in machine driver.
> >What is the issue? It's trivial to skip the regulator if the data is
> >null.
> It skiped all regulators if regulator[0] isn't decleared.
> (pdata == NULL) || (pdata->regulator[0] == NULL)
> So it's a bug.
Surely the obvious fix is to fix the issue and not skip all the
regulators if an individual regulator has no data?
>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011??4??19?? 6:51 PM
>To: Haojian Zhuang
>Cc: Haojian Zhuang; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]
>Subject: Re: [PATCH 13/14] regulator: check name in initialization of
>max8925
>
>On Tue, Apr 19, 2011 at 01:28:23AM -0700, Haojian Zhuang wrote:
>
>> >> There's issue in max8925_core.c since we can't assume regulator[0]
>> >always declared in machine driver.
>
>> >What is the issue? It's trivial to skip the regulator if the data is
>> >null.
>
>> It skiped all regulators if regulator[0] isn't decleared.
>> (pdata == NULL) || (pdata->regulator[0] == NULL)
>
>> So it's a bug.
>
>Surely the obvious fix is to fix the issue and not skip all the
>regulators if an individual regulator has no data?
Yes.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
If regulator[0] is missed in init data, all regulators of max8925 won't
be initialized.
Signed-off-by: Haojian Zhuang <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
---
drivers/mfd/max8925-core.c | 171 ++++++++++++++++++---------------
drivers/regulator/max8925-regulator.c | 37 ++++----
include/linux/mfd/max8925.h | 3 +-
3 files changed, 112 insertions(+), 99 deletions(-)
diff --git a/drivers/mfd/max8925-core.c b/drivers/mfd/max8925-core.c
index a974711..93f2731 100644
--- a/drivers/mfd/max8925-core.c
+++ b/drivers/mfd/max8925-core.c
@@ -75,6 +75,32 @@ static struct resource onkey_resources[] __devinitdata = {
IORESOURCE_IRQ,},
};
+static struct resource regulator_resources[] __devinitdata = {
+ {MAX8925_ID_SD1, MAX8925_ID_SD1, "SD1", IORESOURCE_IO,},
+ {MAX8925_ID_SD2, MAX8925_ID_SD2, "SD2", IORESOURCE_IO,},
+ {MAX8925_ID_SD3, MAX8925_ID_SD3, "SD3", IORESOURCE_IO,},
+ {MAX8925_ID_LDO1, MAX8925_ID_LDO1, "LDO01", IORESOURCE_IO,},
+ {MAX8925_ID_LDO2, MAX8925_ID_LDO2, "LDO02", IORESOURCE_IO,},
+ {MAX8925_ID_LDO3, MAX8925_ID_LDO3, "LDO03", IORESOURCE_IO,},
+ {MAX8925_ID_LDO4, MAX8925_ID_LDO4, "LDO04", IORESOURCE_IO,},
+ {MAX8925_ID_LDO5, MAX8925_ID_LDO5, "LDO05", IORESOURCE_IO,},
+ {MAX8925_ID_LDO6, MAX8925_ID_LDO6, "LDO06", IORESOURCE_IO,},
+ {MAX8925_ID_LDO7, MAX8925_ID_LDO7, "LDO07", IORESOURCE_IO,},
+ {MAX8925_ID_LDO8, MAX8925_ID_LDO8, "LDO08", IORESOURCE_IO,},
+ {MAX8925_ID_LDO9, MAX8925_ID_LDO9, "LDO09", IORESOURCE_IO,},
+ {MAX8925_ID_LDO10, MAX8925_ID_LDO10, "LDO10", IORESOURCE_IO,},
+ {MAX8925_ID_LDO11, MAX8925_ID_LDO11, "LDO11", IORESOURCE_IO,},
+ {MAX8925_ID_LDO12, MAX8925_ID_LDO12, "LDO12", IORESOURCE_IO,},
+ {MAX8925_ID_LDO13, MAX8925_ID_LDO13, "LDO13", IORESOURCE_IO,},
+ {MAX8925_ID_LDO14, MAX8925_ID_LDO14, "LDO14", IORESOURCE_IO,},
+ {MAX8925_ID_LDO15, MAX8925_ID_LDO15, "LDO15", IORESOURCE_IO,},
+ {MAX8925_ID_LDO16, MAX8925_ID_LDO16, "LDO16", IORESOURCE_IO,},
+ {MAX8925_ID_LDO17, MAX8925_ID_LDO17, "LDO17", IORESOURCE_IO,},
+ {MAX8925_ID_LDO18, MAX8925_ID_LDO18, "LDO18", IORESOURCE_IO,},
+ {MAX8925_ID_LDO19, MAX8925_ID_LDO19, "LDO19", IORESOURCE_IO,},
+ {MAX8925_ID_LDO20, MAX8925_ID_LDO20, "LDO20", IORESOURCE_IO,},
+};
+
static struct mfd_cell bk_devs[] = {
{"max8925-backlight", -1,},
};
@@ -95,76 +121,36 @@ static struct mfd_cell onkey_devs[] = {
{"max8925-onkey", -1,},
};
+static struct mfd_cell regulator_devs[] = {
+ {"max8925-regulator", 0,},
+ {"max8925-regulator", 1,},
+ {"max8925-regulator", 2,},
+ {"max8925-regulator", 3,},
+ {"max8925-regulator", 4,},
+ {"max8925-regulator", 5,},
+ {"max8925-regulator", 6,},
+ {"max8925-regulator", 7,},
+ {"max8925-regulator", 8,},
+ {"max8925-regulator", 9,},
+ {"max8925-regulator", 10,},
+ {"max8925-regulator", 11,},
+ {"max8925-regulator", 12,},
+ {"max8925-regulator", 13,},
+ {"max8925-regulator", 14,},
+ {"max8925-regulator", 15,},
+ {"max8925-regulator", 16,},
+ {"max8925-regulator", 17,},
+ {"max8925-regulator", 18,},
+ {"max8925-regulator", 19,},
+ {"max8925-regulator", 20,},
+ {"max8925-regulator", 21,},
+ {"max8925-regulator", 22,},
+};
+
static struct max8925_backlight_pdata bk_pdata;
static struct max8925_touch_pdata touch_pdata;
static struct max8925_power_pdata power_pdata;
-
-#define MAX8925_REG_RESOURCE(_start, _end) \
-{ \
- .start = MAX8925_##_start, \
- .end = MAX8925_##_end, \
- .flags = IORESOURCE_IO, \
-}
-
-static struct resource regulator_resources[] = {
- MAX8925_REG_RESOURCE(SDCTL1, SDCTL1),
- MAX8925_REG_RESOURCE(SDCTL2, SDCTL2),
- MAX8925_REG_RESOURCE(SDCTL3, SDCTL3),
- MAX8925_REG_RESOURCE(LDOCTL1, LDOCTL1),
- MAX8925_REG_RESOURCE(LDOCTL2, LDOCTL2),
- MAX8925_REG_RESOURCE(LDOCTL3, LDOCTL3),
- MAX8925_REG_RESOURCE(LDOCTL4, LDOCTL4),
- MAX8925_REG_RESOURCE(LDOCTL5, LDOCTL5),
- MAX8925_REG_RESOURCE(LDOCTL6, LDOCTL6),
- MAX8925_REG_RESOURCE(LDOCTL7, LDOCTL7),
- MAX8925_REG_RESOURCE(LDOCTL8, LDOCTL8),
- MAX8925_REG_RESOURCE(LDOCTL9, LDOCTL9),
- MAX8925_REG_RESOURCE(LDOCTL10, LDOCTL10),
- MAX8925_REG_RESOURCE(LDOCTL11, LDOCTL11),
- MAX8925_REG_RESOURCE(LDOCTL12, LDOCTL12),
- MAX8925_REG_RESOURCE(LDOCTL13, LDOCTL13),
- MAX8925_REG_RESOURCE(LDOCTL14, LDOCTL14),
- MAX8925_REG_RESOURCE(LDOCTL15, LDOCTL15),
- MAX8925_REG_RESOURCE(LDOCTL16, LDOCTL16),
- MAX8925_REG_RESOURCE(LDOCTL17, LDOCTL17),
- MAX8925_REG_RESOURCE(LDOCTL18, LDOCTL18),
- MAX8925_REG_RESOURCE(LDOCTL19, LDOCTL19),
- MAX8925_REG_RESOURCE(LDOCTL20, LDOCTL20),
-};
-
-#define MAX8925_REG_DEVS(_id) \
-{ \
- .name = "max8925-regulator", \
- .num_resources = 1, \
- .resources = ®ulator_resources[MAX8925_ID_##_id], \
- .id = MAX8925_ID_##_id, \
-}
-
-static struct mfd_cell regulator_devs[] = {
- MAX8925_REG_DEVS(SD1),
- MAX8925_REG_DEVS(SD2),
- MAX8925_REG_DEVS(SD3),
- MAX8925_REG_DEVS(LDO1),
- MAX8925_REG_DEVS(LDO2),
- MAX8925_REG_DEVS(LDO3),
- MAX8925_REG_DEVS(LDO4),
- MAX8925_REG_DEVS(LDO5),
- MAX8925_REG_DEVS(LDO6),
- MAX8925_REG_DEVS(LDO7),
- MAX8925_REG_DEVS(LDO8),
- MAX8925_REG_DEVS(LDO9),
- MAX8925_REG_DEVS(LDO10),
- MAX8925_REG_DEVS(LDO11),
- MAX8925_REG_DEVS(LDO12),
- MAX8925_REG_DEVS(LDO13),
- MAX8925_REG_DEVS(LDO14),
- MAX8925_REG_DEVS(LDO15),
- MAX8925_REG_DEVS(LDO16),
- MAX8925_REG_DEVS(LDO17),
- MAX8925_REG_DEVS(LDO18),
- MAX8925_REG_DEVS(LDO19),
- MAX8925_REG_DEVS(LDO20),
-};
+static struct regulator_init_data regulator_pdata[ARRAY_SIZE(regulator_devs)];
enum {
FLAGS_ADC = 1, /* register in ADC component */
@@ -669,6 +655,45 @@ static void __devinit device_onkey_init(struct max8925_chip *chip,
dev_err(chip->dev, "Failed to add onkey subdev\n");
}
+static void __devinit device_regulator_init(struct max8925_chip *chip,
+ struct i2c_client *i2c,
+ struct max8925_platform_data *pdata)
+{
+ struct regulator_init_data *initdata;
+ int ret, i, seq;
+
+ if ((pdata == NULL) || (pdata->regulator == NULL))
+ return;
+
+ if (pdata->num_regulators > ARRAY_SIZE(regulator_devs))
+ pdata->num_regulators = ARRAY_SIZE(regulator_devs);
+
+ for (i = 0, seq = -1; i < pdata->num_regulators; i++) {
+ initdata = &pdata->regulator[i];
+ seq = *(unsigned int *)initdata->driver_data;
+ if ((seq < 0) || (seq > MAX8925_ID_MAX)) {
+ dev_err(chip->dev, "Wrong ID(%d) on regulator(%s)\n",
+ seq, initdata->constraints.name);
+ goto out;
+ }
+ memcpy(®ulator_pdata[i], &pdata->regulator[i],
+ sizeof(struct regulator_init_data));
+ regulator_devs[i].platform_data = ®ulator_pdata[i];
+ regulator_devs[i].pdata_size = sizeof(regulator_pdata[i]);
+ regulator_devs[i].num_resources = 1;
+ regulator_devs[i].resources = ®ulator_resources[seq];
+
+ ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[i], 1,
+ ®ulator_resources[seq], 0);
+ if (ret < 0) {
+ dev_err(chip->dev, "Failed to add regulator subdev\n");
+ goto out;
+ }
+ }
+out:
+ return;
+}
+
int __devinit max8925_device_init(struct max8925_chip *chip,
struct max8925_platform_data *pdata)
{
@@ -697,18 +722,8 @@ int __devinit max8925_device_init(struct max8925_chip *chip,
device_touch_init(chip, chip->adc, pdata);
device_power_init(chip, chip->adc, pdata);
device_onkey_init(chip, chip->i2c, pdata);
+ device_regulator_init(chip, chip->i2c, pdata);
- if (pdata && pdata->regulator[0]) {
- ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[0],
- ARRAY_SIZE(regulator_devs),
- ®ulator_resources[0], 0);
- if (ret < 0) {
- dev_err(chip->dev, "Failed to add regulator subdev\n");
- goto out_dev;
- }
- }
-
-out_dev:
return 0;
}
diff --git a/drivers/regulator/max8925-regulator.c b/drivers/regulator/max8925-regulator.c
index e4dbd66..733dcd2 100644
--- a/drivers/regulator/max8925-regulator.c
+++ b/drivers/regulator/max8925-regulator.c
@@ -236,39 +236,36 @@ static struct max8925_regulator_info max8925_regulator_info[] = {
MAX8925_LDO(20, 750, 3900, 50),
};
-static struct max8925_regulator_info * __devinit find_regulator_info(int id)
-{
- struct max8925_regulator_info *ri;
- int i;
-
- for (i = 0; i < ARRAY_SIZE(max8925_regulator_info); i++) {
- ri = &max8925_regulator_info[i];
- if (ri->desc.id == id)
- return ri;
- }
- return NULL;
-}
-
static int __devinit max8925_regulator_probe(struct platform_device *pdev)
{
struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
- struct max8925_platform_data *pdata = chip->dev->platform_data;
- struct max8925_regulator_info *ri;
+ struct max8925_regulator_info *ri = NULL;
+ struct regulator_init_data *pdata = pdev->dev.platform_data;
struct regulator_dev *rdev;
+ struct resource *res;
+ int i;
- ri = find_regulator_info(pdev->id);
- if (ri == NULL) {
- dev_err(&pdev->dev, "invalid regulator ID specified\n");
+ res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ if (res == NULL) {
+ dev_err(&pdev->dev, "No I/O resource!\n");
+ return -EINVAL;
+ }
+ i = res->start;
+ if ((i < 0) || (i > MAX8925_ID_MAX)) {
+ dev_err(&pdev->dev, "Failed to find regulator %d\n",
+ res->start);
return -EINVAL;
}
+ ri = &max8925_regulator_info[i];
ri->i2c = chip->i2c;
ri->chip = chip;
+ /* replace driver_data with ri */
rdev = regulator_register(&ri->desc, &pdev->dev,
- pdata->regulator[pdev->id], ri);
+ pdata, ri);
if (IS_ERR(rdev)) {
dev_err(&pdev->dev, "failed to register regulator %s\n",
- ri->desc.name);
+ ri->desc.name);
return PTR_ERR(rdev);
}
diff --git a/include/linux/mfd/max8925.h b/include/linux/mfd/max8925.h
index 5259dfe..438a734 100644
--- a/include/linux/mfd/max8925.h
+++ b/include/linux/mfd/max8925.h
@@ -233,10 +233,11 @@ struct max8925_platform_data {
struct max8925_backlight_pdata *backlight;
struct max8925_touch_pdata *touch;
struct max8925_power_pdata *power;
- struct regulator_init_data *regulator[MAX8925_MAX_REGULATOR];
+ struct regulator_init_data *regulator;
int irq_base;
int tsc_irq;
+ int num_regulators;
};
extern int max8925_reg_read(struct i2c_client *, int);
--
1.5.6.5
On Wed, Apr 20, 2011 at 07:55:39PM +0800, Haojian Zhuang wrote:
> If regulator[0] is missed in init data, all regulators of max8925 won't
> be initialized.
With a changelog like this I'd expect a small change to an error check
in the startup code or something but this is a very big change to the
driver initialisation.
> +static struct regulator_init_data regulator_pdata[ARRAY_SIZE(regulator_devs)];
That looks really suspicious, what happens if there's two of these
devices in the system?
> + memcpy(®ulator_pdata[i], &pdata->regulator[i],
> + sizeof(struct regulator_init_data));
> + regulator_devs[i].platform_data = ®ulator_pdata[i];
> + regulator_devs[i].pdata_size = sizeof(regulator_pdata[i]);
> + regulator_devs[i].num_resources = 1;
> + regulator_devs[i].resources = ®ulator_resources[seq];
> +
> + ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[i], 1,
> + ®ulator_resources[seq], 0);
> + if (ret < 0) {
> + dev_err(chip->dev, "Failed to add regulator subdev\n");
> + goto out;
It's really unclear why the array is needed at all if you're registering
the devices one at a time.
> - if (pdata && pdata->regulator[0]) {
> - ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[0],
> - ARRAY_SIZE(regulator_devs),
> - ®ulator_resources[0], 0);
> - if (ret < 0) {
> - dev_err(chip->dev, "Failed to add regulator subdev\n");
> - goto out_dev;
> - }
Surely the only change that's needed here is to remove the check to see
if pdata->regulator[0] is non-null?
>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011??4??21?? 6:42 PM
>To: Haojian Zhuang
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]
>Subject: Re: [PATCH 13/14] regulator: max8925: fix not add device if
>missing init data
>
>On Wed, Apr 20, 2011 at 07:55:39PM +0800, Haojian Zhuang wrote:
>> If regulator[0] is missed in init data, all regulators of max8925
>won't
>> be initialized.
>
>With a changelog like this I'd expect a small change to an error check
>in the startup code or something but this is a very big change to the
>driver initialisation.
>
>> +static struct regulator_init_data
>regulator_pdata[ARRAY_SIZE(regulator_devs)];
>
>That looks really suspicious, what happens if there's two of these
>devices in the system?
>
It's impossible to use two PMIC in one system. At least, I didn't hear.
>> + memcpy(®ulator_pdata[i], &pdata->regulator[i],
>> + sizeof(struct regulator_init_data));
>> + regulator_devs[i].platform_data = ®ulator_pdata[i];
>> + regulator_devs[i].pdata_size = sizeof(regulator_pdata[i]);
>> + regulator_devs[i].num_resources = 1;
>> + regulator_devs[i].resources = ®ulator_resources[seq];
>> +
>> + ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[i], 1,
>> + ®ulator_resources[seq], 0);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "Failed to add regulator subdev\n");
>> + goto out;
>
>It's really unclear why the array is needed at all if you're registering
>the devices one at a time.
>
Although there're a lot of regulators, maybe only parts of them are used.
So only add the necessary one. Others will be passed. The number is got
from platform data.
>> - if (pdata && pdata->regulator[0]) {
>> - ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[0],
>> - ARRAY_SIZE(regulator_devs),
>> - ®ulator_resources[0], 0);
>> - if (ret < 0) {
>> - dev_err(chip->dev, "Failed to add regulator subdev\n");
>> - goto out_dev;
>> - }
>
>Surely the only change that's needed here is to remove the check to see
>if pdata->regulator[0] is non-null?
All above changes are necessary to me.
Thanks
Haojian
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, Apr 21, 2011 at 04:13:28AM -0700, Haojian Zhuang wrote:
> >> +static struct regulator_init_data
> >regulator_pdata[ARRAY_SIZE(regulator_devs)];
> >That looks really suspicious, what happens if there's two of these
> >devices in the system?
> It's impossible to use two PMIC in one system. At least, I didn't hear.
I don't believe that. It's not going to be a common case but it will be
possible.
> >It's really unclear why the array is needed at all if you're registering
> >the devices one at a time.
> Although there're a lot of regulators, maybe only parts of them are used.
> So only add the necessary one. Others will be passed. The number is got
> from platform data.
You're missing the point here. You're registering a device at a time
but for some reason the platform data is being stored in an array in
order to pass it in, even though only a single element in the array is
in use.
> >> - if (pdata && pdata->regulator[0]) {
> >> - ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[0],
> >> - ARRAY_SIZE(regulator_devs),
> >> - ®ulator_resources[0], 0);
> >> - if (ret < 0) {
> >> - dev_err(chip->dev, "Failed to add regulator subdev\n");
> >> - goto out_dev;
> >> - }
> >Surely the only change that's needed here is to remove the check to see
> >if pdata->regulator[0] is non-null?
> All above changes are necessary to me.
What makes you say this? If you remove the check for the first entry
being null then the driver will go and register all the regulators which
seems fine. That seems like it fixes the problem with skipping all the
regulators if the first one has no platform data.
You're really not explaining the changes you're trying to make here
terribly well, the changes you're making seem very substantial relative
to what you say you're trying to do.
On Thu, Apr 21, 2011 at 8:03 PM, Mark Brown
<[email protected]> wrote:
> On Thu, Apr 21, 2011 at 04:13:28AM -0700, Haojian Zhuang wrote:
>
>> >> +static struct regulator_init_data
>> >regulator_pdata[ARRAY_SIZE(regulator_devs)];
>
>> >That looks really suspicious, what happens if there's two of these
>> >devices in the system?
>
>> It's impossible to use two PMIC in one system. At least, I didn't hear.
>
> I don't believe that. ?It's not going to be a common case but it will be
> possible.
>
OK, I can allocate memory to handle this.
>> >It's really unclear why the array is needed at all if you're registering
>> >the devices one at a time.
>
>> Although there're a lot of regulators, maybe only parts of them are used.
>> So only add the necessary one. Others will be passed. The number is got
>> from platform data.
>
> You're missing the point here. ?You're registering a device at a time
> but for some reason the platform data is being stored in an array in
> order to pass it in, even though only a single element in the array is
> in use.
>
I can allocate memory for platform data.
>> >> - ?if (pdata && pdata->regulator[0]) {
>> >> - ? ? ? ? ?ret = mfd_add_devices(chip->dev, 0, ®ulator_devs[0],
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ARRAY_SIZE(regulator_devs),
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?®ulator_resources[0], 0);
>> >> - ? ? ? ? ?if (ret < 0) {
>> >> - ? ? ? ? ? ? ? ? ?dev_err(chip->dev, "Failed to add regulator subdev\n");
>> >> - ? ? ? ? ? ? ? ? ?goto out_dev;
>> >> - ? ? ? ? ?}
>
>> >Surely the only change that's needed here is to remove the check to see
>> >if pdata->regulator[0] is non-null?
>
>> All above changes are necessary to me.
>
> What makes you say this? ?If you remove the check for the first entry
> being null then the driver will go and register all the regulators which
> seems fine. ?That seems like it fixes the problem with skipping all the
> regulators if the first one has no platform data.
>
If I keep original method and only fix the null issue on regulator[0], I can
also register regulators. But it'll display error messages since
regulator_init_data is null while it's registers.
After switching to the current method, I only register the ones that I really
need. So I can avoid some error messages in boot time.
An array seems not good, and I can switch to allocate memory.
> You're really not explaining the changes you're trying to make here
> terribly well, the changes you're making seem very substantial relative
> to what you say you're trying to do.
>
Yes, I'll list all changes in log.
On Thu, Apr 21, 2011 at 11:23:19PM +0800, Haojian Zhuang wrote:
> On Thu, Apr 21, 2011 at 8:03 PM, Mark Brown
> > What makes you say this? ?If you remove the check for the first entry
> > being null then the driver will go and register all the regulators which
> > seems fine. ?That seems like it fixes the problem with skipping all the
> > regulators if the first one has no platform data.
> If I keep original method and only fix the null issue on regulator[0], I can
> also register regulators. But it'll display error messages since
> regulator_init_data is null while it's registers.
Registering regulators with null init data seems like a reasonable thing
to want to do - let's just remove the error message.