| Message ID | 1343390750-3642-2-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote: > Some device drivers (panel backlights especially) need to follow precise > sequences for powering on and off, involving gpios, regulators, PWMs > with a precise powering order and delays to respect between each steps. > These sequences are board-specific, and do not belong to a particular > driver - therefore they have been performed by board-specific hook > functions to far. > > With the advent of the device tree and of ARM kernels that are not > board-tied, we cannot rely on these board-specific hooks anymore but > need a way to implement these sequences in a portable manner. This patch > introduces a simple interpreter that can execute such power sequences > encoded either as platform data or within the device tree. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/power/power_seq.txt | 120 +++++++++++++++ > drivers/base/Kconfig | 4 + > drivers/base/Makefile | 1 + > drivers/base/power_seq.c | 300 ++++++++++++++++++++++++++++++++++++++ > include/linux/power_seq.h | 139 ++++++++++++++++++ What's wrong with drivers/power/? I sure don't want to maintain this code, and it seems to not be part of the "driver core" infrastructure. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote: > +++ b/include/linux/power_seq.h > @@ -0,0 +1,139 @@ > +/* > + * power_seq.h > + * > + * Simple interpreter for defining power sequences as platform data or device > + * tree properties. Initially designed for use with backlight drivers. > + * > + * Power sequences are designed to replace the callbacks typically used in > + * board-specific files that implement board-specific power sequences of devices > + * such as backlights. A power sequence is an array of resources (which can a > + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or > + * disable) and optional pre and post step delays. By having them interpreted > + * instead of arbitrarily executed, it is possible to describe these in the > + * device tree and thus remove board-specific code from the kernel. > + * > + * Author: Alexandre Courbot <acourbot@nvidia.com> > + * > + * Copyright (c) 2012 NVIDIA Corporation. > + * > + * 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; version 2 of the License. > + * > + * 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. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. As I always say: Unless you want to track the office movements of the FSF for the next 40 years, and keep this file up to date, drop that last paragraph, it's pointless. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/28/2012 03:19 AM, Greg Kroah-Hartman wrote: > On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote: >> Some device drivers (panel backlights especially) need to follow precise >> sequences for powering on and off, involving gpios, regulators, PWMs >> with a precise powering order and delays to respect between each steps. >> These sequences are board-specific, and do not belong to a particular >> driver - therefore they have been performed by board-specific hook >> functions to far. >> >> With the advent of the device tree and of ARM kernels that are not >> board-tied, we cannot rely on these board-specific hooks anymore but >> need a way to implement these sequences in a portable manner. This patch >> introduces a simple interpreter that can execute such power sequences >> encoded either as platform data or within the device tree. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >> --- >> Documentation/power/power_seq.txt | 120 +++++++++++++++ >> drivers/base/Kconfig | 4 + >> drivers/base/Makefile | 1 + >> drivers/base/power_seq.c | 300 ++++++++++++++++++++++++++++++++++++++ >> include/linux/power_seq.h | 139 ++++++++++++++++++ > > What's wrong with drivers/power/? I sure don't want to maintain this > code, and it seems to not be part of the "driver core" infrastructure. I thought about drivers/power/ initially, but quickly realized it was only about the power supply class and its drivers - so I felt like it would be out of place there, as the power sequences have nothing to do with power supply but instead control gpios, regulators and pwms. On the other hand I have just noticed that the apparently unrelated Adaptive Voltage Scaling driver just appeared in drivers/power/avs. So if Anton and David are ok with this, maybe I could put the power sequences code in its own subdirectory within drivers/power. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 30, 2012 at 10:51:42AM +0900, Alex Courbot wrote: [...] > On the other hand I have just noticed that the apparently unrelated > Adaptive Voltage Scaling driver just appeared in drivers/power/avs. > So if Anton and David are ok with this, maybe I could put the power > sequences code in its own subdirectory within drivers/power. Well, currently drivers/power/ is indeed just for power supply class subsystem and drivers. But if the trend is to gather power management ("policy") stuff under one directory, i.e. drivers/ power/ supplies/ <- former "power supply class and drivers" regulators/ idle/ cpuidle/ cpufreq/ devfreq/ avs/ ... That would probably make sense, we could easily see the big picture. But if we're not going to do this long-term, I would suggest to stick to just a new directory under drivers (and move drivers/power/avs/ to drivers/avs). Cc'ing some more people... Thanks, p.s. Jean, why am I the last person who discovers drivers/power/avs/? Would be nice to Cc me on such patches; by moving AVS under drivers/power/ you effectively nominated me as its maintainer. :-)
Hi, On Fri, Jul 27, 2012 at 1:05 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > Some device drivers (panel backlights especially) need to follow precise > sequences for powering on and off, involving gpios, regulators, PWMs > with a precise powering order and delays to respect between each steps. > These sequences are board-specific, and do not belong to a particular > driver - therefore they have been performed by board-specific hook > functions to far. > > With the advent of the device tree and of ARM kernels that are not > board-tied, we cannot rely on these board-specific hooks anymore but > need a way to implement these sequences in a portable manner. This patch > introduces a simple interpreter that can execute such power sequences > encoded either as platform data or within the device tree. This all looks very reasonable to me, just a few comments. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/power/power_seq.txt | 120 +++++++++++++++ > drivers/base/Kconfig | 4 + > drivers/base/Makefile | 1 + > drivers/base/power_seq.c | 300 ++++++++++++++++++++++++++++++++++++++ > include/linux/power_seq.h | 139 ++++++++++++++++++ > 5 files changed, 564 insertions(+) > create mode 100644 Documentation/power/power_seq.txt > create mode 100644 drivers/base/power_seq.c > create mode 100644 include/linux/power_seq.h > > diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > new file mode 100644 > index 0000000..aa2ceb5 > --- /dev/null > +++ b/Documentation/power/power_seq.txt > @@ -0,0 +1,120 @@ > +Runtime Interpreted Power Sequences > +----------------------------------- > + > +Problem > +------- > +One very common board-dependent code is the out-of-driver code that is used to > +turn a device on or off. For instance, SoC boards very commonly use a GPIO > +(abstracted to a regulator or not) to control the power supply of a backlight, > +disabling it when the backlight is not used in order to save power. The GPIO > +that should be used, however, as well as the exact power sequence that may > +involve different resources, is board-dependent and thus unknown of the driver. > + > +This has been addressed so far by using hooks in the device's platform data that > +are called whenever the state of the device might reflect a power change. This > +approach, however, introduces board-dependant code into the kernel and is not > +compatible with the device tree. > + > +The Runtime Interpreted Power Sequences (or power sequences for short) aim at > +turning this code into platform data or device tree nodes. Power sequences are > +described using a simple format and run by a simple interpreter whenever needed. > +This allows to remove the callback mechanism and makes the kernel less > +board-dependant. > + > +Sequences Format > +---------------- > +Power sequences are a series of sequential steps during which an action is > +performed on a resource. The supported resources so far are: > +- GPIOs > +- Regulators > +- PWMs > + > +Each step designates a resource and the following parameters: > +- Whether the step should enable or disable the resource, > +- Delay to wait before performing the action, > +- Delay to wait after performing the action. > + > +Both new resources and parameters can be introduced, but the goal is of course > +to keep things as simple and compact as possible. > + > +The platform data is a simple array of platform_power_seq_step instances, each > +instance describing a step. The type as well as one of id or gpio members > +(depending on the type) must be specified. The last step must be of type > +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are > +identified by number. For example, the following sequence will turn on the > +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1: For the delay, I think milliseconds is reasonable. I suppose there is no reasonable need for microseconds? > + > +struct platform_power_seq_step power_on_seq[] = { > + { > + .type = POWER_SEQ_REGULATOR, > + .id = "power", > + .params = { > + .enable = 1, > + .post_delay = 10, > + }, > + }, > + { > + .type = POWER_SEQ_GPIO, > + .gpio = 110, > + .params = { > + .enable = 1, > + }, > + }, > + { > + .type = POWER_SEQ_STOP, > + }, > +}; > + > +Usage by Drivers and Resources Management > +----------------------------------------- > +Power sequences make use of resources that must be properly allocated and > +managed. The power_seq_build() function takes care of resolving the resources as > +they are met in the sequence and to allocate them if needed: > + > +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, > + platform_power_seq *pseq); > + > +You will need an instance of power_seq_resources to keep track of the resources > +that are already allocated. On success, the function returns a devm allocated > +resolved sequence that is ready to be passed to power_seq_run(). In case of > +failure, and error code is returned. > + > +A resolved power sequence returned by power_seq_build can be run by > +power_run_run(): > + > +int power_seq_run(struct device *dev, power_seq *seq); > + > +It returns 0 if the sequence has successfully been run, or an error code if a > +problem occured. > + > +Finally, some resources that cannot be allocated through devm need to be freed > +manually. Therefore, be sure to call power_seq_free_resources() in your device > +remove function: > + > +void power_seq_free_resources(power_seq_resources *ress); > + > +Device tree > +----------- > +All the same, power sequences can be encoded as device tree nodes. The following > +properties and nodes are equivalent to the platform data defined previously: > + > + power-supply = <&mydevice_reg>; > + enable-gpio = <&gpio 6 0>; > + > + power-on-sequence { > + regulator@0 { > + id = "power"; Is there a reason not to put the phandle here, like: id = <&mydevice_reg>; (or maybe 'device' instead of id?) > + enable; > + post-delay = <10>; > + }; > + gpio@1 { > + id = "enable-gpio"; > + enable; > + }; > + }; > + > +Note that first, the phandles of the regulator and gpio used in the sequences > +are defined as properties. Then the sequence references them through the id > +property of every step. The name of sub-properties defines the type of the step. > +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered > +sequentially. For the regulator and gpio types I think you only have an enable. For the pwm, what is the intended binding? Is that documented elsewhere? Also it might be worth mentioning how you get a struct power_seq from an fdt node, and perhaps given an example of a device which has an attached node, so we can see how it is referenced from the device (of_parse_power_seq I think). Do put the sequence inside the device node or reference it with a phandle? Finally, should you use typedefs? Regards, Simon -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2012 at 09:05:48PM +0900, Alexandre Courbot wrote: > Some device drivers (panel backlights especially) need to follow precise > sequences for powering on and off, involving gpios, regulators, PWMs > with a precise powering order and delays to respect between each steps. > These sequences are board-specific, and do not belong to a particular > driver - therefore they have been performed by board-specific hook > functions to far. > > With the advent of the device tree and of ARM kernels that are not > board-tied, we cannot rely on these board-specific hooks anymore but > need a way to implement these sequences in a portable manner. This patch > introduces a simple interpreter that can execute such power sequences > encoded either as platform data or within the device tree. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/power/power_seq.txt | 120 +++++++++++++++ > drivers/base/Kconfig | 4 + > drivers/base/Makefile | 1 + > drivers/base/power_seq.c | 300 ++++++++++++++++++++++++++++++++++++++ > include/linux/power_seq.h | 139 ++++++++++++++++++ > 5 files changed, 564 insertions(+) > create mode 100644 Documentation/power/power_seq.txt > create mode 100644 drivers/base/power_seq.c > create mode 100644 include/linux/power_seq.h > > diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > new file mode 100644 > index 0000000..aa2ceb5 > --- /dev/null > +++ b/Documentation/power/power_seq.txt > @@ -0,0 +1,120 @@ > +Runtime Interpreted Power Sequences > +----------------------------------- > + > +Problem > +------- > +One very common board-dependent code is the out-of-driver code that is used to > +turn a device on or off. For instance, SoC boards very commonly use a GPIO > +(abstracted to a regulator or not) to control the power supply of a backlight, > +disabling it when the backlight is not used in order to save power. The GPIO > +that should be used, however, as well as the exact power sequence that may > +involve different resources, is board-dependent and thus unknown of the driver. > + > +This has been addressed so far by using hooks in the device's platform data that > +are called whenever the state of the device might reflect a power change. This > +approach, however, introduces board-dependant code into the kernel and is not > +compatible with the device tree. > + > +The Runtime Interpreted Power Sequences (or power sequences for short) aim at > +turning this code into platform data or device tree nodes. Power sequences are > +described using a simple format and run by a simple interpreter whenever needed. > +This allows to remove the callback mechanism and makes the kernel less > +board-dependant. > + > +Sequences Format > +---------------- > +Power sequences are a series of sequential steps during which an action is > +performed on a resource. The supported resources so far are: > +- GPIOs > +- Regulators > +- PWMs > + > +Each step designates a resource and the following parameters: > +- Whether the step should enable or disable the resource, > +- Delay to wait before performing the action, > +- Delay to wait after performing the action. > + > +Both new resources and parameters can be introduced, but the goal is of course > +to keep things as simple and compact as possible. > + > +The platform data is a simple array of platform_power_seq_step instances, each > +instance describing a step. The type as well as one of id or gpio members > +(depending on the type) must be specified. The last step must be of type > +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are > +identified by number. For example, the following sequence will turn on the > +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1: > + > +struct platform_power_seq_step power_on_seq[] = { > + { > + .type = POWER_SEQ_REGULATOR, > + .id = "power", > + .params = { > + .enable = 1, > + .post_delay = 10, > + }, > + }, > + { > + .type = POWER_SEQ_GPIO, > + .gpio = 110, > + .params = { > + .enable = 1, > + }, > + }, > + { > + .type = POWER_SEQ_STOP, > + }, > +}; > + > +Usage by Drivers and Resources Management > +----------------------------------------- > +Power sequences make use of resources that must be properly allocated and > +managed. The power_seq_build() function takes care of resolving the resources as > +they are met in the sequence and to allocate them if needed: > + > +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, > + platform_power_seq *pseq); > + > +You will need an instance of power_seq_resources to keep track of the resources > +that are already allocated. On success, the function returns a devm allocated > +resolved sequence that is ready to be passed to power_seq_run(). In case of > +failure, and error code is returned. I don't quite understand why the struct power_seq_resources is needed. Can this not be stored within power_seq? > + > +A resolved power sequence returned by power_seq_build can be run by > +power_run_run(): > + > +int power_seq_run(struct device *dev, power_seq *seq); Why is the struct device required here? It already is passed during the call to pwm_seq_build(), so perhaps you should keep a reference to it within struct power_seq? > + > +It returns 0 if the sequence has successfully been run, or an error code if a > +problem occured. > + > +Finally, some resources that cannot be allocated through devm need to be freed > +manually. Therefore, be sure to call power_seq_free_resources() in your device > +remove function: > + > +void power_seq_free_resources(power_seq_resources *ress); Could this not also be handled by a managed version? If a power_seq is always managed, then I would assume that it also takes care of freeing the resources, even if the resources have no managed equivalents. Perhaps it would also make sense to provide non-managed version of these functions. I think that would make the managed versions easier and more canonical to implement. > + > +Device tree > +----------- > +All the same, power sequences can be encoded as device tree nodes. The following > +properties and nodes are equivalent to the platform data defined previously: > + > + power-supply = <&mydevice_reg>; > + enable-gpio = <&gpio 6 0>; > + > + power-on-sequence { > + regulator@0 { > + id = "power"; > + enable; > + post-delay = <10>; > + }; > + gpio@1 { > + id = "enable-gpio"; > + enable; > + }; > + }; > + > +Note that first, the phandles of the regulator and gpio used in the sequences > +are defined as properties. Then the sequence references them through the id > +property of every step. The name of sub-properties defines the type of the step. > +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered > +sequentially. I think there has been quite some discussion regarding the naming of subnodes and the conclusion seems to have been to name them uniformly after what they represent. As such the power-on-sequence subnodes should be called step@0, step@1, etc. However, that will require the addition of a property to define the type of resource. Also, is there some way we can make the id property for GPIOs not require the -gpio suffix? If the resource type is already GPIO, then it seems redundant to add -gpio to the ID. > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > index 08b4c52..65bebfe 100644 > --- a/drivers/base/Kconfig > +++ b/drivers/base/Kconfig > @@ -282,4 +282,8 @@ config CMA_AREAS > > endif > > +config POWER_SEQ > + bool > + default n > + "default n" is already the default, so you can drop that line. > endmenu > diff --git a/drivers/base/Makefile b/drivers/base/Makefile > index 5aa2d70..4c498c1 100644 > --- a/drivers/base/Makefile > +++ b/drivers/base/Makefile > @@ -18,6 +18,7 @@ obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o > ifeq ($(CONFIG_SYSFS),y) > obj-$(CONFIG_MODULES) += module.o > endif > +obj-$(CONFIG_POWER_SEQ) += power_seq.o > obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o > obj-$(CONFIG_REGMAP) += regmap/ > obj-$(CONFIG_SOC_BUS) += soc.o > diff --git a/drivers/base/power_seq.c b/drivers/base/power_seq.c > new file mode 100644 > index 0000000..6ccefa1 > --- /dev/null > +++ b/drivers/base/power_seq.c > @@ -0,0 +1,300 @@ > +/* > + * power_seq.c - A simple power sequence interpreter for platform devices > + * and device tree. > + * > + * Author: Alexandre Courbot <acourbot@nvidia.com> > + * > + * Copyright (c) 2012 NVIDIA Corporation. > + * > + * 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; version 2 of the License. > + * > + * 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. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. > + * > + */ > + > +#include <linux/power_seq.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/device.h> > +#include <linux/slab.h> > +#include <linux/delay.h> > +#include <linux/pwm.h> > +#include <linux/regulator/consumer.h> > +#include <linux/gpio.h> > + > +#ifdef CONFIG_OF > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#endif I think you don't need the CONFIG_OF guard around these. Both of.h and of_gpio.h can be included unconditionally and actually contain dummy definitions for the public functions in the !OF case. > + > +static int power_seq_step_run(struct power_seq_step *step) > +{ > + int err = 0; > + > + if (step->params.pre_delay) > + mdelay(step->params.pre_delay); > + > + switch (step->resource->type) { > +#ifdef CONFIG_REGULATOR > + case POWER_SEQ_REGULATOR: > + if (step->params.enable) > + err = regulator_enable(step->resource->regulator); > + else > + err = regulator_disable(step->resource->regulator); > + break; > +#endif > +#ifdef CONFIG_PWM > + case POWER_SEQ_PWM: > + if (step->params.enable) > + err = pwm_enable(step->resource->pwm); > + else > + pwm_disable(step->resource->pwm); > + break; > +#endif > +#ifdef CONFIG_GPIOLIB > + case POWER_SEQ_GPIO: > + gpio_set_value_cansleep(step->resource->gpio, > + step->params.enable); > + break; > +#endif This kind of #ifdef'ery is quite ugly. I don't know if adding separate *_run() functions for each type of resource would be any better, though. Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and GPIOLIB symbols to side-step the issue completely? > + /* > + * should never happen unless the sequence includes a step which > + * type does not have support compiled in > + */ > + default: > + return -EINVAL; > + } > + > + if (err < 0) > + return err; > + > + if (step->params.post_delay) > + mdelay(step->params.post_delay); > + > + return 0; > +} > + > +int power_seq_run(struct device *dev, power_seq *seq) > +{ > + int err; > + > + if (!seq) return 0; I don't think this is acceptable according to the coding style. Also, perhaps returning -EINVAL would be more meaningful? > + > + while (seq->resource) { Perhaps this should check for POWER_SEQ_STOP instead? > + if ((err = power_seq_step_run(seq++))) { > + dev_err(dev, "error %d while running power sequence!\n", > + err); For this kind of diagnostics it could be useful to have a name associated with the power sequence. But I'm not sure that making the power sequence code output an error here is the best solution. I find it to be annoying when core code starts outputting too many error codes. In this case it's particularily easy to catch the errors in the caller. > + return err; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(power_seq_run); > + > +#ifdef CONFIG_OF > +static int of_parse_power_seq_step(struct device *dev, struct device_node *node, > + struct platform_power_seq_step *step) > +{ > + if (of_property_read_string(node, "id", &step->id)) { > + dev_err(dev, "missing id property!\n"); > + return -EINVAL; > + } > + > + if (!strcmp(node->name, "regulator")) { > + step->type = POWER_SEQ_REGULATOR; > +#ifdef CONFIG_OF_GPIO > + } else if (!strcmp(node->name, "gpio")) { > + int gpio; > + > + step->type = POWER_SEQ_GPIO; > + gpio = of_get_named_gpio(dev->of_node, step->id, 0); > + if (gpio < 0) { > + dev_err(dev, "cannot resolve gpio \"%s\"\n", step->id); > + return gpio; > + } > + step->gpio = gpio; > +#endif /* CONFIG_OF_GPIO */ > + } else if (!strcmp(node->name, "pwm")) { > + step->type = POWER_SEQ_PWM; > + } else { > + dev_err(dev, "invalid power seq step type!\n"); > + return -EINVAL; > + } > + > + if (of_find_property(node, "enable", NULL)) { > + step->params.enable = 1; > + } else if (!of_find_property(node, "disable", NULL)) { > + dev_err(dev, "missing enable or disable property!\n"); > + return -EINVAL; > + } > + > + of_property_read_u32(node, "pre-delay", &step->params.pre_delay); > + of_property_read_u32(node, "post-delay", &step->params.post_delay); > + > + return 0; > +} > + > +platform_power_seq *of_parse_power_seq(struct device *dev, > + struct device_node *node) > +{ > + struct device_node *child = NULL; > + platform_power_seq *ret; > + int cpt = 0; > + int err; > + > + if (!node) return NULL; Again, this should probably be split across two lines. > + > + while ((child = of_get_next_child(node, child))) > + cpt++; for_each_child_of_node()? > + > + /* allocate one more step to signal end of sequence */ > + ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL); > + if (!ret) > + return ERR_PTR(-ENOMEM); > + > + cpt = 0; > + while ((child = of_get_next_child(node, child))) { Here as well. > + if ((err = of_parse_power_seq_step(dev, child, &ret[cpt++]))) > + return ERR_PTR(err); > + } > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(of_parse_power_seq); > +#endif /* CONFIG_OF */ > + > +static > +struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress, > + struct platform_power_seq_step *res) > +{ > + struct power_seq_resource *step; > + > + list_for_each_entry(step, ress, list) { > + if (step->type != res->type) continue; > + switch (res->type) { > + case POWER_SEQ_GPIO: > + if (step->gpio == res->gpio) > + return step; > + break; > + default: > + if (!strcmp(step->id, res->id)) > + return step; > + break; > + } > + } > + > + return NULL; > +} > + > +static int power_seq_allocate_resource(struct device *dev, > + struct power_seq_resource *res) > +{ > + int err; > + > + switch (res->type) { > +#ifdef CONFIG_REGULATOR > + case POWER_SEQ_REGULATOR: > + res->regulator = devm_regulator_get(dev, res->id); > + if (IS_ERR(res->regulator)) { > + dev_err(dev, "cannot get regulator \"%s\"\n", res->id); > + return PTR_ERR(res->regulator); > + } > + break; > +#endif > +#ifdef CONFIG_PWM > + case POWER_SEQ_PWM: > + res->pwm = pwm_get(dev, res->id); > + if (IS_ERR(res->pwm)) { > + dev_err(dev, "cannot get pwm \"%s\"\n", res->id); > + return PTR_ERR(res->pwm); > + } > + break; > +#endif > +#ifdef CONFIG_GPIOLIB > + case POWER_SEQ_GPIO: > + err = devm_gpio_request_one(dev, res->gpio, GPIOF_OUT_INIT_HIGH, > + "backlight_gpio"); > + if (err) { > + dev_err(dev, "cannot get gpio %d\n", res->gpio); > + return err; > + } > + break; > +#endif > + default: > + dev_err(dev, "invalid resource type %d\n", res->type); > + return -EINVAL; > + break; > + } > + > + return 0; > +} > + > +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, > + platform_power_seq *pseq) > +{ > + struct power_seq_step *seq = NULL, *ret; > + struct power_seq_resource *res; > + int cpt, err; > + > + /* first pass to count the number of steps to allocate */ > + for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++); Wouldn't it be easier to pass around the number of steps in the sequence instead of having to count in various places? This would be more along the lines of how struct platform_device defines associated resources. > + > + if (!cpt) > + return seq; Perhaps this should return an error-code as well? I find it nice to not have to handle NULL specially when using ERR_PTR et al. > + > + /* 1 more for the STOP step */ > + ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL); > + if (!seq) > + return ERR_PTR(-ENOMEM); > + > + for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) { > + /* create resource node if not referenced already */ > + if (!(res = power_seq_find_resource(ress, pseq))) { > + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL); > + if (!res) > + return ERR_PTR(-ENOMEM); > + res->type = pseq->type; > + > + if (res->type == POWER_SEQ_GPIO) > + res->gpio = pseq->gpio; > + else > + res->id = pseq->id; > + > + if ((err = power_seq_allocate_resource(dev, res)) < 0) > + return ERR_PTR(err); > + > + list_add(&res->list, ress); > + } > + seq->resource = res; > + memcpy(&seq->params, &pseq->params, sizeof(seq->params)); > + } > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(power_seq_build); > + > +void power_seq_free_resources(power_seq_resources *ress) { The brace needs to go on a line by itself. > + struct power_seq_resource *res; > + > +#ifdef CONFIG_PWM > + list_for_each_entry(res, ress, list) { > + if (res->type == POWER_SEQ_PWM) > + pwm_put(res->pwm); > + } > +#endif > +} > +EXPORT_SYMBOL_GPL(power_seq_free_resources); > + > +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>"); > +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h > new file mode 100644 > index 0000000..da0593a > --- /dev/null > +++ b/include/linux/power_seq.h > @@ -0,0 +1,139 @@ > +/* > + * power_seq.h > + * > + * Simple interpreter for defining power sequences as platform data or device > + * tree properties. Initially designed for use with backlight drivers. > + * > + * Power sequences are designed to replace the callbacks typically used in > + * board-specific files that implement board-specific power sequences of devices > + * such as backlights. A power sequence is an array of resources (which can a > + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or > + * disable) and optional pre and post step delays. By having them interpreted > + * instead of arbitrarily executed, it is possible to describe these in the > + * device tree and thus remove board-specific code from the kernel. > + * > + * Author: Alexandre Courbot <acourbot@nvidia.com> > + * > + * Copyright (c) 2012 NVIDIA Corporation. > + * > + * 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; version 2 of the License. > + * > + * 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. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. > + * > + */ > + > +#ifndef __LINUX_POWER_SEQ_H > +#define __LINUX_POWER_SEQ_H > + > +#include <linux/types.h> > + > +struct device; > +struct regulator; > +struct pwm_device; > +struct device_node; > + > +/** > + * The different kinds of resources that can be controlled during the sequences. > + */ > +typedef enum { > + POWER_SEQ_STOP = 0, > + POWER_SEQ_REGULATOR, > + POWER_SEQ_PWM, > + POWER_SEQ_GPIO, > + POWER_SEQ_MAX, > +} power_res_type; Maybe the prefix power_seq should be used here as well, so: power_seq_res_type. > + > +struct power_seq_resource { > + power_res_type type; > + /* name to resolve for resources with a name (regulator, pwm) */ > + const char *id; > + /* resolved resource */ > + union { > + struct regulator *regulator; > + struct pwm_device *pwm; > + int gpio; > + }; > + /* used to maintain the list of resources used by the driver */ > + struct list_head list; > +}; > +typedef struct list_head power_seq_resources; No type definitions like this, please. Also, why define this particular type globally? > + > +struct power_step_params { > + /* enable the resource if 1, disable if 0 */ > + bool enable; > + /* delay (in ms) to wait before executing the step */ > + int pre_delay; > + /* delay (in ms) to wait after executing the step */ > + int post_delay; unsigned int for the delays? > +}; > + > +/** > + * Platform definition of power sequences. A sequence is an array of these, > + * terminated by a STOP instance. > + */ > +struct platform_power_seq_step { > + power_res_type type; > + union { > + /* Used by REGULATOR and PWM types to name the resource */ > + const char *id; > + /* Used by GPIO */ > + int gpio; > + }; > + struct power_step_params params; > +}; > +typedef struct platform_power_seq_step platform_power_seq; Why are the parameters kept in a separate structure? What are the disadvantages of keeping the in the sequence step structure directly? > + > +/** > + * Power sequence steps resolved against their resource. Built by > + * power_seq_build and used to run the sequence. > + */ > +struct power_seq_step { > + struct power_seq_resource *resource; > + struct power_step_params params; > +}; > +typedef struct power_seq_step power_seq; Would it make sense to make the struct power_seq opaque? I don't see why anyone but the power_seq code should access the internals. For resource managing it might also be easier to separate struct power_seq_step and struct power_seq, making the power_seq basically something like: struct power_seq { struct power_seq_step *steps; unsigned int num_steps; }; Perhaps a name field can be included for diagnostic purposes. > + > +#ifdef CONFIG_OF > +/** > + * Build a platform data sequence from a device tree node. Memory for the > + * sequence is allocated using devm_kzalloc on dev. > + */ > +platform_power_seq *of_parse_power_seq(struct device *dev, > + struct device_node *node); > +#else > +platform_power_seq *of_parse_power_seq(struct device *dev, > + struct device_node *node) > +{ > + return NULL; > +} > +#endif > + > +/** > + * Build a runnable power sequence from platform data, and add the resources > + * it uses into ress. Memory for the sequence is allocated using devm_kzalloc > + * on dev. > + */ > +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, > + platform_power_seq *pseq); I already mentioned this above: I fail to see why the ress parameter is needed here. It is an internal implementation detail of the power sequence code. Maybe a better place would be to include it within the struct power_seq. > +/** > + * Free all the resources previously allocated by power_seq_allocate_resources. > + */ > +void power_seq_free_resources(power_seq_resources *ress); > + > +/** > + * Run the given power sequence. Returns 0 on success, error code in case of > + * failure. > + */ > +int power_seq_run(struct device *dev, power_seq *seq); I think the API is too fine grained here. From a user's point of view, I'd expect a sequence like this: seq = power_seq_build(dev, sequence); ... power_seq_run(seq); ... power_seq_free(seq); Perhaps with managed variants where the power_seq_free() is executed automatically: seq = devm_power_seq_build(dev, sequence); ... power_seq_run(seq); Generally I really like where this is going. Thierry
On 07/27/2012 07:05 AM, Alexandre Courbot wrote: > Some device drivers (panel backlights especially) need to follow precise > sequences for powering on and off, involving gpios, regulators, PWMs > with a precise powering order and delays to respect between each steps. > These sequences are board-specific, and do not belong to a particular > driver - therefore they have been performed by board-specific hook > functions to far. > > With the advent of the device tree and of ARM kernels that are not > board-tied, we cannot rely on these board-specific hooks anymore but > need a way to implement these sequences in a portable manner. This patch > introduces a simple interpreter that can execute such power sequences > encoded either as platform data or within the device tree. > Why not? We'll always have some amount of board code. The key is to limit parts that are just data. I'm not sure this is something that should be in devicetree. Perhaps what is needed is a better way to hook into the driver like notifiers? > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/power/power_seq.txt | 120 +++++++++++++++ > drivers/base/Kconfig | 4 + > drivers/base/Makefile | 1 + > drivers/base/power_seq.c | 300 ++++++++++++++++++++++++++++++++++++++ > include/linux/power_seq.h | 139 ++++++++++++++++++ > 5 files changed, 564 insertions(+) > create mode 100644 Documentation/power/power_seq.txt > create mode 100644 drivers/base/power_seq.c > create mode 100644 include/linux/power_seq.h > > diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > new file mode 100644 > index 0000000..aa2ceb5 > --- /dev/null > +++ b/Documentation/power/power_seq.txt > @@ -0,0 +1,120 @@ > +Runtime Interpreted Power Sequences > +----------------------------------- > + > +Problem > +------- > +One very common board-dependent code is the out-of-driver code that is used to > +turn a device on or off. For instance, SoC boards very commonly use a GPIO > +(abstracted to a regulator or not) to control the power supply of a backlight, > +disabling it when the backlight is not used in order to save power. The GPIO > +that should be used, however, as well as the exact power sequence that may > +involve different resources, is board-dependent and thus unknown of the driver. > + > +This has been addressed so far by using hooks in the device's platform data that > +are called whenever the state of the device might reflect a power change. This > +approach, however, introduces board-dependant code into the kernel and is not > +compatible with the device tree. > + > +The Runtime Interpreted Power Sequences (or power sequences for short) aim at > +turning this code into platform data or device tree nodes. Power sequences are > +described using a simple format and run by a simple interpreter whenever needed. > +This allows to remove the callback mechanism and makes the kernel less > +board-dependant. > + > +Sequences Format > +---------------- > +Power sequences are a series of sequential steps during which an action is > +performed on a resource. The supported resources so far are: > +- GPIOs > +- Regulators > +- PWMs > + > +Each step designates a resource and the following parameters: > +- Whether the step should enable or disable the resource, > +- Delay to wait before performing the action, > +- Delay to wait after performing the action. > + > +Both new resources and parameters can be introduced, but the goal is of course > +to keep things as simple and compact as possible. > + > +The platform data is a simple array of platform_power_seq_step instances, each > +instance describing a step. The type as well as one of id or gpio members > +(depending on the type) must be specified. The last step must be of type > +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are > +identified by number. For example, the following sequence will turn on the > +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1: > + > +struct platform_power_seq_step power_on_seq[] = { > + { > + .type = POWER_SEQ_REGULATOR, > + .id = "power", > + .params = { > + .enable = 1, > + .post_delay = 10, > + }, > + }, > + { > + .type = POWER_SEQ_GPIO, > + .gpio = 110, > + .params = { > + .enable = 1, > + }, > + }, > + { > + .type = POWER_SEQ_STOP, > + }, > +}; > + > +Usage by Drivers and Resources Management > +----------------------------------------- > +Power sequences make use of resources that must be properly allocated and > +managed. The power_seq_build() function takes care of resolving the resources as > +they are met in the sequence and to allocate them if needed: > + > +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, > + platform_power_seq *pseq); > + > +You will need an instance of power_seq_resources to keep track of the resources > +that are already allocated. On success, the function returns a devm allocated > +resolved sequence that is ready to be passed to power_seq_run(). In case of > +failure, and error code is returned. > + > +A resolved power sequence returned by power_seq_build can be run by > +power_run_run(): > + > +int power_seq_run(struct device *dev, power_seq *seq); > + > +It returns 0 if the sequence has successfully been run, or an error code if a > +problem occured. > + > +Finally, some resources that cannot be allocated through devm need to be freed > +manually. Therefore, be sure to call power_seq_free_resources() in your device > +remove function: > + > +void power_seq_free_resources(power_seq_resources *ress); > + > +Device tree Bindings need to documented in Documentation/devicetree > +----------- > +All the same, power sequences can be encoded as device tree nodes. The following > +properties and nodes are equivalent to the platform data defined previously: > + > + power-supply = <&mydevice_reg>; > + enable-gpio = <&gpio 6 0>; > + > + power-on-sequence { > + regulator@0 { > + id = "power"; > + enable; What do this mean? Isn't this implied for a regulator? > + post-delay = <10>; > + }; > + gpio@1 { > + id = "enable-gpio"; > + enable; > + }; > + }; This looks like you designed the platform_data structs first and then came up with device nodes to mirror the struct. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 30, 2012 at 10:44:29AM -0500, Rob Herring wrote: > On 07/27/2012 07:05 AM, Alexandre Courbot wrote: > > + power-on-sequence { > > + regulator@0 { > > + id = "power"; > > + enable; > What do this mean? Isn't this implied for a regulator? I assume you might have some sequences which need some things to be turned off for some reason; it at least seems to be something you'd want to design for. > This looks like you designed the platform_data structs first and then > came up with device nodes to mirror the struct. Judging by most of the DT I review this seems idiomatic :) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, July 30, 2012, Anton Vorontsov wrote: > On Mon, Jul 30, 2012 at 10:51:42AM +0900, Alex Courbot wrote: > [...] > > On the other hand I have just noticed that the apparently unrelated > > Adaptive Voltage Scaling driver just appeared in drivers/power/avs. > > So if Anton and David are ok with this, maybe I could put the power > > sequences code in its own subdirectory within drivers/power. > > Well, currently drivers/power/ is indeed just for power supply class > subsystem and drivers. But if the trend is to gather power management > ("policy") stuff under one directory, i.e. > > drivers/ > power/ > supplies/ <- former "power supply class and drivers" > regulators/ > idle/ > cpuidle/ > cpufreq/ > devfreq/ > avs/ > ... > > That would probably make sense, we could easily see the big picture. > But if we're not going to do this long-term, Yes, we may do this eventually, but surely not right now. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/30/2012 09:44 AM, Rob Herring wrote: > On 07/27/2012 07:05 AM, Alexandre Courbot wrote: >> Some device drivers (panel backlights especially) need to follow precise >> sequences for powering on and off, involving gpios, regulators, PWMs >> with a precise powering order and delays to respect between each steps. >> These sequences are board-specific, and do not belong to a particular >> driver - therefore they have been performed by board-specific hook >> functions to far. >> >> With the advent of the device tree and of ARM kernels that are not >> board-tied, we cannot rely on these board-specific hooks anymore but >> need a way to implement these sequences in a portable manner. This patch >> introduces a simple interpreter that can execute such power sequences >> encoded either as platform data or within the device tree. >> > > Why not? We'll always have some amount of board code. The key is to > limit parts that are just data. I'm not sure this is something that > should be in devicetree. > > Perhaps what is needed is a better way to hook into the driver like > notifiers? I would answer that by asking the reverse question - why should we have to put some data in DT, and some data into board files still? I'd certainly argue that the sequence of which GPIOs/regulators/PWMs to manipulate is just data. To be honest, if we're going to have to put some parts of a board's configuration into board files anyway, then the entirety of DT seems useless; I'd far rather see all the configuration in one cohesive place than arbitrarily split into two/n different locations - that would make everything harder to maintain. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/27/2012 06:05 AM, Alexandre Courbot wrote: > Some device drivers (panel backlights especially) need to follow precise > sequences for powering on and off, involving gpios, regulators, PWMs > with a precise powering order and delays to respect between each steps. > These sequences are board-specific, and do not belong to a particular > driver - therefore they have been performed by board-specific hook > functions to far. > diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt > +Sequences Format > +---------------- > +Power sequences are a series of sequential steps during which an action is > +performed on a resource. The supported resources so far are: > +- GPIOs > +- Regulators > +- PWMs > + > +Each step designates a resource and the following parameters: > +- Whether the step should enable or disable the resource, At the highest level, i.e. what's being describe here, I wouldn't even talk about enable/disable, but rather the action to perform on the resource. The set of legal actions may be specific to the type of resource in question. Admittedly enable/disable are common though. > +- Delay to wait before performing the action, > +- Delay to wait after performing the action. I don't see a need to have a delay both before and after an action; except at the start of the sequence, step n's post-delay is at the same point in the sequence as step n+1's pre-delay. Perhaps make a "delay" step type? > +Both new resources and parameters can be introduced, but the goal is of course > +to keep things as simple and compact as possible. > +The platform data is a simple array of platform_power_seq_step instances, each Rather than jumping straight into platform data here, I'd expect an enumeration of legal resource types, and what actions can be performed on each, followed by a description of a sequence (very simply, just a list of actions and their parameters). This could be followed by a section describing the mapping of the abstract concepts to concrete platform data representation (and concrete device tree representation). > +instance describing a step. The type as well as one of id or gpio members > +(depending on the type) must be specified. The last step must be of type > +POWER_SEQ_STOP. I'd certainly suggest having a step count rather than a sentinel value in the list. > Regulator and PWM resources are identified by name. GPIO are > +identified by number. That's a little implementation-specific. I guess it's entirely true for a platform data representation, but not when mapping this into device tree. > +Usage by Drivers and Resources Management > +----------------------------------------- > +Power sequences make use of resources that must be properly allocated and > +managed. The power_seq_build() function takes care of resolving the resources as > +they are met in the sequence and to allocate them if needed: > + > +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, > + platform_power_seq *pseq); > + > +You will need an instance of power_seq_resources to keep track of the resources > +that are already allocated. On success, the function returns a devm allocated > +resolved sequence that is ready to be passed to power_seq_run(). In case of > +failure, and error code is returned. If the result is devm-allocated, the function probably should be named devm_power_seq_build(). I wonder if using the same structure/array as input and output would simplify the API; the platform data would fill in the fields mentioned above, and power_seq_build() would parse those, then set other fields in the same structs to the looked-up handle values? > +Finally, some resources that cannot be allocated through devm need to be freed > +manually. Therefore, be sure to call power_seq_free_resources() in your device > +remove function: > + > +void power_seq_free_resources(power_seq_resources *ress); You can make a custom devm free routine for the power_seq_resources itself, so the overall free'ing of the content can be triggered by devm, but the free'ing function can then call whatever non-devm APIs it wants for the non-devm-allocated members. > +Device tree > +----------- > +All the same, power sequences can be encoded as device tree nodes. The following > +properties and nodes are equivalent to the platform data defined previously: > + > + power-supply = <&mydevice_reg>; > + enable-gpio = <&gpio 6 0>; > + > + power-on-sequence { > + regulator@0 { As Thierry mentioned, the step nodes should be named for the type of object they are (a "step") not the type or name of resource they act upon ("regulator" or "gpio"). If the nodes have a unit address (i.e. end in "@n"), which they will have to if all named "step" and there's more than one of them, then they will need a matching reg property. Equally, the parent node will need #address-cells and #size-cells too. So, the last couple lines would be: power-on-sequence { #address-cells = <1>; #size-cells = <0>; step@0 { reg = <0>; > + id = "power"; "id" is usually a name or identifier. I think you mean "type" or perhaps "action" here: type = "regulator"; action = "enable"; or: action = "enable-regulator"; I wish we had integer constants in DT so we didn't have to do all this with strings. Sigh. > + enable; > + post-delay = <10>; > + }; > + gpio@1 { > + id = "enable-gpio"; > + enable; > + }; > + }; > + > +Note that first, the phandles of the regulator and gpio used in the sequences > +are defined as properties. Then the sequence references them through the id > +property of every step. The name of sub-properties defines the type of the step. > +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered > +sequentially. Oh I see. That's a little confusing. Why not just reference the relevant resources directly in each step; something more like: gpio@1 { action = "enable-gpio"; gpio = <&gpio 1 0>; }; I guess that might make parsing/building a little harder, since you'd have to detect when you'd already done gpio_request() on a given GPIO and not repeat it or something like that, but to me this makes the DT a lot easier to comprehend. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon, On 07/30/2012 08:00 PM, Simon Glass wrote: > For the delay, I think milliseconds is reasonable. I suppose there is > no reasonable need for microseconds? I don't see any need for microseconds myself - anybody sees use for finer-grained delays? Btw, I noticed I was using mdelay instead of msleep - caught and fixed that. >> +Device tree >> +----------- >> +All the same, power sequences can be encoded as device tree nodes. The following >> +properties and nodes are equivalent to the platform data defined previously: >> + >> + power-supply = <&mydevice_reg>; >> + enable-gpio = <&gpio 6 0>; >> + >> + power-on-sequence { >> + regulator@0 { >> + id = "power"; > > Is there a reason not to put the phandle here, like: > > id = <&mydevice_reg>; > > (or maybe 'device' instead of id?) There is one reason, but it might be a bad one. On Tegra, PWM phandle uses an extra cell to encode the duty-cycle the PWM should have when we call get_pwm(). This makes it possible to address the same PWM with different phandles (and different duty cycles), which causes an issue to know whether a PWM is already used in a sequence (potentially resulting in multiple get_pwm calls on the same PWM, and also opens the door to ambiguities in behavior (what is the correct duty-cycle to use if several different values are passed?) Maybe the problem lies in how PWM phandles are handled - if duty-cycle was not part of the information, we would not have this problem. >> +Note that first, the phandles of the regulator and gpio used in the sequences >> +are defined as properties. Then the sequence references them through the id >> +property of every step. The name of sub-properties defines the type of the step. >> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered >> +sequentially. > > For the regulator and gpio types I think you only have an enable. For > the pwm, what is the intended binding? Is that documented elsewhere? Same thing, enable/disable which would call pwm_enable and pwm_disable. One could also image an additional property to set the duty cycle if it can be taken off the phandle. > Also it might be worth mentioning how you get a struct power_seq from > an fdt node, and perhaps given an example of a device which has an > attached node, so we can see how it is referenced from the device > (of_parse_power_seq I think). Do put the sequence inside the device > node or reference it with a phandle? Yes, this definitely needs more documentation - especially the DT part. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 05:37:07PM +0900, Alex Courbot wrote: > Hi Simon, > > On 07/30/2012 08:00 PM, Simon Glass wrote: > >For the delay, I think milliseconds is reasonable. I suppose there is > >no reasonable need for microseconds? > > I don't see any need for microseconds myself - anybody sees use for > finer-grained delays? > > Btw, I noticed I was using mdelay instead of msleep - caught and fixed that. You might want to take a look at Documentation/timers/timers-howto.txt. msleep() isn't very accurate for periods shorter than 20 ms. > >>+Device tree > >>+----------- > >>+All the same, power sequences can be encoded as device tree nodes. The following > >>+properties and nodes are equivalent to the platform data defined previously: > >>+ > >>+ power-supply = <&mydevice_reg>; > >>+ enable-gpio = <&gpio 6 0>; > >>+ > >>+ power-on-sequence { > >>+ regulator@0 { > >>+ id = "power"; > > > >Is there a reason not to put the phandle here, like: > > > > id = <&mydevice_reg>; > > > >(or maybe 'device' instead of id?) > > There is one reason, but it might be a bad one. On Tegra, PWM > phandle uses an extra cell to encode the duty-cycle the PWM should > have when we call get_pwm(). This is not only the case on Tegra, but it is the default unless a driver specifically overrides it. The second cell specifies the index of the PWM device within the PWM chip. The third cell doesn't specify the duty cycle but the period of the PWM. > This makes it possible to address the > same PWM with different phandles (and different duty cycles), How so? A phandle will always refer to a PWM chip. Paired with the second cell, of_pwm_request() will resolve that into the correct PWM device. > which > causes an issue to know whether a PWM is already used in a sequence > (potentially resulting in multiple get_pwm calls on the same PWM, > and also opens the door to ambiguities in behavior (what is the > correct duty-cycle to use if several different values are passed?) You can't request a PWM multiple times. The second call well return -EBUSY, or rather ERR_PTR(-EBUSY). Thierry
On Mon, Jul 30, 2012 at 04:47:06PM +0100, Mark Brown wrote: > On Mon, Jul 30, 2012 at 10:44:29AM -0500, Rob Herring wrote: > > On 07/27/2012 07:05 AM, Alexandre Courbot wrote: > > > > + power-on-sequence { > > > + regulator@0 { > > > + id = "power"; > > > + enable; > > > What do this mean? Isn't this implied for a regulator? > > I assume you might have some sequences which need some things to be > turned off for some reason; it at least seems to be something you'd want > to design for. Furthermore there is the power-off-sequence equivalent, which you use for instance when you turn off the panel. Typically they would do the inverse of the power-on-sequence, so turning off a regulator is definitiely required. Thierry
On 07/30/2012 08:33 PM, Thierry Reding wrote: >> +You will need an instance of power_seq_resources to keep track of the resources >> +that are already allocated. On success, the function returns a devm allocated >> +resolved sequence that is ready to be passed to power_seq_run(). In case of >> +failure, and error code is returned. > > I don't quite understand why the struct power_seq_resources is needed. > Can this not be stored within power_seq? power_seq_resources serves two purposes: 1) When parsing sequences, it keeps track of the resources we have already allocated to avoid getting the same resource twice 2) On cleanup, it cleans the resources that needs to be freed (i.e. those that are not devm-handled). 2) can certainly be removed either by enforcing use of devm, or by doing reference counting. 1) seems more difficult to avoid - we need to keep track of the resources we already own between calls to power_seq_build(). I'd certainly be glad to remove that structure from public view and simplify the code if that is possible though. >> + >> +A resolved power sequence returned by power_seq_build can be run by >> +power_run_run(): >> + >> +int power_seq_run(struct device *dev, power_seq *seq); > > Why is the struct device required here? It already is passed during the > call to pwm_seq_build(), so perhaps you should keep a reference to it > within struct power_seq? The device is only needed for printing error messages. But as you point later, maybe messages should not be printed there at all. I will try to remove that parameter. >> +It returns 0 if the sequence has successfully been run, or an error code if a >> +problem occured. >> + >> +Finally, some resources that cannot be allocated through devm need to be freed >> +manually. Therefore, be sure to call power_seq_free_resources() in your device >> +remove function: >> + >> +void power_seq_free_resources(power_seq_resources *ress); > > Could this not also be handled by a managed version? If a power_seq is > always managed, then I would assume that it also takes care of freeing > the resources, even if the resources have no managed equivalents. Right. > Perhaps it would also make sense to provide non-managed version of these > functions. I think that would make the managed versions easier and more > canonical to implement. A power_seq is a single block of memory, so that should be reasonnably doable indeed. Let me think a little bit more about that. >> +Device tree >> +----------- >> +All the same, power sequences can be encoded as device tree nodes. The following >> +properties and nodes are equivalent to the platform data defined previously: >> + >> + power-supply = <&mydevice_reg>; >> + enable-gpio = <&gpio 6 0>; >> + >> + power-on-sequence { >> + regulator@0 { >> + id = "power"; >> + enable; >> + post-delay = <10>; >> + }; >> + gpio@1 { >> + id = "enable-gpio"; >> + enable; >> + }; >> + }; >> + >> +Note that first, the phandles of the regulator and gpio used in the sequences >> +are defined as properties. Then the sequence references them through the id >> +property of every step. The name of sub-properties defines the type of the step. >> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered >> +sequentially. > > I think there has been quite some discussion regarding the naming of > subnodes and the conclusion seems to have been to name them uniformly > after what they represent. As such the power-on-sequence subnodes should > be called step@0, step@1, etc. However, that will require the addition > of a property to define the type of resource. That's fine I guess - just adds some footprint to the DT, but nothing crazy. > Also, is there some way we can make the id property for GPIOs not > require the -gpio suffix? If the resource type is already GPIO, then it > seems redundant to add -gpio to the ID. There is unfortunately an inconsistency between the way regulators and GPIOs are gotten by name. regulator_get(id) will expect to find a property named "id-supply", while gpio_request_one(id) expects a property named exactly "id". To workaround this we could sprintf the correct property name from a non-suffixed property name within the driver, but I think this actually speaks more in favor of having phandles directly into the sequences. >> +config POWER_SEQ >> + bool >> + default n >> + > > "default n" is already the default, so you can drop that line. Did that, thanks. >> +#ifdef CONFIG_OF >> +#include <linux/of.h> >> +#include <linux/of_gpio.h> >> +#endif > > I think you don't need the CONFIG_OF guard around these. Both of.h and > of_gpio.h can be included unconditionally and actually contain dummy > definitions for the public functions in the !OF case. Fixed, thanks. >> +static int power_seq_step_run(struct power_seq_step *step) >> +{ >> + int err = 0; >> + >> + if (step->params.pre_delay) >> + mdelay(step->params.pre_delay); >> + >> + switch (step->resource->type) { >> +#ifdef CONFIG_REGULATOR >> + case POWER_SEQ_REGULATOR: >> + if (step->params.enable) >> + err = regulator_enable(step->resource->regulator); >> + else >> + err = regulator_disable(step->resource->regulator); >> + break; >> +#endif >> +#ifdef CONFIG_PWM >> + case POWER_SEQ_PWM: >> + if (step->params.enable) >> + err = pwm_enable(step->resource->pwm); >> + else >> + pwm_disable(step->resource->pwm); >> + break; >> +#endif >> +#ifdef CONFIG_GPIOLIB >> + case POWER_SEQ_GPIO: >> + gpio_set_value_cansleep(step->resource->gpio, >> + step->params.enable); >> + break; >> +#endif > > This kind of #ifdef'ery is quite ugly. I don't know if adding separate > *_run() functions for each type of resource would be any better, though. > Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and > GPIOLIB symbols to side-step the issue completely? If it is not realistic to consider a kernel built without regulator, pwm or gpiolib support, then we might as well do that. But isn't that a possibility? >> + if (!seq) return 0; > > I don't think this is acceptable according to the coding style. Also, > perhaps returning -EINVAL would be more meaningful? I neglected running checkpatch before submitting, apologies for that. The return value seems correct to me, a NULL sequence has no effect. >> + >> + while (seq->resource) { > > Perhaps this should check for POWER_SEQ_STOP instead? There is no resource for POWER_SEQ_STOP - therefore, a NULL resource is used instead. >> + if ((err = power_seq_step_run(seq++))) { >> + dev_err(dev, "error %d while running power sequence!\n", >> + err); > > For this kind of diagnostics it could be useful to have a name > associated with the power sequence. But I'm not sure that making the > power sequence code output an error here is the best solution. I find it > to be annoying when core code starts outputting too many error codes. In > this case it's particularily easy to catch the errors in the caller. Giving names to power sequences sounds like a good idea. Let me see how this can be done. It might require some more data structuring. >> + >> + while ((child = of_get_next_child(node, child))) >> + cpt++; > > for_each_child_of_node()? > >> + >> + /* allocate one more step to signal end of sequence */ >> + ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL); >> + if (!ret) >> + return ERR_PTR(-ENOMEM); >> + >> + cpt = 0; >> + while ((child = of_get_next_child(node, child))) { > > Here as well. Ah, didn't know that. Thanks. >> + /* first pass to count the number of steps to allocate */ >> + for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++); > > Wouldn't it be easier to pass around the number of steps in the sequence > instead of having to count in various places? This would be more along > the lines of how struct platform_device defines associated resources. My goal was to limit the number of data structures, but if we add a name to power sequences, we can add a steps count as well. >> + >> + if (!cpt) >> + return seq; > > Perhaps this should return an error-code as well? I find it nice to not > have to handle NULL specially when using ERR_PTR et al. Agreed. >> +typedef enum { >> + POWER_SEQ_STOP = 0, >> + POWER_SEQ_REGULATOR, >> + POWER_SEQ_PWM, >> + POWER_SEQ_GPIO, >> + POWER_SEQ_MAX, >> +} power_res_type; > > Maybe the prefix power_seq should be used here as well, so: > power_seq_res_type. Definitely. >> +typedef struct list_head power_seq_resources; > > No type definitions like this, please. Also, why define this particular > type globally? I will move that into a proper structure with a name and number of steps. >> + >> +struct power_step_params { >> + /* enable the resource if 1, disable if 0 */ >> + bool enable; >> + /* delay (in ms) to wait before executing the step */ >> + int pre_delay; >> + /* delay (in ms) to wait after executing the step */ >> + int post_delay; > > unsigned int for the delays? Yup. >> +typedef struct platform_power_seq_step platform_power_seq; > > Why are the parameters kept in a separate structure? What are the > disadvantages of keeping the in the sequence step structure directly? This ensures the same parameters are used for the platform data and resolved sequences, and also ensures they are all copied correctly using memcpy. But maybe I am just making something complex out of something that ought to be simpler. >> +struct power_seq_step { >> + struct power_seq_resource *resource; >> + struct power_step_params params; >> +}; >> +typedef struct power_seq_step power_seq; > > Would it make sense to make the struct power_seq opaque? I don't see why > anyone but the power_seq code should access the internals. I would like to do that actually. The issue is that it did not work go well with the legacy pwm_backlight behavior: a power sequence needs to be constructed out of a PWM obtained through pwm_request(int pwm_id, char *label) and this behavior cannot be emulated using the new platform data interface (which only works with pwm_get()). But if I remove this old behavior, then I could make power_seq opaque. I don't think many drivers are using it. What do you think? > For resource > managing it might also be easier to separate struct power_seq_step and > struct power_seq, making the power_seq basically something like: > > struct power_seq { > struct power_seq_step *steps; > unsigned int num_steps; > }; > > Perhaps a name field can be included for diagnostic purposes. Yes, looks like we are going in that direction. If this can be made private then the number of public data structures will not be too confusing (platform data only, basically). >> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, >> + platform_power_seq *pseq); > > I already mentioned this above: I fail to see why the ress parameter is > needed here. It is an internal implementation detail of the power > sequence code. Maybe a better place would be to include it within the > struct power_seq. Problem is that I need to track which resources are already allocated between calls to power_seq_build(). Even if I attach the resources into struct power_seq, they won't be attainable by the next call. So I'm afraid we are bound to pass a tracking structure at least to power_seq_build. >> +/** >> + * Free all the resources previously allocated by power_seq_allocate_resources. >> + */ >> +void power_seq_free_resources(power_seq_resources *ress); >> + >> +/** >> + * Run the given power sequence. Returns 0 on success, error code in case of >> + * failure. >> + */ >> +int power_seq_run(struct device *dev, power_seq *seq); > > I think the API is too fine grained here. From a user's point of view, > I'd expect a sequence like this: > > seq = power_seq_build(dev, sequence); > ... > power_seq_run(seq); > ... > power_seq_free(seq); > > Perhaps with managed variants where the power_seq_free() is executed > automatically: > > seq = devm_power_seq_build(dev, sequence); > ... > power_seq_run(seq); I agree. On top of that, of_parse_power_seq() should directly return a resolved power sequence, not the platform data. > Generally I really like where this is going. Thanks - I really appreciate your review. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2012 06:13 PM, Thierry Reding wrote: >> I don't see any need for microseconds myself - anybody sees use for >> finer-grained delays? >> >> Btw, I noticed I was using mdelay instead of msleep - caught and fixed that. > > You might want to take a look at Documentation/timers/timers-howto.txt. > msleep() isn't very accurate for periods shorter than 20 ms. Ok, looks like usleep_range is the way to go here. In that case it would probably not hurt to specify delays in microseconds in the DT and platform data as well. >>>> +Device tree >>>> +----------- >>>> +All the same, power sequences can be encoded as device tree nodes. The following >>>> +properties and nodes are equivalent to the platform data defined previously: >>>> + >>>> + power-supply = <&mydevice_reg>; >>>> + enable-gpio = <&gpio 6 0>; >>>> + >>>> + power-on-sequence { >>>> + regulator@0 { >>>> + id = "power"; >>> >>> Is there a reason not to put the phandle here, like: >>> >>> id = <&mydevice_reg>; >>> >>> (or maybe 'device' instead of id?) >> >> There is one reason, but it might be a bad one. On Tegra, PWM >> phandle uses an extra cell to encode the duty-cycle the PWM should >> have when we call get_pwm(). > > This is not only the case on Tegra, but it is the default unless a > driver specifically overrides it. The second cell specifies the index of > the PWM device within the PWM chip. The third cell doesn't specify the > duty cycle but the period of the PWM. Then I think there is a mistake in Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt: "the second cell is the duty cycle in nanoseconds." >> This makes it possible to address the >> same PWM with different phandles (and different duty cycles), > > How so? A phandle will always refer to a PWM chip. Paired with the > second cell, of_pwm_request() will resolve that into the correct PWM > device. For tegra, we can only address PWMs this way IIRC: pwm = <&pwm 2 5000000>; If we had <&pwm 2>, I agree that there would be no problem. But here the period of the PWM is also given - and in practice, we can request the same PWM using different phandles. For instance, if the above property was part of the power-on sequence, and the following pwm = <&pwm 2 0>; was part of power-off, how can I know that these two different phandles refer to the same PWM without calling pwm_get a second time and getting -EBUSY? Of course if the same period is specified for both, I will not have this issue as the phandles will be identical, but the possibility remains open that we are given a faulty tree here. More generally speaking, wouldn't it make more sense to have the period/duty cycle of a PWM encoded into separate properties when needed and have the phandle just reference the PWM instance? This also seems to stand from an API point of view, since the period is not specified when invoking pwm_request or pwm_get, but set by its own pwm_set_period function? On an unrelated note, I also don't understand why the period is also a parameter of pwm_config and why pwm_set_period does not do anything beyond setting a struct member that is returned by pwm_get_period (but maybe I am missing something here). Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2012 07:26 AM, Stephen Warren wrote: > On 07/30/2012 09:44 AM, Rob Herring wrote: >> On 07/27/2012 07:05 AM, Alexandre Courbot wrote: >>> Some device drivers (panel backlights especially) need to follow precise >>> sequences for powering on and off, involving gpios, regulators, PWMs >>> with a precise powering order and delays to respect between each steps. >>> These sequences are board-specific, and do not belong to a particular >>> driver - therefore they have been performed by board-specific hook >>> functions to far. >>> >>> With the advent of the device tree and of ARM kernels that are not >>> board-tied, we cannot rely on these board-specific hooks anymore but >>> need a way to implement these sequences in a portable manner. This patch >>> introduces a simple interpreter that can execute such power sequences >>> encoded either as platform data or within the device tree. >>> >> >> Why not? We'll always have some amount of board code. The key is to >> limit parts that are just data. I'm not sure this is something that >> should be in devicetree. >> >> Perhaps what is needed is a better way to hook into the driver like >> notifiers? > > I would answer that by asking the reverse question - why should we have > to put some data in DT, and some data into board files still? > > I'd certainly argue that the sequence of which GPIOs/regulators/PWMs to > manipulate is just data. > > To be honest, if we're going to have to put some parts of a board's > configuration into board files anyway, then the entirety of DT seems > useless; I'd far rather see all the configuration in one cohesive place > than arbitrarily split into two/n different locations - that would make > everything harder to maintain. Also, having these sequences into the DT would allow an older kernel to boot on and correctly initialize a newer board with - which is also part of the DT's purpose if I am not mistaken. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote: > On 07/30/2012 08:33 PM, Thierry Reding wrote: > >>+You will need an instance of power_seq_resources to keep track of the resources > >>+that are already allocated. On success, the function returns a devm allocated > >>+resolved sequence that is ready to be passed to power_seq_run(). In case of > >>+failure, and error code is returned. > > > >I don't quite understand why the struct power_seq_resources is needed. > >Can this not be stored within power_seq? > > power_seq_resources serves two purposes: > 1) When parsing sequences, it keeps track of the resources we have > already allocated to avoid getting the same resource twice > 2) On cleanup, it cleans the resources that needs to be freed (i.e. > those that are not devm-handled). > > 2) can certainly be removed either by enforcing use of devm, or by > doing reference counting. 1) seems more difficult to avoid - we need > to keep track of the resources we already own between calls to > power_seq_build(). I'd certainly be glad to remove that structure > from public view and simplify the code if that is possible though. I still don't see the problem. Managing the resources should be part of the power_seq core and shouldn't be visible to users. Maybe what you are worried about is that you may need the same resource both for a power-up and a power-down sequence? I can see how that would require a global list of resources. However I still think it would be easier to encapsulate that completely. Maybe another level of abstraction is required. You could for example add another type to encapsulate several power sequences and that could keep a list of used resources. I can't think of a good name, but maybe the following DT snippet clarifies what I mean: power-sequences { #address-cells = <1>; #size-cells = <0>; sequence@0 { name = "up"; #address-cells = <1>; #size-cells = <0>; step@0 { ... }; ... }; sequence@1 { name = "down"; #address-cells = <1>; #size-cells = <0>; step@0 { ... }; ... }; }; If you add a name property like this, you could extend the API to support running a named sequence: power_seq_run(seq, "up"); ... power_seq_run(seq, "down); > >Also, is there some way we can make the id property for GPIOs not > >require the -gpio suffix? If the resource type is already GPIO, then it > >seems redundant to add -gpio to the ID. > > There is unfortunately an inconsistency between the way regulators > and GPIOs are gotten by name. regulator_get(id) will expect to find > a property named "id-supply", while gpio_request_one(id) expects a > property named exactly "id". To workaround this we could sprintf the > correct property name from a non-suffixed property name within the > driver, but I think this actually speaks more in favor of having > phandles directly into the sequences. Yes, if it can be made to work by specifying the phandle directly that is certainly better. > >>+static int power_seq_step_run(struct power_seq_step *step) > >>+{ > >>+ int err = 0; > >>+ > >>+ if (step->params.pre_delay) > >>+ mdelay(step->params.pre_delay); > >>+ > >>+ switch (step->resource->type) { > >>+#ifdef CONFIG_REGULATOR > >>+ case POWER_SEQ_REGULATOR: > >>+ if (step->params.enable) > >>+ err = regulator_enable(step->resource->regulator); > >>+ else > >>+ err = regulator_disable(step->resource->regulator); > >>+ break; > >>+#endif > >>+#ifdef CONFIG_PWM > >>+ case POWER_SEQ_PWM: > >>+ if (step->params.enable) > >>+ err = pwm_enable(step->resource->pwm); > >>+ else > >>+ pwm_disable(step->resource->pwm); > >>+ break; > >>+#endif > >>+#ifdef CONFIG_GPIOLIB > >>+ case POWER_SEQ_GPIO: > >>+ gpio_set_value_cansleep(step->resource->gpio, > >>+ step->params.enable); > >>+ break; > >>+#endif > > > >This kind of #ifdef'ery is quite ugly. I don't know if adding separate > >*_run() functions for each type of resource would be any better, though. > >Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and > >GPIOLIB symbols to side-step the issue completely? > > If it is not realistic to consider a kernel built without regulator, > pwm or gpiolib support, then we might as well do that. But isn't > that a possibility? I'd say that anything complex enough to make use of power-sequencing probably has all of these enabled anyway. But maybe I'm not very qualified to judge. > > >>+ if (!seq) return 0; > > > >I don't think this is acceptable according to the coding style. Also, > >perhaps returning -EINVAL would be more meaningful? > > I neglected running checkpatch before submitting, apologies for > that. The return value seems correct to me, a NULL sequence has no > effect. But seq == NULL should never happen anyway, right? > > >>+ > >>+ while (seq->resource) { > > > >Perhaps this should check for POWER_SEQ_STOP instead? > > There is no resource for POWER_SEQ_STOP - therefore, a NULL resource > is used instead. Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of a sequence, so intuitively I'd be looking for that as a stop condition. > >>+typedef struct platform_power_seq_step platform_power_seq; > > > >Why are the parameters kept in a separate structure? What are the > >disadvantages of keeping the in the sequence step structure directly? > > This ensures the same parameters are used for the platform data and > resolved sequences, and also ensures they are all copied correctly > using memcpy. But maybe I am just making something complex out of > something that ought to be simpler. > > >>+struct power_seq_step { > >>+ struct power_seq_resource *resource; > >>+ struct power_step_params params; > >>+}; > >>+typedef struct power_seq_step power_seq; > > > >Would it make sense to make the struct power_seq opaque? I don't see why > >anyone but the power_seq code should access the internals. > > I would like to do that actually. The issue is that it did not work > go well with the legacy pwm_backlight behavior: a power sequence > needs to be constructed out of a PWM obtained through > pwm_request(int pwm_id, char *label) and this behavior cannot be > emulated using the new platform data interface (which only works > with pwm_get()). But if I remove this old behavior, then I could > make power_seq opaque. I don't think many drivers are using it. What > do you think? I don't see how that is relevant here, since this power-sequencing code is supposed to be generic and not tied to any specific implementation. Can you explain further? In any case you shouldn't be using pwm_request() in new code. > >For resource > >managing it might also be easier to separate struct power_seq_step and > >struct power_seq, making the power_seq basically something like: > > > > struct power_seq { > > struct power_seq_step *steps; > > unsigned int num_steps; > > }; > > > >Perhaps a name field can be included for diagnostic purposes. > > Yes, looks like we are going in that direction. If this can be made > private then the number of public data structures will not be too > confusing (platform data only, basically). Yes, that sounds like a much cleaner approach. > > >>+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, > >>+ platform_power_seq *pseq); > > > >I already mentioned this above: I fail to see why the ress parameter is > >needed here. It is an internal implementation detail of the power > >sequence code. Maybe a better place would be to include it within the > >struct power_seq. > > Problem is that I need to track which resources are already > allocated between calls to power_seq_build(). Even if I attach the > resources into struct power_seq, they won't be attainable by the > next call. So I'm afraid we are bound to pass a tracking structure > at least to power_seq_build. I think this could be solved nicely by what I proposed earlier. > >>+/** > >>+ * Free all the resources previously allocated by power_seq_allocate_resources. > >>+ */ > >>+void power_seq_free_resources(power_seq_resources *ress); > >>+ > >>+/** > >>+ * Run the given power sequence. Returns 0 on success, error code in case of > >>+ * failure. > >>+ */ > >>+int power_seq_run(struct device *dev, power_seq *seq); > > > >I think the API is too fine grained here. From a user's point of view, > >I'd expect a sequence like this: > > > > seq = power_seq_build(dev, sequence); > > ... > > power_seq_run(seq); > > ... > > power_seq_free(seq); > > > >Perhaps with managed variants where the power_seq_free() is executed > >automatically: > > > > seq = devm_power_seq_build(dev, sequence); > > ... > > power_seq_run(seq); > > I agree. On top of that, of_parse_power_seq() should directly return > a resolved power sequence, not the platform data. I'm not sure. The idiom seems to be to use DT as an alternative source for platform data, which I guess is due to most drivers already using platform data. But it has the advantage that after you have the platform data, be it from DT or directly specified, the subsequent code remains the same. Of course you could provide a separate of_build_power_seq() that wraps both steps for convenience. Thierry
On 07/31/2012 07:45 AM, Stephen Warren wrote: >> +- Delay to wait before performing the action, >> +- Delay to wait after performing the action. > > I don't see a need to have a delay both before and after an action; > except at the start of the sequence, step n's post-delay is at the same > point in the sequence as step n+1's pre-delay. Perhaps make a "delay" > step type? My first version used this actually - and you're right, having a "delay" step type would be more flexible and less redundant. >> +Both new resources and parameters can be introduced, but the goal is of course >> +to keep things as simple and compact as possible. > >> +The platform data is a simple array of platform_power_seq_step instances, each > > Rather than jumping straight into platform data here, I'd expect an > enumeration of legal resource types, and what actions can be performed > on each, followed by a description of a sequence (very simply, just a > list of actions and their parameters). This could be followed by a > section describing the mapping of the abstract concepts to concrete > platform data representation (and concrete device tree representation). Keeping that in mind for the next revision. >> +instance describing a step. The type as well as one of id or gpio members >> +(depending on the type) must be specified. The last step must be of type >> +POWER_SEQ_STOP. > > I'd certainly suggest having a step count rather than a sentinel value > in the list. As Thierry did - I think I will go that way. >> Regulator and PWM resources are identified by name. GPIO are >> +identified by number. > > That's a little implementation-specific. I guess it's entirely true for > a platform data representation, but not when mapping this into device tree. If we can come with a way to properly use phandles within DT sequences (and we should), then this will only apply to platform data. >> +You will need an instance of power_seq_resources to keep track of the resources >> +that are already allocated. On success, the function returns a devm allocated >> +resolved sequence that is ready to be passed to power_seq_run(). In case of >> +failure, and error code is returned. > > If the result is devm-allocated, the function probably should be named > devm_power_seq_build(). Right - more generally this needs to have both devm and non-devm variants. > I wonder if using the same structure/array as input and output would > simplify the API; the platform data would fill in the fields mentioned > above, and power_seq_build() would parse those, then set other fields in > the same structs to the looked-up handle values? The thing is that I am not sure what happens to the platform data once probe() is done. Isn't it customary to mark it with __devinit and have it freed after probing is successful? More generally, I think it is a good practice to have data structures tailored right for what they need to do - code with members that are meaningful only at given points of an instance's life tends to be more confusing. > You can make a custom devm free routine for the power_seq_resources > itself, so the overall free'ing of the content can be triggered by devm, > but the free'ing function can then call whatever non-devm APIs it wants > for the non-devm-allocated members. That sounds good. >> +Device tree >> +----------- >> +All the same, power sequences can be encoded as device tree nodes. The following >> +properties and nodes are equivalent to the platform data defined previously: >> + >> + power-supply = <&mydevice_reg>; >> + enable-gpio = <&gpio 6 0>; >> + >> + power-on-sequence { >> + regulator@0 { > > As Thierry mentioned, the step nodes should be named for the type of > object they are (a "step") not the type or name of resource they act > upon ("regulator" or "gpio"). Will fix that. > If the nodes have a unit address (i.e. end in "@n"), which they will > have to if all named "step" and there's more than one of them, then they > will need a matching reg property. Equally, the parent node will need > #address-cells and #size-cells too. So, the last couple lines would be: > > power-on-sequence { > #address-cells = <1>; > #size-cells = <0>; > step@0 { > reg = <0>; That's precisely what I would like to avoid - I don't need the steps to be numbered and I certainly have no use for a reg property. Isn't there a way to make it simpler? >> + id = "power"; > > "id" is usually a name or identifier. I think you mean "type" or perhaps > "action" here: > > type = "regulator"; > action = "enable"; > > or: > > action = "enable-regulator"; Right, that was a clear misuse. > Oh I see. That's a little confusing. Why not just reference the relevant > resources directly in each step; something more like: > > gpio@1 { > action = "enable-gpio"; > gpio = <&gpio 1 0>; > }; > > I guess that might make parsing/building a little harder, since you'd > have to detect when you'd already done gpio_request() on a given GPIO > and not repeat it or something like that, but to me this makes the DT a > lot easier to comprehend. You can see my reply to Thierry for the reason - the only issue with that is caused by PWM phandles. If we overcome this, then I agree we should use phandles. The code should not even get more complex as I have to check whether a resource is already allocated with strings as well. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 07:11:41PM +0900, Alex Courbot wrote: > On 07/31/2012 06:13 PM, Thierry Reding wrote: > >>I don't see any need for microseconds myself - anybody sees use for > >>finer-grained delays? > >> > >>Btw, I noticed I was using mdelay instead of msleep - caught and fixed that. > > > >You might want to take a look at Documentation/timers/timers-howto.txt. > >msleep() isn't very accurate for periods shorter than 20 ms. > > Ok, looks like usleep_range is the way to go here. In that case it > would probably not hurt to specify delays in microseconds in the DT > and platform data as well. > > >>>>+Device tree > >>>>+----------- > >>>>+All the same, power sequences can be encoded as device tree nodes. The following > >>>>+properties and nodes are equivalent to the platform data defined previously: > >>>>+ > >>>>+ power-supply = <&mydevice_reg>; > >>>>+ enable-gpio = <&gpio 6 0>; > >>>>+ > >>>>+ power-on-sequence { > >>>>+ regulator@0 { > >>>>+ id = "power"; > >>> > >>>Is there a reason not to put the phandle here, like: > >>> > >>> id = <&mydevice_reg>; > >>> > >>>(or maybe 'device' instead of id?) > >> > >>There is one reason, but it might be a bad one. On Tegra, PWM > >>phandle uses an extra cell to encode the duty-cycle the PWM should > >>have when we call get_pwm(). > > > >This is not only the case on Tegra, but it is the default unless a > >driver specifically overrides it. The second cell specifies the index of > >the PWM device within the PWM chip. The third cell doesn't specify the > >duty cycle but the period of the PWM. > > Then I think there is a mistake in > Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt: > > "the second cell is the duty cycle in nanoseconds." Yes, that's a mistake. =\ > >>This makes it possible to address the > >>same PWM with different phandles (and different duty cycles), > > > >How so? A phandle will always refer to a PWM chip. Paired with the > >second cell, of_pwm_request() will resolve that into the correct PWM > >device. > > For tegra, we can only address PWMs this way IIRC: > > pwm = <&pwm 2 5000000>; > > If we had <&pwm 2>, I agree that there would be no problem. But here > the period of the PWM is also given - and in practice, we can > request the same PWM using different phandles. For instance, if the > above property was part of the power-on sequence, and the following > > pwm = <&pwm 2 0>; > > was part of power-off, how can I know that these two different > phandles refer to the same PWM without calling pwm_get a second time > and getting -EBUSY? You should specify the same period regardless of the sequence. But you are right, you still cannot request the device twice. > Of course if the same period is specified for both, I will not have > this issue as the phandles will be identical, but the possibility > remains open that we are given a faulty tree here. I think the phandle is in fact only the reference to the PWM chip, that is: &pwm. The second cell, the PWM index, is part of the PWM specifier. However the issue doesn't go away if you drop the period cell because you still won't be able to request the PWM device a second time. How is this solved for regulators and GPIOs? At least for GPIOs I'm pretty sure that you can't request them more than once either. > More generally speaking, wouldn't it make more sense to have the > period/duty cycle of a PWM encoded into separate properties when > needed and have the phandle just reference the PWM instance? This > also seems to stand from an API point of view, since the period is > not specified when invoking pwm_request or pwm_get, but set by its > own pwm_set_period function? The problem with specifying the period in a separate property is how to map them to the correct PWM device. From a hardware description point of view, making the period part of the specifier makes a lot of sense and hardware description is what DT is about. > On an unrelated note, I also don't understand why the period is also > a parameter of pwm_config and why pwm_set_period does not do > anything beyond setting a struct member that is returned by > pwm_get_period (but maybe I am missing something here). pwm_config() is the legacy API that we need to support for compatibility reasons. Eventually this interface should probably be changed. pwm_get_period() and pwm_set_period() were merely introduced to support DT, but I could imagine them becoming the canonical way for configuring PWM devices in the future, perhaps with a complementary pwm_set_duty_cycle(). But first we need to convert drivers and users. Thierry
On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: > On 07/31/2012 07:45 AM, Stephen Warren wrote: > >I wonder if using the same structure/array as input and output would > >simplify the API; the platform data would fill in the fields mentioned > >above, and power_seq_build() would parse those, then set other fields in > >the same structs to the looked-up handle values? > > The thing is that I am not sure what happens to the platform data > once probe() is done. Isn't it customary to mark it with __devinit > and have it freed after probing is successful? No, platform data should stay around forever. Otherwise, consider what would happen if your driver is built as a module and you unload and load it again. > More generally, I think it is a good practice to have data > structures tailored right for what they need to do - code with > members that are meaningful only at given points of an instance's > life tends to be more confusing. I agree. Furthermore the driver unload/reload would be another reason not to reuse platform data as the output of the build() function. But maybe what Stephen meant was more like filling a structure with data taken from the platform data and pass that to a resolve() function which would fill in the missing pieces like pointers to actual resources. I imagine a managed interface would become a little trickier to do using such an approach. > >If the nodes have a unit address (i.e. end in "@n"), which they will > >have to if all named "step" and there's more than one of them, then they > >will need a matching reg property. Equally, the parent node will need > >#address-cells and #size-cells too. So, the last couple lines would be: > > > > power-on-sequence { > > #address-cells = <1>; > > #size-cells = <0>; > > step@0 { > > reg = <0>; > > That's precisely what I would like to avoid - I don't need the steps > to be numbered and I certainly have no use for a reg property. Isn't > there a way to make it simpler? It's not technically valid to not have the reg property. Or #address-cells and #size-cells properties for that matter. Thierry
On 7/31/2012 6:56 PM, Thierry Reding wrote: > On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: >> On 07/31/2012 07:45 AM, Stephen Warren wrote: >>> I wonder if using the same structure/array as input and output would >>> simplify the API; the platform data would fill in the fields mentioned >>> above, and power_seq_build() would parse those, then set other fields in >>> the same structs to the looked-up handle values? >> >> The thing is that I am not sure what happens to the platform data >> once probe() is done. Isn't it customary to mark it with __devinit >> and have it freed after probing is successful? > > No, platform data should stay around forever. Otherwise, consider what > would happen if your driver is built as a module and you unload and load > it again. > >> More generally, I think it is a good practice to have data >> structures tailored right for what they need to do - code with >> members that are meaningful only at given points of an instance's >> life tends to be more confusing. > > I agree. Furthermore the driver unload/reload would be another reason > not to reuse platform data as the output of the build() function. > > But maybe what Stephen meant was more like filling a structure with data > taken from the platform data and pass that to a resolve() function which > would fill in the missing pieces like pointers to actual resources. I > imagine a managed interface would become a little trickier to do using > such an approach. > >>> If the nodes have a unit address (i.e. end in "@n"), which they will >>> have to if all named "step" and there's more than one of them, then they >>> will need a matching reg property. Equally, the parent node will need >>> #address-cells and #size-cells too. So, the last couple lines would be: >>> >>> power-on-sequence { >>> #address-cells = <1>; >>> #size-cells = <0>; >>> step@0 { >>> reg = <0>; >> >> That's precisely what I would like to avoid - I don't need the steps >> to be numbered and I certainly have no use for a reg property. Isn't >> there a way to make it simpler? > > It's not technically valid to not have the reg property. Or > #address-cells and #size-cells properties for that matter. I'm not keen on this representation where individual steps are nodes. That seems like it could end up being too "heavyweight" for a long sequence. > > Thierry > > > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote: > On 7/31/2012 6:56 PM, Thierry Reding wrote: > > On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: > >> On 07/31/2012 07:45 AM, Stephen Warren wrote: > >>> I wonder if using the same structure/array as input and output would > >>> simplify the API; the platform data would fill in the fields mentioned > >>> above, and power_seq_build() would parse those, then set other fields in > >>> the same structs to the looked-up handle values? > >> > >> The thing is that I am not sure what happens to the platform data > >> once probe() is done. Isn't it customary to mark it with __devinit > >> and have it freed after probing is successful? > > > > No, platform data should stay around forever. Otherwise, consider what > > would happen if your driver is built as a module and you unload and load > > it again. > > > >> More generally, I think it is a good practice to have data > >> structures tailored right for what they need to do - code with > >> members that are meaningful only at given points of an instance's > >> life tends to be more confusing. > > > > I agree. Furthermore the driver unload/reload would be another reason > > not to reuse platform data as the output of the build() function. > > > > But maybe what Stephen meant was more like filling a structure with data > > taken from the platform data and pass that to a resolve() function which > > would fill in the missing pieces like pointers to actual resources. I > > imagine a managed interface would become a little trickier to do using > > such an approach. > > > >>> If the nodes have a unit address (i.e. end in "@n"), which they will > >>> have to if all named "step" and there's more than one of them, then they > >>> will need a matching reg property. Equally, the parent node will need > >>> #address-cells and #size-cells too. So, the last couple lines would be: > >>> > >>> power-on-sequence { > >>> #address-cells = <1>; > >>> #size-cells = <0>; > >>> step@0 { > >>> reg = <0>; > >> > >> That's precisely what I would like to avoid - I don't need the steps > >> to be numbered and I certainly have no use for a reg property. Isn't > >> there a way to make it simpler? > > > > It's not technically valid to not have the reg property. Or > > #address-cells and #size-cells properties for that matter. > > I'm not keen on this representation where individual steps are nodes. > That seems like it could end up being too "heavyweight" for a long sequence. The other alternative would involve using a single property to encode one sequence. I think that was the initial proposal, though using proper phandle encoding it could probably be enhanced a bit. However anything that involves a single property has the problem that we need to encode the type of resource as an integer, and that makes things very hard to read. So it would look something like this: power-on = <1 &gpio 6 0 1 0 10000 2 ® 1 3 &pwm 0 5000000 1>; power-off = <3 &pwm 0 5000000 0 2 ® 0 0 10000 1 &gpio 6 0 0>; So the first cell would encode the type: 0: delay 1: gpio 2: regulator 3: PWM The next n cells would be the phandle and the specifier, while the last cell would encode a resource-specific parameter: delay: time in microseconds gpio: set level (0: low, 1: high) regulator: 0: disable, 1: enable pwm: 0: disable, 1: enable I guess this would be more compact, but it is also very hard to read. Is that something you would be happier with? Perhaps you were thinking of something completely different? Thierry
On 7/31/2012 8:38 PM, Thierry Reding wrote: > On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote: >> On 7/31/2012 6:56 PM, Thierry Reding wrote: >>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: >>>> On 07/31/2012 07:45 AM, Stephen Warren wrote: >>>>> I wonder if using the same structure/array as input and output would >>>>> simplify the API; the platform data would fill in the fields mentioned >>>>> above, and power_seq_build() would parse those, then set other fields in >>>>> the same structs to the looked-up handle values? >>>> >>>> The thing is that I am not sure what happens to the platform data >>>> once probe() is done. Isn't it customary to mark it with __devinit >>>> and have it freed after probing is successful? >>> >>> No, platform data should stay around forever. Otherwise, consider what >>> would happen if your driver is built as a module and you unload and load >>> it again. >>> >>>> More generally, I think it is a good practice to have data >>>> structures tailored right for what they need to do - code with >>>> members that are meaningful only at given points of an instance's >>>> life tends to be more confusing. >>> >>> I agree. Furthermore the driver unload/reload would be another reason >>> not to reuse platform data as the output of the build() function. >>> >>> But maybe what Stephen meant was more like filling a structure with data >>> taken from the platform data and pass that to a resolve() function which >>> would fill in the missing pieces like pointers to actual resources. I >>> imagine a managed interface would become a little trickier to do using >>> such an approach. >>> >>>>> If the nodes have a unit address (i.e. end in "@n"), which they will >>>>> have to if all named "step" and there's more than one of them, then they >>>>> will need a matching reg property. Equally, the parent node will need >>>>> #address-cells and #size-cells too. So, the last couple lines would be: >>>>> >>>>> power-on-sequence { >>>>> #address-cells = <1>; >>>>> #size-cells = <0>; >>>>> step@0 { >>>>> reg = <0>; >>>> >>>> That's precisely what I would like to avoid - I don't need the steps >>>> to be numbered and I certainly have no use for a reg property. Isn't >>>> there a way to make it simpler? >>> >>> It's not technically valid to not have the reg property. Or >>> #address-cells and #size-cells properties for that matter. >> >> I'm not keen on this representation where individual steps are nodes. >> That seems like it could end up being too "heavyweight" for a long sequence. > > The other alternative would involve using a single property to encode > one sequence. I think that was the initial proposal, though using proper > phandle encoding it could probably be enhanced a bit. However anything > that involves a single property has the problem that we need to encode > the type of resource as an integer, and that makes things very hard to > read. > > So it would look something like this: > > power-on = <1 &gpio 6 0 1 > 0 10000 > 2 ® 1 > 3 &pwm 0 5000000 1>; > > power-off = <3 &pwm 0 5000000 0 > 2 ® 0 > 0 10000 > 1 &gpio 6 0 0>; > > So the first cell would encode the type: > 0: delay > 1: gpio > 2: regulator > 3: PWM > > The next n cells would be the phandle and the specifier, while the last > cell would encode a resource-specific parameter: > delay: time in microseconds > gpio: set level (0: low, 1: high) > regulator: 0: disable, 1: enable > pwm: 0: disable, 1: enable > > I guess this would be more compact, but it is also very hard to read. Is > that something you would be happier with? Perhaps you were thinking of > something completely different? Perhaps a compact/flexible encoding could be designed, with a textual encoding that is easy to read. A separate tool could convert the text encoding to the integer format, annotated with comments containing the "source text". A file containing that output could be #included into the dts file. > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote: > 2) On cleanup, it cleans the resources that needs to be freed (i.e. > those that are not devm-handled). > 2) can certainly be removed either by enforcing use of devm, or by > doing reference counting. 1) seems more difficult to avoid - we need I'd just add devm_ for everything that's not got it. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 12:56:40PM +0200, Thierry Reding wrote: > On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: > > The thing is that I am not sure what happens to the platform data > > once probe() is done. Isn't it customary to mark it with __devinit > > and have it freed after probing is successful? > No, platform data should stay around forever. Otherwise, consider what > would happen if your driver is built as a module and you unload and load > it again. __devinit can be discarded if you disable enough kernel features, HOTPLUG is the main one IIRC, modules might also need to go - drivers really ought to take a copy of platform data they plan to use at runtime, though practically speaking you have to try to trigger any problems. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 03:13:29PM +0100, Mark Brown wrote: > On Tue, Jul 31, 2012 at 12:56:40PM +0200, Thierry Reding wrote: > > On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: > > > > The thing is that I am not sure what happens to the platform data > > > once probe() is done. Isn't it customary to mark it with __devinit > > > and have it freed after probing is successful? > > > No, platform data should stay around forever. Otherwise, consider what > > would happen if your driver is built as a module and you unload and load > > it again. > > __devinit can be discarded if you disable enough kernel features, > HOTPLUG is the main one IIRC, modules might also need to go - drivers > really ought to take a copy of platform data they plan to use at > runtime, though practically speaking you have to try to trigger any > problems. HOTPLUG is marked EXPERT and explicitly states that it should only be disabled if you're not using modules or dynamic device discovery. I think if you've ignored all of that you're no longer entitled to complain. Thierry
On Tue, Jul 31, 2012 at 05:37:07PM +0900, Alex Courbot wrote: > On 07/30/2012 08:00 PM, Simon Glass wrote: > >For the delay, I think milliseconds is reasonable. I suppose there is > >no reasonable need for microseconds? > I don't see any need for microseconds myself - anybody sees use for > finer-grained delays? Bouncing reset lines, often the time required for reset is down in the microseconds. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 04:22:17PM +0200, Thierry Reding wrote: > On Tue, Jul 31, 2012 at 03:13:29PM +0100, Mark Brown wrote: > > __devinit can be discarded if you disable enough kernel features, > > HOTPLUG is the main one IIRC, modules might also need to go - drivers > > really ought to take a copy of platform data they plan to use at > > runtime, though practically speaking you have to try to trigger any > > problems. > HOTPLUG is marked EXPERT and explicitly states that it should only be > disabled if you're not using modules or dynamic device discovery. I > think if you've ignored all of that you're no longer entitled to > complain. This is framework code - it doesn't have much option. Disabling HOTPLUG is totally reasonable on space constrained systems, there's no reason for the code to break things for people. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote: > On Tue, Jul 31, 2012 at 04:22:17PM +0200, Thierry Reding wrote: > > On Tue, Jul 31, 2012 at 03:13:29PM +0100, Mark Brown wrote: > > > > __devinit can be discarded if you disable enough kernel features, > > > HOTPLUG is the main one IIRC, modules might also need to go - drivers > > > really ought to take a copy of platform data they plan to use at > > > runtime, though practically speaking you have to try to trigger any > > > problems. > > > HOTPLUG is marked EXPERT and explicitly states that it should only be > > disabled if you're not using modules or dynamic device discovery. I > > think if you've ignored all of that you're no longer entitled to > > complain. > > This is framework code - it doesn't have much option. Disabling HOTPLUG > is totally reasonable on space constrained systems, there's no reason > for the code to break things for people. Still if you use this code and disable HOTPLUG, then you shouldn't be using modules either. I mean there is no way you can write a driver that can gracefully handle its platform data being discarded. Thierry
On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote: > On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote: > > This is framework code - it doesn't have much option. Disabling HOTPLUG > > is totally reasonable on space constrained systems, there's no reason > > for the code to break things for people. > Still if you use this code and disable HOTPLUG, then you shouldn't be > using modules either. I mean there is no way you can write a driver that Of course. > can gracefully handle its platform data being discarded. Sure there is - take a copy of the platform data in probe(). -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote: > On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote: > > On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote: > > > > This is framework code - it doesn't have much option. Disabling HOTPLUG > > > is totally reasonable on space constrained systems, there's no reason > > > for the code to break things for people. > > > Still if you use this code and disable HOTPLUG, then you shouldn't be > > using modules either. I mean there is no way you can write a driver that > > Of course. > > > can gracefully handle its platform data being discarded. > > Sure there is - take a copy of the platform data in probe(). Ick ick ick, devdata needs to die as it's pretty much pointless these days. It's on my TODO list actually... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 09:19:54AM -0700, Greg Kroah-Hartman wrote: > On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote: > > On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote: > > > can gracefully handle its platform data being discarded. > > Sure there is - take a copy of the platform data in probe(). > Ick ick ick, devdata needs to die as it's pretty much pointless these > days. Hrm? I'm not sure I understand the direct relevance here - we're talking about platform data. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2012 04:32 AM, Alex Courbot wrote: > On 07/31/2012 07:45 AM, Stephen Warren wrote: ... >> If the nodes have a unit address (i.e. end in "@n"), which they will >> have to if all named "step" and there's more than one of them, then they >> will need a matching reg property. Equally, the parent node will need >> #address-cells and #size-cells too. So, the last couple lines would be: >> >> power-on-sequence { >> #address-cells = <1>; >> #size-cells = <0>; >> step@0 { >> reg = <0>; > > That's precisely what I would like to avoid - I don't need the steps to > be numbered and I certainly have no use for a reg property. Isn't there > a way to make it simpler? You may be able to get away without using the reg values in the code. However, to have a semantically correct device tree, you really do need all of those properties. That said, I think you might need to use the reg values in code. I believe there's no guarantee of the order in which nodes enumerate in device tree, so you need to look at the reg property in order to find the order in which to execute the nodes/steps. ... although perhaps the desire to avoid a dependency on DT ordering applies more to the order in which devices instantiated from DT get probed, rather than the order of low-level node enumeration, so I may be wrong here. I've definitely seen Grant caution people not to rely on device probe order, but perhaps he didn't make the same comment about node order. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 05:22:30PM +0100, Mark Brown wrote: > On Tue, Jul 31, 2012 at 09:19:54AM -0700, Greg Kroah-Hartman wrote: > > On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote: > > > On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote: > > > > > can gracefully handle its platform data being discarded. > > > > Sure there is - take a copy of the platform data in probe(). > > > Ick ick ick, devdata needs to die as it's pretty much pointless these > > days. > > Hrm? I'm not sure I understand the direct relevance here - we're > talking about platform data. The platform data was marked __devdata, and you said it could be thrown away after init so you should keep it around somehow. Anyway, not really the main point here at all, sorry for the noise... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2012 at 09:42:45AM -0700, Greg Kroah-Hartman wrote: > On Tue, Jul 31, 2012 at 05:22:30PM +0100, Mark Brown wrote: > > Hrm? I'm not sure I understand the direct relevance here - we're > > talking about platform data. > The platform data was marked __devdata, and you said it could be thrown > away after init so you should keep it around somehow. Ah, sorry - I was thinking __devinitdata. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 30, 2012 at 10:59:39PM +0200, Rafael J. Wysocki wrote: [...] > > Well, currently drivers/power/ is indeed just for power supply class > > subsystem and drivers. But if the trend is to gather power management > > ("policy") stuff under one directory, i.e. > > > > drivers/ > > power/ > > supplies/ <- former "power supply class and drivers" > > regulators/ > > idle/ > > cpuidle/ > > cpufreq/ > > devfreq/ > > avs/ > > ... > > > > That would probably make sense, we could easily see the big picture. > > But if we're not going to do this long-term, > > Yes, we may do this eventually, but surely not right now. OK, great! Then I'll probably start thinking about moving power supply drivers into its own directory, maybe in 3.7. Thanks!
On 07/31/2012 09:22 PM, Mitch Bradley wrote: > On 7/31/2012 6:56 PM, Thierry Reding wrote: >> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: >>> On 07/31/2012 07:45 AM, Stephen Warren wrote: >>>> I wonder if using the same structure/array as input and output would >>>> simplify the API; the platform data would fill in the fields mentioned >>>> above, and power_seq_build() would parse those, then set other fields in >>>> the same structs to the looked-up handle values? >>> >>> The thing is that I am not sure what happens to the platform data >>> once probe() is done. Isn't it customary to mark it with __devinit >>> and have it freed after probing is successful? >> >> No, platform data should stay around forever. Otherwise, consider what >> would happen if your driver is built as a module and you unload and load >> it again. >> >>> More generally, I think it is a good practice to have data >>> structures tailored right for what they need to do - code with >>> members that are meaningful only at given points of an instance's >>> life tends to be more confusing. >> >> I agree. Furthermore the driver unload/reload would be another reason >> not to reuse platform data as the output of the build() function. >> >> But maybe what Stephen meant was more like filling a structure with data >> taken from the platform data and pass that to a resolve() function which >> would fill in the missing pieces like pointers to actual resources. I >> imagine a managed interface would become a little trickier to do using >> such an approach. >> >>>> If the nodes have a unit address (i.e. end in "@n"), which they will >>>> have to if all named "step" and there's more than one of them, then they >>>> will need a matching reg property. Equally, the parent node will need >>>> #address-cells and #size-cells too. So, the last couple lines would be: >>>> >>>> power-on-sequence { >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> step@0 { >>>> reg = <0>; >>> >>> That's precisely what I would like to avoid - I don't need the steps >>> to be numbered and I certainly have no use for a reg property. Isn't >>> there a way to make it simpler? >> >> It's not technically valid to not have the reg property. Or >> #address-cells and #size-cells properties for that matter. > > I'm not keen on this representation where individual steps are nodes. > That seems like it could end up being too "heavyweight" for a long sequence. Using nodes has a big advantage though: we can use any arbitrary property to add extra parameters to the resource being controlled. Right now we only use enable/disable, but for example one can imagine an optional voltage setting for a regulator. It is much more future-proof than a design where the number of parameters would be fixed and could not be extended without breaking compatibility. I experimented encoding the whole sequence within a single property. It works of course, but is not really flexible, hard to read, and quite error-prone overall. The memory footprint gain was not so obvious neither (although it was indeed more compact). What bothers me is to have to specify the cells layout and reg property for *every single sequence*, but well, I guess we can live with that. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2012 09:55 PM, Mitch Bradley wrote: > On 7/31/2012 8:38 PM, Thierry Reding wrote: >> On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote: >>> On 7/31/2012 6:56 PM, Thierry Reding wrote: >>>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: >>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote: >>>>>> I wonder if using the same structure/array as input and output would >>>>>> simplify the API; the platform data would fill in the fields mentioned >>>>>> above, and power_seq_build() would parse those, then set other fields in >>>>>> the same structs to the looked-up handle values? >>>>> >>>>> The thing is that I am not sure what happens to the platform data >>>>> once probe() is done. Isn't it customary to mark it with __devinit >>>>> and have it freed after probing is successful? >>>> >>>> No, platform data should stay around forever. Otherwise, consider what >>>> would happen if your driver is built as a module and you unload and load >>>> it again. >>>> >>>>> More generally, I think it is a good practice to have data >>>>> structures tailored right for what they need to do - code with >>>>> members that are meaningful only at given points of an instance's >>>>> life tends to be more confusing. >>>> >>>> I agree. Furthermore the driver unload/reload would be another reason >>>> not to reuse platform data as the output of the build() function. >>>> >>>> But maybe what Stephen meant was more like filling a structure with data >>>> taken from the platform data and pass that to a resolve() function which >>>> would fill in the missing pieces like pointers to actual resources. I >>>> imagine a managed interface would become a little trickier to do using >>>> such an approach. >>>> >>>>>> If the nodes have a unit address (i.e. end in "@n"), which they will >>>>>> have to if all named "step" and there's more than one of them, then they >>>>>> will need a matching reg property. Equally, the parent node will need >>>>>> #address-cells and #size-cells too. So, the last couple lines would be: >>>>>> >>>>>> power-on-sequence { >>>>>> #address-cells = <1>; >>>>>> #size-cells = <0>; >>>>>> step@0 { >>>>>> reg = <0>; >>>>> >>>>> That's precisely what I would like to avoid - I don't need the steps >>>>> to be numbered and I certainly have no use for a reg property. Isn't >>>>> there a way to make it simpler? >>>> >>>> It's not technically valid to not have the reg property. Or >>>> #address-cells and #size-cells properties for that matter. >>> >>> I'm not keen on this representation where individual steps are nodes. >>> That seems like it could end up being too "heavyweight" for a long sequence. >> >> The other alternative would involve using a single property to encode >> one sequence. I think that was the initial proposal, though using proper >> phandle encoding it could probably be enhanced a bit. However anything >> that involves a single property has the problem that we need to encode >> the type of resource as an integer, and that makes things very hard to >> read. >> >> So it would look something like this: >> >> power-on = <1 &gpio 6 0 1 >> 0 10000 >> 2 ® 1 >> 3 &pwm 0 5000000 1>; >> >> power-off = <3 &pwm 0 5000000 0 >> 2 ® 0 >> 0 10000 >> 1 &gpio 6 0 0>; >> >> So the first cell would encode the type: >> 0: delay >> 1: gpio >> 2: regulator >> 3: PWM >> >> The next n cells would be the phandle and the specifier, while the last >> cell would encode a resource-specific parameter: >> delay: time in microseconds >> gpio: set level (0: low, 1: high) >> regulator: 0: disable, 1: enable >> pwm: 0: disable, 1: enable >> >> I guess this would be more compact, but it is also very hard to read. Is >> that something you would be happier with? Perhaps you were thinking of >> something completely different? > > > Perhaps a compact/flexible encoding could be designed, with a textual > encoding that is easy to read. A separate tool could convert the text > encoding to the integer format, annotated with comments containing > the "source text". A file containing that output could be #included > into the dts file. Do you mean having a external compiler that would run before dtc just for producing the power sequences? That sounds a little bit overkill for something that ough to remain simple. Also, although I admit I don't have the whole picture of where they could be used, I don't expect the power sequences to grow to sizes that would make us bother about their footprint. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/1/2012 9:47 AM, Alex Courbot wrote: > On 07/31/2012 09:55 PM, Mitch Bradley wrote: >> On 7/31/2012 8:38 PM, Thierry Reding wrote: >>> On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote: >>>> On 7/31/2012 6:56 PM, Thierry Reding wrote: >>>>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote: >>>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote: >>>>>>> I wonder if using the same structure/array as input and output would >>>>>>> simplify the API; the platform data would fill in the fields mentioned >>>>>>> above, and power_seq_build() would parse those, then set other fields in >>>>>>> the same structs to the looked-up handle values? >>>>>> >>>>>> The thing is that I am not sure what happens to the platform data >>>>>> once probe() is done. Isn't it customary to mark it with __devinit >>>>>> and have it freed after probing is successful? >>>>> >>>>> No, platform data should stay around forever. Otherwise, consider what >>>>> would happen if your driver is built as a module and you unload and load >>>>> it again. >>>>> >>>>>> More generally, I think it is a good practice to have data >>>>>> structures tailored right for what they need to do - code with >>>>>> members that are meaningful only at given points of an instance's >>>>>> life tends to be more confusing. >>>>> >>>>> I agree. Furthermore the driver unload/reload would be another reason >>>>> not to reuse platform data as the output of the build() function. >>>>> >>>>> But maybe what Stephen meant was more like filling a structure with data >>>>> taken from the platform data and pass that to a resolve() function which >>>>> would fill in the missing pieces like pointers to actual resources. I >>>>> imagine a managed interface would become a little trickier to do using >>>>> such an approach. >>>>> >>>>>>> If the nodes have a unit address (i.e. end in "@n"), which they will >>>>>>> have to if all named "step" and there's more than one of them, then they >>>>>>> will need a matching reg property. Equally, the parent node will need >>>>>>> #address-cells and #size-cells too. So, the last couple lines would be: >>>>>>> >>>>>>> power-on-sequence { >>>>>>> #address-cells = <1>; >>>>>>> #size-cells = <0>; >>>>>>> step@0 { >>>>>>> reg = <0>; >>>>>> >>>>>> That's precisely what I would like to avoid - I don't need the steps >>>>>> to be numbered and I certainly have no use for a reg property. Isn't >>>>>> there a way to make it simpler? >>>>> >>>>> It's not technically valid to not have the reg property. Or >>>>> #address-cells and #size-cells properties for that matter. >>>> >>>> I'm not keen on this representation where individual steps are nodes. >>>> That seems like it could end up being too "heavyweight" for a long sequence. >>> >>> The other alternative would involve using a single property to encode >>> one sequence. I think that was the initial proposal, though using proper >>> phandle encoding it could probably be enhanced a bit. However anything >>> that involves a single property has the problem that we need to encode >>> the type of resource as an integer, and that makes things very hard to >>> read. >>> >>> So it would look something like this: >>> >>> power-on = <1 &gpio 6 0 1 >>> 0 10000 >>> 2 ® 1 >>> 3 &pwm 0 5000000 1>; >>> >>> power-off = <3 &pwm 0 5000000 0 >>> 2 ® 0 >>> 0 10000 >>> 1 &gpio 6 0 0>; >>> >>> So the first cell would encode the type: >>> 0: delay >>> 1: gpio >>> 2: regulator >>> 3: PWM >>> >>> The next n cells would be the phandle and the specifier, while the last >>> cell would encode a resource-specific parameter: >>> delay: time in microseconds >>> gpio: set level (0: low, 1: high) >>> regulator: 0: disable, 1: enable >>> pwm: 0: disable, 1: enable >>> >>> I guess this would be more compact, but it is also very hard to read. Is >>> that something you would be happier with? Perhaps you were thinking of >>> something completely different? >> >> >> Perhaps a compact/flexible encoding could be designed, with a textual >> encoding that is easy to read. A separate tool could convert the text >> encoding to the integer format, annotated with comments containing >> the "source text". A file containing that output could be #included >> into the dts file. > > Do you mean having a external compiler that would run before dtc just > for producing the power sequences? That sounds a little bit overkill for > something that ough to remain simple. > > Also, although I admit I don't have the whole picture of where they > could be used, I don't expect the power sequences to grow to sizes that > would make us bother about their footprint. It is axiomatic that every "language", if it succeeds at all, eventually grows into a complete programming language. The more special-purpose that it starts as, the uglier that it ends up. > > Alex. > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2012 07:19 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote: >> On 07/30/2012 08:33 PM, Thierry Reding wrote: >>>> +You will need an instance of power_seq_resources to keep track of the resources >>>> +that are already allocated. On success, the function returns a devm allocated >>>> +resolved sequence that is ready to be passed to power_seq_run(). In case of >>>> +failure, and error code is returned. >>> >>> I don't quite understand why the struct power_seq_resources is needed. >>> Can this not be stored within power_seq? >> >> power_seq_resources serves two purposes: >> 1) When parsing sequences, it keeps track of the resources we have >> already allocated to avoid getting the same resource twice >> 2) On cleanup, it cleans the resources that needs to be freed (i.e. >> those that are not devm-handled). >> >> 2) can certainly be removed either by enforcing use of devm, or by >> doing reference counting. 1) seems more difficult to avoid - we need >> to keep track of the resources we already own between calls to >> power_seq_build(). I'd certainly be glad to remove that structure >> from public view and simplify the code if that is possible though. > > I still don't see the problem. Managing the resources should be part of > the power_seq core and shouldn't be visible to users. Maybe what you are > worried about is that you may need the same resource both for a power-up > and a power-down sequence? I can see how that would require a global > list of resources. Yes, that is precisely my concern. Sorry for not stating that more clearly. > However I still think it would be easier to encapsulate that completely. > Maybe another level of abstraction is required. You could for example > add another type to encapsulate several power sequences and that could > keep a list of used resources. I can't think of a good name, but maybe > the following DT snippet clarifies what I mean: > > power-sequences { > #address-cells = <1>; > #size-cells = <0>; > > sequence@0 { > name = "up"; > > #address-cells = <1>; > #size-cells = <0>; > > step@0 { > ... > }; > > ... > }; > > sequence@1 { > name = "down"; > > #address-cells = <1>; > #size-cells = <0>; > > step@0 { > ... > }; > > ... > }; > }; > > If you add a name property like this, you could extend the API to > support running a named sequence: > > power_seq_run(seq, "up"); > ... > power_seq_run(seq, "down); Mmm, that's something to consider. Forcing power sequences to be grouped within a "power-sequences" node would also make parsing easier from the driver side since it would not have to explicitly parse every sequence. We could even imagine some tighter integration with the device subsystem to automatically run specifically-named sequences during suspend/resume. But maybe I'm thinking too much here. > >>> Also, is there some way we can make the id property for GPIOs not >>> require the -gpio suffix? If the resource type is already GPIO, then it >>> seems redundant to add -gpio to the ID. >> >> There is unfortunately an inconsistency between the way regulators >> and GPIOs are gotten by name. regulator_get(id) will expect to find >> a property named "id-supply", while gpio_request_one(id) expects a >> property named exactly "id". To workaround this we could sprintf the >> correct property name from a non-suffixed property name within the >> driver, but I think this actually speaks more in favor of having >> phandles directly into the sequences. > > Yes, if it can be made to work by specifying the phandle directly that > is certainly better. Let's do that then - as for the PWM issue I had, let's address that by clearly stating in the documentation that phandles referring to a same device *must* be identical. >>>> + if (!seq) return 0; >>> >>> I don't think this is acceptable according to the coding style. Also, >>> perhaps returning -EINVAL would be more meaningful? >> >> I neglected running checkpatch before submitting, apologies for >> that. The return value seems correct to me, a NULL sequence has no >> effect. > > But seq == NULL should never happen anyway, right? It could if you are parsing a NULL node. It seems safe to me to consider that a NULL sequence is an empty sequence, but if I go for your solution involving another data structure to encapsulate the sequence, then this might change. >>> Perhaps this should check for POWER_SEQ_STOP instead? >> >> There is no resource for POWER_SEQ_STOP - therefore, a NULL resource >> is used instead. > > Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of > a sequence, so intuitively I'd be looking for that as a stop condition. That is for platform data - resolved sequences get their type from their resource, and a STOP sequence does not have a resource. But the STOP type will go away too since we will have a steps count in the platform data instead. >> I would like to do that actually. The issue is that it did not work >> go well with the legacy pwm_backlight behavior: a power sequence >> needs to be constructed out of a PWM obtained through >> pwm_request(int pwm_id, char *label) and this behavior cannot be >> emulated using the new platform data interface (which only works >> with pwm_get()). But if I remove this old behavior, then I could >> make power_seq opaque. I don't think many drivers are using it. What >> do you think? > > I don't see how that is relevant here, since this power-sequencing code > is supposed to be generic and not tied to any specific implementation. > Can you explain further? > > In any case you shouldn't be using pwm_request() in new code. Power sequences only rely on pwm_get, and never call pwm_request since it is, as you stated, deprecated. However there are still boards that use the old pwm_id member of the pwm_backlight_platform_data. For these, we must call pwm_request from the pwm_backlight driver in order to resolve the PWM (see pwm_backlight_legacy_probe of the seconds patch). As the PWM is being resolved by the backlight driver, and not within the power sequences parser, the resolved data structure must be visible to pwm_backlight so it can construct it. There are two ways to solve this and keep the power sequences structure private: 1) Add a way to resolve a PWM by id using pwm_request in the power sequences (we probably should not do that) 2) Port the old platform pwm_request code to use pwm_add_table and pwm_get instead. Do I get points if I do 2)? :) Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 01, 2012 at 11:50:35AM +0900, Alex Courbot wrote: > On 07/31/2012 07:19 PM, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote: > >>I would like to do that actually. The issue is that it did not work > >>go well with the legacy pwm_backlight behavior: a power sequence > >>needs to be constructed out of a PWM obtained through > >>pwm_request(int pwm_id, char *label) and this behavior cannot be > >>emulated using the new platform data interface (which only works > >>with pwm_get()). But if I remove this old behavior, then I could > >>make power_seq opaque. I don't think many drivers are using it. What > >>do you think? > > > >I don't see how that is relevant here, since this power-sequencing code > >is supposed to be generic and not tied to any specific implementation. > >Can you explain further? > > > >In any case you shouldn't be using pwm_request() in new code. > > Power sequences only rely on pwm_get, and never call pwm_request > since it is, as you stated, deprecated. However there are still > boards that use the old pwm_id member of the > pwm_backlight_platform_data. For these, we must call pwm_request > from the pwm_backlight driver in order to resolve the PWM (see > pwm_backlight_legacy_probe of the seconds patch). As the PWM is > being resolved by the backlight driver, and not within the power > sequences parser, the resolved data structure must be visible to > pwm_backlight so it can construct it. There are two ways to solve > this and keep the power sequences structure private: > > 1) Add a way to resolve a PWM by id using pwm_request in the power > sequences (we probably should not do that) No. But I think we don't need that either. If you use power sequences, then you shouldn't be using the pwm_id member. Both methods should be exclusive. > 2) Port the old platform pwm_request code to use pwm_add_table and > pwm_get instead. In the long term, every PWM user should move to pwm_add_table() and pwm_get(), but there are quite a lot of boards that need conversion. > Do I get points if I do 2)? :) Yes. =) I have a sample patch for PXA EZX, but as I said, many more boards need conversion. Thierry
On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote: > On Tue, Jul 31, 2012 at 04:32:35PM +0200, Thierry Reding wrote: > > On Tue, Jul 31, 2012 at 03:26:07PM +0100, Mark Brown wrote: > > > > This is framework code - it doesn't have much option. Disabling HOTPLUG > > > is totally reasonable on space constrained systems, there's no reason > > > for the code to break things for people. > > > Still if you use this code and disable HOTPLUG, then you shouldn't be > > using modules either. I mean there is no way you can write a driver that > > Of course. > > > can gracefully handle its platform data being discarded. > > Sure there is - take a copy of the platform data in probe(). Yes, but that will only work for built-in drivers. If you unload the module and that causes the platform data to be discarded, reloading won't work. Thierry
On Wed, Aug 01, 2012 at 09:41:13AM +0200, Thierry Reding wrote: > On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote: > > Sure there is - take a copy of the platform data in probe(). > Yes, but that will only work for built-in drivers. If you unload the > module and that causes the platform data to be discarded, reloading > won't work. This is why __devinit data will only be discarded when this is not possible. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote: > On Wed, Aug 01, 2012 at 09:41:13AM +0200, Thierry Reding wrote: > > On Tue, Jul 31, 2012 at 04:39:41PM +0100, Mark Brown wrote: > > > > Sure there is - take a copy of the platform data in probe(). > > > Yes, but that will only work for built-in drivers. If you unload the > > module and that causes the platform data to be discarded, reloading > > won't work. > > This is why __devinit data will only be discarded when this is not > possible. That's exactly my point. But I seem to have miserably failed to get that across. =) Thierry
On Wed, Aug 01, 2012 at 03:38:14PM +0200, Thierry Reding wrote: > On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote: > > This is why __devinit data will only be discarded when this is not > > possible. > That's exactly my point. But I seem to have miserably failed to get that > across. =) We must be talking at cross purposes then. What I'm saying is that the framework shouldn't rely on platform data and should assume that the kernel might be configured so it can be discarded (by copying most likely). -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 01, 2012 at 02:55:31PM +0100, Mark Brown wrote: > On Wed, Aug 01, 2012 at 03:38:14PM +0200, Thierry Reding wrote: > > On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote: > > > > This is why __devinit data will only be discarded when this is not > > > possible. > > > That's exactly my point. But I seem to have miserably failed to get that > > across. =) > > We must be talking at cross purposes then. What I'm saying is that the > framework shouldn't rely on platform data and should assume that the > kernel might be configured so it can be discarded (by copying most > likely). Yes. I think this should be solved by the power_seq_build() function which uses the platform data description of the sequence and builds it into something for internal use that doesn't rely on anything in the platform data. Thierry
On 07/31/2012 07:45 AM, Stephen Warren wrote: > Oh I see. That's a little confusing. Why not just reference the relevant > resources directly in each step; something more like: > > gpio@1 { > action = "enable-gpio"; > gpio = <&gpio 1 0>; > }; > > I guess that might make parsing/building a little harder, since you'd > have to detect when you'd already done gpio_request() on a given GPIO > and not repeat it or something like that, but to me this makes the DT a > lot easier to comprehend. I tried to move towards having the phandles directly in the sequences themselves - that reminded me why I did not do that in the first place. Let's say we have a sequence like this (reg property omitted on purpose): power-on-sequence { step@0 { regulator = <&backlight_reg>; enable; }; step@1 { delay = <10000>; }; step@2 { pwm = <&pwm 2 5000000>; enable; }; step@3 { gpio = <&gpio 28 0>; enable; }; }; The problem is, how do we turn these phandles into the resource of interest. The type of the resource can be infered by the name of the property. The hard part is resolving the resource from the phandle - it seems like the API just does not allow to do this. GPIO has of_get_named_gpio, but AFAIK there are no equivalent for regulator consumer and PWM: the only way to use the DT with them is through get_regulator and get_pwm which work at the device level. Or is there a way that I overlooked? Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote: > On 07/31/2012 07:45 AM, Stephen Warren wrote: > >Oh I see. That's a little confusing. Why not just reference the relevant > >resources directly in each step; something more like: > > > > gpio@1 { > > action = "enable-gpio"; > > gpio = <&gpio 1 0>; > > }; > > > >I guess that might make parsing/building a little harder, since you'd > >have to detect when you'd already done gpio_request() on a given GPIO > >and not repeat it or something like that, but to me this makes the DT a > >lot easier to comprehend. > > I tried to move towards having the phandles directly in the > sequences themselves - that reminded me why I did not do that in the > first place. Let's say we have a sequence like this (reg property > omitted on purpose): > > power-on-sequence { > step@0 { > regulator = <&backlight_reg>; > enable; > }; > step@1 { > delay = <10000>; > }; > step@2 { > pwm = <&pwm 2 5000000>; > enable; > }; > step@3 { > gpio = <&gpio 28 0>; > enable; > }; > }; > > The problem is, how do we turn these phandles into the resource of > interest. The type of the resource can be infered by the name of the > property. The hard part is resolving the resource from the phandle - > it seems like the API just does not allow to do this. GPIO has > of_get_named_gpio, but AFAIK there are no equivalent for regulator > consumer and PWM: the only way to use the DT with them is through > get_regulator and get_pwm which work at the device level. > > Or is there a way that I overlooked? No, you are right. Perhaps we should add exported functions that do the equivalent of of_pwm_request() or the regulator_dev_lookup() and of_get_regulator() pair. Thierry
On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote: >> On 07/31/2012 07:45 AM, Stephen Warren wrote: >>> Oh I see. That's a little confusing. Why not just reference the relevant >>> resources directly in each step; something more like: >>> >>> gpio@1 { >>> action = "enable-gpio"; >>> gpio = <&gpio 1 0>; >>> }; >>> >>> I guess that might make parsing/building a little harder, since you'd >>> have to detect when you'd already done gpio_request() on a given GPIO >>> and not repeat it or something like that, but to me this makes the DT a >>> lot easier to comprehend. >> >> I tried to move towards having the phandles directly in the >> sequences themselves - that reminded me why I did not do that in the >> first place. Let's say we have a sequence like this (reg property >> omitted on purpose): >> >> power-on-sequence { >> step@0 { >> regulator = <&backlight_reg>; >> enable; >> }; >> step@1 { >> delay = <10000>; >> }; >> step@2 { >> pwm = <&pwm 2 5000000>; >> enable; >> }; >> step@3 { >> gpio = <&gpio 28 0>; >> enable; >> }; >> }; >> >> The problem is, how do we turn these phandles into the resource of >> interest. The type of the resource can be infered by the name of the >> property. The hard part is resolving the resource from the phandle - >> it seems like the API just does not allow to do this. GPIO has >> of_get_named_gpio, but AFAIK there are no equivalent for regulator >> consumer and PWM: the only way to use the DT with them is through >> get_regulator and get_pwm which work at the device level. >> >> Or is there a way that I overlooked? > > No, you are right. Perhaps we should add exported functions that do the > equivalent of of_pwm_request() or the regulator_dev_lookup() and > of_get_regulator() pair. How would that be looked with respect to "good DT practices"? I can somehow understand the wish to restrain DT access to these functions that integrate well with current workflows. Aren't we going to be frowned upon if we make more low-level functions public? Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote: > On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote: > >>On 07/31/2012 07:45 AM, Stephen Warren wrote: > >>>Oh I see. That's a little confusing. Why not just reference the relevant > >>>resources directly in each step; something more like: > >>> > >>> gpio@1 { > >>> action = "enable-gpio"; > >>> gpio = <&gpio 1 0>; > >>> }; > >>> > >>>I guess that might make parsing/building a little harder, since you'd > >>>have to detect when you'd already done gpio_request() on a given GPIO > >>>and not repeat it or something like that, but to me this makes the DT a > >>>lot easier to comprehend. > >> > >>I tried to move towards having the phandles directly in the > >>sequences themselves - that reminded me why I did not do that in the > >>first place. Let's say we have a sequence like this (reg property > >>omitted on purpose): > >> > >> power-on-sequence { > >> step@0 { > >> regulator = <&backlight_reg>; > >> enable; > >> }; > >> step@1 { > >> delay = <10000>; > >> }; > >> step@2 { > >> pwm = <&pwm 2 5000000>; > >> enable; > >> }; > >> step@3 { > >> gpio = <&gpio 28 0>; > >> enable; > >> }; > >> }; > >> > >>The problem is, how do we turn these phandles into the resource of > >>interest. The type of the resource can be infered by the name of the > >>property. The hard part is resolving the resource from the phandle - > >>it seems like the API just does not allow to do this. GPIO has > >>of_get_named_gpio, but AFAIK there are no equivalent for regulator > >>consumer and PWM: the only way to use the DT with them is through > >>get_regulator and get_pwm which work at the device level. > >> > >>Or is there a way that I overlooked? > > > >No, you are right. Perhaps we should add exported functions that do the > >equivalent of of_pwm_request() or the regulator_dev_lookup() and > >of_get_regulator() pair. > > How would that be looked with respect to "good DT practices"? I can > somehow understand the wish to restrain DT access to these functions > that integrate well with current workflows. Aren't we going to be > frowned upon if we make more low-level functions public? Yes, I think that's exactly what will happen. Thierry
On Thu 02 Aug 2012 05:45:41 PM JST, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote: >> On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote: >>>> Old Signed by an unknown key >>> >>> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote: >>>> On 07/31/2012 07:45 AM, Stephen Warren wrote: >>>>> Oh I see. That's a little confusing. Why not just reference the relevant >>>>> resources directly in each step; something more like: >>>>> >>>>> gpio@1 { >>>>> action = "enable-gpio"; >>>>> gpio = <&gpio 1 0>; >>>>> }; >>>>> >>>>> I guess that might make parsing/building a little harder, since you'd >>>>> have to detect when you'd already done gpio_request() on a given GPIO >>>>> and not repeat it or something like that, but to me this makes the DT a >>>>> lot easier to comprehend. >>>> >>>> I tried to move towards having the phandles directly in the >>>> sequences themselves - that reminded me why I did not do that in the >>>> first place. Let's say we have a sequence like this (reg property >>>> omitted on purpose): >>>> >>>> power-on-sequence { >>>> step@0 { >>>> regulator = <&backlight_reg>; >>>> enable; >>>> }; >>>> step@1 { >>>> delay = <10000>; >>>> }; >>>> step@2 { >>>> pwm = <&pwm 2 5000000>; >>>> enable; >>>> }; >>>> step@3 { >>>> gpio = <&gpio 28 0>; >>>> enable; >>>> }; >>>> }; >>>> >>>> The problem is, how do we turn these phandles into the resource of >>>> interest. The type of the resource can be infered by the name of the >>>> property. The hard part is resolving the resource from the phandle - >>>> it seems like the API just does not allow to do this. GPIO has >>>> of_get_named_gpio, but AFAIK there are no equivalent for regulator >>>> consumer and PWM: the only way to use the DT with them is through >>>> get_regulator and get_pwm which work at the device level. >>>> >>>> Or is there a way that I overlooked? >>> >>> No, you are right. Perhaps we should add exported functions that do the >>> equivalent of of_pwm_request() or the regulator_dev_lookup() and >>> of_get_regulator() pair. >> >> How would that be looked with respect to "good DT practices"? I can >> somehow understand the wish to restrain DT access to these functions >> that integrate well with current workflows. Aren't we going to be >> frowned upon if we make more low-level functions public? > > Yes, I think that's exactly what will happen. Probably not worth to try this then? Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote: > On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote: > > The problem is, how do we turn these phandles into the resource of > > interest. The type of the resource can be infered by the name of the > > property. The hard part is resolving the resource from the phandle - > > it seems like the API just does not allow to do this. GPIO has > > of_get_named_gpio, but AFAIK there are no equivalent for regulator > > consumer and PWM: the only way to use the DT with them is through > > get_regulator and get_pwm which work at the device level. > > Or is there a way that I overlooked? > No, you are right. Perhaps we should add exported functions that do the > equivalent of of_pwm_request() or the regulator_dev_lookup() and > of_get_regulator() pair. I missed some of the earlier bits of the thread here but why can't we do device based lookups? -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote: > On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote: >> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote: > >>> The problem is, how do we turn these phandles into the resource of >>> interest. The type of the resource can be infered by the name of the >>> property. The hard part is resolving the resource from the phandle - >>> it seems like the API just does not allow to do this. GPIO has >>> of_get_named_gpio, but AFAIK there are no equivalent for regulator >>> consumer and PWM: the only way to use the DT with them is through >>> get_regulator and get_pwm which work at the device level. > >>> Or is there a way that I overlooked? > >> No, you are right. Perhaps we should add exported functions that do the >> equivalent of of_pwm_request() or the regulator_dev_lookup() and >> of_get_regulator() pair. > > I missed some of the earlier bits of the thread here but why can't we do > device based lookups? That is because the phandles would not be properties of the device node but rather of its sub-nodes: backlight { compatible = "pwm-backlight"; ... power-on-sequence { step@0 { regulator = <&backlight_reg>; enable; }; So here simply using regulator_get on the backlight device would not work. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote: > On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote: > >I missed some of the earlier bits of the thread here but why can't we do > >device based lookups? > That is because the phandles would not be properties of the device > node but rather of its sub-nodes: > backlight { > compatible = "pwm-backlight"; > ... > power-on-sequence { > step@0 { > regulator = <&backlight_reg>; > enable; > }; > > So here simply using regulator_get on the backlight device would not work. Ah, right. DT isn't being terribly helpful here... I think the thing I'd expect to work here is that you have a reference back to the supply property of the backlight device rather than direct to the regulator so you end up writing "enable supply X" rather than "enable regulator X". Not quite sure how exactly you'd accomplish that - I guess if regulator_get() would recursively follow phandles until it hits a node that'd do the trick? -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/04/2012 11:12 PM, Mark Brown wrote: > On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote: >> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote: > >>> I missed some of the earlier bits of the thread here but why can't we do >>> device based lookups? > >> That is because the phandles would not be properties of the device >> node but rather of its sub-nodes: > >> backlight { >> compatible = "pwm-backlight"; >> ... >> power-on-sequence { >> step@0 { >> regulator = <&backlight_reg>; >> enable; >> }; >> > >> So here simply using regulator_get on the backlight device would not work. > > Ah, right. DT isn't being terribly helpful here... I think the thing > I'd expect to work here is that you have a reference back to the supply > property of the backlight device rather than direct to the regulator so > you end up writing "enable supply X" rather than "enable regulator X". > > Not quite sure how exactly you'd accomplish that - I guess if > regulator_get() would recursively follow phandles until it hits a node > that'd do the trick? Do you mean that regulator_get() would parse sub-nodes looking for a match? That seems rather dangerous and error-prone, especially if one has actual devices within the sub-nodes - their regulators could be stolen by the parent device. I think we only have two choices for this: 1) Stick to the scheme where resources are declared at the device level, such as they can be referenced by name in the sub-nodes (basically what I did in this patch): backlight { compatible = "pwm-backlight"; ... backlight-supply = <&backlight_reg>; power-on-sequence { step@0 { regulator = "backlight"; enable; }; This would translate by a get_regulator(dev, "backlight") in the code which would be properly resolved. 2) Export a lower-level DT API for resolving phandles directly from a property, similar to of_get_named_gpio. We would then have of_get_named_regulator and of_get_named_pwm. If 2) is deemed acceptable, then I think we should go for it as it would provide the most compact and readable DT syntax. Otherwise 1) is still acceptable IMHO, as it should at least make sense to people already familiar with how the DT works. Opinions from DT experts? Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Anton, Sorry for the late reply. I was away and back now. On Mon, Jul 30, 2012 at 4:40 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote: > On Mon, Jul 30, 2012 at 10:51:42AM +0900, Alex Courbot wrote: > [...] >> On the other hand I have just noticed that the apparently unrelated >> Adaptive Voltage Scaling driver just appeared in drivers/power/avs. >> So if Anton and David are ok with this, maybe I could put the power >> sequences code in its own subdirectory within drivers/power. > > Well, currently drivers/power/ is indeed just for power supply class > subsystem and drivers. But if the trend is to gather power management > ("policy") stuff under one directory, i.e. > > drivers/ > power/ > supplies/ <- former "power supply class and drivers" > regulators/ > idle/ > cpuidle/ > cpufreq/ > devfreq/ > avs/ > ... > > That would probably make sense, we could easily see the big picture. > But if we're not going to do this long-term, I would suggest to stick > to just a new directory under drivers (and move drivers/power/avs/ to > drivers/avs). > > Cc'ing some more people... > > Thanks, > > p.s. Jean, why am I the last person who discovers drivers/power/avs/? > Would be nice to Cc me on such patches; by moving AVS under > drivers/power/ you effectively nominated me as its maintainer. :-) Oops, I am really sorry about that ;-( . I wrongly assumed Rafael and Greg KH were the contact persons for drivers/power and so I contacted them before moving the code. Thanks for letting me know. Are you ok with the changes? Let me know if some more changes are needed. Regards, Jean > > -- > Anton Vorontsov > Email: cbouatmailru@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/05/2012 08:27 PM, Alex Courbot wrote: > On 08/04/2012 11:12 PM, Mark Brown wrote: >> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote: >>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote: >> >>>> I missed some of the earlier bits of the thread here but why can't >>>> we do >>>> device based lookups? ... > I think we only have two choices for this: > > 1) Stick to the scheme where resources are declared at the device level, > such as they can be referenced by name in the sub-nodes (basically what > I did in this patch): > > backlight { > compatible = "pwm-backlight"; > ... > backlight-supply = <&backlight_reg>; > > power-on-sequence { > step@0 { > regulator = "backlight"; > enable; > }; > > This would translate by a get_regulator(dev, "backlight") in the code > which would be properly resolved. Yes, upon reflection, that scheme does make sense. I withdraw the comments I made re: whether it'd be better to just stick the phandles into the steps. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/07/2012 01:16 AM, Stephen Warren wrote: > On 08/05/2012 08:27 PM, Alex Courbot wrote: >> On 08/04/2012 11:12 PM, Mark Brown wrote: >>> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote: >>>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote: >>> >>>>> I missed some of the earlier bits of the thread here but why can't >>>>> we do >>>>> device based lookups? > ... >> I think we only have two choices for this: >> >> 1) Stick to the scheme where resources are declared at the device level, >> such as they can be referenced by name in the sub-nodes (basically what >> I did in this patch): >> >> backlight { >> compatible = "pwm-backlight"; >> ... >> backlight-supply = <&backlight_reg>; >> >> power-on-sequence { >> step@0 { >> regulator = "backlight"; >> enable; >> }; >> >> This would translate by a get_regulator(dev, "backlight") in the code >> which would be properly resolved. > > Yes, upon reflection, that scheme does make sense. I withdraw the > comments I made re: whether it'd be better to just stick the phandles > into the steps. Right - having the phandles directly in the sequences has its merits, but logically speaking resources are related to a device, so this declarative approach is probably closer to reality anyway. I will revise the patch according to all the feedback received and submit a new version soon. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" 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/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt new file mode 100644 index 0000000..aa2ceb5 --- /dev/null +++ b/Documentation/power/power_seq.txt @@ -0,0 +1,120 @@ +Runtime Interpreted Power Sequences +----------------------------------- + +Problem +------- +One very common board-dependent code is the out-of-driver code that is used to +turn a device on or off. For instance, SoC boards very commonly use a GPIO +(abstracted to a regulator or not) to control the power supply of a backlight, +disabling it when the backlight is not used in order to save power. The GPIO +that should be used, however, as well as the exact power sequence that may +involve different resources, is board-dependent and thus unknown of the driver. + +This has been addressed so far by using hooks in the device's platform data that +are called whenever the state of the device might reflect a power change. This +approach, however, introduces board-dependant code into the kernel and is not +compatible with the device tree. + +The Runtime Interpreted Power Sequences (or power sequences for short) aim at +turning this code into platform data or device tree nodes. Power sequences are +described using a simple format and run by a simple interpreter whenever needed. +This allows to remove the callback mechanism and makes the kernel less +board-dependant. + +Sequences Format +---------------- +Power sequences are a series of sequential steps during which an action is +performed on a resource. The supported resources so far are: +- GPIOs +- Regulators +- PWMs + +Each step designates a resource and the following parameters: +- Whether the step should enable or disable the resource, +- Delay to wait before performing the action, +- Delay to wait after performing the action. + +Both new resources and parameters can be introduced, but the goal is of course +to keep things as simple and compact as possible. + +The platform data is a simple array of platform_power_seq_step instances, each +instance describing a step. The type as well as one of id or gpio members +(depending on the type) must be specified. The last step must be of type +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are +identified by number. For example, the following sequence will turn on the +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1: + +struct platform_power_seq_step power_on_seq[] = { + { + .type = POWER_SEQ_REGULATOR, + .id = "power", + .params = { + .enable = 1, + .post_delay = 10, + }, + }, + { + .type = POWER_SEQ_GPIO, + .gpio = 110, + .params = { + .enable = 1, + }, + }, + { + .type = POWER_SEQ_STOP, + }, +}; + +Usage by Drivers and Resources Management +----------------------------------------- +Power sequences make use of resources that must be properly allocated and +managed. The power_seq_build() function takes care of resolving the resources as +they are met in the sequence and to allocate them if needed: + +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, + platform_power_seq *pseq); + +You will need an instance of power_seq_resources to keep track of the resources +that are already allocated. On success, the function returns a devm allocated +resolved sequence that is ready to be passed to power_seq_run(). In case of +failure, and error code is returned. + +A resolved power sequence returned by power_seq_build can be run by +power_run_run(): + +int power_seq_run(struct device *dev, power_seq *seq); + +It returns 0 if the sequence has successfully been run, or an error code if a +problem occured. + +Finally, some resources that cannot be allocated through devm need to be freed +manually. Therefore, be sure to call power_seq_free_resources() in your device +remove function: + +void power_seq_free_resources(power_seq_resources *ress); + +Device tree +----------- +All the same, power sequences can be encoded as device tree nodes. The following +properties and nodes are equivalent to the platform data defined previously: + + power-supply = <&mydevice_reg>; + enable-gpio = <&gpio 6 0>; + + power-on-sequence { + regulator@0 { + id = "power"; + enable; + post-delay = <10>; + }; + gpio@1 { + id = "enable-gpio"; + enable; + }; + }; + +Note that first, the phandles of the regulator and gpio used in the sequences +are defined as properties. Then the sequence references them through the id +property of every step. The name of sub-properties defines the type of the step. +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered +sequentially. diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 08b4c52..65bebfe 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -282,4 +282,8 @@ config CMA_AREAS endif +config POWER_SEQ + bool + default n + endmenu diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 5aa2d70..4c498c1 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o ifeq ($(CONFIG_SYSFS),y) obj-$(CONFIG_MODULES) += module.o endif +obj-$(CONFIG_POWER_SEQ) += power_seq.o obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o obj-$(CONFIG_REGMAP) += regmap/ obj-$(CONFIG_SOC_BUS) += soc.o diff --git a/drivers/base/power_seq.c b/drivers/base/power_seq.c new file mode 100644 index 0000000..6ccefa1 --- /dev/null +++ b/drivers/base/power_seq.c @@ -0,0 +1,300 @@ +/* + * power_seq.c - A simple power sequence interpreter for platform devices + * and device tree. + * + * Author: Alexandre Courbot <acourbot@nvidia.com> + * + * Copyright (c) 2012 NVIDIA Corporation. + * + * 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; version 2 of the License. + * + * 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. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + */ + +#include <linux/power_seq.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/device.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/pwm.h> +#include <linux/regulator/consumer.h> +#include <linux/gpio.h> + +#ifdef CONFIG_OF +#include <linux/of.h> +#include <linux/of_gpio.h> +#endif + +static int power_seq_step_run(struct power_seq_step *step) +{ + int err = 0; + + if (step->params.pre_delay) + mdelay(step->params.pre_delay); + + switch (step->resource->type) { +#ifdef CONFIG_REGULATOR + case POWER_SEQ_REGULATOR: + if (step->params.enable) + err = regulator_enable(step->resource->regulator); + else + err = regulator_disable(step->resource->regulator); + break; +#endif +#ifdef CONFIG_PWM + case POWER_SEQ_PWM: + if (step->params.enable) + err = pwm_enable(step->resource->pwm); + else + pwm_disable(step->resource->pwm); + break; +#endif +#ifdef CONFIG_GPIOLIB + case POWER_SEQ_GPIO: + gpio_set_value_cansleep(step->resource->gpio, + step->params.enable); + break; +#endif + /* + * should never happen unless the sequence includes a step which + * type does not have support compiled in + */ + default: + return -EINVAL; + } + + if (err < 0) + return err; + + if (step->params.post_delay) + mdelay(step->params.post_delay); + + return 0; +} + +int power_seq_run(struct device *dev, power_seq *seq) +{ + int err; + + if (!seq) return 0; + + while (seq->resource) { + if ((err = power_seq_step_run(seq++))) { + dev_err(dev, "error %d while running power sequence!\n", + err); + return err; + } + } + + return 0; +} +EXPORT_SYMBOL_GPL(power_seq_run); + +#ifdef CONFIG_OF +static int of_parse_power_seq_step(struct device *dev, struct device_node *node, + struct platform_power_seq_step *step) +{ + if (of_property_read_string(node, "id", &step->id)) { + dev_err(dev, "missing id property!\n"); + return -EINVAL; + } + + if (!strcmp(node->name, "regulator")) { + step->type = POWER_SEQ_REGULATOR; +#ifdef CONFIG_OF_GPIO + } else if (!strcmp(node->name, "gpio")) { + int gpio; + + step->type = POWER_SEQ_GPIO; + gpio = of_get_named_gpio(dev->of_node, step->id, 0); + if (gpio < 0) { + dev_err(dev, "cannot resolve gpio \"%s\"\n", step->id); + return gpio; + } + step->gpio = gpio; +#endif /* CONFIG_OF_GPIO */ + } else if (!strcmp(node->name, "pwm")) { + step->type = POWER_SEQ_PWM; + } else { + dev_err(dev, "invalid power seq step type!\n"); + return -EINVAL; + } + + if (of_find_property(node, "enable", NULL)) { + step->params.enable = 1; + } else if (!of_find_property(node, "disable", NULL)) { + dev_err(dev, "missing enable or disable property!\n"); + return -EINVAL; + } + + of_property_read_u32(node, "pre-delay", &step->params.pre_delay); + of_property_read_u32(node, "post-delay", &step->params.post_delay); + + return 0; +} + +platform_power_seq *of_parse_power_seq(struct device *dev, + struct device_node *node) +{ + struct device_node *child = NULL; + platform_power_seq *ret; + int cpt = 0; + int err; + + if (!node) return NULL; + + while ((child = of_get_next_child(node, child))) + cpt++; + + /* allocate one more step to signal end of sequence */ + ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL); + if (!ret) + return ERR_PTR(-ENOMEM); + + cpt = 0; + while ((child = of_get_next_child(node, child))) { + if ((err = of_parse_power_seq_step(dev, child, &ret[cpt++]))) + return ERR_PTR(err); + } + + return ret; +} +EXPORT_SYMBOL_GPL(of_parse_power_seq); +#endif /* CONFIG_OF */ + +static +struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress, + struct platform_power_seq_step *res) +{ + struct power_seq_resource *step; + + list_for_each_entry(step, ress, list) { + if (step->type != res->type) continue; + switch (res->type) { + case POWER_SEQ_GPIO: + if (step->gpio == res->gpio) + return step; + break; + default: + if (!strcmp(step->id, res->id)) + return step; + break; + } + } + + return NULL; +} + +static int power_seq_allocate_resource(struct device *dev, + struct power_seq_resource *res) +{ + int err; + + switch (res->type) { +#ifdef CONFIG_REGULATOR + case POWER_SEQ_REGULATOR: + res->regulator = devm_regulator_get(dev, res->id); + if (IS_ERR(res->regulator)) { + dev_err(dev, "cannot get regulator \"%s\"\n", res->id); + return PTR_ERR(res->regulator); + } + break; +#endif +#ifdef CONFIG_PWM + case POWER_SEQ_PWM: + res->pwm = pwm_get(dev, res->id); + if (IS_ERR(res->pwm)) { + dev_err(dev, "cannot get pwm \"%s\"\n", res->id); + return PTR_ERR(res->pwm); + } + break; +#endif +#ifdef CONFIG_GPIOLIB + case POWER_SEQ_GPIO: + err = devm_gpio_request_one(dev, res->gpio, GPIOF_OUT_INIT_HIGH, + "backlight_gpio"); + if (err) { + dev_err(dev, "cannot get gpio %d\n", res->gpio); + return err; + } + break; +#endif + default: + dev_err(dev, "invalid resource type %d\n", res->type); + return -EINVAL; + break; + } + + return 0; +} + +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, + platform_power_seq *pseq) +{ + struct power_seq_step *seq = NULL, *ret; + struct power_seq_resource *res; + int cpt, err; + + /* first pass to count the number of steps to allocate */ + for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++); + + if (!cpt) + return seq; + + /* 1 more for the STOP step */ + ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL); + if (!seq) + return ERR_PTR(-ENOMEM); + + for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) { + /* create resource node if not referenced already */ + if (!(res = power_seq_find_resource(ress, pseq))) { + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL); + if (!res) + return ERR_PTR(-ENOMEM); + res->type = pseq->type; + + if (res->type == POWER_SEQ_GPIO) + res->gpio = pseq->gpio; + else + res->id = pseq->id; + + if ((err = power_seq_allocate_resource(dev, res)) < 0) + return ERR_PTR(err); + + list_add(&res->list, ress); + } + seq->resource = res; + memcpy(&seq->params, &pseq->params, sizeof(seq->params)); + } + + return ret; +} +EXPORT_SYMBOL_GPL(power_seq_build); + +void power_seq_free_resources(power_seq_resources *ress) { + struct power_seq_resource *res; + +#ifdef CONFIG_PWM + list_for_each_entry(res, ress, list) { + if (res->type == POWER_SEQ_PWM) + pwm_put(res->pwm); + } +#endif +} +EXPORT_SYMBOL_GPL(power_seq_free_resources); + +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>"); +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h new file mode 100644 index 0000000..da0593a --- /dev/null +++ b/include/linux/power_seq.h @@ -0,0 +1,139 @@ +/* + * power_seq.h + * + * Simple interpreter for defining power sequences as platform data or device + * tree properties. Initially designed for use with backlight drivers. + * + * Power sequences are designed to replace the callbacks typically used in + * board-specific files that implement board-specific power sequences of devices + * such as backlights. A power sequence is an array of resources (which can a + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or + * disable) and optional pre and post step delays. By having them interpreted + * instead of arbitrarily executed, it is possible to describe these in the + * device tree and thus remove board-specific code from the kernel. + * + * Author: Alexandre Courbot <acourbot@nvidia.com> + * + * Copyright (c) 2012 NVIDIA Corporation. + * + * 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; version 2 of the License. + * + * 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. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + */ + +#ifndef __LINUX_POWER_SEQ_H +#define __LINUX_POWER_SEQ_H + +#include <linux/types.h> + +struct device; +struct regulator; +struct pwm_device; +struct device_node; + +/** + * The different kinds of resources that can be controlled during the sequences. + */ +typedef enum { + POWER_SEQ_STOP = 0, + POWER_SEQ_REGULATOR, + POWER_SEQ_PWM, + POWER_SEQ_GPIO, + POWER_SEQ_MAX, +} power_res_type; + +struct power_seq_resource { + power_res_type type; + /* name to resolve for resources with a name (regulator, pwm) */ + const char *id; + /* resolved resource */ + union { + struct regulator *regulator; + struct pwm_device *pwm; + int gpio; + }; + /* used to maintain the list of resources used by the driver */ + struct list_head list; +}; +typedef struct list_head power_seq_resources; + +struct power_step_params { + /* enable the resource if 1, disable if 0 */ + bool enable; + /* delay (in ms) to wait before executing the step */ + int pre_delay; + /* delay (in ms) to wait after executing the step */ + int post_delay; +}; + +/** + * Platform definition of power sequences. A sequence is an array of these, + * terminated by a STOP instance. + */ +struct platform_power_seq_step { + power_res_type type; + union { + /* Used by REGULATOR and PWM types to name the resource */ + const char *id; + /* Used by GPIO */ + int gpio; + }; + struct power_step_params params; +}; +typedef struct platform_power_seq_step platform_power_seq; + +/** + * Power sequence steps resolved against their resource. Built by + * power_seq_build and used to run the sequence. + */ +struct power_seq_step { + struct power_seq_resource *resource; + struct power_step_params params; +}; +typedef struct power_seq_step power_seq; + +#ifdef CONFIG_OF +/** + * Build a platform data sequence from a device tree node. Memory for the + * sequence is allocated using devm_kzalloc on dev. + */ +platform_power_seq *of_parse_power_seq(struct device *dev, + struct device_node *node); +#else +platform_power_seq *of_parse_power_seq(struct device *dev, + struct device_node *node) +{ + return NULL; +} +#endif + +/** + * Build a runnable power sequence from platform data, and add the resources + * it uses into ress. Memory for the sequence is allocated using devm_kzalloc + * on dev. + */ +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress, + platform_power_seq *pseq); + +/** + * Free all the resources previously allocated by power_seq_allocate_resources. + */ +void power_seq_free_resources(power_seq_resources *ress); + +/** + * Run the given power sequence. Returns 0 on success, error code in case of + * failure. + */ +int power_seq_run(struct device *dev, power_seq *seq); + +#endif
Some device drivers (panel backlights especially) need to follow precise sequences for powering on and off, involving gpios, regulators, PWMs with a precise powering order and delays to respect between each steps. These sequences are board-specific, and do not belong to a particular driver - therefore they have been performed by board-specific hook functions to far. With the advent of the device tree and of ARM kernels that are not board-tied, we cannot rely on these board-specific hooks anymore but need a way to implement these sequences in a portable manner. This patch introduces a simple interpreter that can execute such power sequences encoded either as platform data or within the device tree. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- Documentation/power/power_seq.txt | 120 +++++++++++++++ drivers/base/Kconfig | 4 + drivers/base/Makefile | 1 + drivers/base/power_seq.c | 300 ++++++++++++++++++++++++++++++++++++++ include/linux/power_seq.h | 139 ++++++++++++++++++ 5 files changed, 564 insertions(+) create mode 100644 Documentation/power/power_seq.txt create mode 100644 drivers/base/power_seq.c create mode 100644 include/linux/power_seq.h