| Message ID | 1353149747-31871-2-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Nov 17, 2012 at 07:55:45PM +0900, Alexandre Courbot wrote: > Some device drivers (e.g. panel or backlights) need to follow precise > sequences for powering on and off, involving GPIOs, regulators, PWMs > and other power-related resources, with a defined powering order and > delays to respect between steps. These sequences are device-specific, > and do not belong to a particular driver - therefore they have been > performed by board-specific hook functions so 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. It also > introduces first support for regulator, GPIO and PWM resources. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > Reviewed-by: Stephen Warren <swarren@wwwdotorg.org> > Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > Tested-by: Thierry Reding <thierry.reding@avionic-design.de> > Tested-by: Stephen Warren <swarren@nvidia.com> > Acked-by: Thierry Reding <thierry.reding@avionic-design.de> > --- This looks almost perfect! Just a few final notes, again mostly about the build system bits. [...] > diff --git a/drivers/power/power_seq/Kconfig b/drivers/power/power_seq/Kconfig > new file mode 100644 > index 0000000..0ece819 > --- /dev/null > +++ b/drivers/power/power_seq/Kconfig > @@ -0,0 +1,2 @@ > +config POWER_SEQ > + tristate This really needs a proper Kconfig description and a help text, shortly describing the purpose of the subsystem. > diff --git a/drivers/power/power_seq/Makefile b/drivers/power/power_seq/Makefile > new file mode 100644 > index 0000000..964b91e > --- /dev/null > +++ b/drivers/power/power_seq/Makefile > @@ -0,0 +1,2 @@ > +obj-$(CONFIG_POWER_SEQ) += power_seq.o > +power_seq-y := core.o delay.o regulator.o gpio.o pwm.o > diff --git a/drivers/power/power_seq/core.c b/drivers/power/power_seq/core.c > new file mode 100644 > index 0000000..d51b9b8 > --- /dev/null > +++ b/drivers/power/power_seq/core.c > @@ -0,0 +1,362 @@ > +/* > + * core.c - power sequence interpreter for platform devices and device tree We usually don't write file names in the comments. > + * > + * Author: Alexandre Courbot <acourbot@nvidia.com> > + * [...] > + return 0; > +} > +EXPORT_SYMBOL_GPL(power_seq_run); > + > +/* defined in power_seq_*.c files */ Why not place the decls into the _priv.h file?.. I understand that this might be a temporary stuff until we have something better for ops registration, but still, I believe it belongs to the header file. > +extern const struct power_seq_res_ops power_seq_delay_ops; > +extern const struct power_seq_res_ops power_seq_gpio_ops; > +extern const struct power_seq_res_ops power_seq_regulator_ops; > +extern const struct power_seq_res_ops power_seq_pwm_ops; > + > +static const struct power_seq_res_ops *power_seq_ops[POWER_SEQ_NUM_TYPES] = { > + [POWER_SEQ_DELAY] = &power_seq_delay_ops, > + [POWER_SEQ_REGULATOR] = &power_seq_regulator_ops, > + [POWER_SEQ_PWM] = &power_seq_gpio_ops, > + [POWER_SEQ_GPIO] = &power_seq_pwm_ops, > +}; > + > +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>"); > +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/power/power_seq/delay.c b/drivers/power/power_seq/delay.c > new file mode 100644 > index 0000000..de6810b > --- /dev/null > +++ b/drivers/power/power_seq/delay.c > @@ -0,0 +1,66 @@ > +/* > + * 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. > + * > + */ > + > +#include "power_seq_priv.h" > +#include <linux/delay.h> Things should not depend on _priv.h's includes. I.e. I see this file uses of_ routines, so it should include <linux/of.h>. <linux/delay.h> for udelay_range(), etc. Also, usually we place "private" headers at the very end, not at the beginning. > + > +#ifdef CONFIG_OF > +static int of_power_seq_parse_delay(struct device_node *node, > + struct power_seq *seq, > + unsigned int step_nbr, > + struct power_seq_resource *res) > +{ > + struct power_seq_step *step = &seq->steps[step_nbr]; > + int err; > + > + err = of_property_read_u32(node, "delay", > + &step->delay.delay); > + if (err < 0) > + power_seq_err(seq, step_nbr, "error reading delay property\n"); > + > + return err; > +} > +#else > +#define of_power_seq_parse_delay NULL > +#endif [...] > +#define power_seq_err(seq, step_nbr, format, ...) \ > + dev_err(seq->set->dev, "%s[%d]: " format, seq->id, step_nbr, \ > + ##__VA_ARGS__); > + > +#ifdef CONFIG_OF > +int of_power_seq_parse_enable_properties(struct device_node *node, > + struct power_seq *seq, > + unsigned int step_nbr, bool *enable); Um, I believe you don't need CONFIG_OF here. (If it's about 'struct device_node', then as I see it in of.h, the header always declares it, even for the !OF case.) > +#endif > + > +/** [...] > +++ b/drivers/power/power_seq/regulator.c > @@ -0,0 +1,87 @@ > +/* > + * 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. > + * > + */ > + > +#include "power_seq_priv.h" > + > +#ifdef CONFIG_REGULATOR Would be really great if you could get rid of the #ifdefs in the .c files. To do so, in the makefile you could write something like this: power_seq-$(CONFIG_REGULATOR) += regulator.o And in the header file (as explained above), you'd have #ifdef CONFIG_REGULATOR #define ...REGULATOR_OPS &power_seq_regulator_ops #else #define ...REGULATOR_OPS NULL #endif Or something along these lines... Thanks, Anton.
On Saturday 17 November 2012 19:38:50 Anton Vorontsov wrote: > > +++ b/drivers/power/power_seq/Kconfig > > @@ -0,0 +1,2 @@ > > +config POWER_SEQ > > + tristate > > This really needs a proper Kconfig description and a help text, shortly > describing the purpose of the subsystem. Does it? The current state makes power seqs automatically selected by drivers that need it, and they do not appear in the configuration menu. If we add a description they will then become a visible item, so the logic would be to make it user-selectable. If that approach is preferred, I will also have to change pwm-backlight so that it compiles without power sequences. Not that I mind, but I liked the idea of not adding yet-another-option to the config menu. Fixed the code according to all your other comments, thanks! Alex.
On Mon, Nov 19, 2012 at 11:29:05AM +0900, Alex Courbot wrote: > On Saturday 17 November 2012 19:38:50 Anton Vorontsov wrote: > > > +++ b/drivers/power/power_seq/Kconfig > > > @@ -0,0 +1,2 @@ > > > +config POWER_SEQ > > > + tristate > > > > This really needs a proper Kconfig description and a help text, shortly > > describing the purpose of the subsystem. > > Does it? The current state makes power seqs automatically selected by drivers > that need it, and they do not appear in the configuration menu. If we add a > description they will then become a visible item, so the logic would be to > make it user-selectable. Ah. OK, then yes, there is no need for the short description, but the help text would a good idea anyway. Thanks, Anton.
Hi, On 2012-11-17 12:55, Alexandre Courbot wrote: A few questions after looking at the documentation: > +Example > +------- > +Here are example sequences declared within a backlight device that use all the > +supported resources types: > + > + backlight { > + compatible = "pwm-backlight"; > + ... > + > + /* resources used by the power sequences */ > + pwms = <&pwm 2 5000000>; > + pwm-names = "backlight"; > + power-supply = <&backlight_reg>; > + > + power-sequences { > + power-on { > + step0 { > + type = "regulator"; > + id = "power"; > + enable; > + }; > + step1 { > + type = "delay"; > + delay = <10000>; > + }; > + step2 { > + type = "pwm"; > + id = "backlight"; > + enable; > + }; > + step3 { > + type = "gpio"; > + gpio = <&gpio 28 0>; > + value = <1>; > + }; > + }; I guess there's a reason, but the above looks a bit inconsistent. For gpio you define the gpio resource inside the step. For power and pwm the resource is defined before the steps. Why wouldn't "pwm = <&pwm 2 5000000>;" work in step2? > +When a power sequence is run, its steps is executed one after the other until > +one step fails or the end of the sequence is reached. The document doesn't give any hint of what the driver should do if running the power sequence fails. Run the "opposite" power sequence? Will that work for all resources? I'm mainly thinking of a case where each enable of the resource should be matched by a disable, i.e. you can't call disable if no enable was called. Tomi
On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote: > Some device drivers (e.g. panel or backlights) need to follow precise > sequences for powering on and off, involving GPIOs, regulators, PWMs > and other power-related resources, with a defined powering order and > delays to respect between steps. These sequences are device-specific, > and do not belong to a particular driver - therefore they have been > performed by board-specific hook functions so far. I must be honest, this series really makes me nervous... > 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 This isn't strictly true. It is still perfectly fine to have board specific code when necessary. However, there is strong encouragement to enable that code in device drivers as much as possible and new board files need to have very strong justification. > 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. It also > introduces first support for regulator, GPIO and PWM resources. This is where I start getting nervous. Up to now I've strongly resisted adding any kind of interpreted code to the device tree. The model is to identify hardware, but require the driver to know how to control it. (as compared to ACPI which is entirely designed around executable bytecode). While the power sequences described here certainly cannot be confused with a Turing complete bytecode, it is a step in that direction. Plus, there will always be that new use case that needs just a "little new feature" to make this work for that too. Without thinking about how to handle that ahead of time is just asking for something to turn into a maintenance nightmare. It's just as important to specify what the limits of this approach are and when to punt to real driver code to handle a device. > +Power Sequences Steps > +--------------------- > +Steps of a sequence describe an action to be performed on a resource. They > +always include a "type" property which indicates what kind of resource this > +step works on. Depending on the resource type, additional properties are defined > +to control the action to be performed. > + > +"delay" type required properties: > + - delay: delay to wait (in microseconds) > + > +"regulator" type required properties: > + - id: name of the regulator to use. > + - enable / disable: one of these two empty properties must be present to > + enable or disable the resource > + > +"pwm" type required properties: > + - id: name of the PWM to use. > + - enable / disable: one of these two empty properties must be present to > + enable or disable the resource > + > +"gpio" type required properties: > + - gpio: phandle of the GPIO to use. > + - value: value this GPIO should take. Must be 0 or 1. > + > +Example > +------- > +Here are example sequences declared within a backlight device that use all the > +supported resources types: > + > + backlight { > + compatible = "pwm-backlight"; > + ... > + > + /* resources used by the power sequences */ > + pwms = <&pwm 2 5000000>; > + pwm-names = "backlight"; > + power-supply = <&backlight_reg>; > + > + power-sequences { > + power-on { > + step0 { > + type = "regulator"; > + id = "power"; > + enable; > + }; > + step1 { > + type = "delay"; > + delay = <10000>; > + }; > + step2 { > + type = "pwm"; > + id = "backlight"; > + enable; > + }; > + step3 { > + type = "gpio"; > + gpio = <&gpio 28 0>; > + value = <1>; > + }; > + }; > + > + power-off { > + step0 { > + type = "gpio"; > + gpio = <&gpio 28 0>; > + value = <0>; > + }; > + step1 { > + type = "pwm"; > + id = "backlight"; > + disable; > + }; > + step2 { > + type = "delay"; > + delay = <10000>; > + }; > + step3 { > + type = "regulator"; > + id = "power"; > + disable; > + }; > + }; > + }; > + }; I think this will get very verbose in a hurry. Already this simple example is 45 lines long. Using the device tree structure to encode the language doesn't look like a very good fit. Not to mention that the order of operations is entirely based on the node name. Want to insert an operation between step0 and step1? Need to rename step1, step2, and step3 to do so. This implementation also isn't very consistent. The gpio is referenced with a phandle in step3/step0, but the regulator and pwm are referenced by id. As an alternative, what about something like the following? backlight { compatible = "pwm-backlight"; ... /* resources used by the power sequences */ pwms = <&pwm 2 5000000>; pwm-names = "backlight"; regulators = <&backlight_reg>; gpios = <&gpio 28 0>; power-on-sequence = "r0e;d10000m;p0e;g0s"; power-off-sequence = "g0c;p0d;d10000m;r0d"; }; I'm thinking about the scripts as trivial-to-parse ascii strings that have a very simple set of commands. The commands use resources already defined in the node. ie. 'g0' refers to the first entry in the gpios property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By no means take this as the format to use, it is just an example off the top of my head, but it is already way easier to work with than putting each command into a node. The trick is still to define a syntax that doesn't fall apart when it needs to be extended. I would also like to get opinions on whether or not conditionals or loops should be supported (ie. loop waiting for a status to change). If they should then we need to be a lot more careful about the design (and due to my aforementioned nervousness, somebody may need to get me therapy). (Mitch, I'll let you make the argument for using Forth here. To be honest, I'm not keen on defining any kind of new language, however simple, but neither am I keen to pull in Forth). > +Platform Data Format > +-------------------- > +All relevant data structures for declaring power sequences are located in > +include/linux/power_seq.h. Hmm... If sequences are switched to a string instead, then platform_data should also use it. The power sequence data structures can be created at runtime by parsing the string. g.
On Tue, Nov 20, 2012 at 09:54:29PM +0000, Grant Likely wrote: > On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote: > > 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 > This isn't strictly true. It is still perfectly fine to have board > specific code when necessary. However, there is strong encouragement to > enable that code in device drivers as much as possible and new board > files need to have very strong justification. This isn't the message that's gone over, and even for device drivers everyone seems to be taking the whole device tree thing as a move to pull all data out of the kernel. In some cases there are some real practical advantages to doing this but a lot of the people making these changes seem to view having things in DT as a goal in itself. > I'm thinking about the scripts as trivial-to-parse ascii strings that > have a very simple set of commands. The commands use resources already > defined in the node. ie. 'g0' refers to the first entry in the gpios > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By > no means take this as the format to use, it is just an example off the > top of my head, but it is already way easier to work with than putting > each command into a node. It does appear to have some legibility challenges, especially with using the indexes into the arrays of resources. On the other hand the arrays should be fairly small. > > +Platform Data Format > > +-------------------- > > +All relevant data structures for declaring power sequences are located in > > +include/linux/power_seq.h. > Hmm... If sequences are switched to a string instead, then platform_data > should also use it. The power sequence data structures can be created at > runtime by parsing the string. Seems like a step backwards to me - why not let people store the more parsed version of the data? It's going to be less convenient for users on non-DT systems. As I said in my earlier reviews I think this is a useful thing to have as a utility library for drivers independantly of the DT bindings, it would allow drivers to become more data driven. Perhaps we can rework the series so that the DT bindings are added as a final patch? All the rest of the code seems OK.
Hi Tomi, On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote: > I guess there's a reason, but the above looks a bit inconsistent. For > gpio you define the gpio resource inside the step. For power and pwm the > resource is defined before the steps. Why wouldn't "pwm = <&pwm 2 > 5000000>;" work in step2? That's mostly a framework issue. Most frameworks do not export a function that allow to dereference a phandle - they expect resources to be declared right under the device node and accessed by name through foo_get(device, name). So using phandles in power sequences would require to export these additional functions and also opens the door to some inconsistencies - for instance, your PWM phandle could be referenced a second time in the sequence with a different period - how do you know that these are actually referring the same PWM device? > > +When a power sequence is run, its steps is executed one after the other > > until +one step fails or the end of the sequence is reached. > > The document doesn't give any hint of what the driver should do if > running the power sequence fails. Run the "opposite" power sequence? > Will that work for all resources? I'm mainly thinking of a case where > each enable of the resource should be matched by a disable, i.e. you > can't call disable if no enable was called. We discussed that issue already (around v5 I think) and the conclusion was that it should be up to the driver. When we simply enable/disable resources it is easy to revert, but in the future non-boolean properties will likely be introduced, and these cannot easily be reverted. Moreover some drivers might have more complex recovery needs. This deserves more discussion I think, as I'd like to have some "generic" recovery mechanism that covers most of the cases. Alex.
Hi Grant, On Wednesday 21 November 2012 05:54:29 Grant Likely wrote: > > 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 > > This isn't strictly true. It is still perfectly fine to have board > specific code when necessary. However, there is strong encouragement to > enable that code in device drivers as much as possible and new board > files need to have very strong justification. But doesn't introducing board-specific code into the kernel just defeats the purpose of the DT? If we extend this logic, we are heading straight back to board-definition files. To a lesser extent than before I agree, but the problem is fundamentally the same. > > 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. It also > > introduces first support for regulator, GPIO and PWM resources. > > This is where I start getting nervous. Up to now I've strongly resisted > adding any kind of interpreted code to the device tree. The model is to > identify hardware, but require the driver to know how to control it. (as > compared to ACPI which is entirely designed around executable > bytecode). > > While the power sequences described here certainly cannot be confused > with a Turing complete bytecode, it is a step in that direction. Technically speaking power sequences are a step towards an interpreter, but it is a very small one and it should not go much further than the current state. I understand the concern of having "code" into the DT but I really think it should be viewed from a different angle. Powering sequences are special in that they can be affected by the board design or the devices variations. For instance hundreds of different panels with backlights are currently compatible with the pwm-backlight driver. The only thing that differenciates them is how the backlight is powered on and off. If you are to build a kernel that is supposed to support all these panels, you would need to embed all the powering sequences in the kernel even though only one of them will be used by one specific board. Power sequences in the DT help preventing that. With that stated, it is clear that we should not need to define more than the short, simple sequences of actions that cannot be elegantly handled by the driver. Anything beyond that should be handled by the driver itself. In particular, here are a few things I do *not* want to see included in power seqs: - conditionals/jumps (or it's not a sequence anymore). - direct access to hardware. Resources must at least be abstracted in some way. You shall not e.g. access the address space directly. - support for non-power related resources - that is out of the special case of powering sequences and should be done by the driver That should keep the "grammar" simple, and the sequences short enough to that we can consider then as data belonging to the device, and not as code that is interpreted. > I think this will get very verbose in a hurry. Already this simple > example is 45 lines long. Using the device tree structure to encode the > language doesn't look like a very good fit. Not to mention that the > order of operations is entirely based on the node name. Want to insert > an operation between step0 and step1? Need to rename step1, step2, and > step3 to do so. I don't like that steps numbering thing neither, but it seems to be the best way to do it so far. As for the DT structure not being adapted for this - I would agree if we wanted to implement a complete interpreter, but that's precisely not the case. More about this later. > This implementation also isn't very consistent. The gpio is referenced > with a phandle in step3/step0, but the regulator and pwm are referenced > by id. Tomi made the same remark - the reason for using the phandle in GPIO is because GPIO framework does not support referencing GPIOs by name yet. I wanted to DT bindings to reflect the underlying framework as much as possible until we have a function like gpio_get(device, id). However I agree that this makes things inconsistent at the moment and would require a bindings change. And in the case of the DT this is actually easy to implement (I did it in some previous versions). I'll make sure to do it. > As an alternative, what about something like the following? > > backlight { > compatible = "pwm-backlight"; > ... > > /* resources used by the power sequences */ > pwms = <&pwm 2 5000000>; > pwm-names = "backlight"; > regulators = <&backlight_reg>; > gpios = <&gpio 28 0>; > > power-on-sequence = "r0e;d10000m;p0e;g0s"; > power-off-sequence = "g0c;p0d;d10000m;r0d"; > }; Well, *now* it really looks like bytecode. :) > I'm thinking about the scripts as trivial-to-parse ascii strings that > have a very simple set of commands. The commands use resources already > defined in the node. ie. 'g0' refers to the first entry in the gpios > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By > no means take this as the format to use, it is just an example off the > top of my head, but it is already way easier to work with than putting > each command into a node. I'd argue that this opens the door wide open towards having a complete interpreter in the device tree. At least DT nodes restrict what we can do conveniently. > The trick is still to define a syntax that doesn't fall apart when it > needs to be extended. I would also like to get opinions on whether or > not conditionals or loops should be supported (ie. loop waiting for a > status to change). If they should then we need to be a lot more careful > about the design (and due to my aforementioned nervousness, somebody may > need to get me therapy). That's IMHO the main argument in favor of using DT nodes here (the syntax extension, not your therapy). They can be extended simply by adding properties to them, and I was relying on this to make power seqs extensible while keeping things consistent (and backward-compatible). For instance, when we want to support voltage setting in regulators we can just add a "voltage" property for that. So to sum up the pros of the current node-base representation: - give a "data like" representation of powering sequences (what they should be, actually) - puts sanity barriers for not turning power seqs into a real code interpreter - easily extensible There are probably a few cons, the numbering of steps being one, but at least we don't need to design a new DSL and care too much about extensibility which is, in the nodes case, built-in and free. I also like to make it clear that I don't want to see this going out of control and that any extension proposal would have to be thoroughly justified and reviewed - and better not contradict any of the 3 points listed above. If that makes you feel better, maybe we can try and fix what is wrong with the current bindings instead of introducing a new language that will be, by nature, more complex to handle and difficult to extend without breaking things? Alex.
On 2012-11-21 03:56, Alex Courbot wrote: > Hi Tomi, > > On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote: >> I guess there's a reason, but the above looks a bit inconsistent. For >> gpio you define the gpio resource inside the step. For power and pwm the >> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2 >> 5000000>;" work in step2? > > That's mostly a framework issue. Most frameworks do not export a function that > allow to dereference a phandle - they expect resources to be declared right > under the device node and accessed by name through foo_get(device, name). So > using phandles in power sequences would require to export these additional Right, I expected something like that. > functions and also opens the door to some inconsistencies - for instance, your > PWM phandle could be referenced a second time in the sequence with a different > period - how do you know that these are actually referring the same PWM > device? This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single device, no matter where and how many times it's used? >>> +When a power sequence is run, its steps is executed one after the other >>> until +one step fails or the end of the sequence is reached. >> >> The document doesn't give any hint of what the driver should do if >> running the power sequence fails. Run the "opposite" power sequence? >> Will that work for all resources? I'm mainly thinking of a case where >> each enable of the resource should be matched by a disable, i.e. you >> can't call disable if no enable was called. > > We discussed that issue already (around v5 I think) and the conclusion was > that it should be up to the driver. When we simply enable/disable resources it > is easy to revert, but in the future non-boolean properties will likely be > introduced, and these cannot easily be reverted. Moreover some drivers might > have more complex recovery needs. This deserves more discussion I think, as > I'd like to have some "generic" recovery mechanism that covers most of the > cases. Ok. I'll need to dig up the conversation. Did you consider any examples of how some driver could handle the error cases? What I'm worried about is that, as far as I understand, the power sequence is kinda like black box to the driver. The driver just does "power-up", without knowing what really goes on in there. And if it doesn't know what goes on in there, nor what's in "power-down" sequence, how can it do anything when an error happens? The only option I see is that the driver doesn't do anything, which will leave some resources enabled, or it can run the power-down sequence, which may or may not work. Tomi
On Wednesday 21 November 2012 16:13:47 Tomi Valkeinen wrote: > * PGP Signed by an unknown key > > On 2012-11-21 03:56, Alex Courbot wrote: > > Hi Tomi, > > > > On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote: > >> I guess there's a reason, but the above looks a bit inconsistent. For > >> gpio you define the gpio resource inside the step. For power and pwm the > >> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2 > >> 5000000>;" work in step2? > > > > That's mostly a framework issue. Most frameworks do not export a function > > that allow to dereference a phandle - they expect resources to be > > declared right under the device node and accessed by name through > > foo_get(device, name). So using phandles in power sequences would require > > to export these additional > Right, I expected something like that. > > > functions and also opens the door to some inconsistencies - for instance, > > your PWM phandle could be referenced a second time in the sequence with a > > different period - how do you know that these are actually referring the > > same PWM device? > > This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single > device, no matter where and how many times it's used? > > >>> +When a power sequence is run, its steps is executed one after the other > >>> until +one step fails or the end of the sequence is reached. > >> > >> The document doesn't give any hint of what the driver should do if > >> running the power sequence fails. Run the "opposite" power sequence? > >> Will that work for all resources? I'm mainly thinking of a case where > >> each enable of the resource should be matched by a disable, i.e. you > >> can't call disable if no enable was called. > > > > We discussed that issue already (around v5 I think) and the conclusion was > > that it should be up to the driver. When we simply enable/disable > > resources it is easy to revert, but in the future non-boolean properties > > will likely be introduced, and these cannot easily be reverted. Moreover > > some drivers might have more complex recovery needs. This deserves more > > discussion I think, as I'd like to have some "generic" recovery mechanism > > that covers most of the cases. > > Ok. I'll need to dig up the conversation IIRC it was somewhere around here: https://lkml.org/lkml/2012/9/7/662 See the parent messages too. > Did you consider any examples > of how some driver could handle the error cases? For all the (limited) use cases I considered, playing the power-off sequence when power-on fails just works. If power-off also fails you are potentially in more trouble though. Maybe we could have another "run" function that does not stop on errors for handling such cases where you want to "stop everything you can". > What I'm worried about is that, as far as I understand, the power > sequence is kinda like black box to the driver. The driver just does > "power-up", without knowing what really goes on in there. The driver could always inspect the sequence, but you are right in that this is not how it is intended to be done. > And if it doesn't know what goes on in there, nor what's in "power-down" > sequence, how can it do anything when an error happens? The only option > I see is that the driver doesn't do anything, which will leave some > resources enabled, or it can run the power-down sequence, which may or > may not work. Failures might be better handled if sequences have some "recovery policy" about what to do when they fail, as mentioned in the link above. As you pointed out, the driver might not always know enough about the resources involved to do the right thing. Alex.
On 2012-11-21 10:32, Alex Courbot wrote: >> Ok. I'll need to dig up the conversation > > IIRC it was somewhere around here: > > https://lkml.org/lkml/2012/9/7/662 > > See the parent messages too. Thanks. >> Did you consider any examples >> of how some driver could handle the error cases? > > For all the (limited) use cases I considered, playing the power-off sequence > when power-on fails just works. If power-off also fails you are potentially in > more trouble though. Maybe we could have another "run" function that does not > stop on errors for handling such cases where you want to "stop everything you > can". If the power-off sequence disables a regulator that was supposed to be enabled by the power-on sequence (but wasn't enabled because of an error), the regulator_disable is still called when the driver runs the power-off sequence, isn't it? Regulator enables and disables are ref counted, and the enables should match the disables. > Failures might be better handled if sequences have some "recovery policy" > about what to do when they fail, as mentioned in the link above. As you > pointed out, the driver might not always know enough about the resources > involved to do the right thing. Yes, I think such recovery policy would be needed. Tomi
On Wednesday 21 November 2012 16:48:45 Tomi Valkeinen wrote: > If the power-off sequence disables a regulator that was supposed to be > enabled by the power-on sequence (but wasn't enabled because of an > error), the regulator_disable is still called when the driver runs the > power-off sequence, isn't it? Regulator enables and disables are ref > counted, and the enables should match the disables. And there collapses my theory. > > Failures might be better handled if sequences have some "recovery policy" > > about what to do when they fail, as mentioned in the link above. As you > > pointed out, the driver might not always know enough about the resources > > involved to do the right thing. > > Yes, I think such recovery policy would be needed. Indeed, from your last paragraph this makes even more sense now. Oh, and I noticed I forgot to reply to this: > This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single > device, no matter where and how many times it's used? That's true - however when dereferencing the phandle, the underlying framework will try to acquire the PWM, which will result in failure if the same resource is referenced several times. One could compare the phandles to avoid this, but in your example you must know that for PWMs the "xyz" part is not relevant for comparison. This makes referencing of resources by name much easier to implement and more elegant with respect to frameworks leveraging. Alex.
On 2012-11-21 06:23, Alex Courbot wrote: > Hi Grant, > > On Wednesday 21 November 2012 05:54:29 Grant Likely wrote: >>> 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 >> >> This isn't strictly true. It is still perfectly fine to have board >> specific code when necessary. However, there is strong encouragement to >> enable that code in device drivers as much as possible and new board >> files need to have very strong justification. > > But doesn't introducing board-specific code into the kernel just defeats the > purpose of the DT? If we extend this logic, we are heading straight back to > board-definition files. To a lesser extent than before I agree, but the problem > is fundamentally the same. I don't think so. I'll reiterate my opinion on this subject, as a summary and for those who haven't read the discussions of the earlier versions of the series. And perhaps I'll even manage to say something new =). First about the board specific code. I think we may need some board specific code, even if this series was merged. Let's call them board drivers. These board drivers would only exist for boards with some weird setups that cannot be expressed or managed with DT and normal drivers. I think these cases would be quite rare, as I can't even come up with a very good example. I guess most likely these cases would involve some small trivial chips for muxing or such, for which it doesn't really make sense to have a real driver. Say, perhaps a board with two LCDs connected to one video output, and only one LCD can be enabled at a time, and you need to set some mux chip to route the signals to the correct LCD. In this case I'd see we should have hotplug support in the display framework, and the board driver would act on user input (sysfs file, perhaps), plugging in/out the LCD device depending on the user input. As for expressing device internal details in the DT data, as done in this series, I don't like it very much. I think the DT data or the board file should just describe which device is the one in question, and how it's connected to other components on the board. The driver for the device should handle everything else. As Alex pointed out, there may be lots of devices that work the same way when enabled, but require slightly different power-on/off sequences. We could have these sequences in the driver itself, either as plain code, or in a table of some sort, possibly using the power sequence framework presented in this series. The correct code or sequence would be ran depending on the particular model of the device. I think this approach is correct in the sense that this is what drivers are supposed to do: handle all the device internal matters. But this raises the problem of bloating the kernel with possibly lots of different power sequences, of which only a few would be used by a board, and all the rest would just be waste of memory. Regarding this problem I have the following questions, to which I don't have clear answers: - How much of this device specific data is too much? If a driver supports 10 different models, and the sequences for each model take 100 bytes, this 1000 bytes doesn't sound too much. But where's the limit? And even if one driver only has 1kB of this data, what if we have lots of similar drivers? - How many bytes does a power sequence presented in this series take, if the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and regulator? - How likely is it that we'll get lots of different models? A hundred different models for a backlight PWM with different power-on/off sequences already sounds a really big number. If we're only going to have each driver supporting, say, no more than 10 models, perhaps the problem is not even an issue in practice. - Are there ways to limit this bloat in the driver? One obvious way would be to discard the unused sequences after driver probe, but that only works for platform devices. Although, I guess these sequences would mostly be used by platform drivers? If so, then the problem could be solved by discarding the data after probe. Another would be to load the sequences from a file as firmware, but that feels quite an awful solution. Anybody have other ideas? One clear positive side with in-driver approach is that it's totally inside the kernel, and can be easily reworked in the future, or even changed to a DT-based approach as presented in this series if that seems like a better solution. The DT based approach, on the other hand, will be more or less written to stone after it's merged. So, I like the framework of expressing the sequences presented in this series (except there's a problem with error cases, as I pointed out in another post), as it can be used inside the drivers. But I'm not so enthusiastic about presenting the sequences in DT. My suggestion would be to go forward with an in-driver solution, and look at the DT based solution later if we are seeing an increasing bloat in the drivers. Tomi
On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote: > On 2012-11-21 06:23, Alex Courbot wrote: > > Hi Grant, > > > > On Wednesday 21 November 2012 05:54:29 Grant Likely wrote: > >>> 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 > >> > >> This isn't strictly true. It is still perfectly fine to have board > >> specific code when necessary. However, there is strong encouragement to > >> enable that code in device drivers as much as possible and new board > >> files need to have very strong justification. > > > > But doesn't introducing board-specific code into the kernel just defeats the > > purpose of the DT? If we extend this logic, we are heading straight back to > > board-definition files. To a lesser extent than before I agree, but the problem > > is fundamentally the same. > > I don't think so. I'll reiterate my opinion on this subject, as a > summary and for those who haven't read the discussions of the earlier > versions of the series. And perhaps I'll even manage to say something > new =). > > > First about the board specific code. I think we may need some board > specific code, even if this series was merged. Let's call them board > drivers. These board drivers would only exist for boards with some weird > setups that cannot be expressed or managed with DT and normal drivers. > > I think these cases would be quite rare, as I can't even come up with a > very good example. I guess most likely these cases would involve some > small trivial chips for muxing or such, for which it doesn't really make > sense to have a real driver. > > Say, perhaps a board with two LCDs connected to one video output, and > only one LCD can be enabled at a time, and you need to set some mux chip > to route the signals to the correct LCD. In this case I'd see we should > have hotplug support in the display framework, and the board driver > would act on user input (sysfs file, perhaps), plugging in/out the LCD > device depending on the user input. > > > As for expressing device internal details in the DT data, as done in > this series, I don't like it very much. I think the DT data or the board > file should just describe which device is the one in question, and how > it's connected to other components on the board. The driver for the > device should handle everything else. > > As Alex pointed out, there may be lots of devices that work the same way > when enabled, but require slightly different power-on/off sequences. We > could have these sequences in the driver itself, either as plain code, > or in a table of some sort, possibly using the power sequence framework > presented in this series. The correct code or sequence would be ran > depending on the particular model of the device. > > I think this approach is correct in the sense that this is what drivers > are supposed to do: handle all the device internal matters. But this > raises the problem of bloating the kernel with possibly lots of > different power sequences, of which only a few would be used by a board, > and all the rest would just be waste of memory. > > Regarding this problem I have the following questions, to which I don't > have clear answers: > > - How much of this device specific data is too much? If a driver > supports 10 different models, and the sequences for each model take 100 > bytes, this 1000 bytes doesn't sound too much. But where's the limit? > And even if one driver only has 1kB of this data, what if we have lots > of similar drivers? > > - How many bytes does a power sequence presented in this series take, if > the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and > regulator? > > - How likely is it that we'll get lots of different models? A hundred > different models for a backlight PWM with different power-on/off > sequences already sounds a really big number. If we're only going to > have each driver supporting, say, no more than 10 models, perhaps the > problem is not even an issue in practice. > > - Are there ways to limit this bloat in the driver? One obvious way > would be to discard the unused sequences after driver probe, but that > only works for platform devices. Although, I guess these sequences would > mostly be used by platform drivers? If so, then the problem could be > solved by discarding the data after probe. Another would be to load the > sequences from a file as firmware, but that feels quite an awful > solution. Anybody have other ideas? > > One clear positive side with in-driver approach is that it's totally > inside the kernel, and can be easily reworked in the future, or even > changed to a DT-based approach as presented in this series if that seems > like a better solution. The DT based approach, on the other hand, will > be more or less written to stone after it's merged. > > > So, I like the framework of expressing the sequences presented in this > series (except there's a problem with error cases, as I pointed out in > another post), as it can be used inside the drivers. But I'm not so > enthusiastic about presenting the sequences in DT. > > My suggestion would be to go forward with an in-driver solution, and > look at the DT based solution later if we are seeing an increasing bloat > in the drivers. Assuming we go with your approach, what's the plan? We're actually facing this problem right now for Tegra. Basically we have a DRM driver that can drive the panel, but we're still missing a way to hook up the backlight and panel enabling code. So we effectively can't support any of the LVDS devices out there without this series. As I understand it, what you propose is similar to what ASoC does. For a specific board, you'd have to write a driver, presumably for the new panel/display framework, that provides code to power the panel on and off. That means we'll have to have a driver for each panel out there basically, or we'd need to write generic drivers that can be configured to some degree (via platform data or DT). This is similar to how ASoC works, where we have a driver that provides support for a specific codec connected to the Tegra SoC. For the display framework things could be done in a similar way I suppose, so that Tegra could have one display driver to handle all aspects of powering on and off the various panels for the various boards out there. Obviously, a lot of the code will be similar for other SoCs, but maybe that's just the way things are if we choose that approach. There's also the potential for factoring out large chunks of common code later on once we start to see common patterns. One thing that's not very clear is how the backlight subsystem should be wired up with the display framework. I have a patch on top of the Tegra DRM driver which adds some ad-hoc display support using this power sequences series and the pwm-backlight. From reading the proposal for the panel/display framework, it sounds like a lot more is planned than just enabling or disabling panels, but it also seems like a lot of code needs to be written to support things like DSI, DBI or other control busses. At least for Tegra, and I think the same holds for a wide variety of other SoCs, dumb panels would be enough for a start. In the interest of getting a working solution for those setups, maybe we can start small and add just enough framework to register dumb panel drivers to along with code to wire up a backlight to light up the display. Then we could possibly still make it to have a proper solution to support the various LVDS panels for Tegra with 3.9. I'm adding Laurent on Cc since he's probably busy working on a new proposal for the panel/display framework. Maybe he can share his thought on this. All of the above said, I'm willing to help out with the coding if that's what is required to reach a solution that everybody can be happy with. Thierry
On 2012-11-21 13:40, Thierry Reding wrote: > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote: (sorry for bouncing back and forth with my private and my @ti addresses. I can't find an option in thunderbird to only use one sender address, and I always forget to change it when responding...) >> My suggestion would be to go forward with an in-driver solution, and >> look at the DT based solution later if we are seeing an increasing bloat >> in the drivers. > > Assuming we go with your approach, what's the plan? We're actually > facing this problem right now for Tegra. Basically we have a DRM driver > that can drive the panel, but we're still missing a way to hook up the > backlight and panel enabling code. So we effectively can't support any > of the LVDS devices out there without this series. Could you describe the hardware setup you have related to the LCD and backlight? Is it a public board with public schematics? I've understood that you don't have anything special in your board, just an LCD and a backlight, and the power sequences are related to powering up the LCD and the backlight, without anything board specific. If so, there's no need for board specific code, but just improving the panel and backlight drivers to support the models you use. > As I understand it, what you propose is similar to what ASoC does. For a > specific board, you'd have to write a driver, presumably for the new > panel/display framework, that provides code to power the panel on and > off. That means we'll have to have a driver for each panel out there > basically, or we'd need to write generic drivers that can be configured > to some degree (via platform data or DT). This is similar to how ASoC > works, where we have a driver that provides support for a specific codec > connected to the Tegra SoC. For the display framework things could be > done in a similar way I suppose, so that Tegra could have one display > driver to handle all aspects of powering on and off the various panels > for the various boards out there. I think we should only need the board drivers for very special cases. If there's just a panel and a backlight, without any special dynamic muxing or other trickery needed, I don't see a need for a board driver. I presume this is the case for most of the boards. > Obviously, a lot of the code will be similar for other SoCs, but maybe > that's just the way things are if we choose that approach. There's also > the potential for factoring out large chunks of common code later on > once we start to see common patterns. > > One thing that's not very clear is how the backlight subsystem should be > wired up with the display framework. I have a patch on top of the Tegra > DRM driver which adds some ad-hoc display support using this power > sequences series and the pwm-backlight. I think that's a separate issue: how to associate the lcd device and backlight device together. I don't have a clear answer to this. There are many ways the backlight may be handled. In some cases the panel and the backlight are truly independent, and you can use the other without using the other (not very practical, though =). But then with some LCDs the backlight may be controlled by sending commands to the panel, and in this case the two may be quite linked. Changing the backlight requires the panel driver to be up and running, and sometimes the sending the backlight commands may need to be (say, DSI display, with backlight commands going over the DSI bus). So my feeling is that the panel driver should know about the related backlight device. In the first case the panel driver would just call enable/disable in the backlight device when the panel is turned on. In the second case of the DSI panel... I'm not sure. I've implemented it so that the panel driver creates the backlight device, and implements the backlight callbacks. It then sends the DSI commands from those callbacks. > From reading the proposal for the panel/display framework, it sounds > like a lot more is planned than just enabling or disabling panels, but > it also seems like a lot of code needs to be written to support things > like DSI, DBI or other control busses. > > At least for Tegra, and I think the same holds for a wide variety of > other SoCs, dumb panels would be enough for a start. In the interest of > getting a working solution for those setups, maybe we can start small > and add just enough framework to register dumb panel drivers to along > with code to wire up a backlight to light up the display. Then we could > possibly still make it to have a proper solution to support the various > LVDS panels for Tegra with 3.9. Yes, we (Laurent and me) both agree that we should start simple. However, the common panel framework is not strictly needed for this. I'm not sure of the current architecture for Tegra, but for OMAP we already have panel drivers (omap specific ones, though). The panel drivers may support multiple models, (for example, drivers/video/omap2/displays/panel-generic-dpi.c). I don't see any problem with adding small Tegra specific panel drivers for the time being, with the intention of converting to common panel framework when that's available. Of course, the DT side is an issue. If you now create DT bindings for a temporary model, and need to change it again later, you'll have some headaches trying managing that without breaking the old bindings... This is why I haven't pushed DT bindings for OMAP, as I know I have to change them in the near future. Tomi
On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote: > On 2012-11-21 13:40, Thierry Reding wrote: > > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote: > > (sorry for bouncing back and forth with my private and my @ti addresses. > I can't find an option in thunderbird to only use one sender address, > and I always forget to change it when responding...) > > >> My suggestion would be to go forward with an in-driver solution, and > >> look at the DT based solution later if we are seeing an increasing bloat > >> in the drivers. > > > > Assuming we go with your approach, what's the plan? We're actually > > facing this problem right now for Tegra. Basically we have a DRM driver > > that can drive the panel, but we're still missing a way to hook up the > > backlight and panel enabling code. So we effectively can't support any > > of the LVDS devices out there without this series. > > Could you describe the hardware setup you have related to the LCD and > backlight? Is it a public board with public schematics? I don't think any of the schematics are public. The Tamonten Evaluation Carrier is available publicly from our website and the schematics are available on demand as well. If required I can probably arrange to send you a copy. > I've understood that you don't have anything special in your board, just > an LCD and a backlight, and the power sequences are related to powering > up the LCD and the backlight, without anything board specific. If so, > there's no need for board specific code, but just improving the panel > and backlight drivers to support the models you use. Correct. Basically we have two GPIOs that each enable the panel or the backlight respectively and one PWM to control the brightness. Are the panel drivers that you refer to those in drivers/video? I'm not sure if adding more ad-hoc drivers there just to move them to a generic framework in the next cycle is a good idea. I'd rather spend some time on helping to get the framework right and have drivers for that instead. From what I understand by looking at the OMAP display drivers, they also provide the timings for the displays. Steffen's videomode helpers can be used to represent these easily in DT, but I suppose if all of those per- panel specifics are represented in the drivers then that won't be needed anymore either. > > As I understand it, what you propose is similar to what ASoC does. For a > > specific board, you'd have to write a driver, presumably for the new > > panel/display framework, that provides code to power the panel on and > > off. That means we'll have to have a driver for each panel out there > > basically, or we'd need to write generic drivers that can be configured > > to some degree (via platform data or DT). This is similar to how ASoC > > works, where we have a driver that provides support for a specific codec > > connected to the Tegra SoC. For the display framework things could be > > done in a similar way I suppose, so that Tegra could have one display > > driver to handle all aspects of powering on and off the various panels > > for the various boards out there. > > I think we should only need the board drivers for very special cases. If > there's just a panel and a backlight, without any special dynamic muxing > or other trickery needed, I don't see a need for a board driver. I > presume this is the case for most of the boards. For Tegra ASoC, the way to provide for this is to allow a specific board to introduce a separate compatible value to enable per-board quirks or special handling if it cannot be supported by the generic driver and configuration mechanisms. > > Obviously, a lot of the code will be similar for other SoCs, but maybe > > that's just the way things are if we choose that approach. There's also > > the potential for factoring out large chunks of common code later on > > once we start to see common patterns. > > > > One thing that's not very clear is how the backlight subsystem should be > > wired up with the display framework. I have a patch on top of the Tegra > > DRM driver which adds some ad-hoc display support using this power > > sequences series and the pwm-backlight. > > I think that's a separate issue: how to associate the lcd device and > backlight device together. I don't have a clear answer to this. > > There are many ways the backlight may be handled. In some cases the > panel and the backlight are truly independent, and you can use the other > without using the other (not very practical, though =). At least for DT I think we can easily wire that up. I've actually posted a patch recently that does so. I think in most cases it makes sense to control them together, such as on DPMS changes, where you really want to turn both the backlight and the LCD off, independent of how they are tied together. > But then with some LCDs the backlight may be controlled by sending > commands to the panel, and in this case the two may be quite linked. > Changing the backlight requires the panel driver to be up and running, > and sometimes the sending the backlight commands may need to be (say, > DSI display, with backlight commands going over the DSI bus). > > So my feeling is that the panel driver should know about the related > backlight device. In the first case the panel driver would just call > enable/disable in the backlight device when the panel is turned on. Exactly. > In the second case of the DSI panel... I'm not sure. I've implemented it > so that the panel driver creates the backlight device, and implements > the backlight callbacks. It then sends the DSI commands from those > callbacks. That certainly sounds like the right approach to me. > > From reading the proposal for the panel/display framework, it sounds > > like a lot more is planned than just enabling or disabling panels, but > > it also seems like a lot of code needs to be written to support things > > like DSI, DBI or other control busses. > > > > At least for Tegra, and I think the same holds for a wide variety of > > other SoCs, dumb panels would be enough for a start. In the interest of > > getting a working solution for those setups, maybe we can start small > > and add just enough framework to register dumb panel drivers to along > > with code to wire up a backlight to light up the display. Then we could > > possibly still make it to have a proper solution to support the various > > LVDS panels for Tegra with 3.9. > > Yes, we (Laurent and me) both agree that we should start simple. > > However, the common panel framework is not strictly needed for this. I'm > not sure of the current architecture for Tegra, but for OMAP we already > have panel drivers (omap specific ones, though). The panel drivers may > support multiple models, (for example, > drivers/video/omap2/displays/panel-generic-dpi.c). > > I don't see any problem with adding small Tegra specific panel drivers > for the time being, with the intention of converting to common panel > framework when that's available. I can take a look at how such a driver could be implemented, but again, I'm a bit reluctant to add something ad-hoc now when maybe we can have it supported in a proper framework not too far away in the future. > Of course, the DT side is an issue. If you now create DT bindings for a > temporary model, and need to change it again later, you'll have some > headaches trying managing that without breaking the old bindings... This > is why I haven't pushed DT bindings for OMAP, as I know I have to change > them in the near future. We're already keeping back on this and none of the patches that define the bindings have been merged yet. Although bindings have been known to change every once in a while even for code that is already in mainline. Thierry
On 2012-11-21 15:00, Thierry Reding wrote: > On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote: >> On 2012-11-21 13:40, Thierry Reding wrote: >>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote: >> >> (sorry for bouncing back and forth with my private and my @ti addresses. >> I can't find an option in thunderbird to only use one sender address, >> and I always forget to change it when responding...) >> >>>> My suggestion would be to go forward with an in-driver solution, and >>>> look at the DT based solution later if we are seeing an increasing bloat >>>> in the drivers. >>> >>> Assuming we go with your approach, what's the plan? We're actually >>> facing this problem right now for Tegra. Basically we have a DRM driver >>> that can drive the panel, but we're still missing a way to hook up the >>> backlight and panel enabling code. So we effectively can't support any >>> of the LVDS devices out there without this series. >> >> Could you describe the hardware setup you have related to the LCD and >> backlight? Is it a public board with public schematics? > > I don't think any of the schematics are public. The Tamonten Evaluation > Carrier is available publicly from our website and the schematics are > available on demand as well. If required I can probably arrange to send > you a copy. No need, I think your answer below is enough. >> I've understood that you don't have anything special in your board, just >> an LCD and a backlight, and the power sequences are related to powering >> up the LCD and the backlight, without anything board specific. If so, >> there's no need for board specific code, but just improving the panel >> and backlight drivers to support the models you use. > > Correct. Basically we have two GPIOs that each enable the panel or the > backlight respectively and one PWM to control the brightness. Are the The panel GPIO goes to the panel hardware device, and enables the panel? And similarly for the PWM GPIO, it goes to the PWM hardware device? Just making sure there are no other components involved. > panel drivers that you refer to those in drivers/video? I'm not sure if > adding more ad-hoc drivers there just to move them to a generic > framework in the next cycle is a good idea. I'd rather spend some time > on helping to get the framework right and have drivers for that instead. We have panel drivers for omap in drivers/video/omap2/displays/. I'm not sure if other platforms have their own versions of panel drivers, but probably adding a simple panel driver system for a platform would not be too difficult. It could even be quite hardcoded, i.e. embedded directly into the display subsystem driver, just to get something working until the common panel framework is available. Yes, I agree it's not good idea to add more platform specific panel drivers. But it's unclear when CPF will be merged, so if you need to get the panel working now, I don't see a simple ad-hoc driver as too horrible. But, of course, I'm not the one making the decision whether to merge or not =). > From what I understand by looking at the OMAP display drivers, they also > provide the timings for the displays. Steffen's videomode helpers can be > used to represent these easily in DT, but I suppose if all of those per- Right. Note that I didn't present omap panel drivers as perfect examples, just examples =). > panel specifics are represented in the drivers then that won't be needed > anymore either. Yes, for most panels with just one native mode and nothing else, the panel driver can contain the timings. However, this subject has been discussed earlier a few times. If the panel in question doesn't need any special power-on/off sequences, just, perhaps, one gpio or such, we could still use DT video modes. This would simplify the cases where you have lots of different very simple panels. Obviously the same questions apply to DT video modes than to the power sequences, and while I do think it's better to handle the timings inside the driver, I'm not too much against video timings in DT. The reason being that the video modes are quite clear, simple and stable data, versus the much more complex and open-ended power sequences. >> I don't see any problem with adding small Tegra specific panel drivers >> for the time being, with the intention of converting to common panel >> framework when that's available. > > I can take a look at how such a driver could be implemented, but again, Don't look at the mainline omap panel drivers, at least not too closely =). They contain hackery that will be cleaned up with CPF. I think there are two methods to implements simple panel drivers: The hardcoded one, where the display subsystem driver manages a few different panel models. This is obviously not very expandable or "correct", but should probably work just fine for a few models, until CPF is usable. Something like CPF will have: have the panel device/driver as a platform device/driver, which will register itself to the display subsystem. And with "itself" I mean some kind of struct panel_entity, with a few ops implemented by the panel driver. Well, this goes a bit out of subject. If you want to discuss panel drivers more, please start a new thread. Laurent's upcoming CPF v2 should give you good ideas what the model will be. Tomi
Mmmm so maybe I am misinterpreting things, but it looks like we have just buried the power sequences here, haven't we? Alex. On Wed, Nov 21, 2012 at 10:32 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On 2012-11-21 15:00, Thierry Reding wrote: >> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote: >>> On 2012-11-21 13:40, Thierry Reding wrote: >>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote: >>> >>> (sorry for bouncing back and forth with my private and my @ti addresses. >>> I can't find an option in thunderbird to only use one sender address, >>> and I always forget to change it when responding...) >>> >>>>> My suggestion would be to go forward with an in-driver solution, and >>>>> look at the DT based solution later if we are seeing an increasing bloat >>>>> in the drivers. >>>> >>>> Assuming we go with your approach, what's the plan? We're actually >>>> facing this problem right now for Tegra. Basically we have a DRM driver >>>> that can drive the panel, but we're still missing a way to hook up the >>>> backlight and panel enabling code. So we effectively can't support any >>>> of the LVDS devices out there without this series. >>> >>> Could you describe the hardware setup you have related to the LCD and >>> backlight? Is it a public board with public schematics? >> >> I don't think any of the schematics are public. The Tamonten Evaluation >> Carrier is available publicly from our website and the schematics are >> available on demand as well. If required I can probably arrange to send >> you a copy. > > No need, I think your answer below is enough. > >>> I've understood that you don't have anything special in your board, just >>> an LCD and a backlight, and the power sequences are related to powering >>> up the LCD and the backlight, without anything board specific. If so, >>> there's no need for board specific code, but just improving the panel >>> and backlight drivers to support the models you use. >> >> Correct. Basically we have two GPIOs that each enable the panel or the >> backlight respectively and one PWM to control the brightness. Are the > > The panel GPIO goes to the panel hardware device, and enables the panel? > And similarly for the PWM GPIO, it goes to the PWM hardware device? Just > making sure there are no other components involved. > >> panel drivers that you refer to those in drivers/video? I'm not sure if >> adding more ad-hoc drivers there just to move them to a generic >> framework in the next cycle is a good idea. I'd rather spend some time >> on helping to get the framework right and have drivers for that instead. > > We have panel drivers for omap in drivers/video/omap2/displays/. I'm not > sure if other platforms have their own versions of panel drivers, but > probably adding a simple panel driver system for a platform would not be > too difficult. It could even be quite hardcoded, i.e. embedded directly > into the display subsystem driver, just to get something working until > the common panel framework is available. > > Yes, I agree it's not good idea to add more platform specific panel > drivers. But it's unclear when CPF will be merged, so if you need to get > the panel working now, I don't see a simple ad-hoc driver as too > horrible. But, of course, I'm not the one making the decision whether to > merge or not =). > >> From what I understand by looking at the OMAP display drivers, they also >> provide the timings for the displays. Steffen's videomode helpers can be >> used to represent these easily in DT, but I suppose if all of those per- > > Right. Note that I didn't present omap panel drivers as perfect > examples, just examples =). > >> panel specifics are represented in the drivers then that won't be needed >> anymore either. > > Yes, for most panels with just one native mode and nothing else, the > panel driver can contain the timings. > > However, this subject has been discussed earlier a few times. If the > panel in question doesn't need any special power-on/off sequences, just, > perhaps, one gpio or such, we could still use DT video modes. This would > simplify the cases where you have lots of different very simple panels. > > Obviously the same questions apply to DT video modes than to the power > sequences, and while I do think it's better to handle the timings inside > the driver, I'm not too much against video timings in DT. The reason > being that the video modes are quite clear, simple and stable data, > versus the much more complex and open-ended power sequences. > >>> I don't see any problem with adding small Tegra specific panel drivers >>> for the time being, with the intention of converting to common panel >>> framework when that's available. >> >> I can take a look at how such a driver could be implemented, but again, > > Don't look at the mainline omap panel drivers, at least not too closely > =). They contain hackery that will be cleaned up with CPF. > > I think there are two methods to implements simple panel drivers: > > The hardcoded one, where the display subsystem driver manages a few > different panel models. This is obviously not very expandable or > "correct", but should probably work just fine for a few models, until > CPF is usable. > > Something like CPF will have: have the panel device/driver as a platform > device/driver, which will register itself to the display subsystem. And > with "itself" I mean some kind of struct panel_entity, with a few ops > implemented by the panel driver. > > Well, this goes a bit out of subject. If you want to discuss panel > drivers more, please start a new thread. Laurent's upcoming CPF v2 > should give you good ideas what the model will be. > > Tomi > >
On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote: > Mmmm so maybe I am misinterpreting things, but it looks like we > have just buried the power sequences here, haven't we? I don't think so. In fact I was just starting to think that maybe for Tegra we could have a generic panel driver which used power sequences to abstract away the pin differences for powering up the panel. Since most likely that will be where the differences are there is a lot of potential to factor things out into sequences. Perhaps what we may want to postpone for now is the DT representation since that's what Tomi and Grant seem to be mostly opposed to. Thierry
On Wed, 21 Nov 2012 10:31:34 +0900, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Tue, Nov 20, 2012 at 09:54:29PM +0000, Grant Likely wrote: > > On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote: > > > > 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 > > > This isn't strictly true. It is still perfectly fine to have board > > specific code when necessary. However, there is strong encouragement to > > enable that code in device drivers as much as possible and new board > > files need to have very strong justification. > > This isn't the message that's gone over, and even for device drivers > everyone seems to be taking the whole device tree thing as a move to > pull all data out of the kernel. In some cases there are some real > practical advantages to doing this but a lot of the people making these > changes seem to view having things in DT as a goal in itself. I know, and I do apologize for my part in not communicating and shepherding enough. For anyone reading; please be pragmatic on putting things into the DT. If a binding starts to look really complex, ask whether or not it helps things. > > I'm thinking about the scripts as trivial-to-parse ascii strings that > > have a very simple set of commands. The commands use resources already > > defined in the node. ie. 'g0' refers to the first entry in the gpios > > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By > > no means take this as the format to use, it is just an example off the > > top of my head, but it is already way easier to work with than putting > > each command into a node. > > It does appear to have some legibility challenges, especially with using > the indexes into the arrays of resources. On the other hand the arrays > should be fairly small. This is merely an example off the top of my head. I'm in no way wed to the syntax. > > > +Platform Data Format > > > +-------------------- > > > +All relevant data structures for declaring power sequences are located in > > > +include/linux/power_seq.h. > > > Hmm... If sequences are switched to a string instead, then platform_data > > should also use it. The power sequence data structures can be created at > > runtime by parsing the string. > > Seems like a step backwards to me - why not let people store the more > parsed version of the data? It's going to be less convenient for users > on non-DT systems. > > As I said in my earlier reviews I think this is a useful thing to have > as a utility library for drivers independantly of the DT bindings, it > would allow drivers to become more data driven. Perhaps we can rework > the series so that the DT bindings are added as a final patch? All the > rest of the code seems OK. I'm fine with that if the bindings don't get merged yet. g.
On Thu, Nov 22, 2012 at 12:12 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote: >> Mmmm so maybe I am misinterpreting things, but it looks like we >> have just buried the power sequences here, haven't we? > > I don't think so. In fact I was just starting to think that maybe for > Tegra we could have a generic panel driver which used power sequences > to abstract away the pin differences for powering up the panel. Since > most likely that will be where the differences are there is a lot of > potential to factor things out into sequences. > > Perhaps what we may want to postpone for now is the DT representation > since that's what Tomi and Grant seem to be mostly opposed to. The thing I don't understand here is why would anyone want power sequences without the DT representation. Guys, that's the whole point! :) If we are to implement things into drivers, then callback functions are going to serve us just as well - even better, for they are more flexible. All we need to do is define a dedicated ops structure and have the driver plug the right callback functions depending on the "compatible" property of the DT device node. We don't need a framework for that. And if we are not going to use power seqs that's probably what we should do in order to get panels to work on Tegra boards for now. Then see how things turn out with the panel framework and whether there is a use for power seqs in it. But by the meantime, I don't see any motivation to merge power seqs sans DT support. Alex.
On Thu, Nov 22, 2012 at 11:01:34AM +0900, Alexandre Courbot wrote: > The thing I don't understand here is why would anyone want power > sequences without the DT representation. Guys, that's the whole point! :) > If we are to implement things into drivers, then callback functions > are going to serve us just as well - even better, for they are more > flexible. All we need to do is define a dedicated ops structure and > have the driver plug the right callback functions depending on the > "compatible" property of the DT device node. We don't need a framework > for that. It allows drivers (both board drivers and actual drivers) to write these things in a semi-scripted form instead of having to open code everything each time, it'd save a bunch of tedious stuff with resource requesting for example.
On Thu, Nov 22, 2012 at 11:06 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Thu, Nov 22, 2012 at 11:01:34AM +0900, Alexandre Courbot wrote: > >> The thing I don't understand here is why would anyone want power >> sequences without the DT representation. Guys, that's the whole point! :) > >> If we are to implement things into drivers, then callback functions >> are going to serve us just as well - even better, for they are more >> flexible. All we need to do is define a dedicated ops structure and >> have the driver plug the right callback functions depending on the >> "compatible" property of the DT device node. We don't need a framework >> for that. > > It allows drivers (both board drivers and actual drivers) to write these > things in a semi-scripted form instead of having to open code everything > each time, it'd save a bunch of tedious stuff with resource requesting > for example. Mmm, I overlooked that point - that's fair enough. Guess I should remove all DT support and stress that point in the documentation. Then maybe we'll have a deal. Alex.
On Wed, Nov 21, 2012 at 2:31 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > As I said in my earlier reviews I think this is a useful thing to have > as a utility library for drivers independantly of the DT bindings, it > would allow drivers to become more data driven. Perhaps we can rework > the series so that the DT bindings are added as a final patch? All the > rest of the code seems OK. I have the same (limited by experience) opinion. Working sort of near som audio drivers I have understood that power sequencing is a big issue not only for things like this backlight example, but even more so in the area of audio to avoid clicks and pops. Is it correct to assume that this library will be useful also for ALSA SoC type of devices? Then whether to couple that to DT or ACPI or SFI or whatever is a different discussion altogether. Yours, Linus Walleij
On Thu, Nov 22, 2012 at 5:57 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > I have the same (limited by experience) opinion. Working sort of > near som audio drivers I have understood that power sequencing > is a big issue not only for things like this backlight example, but > even more so in the area of audio to avoid clicks and pops. > > Is it correct to assume that this library will be useful also for ALSA > SoC type of devices? There should be nothing against that - provided ALSA's needs remain similar to the current set of actions (regulators, gpios, ...) it should be perfectly usable. Can you think of any usage that involves more than dumb regulator/gpio manipulation? If power seqs are to be used this way (as opposed to through the DT), it will become urgent to implement that gpio handlers library we were talking about in another thread, as we don't want to have GPIO numbers hard-coded into the sequences. Alex.
On Wed, Nov 21, 2012 at 10:00 AM, Alex Courbot <acourbot@nvidia.com> wrote: > On Wednesday 21 November 2012 16:48:45 Tomi Valkeinen wrote: >> If the power-off sequence disables a regulator that was supposed to be >> enabled by the power-on sequence (but wasn't enabled because of an >> error), the regulator_disable is still called when the driver runs the >> power-off sequence, isn't it? Regulator enables and disables are ref >> counted, and the enables should match the disables. > > And there collapses my theory. > >> > Failures might be better handled if sequences have some "recovery policy" >> > about what to do when they fail, as mentioned in the link above. As you >> > pointed out, the driver might not always know enough about the resources >> > involved to do the right thing. >> >> Yes, I think such recovery policy would be needed. > > Indeed, from your last paragraph this makes even more sense now. > > Oh, and I noticed I forgot to reply to this: > >> This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single >> device, no matter where and how many times it's used? > > That's true - however when dereferencing the phandle, the underlying framework > will try to acquire the PWM, which will result in failure if the same resource > is referenced several times. > > One could compare the phandles to avoid this, but in your example you must > know that for PWMs the "xyz" part is not relevant for comparison. > > This makes referencing of resources by name much easier to implement and more > elegant with respect to frameworks leveraging. I would rather (at least for how the DT bindings settle out) see the design separate the resource references from the sequence. ie. Declare which resources are used by a device's sequences all in one place and have the commands index into that. g.
On Wed, 21 Nov 2012 13:23:06 +0900, Alex Courbot <acourbot@nvidia.com> wrote: > Hi Grant, > > On Wednesday 21 November 2012 05:54:29 Grant Likely wrote: > > > 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 > > > > This isn't strictly true. It is still perfectly fine to have board > > specific code when necessary. However, there is strong encouragement to > > enable that code in device drivers as much as possible and new board > > files need to have very strong justification. > > But doesn't introducing board-specific code into the kernel just defeats the > purpose of the DT? If we extend this logic, we are heading straight back to > board-definition files. To a lesser extent than before I agree, but the problem > is fundamentally the same. There is *always* board specific code. There is always something fiddly enough, complex enough, or just plain ugly enough that it is best handled in C code. The trick is to make the board-specific stuff the exception, not the rule. When you can factor out common behavour (like you are doing here), then it is a really good candidate for a common binding, but still please always ask yourself the question is this going to make things better or worse in the long run. > > I'm thinking about the scripts as trivial-to-parse ascii strings that > > have a very simple set of commands. The commands use resources already > > defined in the node. ie. 'g0' refers to the first entry in the gpios > > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By > > no means take this as the format to use, it is just an example off the > > top of my head, but it is already way easier to work with than putting > > each command into a node. > > I'd argue that this opens the door wide open towards having a complete > interpreter in the device tree. At least DT nodes restrict what we can do > conveniently. I disagree... see below. > > The trick is still to define a syntax that doesn't fall apart when it > > needs to be extended. I would also like to get opinions on whether or > > not conditionals or loops should be supported (ie. loop waiting for a > > status to change). If they should then we need to be a lot more careful > > about the design (and due to my aforementioned nervousness, somebody may > > need to get me therapy). > > That's IMHO the main argument in favor of using DT nodes here (the syntax > extension, not your therapy). hahaha :-) > They can be extended simply by adding properties > to them, and I was relying on this to make power seqs extensible while keeping > things consistent (and backward-compatible). For instance, when we want to > support voltage setting in regulators we can just add a "voltage" property for > that. > > So to sum up the pros of the current node-base representation: > - give a "data like" representation of powering sequences (what they should > be, actually) > - puts sanity barriers for not turning power seqs into a real code interpreter > - easily extensible > > There are probably a few cons, the numbering of steps being one, but at least > we don't need to design a new DSL and care too much about extensibility which > is, in the nodes case, built-in and free. No matter how it is encoded, this *IS* a new DSL. Using nodes and properties doesn't change that. Extensibility is no more built-in and free with nodes that it is with another encoding. If there aren't clear guidelines on how to extend it then we'll get weird stuff. For example, even with the nodes case someone might do this: step3 { type = "loop"; count = 10; step1 { type = "gpio"; gpio = <&gpio 1>; value = 1; }; step2 { type = "delay"; value = 10000; }; step3 { type = "gpio"; gpio = <&gpio 1>; value = 0; }; step4 { type = "delay"; value = 10000; }; }; And even while cringing as I type the above, I also have to consider that looping may just be a valid use case for sequences. And even here, a very simple sequence fragment required 22 lines of code. Next, I'm concerned about where these will show up. Say for instance there needs to be a power sequence added to an spi bus node. Already spi bus child nodes have a defined meaning; they are spi slaves. How then should the sequence be attached to the spi bus? > If that makes you feel better, maybe we can try and fix what is wrong with the > current bindings instead of introducing a new language that will be, by > nature, more complex to handle and difficult to extend without breaking things? Okay, here are 3 concrete objections with the proposed binding: - The syntax concerns of it being too verbose and it effectively uses line numbers for ordering (Do you remember fighting with BASIC?). - There are many devices that won't be able to use the binding because they already have a meaning for child nodes - I think resources should be declared separate from the sequence based on the assumption that multiple steps will be operating on the same resource. I do think that each sequence should be contained within a single property, but I'm open to other suggestions. g.
On Wed, Nov 21, 2012 at 3:12 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote: >> Mmmm so maybe I am misinterpreting things, but it looks like we >> have just buried the power sequences here, haven't we? > > I don't think so. In fact I was just starting to think that maybe for > Tegra we could have a generic panel driver which used power sequences > to abstract away the pin differences for powering up the panel. Since > most likely that will be where the differences are there is a lot of > potential to factor things out into sequences. > > Perhaps what we may want to postpone for now is the DT representation > since that's what Tomi and Grant seem to be mostly opposed to. Don't give up. I'm am being cautions and trying to think of all the implications because that is what I have to do as a maintainer. I'm not opposed to the feature but I need a good level of confidence that it has been fully thought out. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
On Thu, Nov 22, 2012 at 01:39:41PM +0000, Grant Likely wrote: [...] > I do think that each sequence should be contained within a single > property, but I'm open to other suggestions. IIRC a very early prototype did implement something like that. However because of the resource issues this had to be string based, so that the sequences looked somewhat like (Alex, correct me if I'm wrong): power-on = <"REGULATOR", "power", 1, "GPIO", "enable", 1>; Instead we could possibly have something like: power-on = <0 ® 1, 1 &gpio 42 0 1>; Where the first cell in each entry defines the type (0 = regulator, 1 = GPIO) and the rest would be a regular OF specifier for the given type of resource along with some defined parameter such as enable/disable, voltage, delay in ms, ... I don't know if that sounds any better. It looks sort of cryptic but it is more "in the spirit of" DT, right Grant? Writing this down, it seems to me like even that proposal was already discussed at some point. Again, Alex may remember better. Thierry
On Thu, Nov 22, 2012 at 09:57:22AM +0100, Linus Walleij wrote: > Is it correct to assume that this library will be useful also for ALSA > SoC type of devices? ASoC has facilities for autogenerating the bulk of the sequences which with modern devices is all that you really need.
On Friday 23 November 2012 05:40:21 Thierry Reding wrote: > On Thu, Nov 22, 2012 at 01:39:41PM +0000, Grant Likely wrote: > [...] > > > I do think that each sequence should be contained within a single > > property, but I'm open to other suggestions. > > IIRC a very early prototype did implement something like that. However > because of the resource issues this had to be string based, so that the > sequences looked somewhat like (Alex, correct me if I'm wrong): > > power-on = <"REGULATOR", "power", 1, "GPIO", "enable", 1>; You're right. Back when the burning sun of July was beating down a little bit too hard on my head. https://lkml.org/lkml/2012/7/9/30 > Instead we could possibly have something like: > > power-on = <0 ® 1, > 1 &gpio 42 0 1>; > > Where the first cell in each entry defines the type (0 = regulator, 1 = > GPIO) and the rest would be a regular OF specifier for the given type of > resource along with some defined parameter such as enable/disable, > voltage, delay in ms, ... I don't know if that sounds any better. It > looks sort of cryptic but it is more "in the spirit of" DT, right Grant? > > Writing this down, it seems to me like even that proposal was already > discussed at some point. Again, Alex may remember better. The idea that we had was to use preprocessor support in DTC to use macros instead of strings for the step type. We also thought about using phandles directly in there as well, but this would require some more API support. Anyway, at the current point we are not even sure whether we want or need power seqs in the DT - so let's keep this topic on hold for a while. We can still introduce the feature without DT support, and if it eventually turns out during this winter that expressing power seqs in the DT makes sense, we will have plenty of archives to read in front of the fire. Alex.
On Thu, 22 Nov 2012 22:40:21 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Thu, Nov 22, 2012 at 01:39:41PM +0000, Grant Likely wrote: > [...] > > I do think that each sequence should be contained within a single > > property, but I'm open to other suggestions. > > IIRC a very early prototype did implement something like that. However > because of the resource issues this had to be string based, so that the > sequences looked somewhat like (Alex, correct me if I'm wrong): > > power-on = <"REGULATOR", "power", 1, "GPIO", "enable", 1>; > > Instead we could possibly have something like: > > power-on = <0 ® 1, > 1 &gpio 42 0 1>; Yes, that would work, although I still think it would be a good idea to split the used resources off into the gpios/pwms/regs/etc properties. > Where the first cell in each entry defines the type (0 = regulator, 1 = > GPIO) and the rest would be a regular OF specifier for the given type of > resource along with some defined parameter such as enable/disable, > voltage, delay in ms, ... I don't know if that sounds any better. It > looks sort of cryptic but it is more "in the spirit of" DT, right Grant? It is still kind of a ham-handed approach, but it does fit better with existing conventions than the hierarchy of nodes does. g.
Hi Thierry, Sorry for the late reply. On Wednesday 21 November 2012 12:40:18 Thierry Reding wrote: > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote: > > On 2012-11-21 06:23, Alex Courbot wrote: > > > On Wednesday 21 November 2012 05:54:29 Grant Likely wrote: > > >>> 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 > > >> > > >> This isn't strictly true. It is still perfectly fine to have board > > >> specific code when necessary. However, there is strong encouragement to > > >> enable that code in device drivers as much as possible and new board > > >> files need to have very strong justification. > > > > > > But doesn't introducing board-specific code into the kernel just defeats > > > the purpose of the DT? If we extend this logic, we are heading straight > > > back to board-definition files. To a lesser extent than before I agree, > > > but the problem is fundamentally the same. > > > > I don't think so. I'll reiterate my opinion on this subject, as a > > summary and for those who haven't read the discussions of the earlier > > versions of the series. And perhaps I'll even manage to say something > > new =). > > > > > > First about the board specific code. I think we may need some board > > specific code, even if this series was merged. Let's call them board > > drivers. These board drivers would only exist for boards with some weird > > setups that cannot be expressed or managed with DT and normal drivers. > > > > I think these cases would be quite rare, as I can't even come up with a > > very good example. I guess most likely these cases would involve some > > small trivial chips for muxing or such, for which it doesn't really make > > sense to have a real driver. > > > > Say, perhaps a board with two LCDs connected to one video output, and > > only one LCD can be enabled at a time, and you need to set some mux chip > > to route the signals to the correct LCD. In this case I'd see we should > > have hotplug support in the display framework, and the board driver > > would act on user input (sysfs file, perhaps), plugging in/out the LCD > > device depending on the user input. > > > > > > As for expressing device internal details in the DT data, as done in > > this series, I don't like it very much. I think the DT data or the board > > file should just describe which device is the one in question, and how > > it's connected to other components on the board. The driver for the > > device should handle everything else. > > > > As Alex pointed out, there may be lots of devices that work the same way > > when enabled, but require slightly different power-on/off sequences. We > > could have these sequences in the driver itself, either as plain code, > > or in a table of some sort, possibly using the power sequence framework > > presented in this series. The correct code or sequence would be ran > > depending on the particular model of the device. > > > > I think this approach is correct in the sense that this is what drivers > > are supposed to do: handle all the device internal matters. But this > > raises the problem of bloating the kernel with possibly lots of > > different power sequences, of which only a few would be used by a board, > > and all the rest would just be waste of memory. > > > > Regarding this problem I have the following questions, to which I don't > > have clear answers: > > > > - How much of this device specific data is too much? If a driver > > supports 10 different models, and the sequences for each model take 100 > > bytes, this 1000 bytes doesn't sound too much. But where's the limit? > > And even if one driver only has 1kB of this data, what if we have lots > > of similar drivers? > > > > - How many bytes does a power sequence presented in this series take, if > > the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and > > regulator? > > > > - How likely is it that we'll get lots of different models? A hundred > > different models for a backlight PWM with different power-on/off > > sequences already sounds a really big number. If we're only going to > > have each driver supporting, say, no more than 10 models, perhaps the > > problem is not even an issue in practice. > > > > - Are there ways to limit this bloat in the driver? One obvious way > > would be to discard the unused sequences after driver probe, but that > > only works for platform devices. Although, I guess these sequences would > > mostly be used by platform drivers? If so, then the problem could be > > solved by discarding the data after probe. Another would be to load the > > sequences from a file as firmware, but that feels quite an awful > > solution. Anybody have other ideas? > > > > One clear positive side with in-driver approach is that it's totally > > inside the kernel, and can be easily reworked in the future, or even > > changed to a DT-based approach as presented in this series if that seems > > like a better solution. The DT based approach, on the other hand, will > > be more or less written to stone after it's merged. > > > > > > So, I like the framework of expressing the sequences presented in this > > series (except there's a problem with error cases, as I pointed out in > > another post), as it can be used inside the drivers. But I'm not so > > enthusiastic about presenting the sequences in DT. > > > > My suggestion would be to go forward with an in-driver solution, and > > look at the DT based solution later if we are seeing an increasing bloat > > in the drivers. > > Assuming we go with your approach, what's the plan? We're actually > facing this problem right now for Tegra. Basically we have a DRM driver > that can drive the panel, but we're still missing a way to hook up the > backlight and panel enabling code. So we effectively can't support any > of the LVDS devices out there without this series. > > As I understand it, what you propose is similar to what ASoC does. For a > specific board, you'd have to write a driver, presumably for the new > panel/display framework, that provides code to power the panel on and > off. That means we'll have to have a driver for each panel out there > basically, or we'd need to write generic drivers that can be configured > to some degree (via platform data or DT). This is similar to how ASoC > works, where we have a driver that provides support for a specific codec > connected to the Tegra SoC. For the display framework things could be > done in a similar way I suppose, so that Tegra could have one display > driver to handle all aspects of powering on and off the various panels > for the various boards out there. The common display framework (CDF) includes a driver for dumb parallel panels called panel-dpi. It doesn't handle complex power on/off sequences yet as I haven't run into any need for them, but that's definitely something we could add, possibly using Alexandre's code to express the sequences. Similarly to Tomi, my preferences go for including the sequences in drivers. > Obviously, a lot of the code will be similar for other SoCs, but maybe > that's just the way things are if we choose that approach. There's also > the potential for factoring out large chunks of common code later on > once we start to see common patterns. I don't mind duplicating some code in different drivers as a first step. Factoring out common code in a second step is easier (at least for me) than trying to figure out what the common code would look like before the first driver is even written. > One thing that's not very clear is how the backlight subsystem should be > wired up with the display framework. I have a patch on top of the Tegra > DRM driver which adds some ad-hoc display support using this power > sequences series and the pwm-backlight. I've touched the backlight API a bit, from both the provider and consumer points of view, and my feeling is that the API will need to be reworked to neatly integrate with the CDF. The backlight API currently depends on fbdev, that's something we should aim to remove. Proposals are welcome :-) Should we create a separate mail thread for this ? > From reading the proposal for the panel/display framework, it sounds > like a lot more is planned than just enabling or disabling panels, That's correct. CDF implements several other panel control operations (to retrieve the list of modes supported by the panel for instance), and more operations will be needed in the future. I've decided to only implement in the RFC operations that I need for the platforms I work on, and let other developers propose additional operations depending on their use cases. > but it also seems like a lot of code needs to be written to support things > like DSI, DBI or other control busses. I don't think we need that much code there. The most common control busses for display hardware are DSI, DBI, I2C, SPI and plain GPIOs. The last three control busses are well supported in the Linux kernel, I've implemented DBI support as part of the CDF RFC, and DSI support is missing. If you look at the DBI bus implementation the amount of code isn't overly large. > At least for Tegra, and I think the same holds for a wide variety of other > SoCs, dumb panels would be enough for a start. In the interest of getting a > working solution for those setups, maybe we can start small and add just > enough framework to register dumb panel drivers to along with code to wire > up a backlight to light up the display. Then we could possibly still make it > to have a proper solution to support the various LVDS panels for Tegra with > 3.9. > > I'm adding Laurent on Cc since he's probably busy working on a new > proposal for the panel/display framework. Maybe he can share his thought > on this. > > All of the above said, I'm willing to help out with the coding if that's > what is required to reach a solution that everybody can be happy with. That's much appreciated. If that means a couple additional hours of sleep per week for me I'll be extremely thankful :-)
Hi Tomi, On Wednesday 21 November 2012 14:04:17 Tomi Valkeinen wrote: > On 2012-11-21 13:40, Thierry Reding wrote: [snip] > > One thing that's not very clear is how the backlight subsystem should be > > wired up with the display framework. I have a patch on top of the Tegra > > DRM driver which adds some ad-hoc display support using this power > > sequences series and the pwm-backlight. > > I think that's a separate issue: how to associate the lcd device and > backlight device together. I don't have a clear answer to this. > > There are many ways the backlight may be handled. In some cases the > panel and the backlight are truly independent, and you can use the other > without using the other (not very practical, though =). From a control point of view that's always the case for DPI panels (as those panels have no control bus, the backlight control bus is by definition separate) and is the case for the two DBI panels I've seen (but I won't claim that's a significative number of panels). > But then with some LCDs the backlight may be controlled by sending commands > to the panel, and in this case the two may be quite linked. Changing the > backlight requires the panel driver to be up and running, and sometimes the > sending the backlight commands may need to be (say, DSI display, with > backlight commands going over the DSI bus). When you write "sending commands to the panel", do you mean on the same control bus that the panel use ? Or would that also include for instance an I2C backlight controller integrated inside a DSI panel module ? In the later case there might still be dependencies between the panel controller and the backlight controller (let's say for instance that the panel controller has a DSI-controller GPIO wired to the backlight controller reset signal), but in practice I don't know if that's ever the case. > So my feeling is that the panel driver should know about the related > backlight device. In the first case the panel driver would just call > enable/disable in the backlight device when the panel is turned on. That makes sense. Unless I'm mistaken a backlight is always associated with a panel (it would be called a light if there was no panel in front of it). We can thus expose backlight operations in the panel CDF (in-kernel) API. The panel driver would need a way to retrieve a pointer to the associated backlight device. > In the second case of the DSI panel... I'm not sure. I've implemented it > so that the panel driver creates the backlight device, and implements > the backlight callbacks. It then sends the DSI commands from those > callbacks. If we decide to make the panel expose backlight operations we could get rid of the backlight device in this case. > > From reading the proposal for the panel/display framework, it sounds > > like a lot more is planned than just enabling or disabling panels, but > > it also seems like a lot of code needs to be written to support things > > like DSI, DBI or other control busses. > > > > At least for Tegra, and I think the same holds for a wide variety of > > other SoCs, dumb panels would be enough for a start. In the interest of > > getting a working solution for those setups, maybe we can start small > > and add just enough framework to register dumb panel drivers to along > > with code to wire up a backlight to light up the display. Then we could > > possibly still make it to have a proper solution to support the various > > LVDS panels for Tegra with 3.9. > > Yes, we (Laurent and me) both agree that we should start simple. > > However, the common panel framework is not strictly needed for this. I'm > not sure of the current architecture for Tegra, but for OMAP we already > have panel drivers (omap specific ones, though). The panel drivers may > support multiple models, (for example, > drivers/video/omap2/displays/panel-generic-dpi.c). > > I don't see any problem with adding small Tegra specific panel drivers > for the time being, with the intention of converting to common panel > framework when that's available. I'm fine with that, but using the CDF would be even better :-) > Of course, the DT side is an issue. If you now create DT bindings for a > temporary model, and need to change it again later, you'll have some > headaches trying managing that without breaking the old bindings... This > is why I haven't pushed DT bindings for OMAP, as I know I have to change > them in the near future.
Hi Thierry, On Wednesday 21 November 2012 14:00:39 Thierry Reding wrote: > On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote: > > On 2012-11-21 13:40, Thierry Reding wrote: > > > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote: > > (sorry for bouncing back and forth with my private and my @ti addresses. > > I can't find an option in thunderbird to only use one sender address, > > and I always forget to change it when responding...) > > > > >> My suggestion would be to go forward with an in-driver solution, and > > >> look at the DT based solution later if we are seeing an increasing > > >> bloat in the drivers. > > > > > > Assuming we go with your approach, what's the plan? We're actually > > > facing this problem right now for Tegra. Basically we have a DRM driver > > > that can drive the panel, but we're still missing a way to hook up the > > > backlight and panel enabling code. So we effectively can't support any > > > of the LVDS devices out there without this series. > > > > Could you describe the hardware setup you have related to the LCD and > > backlight? Is it a public board with public schematics? > > I don't think any of the schematics are public. The Tamonten Evaluation > Carrier is available publicly from our website and the schematics are > available on demand as well. If required I can probably arrange to send > you a copy. > > > I've understood that you don't have anything special in your board, just > > an LCD and a backlight, and the power sequences are related to powering > > up the LCD and the backlight, without anything board specific. If so, > > there's no need for board specific code, but just improving the panel > > and backlight drivers to support the models you use. > > Correct. Basically we have two GPIOs that each enable the panel or the > backlight respectively and one PWM to control the brightness. Are the > panel drivers that you refer to those in drivers/video? I'm not sure if > adding more ad-hoc drivers there just to move them to a generic > framework in the next cycle is a good idea. I'd rather spend some time > on helping to get the framework right and have drivers for that instead. Thanks :-) It should be pretty easy to add support for an enable GPIO to the DPI panel driver that I've posted as part of the CDF RFC v2. > From what I understand by looking at the OMAP display drivers, they also > provide the timings for the displays. Steffen's videomode helpers can be > used to represent these easily in DT, but I suppose if all of those per- > panel specifics are represented in the drivers then that won't be needed > anymore either. For DPI panels it might still make sense to provide the timings through DT or platform data, as the driver might grow huge otherwise. Panel drivers that support a couple (dozens) of different models could hardcode the timings. > > > As I understand it, what you propose is similar to what ASoC does. For a > > > specific board, you'd have to write a driver, presumably for the new > > > panel/display framework, that provides code to power the panel on and > > > off. That means we'll have to have a driver for each panel out there > > > basically, or we'd need to write generic drivers that can be configured > > > to some degree (via platform data or DT). This is similar to how ASoC > > > works, where we have a driver that provides support for a specific codec > > > connected to the Tegra SoC. For the display framework things could be > > > done in a similar way I suppose, so that Tegra could have one display > > > driver to handle all aspects of powering on and off the various panels > > > for the various boards out there. > > > > I think we should only need the board drivers for very special cases. If > > there's just a panel and a backlight, without any special dynamic muxing > > or other trickery needed, I don't see a need for a board driver. I > > presume this is the case for most of the boards. > > For Tegra ASoC, the way to provide for this is to allow a specific board > to introduce a separate compatible value to enable per-board quirks or > special handling if it cannot be supported by the generic driver and > configuration mechanisms. > > > > Obviously, a lot of the code will be similar for other SoCs, but maybe > > > that's just the way things are if we choose that approach. There's also > > > the potential for factoring out large chunks of common code later on > > > once we start to see common patterns. > > > > > > One thing that's not very clear is how the backlight subsystem should be > > > wired up with the display framework. I have a patch on top of the Tegra > > > DRM driver which adds some ad-hoc display support using this power > > > sequences series and the pwm-backlight. > > > > I think that's a separate issue: how to associate the lcd device and > > backlight device together. I don't have a clear answer to this. > > > > There are many ways the backlight may be handled. In some cases the > > panel and the backlight are truly independent, and you can use the other > > without using the other (not very practical, though =). > > At least for DT I think we can easily wire that up. I've actually posted > a patch recently that does so. I think in most cases it makes sense to > control them together, such as on DPMS changes, where you really want to > turn both the backlight and the LCD off, independent of how they are > tied together. > > > But then with some LCDs the backlight may be controlled by sending > > commands to the panel, and in this case the two may be quite linked. > > Changing the backlight requires the panel driver to be up and running, > > and sometimes the sending the backlight commands may need to be (say, > > DSI display, with backlight commands going over the DSI bus). > > > > So my feeling is that the panel driver should know about the related > > backlight device. In the first case the panel driver would just call > > enable/disable in the backlight device when the panel is turned on. > > Exactly. > > > In the second case of the DSI panel... I'm not sure. I've implemented it > > so that the panel driver creates the backlight device, and implements > > the backlight callbacks. It then sends the DSI commands from those > > callbacks. > > That certainly sounds like the right approach to me. > > > > From reading the proposal for the panel/display framework, it sounds > > > like a lot more is planned than just enabling or disabling panels, but > > > it also seems like a lot of code needs to be written to support things > > > like DSI, DBI or other control busses. > > > > > > At least for Tegra, and I think the same holds for a wide variety of > > > other SoCs, dumb panels would be enough for a start. In the interest of > > > getting a working solution for those setups, maybe we can start small > > > and add just enough framework to register dumb panel drivers to along > > > with code to wire up a backlight to light up the display. Then we could > > > possibly still make it to have a proper solution to support the various > > > LVDS panels for Tegra with 3.9. > > > > Yes, we (Laurent and me) both agree that we should start simple. > > > > However, the common panel framework is not strictly needed for this. I'm > > not sure of the current architecture for Tegra, but for OMAP we already > > have panel drivers (omap specific ones, though). The panel drivers may > > support multiple models, (for example, > > drivers/video/omap2/displays/panel-generic-dpi.c). > > > > I don't see any problem with adding small Tegra specific panel drivers > > for the time being, with the intention of converting to common panel > > framework when that's available. > > I can take a look at how such a driver could be implemented, but again, > I'm a bit reluctant to add something ad-hoc now when maybe we can have > it supported in a proper framework not too far away in the future. > > > Of course, the DT side is an issue. If you now create DT bindings for a > > temporary model, and need to change it again later, you'll have some > > headaches trying managing that without breaking the old bindings... This > > is why I haven't pushed DT bindings for OMAP, as I know I have to change > > them in the near future. > > We're already keeping back on this and none of the patches that define > the bindings have been merged yet. Although bindings have been known to > change every once in a while even for code that is already in mainline.
On 2012-11-27 17:08, Laurent Pinchart wrote: > Hi Tomi, > > On Wednesday 21 November 2012 14:04:17 Tomi Valkeinen wrote: >> On 2012-11-21 13:40, Thierry Reding wrote: > > [snip] > >>> One thing that's not very clear is how the backlight subsystem should be >>> wired up with the display framework. I have a patch on top of the Tegra >>> DRM driver which adds some ad-hoc display support using this power >>> sequences series and the pwm-backlight. >> >> I think that's a separate issue: how to associate the lcd device and >> backlight device together. I don't have a clear answer to this. >> >> There are many ways the backlight may be handled. In some cases the >> panel and the backlight are truly independent, and you can use the other >> without using the other (not very practical, though =). > > From a control point of view that's always the case for DPI panels (as those > panels have no control bus, the backlight control bus is by definition > separate) and is the case for the two DBI panels I've seen (but I won't claim > that's a significative number of panels). They may have a control bus, I2C, SPI, etc. In some cases that can be used to control the backlight. But yes, it's separate from the video bus. >> But then with some LCDs the backlight may be controlled by sending commands >> to the panel, and in this case the two may be quite linked. Changing the >> backlight requires the panel driver to be up and running, and sometimes the >> sending the backlight commands may need to be (say, DSI display, with >> backlight commands going over the DSI bus). > > When you write "sending commands to the panel", do you mean on the same > control bus that the panel use ? Or would that also include for instance an > I2C backlight controller integrated inside a DSI panel module ? In the later I mean the same control bus that is used to control the panel, be it shared with video bus like DSI, or separate like I2C. > case there might still be dependencies between the panel controller and the > backlight controller (let's say for instance that the panel controller has a > DSI-controller GPIO wired to the backlight controller reset signal), but in > practice I don't know if that's ever the case. > >> So my feeling is that the panel driver should know about the related >> backlight device. In the first case the panel driver would just call >> enable/disable in the backlight device when the panel is turned on. > > That makes sense. Unless I'm mistaken a backlight is always associated with a > panel (it would be called a light if there was no panel in front of it). We > can thus expose backlight operations in the panel CDF (in-kernel) API. The > panel driver would need a way to retrieve a pointer to the associated > backlight device. I agree. >> In the second case of the DSI panel... I'm not sure. I've implemented it >> so that the panel driver creates the backlight device, and implements >> the backlight callbacks. It then sends the DSI commands from those >> callbacks. > > If we decide to make the panel expose backlight operations we could get rid of > the backlight device in this case. Do you mean there would be a real backlight device only when there's a totally independent backlight for the panel? Tomi
Hi Tomi, On Tuesday 27 November 2012 17:19:05 Tomi Valkeinen wrote: > On 2012-11-27 17:08, Laurent Pinchart wrote: > > On Wednesday 21 November 2012 14:04:17 Tomi Valkeinen wrote: > >> On 2012-11-21 13:40, Thierry Reding wrote: > > [snip] > > > >>> One thing that's not very clear is how the backlight subsystem should be > >>> wired up with the display framework. I have a patch on top of the Tegra > >>> DRM driver which adds some ad-hoc display support using this power > >>> sequences series and the pwm-backlight. > >> > >> I think that's a separate issue: how to associate the lcd device and > >> backlight device together. I don't have a clear answer to this. > >> > >> There are many ways the backlight may be handled. In some cases the > >> panel and the backlight are truly independent, and you can use the other > >> without using the other (not very practical, though =). > > > > From a control point of view that's always the case for DPI panels (as > > those panels have no control bus, the backlight control bus is by > > definition separate) and is the case for the two DBI panels I've seen > > (but I won't claim that's a significative number of panels). > > They may have a control bus, I2C, SPI, etc. In some cases that can be > used to control the backlight. But yes, it's separate from the video bus. > > >> But then with some LCDs the backlight may be controlled by sending > >> commands to the panel, and in this case the two may be quite linked. > >> Changing the backlight requires the panel driver to be up and running, > >> and sometimes the sending the backlight commands may need to be (say, DSI > >> display, with backlight commands going over the DSI bus). > > > > When you write "sending commands to the panel", do you mean on the same > > control bus that the panel use ? Or would that also include for instance > > an I2C backlight controller integrated inside a DSI panel module ? In the > > later > > I mean the same control bus that is used to control the panel, be it shared > with video bus like DSI, or separate like I2C. > > > case there might still be dependencies between the panel controller and > > the backlight controller (let's say for instance that the panel controller > > has a DSI-controller GPIO wired to the backlight controller reset signal), > > but in practice I don't know if that's ever the case. > > > >> So my feeling is that the panel driver should know about the related > >> backlight device. In the first case the panel driver would just call > >> enable/disable in the backlight device when the panel is turned on. > > > > That makes sense. Unless I'm mistaken a backlight is always associated > > with a panel (it would be called a light if there was no panel in front > > of it). We can thus expose backlight operations in the panel CDF > > (in-kernel) API. The panel driver would need a way to retrieve a pointer > > to the associated backlight device. > > I agree. > > >> In the second case of the DSI panel... I'm not sure. I've implemented it > >> so that the panel driver creates the backlight device, and implements > >> the backlight callbacks. It then sends the DSI commands from those > >> callbacks. > > > > If we decide to make the panel expose backlight operations we could get > > rid of the backlight device in this case. > > Do you mean there would be a real backlight device only when there's a > totally independent backlight for the panel? I'm toying with that idea. I see two reasons for having backlight devices in the kernel right now. - Integration with the fbdev API to control the backlight automatically on fbdev blank/unblank/suspend/resume. That's something I want to get rid of, backlight should not depend on fbdev. If the backlight in-kernel API is exposed by the panel through CDF operations we won't need this anymore. - Exposing backlight control to userspace in sysfs. That's quite probably something we want to keep, if only for compatibility reasons. Backlight on/off should be controlled through the display APIs through DPMS control, but we have no way that I'm aware of to control the backlight brightness in the FBDEV and KMS APIs. It might make sense to add a backlight API to KMS for the future. One possibly crazy idea I had was to replace backlight devices as we know them with LED devices (a LED driver IC shouldn't be supported by different APIs depending on whether the LEDs it drives are used as a backlight or not), and have the panel hook up with the associated LED device to expose backlight operations through the CDF. Panels with a backlight controller tied to the panel controller (control through DSI for instance) would implement the backlight operations directly without instantiating a LED device. This still leaves the question of the userspace backlight brightness API open.
On Tue, Nov 27, 2012 at 04:37:32PM +0100, Laurent Pinchart wrote: > One possibly crazy idea I had was to replace backlight devices as we know them > with LED devices (a LED driver IC shouldn't be supported by different APIs > depending on whether the LEDs it drives are used as a backlight or not), and > have the panel hook up with the associated LED device to expose backlight > operations through the CDF. Panels with a backlight controller tied to the > panel controller (control through DSI for instance) would implement the > backlight operations directly without instantiating a LED device. This still > leaves the question of the userspace backlight brightness API open. I remember looking at that before but never actually got round to it, it seems like a sensible idea to me.
diff --git a/Documentation/devicetree/bindings/power/power_seq.txt b/Documentation/devicetree/bindings/power/power_seq.txt new file mode 100644 index 0000000..7880a6c --- /dev/null +++ b/Documentation/devicetree/bindings/power/power_seq.txt @@ -0,0 +1,121 @@ +Runtime Interpreted Power Sequences +=================================== + +Power sequences are sequential descriptions of actions to be performed on +power-related resources. Having these descriptions in a well-defined data format +allows us to take much of the board- or device- specific power control code out +of the kernel and place it into the device tree instead, making kernels less +board-dependant. + +A device typically makes use of multiple power sequences, for different purposes +such as powering on and off. All the power sequences of a given device are +grouped into a set. In the device tree, this set is a sub-node of the device +node named "power-sequences". + +Power Sequences Structure +------------------------- +Every device that makes use of power sequences must have a "power-sequences" +node into which individual power sequences are declared as sub-nodes. The name +of the node becomes the name of the sequence within the power sequences +framework. + +Similarly, each power sequence declares its steps as sub-nodes of itself. Steps +must be named sequentially, with the first step named step0, the second step1, +etc. Failure to follow this rule will result in a parsing error. + +Power Sequences Steps +--------------------- +Steps of a sequence describe an action to be performed on a resource. They +always include a "type" property which indicates what kind of resource this +step works on. Depending on the resource type, additional properties are defined +to control the action to be performed. + +"delay" type required properties: + - delay: delay to wait (in microseconds) + +"regulator" type required properties: + - id: name of the regulator to use. + - enable / disable: one of these two empty properties must be present to + enable or disable the resource + +"pwm" type required properties: + - id: name of the PWM to use. + - enable / disable: one of these two empty properties must be present to + enable or disable the resource + +"gpio" type required properties: + - gpio: phandle of the GPIO to use. + - value: value this GPIO should take. Must be 0 or 1. + +Example +------- +Here are example sequences declared within a backlight device that use all the +supported resources types: + + backlight { + compatible = "pwm-backlight"; + ... + + /* resources used by the power sequences */ + pwms = <&pwm 2 5000000>; + pwm-names = "backlight"; + power-supply = <&backlight_reg>; + + power-sequences { + power-on { + step0 { + type = "regulator"; + id = "power"; + enable; + }; + step1 { + type = "delay"; + delay = <10000>; + }; + step2 { + type = "pwm"; + id = "backlight"; + enable; + }; + step3 { + type = "gpio"; + gpio = <&gpio 28 0>; + value = <1>; + }; + }; + + power-off { + step0 { + type = "gpio"; + gpio = <&gpio 28 0>; + value = <0>; + }; + step1 { + type = "pwm"; + id = "backlight"; + disable; + }; + step2 { + type = "delay"; + delay = <10000>; + }; + step3 { + type = "regulator"; + id = "power"; + disable; + }; + }; + }; + }; + +The first part lists the PWM and regulator resources used by the sequences. +These resources will be requested on behalf of the backlight device when the +sequences are built and are declared according to their own bindings (for +instance, regulators and pwms are resolved by name - note though that name +declaration is done differently by the two frameworks). + +After the resources declaration, two sequences follow for powering the backlight +on and off. Their names are specified by the pwm-backlight device bindings. Once +the sequences are built by calling devm_of_parse_power_seq_set() on the +backlight device, they can be added to a set using +power_seq_set_add_sequences(). diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt new file mode 100644 index 0000000..8be0570 --- /dev/null +++ b/Documentation/power/power_seq.txt @@ -0,0 +1,253 @@ +Runtime Interpreted Power Sequences +=================================== + +Problem +------- +Very commonly, boards need the help of out-of-driver code to turn some of their +devices on and off. For instance, SoC boards might use a GPIO (abstracted to a +regulator or not) to control the power supply of a backlight. The GPIO that +should be used, however, as well as the exact power sequence that may also +involve other resources, is board-dependent and thus unknown to the driver. + +This was previously addressed by having hooks in the device's platform data that +are called whenever the state of the device might need a power status 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 lightweight interpreter whenever +needed. This allows device drivers to work without power callbacks and makes the +kernel less board-dependant. + +What are Power Sequences? +------------------------- +A power sequence is an array of sequential steps describing an action to be +performed on a resource. The supported resources and actions operations are: +- delay (just wait for a given number of microseconds) +- GPIO (set to 0 or 1) +- regulator (enable or disable) +- PWM (enable or disable) + +When a power sequence is run, its steps is executed one after the other until +one step fails or the end of the sequence is reached. + +Power sequences are named, and grouped into "sets" which contain all the +sequences of a device as well as the resources they use. + +Power sequences can be declared as platform data or in the device tree. + +Platform Data Format +-------------------- +All relevant data structures for declaring power sequences are located in +include/linux/power_seq.h. + +The platform data for a device may include an instance of platform_power_seq_set +which references all the power sequences used for a device. The power sequences +reference resources in their steps, and setup the union member that corresponds +to the resource's type. Resources, similarly, have a union which relevant member +depends on their type. + +Note that the only "platform data" per se here is platform_power_seq_set. Other +structures (power_seq and power_seq_resource) will be used at runtime and thus +*must* survive initialization, so do not declare them with the __initdata +attribute. + +The following example should make it clear how the platform data for power +sequences is defined. It declares two power sequences named "power-on" and +"power-off" for a backlight device. The "power-on" sequence enables the "power" +regulator of the device, waits for 10ms, and then enables PWM "backlight" and +set GPIO 28 to 1. "power-off" does the opposite. + +struct power_seq_resource reg_res = { + .type = POWER_SEQ_REGULATOR, + .regulator.id = "power", +}; + +struct power_seq_resource gpio_res = { + .type = POWER_SEQ_GPIO, + .gpio.gpio = 28, +}; + +struct power_seq_resource pwm_res = { + .type = POWER_SEQ_PWM, + .pwm.id = "backlight", +}; + +struct power_seq_resource delay_res = { + .type = POWER_SEQ_DELAY, +}; + +struct power_seq power_on_seq = { + .id = "power-on", + .num_steps = 4, + .steps = { + { + .resource = ®_res, + .regulator.enable = true, + }, { + .resource = &delay_res, + .delay.delay = 10000, + }, { + .resource = &pwm_res, + .pwm.enable = true, + }, { + .resource = &gpio_res, + .gpio.value = 1, + }, + }, +}; + +struct power_seq power_off_seq = { + .id = "power-off", + .num_steps = 4, + .steps = { + { + .resource = &gpio_res, + .gpio.value = 0, + }, { + .resource = &pwm_res, + .pwm.enable = false, + }, { + .resource = &delay_res, + .delay.delay = 10000, + }, { + .resource = ®_res, + .regulator.enable = false, + }, + }, +}; + +struct platform_power_seq_set backlight_power_seqs __initdata = { + .num_seqs = 2, + .seqs = { + &power_on_seq, + &power_off_seq, + }, +}; + +"backlight_power_seqs" can then be passed to power_seq_set_add_sequences() in +order to add the sequences to a set and allocate all the necessary resources. +More on this later in this document. + +Device Tree +----------- +Power sequences can also be encoded as device tree nodes. The following +properties and nodes are equivalent to the platform data defined previously: + +pwms = <&pwm 2 5000000>; +pwm-names = "backlight"; +power-supply = <&vdd_bl_reg>; + +power-sequences { + power-on { + step0 { + type = "regulator"; + id = "power"; + enable; + }; + step1 { + type = "delay"; + delay = <10000>; + }; + step2 { + type = "pwm"; + id = "backlight"; + enable; + }; + step3 { + type = "gpio"; + gpio = <&gpio 28 0>; + value = <1>; + }; + }; + + power-off { + step0 { + type = "gpio"; + gpio = <&gpio 28 0>; + value = <0>; + }; + step1 { + type = "pwm"; + id = "backlight"; + disable; + }; + step2 { + type = "delay"; + delay = <10000>; + }; + step3 { + type = "regulator"; + id = "power"; + disable; + }; + }; +}; + +See Documentation/devicetree/bindings/power/power_seq.txt for the complete +syntax of the DT bindings. + +Use by Drivers and Resources Management +--------------------------------------- +Power sequences make use of resources that must be properly allocated and +managed. The power_seq_set structure manages the sequences and resources for a +particular device. A driver willing to use power sequences will thus declare one +instance of power_seq_set per device and initialize it at probe time: + +struct my_device_data { + struct device *dev; + ... + struct power_set_set power_seqs; + ... +}; + +power_seq_set_init(&my_device->power_seqs, my_device->dev); + +The power_seq_set_add_sequence() and power_seq_set_add_sequences() functions are +then used to add one or several sequences to a set. These functions will also +allocate all the resources used by the sequence(s) and make sure they are ready +to be run. All resources are allocated through devm and will thus be freed when +the set's device is removed. + + int power_seq_set_add_sequence(struct power_seq_set *set, + struct power_seq *seq); + int power_seq_set_add_sequences(struct power_seq_set *set, + struct platform_power_seq_set *seqs); + +Power sequences added to a set can then be resolved by their name using +power_seq_lookup(): + + struct power_seq *power_seq_lookup(struct power_seq_set *seqs, + const char *id); + +power_seq_lookup() returns a ready-to-run pointer to the power sequence which +name matches the id parameter. + +A retrieved power sequence can then be executed by power_seq_run: + + int power_seq_run(struct power_seq *seq); + +It returns 0 if the sequence has successfully been run, or an error code if a +problem occurred. + +Sometimes, you may want to browse the list of resources allocated for the +sequences of a device, for instance to ensure that a resource of a given type is +present. The power_seq_for_each_resource() macro does this: + + power_seq_for_each_resource(pos, seqs) + +Here "pos" will be a pointer to a struct power_seq_resource. This structure +contains the type of the resource, the information used for identifying it, and +the resolved resource itself. + +Finally, users of the device tree can obtain a platform_power_seq_set structure +built from the device's node using devm_of_parse_power_seq_set: + + struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev); + +The power sequences must be declared under a "power-sequences" node directly +declared under the device's node. Detailed syntax contained in Documentation/devicetree/bindings/power/power_seq.txt. As the function name +states, all memory is allocated through devm. The returned +platform_power_seq_set can be freed after being added to a set, but the +sequences themselves must be preserved until they are freed by devm. \ No newline at end of file diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index 49a8939..f20d449 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -338,3 +338,4 @@ config AB8500_BATTERY_THERM_ON_BATCTRL endif # POWER_SUPPLY source "drivers/power/avs/Kconfig" +source "drivers/power/power_seq/Kconfig" diff --git a/drivers/power/Makefile b/drivers/power/Makefile index b949cf8..883ad4d 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -49,3 +49,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o obj-$(CONFIG_POWER_AVS) += avs/ obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o +obj-$(CONFIG_POWER_SEQ) += power_seq/ diff --git a/drivers/power/power_seq/Kconfig b/drivers/power/power_seq/Kconfig new file mode 100644 index 0000000..0ece819 --- /dev/null +++ b/drivers/power/power_seq/Kconfig @@ -0,0 +1,2 @@ +config POWER_SEQ + tristate diff --git a/drivers/power/power_seq/Makefile b/drivers/power/power_seq/Makefile new file mode 100644 index 0000000..964b91e --- /dev/null +++ b/drivers/power/power_seq/Makefile @@ -0,0 +1,2 @@ +obj-$(CONFIG_POWER_SEQ) += power_seq.o +power_seq-y := core.o delay.o regulator.o gpio.o pwm.o diff --git a/drivers/power/power_seq/core.c b/drivers/power/power_seq/core.c new file mode 100644 index 0000000..d51b9b8 --- /dev/null +++ b/drivers/power/power_seq/core.c @@ -0,0 +1,362 @@ +/* + * core.c - 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. + * + */ + +#include <linux/power_seq.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/device.h> +#include <linux/of.h> + +#include "power_seq_priv.h" + +static const struct power_seq_res_ops *power_seq_ops[POWER_SEQ_NUM_TYPES]; + +#ifdef CONFIG_OF +int of_power_seq_parse_enable_properties(struct device_node *node, + struct power_seq *seq, + unsigned int step_nbr, bool *enable) +{ + if (of_find_property(node, "enable", NULL)) { + *enable = true; + } else if (of_find_property(node, "disable", NULL)) { + *enable = false; + } else { + power_seq_err(seq, step_nbr, + "missing enable or disable property\n"); + return -EINVAL; + } + + return 0; +} + +static int of_power_seq_parse_step(struct device *dev, + struct device_node *node, + struct power_seq *seq, + unsigned int step_nbr, + struct list_head *resources) +{ + struct power_seq_step *step = &seq->steps[step_nbr]; + struct power_seq_resource res, *res2; + const char *type; + unsigned int i; + int err; + + err = of_property_read_string(node, "type", &type); + if (err < 0) { + power_seq_err(seq, step_nbr, "cannot read type property\n"); + return err; + } + for (i = 0; i < POWER_SEQ_NUM_TYPES; i++) { + if (power_seq_ops[i]->name == NULL) + continue; + if (!strcmp(type, power_seq_ops[i]->name)) + break; + } + if (i >= POWER_SEQ_NUM_TYPES) { + power_seq_err(seq, step_nbr, "unknown type %s\n", type); + return -EINVAL; + } + memset(&res, 0, sizeof(res)); + res.type = i; + err = power_seq_ops[res.type]->of_parse(node, seq, step_nbr, &res); + if (err < 0) + return err; + + /* Use the same instance of the resource if met before */ + list_for_each_entry(res2, resources, list) { + if (res.type == res2->type && + power_seq_ops[res.type]->res_compare(&res, res2)) + break; + } + /* Resource never met before, create it */ + if (&res2->list == resources) { + res2 = devm_kzalloc(dev, sizeof(*res2), GFP_KERNEL); + if (!res2) + return -ENOMEM; + memcpy(res2, &res, sizeof(res)); + list_add_tail(&res2->list, resources); + } + step->resource = res2; + + return 0; +} + +static struct power_seq *of_parse_power_seq(struct device *dev, + struct device_node *node, + struct list_head *resources) +{ + struct device_node *child = NULL; + struct power_seq *pseq; + unsigned int sz; + int num_steps; + int err; + + if (!node) + return ERR_PTR(-EINVAL); + + num_steps = of_get_child_count(node); + sz = sizeof(*pseq) + sizeof(pseq->steps[0]) * num_steps; + pseq = devm_kzalloc(dev, sz, GFP_KERNEL); + if (!pseq) + return ERR_PTR(-ENOMEM); + pseq->id = node->name; + pseq->num_steps = num_steps; + + for_each_child_of_node(node, child) { + unsigned int pos; + + /* Check that the name's format is correct and within bounds */ + if (strncmp("step", child->name, 4)) { + err = -EINVAL; + goto parse_error; + } + + err = kstrtouint(child->name + 4, 10, &pos); + if (err < 0) + goto parse_error; + + /* Invalid step index or step already parsed? */ + if (pos >= num_steps || pseq->steps[pos].resource != NULL) { + err = -EINVAL; + goto parse_error; + } + + err = of_power_seq_parse_step(dev, child, pseq, pos, resources); + if (err) + return ERR_PTR(err); + } + + return pseq; + +parse_error: + dev_err(dev, "%s: invalid power step name %s!\n", pseq->id, + child->name); + return ERR_PTR(err); +} + +/** + * devm_of_parse_power_seq_set - build a power_seq_set from the device tree + * @dev: Device to parse the power sequences of + * + * Sequences must be contained into a subnode named "power-sequences" of the + * device root node. + * + * Memory for the sequence is allocated using devm_kzalloc on dev. The returned + * platform_power_seq_set can be freed by devm_kfree after the sequences have + * been added, but the sequences themselves must be preserved. + * + * Returns the built set on success, or an error code in case of failure. + */ +struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev) +{ + struct platform_power_seq_set *set; + struct device_node *root = dev->of_node; + struct device_node *seq; + struct list_head resources; + unsigned int sz; + int n; + + if (!root) + return NULL; + + root = of_find_node_by_name(root, "power-sequences"); + if (!root) + return NULL; + + n = of_get_child_count(root); + sz = sizeof(*set) + sizeof(struct power_seq *) * n; + set = devm_kzalloc(dev, sz, GFP_KERNEL); + if (!set) + return ERR_PTR(-ENOMEM); + set->num_seqs = n; + + n = 0; + INIT_LIST_HEAD(&resources); + for_each_child_of_node(root, seq) { + struct power_seq *pseq; + + pseq = of_parse_power_seq(dev, seq, &resources); + if (IS_ERR(pseq)) + return (void *)pseq; + + set->seqs[n++] = pseq; + } + + return set; +} +EXPORT_SYMBOL_GPL(devm_of_parse_power_seq_set); +#endif /* CONFIG_OF */ + +/** + * power_seq_set_init - initialize a power_seq_set + * @set: Set to initialize + * @dev: Device this set is going to belong to + */ +void power_seq_set_init(struct power_seq_set *set, struct device *dev) +{ + set->dev = dev; + INIT_LIST_HEAD(&set->resources); + INIT_LIST_HEAD(&set->seqs); +} +EXPORT_SYMBOL_GPL(power_seq_set_init); + +/** + * power_seq_add_sequence - add a power sequence to a set + * @set: Set to add the sequence to + * @seq: Sequence to add + * + * This step will check that all the resources used by the sequence are + * allocated. If they are not, an attempt to allocate them is made. This + * operation can fail and and return an error code. + * + * Returns 0 on success, error code if a resource initialization failed. + */ +int power_seq_add_sequence(struct power_seq_set *set, struct power_seq *seq) +{ + struct power_seq_resource *res; + unsigned int i; + int err; + + for (i = 0; i < seq->num_steps; i++) { + struct power_seq_step *step = &seq->steps[i]; + struct power_seq_resource *step_res = step->resource; + + /* support for resource type not compiled in? */ + if (power_seq_ops[step_res->type]->name == NULL) { + power_seq_err(seq, i, "unsupported resource type\n"); + return -EINVAL; + } + + /* resource already allocated from a previous step ? */ + list_for_each_entry(res, &set->resources, list) { + if (res == step_res) + break; + } + /* resource not allocated yet, allocate and add it */ + if (&res->list == &set->resources) { + err = power_seq_ops[step_res->type]->res_alloc(set->dev, + step_res); + if (err) + return err; + list_add_tail(&step->resource->list, &set->resources); + } + } + + list_add_tail(&seq->list, &set->seqs); + seq->set = set; + + return 0; +} +EXPORT_SYMBOL_GPL(power_seq_add_sequence); + +/** + * power_seq_add_sequences - add power sequences defined as platform data + * @set: Set to add the sequences to + * @seqs: Sequences to add + * + * See power_seq_add_sequence for more details. + * + * Returns 0 on success, error code if a resource initialization failed. + */ +int power_seq_set_add_sequences(struct power_seq_set *set, + struct platform_power_seq_set *seqs) +{ + unsigned int i; + int ret; + + for (i = 0; i < seqs->num_seqs; i++) { + ret = power_seq_add_sequence(set, seqs->seqs[i]); + if (ret < 0) + return ret; + } + + return 0; +} +EXPORT_SYMBOL_GPL(power_seq_set_add_sequences); + +/** + * power_seq_lookup - Lookup a power sequence by name from a set + * @seqs: The set to look in + * @id: Name to look after + * + * Returns a matching power sequence if it exists, NULL if it does not. + */ +struct power_seq *power_seq_lookup(struct power_seq_set *set, const char *id) +{ + struct power_seq *seq; + + list_for_each_entry(seq, &set->seqs, list) { + if (!strcmp(seq->id, id)) + return seq; + } + + return NULL; +} +EXPORT_SYMBOL_GPL(power_seq_lookup); + +/** + * power_seq_run() - run a power sequence + * @seq: The power sequence to run + * + * Returns 0 on success, error code in case of failure. + */ +int power_seq_run(struct power_seq *seq) +{ + unsigned int i; + int err; + + if (!seq) + return 0; + + if (!seq->set) { + pr_err("cannot run a sequence not added to a set"); + return -EINVAL; + } + + for (i = 0; i < seq->num_steps; i++) { + unsigned int type = seq->steps[i].resource->type; + + err = power_seq_ops[type]->step_run(&seq->steps[i]); + if (err) { + power_seq_err(seq, i, + "error %d while running power sequence step\n", + err); + return err; + } + } + + return 0; +} +EXPORT_SYMBOL_GPL(power_seq_run); + +/* defined in power_seq_*.c files */ +extern const struct power_seq_res_ops power_seq_delay_ops; +extern const struct power_seq_res_ops power_seq_gpio_ops; +extern const struct power_seq_res_ops power_seq_regulator_ops; +extern const struct power_seq_res_ops power_seq_pwm_ops; + +static const struct power_seq_res_ops *power_seq_ops[POWER_SEQ_NUM_TYPES] = { + [POWER_SEQ_DELAY] = &power_seq_delay_ops, + [POWER_SEQ_REGULATOR] = &power_seq_regulator_ops, + [POWER_SEQ_PWM] = &power_seq_gpio_ops, + [POWER_SEQ_GPIO] = &power_seq_pwm_ops, +}; + +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>"); +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/power/power_seq/delay.c b/drivers/power/power_seq/delay.c new file mode 100644 index 0000000..de6810b --- /dev/null +++ b/drivers/power/power_seq/delay.c @@ -0,0 +1,66 @@ +/* + * 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. + * + */ + +#include "power_seq_priv.h" +#include <linux/delay.h> + +#ifdef CONFIG_OF +static int of_power_seq_parse_delay(struct device_node *node, + struct power_seq *seq, + unsigned int step_nbr, + struct power_seq_resource *res) +{ + struct power_seq_step *step = &seq->steps[step_nbr]; + int err; + + err = of_property_read_u32(node, "delay", + &step->delay.delay); + if (err < 0) + power_seq_err(seq, step_nbr, "error reading delay property\n"); + + return err; +} +#else +#define of_power_seq_parse_delay NULL +#endif + +static bool power_seq_res_compare_delay(struct power_seq_resource *res, + struct power_seq_resource *res2) +{ + /* Delay resources are just here to hold the type of steps, so they are + * all equivalent. */ + return true; +} + +static int power_seq_res_alloc_delay(struct device *dev, + struct power_seq_resource *res) +{ + return 0; +} + +static int power_seq_step_run_delay(struct power_seq_step *step) +{ + usleep_range(step->delay.delay, + step->delay.delay + 1000); + + return 0; +} + +const struct power_seq_res_ops power_seq_delay_ops = { + .name = "delay", + .of_parse = of_power_seq_parse_delay, + .step_run = power_seq_step_run_delay, + .res_compare = power_seq_res_compare_delay, + .res_alloc = power_seq_res_alloc_delay, +}; diff --git a/drivers/power/power_seq/gpio.c b/drivers/power/power_seq/gpio.c new file mode 100644 index 0000000..0cd143a --- /dev/null +++ b/drivers/power/power_seq/gpio.c @@ -0,0 +1,95 @@ +/* + * 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. + * + */ + +#include "power_seq_priv.h" +#include <linux/gpio.h> +#include <linux/of_gpio.h> + +#ifdef CONFIG_OF +static int of_power_seq_parse_gpio(struct device_node *node, + struct power_seq *seq, + unsigned int step_nbr, + struct power_seq_resource *res) +{ + struct power_seq_step *step = &seq->steps[step_nbr]; + int gpio; + int err; + + gpio = of_get_named_gpio(node, "gpio", 0); + if (gpio < 0) { + power_seq_err(seq, step_nbr, "error reading gpio property\n"); + return gpio; + } + res->gpio.gpio = gpio; + + err = of_property_read_u32(node, "value", &step->gpio.value); + if (err < 0) { + power_seq_err(seq, step_nbr, "error reading value property\n"); + } else if (step->gpio.value < 0 || step->gpio.value > 1) { + power_seq_err(seq, step_nbr, + "value out of range (must be 0 or 1)\n"); + err = -EINVAL; + } + + return err; +} +#else +#define of_power_seq_parse_gpio NULL +#endif + +static bool power_seq_res_compare_gpio(struct power_seq_resource *res, + struct power_seq_resource *res2) +{ + return res->gpio.gpio == res2->gpio.gpio; +} + +static int power_seq_res_alloc_gpio(struct device *dev, + struct power_seq_resource *res) +{ + int err; + + err = devm_gpio_request(dev, res->gpio.gpio, dev_name(dev)); + if (err) { + dev_err(dev, "cannot get gpio %d\n", res->gpio.gpio); + return err; + } + + return 0; +} + +static int power_seq_step_run_gpio(struct power_seq_step *step) +{ + struct power_seq_resource *res = step->resource; + + /* set the GPIO direction at first use */ + if (!res->gpio.is_set) { + int err = gpio_direction_output(res->gpio.gpio, + step->gpio.value); + if (err) + return err; + res->gpio.is_set = true; + } else { + gpio_set_value_cansleep(res->gpio.gpio, step->gpio.value); + } + + return 0; +} + +const struct power_seq_res_ops power_seq_gpio_ops = { + .name = "gpio", + .of_parse = of_power_seq_parse_gpio, + .step_run = power_seq_step_run_gpio, + .res_compare = power_seq_res_compare_gpio, + .res_alloc = power_seq_res_alloc_gpio, +}; diff --git a/drivers/power/power_seq/power_seq_priv.h b/drivers/power/power_seq/power_seq_priv.h new file mode 100644 index 0000000..fa56e21 --- /dev/null +++ b/drivers/power/power_seq/power_seq_priv.h @@ -0,0 +1,56 @@ +/* + * 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. + * + */ + +#ifndef POWER_SEQ_PRIV_H +#define POWER_SEQ_PRIV_H + +#include <linux/device.h> +#include <linux/power_seq.h> +#include <linux/of.h> + +#define power_seq_err(seq, step_nbr, format, ...) \ + dev_err(seq->set->dev, "%s[%d]: " format, seq->id, step_nbr, \ + ##__VA_ARGS__); + +#ifdef CONFIG_OF +int of_power_seq_parse_enable_properties(struct device_node *node, + struct power_seq *seq, + unsigned int step_nbr, bool *enable); +#endif + +/** + * struct power_seq_res_ops - operators for power sequences resources + * @name: Name of the resource type. Set to null when a resource + * type support is not compiled in + * @of_parse: Parse a step for this kind of resource from a device + * tree node. The result of parsing must be written into + * step step_nbr of seq + * @step_run: Run a step for this kind of resource + * @res_compare: Return true if the resource used by the resource is the + * same as the one referenced by the step, false otherwise. + * @res_alloc: Resolve and allocate a resource. Return error code if + * the resource cannot be allocated, 0 otherwise + */ +struct power_seq_res_ops { + const char *name; + int (*of_parse)(struct device_node *node, struct power_seq *seq, + unsigned int step_nbr, struct power_seq_resource *res); + int (*step_run)(struct power_seq_step *step); + bool (*res_compare)(struct power_seq_resource *res, + struct power_seq_resource *res2); + int (*res_alloc)(struct device *dev, + struct power_seq_resource *res); +}; + +#endif diff --git a/drivers/power/power_seq/pwm.c b/drivers/power/power_seq/pwm.c new file mode 100644 index 0000000..a74b768 --- /dev/null +++ b/drivers/power/power_seq/pwm.c @@ -0,0 +1,85 @@ +/* + * 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. + * + */ + +#include "power_seq_priv.h" + +#ifdef CONFIG_PWM + +#include <linux/pwm.h> + +#ifdef CONFIG_OF +static int of_power_seq_parse_pwm(struct device_node *node, + struct power_seq *seq, + unsigned int step_nbr, + struct power_seq_resource *res) +{ + struct power_seq_step *step = &seq->steps[step_nbr]; + int err; + + err = of_property_read_string(node, "id", &res->pwm.id); + if (err) { + power_seq_err(seq, step_nbr, "error reading id property\n"); + return err; + } + + err = of_power_seq_parse_enable_properties(node, seq, step_nbr, + &step->pwm.enable); + return err; +} +#else +#define of_power_seq_parse_pwm NULL +#endif + +static bool power_seq_res_compare_pwm(struct power_seq_resource *res, + struct power_seq_resource *res2) +{ + return !strcmp(res->pwm.id, res2->pwm.id); +} + +static int power_seq_res_alloc_pwm(struct device *dev, + struct power_seq_resource *res) +{ + res->pwm.pwm = devm_pwm_get(dev, res->pwm.id); + if (IS_ERR(res->pwm.pwm)) { + dev_err(dev, "cannot get pwm \"%s\"\n", res->pwm.id); + return PTR_ERR(res->pwm.pwm); + } + + return 0; +} + +static int power_seq_step_run_pwm(struct power_seq_step *step) +{ + if (step->pwm.enable) { + return pwm_enable(step->resource->pwm.pwm); + } else { + pwm_disable(step->resource->pwm.pwm); + return 0; + } +} + +const struct power_seq_res_ops power_seq_pwm_ops = { + .name = "pwm", + .of_parse = of_power_seq_parse_pwm, + .step_run = power_seq_step_run_pwm, + .res_compare = power_seq_res_compare_pwm, + .res_alloc = power_seq_res_alloc_pwm, +}; + +#else + +const struct power_seq_res_ops power_seq_pwm_ops = { +}; + +#endif diff --git a/drivers/power/power_seq/regulator.c b/drivers/power/power_seq/regulator.c new file mode 100644 index 0000000..4663dbc --- /dev/null +++ b/drivers/power/power_seq/regulator.c @@ -0,0 +1,87 @@ +/* + * 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. + * + */ + +#include "power_seq_priv.h" + +#ifdef CONFIG_REGULATOR + +#include <linux/err.h> +#include <linux/regulator/consumer.h> + +#ifdef CONFIG_OF +static int of_power_seq_parse_regulator(struct device_node *node, + struct power_seq *seq, + unsigned int step_nbr, + struct power_seq_resource *res) +{ + struct power_seq_step *step = &seq->steps[step_nbr]; + int err; + + err = of_property_read_string(node, "id", + &res->regulator.id); + if (err) { + power_seq_err(seq, step_nbr, "error reading id property\n"); + return err; + } + + err = of_power_seq_parse_enable_properties(node, seq, step_nbr, + &step->regulator.enable); + return err; +} +#else +#define of_power_seq_parse_regulator NULL +#endif + +static bool +power_seq_res_compare_regulator(struct power_seq_resource *res, + struct power_seq_resource *res2) +{ + return !strcmp(res->regulator.id, res2->regulator.id); +} + +static int power_seq_res_alloc_regulator(struct device *dev, + struct power_seq_resource *res) +{ + res->regulator.regulator = devm_regulator_get(dev, res->regulator.id); + if (IS_ERR(res->regulator.regulator)) { + dev_err(dev, "cannot get regulator \"%s\"\n", + res->regulator.id); + return PTR_ERR(res->regulator.regulator); + } + + return 0; +} + +static int power_seq_step_run_regulator(struct power_seq_step *step) +{ + if (step->regulator.enable) + return regulator_enable(step->resource->regulator.regulator); + else + return regulator_disable(step->resource->regulator.regulator); +} + +const struct power_seq_res_ops power_seq_regulator_ops = { + .name = "regulator", + .of_parse = of_power_seq_parse_regulator, + .step_run = power_seq_step_run_regulator, + .res_compare = power_seq_res_compare_regulator, + .res_alloc = power_seq_res_alloc_regulator, +}; + +#else + +const struct power_seq_res_ops power_seq_regulator_ops = { +}; + +#endif diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h new file mode 100644 index 0000000..21b95b6 --- /dev/null +++ b/include/linux/power_seq.h @@ -0,0 +1,203 @@ +/* + * power_seq.h + * + * Simple interpreter for power sequences defined as platform data or device + * tree properties. + * + * Power sequences are designed to replace the callbacks typically used in + * board-specific files that implement board- or device- specific power + * sequences (such as those of backlights). A power sequence is an array of + * steps referencing resources (regulators, GPIOs, PWMs, ...) with an action to + * perform on them. By having the power sequences interpreted, it becomes + * possible to describe them in the device tree and thus to remove + * board-specific files from the kernel. + * + * See Documentation/power/power_seqs.txt for detailed information. + * + * 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. + * + */ + +#ifndef __LINUX_POWER_SEQ_H +#define __LINUX_POWER_SEQ_H + +#include <linux/types.h> +#include <linux/list.h> + +struct device; +struct regulator; +struct pwm_device; + +/** + * The different kinds of resources that can be controlled by the sequences + */ +enum power_seq_res_type { + POWER_SEQ_DELAY, + POWER_SEQ_REGULATOR, + POWER_SEQ_PWM, + POWER_SEQ_GPIO, + POWER_SEQ_NUM_TYPES, +}; + +/** + * struct power_seq_regulator_resource + * @id: name of the regulator + * @regulator: resolved regulator. Written during resource resolution. + */ +struct power_seq_regulator_resource { + const char *id; + struct regulator *regulator; +}; + +/** + * struct power_seq_pwm_resource + * @id: name of the PWM + * @regulator: resolved PWM. Written during resource resolution. + */ +struct power_seq_pwm_resource { + const char *id; + struct pwm_device *pwm; +}; + +/** + * struct power_seq_gpio_resource + * @gpio: number of the GPIO + * @is_set: track GPIO state to set its direction at first use + */ +struct power_seq_gpio_resource { + int gpio; + bool is_set; +}; + +/** + * struct power_seq_resource - resource used by power sequences + * @type: type of the resource. This decides which member of the union is + * used for this resource + * @list: link resources together in power_seq_set + * @regulator: used if @type == POWER_SEQ_REGULATOR + * @pwm: used if @type == POWER_SEQ_PWM + * @gpio: used if @type == POWER_SEQ_GPIO + */ +struct power_seq_resource { + enum power_seq_res_type type; + struct list_head list; + union { + struct power_seq_regulator_resource regulator; + struct power_seq_pwm_resource pwm; + struct power_seq_gpio_resource gpio; + }; +}; +#define power_seq_for_each_resource(pos, set) \ + list_for_each_entry(pos, &(set)->resources, list) + +/** + * struct power_seq_delay_step - action data for delay steps + * @delay: amount of time to wait, in microseconds + */ +struct power_seq_delay_step { + unsigned int delay; +}; + +/** + * struct power_seq_regulator_step - platform data for regulator steps + * @enable: whether to enable or disable the regulator during this step + */ +struct power_seq_regulator_step { + bool enable; +}; + +/** + * struct power_seq_pwm_step - action data for PWM steps + * @enable: whether to enable or disable the PWM during this step + */ +struct power_seq_pwm_step { + bool enable; +}; + +/** + * struct power_seq_gpio_step - action data for GPIO steps + * @enable: whether to enable or disable the GPIO during this step + */ +struct power_seq_gpio_step { + int value; +}; + +/** + * struct power_seq_step - data for power sequences steps + * @resource: resource used by this step + * @delay: used if resource->type == POWER_SEQ_DELAY + * @regulator: used if resource->type == POWER_SEQ_REGULATOR + * @pwm: used if resource->type == POWER_SEQ_PWN + * @gpio: used if resource->type == POWER_SEQ_GPIO + */ +struct power_seq_step { + struct power_seq_resource *resource; + union { + struct power_seq_delay_step delay; + struct power_seq_regulator_step regulator; + struct power_seq_pwm_step pwm; + struct power_seq_gpio_step gpio; + }; +}; + +struct power_seq_set; + +/** + * struct power_seq - single power sequence + * @id: name of this sequence + * @list: link sequences together in power_seq_set. Leave as-is + * @set: set this sequence belongs to. Written when added to a set + * @num_steps: number of steps in the sequence + * @steps: array of steps that make the sequence + */ +struct power_seq { + const char *id; + struct list_head list; + struct power_seq_set *set; + unsigned int num_steps; + struct power_seq_step steps[]; +}; + +/** + * struct power_seq_set - power sequences and resources used by a device + * @dev: device this set belongs to + * @resources: list of resources used by power sequences + * @seqs: list of power sequences + */ +struct power_seq_set { + struct device *dev; + struct list_head resources; + struct list_head seqs; +}; + +/** + * struct platform_power_seq_set - define power sequences as platform data + * @num_seqs: number of sequences defined + * @seqs: array of num_seqs power sequences + */ +struct platform_power_seq_set { + unsigned int num_seqs; + struct power_seq *seqs[]; +}; + +struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev); +void power_seq_set_init(struct power_seq_set *set, struct device *dev); +int power_seq_set_add_sequence(struct power_seq_set *set, + struct power_seq *seq); +int power_seq_set_add_sequences(struct power_seq_set *set, + struct platform_power_seq_set *seqs); +struct power_seq *power_seq_lookup(struct power_seq_set *seqs, const char *id); +int power_seq_run(struct power_seq *seq); + +#endif