* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Kishon Vijay Abraham I @ 2013-10-24 15:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK9yfHxaLsdFGXiCxvs+HpMSuY6xWd=CGPv-YfSkJqWSxE+f-w@mail.gmail.com>
Hi,
On Thursday 24 October 2013 09:12 PM, Sachin Kamat wrote:
> Hi Olof,
>
> On 24 October 2013 20:00, Olof Johansson <olof@lixom.net> wrote:
>> Hi Kishon,
>>
>> On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
>>> index 32e5406..00b3a52 100644
>>> --- a/drivers/video/exynos/exynos_mipi_dsi.c
>>> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
>>> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
>>> exynos_mipi_regulator_enable(dsim);
>>>
>>> /* enable MIPI-DSI PHY. */
>>> - if (dsim->pd->phy_enable)
>>> - dsim->pd->phy_enable(pdev, true);
>>> + phy_power_on(dsim->phy);
>>>
>>> clk_enable(dsim->clock);
>>>
>>
>> This introduces the below with exynos_defconfig:
>>
>> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
>> 'exynos_mipi_dsi_blank_mode':
>> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
>> variable 'pdev' [-Wunused-variable]
>> struct platform_device *pdev = to_platform_device(dsim->dev);
>>
>
> I have already submitted a patch to fix this [1]
>
> [1] http://marc.info/?l=linux-fbdev&m\x138233359617936&w=2
Sorry, missed that :-(
Thanks
Kishon
>
>
^ permalink raw reply
* Re: [PATCH 1/1] video: exynos_mipi_dsi: Remove unused variable
From: Kishon Vijay Abraham I @ 2013-10-24 15:55 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1382333533-32740-1-git-send-email-sachin.kamat@linaro.org>
On Monday 21 October 2013 11:02 AM, Sachin Kamat wrote:
> 'pdev' is not used anymore (Removed vide commit 7e0be9f9 "video:
> exynos_mipi_dsim: Use the generic PHY driver"). Remove it and
> silence the following compilation warning:
> drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning:
> unused variable ‘pdev’ [-Wunused-variable]
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Reviewed-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> drivers/video/exynos/exynos_mipi_dsi.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
> index 00b3a52..cee9602 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
> @@ -141,7 +141,6 @@ static int exynos_mipi_dsi_early_blank_mode(struct mipi_dsim_device *dsim,
>
> static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
> {
> - struct platform_device *pdev = to_platform_device(dsim->dev);
> struct mipi_dsim_lcd_driver *client_drv = dsim->dsim_lcd_drv;
> struct mipi_dsim_lcd_device *client_dev = dsim->dsim_lcd_dev;
>
>
^ permalink raw reply
* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Sachin Kamat @ 2013-10-24 15:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOesGMhwotSY-1WQmt+wtsrsH2m30VE=j-MwyhpYU3mt_PSPPw@mail.gmail.com>
Hi Olof,
On 24 October 2013 20:00, Olof Johansson <olof@lixom.net> wrote:
> Hi Kishon,
>
> On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
>> index 32e5406..00b3a52 100644
>> --- a/drivers/video/exynos/exynos_mipi_dsi.c
>> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
>> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
>> exynos_mipi_regulator_enable(dsim);
>>
>> /* enable MIPI-DSI PHY. */
>> - if (dsim->pd->phy_enable)
>> - dsim->pd->phy_enable(pdev, true);
>> + phy_power_on(dsim->phy);
>>
>> clk_enable(dsim->clock);
>>
>
> This introduces the below with exynos_defconfig:
>
> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
> 'exynos_mipi_dsi_blank_mode':
> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
> variable 'pdev' [-Wunused-variable]
> struct platform_device *pdev = to_platform_device(dsim->dev);
>
I have already submitted a patch to fix this [1]
[1] http://marc.info/?l=linux-fbdev&m\x138233359617936&w=2
--
With warm regards,
Sachin
^ permalink raw reply
* [PATCH] drivers: video: exynos: Fix compiler Warning
From: Kishon Vijay Abraham I @ 2013-10-24 15:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOesGMhwotSY-1WQmt+wtsrsH2m30VE=j-MwyhpYU3mt_PSPPw@mail.gmail.com>
Fixed the following compilation warning:
../../drivers/video/exynos/exynos_mipi_dsi.c: In function
'exynos_mipi_dsi_blank_mode':
../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
variable 'pdev' [-Wunused-variable]
struct platform_device *pdev = to_platform_device(dsim->dev);
Reported-by: Olof Johansson <olof@lixom.net>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
drivers/video/exynos/exynos_mipi_dsi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
index 00b3a52..cee9602 100644
--- a/drivers/video/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/exynos/exynos_mipi_dsi.c
@@ -141,7 +141,6 @@ static int exynos_mipi_dsi_early_blank_mode(struct mipi_dsim_device *dsim,
static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
{
- struct platform_device *pdev = to_platform_device(dsim->dev);
struct mipi_dsim_lcd_driver *client_drv = dsim->dsim_lcd_drv;
struct mipi_dsim_lcd_device *client_dev = dsim->dsim_lcd_dev;
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] drivers: video: exynos: Fix compiler Warning
From: Felipe Balbi @ 2013-10-24 15:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382628454-11951-1-git-send-email-kishon@ti.com>
[-- Attachment #1: Type: text/plain, Size: 632 bytes --]
On Thu, Oct 24, 2013 at 08:57:34PM +0530, Kishon Vijay Abraham I wrote:
> Fixed the following compilation warning:
> ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
> 'exynos_mipi_dsi_blank_mode':
> ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
> variable 'pdev' [-Wunused-variable]
> struct platform_device *pdev = to_platform_device(dsim->dev);
>
> Reported-by: Olof Johansson <olof@lixom.net>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
pretty obvious patch:
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 5/7] phy: Add driver for Exynos DP PHY
From: Tomasz Stanislawski @ 2013-10-24 15:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1381940896-9355-6-git-send-email-kishon@ti.com>
Hi Kishon,
Recently, I posted 'simple-phy' driver.
It is a part of patchset for HDMI enabling on Exynos4 using Device Tree.
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg23655.html
The simple-phy was dedicated for single register PHYs like HDMI or DP.
Using such a generic phy may help to avoid code duplication.
Regards,
Tomasz Stanislawski
On 10/16/2013 06:28 PM, Kishon Vijay Abraham I wrote:
> From: Jingoo Han <jg1.han@samsung.com>
>
> Add a PHY provider driver for the Samsung Exynos SoC Display Port PHY.
>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> .../devicetree/bindings/phy/samsung-phy.txt | 8 ++
> drivers/phy/Kconfig | 7 ++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-exynos-dp-video.c | 111 ++++++++++++++++++++
> 4 files changed, 127 insertions(+)
> create mode 100644 drivers/phy/phy-exynos-dp-video.c
>
^ permalink raw reply
* Re: [PATCH 3/7] video: exynos_mipi_dsim: Use the generic PHY driver
From: Olof Johansson @ 2013-10-24 14:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1381940896-9355-4-git-send-email-kishon@ti.com>
Hi Kishon,
On Wed, Oct 16, 2013 at 9:28 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
> index 32e5406..00b3a52 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
> @@ -156,8 +157,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
> exynos_mipi_regulator_enable(dsim);
>
> /* enable MIPI-DSI PHY. */
> - if (dsim->pd->phy_enable)
> - dsim->pd->phy_enable(pdev, true);
> + phy_power_on(dsim->phy);
>
> clk_enable(dsim->clock);
>
This introduces the below with exynos_defconfig:
../../drivers/video/exynos/exynos_mipi_dsi.c: In function
'exynos_mipi_dsi_blank_mode':
../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
variable 'pdev' [-Wunused-variable]
struct platform_device *pdev = to_platform_device(dsim->dev);
-Olof
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-24 11:48 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Laurent Pinchart, Tomi Valkeinen, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <2692478.1sYB6OvYJ1@avalon>
[-- Attachment #1: Type: text/plain, Size: 14527 bytes --]
On Thu, Oct 24, 2013 at 01:05:49PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
>
> On Thursday 17 October 2013 14:46:20 Thierry Reding wrote:
> > On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote:
> > > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:
> > > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> > > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
>
> [snip]
>
> > > The point is that the video pipeline must be described in DT. Having a
> > > per-device way to represent connections would be a nightmare to support
> > > from an implementation point of view, hence the need for a generic way to
> > > describe them.
> >
> > Okay, so we're back to the need to describe pipelines in DT. At the risk
> > of sounding selfish: I don't care about pipelines. I just want us to
> > settle on a way to represent a dumb panel in DT so that it can be
> > enabled when it needs to. Furthermore I don't have any hardware that
> > exhibits any of these "advanced" features, so I'm totally unqualified to
> > work on any of this.
> >
> > Can we please try to be a little pragmatic here and solve one problem at
> > a time? I am aware that solving this for panels may require some amount
> > of foresight, but let's not try to solve everything at once at the risk
> > of not getting anything done at all.
>
> I won't force you to care about pipelines, and I don't think this is selfish
> at all :-)
>
> What I would like to ensure is that whatever bindings we come up with, they
> will not preclude us from adding pipeline support when needed (as I'm pretty
> sure it will be needed at some point).
Sure. I think I've explained elsewhere that I consider this easily
possible. Other people have said the same thing. If all we do is add
properties to a binding, then drivers written for an old binding will
have no problem continuing to work.
I also think that we're mostly being too scared. If ever the next device
comes around that requires explicit pipeline support to work, how likely
is it to have the exact same display as prior devices?
> > > > I'm not at all opposed to the idea of CDF or the problems it tries to
> > > > address. I don't think it necessarily should be a separate framework for
> > > > reasons explained earlier. But even if it were to end up in a separate
> > > > framework there's absolutely nothing preventing us from adding a DRM
> > > > panel driver that hooks into CDF as a "backend" of sorts for panels that
> > > > require something more complex than the simple-panel binding.
> > >
> > > Once again it's not about panel having complex needs, but more about using
> > > simple panels in complex pipelines. I'm fine with the drm_panel
> > > infrastructure, what I would like is DT bindings that describe connections
> > > in a more future-proof way. The rest is fine.
> >
> > And I've already said elsewhere that the bindings in their current form
> > are easily extensible to cater for the needs of CDF.
>
> The simple panel bindings do not include any connection information, so we
> could add that later when needed without having to deprecate, remove or
> repurpose existing properties.
Yes, I agree.
> The simple panel driver would need to be extended, which isn't much of a
> problem (except that extending it with CDF support might require changes
> to the users of the simple panel driver, which I believe won't be happily
> accepted, but that's a different issue).
That might be true. But that's just the way kernel development works. We
sometimes require major rework of APIs and we're actually pretty good at
pulling that off, so I don't worry too much about it.
Also, being the original author of the driver makes me the maintainer,
and if it should prove to be necessary I'm absolutely willing to add CDF
support.
> My concern is also on the other side. In the patches you've sent the tegra
> driver uses a custom nvidia,panel property to reference the panel. That would
> of course not be CDF-compatible, but there's no way around that at the moment
> if we don't want to keep development of all ARM KMS drivers stalled for the
> next 6 months.
Well, the nvidia,panel property is part of the display output and it is
an optional property. So the driver already needs to cope with it being
absent. If the display output driver is ever extended with CDF support
then we can just go and use CDF if CDF-specific properties exist, or we
fall back to nvidia,panel if that doesn't exist.
> It boils down to the question of whether DT should be a stable ABI, and
> I'm increasingly tempted to say that I don't care. I want to solve
> issues we have on the display side, the firmware interface isn't my main
> concern.
I wholeheartedly agree. In my opinion it is much more useful to come up
with a solution that works now, rather than discussing and arguing
things to death and not get anything done. If that means that I'll have
to maintain additional code, then so be it.
> > > > But that's precisely the point. Why would we need to go back from the
> > > > panel to the display controller? Especially for dumb panels that can't
> > > > or don't have to be configured in any way. Even if they needed some sort
> > > > of setup, why can't that be done from the display controller/output.
> > > >
> > > > Even given a setup where a DSI controller needs to write some registers
> > > > in a panel upon initialization, I don't see why the reverse connection
> > > > needs to be described. The fact alone that an output dereferences a
> > > > panel's phandle should be enough to connect both of them and have any
> > > > panel driver use the DSI controller that it's been attached to for the
> > > > programming.
> > > >
> > > > That's very much analogous to how I2C works. There are no connections
> > > > back to the I2C master from the slaves. Yet each I2C client driver
> > > > manages to use the services provided by the I2C master to perform
> > > > transactions on the I2C bus. In a similar way the DSI controller is the
> > > > bus master that talks to DSI panels. DSI panels don't actively talk to
> > > > the DSI controller.
> > >
> > > It's all about modeling video pipeline graphs in DT. To be able to walk
> > > the graph we need to describe connections. Not having bidirectional
> > > information means that we restrict the points at which we can start
> > > walking the graph, potentially making our life much more difficult in the
> > > future.
> > >
> > > What's so wrong about adding a port node and link information to the panel
> > > DT node ? It describe what's there: the panel has one input, why not make
> > > that explicit ?
> >
> > What's wrong with it is that there's no way to verify the soundness of
> > the design by means of a full implementation because we're missing the
> > majority of the pieces. Just putting the nodes into DT to provide some
> > imaginary future-proofness isn't helpful either. Without any code that
> > actually uses them they are useless.
> >
> > And again, why should we add them right away (while not needed) when
> > they can easily be added in a backwards-compatible manner at some later
> > point when there's actually any use for them and they can actually be
> > tested in some larger framework?
>
> It's the "easily" part I'm not sure about. I doubt we'll ever have any easy to
> solve DT backward compatibility issue. However, as mentioned above, this
> shouldn't be a show stopper. I'm thus fine with the way the proposed bindings
> describe (or rather don't describe) the connection. However, I will then
> expect your support in the future to implement the "easy" extension of the
> bindings to support CDF.
Again, I don't expect that to ever happen. But if it ever happens
anyway, then...
> Do we have a deal ? ;-)
Yes, we do.
> > > > > > > > +static void panel_simple_enable(struct drm_panel *panel)
> > > > > > > > +{
> > > > > > > > + struct panel_simple *p = to_panel_simple(panel);
> > > > > > > > + int err;
> > > > > > > > +
> > > > > > > > + if (p->enabled)
> > > > > > > > + return;
> > > > > > > > +
> > > > > > > > + err = regulator_enable(p->supply);
> > > > > > > > + if (err < 0)
> > > > > > > > + dev_err(panel->dev, "failed to enable supply: %d\n",
> err);
> > > > > > >
> > > > > > > Is that really a non-fatal error ? Shouldn't the enable operation
> > > > > > > return an int ?
> > > > > >
> > > > > > There's no way to propagate this in DRM, so why go through the
> > > > > > trouble of returning the error? Furthermore, there's nothing that
> > > > > > the caller could do to remedy the situation anyway.
> > > > >
> > > > > That's a DRM issue, which could be fixed. While the caller can't
> > > > > remedy the situation, it should at least report the error to the
> > > > > application instead of silently ignoring it.
> > > >
> > > > Perhaps. It's not really relevant to the discussion and can always be
> > > > fixed in a subsequent patch.
> > >
> > > Most things can be fixed by a subsequent patch, that's not a good enough
> > > reason not to address the known problems before pushing the code to
> > > mainline (to clarify my point of view, there's no need to fix DRM to use
> > > the return value before pushing this patch set to mainline, but I'd like
> > > a v2 with an int return value).
> >
> > Why? What's the use of returning an error if you know up front that it
> > can't be used? This should be changed if or when we "fix" DRM to propagate
> > errors.
>
> Because not doing so now will require us to change (potentially) lots of panel
> drivers at that time. It's much easier to have each panel driver developer
> implement the required code in his/her driver than having a single developer
> refactoring the code later and have to touch all drivers. If your concern is
> that the error paths won't be testable at the moment, you could easily already
> add a WARN_ON() to the caller to catch problems.
I don't mind fixing potentially many drivers. The conversion is pretty
mechanical and therefore easy. We even have tools such as coccinelle to
help with it. But we've probably spent more time arguing the point than
it would take to make this simple change, so I'll just go and change the
return value to an int and return an error.
Perhaps if I'm in the mood I'll even write up a patch to propagate the
error further.
> > > > > > > Instead of hardcoding the modes in the driver, which would then
> > > > > > > require to be updated for every new simple panel model (and we
> > > > > > > know there are lots of them), why don't you specify the modes in
> > > > > > > the panel DT node ? The simple panel driver would then become much
> > > > > > > more generic. It would also allow board designers to tweak h/v
> > > > > > > sync timings depending on the system requirements.
> > > > > >
> > > > > > Sigh... we keep second-guessing ourselves. Back at the time when
> > > > > > power sequences were designed (and NAKed at the last minute), we all
> > > > > > decided that the right thing to do would be to use specific
> > > > > > compatible values for individual panels, because that would allow us
> > > > > > to encode the power sequencing within the driver. And when we
> > > > > > already have the panel model encoded in the compatible value, we
> > > > > > might just as well encode the mode within the driver for that panel.
> > > > > > Otherwise we'll have to keep adding the same mode timings for every
> > > > > > board that uses the same panel.
> > > > > >
> > > > > > I do agree though that it might be useful to tweak the mode in case
> > > > > > the default one doesn't work. How about we provide a means to
> > > > > > override the mode encoded in the driver using one specified in the
> > > > > > DT? That's obviously a backwards-compatible change, so it could be
> > > > > > added if or when it becomes necessary.
> > > > >
> > > > > I share Tomi's point of view here. Your three panels use the same
> > > > > power sequence, so I believe we should have a generic panel compatible
> > > > > string that would use modes described in DT for the common case. Only
> > > > > if a panel required something more complex which can't (or which
> > > > > could, but won't for any reason) accurately be described in DT would
> > > > > you need to extend the driver.
> > > >
> > > > I don't see the point quite frankly. You seem to be assuming that every
> > > > panel will only be used in a single board.
> > >
> > > No, but in practice that's often the case, at least for boards with .dts
> > > files in the mainline kernel.
> > >
> > > > However what you're proposing will require the mode timings to be
> > > > repeated in every board's DT file, if the same panel is ever used on
> > > > more than a single board.
> > >
> > > Is that a problem ? You could #include a .dtsi for the panel that would
> > > specify the video mode if you want to avoid multiple copies.
> >
> > Yes, I don't think it's desirable to duplicate data needlessly in DT. It
> > also makes the binding more difficult to use. If I know that the panel
> > is one supported by the simple-panel binding, I can just go and add the
> > right compatible value without having to bother looking up the mode
> > timings and duplicating them. That way DT writers get to concern
> > themselves only with the variable data.
>
> I've had a quick chat with Dave Airlie and Hans de Goede yesterday about this.
> As most panels will use standard timings, Hans proposed adding a timings
> property that would reference the standard timings the panel uses (this could
> be a string or integer defined in include/dt-bindings/...). In most case DT
> would just have a single property to select the timings, and only in more
> complex cases where the system designer wants to use custom timings would the
> timings need to be manually defined.
We can do the same thing within the kernel. We already have a list of
standard EDID/HDMI/CEA display modes, so we could similarly add a new
list of common display panel modes and make each driver reference that
instead of defining a custom one.
And that still enables us to add a property that would allow DT writers
to override the display mode if they need to.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] drm/omap: fix (un)registering irqs inside an irq handler
From: Rob Clark @ 2013-10-24 11:36 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Linux Fbdev development list, dri-devel@lists.freedesktop.org
In-Reply-To: <1382597450-23066-1-git-send-email-tomi.valkeinen@ti.com>
On Thu, Oct 24, 2013 at 2:50 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> omapdrm (un)registers irqs inside an irq handler. The problem is that
> the (un)register function uses dispc_runtime_get/put() to enable the
> clocks, and those functions are not irq safe by default.
>
> This was kind of fixed in 48664b21aeeffb40c5fa06843f18052e2c4ec9ef
> (OMAPDSS: DISPC: set irq_safe for runtime PM), which makes dispc's
> runtime calls irq-safe.
>
> However, using pm_runtime_irq_safe in dispc makes the parent of dispc,
> dss, always enabled, effectively preventing PM for the whole DSS module.
>
> This patch makes omapdrm behave better by adding new irq (un)register
> functions that do not use dispc_runtime_get/put, and using those
> functions in interrupt context. Thus we can make dispc again
> non-irq-safe, allowing proper PM.
Looks good
Reviewed-by: Rob Clark <robdclark@gmail.com>
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Rob Clark <robdclark@gmail.com>
> ---
> drivers/gpu/drm/omapdrm/omap_crtc.c | 6 +++---
> drivers/gpu/drm/omapdrm/omap_drv.h | 2 ++
> drivers/gpu/drm/omapdrm/omap_irq.c | 22 ++++++++++++++++++----
> drivers/video/omap2/dss/dispc.c | 1 -
> 4 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 0fd2eb1..e6241c2 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -411,7 +411,7 @@ static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
> struct drm_crtc *crtc = &omap_crtc->base;
> DRM_ERROR("%s: errors: %08x\n", omap_crtc->name, irqstatus);
> /* avoid getting in a flood, unregister the irq until next vblank */
> - omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
> + __omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
> }
>
> static void omap_crtc_apply_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
> @@ -421,13 +421,13 @@ static void omap_crtc_apply_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
> struct drm_crtc *crtc = &omap_crtc->base;
>
> if (!omap_crtc->error_irq.registered)
> - omap_irq_register(crtc->dev, &omap_crtc->error_irq);
> + __omap_irq_register(crtc->dev, &omap_crtc->error_irq);
>
> if (!dispc_mgr_go_busy(omap_crtc->channel)) {
> struct omap_drm_private *priv > crtc->dev->dev_private;
> DBG("%s: apply done", omap_crtc->name);
> - omap_irq_unregister(crtc->dev, &omap_crtc->apply_irq);
> + __omap_irq_unregister(crtc->dev, &omap_crtc->apply_irq);
> queue_work(priv->wq, &omap_crtc->apply_work);
> }
> }
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 30b95b7..cfb6c2e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -145,6 +145,8 @@ irqreturn_t omap_irq_handler(DRM_IRQ_ARGS);
> void omap_irq_preinstall(struct drm_device *dev);
> int omap_irq_postinstall(struct drm_device *dev);
> void omap_irq_uninstall(struct drm_device *dev);
> +void __omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq);
> +void __omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq);
> void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq);
> void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq);
> int omap_drm_irq_uninstall(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
> index 9263db1..b08b902 100644
> --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> @@ -45,12 +45,11 @@ static void omap_irq_update(struct drm_device *dev)
> dispc_read_irqenable(); /* flush posted write */
> }
>
> -void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
> +void __omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
> {
> struct omap_drm_private *priv = dev->dev_private;
> unsigned long flags;
>
> - dispc_runtime_get();
> spin_lock_irqsave(&list_lock, flags);
>
> if (!WARN_ON(irq->registered)) {
> @@ -60,14 +59,21 @@ void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
> }
>
> spin_unlock_irqrestore(&list_lock, flags);
> +}
> +
> +void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
> +{
> + dispc_runtime_get();
> +
> + __omap_irq_register(dev, irq);
> +
> dispc_runtime_put();
> }
>
> -void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
> +void __omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
> {
> unsigned long flags;
>
> - dispc_runtime_get();
> spin_lock_irqsave(&list_lock, flags);
>
> if (!WARN_ON(!irq->registered)) {
> @@ -77,6 +83,14 @@ void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
> }
>
> spin_unlock_irqrestore(&list_lock, flags);
> +}
> +
> +void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
> +{
> + dispc_runtime_get();
> +
> + __omap_irq_unregister(dev, irq);
> +
> dispc_runtime_put();
> }
>
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index 4779750..02a7340 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -3691,7 +3691,6 @@ static int __init omap_dispchw_probe(struct platform_device *pdev)
> }
>
> pm_runtime_enable(&pdev->dev);
> - pm_runtime_irq_safe(&pdev->dev);
>
> r = dispc_runtime_get();
> if (r)
> --
> 1.8.1.2
>
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-24 11:20 UTC (permalink / raw)
To: Stephen Warren
Cc: Thierry Reding, Tomi Valkeinen, Dave Airlie, Laurent Pinchart,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <52645428.9080607-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Hi Stephen,
On Sunday 20 October 2013 23:07:36 Stephen Warren wrote:
> On 10/17/2013 12:07 PM, Laurent Pinchart wrote:
> ...
>
> >> As I said, anything that really needs a CDF binding to work
> >> likely isn't "simple" anymore, therefore a separate driver can
> >> easily be justified.
> >
> > The system as a whole would be more complex, but the panel could be
> > the same. We can't have two drivers for the same piece of hardware
> > in the DT world, as there will be a single compatible string and no
> > way to choose between the drivers (unlike the board code world that
> > could set device names to "foo- encoder-v4l2" or "foo-encoder-drm"
> > and live happily with that ever after).
>
> That's not true. We can certainly define two different compatible
> values for a piece of HW if we have to. We can easily control whether
> they are handled by the same or different drivers in the OS.
From an implementation point of view, sure. But from a conceptual point of
view, that would make the DT bindings pretty Linux-specific, with a
description of what the operating system should do instead of a description of
what the hardware looks like. My understanding is that we've tried pretty hard
in the past not to open that Pandora's box.
The case I'm mostly concerned about would be two different compatibility
strings to select whether the device should be handled by a KMS or V4L driver.
I don't think that's a good idea.
> Now, we should try to avoid this, because then that means that the
> original binding wasn't fully describing the HW. However, at least in
> the case of these simple LCD panels, it's hard to see that there is
> anything more than what's already in Thierry's binding. Remember, the
> binding is a description of the HW, not any Linux-internal details of
> how e.g. a CDF or DRM subsystem is supposed to use the HW; that had
> better be embodied into the driver or subsystem code, which aren't
> ABIs, and hence can be easily modified/enhanced.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-24 11:05 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Tomi Valkeinen, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131017124619.GG10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 14225 bytes --]
Hi Thierry,
On Thursday 17 October 2013 14:46:20 Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:
> > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
[snip]
> > The point is that the video pipeline must be described in DT. Having a
> > per-device way to represent connections would be a nightmare to support
> > from an implementation point of view, hence the need for a generic way to
> > describe them.
>
> Okay, so we're back to the need to describe pipelines in DT. At the risk
> of sounding selfish: I don't care about pipelines. I just want us to
> settle on a way to represent a dumb panel in DT so that it can be
> enabled when it needs to. Furthermore I don't have any hardware that
> exhibits any of these "advanced" features, so I'm totally unqualified to
> work on any of this.
>
> Can we please try to be a little pragmatic here and solve one problem at
> a time? I am aware that solving this for panels may require some amount
> of foresight, but let's not try to solve everything at once at the risk
> of not getting anything done at all.
I won't force you to care about pipelines, and I don't think this is selfish
at all :-)
What I would like to ensure is that whatever bindings we come up with, they
will not preclude us from adding pipeline support when needed (as I'm pretty
sure it will be needed at some point).
> > > > I'll still keep trying of course, but not for years.I have yet to
> > > > see someone proposing a viable solution to share drivers between DRM
> > > > and V4L2, which is a real pressing requirement, partly due to DT.
> > >
> > > Yeah... I still don't get that side of the argument. Why exactly do we
> > > need to share the drivers again?
> >
> > Think about a scaler IP in a SoC or FPGA, with one instance used in a
> > camera capture pipeline, supported by a V4L2 driver, and one instance
> > used in a video output pipeline, supported by a DRM driver. We want a
> > single driver for that IP core. Same story for video encoders.
>
> Yes, I see. This doesn't immediately concern the simple panel problem that
> I'm trying to solve, so perhaps we can have a separate discussion about it.
> Perhaps this would more easily be solved by providing some sort of helper
> library for the lower level control of the device and wrapping that with
> V4L2 or DRM APIs.
That's more or less the idea of CDF ;-)
> > > If we really need to support display output within V4L2, shouldn't we
> > > instead improve interoperability between the two and use DRM/KMS
> > > exclusively for the output part? Then the problem of having to share
> > > drivers between subsystems goes away and we'll actually be using the
> > > right subsystem for the right job at hand.
> >
> > In the long term we definitely should improve interoperability between the
> > two. I'd go further than that, having one API for video capture and one
> > API for video output doesn't make much sense, except for historical
> > reasons.
>
> Having two separate subsystems has worked out pretty well so far. In my
> opinion having strong boundaries between subsystems helps with design.
I'm not saying we've made a mistake. A unified subsystem wouldn't have got it
right in the first place either anyway. My point is that the boundary has
collapsed on the hardware side, so I believe the software side will need to
follow somehow.
> > > Of course none of that is relevant to the topic of DT bindings, which is
> > > what we should probably be concentrating on.
> >
> > It actually is. Given that DT bindings must describe hardware, the scaler
> > (or encoder, or other entity) would use the same bindings regardless of
> > the Linux subsystem that will be involved. We thus need to make sure the
> > bindings will be usable on both sides.
>
> Perhaps a single driver could expose both interfaces?
That's one idea that has been proposed. I believe it would be simpler to
standardize a common API and have translation layers specific to V4L and KMS
in the core, instead of pushing that translation to all
panel/bridges/encoders/whatever drivers.
> > > I'm not at all opposed to the idea of CDF or the problems it tries to
> > > address. I don't think it necessarily should be a separate framework for
> > > reasons explained earlier. But even if it were to end up in a separate
> > > framework there's absolutely nothing preventing us from adding a DRM
> > > panel driver that hooks into CDF as a "backend" of sorts for panels that
> > > require something more complex than the simple-panel binding.
> >
> > Once again it's not about panel having complex needs, but more about using
> > simple panels in complex pipelines. I'm fine with the drm_panel
> > infrastructure, what I would like is DT bindings that describe connections
> > in a more future-proof way. The rest is fine.
>
> And I've already said elsewhere that the bindings in their current form
> are easily extensible to cater for the needs of CDF.
The simple panel bindings do not include any connection information, so we
could add that later when needed without having to deprecate, remove or
repurpose existing properties. The simple panel driver would need to be
extended, which isn't much of a problem (except that extending it with CDF
support might require changes to the users of the simple panel driver, which I
believe won't be happily accepted, but that's a different issue).
My concern is also on the other side. In the patches you've sent the tegra
driver uses a custom nvidia,panel property to reference the panel. That would
of course not be CDF-compatible, but there's no way around that at the moment
if we don't want to keep development of all ARM KMS drivers stalled for the
next 6 months. It boils down to the question of whether DT should be a stable
ABI, and I'm increasingly tempted to say that I don't care. I want to solve
issues we have on the display side, the firmware interface isn't my main
concern.
> > > But that's precisely the point. Why would we need to go back from the
> > > panel to the display controller? Especially for dumb panels that can't
> > > or don't have to be configured in any way. Even if they needed some sort
> > > of setup, why can't that be done from the display controller/output.
> > >
> > > Even given a setup where a DSI controller needs to write some registers
> > > in a panel upon initialization, I don't see why the reverse connection
> > > needs to be described. The fact alone that an output dereferences a
> > > panel's phandle should be enough to connect both of them and have any
> > > panel driver use the DSI controller that it's been attached to for the
> > > programming.
> > >
> > > That's very much analogous to how I2C works. There are no connections
> > > back to the I2C master from the slaves. Yet each I2C client driver
> > > manages to use the services provided by the I2C master to perform
> > > transactions on the I2C bus. In a similar way the DSI controller is the
> > > bus master that talks to DSI panels. DSI panels don't actively talk to
> > > the DSI controller.
> >
> > It's all about modeling video pipeline graphs in DT. To be able to walk
> > the graph we need to describe connections. Not having bidirectional
> > information means that we restrict the points at which we can start
> > walking the graph, potentially making our life much more difficult in the
> > future.
> >
> > What's so wrong about adding a port node and link information to the panel
> > DT node ? It describe what's there: the panel has one input, why not make
> > that explicit ?
>
> What's wrong with it is that there's no way to verify the soundness of
> the design by means of a full implementation because we're missing the
> majority of the pieces. Just putting the nodes into DT to provide some
> imaginary future-proofness isn't helpful either. Without any code that
> actually uses them they are useless.
>
> And again, why should we add them right away (while not needed) when
> they can easily be added in a backwards-compatible manner at some later
> point when there's actually any use for them and they can actually be
> tested in some larger framework?
It's the "easily" part I'm not sure about. I doubt we'll ever have any easy to
solve DT backward compatibility issue. However, as mentioned above, this
shouldn't be a show stopper. I'm thus fine with the way the proposed bindings
describe (or rather don't describe) the connection. However, I will then
expect your support in the future to implement the "easy" extension of the
bindings to support CDF. De we have a deal ? ;-)
> > > > > > > +static void panel_simple_enable(struct drm_panel *panel)
> > > > > > > +{
> > > > > > > + struct panel_simple *p = to_panel_simple(panel);
> > > > > > > + int err;
> > > > > > > +
> > > > > > > + if (p->enabled)
> > > > > > > + return;
> > > > > > > +
> > > > > > > + err = regulator_enable(p->supply);
> > > > > > > + if (err < 0)
> > > > > > > + dev_err(panel->dev, "failed to enable supply: %d\n",
err);
> > > > > >
> > > > > > Is that really a non-fatal error ? Shouldn't the enable operation
> > > > > > return an int ?
> > > > >
> > > > > There's no way to propagate this in DRM, so why go through the
> > > > > trouble of returning the error? Furthermore, there's nothing that
> > > > > the caller could do to remedy the situation anyway.
> > > >
> > > > That's a DRM issue, which could be fixed. While the caller can't
> > > > remedy the situation, it should at least report the error to the
> > > > application instead of silently ignoring it.
> > >
> > > Perhaps. It's not really relevant to the discussion and can always be
> > > fixed in a subsequent patch.
> >
> > Most things can be fixed by a subsequent patch, that's not a good enough
> > reason not to address the known problems before pushing the code to
> > mainline (to clarify my point of view, there's no need to fix DRM to use
> > the return value before pushing this patch set to mainline, but I'd like
> > a v2 with an int return value).
>
> Why? What's the use of returning an error if you know up front that it
> can't be used? This should be changed if or when we "fix" DRM to propagate
> errors.
Because not doing so now will require us to change (potentially) lots of panel
drivers at that time. It's much easier to have each panel driver developer
implement the required code in his/her driver than having a single developer
refactoring the code later and have to touch all drivers. If your concern is
that the error paths won't be testable at the moment, you could easily already
add a WARN_ON() to the caller to catch problems.
> > > > > > Instead of hardcoding the modes in the driver, which would then
> > > > > > require to be updated for every new simple panel model (and we
> > > > > > know there are lots of them), why don't you specify the modes in
> > > > > > the panel DT node ? The simple panel driver would then become much
> > > > > > more generic. It would also allow board designers to tweak h/v
> > > > > > sync timings depending on the system requirements.
> > > > >
> > > > > Sigh... we keep second-guessing ourselves. Back at the time when
> > > > > power sequences were designed (and NAKed at the last minute), we all
> > > > > decided that the right thing to do would be to use specific
> > > > > compatible values for individual panels, because that would allow us
> > > > > to encode the power sequencing within the driver. And when we
> > > > > already have the panel model encoded in the compatible value, we
> > > > > might just as well encode the mode within the driver for that panel.
> > > > > Otherwise we'll have to keep adding the same mode timings for every
> > > > > board that uses the same panel.
> > > > >
> > > > > I do agree though that it might be useful to tweak the mode in case
> > > > > the default one doesn't work. How about we provide a means to
> > > > > override the mode encoded in the driver using one specified in the
> > > > > DT? That's obviously a backwards-compatible change, so it could be
> > > > > added if or when it becomes necessary.
> > > >
> > > > I share Tomi's point of view here. Your three panels use the same
> > > > power sequence, so I believe we should have a generic panel compatible
> > > > string that would use modes described in DT for the common case. Only
> > > > if a panel required something more complex which can't (or which
> > > > could, but won't for any reason) accurately be described in DT would
> > > > you need to extend the driver.
> > >
> > > I don't see the point quite frankly. You seem to be assuming that every
> > > panel will only be used in a single board.
> >
> > No, but in practice that's often the case, at least for boards with .dts
> > files in the mainline kernel.
> >
> > > However what you're proposing will require the mode timings to be
> > > repeated in every board's DT file, if the same panel is ever used on
> > > more than a single board.
> >
> > Is that a problem ? You could #include a .dtsi for the panel that would
> > specify the video mode if you want to avoid multiple copies.
>
> Yes, I don't think it's desirable to duplicate data needlessly in DT. It
> also makes the binding more difficult to use. If I know that the panel
> is one supported by the simple-panel binding, I can just go and add the
> right compatible value without having to bother looking up the mode
> timings and duplicating them. That way DT writers get to concern
> themselves only with the variable data.
I've had a quick chat with Dave Airlie and Hans de Goede yesterday about this.
As most panels will use standard timings, Hans proposed adding a timings
property that would reference the standard timings the panel uses (this could
be a string or integer defined in include/dt-bindings/...). In most case DT
would just have a single property to select the timings, and only in more
complex cases where the system designer wants to use custom timings would the
timings need to be manually defined.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-24 10:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, devicetree,
linux-fbdev, dri-devel, Dave Airlie, linux-media,
sylvester.nawrocki, Guennadi Liakhovetski
In-Reply-To: <1853455.BSevh91aGB@avalon>
[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]
On 24/10/13 13:40, Laurent Pinchart wrote:
>> panel {
>> remote = <&remote-endpoint>;
>> common-video-property = <asd>;
>> };
>>
>> panel {
>> port {
>> endpoint {
>> remote = <&remote-endpoint>;
>> common-video-property = <asd>;
>> };
>> };
>> };
>
> Please note that the common video properties would be in the panel node, not
> in the endpoint node (unless you have specific requirements to do so, which
> isn't the common case).
Hmm, well, the panel driver must look for its properties either in the
panel node, or in the endpoint node (I guess it could look them from
both, but that doesn't sound good).
If you write the panel driver, and in all your cases the properties work
fine in the panel node, does that mean they'll work fine with everybody?
I guess there are different kinds of properties. Something like a
regulator is obviously property of the panel. But anything related to
the video itself, like DPI's bus width, or perhaps even something like
"orientation" if the panel supports such, could need to be in the
endpoint node.
But yes, I understand what you mean. With "common-video-property" I
meant common properties like DPI bus width.
>> If that can be supported in the SW by adding complexity to a few functions,
>> and it covers practically all the panels, isn't it worth it?
>>
>> Note that I have not tried this, so I don't know if there are issues.
>> It's just a thought. Even if there's need for a endpoint node, perhaps
>> the port node can be made optional.
>
> It can be worth it, as long as we make sure that simplified bindings cover the
> needs of the generic code.
>
> We could assume that, if the port subnode isn't present, the device will have
> a single port, with a single endpoint. However, isn't the number of endpoints
Right.
> a system property rather than a device property ? If a port of a device is
Yes.
> connected to two remote ports it will require two endpoints. We could select
> the simplified or full bindings based on the system topology though.
The drivers should not know about simplified/normal bindings. They
should use CDF DT helper functions to get the port and endpoint
information. The helper functions would do the assuming.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-24 10:40 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, devicetree,
linux-fbdev, dri-devel, Dave Airlie, linux-media,
sylvester.nawrocki, Guennadi Liakhovetski
In-Reply-To: <525FD8DD.3090509@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3419 bytes --]
Hi Tomi,
On Thursday 17 October 2013 15:32:29 Tomi Valkeinen wrote:
> On 17/10/13 15:17, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote:
> >> On 17/10/13 14:51, Laurent Pinchart wrote:
> >>>> I'm not sure if there's a specific need for the port or endpoint nodes
> >>>> in cases like the above. Even if we have common properties describing
> >>>> the endpoint, I guess they could just be in the parent node.
> >>>>
> >>>> panel {
> >>>> remote = <&dc>;
> >>>> common-video-property = <asd>;
> >>>> };
> >>>>
> >>>> The above would imply one port and one endpoint. Would that work? If we
> >>>> had a function like parse_endpoint(node), we could just point it to
> >>>> either a real endpoint node, or to the device's node.
> >>>
> >>> You reference the display controller here, not a specific display
> >>> controller output. Don't most display controllers have several outputs ?
> >>
> >> Sure. Then the display controller could have more verbose description.
> >> But the panel could still have what I wrote above, except the 'remote'
> >> property would point to a real endpoint node inside the dispc node, not
> >> to the dispc node.
> >>
> >> This would, of course, need some extra code to handle the different
> >> cases, but just from DT point of view, I think all the relevant
> >> information is there.
> >
> > There's many ways to describe the same information in DT. While we could
> > have DT bindings that use different descriptions for different devices
> > and still support all of them in our code, why should we opt for that
> > option that will make the implementation much more complex when we can
> > describe connections in a simple and generic way ?
>
> My suggestion was simple and generic. I'm not proposing per-device
> custom bindings.
>
> My point was, if we can describe the connections as I described above,
> which to me sounds possible, we can simplify the panel DT data for 99.9%
> of the cases.
>
> To me, the first of these looks much nicer:
>
> panel {
> remote = <&remote-endpoint>;
> common-video-property = <asd>;
> };
>
> panel {
> port {
> endpoint {
> remote = <&remote-endpoint>;
> common-video-property = <asd>;
> };
> };
> };
Please note that the common video properties would be in the panel node, not
in the endpoint node (unless you have specific requirements to do so, which
isn't the common case).
> If that can be supported in the SW by adding complexity to a few functions,
> and it covers practically all the panels, isn't it worth it?
>
> Note that I have not tried this, so I don't know if there are issues.
> It's just a thought. Even if there's need for a endpoint node, perhaps
> the port node can be made optional.
It can be worth it, as long as we make sure that simplified bindings cover the
needs of the generic code.
We could assume that, if the port subnode isn't present, the device will have
a single port, with a single endpoint. However, isn't the number of endpoints
a system property rather than a device property ? If a port of a device is
connected to two remote ports it will require two endpoints. We could select
the simplified or full bindings based on the system topology though.
I've CC'ed Sylwester Nawrocki and Guennadi Liakhovetski, the V4L2 DT bindings
authors, as well as the linux-media list, to get their opinion on this.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* [PATCHv7][ 2/6] video: imxfb: Also add pwmr for the device tree.
From: Denis Carikli @ 2013-10-24 8:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382605103-9595-1-git-send-email-denis@eukrea.com>
pwmr has to be set to get the imxfb backlight work,
though pwmr was only configurable trough the platform data.
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree@vger.kernel.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
.../devicetree/bindings/video/fsl,imx-fb.txt | 3 +++
drivers/video/imxfb.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
index 46da08d..ac457ae 100644
--- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
+++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
@@ -17,6 +17,9 @@ Required nodes:
Optional properties:
- fsl,dmacr: DMA Control Register value. This is optional. By default, the
register is not modified as recommended by the datasheet.
+- fsl,pwmr: LCDC PWM Contrast Control Register value. That property is
+ optional, but defining it is necessary to get the backlight working. If that
+ property is ommited, the register is zeroed.
- fsl,lscr1: LCDC Sharp Configuration Register value.
Example:
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 4bf3837..f09372d 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -833,6 +833,8 @@ static int imxfb_init_fbinfo(struct platform_device *pdev)
of_property_read_u32(np, "fsl,dmacr", &fbi->dmacr);
+ of_property_read_u32(np, "fsl,pwmr", &fbi->pwmr);
+
/* These two function pointers could be used by some specific
* platforms. */
fbi->lcd_power = NULL;
--
1.7.9.5
^ permalink raw reply related
* [PATCHv7][ 1/6] video: imxfb: Introduce regulator support.
From: Denis Carikli @ 2013-10-24 8:58 UTC (permalink / raw)
To: linux-arm-kernel
This commit is based on the following commit by Fabio Estevam:
4344429 video: mxsfb: Introduce regulator support
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
drivers/video/imxfb.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 44ee678..4bf3837 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -28,6 +28,7 @@
#include <linux/cpufreq.h>
#include <linux/clk.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
#include <linux/dma-mapping.h>
#include <linux/io.h>
#include <linux/math64.h>
@@ -145,6 +146,7 @@ struct imxfb_info {
struct clk *clk_ipg;
struct clk *clk_ahb;
struct clk *clk_per;
+ struct regulator *reg_lcd;
enum imxfb_type devtype;
bool enabled;
@@ -563,12 +565,23 @@ static void imxfb_exit_backlight(struct imxfb_info *fbi)
static void imxfb_enable_controller(struct imxfb_info *fbi)
{
+ int ret;
if (fbi->enabled)
return;
pr_debug("Enabling LCD controller\n");
+ if (fbi->reg_lcd) {
+ ret = regulator_enable(fbi->reg_lcd);
+ if (ret) {
+ dev_err(&fbi->pdev->dev,
+ "lcd regulator enable failed with error: %d\n",
+ ret);
+ return;
+ }
+ }
+
writel(fbi->screen_dma, fbi->regs + LCDC_SSA);
/* panning offset 0 (0 pixel offset) */
@@ -597,6 +610,8 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
static void imxfb_disable_controller(struct imxfb_info *fbi)
{
+ int ret;
+
if (!fbi->enabled)
return;
@@ -613,6 +628,14 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
fbi->enabled = false;
writel(0, fbi->regs + LCDC_RMCR);
+
+ if (fbi->reg_lcd) {
+ ret = regulator_disable(fbi->reg_lcd);
+ if (ret)
+ dev_err(&fbi->pdev->dev,
+ "lcd regulator disable failed with error: %d\n",
+ ret);
+ }
}
static int imxfb_blank(int blank, struct fb_info *info)
@@ -1020,6 +1043,12 @@ static int imxfb_probe(struct platform_device *pdev)
goto failed_register;
}
+ fbi->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
+ if (IS_ERR(fbi->reg_lcd)) {
+ dev_info(&pdev->dev, "No lcd regulator used.\n");
+ fbi->reg_lcd = NULL;
+ }
+
imxfb_enable_controller(fbi);
fbi->pdev = pdev;
#ifdef PWMR_BACKLIGHT_AVAILABLE
--
1.7.9.5
^ permalink raw reply related
* [PATCHv6][ 2/7] video: imxfb: Also add pwmr for the device tree.
From: Denis Carikli @ 2013-10-24 8:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382601665-6830-1-git-send-email-denis@eukrea.com>
pwmr has to be set to get the imxfb backlight work,
though pwmr was only configurable trough the platform data.
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree@vger.kernel.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
.../devicetree/bindings/video/fsl,imx-fb.txt | 3 +++
drivers/video/imxfb.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
index 46da08d..ac457ae 100644
--- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
+++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
@@ -17,6 +17,9 @@ Required nodes:
Optional properties:
- fsl,dmacr: DMA Control Register value. This is optional. By default, the
register is not modified as recommended by the datasheet.
+- fsl,pwmr: LCDC PWM Contrast Control Register value. That property is
+ optional, but defining it is necessary to get the backlight working. If that
+ property is ommited, the register is zeroed.
- fsl,lscr1: LCDC Sharp Configuration Register value.
Example:
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 4bf3837..f09372d 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -833,6 +833,8 @@ static int imxfb_init_fbinfo(struct platform_device *pdev)
of_property_read_u32(np, "fsl,dmacr", &fbi->dmacr);
+ of_property_read_u32(np, "fsl,pwmr", &fbi->pwmr);
+
/* These two function pointers could be used by some specific
* platforms. */
fbi->lcd_power = NULL;
--
1.7.9.5
^ permalink raw reply related
* [PATCHv6][ 1/7] video: imxfb: Introduce regulator support.
From: Denis Carikli @ 2013-10-24 8:00 UTC (permalink / raw)
To: linux-arm-kernel
This commit is based on the following commit by Fabio Estevam:
4344429 video: mxsfb: Introduce regulator support
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
drivers/video/imxfb.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 44ee678..4bf3837 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -28,6 +28,7 @@
#include <linux/cpufreq.h>
#include <linux/clk.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
#include <linux/dma-mapping.h>
#include <linux/io.h>
#include <linux/math64.h>
@@ -145,6 +146,7 @@ struct imxfb_info {
struct clk *clk_ipg;
struct clk *clk_ahb;
struct clk *clk_per;
+ struct regulator *reg_lcd;
enum imxfb_type devtype;
bool enabled;
@@ -563,12 +565,23 @@ static void imxfb_exit_backlight(struct imxfb_info *fbi)
static void imxfb_enable_controller(struct imxfb_info *fbi)
{
+ int ret;
if (fbi->enabled)
return;
pr_debug("Enabling LCD controller\n");
+ if (fbi->reg_lcd) {
+ ret = regulator_enable(fbi->reg_lcd);
+ if (ret) {
+ dev_err(&fbi->pdev->dev,
+ "lcd regulator enable failed with error: %d\n",
+ ret);
+ return;
+ }
+ }
+
writel(fbi->screen_dma, fbi->regs + LCDC_SSA);
/* panning offset 0 (0 pixel offset) */
@@ -597,6 +610,8 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
static void imxfb_disable_controller(struct imxfb_info *fbi)
{
+ int ret;
+
if (!fbi->enabled)
return;
@@ -613,6 +628,14 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
fbi->enabled = false;
writel(0, fbi->regs + LCDC_RMCR);
+
+ if (fbi->reg_lcd) {
+ ret = regulator_disable(fbi->reg_lcd);
+ if (ret)
+ dev_err(&fbi->pdev->dev,
+ "lcd regulator disable failed with error: %d\n",
+ ret);
+ }
}
static int imxfb_blank(int blank, struct fb_info *info)
@@ -1020,6 +1043,12 @@ static int imxfb_probe(struct platform_device *pdev)
goto failed_register;
}
+ fbi->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
+ if (IS_ERR(fbi->reg_lcd)) {
+ dev_info(&pdev->dev, "No lcd regulator used.\n");
+ fbi->reg_lcd = NULL;
+ }
+
imxfb_enable_controller(fbi);
fbi->pdev = pdev;
#ifdef PWMR_BACKLIGHT_AVAILABLE
--
1.7.9.5
^ permalink raw reply related
* [PATCH] drm/omap: fix (un)registering irqs inside an irq handler
From: Tomi Valkeinen @ 2013-10-24 6:50 UTC (permalink / raw)
To: dri-devel; +Cc: linux-fbdev, Tomi Valkeinen
omapdrm (un)registers irqs inside an irq handler. The problem is that
the (un)register function uses dispc_runtime_get/put() to enable the
clocks, and those functions are not irq safe by default.
This was kind of fixed in 48664b21aeeffb40c5fa06843f18052e2c4ec9ef
(OMAPDSS: DISPC: set irq_safe for runtime PM), which makes dispc's
runtime calls irq-safe.
However, using pm_runtime_irq_safe in dispc makes the parent of dispc,
dss, always enabled, effectively preventing PM for the whole DSS module.
This patch makes omapdrm behave better by adding new irq (un)register
functions that do not use dispc_runtime_get/put, and using those
functions in interrupt context. Thus we can make dispc again
non-irq-safe, allowing proper PM.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Rob Clark <robdclark@gmail.com>
---
drivers/gpu/drm/omapdrm/omap_crtc.c | 6 +++---
drivers/gpu/drm/omapdrm/omap_drv.h | 2 ++
drivers/gpu/drm/omapdrm/omap_irq.c | 22 ++++++++++++++++++----
drivers/video/omap2/dss/dispc.c | 1 -
4 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 0fd2eb1..e6241c2 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -411,7 +411,7 @@ static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
struct drm_crtc *crtc = &omap_crtc->base;
DRM_ERROR("%s: errors: %08x\n", omap_crtc->name, irqstatus);
/* avoid getting in a flood, unregister the irq until next vblank */
- omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
+ __omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
}
static void omap_crtc_apply_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
@@ -421,13 +421,13 @@ static void omap_crtc_apply_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
struct drm_crtc *crtc = &omap_crtc->base;
if (!omap_crtc->error_irq.registered)
- omap_irq_register(crtc->dev, &omap_crtc->error_irq);
+ __omap_irq_register(crtc->dev, &omap_crtc->error_irq);
if (!dispc_mgr_go_busy(omap_crtc->channel)) {
struct omap_drm_private *priv crtc->dev->dev_private;
DBG("%s: apply done", omap_crtc->name);
- omap_irq_unregister(crtc->dev, &omap_crtc->apply_irq);
+ __omap_irq_unregister(crtc->dev, &omap_crtc->apply_irq);
queue_work(priv->wq, &omap_crtc->apply_work);
}
}
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 30b95b7..cfb6c2e 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -145,6 +145,8 @@ irqreturn_t omap_irq_handler(DRM_IRQ_ARGS);
void omap_irq_preinstall(struct drm_device *dev);
int omap_irq_postinstall(struct drm_device *dev);
void omap_irq_uninstall(struct drm_device *dev);
+void __omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq);
+void __omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq);
void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq);
void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq);
int omap_drm_irq_uninstall(struct drm_device *dev);
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index 9263db1..b08b902 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -45,12 +45,11 @@ static void omap_irq_update(struct drm_device *dev)
dispc_read_irqenable(); /* flush posted write */
}
-void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
+void __omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
{
struct omap_drm_private *priv = dev->dev_private;
unsigned long flags;
- dispc_runtime_get();
spin_lock_irqsave(&list_lock, flags);
if (!WARN_ON(irq->registered)) {
@@ -60,14 +59,21 @@ void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
}
spin_unlock_irqrestore(&list_lock, flags);
+}
+
+void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
+{
+ dispc_runtime_get();
+
+ __omap_irq_register(dev, irq);
+
dispc_runtime_put();
}
-void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
+void __omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
{
unsigned long flags;
- dispc_runtime_get();
spin_lock_irqsave(&list_lock, flags);
if (!WARN_ON(!irq->registered)) {
@@ -77,6 +83,14 @@ void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
}
spin_unlock_irqrestore(&list_lock, flags);
+}
+
+void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
+{
+ dispc_runtime_get();
+
+ __omap_irq_unregister(dev, irq);
+
dispc_runtime_put();
}
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 4779750..02a7340 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3691,7 +3691,6 @@ static int __init omap_dispchw_probe(struct platform_device *pdev)
}
pm_runtime_enable(&pdev->dev);
- pm_runtime_irq_safe(&pdev->dev);
r = dispc_runtime_get();
if (r)
--
1.8.1.2
^ permalink raw reply related
* [PATCHv3][ 5/5] ARM: dts: mbimxsd35 Add video and displays support.
From: Denis Carikli @ 2013-10-23 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382532229-32755-1-git-send-email-denis@eukrea.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
ChangeLog v2->v3:
- The dts were adapted to the new DT bindings which looks more like the IPUv3
ones.
---
.../imx35-eukrea-mbimxsd35-baseboard-cmo-qvga.dts | 61 ++++++++++++++++++++
.../imx35-eukrea-mbimxsd35-baseboard-dvi-svga.dts | 51 ++++++++++++++++
.../imx35-eukrea-mbimxsd35-baseboard-dvi-vga.dts | 51 ++++++++++++++++
3 files changed, 163 insertions(+)
create mode 100644 arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-cmo-qvga.dts
create mode 100644 arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-svga.dts
create mode 100644 arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-vga.dts
diff --git a/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-cmo-qvga.dts b/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-cmo-qvga.dts
new file mode 100644
index 0000000..6d186ca
--- /dev/null
+++ b/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-cmo-qvga.dts
@@ -0,0 +1,61 @@
+/*
+ * Copyright 2013 Eukréa Electromatique <denis@eukrea.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+
+#include "imx35-eukrea-mbimxsd35-baseboard.dts"
+
+/ {
+ model = "Eukrea MBIMXSD35 with the CMO-QVGA Display";
+ compatible = "eukrea,mbimxsd35-baseboard-cmo-qvga", "eukrea,mbimxsd35-baseboard", "eukrea,cpuimx35", "fsl,imx35";
+
+ display@di0 {
+ regulator-name = "lcd";
+ interface-pix-fmt = "rgb666";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lcdc_1>;
+ model = "CMO-QVGA";
+ display-timings {
+ qvga_timings: 320x240 {
+ clock-frequency = <6500000>;
+ hactive = <320>;
+ vactive = <240>;
+ hback-porch = <68>;
+ hfront-porch = <20>;
+ vback-porch = <15>;
+ vfront-porch = <4>;
+ hsync-len = <30>;
+ vsync-len = <3>;
+ };
+ };
+ };
+
+ reg_lcd_3v3: lcd-en {
+ compatible = "regulator-fixed";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_reg_lcd_3v3>;
+ regulator-name = "lcd";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ gpio = <&gpio1 4 0>;
+ enable-active-high;
+ };
+};
+
+&lcdc {
+ lcd-supply = <®_lcd_3v3>;
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-svga.dts b/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-svga.dts
new file mode 100644
index 0000000..ccf5f48
--- /dev/null
+++ b/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-svga.dts
@@ -0,0 +1,51 @@
+/*
+ * Copyright 2013 Eukréa Electromatique <denis@eukrea.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+
+#include "imx35-eukrea-mbimxsd35-baseboard.dts"
+
+/ {
+ model = "Eukrea MBIMXSD35 with the DVI-SVGA Display";
+ compatible = "eukrea,mbimxsd35-baseboard-dvi-svga", "eukrea,mbimxsd35-baseboard", "eukrea,cpuimx35", "fsl,imx35";
+ display@di0 {
+ interface-pix-fmt = "rgb666";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lcdc_1>;
+ model = "DVI-SVGA";
+ display-timings {
+ svga_timings: 800x600 {
+ clock-frequency = <40000000>;
+ hactive = <800>;
+ vactive = <600>;
+ hback-porch = <75>;
+ hfront-porch = <75>;
+ vback-porch = <7>;
+ vfront-porch = <75>;
+ hsync-len = <7>;
+ vsync-len = <7>;
+ hsync-active = <1>;
+ vsync-active = <1>;
+ de-active = <1>;
+ pixelclk-active = <0>;
+ };
+ };
+ };
+};
+
+&lcdc {
+ status = "okay";
+};
diff --git a/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-vga.dts b/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-vga.dts
new file mode 100644
index 0000000..6e31684
--- /dev/null
+++ b/arch/arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard-dvi-vga.dts
@@ -0,0 +1,51 @@
+/*
+ * Copyright 2013 Eukréa Electromatique <denis@eukrea.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301, USA.
+ */
+
+#include "imx35-eukrea-mbimxsd35-baseboard.dts"
+
+/ {
+ model = "Eukrea MBIMXSD35 with the DVI-VGA Display";
+ compatible = "eukrea,mbimxsd35-baseboard-dvi-vga", "eukrea,mbimxsd35-baseboard", "eukrea,cpuimx35", "fsl,imx35";
+ display@di0 {
+ interface-pix-fmt = "rgb666";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lcdc_1>;
+ model = "DVI-VGA";
+ display-timings {
+ vga_timings: 640x480 {
+ clock-frequency = <31250000>;
+ hactive = <640>;
+ vactive = <480>;
+ hback-porch = <100>;
+ hfront-porch = <100>;
+ vback-porch = <7>;
+ vfront-porch = <100>;
+ hsync-len = <7>;
+ vsync-len = <7>;
+ hsync-active = <1>;
+ vsync-active = <1>;
+ de-active = <1>;
+ pixelclk-active = <0>;
+ };
+ };
+ };
+};
+
+&lcdc {
+ status = "okay";
+};
--
1.7.9.5
^ permalink raw reply related
* [PATCHv3][ 4/5] video: mx3fb: Introduce regulator support.
From: Denis Carikli @ 2013-10-23 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382532229-32755-1-git-send-email-denis@eukrea.com>
This commit is based on the following commit by Fabio Estevam:
4344429 video: mxsfb: Introduce regulator support
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
ChangeLog v2->v3:
- The prints are now replaced with non line wrapped prints.
- The regulator retrival has been adapted to the new DT bindings which looks
more like the IPUv3 ones.
- The regulator_is_enabled checks were kept, because regulator_disable do not
do such check.
---
drivers/video/mx3fb.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index de5a6c8..1f2ce6d 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -27,6 +27,7 @@
#include <linux/clk.h>
#include <linux/mutex.h>
#include <linux/dma/ipu-dma.h>
+#include <linux/regulator/consumer.h>
#include <linux/platform_data/dma-imx.h>
#include <linux/platform_data/video-mx3fb.h>
@@ -269,6 +270,7 @@ struct mx3fb_info {
struct dma_async_tx_descriptor *txd;
dma_cookie_t cookie;
struct scatterlist sg[2];
+ struct regulator *reg_lcd;
struct fb_var_screeninfo cur_var; /* current var info */
};
@@ -1005,6 +1007,7 @@ static void __blank(int blank, struct fb_info *fbi)
struct mx3fb_info *mx3_fbi = fbi->par;
struct mx3fb_data *mx3fb = mx3_fbi->mx3fb;
int was_blank = mx3_fbi->blank;
+ int ret;
mx3_fbi->blank = blank;
@@ -1023,6 +1026,15 @@ static void __blank(int blank, struct fb_info *fbi)
case FB_BLANK_HSYNC_SUSPEND:
case FB_BLANK_NORMAL:
sdc_set_brightness(mx3fb, 0);
+ if (mx3_fbi->reg_lcd) {
+ if (regulator_is_enabled(mx3_fbi->reg_lcd)) {
+ ret = regulator_disable(mx3_fbi->reg_lcd);
+ if (ret)
+ dev_warn(fbi->device,
+ "lcd regulator disable failed with error: %d\n",
+ ret);
+ }
+ }
memset((char *)fbi->screen_base, 0, fbi->fix.smem_len);
/* Give LCD time to update - enough for 50 and 60 Hz */
msleep(25);
@@ -1030,6 +1042,15 @@ static void __blank(int blank, struct fb_info *fbi)
break;
case FB_BLANK_UNBLANK:
sdc_enable_channel(mx3_fbi);
+ if (mx3_fbi->reg_lcd) {
+ if (!regulator_is_enabled(mx3_fbi->reg_lcd)) {
+ ret = regulator_enable(mx3_fbi->reg_lcd);
+ if (ret)
+ dev_warn(fbi->device,
+ "lcd regulator enable failed with error: %d\n",
+ ret);
+ }
+ }
sdc_set_brightness(mx3fb, mx3fb->backlight_level);
break;
}
@@ -1206,6 +1227,7 @@ static int mx3fb_suspend(struct platform_device *pdev, pm_message_t state)
{
struct mx3fb_data *mx3fb = platform_get_drvdata(pdev);
struct mx3fb_info *mx3_fbi = mx3fb->fbi->par;
+ int ret;
console_lock();
fb_set_suspend(mx3fb->fbi, 1);
@@ -1214,7 +1236,15 @@ static int mx3fb_suspend(struct platform_device *pdev, pm_message_t state)
if (mx3_fbi->blank = FB_BLANK_UNBLANK) {
sdc_disable_channel(mx3_fbi);
sdc_set_brightness(mx3fb, 0);
-
+ if (mx3_fbi->reg_lcd) {
+ if (regulator_is_enabled(mx3_fbi->reg_lcd)) {
+ ret = regulator_disable(mx3_fbi->reg_lcd);
+ if (ret)
+ dev_warn(&pdev->dev,
+ "lcd regulator disable failed with error: %d\n",
+ ret);
+ }
+ }
}
return 0;
}
@@ -1226,10 +1256,20 @@ static int mx3fb_resume(struct platform_device *pdev)
{
struct mx3fb_data *mx3fb = platform_get_drvdata(pdev);
struct mx3fb_info *mx3_fbi = mx3fb->fbi->par;
+ int ret;
if (mx3_fbi->blank = FB_BLANK_UNBLANK) {
sdc_enable_channel(mx3_fbi);
sdc_set_brightness(mx3fb, mx3fb->backlight_level);
+ if (mx3_fbi->reg_lcd) {
+ if (!regulator_is_enabled(mx3_fbi->reg_lcd)) {
+ ret = regulator_enable(mx3_fbi->reg_lcd);
+ if (ret)
+ dev_warn(&pdev->dev,
+ "lcd regulator enable failed with error: %d\n",
+ ret);
+ }
+ }
}
console_lock();
@@ -1373,6 +1413,7 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
struct mx3fb_platform_data *mx3fb_pdata = dev_get_platdata(dev);
struct device_node *np = dev->of_node;
const char *name;
+ const char *regulator_name;
const char *ipu_disp_format;
unsigned int irq;
struct fb_info *fbi;
@@ -1395,6 +1436,9 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
return -EINVAL;
}
+ of_property_read_string(display_np, "regulator-name",
+ ®ulator_name);
+
of_property_read_string(display_np, "interface-pix-fmt",
&ipu_disp_format);
if (!ipu_disp_format) {
@@ -1509,6 +1553,24 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
if (ret < 0)
goto erfb;
+ /* In dt mode,
+ * using devm_regulator_get would require that the proprety referencing
+ * the regulator phandle has to be inside the mx3fb node.
+ */
+ if (np) {
+ if (regulator_name)
+ mx3fbi->reg_lcd = regulator_get(NULL, regulator_name);
+ } else {
+ mx3fbi->reg_lcd = devm_regulator_get(dev, "lcd");
+ }
+
+ if (IS_ERR(mx3fbi->reg_lcd)) {
+ dev_warn(mx3fb->dev, "Operating without regulator \"lcd\"\n");
+ mx3fbi->reg_lcd = NULL;
+ } else {
+ dev_info(mx3fb->dev, "Using \"lcd\" Regulator\n");
+ }
+
return 0;
erfb:
@@ -1575,6 +1637,7 @@ static int mx3fb_probe(struct platform_device *pdev)
dma_cap_mask_t mask;
struct dma_chan *chan;
struct dma_chan_request rq;
+ struct mx3fb_info *mx3_fbi;
/*
* Display Interface (DI) and Synchronous Display Controller (SDC)
@@ -1630,6 +1693,8 @@ ersdc0:
dmaengine_put();
iounmap(mx3fb->reg_base);
eremap:
+ mx3_fbi = mx3fb->fbi->par;
+ regulator_put(mx3_fbi->reg_lcd);
kfree(mx3fb);
dev_err(dev, "mx3fb: failed to register fb\n");
return ret;
--
1.7.9.5
^ permalink raw reply related
* [PATCHv3][ 3/5] video: mx3fb: Add device tree suport.
From: Denis Carikli @ 2013-10-23 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382532229-32755-1-git-send-email-denis@eukrea.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
---
ChangeLog v2->v3:
- The device tree bindings were reworked in order to make it look more like the
IPUv3 bindings.
- The interface_pix_fmt property now looks like the IPUv3 one.
---
.../devicetree/bindings/video/fsl,mx3-fb.txt | 35 ++++++
drivers/video/Kconfig | 2 +
drivers/video/mx3fb.c | 125 +++++++++++++++++---
3 files changed, 147 insertions(+), 15 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
diff --git a/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
new file mode 100644
index 0000000..0b31374
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
@@ -0,0 +1,35 @@
+Freescale MX3 fb
+========
+
+Required properties:
+- compatible: Should be "fsl,mx3fb". compatible chips include the imx31 and the
+ imx35.
+- reg: should be register base and length as documented in the datasheet.
+- clocks: Handle to the ipu_gate clock.
+
+Example:
+
+lcdc: mx3fb@53fc00b4 {
+ compatible = "fsl,mx3-fb";
+ reg = <0x53fc00b4 0x0b>;
+ clocks = <&clks 55>;
+};
+
+Display support
+=======+Required properties:
+- model : The user-visible name of the display.
+
+Optional properties:
+- interface_pix_fmt: How this display is connected to the
+ crtc. Currently supported types: "rgb24", "rgb565", "rgb666"
+
+It can also have an optional timing subnode as described in
+ Documentation/devicetree/bindings/video/display-timing.txt.
+
+Example:
+
+display@di0 {
+ interface-pix-fmt = "rgb666";
+ model = "CMO-QVGA";
+};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 14317b7..2a638df 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2359,6 +2359,8 @@ config FB_MX3
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
+ select VIDEOMODE_HELPERS
+ select FB_MODE_HELPERS
default y
help
This is a framebuffer device for the i.MX31 LCD Controller. So
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index 804f874..de5a6c8 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -31,6 +31,8 @@
#include <linux/platform_data/dma-imx.h>
#include <linux/platform_data/video-mx3fb.h>
+#include <video/of_display_timing.h>
+
#include <asm/io.h>
#include <asm/uaccess.h>
@@ -757,11 +759,13 @@ static int __set_par(struct fb_info *fbi, bool lock)
sig_cfg.Hsync_pol = true;
if (fbi->var.sync & FB_SYNC_VERT_HIGH_ACT)
sig_cfg.Vsync_pol = true;
- if (fbi->var.sync & FB_SYNC_CLK_INVERT)
+ if ((fbi->var.sync & FB_SYNC_CLK_INVERT) ||
+ (fbi->var.sync & FB_SYNC_PIXDAT_HIGH_ACT))
sig_cfg.clk_pol = true;
if (fbi->var.sync & FB_SYNC_DATA_INVERT)
sig_cfg.data_pol = true;
- if (fbi->var.sync & FB_SYNC_OE_ACT_HIGH)
+ if ((fbi->var.sync & FB_SYNC_OE_ACT_HIGH) ||
+ (fbi->var.sync & FB_SYNC_DE_HIGH_ACT))
sig_cfg.enable_pol = true;
if (fbi->var.sync & FB_SYNC_CLK_IDLE_EN)
sig_cfg.clkidle_en = true;
@@ -1351,21 +1355,75 @@ static struct fb_info *mx3fb_init_fbinfo(struct device *dev, struct fb_ops *ops)
return fbi;
}
+static int match_dt_disp_data(const char *property)
+{
+ if (!strcmp("rgb666", property))
+ return IPU_DISP_DATA_MAPPING_RGB666;
+ else if (!strcmp("rgb565", property))
+ return IPU_DISP_DATA_MAPPING_RGB565;
+ else if (!strcmp("rgb24", property))
+ return IPU_DISP_DATA_MAPPING_RGB888;
+ else
+ return -EINVAL;
+}
+
static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
{
struct device *dev = mx3fb->dev;
struct mx3fb_platform_data *mx3fb_pdata = dev_get_platdata(dev);
- const char *name = mx3fb_pdata->name;
+ struct device_node *np = dev->of_node;
+ const char *name;
+ const char *ipu_disp_format;
unsigned int irq;
struct fb_info *fbi;
struct mx3fb_info *mx3fbi;
const struct fb_videomode *mode;
int ret, num_modes;
+ struct fb_videomode of_mode;
+ struct device_node *display_np, *root_np;
+
+ if (np) {
+ root_np = of_find_node_by_path("/");
+ if (!root_np) {
+ dev_err(dev, "Can't get the \"/\" node.\n");
+ return -EINVAL;
+ }
+
+ display_np = of_find_node_by_name(root_np, "display");
+ if (!display_np) {
+ dev_err(dev, "Can't get the display device node.\n");
+ return -EINVAL;
+ }
+
+ of_property_read_string(display_np, "interface-pix-fmt",
+ &ipu_disp_format);
+ if (!ipu_disp_format) {
+ mx3fb->disp_data_fmt = IPU_DISP_DATA_MAPPING_RGB666;
+ dev_warn(dev,
+ "ipu display data mapping was not defined, using the default rgb666.\n");
+ } else {
+ mx3fb->disp_data_fmt + match_dt_disp_data(ipu_disp_format);
+ }
+
+ if (mx3fb->disp_data_fmt = -EINVAL) {
+ dev_err(dev, "Illegal display data format \"%s\"\n",
+ ipu_disp_format);
+ return -EINVAL;
+ }
- if (mx3fb_pdata->disp_data_fmt >= ARRAY_SIZE(di_mappings)) {
- dev_err(dev, "Illegal display data format %d\n",
+ of_property_read_string(display_np, "model", &name);
+ if (ret) {
+ dev_err(dev, "Missing display model name\n");
+ return -EINVAL;
+ }
+ } else {
+ name = mx3fb_pdata->name;
+ if (mx3fb_pdata->disp_data_fmt >= ARRAY_SIZE(di_mappings)) {
+ dev_err(dev, "Illegal display data format %d\n",
mx3fb_pdata->disp_data_fmt);
- return -EINVAL;
+ return -EINVAL;
+ }
}
ichan->client = mx3fb;
@@ -1386,12 +1444,24 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
goto emode;
}
- if (mx3fb_pdata->mode && mx3fb_pdata->num_modes) {
- mode = mx3fb_pdata->mode;
- num_modes = mx3fb_pdata->num_modes;
+ if (np) {
+ ret = of_get_fb_videomode(display_np, &of_mode,
+ OF_USE_NATIVE_MODE);
+ if (!ret) {
+ mode = &of_mode;
+ num_modes = 1;
+ } else {
+ mode = mx3fb_modedb;
+ num_modes = ARRAY_SIZE(mx3fb_modedb);
+ }
} else {
- mode = mx3fb_modedb;
- num_modes = ARRAY_SIZE(mx3fb_modedb);
+ if (mx3fb_pdata->mode || !mx3fb_pdata->num_modes) {
+ mode = mx3fb_pdata->mode;
+ num_modes = mx3fb_pdata->num_modes;
+ } else {
+ mode = mx3fb_modedb;
+ num_modes = ARRAY_SIZE(mx3fb_modedb);
+ }
}
if (!fb_find_mode(&fbi->var, fbi, fb_mode, mode,
@@ -1421,7 +1491,8 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
mx3fbi->mx3fb = mx3fb;
mx3fbi->blank = FB_BLANK_NORMAL;
- mx3fb->disp_data_fmt = mx3fb_pdata->disp_data_fmt;
+ if (!np)
+ mx3fb->disp_data_fmt = mx3fb_pdata->disp_data_fmt;
init_completion(&mx3fbi->flip_cmpl);
disable_irq(ichan->eof_irq);
@@ -1449,13 +1520,26 @@ emode:
return ret;
}
+static int imx_dma_is_dt_ipu(struct dma_chan *chan)
+{
+ /* Example: 53fc0000.ipu */
+ if (chan && chan->device && chan->device->dev) {
+ char *name = dev_name(chan->device->dev);
+ name = strchr(name, '.');
+ if (name)
+ return !strcmp(name, ".ipu");
+ }
+
+ return 0;
+}
+
static bool chan_filter(struct dma_chan *chan, void *arg)
{
struct dma_chan_request *rq = arg;
struct device *dev;
struct mx3fb_platform_data *mx3fb_pdata;
- if (!imx_dma_is_ipu(chan))
+ if (!imx_dma_is_ipu(chan) && !imx_dma_is_dt_ipu(chan))
return false;
if (!rq)
@@ -1464,8 +1548,12 @@ static bool chan_filter(struct dma_chan *chan, void *arg)
dev = rq->mx3fb->dev;
mx3fb_pdata = dev_get_platdata(dev);
- return rq->id = chan->chan_id &&
- mx3fb_pdata->dma_dev = chan->device->dev;
+ /* When using the devicetree, mx3fb_pdata is NULL */
+ if (imx_dma_is_dt_ipu(chan))
+ return rq->id = chan->chan_id;
+ else
+ return rq->id = chan->chan_id &&
+ mx3fb_pdata->dma_dev = chan->device->dev;
}
static void release_fbi(struct fb_info *fbi)
@@ -1565,9 +1653,16 @@ static int mx3fb_remove(struct platform_device *dev)
return 0;
}
+static struct of_device_id mx3fb_of_dev_id[] = {
+ { .compatible = "fsl,mx3-fb", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mx3fb_of_dev_id);
+
static struct platform_driver mx3fb_driver = {
.driver = {
.name = MX3FB_NAME,
+ .of_match_table = mx3fb_of_dev_id,
.owner = THIS_MODULE,
},
.probe = mx3fb_probe,
--
1.7.9.5
^ permalink raw reply related
* [PATCHv3][ 1/5] fbdev: Add the lacking FB_SYNC_* for matching the DISPLAY_FLAGS_*
From: Denis Carikli @ 2013-10-23 12:43 UTC (permalink / raw)
To: linux-arm-kernel
Without that fix, drivers using the fb_videomode_from_videomode
function will not be able to get certain information because
some DISPLAY_FLAGS_* have no corresponding FB_SYNC_*.
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
ChangeLog v2->v3:
- Added Jean-Christophe PLAGNIOL-VILLARD's ACK.
---
drivers/video/fbmon.c | 4 ++++
include/uapi/linux/fb.h | 2 ++
2 files changed, 6 insertions(+)
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index 6103fa6..29a9ed0 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -1402,6 +1402,10 @@ int fb_videomode_from_videomode(const struct videomode *vm,
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
if (vm->flags & DISPLAY_FLAGS_VSYNC_HIGH)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+ if (vm->flags & DISPLAY_FLAGS_DE_HIGH)
+ fbmode->sync |= FB_SYNC_DE_HIGH_ACT;
+ if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
+ fbmode->sync |= FB_SYNC_PIXDAT_HIGH_ACT;
if (vm->flags & DISPLAY_FLAGS_INTERLACED)
fbmode->vmode |= FB_VMODE_INTERLACED;
if (vm->flags & DISPLAY_FLAGS_DOUBLESCAN)
diff --git a/include/uapi/linux/fb.h b/include/uapi/linux/fb.h
index fb795c3..30487df 100644
--- a/include/uapi/linux/fb.h
+++ b/include/uapi/linux/fb.h
@@ -215,6 +215,8 @@ struct fb_bitfield {
/* vtotal = 144d/288n/576i => PAL */
/* vtotal = 121d/242n/484i => NTSC */
#define FB_SYNC_ON_GREEN 32 /* sync on green */
+#define FB_SYNC_DE_HIGH_ACT 64 /* data enable high active */
+#define FB_SYNC_PIXDAT_HIGH_ACT 64 /* data enable high active */
#define FB_VMODE_NONINTERLACED 0 /* non interlaced */
#define FB_VMODE_INTERLACED 1 /* interlaced */
--
1.7.9.5
^ permalink raw reply related
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-23 11:34 UTC (permalink / raw)
To: Thierry Reding
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20131023105821.GC15082@ulmo.nvidia.com>
On 10/23/2013 06:58 PM, Thierry Reding wrote:
> On Wed, Oct 23, 2013 at 06:46:17PM +0800, Mark Zhang wrote:
>> On 10/23/2013 06:36 PM, Mark Zhang wrote:
>>> On 10/23/2013 05:09 PM, Thierry Reding wrote:
>>>> On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
>>>>> On 10/23/2013 04:00 PM, Thierry Reding wrote:
>>>>>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
>>>>>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
>>>>>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
>>>>>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
>>>>>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
>>>>>>>>> [...]
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
>>>>>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
>>>>>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
>>>>>>>>>>> how to do that, unless we define the platform data explicitly.
>>>>>>>>>>
>>>>>>>>>> Okay, my question should have been what you need the functions for and
>>>>>>>>>> why you think you need them.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If I understanding you correctly, I suppose I've said that: "because I
>>>>>>>>> want to tune the brightness value before the pwm-bl sets the brightness
>>>>>>>>> to hardware".
>>>>>>>>
>>>>>>>> Why do you want to tune the brightness value? What are you trying to
>>>>>>>> achieve?
>>>>>>>>
>>>>>>>
>>>>>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
>>>>>>> color value to make the display looks bright so that we can reduce the
>>>>>>> backlight brightness to save power. So everytime PRISM is triggered, we
>>>>>>> call "backlight_update_status", then in the "notify" callback, we change
>>>>>>> the brightness value which pwm-bl provides by considering the PRISM
>>>>>>> compensations.
>>>>>>
>>>>>> If you automatically call backlight_update_status() everytime PRISM
>>>>>> gives you new data, can't you just pass the correct value in in the
>>>>>> first place so that you don't have to tweak it in the .notify()
>>>>>> callback?
>>>>>
>>>>> OK, how to do that? -- "pass the correct value in in the first place"?
>>>>
>>>> Well, if you call backlight_update_status(), then you can pass in a
>>>> brightness value, right? You usually do that by setting the backlight's
>>>> props.brightness field.
>>>>
>>>> So when PRISM gives you new data, you could just read out the current
>>>> brightness, compute the new one based on the current one and the PRISM
>>>> data, set the props.brightness field to that value and then call
>>>> backlight_update_status().
>>>>
>>>
>>> Okay, anyway, I got the idea. Actually it's simpler. :)
>>> I'm just curious that if we can do that in this way, why we need
>>> "notify" anymore?
>>
>> Oh, by the way, how about "check_fb" fops? Is there some kind of
>> substitution as well? I mean, if I wanna set "check_fb" and also want to
>> define the backlight device via DT, is there a way to do that?
>
> No, there's no substitution for that. .check_fb() is used to verify that
> a backlight device is associated with a framebuffer device. There are
> better ways to check for that with DT, although nothing has been merged
> into mainline yet.
>
Okay, thanks for the explanation.
Mark
> Thierry
>
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-23 11:33 UTC (permalink / raw)
To: Thierry Reding
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20131023105456.GB15082@ulmo.nvidia.com>
On 10/23/2013 06:54 PM, Thierry Reding wrote:
> On Wed, Oct 23, 2013 at 06:31:30PM +0800, Mark Zhang wrote:
>> On 10/23/2013 05:09 PM, Thierry Reding wrote:
>>> On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
>>>> On 10/23/2013 04:00 PM, Thierry Reding wrote:
>>>>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
>>>>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
>>>>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
>>>>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
>>>>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
>>>>>>>> [...]
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
>>>>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
>>>>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
>>>>>>>>>> how to do that, unless we define the platform data explicitly.
>>>>>>>>>
>>>>>>>>> Okay, my question should have been what you need the functions for and
>>>>>>>>> why you think you need them.
>>>>>>>>>
>>>>>>>>
>>>>>>>> If I understanding you correctly, I suppose I've said that: "because I
>>>>>>>> want to tune the brightness value before the pwm-bl sets the brightness
>>>>>>>> to hardware".
>>>>>>>
>>>>>>> Why do you want to tune the brightness value? What are you trying to
>>>>>>> achieve?
>>>>>>>
>>>>>>
>>>>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
>>>>>> color value to make the display looks bright so that we can reduce the
>>>>>> backlight brightness to save power. So everytime PRISM is triggered, we
>>>>>> call "backlight_update_status", then in the "notify" callback, we change
>>>>>> the brightness value which pwm-bl provides by considering the PRISM
>>>>>> compensations.
>>>>>
>>>>> If you automatically call backlight_update_status() everytime PRISM
>>>>> gives you new data, can't you just pass the correct value in in the
>>>>> first place so that you don't have to tweak it in the .notify()
>>>>> callback?
>>>>
>>>> OK, how to do that? -- "pass the correct value in in the first place"?
>>>
>>> Well, if you call backlight_update_status(), then you can pass in a
>>> brightness value, right? You usually do that by setting the backlight's
>>> props.brightness field.
>>>
>>> So when PRISM gives you new data, you could just read out the current
>>> brightness, compute the new one based on the current one and the PRISM
>>> data, set the props.brightness field to that value and then call
>>> backlight_update_status().
>>>
>>
>> The param of "backlight_update_status" is "struct backlight_device *".
>> So you mean after I get a pointer of correct backlight device, just set
>> the brightness value I want to it's "props.brightness" directly?
>
> Yes.
>
>> I think the "struct backlight_device" should be opaque(although it's
>> definition is in include/linux/backlight.h, I know that), so it's better
>> not to touch it's member directly, that's why I wanna use that "notify"
>> callback.
>
> Well, backlight_update_status() is the only way to change the brightness
> of a backlight. If nobody ever calls backlight_update_status() then the
> .notify() callback will never be called either.
>
Yes, so this is exactly my understanding before I posted this thread. I
think "struct backlight_device" is a handle, we get it and call
"backlight_update_status", then if we wanna change it's default behavior
or brightness value, we overwrite the fops it provides.
> By the way, what method do you use to control the backlight brightness?
> Do you use the sysfs interface from userspace?
>
Yes.
Mark
> Thierry
>
^ permalink raw reply
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Thierry Reding @ 2013-10-23 10:58 UTC (permalink / raw)
To: Mark Zhang
Cc: rpurdie, jg1.han, Jean-Christophe PLAGNIOL-VILLARD,
tomi.valkeinen, linux-pwm, linux-fbdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <5267A8F9.9050907@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]
On Wed, Oct 23, 2013 at 06:46:17PM +0800, Mark Zhang wrote:
> On 10/23/2013 06:36 PM, Mark Zhang wrote:
> > On 10/23/2013 05:09 PM, Thierry Reding wrote:
> >> On Wed, Oct 23, 2013 at 04:49:29PM +0800, Mark Zhang wrote:
> >>> On 10/23/2013 04:00 PM, Thierry Reding wrote:
> >>>> On Wed, Oct 23, 2013 at 10:16:24AM +0800, Mark Zhang wrote:
> >>>>> On 10/22/2013 08:49 PM, Thierry Reding wrote:
> >>>>>> On Tue, Oct 22, 2013 at 04:55:09PM +0800, Mark Zhang wrote:
> >>>>>>> On 10/22/2013 03:24 PM, Thierry Reding wrote:
> >>>>>>>> On Fri, Oct 18, 2013 at 12:48:12PM +0800, Mark Zhang wrote:
> >>>>>>> [...]
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Okay, I just want to set the "notify" function pointer in "struct
> >>>>>>>>> platform_pwm_backlight_data", because I want to tune the brightness
> >>>>>>>>> value before the pwm-bl sets the brightness to hardware. I don't know
> >>>>>>>>> how to do that, unless we define the platform data explicitly.
> >>>>>>>>
> >>>>>>>> Okay, my question should have been what you need the functions for and
> >>>>>>>> why you think you need them.
> >>>>>>>>
> >>>>>>>
> >>>>>>> If I understanding you correctly, I suppose I've said that: "because I
> >>>>>>> want to tune the brightness value before the pwm-bl sets the brightness
> >>>>>>> to hardware".
> >>>>>>
> >>>>>> Why do you want to tune the brightness value? What are you trying to
> >>>>>> achieve?
> >>>>>>
> >>>>>
> >>>>> Oh, Tegra has a feature named PRISM(aka SmartDimmer). It changes the
> >>>>> color value to make the display looks bright so that we can reduce the
> >>>>> backlight brightness to save power. So everytime PRISM is triggered, we
> >>>>> call "backlight_update_status", then in the "notify" callback, we change
> >>>>> the brightness value which pwm-bl provides by considering the PRISM
> >>>>> compensations.
> >>>>
> >>>> If you automatically call backlight_update_status() everytime PRISM
> >>>> gives you new data, can't you just pass the correct value in in the
> >>>> first place so that you don't have to tweak it in the .notify()
> >>>> callback?
> >>>
> >>> OK, how to do that? -- "pass the correct value in in the first place"?
> >>
> >> Well, if you call backlight_update_status(), then you can pass in a
> >> brightness value, right? You usually do that by setting the backlight's
> >> props.brightness field.
> >>
> >> So when PRISM gives you new data, you could just read out the current
> >> brightness, compute the new one based on the current one and the PRISM
> >> data, set the props.brightness field to that value and then call
> >> backlight_update_status().
> >>
> >
> > Okay, anyway, I got the idea. Actually it's simpler. :)
> > I'm just curious that if we can do that in this way, why we need
> > "notify" anymore?
>
> Oh, by the way, how about "check_fb" fops? Is there some kind of
> substitution as well? I mean, if I wanna set "check_fb" and also want to
> define the backlight device via DT, is there a way to do that?
No, there's no substitution for that. .check_fb() is used to verify that
a backlight device is associated with a framebuffer device. There are
better ways to check for that with DT, although nothing has been merged
into mainline yet.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ 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