2012-06-15 08:46:45

by Kim, Milo

[permalink] [raw]
Subject: [PATCH v2] regulator: add new regulator driver for lp872x

This driver supports TI/National Semiconductor LP8720, LP8725 PMIC.

patch v2
--------
(a) remove unnecessary i/p mutex for i2c
(b) replace regulator ops functions with updated regulator ops functions

list_voltage -> regulator_list_voltage_table
set_voltage_sel -> regulator_set_voltage_sel_regmap
get_voltage_sel -> regulator_get_voltage_sel_regmap
regulator_enable -> regulator_enable_regmap
regulator_disable -> regulator_disable_regmap
regulator_is_enabled -> regulator_is_enabled_regmap

(c) add dvs platform data
: add dvs gpio pin control on choosing buck voltage registers

(d) add 'max_register' member in lp872x_regmap_config
: dump registers in debugfs

(e) change condition for registering regulators
: find matched regulator init data from platform data on registering regulators

(f) use devm_regmap_init_i2c() rather than regmap_init_i2c()
: and remove regmap_exit()

Signed-off-by: Milo(Woogyom) Kim <[email protected]>
---
drivers/regulator/Kconfig | 7 +
drivers/regulator/Makefile | 1 +
drivers/regulator/lp872x.c | 947 ++++++++++++++++++++++++++++++++++++++
include/linux/regulator/lp872x.h | 88 ++++
4 files changed, 1043 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/lp872x.c
create mode 100644 include/linux/regulator/lp872x.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 00ffe05..870f730 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -224,6 +224,13 @@ config REGULATOR_LP3972
Say Y here to support the voltage regulators and convertors
on National Semiconductors LP3972 PMIC

+config REGULATOR_LP872X
+ tristate "TI/National Semiconductor LP8720/LP8725 voltage regulators"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ This driver supports LP8720/LP8725 PMIC
+
config REGULATOR_PCF50633
tristate "NXP PCF50633 regulator driver"
depends on MFD_PCF50633
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index d854453..e6f7007 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_REGULATOR_GPIO) += gpio-regulator.o
obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
obj-$(CONFIG_REGULATOR_LP3972) += lp3972.o
+obj-$(CONFIG_REGULATOR_LP872X) += lp872x.o
obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
obj-$(CONFIG_REGULATOR_MAX8649) += max8649.o
obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
new file mode 100644
index 0000000..44f5b26
--- /dev/null
+++ b/drivers/regulator/lp872x.c
@@ -0,0 +1,947 @@
+/*
+ * Copyright 2012 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim <[email protected]>
+ *
+ * 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/module.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/regulator/lp872x.h>
+#include <linux/regulator/driver.h>
+#include <linux/platform_device.h>
+
+/* Registers : LP8720/8725 shared */
+#define LP872X_GENERAL_CFG 0x00
+#define LP872X_LDO1_VOUT 0x01
+#define LP872X_LDO2_VOUT 0x02
+#define LP872X_LDO3_VOUT 0x03
+#define LP872X_LDO4_VOUT 0x04
+#define LP872X_LDO5_VOUT 0x05
+
+/* Registers : LP8720 */
+#define LP8720_BUCK_VOUT1 0x06
+#define LP8720_BUCK_VOUT2 0x07
+#define LP8720_ENABLE 0x08
+
+/* Registers : LP8725 */
+#define LP8725_LILO1_VOUT 0x06
+#define LP8725_LILO2_VOUT 0x07
+#define LP8725_BUCK1_VOUT1 0x08
+#define LP8725_BUCK1_VOUT2 0x09
+#define LP8725_BUCK2_VOUT1 0x0A
+#define LP8725_BUCK2_VOUT2 0x0B
+#define LP8725_BUCK_CTRL 0x0C
+#define LP8725_LDO_CTRL 0x0D
+
+/* Mask/shift : LP8720/LP8725 shared */
+#define LP872X_VOUT_M 0x1F
+#define LP872X_START_DELAY_M 0xE0
+#define LP872X_START_DELAY_S 5
+#define LP872X_EN_LDO1_M BIT(0)
+#define LP872X_EN_LDO2_M BIT(1)
+#define LP872X_EN_LDO3_M BIT(2)
+#define LP872X_EN_LDO4_M BIT(3)
+#define LP872X_EN_LDO5_M BIT(4)
+
+/* Mask/shift : LP8720 */
+#define LP8720_TIMESTEP_S 0 /* Addr 00h */
+#define LP8720_TIMESTEP_M BIT(0)
+#define LP8720_EXT_DVS_M BIT(2)
+#define LP8720_BUCK_FPWM_S 5 /* Addr 07h */
+#define LP8720_BUCK_FPWM_M BIT(5)
+#define LP8720_EN_BUCK_M BIT(5) /* Addr 08h */
+#define LP8720_DVS_SEL_M BIT(7)
+
+/* Mask/shift : LP8725 */
+#define LP8725_TIMESTEP_M 0xC0 /* Addr 00h */
+#define LP8725_TIMESTEP_S 6
+#define LP8725_BUCK1_EN_M BIT(0)
+#define LP8725_DVS1_M BIT(2)
+#define LP8725_DVS2_M BIT(3)
+#define LP8725_BUCK2_EN_M BIT(4)
+#define LP8725_BUCK_CL_M 0xC0 /* Addr 09h, 0Bh */
+#define LP8725_BUCK_CL_S 6
+#define LP8725_BUCK1_FPWM_S 1 /* Addr 0Ch */
+#define LP8725_BUCK1_FPWM_M BIT(1)
+#define LP8725_BUCK2_FPWM_S 5
+#define LP8725_BUCK2_FPWM_M BIT(5)
+#define LP8725_EN_LILO1_M BIT(5) /* Addr 0Dh */
+#define LP8725_EN_LILO2_M BIT(6)
+
+/* PWM mode */
+#define LP872X_FORCE_PWM 1
+#define LP872X_AUTO_PWM 0
+
+#define LP8720_NUM_REGULATORS 6
+#define LP8725_NUM_REGULATORS 9
+#define EXTERN_DVS_USED 0
+#define MAX_DELAY 6
+
+/* dump registers in regmap-debugfs */
+#define MAX_REGISTERS 0x0F
+
+enum lp872x_id {
+ LP8720,
+ LP8725,
+};
+
+struct lp872x {
+ struct regmap *regmap;
+ struct device *dev;
+ enum lp872x_id chipid;
+ struct lp872x_platform_data *pdata;
+ struct regulator_dev **regulators;
+ int num_regulators;
+ enum lp872x_dvs_state dvs_pin;
+ int dvs_gpio;
+};
+
+/* LP8720/LP8725 shared voltage table for LDOs */
+static const unsigned int lp872x_ldo_vtbl[] = {
+ 1200000, 1250000, 1300000, 1350000, 1400000, 1450000, 1500000, 1550000,
+ 1600000, 1650000, 1700000, 1750000, 1800000, 1850000, 1900000, 2000000,
+ 2100000, 2200000, 2300000, 2400000, 2500000, 2600000, 2650000, 2700000,
+ 2750000, 2800000, 2850000, 2900000, 2950000, 3000000, 3100000, 3300000,
+};
+
+/* LP8720 LDO4 voltage table */
+static const unsigned int lp8720_ldo4_vtbl[] = {
+ 800000, 850000, 900000, 1000000, 1100000, 1200000, 1250000, 1300000,
+ 1350000, 1400000, 1450000, 1500000, 1550000, 1600000, 1650000, 1700000,
+ 1750000, 1800000, 1850000, 1900000, 2000000, 2100000, 2200000, 2300000,
+ 2400000, 2500000, 2600000, 2650000, 2700000, 2750000, 2800000, 2850000,
+};
+
+/* LP8725 LILO(Low Input Low Output) voltage table */
+static const unsigned int lp8725_lilo_vtbl[] = {
+ 800000, 850000, 900000, 950000, 1000000, 1050000, 1100000, 1150000,
+ 1200000, 1250000, 1300000, 1350000, 1400000, 1500000, 1600000, 1700000,
+ 1800000, 1900000, 2000000, 2100000, 2200000, 2300000, 2400000, 2500000,
+ 2600000, 2700000, 2800000, 2850000, 2900000, 3000000, 3100000, 3300000,
+};
+
+/* LP8720 BUCK voltage table */
+#define EXT_R 0 /* external resistor divider */
+static const unsigned int lp8720_buck_vtbl[] = {
+ EXT_R, 800000, 850000, 900000, 950000, 1000000, 1050000, 1100000,
+ 1150000, 1200000, 1250000, 1300000, 1350000, 1400000, 1450000, 1500000,
+ 1550000, 1600000, 1650000, 1700000, 1750000, 1800000, 1850000, 1900000,
+ 1950000, 2000000, 2050000, 2100000, 2150000, 2200000, 2250000, 2300000,
+};
+
+/* LP8725 BUCK voltage table */
+static const unsigned int lp8725_buck_vtbl[] = {
+ 800000, 850000, 900000, 950000, 1000000, 1050000, 1100000, 1150000,
+ 1200000, 1250000, 1300000, 1350000, 1400000, 1500000, 1600000, 1700000,
+ 1750000, 1800000, 1850000, 1900000, 2000000, 2100000, 2200000, 2300000,
+ 2400000, 2500000, 2600000, 2700000, 2800000, 2850000, 2900000, 3000000,
+};
+
+/* LP8725 BUCK current limit */
+static const unsigned int lp8725_buck_uA[] = {
+ 460000, 780000, 1050000, 1370000,
+};
+
+static int lp872x_read_byte(struct lp872x *lp, u8 addr, u8 *data)
+{
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(lp->regmap, addr, &val);
+ if (ret < 0) {
+ dev_err(lp->dev, "failed to read 0x%.2x\n", addr);
+ return ret;
+ }
+
+ *data = (u8)val;
+ return 0;
+}
+
+static inline int lp872x_write_byte(struct lp872x *lp, u8 addr, u8 data)
+{
+ return regmap_write(lp->regmap, addr, data);
+}
+
+static inline int lp872x_update_bits(struct lp872x *lp, u8 addr,
+ unsigned int mask, u8 data)
+{
+ return regmap_update_bits(lp->regmap, addr, mask, data);
+}
+
+static int _rdev_to_offset(struct regulator_dev *rdev)
+{
+ enum lp872x_regulator_id id = rdev_get_id(rdev);
+
+ switch (id) {
+ case LP8720_ID_LDO1 ... LP8720_ID_BUCK:
+ return id;
+ case LP8725_ID_LDO1 ... LP8725_ID_BUCK2:
+ return id - LP8725_ID_BASE;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int lp872x_get_timestep_usec(struct lp872x *lp)
+{
+ enum lp872x_id chip = lp->chipid;
+ u8 val, mask, shift;
+ int *time_usec, size, ret;
+ int lp8720_time_usec[] = { 25, 50 };
+ int lp8725_time_usec[] = { 32, 64, 128, 256 };
+
+ if (chip == LP8720) {
+ mask = LP8720_TIMESTEP_M;
+ shift = LP8720_TIMESTEP_S;
+ time_usec = &lp8720_time_usec[0];
+ size = ARRAY_SIZE(lp8720_time_usec);
+ } else if (chip == LP8725) {
+ mask = LP8725_TIMESTEP_M;
+ shift = LP8725_TIMESTEP_S;
+ time_usec = &lp8725_time_usec[0];
+ size = ARRAY_SIZE(lp8725_time_usec);
+ } else {
+ return -EINVAL;
+ }
+
+ ret = lp872x_read_byte(lp, LP872X_GENERAL_CFG, &val);
+ if (ret)
+ return -EINVAL;
+
+ val = (val & mask) >> shift;
+ if (val >= size)
+ return -EINVAL;
+
+ return *(time_usec + val);
+}
+
+static int lp872x_regulator_enable_time(struct regulator_dev *rdev)
+{
+ struct lp872x *lp = rdev_get_drvdata(rdev);
+ enum lp872x_regulator_id regulator = rdev_get_id(rdev);
+ int time_step_us = lp872x_get_timestep_usec(lp);
+ int ret, offset;
+ u8 addr, val;
+
+ if (time_step_us < 0)
+ return -EINVAL;
+
+ switch (regulator) {
+ case LP8720_ID_LDO1 ... LP8720_ID_LDO5:
+ case LP8725_ID_LDO1 ... LP8725_ID_LILO2:
+ offset = _rdev_to_offset(rdev);
+ if (offset < 0)
+ return -EINVAL;
+
+ addr = LP872X_LDO1_VOUT + offset;
+ break;
+ case LP8720_ID_BUCK:
+ addr = LP8720_BUCK_VOUT1;
+ break;
+ case LP8725_ID_BUCK1:
+ addr = LP8725_BUCK1_VOUT1;
+ break;
+ case LP8725_ID_BUCK2:
+ addr = LP8725_BUCK2_VOUT1;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = lp872x_read_byte(lp, addr, &val);
+ if (ret)
+ return ret;
+
+ val = (val & LP872X_START_DELAY_M) >> LP872X_START_DELAY_S;
+
+ return val > MAX_DELAY ? 0 : val * time_step_us;
+}
+
+static void lp872x_set_dvs(struct lp872x *lp, int gpio)
+{
+ enum lp872x_dvs_sel dvs_sel = lp->pdata->dvs->vsel;
+ enum lp872x_dvs_state state;
+
+ state = dvs_sel == SEL_V1 ? DVS_HIGH : DVS_LOW;
+ gpio_set_value(gpio, state);
+ lp->dvs_pin = state;
+}
+
+static u8 lp872x_select_buck_vout_addr(struct lp872x *lp,
+ enum lp872x_regulator_id buck)
+{
+ u8 val, addr;
+
+ if (lp872x_read_byte(lp, LP872X_GENERAL_CFG, &val))
+ return 0;
+
+ switch (buck) {
+ case LP8720_ID_BUCK:
+ if (val & LP8720_EXT_DVS_M) {
+ addr = (lp->dvs_pin == DVS_HIGH) ?
+ LP8720_BUCK_VOUT1 : LP8720_BUCK_VOUT2;
+ } else {
+ if (lp872x_read_byte(lp, LP8720_ENABLE, &val))
+ return 0;
+
+ addr = val & LP8720_DVS_SEL_M ?
+ LP8720_BUCK_VOUT1 : LP8720_BUCK_VOUT2;
+ }
+ break;
+ case LP8725_ID_BUCK1:
+ if (val & LP8725_DVS1_M)
+ addr = LP8725_BUCK1_VOUT1;
+ else
+ addr = (lp->dvs_pin == DVS_HIGH) ?
+ LP8725_BUCK1_VOUT1 : LP8725_BUCK1_VOUT2;
+ break;
+ case LP8725_ID_BUCK2:
+ addr = val & LP8725_DVS2_M ?
+ LP8725_BUCK2_VOUT1 : LP8725_BUCK2_VOUT2;
+ break;
+ default:
+ return 0;
+ }
+
+ return addr;
+}
+
+static bool lp872x_is_valid_buck_addr(u8 addr)
+{
+ switch (addr) {
+ case LP8720_BUCK_VOUT1:
+ case LP8720_BUCK_VOUT2:
+ case LP8725_BUCK1_VOUT1:
+ case LP8725_BUCK1_VOUT2:
+ case LP8725_BUCK2_VOUT1:
+ case LP8725_BUCK2_VOUT2:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static int lp872x_buck_set_voltage_sel(struct regulator_dev *rdev,
+ unsigned selector)
+{
+ struct lp872x *lp = rdev_get_drvdata(rdev);
+ enum lp872x_regulator_id buck = rdev_get_id(rdev);
+ u8 addr, mask = LP872X_VOUT_M;
+ struct lp872x_dvs *dvs = lp->pdata->dvs;
+
+ if (dvs && gpio_is_valid(dvs->gpio))
+ lp872x_set_dvs(lp, dvs->gpio);
+
+ addr = lp872x_select_buck_vout_addr(lp, buck);
+ if (!lp872x_is_valid_buck_addr(addr))
+ return -EINVAL;
+
+ return lp872x_update_bits(lp, addr, mask, selector);
+}
+
+static int lp872x_buck_get_voltage_sel(struct regulator_dev *rdev)
+{
+ struct lp872x *lp = rdev_get_drvdata(rdev);
+ enum lp872x_regulator_id buck = rdev_get_id(rdev);
+ u8 addr, val;
+ int ret;
+
+ addr = lp872x_select_buck_vout_addr(lp, buck);
+ if (!lp872x_is_valid_buck_addr(addr))
+ return -EINVAL;
+
+ ret = lp872x_read_byte(lp, addr, &val);
+ if (ret)
+ return ret;
+
+ return val & LP872X_VOUT_M;
+}
+
+static int lp8725_buck_set_current_limit(struct regulator_dev *rdev,
+ int min_uA, int max_uA)
+{
+ struct lp872x *lp = rdev_get_drvdata(rdev);
+ enum lp872x_regulator_id buck = rdev_get_id(rdev);
+ int i, max = ARRAY_SIZE(lp8725_buck_uA);
+ u8 addr, val;
+
+ if (buck == LP8725_ID_BUCK1)
+ addr = LP8725_BUCK1_VOUT2;
+ else if (buck == LP8725_ID_BUCK2)
+ addr = LP8725_BUCK2_VOUT2;
+ else
+ return -EINVAL;
+
+ for (i = 0 ; i < max ; i++)
+ if (lp8725_buck_uA[i] >= min_uA &&
+ lp8725_buck_uA[i] <= max_uA)
+ break;
+
+ if (i == max)
+ return -EINVAL;
+
+ val = i << LP8725_BUCK_CL_S;
+
+ return lp872x_update_bits(lp, addr, LP8725_BUCK_CL_M, val);
+}
+
+static int lp8725_buck_get_current_limit(struct regulator_dev *rdev)
+{
+ struct lp872x *lp = rdev_get_drvdata(rdev);
+ enum lp872x_regulator_id buck = rdev_get_id(rdev);
+ u8 addr, val;
+ int ret;
+
+ if (buck == LP8725_ID_BUCK1)
+ addr = LP8725_BUCK1_VOUT2;
+ else if (buck == LP8725_ID_BUCK2)
+ addr = LP8725_BUCK2_VOUT2;
+ else
+ return -EINVAL;
+
+ ret = lp872x_read_byte(lp, addr, &val);
+ if (ret)
+ return ret;
+
+ val = (val & LP8725_BUCK_CL_M) >> LP8725_BUCK_CL_S;
+
+ return (val < ARRAY_SIZE(lp8725_buck_uA)) ?
+ lp8725_buck_uA[val] : -EINVAL;
+}
+
+static int lp872x_buck_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ struct lp872x *lp = rdev_get_drvdata(rdev);
+ enum lp872x_regulator_id buck = rdev_get_id(rdev);
+ u8 addr, mask, shift, val;
+
+ switch (buck) {
+ case LP8720_ID_BUCK:
+ addr = LP8720_BUCK_VOUT2;
+ mask = LP8720_BUCK_FPWM_M;
+ shift = LP8720_BUCK_FPWM_S;
+ break;
+ case LP8725_ID_BUCK1:
+ addr = LP8725_BUCK_CTRL;
+ mask = LP8725_BUCK1_FPWM_M;
+ shift = LP8725_BUCK1_FPWM_S;
+ break;
+ case LP8725_ID_BUCK2:
+ addr = LP8725_BUCK_CTRL;
+ mask = LP8725_BUCK2_FPWM_M;
+ shift = LP8725_BUCK2_FPWM_S;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (mode == REGULATOR_MODE_FAST)
+ val = LP872X_FORCE_PWM << shift;
+ else if (mode == REGULATOR_MODE_NORMAL)
+ val = LP872X_AUTO_PWM << shift;
+ else
+ return -EINVAL;
+
+ return lp872x_update_bits(lp, addr, mask, val);
+}
+
+static unsigned int lp872x_buck_get_mode(struct regulator_dev *rdev)
+{
+ struct lp872x *lp = rdev_get_drvdata(rdev);
+ enum lp872x_regulator_id buck = rdev_get_id(rdev);
+ u8 addr, mask, val;
+ int ret;
+
+ switch (buck) {
+ case LP8720_ID_BUCK:
+ addr = LP8720_BUCK_VOUT2;
+ mask = LP8720_BUCK_FPWM_M;
+ break;
+ case LP8725_ID_BUCK1:
+ addr = LP8725_BUCK_CTRL;
+ mask = LP8725_BUCK1_FPWM_M;
+ break;
+ case LP8725_ID_BUCK2:
+ addr = LP8725_BUCK_CTRL;
+ mask = LP8725_BUCK2_FPWM_M;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = lp872x_read_byte(lp, addr, &val);
+ if (ret)
+ return ret;
+
+ return val & mask ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
+}
+
+static struct regulator_ops lp872x_ldo_ops = {
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .enable_time = lp872x_regulator_enable_time,
+};
+
+static struct regulator_ops lp8720_buck_ops = {
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = lp872x_buck_set_voltage_sel,
+ .get_voltage_sel = lp872x_buck_get_voltage_sel,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .enable_time = lp872x_regulator_enable_time,
+ .set_mode = lp872x_buck_set_mode,
+ .get_mode = lp872x_buck_get_mode,
+};
+
+static struct regulator_ops lp8725_buck_ops = {
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = lp872x_buck_set_voltage_sel,
+ .get_voltage_sel = lp872x_buck_get_voltage_sel,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .enable_time = lp872x_regulator_enable_time,
+ .set_mode = lp872x_buck_set_mode,
+ .get_mode = lp872x_buck_get_mode,
+ .set_current_limit = lp8725_buck_set_current_limit,
+ .get_current_limit = lp8725_buck_get_current_limit,
+};
+
+static struct regulator_desc lp8720_regulator_desc[] = {
+ {
+ .name = "ldo1",
+ .id = LP8720_ID_LDO1,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO1_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8720_ENABLE,
+ .enable_mask = LP872X_EN_LDO1_M,
+ },
+ {
+ .name = "ldo2",
+ .id = LP8720_ID_LDO2,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO2_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8720_ENABLE,
+ .enable_mask = LP872X_EN_LDO2_M,
+ },
+ {
+ .name = "ldo3",
+ .id = LP8720_ID_LDO3,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO3_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8720_ENABLE,
+ .enable_mask = LP872X_EN_LDO3_M,
+ },
+ {
+ .name = "ldo4",
+ .id = LP8720_ID_LDO4,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl),
+ .volt_table = lp8720_ldo4_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO4_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8720_ENABLE,
+ .enable_mask = LP872X_EN_LDO4_M,
+ },
+ {
+ .name = "ldo5",
+ .id = LP8720_ID_LDO5,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO5_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8720_ENABLE,
+ .enable_mask = LP872X_EN_LDO5_M,
+ },
+ {
+ .name = "buck",
+ .id = LP8720_ID_BUCK,
+ .ops = &lp8720_buck_ops,
+ .n_voltages = ARRAY_SIZE(lp8720_buck_vtbl),
+ .volt_table = lp8720_buck_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = LP8720_ENABLE,
+ .enable_mask = LP8720_EN_BUCK_M,
+ },
+};
+
+static struct regulator_desc lp8725_regulator_desc[] = {
+ {
+ .name = "ldo1",
+ .id = LP8725_ID_LDO1,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO1_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8725_LDO_CTRL,
+ .enable_mask = LP872X_EN_LDO1_M,
+ },
+ {
+ .name = "ldo2",
+ .id = LP8725_ID_LDO2,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO2_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8725_LDO_CTRL,
+ .enable_mask = LP872X_EN_LDO2_M,
+ },
+ {
+ .name = "ldo3",
+ .id = LP8725_ID_LDO3,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO3_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8725_LDO_CTRL,
+ .enable_mask = LP872X_EN_LDO3_M,
+ },
+ {
+ .name = "ldo4",
+ .id = LP8725_ID_LDO4,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO4_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8725_LDO_CTRL,
+ .enable_mask = LP872X_EN_LDO4_M,
+ },
+ {
+ .name = "ldo5",
+ .id = LP8725_ID_LDO5,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
+ .volt_table = lp872x_ldo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP872X_LDO5_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8725_LDO_CTRL,
+ .enable_mask = LP872X_EN_LDO5_M,
+ },
+ {
+ .name = "lilo1",
+ .id = LP8725_ID_LILO1,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
+ .volt_table = lp8725_lilo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP8725_LILO1_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8725_LDO_CTRL,
+ .enable_mask = LP8725_EN_LILO1_M,
+ },
+ {
+ .name = "lilo2",
+ .id = LP8725_ID_LILO2,
+ .ops = &lp872x_ldo_ops,
+ .n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
+ .volt_table = lp8725_lilo_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .vsel_reg = LP8725_LILO2_VOUT,
+ .vsel_mask = LP872X_VOUT_M,
+ .enable_reg = LP8725_LDO_CTRL,
+ .enable_mask = LP8725_EN_LILO2_M,
+ },
+ {
+ .name = "buck1",
+ .id = LP8725_ID_BUCK1,
+ .ops = &lp8725_buck_ops,
+ .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
+ .volt_table = lp8725_buck_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = LP872X_GENERAL_CFG,
+ .enable_mask = LP8725_BUCK1_EN_M,
+ },
+ {
+ .name = "buck2",
+ .id = LP8725_ID_BUCK2,
+ .ops = &lp8725_buck_ops,
+ .n_voltages = ARRAY_SIZE(lp8725_buck_vtbl),
+ .volt_table = lp8725_buck_vtbl,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .enable_reg = LP872X_GENERAL_CFG,
+ .enable_mask = LP8725_BUCK2_EN_M,
+ },
+};
+
+static int lp872x_check_dvs_validity(struct lp872x *lp)
+{
+ struct lp872x_dvs *dvs = lp->pdata->dvs;
+ u8 val = 0;
+ int ret;
+
+ ret = lp872x_read_byte(lp, LP872X_GENERAL_CFG, &val);
+ if (ret)
+ return ret;
+
+ ret = 0;
+ if (lp->chipid == LP8720) {
+ if (val & LP8720_EXT_DVS_M)
+ ret = dvs ? 0 : -EINVAL;
+ } else {
+ if ((val & LP8725_DVS1_M) == EXTERN_DVS_USED)
+ ret = dvs ? 0 : -EINVAL;
+ }
+
+ return ret;
+}
+
+static int lp872x_init_dvs(struct lp872x *lp)
+{
+ int ret, gpio;
+ struct lp872x_dvs *dvs = lp->pdata->dvs;
+
+ ret = lp872x_check_dvs_validity(lp);
+ if (ret) {
+ dev_warn(lp->dev, "invalid dvs data: %d\n", ret);
+ return ret;
+ }
+
+ gpio = dvs->gpio;
+
+ if (!gpio_is_valid(gpio)) {
+ dev_err(lp->dev, "invalid gpio: %d\n", gpio);
+ return -EINVAL;
+ }
+
+ ret = gpio_request(gpio, "LP872X DVS");
+ if (ret) {
+ dev_err(lp->dev, "gpio request err: %d\n", ret);
+ return ret;
+ }
+
+ gpio_direction_output(gpio, dvs->init_state);
+ lp->dvs_pin = dvs->init_state;
+ lp->dvs_gpio = gpio;
+
+ return 0;
+}
+
+static void lp872x_deinit_dvs(struct lp872x *lp)
+{
+ if (lp->dvs_gpio)
+ gpio_free(lp->dvs_gpio);
+}
+
+static int lp872x_config(struct lp872x *lp)
+{
+ struct lp872x_platform_data *pdata = lp->pdata;
+ int ret;
+
+ if (!pdata) {
+ dev_warn(lp->dev, "no platform data\n");
+ return 0;
+ }
+
+ if (!pdata->update_config)
+ return 0;
+
+ ret = lp872x_write_byte(lp, LP872X_GENERAL_CFG, pdata->general_config);
+ if (ret)
+ return ret;
+
+ return lp872x_init_dvs(lp);
+}
+
+static struct regulator_init_data
+*lp872x_find_regulator_init_data(int idx, struct lp872x *lp)
+{
+ int i;
+ int base = lp->chipid == LP8725 ? LP8725_ID_BASE : 0;
+ int id = idx + base;
+
+ for (i = 0 ; i < LP872X_MAX_REGULATORS ; i++)
+ if (lp->pdata->regulator_data[i].id == id)
+ break;
+
+ return (i == LP872X_MAX_REGULATORS) ? NULL :
+ lp->pdata->regulator_data[i].init_data;
+}
+
+static int lp872x_regulator_register(struct lp872x *lp)
+{
+ struct regulator_desc *desc;
+ struct regulator_config cfg = { };
+ struct regulator_dev *rdev;
+ int i, ret;
+
+ for (i = 0 ; i < lp->num_regulators ; i++) {
+ desc = (lp->chipid == LP8720) ? &lp8720_regulator_desc[i] :
+ &lp8725_regulator_desc[i];
+
+ cfg.dev = lp->dev;
+ cfg.init_data = lp872x_find_regulator_init_data(i, lp);
+ cfg.driver_data = lp;
+ cfg.regmap = lp->regmap;
+
+ rdev = regulator_register(desc, &cfg);
+ if (IS_ERR(rdev)) {
+ dev_err(lp->dev, "regulator register err");
+ ret = PTR_ERR(rdev);
+ goto err;
+ }
+
+ *(lp->regulators + i) = rdev;
+ }
+
+ return 0;
+err:
+ while (--i >= 0) {
+ rdev = *(lp->regulators + i);
+ regulator_unregister(rdev);
+ }
+ return ret;
+}
+
+static void lp872x_regulator_unregister(struct lp872x *lp)
+{
+ struct regulator_dev *rdev;
+ int i;
+
+ for (i = 0 ; i < lp->num_regulators ; i++) {
+ rdev = *(lp->regulators + i);
+ regulator_unregister(rdev);
+ }
+}
+
+static const struct regmap_config lp872x_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MAX_REGISTERS,
+};
+
+static int lp872x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
+{
+ struct lp872x *lp;
+ struct lp872x_platform_data *pdata = cl->dev.platform_data;
+ int ret, size, num_regulators;
+ const int lp872x_num_regulators[] = {
+ [LP8720] = LP8720_NUM_REGULATORS,
+ [LP8725] = LP8725_NUM_REGULATORS,
+ };
+
+ lp = devm_kzalloc(&cl->dev, sizeof(struct lp872x), GFP_KERNEL);
+ if (!lp)
+ goto err_mem;
+
+ num_regulators = lp872x_num_regulators[id->driver_data];
+ size = sizeof(struct regulator_dev *) * num_regulators;
+
+ lp->regulators = devm_kzalloc(&cl->dev, size, GFP_KERNEL);
+ if (!lp->regulators)
+ goto err_mem;
+
+ lp->regmap = devm_regmap_init_i2c(cl, &lp872x_regmap_config);
+ if (IS_ERR(lp->regmap)) {
+ ret = PTR_ERR(lp->regmap);
+ dev_err(&cl->dev, "regmap init i2c err: %d\n", ret);
+ goto err_dev;
+ }
+
+ lp->dev = &cl->dev;
+ lp->pdata = pdata;
+ lp->chipid = id->driver_data;
+ lp->num_regulators = num_regulators;
+ i2c_set_clientdata(cl, lp);
+
+ ret = lp872x_config(lp);
+ if (ret)
+ goto err_dev;
+
+ ret = lp872x_regulator_register(lp);
+ if (ret)
+ goto err_reg;
+
+ return 0;
+
+err_mem:
+ return -ENOMEM;
+err_reg:
+ lp872x_deinit_dvs(lp);
+err_dev:
+ return ret;
+}
+
+static int __devexit lp872x_remove(struct i2c_client *cl)
+{
+ struct lp872x *lp = i2c_get_clientdata(cl);
+
+ lp872x_regulator_unregister(lp);
+ lp872x_deinit_dvs(lp);
+ return 0;
+}
+
+static const struct i2c_device_id lp872x_ids[] = {
+ {"lp8720", LP8720},
+ {"lp8725", LP8725},
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, lp872x_ids);
+
+static struct i2c_driver lp872x_driver = {
+ .driver = {
+ .name = "lp872x",
+ .owner = THIS_MODULE,
+ },
+ .probe = lp872x_probe,
+ .remove = __devexit_p(lp872x_remove),
+ .id_table = lp872x_ids,
+};
+
+module_i2c_driver(lp872x_driver);
+
+MODULE_DESCRIPTION("TI/National Semiconductor LP872x PMU Regulator Driver");
+MODULE_AUTHOR("Milo Kim");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regulator/lp872x.h b/include/linux/regulator/lp872x.h
new file mode 100644
index 0000000..86fdc98
--- /dev/null
+++ b/include/linux/regulator/lp872x.h
@@ -0,0 +1,88 @@
+/*
+ * Copyright 2012 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim <[email protected]>
+ *
+ * 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 __LP872X_REGULATOR_H__
+#define __LP872X_REGULATOR_H__
+
+#include <linux/regulator/machine.h>
+#include <linux/platform_device.h>
+
+#define LP872X_MAX_REGULATORS 9
+
+enum lp872x_regulator_id {
+ LP8720_ID_LDO1,
+ LP8720_ID_LDO2,
+ LP8720_ID_LDO3,
+ LP8720_ID_LDO4,
+ LP8720_ID_LDO5,
+ LP8720_ID_BUCK,
+
+ LP8725_ID_BASE,
+ LP8725_ID_LDO1 = LP8725_ID_BASE,
+ LP8725_ID_LDO2,
+ LP8725_ID_LDO3,
+ LP8725_ID_LDO4,
+ LP8725_ID_LDO5,
+ LP8725_ID_LILO1,
+ LP8725_ID_LILO2,
+ LP8725_ID_BUCK1,
+ LP8725_ID_BUCK2,
+
+ LP872X_ID_MAX,
+};
+
+enum lp872x_dvs_state {
+ DVS_LOW,
+ DVS_HIGH,
+};
+
+enum lp872x_dvs_sel {
+ SEL_V1,
+ SEL_V2,
+};
+
+/**
+ * lp872x_dvs
+ * @gpio : gpio pin number for dvs control
+ * @vsel : dvs selector for buck v1 or buck v2 register
+ * @init_state : initial dvs pin state
+ */
+struct lp872x_dvs {
+ int gpio;
+ enum lp872x_dvs_sel vsel;
+ enum lp872x_dvs_state init_state;
+};
+
+/**
+ * lp872x_regdata
+ * @id : regulator id
+ * @init_data : init data for each regulator
+ */
+struct lp872x_regulator_data {
+ enum lp872x_regulator_id id;
+ struct regulator_init_data *init_data;
+};
+
+/**
+ * lp872x_platform_data
+ * @general_config : the value of LP872X_GENERAL_CFG register
+ * @update_config : if LP872X_GENERAL_CFG register is updated, set true
+ * @regulator_data : platform regulator id and init data
+ * @dvs : dvs data for buck voltage control
+ */
+struct lp872x_platform_data {
+ u8 general_config;
+ bool update_config;
+ struct lp872x_regulator_data regulator_data[LP872X_MAX_REGULATORS];
+ struct lp872x_dvs *dvs;
+};
+
+#endif
--
1.7.4.1


Best Regards
Milo -


2012-06-15 11:12:11

by Axel Lin

[permalink] [raw]
Subject: Re: [PATCH v2] regulator: add new regulator driver for lp872x

> +
> +static int lp872x_init_dvs(struct lp872x *lp)
> +{
> + ? ? ? int ret, gpio;
> + ? ? ? struct lp872x_dvs *dvs = lp->pdata->dvs;
> +
> + ? ? ? ret = lp872x_check_dvs_validity(lp);
> + ? ? ? if (ret) {
> + ? ? ? ? ? ? ? dev_warn(lp->dev, "invalid dvs data: %d\n", ret);
> + ? ? ? ? ? ? ? return ret;
> + ? ? ? }
> +
> + ? ? ? gpio = dvs->gpio;
> +
> + ? ? ? if (!gpio_is_valid(gpio)) {
> + ? ? ? ? ? ? ? dev_err(lp->dev, "invalid gpio: %d\n", gpio);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? ret = gpio_request(gpio, "LP872X DVS");
devm_gpio_request

> + ? ? ? if (ret) {
> + ? ? ? ? ? ? ? dev_err(lp->dev, "gpio request err: %d\n", ret);
> + ? ? ? ? ? ? ? return ret;
> + ? ? ? }
> +
> + ? ? ? gpio_direction_output(gpio, dvs->init_state);
> + ? ? ? lp->dvs_pin = dvs->init_state;
> + ? ? ? lp->dvs_gpio = gpio;
> +
> + ? ? ? return 0;
> +}
> +
> +static void lp872x_deinit_dvs(struct lp872x *lp)
> +{
> + ? ? ? if (lp->dvs_gpio)
> + ? ? ? ? ? ? ? gpio_free(lp->dvs_gpio);
> +}

If you are using devm_gpio_request in lp872x_init_dvs() , you can just
kill lp872x_deinit_dvs() function.


> +static struct regulator_init_data
> +*lp872x_find_regulator_init_data(int idx, struct lp872x *lp)
> +{
> + ? ? ? int i;
> + ? ? ? int base = lp->chipid == LP8725 ? LP8725_ID_BASE : 0;
> + ? ? ? int id = idx + base;
> +
> + ? ? ? for (i = 0 ; i < LP872X_MAX_REGULATORS ; i++)
> + ? ? ? ? ? ? ? if (lp->pdata->regulator_data[i].id == id)
> + ? ? ? ? ? ? ? ? ? ? ? break;
return lp->pdata->regulator_data[i].init_data;
> +
> + ? ? ? return (i == LP872X_MAX_REGULATORS) ? NULL :
> + ? ? ? ? ? ? ? ? ? ? ? lp->pdata->regulator_data[i].init_data;
return NULL; // seems better readability this way
> +}
> +

> +module_i2c_driver(lp872x_driver);

I think you need to use subsys_initcall() here.

Regulators need to be available early in init in order to allow them
to be available for consumers when requested.

2012-06-15 11:41:28

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2] regulator: add new regulator driver for lp872x

On Fri, Jun 15, 2012 at 07:11:48PM +0800, Axel Lin wrote:

> > + ? ? ? ret = gpio_request(gpio, "LP872X DVS");

> devm_gpio_request

Better yet...

> > + ? ? ? gpio_direction_output(gpio, dvs->init_state);

...devm_gpio_request_one().

> > +module_i2c_driver(lp872x_driver);

> I think you need to use subsys_initcall() here.

> Regulators need to be available early in init in order to allow them
> to be available for consumers when requested.

Well, it depends on the regulator. Those with DCDCs that might supply
cpufreq do because cpufreq can't use deferred probes. Others should now
be able to use deferred probes which should removed the need for this.


Attachments:
(No filename) (662.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-06-15 13:21:34

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2] regulator: add new regulator driver for lp872x

On Fri, Jun 15, 2012 at 08:46:34AM +0000, Kim, Milo wrote:
> This driver supports TI/National Semiconductor LP8720, LP8725 PMIC.

Overall looks very good, just a few fairly minor things below but
nothing substantial.

>
> patch v2
> --------

See Documentation/SubmittingPatches for how to format stuff like this -
put it after the ---

> + if (chip == LP8720) {
> + mask = LP8720_TIMESTEP_M;
> + shift = LP8720_TIMESTEP_S;
> + time_usec = &lp8720_time_usec[0];
> + size = ARRAY_SIZE(lp8720_time_usec);
> + } else if (chip == LP8725) {
> + mask = LP8725_TIMESTEP_M;
> + shift = LP8725_TIMESTEP_S;
> + time_usec = &lp8725_time_usec[0];
> + size = ARRAY_SIZE(lp8725_time_usec);
> + } else {
> + return -EINVAL;
> + }

This should be a switch statement.

> +static int lp8725_buck_set_current_limit(struct regulator_dev *rdev,
> + int min_uA, int max_uA)
> +{
> + struct lp872x *lp = rdev_get_drvdata(rdev);
> + enum lp872x_regulator_id buck = rdev_get_id(rdev);
> + int i, max = ARRAY_SIZE(lp8725_buck_uA);
> + u8 addr, val;
> +
> + if (buck == LP8725_ID_BUCK1)
> + addr = LP8725_BUCK1_VOUT2;
> + else if (buck == LP8725_ID_BUCK2)
> + addr = LP8725_BUCK2_VOUT2;
> + else
> + return -EINVAL;

Again, switch statement.

> +static const struct regmap_config lp872x_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX_REGISTERS,
> +};

Probably worth enabling cache here, it'll cut out read/modify/write
cycles which will improve the performance as the reads should be free.
Not essential but worth considering.


Attachments:
(No filename) (1.51 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2012-06-15 14:50:34

by Axel Lin

[permalink] [raw]
Subject: Re: [PATCH v2] regulator: add new regulator driver for lp872x

> +static struct regulator_init_data
> +*lp872x_find_regulator_init_data(int idx, struct lp872x *lp)
> +{
> + ? ? ? int i;
> + ? ? ? int base = lp->chipid == LP8725 ? LP8725_ID_BASE : 0;
> + ? ? ? int id = idx + base;
> +
> + ? ? ? for (i = 0 ; i < LP872X_MAX_REGULATORS ; i++)

LP872X_MAX_REGULATORS is 9.
But for LP8720, the LP8720_NUM_REGULATORS is 6.
So this should be:
for (i = 0 ; i < lp->num_regulators ; i++)

2012-06-19 06:42:12

by Kim, Milo

[permalink] [raw]
Subject: RE: [PATCH v2] regulator: add new regulator driver for lp872x

>
> On Fri, Jun 15, 2012 at 07:11:48PM +0800, Axel Lin wrote:
>
> > > + ? ? ? ret = gpio_request(gpio, "LP872X DVS");
>
> > devm_gpio_request
>
> Better yet...
>
> > > + ? ? ? gpio_direction_output(gpio, dvs->init_state);
>
> ...devm_gpio_request_one().
>

Thanks for the update.
In patch v3, devm_gpio_request_one() will be used in replacement of gpio_request() and gpio_direction_output().

Thanks & BR
Milo -

2012-06-19 06:42:26

by Kim, Milo

[permalink] [raw]
Subject: RE: [PATCH v2] regulator: add new regulator driver for lp872x

> LP872X_MAX_REGULATORS is 9.
> But for LP8720, the LP8720_NUM_REGULATORS is 6.
> So this should be:
> for (i = 0 ; i < lp->num_regulators ; i++)

Thanks for your opinion.
In the patch v3, max loop count will be variable in case of PMU.

static struct regulator_init_data
*lp872x_find_regulator_init_data(int idx, struct lp872x *lp)
{
int max_regulators;

switch (lp->chipid) {
case LP8720:
max_regulators = LP8720_NUM_REGULATORS;
break;
case LP8725:
max_regulators = LP8725_NUM_REGULATORS;
break;
default:
return NULL;
}

for (i = 0 ; i < max_regulators ; i++)

...

Thanks & BR
Milo -

2012-06-19 07:01:39

by Kim, Milo

[permalink] [raw]
Subject: RE: [PATCH v2] regulator: add new regulator driver for lp872x


> > + if (chip == LP8720) {
> > + mask = LP8720_TIMESTEP_M;
> > + shift = LP8720_TIMESTEP_S;
> > + time_usec = &lp8720_time_usec[0];
> > + size = ARRAY_SIZE(lp8720_time_usec);
> > + } else if (chip == LP8725) {
> > + mask = LP8725_TIMESTEP_M;
> > + shift = LP8725_TIMESTEP_S;
> > + time_usec = &lp8725_time_usec[0];
> > + size = ARRAY_SIZE(lp8725_time_usec);
> > + } else {
> > + return -EINVAL;
> > + }
>
> This should be a switch statement.
>
> > +static int lp8725_buck_set_current_limit(struct regulator_dev *rdev,
> > + int min_uA, int max_uA)
> > +{
> > + struct lp872x *lp = rdev_get_drvdata(rdev);
> > + enum lp872x_regulator_id buck = rdev_get_id(rdev);
> > + int i, max = ARRAY_SIZE(lp8725_buck_uA);
> > + u8 addr, val;
> > +
> > + if (buck == LP8725_ID_BUCK1)
> > + addr = LP8725_BUCK1_VOUT2;
> > + else if (buck == LP8725_ID_BUCK2)
> > + addr = LP8725_BUCK2_VOUT2;
> > + else
> > + return -EINVAL;
>
> Again, switch statement.
>

In patch v3, some if-statements will be replaced with switch-statements.
Thank you.

Best Regards,
Milo