* Re: [PATCH 12/17] OMAPDSS: clean up dss_mgr_set_timings
From: Archit Taneja @ 2012-09-05 11:46 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1346841713.32747.6.camel@deskari>
On Wednesday 05 September 2012 04:11 PM, Tomi Valkeinen wrote:
> On Wed, 2012-09-05 at 14:45 +0530, Archit Taneja wrote:
>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
>>> dss_mgr_set_timings() can only be called when the output is not active.
>>> This means that most of the code in the function is extra, as there's no
>>> need to write the values to registers, etc, because that will be handled
>>> when the output will be enabled.
>>
>> We need to fix dpi_set_timings() before we can make this change. DPI
>> still tries to change timings on the fly, i.e, with manager enabled. We
>> need to disable and enable the DPI output like we do for other outputs.
>
> Yep, and for HDMI also (I didn't check the others yet).
I don't think HDMI is impacted, we do the full power off and power on
for HDMI, so the manager would be disabled when we set the timings.
>
> I think the simplest way to handle this is to only write the dpi.timings
> in omapdss_dpi_set_timings, and remove the call to dss_mgr_set_timings.
> This is not perfect, as a call to omapdss_dpi_set_timings when the
> display is enabled would result in changing the dpi.timings, but the
> changes wouldn't be actually in use.
The simplest way would be to do what other outputs do, disable the
output and re-enable the output with the new timings value, if the panel
is enabled.
Archit
^ permalink raw reply
* Re: [PATCH 12/17] OMAPDSS: clean up dss_mgr_set_timings
From: Archit Taneja @ 2012-09-05 11:48 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1346845276.32747.11.camel@deskari>
On Wednesday 05 September 2012 05:11 PM, Tomi Valkeinen wrote:
> On Wed, 2012-09-05 at 17:04 +0530, Archit Taneja wrote:
>> On Wednesday 05 September 2012 04:11 PM, Tomi Valkeinen wrote:
>>> On Wed, 2012-09-05 at 14:45 +0530, Archit Taneja wrote:
>>>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
>>>>> dss_mgr_set_timings() can only be called when the output is not active.
>>>>> This means that most of the code in the function is extra, as there's no
>>>>> need to write the values to registers, etc, because that will be handled
>>>>> when the output will be enabled.
>>>>
>>>> We need to fix dpi_set_timings() before we can make this change. DPI
>>>> still tries to change timings on the fly, i.e, with manager enabled. We
>>>> need to disable and enable the DPI output like we do for other outputs.
>>>
>>> Yep, and for HDMI also (I didn't check the others yet).
>>
>> I don't think HDMI is impacted, we do the full power off and power on
>> for HDMI, so the manager would be disabled when we set the timings.
>
> Ah right.
>
>>> I think the simplest way to handle this is to only write the dpi.timings
>>> in omapdss_dpi_set_timings, and remove the call to dss_mgr_set_timings.
>>> This is not perfect, as a call to omapdss_dpi_set_timings when the
>>> display is enabled would result in changing the dpi.timings, but the
>>> changes wouldn't be actually in use.
>>
>> The simplest way would be to do what other outputs do, disable the
>> output and re-enable the output with the new timings value, if the panel
>> is enabled.
>
> Not quite, as there's the mutex in dpi so we can't call enable/disable
> from set_timings. I could create separate non-locked internal functions
> for enable and disable, but that feels more complex than just removing
> the enable & disable from set_timings.
Okay, right.
>
> In the end we'll anyway only allow changing timings when the output is
> disabled.
>
> The only change I had to do, in addition to removing code from
> set_timings functions, was to add display disable & enable calls to the
> "timings" sysfs write. omapfb already only calls set_timings when the
> output is disabled, and omapdrm does the same.
Okay, that sounds good then.
Archit
^ permalink raw reply
* Re: [PATCH 15/17] OMAPDSS: remove extra_info completion code
From: Archit Taneja @ 2012-09-05 13:43 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1346833555-31258-16-git-send-email-tomi.valkeinen@ti.com>
On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
> Now that fifo merge has been removed, nobody uses the extra_info related
> completion code, which can be removed.
I think this might come into use when we fix the usage of channel out
field. That is, since channel out is an immediate write field, when we
set a new manager for an overlay, we may need to wait till the overlay
disable kicks in, only then we should change channel out.
For this, we would need some wait for extra_info, right?
Archit
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 69 ---------------------------------------
> 1 file changed, 69 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 32b5379..2579d15 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -118,7 +118,6 @@ static struct {
> static spinlock_t data_lock;
> /* lock for blocking functions */
> static DEFINE_MUTEX(apply_lock);
> -static DECLARE_COMPLETION(extra_updated_completion);
>
> static void dss_register_vsync_isr(void);
>
> @@ -355,69 +354,6 @@ static bool need_go(struct omap_overlay_manager *mgr)
> return false;
> }
>
> -/* returns true if an extra_info field is currently being updated */
> -static bool extra_info_update_ongoing(void)
> -{
> - const int num_mgrs = dss_feat_get_num_mgrs();
> - int i;
> -
> - for (i = 0; i < num_mgrs; ++i) {
> - struct omap_overlay_manager *mgr;
> - struct omap_overlay *ovl;
> - struct mgr_priv_data *mp;
> -
> - mgr = omap_dss_get_overlay_manager(i);
> - mp = get_mgr_priv(mgr);
> -
> - if (!mp->enabled)
> - continue;
> -
> - if (!mp->updating)
> - continue;
> -
> - if (mp->extra_info_dirty || mp->shadow_extra_info_dirty)
> - return true;
> -
> - list_for_each_entry(ovl, &mgr->overlays, list) {
> - struct ovl_priv_data *op = get_ovl_priv(ovl);
> -
> - if (op->extra_info_dirty || op->shadow_extra_info_dirty)
> - return true;
> - }
> - }
> -
> - return false;
> -}
> -
> -/* wait until no extra_info updates are pending */
> -static void wait_pending_extra_info_updates(void)
> -{
> - bool updating;
> - unsigned long flags;
> - unsigned long t;
> - int r;
> -
> - spin_lock_irqsave(&data_lock, flags);
> -
> - updating = extra_info_update_ongoing();
> -
> - if (!updating) {
> - spin_unlock_irqrestore(&data_lock, flags);
> - return;
> - }
> -
> - init_completion(&extra_updated_completion);
> -
> - spin_unlock_irqrestore(&data_lock, flags);
> -
> - t = msecs_to_jiffies(500);
> - r = wait_for_completion_timeout(&extra_updated_completion, t);
> - if (r = 0)
> - DSSWARN("timeout in wait_pending_extra_info_updates\n");
> - else if (r < 0)
> - DSSERR("wait_pending_extra_info_updates failed: %d\n", r);
> -}
> -
> int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
> {
> unsigned long timeout = msecs_to_jiffies(500);
> @@ -823,7 +759,6 @@ static void dss_apply_irq_handler(void *data, u32 mask)
> {
> const int num_mgrs = dss_feat_get_num_mgrs();
> int i;
> - bool extra_updating;
>
> spin_lock(&data_lock);
>
> @@ -854,10 +789,6 @@ static void dss_apply_irq_handler(void *data, u32 mask)
> dss_write_regs();
> dss_set_go_bits();
>
> - extra_updating = extra_info_update_ongoing();
> - if (!extra_updating)
> - complete_all(&extra_updated_completion);
> -
> if (!need_isr())
> dss_unregister_vsync_isr();
>
>
^ permalink raw reply
* Re: [PATCH 13/17] Revert "OMAPDSS: APPLY: add fifo-merge support"
From: Archit Taneja @ 2012-09-05 13:55 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1346833555-31258-14-git-send-email-tomi.valkeinen@ti.com>
On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
> This reverts commit 1d71f42b35ed66d90a9a39bc515bb16cfe2d4a46.
>
> Adding fifo merge feature as an omapdss internal configuration was a
> mistake. We cannot hide from the users of omapdss the complexities of
> fifo merge.
>
> This commit removes the fifo merge support, which luckily is easily done
> as it was handled totally inside apply.c.
>
> The plan is to try fifo merge again later when it is more clear how the
> hardware acts in various situations, and how the omapdrm wants to use
> fifo merge.
Nitpick: This isn't an exact revert right? Maybe we should mention that,
so that if someone tries to apply the old patch, it doesn't fail.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 154 ++-------------------------------------
> 1 file changed, 8 insertions(+), 146 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 1b49019..02943a5 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -993,11 +993,11 @@ static void dss_apply_fifo_merge(bool use_fifo_merge)
> dss_data.fifo_merge_dirty = true;
> }
>
> -static void dss_ovl_setup_fifo(struct omap_overlay *ovl,
> - bool use_fifo_merge)
> +static void dss_ovl_setup_fifo(struct omap_overlay *ovl)
> {
> struct ovl_priv_data *op = get_ovl_priv(ovl);
> u32 fifo_low, fifo_high;
> + bool use_fifo_merge = false;
This looks like the line which wasn't removed in the original commit.
Archit
^ permalink raw reply
* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
From: Stephen Warren @ 2012-09-05 17:19 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Stephen Warren, Thierry Reding, 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-pm, linux-doc
In-Reply-To: <1346412846-17102-2-git-send-email-acourbot@nvidia.com>
On 08/31/2012 05:34 AM, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
>
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> +Runtime Interpreted Power Sequences
> +=================> +
> +Power sequences are sequential descriptions of actions to be performed on
> +power-related resources. Having these descriptions in a precise data format
> +allows us to take much of the board-specific power control code out of the
> +kernel and place it into the device tree instead, making kernels less
> +board-dependant.
> +
> +In the device tree, power sequences are grouped into a set. The set is always
> +declared as the "power-sequences" sub-node of the device node. Power sequences
> +may reference resources declared by that device.
I had to read that a few times to realize this was talking about a
device with multiple power sequences, and not talking about the steps in
a sequence. I think if you add the following sentence at the start of
that paragraph, it will be clearer:
A device may support multiple power sequences, for different events such
as powering on and off.
Also, perhaps add "these" at the first comma, so the above would read:
In the device tree, these power sequences are...
> +Power Sequences Structure
> +-------------------------
> +Every device that makes use of power sequences must have a "power-sequences"
> +sub-node. Power sequences are sub-nodes of this set node, and their node name
> +indicates the id of the sequence.
> +
> +Every power sequence in turn contains its steps as sub-nodes of itself. Step
The last word on that line should be "Steps".
> +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
> +---------------------
> +Step of a sequence describes an action to be performed on a resource. They
s/Step/Steps/, s/describes/describe/.
> +always include a "type" property which indicates what kind of resource this
> +step works on. Depending on the resource type, additional properties are defined
> +to control the action to be performed.
> +
> +"delay" type required properties:
> + - delay_us: delay to wait in microseconds
DT property name should use "-" not "_" to separate words, so "delay-us".
> +"regulator" type required properties:
> + - id: name of the regulator to use. Regulator is obtained by
> + regulator_get(dev, id)
> + - enable / disable: one of these two empty properties must be present to
> + enable or disable the resource
> +
> +"pwm" type required properties:
> + - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
> + - enable / disable: one of these two empty properties must be present to
> + enable or disable the resource
For those two, would "name" be a better property name than "id"?
I wonder if we should have "regulator-enable" and "regulator-disable"
step types, rather than a "regulator" step type, with an "enable" or
"disable" property within it. I don't feel strongly though; this is
probably fine.
> +"gpio" type required properties:
> + - number: phandle of the GPIO to use.
Naming the property "gpio" would seem more consistent with our GPIO
properties.
> + - enable / disable: one of these two empty properties must be present to
> + enable or disable the resource
You can't really enable or disable a GPIO (well, perhaps you can, but
I'd consider that to affect tri-state rather than value); it's more that
you're setting the output value to 0 or 1. I think a "value" or
"set-value" property with value <0> or <1> would be better.
...
> +After the resources declaration, two sequences follow for powering the backlight
> +on and off. Their names are specified by the pwm-backlight driver.
Not the driver, but the binding for the device.
Overall, the general structure of the bindings looks reasonable to me.
> +++ b/Documentation/power/power_seq.txt
...
> +Platform Data Format
> +--------------------
> +All relevant data structures for declaring power sequences are located in
> +include/linux/power_seq.h.
> +
> +The platform data for a given device is an instance of platform_power_seq_set
> +which points to instances of platform_power_seq. Every platform_power_seq is a
> +single power sequence, and is itself composed of a variable length array of
> +steps.
I don't think you can mandate that the entire platform data structure
for a device is a platform_power_seq_set. Instead, I think you should
say that "The platform data for a device may contain an instance of
platform_power_seq_set...".
...
> +A power sequence can be executed by power_seq_run:
> +
> + int power_seq_run(struct power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.
s/occured/occurred/
> +Sometimes, you may want to browse the list of resources allocated by a sequence,
> +to for instance ensure that a resource of a given type is present. The
> +power_seq_set_resources() function returns a list head that can be used with
> +the power_seq_for_each_resource() macro to browse all the resources of a set:
> +
> + struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
> + power_seq_for_each_resource(pos, seqs)
> +
> +Here "pos" will be of type struct power_seq_resource. This structure contains a
> +"pdata" pointer that can be used to explore the platform data of this resource,
> +as well as the resolved resource, if applicable.
I'm not sure what "pdata" is supposed to point at; platform data applies
to the original "struct device", not one of the resources used by the
power sequences.
> +Finally, users of the device tree can build the platform data corresponding to
> +the tree node using this function:
> +
> + struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev);
Hmmm. It'd be nice not to need separate functions for the non-DT and DT
cases. That would require that devm_power_seq_set_build() be able to
find the power sequence definitions somewhere other than platform data
in the non-DT case - that's exactly why the regulator and pinctrl
subsystems represent the device<->data mapping table separately from the
device's platform data.
> +++ b/drivers/power/power_seq/Kconfig
> +config POWER_SEQ
> + bool
Some kind of help text might be useful?
I didn't review any of the .c files.
> +++ b/include/linux/power_seq.h
> +/**
> + * struct platform_power_seq_step - platform data for power sequences steps
> + * @type: The type of this step. This decides which member of the union is
> + * valid for this step.
> + * @delay: Used if type = POWER_SEQ_DELAY
> + * @regulator: Used if type = POWER_SEQ_REGULATOR
> + * @pwm: Used if type = POWER_SEQ_PWN
> + * @gpio: Used if type = POWER_SEQ_GPIO
In those last 4 line, I think s/type/@type/ since you're referencing
another parameter?
> +struct power_seq_resource {
> + /* relevant for resolving the resource and knowing its type */
> + struct platform_power_seq_step *pdata;
Aha. So this isn't really platform data for the resource, but actually a
step definition that referenced it. I think it'd be better to rename
this field "step", and amend the documentation above not to refer to
"pdata" but explicitly talk about a step definition; the step may have
been defined in pdata, but isn't in the DT case.
Alternatively, why not just copy the step type enum here, rather than
referencing the step definition?
^ permalink raw reply
* Re: [PATCH v5 2/4] pwm_backlight: use power sequences
From: Stephen Warren @ 2012-09-05 17:25 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann, linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1346412846-17102-3-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 08/31/2012 05:34 AM, Alexandre Courbot wrote:
> Make use of the power sequences specified in the device tree or platform
> data to control how the backlight is powered on and off.
> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> Optional properties:
> - - pwm-names: a list of names for the PWM devices specified in the
> - "pwms" property (see PWM binding[0])
> + - pwm-names: name for the PWM device specified in the "pwms" property (see PWM
> + binding[0]). Necessary if power sequences are used
So this implies that power sequence are completely optional in the pwm
binding...
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> config BACKLIGHT_PWM
> tristate "Generic PWM based Backlight Driver"
> depends on PWM
> + select POWER_SEQ
... but that implies they're basically mandatory.
Briefly looking at the code, power sequences don't appear to be
optional, at least for the DT case, so perhaps you just need to update
the documentation to make "pwm-names" non-optional?
^ permalink raw reply
* Re: [PATCH 2/4] video: mmp fb support
From: Jun Nie @ 2012-09-06 3:01 UTC (permalink / raw)
To: linux-fbdev
2012/8/29 Zhou Zhu <zzhu3@marvell.com>:
> Added fb support for Marvell mmp display subsystem.
> This driver is configured using "buffer driver mach info".
> With configured name of path, this driver get path using
> using exported interface of mmp display driver.
> Then this driver get ovly using configured id and operates
> on this ovly to show buffers on display devices.
>
> Change-Id: I1a6fb4f89ac933e1364a4511dd3ec1463463d9c8
> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> Signed-off-by: Lisa Du <cldu@marvell.com>
> ---
> drivers/video/mmp/Kconfig | 4 +
> drivers/video/mmp/Makefile | 2 +-
> drivers/video/mmp/fb/Kconfig | 13 +
> drivers/video/mmp/fb/Makefile | 1 +
> drivers/video/mmp/fb/mmpfb.c | 663 +++++++++++++++++++++++++++++++++++++++++
> drivers/video/mmp/fb/mmpfb.h | 51 ++++
> 6 files changed, 733 insertions(+), 1 deletions(-)
> create mode 100644 drivers/video/mmp/fb/Kconfig
> create mode 100644 drivers/video/mmp/fb/Makefile
> create mode 100644 drivers/video/mmp/fb/mmpfb.c
> create mode 100644 drivers/video/mmp/fb/mmpfb.h
>
> diff --git a/drivers/video/mmp/Kconfig b/drivers/video/mmp/Kconfig
> index 0554336..6a0b056 100644
> --- a/drivers/video/mmp/Kconfig
> +++ b/drivers/video/mmp/Kconfig
> @@ -3,3 +3,7 @@ menuconfig MMP_DISP
> depends on CPU_PXA910 || CPU_MMP2 || CPU_MMP3 || CPU_PXA988
> help
> Marvell Display Subsystem support.
> +
> +if MMP_DISP
> +source "drivers/video/mmp/fb/Kconfig"
> +endif
> diff --git a/drivers/video/mmp/Makefile b/drivers/video/mmp/Makefile
> index 820eb10..fdcd833 100644
> --- a/drivers/video/mmp/Makefile
> +++ b/drivers/video/mmp/Makefile
> @@ -1 +1 @@
> -obj-y += core.o
> +obj-y += core.o fb/
> diff --git a/drivers/video/mmp/fb/Kconfig b/drivers/video/mmp/fb/Kconfig
> new file mode 100644
> index 0000000..9b0141f
> --- /dev/null
> +++ b/drivers/video/mmp/fb/Kconfig
> @@ -0,0 +1,13 @@
> +if MMP_DISP
> +
> +config MMP_FB
> + bool "fb driver for Marvell MMP Display Subsystem"
> + depends on FB
> + select FB_CFB_FILLRECT
> + select FB_CFB_COPYAREA
> + select FB_CFB_IMAGEBLIT
> + default y
> + help
> + fb driver for Marvell MMP Display Subsystem
> +
> +endif
> diff --git a/drivers/video/mmp/fb/Makefile b/drivers/video/mmp/fb/Makefile
> new file mode 100644
> index 0000000..709fd1f
> --- /dev/null
> +++ b/drivers/video/mmp/fb/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_MMP_FB) += mmpfb.o
> diff --git a/drivers/video/mmp/fb/mmpfb.c b/drivers/video/mmp/fb/mmpfb.c
> new file mode 100644
> index 0000000..83e4a0c
> --- /dev/null
> +++ b/drivers/video/mmp/fb/mmpfb.c
> @@ -0,0 +1,663 @@
> +/*
> + * linux/drivers/video/mmp/fb/mmpfb.c
> + * Framebuffer driver for Marvell Display controller.
> + *
> + * Copyright (C) 2012 Marvell Technology Group Ltd.
> + * Authors: Zhou Zhu <zzhu3@marvell.com>
> + *
> + * 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; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/vmalloc.h>
> +#include <asm/cacheflush.h>
> +#include "mmpfb.h"
> +
> +static int var_to_pixfmt(struct fb_var_screeninfo *var)
> +{
> + /*
> + * Pseudocolor mode?
> + */
> + if (var->bits_per_pixel = 8)
> + return PIXFMT_PSEUDOCOLOR;
> +
> + /*
> + * Check for YUV422PLANAR.
> + */
> + if (var->bits_per_pixel = 16 && var->red.length = 8 &&
> + var->green.length = 4 && var->blue.length = 4) {
> + if (var->green.offset >= var->blue.offset)
> + return PIXFMT_YUV422P;
> + else
> + return PIXFMT_YVU422P;
> + }
> +
> + /*
> + * Check for YUV420PLANAR.
> + */
> + if (var->bits_per_pixel = 12 && var->red.length = 8 &&
> + var->green.length = 2 && var->blue.length = 2) {
> + if (var->green.offset >= var->blue.offset)
> + return PIXFMT_YUV420P;
> + else
> + return PIXFMT_YVU420P;
> + }
> +
> + /*
> + * Check for YUV422PACK.
> + */
> + if (var->bits_per_pixel = 16 && var->red.length = 16 &&
> + var->green.length = 16 && var->blue.length = 16) {
> + if (var->red.offset = 0)
> + return PIXFMT_YUYV;
> + else if (var->green.offset >= var->blue.offset)
> + return PIXFMT_UYVY;
> + else
> + return PIXFMT_VYUY;
> + }
> +
> + /*
> + * Check for 565/1555.
> + */
> + if (var->bits_per_pixel = 16 && var->red.length <= 5 &&
> + var->green.length <= 6 && var->blue.length <= 5) {
> + if (var->transp.length = 0) {
> + if (var->red.offset >= var->blue.offset)
> + return PIXFMT_RGB565;
> + else
> + return PIXFMT_BGR565;
> + }
> + }
> +
> + /*
> + * Check for 888/A888.
> + */
> + if (var->bits_per_pixel <= 32 && var->red.length <= 8 &&
> + var->green.length <= 8 && var->blue.length <= 8) {
> + if (var->bits_per_pixel = 24 && var->transp.length = 0) {
> + if (var->red.offset >= var->blue.offset)
> + return PIXFMT_RGB888PACK;
> + else
> + return PIXFMT_BGR888PACK;
> + }
> +
> + if (var->bits_per_pixel = 32 && var->transp.offset = 24) {
> + if (var->red.offset >= var->blue.offset)
> + return PIXFMT_RGBA888;
> + else
> + return PIXFMT_BGRA888;
> + } else {
> + if (var->red.offset >= var->blue.offset)
> + return PIXFMT_RGB888UNPACK;
> + else
> + return PIXFMT_BGR888UNPACK;
> + }
> +
> + /* fall through */
> + }
> +
> + return -EINVAL;
> +
> +}
> +
> +static void pixfmt_to_var(struct fb_var_screeninfo *var, int pix_fmt)
> +{
> + switch (pix_fmt) {
> + case PIXFMT_RGB565:
> + var->bits_per_pixel = 16;
> + var->red.offset = 11; var->red.length = 5;
> + var->green.offset = 5; var->green.length = 6;
> + var->blue.offset = 0; var->blue.length = 5;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_BGR565:
> + var->bits_per_pixel = 16;
> + var->red.offset = 0; var->red.length = 5;
> + var->green.offset = 5; var->green.length = 6;
> + var->blue.offset = 11; var->blue.length = 5;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_RGB888UNPACK:
> + var->bits_per_pixel = 32;
> + var->red.offset = 16; var->red.length = 8;
> + var->green.offset = 8; var->green.length = 8;
> + var->blue.offset = 0; var->blue.length = 8;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_BGR888UNPACK:
> + var->bits_per_pixel = 32;
> + var->red.offset = 0; var->red.length = 8;
> + var->green.offset = 8; var->green.length = 8;
> + var->blue.offset = 16; var->blue.length = 8;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_RGBA888:
> + var->bits_per_pixel = 32;
> + var->red.offset = 16; var->red.length = 8;
> + var->green.offset = 8; var->green.length = 8;
> + var->blue.offset = 0; var->blue.length = 8;
> + var->transp.offset = 24; var->transp.length = 8;
> + break;
> + case PIXFMT_BGRA888:
> + var->bits_per_pixel = 32;
> + var->red.offset = 0; var->red.length = 8;
> + var->green.offset = 8; var->green.length = 8;
> + var->blue.offset = 16; var->blue.length = 8;
> + var->transp.offset = 24; var->transp.length = 8;
> + break;
> + case PIXFMT_RGB888PACK:
> + var->bits_per_pixel = 24;
> + var->red.offset = 16; var->red.length = 8;
> + var->green.offset = 8; var->green.length = 8;
> + var->blue.offset = 0; var->blue.length = 8;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_BGR888PACK:
> + var->bits_per_pixel = 24;
> + var->red.offset = 0; var->red.length = 8;
> + var->green.offset = 8; var->green.length = 8;
> + var->blue.offset = 16; var->blue.length = 8;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_YUV420P:
> + var->bits_per_pixel = 12;
> + var->red.offset = 4; var->red.length = 8;
> + var->green.offset = 2; var->green.length = 2;
> + var->blue.offset = 0; var->blue.length = 2;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_YVU420P:
> + var->bits_per_pixel = 12;
> + var->red.offset = 4; var->red.length = 8;
> + var->green.offset = 0; var->green.length = 2;
> + var->blue.offset = 2; var->blue.length = 2;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_YUV422P:
> + var->bits_per_pixel = 16;
> + var->red.offset = 8; var->red.length = 8;
> + var->green.offset = 4; var->green.length = 4;
> + var->blue.offset = 0; var->blue.length = 4;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_YVU422P:
> + var->bits_per_pixel = 16;
> + var->red.offset = 8; var->red.length = 8;
> + var->green.offset = 0; var->green.length = 4;
> + var->blue.offset = 4; var->blue.length = 4;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_UYVY:
> + var->bits_per_pixel = 16;
> + var->red.offset = 8; var->red.length = 16;
> + var->green.offset = 4; var->green.length = 16;
> + var->blue.offset = 0; var->blue.length = 16;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_VYUY:
> + var->bits_per_pixel = 16;
> + var->red.offset = 8; var->red.length = 16;
> + var->green.offset = 0; var->green.length = 16;
> + var->blue.offset = 4; var->blue.length = 16;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_YUYV:
> + var->bits_per_pixel = 16;
> + var->red.offset = 0; var->red.length = 16;
> + var->green.offset = 4; var->green.length = 16;
> + var->blue.offset = 8; var->blue.length = 16;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + case PIXFMT_PSEUDOCOLOR:
> + var->bits_per_pixel = 8;
> + var->red.offset = 0; var->red.length = 8;
> + var->green.offset = 0; var->green.length = 8;
> + var->blue.offset = 0; var->blue.length = 8;
> + var->transp.offset = 0; var->transp.length = 0;
> + break;
> + }
> +}
> +
> +/*
> + * fb framework has its limitation:
> + * 1. input color/output color is not seprated
> + * 2. fb_videomode not include output color
> + * so for fb usage, we keep a output format which is not changed
> + * then it's added for mmpmode
> + */
> +static void fbmode_to_mmpmode(struct mmp_mode *mode,
> + struct fb_videomode *videomode, int output_fmt)
> +{
> + u64 div_result = 1000000000000ll;
> + mode->name = videomode->name;
> + mode->refresh = videomode->refresh;
> + mode->xres = videomode->xres;
> + mode->yres = videomode->yres;
> +
> + do_div(div_result, videomode->pixclock);
> + mode->pixclock_freq = (u32)div_result;
> +
> + mode->left_margin = videomode->left_margin;
> + mode->right_margin = videomode->right_margin;
> + mode->upper_margin = videomode->upper_margin;
> + mode->lower_margin = videomode->lower_margin;
> + mode->hsync_len = videomode->hsync_len;
> + mode->vsync_len = videomode->vsync_len;
> + mode->hsync_invert = !!(videomode->sync & FB_SYNC_HOR_HIGH_ACT);
> + mode->vsync_invert = !!(videomode->sync & FB_SYNC_VERT_HIGH_ACT);
> + /* no defined flag in fb, use vmode>>3*/
> + mode->invert_pixclock = !!(videomode->vmode & 8);
> + mode->pix_fmt_out = output_fmt;
> +}
> +
> +static void mmpmode_to_fbmode(struct fb_videomode *videomode,
> + struct mmp_mode *mode)
> +{
> + u64 div_result = 1000000000000ll;
> +
> + videomode->name = mode->name;
> + videomode->refresh = mode->refresh;
> + videomode->xres = mode->xres;
> + videomode->yres = mode->yres;
> +
> + do_div(div_result, mode->pixclock_freq);
> + videomode->pixclock = (u32)div_result;
> +
> + videomode->left_margin = mode->left_margin;
> + videomode->right_margin = mode->right_margin;
> + videomode->upper_margin = mode->upper_margin;
> + videomode->lower_margin = mode->lower_margin;
> + videomode->hsync_len = mode->hsync_len;
> + videomode->vsync_len = mode->vsync_len;
> + videomode->sync = (mode->hsync_invert ? FB_SYNC_HOR_HIGH_ACT : 0)
> + | (mode->vsync_invert ? FB_SYNC_VERT_HIGH_ACT : 0);
> + videomode->vmode = mode->invert_pixclock ? 8 : 0;
> +}
> +
> +
> +static void *alloc_framebuffer(size_t size, dma_addr_t *dma)
> +{
> + int nr, i = 0;
> + struct page **pages;
> + void *start;
> +
> + nr = size >> PAGE_SHIFT;
> + start = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
> + if (start = NULL)
> + return NULL;
> +
> + *dma = virt_to_phys(start);
> + pages = vmalloc(sizeof(struct page *) * nr);
> + if (pages = NULL)
> + return NULL;
> +
> + while (i < nr) {
> + pages[i] = phys_to_page(*dma + (i << PAGE_SHIFT));
> + i++;
> + }
> + start = vmap(pages, nr, 0, pgprot_writecombine(pgprot_kernel));
> +
> + vfree(pages);
> + return start;
> +}
> +
> +static int mmpfb_check_var(struct fb_var_screeninfo *var,
> + struct fb_info *info)
> +{
> + struct mmpfb_info *fbi = info->par;
> +
> + if (var->bits_per_pixel = 8)
> + return -EINVAL;
> + /*
> + * Basic geometry sanity checks.
> + */
> + if (var->xoffset + var->xres > var->xres_virtual)
> + return -EINVAL;
> + if (var->yoffset + var->yres > var->yres_virtual)
> + return -EINVAL;
> +
> + /*
> + * Check size of framebuffer.
> + */
> + if (var->xres_virtual * var->yres_virtual *
> + (var->bits_per_pixel >> 3) > fbi->fb_size)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static unsigned int chan_to_field(unsigned int chan, struct fb_bitfield *bf)
> +{
> + return ((chan & 0xffff) >> (16 - bf->length)) << bf->offset;
> +}
> +
> +static u32 to_rgb(u16 red, u16 green, u16 blue)
> +{
> + red >>= 8;
> + green >>= 8;
> + blue >>= 8;
> +
> + return (red << 16) | (green << 8) | blue;
> +}
> +
> +static int mmpfb_setcolreg(unsigned int regno, unsigned int red,
> + unsigned int green, unsigned int blue,
> + unsigned int trans, struct fb_info *info)
> +{
> + struct mmpfb_info *fbi = info->par;
> + u32 val;
> +
> + if (info->fix.visual = FB_VISUAL_TRUECOLOR && regno < 16) {
> + val = chan_to_field(red, &info->var.red);
> + val |= chan_to_field(green, &info->var.green);
> + val |= chan_to_field(blue , &info->var.blue);
> + fbi->pseudo_palette[regno] = val;
> + }
> +
> + if (info->fix.visual = FB_VISUAL_PSEUDOCOLOR && regno < 256) {
> + val = to_rgb(red, green, blue);
> + /* TODO */
> + }
> +
> + return 0;
> +}
> +
> +static int mmpfb_pan_display(struct fb_var_screeninfo *var,
> + struct fb_info *info)
> +{
> + struct mmpfb_info *fbi = (struct mmpfb_info *)info->par;
> + struct mmp_addr addr;
> +
> + addr.phys[0] = (var->yoffset * var->xres_virtual + var->xoffset)
> + * var->bits_per_pixel / 8 + fbi->fb_start_dma;
> + mmp_ovly_set_addr(fbi->ovly, &addr);
> +
> + return 0;
> +}
> +
> +static int var_update(struct fb_info *info)
> +{
> + struct mmpfb_info *fbi = info->par;
> + struct fb_var_screeninfo *var = &info->var;
> + struct fb_videomode *m;
> + int pix_fmt;
> +
> + /* set pix_fmt */
> + pix_fmt = var_to_pixfmt(var);
> + if (pix_fmt < 0)
> + return -EINVAL;
> + pixfmt_to_var(var, pix_fmt);
> + fbi->pix_fmt = pix_fmt;
> +
> + /* set var according to best video mode*/
> + m = (struct fb_videomode *)fb_match_mode(var, &info->modelist);
> + if (!m) {
> + dev_err(fbi->dev, "set par: no match mode, use best mode\n");
> + m = (struct fb_videomode *)fb_find_best_mode(var,
> + &info->modelist);
> + fb_videomode_to_var(var, m);
> + }
> + memcpy(&fbi->mode, m, sizeof(struct fb_videomode));
> +
> + /* fix to 2* yres */
> + var->yres_virtual = var->yres * 2;
> + info->fix.visual = (pix_fmt = PIXFMT_PSEUDOCOLOR) ?
> + FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
> + info->fix.line_length = var->xres_virtual * var->bits_per_pixel / 8;
> + info->fix.ypanstep = var->yres;
> + return 0;
> +}
> +
> +static int mmpfb_set_par(struct fb_info *info)
> +{
> + struct mmpfb_info *fbi = info->par;
> + struct fb_var_screeninfo *var = &info->var;
> + struct mmp_addr addr;
> + struct mmp_win win;
> + struct mmp_mode mode;
> +
> + int ret = var_update(info);
> + if (ret != 0)
> + return ret;
> +
> + /* set window/path according to new videomode */
> + fbmode_to_mmpmode(&mode, &fbi->mode, fbi->output_fmt);
> + mmp_path_set_mode(fbi->path, &mode);
> +
> + win.xsrc = win.xdst = fbi->mode.xres;
> + win.ysrc = win.ydst = fbi->mode.yres;
> + win.pix_fmt = fbi->pix_fmt;
> + mmp_ovly_set_win(fbi->ovly, &win);
> +
> + /* set address always */
> + addr.phys[0] = (var->yoffset * var->xres_virtual + var->xoffset)
> + * var->bits_per_pixel / 8 + fbi->fb_start_dma;
> + mmp_ovly_set_addr(fbi->ovly, &addr);
> +
> + return 0;
> +}
> +
> +static void mmpfb_gfx_power(struct mmpfb_info *fbi, int power)
> +{
> + mmp_ovly_set_onoff(fbi->ovly, power);
> +}
> +
> +static int mmpfb_gfx_blank(int blank, struct fb_info *info)
> +{
> + struct mmpfb_info *fbi = info->par;
> +
> + mmpfb_gfx_power(fbi, (blank = FB_BLANK_UNBLANK));
> +
> + return 0;
> +}
> +
> +static struct fb_ops mmpfb_gfx_ops = {
> + .owner = THIS_MODULE,
> + .fb_blank = mmpfb_gfx_blank,
> + .fb_check_var = mmpfb_check_var,
> + .fb_set_par = mmpfb_set_par,
> + .fb_setcolreg = mmpfb_setcolreg,
> + .fb_pan_display = mmpfb_pan_display,
> + .fb_fillrect = cfb_fillrect,
> + .fb_copyarea = cfb_copyarea,
> + .fb_imageblit = cfb_imageblit,
> +};
> +
> +static int __devinit mmpfb_probe(struct platform_device *pdev)
> +{
> + struct mmp_buffer_driver_mach_info *mi;
> + struct fb_info *info = 0;
> + struct mmpfb_info *fbi = 0;
> + int ret, videomode_num, i;
> + struct fb_videomode *videomodes;
> + struct mmp_mode *mmp_modes;
> + struct mmp_win win;
> + struct mmp_addr addr;
> +
> + mi = pdev->dev.platform_data;
> + if (mi = NULL) {
> + dev_err(&pdev->dev, "no platform data defined\n");
> + return -EINVAL;
> + }
> +
> + /* initialize fb */
> + info = framebuffer_alloc(sizeof(struct mmpfb_info), &pdev->dev);
> + if (info = NULL)
> + return -ENOMEM;
> + fbi = info->par;
> + if (!fbi) {
> + ret = -EINVAL;
> + goto failed;
> + }
> +
> + /* init fb */
> + fbi->fb_info = info;
> + platform_set_drvdata(pdev, fbi);
> + fbi->dev = &pdev->dev;
> + fbi->pix_fmt = mi->default_pixfmt;
> + mutex_init(&fbi->access_ok);
> +
> + /* get display path by name */
> + fbi->path = mmp_get_path(mi->path_name);
> + if (!fbi->path) {
> + dev_err(&pdev->dev, "can't get the path %s\n", mi->path_name);
> + ret = -EINVAL;
> + goto failed_destroy_mutex;
> + }
> +
> + dev_info(fbi->dev, "path %s get\n", fbi->path->name);
> +
> + /* get videomodes from path */
> + videomode_num = mmp_path_get_modelist(fbi->path, &mmp_modes);
> + if (!videomode_num) {
> + dev_err(&pdev->dev, "can't get videomode num\n");
> + ret = -EINVAL;
> + goto failed_destroy_mutex;
> + }
Can you just print warning if mode number is 0, instead of failure?
Plug-able panel is likely not ready when probe and panel driver has to
return no mode info.
> + /* put videomode list to info structure */
> + videomodes = kzalloc(sizeof(struct fb_videomode) * videomode_num,
> + GFP_KERNEL);
> + if (!videomodes) {
> + dev_err(&pdev->dev, "can't malloc video modes\n");
> + ret = -ENOMEM;
> + goto failed_destroy_mutex;
> + }
> + for (i = 0; i < videomode_num; i++)
> + mmpmode_to_fbmode(&videomodes[i], &mmp_modes[i]);
> + fb_videomode_to_modelist(videomodes, videomode_num, &info->modelist);
> +
> + /* set videomode[0] as default mode */
> + memcpy(&fbi->mode, &videomodes[0], sizeof(struct fb_videomode));
> + fbi->output_fmt = mmp_modes[0].pix_fmt_out;
> + fb_videomode_to_var(&info->var, &fbi->mode);
> + mmp_path_set_mode(fbi->path, &mmp_modes[0]);
> + /* fix to 2* yres */
> + info->var.yres_virtual = info->var.yres * 2;
> + pixfmt_to_var(&info->var, fbi->pix_fmt);
> +
> + /* Allocate framebuffer memory: size = modes xy *4 .*/
> + fbi->fb_size = PAGE_ALIGN(info->var.xres_virtual *
> + info->var.yres_virtual * info->var.bits_per_pixel / 8);
> + fbi->fb_start = alloc_framebuffer(fbi->fb_size + PAGE_SIZE,
> + &fbi->fb_start_dma);
> +
> + if (fbi->fb_start = NULL) {
> + dev_err(&pdev->dev, "can't alloc framebuffer\n");
> + ret = -ENOMEM;
> + goto failed_destroy_mutex;
> + }
> + memset(fbi->fb_start, 0, fbi->fb_size);
> +
> + dev_info(fbi->dev, "fb %dk allocated\n", fbi->fb_size/1024);
> +
> + /* get ovly */
> + fbi->ovly = mmp_path_get_ovly(fbi->path, mi->ovly_id);
> + if (!fbi->ovly) {
> + ret = -EINVAL;
> + goto failed_destroy_mutex;
> + }
> + /* set fetch used */
> + mmp_ovly_set_fetch(fbi->ovly, mi->dmafetch_id);
> +
> + /* set win */
> + memset(&win, 0, sizeof(win));
> + win.pix_fmt = fbi->pix_fmt;
> + win.xsrc = win.xdst = fbi->mode.xres;
> + win.ysrc = win.ydst = fbi->mode.yres;
> + mmp_ovly_set_win(fbi->ovly, &win);
> + /* set addr */
> + memset(&addr, 0, sizeof(addr));
> + addr.phys[0] = fbi->fb_start_dma;
> + mmp_ovly_set_addr(fbi->ovly, &addr);
> + mmp_ovly_set_onoff(fbi->ovly, 1);
> +
> + /* Initialise static fb parameters.*/
> + info->flags = FBINFO_DEFAULT | FBINFO_PARTIAL_PAN_OK |
> + FBINFO_HWACCEL_XPAN | FBINFO_HWACCEL_YPAN;
> + info->node = -1;
> + strcpy(info->fix.id, mi->name);
> + info->fix.type = FB_TYPE_PACKED_PIXELS;
> + info->fix.type_aux = 0;
> + info->fix.xpanstep = 0;
> + info->fix.ypanstep = info->var.yres;
> + info->fix.ywrapstep = 0;
> + info->fix.accel = FB_ACCEL_NONE;
> + info->fix.smem_start = fbi->fb_start_dma;
> + info->fix.smem_len = fbi->fb_size;
> + info->fix.visual = (fbi->pix_fmt = PIXFMT_PSEUDOCOLOR) ?
> + FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
> + info->fix.line_length = info->var.xres_virtual *
> + info->var.bits_per_pixel / 8;
> + info->fbops = &mmpfb_gfx_ops;
> + info->pseudo_palette = fbi->pseudo_palette;
> + info->screen_base = fbi->fb_start;
> + info->screen_size = fbi->fb_size;
> +
> + /* For FB framework: Allocate color map and Register framebuffer*/
> + if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
> + ret = -ENOMEM;
> + goto failed_free_buff;
> + }
> + ret = register_framebuffer(info);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to register fb: %d\n", ret);
> + ret = -ENXIO;
> + goto failed_free_cmap;
> + }
> +
> + dev_info(fbi->dev, "loaded to /dev/fb%d <%s>.\n",
> + info->node, info->fix.id);
> +
> +#ifdef CONFIG_ANDROID
> + if (fbi->fb_start) {
> + fb_prepare_logo(info, 0);
> + fb_show_logo(info, 0);
> + }
> +#endif
> +
> + return 0;
> +
> +failed_free_cmap:
> + fb_dealloc_cmap(&info->cmap);
> +failed_free_buff:
> + vfree(fbi->fb_start);
> + kfree(videomodes);
> +failed_destroy_mutex:
> + mutex_destroy(&fbi->access_ok);
> +failed:
> + dev_err(fbi->dev, "mmp-fb: frame buffer device init failed\n");
> + platform_set_drvdata(pdev, NULL);
> +
> + framebuffer_release(info);
> +
> + return ret;
> +}
> +
> +static struct platform_driver mmpfb_driver = {
> + .driver = {
> + .name = "mmp-fb",
> + .owner = THIS_MODULE,
> + },
> + .probe = mmpfb_probe,
> +};
> +
> +static int __devinit mmpfb_init(void)
> +{
> + return platform_driver_register(&mmpfb_driver);
> +}
> +module_init(mmpfb_init);
> +
> +MODULE_AUTHOR("Zhou Zhu <zhou.zhu@marvell.com>");
> +MODULE_DESCRIPTION("Framebuffer driver for Marvell displays");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/video/mmp/fb/mmpfb.h b/drivers/video/mmp/fb/mmpfb.h
> new file mode 100644
> index 0000000..7f8a71d
> --- /dev/null
> +++ b/drivers/video/mmp/fb/mmpfb.h
> @@ -0,0 +1,51 @@
> +/*
> + * linux/drivers/video/mmp/fb/mmpfb.h
> + * Framebuffer driver for Marvell Display controller.
> + *
> + * Copyright (C) 2012 Marvell Technology Group Ltd.
> + * Authors: Zhou Zhu <zzhu3@marvell.com>
> + *
> + * 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; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef _MMP_FB_H_
> +#define _MMP_FB_H_
> +
> +#include <video/mmp_disp.h>
> +#include <linux/fb.h>
> +
> +/* LCD controller private state. */
> +struct mmpfb_info {
> + struct device *dev;
> + int id;
> +
> + struct fb_info *fb_info;
> + /* basicaly videomode is for output */
> + struct fb_videomode mode;
> + int pix_fmt;
> +
> + void *fb_start;
> + int fb_size;
> + dma_addr_t fb_start_dma;
> +
> + struct mmp_ovly *ovly;
> + struct mmp_path *path;
> +
> + struct mutex access_ok;
> +
> + unsigned int pseudo_palette[16];
> + int output_fmt;
> +};
> +#endif /* _MMP_FB_H_ */
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] video: bf*: Add missing spinlock init
From: Jean Delvare @ 2012-09-06 7:23 UTC (permalink / raw)
To: linux-fbdev
It doesn't seem these spinlocks were properly initialized.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
---
I can't even build-test this.
drivers/video/bf54x-lq043fb.c | 1 +
drivers/video/bfin-lq035q1-fb.c | 1 +
drivers/video/bfin-t350mcqb-fb.c | 1 +
3 files changed, 3 insertions(+)
--- linux-3.6-rc4.orig/drivers/video/bf54x-lq043fb.c 2012-07-21 22:58:29.000000000 +0200
+++ linux-3.6-rc4/drivers/video/bf54x-lq043fb.c 2012-09-06 08:49:37.812899185 +0200
@@ -525,6 +525,7 @@ static int __devinit bfin_bf54x_probe(st
info = fbinfo->par;
info->fb = fbinfo;
info->dev = &pdev->dev;
+ spin_lock_init(&info->lock);
platform_set_drvdata(pdev, fbinfo);
--- linux-3.6-rc4.orig/drivers/video/bfin-lq035q1-fb.c 2012-07-21 22:58:29.000000000 +0200
+++ linux-3.6-rc4/drivers/video/bfin-lq035q1-fb.c 2012-09-06 08:48:50.618858480 +0200
@@ -577,6 +577,7 @@ static int __devinit bfin_lq035q1_probe(
info = fbinfo->par;
info->fb = fbinfo;
info->dev = &pdev->dev;
+ spin_lock_init(&info->lock);
info->disp_info = pdev->dev.platform_data;
--- linux-3.6-rc4.orig/drivers/video/bfin-t350mcqb-fb.c 2012-07-21 22:58:29.000000000 +0200
+++ linux-3.6-rc4/drivers/video/bfin-t350mcqb-fb.c 2012-09-06 08:48:12.310825601 +0200
@@ -447,6 +447,7 @@ static int __devinit bfin_t350mcqb_probe
info = fbinfo->par;
info->fb = fbinfo;
info->dev = &pdev->dev;
+ spin_lock_init(&info->lock);
platform_set_drvdata(pdev, fbinfo);
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH 4/4] video: mmp: add tpo hvga panel support
From: Jun Nie @ 2012-09-06 12:01 UTC (permalink / raw)
To: linux-fbdev
2012/8/29 Zhou Zhu <zzhu3@marvell.com>:
> From: Lisa Du <cldu@marvell.com>
>
> Add tpo hvga panel support in marvell display framework.
> This panel driver implements modes query and on/off commands by spi.
> This panel driver also get panel config/ plat power on/off/ connected
> path name from machine-info.
> This panel driver uses mmp_disp supplied register_panel function to
> register panel to path as machine-info defined.
>
> Change-Id: I1d37a9b436f64a01954d7f32407f84f67945286f
> Signed-off-by: Lisa Du <cldu@marvell.com>
> ---
> drivers/video/mmp/Kconfig | 1 +
> drivers/video/mmp/Makefile | 2 +-
> drivers/video/mmp/panel/Kconfig | 5 +
> drivers/video/mmp/panel/Makefile | 1 +
> drivers/video/mmp/panel/tpo_tj032md01bw.c | 181 +++++++++++++++++++++++++++++
> 5 files changed, 189 insertions(+), 1 deletions(-)
> create mode 100644 drivers/video/mmp/panel/Kconfig
> create mode 100644 drivers/video/mmp/panel/Makefile
> create mode 100644 drivers/video/mmp/panel/tpo_tj032md01bw.c
>
> diff --git a/drivers/video/mmp/Kconfig b/drivers/video/mmp/Kconfig
> index ed51d15..e9ea39e 100644
> --- a/drivers/video/mmp/Kconfig
> +++ b/drivers/video/mmp/Kconfig
> @@ -6,5 +6,6 @@ menuconfig MMP_DISP
>
> if MMP_DISP
> source "drivers/video/mmp/hw/Kconfig"
> +source "drivers/video/mmp/panel/Kconfig"
> source "drivers/video/mmp/fb/Kconfig"
> endif
> diff --git a/drivers/video/mmp/Makefile b/drivers/video/mmp/Makefile
> index 6999a09..a014cb3 100644
> --- a/drivers/video/mmp/Makefile
> +++ b/drivers/video/mmp/Makefile
> @@ -1 +1 @@
> -obj-y += core.o hw/ fb/
> +obj-y += core.o hw/ panel/ fb/
> diff --git a/drivers/video/mmp/panel/Kconfig b/drivers/video/mmp/panel/Kconfig
> new file mode 100644
> index 0000000..b6aac76
> --- /dev/null
> +++ b/drivers/video/mmp/panel/Kconfig
> @@ -0,0 +1,5 @@
> +config MMP_PANEL_TPOHVGA
> + bool "tpohvga panel TJ032MD01BW support"
> + default n
> + help
> + tpohvga panel support
> diff --git a/drivers/video/mmp/panel/Makefile b/drivers/video/mmp/panel/Makefile
> new file mode 100644
> index 0000000..2f91611
> --- /dev/null
> +++ b/drivers/video/mmp/panel/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_MMP_PANEL_TPOHVGA) += tpo_tj032md01bw.o
> diff --git a/drivers/video/mmp/panel/tpo_tj032md01bw.c b/drivers/video/mmp/panel/tpo_tj032md01bw.c
> new file mode 100644
> index 0000000..8217650
> --- /dev/null
> +++ b/drivers/video/mmp/panel/tpo_tj032md01bw.c
> @@ -0,0 +1,181 @@
> +/*
> + * linux/drivers/video/mmp/panel/tpo_tj032md01bw.c
> + * active panel using spi interface to do init
> + *
> + * Copyright (C) 2012 Marvell Technology Group Ltd.
> + * Authors: Guoqing Li <ligq@marvell.com>
> + * Lisa Du <cldu@marvell.com>
> + * Zhou Zhu <zzhu3@marvell.com>
> + *
> + * 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; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <video/mmp_disp.h>
> +
> +static u16 panel_init[] = {
> + 0x0801,
> + 0x0800,
> + 0x0200,
> + 0x0304,
> + 0x040e,
> + 0x0903,
> + 0x0b18,
> + 0x0c53,
> + 0x0d01,
> + 0x0ee0,
> + 0x0f01,
> + 0x1058,
> + 0x201e,
> + 0x210a,
> + 0x220a,
> + 0x231e,
> + 0x2400,
> + 0x2532,
> + 0x2600,
> + 0x27ac,
> + 0x2904,
> + 0x2aa2,
> + 0x2b45,
> + 0x2c45,
> + 0x2d15,
> + 0x2e5a,
> + 0x2fff,
> + 0x306b,
> + 0x310d,
> + 0x3248,
> + 0x3382,
> + 0x34bd,
> + 0x35e7,
> + 0x3618,
> + 0x3794,
> + 0x3801,
> + 0x395d,
> + 0x3aae,
> + 0x3bff,
> + 0x07c9,
> +};
> +
> +static u16 panel_poweroff[] = {
> + 0x07d9,
> +};
> +
> +static struct mmp_panel_cmds tpohvga_on_cmds[] = {
> + [0] = {
> + .cmds = panel_init,
> + .cmds_num = ARRAY_SIZE(panel_init),
> + .sleep = 0,
> + },
> +};
> +
> +static struct mmp_panel_cmds tpohvga_off_cmds[] = {
> + [0] = {
> + .cmds = panel_poweroff,
> + .cmds_num = ARRAY_SIZE(panel_poweroff),
> + .sleep = 0,
> + },
> +};
> +
> +static struct mmp_spi_cfg tpohvga_spi_cfg = {
> + .clk_cnt = 16,
> + .tx_bits = 16,
> + .wire_num = 1,
> +};
> +
> +static struct mmp_panel_cmd_sets tpohvga_spi_cmds = {
> + .type = PANEL_CMDS_SPI,
> + .on_cmds = tpohvga_on_cmds,
> + .on_cmds_num = ARRAY_SIZE(tpohvga_on_cmds),
> + .off_cmds = tpohvga_off_cmds,
> + .off_cmds_num = ARRAY_SIZE(tpohvga_off_cmds),
> + .config = &tpohvga_spi_cfg,
> +};
struct mmp_panel_cmds and struct mmp_panel_cmd_sets are not generic,
can not cover all spi panel neither. Some spi panel initial sequence
include several cmd arrays and delay is needed between arrays. So an
ops is more proper for LCD driver to hook, instead of feeding
commands.
Standard SPI bus implementation is decent than current implementation.
> +
> +static struct mmp_mode mmp_modes_tpohvga[] = {
> + [0] = {
> + .pixclock_freq = 10394400,
> + .refresh = 60,
> + .xres = 320,
> + .yres = 480,
> + .hsync_len = 10,
> + .left_margin = 15,
> + .right_margin = 10,
> + .vsync_len = 2,
> + .upper_margin = 4,
> + .lower_margin = 2,
> + .invert_pixclock = 1,
> + .pix_fmt_out = PIXFMT_RGB565,
> + },
> +};
> +
> +static int tpohvga_get_modelist(struct mmp_panel *panel,
> + struct mmp_mode **modelist)
> +{
> + *modelist = mmp_modes_tpohvga;
> + return 1;
> +}
> +
> +static struct mmp_panel panel_tpohvga = {
> + .name = "tpohvga",
> + .panel_type = PANELTYPE_Active,
> + .get_modelist = tpohvga_get_modelist,
> + .cmd_set = &tpohvga_spi_cmds,
> +};
> +
> +static int __devinit tpohvga_probe(struct platform_device *pdev)
> +{
> + struct mmp_mach_panel_info *mi;
> + /* get configs from platform data */
> + mi = pdev->dev.platform_data;
> + if (mi = NULL) {
> + dev_err(&pdev->dev, "%s: no platform data defined\n", __func__);
> + return -EINVAL;
> + }
> +
> + panel_tpohvga.plat_set_onoff = mi->plat_set_onoff;
> + panel_tpohvga.plat_path_name = mi->plat_path_name;
> + panel_tpohvga.dev = &pdev->dev;
> +
> + if (!mmp_register_panel(&panel_tpohvga)) {
> + dev_err(&pdev->dev, "%s: register failed\n", __func__);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static struct platform_driver panel_tpohvga_driver = {
> + .driver = {
> + .name = "tpo-hvga",
> + .owner = THIS_MODULE,
> + },
> + .probe = tpohvga_probe,
> +};
> +
> +static int __devinit panel_tpohvga_init(void)
> +{
> + return platform_driver_register(&panel_tpohvga_driver);
> +}
> +module_init(panel_tpohvga_init);
> +
> +MODULE_AUTHOR("Lisa Du<cldu@marvell.com>");
> +MODULE_DESCRIPTION("Panel driver for tpohvga");
> +MODULE_LICENSE("GPL");
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 13/17] Revert "OMAPDSS: APPLY: add fifo-merge support"
From: Tomi Valkeinen @ 2012-09-06 12:55 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <5047570D.1030601@ti.com>
[-- Attachment #1: Type: text/plain, Size: 850 bytes --]
On Wed, 2012-09-05 at 19:13 +0530, Archit Taneja wrote:
> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
> > This reverts commit 1d71f42b35ed66d90a9a39bc515bb16cfe2d4a46.
> >
> > Adding fifo merge feature as an omapdss internal configuration was a
> > mistake. We cannot hide from the users of omapdss the complexities of
> > fifo merge.
> >
> > This commit removes the fifo merge support, which luckily is easily done
> > as it was handled totally inside apply.c.
> >
> > The plan is to try fifo merge again later when it is more clear how the
> > hardware acts in various situations, and how the omapdrm wants to use
> > fifo merge.
>
> Nitpick: This isn't an exact revert right? Maybe we should mention that,
> so that if someone tries to apply the old patch, it doesn't fail.
Yep, I added a note.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 15/17] OMAPDSS: remove extra_info completion code
From: Tomi Valkeinen @ 2012-09-06 13:04 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <50475444.2080509@ti.com>
[-- Attachment #1: Type: text/plain, Size: 974 bytes --]
On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote:
> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
> > Now that fifo merge has been removed, nobody uses the extra_info related
> > completion code, which can be removed.
>
> I think this might come into use when we fix the usage of channel out
> field. That is, since channel out is an immediate write field, when we
> set a new manager for an overlay, we may need to wait till the overlay
> disable kicks in, only then we should change channel out.
>
> For this, we would need some wait for extra_info, right?
Hmm, yes, I think you are right. Previously the ovl_disable waited until
the overlay is not used anymore, but now it doesn't.
So I think I need to add wait_pending_extra_info_updates() call to the
beginning of dss_ovl_set_manager(). Or, should it be in unset_manager...
I think unset is better, then a "free" overlay always disabled also in
the HW level.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC PATCH] OMAPDSS: DISPC: Fix IRQ unregister race
From: Tomi Valkeinen @ 2012-09-06 13:36 UTC (permalink / raw)
To: Dimitar Dimitrov; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1346613123-24053-1-git-send-email-dinuxbg@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]
Hi,
On Sun, 2012-09-02 at 22:12 +0300, Dimitar Dimitrov wrote:
> Very rare kernel crashes are reported on a custom OMAP4 board. Kernel
> panics due to corrupted completion structure while executing
> dispc_irq_wait_handler(). Excerpt from kernel log:
>
> Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP
> Unable to handle kernel paging request at virtual address 00400130
> ...
> PC is at 0xebf205bc
> LR is at __wake_up_common+0x54/0x94
> ...
> (__wake_up_common+0x0/0x94)
> (complete+0x0/0x60)
> (dispc_irq_wait_handler.36902+0x0/0x14)
> (omap_dispc_irq_handler+0x0/0x354)
> (handle_irq_event_percpu+0x0/0x188)
> (handle_irq_event+0x0/0x64)
> (handle_fasteoi_irq+0x0/0x10c)
> (generic_handle_irq+0x0/0x48)
> (asm_do_IRQ+0x0/0xc0)
>
> DISPC IRQ executes callbacks with dispc.irq_lock released. Hence
> unregister_isr() and DISPC IRQ might be running in parallel on different
> CPUs. So there is a chance that a callback is executed even though it
> has been unregistered. As omap_dispc_wait_for_irq_timeout() declares a
> completion on stack, the dispc_irq_wait_handler() callback might try to
> access a completion structure that is invalid. This leads to crashes and
> hangs.
>
> Solution is to divide unregister calls into two sets:
> 1. Non-strict unregistering of callbacks. Callbacks could safely be
> executed after unregistering them. This is the case with unregister
> calls from the IRQ handler itself.
> 2. Strict (synchronized) unregistering. Callbacks are not allowed
> after unregistering. This is the case with completion waiting.
>
> The above solution should satisfy one of the original intentions of the
> driver: callbacks should be able to unregister themselves.
I think it'd be better to create a new function for the nosync version,
and keep the old name for the sync version. The reason for this is to
minimize the amount of changes, as I think this one needs to be applied
to stable kernel trees also.
Also, I think we need similar one for dsi.c, as it has the same kind of
irq handling. But with a quick glance only sync version is needed there.
However, I'm not quite sure about this approach. The fix makes sense,
but it makes me think if the irq handling is designed the wrong way.
While debugging and fixing this, did you think some other irq handling
approach would be saner?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 15/17] OMAPDSS: remove extra_info completion code
From: Tomi Valkeinen @ 2012-09-06 13:42 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <5048A6AA.50009@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]
On Thu, 2012-09-06 at 19:05 +0530, Archit Taneja wrote:
> On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote:
> > On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote:
> >> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
> >>> Now that fifo merge has been removed, nobody uses the extra_info related
> >>> completion code, which can be removed.
> >>
> >> I think this might come into use when we fix the usage of channel out
> >> field. That is, since channel out is an immediate write field, when we
> >> set a new manager for an overlay, we may need to wait till the overlay
> >> disable kicks in, only then we should change channel out.
> >>
> >> For this, we would need some wait for extra_info, right?
> >
> > Hmm, yes, I think you are right. Previously the ovl_disable waited until
> > the overlay is not used anymore, but now it doesn't.
> >
> > So I think I need to add wait_pending_extra_info_updates() call to the
> > beginning of dss_ovl_set_manager(). Or, should it be in unset_manager...
> > I think unset is better, then a "free" overlay always disabled also in
> > the HW level.
>
> Yes, I also think it should be in unset_manager. One option could be to
> leave the wait_pending_extra_info_updates() in ovl_disable itself, as it
> was before. But that would force us to use mutexes there, and we'd
> rather have overlay enabling and disabling as a non blocking thing.
Actually, we do have mutexes there. You are thinking about the prototype
API I have. (I also thought we didn't have mutex there =).
So, in fact, we can have the wait at ovl_disable like it was before. The
prototype API, which cannot block, will not have the wait, but there the
caller (i.e. omapdrm) will have to manage the proper wait.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 15/17] OMAPDSS: remove extra_info completion code
From: Archit Taneja @ 2012-09-06 13:47 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1346936681.2737.61.camel@deskari>
On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote:
> On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote:
>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
>>> Now that fifo merge has been removed, nobody uses the extra_info related
>>> completion code, which can be removed.
>>
>> I think this might come into use when we fix the usage of channel out
>> field. That is, since channel out is an immediate write field, when we
>> set a new manager for an overlay, we may need to wait till the overlay
>> disable kicks in, only then we should change channel out.
>>
>> For this, we would need some wait for extra_info, right?
>
> Hmm, yes, I think you are right. Previously the ovl_disable waited until
> the overlay is not used anymore, but now it doesn't.
>
> So I think I need to add wait_pending_extra_info_updates() call to the
> beginning of dss_ovl_set_manager(). Or, should it be in unset_manager...
> I think unset is better, then a "free" overlay always disabled also in
> the HW level.
Yes, I also think it should be in unset_manager. One option could be to
leave the wait_pending_extra_info_updates() in ovl_disable itself, as it
was before. But that would force us to use mutexes there, and we'd
rather have overlay enabling and disabling as a non blocking thing.
Archit
^ permalink raw reply
* Re: [PATCH 15/17] OMAPDSS: remove extra_info completion code
From: Archit Taneja @ 2012-09-06 14:13 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1346938926.2737.71.camel@deskari>
On Thursday 06 September 2012 07:12 PM, Tomi Valkeinen wrote:
> On Thu, 2012-09-06 at 19:05 +0530, Archit Taneja wrote:
>> On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote:
>>> On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote:
>>>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
>>>>> Now that fifo merge has been removed, nobody uses the extra_info related
>>>>> completion code, which can be removed.
>>>>
>>>> I think this might come into use when we fix the usage of channel out
>>>> field. That is, since channel out is an immediate write field, when we
>>>> set a new manager for an overlay, we may need to wait till the overlay
>>>> disable kicks in, only then we should change channel out.
>>>>
>>>> For this, we would need some wait for extra_info, right?
>>>
>>> Hmm, yes, I think you are right. Previously the ovl_disable waited until
>>> the overlay is not used anymore, but now it doesn't.
>>>
>>> So I think I need to add wait_pending_extra_info_updates() call to the
>>> beginning of dss_ovl_set_manager(). Or, should it be in unset_manager...
>>> I think unset is better, then a "free" overlay always disabled also in
>>> the HW level.
>>
>> Yes, I also think it should be in unset_manager. One option could be to
>> leave the wait_pending_extra_info_updates() in ovl_disable itself, as it
>> was before. But that would force us to use mutexes there, and we'd
>> rather have overlay enabling and disabling as a non blocking thing.
>
> Actually, we do have mutexes there. You are thinking about the prototype
> API I have. (I also thought we didn't have mutex there =).
Ah, I missed looking at that :)
>
> So, in fact, we can have the wait at ovl_disable like it was before. The
> prototype API, which cannot block, will not have the wait, but there the
> caller (i.e. omapdrm) will have to manage the proper wait.
I'm more inclined towards waiting in the unset_manager() now, we have a
choice between "wait in ovl_disable, ensure the overlay is actually
disabled in hw, and then get out" and "wait only when you know you need
to wait (i.e, in unset_manager)". The second choice seems more efficient.
This wait would could last for a 1 VSYNC if we do it in ovl_disable. If
the next task of the user of DSS is to enable another overlay, this wait
would unnecessarily delay the enabling of the second overlay by a VSYNC.
We could have done these tasks in the same VSYNC (since we aren't
supporting fifomerge).
So, I feel that we should rather wait in unset_manager, where we know an
immediate write can mess things up. Maybe, we could delay it set_manager
too. But yeah, we won't know whether we are aligned with hw or not.
So even with the prototype API, where omapdrm is responsible for doing
the waiting, it should probably wait when switching the manager, rather
than when disabling the overlay.
Archit
^ permalink raw reply
* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
From: Heiko Stübner @ 2012-09-06 14:14 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Stephen Warren, Thierry Reding, 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-pm, linux-doc
In-Reply-To: <1346412846-17102-2-git-send-email-acourbot@nvidia.com>
Hi Alexander,
Am Freitag, 31. August 2012, 13:34:03 schrieb Alexandre Courbot:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
>
> With the advent of the device tree 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>
I really like this idea, also because I'll have to solve similar problems on
the way to dt for my tinker-platform.
For your power_seq_run function you write that it simply returns an error code
on failure and looking through it I also just found the error return
statement. This would leave a device half turned on.
So I'm wondering, if it shouldn't turn off all the things it turned on until
the step that produced the error. All your possible step types (execpt the
delay) are booleans, so it should be possible to simply negate them when
backtracking through the previous steps.
Heiko
> ---
> .../devicetree/bindings/power_seq/power_seq.txt | 117 ++++++
> Documentation/power/power_seq.txt | 225 +++++++++++
> drivers/power/Kconfig | 1 +
> drivers/power/Makefile | 1 +
> drivers/power/power_seq/Kconfig | 2 +
> drivers/power/power_seq/Makefile | 1 +
> drivers/power/power_seq/power_seq.c | 446
> +++++++++++++++++++++ drivers/power/power_seq/power_seq_delay.c |
> 51 +++
> drivers/power/power_seq/power_seq_gpio.c | 81 ++++
> drivers/power/power_seq/power_seq_pwm.c | 85 ++++
> drivers/power/power_seq/power_seq_regulator.c | 86 ++++
> include/linux/power_seq.h | 174 ++++++++
> 12 files changed, 1270 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/Kconfig
> create mode 100644 drivers/power/power_seq/Makefile
> create mode 100644 drivers/power/power_seq/power_seq.c
> create mode 100644 drivers/power/power_seq/power_seq_delay.c
> create mode 100644 drivers/power/power_seq/power_seq_gpio.c
> create mode 100644 drivers/power/power_seq/power_seq_pwm.c
> create mode 100644 drivers/power/power_seq/power_seq_regulator.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..d3e3f6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> @@ -0,0 +1,117 @@
> +Runtime Interpreted Power Sequences
> +=================> +
> +Power sequences are sequential descriptions of actions to be performed on
> +power-related resources. Having these descriptions in a precise data
> format +allows us to take much of the board-specific power control code
> out of the +kernel and place it into the device tree instead, making
> kernels less +board-dependant.
> +
> +In the device tree, power sequences are grouped into a set. The set is
> always +declared as the "power-sequences" sub-node of the device node.
> Power sequences +may reference resources declared by that device.
> +
> +Power Sequences Structure
> +-------------------------
> +Every device that makes use of power sequences must have a
> "power-sequences" +sub-node. Power sequences are sub-nodes of this set
> node, and their node name +indicates the id of the sequence.
> +
> +Every power sequence in turn contains its steps as sub-nodes of itself.
> 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
> +---------------------
> +Step of a sequence describes an action to be performed on a resource. They
> +always include a "type" property which indicates what kind of resource
> this +step works on. Depending on the resource type, additional properties
> are defined +to control the action to be performed.
> +
> +"delay" type required properties:
> + - delay_us: delay to wait in microseconds
> +
> +"regulator" type required properties:
> + - id: name of the regulator to use. Regulator is obtained by
> + regulator_get(dev, id)
> + - enable / disable: one of these two empty properties must be present to
> + enable or disable the resource
> +
> +"pwm" type required properties:
> + - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
> + - enable / disable: one of these two empty properties must be present to
> + enable or disable the resource
> +
> +"gpio" type required properties:
> + - number: phandle of the GPIO to use.
> + - enable / disable: one of these two empty properties must be present to
> + enable or disable the resource
> +
> +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 power sequences */
> + pwms = <&pwm 2 5000000>;
> + pwm-names = "backlight";
> + power-supply = <&backlight_reg>;
> +
> + power-sequences {
> + power-on {
> + step0 {
> + type = "regulator";
> + id = "power";
> + enable;
> + };
> + step1 {
> + type = "delay";
> + delay_us = <10000>;
> + };
> + step2 {
> + type = "pwm";
> + id = "backlight";
> + enable;
> + };
> + step3 {
> + type = "gpio";
> + number = <&gpio 28 0>;
> + enable;
> + };
> + };
> +
> + power-off {
> + step0 {
> + type = "gpio";
> + number = <&gpio 28 0>;
> + disable;
> + };
> + step1 {
> + type = "pwm";
> + id = "backlight";
> + disable;
> + };
> + step2 {
> + type = "delay";
> + delay_us = <10000>;
> + };
> + step3 {
> + type = "regulator";
> + id = "power";
> + disable;
> + };
> + };
> + };
> + };
> +
> +The first part lists the PWM and regulator 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. diff --git a/Documentation/power/power_seq.txt
> b/Documentation/power/power_seq.txt new file mode 100644
> index 0000000..48d1f6b
> --- /dev/null
> +++ b/Documentation/power/power_seq.txt
> @@ -0,0 +1,225 @@
> +Runtime Interpreted Power Sequences
> +=================> +
> +Problem
> +-------
> +Very commonly, boards need the help of out-of-driver code to turn some of
> their +devices on and 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 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) aim
> at +turning this code into platform data or device tree nodes. Power
> sequences are +described using a simple format and run by a lightweight
> 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 and actions operations
> are: +- delay (just wait for a given number of microseconds)
> +- GPIO (enable or disable)
> +- regulator (enable or disable)
> +- PWM (enable or disable)
> +
> +When a power sequence is run, each of its steps is executed sequentially
> until +one step fails or the end of the sequence is reached.
> +
> +Power sequences are grouped in "sets" and declared per-device. Every
> sequence +must be attributed a name that can be used to retrieve it from
> its set when it +is needed.
> +
> +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 for a given device is an instance of
> platform_power_seq_set +which points to instances of platform_power_seq.
> Every platform_power_seq is a +single power sequence, and is itself
> composed of a variable length array of +steps.
> +
> +A step is a union of all the step structures. Which one is to be used
> depends on +the type of the step. Step structures are documented in the
> +include/linux/power_seq.h file ; please refer to it for all details, but
> the +following example will probably make it clear how power sequences
> should be +defined. It defines two power sequences named "power_on" and
> "power_off". The +"power_on" sequence enables a regulator called "power"
> (retrieved from the +device using regulator_get()), waits for 10ms, and
> then enabled GPIO 110. +"power_off" does the opposite.
> +
> +static struct platform_power_seq power_on_seq = {
> + .id = "power_on",
> + .num_steps = 3,
> + .steps = {
> + {
> + .type = POWER_SEQ_REGULATOR,
> + .regulator = {
> + .id = "power",
> + .enable = true,
> + },
> + },
> + {
> + .type = POWER_SEQ_DELAY,
> + .delay = {
> + .delay_us = 10000,
> + },
> + },
> + {
> + .type = POWER_SEQ_GPIO,
> + .gpio = {
> + .number = 110,
> + .enable = true,
> + },
> + },
> + },
> +};
> +
> +static struct platform_power_seq power_off_seq = {
> + .id = "power_off",
> + .num_steps = 3,
> + .steps = {
> + {
> + .type = POWER_SEQ_GPIO,
> + .gpio = {
> + .number = 110,
> + .enable = false,
> + },
> + },
> + {
> + .type = POWER_SEQ_DELAY,
> + .delay = {
> + .delay_us = 10000,
> + },
> + },
> + {
> + .type = POWER_SEQ_REGULATOR,
> + .regulator = {
> + .id = "power",
> + .enable = false,
> + },
> + },
> + },
> +};
> +
> +static struct platform_power_seq_set power_sequences = {
> + .num_seqs = 2,
> + .seqs = {
> + &power_on_seq,
> + &power_off_seq,
> + },
> +};
> +
> +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>;
> +
> +power-sequences {
> + power-on {
> + step0 {
> + type = "regulator";
> + id = "power";
> + enable;
> + };
> + step1 {
> + type = "delay";
> + delay = <10000>;
> + };
> + step2 {
> + type = "gpio";
> + number = <&gpio 110 0>;
> + enable;
> + };
> + }
> + power-off {
> + step0 {
> + type = "gpio";
> + number = <&gpio 110 0>;
> + disable;
> + };
> + step1 {
> + type = "delay";
> + delay = <10000>;
> + };
> + step2 {
> + type = "regulator";
> + id = "power";
> + disable;
> + };
> + }
> +};
> +
> +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 devm_power_seq_set_build() function builds a power sequence
> set +from platform data. It also takes care of resolving and allocating
> the resources +referenced by the sequence:
> +
> + struct power_seq_set *devm_power_seq_set_build(struct device *dev,
> + struct platform_power_seq_set *pseq);
> +
> +As its name states, all memory and resources are devm-allocated. The 'dev'
> +argument is the device in the name of which the resources are to be
> allocated. +
> +On success, the function returns a devm allocated resolved sequences set
> for +which all the resources are allocated. In case of failure, an error
> code is +returned.
> +
> +Power sequences can then be retrieved by their name using
> power_seq_lookup: +
> + struct power_seq *power_seq_lookup(struct power_seq_set *seqs,
> + const char *id);
> +
> +A power sequence can be executed by power_seq_run:
> +
> + int power_seq_run(struct power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code
> if a +problem occured.
> +
> +Sometimes, you may want to browse the list of resources allocated by a
> sequence, +to for instance ensure that a resource of a given type is
> present. The +power_seq_set_resources() function returns a list head that
> can be used with +the power_seq_for_each_resource() macro to browse all
> the resources of a set: +
> + struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
> + power_seq_for_each_resource(pos, seqs)
> +
> +Here "pos" will be of type struct power_seq_resource. This structure
> contains a +"pdata" pointer that can be used to explore the platform data
> of this resource, +as well as the resolved resource, if applicable.
> +
> +Finally, users of the device tree can build the platform data
> corresponding to +the tree node using this function:
> +
> + struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device
> *dev); +
> +As the device tree syntax unambiguously states the name of the node
> containing +the power sequences, it only needs a pointer to the device to
> work. The result +can then be passed to devm_power_seq_set_build() in
> order to get a set of +runnable sequences.
> +
> +devm_of_parse_power_seq_set allocates its memory using devm, but the
> platform +data becomes unneeded after devm_power_seq_set_build() is called
> on it and can +thus be freed. Be aware though that one allocation is
> performed for the set and +for every sequence. The
> devm_power_seq_platform_data_free() function takes care +of freeing the
> memory properly:
> +
> + void devm_platform_power_seq_set_free(struct platform_power_seq_set
> *pseq); diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index fcc1bb0..5fdfd84 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -312,3 +312,4 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
> endif # POWER_SUPPLY
>
> source "drivers/power/avs/Kconfig"
> +source "drivers/power/power_seq/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index ee58afb..d3c893b 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/
> diff --git a/drivers/power/power_seq/Kconfig
> b/drivers/power/power_seq/Kconfig new file mode 100644
> index 0000000..3bff26e
> --- /dev/null
> +++ b/drivers/power/power_seq/Kconfig
> @@ -0,0 +1,2 @@
> +config POWER_SEQ
> + bool
> diff --git a/drivers/power/power_seq/Makefile
> b/drivers/power/power_seq/Makefile new file mode 100644
> index 0000000..f77a359
> --- /dev/null
> +++ b/drivers/power/power_seq/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_POWER_SEQ) += power_seq.o
> diff --git a/drivers/power/power_seq/power_seq.c
> b/drivers/power/power_seq/power_seq.c new file mode 100644
> index 0000000..e4d482c
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq.c
> @@ -0,0 +1,446 @@
> +/*
> + * 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/of.h>
> +
> +struct power_seq_set {
> + struct device *dev;
> + struct list_head resources;
> + struct list_head sequences;
> +};
> +
> +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 {
> + /* Set this sequence belongs to */
> + struct power_seq_set *parent_set;
> + const char *id;
> + /* To thread into power_seqs structure */
> + struct list_head list;
> + unsigned int num_steps;
> + struct power_seq_step steps[];
> +};
> +
> +#define power_seq_err(dev, seq, step_nbr, format, ...) \
> + dev_err(dev, "%s[%d]: " format, seq->id, step_nbr, ##__VA_ARGS__);
> +
> +/**
> + * struct power_seq_res_type - operators for power sequences resources
> + * @name: Name of the resource type. Set to null when a resource
> + * type support is not compiled in
> + * @need_resource: Whether a resource needs to be allocated when steps of
> + * this kind are met. If set to false, res_compare and
> + * res_alloc need not be set
> + * @of_parse: Parse a step for this kind of resource from a device
> + * tree node. The result of parsing must be written into
> + * step step_nbr of seq
> + * @step_run: Run a step for this kind of resource
> + * @res_compare: Return true if the resource used by both steps is the
> + * same, false otherwise
> + * @res_alloc: Resolve and allocate the resource passed from seq
> + * Return error code if the resource cannot be allocated
> + */
> +struct power_seq_res_ops {
> + const char *name;
> + bool need_resource;
> + int (*of_parse)(struct device *dev, struct device_node *node,
> + struct platform_power_seq *seq, unsigned int step_nbr);
> + int (*step_run)(struct power_seq_step *step);
> + bool (*res_compare)(struct platform_power_seq_step *step1,
> + struct platform_power_seq_step *step2);
> + int (*res_alloc)(struct device *dev, struct power_seq_resource *seq);
> +};
> +
> +static const struct power_seq_res_ops
> power_seq_types[POWER_SEQ_NUM_TYPES]; +
> +#ifdef CONFIG_OF
> +static int of_power_seq_parse_enable_properties(struct device *dev,
> + struct device_node *node,
> + struct platform_power_seq *seq,
> + unsigned int step_nbr,
> + bool *enable)
> +{
> + if (of_find_property(node, "enable", NULL)) {
> + *enable = true;
> + } else if (of_find_property(node, "disable", NULL)) {
> + *enable = false;
> + } else {
> + power_seq_err(dev, seq, step_nbr,
> + "missing enable or disable property\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int of_power_seq_parse_step(struct device *dev, struct device_node
> *node, + struct platform_power_seq *seq,
> + unsigned int step_nbr)
> +{
> + struct platform_power_seq_step *step = &seq->steps[step_nbr];
> + const char *type;
> + int i, err;
> +
> + err = of_property_read_string(node, "type", &type);
> + if (err < 0) {
> + power_seq_err(dev, seq, step_nbr,
> + "cannot read type property\n");
> + return err;
> + }
> + for (i = 0; i < POWER_SEQ_NUM_TYPES; i++) {
> + if (power_seq_types[i].name = NULL)
> + continue;
> + if (!strcmp(type, power_seq_types[i].name))
> + break;
> + }
> + if (i >= POWER_SEQ_NUM_TYPES) {
> + power_seq_err(dev, seq, step_nbr, "unknown type %s\n", type);
> + return -EINVAL;
> + }
> + step->type = i;
> + err = power_seq_types[step->type].of_parse(dev, node, seq, step_nbr);
> +
> + return err;
> +}
> +
> +static 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 num_steps, sz;
> + int err;
> +
> + if (!node)
> + return ERR_PTR(-EINVAL);
> +
> + num_steps = of_get_child_count(node);
> + sz = sizeof(*pseq) + sizeof(pseq->steps[0]) * num_steps;
> + pseq = devm_kzalloc(dev, sz, GFP_KERNEL);
> + if (!pseq)
> + return ERR_PTR(-ENOMEM);
> + pseq->num_steps = num_steps;
> + pseq->id = node->name;
> +
> + 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 = kstrtouint(child->name + 4, 10, &pos);
> + if (err < 0)
> + goto parse_error;
> +
> + if (pos >= num_steps || pseq->steps[pos].type != 0) {
> + err = -EINVAL;
> + goto parse_error;
> + }
> +
> + err = of_power_seq_parse_step(dev, child, pseq, pos);
> + if (err)
> + return ERR_PTR(err);
> + }
> +
> + return pseq;
> +
> +parse_error:
> + dev_err(dev, "%s: invalid power step name %s!\n", pseq->id,
> + child->name);
> + return ERR_PTR(err);
> +}
> +
> +/**
> + * of_parse_power_seq_set() - build platform data corresponding to a DT
> node + * @dev: Device on behalf of which the sequence is to be built
> + *
> + * Sequences must be contained into a subnode named "power-sequences" of
> the + * device root node.
> + *
> + * Memory for the platform sequence is allocated using devm_kzalloc on dev
> and + * can be freed by devm_kfree after power_seq_set_build returned.
> Beware that on + * top of the set itself, platform data for individual
> sequences should also be + * freed.
> + *
> + * Returns the built power sequence set on success, or an error code in
> case of + * failure.
> + */
> +struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device
> *dev) +{
> + struct platform_power_seq_set *seqs;
> + struct device_node *root = dev->of_node;
> + struct device_node *seq;
> + int num_seqs, sz, i = 0;
> +
> + if (!root)
> + return NULL;
> +
> + root = of_find_node_by_name(root, "power-sequences");
> + if (!root)
> + return NULL;
> +
> + num_seqs = of_get_child_count(root);
> + sz = sizeof(*seqs) + sizeof(seqs->seqs[0]) * num_seqs;
> + seqs = devm_kzalloc(dev, sz, GFP_KERNEL);
> + if (!seqs)
> + return ERR_PTR(-ENOMEM);
> + seqs->num_seqs = num_seqs;
> +
> + for_each_child_of_node(root, seq) {
> + struct platform_power_seq *pseq;
> +
> + pseq = of_parse_power_seq(dev, seq);
> + if (IS_ERR(pseq))
> + return (void *)pseq;
> +
> + seqs->seqs[i++] = pseq;
> + }
> +
> + return seqs;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_parse_power_seq_set);
> +#endif /* CONFIG_OF */
> +
> +/**
> + * devm_platform_power_seq_set_free() - free data allocated by
> of_parse_power_seq_set + * @pseq: Platform data to free
> + *
> + * This function can be called *only* on data returned by
> of_parse_power_seq_set + * and *after* devm_power_seq_set_build has been
> called on it.
> + */
> +void devm_platform_power_seq_set_free(struct device *dev,
> + struct platform_power_seq_set *pseq)
> +{
> + int i;
> +
> + for (i = 0; i < pseq->num_seqs; i++)
> + devm_kfree(dev, pseq->seqs[i]);
> +
> + devm_kfree(dev, pseq);
> +}
> +EXPORT_SYMBOL_GPL(devm_platform_power_seq_set_free);
> +
> +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;
> +
> + if (power_seq_types[pdata->type].res_compare(pdata, step))
> + return res;
> + }
> +
> + return NULL;
> +}
> +
> +static struct power_seq *power_seq_build_one(struct device *dev,
> + struct power_seq_set *seqs,
> + struct platform_power_seq *pseq)
> +{
> + struct power_seq *seq;
> + struct power_seq_resource *res;
> + int i, err;
> +
> + seq = devm_kzalloc(dev, sizeof(*seq) + sizeof(seq->steps[0]) *
> + pseq->num_steps, GFP_KERNEL);
> + if (!seq)
> + return ERR_PTR(-ENOMEM);
> +
> + INIT_LIST_HEAD(&seq->list);
> + seq->parent_set = seqs;
> + seq->num_steps = pseq->num_steps;
> + seq->id = pseq->id;
> +
> + for (i = 0; i < seq->num_steps; i++) {
> + struct platform_power_seq_step *pstep = &pseq->steps[i];
> + struct power_seq_step *step = &seq->steps[i];
> +
> + if (pstep->type >= POWER_SEQ_NUM_TYPES ||
> + power_seq_types[pstep->type].name = NULL) {
> + power_seq_err(dev, seq, i,
> + "invalid power sequence type %d!",
> + pstep->type);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + memcpy(&step->pdata, pstep, sizeof(step->pdata));
> +
> + /* Steps without resource need not to continue */
> + if (!power_seq_types[pstep->type].need_resource)
> + continue;
> +
> + /* create resource node if not referenced already */
> + res = power_seq_find_resource(&seqs->resources, pstep);
> + if (!res) {
> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return ERR_PTR(-ENOMEM);
> +
> + res->pdata = &step->pdata;
> +
> + err = power_seq_types[res->pdata->type].res_alloc(dev, res);
> + if (err < 0) {
> + power_seq_err(dev, seq, i,
> + "error building sequence\n");
> + return ERR_PTR(err);
> + }
> +
> + list_add_tail(&res->list, &seqs->resources);
> + }
> + step->resource = res;
> + }
> +
> + return seq;
> +}
> +
> +/**
> + * power_seq_set_build() - build a set of runnable sequences from platform
> data + * @dev: Device that will use the power sequences. All resources
> will be + * devm-allocated against it
> + * @pseq: Platform data for the power sequences. It can be freed after
> + * this function returns
> + *
> + * All memory and resources (regulators, GPIOs, etc.) are allocated using
> devm + * functions.
> + *
> + * Returns the built sequence on success, an error code in case or
> failure. + */
> +struct power_seq_set *devm_power_seq_set_build(struct device *dev,
> + struct platform_power_seq_set *pseq)
> +{
> + struct power_seq_set *seqs;
> + int i;
> +
> + seqs = devm_kzalloc(dev, sizeof(*seqs), GFP_KERNEL);
> +
> + if (!seqs)
> + return ERR_PTR(-ENOMEM);
> +
> + INIT_LIST_HEAD(&seqs->resources);
> + INIT_LIST_HEAD(&seqs->sequences);
> + for (i = 0; i < pseq->num_seqs; i++) {
> + struct power_seq *seq;
> +
> + seq = power_seq_build_one(dev, seqs, pseq->seqs[i]);
> + if (IS_ERR(seq))
> + return (void *)seq;
> +
> + list_add_tail(&seq->list, &seqs->sequences);
> + }
> +
> + return seqs;
> +}
> +EXPORT_SYMBOL_GPL(devm_power_seq_set_build);
> +
> +/**
> + * power_seq_lookup - Lookup a power sequence by name from a set
> + * @seqs: The set to look in
> + * @id: Name to look after
> + *
> + * Returns a matching power sequence if it exists, NULL if it does not.
> + */
> +struct power_seq *power_seq_lookup(struct power_seq_set *seqs, const char
> *id) +{
> + struct power_seq *seq;
> +
> + list_for_each_entry(seq, &seqs->sequences, list) {
> + if (!strcmp(seq->id, id))
> + return seq;
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_lookup);
> +
> +/**
> + * power_seq_set_resources - return a list of all the resources used by a
> set + * @seqs: Power sequences set we are interested in getting the
> resources + *
> + * The returned list can be parsed using the power_seq_for_each_resource
> macro. + */
> +struct list_head *power_seq_set_resources(struct power_seq_set *seqs)
> +{
> + return &seqs->resources;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_set_resources);
> +
> +/**
> + * power_seq_run() - run a power sequence
> + * @seq: The power sequence to run
> + *
> + * Returns 0 on success, error code in case of failure.
> + */
> +int power_seq_run(struct power_seq *seq)
> +{
> + unsigned int i;
> + int err;
> +
> + if (!seq)
> + return 0;
> +
> + for (i = 0; i < seq->num_steps; i++) {
> + unsigned int type = seq->steps[i].pdata.type;
> +
> + err = power_seq_types[type].step_run(&seq->steps[i]);
> + if (err) {
> + power_seq_err(seq->parent_set->dev, seq, i,
> + "error %d while running power sequence step\n",
> + err);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(power_seq_run);
> +
> +#include "power_seq_delay.c"
> +#include "power_seq_regulator.c"
> +#include "power_seq_pwm.c"
> +#include "power_seq_gpio.c"
> +
> +static const struct power_seq_res_ops power_seq_types[POWER_SEQ_NUM_TYPES]
> = { + [POWER_SEQ_DELAY] = POWER_SEQ_DELAY_TYPE,
> + [POWER_SEQ_REGULATOR] = POWER_SEQ_REGULATOR_TYPE,
> + [POWER_SEQ_PWM] = POWER_SEQ_PWM_TYPE,
> + [POWER_SEQ_GPIO] = POWER_SEQ_GPIO_TYPE,
> +};
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
> +MODULE_DESCRIPTION("Runtime Interpreted Power Sequences");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/power/power_seq/power_seq_delay.c
> b/drivers/power/power_seq/power_seq_delay.c new file mode 100644
> index 0000000..072bf50
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_delay.c
> @@ -0,0 +1,51 @@
> +/*
> + * 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/delay.h>
> +
> +#ifdef CONFIG_OF
> +static int of_power_seq_parse_delay(struct device *dev,
> + struct device_node *node,
> + struct platform_power_seq *seq,
> + unsigned int step_nbr)
> +{
> + struct platform_power_seq_step *step = &seq->steps[step_nbr];
> + int err;
> +
> + err = of_property_read_u32(node, "delay_us",
> + &step->delay.delay_us);
> + if (err < 0)
> + power_seq_err(dev, seq, step_nbr,
> + "error reading delay_us property\n");
> +
> + return err;
> +}
> +#else
> +#define of_power_seq_parse_delay NULL
> +#endif
> +
> +static int power_seq_step_run_delay(struct power_seq_step *step)
> +{
> + usleep_range(step->pdata.delay.delay_us,
> + step->pdata.delay.delay_us + 1000);
> +
> + return 0;
> +}
> +
> +#define POWER_SEQ_DELAY_TYPE { \
> + .name = "delay", \
> + .need_resource = false, \
> + .of_parse = of_power_seq_parse_delay, \
> + .step_run = power_seq_step_run_delay, \
> +}
> diff --git a/drivers/power/power_seq/power_seq_gpio.c
> b/drivers/power/power_seq/power_seq_gpio.c new file mode 100644
> index 0000000..2e9a49f
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_gpio.c
> @@ -0,0 +1,81 @@
> +/*
> + * 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/gpio.h>
> +#include <linux/of_gpio.h>
> +
> +#ifdef CONFIG_OF
> +static int power_seq_of_parse_gpio(struct device *dev,
> + struct device_node *node,
> + struct platform_power_seq *seq,
> + unsigned int step_nbr)
> +{
> + struct platform_power_seq_step *step = &seq->steps[step_nbr];
> + int gpio;
> + int err;
> +
> + gpio = of_get_named_gpio(node, "number", 0);
> + if (gpio < 0) {
> + power_seq_err(dev, seq, step_nbr,
> + "error reading number property\n");
> + return gpio;
> + }
> + step->gpio.number = gpio;
> +
> + err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
> + &step->gpio.enable);
> +
> + return err;
> +}
> +#else
> +#define of_power_seq_parse_gpio NULL
> +#endif
> +
> +static bool power_seq_res_compare_gpio(struct platform_power_seq_step
> *step1, + struct platform_power_seq_step *step2)
> +{
> + return step1->gpio.number = step2->gpio.number;
> +}
> +
> +static int power_seq_res_alloc_gpio(struct device *dev,
> + struct power_seq_resource *res)
> +{
> + int err;
> +
> + err = devm_gpio_request_one(dev, res->pdata->gpio.number,
> + GPIOF_OUT_INIT_LOW, dev_name(dev));
> + if (err) {
> + dev_err(dev, "cannot get gpio %d\n", res->pdata->gpio.number);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int power_seq_step_run_gpio(struct power_seq_step *step)
> +{
> + gpio_set_value_cansleep(step->pdata.gpio.number,
> + step->pdata.gpio.enable);
> +
> + return 0;
> +}
> +
> +#define POWER_SEQ_GPIO_TYPE { \
> + .name = "gpio", \
> + .need_resource = true, \
> + .of_parse = power_seq_of_parse_gpio, \
> + .step_run = power_seq_step_run_gpio, \
> + .res_compare = power_seq_res_compare_gpio, \
> + .res_alloc = power_seq_res_alloc_gpio, \
> +}
> diff --git a/drivers/power/power_seq/power_seq_pwm.c
> b/drivers/power/power_seq/power_seq_pwm.c new file mode 100644
> index 0000000..a80514f
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_pwm.c
> @@ -0,0 +1,85 @@
> +/*
> + * 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.
> + *
> + */
> +
> +#ifdef CONFIG_PWM
> +
> +#include <linux/pwm.h>
> +
> +#ifdef CONFIG_OF
> +static int power_seq_of_parse_pwm(struct device *dev,
> + struct device_node *node,
> + struct platform_power_seq *seq,
> + unsigned int step_nbr)
> +{
> + struct platform_power_seq_step *step = &seq->steps[step_nbr];
> + int err;
> +
> + err = of_property_read_string(node, "id",
> + &step->pwm.id);
> + if (err) {
> + power_seq_err(dev, seq, step_nbr,
> + "error reading id property\n");
> + return err;
> + }
> +
> + err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
> + &step->pwm.enable);
> + return err;
> +}
> +#else
> +#define of_power_seq_parse_pwm NULL
> +#endif
> +
> +static bool power_seq_res_compare_pwm(struct platform_power_seq_step
> *step1, + struct platform_power_seq_step *step2)
> +{
> + return (!strcmp(step1->pwm.id, step2->pwm.id));
> +}
> +
> +static int power_seq_res_alloc_pwm(struct device *dev,
> + struct power_seq_resource *res)
> +{
> + res->pwm = devm_pwm_get(dev, res->pdata->pwm.id);
> + if (IS_ERR(res->pwm)) {
> + dev_err(dev, "cannot get pwm \"%s\"\n", res->pdata->pwm.id);
> + return PTR_ERR(res->pwm);
> + }
> +
> + return 0;
> +}
> +
> +static int power_seq_step_run_pwm(struct power_seq_step *step)
> +{
> + if (step->pdata.gpio.enable) {
> + return pwm_enable(step->resource->pwm);
> + } else {
> + pwm_disable(step->resource->pwm);
> + return 0;
> + }
> +}
> +
> +#define POWER_SEQ_PWM_TYPE { \
> + .name = "pwm", \
> + .need_resource = true, \
> + .of_parse = power_seq_of_parse_pwm, \
> + .step_run = power_seq_step_run_pwm, \
> + .res_compare = power_seq_res_compare_pwm, \
> + .res_alloc = power_seq_res_alloc_pwm, \
> +}
> +
> +#else
> +
> +#define POWER_SEQ_PWM_TYPE {}
> +
> +#endif
> diff --git a/drivers/power/power_seq/power_seq_regulator.c
> b/drivers/power/power_seq/power_seq_regulator.c new file mode 100644
> index 0000000..915eac1
> --- /dev/null
> +++ b/drivers/power/power_seq/power_seq_regulator.c
> @@ -0,0 +1,86 @@
> +/*
> + * 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.
> + *
> + */
> +
> +#ifdef CONFIG_REGULATOR
> +
> +#include <linux/regulator/consumer.h>
> +
> +/* TODO change "which" */
> +#ifdef CONFIG_OF
> +static int power_seq_of_parse_regulator(struct device *dev,
> + struct device_node *node,
> + struct platform_power_seq *seq,
> + unsigned int step_nbr)
> +{
> + struct platform_power_seq_step *step = &seq->steps[step_nbr];
> + int err;
> +
> + err = of_property_read_string(node, "id",
> + &step->regulator.id);
> + if (err) {
> + power_seq_err(dev, seq, step_nbr,
> + "error reading id property\n");
> + return err;
> + }
> +
> + err = of_power_seq_parse_enable_properties(dev, node, seq, step_nbr,
> + &step->regulator.enable);
> + return err;
> +}
> +#else
> +#define of_power_seq_parse_regulator NULL
> +#endif
> +
> +static bool
> +power_seq_res_compare_regulator(struct platform_power_seq_step *step1,
> + struct platform_power_seq_step *step2)
> +{
> + return (!strcmp(step1->regulator.id, step2->regulator.id));
> +}
> +
> +static int power_seq_res_alloc_regulator(struct device *dev,
> + struct power_seq_resource *res)
> +{
> + res->regulator = devm_regulator_get(dev, res->pdata->regulator.id);
> + if (IS_ERR(res->regulator)) {
> + dev_err(dev, "cannot get regulator \"%s\"\n",
> + res->pdata->regulator.id);
> + return PTR_ERR(res->regulator);
> + }
> +
> + return 0;
> +}
> +
> +static int power_seq_step_run_regulator(struct power_seq_step *step)
> +{
> + if (step->pdata.regulator.enable)
> + return regulator_enable(step->resource->regulator);
> + else
> + return regulator_disable(step->resource->regulator);
> +}
> +
> +#define POWER_SEQ_REGULATOR_TYPE { \
> + .name = "regulator", \
> + .need_resource = true, \
> + .of_parse = power_seq_of_parse_regulator, \
> + .step_run = power_seq_step_run_regulator, \
> + .res_compare = power_seq_res_compare_regulator, \
> + .res_alloc = power_seq_res_alloc_regulator, \
> +}
> +
> +#else
> +
> +#define POWER_SEQ_REGULATOR_TYPE {}
> +
> +#endif
> diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
> new file mode 100644
> index 0000000..78e8d77
> --- /dev/null
> +++ b/include/linux/power_seq.h
> @@ -0,0 +1,174 @@
> +/*
> + * 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_NUM_TYPES,
> +};
> +
> +/**
> + * struct platform_power_seq_delay_step - platform data for delay steps
> + * @delay_us: Amount of time to wait, in microseconds.
> + */
> +struct platform_power_seq_delay_step {
> + unsigned int delay_us;
> +};
> +
> +/**
> + * struct platform_power_seq_regulator_step - platform data for regulator
> steps + * @id: Name of the regulator to use. The regulator will be
> obtained + * using devm_regulator_get(dev, name)
> + * @enable: Whether to enable or disable the regulator during this step
> + */
> +struct platform_power_seq_regulator_step {
> + const char *id;
> + bool enable;
> +};
> +
> +/**
> + * struct platform_power_seq_pwm_step - platform data for PWM steps
> + * @id: Name of the pwm to use. The PWM will be obtained using
> + * devm_pwm_get(dev, name)
> + * @enable: Whether to enable or disable the PWM during this step
> + */
> +struct platform_power_seq_pwm_step {
> + const char *id;
> + bool enable;
> +};
> +
> +/**
> + * struct platform_power_seq_gpio_step - platform data for GPIO steps
> + * @number: Number of the GPIO to use. The GPIO will be obtained using
> + * devm_gpio_request_one(dev, number)
> + * @enable: Whether to enable or disable the GPIO during this step
> + */
> +struct platform_power_seq_gpio_step {
> + int number;
> + bool enable;
> +};
> +
> +/**
> + * struct platform_power_seq_step - platform data for power sequences
> steps + * @type: The type of this step. This decides which member of the
> union is + * valid for this step.
> + * @delay: Used if type = POWER_SEQ_DELAY
> + * @regulator: Used if type = POWER_SEQ_REGULATOR
> + * @pwm: Used if type = POWER_SEQ_PWN
> + * @gpio: Used if type = POWER_SEQ_GPIO
> + */
> +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 - platform data for power sequences
> + * @id: Name through which this sequence is refered
> + * @num_steps: Number of steps in that sequence
> + * @steps: Array of num_steps steps describing the sequence
> + */
> +struct platform_power_seq {
> + const char *id;
> + unsigned int num_steps;
> + struct platform_power_seq_step steps[];
> +};
> +
> +/**
> + * struct platform_power_seq_set - platform data for sets of sequences
> + * @num_seqs: Number of sequences in this set
> + * @seqs: Array of pointers to individual sequences
> + */
> +struct platform_power_seq_set {
> + unsigned int num_seqs;
> + struct platform_power_seq* seqs[];
> +};
> +
> +/**
> + * struct power_seq_resource - resource used by a power sequence set
> + * @pdata: Pointer to the platform data used to resolve this resource
> + * @regulator: Resolved regulator if of type POWER_SEQ_REGULATOR
> + * @pwm: Resolved PWM if of type POWER_SEQ_PWM
> + * @list: Used to link resources together
> + */
> +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;
> +};
> +#define power_seq_for_each_resource(pos, seqs) \
> + list_for_each_entry(pos, power_seq_set_resources(seqs), list)
> +
> +struct power_seq_resource;
> +struct power_seq;
> +struct power_seq_set;
> +
> +#ifdef CONFIG_OF
> +struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device
> *dev); +#else
> +inline struct platform_power_seq_set *of_parse_power_seq_set(struct device
> *dev) +{
> + return NULL;
> +}
> +#endif
> +void devm_platform_power_seq_set_free(struct device *dev,
> + struct platform_power_seq_set *pseq);
> +
> +struct power_seq_set *devm_power_seq_set_build(struct device *dev,
> + struct platform_power_seq_set *pseq);
> +struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
> +struct power_seq *power_seq_lookup(struct power_seq_set *seqs, const char
> *id); +int power_seq_run(struct power_seq *seq);
> +
> +#endif
^ permalink raw reply
* Re: [PATCH 15/17] OMAPDSS: remove extra_info completion code
From: Tomi Valkeinen @ 2012-09-06 14:29 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <5048ACBC.8010502@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3812 bytes --]
On Thu, 2012-09-06 at 19:31 +0530, Archit Taneja wrote:
> On Thursday 06 September 2012 07:12 PM, Tomi Valkeinen wrote:
> > On Thu, 2012-09-06 at 19:05 +0530, Archit Taneja wrote:
> >> On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote:
> >>> On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote:
> >>>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote:
> >>>>> Now that fifo merge has been removed, nobody uses the extra_info related
> >>>>> completion code, which can be removed.
> >>>>
> >>>> I think this might come into use when we fix the usage of channel out
> >>>> field. That is, since channel out is an immediate write field, when we
> >>>> set a new manager for an overlay, we may need to wait till the overlay
> >>>> disable kicks in, only then we should change channel out.
> >>>>
> >>>> For this, we would need some wait for extra_info, right?
> >>>
> >>> Hmm, yes, I think you are right. Previously the ovl_disable waited until
> >>> the overlay is not used anymore, but now it doesn't.
> >>>
> >>> So I think I need to add wait_pending_extra_info_updates() call to the
> >>> beginning of dss_ovl_set_manager(). Or, should it be in unset_manager...
> >>> I think unset is better, then a "free" overlay always disabled also in
> >>> the HW level.
> >>
> >> Yes, I also think it should be in unset_manager. One option could be to
> >> leave the wait_pending_extra_info_updates() in ovl_disable itself, as it
> >> was before. But that would force us to use mutexes there, and we'd
> >> rather have overlay enabling and disabling as a non blocking thing.
> >
> > Actually, we do have mutexes there. You are thinking about the prototype
> > API I have. (I also thought we didn't have mutex there =).
>
> Ah, I missed looking at that :)
>
> >
> > So, in fact, we can have the wait at ovl_disable like it was before. The
> > prototype API, which cannot block, will not have the wait, but there the
> > caller (i.e. omapdrm) will have to manage the proper wait.
>
> I'm more inclined towards waiting in the unset_manager() now, we have a
> choice between "wait in ovl_disable, ensure the overlay is actually
> disabled in hw, and then get out" and "wait only when you know you need
> to wait (i.e, in unset_manager)". The second choice seems more efficient.
>
> This wait would could last for a 1 VSYNC if we do it in ovl_disable. If
> the next task of the user of DSS is to enable another overlay, this wait
> would unnecessarily delay the enabling of the second overlay by a VSYNC.
> We could have done these tasks in the same VSYNC (since we aren't
> supporting fifomerge).
>
> So, I feel that we should rather wait in unset_manager, where we know an
> immediate write can mess things up. Maybe, we could delay it set_manager
> too. But yeah, we won't know whether we are aligned with hw or not.
Good points. Then again, the wait function doesn't wait for the ovl to
be disabled, it waits for all extra_info changes to be done. So we could
be waiting unnecessarily in unset/set_manager. Which makes me think that
we should remove the current wait function and implement a specialized
wait for ovl disable. But that can be looked at later if seen necessary.
Also, it feels safer to ensure the ovl is disabled at ovl_disable call.
However, I was going through the code and I didn't come up with a case
where it would cause problems, except the set_manager part.
So, I guess having the wait in unset_manager is still best overall, we
don't unnecessarily spend time waiting at ovl_disable.
> So even with the prototype API, where omapdrm is responsible for doing
> the waiting, it should probably wait when switching the manager, rather
> than when disabling the overlay.
Yep.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 7/10] drivers/video/sis/initextlfb.c: removes unnecessary semicolon
From: Peter Senna Tschudin @ 2012-09-06 16:09 UTC (permalink / raw)
To: Thomas Winischhofer
Cc: kernel-janitors, Peter Senna Tschudin, Florian Tobias Schandinat,
linux-fbdev, linux-kernel
In-Reply-To: <1346947757-10481-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
removes unnecessary semicolon
Found by Coccinelle: http://coccinelle.lip6.fr/
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
drivers/video/sis/initextlfb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/video/sis/initextlfb.c b/drivers/video/sis/initextlfb.c
--- a/drivers/video/sis/initextlfb.c
+++ b/drivers/video/sis/initextlfb.c
@@ -65,7 +65,7 @@ sisfb_mode_rate_to_dclock(struct SiS_Pri
}
#endif
- if(!(SiS_SearchModeID(SiS_Pr, &ModeNo, &ModeIdIndex))) {;
+ if(!(SiS_SearchModeID(SiS_Pr, &ModeNo, &ModeIdIndex))) {
printk(KERN_ERR "Could not find mode %x\n", ModeNo);
return 65000;
}
^ permalink raw reply
* Re: [PATCH V5 0/6] OMAPDSS: Cleanup cpu_is checks
From: Tony Lindgren @ 2012-09-06 20:08 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Chandrabhanu Mahapatra, paul, linux-omap, linux-fbdev
In-Reply-To: <1346412220.16067.8.camel@deskari>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [120831 04:24]:
> On Thu, 2012-08-30 at 10:19 -0700, Tony Lindgren wrote:
> > Hi,
> >
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [120830 00:35]:
> > > On Wed, 2012-08-29 at 17:20 -0700, Tony Lindgren wrote:
> > > >
> > > > Good to see this, we need this badly to avoid blocking
> > > > single zImage effort on omaps. Can you also please take
> > >
> > > What is the issue with single zImage? How do cpu_is_ check affect it?
> >
> > The usage for that should only be limited to arch/arm/mach-omap2
> > so we can make cpu.h local as we can't include mach and plat
> > header files from the drivers with single zImage.
>
> Ok.
>
> > > $ git grep -E "<plat|<mach" drivers/video/omap*
> > > drivers/video/omap/lcd_ams_delta.c:#include <plat/board-ams-delta.h>
> > > * Needs to be moved
> >
> > Yes, that should be either mach/board-ams-delta.h, or separate driver
> > specific headers in include/linux/platform_data. For omap1 we are not
> > planning common zImage support, so let's just make sure we're not
> > breaking anything there as people are still using it.
>
> Hmm, so did I understand right, for omap1 stuff we can still include
> from arch/arm/mach-omap1/include/mach?
Yes that's a separate issue to fix that up for armv4/5 common
zImage support if people want to do that later on.
> If so, that makes things easier. I can manage the omap2+ stuff fine, but
> I have no experience with omap1, nor do I have any omap1 devices. So I'd
> rather keep the omap1 code as it is, in fear that I'd just break it
> totally, and I'd rather spend my time on omap2+ code.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH 7/8] OMAP: 4430SDP: remove DSI clock config from board file
From: Tony Lindgren @ 2012-09-06 20:11 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: archit, linux-omap, linux-fbdev
In-Reply-To: <1345729514-2441-8-git-send-email-tomi.valkeinen@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [120823 06:46]:
> DSI clocks are now configured dynamically by the DSI driver, so we can
> remove the hardcoded clock configuration from the board file.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
This should be safe to merge via fb tree:
Acked-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply
* Re: [PATCH 3/8] OMAP4: TWL: add vdda_hdmi_dac regulator supply
From: Tony Lindgren @ 2012-09-06 20:12 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: archit, linux-omap, linux-fbdev
In-Reply-To: <1345729514-2441-4-git-send-email-tomi.valkeinen@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [120823 06:46]:
> HDMI requires vdda_hdmi_dac (vdac) power for operation. The regulator,
> or the regulator supplying the vdac, has been enabled by default and
> things have worked without the HDMI driver enabling the vdac.
>
> I encountered the problem when implementing HDMI device tree support,
> where the regulator was not enabled by default.
>
> This patch adds the vdda_hdmi_dac to twl-common.c so that the HDMI
> driver can use it.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
Looks safe to merge vi fb changes:
Acked-by: Tony Lindgren <tony@atomide.com>
> ---
> arch/arm/mach-omap2/twl-common.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/twl-common.c b/arch/arm/mach-omap2/twl-common.c
> index 119d5a9..bf90356 100644
> --- a/arch/arm/mach-omap2/twl-common.c
> +++ b/arch/arm/mach-omap2/twl-common.c
> @@ -257,6 +257,10 @@ static struct twl4030_usb_data omap4_usb_pdata = {
> .phy_suspend = omap4430_phy_suspend,
> };
>
> +static struct regulator_consumer_supply omap4_vdda_hdmi_dac_supplies[] = {
> + REGULATOR_SUPPLY("vdda_hdmi_dac", "omapdss_hdmi"),
> +};
> +
> static struct regulator_init_data omap4_vdac_idata = {
> .constraints = {
> .min_uV = 1800000,
> @@ -266,6 +270,8 @@ static struct regulator_init_data omap4_vdac_idata = {
> .valid_ops_mask = REGULATOR_CHANGE_MODE
> | REGULATOR_CHANGE_STATUS,
> },
> + .num_consumer_supplies = ARRAY_SIZE(omap4_vdda_hdmi_dac_supplies),
> + .consumer_supplies = omap4_vdda_hdmi_dac_supplies,
> .supply_regulator = "V2V1",
> };
>
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH 0/8] OMAPDSS: Misc improvements
From: Tony Lindgren @ 2012-09-06 20:13 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, archit
In-Reply-To: <1346743368.2586.1.camel@deskari>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [120904 00:23]:
> Hi Tony,
>
> Can you check the arch/arm patches below, and suggest how you'd like to
> go forward with them?
Acked them, then as soon as we have the initial immutable header
move branch available, you should merge with that to avoid
merge conflicts in upstream.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-09-07 8:04 UTC (permalink / raw)
To: Heiko Stübner
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Brown,
Stephen Warren, linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
Anton Vorontsov,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
David Woodhouse,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <201209061614.54022.heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Hi Heiko,
On Thursday 06 September 2012 22:14:53 Heiko Stübner wrote:
> Hi Alexander,
>
> Am Freitag, 31. August 2012, 13:34:03 schrieb Alexandre Courbot:
> > Some device drivers (panel backlights especially) need to follow precise
> > sequences for powering on and off, involving gpios, regulators, PWMs
> > with a precise powering order and delays to respect between each steps.
> > These sequences are board-specific, and do not belong to a particular
> > driver - therefore they have been performed by board-specific hook
> > functions to far.
> >
> > With the advent of the device tree 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>
>
> I really like this idea, also because I'll have to solve similar problems on
> the way to dt for my tinker-platform.
Glad to read that! :)
> For your power_seq_run function you write that it simply returns an error
> code on failure and looking through it I also just found the error return
> statement. This would leave a device half turned on.
>
> So I'm wondering, if it shouldn't turn off all the things it turned on until
> the step that produced the error. All your possible step types (execpt the
> delay) are booleans, so it should be possible to simply negate them when
> backtracking through the previous steps.
Indeed, I think you raised an important point. Right now all step types are
invertible, but we cannot rely on that statement to be true forever. For
instance, one short-term improvement will be to allow finer regulator control,
like voltage setting. In this case, how can we go back to the initial state
without recording it?
If e.g. the power on sequence fails at step N (of M steps for that sequence),
one could try playing the corresponding power off sequence (either completely
of from step M - N), but then again we cannot rely on sequences to be
perfectly symetrical. Maybe this is more something for the calling driver to
check for and control?
Alex.
^ permalink raw reply
* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
From: Mark Brown @ 2012-09-07 8:15 UTC (permalink / raw)
To: Alex Courbot
Cc: Heiko Stübner, Stephen Warren, Thierry Reding, Simon Glass,
Grant Likely, Rob Herring, 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-pm@vger.kernel.org,
linux-doc@vger.kernel.org
In-Reply-To: <1887927.1deN8M9siP@percival>
On Fri, Sep 07, 2012 at 05:04:24PM +0900, Alex Courbot wrote:
> If e.g. the power on sequence fails at step N (of M steps for that sequence),
> one could try playing the corresponding power off sequence (either completely
> of from step M - N), but then again we cannot rely on sequences to be
> perfectly symetrical. Maybe this is more something for the calling driver to
> check for and control?
That had been my thought too - depending on what the sequence is for it
may be that the corrective action is something very different to
reversing the sequence, for example a device reset may be required.
^ permalink raw reply
* Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-09-07 8:21 UTC (permalink / raw)
To: Stephen Warren
Cc: Stephen Warren, Thierry Reding, 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-pm@vger.kernel.org,
linux-doc@vger.kernel.org
In-Reply-To: <504789B1.50205@wwwdotorg.org>
Hi Stephen,
Skipping the typos and rephrasing issues (which will all be addressed,
thanks!), these issues caught my attention more particularly:
On Thursday 06 September 2012 01:19:45 Stephen Warren wrote:
> > +"regulator" type required properties:
> > + - id: name of the regulator to use. Regulator is obtained by
> > + regulator_get(dev, id)
> > + - enable / disable: one of these two empty properties must be present
> > to
> > + enable or disable the resource
> > +
> > +"pwm" type required properties:
> > + - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
> > + - enable / disable: one of these two empty properties must be present
> > to
> > + enable or disable the resource
>
> For those two, would "name" be a better property name than "id"?
IIRC "name" is a reserved property name in the DT (I remember seeing an error
message when compiling nodes with a "name" property that did not match the
node's name). "id" was the second best candidate in my mind.
> > +"gpio" type required properties:
> > + - number: phandle of the GPIO to use.
>
> Naming the property "gpio" would seem more consistent with our GPIO
> properties.
Ok, although "gpio.gpio" might look strange in the platform data.
> > + - enable / disable: one of these two empty properties must be present
> > to
> > + enable or disable the resource
>
> You can't really enable or disable a GPIO (well, perhaps you can, but
> I'd consider that to affect tri-state rather than value); it's more that
> you're setting the output value to 0 or 1. I think a "value" or
> "set-value" property with value <0> or <1> would be better.
Right - will fix that.
> Overall, the general structure of the bindings looks reasonable to me.
Great, maybe we are finally headed to something stable!
> I'm not sure what "pdata" is supposed to point at; platform data applies
> to the original "struct device", not one of the resources used by the
> power sequences.
Right - more on this later.
> Hmmm. It'd be nice not to need separate functions for the non-DT and DT
> cases. That would require that devm_power_seq_set_build() be able to
> find the power sequence definitions somewhere other than platform data
> in the non-DT case - that's exactly why the regulator and pinctrl
> subsystems represent the device<->data mapping table separately from the
> device's platform data.
Oh, that sounds better indeed - I was still mentally stuck in the previous
scheme where power sequences were not accessible from a fixed node of the
device. Thanks.
> > +++ b/drivers/power/power_seq/Kconfig
> >
> > +config POWER_SEQ
> > + bool
>
> Some kind of help text might be useful?
As this option was not user-visible but automatically enabled by drivers, I
did not judge it to be necessary - but after reading your other mail we might
need to make it visible and documented after all.
> > +/**
> > + * struct platform_power_seq_step - platform data for power sequences
> > steps + * @type: The type of this step. This decides which member of
the
> > union is + * valid for this step.
> > + * @delay: Used if type = POWER_SEQ_DELAY
> > + * @regulator: Used if type = POWER_SEQ_REGULATOR
> > + * @pwm: Used if type = POWER_SEQ_PWN
> > + * @gpio: Used if type = POWER_SEQ_GPIO
>
> In those last 4 line, I think s/type/@type/ since you're referencing
> another parameter?
Absolutely.
> > +struct power_seq_resource {
> > + /* relevant for resolving the resource and knowing its type */
> > + struct platform_power_seq_step *pdata;
>
> Aha. So this isn't really platform data for the resource, but actually a
> step definition that referenced it. I think it'd be better to rename
> this field "step", and amend the documentation above not to refer to
> "pdata" but explicitly talk about a step definition; the step may have
> been defined in pdata, but isn't in the DT case.
This is a little bit confusing, isn't it? The resource identifier is duplicated
in the platform data by every step that uses it. To avoid copying that
information again into the resource structure, I just use this pointer to the
first step that uses this resource. So a step not only contains step
information, but also part of it might be used by a resource instance. Ugh,
that's horribly confusing, actually.
> Alternatively, why not just copy the step type enum here, rather than
> referencing the step definition?
On top of the type we will also need the identifier of the resource (string for
pwm and regulator, number for GPIO) so we can compare the resource against
platform data when building the sequence to avoid allocating it twice. That's
a little bit of extra memory, but the gain in clarity is probably worth it.
Alex.
^ 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