Message-ID: <20120816074232.GA17917@avionic-0098.mockup.avionic-design.de>
Date:	Thu, 16 Aug 2012 09:42:32 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Alexandre Courbot <acourbot@...dia.com>
Cc:	Stephen Warren <swarren@...dia.com>,
	Simon Glass <sjg@...omium.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Anton Vorontsov <cbou@...l.ru>,
	David Woodhouse <dwmw2@...radead.org>,
	Arnd Bergmann <arnd@...db.de>,
	Leela Krishna Amudala <leelakrishna.a@...il.com>,
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fbdev@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences

On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
> 
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> 
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
> ---
>  .../devicetree/bindings/power_seq/power_seq.txt    | 101 +++++
>  Documentation/power/power_seq.txt                  | 129 +++++++
>  drivers/power/Kconfig                              |   3 +
>  drivers/power/Makefile                             |   1 +
>  drivers/power/power_seq.c                          | 420 +++++++++++++++++++++
>  include/linux/power_seq.h                          | 142 +++++++
>  6 files changed, 796 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
>  create mode 100644 Documentation/power/power_seq.txt
>  create mode 100644 drivers/power/power_seq.c
>  create mode 100644 include/linux/power_seq.h
> 
> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> new file mode 100644
> index 0000000..749c6e4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> @@ -0,0 +1,101 @@
> +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.
> +
> +Power Sequences Structure
> +-------------------------
> +Power sequences are sub-nodes that are named such as the device driver can find

"named such that"?

> +them. The driver's documentation should list the sequence names it recognizes.
> +
> +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.
> +
> +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.
> +
> +Currently supported resource types are:

I think the "currently" can be dropped.

> +- "delay", which should simply contain a delay in microseconds to wait before
> +  going on with the rest of the sequence. It takes no additional property.
> +- "regulator" contains the name of a regulator to be acquired using
> +  regulator_get(). An additional property, either "enable" or "disable", must be
> +  present to control whether the regulator should be enabled or disabled during
> +  that step.
> +- "pwm" is set to the name of a PWM acquired using pwm_get(). As with regulator,
> +  an additional "enable" or "disable" property is required.
> +- "gpio" contains the name of a GPIO to enable or disable using the same
> +  additional property as regulator or pwm. The gpio is resolved by appending
> +  "-gpio" to the given name and looking for a device property with a GPIO
> +  phandle.

I find this part slightly confusing. It doesn't seem quite obvious from
the text that "delay", "regulator", "pwm" and "gpio" are also actual
property names.

It might also be worth saying that the "enable" and "disable" properties
are boolean in nature and don't need a value.

Then again this becomes much clearer with the example below, so maybe
I'm just being too picky here.

> +
> +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;
> +			};
> +		};
> +	};
> +
> +The first part lists the PWM, regulator, and GPIO 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 framework
> +in a way that makes them accessible by name.
> +
> +After the resources declaration, two sequences follow for powering the backlight
> +on and off. Their names are specified by the pwm-backlight driver. Every step
> +uses one of the "delay", "regulator", "pwm" or "gpio" properties to reference a
> +previously-declared resource. Additional "enable" or "disable" properties are
> +also used as needed.
> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
> new file mode 100644
> index 0000000..3ab4f93
> --- /dev/null
> +++ b/Documentation/power/power_seq.txt
> @@ -0,0 +1,129 @@
> +Runtime Interpreted Power Sequences
> +===================================
> +
> +Problem
> +-------
> +One very common board-dependent code is the out-of-driver code that is used to
> +turn a device on or off. For instance, SoC boards very commonly use a GPIO
> +(abstracted to a regulator or not) to control the power supply of a backlight,
> +disabling it when the backlight is not used in order to save power. The GPIO
> +that should be used, however, as well as the exact power sequence that may
> +also involve other resources, is board-dependent and thus unknown of the driver.

"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 reflect a power change. This
> +approach, however, introduces board-dependant code into the kernel and is not
> +compatible with the device tree.
> +
> +The Runtime Interpreted Power Sequences (or power sequences for short) aims at

"... Sequences [...] aim"?

> +turning this code into platform data or device tree nodes. Power sequences are
> +described using a simple format and run by a simple interpreter whenever needed.
> +This allows to remove the callback mechanism and makes the kernel less
> +board-dependant.
> +
> +What are Power Sequences?
> +-------------------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources so far are:

Again, I don't see a need for "so far" here. It implies that new types
may be added. While it is quite possible and maybe even likely to happen
the new types can be added to the documentation at the same time.

> +- delay (just wait for the delay given in microseconds)
> +- GPIO (enable or disable)
> +- regulator (enable or disable)
> +- PWM (enable or disable)
> +
> +Every step designates a resource type and parameters that are relevant to it.
> +For instance, GPIO and PWMs can be enabled or disabled.
> +
> +When a power sequence is run, each of its step is executed sequentially until

"each of its steps"

> +one step fails or the end of the sequence is reached.
> +
> +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 is a static instance of  simple array of
> +platform_power_seq_step instances, each
> +instance describing a step. The type as well as one of id or gpio members
> +(depending on the type) must be specified. The last step must be of type
> +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
> +identified by number. For example, the following sequence will turn on the
> +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:
> +
> +static struct platform_power_seq power_on_seq = {
> +		.nb_steps = 3,

I think num_steps would be more canonical.

> +		.steps = {
> +			{
> +				.type = POWER_SEQ_REGULATOR,
> +				.regulator.regulator = "power",
> +				.regulator.enable = 1,
> +			},

This may be easier to read as:

	.type = POWER_SEQ_REGULATOR,
	.regulator {
		.regulator = "power",
		.enable = 1,
	}

Also, why not rename the .regulator field to .name? That describes
better what it is and removes the redundancy of having the structure
named regulator and a regulator field within.

> +			{
> +				.type = POWER_SEQ_DELAY,
> +				.delay.delay_us = 10000,
> +			},
> +			{
> +				.type = POWER_SEQ_GPIO,
> +				.gpio.gpio = 110,
> +				.gpio.enable = 1,
> +			},

Same comments as for the regulator step above. Also, since enable is a
boolean field, maybe you should use true and false instead.

> +		},
> +};
> +
> +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:
> +
> +		power-supply = <&power_reg>;
> +		switch-gpio = <&gpio 110 0>;
> +
> +		power-on-sequence {
> +			step0 {
> +				regulator = "power";
> +				enable;
> +			};
> +			step1 {
> +				delay = <10000>;
> +			};
> +			step2 {
> +				gpio = "switch";
> +				enable;
> +			};
> +		};
> +
> +See Documentation/devicetree/bindings/power_seq/power_seq.txt for the complete
> +syntax of the bindings.
> +
> +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.
> +
> +On success, the function returns a devm allocated resolved sequence that is
> +ready to be passed to power_seq_run(). In case of failure, and error code is
> +returned.
> +
> +A resolved power sequence returned by power_seq_build can be run by
> +power_run_run():
> +
> +  int power_seq_run(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.

I had some comments about this particular interface for creating
sequences in the last series. My point was that explicitly requiring
drivers to manage a list of already allocated resources may be too much
added complexity. Power sequences should be easy to use, and I find the
requirement for a separately managed list of resources cumbersome.

What I proposed last time was to collect all power sequences under a
common parent object, which in turn would take care of managing the
resources.

> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index c1892f3..4172fe4 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -312,4 +312,7 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
>  	  thermistor connected on BATCTRL ADC.
>  endif # POWER_SUPPLY
>  
> +config POWER_SEQ
> +	bool
> +
>  source "drivers/power/avs/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index ee58afb..828859c 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -45,3 +45,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.o
> diff --git a/drivers/power/power_seq.c b/drivers/power/power_seq.c
> new file mode 100644
> index 0000000..1dcdbe0
> --- /dev/null
> +++ b/drivers/power/power_seq.c
> @@ -0,0 +1,420 @@
> +/*
> + * power_seq.c - A simple power sequence interpreter for platform devices
> + *               and device tree.
> + *
> + * Author: Alexandre Courbot <acourbot@...dia.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/slab.h>
> +#include <linux/delay.h>
> +#include <linux/pwm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +
> +struct power_seq_step {
> +	/* Copy of the platform data */
> +	struct platform_power_seq_step pdata;
> +	/* Resolved resource */
> +	struct power_seq_resource *resource;
> +};
> +
> +struct power_seq {
> +	struct device *dev;
> +	unsigned int nb_steps;
> +	struct power_seq_step steps[];
> +};
> +
> +static char *res_names[POWER_SEQ_MAX] = {
> +	[POWER_SEQ_DELAY] = "delay",
> +	[POWER_SEQ_REGULATOR] = "regulator",
> +	[POWER_SEQ_GPIO] = "gpio",
> +	[POWER_SEQ_PWM] = "pwm",
> +};

static const?

> +
> +static int power_seq_step_run(struct power_seq_step *step)
> +{
> +	struct platform_power_seq_step *pdata = &step->pdata;
> +	int err = 0;
> +
> +	switch (pdata->type) {
> +	case POWER_SEQ_DELAY:
> +		usleep_range(pdata->delay.delay_us,
> +			     pdata->delay.delay_us + 1000);
> +		break;
> +#ifdef CONFIG_REGULATOR
> +	case POWER_SEQ_REGULATOR:
> +		if (pdata->regulator.enable)
> +			err = regulator_enable(step->resource->regulator);
> +		else
> +			err = regulator_disable(step->resource->regulator);
> +		break;
> +#endif
> +#ifdef CONFIG_PWM
> +	case POWER_SEQ_PWM:
> +		if (pdata->gpio.enable)
> +			err = pwm_enable(step->resource->pwm);
> +		else
> +			pwm_disable(step->resource->pwm);
> +		break;
> +#endif
> +#ifdef CONFIG_GPIOLIB
> +	case POWER_SEQ_GPIO:
> +		gpio_set_value_cansleep(pdata->gpio.gpio, pdata->gpio.enable);
> +		break;
> +#endif
> +	/*
> +	 * should never happen unless the sequence includes a step which
> +	 * type does not have support compiled in

I think this should be "whose type"? I also remember commenting on the
whole #ifdef'ery here. I really don't think it is necessary. At least
for regulators I know that the functions can be used even if the
subsystem itself isn't supported. The same seems to hold for GPIO and we
can probably add something similar for PWM.

It might also be a good idea to just skip unsupported resource types
when the sequence is built, accompanied by runtime warnings that the
type is not supported.

> +	 */
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (err < 0)
> +		return err;
> +
> +	return 0;

This can probably be collapsed to just "return err;", can't it?

> +}
> +
> +int power_seq_run(struct power_seq *seq)
> +{
> +	struct device *dev = seq->dev;
> +	int err, cpt;

Any reason why you call the loop variable cpt instead of something more
canonical such as i? Also it should be of type unsigned int.

> +
> +	if (!seq)
> +		return 0;
> +
> +	for (cpt = 0; cpt < seq->nb_steps; cpt++) {
> +		err = power_seq_step_run(&seq->steps[cpt]);
> +		if (err) {
> +			dev_err(dev, "error %d while running power sequence!\n",
> +				err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_run);
> +
> +#ifdef CONFIG_OF
> +static int of_power_seq_enable_properties(struct device *dev,
> +					  struct device_node *node,
> +					  bool *enable)

Maybe rename this to of_power_seq_parse_enable_properties() to make it
more obvious that it is actually parsing data. It's an awfully long name
for a function, though.

> +{
> +	if (of_find_property(node, "enable", NULL)) {
> +		*enable = true;
> +	} else if (of_find_property(node, "disable", NULL)) {
> +		*enable = false;
> +	} else {
> +		dev_err(dev, "missing enable or disable property!\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int of_parse_power_seq_step(struct device *dev, struct device_node *node,
> +				   struct platform_power_seq_step *step)
> +{
> +	struct property *res_id = NULL;
> +	int i, err;
> +
> +	/* Try to find a meaningful name property */
> +	for (i = 0; i < POWER_SEQ_MAX; i++) {

Maybe this should be renamed to POWER_SEQ_MAX_TYPE, POWER_SEQ_TYPE_MAX,
POWER_SEQ_NUM_TYPES or some such. I had assumed POWER_SEQ_MAX would mean
the maximum length of a power sequence.

> +		struct property *mprop;
> +
> +		mprop = of_find_property(node, res_names[i], NULL);
> +		if (mprop) {
> +			if (res_id) {
> +				dev_err(dev,
> +					"more than one resource in step!\n");
> +				return -EINVAL;
> +			}
> +			step->type = i;
> +			res_id = mprop;
> +		}
> +	}
> +	if (!res_id) {
> +		dev_err(dev, "missing resource property for power seq step!\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Now parse resource-specific properties */
> +	switch (step->type) {
> +	case POWER_SEQ_DELAY:
> +		err = of_property_read_u32(node, res_id->name,
> +					   &step->delay.delay_us);
> +		if (err)
> +			goto err_read_property;
> +
> +		break;
> +
> +	case POWER_SEQ_REGULATOR:
> +		err = of_property_read_string(node, res_id->name,
> +					    &step->regulator.regulator);
> +		if (err)
> +			goto err_read_property;
> +
> +		err = of_power_seq_enable_properties(dev, node,
> +						     &step->regulator.enable);
> +		if (err)
> +			return err;
> +
> +		break;
> +
> +	case POWER_SEQ_PWM:
> +		err = of_property_read_string(node, res_id->name,
> +					      &step->pwm.pwm);
> +		if (err)
> +			goto err_read_property;
> +
> +		err = of_power_seq_enable_properties(dev, node,
> +						     &step->pwm.enable);
> +		if (err)
> +			return err;
> +
> +		break;
> +
> +#ifdef CONFIG_OF_GPIO
> +	case POWER_SEQ_GPIO:
> +	{
> +		char prop_name[32]; /* max size of property name */
> +		const char *gpio_name;
> +		int gpio;
> +
> +		err = of_property_read_string(node, res_id->name, &gpio_name);
> +		if (err)
> +			goto err_read_property;
> +
> +		/* Resolve the GPIO name */
> +		snprintf(prop_name, 32, "%s-gpio", gpio_name);

I'm not sure if there's a limit on the length of DT property names, but
maybe using kasprintf would be a better idea here.

> +		gpio = of_get_named_gpio(dev->of_node, prop_name, 0);
> +		if (gpio < 0) {
> +			dev_err(dev, "cannot resolve gpio \"%s\"\n", gpio_name);
> +			return gpio;
> +		}
> +		step->gpio.gpio = gpio;
> +
> +		err = of_power_seq_enable_properties(dev, node,
> +						     &step->gpio.enable);
> +		if (err)
> +			return err;
> +
> +		break;
> +	}
> +#endif /* CONFIG_OF_GPIO */
> +
> +	default:
> +		dev_err(dev, "unhandled power sequence step type %s\n",
> +			res_names[step->type]);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +
> +err_read_property:
> +	dev_err(dev, "cannot read %s property!", res_names[step->type]);
> +	return -EINVAL;
> +}

Given the size of this function, I think it might become more readable
if it were split into separate parse functions for each type of
resource. It will also allow you to get rid of this #ifdef within the
function.

> +
> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
> +					      struct device_node *node)
> +{
> +	struct device_node *child = NULL;
> +	struct platform_power_seq *pseq;
> +	int nb_steps = 0, size;
> +	int err;
> +
> +	if (!node)
> +		return ERR_PTR(-EINVAL);
> +
> +	nb_steps = of_get_child_count(node);
> +	size = sizeof(pseq) + sizeof(struct platform_power_seq_step) * nb_steps;

Shouldn't the first term be sizeof(*pseq)?

> +	pseq = devm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!pseq)
> +		return ERR_PTR(-ENOMEM);
> +	pseq->nb_steps = nb_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 = kstrtoint(child->name + 4, 10, &pos);
> +		if (err < 0)
> +			goto parse_error;

kstrtouint()? Also this is somewhat ugly. Perhaps adding #address-cells
and #size-cells properties isn't that bad after all.

> +
> +		if (pos >= nb_steps || pseq->steps[pos].type != 0) {
> +			err = -EINVAL;
> +			goto parse_error;
> +		}
> +
> +		err = of_parse_power_seq_step(dev, child, &pseq->steps[pos]);
> +		if (err)
> +			return ERR_PTR(err);
> +	}
> +
> +	return pseq;
> +
> +parse_error:
> +	dev_err(dev, "invalid power step name %s!\n", child->name);
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(of_parse_power_seq);
> +#endif /* CONFIG_OF */
> +
> +static
> +struct power_seq_resource *power_seq_find_resource(struct list_head *ress,
> +					struct platform_power_seq_step *step)

I think it is customary to put the return value type on the same line as
the static modifier.

> +{
> +	struct power_seq_resource *res;
> +
> +	list_for_each_entry(res, ress, list) {
> +		struct platform_power_seq_step *pdata = res->pdata;
> +
> +		if (pdata->type != step->type)
> +			continue;
> +
> +		switch (pdata->type) {
> +		case POWER_SEQ_REGULATOR:
> +			if (!strcmp(pdata->regulator.regulator,
> +				    step->regulator.regulator))
> +				return res;
> +			break;
> +		case POWER_SEQ_PWM:
> +			if (!strcmp(pdata->pwm.pwm, step->pwm.pwm))
> +				return res;
> +			break;
> +		case POWER_SEQ_GPIO:
> +			if (pdata->gpio.gpio == step->gpio.gpio)
> +				return res;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static int power_seq_allocate_resource(struct device *dev,
> +				       struct power_seq_resource *res)
> +{
> +	struct platform_power_seq_step *pdata = res->pdata;
> +	int err;
> +
> +	switch (pdata->type) {
> +	case POWER_SEQ_DELAY:
> +		break;
> +#ifdef CONFIG_REGULATOR
> +	case POWER_SEQ_REGULATOR:
> +		res->regulator = devm_regulator_get(dev,
> +						    pdata->regulator.regulator);
> +		if (IS_ERR(res->regulator)) {
> +			dev_err(dev, "cannot get regulator \"%s\"\n",
> +				pdata->regulator.regulator);
> +			return PTR_ERR(res->regulator);
> +		}
> +		break;
> +#endif
> +#ifdef CONFIG_PWM
> +	case POWER_SEQ_PWM:
> +		res->pwm = devm_pwm_get(dev, pdata->pwm.pwm);
> +		if (IS_ERR(res->pwm)) {
> +			dev_err(dev, "cannot get pwm \"%s\"\n", pdata->pwm.pwm);
> +			return PTR_ERR(res->pwm);
> +		}
> +		break;
> +#endif
> +#ifdef CONFIG_GPIOLIB
> +	case POWER_SEQ_GPIO:
> +		err = devm_gpio_request_one(dev, pdata->gpio.gpio,
> +					 GPIOF_OUT_INIT_HIGH, "backlight_gpio");
> +		if (err) {
> +			dev_err(dev, "cannot get gpio %d\n", pdata->gpio.gpio);
> +			return err;
> +		}
> +		break;
> +#endif
> +	default:
> +		dev_err(dev, "invalid resource type %d\n", pdata->type);
> +		return -EINVAL;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> +				  struct platform_power_seq *pseq)
> +{
> +	struct power_seq *seq;
> +	struct power_seq_resource *res;
> +	int cpt, err;
> +
> +	seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(struct power_seq_step) *
> +			   pseq->nb_steps, GFP_KERNEL);
> +	if (!seq)
> +		return ERR_PTR(-ENOMEM);
> +
> +	seq->dev = dev;
> +	seq->nb_steps = pseq->nb_steps;
> +
> +	for (cpt = 0; cpt < seq->nb_steps; cpt++) {
> +		struct platform_power_seq_step *pstep = &pseq->steps[cpt];
> +		struct power_seq_step *step = &seq->steps[cpt];
> +
> +		memcpy(&step->pdata, pstep, sizeof(step->pdata));
> +
> +		/* Delay steps have no resource */
> +		if (pstep->type == POWER_SEQ_DELAY)
> +			continue;
> +
> +		/* create resource node if not referenced already */
> +		res = power_seq_find_resource(ress, pstep);
> +		if (!res) {
> +			res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> +			if (!res)
> +				return ERR_PTR(-ENOMEM);
> +			res->pdata = &step->pdata;
> +
> +			err = power_seq_allocate_resource(dev, res);
> +			if (err < 0)
> +				return ERR_PTR(err);
> +
> +			list_add(&res->list, ress);
> +		}
> +		step->resource = res;
> +	}
> +
> +	return seq;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_build);
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@...dia.com>");
> +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
> new file mode 100644
> index 0000000..d9dd277
> --- /dev/null
> +++ b/include/linux/power_seq.h
> @@ -0,0 +1,142 @@
> +/*
> + * power_seq.h
> + *
> + * Simple interpreter for defining power sequences as platform data or device
> + * tree properties.
> + *
> + * Power sequences are designed to replace the callbacks typically used in
> + * board-specific files that implement board-specific power sequences of devices
> + * such as backlights. A power sequence is an array of resources (which can a
> + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
> + * disable) and optional pre and post step delays. By having them interpreted
> + * instead of arbitrarily executed, it is possible to describe these in the
> + * device tree and thus remove board-specific code from the kernel.
> + *
> + * Author: Alexandre Courbot <acourbot@...dia.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>
> +
> +struct device;
> +struct regulator;
> +struct pwm_device;
> +struct device_node;
> +
> +/**
> + * The different kinds of resources that can be controlled during the sequences.
> + */
> +enum power_seq_res_type {
> +	POWER_SEQ_DELAY,
> +	POWER_SEQ_REGULATOR,
> +	POWER_SEQ_PWM,
> +	POWER_SEQ_GPIO,
> +	POWER_SEQ_MAX,
> +};
> +
> +struct platform_power_seq_delay_step {
> +	unsigned int delay_us;
> +};
> +
> +struct platform_power_seq_regulator_step {
> +	const char *regulator;
> +	bool enable;
> +};
> +
> +struct platform_power_seq_pwm_step {
> +	const char *pwm;
> +	bool enable;
> +};
> +
> +struct platform_power_seq_gpio_step {
> +	int gpio;
> +	bool enable;
> +};
> +
> +/**
> + * Platform definition of power sequences. A sequence is an array of these,
> + * terminated by a STOP instance.
> + */
> +struct platform_power_seq_step {
> +	enum power_seq_res_type type;
> +	union {
> +		struct platform_power_seq_delay_step delay;
> +		struct platform_power_seq_regulator_step regulator;
> +		struct platform_power_seq_pwm_step pwm;
> +		struct platform_power_seq_gpio_step gpio;
> +	};
> +};
> +
> +struct platform_power_seq {
> +	unsigned int nb_steps;
> +	struct platform_power_seq_step steps[];
> +};
> +
> +/**
> + * We maintain a list of these to monitor which resources have already
> + * been met and allocated while building the sequences.
> + */
> +struct power_seq_resource {
> +	/* relevant for resolving the resource and knowing its type */
> +	struct platform_power_seq_step *pdata;
> +	/* resolved resource (if any) */
> +	union {
> +		struct regulator *regulator;
> +		struct pwm_device *pwm;
> +	};
> +	struct list_head list;
> +};
> +
> +struct power_seq_resource;
> +struct power_seq;
> +
> +#ifdef CONFIG_OF
> +/**
> + * Build a platform data sequence from a device tree node. Memory for the
> + * platform sequence is allocated using devm_kzalloc on dev and can be freed
> + * by devm_kfree after power_seq_build is called.
> + */
> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
> +					      struct device_node *node);
> +#else
> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
> +					      struct device_node *node)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +#endif
> +
> +/**
> + * Build a runnable power sequence from platform data, and add the resources
> + * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
> + * on dev.
> + * @dev device that will use the power sequence. All resources will be
> + *      devm-allocated against it.
> + * @ress list that holds the power_seq_resources already used by this device.
> + *       Resources newly met in the sequence will be added to it.
> + * @pseq power sequence in platform format.
> + */
> +struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> +				  struct platform_power_seq *pseq);

I believe kernel-doc comments should precede the implementation, not the
prototype. Also the kernel-doc comment doesn't correspond to what is
described in Documentation/kernel-doc-nano-HOWTO.txt.

Thierry

> +
> +/**
> + * Run the given power sequence. Returns 0 on success, error code in case of
> + * failure.
> + */
> +int power_seq_run(struct power_seq *seq);
> +
> +#endif
> -- 
> 1.7.11.4
> 
> 
> 

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists