Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
On Tue, 2012-02-14 at 11:16 -0800, Kevin Hilman wrote: Tero Kristo t-kri...@ti.com writes: On Tue, 2012-01-10 at 19:10 +, Mark Brown wrote: On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote: Yes, some of the control still goes via the normal path (although I forget which, maybe Benoit can remind us), so I think it's best to add the HW control part to each regulator that might uses it. Ideally this could be facilitated by adding the extentions to the regulator core so the amount of code needed for each regulator driver would be minimal. I think the original version of the patch was something along those lines but it was just a general facility which ignored the regulator driver entirely which didn't feel well integrated. The discussion suggested that this wasn't something that'd work with other regulators so a per-driver solution seemed OK. Coming back to this patch now as I have time to look at it, what is the general opinion, is it acceptable to patch the regulator core to add support for the external controller or should I just resend the latest version with changes Mark suggested? This will probably mean that once we add new regulator drivers (e.g. pmics) we may need to duplicate the external controller support here. How about you keep it in the regulator driver for now, and when we need to abstract it out, we make the case for it then. Okay, sounds good to me, once we have a second driver that actually requires this, we have more backing for the request. Currently the regulator core change is difficult to justify. I'll rework the set based on last comments and send it out. -Tero -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
On Tue, 2012-01-10 at 19:10 +, Mark Brown wrote: On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote: Yes, some of the control still goes via the normal path (although I forget which, maybe Benoit can remind us), so I think it's best to add the HW control part to each regulator that might uses it. Ideally this could be facilitated by adding the extentions to the regulator core so the amount of code needed for each regulator driver would be minimal. I think the original version of the patch was something along those lines but it was just a general facility which ignored the regulator driver entirely which didn't feel well integrated. The discussion suggested that this wasn't something that'd work with other regulators so a per-driver solution seemed OK. Coming back to this patch now as I have time to look at it, what is the general opinion, is it acceptable to patch the regulator core to add support for the external controller or should I just resend the latest version with changes Mark suggested? This will probably mean that once we add new regulator drivers (e.g. pmics) we may need to duplicate the external controller support here. -Tero -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
Tero Kristo t-kri...@ti.com writes: On Tue, 2012-01-10 at 19:10 +, Mark Brown wrote: On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote: Yes, some of the control still goes via the normal path (although I forget which, maybe Benoit can remind us), so I think it's best to add the HW control part to each regulator that might uses it. Ideally this could be facilitated by adding the extentions to the regulator core so the amount of code needed for each regulator driver would be minimal. I think the original version of the patch was something along those lines but it was just a general facility which ignored the regulator driver entirely which didn't feel well integrated. The discussion suggested that this wasn't something that'd work with other regulators so a per-driver solution seemed OK. Coming back to this patch now as I have time to look at it, what is the general opinion, is it acceptable to patch the regulator core to add support for the external controller or should I just resend the latest version with changes Mark suggested? This will probably mean that once we add new regulator drivers (e.g. pmics) we may need to duplicate the external controller support here. How about you keep it in the regulator driver for now, and when we need to abstract it out, we make the case for it then. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
On Mon, 2012-01-09 at 17:02 -0800, Kevin Hilman wrote: Tero, Mark Brown broo...@opensource.wolfsonmicro.com writes: On Mon, Dec 19, 2011 at 09:56:50AM +, Girdwood, Liam wrote: Mark will take it for this kernel release (if he's OK with it). Someone would need to send the change to me, prefereably with a subject line which makes it look relevant. Are you taking care of this? Yea, the patch was lost due to misleading subject line. I re-sent the file and there are some pending comments for it, so new version will be written. I've been busy with other work during the last few weeks though so haven't been able to do it. I will hopefully send a new version during the next couple weeks, if the issue you just raised about the generic external controller support can be resolved by then. -Tero -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
Mark Brown broo...@opensource.wolfsonmicro.com writes: On Mon, Jan 09, 2012 at 05:11:11PM -0800, Kevin Hilman wrote: Seems to me like the get/set override should be more generic (part of regulator core) instead of TWL specific? Otherwise, whenever someone hooks up a non-TWL regulator to an OMAP and is using HW control (via VC/VP), they'll have to duplicate all of this stuff in their regulator driver? Frankly I'm not sure I understand how the hardware is supposed to work here - originally the plan was to just add a new regulator driver for the hardware control block which is I guess clean enough but it seems like some of the other control still goes via the normal path. Yes, some of the control still goes via the normal path (although I forget which, maybe Benoit can remind us), so I think it's best to add the HW control part to each regulator that might uses it. Ideally this could be facilitated by adding the extentions to the regulator core so the amount of code needed for each regulator driver would be minimal. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote: Yes, some of the control still goes via the normal path (although I forget which, maybe Benoit can remind us), so I think it's best to add the HW control part to each regulator that might uses it. Ideally this could be facilitated by adding the extentions to the regulator core so the amount of code needed for each regulator driver would be minimal. I think the original version of the patch was something along those lines but it was just a general facility which ignored the regulator driver entirely which didn't feel well integrated. The discussion suggested that this wasn't something that'd work with other regulators so a per-driver solution seemed OK. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
Tero Kristo t-kri...@ti.com writes:
This is needed for SMPS regulators, which use the OMAP voltage
processor for voltage get/set functions instead of the normal I2C
channel. For this purpose, regulator_init_data-driver_data contents
are expanded, it is now a struct which contains function pointers
for the set/get voltage operations, a data pointer for these, and
the previously used features bitmask.
Signed-off-by: Tero Kristo t-kri...@ti.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Liam Girdwood l...@ti.com
Cc: Samuel Ortiz sa...@linux.intel.com
Seems to me like the get/set override should be more generic (part of
regulator core) instead of TWL specific?
Otherwise, whenever someone hooks up a non-TWL regulator to an OMAP and
is using HW control (via VC/VP), they'll have to duplicate all of this
stuff in their regulator driver?
Kevin
---
drivers/mfd/twl-core.c| 16 ++-
drivers/regulator/twl-regulator.c | 39
include/linux/i2c/twl.h |7 ++
3 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 01ecfee..009f62b 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -609,6 +609,8 @@ add_regulator_linked(int num, struct regulator_init_data
*pdata,
unsigned num_consumers, unsigned long features)
{
unsigned sub_chip_id;
+ struct twl_regulator_driver_data drv_data;
+
/* regulator framework demands init_data ... */
if (!pdata)
return NULL;
@@ -618,7 +620,19 @@ add_regulator_linked(int num, struct regulator_init_data
*pdata,
pdata-num_consumer_supplies = num_consumers;
}
- pdata-driver_data = (void *)features;
+ if (pdata-driver_data) {
+ /* If we have existing drv_data, just add the flags */
+ struct twl_regulator_driver_data *tmp;
+ tmp = pdata-driver_data;
+ tmp-features |= features;
+ } else {
+ /* add new driver data struct, used only during init */
+ drv_data.features = features;
+ drv_data.set_voltage = NULL;
+ drv_data.get_voltage = NULL;
+ drv_data.data = NULL;
+ pdata-driver_data = drv_data;
+ }
/* NOTE: we currently ignore regulator IRQs, e.g. for short circuits */
sub_chip_id = twl_map[TWL_MODULE_PM_MASTER].sid;
diff --git a/drivers/regulator/twl-regulator.c
b/drivers/regulator/twl-regulator.c
index 11cc308..29eda40 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -58,6 +58,16 @@ struct twlreg_info {
/* chip specific features */
unsigned long features;
+
+ /*
+ * optional override functions for voltage set/get
+ * these are currently only used for SMPS regulators
+ */
+ int (*get_voltage)(void *data);
+ int (*set_voltage)(void *data, int min_uV);
+
+ /* data passed from board for external get/set voltage */
+ void*data;
};
@@ -522,15 +532,25 @@ twl4030smps_set_voltage(struct regulator_dev *rdev, int
min_uV, int max_uV,
struct twlreg_info *info = rdev_get_drvdata(rdev);
int vsel = DIV_ROUND_UP(min_uV - 60, 12500);
- twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
- vsel);
+ if (info-set_voltage) {
+ return info-set_voltage(info-data, min_uV);
+ } else {
+ twlreg_write(info, TWL_MODULE_PM_RECEIVER,
+ VREG_VOLTAGE_SMPS_4030, vsel);
+ }
+
return 0;
}
static int twl4030smps_get_voltage(struct regulator_dev *rdev)
{
struct twlreg_info *info = rdev_get_drvdata(rdev);
- int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+ int vsel;
+
+ if (info-get_voltage)
+ return info-get_voltage(info-data);
+
+ vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
VREG_VOLTAGE_SMPS_4030);
return vsel * 12500 + 60;
@@ -1052,6 +1072,7 @@ static int __devinit twlreg_probe(struct
platform_device *pdev)
struct regulator_init_data *initdata;
struct regulation_constraints *c;
struct regulator_dev*rdev;
+ struct twl_regulator_driver_data*drvdata;
for (i = 0, info = NULL; i ARRAY_SIZE(twl_regs); i++) {
if (twl_regs[i].desc.id != pdev-id)
@@ -1066,8 +1087,16 @@ static int __devinit twlreg_probe(struct
platform_device *pdev)
if (!initdata)
return -EINVAL;
- /* copy the features into regulator data */
- info-features = (unsigned long)initdata-driver_data;
+ drvdata = initdata-driver_data;
+
+ if (!drvdata)
+ return -EINVAL;
+
+ /* copy the driver data
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
On Mon, Jan 09, 2012 at 05:11:11PM -0800, Kevin Hilman wrote: Seems to me like the get/set override should be more generic (part of regulator core) instead of TWL specific? Otherwise, whenever someone hooks up a non-TWL regulator to an OMAP and is using HW control (via VC/VP), they'll have to duplicate all of this stuff in their regulator driver? Frankly I'm not sure I understand how the hardware is supposed to work here - originally the plan was to just add a new regulator driver for the hardware control block which is I guess clean enough but it seems like some of the other control still goes via the normal path. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
Hi Tero, On Fri, Dec 09, 2011 at 05:29:48PM +0200, Tero Kristo wrote: This is needed for SMPS regulators, which use the OMAP voltage processor for voltage get/set functions instead of the normal I2C channel. For this purpose, regulator_init_data-driver_data contents are expanded, it is now a struct which contains function pointers for the set/get voltage operations, a data pointer for these, and the previously used features bitmask. Looks better than the previous versions. For the MFD part: Acked-by: Samuel Ortiz sa...@linux.intel.com Liam, I assume this is going through your tree ? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
On 19 December 2011 08:33, Samuel Ortiz sa...@linux.intel.com wrote: Hi Tero, On Fri, Dec 09, 2011 at 05:29:48PM +0200, Tero Kristo wrote: This is needed for SMPS regulators, which use the OMAP voltage processor for voltage get/set functions instead of the normal I2C channel. For this purpose, regulator_init_data-driver_data contents are expanded, it is now a struct which contains function pointers for the set/get voltage operations, a data pointer for these, and the previously used features bitmask. Looks better than the previous versions. For the MFD part: Acked-by: Samuel Ortiz sa...@linux.intel.com Liam, I assume this is going through your tree ? Cheers, Samuel. Mark will take it for this kernel release (if he's OK with it). Thanks Liam -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set
On Mon, Dec 19, 2011 at 09:56:50AM +, Girdwood, Liam wrote: Mark will take it for this kernel release (if he's OK with it). Someone would need to send the change to me, prefereably with a subject line which makes it look relevant. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv8 4/5] twl4030: add support for external voltage get/set
This is needed for SMPS regulators, which use the OMAP voltage
processor for voltage get/set functions instead of the normal I2C
channel. For this purpose, regulator_init_data-driver_data contents
are expanded, it is now a struct which contains function pointers
for the set/get voltage operations, a data pointer for these, and
the previously used features bitmask.
Signed-off-by: Tero Kristo t-kri...@ti.com
Cc: Mark Brown broo...@opensource.wolfsonmicro.com
Cc: Liam Girdwood l...@ti.com
Cc: Samuel Ortiz sa...@linux.intel.com
---
drivers/mfd/twl-core.c| 16 ++-
drivers/regulator/twl-regulator.c | 39
include/linux/i2c/twl.h |7 ++
3 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 01ecfee..009f62b 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -609,6 +609,8 @@ add_regulator_linked(int num, struct regulator_init_data
*pdata,
unsigned num_consumers, unsigned long features)
{
unsigned sub_chip_id;
+ struct twl_regulator_driver_data drv_data;
+
/* regulator framework demands init_data ... */
if (!pdata)
return NULL;
@@ -618,7 +620,19 @@ add_regulator_linked(int num, struct regulator_init_data
*pdata,
pdata-num_consumer_supplies = num_consumers;
}
- pdata-driver_data = (void *)features;
+ if (pdata-driver_data) {
+ /* If we have existing drv_data, just add the flags */
+ struct twl_regulator_driver_data *tmp;
+ tmp = pdata-driver_data;
+ tmp-features |= features;
+ } else {
+ /* add new driver data struct, used only during init */
+ drv_data.features = features;
+ drv_data.set_voltage = NULL;
+ drv_data.get_voltage = NULL;
+ drv_data.data = NULL;
+ pdata-driver_data = drv_data;
+ }
/* NOTE: we currently ignore regulator IRQs, e.g. for short circuits */
sub_chip_id = twl_map[TWL_MODULE_PM_MASTER].sid;
diff --git a/drivers/regulator/twl-regulator.c
b/drivers/regulator/twl-regulator.c
index 11cc308..29eda40 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -58,6 +58,16 @@ struct twlreg_info {
/* chip specific features */
unsigned long features;
+
+ /*
+* optional override functions for voltage set/get
+* these are currently only used for SMPS regulators
+*/
+ int (*get_voltage)(void *data);
+ int (*set_voltage)(void *data, int min_uV);
+
+ /* data passed from board for external get/set voltage */
+ void*data;
};
@@ -522,15 +532,25 @@ twl4030smps_set_voltage(struct regulator_dev *rdev, int
min_uV, int max_uV,
struct twlreg_info *info = rdev_get_drvdata(rdev);
int vsel = DIV_ROUND_UP(min_uV - 60, 12500);
- twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
- vsel);
+ if (info-set_voltage) {
+ return info-set_voltage(info-data, min_uV);
+ } else {
+ twlreg_write(info, TWL_MODULE_PM_RECEIVER,
+ VREG_VOLTAGE_SMPS_4030, vsel);
+ }
+
return 0;
}
static int twl4030smps_get_voltage(struct regulator_dev *rdev)
{
struct twlreg_info *info = rdev_get_drvdata(rdev);
- int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+ int vsel;
+
+ if (info-get_voltage)
+ return info-get_voltage(info-data);
+
+ vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
VREG_VOLTAGE_SMPS_4030);
return vsel * 12500 + 60;
@@ -1052,6 +1072,7 @@ static int __devinit twlreg_probe(struct platform_device
*pdev)
struct regulator_init_data *initdata;
struct regulation_constraints *c;
struct regulator_dev*rdev;
+ struct twl_regulator_driver_data*drvdata;
for (i = 0, info = NULL; i ARRAY_SIZE(twl_regs); i++) {
if (twl_regs[i].desc.id != pdev-id)
@@ -1066,8 +1087,16 @@ static int __devinit twlreg_probe(struct platform_device
*pdev)
if (!initdata)
return -EINVAL;
- /* copy the features into regulator data */
- info-features = (unsigned long)initdata-driver_data;
+ drvdata = initdata-driver_data;
+
+ if (!drvdata)
+ return -EINVAL;
+
+ /* copy the driver data into regulator data */
+ info-features = drvdata-features;
+ info-data = drvdata-data;
+ info-set_voltage = drvdata-set_voltage;
+ info-get_voltage = drvdata-get_voltage;
/* Constrain board-specific capabilities according to what
* this driver and the chip itself can actually do.
