* [PATCH v4 0/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw)
To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann
Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
devicetree-discuss, linux-doc, Alexandre Courbot
Overdue revision of this new feature, some changes required additional thought
and rework.
The most important change is in the way power sequences are expressed in the
device tree. In order to avoid having to specify #address-cells, #size-cells and
reg properties, the @ notation in the step names is dropped, and instead a
fixed, sequential naming is adopted. The type of the resource used by a step is
decided by the presence of some recognized properties:
power-on-sequence {
step0 {
regulator = "power";
enable;
};
step1 {
delay = <10000>;
};
step2 {
pwm = "backlight";
enable;
};
...
To me this looks safe, clear and close to the platform data representation, but
needs approval from DT experts.
Resources are still referenced by name instead of having their phandles defined
directly inside the sequences, as previous discussion came to the conclusion
that doing so would require controversial changes to the regulator and PWM
frameworks, and that having the resources declared at the device level was
making sense logically speaking.
Other changes/fixes since last revision:
* Move to drivers/power/ (hope this is ok with the maintainers?)
* Use microseconds for delay
* Use devm for PWM resources and remove cleanup function as all resources are
devm-managed
* Remove "-gpio" suffix for GPIO reference in the driver
* Remove params structure
* Make power_seq structure private
* Number of steps in a sequence is explicitly stated instead of resorting to a
"stop" sequence step
* Delays are a step instead of being a step parameter
* Use flexible member arrays to limit number of memory allocations
* Add documentation to DT bindings
There was a lot of feedback on the previous version (thanks!) so if I forgot
to address some important point, please bring it to my attention again.
Alexandre Courbot (3):
Runtime Interpreted Power Sequences
pwm_backlight: use power sequences
tegra: add pwm backlight device tree nodes
.../devicetree/bindings/power_seq/power_seq.txt | 101 +++++
.../bindings/video/backlight/pwm-backlight.txt | 62 ++-
Documentation/power/power_seq.txt | 129 +++++++
arch/arm/boot/dts/tegra20-ventana.dts | 58 +++
arch/arm/boot/dts/tegra20.dtsi | 2 +-
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +++++++++++++++++++++
drivers/video/backlight/Kconfig | 1 +
drivers/video/backlight/pwm_bl.c | 192 +++++++---
include/linux/power_seq.h | 142 +++++++
include/linux/pwm_backlight.h | 16 +-
12 files changed, 1071 insertions(+), 56 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
--
1.7.11.4
^ permalink raw reply
* [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw)
To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann
Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
devicetree-discuss, linux-doc, Alexandre Courbot
In-Reply-To: <1345097337-24170-1-git-send-email-acourbot@nvidia.com>
Some device drivers (panel backlights especially) need to follow precise
sequences for powering on and off, involving gpios, regulators, PWMs
with a precise powering order and delays to respect between each steps.
These sequences are board-specific, and do not belong to a particular
driver - therefore they have been performed by board-specific hook
functions to far.
With the advent of the device tree and of ARM kernels that are not
board-tied, we cannot rely on these board-specific hooks anymore but
need a way to implement these sequences in a portable manner. This patch
introduces a simple interpreter that can execute such power sequences
encoded either as platform data or within the device tree.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
.../devicetree/bindings/power_seq/power_seq.txt | 101 +++++
Documentation/power/power_seq.txt | 129 +++++++
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +++++++++++++++++++++
include/linux/power_seq.h | 142 +++++++
6 files changed, 796 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
new file mode 100644
index 0000000..749c6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
@@ -0,0 +1,101 @@
+Specifying Power Sequences in the Device Tree
+======================+In the device tree, power sequences are specified as sub-nodes of the device
+node and reference resources declared by that device.
+
+For an introduction about runtime interpreted power sequences, see
+Documentation/power/power_seq.txt and include/linux/power_seq.h.
+
+Power Sequences Structure
+-------------------------
+Power sequences are sub-nodes that are named such as the device driver can find
+them. The driver's documentation should list the sequence names it recognizes.
+
+Inside a power sequence node are sub-nodes that describe the different steps
+of the sequence. Each step must be named sequentially, with the first step
+named step0, the second step1, etc. Failure to follow this rule will result in a
+parsing error.
+
+Power Sequences Steps
+---------------------
+Every step of a sequence describes an action to be performed on a resource. It
+generally includes a property named after the resource type, and which value
+references the resource to be used. Depending on the resource type, additional
+properties can be defined to control the action to be performed.
+
+Currently supported resource types are:
+- "delay", which should simply contain a delay in microseconds to wait before
+ going on with the rest of the sequence. It takes no additional property.
+- "regulator" contains the name of a regulator to be acquired using
+ regulator_get(). An additional property, either "enable" or "disable", must be
+ present to control whether the regulator should be enabled or disabled during
+ that step.
+- "pwm" is set to the name of a PWM acquired using pwm_get(). As with regulator,
+ an additional "enable" or "disable" property is required.
+- "gpio" contains the name of a GPIO to enable or disable using the same
+ additional property as regulator or pwm. The gpio is resolved by appending
+ "-gpio" to the given name and looking for a device property with a GPIO
+ phandle.
+
+Example
+-------
+Here are example sequences declared within a backlight device that use all the
+supported resources types:
+
+ backlight {
+ compatible = "pwm-backlight";
+ ...
+
+ /* resources used by the sequences */
+ pwms = <&pwm 2 5000000>;
+ pwm-names = "backlight";
+ power-supply = <&backlight_reg>;
+ enable-gpio = <&gpio 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ pwm = "backlight";
+ enable;
+ };
+ step3 {
+ gpio = "enable";
+ enable;
+ };
+ };
+
+ power-off-sequence {
+ step0 {
+ gpio = "enable";
+ disable;
+ };
+ step1 {
+ pwm = "backlight";
+ disable;
+ };
+ step2 {
+ delay = <10000>;
+ };
+ step3 {
+ regulator = "power";
+ disable;
+ };
+ };
+ };
+
+The first part lists the PWM, regulator, and GPIO resources used by the
+sequences. These resources will be requested on behalf of the backlight device
+when the sequences are built and are declared according to their own framework
+in a way that makes them accessible by name.
+
+After the resources declaration, two sequences follow for powering the backlight
+on and off. Their names are specified by the pwm-backlight driver. Every step
+uses one of the "delay", "regulator", "pwm" or "gpio" properties to reference a
+previously-declared resource. Additional "enable" or "disable" properties are
+also used as needed.
diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
new file mode 100644
index 0000000..3ab4f93
--- /dev/null
+++ b/Documentation/power/power_seq.txt
@@ -0,0 +1,129 @@
+Runtime Interpreted Power Sequences
+=================+
+Problem
+-------
+One very common board-dependent code is the out-of-driver code that is used to
+turn a device on or off. For instance, SoC boards very commonly use a GPIO
+(abstracted to a regulator or not) to control the power supply of a backlight,
+disabling it when the backlight is not used in order to save power. The GPIO
+that should be used, however, as well as the exact power sequence that may
+also involve other resources, is board-dependent and thus unknown of the driver.
+
+This was previously addressed by having hooks in the device's platform data that
+are called whenever the state of the device might reflect a power change. This
+approach, however, introduces board-dependant code into the kernel and is not
+compatible with the device tree.
+
+The Runtime Interpreted Power Sequences (or power sequences for short) aims at
+turning this code into platform data or device tree nodes. Power sequences are
+described using a simple format and run by a simple interpreter whenever needed.
+This allows to remove the callback mechanism and makes the kernel less
+board-dependant.
+
+What are Power Sequences?
+-------------------------
+Power sequences are a series of sequential steps during which an action is
+performed on a resource. The supported resources so far are:
+- delay (just wait for the delay given in microseconds)
+- GPIO (enable or disable)
+- regulator (enable or disable)
+- PWM (enable or disable)
+
+Every step designates a resource type and parameters that are relevant to it.
+For instance, GPIO and PWMs can be enabled or disabled.
+
+When a power sequence is run, each of its step is executed sequentially until
+one step fails or the end of the sequence is reached.
+
+Power sequences can be declared as platform data or in the device tree.
+
+Platform Data Format
+--------------------
+All relevant data structures for declaring power sequences are located in
+include/linux/power_seq.h.
+
+The platform data is a static instance of simple array of
+platform_power_seq_step instances, each
+instance describing a step. The type as well as one of id or gpio members
+(depending on the type) must be specified. The last step must be of type
+POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
+identified by number. For example, the following sequence will turn on the
+"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:
+
+static struct platform_power_seq power_on_seq = {
+ .nb_steps = 3,
+ .steps = {
+ {
+ .type = POWER_SEQ_REGULATOR,
+ .regulator.regulator = "power",
+ .regulator.enable = 1,
+ },
+ {
+ .type = POWER_SEQ_DELAY,
+ .delay.delay_us = 10000,
+ },
+ {
+ .type = POWER_SEQ_GPIO,
+ .gpio.gpio = 110,
+ .gpio.enable = 1,
+ },
+ },
+};
+
+Device Tree
+-----------
+Power sequences can also be encoded as device tree nodes. The following
+properties and nodes are equivalent to the platform data defined previously:
+
+ power-supply = <&power_reg>;
+ switch-gpio = <&gpio 110 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ gpio = "switch";
+ enable;
+ };
+ };
+
+See Documentation/devicetree/bindings/power_seq/power_seq.txt for the complete
+syntax of the bindings.
+
+Usage by Drivers and Resources Management
+-----------------------------------------
+Power sequences make use of resources that must be properly allocated and
+managed. The power_seq_build() function builds a power sequence from the
+platform data. It also takes care of resolving and allocating the resources
+referenced by the sequence if needed:
+
+ struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
+ struct platform_power_seq *pseq);
+
+The 'dev' argument is the device in the name of which the resources are to be
+allocated.
+
+The 'ress' argument is a list to which the resolved resources are appended. This
+avoids allocating a resource referenced in several power sequences multiple
+times.
+
+On success, the function returns a devm allocated resolved sequence that is
+ready to be passed to power_seq_run(). In case of failure, and error code is
+returned.
+
+A resolved power sequence returned by power_seq_build can be run by
+power_run_run():
+
+ int power_seq_run(power_seq *seq);
+
+It returns 0 if the sequence has successfully been run, or an error code if a
+problem occured.
+
+There is no need to explicitly free the resources used by the sequence as they
+are devm-allocated.
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index c1892f3..4172fe4 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -312,4 +312,7 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
thermistor connected on BATCTRL ADC.
endif # POWER_SUPPLY
+config POWER_SEQ
+ bool
+
source "drivers/power/avs/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index ee58afb..828859c 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
obj-$(CONFIG_POWER_AVS) += avs/
obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
+obj-$(CONFIG_POWER_SEQ) += power_seq.o
diff --git a/drivers/power/power_seq.c b/drivers/power/power_seq.c
new file mode 100644
index 0000000..1dcdbe0
--- /dev/null
+++ b/drivers/power/power_seq.c
@@ -0,0 +1,420 @@
+/*
+ * power_seq.c - A simple power sequence interpreter for platform devices
+ * and device tree.
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include <linux/power_seq.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/gpio.h>
+
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+
+struct power_seq_step {
+ /* Copy of the platform data */
+ struct platform_power_seq_step pdata;
+ /* Resolved resource */
+ struct power_seq_resource *resource;
+};
+
+struct power_seq {
+ struct device *dev;
+ unsigned int nb_steps;
+ struct power_seq_step steps[];
+};
+
+static char *res_names[POWER_SEQ_MAX] = {
+ [POWER_SEQ_DELAY] = "delay",
+ [POWER_SEQ_REGULATOR] = "regulator",
+ [POWER_SEQ_GPIO] = "gpio",
+ [POWER_SEQ_PWM] = "pwm",
+};
+
+static int power_seq_step_run(struct power_seq_step *step)
+{
+ struct platform_power_seq_step *pdata = &step->pdata;
+ int err = 0;
+
+ switch (pdata->type) {
+ case POWER_SEQ_DELAY:
+ usleep_range(pdata->delay.delay_us,
+ pdata->delay.delay_us + 1000);
+ break;
+#ifdef CONFIG_REGULATOR
+ case POWER_SEQ_REGULATOR:
+ if (pdata->regulator.enable)
+ err = regulator_enable(step->resource->regulator);
+ else
+ err = regulator_disable(step->resource->regulator);
+ break;
+#endif
+#ifdef CONFIG_PWM
+ case POWER_SEQ_PWM:
+ if (pdata->gpio.enable)
+ err = pwm_enable(step->resource->pwm);
+ else
+ pwm_disable(step->resource->pwm);
+ break;
+#endif
+#ifdef CONFIG_GPIOLIB
+ case POWER_SEQ_GPIO:
+ gpio_set_value_cansleep(pdata->gpio.gpio, pdata->gpio.enable);
+ break;
+#endif
+ /*
+ * should never happen unless the sequence includes a step which
+ * type does not have support compiled in
+ */
+ default:
+ return -EINVAL;
+ }
+
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
+int power_seq_run(struct power_seq *seq)
+{
+ struct device *dev = seq->dev;
+ int err, cpt;
+
+ if (!seq)
+ return 0;
+
+ for (cpt = 0; cpt < seq->nb_steps; cpt++) {
+ err = power_seq_step_run(&seq->steps[cpt]);
+ if (err) {
+ dev_err(dev, "error %d while running power sequence!\n",
+ err);
+ return err;
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(power_seq_run);
+
+#ifdef CONFIG_OF
+static int of_power_seq_enable_properties(struct device *dev,
+ struct device_node *node,
+ bool *enable)
+{
+ if (of_find_property(node, "enable", NULL)) {
+ *enable = true;
+ } else if (of_find_property(node, "disable", NULL)) {
+ *enable = false;
+ } else {
+ dev_err(dev, "missing enable or disable property!\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int of_parse_power_seq_step(struct device *dev, struct device_node *node,
+ struct platform_power_seq_step *step)
+{
+ struct property *res_id = NULL;
+ int i, err;
+
+ /* Try to find a meaningful name property */
+ for (i = 0; i < POWER_SEQ_MAX; i++) {
+ struct property *mprop;
+
+ mprop = of_find_property(node, res_names[i], NULL);
+ if (mprop) {
+ if (res_id) {
+ dev_err(dev,
+ "more than one resource in step!\n");
+ return -EINVAL;
+ }
+ step->type = i;
+ res_id = mprop;
+ }
+ }
+ if (!res_id) {
+ dev_err(dev, "missing resource property for power seq step!\n");
+ return -EINVAL;
+ }
+
+ /* Now parse resource-specific properties */
+ switch (step->type) {
+ case POWER_SEQ_DELAY:
+ err = of_property_read_u32(node, res_id->name,
+ &step->delay.delay_us);
+ if (err)
+ goto err_read_property;
+
+ break;
+
+ case POWER_SEQ_REGULATOR:
+ err = of_property_read_string(node, res_id->name,
+ &step->regulator.regulator);
+ if (err)
+ goto err_read_property;
+
+ err = of_power_seq_enable_properties(dev, node,
+ &step->regulator.enable);
+ if (err)
+ return err;
+
+ break;
+
+ case POWER_SEQ_PWM:
+ err = of_property_read_string(node, res_id->name,
+ &step->pwm.pwm);
+ if (err)
+ goto err_read_property;
+
+ err = of_power_seq_enable_properties(dev, node,
+ &step->pwm.enable);
+ if (err)
+ return err;
+
+ break;
+
+#ifdef CONFIG_OF_GPIO
+ case POWER_SEQ_GPIO:
+ {
+ char prop_name[32]; /* max size of property name */
+ const char *gpio_name;
+ int gpio;
+
+ err = of_property_read_string(node, res_id->name, &gpio_name);
+ if (err)
+ goto err_read_property;
+
+ /* Resolve the GPIO name */
+ snprintf(prop_name, 32, "%s-gpio", gpio_name);
+ gpio = of_get_named_gpio(dev->of_node, prop_name, 0);
+ if (gpio < 0) {
+ dev_err(dev, "cannot resolve gpio \"%s\"\n", gpio_name);
+ return gpio;
+ }
+ step->gpio.gpio = gpio;
+
+ err = of_power_seq_enable_properties(dev, node,
+ &step->gpio.enable);
+ if (err)
+ return err;
+
+ break;
+ }
+#endif /* CONFIG_OF_GPIO */
+
+ default:
+ dev_err(dev, "unhandled power sequence step type %s\n",
+ res_names[step->type]);
+ return -EINVAL;
+ }
+
+ return 0;
+
+err_read_property:
+ dev_err(dev, "cannot read %s property!", res_names[step->type]);
+ return -EINVAL;
+}
+
+struct platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node)
+{
+ struct device_node *child = NULL;
+ struct platform_power_seq *pseq;
+ int nb_steps = 0, size;
+ int err;
+
+ if (!node)
+ return ERR_PTR(-EINVAL);
+
+ nb_steps = of_get_child_count(node);
+ size = sizeof(pseq) + sizeof(struct platform_power_seq_step) * nb_steps;
+ pseq = devm_kzalloc(dev, size, GFP_KERNEL);
+ if (!pseq)
+ return ERR_PTR(-ENOMEM);
+ pseq->nb_steps = nb_steps;
+
+ for_each_child_of_node(node, child) {
+ unsigned int pos;
+
+ /* Check that the name's format is correct and within bounds */
+ if (strncmp("step", child->name, 4)) {
+ err = -EINVAL;
+ goto parse_error;
+ }
+
+ err = kstrtoint(child->name + 4, 10, &pos);
+ if (err < 0)
+ goto parse_error;
+
+ if (pos >= nb_steps || pseq->steps[pos].type != 0) {
+ err = -EINVAL;
+ goto parse_error;
+ }
+
+ err = of_parse_power_seq_step(dev, child, &pseq->steps[pos]);
+ if (err)
+ return ERR_PTR(err);
+ }
+
+ return pseq;
+
+parse_error:
+ dev_err(dev, "invalid power step name %s!\n", child->name);
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(of_parse_power_seq);
+#endif /* CONFIG_OF */
+
+static
+struct power_seq_resource *power_seq_find_resource(struct list_head *ress,
+ struct platform_power_seq_step *step)
+{
+ struct power_seq_resource *res;
+
+ list_for_each_entry(res, ress, list) {
+ struct platform_power_seq_step *pdata = res->pdata;
+
+ if (pdata->type != step->type)
+ continue;
+
+ switch (pdata->type) {
+ case POWER_SEQ_REGULATOR:
+ if (!strcmp(pdata->regulator.regulator,
+ step->regulator.regulator))
+ return res;
+ break;
+ case POWER_SEQ_PWM:
+ if (!strcmp(pdata->pwm.pwm, step->pwm.pwm))
+ return res;
+ break;
+ case POWER_SEQ_GPIO:
+ if (pdata->gpio.gpio = step->gpio.gpio)
+ return res;
+ break;
+ default:
+ break;
+ }
+ }
+
+ return NULL;
+}
+
+static int power_seq_allocate_resource(struct device *dev,
+ struct power_seq_resource *res)
+{
+ struct platform_power_seq_step *pdata = res->pdata;
+ int err;
+
+ switch (pdata->type) {
+ case POWER_SEQ_DELAY:
+ break;
+#ifdef CONFIG_REGULATOR
+ case POWER_SEQ_REGULATOR:
+ res->regulator = devm_regulator_get(dev,
+ pdata->regulator.regulator);
+ if (IS_ERR(res->regulator)) {
+ dev_err(dev, "cannot get regulator \"%s\"\n",
+ pdata->regulator.regulator);
+ return PTR_ERR(res->regulator);
+ }
+ break;
+#endif
+#ifdef CONFIG_PWM
+ case POWER_SEQ_PWM:
+ res->pwm = devm_pwm_get(dev, pdata->pwm.pwm);
+ if (IS_ERR(res->pwm)) {
+ dev_err(dev, "cannot get pwm \"%s\"\n", pdata->pwm.pwm);
+ return PTR_ERR(res->pwm);
+ }
+ break;
+#endif
+#ifdef CONFIG_GPIOLIB
+ case POWER_SEQ_GPIO:
+ err = devm_gpio_request_one(dev, pdata->gpio.gpio,
+ GPIOF_OUT_INIT_HIGH, "backlight_gpio");
+ if (err) {
+ dev_err(dev, "cannot get gpio %d\n", pdata->gpio.gpio);
+ return err;
+ }
+ break;
+#endif
+ default:
+ dev_err(dev, "invalid resource type %d\n", pdata->type);
+ return -EINVAL;
+ break;
+ }
+
+ return 0;
+}
+
+struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
+ struct platform_power_seq *pseq)
+{
+ struct power_seq *seq;
+ struct power_seq_resource *res;
+ int cpt, err;
+
+ seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(struct power_seq_step) *
+ pseq->nb_steps, GFP_KERNEL);
+ if (!seq)
+ return ERR_PTR(-ENOMEM);
+
+ seq->dev = dev;
+ seq->nb_steps = pseq->nb_steps;
+
+ for (cpt = 0; cpt < seq->nb_steps; cpt++) {
+ struct platform_power_seq_step *pstep = &pseq->steps[cpt];
+ struct power_seq_step *step = &seq->steps[cpt];
+
+ memcpy(&step->pdata, pstep, sizeof(step->pdata));
+
+ /* Delay steps have no resource */
+ if (pstep->type = POWER_SEQ_DELAY)
+ continue;
+
+ /* create resource node if not referenced already */
+ res = power_seq_find_resource(ress, pstep);
+ if (!res) {
+ res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return ERR_PTR(-ENOMEM);
+ res->pdata = &step->pdata;
+
+ err = power_seq_allocate_resource(dev, res);
+ if (err < 0)
+ return ERR_PTR(err);
+
+ list_add(&res->list, ress);
+ }
+ step->resource = res;
+ }
+
+ return seq;
+}
+EXPORT_SYMBOL_GPL(power_seq_build);
+
+MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
+MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
new file mode 100644
index 0000000..d9dd277
--- /dev/null
+++ b/include/linux/power_seq.h
@@ -0,0 +1,142 @@
+/*
+ * power_seq.h
+ *
+ * Simple interpreter for defining power sequences as platform data or device
+ * tree properties.
+ *
+ * Power sequences are designed to replace the callbacks typically used in
+ * board-specific files that implement board-specific power sequences of devices
+ * such as backlights. A power sequence is an array of resources (which can a
+ * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
+ * disable) and optional pre and post step delays. By having them interpreted
+ * instead of arbitrarily executed, it is possible to describe these in the
+ * device tree and thus remove board-specific code from the kernel.
+ *
+ * Author: Alexandre Courbot <acourbot@nvidia.com>
+ *
+ * Copyright (c) 2012 NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#ifndef __LINUX_POWER_SEQ_H
+#define __LINUX_POWER_SEQ_H
+
+#include <linux/types.h>
+
+struct device;
+struct regulator;
+struct pwm_device;
+struct device_node;
+
+/**
+ * The different kinds of resources that can be controlled during the sequences.
+ */
+enum power_seq_res_type {
+ POWER_SEQ_DELAY,
+ POWER_SEQ_REGULATOR,
+ POWER_SEQ_PWM,
+ POWER_SEQ_GPIO,
+ POWER_SEQ_MAX,
+};
+
+struct platform_power_seq_delay_step {
+ unsigned int delay_us;
+};
+
+struct platform_power_seq_regulator_step {
+ const char *regulator;
+ bool enable;
+};
+
+struct platform_power_seq_pwm_step {
+ const char *pwm;
+ bool enable;
+};
+
+struct platform_power_seq_gpio_step {
+ int gpio;
+ bool enable;
+};
+
+/**
+ * Platform definition of power sequences. A sequence is an array of these,
+ * terminated by a STOP instance.
+ */
+struct platform_power_seq_step {
+ enum power_seq_res_type type;
+ union {
+ struct platform_power_seq_delay_step delay;
+ struct platform_power_seq_regulator_step regulator;
+ struct platform_power_seq_pwm_step pwm;
+ struct platform_power_seq_gpio_step gpio;
+ };
+};
+
+struct platform_power_seq {
+ unsigned int nb_steps;
+ struct platform_power_seq_step steps[];
+};
+
+/**
+ * We maintain a list of these to monitor which resources have already
+ * been met and allocated while building the sequences.
+ */
+struct power_seq_resource {
+ /* relevant for resolving the resource and knowing its type */
+ struct platform_power_seq_step *pdata;
+ /* resolved resource (if any) */
+ union {
+ struct regulator *regulator;
+ struct pwm_device *pwm;
+ };
+ struct list_head list;
+};
+
+struct power_seq_resource;
+struct power_seq;
+
+#ifdef CONFIG_OF
+/**
+ * Build a platform data sequence from a device tree node. Memory for the
+ * platform sequence is allocated using devm_kzalloc on dev and can be freed
+ * by devm_kfree after power_seq_build is called.
+ */
+struct platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node);
+#else
+struct platform_power_seq *of_parse_power_seq(struct device *dev,
+ struct device_node *node)
+{
+ return ERR_PTR(-EINVAL);
+}
+#endif
+
+/**
+ * Build a runnable power sequence from platform data, and add the resources
+ * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
+ * on dev.
+ * @dev device that will use the power sequence. All resources will be
+ * devm-allocated against it.
+ * @ress list that holds the power_seq_resources already used by this device.
+ * Resources newly met in the sequence will be added to it.
+ * @pseq power sequence in platform format.
+ */
+struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
+ struct platform_power_seq *pseq);
+
+/**
+ * Run the given power sequence. Returns 0 on success, error code in case of
+ * failure.
+ */
+int power_seq_run(struct power_seq *seq);
+
+#endif
--
1.7.11.4
^ permalink raw reply related
* [PATCH v4 2/3] pwm_backlight: use power sequences
From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw)
To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann
Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
devicetree-discuss, linux-doc, Alexandre Courbot
In-Reply-To: <1345097337-24170-1-git-send-email-acourbot@nvidia.com>
Make use of the power sequences specified in the device tree or platform
data to control how the backlight is powered on and off.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
.../bindings/video/backlight/pwm-backlight.txt | 62 ++++++-
drivers/video/backlight/Kconfig | 1 +
drivers/video/backlight/pwm_bl.c | 192 +++++++++++++++------
include/linux/pwm_backlight.h | 16 +-
4 files changed, 216 insertions(+), 55 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..51e814d 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -2,7 +2,6 @@ pwm-backlight bindings
Required properties:
- compatible: "pwm-backlight"
- - pwms: OF device-tree PWM specification (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,19 +9,72 @@ 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)
+ - pwms: OF device-tree PWM specification (see PWM binding[0]). Exactly one PWM
+ must be specified
Optional properties:
- - pwm-names: a list of names for the PWM devices specified in the
- "pwms" property (see PWM binding[0])
+ - *-supply: regulators used within a power sequence
+ - *-gpio: GPIOs used within a power sequence
+ - pwm-names: name for the PWM device specified in the "pwms" property (see PWM
+ binding[0]). Necessary if power sequences are used
+ - power-on-sequence: Power sequence (see Power sequences[1]) used to bring the
+ backlight on. This sequence must reference the PWM specified in the pwms
+ property by its name. It can also reference other resources supported by
+ the power sequences mechanism
+ - power-off-sequence: Power sequence (see Power sequences[1]) used to bring
+ the backlight off. This sequence must reference the PWM specified in the
+ pwms property by its name. It can also reference other resources supported
+ by the power sequences mechanism
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
+[1]: Documentation/devicetree/bindings/power_seq/power_seq.txt
Example:
backlight {
compatible = "pwm-backlight";
- pwms = <&pwm 0 5000000>;
-
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+
+ /* resources used by the sequences */
+ pwms = <&pwm 2 5000000>;
+ pwm-names = "backlight";
+ power-supply = <&backlight_reg>;
+ enable-gpio = <&gpio 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ pwm = "backlight";
+ enable;
+ };
+ step3 {
+ gpio = "enable";
+ enable;
+ };
+ };
+
+ power-off-sequence {
+ step0 {
+ gpio = "enable";
+ disable;
+ };
+ step1 {
+ pwm = "backlight";
+ disable;
+ };
+ step2 {
+ delay = <10000>;
+ };
+ step3 {
+ regulator = "power";
+ disable;
+ };
+ };
};
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index cf28276..6fb8aa3 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -246,6 +246,7 @@ config BACKLIGHT_CARILLO_RANCH
config BACKLIGHT_PWM
tristate "Generic PWM based Backlight Driver"
depends on PWM
+ select POWER_SEQ
help
If you have a LCD backlight adjustable by PWM, say Y to enable
this driver.
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 995f016..f456a71 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -27,6 +27,13 @@ struct pwm_bl_data {
unsigned int period;
unsigned int lth_brightness;
unsigned int *levels;
+ bool enabled;
+ bool use_power_seqs;
+ struct list_head resources;
+ struct power_seq *power_on_seq;
+ struct power_seq *power_off_seq;
+
+ /* Legacy callbacks */
int (*notify)(struct device *,
int brightness);
void (*notify_after)(struct device *,
@@ -35,6 +42,49 @@ 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;
+
+ /* Legacy framework? */
+ if (!pb->use_power_seqs) {
+ pwm_config(pb->pwm, 0, pb->period);
+ pwm_disable(pb->pwm);
+ return;
+ }
+
+ ret = power_seq_run(pb->power_on_seq);
+ if (ret < 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;
+
+ /* Legacy framework? */
+ if (!pb->use_power_seqs) {
+ pwm_enable(pb->pwm);
+ return;
+ }
+
+ ret = power_seq_run(pb->power_off_seq);
+ if (ret < 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 +101,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;
@@ -66,7 +115,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);
}
if (pb->notify_after)
@@ -98,7 +147,7 @@ static const struct backlight_ops pwm_backlight_ops = {
static int pwm_backlight_parse_dt(struct device *dev,
struct platform_pwm_backlight_data *data)
{
- struct device_node *node = dev->of_node;
+ struct device_node *node = dev->of_node, *pseq_node;
struct property *prop;
int length;
u32 value;
@@ -145,11 +194,18 @@ 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.
- */
+ /* convert power sequences to platform data, if any */
+ pseq_node = of_find_node_by_name(node, "power-on-sequence");
+ if (pseq_node)
+ data->power_on_seq = of_parse_power_seq(dev, pseq_node);
+ if (IS_ERR(data->power_on_seq))
+ return PTR_ERR(data->power_on_seq);
+
+ pseq_node = of_find_node_by_name(node, "power-off-sequence");
+ if (pseq_node)
+ data->power_off_seq = of_parse_power_seq(dev, pseq_node);
+ if (IS_ERR(data->power_off_seq))
+ return PTR_ERR(data->power_off_seq);
return 0;
}
@@ -172,33 +228,98 @@ 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;
}
+ /* using legacy interface? */
+ if (!data->power_on_seq && !data->power_off_seq) {
+ pb->pwm = devm_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");
+ return PTR_ERR(pb->pwm);
+ }
+ }
+
+ /*
+ * 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);
+ } 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);
+ }
+
+ /* we must have exactly one PWM for this driver */
+ list_for_each_entry(res, &pb->resources, list) {
+ if (res->pdata->type != POWER_SEQ_PWM)
+ continue;
+ if (pb->pwm) {
+ dev_err(&pdev->dev, "more than one PWM used\n");
+ return -EINVAL;
+ }
+ /* keep the pwm at hand */
+ pb->pwm = res->pwm;
+ }
+
+ pb->use_power_seqs = true;
+ }
+
if (data->init) {
ret = data->init(&pdev->dev);
if (ret < 0)
- return ret;
+ goto err;
}
- 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->levels) {
@@ -213,28 +334,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
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,18 +345,17 @@ 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);
return ret;
@@ -269,9 +367,8 @@ 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);
+
if (pb->exit)
pb->exit(&pdev->dev);
return 0;
@@ -285,8 +382,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..5bc5e39 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -5,14 +5,26 @@
#define __LINUX_PWM_BACKLIGHT_H
#include <linux/backlight.h>
+#include <linux/power_seq.h>
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;
+ /*
+ * New interface using power sequences
+ */
+ struct platform_power_seq *power_on_seq;
+ struct platform_power_seq *power_off_seq;
+ /*
+ * Legacy interface - use power sequences instead!
+ *
+ * pwm_id and pwm_period_ns need only be specified
+ * if get_pwm(dev, NULL) would 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.4
^ permalink raw reply related
* [PATCH v4 3/3] tegra: add pwm backlight device tree nodes
From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw)
To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann
Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
devicetree-discuss, linux-doc, Alexandre Courbot
In-Reply-To: <1345097337-24170-1-git-send-email-acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
arch/arm/boot/dts/tegra20-ventana.dts | 58 +++++++++++++++++++++++++++++++++++
arch/arm/boot/dts/tegra20.dtsi | 2 +-
2 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index be90544..b253697 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -317,6 +317,64 @@
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>;
+
+ /* resources used by the power sequences */
+ pwms = <&pwm 2 5000000>;
+ pwm-names = "backlight";
+ power-supply = <&backlight_reg>;
+ enable-gpio = <&gpio 28 0>;
+
+ power-on-sequence {
+ step0 {
+ regulator = "power";
+ enable;
+ };
+ step1 {
+ delay = <10000>;
+ };
+ step2 {
+ pwm = "backlight";
+ enable;
+ };
+ step3 {
+ gpio = "enable";
+ enable;
+ };
+ };
+ power-off-sequence {
+ step0 {
+ gpio = "enable";
+ disable;
+ };
+ step1 {
+ pwm = "backlight";
+ disable;
+ };
+ step2 {
+ delay = <10000>;
+ };
+ step3 {
+ regulator = "power";
+ disable;
+ };
+ };
+ };
+
+ 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.4
^ permalink raw reply related
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-08-16 7:42 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann,
Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
devicetree-discuss, linux-doc
In-Reply-To: <1345097337-24170-2-git-send-email-acourbot@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 31667 bytes --]
On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
>
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> .../devicetree/bindings/power_seq/power_seq.txt | 101 +++++
> Documentation/power/power_seq.txt | 129 +++++++
> drivers/power/Kconfig | 3 +
> drivers/power/Makefile | 1 +
> drivers/power/power_seq.c | 420 +++++++++++++++++++++
> include/linux/power_seq.h | 142 +++++++
> 6 files changed, 796 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
> create mode 100644 Documentation/power/power_seq.txt
> create mode 100644 drivers/power/power_seq.c
> create mode 100644 include/linux/power_seq.h
>
> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> new file mode 100644
> index 0000000..749c6e4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> @@ -0,0 +1,101 @@
> +Specifying Power Sequences in the Device Tree
> +=============================================
> +In the device tree, power sequences are specified as sub-nodes of the device
> +node and reference resources declared by that device.
> +
> +For an introduction about runtime interpreted power sequences, see
> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
> +
> +Power Sequences Structure
> +-------------------------
> +Power sequences are sub-nodes that are named such as the device driver can find
"named such that"?
> +them. The driver's documentation should list the sequence names it recognizes.
> +
> +Inside a power sequence node are sub-nodes that describe the different steps
> +of the sequence. Each step must be named sequentially, with the first step
> +named step0, the second step1, etc. Failure to follow this rule will result in a
> +parsing error.
> +
> +Power Sequences Steps
> +---------------------
> +Every step of a sequence describes an action to be performed on a resource. It
> +generally includes a property named after the resource type, and which value
> +references the resource to be used. Depending on the resource type, additional
> +properties can be defined to control the action to be performed.
> +
> +Currently supported resource types are:
I think the "currently" can be dropped.
> +- "delay", which should simply contain a delay in microseconds to wait before
> + going on with the rest of the sequence. It takes no additional property.
> +- "regulator" contains the name of a regulator to be acquired using
> + regulator_get(). An additional property, either "enable" or "disable", must be
> + present to control whether the regulator should be enabled or disabled during
> + that step.
> +- "pwm" is set to the name of a PWM acquired using pwm_get(). As with regulator,
> + an additional "enable" or "disable" property is required.
> +- "gpio" contains the name of a GPIO to enable or disable using the same
> + additional property as regulator or pwm. The gpio is resolved by appending
> + "-gpio" to the given name and looking for a device property with a GPIO
> + phandle.
I find this part slightly confusing. It doesn't seem quite obvious from
the text that "delay", "regulator", "pwm" and "gpio" are also actual
property names.
It might also be worth saying that the "enable" and "disable" properties
are boolean in nature and don't need a value.
Then again this becomes much clearer with the example below, so maybe
I'm just being too picky here.
> +
> +Example
> +-------
> +Here are example sequences declared within a backlight device that use all the
> +supported resources types:
> +
> + backlight {
> + compatible = "pwm-backlight";
> + ...
> +
> + /* resources used by the sequences */
> + pwms = <&pwm 2 5000000>;
> + pwm-names = "backlight";
> + power-supply = <&backlight_reg>;
> + enable-gpio = <&gpio 28 0>;
> +
> + power-on-sequence {
> + step0 {
> + regulator = "power";
> + enable;
> + };
> + step1 {
> + delay = <10000>;
> + };
> + step2 {
> + pwm = "backlight";
> + enable;
> + };
> + step3 {
> + gpio = "enable";
> + enable;
> + };
> + };
> +
> + power-off-sequence {
> + step0 {
> + gpio = "enable";
> + disable;
> + };
> + step1 {
> + pwm = "backlight";
> + disable;
> + };
> + step2 {
> + delay = <10000>;
> + };
> + step3 {
> + regulator = "power";
> + disable;
> + };
> + };
> + };
> +
> +The first part lists the PWM, regulator, and GPIO resources used by the
> +sequences. These resources will be requested on behalf of the backlight device
> +when the sequences are built and are declared according to their own framework
> +in a way that makes them accessible by name.
> +
> +After the resources declaration, two sequences follow for powering the backlight
> +on and off. Their names are specified by the pwm-backlight driver. Every step
> +uses one of the "delay", "regulator", "pwm" or "gpio" properties to reference a
> +previously-declared resource. Additional "enable" or "disable" properties are
> +also used as needed.
> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
> new file mode 100644
> index 0000000..3ab4f93
> --- /dev/null
> +++ b/Documentation/power/power_seq.txt
> @@ -0,0 +1,129 @@
> +Runtime Interpreted Power Sequences
> +===================================
> +
> +Problem
> +-------
> +One very common board-dependent code is the out-of-driver code that is used to
> +turn a device on or off. For instance, SoC boards very commonly use a GPIO
> +(abstracted to a regulator or not) to control the power supply of a backlight,
> +disabling it when the backlight is not used in order to save power. The GPIO
> +that should be used, however, as well as the exact power sequence that may
> +also involve other resources, is board-dependent and thus unknown of the driver.
"unknown to the driver"?
> +
> +This was previously addressed by having hooks in the device's platform data that
> +are called whenever the state of the device might reflect a power change. This
> +approach, however, introduces board-dependant code into the kernel and is not
> +compatible with the device tree.
> +
> +The Runtime Interpreted Power Sequences (or power sequences for short) aims at
"... Sequences [...] aim"?
> +turning this code into platform data or device tree nodes. Power sequences are
> +described using a simple format and run by a simple interpreter whenever needed.
> +This allows to remove the callback mechanism and makes the kernel less
> +board-dependant.
> +
> +What are Power Sequences?
> +-------------------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources so far are:
Again, I don't see a need for "so far" here. It implies that new types
may be added. While it is quite possible and maybe even likely to happen
the new types can be added to the documentation at the same time.
> +- delay (just wait for the delay given in microseconds)
> +- GPIO (enable or disable)
> +- regulator (enable or disable)
> +- PWM (enable or disable)
> +
> +Every step designates a resource type and parameters that are relevant to it.
> +For instance, GPIO and PWMs can be enabled or disabled.
> +
> +When a power sequence is run, each of its step is executed sequentially until
"each of its steps"
> +one step fails or the end of the sequence is reached.
> +
> +Power sequences can be declared as platform data or in the device tree.
> +
> +Platform Data Format
> +--------------------
> +All relevant data structures for declaring power sequences are located in
> +include/linux/power_seq.h.
> +
> +The platform data is a static instance of simple array of
> +platform_power_seq_step instances, each
> +instance describing a step. The type as well as one of id or gpio members
> +(depending on the type) must be specified. The last step must be of type
> +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
> +identified by number. For example, the following sequence will turn on the
> +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:
> +
> +static struct platform_power_seq power_on_seq = {
> + .nb_steps = 3,
I think num_steps would be more canonical.
> + .steps = {
> + {
> + .type = POWER_SEQ_REGULATOR,
> + .regulator.regulator = "power",
> + .regulator.enable = 1,
> + },
This may be easier to read as:
.type = POWER_SEQ_REGULATOR,
.regulator {
.regulator = "power",
.enable = 1,
}
Also, why not rename the .regulator field to .name? That describes
better what it is and removes the redundancy of having the structure
named regulator and a regulator field within.
> + {
> + .type = POWER_SEQ_DELAY,
> + .delay.delay_us = 10000,
> + },
> + {
> + .type = POWER_SEQ_GPIO,
> + .gpio.gpio = 110,
> + .gpio.enable = 1,
> + },
Same comments as for the regulator step above. Also, since enable is a
boolean field, maybe you should use true and false instead.
> + },
> +};
> +
> +Device Tree
> +-----------
> +Power sequences can also be encoded as device tree nodes. The following
> +properties and nodes are equivalent to the platform data defined previously:
> +
> + power-supply = <&power_reg>;
> + switch-gpio = <&gpio 110 0>;
> +
> + power-on-sequence {
> + step0 {
> + regulator = "power";
> + enable;
> + };
> + step1 {
> + delay = <10000>;
> + };
> + step2 {
> + gpio = "switch";
> + enable;
> + };
> + };
> +
> +See Documentation/devicetree/bindings/power_seq/power_seq.txt for the complete
> +syntax of the bindings.
> +
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function builds a power sequence from the
> +platform data. It also takes care of resolving and allocating the resources
> +referenced by the sequence if needed:
> +
> + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> + struct platform_power_seq *pseq);
> +
> +The 'dev' argument is the device in the name of which the resources are to be
> +allocated.
> +
> +The 'ress' argument is a list to which the resolved resources are appended. This
> +avoids allocating a resource referenced in several power sequences multiple
> +times.
> +
> +On success, the function returns a devm allocated resolved sequence that is
> +ready to be passed to power_seq_run(). In case of failure, and error code is
> +returned.
> +
> +A resolved power sequence returned by power_seq_build can be run by
> +power_run_run():
> +
> + int power_seq_run(power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.
> +
> +There is no need to explicitly free the resources used by the sequence as they
> +are devm-allocated.
I had some comments about this particular interface for creating
sequences in the last series. My point was that explicitly requiring
drivers to manage a list of already allocated resources may be too much
added complexity. Power sequences should be easy to use, and I find the
requirement for a separately managed list of resources cumbersome.
What I proposed last time was to collect all power sequences under a
common parent object, which in turn would take care of managing the
resources.
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index c1892f3..4172fe4 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -312,4 +312,7 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
> thermistor connected on BATCTRL ADC.
> endif # POWER_SUPPLY
>
> +config POWER_SEQ
> + bool
> +
> source "drivers/power/avs/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index ee58afb..828859c 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
> obj-$(CONFIG_POWER_AVS) += avs/
> obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
> +obj-$(CONFIG_POWER_SEQ) += power_seq.o
> diff --git a/drivers/power/power_seq.c b/drivers/power/power_seq.c
> new file mode 100644
> index 0000000..1dcdbe0
> --- /dev/null
> +++ b/drivers/power/power_seq.c
> @@ -0,0 +1,420 @@
> +/*
> + * power_seq.c - A simple power sequence interpreter for platform devices
> + * and device tree.
> + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#include <linux/power_seq.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/pwm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +
> +struct power_seq_step {
> + /* Copy of the platform data */
> + struct platform_power_seq_step pdata;
> + /* Resolved resource */
> + struct power_seq_resource *resource;
> +};
> +
> +struct power_seq {
> + struct device *dev;
> + unsigned int nb_steps;
> + struct power_seq_step steps[];
> +};
> +
> +static char *res_names[POWER_SEQ_MAX] = {
> + [POWER_SEQ_DELAY] = "delay",
> + [POWER_SEQ_REGULATOR] = "regulator",
> + [POWER_SEQ_GPIO] = "gpio",
> + [POWER_SEQ_PWM] = "pwm",
> +};
static const?
> +
> +static int power_seq_step_run(struct power_seq_step *step)
> +{
> + struct platform_power_seq_step *pdata = &step->pdata;
> + int err = 0;
> +
> + switch (pdata->type) {
> + case POWER_SEQ_DELAY:
> + usleep_range(pdata->delay.delay_us,
> + pdata->delay.delay_us + 1000);
> + break;
> +#ifdef CONFIG_REGULATOR
> + case POWER_SEQ_REGULATOR:
> + if (pdata->regulator.enable)
> + err = regulator_enable(step->resource->regulator);
> + else
> + err = regulator_disable(step->resource->regulator);
> + break;
> +#endif
> +#ifdef CONFIG_PWM
> + case POWER_SEQ_PWM:
> + if (pdata->gpio.enable)
> + err = pwm_enable(step->resource->pwm);
> + else
> + pwm_disable(step->resource->pwm);
> + break;
> +#endif
> +#ifdef CONFIG_GPIOLIB
> + case POWER_SEQ_GPIO:
> + gpio_set_value_cansleep(pdata->gpio.gpio, pdata->gpio.enable);
> + break;
> +#endif
> + /*
> + * should never happen unless the sequence includes a step which
> + * type does not have support compiled in
I think this should be "whose type"? I also remember commenting on the
whole #ifdef'ery here. I really don't think it is necessary. At least
for regulators I know that the functions can be used even if the
subsystem itself isn't supported. The same seems to hold for GPIO and we
can probably add something similar for PWM.
It might also be a good idea to just skip unsupported resource types
when the sequence is built, accompanied by runtime warnings that the
type is not supported.
> + */
> + default:
> + return -EINVAL;
> + }
> +
> + if (err < 0)
> + return err;
> +
> + return 0;
This can probably be collapsed to just "return err;", can't it?
> +}
> +
> +int power_seq_run(struct power_seq *seq)
> +{
> + struct device *dev = seq->dev;
> + int err, cpt;
Any reason why you call the loop variable cpt instead of something more
canonical such as i? Also it should be of type unsigned int.
> +
> + if (!seq)
> + return 0;
> +
> + for (cpt = 0; cpt < seq->nb_steps; cpt++) {
> + err = power_seq_step_run(&seq->steps[cpt]);
> + if (err) {
> + dev_err(dev, "error %d while running power sequence!\n",
> + err);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_run);
> +
> +#ifdef CONFIG_OF
> +static int of_power_seq_enable_properties(struct device *dev,
> + struct device_node *node,
> + bool *enable)
Maybe rename this to of_power_seq_parse_enable_properties() to make it
more obvious that it is actually parsing data. It's an awfully long name
for a function, though.
> +{
> + if (of_find_property(node, "enable", NULL)) {
> + *enable = true;
> + } else if (of_find_property(node, "disable", NULL)) {
> + *enable = false;
> + } else {
> + dev_err(dev, "missing enable or disable property!\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int of_parse_power_seq_step(struct device *dev, struct device_node *node,
> + struct platform_power_seq_step *step)
> +{
> + struct property *res_id = NULL;
> + int i, err;
> +
> + /* Try to find a meaningful name property */
> + for (i = 0; i < POWER_SEQ_MAX; i++) {
Maybe this should be renamed to POWER_SEQ_MAX_TYPE, POWER_SEQ_TYPE_MAX,
POWER_SEQ_NUM_TYPES or some such. I had assumed POWER_SEQ_MAX would mean
the maximum length of a power sequence.
> + struct property *mprop;
> +
> + mprop = of_find_property(node, res_names[i], NULL);
> + if (mprop) {
> + if (res_id) {
> + dev_err(dev,
> + "more than one resource in step!\n");
> + return -EINVAL;
> + }
> + step->type = i;
> + res_id = mprop;
> + }
> + }
> + if (!res_id) {
> + dev_err(dev, "missing resource property for power seq step!\n");
> + return -EINVAL;
> + }
> +
> + /* Now parse resource-specific properties */
> + switch (step->type) {
> + case POWER_SEQ_DELAY:
> + err = of_property_read_u32(node, res_id->name,
> + &step->delay.delay_us);
> + if (err)
> + goto err_read_property;
> +
> + break;
> +
> + case POWER_SEQ_REGULATOR:
> + err = of_property_read_string(node, res_id->name,
> + &step->regulator.regulator);
> + if (err)
> + goto err_read_property;
> +
> + err = of_power_seq_enable_properties(dev, node,
> + &step->regulator.enable);
> + if (err)
> + return err;
> +
> + break;
> +
> + case POWER_SEQ_PWM:
> + err = of_property_read_string(node, res_id->name,
> + &step->pwm.pwm);
> + if (err)
> + goto err_read_property;
> +
> + err = of_power_seq_enable_properties(dev, node,
> + &step->pwm.enable);
> + if (err)
> + return err;
> +
> + break;
> +
> +#ifdef CONFIG_OF_GPIO
> + case POWER_SEQ_GPIO:
> + {
> + char prop_name[32]; /* max size of property name */
> + const char *gpio_name;
> + int gpio;
> +
> + err = of_property_read_string(node, res_id->name, &gpio_name);
> + if (err)
> + goto err_read_property;
> +
> + /* Resolve the GPIO name */
> + snprintf(prop_name, 32, "%s-gpio", gpio_name);
I'm not sure if there's a limit on the length of DT property names, but
maybe using kasprintf would be a better idea here.
> + gpio = of_get_named_gpio(dev->of_node, prop_name, 0);
> + if (gpio < 0) {
> + dev_err(dev, "cannot resolve gpio \"%s\"\n", gpio_name);
> + return gpio;
> + }
> + step->gpio.gpio = gpio;
> +
> + err = of_power_seq_enable_properties(dev, node,
> + &step->gpio.enable);
> + if (err)
> + return err;
> +
> + break;
> + }
> +#endif /* CONFIG_OF_GPIO */
> +
> + default:
> + dev_err(dev, "unhandled power sequence step type %s\n",
> + res_names[step->type]);
> + return -EINVAL;
> + }
> +
> + return 0;
> +
> +err_read_property:
> + dev_err(dev, "cannot read %s property!", res_names[step->type]);
> + return -EINVAL;
> +}
Given the size of this function, I think it might become more readable
if it were split into separate parse functions for each type of
resource. It will also allow you to get rid of this #ifdef within the
function.
> +
> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
> + struct device_node *node)
> +{
> + struct device_node *child = NULL;
> + struct platform_power_seq *pseq;
> + int nb_steps = 0, size;
> + int err;
> +
> + if (!node)
> + return ERR_PTR(-EINVAL);
> +
> + nb_steps = of_get_child_count(node);
> + size = sizeof(pseq) + sizeof(struct platform_power_seq_step) * nb_steps;
Shouldn't the first term be sizeof(*pseq)?
> + pseq = devm_kzalloc(dev, size, GFP_KERNEL);
> + if (!pseq)
> + return ERR_PTR(-ENOMEM);
> + pseq->nb_steps = nb_steps;
> +
> + for_each_child_of_node(node, child) {
> + unsigned int pos;
> +
> + /* Check that the name's format is correct and within bounds */
> + if (strncmp("step", child->name, 4)) {
> + err = -EINVAL;
> + goto parse_error;
> + }
> +
> + err = kstrtoint(child->name + 4, 10, &pos);
> + if (err < 0)
> + goto parse_error;
kstrtouint()? Also this is somewhat ugly. Perhaps adding #address-cells
and #size-cells properties isn't that bad after all.
> +
> + if (pos >= nb_steps || pseq->steps[pos].type != 0) {
> + err = -EINVAL;
> + goto parse_error;
> + }
> +
> + err = of_parse_power_seq_step(dev, child, &pseq->steps[pos]);
> + if (err)
> + return ERR_PTR(err);
> + }
> +
> + return pseq;
> +
> +parse_error:
> + dev_err(dev, "invalid power step name %s!\n", child->name);
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(of_parse_power_seq);
> +#endif /* CONFIG_OF */
> +
> +static
> +struct power_seq_resource *power_seq_find_resource(struct list_head *ress,
> + struct platform_power_seq_step *step)
I think it is customary to put the return value type on the same line as
the static modifier.
> +{
> + struct power_seq_resource *res;
> +
> + list_for_each_entry(res, ress, list) {
> + struct platform_power_seq_step *pdata = res->pdata;
> +
> + if (pdata->type != step->type)
> + continue;
> +
> + switch (pdata->type) {
> + case POWER_SEQ_REGULATOR:
> + if (!strcmp(pdata->regulator.regulator,
> + step->regulator.regulator))
> + return res;
> + break;
> + case POWER_SEQ_PWM:
> + if (!strcmp(pdata->pwm.pwm, step->pwm.pwm))
> + return res;
> + break;
> + case POWER_SEQ_GPIO:
> + if (pdata->gpio.gpio == step->gpio.gpio)
> + return res;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static int power_seq_allocate_resource(struct device *dev,
> + struct power_seq_resource *res)
> +{
> + struct platform_power_seq_step *pdata = res->pdata;
> + int err;
> +
> + switch (pdata->type) {
> + case POWER_SEQ_DELAY:
> + break;
> +#ifdef CONFIG_REGULATOR
> + case POWER_SEQ_REGULATOR:
> + res->regulator = devm_regulator_get(dev,
> + pdata->regulator.regulator);
> + if (IS_ERR(res->regulator)) {
> + dev_err(dev, "cannot get regulator \"%s\"\n",
> + pdata->regulator.regulator);
> + return PTR_ERR(res->regulator);
> + }
> + break;
> +#endif
> +#ifdef CONFIG_PWM
> + case POWER_SEQ_PWM:
> + res->pwm = devm_pwm_get(dev, pdata->pwm.pwm);
> + if (IS_ERR(res->pwm)) {
> + dev_err(dev, "cannot get pwm \"%s\"\n", pdata->pwm.pwm);
> + return PTR_ERR(res->pwm);
> + }
> + break;
> +#endif
> +#ifdef CONFIG_GPIOLIB
> + case POWER_SEQ_GPIO:
> + err = devm_gpio_request_one(dev, pdata->gpio.gpio,
> + GPIOF_OUT_INIT_HIGH, "backlight_gpio");
> + if (err) {
> + dev_err(dev, "cannot get gpio %d\n", pdata->gpio.gpio);
> + return err;
> + }
> + break;
> +#endif
> + default:
> + dev_err(dev, "invalid resource type %d\n", pdata->type);
> + return -EINVAL;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> + struct platform_power_seq *pseq)
> +{
> + struct power_seq *seq;
> + struct power_seq_resource *res;
> + int cpt, err;
> +
> + seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(struct power_seq_step) *
> + pseq->nb_steps, GFP_KERNEL);
> + if (!seq)
> + return ERR_PTR(-ENOMEM);
> +
> + seq->dev = dev;
> + seq->nb_steps = pseq->nb_steps;
> +
> + for (cpt = 0; cpt < seq->nb_steps; cpt++) {
> + struct platform_power_seq_step *pstep = &pseq->steps[cpt];
> + struct power_seq_step *step = &seq->steps[cpt];
> +
> + memcpy(&step->pdata, pstep, sizeof(step->pdata));
> +
> + /* Delay steps have no resource */
> + if (pstep->type == POWER_SEQ_DELAY)
> + continue;
> +
> + /* create resource node if not referenced already */
> + res = power_seq_find_resource(ress, pstep);
> + if (!res) {
> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return ERR_PTR(-ENOMEM);
> + res->pdata = &step->pdata;
> +
> + err = power_seq_allocate_resource(dev, res);
> + if (err < 0)
> + return ERR_PTR(err);
> +
> + list_add(&res->list, ress);
> + }
> + step->resource = res;
> + }
> +
> + return seq;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_build);
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
> +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
> new file mode 100644
> index 0000000..d9dd277
> --- /dev/null
> +++ b/include/linux/power_seq.h
> @@ -0,0 +1,142 @@
> +/*
> + * power_seq.h
> + *
> + * Simple interpreter for defining power sequences as platform data or device
> + * tree properties.
> + *
> + * Power sequences are designed to replace the callbacks typically used in
> + * board-specific files that implement board-specific power sequences of devices
> + * such as backlights. A power sequence is an array of resources (which can a
> + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
> + * disable) and optional pre and post step delays. By having them interpreted
> + * instead of arbitrarily executed, it is possible to describe these in the
> + * device tree and thus remove board-specific code from the kernel.
> + *
> + * Author: Alexandre Courbot <acourbot@nvidia.com>
> + *
> + * Copyright (c) 2012 NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#ifndef __LINUX_POWER_SEQ_H
> +#define __LINUX_POWER_SEQ_H
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct regulator;
> +struct pwm_device;
> +struct device_node;
> +
> +/**
> + * The different kinds of resources that can be controlled during the sequences.
> + */
> +enum power_seq_res_type {
> + POWER_SEQ_DELAY,
> + POWER_SEQ_REGULATOR,
> + POWER_SEQ_PWM,
> + POWER_SEQ_GPIO,
> + POWER_SEQ_MAX,
> +};
> +
> +struct platform_power_seq_delay_step {
> + unsigned int delay_us;
> +};
> +
> +struct platform_power_seq_regulator_step {
> + const char *regulator;
> + bool enable;
> +};
> +
> +struct platform_power_seq_pwm_step {
> + const char *pwm;
> + bool enable;
> +};
> +
> +struct platform_power_seq_gpio_step {
> + int gpio;
> + bool enable;
> +};
> +
> +/**
> + * Platform definition of power sequences. A sequence is an array of these,
> + * terminated by a STOP instance.
> + */
> +struct platform_power_seq_step {
> + enum power_seq_res_type type;
> + union {
> + struct platform_power_seq_delay_step delay;
> + struct platform_power_seq_regulator_step regulator;
> + struct platform_power_seq_pwm_step pwm;
> + struct platform_power_seq_gpio_step gpio;
> + };
> +};
> +
> +struct platform_power_seq {
> + unsigned int nb_steps;
> + struct platform_power_seq_step steps[];
> +};
> +
> +/**
> + * We maintain a list of these to monitor which resources have already
> + * been met and allocated while building the sequences.
> + */
> +struct power_seq_resource {
> + /* relevant for resolving the resource and knowing its type */
> + struct platform_power_seq_step *pdata;
> + /* resolved resource (if any) */
> + union {
> + struct regulator *regulator;
> + struct pwm_device *pwm;
> + };
> + struct list_head list;
> +};
> +
> +struct power_seq_resource;
> +struct power_seq;
> +
> +#ifdef CONFIG_OF
> +/**
> + * Build a platform data sequence from a device tree node. Memory for the
> + * platform sequence is allocated using devm_kzalloc on dev and can be freed
> + * by devm_kfree after power_seq_build is called.
> + */
> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
> + struct device_node *node);
> +#else
> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
> + struct device_node *node)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +#endif
> +
> +/**
> + * Build a runnable power sequence from platform data, and add the resources
> + * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
> + * on dev.
> + * @dev device that will use the power sequence. All resources will be
> + * devm-allocated against it.
> + * @ress list that holds the power_seq_resources already used by this device.
> + * Resources newly met in the sequence will be added to it.
> + * @pseq power sequence in platform format.
> + */
> +struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> + struct platform_power_seq *pseq);
I believe kernel-doc comments should precede the implementation, not the
prototype. Also the kernel-doc comment doesn't correspond to what is
described in Documentation/kernel-doc-nano-HOWTO.txt.
Thierry
> +
> +/**
> + * Run the given power sequence. Returns 0 on success, error code in case of
> + * failure.
> + */
> +int power_seq_run(struct power_seq *seq);
> +
> +#endif
> --
> 1.7.11.4
>
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 0/6] OMAPDSS: Pass output specific parameters from panel driver to output
From: Archit Taneja @ 2012-08-16 7:48 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <343817088-29645-1-git-send-email-archit@ti.com>
This is a continuation of the series below:
http://marc.info/?l=linux-omap&m\x134381744304672&w=2
This series tries to pass output specific parameters from panel driver to the
connected output. These are basically parameters which can't really be shared
across different outputs. An attempt has been made to have similar looking
functions for setting DSI videomode timings and RFBI timings. But all the other
parameters in the series take output specific arguments.
After this series, the interface drivers look more independent of
omap_dss_device. It'll now be easier to add outputs as another DSS entity, and
pass them rather than passing omap_dss_device from panel drivers to
output/interface drivers.
git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git pass_output_specific
Tested on 4430SDP and 3430SDP
Archit Taneja (6):
OMAPDSS: DSI: Maintain copy of operation mode in driver data
OMAPDSS: DSI: Rename dsi_videomode_data to dsi_videomode_timings
OMAPDSS: DSI: Maintain copy of video mode timings in driver data
OMAPDSS: RFBI: Maitain copy of rfbi timings in driver data
OMAPDSS: VENC: Maintain copy of venc type in driver data
OMAPDSS: VENC: Maintian copy of video output polarity in private data
drivers/video/omap2/displays/panel-n8x0.c | 2 +
drivers/video/omap2/displays/panel-taal.c | 1 +
drivers/video/omap2/dss/dsi.c | 94 ++++++++++++++++++++---------
drivers/video/omap2/dss/dss.h | 4 ++
drivers/video/omap2/dss/rfbi.c | 14 ++++-
drivers/video/omap2/dss/venc.c | 28 ++++++++-
drivers/video/omap2/dss/venc_panel.c | 8 +++
include/video/omapdss.h | 10 ++-
8 files changed, 126 insertions(+), 35 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH 1/6] OMAPDSS: DSI: Maintain copy of operation mode in driver data
From: Archit Taneja @ 2012-08-16 7:48 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345102594-6222-1-git-send-email-archit@ti.com>
The DSI driver currently relies on the omap_dss_device struct to know the mode
of operation of the DSI protocol(command or video mode). This makes the DSI
interface driver dependent on the omap_dss_device struct.
Make the DSI driver data maintain it's own operation mode field. The panel
driver is expected to call omapdss_dsi_set_operation_mode() before the interface
is enabled.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/displays/panel-taal.c | 1 +
drivers/video/omap2/dss/dsi.c | 42 +++++++++++++++++++++--------
include/video/omapdss.h | 2 ++
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
index d220f19..649247f 100644
--- a/drivers/video/omap2/displays/panel-taal.c
+++ b/drivers/video/omap2/displays/panel-taal.c
@@ -1063,6 +1063,7 @@ static int taal_power_on(struct omap_dss_device *dssdev)
omapdss_dsi_set_size(dssdev, dssdev->panel.timings.x_res,
dssdev->panel.timings.y_res);
omapdss_dsi_set_pixel_format(dssdev, dssdev->panel.dsi_pix_fmt);
+ omapdss_dsi_set_operation_mode(dssdev, dssdev->panel.dsi_mode);
r = omapdss_dsi_display_enable(dssdev);
if (r) {
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 8f94cb80..c10c8cb 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -335,6 +335,7 @@ struct dsi_data {
struct dss_lcd_mgr_config mgr_config;
struct omap_video_timings timings;
enum omap_dss_dsi_pixel_format pix_fmt;
+ enum omap_dss_dsi_mode mode;
};
struct dsi_packet_sent_handler_data {
@@ -2362,7 +2363,7 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
dsi_cio_timings(dsidev);
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
/* DDR_CLK_ALWAYS_ON */
REG_FLD_MOD(dsidev, DSI_CLK_CTRL,
dssdev->panel.dsi_vm_data.ddr_clk_always_on, 13, 13);
@@ -3609,9 +3610,10 @@ static void dsi_set_hs_tx_timeout(struct platform_device *dsidev,
static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int num_line_buffers;
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
int bpp = dsi_get_pixel_size(dsi->pix_fmt);
unsigned line_buf_size = dsi_get_line_buf_size(dsidev);
@@ -3909,7 +3911,7 @@ static int dsi_proto_config(struct omap_dss_device *dssdev)
dsi_config_vp_num_line_buffers(dssdev);
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
dsi_config_vp_sync_events(dssdev);
dsi_config_blanking_modes(dssdev);
dsi_config_cmd_mode_interleaving(dssdev);
@@ -3988,7 +3990,7 @@ static void dsi_proto_timings(struct omap_dss_device *dssdev)
DSSDBG("enter_hs_mode_lat %u, exit_hs_mode_lat %u\n",
enter_hs_mode_lat, exit_hs_mode_lat);
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
/* TODO: Implement a video mode check_timings function */
int hsa = dssdev->panel.dsi_vm_data.hsa;
int hfp = dssdev->panel.dsi_vm_data.hfp;
@@ -4113,7 +4115,7 @@ int dsi_enable_video_output(struct omap_dss_device *dssdev, int channel)
u16 word_count;
int r;
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
switch (dsi->pix_fmt) {
case OMAP_DSS_DSI_FMT_RGB888:
data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
@@ -4149,7 +4151,7 @@ int dsi_enable_video_output(struct omap_dss_device *dssdev, int channel)
r = dss_mgr_enable(dssdev->manager);
if (r) {
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
dsi_if_enable(dsidev, false);
dsi_vc_enable(dsidev, channel, false);
}
@@ -4164,8 +4166,9 @@ EXPORT_SYMBOL(dsi_enable_video_output);
void dsi_disable_video_output(struct omap_dss_device *dssdev, int channel)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
dsi_if_enable(dsidev, false);
dsi_vc_enable(dsidev, channel, false);
@@ -4379,7 +4382,7 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
int r;
u32 irq = 0;
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_CMD_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_CMD_MODE) {
dsi->timings.hsw = 1;
dsi->timings.hfp = 1;
dsi->timings.hbp = 1;
@@ -4429,7 +4432,7 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
return 0;
err1:
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_CMD_MODE)
+ if (dsi->mode = OMAP_DSS_DSI_CMD_MODE)
omap_dispc_unregister_isr(dsi_framedone_irq_callback,
(void *) dssdev, irq);
err:
@@ -4438,7 +4441,10 @@ err:
static void dsi_display_uninit_dispc(struct omap_dss_device *dssdev)
{
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_CMD_MODE) {
+ struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
+
+ if (dsi->mode = OMAP_DSS_DSI_CMD_MODE) {
u32 irq;
irq = dispc_mgr_get_framedone_irq(dssdev->manager->id);
@@ -4695,6 +4701,20 @@ void omapdss_dsi_set_pixel_format(struct omap_dss_device *dssdev,
}
EXPORT_SYMBOL(omapdss_dsi_set_pixel_format);
+void omapdss_dsi_set_operation_mode(struct omap_dss_device *dssdev,
+ enum omap_dss_dsi_mode mode)
+{
+ struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
+
+ mutex_lock(&dsi->lock);
+
+ dsi->mode = mode;
+
+ mutex_unlock(&dsi->lock);
+}
+EXPORT_SYMBOL(omapdss_dsi_set_operation_mode);
+
static int __init dsi_init_display(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
@@ -4702,7 +4722,7 @@ static int __init dsi_init_display(struct omap_dss_device *dssdev)
DSSDBG("DSI init\n");
- if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_CMD_MODE) {
+ if (dsi->mode = OMAP_DSS_DSI_CMD_MODE) {
dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE |
OMAP_DSS_DISPLAY_CAP_TEAR_ELIM;
}
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 88ac6e8..ef14ac5 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -724,6 +724,8 @@ void omapdss_dsi_set_timings(struct omap_dss_device *dssdev,
void omapdss_dsi_set_size(struct omap_dss_device *dssdev, u16 w, u16 h);
void omapdss_dsi_set_pixel_format(struct omap_dss_device *dssdev,
enum omap_dss_dsi_pixel_format fmt);
+void omapdss_dsi_set_operation_mode(struct omap_dss_device *dssdev,
+ enum omap_dss_dsi_mode mode);
int omap_dsi_update(struct omap_dss_device *dssdev, int channel,
void (*callback)(int, void *), void *data);
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/6] OMAPDSS: DSI: Rename dsi_videomode_data to dsi_videomode_timings
From: Archit Taneja @ 2012-08-16 7:48 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345102594-6222-1-git-send-email-archit@ti.com>
The struct omap_dss_dsi_videomode_data holds fields which need to be configured
for DSI to operate in video mode. Rename the struct to dsi_videomode_timings.
One reason to do this is because most of the fields in the struct are timings
related. The other reason is to create a generic op for output specific
timings. This generic op can be considered as a way to set custom or private
timings for the output.
In the case of OMAP, DSI and RFBI require some more timings apart from the
regular DISPC timings. The structs omap_dss_videomode_timings and rfbi_timings
can be considered as these output specific timings respectively.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dsi.c | 32 ++++++++++++++++----------------
include/video/omapdss.h | 4 ++--
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index c10c8cb..62549f6 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -2366,7 +2366,7 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
/* DDR_CLK_ALWAYS_ON */
REG_FLD_MOD(dsidev, DSI_CLK_CTRL,
- dssdev->panel.dsi_vm_data.ddr_clk_always_on, 13, 13);
+ dssdev->panel.dsi_vm_timings.ddr_clk_always_on, 13, 13);
}
dsi->ulps_enabled = false;
@@ -2704,7 +2704,7 @@ void omapdss_dsi_vc_enable_hs(struct omap_dss_device *dssdev, int channel,
dsi_force_tx_stop_mode_io(dsidev);
/* start the DDR clock by sending a NULL packet */
- if (dssdev->panel.dsi_vm_data.ddr_clk_always_on && enable)
+ if (dssdev->panel.dsi_vm_timings.ddr_clk_always_on && enable)
dsi_vc_send_null(dssdev, channel);
}
EXPORT_SYMBOL(omapdss_dsi_vc_enable_hs);
@@ -3638,8 +3638,8 @@ static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev)
static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
- bool vsync_end = dssdev->panel.dsi_vm_data.vp_vsync_end;
- bool hsync_end = dssdev->panel.dsi_vm_data.vp_hsync_end;
+ bool vsync_end = dssdev->panel.dsi_vm_timings.vp_vsync_end;
+ bool hsync_end = dssdev->panel.dsi_vm_timings.vp_hsync_end;
u32 r;
r = dsi_read_reg(dsidev, DSI_CTRL);
@@ -3656,10 +3656,10 @@ static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
static void dsi_config_blanking_modes(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
- int blanking_mode = dssdev->panel.dsi_vm_data.blanking_mode;
- int hfp_blanking_mode = dssdev->panel.dsi_vm_data.hfp_blanking_mode;
- int hbp_blanking_mode = dssdev->panel.dsi_vm_data.hbp_blanking_mode;
- int hsa_blanking_mode = dssdev->panel.dsi_vm_data.hsa_blanking_mode;
+ int blanking_mode = dssdev->panel.dsi_vm_timings.blanking_mode;
+ int hfp_blanking_mode = dssdev->panel.dsi_vm_timings.hfp_blanking_mode;
+ int hbp_blanking_mode = dssdev->panel.dsi_vm_timings.hbp_blanking_mode;
+ int hsa_blanking_mode = dssdev->panel.dsi_vm_timings.hsa_blanking_mode;
u32 r;
/*
@@ -3992,14 +3992,14 @@ static void dsi_proto_timings(struct omap_dss_device *dssdev)
if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
/* TODO: Implement a video mode check_timings function */
- int hsa = dssdev->panel.dsi_vm_data.hsa;
- int hfp = dssdev->panel.dsi_vm_data.hfp;
- int hbp = dssdev->panel.dsi_vm_data.hbp;
- int vsa = dssdev->panel.dsi_vm_data.vsa;
- int vfp = dssdev->panel.dsi_vm_data.vfp;
- int vbp = dssdev->panel.dsi_vm_data.vbp;
- int window_sync = dssdev->panel.dsi_vm_data.window_sync;
- bool hsync_end = dssdev->panel.dsi_vm_data.vp_hsync_end;
+ int hsa = dssdev->panel.dsi_vm_timings.hsa;
+ int hfp = dssdev->panel.dsi_vm_timings.hfp;
+ int hbp = dssdev->panel.dsi_vm_timings.hbp;
+ int vsa = dssdev->panel.dsi_vm_timings.vsa;
+ int vfp = dssdev->panel.dsi_vm_timings.vfp;
+ int vbp = dssdev->panel.dsi_vm_timings.vbp;
+ int window_sync = dssdev->panel.dsi_vm_timings.window_sync;
+ bool hsync_end = dssdev->panel.dsi_vm_timings.vp_hsync_end;
struct omap_video_timings *timings = &dsi->timings;
int bpp = dsi_get_pixel_size(dsi->pix_fmt);
int tl, t_he, width_bytes;
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index ef14ac5..363235c 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -243,7 +243,7 @@ void rfbi_bus_unlock(void);
/* DSI */
-struct omap_dss_dsi_videomode_data {
+struct omap_dss_dsi_videomode_timings {
/* DSI video mode blanking data */
/* Unit: byte clock cycles */
u16 hsa;
@@ -564,7 +564,7 @@ struct omap_dss_device {
enum omap_dss_dsi_pixel_format dsi_pix_fmt;
enum omap_dss_dsi_mode dsi_mode;
- struct omap_dss_dsi_videomode_data dsi_vm_data;
+ struct omap_dss_dsi_videomode_timings dsi_vm_timings;
} panel;
struct {
--
1.7.9.5
^ permalink raw reply related
* [PATCH 3/6] OMAPDSS: DSI: Maintain copy of video mode timings in driver data
From: Archit Taneja @ 2012-08-16 7:48 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345102594-6222-1-git-send-email-archit@ti.com>
The DSI driver currently relies on the omap_dss_device struct to receive the
video mode timings requested by the panel driver. This makes the DSI interface
driver dependent on the omap_dss_device struct.
Make the DSI driver data maintain it's own video mode timings field. The panel
driver is expected to call omapdss_dsi_set_videomode_timings() to configure the
video mode timings before the interface is enabled. The function takes in a
void pointer rather than a pointer to omap_dss_dsi_videomode_timings struct.
This is because this function will finally be an output op shared across
different outputs to set custom or private timings.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dsi.c | 52 ++++++++++++++++++++++++++++-------------
include/video/omapdss.h | 2 ++
2 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 62549f6..6c2c746 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -336,6 +336,7 @@ struct dsi_data {
struct omap_video_timings timings;
enum omap_dss_dsi_pixel_format pix_fmt;
enum omap_dss_dsi_mode mode;
+ struct omap_dss_dsi_videomode_timings vm_timings;
};
struct dsi_packet_sent_handler_data {
@@ -2366,7 +2367,7 @@ static int dsi_cio_init(struct omap_dss_device *dssdev)
if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
/* DDR_CLK_ALWAYS_ON */
REG_FLD_MOD(dsidev, DSI_CLK_CTRL,
- dssdev->panel.dsi_vm_timings.ddr_clk_always_on, 13, 13);
+ dsi->vm_timings.ddr_clk_always_on, 13, 13);
}
dsi->ulps_enabled = false;
@@ -2688,6 +2689,7 @@ void omapdss_dsi_vc_enable_hs(struct omap_dss_device *dssdev, int channel,
bool enable)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
DSSDBG("dsi_vc_enable_hs(%d, %d)\n", channel, enable);
@@ -2704,7 +2706,7 @@ void omapdss_dsi_vc_enable_hs(struct omap_dss_device *dssdev, int channel,
dsi_force_tx_stop_mode_io(dsidev);
/* start the DDR clock by sending a NULL packet */
- if (dssdev->panel.dsi_vm_timings.ddr_clk_always_on && enable)
+ if (dsi->vm_timings.ddr_clk_always_on && enable)
dsi_vc_send_null(dssdev, channel);
}
EXPORT_SYMBOL(omapdss_dsi_vc_enable_hs);
@@ -3638,8 +3640,9 @@ static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev)
static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
- bool vsync_end = dssdev->panel.dsi_vm_timings.vp_vsync_end;
- bool hsync_end = dssdev->panel.dsi_vm_timings.vp_hsync_end;
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
+ bool vsync_end = dsi->vm_timings.vp_vsync_end;
+ bool hsync_end = dsi->vm_timings.vp_hsync_end;
u32 r;
r = dsi_read_reg(dsidev, DSI_CTRL);
@@ -3656,10 +3659,11 @@ static void dsi_config_vp_sync_events(struct omap_dss_device *dssdev)
static void dsi_config_blanking_modes(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
- int blanking_mode = dssdev->panel.dsi_vm_timings.blanking_mode;
- int hfp_blanking_mode = dssdev->panel.dsi_vm_timings.hfp_blanking_mode;
- int hbp_blanking_mode = dssdev->panel.dsi_vm_timings.hbp_blanking_mode;
- int hsa_blanking_mode = dssdev->panel.dsi_vm_timings.hsa_blanking_mode;
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
+ int blanking_mode = dsi->vm_timings.blanking_mode;
+ int hfp_blanking_mode = dsi->vm_timings.hfp_blanking_mode;
+ int hbp_blanking_mode = dsi->vm_timings.hbp_blanking_mode;
+ int hsa_blanking_mode = dsi->vm_timings.hsa_blanking_mode;
u32 r;
/*
@@ -3992,14 +3996,14 @@ static void dsi_proto_timings(struct omap_dss_device *dssdev)
if (dsi->mode = OMAP_DSS_DSI_VIDEO_MODE) {
/* TODO: Implement a video mode check_timings function */
- int hsa = dssdev->panel.dsi_vm_timings.hsa;
- int hfp = dssdev->panel.dsi_vm_timings.hfp;
- int hbp = dssdev->panel.dsi_vm_timings.hbp;
- int vsa = dssdev->panel.dsi_vm_timings.vsa;
- int vfp = dssdev->panel.dsi_vm_timings.vfp;
- int vbp = dssdev->panel.dsi_vm_timings.vbp;
- int window_sync = dssdev->panel.dsi_vm_timings.window_sync;
- bool hsync_end = dssdev->panel.dsi_vm_timings.vp_hsync_end;
+ int hsa = dsi->vm_timings.hsa;
+ int hfp = dsi->vm_timings.hfp;
+ int hbp = dsi->vm_timings.hbp;
+ int vsa = dsi->vm_timings.vsa;
+ int vfp = dsi->vm_timings.vfp;
+ int vbp = dsi->vm_timings.vbp;
+ int window_sync = dsi->vm_timings.window_sync;
+ bool hsync_end = dsi->vm_timings.vp_hsync_end;
struct omap_video_timings *timings = &dsi->timings;
int bpp = dsi_get_pixel_size(dsi->pix_fmt);
int tl, t_he, width_bytes;
@@ -4715,6 +4719,22 @@ void omapdss_dsi_set_operation_mode(struct omap_dss_device *dssdev,
}
EXPORT_SYMBOL(omapdss_dsi_set_operation_mode);
+void omapdss_dsi_set_videomode_timings(struct omap_dss_device *dssdev,
+ void *timings)
+{
+ struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
+ struct omap_dss_dsi_videomode_timings *vm_timings + (struct omap_dss_dsi_videomode_timings *) timings;
+
+ mutex_lock(&dsi->lock);
+
+ dsi->vm_timings = *vm_timings;
+
+ mutex_unlock(&dsi->lock);
+}
+EXPORT_SYMBOL(omapdss_dsi_set_videomode_timings);
+
static int __init dsi_init_display(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 363235c..faac986 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -726,6 +726,8 @@ void omapdss_dsi_set_pixel_format(struct omap_dss_device *dssdev,
enum omap_dss_dsi_pixel_format fmt);
void omapdss_dsi_set_operation_mode(struct omap_dss_device *dssdev,
enum omap_dss_dsi_mode mode);
+void omapdss_dsi_set_videomode_timings(struct omap_dss_device *dssdev,
+ void *timings);
int omap_dsi_update(struct omap_dss_device *dssdev, int channel,
void (*callback)(int, void *), void *data);
--
1.7.9.5
^ permalink raw reply related
* [PATCH 4/6] OMAPDSS: RFBI: Maitain copy of rfbi timings in driver data
From: Archit Taneja @ 2012-08-16 7:48 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345102594-6222-1-git-send-email-archit@ti.com>
The RFBI driver currently relies on the omap_dss_device struct to receive the
rfbi specific timings requested by the panel driver. This makes the RFBI
interface driver dependent on the omap_dss_device struct.
Make the RFBI driver data maintain it's own rfbi specific timings field. The
panel driver is expected to call omapdss_rfbi_set_interface_timings() to
configure the rfbi timings before the interface is enabled. The function takes
a void pointer rather than a pointer to rfbi_timings struct. This is because
this function will finally be an output op shared across different outputs to
set custom or private timings.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/displays/panel-n8x0.c | 2 ++
drivers/video/omap2/dss/rfbi.c | 14 +++++++++++---
include/video/omapdss.h | 2 ++
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/video/omap2/displays/panel-n8x0.c b/drivers/video/omap2/displays/panel-n8x0.c
index 3ffd987..14adc1d 100644
--- a/drivers/video/omap2/displays/panel-n8x0.c
+++ b/drivers/video/omap2/displays/panel-n8x0.c
@@ -307,6 +307,8 @@ static int n8x0_panel_power_on(struct omap_dss_device *dssdev)
dssdev->panel.timings.y_res);
omapdss_rfbi_set_pixel_size(dssdev, dssdev->ctrl.pixel_size);
omapdss_rfbi_set_data_lines(dssdev, dssdev->phy.rfbi.data_lines);
+ omapdss_rfbi_set_interface_timings(dssdev,
+ (void *) &dssdev->ctrl.rfbi_timings);
r = omapdss_rfbi_display_enable(dssdev);
if (r)
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 698d5b9..b5b0bc6 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -115,6 +115,7 @@ static struct {
struct omap_video_timings timings;
int pixel_size;
int data_lines;
+ struct rfbi_timings intf_timings;
} rfbi;
static inline void rfbi_write_reg(const struct rfbi_reg idx, u32 val)
@@ -799,6 +800,15 @@ void omapdss_rfbi_set_data_lines(struct omap_dss_device *dssdev, int data_lines)
}
EXPORT_SYMBOL(omapdss_rfbi_set_data_lines);
+void omapdss_rfbi_set_interface_timings(struct omap_dss_device *dssdev,
+ void *timings)
+{
+ struct rfbi_timings *t = (struct rfbi_timings *) timings;
+
+ rfbi.intf_timings = *t;
+}
+EXPORT_SYMBOL(omapdss_rfbi_set_interface_timings);
+
static void rfbi_dump_regs(struct seq_file *s)
{
#define DUMPREG(r) seq_printf(s, "%-35s %08x\n", #r, rfbi_read_reg(r))
@@ -907,9 +917,7 @@ int omapdss_rfbi_display_enable(struct omap_dss_device *dssdev)
rfbi_configure(dssdev->phy.rfbi.channel, rfbi.pixel_size,
rfbi.data_lines);
- rfbi_set_timings(dssdev->phy.rfbi.channel,
- &dssdev->ctrl.rfbi_timings);
-
+ rfbi_set_timings(dssdev->phy.rfbi.channel, &rfbi.intf_timings);
return 0;
err1:
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index faac986..91eba93 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -765,5 +765,7 @@ void omapdss_rfbi_set_pixel_size(struct omap_dss_device *dssdev,
int pixel_size);
void omapdss_rfbi_set_data_lines(struct omap_dss_device *dssdev,
int data_lines);
+void omapdss_rfbi_set_interface_timings(struct omap_dss_device *dssdev,
+ void *timings);
#endif
--
1.7.9.5
^ permalink raw reply related
* [PATCH 5/6] OMAPDSS: VENC: Maintain copy of venc type in driver data
From: Archit Taneja @ 2012-08-16 7:48 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345102594-6222-1-git-send-email-archit@ti.com>
The VENC driver currently relies on the omap_dss_device struct to configure the
venc type. This makes the VENC interface driver dependent on the omap_dss_device
struct.
Make the VENC driver data maintain it's own 'venc type' field. A panel driver
is expected to call omapdss_venc_set_type() before enabling the interface or
changing the type via display sysfs attributes.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dss.h | 2 ++
drivers/video/omap2/dss/venc.c | 15 +++++++++++++--
drivers/video/omap2/dss/venc_panel.c | 2 ++
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index f919dc8..c17d298 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -477,6 +477,8 @@ int omapdss_venc_check_timings(struct omap_dss_device *dssdev,
struct omap_video_timings *timings);
u32 omapdss_venc_get_wss(struct omap_dss_device *dssdev);
int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss);
+void omapdss_venc_set_type(struct omap_dss_device *dssdev,
+ enum omap_dss_venc_type type);
int venc_panel_init(void);
void venc_panel_exit(void);
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index d96025e..2d90fcf 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -302,6 +302,7 @@ static struct {
struct clk *tv_dac_clk;
struct omap_video_timings timings;
+ enum omap_dss_venc_type type;
} venc;
static inline void venc_write_reg(int idx, u32 val)
@@ -436,12 +437,12 @@ static int venc_power_on(struct omap_dss_device *dssdev)
venc_reset();
venc_write_config(venc_timings_to_config(&venc.timings));
- dss_set_venc_output(dssdev->phy.venc.type);
+ dss_set_venc_output(venc.type);
dss_set_dac_pwrdn_bgz(1);
l = 0;
- if (dssdev->phy.venc.type = OMAP_DSS_VENC_TYPE_COMPOSITE)
+ if (venc.type = OMAP_DSS_VENC_TYPE_COMPOSITE)
l |= 1 << 1;
else /* S-Video */
l |= (1 << 0) | (1 << 2);
@@ -628,6 +629,16 @@ err:
return r;
}
+void omapdss_venc_set_type(struct omap_dss_device *dssdev,
+ enum omap_dss_venc_type type)
+{
+ mutex_lock(&venc.venc_lock);
+
+ venc.type = type;
+
+ mutex_unlock(&venc.venc_lock);
+}
+
static int __init venc_init_display(struct omap_dss_device *dssdev)
{
DSSDBG("init_display\n");
diff --git a/drivers/video/omap2/dss/venc_panel.c b/drivers/video/omap2/dss/venc_panel.c
index 8096369..ef21361 100644
--- a/drivers/video/omap2/dss/venc_panel.c
+++ b/drivers/video/omap2/dss/venc_panel.c
@@ -68,6 +68,7 @@ static ssize_t display_output_type_store(struct device *dev,
if (dssdev->phy.venc.type != new_type) {
dssdev->phy.venc.type = new_type;
+ omapdss_venc_set_type(dssdev, new_type);
if (dssdev->state = OMAP_DSS_DISPLAY_ACTIVE) {
omapdss_venc_display_disable(dssdev);
omapdss_venc_display_enable(dssdev);
@@ -128,6 +129,7 @@ static int venc_panel_enable(struct omap_dss_device *dssdev)
}
omapdss_venc_set_timings(dssdev, &dssdev->panel.timings);
+ omapdss_venc_set_type(dssdev, dssdev->phy.venc.type);
r = omapdss_venc_display_enable(dssdev);
if (r)
--
1.7.9.5
^ permalink raw reply related
* [PATCH 6/6] OMAPDSS: VENC: Maintian copy of video output polarity in private data
From: Archit Taneja @ 2012-08-16 7:48 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Archit Taneja
In-Reply-To: <1345102594-6222-1-git-send-email-archit@ti.com>
The VENC driver currently relies on the omap_dss_device struct to configure the
video output polarity. This makes the VENC interface driver dependent on the
omap_dss_device struct.
Make the VENC driver data maintain it's own polarity field. A panel driver
is expected to call omapdss_venc_set_vid_out_polarity() before enabling the
interface.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dss.h | 2 ++
drivers/video/omap2/dss/venc.c | 13 ++++++++++++-
drivers/video/omap2/dss/venc_panel.c | 6 ++++++
3 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index c17d298..b2cf5530 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -479,6 +479,8 @@ u32 omapdss_venc_get_wss(struct omap_dss_device *dssdev);
int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss);
void omapdss_venc_set_type(struct omap_dss_device *dssdev,
enum omap_dss_venc_type type);
+void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
+ enum omap_dss_signal_level vid_out_pol);
int venc_panel_init(void);
void venc_panel_exit(void);
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 2d90fcf..8cb372f 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -303,6 +303,7 @@ static struct {
struct omap_video_timings timings;
enum omap_dss_venc_type type;
+ enum omap_dss_signal_level vid_out_pol;
} venc;
static inline void venc_write_reg(int idx, u32 val)
@@ -447,7 +448,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
else /* S-Video */
l |= (1 << 0) | (1 << 2);
- if (dssdev->phy.venc.invert_polarity = false)
+ if (venc.vid_out_pol = OMAPDSS_SIG_ACTIVE_HIGH)
l |= 1 << 3;
venc_write_reg(VENC_OUTPUT_CONTROL, l);
@@ -639,6 +640,16 @@ void omapdss_venc_set_type(struct omap_dss_device *dssdev,
mutex_unlock(&venc.venc_lock);
}
+void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
+ enum omap_dss_signal_level vid_out_pol)
+{
+ mutex_lock(&venc.venc_lock);
+
+ venc.vid_out_pol = vid_out_pol;
+
+ mutex_unlock(&venc.venc_lock);
+}
+
static int __init venc_init_display(struct omap_dss_device *dssdev)
{
DSSDBG("init_display\n");
diff --git a/drivers/video/omap2/dss/venc_panel.c b/drivers/video/omap2/dss/venc_panel.c
index ef21361..a8117d0 100644
--- a/drivers/video/omap2/dss/venc_panel.c
+++ b/drivers/video/omap2/dss/venc_panel.c
@@ -118,6 +118,7 @@ static void venc_panel_remove(struct omap_dss_device *dssdev)
static int venc_panel_enable(struct omap_dss_device *dssdev)
{
int r;
+ enum omap_dss_signal_level vid_out_pol;
dev_dbg(&dssdev->dev, "venc_panel_enable\n");
@@ -131,6 +132,11 @@ static int venc_panel_enable(struct omap_dss_device *dssdev)
omapdss_venc_set_timings(dssdev, &dssdev->panel.timings);
omapdss_venc_set_type(dssdev, dssdev->phy.venc.type);
+ vid_out_pol = dssdev->phy.venc.invert_polarity ?
+ OMAPDSS_SIG_ACTIVE_LOW : OMAPDSS_SIG_ACTIVE_HIGH;
+
+ omapdss_venc_set_vid_out_polarity(dssdev, vid_out_pol);
+
r = omapdss_venc_display_enable(dssdev);
if (r)
goto err;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-08-16 9:19 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann,
Leela Krishna Amudala,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20120816074232.GA17917-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
On 08/16/2012 04:42 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
>> Some device drivers (panel backlights especially) need to follow precise
>> sequences for powering on and off, involving gpios, regulators, PWMs
>> with a precise powering order and delays to respect between each steps.
>> These sequences are board-specific, and do not belong to a particular
>> driver - therefore they have been performed by board-specific hook
>> functions to far.
>>
>> With the advent of the device tree and of ARM kernels that are not
>> board-tied, we cannot rely on these board-specific hooks anymore but
>> need a way to implement these sequences in a portable manner. This patch
>> introduces a simple interpreter that can execute such power sequences
>> encoded either as platform data or within the device tree.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> .../devicetree/bindings/power_seq/power_seq.txt | 101 +++++
>> Documentation/power/power_seq.txt | 129 +++++++
>> drivers/power/Kconfig | 3 +
>> drivers/power/Makefile | 1 +
>> drivers/power/power_seq.c | 420 +++++++++++++++++++++
>> include/linux/power_seq.h | 142 +++++++
>> 6 files changed, 796 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
>> create mode 100644 Documentation/power/power_seq.txt
>> create mode 100644 drivers/power/power_seq.c
>> create mode 100644 include/linux/power_seq.h
>>
>> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt
>> new file mode 100644
>> index 0000000..749c6e4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
>> @@ -0,0 +1,101 @@
>> +Specifying Power Sequences in the Device Tree
>> +======================>> +In the device tree, power sequences are specified as sub-nodes of the device
>> +node and reference resources declared by that device.
>> +
>> +For an introduction about runtime interpreted power sequences, see
>> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
>> +
>> +Power Sequences Structure
>> +-------------------------
>> +Power sequences are sub-nodes that are named such as the device driver can find
>
> "named such that"?
>
>> +them. The driver's documentation should list the sequence names it recognizes.
>> +
>> +Inside a power sequence node are sub-nodes that describe the different steps
>> +of the sequence. Each step must be named sequentially, with the first step
>> +named step0, the second step1, etc. Failure to follow this rule will result in a
>> +parsing error.
>> +
>> +Power Sequences Steps
>> +---------------------
>> +Every step of a sequence describes an action to be performed on a resource. It
>> +generally includes a property named after the resource type, and which value
>> +references the resource to be used. Depending on the resource type, additional
>> +properties can be defined to control the action to be performed.
>> +
>> +Currently supported resource types are:
>
> I think the "currently" can be dropped.
>
>> +- "delay", which should simply contain a delay in microseconds to wait before
>> + going on with the rest of the sequence. It takes no additional property.
>> +- "regulator" contains the name of a regulator to be acquired using
>> + regulator_get(). An additional property, either "enable" or "disable", must be
>> + present to control whether the regulator should be enabled or disabled during
>> + that step.
>> +- "pwm" is set to the name of a PWM acquired using pwm_get(). As with regulator,
>> + an additional "enable" or "disable" property is required.
>> +- "gpio" contains the name of a GPIO to enable or disable using the same
>> + additional property as regulator or pwm. The gpio is resolved by appending
>> + "-gpio" to the given name and looking for a device property with a GPIO
>> + phandle.
>
> I find this part slightly confusing. It doesn't seem quite obvious from
> the text that "delay", "regulator", "pwm" and "gpio" are also actual
> property names.
>
> It might also be worth saying that the "enable" and "disable" properties
> are boolean in nature and don't need a value.
>
> Then again this becomes much clearer with the example below, so maybe
> I'm just being too picky here.
This could clearly be improved. I have added a sentence before that says
that every step must define one property named after a supported
resource type - that should help. Then, as you said, the example should
clear all remaining doubts.
>
>> +
>> +Example
>> +-------
>> +Here are example sequences declared within a backlight device that use all the
>> +supported resources types:
>> +
>> + backlight {
>> + compatible = "pwm-backlight";
>> + ...
>> +
>> + /* resources used by the sequences */
>> + pwms = <&pwm 2 5000000>;
>> + pwm-names = "backlight";
>> + power-supply = <&backlight_reg>;
>> + enable-gpio = <&gpio 28 0>;
>> +
>> + power-on-sequence {
>> + step0 {
>> + regulator = "power";
>> + enable;
>> + };
>> + step1 {
>> + delay = <10000>;
>> + };
>> + step2 {
>> + pwm = "backlight";
>> + enable;
>> + };
>> + step3 {
>> + gpio = "enable";
>> + enable;
>> + };
>> + };
>> +
>> + power-off-sequence {
>> + step0 {
>> + gpio = "enable";
>> + disable;
>> + };
>> + step1 {
>> + pwm = "backlight";
>> + disable;
>> + };
>> + step2 {
>> + delay = <10000>;
>> + };
>> + step3 {
>> + regulator = "power";
>> + disable;
>> + };
>> + };
>> + };
>> +
>> +The first part lists the PWM, regulator, and GPIO resources used by the
>> +sequences. These resources will be requested on behalf of the backlight device
>> +when the sequences are built and are declared according to their own framework
>> +in a way that makes them accessible by name.
>> +
>> +After the resources declaration, two sequences follow for powering the backlight
>> +on and off. Their names are specified by the pwm-backlight driver. Every step
>> +uses one of the "delay", "regulator", "pwm" or "gpio" properties to reference a
>> +previously-declared resource. Additional "enable" or "disable" properties are
>> +also used as needed.
>> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
>> new file mode 100644
>> index 0000000..3ab4f93
>> --- /dev/null
>> +++ b/Documentation/power/power_seq.txt
>> @@ -0,0 +1,129 @@
>> +Runtime Interpreted Power Sequences
>> +=================>> +
>> +Problem
>> +-------
>> +One very common board-dependent code is the out-of-driver code that is used to
>> +turn a device on or off. For instance, SoC boards very commonly use a GPIO
>> +(abstracted to a regulator or not) to control the power supply of a backlight,
>> +disabling it when the backlight is not used in order to save power. The GPIO
>> +that should be used, however, as well as the exact power sequence that may
>> +also involve other resources, is board-dependent and thus unknown of the driver.
>
> "unknown to the driver"?
>
>> +
>> +This was previously addressed by having hooks in the device's platform data that
>> +are called whenever the state of the device might reflect a power change. This
>> +approach, however, introduces board-dependant code into the kernel and is not
>> +compatible with the device tree.
>> +
>> +The Runtime Interpreted Power Sequences (or power sequences for short) aims at
>
> "... Sequences [...] aim"?
>
>> +turning this code into platform data or device tree nodes. Power sequences are
>> +described using a simple format and run by a simple interpreter whenever needed.
>> +This allows to remove the callback mechanism and makes the kernel less
>> +board-dependant.
>> +
>> +What are Power Sequences?
>> +-------------------------
>> +Power sequences are a series of sequential steps during which an action is
>> +performed on a resource. The supported resources so far are:
>
> Again, I don't see a need for "so far" here. It implies that new types
> may be added. While it is quite possible and maybe even likely to happen
> the new types can be added to the documentation at the same time.
>
>> +- delay (just wait for the delay given in microseconds)
>> +- GPIO (enable or disable)
>> +- regulator (enable or disable)
>> +- PWM (enable or disable)
>> +
>> +Every step designates a resource type and parameters that are relevant to it.
>> +For instance, GPIO and PWMs can be enabled or disabled.
>> +
>> +When a power sequence is run, each of its step is executed sequentially until
>
> "each of its steps"
>
>> +one step fails or the end of the sequence is reached.
>> +
>> +Power sequences can be declared as platform data or in the device tree.
>> +
>> +Platform Data Format
>> +--------------------
>> +All relevant data structures for declaring power sequences are located in
>> +include/linux/power_seq.h.
>> +
>> +The platform data is a static instance of simple array of
>> +platform_power_seq_step instances, each
>> +instance describing a step. The type as well as one of id or gpio members
>> +(depending on the type) must be specified. The last step must be of type
>> +POWER_SEQ_STOP. Regulator and PWM resources are identified by name. GPIO are
>> +identified by number. For example, the following sequence will turn on the
>> +"power" regulator of the device, wait 10ms, and set GPIO number 110 to 1:
>> +
>> +static struct platform_power_seq power_on_seq = {
>> + .nb_steps = 3,
>
> I think num_steps would be more canonical.
>
>> + .steps = {
>> + {
>> + .type = POWER_SEQ_REGULATOR,
>> + .regulator.regulator = "power",
>> + .regulator.enable = 1,
>> + },
>
> This may be easier to read as:
>
> .type = POWER_SEQ_REGULATOR,
> .regulator {
> .regulator = "power",
> .enable = 1,
> }
>
> Also, why not rename the .regulator field to .name? That describes
> better what it is and removes the redundancy of having the structure
> named regulator and a regulator field within.
That's right - this is a remain from the time the union was not used to
differenciate the resource types.
>
>> + {
>> + .type = POWER_SEQ_DELAY,
>> + .delay.delay_us = 10000,
>> + },
>> + {
>> + .type = POWER_SEQ_GPIO,
>> + .gpio.gpio = 110,
>> + .gpio.enable = 1,
>> + },
>
> Same comments as for the regulator step above. Also, since enable is a
> boolean field, maybe you should use true and false instead.
>
>> + },
>> +};
>> +
>> +Device Tree
>> +-----------
>> +Power sequences can also be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> + power-supply = <&power_reg>;
>> + switch-gpio = <&gpio 110 0>;
>> +
>> + power-on-sequence {
>> + step0 {
>> + regulator = "power";
>> + enable;
>> + };
>> + step1 {
>> + delay = <10000>;
>> + };
>> + step2 {
>> + gpio = "switch";
>> + enable;
>> + };
>> + };
>> +
>> +See Documentation/devicetree/bindings/power_seq/power_seq.txt for the complete
>> +syntax of the bindings.
>> +
>> +Usage by Drivers and Resources Management
>> +-----------------------------------------
>> +Power sequences make use of resources that must be properly allocated and
>> +managed. The power_seq_build() function builds a power sequence from the
>> +platform data. It also takes care of resolving and allocating the resources
>> +referenced by the sequence if needed:
>> +
>> + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
>> + struct platform_power_seq *pseq);
>> +
>> +The 'dev' argument is the device in the name of which the resources are to be
>> +allocated.
>> +
>> +The 'ress' argument is a list to which the resolved resources are appended. This
>> +avoids allocating a resource referenced in several power sequences multiple
>> +times.
>> +
>> +On success, the function returns a devm allocated resolved sequence that is
>> +ready to be passed to power_seq_run(). In case of failure, and error code is
>> +returned.
>> +
>> +A resolved power sequence returned by power_seq_build can be run by
>> +power_run_run():
>> +
>> + int power_seq_run(power_seq *seq);
>> +
>> +It returns 0 if the sequence has successfully been run, or an error code if a
>> +problem occured.
>> +
>> +There is no need to explicitly free the resources used by the sequence as they
>> +are devm-allocated.
>
> I had some comments about this particular interface for creating
> sequences in the last series. My point was that explicitly requiring
> drivers to manage a list of already allocated resources may be too much
> added complexity. Power sequences should be easy to use, and I find the
> requirement for a separately managed list of resources cumbersome.
>
> What I proposed last time was to collect all power sequences under a
> common parent object, which in turn would take care of managing the
> resources.
Yes, I remember that. While I see why you don't like this list, having a
common parent object to all sequences will not reduce the number of
arguments to pass to power_seq_build() (which is the only function that
has to handle this list now). Also having the list of resources at hand
is needed for some drivers: for instance, pwm-backlight needs to check
that exactly one PWM has been allocated, and takes a reference to it
from this list in order to control the brightness.
Ideally we could embed the list into the device structure, but I don't
see how we can do that without modifying it (and we don't want to modify
it). Another solution would be to keep a static mapping table that
associates a device to its power_seq related resources within
power_seq.c. If we protect it for concurrent access this should make it
possible to make resources management transparent. How does this sound?
Only drawback I see is that we would need to explicitly clean it up
through a dedicated function when the driver exits.
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index c1892f3..4172fe4 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -312,4 +312,7 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
>> thermistor connected on BATCTRL ADC.
>> endif # POWER_SUPPLY
>>
>> +config POWER_SEQ
>> + bool
>> +
>> source "drivers/power/avs/Kconfig"
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index ee58afb..828859c 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -45,3 +45,4 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
>> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
>> obj-$(CONFIG_POWER_AVS) += avs/
>> obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
>> +obj-$(CONFIG_POWER_SEQ) += power_seq.o
>> diff --git a/drivers/power/power_seq.c b/drivers/power/power_seq.c
>> new file mode 100644
>> index 0000000..1dcdbe0
>> --- /dev/null
>> +++ b/drivers/power/power_seq.c
>> @@ -0,0 +1,420 @@
>> +/*
>> + * power_seq.c - A simple power sequence interpreter for platform devices
>> + * and device tree.
>> + *
>> + * Author: Alexandre Courbot <acourbot@nvidia.com>
>> + *
>> + * Copyright (c) 2012 NVIDIA Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + */
>> +
>> +#include <linux/power_seq.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/device.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/gpio.h>
>> +
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +
>> +struct power_seq_step {
>> + /* Copy of the platform data */
>> + struct platform_power_seq_step pdata;
>> + /* Resolved resource */
>> + struct power_seq_resource *resource;
>> +};
>> +
>> +struct power_seq {
>> + struct device *dev;
>> + unsigned int nb_steps;
>> + struct power_seq_step steps[];
>> +};
>> +
>> +static char *res_names[POWER_SEQ_MAX] = {
>> + [POWER_SEQ_DELAY] = "delay",
>> + [POWER_SEQ_REGULATOR] = "regulator",
>> + [POWER_SEQ_GPIO] = "gpio",
>> + [POWER_SEQ_PWM] = "pwm",
>> +};
>
> static const?
>
>> +
>> +static int power_seq_step_run(struct power_seq_step *step)
>> +{
>> + struct platform_power_seq_step *pdata = &step->pdata;
>> + int err = 0;
>> +
>> + switch (pdata->type) {
>> + case POWER_SEQ_DELAY:
>> + usleep_range(pdata->delay.delay_us,
>> + pdata->delay.delay_us + 1000);
>> + break;
>> +#ifdef CONFIG_REGULATOR
>> + case POWER_SEQ_REGULATOR:
>> + if (pdata->regulator.enable)
>> + err = regulator_enable(step->resource->regulator);
>> + else
>> + err = regulator_disable(step->resource->regulator);
>> + break;
>> +#endif
>> +#ifdef CONFIG_PWM
>> + case POWER_SEQ_PWM:
>> + if (pdata->gpio.enable)
>> + err = pwm_enable(step->resource->pwm);
>> + else
>> + pwm_disable(step->resource->pwm);
>> + break;
>> +#endif
>> +#ifdef CONFIG_GPIOLIB
>> + case POWER_SEQ_GPIO:
>> + gpio_set_value_cansleep(pdata->gpio.gpio, pdata->gpio.enable);
>> + break;
>> +#endif
>> + /*
>> + * should never happen unless the sequence includes a step which
>> + * type does not have support compiled in
>
> I think this should be "whose type"? I also remember commenting on the
> whole #ifdef'ery here. I really don't think it is necessary. At least
> for regulators I know that the functions can be used even if the
> subsystem itself isn't supported. The same seems to hold for GPIO and we
> can probably add something similar for PWM.
Actually I kept them because I don't really like the empty function
definitions in the regulator framework. They all return 0 as if the
function completed successfully - here we should at least warn the user
that proper support for that resource is missing.
>
> It might also be a good idea to just skip unsupported resource types
> when the sequence is built, accompanied by runtime warnings that the
> type is not supported.
Agreed.
>
>> + */
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + if (err < 0)
>> + return err;
>> +
>> + return 0;
>
> This can probably be collapsed to just "return err;", can't it?
Totally.
>
>> +}
>> +
>> +int power_seq_run(struct power_seq *seq)
>> +{
>> + struct device *dev = seq->dev;
>> + int err, cpt;
>
> Any reason why you call the loop variable cpt instead of something more
> canonical such as i? Also it should be of type unsigned int.
Fixed.
>
>> +
>> + if (!seq)
>> + return 0;
>> +
>> + for (cpt = 0; cpt < seq->nb_steps; cpt++) {
>> + err = power_seq_step_run(&seq->steps[cpt]);
>> + if (err) {
>> + dev_err(dev, "error %d while running power sequence!\n",
>> + err);
>> + return err;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(power_seq_run);
>> +
>> +#ifdef CONFIG_OF
>> +static int of_power_seq_enable_properties(struct device *dev,
>> + struct device_node *node,
>> + bool *enable)
>
> Maybe rename this to of_power_seq_parse_enable_properties() to make it
> more obvious that it is actually parsing data. It's an awfully long name
> for a function, though.
>
>> +{
>> + if (of_find_property(node, "enable", NULL)) {
>> + *enable = true;
>> + } else if (of_find_property(node, "disable", NULL)) {
>> + *enable = false;
>> + } else {
>> + dev_err(dev, "missing enable or disable property!\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int of_parse_power_seq_step(struct device *dev, struct device_node *node,
>> + struct platform_power_seq_step *step)
>> +{
>> + struct property *res_id = NULL;
>> + int i, err;
>> +
>> + /* Try to find a meaningful name property */
>> + for (i = 0; i < POWER_SEQ_MAX; i++) {
>
> Maybe this should be renamed to POWER_SEQ_MAX_TYPE, POWER_SEQ_TYPE_MAX,
> POWER_SEQ_NUM_TYPES or some such. I had assumed POWER_SEQ_MAX would mean
> the maximum length of a power sequence.
>
>> + struct property *mprop;
>> +
>> + mprop = of_find_property(node, res_names[i], NULL);
>> + if (mprop) {
>> + if (res_id) {
>> + dev_err(dev,
>> + "more than one resource in step!\n");
>> + return -EINVAL;
>> + }
>> + step->type = i;
>> + res_id = mprop;
>> + }
>> + }
>> + if (!res_id) {
>> + dev_err(dev, "missing resource property for power seq step!\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Now parse resource-specific properties */
>> + switch (step->type) {
>> + case POWER_SEQ_DELAY:
>> + err = of_property_read_u32(node, res_id->name,
>> + &step->delay.delay_us);
>> + if (err)
>> + goto err_read_property;
>> +
>> + break;
>> +
>> + case POWER_SEQ_REGULATOR:
>> + err = of_property_read_string(node, res_id->name,
>> + &step->regulator.regulator);
>> + if (err)
>> + goto err_read_property;
>> +
>> + err = of_power_seq_enable_properties(dev, node,
>> + &step->regulator.enable);
>> + if (err)
>> + return err;
>> +
>> + break;
>> +
>> + case POWER_SEQ_PWM:
>> + err = of_property_read_string(node, res_id->name,
>> + &step->pwm.pwm);
>> + if (err)
>> + goto err_read_property;
>> +
>> + err = of_power_seq_enable_properties(dev, node,
>> + &step->pwm.enable);
>> + if (err)
>> + return err;
>> +
>> + break;
>> +
>> +#ifdef CONFIG_OF_GPIO
>> + case POWER_SEQ_GPIO:
>> + {
>> + char prop_name[32]; /* max size of property name */
>> + const char *gpio_name;
>> + int gpio;
>> +
>> + err = of_property_read_string(node, res_id->name, &gpio_name);
>> + if (err)
>> + goto err_read_property;
>> +
>> + /* Resolve the GPIO name */
>> + snprintf(prop_name, 32, "%s-gpio", gpio_name);
>
> I'm not sure if there's a limit on the length of DT property names, but
> maybe using kasprintf would be a better idea here.
According to the regulator code (from which this is inspired) this is
the case - I try to limit dynamic memory allocations as much as possible.
>
>> + gpio = of_get_named_gpio(dev->of_node, prop_name, 0);
>> + if (gpio < 0) {
>> + dev_err(dev, "cannot resolve gpio \"%s\"\n", gpio_name);
>> + return gpio;
>> + }
>> + step->gpio.gpio = gpio;
>> +
>> + err = of_power_seq_enable_properties(dev, node,
>> + &step->gpio.enable);
>> + if (err)
>> + return err;
>> +
>> + break;
>> + }
>> +#endif /* CONFIG_OF_GPIO */
>> +
>> + default:
>> + dev_err(dev, "unhandled power sequence step type %s\n",
>> + res_names[step->type]);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +
>> +err_read_property:
>> + dev_err(dev, "cannot read %s property!", res_names[step->type]);
>> + return -EINVAL;
>> +}
>
> Given the size of this function, I think it might become more readable
> if it were split into separate parse functions for each type of
> resource. It will also allow you to get rid of this #ifdef within the
> function.
Right.
>
>> +
>> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
>> + struct device_node *node)
>> +{
>> + struct device_node *child = NULL;
>> + struct platform_power_seq *pseq;
>> + int nb_steps = 0, size;
>> + int err;
>> +
>> + if (!node)
>> + return ERR_PTR(-EINVAL);
>> +
>> + nb_steps = of_get_child_count(node);
>> + size = sizeof(pseq) + sizeof(struct platform_power_seq_step) * nb_steps;
>
> Shouldn't the first term be sizeof(*pseq)?
Ouch. >_< Thanks.
>
>> + pseq = devm_kzalloc(dev, size, GFP_KERNEL);
>> + if (!pseq)
>> + return ERR_PTR(-ENOMEM);
>> + pseq->nb_steps = nb_steps;
>> +
>> + for_each_child_of_node(node, child) {
>> + unsigned int pos;
>> +
>> + /* Check that the name's format is correct and within bounds */
>> + if (strncmp("step", child->name, 4)) {
>> + err = -EINVAL;
>> + goto parse_error;
>> + }
>> +
>> + err = kstrtoint(child->name + 4, 10, &pos);
>> + if (err < 0)
>> + goto parse_error;
>
> kstrtouint()? Also this is somewhat ugly. Perhaps adding #address-cells
> and #size-cells properties isn't that bad after all.
I really prefer this solution to #address-cells and #size-cells
personally. Using these makes sense when you need to refer to the DT
node through a phandle, which is definitely not the case here. Having
them would just be confusing and error-prone.
>
>> +
>> + if (pos >= nb_steps || pseq->steps[pos].type != 0) {
>> + err = -EINVAL;
>> + goto parse_error;
>> + }
>> +
>> + err = of_parse_power_seq_step(dev, child, &pseq->steps[pos]);
>> + if (err)
>> + return ERR_PTR(err);
>> + }
>> +
>> + return pseq;
>> +
>> +parse_error:
>> + dev_err(dev, "invalid power step name %s!\n", child->name);
>> + return ERR_PTR(err);
>> +}
>> +EXPORT_SYMBOL_GPL(of_parse_power_seq);
>> +#endif /* CONFIG_OF */
>> +
>> +static
>> +struct power_seq_resource *power_seq_find_resource(struct list_head *ress,
>> + struct platform_power_seq_step *step)
>
> I think it is customary to put the return value type on the same line as
> the static modifier.
>
>> +{
>> + struct power_seq_resource *res;
>> +
>> + list_for_each_entry(res, ress, list) {
>> + struct platform_power_seq_step *pdata = res->pdata;
>> +
>> + if (pdata->type != step->type)
>> + continue;
>> +
>> + switch (pdata->type) {
>> + case POWER_SEQ_REGULATOR:
>> + if (!strcmp(pdata->regulator.regulator,
>> + step->regulator.regulator))
>> + return res;
>> + break;
>> + case POWER_SEQ_PWM:
>> + if (!strcmp(pdata->pwm.pwm, step->pwm.pwm))
>> + return res;
>> + break;
>> + case POWER_SEQ_GPIO:
>> + if (pdata->gpio.gpio = step->gpio.gpio)
>> + return res;
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static int power_seq_allocate_resource(struct device *dev,
>> + struct power_seq_resource *res)
>> +{
>> + struct platform_power_seq_step *pdata = res->pdata;
>> + int err;
>> +
>> + switch (pdata->type) {
>> + case POWER_SEQ_DELAY:
>> + break;
>> +#ifdef CONFIG_REGULATOR
>> + case POWER_SEQ_REGULATOR:
>> + res->regulator = devm_regulator_get(dev,
>> + pdata->regulator.regulator);
>> + if (IS_ERR(res->regulator)) {
>> + dev_err(dev, "cannot get regulator \"%s\"\n",
>> + pdata->regulator.regulator);
>> + return PTR_ERR(res->regulator);
>> + }
>> + break;
>> +#endif
>> +#ifdef CONFIG_PWM
>> + case POWER_SEQ_PWM:
>> + res->pwm = devm_pwm_get(dev, pdata->pwm.pwm);
>> + if (IS_ERR(res->pwm)) {
>> + dev_err(dev, "cannot get pwm \"%s\"\n", pdata->pwm.pwm);
>> + return PTR_ERR(res->pwm);
>> + }
>> + break;
>> +#endif
>> +#ifdef CONFIG_GPIOLIB
>> + case POWER_SEQ_GPIO:
>> + err = devm_gpio_request_one(dev, pdata->gpio.gpio,
>> + GPIOF_OUT_INIT_HIGH, "backlight_gpio");
>> + if (err) {
>> + dev_err(dev, "cannot get gpio %d\n", pdata->gpio.gpio);
>> + return err;
>> + }
>> + break;
>> +#endif
>> + default:
>> + dev_err(dev, "invalid resource type %d\n", pdata->type);
>> + return -EINVAL;
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
>> + struct platform_power_seq *pseq)
>> +{
>> + struct power_seq *seq;
>> + struct power_seq_resource *res;
>> + int cpt, err;
>> +
>> + seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(struct power_seq_step) *
>> + pseq->nb_steps, GFP_KERNEL);
>> + if (!seq)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + seq->dev = dev;
>> + seq->nb_steps = pseq->nb_steps;
>> +
>> + for (cpt = 0; cpt < seq->nb_steps; cpt++) {
>> + struct platform_power_seq_step *pstep = &pseq->steps[cpt];
>> + struct power_seq_step *step = &seq->steps[cpt];
>> +
>> + memcpy(&step->pdata, pstep, sizeof(step->pdata));
>> +
>> + /* Delay steps have no resource */
>> + if (pstep->type = POWER_SEQ_DELAY)
>> + continue;
>> +
>> + /* create resource node if not referenced already */
>> + res = power_seq_find_resource(ress, pstep);
>> + if (!res) {
>> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
>> + if (!res)
>> + return ERR_PTR(-ENOMEM);
>> + res->pdata = &step->pdata;
>> +
>> + err = power_seq_allocate_resource(dev, res);
>> + if (err < 0)
>> + return ERR_PTR(err);
>> +
>> + list_add(&res->list, ress);
>> + }
>> + step->resource = res;
>> + }
>> +
>> + return seq;
>> +}
>> +EXPORT_SYMBOL_GPL(power_seq_build);
>> +
>> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
>> +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
>> new file mode 100644
>> index 0000000..d9dd277
>> --- /dev/null
>> +++ b/include/linux/power_seq.h
>> @@ -0,0 +1,142 @@
>> +/*
>> + * power_seq.h
>> + *
>> + * Simple interpreter for defining power sequences as platform data or device
>> + * tree properties.
>> + *
>> + * Power sequences are designed to replace the callbacks typically used in
>> + * board-specific files that implement board-specific power sequences of devices
>> + * such as backlights. A power sequence is an array of resources (which can a
>> + * regulator, a GPIO, a PWM, ...) with an action to perform on it (enable or
>> + * disable) and optional pre and post step delays. By having them interpreted
>> + * instead of arbitrarily executed, it is possible to describe these in the
>> + * device tree and thus remove board-specific code from the kernel.
>> + *
>> + * Author: Alexandre Courbot <acourbot@nvidia.com>
>> + *
>> + * Copyright (c) 2012 NVIDIA Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + */
>> +
>> +#ifndef __LINUX_POWER_SEQ_H
>> +#define __LINUX_POWER_SEQ_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct device;
>> +struct regulator;
>> +struct pwm_device;
>> +struct device_node;
>> +
>> +/**
>> + * The different kinds of resources that can be controlled during the sequences.
>> + */
>> +enum power_seq_res_type {
>> + POWER_SEQ_DELAY,
>> + POWER_SEQ_REGULATOR,
>> + POWER_SEQ_PWM,
>> + POWER_SEQ_GPIO,
>> + POWER_SEQ_MAX,
>> +};
>> +
>> +struct platform_power_seq_delay_step {
>> + unsigned int delay_us;
>> +};
>> +
>> +struct platform_power_seq_regulator_step {
>> + const char *regulator;
>> + bool enable;
>> +};
>> +
>> +struct platform_power_seq_pwm_step {
>> + const char *pwm;
>> + bool enable;
>> +};
>> +
>> +struct platform_power_seq_gpio_step {
>> + int gpio;
>> + bool enable;
>> +};
>> +
>> +/**
>> + * Platform definition of power sequences. A sequence is an array of these,
>> + * terminated by a STOP instance.
>> + */
>> +struct platform_power_seq_step {
>> + enum power_seq_res_type type;
>> + union {
>> + struct platform_power_seq_delay_step delay;
>> + struct platform_power_seq_regulator_step regulator;
>> + struct platform_power_seq_pwm_step pwm;
>> + struct platform_power_seq_gpio_step gpio;
>> + };
>> +};
>> +
>> +struct platform_power_seq {
>> + unsigned int nb_steps;
>> + struct platform_power_seq_step steps[];
>> +};
>> +
>> +/**
>> + * We maintain a list of these to monitor which resources have already
>> + * been met and allocated while building the sequences.
>> + */
>> +struct power_seq_resource {
>> + /* relevant for resolving the resource and knowing its type */
>> + struct platform_power_seq_step *pdata;
>> + /* resolved resource (if any) */
>> + union {
>> + struct regulator *regulator;
>> + struct pwm_device *pwm;
>> + };
>> + struct list_head list;
>> +};
>> +
>> +struct power_seq_resource;
>> +struct power_seq;
>> +
>> +#ifdef CONFIG_OF
>> +/**
>> + * Build a platform data sequence from a device tree node. Memory for the
>> + * platform sequence is allocated using devm_kzalloc on dev and can be freed
>> + * by devm_kfree after power_seq_build is called.
>> + */
>> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
>> + struct device_node *node);
>> +#else
>> +struct platform_power_seq *of_parse_power_seq(struct device *dev,
>> + struct device_node *node)
>> +{
>> + return ERR_PTR(-EINVAL);
>> +}
>> +#endif
>> +
>> +/**
>> + * Build a runnable power sequence from platform data, and add the resources
>> + * it uses into ress. Memory for the sequence is allocated using devm_kzalloc
>> + * on dev.
>> + * @dev device that will use the power sequence. All resources will be
>> + * devm-allocated against it.
>> + * @ress list that holds the power_seq_resources already used by this device.
>> + * Resources newly met in the sequence will be added to it.
>> + * @pseq power sequence in platform format.
>> + */
>> +struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
>> + struct platform_power_seq *pseq);
>
> I believe kernel-doc comments should precede the implementation, not the
> prototype. Also the kernel-doc comment doesn't correspond to what is
> described in Documentation/kernel-doc-nano-HOWTO.txt.
Right, fixed it. I never really understood why we don't document the
prototype, since that's usually what the user of a function will look at
first.
I have also addressed all the typos and naming issues you mentioned.
Thanks!
Alex.
>
> Thierry
>
>> +
>> +/**
>> + * Run the given power sequence. Returns 0 on success, error code in case of
>> + * failure.
>> + */
>> +int power_seq_run(struct power_seq *seq);
>> +
>> +#endif
>> --
>> 1.7.11.4
>>
>>
>>
>
> * Unknown Key
> * 0x7F3EB3A1
>
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-08-16 9:52 UTC (permalink / raw)
To: Alex Courbot
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann,
Leela Krishna Amudala, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org
In-Reply-To: <502CBB0C.70102@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 6392 bytes --]
On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
> On 08/16/2012 04:42 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
[...]
> >>+Usage by Drivers and Resources Management
> >>+-----------------------------------------
> >>+Power sequences make use of resources that must be properly allocated and
> >>+managed. The power_seq_build() function builds a power sequence from the
> >>+platform data. It also takes care of resolving and allocating the resources
> >>+referenced by the sequence if needed:
> >>+
> >>+ struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> >>+ struct platform_power_seq *pseq);
> >>+
> >>+The 'dev' argument is the device in the name of which the resources are to be
> >>+allocated.
> >>+
> >>+The 'ress' argument is a list to which the resolved resources are appended. This
> >>+avoids allocating a resource referenced in several power sequences multiple
> >>+times.
> >>+
> >>+On success, the function returns a devm allocated resolved sequence that is
> >>+ready to be passed to power_seq_run(). In case of failure, and error code is
> >>+returned.
> >>+
> >>+A resolved power sequence returned by power_seq_build can be run by
> >>+power_run_run():
> >>+
> >>+ int power_seq_run(power_seq *seq);
> >>+
> >>+It returns 0 if the sequence has successfully been run, or an error code if a
> >>+problem occured.
> >>+
> >>+There is no need to explicitly free the resources used by the sequence as they
> >>+are devm-allocated.
> >
> >I had some comments about this particular interface for creating
> >sequences in the last series. My point was that explicitly requiring
> >drivers to manage a list of already allocated resources may be too much
> >added complexity. Power sequences should be easy to use, and I find the
> >requirement for a separately managed list of resources cumbersome.
> >
> >What I proposed last time was to collect all power sequences under a
> >common parent object, which in turn would take care of managing the
> >resources.
>
> Yes, I remember that. While I see why you don't like this list,
> having a common parent object to all sequences will not reduce the
> number of arguments to pass to power_seq_build() (which is the only
> function that has to handle this list now). Also having the list of
> resources at hand is needed for some drivers: for instance,
> pwm-backlight needs to check that exactly one PWM has been
> allocated, and takes a reference to it from this list in order to
> control the brightness.
I'm not complaining about the additional argument to power_seq_build()
but about the missing encapsulation. I just think that keeping a list
external to the power sequencing code is error-prone. Drivers could do
just about anything with it between calls to power_seq_build(). If you
do all of this internally, then you don't depend on the driver at all
and power sequencing code can just do the right thing.
Obtaining a reference to the PWM, or any other resource for that matter,
from the power sequence could be done via an explicit API.
> Ideally we could embed the list into the device structure, but I
> don't see how we can do that without modifying it (and we don't want
> to modify it). Another solution would be to keep a static mapping
> table that associates a device to its power_seq related resources
> within power_seq.c. If we protect it for concurrent access this
> should make it possible to make resources management transparent.
> How does this sound? Only drawback I see is that we would need to
> explicitly clean it up through a dedicated function when the driver
> exits.
I don't think that's much better. Since the power sequences will be very
tightly coupled to a specific device, tying the sequences and their
resources to the device makes a lot of sense. Keeping a global list of
resources doesn't in my opinion.
> >>+static int power_seq_step_run(struct power_seq_step *step)
> >>+{
> >>+ struct platform_power_seq_step *pdata = &step->pdata;
> >>+ int err = 0;
> >>+
> >>+ switch (pdata->type) {
> >>+ case POWER_SEQ_DELAY:
> >>+ usleep_range(pdata->delay.delay_us,
> >>+ pdata->delay.delay_us + 1000);
> >>+ break;
> >>+#ifdef CONFIG_REGULATOR
> >>+ case POWER_SEQ_REGULATOR:
> >>+ if (pdata->regulator.enable)
> >>+ err = regulator_enable(step->resource->regulator);
> >>+ else
> >>+ err = regulator_disable(step->resource->regulator);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_PWM
> >>+ case POWER_SEQ_PWM:
> >>+ if (pdata->gpio.enable)
> >>+ err = pwm_enable(step->resource->pwm);
> >>+ else
> >>+ pwm_disable(step->resource->pwm);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_GPIOLIB
> >>+ case POWER_SEQ_GPIO:
> >>+ gpio_set_value_cansleep(pdata->gpio.gpio, pdata->gpio.enable);
> >>+ break;
> >>+#endif
> >>+ /*
> >>+ * should never happen unless the sequence includes a step which
> >>+ * type does not have support compiled in
> >
> >I think this should be "whose type"? I also remember commenting on the
> >whole #ifdef'ery here. I really don't think it is necessary. At least
> >for regulators I know that the functions can be used even if the
> >subsystem itself isn't supported. The same seems to hold for GPIO and we
> >can probably add something similar for PWM.
>
> Actually I kept them because I don't really like the empty function
> definitions in the regulator framework. They all return 0 as if the
> function completed successfully - here we should at least warn the
> user that proper support for that resource is missing.
>
> >
> >It might also be a good idea to just skip unsupported resource types
> >when the sequence is built, accompanied by runtime warnings that the
> >type is not supported.
>
> Agreed.
If you do this, then I think the above #ifdef'ery becomes obsolete
because any errors that could potentially be hidden have already been
caught when the list was built.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v4] fbdev: sh_mobile_lcdc: use dma_mmap_coherent
From: Hideki EIRAKU @ 2012-08-16 10:13 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: linux-fbdev, linux-kernel, Marek Szyprowski, Laurent Pinchart,
Katsuya MATSUBARA, Damian Hobson-Garcia, Hideki EIRAKU
fb_mmap() implemented in fbmem.c uses smem_start as the physical
address of the frame buffer. In the sh_mobile_lcdc driver, the
smem_start is a dma_addr_t that is not a physical address when IOMMU is
enabled. dma_mmap_coherent() maps the address correctly.
Signed-off-by: Hideki EIRAKU <hdk@igel.co.jp>
---
drivers/video/sh_mobile_lcdcfb.c | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
index 699487c..bccfd7e 100644
--- a/drivers/video/sh_mobile_lcdcfb.c
+++ b/drivers/video/sh_mobile_lcdcfb.c
@@ -1614,6 +1614,15 @@ static int sh_mobile_lcdc_overlay_blank(int blank, struct fb_info *info)
return 1;
}
+static int
+sh_mobile_lcdc_overlay_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+ struct sh_mobile_lcdc_overlay *ovl = info->par;
+
+ return dma_mmap_coherent(ovl->channel->lcdc->dev, vma, ovl->fb_mem,
+ ovl->dma_handle, ovl->fb_size);
+}
+
static struct fb_ops sh_mobile_lcdc_overlay_ops = {
.owner = THIS_MODULE,
.fb_read = fb_sys_read,
@@ -1626,6 +1635,7 @@ static struct fb_ops sh_mobile_lcdc_overlay_ops = {
.fb_ioctl = sh_mobile_lcdc_overlay_ioctl,
.fb_check_var = sh_mobile_lcdc_overlay_check_var,
.fb_set_par = sh_mobile_lcdc_overlay_set_par,
+ .fb_mmap = sh_mobile_lcdc_overlay_mmap,
};
static void
@@ -2093,6 +2103,15 @@ static int sh_mobile_lcdc_blank(int blank, struct fb_info *info)
return 0;
}
+static int
+sh_mobile_lcdc_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+ struct sh_mobile_lcdc_chan *ch = info->par;
+
+ return dma_mmap_coherent(ch->lcdc->dev, vma, ch->fb_mem,
+ ch->dma_handle, ch->fb_size);
+}
+
static struct fb_ops sh_mobile_lcdc_ops = {
.owner = THIS_MODULE,
.fb_setcolreg = sh_mobile_lcdc_setcolreg,
@@ -2108,6 +2127,7 @@ static struct fb_ops sh_mobile_lcdc_ops = {
.fb_release = sh_mobile_lcdc_release,
.fb_check_var = sh_mobile_lcdc_check_var,
.fb_set_par = sh_mobile_lcdc_set_par,
+ .fb_mmap = sh_mobile_lcdc_mmap,
};
static void
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH v3 3/4] media: videobuf2-dma-contig: use dma_mmap_coherent if available
From: Hideki EIRAKU @ 2012-08-16 10:13 UTC (permalink / raw)
To: m.szyprowski
Cc: laurent.pinchart, linux, pawel, kyungmin.park, mchehab,
FlorianSchandinat, perex, tiwai, t.stanislaws, linux-arm-kernel,
linux-kernel, linux-media, linux-fbdev, alsa-devel, matsu,
dhobsong
In-Reply-To: <012701cd74ac$6a617060$3f245120$%szyprowski@samsung.com>
Hello,
From: Marek Szyprowski <m.szyprowski@samsung.com>
Subject: RE: [PATCH v3 3/4] media: videobuf2-dma-contig: use dma_mmap_coherent if available
Date: Tue, 07 Aug 2012 16:53:25 +0200
> I'm sorry for bringing this issue now, once you have already created v3 of your
> patches, but similar patch has been already proposed some time ago. It is already
> processed together with general videobuf2-dma-contig redesign and dma-buf extensions
> by Tomasz Stanislawski.
>
> See post http://thread.gmane.org/gmane.comp.video.dri.devel/70402/focusI461 and
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/49438
>
> It doesn't use conditional code inside videobuf2 allocator and rely entirely on
> dma-mapping subsystem to provide a working dma_mmap_coherent/writecombine/attrs()
> function. When it was posted, it relied on the dma-mapping extensions, which now
> have been finally merged to v3.6-rc1. Now I wonder if there are any architectures,
> which don't use dma_map_ops based dma-mapping framework, which might use
> videobuf2-dma-conting module.
Thank you for telling me about videobuf2-dma-contig and v3.6-rc1. The
videobuf2-dma-contig patch I sent is now unnecessary. So I will
remove the patch. I will remove the patch defining
ARCH_HAS_DMA_MMAP_COHERENT too because the v3.6-rc1 kernel has generic
dma_mmap_coherent() API for every architecture.
I will also remove the Laurent's patch I sent because it was related
to ARCH_HAS_DMA_MMAP_COHERENT.
The remaining patch is sh_mobile_lcdc. I will remove ifdefs from the
patch and re-post it as a patch v4.
--
Hideki EIRAKU <hdk@igel.co.jp>
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-08-16 10:33 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann,
Leela Krishna Amudala, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org
In-Reply-To: <20120816095251.GA30646@avionic-0098.mockup.avionic-design.de>
On 08/16/2012 06:52 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
>> On 08/16/2012 04:42 PM, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> [...]
>>>> +Usage by Drivers and Resources Management
>>>> +-----------------------------------------
>>>> +Power sequences make use of resources that must be properly allocated and
>>>> +managed. The power_seq_build() function builds a power sequence from the
>>>> +platform data. It also takes care of resolving and allocating the resources
>>>> +referenced by the sequence if needed:
>>>> +
>>>> + struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
>>>> + struct platform_power_seq *pseq);
>>>> +
>>>> +The 'dev' argument is the device in the name of which the resources are to be
>>>> +allocated.
>>>> +
>>>> +The 'ress' argument is a list to which the resolved resources are appended. This
>>>> +avoids allocating a resource referenced in several power sequences multiple
>>>> +times.
>>>> +
>>>> +On success, the function returns a devm allocated resolved sequence that is
>>>> +ready to be passed to power_seq_run(). In case of failure, and error code is
>>>> +returned.
>>>> +
>>>> +A resolved power sequence returned by power_seq_build can be run by
>>>> +power_run_run():
>>>> +
>>>> + int power_seq_run(power_seq *seq);
>>>> +
>>>> +It returns 0 if the sequence has successfully been run, or an error code if a
>>>> +problem occured.
>>>> +
>>>> +There is no need to explicitly free the resources used by the sequence as they
>>>> +are devm-allocated.
>>>
>>> I had some comments about this particular interface for creating
>>> sequences in the last series. My point was that explicitly requiring
>>> drivers to manage a list of already allocated resources may be too much
>>> added complexity. Power sequences should be easy to use, and I find the
>>> requirement for a separately managed list of resources cumbersome.
>>>
>>> What I proposed last time was to collect all power sequences under a
>>> common parent object, which in turn would take care of managing the
>>> resources.
>>
>> Yes, I remember that. While I see why you don't like this list,
>> having a common parent object to all sequences will not reduce the
>> number of arguments to pass to power_seq_build() (which is the only
>> function that has to handle this list now). Also having the list of
>> resources at hand is needed for some drivers: for instance,
>> pwm-backlight needs to check that exactly one PWM has been
>> allocated, and takes a reference to it from this list in order to
>> control the brightness.
>
> I'm not complaining about the additional argument to power_seq_build()
> but about the missing encapsulation. I just think that keeping a list
> external to the power sequencing code is error-prone. Drivers could do
> just about anything with it between calls to power_seq_build(). If you
> do all of this internally, then you don't depend on the driver at all
> and power sequencing code can just do the right thing.
On the opposite side, I am concerned about over-encapsulation. :) IIRC
you proposed to have a top structure to hold the power sequences, their
resources and the associated device. Power sequences would then have a
name and be run through a 2 arguments power_seq_run():
power_seq_run(sequences, "up");
There are two things that bother me with this solution. First is that
addressing power sequences by name looks a little bit overkill, when a
single pointer should be enough. It would also complicate the design.
Second thing is that this design would place the power sequences
structure on top of the device - in effect, you could perfectly have
several of these structures all using the same device and failing to see
each other's resources. While that would be a error from the device
driver's side, the design allows it.
>
> Obtaining a reference to the PWM, or any other resource for that matter,
> from the power sequence could be done via an explicit API.
>
>> Ideally we could embed the list into the device structure, but I
>> don't see how we can do that without modifying it (and we don't want
>> to modify it). Another solution would be to keep a static mapping
>> table that associates a device to its power_seq related resources
>> within power_seq.c. If we protect it for concurrent access this
>> should make it possible to make resources management transparent.
>> How does this sound? Only drawback I see is that we would need to
>> explicitly clean it up through a dedicated function when the driver
>> exits.
>
> I don't think that's much better. Since the power sequences will be very
> tightly coupled to a specific device, tying the sequences and their
> resources to the device makes a lot of sense. Keeping a global list of
> resources doesn't in my opinion.
That is not what would happen actually - what I proposed is to have a
mapping (hash map, or more likely binary tree) between a device and the
list_head of the resources for that device. In C++ (forgive me, this
makes the types more explicit) that would be:
static std::map<struct device *, struct list_head> device_resources;
That way you would have exactly one list per device, could keep
resource-management totally transparent without exposing the list_head,
and keep the API and design simple.
For special cases (like pwm-backlight which needs to get the PWM), the
list_head could be obtained through a dedicated API.
Alex.
^ permalink raw reply
* RE: [PATCH v3 3/4] media: videobuf2-dma-contig: use dma_mmap_coherent if available
From: Marek Szyprowski @ 2012-08-16 10:39 UTC (permalink / raw)
To: 'Hideki EIRAKU'
Cc: laurent.pinchart, linux, pawel, kyungmin.park, mchehab,
FlorianSchandinat, perex, tiwai, Tomasz Stanislawski,
linux-arm-kernel, linux-kernel, linux-media, linux-fbdev,
alsa-devel, matsu, dhobsong
In-Reply-To: <20120816.191358.127675610.hdk@igel.co.jp>
Hello,
On Thursday, August 16, 2012 12:14 PM Hideki EIRAKU wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> Subject: RE: [PATCH v3 3/4] media: videobuf2-dma-contig: use dma_mmap_coherent if available
> Date: Tue, 07 Aug 2012 16:53:25 +0200
>
> > I'm sorry for bringing this issue now, once you have already created v3 of your
> > patches, but similar patch has been already proposed some time ago. It is already
> > processed together with general videobuf2-dma-contig redesign and dma-buf extensions
> > by Tomasz Stanislawski.
> >
> > See post http://thread.gmane.org/gmane.comp.video.dri.devel/70402/focusI461 and
> > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/49438
> >
> > It doesn't use conditional code inside videobuf2 allocator and rely entirely on
> > dma-mapping subsystem to provide a working dma_mmap_coherent/writecombine/attrs()
> > function. When it was posted, it relied on the dma-mapping extensions, which now
> > have been finally merged to v3.6-rc1. Now I wonder if there are any architectures,
> > which don't use dma_map_ops based dma-mapping framework, which might use
> > videobuf2-dma-conting module.
>
> Thank you for telling me about videobuf2-dma-contig and v3.6-rc1. The
> videobuf2-dma-contig patch I sent is now unnecessary. So I will
> remove the patch. I will remove the patch defining
> ARCH_HAS_DMA_MMAP_COHERENT too because the v3.6-rc1 kernel has generic
> dma_mmap_coherent() API for every architecture.
Just to let you know - Tomasz has posted an updated version of the dma-buf/vb2-dma-contig
patches:
http://www.spinics.net/lists/linux-media/msg51768.html
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-08-16 10:52 UTC (permalink / raw)
To: Alex Courbot
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Mark Brown, Anton Vorontsov, David Woodhouse, Arnd Bergmann,
Leela Krishna Amudala, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org
In-Reply-To: <502CCC77.2010005@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 6396 bytes --]
On Thu, Aug 16, 2012 at 07:33:27PM +0900, Alex Courbot wrote:
> On 08/16/2012 06:52 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
> >>On 08/16/2012 04:42 PM, Thierry Reding wrote:
> >>>>Old Signed by an unknown key
> >>>
> >>>On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> >[...]
> >>>>+Usage by Drivers and Resources Management
> >>>>+-----------------------------------------
> >>>>+Power sequences make use of resources that must be properly allocated and
> >>>>+managed. The power_seq_build() function builds a power sequence from the
> >>>>+platform data. It also takes care of resolving and allocating the resources
> >>>>+referenced by the sequence if needed:
> >>>>+
> >>>>+ struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> >>>>+ struct platform_power_seq *pseq);
> >>>>+
> >>>>+The 'dev' argument is the device in the name of which the resources are to be
> >>>>+allocated.
> >>>>+
> >>>>+The 'ress' argument is a list to which the resolved resources are appended. This
> >>>>+avoids allocating a resource referenced in several power sequences multiple
> >>>>+times.
> >>>>+
> >>>>+On success, the function returns a devm allocated resolved sequence that is
> >>>>+ready to be passed to power_seq_run(). In case of failure, and error code is
> >>>>+returned.
> >>>>+
> >>>>+A resolved power sequence returned by power_seq_build can be run by
> >>>>+power_run_run():
> >>>>+
> >>>>+ int power_seq_run(power_seq *seq);
> >>>>+
> >>>>+It returns 0 if the sequence has successfully been run, or an error code if a
> >>>>+problem occured.
> >>>>+
> >>>>+There is no need to explicitly free the resources used by the sequence as they
> >>>>+are devm-allocated.
> >>>
> >>>I had some comments about this particular interface for creating
> >>>sequences in the last series. My point was that explicitly requiring
> >>>drivers to manage a list of already allocated resources may be too much
> >>>added complexity. Power sequences should be easy to use, and I find the
> >>>requirement for a separately managed list of resources cumbersome.
> >>>
> >>>What I proposed last time was to collect all power sequences under a
> >>>common parent object, which in turn would take care of managing the
> >>>resources.
> >>
> >>Yes, I remember that. While I see why you don't like this list,
> >>having a common parent object to all sequences will not reduce the
> >>number of arguments to pass to power_seq_build() (which is the only
> >>function that has to handle this list now). Also having the list of
> >>resources at hand is needed for some drivers: for instance,
> >>pwm-backlight needs to check that exactly one PWM has been
> >>allocated, and takes a reference to it from this list in order to
> >>control the brightness.
> >
> >I'm not complaining about the additional argument to power_seq_build()
> >but about the missing encapsulation. I just think that keeping a list
> >external to the power sequencing code is error-prone. Drivers could do
> >just about anything with it between calls to power_seq_build(). If you
> >do all of this internally, then you don't depend on the driver at all
> >and power sequencing code can just do the right thing.
>
> On the opposite side, I am concerned about over-encapsulation. :)
> IIRC you proposed to have a top structure to hold the power
> sequences, their resources and the associated device. Power
> sequences would then have a name and be run through a 2 arguments
> power_seq_run():
>
> power_seq_run(sequences, "up");
>
> There are two things that bother me with this solution. First is
> that addressing power sequences by name looks a little bit overkill,
> when a single pointer should be enough. It would also complicate the
> design. Second thing is that this design would place the power
> sequences structure on top of the device - in effect, you could
> perfectly have several of these structures all using the same device
> and failing to see each other's resources. While that would be a
> error from the device driver's side, the design allows it.
I see. Perhaps I'm just bugged by the interface being a simple list. If
it was something just a little more sophisticated, like a very primitive
resource manager attached to one device, I would be appeased. Maybe an
opaque structure that carries the list and hides it for drivers would do
as well.
> >Obtaining a reference to the PWM, or any other resource for that matter,
> >from the power sequence could be done via an explicit API.
> >
> >>Ideally we could embed the list into the device structure, but I
> >>don't see how we can do that without modifying it (and we don't want
> >>to modify it). Another solution would be to keep a static mapping
> >>table that associates a device to its power_seq related resources
> >>within power_seq.c. If we protect it for concurrent access this
> >>should make it possible to make resources management transparent.
> >>How does this sound? Only drawback I see is that we would need to
> >>explicitly clean it up through a dedicated function when the driver
> >>exits.
> >
> >I don't think that's much better. Since the power sequences will be very
> >tightly coupled to a specific device, tying the sequences and their
> >resources to the device makes a lot of sense. Keeping a global list of
> >resources doesn't in my opinion.
>
> That is not what would happen actually - what I proposed is to have
> a mapping (hash map, or more likely binary tree) between a device
> and the list_head of the resources for that device. In C++ (forgive
> me, this makes the types more explicit) that would be:
>
> static std::map<struct device *, struct list_head> device_resources;
>
> That way you would have exactly one list per device, could keep
> resource-management totally transparent without exposing the
> list_head, and keep the API and design simple.
>
> For special cases (like pwm-backlight which needs to get the PWM),
> the list_head could be obtained through a dedicated API.
I understand. You could use an idr (include/linux/idr.h) for this
purpose. However I don't know if this would be any better than the
above.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/6] OMAPDSS: DSI: Maintain copy of operation mode in driver data
From: Tomi Valkeinen @ 2012-08-16 11:19 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345102594-6222-2-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]
On Thu, 2012-08-16 at 13:06 +0530, Archit Taneja wrote:
> The DSI driver currently relies on the omap_dss_device struct to know the mode
> of operation of the DSI protocol(command or video mode). This makes the DSI
> interface driver dependent on the omap_dss_device struct.
>
> Make the DSI driver data maintain it's own operation mode field. The panel
> driver is expected to call omapdss_dsi_set_operation_mode() before the interface
> is enabled.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/displays/panel-taal.c | 1 +
> drivers/video/omap2/dss/dsi.c | 42 +++++++++++++++++++++--------
> include/video/omapdss.h | 2 ++
> 3 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
> index d220f19..649247f 100644
> --- a/drivers/video/omap2/displays/panel-taal.c
> +++ b/drivers/video/omap2/displays/panel-taal.c
> @@ -1063,6 +1063,7 @@ static int taal_power_on(struct omap_dss_device *dssdev)
> omapdss_dsi_set_size(dssdev, dssdev->panel.timings.x_res,
> dssdev->panel.timings.y_res);
> omapdss_dsi_set_pixel_format(dssdev, dssdev->panel.dsi_pix_fmt);
> + omapdss_dsi_set_operation_mode(dssdev, dssdev->panel.dsi_mode);
Taal is always in cmd mode. Shouldn't we just use a hardcoded value
here?
Actually I think the same goes for the pix_fmt above. It's always the
same for Taal.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH V4 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Chandrabhanu Mahapatra @ 2012-08-16 11:30 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <1344859135-4470-1-git-send-email-cmahapatra@ti.com>
All the cpu_is checks have been moved to dispc_init_features function providing
a much more generic and cleaner interface. The OMAP version and revision
specific functions and data are initialized by dispc_features structure which is
local to dispc.c.
Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
drivers/video/omap2/dss/dispc.c | 428 +++++++++++++++++++++++++--------------
1 file changed, 273 insertions(+), 155 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 5b289c5..0415845 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -81,6 +81,23 @@ struct dispc_irq_stats {
unsigned irqs[32];
};
+struct dispc_features {
+ int hp_max;
+ int vp_max;
+ int sw_max;
+ int sw_start;
+ int fp_start;
+ int bp_start;
+ int (*calc_scaling) (enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings,
+ u16 width, u16 height, u16 out_width, u16 out_height,
+ enum omap_color_mode color_mode, bool *five_taps,
+ int *x_predecim, int *y_predecim, int *decim_x, int *decim_y,
+ u16 pos_x, unsigned long *core_clk);
+ unsigned long (*calc_core_clk) (enum omap_channel channel,
+ u16 width, u16 height, u16 out_width, u16 out_height);
+};
+
static struct {
struct platform_device *pdev;
void __iomem *base;
@@ -101,6 +118,8 @@ static struct {
bool ctx_valid;
u32 ctx[DISPC_SZ_REGS / sizeof(u32)];
+ const struct dispc_features *feat;
+
#ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
spinlock_t irq_stats_lock;
struct dispc_irq_stats irq_stats;
@@ -1939,7 +1958,18 @@ static unsigned long calc_core_clk_five_taps(enum omap_channel channel,
return core_clk;
}
-static unsigned long calc_core_clk(enum omap_channel channel, u16 width,
+static unsigned long calc_core_clk_24xx(enum omap_channel channel, u16 width,
+ u16 height, u16 out_width, u16 out_height)
+{
+ unsigned long pclk = dispc_mgr_pclk_rate(channel);
+
+ if (height > out_height && width > out_width)
+ return pclk * 4;
+ else
+ return pclk * 2;
+}
+
+static unsigned long calc_core_clk_34xx(enum omap_channel channel, u16 width,
u16 height, u16 out_width, u16 out_height)
{
unsigned int hf, vf;
@@ -1958,25 +1988,163 @@ static unsigned long calc_core_clk(enum omap_channel channel, u16 width,
hf = 2;
else
hf = 1;
-
if (height > out_height)
vf = 2;
else
vf = 1;
- if (cpu_is_omap24xx()) {
- if (vf > 1 && hf > 1)
- return pclk * 4;
- else
- return pclk * 2;
- } else if (cpu_is_omap34xx()) {
- return pclk * vf * hf;
- } else {
- if (hf > 1)
- return DIV_ROUND_UP(pclk, out_width) * width;
- else
- return pclk;
+ return pclk * vf * hf;
+}
+
+static unsigned long calc_core_clk_44xx(enum omap_channel channel, u16 width,
+ u16 height, u16 out_width, u16 out_height)
+{
+ unsigned long pclk = dispc_mgr_pclk_rate(channel);
+
+ if (width > out_width)
+ return DIV_ROUND_UP(pclk, out_width) * width;
+ else
+ return pclk;
+}
+
+static int dispc_ovl_calc_scaling_24xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings,
+ u16 width, u16 height, u16 out_width, u16 out_height,
+ enum omap_color_mode color_mode, bool *five_taps,
+ int *x_predecim, int *y_predecim, int *decim_x, int *decim_y,
+ u16 pos_x, unsigned long *core_clk)
+{
+ int error;
+ u16 in_width, in_height;
+ int min_factor = min(*decim_x, *decim_y);
+ const int maxsinglelinewidth + dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
+ *five_taps = false;
+
+ do {
+ in_height = DIV_ROUND_UP(height, *decim_y);
+ in_width = DIV_ROUND_UP(width, *decim_x);
+ *core_clk = dispc.feat->calc_core_clk(channel, in_width,
+ in_height, out_width, out_height);
+ error = (in_width > maxsinglelinewidth || !*core_clk ||
+ *core_clk > dispc_core_clk_rate());
+ if (error) {
+ if (*decim_x = *decim_y) {
+ *decim_x = min_factor;
+ ++*decim_y;
+ } else {
+ swap(*decim_x, *decim_y);
+ if (*decim_x < *decim_y)
+ ++*decim_x;
+ }
+ }
+ } while (*decim_x <= *x_predecim && *decim_y <= *y_predecim && error);
+
+ if (in_width > maxsinglelinewidth) {
+ DSSERR("Cannot scale max input width exceeded");
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int dispc_ovl_calc_scaling_34xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings,
+ u16 width, u16 height, u16 out_width, u16 out_height,
+ enum omap_color_mode color_mode, bool *five_taps,
+ int *x_predecim, int *y_predecim, int *decim_x, int *decim_y,
+ u16 pos_x, unsigned long *core_clk)
+{
+ int error;
+ u16 in_width, in_height;
+ int min_factor = min(*decim_x, *decim_y);
+ const int maxsinglelinewidth + dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
+
+ do {
+ in_height = DIV_ROUND_UP(height, *decim_y);
+ in_width = DIV_ROUND_UP(width, *decim_x);
+ *core_clk = calc_core_clk_five_taps(channel, mgr_timings,
+ in_width, in_height, out_width, out_height, color_mode);
+
+ error = check_horiz_timing_omap3(channel, mgr_timings, pos_x,
+ in_width, in_height, out_width, out_height);
+
+ if (in_width > maxsinglelinewidth)
+ if (in_height > out_height &&
+ in_height < out_height * 2)
+ *five_taps = false;
+ if (!*five_taps)
+ *core_clk = dispc.feat->calc_core_clk(channel, in_width,
+ in_height, out_width, out_height);
+
+ error = (error || in_width > maxsinglelinewidth * 2 ||
+ (in_width > maxsinglelinewidth && *five_taps) ||
+ !*core_clk || *core_clk > dispc_core_clk_rate());
+ if (error) {
+ if (*decim_x = *decim_y) {
+ *decim_x = min_factor;
+ ++*decim_y;
+ } else {
+ swap(*decim_x, *decim_y);
+ if (*decim_x < *decim_y)
+ ++*decim_x;
+ }
+ }
+ } while (*decim_x <= *x_predecim && *decim_y <= *y_predecim && error);
+
+ if (check_horiz_timing_omap3(channel, mgr_timings, pos_x, width, height,
+ out_width, out_height)){
+ DSSERR("horizontal timing too tight\n");
+ return -EINVAL;
}
+
+ if (in_width > (maxsinglelinewidth * 2)) {
+ DSSERR("Cannot setup scaling");
+ DSSERR("width exceeds maximum width possible");
+ return -EINVAL;
+ }
+
+ if (in_width > maxsinglelinewidth && *five_taps) {
+ DSSERR("cannot setup scaling with five taps");
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int dispc_ovl_calc_scaling_44xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings,
+ u16 width, u16 height, u16 out_width, u16 out_height,
+ enum omap_color_mode color_mode, bool *five_taps,
+ int *x_predecim, int *y_predecim, int *decim_x, int *decim_y,
+ u16 pos_x, unsigned long *core_clk)
+{
+ u16 in_width, in_width_max;
+ int decim_x_min = *decim_x;
+ u16 in_height = DIV_ROUND_UP(height, *decim_y);
+ const int maxsinglelinewidth + dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
+
+ in_width_max = dispc_core_clk_rate() /
+ DIV_ROUND_UP(dispc_mgr_pclk_rate(channel), out_width);
+ *decim_x = DIV_ROUND_UP(width, in_width_max);
+
+ *decim_x = *decim_x > decim_x_min ? *decim_x : decim_x_min;
+ if (*decim_x > *x_predecim)
+ return -EINVAL;
+
+ do {
+ in_width = DIV_ROUND_UP(width, *decim_x);
+ } while (*decim_x <= *x_predecim &&
+ in_width > maxsinglelinewidth && ++*decim_x);
+
+ if (in_width > maxsinglelinewidth) {
+ DSSERR("Cannot scale width exceeds max line width");
+ return -EINVAL;
+ }
+
+ *core_clk = dispc.feat->calc_core_clk(channel, in_width, in_height,
+ out_width, out_height);
+ return 0;
}
static int dispc_ovl_calc_scaling(enum omap_plane plane,
@@ -1988,12 +2156,9 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
{
struct omap_overlay *ovl = omap_dss_get_overlay(plane);
const int maxdownscale = dss_feat_get_param_max(FEAT_PARAM_DOWNSCALE);
- const int maxsinglelinewidth - dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
const int max_decim_limit = 16;
unsigned long core_clk = 0;
- int decim_x, decim_y, error, min_factor;
- u16 in_width, in_height, in_width_max = 0;
+ int decim_x, decim_y, ret;
if (width = out_width && height = out_height)
return 0;
@@ -2017,118 +2182,17 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
decim_x = DIV_ROUND_UP(DIV_ROUND_UP(width, out_width), maxdownscale);
decim_y = DIV_ROUND_UP(DIV_ROUND_UP(height, out_height), maxdownscale);
- min_factor = min(decim_x, decim_y);
-
if (decim_x > *x_predecim || out_width > width * 8)
return -EINVAL;
if (decim_y > *y_predecim || out_height > height * 8)
return -EINVAL;
- if (cpu_is_omap24xx()) {
- *five_taps = false;
-
- do {
- in_height = DIV_ROUND_UP(height, decim_y);
- in_width = DIV_ROUND_UP(width, decim_x);
- core_clk = calc_core_clk(channel, in_width, in_height,
- out_width, out_height);
- error = (in_width > maxsinglelinewidth || !core_clk ||
- core_clk > dispc_core_clk_rate());
- if (error) {
- if (decim_x = decim_y) {
- decim_x = min_factor;
- decim_y++;
- } else {
- swap(decim_x, decim_y);
- if (decim_x < decim_y)
- decim_x++;
- }
- }
- } while (decim_x <= *x_predecim && decim_y <= *y_predecim &&
- error);
-
- if (in_width > maxsinglelinewidth) {
- DSSERR("Cannot scale max input width exceeded");
- return -EINVAL;
- }
- } else if (cpu_is_omap34xx()) {
-
- do {
- in_height = DIV_ROUND_UP(height, decim_y);
- in_width = DIV_ROUND_UP(width, decim_x);
- core_clk = calc_core_clk_five_taps(channel, mgr_timings,
- in_width, in_height, out_width, out_height,
- color_mode);
-
- error = check_horiz_timing_omap3(channel, mgr_timings,
- pos_x, in_width, in_height, out_width,
- out_height);
-
- if (in_width > maxsinglelinewidth)
- if (in_height > out_height &&
- in_height < out_height * 2)
- *five_taps = false;
- if (!*five_taps)
- core_clk = calc_core_clk(channel, in_width,
- in_height, out_width, out_height);
- error = (error || in_width > maxsinglelinewidth * 2 ||
- (in_width > maxsinglelinewidth && *five_taps) ||
- !core_clk || core_clk > dispc_core_clk_rate());
- if (error) {
- if (decim_x = decim_y) {
- decim_x = min_factor;
- decim_y++;
- } else {
- swap(decim_x, decim_y);
- if (decim_x < decim_y)
- decim_x++;
- }
- }
- } while (decim_x <= *x_predecim && decim_y <= *y_predecim
- && error);
-
- if (check_horiz_timing_omap3(channel, mgr_timings, pos_x, width,
- height, out_width, out_height)){
- DSSERR("horizontal timing too tight\n");
- return -EINVAL;
- }
-
- if (in_width > (maxsinglelinewidth * 2)) {
- DSSERR("Cannot setup scaling");
- DSSERR("width exceeds maximum width possible");
- return -EINVAL;
- }
-
- if (in_width > maxsinglelinewidth && *five_taps) {
- DSSERR("cannot setup scaling with five taps");
- return -EINVAL;
- }
- } else {
- int decim_x_min = decim_x;
- in_height = DIV_ROUND_UP(height, decim_y);
- in_width_max = dispc_core_clk_rate() /
- DIV_ROUND_UP(dispc_mgr_pclk_rate(channel),
- out_width);
- decim_x = DIV_ROUND_UP(width, in_width_max);
-
- decim_x = decim_x > decim_x_min ? decim_x : decim_x_min;
- if (decim_x > *x_predecim)
- return -EINVAL;
-
- do {
- in_width = DIV_ROUND_UP(width, decim_x);
- } while (decim_x <= *x_predecim &&
- in_width > maxsinglelinewidth && decim_x++);
-
- if (in_width > maxsinglelinewidth) {
- DSSERR("Cannot scale width exceeds max line width");
- return -EINVAL;
- }
-
- core_clk = calc_core_clk(channel, in_width, in_height,
- out_width, out_height);
- }
+ ret = dispc.feat->calc_scaling(channel, mgr_timings, width, height,
+ out_width, out_height, color_mode, five_taps, x_predecim,
+ y_predecim, &decim_x, &decim_y, pos_x, &core_clk);
+ if (ret)
+ return ret;
DSSDBG("required core clk rate = %lu Hz\n", core_clk);
DSSDBG("current core clk rate = %lu Hz\n", dispc_core_clk_rate());
@@ -2604,24 +2668,13 @@ static bool _dispc_mgr_size_ok(u16 width, u16 height)
static bool _dispc_lcd_timings_ok(int hsw, int hfp, int hbp,
int vsw, int vfp, int vbp)
{
- if (cpu_is_omap24xx() || omap_rev() < OMAP3430_REV_ES3_0) {
- if (hsw < 1 || hsw > 64 ||
- hfp < 1 || hfp > 256 ||
- hbp < 1 || hbp > 256 ||
- vsw < 1 || vsw > 64 ||
- vfp < 0 || vfp > 255 ||
- vbp < 0 || vbp > 255)
- return false;
- } else {
- if (hsw < 1 || hsw > 256 ||
- hfp < 1 || hfp > 4096 ||
- hbp < 1 || hbp > 4096 ||
- vsw < 1 || vsw > 256 ||
- vfp < 0 || vfp > 4095 ||
- vbp < 0 || vbp > 4095)
- return false;
- }
-
+ if (hsw < 1 || hsw > dispc.feat->sw_max ||
+ hfp < 1 || hfp > dispc.feat->hp_max ||
+ hbp < 1 || hbp > dispc.feat->hp_max ||
+ vsw < 1 || vsw > dispc.feat->sw_max ||
+ vfp < 0 || vfp > dispc.feat->vp_max ||
+ vbp < 0 || vbp > dispc.feat->vp_max)
+ return false;
return true;
}
@@ -2653,19 +2706,12 @@ static void _dispc_mgr_set_lcd_timings(enum omap_channel channel, int hsw,
u32 timing_h, timing_v, l;
bool onoff, rf, ipc;
- if (cpu_is_omap24xx() || omap_rev() < OMAP3430_REV_ES3_0) {
- timing_h = FLD_VAL(hsw-1, 5, 0) | FLD_VAL(hfp-1, 15, 8) |
- FLD_VAL(hbp-1, 27, 20);
-
- timing_v = FLD_VAL(vsw-1, 5, 0) | FLD_VAL(vfp, 15, 8) |
- FLD_VAL(vbp, 27, 20);
- } else {
- timing_h = FLD_VAL(hsw-1, 7, 0) | FLD_VAL(hfp-1, 19, 8) |
- FLD_VAL(hbp-1, 31, 20);
-
- timing_v = FLD_VAL(vsw-1, 7, 0) | FLD_VAL(vfp, 19, 8) |
- FLD_VAL(vbp, 31, 20);
- }
+ timing_h = FLD_VAL(hsw-1, dispc.feat->sw_start, 0) |
+ FLD_VAL(hfp-1, dispc.feat->fp_start, 8) |
+ FLD_VAL(hbp-1, dispc.feat->bp_start, 20);
+ timing_v = FLD_VAL(vsw-1, dispc.feat->sw_start, 0) |
+ FLD_VAL(vfp, dispc.feat->fp_start, 8) |
+ FLD_VAL(vbp, dispc.feat->bp_start, 20);
dispc_write_reg(DISPC_TIMING_H(channel), timing_h);
dispc_write_reg(DISPC_TIMING_V(channel), timing_v);
@@ -3671,6 +3717,74 @@ static void _omap_dispc_initial_config(void)
dispc_ovl_enable_zorder_planes();
}
+static const struct dispc_features omap24xx_dispc_features __initconst = {
+ .hp_max = 256,
+ .vp_max = 255,
+ .sw_max = 64,
+ .sw_start = 5,
+ .fp_start = 15,
+ .bp_start = 27,
+ .calc_scaling = dispc_ovl_calc_scaling_24xx,
+ .calc_core_clk = calc_core_clk_24xx,
+};
+
+static const struct dispc_features omap34xx_rev1_0_dispc_features __initconst = {
+ .hp_max = 256,
+ .vp_max = 255,
+ .sw_max = 64,
+ .sw_start = 5,
+ .fp_start = 15,
+ .bp_start = 27,
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+};
+
+static const struct dispc_features omap34xx_rev3_0_dispc_features __initconst = {
+ .hp_max = 4096,
+ .vp_max = 4095,
+ .sw_max = 256,
+ .sw_start = 7,
+ .fp_start = 19,
+ .bp_start = 31,
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+};
+
+static const struct dispc_features omap44xx_dispc_features __initconst = {
+ .hp_max = 4096,
+ .vp_max = 4095,
+ .sw_max = 256,
+ .sw_start = 7,
+ .fp_start = 19,
+ .bp_start = 31,
+ .calc_scaling = dispc_ovl_calc_scaling_44xx,
+ .calc_core_clk = calc_core_clk_44xx,
+};
+
+static int __init dispc_init_features(struct device *dev)
+{
+ dispc.feat = devm_kzalloc(dev, sizeof(*dispc.feat), GFP_KERNEL);
+ if (!dispc.feat) {
+ dev_err(dev, "Failed to allocate DISPC Features\n");
+ return -ENOMEM;
+ }
+
+ if (cpu_is_omap24xx()) {
+ dispc.feat = &omap24xx_dispc_features;
+ } else if (cpu_is_omap34xx()) {
+ if (omap_rev() < OMAP3430_REV_ES3_0)
+ dispc.feat = &omap34xx_rev1_0_dispc_features;
+ else
+ dispc.feat = &omap34xx_rev3_0_dispc_features;
+ } else if (cpu_is_omap44xx()) {
+ dispc.feat = &omap44xx_dispc_features;
+ } else {
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
/* DISPC HW IP initialisation */
static int __init omap_dispchw_probe(struct platform_device *pdev)
{
@@ -3725,6 +3839,10 @@ static int __init omap_dispchw_probe(struct platform_device *pdev)
dispc.dss_clk = clk;
+ r = dispc_init_features(&dispc.pdev->dev);
+ if (r)
+ return r;
+
pm_runtime_enable(&pdev->dev);
r = dispc_runtime_get();
--
1.7.10
changes since V1
V2 : moved dispc_ops structure definitions from dss_features.c to dispc.c
renamed dispc_ops to dispc_features
V3 : moved dispc_features definitions close to dispc_init_features thereby
eliminating various functions declarations from top of dispc.c
used __initdata before dispc_features definitions and __init before
dispc_init_features()
initialized dispc.feat with devm_kzalloc
removed definitions of _dispc_lcd_timings_ok_24xx,
_dispc_lcd_timings_ok_44xx, _dispc_mgr_set_lcd_timings_hv_24xx and
_dispc_mgr_set_lcd_timings_hv_44xx replacing them with appropiate
variables
V4 : replaced __initdata with __initconst as per guidelines in
include/linux/init.h
renamed various dispc_features structures to give consistent naming
removed dispc.feat deinitialization code from omap_dispchw_remove()
added a check for omap4 in dispc_init_features()
^ permalink raw reply related
* [PATCH V4 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Chandrabhanu Mahapatra @ 2012-08-16 11:30 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <1344859176-4512-1-git-send-email-cmahapatra@ti.com>
All the cpu_is checks have been moved to dss_init_features function providing a
much more generic and cleaner interface. The OMAP version and revision specific
initializations in various functions are cleaned and the necessary data are
moved to dss_features structure which is local to dss.c.
Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
drivers/video/omap2/dss/dss.c | 114 ++++++++++++++++++++++++++---------------
1 file changed, 73 insertions(+), 41 deletions(-)
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 7b1c6ac..a9cb84b 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -31,6 +31,7 @@
#include <linux/clk.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/gfp.h>
#include <video/omapdss.h>
@@ -65,6 +66,12 @@ struct dss_reg {
static int dss_runtime_get(void);
static void dss_runtime_put(void);
+struct dss_features {
+ u16 fck_div_max;
+ int dss_fck_multiplier;
+ char *clk_name;
+};
+
static struct {
struct platform_device *pdev;
void __iomem *base;
@@ -83,6 +90,8 @@ static struct {
bool ctx_valid;
u32 ctx[DSS_SZ_REGS / sizeof(u32)];
+
+ const struct dss_features *feat;
} dss;
static const char * const dss_generic_clk_source_names[] = {
@@ -91,6 +100,30 @@ static const char * const dss_generic_clk_source_names[] = {
[OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK",
};
+static const struct dss_features omap24xx_dss_features __initconst = {
+ .fck_div_max = 16,
+ .dss_fck_multiplier = 2,
+ .clk_name = NULL,
+};
+
+static const struct dss_features omap34xx_dss_features __initconst = {
+ .fck_div_max = 16,
+ .dss_fck_multiplier = 2,
+ .clk_name = "dpll4_m4_ck",
+};
+
+static const struct dss_features omap3630_dss_features __initconst = {
+ .fck_div_max = 32,
+ .dss_fck_multiplier = 1,
+ .clk_name = "dpll4_m4_ck",
+};
+
+static const struct dss_features omap44xx_dss_features __initconst = {
+ .fck_div_max = 32,
+ .dss_fck_multiplier = 1,
+ .clk_name = "dpll_per_m5x2_ck",
+};
+
static inline void dss_write_reg(const struct dss_reg idx, u32 val)
{
__raw_writel(val, dss.base + idx.idx);
@@ -236,7 +269,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src)
return dss_generic_clk_source_names[clk_src];
}
-
void dss_dump_clocks(struct seq_file *s)
{
unsigned long dpll4_ck_rate;
@@ -259,18 +291,10 @@ void dss_dump_clocks(struct seq_file *s)
seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate);
- if (cpu_is_omap3630() || cpu_is_omap44xx())
- seq_printf(s, "%s (%s) = %lu / %lu = %lu\n",
- fclk_name, fclk_real_name,
- dpll4_ck_rate,
- dpll4_ck_rate / dpll4_m4_ck_rate,
- fclk_rate);
- else
- seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n",
- fclk_name, fclk_real_name,
- dpll4_ck_rate,
- dpll4_ck_rate / dpll4_m4_ck_rate,
- fclk_rate);
+ seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n",
+ fclk_name, fclk_real_name, dpll4_ck_rate,
+ dpll4_ck_rate / dpll4_m4_ck_rate,
+ dss.feat->dss_fck_multiplier, fclk_rate);
} else {
seq_printf(s, "%s (%s) = %lu\n",
fclk_name, fclk_real_name,
@@ -470,7 +494,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
unsigned long fck, max_dss_fck;
- u16 fck_div, fck_div_max = 16;
+ u16 fck_div;
int match = 0;
int min_fck_per_pck;
@@ -480,9 +504,8 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
fck = clk_get_rate(dss.dss_clk);
- if (req_pck = dss.cache_req_pck &&
- ((cpu_is_omap34xx() && prate = dss.cache_prate) ||
- dss.cache_dss_cinfo.fck = fck)) {
+ if (req_pck = dss.cache_req_pck && prate = dss.cache_prate &&
+ dss.cache_dss_cinfo.fck = fck) {
DSSDBG("dispc clock info found from cache.\n");
*dss_cinfo = dss.cache_dss_cinfo;
*dispc_cinfo = dss.cache_dispc_cinfo;
@@ -519,16 +542,10 @@ retry:
goto found;
} else {
- if (cpu_is_omap3630() || cpu_is_omap44xx())
- fck_div_max = 32;
-
- for (fck_div = fck_div_max; fck_div > 0; --fck_div) {
+ for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) {
struct dispc_clock_info cur_dispc;
- if (fck_div_max = 32)
- fck = prate / fck_div;
- else
- fck = prate / fck_div * 2;
+ fck = prate / fck_div * dss.feat->dss_fck_multiplier;
if (fck > max_dss_fck)
continue;
@@ -633,22 +650,11 @@ static int dss_get_clocks(void)
dss.dss_clk = clk;
- if (cpu_is_omap34xx()) {
- clk = clk_get(NULL, "dpll4_m4_ck");
- if (IS_ERR(clk)) {
- DSSERR("Failed to get dpll4_m4_ck\n");
- r = PTR_ERR(clk);
- goto err;
- }
- } else if (cpu_is_omap44xx()) {
- clk = clk_get(NULL, "dpll_per_m5x2_ck");
- if (IS_ERR(clk)) {
- DSSERR("Failed to get dpll_per_m5x2_ck\n");
- r = PTR_ERR(clk);
- goto err;
- }
- } else { /* omap24xx */
- clk = NULL;
+ clk = clk_get(NULL, dss.feat->clk_name);
+ if (IS_ERR(clk)) {
+ DSSERR("Failed to get %s\n", dss.feat->clk_name);
+ r = PTR_ERR(clk);
+ goto err;
}
dss.dpll4_m4_ck = clk;
@@ -704,6 +710,28 @@ void dss_debug_dump_clocks(struct seq_file *s)
}
#endif
+static int __init dss_init_features(struct device *dev)
+{
+ dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
+ if (!dss.feat) {
+ dev_err(dev, "Failed to allocate local DSS Features\n");
+ return -ENOMEM;
+ }
+
+ if (cpu_is_omap24xx())
+ dss.feat = &omap24xx_dss_features;
+ else if (cpu_is_omap34xx())
+ dss.feat = &omap34xx_dss_features;
+ else if (cpu_is_omap3630())
+ dss.feat = &omap3630_dss_features;
+ else if (cpu_is_omap44xx())
+ dss.feat = &omap44xx_dss_features;
+ else
+ return -ENODEV;
+
+ return 0;
+}
+
/* DSS HW IP initialisation */
static int __init omap_dsshw_probe(struct platform_device *pdev)
{
@@ -750,6 +778,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
+ r = dss_init_features(&dss.pdev->dev);
+ if (r)
+ return r;
+
rev = dss_read_reg(DSS_REVISION);
printk(KERN_INFO "OMAP DSS rev %d.%d\n",
FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
--
1.7.10
changes since V1
V2 : moved dss_ops structure definitions from dss_features.c to dss.c
renamed dss_ops to dss_features
removed dss_get_clk_24xx(), dss_get_clk_34xx(), dss_get_clk_44xx,
set_dump_clk_str_24_34() and set_dump_clk_str() replacing them
with appropiate variables in dss_features structure
V3 : used __initdata before dss_features definitions and __init before
dss_init_features()
initialized dss.feat with devm_kzalloc
removed definitions of check_dss_cinfo_fck(),
check_dss_cinfo_fck_34xx(), dss_get_clk_24xx(), dss_get_clk_34xx()
and dss_get_clk_44xx() replacing them with appropiate variables
V4 : replaced __initdata with __initconst as per guidelines in
include/linux/init.h
renamed various dss_features structures to give consistent naming
removed dss.feat deinitialization code from omap_dispchw_remove()
replaced #include <linux/dma-mapping.h> with <linux/gfx.h>
renamed factor variable of dss_features structure with
dss_fck_multiplier
added a check for omap4 in dss_init_features()
^ permalink raw reply related
* Re: [PATCH 3/6] OMAPDSS: DSI: Maintain copy of video mode timings in driver data
From: Tomi Valkeinen @ 2012-08-16 11:31 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345102594-6222-4-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]
On Thu, 2012-08-16 at 13:06 +0530, Archit Taneja wrote:
> The DSI driver currently relies on the omap_dss_device struct to receive the
> video mode timings requested by the panel driver. This makes the DSI interface
> driver dependent on the omap_dss_device struct.
>
> Make the DSI driver data maintain it's own video mode timings field. The panel
> driver is expected to call omapdss_dsi_set_videomode_timings() to configure the
> video mode timings before the interface is enabled. The function takes in a
> void pointer rather than a pointer to omap_dss_dsi_videomode_timings struct.
> This is because this function will finally be an output op shared across
> different outputs to set custom or private timings.
I don't think the function should take a void * in any case. If we want
to share the function, it should take a struct that perhaps contains an
union of rfbi and dsi timings.
But I'm not sure if there's any benefit for that...
So do you see us having just one set_timings, which would take either
the normal video timings, rfbi timings or dsi timings?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 6/6] OMAPDSS: VENC: Maintian copy of video output polarity in private data
From: Tomi Valkeinen @ 2012-08-16 11:38 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345102594-6222-7-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]
On Thu, 2012-08-16 at 13:06 +0530, Archit Taneja wrote:
> The VENC driver currently relies on the omap_dss_device struct to configure the
> video output polarity. This makes the VENC interface driver dependent on the
> omap_dss_device struct.
>
> Make the VENC driver data maintain it's own polarity field. A panel driver
> is expected to call omapdss_venc_set_vid_out_polarity() before enabling the
> interface.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/dss/dss.h | 2 ++
> drivers/video/omap2/dss/venc.c | 13 ++++++++++++-
> drivers/video/omap2/dss/venc_panel.c | 6 ++++++
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index c17d298..b2cf5530 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -479,6 +479,8 @@ u32 omapdss_venc_get_wss(struct omap_dss_device *dssdev);
> int omapdss_venc_set_wss(struct omap_dss_device *dssdev, u32 wss);
> void omapdss_venc_set_type(struct omap_dss_device *dssdev,
> enum omap_dss_venc_type type);
> +void omapdss_venc_set_vid_out_polarity(struct omap_dss_device *dssdev,
> + enum omap_dss_signal_level vid_out_pol);
> int venc_panel_init(void);
> void venc_panel_exit(void);
>
> diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
> index 2d90fcf..8cb372f 100644
> --- a/drivers/video/omap2/dss/venc.c
> +++ b/drivers/video/omap2/dss/venc.c
> @@ -303,6 +303,7 @@ static struct {
>
> struct omap_video_timings timings;
> enum omap_dss_venc_type type;
> + enum omap_dss_signal_level vid_out_pol;
> } venc;
>
> static inline void venc_write_reg(int idx, u32 val)
> @@ -447,7 +448,7 @@ static int venc_power_on(struct omap_dss_device *dssdev)
> else /* S-Video */
> l |= (1 << 0) | (1 << 2);
>
> - if (dssdev->phy.venc.invert_polarity == false)
> + if (venc.vid_out_pol == OMAPDSS_SIG_ACTIVE_HIGH)
> l |= 1 << 3;
Are you sure this is correct? I know practically nothing about analog
TV, but the TRM doesn't seem to say much about that bit, except it can
be used to "invert the video output". It doesn't say there's an
active/inactive level for the signal.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/6] OMAPDSS: DSI: Maintain copy of video mode timings in driver data
From: Archit Taneja @ 2012-08-16 11:58 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345116666.15132.4.camel@lappyti>
On Thursday 16 August 2012 05:01 PM, Tomi Valkeinen wrote:
> On Thu, 2012-08-16 at 13:06 +0530, Archit Taneja wrote:
>> The DSI driver currently relies on the omap_dss_device struct to receive the
>> video mode timings requested by the panel driver. This makes the DSI interface
>> driver dependent on the omap_dss_device struct.
>>
>> Make the DSI driver data maintain it's own video mode timings field. The panel
>> driver is expected to call omapdss_dsi_set_videomode_timings() to configure the
>> video mode timings before the interface is enabled. The function takes in a
>> void pointer rather than a pointer to omap_dss_dsi_videomode_timings struct.
>> This is because this function will finally be an output op shared across
>> different outputs to set custom or private timings.
>
> I don't think the function should take a void * in any case. If we want
> to share the function, it should take a struct that perhaps contains an
> union of rfbi and dsi timings.
>
> But I'm not sure if there's any benefit for that...
>
> So do you see us having just one set_timings, which would take either
> the normal video timings, rfbi timings or dsi timings?
I thought of having 2 timing ops, one is a standard "modeline like"
set_timings(), and the other a vague-ish set_custom_timings(). For
us(OMAP), we need to use it for DSI videomode and RFBI, we may reduce
that to only RFBI later if we calculate for DSI timings automatically.
For these extra timings to be consistent across SoCs, we would need to
align to get a common struct of some sort, which could then have unions
as you said. For now, I thought having a void pointer might suffice.
Archit
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox