| Message ID | 1310565638-13140-3-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jul 13, 2011 at 05:00:35PM +0300, Tero Kristo wrote: > +config REGULATOR_OMAP_SMPS > + tristate "TI OMAP SMPS Power Regulators" > + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM && TWL4030_CORE What is the dependency on the TWL4030? I see no references to it in the code... > + for (i = 0; i < pdata->num_regulators; i++) { > + initdata = pdata->regulators[i]; > + I do strongly prefer the idiom of just registering all the regulators even if they're read only. > + c = &initdata->constraints; > + c->valid_modes_mask &= REGULATOR_MODE_NORMAL; > + c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE; > + c->always_on = true; No, this is bad. We *always* pay attention to the constraints the user set even if they're nuts or won't work, the machine driver has the final say on what is or isn't allowed on a given board. The mode setting is especially suspect as there's no mode support in the driver. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-07-13 at 16:40 +0200, Mark Brown wrote: > On Wed, Jul 13, 2011 at 05:00:35PM +0300, Tero Kristo wrote: > > > +config REGULATOR_OMAP_SMPS > > + tristate "TI OMAP SMPS Power Regulators" > > + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM && TWL4030_CORE > > What is the dependency on the TWL4030? I see no references to it in the > code... Hmm, true... I was mainly thinking about the HW setup where we usually have a TWL family chip which is controlled by the SMPS regulator driver. I think that one can actually be dropped as it might be possible to use some other power IC behind the SMPS channels. I think I'll remove all the references to TWL4030 / TWL6030 from this patch. > > > + for (i = 0; i < pdata->num_regulators; i++) { > > + initdata = pdata->regulators[i]; > > + > > I do strongly prefer the idiom of just registering all the regulators > even if they're read only. Number of available SMPS regulators is kind of board specific issue. OMAP3 has 2 available, OMAP4 has 3. If we are using some custom powering solution, we might have even different amounts for these. > > > + c = &initdata->constraints; > > + c->valid_modes_mask &= REGULATOR_MODE_NORMAL; > > + c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE; > > + c->always_on = true; > > No, this is bad. We *always* pay attention to the constraints the user > set even if they're nuts or won't work, the machine driver has the final > say on what is or isn't allowed on a given board. The mode setting is > especially suspect as there's no mode support in the driver. Just a clarification on this one that I have understood your comment right... Do you mean that I should be checking the constraints user sets more thoroughly to see if there is something bogus? I was looking at some of the other regulator drivers and they seem to be fiddling with the constraints in similar manner. -Tero Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 13, 2011 at 06:53:45PM +0300, Tero Kristo wrote: > On Wed, 2011-07-13 at 16:40 +0200, Mark Brown wrote: > > I do strongly prefer the idiom of just registering all the regulators > > even if they're read only. > Number of available SMPS regulators is kind of board specific issue. > OMAP3 has 2 available, OMAP4 has 3. If we are using some custom powering > solution, we might have even different amounts for these. Right, but the interface to them is always there? > > No, this is bad. We *always* pay attention to the constraints the user > > set even if they're nuts or won't work, the machine driver has the final > > say on what is or isn't allowed on a given board. The mode setting is > > especially suspect as there's no mode support in the driver. > Just a clarification on this one that I have understood your comment > right... Do you mean that I should be checking the constraints user sets > more thoroughly to see if there is something bogus? I was looking at > some of the other regulator drivers and they seem to be fiddling with > the constraints in similar manner. No! You should *always* use the constraints the user has set, don't randomly add new permissions without them doing so. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 13, 2011 at 05:00:35PM +0300, Tero Kristo wrote: > OMAP SMPS regulator driver provides access to OMAP voltage processor > controlled regulators. These include VDD1 and VDD2 for OMAP3 and additionally > VDD3 for OMAP4. SMPS regulators use the OMAP voltage layer for the actual > voltage regulation operations. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- ... > + for (i = 0; i < pdata->num_regulators; i++) { > + initdata = pdata->regulators[i]; > + > + c = &initdata->constraints; > + c->valid_modes_mask &= REGULATOR_MODE_NORMAL; > + c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE; > + c->always_on = true; > + > + voltdm = omap_voltage_domain_lookup( > + initdata->consumer_supplies[0].dev_name); > + > + if (IS_ERR(voltdm)) { > + dev_err(&pdev->dev, "can't find voltdm %s, %ld\n", > + initdata->consumer_supplies[0].dev_name, > + PTR_ERR(voltdm)); > + return PTR_ERR(voltdm); Or maybe continue to next loop iteration. > + } > + info = kzalloc(sizeof(struct omap_smps_reg_info), GFP_KERNEL); > + > + info->voltdm = voltdm; Check kzalloc NULL return. > + info->desc.ops = &omap_smps_ops; > + info->desc.name = c->name; > + info->desc.type = REGULATOR_VOLTAGE; > + info->desc.n_voltages = 0; > + rdev = regulator_register(&info->desc, &pdev->dev, initdata, > + info); > + if (IS_ERR(rdev)) { > + dev_err(&pdev->dev, "can't register %s, %ld\n", > + info->desc.name, PTR_ERR(rdev)); > + return PTR_ERR(rdev); Or suggest continue to next loop iteration. > + } > + platform_set_drvdata(pdev, rdev); Performed in a loop, but only last iteration's rdev is set as the driver data for pdev. Platform driver data should be pdata? > + } > + > + return 0; > +} > + > +static int omap_smps_reg_remove(struct platform_device *pdev) > +{ > + regulator_unregister(platform_get_drvdata(pdev)); Should kfree() the struct omap_smps_reg_info data structure(s) and platform_set_drvdata(pdev, NULL) ? Todd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-07-14 at 08:29 +0200, Todd Poynor wrote: > On Wed, Jul 13, 2011 at 05:00:35PM +0300, Tero Kristo wrote: > > OMAP SMPS regulator driver provides access to OMAP voltage processor > > controlled regulators. These include VDD1 and VDD2 for OMAP3 and additionally > > VDD3 for OMAP4. SMPS regulators use the OMAP voltage layer for the actual > > voltage regulation operations. > > > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > > --- > ... > > + for (i = 0; i < pdata->num_regulators; i++) { > > + initdata = pdata->regulators[i]; > > + > > + c = &initdata->constraints; > > + c->valid_modes_mask &= REGULATOR_MODE_NORMAL; > > + c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE; > > + c->always_on = true; > > + > > + voltdm = omap_voltage_domain_lookup( > > + initdata->consumer_supplies[0].dev_name); > > + > > + if (IS_ERR(voltdm)) { > > + dev_err(&pdev->dev, "can't find voltdm %s, %ld\n", > > + initdata->consumer_supplies[0].dev_name, > > + PTR_ERR(voltdm)); > > + return PTR_ERR(voltdm); > > > Or maybe continue to next loop iteration. Not sure about this. If one of the regulators fails, it is probably better to fail/disable them all, otherwise we end up in partly functioning situation. I think I'll just change this into a setup where all the SMPS regulators are disabled if init of any of them fails. > > > + } > > + info = kzalloc(sizeof(struct omap_smps_reg_info), GFP_KERNEL); > > + > > + info->voltdm = voltdm; > > > Check kzalloc NULL return. True. > > > + info->desc.ops = &omap_smps_ops; > > + info->desc.name = c->name; > > + info->desc.type = REGULATOR_VOLTAGE; > > + info->desc.n_voltages = 0; > > + rdev = regulator_register(&info->desc, &pdev->dev, initdata, > > + info); > > + if (IS_ERR(rdev)) { > > + dev_err(&pdev->dev, "can't register %s, %ld\n", > > + info->desc.name, PTR_ERR(rdev)); > > + return PTR_ERR(rdev); > > Or suggest continue to next loop iteration. Same as above continue comment. > > > + } > > + platform_set_drvdata(pdev, rdev); > > Performed in a loop, but only last iteration's rdev is set as the > driver data for pdev. Platform driver data should be pdata? I'll check what the cleanup part needs and fix it. > > > + } > > + > > + return 0; > > +} > > + > > +static int omap_smps_reg_remove(struct platform_device *pdev) > > +{ > > + regulator_unregister(platform_get_drvdata(pdev)); > > > Should kfree() the struct omap_smps_reg_info data structure(s) and > platform_set_drvdata(pdev, NULL) ? Same as above. > > > Todd Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-07-14 at 00:55 +0200, Mark Brown wrote: > On Wed, Jul 13, 2011 at 06:53:45PM +0300, Tero Kristo wrote: > > On Wed, 2011-07-13 at 16:40 +0200, Mark Brown wrote: > > > > I do strongly prefer the idiom of just registering all the regulators > > > even if they're read only. > > > Number of available SMPS regulators is kind of board specific issue. > > OMAP3 has 2 available, OMAP4 has 3. If we are using some custom powering > > solution, we might have even different amounts for these. > > Right, but the interface to them is always there? The library used in this driver will attempt to lookup for the voltagedomains, and this will fail for 'iva' on omap3, as it does not exist. I could change the driver to always try to look for all of the possible known domains, and just register the ones it finds, and apply user settings for the ones that board file provides. This contradicts now a bit on the comment I just said to Todd, but you believe this to be a better way? > > > > No, this is bad. We *always* pay attention to the constraints the user > > > set even if they're nuts or won't work, the machine driver has the final > > > say on what is or isn't allowed on a given board. The mode setting is > > > especially suspect as there's no mode support in the driver. > > > Just a clarification on this one that I have understood your comment > > right... Do you mean that I should be checking the constraints user sets > > more thoroughly to see if there is something bogus? I was looking at > > some of the other regulator drivers and they seem to be fiddling with > > the constraints in similar manner. > > No! You should *always* use the constraints the user has set, don't > randomly add new permissions without them doing so. Ah ok, so no fiddling with the constraints at all, core should take care of proper functioning regarding this part. -Tero Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index d7ed20f..bb18ff2 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -303,5 +303,14 @@ config REGULATOR_TPS65910 help This driver supports TPS65910 voltage regulator chips. +config REGULATOR_OMAP_SMPS + tristate "TI OMAP SMPS Power Regulators" + depends on (ARCH_OMAP3 || ARCH_OMAP4) && PM && TWL4030_CORE + help + This driver supports the OMAP3 / OMAP4 SMPS regulators for VDD1, + VDD2 and VDD3. These regulators reside inside the TWL4030 / + TWL6030 chip but are accessed using the voltage processor + interface of OMAP. + endif diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 3932d2e..191e3d5 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -43,5 +43,6 @@ obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o obj-$(CONFIG_REGULATOR_AB8500) += ab8500.o obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o +obj-$(CONFIG_REGULATOR_OMAP_SMPS) += omap-smps-regulator.o ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG diff --git a/drivers/regulator/omap-smps-regulator.c b/drivers/regulator/omap-smps-regulator.c new file mode 100644 index 0000000..88009b1 --- /dev/null +++ b/drivers/regulator/omap-smps-regulator.c @@ -0,0 +1,126 @@ +/* + * omap-vp-regulator.c -- support SMPS regulators for OMAP chips + * + * Copyright (C) 2011 Texas Instruments, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/delay.h> +#include <linux/platform_device.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> +#include <linux/regulator/omap-smps.h> +#include <plat/voltage.h> + +#define DRIVER_NAME "omap-smps" + +struct omap_smps_reg_info { + struct voltagedomain *voltdm; + struct regulator_desc desc; +}; + +static int omap_smps_set_voltage(struct regulator_dev *rdev, int min_uV, + int max_uV, unsigned *selector) +{ + struct omap_smps_reg_info *info = rdev_get_drvdata(rdev); + return omap_voltage_scale_vdd(info->voltdm, min_uV); +} + +static int omap_smps_get_voltage(struct regulator_dev *rdev) +{ + struct omap_smps_reg_info *info = rdev_get_drvdata(rdev); + return omap_vp_get_curr_volt(info->voltdm); +} + +static struct regulator_ops omap_smps_ops = { + .set_voltage = omap_smps_set_voltage, + .get_voltage = omap_smps_get_voltage, +}; + +static int __devinit omap_smps_reg_probe(struct platform_device *pdev) +{ + int i; + struct omap_smps_reg_info *info; + struct omap_smps_platform_data *pdata; + struct regulation_constraints *c; + struct regulator_dev *rdev; + struct regulator_init_data *initdata; + struct voltagedomain *voltdm; + + pdata = pdev->dev.platform_data; + + for (i = 0; i < pdata->num_regulators; i++) { + initdata = pdata->regulators[i]; + + c = &initdata->constraints; + c->valid_modes_mask &= REGULATOR_MODE_NORMAL; + c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE; + c->always_on = true; + + voltdm = omap_voltage_domain_lookup( + initdata->consumer_supplies[0].dev_name); + + if (IS_ERR(voltdm)) { + dev_err(&pdev->dev, "can't find voltdm %s, %ld\n", + initdata->consumer_supplies[0].dev_name, + PTR_ERR(voltdm)); + return PTR_ERR(voltdm); + } + info = kzalloc(sizeof(struct omap_smps_reg_info), GFP_KERNEL); + + info->voltdm = voltdm; + info->desc.ops = &omap_smps_ops; + info->desc.name = c->name; + info->desc.type = REGULATOR_VOLTAGE; + info->desc.n_voltages = 0; + rdev = regulator_register(&info->desc, &pdev->dev, initdata, + info); + if (IS_ERR(rdev)) { + dev_err(&pdev->dev, "can't register %s, %ld\n", + info->desc.name, PTR_ERR(rdev)); + return PTR_ERR(rdev); + } + platform_set_drvdata(pdev, rdev); + } + + return 0; +} + +static int omap_smps_reg_remove(struct platform_device *pdev) +{ + regulator_unregister(platform_get_drvdata(pdev)); + return 0; +} + +static struct platform_driver omap_smps_reg_driver = { + .probe = omap_smps_reg_probe, + .remove = __devexit_p(omap_smps_reg_remove), + .driver.name = DRIVER_NAME, + .driver.owner = THIS_MODULE, +}; + +static int __init omap_smps_reg_init(void) +{ + return platform_driver_register(&omap_smps_reg_driver); +} +subsys_initcall(omap_smps_reg_init); + +static void __exit omap_smps_reg_exit(void) +{ + platform_driver_unregister(&omap_smps_reg_driver); +} +module_exit(omap_smps_reg_exit); + +MODULE_ALIAS("platform:"DRIVER_NAME); +MODULE_AUTHOR("Tero Kristo <t-kristo@ti.com>"); +MODULE_DESCRIPTION("OMAP SMPS regulator driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/regulator/omap-smps.h b/include/linux/regulator/omap-smps.h new file mode 100644 index 0000000..1d5f940 --- /dev/null +++ b/include/linux/regulator/omap-smps.h @@ -0,0 +1,20 @@ +/* + * omap-smps.h - header for OMAP SMPS regulator support + * + * Copyright (C) 2011 Texas Instruments, Inc. + * + * + * 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 __OMAP_SMPS_H__ +#define __OMAP_SMPS_H__ + +struct omap_smps_platform_data { + struct regulator_init_data **regulators; + int num_regulators; +}; + +#endif /* End of __OMAP_SMPS_H__ */
OMAP SMPS regulator driver provides access to OMAP voltage processor controlled regulators. These include VDD1 and VDD2 for OMAP3 and additionally VDD3 for OMAP4. SMPS regulators use the OMAP voltage layer for the actual voltage regulation operations. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/regulator/Kconfig | 9 ++ drivers/regulator/Makefile | 1 + drivers/regulator/omap-smps-regulator.c | 126 +++++++++++++++++++++++++++++++ include/linux/regulator/omap-smps.h | 20 +++++ 4 files changed, 156 insertions(+), 0 deletions(-) create mode 100644 drivers/regulator/omap-smps-regulator.c create mode 100644 include/linux/regulator/omap-smps.h