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] [day] [month] [year] [list]

Message-ID: <4EA58EC0.7080502@fuel7.com>
Date:	Mon, 24 Oct 2011 11:13:52 -0500
From:	Kyle Manna <kyle.manna@...l7.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.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 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/