* Re: [PATCH] video: exynos_dp: remove duplicated declarations from header file
From: Florian Tobias Schandinat @ 2012-06-24 21:00 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <000601cd4f94$415cd750$c41685f0$%han@samsung.com>
On 06/21/2012 09:57 AM, Jingoo Han wrote:
> Some functions are declared twice in header file; thus, these
> declarations are unnecessary.
>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/exynos/exynos_dp_core.h | 4 ----
> 1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 1e0f998..8526e54 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -85,10 +85,6 @@ void exynos_dp_set_link_bandwidth(struct exynos_dp_device *dp, u32 bwtype);
> void exynos_dp_get_link_bandwidth(struct exynos_dp_device *dp, u32 *bwtype);
> void exynos_dp_set_lane_count(struct exynos_dp_device *dp, u32 count);
> void exynos_dp_get_lane_count(struct exynos_dp_device *dp, u32 *count);
> -void exynos_dp_set_link_bandwidth(struct exynos_dp_device *dp, u32 bwtype);
> -void exynos_dp_get_link_bandwidth(struct exynos_dp_device *dp, u32 *bwtype);
> -void exynos_dp_set_lane_count(struct exynos_dp_device *dp, u32 count);
> -void exynos_dp_get_lane_count(struct exynos_dp_device *dp, u32 *count);
> void exynos_dp_enable_enhanced_mode(struct exynos_dp_device *dp, bool enable);
> void exynos_dp_set_training_pattern(struct exynos_dp_device *dp,
> enum pattern_set pattern);
^ permalink raw reply
* Re: [PATCH v2] video: exynos_dp: fix build warning due to uninitialized value
From: Florian Tobias Schandinat @ 2012-06-24 21:00 UTC (permalink / raw)
To: Jingoo Han; +Cc: linux-fbdev, linux-kernel, 'Olof Johansson'
In-Reply-To: <000301cd4e83$9f5fcb30$de1f6190$%han@samsung.com>
On 06/20/2012 01:25 AM, Jingoo Han wrote:
> This patch fixes build warning due to uninitialized value dereference.
>
> drivers/video/exynos/exynos_dp_core.c: In function 'exynos_dp_set_link_train':
> drivers/video/exynos/exynos_dp_core.c:529:18: warning: 'reg' may be used uninitialized in this function [-Wuninitialized]
> drivers/video/exynos/exynos_dp_core.c:395:6: note: 'reg' was declared here
>
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/exynos/exynos_dp_core.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index a36b2d2..9db7b9f 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -407,6 +407,9 @@ static unsigned int exynos_dp_get_lane_link_training(
> case 3:
> reg = exynos_dp_get_lane3_link_training(dp);
> break;
> + default:
> + WARN_ON(1);
> + return 0;
> }
>
> return reg;
^ permalink raw reply
* Re: [PATCH] video: backlight: remove unused header
From: Florian Tobias Schandinat @ 2012-06-24 20:59 UTC (permalink / raw)
To: Paul Bolle; +Cc: linux-fbdev, linux-kernel
In-Reply-To: <1339573651.30984.130.camel@x61.thuisdomein>
On 06/13/2012 07:47 AM, Paul Bolle wrote:
> Commit 9befe40f6e018e508b047eb76d189ede9b4ff03d ("video: backlight:
> support s6e8ax0 panel driver based on MIPI DSI") added s6e8ax0.h, but
> no file includes it. That's probably a good thing, because it declares
> an extern void function that is defined static int in s6e8ax0.c.
> Besides, that function is also wrapped in the module_init() macro, which
> should do everything needed to make that function available to the code
> outside of s6e8ax0.c. This header can safely be removed.
>
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> 0) Tested mainly by using various git tools on the (history of the)
> tree.
>
> 1) Shouldn't s6e8ax0_init() and s6e8ax0_exit(), both in s6e8ax0.c, carry
> the usual __init and __exit attributes?
>
> 2) But note that all the module related code in s6e8ax0.c seems moot
> currently: EXYNOS_LCD_S6E8AX0 is a boolean Kconfig symbol, so the code
> can only be used builtin. So, as far as I can tell, either that symbol
> (and the symbols on which it depends) should be made tristate, or the
> module related code can be removed from s6e8ax0.c.
>
> drivers/video/exynos/s6e8ax0.h | 21 ---------------------
> 1 files changed, 0 insertions(+), 21 deletions(-)
> delete mode 100644 drivers/video/exynos/s6e8ax0.h
>
> diff --git a/drivers/video/exynos/s6e8ax0.h b/drivers/video/exynos/s6e8ax0.h
> deleted file mode 100644
> index 1f1b270..0000000
> --- a/drivers/video/exynos/s6e8ax0.h
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/* linux/drivers/video/backlight/s6e8ax0.h
> - *
> - * MIPI-DSI based s6e8ax0 AMOLED LCD Panel definitions.
> - *
> - * Copyright (c) 2011 Samsung Electronics
> - *
> - * Inki Dae, <inki.dae@samsung.com>
> - * Donghwa Lee <dh09.lee@samsung.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> -*/
> -
> -#ifndef _S6E8AX0_H
> -#define _S6E8AX0_H
> -
> -extern void s6e8ax0_init(void);
> -
> -#endif
> -
^ permalink raw reply
* Re: [PATCH] grvga: Fix error handling issues
From: Emil Goode @ 2012-06-24 10:36 UTC (permalink / raw)
To: Julia Lawall
Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors,
w.sang
In-Reply-To: <alpine.DEB.2.02.1206240711580.2178@localhost6.localdomain6>
Yes I considered converting to devm_ and keeping the of_iounmap in.
But I just feel like I'm mixing two api's. If converting to devm_ really
is the right thing to do, tell me and I will send another patch :)
Thanks,
Emil
On Sun, 2012-06-24 at 07:15 +0200, Julia Lawall wrote:
> I see. You could still use a devm_ function for request_mem_region.
> Also I noticed that the remove function uses iounmap directly, not
> io_iounmap. The latter is just a wrapper for the former, but it could be
> good to use the right name.
>
> julia
>
> On Sun, 24 Jun 2012, Emil Goode wrote:
>
> > The of_ioremap function is used in this code as well and I don't know of
> > a devm_ equivalent for it. For consistency I think it is better to leave
> > it as it is in this case. So I stick with v1 of this patch.
> >
> > Thanks,
> >
> > Emil
> >
> > On Fri, 2012-06-22 at 18:21 +0200, Julia Lawall wrote:
> >> On Fri, 22 Jun 2012, Emil Goode wrote:
> >>
> >>> Good idea, I will take another look at it tomorrow.
> >>
> >> There is a devm_ function that combines request_mem_region and ioremap
> >> that could be useful.
> >>
> >> julia
> >>
> >>>
> >>> Thanks,
> >>>
> >>> Emil
> >>>
> >>> On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
> >>>> Maybe you could used the devm_ functions for request_mem_region and
> >>>> ioremap so that the error handling can just be dropped?
> >>>>
> >>>> julia
> >>>>
> >>>> On Fri, 22 Jun 2012, Emil Goode wrote:
> >>>>
> >>>>> This patch fixes two problems with the error handling in the
> >>>>> grvga_probe function.
> >>>>>
> >>>>> - If the call to grvga_parse_custom on line 370 fails we use
> >>>>> the wrong label so that release_mem_region will be called
> >>>>> without a call to request_mem_region being made.
> >>>>>
> >>>>> - If the call to ioremap on line 436 fails we should not try
> >>>>> to call iounmap. I added an if statement to check whether or
> >>>>> not a call to iounmap should be made.
> >>>>>
> >>>>> - I also changed the names of the labels to make the code
> >>>>> easier to read.
> >>>>>
> >>>>> Signed-off-by: Emil Goode <emilgoode@gmail.com>
> >>>>> ---
> >>>>> drivers/video/grvga.c | 34 ++++++++++++++++++----------------
> >>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
> >>>>> index da066c2..d9d688a 100644
> >>>>> --- a/drivers/video/grvga.c
> >>>>> +++ b/drivers/video/grvga.c
> >>>>> @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> */
> >>>>> if (fb_get_options("grvga", &options)) {
> >>>>> retval = -ENODEV;
> >>>>> - goto err;
> >>>>> + goto free_fb;
> >>>>> }
> >>>>>
> >>>>> if (!options || !*options)
> >>>>> @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> if (grvga_parse_custom(this_opt, &info->var) < 0) {
> >>>>> dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
> >>>>> retval = -EINVAL;
> >>>>> - goto err1;
> >>>>> + goto free_fb;
> >>>>> }
> >>>>> } else if (!strncmp(this_opt, "addr", 4))
> >>>>> grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
> >>>>> @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
> >>>>> dev_err(&dev->dev, "registers already mapped\n");
> >>>>> retval = -EBUSY;
> >>>>> - goto err;
> >>>>> + goto free_fb;
> >>>>> }
> >>>>>
> >>>>> par->regs = of_ioremap(&dev->resource[0], 0,
> >>>>> @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> if (!par->regs) {
> >>>>> dev_err(&dev->dev, "failed to map registers\n");
> >>>>> retval = -ENOMEM;
> >>>>> - goto err1;
> >>>>> + goto release_regs;
> >>>>> }
> >>>>>
> >>>>> retval = fb_alloc_cmap(&info->cmap, 256, 0);
> >>>>> if (retval < 0) {
> >>>>> dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
> >>>>> retval = -ENOMEM;
> >>>>> - goto err2;
> >>>>> + goto unmap_regs;
> >>>>> }
> >>>>>
> >>>>> if (mode_opt) {
> >>>>> @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
> >>>>> if (!retval || retval = 4) {
> >>>>> retval = -EINVAL;
> >>>>> - goto err3;
> >>>>> + goto dealloc_cmap;
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
> >>>>> dev_err(&dev->dev, "failed to request memory region\n");
> >>>>> retval = -ENOMEM;
> >>>>> - goto err3;
> >>>>> + goto dealloc_cmap;
> >>>>> }
> >>>>>
> >>>>> virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
> >>>>> @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> if (!virtual_start) {
> >>>>> dev_err(&dev->dev, "error mapping framebuffer memory\n");
> >>>>> retval = -ENOMEM;
> >>>>> - goto err4;
> >>>>> + goto free_mem;
> >>>>> }
> >>>>> } else { /* Allocate frambuffer memory */
> >>>>>
> >>>>> @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> "unable to allocate framebuffer memory (%lu bytes)\n",
> >>>>> grvga_mem_size);
> >>>>> retval = -ENOMEM;
> >>>>> - goto err3;
> >>>>> + goto dealloc_cmap;
> >>>>> }
> >>>>>
> >>>>> physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
> >>>>> @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>> retval = register_framebuffer(info);
> >>>>> if (retval < 0) {
> >>>>> dev_err(&dev->dev, "failed to register framebuffer\n");
> >>>>> - goto err4;
> >>>>> + goto free_mem;
> >>>>> }
> >>>>>
> >>>>> __raw_writel(physical_start, &par->regs->fb_pos);
> >>>>> @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >>>>>
> >>>>> return 0;
> >>>>>
> >>>>> -err4:
> >>>>> +free_mem:
> >>>>> dev_set_drvdata(&dev->dev, NULL);
> >>>>> if (grvga_fix_addr) {
> >>>>> release_mem_region(physical_start, grvga_mem_size);
> >>>>> - iounmap((void *)virtual_start);
> >>>>> + if (virtual_start)
> >>>>> + iounmap((void *)virtual_start);
> >>>>> } else
> >>>>> kfree((void *)virtual_start);
> >>>>> -err3:
> >>>>> +
> >>>>> +dealloc_cmap:
> >>>>> fb_dealloc_cmap(&info->cmap);
> >>>>> -err2:
> >>>>> +unmap_regs:
> >>>>> of_iounmap(&dev->resource[0], par->regs,
> >>>>> resource_size(&dev->resource[0]));
> >>>>> -err1:
> >>>>> +release_regs:
> >>>>> release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
> >>>>> -err:
> >>>>> +free_fb:
> >>>>> framebuffer_release(info);
> >>>>>
> >>>>> return retval;
> >>>>> --
> >>>>> 1.7.10
> >>>>>
> >>>>> --
> >>>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> >>>>> the body of a message to majordomo@vger.kernel.org
> >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>>>>
> >>>
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>>
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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] grvga: Fix error handling issues
From: Emil Goode @ 2012-06-24 9:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors
In-Reply-To: <20120623223319.GG5333@mwanda>
This code is for the sparc architecture and the iounmap doesn't check
for NULL directly as in the x86 version. I think it is safer not to pass
NULL to the sparc version.
Thanks,
Emil
On Sun, 2012-06-24 at 01:33 +0300, Dan Carpenter wrote:
> On Fri, Jun 22, 2012 at 06:11:19PM +0200, Emil Goode wrote:
> > This patch fixes two problems with the error handling in the
> > grvga_probe function.
> >
> > - If the call to grvga_parse_custom on line 370 fails we use
> > the wrong label so that release_mem_region will be called
> > without a call to request_mem_region being made.
> >
> > - If the call to ioremap on line 436 fails we should not try
> > to call iounmap. I added an if statement to check whether or
> > not a call to iounmap should be made.
> >
>
> Doesn't iounmap() have a check for NULL? On x86 it does.
>
> if ((void __force *)addr <= high_memory)
> return;
>
> regards,
> dan carpenter
>
>
^ permalink raw reply
* boot hang on CONFIG_FB_VGA16
From: Fengguang Wu @ 2012-06-24 5:21 UTC (permalink / raw)
To: Antonino A. Daplas; +Cc: Florian Tobias Schandinat, linux-fbdev, linux-kernel
Hi,
I can reproduce this easily when running CONFIG_FB_VGA16=y kernels
inside KVM. Older kernels like 3.2, 3.3, 3.4 don't have these warning
messages, but just randomly hang during boot.
I'm not sure if this is a relevant problem, but anyway, here is the
bug report.
INFO: rcu_sched self-detected stall on CPU { 1} (t\x15003 jiffies)
Pid: 8, comm: kworker/1:0 Not tainted 3.5.0-rc3-wl+ #11
Call Trace:
<IRQ> [<ffffffff810a1e35>] __rcu_pending+0xc2/0x3c3
[<ffffffff8107a1bc>] ? tick_nohz_handler+0xd5/0xd5
[<ffffffff810a3052>] rcu_check_callbacks+0x9d/0x149
[<ffffffff81061b01>] ? sched_clock_cpu+0xbb/0xcc
[<ffffffff8103db27>] update_process_times+0x3e/0x72
[<ffffffff8107a228>] tick_sched_timer+0x6c/0x95
[<ffffffff81061b21>] ? local_clock+0xf/0x3c
[<ffffffff810537d3>] __run_hrtimer+0xcf/0x1d1
[<ffffffff810541c6>] hrtimer_interrupt+0xc6/0x191
[<ffffffff817bc52a>] smp_apic_timer_interrupt+0x86/0x99
[<ffffffff817bb51c>] apic_timer_interrupt+0x6c/0x80
<EOI> [<ffffffff81030a8c>] ? console_unlock+0x21b/0x2c5
[<ffffffff813304a1>] ? update_attr.isra.1+0x8a/0x8a
[<ffffffff8132b7a7>] fb_flashcursor+0x114/0x123
[<ffffffff81048529>] process_one_work+0x238/0x45c
[<ffffffff8104848c>] ? process_one_work+0x19b/0x45c
[<ffffffff8132b693>] ? get_color+0xfd/0xfd
[<ffffffff8104876e>] process_scheduled_works+0x21/0x2f
[<ffffffff810488f5>] worker_thread+0x179/0x240
Thanks,
Fengguang
^ permalink raw reply
* Re: [PATCH] grvga: Fix error handling issues
From: Julia Lawall @ 2012-06-24 5:18 UTC (permalink / raw)
To: Julia Lawall
Cc: Emil Goode, FlorianSchandinat, linux-fbdev, linux-kernel,
kernel-janitors, w.sang
In-Reply-To: <alpine.DEB.2.02.1206240711580.2178@localhost6.localdomain6>
On Sun, 24 Jun 2012, Julia Lawall wrote:
> I see. You could still use a devm_ function for request_mem_region.
> Also I noticed that the remove function uses iounmap directly, not
> io_iounmap. The latter is just a wrapper for the former, but it could be
> good to use the right name.
Oops, drop the last comment; the function uses both.
julia
>
> julia
>
> On Sun, 24 Jun 2012, Emil Goode wrote:
>
>> The of_ioremap function is used in this code as well and I don't know of
>> a devm_ equivalent for it. For consistency I think it is better to leave
>> it as it is in this case. So I stick with v1 of this patch.
>>
>> Thanks,
>>
>> Emil
>>
>> On Fri, 2012-06-22 at 18:21 +0200, Julia Lawall wrote:
>>> On Fri, 22 Jun 2012, Emil Goode wrote:
>>>
>>>> Good idea, I will take another look at it tomorrow.
>>>
>>> There is a devm_ function that combines request_mem_region and ioremap
>>> that could be useful.
>>>
>>> julia
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Emil
>>>>
>>>> On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
>>>>> Maybe you could used the devm_ functions for request_mem_region and
>>>>> ioremap so that the error handling can just be dropped?
>>>>>
>>>>> julia
>>>>>
>>>>> On Fri, 22 Jun 2012, Emil Goode wrote:
>>>>>
>>>>>> This patch fixes two problems with the error handling in the
>>>>>> grvga_probe function.
>>>>>>
>>>>>> - If the call to grvga_parse_custom on line 370 fails we use
>>>>>> the wrong label so that release_mem_region will be called
>>>>>> without a call to request_mem_region being made.
>>>>>>
>>>>>> - If the call to ioremap on line 436 fails we should not try
>>>>>> to call iounmap. I added an if statement to check whether or
>>>>>> not a call to iounmap should be made.
>>>>>>
>>>>>> - I also changed the names of the labels to make the code
>>>>>> easier to read.
>>>>>>
>>>>>> Signed-off-by: Emil Goode <emilgoode@gmail.com>
>>>>>> ---
>>>>>> drivers/video/grvga.c | 34 ++++++++++++++++++----------------
>>>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
>>>>>> index da066c2..d9d688a 100644
>>>>>> --- a/drivers/video/grvga.c
>>>>>> +++ b/drivers/video/grvga.c
>>>>>> @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> */
>>>>>> if (fb_get_options("grvga", &options)) {
>>>>>> retval = -ENODEV;
>>>>>> - goto err;
>>>>>> + goto free_fb;
>>>>>> }
>>>>>>
>>>>>> if (!options || !*options)
>>>>>> @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> if (grvga_parse_custom(this_opt, &info->var) < 0) {
>>>>>> dev_err(&dev->dev, "Failed to parse custom
>>>>>> mode (%s).\n", this_opt);
>>>>>> retval = -EINVAL;
>>>>>> - goto err1;
>>>>>> + goto free_fb;
>>>>>> }
>>>>>> } else if (!strncmp(this_opt, "addr", 4))
>>>>>> grvga_fix_addr = simple_strtoul(this_opt + 5, NULL,
>>>>>> 16);
>>>>>> @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> if (!request_mem_region(dev->resource[0].start,
>>>>>> resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
>>>>>> dev_err(&dev->dev, "registers already mapped\n");
>>>>>> retval = -EBUSY;
>>>>>> - goto err;
>>>>>> + goto free_fb;
>>>>>> }
>>>>>>
>>>>>> par->regs = of_ioremap(&dev->resource[0], 0,
>>>>>> @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> if (!par->regs) {
>>>>>> dev_err(&dev->dev, "failed to map registers\n");
>>>>>> retval = -ENOMEM;
>>>>>> - goto err1;
>>>>>> + goto release_regs;
>>>>>> }
>>>>>>
>>>>>> retval = fb_alloc_cmap(&info->cmap, 256, 0);
>>>>>> if (retval < 0) {
>>>>>> dev_err(&dev->dev, "failed to allocate mem with
>>>>>> fb_alloc_cmap\n");
>>>>>> retval = -ENOMEM;
>>>>>> - goto err2;
>>>>>> + goto unmap_regs;
>>>>>> }
>>>>>>
>>>>>> if (mode_opt) {
>>>>>> @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> grvga_modedb, sizeof(grvga_modedb),
>>>>>> &grvga_modedb[0], 8);
>>>>>> if (!retval || retval = 4) {
>>>>>> retval = -EINVAL;
>>>>>> - goto err3;
>>>>>> + goto dealloc_cmap;
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> if (!request_mem_region(physical_start, grvga_mem_size,
>>>>>> dev->name)) {
>>>>>> dev_err(&dev->dev, "failed to request memory
>>>>>> region\n");
>>>>>> retval = -ENOMEM;
>>>>>> - goto err3;
>>>>>> + goto dealloc_cmap;
>>>>>> }
>>>>>>
>>>>>> virtual_start = (unsigned long) ioremap(physical_start,
>>>>>> grvga_mem_size);
>>>>>> @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> if (!virtual_start) {
>>>>>> dev_err(&dev->dev, "error mapping framebuffer
>>>>>> memory\n");
>>>>>> retval = -ENOMEM;
>>>>>> - goto err4;
>>>>>> + goto free_mem;
>>>>>> }
>>>>>> } else { /* Allocate frambuffer memory */
>>>>>>
>>>>>> @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> "unable to allocate framebuffer memory (%lu
>>>>>> bytes)\n",
>>>>>> grvga_mem_size);
>>>>>> retval = -ENOMEM;
>>>>>> - goto err3;
>>>>>> + goto dealloc_cmap;
>>>>>> }
>>>>>>
>>>>>> physical_start = dma_map_single(&dev->dev, (void
>>>>>> *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
>>>>>> @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>> retval = register_framebuffer(info);
>>>>>> if (retval < 0) {
>>>>>> dev_err(&dev->dev, "failed to register framebuffer\n");
>>>>>> - goto err4;
>>>>>> + goto free_mem;
>>>>>> }
>>>>>>
>>>>>> __raw_writel(physical_start, &par->regs->fb_pos);
>>>>>> @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct
>>>>>> platform_device *dev)
>>>>>>
>>>>>> return 0;
>>>>>>
>>>>>> -err4:
>>>>>> +free_mem:
>>>>>> dev_set_drvdata(&dev->dev, NULL);
>>>>>> if (grvga_fix_addr) {
>>>>>> release_mem_region(physical_start, grvga_mem_size);
>>>>>> - iounmap((void *)virtual_start);
>>>>>> + if (virtual_start)
>>>>>> + iounmap((void *)virtual_start);
>>>>>> } else
>>>>>> kfree((void *)virtual_start);
>>>>>> -err3:
>>>>>> +
>>>>>> +dealloc_cmap:
>>>>>> fb_dealloc_cmap(&info->cmap);
>>>>>> -err2:
>>>>>> +unmap_regs:
>>>>>> of_iounmap(&dev->resource[0], par->regs,
>>>>>> resource_size(&dev->resource[0]));
>>>>>> -err1:
>>>>>> +release_regs:
>>>>>> release_mem_region(dev->resource[0].start,
>>>>>> resource_size(&dev->resource[0]));
>>>>>> -err:
>>>>>> +free_fb:
>>>>>> framebuffer_release(info);
>>>>>>
>>>>>> return retval;
>>>>>> --
>>>>>> 1.7.10
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>>> kernel-janitors" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> kernel-janitors" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors"
>> 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] grvga: Fix error handling issues
From: Julia Lawall @ 2012-06-24 5:15 UTC (permalink / raw)
To: Emil Goode
Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors,
w.sang
In-Reply-To: <1340488911.5321.10.camel@localhost>
I see. You could still use a devm_ function for request_mem_region.
Also I noticed that the remove function uses iounmap directly, not
io_iounmap. The latter is just a wrapper for the former, but it could be
good to use the right name.
julia
On Sun, 24 Jun 2012, Emil Goode wrote:
> The of_ioremap function is used in this code as well and I don't know of
> a devm_ equivalent for it. For consistency I think it is better to leave
> it as it is in this case. So I stick with v1 of this patch.
>
> Thanks,
>
> Emil
>
> On Fri, 2012-06-22 at 18:21 +0200, Julia Lawall wrote:
>> On Fri, 22 Jun 2012, Emil Goode wrote:
>>
>>> Good idea, I will take another look at it tomorrow.
>>
>> There is a devm_ function that combines request_mem_region and ioremap
>> that could be useful.
>>
>> julia
>>
>>>
>>> Thanks,
>>>
>>> Emil
>>>
>>> On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
>>>> Maybe you could used the devm_ functions for request_mem_region and
>>>> ioremap so that the error handling can just be dropped?
>>>>
>>>> julia
>>>>
>>>> On Fri, 22 Jun 2012, Emil Goode wrote:
>>>>
>>>>> This patch fixes two problems with the error handling in the
>>>>> grvga_probe function.
>>>>>
>>>>> - If the call to grvga_parse_custom on line 370 fails we use
>>>>> the wrong label so that release_mem_region will be called
>>>>> without a call to request_mem_region being made.
>>>>>
>>>>> - If the call to ioremap on line 436 fails we should not try
>>>>> to call iounmap. I added an if statement to check whether or
>>>>> not a call to iounmap should be made.
>>>>>
>>>>> - I also changed the names of the labels to make the code
>>>>> easier to read.
>>>>>
>>>>> Signed-off-by: Emil Goode <emilgoode@gmail.com>
>>>>> ---
>>>>> drivers/video/grvga.c | 34 ++++++++++++++++++----------------
>>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
>>>>> index da066c2..d9d688a 100644
>>>>> --- a/drivers/video/grvga.c
>>>>> +++ b/drivers/video/grvga.c
>>>>> @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> */
>>>>> if (fb_get_options("grvga", &options)) {
>>>>> retval = -ENODEV;
>>>>> - goto err;
>>>>> + goto free_fb;
>>>>> }
>>>>>
>>>>> if (!options || !*options)
>>>>> @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> if (grvga_parse_custom(this_opt, &info->var) < 0) {
>>>>> dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
>>>>> retval = -EINVAL;
>>>>> - goto err1;
>>>>> + goto free_fb;
>>>>> }
>>>>> } else if (!strncmp(this_opt, "addr", 4))
>>>>> grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
>>>>> @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
>>>>> dev_err(&dev->dev, "registers already mapped\n");
>>>>> retval = -EBUSY;
>>>>> - goto err;
>>>>> + goto free_fb;
>>>>> }
>>>>>
>>>>> par->regs = of_ioremap(&dev->resource[0], 0,
>>>>> @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> if (!par->regs) {
>>>>> dev_err(&dev->dev, "failed to map registers\n");
>>>>> retval = -ENOMEM;
>>>>> - goto err1;
>>>>> + goto release_regs;
>>>>> }
>>>>>
>>>>> retval = fb_alloc_cmap(&info->cmap, 256, 0);
>>>>> if (retval < 0) {
>>>>> dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
>>>>> retval = -ENOMEM;
>>>>> - goto err2;
>>>>> + goto unmap_regs;
>>>>> }
>>>>>
>>>>> if (mode_opt) {
>>>>> @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
>>>>> if (!retval || retval = 4) {
>>>>> retval = -EINVAL;
>>>>> - goto err3;
>>>>> + goto dealloc_cmap;
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
>>>>> dev_err(&dev->dev, "failed to request memory region\n");
>>>>> retval = -ENOMEM;
>>>>> - goto err3;
>>>>> + goto dealloc_cmap;
>>>>> }
>>>>>
>>>>> virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
>>>>> @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> if (!virtual_start) {
>>>>> dev_err(&dev->dev, "error mapping framebuffer memory\n");
>>>>> retval = -ENOMEM;
>>>>> - goto err4;
>>>>> + goto free_mem;
>>>>> }
>>>>> } else { /* Allocate frambuffer memory */
>>>>>
>>>>> @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> "unable to allocate framebuffer memory (%lu bytes)\n",
>>>>> grvga_mem_size);
>>>>> retval = -ENOMEM;
>>>>> - goto err3;
>>>>> + goto dealloc_cmap;
>>>>> }
>>>>>
>>>>> physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
>>>>> @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>> retval = register_framebuffer(info);
>>>>> if (retval < 0) {
>>>>> dev_err(&dev->dev, "failed to register framebuffer\n");
>>>>> - goto err4;
>>>>> + goto free_mem;
>>>>> }
>>>>>
>>>>> __raw_writel(physical_start, &par->regs->fb_pos);
>>>>> @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
>>>>>
>>>>> return 0;
>>>>>
>>>>> -err4:
>>>>> +free_mem:
>>>>> dev_set_drvdata(&dev->dev, NULL);
>>>>> if (grvga_fix_addr) {
>>>>> release_mem_region(physical_start, grvga_mem_size);
>>>>> - iounmap((void *)virtual_start);
>>>>> + if (virtual_start)
>>>>> + iounmap((void *)virtual_start);
>>>>> } else
>>>>> kfree((void *)virtual_start);
>>>>> -err3:
>>>>> +
>>>>> +dealloc_cmap:
>>>>> fb_dealloc_cmap(&info->cmap);
>>>>> -err2:
>>>>> +unmap_regs:
>>>>> of_iounmap(&dev->resource[0], par->regs,
>>>>> resource_size(&dev->resource[0]));
>>>>> -err1:
>>>>> +release_regs:
>>>>> release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
>>>>> -err:
>>>>> +free_fb:
>>>>> framebuffer_release(info);
>>>>>
>>>>> return retval;
>>>>> --
>>>>> 1.7.10
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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] grvga: Fix error handling issues
From: Dan Carpenter @ 2012-06-23 22:33 UTC (permalink / raw)
To: Emil Goode; +Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors
In-Reply-To: <1340381479-6371-1-git-send-email-emilgoode@gmail.com>
On Fri, Jun 22, 2012 at 06:11:19PM +0200, Emil Goode wrote:
> This patch fixes two problems with the error handling in the
> grvga_probe function.
>
> - If the call to grvga_parse_custom on line 370 fails we use
> the wrong label so that release_mem_region will be called
> without a call to request_mem_region being made.
>
> - If the call to ioremap on line 436 fails we should not try
> to call iounmap. I added an if statement to check whether or
> not a call to iounmap should be made.
>
Doesn't iounmap() have a check for NULL? On x86 it does.
if ((void __force *)addr <= high_memory)
return;
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] grvga: Fix error handling issues
From: Emil Goode @ 2012-06-23 22:01 UTC (permalink / raw)
To: Julia Lawall
Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors
In-Reply-To: <alpine.DEB.2.02.1206221820340.1901@hadrien>
The of_ioremap function is used in this code as well and I don't know of
a devm_ equivalent for it. For consistency I think it is better to leave
it as it is in this case. So I stick with v1 of this patch.
Thanks,
Emil
On Fri, 2012-06-22 at 18:21 +0200, Julia Lawall wrote:
> On Fri, 22 Jun 2012, Emil Goode wrote:
>
> > Good idea, I will take another look at it tomorrow.
>
> There is a devm_ function that combines request_mem_region and ioremap
> that could be useful.
>
> julia
>
> >
> > Thanks,
> >
> > Emil
> >
> > On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
> > > Maybe you could used the devm_ functions for request_mem_region and
> > > ioremap so that the error handling can just be dropped?
> > >
> > > julia
> > >
> > > On Fri, 22 Jun 2012, Emil Goode wrote:
> > >
> > > > This patch fixes two problems with the error handling in the
> > > > grvga_probe function.
> > > >
> > > > - If the call to grvga_parse_custom on line 370 fails we use
> > > > the wrong label so that release_mem_region will be called
> > > > without a call to request_mem_region being made.
> > > >
> > > > - If the call to ioremap on line 436 fails we should not try
> > > > to call iounmap. I added an if statement to check whether or
> > > > not a call to iounmap should be made.
> > > >
> > > > - I also changed the names of the labels to make the code
> > > > easier to read.
> > > >
> > > > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > > > ---
> > > > drivers/video/grvga.c | 34 ++++++++++++++++++----------------
> > > > 1 file changed, 18 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
> > > > index da066c2..d9d688a 100644
> > > > --- a/drivers/video/grvga.c
> > > > +++ b/drivers/video/grvga.c
> > > > @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > */
> > > > if (fb_get_options("grvga", &options)) {
> > > > retval = -ENODEV;
> > > > - goto err;
> > > > + goto free_fb;
> > > > }
> > > >
> > > > if (!options || !*options)
> > > > @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > if (grvga_parse_custom(this_opt, &info->var) < 0) {
> > > > dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
> > > > retval = -EINVAL;
> > > > - goto err1;
> > > > + goto free_fb;
> > > > }
> > > > } else if (!strncmp(this_opt, "addr", 4))
> > > > grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
> > > > @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
> > > > dev_err(&dev->dev, "registers already mapped\n");
> > > > retval = -EBUSY;
> > > > - goto err;
> > > > + goto free_fb;
> > > > }
> > > >
> > > > par->regs = of_ioremap(&dev->resource[0], 0,
> > > > @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > if (!par->regs) {
> > > > dev_err(&dev->dev, "failed to map registers\n");
> > > > retval = -ENOMEM;
> > > > - goto err1;
> > > > + goto release_regs;
> > > > }
> > > >
> > > > retval = fb_alloc_cmap(&info->cmap, 256, 0);
> > > > if (retval < 0) {
> > > > dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
> > > > retval = -ENOMEM;
> > > > - goto err2;
> > > > + goto unmap_regs;
> > > > }
> > > >
> > > > if (mode_opt) {
> > > > @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
> > > > if (!retval || retval = 4) {
> > > > retval = -EINVAL;
> > > > - goto err3;
> > > > + goto dealloc_cmap;
> > > > }
> > > > }
> > > >
> > > > @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
> > > > dev_err(&dev->dev, "failed to request memory region\n");
> > > > retval = -ENOMEM;
> > > > - goto err3;
> > > > + goto dealloc_cmap;
> > > > }
> > > >
> > > > virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
> > > > @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > if (!virtual_start) {
> > > > dev_err(&dev->dev, "error mapping framebuffer memory\n");
> > > > retval = -ENOMEM;
> > > > - goto err4;
> > > > + goto free_mem;
> > > > }
> > > > } else { /* Allocate frambuffer memory */
> > > >
> > > > @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > "unable to allocate framebuffer memory (%lu bytes)\n",
> > > > grvga_mem_size);
> > > > retval = -ENOMEM;
> > > > - goto err3;
> > > > + goto dealloc_cmap;
> > > > }
> > > >
> > > > physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
> > > > @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > > retval = register_framebuffer(info);
> > > > if (retval < 0) {
> > > > dev_err(&dev->dev, "failed to register framebuffer\n");
> > > > - goto err4;
> > > > + goto free_mem;
> > > > }
> > > >
> > > > __raw_writel(physical_start, &par->regs->fb_pos);
> > > > @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >
> > > > return 0;
> > > >
> > > > -err4:
> > > > +free_mem:
> > > > dev_set_drvdata(&dev->dev, NULL);
> > > > if (grvga_fix_addr) {
> > > > release_mem_region(physical_start, grvga_mem_size);
> > > > - iounmap((void *)virtual_start);
> > > > + if (virtual_start)
> > > > + iounmap((void *)virtual_start);
> > > > } else
> > > > kfree((void *)virtual_start);
> > > > -err3:
> > > > +
> > > > +dealloc_cmap:
> > > > fb_dealloc_cmap(&info->cmap);
> > > > -err2:
> > > > +unmap_regs:
> > > > of_iounmap(&dev->resource[0], par->regs,
> > > > resource_size(&dev->resource[0]));
> > > > -err1:
> > > > +release_regs:
> > > > release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
> > > > -err:
> > > > +free_fb:
> > > > framebuffer_release(info);
> > > >
> > > > return retval;
> > > > --
> > > > 1.7.10
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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] OMAPDSS: HDMI: Replace spinlock with mutex in hdmi_check_hpd_state
From: jaswinder.singh @ 2012-06-23 8:19 UTC (permalink / raw)
To: tomi.valkeinen, mythripk
Cc: linux-omap, linux-fbdev, andy.green, n-dechesne, Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
State change of HDMI PHY could potentially take many millisecs, we can do
better by protecting things in hdmi_set_phy_pwr() with a mutex rather than
a spin_lock_irqsave.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/video/omap2/dss/hdmi.c | 1 +
drivers/video/omap2/dss/ti_hdmi.h | 1 +
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 6 ++----
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 8195c71..0738090 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -758,6 +758,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
hdmi.ip_data.phy_offset = HDMI_PHY;
+ mutex_init(&hdmi.ip_data.lock);
hdmi_panel_init();
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index d174ca1..cc292b8 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,6 +177,7 @@ struct hdmi_ip_data {
/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
int hpd_gpio;
+ struct mutex lock;
};
int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 3fa3d98..04acca9 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -238,10 +238,8 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
unsigned long flags;
bool hpd;
int r;
- /* this should be in ti_hdmi_4xxx_ip private data */
- static DEFINE_SPINLOCK(phy_tx_lock);
- spin_lock_irqsave(&phy_tx_lock, flags);
+ mutex_lock(&ip_data->lock);
hpd = gpio_get_value(ip_data->hpd_gpio);
@@ -257,7 +255,7 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
}
err:
- spin_unlock_irqrestore(&phy_tx_lock, flags);
+ mutex_unlock(&ip_data->lock);
return r;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH] OMAPDSS: HDMI: Discard phy_tx_enabled member
From: jaswinder.singh @ 2012-06-23 8:19 UTC (permalink / raw)
To: tomi.valkeinen, mythripk
Cc: linux-omap, linux-fbdev, andy.green, n-dechesne, Jassi Brar
In-Reply-To: <1339797701-11540-1-git-send-email-jaswinder.singh@linaro.org>
From: Jassi Brar <jaswinder.singh@linaro.org>
It is simpler to read the current status from a register as compared
to maintaining a state variable to hold the information.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/video/omap2/dss/ti_hdmi.h | 1 -
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 11 ++++-------
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index e734cb4..d174ca1 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -177,7 +177,6 @@ struct hdmi_ip_data {
/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
int hpd_gpio;
- bool phy_tx_enabled;
};
int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 4dae1b2..3fa3d98 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -157,6 +157,10 @@ static int hdmi_pll_init(struct hdmi_ip_data *ip_data)
/* PHY_PWR_CMD */
static int hdmi_set_phy_pwr(struct hdmi_ip_data *ip_data, enum hdmi_phy_pwr val)
{
+ /* Return if already the state */
+ if (REG_GET(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, 5, 4) = val)
+ return 0;
+
/* Command for power control of HDMI PHY */
REG_FLD_MOD(hdmi_wp_base(ip_data), HDMI_WP_PWR_CTRL, val, 7, 6);
@@ -241,11 +245,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
hpd = gpio_get_value(ip_data->hpd_gpio);
- if (hpd = ip_data->phy_tx_enabled) {
- spin_unlock_irqrestore(&phy_tx_lock, flags);
- return 0;
- }
-
if (hpd)
r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
else
@@ -257,7 +256,6 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
goto err;
}
- ip_data->phy_tx_enabled = hpd;
err:
spin_unlock_irqrestore(&phy_tx_lock, flags);
return r;
@@ -327,7 +325,6 @@ void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data)
free_irq(gpio_to_irq(ip_data->hpd_gpio), ip_data);
hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_OFF);
- ip_data->phy_tx_enabled = false;
}
static int hdmi_core_ddc_init(struct hdmi_ip_data *ip_data)
--
1.7.4.1
^ permalink raw reply related
* [PATCH] OMAPDSS: HDMI: Cache EDID
From: jaswinder.singh @ 2012-06-23 8:18 UTC (permalink / raw)
To: tomi.valkeinen, mythripk
Cc: linux-omap, linux-fbdev, andy.green, n-dechesne, Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
We can easily keep track of latest EDID from the display and hence avoid
expensive EDID re-reads over I2C.
This could also help some cheapo displays that provide EDID reliably only
immediately after asserting HPD and not later.
Even with good displays, there is something in OMAPDSS that apparantly
messes up DDC occasionally while EDID is being read, giving the
"operation stopped when reading edid" error.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
drivers/video/omap2/dss/hdmi.c | 1 +
drivers/video/omap2/dss/ti_hdmi.h | 2 ++
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 23 ++++++++++++++++++++---
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 900e621..0a8c825 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
hdmi.ip_data.phy_offset = HDMI_PHY;
+ hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
mutex_init(&hdmi.ip_data.lock);
hdmi_panel_init();
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index cc292b8..4735860 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -178,6 +178,8 @@ struct hdmi_ip_data {
/* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
int hpd_gpio;
struct mutex lock;
+ u8 edid_cached[256];
+ unsigned edid_len;
};
int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 04acca9..2633ade 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
hpd = gpio_get_value(ip_data->hpd_gpio);
- if (hpd)
+ if (hpd) {
r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
- else
+ } else {
+ /* Invalidate EDID Cache */
+ ip_data->edid_len = 0;
r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
+ }
if (r) {
DSSERR("Failed to %s PHY TX power\n",
@@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
{
int r, l;
+ if (ip_data->edid_len) {
+ memcpy(edid, ip_data->edid_cached, ip_data->edid_len);
+ return ip_data->edid_len;
+ }
+
if (len < 128)
return -EINVAL;
@@ -474,12 +482,21 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
l += 128;
}
+ ip_data->edid_len = l;
+ memcpy(ip_data->edid_cached, edid, l);
+
return l;
}
bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
{
- return gpio_get_value(ip_data->hpd_gpio);
+ if (gpio_get_value(ip_data->hpd_gpio))
+ return true;
+
+ /* Invalidate EDID Cache */
+ ip_data->edid_len = 0;
+
+ return false;
}
static void hdmi_core_init(struct hdmi_core_video_config *video_cfg,
--
1.7.4.1
^ permalink raw reply related
* [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: jaswinder.singh @ 2012-06-23 8:18 UTC (permalink / raw)
To: tomi.valkeinen, mythripk
Cc: linux-omap, linux-fbdev, andy.green, n-dechesne, Jassi Brar
From: Jassi Brar <jaswinder.singh@linaro.org>
If the runtime PM of the device is disabled (for example in resume from
suspend path), it doesn't make sense to attempt pm_runtime_get/put, esp
when their return values affect the control flow path.
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.
drivers/video/omap2/dss/dispc.c | 6 ++++++
drivers/video/omap2/dss/dsi.c | 6 ++++++
drivers/video/omap2/dss/dss.c | 6 ++++++
drivers/video/omap2/dss/hdmi.c | 6 ++++++
drivers/video/omap2/dss/rfbi.c | 6 ++++++
drivers/video/omap2/dss/venc.c | 6 ++++++
6 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 4749ac3..2c3266f 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -372,6 +372,9 @@ int dispc_runtime_get(void)
DSSDBG("dispc_runtime_get\n");
+ if (!pm_runtime_enabled(&dispc.pdev->dev))
+ return 0;
+
r = pm_runtime_get_sync(&dispc.pdev->dev);
WARN_ON(r < 0);
return r < 0 ? r : 0;
@@ -383,6 +386,9 @@ void dispc_runtime_put(void)
DSSDBG("dispc_runtime_put\n");
+ if (!pm_runtime_enabled(&dispc.pdev->dev))
+ return;
+
r = pm_runtime_put_sync(&dispc.pdev->dev);
WARN_ON(r < 0);
}
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index ca8382d..6db4cb1 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1062,6 +1062,9 @@ int dsi_runtime_get(struct platform_device *dsidev)
DSSDBG("dsi_runtime_get\n");
+ if (!pm_runtime_enabled(&dsi->pdev->dev))
+ return 0;
+
r = pm_runtime_get_sync(&dsi->pdev->dev);
WARN_ON(r < 0);
return r < 0 ? r : 0;
@@ -1074,6 +1077,9 @@ void dsi_runtime_put(struct platform_device *dsidev)
DSSDBG("dsi_runtime_put\n");
+ if (!pm_runtime_enabled(&dsi->pdev->dev))
+ return;
+
r = pm_runtime_put_sync(&dsi->pdev->dev);
WARN_ON(r < 0);
}
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 7706323..5e52224 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -719,6 +719,9 @@ static int dss_runtime_get(void)
DSSDBG("dss_runtime_get\n");
+ if (!pm_runtime_enabled(&dss.pdev->dev))
+ return 0;
+
r = pm_runtime_get_sync(&dss.pdev->dev);
WARN_ON(r < 0);
return r < 0 ? r : 0;
@@ -730,6 +733,9 @@ static void dss_runtime_put(void)
DSSDBG("dss_runtime_put\n");
+ if (!pm_runtime_enabled(&dss.pdev->dev))
+ return;
+
r = pm_runtime_put_sync(&dss.pdev->dev);
WARN_ON(r < 0 && r != -EBUSY);
}
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 0738090..900e621 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -123,6 +123,9 @@ static int hdmi_runtime_get(void)
DSSDBG("hdmi_runtime_get\n");
+ if (!pm_runtime_enabled(&hdmi.pdev->dev))
+ return 0;
+
r = pm_runtime_get_sync(&hdmi.pdev->dev);
WARN_ON(r < 0);
if (r < 0)
@@ -137,6 +140,9 @@ static void hdmi_runtime_put(void)
DSSDBG("hdmi_runtime_put\n");
+ if (!pm_runtime_enabled(&hdmi.pdev->dev))
+ return;
+
r = pm_runtime_put_sync(&hdmi.pdev->dev);
WARN_ON(r < 0);
}
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 3d8c206..401384a 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -129,6 +129,9 @@ static int rfbi_runtime_get(void)
DSSDBG("rfbi_runtime_get\n");
+ if (!pm_runtime_enabled(&rfbi.pdev->dev))
+ return 0;
+
r = pm_runtime_get_sync(&rfbi.pdev->dev);
WARN_ON(r < 0);
return r < 0 ? r : 0;
@@ -140,6 +143,9 @@ static void rfbi_runtime_put(void)
DSSDBG("rfbi_runtime_put\n");
+ if (!pm_runtime_enabled(&rfbi.pdev->dev))
+ return;
+
r = pm_runtime_put_sync(&rfbi.pdev->dev);
WARN_ON(r < 0);
}
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 2b89739..edd8710 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -390,6 +390,9 @@ static int venc_runtime_get(void)
DSSDBG("venc_runtime_get\n");
+ if (!pm_runtime_enabled(&venc.pdev->dev))
+ return 0;
+
r = pm_runtime_get_sync(&venc.pdev->dev);
WARN_ON(r < 0);
return r < 0 ? r : 0;
@@ -401,6 +404,9 @@ static void venc_runtime_put(void)
DSSDBG("venc_runtime_put\n");
+ if (!pm_runtime_enabled(&venc.pdev->dev))
+ return;
+
r = pm_runtime_put_sync(&venc.pdev->dev);
WARN_ON(r < 0);
}
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] grvga: Fix error handling issues
From: Julia Lawall @ 2012-06-22 16:21 UTC (permalink / raw)
To: Emil Goode
Cc: Julia Lawall, FlorianSchandinat, linux-fbdev, linux-kernel,
kernel-janitors
In-Reply-To: <1340382044.3963.2.camel@localhost>
On Fri, 22 Jun 2012, Emil Goode wrote:
> Good idea, I will take another look at it tomorrow.
There is a devm_ function that combines request_mem_region and ioremap
that could be useful.
julia
>
> Thanks,
>
> Emil
>
> On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
> > Maybe you could used the devm_ functions for request_mem_region and
> > ioremap so that the error handling can just be dropped?
> >
> > julia
> >
> > On Fri, 22 Jun 2012, Emil Goode wrote:
> >
> > > This patch fixes two problems with the error handling in the
> > > grvga_probe function.
> > >
> > > - If the call to grvga_parse_custom on line 370 fails we use
> > > the wrong label so that release_mem_region will be called
> > > without a call to request_mem_region being made.
> > >
> > > - If the call to ioremap on line 436 fails we should not try
> > > to call iounmap. I added an if statement to check whether or
> > > not a call to iounmap should be made.
> > >
> > > - I also changed the names of the labels to make the code
> > > easier to read.
> > >
> > > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > > ---
> > > drivers/video/grvga.c | 34 ++++++++++++++++++----------------
> > > 1 file changed, 18 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
> > > index da066c2..d9d688a 100644
> > > --- a/drivers/video/grvga.c
> > > +++ b/drivers/video/grvga.c
> > > @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > */
> > > if (fb_get_options("grvga", &options)) {
> > > retval = -ENODEV;
> > > - goto err;
> > > + goto free_fb;
> > > }
> > >
> > > if (!options || !*options)
> > > @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > if (grvga_parse_custom(this_opt, &info->var) < 0) {
> > > dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
> > > retval = -EINVAL;
> > > - goto err1;
> > > + goto free_fb;
> > > }
> > > } else if (!strncmp(this_opt, "addr", 4))
> > > grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
> > > @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
> > > dev_err(&dev->dev, "registers already mapped\n");
> > > retval = -EBUSY;
> > > - goto err;
> > > + goto free_fb;
> > > }
> > >
> > > par->regs = of_ioremap(&dev->resource[0], 0,
> > > @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > if (!par->regs) {
> > > dev_err(&dev->dev, "failed to map registers\n");
> > > retval = -ENOMEM;
> > > - goto err1;
> > > + goto release_regs;
> > > }
> > >
> > > retval = fb_alloc_cmap(&info->cmap, 256, 0);
> > > if (retval < 0) {
> > > dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
> > > retval = -ENOMEM;
> > > - goto err2;
> > > + goto unmap_regs;
> > > }
> > >
> > > if (mode_opt) {
> > > @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
> > > if (!retval || retval = 4) {
> > > retval = -EINVAL;
> > > - goto err3;
> > > + goto dealloc_cmap;
> > > }
> > > }
> > >
> > > @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
> > > dev_err(&dev->dev, "failed to request memory region\n");
> > > retval = -ENOMEM;
> > > - goto err3;
> > > + goto dealloc_cmap;
> > > }
> > >
> > > virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
> > > @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > if (!virtual_start) {
> > > dev_err(&dev->dev, "error mapping framebuffer memory\n");
> > > retval = -ENOMEM;
> > > - goto err4;
> > > + goto free_mem;
> > > }
> > > } else { /* Allocate frambuffer memory */
> > >
> > > @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > "unable to allocate framebuffer memory (%lu bytes)\n",
> > > grvga_mem_size);
> > > retval = -ENOMEM;
> > > - goto err3;
> > > + goto dealloc_cmap;
> > > }
> > >
> > > physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
> > > @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > retval = register_framebuffer(info);
> > > if (retval < 0) {
> > > dev_err(&dev->dev, "failed to register framebuffer\n");
> > > - goto err4;
> > > + goto free_mem;
> > > }
> > >
> > > __raw_writel(physical_start, &par->regs->fb_pos);
> > > @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >
> > > return 0;
> > >
> > > -err4:
> > > +free_mem:
> > > dev_set_drvdata(&dev->dev, NULL);
> > > if (grvga_fix_addr) {
> > > release_mem_region(physical_start, grvga_mem_size);
> > > - iounmap((void *)virtual_start);
> > > + if (virtual_start)
> > > + iounmap((void *)virtual_start);
> > > } else
> > > kfree((void *)virtual_start);
> > > -err3:
> > > +
> > > +dealloc_cmap:
> > > fb_dealloc_cmap(&info->cmap);
> > > -err2:
> > > +unmap_regs:
> > > of_iounmap(&dev->resource[0], par->regs,
> > > resource_size(&dev->resource[0]));
> > > -err1:
> > > +release_regs:
> > > release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
> > > -err:
> > > +free_fb:
> > > framebuffer_release(info);
> > >
> > > return retval;
> > > --
> > > 1.7.10
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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] grvga: Fix error handling issues
From: Emil Goode @ 2012-06-22 16:20 UTC (permalink / raw)
To: Julia Lawall
Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors
In-Reply-To: <alpine.DEB.2.02.1206221811150.1901@hadrien>
Good idea, I will take another look at it tomorrow.
Thanks,
Emil
On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
> Maybe you could used the devm_ functions for request_mem_region and
> ioremap so that the error handling can just be dropped?
>
> julia
>
> On Fri, 22 Jun 2012, Emil Goode wrote:
>
> > This patch fixes two problems with the error handling in the
> > grvga_probe function.
> >
> > - If the call to grvga_parse_custom on line 370 fails we use
> > the wrong label so that release_mem_region will be called
> > without a call to request_mem_region being made.
> >
> > - If the call to ioremap on line 436 fails we should not try
> > to call iounmap. I added an if statement to check whether or
> > not a call to iounmap should be made.
> >
> > - I also changed the names of the labels to make the code
> > easier to read.
> >
> > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > ---
> > drivers/video/grvga.c | 34 ++++++++++++++++++----------------
> > 1 file changed, 18 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
> > index da066c2..d9d688a 100644
> > --- a/drivers/video/grvga.c
> > +++ b/drivers/video/grvga.c
> > @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > */
> > if (fb_get_options("grvga", &options)) {
> > retval = -ENODEV;
> > - goto err;
> > + goto free_fb;
> > }
> >
> > if (!options || !*options)
> > @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > if (grvga_parse_custom(this_opt, &info->var) < 0) {
> > dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
> > retval = -EINVAL;
> > - goto err1;
> > + goto free_fb;
> > }
> > } else if (!strncmp(this_opt, "addr", 4))
> > grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
> > @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
> > dev_err(&dev->dev, "registers already mapped\n");
> > retval = -EBUSY;
> > - goto err;
> > + goto free_fb;
> > }
> >
> > par->regs = of_ioremap(&dev->resource[0], 0,
> > @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > if (!par->regs) {
> > dev_err(&dev->dev, "failed to map registers\n");
> > retval = -ENOMEM;
> > - goto err1;
> > + goto release_regs;
> > }
> >
> > retval = fb_alloc_cmap(&info->cmap, 256, 0);
> > if (retval < 0) {
> > dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
> > retval = -ENOMEM;
> > - goto err2;
> > + goto unmap_regs;
> > }
> >
> > if (mode_opt) {
> > @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
> > if (!retval || retval = 4) {
> > retval = -EINVAL;
> > - goto err3;
> > + goto dealloc_cmap;
> > }
> > }
> >
> > @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
> > dev_err(&dev->dev, "failed to request memory region\n");
> > retval = -ENOMEM;
> > - goto err3;
> > + goto dealloc_cmap;
> > }
> >
> > virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
> > @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > if (!virtual_start) {
> > dev_err(&dev->dev, "error mapping framebuffer memory\n");
> > retval = -ENOMEM;
> > - goto err4;
> > + goto free_mem;
> > }
> > } else { /* Allocate frambuffer memory */
> >
> > @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > "unable to allocate framebuffer memory (%lu bytes)\n",
> > grvga_mem_size);
> > retval = -ENOMEM;
> > - goto err3;
> > + goto dealloc_cmap;
> > }
> >
> > physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
> > @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > retval = register_framebuffer(info);
> > if (retval < 0) {
> > dev_err(&dev->dev, "failed to register framebuffer\n");
> > - goto err4;
> > + goto free_mem;
> > }
> >
> > __raw_writel(physical_start, &par->regs->fb_pos);
> > @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
> >
> > return 0;
> >
> > -err4:
> > +free_mem:
> > dev_set_drvdata(&dev->dev, NULL);
> > if (grvga_fix_addr) {
> > release_mem_region(physical_start, grvga_mem_size);
> > - iounmap((void *)virtual_start);
> > + if (virtual_start)
> > + iounmap((void *)virtual_start);
> > } else
> > kfree((void *)virtual_start);
> > -err3:
> > +
> > +dealloc_cmap:
> > fb_dealloc_cmap(&info->cmap);
> > -err2:
> > +unmap_regs:
> > of_iounmap(&dev->resource[0], par->regs,
> > resource_size(&dev->resource[0]));
> > -err1:
> > +release_regs:
> > release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
> > -err:
> > +free_fb:
> > framebuffer_release(info);
> >
> > return retval;
> > --
> > 1.7.10
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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] grvga: Fix error handling issues
From: Julia Lawall @ 2012-06-22 16:12 UTC (permalink / raw)
To: Emil Goode; +Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors
In-Reply-To: <1340381479-6371-1-git-send-email-emilgoode@gmail.com>
Maybe you could used the devm_ functions for request_mem_region and
ioremap so that the error handling can just be dropped?
julia
On Fri, 22 Jun 2012, Emil Goode wrote:
> This patch fixes two problems with the error handling in the
> grvga_probe function.
>
> - If the call to grvga_parse_custom on line 370 fails we use
> the wrong label so that release_mem_region will be called
> without a call to request_mem_region being made.
>
> - If the call to ioremap on line 436 fails we should not try
> to call iounmap. I added an if statement to check whether or
> not a call to iounmap should be made.
>
> - I also changed the names of the labels to make the code
> easier to read.
>
> Signed-off-by: Emil Goode <emilgoode@gmail.com>
> ---
> drivers/video/grvga.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
> index da066c2..d9d688a 100644
> --- a/drivers/video/grvga.c
> +++ b/drivers/video/grvga.c
> @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> */
> if (fb_get_options("grvga", &options)) {
> retval = -ENODEV;
> - goto err;
> + goto free_fb;
> }
>
> if (!options || !*options)
> @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> if (grvga_parse_custom(this_opt, &info->var) < 0) {
> dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
> retval = -EINVAL;
> - goto err1;
> + goto free_fb;
> }
> } else if (!strncmp(this_opt, "addr", 4))
> grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
> @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
> dev_err(&dev->dev, "registers already mapped\n");
> retval = -EBUSY;
> - goto err;
> + goto free_fb;
> }
>
> par->regs = of_ioremap(&dev->resource[0], 0,
> @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
> if (!par->regs) {
> dev_err(&dev->dev, "failed to map registers\n");
> retval = -ENOMEM;
> - goto err1;
> + goto release_regs;
> }
>
> retval = fb_alloc_cmap(&info->cmap, 256, 0);
> if (retval < 0) {
> dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
> retval = -ENOMEM;
> - goto err2;
> + goto unmap_regs;
> }
>
> if (mode_opt) {
> @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
> if (!retval || retval = 4) {
> retval = -EINVAL;
> - goto err3;
> + goto dealloc_cmap;
> }
> }
>
> @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
> dev_err(&dev->dev, "failed to request memory region\n");
> retval = -ENOMEM;
> - goto err3;
> + goto dealloc_cmap;
> }
>
> virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
> @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> if (!virtual_start) {
> dev_err(&dev->dev, "error mapping framebuffer memory\n");
> retval = -ENOMEM;
> - goto err4;
> + goto free_mem;
> }
> } else { /* Allocate frambuffer memory */
>
> @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> "unable to allocate framebuffer memory (%lu bytes)\n",
> grvga_mem_size);
> retval = -ENOMEM;
> - goto err3;
> + goto dealloc_cmap;
> }
>
> physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
> @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> retval = register_framebuffer(info);
> if (retval < 0) {
> dev_err(&dev->dev, "failed to register framebuffer\n");
> - goto err4;
> + goto free_mem;
> }
>
> __raw_writel(physical_start, &par->regs->fb_pos);
> @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
>
> return 0;
>
> -err4:
> +free_mem:
> dev_set_drvdata(&dev->dev, NULL);
> if (grvga_fix_addr) {
> release_mem_region(physical_start, grvga_mem_size);
> - iounmap((void *)virtual_start);
> + if (virtual_start)
> + iounmap((void *)virtual_start);
> } else
> kfree((void *)virtual_start);
> -err3:
> +
> +dealloc_cmap:
> fb_dealloc_cmap(&info->cmap);
> -err2:
> +unmap_regs:
> of_iounmap(&dev->resource[0], par->regs,
> resource_size(&dev->resource[0]));
> -err1:
> +release_regs:
> release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
> -err:
> +free_fb:
> framebuffer_release(info);
>
> return retval;
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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] grvga: Fix error handling issues
From: Emil Goode @ 2012-06-22 16:11 UTC (permalink / raw)
To: FlorianSchandinat; +Cc: linux-fbdev, linux-kernel, kernel-janitors, Emil Goode
This patch fixes two problems with the error handling in the
grvga_probe function.
- If the call to grvga_parse_custom on line 370 fails we use
the wrong label so that release_mem_region will be called
without a call to request_mem_region being made.
- If the call to ioremap on line 436 fails we should not try
to call iounmap. I added an if statement to check whether or
not a call to iounmap should be made.
- I also changed the names of the labels to make the code
easier to read.
Signed-off-by: Emil Goode <emilgoode@gmail.com>
---
drivers/video/grvga.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
index da066c2..d9d688a 100644
--- a/drivers/video/grvga.c
+++ b/drivers/video/grvga.c
@@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
*/
if (fb_get_options("grvga", &options)) {
retval = -ENODEV;
- goto err;
+ goto free_fb;
}
if (!options || !*options)
@@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (grvga_parse_custom(this_opt, &info->var) < 0) {
dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
retval = -EINVAL;
- goto err1;
+ goto free_fb;
}
} else if (!strncmp(this_opt, "addr", 4))
grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
@@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
dev_err(&dev->dev, "registers already mapped\n");
retval = -EBUSY;
- goto err;
+ goto free_fb;
}
par->regs = of_ioremap(&dev->resource[0], 0,
@@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!par->regs) {
dev_err(&dev->dev, "failed to map registers\n");
retval = -ENOMEM;
- goto err1;
+ goto release_regs;
}
retval = fb_alloc_cmap(&info->cmap, 256, 0);
if (retval < 0) {
dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
retval = -ENOMEM;
- goto err2;
+ goto unmap_regs;
}
if (mode_opt) {
@@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
if (!retval || retval = 4) {
retval = -EINVAL;
- goto err3;
+ goto dealloc_cmap;
}
}
@@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
dev_err(&dev->dev, "failed to request memory region\n");
retval = -ENOMEM;
- goto err3;
+ goto dealloc_cmap;
}
virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
@@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
if (!virtual_start) {
dev_err(&dev->dev, "error mapping framebuffer memory\n");
retval = -ENOMEM;
- goto err4;
+ goto free_mem;
}
} else { /* Allocate frambuffer memory */
@@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
"unable to allocate framebuffer memory (%lu bytes)\n",
grvga_mem_size);
retval = -ENOMEM;
- goto err3;
+ goto dealloc_cmap;
}
physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
@@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
retval = register_framebuffer(info);
if (retval < 0) {
dev_err(&dev->dev, "failed to register framebuffer\n");
- goto err4;
+ goto free_mem;
}
__raw_writel(physical_start, &par->regs->fb_pos);
@@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
return 0;
-err4:
+free_mem:
dev_set_drvdata(&dev->dev, NULL);
if (grvga_fix_addr) {
release_mem_region(physical_start, grvga_mem_size);
- iounmap((void *)virtual_start);
+ if (virtual_start)
+ iounmap((void *)virtual_start);
} else
kfree((void *)virtual_start);
-err3:
+
+dealloc_cmap:
fb_dealloc_cmap(&info->cmap);
-err2:
+unmap_regs:
of_iounmap(&dev->resource[0], par->regs,
resource_size(&dev->resource[0]));
-err1:
+release_regs:
release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
-err:
+free_fb:
framebuffer_release(info);
return retval;
--
1.7.10
^ permalink raw reply related
* [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Rajendra Nayak @ 2012-06-22 13:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340372890-10091-1-git-send-email-rnayak@ti.com>
In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
and clk_unprepare() for the omapdss clocks.
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: <linux-fbdev@vger.kernel.org>
---
drivers/video/omap2/dss/dsi.c | 5 ++++-
drivers/video/omap2/dss/hdmi.c | 5 ++++-
drivers/video/omap2/dss/venc.c | 5 ++++-
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index ca8382d..81b474d 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4776,6 +4776,7 @@ static int dsi_get_clocks(struct platform_device *dsidev)
}
dsi->sys_clk = clk;
+ clk_prepare(dsi->sys_clk);
return 0;
}
@@ -4786,8 +4787,10 @@ static void dsi_put_clocks(struct platform_device *dsidev)
if (dsi->dss_clk)
clk_put(dsi->dss_clk);
- if (dsi->sys_clk)
+ if (dsi->sys_clk) {
+ clk_unprepare(dsi->sys_clk);
clk_put(dsi->sys_clk);
+ }
}
static void __init dsi_probe_pdata(struct platform_device *dsidev)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 8195c71..0b7ded3 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -544,14 +544,17 @@ static int hdmi_get_clocks(struct platform_device *pdev)
}
hdmi.sys_clk = clk;
+ clk_prepare(hdmi.sys_clk);
return 0;
}
static void hdmi_put_clocks(void)
{
- if (hdmi.sys_clk)
+ if (hdmi.sys_clk) {
+ clk_unprepare(hdmi.sys_clk);
clk_put(hdmi.sys_clk);
+ }
}
#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 2b89739..62f395d 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -818,14 +818,17 @@ static int venc_get_clocks(struct platform_device *pdev)
}
venc.tv_dac_clk = clk;
+ clk_prepare(venc.tv_dac_clk);
return 0;
}
static void venc_put_clocks(void)
{
- if (venc.tv_dac_clk)
+ if (venc.tv_dac_clk) {
+ clk_unprepare(venc.tv_dac_clk);
clk_put(venc.tv_dac_clk);
+ }
}
static void __init venc_probe_pdata(struct platform_device *pdev)
--
1.7.1
^ permalink raw reply related
* [PATCH] fbdev: Add ioctls FBIO_COPYRECT, FBIO_FILLRECT, lest these operations be done in software.
From: Strake @ 2012-06-21 12:22 UTC (permalink / raw)
To: Florian Tobias Schandinat, linux-fbdev, linux-kernel
From: Matthew Farkas-Dyck <strake888@gmail.com>
Add ioctls FBIO_COPYRECT, FBIO_FILLRECT, lest these operations be done
in software.
Signed-off-by: Matthew Farkas-Dyck <strake888@gmail.com>
---
Reason: If it can be done in hardware, it ought to be. This will
enable user-mode software to do so.
commit 2c002c58b6c5d5b211423da8eaa242ca443fdd72
Author: strake <strake888@gmail.com>
Date: Wed Jan 1 19:10:24 2003 -0500
Add ioctls FBIO_COPYRECT, FBIO_FILLRECT
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 4ac1201..f525e91 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1019,6 +1019,8 @@ static long do_fb_ioctl(struct fb_info *info,
unsigned int cmd,
struct fb_cmap cmap_from;
struct fb_cmap_user cmap;
struct fb_event event;
+ struct fb_copyarea copy;
+ struct fb_fillrect fill;
void __user *argp = (void __user *)arg;
long ret = 0;
@@ -1126,6 +1128,26 @@ static long do_fb_ioctl(struct fb_info *info,
unsigned int cmd,
console_unlock();
unlock_fb_info(info);
break;
+ case FBIO_FILLRECT:
+ if (copy_from_user(&fill, argp, sizeof(fill)))
+ return -EFAULT;
+ if (!lock_fb_info(info))
+ return -ENODEV;
+ console_lock();
+ (*(info->fbops->fb_fillrect))(info, &fill);
+ console_unlock();
+ unlock_fb_info(info);
+ break;
+ case FBIO_COPYRECT:
+ if (copy_from_user(©, argp, sizeof(copy)))
+ return -EFAULT;
+ if (!lock_fb_info(info))
+ return -ENODEV;
+ console_lock();
+ (*(info->fbops->fb_copyarea))(info, ©);
+ console_unlock();
+ unlock_fb_info(info);
+ break;
default:
if (!lock_fb_info(info))
return -ENODEV;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index d1631d3..fc18459 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -39,6 +39,8 @@
#define FBIOPUT_MODEINFO 0x4617
#define FBIOGET_DISPINFO 0x4618
#define FBIO_WAITFORVSYNC _IOW('F', 0x20, __u32)
+#define FBIO_COPYRECT _IOW('F', 0x22, struct fb_copyarea)
+#define FBIO_FILLRECT _IOW('F', 0x23, struct fb_fillrect)
#define FB_TYPE_PACKED_PIXELS 0 /* Packed Pixels */
#define FB_TYPE_PLANES 1 /* Non interleaved planes */
^ permalink raw reply related
* [PATCH] video: exynos_dp: remove duplicated declarations from header file
From: Jingoo Han @ 2012-06-21 9:57 UTC (permalink / raw)
To: linux-fbdev
Some functions are declared twice in header file; thus, these
declarations are unnecessary.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/exynos/exynos_dp_core.h | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
index 1e0f998..8526e54 100644
--- a/drivers/video/exynos/exynos_dp_core.h
+++ b/drivers/video/exynos/exynos_dp_core.h
@@ -85,10 +85,6 @@ void exynos_dp_set_link_bandwidth(struct exynos_dp_device *dp, u32 bwtype);
void exynos_dp_get_link_bandwidth(struct exynos_dp_device *dp, u32 *bwtype);
void exynos_dp_set_lane_count(struct exynos_dp_device *dp, u32 count);
void exynos_dp_get_lane_count(struct exynos_dp_device *dp, u32 *count);
-void exynos_dp_set_link_bandwidth(struct exynos_dp_device *dp, u32 bwtype);
-void exynos_dp_get_link_bandwidth(struct exynos_dp_device *dp, u32 *bwtype);
-void exynos_dp_set_lane_count(struct exynos_dp_device *dp, u32 count);
-void exynos_dp_get_lane_count(struct exynos_dp_device *dp, u32 *count);
void exynos_dp_enable_enhanced_mode(struct exynos_dp_device *dp, bool enable);
void exynos_dp_set_training_pattern(struct exynos_dp_device *dp,
enum pattern_set pattern);
--
1.7.1
^ permalink raw reply related
* Re: [Linaro-mm-sig] [RFC/PATCH] fb: Add dma-buf support
From: Laurent Pinchart @ 2012-06-21 9:05 UTC (permalink / raw)
To: Tomasz Stanislawski; +Cc: linux-fbdev, linaro-mm-sig, linux-media
In-Reply-To: <4FE1EF42.1000403@samsung.com>
Hi Tomasz,
Thank you for the review.
On Wednesday 20 June 2012 17:41:54 Tomasz Stanislawski wrote:
> Hi Laurent,
> Thank you for the patch.
>
> On 06/20/2012 04:09 PM, Laurent Pinchart wrote:
> > Add support for the dma-buf exporter role to the frame buffer API. The
> > importer role isn't meaningful for frame buffer devices, as the frame
> > buffer device model doesn't allow using externally allocated memory.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > Documentation/fb/api.txt | 36 ++++++++++++++++++++++++++++++++++++
> > drivers/video/fbmem.c | 36 ++++++++++++++++++++++++++++++++++++
> > include/linux/fb.h | 12 ++++++++++++
> > 3 files changed, 84 insertions(+), 0 deletions(-)
>
> [snip]
>
> > +The export a frame buffer as a dma-buf file descriptors, applications
> > call the +FBIOGET_DMABUF ioctl. The ioctl takes a pointer to a
> > fb_dmabuf_export +structure.
> > +
> > +struct fb_dmabuf_export {
> > + __u32 fd;
> > + __u32 flags;
> > +};
>
> What do you think about adding some reserved fields to struct
> fb_dmabuf_export to make it future-proof for DMABUF extensions?
Already done. I've added them at the last minute, and for some reason I've
sent the previous version of the patch.
> > +
> > +The flag field specifies the flags to be used when creating the dma-buf
> > file +descriptor. The only supported flag is O_CLOEXEC. If the call is
> > successful, +the driver will set the fd field to a file descriptor
> > corresponding to the +dma-buf object.
> > +
> > +Applications can then pass the file descriptors to another application or
> > +another device driver. The dma-buf object is automatically
> > reference-counted, +applications can and should close the file descriptor
> > as soon as they don't +need it anymore. The underlying dma-buf object
> > will not be freed before the +last device that uses the dma-buf object
> > releases it.
> > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> > index 0dff12a..400e449 100644
> > --- a/drivers/video/fbmem.c
> > +++ b/drivers/video/fbmem.c
> > @@ -15,6 +15,7 @@
> >
> > #include <linux/compat.h>
> > #include <linux/types.h>
> >
> > +#include <linux/dma-buf.h>
> >
> > #include <linux/errno.h>
> > #include <linux/kernel.h>
> > #include <linux/major.h>
> >
> > @@ -1074,6 +1075,23 @@ fb_blank(struct fb_info *info, int blank)
> >
> > return ret;
> >
> > }
> >
> > +#ifdef CONFIG_DMA_SHARED_BUFFER
> > +int
> > +fb_get_dmabuf(struct fb_info *info, int flags)
> > +{
> > + struct dma_buf *dmabuf;
> > +
> > + if (info->fbops->fb_dmabuf_export = NULL)
> > + return -ENOTTY;
> > +
> > + dmabuf = info->fbops->fb_dmabuf_export(info);
>
> IMO, it is not a good idea to delegate an implementation of DMABUF ops to
> the driver. Maybe exporting could be handled inside FB stack. As I
> understand, the FB stack needs only 'get-scatterlist' ops from an FB driver.
> All other DMABUF magic does not need driver involvement, does it?
Beside creating the sg-list, drivers also need to prevent the underlying
memory from being freed. If an application requests a frame buffer format or
size change, the driver could resize the underlying memory, which needs to be
prevented as long as the buffer is exported.
The dma-buf support implementation in the sh_mobile_lcdcfb driver also takes a
reference to the device and the driver for the duration of the export, to
prevent the driver from being unloaded and the device from being released.
static struct dma_buf *sh_mobile_dmabuf_export(struct fb_info *info)
{
struct sh_mobile_lcdc_chan *ch = info->par;
struct dma_buf *dmabuf;
dmabuf = dma_buf_export(ch, &sh_mobile_lcdc_dmabuf_ops, ch->fb_size, 0);
if (!IS_ERR(dmabuf)) {
/* We already hold references to the module and the device, so
* we don't have to take them prior to calling dma_buf_export().
*/
get_device(ch->lcdc->dev);
__module_get(THIS_MODULE);
}
return dmabuf;
}
This could be moved in the fbdev core though, as ch->lcdc->dev is available
trhough info->dev, and THIS_MODULE could be replaced with info->dev->driver-
>owner.
Moving the implementation to the fbdev core would thus require 3 callbacks to
lock the frame buffer, get the sg list, and release the frame buffer. However,
in practice, we might not have drivers that resize buffers when the format or
size is modified, in which case a single callback would be enough.
> > + if (IS_ERR(dmabuf))
> > + return PTR_ERR(dmabuf);
> > +
> > + return dma_buf_fd(dmabuf, flags);
> > +}
> > +#endif
> > +
>
> [snip]
>
> > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > index ac3f1c6..c9fee75 100644
> > --- a/include/linux/fb.h
> > +++ b/include/linux/fb.h
> > @@ -701,6 +708,11 @@ struct fb_ops {
> >
> > /* called at KDB enter and leave time to prepare the console */
> > int (*fb_debug_enter)(struct fb_info *info);
> > int (*fb_debug_leave)(struct fb_info *info);
> >
> > +
> > +#ifdef CONFIG_DMA_SHARED_BUFFER
> > + /* Export the frame buffer as a dmabuf object */
> > + struct dma_buf *(*fb_dmabuf_export)(struct fb_info *info);
> > +#endif
>
> Memory trashing or even kernel crash may happen if a module compiled
> with CONFIG_DMA_SHARED_BUFFER enabled is loaded into kernel with
> CONFIG_DMA_SHARED_BUFFER disabled.
I'll remove the #ifdef. The memory savings are not worth the potential issues.
> > };
> >
> > #ifdef CONFIG_FB_TILEBLITTING
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [Linaro-mm-sig] [RFC/PATCH] fb: Add dma-buf support
From: Laurent Pinchart @ 2012-06-21 8:46 UTC (permalink / raw)
To: Jesse Barker; +Cc: linux-fbdev, linaro-mm-sig, linux-media
In-Reply-To: <CAC57bwtvfYDGO91r3zRry+WENm7x=UZ6TdOkXpmHDbQWT0feRA@mail.gmail.com>
Hi Jesse,
Thank you for the review.
On Wednesday 20 June 2012 08:09:19 Jesse Barker wrote:
> Laurent,
>
> Your recent documentation efforts are extremely commendable. Just a
> couple of small nits below...
>
> On Wed, Jun 20, 2012 at 7:09 AM, Laurent Pinchart
>
> <laurent.pinchart@ideasonboard.com> wrote:
> > Add support for the dma-buf exporter role to the frame buffer API. The
> > importer role isn't meaningful for frame buffer devices, as the frame
> > buffer device model doesn't allow using externally allocated memory.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Documentation/fb/api.txt | 36 ++++++++++++++++++++++++++++++++++++
> > drivers/video/fbmem.c | 36 ++++++++++++++++++++++++++++++++++++
> > include/linux/fb.h | 12 ++++++++++++
> > 3 files changed, 84 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/fb/api.txt b/Documentation/fb/api.txt
> > index d4ff7de..f0b2173 100644
> > --- a/Documentation/fb/api.txt
> > +++ b/Documentation/fb/api.txt
> > @@ -304,3 +304,39 @@ extensions.
> > Upon successful format configuration, drivers update the
> > fb_fix_screeninfo
> > type, visual and line_length fields depending on the selected format. The
> > type and visual fields are set to FB_TYPE_FOURCC and FB_VISUAL_FOURCC
> > respectively. +
> > +
> > +5. DMA buffer sharing
> > +---------------------
> > +
> > +The dma-buf kernel framework allows DMA buffers to be shared across
> > devices +and applications. Sharing buffers across display devices and
> > video capture or +video decoding devices allow zero-copy operation when
> > displaying video content +produced by a hardware device such as a camera
> > or a hardware codec. This is +crucial to achieve optimal system
> > performances during video display. +
> > +While dma-buf supports both exporting internally allocated memory as a
> > dma-buf +object (known as the exporter role) and importing a dma-buf
> > object to be used +as device memory (known as the importer role), the
> > frame buffer API only +supports the exporter role, as the frame buffer
> > device model doesn't support +using externally-allocated memory.
> > +
> > +The export a frame buffer as a dma-buf file descriptors, applications
> > call the
> s/The/To
> s/descriptors/descriptor
Fixed, thanks.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [Linaro-mm-sig] [RFC/PATCH] fb: Add dma-buf support
From: Tomasz Stanislawski @ 2012-06-20 15:41 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-fbdev, linaro-mm-sig, linux-media
In-Reply-To: <1340201368-20751-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Laurent,
Thank you for the patch.
On 06/20/2012 04:09 PM, Laurent Pinchart wrote:
> Add support for the dma-buf exporter role to the frame buffer API. The
> importer role isn't meaningful for frame buffer devices, as the frame
> buffer device model doesn't allow using externally allocated memory.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Documentation/fb/api.txt | 36 ++++++++++++++++++++++++++++++++++++
> drivers/video/fbmem.c | 36 ++++++++++++++++++++++++++++++++++++
> include/linux/fb.h | 12 ++++++++++++
> 3 files changed, 84 insertions(+), 0 deletions(-)
>
[snip]
> +The export a frame buffer as a dma-buf file descriptors, applications call the
> +FBIOGET_DMABUF ioctl. The ioctl takes a pointer to a fb_dmabuf_export
> +structure.
> +
> +struct fb_dmabuf_export {
> + __u32 fd;
> + __u32 flags;
> +};
What do you think about adding some reserved fields to
struct fb_dmabuf_export to make it future-proof for
DMABUF extensions?
> +
> +The flag field specifies the flags to be used when creating the dma-buf file
> +descriptor. The only supported flag is O_CLOEXEC. If the call is successful,
> +the driver will set the fd field to a file descriptor corresponding to the
> +dma-buf object.
> +
> +Applications can then pass the file descriptors to another application or
> +another device driver. The dma-buf object is automatically reference-counted,
> +applications can and should close the file descriptor as soon as they don't
> +need it anymore. The underlying dma-buf object will not be freed before the
> +last device that uses the dma-buf object releases it.
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 0dff12a..400e449 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -15,6 +15,7 @@
>
> #include <linux/compat.h>
> #include <linux/types.h>
> +#include <linux/dma-buf.h>
> #include <linux/errno.h>
> #include <linux/kernel.h>
> #include <linux/major.h>
> @@ -1074,6 +1075,23 @@ fb_blank(struct fb_info *info, int blank)
> return ret;
> }
>
> +#ifdef CONFIG_DMA_SHARED_BUFFER
> +int
> +fb_get_dmabuf(struct fb_info *info, int flags)
> +{
> + struct dma_buf *dmabuf;
> +
> + if (info->fbops->fb_dmabuf_export = NULL)
> + return -ENOTTY;
> +
> + dmabuf = info->fbops->fb_dmabuf_export(info);
IMO, it is not a good idea to delegate an implementation of
DMABUF ops to the driver. Maybe exporting could be handled
inside FB stack. As I understand, the FB stack needs only
'get-scatterlist' ops from an FB driver. All other
DMABUF magic does not need driver involvement, does it?
> + if (IS_ERR(dmabuf))
> + return PTR_ERR(dmabuf);
> +
> + return dma_buf_fd(dmabuf, flags);
> +}
> +#endif
> +
>
[snip]
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index ac3f1c6..c9fee75 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -701,6 +708,11 @@ struct fb_ops {
> /* called at KDB enter and leave time to prepare the console */
> int (*fb_debug_enter)(struct fb_info *info);
> int (*fb_debug_leave)(struct fb_info *info);
> +
> +#ifdef CONFIG_DMA_SHARED_BUFFER
> + /* Export the frame buffer as a dmabuf object */
> + struct dma_buf *(*fb_dmabuf_export)(struct fb_info *info);
> +#endif
Memory trashing or even kernel crash may happen if a module compiled
with CONFIG_DMA_SHARED_BUFFER enabled is loaded into kernel with
CONFIG_DMA_SHARED_BUFFER disabled.
> };
>
> #ifdef CONFIG_FB_TILEBLITTING
^ permalink raw reply
* Re: [Linaro-mm-sig] [RFC/PATCH] fb: Add dma-buf support
From: Jesse Barker @ 2012-06-20 15:09 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-fbdev, linaro-mm-sig, linux-media
In-Reply-To: <1340201368-20751-1-git-send-email-laurent.pinchart@ideasonboard.com>
Laurent,
Your recent documentation efforts are extremely commendable. Just a
couple of small nits below...
On Wed, Jun 20, 2012 at 7:09 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Add support for the dma-buf exporter role to the frame buffer API. The
> importer role isn't meaningful for frame buffer devices, as the frame
> buffer device model doesn't allow using externally allocated memory.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Documentation/fb/api.txt | 36 ++++++++++++++++++++++++++++++++++++
> drivers/video/fbmem.c | 36 ++++++++++++++++++++++++++++++++++++
> include/linux/fb.h | 12 ++++++++++++
> 3 files changed, 84 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/fb/api.txt b/Documentation/fb/api.txt
> index d4ff7de..f0b2173 100644
> --- a/Documentation/fb/api.txt
> +++ b/Documentation/fb/api.txt
> @@ -304,3 +304,39 @@ extensions.
> Upon successful format configuration, drivers update the fb_fix_screeninfo
> type, visual and line_length fields depending on the selected format. The type
> and visual fields are set to FB_TYPE_FOURCC and FB_VISUAL_FOURCC respectively.
> +
> +
> +5. DMA buffer sharing
> +---------------------
> +
> +The dma-buf kernel framework allows DMA buffers to be shared across devices
> +and applications. Sharing buffers across display devices and video capture or
> +video decoding devices allow zero-copy operation when displaying video content
> +produced by a hardware device such as a camera or a hardware codec. This is
> +crucial to achieve optimal system performances during video display.
> +
> +While dma-buf supports both exporting internally allocated memory as a dma-buf
> +object (known as the exporter role) and importing a dma-buf object to be used
> +as device memory (known as the importer role), the frame buffer API only
> +supports the exporter role, as the frame buffer device model doesn't support
> +using externally-allocated memory.
> +
> +The export a frame buffer as a dma-buf file descriptors, applications call the
s/The/To
s/descriptors/descriptor
cheers,
Jesse
> +FBIOGET_DMABUF ioctl. The ioctl takes a pointer to a fb_dmabuf_export
> +structure.
> +
> +struct fb_dmabuf_export {
> + __u32 fd;
> + __u32 flags;
> +};
> +
> +The flag field specifies the flags to be used when creating the dma-buf file
> +descriptor. The only supported flag is O_CLOEXEC. If the call is successful,
> +the driver will set the fd field to a file descriptor corresponding to the
> +dma-buf object.
> +
> +Applications can then pass the file descriptors to another application or
> +another device driver. The dma-buf object is automatically reference-counted,
> +applications can and should close the file descriptor as soon as they don't
> +need it anymore. The underlying dma-buf object will not be freed before the
> +last device that uses the dma-buf object releases it.
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 0dff12a..400e449 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -15,6 +15,7 @@
>
> #include <linux/compat.h>
> #include <linux/types.h>
> +#include <linux/dma-buf.h>
> #include <linux/errno.h>
> #include <linux/kernel.h>
> #include <linux/major.h>
> @@ -1074,6 +1075,23 @@ fb_blank(struct fb_info *info, int blank)
> return ret;
> }
>
> +#ifdef CONFIG_DMA_SHARED_BUFFER
> +int
> +fb_get_dmabuf(struct fb_info *info, int flags)
> +{
> + struct dma_buf *dmabuf;
> +
> + if (info->fbops->fb_dmabuf_export = NULL)
> + return -ENOTTY;
> +
> + dmabuf = info->fbops->fb_dmabuf_export(info);
> + if (IS_ERR(dmabuf))
> + return PTR_ERR(dmabuf);
> +
> + return dma_buf_fd(dmabuf, flags);
> +}
> +#endif
> +
> static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> unsigned long arg)
> {
> @@ -1084,6 +1102,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> struct fb_cmap cmap_from;
> struct fb_cmap_user cmap;
> struct fb_event event;
> + struct fb_dmabuf_export dmaexp;
> void __user *argp = (void __user *)arg;
> long ret = 0;
>
> @@ -1191,6 +1210,23 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> console_unlock();
> unlock_fb_info(info);
> break;
> +#ifdef CONFIG_DMA_SHARED_BUFFER
> + case FBIOGET_DMABUF:
> + if (copy_from_user(&dmaexp, argp, sizeof(dmaexp)))
> + return -EFAULT;
> +
> + if (!lock_fb_info(info))
> + return -ENODEV;
> + dmaexp.fd = fb_get_dmabuf(info, dmaexp.flags);
> + unlock_fb_info(info);
> +
> + if (dmaexp.fd < 0)
> + return dmaexp.fd;
> +
> + ret = copy_to_user(argp, &dmaexp, sizeof(dmaexp))
> + ? -EFAULT : 0;
> + break;
> +#endif
> default:
> if (!lock_fb_info(info))
> return -ENODEV;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index ac3f1c6..c9fee75 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -39,6 +39,7 @@
> #define FBIOPUT_MODEINFO 0x4617
> #define FBIOGET_DISPINFO 0x4618
> #define FBIO_WAITFORVSYNC _IOW('F', 0x20, __u32)
> +#define FBIOGET_DMABUF _IOR('F', 0x21, struct fb_dmabuf_export)
>
> #define FB_TYPE_PACKED_PIXELS 0 /* Packed Pixels */
> #define FB_TYPE_PLANES 1 /* Non interleaved planes */
> @@ -403,6 +404,11 @@ struct fb_cursor {
> #define FB_BACKLIGHT_MAX 0xFF
> #endif
>
> +struct fb_dmabuf_export {
> + __u32 fd;
> + __u32 flags;
> +};
> +
> #ifdef __KERNEL__
>
> #include <linux/fs.h>
> @@ -418,6 +424,7 @@ struct vm_area_struct;
> struct fb_info;
> struct device;
> struct file;
> +struct dma_buf;
>
> /* Definitions below are used in the parsed monitor specs */
> #define FB_DPMS_ACTIVE_OFF 1
> @@ -701,6 +708,11 @@ struct fb_ops {
> /* called at KDB enter and leave time to prepare the console */
> int (*fb_debug_enter)(struct fb_info *info);
> int (*fb_debug_leave)(struct fb_info *info);
> +
> +#ifdef CONFIG_DMA_SHARED_BUFFER
> + /* Export the frame buffer as a dmabuf object */
> + struct dma_buf *(*fb_dmabuf_export)(struct fb_info *info);
> +#endif
> };
>
> #ifdef CONFIG_FB_TILEBLITTING
> --
> Regards,
>
> Laurent Pinchart
>
>
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
^ 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