Feedback-in-the-form-of-a-patch ...
- Dave
Fives various problems with the latest tps6235x driver patch:
- Remove most board-specific comments, policy, and infrastructure
- Let it compile as a module; useful for built tests etc
- Support the other five values of "x", not just "2" and "3"
- Implement the missing register read/write operations
- Partial bugfix to is_enabled() ... fault handling is unclear
Initialization still looks iffy to me; it's making assumptions that
may not be correct in any given system. There's a comment saying how
to address that using the regulator_init_data.
---
drivers/regulator/Kconfig | 12 -
drivers/regulator/tps6235x-regulator.c | 353 ++++++++++++++++++-------------
2 files changed, 211 insertions(+), 154 deletions(-)
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -81,13 +81,11 @@ config REGULATOR_DA903X
Dialog Semiconductor DA9030/DA9034 PMIC.
config REGULATOR_TPS6235X
- bool "TPS6235X Power regulator for OMAP3EVM"
- depends on I2C=y
+ tristate "TI TPS6235x Power regulators"
+ depends on I2C
help
- This driver supports the voltage regulators provided by TPS6235x
chips.
- The TPS62352 and TPS62353 are mounted on PR785 Power module card for
- providing voltage regulator functions. The PR785 Power board from
- Texas Instruments Inc is a TPS6235X based power card used with OMAP3
- EVM boards.
+ This driver supports TPS6235x voltage regulator chips, for values
+ of "x" from 0 to 6. These are buck converters which support TI's
+ hardware based "SmartReflex" dynamic voltage scaling.
endif
--- a/drivers/regulator/tps6235x-regulator.c
+++ b/drivers/regulator/tps6235x-regulator.c
@@ -19,39 +19,17 @@
#include <linux/i2c.h>
#include <linux/delay.h>
-int tps_6235x_read_reg(struct i2c_client *client, u8 reg, u8 *val);
-int tps_6235x_write_reg(struct i2c_client *client, u8 reg, u8 val);
-extern struct regulator_consumer_supply tps62352_core_consumers;
-extern struct regulator_consumer_supply tps62352_mpu_consumers;
-
-/* Minimum and Maximum dc-dc voltage supported by the TPS6235x devices
-All voltages given in millivolts */
-#define TPS62352_MIN_CORE_VOLT 750
-#define TPS62352_MAX_CORE_VOLT 1537
-#define TPS62353_MIN_MPU_VOLT 750
-#define TPS62353_MAX_MPU_VOLT 1537
-/* Register bit settings */
-#define TPS6235X_EN_DCDC (0x1 << 0x7)
-#define TPS6235X_VSM_MSK (0x3F)
-#define TPS6235X_EN_SYN_MSK (0x1 << 0x5)
-#define TPS6235X_SW_VOLT_MSK (0x1 << 0x4)
-#define TPS6235X_PWR_OK_MSK (0x1 << 0x5)
-#define TPS6235X_OUT_DIS_MSK (0x1 << 0x6)
-#define TPS6235X_GO_MSK (0x1 << 0x7)
-
-#define MODULE_NAME "tps_6235x_pwr"
/*
* These chips are often used in OMAP-based systems.
*
* This driver implements software-based resource control for various
* voltage regulators. This is usually augmented with state machine
* based control.
- */
-
-/* LDO control registers ... offset is from the base of its register bank.
- * The first three registers of all power resource banks help hardware to
- * manage the various resource groups.
+ *
+ * For now, all regulator operations apply to VSEL1 (the "ceiling"),
+ * instead of VSEL0 (the "floor") which is used for low power modes.
+ * Also, this *assumes* only software mode control is used...
*/
#define TPS6235X_REG_VSEL0 0
@@ -59,37 +37,74 @@ All voltages given in millivolts */
#define TPS6235X_REG_CTRL1 2
#define TPS6235X_REG_CTRL2 3
-/* Device addresses for TPS devices */
-#define TPS_62352_CORE_ADDR 0x4A
-#define TPS_62353_MPU_ADDR 0x48
+/* VSEL bitfields (EN_DCDC is shared) */
+#define TPS6235X_EN_DCDC BIT(7)
+#define TPS6235X_LIGHTPFM BIT(6)
+#define TPS6235X_VSM_MSK (0x3F)
-int omap_i2c_match_child(struct device *dev, void *data);
+/* CTRL1 bitfields */
+#define TPS6235X_EN_SYNC BIT(5)
+#define TPS6235X_HW_nSW BIT(4)
+/* REVISIT plus mode controls */
+
+/* CTRL2 bitfields */
+#define TPS6235X_PWR_OK_MSK BIT(5)
+#define TPS6235X_OUT_DIS_MSK BIT(6)
+#define TPS6235X_GO_MSK BIT(7)
+
+struct tps_info {
+ unsigned min_uV;
+ unsigned max_uV;
+ unsigned mult_uV;
+ bool fixed;
+};
+
+struct tps {
+ struct regulator_desc desc;
+ struct i2c_client *client;
+ struct regulator_dev *rdev;
+ const struct tps_info *info;
+};
+
+
+static inline int tps_6235x_read_reg(struct tps *tps, u8 reg, u8 *val)
+{
+ int status;
+
+ status = i2c_smbus_read_byte_data(tps->client, reg);
+ *val = status;
+ if (status < 0)
+ return status;
+ return 0;
+}
+
+static inline int tps_6235x_write_reg(struct tps *tps, u8 reg, u8 val)
+{
+ return i2c_smbus_write_byte_data(tps->client, reg, val);
+}
static int tps6235x_dcdc_is_enabled(struct regulator_dev *dev)
{
unsigned char vsel1;
int ret;
- struct i2c_client *tps_info =
- rdev_get_drvdata(dev);
- ret = tps_6235x_read_reg(tps_info, TPS6235X_REG_VSEL1, &vsel1);
- ret &= TPS6235X_EN_DCDC;
- if (ret)
- return 1;
- else
- return 0;
+ struct tps *tps = rdev_get_drvdata(dev);
+
+ ret = tps_6235x_read_reg(tps, TPS6235X_REG_VSEL1, &vsel1);
+ /* REVISIT we need to be able to report errors here ... */
+
+ return !!(vsel1 & TPS6235X_EN_DCDC);
}
static int tps6235x_dcdc_enable(struct regulator_dev *dev)
{
unsigned char vsel1;
int ret;
+ struct tps *tps = rdev_get_drvdata(dev);
- struct i2c_client *client = rdev_get_drvdata(dev);
-
- ret = tps_6235x_read_reg(client, TPS6235X_REG_VSEL1, &vsel1);
+ ret = tps_6235x_read_reg(tps, TPS6235X_REG_VSEL1, &vsel1);
if (ret == 0) {
vsel1 |= TPS6235X_EN_DCDC;
- ret = tps_6235x_write_reg(client, TPS6235X_REG_VSEL1, vsel1);
+ ret = tps_6235x_write_reg(tps, TPS6235X_REG_VSEL1, vsel1);
}
return ret;
}
@@ -98,164 +113,216 @@ static int tps6235x_dcdc_disable(struct
{
unsigned char vsel1;
int ret;
- struct i2c_client *client = rdev_get_drvdata(dev);
+ struct tps *tps = rdev_get_drvdata(dev);
- ret = tps_6235x_read_reg(client, TPS6235X_REG_VSEL1, &vsel1);
+ ret = tps_6235x_read_reg(tps, TPS6235X_REG_VSEL1, &vsel1);
if (ret == 0) {
vsel1 &= ~(TPS6235X_EN_DCDC);
- ret = tps_6235x_write_reg(client, TPS6235X_REG_VSEL1, vsel1);
+ ret = tps_6235x_write_reg(tps, TPS6235X_REG_VSEL1, vsel1);
}
return ret;
}
static int tps6235x_dcdc_get_voltage(struct regulator_dev *dev)
{
- struct i2c_client *tps_info = rdev_get_drvdata(dev);
+ struct tps *tps = rdev_get_drvdata(dev);
+ const struct tps_info *info = tps->info;
+ int status;
unsigned char vsel1;
- unsigned int volt;
/* Read the VSEL1 register to get VSM */
- tps_6235x_read_reg(tps_info, TPS6235X_REG_VSEL1, &vsel1);
- /* Output voltage set is = min_op_volt + ( VSM * 12.5mv) */
- /* To cut out floating point operation we will multiply by 25
- divide by 2 */
- volt = (((vsel1 & TPS6235X_VSM_MSK) * 25) / 2) + TPS62352_MIN_CORE_VOLT;
+ status = tps_6235x_read_reg(tps, TPS6235X_REG_VSEL1, &vsel1);
+ if (status < 0)
+ return status;
- return volt * 1000;
+ return info->min_uV + ((vsel1 & TPS6235X_VSM_MSK) * info->mult_uV);
}
static int tps6235x_dcdc_set_voltage(struct regulator_dev *dev,
int min_uV, int max_uV)
{
+ struct tps *tps = rdev_get_drvdata(dev);
+ const struct tps_info *info = tps->info;
unsigned char vsel1;
- unsigned int volt;
- struct i2c_client *tps_info = rdev_get_drvdata(dev);
- unsigned int millivolts = min_uV / 1000;
+ unsigned step;
+ int status;
- /* check if the millivolts is within range */
- if ((millivolts < TPS62352_MIN_CORE_VOLT) ||
- (millivolts > TPS62352_MAX_CORE_VOLT))
+ /* adjust to match supported range, fail if out of range */
+ if (min_uV < info->min_uV)
+ min_uV = info->min_uV;
+ if (max_uV > info->max_uV)
+ max_uV = info->min_uV;
+ if (min_uV > max_uV)
return -EINVAL;
- /* Output voltage set is = min_op_volt + ( VSM * 12.5mv) */
- volt = millivolts - TPS62352_MIN_CORE_VOLT;
- volt /= 25;
- volt *= 2;
- vsel1 = ((TPS6235X_EN_DCDC) | (volt & TPS6235X_VSM_MSK));
- tps_6235x_write_reg(tps_info, TPS6235X_REG_VSEL1, vsel1);
- return 0;
+ /* compute and sanity-check voltage step multiplier */
+ step = DIV_ROUND_UP(min_uV - info->min_uV, info->mult_uV);
+ if ((info->min_uV + (step * info->mult_uV)) > max_uV)
+ return -EINVAL;
+
+ status = tps_6235x_read_reg(tps, TPS6235X_REG_VSEL1, &vsel1);
+ if (status < 0)
+ return status;
+
+ /* update voltage */
+ vsel1 &= ~TPS6235X_VSM_MSK;
+ vsel1 |= step;
+ return tps_6235x_write_reg(tps, TPS6235X_REG_VSEL1, vsel1);
}
-static struct regulator_ops tps62352_dcdc_ops = {
- .is_enabled = tps6235x_dcdc_is_enabled,
- .get_voltage = tps6235x_dcdc_get_voltage,
- .set_voltage = tps6235x_dcdc_set_voltage,
-};
+/* tps6345{0,2,4,5} have some parameters hard-wired */
+static struct regulator_ops tps6235x_fixed_dcdc_ops = {
+ .is_enabled = tps6235x_dcdc_is_enabled,
+ .get_voltage = tps6235x_dcdc_get_voltage,
+ .set_voltage = tps6235x_dcdc_set_voltage,
-static struct regulator_ops tps62353_dcdc_ops = {
- .is_enabled = tps6235x_dcdc_is_enabled,
- .enable = tps6235x_dcdc_enable,
- .disable = tps6235x_dcdc_disable,
- .get_voltage = tps6235x_dcdc_get_voltage,
- .set_voltage = tps6235x_dcdc_set_voltage,
+ /* REVISIT these can support regulator mode operations too */
};
-static struct regulator_desc regulators[] = {
- {
- .name = "tps62352",
- .id = 2,
- .ops = &tps62352_dcdc_ops,
- .type = REGULATOR_VOLTAGE,
- .owner = THIS_MODULE,
- },
- {
- .name = "tps62353",
- .id = 3,
- .ops = &tps62353_dcdc_ops,
- .type = REGULATOR_VOLTAGE,
- .owner = THIS_MODULE,
- },
-};
+/* tps6345{1,3,6} are more programmable */
+static struct regulator_ops tps6235x_dcdc_ops = {
+ .is_enabled = tps6235x_dcdc_is_enabled,
+ .enable = tps6235x_dcdc_enable,
+ .disable = tps6235x_dcdc_disable,
+ .get_voltage = tps6235x_dcdc_get_voltage,
+ .set_voltage = tps6235x_dcdc_set_voltage,
-static const char *regulator_consumer_name[] = {
- "vdd2",
- "vdd1",
+ /* REVISIT these can support regulator mode operations too */
};
static
int tps_6235x_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
- struct regulator_dev *rdev = NULL;
+ static int desc_id;
+
+ const struct tps_info *info = (void *)id->driver_data;
+ struct regulator_init_data *init_data;
+ struct regulator_dev *rdev;
+ struct tps *tps;
unsigned char reg_val;
- struct device *dev_child = NULL;
- tps_6235x_read_reg(client, TPS6235X_REG_CTRL2, ®_val);
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+ return -EIO;
+
+ init_data = client->dev.platform_data;
+ if (!init_data)
+ return -EIO;
+
+ tps = kzalloc(sizeof(*tps), GFP_KERNEL);
+ if (!tps)
+ return -ENOMEM;
+
+ tps->desc.name = id->name;
+ tps->desc.id = desc_id++;
+ tps->desc.ops = info->fixed ? &tps6235x_fixed_dcdc_ops :
&tps6235x_dcdc_ops;
+ tps->desc.type = REGULATOR_VOLTAGE;
+ tps->desc.owner = THIS_MODULE;
+
+ tps->client = client;
+ tps->info = info;
+
+ /* FIXME board init code should provide init_data->driver_data
+ * saying how to configure this regulator: how big is the
+ * inductor (affects light PFM mode optimization), slew rate,
+ * PLL multiplier, and so forth.
+ */
+ tps_6235x_read_reg(tps, TPS6235X_REG_CTRL2, ®_val);
reg_val |= (TPS6235X_OUT_DIS_MSK | TPS6235X_GO_MSK);
- tps_6235x_write_reg(client, TPS6235X_REG_CTRL2, reg_val);
- tps_6235x_read_reg(client, TPS6235X_REG_CTRL2, ®_val);
+ tps_6235x_write_reg(tps, TPS6235X_REG_CTRL2, reg_val);
+ tps_6235x_read_reg(tps, TPS6235X_REG_CTRL2, ®_val);
if (reg_val & TPS6235X_PWR_OK_MSK)
dev_dbg(&client->dev, "Power is OK %x\n", reg_val);
- else {
+ else
dev_dbg(&client->dev, "Power not in range = %x\n", reg_val);
- return -EIO;
- }
-
- /* Register the regulators */
- dev_child = device_find_child(client->adapter->dev.parent,
- (void *)regulator_consumer_name[id->driver_data],
- omap_i2c_match_child);
- rdev = regulator_register(®ulators[id->driver_data],
- dev_child, client);
+ /* Register the regulator */
+ rdev = regulator_register(&tps->desc, &client->dev, tps);
if (IS_ERR(rdev)) {
- dev_err(dev_child, "failed to register %s\n",
- regulator_consumer_name[id->driver_data]);
+ dev_err(&client->dev, "failed to register %s\n", id->name);
+ kfree(tps);
return PTR_ERR(rdev);
}
- /* Set the regulator platform data for unregistration later on */
- i2c_set_clientdata(client, rdev);
+ /* Save regulator for cleanup */
+ tps->rdev = rdev;
+ i2c_set_clientdata(client, tps);
return 0;
}
-/**
- * tps_6235x_remove - TPS6235x driver i2c remove handler
- * @client: i2c driver client device structure
- *
- * Unregister TPS driver as an i2c client device driver
- */
-static int __exit tps_6235x_remove(struct i2c_client *client)
+static int __devexit tps_6235x_remove(struct i2c_client *client)
{
- struct regulator_dev *rdev = NULL;
-
- if (!client->adapter)
- return -ENODEV; /* our client isn't attached */
+ struct tps *tps = i2c_get_clientdata(client);
- rdev = (struct regulator_dev *)i2c_get_clientdata(client);
- regulator_unregister(rdev);
- /* clear the client data in i2c */
+ regulator_unregister(tps->rdev);
+ kfree(tps);
i2c_set_clientdata(client, NULL);
-
return 0;
}
+/*
+ * These regulators have the same register structure, and differ
+ * primarily according to supported voltages and default settings.
+ */
+static const struct tps_info tps62350_info = {
+ .min_uV = 750000,
+ .max_uV = 1537500,
+ .mult_uV = 12500,
+ .fixed = true,
+};
+static const struct tps_info tps62351_info = {
+ .min_uV = 900000,
+ .max_uV = 1687500,
+ .mult_uV = 12500,
+};
+static const struct tps_info tps62352_info = {
+ .min_uV = 750000,
+ .max_uV = 1437500,
+ .mult_uV = 12500,
+ .fixed = true,
+};
+static const struct tps_info tps62353_info = {
+ .min_uV = 750000,
+ .max_uV = 1537500,
+ .mult_uV = 12500,
+};
+static const struct tps_info tps62354_info = {
+ .min_uV = 750000,
+ .max_uV = 1537500,
+ .mult_uV = 12500,
+ .fixed = true,
+};
+static const struct tps_info tps62355_info = {
+ .min_uV = 750000,
+ .max_uV = 1537500,
+ .mult_uV = 12500,
+ .fixed = true,
+};
+static const struct tps_info tps62356_info = {
+ .min_uV = 1500000,
+ .max_uV = 1975000,
+ .mult_uV = 25000,
+};
+
static const struct i2c_device_id tps_6235x_id[] = {
- { "tps62352", 0},
- { "tps62353", 1},
+ { "tps62350", (unsigned long) &tps62350_info, },
+ { "tps62351", (unsigned long) &tps62351_info, },
+ { "tps62352", (unsigned long) &tps62352_info, },
+ { "tps62353", (unsigned long) &tps62353_info, },
+ { "tps62354", (unsigned long) &tps62354_info, },
+ { "tps62355", (unsigned long) &tps62355_info, },
+ { "tps62356", (unsigned long) &tps62356_info, },
{},
};
-
MODULE_DEVICE_TABLE(i2c, tps_6235x_id);
static struct i2c_driver tps_6235x_i2c_driver = {
.driver = {
- .name = MODULE_NAME,
- .owner = THIS_MODULE,
+ .name = "tps_6235x_pwr"
},
.probe = tps_6235x_probe,
- .remove = __exit_p(tps_6235x_remove),
+ .remove = __devexit_p(tps_6235x_remove),
.id_table = tps_6235x_id,
};
@@ -266,15 +333,9 @@ static struct i2c_driver tps_6235x_i2c_d
*/
static int __init tps_6235x_init(void)
{
- int err;
-
- err = i2c_add_driver(&tps_6235x_i2c_driver);
- if (err) {
- printk(KERN_ERR "Failed to register " MODULE_NAME ".\n");
- return err;
- }
- return 0;
+ return i2c_add_driver(&tps_6235x_i2c_driver);
}
+subsys_initcall(tps_6235x_init);
/**
@@ -286,10 +347,8 @@ static void __exit tps_6235x_cleanup(voi
{
i2c_del_driver(&tps_6235x_i2c_driver);
}
-
-late_initcall(tps_6235x_init);
module_exit(tps_6235x_cleanup);
MODULE_AUTHOR("Texas Instruments");
-MODULE_DESCRIPTION("TPS6235x based linux driver");
+MODULE_DESCRIPTION("TPS6235x voltage regulator driver");
MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
