* Re: tty, vt: lockdep warnings
From: Hugh Dickins @ 2012-11-05 19:17 UTC (permalink / raw)
To: Sasha Levin
Cc: Daniel Vetter, Alan Cox, Sasha Levin, Greg Kroah-Hartman,
Jiri Slaby, linux-kernel, Dave Jones, linux-fbdev,
florianSchandinat
In-Reply-To: <5097FEA9.2090603@oracle.com>
On Mon, 5 Nov 2012, Sasha Levin wrote:
> On 11/05/2012 12:59 PM, Alan Cox wrote:
> > On Mon, 5 Nov 2012 12:26:43 -0500
> > Sasha Levin <levinsasha928@gmail.com> wrote:
> >
> >> Ping? Should I bisect it?
> >>
> >> On Fri, Oct 26, 2012 at 9:37 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >>> On Thu, 25 Oct 2012 15:37:43 -0400
> >>> Sasha Levin <sasha.levin@oracle.com> wrote:
> >>>
> >>>> Hi all,
> >>>>
> >>>> While fuzzing with trinity inside a KVM tools (lkvm) guest running latest -next kernel,
> >>>> I've stumbled on the following spew:
> >>>
> >>> Looks real enough but its not a tty/vt layer spew. This is all coming out
> >>> of the core framebuffer code which doesn't seem to be able to decide what
> >>> the locking rules at the invocation of fb_notifier_call_chain are.
> >>>
> >>> It might need some console layer tweaking to provide 'register console
> >>> and I already hold the locks' or similar but that notifier needs some
> >>> kind of sanity applying as well.
> >>>
> >>> Cc'ing the fbdev folks
> >
> > I've cc'd the framebuffer folks. I can see why its occurring but I have
> > no idea how they intend to fix it and I've not seen any replies.
> >
> > Sorry but I've got enough other things on my plate right now without
> > trying to deal with the locking brain damage that the fbdev layer is.
> >
> > As far as I can tell the actual bug proper is years old.
> >
> > Alan
> >
>
> Ow, I figured it's something new since I've only now started seeing it in fuzz
> tests, and it reproduces pretty much every time.
The fbdev potential for deadlock may be years old, but the warning
(and consequent disabling of lockdep from that point on - making it
useless to everybody else in need of it) is new, and comes from the
commit below in linux-next.
I revert it in my own testing: if there is no quick fix to the
fbdev issue on the way, Daniel, please revert it from your tree.
Thanks,
Hugh
commit daee779718a319ff9f83e1ba3339334ac650bb22
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Sat Sep 22 19:52:11 2012 +0200
console: implement lockdep support for console_lock
Dave Airlie recently discovered a locking bug in the fbcon layer,
where a timer_del_sync (for the blinking cursor) deadlocks with the
timer itself, since both (want to) hold the console_lock:
https://lkml.org/lkml/2012/8/21/36
Unfortunately the console_lock isn't a plain mutex and hence has no
lockdep support. Which resulted in a few days wasted of tracking down
this bug (complicated by the fact that printk doesn't show anything
when the console is locked) instead of noticing the bug much earlier
with the lockdep splat.
Hence I've figured I need to fix that for the next deadlock involving
console_lock - and with kms/drm growing ever more complex locking
that'll eventually happen.
Now the console_lock has rather funky semantics, so after a quick irc
discussion with Thomas Gleixner and Dave Airlie I've quickly ditched
the original idead of switching to a real mutex (since it won't work)
and instead opted to annotate the console_lock with lockdep
information manually.
There are a few special cases:
- The console_lock state is protected by the console_sem, and usually
grabbed/dropped at _lock/_unlock time. But the suspend/resume code
drops the semaphore without dropping the console_lock (see
suspend_console/resume_console). But since the same thread that did
the suspend will do the resume, we don't need to fix up anything.
- In the printk code there's a special trylock, only used to kick off
the logbuffer printk'ing in console_unlock. But all that happens
while lockdep is disable (since printk does a few other evil
tricks). So no issue there, either.
- The console_lock can also be acquired form irq context (but only
with a trylock). lockdep already handles that.
This all leaves us with annotating the normal console_lock, _unlock
and _trylock functions.
And yes, it works - simply unloading a drm kms driver resulted in
lockdep complaining about the deadlock in fbcon_deinit:
===========================
[ INFO: possible circular locking dependency detected ]
3.6.0-rc2+ #552 Not tainted
-------------------------------------------------------
kms-reload/3577 is trying to acquire lock:
((&info->queue)){+.+...}, at: [<ffffffff81058c70>] wait_on_work+0x0/0xa7
but task is already holding lock:
(console_lock){+.+.+.}, at: [<ffffffff81264686>] bind_con_driver+0x38/0x263
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (console_lock){+.+.+.}:
[<ffffffff81087440>] lock_acquire+0x95/0x105
[<ffffffff81040190>] console_lock+0x59/0x5b
[<ffffffff81209cb6>] fb_flashcursor+0x2e/0x12c
[<ffffffff81057c3e>] process_one_work+0x1d9/0x3b4
[<ffffffff810584a2>] worker_thread+0x1a7/0x24b
[<ffffffff8105ca29>] kthread+0x7f/0x87
[<ffffffff813b1204>] kernel_thread_helper+0x4/0x10
-> #0 ((&info->queue)){+.+...}:
[<ffffffff81086cb3>] __lock_acquire+0x999/0xcf6
[<ffffffff81087440>] lock_acquire+0x95/0x105
[<ffffffff81058cab>] wait_on_work+0x3b/0xa7
[<ffffffff81058dd6>] __cancel_work_timer+0xbf/0x102
[<ffffffff81058e33>] cancel_work_sync+0xb/0xd
[<ffffffff8120a3b3>] fbcon_deinit+0x11c/0x1dc
[<ffffffff81264793>] bind_con_driver+0x145/0x263
[<ffffffff81264a45>] unbind_con_driver+0x14f/0x195
[<ffffffff8126540c>] store_bind+0x1ad/0x1c1
[<ffffffff8127cbb7>] dev_attr_store+0x13/0x1f
[<ffffffff8116d884>] sysfs_write_file+0xe9/0x121
[<ffffffff811145b2>] vfs_write+0x9b/0xfd
[<ffffffff811147b7>] sys_write+0x3e/0x6b
[<ffffffff813b0039>] system_call_fastpath+0x16/0x1b
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(console_lock);
lock((&info->queue));
lock(console_lock);
lock((&info->queue));
*** DEADLOCK ***
v2: Mark the lockdep_map static, noticed by Jani Nikula.
Cc: Dave Airlie <airlied@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/kernel/printk.c b/kernel/printk.c
index 2d607f4..ee79f14 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -87,6 +87,12 @@ static DEFINE_SEMAPHORE(console_sem);
struct console *console_drivers;
EXPORT_SYMBOL_GPL(console_drivers);
+#ifdef CONFIG_LOCKDEP
+static struct lockdep_map console_lock_dep_map = {
+ .name = "console_lock"
+};
+#endif
+
/*
* This is used for debugging the mess that is the VT code by
* keeping track if we have the console semaphore held. It's
@@ -1914,6 +1920,7 @@ void console_lock(void)
return;
console_locked = 1;
console_may_schedule = 1;
+ mutex_acquire(&console_lock_dep_map, 0, 0, _RET_IP_);
}
EXPORT_SYMBOL(console_lock);
@@ -1935,6 +1942,7 @@ int console_trylock(void)
}
console_locked = 1;
console_may_schedule = 0;
+ mutex_acquire(&console_lock_dep_map, 0, 1, _RET_IP_);
return 1;
}
EXPORT_SYMBOL(console_trylock);
@@ -2095,6 +2103,7 @@ skip:
local_irq_restore(flags);
}
console_locked = 0;
+ mutex_release(&console_lock_dep_map, 1, _RET_IP_);
/* Release the exclusive_console once it is used */
if (unlikely(exclusive_console))
^ permalink raw reply related
* Re: tty, vt: lockdep warnings
From: Sasha Levin @ 2012-11-05 18:00 UTC (permalink / raw)
To: Alan Cox
Cc: Sasha Levin, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org, Dave Jones, linux-fbdev,
florianSchandinat
In-Reply-To: <20121105175937.26f31d2a@pyramind.ukuu.org.uk>
On 11/05/2012 12:59 PM, Alan Cox wrote:
> On Mon, 5 Nov 2012 12:26:43 -0500
> Sasha Levin <levinsasha928@gmail.com> wrote:
>
>> Ping? Should I bisect it?
>>
>> On Fri, Oct 26, 2012 at 9:37 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>> On Thu, 25 Oct 2012 15:37:43 -0400
>>> Sasha Levin <sasha.levin@oracle.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> While fuzzing with trinity inside a KVM tools (lkvm) guest running latest -next kernel,
>>>> I've stumbled on the following spew:
>>>
>>> Looks real enough but its not a tty/vt layer spew. This is all coming out
>>> of the core framebuffer code which doesn't seem to be able to decide what
>>> the locking rules at the invocation of fb_notifier_call_chain are.
>>>
>>> It might need some console layer tweaking to provide 'register console
>>> and I already hold the locks' or similar but that notifier needs some
>>> kind of sanity applying as well.
>>>
>>> Cc'ing the fbdev folks
>
> I've cc'd the framebuffer folks. I can see why its occurring but I have
> no idea how they intend to fix it and I've not seen any replies.
>
> Sorry but I've got enough other things on my plate right now without
> trying to deal with the locking brain damage that the fbdev layer is.
>
> As far as I can tell the actual bug proper is years old.
>
> Alan
>
Ow, I figured it's something new since I've only now started seeing it in fuzz
tests, and it reproduces pretty much every time.
Thanks,
Sasha
^ permalink raw reply
* Re: tty, vt: lockdep warnings
From: Alan Cox @ 2012-11-05 17:59 UTC (permalink / raw)
To: Sasha Levin
Cc: Sasha Levin, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org, Dave Jones, linux-fbdev,
florianSchandinat
In-Reply-To: <CA+1xoqdEesjh1EZvR_r7Hn0GUc7741EDWn59qagngcxg4=9bjQ@mail.gmail.com>
On Mon, 5 Nov 2012 12:26:43 -0500
Sasha Levin <levinsasha928@gmail.com> wrote:
> Ping? Should I bisect it?
>
> On Fri, Oct 26, 2012 at 9:37 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > On Thu, 25 Oct 2012 15:37:43 -0400
> > Sasha Levin <sasha.levin@oracle.com> wrote:
> >
> >> Hi all,
> >>
> >> While fuzzing with trinity inside a KVM tools (lkvm) guest running latest -next kernel,
> >> I've stumbled on the following spew:
> >
> > Looks real enough but its not a tty/vt layer spew. This is all coming out
> > of the core framebuffer code which doesn't seem to be able to decide what
> > the locking rules at the invocation of fb_notifier_call_chain are.
> >
> > It might need some console layer tweaking to provide 'register console
> > and I already hold the locks' or similar but that notifier needs some
> > kind of sanity applying as well.
> >
> > Cc'ing the fbdev folks
I've cc'd the framebuffer folks. I can see why its occurring but I have
no idea how they intend to fix it and I've not seen any replies.
Sorry but I've got enough other things on my plate right now without
trying to deal with the locking brain damage that the fbdev layer is.
As far as I can tell the actual bug proper is years old.
Alan
^ permalink raw reply
* Re: tty, vt: lockdep warnings
From: Sasha Levin @ 2012-11-05 17:26 UTC (permalink / raw)
To: Alan Cox
Cc: Sasha Levin, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org, Dave Jones, linux-fbdev,
florianSchandinat
In-Reply-To: <20121026143754.50277bd8@pyramind.ukuu.org.uk>
Ping? Should I bisect it?
On Fri, Oct 26, 2012 at 9:37 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Thu, 25 Oct 2012 15:37:43 -0400
> Sasha Levin <sasha.levin@oracle.com> wrote:
>
>> Hi all,
>>
>> While fuzzing with trinity inside a KVM tools (lkvm) guest running latest -next kernel,
>> I've stumbled on the following spew:
>
> Looks real enough but its not a tty/vt layer spew. This is all coming out
> of the core framebuffer code which doesn't seem to be able to decide what
> the locking rules at the invocation of fb_notifier_call_chain are.
>
> It might need some console layer tweaking to provide 'register console
> and I already hold the locks' or similar but that notifier needs some
> kind of sanity applying as well.
>
> Cc'ing the fbdev folks
>
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available
From: Rob Clark @ 2012-11-05 14:21 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, linux-omap, linux-fbdev, dri-devel
In-Reply-To: <50977EEF.3040403@ti.com>
On 11/05/2012 02:55 AM, Tomi Valkeinen wrote:
>>> But even then, choosing the manager is not easy, as whoever chooses the
>>> >>manager needs to observe all the possible displays used at the same
>>> >>time...
>> >
>> >Right. I was wondering if omapfb/omapdrm could understand the 'all
>> >possible displays information' better compared to a panel's probe.
>> >
>> >Even omapdrm/omafb can't be perfect because we could insert a panel
>> >driver module at any time, and omapfb/omapdrm may miss that out.
> True, omapdrm/fb may have a better idea. It's still unclear though.
> Currently we have quite strict order in the sequence the modules need to
> be loaded, which is quite bad and causes issues. We should make things
> more dynamic, so that the initialization of the drivers could happen
> more freely.
>
> But that creates more problems: when booting up, omapfb starts. But
> omapfb can't know if all the panel drivers have already been loaded.
> omapfb may see that DVI is the default display, but what should it do if
> DVI doesn't have a driver yet? It could wait, but perhaps the driver for
> DVI will never even be loaded.
The encoder which is connected to the crtc (manager) is picked by
combination of encoder->possible_crtcs bitmask and
connector->best_encoder(). We could keep things limited so that the
association of crtc to encoder (manager to output, roughly) never
changes, but this isn't really the right thing to do. It is better that
the dssdev not rely on knowing the manager it is attached to at probe
time, but instead grab resources more dynamically.
Also, at the moment we don't really have any notification to userspace
about new encoders/connectors showing up (or conversely, being
removed). Only about existing connectors being plugged/unplugged. The
closest analogy is perhaps the USB display devices, but even there it is
only the entire drm device that is plugged/unplugged. And TBH I don't
really see the point in supporting panel drivers being dynamically
loaded. It isn't like someone is dynamically soldering on a new display
connector to some board that is running. I think omapfb or omapdrm
probe should trigger registering the compiled-in panel drivers, so that
it can be sure that the dssdev's pop up before it goes and creates drm
connector objects. Currently we have to hack around this in omapdrm
with late_initcall() to ensure the panel drivers are probed first, but
that is an ugly hack that I'd like to get rid of.
BR,
-R
^ permalink raw reply
* Only for test purpose
From: Stefan Koch @ 2012-11-05 13:00 UTC (permalink / raw)
To: linux-fbdev
This is a test mail. Because I cannot see my mails to this mailing list
in marc.info archive, but messages from other people that have send
their messages later I can see.
^ permalink raw reply
* [GIT PULL] omapdss fixes for 3.7-rc
From: Tomi Valkeinen @ 2012-11-05 9:52 UTC (permalink / raw)
To: Florian Tobias Schandinat, linux-omap@vger.kernel.org,
linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]
Hi Florian,
Here are a few omapdss fixes, two fixing crasher bugs and the third
fixes a missing unlock. These are based on 3.7-rc1. I can rebase on
top of something else if you prefer that.
Tomi
The following changes since commit ddffeb8c4d0331609ef2581d84de4d763607bd37:
Linux 3.7-rc1 (2012-10-14 14:41:04 -0700)
are available in the git repository at:
git://gitorious.org/linux-omap-dss2/linux.git tags/omapdss-for-3.7-rc5
for you to fetch changes up to dffc70ade1d13edd186f542718c4d78a31d92fb8:
OMAPDSS: HDMI: fix missing unlock on error in hdmi_dump_regs() (2012-10-26 08:45:43 +0300)
----------------------------------------------------------------
omapdss fixes for 3.7-rc5
omapdss fixes for two crashes bugs and one missing mutex unlock.
----------------------------------------------------------------
Laurent Pinchart (1):
omapdss: dss: Fix clocks on OMAP363x
Tomi Valkeinen (1):
OMAPDSS: DSI: fix dsi_get_dsidev_from_id()
Wei Yongjun (1):
OMAPDSS: HDMI: fix missing unlock on error in hdmi_dump_regs()
drivers/video/omap2/dss/dsi.c | 13 +++++++++++--
drivers/video/omap2/dss/dss.c | 4 ++--
drivers/video/omap2/dss/hdmi.c | 4 +++-
3 files changed, 16 insertions(+), 5 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCH v7 8/8] drm_modes: add of_videomode helpers
From: Thierry Reding @ 2012-11-05 9:10 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Guennady Liakhovetski, linux-media,
Tomi Valkeinen, Stephen Warren, kernel
In-Reply-To: <1351675689-26814-9-git-send-email-s.trumtrar@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 787 bytes --]
On Wed, Oct 31, 2012 at 10:28:08AM +0100, Steffen Trumtrar wrote:
[...]
> +/**
> + * of_get_drm_display_mode - get a drm_display_mode from devicetree
> + * @np: device_node with the timing specification
> + * @dmode: will be set to the return value
> + * @index: index into the list of display timings in devicetree
> + *
> + * DESCRIPTION:
I don't think this is necessary.
> + * This function is expensive and should only be used, if only one mode is to be
> + * read from DT. To get multiple modes start with of_get_display_timing_list ond
You probably meant "and" at the end of this line. Also I'm not even sure
that we should be exposing this function, but rather provide a helper
which automatically adds the parsed modes to a DRM connector object.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available
From: Tomi Valkeinen @ 2012-11-05 8:55 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev, rob
In-Reply-To: <5093B4F3.1000703@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]
On 2012-11-02 13:56, Archit Taneja wrote:
> On Friday 02 November 2012 04:58 PM, Tomi Valkeinen wrote:
>> On 2012-11-02 13:09, Archit Taneja wrote:
>>> On Friday 02 November 2012 04:19 PM, Tomi Valkeinen wrote:
>>>> On 2012-11-02 12:44, Archit Taneja wrote:
>>>>
>>>>> Hmm, that makes sense. Anyway, I don't think it's really bad if we
>>>>> refer
>>>>> to dssdev->channel for now.
>>>>
>>>> It is, because dssdev->channel doesn't exist with DT.
>>>>
>>>> With DT we either need to figure out the channel in omapdss at runtime,
>>>> or add a property to the DT data telling the channel. And adding such a
>>>> property is not correct, as DT should be about describing the HW.
>>>
>>> Ok.
>>>
>>> I don't totally agree with your idea of figuring out the manager in
>>> panel the panel's probe. If it's done in the panel driver's probe
>>> itself, then by this point of time we have already set
>>> mgr->output->device links. If omapdss only does this stuff, then
>>
>> Hmm, I'm not sure I understand what's your point above? If figuring out
>> the mgr is done in panel's probe, the mgr->output link is not yet made
>> before that time.
>
> My point is that we are trying to find a manager at panel's probe
> itself. It think that's what we do now. But one of your recent patch
> moves that to omapfb.
Ah. Yes, that's true.
>>> omapfb/omapdrm have just the job of connecting the overlays to the
>>> manager. Do you think that's okay?
>>
>> Yes, that's how I think it should be. I don't see why omapfb/omapdrm
>> should care about which manager is being used for the output, it doesn't
>> really matter as long there is one and it works.
>>
>> Then again, I don't have anything against omapfb/omapdrm choosing the
>> manager, but I don't see how they would have any better idea of which
>> manager to use than omapdss.
>>
>> But as doing the connections at probe time is a bit problematic, perhaps
>> we should have a new step in this whole sequence. Something like
>> "connect" or whatever, which would lock the required blocks in the whole
>> pipeline, and acquire the required resources that couldn't be gotten at
>> probe time.
>>
>> But even then, choosing the manager is not easy, as whoever chooses the
>> manager needs to observe all the possible displays used at the same
>> time...
>
> Right. I was wondering if omapfb/omapdrm could understand the 'all
> possible displays information' better compared to a panel's probe.
>
> Even omapdrm/omafb can't be perfect because we could insert a panel
> driver module at any time, and omapfb/omapdrm may miss that out.
True, omapdrm/fb may have a better idea. It's still unclear though.
Currently we have quite strict order in the sequence the modules need to
be loaded, which is quite bad and causes issues. We should make things
more dynamic, so that the initialization of the drivers could happen
more freely.
But that creates more problems: when booting up, omapfb starts. But
omapfb can't know if all the panel drivers have already been loaded.
omapfb may see that DVI is the default display, but what should it do if
DVI doesn't have a driver yet? It could wait, but perhaps the driver for
DVI will never even be loaded.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* [PATCH V2] video: exynos_dp: Fix incorrect setting for INT_CTL
From: Ajay Kumar @ 2012-11-05 7:54 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <002901cdbb28$82ecb7d0$88c62770$%han@samsung.com>
INT_CTL register contains bits INT_POL0 and INT_POL1, and not INT_POL.
This patch fixes the wrong register setting for INT_CTL.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
drivers/video/exynos/exynos_dp_reg.c | 2 +-
drivers/video/exynos/exynos_dp_reg.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index 3f5ca8a..d67f49b 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -88,7 +88,7 @@ void exynos_dp_init_analog_param(struct exynos_dp_device *dp)
void exynos_dp_init_interrupt(struct exynos_dp_device *dp)
{
/* Set interrupt pin assertion polarity as high */
- writel(INT_POL, dp->reg_base + EXYNOS_DP_INT_CTL);
+ writel(INT_POL0 | INT_POL1, dp->reg_base + EXYNOS_DP_INT_CTL);
/* Clear pending regisers */
writel(0xff, dp->reg_base + EXYNOS_DP_COMMON_INT_STA_1);
diff --git a/drivers/video/exynos/exynos_dp_reg.h b/drivers/video/exynos/exynos_dp_reg.h
index 1f2f014..8548b91 100644
--- a/drivers/video/exynos/exynos_dp_reg.h
+++ b/drivers/video/exynos/exynos_dp_reg.h
@@ -242,7 +242,8 @@
/* EXYNOS_DP_INT_CTL */
#define SOFT_INT_CTRL (0x1 << 2)
-#define INT_POL (0x1 << 0)
+#define INT_POL0 (0x1 << 0)
+#define INT_POL1 (0x1 << 1)
/* EXYNOS_DP_SYS_CTL_1 */
#define DET_STA (0x1 << 2)
--
1.7.0.4
^ permalink raw reply related
* [PATCH V2] video: exynos_dp: Reset and initialize DP before requesting irq
From: Ajay Kumar @ 2012-11-05 7:54 UTC (permalink / raw)
To: linux-fbdev
If DP is not reset properly before kernel bootup(in bootloader code),
there can be few pending interrupts, and sometimes they invoke
DP irq handler as soon as the irq handler is registered in DP probe.
So, we make the DP driver more robust by resetting and
initializing DP at the earliest and then registering the irq handler.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/exynos/exynos_dp_core.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index d55470e..d241fd3 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -901,6 +901,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return -ENODEV;
}
+ dp->video_info = pdata->video_info;
+ if (pdata->phy_init)
+ pdata->phy_init();
+
+ exynos_dp_init_dp(dp);
+
ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
"exynos-dp", dp);
if (ret) {
@@ -908,12 +914,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return ret;
}
- dp->video_info = pdata->video_info;
- if (pdata->phy_init)
- pdata->phy_init();
-
- exynos_dp_init_dp(dp);
-
ret = exynos_dp_detect_hpd(dp);
if (ret) {
dev_err(&pdev->dev, "unable to detect hpd\n");
--
1.7.0.4
^ permalink raw reply related
* [PATCH v3 2/2] video: exynos_dp: Fix incorrect setting for INT_CTL
From: Jingoo Han @ 2012-11-05 7:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <003401cdbb29$60b46220$221d2660$%han@samsung.com>
From: Ajay Kumar <ajaykumar.rs@samsung.com>
INT_CTL register contains bits INT_POL0 and INT_POL1, and not INT_POL.
This patch fixes the wrong register setting for INT_CTL.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
Added 'From: Ajay Kumar <ajaykumar.rs@samsung.com>'
drivers/video/exynos/exynos_dp_reg.c | 2 +-
drivers/video/exynos/exynos_dp_reg.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index 3f5ca8a..cc7765f 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -88,7 +88,7 @@ void exynos_dp_init_analog_param(struct exynos_dp_device *dp)
void exynos_dp_init_interrupt(struct exynos_dp_device *dp)
{
/* Set interrupt pin assertion polarity as high */
- writel(INT_POL, dp->reg_base + EXYNOS_DP_INT_CTL);
+ writel(INT_POL1 | INT_POL0, dp->reg_base + EXYNOS_DP_INT_CTL);
/* Clear pending regisers */
writel(0xff, dp->reg_base + EXYNOS_DP_COMMON_INT_STA_1);
diff --git a/drivers/video/exynos/exynos_dp_reg.h b/drivers/video/exynos/exynos_dp_reg.h
index 1f2f014..2e9bd0e 100644
--- a/drivers/video/exynos/exynos_dp_reg.h
+++ b/drivers/video/exynos/exynos_dp_reg.h
@@ -242,7 +242,8 @@
/* EXYNOS_DP_INT_CTL */
#define SOFT_INT_CTRL (0x1 << 2)
-#define INT_POL (0x1 << 0)
+#define INT_POL1 (0x1 << 1)
+#define INT_POL0 (0x1 << 0)
/* EXYNOS_DP_SYS_CTL_1 */
#define DET_STA (0x1 << 2)
--
1.7.1
^ permalink raw reply related
* [PATCH v3 1/2] video: exynos_dp: Reset and initialize DP before requesting irq
From: Jingoo Han @ 2012-11-05 7:51 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <003301cdbb29$421e2ee0$c65a8ca0$%han@samsung.com>
From: Ajay Kumar <ajaykumar.rs@samsung.com>
If DP is not reset properly before kernel bootup(in bootloader code),
there can be few pending interrupts, and sometimes they invoke
DP irq handler as soon as the irq handler is registered in DP probe.
So, we make the DP driver more robust by resetting and
initializing DP at the earliest and then registering the irq handler.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
Added 'From: Ajay Kumar <ajaykumar.rs@samsung.com>'
drivers/video/exynos/exynos_dp_core.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index d55470e..d241fd3 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -901,6 +901,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return -ENODEV;
}
+ dp->video_info = pdata->video_info;
+ if (pdata->phy_init)
+ pdata->phy_init();
+
+ exynos_dp_init_dp(dp);
+
ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
"exynos-dp", dp);
if (ret) {
@@ -908,12 +914,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return ret;
}
- dp->video_info = pdata->video_info;
- if (pdata->phy_init)
- pdata->phy_init();
-
- exynos_dp_init_dp(dp);
-
ret = exynos_dp_detect_hpd(dp);
if (ret) {
dev_err(&pdev->dev, "unable to detect hpd\n");
--
1.7.1
^ permalink raw reply related
* [PATCH v3 2/2] video: exynos_dp: Fix incorrect setting for INT_CTL
From: Jingoo Han @ 2012-11-05 7:44 UTC (permalink / raw)
To: linux-fbdev
INT_CTL register contains bits INT_POL0 and INT_POL1, and not INT_POL.
This patch fixes the wrong register setting for INT_CTL.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
Tested with Exynos5250
drivers/video/exynos/exynos_dp_reg.c | 2 +-
drivers/video/exynos/exynos_dp_reg.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index 3f5ca8a..cc7765f 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -88,7 +88,7 @@ void exynos_dp_init_analog_param(struct exynos_dp_device *dp)
void exynos_dp_init_interrupt(struct exynos_dp_device *dp)
{
/* Set interrupt pin assertion polarity as high */
- writel(INT_POL, dp->reg_base + EXYNOS_DP_INT_CTL);
+ writel(INT_POL1 | INT_POL0, dp->reg_base + EXYNOS_DP_INT_CTL);
/* Clear pending regisers */
writel(0xff, dp->reg_base + EXYNOS_DP_COMMON_INT_STA_1);
diff --git a/drivers/video/exynos/exynos_dp_reg.h b/drivers/video/exynos/exynos_dp_reg.h
index 1f2f014..2e9bd0e 100644
--- a/drivers/video/exynos/exynos_dp_reg.h
+++ b/drivers/video/exynos/exynos_dp_reg.h
@@ -242,7 +242,8 @@
/* EXYNOS_DP_INT_CTL */
#define SOFT_INT_CTRL (0x1 << 2)
-#define INT_POL (0x1 << 0)
+#define INT_POL1 (0x1 << 1)
+#define INT_POL0 (0x1 << 0)
/* EXYNOS_DP_SYS_CTL_1 */
#define DET_STA (0x1 << 2)
--
1.7.1
^ permalink raw reply related
* [PATCH v3 1/2] video: exynos_dp: Reset and initialize DP before requesting irq
From: Jingoo Han @ 2012-11-05 7:43 UTC (permalink / raw)
To: linux-fbdev
If DP is not reset properly before kernel bootup(in bootloader code),
there can be few pending interrupts, and sometimes they invoke
DP irq handler as soon as the irq handler is registered in DP probe.
So, we make the DP driver more robust by resetting and
initializing DP at the earliest and then registering the irq handler.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
Tested with Exynos5250
drivers/video/exynos/exynos_dp_core.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index d55470e..d241fd3 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -901,6 +901,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return -ENODEV;
}
+ dp->video_info = pdata->video_info;
+ if (pdata->phy_init)
+ pdata->phy_init();
+
+ exynos_dp_init_dp(dp);
+
ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
"exynos-dp", dp);
if (ret) {
@@ -908,12 +914,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return ret;
}
- dp->video_info = pdata->video_info;
- if (pdata->phy_init)
- pdata->phy_init();
-
- exynos_dp_init_dp(dp);
-
ret = exynos_dp_detect_hpd(dp);
if (ret) {
dev_err(&pdev->dev, "unable to detect hpd\n");
--
1.7.1
^ permalink raw reply related
* Re: [PATCH V2] video: exynos_dp: Fix incorrect setting for INT_CTL
From: Jingoo Han @ 2012-11-05 7:38 UTC (permalink / raw)
To: linux-fbdev
On Monday, November 05, 2012 4:43 PM Ajay Kumar wrote
>
> INT_CTL register contains bits INT_POL0 and INT_POL1, and not INT_POL.
> This patch fixes the wrong register setting for INT_CTL.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
> drivers/video/exynos/exynos_dp_reg.c | 2 +-
> drivers/video/exynos/exynos_dp_reg.h | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index 3f5ca8a..d67f49b 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -88,7 +88,7 @@ void exynos_dp_init_analog_param(struct exynos_dp_device *dp)
> void exynos_dp_init_interrupt(struct exynos_dp_device *dp)
> {
> /* Set interrupt pin assertion polarity as high */
> - writel(INT_POL, dp->reg_base + EXYNOS_DP_INT_CTL);
> + writel(INT_POL0 | INT_POL1, dp->reg_base + EXYNOS_DP_INT_CTL);
>
> /* Clear pending regisers */
> writel(0xff, dp->reg_base + EXYNOS_DP_COMMON_INT_STA_1);
> diff --git a/drivers/video/exynos/exynos_dp_reg.h b/drivers/video/exynos/exynos_dp_reg.h
> index 1f2f014..8548b91 100644
> --- a/drivers/video/exynos/exynos_dp_reg.h
> +++ b/drivers/video/exynos/exynos_dp_reg.h
> @@ -242,7 +242,8 @@
>
> /* EXYNOS_DP_INT_CTL */
> #define SOFT_INT_CTRL (0x1 << 2)
> -#define INT_POL (0x1 << 0)
> +#define INT_POL0 (0x1 << 0)
> +#define INT_POL1 (0x1 << 1)
Please keep the bit order in descending order, for readability.
It is not big deal, so I will send the v3 patch, soon.
>
> /* EXYNOS_DP_SYS_CTL_1 */
> #define DET_STA (0x1 << 2)
> --
> 1.7.0.4
^ permalink raw reply
* Re: [PATCH] pwm-backlight: Pinctrl-fy
From: Thierry Reding @ 2012-11-05 7:33 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Richard Purdie, Florian Tobias Schandinat, linux-fbdev,
linux-kernel, Koen Kooi, Matt Porter, Russ Dill, linux-omap
In-Reply-To: <1351699047-4487-1-git-send-email-panto@antoniou-consulting.com>
[-- Attachment #1: Type: text/plain, Size: 387 bytes --]
On Wed, Oct 31, 2012 at 05:57:27PM +0200, Pantelis Antoniou wrote:
[...]
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
[...]
> @@ -20,6 +20,8 @@
> #include <linux/pwm.h>
> #include <linux/pwm_backlight.h>
> #include <linux/slab.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/err.h>
linux/err.h is already included earlier.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: video: exynos_dp: Reset and initialize DP before requesting irq
From: Jingoo Han @ 2012-11-05 2:47 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1351926402-25484-1-git-send-email-ajaykumar.rs@samsung.com>
On Saturday, November 03, 2012 4:07 PM Ajay Kumar wrote
> Subject: video: exynos_dp: Reset and initialize DP before requesting irq
Please don't forget to add "[PATCH] " as below.
[PATCH] video: exynos_dp: Reset and initialize DP before requesting irq
>
> If DP is not reset properly before kernel bootup(in bootloader code),
> there can be few pending interrupts, and sometimes they invoke
> DP irq handler as soon as the irq handler is registered in DP probe.
> So, we make the DP driver more robust by resetting and
> initializing DP at the earliest and then registering the irq handler.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Tested with Exynos5250.
> ---
> drivers/video/exynos/exynos_dp_core.c | 12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index d55470e..d241fd3 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -901,6 +901,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> + dp->video_info = pdata->video_info;
> + if (pdata->phy_init)
> + pdata->phy_init();
> +
> + exynos_dp_init_dp(dp);
> +
> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> "exynos-dp", dp);
> if (ret) {
> @@ -908,12 +914,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> return ret;
> }
>
> - dp->video_info = pdata->video_info;
> - if (pdata->phy_init)
> - pdata->phy_init();
> -
> - exynos_dp_init_dp(dp);
> -
> ret = exynos_dp_detect_hpd(dp);
> if (ret) {
> dev_err(&pdev->dev, "unable to detect hpd\n");
> --
> 1.7.0.4
^ permalink raw reply
* Re: video: exynos_dp: Fix incorrect setting for INT_CTL
From: Jingoo Han @ 2012-11-05 2:28 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1351926402-25484-2-git-send-email-ajaykumar.rs@samsung.com>
On Saturday, November 03, 2012 4:07 PM Ajay Kumar wrote
> Subject: video: exynos_dp: Fix incorrect setting for INT_CTL
Please don't forget to add "[PATCH] " as below.
[PATCH] video: exynos_dp: Fix incorrect setting for INT_CTL
>
> INT_CTL register contains bits INT_POL0 and INT_POL1, and not INT_POL.
> This patch fixes the wrong register setting for INT_CTL.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
> drivers/video/exynos/exynos_dp_reg.c | 2 +-
> drivers/video/exynos/exynos_dp_reg.h | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index 3f5ca8a..d67f49b 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -88,7 +88,7 @@ void exynos_dp_init_analog_param(struct exynos_dp_device *dp)
> void exynos_dp_init_interrupt(struct exynos_dp_device *dp)
> {
> /* Set interrupt pin assertion polarity as high */
> - writel(INT_POL, dp->reg_base + EXYNOS_DP_INT_CTL);
> + writel(INT_POL0 | INT_POL1, dp->reg_base + EXYNOS_DP_INT_CTL);
>
> /* Clear pending regisers */
> writel(0xff, dp->reg_base + EXYNOS_DP_COMMON_INT_STA_1);
> diff --git a/drivers/video/exynos/exynos_dp_reg.h b/drivers/video/exynos/exynos_dp_reg.h
> index 1f2f014..fcf386e 100644
> --- a/drivers/video/exynos/exynos_dp_reg.h
> +++ b/drivers/video/exynos/exynos_dp_reg.h
> @@ -242,7 +242,8 @@
>
> /* EXYNOS_DP_INT_CTL */
> #define SOFT_INT_CTRL (0x1 << 2)
> -#define INT_POL (0x1 << 0)
> +#define INT_POL0 (0x1 << 0)
> +#define INT_POL1 (0x1 << 0)
Why are the bit definitions the same???
INT_POL1 uses 1st bit of DP_INT_CTL register.
Please fix this bug as below.
#define INT_POL1 (0x1 << 1)
#define INT_POL0 (0x1 << 0)
Ajay,
please don't make careless mistake.
I don't want to waste my time.
>
> /* EXYNOS_DP_SYS_CTL_1 */
> #define DET_STA (0x1 << 2)
> --
> 1.7.0.4
^ permalink raw reply
* Re: [PATCH v7 2/8] of: add helper to parse display timings
From: Steffen Trumtrar @ 2012-11-04 17:23 UTC (permalink / raw)
To: Leela Krishna Amudala
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Tomi Valkeinen, Stephen Warren, kernel
In-Reply-To: <CAL1wa8dGiS-UDk7vjPKBaxQHB2FyNgLRYr5jsJZe4GjdzHELLQ@mail.gmail.com>
Hi!
On Fri, Nov 02, 2012 at 10:49:47PM +0530, Leela Krishna Amudala wrote:
> Hello Steffen,
>
> On Wed, Oct 31, 2012 at 2:58 PM, Steffen Trumtrar
> > +static int parse_property(struct device_node *np, char *name,
> > + struct timing_entry *result)
> > +{
> > + struct property *prop;
> > + int length;
> > + int cells;
> > + int ret;
> > +
> > + prop = of_find_property(np, name, &length);
> > + if (!prop) {
> > + pr_err("%s: could not find property %s\n", __func__, name);
> > + return -EINVAL;
> > + }
> > +
> > + cells = length / sizeof(u32);
> > + if (cells = 1) {
> > + ret = of_property_read_u32_array(np, name, &result->typ, cells);
>
> As you are reading only one vaue, you can use of_property_read_u32 instead.
>
Yes, thats copypasta, no need for _array here.
> > + result->min = result->typ;
> > + result->max = result->typ;
> > + } else if (cells = 3) {
> > + ret = of_property_read_u32_array(np, name, &result->min, cells);
>
> You are considering only min element, what about typ and max elements?
>
I start at the address of result->min and read three u32-values, therefore all
three (min,typ,max) are filled with values.
> > +
> > +/**
> > + * of_display_timings_exists - check if a display-timings node is provided
> > + * @np: device_node with the timing
> > + **/
> > +int of_display_timings_exists(struct device_node *np)
> > +{
> > + struct device_node *timings_np;
> > + struct device_node *default_np;
> > +
> > + if (!np)
> > + return -EINVAL;
> > +
> > + timings_np = of_parse_phandle(np, "display-timings", 0);
> > + if (!timings_np)
> > + return -EINVAL;
> > +
> > + return -EINVAL;
>
> Here it should return success instead of -EINVAL.
>
Yes.
> And one query.. are the binding properties names and "display-timings"
> node structure template finalized..?
>
I sure hope so. There actually is one error in the examples though.
The property clock is called clock-frequency. I included it correctly
at the top of display-timings.txt, but overlooked it in the examples.
Regards,
Steffen
--
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: [PATCH v7 2/8] of: add helper to parse display timings
From: Steffen Trumtrar @ 2012-11-04 17:10 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Tomi Valkeinen,
Laurent Pinchart, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
Guennady Liakhovetski, linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20121101201510.GB13137-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
On Thu, Nov 01, 2012 at 09:15:10PM +0100, Thierry Reding wrote:
> On Wed, Oct 31, 2012 at 10:28:02AM +0100, Steffen Trumtrar wrote:
> [...]
> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> [...]
> > @@ -0,0 +1,139 @@
> > +display-timings bindings
> > +=========
> > +
> > +display-timings-node
> > +------------
>
> Maybe extend the underline to the length of the section and subsection
> titles respectively?
>
> > +struct display_timing
> > +=========>
> Same here.
>
> > +config OF_DISPLAY_TIMINGS
> > + def_bool y
> > + depends on DISPLAY_TIMING
>
> Maybe this should be called OF_DISPLAY_TIMING to match DISPLAY_TIMING,
> or rename DISPLAY_TIMING to DISPLAY_TIMINGS for the sake of consistency?
>
Yes, to all three above.
> > +/**
> > + * of_get_display_timing_list - parse all display_timing entries from a device_node
> > + * @np: device_node with the subnodes
> > + **/
> > +struct display_timings *of_get_display_timing_list(struct device_node *np)
>
> Perhaps this would better be named of_get_display_timings() to match the
> return type?
>
Hm, I'm not really sure about that. I found it to error prone, to have a function
of_get_display_timing and of_get_display_timings. That's why I chose
of_get_display_timing_list. But you are correct, that it doesn't match the return
value. Maybe I should just make the first function static and change the name as you
suggested.
> > + disp = kzalloc(sizeof(*disp), GFP_KERNEL);
>
> Shouldn't you be checking this for allocation failures?
>
> > + disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
> > + GFP_KERNEL);
>
> Same here.
>
Yes, to both.
Regards,
Steffen
--
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: [PATCH v7 1/8] video: add display_timing struct and helpers
From: Steffen Trumtrar @ 2012-11-04 16:53 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-fbdev, devicetree-discuss, dri-devel, Tomi Valkeinen,
Laurent Pinchart, kernel, Guennady Liakhovetski, linux-media
In-Reply-To: <20121101200842.GA13137@avionic-0098.mockup.avionic-design.de>
On Thu, Nov 01, 2012 at 09:08:42PM +0100, Thierry Reding wrote:
> On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote:
> [...]
> > +void timings_release(struct display_timings *disp)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < disp->num_timings; i++)
> > + kfree(disp->timings[i]);
> > +}
> > +
> > +void display_timings_release(struct display_timings *disp)
> > +{
> > + timings_release(disp);
> > + kfree(disp->timings);
> > +}
>
> I'm not quite sure I understand how these are supposed to be used. The
> only use-case where a struct display_timings is dynamically allocated is
> for the OF helpers. In that case, wouldn't it be more useful to have a
> function that frees the complete structure, including the struct
> display_timings itself? Something like this, which has all of the above
> rolled into one:
>
> void display_timings_free(struct display_timings *disp)
> {
> if (disp->timings) {
> unsigned int i;
>
> for (i = 0; i < disp->num_timings; i++)
> kfree(disp->timings[i]);
> }
>
> kfree(disp->timings);
> kfree(disp);
> }
>
Well, you are right. They can be rolled into one function.
The extra function call is useless and as it seems confusing.
Regards,
Steffen
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
--
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
* video: exynos_dp: Fix incorrect setting for INT_CTL
From: Ajay Kumar @ 2012-11-03 6:47 UTC (permalink / raw)
To: linux-fbdev
INT_CTL register contains bits INT_POL0 and INT_POL1, and not INT_POL.
This patch fixes the wrong register setting for INT_CTL.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
drivers/video/exynos/exynos_dp_reg.c | 2 +-
drivers/video/exynos/exynos_dp_reg.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index 3f5ca8a..d67f49b 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -88,7 +88,7 @@ void exynos_dp_init_analog_param(struct exynos_dp_device *dp)
void exynos_dp_init_interrupt(struct exynos_dp_device *dp)
{
/* Set interrupt pin assertion polarity as high */
- writel(INT_POL, dp->reg_base + EXYNOS_DP_INT_CTL);
+ writel(INT_POL0 | INT_POL1, dp->reg_base + EXYNOS_DP_INT_CTL);
/* Clear pending regisers */
writel(0xff, dp->reg_base + EXYNOS_DP_COMMON_INT_STA_1);
diff --git a/drivers/video/exynos/exynos_dp_reg.h b/drivers/video/exynos/exynos_dp_reg.h
index 1f2f014..fcf386e 100644
--- a/drivers/video/exynos/exynos_dp_reg.h
+++ b/drivers/video/exynos/exynos_dp_reg.h
@@ -242,7 +242,8 @@
/* EXYNOS_DP_INT_CTL */
#define SOFT_INT_CTRL (0x1 << 2)
-#define INT_POL (0x1 << 0)
+#define INT_POL0 (0x1 << 0)
+#define INT_POL1 (0x1 << 0)
/* EXYNOS_DP_SYS_CTL_1 */
#define DET_STA (0x1 << 2)
--
1.7.0.4
^ permalink raw reply related
* video: exynos_dp: Reset and initialize DP before requesting irq
From: Ajay Kumar @ 2012-11-03 6:47 UTC (permalink / raw)
To: linux-fbdev
If DP is not reset properly before kernel bootup(in bootloader code),
there can be few pending interrupts, and sometimes they invoke
DP irq handler as soon as the irq handler is registered in DP probe.
So, we make the DP driver more robust by resetting and
initializing DP at the earliest and then registering the irq handler.
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
drivers/video/exynos/exynos_dp_core.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index d55470e..d241fd3 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -901,6 +901,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return -ENODEV;
}
+ dp->video_info = pdata->video_info;
+ if (pdata->phy_init)
+ pdata->phy_init();
+
+ exynos_dp_init_dp(dp);
+
ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
"exynos-dp", dp);
if (ret) {
@@ -908,12 +914,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
return ret;
}
- dp->video_info = pdata->video_info;
- if (pdata->phy_init)
- pdata->phy_init();
-
- exynos_dp_init_dp(dp);
-
ret = exynos_dp_detect_hpd(dp);
if (ret) {
dev_err(&pdev->dev, "unable to detect hpd\n");
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH v7 2/8] of: add helper to parse display timings
From: Leela Krishna Amudala @ 2012-11-02 17:31 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Tomi Valkeinen, Stephen Warren, kernel
In-Reply-To: <1351675689-26814-3-git-send-email-s.trumtrar@pengutronix.de>
Hello Steffen,
On Wed, Oct 31, 2012 at 2:58 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
> .../devicetree/bindings/video/display-timings.txt | 139 +++++++++++++++
> drivers/of/Kconfig | 6 +
> drivers/of/Makefile | 1 +
> drivers/of/of_display_timings.c | 185 ++++++++++++++++++++
> include/linux/of_display_timings.h | 20 +++
> 5 files changed, 351 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
> create mode 100644 drivers/of/of_display_timings.c
> create mode 100644 include/linux/of_display_timings.h
>
> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> new file mode 100644
> index 0000000..04c94a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,139 @@
> +display-timings bindings
> +=========
> +
> +display-timings-node
> +------------
> +
> +required properties:
> + - none
> +
> +optional properties:
> + - native-mode: the native mode for the display, in case multiple modes are
> + provided. When omitted, assume the first node is the native.
> +
> +timings-subnode
> +---------------
> +
> +required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> + in pixels
> + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> + lines
> + - clock-frequency: displayclock in Hz
> +
> +optional properties:
> + - hsync-active : Hsync pulse is active low/high/ignored
> + - vsync-active : Vsync pulse is active low/high/ignored
> + - de-active : Data-Enable pulse is active low/high/ignored
> + - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
> + - interlaced (bool)
> + - doublescan (bool)
> +
> +All the optional properties that are not bool follow the following logic:
> + <1> : high active
> + <0> : low active
> + omitted : not used on hardware
> +
> +There are different ways of describing the capabilities of a display. The devicetree
> +representation corresponds to the one commonly found in datasheets for displays.
> +If a display supports multiple signal timings, the native-mode can be specified.
> +
> +The parameters are defined as
> +
> +struct display_timing
> +=========> +
> + +----------+---------------------------------------------+----------+-------+
> + | | ↑ | | |
> + | | |vback_porch | | |
> + | | ↓ | | |
> + +----------###############################################----------+-------+
> + | # ↑ # | |
> + | # | # | |
> + | hback # | # hfront | hsync |
> + | porch # | hactive # porch | len |
> + |<-------->#<---------------+--------------------------->#<-------->|<----->|
> + | # | # | |
> + | # |vactive # | |
> + | # | # | |
> + | # ↓ # | |
> + +----------###############################################----------+-------+
> + | | ↑ | | |
> + | | |vfront_porch | | |
> + | | ↓ | | |
> + +----------+---------------------------------------------+----------+-------+
> + | | ↑ | | |
> + | | |vsync_len | | |
> + | | ↓ | | |
> + +----------+---------------------------------------------+----------+-------+
> +
> +
> +Example:
> +
> + display-timings {
> + native-mode = <&timing0>;
> + timing0: 1920p24 {
> + /* 1920x1080p24 */
> + clock = <52000000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hfront-porch = <25>;
> + hback-porch = <25>;
> + hsync-len = <25>;
> + vback-porch = <2>;
> + vfront-porch = <2>;
> + vsync-len = <2>;
> + hsync-active = <1>;
> + };
> + };
> +
> +Every required property also supports the use of ranges, so the commonly used
> +datasheet description with <min typ max>-tuples can be used.
> +
> +Example:
> +
> + timing1: timing {
> + /* 1920x1080p24 */
> + clock = <148500000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hsync-len = <0 44 60>;
> + hfront-porch = <80 88 95>;
> + hback-porch = <100 148 160>;
> + vfront-porch = <0 4 6>;
> + vback-porch = <0 36 50>;
> + vsync-len = <0 5 6>;
> + };
> +
> +
> +Usage in backend
> +========
> +
> +A backend driver may choose to use the display-timings directly and convert the timing
> +ranges to a suitable mode. Or it may just use the conversion of the display timings
> +to the required mode via the generic videomode struct.
> +
> + dtb
> + |
> + | of_get_display_timing_list
> + ↓
> + struct display_timings
> + |
> + | videomode_from_timing
> + ↓
> + --- struct videomode ---
> + | |
> + videomode_to_displaymode | | videomode_to_fb_videomode
> + ↓ ↓
> + drm_display_mode fb_videomode
> +
> +The functions of_get_fb_videomode and of_get_display_mode are provided
> +to conveniently get the respective mode representation from the devicetree.
> +
> +Conversion to videomode
> +===========> +
> +As device drivers normally work with some kind of video mode, the timings can be
> +converted (may be just a simple copying of the typical value) to a generic videomode
> +structure which then can be converted to the according mode used by the backend.
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..781e773 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,10 @@ config OF_MTD
> depends on MTD
> def_bool y
>
> +config OF_DISPLAY_TIMINGS
> + def_bool y
> + depends on DISPLAY_TIMING
> + help
> + helper to parse display timings from the devicetree
> +
> endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..c8e9603 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
> obj-$(CONFIG_OF_PCI) += of_pci.o
> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> obj-$(CONFIG_OF_MTD) += of_mtd.o
> +obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
> diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c
> new file mode 100644
> index 0000000..388fe4c
> --- /dev/null
> +++ b/drivers/of/of_display_timings.c
> @@ -0,0 +1,185 @@
> +/*
> + * OF helpers for parsing display timings
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> + *
> + * based on of_videomode.c by Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/of_display_timings.h>
> +
> +/**
> + * parse_property - parse timing_entry from device_node
> + * @np: device_node with the property
> + * @name: name of the property
> + * @result: will be set to the return value
> + *
> + * DESCRIPTION:
> + * Every display_timing can be specified with either just the typical value or
> + * a range consisting of min/typ/max. This function helps handling this
> + **/
> +static int parse_property(struct device_node *np, char *name,
> + struct timing_entry *result)
> +{
> + struct property *prop;
> + int length;
> + int cells;
> + int ret;
> +
> + prop = of_find_property(np, name, &length);
> + if (!prop) {
> + pr_err("%s: could not find property %s\n", __func__, name);
> + return -EINVAL;
> + }
> +
> + cells = length / sizeof(u32);
> + if (cells = 1) {
> + ret = of_property_read_u32_array(np, name, &result->typ, cells);
As you are reading only one vaue, you can use of_property_read_u32 instead.
> + result->min = result->typ;
> + result->max = result->typ;
> + } else if (cells = 3) {
> + ret = of_property_read_u32_array(np, name, &result->min, cells);
You are considering only min element, what about typ and max elements?
> + } else {
> + pr_err("%s: illegal timing specification in %s\n", __func__, name);
> + return -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * of_get_display_timing - parse display_timing entry from device_node
> + * @np: device_node with the properties
> + **/
> +struct display_timing *of_get_display_timing(struct device_node *np)
> +{
> + struct display_timing *dt;
> + int ret = 0;
> +
> + dt = kzalloc(sizeof(*dt), GFP_KERNEL);
> + if (!dt) {
> + pr_err("%s: could not allocate display_timing struct\n", __func__);
> + return NULL;
> + }
> +
> + ret |= parse_property(np, "hback-porch", &dt->hback_porch);
> + ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
> + ret |= parse_property(np, "hactive", &dt->hactive);
> + ret |= parse_property(np, "hsync-len", &dt->hsync_len);
> + ret |= parse_property(np, "vback-porch", &dt->vback_porch);
> + ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
> + ret |= parse_property(np, "vactive", &dt->vactive);
> + ret |= parse_property(np, "vsync-len", &dt->vsync_len);
> + ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
> +
> + of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
> + of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
> + of_property_read_u32(np, "de-active", &dt->de_pol_active);
> + of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
> + dt->interlaced = of_property_read_bool(np, "interlaced");
> + dt->doublescan = of_property_read_bool(np, "doublescan");
> +
> + if (ret) {
> + pr_err("%s: error reading timing properties\n", __func__);
> + return NULL;
> + }
> +
> + return dt;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_timing);
> +
> +/**
> + * of_get_display_timing_list - parse all display_timing entries from a device_node
> + * @np: device_node with the subnodes
> + **/
> +struct display_timings *of_get_display_timing_list(struct device_node *np)
> +{
> + struct device_node *timings_np;
> + struct device_node *entry;
> + struct device_node *native_mode;
> + struct display_timings *disp;
> +
> + if (!np) {
> + pr_err("%s: no devicenode given\n", __func__);
> + return NULL;
> + }
> +
> + timings_np = of_find_node_by_name(np, "display-timings");
> + if (!timings_np) {
> + pr_err("%s: could not find display-timings node\n", __func__);
> + return NULL;
> + }
> +
> + disp = kzalloc(sizeof(*disp), GFP_KERNEL);
> +
> + entry = of_parse_phandle(timings_np, "native-mode", 0);
> + /* assume first child as native mode if none provided */
> + if (!entry)
> + entry = of_get_next_child(np, NULL);
> + if (!entry) {
> + pr_err("%s: no timing specifications given\n", __func__);
> + return NULL;
> + }
> +
> + pr_info("%s: using %s as default timing\n", __func__, entry->name);
> +
> + native_mode = entry;
> +
> + disp->num_timings = of_get_child_count(timings_np);
> + disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
> + GFP_KERNEL);
> + disp->num_timings = 0;
> + disp->native_mode = 0;
> +
> + for_each_child_of_node(timings_np, entry) {
> + struct display_timing *dt;
> +
> + dt = of_get_display_timing(entry);
> + if (!dt) {
> + /* to not encourage wrong devicetrees, fail in case of an error */
> + pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
> + return NULL;
> + }
> +
> + if (native_mode = entry)
> + disp->native_mode = disp->num_timings;
> +
> + disp->timings[disp->num_timings] = dt;
> + disp->num_timings++;
> + }
> + of_node_put(timings_np);
> +
> + if (disp->num_timings > 0)
> + pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
> + disp->num_timings , disp->native_mode + 1);
> + else {
> + pr_err("%s: no valid timings specified\n", __func__);
> + return NULL;
> + }
> + return disp;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_timing_list);
> +
> +/**
> + * of_display_timings_exists - check if a display-timings node is provided
> + * @np: device_node with the timing
> + **/
> +int of_display_timings_exists(struct device_node *np)
> +{
> + struct device_node *timings_np;
> + struct device_node *default_np;
> +
> + if (!np)
> + return -EINVAL;
> +
> + timings_np = of_parse_phandle(np, "display-timings", 0);
> + if (!timings_np)
> + return -EINVAL;
> +
> + return -EINVAL;
Here it should return success instead of -EINVAL.
And one query.. are the binding properties names and "display-timings"
node structure template finalized..?
Best Wishes,
Leela Krishna Amudala.
> +}
> +EXPORT_SYMBOL_GPL(of_display_timings_exists);
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> new file mode 100644
> index 0000000..e4e1f22
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * display timings of helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H
> +
> +#include <linux/display_timing.h>
> +
> +#define OF_USE_NATIVE_MODE -1
> +
> +struct display_timings *of_get_display_timing_list(struct device_node *np);
> +struct display_timing *of_get_display_timing(struct device_node *np);
> +int of_display_timings_exists(struct device_node *np);
> +
> +#endif
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
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