* Re: [PATCH] backlight: check null deference of name when device is registered
From: Andrew Morton @ 2013-01-08 1:35 UTC (permalink / raw)
To: Jingoo Han
Cc: 'LKML', linux-fbdev, 'Richard Purdie',
'Devendra Naga'
In-Reply-To: <003c01cded3f$0f25a3e0$2d70eba0$%han@samsung.com>
On Tue, 08 Jan 2013 10:25:35 +0900 Jingoo Han <jg1.han@samsung.com> wrote:
> On Tuesday, January 08, 2013 9:02 AM, Andrew Morton wrote
> > On Fri, 04 Jan 2013 17:29:11 +0900
> > Jingoo Han <jg1.han@samsung.com> wrote:
> >
> > > NULL deference of name is checked when device is registered.
> > > If the name is null, it will cause a kernel oops in dev_set_name().
> > >
> > > ...
> > >
> > > --- a/drivers/video/backlight/backlight.c
> > > +++ b/drivers/video/backlight/backlight.c
> > > @@ -292,6 +292,11 @@ struct backlight_device *backlight_device_register(const char *name,
> > > struct backlight_device *new_bd;
> > > int rc;
> > >
> > > + if (name = NULL) {
> > > + pr_err("backlight name is null\n");
> > > + return ERR_PTR(-EINVAL);
> > > + }
> > > +
> > > pr_debug("backlight_device_register: name=%s\n", name);
> >
> > I don't understand this.
> >
> > Is there some driver which is calling these functions with name=NULL?
> > If so, which one(s)?
>
> No, there is no one.
>
> >
> > If "no" then why don't we declare that "passing name=NULL is a bug" and
> > leave the code as-is?
>
> Do you mean following?
>
> + if (name = NULL)
> + pr_err("passing name=NULL is a bug");
> +
> pr_debug("backlight_device_register: name=%s\n", name);
Nope; I'm suggesting we leave the code alone. If someone passes in
NULL they will get a nice oops and their bug will then get fixed.
^ permalink raw reply
* Re: [PATCH] backlight: check null deference of name when device is registered
From: Jingoo Han @ 2013-01-08 1:39 UTC (permalink / raw)
To: 'Andrew Morton'
Cc: 'LKML', linux-fbdev, 'Richard Purdie',
'Devendra Naga', 'Jingoo Han'
In-Reply-To: <20130107173557.dadc0a7a.akpm@linux-foundation.org>
On Tuesday, January 08, 2013 10:36 AM, Andrew Morton wrote
> On Tue, 08 Jan 2013 10:25:35 +0900 Jingoo Han <jg1.han@samsung.com> wrote:
>
> > On Tuesday, January 08, 2013 9:02 AM, Andrew Morton wrote
> > > On Fri, 04 Jan 2013 17:29:11 +0900
> > > Jingoo Han <jg1.han@samsung.com> wrote:
> > >
> > > > NULL deference of name is checked when device is registered.
> > > > If the name is null, it will cause a kernel oops in dev_set_name().
> > > >
> > > > ...
> > > >
> > > > --- a/drivers/video/backlight/backlight.c
> > > > +++ b/drivers/video/backlight/backlight.c
> > > > @@ -292,6 +292,11 @@ struct backlight_device *backlight_device_register(const char *name,
> > > > struct backlight_device *new_bd;
> > > > int rc;
> > > >
> > > > + if (name = NULL) {
> > > > + pr_err("backlight name is null\n");
> > > > + return ERR_PTR(-EINVAL);
> > > > + }
> > > > +
> > > > pr_debug("backlight_device_register: name=%s\n", name);
> > >
> > > I don't understand this.
> > >
> > > Is there some driver which is calling these functions with name=NULL?
> > > If so, which one(s)?
> >
> > No, there is no one.
> >
> > >
> > > If "no" then why don't we declare that "passing name=NULL is a bug" and
> > > leave the code as-is?
> >
> > Do you mean following?
> >
> > + if (name = NULL)
> > + pr_err("passing name=NULL is a bug");
> > +
> > pr_debug("backlight_device_register: name=%s\n", name);
>
> Nope; I'm suggesting we leave the code alone. If someone passes in
> NULL they will get a nice oops and their bug will then get fixed.
I see. I agree with you.
Please, abandon this patch.
Best regards,
Jingoo Han
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] backlight: check null deference of name when device is registered
From: devendra.aaru @ 2013-01-08 4:28 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jingoo Han, LKML, linux-fbdev, Richard Purdie
In-Reply-To: <20130107173557.dadc0a7a.akpm@linux-foundation.org>
On Mon, Jan 7, 2013 at 8:35 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 08 Jan 2013 10:25:35 +0900 Jingoo Han <jg1.han@samsung.com> wrote:
>
>> On Tuesday, January 08, 2013 9:02 AM, Andrew Morton wrote
>> > On Fri, 04 Jan 2013 17:29:11 +0900
>> > Jingoo Han <jg1.han@samsung.com> wrote:
>> >
>> > > NULL deference of name is checked when device is registered.
>> > > If the name is null, it will cause a kernel oops in dev_set_name().
>> > >
>> > > ...
>> > >
>> > > --- a/drivers/video/backlight/backlight.c
>> > > +++ b/drivers/video/backlight/backlight.c
>> > > @@ -292,6 +292,11 @@ struct backlight_device *backlight_device_register(const char *name,
>> > > struct backlight_device *new_bd;
>> > > int rc;
>> > >
>> > > + if (name = NULL) {
>> > > + pr_err("backlight name is null\n");
>> > > + return ERR_PTR(-EINVAL);
>> > > + }
>> > > +
>> > > pr_debug("backlight_device_register: name=%s\n", name);
>> >
>> > I don't understand this.
>> >
>> > Is there some driver which is calling these functions with name=NULL?
>> > If so, which one(s)?
>>
>> No, there is no one.
>>
>> >
>> > If "no" then why don't we declare that "passing name=NULL is a bug" and
>> > leave the code as-is?
>>
>> Do you mean following?
>>
>> + if (name = NULL)
>> + pr_err("passing name=NULL is a bug");
>> +
>> pr_debug("backlight_device_register: name=%s\n", name);
>
> Nope; I'm suggesting we leave the code alone. If someone passes in
> NULL they will get a nice oops and their bug will then get fixed.
>
We still fail the probe in the patch Jingoo Han sent,
anyways if i catch your point correctly is this the lines you wanted?
+ /**
+ * BUG if the name of the backlight device
+ * is a NULL
+ */
+ BUG_ON(!name);
^ permalink raw reply
* RE: [PATCHv16 5/7] fbmon: add of_videomode helpers
From: Mohammed, Afzal @ 2013-01-08 5:31 UTC (permalink / raw)
To: Rob Clark
Cc: Steffen Trumtrar, devicetree-discuss@lists.ozlabs.org,
linux-fbdev@vger.kernel.org, David Airlie,
Florian Tobias Schandinat, dri-devel@lists.freedesktop.org,
Valkeinen, Tomi, Laurent Pinchart, kernel@pengutronix.de,
Guennady Liakhovetski, linux-media@vger.kernel.org, Nori, Sekhar
In-Reply-To: <CAF6AEGuuM9_n+A4q4tq+24i4YcW97orMN_RKbJ95gFie_qoktA@mail.gmail.com>
SGkgUm9iLA0KDQpPbiBUdWUsIEphbiAwOCwgMjAxMyBhdCAwMTozNjo1MCwgUm9iIENsYXJrIHdy
b3RlOg0KPiBPbiBNb24sIEphbiA3LCAyMDEzIGF0IDI6NDYgQU0sIE1vaGFtbWVkLCBBZnphbCA8
YWZ6YWxAdGkuY29tPiB3cm90ZToNCj4gPiBPbiBNb24sIEphbiAwNywgMjAxMyBhdCAxMzozNjo0
OCwgU3RlZmZlbiBUcnVtdHJhciB3cm90ZToNCg0KPiA+PiBJIGp1c3QgZGlkIGEgcXVpY2sgIm1h
a2UgZGE4eHhfb21hcGxfZGVmY29uZmlnICYmIG1ha2UiIGFuZCBpdCBidWlsZHMganVzdCBmaW5l
Lg0KPiA+PiBPbiB3aGF0IHZlcnNpb24gZGlkIHlvdSBhcHBseSB0aGUgc2VyaWVzPw0KPiA+PiBB
dCB0aGUgbW9tZW50IEkgaGF2ZSB0aGUgc2VyaWVzIHNpdHRpbmcgb24gMy43LiBEaWRuJ3QgdHJ5
IGFueSAzLjgtcmN4IHlldC4NCj4gPj4gQnV0IGZpeGluZyB0aGlzIHNob3VsZG4ndCBiZSBhIHBy
b2JsZW0uDQoNCj4gPiBUaGUgY2hhbmdlIGFzIEkgbWVudGlvbmVkIG9yIHNvbWV0aGluZyBzaW1p
bGFyIHdvdWxkIGJlIHJlcXVpcmVkIGFzDQo+ID4gYW55IGRyaXZlciB0aGF0IGlzIGdvaW5nIHRv
IG1ha2UgdXNlIG9mIG9mX2dldF9mYl92aWRlb21vZGUoKSB3b3VsZA0KPiA+IGJyZWFrIGlmIENP
TkZJR19PRl9WSURFT01PREUgb3IgQ09ORklHX0ZCX01PREVfSEVMUEVSUyBpcyBub3QgZGVmaW5l
ZC4NCg0KPiBTaG91bGRuJ3QgdGhlIGRyaXZlciB0aGF0IGRlcGVuZHMgb24gQ09ORklHX09GX1ZJ
REVPTU9ERSBhbmQNCj4gQ09ORklHX0ZCX01PREVfSEVMUEVSUywgZXhwbGljaXRseSBzZWxlY3Qg
dGhlbT8gIEkgZG9uJ3QgcmVhbGx5IHNlZQ0KPiB0aGUgcG9pbnQgb2YgaGF2aW5nIHRoZSBzdGF0
aWMtaW5saW5lIGZhbGxiYWNrcy4NCg0KQnV0IGhlcmUgZGE4eHgtZmIgZHJpdmVyIGRvZXMgbm90
IGRlcGVuZCBvbiBfT0ZfVklERU9NT0RFIGFuZA0KX0ZCX01PREVfSEVMUEVSUywgY3VycmVudGx5
IGl0IHdvcmtzIGFzIGEgcHVyZSBwbGF0Zm9ybSBkcml2ZXINCmZvciBEYVZpbmNpIFNvQydzIHdp
dGhvdXQgdGhvc2UgQ09ORklHJ3MuIEl0IGlzIG9ubHkgdXBvbg0KZW5oYW5jaW5nIHRoZSBkcml2
ZXIgdG8gbWFrZSB1c2Ugb2Ygb2ZfZ2V0X2ZiX3ZpZGVvbW9kZSgpIGZvcg0KRFQgc3VwcG9ydCB0
aG9zZSBDT05GSUcncyBhcmUgYmVpbmcgbWFkZSB1c2Ugb2YuDQoNCkFzIHRoZSBkcml2ZXIgY2Fu
IHdvcmsgdy9vIHRoZXNlIENPTkZJRydzIGFuZCBzbyBhcyBpdCBpcyBub3QgYQ0KZGVwZW5kZW5j
eSBmb3IgZHJpdmVyIG9uIG5vbi1EVCBib290IChhcyBpbiB0aGUgY2FzZSBvZiBEYVZpbmNpKSwN
CkkgZGlzYWdyZWUgaW4gc2VsZWN0aW5nIHRob3NlIG9wdGlvbnMgYWx3YXlzLCBidXQgcmF0aGVy
IGdpdmluZw0KdXNlciBhbiBvcHRpb24gdG8gc2VsZWN0Lg0KDQpBbmQgc2VsZWN0aW5nIHRoZXNl
IG9wdGlvbnMgYWx3YXlzIHdpbGwgYnJpbmcgaW4gc29tZSBhbW91bnQgb2YgY29kZQ0Kb250byBL
ZXJuZWwgaW1hZ2Ugdy9vIGFueSBwdXJwb3NlIGluIHRoZSBjYXNlIG9mIERhVmluY2kgYnVpbGRz
Lg0KDQpBbm90aGVyIG9wdGlvbiB3b3VsZCBiZSB0byBzcHJpbmtsZSBkcml2ZXIgd2l0aCBpZmRl
ZidzIHRvIGF2b2lkDQppbmxpbmUgZmFsbGJhY2tzLCB3aGljaCBpcyBub3QgYSBnb29kIHRoaW5n
IHRvIGRvLg0KDQpNb3Jlb3ZlciBoYXZpbmcgYSBzdGF0aWMgaW5saW5lIGZhbGxiYWNrIGlzIG1v
cmUgaW4gbGluZSB3aXRoIG90aGVyDQpvZl8qJ3MuDQoNCj4gZndpdywgdXNpbmcgJ3NlbGVjdCcg
aXMgd2hhdCBJIHdhcyBkb2luZyBmb3IgbGNkIHBhbmVsIHN1cHBvcnQgZm9yDQo+IGxjZGMvZGE4
eHggZHJtIGRyaXZlciAod2hpY2ggd2FzIHVzaW5nIHRoZSBvZiB2aWRlb21vZGUgaGVscGVycywN
Cj4gYWxiZWl0IGEgc2xpZ2h0bHkgZWFybGllciB2ZXJzaW9uIG9mIHRoZSBwYXRjaGVzKToNCg0K
SW4geW91ciBjYXNlIGFzIGl0IGlzIGEgbmV3IGRyaXZlciAmIGlzIG1lYW50IG9ubHkgZm9yIERU
LCB0aGF0DQppcyBmaW5lLCBidXQgaGVyZSBpdCBpcyBhbiBleGlzdGluZyBkcml2ZXIgdGhhdCB3
b3JrcyB3L28gdGhlc2UuDQoNClJlZ2FyZHMNCkFmemFsDQoNCg=
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Laurent Pinchart @ 2013-01-08 8:18 UTC (permalink / raw)
To: Tomasz Figa
Cc: dri-devel, Vikas Sajjan, Thomas Petazzoni, linux-fbdev,
Benjamin Gaignard, Tom Gall, Kyungmin Park, Rob Clark,
Ragesh Radhakrishnan, Tomi Valkeinen, Philipp Zabel, Bryan Wu,
Maxime Ripard, sunil joshi, Sumit Semwal, Sebastien Guiriec,
linux-media
In-Reply-To: <2529718.glQX8guWfJ@amdc1227>
Hi Tomasz,
On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:
> On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:
> > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:
> > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:
> > > > On 17 December 2012 20:55, Laurent Pinchart wrote:
> > > > > Hi Vikas,
> > > > >
> > > > > Sorry for the late reply. I now have more time to work on CDF, so
> > > > > delays should be much shorter.
> > > > >
> > > > > On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:
> > > > > > Hi Laurent,
> > > > > >
> > > > > > I was thinking of porting CDF to samsung EXYNOS 5250 platform,
> > > > > > what I found is that, the exynos display controller is MIPI DSI
> > > > > > based controller.
> > > > > >
> > > > > > But if I look at CDF patches, it has only support for MIPI DBI
> > > > > > based Display controller.
> > > > > >
> > > > > > So my question is, do we have any generic framework for MIPI DSI
> > > > > > based display controller? basically I wanted to know, how to go
> > > > > > about porting CDF for such kind of display controller.
> > > > >
> > > > > MIPI DSI support is not available yet. The only reason for that is
> > > > > that I don't have any MIPI DSI hardware to write and test the code
> > > > > with :-)
> > > > >
> > > > > The common display framework should definitely support MIPI DSI. I
> > > > > think the existing MIPI DBI code could be used as a base, so the
> > > > > implementation shouldn't be too high.
> > > > >
> > > > > Yeah, i was also thinking in similar lines, below is my though for
> > > > > MIPI DSI support in CDF.
> > > >
> > > > o MIPI DSI support as part of CDF framework will expose
> > > > § mipi_dsi_register_device(mpi_device) (will be called mach-xxx-dt.c
> > > > file )
> > > > § mipi_dsi_register_driver(mipi_driver, bus ops) (will be called
> > > > from platform specific init driver call )
> > > > · bus ops will be
> > > > o read data
> > > > o write data
> > > > o write command
> > > > § MIPI DSI will be registered as bus_register()
> > > >
> > > > When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI)
> > > > will initialize the MIPI DSI HW IP.
> > > >
> > > > This probe will also parse the DT file for MIPI DSI based panel, add
> > > > the panel device (device_add() ) to kernel and register the display
> > > > entity with its control and video ops with CDF.
> > > >
> > > > I can give this a try.
> > >
> > > I am currently in progress of reworking Exynos MIPI DSIM code and
> > > s6e8ax0 LCD driver to use the v2 RFC of Common Display Framework. I
> > > have most of the work done, I have just to solve several remaining
> > > problems.
> >
> > Do you already have code that you can publish ? I'm particularly
> > interested (and I think Tomi Valkeinen would be as well) in looking at
> > the DSI operations you expose to DSI sinks (panels, transceivers, ...).
>
> Well, I'm afraid this might be little below your expectations, but here's
> an initial RFC of the part defining just the DSI bus. I need a bit more
> time for patches for Exynos MIPI DSI master and s6e8ax0 LCD.
No worries. I was particularly interested in the DSI operations you needed to
export, they seem pretty simple. Thank you for sharing the code.
> The implementation is very simple and heavily based on your MIPI DBI
> support and existing Exynos MIPI DSIM framework. Provided operation set is
> based on operation set used by Exynos s6e8ax0 LCD driver. Unfortunately
> this is my only source of information about MIPI DSI.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Laurent Pinchart @ 2013-01-08 8:25 UTC (permalink / raw)
To: Rob Clark
Cc: Dave Airlie, Thomas Petazzoni, Linux Fbdev development list,
Philipp Zabel, Tom Gall, Ragesh Radhakrishnan,
dri-devel@lists.freedesktop.org, Kyungmin Park, Tomi Valkeinen,
Benjamin Gaignard, Bryan Wu, Maxime Ripard, Vikas Sajjan,
Sumit Semwal, Sebastien Guiriec, linux-media@vger.kernel.org
In-Reply-To: <CAF6AEGt+gwUq-xGze5bTgrKUMRijSBo_ORreq=Ot1RMD-WrbYQ@mail.gmail.com>
Hi Rob,
On Thursday 27 December 2012 09:54:55 Rob Clark wrote:
> On Mon, Dec 24, 2012 at 7:37 AM, Laurent Pinchart wrote:
> > On Tuesday 18 December 2012 00:21:32 Rob Clark wrote:
> >> On Mon, Dec 17, 2012 at 11:04 PM, Dave Airlie <airlied@gmail.com> wrote:
> >> >> Many developers showed interest in the first RFC, and I've had the
> >> >> opportunity to discuss it with most of them. I would like to thank (in
> >> >> no particular order) Tomi Valkeinen for all the time he spend helping
> >> >> me to draft v2, Marcus Lorentzon for his useful input during Linaro
> >> >> Connect Q4 2012, and Linaro for inviting me to Connect and providing a
> >> >> venue to discuss this topic.
> >> >
> >> > So this might be a bit off topic but this whole CDF triggered me
> >> > looking at stuff I generally avoid:
> >> >
> >> > The biggest problem I'm having currently with the whole ARM graphics
> >> > and output world is the proliferation of platform drivers for every
> >> > little thing. The whole ordering of operations with respect to things
> >> > like suspend/resume or dynamic power management is going to be a real
> >> > nightmare if there are dependencies between the drivers. How do you
> >> > enforce ordering of s/r operations between all the various components?
> >>
> >> I tend to think that sub-devices are useful just to have a way to probe
> >> hw which may or may not be there, since on ARM we often don't have any
> >> alternative.. but beyond that, suspend/resume, and other life-cycle
> >> aspects, they should really be treated as all one device. Especially to
> >> avoid undefined suspend/resume ordering.
> >
> > I tend to agree, except that I try to reuse the existing PM infrastructure
> > when possible to avoid reinventing the wheel. So far handling
> > suspend/resume ordering related to data busses in early suspend/late
> > resume operations and allowing the Linux PM core to handle control busses
> > using the Linux device tree worked pretty well.
> >
> >> CDF or some sort of mechanism to share panel drivers between drivers is
> >> useful. Keeping it within drm, is probably a good idea, if nothing else
> >> to simplify re-use of helper fxns (like avi-infoframe stuff, for example)
> >> and avoid dealing with merging changes across multiple trees. Treating
> >> them more like shared libraries and less like sub-devices which can be
> >> dynamically loaded/unloaded (ie. they should be not built as separate
> >> modules or suspend/resumed or probed/removed independently of the master
> >> driver) is a really good idea to avoid uncovering nasty synchronization
> >> issues later (remove vs modeset or pageflip) or surprising userspace in
> >> bad ways.
> >
> > We've tried that in V4L2 years ago and realized that the approach led to a
> > dead-end, especially when OF/DT got involved. With DT-based device
> > probing, I2C camera sensors started getting probed asynchronously to the
> > main camera device, as they are children of the I2C bus master. We will
> > have similar issues with I2C HDMI transmitters or panels, so we should be
> > prepared for it.
>
> What I've done to avoid that so far is that the master device registers the
> drivers for it's output sub-devices before registering it's own device.
I'm not sure to follow you here. The master device doesn't register anything,
do you mean the master device driver ? If so, how does the master device
driver register its own device ? Devices are not registered by their driver.
> At least this way I can control that they are probed first. Not the
> prettiest thing, but avoids even uglier problems.
>
> > On PC hardware the I2C devices are connected to an I2C master provided by
> > the GPU, but on embedded devices they are usually connected to an
> > independent I2C master. We thus can't have a single self-contained driver
> > that controls everything internally, and need to interface with the rest
> > of the SoC drivers.
> >
> > I agree that probing/removing devices independently of the master driver
> > can lead to bad surprises, which is why I want to establish clear rules
> > in CDF regarding what can and can't be done with display entities.
> > Reference counting will be one way to make sure that devices don't
> > disappear all of a sudden.
>
> That at least helps cover some issues.. although it doesn't really help
> userspace confusion.
>
> Anyways, with enough work perhaps all problems could be solved.. otoh, there
> are plenty of other important problems to solve in the world of gpus and
> kms, so my preference is always not to needlessly over-complicate CDF and
> instead leave some time for other things
My customer is interested in CDF at the moment. If they ask me to solve other
GPU-related problems, sure, I can work on that, but that's not planned.
> >> > The other thing I'd like you guys to do is kill the idea of fbdev and
> >> > v4l drivers that are "shared" with the drm codebase, really just
> >> > implement fbdev and v4l on top of the drm layer, some people might
> >> > think this is some sort of maintainer thing, but really nothing else
> >> > makes sense, and having these shared display frameworks just to avoid
> >> > having using drm/kms drivers seems totally pointless. Fix the drm
> >> > fbdev emulation if an fbdev interface is needed. But creating a fourth
> >> > framework because our previous 3 frameworks didn't work out doesn't
> >> > seem like a situation I want to get behind too much.
> >>
> >> yeah, let's not have multiple frameworks to do the same thing.. For
> >> fbdev, it is pretty clear that it is a dead end. For v4l2 (subdev+mcf),
> >> it is perhaps bit more flexible when it comes to random arbitrary hw
> >> pipelines than kms. But to take advantage of that, your userspace isn't
> >> going to be portable anyways, so you might as well use driver specific
> >> properties/ioctls. But I tend to think that is more useful for cameras.
> >> And from userspace perspective, kms planes are less painful to use for
> >> output than v4l2, so lets stick to drm/kms for output (and not try to add
> >> camera/capture support to kms)..
> >
> > Agreed. I've started to advocate the deprecation of FBDEV during LPC. The
> > positive response has motivated me to continue doing so :-) For V4L2 the
> > situation is a little bit different, I think V4L2 shouldn't be used for
> > graphics and display hardware, but it still has use cases on the video
> > output side for pure video devices (such as pass-through video pipelines
> > with embedded processing for instance). As those can use subdevices found
> > in display and graphics hardware, I'd like to avoid code duplication.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Laurent Pinchart @ 2013-01-08 8:33 UTC (permalink / raw)
To: Sascha Hauer
Cc: Rob Clark, Thomas Petazzoni, Linux Fbdev development list,
Benjamin Gaignard, Tom Gall, Kyungmin Park,
dri-devel@lists.freedesktop.org, Ragesh Radhakrishnan,
Tomi Valkeinen, Philipp Zabel, Maxime Ripard, Vikas Sajjan,
Sumit Semwal, Sebastien Guiriec, linux-media@vger.kernel.org
In-Reply-To: <20121228000404.GY26326@pengutronix.de>
On Friday 28 December 2012 01:04:04 Sascha Hauer wrote:
> On Thu, Dec 27, 2012 at 01:57:56PM -0600, Rob Clark wrote:
> > On Thu, Dec 27, 2012 at 1:18 PM, Sascha Hauer wrote:
> > > On Thu, Dec 27, 2012 at 09:54:55AM -0600, Rob Clark wrote:
> > >> On Mon, Dec 24, 2012 at 7:37 AM, Laurent Pinchart
> > >
> > > This implies that the master driver knows all potential subdevices,
> > > something which is not true for SoCs which have external i2c encoders
> > > attached to unrelated i2c controllers.
> >
> > well, it can be brute-forced.. ie. drm driver calls common
> > register_all_panels() fxn, which, well, registers all the
> > panel/display subdev's based on their corresponding CONFIG_FOO_PANEL
> > defines. If you anyways aren't building the panels as separate
> > modules, that would work. Maybe not the most *elegant* approach, but
> > simple and functional.
> >
> > I guess it partly depends on the structure in devicetree. If you are
> >
> > assuming that the i2c encoder belongs inside the i2c bus, like:
> > &i2cN {
> >
> > foo-i2c-encoder {
> >
> > ....
> >
> > };
> >
> > };
> >
> > and you are letting devicetree create the devices, then it doesn't
> > quite work. I'm not entirely convinced you should do it that way.
> > Really any device like that is going to be hooked up to at least a
> > couple busses.. i2c, some sort of bus carrying pixel data, maybe some
> > gpio's, etc. So maybe makes more sense for a virtual drm/kms bus, and
> > then use phandle stuff to link it to the various other busses it
> >
> > needs:
> > mydrmdev {
> > foo-i2c-encoder {
> > i2c = <&i2cN>;
> > gpio = <&gpioM 2 3>
> > ...
> > };
> > };
>
> This seems to shift initialization order problem to another place. Here we
> have to make sure the controller is initialized before the drm driver. Same
> with suspend/resume.
>
> It's not only i2c devices, also platform devices. On i.MX for example we
> have a hdmi transmitter which is somewhere on the physical address space.
>
> I think grouping the different units together in a devicetree blob because
> we think they might form a logical virtual device is not going to work. It
> might make it easier from a drm perspective, but I think doing this will
> make for a lot of special cases. What will happen for example if you have
> two encoder devices in a row to configure? The foo-i2c-encoder would then
> get another child node.
>
> Right now the devicetree is strictly ordered by (control-, not data-) bus
> topology. Linux has great helper code to support this model. Giving up this
> help to brute force a different topology and then trying to fit the result
> back into the Linux Bus hierarchy doesn't sound like a good idea to me.
I agree. The Linux device model is architectured around a control bus based
tree, I don't want to change that. With devices hooked up on several busses we
will have dependency issues anyway, regardless of how we describe them in DT.
If we hook up the nodes from a data bus perspective we will run into control
bus dependency issues. It's thus better in my opinion to keep the classic
control bus based model and solve the data bus dependency issues.
> > ok, admittedly that is a bit different from other proposals about how
> > this all fits in devicetree.. but otoh, I'm not a huge believer in
> > letting something that is supposed to make life easier (DT), actually
> > make things harder or more complicated. Plus this CDF stuff all needs
> > to also work on platforms not using OF/DT.
>
> Right, but every other platform I know of is also described by its bus
> topology, be it platform device based or PCI or maybe even USB based.
>
> CDF has to solve the same problem as ASoC and soc-camera: subdevices for
> a virtual device can come from many different corners of the system. BTW
> one example for a i2c encoder would be the SiI9022 which could not only
> be part of a drm device, but also of an ASoC device.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Laurent Pinchart @ 2013-01-08 8:41 UTC (permalink / raw)
To: Daniel Vetter
Cc: Rob Clark, Thomas Petazzoni, linux-fbdev@vger.kernel.org,
Philipp Zabel, Tom Gall, Ragesh Radhakrishnan,
dri-devel@lists.freedesktop.org, marcus.lorentzon, Kyungmin Park,
Tomi Valkeinen, Benjamin Gaignard, Maxime Ripard, Vikas Sajjan,
Sumit Semwal, Sebastien Guiriec, linux-media@vger.kernel.org,
Luc Verhaegen
In-Reply-To: <20130106174647.GO5737@phenom.ffwll.local>
Hi Daniel,
On Sunday 06 January 2013 18:46:47 Daniel Vetter wrote:
> On Thu, Dec 27, 2012 at 09:57:25AM -0600, Rob Clark wrote:
> > On Mon, Dec 24, 2012 at 11:09 AM, Laurent Pinchart wrote:
> > > On the topic of discussions, would anyone be interested in a
> > > BoF/brainstorming/whatever session during the FOSDEM ?
> >
> > I will be at FOSDEM.. and from http://wiki.x.org/wiki/fosdem2013 it
> > looks like at least Daniel will be there. If enough others are, it
> > could be a good idea.
>
> Seconded. Jesse should be there, too, and from the Helsinki guys Ville and
> Andy should show up. Doesn't look like Jani will be able to make it. I think
> something on Sunday (to not clash with the X devroom) would be good.
>
> Should we apply for an offical BOF/Is there a process for tahat? Adding
> Luc in case he knows ...
From the event website it looks like there are free rooms on Sunday, it would
be good if we could secure one of them.
Are there other X/display related topics that need to be discussed on Sunday ?
How much time should we set aside ?
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCHv4 1/9] video: mmp display subsystem
From: Zhou Zhu @ 2013-01-08 9:22 UTC (permalink / raw)
To: linux-fbdev
Hi, Greg,
My patches have been there for several months and still have no
response. Would you please help me to check?
Thank you very much for your help!
On Mon, Dec 17, 2012 at 7:47 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Wed, Oct 24, 2012 at 2:56 PM, Zhou Zhu <zzhu3@marvell.com> wrote:
>> Added mmp display subsystem to support Marvell MMP display controllers.
>>
>> This subsystem contains 4 parts:
>> --fb folder
>> --core.c
>> --hw folder
>> --panel folder
>>
>> 1. fb folder contains implementation of fb.
>> fb get path and ovly from common interface and operates on these structures.
>>
>> 2. core.c provides common interface for a hardware abstraction.
>> Major parts of this interface are:
>> a) Path: path is a output device connected to a panel or HDMI TV.
>> Main operations of the path is set/get timing/output color.
>> fb operates output device through path structure.
>> b) Ovly: Ovly is a buffer shown on the path.
>> Ovly describes frame buffer and its source/destination size, offset, input
>> color, buffer address, z-order, and so on.
>> Each fb device maps to one ovly.
>>
>> 3. hw folder contains implementation of hardware operations defined by core.c.
>> It registers paths for fb use.
>>
>> 4. panel folder contains implementation of panels.
>> It's connected to path. Panel drivers would also regiester panels and linked
>> to path when probe.
>>
>> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
>> Signed-off-by: Lisa Du <cldu@marvell.com>
>> ---
>> drivers/video/Kconfig | 1 +
>> drivers/video/Makefile | 1 +
>> drivers/video/mmp/Kconfig | 5 +
>> drivers/video/mmp/Makefile | 1 +
>> drivers/video/mmp/core.c | 217 +++++++++++++++++++++++++++
>> include/video/mmp_disp.h | 351 ++++++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 576 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/video/mmp/Kconfig
>> create mode 100644 drivers/video/mmp/Makefile
>> create mode 100644 drivers/video/mmp/core.c
>> create mode 100644 include/video/mmp_disp.h
>>
>
> Hi Florian,
>
> This patch series have been sent for a long time. And there's no
> comments on this.
>
> I need this patch series on enabling LCD on pxa910 platform. Do you
> think it's OK to merge?
>
> Best Regards
> Haojian
--
Thanks,
-Zhou
^ permalink raw reply
* [PATCH 1/2] backlight: ld9040: Use devm_regulator_bulk_get API
From: Sachin Kamat @ 2013-01-08 9:31 UTC (permalink / raw)
To: linux-fbdev
devm_regulator_bulk_get is device managed and saves some cleanup
and exit code.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Cc: Donghwa Lee <dh09.lee@samsung.com>
---
drivers/video/backlight/ld9040.c | 11 +++--------
1 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/video/backlight/ld9040.c b/drivers/video/backlight/ld9040.c
index 24f2c9a..1b642f5 100644
--- a/drivers/video/backlight/ld9040.c
+++ b/drivers/video/backlight/ld9040.c
@@ -710,17 +710,15 @@ static int ld9040_probe(struct spi_device *spi)
mutex_init(&lcd->lock);
- ret = regulator_bulk_get(lcd->dev, ARRAY_SIZE(supplies), supplies);
+ ret = devm_regulator_bulk_get(lcd->dev, ARRAY_SIZE(supplies), supplies);
if (ret) {
dev_err(lcd->dev, "Failed to get regulators: %d\n", ret);
return ret;
}
ld = lcd_device_register("ld9040", &spi->dev, lcd, &ld9040_lcd_ops);
- if (IS_ERR(ld)) {
- ret = PTR_ERR(ld);
- goto out_free_regulator;
- }
+ if (IS_ERR(ld))
+ return PTR_ERR(ld);
lcd->ld = ld;
@@ -762,8 +760,6 @@ static int ld9040_probe(struct spi_device *spi)
out_unregister_lcd:
lcd_device_unregister(lcd->ld);
-out_free_regulator:
- regulator_bulk_free(ARRAY_SIZE(supplies), supplies);
return ret;
}
@@ -775,7 +771,6 @@ static int ld9040_remove(struct spi_device *spi)
ld9040_power(lcd, FB_BLANK_POWERDOWN);
backlight_device_unregister(lcd->bd);
lcd_device_unregister(lcd->ld);
- regulator_bulk_free(ARRAY_SIZE(supplies), supplies);
return 0;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/2] backlight: l4f00242t03: Use devm_regulator_get API
From: Sachin Kamat @ 2013-01-08 9:31 UTC (permalink / raw)
To: linux-fbdev
devm_regulator_get is device managed and saves some cleanup
and exit code.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
drivers/video/backlight/l4f00242t03.c | 23 +++++------------------
1 files changed, 5 insertions(+), 18 deletions(-)
diff --git a/drivers/video/backlight/l4f00242t03.c b/drivers/video/backlight/l4f00242t03.c
index 9bef9cf..9d18073 100644
--- a/drivers/video/backlight/l4f00242t03.c
+++ b/drivers/video/backlight/l4f00242t03.c
@@ -190,27 +190,24 @@ static int l4f00242t03_probe(struct spi_device *spi)
return ret;
}
- priv->io_reg = regulator_get(&spi->dev, "vdd");
+ priv->io_reg = devm_regulator_get(&spi->dev, "vdd");
if (IS_ERR(priv->io_reg)) {
dev_err(&spi->dev, "%s: Unable to get the IO regulator\n",
__func__);
return PTR_ERR(priv->io_reg);
}
- priv->core_reg = regulator_get(&spi->dev, "vcore");
+ priv->core_reg = devm_regulator_get(&spi->dev, "vcore");
if (IS_ERR(priv->core_reg)) {
- ret = PTR_ERR(priv->core_reg);
dev_err(&spi->dev, "%s: Unable to get the core regulator\n",
__func__);
- goto err1;
+ return PTR_ERR(priv->core_reg);
}
priv->ld = lcd_device_register("l4f00242t03",
&spi->dev, priv, &l4f_ops);
- if (IS_ERR(priv->ld)) {
- ret = PTR_ERR(priv->ld);
- goto err2;
- }
+ if (IS_ERR(priv->ld))
+ return PTR_ERR(priv->ld);
/* Init the LCD */
l4f00242t03_lcd_init(spi);
@@ -220,13 +217,6 @@ static int l4f00242t03_probe(struct spi_device *spi)
dev_info(&spi->dev, "Epson l4f00242t03 lcd probed.\n");
return 0;
-
-err2:
- regulator_put(priv->core_reg);
-err1:
- regulator_put(priv->io_reg);
-
- return ret;
}
static int l4f00242t03_remove(struct spi_device *spi)
@@ -238,9 +228,6 @@ static int l4f00242t03_remove(struct spi_device *spi)
spi_set_drvdata(spi, NULL);
- regulator_put(priv->io_reg);
- regulator_put(priv->core_reg);
-
return 0;
}
--
1.7.4.1
^ permalink raw reply related
* Re: [RFC v2 0/5] Common Display Framework
From: Marcus Lorentzon @ 2013-01-08 10:12 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Tomasz Figa, Thomas Petazzoni, linux-fbdev@vger.kernel.org,
Philipp Zabel, Tom Gall, Ragesh Radhakrishnan,
dri-devel@lists.freedesktop.org, Rob Clark, Kyungmin Park,
Tomi Valkeinen, sunil joshi, Benjamin Gaignard, Bryan Wu,
Maxime Ripard, Vikas Sajjan, Sumit Semwal, Sebastien Guiriec,
linux-media@vger.kernel.org
In-Reply-To: <3584709.mPLC5exzRY@avalon>
On 01/08/2013 09:18 AM, Laurent Pinchart wrote:
> On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:
>> > On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:
>>> > > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:
>>>> > > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:
>>>>> > > > > On 17 December 2012 20:55, Laurent Pinchart wrote:
>>>>>> > > > > > Hi Vikas,
>>>>>> > > > > >
>>>>>> > > > > > Sorry for the late reply. I now have more time to work on CDF, so
>>>>>> > > > > > delays should be much shorter.
>>>>>> > > > > >
>>>>>> > > > > > On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:
>>>>>>> > > > > > > Hi Laurent,
>>>>>>> > > > > > >
>>>>>>> > > > > > > I was thinking of porting CDF to samsung EXYNOS 5250 platform,
>>>>>>> > > > > > > what I found is that, the exynos display controller is MIPI DSI
>>>>>>> > > > > > > based controller.
>>>>>>> > > > > > >
>>>>>>> > > > > > > But if I look at CDF patches, it has only support for MIPI DBI
>>>>>>> > > > > > > based Display controller.
>>>>>>> > > > > > >
>>>>>>> > > > > > > So my question is, do we have any generic framework for MIPI DSI
>>>>>>> > > > > > > based display controller? basically I wanted to know, how to go
>>>>>>> > > > > > > about porting CDF for such kind of display controller.
>>>>>> > > > > >
>>>>>> > > > > > MIPI DSI support is not available yet. The only reason for that is
>>>>>> > > > > > that I don't have any MIPI DSI hardware to write and test the code
>>>>>> > > > > > with:-)
>>>>>> > > > > >
>>>>>> > > > > > The common display framework should definitely support MIPI DSI. I
>>>>>> > > > > > think the existing MIPI DBI code could be used as a base, so the
>>>>>> > > > > > implementation shouldn't be too high.
>>>>>> > > > > >
>>>>>> > > > > > Yeah, i was also thinking in similar lines, below is my though for
>>>>>> > > > > > MIPI DSI support in CDF.
>>>>> > > > >
>>>>> > > > > o MIPI DSI support as part of CDF framework will expose
>>>>> > > > > § mipi_dsi_register_device(mpi_device) (will be called mach-xxx-dt.c
>>>>> > > > > file )
>>>>> > > > > § mipi_dsi_register_driver(mipi_driver, bus ops) (will be called
>>>>> > > > > from platform specific init driver call )
>>>>> > > > > · bus ops will be
>>>>> > > > > o read data
>>>>> > > > > o write data
>>>>> > > > > o write command
>>>>> > > > > § MIPI DSI will be registered as bus_register()
>>>>> > > > >
>>>>> > > > > When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI)
>>>>> > > > > will initialize the MIPI DSI HW IP.
>>>>> > > > >
>>>>> > > > > This probe will also parse the DT file for MIPI DSI based panel, add
>>>>> > > > > the panel device (device_add() ) to kernel and register the display
>>>>> > > > > entity with its control and video ops with CDF.
>>>>> > > > >
>>>>> > > > > I can give this a try.
>>>> > > >
>>>> > > > I am currently in progress of reworking Exynos MIPI DSIM code and
>>>> > > > s6e8ax0 LCD driver to use the v2 RFC of Common Display Framework. I
>>>> > > > have most of the work done, I have just to solve several remaining
>>>> > > > problems.
>>> > >
>>> > > Do you already have code that you can publish ? I'm particularly
>>> > > interested (and I think Tomi Valkeinen would be as well) in looking at
>>> > > the DSI operations you expose to DSI sinks (panels, transceivers, ...).
>> >
>> > Well, I'm afraid this might be little below your expectations, but here's
>> > an initial RFC of the part defining just the DSI bus. I need a bit more
>> > time for patches for Exynos MIPI DSI master and s6e8ax0 LCD.
> No worries. I was particularly interested in the DSI operations you needed to
> export, they seem pretty simple. Thank you for sharing the code.
>
FYI,
here is STE "DSI API":
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD#l361
But it is not perfect. After a couple of products we realized that most
panel drivers want an easy way to send a bunch of init commands in one
go. So I think it should be an op for sending an array of commands at
once. Something like
struct dsi_cmd {
enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
u8 cmd;
int dataLen;
u8 *data;
}
struct dsi_ops {
int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
...
}
The rest of "DSI write API" could be made helpers on top of this one op.
This grouping also allows driver to describe intent to send a bunch of
commands together which might be of interest with mode set (if you need
to synchronize a bunch of commands with a mode set, like setting smart
panel rotation in synch with new framebuffer in dsi video mode).
I also looked at the video source in Tomi's git tree
(http://gitorious.org/linux-omap-dss2/linux/blobs/work/dss-dev-model-cdf/include/video/display.h).
I think I would prefer a single "setup" op taking a "struct dsi_config"
as argument. Then each DSI formatter/encoder driver could decide best
way to set that up. We have something similar at
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD#l118
And I think I still prefer the dsi_bus in favor of the abstract video
source. It just looks like a home made bus with bus-ops ... can't you do
something similar using the normal driver framework? enable/disable
looks like suspend/resume, register/unregister_vid_src is like
bus_(un)register_device, ... the video source anyway seems unattached to
the panel stuff with the find_video_source call.
/BR
/Marcus
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Rob Clark @ 2013-01-08 16:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Dave Airlie, Thomas Petazzoni, Linux Fbdev development list,
Philipp Zabel, Tom Gall, Ragesh Radhakrishnan,
dri-devel@lists.freedesktop.org, Kyungmin Park, Tomi Valkeinen,
Benjamin Gaignard, Maxime Ripard, Vikas Sajjan, Sumit Semwal,
Sebastien Guiriec, linux-media@vger.kernel.org
In-Reply-To: <1563062.kh1jqNm1kH@avalon>
On Tue, Jan 8, 2013 at 2:25 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Rob,
>
> On Thursday 27 December 2012 09:54:55 Rob Clark wrote:
>> What I've done to avoid that so far is that the master device registers the
>> drivers for it's output sub-devices before registering it's own device.
>
> I'm not sure to follow you here. The master device doesn't register anything,
> do you mean the master device driver ? If so, how does the master device
> driver register its own device ? Devices are not registered by their driver.
sorry, that should have read "master driver registers drivers for it's
sub-devices.."
BR,
-R
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Tomasz Figa @ 2013-01-08 16:36 UTC (permalink / raw)
To: Marcus Lorentzon
Cc: Laurent Pinchart, Thomas Petazzoni, linux-fbdev@vger.kernel.org,
Philipp Zabel, Tom Gall, Ragesh Radhakrishnan,
dri-devel@lists.freedesktop.org, Rob Clark, Kyungmin Park,
Tomi Valkeinen, sunil joshi, Benjamin Gaignard, Bryan Wu,
Maxime Ripard, Vikas Sajjan, Sumit Semwal, Sebastien Guiriec,
linux-media@vger.kernel.org
In-Reply-To: <50EBF10A.7080906@stericsson.com>
On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:
> On 01/08/2013 09:18 AM, Laurent Pinchart wrote:
> > On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:
> >> > On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:
> >>> > > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:
> >>>> > > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan
wrote:
> >>>>> > > > > On 17 December 2012 20:55, Laurent Pinchart wrote:
> >>>>>> > > > > > Hi Vikas,
> >>>>>> > > > > >
> >>>>>> > > > > > Sorry for the late reply. I now have more time to
> >>>>>> > > > > > work on CDF, so
> >>>>>> > > > > > delays should be much shorter.
> >>>>>> > > > > >
> >>>>>> > > > > > On Thursday 06 December 2012 10:51:15 Vikas Sajjan
wrote:
> >>>>>>> > > > > > > Hi Laurent,
> >>>>>>> > > > > > >
> >>>>>>> > > > > > > I was thinking of porting CDF to samsung
> >>>>>>> > > > > > > EXYNOS 5250 platform,
> >>>>>>> > > > > > > what I found is that, the exynos display
> >>>>>>> > > > > > > controller is MIPI DSI
> >>>>>>> > > > > > > based controller.
> >>>>>>> > > > > > >
> >>>>>>> > > > > > > But if I look at CDF patches, it has only
> >>>>>>> > > > > > > support for MIPI DBI
> >>>>>>> > > > > > > based Display controller.
> >>>>>>> > > > > > >
> >>>>>>> > > > > > > So my question is, do we have any generic
> >>>>>>> > > > > > > framework for MIPI DSI
> >>>>>>> > > > > > > based display controller? basically I wanted
> >>>>>>> > > > > > > to know, how to go
> >>>>>>> > > > > > > about porting CDF for such kind of display
> >>>>>>> > > > > > > controller.
> >>>>>> > > > > >
> >>>>>> > > > > > MIPI DSI support is not available yet. The only
> >>>>>> > > > > > reason for that is
> >>>>>> > > > > > that I don't have any MIPI DSI hardware to write
> >>>>>> > > > > > and test the code
> >>>>>> > > > > > with:-)
> >>>>>> > > > > >
> >>>>>> > > > > > The common display framework should definitely
> >>>>>> > > > > > support MIPI DSI. I
> >>>>>> > > > > > think the existing MIPI DBI code could be used as
> >>>>>> > > > > > a base, so the
> >>>>>> > > > > > implementation shouldn't be too high.
> >>>>>> > > > > >
> >>>>>> > > > > > Yeah, i was also thinking in similar lines, below
> >>>>>> > > > > > is my though for
> >>>>>> > > > > > MIPI DSI support in CDF.
> >>>>> > > > >
> >>>>> > > > > o MIPI DSI support as part of CDF framework will
> >>>>> > > > > expose
> >>>>> > > > > § mipi_dsi_register_device(mpi_device) (will be
> >>>>> > > > > called mach-xxx-dt.c
> >>>>> > > > > file )
> >>>>> > > > > § mipi_dsi_register_driver(mipi_driver, bus ops)
> >>>>> > > > > (will be called
> >>>>> > > > > from platform specific init driver call )
> >>>>> > > > > · bus ops will be
> >>>>> > > > > o read data
> >>>>> > > > > o write data
> >>>>> > > > > o write command
> >>>>> > > > > § MIPI DSI will be registered as bus_register()
> >>>>> > > > >
> >>>>> > > > > When MIPI DSI probe is called, it (e.g., Exynos or
> >>>>> > > > > OMAP MIPI DSI)
> >>>>> > > > > will initialize the MIPI DSI HW IP.
> >>>>> > > > >
> >>>>> > > > > This probe will also parse the DT file for MIPI DSI
> >>>>> > > > > based panel, add
> >>>>> > > > > the panel device (device_add() ) to kernel and
> >>>>> > > > > register the display
> >>>>> > > > > entity with its control and video ops with CDF.
> >>>>> > > > >
> >>>>> > > > > I can give this a try.
> >>>> > > >
> >>>> > > > I am currently in progress of reworking Exynos MIPI DSIM
> >>>> > > > code and
> >>>> > > > s6e8ax0 LCD driver to use the v2 RFC of Common Display
> >>>> > > > Framework. I
> >>>> > > > have most of the work done, I have just to solve several
> >>>> > > > remaining
> >>>> > > > problems.
> >>> > >
> >>> > > Do you already have code that you can publish ? I'm
> >>> > > particularly
> >>> > > interested (and I think Tomi Valkeinen would be as well) in
> >>> > > looking at
> >>> > > the DSI operations you expose to DSI sinks (panels,
> >>> > > transceivers, ...).
> >> >
> >> > Well, I'm afraid this might be little below your expectations, but
> >> > here's an initial RFC of the part defining just the DSI bus. I
> >> > need a bit more time for patches for Exynos MIPI DSI master and
> >> > s6e8ax0 LCD.
> >
> > No worries. I was particularly interested in the DSI operations you
> > needed to export, they seem pretty simple. Thank you for sharing the
> > code.
> FYI,
> here is STE "DSI API":
> http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
> =include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
> #l361
>
> But it is not perfect. After a couple of products we realized that most
> panel drivers want an easy way to send a bunch of init commands in one
> go. So I think it should be an op for sending an array of commands at
> once. Something like
>
> struct dsi_cmd {
> enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
> u8 cmd;
> int dataLen;
> u8 *data;
> }
> struct dsi_ops {
> int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
> ...
> }
Yes, this should be flexible enough to cover most of (or even whole) DSI
specification.
However I'm not sure whether the dsi_write name would be appropriate,
since DSI packet types include also read and special transactions. So,
according to DSI terminology, maybe dsi_transaction would be better?
>
> The rest of "DSI write API" could be made helpers on top of this one op.
> This grouping also allows driver to describe intent to send a bunch of
> commands together which might be of interest with mode set (if you need
> to synchronize a bunch of commands with a mode set, like setting smart
> panel rotation in synch with new framebuffer in dsi video mode).
>
> I also looked at the video source in Tomi's git tree
> (http://gitorious.org/linux-omap-dss2/linux/blobs/work/dss-dev-model-cdf
> /include/video/display.h). I think I would prefer a single "setup" op
> taking a "struct dsi_config" as argument. Then each DSI
> formatter/encoder driver could decide best way to set that up. We have
> something similar at
> http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
> =include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
> #l118
Yes, this would be definitely better, because such configuration changes
usually come together (i.e. display connected, which needs complete
reconfiguration of all parameters).
> And I think I still prefer the dsi_bus in favor of the abstract video
> source. It just looks like a home made bus with bus-ops ... can't you do
> something similar using the normal driver framework? enable/disable
> looks like suspend/resume, register/unregister_vid_src is like
> bus_(un)register_device, ... the video source anyway seems unattached
> to the panel stuff with the find_video_source call.
DSI needs specific power management. It's necessary to power up the panel
first to make it wait for Tinit event and then enable DSI master to
trigger such event. Only then rest of panel initialization can be
completed.
Also, as discussed in previous posts, some panels might use DSI only for
video data and another interface (I2C, SPI) for control data. In such case
it would be impossible to represent such device in a reasonable way using
current driver model.
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Marcus Lorentzon @ 2013-01-08 17:08 UTC (permalink / raw)
To: Tomasz Figa
Cc: Laurent Pinchart, Thomas Petazzoni, linux-fbdev@vger.kernel.org,
Philipp Zabel, Tom Gall, Ragesh Radhakrishnan,
dri-devel@lists.freedesktop.org, Rob Clark, Kyungmin Park,
Tomi Valkeinen, sunil joshi, Benjamin Gaignard, Bryan Wu,
Maxime Ripard, Vikas Sajjan, Sumit Semwal, Sebastien Guiriec,
linux-media@vger.kernel.org
In-Reply-To: <1987992.4TmVjQaiLj@amdc1227>
On 01/08/2013 05:36 PM, Tomasz Figa wrote:
> On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:
>> On 01/08/2013 09:18 AM, Laurent Pinchart wrote:
>>> On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:
>>>>> On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:
>>>>>> > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:
>>>>>>> > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan
> wrote:
>>>>>>>> > > > On 17 December 2012 20:55, Laurent Pinchart wrote:
>>>>>>>>> > > > > Hi Vikas,
>>>>>>>>> > > > >
>>>>>>>>> > > > > Sorry for the late reply. I now have more time to
>>>>>>>>> > > > > work on CDF, so
>>>>>>>>> > > > > delays should be much shorter.
>>>>>>>>> > > > >
>>>>>>>>> > > > > On Thursday 06 December 2012 10:51:15 Vikas Sajjan
> wrote:
>>>>>>>>>> > > > > > Hi Laurent,
>>>>>>>>>> > > > > >
>>>>>>>>>> > > > > > I was thinking of porting CDF to samsung
>>>>>>>>>> > > > > > EXYNOS 5250 platform,
>>>>>>>>>> > > > > > what I found is that, the exynos display
>>>>>>>>>> > > > > > controller is MIPI DSI
>>>>>>>>>> > > > > > based controller.
>>>>>>>>>> > > > > >
>>>>>>>>>> > > > > > But if I look at CDF patches, it has only
>>>>>>>>>> > > > > > support for MIPI DBI
>>>>>>>>>> > > > > > based Display controller.
>>>>>>>>>> > > > > >
>>>>>>>>>> > > > > > So my question is, do we have any generic
>>>>>>>>>> > > > > > framework for MIPI DSI
>>>>>>>>>> > > > > > based display controller? basically I wanted
>>>>>>>>>> > > > > > to know, how to go
>>>>>>>>>> > > > > > about porting CDF for such kind of display
>>>>>>>>>> > > > > > controller.
>>>>>>>>> > > > >
>>>>>>>>> > > > > MIPI DSI support is not available yet. The only
>>>>>>>>> > > > > reason for that is
>>>>>>>>> > > > > that I don't have any MIPI DSI hardware to write
>>>>>>>>> > > > > and test the code
>>>>>>>>> > > > > with:-)
>>>>>>>>> > > > >
>>>>>>>>> > > > > The common display framework should definitely
>>>>>>>>> > > > > support MIPI DSI. I
>>>>>>>>> > > > > think the existing MIPI DBI code could be used as
>>>>>>>>> > > > > a base, so the
>>>>>>>>> > > > > implementation shouldn't be too high.
>>>>>>>>> > > > >
>>>>>>>>> > > > > Yeah, i was also thinking in similar lines, below
>>>>>>>>> > > > > is my though for
>>>>>>>>> > > > > MIPI DSI support in CDF.
>>>>>>>> > > >
>>>>>>>> > > > o MIPI DSI support as part of CDF framework will
>>>>>>>> > > > expose
>>>>>>>> > > > § mipi_dsi_register_device(mpi_device) (will be
>>>>>>>> > > > called mach-xxx-dt.c
>>>>>>>> > > > file )
>>>>>>>> > > > § mipi_dsi_register_driver(mipi_driver, bus ops)
>>>>>>>> > > > (will be called
>>>>>>>> > > > from platform specific init driver call )
>>>>>>>> > > > · bus ops will be
>>>>>>>> > > > o read data
>>>>>>>> > > > o write data
>>>>>>>> > > > o write command
>>>>>>>> > > > § MIPI DSI will be registered as bus_register()
>>>>>>>> > > >
>>>>>>>> > > > When MIPI DSI probe is called, it (e.g., Exynos or
>>>>>>>> > > > OMAP MIPI DSI)
>>>>>>>> > > > will initialize the MIPI DSI HW IP.
>>>>>>>> > > >
>>>>>>>> > > > This probe will also parse the DT file for MIPI DSI
>>>>>>>> > > > based panel, add
>>>>>>>> > > > the panel device (device_add() ) to kernel and
>>>>>>>> > > > register the display
>>>>>>>> > > > entity with its control and video ops with CDF.
>>>>>>>> > > >
>>>>>>>> > > > I can give this a try.
>>>>>>> > >
>>>>>>> > > I am currently in progress of reworking Exynos MIPI DSIM
>>>>>>> > > code and
>>>>>>> > > s6e8ax0 LCD driver to use the v2 RFC of Common Display
>>>>>>> > > Framework. I
>>>>>>> > > have most of the work done, I have just to solve several
>>>>>>> > > remaining
>>>>>>> > > problems.
>>>>>> >
>>>>>> > Do you already have code that you can publish ? I'm
>>>>>> > particularly
>>>>>> > interested (and I think Tomi Valkeinen would be as well) in
>>>>>> > looking at
>>>>>> > the DSI operations you expose to DSI sinks (panels,
>>>>>> > transceivers, ...).
>>>>>
>>>>> Well, I'm afraid this might be little below your expectations, but
>>>>> here's an initial RFC of the part defining just the DSI bus. I
>>>>> need a bit more time for patches for Exynos MIPI DSI master and
>>>>> s6e8ax0 LCD.
>>> No worries. I was particularly interested in the DSI operations you
>>> needed to export, they seem pretty simple. Thank you for sharing the
>>> code.
>> FYI,
>> here is STE "DSI API":
>> http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
>> =include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
>> #l361
>>
>> But it is not perfect. After a couple of products we realized that most
>> panel drivers want an easy way to send a bunch of init commands in one
>> go. So I think it should be an op for sending an array of commands at
>> once. Something like
>>
>> struct dsi_cmd {
>> enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
>> u8 cmd;
>> int dataLen;
>> u8 *data;
>> }
>> struct dsi_ops {
>> int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
>> ...
>> }
> Yes, this should be flexible enough to cover most of (or even whole) DSI
> specification.
>
> However I'm not sure whether the dsi_write name would be appropriate,
> since DSI packet types include also read and special transactions. So,
> according to DSI terminology, maybe dsi_transaction would be better?
I think read should still be separate. At least on my HW read and write
are quite different. But all "transactions" are very much the same in HW
setup. The ... was dsi_write etc ;) Like set_max_packet_size should
maybe be an ops. Since only the implementer of the "video source" will
know what the max read return packet size for that DSI IP is. The panels
don't know that. Maybe another ops to retrieve some info about the caps
of the video source would help that. Then a helper could call that and
then the dsi_write one.
>> And I think I still prefer the dsi_bus in favor of the abstract video
>> source. It just looks like a home made bus with bus-ops ... can't you do
>> something similar using the normal driver framework? enable/disable
>> looks like suspend/resume, register/unregister_vid_src is like
>> bus_(un)register_device, ... the video source anyway seems unattached
>> to the panel stuff with the find_video_source call.
> DSI needs specific power management. It's necessary to power up the panel
> first to make it wait for Tinit event and then enable DSI master to
> trigger such event. Only then rest of panel initialization can be
> completed.
I know, we have a very complex sequence for our HDMI encoder which uses
sort of continuous DSI cmmand mode. And power/clock on sequences are
tricky to get right in our current "CDF" API (mcde_display). But I fail
to see how the current video source API is different from just using the
bus/device APIs.
>
> Also, as discussed in previous posts, some panels might use DSI only for
> video data and another interface (I2C, SPI) for control data. In such case
> it would be impossible to represent such device in a reasonable way using
> current driver model.
>
I understand that you need to get hold of both the control and data bus
device in the driver. (Toshiba DSI-LVDS bridge is a good example and
commonly used "encoder" that can use both DSI and I2C control
interface.) But the control bus you get from device probe, and I guess
you could call bus_find_device_by_name(dsi_bus, "mydev") and return the
"datadev" which will have access to dsi bus ops just as you call
find_video_source("mysource") to access the "databus" ops directly with
a logical device (display entity).
I'm not saying I would refuse to use video sources. I just think the two
models are so similar so it would be worth exploring how a device model
style API would look like and to compare against.
/BR
/Marcus
^ permalink raw reply
* [PATCH] backlight: add an AS3711 PMIC backlight driver
From: Guennadi Liakhovetski @ 2013-01-08 17:54 UTC (permalink / raw)
To: linux-fbdev
This is an initial commit of a backlight driver, using step-up DCDC power
supplies on AS3711 PMIC. Only one mode has actually been tested, several
further modes have been implemented "dry," but disabled to avoid accidental
hardware damage. Anyone wishing to use any of those modes will have to
modify the driver.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
Tested on sh73a0-based kzm9g board. As the commit message says, only one
mode has been tested and is enabled. That mode copies the sample code from
the manufacturer. Deviations from that code proved to be fatal for the
hardware...
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/as3711_bl.c | 379 +++++++++++++++++++++++++++++++++++
3 files changed, 387 insertions(+), 0 deletions(-)
create mode 100644 drivers/video/backlight/as3711_bl.c
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 765a945..6ef0ede 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -390,6 +390,13 @@ config BACKLIGHT_TPS65217
If you have a Texas Instruments TPS65217 say Y to enable the
backlight driver.
+config BACKLIGHT_AS3711
+ tristate "AS3711 Backlight"
+ depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
+ help
+ If you have an Austrian Microsystems AS3711 say Y to enable the
+ backlight driver.
+
endif # BACKLIGHT_CLASS_DEVICE
endif # BACKLIGHT_LCD_SUPPORT
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index e7ce729..d3e188f 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
+obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
new file mode 100644
index 0000000..c6bc65d
--- /dev/null
+++ b/drivers/video/backlight/as3711_bl.c
@@ -0,0 +1,379 @@
+/*
+ * AS3711 PMIC backlight driver, using DCDC Step Up Converters
+ *
+ * Copyright (C) 2012 Renesas Electronics Corporation
+ * Author: Guennadi Liakhovetski, <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the version 2 of the GNU General Public License as
+ * published by the Free Software Foundation
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/kernel.h>
+#include <linux/mfd/as3711.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+enum as3711_bl_type {
+ AS3711_BL_SU1,
+ AS3711_BL_SU2,
+};
+
+struct as3711_bl_data {
+ bool powered;
+ const char *fb_name;
+ struct device *fb_dev;
+ enum as3711_bl_type type;
+ int brightness;
+ struct backlight_device *bl;
+};
+
+struct as3711_bl_supply {
+ struct as3711_bl_data su1;
+ struct as3711_bl_data su2;
+ const struct as3711_bl_pdata *pdata;
+ struct as3711 *as3711;
+};
+
+static struct as3711_bl_supply *to_supply(struct as3711_bl_data *su)
+{
+ switch (su->type) {
+ case AS3711_BL_SU1:
+ return container_of(su, struct as3711_bl_supply, su1);
+ case AS3711_BL_SU2:
+ return container_of(su, struct as3711_bl_supply, su2);
+ }
+ return NULL;
+}
+
+static int as3711_set_brightness_auto_i(struct as3711_bl_data *data,
+ unsigned int brightness)
+{
+ struct as3711_bl_supply *supply = to_supply(data);
+ struct as3711 *as3711 = supply->as3711;
+ const struct as3711_bl_pdata *pdata = supply->pdata;
+ int ret = 0;
+
+ /* Only all equal current values are supported */
+ if (pdata->su2_auto_curr1)
+ ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
+ brightness);
+ if (!ret && pdata->su2_auto_curr2)
+ ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
+ brightness);
+ if (!ret && pdata->su2_auto_curr3)
+ ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
+ brightness);
+
+ return ret;
+}
+
+static int as3711_set_brightness_v(struct as3711 *as3711,
+ unsigned int brightness,
+ unsigned int reg)
+{
+ if (brightness > 31)
+ return -EINVAL;
+
+ return regmap_update_bits(as3711->regmap, reg, 0xf0,
+ brightness << 4);
+}
+
+static int as3711_bl_su2_reset(struct as3711_bl_supply *supply)
+{
+ struct as3711 *as3711 = supply->as3711;
+ int ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_5,
+ 3, supply->pdata->su2_fbprot);
+ if (!ret)
+ ret = regmap_update_bits(as3711->regmap,
+ AS3711_STEPUP_CONTROL_2, 1, 0);
+ if (!ret)
+ ret = regmap_update_bits(as3711->regmap,
+ AS3711_STEPUP_CONTROL_2, 1, 1);
+ return ret;
+}
+
+/*
+ * Someone with less fragile or less expensive hardware could try to simplify
+ * the brightness adjustment procedure.
+ */
+static int as3711_bl_update_status(struct backlight_device *bl)
+{
+ struct as3711_bl_data *data = bl_get_data(bl);
+ struct as3711_bl_supply *supply = to_supply(data);
+ struct as3711 *as3711 = supply->as3711;
+ int brightness = bl->props.brightness;
+ int ret = 0;
+
+ dev_dbg(&bl->dev, "%s(): brightness %u, pwr %x, blank %x, state %x\n",
+ __func__, bl->props.brightness, bl->props.power,
+ bl->props.fb_blank, bl->props.state);
+
+ if (bl->props.power != FB_BLANK_UNBLANK ||
+ bl->props.fb_blank != FB_BLANK_UNBLANK ||
+ bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
+ brightness = 0;
+
+ if (data->type = AS3711_BL_SU1) {
+ ret = as3711_set_brightness_v(as3711, brightness,
+ AS3711_STEPUP_CONTROL_1);
+ } else {
+ const struct as3711_bl_pdata *pdata = supply->pdata;
+
+ switch (pdata->su2_feedback) {
+ case AS3711_SU2_VOLTAGE:
+ ret = as3711_set_brightness_v(as3711, brightness,
+ AS3711_STEPUP_CONTROL_2);
+ break;
+ case AS3711_SU2_CURR_AUTO:
+ ret = as3711_set_brightness_auto_i(data, brightness / 4);
+ if (ret < 0)
+ return ret;
+ if (brightness) {
+ ret = as3711_bl_su2_reset(supply);
+ if (ret < 0)
+ return ret;
+ udelay(500);
+ ret = as3711_set_brightness_auto_i(data, brightness);
+ } else {
+ ret = regmap_update_bits(as3711->regmap,
+ AS3711_STEPUP_CONTROL_2, 1, 0);
+ }
+ break;
+ /* Manual one current feedback pin below */
+ case AS3711_SU2_CURR1:
+ ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
+ brightness);
+ break;
+ case AS3711_SU2_CURR2:
+ ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
+ brightness);
+ break;
+ case AS3711_SU2_CURR3:
+ ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
+ brightness);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ }
+ if (!ret)
+ data->brightness = brightness;
+
+ return ret;
+}
+
+static int as3711_bl_get_brightness(struct backlight_device *bl)
+{
+ struct as3711_bl_data *data = bl_get_data(bl);
+
+ return data->brightness;
+}
+
+static const struct backlight_ops as3711_bl_ops = {
+ .update_status = as3711_bl_update_status,
+ .get_brightness = as3711_bl_get_brightness,
+};
+
+static int as3711_bl_init_su2(struct as3711_bl_supply *supply)
+{
+ struct as3711 *as3711 = supply->as3711;
+ const struct as3711_bl_pdata *pdata = supply->pdata;
+ u8 ctl = 0;
+ int ret;
+
+ dev_dbg(as3711->dev, "%s(): use %u\n", __func__, pdata->su2_feedback);
+
+ /* Turn SU2 off */
+ ret = regmap_write(as3711->regmap, AS3711_STEPUP_CONTROL_2, 0);
+ if (ret < 0)
+ return ret;
+
+ switch (pdata->su2_feedback) {
+ case AS3711_SU2_VOLTAGE:
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 0);
+ break;
+ case AS3711_SU2_CURR1:
+ ctl = 1;
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 1);
+ break;
+ case AS3711_SU2_CURR2:
+ ctl = 4;
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 2);
+ break;
+ case AS3711_SU2_CURR3:
+ ctl = 0x10;
+ ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 3);
+ break;
+ case AS3711_SU2_CURR_AUTO:
+ if (pdata->su2_auto_curr1)
+ ctl = 2;
+ if (pdata->su2_auto_curr2)
+ ctl |= 8;
+ if (pdata->su2_auto_curr3)
+ ctl |= 0x20;
+ ret = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (!ret)
+ ret = regmap_write(as3711->regmap, AS3711_CURR_CONTROL, ctl);
+
+ return ret;
+}
+
+static int as3711_bl_register(struct platform_device *pdev,
+ unsigned int max_brightness, struct as3711_bl_data *su)
+{
+ struct backlight_properties props = {.type = BACKLIGHT_RAW,};
+ struct backlight_device *bl;
+
+ /* max tuning I = 31uA for voltage- and 38250uA for current-feedback */
+ props.max_brightness = max_brightness;
+
+ bl = backlight_device_register(su->type = AS3711_BL_SU1 ?
+ "as3711-su1" : "as3711-su2",
+ &pdev->dev, su,
+ &as3711_bl_ops, &props);
+ if (IS_ERR(bl)) {
+ dev_err(&pdev->dev, "failed to register backlight\n");
+ return PTR_ERR(bl);
+ }
+
+ bl->props.brightness = props.max_brightness;
+
+ backlight_update_status(bl);
+
+ su->bl = bl;
+
+ return 0;
+}
+
+static int as3711_backlight_probe(struct platform_device *pdev)
+{
+ struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
+ struct as3711 *as3711 = dev_get_drvdata(pdev->dev.parent);
+ struct as3711_bl_supply *supply;
+ struct as3711_bl_data *su;
+ unsigned int max_brightness;
+ int ret;
+
+ if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
+ dev_err(&pdev->dev, "No platform data, exiting...\n");
+ return -ENODEV;
+ }
+
+ /*
+ * Due to possible hardware damage I chose to block all modes,
+ * unsupported on my hardware. Anyone, wishing to use any of those modes
+ * will have to first review the code, then activate and test it.
+ */
+ if (pdata->su1_fb ||
+ pdata->su2_fbprot != AS3711_SU2_GPIO4 ||
+ pdata->su2_feedback != AS3711_SU2_CURR_AUTO) {
+ dev_warn(&pdev->dev,
+ "Attention! An untested mode has been chosen!\n"
+ "Please, review the code, enable, test, and report success:-)\n");
+ return -EINVAL;
+ }
+
+ supply = devm_kzalloc(&pdev->dev, sizeof(*supply), GFP_KERNEL);
+ if (!supply)
+ return -ENOMEM;
+
+ supply->as3711 = as3711;
+ supply->pdata = pdata;
+
+ if (pdata->su1_fb) {
+ su = &supply->su1;
+ su->fb_name = pdata->su1_fb;
+ su->type = AS3711_BL_SU1;
+
+ max_brightness = min(pdata->su1_max_uA, 31);
+ ret = as3711_bl_register(pdev, max_brightness, su);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (pdata->su2_fb) {
+ su = &supply->su2;
+ su->fb_name = pdata->su2_fb;
+ su->type = AS3711_BL_SU2;
+
+ switch (pdata->su2_fbprot) {
+ case AS3711_SU2_GPIO2:
+ case AS3711_SU2_GPIO3:
+ case AS3711_SU2_GPIO4:
+ case AS3711_SU2_LX_SD4:
+ break;
+ default:
+ ret = -EINVAL;
+ goto esu2;
+ }
+
+ switch (pdata->su2_feedback) {
+ case AS3711_SU2_VOLTAGE:
+ max_brightness = min(pdata->su2_max_uA, 31);
+ break;
+ case AS3711_SU2_CURR1:
+ case AS3711_SU2_CURR2:
+ case AS3711_SU2_CURR3:
+ case AS3711_SU2_CURR_AUTO:
+ max_brightness = min(pdata->su2_max_uA / 150, 255);
+ break;
+ default:
+ ret = -EINVAL;
+ goto esu2;
+ }
+
+ ret = as3711_bl_init_su2(supply);
+ if (ret < 0)
+ return ret;
+
+ ret = as3711_bl_register(pdev, max_brightness, su);
+ if (ret < 0)
+ goto esu2;
+ }
+
+ platform_set_drvdata(pdev, supply);
+
+ return 0;
+
+esu2:
+ backlight_device_unregister(supply->su1.bl);
+ return ret;
+}
+
+static int as3711_backlight_remove(struct platform_device *pdev)
+{
+ struct as3711_bl_supply *supply = platform_get_drvdata(pdev);
+
+ backlight_device_unregister(supply->su1.bl);
+ backlight_device_unregister(supply->su2.bl);
+
+ return 0;
+}
+
+static struct platform_driver as3711_backlight_driver = {
+ .driver = {
+ .name = "as3711-backlight",
+ .owner = THIS_MODULE,
+ },
+ .probe = as3711_backlight_probe,
+ .remove = as3711_backlight_remove,
+};
+
+module_platform_driver(as3711_backlight_driver);
+
+MODULE_DESCRIPTION("Backlight Driver for AS3711 PMICs");
+MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:as3711-backlight");
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH] video: exynos_mipi_dsi: Use devm_* APIs
From: Sachin Kamat @ 2013-01-09 4:56 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1357196444-8077-1-git-send-email-sachin.kamat@linaro.org>
Hi Donghwa,
Any comments on this patch?
On 3 January 2013 12:30, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> devm_* APIs are device managed and make exit and cleanup code simpler.
> While at it also remove some unused labels and fix an error path.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
> Compile tested using the latest linux-next tree.
> This patch should be applied on top of the following patch:
> http://www.spinics.net/lists/linux-fbdev/msg09303.html
> ---
> drivers/video/exynos/exynos_mipi_dsi.c | 68 ++++++++------------------------
> include/video/exynos_mipi_dsim.h | 1 -
> 2 files changed, 17 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
> index f623dfc..fac7df6 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
> @@ -338,7 +338,8 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
> struct mipi_dsim_ddi *dsim_ddi;
> int ret = -EINVAL;
>
> - dsim = kzalloc(sizeof(struct mipi_dsim_device), GFP_KERNEL);
> + dsim = devm_kzalloc(&pdev->dev, sizeof(struct mipi_dsim_device),
> + GFP_KERNEL);
> if (!dsim) {
> dev_err(&pdev->dev, "failed to allocate dsim object.\n");
> return -ENOMEM;
> @@ -352,13 +353,13 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
> dsim_pd = (struct mipi_dsim_platform_data *)dsim->pd;
> if (dsim_pd = NULL) {
> dev_err(&pdev->dev, "failed to get platform data for dsim.\n");
> - goto err_clock_get;
> + return -EINVAL;
> }
> /* get mipi_dsim_config. */
> dsim_config = dsim_pd->dsim_config;
> if (dsim_config = NULL) {
> dev_err(&pdev->dev, "failed to get dsim config data.\n");
> - goto err_clock_get;
> + return -EINVAL;
> }
>
> dsim->dsim_config = dsim_config;
> @@ -366,41 +367,28 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
>
> mutex_init(&dsim->lock);
>
> - ret = regulator_bulk_get(&pdev->dev, ARRAY_SIZE(supplies), supplies);
> + ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(supplies),
> + supplies);
> if (ret) {
> dev_err(&pdev->dev, "Failed to get regulators: %d\n", ret);
> - goto err_clock_get;
> + return ret;
> }
>
> - dsim->clock = clk_get(&pdev->dev, "dsim0");
> + dsim->clock = devm_clk_get(&pdev->dev, "dsim0");
> if (IS_ERR(dsim->clock)) {
> dev_err(&pdev->dev, "failed to get dsim clock source\n");
> - ret = -ENODEV;
> - goto err_clock_get;
> + return -ENODEV;
> }
>
> clk_enable(dsim->clock);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&pdev->dev, "failed to get io memory region\n");
> - ret = -ENODEV;
> - goto err_platform_get;
> - }
> -
> - dsim->res = request_mem_region(res->start, resource_size(res),
> - dev_name(&pdev->dev));
> - if (!dsim->res) {
> - dev_err(&pdev->dev, "failed to request io memory region\n");
> - ret = -ENOMEM;
> - goto err_mem_region;
> - }
>
> - dsim->reg_base = ioremap(res->start, resource_size(res));
> + dsim->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> if (!dsim->reg_base) {
> dev_err(&pdev->dev, "failed to remap io region\n");
> ret = -ENOMEM;
> - goto err_ioremap;
> + goto error;
> }
>
> mutex_init(&dsim->lock);
> @@ -410,26 +398,27 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
> if (!dsim_ddi) {
> dev_err(&pdev->dev, "mipi_dsim_ddi object not found.\n");
> ret = -EINVAL;
> - goto err_bind;
> + goto error;
> }
>
> dsim->irq = platform_get_irq(pdev, 0);
> if (IS_ERR_VALUE(dsim->irq)) {
> dev_err(&pdev->dev, "failed to request dsim irq resource\n");
> ret = -EINVAL;
> - goto err_platform_get_irq;
> + goto error;
> }
>
> init_completion(&dsim_wr_comp);
> init_completion(&dsim_rd_comp);
> platform_set_drvdata(pdev, dsim);
>
> - ret = request_irq(dsim->irq, exynos_mipi_dsi_interrupt_handler,
> + ret = devm_request_irq(&pdev->dev, dsim->irq,
> + exynos_mipi_dsi_interrupt_handler,
> IRQF_SHARED, dev_name(&pdev->dev), dsim);
> if (ret != 0) {
> dev_err(&pdev->dev, "failed to request dsim irq\n");
> ret = -EINVAL;
> - goto err_bind;
> + goto error;
> }
>
> /* enable interrupts */
> @@ -471,22 +460,8 @@ done:
>
> return 0;
>
> -err_bind:
> - iounmap(dsim->reg_base);
> -
> -err_ioremap:
> - release_mem_region(dsim->res->start, resource_size(dsim->res));
> -
> -err_mem_region:
> - release_resource(dsim->res);
> -
> -err_platform_get:
> +error:
> clk_disable(dsim->clock);
> - clk_put(dsim->clock);
> -err_clock_get:
> - kfree(dsim);
> -
> -err_platform_get_irq:
> return ret;
> }
>
> @@ -496,13 +471,7 @@ static int exynos_mipi_dsi_remove(struct platform_device *pdev)
> struct mipi_dsim_ddi *dsim_ddi, *next;
> struct mipi_dsim_lcd_driver *dsim_lcd_drv;
>
> - iounmap(dsim->reg_base);
> -
> clk_disable(dsim->clock);
> - clk_put(dsim->clock);
> -
> - release_resource(dsim->res);
> - release_mem_region(dsim->res->start, resource_size(dsim->res));
>
> list_for_each_entry_safe(dsim_ddi, next, &dsim_ddi_list, list) {
> if (dsim_ddi) {
> @@ -518,9 +487,6 @@ static int exynos_mipi_dsi_remove(struct platform_device *pdev)
> }
> }
>
> - regulator_bulk_free(ARRAY_SIZE(supplies), supplies);
> - kfree(dsim);
> -
> return 0;
> }
>
> diff --git a/include/video/exynos_mipi_dsim.h b/include/video/exynos_mipi_dsim.h
> index 83ce5e6..89dc88a 100644
> --- a/include/video/exynos_mipi_dsim.h
> +++ b/include/video/exynos_mipi_dsim.h
> @@ -220,7 +220,6 @@ struct mipi_dsim_config {
> struct mipi_dsim_device {
> struct device *dev;
> int id;
> - struct resource *res;
> struct clk *clock;
> unsigned int irq;
> void __iomem *reg_base;
> --
> 1.7.4.1
>
--
With warm regards,
Sachin
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Rahul Sharma @ 2013-01-09 8:35 UTC (permalink / raw)
To: Rob Clark
Cc: Thomas Petazzoni, Linux Fbdev development list, Philipp Zabel,
Tom Gall, Ragesh Radhakrishnan, dri-devel@lists.freedesktop.org,
Kyungmin Park, Tomi Valkeinen, Laurent Pinchart,
Benjamin Gaignard, Maxime Ripard, Vikas Sajjan, Sumit Semwal,
Sebastien Guiriec, linux-media@vger.kernel.org
In-Reply-To: <CAN_cFWPyrvO5RAvMHhZgQySf_Y5N2pz64uMurvdG0d-4zDjPFQ@mail.gmail.com>
Hi Laurent,
CDF will also be helpful in supporting Panels with integrated
audio (HDMI/DP) if we can add audio related control operations to
display_entity_control_ops. Video controls will be called by crtc
in DRM/V4L and audio controls from Alsa.
Secondly, if I need to support get_modes operation in hdmi/dp
panel, I need to implement edid parser inside the panel driver. It
will be meaningful to add get_edid control operation for hdmi/dp.
regards,
Rahul Sharma.
On Tue, Jan 8, 2013 at 9:43 PM, Rob Clark <rob.clark@linaro.org> wrote:
> On Tue, Jan 8, 2013 at 2:25 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Rob,
>>
>> On Thursday 27 December 2012 09:54:55 Rob Clark wrote:
>>> What I've done to avoid that so far is that the master device registers the
>>> drivers for it's output sub-devices before registering it's own device.
>>
>> I'm not sure to follow you here. The master device doesn't register anything,
>> do you mean the master device driver ? If so, how does the master device
>> driver register its own device ? Devices are not registered by their driver.
>
> sorry, that should have read "master driver registers drivers for it's
> sub-devices.."
>
> BR,
> -R
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH] video: exynos_mipi_dsi: Use devm_* APIs
From: Donghwa Lee @ 2013-01-09 10:12 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1357196444-8077-1-git-send-email-sachin.kamat@linaro.org>
Hi, Sachin Kamat,
I'm sorry I didn't see your mail.
It seems to more simple to me.
Acked-by: Donghwa Lee <dh09.lee@samsung.com>
Thank you,
Donghwa Lee
On 3 Jan 2013 16:00, Sachin Kamat wrote:
> devm_* APIs are device managed and make exit and cleanup code simpler.
> While at it also remove some unused labels and fix an error path.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
> Compile tested using the latest linux-next tree.
> This patch should be applied on top of the following patch:
> http://www.spinics.net/lists/linux-fbdev/msg09303.html
> ---
> drivers/video/exynos/exynos_mipi_dsi.c | 68 ++++++++------------------------
> include/video/exynos_mipi_dsim.h | 1 -
> 2 files changed, 17 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
> index f623dfc..fac7df6 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
> @@ -338,7 +338,8 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
> struct mipi_dsim_ddi *dsim_ddi;
> int ret = -EINVAL;
>
> - dsim = kzalloc(sizeof(struct mipi_dsim_device), GFP_KERNEL);
> + dsim = devm_kzalloc(&pdev->dev, sizeof(struct mipi_dsim_device),
> + GFP_KERNEL);
> if (!dsim) {
> dev_err(&pdev->dev, "failed to allocate dsim object.\n");
> return -ENOMEM;
> @@ -352,13 +353,13 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
> dsim_pd = (struct mipi_dsim_platform_data *)dsim->pd;
> if (dsim_pd = NULL) {
> dev_err(&pdev->dev, "failed to get platform data for dsim.\n");
> - goto err_clock_get;
> + return -EINVAL;
> }
> /* get mipi_dsim_config. */
> dsim_config = dsim_pd->dsim_config;
> if (dsim_config = NULL) {
> dev_err(&pdev->dev, "failed to get dsim config data.\n");
> - goto err_clock_get;
> + return -EINVAL;
> }
>
> dsim->dsim_config = dsim_config;
> @@ -366,41 +367,28 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
>
> mutex_init(&dsim->lock);
>
> - ret = regulator_bulk_get(&pdev->dev, ARRAY_SIZE(supplies), supplies);
> + ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(supplies),
> + supplies);
> if (ret) {
> dev_err(&pdev->dev, "Failed to get regulators: %d\n", ret);
> - goto err_clock_get;
> + return ret;
> }
>
> - dsim->clock = clk_get(&pdev->dev, "dsim0");
> + dsim->clock = devm_clk_get(&pdev->dev, "dsim0");
> if (IS_ERR(dsim->clock)) {
> dev_err(&pdev->dev, "failed to get dsim clock source\n");
> - ret = -ENODEV;
> - goto err_clock_get;
> + return -ENODEV;
> }
>
> clk_enable(dsim->clock);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&pdev->dev, "failed to get io memory region\n");
> - ret = -ENODEV;
> - goto err_platform_get;
> - }
> -
> - dsim->res = request_mem_region(res->start, resource_size(res),
> - dev_name(&pdev->dev));
> - if (!dsim->res) {
> - dev_err(&pdev->dev, "failed to request io memory region\n");
> - ret = -ENOMEM;
> - goto err_mem_region;
> - }
>
> - dsim->reg_base = ioremap(res->start, resource_size(res));
> + dsim->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> if (!dsim->reg_base) {
> dev_err(&pdev->dev, "failed to remap io region\n");
> ret = -ENOMEM;
> - goto err_ioremap;
> + goto error;
> }
>
> mutex_init(&dsim->lock);
> @@ -410,26 +398,27 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
> if (!dsim_ddi) {
> dev_err(&pdev->dev, "mipi_dsim_ddi object not found.\n");
> ret = -EINVAL;
> - goto err_bind;
> + goto error;
> }
>
> dsim->irq = platform_get_irq(pdev, 0);
> if (IS_ERR_VALUE(dsim->irq)) {
> dev_err(&pdev->dev, "failed to request dsim irq resource\n");
> ret = -EINVAL;
> - goto err_platform_get_irq;
> + goto error;
> }
>
> init_completion(&dsim_wr_comp);
> init_completion(&dsim_rd_comp);
> platform_set_drvdata(pdev, dsim);
>
> - ret = request_irq(dsim->irq, exynos_mipi_dsi_interrupt_handler,
> + ret = devm_request_irq(&pdev->dev, dsim->irq,
> + exynos_mipi_dsi_interrupt_handler,
> IRQF_SHARED, dev_name(&pdev->dev), dsim);
> if (ret != 0) {
> dev_err(&pdev->dev, "failed to request dsim irq\n");
> ret = -EINVAL;
> - goto err_bind;
> + goto error;
> }
>
> /* enable interrupts */
> @@ -471,22 +460,8 @@ done:
>
> return 0;
>
> -err_bind:
> - iounmap(dsim->reg_base);
> -
> -err_ioremap:
> - release_mem_region(dsim->res->start, resource_size(dsim->res));
> -
> -err_mem_region:
> - release_resource(dsim->res);
> -
> -err_platform_get:
> +error:
> clk_disable(dsim->clock);
> - clk_put(dsim->clock);
> -err_clock_get:
> - kfree(dsim);
> -
> -err_platform_get_irq:
> return ret;
> }
>
> @@ -496,13 +471,7 @@ static int exynos_mipi_dsi_remove(struct platform_device *pdev)
> struct mipi_dsim_ddi *dsim_ddi, *next;
> struct mipi_dsim_lcd_driver *dsim_lcd_drv;
>
> - iounmap(dsim->reg_base);
> -
> clk_disable(dsim->clock);
> - clk_put(dsim->clock);
> -
> - release_resource(dsim->res);
> - release_mem_region(dsim->res->start, resource_size(dsim->res));
>
> list_for_each_entry_safe(dsim_ddi, next, &dsim_ddi_list, list) {
> if (dsim_ddi) {
> @@ -518,9 +487,6 @@ static int exynos_mipi_dsi_remove(struct platform_device *pdev)
> }
> }
>
> - regulator_bulk_free(ARRAY_SIZE(supplies), supplies);
> - kfree(dsim);
> -
> return 0;
> }
>
> diff --git a/include/video/exynos_mipi_dsim.h b/include/video/exynos_mipi_dsim.h
> index 83ce5e6..89dc88a 100644
> --- a/include/video/exynos_mipi_dsim.h
> +++ b/include/video/exynos_mipi_dsim.h
> @@ -220,7 +220,6 @@ struct mipi_dsim_config {
> struct mipi_dsim_device {
> struct device *dev;
> int id;
> - struct resource *res;
> struct clk *clock;
> unsigned int irq;
> void __iomem *reg_base;
^ permalink raw reply
* [PATCH v2] fb: udlfb: fix scheduling while atomic.
From: Alexander Holler @ 2013-01-09 13:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-fbdev, Bernie Thompson, Florian Tobias Schandinat, Alan Cox,
Steve Glendinning, Dave Airlie, Alexander Holler, stable
In-Reply-To: <50E9722D.2090602@ahsoftware.de>
The console functions are using spinlocks while calling fb-driver ops
but udlfb waits for a semaphore in many ops. This results in the BUG
"scheduling while atomic". One of those call flows is e.g.
vt_console_print() (spinlock printing_lock)
(...)
dlfb_ops_imageblit()
dlfb_handle_damage()
dlfb_get_urb()
down_timeout(semaphore)
BUG: scheduling while atomic
(...)
vt_console_print() (release spinlock printing_lock)
Fix this through a workqueue for dlfb_handle_damage().
Cc: <stable@vger.kernel.org>
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
drivers/video/udlfb.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
include/video/udlfb.h | 1 +
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index 86d449e..4a90784 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -569,7 +569,7 @@ static int dlfb_render_hline(struct dlfb_data *dev, struct urb **urb_ptr,
return 0;
}
-int dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+int dlfb_handle_damage_queued(struct dlfb_data *dev, int x, int y,
int width, int height, char *data)
{
int i, ret;
@@ -630,6 +630,44 @@ error:
return 0;
}
+struct dlfb_handle_damage_work {
+ struct work_struct my_work;
+ struct dlfb_data *dev;
+ char *data;
+ int x, y, width, height;
+};
+
+static void dlfb_handle_damage_work(struct work_struct *work)
+{
+ struct dlfb_handle_damage_work *my_work + (struct dlfb_handle_damage_work *)work;
+
+ dlfb_handle_damage_queued(my_work->dev, my_work->x, my_work->y,
+ my_work->width, my_work->height, my_work->data);
+ kfree(work);
+ return;
+}
+
+void dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+ int width, int height, char *data)
+{
+ struct dlfb_handle_damage_work *work + kmalloc(sizeof(struct dlfb_handle_damage_work), GFP_KERNEL);
+
+ if (!work) {
+ pr_err("unable to allocate work\n");
+ return;
+ }
+ INIT_WORK((struct work_struct *)work, dlfb_handle_damage_work);
+ work->dev = dev;
+ work->x = x;
+ work->y = y;
+ work->width = width;
+ work->height = height;
+ work->data = data;
+ queue_work(dev->handle_damage_wq, (struct work_struct *)work);
+}
+
/*
* Path triggered by usermode clients who write to filesystem
* e.g. cat filename > /dev/fb1
@@ -945,6 +983,9 @@ static void dlfb_free_framebuffer(struct dlfb_data *dev)
unregister_framebuffer(info);
+ if (dev->handle_damage_wq)
+ destroy_workqueue(dev->handle_damage_wq);
+
if (info->cmap.len != 0)
fb_dealloc_cmap(&info->cmap);
if (info->monspecs.modedb)
@@ -1694,6 +1735,13 @@ static void dlfb_init_framebuffer_work(struct work_struct *work)
goto error;
}
+ dev->handle_damage_wq = alloc_workqueue("udlfb_damage",
+ WQ_MEM_RECLAIM, 0);
+ if (dev->handle_damage_wq = NULL) {
+ pr_err("unable to allocate workqueue\n");
+ goto error;
+ }
+
/* ready to begin using device */
atomic_set(&dev->usb_active, 1);
diff --git a/include/video/udlfb.h b/include/video/udlfb.h
index f9466fa..1e765f9 100644
--- a/include/video/udlfb.h
+++ b/include/video/udlfb.h
@@ -43,6 +43,7 @@ struct dlfb_data {
bool virtualized; /* true when physical usb device not present */
struct delayed_work init_framebuffer_work;
struct delayed_work free_framebuffer_work;
+ struct workqueue_struct *handle_damage_wq;
atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */
atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
char *edid; /* null until we read edid from hw or get from sysfs */
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCHv16 0/7] of: add display helper
From: Marek Vasut @ 2013-01-09 19:12 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Tomi Valkeinen, Stephen Warren, kernel,
Florian Tobias Schandinat, David Airlie, Rob Clark,
Leela Krishna Amudala
In-Reply-To: <1355850256-16135-1-git-send-email-s.trumtrar@pengutronix.de>
Dear Steffen Trumtrar,
> Hi!
>
> Finally, right in time before the end of the world on friday, v16 of the
> display helpers.
I tested this on 3.8-rc1 (next 20130103) with the imx drm driver. After adding
the following piece of code (quick hack), this works just fine. Thanks!
diff --git a/drivers/staging/imx-drm/parallel-display.c b/drivers/staging/imx-
drm/parallel-display.c
index a8064fc..e45002a 100644
--- a/drivers/staging/imx-drm/parallel-display.c
+++ b/drivers/staging/imx-drm/parallel-display.c
@@ -57,6 +57,7 @@ static void imx_pd_connector_destroy(struct drm_connector
*connector)
static int imx_pd_connector_get_modes(struct drm_connector *connector)
{
struct imx_parallel_display *imxpd = con_to_imxpd(connector);
+ struct device_node *np = imxpd->dev->of_node;
int num_modes = 0;
if (imxpd->edid) {
@@ -72,6 +73,15 @@ static int imx_pd_connector_get_modes(struct drm_connector
*connector)
num_modes++;
}
+ if (np) {
+ struct drm_display_mode *mode = drm_mode_create(connector->dev);
+ of_get_drm_display_mode(np, &imxpd->mode, 0);
+ drm_mode_copy(mode, &imxpd->mode);
+ mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+ drm_mode_probed_add(connector, mode);
+ num_modes++;
+ }
+
return num_modes;
}
Best regards,
Marek Vasut
^ permalink raw reply related
* Re: [PATCHv16 0/7] of: add display helper
From: Steffen Trumtrar @ 2013-01-09 19:39 UTC (permalink / raw)
To: Marek Vasut
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Tomi Valkeinen, Stephen Warren, kernel,
Florian Tobias Schandinat, David Airlie, Rob Clark,
Leela Krishna Amudala
In-Reply-To: <201301092012.01985.marex@denx.de>
Hi!
On Wed, Jan 09, 2013 at 08:12:01PM +0100, Marek Vasut wrote:
> Dear Steffen Trumtrar,
>
> I tested this on 3.8-rc1 (next 20130103) with the imx drm driver. After adding
> the following piece of code (quick hack), this works just fine. Thanks!
>
> diff --git a/drivers/staging/imx-drm/parallel-display.c b/drivers/staging/imx-
> drm/parallel-display.c
> index a8064fc..e45002a 100644
> --- a/drivers/staging/imx-drm/parallel-display.c
> +++ b/drivers/staging/imx-drm/parallel-display.c
> @@ -57,6 +57,7 @@ static void imx_pd_connector_destroy(struct drm_connector
> *connector)
> static int imx_pd_connector_get_modes(struct drm_connector *connector)
> {
> struct imx_parallel_display *imxpd = con_to_imxpd(connector);
> + struct device_node *np = imxpd->dev->of_node;
> int num_modes = 0;
>
> if (imxpd->edid) {
> @@ -72,6 +73,15 @@ static int imx_pd_connector_get_modes(struct drm_connector
> *connector)
> num_modes++;
> }
>
> + if (np) {
> + struct drm_display_mode *mode = drm_mode_create(connector->dev);
> + of_get_drm_display_mode(np, &imxpd->mode, 0);
> + drm_mode_copy(mode, &imxpd->mode);
> + mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> + drm_mode_probed_add(connector, mode);
> + num_modes++;
> + }
> +
> return num_modes;
> }
>
Nice! I haven't tried the parallel display, but I think Philipp Zabel might
already have a patch for it. If not, I will definitly keep your patch in my
topic branch.
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCHv16 0/7] of: add display helper
From: Marek Vasut @ 2013-01-09 19:56 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Tomi Valkeinen, Stephen Warren, kernel,
Florian Tobias Schandinat, David Airlie, Rob Clark,
Leela Krishna Amudala
In-Reply-To: <20130109193953.GA4780@pengutronix.de>
Dear Steffen Trumtrar,
> Hi!
>
> On Wed, Jan 09, 2013 at 08:12:01PM +0100, Marek Vasut wrote:
> > Dear Steffen Trumtrar,
> >
> > I tested this on 3.8-rc1 (next 20130103) with the imx drm driver. After
> > adding the following piece of code (quick hack), this works just fine.
> > Thanks!
> >
> > diff --git a/drivers/staging/imx-drm/parallel-display.c
> > b/drivers/staging/imx- drm/parallel-display.c
> > index a8064fc..e45002a 100644
> > --- a/drivers/staging/imx-drm/parallel-display.c
> > +++ b/drivers/staging/imx-drm/parallel-display.c
> > @@ -57,6 +57,7 @@ static void imx_pd_connector_destroy(struct
> > drm_connector *connector)
> >
> > static int imx_pd_connector_get_modes(struct drm_connector *connector)
> > {
> >
> > struct imx_parallel_display *imxpd = con_to_imxpd(connector);
> >
> > + struct device_node *np = imxpd->dev->of_node;
> >
> > int num_modes = 0;
> >
> > if (imxpd->edid) {
> >
> > @@ -72,6 +73,15 @@ static int imx_pd_connector_get_modes(struct
> > drm_connector *connector)
> >
> > num_modes++;
> >
> > }
> >
> > + if (np) {
> > + struct drm_display_mode *mode > > drm_mode_create(connector->dev); +
> > of_get_drm_display_mode(np, &imxpd->mode, 0);
> > + drm_mode_copy(mode, &imxpd->mode);
> > + mode->type |= DRM_MODE_TYPE_DRIVER |
> > DRM_MODE_TYPE_PREFERRED, + drm_mode_probed_add(connector,
> > mode);
> > + num_modes++;
> > + }
> > +
> >
> > return num_modes;
> >
> > }
>
> Nice! I haven't tried the parallel display, but I think Philipp Zabel might
> already have a patch for it. If not, I will definitly keep your patch in my
> topic branch.
Works like charm here.
Make sure to adjust the patch and check for the return value of
of_get_drm_display_mode(np, &imxpd->mode, 0); call, that's probably the only
issue that needs fixing in that hack. Checking if np != NULL might not hurt
either. I can roll you a real patch if it helps.
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCHv16 0/7] of: add display helper
From: Steffen Trumtrar @ 2013-01-09 20:15 UTC (permalink / raw)
To: devicetree-discuss
Cc: Rob Herring, linux-fbdev, dri-devel, Laurent Pinchart,
Thierry Reding, Guennady Liakhovetski, linux-media,
Tomi Valkeinen, Stephen Warren, kernel, Florian Tobias Schandinat,
David Airlie, Rob Clark, Leela Krishna Amudala
In-Reply-To: <1355850256-16135-1-git-send-email-s.trumtrar@pengutronix.de>
On Tue, Dec 18, 2012 at 06:04:09PM +0100, Steffen Trumtrar wrote:
> Hi!
>
> Finally, right in time before the end of the world on friday, v16 of the
> display helpers.
>
So, any more criticism on the series? Any takers for the series as is?
I guess it could be merged via the fbdev-tree if David Airlie can agree
to the DRM patches ?! Does that sound about right?
I think the series was tested at least with
- imx6q: sabrelite, sabresd
- imx53: tqma53/mba53
- omap: DA850 EVM, AM335x EVM, EVM-SK
I don't know what Laurent Pinchart, Marek Vasut and Leela Krishna Amudala
are using. Those are the people I know from the top of my head, that use
or at least did use the patches in one of its iterations. If I forgot
anyone, please speak up and possibly add your new HW to the list of tested
devices.
Thanks,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCHv16 0/7] of: add display helper
From: Thierry Reding @ 2013-01-09 20:37 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, David Airlie,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Clark,
Tomi Valkeinen, Laurent Pinchart, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
Guennady Liakhovetski, linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20130109201541.GB4780-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]
On Wed, Jan 09, 2013 at 09:15:41PM +0100, Steffen Trumtrar wrote:
> On Tue, Dec 18, 2012 at 06:04:09PM +0100, Steffen Trumtrar wrote:
> > Hi!
> >
> > Finally, right in time before the end of the world on friday, v16 of the
> > display helpers.
> >
>
> So, any more criticism on the series? Any takers for the series as is?
> I guess it could be merged via the fbdev-tree if David Airlie can agree
> to the DRM patches ?! Does that sound about right?
>
> I think the series was tested at least with
> - imx6q: sabrelite, sabresd
> - imx53: tqma53/mba53
> - omap: DA850 EVM, AM335x EVM, EVM-SK
>
> I don't know what Laurent Pinchart, Marek Vasut and Leela Krishna Amudala
> are using. Those are the people I know from the top of my head, that use
> or at least did use the patches in one of its iterations. If I forgot
> anyone, please speak up and possibly add your new HW to the list of tested
> devices.
I tested earlier versions on Tegra. The latest one was v15 I think.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox