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

[Kyle Manna]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/