Message ID | 1347977875-16855-5-git-send-email-pawel.moll@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Sep 18, 2012 at 03:17:46PM +0100, Pawel Moll wrote: > Implementation of the regulator framework driver for the > Versatile Express voltage control. Devices without > voltage constraints (ie. "regulator-[min|max]-microvolt" > properties in the DT node) are treated as fixed (or rather > read-only) regulators. This doesn't seem great... it doesn't seem to know or represent anything at all about the hardware, I'd expect a voltage regulator to at a minimum be able to implement list_voltage(). You've not provided any information on what the hardware actually is and the driver just seems to proxy through to some other API which actually implements the regulator support. > + init_data->constraints.apply_uV = 0; This seems broken, why are you interfering with the supplied constraints?
On Tue, 2012-09-18 at 16:02 +0100, Mark Brown wrote: > On Tue, Sep 18, 2012 at 03:17:46PM +0100, Pawel Moll wrote: > > Implementation of the regulator framework driver for the > > Versatile Express voltage control. Devices without > > voltage constraints (ie. "regulator-[min|max]-microvolt" > > properties in the DT node) are treated as fixed (or rather > > read-only) regulators. > > This doesn't seem great... it doesn't seem to know or represent > anything at all about the hardware, I'd expect a voltage regulator to at > a minimum be able to implement list_voltage(). You've not provided any > information on what the hardware actually is and the driver just seems > to proxy through to some other API which actually implements the > regulator support. Well, that's what it really is. The config API sends a request "set xyz uV" to the microcontrollers. And the micro can (at least in theory) get you any voltage within the min/max limits by whatever means it has (at least some of the daugtherboards use micro's DAC to adjust reference voltage for a DC/DC converter with a feedback loop using ADC). But fair enough, I should have done better work in describing this. > > + init_data->constraints.apply_uV = 0; > > This seems broken, why are you interfering with the supplied > constraints? Hm. It's been about a month since I wrote that, so the best I can tell now is "because fixed.c does the same" (and that's what I was looking at)... Anyway, looked at the code again and tried everything without that line and indeed I see no reason to do that, so consider it gone. v3 to follow. Cheers! Pawel
On Tue, Sep 18, 2012 at 04:44:16PM +0100, Pawel Moll wrote: > Well, that's what it really is. The config API sends a request "set xyz > uV" to the microcontrollers. And the micro can (at least in theory) get > you any voltage within the min/max limits by whatever means it has (at > least some of the daugtherboards use micro's DAC to adjust reference > voltage for a DC/DC converter with a feedback loop using ADC). And the microcontroller is incapable of telling us what it supports, or even what's physically present? > But fair enough, I should have done better work in describing this. So this is going to break interoperation with a bunch of consumer drivers that rely on being able to tell what voltages are supported. The key thing for them would be that regulator_is_supported_voltage() works, currently it relies on list_voltage() as that's the only way to do that right now. > v3 to follow. v3? I didn't see V2. Please also CC posts to the relevant mailing lists for the subsystem (lkml in this case).
On Tue, 2012-09-18 at 17:09 +0100, Mark Brown wrote: > On Tue, Sep 18, 2012 at 04:44:16PM +0100, Pawel Moll wrote: > > > Well, that's what it really is. The config API sends a request "set xyz > > uV" to the microcontrollers. And the micro can (at least in theory) get > > you any voltage within the min/max limits by whatever means it has (at > > least some of the daugtherboards use micro's DAC to adjust reference > > voltage for a DC/DC converter with a feedback loop using ADC). > > And the microcontroller is incapable of telling us what it supports, No, there is no such . Device Tree defines per-device min/max constraints and any value between these values should be supported. > or even what's physically present? You mean read back what voltage is set now? By all means - that's what it is doing for "read only" devices. > > But fair enough, I should have done better work in describing this. > > So this is going to break interoperation with a bunch of consumer > drivers that rely on being able to tell what voltages are supported. > The key thing for them would be that regulator_is_supported_voltage() > works, currently it relies on list_voltage() as that's the only way to > do that right now. Ok, I guess I should use regulator_list_voltage_linear() and regulator_map_voltage_linear() then? I'll just have to carefully think what step to choose. > > v3 to follow. > > v3? I didn't see V2. Please also CC posts to the relevant mailing > lists for the subsystem (lkml in this case). Sure, will do. Pawel
On Tue, Sep 18, 2012 at 06:03:39PM +0100, Pawel Moll wrote: > On Tue, 2012-09-18 at 17:09 +0100, Mark Brown wrote: > > or even what's physically present? > You mean read back what voltage is set now? By all means - that's what > it is doing for "read only" devices. No, I mean discovering which regulators are present and what they can do. > > So this is going to break interoperation with a bunch of consumer > > drivers that rely on being able to tell what voltages are supported. > > The key thing for them would be that regulator_is_supported_voltage() > > works, currently it relies on list_voltage() as that's the only way to > > do that right now. > Ok, I guess I should use regulator_list_voltage_linear() and > regulator_map_voltage_linear() then? I'll just have to carefully think > what step to choose. No, we should provide a way to describe this situation in the API - it's not unreasonable and having to pick step sizes is obviously suboptimal.
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 4e932cc..a823951 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -404,6 +404,13 @@ config REGULATOR_TWL4030 This driver supports the voltage regulators provided by this family of companion chips. +config REGULATOR_VEXPRESS + tristate "Versatile Express regulators" + depends on VEXPRESS_CONFIG + help + This driver provides support for voltage regulators available + on the ARM Ltd's Versatile Express platform. + config REGULATOR_WM831X tristate "Wolfson Microelectronics WM831x PMIC regulators" depends on MFD_WM831X diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 3342615..0d4e10f 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -55,6 +55,7 @@ obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o +obj-$(CONFIG_REGULATOR_VEXPRESS) += vexpress.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-dcdc.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-isink.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o diff --git a/drivers/regulator/vexpress.c b/drivers/regulator/vexpress.c new file mode 100644 index 0000000..a2682bb --- /dev/null +++ b/drivers/regulator/vexpress.c @@ -0,0 +1,155 @@ +/* + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Copyright (C) 2012 ARM Limited + */ + +#define DRVNAME "vexpress-regulator" +#define pr_fmt(fmt) DRVNAME ": " fmt + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> +#include <linux/regulator/of_regulator.h> +#include <linux/vexpress.h> + +struct vexpress_regulator { + struct regulator_desc desc; + struct regulator_dev *regdev; + struct vexpress_config_func *func; +}; + +static int vexpress_regulator_get_voltage(struct regulator_dev *regdev) +{ + struct vexpress_regulator *reg = rdev_get_drvdata(regdev); + u32 uV; + int err = vexpress_config_read(reg->func, 0, &uV); + + return err ? err : uV; +} + +static int vexpress_regulator_set_voltage(struct regulator_dev *regdev, + int min_uV, int max_uV, unsigned *selector) +{ + struct vexpress_regulator *reg = rdev_get_drvdata(regdev); + + return vexpress_config_write(reg->func, 0, min_uV); +} + +static struct regulator_ops vexpress_regulator_ops_ro = { + .get_voltage = vexpress_regulator_get_voltage, +}; + +static struct regulator_ops vexpress_regulator_ops = { + .get_voltage = vexpress_regulator_get_voltage, + .set_voltage = vexpress_regulator_set_voltage, +}; + +static int vexpress_regulator_probe(struct platform_device *pdev) +{ + int err; + struct vexpress_regulator *reg; + struct regulator_init_data *init_data; + struct regulator_config config = { }; + + reg = devm_kzalloc(&pdev->dev, sizeof(*reg), GFP_KERNEL); + if (!reg) { + err = -ENOMEM; + goto error_kzalloc; + } + + reg->func = vexpress_config_func_get_by_dev(&pdev->dev); + if (!reg->func) { + err = -ENXIO; + goto error_get_func; + } + + reg->desc.name = dev_name(&pdev->dev); + reg->desc.type = REGULATOR_VOLTAGE; + reg->desc.owner = THIS_MODULE; + + init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node); + if (!init_data) { + err = -EINVAL; + goto error_get_regulator_init_data; + } + + init_data->constraints.apply_uV = 0; + if (init_data->constraints.min_uV && init_data->constraints.max_uV) + reg->desc.ops = &vexpress_regulator_ops; + else + reg->desc.ops = &vexpress_regulator_ops_ro; + + config.dev = &pdev->dev; + config.init_data = init_data; + config.driver_data = reg; + config.of_node = pdev->dev.of_node; + + reg->regdev = regulator_register(®->desc, &config); + if (IS_ERR(reg->regdev)) { + err = PTR_ERR(reg->regdev); + goto error_regulator_register; + } + + platform_set_drvdata(pdev, reg); + + return 0; + +error_regulator_register: +error_get_regulator_init_data: + vexpress_config_func_put(reg->func); +error_get_func: +error_kzalloc: + return err; +} + +static int __devexit vexpress_regulator_remove(struct platform_device *pdev) +{ + struct vexpress_regulator *reg = platform_get_drvdata(pdev); + + vexpress_config_func_put(reg->func); + regulator_unregister(reg->regdev); + + return 0; +} + +static struct of_device_id vexpress_regulator_of_match[] = { + { .compatible = "arm,vexpress-volt", }, +}; + +static struct platform_driver vexpress_regulator_driver = { + .probe = vexpress_regulator_probe, + .remove = __devexit_p(vexpress_regulator_remove), + .driver = { + .name = DRVNAME, + .owner = THIS_MODULE, + .of_match_table = vexpress_regulator_of_match, + }, +}; + +static int __init vexpress_regulator_init(void) +{ + return platform_driver_register(&vexpress_regulator_driver); +} + +static void __exit vexpress_regulator_exit(void) +{ + platform_driver_unregister(&vexpress_regulator_driver); +} + +MODULE_AUTHOR("Pawel Moll <pawel.moll@arm.com>"); +MODULE_DESCRIPTION("Versatile Express regulator"); +MODULE_LICENSE("GPL"); + +module_init(vexpress_regulator_init); +module_exit(vexpress_regulator_exit);
Implementation of the regulator framework driver for the Versatile Express voltage control. Devices without voltage constraints (ie. "regulator-[min|max]-microvolt" properties in the DT node) are treated as fixed (or rather read-only) regulators. Signed-off-by: Pawel Moll <pawel.moll@arm.com> --- drivers/regulator/Kconfig | 7 ++ drivers/regulator/Makefile | 1 + drivers/regulator/vexpress.c | 155 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 drivers/regulator/vexpress.c Hi Liam, Mark, Do you think it would be possible to have this merged in time for 3.7? Cheers! Pawel