Re: [PATCH 6/6] mfd: TPS65910: Improve regulator init data

[Kyle Manna <kyle.manna@fuel7.com>]

On 10/19/2011 09:00 AM, Mark Brown wrote:
> On Tue, Oct 18, 2011 at 01:26:28PM -0500, Kyle Manna wrote:
>> Improve the interface between platform code/board files to the TPS65910
> Again, *always* CC maintainers on patches.

This was an oversight on my part.

>
>> regulators.  The TWL4030/6030 code was used as an example interface.
> This isn't a good sign...

I've reviewed other PMICs (ie Wolfson Micro ;) ) and will post an 
updated series with an interface similar to what is used there.  The new 
approach makes more sense and keeps the code/patch small.

>
>> This improved interface will allow use of the regulators without
>> specifying all the constraints. Also gets rid of an assumption that
>> the platform pass in an array of correct size and was unchecked.
> You've not described the changes between the two interfaces.  Note that
> empty constraints should be absolutely fine with the API.
>
>> +	if (init_data->constraints.name)
>> +		pmic->desc[i].name = init_data->constraints.name;
>> +	else
>> +		pmic->desc[i].name = info[i].name;
> No, this is broken.  The name of the regulator is a fixed property of
> the device and isn't something that ought to be overridden per system.

Understood.

>
>> +	/* TPS65910 and TPS65911 Regulators */
>> +	rdev = add_regulator(pmic, info, TPS65910_REG_VRTC,
>> +			pmic_plat_data->vrtc);
>> +	if (IS_ERR(rdev))
>> +		return PTR_ERR(rdev);
>> +	rdev = add_regulator(pmic, info, TPS65910_REG_VIO,
>> +			pmic_plat_data->vio);
>> +
>> +	if (IS_ERR(rdev))
>> +		return PTR_ERR(rdev);
>> +
>> +	rdev = add_regulator(pmic, info, TPS65910_REG_VDD1,
>> +			pmic_plat_data->vdd1);
>> +	if (IS_ERR(rdev))
>> +		return PTR_ERR(rdev);
> This looks like a regression - we've gone from looping over an array
> which is nice and simple to explicit code for each individual regulator
> giving us lots of repetitive code...

Will be revised.
>> -err_unregister_regulator:
>> -	while (--i>= 0)
>> -		regulator_unregister(pmic->rdev[i]);
>> -	kfree(pmic->rdev);
> ...and loosing all our cleanup if things go wrong which isn't great
> either.