linux-kernel - Re: [PATCH V2] regulator: 88pm800: add regulator driver for 88pm800

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: <20130621152416.GC27646@sirena.org.uk>
Date:	Fri, 21 Jun 2013 16:24:16 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Chao Xie <chao.xie@...vell.com>
Cc:	lgirdwood@...il.com, linux-kernel@...r.kernel.org,
	yizhang@...vell.com, xiechao.mail@...il.com
Subject: Re: [PATCH V2] regulator: 88pm800: add regulator driver for 88pm800

On Thu, Jun 20, 2013 at 03:31:07AM -0400, Chao Xie wrote:

Looks pretty good, main thing here is that some of the functionality
here appears to be reproducing standard code.

> +static int pm800_set_voltage(struct regulator_dev *rdev,
> +			     int min_uv, int max_uv, unsigned *selector)
> +{

Use set_voltage_sel() and map_voltge_ascend()

> +static int pm800_get_voltage(struct regulator_dev *rdev)
> +{
> +

Just provide get_voltage_sel(), the core will do the mapping to voltages
using list_voltage().

> +	} else if (pdata->num_regulators) {
> +		/* Check whether num_regulator is valid. */
> +		unsigned int count = 0;
> +		for (i = 0; pdata->regulators[i]; i++)
> +			count++;
> +		if (count != pdata->num_regulators)
> +			return -EINVAL;

This looks...  odd.

> +static int __init pm800_regulator_init(void)
> +{
> +	return platform_driver_register(&pm800_regulator_driver);
> +}
> +subsys_initcall(pm800_regulator_init);
> +
> +static void __exit pm800_regulator_exit(void)
> +{
> +	platform_driver_unregister(&pm800_regulator_driver);
> +}
> +module_exit(pm800_regulator_exit);

With deferred probing you should just be able to use
module_platform_driver().

> +config REGULATOR_88PM800
> +	bool "Marvell 88PM800 Power regulators"
> +	depends on MFD_88PM800=y
> +	help
> +	  This driver supports Marvell 88PM800 voltage regulator chips.
> +	  It delivers digitally programmable output,
> +	  the voltage is programmed via I2C interface.
> +	  It's suitable to support PXA988 chips to control VCC_MAIN and
> +	  various voltages.
> +

Keep this file sorted please.

> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
> +obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o

Same here.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)