From mboxrd@z Thu Jan  1 00:00:00 1970
From: u.kleine-koenig@pengutronix.de (Uwe =?iso-8859-1?Q?Kleine-K=F6nig?=)
Date: Wed, 8 Dec 2010 10:12:26 +0100
Subject: [PATCHv3] support PMIC mc13892
In-Reply-To: <1291778460-22819-2-git-send-email-yong.shen@freescale.com>
References: <1291778460-22819-1-git-send-email-yong.shen@freescale.com>
	<1291778460-22819-2-git-send-email-yong.shen@freescale.com>
Message-ID: <20101208091226.GD18244@pengutronix.de>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

Hello,

Can you please add "regulator:" to the subject?

Some of the following comments are admittedly a bit picky, still I'd
like them fixed.

On Wed, Dec 08, 2010 at 11:21:00AM +0800, yong.shen at freescale.com wrote:
> From: Yong Shen <yong.shen@linaro.org>
> 
> add support for mc13892, tested on mx51 babbage board
> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Signed-off-by: Yong Shen <yong.shen@linaro.org>
> ---
>  drivers/regulator/Kconfig             |    8 +
>  drivers/regulator/Makefile            |    1 +
>  drivers/regulator/mc13892-regulator.c |  644 +++++++++++++++++++++++++++++++++
>  include/linux/mfd/mc13892.h           |   38 ++
>  4 files changed, 691 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/regulator/mc13892-regulator.c
>  create mode 100644 include/linux/mfd/mc13892.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 6e54253..1673a4f 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -197,6 +197,14 @@ config REGULATOR_MC13783
>  	  Say y here to support the regulators found on the Freescale MC13783
>  	  PMIC.
>  
> +config REGULATOR_MC13892
> +	tristate "Support regulators on Freescale MC13892 PMIC"
> +	depends on MFD_MC13XXX
> +	select REGULATOR_MC13XXX_CORE
> +	help
> +	  Say y here to support the regulators found on the Freescale MC13892
> +	  PMIC.
> +
>  config REGULATOR_AB3100
>  	tristate "ST-Ericsson AB3100 Regulator functions"
>  	depends on AB3100_CORE
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 11876be..3107480 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
>  obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
>  obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
> +obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
>  obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o
>  
> diff --git a/drivers/regulator/mc13892-regulator.c b/drivers/regulator/mc13892-regulator.c
> new file mode 100644
> index 0000000..1b23170
> --- /dev/null
> +++ b/drivers/regulator/mc13892-regulator.c
> @@ -0,0 +1,644 @@
> +/*
> + * Regulator Driver for Freescale MC13892 PMIC
> + *
> + * Copyright 2010 Yong Shen <yong.shen@linaro.org>
> + *
> + * Based on draft driver from Arnaud Patard <arnaud.patard@rtp-net.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/mfd/mc13892.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/platform_device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include "mc13xxx.h"
> +
> +#define MC13892_REVISION				7
> +
> +#define MC13892_POWERCTL0				13
> +#define MC13892_POWERCTL0_USEROFFSPI		3
> +#define MC13892_POWERCTL0_VCOINCELLVSEL		20
> +#define MC13892_POWERCTL0_VCOINCELLVSEL_M		(7<<20)
> +#define MC13892_POWERCTL0_VCOINCELLEN		(1<<23)
> +
> +#define MC13892_SWITCHERS0_SWxHI			(1<<23)
> +
> +#define MC13892_SWITCHERS0				24
> +#define MC13892_SWITCHERS0_SW1VSEL			0
> +#define MC13892_SWITCHERS0_SW1VSEL_M		(0x1f<<0)
> +#define MC13892_SWITCHERS0_SW1HI			(1<<23)
> +#define MC13892_SWITCHERS0_SW1EN		0
> +
> +#define MC13892_SWITCHERS1				25
> +#define MC13892_SWITCHERS1_SW2VSEL			0
> +#define MC13892_SWITCHERS1_SW2VSEL_M		(0x1f<<0)
> +#define MC13892_SWITCHERS1_SW2HI			(1<<23)
> +#define MC13892_SWITCHERS1_SW2EN		0
> +
> +#define MC13892_SWITCHERS2				26
> +#define MC13892_SWITCHERS2_SW3VSEL			0
> +#define MC13892_SWITCHERS2_SW3VSEL_M		(0x1f<<0)
> +#define MC13892_SWITCHERS2_SW3HI			(1<<23)
> +#define MC13892_SWITCHERS2_SW3EN		0
> +
> +#define MC13892_SWITCHERS3				27
> +#define MC13892_SWITCHERS3_SW4VSEL			0
> +#define MC13892_SWITCHERS3_SW4VSEL_M		(0x1f<<0)
> +#define MC13892_SWITCHERS3_SW4HI			(1<<23)
> +#define MC13892_SWITCHERS3_SW4EN		0
> +
> +#define MC13892_SWITCHERS4				28
> +#define MC13892_SWITCHERS4_SW1MODE			0
> +#define MC13892_SWITCHERS4_SW1MODE_AUTO		(8<<0)
> +#define MC13892_SWITCHERS4_SW1MODE_M		(0xf<<0)
> +#define MC13892_SWITCHERS4_SW2MODE			10
> +#define MC13892_SWITCHERS4_SW2MODE_AUTO		(8<<10)
> +#define MC13892_SWITCHERS4_SW2MODE_M		(0xf<<10)
> +
> +#define MC13892_SWITCHERS5				29
> +#define MC13892_SWITCHERS5_SW3MODE			0
> +#define MC13892_SWITCHERS5_SW3MODE_AUTO		(8<<0)
> +#define MC13892_SWITCHERS5_SW3MODE_M		(0xf<<0)
> +#define MC13892_SWITCHERS5_SW4MODE			8
> +#define MC13892_SWITCHERS5_SW4MODE_AUTO		(8<<8)
> +#define MC13892_SWITCHERS5_SW4MODE_M		(0xf<<8)
> +#define MC13892_SWITCHERS5_SWBSTEN			(1<<20)
> +
> +
This double blank line is inconsitent.  If you want you can better add a
comment here (IMHO).

> +#define MC13892_REGULATORSETTING0			30
> +#define MC13892_REGULATORSETTING0_VGEN1VSEL		0
> +#define MC13892_REGULATORSETTING0_VDIGVSEL		4
> +#define MC13892_REGULATORSETTING0_VGEN2VSEL		6
> +#define MC13892_REGULATORSETTING0_VPLLVSEL		9
> +#define MC13892_REGULATORSETTING0_VUSB2VSEL		11
> +#define MC13892_REGULATORSETTING0_VGEN3VSEL		14
> +#define MC13892_REGULATORSETTING0_VCAMVSEL		16
> +
> +#define MC13892_REGULATORSETTING0_VGEN1VSEL_M	(3<<0)
> +#define MC13892_REGULATORSETTING0_VDIGVSEL_M	(3<<4)
> +#define MC13892_REGULATORSETTING0_VGEN2VSEL_M	(7<<6)
> +#define MC13892_REGULATORSETTING0_VPLLVSEL_M	(3<<9)
> +#define MC13892_REGULATORSETTING0_VUSB2VSEL_M	(3<<11)
> +#define MC13892_REGULATORSETTING0_VGEN3VSEL_M	(1<<14)
> +#define MC13892_REGULATORSETTING0_VCAMVSEL_M	(3<<16)
> +
> +#define MC13892_REGULATORSETTING1			31
> +#define MC13892_REGULATORSETTING1_VVIDEOVSEL	2
> +#define MC13892_REGULATORSETTING1_VAUDIOVSEL	4
> +#define MC13892_REGULATORSETTING1_VSDVSEL		6
> +
> +#define MC13892_REGULATORSETTING1_VVIDEOVSEL_M	(3<<2)
> +#define MC13892_REGULATORSETTING1_VAUDIOVSEL_M	(3<<4)
> +#define MC13892_REGULATORSETTING1_VSDVSEL_M		(7<<6)
> +
> +#define MC13892_REGULATORMODE0			32
> +#define MC13892_REGULATORMODE0_VGEN1EN		(1<<0)
> +#define MC13892_REGULATORMODE0_VGEN1STDBY		(1<<1)
> +#define MC13892_REGULATORMODE0_VGEN1MODE		(1<<2)
> +#define MC13892_REGULATORMODE0_VIOHIEN		(1<<3)
> +#define MC13892_REGULATORMODE0_VIOHISTDBY		(1<<4)
> +#define MC13892_REGULATORMODE0_VIOHIMODE		(1<<5)
> +#define MC13892_REGULATORMODE0_VDIGEN		(1<<9)
> +#define MC13892_REGULATORMODE0_VDIGSTDBY		(1<<10)
> +#define MC13892_REGULATORMODE0_VDIGMODE		(1<<11)
> +#define MC13892_REGULATORMODE0_VGEN2EN		(1<<12)
> +#define MC13892_REGULATORMODE0_VGEN2STDBY		(1<<13)
> +#define MC13892_REGULATORMODE0_VGEN2MODE		(1<<14)
> +#define MC13892_REGULATORMODE0_VPLLEN		(1<<15)
> +#define MC13892_REGULATORMODE0_VPLLSTDBY		(1<<16)
> +#define MC13892_REGULATORMODE0_VPLLMODE		(1<<17)
> +#define MC13892_REGULATORMODE0_VUSB2EN		(1<<18)
> +#define MC13892_REGULATORMODE0_VUSB2STDBY		(1<<19)
> +#define MC13892_REGULATORMODE0_VUSB2MODE		(1<<20)
> +
> +#define MC13892_REGULATORMODE1			33
> +#define MC13892_REGULATORMODE1_VGEN3EN		(1<<0)
> +#define MC13892_REGULATORMODE1_VGEN3STDBY		(1<<1)
> +#define MC13892_REGULATORMODE1_VGEN3MODE		(1<<2)
> +#define MC13892_REGULATORMODE1_VCAMEN		(1<<6)
> +#define MC13892_REGULATORMODE1_VCAMSTDBY		(1<<7)
> +#define MC13892_REGULATORMODE1_VCAMMODE		(1<<8)
> +#define MC13892_REGULATORMODE1_VCAMCONFIGEN		(1<<9)
> +#define MC13892_REGULATORMODE1_VVIDEOEN		(1<<12)
> +#define MC13892_REGULATORMODE1_VVIDEOSTDBY		(1<<13)
> +#define MC13892_REGULATORMODE1_VVIDEOMODE		(1<<14)
> +#define MC13892_REGULATORMODE1_VAUDIOEN		(1<<15)
> +#define MC13892_REGULATORMODE1_VAUDIOSTDBY		(1<<16)
> +#define MC13892_REGULATORMODE1_VAUDIOMODE		(1<<17)
> +#define MC13892_REGULATORMODE1_VSDEN		(1<<18)
> +#define MC13892_REGULATORMODE1_VSDSTDBY		(1<<19)
> +#define MC13892_REGULATORMODE1_VSDMODE		(1<<20)
> +
> +#define MC13892_POWERMISC				34
> +#define MC13892_POWERMISC_GPO1EN			(1<<6)
> +#define MC13892_POWERMISC_GPO2EN			(1<<8)
> +#define MC13892_POWERMISC_GPO3EN			(1<<10)
> +#define MC13892_POWERMISC_GPO4EN			(1<<12)
> +#define MC13892_POWERMISC_PWGT1SPIEN		(1<<15)
> +#define MC13892_POWERMISC_PWGT2SPIEN		(1<<16)
> +#define MC13892_POWERMISC_GPO4ADINEN		(1<<21)
> +
> +#define MC13892_POWERMISC_PWGTSPI_M			(3 << 15)
> +
> +#define MC13892_USB1				50
> +#define MC13892_USB1_VUSBEN				(1<<3)
> +
> +static const int mc13892_vcoincell[] = {
> +	2500000, 2700000, 2800000, 2900000, 3000000, 3100000,
> +	3200000, 3300000,
> +};
> +
> +static const int mc13892_sw1[] = {
> +	600000,   625000,  650000,  675000,  700000,  725000,
> +	750000,   775000,  800000,  825000,  850000,  875000,
> +	900000,   925000,  950000,  975000, 1000000, 1025000,
> +	1050000, 1075000, 1100000, 1125000, 1150000, 1175000,
> +	1200000, 1225000, 1250000, 1275000, 1300000, 1325000,
> +	1350000, 1375000
> +};
> +
> +static const int mc13892_sw[] = {
> +	600000,   625000,  650000,  675000,  700000,  725000,
> +	750000,   775000,  800000,  825000,  850000,  875000,
> +	900000,   925000,  950000,  975000, 1000000, 1025000,
> +	1050000, 1075000, 1100000, 1125000, 1150000, 1175000,
> +	1200000, 1225000, 1250000, 1275000, 1300000, 1325000,
> +	1350000, 1375000, 1400000, 1425000, 1450000, 1475000,
> +	1500000, 1525000, 1550000, 1575000, 1600000, 1625000,
> +	1650000, 1675000, 1700000, 1725000, 1750000, 1775000,
> +	1800000, 1825000, 1850000, 1875000
> +};
> +
> +static const int mc13892_swbst[] = {
> +	5000000,
> +};
> +
> +static const int mc13892_viohi[] = {
> +	2775000,
> +};
> +
> +static const int mc13892_vpll[] = {
> +	1050000, 1250000, 1650000, 1800000,
> +};
> +
> +static const int mc13892_vdig[] = {
> +	1050000, 1250000, 1650000, 1800000,
> +};
> +
> +static const int mc13892_vsd[] = {
> +	1800000, 2000000, 2600000, 2700000,
> +	2800000, 2900000, 3000000, 3150000,
> +};
> +
> +static const int mc13892_vusb2[] = {
> +	2400000, 2600000, 2700000, 2775000,
> +};
> +
> +static const int mc13892_vvideo[] = {
> +	2700000, 2775000, 2500000, 2600000,
> +};
> +
> +static const int mc13892_vaudio[] = {
> +	2300000, 2500000, 2775000, 3000000,
> +};
> +
> +static const int mc13892_vcam[] = {
> +	2500000, 2600000, 2750000, 3000000,
> +};
> +
> +static const int mc13892_vgen1[] = {
> +	1200000, 1500000, 2775000, 3150000,
> +};
> +
> +static const int mc13892_vgen2[] = {
> +	1200000, 1500000, 1600000, 1800000,
> +	2700000, 2800000, 3000000, 3150000,
> +};
> +
> +static const int mc13892_vgen3[] = {
> +	1800000, 2900000,
> +};
> +
> +static const int mc13892_vusb[] = {
> +	3300000,
> +};
> +
> +static const int mc13892_gpo[] = {
> +	2750000,
> +};
> +
> +static const int mc13892_pwgtdrv[] = {
> +	5000000,
> +};
> +
> +static struct regulator_ops mc13892_gpo_regulator_ops;
> +/* sw regulators need special care due to the "hi bit" */
> +static struct regulator_ops mc13892_sw_regulator_ops;
> +
> +
> +#define MC13892_FIXED_DEFINE(name, reg, voltages)\
> +	MC13xxx_FIXED_DEFINE(MC13892_, name, reg, voltages, \
> +			mc13xxx_fixed_regulator_ops)
> +
> +#define MC13892_GPO_DEFINE(name, reg, voltages)		\
> +	MC13xxx_GPO_DEFINE(MC13892_, name, reg, voltages, \
> +			mc13892_gpo_regulator_ops)
> +
> +#define MC13892_SW_DEFINE(name, reg, vsel_reg, voltages)		\
> +	MC13xxx_DEFINE(MC13892_, name, reg, vsel_reg, voltages, \
> +			mc13892_sw_regulator_ops)
> +
> +#define MC13892_DEFINE_REGU(name, reg, vsel_reg, voltages)		\
> +	MC13xxx_DEFINE(MC13892_, name, reg, vsel_reg, voltages, \
> +			mc13xxx_regulator_ops)
> +
> +static struct mc13xxx_regulator mc13892_regulators[] = {
> +	MC13892_DEFINE_REGU(VCOINCELL, POWERCTL0, POWERCTL0,		\
> +			    mc13892_vcoincell),
> +	MC13892_SW_DEFINE(SW1, SWITCHERS0, SWITCHERS0, mc13892_sw1),
> +	MC13892_SW_DEFINE(SW2, SWITCHERS1, SWITCHERS1, mc13892_sw),
> +	MC13892_SW_DEFINE(SW3, SWITCHERS2, SWITCHERS2, mc13892_sw),
> +	MC13892_SW_DEFINE(SW4, SWITCHERS3, SWITCHERS3, mc13892_sw),
> +	MC13892_FIXED_DEFINE(SWBST, SWITCHERS5, mc13892_swbst),
> +	MC13892_FIXED_DEFINE(VIOHI, REGULATORMODE0, mc13892_viohi),
> +	MC13892_DEFINE_REGU(VPLL, REGULATORMODE0, REGULATORSETTING0,	\
> +			    mc13892_vpll),
> +	MC13892_DEFINE_REGU(VDIG, REGULATORMODE0, REGULATORSETTING0,	\
> +			    mc13892_vdig),
> +	MC13892_DEFINE_REGU(VSD, REGULATORMODE1, REGULATORSETTING1,	\
> +			    mc13892_vsd),
> +	MC13892_DEFINE_REGU(VUSB2, REGULATORMODE0, REGULATORSETTING0,	\
> +			    mc13892_vusb2),
> +	MC13892_DEFINE_REGU(VVIDEO, REGULATORMODE1, REGULATORSETTING1,	\
> +			    mc13892_vvideo),
> +	MC13892_DEFINE_REGU(VAUDIO, REGULATORMODE1, REGULATORSETTING1,	\
> +			    mc13892_vaudio),
> +	MC13892_DEFINE_REGU(VCAM, REGULATORMODE1, REGULATORSETTING0, \
> +			    mc13892_vcam),
> +	MC13892_DEFINE_REGU(VGEN1, REGULATORMODE0, REGULATORSETTING0,	\
> +			    mc13892_vgen1),
> +	MC13892_DEFINE_REGU(VGEN2, REGULATORMODE0, REGULATORSETTING0,	\
> +			    mc13892_vgen2),
> +	MC13892_DEFINE_REGU(VGEN3, REGULATORMODE1, REGULATORSETTING0,	\
> +			    mc13892_vgen3),
> +	MC13892_FIXED_DEFINE(VUSB, USB1, mc13892_vusb),
> +	MC13892_GPO_DEFINE(GPO1, POWERMISC, mc13892_gpo),
> +	MC13892_GPO_DEFINE(GPO2, POWERMISC, mc13892_gpo),
> +	MC13892_GPO_DEFINE(GPO3, POWERMISC, mc13892_gpo),
> +	MC13892_GPO_DEFINE(GPO4, POWERMISC, mc13892_gpo),
> +	MC13892_GPO_DEFINE(PWGT1SPI, POWERMISC, mc13892_pwgtdrv),
> +	MC13892_GPO_DEFINE(PWGT2SPI, POWERMISC, mc13892_pwgtdrv),
> +};
> +
> +int mc13892_powermisc_rmw(struct mc13xxx_regulator_priv *priv, u32 mask,
> +									u32 val)
How do you decide how many tabs are used for indention?  This seems to
be inconsistent over the patch.  The usual conventions I know of are:
	- align to opening paren
	- two tabs
(I prefer the latter because if the function name (or it's attributes)
change there is no need to relayout the lines of the arguments.

> +{
> +	struct mc13xxx *mc13892 = priv->mc13xxx;
> +	int ret;
> +	u32 valread;
> +
> +	BUG_ON(val & ~mask);
> +
> +	ret = mc13xxx_reg_read(mc13892, MC13892_POWERMISC, &valread);
> +	if (ret)
> +		return ret;
> +
> +	/* Update the stored state for Power Gates. */
> +	priv->powermisc_pwgt_state =
> +				(priv->powermisc_pwgt_state & ~mask) | val;
Here a single tab more than the preceeding line is usual.

> +	priv->powermisc_pwgt_state &= MC13892_POWERMISC_PWGTSPI_M;
> +
> +	/* Construct the new register value */
> +	valread = (valread & ~mask) | val;
> +	/* Overwrite the PWGTxEN with the stored version */
> +	valread = (valread & ~MC13892_POWERMISC_PWGTSPI_M) |
> +						priv->powermisc_pwgt_state;
> +
> +	return mc13xxx_reg_write(mc13892, MC13892_POWERMISC, valread);
> +}
> +
> +static int mc13892_gpo_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct mc13xxx_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	int id = rdev_get_id(rdev);
> +	int ret;
> +	u32 en_val = mc13892_regulators[id].enable_bit;
> +	u32 mask = mc13892_regulators[id].enable_bit;
> +
> +	dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
> +
> +	/* Power Gate enable value is 0 */
> +	if (id == MC13892_PWGT1SPI ||
> +	    id == MC13892_PWGT2SPI)
> +		en_val = 0;
> +
> +	if (id == MC13892_GPO4)
> +		mask |= MC13892_POWERMISC_GPO4ADINEN;
> +
> +	mc13xxx_lock(priv->mc13xxx);
> +	ret = mc13892_powermisc_rmw(priv, mask, en_val);
> +	mc13xxx_unlock(priv->mc13xxx);
> +
> +	return ret;
> +}
> +
> +static int mc13892_gpo_regulator_disable(struct regulator_dev *rdev)
> +{
> +	struct mc13xxx_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	int id = rdev_get_id(rdev);
> +	int ret;
> +	u32 dis_val = 0;
> +
> +	dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
> +
> +	/* Power Gate disable value is 1 */
> +	if (id == MC13892_PWGT1SPI ||
> +	    id == MC13892_PWGT2SPI)
> +		dis_val = mc13892_regulators[id].enable_bit;
> +
> +	mc13xxx_lock(priv->mc13xxx);
> +	ret = mc13892_powermisc_rmw(priv, mc13892_regulators[id].enable_bit,
> +					dis_val);
> +	mc13xxx_unlock(priv->mc13xxx);
> +
> +	return ret;
> +}
> +
> +static int mc13892_gpo_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct mc13xxx_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	int ret, id = rdev_get_id(rdev);
> +	unsigned int val;
> +
> +	mc13xxx_lock(priv->mc13xxx);
> +	ret = mc13xxx_reg_read(priv->mc13xxx, mc13892_regulators[id].reg, &val);
> +	mc13xxx_unlock(priv->mc13xxx);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Power Gates state is stored in powermisc_pwgt_state
> +	 * where the meaning of bits is negated */
> +	val = (val & ~MC13892_POWERMISC_PWGTSPI_M) |
> +	      (priv->powermisc_pwgt_state ^ MC13892_POWERMISC_PWGTSPI_M);
> +
> +	return (val & mc13892_regulators[id].enable_bit) != 0;
> +}
> +
> +
> +static struct regulator_ops mc13892_gpo_regulator_ops = {
> +	.enable = mc13892_gpo_regulator_enable,
> +	.disable = mc13892_gpo_regulator_disable,
> +	.is_enabled = mc13892_gpo_regulator_is_enabled,
> +	.list_voltage = mc13xxx_regulator_list_voltage,
> +	.set_voltage = mc13xxx_fixed_regulator_set_voltage,
> +	.get_voltage = mc13xxx_fixed_regulator_get_voltage,
> +};
> +
> +static int mc13892_sw_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct mc13xxx_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	int ret, id = rdev_get_id(rdev);
> +	unsigned int val, hi;
> +
> +	dev_dbg(rdev_get_dev(rdev), "%s id: %d\n", __func__, id);
> +
> +	mc13xxx_lock(priv->mc13xxx);
> +	ret = mc13xxx_reg_read(priv->mc13xxx,
> +				mc13892_regulators[id].vsel_reg, &val);
> +	mc13xxx_unlock(priv->mc13xxx);
> +	if (ret)
> +		return ret;
> +
> +	hi  = val & MC13892_SWITCHERS0_SWxHI;
> +	val = (val & mc13892_regulators[id].vsel_mask)
> +		>> mc13892_regulators[id].vsel_shift;
> +
> +	dev_dbg(rdev_get_dev(rdev), "%s id: %d val: %d\n", __func__, id, val);
> +
> +	if (hi)
> +		val = (25000 * val) + 1100000;
> +	else
> +		val = (25000 * val) + 600000;
> +
> +	return val;
> +}
> +
> +static int mc13892_sw_regulator_set_voltage(struct regulator_dev *rdev,
> +						int min_uV, int max_uV)
> +{
> +	struct mc13xxx_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	int hi, value, val, mask, id = rdev_get_id(rdev);
> +	int ret;
> +
> +	dev_dbg(rdev_get_dev(rdev), "%s id: %d min_uV: %d max_uV: %d\n",
> +		__func__, id, min_uV, max_uV);
> +
> +	/* Find the best index */
> +	value = mc13xxx_get_best_voltage_index(rdev, min_uV, max_uV);
> +	dev_dbg(rdev_get_dev(rdev), "%s best value: %d\n", __func__, value);
> +	if (value < 0)
> +		return value;
> +
> +	value = mc13892_regulators[id].voltages[value];
> +
> +	mc13xxx_lock(priv->mc13xxx);
> +	ret = mc13xxx_reg_read(priv->mc13xxx,
> +				mc13892_regulators[id].vsel_reg, &val);
> +	if (ret)
> +		goto err;
> +
> +	hi  = val & MC13892_SWITCHERS0_SWxHI;
> +	if (value > 1375)
> +		hi = 1;
> +	if (value < 1100)
> +		hi = 0;
> +
> +	if (hi) {
> +		value = (value - 1100000) / 25000;
> +		value |= MC13892_SWITCHERS0_SWxHI;
> +	} else
> +		value = (value - 600000) / 25000;
> +
> +	mask = mc13892_regulators[id].vsel_mask | MC13892_SWITCHERS0_SWxHI;
> +	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].vsel_reg,
> +			mask,
> +			value << mc13892_regulators[id].vsel_shift);
The content of the two preceeding lines fit on a single line.

> +err:
> +	mc13xxx_unlock(priv->mc13xxx);
> +
> +	return ret;
> +}
> +
> +static struct regulator_ops mc13892_sw_regulator_ops = {
> +	.is_enabled = mc13xxx_sw_regulator_is_enabled,
> +	.list_voltage = mc13xxx_regulator_list_voltage,
> +	.set_voltage = mc13892_sw_regulator_set_voltage,
> +	.get_voltage = mc13892_sw_regulator_get_voltage,
> +};
> +
> +static int mc13892_vcam_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{
> +	unsigned int en_val = 0;
> +	struct mc13xxx_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	int ret, id = rdev_get_id(rdev);
> +
> +	if (mode == REGULATOR_MODE_FAST)
> +		en_val = MC13892_REGULATORMODE1_VCAMCONFIGEN;
> +
> +	mc13xxx_lock(priv->mc13xxx);
> +	ret = mc13xxx_reg_rmw(priv->mc13xxx, mc13892_regulators[id].reg,
> +			MC13892_REGULATORMODE1_VCAMCONFIGEN,
> +			en_val);
> +	mc13xxx_unlock(priv->mc13xxx);
> +
> +	return ret;
> +}
> +
> +unsigned int mc13892_vcam_get_mode(struct regulator_dev *rdev)
> +{
> +	struct mc13xxx_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	int ret, id = rdev_get_id(rdev);
> +	unsigned int val;
> +
> +	mc13xxx_lock(priv->mc13xxx);
> +	ret = mc13xxx_reg_read(priv->mc13xxx, mc13892_regulators[id].reg, &val);
> +	mc13xxx_unlock(priv->mc13xxx);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (val & MC13892_REGULATORMODE1_VCAMCONFIGEN)
> +		return REGULATOR_MODE_FAST;
> +
> +	return REGULATOR_MODE_NORMAL;
> +}
> +
> +
> +static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
> +{
> +	struct mc13xxx_regulator_priv *priv;
> +	struct mc13xxx *mc13892 = dev_get_drvdata(pdev->dev.parent);
> +	struct mc13xxx_regulator_platform_data *pdata =
> +		dev_get_platdata(&pdev->dev);
> +	struct mc13xxx_regulator_init_data *init_data;
> +	int i, ret;
> +	u32 val;
> +
> +	priv = kzalloc(sizeof(*priv) +
> +			pdata->num_regulators * sizeof(priv->regulators[0]),
> +			GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->mc13xxx_regulators = mc13892_regulators;
> +	priv->mc13xxx = mc13892;
> +
> +	mc13xxx_lock(mc13892);
> +	ret = mc13xxx_reg_read(mc13892, MC13892_REVISION, &val);
> +	mc13xxx_unlock(mc13892);
You can keep holding the lock here.
> +	if (ret)
> +		goto err_alloc;
err_alloc seems wrong.  The goto is taken when reading the revision
register fails, not on a failed allocation.
> +
> +	/* enable switch auto mode */
> +	if ((val & 0x0000FFFF) == 0x45d0) {
> +		mc13xxx_lock(mc13892);
> +		ret = mc13xxx_reg_rmw(mc13892, MC13892_SWITCHERS4,
> +			MC13892_SWITCHERS4_SW1MODE_M |
> +			MC13892_SWITCHERS4_SW2MODE_M,
> +			MC13892_SWITCHERS4_SW1MODE_AUTO |
> +			MC13892_SWITCHERS4_SW2MODE_AUTO);
> +		mc13xxx_unlock(mc13892);
ditto

> +		if (ret)
> +			goto err_alloc;
again, err_alloc is the wrong name

> +
> +		mc13xxx_lock(mc13892);
> +		mc13xxx_reg_rmw(mc13892, MC13892_SWITCHERS5,
> +			MC13892_SWITCHERS5_SW3MODE_M |
> +			MC13892_SWITCHERS5_SW4MODE_M,
> +			MC13892_SWITCHERS5_SW3MODE_AUTO |
> +			MC13892_SWITCHERS5_SW4MODE_AUTO);
> +		mc13xxx_unlock(mc13892);
> +		if (ret)
> +			goto err_alloc;
ditto

> +	}
> +
> +	mc13892_regulators[MC13892_VCAM].desc.ops->set_mode
> +		= mc13892_vcam_set_mode;
> +	mc13892_regulators[MC13892_VCAM].desc.ops->get_mode
> +		= mc13892_vcam_get_mode;
> +	for (i = 0; i < pdata->num_regulators; i++) {
> +		init_data = &pdata->regulators[i];
> +		priv->regulators[i] = regulator_register(
> +				&mc13892_regulators[init_data->id].desc,
> +				&pdev->dev, init_data->init_data, priv);
> +
> +		if (IS_ERR(priv->regulators[i])) {
> +			dev_err(&pdev->dev, "failed to register regulator %s\n",
> +				mc13892_regulators[i].desc.name);
> +			ret = PTR_ERR(priv->regulators[i]);
> +			goto err;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +err:
> +	while (--i >= 0)
> +		regulator_unregister(priv->regulators[i]);
> +
> +err_alloc:
> +	kfree(priv);
> +
> +	return ret;
> +}
> +
> +static int __devexit mc13892_regulator_remove(struct platform_device *pdev)
> +{
> +	struct mc13xxx_regulator_priv *priv = platform_get_drvdata(pdev);
> +	struct mc13xxx_regulator_platform_data *pdata =
> +		dev_get_platdata(&pdev->dev);
> +	int i;
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	for (i = 0; i < pdata->num_regulators; i++)
> +		regulator_unregister(priv->regulators[i]);
> +
> +	kfree(priv);
> +	return 0;
> +}
> +
> +static struct platform_driver mc13892_regulator_driver = {
> +	.driver	= {
> +		.name	= "mc13892-regulator",
> +		.owner	= THIS_MODULE,
> +	},
> +	.remove	= __devexit_p(mc13892_regulator_remove),
> +	.probe	= mc13892_regulator_probe,
> +};
> +
> +static int __init mc13892_regulator_init(void)
> +{
> +	return platform_driver_register(&mc13892_regulator_driver);
> +}
> +subsys_initcall(mc13892_regulator_init);
> +
> +static void __exit mc13892_regulator_exit(void)
> +{
> +	platform_driver_unregister(&mc13892_regulator_driver);
> +}
> +module_exit(mc13892_regulator_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Yong Shen <yong.shen@linaro.org>");
> +MODULE_DESCRIPTION("Regulator Driver for Freescale MC13892 PMIC");
> +MODULE_ALIAS("platform:mc13892-regulator");
> diff --git a/include/linux/mfd/mc13892.h b/include/linux/mfd/mc13892.h
> new file mode 100644
> index 0000000..7df13e8
> --- /dev/null
> +++ b/include/linux/mfd/mc13892.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright 2010 Yong Shen <yong.shen@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation.
> + */
> +
> +#ifndef __LINUX_MFD_MC13892_H
> +#define __LINUX_MFD_MC13892_H
newline here please
> +#include <linux/mfd/mc13xxx.h>
> +
> +#define MC13892_SW1		0
> +#define MC13892_SW2		1
> +#define MC13892_SW3		2
> +#define MC13892_SW4		3
> +#define MC13892_SWBST	4
> +#define MC13892_VIOHI	5
> +#define MC13892_VPLL	6
> +#define MC13892_VDIG	7
> +#define MC13892_VSD	8
> +#define MC13892_VUSB2	9
> +#define MC13892_VVIDEO	10
> +#define MC13892_VAUDIO	11
> +#define MC13892_VCAM	12
> +#define MC13892_VGEN1	13
> +#define MC13892_VGEN2	14
> +#define MC13892_VGEN3	15
> +#define MC13892_VUSB	16
> +#define MC13892_GPO1	17
> +#define MC13892_GPO2	18
> +#define MC13892_GPO3	19
> +#define MC13892_GPO4	20
> +#define MC13892_PWGT1SPI	21
> +#define MC13892_PWGT2SPI	22
> +#define MC13892_VCOINCELL	23
I thought you wanted to put these somewhere below drivers/regulator/.

> +
> +#endif
> -- 
> 1.7.0.4
> 
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |