* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Tomi Valkeinen @ 2012-06-25 13:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4FE85005.4090303@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3644 bytes --]
On Mon, 2012-06-25 at 17:18 +0530, Rajendra Nayak wrote:
> On Monday 25 June 2012 01:28 PM, Tomi Valkeinen wrote:
> > On Mon, 2012-06-25 at 12:29 +0530, Rajendra Nayak wrote:
> >> On Monday 25 June 2012 11:37 AM, Tomi Valkeinen wrote:
> >>> Hi,
> >>>
> >>> On Fri, 2012-06-22 at 19:18 +0530, Rajendra Nayak wrote:
> >>>> In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
> >>>> and clk_unprepare() for the omapdss clocks.
> >>>
> >>> You used clk_prepare and clk_unprepare instead of clk_prepare_enable and
> >>> clk_disable_unprepare. I didn't check the dss driver yet, but my hunch
> >>> is that the clocks are normally not enabled/disabled from atomic
> >>> context.
> >>>
> >>> What does the prepare/unprepare actually do? Is there any benefit in
> >>> delaying preparing, i.e. is there a difference between prepare right
> >>> after clk_get, or prepare right before clk_enable? (And similarly for
> >>> unprepare)
> >>
> >> clk_prepare/unprepare are useful for clocks which need the 'enable'
> >> logic to be implemented as a slow part (which can sleep) and a fast part
> >> (which does not sleep). For all the dss clocks in question we don't need
> >> a slow part and hence they do not have a .clk_prepare/unprepare
> >> platform hook.
> >>
> >> The framework however still does prepare usecounting (it has a prepare
> >> count and an enable count, and prepare count is expected to be non-zero
> >> while the clock is being enabled) and uses a mutex around to guard it.
> >> So while the dss driver would do multiple clk_enable/disable while its
> >> active, it seems fair to just prepare/unprepare the clocks once just
> >> after clk_get() and before clk_put() in this particular case.
> >
> > But the driver should not presume anything special about the clocks. In
> > this case the dss driver would presume that the clocks it uses do not
> > have prepare and unprepare hooks.
> >
> > If the generally proper way to use prepare/unprepare is in combination
> > of enable/disable, then I think we should try to do that.
>
> makes sense. Lets see if any of the clk_enable/disable happen in atomic
> context, if not it would be just a matter of replacing all with a
> clk_prepare_enable/disable_unprepare. Else we might have to find a safe
> place sometime before clk_enable to prepare the clk and after
> clk_disable to unprepare it.
>
> >
> > I'll check if any of the dss clocks are enabled or disabled in atomic
> > context.
venc and hdmi use clk_enable/disable in runtime PM callbacks (suspend &
resume). If I understand correctly, the callbacks are not called in
atomic context if pm_runtime_irq_safe() has not been used. And it is not
used in omapdss.
dsi uses clk_enable/disable in a different manner, but not in atomic
context.
So as far as I see, clocks are never handled in atomic context. Is
everything related to the base clk stuff already in mainline? Can I take
the clk_prepare/unprepare patch into my omapdss tree?
A question about clk_prepare/unprepare, not directly related: let's say
I have a driver for some HW block. The driver doesn't use clk functions,
but uses runtime PM. The driver also sets pm_runtime_irq_safe().
Now, the driver can call pm_runtime_get_sync() in an atomic context, and
this would lead to the underlying framework (hwmod, omap_device, I don't
know who =) enabling the func clock for that HW. But this would happen
in atomic context, so the underlying framework can't use clk_prepare.
How does the underlying framework handle that case? (sorry if that's a
stupid question =).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Rajendra Nayak @ 2012-06-25 12:54 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Grazvydas Ignotas, jaswinder.singh, mythripk, linux-omap,
linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340627459.3395.54.camel@deskari>
On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:
> On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:
>> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen<tomi.valkeinen@ti.com> wrote:
>>> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>>>>
>>>> Currenlty HDMI fails to come up in the suspend-resume path.
>>>> This patch helps that real-world scenario.
>>>
>>> What is the problem there? It'd be good to explain the problem in the
>>> patch description. Does the pm_runtime_get return -EACCES?
>>
>> On slightly different but related issue, currently OMAPDSS always
>> spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
>> because pm_runtime_put* always return -ENOSYS without
>> CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
>> maybe WARN_ON should check for -ENOSYS, I don't know..
>
> Hmm. I guess I'm missing some understanding about runtime PM. omapdss
> uses runtime PM to enable the underlying DSS hardware. If there's no
> runtime PM, how does the driver work? Or is it the job of
> hwmod/omap_device to keep all the hardware always enabled if runtime PM
> is not compiled in?
Yes, the below trick keeps all hwmods always enabled post the initial
setup if runtime PM is disabled.
from arch/arm/mach-omap2/io.c
static void __init omap_hwmod_init_postsetup(void)
{
u8 postsetup_state;
/* Set the default postsetup state for all hwmods */
#ifdef CONFIG_PM_RUNTIME
postsetup_state = _HWMOD_STATE_IDLE;
#else
postsetup_state = _HWMOD_STATE_ENABLED;
#endif
omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);
omap_pm_if_early_init();
}
>
> Tomi
>
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Tomi Valkeinen @ 2012-06-25 12:50 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Grazvydas Ignotas, jaswinder.singh, mythripk, linux-omap,
linux-fbdev, andy.green, n-dechesne
In-Reply-To: <4FE85CCA.80903@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]
On Mon, 2012-06-25 at 18:12 +0530, Rajendra Nayak wrote:
> On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:
> > On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:
> >> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen<tomi.valkeinen@ti.com> wrote:
> >>> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> >>>>
> >>>> Currenlty HDMI fails to come up in the suspend-resume path.
> >>>> This patch helps that real-world scenario.
> >>>
> >>> What is the problem there? It'd be good to explain the problem in the
> >>> patch description. Does the pm_runtime_get return -EACCES?
> >>
> >> On slightly different but related issue, currently OMAPDSS always
> >> spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
> >> because pm_runtime_put* always return -ENOSYS without
> >> CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
> >> maybe WARN_ON should check for -ENOSYS, I don't know..
> >
> > Hmm. I guess I'm missing some understanding about runtime PM. omapdss
> > uses runtime PM to enable the underlying DSS hardware. If there's no
> > runtime PM, how does the driver work? Or is it the job of
> > hwmod/omap_device to keep all the hardware always enabled if runtime PM
> > is not compiled in?
>
> Yes, the below trick keeps all hwmods always enabled post the initial
> setup if runtime PM is disabled.
>
> from arch/arm/mach-omap2/io.c
>
> static void __init omap_hwmod_init_postsetup(void)
> {
> u8 postsetup_state;
>
> /* Set the default postsetup state for all hwmods */
> #ifdef CONFIG_PM_RUNTIME
> postsetup_state = _HWMOD_STATE_IDLE;
> #else
> postsetup_state = _HWMOD_STATE_ENABLED;
> #endif
> omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);
>
> omap_pm_if_early_init();
> }
Ah, ok, thanks.
Do you know how the drivers should handle CONFIG_PM_RUNTIME=n?
Are they supposed to handle the error values returned by runtime PM
functions somehow, or should they use #ifdef CONFIG_PM_RUNTIME?
Both options sound a bit difficult to me... With the first one it's
difficult to see if there was an actual error and we should somehow
react to it, or is everything fine and we just shouldn't care about
runtime PM. The second one requires ifdefs in many places.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Jassi Brar @ 2012-06-25 12:45 UTC (permalink / raw)
To: Grazvydas Ignotas
Cc: Tomi Valkeinen, mythripk, linux-omap, linux-fbdev, andy.green,
n-dechesne
In-Reply-To: <CANOLnOOodXtqV7Hs-SsEUfB3Uo1PSGb8bkaeN7M48jVpQrb-cA@mail.gmail.com>
On 25 June 2012 17:35, Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>>>
>>> Currenlty HDMI fails to come up in the suspend-resume path.
>>> This patch helps that real-world scenario.
>>
>> What is the problem there? It'd be good to explain the problem in the
>> patch description. Does the pm_runtime_get return -EACCES?
>
> On slightly different but related issue, currently OMAPDSS always
> spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
> because pm_runtime_put* always return -ENOSYS without
> CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
> maybe WARN_ON should check for -ENOSYS, I don't know..
>
I didn't check, but this patch should already fix that I think ?
IMHO, for omapdss, we need not differentiate between -ENOSYS and
-EACCESS because anyway the ultimate functions dispc_runtime_resume()
and dispc_runtime_suspend() can't report failure (they always return
0).
-j
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Tomi Valkeinen @ 2012-06-25 12:41 UTC (permalink / raw)
To: Jassi Brar; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <CAJe_ZhftuNggxH+269-ZJbWJo5nSzSjo1dDCTQGMbsjPWoUpyg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]
On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote:
> On 25 June 2012 15:00, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > The driver needs to enable the HW and the call to pm_runtime_get() is
> > skipped. Won't this lead to crash as the DSS registers are accessed
> > without the HW in enabled state?
> >
> Hmm... how does the extant code in hdmi driver ensures DSS is up and running ?
> While it does sound important even to my limited knowledge of OMAPDSS,
> I see rpm of HDMI, VENC and RFBI only dependent on DISPC, not DSS.
DSS device is parent to all the DSS subdevices. So when a subdevice is
enabled, DSS device is enabled first.
But anyway, I wasn't referring to the DSS part of OMAPDSS, but to
omapdss generally. If we do this:
/* this is skipped, if runtime PM is disabled */
dispc_runtime_get();
/* this accesses a register, but the HW is disabled? */
dispc_read_reg(FOO);
So again, I don't understand how the underlying HW gets enabled. Or does
hwmod/omap_device code make sure that it's enabled while the board is
being resumed? If so, what would happen if we continue the above
scenario as follows:
/* this is skipped, if runtime PM is disabled */
dispc_runtime_get();
/* this accesses a register, the HW is kept enabled by hwmod */
dispc_read_reg(FOO);
/* at some time later the resume procedure ends, and hwmod doesn't keep
the HW enabled any more */
/* this accesses a register, the HW is disabled */
dispc_read_reg(FOO);
> And for DISPC these drivers already hold a reference in their display
> enable/resume and keep it until disable/suspend. So we don't lose
> DISPC anytime it is really required.
If all the displays are disabled, nobody keeps a reference to dispc.
> > And what happens if the pm_runtime_get() call is skipped, but pm_runtime_put() is not?
> >
> Not sure in what newly introduced scenario by this patch, because
> get/put both check for pm_enabled before proceeding. Am I overlooking
> something?
Currently (for example) dispc_runtime_get/put call
pm_runtime_get/put_sync. When somebody uses dispc_runtime_get, the same
somebody knows it needs to call dispc_runtime_put later.
Now, what happens if dispc_runtime_get is called when runtime PM is
disabled (i.e. pm_runtime_get_sync is skipped), but runtime PM is
enabled later when that somebody calls dispc_runtime_put (i.e.
pm_runtime_put_sync is _not_ skipped)?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Jassi Brar @ 2012-06-25 12:39 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340616643.3395.19.camel@deskari>
On 25 June 2012 15:00, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-25 at 14:19 +0530, Jassi Brar wrote:
>> On 25 June 2012 11:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>> ....
>> >> Currenlty HDMI fails to come up in the suspend-resume path.
>> >> This patch helps that real-world scenario.
>> >
>> > What is the problem there? It'd be good to explain the problem in the
>> > patch description. Does the pm_runtime_get return -EACCES?
>> >
>> Yes, it returns -EACCESS because RPM on devices is disabled during the
>> period from suspend-start to resume-finished.
>
> So... You didn't answer my first comment, how can the code work?
>
Sorry, don't know why I thought I didn't miss anything.
> The driver needs to enable the HW and the call to pm_runtime_get() is
> skipped. Won't this lead to crash as the DSS registers are accessed
> without the HW in enabled state?
>
Hmm... how does the extant code in hdmi driver ensures DSS is up and running ?
While it does sound important even to my limited knowledge of OMAPDSS,
I see rpm of HDMI, VENC and RFBI only dependent on DISPC, not DSS.
And for DISPC these drivers already hold a reference in their display
enable/resume and keep it until disable/suspend. So we don't lose
DISPC anytime it is really required.
> And what happens if the pm_runtime_get() call is skipped, but pm_runtime_put() is not?
>
Not sure in what newly introduced scenario by this patch, because
get/put both check for pm_enabled before proceeding. Am I overlooking
something?
thnx
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Tomi Valkeinen @ 2012-06-25 12:30 UTC (permalink / raw)
To: Grazvydas Ignotas
Cc: jaswinder.singh, mythripk, linux-omap, linux-fbdev, andy.green,
n-dechesne
In-Reply-To: <CANOLnOOodXtqV7Hs-SsEUfB3Uo1PSGb8bkaeN7M48jVpQrb-cA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]
On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:
> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> >>
> >> Currenlty HDMI fails to come up in the suspend-resume path.
> >> This patch helps that real-world scenario.
> >
> > What is the problem there? It'd be good to explain the problem in the
> > patch description. Does the pm_runtime_get return -EACCES?
>
> On slightly different but related issue, currently OMAPDSS always
> spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
> because pm_runtime_put* always return -ENOSYS without
> CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
> maybe WARN_ON should check for -ENOSYS, I don't know..
Hmm. I guess I'm missing some understanding about runtime PM. omapdss
uses runtime PM to enable the underlying DSS hardware. If there's no
runtime PM, how does the driver work? Or is it the job of
hwmod/omap_device to keep all the hardware always enabled if runtime PM
is not compiled in?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Grazvydas Ignotas @ 2012-06-25 12:05 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: jaswinder.singh, mythripk, linux-omap, linux-fbdev, andy.green,
n-dechesne
In-Reply-To: <1340605221.12683.30.camel@lappyti>
On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>>
>> Currenlty HDMI fails to come up in the suspend-resume path.
>> This patch helps that real-world scenario.
>
> What is the problem there? It'd be good to explain the problem in the
> patch description. Does the pm_runtime_get return -EACCES?
On slightly different but related issue, currently OMAPDSS always
spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
because pm_runtime_put* always return -ENOSYS without
CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
maybe WARN_ON should check for -ENOSYS, I don't know..
--
Gražvydas
^ permalink raw reply
* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Rajendra Nayak @ 2012-06-25 11:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340611133.3395.3.camel@deskari>
On Monday 25 June 2012 01:28 PM, Tomi Valkeinen wrote:
> On Mon, 2012-06-25 at 12:29 +0530, Rajendra Nayak wrote:
>> On Monday 25 June 2012 11:37 AM, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On Fri, 2012-06-22 at 19:18 +0530, Rajendra Nayak wrote:
>>>> In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
>>>> and clk_unprepare() for the omapdss clocks.
>>>
>>> You used clk_prepare and clk_unprepare instead of clk_prepare_enable and
>>> clk_disable_unprepare. I didn't check the dss driver yet, but my hunch
>>> is that the clocks are normally not enabled/disabled from atomic
>>> context.
>>>
>>> What does the prepare/unprepare actually do? Is there any benefit in
>>> delaying preparing, i.e. is there a difference between prepare right
>>> after clk_get, or prepare right before clk_enable? (And similarly for
>>> unprepare)
>>
>> clk_prepare/unprepare are useful for clocks which need the 'enable'
>> logic to be implemented as a slow part (which can sleep) and a fast part
>> (which does not sleep). For all the dss clocks in question we don't need
>> a slow part and hence they do not have a .clk_prepare/unprepare
>> platform hook.
>>
>> The framework however still does prepare usecounting (it has a prepare
>> count and an enable count, and prepare count is expected to be non-zero
>> while the clock is being enabled) and uses a mutex around to guard it.
>> So while the dss driver would do multiple clk_enable/disable while its
>> active, it seems fair to just prepare/unprepare the clocks once just
>> after clk_get() and before clk_put() in this particular case.
>
> But the driver should not presume anything special about the clocks. In
> this case the dss driver would presume that the clocks it uses do not
> have prepare and unprepare hooks.
>
> If the generally proper way to use prepare/unprepare is in combination
> of enable/disable, then I think we should try to do that.
makes sense. Lets see if any of the clk_enable/disable happen in atomic
context, if not it would be just a matter of replacing all with a
clk_prepare_enable/disable_unprepare. Else we might have to find a safe
place sometime before clk_enable to prepare the clk and after
clk_disable to unprepare it.
>
> I'll check if any of the dss clocks are enabled or disabled in atomic
> context.
>
> Tomi
>
^ permalink raw reply
* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Russell King - ARM Linux @ 2012-06-25 11:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340604478.12683.25.camel@lappyti>
On Mon, Jun 25, 2012 at 09:07:58AM +0300, Tomi Valkeinen wrote:
> On Fri, 2012-06-22 at 19:18 +0530, Rajendra Nayak wrote:
> > In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
> > and clk_unprepare() for the omapdss clocks.
>
> You used clk_prepare and clk_unprepare instead of clk_prepare_enable and
> clk_disable_unprepare. I didn't check the dss driver yet, but my hunch
> is that the clocks are normally not enabled/disabled from atomic
> context.
>
> What does the prepare/unprepare actually do? Is there any benefit in
> delaying preparing, i.e. is there a difference between prepare right
> after clk_get, or prepare right before clk_enable? (And similarly for
> unprepare)
I think you're looking at this the wrong way.
Think of clk_prepare() as the _sleepable_ part of clk_enable(). So, think
of clk_prepare() as ideally being placed just before clk_enable() with the
exception that if that's not possible (because you're in a non-atomic
context) it should be placed in a position as close to clk_enable() as
possible which is atomic.
Don't think of it as an extra step to be done after clk_get() but as an
extra step needing to be done before clk_enable().
^ permalink raw reply
* Re: [PATCH] grvga: Fix error handling issues
From: Emil Goode @ 2012-06-25 9:58 UTC (permalink / raw)
To: Julia Lawall
Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors,
w.sang
In-Reply-To: <alpine.DEB.2.02.1206250718360.1874@hadrien>
It would be nice to get rid of the indention levels in the error
handling code and the devm is to prefer from the older api.
I'm traveling but will probably send another version of this patch this
evening.
Thanks,
Emil
On Mon, 2012-06-25 at 07:20 +0200, Julia Lawall wrote:
> On Sun, 24 Jun 2012, Emil Goode wrote:
>
> > Yes I considered converting to devm_ and keeping the of_iounmap in.
> > But I just feel like I'm mixing two api's. If converting to devm_ really
> > is the right thing to do, tell me and I will send another patch :)
>
> devm is safer and simpler, because you can just get rid of error handling
> code rather than ensuring over time that it continues to be done in all of
> the place where it is needed. So I would think that it is always better
> to use it. But others may think differently. Another option would be to
> only use it for request_mem_region, which you can do consistently in this
> case, and leave both of_ioremap and ioremap as they are.
>
> julia
>
>
> >
> > Thanks,
> >
> > Emil
> >
> > On Sun, 2012-06-24 at 07:15 +0200, Julia Lawall wrote:
> > > I see. You could still use a devm_ function for request_mem_region.
> > > Also I noticed that the remove function uses iounmap directly, not
> > > io_iounmap. The latter is just a wrapper for the former, but it could be
> > > good to use the right name.
> > >
> > > julia
> > >
> > > On Sun, 24 Jun 2012, Emil Goode wrote:
> > >
> > > > The of_ioremap function is used in this code as well and I don't know of
> > > > a devm_ equivalent for it. For consistency I think it is better to leave
> > > > it as it is in this case. So I stick with v1 of this patch.
> > > >
> > > > Thanks,
> > > >
> > > > Emil
> > > >
> > > > On Fri, 2012-06-22 at 18:21 +0200, Julia Lawall wrote:
> > > >> On Fri, 22 Jun 2012, Emil Goode wrote:
> > > >>
> > > >>> Good idea, I will take another look at it tomorrow.
> > > >>
> > > >> There is a devm_ function that combines request_mem_region and ioremap
> > > >> that could be useful.
> > > >>
> > > >> julia
> > > >>
> > > >>>
> > > >>> Thanks,
> > > >>>
> > > >>> Emil
> > > >>>
> > > >>> On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
> > > >>>> Maybe you could used the devm_ functions for request_mem_region and
> > > >>>> ioremap so that the error handling can just be dropped?
> > > >>>>
> > > >>>> julia
> > > >>>>
> > > >>>> On Fri, 22 Jun 2012, Emil Goode wrote:
> > > >>>>
> > > >>>>> This patch fixes two problems with the error handling in the
> > > >>>>> grvga_probe function.
> > > >>>>>
> > > >>>>> - If the call to grvga_parse_custom on line 370 fails we use
> > > >>>>> the wrong label so that release_mem_region will be called
> > > >>>>> without a call to request_mem_region being made.
> > > >>>>>
> > > >>>>> - If the call to ioremap on line 436 fails we should not try
> > > >>>>> to call iounmap. I added an if statement to check whether or
> > > >>>>> not a call to iounmap should be made.
> > > >>>>>
> > > >>>>> - I also changed the names of the labels to make the code
> > > >>>>> easier to read.
> > > >>>>>
> > > >>>>> Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > > >>>>> ---
> > > >>>>> drivers/video/grvga.c | 34 ++++++++++++++++++----------------
> > > >>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
> > > >>>>> index da066c2..d9d688a 100644
> > > >>>>> --- a/drivers/video/grvga.c
> > > >>>>> +++ b/drivers/video/grvga.c
> > > >>>>> @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> */
> > > >>>>> if (fb_get_options("grvga", &options)) {
> > > >>>>> retval = -ENODEV;
> > > >>>>> - goto err;
> > > >>>>> + goto free_fb;
> > > >>>>> }
> > > >>>>>
> > > >>>>> if (!options || !*options)
> > > >>>>> @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> if (grvga_parse_custom(this_opt, &info->var) < 0) {
> > > >>>>> dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
> > > >>>>> retval = -EINVAL;
> > > >>>>> - goto err1;
> > > >>>>> + goto free_fb;
> > > >>>>> }
> > > >>>>> } else if (!strncmp(this_opt, "addr", 4))
> > > >>>>> grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
> > > >>>>> @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
> > > >>>>> dev_err(&dev->dev, "registers already mapped\n");
> > > >>>>> retval = -EBUSY;
> > > >>>>> - goto err;
> > > >>>>> + goto free_fb;
> > > >>>>> }
> > > >>>>>
> > > >>>>> par->regs = of_ioremap(&dev->resource[0], 0,
> > > >>>>> @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> if (!par->regs) {
> > > >>>>> dev_err(&dev->dev, "failed to map registers\n");
> > > >>>>> retval = -ENOMEM;
> > > >>>>> - goto err1;
> > > >>>>> + goto release_regs;
> > > >>>>> }
> > > >>>>>
> > > >>>>> retval = fb_alloc_cmap(&info->cmap, 256, 0);
> > > >>>>> if (retval < 0) {
> > > >>>>> dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
> > > >>>>> retval = -ENOMEM;
> > > >>>>> - goto err2;
> > > >>>>> + goto unmap_regs;
> > > >>>>> }
> > > >>>>>
> > > >>>>> if (mode_opt) {
> > > >>>>> @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
> > > >>>>> if (!retval || retval = 4) {
> > > >>>>> retval = -EINVAL;
> > > >>>>> - goto err3;
> > > >>>>> + goto dealloc_cmap;
> > > >>>>> }
> > > >>>>> }
> > > >>>>>
> > > >>>>> @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
> > > >>>>> dev_err(&dev->dev, "failed to request memory region\n");
> > > >>>>> retval = -ENOMEM;
> > > >>>>> - goto err3;
> > > >>>>> + goto dealloc_cmap;
> > > >>>>> }
> > > >>>>>
> > > >>>>> virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
> > > >>>>> @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> if (!virtual_start) {
> > > >>>>> dev_err(&dev->dev, "error mapping framebuffer memory\n");
> > > >>>>> retval = -ENOMEM;
> > > >>>>> - goto err4;
> > > >>>>> + goto free_mem;
> > > >>>>> }
> > > >>>>> } else { /* Allocate frambuffer memory */
> > > >>>>>
> > > >>>>> @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> "unable to allocate framebuffer memory (%lu bytes)\n",
> > > >>>>> grvga_mem_size);
> > > >>>>> retval = -ENOMEM;
> > > >>>>> - goto err3;
> > > >>>>> + goto dealloc_cmap;
> > > >>>>> }
> > > >>>>>
> > > >>>>> physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
> > > >>>>> @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>> retval = register_framebuffer(info);
> > > >>>>> if (retval < 0) {
> > > >>>>> dev_err(&dev->dev, "failed to register framebuffer\n");
> > > >>>>> - goto err4;
> > > >>>>> + goto free_mem;
> > > >>>>> }
> > > >>>>>
> > > >>>>> __raw_writel(physical_start, &par->regs->fb_pos);
> > > >>>>> @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > > >>>>>
> > > >>>>> return 0;
> > > >>>>>
> > > >>>>> -err4:
> > > >>>>> +free_mem:
> > > >>>>> dev_set_drvdata(&dev->dev, NULL);
> > > >>>>> if (grvga_fix_addr) {
> > > >>>>> release_mem_region(physical_start, grvga_mem_size);
> > > >>>>> - iounmap((void *)virtual_start);
> > > >>>>> + if (virtual_start)
> > > >>>>> + iounmap((void *)virtual_start);
> > > >>>>> } else
> > > >>>>> kfree((void *)virtual_start);
> > > >>>>> -err3:
> > > >>>>> +
> > > >>>>> +dealloc_cmap:
> > > >>>>> fb_dealloc_cmap(&info->cmap);
> > > >>>>> -err2:
> > > >>>>> +unmap_regs:
> > > >>>>> of_iounmap(&dev->resource[0], par->regs,
> > > >>>>> resource_size(&dev->resource[0]));
> > > >>>>> -err1:
> > > >>>>> +release_regs:
> > > >>>>> release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
> > > >>>>> -err:
> > > >>>>> +free_fb:
> > > >>>>> framebuffer_release(info);
> > > >>>>>
> > > >>>>> return retval;
> > > >>>>> --
> > > >>>>> 1.7.10
> > > >>>>>
> > > >>>>> --
> > > >>>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > >>>>> the body of a message to majordomo@vger.kernel.org
> > > >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >>>>>
> > > >>>
> > > >>>
> > > >>> --
> > > >>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > >>> the body of a message to majordomo@vger.kernel.org
> > > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >>>
> > > >
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >
> >
> >
> >
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Tomi Valkeinen @ 2012-06-25 9:30 UTC (permalink / raw)
To: Jassi Brar; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <CAJe_Zhf-H0dLtFsFAcryDy3av60RjmWmP+d3GSaxq8dNZKiXWw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 937 bytes --]
On Mon, 2012-06-25 at 14:19 +0530, Jassi Brar wrote:
> On 25 June 2012 11:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> ....
> >> Currenlty HDMI fails to come up in the suspend-resume path.
> >> This patch helps that real-world scenario.
> >
> > What is the problem there? It'd be good to explain the problem in the
> > patch description. Does the pm_runtime_get return -EACCES?
> >
> Yes, it returns -EACCESS because RPM on devices is disabled during the
> period from suspend-start to resume-finished.
So... You didn't answer my first comment, how can the code work? The
driver needs to enable the HW and the call to pm_runtime_get() is
skipped. Won't this lead to crash as the DSS registers are accessed
without the HW in enabled state? And what happens if the
pm_runtime_get() call is skipped, but pm_runtime_put() is not?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-25 9:16 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340611895.3395.14.camel@deskari>
On 25 June 2012 13:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>> I am perfectly OK to resend as a patch series, if you want.
>
> Yes please.
>
OK, will do.
>> >> bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
>> >> {
>> >> - return gpio_get_value(ip_data->hpd_gpio);
>> >> + if (gpio_get_value(ip_data->hpd_gpio))
>> >> + return true;
>> >> +
>> >> + /* Invalidate EDID Cache */
>> >> + ip_data->edid_len = 0;
>> >> +
>> >> + return false;
>> >
>> > Why is this needed? The HPD interrupt should handle this already. And if
>> > the HPD interrupt doesn't work for some reason, this won't work either,
>> > as the user can plug and unplug his HDMI monitors a thousand times
>> > between two detect calls.
>> >
>> I thought it is almost impossible to unplug->plug cycle the HDMI cable
>> even twice a second, let alone 1000 times against the 10Hz .detect()
>> poll :) Or you mean some relay controlled HDMI switching
>> mechanism?
>
> omapdss doesn't call the detect function, so it can't presume it's used
> in some certain frequency. Also, last time I tested omapdrm, I think
> detect was called once in 5 secs or so.
>
It's not omapdss. It's the DRM stack, via the omapdrm, that polls
every 10 secs (not 5).
Sorry I said 10Hz instead of 1/10Hz.
>> Anyways, that is not the point of this patch. This patch only aims to
>> avoid un-ncessary EDID reads while doing its best to make sure we
>> invalidate EDID 'as soon as possible'.
>
> I'm not sure I understand your point. If the HPD interrupt works
> properly, EDID is invalidated there, and the code in detect is not
> needed. And if the HPD interrupt doesn't work (although I don't see why
> it wouldn't), the code in detect doesn't work. In either case it's not
> correct.
>
Well, the idea was to tie edid-cache invalidating with de-asserted
HPD, wherever we read HPD. I will drop it.
thnx
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Jassi Brar @ 2012-06-25 8:53 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340605221.12683.30.camel@lappyti>
On 25 June 2012 11:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
....
>> Currenlty HDMI fails to come up in the suspend-resume path.
>> This patch helps that real-world scenario.
>
> What is the problem there? It'd be good to explain the problem in the
> patch description. Does the pm_runtime_get return -EACCES?
>
Yes, it returns -EACCESS because RPM on devices is disabled during the
period from suspend-start to resume-finished.
^ permalink raw reply
* Re: [PATCH] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-25 8:11 UTC (permalink / raw)
To: Jassi Brar; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <CAJe_Zhf1RsDtKXOpQ97Am-ByBYUtOgjJYyySGYGL9zHNYB_Ejw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4332 bytes --]
On Mon, 2012-06-25 at 13:26 +0530, Jassi Brar wrote:
> On 25 June 2012 12:05, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> >> From: Jassi Brar <jaswinder.singh@linaro.org>
> >>
> >> We can easily keep track of latest EDID from the display and hence avoid
> >> expensive EDID re-reads over I2C.
> >> This could also help some cheapo displays that provide EDID reliably only
> >> immediately after asserting HPD and not later.
> >> Even with good displays, there is something in OMAPDSS that apparantly
> >> messes up DDC occasionally while EDID is being read, giving the
> >> "operation stopped when reading edid" error.
> >>
> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >> ---
> >> drivers/video/omap2/dss/hdmi.c | 1 +
> >> drivers/video/omap2/dss/ti_hdmi.h | 2 ++
> >> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 23 ++++++++++++++++++++---
> >> 3 files changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> >> index 900e621..0a8c825 100644
> >> --- a/drivers/video/omap2/dss/hdmi.c
> >> +++ b/drivers/video/omap2/dss/hdmi.c
> >> @@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
> >> hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
> >> hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
> >> hdmi.ip_data.phy_offset = HDMI_PHY;
> >> + hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
> >> mutex_init(&hdmi.ip_data.lock);
> >
> > Your HDMI patches seem to depend on each other. Please post them as a
> > proper patch series, instead of each one separately.
> >
> They don't depend functionality wise. Any fix can be accepted
> regardless of others.
> I deliberately avoided a series, because revision of just one could
> require resending 3, otherwise
> perfectly OK, patches. I just wanted to limit the noise.
You don't need to send the whole series, you can just send a revised
patch as a reply to the older version of that patch. (see --in-reply-to
of git send-email).
Of course if you end up changing many of the patches, or one patch lots
of times, it is good to send the whole series at some point later when
the patches have stabilized.
> I understand, 'git am' might complain but I think that should be trivial to fix.
I'd rather not spend time doing trivial fixes, or trying to find latest
versions of individual patches that have dependencies. Having the
patches in a series and replying with new versions to the older versions
makes my life much easier. I'll see all of them in my email client in
one bunch, properly sorted.
> I am perfectly OK to resend as a patch series, if you want.
Yes please.
> >> bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
> >> {
> >> - return gpio_get_value(ip_data->hpd_gpio);
> >> + if (gpio_get_value(ip_data->hpd_gpio))
> >> + return true;
> >> +
> >> + /* Invalidate EDID Cache */
> >> + ip_data->edid_len = 0;
> >> +
> >> + return false;
> >
> > Why is this needed? The HPD interrupt should handle this already. And if
> > the HPD interrupt doesn't work for some reason, this won't work either,
> > as the user can plug and unplug his HDMI monitors a thousand times
> > between two detect calls.
> >
> I thought it is almost impossible to unplug->plug cycle the HDMI cable
> even twice a second, let alone 1000 times against the 10Hz .detect()
> poll :) Or you mean some relay controlled HDMI switching
> mechanism?
omapdss doesn't call the detect function, so it can't presume it's used
in some certain frequency. Also, last time I tested omapdrm, I think
detect was called once in 5 secs or so.
> Anyways, that is not the point of this patch. This patch only aims to
> avoid un-ncessary EDID reads while doing its best to make sure we
> invalidate EDID 'as soon as possible'.
I'm not sure I understand your point. If the HPD interrupt works
properly, EDID is invalidated there, and the code in detect is not
needed. And if the HPD interrupt doesn't work (although I don't see why
it wouldn't), the code in detect doesn't work. In either case it's not
correct.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-25 8:08 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340606154.12683.34.camel@lappyti>
On 25 June 2012 12:05, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> We can easily keep track of latest EDID from the display and hence avoid
>> expensive EDID re-reads over I2C.
>> This could also help some cheapo displays that provide EDID reliably only
>> immediately after asserting HPD and not later.
>> Even with good displays, there is something in OMAPDSS that apparantly
>> messes up DDC occasionally while EDID is being read, giving the
>> "operation stopped when reading edid" error.
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>> ---
>> drivers/video/omap2/dss/hdmi.c | 1 +
>> drivers/video/omap2/dss/ti_hdmi.h | 2 ++
>> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 23 ++++++++++++++++++++---
>> 3 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
>> index 900e621..0a8c825 100644
>> --- a/drivers/video/omap2/dss/hdmi.c
>> +++ b/drivers/video/omap2/dss/hdmi.c
>> @@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
>> hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
>> hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
>> hdmi.ip_data.phy_offset = HDMI_PHY;
>> + hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
>> mutex_init(&hdmi.ip_data.lock);
>
> Your HDMI patches seem to depend on each other. Please post them as a
> proper patch series, instead of each one separately.
>
They don't depend functionality wise. Any fix can be accepted
regardless of others.
I deliberately avoided a series, because revision of just one could
require resending 3, otherwise
perfectly OK, patches. I just wanted to limit the noise.
I understand, 'git am' might complain but I think that should be trivial to fix.
I am perfectly OK to resend as a patch series, if you want.
>> hdmi_panel_init();
>> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
>> index cc292b8..4735860 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi.h
>> +++ b/drivers/video/omap2/dss/ti_hdmi.h
>> @@ -178,6 +178,8 @@ struct hdmi_ip_data {
>> /* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
>> int hpd_gpio;
>> struct mutex lock;
>> + u8 edid_cached[256];
>> + unsigned edid_len;
>> };
>> int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
>> void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> index 04acca9..2633ade 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>>
>> hpd = gpio_get_value(ip_data->hpd_gpio);
>>
>> - if (hpd)
>> + if (hpd) {
>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
>> - else
>> + } else {
>> + /* Invalidate EDID Cache */
>> + ip_data->edid_len = 0;
>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
>> + }
>>
>> if (r) {
>> DSSERR("Failed to %s PHY TX power\n",
>> @@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
>> {
>> int r, l;
>>
>> + if (ip_data->edid_len) {
>> + memcpy(edid, ip_data->edid_cached, ip_data->edid_len);
>> + return ip_data->edid_len;
>> + }
>> +
>> if (len < 128)
>> return -EINVAL;
>>
>> @@ -474,12 +482,21 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
>> l += 128;
>> }
>>
>> + ip_data->edid_len = l;
>> + memcpy(ip_data->edid_cached, edid, l);
>> +
>> return l;
>> }
>>
>> bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
>> {
>> - return gpio_get_value(ip_data->hpd_gpio);
>> + if (gpio_get_value(ip_data->hpd_gpio))
>> + return true;
>> +
>> + /* Invalidate EDID Cache */
>> + ip_data->edid_len = 0;
>> +
>> + return false;
>
> Why is this needed? The HPD interrupt should handle this already. And if
> the HPD interrupt doesn't work for some reason, this won't work either,
> as the user can plug and unplug his HDMI monitors a thousand times
> between two detect calls.
>
I thought it is almost impossible to unplug->plug cycle the HDMI cable
even twice a second, let alone 1000 times against the 10Hz .detect()
poll :) Or you mean some relay controlled HDMI switching
mechanism?
Anyways, that is not the point of this patch. This patch only aims to
avoid un-ncessary EDID reads while doing its best to make sure we
invalidate EDID 'as soon as possible'.
Thanks.
^ permalink raw reply
* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Tomi Valkeinen @ 2012-06-25 7:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4FE80C43.6090802@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]
On Mon, 2012-06-25 at 12:29 +0530, Rajendra Nayak wrote:
> On Monday 25 June 2012 11:37 AM, Tomi Valkeinen wrote:
> > Hi,
> >
> > On Fri, 2012-06-22 at 19:18 +0530, Rajendra Nayak wrote:
> >> In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
> >> and clk_unprepare() for the omapdss clocks.
> >
> > You used clk_prepare and clk_unprepare instead of clk_prepare_enable and
> > clk_disable_unprepare. I didn't check the dss driver yet, but my hunch
> > is that the clocks are normally not enabled/disabled from atomic
> > context.
> >
> > What does the prepare/unprepare actually do? Is there any benefit in
> > delaying preparing, i.e. is there a difference between prepare right
> > after clk_get, or prepare right before clk_enable? (And similarly for
> > unprepare)
>
> clk_prepare/unprepare are useful for clocks which need the 'enable'
> logic to be implemented as a slow part (which can sleep) and a fast part
> (which does not sleep). For all the dss clocks in question we don't need
> a slow part and hence they do not have a .clk_prepare/unprepare
> platform hook.
>
> The framework however still does prepare usecounting (it has a prepare
> count and an enable count, and prepare count is expected to be non-zero
> while the clock is being enabled) and uses a mutex around to guard it.
> So while the dss driver would do multiple clk_enable/disable while its
> active, it seems fair to just prepare/unprepare the clocks once just
> after clk_get() and before clk_put() in this particular case.
But the driver should not presume anything special about the clocks. In
this case the dss driver would presume that the clocks it uses do not
have prepare and unprepare hooks.
If the generally proper way to use prepare/unprepare is in combination
of enable/disable, then I think we should try to do that.
I'll check if any of the dss clocks are enabled or disabled in atomic
context.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Rajendra Nayak @ 2012-06-25 7:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340604478.12683.25.camel@lappyti>
On Monday 25 June 2012 11:37 AM, Tomi Valkeinen wrote:
> Hi,
>
> On Fri, 2012-06-22 at 19:18 +0530, Rajendra Nayak wrote:
>> In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
>> and clk_unprepare() for the omapdss clocks.
>
> You used clk_prepare and clk_unprepare instead of clk_prepare_enable and
> clk_disable_unprepare. I didn't check the dss driver yet, but my hunch
> is that the clocks are normally not enabled/disabled from atomic
> context.
>
> What does the prepare/unprepare actually do? Is there any benefit in
> delaying preparing, i.e. is there a difference between prepare right
> after clk_get, or prepare right before clk_enable? (And similarly for
> unprepare)
clk_prepare/unprepare are useful for clocks which need the 'enable'
logic to be implemented as a slow part (which can sleep) and a fast part
(which does not sleep). For all the dss clocks in question we don't need
a slow part and hence they do not have a .clk_prepare/unprepare
platform hook.
The framework however still does prepare usecounting (it has a prepare
count and an enable count, and prepare count is expected to be non-zero
while the clock is being enabled) and uses a mutex around to guard it.
So while the dss driver would do multiple clk_enable/disable while its
active, it seems fair to just prepare/unprepare the clocks once just
after clk_get() and before clk_put() in this particular case.
>
> Tom
>
^ permalink raw reply
* Re: [PATCH] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-25 6:35 UTC (permalink / raw)
To: jaswinder.singh; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340438806-25622-1-git-send-email-jaswinder.singh@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 3801 bytes --]
On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> We can easily keep track of latest EDID from the display and hence avoid
> expensive EDID re-reads over I2C.
> This could also help some cheapo displays that provide EDID reliably only
> immediately after asserting HPD and not later.
> Even with good displays, there is something in OMAPDSS that apparantly
> messes up DDC occasionally while EDID is being read, giving the
> "operation stopped when reading edid" error.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> drivers/video/omap2/dss/hdmi.c | 1 +
> drivers/video/omap2/dss/ti_hdmi.h | 2 ++
> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 23 ++++++++++++++++++++---
> 3 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 900e621..0a8c825 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -764,6 +764,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
> hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
> hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
> hdmi.ip_data.phy_offset = HDMI_PHY;
> + hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */
> mutex_init(&hdmi.ip_data.lock);
Your HDMI patches seem to depend on each other. Please post them as a
proper patch series, instead of each one separately.
> hdmi_panel_init();
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index cc292b8..4735860 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -178,6 +178,8 @@ struct hdmi_ip_data {
> /* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
> int hpd_gpio;
> struct mutex lock;
> + u8 edid_cached[256];
> + unsigned edid_len;
> };
> int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
> void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> index 04acca9..2633ade 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>
> hpd = gpio_get_value(ip_data->hpd_gpio);
>
> - if (hpd)
> + if (hpd) {
> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
> - else
> + } else {
> + /* Invalidate EDID Cache */
> + ip_data->edid_len = 0;
> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
> + }
>
> if (r) {
> DSSERR("Failed to %s PHY TX power\n",
> @@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
> {
> int r, l;
>
> + if (ip_data->edid_len) {
> + memcpy(edid, ip_data->edid_cached, ip_data->edid_len);
> + return ip_data->edid_len;
> + }
> +
> if (len < 128)
> return -EINVAL;
>
> @@ -474,12 +482,21 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data,
> l += 128;
> }
>
> + ip_data->edid_len = l;
> + memcpy(ip_data->edid_cached, edid, l);
> +
> return l;
> }
>
> bool ti_hdmi_4xxx_detect(struct hdmi_ip_data *ip_data)
> {
> - return gpio_get_value(ip_data->hpd_gpio);
> + if (gpio_get_value(ip_data->hpd_gpio))
> + return true;
> +
> + /* Invalidate EDID Cache */
> + ip_data->edid_len = 0;
> +
> + return false;
Why is this needed? The HPD interrupt should handle this already. And if
the HPD interrupt doesn't work for some reason, this won't work either,
as the user can plug and unplug his HDMI monitors a thousand times
between two detect calls.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state
From: Tomi Valkeinen @ 2012-06-25 6:20 UTC (permalink / raw)
To: jaswinder.singh; +Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne
In-Reply-To: <1340438771-25587-1-git-send-email-jaswinder.singh@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 841 bytes --]
On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> If the runtime PM of the device is disabled (for example in resume from
> suspend path), it doesn't make sense to attempt pm_runtime_get/put, esp
> when their return values affect the control flow path.
This looks strange. When the driver does pm_runtime_get() it expects the
HW to be functional and registers accessible. If we just skip the
pm_runtime_get(), how can the code work?
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>
> Currenlty HDMI fails to come up in the suspend-resume path.
> This patch helps that real-world scenario.
What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare
From: Tomi Valkeinen @ 2012-06-25 6:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340372890-10091-6-git-send-email-rnayak@ti.com>
[-- Attachment #1: Type: text/plain, Size: 652 bytes --]
Hi,
On Fri, 2012-06-22 at 19:18 +0530, Rajendra Nayak wrote:
> In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
> and clk_unprepare() for the omapdss clocks.
You used clk_prepare and clk_unprepare instead of clk_prepare_enable and
clk_disable_unprepare. I didn't check the dss driver yet, but my hunch
is that the clocks are normally not enabled/disabled from atomic
context.
What does the prepare/unprepare actually do? Is there any benefit in
delaying preparing, i.e. is there a difference between prepare right
after clk_get, or prepare right before clk_enable? (And similarly for
unprepare)
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] grvga: Fix error handling issues
From: Julia Lawall @ 2012-06-25 5:20 UTC (permalink / raw)
To: Emil Goode
Cc: FlorianSchandinat, linux-fbdev, linux-kernel, kernel-janitors,
w.sang
In-Reply-To: <1340534180.4696.14.camel@localhost>
On Sun, 24 Jun 2012, Emil Goode wrote:
> Yes I considered converting to devm_ and keeping the of_iounmap in.
> But I just feel like I'm mixing two api's. If converting to devm_ really
> is the right thing to do, tell me and I will send another patch :)
devm is safer and simpler, because you can just get rid of error handling
code rather than ensuring over time that it continues to be done in all of
the place where it is needed. So I would think that it is always better
to use it. But others may think differently. Another option would be to
only use it for request_mem_region, which you can do consistently in this
case, and leave both of_ioremap and ioremap as they are.
julia
>
> Thanks,
>
> Emil
>
> On Sun, 2012-06-24 at 07:15 +0200, Julia Lawall wrote:
> > I see. You could still use a devm_ function for request_mem_region.
> > Also I noticed that the remove function uses iounmap directly, not
> > io_iounmap. The latter is just a wrapper for the former, but it could be
> > good to use the right name.
> >
> > julia
> >
> > On Sun, 24 Jun 2012, Emil Goode wrote:
> >
> > > The of_ioremap function is used in this code as well and I don't know of
> > > a devm_ equivalent for it. For consistency I think it is better to leave
> > > it as it is in this case. So I stick with v1 of this patch.
> > >
> > > Thanks,
> > >
> > > Emil
> > >
> > > On Fri, 2012-06-22 at 18:21 +0200, Julia Lawall wrote:
> > >> On Fri, 22 Jun 2012, Emil Goode wrote:
> > >>
> > >>> Good idea, I will take another look at it tomorrow.
> > >>
> > >> There is a devm_ function that combines request_mem_region and ioremap
> > >> that could be useful.
> > >>
> > >> julia
> > >>
> > >>>
> > >>> Thanks,
> > >>>
> > >>> Emil
> > >>>
> > >>> On Fri, 2012-06-22 at 18:12 +0200, Julia Lawall wrote:
> > >>>> Maybe you could used the devm_ functions for request_mem_region and
> > >>>> ioremap so that the error handling can just be dropped?
> > >>>>
> > >>>> julia
> > >>>>
> > >>>> On Fri, 22 Jun 2012, Emil Goode wrote:
> > >>>>
> > >>>>> This patch fixes two problems with the error handling in the
> > >>>>> grvga_probe function.
> > >>>>>
> > >>>>> - If the call to grvga_parse_custom on line 370 fails we use
> > >>>>> the wrong label so that release_mem_region will be called
> > >>>>> without a call to request_mem_region being made.
> > >>>>>
> > >>>>> - If the call to ioremap on line 436 fails we should not try
> > >>>>> to call iounmap. I added an if statement to check whether or
> > >>>>> not a call to iounmap should be made.
> > >>>>>
> > >>>>> - I also changed the names of the labels to make the code
> > >>>>> easier to read.
> > >>>>>
> > >>>>> Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > >>>>> ---
> > >>>>> drivers/video/grvga.c | 34 ++++++++++++++++++----------------
> > >>>>> 1 file changed, 18 insertions(+), 16 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/video/grvga.c b/drivers/video/grvga.c
> > >>>>> index da066c2..d9d688a 100644
> > >>>>> --- a/drivers/video/grvga.c
> > >>>>> +++ b/drivers/video/grvga.c
> > >>>>> @@ -354,7 +354,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> */
> > >>>>> if (fb_get_options("grvga", &options)) {
> > >>>>> retval = -ENODEV;
> > >>>>> - goto err;
> > >>>>> + goto free_fb;
> > >>>>> }
> > >>>>>
> > >>>>> if (!options || !*options)
> > >>>>> @@ -370,7 +370,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> if (grvga_parse_custom(this_opt, &info->var) < 0) {
> > >>>>> dev_err(&dev->dev, "Failed to parse custom mode (%s).\n", this_opt);
> > >>>>> retval = -EINVAL;
> > >>>>> - goto err1;
> > >>>>> + goto free_fb;
> > >>>>> }
> > >>>>> } else if (!strncmp(this_opt, "addr", 4))
> > >>>>> grvga_fix_addr = simple_strtoul(this_opt + 5, NULL, 16);
> > >>>>> @@ -390,7 +390,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> if (!request_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]), "grlib-svgactrl regs")) {
> > >>>>> dev_err(&dev->dev, "registers already mapped\n");
> > >>>>> retval = -EBUSY;
> > >>>>> - goto err;
> > >>>>> + goto free_fb;
> > >>>>> }
> > >>>>>
> > >>>>> par->regs = of_ioremap(&dev->resource[0], 0,
> > >>>>> @@ -400,14 +400,14 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> if (!par->regs) {
> > >>>>> dev_err(&dev->dev, "failed to map registers\n");
> > >>>>> retval = -ENOMEM;
> > >>>>> - goto err1;
> > >>>>> + goto release_regs;
> > >>>>> }
> > >>>>>
> > >>>>> retval = fb_alloc_cmap(&info->cmap, 256, 0);
> > >>>>> if (retval < 0) {
> > >>>>> dev_err(&dev->dev, "failed to allocate mem with fb_alloc_cmap\n");
> > >>>>> retval = -ENOMEM;
> > >>>>> - goto err2;
> > >>>>> + goto unmap_regs;
> > >>>>> }
> > >>>>>
> > >>>>> if (mode_opt) {
> > >>>>> @@ -415,7 +415,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> grvga_modedb, sizeof(grvga_modedb), &grvga_modedb[0], 8);
> > >>>>> if (!retval || retval = 4) {
> > >>>>> retval = -EINVAL;
> > >>>>> - goto err3;
> > >>>>> + goto dealloc_cmap;
> > >>>>> }
> > >>>>> }
> > >>>>>
> > >>>>> @@ -430,7 +430,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> if (!request_mem_region(physical_start, grvga_mem_size, dev->name)) {
> > >>>>> dev_err(&dev->dev, "failed to request memory region\n");
> > >>>>> retval = -ENOMEM;
> > >>>>> - goto err3;
> > >>>>> + goto dealloc_cmap;
> > >>>>> }
> > >>>>>
> > >>>>> virtual_start = (unsigned long) ioremap(physical_start, grvga_mem_size);
> > >>>>> @@ -438,7 +438,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> if (!virtual_start) {
> > >>>>> dev_err(&dev->dev, "error mapping framebuffer memory\n");
> > >>>>> retval = -ENOMEM;
> > >>>>> - goto err4;
> > >>>>> + goto free_mem;
> > >>>>> }
> > >>>>> } else { /* Allocate frambuffer memory */
> > >>>>>
> > >>>>> @@ -451,7 +451,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> "unable to allocate framebuffer memory (%lu bytes)\n",
> > >>>>> grvga_mem_size);
> > >>>>> retval = -ENOMEM;
> > >>>>> - goto err3;
> > >>>>> + goto dealloc_cmap;
> > >>>>> }
> > >>>>>
> > >>>>> physical_start = dma_map_single(&dev->dev, (void *)virtual_start, grvga_mem_size, DMA_TO_DEVICE);
> > >>>>> @@ -484,7 +484,7 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>> retval = register_framebuffer(info);
> > >>>>> if (retval < 0) {
> > >>>>> dev_err(&dev->dev, "failed to register framebuffer\n");
> > >>>>> - goto err4;
> > >>>>> + goto free_mem;
> > >>>>> }
> > >>>>>
> > >>>>> __raw_writel(physical_start, &par->regs->fb_pos);
> > >>>>> @@ -493,21 +493,23 @@ static int __devinit grvga_probe(struct platform_device *dev)
> > >>>>>
> > >>>>> return 0;
> > >>>>>
> > >>>>> -err4:
> > >>>>> +free_mem:
> > >>>>> dev_set_drvdata(&dev->dev, NULL);
> > >>>>> if (grvga_fix_addr) {
> > >>>>> release_mem_region(physical_start, grvga_mem_size);
> > >>>>> - iounmap((void *)virtual_start);
> > >>>>> + if (virtual_start)
> > >>>>> + iounmap((void *)virtual_start);
> > >>>>> } else
> > >>>>> kfree((void *)virtual_start);
> > >>>>> -err3:
> > >>>>> +
> > >>>>> +dealloc_cmap:
> > >>>>> fb_dealloc_cmap(&info->cmap);
> > >>>>> -err2:
> > >>>>> +unmap_regs:
> > >>>>> of_iounmap(&dev->resource[0], par->regs,
> > >>>>> resource_size(&dev->resource[0]));
> > >>>>> -err1:
> > >>>>> +release_regs:
> > >>>>> release_mem_region(dev->resource[0].start, resource_size(&dev->resource[0]));
> > >>>>> -err:
> > >>>>> +free_fb:
> > >>>>> framebuffer_release(info);
> > >>>>>
> > >>>>> return retval;
> > >>>>> --
> > >>>>> 1.7.10
> > >>>>>
> > >>>>> --
> > >>>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > >>>>> the body of a message to majordomo@vger.kernel.org
> > >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >>>>>
> > >>>
> > >>>
> > >>> --
> > >>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > >>> the body of a message to majordomo@vger.kernel.org
> > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >>>
> > >
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
>
>
>
^ permalink raw reply
* Re: [PATCH RESEND] video: w100fb: Reduce sleep mode battery discharge
From: Florian Tobias Schandinat @ 2012-06-24 21:24 UTC (permalink / raw)
To: Paul Parsons
Cc: mreimer, philipp.zabel, linux-kernel, linux-fbdev@vger.kernel.org
In-Reply-To: <20120603103355.3845gmx1@mx072.gmx.net>
Hi Paul,
you should als the relevant mailing list, linux-fbdev (done now)
On 06/03/2012 10:33 AM, Paul Parsons wrote:
> In 2006 and 2007 the handhelds.org kernel w100fb driver was patched to
> reduce sleep mode battery discharge. Unfortunately those two patches
> never migrated to the mainline kernel.
>
> Fortunately the function affected - w100_suspend() - has not changed
> since; thus those patches still apply cleanly.
>
> Applying those patches to linux-3.4-rc3 running on an HP iPAQ hx4700
> reduces the sleep mode battery discharge from approximately 26 mA to
> approximately 11 mA.
>
> This patch combines those two patches into a single unified patch.
>
> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> Cc: Matt Reimer <mreimer@sdgsystems.com>
> Cc: Philipp Zabel <philipp.zabel@gmail.com>
Actually it would be good if we had the Signed-offs of the original
authors. Oh well, I think if it can't be helped yours should be sufficient.
> ---
>
> Many thanks to Matt Reimer for suggesting this.
>
> The seeming demise of handhelds.org has removed any formal patch logs.
> However the patches can still be viewed online:
> 2006 patch:
> http://paste.lisp.org/display/31700
> 2007 patch:
> http://osdir.com/ml/handhelds.linux.kernel/2007-02/msg00020.html
>
> And the fully patched handhelds.org kernel is still available, e.g.:
> http://www.linuxtogo.org/~goxboxlive/htcuniversal/Kernel/linux-2.6.21-hh20.tar.bz2
>
> drivers/video/w100fb.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/w100fb.c b/drivers/video/w100fb.c
> index 90a2e30..2f6b2b8 100644
> --- a/drivers/video/w100fb.c
> +++ b/drivers/video/w100fb.c
> @@ -1567,6 +1567,18 @@ static void w100_suspend(u32 mode)
> val = readl(remapped_regs + mmPLL_CNTL);
> val |= 0x00000004; /* bit2=1 */
> writel(val, remapped_regs + mmPLL_CNTL);
> +
> + writel(0x00000000, remapped_regs + mmLCDD_CNTL1);
> + writel(0x00000000, remapped_regs + mmLCDD_CNTL2);
> + writel(0x00000000, remapped_regs + mmGENLCD_CNTL1);
> + writel(0x00000000, remapped_regs + mmGENLCD_CNTL2);
> + writel(0x00000000, remapped_regs + mmGENLCD_CNTL3);
> +
> + val = readl(remapped_regs + mmMEM_EXT_CNTL);
> + val |= 0xF0000000;
> + val &= ~(0x00000001);
> + writel(val, remapped_regs + mmMEM_EXT_CNTL);
> +
If anyone understands what this change does (like for example disabling
the LCD controller) a comment would be useful.
> writel(0x0000001d, remapped_regs + mmPWRMGT_CNTL);
> }
> }
If nobody comments on this I will apply it in a few days.
Best regards,
Florian Tobias Schandinat
^ permalink raw reply
* Re: [PULL for v3.6] SH Mobile LCDC fixes and planes support
From: Florian Tobias Schandinat @ 2012-06-24 21:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <3246665.HJN7RE5zfe@avalon>
On 06/20/2012 08:31 AM, Laurent Pinchart wrote:
> Hi Florian,
>
> On Wednesday 20 June 2012 01:01:31 Florian Tobias Schandinat wrote:
>> On 06/16/2012 02:07 PM, Laurent Pinchart wrote:
>>> Hi Florian,
>>>
>>> Could you please pull the following LCDC patches ?
>>
>> looks good, except...
>>
>>> The following changes since commit
> b67989515defba7412acff01162e5bb1f0f5923a:
>>> video: s3c-fb: fix possible division by zero in s3c_fb_calc_pixclk
>>>
>>> (2012-06-13 17:34:16 +0000)
>>>
>>> are available in the git repository at:
>>> git://linuxtv.org/pinchartl/fbdev.git planes
>>>
>>> Guennadi Liakhovetski (1):
>>> fbdev: sh_mipi_dsi: fix a section mismatch
>>>
>>> Laurent Pinchart (4):
>>> fbdev: sh_mobile_lcdc: Don't confuse line size with pitch
>>
>> ...for this patch it would have been better if you based your work on a
>> more recent tree of Linus that already contains it instead of asking me
>> to pull it again [I now forwarded my fbdev-next to -rc3]. It's not a big
>> issue as git handles such situations but pushing patches around that are
>> essentially nops should be avoided I think. Well, if you decide to fix
>> it I'd be happy, otherwise I'll pull this in a few days nonetheless.
>
> Sorry about that. I've rebased the branch on top of the latest fbdev-next and
> pushed it to git://linuxtv.org/pinchartl/fbdev.git planes, that no-op patches
> isn't present anymore.
Merged.
Thanks,
Florian Tobias Schandinat
^ permalink raw reply
* Re: [PATCH] video: exynos_dp: remove duplicated declarations from header file
From: Florian Tobias Schandinat @ 2012-06-24 21:00 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <000601cd4f94$415cd750$c41685f0$%han@samsung.com>
On 06/21/2012 09:57 AM, Jingoo Han wrote:
> Some functions are declared twice in header file; thus, these
> declarations are unnecessary.
>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/exynos/exynos_dp_core.h | 4 ----
> 1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 1e0f998..8526e54 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -85,10 +85,6 @@ void exynos_dp_set_link_bandwidth(struct exynos_dp_device *dp, u32 bwtype);
> void exynos_dp_get_link_bandwidth(struct exynos_dp_device *dp, u32 *bwtype);
> void exynos_dp_set_lane_count(struct exynos_dp_device *dp, u32 count);
> void exynos_dp_get_lane_count(struct exynos_dp_device *dp, u32 *count);
> -void exynos_dp_set_link_bandwidth(struct exynos_dp_device *dp, u32 bwtype);
> -void exynos_dp_get_link_bandwidth(struct exynos_dp_device *dp, u32 *bwtype);
> -void exynos_dp_set_lane_count(struct exynos_dp_device *dp, u32 count);
> -void exynos_dp_get_lane_count(struct exynos_dp_device *dp, u32 *count);
> void exynos_dp_enable_enhanced_mode(struct exynos_dp_device *dp, bool enable);
> void exynos_dp_set_training_pattern(struct exynos_dp_device *dp,
> enum pattern_set pattern);
^ permalink raw reply
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