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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111019140027.GH18713@sirena.org.uk>
Date:	Wed, 19 Oct 2011 15:00:28 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Kyle Manna <kyle.manna@...l7.com>
Cc:	linux-kernel@...r.kernel.org, Samuel Ortiz <sameo@...ux.intel.com>,
	Liam Girdwood <lrg@...com>,
	Jorge Eduardo Candelaria <jedu@...mlogic.co.uk>,
	Graeme Gregory <gg@...mlogic.co.uk>
Subject: Re: [PATCH 6/6] mfd: TPS65910: Improve regulator init data

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/