* 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
* 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
* [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
* [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 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 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 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 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 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
* 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 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
* [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 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 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
* Re: [PATCH 0/7] HID: picoLCD updates
From: Jiri Kosina @ 2012-08-15 21:32 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120815171635.2564a4a8@neptune.home>
On Wed, 15 Aug 2012, Bruno Prémont wrote:
> > I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
> > I have expected all of those to actually be on theoretical problems not
> > ever having happened in the wild, but it might be that you are actually
> > chasing on of those.
> >
> > Could you please retest with latest Linus' tree (or at least eb055fd0560b)
> > to see whether this hasn't actually been fixed already by Alan's series?
>
> I've started trying that out, it seems Alan's work improved things.
>
> For the first few attempts I have not seen SLAB corruptions, though after
> a few rounds I hit accumulation of the following messages:
> [ 297.174828] hid-picolcd 0003:04D8:C002.0003: output queue full
> [ 297.181098] hid-picolcd 0003:04D8:C002.0003: output queue full
> [ 297.187820] hid-picolcd 0003:04D8:C002.0003: output queue full
> [ 297.194087] hid-picolcd 0003:04D8:C002.0003: output queue full
>
> with sporadically in between:
> [ 292.668019] hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
>
> At first glance I think the queue filling up and never draining is caused
> by hid_hw_stop() stalling the queue and the time between both being just too
> short.
I don't really understand this explanation. Once usb_kill_urb() returns,
the URB should be available for future use (and therefore all queues
completely drained).
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCHv2 0/8] *** ARM: Update arch-vt8500 to Devicetree ***
From: Stephen Warren @ 2012-08-15 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1344477300-25251-1-git-send-email-linux@prisktech.co.nz>
On 08/08/2012 07:54 PM, Tony Prisk wrote:
> This patchset updates arch-vt8500 to devicetree and removes all the old-style
> code. Support for WM8650 has also been added.
Sorry for taking such a long time to re-review this.
I scanned the series looking for just the changes to the issues I raised
on v1, and I do see almost everything has been fixed up OK, except for
the one issue I just commented on. So, aside from that, this series is
fine by me. I'll defer to others about whether it's necessary to fix the
display node-name-vs.-phandle issue I pointed out, or defer that until
later.
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-15 15:16 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <alpine.LNX.2.00.1208151409500.7026@pobox.suse.cz>
Hi Jiri,
On Wed, 15 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
> I have expected all of those to actually be on theoretical problems not
> ever having happened in the wild, but it might be that you are actually
> chasing on of those.
>
> Could you please retest with latest Linus' tree (or at least eb055fd0560b)
> to see whether this hasn't actually been fixed already by Alan's series?
I've started trying that out, it seems Alan's work improved things.
For the first few attempts I have not seen SLAB corruptions, though after
a few rounds I hit accumulation of the following messages:
[ 297.174828] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.181098] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.187820] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.194087] hid-picolcd 0003:04D8:C002.0003: output queue full
with sporadically in between:
[ 292.668019] hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
At first glance I think the queue filling up and never draining is caused
by hid_hw_stop() stalling the queue and the time between both being just too
short.
Maybe me (un)binding on hid-picolcd driver level is the major cause of this
and (un)binding at lower usb level would kill the queue thus preventing
it just growing until full...
Is there a proper way to do rate-limiting or throttling when submitting
reports (which are the urbs deeper in the stack) and catch -ENODEV early
while remove() is delayed for locking reasons?
At least until 3.5 submitting reports provides no return value for driver
which could convey errors like -ENODEV, -EBUSY.
After a few bind-unbind iterations I have also hit a race in hid-picolcd
with regard to fbdefio.
That looks like chicken-egg release sequencing between hid-picolcd and
fb_info subdev -- though I'm wondering how that happens as the only framebuffer
user I know of - fbcon - should have stopped using it after
unregister_framebuffer() returned and released the last reference!
But that part can be improved rather easily with a new spinlock synchronizing
interoperation between hid side and framebuffer side.
Thanks,
Bruno
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Jiri Kosina @ 2012-08-15 12:11 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120815114236.0f7db40e@neptune.home>
On Wed, 15 Aug 2012, Bruno Prémont wrote:
> > > [ 6383.521833] ======================================> > > [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
> > > [ 6383.530020] -----------------------------------------------------------------------------
> > > [ 6383.530020]
> > > [ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
> > > [ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
> > > [ 6383.530020]
> > > [ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
> > > [ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > > [ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > > [ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > > [ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
> > > [ 6383.530020] Redzone cef511d0: bb bb bb bb ....
> > > [ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
> > > [ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
> > > [ 6383.530020] Call Trace:
> > > [ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
> > > [ 6383.530020] [<c10bd415>] object_err+0x35/0x40
> > > [ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
> > > [ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
> > > [ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
> > > [ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
> > > [ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
> > > [ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
> > > [ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
> > > [ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
> > > [ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
> > > [ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
> > > [ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
> > > [ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
> > > [ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
> > > [ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
> > > [ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
> > > [ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
> > > [ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
> > > [ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
> > > [ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
> > > [ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
> > > [ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
> > > [ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
> >
> > So I am wondering whether the path this happens on is
> >
> > if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
> > usbhid_restart_out_queue(usbhid);
> >
> > in __usbhid_submit_report(). It would then indicate perhaps some race with
> > iofl handling.
>
> Huh, that specific test_bit hunk I can't find in __usbhid_submit_report,
> is that 3.6 material?
> I'm running my tests against 3.5...
I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
I have expected all of those to actually be on theoretical problems not
ever having happened in the wild, but it might be that you are actually
chasing on of those.
Could you please retest with latest Linus' tree (or at least eb055fd0560b)
to see whether this hasn't actually been fixed already by Alan's series?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-15 9:42 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <alpine.LNX.2.00.1208151016221.7026@pobox.suse.cz>
Hi Jiri,
On Wed, 15 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 30 Jul 2012, Bruno Prémont wrote:
> > Hi,
> >
> > This series updates picoLCD driver:
> > - split the driver functions into separate files which get included
> > depending on Kconfig selection
> > (implementation for CIR using RC_CORE will follow later)
> > - drop private framebuffer refcounting in favor of refcounting added
> > to fb_info some time ago
> > - fix various bugs issues
> > - disabled firmware version checking in probe() as it does not work
> > anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> > [HID: Fix race condition between driver core and ll-driver]
>
> I have now applied the series to my 'picolcd' branch, except for 6/7,
> please see the comment I sent to it separately.
Will respin that one soon
> > Note: I still get weird behavior on quick unbind/bind sequences
> > issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
> > support and apparently more specifically fb_defio part of it.
> >
> > Unfortunately I'm out of ideas as to how to track down the problem which
> > shows either as SLAB corruption (detected with SLUB debugging, e.g.
>
> Would be nice to have this sorted out before the next merge window indeed,
> so that it can go in together with the rest of the changes.
>
> >
> > [ 6383.521833] ======================================> > [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
> > [ 6383.530020] -----------------------------------------------------------------------------
> > [ 6383.530020]
> > [ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
> > [ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
> > [ 6383.530020]
> > [ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
> > [ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > [ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > [ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > [ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
> > [ 6383.530020] Redzone cef511d0: bb bb bb bb ....
> > [ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
> > [ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
> > [ 6383.530020] Call Trace:
> > [ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
> > [ 6383.530020] [<c10bd415>] object_err+0x35/0x40
> > [ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
> > [ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
> > [ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
> > [ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
> > [ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
> > [ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
> > [ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
> > [ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
> > [ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
> > [ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
> > [ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
> > [ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
> > [ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
> > [ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
> > [ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
> > [ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
> > [ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
> > [ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
> > [ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
> > [ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
> > [ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
> > [ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
>
> So I am wondering whether the path this happens on is
>
> if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
> usbhid_restart_out_queue(usbhid);
>
> in __usbhid_submit_report(). It would then indicate perhaps some race with
> iofl handling.
Huh, that specific test_bit hunk I can't find in __usbhid_submit_report,
is that 3.6 material?
I'm running my tests against 3.5...
The nearest I have is:
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
if (!irq_out_pump_restart(hid))
set_bit(HID_OUT_RUNNING, &usbhid->iofl);
> Could you please stick some printk() just before and after this
> usbhid_restart_out_queue() call, so that we know that it's this triggering
> it?
Thanks,
Bruno
^ permalink raw reply
* [PATCH] video:uvesafb: reduce the double check
From: Wang YanQing @ 2012-08-15 9:33 UTC (permalink / raw)
To: FlorianSchandinat; +Cc: linux-fbdev, linux-kernel, spock
uvesafb_open had checked the par->vbe_state_size,
so we don't need to check it again in uvesafb_vbe_state_save,
this patch just can reduce a few lines of code.
Signed-off-by: Wang YanQing <udknight@gmail.com>
---
drivers/video/uvesafb.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index 2f8f82d..b064a3e 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -357,9 +357,6 @@ static u8 *uvesafb_vbe_state_save(struct uvesafb_par *par)
u8 *state;
int err;
- if (!par->vbe_state_size)
- return NULL;
-
state = kmalloc(par->vbe_state_size, GFP_KERNEL);
if (!state)
return ERR_PTR(-ENOMEM);
--
1.7.11.1.116.g8228a23
^ permalink raw reply related
* Re: [PATCH 0/5] Pass data lines and pixel format info from panel driver to interface
From: Tomi Valkeinen @ 2012-08-15 8:41 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344595026-10607-1-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]
On Fri, 2012-08-10 at 16:07 +0530, Archit Taneja wrote:
> This is a continuation of the work started in the series:
>
> http://marc.info/?l=linux-omap&m=134381744304672&w=2
>
> This makes the panel driver call interface specific functions to configure the
> data lines and pixel format requested by the panel.
>
> Configuration of data lines doesn't happen for DSI or HDMI. For DSI, this
> information is more complex and is taken care by omapdss_dsi_configure_pins(),
> for HDMI, the number of TMDS lines is always fixed.
>
> Configuration of pixel format is done only for DSI in command mode and RFBI.
> Currently, these new functions take different arguments,
> omapdss_dsi_set_pixel_format() takes pixel format of type
> omap_dss_dsi_pixel_format enum, and RFBI omapdss_rfbi_set_pixel_size() takes
> the pixel size directly. It would be good if these could be merged by using
> some sort of MIPI standard for pixel formats. Having the same argument would
> help in having a common interface op in the future.
This series looks good.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Jiri Kosina @ 2012-08-15 8:27 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120730213656.0a9f6d30@neptune.home>
On Mon, 30 Jul 2012, Bruno Prémont wrote:
> Hi,
>
> This series updates picoLCD driver:
> - split the driver functions into separate files which get included
> depending on Kconfig selection
> (implementation for CIR using RC_CORE will follow later)
> - drop private framebuffer refcounting in favor of refcounting added
> to fb_info some time ago
> - fix various bugs issues
> - disabled firmware version checking in probe() as it does not work
> anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> [HID: Fix race condition between driver core and ll-driver]
I have now applied the series to my 'picolcd' branch, except for 6/7,
please see the comment I sent to it separately.
> Note: I still get weird behavior on quick unbind/bind sequences
> issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
> support and apparently more specifically fb_defio part of it.
>
> Unfortunately I'm out of ideas as to how to track down the problem which
> shows either as SLAB corruption (detected with SLUB debugging, e.g.
Would be nice to have this sorted out before the next merge window indeed,
so that it can go in together with the rest of the changes.
>
> [ 6383.521833] ======================================> [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
> [ 6383.530020] -----------------------------------------------------------------------------
> [ 6383.530020]
> [ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
> [ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
> [ 6383.530020]
> [ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
> [ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
> [ 6383.530020] Redzone cef511d0: bb bb bb bb ....
> [ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
> [ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
> [ 6383.530020] Call Trace:
> [ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
> [ 6383.530020] [<c10bd415>] object_err+0x35/0x40
> [ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
> [ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
> [ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
> [ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
> [ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
> [ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
> [ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
> [ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
> [ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
> [ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
> [ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
> [ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
> [ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
> [ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
> [ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
> [ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
> [ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
> [ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
> [ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
> [ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
> [ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
> [ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
So I am wondering whether the path this happens on is
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
usbhid_restart_out_queue(usbhid);
in __usbhid_submit_report(). It would then indicate perhaps some race with
iofl handling.
Could you please stick some printk() just before and after this
usbhid_restart_out_queue() call, so that we know that it's this triggering
it?
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 6/7] HID: picoLCD: disable version check during probe
From: Jiri Kosina @ 2012-08-15 8:15 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120730213859.063173c1@neptune.home>
On Mon, 30 Jul 2012, Bruno Prémont wrote:
> Commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> [HID: Fix race condition between driver core and ll-driver] introduced
> new locking around proce/remove functions that prevent any report/reply
> from hardware to reach driver until it returned from probe.
>
> As such, the ask-reply way to checking picoLCD firmware version during
> probe is bound to timeout and let probe fail.
>
> Disabling the check lets driver sucessfully probe again.
>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> drivers/hid/hid-picolcd_core.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 2d7ef68..42d0791 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -478,13 +478,13 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data)
> {
> int error;
>
> - error = picolcd_check_version(hdev);
> +/* error = picolcd_check_version(hdev);
> if (error)
> return error;
>
> if (data->version[0] != 0 && data->version[1] != 3)
> hid_info(hdev, "Device with untested firmware revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",
> - dev_name(&hdev->dev));
> + dev_name(&hdev->dev)); */
Please just remove it altogether, I don't see a reason to keep the
commented-out code in the in-tree driver.
Once the locking mess is sorted out, we can re-introduce it again as
necessary.
Thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Tomi Valkeinen @ 2012-08-15 6:43 UTC (permalink / raw)
To: Archit Taneja; +Cc: Rob Clark, linux-fbdev, linux-omap
In-Reply-To: <502AA232.1040305@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3953 bytes --]
On Wed, 2012-08-15 at 00:38 +0530, Archit Taneja wrote:
> On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote:
> > On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
> >> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
> >
> >> I guess it depends on how drm/fb want to use it. I guess an output
> >> should have a set_timings() kind of op if it can do it seamlessly. I
> >> guess we can do that easily in DPI, for example, we could reduce the fps
> >> from 60 to 30 without causing an artefacts(I think). For outputs which
> >
> > Yes, that kind of thing is easy to do by just changing the pck divider,
> > which is in a shadow register. I mean, "easy" in theory, at least. Our
> > clock calculation doesn't work like that currently, though, so it could
> > end up changing DSS fck.
> >
> >> can't do it, we could remove the set_timings totally.
> >
> > But we do need set_timings for other outputs also (like sdi). We just
> > can't change them just like that. Only outputs that do not need timings
> > are DSI command mode and rfbi.
> >
> >> However, it'll be kind of inconsistent for some outputs to set timings,
> >> and for others to not, and if in the future drm/fb gets exposed to ops
> >> too, we may have dirty checks to see if set_timings is populated or not.
> >>
> >> The easiest way would be to make all set_timings just update the copy of
> >> the timings output has, and expect drm/fb to disable and re enable the
> >> panel. We may end up doing unnecessary gpio resets and configuration of
> >> the panels though.
> >
> > I think changing things like timings is quite a rare operation. The only
> > case it'd be necessary to change timings often, with speed, and without
> > artifacts would be the fps drop you mentioned, for lower power use with
> > panels that don't mind the fps drop.
> >
> > If I understood correctly, Rob said that drm already disables the output
> > when changing the mode, when I asked if it's ok for the apply's
> > set_timings to require the output to be off.
> >
> > In any case this is not a big issue, I mean, it's not causing any
> > problems. Somebody is going to disable the output anyway when changing
> > the timings. Perhaps even these patches are good, because they make the
> > set_timings consistent across the output drivers (don't they?).
>
> Yes, they do, there isn't a set_timings for RFBI though, only a
> set_size, and DSI has set_timings for video mode and a set_size for
> command mode, I haven't put checks in the ops for a panel driver to
> wrongly call set_timings in commmand mode, and set_size in video mode.
Ok. Well, perhaps we should go forward with these patches then. They
make things consistent, and we don't really know which would be the best
way to handle this, so the method in your patches is as good as some
other.
The dssdev->state needs to be removed at some point, but that can be a
separate task.
> I haven't done that yet because a future patch of mine will have a DSI
> specific op called set_operation_mode() to make us independent of
> dssdev->panel.dsi_mode, there is no guarantee that the panel driver to
> first call set_operation_mode(), and then set_timings(), so I'm not sure
> yet how to deal with that. Probably having a mode/state which says that
> a field is unintialized might help, but that would overcomplicate things.
Well, we can define that set_operation_mode needs to be called before
any other dsi functions. They are kernel drivers, we can presume they
act correctly (although we should of course try to handle error cases
anyway). If they don't, we need to fix them.
If you want to be extra safe there, you could add third value to the
dsi_mode enum: UNDEFINED or such (I guess this is what you meant also).
By default dsi's mode would be undefined, and functions could check it.
But I agree it'd add lots of checks all around.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
From: Ryan Mallon @ 2012-08-15 0:17 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <CANq1E4QP8PwQRFBr++b3cu1dM9NWf6+Y45rq4UAirjaAHyeM4Q@mail.gmail.com>
On 14/08/12 21:01, David Herrmann wrote:
> Hi Ryan
>
> On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 13/08/12 00:53, David Herrmann wrote:
>>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 195 insertions(+)
>>>
>>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>>> index fb39737..279f4d8 100644
>>> --- a/drivers/video/console/fblog.c
>>> +++ b/drivers/video/console/fblog.c
>>> @@ -23,15 +23,210 @@
>>> * all fblog instances before running other graphics applications.
>>> */
>>>
>>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/fb.h>
>>> #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +enum fblog_flags {
>>> + FBLOG_KILLED,
>>> +};
>>> +
>>> +struct fblog_fb {
>>> + unsigned long flags;
>>
>> Are more flags added in later patches? If not, why not just have:
>>
>> bool is_killed;
>>
>> ?
>
> Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
> FBLOG_SUSPENDED, FBLOG_BLANKED.
>
>>> +static void fblog_release(struct device *dev)
>>> +{
>>> + struct fblog_fb *fb = to_fblog_dev(dev);
>>> +
>>> + kfree(fb);
>>> + module_put(THIS_MODULE);
>>> +}
>>> +
>>> +static void fblog_do_unregister(struct fb_info *info)
>>> +{
>>> + struct fblog_fb *fb;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (!fb || fb->info != info)
>>> + return;
>>> +
>>> + fblog_fbs[info->node] = NULL;
>>> +
>>> + device_del(&fb->dev);
>>> + put_device(&fb->dev);
>>
>> device_unregister?
>
> Right, I will replace it.
>
>>> +}
>>> +
>>> +static void fblog_do_register(struct fb_info *info, bool force)
>>> +{
>>> + struct fblog_fb *fb;
>>> + int ret;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (fb && fb->info != info) {
>>> + if (!force)
>>> + return;
>>> +
>>> + fblog_do_unregister(fb->info);
>>> + }
>>> +
>>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>>> + if (!fb)
>>> + return;
>>> +
>>> + fb->info = info;
>>> + __module_get(THIS_MODULE);
>>> + device_initialize(&fb->dev);
>>> + fb->dev.class = fb_class;
>>> + fb->dev.release = fblog_release;
>>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>>> + fblog_fbs[info->node] = fb;
>>> +
>>> + ret = device_add(&fb->dev);
>>> + if (ret) {
>>> + fblog_fbs[info->node] = NULL;
>>> + set_bit(FBLOG_KILLED, &fb->flags);
>>> + put_device(&fb->dev);
>>
>> kfree(fb); ?
>
> No. See device_initialize() in ./drivers/base/core.c. After a call to
> device_initialize() the object is ref-counted so put_device() will
> invoke the fblog_release() callback which will call kfree(fb) itself.
>
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static void fblog_register(struct fb_info *info, bool force)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_register(info, force);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>> +
>>> +static void fblog_unregister(struct fb_info *info)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_unregister(info);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>
>> This locking is needlessly heavy, and could easily pushed down into the
>> fb_do_(un)register functions. It would also help make it clear exactly
>> what the lock is protecting.
>
> I need to call fblog_do_unregister() from within fblog_do_register().
> I cannot release the locks while calling fblog_do_unregister() so I
> need the unlocked fblog_do_unregister() function. So the locking must
> be in a wrapper function.
>
> See below for an explanation of the locks.
I meant something like the below. It doesn't actually make the lock much
more fine-grained, but (IMHO) it does make it a bit more clear how the
lock is being used. I also don't think you need to split
device_initialize and device_add, which can make the code a bit simpler:
static void __fblog_unregister(struct fblog_fb *fb)
{
fblog_fbs[fb->info->node] = NULL;
device_unregister(&fb->dev);
}
static void fblog_unregister(struct fb_info *info)
{
struct fblog_fb *fb;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (!fb || fb->info != info) {
mutex_unlock(&fblog_registration_lock);
return;
}
__fblog_unregister(fb);
mutex_unlock(&fblog_registration_lock);
}
static int fblog_register(struct fb_info *info, bool force)
{
struct fblog_fb *fb;
int ret;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (fb && fb->info != info) {
if (!force) {
mutex_unlock(&fblog_registration_lock);
return -EEXIST;
}
__fblog_unregister(fb);
}
fb = kzalloc(sizeof(*fb), GFP_KERNEL);
if (!fb)
return;
fb->info = info;
__module_get(THIS_MODULE);
fb->dev.class = fb_class;
fb->dev.release = fblog_release;
dev_set_name(&fb->dev, "fblog%d", info->node);
ret = device_register(&fb->dev);
if (ret) {
mutex_unlock(&fblog_registeration_lock);
put_device(&fb->dev);
return ret;
}
fblog_fbs[info->node] = fb;
mutex_unlock(&fblog_registeration_lock);
return 0;
}
Functions which do:
foo() {
lock(some_lock);
do_foo();
unlock(some_lock);
}
can be a valid pattern for locked/unlocked versions (usually the
unlocked version do_foo will be called __foo). But other times it looks
lazy, where the lock is just serialising everthing and doesn't scale
well. Granted, in a case like this it probably doesn't matter, but it
still a good idea to try and make the locking as fine grained as
possible. It also helps when trying to determine what a lock is actually
protecting, since if do_foo is long, the lock may or may not be
protecting any number of things inside it.
>>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct fb_event *event = data;
>>> + struct fb_info *info = event->info;
>>> +
>>> + switch(action) {
>>> + case FB_EVENT_FB_REGISTERED:
>>> + /* This is called when a low-level system driver registers a new
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held when this is called. */
>>
>> Nitpick:
>>
>> /*
>> * The Linux kernel multi-line
>> * comment style looks like
>> * this.
>> */
>
> I was confused by a recent discussion on the LKML:
> http://comments.gmane.org/gmane.linux.kernel/1282421
> However, turns out they didn't add this to CodingStyle so I will adopt
> the old style again. Will be fixed in the next revision, thanks.
>
>>> + fblog_register(info, true);
>>> + break;
>>> + case FB_EVENT_FB_UNREGISTERED:
>>> + /* This is called when a low-level system driver unregisters a
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held. */
>>> + fblog_unregister(info);
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void fblog_scan(void)
>>> +{
>>> + unsigned int i;
>>> + struct fb_info *info, *tmp;
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>
>> Nitpick:
>>
>> if (IS_ERR_OR_NULL(info))
>
> Didn't know of this macro, thanks.
>
>>> + continue;
>>> +
>>> + fblog_register(info, false);
>>
>> This function should really return a value to indicate if it failed.
>> There is no point continuing if it didn't register anything.
>
> Indeed.
>
>>> + /* There is a very subtle race-condition. Even though we might
>>> + * own a reference to the fb, it may still get unregistered
>>> + * between our call from get_fb_info() and fblog_register().
>>> + * Therefore, we simply check whether the same fb still is
>>> + * registered by calling get_fb_info() again. Only if they
>>> + * differ we know that it got unregistered, therefore, we
>>> + * call fblog_unregister() with the old pointer. */
>>> +
>>> + tmp = get_fb_info(i);
>>> + if (tmp && !IS_ERR(tmp))
>>> + put_fb_info(tmp);
>>> + if (tmp != info)
>>> + fblog_unregister(info);
>>
>> It would be better to fix this issue properly. Calling fblog_unregister
>> here also looks unsafe if the call to fblog_register above failed.
>
> fblog_unregister() does nothing if the passed "info" does not match
> the found "info" so this is safe. And as we are holding a reference to
> "info" here, the pointer is always valid and cannot be used by other
> FBs.
>
> Fixing this properly means changing the locking of fbdev. This can
> either be done by exporting the fbdev-registration-lock (which I want
> to avoid as locking should never be exported in an API) or by changing
> fbdev to provide an scan/enumeration function itself. However, in my
> opinion both would be uglier than using this race-condition-check
> here.
>
> The best way would be redesigning the fbdev in-kernel API. But that
> means checking that fbcon will not break and this is something I
> really don't want to touch.
Fair enough. It might be something to come back to once this gets merged.
>>> + /* Here we either called fblog_unregister() and therefore do not
>>> + * need any reference to the fb, or we can be sure that the FB
>>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>>> + * before the last reference is dropped. Hence, we can drop our
>>> + * reference here. */
>>
>> This seems a slightly odd reasoning. Why would you not hold a reference
>> to something you are using?
>
> It's because get_fb_info() takes the fbdev-registration-lock so we
> cannot call it from fblog_event().
That could possibly be fixed by providing an unlocked __get_fb_info
function. Then the ref-counting could be done by calling __get_fb_info
in fblog_register and __put_fb_info in fblog_unregister, so that you
hold the ref-count for the lifetime of the fblog object which I think
makes a bit more sense.
> And fblog_event() calls
> fblog_register() for hotplugged framebuffers so we cannot get a refcnt
> for hotplugged framebuffers.
> Now I can either increase the fbdev-refcount manually without calling
> get_fb_info() in fblog_register() or I can simply drop the framebuffer
> when the fbdev-core notifies me that it is gone. I chose the latter
> one.
>
>>> + put_fb_info(info);
>>> + }
>>> +}
>>> +
>>> +static struct notifier_block fblog_notifier = {
>>> + .notifier_call = fblog_event,
>>> +};
>>>
>>> static int __init fblog_init(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = fb_register_client(&fblog_notifier);
>>> + if (ret) {
>>> + pr_err("cannot register framebuffer notifier\n");
>>> + return ret;
>>> + }
>>> +
>>> + fblog_scan();
>>> +
>>> return 0;
>>> }
>>>
>>> static void __exit fblog_exit(void)
>>> {
>>> + unsigned int i;
>>> + struct fb_info *info;
>>> +
>>> + fb_unregister_client(&fblog_notifier);
>>> +
>>> + /* We scan through the whole registered_fb array here instead of
>>> + * fblog_fbs because we need to get the device lock _before_ the
>>> + * fblog-registration-lock. */
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>> + continue;
>>> +
>>> + fblog_unregister(info);
>>
>> Given the description of the get_fb_info/fblog_register race above, can
>> this unregister the wrong framebuffer?
>
> No. fblog_unregister() will do nothing if the "info" pointers do not
> match. Moreover, this unregisters _all_ framebuffers, so I don't
> understand how this can unregister the "wrong" framebuffer?
>
> But I just noticed a race here. If the fbdev core unregisters a
> framebuffer during my loop, I will not call fblog_unregister() on it,
> as it does not exist anymore. Therefore, I will not free it in my
> fblog_fbs array as the fblog_notifier has already been unregistered.
> So I need to set a global "exiting" flag and fblog_event() shall only
> handle the UNBIND/UNREGISTER events during exit. Then I simply move
> the fb_unregister_client() call below this loop.
>
>>> + put_fb_info(info);
>>> + }
>>> }
>>>
>>> module_init(fblog_init);
>>>
>>
>
> A short explanation for all locks:
> First of all, I spent hours getting this right. The easy way would be
> removing all locks and relying on the fbdev locking (fbcon does this).
> But obviously, that doesn't work as fbdev has no safe way of
> scanning/enumerating all existing devices. And this is needed as fblog
> can be compiled as a module.
> fbdev has a core registration-lock and each framebuffer has its own
> fb-lock. fblog_event() is sometimes called with these locks held,
> sometimes without any locks held (see the comments in this function)
> and I need to work around this.
> So fblog_event() as entry point for hotplugging is called with the
> fbdev-registration-lock held. And it calls fblog_(un)register() which
> uses its own locks. So when calling this from fblog_scan(), I need to
> make sure to guarantee that the locks are acquired in the same order
> as in fblog_event(), otherwise I might have deadlocks. But I have no
> outside access to the fbdev-registration-lock, therefore, the
> fblog-registration-lock is needed and fblog_scan() needs to check for
> those ugly races.
Right, I think providing unlocked versions of get/put_fb_info will help
fix part of this.
> As you mentioned that this locking is needlessly complex, I have to
> disagree.
Sorry, poor choice of words. I meant 'coarse-grained', not complex.
However, some documentation in the code explaining how the locking
works, and what the locking order is never goes amiss.
> I use one lock to protect the registration
> (fblog_registration_lock) and one lock to protect each registered
> framebuffer from concurrent access (struct fblog_fb->lock). This is
> the most common way to protect hotplugged devices and I don't see how
> this can be done with less locks? (these are the only locks that are
> added by fblog).
I was only referring to the 'heavy' usage of the registration lock by
just acquiring it for the whole register/unregister functions. I was
skimming through the code and was assuming that the actual concurrent
part would just be the addition/removal in the fblog_fbs array, and
therefore the lock was being held for much longer than it needed to be.
As shown above, it isn't as bad as I thought it was.
> Normally, this would be all locks I have to access. However, the
> fbdev-core wasn't designed as in-kernel API and thus has very
> inconsistent locking. And all entry points to fblog that are not
> through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
> acquire the same locks as the fbdev-core to avoid races with the
> fbdev-core. As this is not possible, because the fbdev-locks are not
> exported, I need to carefully use fbdev-functions that guarantee that
> I have no races. And I think I found the only way to guarantee this.
> If anyone has other ideas, I would be glad to hear them.
Yeah, this makes sense. It would be good, as you say, to not export the
locks for fbmem. I think adding a couple of functions to fbmem.c for
doing unlocked access might help a lot though.
~Ryan
^ 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