Hi, Let me summarise the main problem I have been facing with regulators. The TPS6235x are I2C based devices. The regulator_register() functions require that the regulator_init_data is passed as platform_data(). But for i2c_client's platform data is initialized to some other value in the I2C driver. So the regulator_register() function fails. The other problem which I encounter is with the function regulator_get(). Regulator_get() requires 2 parameters - device pointer should point to the One passed during regulator_register. It also takes a string a second parameter Which is compared with the supply string initialized in the regulator init data element regulator_consumer_supply.supply. Passing the client->dev, invoking regulator_register() fails. Regards Mani > -----Original Message----- > From: David Brownell [mailto:david-b@xxxxxxxxxxx] > Sent: Wednesday, January 14, 2009 5:49 AM > To: Pillai, Manikandan > Cc: linux-omap@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxxxxx > Subject: Re: [PATCH 1/2] Include TPS6235x based Power regulator support > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
