[PATCHv3] support PMIC mc13892
Yong Shen
yong.shen at linaro.org
Wed Dec 8 04:51:03 EST 2010
Hi Uwe,
It's good to get more refined comments. I acknowledge most of them.
please see inline feedback for some iterms.
2010/12/8 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> 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 at linaro.org>
>>
>> add support for mc13892, tested on mx51 babbage board
>>
>> Signed-off-by: Arnaud Patard <arnaud.patard at rtp-net.org>
>> Signed-off-by: Yong Shen <yong.shen at 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 at linaro.org>
>> + *
>> + * Based on draft driver from Arnaud Patard <arnaud.patard at 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.
Both are ok for me, and what you prefer also makes sense to me.
>
>> +{
>> + 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.
I missed this last time. But IMHO, this label can also refer the place
for error handle, which is dealloc memory in this case. So if it is
not accurate, it could be err_dealloc or alike. Do you mean it should
be something like err_revision? correct me.
>> +
>> + /* 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 at 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 at 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/.
This file is supposed to be in include/linux/mfd, since register names
are also likely to be used by other mfd code.
>
>> +
>> +#endif
>> --
>> 1.7.0.4
>>
>>
>>
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
>
More information about the linux-arm-kernel
mailing list