Message-Id: <20121120215429.B621F3E1821@localhost>
Date:	Tue, 20 Nov 2012 21:54:29 +0000
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Alexandre Courbot <acourbot@...dia.com>,
	Anton Vorontsov <cbouatmailru@...il.com>,
	Stephen Warren <swarren@...dia.com>,
	Thierry Reding <thierry.reding@...onic-design.de>,
	Mark Zhang <markz@...dia.com>,
	Rob Herring <rob.herring@...xeda.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Arnd Bergmann <arnd@...db.de>
Cc:	linux-tegra@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org, linux-pm@...r.kernel.org,
	Alexandre Courbot <gnurou@...il.com>,
	Alexandre Courbot <acourbot@...dia.com>
Subject: Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences

On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@...dia.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists