RE: [PATCH 13/14] regulator: max8925: fix not add device if missinginit data

[Haojian Zhuang]

>-----Original Message-----
>From: Mark Brown [mailto:broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx]
>Sent: 2011年4月21日 6:42 PM
>To: Haojian Zhuang
>Cc: sameo@xxxxxxxxxxxxxxx; haojian.zhuang@xxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; lrg@xxxxxxxxxxxxxxx
>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(&regulator_pdata[i], &pdata->regulator[i],
>> +			sizeof(struct regulator_init_data));
>> +		regulator_devs[i].platform_data = &regulator_pdata[i];
>> +		regulator_devs[i].pdata_size = sizeof(regulator_pdata[i]);
>> +		regulator_devs[i].num_resources = 1;
>> +		regulator_devs[i].resources = &regulator_resources[seq];
>> +
>> +		ret = mfd_add_devices(chip->dev, 0, &regulator_devs[i], 1,
>> +				      &regulator_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, &regulator_devs[0],
>> -				      ARRAY_SIZE(regulator_devs),
>> -				      &regulator_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?????%?lzwm?b?Р骒r?zXЩ??{ay????j?f＂?????ア?⒎?:+v???????赙zZ+????"?!?O???v??m?鹈n?帼Y&—