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

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature