All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC][PATCHv2 0/3] Power sequences interpreter for pwm_backlight
@ 2012-07-09  6:08 ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Alexandre Courbot

This is a RFC since this patch largely drifted beyond its original goal
of supporting one GPIO and one regulator for the pwm_backlight driver.

The issue to address is that backlight power sequences, which were
implemented using board-specific callbacks so far, could not be used with
the device tree. This series of patches adds a small power sequence 
interpreter that allows to acquire and control regulators, GPIOs, and PWMs
during sequences defined in the device tree. It is easy to use,
low-footprint, and takes care of managing the resources that it acquires.

The implementation is working and should be complete, but documentation is
lacking. Also since the interpreter could be used by other drivers (which
ones?), it may make sense to have it in a better place than
drivers/video/backlight/.

The tegra device tree nodes are just here as an example usage.

Alexandre Courbot (3):
  Power sequences interpreter for device tree
  pwm-backlight: use power sequences
  tegra: add pwm backlight device tree nodes

 .../bindings/video/backlight/pwm-backlight.txt     |  28 +-
 arch/arm/boot/dts/tegra20-ventana.dts              |  31 +++
 arch/arm/boot/dts/tegra20.dtsi                     |   2 +-
 drivers/video/backlight/Makefile                   |   2 +-
 drivers/video/backlight/power_seq.c                | 298 +++++++++++++++++++++
 drivers/video/backlight/pwm_bl.c                   | 212 +++++++++++----
 include/linux/power_seq.h                          |  96 +++++++
 include/linux/pwm_backlight.h                      |  37 ++-
 8 files changed, 645 insertions(+), 61 deletions(-)
 create mode 100644 drivers/video/backlight/power_seq.c
 create mode 100644 include/linux/power_seq.h

-- 
1.7.11.1

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [RFC][PATCHv2 0/3] Power sequences interpreter for pwm_backlight
@ 2012-07-09  6:08 ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

This is a RFC since this patch largely drifted beyond its original goal
of supporting one GPIO and one regulator for the pwm_backlight driver.

The issue to address is that backlight power sequences, which were
implemented using board-specific callbacks so far, could not be used with
the device tree. This series of patches adds a small power sequence 
interpreter that allows to acquire and control regulators, GPIOs, and PWMs
during sequences defined in the device tree. It is easy to use,
low-footprint, and takes care of managing the resources that it acquires.

The implementation is working and should be complete, but documentation is
lacking. Also since the interpreter could be used by other drivers (which
ones?), it may make sense to have it in a better place than
drivers/video/backlight/.

The tegra device tree nodes are just here as an example usage.

Alexandre Courbot (3):
  Power sequences interpreter for device tree
  pwm-backlight: use power sequences
  tegra: add pwm backlight device tree nodes

 .../bindings/video/backlight/pwm-backlight.txt     |  28 +-
 arch/arm/boot/dts/tegra20-ventana.dts              |  31 +++
 arch/arm/boot/dts/tegra20.dtsi                     |   2 +-
 drivers/video/backlight/Makefile                   |   2 +-
 drivers/video/backlight/power_seq.c                | 298 +++++++++++++++++++++
 drivers/video/backlight/pwm_bl.c                   | 212 +++++++++++----
 include/linux/power_seq.h                          |  96 +++++++
 include/linux/pwm_backlight.h                      |  37 ++-
 8 files changed, 645 insertions(+), 61 deletions(-)
 create mode 100644 drivers/video/backlight/power_seq.c
 create mode 100644 include/linux/power_seq.h

-- 
1.7.11.1


^ permalink raw reply	[flat|nested] 34+ messages in thread
* [RFC][PATCHv2 0/3] Power sequences interpreter for pwm_backlight
@ 2012-07-09  6:08 ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Alexandre Courbot

This is a RFC since this patch largely drifted beyond its original goal
of supporting one GPIO and one regulator for the pwm_backlight driver.

The issue to address is that backlight power sequences, which were
implemented using board-specific callbacks so far, could not be used with
the device tree. This series of patches adds a small power sequence 
interpreter that allows to acquire and control regulators, GPIOs, and PWMs
during sequences defined in the device tree. It is easy to use,
low-footprint, and takes care of managing the resources that it acquires.

The implementation is working and should be complete, but documentation is
lacking. Also since the interpreter could be used by other drivers (which
ones?), it may make sense to have it in a better place than
drivers/video/backlight/.

The tegra device tree nodes are just here as an example usage.

Alexandre Courbot (3):
  Power sequences interpreter for device tree
  pwm-backlight: use power sequences
  tegra: add pwm backlight device tree nodes

 .../bindings/video/backlight/pwm-backlight.txt     |  28 +-
 arch/arm/boot/dts/tegra20-ventana.dts              |  31 +++
 arch/arm/boot/dts/tegra20.dtsi                     |   2 +-
 drivers/video/backlight/Makefile                   |   2 +-
 drivers/video/backlight/power_seq.c                | 298 +++++++++++++++++++++
 drivers/video/backlight/pwm_bl.c                   | 212 +++++++++++----
 include/linux/power_seq.h                          |  96 +++++++
 include/linux/pwm_backlight.h                      |  37 ++-
 8 files changed, 645 insertions(+), 61 deletions(-)
 create mode 100644 drivers/video/backlight/power_seq.c
 create mode 100644 include/linux/power_seq.h

-- 
1.7.11.1


^ permalink raw reply	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 1/3] power sequences interpreter for device tree
  2012-07-09  6:08 ` Alexandre Courbot
  (?)
@ 2012-07-09  6:08   ` Alexandre Courbot
  -1 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

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, we cannot rely of board-specific
hooks anymore, but still need a way to implement these sequences in a
portable manner. This patch introduces a simple interpreter that can
execute such power sequences encoded either as platform data or within
the device tree.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/video/backlight/Makefile    |   2 +-
 drivers/video/backlight/power_seq.c | 298 ++++++++++++++++++++++++++++++++++++
 drivers/video/backlight/pwm_bl.c    |   3 +-
 include/linux/power_seq.h           |  96 ++++++++++++
 4 files changed, 397 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/backlight/power_seq.c
 create mode 100644 include/linux/power_seq.h

diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index a2ac9cf..6bff124 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -28,7 +28,7 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)	+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)	+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PROGEAR) += progear_bl.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
-obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o
+obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o power_seq.o
 obj-$(CONFIG_BACKLIGHT_DA903X)	+= da903x_bl.o
 obj-$(CONFIG_BACKLIGHT_DA9052)	+= da9052_bl.o
 obj-$(CONFIG_BACKLIGHT_MAX8925)	+= max8925_bl.o
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
new file mode 100644
index 0000000..f54cb7d
--- /dev/null
+++ b/drivers/video/backlight/power_seq.c
@@ -0,0 +1,298 @@
+#include <linux/err.h>
+#include <linux/of_gpio.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/power_seq.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+
+#define PWM_SEQ_TYPE(type) [POWER_SEQ_ ## type] = #type
+static const char *pwm_seq_types[] = {
+	PWM_SEQ_TYPE(STOP),
+	PWM_SEQ_TYPE(DELAY),
+	PWM_SEQ_TYPE(REGULATOR),
+	PWM_SEQ_TYPE(PWM),
+	PWM_SEQ_TYPE(GPIO),
+};
+#undef PWM_SEQ_TYPE
+
+static bool power_seq_step_run(struct power_seq_step *step)
+{
+	switch (step->type) {
+	case POWER_SEQ_DELAY:
+		msleep(step->parameter);
+		break;
+	case POWER_SEQ_REGULATOR:
+		if (step->parameter)
+			regulator_enable(step->resource->regulator);
+		else
+			regulator_disable(step->resource->regulator);
+		break;
+	case POWER_SEQ_PWM:
+		if (step->parameter)
+			pwm_enable(step->resource->pwm);
+		else
+			pwm_disable(step->resource->pwm);
+		break;
+	case POWER_SEQ_GPIO:
+		gpio_set_value_cansleep(step->resource->gpio, step->parameter);
+		break;
+	/* should never happen since we verify the data when building it */
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int power_seq_run(power_seq *seq)
+{
+	int err;
+
+	if (!seq) return 0;
+
+	while (seq->type != POWER_SEQ_STOP) {
+		if ((err = power_seq_step_run(seq++))) {
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int of_parse_power_seq_step(struct device *dev, struct property *prop,
+				   struct platform_power_seq_step *seq,
+				   int max_steps)
+{
+	void *value = prop->value;
+	void *end = prop->value + prop->length;
+	int slen, smax, cpt = 0, i, ret;
+	char tmp_buf[32];
+
+	while (value < end && cpt < max_steps) {
+		smax = value - end;
+		slen = strnlen(value, end - value);
+
+		/* Unterminated string / not a string? */
+		if (slen >= end - value)
+			goto invalid_seq;
+
+		/* Find a matching sequence step type */
+		for (i = 0; i < POWER_SEQ_MAX; i++)
+			if (!strcmp(value, pwm_seq_types[i]))
+				break;
+
+		if (i >= POWER_SEQ_MAX)
+			goto unknown_step;
+
+		value += slen + 1;
+
+		seq[cpt].type = i;
+		switch (seq[cpt].type) {
+		case POWER_SEQ_DELAY:
+			/* integer parameter */
+			seq[cpt].parameter = be32_to_cpup(value);
+			value += sizeof(__be32);
+			break;
+		case POWER_SEQ_REGULATOR:
+		case POWER_SEQ_PWM:
+		case POWER_SEQ_GPIO:
+			/* consumer string */
+			slen = strnlen(value, end - value);
+			if (slen >= end - value)
+				goto invalid_seq;
+			seq[cpt].id = value;
+			value += slen + 1;
+
+			/* parameter */
+			seq[cpt].parameter = be32_to_cpup(value);
+			be32_to_cpup(value);
+			value += sizeof(__be32);
+
+			/* For GPIO we still need to resolve the phandle */
+			if (seq[cpt].type != POWER_SEQ_GPIO)
+				break;
+
+			strncpy(tmp_buf, seq[cpt].id, sizeof(tmp_buf) - 6);
+			tmp_buf[sizeof(tmp_buf) - 6] = 0;
+			strcat(tmp_buf, "-gpios");
+			ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
+			if (ret >= 0)
+				seq[cpt].value = ret;
+			else {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "cannot get gpio \"%s\"\n",
+						seq[cpt].id);
+				return ret;
+			}
+		default:
+			break;
+		}
+
+		cpt++;
+	}
+
+	if (cpt >= max_steps)
+		return -EOVERFLOW;
+
+	return 0;
+
+invalid_seq:
+	dev_err(dev, "invalid sequence \"%s\"\n", prop->name);
+		return -EINVAL;
+unknown_step:
+	dev_err(dev, "unknown step type \"%s\" in sequence \"%s\"\n",
+		(char *)value, prop->name);
+	return -EINVAL;
+}
+
+#define PWM_SEQ_MAX_LENGTH 16
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node, char *propname)
+{
+	platform_power_seq *seq = NULL;
+	struct property *prop;
+	int length;
+	int ret;
+
+	prop = of_find_property(node, propname, &length);
+	if (prop && length > 0) {
+		seq = devm_kzalloc(dev, sizeof(*seq) * PWM_SEQ_MAX_LENGTH,
+				   GFP_KERNEL);
+		if (!seq)
+			return ERR_PTR(-ENOMEM);
+		/* keep one empty entry for the STOP step */
+		ret = of_parse_power_seq_step(dev, prop, seq,
+					      PWM_SEQ_MAX_LENGTH - 1);
+		if (ret < 0)
+			return ERR_PTR(ret);
+	}
+
+	return seq;
+}
+
+static
+struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress,
+					struct platform_power_seq_step *res)
+{
+	struct power_seq_resource *step;
+
+	list_for_each_entry(step, ress, list) {
+		if (step->plat.type != res->type) continue;
+		switch (res->type) {
+		case POWER_SEQ_DELAY:
+		case POWER_SEQ_GPIO:
+			if (step->plat.value == res->value)
+				return step;
+			break;
+		default:
+			if (!strcmp(step->plat.id, res->id))
+				return step;
+			break;
+		}
+	}
+
+	return NULL;
+}
+
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq)
+{
+	struct power_seq_step *seq = NULL, *ret;
+	struct power_seq_resource *res;
+	int cpt;
+
+	/* first pass to count the number of elements */
+	for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
+
+	if (!cpt)
+		return seq;
+
+	/* 1 more for the STOP step */
+	ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL);
+	if (!seq)
+		return ERR_PTR(-ENOMEM);
+
+	for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) {
+		seq->type = pseq->type;
+
+		switch (pseq->type) {
+			case POWER_SEQ_REGULATOR:
+			case POWER_SEQ_GPIO:
+			case POWER_SEQ_PWM:
+				if (!(res = power_seq_find_resource(ress, pseq))) {
+					/* create resource node */
+					res = devm_kzalloc(dev, sizeof(*res),
+							   GFP_KERNEL);
+					if (!res)
+						return ERR_PTR(-ENOMEM);
+					memcpy(&res->plat, pseq, sizeof(*pseq));
+
+					list_add(&res->list, ress);
+				}
+				seq->resource = res;
+			case POWER_SEQ_DELAY:
+				seq->parameter = pseq->parameter;
+				break;
+			default:
+				dev_err(dev, "invalid sequence step type!\n");
+				return ERR_PTR(-EINVAL);
+		}
+	}
+
+	return ret;
+}
+
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress)
+{
+	struct power_seq_resource *res;
+	int err;
+
+	list_for_each_entry(res, ress, list) {
+		switch (res->plat.type) {
+		case POWER_SEQ_REGULATOR:
+			res->regulator = devm_regulator_get(dev, res->plat.id);
+			if (IS_ERR(res->regulator)) {
+				dev_err(dev, "cannot get regulator \"%s\"\n",
+					res->plat.id);
+				return PTR_ERR(res->regulator);
+			}
+			dev_dbg(dev, "got regulator %s\n", res->plat.id);
+			break;
+		case POWER_SEQ_PWM:
+			res->pwm = pwm_get(dev, res->plat.id);
+			if (IS_ERR(res->pwm)) {
+				dev_err(dev, "cannot get pwm \"%s\"\n",
+					res->plat.id);
+				return PTR_ERR(res->pwm);
+			}
+			dev_dbg(dev, "got PWM %s\n", res->plat.id);
+			break;
+		case POWER_SEQ_GPIO:
+			err = devm_gpio_request_one(dev, res->plat.value,
+				GPIOF_OUT_INIT_HIGH, "backlight_gpio");
+			if (err) {
+				dev_err(dev, "cannot get gpio %d\n",
+					res->plat.value);
+				return err;
+			}
+			res->gpio = res->plat.value;
+			dev_dbg(dev, "got GPIO %d\n", res->plat.value);
+			break;
+		default:
+			break;
+		};
+	}
+
+	return 0;
+}
+
+void power_seq_free_resources(power_seq_resources *ress) {
+	struct power_seq_resource *res;
+
+	list_for_each_entry(res, ress, list) {
+		if (res->plat.type == POWER_SEQ_PWM)
+			pwm_put(res->pwm);
+	}
+}
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index be48517..1a38953 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -203,8 +203,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (data->levels) {
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
-	} else
+	} else {
 		max = data->max_brightness;
+	}
 
 	pb->notify = data->notify;
 	pb->notify_after = data->notify_after;
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..7f76270
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,96 @@
+/*
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties. Mainly for use with backlight drivers.
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/of.h>
+#include <linux/types.h>
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+typedef enum {
+	POWER_SEQ_STOP = 0,
+	POWER_SEQ_DELAY,
+	POWER_SEQ_REGULATOR,
+	POWER_SEQ_PWM,
+	POWER_SEQ_GPIO,
+	POWER_SEQ_MAX,
+} seq_type;
+
+/**
+ * Describe something to do during the power-up/down sequence.
+ */
+struct platform_power_seq_step {
+	seq_type type;
+	/**
+	 * Identify the resource. Steps of type DELAY use value, others name the
+	 * consumer to use in id.
+	 */
+	union {
+		const char *id;
+		int value;
+	};
+	/**
+	 * A value of 0 disables the resource, while a non-zero enables it.
+	 * For DELAY steps this contains the delay to wait in milliseconds.
+	 */
+	int parameter;
+};
+typedef struct platform_power_seq_step platform_power_seq;
+
+struct power_seq_resource {
+	/* copied from platform data */
+	struct platform_power_seq_step plat;
+	/* resolved resource */
+	union {
+		struct regulator *regulator;
+		struct pwm_device *pwm;
+		int gpio;
+	};
+	/* used to maintain a list of resources used by the driver */
+	struct list_head list;
+};
+typedef struct list_head power_seq_resources;
+
+struct power_seq_step {
+	seq_type type;
+	int parameter;
+	struct power_seq_resource *resource;
+};
+typedef struct power_seq_step power_seq;
+
+/**
+ * Build a platform data sequence from a device tree node. Memory for the
+ * sequence is allocated using devm_kzalloc on dev.
+ */
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node,
+				       char *propname);
+/**
+ * Build a runnable power sequence from platform data, and add the resources
+ * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
+ * on dev.
+ */
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq);
+/**
+ * Allocate all resources (regulators, PWMs, GPIOs) found by calls to
+ * power_seq_build() for use by dev. Return 0 in case of success, error code
+ * otherwise.
+ */
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress);
+/**
+ * Free all the resources previously allocated by power_seq_allocate_resources.
+ */
+void power_seq_free_resources(power_seq_resources *ress);
+/**
+ * Run the given power sequence. Returns 0 on success, error code in case of
+ * failure.
+ */
+int power_seq_run(power_seq *seq);
+
+#endif
-- 
1.7.11.1

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 1/3] power sequences interpreter for device tree
@ 2012-07-09  6:08   ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

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, we cannot rely of board-specific
hooks anymore, but still need a way to implement these sequences in a
portable manner. This patch introduces a simple interpreter that can
execute such power sequences encoded either as platform data or within
the device tree.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/video/backlight/Makefile    |   2 +-
 drivers/video/backlight/power_seq.c | 298 ++++++++++++++++++++++++++++++++++++
 drivers/video/backlight/pwm_bl.c    |   3 +-
 include/linux/power_seq.h           |  96 ++++++++++++
 4 files changed, 397 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/backlight/power_seq.c
 create mode 100644 include/linux/power_seq.h

diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index a2ac9cf..6bff124 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -28,7 +28,7 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)	+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)	+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PROGEAR) += progear_bl.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
-obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o
+obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o power_seq.o
 obj-$(CONFIG_BACKLIGHT_DA903X)	+= da903x_bl.o
 obj-$(CONFIG_BACKLIGHT_DA9052)	+= da9052_bl.o
 obj-$(CONFIG_BACKLIGHT_MAX8925)	+= max8925_bl.o
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
new file mode 100644
index 0000000..f54cb7d
--- /dev/null
+++ b/drivers/video/backlight/power_seq.c
@@ -0,0 +1,298 @@
+#include <linux/err.h>
+#include <linux/of_gpio.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/power_seq.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+
+#define PWM_SEQ_TYPE(type) [POWER_SEQ_ ## type] = #type
+static const char *pwm_seq_types[] = {
+	PWM_SEQ_TYPE(STOP),
+	PWM_SEQ_TYPE(DELAY),
+	PWM_SEQ_TYPE(REGULATOR),
+	PWM_SEQ_TYPE(PWM),
+	PWM_SEQ_TYPE(GPIO),
+};
+#undef PWM_SEQ_TYPE
+
+static bool power_seq_step_run(struct power_seq_step *step)
+{
+	switch (step->type) {
+	case POWER_SEQ_DELAY:
+		msleep(step->parameter);
+		break;
+	case POWER_SEQ_REGULATOR:
+		if (step->parameter)
+			regulator_enable(step->resource->regulator);
+		else
+			regulator_disable(step->resource->regulator);
+		break;
+	case POWER_SEQ_PWM:
+		if (step->parameter)
+			pwm_enable(step->resource->pwm);
+		else
+			pwm_disable(step->resource->pwm);
+		break;
+	case POWER_SEQ_GPIO:
+		gpio_set_value_cansleep(step->resource->gpio, step->parameter);
+		break;
+	/* should never happen since we verify the data when building it */
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int power_seq_run(power_seq *seq)
+{
+	int err;
+
+	if (!seq) return 0;
+
+	while (seq->type != POWER_SEQ_STOP) {
+		if ((err = power_seq_step_run(seq++))) {
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int of_parse_power_seq_step(struct device *dev, struct property *prop,
+				   struct platform_power_seq_step *seq,
+				   int max_steps)
+{
+	void *value = prop->value;
+	void *end = prop->value + prop->length;
+	int slen, smax, cpt = 0, i, ret;
+	char tmp_buf[32];
+
+	while (value < end && cpt < max_steps) {
+		smax = value - end;
+		slen = strnlen(value, end - value);
+
+		/* Unterminated string / not a string? */
+		if (slen >= end - value)
+			goto invalid_seq;
+
+		/* Find a matching sequence step type */
+		for (i = 0; i < POWER_SEQ_MAX; i++)
+			if (!strcmp(value, pwm_seq_types[i]))
+				break;
+
+		if (i >= POWER_SEQ_MAX)
+			goto unknown_step;
+
+		value += slen + 1;
+
+		seq[cpt].type = i;
+		switch (seq[cpt].type) {
+		case POWER_SEQ_DELAY:
+			/* integer parameter */
+			seq[cpt].parameter = be32_to_cpup(value);
+			value += sizeof(__be32);
+			break;
+		case POWER_SEQ_REGULATOR:
+		case POWER_SEQ_PWM:
+		case POWER_SEQ_GPIO:
+			/* consumer string */
+			slen = strnlen(value, end - value);
+			if (slen >= end - value)
+				goto invalid_seq;
+			seq[cpt].id = value;
+			value += slen + 1;
+
+			/* parameter */
+			seq[cpt].parameter = be32_to_cpup(value);
+			be32_to_cpup(value);
+			value += sizeof(__be32);
+
+			/* For GPIO we still need to resolve the phandle */
+			if (seq[cpt].type != POWER_SEQ_GPIO)
+				break;
+
+			strncpy(tmp_buf, seq[cpt].id, sizeof(tmp_buf) - 6);
+			tmp_buf[sizeof(tmp_buf) - 6] = 0;
+			strcat(tmp_buf, "-gpios");
+			ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
+			if (ret >= 0)
+				seq[cpt].value = ret;
+			else {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "cannot get gpio \"%s\"\n",
+						seq[cpt].id);
+				return ret;
+			}
+		default:
+			break;
+		}
+
+		cpt++;
+	}
+
+	if (cpt >= max_steps)
+		return -EOVERFLOW;
+
+	return 0;
+
+invalid_seq:
+	dev_err(dev, "invalid sequence \"%s\"\n", prop->name);
+		return -EINVAL;
+unknown_step:
+	dev_err(dev, "unknown step type \"%s\" in sequence \"%s\"\n",
+		(char *)value, prop->name);
+	return -EINVAL;
+}
+
+#define PWM_SEQ_MAX_LENGTH 16
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node, char *propname)
+{
+	platform_power_seq *seq = NULL;
+	struct property *prop;
+	int length;
+	int ret;
+
+	prop = of_find_property(node, propname, &length);
+	if (prop && length > 0) {
+		seq = devm_kzalloc(dev, sizeof(*seq) * PWM_SEQ_MAX_LENGTH,
+				   GFP_KERNEL);
+		if (!seq)
+			return ERR_PTR(-ENOMEM);
+		/* keep one empty entry for the STOP step */
+		ret = of_parse_power_seq_step(dev, prop, seq,
+					      PWM_SEQ_MAX_LENGTH - 1);
+		if (ret < 0)
+			return ERR_PTR(ret);
+	}
+
+	return seq;
+}
+
+static
+struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress,
+					struct platform_power_seq_step *res)
+{
+	struct power_seq_resource *step;
+
+	list_for_each_entry(step, ress, list) {
+		if (step->plat.type != res->type) continue;
+		switch (res->type) {
+		case POWER_SEQ_DELAY:
+		case POWER_SEQ_GPIO:
+			if (step->plat.value == res->value)
+				return step;
+			break;
+		default:
+			if (!strcmp(step->plat.id, res->id))
+				return step;
+			break;
+		}
+	}
+
+	return NULL;
+}
+
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq)
+{
+	struct power_seq_step *seq = NULL, *ret;
+	struct power_seq_resource *res;
+	int cpt;
+
+	/* first pass to count the number of elements */
+	for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
+
+	if (!cpt)
+		return seq;
+
+	/* 1 more for the STOP step */
+	ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL);
+	if (!seq)
+		return ERR_PTR(-ENOMEM);
+
+	for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) {
+		seq->type = pseq->type;
+
+		switch (pseq->type) {
+			case POWER_SEQ_REGULATOR:
+			case POWER_SEQ_GPIO:
+			case POWER_SEQ_PWM:
+				if (!(res = power_seq_find_resource(ress, pseq))) {
+					/* create resource node */
+					res = devm_kzalloc(dev, sizeof(*res),
+							   GFP_KERNEL);
+					if (!res)
+						return ERR_PTR(-ENOMEM);
+					memcpy(&res->plat, pseq, sizeof(*pseq));
+
+					list_add(&res->list, ress);
+				}
+				seq->resource = res;
+			case POWER_SEQ_DELAY:
+				seq->parameter = pseq->parameter;
+				break;
+			default:
+				dev_err(dev, "invalid sequence step type!\n");
+				return ERR_PTR(-EINVAL);
+		}
+	}
+
+	return ret;
+}
+
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress)
+{
+	struct power_seq_resource *res;
+	int err;
+
+	list_for_each_entry(res, ress, list) {
+		switch (res->plat.type) {
+		case POWER_SEQ_REGULATOR:
+			res->regulator = devm_regulator_get(dev, res->plat.id);
+			if (IS_ERR(res->regulator)) {
+				dev_err(dev, "cannot get regulator \"%s\"\n",
+					res->plat.id);
+				return PTR_ERR(res->regulator);
+			}
+			dev_dbg(dev, "got regulator %s\n", res->plat.id);
+			break;
+		case POWER_SEQ_PWM:
+			res->pwm = pwm_get(dev, res->plat.id);
+			if (IS_ERR(res->pwm)) {
+				dev_err(dev, "cannot get pwm \"%s\"\n",
+					res->plat.id);
+				return PTR_ERR(res->pwm);
+			}
+			dev_dbg(dev, "got PWM %s\n", res->plat.id);
+			break;
+		case POWER_SEQ_GPIO:
+			err = devm_gpio_request_one(dev, res->plat.value,
+				GPIOF_OUT_INIT_HIGH, "backlight_gpio");
+			if (err) {
+				dev_err(dev, "cannot get gpio %d\n",
+					res->plat.value);
+				return err;
+			}
+			res->gpio = res->plat.value;
+			dev_dbg(dev, "got GPIO %d\n", res->plat.value);
+			break;
+		default:
+			break;
+		};
+	}
+
+	return 0;
+}
+
+void power_seq_free_resources(power_seq_resources *ress) {
+	struct power_seq_resource *res;
+
+	list_for_each_entry(res, ress, list) {
+		if (res->plat.type == POWER_SEQ_PWM)
+			pwm_put(res->pwm);
+	}
+}
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index be48517..1a38953 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -203,8 +203,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (data->levels) {
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
-	} else
+	} else {
 		max = data->max_brightness;
+	}
 
 	pb->notify = data->notify;
 	pb->notify_after = data->notify_after;
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..7f76270
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,96 @@
+/*
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties. Mainly for use with backlight drivers.
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/of.h>
+#include <linux/types.h>
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+typedef enum {
+	POWER_SEQ_STOP = 0,
+	POWER_SEQ_DELAY,
+	POWER_SEQ_REGULATOR,
+	POWER_SEQ_PWM,
+	POWER_SEQ_GPIO,
+	POWER_SEQ_MAX,
+} seq_type;
+
+/**
+ * Describe something to do during the power-up/down sequence.
+ */
+struct platform_power_seq_step {
+	seq_type type;
+	/**
+	 * Identify the resource. Steps of type DELAY use value, others name the
+	 * consumer to use in id.
+	 */
+	union {
+		const char *id;
+		int value;
+	};
+	/**
+	 * A value of 0 disables the resource, while a non-zero enables it.
+	 * For DELAY steps this contains the delay to wait in milliseconds.
+	 */
+	int parameter;
+};
+typedef struct platform_power_seq_step platform_power_seq;
+
+struct power_seq_resource {
+	/* copied from platform data */
+	struct platform_power_seq_step plat;
+	/* resolved resource */
+	union {
+		struct regulator *regulator;
+		struct pwm_device *pwm;
+		int gpio;
+	};
+	/* used to maintain a list of resources used by the driver */
+	struct list_head list;
+};
+typedef struct list_head power_seq_resources;
+
+struct power_seq_step {
+	seq_type type;
+	int parameter;
+	struct power_seq_resource *resource;
+};
+typedef struct power_seq_step power_seq;
+
+/**
+ * Build a platform data sequence from a device tree node. Memory for the
+ * sequence is allocated using devm_kzalloc on dev.
+ */
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node,
+				       char *propname);
+/**
+ * Build a runnable power sequence from platform data, and add the resources
+ * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
+ * on dev.
+ */
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq);
+/**
+ * Allocate all resources (regulators, PWMs, GPIOs) found by calls to
+ * power_seq_build() for use by dev. Return 0 in case of success, error code
+ * otherwise.
+ */
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress);
+/**
+ * Free all the resources previously allocated by power_seq_allocate_resources.
+ */
+void power_seq_free_resources(power_seq_resources *ress);
+/**
+ * Run the given power sequence. Returns 0 on success, error code in case of
+ * failure.
+ */
+int power_seq_run(power_seq *seq);
+
+#endif
-- 
1.7.11.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 1/3] power sequences interpreter for device tree
@ 2012-07-09  6:08   ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

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, we cannot rely of board-specific
hooks anymore, but still need a way to implement these sequences in a
portable manner. This patch introduces a simple interpreter that can
execute such power sequences encoded either as platform data or within
the device tree.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/video/backlight/Makefile    |   2 +-
 drivers/video/backlight/power_seq.c | 298 ++++++++++++++++++++++++++++++++++++
 drivers/video/backlight/pwm_bl.c    |   3 +-
 include/linux/power_seq.h           |  96 ++++++++++++
 4 files changed, 397 insertions(+), 2 deletions(-)
 create mode 100644 drivers/video/backlight/power_seq.c
 create mode 100644 include/linux/power_seq.h

diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index a2ac9cf..6bff124 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -28,7 +28,7 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)	+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)	+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PROGEAR) += progear_bl.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
-obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o
+obj-$(CONFIG_BACKLIGHT_PWM)	+= pwm_bl.o power_seq.o
 obj-$(CONFIG_BACKLIGHT_DA903X)	+= da903x_bl.o
 obj-$(CONFIG_BACKLIGHT_DA9052)	+= da9052_bl.o
 obj-$(CONFIG_BACKLIGHT_MAX8925)	+= max8925_bl.o
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
new file mode 100644
index 0000000..f54cb7d
--- /dev/null
+++ b/drivers/video/backlight/power_seq.c
@@ -0,0 +1,298 @@
+#include <linux/err.h>
+#include <linux/of_gpio.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/power_seq.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+
+#define PWM_SEQ_TYPE(type) [POWER_SEQ_ ## type] = #type
+static const char *pwm_seq_types[] = {
+	PWM_SEQ_TYPE(STOP),
+	PWM_SEQ_TYPE(DELAY),
+	PWM_SEQ_TYPE(REGULATOR),
+	PWM_SEQ_TYPE(PWM),
+	PWM_SEQ_TYPE(GPIO),
+};
+#undef PWM_SEQ_TYPE
+
+static bool power_seq_step_run(struct power_seq_step *step)
+{
+	switch (step->type) {
+	case POWER_SEQ_DELAY:
+		msleep(step->parameter);
+		break;
+	case POWER_SEQ_REGULATOR:
+		if (step->parameter)
+			regulator_enable(step->resource->regulator);
+		else
+			regulator_disable(step->resource->regulator);
+		break;
+	case POWER_SEQ_PWM:
+		if (step->parameter)
+			pwm_enable(step->resource->pwm);
+		else
+			pwm_disable(step->resource->pwm);
+		break;
+	case POWER_SEQ_GPIO:
+		gpio_set_value_cansleep(step->resource->gpio, step->parameter);
+		break;
+	/* should never happen since we verify the data when building it */
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int power_seq_run(power_seq *seq)
+{
+	int err;
+
+	if (!seq) return 0;
+
+	while (seq->type != POWER_SEQ_STOP) {
+		if ((err = power_seq_step_run(seq++))) {
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int of_parse_power_seq_step(struct device *dev, struct property *prop,
+				   struct platform_power_seq_step *seq,
+				   int max_steps)
+{
+	void *value = prop->value;
+	void *end = prop->value + prop->length;
+	int slen, smax, cpt = 0, i, ret;
+	char tmp_buf[32];
+
+	while (value < end && cpt < max_steps) {
+		smax = value - end;
+		slen = strnlen(value, end - value);
+
+		/* Unterminated string / not a string? */
+		if (slen >= end - value)
+			goto invalid_seq;
+
+		/* Find a matching sequence step type */
+		for (i = 0; i < POWER_SEQ_MAX; i++)
+			if (!strcmp(value, pwm_seq_types[i]))
+				break;
+
+		if (i >= POWER_SEQ_MAX)
+			goto unknown_step;
+
+		value += slen + 1;
+
+		seq[cpt].type = i;
+		switch (seq[cpt].type) {
+		case POWER_SEQ_DELAY:
+			/* integer parameter */
+			seq[cpt].parameter = be32_to_cpup(value);
+			value += sizeof(__be32);
+			break;
+		case POWER_SEQ_REGULATOR:
+		case POWER_SEQ_PWM:
+		case POWER_SEQ_GPIO:
+			/* consumer string */
+			slen = strnlen(value, end - value);
+			if (slen >= end - value)
+				goto invalid_seq;
+			seq[cpt].id = value;
+			value += slen + 1;
+
+			/* parameter */
+			seq[cpt].parameter = be32_to_cpup(value);
+			be32_to_cpup(value);
+			value += sizeof(__be32);
+
+			/* For GPIO we still need to resolve the phandle */
+			if (seq[cpt].type != POWER_SEQ_GPIO)
+				break;
+
+			strncpy(tmp_buf, seq[cpt].id, sizeof(tmp_buf) - 6);
+			tmp_buf[sizeof(tmp_buf) - 6] = 0;
+			strcat(tmp_buf, "-gpios");
+			ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
+			if (ret >= 0)
+				seq[cpt].value = ret;
+			else {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "cannot get gpio \"%s\"\n",
+						seq[cpt].id);
+				return ret;
+			}
+		default:
+			break;
+		}
+
+		cpt++;
+	}
+
+	if (cpt >= max_steps)
+		return -EOVERFLOW;
+
+	return 0;
+
+invalid_seq:
+	dev_err(dev, "invalid sequence \"%s\"\n", prop->name);
+		return -EINVAL;
+unknown_step:
+	dev_err(dev, "unknown step type \"%s\" in sequence \"%s\"\n",
+		(char *)value, prop->name);
+	return -EINVAL;
+}
+
+#define PWM_SEQ_MAX_LENGTH 16
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node, char *propname)
+{
+	platform_power_seq *seq = NULL;
+	struct property *prop;
+	int length;
+	int ret;
+
+	prop = of_find_property(node, propname, &length);
+	if (prop && length > 0) {
+		seq = devm_kzalloc(dev, sizeof(*seq) * PWM_SEQ_MAX_LENGTH,
+				   GFP_KERNEL);
+		if (!seq)
+			return ERR_PTR(-ENOMEM);
+		/* keep one empty entry for the STOP step */
+		ret = of_parse_power_seq_step(dev, prop, seq,
+					      PWM_SEQ_MAX_LENGTH - 1);
+		if (ret < 0)
+			return ERR_PTR(ret);
+	}
+
+	return seq;
+}
+
+static
+struct power_seq_resource * power_seq_find_resource(power_seq_resources *ress,
+					struct platform_power_seq_step *res)
+{
+	struct power_seq_resource *step;
+
+	list_for_each_entry(step, ress, list) {
+		if (step->plat.type != res->type) continue;
+		switch (res->type) {
+		case POWER_SEQ_DELAY:
+		case POWER_SEQ_GPIO:
+			if (step->plat.value = res->value)
+				return step;
+			break;
+		default:
+			if (!strcmp(step->plat.id, res->id))
+				return step;
+			break;
+		}
+	}
+
+	return NULL;
+}
+
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq)
+{
+	struct power_seq_step *seq = NULL, *ret;
+	struct power_seq_resource *res;
+	int cpt;
+
+	/* first pass to count the number of elements */
+	for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
+
+	if (!cpt)
+		return seq;
+
+	/* 1 more for the STOP step */
+	ret = seq = devm_kzalloc(dev, sizeof(*seq) * (cpt + 1), GFP_KERNEL);
+	if (!seq)
+		return ERR_PTR(-ENOMEM);
+
+	for (; pseq->type != POWER_SEQ_STOP; pseq++, seq++) {
+		seq->type = pseq->type;
+
+		switch (pseq->type) {
+			case POWER_SEQ_REGULATOR:
+			case POWER_SEQ_GPIO:
+			case POWER_SEQ_PWM:
+				if (!(res = power_seq_find_resource(ress, pseq))) {
+					/* create resource node */
+					res = devm_kzalloc(dev, sizeof(*res),
+							   GFP_KERNEL);
+					if (!res)
+						return ERR_PTR(-ENOMEM);
+					memcpy(&res->plat, pseq, sizeof(*pseq));
+
+					list_add(&res->list, ress);
+				}
+				seq->resource = res;
+			case POWER_SEQ_DELAY:
+				seq->parameter = pseq->parameter;
+				break;
+			default:
+				dev_err(dev, "invalid sequence step type!\n");
+				return ERR_PTR(-EINVAL);
+		}
+	}
+
+	return ret;
+}
+
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress)
+{
+	struct power_seq_resource *res;
+	int err;
+
+	list_for_each_entry(res, ress, list) {
+		switch (res->plat.type) {
+		case POWER_SEQ_REGULATOR:
+			res->regulator = devm_regulator_get(dev, res->plat.id);
+			if (IS_ERR(res->regulator)) {
+				dev_err(dev, "cannot get regulator \"%s\"\n",
+					res->plat.id);
+				return PTR_ERR(res->regulator);
+			}
+			dev_dbg(dev, "got regulator %s\n", res->plat.id);
+			break;
+		case POWER_SEQ_PWM:
+			res->pwm = pwm_get(dev, res->plat.id);
+			if (IS_ERR(res->pwm)) {
+				dev_err(dev, "cannot get pwm \"%s\"\n",
+					res->plat.id);
+				return PTR_ERR(res->pwm);
+			}
+			dev_dbg(dev, "got PWM %s\n", res->plat.id);
+			break;
+		case POWER_SEQ_GPIO:
+			err = devm_gpio_request_one(dev, res->plat.value,
+				GPIOF_OUT_INIT_HIGH, "backlight_gpio");
+			if (err) {
+				dev_err(dev, "cannot get gpio %d\n",
+					res->plat.value);
+				return err;
+			}
+			res->gpio = res->plat.value;
+			dev_dbg(dev, "got GPIO %d\n", res->plat.value);
+			break;
+		default:
+			break;
+		};
+	}
+
+	return 0;
+}
+
+void power_seq_free_resources(power_seq_resources *ress) {
+	struct power_seq_resource *res;
+
+	list_for_each_entry(res, ress, list) {
+		if (res->plat.type = POWER_SEQ_PWM)
+			pwm_put(res->pwm);
+	}
+}
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index be48517..1a38953 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -203,8 +203,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (data->levels) {
 		max = data->levels[data->max_brightness];
 		pb->levels = data->levels;
-	} else
+	} else {
 		max = data->max_brightness;
+	}
 
 	pb->notify = data->notify;
 	pb->notify_after = data->notify_after;
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..7f76270
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,96 @@
+/*
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties. Mainly for use with backlight drivers.
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/of.h>
+#include <linux/types.h>
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+typedef enum {
+	POWER_SEQ_STOP = 0,
+	POWER_SEQ_DELAY,
+	POWER_SEQ_REGULATOR,
+	POWER_SEQ_PWM,
+	POWER_SEQ_GPIO,
+	POWER_SEQ_MAX,
+} seq_type;
+
+/**
+ * Describe something to do during the power-up/down sequence.
+ */
+struct platform_power_seq_step {
+	seq_type type;
+	/**
+	 * Identify the resource. Steps of type DELAY use value, others name the
+	 * consumer to use in id.
+	 */
+	union {
+		const char *id;
+		int value;
+	};
+	/**
+	 * A value of 0 disables the resource, while a non-zero enables it.
+	 * For DELAY steps this contains the delay to wait in milliseconds.
+	 */
+	int parameter;
+};
+typedef struct platform_power_seq_step platform_power_seq;
+
+struct power_seq_resource {
+	/* copied from platform data */
+	struct platform_power_seq_step plat;
+	/* resolved resource */
+	union {
+		struct regulator *regulator;
+		struct pwm_device *pwm;
+		int gpio;
+	};
+	/* used to maintain a list of resources used by the driver */
+	struct list_head list;
+};
+typedef struct list_head power_seq_resources;
+
+struct power_seq_step {
+	seq_type type;
+	int parameter;
+	struct power_seq_resource *resource;
+};
+typedef struct power_seq_step power_seq;
+
+/**
+ * Build a platform data sequence from a device tree node. Memory for the
+ * sequence is allocated using devm_kzalloc on dev.
+ */
+platform_power_seq *of_parse_power_seq(struct device *dev,
+				       struct device_node *node,
+				       char *propname);
+/**
+ * Build a runnable power sequence from platform data, and add the resources
+ * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
+ * on dev.
+ */
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+			   platform_power_seq *pseq);
+/**
+ * Allocate all resources (regulators, PWMs, GPIOs) found by calls to
+ * power_seq_build() for use by dev. Return 0 in case of success, error code
+ * otherwise.
+ */
+int power_seq_allocate_resources(struct device *dev, power_seq_resources *ress);
+/**
+ * Free all the resources previously allocated by power_seq_allocate_resources.
+ */
+void power_seq_free_resources(power_seq_resources *ress);
+/**
+ * Run the given power sequence. Returns 0 on success, error code in case of
+ * failure.
+ */
+int power_seq_run(power_seq *seq);
+
+#endif
-- 
1.7.11.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
  2012-07-09  6:08 ` Alexandre Courbot
  (?)
@ 2012-07-09  6:08   ` Alexandre Courbot
  -1 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

Make use of the power sequences specified in the device tree or platform
data, if any.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../bindings/video/backlight/pwm-backlight.txt     |  28 ++-
 drivers/video/backlight/power_seq.c                |  44 ++---
 drivers/video/backlight/pwm_bl.c                   | 210 +++++++++++++++------
 include/linux/pwm_backlight.h                      |  37 +++-
 4 files changed, 239 insertions(+), 80 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..86c9253 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -2,7 +2,10 @@ pwm-backlight bindings
 
 Required properties:
   - compatible: "pwm-backlight"
-  - pwms: OF device-tree PWM specification (see PWM binding[0])
+  - pwms: OF device-tree PWM specification (see PWM binding[0]). Exactly one PWM
+      must be specified
+  - pwm-names: a list of names for the PWM devices specified in the
+      "pwms" property (see PWM binding[0])
   - brightness-levels: Array of distinct brightness levels. Typically these
       are in the range from 0 to 255, but any range starting at 0 will do.
       The actual brightness level (PWM duty cycle) will be interpolated
@@ -10,10 +13,18 @@ Required properties:
       last value in the array represents a 100% duty cycle (brightest).
   - default-brightness-level: the default brightness level (index into the
       array defined by the "brightness-levels" property)
+  - power-on-sequence: Power sequence that will bring the backlight on. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
+  - power-off-sequence: Power sequence that will bring the backlight off. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
 
 Optional properties:
-  - pwm-names: a list of names for the PWM devices specified in the
-               "pwms" property (see PWM binding[0])
+  - *-supply: a reference to a regulator used within a power sequence
+  - *-gpios: a reference to a GPIO used within a power sequence.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -22,7 +33,18 @@ Example:
 	backlight {
 		compatible = "pwm-backlight";
 		pwms = <&pwm 0 5000000>;
+		pwm-names = "backlight";
 
 		brightness-levels = <0 4 8 16 32 64 128 255>;
 		default-brightness-level = <6>;
+		power-supply = <&backlight_reg>;
+		enable-gpios = <&gpio 6 0>;
+		power-on-sequence = "REGULATOR", "power", <1>,
+				    "DELAY", <10>,
+				    "PWM", "backlight", <1>,
+				    "GPIO", "enable", <1>;
+		power-off-sequence = "GPIO", "enable", <0>,
+				     "PWM", "backlight", <0>,
+				     "DELAY", <10>,
+				     "REGULATOR", "power", <0>;
 	};
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
index f54cb7d..f8737db 100644
--- a/drivers/video/backlight/power_seq.c
+++ b/drivers/video/backlight/power_seq.c
@@ -118,9 +118,9 @@ static int of_parse_power_seq_step(struct device *dev, struct property *prop,
 			tmp_buf[sizeof(tmp_buf) - 6] = 0;
 			strcat(tmp_buf, "-gpios");
 			ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
-			if (ret >= 0)
+			if (ret >= 0) {
 				seq[cpt].value = ret;
-			else {
+			} else {
 				if (ret != -EPROBE_DEFER)
 					dev_err(dev, "cannot get gpio \"%s\"\n",
 						seq[cpt].id);
@@ -218,26 +218,26 @@ power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
 		seq->type = pseq->type;
 
 		switch (pseq->type) {
-			case POWER_SEQ_REGULATOR:
-			case POWER_SEQ_GPIO:
-			case POWER_SEQ_PWM:
-				if (!(res = power_seq_find_resource(ress, pseq))) {
-					/* create resource node */
-					res = devm_kzalloc(dev, sizeof(*res),
-							   GFP_KERNEL);
-					if (!res)
-						return ERR_PTR(-ENOMEM);
-					memcpy(&res->plat, pseq, sizeof(*pseq));
-
-					list_add(&res->list, ress);
-				}
-				seq->resource = res;
-			case POWER_SEQ_DELAY:
-				seq->parameter = pseq->parameter;
-				break;
-			default:
-				dev_err(dev, "invalid sequence step type!\n");
-				return ERR_PTR(-EINVAL);
+		case POWER_SEQ_REGULATOR:
+		case POWER_SEQ_GPIO:
+		case POWER_SEQ_PWM:
+			if (!(res = power_seq_find_resource(ress, pseq))) {
+				/* create resource node */
+				res = devm_kzalloc(dev, sizeof(*res),
+						   GFP_KERNEL);
+				if (!res)
+					return ERR_PTR(-ENOMEM);
+				memcpy(&res->plat, pseq, sizeof(*pseq));
+
+				list_add(&res->list, ress);
+			}
+			seq->resource = res;
+		case POWER_SEQ_DELAY:
+			seq->parameter = pseq->parameter;
+			break;
+		default:
+			dev_err(dev, "invalid sequence step type!\n");
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1a38953..2936819 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,12 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	bool			enabled;
+	power_seq_resources	resources;
+	power_seq		*power_on_seq;
+	power_seq		*power_off_seq;
+
+	/* Legacy callbacks */
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -35,6 +41,34 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static void pwm_backlight_on(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (pb->enabled)
+		return;
+
+	if ((ret = power_seq_run(pb->power_on_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power on sequence\n");
+
+	pb->enabled = true;
+}
+
+static void pwm_backlight_off(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (!pb->enabled)
+		return;
+
+	if ((ret = power_seq_run(pb->power_off_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power off sequence\n");
+
+	pb->enabled = false;
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +85,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		brightness = pb->notify(pb->dev, brightness);
 
 	if (brightness == 0) {
-		pwm_config(pb->pwm, 0, pb->period);
-		pwm_disable(pb->pwm);
+		pwm_backlight_off(bl);
 	} else {
 		int duty_cycle;
 		if (pb->levels) {
@@ -144,12 +177,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		data->max_brightness--;
 	}
 
-	/*
-	 * TODO: Most users of this driver use a number of GPIOs to control
-	 *       backlight power. Support for specifying these needs to be
-	 *       added.
-	 */
+	data->power_on_seq = of_parse_power_seq(dev, node, "power-on-sequence");
+	if (IS_ERR(data->power_on_seq))
+		return PTR_ERR(data->power_on_seq);
+	data->power_off_seq = of_parse_power_seq(dev, node,
+						 "power-off-sequence");
+	if (IS_ERR(data->power_off_seq))
+		return PTR_ERR(data->power_off_seq);
 
+	data->use_power_sequences = true;
 	return 0;
 }
 
@@ -167,37 +203,134 @@ static int pwm_backlight_parse_dt(struct device *dev,
 }
 #endif
 
+/**
+ * Construct the power sequences corresponding to the legacy platform data.
+ */
+static int pwm_backlight_legacy_probe(struct platform_device *pdev,
+				      struct pwm_bl_data *pb)
+{
+	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct power_seq_resource *res;
+	struct power_seq_step *step;
+
+	pb->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(pb->pwm)) {
+		dev_warn(dev, "unable to request PWM, trying legacy API\n");
+
+		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
+		if (IS_ERR(pb->pwm)) {
+			dev_err(dev, "unable to request legacy PWM\n");
+			return PTR_ERR(pb->pwm);
+		}
+		pwm_set_period(pb->pwm, data->pwm_period_ns);
+	}
+
+	pb->notify = data->notify;
+	pb->notify_after = data->notify_after;
+	pb->check_fb = data->check_fb;
+	pb->exit = data->exit;
+	pb->dev = dev;
+
+	/* Now build the resources and sequences corresponding to this PWM */
+	res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+	if (!res) return -ENOMEM;
+	res->plat.type = POWER_SEQ_PWM;
+	res->plat.id = "pwm-backlight";
+	res->pwm = pb->pwm;
+	list_add(&res->list, &pb->resources);
+
+	/* allocate both power on and off sequences at the same time */
+	step = devm_kzalloc(dev, sizeof(*step) * 4, GFP_KERNEL);
+	if (!step) return -ENOMEM;
+	step->type = POWER_SEQ_PWM;
+	step->resource = res;
+	memcpy(&step[2], &step[0], sizeof(*step));
+	step[0].parameter = 1;
+	pb->power_on_seq = &step[0];
+	pb->power_off_seq = &step[2];
+
+	return 0;
+}
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
 	struct platform_pwm_backlight_data defdata;
+	struct power_seq_resource *res;
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
 	unsigned int max;
 	int ret;
 
+	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
+	if (!pb) {
+		dev_err(&pdev->dev, "no memory for state\n");
+		return -ENOMEM;
+	}
+
+	INIT_LIST_HEAD(&pb->resources);
+
+	/* using new interface or device tree */
 	if (!data) {
+		/* build platform data from device tree */
 		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
-		if (ret < 0) {
+		if (ret == -EPROBE_DEFER) {
+			return ret;
+		} else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to find platform data\n");
 			return ret;
 		}
-
 		data = &defdata;
 	}
 
-	if (data->init) {
-		ret = data->init(&pdev->dev);
+	if (!data->use_power_sequences) {
+		/* using legacy interface */
+		ret = pwm_backlight_legacy_probe(pdev, pb);
+		if (ret < 0)
+			return ret;
+	} else {
+		/* build sequences and allocate resources from platform data */
+		if (data->power_on_seq) {
+			pb->power_on_seq = power_seq_build(&pdev->dev, &pb->resources,
+							   data->power_on_seq);
+			if (IS_ERR(pb->power_on_seq))
+				return PTR_ERR(pb->power_on_seq);
+		}
+		if (data->power_off_seq) {
+			pb->power_off_seq = power_seq_build(&pdev->dev, &pb->resources,
+							   data->power_off_seq);
+			if (IS_ERR(pb->power_off_seq))
+				return PTR_ERR(pb->power_off_seq);
+		}
+		ret = power_seq_allocate_resources(&pdev->dev, &pb->resources);
 		if (ret < 0)
 			return ret;
+
+		/* we must have exactly one PWM for this driver */
+		list_for_each_entry(res, &pb->resources, list) {
+			if (res->plat.type != POWER_SEQ_PWM)
+				continue;
+			if (pb->pwm) {
+				dev_err(&pdev->dev, "cannot use more than one PWM\n");
+				return -EINVAL;
+			}
+			/* keep the pwm at hand */
+			pb->pwm = res->pwm;
+		}
 	}
 
-	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
-	if (!pb) {
-		dev_err(&pdev->dev, "no memory for state\n");
-		ret = -ENOMEM;
-		goto err_alloc;
+	/* from here we should have a PWM */
+	if (!pb->pwm) {
+		dev_err(&pdev->dev, "no PWM defined!\n");
+		return -EINVAL;
+	}
+
+	if (data->init) {
+		ret = data->init(&pdev->dev);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (data->levels) {
@@ -207,34 +340,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		max = data->max_brightness;
 	}
 
-	pb->notify = data->notify;
-	pb->notify_after = data->notify_after;
-	pb->check_fb = data->check_fb;
-	pb->exit = data->exit;
-	pb->dev = &pdev->dev;
-
-	pb->pwm = pwm_get(&pdev->dev, NULL);
-	if (IS_ERR(pb->pwm)) {
-		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
-
-		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
-		if (IS_ERR(pb->pwm)) {
-			dev_err(&pdev->dev, "unable to request legacy PWM\n");
-			ret = PTR_ERR(pb->pwm);
-			goto err_alloc;
-		}
-	}
-
-	dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
-	/*
-	 * The DT case will set the pwm_period_ns field to 0 and store the
-	 * period, parsed from the DT, in the PWM device. For the non-DT case,
-	 * set the period from platform data.
-	 */
-	if (data->pwm_period_ns > 0)
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-
 	pb->period = pwm_get_period(pb->pwm);
 	pb->lth_brightness = data->lth_brightness * (pb->period / max);
 
@@ -246,20 +351,20 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (IS_ERR(bl)) {
 		dev_err(&pdev->dev, "failed to register backlight\n");
 		ret = PTR_ERR(bl);
-		goto err_bl;
+		goto err;
 	}
 
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);
+
 	return 0;
 
-err_bl:
-	pwm_put(pb->pwm);
-err_alloc:
+err:
 	if (data->exit)
 		data->exit(&pdev->dev);
+	power_seq_free_resources(&pb->resources);
 	return ret;
 }
 
@@ -269,9 +374,9 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
 
 	backlight_device_unregister(bl);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
-	pwm_put(pb->pwm);
+	pwm_backlight_off(bl);
+	power_seq_free_resources(&pb->resources);
+
 	if (pb->exit)
 		pb->exit(&pdev->dev);
 	return 0;
@@ -285,8 +390,7 @@ static int pwm_backlight_suspend(struct device *dev)
 
 	if (pb->notify)
 		pb->notify(pb->dev, 0);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
+	pwm_backlight_off(bl);
 	if (pb->notify_after)
 		pb->notify_after(pb->dev, 0);
 	return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..dda267e 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,47 @@
 #define __LINUX_PWM_BACKLIGHT_H
 
 #include <linux/backlight.h>
+#include <linux/power_seq.h>
 
+/**
+ * Two ways of passing data to the driver can be used:
+ * 1) If not using device tree and use_power_sequences is not set, the legacy
+ *    interface is used. power_on_sequence and power_off_sequences are ignored,
+ *    and pwm_id and pwm_period_ns can be used to assign a PWM and period to
+ *    the backlight. The callback functions will also be called by the driver
+ *    at appropriate times.
+ * 2) If use_power_sequences is set, the power sequences should either be NULL
+ *    of contain an array of platform_pwm_backlight_seq_step instances
+ *    terminated by a full-zero'd one. The described sequences will then be used
+ *    for powering the backlight on and off, and the callbacks will not be
+ *    called. Instances of resources will be obtained using the get_* functions,
+ *    giving id as a consumer name.
+ *
+ * If the device tree is used, the power sequences properties are parsed and
+ * converted to the corresponding power sequences of this structure, which is
+ * passed to the driver with use_power_sequences set to true. See the
+ * pwm-backlight bindings documentation file for more details.
+ */
 struct platform_pwm_backlight_data {
-	int pwm_id;
 	unsigned int max_brightness;
 	unsigned int dft_brightness;
 	unsigned int lth_brightness;
-	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	/* Set this to true otherwise the legacy interface will be used */
+	bool use_power_sequences;
+	/*
+	 * New interface - arrays of steps terminated by a STOP instance, or
+	 * NULL if unused.
+	 */
+	struct platform_power_seq_step *power_on_seq;
+	struct platform_power_seq_step *power_off_seq;
+	/*
+	 * Legacy interface - single PWM and callback methods to control
+	 * the power sequence. pwm_id and pwm_period_ns need only be specified
+	 * if get_pwm(dev, NULL) will return NULL.
+	 */
+	int pwm_id;
+	unsigned int pwm_period_ns;
 	int (*init)(struct device *dev);
 	int (*notify)(struct device *dev, int brightness);
 	void (*notify_after)(struct device *dev, int brightness);
-- 
1.7.11.1

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
@ 2012-07-09  6:08   ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

Make use of the power sequences specified in the device tree or platform
data, if any.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../bindings/video/backlight/pwm-backlight.txt     |  28 ++-
 drivers/video/backlight/power_seq.c                |  44 ++---
 drivers/video/backlight/pwm_bl.c                   | 210 +++++++++++++++------
 include/linux/pwm_backlight.h                      |  37 +++-
 4 files changed, 239 insertions(+), 80 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..86c9253 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -2,7 +2,10 @@ pwm-backlight bindings
 
 Required properties:
   - compatible: "pwm-backlight"
-  - pwms: OF device-tree PWM specification (see PWM binding[0])
+  - pwms: OF device-tree PWM specification (see PWM binding[0]). Exactly one PWM
+      must be specified
+  - pwm-names: a list of names for the PWM devices specified in the
+      "pwms" property (see PWM binding[0])
   - brightness-levels: Array of distinct brightness levels. Typically these
       are in the range from 0 to 255, but any range starting at 0 will do.
       The actual brightness level (PWM duty cycle) will be interpolated
@@ -10,10 +13,18 @@ Required properties:
       last value in the array represents a 100% duty cycle (brightest).
   - default-brightness-level: the default brightness level (index into the
       array defined by the "brightness-levels" property)
+  - power-on-sequence: Power sequence that will bring the backlight on. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
+  - power-off-sequence: Power sequence that will bring the backlight off. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
 
 Optional properties:
-  - pwm-names: a list of names for the PWM devices specified in the
-               "pwms" property (see PWM binding[0])
+  - *-supply: a reference to a regulator used within a power sequence
+  - *-gpios: a reference to a GPIO used within a power sequence.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -22,7 +33,18 @@ Example:
 	backlight {
 		compatible = "pwm-backlight";
 		pwms = <&pwm 0 5000000>;
+		pwm-names = "backlight";
 
 		brightness-levels = <0 4 8 16 32 64 128 255>;
 		default-brightness-level = <6>;
+		power-supply = <&backlight_reg>;
+		enable-gpios = <&gpio 6 0>;
+		power-on-sequence = "REGULATOR", "power", <1>,
+				    "DELAY", <10>,
+				    "PWM", "backlight", <1>,
+				    "GPIO", "enable", <1>;
+		power-off-sequence = "GPIO", "enable", <0>,
+				     "PWM", "backlight", <0>,
+				     "DELAY", <10>,
+				     "REGULATOR", "power", <0>;
 	};
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
index f54cb7d..f8737db 100644
--- a/drivers/video/backlight/power_seq.c
+++ b/drivers/video/backlight/power_seq.c
@@ -118,9 +118,9 @@ static int of_parse_power_seq_step(struct device *dev, struct property *prop,
 			tmp_buf[sizeof(tmp_buf) - 6] = 0;
 			strcat(tmp_buf, "-gpios");
 			ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
-			if (ret >= 0)
+			if (ret >= 0) {
 				seq[cpt].value = ret;
-			else {
+			} else {
 				if (ret != -EPROBE_DEFER)
 					dev_err(dev, "cannot get gpio \"%s\"\n",
 						seq[cpt].id);
@@ -218,26 +218,26 @@ power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
 		seq->type = pseq->type;
 
 		switch (pseq->type) {
-			case POWER_SEQ_REGULATOR:
-			case POWER_SEQ_GPIO:
-			case POWER_SEQ_PWM:
-				if (!(res = power_seq_find_resource(ress, pseq))) {
-					/* create resource node */
-					res = devm_kzalloc(dev, sizeof(*res),
-							   GFP_KERNEL);
-					if (!res)
-						return ERR_PTR(-ENOMEM);
-					memcpy(&res->plat, pseq, sizeof(*pseq));
-
-					list_add(&res->list, ress);
-				}
-				seq->resource = res;
-			case POWER_SEQ_DELAY:
-				seq->parameter = pseq->parameter;
-				break;
-			default:
-				dev_err(dev, "invalid sequence step type!\n");
-				return ERR_PTR(-EINVAL);
+		case POWER_SEQ_REGULATOR:
+		case POWER_SEQ_GPIO:
+		case POWER_SEQ_PWM:
+			if (!(res = power_seq_find_resource(ress, pseq))) {
+				/* create resource node */
+				res = devm_kzalloc(dev, sizeof(*res),
+						   GFP_KERNEL);
+				if (!res)
+					return ERR_PTR(-ENOMEM);
+				memcpy(&res->plat, pseq, sizeof(*pseq));
+
+				list_add(&res->list, ress);
+			}
+			seq->resource = res;
+		case POWER_SEQ_DELAY:
+			seq->parameter = pseq->parameter;
+			break;
+		default:
+			dev_err(dev, "invalid sequence step type!\n");
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1a38953..2936819 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,12 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	bool			enabled;
+	power_seq_resources	resources;
+	power_seq		*power_on_seq;
+	power_seq		*power_off_seq;
+
+	/* Legacy callbacks */
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -35,6 +41,34 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static void pwm_backlight_on(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (pb->enabled)
+		return;
+
+	if ((ret = power_seq_run(pb->power_on_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power on sequence\n");
+
+	pb->enabled = true;
+}
+
+static void pwm_backlight_off(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (!pb->enabled)
+		return;
+
+	if ((ret = power_seq_run(pb->power_off_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power off sequence\n");
+
+	pb->enabled = false;
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +85,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		brightness = pb->notify(pb->dev, brightness);
 
 	if (brightness == 0) {
-		pwm_config(pb->pwm, 0, pb->period);
-		pwm_disable(pb->pwm);
+		pwm_backlight_off(bl);
 	} else {
 		int duty_cycle;
 		if (pb->levels) {
@@ -144,12 +177,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		data->max_brightness--;
 	}
 
-	/*
-	 * TODO: Most users of this driver use a number of GPIOs to control
-	 *       backlight power. Support for specifying these needs to be
-	 *       added.
-	 */
+	data->power_on_seq = of_parse_power_seq(dev, node, "power-on-sequence");
+	if (IS_ERR(data->power_on_seq))
+		return PTR_ERR(data->power_on_seq);
+	data->power_off_seq = of_parse_power_seq(dev, node,
+						 "power-off-sequence");
+	if (IS_ERR(data->power_off_seq))
+		return PTR_ERR(data->power_off_seq);
 
+	data->use_power_sequences = true;
 	return 0;
 }
 
@@ -167,37 +203,134 @@ static int pwm_backlight_parse_dt(struct device *dev,
 }
 #endif
 
+/**
+ * Construct the power sequences corresponding to the legacy platform data.
+ */
+static int pwm_backlight_legacy_probe(struct platform_device *pdev,
+				      struct pwm_bl_data *pb)
+{
+	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct power_seq_resource *res;
+	struct power_seq_step *step;
+
+	pb->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(pb->pwm)) {
+		dev_warn(dev, "unable to request PWM, trying legacy API\n");
+
+		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
+		if (IS_ERR(pb->pwm)) {
+			dev_err(dev, "unable to request legacy PWM\n");
+			return PTR_ERR(pb->pwm);
+		}
+		pwm_set_period(pb->pwm, data->pwm_period_ns);
+	}
+
+	pb->notify = data->notify;
+	pb->notify_after = data->notify_after;
+	pb->check_fb = data->check_fb;
+	pb->exit = data->exit;
+	pb->dev = dev;
+
+	/* Now build the resources and sequences corresponding to this PWM */
+	res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+	if (!res) return -ENOMEM;
+	res->plat.type = POWER_SEQ_PWM;
+	res->plat.id = "pwm-backlight";
+	res->pwm = pb->pwm;
+	list_add(&res->list, &pb->resources);
+
+	/* allocate both power on and off sequences at the same time */
+	step = devm_kzalloc(dev, sizeof(*step) * 4, GFP_KERNEL);
+	if (!step) return -ENOMEM;
+	step->type = POWER_SEQ_PWM;
+	step->resource = res;
+	memcpy(&step[2], &step[0], sizeof(*step));
+	step[0].parameter = 1;
+	pb->power_on_seq = &step[0];
+	pb->power_off_seq = &step[2];
+
+	return 0;
+}
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
 	struct platform_pwm_backlight_data defdata;
+	struct power_seq_resource *res;
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
 	unsigned int max;
 	int ret;
 
+	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
+	if (!pb) {
+		dev_err(&pdev->dev, "no memory for state\n");
+		return -ENOMEM;
+	}
+
+	INIT_LIST_HEAD(&pb->resources);
+
+	/* using new interface or device tree */
 	if (!data) {
+		/* build platform data from device tree */
 		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
-		if (ret < 0) {
+		if (ret == -EPROBE_DEFER) {
+			return ret;
+		} else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to find platform data\n");
 			return ret;
 		}
-
 		data = &defdata;
 	}
 
-	if (data->init) {
-		ret = data->init(&pdev->dev);
+	if (!data->use_power_sequences) {
+		/* using legacy interface */
+		ret = pwm_backlight_legacy_probe(pdev, pb);
+		if (ret < 0)
+			return ret;
+	} else {
+		/* build sequences and allocate resources from platform data */
+		if (data->power_on_seq) {
+			pb->power_on_seq = power_seq_build(&pdev->dev, &pb->resources,
+							   data->power_on_seq);
+			if (IS_ERR(pb->power_on_seq))
+				return PTR_ERR(pb->power_on_seq);
+		}
+		if (data->power_off_seq) {
+			pb->power_off_seq = power_seq_build(&pdev->dev, &pb->resources,
+							   data->power_off_seq);
+			if (IS_ERR(pb->power_off_seq))
+				return PTR_ERR(pb->power_off_seq);
+		}
+		ret = power_seq_allocate_resources(&pdev->dev, &pb->resources);
 		if (ret < 0)
 			return ret;
+
+		/* we must have exactly one PWM for this driver */
+		list_for_each_entry(res, &pb->resources, list) {
+			if (res->plat.type != POWER_SEQ_PWM)
+				continue;
+			if (pb->pwm) {
+				dev_err(&pdev->dev, "cannot use more than one PWM\n");
+				return -EINVAL;
+			}
+			/* keep the pwm at hand */
+			pb->pwm = res->pwm;
+		}
 	}
 
-	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
-	if (!pb) {
-		dev_err(&pdev->dev, "no memory for state\n");
-		ret = -ENOMEM;
-		goto err_alloc;
+	/* from here we should have a PWM */
+	if (!pb->pwm) {
+		dev_err(&pdev->dev, "no PWM defined!\n");
+		return -EINVAL;
+	}
+
+	if (data->init) {
+		ret = data->init(&pdev->dev);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (data->levels) {
@@ -207,34 +340,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		max = data->max_brightness;
 	}
 
-	pb->notify = data->notify;
-	pb->notify_after = data->notify_after;
-	pb->check_fb = data->check_fb;
-	pb->exit = data->exit;
-	pb->dev = &pdev->dev;
-
-	pb->pwm = pwm_get(&pdev->dev, NULL);
-	if (IS_ERR(pb->pwm)) {
-		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
-
-		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
-		if (IS_ERR(pb->pwm)) {
-			dev_err(&pdev->dev, "unable to request legacy PWM\n");
-			ret = PTR_ERR(pb->pwm);
-			goto err_alloc;
-		}
-	}
-
-	dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
-	/*
-	 * The DT case will set the pwm_period_ns field to 0 and store the
-	 * period, parsed from the DT, in the PWM device. For the non-DT case,
-	 * set the period from platform data.
-	 */
-	if (data->pwm_period_ns > 0)
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-
 	pb->period = pwm_get_period(pb->pwm);
 	pb->lth_brightness = data->lth_brightness * (pb->period / max);
 
@@ -246,20 +351,20 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (IS_ERR(bl)) {
 		dev_err(&pdev->dev, "failed to register backlight\n");
 		ret = PTR_ERR(bl);
-		goto err_bl;
+		goto err;
 	}
 
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);
+
 	return 0;
 
-err_bl:
-	pwm_put(pb->pwm);
-err_alloc:
+err:
 	if (data->exit)
 		data->exit(&pdev->dev);
+	power_seq_free_resources(&pb->resources);
 	return ret;
 }
 
@@ -269,9 +374,9 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
 
 	backlight_device_unregister(bl);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
-	pwm_put(pb->pwm);
+	pwm_backlight_off(bl);
+	power_seq_free_resources(&pb->resources);
+
 	if (pb->exit)
 		pb->exit(&pdev->dev);
 	return 0;
@@ -285,8 +390,7 @@ static int pwm_backlight_suspend(struct device *dev)
 
 	if (pb->notify)
 		pb->notify(pb->dev, 0);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
+	pwm_backlight_off(bl);
 	if (pb->notify_after)
 		pb->notify_after(pb->dev, 0);
 	return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..dda267e 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,47 @@
 #define __LINUX_PWM_BACKLIGHT_H
 
 #include <linux/backlight.h>
+#include <linux/power_seq.h>
 
+/**
+ * Two ways of passing data to the driver can be used:
+ * 1) If not using device tree and use_power_sequences is not set, the legacy
+ *    interface is used. power_on_sequence and power_off_sequences are ignored,
+ *    and pwm_id and pwm_period_ns can be used to assign a PWM and period to
+ *    the backlight. The callback functions will also be called by the driver
+ *    at appropriate times.
+ * 2) If use_power_sequences is set, the power sequences should either be NULL
+ *    of contain an array of platform_pwm_backlight_seq_step instances
+ *    terminated by a full-zero'd one. The described sequences will then be used
+ *    for powering the backlight on and off, and the callbacks will not be
+ *    called. Instances of resources will be obtained using the get_* functions,
+ *    giving id as a consumer name.
+ *
+ * If the device tree is used, the power sequences properties are parsed and
+ * converted to the corresponding power sequences of this structure, which is
+ * passed to the driver with use_power_sequences set to true. See the
+ * pwm-backlight bindings documentation file for more details.
+ */
 struct platform_pwm_backlight_data {
-	int pwm_id;
 	unsigned int max_brightness;
 	unsigned int dft_brightness;
 	unsigned int lth_brightness;
-	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	/* Set this to true otherwise the legacy interface will be used */
+	bool use_power_sequences;
+	/*
+	 * New interface - arrays of steps terminated by a STOP instance, or
+	 * NULL if unused.
+	 */
+	struct platform_power_seq_step *power_on_seq;
+	struct platform_power_seq_step *power_off_seq;
+	/*
+	 * Legacy interface - single PWM and callback methods to control
+	 * the power sequence. pwm_id and pwm_period_ns need only be specified
+	 * if get_pwm(dev, NULL) will return NULL.
+	 */
+	int pwm_id;
+	unsigned int pwm_period_ns;
 	int (*init)(struct device *dev);
 	int (*notify)(struct device *dev, int brightness);
 	void (*notify_after)(struct device *dev, int brightness);
-- 
1.7.11.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
@ 2012-07-09  6:08   ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

Make use of the power sequences specified in the device tree or platform
data, if any.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../bindings/video/backlight/pwm-backlight.txt     |  28 ++-
 drivers/video/backlight/power_seq.c                |  44 ++---
 drivers/video/backlight/pwm_bl.c                   | 210 +++++++++++++++------
 include/linux/pwm_backlight.h                      |  37 +++-
 4 files changed, 239 insertions(+), 80 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..86c9253 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -2,7 +2,10 @@ pwm-backlight bindings
 
 Required properties:
   - compatible: "pwm-backlight"
-  - pwms: OF device-tree PWM specification (see PWM binding[0])
+  - pwms: OF device-tree PWM specification (see PWM binding[0]). Exactly one PWM
+      must be specified
+  - pwm-names: a list of names for the PWM devices specified in the
+      "pwms" property (see PWM binding[0])
   - brightness-levels: Array of distinct brightness levels. Typically these
       are in the range from 0 to 255, but any range starting at 0 will do.
       The actual brightness level (PWM duty cycle) will be interpolated
@@ -10,10 +13,18 @@ Required properties:
       last value in the array represents a 100% duty cycle (brightest).
   - default-brightness-level: the default brightness level (index into the
       array defined by the "brightness-levels" property)
+  - power-on-sequence: Power sequence that will bring the backlight on. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
+  - power-off-sequence: Power sequence that will bring the backlight off. This
+      sequence must reference the PWM specified in the pwms property by its
+      name. It can also reference extra GPIOs or regulators, and introduce
+      delays between sequence steps
 
 Optional properties:
-  - pwm-names: a list of names for the PWM devices specified in the
-               "pwms" property (see PWM binding[0])
+  - *-supply: a reference to a regulator used within a power sequence
+  - *-gpios: a reference to a GPIO used within a power sequence.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -22,7 +33,18 @@ Example:
 	backlight {
 		compatible = "pwm-backlight";
 		pwms = <&pwm 0 5000000>;
+		pwm-names = "backlight";
 
 		brightness-levels = <0 4 8 16 32 64 128 255>;
 		default-brightness-level = <6>;
+		power-supply = <&backlight_reg>;
+		enable-gpios = <&gpio 6 0>;
+		power-on-sequence = "REGULATOR", "power", <1>,
+				    "DELAY", <10>,
+				    "PWM", "backlight", <1>,
+				    "GPIO", "enable", <1>;
+		power-off-sequence = "GPIO", "enable", <0>,
+				     "PWM", "backlight", <0>,
+				     "DELAY", <10>,
+				     "REGULATOR", "power", <0>;
 	};
diff --git a/drivers/video/backlight/power_seq.c b/drivers/video/backlight/power_seq.c
index f54cb7d..f8737db 100644
--- a/drivers/video/backlight/power_seq.c
+++ b/drivers/video/backlight/power_seq.c
@@ -118,9 +118,9 @@ static int of_parse_power_seq_step(struct device *dev, struct property *prop,
 			tmp_buf[sizeof(tmp_buf) - 6] = 0;
 			strcat(tmp_buf, "-gpios");
 			ret = of_get_named_gpio(dev->of_node, tmp_buf, 0);
-			if (ret >= 0)
+			if (ret >= 0) {
 				seq[cpt].value = ret;
-			else {
+			} else {
 				if (ret != -EPROBE_DEFER)
 					dev_err(dev, "cannot get gpio \"%s\"\n",
 						seq[cpt].id);
@@ -218,26 +218,26 @@ power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
 		seq->type = pseq->type;
 
 		switch (pseq->type) {
-			case POWER_SEQ_REGULATOR:
-			case POWER_SEQ_GPIO:
-			case POWER_SEQ_PWM:
-				if (!(res = power_seq_find_resource(ress, pseq))) {
-					/* create resource node */
-					res = devm_kzalloc(dev, sizeof(*res),
-							   GFP_KERNEL);
-					if (!res)
-						return ERR_PTR(-ENOMEM);
-					memcpy(&res->plat, pseq, sizeof(*pseq));
-
-					list_add(&res->list, ress);
-				}
-				seq->resource = res;
-			case POWER_SEQ_DELAY:
-				seq->parameter = pseq->parameter;
-				break;
-			default:
-				dev_err(dev, "invalid sequence step type!\n");
-				return ERR_PTR(-EINVAL);
+		case POWER_SEQ_REGULATOR:
+		case POWER_SEQ_GPIO:
+		case POWER_SEQ_PWM:
+			if (!(res = power_seq_find_resource(ress, pseq))) {
+				/* create resource node */
+				res = devm_kzalloc(dev, sizeof(*res),
+						   GFP_KERNEL);
+				if (!res)
+					return ERR_PTR(-ENOMEM);
+				memcpy(&res->plat, pseq, sizeof(*pseq));
+
+				list_add(&res->list, ress);
+			}
+			seq->resource = res;
+		case POWER_SEQ_DELAY:
+			seq->parameter = pseq->parameter;
+			break;
+		default:
+			dev_err(dev, "invalid sequence step type!\n");
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1a38953..2936819 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,12 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	bool			enabled;
+	power_seq_resources	resources;
+	power_seq		*power_on_seq;
+	power_seq		*power_off_seq;
+
+	/* Legacy callbacks */
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -35,6 +41,34 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static void pwm_backlight_on(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (pb->enabled)
+		return;
+
+	if ((ret = power_seq_run(pb->power_on_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power on sequence\n");
+
+	pb->enabled = true;
+}
+
+static void pwm_backlight_off(struct backlight_device *bl)
+{
+	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
+	int ret;
+
+	if (!pb->enabled)
+		return;
+
+	if ((ret = power_seq_run(pb->power_off_seq)) < 0)
+		dev_err(&bl->dev, "cannot run power off sequence\n");
+
+	pb->enabled = false;
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +85,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		brightness = pb->notify(pb->dev, brightness);
 
 	if (brightness = 0) {
-		pwm_config(pb->pwm, 0, pb->period);
-		pwm_disable(pb->pwm);
+		pwm_backlight_off(bl);
 	} else {
 		int duty_cycle;
 		if (pb->levels) {
@@ -144,12 +177,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		data->max_brightness--;
 	}
 
-	/*
-	 * TODO: Most users of this driver use a number of GPIOs to control
-	 *       backlight power. Support for specifying these needs to be
-	 *       added.
-	 */
+	data->power_on_seq = of_parse_power_seq(dev, node, "power-on-sequence");
+	if (IS_ERR(data->power_on_seq))
+		return PTR_ERR(data->power_on_seq);
+	data->power_off_seq = of_parse_power_seq(dev, node,
+						 "power-off-sequence");
+	if (IS_ERR(data->power_off_seq))
+		return PTR_ERR(data->power_off_seq);
 
+	data->use_power_sequences = true;
 	return 0;
 }
 
@@ -167,37 +203,134 @@ static int pwm_backlight_parse_dt(struct device *dev,
 }
 #endif
 
+/**
+ * Construct the power sequences corresponding to the legacy platform data.
+ */
+static int pwm_backlight_legacy_probe(struct platform_device *pdev,
+				      struct pwm_bl_data *pb)
+{
+	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct power_seq_resource *res;
+	struct power_seq_step *step;
+
+	pb->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(pb->pwm)) {
+		dev_warn(dev, "unable to request PWM, trying legacy API\n");
+
+		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
+		if (IS_ERR(pb->pwm)) {
+			dev_err(dev, "unable to request legacy PWM\n");
+			return PTR_ERR(pb->pwm);
+		}
+		pwm_set_period(pb->pwm, data->pwm_period_ns);
+	}
+
+	pb->notify = data->notify;
+	pb->notify_after = data->notify_after;
+	pb->check_fb = data->check_fb;
+	pb->exit = data->exit;
+	pb->dev = dev;
+
+	/* Now build the resources and sequences corresponding to this PWM */
+	res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+	if (!res) return -ENOMEM;
+	res->plat.type = POWER_SEQ_PWM;
+	res->plat.id = "pwm-backlight";
+	res->pwm = pb->pwm;
+	list_add(&res->list, &pb->resources);
+
+	/* allocate both power on and off sequences at the same time */
+	step = devm_kzalloc(dev, sizeof(*step) * 4, GFP_KERNEL);
+	if (!step) return -ENOMEM;
+	step->type = POWER_SEQ_PWM;
+	step->resource = res;
+	memcpy(&step[2], &step[0], sizeof(*step));
+	step[0].parameter = 1;
+	pb->power_on_seq = &step[0];
+	pb->power_off_seq = &step[2];
+
+	return 0;
+}
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = pdev->dev.platform_data;
 	struct platform_pwm_backlight_data defdata;
+	struct power_seq_resource *res;
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct pwm_bl_data *pb;
 	unsigned int max;
 	int ret;
 
+	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
+	if (!pb) {
+		dev_err(&pdev->dev, "no memory for state\n");
+		return -ENOMEM;
+	}
+
+	INIT_LIST_HEAD(&pb->resources);
+
+	/* using new interface or device tree */
 	if (!data) {
+		/* build platform data from device tree */
 		ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
-		if (ret < 0) {
+		if (ret = -EPROBE_DEFER) {
+			return ret;
+		} else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to find platform data\n");
 			return ret;
 		}
-
 		data = &defdata;
 	}
 
-	if (data->init) {
-		ret = data->init(&pdev->dev);
+	if (!data->use_power_sequences) {
+		/* using legacy interface */
+		ret = pwm_backlight_legacy_probe(pdev, pb);
+		if (ret < 0)
+			return ret;
+	} else {
+		/* build sequences and allocate resources from platform data */
+		if (data->power_on_seq) {
+			pb->power_on_seq = power_seq_build(&pdev->dev, &pb->resources,
+							   data->power_on_seq);
+			if (IS_ERR(pb->power_on_seq))
+				return PTR_ERR(pb->power_on_seq);
+		}
+		if (data->power_off_seq) {
+			pb->power_off_seq = power_seq_build(&pdev->dev, &pb->resources,
+							   data->power_off_seq);
+			if (IS_ERR(pb->power_off_seq))
+				return PTR_ERR(pb->power_off_seq);
+		}
+		ret = power_seq_allocate_resources(&pdev->dev, &pb->resources);
 		if (ret < 0)
 			return ret;
+
+		/* we must have exactly one PWM for this driver */
+		list_for_each_entry(res, &pb->resources, list) {
+			if (res->plat.type != POWER_SEQ_PWM)
+				continue;
+			if (pb->pwm) {
+				dev_err(&pdev->dev, "cannot use more than one PWM\n");
+				return -EINVAL;
+			}
+			/* keep the pwm at hand */
+			pb->pwm = res->pwm;
+		}
 	}
 
-	pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL);
-	if (!pb) {
-		dev_err(&pdev->dev, "no memory for state\n");
-		ret = -ENOMEM;
-		goto err_alloc;
+	/* from here we should have a PWM */
+	if (!pb->pwm) {
+		dev_err(&pdev->dev, "no PWM defined!\n");
+		return -EINVAL;
+	}
+
+	if (data->init) {
+		ret = data->init(&pdev->dev);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (data->levels) {
@@ -207,34 +340,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		max = data->max_brightness;
 	}
 
-	pb->notify = data->notify;
-	pb->notify_after = data->notify_after;
-	pb->check_fb = data->check_fb;
-	pb->exit = data->exit;
-	pb->dev = &pdev->dev;
-
-	pb->pwm = pwm_get(&pdev->dev, NULL);
-	if (IS_ERR(pb->pwm)) {
-		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
-
-		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
-		if (IS_ERR(pb->pwm)) {
-			dev_err(&pdev->dev, "unable to request legacy PWM\n");
-			ret = PTR_ERR(pb->pwm);
-			goto err_alloc;
-		}
-	}
-
-	dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
-	/*
-	 * The DT case will set the pwm_period_ns field to 0 and store the
-	 * period, parsed from the DT, in the PWM device. For the non-DT case,
-	 * set the period from platform data.
-	 */
-	if (data->pwm_period_ns > 0)
-		pwm_set_period(pb->pwm, data->pwm_period_ns);
-
 	pb->period = pwm_get_period(pb->pwm);
 	pb->lth_brightness = data->lth_brightness * (pb->period / max);
 
@@ -246,20 +351,20 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (IS_ERR(bl)) {
 		dev_err(&pdev->dev, "failed to register backlight\n");
 		ret = PTR_ERR(bl);
-		goto err_bl;
+		goto err;
 	}
 
 	bl->props.brightness = data->dft_brightness;
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);
+
 	return 0;
 
-err_bl:
-	pwm_put(pb->pwm);
-err_alloc:
+err:
 	if (data->exit)
 		data->exit(&pdev->dev);
+	power_seq_free_resources(&pb->resources);
 	return ret;
 }
 
@@ -269,9 +374,9 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 	struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
 
 	backlight_device_unregister(bl);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
-	pwm_put(pb->pwm);
+	pwm_backlight_off(bl);
+	power_seq_free_resources(&pb->resources);
+
 	if (pb->exit)
 		pb->exit(&pdev->dev);
 	return 0;
@@ -285,8 +390,7 @@ static int pwm_backlight_suspend(struct device *dev)
 
 	if (pb->notify)
 		pb->notify(pb->dev, 0);
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
+	pwm_backlight_off(bl);
 	if (pb->notify_after)
 		pb->notify_after(pb->dev, 0);
 	return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..dda267e 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,47 @@
 #define __LINUX_PWM_BACKLIGHT_H
 
 #include <linux/backlight.h>
+#include <linux/power_seq.h>
 
+/**
+ * Two ways of passing data to the driver can be used:
+ * 1) If not using device tree and use_power_sequences is not set, the legacy
+ *    interface is used. power_on_sequence and power_off_sequences are ignored,
+ *    and pwm_id and pwm_period_ns can be used to assign a PWM and period to
+ *    the backlight. The callback functions will also be called by the driver
+ *    at appropriate times.
+ * 2) If use_power_sequences is set, the power sequences should either be NULL
+ *    of contain an array of platform_pwm_backlight_seq_step instances
+ *    terminated by a full-zero'd one. The described sequences will then be used
+ *    for powering the backlight on and off, and the callbacks will not be
+ *    called. Instances of resources will be obtained using the get_* functions,
+ *    giving id as a consumer name.
+ *
+ * If the device tree is used, the power sequences properties are parsed and
+ * converted to the corresponding power sequences of this structure, which is
+ * passed to the driver with use_power_sequences set to true. See the
+ * pwm-backlight bindings documentation file for more details.
+ */
 struct platform_pwm_backlight_data {
-	int pwm_id;
 	unsigned int max_brightness;
 	unsigned int dft_brightness;
 	unsigned int lth_brightness;
-	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	/* Set this to true otherwise the legacy interface will be used */
+	bool use_power_sequences;
+	/*
+	 * New interface - arrays of steps terminated by a STOP instance, or
+	 * NULL if unused.
+	 */
+	struct platform_power_seq_step *power_on_seq;
+	struct platform_power_seq_step *power_off_seq;
+	/*
+	 * Legacy interface - single PWM and callback methods to control
+	 * the power sequence. pwm_id and pwm_period_ns need only be specified
+	 * if get_pwm(dev, NULL) will return NULL.
+	 */
+	int pwm_id;
+	unsigned int pwm_period_ns;
 	int (*init)(struct device *dev);
 	int (*notify)(struct device *dev, int brightness);
 	void (*notify_after)(struct device *dev, int brightness);
-- 
1.7.11.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
  2012-07-09  6:08 ` Alexandre Courbot
  (?)
@ 2012-07-09  6:08   ` Alexandre Courbot
  -1 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
 arch/arm/boot/dts/tegra20.dtsi        |  2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..c67d9e1 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -317,6 +317,37 @@
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&backlight_reg>;
+		enable-gpios = <&gpio 28 0>;
+
+		power-on-sequence = "REGULATOR", "power", <1>,
+				    "DELAY", <10>,
+				    "PWM", "backlight", <1>,
+				    "GPIO", "enable", <1>;
+		power-off-sequence = "GPIO", "enable", <0>,
+				     "PWM", "backlight", <0>,
+				     "DELAY", <10>,
+				     "REGULATOR", "power", <0>;
+	};
+
+	backlight_reg: fixedregulator@176 {
+		compatible = "regulator-fixed";
+		regulator-name = "backlight_regulator";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		gpio = <&gpio 176 0>;
+		startup-delay-us = <0>;
+		enable-active-high;
+		regulator-boot-off;
+	};
+
 	sound {
 		compatible = "nvidia,tegra-audio-wm8903-ventana",
 			     "nvidia,tegra-audio-wm8903";
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 405d167..67a6cd9 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -123,7 +123,7 @@
 		status = "disabled";
 	};
 
-	pwm {
+	pwm: pwm {
 		compatible = "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
 		#pwm-cells = <2>;
-- 
1.7.11.1

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-09  6:08   ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
 arch/arm/boot/dts/tegra20.dtsi        |  2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..c67d9e1 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -317,6 +317,37 @@
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&backlight_reg>;
+		enable-gpios = <&gpio 28 0>;
+
+		power-on-sequence = "REGULATOR", "power", <1>,
+				    "DELAY", <10>,
+				    "PWM", "backlight", <1>,
+				    "GPIO", "enable", <1>;
+		power-off-sequence = "GPIO", "enable", <0>,
+				     "PWM", "backlight", <0>,
+				     "DELAY", <10>,
+				     "REGULATOR", "power", <0>;
+	};
+
+	backlight_reg: fixedregulator@176 {
+		compatible = "regulator-fixed";
+		regulator-name = "backlight_regulator";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		gpio = <&gpio 176 0>;
+		startup-delay-us = <0>;
+		enable-active-high;
+		regulator-boot-off;
+	};
+
 	sound {
 		compatible = "nvidia,tegra-audio-wm8903-ventana",
 			     "nvidia,tegra-audio-wm8903";
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 405d167..67a6cd9 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -123,7 +123,7 @@
 		status = "disabled";
 	};
 
-	pwm {
+	pwm: pwm {
 		compatible = "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
 		#pwm-cells = <2>;
-- 
1.7.11.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-09  6:08   ` Alexandre Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alexandre Courbot @ 2012-07-09  6:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-tegra, linux-kernel, linux-fbdev, devicetree-discuss,
	Alexandre Courbot

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
 arch/arm/boot/dts/tegra20.dtsi        |  2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..c67d9e1 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -317,6 +317,37 @@
 		bus-width = <8>;
 	};
 
+	backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <12>;
+
+		pwms = <&pwm 2 5000000>;
+		pwm-names = "backlight";
+		power-supply = <&backlight_reg>;
+		enable-gpios = <&gpio 28 0>;
+
+		power-on-sequence = "REGULATOR", "power", <1>,
+				    "DELAY", <10>,
+				    "PWM", "backlight", <1>,
+				    "GPIO", "enable", <1>;
+		power-off-sequence = "GPIO", "enable", <0>,
+				     "PWM", "backlight", <0>,
+				     "DELAY", <10>,
+				     "REGULATOR", "power", <0>;
+	};
+
+	backlight_reg: fixedregulator@176 {
+		compatible = "regulator-fixed";
+		regulator-name = "backlight_regulator";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		gpio = <&gpio 176 0>;
+		startup-delay-us = <0>;
+		enable-active-high;
+		regulator-boot-off;
+	};
+
 	sound {
 		compatible = "nvidia,tegra-audio-wm8903-ventana",
 			     "nvidia,tegra-audio-wm8903";
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 405d167..67a6cd9 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -123,7 +123,7 @@
 		status = "disabled";
 	};
 
-	pwm {
+	pwm: pwm {
 		compatible = "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
 		#pwm-cells = <2>;
-- 
1.7.11.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
  2012-07-09  6:08   ` Alexandre Courbot
  (?)
@ 2012-07-09  7:48       ` Alex Courbot
  -1 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-09  7:48 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Sorry, I just noticed a mistake in this patch I made while merging 
another one. The following also needs to be changed, otherwise the 
power-on sequence will never be executed:

diff --git a/drivers/video/backlight/pwm_bl.c 
b/drivers/video/backlight/pwm_bl.c
index 1a38953..4546d23 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -65,7 +98,7 @@ static int pwm_backlight_update_status(struct 
backlight_device *bl)
                 duty_cycle = pb->lth_brightness +
                      (duty_cycle * (pb->period - pb->lth_brightness) / 
max);
                 pwm_config(pb->pwm, duty_cycle, pb->period);
-               pwm_enable(pb->pwm);
+               pwm_backlight_on(bl);
         }


Apologies for the inconvenience.

Alex.

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
@ 2012-07-09  7:48       ` Alex Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-09  7:48 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

Sorry, I just noticed a mistake in this patch I made while merging 
another one. The following also needs to be changed, otherwise the 
power-on sequence will never be executed:

diff --git a/drivers/video/backlight/pwm_bl.c 
b/drivers/video/backlight/pwm_bl.c
index 1a38953..4546d23 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -65,7 +98,7 @@ static int pwm_backlight_update_status(struct 
backlight_device *bl)
                 duty_cycle = pb->lth_brightness +
                      (duty_cycle * (pb->period - pb->lth_brightness) / 
max);
                 pwm_config(pb->pwm, duty_cycle, pb->period);
-               pwm_enable(pb->pwm);
+               pwm_backlight_on(bl);
         }


Apologies for the inconvenience.

Alex.


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 2/3] pwm_backlight: use power sequences
@ 2012-07-09  7:48       ` Alex Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-09  7:48 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Sorry, I just noticed a mistake in this patch I made while merging 
another one. The following also needs to be changed, otherwise the 
power-on sequence will never be executed:

diff --git a/drivers/video/backlight/pwm_bl.c 
b/drivers/video/backlight/pwm_bl.c
index 1a38953..4546d23 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -65,7 +98,7 @@ static int pwm_backlight_update_status(struct 
backlight_device *bl)
                 duty_cycle = pb->lth_brightness +
                      (duty_cycle * (pb->period - pb->lth_brightness) / 
max);
                 pwm_config(pb->pwm, duty_cycle, pb->period);
-               pwm_enable(pb->pwm);
+               pwm_backlight_on(bl);
         }


Apologies for the inconvenience.

Alex.


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
  2012-07-09  6:08   ` Alexandre Courbot
  (?)
@ 2012-07-12  9:37       ` Simon Glass
  -1 siblings, 0 replies; 34+ messages in thread
From: Simon Glass @ 2012-07-12  9:37 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Hi Alex,

On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/tegra20.dtsi        |  2 +-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
> index be90544..c67d9e1 100644
> --- a/arch/arm/boot/dts/tegra20-ventana.dts
> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> @@ -317,6 +317,37 @@
>                 bus-width = <8>;
>         };
>

I would like to do something similar in U-Boot for Tegra, although
perhaps not right away. For now I will go with something considerably
simpler! But if this is merged into the kernel we will move to it in
U-Boot. Anyway here are my comments:

> +       backlight {
> +               compatible = "pwm-backlight";
> +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;

We seem to have a lot of these - should we move to a range and step size?

> +               default-brightness-level = <12>;
> +
> +               pwms = <&pwm 2 5000000>;
> +               pwm-names = "backlight";
> +               power-supply = <&backlight_reg>;
> +               enable-gpios = <&gpio 28 0>;
> +
> +               power-on-sequence = "REGULATOR", "power", <1>,
> +                                   "DELAY", <10>,
> +                                   "PWM", "backlight", <1>,
> +                                   "GPIO", "enable", <1>;

So the names REGULATOR, DELAY, etc. here are looked up through some
existing mechanism? In general I am not a big fan of mixing strings
and numbers in a property. Maybe something like:

power_on_sequence {
   step@0 {
      phandle = <&backlight_reg>;
      enable = <1>;
      post-delay = <10>;
   }
   step@1 {
      phandle = <&pwm 2 5000000>;
   }
   step@2 {
      phandle = <&gpio 28 0>;
      enable = <1>;
   }

> +               power-off-sequence = "GPIO", "enable", <0>,
> +                                    "PWM", "backlight", <0>,
> +                                    "DELAY", <10>,
> +                                    "REGULATOR", "power", <0>;
> +       };
> +
> +       backlight_reg: fixedregulator@176 {
> +               compatible = "regulator-fixed";
> +               regulator-name = "backlight_regulator";
> +               regulator-min-microvolt = <1800000>;
> +               regulator-max-microvolt = <1800000>;
> +               gpio = <&gpio 176 0>;
> +               startup-delay-us = <0>;
> +               enable-active-high;
> +               regulator-boot-off;
> +       };
> +
>         sound {
>                 compatible = "nvidia,tegra-audio-wm8903-ventana",
>                              "nvidia,tegra-audio-wm8903";
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 405d167..67a6cd9 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -123,7 +123,7 @@
>                 status = "disabled";
>         };
>
> -       pwm {
> +       pwm: pwm {
>                 compatible = "nvidia,tegra20-pwm";
>                 reg = <0x7000a000 0x100>;
>                 #pwm-cells = <2>;
> --
> 1.7.11.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,
Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12  9:37       ` Simon Glass
  0 siblings, 0 replies; 34+ messages in thread
From: Simon Glass @ 2012-07-12  9:37 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

Hi Alex,

On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/tegra20.dtsi        |  2 +-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
> index be90544..c67d9e1 100644
> --- a/arch/arm/boot/dts/tegra20-ventana.dts
> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> @@ -317,6 +317,37 @@
>                 bus-width = <8>;
>         };
>

I would like to do something similar in U-Boot for Tegra, although
perhaps not right away. For now I will go with something considerably
simpler! But if this is merged into the kernel we will move to it in
U-Boot. Anyway here are my comments:

> +       backlight {
> +               compatible = "pwm-backlight";
> +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;

We seem to have a lot of these - should we move to a range and step size?

> +               default-brightness-level = <12>;
> +
> +               pwms = <&pwm 2 5000000>;
> +               pwm-names = "backlight";
> +               power-supply = <&backlight_reg>;
> +               enable-gpios = <&gpio 28 0>;
> +
> +               power-on-sequence = "REGULATOR", "power", <1>,
> +                                   "DELAY", <10>,
> +                                   "PWM", "backlight", <1>,
> +                                   "GPIO", "enable", <1>;

So the names REGULATOR, DELAY, etc. here are looked up through some
existing mechanism? In general I am not a big fan of mixing strings
and numbers in a property. Maybe something like:

power_on_sequence {
   step@0 {
      phandle = <&backlight_reg>;
      enable = <1>;
      post-delay = <10>;
   }
   step@1 {
      phandle = <&pwm 2 5000000>;
   }
   step@2 {
      phandle = <&gpio 28 0>;
      enable = <1>;
   }

> +               power-off-sequence = "GPIO", "enable", <0>,
> +                                    "PWM", "backlight", <0>,
> +                                    "DELAY", <10>,
> +                                    "REGULATOR", "power", <0>;
> +       };
> +
> +       backlight_reg: fixedregulator@176 {
> +               compatible = "regulator-fixed";
> +               regulator-name = "backlight_regulator";
> +               regulator-min-microvolt = <1800000>;
> +               regulator-max-microvolt = <1800000>;
> +               gpio = <&gpio 176 0>;
> +               startup-delay-us = <0>;
> +               enable-active-high;
> +               regulator-boot-off;
> +       };
> +
>         sound {
>                 compatible = "nvidia,tegra-audio-wm8903-ventana",
>                              "nvidia,tegra-audio-wm8903";
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 405d167..67a6cd9 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -123,7 +123,7 @@
>                 status = "disabled";
>         };
>
> -       pwm {
> +       pwm: pwm {
>                 compatible = "nvidia,tegra20-pwm";
>                 reg = <0x7000a000 0x100>;
>                 #pwm-cells = <2>;
> --
> 1.7.11.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,
Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12  9:37       ` Simon Glass
  0 siblings, 0 replies; 34+ messages in thread
From: Simon Glass @ 2012-07-12  9:37 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Hi Alex,

On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/tegra20.dtsi        |  2 +-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
> index be90544..c67d9e1 100644
> --- a/arch/arm/boot/dts/tegra20-ventana.dts
> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> @@ -317,6 +317,37 @@
>                 bus-width = <8>;
>         };
>

I would like to do something similar in U-Boot for Tegra, although
perhaps not right away. For now I will go with something considerably
simpler! But if this is merged into the kernel we will move to it in
U-Boot. Anyway here are my comments:

> +       backlight {
> +               compatible = "pwm-backlight";
> +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;

We seem to have a lot of these - should we move to a range and step size?

> +               default-brightness-level = <12>;
> +
> +               pwms = <&pwm 2 5000000>;
> +               pwm-names = "backlight";
> +               power-supply = <&backlight_reg>;
> +               enable-gpios = <&gpio 28 0>;
> +
> +               power-on-sequence = "REGULATOR", "power", <1>,
> +                                   "DELAY", <10>,
> +                                   "PWM", "backlight", <1>,
> +                                   "GPIO", "enable", <1>;

So the names REGULATOR, DELAY, etc. here are looked up through some
existing mechanism? In general I am not a big fan of mixing strings
and numbers in a property. Maybe something like:

power_on_sequence {
   step@0 {
      phandle = <&backlight_reg>;
      enable = <1>;
      post-delay = <10>;
   }
   step@1 {
      phandle = <&pwm 2 5000000>;
   }
   step@2 {
      phandle = <&gpio 28 0>;
      enable = <1>;
   }

> +               power-off-sequence = "GPIO", "enable", <0>,
> +                                    "PWM", "backlight", <0>,
> +                                    "DELAY", <10>,
> +                                    "REGULATOR", "power", <0>;
> +       };
> +
> +       backlight_reg: fixedregulator@176 {
> +               compatible = "regulator-fixed";
> +               regulator-name = "backlight_regulator";
> +               regulator-min-microvolt = <1800000>;
> +               regulator-max-microvolt = <1800000>;
> +               gpio = <&gpio 176 0>;
> +               startup-delay-us = <0>;
> +               enable-active-high;
> +               regulator-boot-off;
> +       };
> +
>         sound {
>                 compatible = "nvidia,tegra-audio-wm8903-ventana",
>                              "nvidia,tegra-audio-wm8903";
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 405d167..67a6cd9 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -123,7 +123,7 @@
>                 status = "disabled";
>         };
>
> -       pwm {
> +       pwm: pwm {
>                 compatible = "nvidia,tegra20-pwm";
>                 reg = <0x7000a000 0x100>;
>                 #pwm-cells = <2>;
> --
> 1.7.11.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,
Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
  2012-07-12  9:37       ` Simon Glass
  (?)
@ 2012-07-12 10:04           ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2012-07-12 10:04 UTC (permalink / raw)
  To: Simon Glass
  Cc: Alexandre Courbot, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

[-- Attachment #1: Type: text/plain, Size: 3749 bytes --]

On Thu, Jul 12, 2012 at 11:37:33AM +0200, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
> >  arch/arm/boot/dts/tegra20.dtsi        |  2 +-
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
> > index be90544..c67d9e1 100644
> > --- a/arch/arm/boot/dts/tegra20-ventana.dts
> > +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> > @@ -317,6 +317,37 @@
> >                 bus-width = <8>;
> >         };
> >
> 
> I would like to do something similar in U-Boot for Tegra, although
> perhaps not right away. For now I will go with something considerably
> simpler! But if this is merged into the kernel we will move to it in
> U-Boot. Anyway here are my comments:
> 
> > +       backlight {
> > +               compatible = "pwm-backlight";
> > +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> 
> We seem to have a lot of these - should we move to a range and step size?

These actually seem to be a little bogus. The reason for introducing the
levels was to allow calibration of these values because they in fact
usually do not scale linearly. Instead, a linear brightness increase
usually maps to something like a logarithmic (at best) increase of the
duty cycle.

> > +               default-brightness-level = <12>;
> > +
> > +               pwms = <&pwm 2 5000000>;
> > +               pwm-names = "backlight";
> > +               power-supply = <&backlight_reg>;
> > +               enable-gpios = <&gpio 28 0>;
> > +
> > +               power-on-sequence = "REGULATOR", "power", <1>,
> > +                                   "DELAY", <10>,
> > +                                   "PWM", "backlight", <1>,
> > +                                   "GPIO", "enable", <1>;
> 
> So the names REGULATOR, DELAY, etc. here are looked up through some
> existing mechanism? In general I am not a big fan of mixing strings
> and numbers in a property. Maybe something like:
> 
> power_on_sequence {
>    step@0 {
>       phandle = <&backlight_reg>;
>       enable = <1>;
>       post-delay = <10>;
>    }
>    step@1 {
>       phandle = <&pwm 2 5000000>;
>    }
>    step@2 {
>       phandle = <&gpio 28 0>;
>       enable = <1>;
>    }

I actually like that a lot. What's missing from your example is a way to
specify the type of the phandle because it cannot be safely inferred.
Perhaps this could be done by just the property name:

	power_on_sequence {
		step@0 {
			power-supply = <&backlight_reg>;
			enable = <1>;
			post-delay = <10>;
		};
		step@1 {
			pwm = <&pwm 2 5000000>;
		};
		step@2 {
			enable-gpios = <&gpio 28 0>;
			enable = <1>;
		};
	};

However I think one of the reasons for Alex to choose his particular
representation is that people want this to be representable in the
platform data as well, which unfortunately can't deal very well with
this kind of situation.

Usually in the kernel you have one API to obtain a resource (pwm_get(),
regulator_get(), ...) which takes as an argument a device handle and a
name. For device tree, that API will lookup the resource by phandle,
while the non-DT code path typically uses a static lookup table.

It's a shame that we have to keep this kind of backwards compatibility
because it prevents these things from being represented in a logic way
in the DT sense.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12 10:04           ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2012-07-12 10:04 UTC (permalink / raw)
  To: Simon Glass
  Cc: Alexandre Courbot, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Thu, Jul 12, 2012 at 11:37:33AM +0200, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> > ---
> >  arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
> >  arch/arm/boot/dts/tegra20.dtsi        |  2 +-
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
> > index be90544..c67d9e1 100644
> > --- a/arch/arm/boot/dts/tegra20-ventana.dts
> > +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> > @@ -317,6 +317,37 @@
> >                 bus-width = <8>;
> >         };
> >
> 
> I would like to do something similar in U-Boot for Tegra, although
> perhaps not right away. For now I will go with something considerably
> simpler! But if this is merged into the kernel we will move to it in
> U-Boot. Anyway here are my comments:
> 
> > +       backlight {
> > +               compatible = "pwm-backlight";
> > +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> 
> We seem to have a lot of these - should we move to a range and step size?

These actually seem to be a little bogus. The reason for introducing the
levels was to allow calibration of these values because they in fact
usually do not scale linearly. Instead, a linear brightness increase
usually maps to something like a logarithmic (at best) increase of the
duty cycle.

> > +               default-brightness-level = <12>;
> > +
> > +               pwms = <&pwm 2 5000000>;
> > +               pwm-names = "backlight";
> > +               power-supply = <&backlight_reg>;
> > +               enable-gpios = <&gpio 28 0>;
> > +
> > +               power-on-sequence = "REGULATOR", "power", <1>,
> > +                                   "DELAY", <10>,
> > +                                   "PWM", "backlight", <1>,
> > +                                   "GPIO", "enable", <1>;
> 
> So the names REGULATOR, DELAY, etc. here are looked up through some
> existing mechanism? In general I am not a big fan of mixing strings
> and numbers in a property. Maybe something like:
> 
> power_on_sequence {
>    step@0 {
>       phandle = <&backlight_reg>;
>       enable = <1>;
>       post-delay = <10>;
>    }
>    step@1 {
>       phandle = <&pwm 2 5000000>;
>    }
>    step@2 {
>       phandle = <&gpio 28 0>;
>       enable = <1>;
>    }

I actually like that a lot. What's missing from your example is a way to
specify the type of the phandle because it cannot be safely inferred.
Perhaps this could be done by just the property name:

	power_on_sequence {
		step@0 {
			power-supply = <&backlight_reg>;
			enable = <1>;
			post-delay = <10>;
		};
		step@1 {
			pwm = <&pwm 2 5000000>;
		};
		step@2 {
			enable-gpios = <&gpio 28 0>;
			enable = <1>;
		};
	};

However I think one of the reasons for Alex to choose his particular
representation is that people want this to be representable in the
platform data as well, which unfortunately can't deal very well with
this kind of situation.

Usually in the kernel you have one API to obtain a resource (pwm_get(),
regulator_get(), ...) which takes as an argument a device handle and a
name. For device tree, that API will lookup the resource by phandle,
while the non-DT code path typically uses a static lookup table.

It's a shame that we have to keep this kind of backwards compatibility
because it prevents these things from being represented in a logic way
in the DT sense.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12 10:04           ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2012-07-12 10:04 UTC (permalink / raw)
  To: Simon Glass
  Cc: Alexandre Courbot, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Thu, Jul 12, 2012 at 11:37:33AM +0200, Simon Glass wrote:
> Hi Alex,
> 
> On Mon, Jul 9, 2012 at 8:08 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> > ---
> >  arch/arm/boot/dts/tegra20-ventana.dts | 31 +++++++++++++++++++++++++++++++
> >  arch/arm/boot/dts/tegra20.dtsi        |  2 +-
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
> > index be90544..c67d9e1 100644
> > --- a/arch/arm/boot/dts/tegra20-ventana.dts
> > +++ b/arch/arm/boot/dts/tegra20-ventana.dts
> > @@ -317,6 +317,37 @@
> >                 bus-width = <8>;
> >         };
> >
> 
> I would like to do something similar in U-Boot for Tegra, although
> perhaps not right away. For now I will go with something considerably
> simpler! But if this is merged into the kernel we will move to it in
> U-Boot. Anyway here are my comments:
> 
> > +       backlight {
> > +               compatible = "pwm-backlight";
> > +               brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
> 
> We seem to have a lot of these - should we move to a range and step size?

These actually seem to be a little bogus. The reason for introducing the
levels was to allow calibration of these values because they in fact
usually do not scale linearly. Instead, a linear brightness increase
usually maps to something like a logarithmic (at best) increase of the
duty cycle.

> > +               default-brightness-level = <12>;
> > +
> > +               pwms = <&pwm 2 5000000>;
> > +               pwm-names = "backlight";
> > +               power-supply = <&backlight_reg>;
> > +               enable-gpios = <&gpio 28 0>;
> > +
> > +               power-on-sequence = "REGULATOR", "power", <1>,
> > +                                   "DELAY", <10>,
> > +                                   "PWM", "backlight", <1>,
> > +                                   "GPIO", "enable", <1>;
> 
> So the names REGULATOR, DELAY, etc. here are looked up through some
> existing mechanism? In general I am not a big fan of mixing strings
> and numbers in a property. Maybe something like:
> 
> power_on_sequence {
>    step@0 {
>       phandle = <&backlight_reg>;
>       enable = <1>;
>       post-delay = <10>;
>    }
>    step@1 {
>       phandle = <&pwm 2 5000000>;
>    }
>    step@2 {
>       phandle = <&gpio 28 0>;
>       enable = <1>;
>    }

I actually like that a lot. What's missing from your example is a way to
specify the type of the phandle because it cannot be safely inferred.
Perhaps this could be done by just the property name:

	power_on_sequence {
		step@0 {
			power-supply = <&backlight_reg>;
			enable = <1>;
			post-delay = <10>;
		};
		step@1 {
			pwm = <&pwm 2 5000000>;
		};
		step@2 {
			enable-gpios = <&gpio 28 0>;
			enable = <1>;
		};
	};

However I think one of the reasons for Alex to choose his particular
representation is that people want this to be representable in the
platform data as well, which unfortunately can't deal very well with
this kind of situation.

Usually in the kernel you have one API to obtain a resource (pwm_get(),
regulator_get(), ...) which takes as an argument a device handle and a
name. For device tree, that API will lookup the resource by phandle,
while the non-DT code path typically uses a static lookup table.

It's a shame that we have to keep this kind of backwards compatibility
because it prevents these things from being represented in a logic way
in the DT sense.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
  2012-07-12  9:37       ` Simon Glass
  (?)
@ 2012-07-12 10:11         ` Alex Courbot
  -1 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-12 10:11 UTC (permalink / raw)
  To: Simon Glass
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

Hi Simon,

On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
> I would like to do something similar in U-Boot for Tegra, although
> perhaps not right away. For now I will go with something considerably
> simpler! But if this is merged into the kernel we will move to it in
> U-Boot.

Cool, I'd love to see that used in U-Boot as well.

>> +               default-brightness-level = <12>;
>> +
>> +               pwms = <&pwm 2 5000000>;
>> +               pwm-names = "backlight";
>> +               power-supply = <&backlight_reg>;
>> +               enable-gpios = <&gpio 28 0>;
>> +
>> +               power-on-sequence = "REGULATOR", "power", <1>,
>> +                                   "DELAY", <10>,
>> +                                   "PWM", "backlight", <1>,
>> +                                   "GPIO", "enable", <1>;
>
> So the names REGULATOR, DELAY, etc. here are looked up through some
> existing mechanism? In general I am not a big fan of mixing strings
> and numbers in a property.

Yes, these are strings we are looking up to know the type of the next 
element to power on/off in the sequence. I don't like these, honestly, 
and would rather have them replaced by constants - however there is no 
way to define constants in the DT AFAIK (but I have heard some other 
persons are interested in having them too), and this is the only way I 
have found to keep the sequence readable.

> Maybe something like:
>
> power_on_sequence {
>     step@0 {
>        phandle = <&backlight_reg>;
>        enable = <1>;
>        post-delay = <10>;
>     }
>     step@1 {
>        phandle = <&pwm 2 5000000>;
>     }
>     step@2 {
>        phandle = <&gpio 28 0>;
>        enable = <1>;
>     }

I see a few problems with this: first, how do you know the types of your 
phandles? At step 0, we should get a regulator, step 1 is a PWM and step 
2 is a GPIO. This is why I have used strings to identify the type of the 
phandle and the number (and type) of additional arguments to parse for a 
step.

Second, I am afraid the representation in memory would be significantly 
bigger since the properties names would have to be stored as well (I am 
no DT expert, please correct me if I am wrong). Lastly, the additional 
freedom of having extra properties might make the parser more complicated.

I agree the type strings are a problem in the current form - if we could 
get constants in the device tree, that would be much better. Your way of 
representing the sequences is interesting though, if we can solve the 
type issue (and also evaluate its cost in terms of memory footprint), it 
would be interesting to consider it as well.

Alex.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12 10:11         ` Alex Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-12 10:11 UTC (permalink / raw)
  To: Simon Glass
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

Hi Simon,

On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
> I would like to do something similar in U-Boot for Tegra, although
> perhaps not right away. For now I will go with something considerably
> simpler! But if this is merged into the kernel we will move to it in
> U-Boot.

Cool, I'd love to see that used in U-Boot as well.

>> +               default-brightness-level = <12>;
>> +
>> +               pwms = <&pwm 2 5000000>;
>> +               pwm-names = "backlight";
>> +               power-supply = <&backlight_reg>;
>> +               enable-gpios = <&gpio 28 0>;
>> +
>> +               power-on-sequence = "REGULATOR", "power", <1>,
>> +                                   "DELAY", <10>,
>> +                                   "PWM", "backlight", <1>,
>> +                                   "GPIO", "enable", <1>;
>
> So the names REGULATOR, DELAY, etc. here are looked up through some
> existing mechanism? In general I am not a big fan of mixing strings
> and numbers in a property.

Yes, these are strings we are looking up to know the type of the next 
element to power on/off in the sequence. I don't like these, honestly, 
and would rather have them replaced by constants - however there is no 
way to define constants in the DT AFAIK (but I have heard some other 
persons are interested in having them too), and this is the only way I 
have found to keep the sequence readable.

> Maybe something like:
>
> power_on_sequence {
>     step@0 {
>        phandle = <&backlight_reg>;
>        enable = <1>;
>        post-delay = <10>;
>     }
>     step@1 {
>        phandle = <&pwm 2 5000000>;
>     }
>     step@2 {
>        phandle = <&gpio 28 0>;
>        enable = <1>;
>     }

I see a few problems with this: first, how do you know the types of your 
phandles? At step 0, we should get a regulator, step 1 is a PWM and step 
2 is a GPIO. This is why I have used strings to identify the type of the 
phandle and the number (and type) of additional arguments to parse for a 
step.

Second, I am afraid the representation in memory would be significantly 
bigger since the properties names would have to be stored as well (I am 
no DT expert, please correct me if I am wrong). Lastly, the additional 
freedom of having extra properties might make the parser more complicated.

I agree the type strings are a problem in the current form - if we could 
get constants in the device tree, that would be much better. Your way of 
representing the sequences is interesting though, if we can solve the 
type issue (and also evaluate its cost in terms of memory footprint), it 
would be interesting to consider it as well.

Alex.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12 10:11         ` Alex Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-12 10:11 UTC (permalink / raw)
  To: Simon Glass
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

Hi Simon,

On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
> I would like to do something similar in U-Boot for Tegra, although
> perhaps not right away. For now I will go with something considerably
> simpler! But if this is merged into the kernel we will move to it in
> U-Boot.

Cool, I'd love to see that used in U-Boot as well.

>> +               default-brightness-level = <12>;
>> +
>> +               pwms = <&pwm 2 5000000>;
>> +               pwm-names = "backlight";
>> +               power-supply = <&backlight_reg>;
>> +               enable-gpios = <&gpio 28 0>;
>> +
>> +               power-on-sequence = "REGULATOR", "power", <1>,
>> +                                   "DELAY", <10>,
>> +                                   "PWM", "backlight", <1>,
>> +                                   "GPIO", "enable", <1>;
>
> So the names REGULATOR, DELAY, etc. here are looked up through some
> existing mechanism? In general I am not a big fan of mixing strings
> and numbers in a property.

Yes, these are strings we are looking up to know the type of the next 
element to power on/off in the sequence. I don't like these, honestly, 
and would rather have them replaced by constants - however there is no 
way to define constants in the DT AFAIK (but I have heard some other 
persons are interested in having them too), and this is the only way I 
have found to keep the sequence readable.

> Maybe something like:
>
> power_on_sequence {
>     step@0 {
>        phandle = <&backlight_reg>;
>        enable = <1>;
>        post-delay = <10>;
>     }
>     step@1 {
>        phandle = <&pwm 2 5000000>;
>     }
>     step@2 {
>        phandle = <&gpio 28 0>;
>        enable = <1>;
>     }

I see a few problems with this: first, how do you know the types of your 
phandles? At step 0, we should get a regulator, step 1 is a PWM and step 
2 is a GPIO. This is why I have used strings to identify the type of the 
phandle and the number (and type) of additional arguments to parse for a 
step.

Second, I am afraid the representation in memory would be significantly 
bigger since the properties names would have to be stored as well (I am 
no DT expert, please correct me if I am wrong). Lastly, the additional 
freedom of having extra properties might make the parser more complicated.

I agree the type strings are a problem in the current form - if we could 
get constants in the device tree, that would be much better. Your way of 
representing the sequences is interesting though, if we can solve the 
type issue (and also evaluate its cost in terms of memory footprint), it 
would be interesting to consider it as well.

Alex.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
  2012-07-12 10:11         ` Alex Courbot
  (?)
  (?)
@ 2012-07-12 14:27             ` Simon Glass
  -1 siblings, 0 replies; 34+ messages in thread
From: Simon Glass @ 2012-07-12 14:27 UTC (permalink / raw)
  To: Alex Courbot
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Hi Alex,

On Thu, Jul 12, 2012 at 12:11 PM, Alex Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Simon,
>
>
> On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
>>
>> I would like to do something similar in U-Boot for Tegra, although
>> perhaps not right away. For now I will go with something considerably
>> simpler! But if this is merged into the kernel we will move to it in
>> U-Boot.
>
>
> Cool, I'd love to see that used in U-Boot as well.
>
>
>>> +               default-brightness-level = <12>;
>>> +
>>> +               pwms = <&pwm 2 5000000>;
>>> +               pwm-names = "backlight";
>>> +               power-supply = <&backlight_reg>;
>>> +               enable-gpios = <&gpio 28 0>;
>>> +
>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;
>>
>>
>> So the names REGULATOR, DELAY, etc. here are looked up through some
>> existing mechanism? In general I am not a big fan of mixing strings
>> and numbers in a property.
>
>
> Yes, these are strings we are looking up to know the type of the next
> element to power on/off in the sequence. I don't like these, honestly, and
> would rather have them replaced by constants - however there is no way to
> define constants in the DT AFAIK (but I have heard some other persons are
> interested in having them too), and this is the only way I have found to
> keep the sequence readable.

That might be the 100th time that I have heard this. I have brought a
patch from Stephen Warren into our tree to solve this locally, but it
is not merged upstream.

>
>
>> Maybe something like:
>>
>> power_on_sequence {
>>     step@0 {
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }
>
>
> I see a few problems with this: first, how do you know the types of your
> phandles? At step 0, we should get a regulator, step 1 is a PWM and step 2
> is a GPIO. This is why I have used strings to identify the type of the
> phandle and the number (and type) of additional arguments to parse for a
> step.

Well it's similar to giving them names - you would need to look up the
phandle and see its compatible string or which driver type owns it.
Maybe too complicated, and no such infrastructure exists, so:

>> power_on_sequence {
>>     step@0 {
            type = "regulator";
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
            type = "pwm";
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
            type = "gpio";
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }

>
> Second, I am afraid the representation in memory would be significantly
> bigger since the properties names would have to be stored as well (I am no
> DT expert, please correct me if I am wrong). Lastly, the additional freedom
> of having extra properties might make the parser more complicated.

The property names are only stored one, in the string table. I am not
sure about parsing complexity, but it might actually be easier.

>
> I agree the type strings are a problem in the current form - if we could get
> constants in the device tree, that would be much better. Your way of
> representing the sequences is interesting though, if we can solve the type
> issue (and also evaluate its cost in terms of memory footprint), it would be
> interesting to consider it as well.

At a guess:

>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;

About 106 bytes I think

>>     step@0 { 16
            type = "regulator"; 24
>>        phandle = <&backlight_reg>; 16
>>        enable = <1>; 16
>>        post-delay = <10>; 16
>>     }
>>     step@1 { 16
            type = "pwm"; 16
>>        phandle = <&pwm 2 5000000>; 24
>>     }
>>     step@2 { 16
            type = "gpio"; 20
>>        phandle = <&gpio 28 0>; 24
>>        enable = <1>; 16
>>     }

220?

From my understanding mixing strings and numbers in a property is
frowned on though.

>
> Alex.

Regards,
Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12 14:27             ` Simon Glass
  0 siblings, 0 replies; 34+ messages in thread
From: Simon Glass @ 2012-07-12 14:27 UTC (permalink / raw)
  To: Alex Courbot
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

Hi Alex,

On Thu, Jul 12, 2012 at 12:11 PM, Alex Courbot <acourbot@nvidia.com> wrote:
> Hi Simon,
>
>
> On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
>>
>> I would like to do something similar in U-Boot for Tegra, although
>> perhaps not right away. For now I will go with something considerably
>> simpler! But if this is merged into the kernel we will move to it in
>> U-Boot.
>
>
> Cool, I'd love to see that used in U-Boot as well.
>
>
>>> +               default-brightness-level = <12>;
>>> +
>>> +               pwms = <&pwm 2 5000000>;
>>> +               pwm-names = "backlight";
>>> +               power-supply = <&backlight_reg>;
>>> +               enable-gpios = <&gpio 28 0>;
>>> +
>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;
>>
>>
>> So the names REGULATOR, DELAY, etc. here are looked up through some
>> existing mechanism? In general I am not a big fan of mixing strings
>> and numbers in a property.
>
>
> Yes, these are strings we are looking up to know the type of the next
> element to power on/off in the sequence. I don't like these, honestly, and
> would rather have them replaced by constants - however there is no way to
> define constants in the DT AFAIK (but I have heard some other persons are
> interested in having them too), and this is the only way I have found to
> keep the sequence readable.

That might be the 100th time that I have heard this. I have brought a
patch from Stephen Warren into our tree to solve this locally, but it
is not merged upstream.

>
>
>> Maybe something like:
>>
>> power_on_sequence {
>>     step@0 {
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }
>
>
> I see a few problems with this: first, how do you know the types of your
> phandles? At step 0, we should get a regulator, step 1 is a PWM and step 2
> is a GPIO. This is why I have used strings to identify the type of the
> phandle and the number (and type) of additional arguments to parse for a
> step.

Well it's similar to giving them names - you would need to look up the
phandle and see its compatible string or which driver type owns it.
Maybe too complicated, and no such infrastructure exists, so:

>> power_on_sequence {
>>     step@0 {
            type = "regulator";
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
            type = "pwm";
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
            type = "gpio";
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }

>
> Second, I am afraid the representation in memory would be significantly
> bigger since the properties names would have to be stored as well (I am no
> DT expert, please correct me if I am wrong). Lastly, the additional freedom
> of having extra properties might make the parser more complicated.

The property names are only stored one, in the string table. I am not
sure about parsing complexity, but it might actually be easier.

>
> I agree the type strings are a problem in the current form - if we could get
> constants in the device tree, that would be much better. Your way of
> representing the sequences is interesting though, if we can solve the type
> issue (and also evaluate its cost in terms of memory footprint), it would be
> interesting to consider it as well.

At a guess:

>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;

About 106 bytes I think

>>     step@0 { 16
            type = "regulator"; 24
>>        phandle = <&backlight_reg>; 16
>>        enable = <1>; 16
>>        post-delay = <10>; 16
>>     }
>>     step@1 { 16
            type = "pwm"; 16
>>        phandle = <&pwm 2 5000000>; 24
>>     }
>>     step@2 { 16
            type = "gpio"; 20
>>        phandle = <&gpio 28 0>; 24
>>        enable = <1>; 16
>>     }

220?

>From my understanding mixing strings and numbers in a property is
frowned on though.

>
> Alex.

Regards,
Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12 14:27             ` Simon Glass
  0 siblings, 0 replies; 34+ messages in thread
From: Simon Glass @ 2012-07-12 14:27 UTC (permalink / raw)
  To: Alex Courbot
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Hi Alex,

On Thu, Jul 12, 2012 at 12:11 PM, Alex Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Simon,
>
>
> On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
>>
>> I would like to do something similar in U-Boot for Tegra, although
>> perhaps not right away. For now I will go with something considerably
>> simpler! But if this is merged into the kernel we will move to it in
>> U-Boot.
>
>
> Cool, I'd love to see that used in U-Boot as well.
>
>
>>> +               default-brightness-level = <12>;
>>> +
>>> +               pwms = <&pwm 2 5000000>;
>>> +               pwm-names = "backlight";
>>> +               power-supply = <&backlight_reg>;
>>> +               enable-gpios = <&gpio 28 0>;
>>> +
>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;
>>
>>
>> So the names REGULATOR, DELAY, etc. here are looked up through some
>> existing mechanism? In general I am not a big fan of mixing strings
>> and numbers in a property.
>
>
> Yes, these are strings we are looking up to know the type of the next
> element to power on/off in the sequence. I don't like these, honestly, and
> would rather have them replaced by constants - however there is no way to
> define constants in the DT AFAIK (but I have heard some other persons are
> interested in having them too), and this is the only way I have found to
> keep the sequence readable.

That might be the 100th time that I have heard this. I have brought a
patch from Stephen Warren into our tree to solve this locally, but it
is not merged upstream.

>
>
>> Maybe something like:
>>
>> power_on_sequence {
>>     step@0 {
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }
>
>
> I see a few problems with this: first, how do you know the types of your
> phandles? At step 0, we should get a regulator, step 1 is a PWM and step 2
> is a GPIO. This is why I have used strings to identify the type of the
> phandle and the number (and type) of additional arguments to parse for a
> step.

Well it's similar to giving them names - you would need to look up the
phandle and see its compatible string or which driver type owns it.
Maybe too complicated, and no such infrastructure exists, so:

>> power_on_sequence {
>>     step@0 {
            type = "regulator";
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
            type = "pwm";
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
            type = "gpio";
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }

>
> Second, I am afraid the representation in memory would be significantly
> bigger since the properties names would have to be stored as well (I am no
> DT expert, please correct me if I am wrong). Lastly, the additional freedom
> of having extra properties might make the parser more complicated.

The property names are only stored one, in the string table. I am not
sure about parsing complexity, but it might actually be easier.

>
> I agree the type strings are a problem in the current form - if we could get
> constants in the device tree, that would be much better. Your way of
> representing the sequences is interesting though, if we can solve the type
> issue (and also evaluate its cost in terms of memory footprint), it would be
> interesting to consider it as well.

At a guess:

>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;

About 106 bytes I think

>>     step@0 { 16
            type = "regulator"; 24
>>        phandle = <&backlight_reg>; 16
>>        enable = <1>; 16
>>        post-delay = <10>; 16
>>     }
>>     step@1 { 16
            type = "pwm"; 16
>>        phandle = <&pwm 2 5000000>; 24
>>     }
>>     step@2 { 16
            type = "gpio"; 20
>>        phandle = <&gpio 28 0>; 24
>>        enable = <1>; 16
>>     }

220?

>From my understanding mixing strings and numbers in a property is
frowned on though.

>
> Alex.

Regards,
Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-12 14:27             ` Simon Glass
  0 siblings, 0 replies; 34+ messages in thread
From: Simon Glass @ 2012-07-12 14:27 UTC (permalink / raw)
  To: Alex Courbot
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ

Hi Alex,

On Thu, Jul 12, 2012 at 12:11 PM, Alex Courbot <acourbot@nvidia.com> wrote:
> Hi Simon,
>
>
> On Thu 12 Jul 2012 06:37:33 PM JST, Simon Glass wrote:
>>
>> I would like to do something similar in U-Boot for Tegra, although
>> perhaps not right away. For now I will go with something considerably
>> simpler! But if this is merged into the kernel we will move to it in
>> U-Boot.
>
>
> Cool, I'd love to see that used in U-Boot as well.
>
>
>>> +               default-brightness-level = <12>;
>>> +
>>> +               pwms = <&pwm 2 5000000>;
>>> +               pwm-names = "backlight";
>>> +               power-supply = <&backlight_reg>;
>>> +               enable-gpios = <&gpio 28 0>;
>>> +
>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;
>>
>>
>> So the names REGULATOR, DELAY, etc. here are looked up through some
>> existing mechanism? In general I am not a big fan of mixing strings
>> and numbers in a property.
>
>
> Yes, these are strings we are looking up to know the type of the next
> element to power on/off in the sequence. I don't like these, honestly, and
> would rather have them replaced by constants - however there is no way to
> define constants in the DT AFAIK (but I have heard some other persons are
> interested in having them too), and this is the only way I have found to
> keep the sequence readable.

That might be the 100th time that I have heard this. I have brought a
patch from Stephen Warren into our tree to solve this locally, but it
is not merged upstream.

>
>
>> Maybe something like:
>>
>> power_on_sequence {
>>     step@0 {
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }
>
>
> I see a few problems with this: first, how do you know the types of your
> phandles? At step 0, we should get a regulator, step 1 is a PWM and step 2
> is a GPIO. This is why I have used strings to identify the type of the
> phandle and the number (and type) of additional arguments to parse for a
> step.

Well it's similar to giving them names - you would need to look up the
phandle and see its compatible string or which driver type owns it.
Maybe too complicated, and no such infrastructure exists, so:

>> power_on_sequence {
>>     step@0 {
            type = "regulator";
>>        phandle = <&backlight_reg>;
>>        enable = <1>;
>>        post-delay = <10>;
>>     }
>>     step@1 {
            type = "pwm";
>>        phandle = <&pwm 2 5000000>;
>>     }
>>     step@2 {
            type = "gpio";
>>        phandle = <&gpio 28 0>;
>>        enable = <1>;
>>     }

>
> Second, I am afraid the representation in memory would be significantly
> bigger since the properties names would have to be stored as well (I am no
> DT expert, please correct me if I am wrong). Lastly, the additional freedom
> of having extra properties might make the parser more complicated.

The property names are only stored one, in the string table. I am not
sure about parsing complexity, but it might actually be easier.

>
> I agree the type strings are a problem in the current form - if we could get
> constants in the device tree, that would be much better. Your way of
> representing the sequences is interesting though, if we can solve the type
> issue (and also evaluate its cost in terms of memory footprint), it would be
> interesting to consider it as well.

At a guess:

>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>> +                                   "DELAY", <10>,
>>> +                                   "PWM", "backlight", <1>,
>>> +                                   "GPIO", "enable", <1>;

About 106 bytes I think

>>     step@0 { 16
            type = "regulator"; 24
>>        phandle = <&backlight_reg>; 16
>>        enable = <1>; 16
>>        post-delay = <10>; 16
>>     }
>>     step@1 { 16
            type = "pwm"; 16
>>        phandle = <&pwm 2 5000000>; 24
>>     }
>>     step@2 { 16
            type = "gpio"; 20
>>        phandle = <&gpio 28 0>; 24
>>        enable = <1>; 16
>>     }

220?

From my understanding mixing strings and numbers in a property is
frowned on though.

>
> Alex.

Regards,
Simon

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
  2012-07-12 14:27             ` Simon Glass
  (?)
@ 2012-07-13  5:32               ` Alex Courbot
  -1 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-13  5:32 UTC (permalink / raw)
  To: Simon Glass
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

On 07/12/2012 11:27 PM, Simon Glass wrote
>> I agree the type strings are a problem in the current form - if we could get
>> constants in the device tree, that would be much better. Your way of
>> representing the sequences is interesting though, if we can solve the type
>> issue (and also evaluate its cost in terms of memory footprint), it would be
>> interesting to consider it as well.
>
> At a guess:
>
>>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>>> +                                   "DELAY", <10>,
>>>> +                                   "PWM", "backlight", <1>,
>>>> +                                   "GPIO", "enable", <1>;
>
> About 106 bytes I think
>
>>>      step@0 { 16
>              type = "regulator"; 24
>>>         phandle = <&backlight_reg>; 16
>>>         enable = <1>; 16
>>>         post-delay = <10>; 16
>>>      }
>>>      step@1 { 16
>              type = "pwm"; 16
>>>         phandle = <&pwm 2 5000000>; 24
>>>      }
>>>      step@2 { 16
>              type = "gpio"; 20
>>>         phandle = <&gpio 28 0>; 24
>>>         enable = <1>; 16
>>>      }
>
> 220?

I compiled both versions to try it out. Your version was just 50 bytes 
larger than mine (I assumed that with yours, we would be able to remove 
the top-level pwm/regulator/gpio definitions that are referred by the 
sequence). The question here is do we want to have something more 
DT-ish, or are we trying to save every possible byte in the DT structure?

As Thierry also mentionned, we are trying to provide the same feature 
using the platform interface. I am not sure how we can elegantly support 
both ways through this.

>  From my understanding mixing strings and numbers in a property is
> frowned on though.

But doesn't it make sense in the current case? The power sequence is 
basically a program that is run by an interpreter. From this 
perspective, it makes more sense to me to have it as a binary field 
rather than a hierarchy of nodes and properties that will be harder to 
parse and will make error detection more complicated. I don't really see 
any practical benefit from turning the steps into sub-nodes, but then 
again I am not so familiar with the DT.

Alex.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-13  5:32               ` Alex Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-13  5:32 UTC (permalink / raw)
  To: Simon Glass
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

On 07/12/2012 11:27 PM, Simon Glass wrote
>> I agree the type strings are a problem in the current form - if we could get
>> constants in the device tree, that would be much better. Your way of
>> representing the sequences is interesting though, if we can solve the type
>> issue (and also evaluate its cost in terms of memory footprint), it would be
>> interesting to consider it as well.
>
> At a guess:
>
>>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>>> +                                   "DELAY", <10>,
>>>> +                                   "PWM", "backlight", <1>,
>>>> +                                   "GPIO", "enable", <1>;
>
> About 106 bytes I think
>
>>>      step@0 { 16
>              type = "regulator"; 24
>>>         phandle = <&backlight_reg>; 16
>>>         enable = <1>; 16
>>>         post-delay = <10>; 16
>>>      }
>>>      step@1 { 16
>              type = "pwm"; 16
>>>         phandle = <&pwm 2 5000000>; 24
>>>      }
>>>      step@2 { 16
>              type = "gpio"; 20
>>>         phandle = <&gpio 28 0>; 24
>>>         enable = <1>; 16
>>>      }
>
> 220?

I compiled both versions to try it out. Your version was just 50 bytes 
larger than mine (I assumed that with yours, we would be able to remove 
the top-level pwm/regulator/gpio definitions that are referred by the 
sequence). The question here is do we want to have something more 
DT-ish, or are we trying to save every possible byte in the DT structure?

As Thierry also mentionned, we are trying to provide the same feature 
using the platform interface. I am not sure how we can elegantly support 
both ways through this.

>  From my understanding mixing strings and numbers in a property is
> frowned on though.

But doesn't it make sense in the current case? The power sequence is 
basically a program that is run by an interpreter. From this 
perspective, it makes more sense to me to have it as a binary field 
rather than a hierarchy of nodes and properties that will be harder to 
parse and will make error detection more complicated. I don't really see 
any practical benefit from turning the steps into sub-nodes, but then 
again I am not so familiar with the DT.

Alex.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-13  5:32               ` Alex Courbot
  0 siblings, 0 replies; 34+ messages in thread
From: Alex Courbot @ 2012-07-13  5:32 UTC (permalink / raw)
  To: Simon Glass
  Cc: Thierry Reding, linux-tegra, linux-kernel, linux-fbdev,
	devicetree-discuss

On 07/12/2012 11:27 PM, Simon Glass wrote
>> I agree the type strings are a problem in the current form - if we could get
>> constants in the device tree, that would be much better. Your way of
>> representing the sequences is interesting though, if we can solve the type
>> issue (and also evaluate its cost in terms of memory footprint), it would be
>> interesting to consider it as well.
>
> At a guess:
>
>>>> +               power-on-sequence = "REGULATOR", "power", <1>,
>>>> +                                   "DELAY", <10>,
>>>> +                                   "PWM", "backlight", <1>,
>>>> +                                   "GPIO", "enable", <1>;
>
> About 106 bytes I think
>
>>>      step@0 { 16
>              type = "regulator"; 24
>>>         phandle = <&backlight_reg>; 16
>>>         enable = <1>; 16
>>>         post-delay = <10>; 16
>>>      }
>>>      step@1 { 16
>              type = "pwm"; 16
>>>         phandle = <&pwm 2 5000000>; 24
>>>      }
>>>      step@2 { 16
>              type = "gpio"; 20
>>>         phandle = <&gpio 28 0>; 24
>>>         enable = <1>; 16
>>>      }
>
> 220?

I compiled both versions to try it out. Your version was just 50 bytes 
larger than mine (I assumed that with yours, we would be able to remove 
the top-level pwm/regulator/gpio definitions that are referred by the 
sequence). The question here is do we want to have something more 
DT-ish, or are we trying to save every possible byte in the DT structure?

As Thierry also mentionned, we are trying to provide the same feature 
using the platform interface. I am not sure how we can elegantly support 
both ways through this.

>  From my understanding mixing strings and numbers in a property is
> frowned on though.

But doesn't it make sense in the current case? The power sequence is 
basically a program that is run by an interpreter. From this 
perspective, it makes more sense to me to have it as a binary field 
rather than a hierarchy of nodes and properties that will be harder to 
parse and will make error detection more complicated. I don't really see 
any practical benefit from turning the steps into sub-nodes, but then 
again I am not so familiar with the DT.

Alex.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
  2012-07-13  5:32               ` Alex Courbot
  (?)
@ 2012-07-23 20:38                 ` Stephen Warren
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Warren @ 2012-07-23 20:38 UTC (permalink / raw)
  To: Alex Courbot
  Cc: Simon Glass, Thierry Reding, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss

On 07/12/2012 11:32 PM, Alex Courbot wrote:
> On 07/12/2012 11:27 PM, Simon Glass wrote
...
>>  From my understanding mixing strings and numbers in a property is
>> frowned on though.
> 
> But doesn't it make sense in the current case? The power sequence is
> basically a program that is run by an interpreter. From this
> perspective, it makes more sense to me to have it as a binary field
> rather than a hierarchy of nodes and properties that will be harder to
> parse and will make error detection more complicated. I don't really see
> any practical benefit from turning the steps into sub-nodes, but then
> again I am not so familiar with the DT.

Mixing strings and integers in a property isn't "allowed" (by convention
of DT bindings reviewers - as you noticed, dtc will happily compile it);
there are practical issues with attempting to do so, such as causing the
integer values to be unaligned, and thus causing the current integer
parsing code to fail, etc.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-23 20:38                 ` Stephen Warren
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Warren @ 2012-07-23 20:38 UTC (permalink / raw)
  To: Alex Courbot
  Cc: Simon Glass, Thierry Reding, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss

On 07/12/2012 11:32 PM, Alex Courbot wrote:
> On 07/12/2012 11:27 PM, Simon Glass wrote
...
>>  From my understanding mixing strings and numbers in a property is
>> frowned on though.
> 
> But doesn't it make sense in the current case? The power sequence is
> basically a program that is run by an interpreter. From this
> perspective, it makes more sense to me to have it as a binary field
> rather than a hierarchy of nodes and properties that will be harder to
> parse and will make error detection more complicated. I don't really see
> any practical benefit from turning the steps into sub-nodes, but then
> again I am not so familiar with the DT.

Mixing strings and integers in a property isn't "allowed" (by convention
of DT bindings reviewers - as you noticed, dtc will happily compile it);
there are practical issues with attempting to do so, such as causing the
integer values to be unaligned, and thus causing the current integer
parsing code to fail, etc.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes
@ 2012-07-23 20:38                 ` Stephen Warren
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Warren @ 2012-07-23 20:38 UTC (permalink / raw)
  To: Alex Courbot
  Cc: Simon Glass, Thierry Reding, linux-tegra, linux-kernel,
	linux-fbdev, devicetree-discuss

On 07/12/2012 11:32 PM, Alex Courbot wrote:
> On 07/12/2012 11:27 PM, Simon Glass wrote
...
>>  From my understanding mixing strings and numbers in a property is
>> frowned on though.
> 
> But doesn't it make sense in the current case? The power sequence is
> basically a program that is run by an interpreter. From this
> perspective, it makes more sense to me to have it as a binary field
> rather than a hierarchy of nodes and properties that will be harder to
> parse and will make error detection more complicated. I don't really see
> any practical benefit from turning the steps into sub-nodes, but then
> again I am not so familiar with the DT.

Mixing strings and integers in a property isn't "allowed" (by convention
of DT bindings reviewers - as you noticed, dtc will happily compile it);
there are practical issues with attempting to do so, such as causing the
integer values to be unaligned, and thus causing the current integer
parsing code to fail, etc.

^ permalink raw reply	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2012-07-23 20:38 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-09  6:08 [RFC][PATCHv2 0/3] Power sequences interpreter for pwm_backlight Alexandre Courbot
2012-07-09  6:08 ` Alexandre Courbot
2012-07-09  6:08 ` Alexandre Courbot
2012-07-09  6:08 ` [RFC][PATCH V2 1/3] power sequences interpreter for device tree Alexandre Courbot
2012-07-09  6:08   ` Alexandre Courbot
2012-07-09  6:08   ` Alexandre Courbot
2012-07-09  6:08 ` [RFC][PATCH V2 2/3] pwm_backlight: use power sequences Alexandre Courbot
2012-07-09  6:08   ` Alexandre Courbot
2012-07-09  6:08   ` Alexandre Courbot
     [not found]   ` <1341814105-20690-3-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-07-09  7:48     ` Alex Courbot
2012-07-09  7:48       ` Alex Courbot
2012-07-09  7:48       ` Alex Courbot
2012-07-09  6:08 ` [RFC][PATCH V2 3/3] tegra: add pwm backlight device tree nodes Alexandre Courbot
2012-07-09  6:08   ` Alexandre Courbot
2012-07-09  6:08   ` Alexandre Courbot
     [not found]   ` <1341814105-20690-4-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-07-12  9:37     ` Simon Glass
2012-07-12  9:37       ` Simon Glass
2012-07-12  9:37       ` Simon Glass
     [not found]       ` <CAPnjgZ1QbjE+-tr0c01K2feUdEE2wMBfR=bKpTxnyDOJbY8+1Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-07-12 10:04         ` Thierry Reding
2012-07-12 10:04           ` Thierry Reding
2012-07-12 10:04           ` Thierry Reding
2012-07-12 10:11       ` Alex Courbot
2012-07-12 10:11         ` Alex Courbot
2012-07-12 10:11         ` Alex Courbot
     [not found]         ` <4FFEA2D4.9050308-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2012-07-12 14:27           ` Simon Glass
2012-07-12 14:27             ` Simon Glass
2012-07-12 14:27             ` Simon Glass
2012-07-12 14:27             ` Simon Glass
2012-07-13  5:32             ` Alex Courbot
2012-07-13  5:32               ` Alex Courbot
2012-07-13  5:32               ` Alex Courbot
2012-07-23 20:38               ` Stephen Warren
2012-07-23 20:38                 ` Stephen Warren
2012-07-23 20:38                 ` Stephen Warren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.