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

[Mark Brown]

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.

> regulators.  The TWL4030/6030 code was used as an example interface.

This isn't a good sign...

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

> +	/* 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...

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