From: Stephen Warren <swarren@wwwdotorg.org>
To: Alexandre Courbot <acourbot@nvidia.com>
Cc: Stephen Warren <swarren@nvidia.com>,
Thierry Reding <thierry.reding@avionic-design.de>,
Simon Glass <sjg@chromium.org>,
Grant Likely <grant.likely@secretlab.ca>,
Rob Herring <rob.herring@calxeda.com>,
Mark Brown <broonie@opensource.wolfsonmicro.com>,
Anton Vorontsov <cbou@mail.ru>,
David Woodhouse <dwmw2@infradead.org>,
Arnd Bergmann <arnd@arndb.de>,
Leela Krishna Amudala <leelakrishna.a@gmail.com>,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
linux-doc@vger.kernel.org
Subject: Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
Date: Thu, 16 Aug 2012 12:38:33 -0600 [thread overview]
Message-ID: <502D3E29.1010501@wwwdotorg.org> (raw)
In-Reply-To: <1345097337-24170-2-git-send-email-acourbot@nvidia.com>
On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
>
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> +Specifying Power Sequences in the Device Tree
> +=============================================
> +In the device tree, power sequences are specified as sub-nodes of the device
> +node and reference resources declared by that device.
> +
> +For an introduction about runtime interpreted power sequences, see
> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
Device tree bindings shouldn't reference Linux documentation; the
bindings are supposed to be OS-agnostic.
> +Power Sequences Structure
> +-------------------------
> +Power sequences are sub-nodes that are named such as the device driver can find
> +them. The driver's documentation should list the sequence names it recognizes.
That's a little roundabout. That might be better as simply:
Valid power sequence names are defined by each device's binding. For a
power sequence named "foo", a node named "foo-power-sequence" defines
that sequence.
> +Inside a power sequence node are sub-nodes that describe the different steps
> +of the sequence. Each step 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.
Node names shouldn't be interpreted by the code; nodes should all be
named after the type of object the represent. Hence, every step should
be named just "step" for example.
The node's unit address (@0) should be used to distinguish the nodes.
This requires reg properties within each node to match the unit address,
and hence #address-cells and #size-cells properties in the power
sequence itself.
Note that this somewhat conflicts with accessing the top-level power
sequence by name too; perhaps that should be re-thought too. I must
admit this DT rule kinda sucks.
> +Power Sequences Steps
> +---------------------
> +Every step of a sequence describes an action to be performed on a resource. It
> +generally includes a property named after the resource type, and which value
> +references the resource to be used. Depending on the resource type, additional
> +properties can be defined to control the action to be performed.
I think you need to add a property that indicates what type of resource
each step applies to. Sure, this is implicit in that if a "gpio"
property exists, the step is a GPIO step, but in order to make that
work, you'd have to search each node for each possible resource type's
property name. It'd be far better to read a single type="gpio" property,
then parse the step based on that.
> +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 sequences */
> + pwms = <&pwm 2 5000000>;
> + pwm-names = "backlight";
> + power-supply = <&backlight_reg>;
> + enable-gpio = <&gpio 28 0>;
> +
> + power-on-sequence {
> + step0 {
> + regulator = "power";
> + enable;
> + };
> + step1 {
> + delay = <10000>;
> + };
> + step2 {
> + pwm = "backlight";
> + enable;
> + };
> + step3 {
> + gpio = "enable";
> + enable;
> + };
> + };
> +
> + power-off-sequence {
> + step0 {
> + gpio = "enable";
> + disable;
> + };
> + step1 {
> + pwm = "backlight";
> + disable;
> + };
> + step2 {
> + delay = <10000>;
> + };
> + step3 {
> + regulator = "power";
> + disable;
> + };
> + };
> + };
I notice that for clocks, pwms, and interrupts, the initial step of the
lookup is via a single property that lists all know resources of that
type. Regulators and GPIOs don't follow this style though. Using the
same mechanism for power-sequences would yield something like the
following, which would avoid the "node names must be significant" issue
I mention above, although it does make everything rather more wordy.
[start my proposal]
> backlight {
> compatible = "pwm-backlight";
>
> /* resources used by the sequences */
> pwms = <&pwm 2 5000000>;
> pwm-names = "backlight";
> power-supply = <&backlight_reg>;
> bl-enable-gpio = <&gpio 28 0>;
> pwr-seqs = <&bl_on &bl_off>;
> pwr-seq-names = "on", "off";
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> bl_on: pwr-seq@0 {
> reg = <0>;
> #address-cells = <1>;
> #size-cells = <0>;
> step@0 {
> reg = <0>;
> type = "regulator";
> regulator = "power";
> enable;
> };
> step@1 {
> reg = <1>;
> type = "delay";
> delay = <10000>;
> };
> step@2 {
> reg = <2>;
> type = "pwm";
> pwm = "backlight";
> enable;
> };
> step@3 {
> reg = <3>;
> type = "gpio";
> gpio = "bl-enable";
> enable;
> };
> };
>
> bl_off: pwr-seq@1 {
> reg = <1>;
> #address-cells = <1>;
> #size-cells = <0>;
> step@0 {
> reg = <0>;
> type = "gpio";
> gpio = "bl-enable";
> disable;
> };
> step@1 {
> reg = <1>;
> type = "pwm";
> pwm = "backlight";
> disable;
> };
> step@2 {
> reg = <2>;
> type = "delay";
> delay = <10000>;
> };
> step@3 {
> reg = <3>;
> type = "regulator";
> regulator = "power";
> disable;
> };
> };
> };
>
[end my proposal]
> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function builds a power sequence from the
> +platform data. It also takes care of resolving and allocating the resources
> +referenced by the sequence if needed:
> +
> + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> + struct platform_power_seq *pseq);
> +
> +The 'dev' argument is the device in the name of which the resources are to be
> +allocated.
> +
> +The 'ress' argument is a list to which the resolved resources are appended. This
> +avoids allocating a resource referenced in several power sequences multiple
> +times.
I see in other parts of the thread, there has been discussion re:
needing the separate ress parameter, and requiring the driver to pass
this in to multiple power_seq_build calls.
The way the pinctrl subsystem solved this was to have a single function
that parsed all pinctrl setting (c.f. all power sequences) at once, and
return a single handle. Later, the driver passes this handle
pinctrl_lookup_state(), along with the requested state (c.f. sequence
name) to search for, and finally passes that handle to
pinctrl_select_state(). Doing something similar here would result in:
struct power_seqs *power_seq_get(struct device *dev);
void power_seq_put(struct power_seqs *seqs);
struct power_seq *power_seq_lookup(struct power_seqs *seqs,
const char *name);
int power_seq_executestruct power_seq *seq);
struct power_seqs *devm_power_seq_get(struct device *dev);
void devm_power_seq_put(struct power_seqs *seqs);
> +On success, the function returns a devm allocated resolved sequence that is
Perhaps the function should be named devm_power_seq_build(), in order to
make this obvious to people who only read the client code, and not this
documentation.
> +ready to be passed to power_seq_run(). In case of failure, and error code is
> +returned.
> +
> +A resolved power sequence returned by power_seq_build can be run by
> +power_run_run():
> +
> + int power_seq_run(power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.
> +
> +There is no need to explicitly free the resources used by the sequence as they
> +are devm-allocated.
next prev parent reply other threads:[~2012-08-16 18:38 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-16 6:08 [PATCH v4 0/3] Runtime Interpreted Power Sequences Alexandre Courbot
2012-08-16 6:08 ` [PATCH v4 1/3] " Alexandre Courbot
2012-08-16 7:42 ` Thierry Reding
2012-08-16 9:19 ` Alex Courbot
2012-08-16 9:52 ` Thierry Reding
2012-08-16 10:33 ` Alex Courbot
2012-08-16 10:52 ` Thierry Reding
2012-08-16 14:14 ` Mark Brown
2012-08-16 18:38 ` Stephen Warren [this message]
2012-08-16 18:47 ` Mark Brown
2012-08-16 18:57 ` Stephen Warren
2012-08-16 19:35 ` Stephen Warren
2012-08-16 19:49 ` Thierry Reding
2012-08-16 21:10 ` Mitch Bradley
2012-08-17 23:04 ` Mark Brown
2012-08-17 8:52 ` Alex Courbot
2012-08-21 7:44 ` Tomi Valkeinen
2012-08-21 8:22 ` Alex Courbot
2012-08-21 8:33 ` Thierry Reding
2012-08-21 8:53 ` Alex Courbot
2012-08-21 8:57 ` Tomi Valkeinen
2012-08-21 9:13 ` Thierry Reding
2012-08-21 9:54 ` Tomi Valkeinen
2012-08-21 16:57 ` Mark Brown
2012-08-22 5:42 ` Thierry Reding
2012-08-24 9:24 ` Alex Courbot
2012-08-24 10:34 ` Tomi Valkeinen
2012-08-16 6:08 ` [PATCH v4 2/3] pwm_backlight: use power sequences Alexandre Courbot
2012-08-16 18:42 ` Stephen Warren
2012-08-16 6:08 ` [PATCH v4 3/3] tegra: add pwm backlight device tree nodes Alexandre Courbot
2012-08-16 18:45 ` Stephen Warren
2012-08-16 21:47 ` [PATCH v4 0/3] Runtime Interpreted Power Sequences Rafael J. Wysocki
2012-08-17 8:54 ` Alex Courbot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=502D3E29.1010501@wwwdotorg.org \
--to=swarren@wwwdotorg.org \
--cc=acourbot@nvidia.com \
--cc=arnd@arndb.de \
--cc=broonie@opensource.wolfsonmicro.com \
--cc=cbou@mail.ru \
--cc=devicetree-discuss@lists.ozlabs.org \
--cc=dwmw2@infradead.org \
--cc=grant.likely@secretlab.ca \
--cc=leelakrishna.a@gmail.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=rob.herring@calxeda.com \
--cc=sjg@chromium.org \
--cc=swarren@nvidia.com \
--cc=thierry.reding@avionic-design.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).