* [PATCH 6/6] omapdss: hdmi4/hdmi5: set regulator voltage to required level
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja, Somnath Mukherjee
In-Reply-To: <1382009202-18984-1-git-send-email-archit@ti.com>
It's not sufficient to request the regulator. We need to set the IO voltage as
well if the min and max voltage values of the regulator differ.
However, if the regulator on a platform doesn't permit the range we requested
for, or is a fixed voltage regulator, we just give a warning that we didn't get
voltage range we requested for. The recommended voltage range for vdda_hdmi as
per the HDMI PHY spec is 1.8V to 1.98V for OMAP4 and 1.5V to 1.8V for OMAP5.
Originally worked on by Somnath Mukherjee <somnath@ti.com>
Cc: Somnath Mukherjee <somnath@ti.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/hdmi4.c | 5 +++++
drivers/video/omap2/dss/hdmi5.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/drivers/video/omap2/dss/hdmi4.c b/drivers/video/omap2/dss/hdmi4.c
index e140096..95638b8 100644
--- a/drivers/video/omap2/dss/hdmi4.c
+++ b/drivers/video/omap2/dss/hdmi4.c
@@ -83,6 +83,7 @@ static void hdmi_runtime_put(void)
static int hdmi_init_regulator(void)
{
+ int r;
struct regulator *reg;
if (hdmi.vdda_hdmi_dac_reg != NULL)
@@ -99,6 +100,10 @@ static int hdmi_init_regulator(void)
return PTR_ERR(reg);
}
+ r = regulator_set_voltage(reg, 1800000, 1980000);
+ if (r)
+ DSSWARN("can't set regulator voltage");
+
hdmi.vdda_hdmi_dac_reg = reg;
return 0;
diff --git a/drivers/video/omap2/dss/hdmi5.c b/drivers/video/omap2/dss/hdmi5.c
index 5a1cfd6..aa91f4e 100644
--- a/drivers/video/omap2/dss/hdmi5.c
+++ b/drivers/video/omap2/dss/hdmi5.c
@@ -85,6 +85,7 @@ static void hdmi_runtime_put(void)
static int hdmi_init_regulator(void)
{
+ int r;
struct regulator *reg;
if (hdmi.vdda_hdmi_dac_reg != NULL)
@@ -96,6 +97,10 @@ static int hdmi_init_regulator(void)
return PTR_ERR(reg);
}
+ r = regulator_set_voltage(reg, 1500000, 1800000);
+ if (r)
+ DSSWARN("can't set regulator voltage");
+
hdmi.vdda_hdmi_dac_reg = reg;
return 0;
--
1.8.1.2
^ permalink raw reply related
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 11:41 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: <12566222.aRYc4MDoOm@avalon>
[-- Attachment #1: Type: text/plain, Size: 10518 bytes --]
On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
[...]
> CDF needs to model video pipelines that can be more complex than just a
> display controller with a single output connected to a panel with a single
> input. For that reason the DT bindings need to describe connections in a
> generic enough way. The above DT fragment might be slightly more complex than
> one would expect when thinking about the really simple case only, but it's
> nowhere near overly complex in my opinion.
>
> Please note that, when a device has as single port, the ports node can be
> omitted, and the port doesn't need to be numbered. You would then end up with
>
> dc: display-controller {
> port {
> remote-endpoint = <&panel>;
> };
> };
>
> panel: panel {
> port {
> remote-endpoint = <&dc>;
> };
> };
>
> I don't think there's a way to simplify it further.
That binding looks completely inconsistent to me. If we can write it
either using a ports node with port@X subnodes, in my opinion we can
equally well just use a unidirectional link for the case where we
don't need the link back from the panel to the output. Where's the
difference?
Consider for example the case where an LVDS panel is connected to a
simple LVDS output. For what possible reason would the panel need to
access the output?
> > > I also plan to specify video bus parameters in DT for CDF, but this hasn't
> > > been finalized yet.
> >
> > That effectively blocks any of the code that I've been working on until
> > CDF has been merged. It's been discussed for over a year now, and there
> > has even been significant pushback on using CDF in DRM, so even if CDF
> > is eventually merged, that will still leave DRM with no way to turn on
> > a simple panel.
>
> I'm probably even more concerned about the long time all this took and will
> still take than you are, sincerely. I'm obviously to blame for not being able
> to explain the problems and solutions correctly, as I see no way why people
> would push back if they realized that the other solutions that have been
> proposed, far from being future-proof, won't even be present-proof. We would
> be shooting ourselves in the foot, but what can I do when too many people want
> to do so ?
I'm curious why you think this current proposal isn't present-proof. All
three panels that I've tested with work with this proposal currently and
I'm convinced that many more setups can be covered using something as
simple as this.
> 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? 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.
Of course none of that is relevant to the topic of DT bindings, which is
what we should probably be concentrating on. Nobody's been able to come
up with a valid reason why the proposed binding is broken. It works for
the tested devices (likely many more as well) and I don't forsee why it
can't be future-proof. Why would it suddenly need to change? And even if
it needed to change, why would the changes be backwards-incompatible
rather than simply additional features that don't impact existing users?
> Regarding the use of CDF in DRM, please note that it will be optional for
> drivers to switch to the new framework. Nothing should hopefully require
> existing drivers to be converted, drivers that want to make use of CDF to
> support panels will be welcome to do so, but they big DRM drivers for desktop
> GPUs won't be affected.
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.
> > I keep wondering why we absolutely must have compatibility between CDF
> > and this simple panel binding. DT content is supposed to concern itself
> > with the description of hardware only. What about the following isn't an
> > accurate description of the panel hardware?
> >
> > panel: panel {
> > compatible = "cptt,claa101wb01";
> >
> > power-supply = <&vdd_pnl_reg>;
> > enable-gpios = <&gpio 90 0>;
> >
> > backlight = <&backlight>;
> > };
> >
> > dsi-controller {
> > panel = <&panel>;
> > };
> >
> > Note how the above is also not incompatible with anything that the CDF
> > (to be) mandates, so it could easily be extended with any nodes or
> > properties that CDF needs to work properly without breaking backwards-
> > compatibility.
>
> As mentioned above, CDF needs to describe more complex pipelines. To make the
> driver life as simple as possible the core code will handle the pipeline
> description in the DT bindings, and it does require it to be generic enough.
> You can of course describe connections in various ways depending on the
> hardware, which would make the DT-related code needlessly overcomplex. One of
> the issue with the above description is that there's no way to go from the
> panel to the display controller, which might be a show stopper in the future
> when code requires that. A workaround would be to look through the whole DT
> for the reverse link, which would be pretty inefficient.
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.
> > > > +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.
> > > > +static const struct drm_display_mode auo_b101aw03_mode = {
> > > > + .clock = 51450,
> > > > + .hdisplay = 1024,
> > > > + .hsync_start = 1024 + 156,
> > > > + .hsync_end = 1024 + 156 + 8,
> > > > + .htotal = 1024 + 156 + 8 + 156,
> > > > + .vdisplay = 600,
> > > > + .vsync_start = 600 + 16,
> > > > + .vsync_end = 600 + 16 + 6,
> > > > + .vtotal = 600 + 16 + 6 + 16,
> > > > + .vrefresh = 60,
> > > > +};
> > >
> > > 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. 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.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-17 11:50 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131017110517.GC10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]
On 17/10/13 14:05, Thierry Reding wrote:
>> That's quite similar to how my current out-of-mainline OMAP DSS DT
>> bindings work. The difference is that I have a reference from the panel
>> node to the video source (dsi-controller), instead of the other way
>> around. I just find it more natural. It works the same way as, say,
>> regulators, gpios, etc.
>
> I suppose that depends on the way you look at it. In the above proposal
> I consider the output (DSI controller) to use the panel as a resource
> that provides a certain set of services (query mode timings, provide a
> way to enable or disable the panel). Similarly the panel uses the
> backlight as a resource that provides a certain set of services (such as
> changing the brightness).
>
> The above also follows the natural order of control. The panel has no
> way to control the DSI output. However, it is the output that controls
> when a panel is required and turn it on as appropriate.
I've discussed this issue to death with other people, and for some
reason I still see this the other way =).
I see the panel using the DSI controller as a resource. The panel driver
uses the DSI controller to send the video data to the panel device.
Compare this to, say, i2c client driver, which uses i2c master to send
data to the i2c device.
In the OMAP DSS driver model, the panel driver is in control. The panel
driver tells the DSI controller which kind of video timings to use, or
which bus speed to use, or whether to use low-power or high-speed mode,
when to enable the DSI clock or DSI video stream, etc.
With a simple panel the model you describe works usually fine. The
reason I went with the different model is that we had rather complex
display peripherals, that needed more specific control of the bus.
A simple example could be the enable sequence. I presume in your model
the DSI controller's enable would be maybe something like:
configure_dsi_timings(panel->get_timings());
enable_dsi_output();
panel->enable();
What if the DSI peripheral requires an enable sequence of, say:
- enable DSI clock
- use LP mode
- send some config messages
- re-configure DSI clock to higher speed
- send some more config messages
- enable HS mode
- send some more config messages
- enable DSI video stream
It gets a bit difficult to manage that kind of setup in a generic way in
the DSI controller driver. As I see it, the DSI peripheral driver has to
be in control of the nuances. The same goes for other video buses also,
but DSI is perhaps one of the most complex ones to support.
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-17 11:51 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <525FCB95.6070401-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]
Hi Tomi,
On Thursday 17 October 2013 14:35:49 Tomi Valkeinen wrote:
> On 17/10/13 14:02, Laurent Pinchart wrote:
> >> Okay, so if I understand correctly, translating those bindings to panel
> >>
> >> nodes would look somewhat like this:
> >> dc: display-controller {
> >> ports {
> >> port@0 {
> >> remote-endpoint = <&panel>;
> >> };
> >> };
> >> };
> >>
> >> panel: panel {
> >> ports {
> >> port@0 {
> >> remote-endpoint = <&dc>;
> >> };
> >> };
> >> };
> >>
> >> The above leaves out any of the other, non-relevant properties. Does
> >> that sound about right?
> >
> > Yes it does.
>
> It does?
>
> Shouldn't it be something like:
>
> panel {
> ports {
> port@0 {
> endpoint@0 {
> remote = <&dc>;
> };
> };
> };
> };
>
> And simplified:
>
> panel {
> port {
> endpoint@0 {
> remote = <&dc>;
> };
> };
> };
>
> You do need a node for the endpoint, a remote-endpoint property is not
> enough.
My bad, you'r absolutely right. More sleep is needed.
(And while we're at it, the remote-endpoint properties must point to an
endpoint, not the device DT node.
> > Please note that, when a device has as single port, the ports node can be
> > omitted, and the port doesn't need to be numbered. You would then end up
> > with>
> > dc: display-controller {
> > port {
> > remote-endpoint = <&panel>;
> > };
> > };
> >
> > panel: panel {
> > port {
> > remote-endpoint = <&dc>;
> > };
> > };
> >
> > I don't think there's a way to simplify it further.
>
> 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 ?
--
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-17 11:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <4885946.7Zgjf9zNXx@avalon>
[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]
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.
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: Thierry Reding @ 2013-10-17 12:06 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Tomi Valkeinen, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie,
Rob Clark, Daniel Vetter
In-Reply-To: <3610893.n6xJ6DUJ0E@avalon>
[-- Attachment #1: Type: text/plain, Size: 6882 bytes --]
On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
>
> On Thursday 17 October 2013 13:05:18 Thierry Reding wrote:
> > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> > > On 17/10/13 11:53, Thierry Reding wrote:
> > > > I keep wondering why we absolutely must have compatibility between CDF
> > > > and this simple panel binding. DT content is supposed to concern itself
> > > > with the description of hardware only. What about the following isn't an
> > > > accurate description of the panel hardware?
> > > >
> > > > panel: panel {
> > > > compatible = "cptt,claa101wb01";
> > > >
> > > > power-supply = <&vdd_pnl_reg>;
> > > > enable-gpios = <&gpio 90 0>;
> > > >
> > > > backlight = <&backlight>;
> > > > };
> > > >
> > > > dsi-controller {
> > > > panel = <&panel>;
> > > > };
> > >
> > > That's quite similar to how my current out-of-mainline OMAP DSS DT
> > > bindings work. The difference is that I have a reference from the panel
> > > node to the video source (dsi-controller), instead of the other way
> > > around. I just find it more natural. It works the same way as, say,
> > > regulators, gpios, etc.
> >
> > I suppose that depends on the way you look at it. In the above proposal
> > I consider the output (DSI controller) to use the panel as a resource
> > that provides a certain set of services (query mode timings, provide a
> > way to enable or disable the panel). Similarly the panel uses the
> > backlight as a resource that provides a certain set of services (such as
> > changing the brightness).
> >
> > The above also follows the natural order of control. The panel has no
> > way to control the DSI output. However, it is the output that controls
> > when a panel is required and turn it on as appropriate.
>
> I'm no DSI expert, but I know enough about it to be sure that Tomi will
> disagree. DSI panels can have complex power sequences that require the input
> stream to be finely controlled (for instance it might require a clock without
> video frames for some time, switch a GPIO or regulator, send a command to the
> panel, and then only get video frames). For that reason all developers I've
> talked to who have an in-depth knowledge of DSI and DSI panels told me that
> the panel needs to control the video bus, and request the video source to
> start/stop the video stream.
Oh, I'm very well aware of the various flavours of funkiness that DSI
panels come in. But it's wrong to say that the panel needs to control
the video bus. There's simply no way that a panel can actually do that.
It is true, however, that in order to make this work in a maintainable
fashion, the DSI panel *driver* may need to control the DSI bus. That's
an entirely different story.
Again, that's exactly how I2C works as well. I2C slaves, by definition,
do not control the I2C bus. However, I2C client drivers can do so by
using the (master) services provided by the I2C controller.
So for DSI what we could easily do is provide a DSI "adapter" to the
panel when it is attached to the DSI controller. If the DSI adapter
implements all of the required services, then the DSI panel driver is
easily capable of handling all the required special quirks.
I'm Cc'ing Rob and Daniel, both of whom have been working on this
lately. Actually I'm not sure if Daniel was directly involved, but at
least somebody from Intel's team was working on it.
> > > However, one thing unclear is where the panel node should be. You seem
> > > to have a DSI panel here. If the panel is truly not controlled in any
> > > way, maybe having the panel as a normal platform device is ok. But if
> > > the DSI panel is controlled with DSI messages, should it be a child of
> > > the dsi-controller node, the same way i2c peripherals are children of
> > > i2c master?
> >
> > That's one possibility. In this particular case it's not even necessary
> > to use any of DSIs control methods to talk to the panel. The panel only
> > receives the data stream and "just works".
> >
> > Even if the panel were to be controlled via DSI, I think keeping it as
> > a separate device node is equally valid. It's really boils down to what
> > I already said above. The output uses the panel as a resource, similar
> > to how other devices might use a GPIO controller to use a GPIO pin, or
> > an interrupt controller to request an IRQ.
>
> Except that the display controller can also provides resources to the panel.
> For DSI panels that support DSI commands, the control bus is provided by the
> display controller. Much like an I2C device must be a child of its I2C
> controller node, the DSI panel device should then be a child of the display
> controller node.
I don't think resources is the right term here. The DSI controller
provides services. That's traditionally been handled in DT by handing
out phandles to the service providers.
That said I see why it would make sense to make a DSI panel the child of
the DSI controller node, so we agree at least partially. I don't like it
all that much because it makes handling of panels inconsistent across
various types of output and therefore makes it more complicated to
support it drivers, but I'm sure I can live with it if somebody wants to
enforce that.
> > > > 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.
> > >
> > > This sounds good to me.
> > >
> > > Although maybe it'd be good to have the driver compatible with something
> > > like "generic-panel", so that you could have:
> > >
> > > compatible = "foo,specific-panel", "generic-panel";
> > >
> > > and if there's no need for any power/gpio setup for your board, you may
> > > skip adding "foo,specific-panel" support to the panel driver. Later,
> > > somebody else may need to implement fine grained power/gpio support for
> > > "foo,specific-panel", and it would just work.
> > >
> > > Maybe that would help us with the problem of adding support in the
> > > driver for a hundred different simple panels each used only once on a
> > > specific board.
> >
> > Sure, that all sounds like reasonable additions. All of the suggestions
> > are even backwards-compatible and hence can be added when needed without
> > breaking compatibility with existing users of the binding.
>
> What's wrong with moving the three hardcoded modes we already have in the
> driver to DT before pushing this to mainline ?
I think I've answered that in a different subthread.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 12:12 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Laurent Pinchart, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <525FCF07.6070006-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]
On Thu, Oct 17, 2013 at 02:50:31PM +0300, Tomi Valkeinen wrote:
> On 17/10/13 14:05, Thierry Reding wrote:
>
> >> That's quite similar to how my current out-of-mainline OMAP DSS DT
> >> bindings work. The difference is that I have a reference from the panel
> >> node to the video source (dsi-controller), instead of the other way
> >> around. I just find it more natural. It works the same way as, say,
> >> regulators, gpios, etc.
> >
> > I suppose that depends on the way you look at it. In the above proposal
> > I consider the output (DSI controller) to use the panel as a resource
> > that provides a certain set of services (query mode timings, provide a
> > way to enable or disable the panel). Similarly the panel uses the
> > backlight as a resource that provides a certain set of services (such as
> > changing the brightness).
> >
> > The above also follows the natural order of control. The panel has no
> > way to control the DSI output. However, it is the output that controls
> > when a panel is required and turn it on as appropriate.
>
> I've discussed this issue to death with other people, and for some
> reason I still see this the other way =).
>
> I see the panel using the DSI controller as a resource. The panel driver
> uses the DSI controller to send the video data to the panel device.
> Compare this to, say, i2c client driver, which uses i2c master to send
> data to the i2c device.
>
> In the OMAP DSS driver model, the panel driver is in control. The panel
> driver tells the DSI controller which kind of video timings to use, or
> which bus speed to use, or whether to use low-power or high-speed mode,
> when to enable the DSI clock or DSI video stream, etc.
>
> With a simple panel the model you describe works usually fine. The
> reason I went with the different model is that we had rather complex
> display peripherals, that needed more specific control of the bus.
>
> A simple example could be the enable sequence. I presume in your model
> the DSI controller's enable would be maybe something like:
>
> configure_dsi_timings(panel->get_timings());
> enable_dsi_output();
> panel->enable();
>
> What if the DSI peripheral requires an enable sequence of, say:
>
> - enable DSI clock
> - use LP mode
> - send some config messages
> - re-configure DSI clock to higher speed
> - send some more config messages
> - enable HS mode
> - send some more config messages
> - enable DSI video stream
>
> It gets a bit difficult to manage that kind of setup in a generic way in
> the DSI controller driver. As I see it, the DSI peripheral driver has to
> be in control of the nuances. The same goes for other video buses also,
> but DSI is perhaps one of the most complex ones to support.
I've briefly gone over this in another subthread, but here it is again
for the sake of completeness.
The difference in perception I think comes from the fact that the DSI
panel *driver* may need to control the DSI bus. However, DT describes
the hardware in terms of devices. And from that point of view it is
still the DSI _control_ler that _control_s the panel. There's no
electrical way that the panel can take possession of the bus. Well,
there's BTA, but even that happens under supervision of the DSI
controller.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 12:14 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: <20131017114139.GD10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 13265 bytes --]
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:
> [...]
>
> > CDF needs to model video pipelines that can be more complex than just a
> > display controller with a single output connected to a panel with a single
> > input. For that reason the DT bindings need to describe connections in a
> > generic enough way. The above DT fragment might be slightly more complex
> > than one would expect when thinking about the really simple case only,
> > but it's nowhere near overly complex in my opinion.
> >
> > Please note that, when a device has as single port, the ports node can be
> > omitted, and the port doesn't need to be numbered. You would then end up
> > with>
> > dc: display-controller {
> > port {
> > remote-endpoint = <&panel>;
> > };
> > };
> >
> > panel: panel {
> > port {
> > remote-endpoint = <&dc>;
> > };
> > };
> >
> > I don't think there's a way to simplify it further.
>
> That binding looks completely inconsistent to me. If we can write it either
> using a ports node with port@X subnodes, in my opinion we can equally well
> just use a unidirectional link for the case where we don't need the link
> back from the panel to the output. Where's the difference?
>
> Consider for example the case where an LVDS panel is connected to a simple
> LVDS output. For what possible reason would the panel need to access the
> output?
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.
> > > > I also plan to specify video bus parameters in DT for CDF, but this
> > > > hasn't been finalized yet.
> > >
> > > That effectively blocks any of the code that I've been working on until
> > > CDF has been merged. It's been discussed for over a year now, and there
> > > has even been significant pushback on using CDF in DRM, so even if CDF
> > > is eventually merged, that will still leave DRM with no way to turn on
> > > a simple panel.
> >
> > I'm probably even more concerned about the long time all this took and
> > will still take than you are, sincerely. I'm obviously to blame for not
> > being able to explain the problems and solutions correctly, as I see no
> > way why people would push back if they realized that the other solutions
> > that have been proposed, far from being future-proof, won't even be
> > present-proof. We would be shooting ourselves in the foot, but what can I
> > do when too many people want to do so ?
>
> I'm curious why you think this current proposal isn't present-proof. All
> three panels that I've tested with work with this proposal currently and
> I'm convinced that many more setups can be covered using something as
> simple as this.
My apologies for not being clear enough. I wasn't implying that your proposal
isn't present-proof, I was talking about the counter-proposals that have been
made in reaction to CDF.
> > 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.
> 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.
> 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.
> Nobody's been able to come up with a valid reason why the proposed binding
> is broken. It works for the tested devices (likely many more as well) and I
> don't forsee why it can't be future-proof. Why would it suddenly need to
> change? And even if it needed to change, why would the changes be backwards-
> incompatible rather than simply additional features that don't impact
> existing users?
>
> > Regarding the use of CDF in DRM, please note that it will be optional for
> > drivers to switch to the new framework. Nothing should hopefully require
> > existing drivers to be converted, drivers that want to make use of CDF to
> > support panels will be welcome to do so, but they big DRM drivers for
> > desktop GPUs won't be affected.
>
> 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.
> > > I keep wondering why we absolutely must have compatibility between CDF
> > > and this simple panel binding. DT content is supposed to concern itself
> > > with the description of hardware only. What about the following isn't an
> > > accurate description of the panel hardware?
> > >
> > > panel: panel {
> > > compatible = "cptt,claa101wb01";
> > >
> > > power-supply = <&vdd_pnl_reg>;
> > > enable-gpios = <&gpio 90 0>;
> > >
> > > backlight = <&backlight>;
> > > };
> > >
> > > dsi-controller {
> > > panel = <&panel>;
> > > };
> > >
> > > Note how the above is also not incompatible with anything that the CDF
> > > (to be) mandates, so it could easily be extended with any nodes or
> > > properties that CDF needs to work properly without breaking backwards-
> > > compatibility.
> >
> > As mentioned above, CDF needs to describe more complex pipelines. To make
> > the driver life as simple as possible the core code will handle the
> > pipeline description in the DT bindings, and it does require it to be
> > generic enough. You can of course describe connections in various ways
> > depending on the hardware, which would make the DT-related code
> > needlessly overcomplex. One of the issue with the above description is
> > that there's no way to go from the panel to the display controller, which
> > might be a show stopper in the future when code requires that. A
> > workaround would be to look through the whole DT for the reverse link,
> > which would be pretty inefficient.
>
> 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 ?
> > > > > +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).
> > > > > +static const struct drm_display_mode auo_b101aw03_mode = {
> > > > > + .clock = 51450,
> > > > > + .hdisplay = 1024,
> > > > > + .hsync_start = 1024 + 156,
> > > > > + .hsync_end = 1024 + 156 + 8,
> > > > > + .htotal = 1024 + 156 + 8 + 156,
> > > > > + .vdisplay = 600,
> > > > > + .vsync_start = 600 + 16,
> > > > > + .vsync_end = 600 + 16 + 6,
> > > > > + .vtotal = 600 + 16 + 6 + 16,
> > > > > + .vrefresh = 60,
> > > > > +};
> > > >
> > > > 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.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 12:17 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <525FD12D.3000200-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1564 bytes --]
Hi Tomi,
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 ?
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 12:23 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomi Valkeinen, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie,
Rob Clark, Daniel Vetter
In-Reply-To: <20131017120646.GE10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 7685 bytes --]
Hi Thierry,
On Thursday 17 October 2013 14:06:47 Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 13:05:18 Thierry Reding wrote:
> > > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> > > > On 17/10/13 11:53, Thierry Reding wrote:
> > > > > I keep wondering why we absolutely must have compatibility between
> > > > > CDF and this simple panel binding. DT content is supposed to concern
> > > > > itself with the description of hardware only. What about the
> > > > > following isn't an accurate description of the panel hardware?
> > > > >
> > > > > panel: panel {
> > > > > compatible = "cptt,claa101wb01";
> > > > >
> > > > > power-supply = <&vdd_pnl_reg>;
> > > > > enable-gpios = <&gpio 90 0>;
> > > > >
> > > > > backlight = <&backlight>;
> > > > > };
> > > > >
> > > > > dsi-controller {
> > > > > panel = <&panel>;
> > > > > };
> > > >
> > > > That's quite similar to how my current out-of-mainline OMAP DSS DT
> > > > bindings work. The difference is that I have a reference from the
> > > > panel node to the video source (dsi-controller), instead of the other
> > > > way around. I just find it more natural. It works the same way as,
> > > > say, regulators, gpios, etc.
> > >
> > > I suppose that depends on the way you look at it. In the above proposal
> > > I consider the output (DSI controller) to use the panel as a resource
> > > that provides a certain set of services (query mode timings, provide a
> > > way to enable or disable the panel). Similarly the panel uses the
> > > backlight as a resource that provides a certain set of services (such as
> > > changing the brightness).
> > >
> > > The above also follows the natural order of control. The panel has no
> > > way to control the DSI output. However, it is the output that controls
> > > when a panel is required and turn it on as appropriate.
> >
> > I'm no DSI expert, but I know enough about it to be sure that Tomi will
> > disagree. DSI panels can have complex power sequences that require the
> > input stream to be finely controlled (for instance it might require a
> > clock without video frames for some time, switch a GPIO or regulator,
> > send a command to the panel, and then only get video frames). For that
> > reason all developers I've talked to who have an in-depth knowledge of
> > DSI and DSI panels told me that the panel needs to control the video bus,
> > and request the video source to start/stop the video stream.
>
> Oh, I'm very well aware of the various flavours of funkiness that DSI
> panels come in. But it's wrong to say that the panel needs to control
> the video bus. There's simply no way that a panel can actually do that.
> It is true, however, that in order to make this work in a maintainable
> fashion, the DSI panel *driver* may need to control the DSI bus. That's
> an entirely different story.
Sure, but I don't think that's really related to the DT bindings. We don't
have to model every electrical signal in a detailed way that matches the
direction of the electrons flow :-) What we need to model is a connection
between a display controller and a panel (possibly with a direction). What I'd
like to do is to express that link in a way that can also express more complex
pipeline topologies. I don't want to make it overly complex, I had hoped that
my DT bindings proposal would be a good approach as it's both generic and
still pretty simple.
> Again, that's exactly how I2C works as well. I2C slaves, by definition,
> do not control the I2C bus. However, I2C client drivers can do so by
> using the (master) services provided by the I2C controller.
>
> So for DSI what we could easily do is provide a DSI "adapter" to the
> panel when it is attached to the DSI controller. If the DSI adapter
> implements all of the required services, then the DSI panel driver is
> easily capable of handling all the required special quirks.
>
> I'm Cc'ing Rob and Daniel, both of whom have been working on this
> lately. Actually I'm not sure if Daniel was directly involved, but at
> least somebody from Intel's team was working on it.
>
> > > > However, one thing unclear is where the panel node should be. You seem
> > > > to have a DSI panel here. If the panel is truly not controlled in any
> > > > way, maybe having the panel as a normal platform device is ok. But if
> > > > the DSI panel is controlled with DSI messages, should it be a child of
> > > > the dsi-controller node, the same way i2c peripherals are children of
> > > > i2c master?
> > >
> > > That's one possibility. In this particular case it's not even necessary
> > > to use any of DSIs control methods to talk to the panel. The panel only
> > > receives the data stream and "just works".
> > >
> > > Even if the panel were to be controlled via DSI, I think keeping it as
> > > a separate device node is equally valid. It's really boils down to what
> > > I already said above. The output uses the panel as a resource, similar
> > > to how other devices might use a GPIO controller to use a GPIO pin, or
> > > an interrupt controller to request an IRQ.
> >
> > Except that the display controller can also provides resources to the
> > panel. For DSI panels that support DSI commands, the control bus is
> > provided by the display controller. Much like an I2C device must be a
> > child of its I2C controller node, the DSI panel device should then be a
> > child of the display controller node.
>
> I don't think resources is the right term here. The DSI controller
> provides services. That's traditionally been handled in DT by handing
> out phandles to the service providers.
>
> That said I see why it would make sense to make a DSI panel the child of
> the DSI controller node, so we agree at least partially. I don't like it
> all that much because it makes handling of panels inconsistent across
> various types of output and therefore makes it more complicated to
> support it drivers, but I'm sure I can live with it if somebody wants to
> enforce that.
>
> > > > > 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.
> > > >
> > > > This sounds good to me.
> > > >
> > > > Although maybe it'd be good to have the driver compatible with
> > > > something like "generic-panel", so that you could have:
> > > >
> > > > compatible = "foo,specific-panel", "generic-panel";
> > > >
> > > > and if there's no need for any power/gpio setup for your board, you
> > > > may skip adding "foo,specific-panel" support to the panel driver.
> > > > Later, somebody else may need to implement fine grained power/gpio
> > > > support for "foo,specific-panel", and it would just work.
> > > >
> > > > Maybe that would help us with the problem of adding support in the
> > > > driver for a hundred different simple panels each used only once on a
> > > > specific board.
> > >
> > > Sure, that all sounds like reasonable additions. All of the suggestions
> > > are even backwards-compatible and hence can be added when needed without
> > > breaking compatibility with existing users of the binding.
> >
> > What's wrong with moving the three hardcoded modes we already have in the
> > driver to DT before pushing this to mainline ?
>
> I think I've answered that in a different subthread.
Then I might not have understood your answer :-)
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH/RFC v3 00/19] Common Display Framework
From: Andrzej Hajda @ 2013-10-17 12:26 UTC (permalink / raw)
To: Tomi Valkeinen, Laurent Pinchart
Cc: linux-fbdev, dri-devel, Jesse Barnes, Benjamin Gaignard, Tom Gall,
Kyungmin Park, linux-media, Stephen Warren, Mark Zhang,
Alexandre Courbot, Ragesh Radhakrishnan, Thomas Petazzoni,
Sunil Joshi, Maxime Ripard, Vikas Sajjan, Marcus Lorentzon
In-Reply-To: <525F9D4D.4000702@ti.com>
On 10/17/2013 10:18 AM, Tomi Valkeinen wrote:
> On 17/10/13 10:48, Andrzej Hajda wrote:
>
>> The main function of DSI is to transport pixels from one IP to another IP
>> and this function IMO should not be modeled by display entity.
>> "Power, clocks, etc" will be performed via control bus according to
>> panel demands.
>> If 'DSI chip' has additional functions for video processing they can
>> be modeled by CDF entity if it makes sense.
> Now I don't follow. What do you mean with "display entity" and with "CDF
> entity"? Are they the same?
Yes, they are the same, sorry for confusion.
>
> Let me try to clarify my point:
>
> On OMAP SoC we have a DSI encoder, which takes input from the display
> controller in parallel RGB format, and outputs DSI.
>
> Then there are external encoders that take MIPI DPI as input, and output
> DSI.
>
> The only difference with the above two components is that the first one
> is embedded into the SoC. I see no reason to represent them in different
> ways (i.e. as you suggested, not representing the SoC's DSI at all).
>
> Also, if you use DSI burst mode, you will have to have different video
> timings in the DSI encoder's input and output. And depending on the
> buffering of the DSI encoder, you could have different timings in any case.
I am not sure what exactly the encoder performs, if this is only image
transport from dispc to panel CDF pipeline in both cases should look like:
dispc ----> panel
The only difference is that panels will be connected via different Linux bus
adapters, but it will be irrelevant to CDF itself. In this case I would say
this is DSI-master rather than encoder, or at least that the only
function of the
encoder is DSI.
If display_timings on input and output differs, I suppose it should be
modeled
as display_entity, as this is an additional functionality(not covered by
DSI standard AFAIK).
CDF in such case:
dispc ---> encoder ---> panel
In this case I would call it encoder with DSI master.
>
> Furthermore, both components could have extra processing. I know the
> external encoders sometimes do have features like scaling.
The same as above, ISP with embedded DSI.
>
>>> We still have two different endpoint configurations for the same
>>> DSI-master port. If that configuration is in the DSI-master's port node,
>>> not inside an endpoint data, then that can't be supported.
>> I am not sure if I understand it correctly. But it seems quite simple:
>> when panel starts/resumes it request DSI (via control bus) to fulfill
>> its configuration settings.
>> Of course there are some settings which are not panel dependent and those
>> should reside in DSI node.
> Exactly. And when the two panels require different non-panel-dependent
> settings, how do you represent them in the DT data?
non-panel-dependent setting cannot depend on panel, by definition :)
>
>>>> We say then: callee handles locking :)
>>> Sure, but my point was that the caller handling the locking is much
>>> simpler than the callee handling locking. And the latter causes
>>> atomicity issues, as the other API could be invoked in between two calls
>>> for the first API.
>>>
>>>
>> Could you describe such scenario?
> If we have two independent APIs, ctrl and video, that affect the same
> underlying hardware, the DSI bus, we could have a scenario like this:
>
> thread 1:
>
> ctrl->op_foo();
> ctrl->op_bar();
>
> thread 2:
>
> video->op_baz();
>
> Even if all those ops do locking properly internally, the fact that
> op_baz() can be called in between op_foo() and op_bar() may cause problems.
>
> To avoid that issue with two APIs we'd need something like:
>
> thread 1:
>
> ctrl->lock();
> ctrl->op_foo();
> ctrl->op_bar();
> ctrl->unlock();
>
> thread 2:
>
> video->lock();
> video->op_baz();
> video->unlock();
I should mention I was asking for real hw/drivers configuration.
I do not know what do you mean with video->op_baz() ?
DSI-master is not modeled in CDF, and only CDF provides video
operations.
I guess one scenario, when two panels are connected to single DSI-master.
In such case both can call DSI ops, but I do not know how do you want to
prevent it in case of your CDF-T implementation.
>
>>>> Platform devices
>>>> ~~~~~~~~~~~~~~~~
>>>> Platform devices are devices that typically appear as autonomous
>>>> entities in the system. This includes legacy port-based devices and
>>>> host bridges to peripheral buses, and most controllers integrated
>>>> into system-on-chip platforms. What they usually have in common
>>>> is direct addressing from a CPU bus. Rarely, a platform_device will
>>>> be connected through a segment of some other kind of bus; but its
>>>> registers will still be directly addressable.
>>> Yep, "typically" and "rarely" =). I agree, it's not clear. I think there
>>> are things with DBI/DSI that clearly point to a platform device, but
>>> also the other way.
>> Just to be sure, we are talking here about DSI-slaves, ie. for example
>> about panels,
>> where direct accessing from CPU bus usually is not possible.
> Yes. My point is that with DBI/DSI there's not much bus there (if a
> normal bus would be PCI/USB/i2c etc), it's just a point to point link
> without probing or a clearly specified setup sequence.
This is why I considered replacing DSI bus with DSI-master as parent
device and panel as slave platorm_device, like in MFD devices.
>
> If DSI/DBI was used only for control, a linux bus would probably make
> sense. But DSI/DBI is mainly a video transport channel, with the
> control-part being "secondary".
>
> And when considering that the video and control data are sent over the
> same channel (i.e. there's no separate, independent ctrl channel), and
> the strict timing restrictions with video, my gut feeling is just that
> all the extra complexity brought with separating the control to a
> separate bus is not worth it.
There is additional complexity due to bus implementation requirements
(I would rather call it boiler-plate code), but in core it is still a
matter of ops.
With Linux bus those ops are available only to DSI-slave, which is
also a good thing I guess.
Andrzej
>
> Tomi
>
>
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-17 12:32 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <3768216.eiA2v5KI6a@avalon>
[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]
On 17/10/13 15:17, Laurent Pinchart wrote:
> Hi Tomi,
>
> 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>;
};
};
};
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.
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: Thierry Reding @ 2013-10-17 12:46 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: <1673630.A5yYaNBe3X@avalon>
[-- Attachment #1: Type: text/plain, Size: 12174 bytes --]
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:
> > [...]
> >
> > > CDF needs to model video pipelines that can be more complex than just a
> > > display controller with a single output connected to a panel with a single
> > > input. For that reason the DT bindings need to describe connections in a
> > > generic enough way. The above DT fragment might be slightly more complex
> > > than one would expect when thinking about the really simple case only,
> > > but it's nowhere near overly complex in my opinion.
> > >
> > > Please note that, when a device has as single port, the ports node can be
> > > omitted, and the port doesn't need to be numbered. You would then end up
> > > with>
> > > dc: display-controller {
> > > port {
> > > remote-endpoint = <&panel>;
> > > };
> > > };
> > >
> > > panel: panel {
> > > port {
> > > remote-endpoint = <&dc>;
> > > };
> > > };
> > >
> > > I don't think there's a way to simplify it further.
> >
> > That binding looks completely inconsistent to me. If we can write it either
> > using a ports node with port@X subnodes, in my opinion we can equally well
> > just use a unidirectional link for the case where we don't need the link
> > back from the panel to the output. Where's the difference?
> >
> > Consider for example the case where an LVDS panel is connected to a simple
> > LVDS output. For what possible reason would the panel need to access the
> > output?
>
> 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'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.
> > 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.
> > 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?
> > 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.
> > 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?
> > > > > > +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.
> > > > > 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.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 12:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Tomi Valkeinen, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie,
Rob Clark, Daniel Vetter
In-Reply-To: <2414405.blLIekLlE8@avalon>
[-- Attachment #1: Type: text/plain, Size: 4135 bytes --]
On Thu, Oct 17, 2013 at 02:23:32PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
>
> On Thursday 17 October 2013 14:06:47 Thierry Reding wrote:
> > On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote:
> > > On Thursday 17 October 2013 13:05:18 Thierry Reding wrote:
> > > > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> > > > > On 17/10/13 11:53, Thierry Reding wrote:
> > > > > > I keep wondering why we absolutely must have compatibility between
> > > > > > CDF and this simple panel binding. DT content is supposed to concern
> > > > > > itself with the description of hardware only. What about the
> > > > > > following isn't an accurate description of the panel hardware?
> > > > > >
> > > > > > panel: panel {
> > > > > > compatible = "cptt,claa101wb01";
> > > > > >
> > > > > > power-supply = <&vdd_pnl_reg>;
> > > > > > enable-gpios = <&gpio 90 0>;
> > > > > >
> > > > > > backlight = <&backlight>;
> > > > > > };
> > > > > >
> > > > > > dsi-controller {
> > > > > > panel = <&panel>;
> > > > > > };
> > > > >
> > > > > That's quite similar to how my current out-of-mainline OMAP DSS DT
> > > > > bindings work. The difference is that I have a reference from the
> > > > > panel node to the video source (dsi-controller), instead of the other
> > > > > way around. I just find it more natural. It works the same way as,
> > > > > say, regulators, gpios, etc.
> > > >
> > > > I suppose that depends on the way you look at it. In the above proposal
> > > > I consider the output (DSI controller) to use the panel as a resource
> > > > that provides a certain set of services (query mode timings, provide a
> > > > way to enable or disable the panel). Similarly the panel uses the
> > > > backlight as a resource that provides a certain set of services (such as
> > > > changing the brightness).
> > > >
> > > > The above also follows the natural order of control. The panel has no
> > > > way to control the DSI output. However, it is the output that controls
> > > > when a panel is required and turn it on as appropriate.
> > >
> > > I'm no DSI expert, but I know enough about it to be sure that Tomi will
> > > disagree. DSI panels can have complex power sequences that require the
> > > input stream to be finely controlled (for instance it might require a
> > > clock without video frames for some time, switch a GPIO or regulator,
> > > send a command to the panel, and then only get video frames). For that
> > > reason all developers I've talked to who have an in-depth knowledge of
> > > DSI and DSI panels told me that the panel needs to control the video bus,
> > > and request the video source to start/stop the video stream.
> >
> > Oh, I'm very well aware of the various flavours of funkiness that DSI
> > panels come in. But it's wrong to say that the panel needs to control
> > the video bus. There's simply no way that a panel can actually do that.
> > It is true, however, that in order to make this work in a maintainable
> > fashion, the DSI panel *driver* may need to control the DSI bus. That's
> > an entirely different story.
>
> Sure, but I don't think that's really related to the DT bindings. We don't
> have to model every electrical signal in a detailed way that matches the
> direction of the electrons flow :-) What we need to model is a connection
> between a display controller and a panel (possibly with a direction). What I'd
> like to do is to express that link in a way that can also express more complex
> pipeline topologies. I don't want to make it overly complex, I had hoped that
> my DT bindings proposal would be a good approach as it's both generic and
> still pretty simple.
I get that, and for what it's worth I do think that your proposal looks
simple enough and if it can solve any of the problem you're facing with
CDF, then that's great.
But I don't think we should force inclusion of these properties on every
panel, even if it doesn't use any of the graph functionality. Is there
any problem with making them optional?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH/RFC v3 00/19] Common Display Framework
From: Tomi Valkeinen @ 2013-10-17 12:55 UTC (permalink / raw)
To: Andrzej Hajda, Laurent Pinchart
Cc: linux-fbdev, dri-devel, Jesse Barnes, Benjamin Gaignard, Tom Gall,
Kyungmin Park, linux-media, Stephen Warren, Mark Zhang,
Alexandre Courbot, Ragesh Radhakrishnan, Thomas Petazzoni,
Sunil Joshi, Maxime Ripard, Vikas Sajjan, Marcus Lorentzon
In-Reply-To: <525FD760.20506@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 4219 bytes --]
On 17/10/13 15:26, Andrzej Hajda wrote:
> I am not sure what exactly the encoder performs, if this is only image
> transport from dispc to panel CDF pipeline in both cases should look like:
> dispc ----> panel
> The only difference is that panels will be connected via different Linux bus
> adapters, but it will be irrelevant to CDF itself. In this case I would say
> this is DSI-master rather than encoder, or at least that the only
> function of the
> encoder is DSI.
Yes, as I said, it's up to the driver writer how he wants to use CDF. If
he doesn't see the point of representing the SoC's DSI encoder as a
separate CDF entity, nobody forces him to do that.
On OMAP, we have single DISPC with multiple parallel outputs, and a
bunch of encoder IPs (MIPI DPI, DSI, DBI, etc). Each encoder IP can be
connected to some of the DISPC's output. In this case, even if the DSI
encoder does nothing special, I see it much better to represent the DSI
encoder as a CDF entity so that the links between DISPC, DSI, and the
DSI peripherals are all there.
> If display_timings on input and output differs, I suppose it should be
> modeled
> as display_entity, as this is an additional functionality(not covered by
> DSI standard AFAIK).
Well, DSI standard is about the DSI output. Not about the encoder's
input, or the internal operation of the encoder.
>>> Of course there are some settings which are not panel dependent and those
>>> should reside in DSI node.
>> Exactly. And when the two panels require different non-panel-dependent
>> settings, how do you represent them in the DT data?
>
> non-panel-dependent setting cannot depend on panel, by definition :)
With "non-panel-dependent" setting I meant something that is a property
of the DSI master device, but still needs to be configured differently
for each panel.
Say, pin configuration. When using panel A, the first pin of the DSI
block could be clock+. With panel B, the first pin could be clock-. This
configuration is about DSI master, but it is different for each panel.
If we have separate endpoint in the DSI master for each panel, this data
can be there. If we don't have the endpoint, as is the case with
separate control bus, where is that data?
>>> Could you describe such scenario?
>> If we have two independent APIs, ctrl and video, that affect the same
>> underlying hardware, the DSI bus, we could have a scenario like this:
>>
>> thread 1:
>>
>> ctrl->op_foo();
>> ctrl->op_bar();
>>
>> thread 2:
>>
>> video->op_baz();
>>
>> Even if all those ops do locking properly internally, the fact that
>> op_baz() can be called in between op_foo() and op_bar() may cause problems.
>>
>> To avoid that issue with two APIs we'd need something like:
>>
>> thread 1:
>>
>> ctrl->lock();
>> ctrl->op_foo();
>> ctrl->op_bar();
>> ctrl->unlock();
>>
>> thread 2:
>>
>> video->lock();
>> video->op_baz();
>> video->unlock();
> I should mention I was asking for real hw/drivers configuration.
> I do not know what do you mean with video->op_baz() ?
> DSI-master is not modeled in CDF, and only CDF provides video
> operations.
It was just an example of the additional complexity with regarding
locking when using two APIs.
The point is that if the panel driver has two pointers (i.e. API), one
for the control bus, one for the video bus, and ops on both buses affect
the same hardware, the locking is not easy.
If, on the other hand, the panel driver only has one API to use, it's
simple to require the caller to handle any locking.
> I guess one scenario, when two panels are connected to single DSI-master.
> In such case both can call DSI ops, but I do not know how do you want to
> prevent it in case of your CDF-T implementation.
No, that was not the case I was describing. This was about a single panel.
If we have two independent APIs, we need to define how locking is
managed for those APIs. Even if in practice both APIs are used by the
same driver, and the driver can manage the locking, that's not really a
valid requirement. It'd be almost the same as requiring that gpio API
cannot be called at the same time as i2c API.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* [PATCH v2] omapdss: Add new panel driver for Topolly td028ttec1 LCD.
From: Marek Belisko @ 2013-10-17 14:14 UTC (permalink / raw)
To: tomi.valkeinen
Cc: plagnioj, linux-kernel, linux-omap, linux-fbdev, Marek Belisko,
H. Nikolaus Schaller
Signed-off-by: Marek Belisko <marek@goldelico.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
changes from v1:
- reworked to be spi driver instead platform with custom spi bitbang
this configuration was tested with spi_gpio bitbang driver on gta04 board
and works fine (thanks Tomi and Lars-Peter for comments)
- address previous comments
drivers/video/omap2/displays-new/Kconfig | 6 +
drivers/video/omap2/displays-new/Makefile | 1 +
.../omap2/displays-new/panel-tpo-td028ttec1.c | 486 +++++++++++++++++++++
include/video/omap-panel-data.h | 13 +
4 files changed, 506 insertions(+)
create mode 100644 drivers/video/omap2/displays-new/panel-tpo-td028ttec1.c
diff --git a/drivers/video/omap2/displays-new/Kconfig b/drivers/video/omap2/displays-new/Kconfig
index 6c90885..1054249 100644
--- a/drivers/video/omap2/displays-new/Kconfig
+++ b/drivers/video/omap2/displays-new/Kconfig
@@ -56,6 +56,12 @@ config DISPLAY_PANEL_SHARP_LS037V7DW01
help
LCD Panel used in TI's SDP3430 and EVM boards
+config DISPLAY_PANEL_TPO_TD028TTEC1
+ tristate "TPO TD028TTEC1 LCD Panel"
+ depends on SPI
+ help
+ LCD panel used in Openmoko.
+
config DISPLAY_PANEL_TPO_TD043MTEA1
tristate "TPO TD043MTEA1 LCD Panel"
depends on SPI
diff --git a/drivers/video/omap2/displays-new/Makefile b/drivers/video/omap2/displays-new/Makefile
index 5aeb11b..0323a8a 100644
--- a/drivers/video/omap2/displays-new/Makefile
+++ b/drivers/video/omap2/displays-new/Makefile
@@ -8,5 +8,6 @@ obj-$(CONFIG_DISPLAY_PANEL_DSI_CM) += panel-dsi-cm.o
obj-$(CONFIG_DISPLAY_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
obj-$(CONFIG_DISPLAY_PANEL_LGPHILIPS_LB035Q02) += panel-lgphilips-lb035q02.o
obj-$(CONFIG_DISPLAY_PANEL_SHARP_LS037V7DW01) += panel-sharp-ls037v7dw01.o
+obj-$(CONFIG_DISPLAY_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o
obj-$(CONFIG_DISPLAY_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o
obj-$(CONFIG_DISPLAY_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
diff --git a/drivers/video/omap2/displays-new/panel-tpo-td028ttec1.c b/drivers/video/omap2/displays-new/panel-tpo-td028ttec1.c
new file mode 100644
index 0000000..5a44d30
--- /dev/null
+++ b/drivers/video/omap2/displays-new/panel-tpo-td028ttec1.c
@@ -0,0 +1,486 @@
+/*
+ * Toppoly TD028TTEC1 panel support
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Author: Tomi Valkeinen <tomi.valkeinen@nokia.com>
+ *
+ * Neo 1973 code (jbt6k74.c):
+ * Copyright (C) 2006-2007 by OpenMoko, Inc.
+ * Author: Harald Welte <laforge@openmoko.org>
+ *
+ * Ported and adapted from Neo 1973 U-Boot by:
+ * H. Nikolaus Schaller <hns@goldelico.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+#include <linux/gpio.h>
+#include <video/omapdss.h>
+#include <video/omap-panel-data.h>
+
+struct panel_drv_data {
+ struct omap_dss_device dssdev;
+ struct omap_dss_device *in;
+
+ int data_lines;
+
+ struct omap_video_timings videomode;
+
+ struct spi_device *spi_dev;
+
+ u16 tx_buf[4];
+};
+
+static struct omap_video_timings td028ttec1_panel_timings = {
+ .x_res = 480,
+ .y_res = 640,
+ .pixel_clock = 22153,
+ .hfp = 24,
+ .hsw = 8,
+ .hbp = 8,
+ .vfp = 4,
+ .vsw = 2,
+ .vbp = 2,
+
+ .vsync_level = OMAPDSS_SIG_ACTIVE_LOW,
+ .hsync_level = OMAPDSS_SIG_ACTIVE_LOW,
+
+ .data_pclk_edge = OMAPDSS_DRIVE_SIG_FALLING_EDGE,
+ .de_level = OMAPDSS_SIG_ACTIVE_HIGH,
+ .sync_pclk_edge = OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
+};
+
+#define JBT_COMMAND 0x000
+#define JBT_DATA 0x100
+
+int jbt_reg_write_nodata(struct panel_drv_data *ddata, u8 reg)
+{
+ int rc;
+
+ ddata->tx_buf[0] = JBT_COMMAND | reg;
+ rc = spi_write(ddata->spi_dev, (u8 *)ddata->tx_buf,
+ 1*sizeof(u16));
+ if (rc != 0)
+ dev_err(&ddata->spi_dev->dev,
+ "jbt_reg_write_nodata spi_write ret %d\n", rc);
+
+ return rc;
+}
+
+int jbt_reg_write(struct panel_drv_data *ddata, u8 reg, u8 data)
+{
+ int rc;
+
+ ddata->tx_buf[0] = JBT_COMMAND | reg;
+ ddata->tx_buf[1] = JBT_DATA | data;
+ rc = spi_write(ddata->spi_dev, (u8 *)ddata->tx_buf,
+ 2*sizeof(u16));
+ if (rc != 0)
+ dev_err(&ddata->spi_dev->dev,
+ "jbt_reg_write spi_write ret %d\n", rc);
+
+ return rc;
+}
+
+int jbt_reg_write16(struct panel_drv_data *ddata, u8 reg, u16 data)
+{
+ int rc;
+
+ ddata->tx_buf[0] = JBT_COMMAND | reg;
+ ddata->tx_buf[1] = JBT_DATA | (data >> 8);
+ ddata->tx_buf[2] = JBT_DATA | (data & 0xff);
+
+ rc = spi_write(ddata->spi_dev, (u8 *)ddata->tx_buf,
+ 3*sizeof(u16));
+
+ if (rc != 0)
+ dev_err(&ddata->spi_dev->dev,
+ "jbt_reg_write16 spi_write ret %d\n", rc);
+
+ return rc;
+}
+
+enum jbt_register {
+ JBT_REG_SLEEP_IN = 0x10,
+ JBT_REG_SLEEP_OUT = 0x11,
+
+ JBT_REG_DISPLAY_OFF = 0x28,
+ JBT_REG_DISPLAY_ON = 0x29,
+
+ JBT_REG_RGB_FORMAT = 0x3a,
+ JBT_REG_QUAD_RATE = 0x3b,
+
+ JBT_REG_POWER_ON_OFF = 0xb0,
+ JBT_REG_BOOSTER_OP = 0xb1,
+ JBT_REG_BOOSTER_MODE = 0xb2,
+ JBT_REG_BOOSTER_FREQ = 0xb3,
+ JBT_REG_OPAMP_SYSCLK = 0xb4,
+ JBT_REG_VSC_VOLTAGE = 0xb5,
+ JBT_REG_VCOM_VOLTAGE = 0xb6,
+ JBT_REG_EXT_DISPL = 0xb7,
+ JBT_REG_OUTPUT_CONTROL = 0xb8,
+ JBT_REG_DCCLK_DCEV = 0xb9,
+ JBT_REG_DISPLAY_MODE1 = 0xba,
+ JBT_REG_DISPLAY_MODE2 = 0xbb,
+ JBT_REG_DISPLAY_MODE = 0xbc,
+ JBT_REG_ASW_SLEW = 0xbd,
+ JBT_REG_DUMMY_DISPLAY = 0xbe,
+ JBT_REG_DRIVE_SYSTEM = 0xbf,
+
+ JBT_REG_SLEEP_OUT_FR_A = 0xc0,
+ JBT_REG_SLEEP_OUT_FR_B = 0xc1,
+ JBT_REG_SLEEP_OUT_FR_C = 0xc2,
+ JBT_REG_SLEEP_IN_LCCNT_D = 0xc3,
+ JBT_REG_SLEEP_IN_LCCNT_E = 0xc4,
+ JBT_REG_SLEEP_IN_LCCNT_F = 0xc5,
+ JBT_REG_SLEEP_IN_LCCNT_G = 0xc6,
+
+ JBT_REG_GAMMA1_FINE_1 = 0xc7,
+ JBT_REG_GAMMA1_FINE_2 = 0xc8,
+ JBT_REG_GAMMA1_INCLINATION = 0xc9,
+ JBT_REG_GAMMA1_BLUE_OFFSET = 0xca,
+
+ JBT_REG_BLANK_CONTROL = 0xcf,
+ JBT_REG_BLANK_TH_TV = 0xd0,
+ JBT_REG_CKV_ON_OFF = 0xd1,
+ JBT_REG_CKV_1_2 = 0xd2,
+ JBT_REG_OEV_TIMING = 0xd3,
+ JBT_REG_ASW_TIMING_1 = 0xd4,
+ JBT_REG_ASW_TIMING_2 = 0xd5,
+
+ JBT_REG_HCLOCK_VGA = 0xec,
+ JBT_REG_HCLOCK_QVGA = 0xed,
+};
+
+#define to_panel_data(p) container_of(p, struct panel_drv_data, dssdev)
+
+static int td028ttec1_panel_connect(struct omap_dss_device *dssdev)
+{
+ struct panel_drv_data *ddata = to_panel_data(dssdev);
+ struct omap_dss_device *in = ddata->in;
+ int r;
+
+ if (omapdss_device_is_connected(dssdev))
+ return 0;
+
+ r = in->ops.dpi->connect(in, dssdev);
+ if (r)
+ return r;
+
+ return 0;
+}
+
+static void td028ttec1_panel_disconnect(struct omap_dss_device *dssdev)
+{
+ struct panel_drv_data *ddata = to_panel_data(dssdev);
+ struct omap_dss_device *in = ddata->in;
+
+ if (!omapdss_device_is_connected(dssdev))
+ return;
+
+ in->ops.dpi->disconnect(in, dssdev);
+}
+
+static int td028ttec1_panel_enable(struct omap_dss_device *dssdev)
+{
+ struct panel_drv_data *ddata = to_panel_data(dssdev);
+ struct omap_dss_device *in = ddata->in;
+ int r;
+
+ if (!omapdss_device_is_connected(dssdev))
+ return -ENODEV;
+
+ if (omapdss_device_is_enabled(dssdev))
+ return 0;
+
+ in->ops.dpi->set_data_lines(in, ddata->data_lines);
+ in->ops.dpi->set_timings(in, &ddata->videomode);
+
+ r = in->ops.dpi->enable(in);
+ if (r)
+ return r;
+
+ dev_dbg(dssdev->dev, "td028ttec1_panel_enable() - state %d\n",
+ dssdev->state);
+
+ /*
+ * according to data sheet: wait 50ms (Tpos of LCM). However, 50ms
+ * seems unreliable with later LCM batches, increasing to 90ms
+ */
+ msleep(90);
+
+ /* three times command zero */
+ r |= jbt_reg_write_nodata(ddata, 0x00);
+ msleep(1000);
+ r |= jbt_reg_write_nodata(ddata, 0x00);
+ msleep(1000);
+ r |= jbt_reg_write_nodata(ddata, 0x00);
+ msleep(1000);
+
+ if (r) {
+ dev_warn(dssdev->dev, "transfer error\n");
+ goto transfer_err;
+ }
+
+ /* deep standby out */
+ r |= jbt_reg_write(ddata, JBT_REG_POWER_ON_OFF, 0x17);
+
+ /* RGB I/F on, RAM write off, QVGA through, SIGCON enable */
+ r |= jbt_reg_write(ddata, JBT_REG_DISPLAY_MODE, 0x80);
+
+ /* Quad mode off */
+ r |= jbt_reg_write(ddata, JBT_REG_QUAD_RATE, 0x00);
+
+ /* AVDD on, XVDD on */
+ r |= jbt_reg_write(ddata, JBT_REG_POWER_ON_OFF, 0x16);
+
+ /* Output control */
+ r |= jbt_reg_write16(ddata, JBT_REG_OUTPUT_CONTROL, 0xfff9);
+
+ /* Sleep mode off */
+ r |= jbt_reg_write_nodata(ddata, JBT_REG_SLEEP_OUT);
+
+ /* at this point we have like 50% grey */
+
+ /* initialize register set */
+ r |= jbt_reg_write(ddata, JBT_REG_DISPLAY_MODE1, 0x01);
+ r |= jbt_reg_write(ddata, JBT_REG_DISPLAY_MODE2, 0x00);
+ r |= jbt_reg_write(ddata, JBT_REG_RGB_FORMAT, 0x60);
+ r |= jbt_reg_write(ddata, JBT_REG_DRIVE_SYSTEM, 0x10);
+ r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_OP, 0x56);
+ r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_MODE, 0x33);
+ r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_FREQ, 0x11);
+ r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_FREQ, 0x11);
+ r |= jbt_reg_write(ddata, JBT_REG_OPAMP_SYSCLK, 0x02);
+ r |= jbt_reg_write(ddata, JBT_REG_VSC_VOLTAGE, 0x2b);
+ r |= jbt_reg_write(ddata, JBT_REG_VCOM_VOLTAGE, 0x40);
+ r |= jbt_reg_write(ddata, JBT_REG_EXT_DISPL, 0x03);
+ r |= jbt_reg_write(ddata, JBT_REG_DCCLK_DCEV, 0x04);
+ /*
+ * default of 0x02 in JBT_REG_ASW_SLEW responsible for 72Hz requirement
+ * to avoid red / blue flicker
+ */
+ r |= jbt_reg_write(ddata, JBT_REG_ASW_SLEW, 0x04);
+ r |= jbt_reg_write(ddata, JBT_REG_DUMMY_DISPLAY, 0x00);
+
+ r |= jbt_reg_write(ddata, JBT_REG_SLEEP_OUT_FR_A, 0x11);
+ r |= jbt_reg_write(ddata, JBT_REG_SLEEP_OUT_FR_B, 0x11);
+ r |= jbt_reg_write(ddata, JBT_REG_SLEEP_OUT_FR_C, 0x11);
+ r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_D, 0x2040);
+ r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_E, 0x60c0);
+ r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_F, 0x1020);
+ r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_G, 0x60c0);
+
+ r |= jbt_reg_write16(ddata, JBT_REG_GAMMA1_FINE_1, 0x5533);
+ r |= jbt_reg_write(ddata, JBT_REG_GAMMA1_FINE_2, 0x00);
+ r |= jbt_reg_write(ddata, JBT_REG_GAMMA1_INCLINATION, 0x00);
+ r |= jbt_reg_write(ddata, JBT_REG_GAMMA1_BLUE_OFFSET, 0x00);
+
+ r |= jbt_reg_write16(ddata, JBT_REG_HCLOCK_VGA, 0x1f0);
+ r |= jbt_reg_write(ddata, JBT_REG_BLANK_CONTROL, 0x02);
+ r |= jbt_reg_write16(ddata, JBT_REG_BLANK_TH_TV, 0x0804);
+
+ r |= jbt_reg_write(ddata, JBT_REG_CKV_ON_OFF, 0x01);
+ r |= jbt_reg_write16(ddata, JBT_REG_CKV_1_2, 0x0000);
+
+ r |= jbt_reg_write16(ddata, JBT_REG_OEV_TIMING, 0x0d0e);
+ r |= jbt_reg_write16(ddata, JBT_REG_ASW_TIMING_1, 0x11a4);
+ r |= jbt_reg_write(ddata, JBT_REG_ASW_TIMING_2, 0x0e);
+
+ r |= jbt_reg_write_nodata(ddata, JBT_REG_DISPLAY_ON);
+
+ dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
+
+transfer_err:
+
+ return r ? -EIO : 0;
+}
+
+static void td028ttec1_panel_disable(struct omap_dss_device *dssdev)
+{
+ struct panel_drv_data *ddata = to_panel_data(dssdev);
+ struct omap_dss_device *in = ddata->in;
+
+ if (!omapdss_device_is_enabled(dssdev))
+ return;
+
+ dev_dbg(dssdev->dev, "td028ttec1_panel_disable()\n");
+
+ in->ops.dpi->disable(in);
+
+ jbt_reg_write_nodata(ddata, JBT_REG_DISPLAY_OFF);
+ jbt_reg_write16(ddata, JBT_REG_OUTPUT_CONTROL, 0x8002);
+ jbt_reg_write_nodata(ddata, JBT_REG_SLEEP_IN);
+ jbt_reg_write(ddata, JBT_REG_POWER_ON_OFF, 0x00);
+
+ dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
+}
+
+static void td028ttec1_panel_set_timings(struct omap_dss_device *dssdev,
+ struct omap_video_timings *timings)
+{
+ struct panel_drv_data *ddata = to_panel_data(dssdev);
+ struct omap_dss_device *in = ddata->in;
+
+ ddata->videomode = *timings;
+ dssdev->panel.timings = *timings;
+
+ in->ops.dpi->set_timings(in, timings);
+}
+
+static void td028ttec1_panel_get_timings(struct omap_dss_device *dssdev,
+ struct omap_video_timings *timings)
+{
+ struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+ *timings = ddata->videomode;
+}
+
+static int td028ttec1_panel_check_timings(struct omap_dss_device *dssdev,
+ struct omap_video_timings *timings)
+{
+ struct panel_drv_data *ddata = to_panel_data(dssdev);
+ struct omap_dss_device *in = ddata->in;
+
+ return in->ops.dpi->check_timings(in, timings);
+}
+
+static struct omap_dss_driver td028ttec1_ops = {
+ .connect = td028ttec1_panel_connect,
+ .disconnect = td028ttec1_panel_disconnect,
+
+ .enable = td028ttec1_panel_enable,
+ .disable = td028ttec1_panel_disable,
+
+ .set_timings = td028ttec1_panel_set_timings,
+ .get_timings = td028ttec1_panel_get_timings,
+ .check_timings = td028ttec1_panel_check_timings,
+};
+
+static int td028ttec1_panel_probe_pdata(struct spi_device *spi)
+{
+ const struct panel_tpo_td028ttec1_platform_data *pdata;
+ struct panel_drv_data *ddata = dev_get_drvdata(&spi->dev);
+ struct omap_dss_device *dssdev, *in;
+
+ pdata = dev_get_platdata(&spi->dev);
+
+ in = omap_dss_find_output(pdata->source);
+ if (in = NULL) {
+ dev_err(&spi->dev, "failed to find video source '%s'\n",
+ pdata->source);
+ return -EPROBE_DEFER;
+ }
+
+ ddata->in = in;
+
+ ddata->data_lines = pdata->data_lines;
+
+ dssdev = &ddata->dssdev;
+ dssdev->name = pdata->name;
+
+ return 0;
+}
+
+static int td028ttec1_panel_probe(struct spi_device *spi)
+{
+ struct panel_drv_data *ddata;
+ struct omap_dss_device *dssdev;
+ int r;
+
+ dev_dbg(&spi->dev, "%s\n", __func__);
+
+ spi->bits_per_word = 9;
+ spi->mode = SPI_MODE_3;
+
+ r = spi_setup(spi);
+ if (r < 0) {
+ dev_err(&spi->dev, "spi_setup failed: %d\n", r);
+ return r;
+ }
+
+ ddata = devm_kzalloc(&spi->dev, sizeof(*ddata), GFP_KERNEL);
+ if (ddata = NULL)
+ return -ENOMEM;
+
+ dev_set_drvdata(&spi->dev, ddata);
+
+ ddata->spi_dev = spi;
+
+ if (dev_get_platdata(&spi->dev)) {
+ r = td028ttec1_panel_probe_pdata(spi);
+ if (r)
+ return r;
+ } else {
+ return -ENODEV;
+ }
+
+ ddata->videomode = td028ttec1_panel_timings;
+
+ dssdev = &ddata->dssdev;
+ dssdev->dev = &spi->dev;
+ dssdev->driver = &td028ttec1_ops;
+ dssdev->type = OMAP_DISPLAY_TYPE_DPI;
+ dssdev->owner = THIS_MODULE;
+ dssdev->panel.timings = ddata->videomode;
+ dssdev->phy.dpi.data_lines = ddata->data_lines;
+
+ r = omapdss_register_display(dssdev);
+ if (r) {
+ dev_err(&spi->dev, "Failed to register panel\n");
+ goto err_reg;
+ }
+
+ return 0;
+
+err_reg:
+ omap_dss_put_device(ddata->in);
+ return r;
+}
+
+static int td028ttec1_panel_remove(struct spi_device *spi)
+{
+ struct panel_drv_data *ddata = dev_get_drvdata(&spi->dev);
+ struct omap_dss_device *dssdev = &ddata->dssdev;
+ struct omap_dss_device *in = ddata->in;
+
+ dev_dbg(&ddata->spi_dev->dev, "%s\n", __func__);
+
+ omapdss_unregister_display(dssdev);
+
+ td028ttec1_panel_disable(dssdev);
+ td028ttec1_panel_disconnect(dssdev);
+
+ omap_dss_put_device(in);
+
+ return 0;
+}
+
+static struct spi_driver td028ttec1_spi_driver = {
+ .probe = td028ttec1_panel_probe,
+ .remove = td028ttec1_panel_remove,
+
+ .driver = {
+ .name = "panel-tpo-td028ttec1",
+ .owner = THIS_MODULE,
+ },
+};
+
+module_spi_driver(td028ttec1_spi_driver);
+
+MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
+MODULE_DESCRIPTION("Toppoly TD028TTEC1 panel driver");
+MODULE_LICENSE("GPL");
diff --git a/include/video/omap-panel-data.h b/include/video/omap-panel-data.h
index f7ac8d9..69279c0 100644
--- a/include/video/omap-panel-data.h
+++ b/include/video/omap-panel-data.h
@@ -238,4 +238,17 @@ struct panel_nec_nl8048hl11_platform_data {
int qvga_gpio;
};
+/**
+ * panel-tpo-td028ttec1 platform data
+ * @name: name for display entity
+ * @source: name of the display entity used as a video source
+ * @data_lines: number of DPI datalines
+ */
+struct panel_tpo_td028ttec1_platform_data {
+ const char *name;
+ const char *source;
+
+ int data_lines;
+};
+
#endif /* __OMAP_PANEL_DATA_H */
--
1.8.1.2
^ permalink raw reply related
* Backlight enabled by default
From: Thierry Reding @ 2013-10-17 14:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jingoo Han, 'Tomi Valkeinen', 'Richard Purdie',
'Jean-Christophe Plagniol-Villard', linux-fbdev,
linux-kernel, 'Laurent Pinchart'
[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]
While working on integrating backlight functionality with the DRM sub-
system, I came across an oddity of sorts. The backlight subsystem seems
to have an implicit policy of enabling the backlight device when it's
registered. Pretty much every driver seems to do something like this in
.probe():
bl->props.brightness = default_brightness;
bl->props.fb_blank = FB_BLANK_UNBLANK;
backlight_update_status(bl);
There are variations of this theme, but the tendency is certainly to
enable the backlight once it has been registered. Some don't even set
the .fb_blank field, so it will usually stay initialized to zero, which
happens to be FB_BLANK_UNBLANK, so the result will be the same.
I understand that this is actually useful when using something such as
fbdev where the backlight isn't necessarily bound to a display device,
but when used in conjunction with a higher-level API such as DRM, then
this default behaviour becomes somewhat annoying.
The problem is that backlight devices are usually instantiated
separately from the display driver, so with the current default the
backlight will be enabled at some random point in time during boot.
However, DRM for example provides for a display to control very
precisely when the backlight should be enabled, which in turn makes it
easy to light it up right after the display has initialized. If the
backlight is turned on right after it has been probed this could mean
that the display hasn't been initialized yet and therefore there's no
meaningful content yet, and worse, the display might show garbage during
initialization of the display controller.
So I wonder if perhaps a better default would be to not enable the
backlight on boot. If so, this will actually cause a regression of some
sort on non-DRM systems because suddenly the backlight is no longer
enabled by default on boot.
Does anyone have any comments or ideas?
My first reaction was to add a property to the DT so that the driver
could skip enabling the backlight, but I don't think that will be
accepted because it encodes software policy in DT rather than purely
a description of hardware.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [Patch v2][ 05/37] fbdev: Add the lacking FB_SYNC_* for matching the DISPLAY_FLAGS_*
From: Denis Carikli @ 2013-10-17 15:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382022155-21954-1-git-send-email-denis@eukrea.com>
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>
---
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
* [Patch v2][ 07/37] video: mx3fb: Introduce regulator support.
From: Denis Carikli @ 2013-10-17 15:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382022155-21954-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>
---
drivers/video/mx3fb.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index 804f874..37704da 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>
@@ -267,6 +268,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 */
};
@@ -1001,6 +1003,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;
@@ -1019,6 +1022,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);
@@ -1026,7 +1038,17 @@ 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;
}
}
@@ -1202,6 +1224,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);
@@ -1210,7 +1233,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;
}
@@ -1222,10 +1253,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();
@@ -1438,6 +1479,10 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
if (ret < 0)
goto erfb;
+ mx3fbi->reg_lcd = devm_regulator_get(dev, "lcd");
+ if (IS_ERR(mx3fbi->reg_lcd))
+ mx3fbi->reg_lcd = NULL;
+
return 0;
erfb:
--
1.7.9.5
^ permalink raw reply related
* [Patch v2][ 08/37] video: mx3fb: Add device tree suport.
From: Denis Carikli @ 2013-10-17 15:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382022155-21954-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: 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>
---
.../devicetree/bindings/video/fsl,mx3-fb.txt | 52 ++++++++
drivers/video/Kconfig | 2 +
drivers/video/mx3fb.c | 133 +++++++++++++++++---
3 files changed, 171 insertions(+), 16 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..ae0b343
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fsl,mx3-fb.txt
@@ -0,0 +1,52 @@
+Freescale mx3 Framebuffer
+
+This framebuffer driver supports the imx31 and imx35 devices.
+
+Required properties:
+- compatible : Must be "fsl,mx3-fb".
+- reg : Should contain 1 register ranges(address and length).
+- dmas : Phandle to the ipu dma node as described in
+ Documentation/devicetree/bindings/dma/dma.txt
+- dma-names : Must be "tx".
+- clocks : Phandle to the ipu source clock.
+- display: Phandle to a display node as described in
+ Documentation/devicetree/bindings/video/display-timing.txt
+ Additional, the display node has to define properties:
+ - bits-per-pixel: lcd panel bit-depth.
+
+Optional properties:
+- ipu-disp-format: could be "rgb666", "rgb565", or "rgb888".
+ If not specified defaults to "rgb666".
+
+Example:
+
+ lcdc: mx3fb@53fc00b4 {
+ compatible = "fsl,mx3-fb";
+ reg = <0x53fc00b4 0x0b>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_lcdc_1>;
+ clocks = <&clks 55>;
+ dmas = <&ipu 14>;
+ dma-names = "tx";
+ };
+
+ ...
+
+ cmo_qvga: display {
+ model = "CMO-QVGA";
+ bits-per-pixel = <16>;
+ native-mode = <&qvga_timings>;
+ display-timings {
+ qvga_timings: 320x240 {
+ clock-frequency = <6500000>;
+ hactive = <320>;
+ vactive = <240>;
+ hback-porch = <30>;
+ hfront-porch = <38>;
+ vback-porch = <20>;
+ vfront-porch = <3>;
+ hsync-len = <15>;
+ vsync-len = <4>;
+ };
+ };
+ };
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 37704da..8683dda 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -32,6 +32,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>
@@ -759,11 +761,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;
@@ -1392,21 +1396,63 @@ 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("rgb888", 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;
+ enum disp_data_mapping disp_data_fmt = IPU_DISP_DATA_MAPPING_RGB666;
+ struct device_node *display_np, *timings_np;
+
+ if (np) {
+ of_property_read_string(np, "ipu-disp-format",
+ &ipu_disp_format);
+ if (!ipu_disp_format) {
+ dev_err(dev, "Can't get ipu display data mapping.\n");
+ return -EINVAL;
+ }
+
+ 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",
+ display_np = of_parse_phandle(np, "display", 0);
+ if (!display_np) {
+ dev_err(dev, "failed to find display phandle\n");
+ return -ENOENT;
+ }
+
+ of_property_read_string(display_np, "model", &name);
+ } 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;
@@ -1427,12 +1473,39 @@ 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) {
+ of_property_read_string(np, "ipu-disp-format",
+ &ipu_disp_format);
+ if (!ipu_disp_format) {
+ dev_err(dev, "Can't get ipu display data mapping.\n");
+ return -EINVAL;
+ }
+
+ disp_data_fmt = match_dt_disp_data(ipu_disp_format);
+
+ if (disp_data_fmt = -EINVAL) {
+ dev_err(dev, "Illegal display data format \"%s\" for "
+ "the node %s\n", ipu_disp_format, np->name);
+ return -EINVAL;
+ }
+
+ 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,
@@ -1462,7 +1535,10 @@ 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;
+ else
+ mx3fb->disp_data_fmt = disp_data_fmt;
init_completion(&mx3fbi->flip_cmpl);
disable_irq(ichan->eof_irq);
@@ -1494,13 +1570,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)
@@ -1509,8 +1598,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)
@@ -1567,7 +1660,8 @@ static int mx3fb_probe(struct platform_device *pdev)
dma_cap_set(DMA_SLAVE, mask);
dma_cap_set(DMA_PRIVATE, mask);
rq.id = IDMAC_SDC_0;
- chan = dma_request_channel(mask, chan_filter, &rq);
+ chan = dma_request_slave_channel_compat(mask, chan_filter, &rq, dev,
+ "tx");
if (!chan) {
ret = -EBUSY;
goto ersdc0;
@@ -1610,9 +1704,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
* [Patch v2][ 09/37] video: imxfb: Introduce regulator support.
From: Denis Carikli @ 2013-10-17 15:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382022155-21954-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: 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 | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 44ee678..4dd7678 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,10 @@ 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))
+ fbi->reg_lcd = NULL;
+
imxfb_enable_controller(fbi);
fbi->pdev = pdev;
#ifdef PWMR_BACKLIGHT_AVAILABLE
--
1.7.9.5
^ permalink raw reply related
* [Patch v2][ 10/37] video: imxfb: Also add pwmr for the device tree.
From: Denis Carikli @ 2013-10-17 15:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1382022155-21954-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>
---
.../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 4dd7678..9da29b3 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
* Re: How to set fops in "struct platform_pwm_backlight_data"?
From: Mark Zhang @ 2013-10-18 4:48 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: <20131017071445.GA2502@ulmo.nvidia.com>
On 10/17/2013 03:14 PM, Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 02:49:57PM +0800, Mark Zhang wrote:
>> Hi,
>>
>> This is the first time I send mail to linux-pwm, I didn't read through
>> the mails in this list, so if somebody already asked this question, I'm
>> sorry about that.
>>
>> I wanna set some fops in "struct platform_pwm_backlight_data". But the
>> currrent probe function in pwm_bl.c says:
>>
>> -------
>> if (!data) {
>> ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
>> if (ret < 0) {
>> dev_err(&pdev->dev, "failed to find platform data\n");
>> return ret;
>> }
>>
>> data = &defdata;
>> }
>> -------
>>
>> This looks like if we set the platform data for pwm backlight device,
>> "pwm_backlight_parse_dt" will never have a chance to be called, which
>> means the stuffs I defined in backlight DT node will be ignored.
>>
>> If I don't set the platform data for pwm backlight device, according to
>> the pwm_backlight_probe, I will never have a chance to set some fops
>> which I need(like "notify", "check_fb"...).
>>
>> So, what I suppose to do now? Maybe there is a way to set function
>> pointers in DT?
>
> Perhaps you could describe in more detail what you need the functions
> for.
>
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.
Mark
> Generally you're not supposed to mix DT and platform data. Without more
> info that's about all I can say.
>
> Thierry
>
^ permalink raw reply
* Re: [PATCH v2] pwm-backlight: allow for non-increasing brightness levels
From: Thierry Reding @ 2013-10-18 7:46 UTC (permalink / raw)
To: Mike Dunn
Cc: Richard Purdie, Jingoo Han, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Grant Likely, Rob Herring, linux-pwm, linux-fbdev,
linux-kernel, devicetree, Robert Jarzmik, Marek Vasut
In-Reply-To: <1379869196-19377-1-git-send-email-mikedunn@newsguy.com>
[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]
On Sun, Sep 22, 2013 at 09:59:56AM -0700, Mike Dunn wrote:
> Currently the driver assumes that the values specified in the
> brightness-levels device tree property increase as they are parsed from
> left to right. But boards that invert the signal between the PWM output
> and the backlight will need to specify decreasing brightness-levels.
> This patch removes the assumption that the last element of the array is
> the maximum value, and instead searches the array for the maximum value
> and uses that in the duty cycle calculation.
>
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> Changelog:
> v2:
> - commit message reworded; correct line wrap used
> - 'max_level' variable renamed to 'scale'
> - loop counter variable type changed to unsigned int
> - value held in scale changed from array index to actual maximum level
> - blank lines added around loop for readability
>
> drivers/video/backlight/pwm_bl.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
Hey Mike,
I've pushed a slightly different version of this patch which gets rid of
the intermediate max variable and uses the new scale field exclusively
to pass the same information around. Could you look at the patch from my
for-next branch in the PWM tree and see whether that still works for the
specific hardware that you need this for?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] set data enable logic level to low
From: Tomi Valkeinen @ 2013-10-18 8:01 UTC (permalink / raw)
To: Roel Kluin
Cc: Jean-Christophe Plagniol-Villard, linux-omap, linux-fbdev,
linux-kernel
In-Reply-To: <1381704273-31540-1-git-send-email-roel.kluin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 982 bytes --]
On 14/10/13 01:44, Roel Kluin wrote:
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> drivers/video/omap2/dss/display.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/omap2/dss/display.c b/drivers/video/omap2/dss/display.c
> index fafe7c9..669a81f 100644
> --- a/drivers/video/omap2/dss/display.c
> +++ b/drivers/video/omap2/dss/display.c
> @@ -266,7 +266,7 @@ void videomode_to_omap_video_timings(const struct videomode *vm,
> OMAPDSS_SIG_ACTIVE_LOW;
> ovt->de_level = vm->flags & DISPLAY_FLAGS_DE_HIGH ?
> OMAPDSS_SIG_ACTIVE_HIGH :
> - OMAPDSS_SIG_ACTIVE_HIGH;
> + OMAPDSS_SIG_ACTIVE_LOW;
> ovt->data_pclk_edge = vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE ?
> OMAPDSS_DRIVE_SIG_RISING_EDGE :
> OMAPDSS_DRIVE_SIG_FALLING_EDGE;
>
Thanks. Next time, please write a patch description and prefix patch
subject with appropriate string. In this case "OMAPDSS: ..." would be fine.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 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