* Re: linux-next: build failure after merge of the final tree (fbdev tree related)
From: Michal Simek @ 2013-10-28 14:39 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Jean-Christophe PLAGNIOL-VILLARD, Florian Tobias Schandinat,
Tomi Valkeinen, linux-fbdev, linux-next, linux-kernel,
Michal Simek
In-Reply-To: <20131029012317.b9ba8173dee5bac1a5bd2404@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]
On 10/28/2013 03:23 PM, Stephen Rothwell wrote:
> Hi all,
>
> After merging the final tree, today's linux-next build (powerpc
> ppc44x_defconfig) failed like this:
>
> drivers/video/xilinxfb.c: In function 'xilinxfb_of_probe':
> drivers/video/xilinxfb.c:431:30: error: 'op' undeclared (first use in this function)
>
> Caused by commit 353846fb8bb7 ("video: xilinxfb: Use standard variable
> name convention") from the fbdev tree.
>
> I have applied the following fix patch for today:
>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Tue, 29 Oct 2013 01:18:22 +1100
> Subject: [PATCH] video: xilinxfb: Fix for "Use standard variable name convention"
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
> drivers/video/xilinxfb.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> index 9eedf9673b7f..6ff1a91e9dfd 100644
> --- a/drivers/video/xilinxfb.c
> +++ b/drivers/video/xilinxfb.c
> @@ -428,11 +428,11 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
> #ifdef CONFIG_PPC_DCR
> else {
> int start;
> - start = dcr_resource_start(op->dev.of_node, 0);
> - drvdata->dcr_len = dcr_resource_len(op->dev.of_node, 0);
> - drvdata->dcr_host = dcr_map(op->dev.of_node, start, drvdata->dcr_len);
> + start = dcr_resource_start(pdev->dev.of_node, 0);
> + drvdata->dcr_len = dcr_resource_len(pdev->dev.of_node, 0);
> + drvdata->dcr_host = dcr_map(pdev->dev.of_node, start, drvdata->dcr_len);
> if (!DCR_MAP_OK(drvdata->dcr_host)) {
> - dev_err(&op->dev, "invalid DCR address\n");
> + dev_err(&pdev->dev, "invalid DCR address\n");
> return -ENODEV;
> }
> }
>
Your fix is correct.
Tested-by: Michal Simek <monstr@monstr.eu>
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* linux-next: build failure after merge of the final tree (fbdev tree related)
From: Stephen Rothwell @ 2013-10-28 14:23 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD, Florian Tobias Schandinat,
Tomi Valkeinen, linux-fbdev
Cc: linux-next, linux-kernel, Michal Simek
In-Reply-To: <20110520163233.abcabd67.sfr@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
Hi all,
After merging the final tree, today's linux-next build (powerpc
ppc44x_defconfig) failed like this:
drivers/video/xilinxfb.c: In function 'xilinxfb_of_probe':
drivers/video/xilinxfb.c:431:30: error: 'op' undeclared (first use in this function)
Caused by commit 353846fb8bb7 ("video: xilinxfb: Use standard variable
name convention") from the fbdev tree.
I have applied the following fix patch for today:
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 29 Oct 2013 01:18:22 +1100
Subject: [PATCH] video: xilinxfb: Fix for "Use standard variable name convention"
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/video/xilinxfb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 9eedf9673b7f..6ff1a91e9dfd 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -428,11 +428,11 @@ static int xilinxfb_of_probe(struct platform_device *pdev)
#ifdef CONFIG_PPC_DCR
else {
int start;
- start = dcr_resource_start(op->dev.of_node, 0);
- drvdata->dcr_len = dcr_resource_len(op->dev.of_node, 0);
- drvdata->dcr_host = dcr_map(op->dev.of_node, start, drvdata->dcr_len);
+ start = dcr_resource_start(pdev->dev.of_node, 0);
+ drvdata->dcr_len = dcr_resource_len(pdev->dev.of_node, 0);
+ drvdata->dcr_host = dcr_map(pdev->dev.of_node, start, drvdata->dcr_len);
if (!DCR_MAP_OK(drvdata->dcr_host)) {
- dev_err(&op->dev, "invalid DCR address\n");
+ dev_err(&pdev->dev, "invalid DCR address\n");
return -ENODEV;
}
}
--
1.8.4.rc3
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Tomasz Figa @ 2013-10-28 12:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <526E0038.7050805@samsung.com>
Hi Donghwa,
On Monday 28 of October 2013 15:12:08 Donghwa Lee wrote:
> On Fri, OCT 25, 2013 06:57, Sylwester Nawrocki wrote:
> > On 10/24/2013 05:57 PM, Kishon Vijay Abraham I wrote:
> >> On Thursday 24 October 2013 09:12 PM, Sachin Kamat wrote:
> >>> On 24 October 2013 20:00, Olof Johansson<olof@lixom.net> wrote:
> >>>> On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I<kishon@ti.com> wrote:
> >>>>> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
> >>>>> index 32e5406..00b3a52 100644
> >>>>> --- a/drivers/video/exynos/exynos_mipi_dsi.c
> >>>>> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
> >>>>> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
> >>>>> exynos_mipi_regulator_enable(dsim);
> >>>>>
> >>>>> /* enable MIPI-DSI PHY. */
> >>>>> - if (dsim->pd->phy_enable)
> >>>>> - dsim->pd->phy_enable(pdev, true);
> >>>>> + phy_power_on(dsim->phy);
> >>>>>
> >>>>> clk_enable(dsim->clock);
> >>>>>
> >>>>
> >>>> This introduces the below with exynos_defconfig:
> >>>>
> >>>> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
> >>>> 'exynos_mipi_dsi_blank_mode':
> >>>> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
> >>>> variable 'pdev' [-Wunused-variable]
> >>>> struct platform_device *pdev = to_platform_device(dsim->dev);
> >
> > Sorry about missing that, I only noticed this warning recently and didn't
> > get around to submit a patch.
> >
> >>> I have already submitted a patch to fix this [1]
> >
> > Thanks a lot guys for fixing this.
> >
> >>> [1] http://marc.info/?l=linux-fbdev&m\x138233359617936&w=2
> >>
> >> Sorry, missed that :-(
> >
> > This MIPI DSIM driver is affectively a dead code in the mainline now, once
> > Exynos become a dt-only platform. I guess it can be deleted for 3.14, once
> > S5P gets converted to the device tree. The new driver using CDF is basically
> > a complete rewrite. Or device tree support should be added to that driver,
> > but I believe it doesn't make sense without CDF.
> >
>
> MIPI DSIM driver is not a dead code. There is a steady trickle of patches.
> It's kind of late, but, I will update it as DT based drivers as soon as possible.
> And Why do you think that DT support of existing MIPI DSIM is something less
> than great?
First of all, the exynos_mipi_dsim driver has currently no users in
mainline kernel, so it is essentially dead code. In addition, on
a platform that is the primary candidate for using it, which is Exynos,
there is no way to use it, due to no DT support.
As for the driver itself, it is not really a great example of good code.
It contains a hacks, like calling msleep() without any clear reason and
also many coding style issues. I'd prefer to replace it with the new
exynos-dsi driver rewritten completely in SRPOL, when CDF is finished.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Donghwa Lee @ 2013-10-28 6:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <526997BC.8070602@gmail.com>
On Fri, OCT 25, 2013 06:57, Sylwester Nawrocki wrote:
> On 10/24/2013 05:57 PM, Kishon Vijay Abraham I wrote:
>> On Thursday 24 October 2013 09:12 PM, Sachin Kamat wrote:
>>> On 24 October 2013 20:00, Olof Johansson<olof@lixom.net> wrote:
>>>> On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I<kishon@ti.com> wrote:
>>>>> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
>>>>> index 32e5406..00b3a52 100644
>>>>> --- a/drivers/video/exynos/exynos_mipi_dsi.c
>>>>> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
>>>>> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
>>>>> exynos_mipi_regulator_enable(dsim);
>>>>>
>>>>> /* enable MIPI-DSI PHY. */
>>>>> - if (dsim->pd->phy_enable)
>>>>> - dsim->pd->phy_enable(pdev, true);
>>>>> + phy_power_on(dsim->phy);
>>>>>
>>>>> clk_enable(dsim->clock);
>>>>>
>>>>
>>>> This introduces the below with exynos_defconfig:
>>>>
>>>> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
>>>> 'exynos_mipi_dsi_blank_mode':
>>>> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
>>>> variable 'pdev' [-Wunused-variable]
>>>> struct platform_device *pdev = to_platform_device(dsim->dev);
>
> Sorry about missing that, I only noticed this warning recently and didn't
> get around to submit a patch.
>
>>> I have already submitted a patch to fix this [1]
>
> Thanks a lot guys for fixing this.
>
>>> [1] http://marc.info/?l=linux-fbdev&m\x138233359617936&w=2
>>
>> Sorry, missed that :-(
>
> This MIPI DSIM driver is affectively a dead code in the mainline now, once
> Exynos become a dt-only platform. I guess it can be deleted for 3.14, once
> S5P gets converted to the device tree. The new driver using CDF is basically
> a complete rewrite. Or device tree support should be added to that driver,
> but I believe it doesn't make sense without CDF.
>
MIPI DSIM driver is not a dead code. There is a steady trickle of patches.
It's kind of late, but, I will update it as DT based drivers as soon as possible.
And Why do you think that DT support of existing MIPI DSIM is something less
than great?
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" 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
* [RFC PATCH RESEND] fb: reorder the lock sequence to fix a potential lockdep
From: Gu Zheng @ 2013-10-28 6:01 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD, Tomi Valkeinen
Cc: Linux Fbdev development list, linux-kernel
Following commits:
50e244cc79 fb: rework locking to fix lock ordering on takeover
e93a9a8687 fb: Yet another band-aid for fixing lockdep mess
054430e773 fbcon: fix locking harder
reworked locking to fix related lock ordering on takeover, and introduced console_lock
into fbmem, but it seems that the new lock sequence(fb_info->lock ---> console_lock)
is against with the one in console_callback(console_lock ---> fb_info->lock), and leads to
a potential deadlock as following:
[ 601.079000] ===========================
[ 601.079000] [ INFO: possible circular locking dependency detected ]
[ 601.079000] 3.11.0 #189 Not tainted
[ 601.079000] -------------------------------------------------------
[ 601.079000] kworker/0:3/619 is trying to acquire lock:
[ 601.079000] (&fb_info->lock){+.+.+.}, at: [<ffffffff81397566>] lock_fb_info+0x26/0x60
[ 601.079000]
but task is already holding lock:
[ 601.079000] (console_lock){+.+.+.}, at: [<ffffffff8141aae3>] console_callback+0x13/0x160
[ 601.079000]
which lock already depends on the new lock.
[ 601.079000]
the existing dependency chain (in reverse order) is:
[ 601.079000]
-> #1 (console_lock){+.+.+.}:
[ 601.079000] [<ffffffff810dc971>] lock_acquire+0xa1/0x140
[ 601.079000] [<ffffffff810c6267>] console_lock+0x77/0x80
[ 601.079000] [<ffffffff81399448>] register_framebuffer+0x1d8/0x320
[ 601.079000] [<ffffffff81cfb4c8>] efifb_probe+0x408/0x48f
[ 601.079000] [<ffffffff8144a963>] platform_drv_probe+0x43/0x80
[ 601.079000] [<ffffffff8144853b>] driver_probe_device+0x8b/0x390
[ 601.079000] [<ffffffff814488eb>] __driver_attach+0xab/0xb0
[ 601.079000] [<ffffffff814463bd>] bus_for_each_dev+0x5d/0xa0
[ 601.079000] [<ffffffff81447e6e>] driver_attach+0x1e/0x20
[ 601.079000] [<ffffffff81447a07>] bus_add_driver+0x117/0x290
[ 601.079000] [<ffffffff81448fea>] driver_register+0x7a/0x170
[ 601.079000] [<ffffffff8144a10a>] __platform_driver_register+0x4a/0x50
[ 601.079000] [<ffffffff8144a12d>] platform_driver_probe+0x1d/0xb0
[ 601.079000] [<ffffffff81cfb0a1>] efifb_init+0x273/0x292
[ 601.079000] [<ffffffff81002132>] do_one_initcall+0x102/0x1c0
[ 601.079000] [<ffffffff81cb80a6>] kernel_init_freeable+0x15d/0x1ef
[ 601.079000] [<ffffffff8166d2de>] kernel_init+0xe/0xf0
[ 601.079000] [<ffffffff816914ec>] ret_from_fork+0x7c/0xb0
[ 601.079000]
-> #0 (&fb_info->lock){+.+.+.}:
[ 601.079000] [<ffffffff810dc1d8>] __lock_acquire+0x1e18/0x1f10
[ 601.079000] [<ffffffff810dc971>] lock_acquire+0xa1/0x140
[ 601.079000] [<ffffffff816835ca>] mutex_lock_nested+0x7a/0x3b0
[ 601.079000] [<ffffffff81397566>] lock_fb_info+0x26/0x60
[ 601.079000] [<ffffffff813a4aeb>] fbcon_blank+0x29b/0x2e0
[ 601.079000] [<ffffffff81418658>] do_blank_screen+0x1d8/0x280
[ 601.079000] [<ffffffff8141ab34>] console_callback+0x64/0x160
[ 601.079000] [<ffffffff8108d855>] process_one_work+0x1f5/0x540
[ 601.079000] [<ffffffff8108e04c>] worker_thread+0x11c/0x370
[ 601.079000] [<ffffffff81095fbd>] kthread+0xed/0x100
[ 601.079000] [<ffffffff816914ec>] ret_from_fork+0x7c/0xb0
[ 601.079000]
other info that might help us debug this:
[ 601.079000] Possible unsafe locking scenario:
[ 601.079000] CPU0 CPU1
[ 601.079000] ---- ----
[ 601.079000] lock(console_lock);
[ 601.079000] lock(&fb_info->lock);
[ 601.079000] lock(console_lock);
[ 601.079000] lock(&fb_info->lock);
[ 601.079000]
*** DEADLOCK ***
so we reorder the lock sequence the same as it in console_callback() to
avoid this issue.
Not very sure this change is suitable, any comments is welcome.
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
drivers/video/fbmem.c | 50 +++++++++++++++++++++++++++++++-----------------
1 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index dacaf74..010d191 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1108,14 +1108,16 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
case FBIOPUT_VSCREENINFO:
if (copy_from_user(&var, argp, sizeof(var)))
return -EFAULT;
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
info->flags |= FBINFO_MISC_USEREVENT;
ret = fb_set_var(info, &var);
info->flags &= ~FBINFO_MISC_USEREVENT;
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
if (!ret && copy_to_user(argp, &var, sizeof(var)))
ret = -EFAULT;
break;
@@ -1144,12 +1146,14 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
case FBIOPAN_DISPLAY:
if (copy_from_user(&var, argp, sizeof(var)))
return -EFAULT;
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
ret = fb_pan_display(info, &var);
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
if (ret = 0 && copy_to_user(argp, &var, sizeof(var)))
return -EFAULT;
break;
@@ -1184,23 +1188,27 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
break;
}
event.data = &con2fb;
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
event.info = info;
ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP, &event);
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
break;
case FBIOBLANK:
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
info->flags |= FBINFO_MISC_USEREVENT;
ret = fb_blank(info, arg);
info->flags &= ~FBINFO_MISC_USEREVENT;
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
break;
default:
if (!lock_fb_info(info))
@@ -1660,12 +1668,15 @@ static int do_register_framebuffer(struct fb_info *fb_info)
registered_fb[i] = fb_info;
event.info = fb_info;
- if (!lock_fb_info(fb_info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(fb_info)) {
+ console_unlock();
+ return -ENODEV;
+ }
+
fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
- console_unlock();
unlock_fb_info(fb_info);
+ console_unlock();
return 0;
}
@@ -1678,13 +1689,16 @@ static int do_unregister_framebuffer(struct fb_info *fb_info)
if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
return -EINVAL;
- if (!lock_fb_info(fb_info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(fb_info)) {
+ console_unlock();
+ return -ENODEV;
+ }
+
event.info = fb_info;
ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
- console_unlock();
unlock_fb_info(fb_info);
+ console_unlock();
if (ret)
return -EINVAL;
--
1.7.7
^ permalink raw reply related
* Re: [PATCH 1/1] backlight: hx8357: Remove redundant of_match_ptr
From: Jingoo Han @ 2013-10-27 23:08 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1382702373-19573-1-git-send-email-sachin.kamat@linaro.org>
On Sunday, October 27, 2013 11:22 PM, Maxime Ripard wrote:
> On Fri, Oct 25, 2013 at 05:29:33PM +0530, Sachin Kamat wrote:
> > 'hx8357_dt_ids' is always compiled in. Hence of_match_ptr is
> > not needed.
> >
> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Best regards,
Jingoo Han
^ permalink raw reply
* Re: [PATCH 1/1] backlight: hx8357: Remove redundant of_match_ptr
From: Maxime Ripard @ 2013-10-27 14:21 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1382702373-19573-1-git-send-email-sachin.kamat@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 467 bytes --]
Hi Sachin,
On Fri, Oct 25, 2013 at 05:29:33PM +0530, Sachin Kamat wrote:
> 'hx8357_dt_ids' is always compiled in. Hence of_match_ptr is
> not needed.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] drivers: video: metronomefb: avoid out-of-bounds array access
From: Michal Nazarewicz @ 2013-10-26 14:58 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: linux-fbdev, linux-kernel
From: Michal Nazarewicz <mina86@mina86.com>
load_waveform function checks whether padding bytes in stuff2a
and stuff2b are all zero, but does so by treating those arrays
as a single longer array. Since the structure is packed, and
the size sum matches, it all works, but creates some confusion
in the code.
This commit changes the stuff2a and stuff2b arrays into pad1 and
pad2 fields such that they cover the same bytes as the arrays
covered, and changes the check in the load_waveform function so
that the fields are read instead of iterating over an arary.
It also renames the other “stuff” fields to “ignore*” fields to
give them more semantic meaning.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/video/metronomefb.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/video/metronomefb.c b/drivers/video/metronomefb.c
index f30150d..44ef8b0 100644
--- a/drivers/video/metronomefb.c
+++ b/drivers/video/metronomefb.c
@@ -126,7 +126,7 @@ static struct fb_var_screeninfo metronomefb_var = {
/* the waveform structure that is coming from userspace firmware */
struct waveform_hdr {
- u8 stuff[32];
+ u8 ignore1[32];
u8 wmta[3];
u8 fvsn;
@@ -134,13 +134,14 @@ struct waveform_hdr {
u8 luts;
u8 mc;
u8 trc;
- u8 stuff3;
+ u8 ignore2;
u8 endb;
u8 swtb;
- u8 stuff2a[2];
+ u32 pad1; /* u16 halfof(pad1) */
- u8 stuff2b[3];
+ /* u16 halfof(pad1) */
+ u8 pad2;
u8 wfm_cs;
} __attribute__ ((packed));
@@ -210,11 +211,9 @@ static int load_waveform(u8 *mem, size_t size, int m, int t,
}
wfm_hdr->mc += 1;
wfm_hdr->trc += 1;
- for (i = 0; i < 5; i++) {
- if (*(wfm_hdr->stuff2a + i) != 0) {
- dev_err(dev, "Error: unexpected value in padding\n");
- return -EINVAL;
- }
+ if (wfm_hdr->pad1 || wfm_hdr->pad2) {
+ dev_err(dev, "Error: unexpected value in padding\n");
+ return -EINVAL;
}
/* calculating trn. trn is something used to index into
--
1.8.4
^ permalink raw reply related
* my subject
From: Mr. X @ 2013-10-26 8:51 UTC (permalink / raw)
To: linux-fbdev
Need a Loan, Loans from $5000 - $10,000,000 00. Get your no obligation FREE quote now! Repayments up to 54 Months. No Collateral, Money paid into your account within 24 hours after approval. For more Info contact us today guaranteeloancompany1@gmail.com
^ permalink raw reply
* Re: [PATCHv3][ 3/5] video: mx3fb: Add device tree suport.
From: Kumar Gala @ 2013-10-26 6:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20131026001854.GE17135@pengutronix.de>
On Oct 25, 2013, at 7:18 PM, Sascha Hauer wrote:
> On Fri, Oct 25, 2013 at 08:50:40PM +0100, Grant Likely wrote:
>> On Wed, 23 Oct 2013 14:43:47 +0200, Denis Carikli <denis@eukrea.com> wrote:
>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: linux-fbdev@vger.kernel.org
>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>> Cc: Pawel Moll <pawel.moll@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: Eric Bénard <eric@eukrea.com>
>>> Signed-off-by: Denis Carikli <denis@eukrea.com>
>>> ---
>>> ChangeLog v2->v3:
>>> - The device tree bindings were reworked in order to make it look more like the
>>> IPUv3 bindings.
>>> - The interface_pix_fmt property now looks like the IPUv3 one.
>>> ---
>>> .../devicetree/bindings/video/fsl,mx3-fb.txt | 35 ++++++
>>> drivers/video/Kconfig | 2 +
>>> drivers/video/mx3fb.c | 125 +++++++++++++++++---
>>> 3 files changed, 147 insertions(+), 15 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
>>> new file mode 100644
>>> index 0000000..0b31374
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
>>> @@ -0,0 +1,35 @@
>>> +Freescale MX3 fb
>>> +========
>>> +
>>> +Required properties:
>>> +- compatible: Should be "fsl,mx3fb". compatible chips include the imx31 and the
>>> + imx35.
>>> +- reg: should be register base and length as documented in the datasheet.
>>> +- clocks: Handle to the ipu_gate clock.
>>> +
>>> +Example:
>>> +
>>> +lcdc: mx3fb@53fc00b4 {
>>> + compatible = "fsl,mx3-fb";
>>> + reg = <0x53fc00b4 0x0b>;
>>> + clocks = <&clks 55>;
>>> +};
>>
>> This (and some of the other bindings) are trivial, and they are all
>> associated with a single SoC. I think it would be better to collect all
>> the mx3 bindings into a single file rather than distributing them all
>> over the bindings tree.
>>
>> I started thinking about this after some of the DT conversations in
>> Edinburgh this week. Unless there is a high likelyhood of components
>> being used separately, I think it is far more useful to collect all the
>> bindings for an SoC into a single file. It will certainly reduce a lot
>> of the boilerplate that we've been collecting in bindings documentation
>> files.
>>
>> A long time ago I took that approach for the mpc5200 documentation[1].
>> Take a look at that organization and let me know what you think.
>
> I don't think this is a good idea. When a new SoC comes out we don't
> know which components will be reused on the next SoC. This will cause a
> lot of bikeshedding when it actually is reused and then has to be moved.
>
> Also I would find it quite inconsistent if I had to lookup some devices
> in a SoC file and most bindings in subsystem specific files. So when
> searching for a binding I would first have to know if the hardware is
> unique to the SoC or not.
I agree that as new SoC come out its better to have these things split out. Also for IP that is sold by vendors like Synopsys/Designware that get used on a lot of SoCs its makes it easier to ensure consistency by having the binding split out for the IP and not the SoC.
Also, by having bindings for similar devices for different SoCs kept together it becomes easier for us to see patterns across SoCs as we might come up with a more generic binding in the future. Far more difficult if things are SoC oriented.
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCHv3][ 3/5] video: mx3fb: Add device tree suport.
From: Kumar Gala @ 2013-10-26 6:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382532229-32755-3-git-send-email-denis@eukrea.com>
On Oct 23, 2013, at 7:43 AM, Denis Carikli wrote:
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: devicetree@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v2->v3:
> - The device tree bindings were reworked in order to make it look more like the
> IPUv3 bindings.
> - The interface_pix_fmt property now looks like the IPUv3 one.
> ---
> .../devicetree/bindings/video/fsl,mx3-fb.txt | 35 ++++++
> drivers/video/Kconfig | 2 +
> drivers/video/mx3fb.c | 125 +++++++++++++++++---
> 3 files changed, 147 insertions(+), 15 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
>
> diff --git a/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> new file mode 100644
> index 0000000..0b31374
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> @@ -0,0 +1,35 @@
> +Freescale MX3 fb
Can you spell out framebuffer here instead of fb
> +========
> +
> +Required properties:
> +- compatible: Should be "fsl,mx3fb". compatible chips include the imx31 and the
> + imx35.
> +- reg: should be register base and length as documented in the datasheet.
> +- clocks: Handle to the ipu_gate clock.
> +
> +Example:
> +
> +lcdc: mx3fb@53fc00b4 {
> + compatible = "fsl,mx3-fb";
> + reg = <0x53fc00b4 0x0b>;
> + clocks = <&clks 55>;
> +};
> +
> +Display support
> +=======> +Required properties:
> +- model : The user-visible name of the display.
> +
> +Optional properties:
> +- interface_pix_fmt: How this display is connected to the
> + crtc. Currently supported types: "rgb24", "rgb565", "rgb666"
Why is there no compatible for the display?
> +
> +It can also have an optional timing subnode as described in
> + Documentation/devicetree/bindings/video/display-timing.txt.
> +
> +Example:
> +
> +display@di0 {
> + interface-pix-fmt = "rgb666";
> + model = "CMO-QVGA";
> +};
how do you relate the display to the framebuffer?
- k
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 14317b7..2a638df 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2359,6 +2359,8 @@ config FB_MX3
> select FB_CFB_FILLRECT
> select FB_CFB_COPYAREA
> select FB_CFB_IMAGEBLIT
> + select VIDEOMODE_HELPERS
> + select FB_MODE_HELPERS
> default y
> help
> This is a framebuffer device for the i.MX31 LCD Controller. So
> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> index 804f874..de5a6c8 100644
> --- a/drivers/video/mx3fb.c
> +++ b/drivers/video/mx3fb.c
> @@ -31,6 +31,8 @@
> #include <linux/platform_data/dma-imx.h>
> #include <linux/platform_data/video-mx3fb.h>
>
> +#include <video/of_display_timing.h>
> +
> #include <asm/io.h>
> #include <asm/uaccess.h>
>
> @@ -757,11 +759,13 @@ static int __set_par(struct fb_info *fbi, bool lock)
> sig_cfg.Hsync_pol = true;
> if (fbi->var.sync & FB_SYNC_VERT_HIGH_ACT)
> sig_cfg.Vsync_pol = true;
> - if (fbi->var.sync & FB_SYNC_CLK_INVERT)
> + if ((fbi->var.sync & FB_SYNC_CLK_INVERT) ||
> + (fbi->var.sync & FB_SYNC_PIXDAT_HIGH_ACT))
> sig_cfg.clk_pol = true;
> if (fbi->var.sync & FB_SYNC_DATA_INVERT)
> sig_cfg.data_pol = true;
> - if (fbi->var.sync & FB_SYNC_OE_ACT_HIGH)
> + if ((fbi->var.sync & FB_SYNC_OE_ACT_HIGH) ||
> + (fbi->var.sync & FB_SYNC_DE_HIGH_ACT))
> sig_cfg.enable_pol = true;
> if (fbi->var.sync & FB_SYNC_CLK_IDLE_EN)
> sig_cfg.clkidle_en = true;
> @@ -1351,21 +1355,75 @@ static struct fb_info *mx3fb_init_fbinfo(struct device *dev, struct fb_ops *ops)
> return fbi;
> }
>
> +static int match_dt_disp_data(const char *property)
> +{
> + if (!strcmp("rgb666", property))
> + return IPU_DISP_DATA_MAPPING_RGB666;
> + else if (!strcmp("rgb565", property))
> + return IPU_DISP_DATA_MAPPING_RGB565;
> + else if (!strcmp("rgb24", property))
> + return IPU_DISP_DATA_MAPPING_RGB888;
> + else
> + return -EINVAL;
> +}
> +
> static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
> {
> struct device *dev = mx3fb->dev;
> struct mx3fb_platform_data *mx3fb_pdata = dev_get_platdata(dev);
> - const char *name = mx3fb_pdata->name;
> + struct device_node *np = dev->of_node;
> + const char *name;
> + const char *ipu_disp_format;
> unsigned int irq;
> struct fb_info *fbi;
> struct mx3fb_info *mx3fbi;
> const struct fb_videomode *mode;
> int ret, num_modes;
> + struct fb_videomode of_mode;
> + struct device_node *display_np, *root_np;
> +
> + if (np) {
> + root_np = of_find_node_by_path("/");
> + if (!root_np) {
> + dev_err(dev, "Can't get the \"/\" node.\n");
> + return -EINVAL;
> + }
> +
> + display_np = of_find_node_by_name(root_np, "display");
> + if (!display_np) {
> + dev_err(dev, "Can't get the display device node.\n");
> + return -EINVAL;
> + }
> +
> + of_property_read_string(display_np, "interface-pix-fmt",
> + &ipu_disp_format);
> + if (!ipu_disp_format) {
> + mx3fb->disp_data_fmt = IPU_DISP_DATA_MAPPING_RGB666;
> + dev_warn(dev,
> + "ipu display data mapping was not defined, using the default rgb666.\n");
> + } else {
> + mx3fb->disp_data_fmt > + match_dt_disp_data(ipu_disp_format);
> + }
> +
> + if (mx3fb->disp_data_fmt = -EINVAL) {
> + dev_err(dev, "Illegal display data format \"%s\"\n",
> + ipu_disp_format);
> + return -EINVAL;
> + }
>
> - if (mx3fb_pdata->disp_data_fmt >= ARRAY_SIZE(di_mappings)) {
> - dev_err(dev, "Illegal display data format %d\n",
> + of_property_read_string(display_np, "model", &name);
> + if (ret) {
> + dev_err(dev, "Missing display model name\n");
> + return -EINVAL;
> + }
> + } else {
> + name = mx3fb_pdata->name;
> + if (mx3fb_pdata->disp_data_fmt >= ARRAY_SIZE(di_mappings)) {
> + dev_err(dev, "Illegal display data format %d\n",
> mx3fb_pdata->disp_data_fmt);
> - return -EINVAL;
> + return -EINVAL;
> + }
> }
>
> ichan->client = mx3fb;
> @@ -1386,12 +1444,24 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
> goto emode;
> }
>
> - if (mx3fb_pdata->mode && mx3fb_pdata->num_modes) {
> - mode = mx3fb_pdata->mode;
> - num_modes = mx3fb_pdata->num_modes;
> + if (np) {
> + ret = of_get_fb_videomode(display_np, &of_mode,
> + OF_USE_NATIVE_MODE);
> + if (!ret) {
> + mode = &of_mode;
> + num_modes = 1;
> + } else {
> + mode = mx3fb_modedb;
> + num_modes = ARRAY_SIZE(mx3fb_modedb);
> + }
> } else {
> - mode = mx3fb_modedb;
> - num_modes = ARRAY_SIZE(mx3fb_modedb);
> + if (mx3fb_pdata->mode || !mx3fb_pdata->num_modes) {
> + mode = mx3fb_pdata->mode;
> + num_modes = mx3fb_pdata->num_modes;
> + } else {
> + mode = mx3fb_modedb;
> + num_modes = ARRAY_SIZE(mx3fb_modedb);
> + }
> }
>
> if (!fb_find_mode(&fbi->var, fbi, fb_mode, mode,
> @@ -1421,7 +1491,8 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
> mx3fbi->mx3fb = mx3fb;
> mx3fbi->blank = FB_BLANK_NORMAL;
>
> - mx3fb->disp_data_fmt = mx3fb_pdata->disp_data_fmt;
> + if (!np)
> + mx3fb->disp_data_fmt = mx3fb_pdata->disp_data_fmt;
>
> init_completion(&mx3fbi->flip_cmpl);
> disable_irq(ichan->eof_irq);
> @@ -1449,13 +1520,26 @@ emode:
> return ret;
> }
>
> +static int imx_dma_is_dt_ipu(struct dma_chan *chan)
> +{
> + /* Example: 53fc0000.ipu */
> + if (chan && chan->device && chan->device->dev) {
> + char *name = dev_name(chan->device->dev);
> + name = strchr(name, '.');
> + if (name)
> + return !strcmp(name, ".ipu");
> + }
> +
> + return 0;
> +}
> +
> static bool chan_filter(struct dma_chan *chan, void *arg)
> {
> struct dma_chan_request *rq = arg;
> struct device *dev;
> struct mx3fb_platform_data *mx3fb_pdata;
>
> - if (!imx_dma_is_ipu(chan))
> + if (!imx_dma_is_ipu(chan) && !imx_dma_is_dt_ipu(chan))
> return false;
>
> if (!rq)
> @@ -1464,8 +1548,12 @@ static bool chan_filter(struct dma_chan *chan, void *arg)
> dev = rq->mx3fb->dev;
> mx3fb_pdata = dev_get_platdata(dev);
>
> - return rq->id = chan->chan_id &&
> - mx3fb_pdata->dma_dev = chan->device->dev;
> + /* When using the devicetree, mx3fb_pdata is NULL */
> + if (imx_dma_is_dt_ipu(chan))
> + return rq->id = chan->chan_id;
> + else
> + return rq->id = chan->chan_id &&
> + mx3fb_pdata->dma_dev = chan->device->dev;
> }
>
> static void release_fbi(struct fb_info *fbi)
> @@ -1565,9 +1653,16 @@ static int mx3fb_remove(struct platform_device *dev)
> return 0;
> }
>
> +static struct of_device_id mx3fb_of_dev_id[] = {
> + { .compatible = "fsl,mx3-fb", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mx3fb_of_dev_id);
> +
> static struct platform_driver mx3fb_driver = {
> .driver = {
> .name = MX3FB_NAME,
> + .of_match_table = mx3fb_of_dev_id,
> .owner = THIS_MODULE,
> },
> .probe = mx3fb_probe,
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCHv3][ 3/5] video: mx3fb: Add device tree suport.
From: Sascha Hauer @ 2013-10-26 0:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20131025195040.0CCC3C404DA@trevor.secretlab.ca>
On Fri, Oct 25, 2013 at 08:50:40PM +0100, Grant Likely wrote:
> On Wed, 23 Oct 2013 14:43:47 +0200, Denis Carikli <denis@eukrea.com> wrote:
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: linux-fbdev@vger.kernel.org
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: devicetree@vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: Eric Bénard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > ---
> > ChangeLog v2->v3:
> > - The device tree bindings were reworked in order to make it look more like the
> > IPUv3 bindings.
> > - The interface_pix_fmt property now looks like the IPUv3 one.
> > ---
> > .../devicetree/bindings/video/fsl,mx3-fb.txt | 35 ++++++
> > drivers/video/Kconfig | 2 +
> > drivers/video/mx3fb.c | 125 +++++++++++++++++---
> > 3 files changed, 147 insertions(+), 15 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> >
> > diff --git a/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> > new file mode 100644
> > index 0000000..0b31374
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
> > @@ -0,0 +1,35 @@
> > +Freescale MX3 fb
> > +========
> > +
> > +Required properties:
> > +- compatible: Should be "fsl,mx3fb". compatible chips include the imx31 and the
> > + imx35.
> > +- reg: should be register base and length as documented in the datasheet.
> > +- clocks: Handle to the ipu_gate clock.
> > +
> > +Example:
> > +
> > +lcdc: mx3fb@53fc00b4 {
> > + compatible = "fsl,mx3-fb";
> > + reg = <0x53fc00b4 0x0b>;
> > + clocks = <&clks 55>;
> > +};
>
> This (and some of the other bindings) are trivial, and they are all
> associated with a single SoC. I think it would be better to collect all
> the mx3 bindings into a single file rather than distributing them all
> over the bindings tree.
>
> I started thinking about this after some of the DT conversations in
> Edinburgh this week. Unless there is a high likelyhood of components
> being used separately, I think it is far more useful to collect all the
> bindings for an SoC into a single file. It will certainly reduce a lot
> of the boilerplate that we've been collecting in bindings documentation
> files.
>
> A long time ago I took that approach for the mpc5200 documentation[1].
> Take a look at that organization and let me know what you think.
I don't think this is a good idea. When a new SoC comes out we don't
know which components will be reused on the next SoC. This will cause a
lot of bikeshedding when it actually is reused and then has to be moved.
Also I would find it quite inconsistent if I had to lookup some devices
in a SoC file and most bindings in subsystem specific files. So when
searching for a binding I would first have to know if the hardware is
unique to the SoC or not.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Stephen Warren @ 2013-10-25 15:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <1580941.z2CDo8WmRF@avalon>
On 10/25/2013 12:33 PM, Laurent Pinchart wrote:
> On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
>> On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
...
>>> The case I'm mostly concerned about would be two different compatibility
>>> strings to select whether the device should be handled by a KMS or V4L
>>> driver. I don't think that's a good idea.
>>
>> I wouldn't think of the two compatible values as selecting different
>> specific Linux drivers, but rather they simply describe the HW in
>> different levels of detail. The fact that if we know a certain level of
>> detail about the HW means that Linux can and does create a KMS driver
>> rather than a V4L2 driver seems like a detail that's completely hidden
>> inside the OS.
>
> I expect the same level of details to be needed on both the KMS and V4L sides.
> Taking the example of the ADV7511 HDMI transmitter, the only change in the DT
> bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l"
> and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and
> -kms to different names wouldn't fundamentally change the problem.
Yes, two compatible values such as "adi,adv7511-v4l" and
"adi,adv7511-kms" sounds like a bad idea.
Rather, shouldn't we have just "adi,adv7511", and the driver for that
should register itself in a way that allows either/both the top-level
DRM or top-level V4L drivers to find it, and make use of its services.
There are plenty of drivers in the kernel already that export services
through two different subsystems, e.g. a GPIO HW module that registers
as both a struct gpio_chip and a struct irq_chip, or both a struct
gpio_chip and a pinctrl device.
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 14:22 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131025141029.GD1551-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6360 bytes --]
Hi Thierry,
On Friday 25 October 2013 16:10:30 Thierry Reding wrote:
> On Fri, Oct 25, 2013 at 03:47:21PM +0200, Laurent Pinchart wrote:
> > On Friday 25 October 2013 10:13:15 Thierry Reding wrote:
> > > On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote:
> > > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > > > >> ...
> > > > >>
> > > > >>>> As I said, anything that really needs a CDF binding to work
> > > > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > > > >>>> easily be justified.
> > > > >>>
> > > > >>> The system as a whole would be more complex, but the panel could
> > > > >>> be the same. We can't have two drivers for the same piece of
> > > > >>> hardware in the DT world, as there will be a single compatible
> > > > >>> string and no way to choose between the drivers (unlike the board
> > > > >>> code world that could set device names to "foo- encoder-v4l2" or
> > > > >>> "foo-encoder-drm" and live happily with that ever after).
> > > > >>
> > > > >> That's not true. We can certainly define two different compatible
> > > > >> values for a piece of HW if we have to. We can easily control
> > > > >> whether they are handled by the same or different drivers in the
> > > > >> OS.
> > > > >
> > > > > From an implementation point of view, sure. But from a conceptual
> > > > > point of view, that would make the DT bindings pretty Linux-
> > > > > specific, with a description of what the operating system should do
> > > > > instead of a description of what the hardware looks like. My
> > > > > understanding is that we've tried pretty hard in the past not to
> > > > > open that Pandora's box.
> > > > >
> > > > > The case I'm mostly concerned about would be two different
> > > > > compatibility strings to select whether the device should be handled
> > > > > by a KMS or V4L driver. I don't think that's a good idea.
> > > >
> > > > I wouldn't think of the two compatible values as selecting different
> > > > specific Linux drivers, but rather they simply describe the HW in
> > > > different levels of detail. The fact that if we know a certain level
> > > > of detail about the HW means that Linux can and does create a KMS
> > > > driver rather than a V4L2 driver seems like a detail that's completely
> > > > hidden inside the OS.
> > >
> > > I've had a somewhat similar idea the other day but couldn't really put
> > > it into words. Interestingly someone else mentioned a similar concept in
> > > a different thread which I think describes what I had in mind as well.
> > >
> > > I was wondering if we couldn't use two compatible values to denote two
> > >
> > > interfaces that the device implements. Something along the lines of:
> > > compatible = "vendor,block-name", "encoder";
> > >
> > > So a driver could primarily match on "vendor,block-name", but at the
> > > same time it could use the additional information of being required to
> > > implement "encoder" to expose an additonal interface.
> >
> > Let's take the hardware architecture described in
> > http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39)
> > as an example. The green blocks are part of the capture pipeline and
> > handled by the V4L subsystem. The blue blocks are part of the display
> > pipeline and handled by the KMS subsystem. One ADV7511 HDMI transmitter
> > instance need to be controlled by a KMS driver and the second one by a
> > V4L driver.
> >
> > The two instances are identical, so their DT nodes will show no difference
> > if we stick to a hardware description only. There would then be no way to
> > bind to different drivers.
>
> I don't quite see why some of the green parts couldn't be part of the
> display (KMS) pipeline.
Because there might be no blue pipeline in the device, just the green one. The
green pipeline is a video capture pipeline and happens to have an HDMI output
with a deep pipeline only, with no frame buffer. We can't create a KMS driver
for that, as there's no frame buffer and no CRTC.
> I thought I remember you say something about implementing deep pipelining
> support in one of the more recent talks. This sounds exactly like a case
> where this would be useful.
>
> Obviously as long as that work isn't finished that won't help you, but I
> think that providing a way to pass a video stream object from V4L2 to
> DRM/KMS would be a more proper solution to this problem.
We need that as well, but it won't solve the problem when no KMS device is
present.
> > > I suppose that perhaps something like a device_type property could be
> > > used for that as well, and that might even be the more correct thing to
> > > do.
> > >
> > > We already do something similar to make GPIO controllers expose an
> > > interrupt chip by adding an interrupt-controller property. We also use
> > > the gpio-controller property to mark a device node as exposing the GPIO
> > > interface for that matter.
> > >
> > > So if a HW block can actually implement two different interfaces, each
> > > of them being optional, then there should be ways to represent that in
> > > DT as well. We already do that for "simpler" HW blocks, so there's no
> > > reason we shouldn't be able to do the same with multimedia components.
> > >
> > > If it's really an encoder, though, the problem might be different,
> > > though, since the interface (at a hardware or functional level if you
> > > will) remains the same. But I think in that case it's something that
> > > needs to be figured out internally by the OS. In my opinion, if we are
> > > in a situation where we have two different drivers in two subsystems for
> > > the same device, then we're doing something wrong and it should be fixed
> > > at that level, not by quirking the DT into making a decision for us.
> >
> > I tend to agree, and I'd like to be able to share drivers between V4L and
> > KMS. This is way down the road though.
>
> My point is that I don't see a need for sharing the drivers if we can
> make both V4L2 and DRM/KMS interoperate well enough to cover such use
> cases.
>
> Why share the drivers if we can make it work by sharing the data?
Would you like to try and merge the two subsystems ? :-)
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-25 14:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <1394972.QJSGNM8CJk@avalon>
[-- Attachment #1: Type: text/plain, Size: 5732 bytes --]
On Fri, Oct 25, 2013 at 03:47:21PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
>
> On Friday 25 October 2013 10:13:15 Thierry Reding wrote:
> > On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote:
> > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > > >> ...
> > > >>
> > > >>>> As I said, anything that really needs a CDF binding to work
> > > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > > >>>> easily be justified.
> > > >>>
> > > >>> The system as a whole would be more complex, but the panel could be
> > > >>> the same. We can't have two drivers for the same piece of hardware
> > > >>> in the DT world, as there will be a single compatible string and no
> > > >>> way to choose between the drivers (unlike the board code world that
> > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > > >>> and live happily with that ever after).
> > > >>
> > > >> That's not true. We can certainly define two different compatible
> > > >> values for a piece of HW if we have to. We can easily control whether
> > > >> they are handled by the same or different drivers in the OS.
> > > >
> > > > From an implementation point of view, sure. But from a conceptual point
> > > > of view, that would make the DT bindings pretty Linux-specific, with a
> > > > description of what the operating system should do instead of a
> > > > description of what the hardware looks like. My understanding is that
> > > > we've tried pretty hard in the past not to open that Pandora's box.
> > > >
> > > > The case I'm mostly concerned about would be two different compatibility
> > > > strings to select whether the device should be handled by a KMS or V4L
> > > > driver. I don't think that's a good idea.
> > >
> > > I wouldn't think of the two compatible values as selecting different
> > > specific Linux drivers, but rather they simply describe the HW in
> > > different levels of detail. The fact that if we know a certain level of
> > > detail about the HW means that Linux can and does create a KMS driver
> > > rather than a V4L2 driver seems like a detail that's completely hidden
> > > inside the OS.
> >
> > I've had a somewhat similar idea the other day but couldn't really put
> > it into words. Interestingly someone else mentioned a similar concept in
> > a different thread which I think describes what I had in mind as well.
> >
> > I was wondering if we couldn't use two compatible values to denote two
> > interfaces that the device implements. Something along the lines of:
> >
> > compatible = "vendor,block-name", "encoder";
> >
> > So a driver could primarily match on "vendor,block-name", but at the
> > same time it could use the additional information of being required to
> > implement "encoder" to expose an additonal interface.
>
> Let's take the hardware architecture described in
> http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39) as
> an example. The green blocks are part of the capture pipeline and handled by
> the V4L subsystem. The blue blocks are part of the display pipeline and
> handled by the KMS subsystem. One ADV7511 HDMI transmitter instance need to be
> controlled by a KMS driver and the second one by a V4L driver.
>
> The two instances are identical, so their DT nodes will show no difference if
> we stick to a hardware description only. There would then be no way to bind to
> different drivers.
I don't quite see why some of the green parts couldn't be part of the
display (KMS) pipeline. I thought I remember you say something about
implementing deep pipelining support in one of the more recent talks.
This sounds exactly like a case where this would be useful.
Obviously as long as that work isn't finished that won't help you, but I
think that providing a way to pass a video stream object from V4L2 to
DRM/KMS would be a more proper solution to this problem.
> > I suppose that perhaps something like a device_type property could be
> > used for that as well, and that might even be the more correct thing to
> > do.
> >
> > We already do something similar to make GPIO controllers expose an
> > interrupt chip by adding an interrupt-controller property. We also use
> > the gpio-controller property to mark a device node as exposing the GPIO
> > interface for that matter.
> >
> > So if a HW block can actually implement two different interfaces, each
> > of them being optional, then there should be ways to represent that in
> > DT as well. We already do that for "simpler" HW blocks, so there's no
> > reason we shouldn't be able to do the same with multimedia components.
> >
> > If it's really an encoder, though, the problem might be different,
> > though, since the interface (at a hardware or functional level if you
> > will) remains the same. But I think in that case it's something that
> > needs to be figured out internally by the OS. In my opinion, if we are
> > in a situation where we have two different drivers in two subsystems for
> > the same device, then we're doing something wrong and it should be fixed
> > at that level, not by quirking the DT into making a decision for us.
>
> I tend to agree, and I'd like to be able to share drivers between V4L and KMS.
> This is way down the road though.
My point is that I don't see a need for sharing the drivers if we can
make both V4L2 and DRM/KMS interoperate well enough to cover such use
cases.
Why share the drivers if we can make it work by sharing the data?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 13:49 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131025122925.GA24720-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]
Hi Thierry,
On Friday 25 October 2013 14:29:26 Thierry Reding wrote:
> On Fri, Oct 25, 2013 at 01:33:11PM +0200, Laurent Pinchart wrote:
> > On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
> > > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > > >> ...
> > > >>
> > > >>>> As I said, anything that really needs a CDF binding to work
> > > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > > >>>> easily be justified.
> > > >>>
> > > >>> The system as a whole would be more complex, but the panel could be
> > > >>> the same. We can't have two drivers for the same piece of hardware
> > > >>> in the DT world, as there will be a single compatible string and no
> > > >>> way to choose between the drivers (unlike the board code world that
> > > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > > >>> and live happily with that ever after).
> > > >>
> > > >> That's not true. We can certainly define two different compatible
> > > >> values for a piece of HW if we have to. We can easily control whether
> > > >> they are handled by the same or different drivers in the OS.
> > > >
> > > > From an implementation point of view, sure. But from a conceptual
> > > > point of view, that would make the DT bindings pretty Linux-specific,
> > > > with a description of what the operating system should do instead of a
> > > > description of what the hardware looks like. My understanding is that
> > > > we've tried pretty hard in the past not to open that Pandora's box.
> > > >
> > > > The case I'm mostly concerned about would be two different
> > > > compatibility strings to select whether the device should be handled
> > > > by a KMS or V4L driver. I don't think that's a good idea.
> > >
> > > I wouldn't think of the two compatible values as selecting different
> > > specific Linux drivers, but rather they simply describe the HW in
> > > different levels of detail. The fact that if we know a certain level of
> > > detail about the HW means that Linux can and does create a KMS driver
> > > rather than a V4L2 driver seems like a detail that's completely hidden
> > > inside the OS.
> >
> > I expect the same level of details to be needed on both the KMS and V4L
> > sides. Taking the example of the ADV7511 HDMI transmitter, the only
> > change in the DT bindings between KMS and V4L would be the compatible
> > string. "adi,adv7511-v4l" and "adi,adv7511-kms" is an option that I don't
> > really like. Renaming -v4l and -kms to different names wouldn't
> > fundamentally change the problem.
>
> I think that we're doing something fundamentally wrong if we use the
> same device (with the same functionality) in two different subsystems.
> If the device is used to display something, shouldn't we be moving the
> driver to DRM?
Let's discuss this in reply to the e-mail from this thread in which I mention
the ADV7511 example.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 13:47 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131025081314.GC19622-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]
Hi Thierry,
On Friday 25 October 2013 10:13:15 Thierry Reding wrote:
> On Thu, Oct 24, 2013 at 11:06:18PM +0100, Stephen Warren wrote:
> > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > >> ...
> > >>
> > >>>> As I said, anything that really needs a CDF binding to work
> > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > >>>> easily be justified.
> > >>>
> > >>> The system as a whole would be more complex, but the panel could be
> > >>> the same. We can't have two drivers for the same piece of hardware
> > >>> in the DT world, as there will be a single compatible string and no
> > >>> way to choose between the drivers (unlike the board code world that
> > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > >>> and live happily with that ever after).
> > >>
> > >> That's not true. We can certainly define two different compatible
> > >> values for a piece of HW if we have to. We can easily control whether
> > >> they are handled by the same or different drivers in the OS.
> > >
> > > From an implementation point of view, sure. But from a conceptual point
> > > of view, that would make the DT bindings pretty Linux-specific, with a
> > > description of what the operating system should do instead of a
> > > description of what the hardware looks like. My understanding is that
> > > we've tried pretty hard in the past not to open that Pandora's box.
> > >
> > > The case I'm mostly concerned about would be two different compatibility
> > > strings to select whether the device should be handled by a KMS or V4L
> > > driver. I don't think that's a good idea.
> >
> > I wouldn't think of the two compatible values as selecting different
> > specific Linux drivers, but rather they simply describe the HW in
> > different levels of detail. The fact that if we know a certain level of
> > detail about the HW means that Linux can and does create a KMS driver
> > rather than a V4L2 driver seems like a detail that's completely hidden
> > inside the OS.
>
> I've had a somewhat similar idea the other day but couldn't really put
> it into words. Interestingly someone else mentioned a similar concept in
> a different thread which I think describes what I had in mind as well.
>
> I was wondering if we couldn't use two compatible values to denote two
> interfaces that the device implements. Something along the lines of:
>
> compatible = "vendor,block-name", "encoder";
>
> So a driver could primarily match on "vendor,block-name", but at the
> same time it could use the additional information of being required to
> implement "encoder" to expose an additonal interface.
Let's take the hardware architecture described in
http://www.ideasonboard.org/media/meetings/20131024-elce.pdf#39 (page 39) as
an example. The green blocks are part of the capture pipeline and handled by
the V4L subsystem. The blue blocks are part of the display pipeline and
handled by the KMS subsystem. One ADV7511 HDMI transmitter instance need to be
controlled by a KMS driver and the second one by a V4L driver.
The two instances are identical, so their DT nodes will show no difference if
we stick to a hardware description only. There would then be no way to bind to
different drivers.
> I suppose that perhaps something like a device_type property could be
> used for that as well, and that might even be the more correct thing to
> do.
>
> We already do something similar to make GPIO controllers expose an
> interrupt chip by adding an interrupt-controller property. We also use
> the gpio-controller property to mark a device node as exposing the GPIO
> interface for that matter.
>
> So if a HW block can actually implement two different interfaces, each
> of them being optional, then there should be ways to represent that in
> DT as well. We already do that for "simpler" HW blocks, so there's no
> reason we shouldn't be able to do the same with multimedia components.
>
> If it's really an encoder, though, the problem might be different,
> though, since the interface (at a hardware or functional level if you
> will) remains the same. But I think in that case it's something that
> needs to be figured out internally by the OS. In my opinion, if we are
> in a situation where we have two different drivers in two subsystems for
> the same device, then we're doing something wrong and it should be fixed
> at that level, not by quirking the DT into making a decision for us.
I tend to agree, and I'd like to be able to share drivers between V4L and KMS.
This is way down the road though.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-25 12:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Stephen Warren, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <1580941.z2CDo8WmRF@avalon>
[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]
On Fri, Oct 25, 2013 at 01:33:11PM +0200, Laurent Pinchart wrote:
> Hi Stephen,
>
> On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
> > On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> > >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> > >> ...
> > >>
> > >>>> As I said, anything that really needs a CDF binding to work
> > >>>> likely isn't "simple" anymore, therefore a separate driver can
> > >>>> easily be justified.
> > >>>
> > >>> The system as a whole would be more complex, but the panel could be
> > >>> the same. We can't have two drivers for the same piece of hardware
> > >>> in the DT world, as there will be a single compatible string and no
> > >>> way to choose between the drivers (unlike the board code world that
> > >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > >>> and live happily with that ever after).
> > >>
> > >> That's not true. We can certainly define two different compatible
> > >> values for a piece of HW if we have to. We can easily control whether
> > >> they are handled by the same or different drivers in the OS.
> > >
> > > From an implementation point of view, sure. But from a conceptual point of
> > > view, that would make the DT bindings pretty Linux-specific, with a
> > > description of what the operating system should do instead of a
> > > description of what the hardware looks like. My understanding is that
> > > we've tried pretty hard in the past not to open that Pandora's box.
> > >
> > > The case I'm mostly concerned about would be two different compatibility
> > > strings to select whether the device should be handled by a KMS or V4L
> > > driver. I don't think that's a good idea.
> >
> > I wouldn't think of the two compatible values as selecting different
> > specific Linux drivers, but rather they simply describe the HW in
> > different levels of detail. The fact that if we know a certain level of
> > detail about the HW means that Linux can and does create a KMS driver
> > rather than a V4L2 driver seems like a detail that's completely hidden
> > inside the OS.
>
> I expect the same level of details to be needed on both the KMS and V4L sides.
> Taking the example of the ADV7511 HDMI transmitter, the only change in the DT
> bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l"
> and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and
> -kms to different names wouldn't fundamentally change the problem.
I think that we're doing something fundamentally wrong if we use the
same device (with the same functionality) in two different subsystems.
If the device is used to display something, shouldn't we be moving the
driver to DRM?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 1/1] backlight: hx8357: Remove redundant of_match_ptr
From: Sachin Kamat @ 2013-10-25 12:11 UTC (permalink / raw)
To: linux-fbdev
'hx8357_dt_ids' is always compiled in. Hence of_match_ptr is
not needed.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/backlight/hx8357.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index c7af8c4..5a4dd33 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -685,7 +685,7 @@ static struct spi_driver hx8357_driver = {
.remove = hx8357_remove,
.driver = {
.name = "hx8357",
- .of_match_table = of_match_ptr(hx8357_dt_ids),
+ .of_match_table = hx8357_dt_ids,
},
};
--
1.7.9.5
^ permalink raw reply related
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 11:33 UTC (permalink / raw)
To: Stephen Warren
Cc: Thierry Reding, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <526999DA.7080409-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Hi Stephen,
On Thursday 24 October 2013 23:06:18 Stephen Warren wrote:
> On 10/24/2013 12:20 PM, Laurent Pinchart wrote:
> > On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> >> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> >> ...
> >>
> >>>> As I said, anything that really needs a CDF binding to work
> >>>> likely isn't "simple" anymore, therefore a separate driver can
> >>>> easily be justified.
> >>>
> >>> The system as a whole would be more complex, but the panel could be
> >>> the same. We can't have two drivers for the same piece of hardware
> >>> in the DT world, as there will be a single compatible string and no
> >>> way to choose between the drivers (unlike the board code world that
> >>> could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> >>> and live happily with that ever after).
> >>
> >> That's not true. We can certainly define two different compatible
> >> values for a piece of HW if we have to. We can easily control whether
> >> they are handled by the same or different drivers in the OS.
> >
> > From an implementation point of view, sure. But from a conceptual point of
> > view, that would make the DT bindings pretty Linux-specific, with a
> > description of what the operating system should do instead of a
> > description of what the hardware looks like. My understanding is that
> > we've tried pretty hard in the past not to open that Pandora's box.
> >
> > The case I'm mostly concerned about would be two different compatibility
> > strings to select whether the device should be handled by a KMS or V4L
> > driver. I don't think that's a good idea.
>
> I wouldn't think of the two compatible values as selecting different
> specific Linux drivers, but rather they simply describe the HW in
> different levels of detail. The fact that if we know a certain level of
> detail about the HW means that Linux can and does create a KMS driver
> rather than a V4L2 driver seems like a detail that's completely hidden
> inside the OS.
I expect the same level of details to be needed on both the KMS and V4L sides.
Taking the example of the ADV7511 HDMI transmitter, the only change in the DT
bindings between KMS and V4L would be the compatible string. "adi,adv7511-v4l"
and "adi,adv7511-kms" is an option that I don't really like. Renaming -v4l and
-kms to different names wouldn't fundamentally change the problem.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-25 11:27 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Tomi Valkeinen, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131024114823.GC11296-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 15398 bytes --]
Hi Thierry,
On Thursday 24 October 2013 13:48:24 Thierry Reding wrote:
> On Thu, Oct 24, 2013 at 01:05:49PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 14:46:20 Thierry Reding wrote:
> > > On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:
> > > > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> > > > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
> > [snip]
> >
> > > > The point is that the video pipeline must be described in DT. Having a
> > > > per-device way to represent connections would be a nightmare to
> > > > support from an implementation point of view, hence the need for a
> > > > generic way to describe them.
> > >
> > > Okay, so we're back to the need to describe pipelines in DT. At the risk
> > > of sounding selfish: I don't care about pipelines. I just want us to
> > > settle on a way to represent a dumb panel in DT so that it can be
> > > enabled when it needs to. Furthermore I don't have any hardware that
> > > exhibits any of these "advanced" features, so I'm totally unqualified to
> > > work on any of this.
> > >
> > > Can we please try to be a little pragmatic here and solve one problem at
> > > a time? I am aware that solving this for panels may require some amount
> > > of foresight, but let's not try to solve everything at once at the risk
> > > of not getting anything done at all.
> >
> > I won't force you to care about pipelines, and I don't think this is
> > selfish at all :-)
> >
> > What I would like to ensure is that whatever bindings we come up with,
> > they will not preclude us from adding pipeline support when needed (as I'm
> > pretty sure it will be needed at some point).
>
> Sure. I think I've explained elsewhere that I consider this easily
> possible. Other people have said the same thing. If all we do is add
> properties to a binding, then drivers written for an old binding will
> have no problem continuing to work.
>
> I also think that we're mostly being too scared. If ever the next device
> comes around that requires explicit pipeline support to work, how likely
> is it to have the exact same display as prior devices?
The exact same panel, probably low, but a panel supported by the same driver
using the same DT bindings (such as the generic simple panel driver), quite
high.
> > > > > I'm not at all opposed to the idea of CDF or the problems it tries
> > > > > to address. I don't think it necessarily should be a separate
> > > > > framework for reasons explained earlier. But even if it were to end
> > > > > up in a separate framework there's absolutely nothing preventing us
> > > > > from adding a DRM panel driver that hooks into CDF as a "backend" of
> > > > > sorts for panels that require something more complex than the
> > > > > simple-panel binding.
> > > >
> > > > Once again it's not about panel having complex needs, but more about
> > > > using simple panels in complex pipelines. I'm fine with the drm_panel
> > > > infrastructure, what I would like is DT bindings that describe
> > > > connections in a more future-proof way. The rest is fine.
> > >
> > > And I've already said elsewhere that the bindings in their current form
> > > are easily extensible to cater for the needs of CDF.
> >
> > The simple panel bindings do not include any connection information, so we
> > could add that later when needed without having to deprecate, remove or
> > repurpose existing properties.
>
> Yes, I agree.
>
> > The simple panel driver would need to be extended, which isn't much of a
> > problem (except that extending it with CDF support might require changes
> > to the users of the simple panel driver, which I believe won't be happily
> > accepted, but that's a different issue).
>
> That might be true. But that's just the way kernel development works. We
> sometimes require major rework of APIs and we're actually pretty good at
> pulling that off, so I don't worry too much about it.
>
> Also, being the original author of the driver makes me the maintainer,
> and if it should prove to be necessary I'm absolutely willing to add CDF
> support.
Much appreciated :-)
> > My concern is also on the other side. In the patches you've sent the tegra
> > driver uses a custom nvidia,panel property to reference the panel. That
> > would of course not be CDF-compatible, but there's no way around that at
> > the moment if we don't want to keep development of all ARM KMS drivers
> > stalled for the next 6 months.
>
> Well, the nvidia,panel property is part of the display output and it is
> an optional property. So the driver already needs to cope with it being
> absent. If the display output driver is ever extended with CDF support
> then we can just go and use CDF if CDF-specific properties exist, or we
> fall back to nvidia,panel if that doesn't exist.
>
> > It boils down to the question of whether DT should be a stable ABI, and
> > I'm increasingly tempted to say that I don't care. I want to solve
> > issues we have on the display side, the firmware interface isn't my main
> > concern.
>
> I wholeheartedly agree. In my opinion it is much more useful to come up
> with a solution that works now, rather than discussing and arguing
> things to death and not get anything done. If that means that I'll have
> to maintain additional code, then so be it.
>
> > > > > But that's precisely the point. Why would we need to go back from
> > > > > the panel to the display controller? Especially for dumb panels that
> > > > > can't or don't have to be configured in any way. Even if they needed
> > > > > some sort of setup, why can't that be done from the display
> > > > > controller/output.
> > > > >
> > > > > Even given a setup where a DSI controller needs to write some
> > > > > registers in a panel upon initialization, I don't see why the
> > > > > reverse connection needs to be described. The fact alone that an
> > > > > output dereferences a panel's phandle should be enough to connect
> > > > > both of them and have any panel driver use the DSI controller that
> > > > > it's been attached to for the programming.
> > > > >
> > > > > That's very much analogous to how I2C works. There are no
> > > > > connections back to the I2C master from the slaves. Yet each I2C
> > > > > client driver manages to use the services provided by the I2C master
> > > > > to perform transactions on the I2C bus. In a similar way the DSI
> > > > > controller is the bus master that talks to DSI panels. DSI panels
> > > > > don't actively talk to the DSI controller.
> > > >
> > > > It's all about modeling video pipeline graphs in DT. To be able to
> > > > walk the graph we need to describe connections. Not having
> > > > bidirectional information means that we restrict the points at which
> > > > we can start walking the graph, potentially making our life much more
> > > > difficult in the future.
> > > >
> > > > What's so wrong about adding a port node and link information to the
> > > > panel DT node ? It describe what's there: the panel has one input, why
> > > > not make that explicit ?
> > >
> > > What's wrong with it is that there's no way to verify the soundness of
> > > the design by means of a full implementation because we're missing the
> > > majority of the pieces. Just putting the nodes into DT to provide some
> > > imaginary future-proofness isn't helpful either. Without any code that
> > > actually uses them they are useless.
> > >
> > > And again, why should we add them right away (while not needed) when
> > > they can easily be added in a backwards-compatible manner at some later
> > > point when there's actually any use for them and they can actually be
> > > tested in some larger framework?
> >
> > It's the "easily" part I'm not sure about. I doubt we'll ever have any
> > easy to solve DT backward compatibility issue. However, as mentioned
> > above, this shouldn't be a show stopper. I'm thus fine with the way the
> > proposed bindings describe (or rather don't describe) the connection.
> > However, I will then expect your support in the future to implement the
> > "easy" extension of the bindings to support CDF.
>
> Again, I don't expect that to ever happen. But if it ever happens anyway,
> then...
>
> > Do we have a deal ? ;-)
>
> Yes, we do.
Great!
> > > > > > > > > +static void panel_simple_enable(struct drm_panel *panel)
> > > > > > > > > +{
> > > > > > > > > + struct panel_simple *p = to_panel_simple(panel);
> > > > > > > > > + int err;
> > > > > > > > > +
> > > > > > > > > + if (p->enabled)
> > > > > > > > > + return;
> > > > > > > > > +
> > > > > > > > > + err = regulator_enable(p->supply);
> > > > > > > > > + if (err < 0)
> > > > > > > > > + dev_err(panel->dev, "failed to enable supply:
%d\n",
> >
> > err);
> >
> > > > > > > > Is that really a non-fatal error ? Shouldn't the enable
> > > > > > > > operation return an int ?
> > > > > > >
> > > > > > > There's no way to propagate this in DRM, so why go through the
> > > > > > > trouble of returning the error? Furthermore, there's nothing
> > > > > > > that the caller could do to remedy the situation anyway.
> > > > > >
> > > > > > That's a DRM issue, which could be fixed. While the caller can't
> > > > > > remedy the situation, it should at least report the error to the
> > > > > > application instead of silently ignoring it.
> > > > >
> > > > > Perhaps. It's not really relevant to the discussion and can always
> > > > > be fixed in a subsequent patch.
> > > >
> > > > Most things can be fixed by a subsequent patch, that's not a good
> > > > enough reason not to address the known problems before pushing the
> > > > code to mainline (to clarify my point of view, there's no need to fix
> > > > DRM to use the return value before pushing this patch set to mainline,
> > > > but I'd like a v2 with an int return value).
> > >
> > > Why? What's the use of returning an error if you know up front that it
> > > can't be used? This should be changed if or when we "fix" DRM to
> > > propagate errors.
> >
> > Because not doing so now will require us to change (potentially) lots of
> > panel drivers at that time. It's much easier to have each panel driver
> > developer implement the required code in his/her driver than having a
> > single developer refactoring the code later and have to touch all
> > drivers. If your concern is that the error paths won't be testable at the
> > moment, you could easily already add a WARN_ON() to the caller to catch
> > problems.
>
> I don't mind fixing potentially many drivers. The conversion is pretty
> mechanical and therefore easy. We even have tools such as coccinelle to
> help with it. But we've probably spent more time arguing the point than
> it would take to make this simple change, so I'll just go and change the
> return value to an int and return an error.
Thank you.
> Perhaps if I'm in the mood I'll even write up a patch to propagate the
> error further.
>
> > > > > > > > Instead of hardcoding the modes in the driver, which would
> > > > > > > > then require to be updated for every new simple panel model
> > > > > > > > (and we know there are lots of them), why don't you specify
> > > > > > > > the modes in the panel DT node ? The simple panel driver would
> > > > > > > > then become much more generic. It would also allow board
> > > > > > > > designers to tweak h/v sync timings depending on the system
> > > > > > > > requirements.
> > > > > > >
> > > > > > > Sigh... we keep second-guessing ourselves. Back at the time when
> > > > > > > power sequences were designed (and NAKed at the last minute), we
> > > > > > > all decided that the right thing to do would be to use specific
> > > > > > > compatible values for individual panels, because that would
> > > > > > > allow us to encode the power sequencing within the driver. And
> > > > > > > when we already have the panel model encoded in the compatible
> > > > > > > value, we might just as well encode the mode within the driver
> > > > > > > for that panel. Otherwise we'll have to keep adding the same
> > > > > > > mode timings for every board that uses the same panel.
> > > > > > >
> > > > > > > I do agree though that it might be useful to tweak the mode in
> > > > > > > case the default one doesn't work. How about we provide a means
> > > > > > > to override the mode encoded in the driver using one specified
> > > > > > > in the DT? That's obviously a backwards-compatible change, so it
> > > > > > > could be added if or when it becomes necessary.
> > > > > >
> > > > > > I share Tomi's point of view here. Your three panels use the same
> > > > > > power sequence, so I believe we should have a generic panel
> > > > > > compatible string that would use modes described in DT for the
> > > > > > common case. Only if a panel required something more complex which
> > > > > > can't (or which could, but won't for any reason) accurately be
> > > > > > described in DT would you need to extend the driver.
> > > > >
> > > > > I don't see the point quite frankly. You seem to be assuming that
> > > > > every panel will only be used in a single board.
> > > >
> > > > No, but in practice that's often the case, at least for boards with
> > > > .dts files in the mainline kernel.
> > > >
> > > > > However what you're proposing will require the mode timings to be
> > > > > repeated in every board's DT file, if the same panel is ever used on
> > > > > more than a single board.
> > > >
> > > > Is that a problem ? You could #include a .dtsi for the panel that
> > > > would specify the video mode if you want to avoid multiple copies.
> > >
> > > Yes, I don't think it's desirable to duplicate data needlessly in DT. It
> > > also makes the binding more difficult to use. If I know that the panel
> > > is one supported by the simple-panel binding, I can just go and add the
> > > right compatible value without having to bother looking up the mode
> > > timings and duplicating them. That way DT writers get to concern
> > > themselves only with the variable data.
> >
> > I've had a quick chat with Dave Airlie and Hans de Goede yesterday about
> > this. As most panels will use standard timings, Hans proposed adding a
> > timings property that would reference the standard timings the panel uses
> > (this could be a string or integer defined in include/dt-bindings/...).
> > In most case DT would just have a single property to select the timings,
> > and only in more complex cases where the system designer wants to use
> > custom timings would the timings need to be manually defined.
>
> We can do the same thing within the kernel. We already have a list of
> standard EDID/HDMI/CEA display modes, so we could similarly add a new
> list of common display panel modes and make each driver reference that
> instead of defining a custom one.
Sure. My point is that I would like to avoid adding zillions of compatible
properties to the driver, when we could use a single property in the DT
bindings that would specify the timings instead. This would lower the amount
of changes made to the simple panel driver, while keeping DT simple enough (at
least in my opinion).
> And that still enables us to add a property that would allow DT writers
> to override the display mode if they need to.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH 9/9] backlight: atmel-pwm-bl: use gpio_request_one
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:15 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-10-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Use devm_gpio_request_one rather than requesting and setting direction
> in two calls.
this is the same I do not see any advantage
and as I said for ather backligth It's wrong to enable or disable it at probe
as the bootloader might have already enable it for splash screen
Best Regards,
J.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>:w
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index 1277e0c..5ea2517 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -124,6 +124,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> const struct atmel_pwm_bl_platform_data *pdata;
> struct backlight_device *bldev;
> struct atmel_pwm_bl *pwmbl;
> + int flags;
> int retval;
>
> pdata = dev_get_platdata(&pdev->dev);
> @@ -149,14 +150,14 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> return retval;
>
> if (gpio_is_valid(pwmbl->gpio_on)) {
> - retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
> - "gpio_atmel_pwm_bl");
> - if (retval)
> - goto err_free_pwm;
> -
> /* Turn display off by default. */
> - retval = gpio_direction_output(pwmbl->gpio_on,
> - 0 ^ pdata->on_active_low);
> + if (pdata->on_active_low)
> + flags = GPIOF_OUT_INIT_HIGH;
> + else
> + flags = GPIOF_OUT_INIT_LOW;
> +
> + retval = devm_gpio_request_one(&pdev->dev, pwmbl->gpio_on,
> + flags, "gpio_atmel_pwm_bl");
> if (retval)
> goto err_free_pwm;
> }
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 8/9] backlight: atmel-pwm-bl: refactor gpio_on handling
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:13 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-9-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Add helper function to control the gpio_on signal.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
ok
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index ffc30d2..1277e0c 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -26,6 +26,14 @@ struct atmel_pwm_bl {
> int gpio_on;
> };
>
> +static void atmel_pwm_bl_set_gpio_on(struct atmel_pwm_bl *pwmbl, int on)
> +{
> + if (!gpio_is_valid(pwmbl->gpio_on))
> + return;
> +
> + gpio_set_value(pwmbl->gpio_on, on ^ pwmbl->pdata->on_active_low);
> +}
> +
> static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> {
> struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> @@ -48,19 +56,13 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> pwm_duty = pwmbl->pdata->pwm_duty_min;
>
> if (!intensity) {
> - if (gpio_is_valid(pwmbl->gpio_on)) {
> - gpio_set_value(pwmbl->gpio_on,
> - 0 ^ pwmbl->pdata->on_active_low);
> - }
> + atmel_pwm_bl_set_gpio_on(pwmbl, 0);
> pwm_channel_writel(&pwmbl->pwmc, PWM_CUPD, pwm_duty);
> pwm_channel_disable(&pwmbl->pwmc);
> } else {
> pwm_channel_enable(&pwmbl->pwmc);
> pwm_channel_writel(&pwmbl->pwmc, PWM_CUPD, pwm_duty);
> - if (gpio_is_valid(pwmbl->gpio_on)) {
> - gpio_set_value(pwmbl->gpio_on,
> - 1 ^ pwmbl->pdata->on_active_low);
> - }
> + atmel_pwm_bl_set_gpio_on(pwmbl, 1);
> }
>
> return 0;
> @@ -196,10 +198,7 @@ static int atmel_pwm_bl_remove(struct platform_device *pdev)
> {
> struct atmel_pwm_bl *pwmbl = platform_get_drvdata(pdev);
>
> - if (gpio_is_valid(pwmbl->gpio_on)) {
> - gpio_set_value(pwmbl->gpio_on,
> - 0 ^ pwmbl->pdata->on_active_low);
> - }
> + atmel_pwm_bl_set_gpio_on(pwmbl, 0);
> pwm_channel_disable(&pwmbl->pwmc);
> pwm_channel_free(&pwmbl->pwmc);
>
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 5/9] backlight: atmel-pwm-bl: clean up get_intensity
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:12 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-6-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Clean up get_intensity to increase readability.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
this ok
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index 52a8134..504061c 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -70,15 +70,14 @@ static int atmel_pwm_bl_set_intensity(struct backlight_device *bd)
> static int atmel_pwm_bl_get_intensity(struct backlight_device *bd)
> {
> struct atmel_pwm_bl *pwmbl = bl_get_data(bd);
> + u32 cdty;
> u32 intensity;
>
> - if (pwmbl->pdata->pwm_active_low) {
> - intensity = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY) -
> - pwmbl->pdata->pwm_duty_min;
> - } else {
> - intensity = pwmbl->pdata->pwm_duty_max -
> - pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> - }
> + cdty = pwm_channel_readl(&pwmbl->pwmc, PWM_CDTY);
> + if (pwmbl->pdata->pwm_active_low)
> + intensity = cdty - pwmbl->pdata->pwm_duty_min;
> + else
> + intensity = pwmbl->pdata->pwm_duty_max - cdty;
>
> return (u16)intensity;
> }
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH 4/9] backlight: atmel-pwm-bl: clean up probe error handling
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-25 11:11 UTC (permalink / raw)
To: Johan Hovold
Cc: Richard Purdie, Jingoo Han, Nicolas Ferre, Tomi Valkeinen,
Andrew Morton, linux-fbdev, linux-kernel
In-Reply-To: <1382522143-32072-5-git-send-email-jhovold@gmail.com>
On 11:55 Wed 23 Oct , Johan Hovold wrote:
> Clean up probe error handling by checking parameters before any
> allocations and removing an obsolete error label. Also remove
> unnecessary reset of private gpio number.
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> drivers/video/backlight/atmel-pwm-bl.c | 31 ++++++++++++-------------------
> 1 file changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/video/backlight/atmel-pwm-bl.c b/drivers/video/backlight/atmel-pwm-bl.c
> index cc5a5ed..52a8134 100644
> --- a/drivers/video/backlight/atmel-pwm-bl.c
> +++ b/drivers/video/backlight/atmel-pwm-bl.c
> @@ -126,40 +126,33 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
> struct atmel_pwm_bl *pwmbl;
> int retval;
>
> + pdata = dev_get_platdata(&pdev->dev);
> + if (!pdata)
> + return -ENODEV;
> +
> + if (pdata->pwm_compare_max < pdata->pwm_duty_max ||
> + pdata->pwm_duty_min > pdata->pwm_duty_max ||
> + pdata->pwm_frequency = 0)
> + return -EINVAL;
> +
> pwmbl = devm_kzalloc(&pdev->dev, sizeof(struct atmel_pwm_bl),
> GFP_KERNEL);
> if (!pwmbl)
> return -ENOMEM;
>
> pwmbl->pdev = pdev;
> -
> - pdata = dev_get_platdata(&pdev->dev);
> - if (!pdata) {
> - retval = -ENODEV;
> - goto err_free_mem;
> - }
> -
> - if (pdata->pwm_compare_max < pdata->pwm_duty_max ||
> - pdata->pwm_duty_min > pdata->pwm_duty_max ||
> - pdata->pwm_frequency = 0) {
> - retval = -EINVAL;
> - goto err_free_mem;
> - }
> -
> pwmbl->pdata = pdata;
> pwmbl->gpio_on = pdata->gpio_on;
>
> retval = pwm_channel_alloc(pdata->pwm_channel, &pwmbl->pwmc);
> if (retval)
> - goto err_free_mem;
> + return retval;
>
> if (pwmbl->gpio_on != -1) {
gpio_is_valid here
> retval = devm_gpio_request(&pdev->dev, pwmbl->gpio_on,
> "gpio_atmel_pwm_bl");
> - if (retval) {
> - pwmbl->gpio_on = -1;
> + if (retval)
> goto err_free_pwm;
> - }
>
> /* Turn display off by default. */
> retval = gpio_direction_output(pwmbl->gpio_on,
> @@ -197,7 +190,7 @@ static int atmel_pwm_bl_probe(struct platform_device *pdev)
>
> err_free_pwm:
> pwm_channel_free(&pwmbl->pwmc);
> -err_free_mem:
> +
> return retval;
> }
>
> --
> 1.8.4
>
^ 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