[PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Raphael Assenat]

Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.

Signed-off-by: Raphael Assenat <raph@8d.com>

--- a/drivers/video/omap2/displays/panel-generic-dpi.c
+++ b/drivers/video/omap2/displays/panel-generic-dpi.c
@@ -486,6 +486,80 @@ static struct panel_config generic_dpi_panels[] = {
 					  OMAP_DSS_LCD_IHS | OMAP_DSS_LCD_IPC,
 		.name			= "primeview_pd104slf",
 	},
+
+	/* ChiMei G121S1-L01 */
+	{
+		{
+			.x_res		= 800,
+			.y_res		= 600,
+
+			.pixel_clock	= 39700,
+
+			.hfp		= 128,
+			.hsw		= 1,
+			.hbp		= 128,
+
+			.vfp		= 28,
+			.vsw		= 1,
+			.vbp		= 28
+		},
+		.acbi			= 0x0,
+		.acb			= 0x0,
+		.config			= OMAP_DSS_LCD_TFT,
+		.power_on_delay		= 0,
+		.power_off_delay	= 0,
+		.name			= "chimei_g121s1-l01",
+	},
+
+	/* ChiMei G121S1-L02 */
+	{
+		{
+			.x_res		= 800,
+			.y_res		= 600,
+
+			.pixel_clock	= 40000,
+
+			.hfp		= 1,
+			.hsw		= 256,
+			.hbp		= 1,
+
+			.vfp		= 1,
+			.vsw		= 28,
+			.vbp		= 1
+		},
+		.acbi			= 0x0,
+		.acb			= 0x0,
+		.config			= OMAP_DSS_LCD_TFT,
+		.power_on_delay		= 0,
+		.power_off_delay	= 0,
+		.name			= "chimei_g121s1-l02",
+	},
+
+
+	/* ChiMei G121X1-L01 */
+	{
+		{
+			.x_res		= 1024,
+			.y_res		= 768,
+
+			.pixel_clock	= 64900,
+
+			.hfp		= 160,
+			.hsw		= 1,
+			.hbp		= 160,
+
+			.vfp		= 38,
+			.vsw		= 1,
+			.vbp		= 38
+		},
+		.acbi			= 0x0,
+		.acb			= 0x0,
+		.config			= OMAP_DSS_LCD_TFT,
+		.power_on_delay		= 0,
+		.power_off_delay	= 0,
+		.name			= "chimei_g121x1-l01",
+	},
+
 };
 
 struct panel_drv_data {

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Jassi Brar]

[CC'ing OMAPDSS matinainer]

On 17 July 2012 19:31, Raphael Assenat <raph@8d.com> wrote:
> Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
>
Display panels are board specific and there is no limit to the number
of panels that could be connected to omap dss.
 Does it make sense to get panel params via DT? Or at least have them
come from board file? (esp when there is hardly a panel shared by two
boards, and some panels aren't even used by any board in mainline)

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Archit Taneja]

Hi,

On Tuesday 17 July 2012 09:57 PM, Jassi Brar wrote:
> [CC'ing OMAPDSS matinainer]
>
> On 17 July 2012 19:31, Raphael Assenat <raph@8d.com> wrote:
>> Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
>>
> Display panels are board specific and there is no limit to the number
> of panels that could be connected to omap dss.
>   Does it make sense to get panel params via DT? Or at least have them
> come from board file? (esp when there is hardly a panel shared by two
> boards, and some panels aren't even used by any board in mainline)
>

A panel specific param should stay in the panel driver, it's something 
which is specific to the panel and not the platform it is in, things 
like the gpio reset number, i2c connections with the panel etc would 
make sense to come via DT/board file.

It's true that currently omap platforms don't share the same panels, but 
there is no stopping us to do that. We could remove the default panel 
and attach a new one, even though we won't upstream non default panels 
in the DT/board file, it would be always easier to make this change in 
software if most of the panel specific info stays in the panel driver.

Also, 2 platforms of different SoC's may use the same panel. Currently 
the panel drivers are SoC specific, but there is work being done between 
different display maintainers so that the same panel driver works across 
different SoCs.

Archit

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Jassi Brar]

On 20 July 2012 13:41, Archit Taneja <a0393947@ti.com> wrote:
> On Tuesday 17 July 2012 09:57 PM, Jassi Brar wrote:
>>>
>>> Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
>>>
>> Display panels are board specific and there is no limit to the number
>> of panels that could be connected to omap dss.
>>   Does it make sense to get panel params via DT? Or at least have them
>> come from board file? (esp when there is hardly a panel shared by two
>> boards, and some panels aren't even used by any board in mainline)
>>
>
> A panel specific param should stay in the panel driver, it's something which
> is specific to the panel and not the platform it is in
>
Yes it is board specific, but no it should not stay in the driver. The
driver simply needs one compatible set of 15 numbers to do its job.

Let me explain my point in detail....
The array generic_dpi_panels[] is but a limited list of compatible
configurations of a 'generic' panel. Each occupying ~20 lines in
kernel. There would be dozens of supported panels that exist but are
not listed in this array, and countless more that are possible to
manufacture. If I submit a 2000 lines patch with only 100 such
configurations you would have no reason to reject other than "I know
what you mean" :)

> It's true that currently omap platforms don't share the same panels, but
> there is no stopping us to do that. We could remove the default panel and
> attach a new one, even though we won't upstream non default panels in the
> DT/board file, it would be always easier to make this change in software if
> most of the panel specific info stays in the panel driver.
>
You mean you want to hardcode parameters in the driver instead of
modifying the dtb that you pass to the kernel?

> Also, 2 platforms of different SoC's may use the same panel. Currently the
> panel drivers are SoC specific, but there is work being done between
> different display maintainers so that the same panel driver works across
> different SoCs.
>
Doesn't that make the case for DT/platform_data even stronger?

Of course you as a maintainer have the final say. I am out of ways to
explain my point.

Cheers.

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Archit Taneja]

On Friday 20 July 2012 05:43 PM, Jassi Brar wrote:
> On 20 July 2012 13:41, Archit Taneja <a0393947@ti.com> wrote:
>> On Tuesday 17 July 2012 09:57 PM, Jassi Brar wrote:
>>>>
>>>> Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
>>>>
>>> Display panels are board specific and there is no limit to the number
>>> of panels that could be connected to omap dss.
>>>    Does it make sense to get panel params via DT? Or at least have them
>>> come from board file? (esp when there is hardly a panel shared by two
>>> boards, and some panels aren't even used by any board in mainline)
>>>
>>
>> A panel specific param should stay in the panel driver, it's something which
>> is specific to the panel and not the platform it is in
>>
> Yes it is board specific, but no it should not stay in the driver. The
> driver simply needs one compatible set of 15 numbers to do its job.

I agree that the panel on the 'current' platform just needs the 15 
numbers. The older way how this was done was to have a separate driver 
for each such panel. There used to be a ton of panel driver c files 
doing almost the same thing, having only these 15 parameters different. 
This was merged into one generic driver, with each panel's properties as 
an element of the array.

>
> Let me explain my point in detail....
> The array generic_dpi_panels[] is but a limited list of compatible
> configurations of a 'generic' panel. Each occupying ~20 lines in
> kernel. There would be dozens of supported panels that exist but are
> not listed in this array, and countless more that are possible to
> manufacture. If I submit a 2000 lines patch with only 100 such
> configurations you would have no reason to reject other than "I know
> what you mean" :)

I think I get what you mean now. You are saying that we should create a 
struct/member in DT which supports this class of dumb DPI panels. You 
are sort of reducing the panel to a set of parameters like resolution, 
hsync/vsync polarities.

It sort of makes sense, but this will be exclusive to such dumb DPI 
panels. The moment a panel becomes more complex, for example, have it's 
own register set, I guess we would need to have keep those properties in 
the panel driver, rather than DT.

I don't know much about DT. But are there other devices which are 
completely represented by DT? For example, would a keypad/keyboard's 
parameters be totally represented in the DT blob, i.e, the number of 
keys, the mappings etc?

>
>> It's true that currently omap platforms don't share the same panels, but
>> there is no stopping us to do that. We could remove the default panel and
>> attach a new one, even though we won't upstream non default panels in the
>> DT/board file, it would be always easier to make this change in software if
>> most of the panel specific info stays in the panel driver.
>>
> You mean you want to hardcode parameters in the driver instead of
> modifying the dtb that you pass to the kernel?

I meant that, but if we go with your approach, which sort of makes sense 
now, it would be in the dtb file.

>
>> Also, 2 platforms of different SoC's may use the same panel. Currently the
>> panel drivers are SoC specific, but there is work being done between
>> different display maintainers so that the same panel driver works across
>> different SoCs.
>>
> Doesn't that make the case for DT/platform_data even stronger?
>
> Of course you as a maintainer have the final say. I am out of ways to
> explain my point.

I get your point now. You are generalising/reducing a panel as a set of 
properties which can be linked to a platform. I didn't think of it that way.

One little negative I see with this approach though is the integrity of 
the panel parameters in the dtb file. If a person working on a new 
platform has a panel that's already in the 'list', he/she would only 
need to specify the name, and be assured that the driver has the right 
parameters to configure it. With the dtb way, if the person feeds a 
wrong value in the dtb, say an incorrect resolution, we'll be in 
trouble. But as I said, it's a little negative, it's not our fault if 
the dtb writer of the platform makes mistakes :)

I am not the maintainer, Tomi is :), we could wait for his comments once 
he's back from vacation.

Thanks,
Archit

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Jassi Brar]

On 20 July 2012 18:14, Archit Taneja <a0393947@ti.com> wrote:
> On Friday 20 July 2012 05:43 PM, Jassi Brar wrote:
>>
>>> It's true that currently omap platforms don't share the same panels, but
>>> there is no stopping us to do that. We could remove the default panel and
>>> attach a new one, even though we won't upstream non default panels in the
>>> DT/board file, it would be always easier to make this change in software
>>> if
>>> most of the panel specific info stays in the panel driver.
>>>
>> You mean you want to hardcode parameters in the driver instead of
>> modifying the dtb that you pass to the kernel?
>
> I meant that, but if we go with your approach, which sort of makes sense
> now, it would be in the dtb file.
>
In dtb or via platform_data until we have common DT bindings. I have
sent a patch doing it via platform_data, which maybe taken as such or
after making it prettier to taste.

Thanks.

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On Tue, 2012-07-17 at 21:57 +0530, Jassi Brar wrote:
> [CC'ing OMAPDSS matinainer]
> 
> On 17 July 2012 19:31, Raphael Assenat <raph@8d.com> wrote:
> > Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
> >
> Display panels are board specific and there is no limit to the number
> of panels that could be connected to omap dss.
>  Does it make sense to get panel params via DT? Or at least have them
> come from board file? (esp when there is hardly a panel shared by two
> boards, and some panels aren't even used by any board in mainline)

So we have two options, with pros and cons:

1) Have the configuration for countless panels specified in the driver

- Pro: driver for the device is the right place to define hardcoded
device properties
- Pro: panels can be easily used from the board file, just define the
name of the panel
- Pro: the same panel can be easily used from multiple board files,
without duplicating the configs
- Con: Adds lines to the kernel (not really a con, all features add
lines to the kernel. and we can restructure the data to fit fewer
lines.)
- Con: We could have "leftover" panel data, not used by anyone.

2) Have the configuration for countless panels specified in the DT data

- Con: DT data is not the right place to describe device's internal
hardcoded properties. DT data should be about HW connections and
configurable options.
- Con: Adds lines to the DT data

What were the pros for option 2? I didn't really see them in this mail
thread, except moving lines from the kernel to the DT, which I don't
really see as a pro.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Jassi Brar]

On 31 July 2012 13:21, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> 2) Have the configuration for countless panels specified in the DT data
>
Why should a DT blob for a board contain more than 1 panel configuration?

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

On Tue, 2012-07-31 at 13:33 +0530, Jassi Brar wrote:
> On 31 July 2012 13:21, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 
> > 2) Have the configuration for countless panels specified in the DT data
> >
> Why should a DT blob for a board contain more than 1 panel configuration?

I meant the DT data generally, for all boards.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Jassi Brar]

On 31 July 2012 13:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-07-31 at 13:33 +0530, Jassi Brar wrote:
>> On 31 July 2012 13:21, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>> > 2) Have the configuration for countless panels specified in the DT data
>> >
>> Why should a DT blob for a board contain more than 1 panel configuration?
>
> I meant the DT data generally, for all boards.
>
If you mean : Why have the configuration (those 15 integers) of the
panel on a board specified in board.dtb?
Well, that is an important purpose of DT - moving board specific
parameters, on which a generic code works, out of kernel (I am
refraining from preaching the goodness of that).

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

On Tue, 2012-07-31 at 13:57 +0530, Jassi Brar wrote:
> On 31 July 2012 13:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Tue, 2012-07-31 at 13:33 +0530, Jassi Brar wrote:
> >> On 31 July 2012 13:21, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>
> >> > 2) Have the configuration for countless panels specified in the DT data
> >> >
> >> Why should a DT blob for a board contain more than 1 panel configuration?
> >
> > I meant the DT data generally, for all boards.
> >
> If you mean : Why have the configuration (those 15 integers) of the
> panel on a board specified in board.dtb?
> Well, that is an important purpose of DT - moving board specific
> parameters, on which a generic code works, out of kernel (I am
> refraining from preaching the goodness of that).

Sure. But panel's unconfigurable properties are not board specific
parameters, they are panel's internal stuff. It doesn't matter to which
board I attach Acme Foo-123 panel, the panel timings are still the same.

Okay, agreed, the timing properties are not unconfigurable. But I'd
expect that normally the timings used are the normal timings specified
in the panel's datasheet.

For the cases where the board manufacturer wants to use non-standard
timings, perhaps we could allow overriding with DT data the ones
specified in the kernel. Or have a panel driver that expects to get all
the data from DT.

But I don't see why a panel database in the kernel driver is a bad thing
(presuming it can be used in most cases). The driver should allow using
the panel device without requiring the user to give extra information.
(extra in the sense that the driver should already know about that
info).

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Jassi Brar]

On 31 July 2012 14:12, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-07-31 at 13:57 +0530, Jassi Brar wrote:
>> On 31 July 2012 13:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Tue, 2012-07-31 at 13:33 +0530, Jassi Brar wrote:
>> >> On 31 July 2012 13:21, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> >>
>> >> > 2) Have the configuration for countless panels specified in the DT data
>> >> >
>> >> Why should a DT blob for a board contain more than 1 panel configuration?
>> >
>> > I meant the DT data generally, for all boards.
>> >
>> If you mean : Why have the configuration (those 15 integers) of the
>> panel on a board specified in board.dtb?
>> Well, that is an important purpose of DT - moving board specific
>> parameters, on which a generic code works, out of kernel (I am
>> refraining from preaching the goodness of that).
>
> Sure. But panel's unconfigurable properties are not board specific
> parameters, they are panel's internal stuff. It doesn't matter to which
> board I attach Acme Foo-123 panel, the panel timings are still the same.
>
It's not about the panel, it's about the board. For the generic driver
in the kernel , the 'panel' is just a set of 15 integer values.
There's no "Acme Foo-123" or "Acme Bar-123".  In fact, the _only_
purpose of the panel's name string in the driver is to pick the
correct set of "15 integers". With DT, the name string would be
unnecessary.

Consider two panels "ABC_123" and "XYZ_321" having identical
parameters but different internals.
Would you have duplicate elements in the generic_dpi_panels[] array ?
Because the 'panels' are different.
Or would you simply assume the XYZ_Board has the panel 'ABC_123'?
Because after all it's the parameters that matter.

In short, we should see a 'panel' simply as a set of 15 integers.

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4371 bytes --]

On Tue, 2012-07-31 at 14:27 +0530, Jassi Brar wrote:
> On 31 July 2012 14:12, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Tue, 2012-07-31 at 13:57 +0530, Jassi Brar wrote:
> >> On 31 July 2012 13:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> > On Tue, 2012-07-31 at 13:33 +0530, Jassi Brar wrote:
> >> >> On 31 July 2012 13:21, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> >>
> >> >> > 2) Have the configuration for countless panels specified in the DT data
> >> >> >
> >> >> Why should a DT blob for a board contain more than 1 panel configuration?
> >> >
> >> > I meant the DT data generally, for all boards.
> >> >
> >> If you mean : Why have the configuration (those 15 integers) of the
> >> panel on a board specified in board.dtb?
> >> Well, that is an important purpose of DT - moving board specific
> >> parameters, on which a generic code works, out of kernel (I am
> >> refraining from preaching the goodness of that).
> >
> > Sure. But panel's unconfigurable properties are not board specific
> > parameters, they are panel's internal stuff. It doesn't matter to which
> > board I attach Acme Foo-123 panel, the panel timings are still the same.
> >
> It's not about the panel, it's about the board. For the generic driver
> in the kernel , the 'panel' is just a set of 15 integer values.
> There's no "Acme Foo-123" or "Acme Bar-123".  In fact, the _only_
> purpose of the panel's name string in the driver is to pick the
> correct set of "15 integers". With DT, the name string would be
> unnecessary.

Yes, the panel's name is used to "probe" the correct config. If we had
panels that could be asked "which panel are you" we could use that, but
with dummy panels we need to manually give the identifier (name) so that
the driver can do the probe.

> Consider two panels "ABC_123" and "XYZ_321" having identical
> parameters but different internals.
> Would you have duplicate elements in the generic_dpi_panels[] array ?
> Because the 'panels' are different.
> Or would you simply assume the XYZ_Board has the panel 'ABC_123'?
> Because after all it's the parameters that matter.

I would duplicate the elements. Or, if we have lots of panels having the
exact same parameters, we could have an array of names instead of just a
name.

> In short, we should see a 'panel' simply as a set of 15 integers.

Ok, I see. You mean that the 15 integers define the panel, so, in a
sense, the 15 integers is the name/identifier for the panel.

It would technically work, of course. But I do disagree with it:

1) I still don't see why you say it's board related. The properties in
question are properties of the panel, told in the panel specs, and
programmed when using the panel. No matter where the panel is used, the
same properties should be used.

2) As I see it, describing non-configurable device hardware properties
in the DT data is the wrong way. The driver should either probe the
properties or an ID from the device, or the ID of the device should be
given to the driver (a bit like what can be done with i2c).

3) Moving the data to DT would make any future changes more difficult.
Say, we could (probably should) add some regulator handling to the
driver, because usually panels need power to operate. Currently we just
presume the powers are always on.

Adding this is easy with the current approach. Adding it if the data is
in DT would be difficult, if not impossible, as all the board out there
could already be using the old DT format which doesn't have the
regulator data. Even in the best case all the boards out there would not
be able to use the regulator stuff.


Academic issues aside, what is the issue with the current approach in
practice? How would the DT approach make it better? Both approaches work
just fine, afaik. The current approach requires some maintenance from
me, but that's rather minor.

Anyway, even if I don't see the point, I'm not strictly against your
approach. If everybody thinks it's much better, it's fine for me. We're
currently discussing a platform agnostic approach to panel drivers with
some other SoC guys, I'll raise this question.

However, I don't want to apply the patch to move the properties to board
files. We're gonna move to DT anyway, and that patch would be just extra
stuff.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Jassi Brar]

On 31 July 2012 15:27, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-07-31 at 14:27 +0530, Jassi Brar wrote:
>> On 31 July 2012 14:12, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Tue, 2012-07-31 at 13:57 +0530, Jassi Brar wrote:
>> >> On 31 July 2012 13:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> >> > On Tue, 2012-07-31 at 13:33 +0530, Jassi Brar wrote:
>> >> >> On 31 July 2012 13:21, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> >> >>
>> >> >> > 2) Have the configuration for countless panels specified in the DT data
>> >> >> >
>> >> >> Why should a DT blob for a board contain more than 1 panel configuration?
>> >> >
>> >> > I meant the DT data generally, for all boards.
>> >> >
>> >> If you mean : Why have the configuration (those 15 integers) of the
>> >> panel on a board specified in board.dtb?
>> >> Well, that is an important purpose of DT - moving board specific
>> >> parameters, on which a generic code works, out of kernel (I am
>> >> refraining from preaching the goodness of that).
>> >
>> > Sure. But panel's unconfigurable properties are not board specific
>> > parameters, they are panel's internal stuff. It doesn't matter to which
>> > board I attach Acme Foo-123 panel, the panel timings are still the same.
>> >
>> It's not about the panel, it's about the board. For the generic driver
>> in the kernel , the 'panel' is just a set of 15 integer values.
>> There's no "Acme Foo-123" or "Acme Bar-123".  In fact, the _only_
>> purpose of the panel's name string in the driver is to pick the
>> correct set of "15 integers". With DT, the name string would be
>> unnecessary.
>
> Yes, the panel's name is used to "probe" the correct config. If we had
> panels that could be asked "which panel are you" we could use that, but
> with dummy panels we need to manually give the identifier (name) so that
> the driver can do the probe.
>
>> Consider two panels "ABC_123" and "XYZ_321" having identical
>> parameters but different internals.
>> Would you have duplicate elements in the generic_dpi_panels[] array ?
>> Because the 'panels' are different.
>> Or would you simply assume the XYZ_Board has the panel 'ABC_123'?
>> Because after all it's the parameters that matter.
>
> I would duplicate the elements. Or, if we have lots of panels having the
> exact same parameters, we could have an array of names instead of just a
> name.
>
>> In short, we should see a 'panel' simply as a set of 15 integers.
>
> Ok, I see. You mean that the 15 integers define the panel, so, in a
> sense, the 15 integers is the name/identifier for the panel.
>
> It would technically work, of course. But I do disagree with it:
>
> 1) I still don't see why you say it's board related. The properties in
> question are properties of the panel, told in the panel specs, and
> programmed when using the panel. No matter where the panel is used, the
> same properties should be used.
>
> 2) As I see it, describing non-configurable device hardware properties
> in the DT data is the wrong way. The driver should either probe the
> properties or an ID from the device, or the ID of the device should be
> given to the driver (a bit like what can be done with i2c).
>
> 3) Moving the data to DT would make any future changes more difficult.
> Say, we could (probably should) add some regulator handling to the
> driver, because usually panels need power to operate. Currently we just
> presume the powers are always on.
>
> Adding this is easy with the current approach. Adding it if the data is
> in DT would be difficult, if not impossible, as all the board out there
> could already be using the old DT format which doesn't have the
> regulator data. Even in the best case all the boards out there would not
> be able to use the regulator stuff.
>
>
> Academic issues aside, what is the issue with the current approach in
> practice? How would the DT approach make it better? Both approaches work
> just fine, afaik. The current approach requires some maintenance from
> me, but that's rather minor.
>
> Anyway, even if I don't see the point, I'm not strictly against your
> approach. If everybody thinks it's much better, it's fine for me.
>
No, I don't insist. Its only I in 'everybody' here, so please do it
only if you see any merit in passing panel parameters via DT. I only
wanted to share my opinion and so I did.

Best Regards.
-jassi

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2664 bytes --]

Hi,

On Tue, 2012-07-17 at 10:01 -0400, Raphael Assenat wrote:
> Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
> 

This needed to be converted to work with latest kernel. See patch below.
I've applied the patch to my dev branch, mail me if it's not correct.


commit f4e491f283266b53a926eb3c9017505b04786b9b (HEAD, dev)
Author: Raphael Assenat <raph@8d.com>
Date:   Tue Jul 17 10:01:40 2012 -0400

    OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays
    
    Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
    
    Signed-off-by: Raphael Assenat <raph@8d.com>
    Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

diff --git a/drivers/video/omap2/displays/panel-generic-dpi.c b/drivers/video/omap2/displays/panel-generic-dpi.c
index bc5af25..ae862bb 100644
--- a/drivers/video/omap2/displays/panel-generic-dpi.c
+++ b/drivers/video/omap2/displays/panel-generic-dpi.c
@@ -538,6 +538,82 @@ static struct panel_config generic_dpi_panels[] = {
 		},
 		.name			= "primeview_pd104slf",
 	},
+
+	/* ChiMei G121S1-L01 */
+	{
+		{
+			.x_res		= 800,
+			.y_res		= 600,
+
+			.pixel_clock	= 39700,
+
+			.hfp		= 128,
+			.hsw		= 1,
+			.hbp		= 128,
+
+			.vfp		= 28,
+			.vsw		= 1,
+			.vbp		= 28,
+
+			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
+			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
+		},
+		.name			= "chimei_g121s1-l01",
+	},
+
+	/* ChiMei G121S1-L02 */
+	{
+		{
+			.x_res		= 800,
+			.y_res		= 600,
+
+			.pixel_clock	= 40000,
+
+			.hfp		= 1,
+			.hsw		= 256,
+			.hbp		= 1,
+
+			.vfp		= 1,
+			.vsw		= 28,
+			.vbp		= 1,
+
+			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
+			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
+		},
+		.name			= "chimei_g121s1-l02",
+	},
+
+
+	/* ChiMei G121X1-L01 */
+	{
+		{
+			.x_res		= 1024,
+			.y_res		= 768,
+
+			.pixel_clock	= 64900,
+
+			.hfp		= 160,
+			.hsw		= 1,
+			.hbp		= 160,
+
+			.vfp		= 38,
+			.vsw		= 1,
+			.vbp		= 38,
+
+			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
+			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
+		},
+		.name			= "chimei_g121x1-l01",
+	},
 };
 
 struct panel_drv_data {


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Raphaël Assénat]

Hi,

On 15/08/12 05:31 AM, Tomi Valkeinen wrote:
> Hi,
> 
> On Tue, 2012-07-17 at 10:01 -0400, Raphael Assenat wrote:
>> Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
>>
> 
> This needed to be converted to work with latest kernel. See patch below.
> I've applied the patch to my dev branch, mail me if it's not correct.
> 
> 
> commit f4e491f283266b53a926eb3c9017505b04786b9b (HEAD, dev)
> Author: Raphael Assenat <raph@8d.com>
> Date:   Tue Jul 17 10:01:40 2012 -0400
> 
>     OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays
>     
>     Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays.
>     
>     Signed-off-by: Raphael Assenat <raph@8d.com>
>     Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> diff --git a/drivers/video/omap2/displays/panel-generic-dpi.c b/drivers/video/omap2/displays/panel-generic-dpi.c
> index bc5af25..ae862bb 100644
> --- a/drivers/video/omap2/displays/panel-generic-dpi.c
> +++ b/drivers/video/omap2/displays/panel-generic-dpi.c
> @@ -538,6 +538,82 @@ static struct panel_config generic_dpi_panels[] = {
>  		},
>  		.name			= "primeview_pd104slf",
>  	},
> +
> +	/* ChiMei G121S1-L01 */
> +	{
> +		{

...

> +			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> +			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> +			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
> +			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> +			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,

Actually those 3 panels only use the DE signal. The hsync/vsync signals
are not used and on our system we mux them out to make sure they are
kept low as recommended in the panel datasheets.

Since vsync/hsync are not used, I think the vsync_level, hsync_level and
sync_pclk_edge entries could be removed. Otherwise the updated patch
works fine as is.

Best regards,
Raphaël Assénat
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Wed, 2012-08-15 at 11:26 -0400, Raphaël Assénat wrote:

> > +
> > +	/* ChiMei G121S1-L01 */
> > +	{
> > +		{
> 
> ...
> 
> > +			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> > +			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> > +			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
> > +			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> > +			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
> 
> Actually those 3 panels only use the DE signal. The hsync/vsync signals
> are not used and on our system we mux them out to make sure they are
> kept low as recommended in the panel datasheets.
> 
> Since vsync/hsync are not used, I think the vsync_level, hsync_level and
> sync_pclk_edge entries could be removed. Otherwise the updated patch
> works fine as is.

Okay. How do panels like that work? How can they know where a new frame
starts?

Actually, I now googled for those panels, and they are all LVDS panels,
not DPI panels. So the patch doesn't look correct at all.

Do you have a DPI-to-LVDS converter chip on your board?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Raphaël Assénat]

On 21/08/12 06:49 AM, Tomi Valkeinen wrote:
> On Wed, 2012-08-15 at 11:26 -0400, Raphaël Assénat wrote:
> 
>>> +
>>> +	/* ChiMei G121S1-L01 */
>>> +	{
>>> +		{
>>
>> ...
>>
>>> +			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
>>> +			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
>>> +			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
>>> +			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
>>> +			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
>>
>> Actually those 3 panels only use the DE signal. The hsync/vsync signals
>> are not used and on our system we mux them out to make sure they are
>> kept low as recommended in the panel datasheets.
>>
>> Since vsync/hsync are not used, I think the vsync_level, hsync_level and
>> sync_pclk_edge entries could be removed. Otherwise the updated patch
>> works fine as is.
> 
> Okay. How do panels like that work? How can they know where a new frame
> starts?

By DE being inactive for a different number of pixel clock cycles during
the vertical and horizontal blanking periods.


> Actually, I now googled for those panels, and they are all LVDS panels,
> not DPI panels. So the patch doesn't look correct at all.
> 
> Do you have a DPI-to-LVDS converter chip on your board?

Yes, we do. Depending on the board, we use a SN75LVDS83B or a SN65LVDS84.

The reason for using this approach was that the panels covered by
this patch seemed not to be compatible with Flatlink 3G, which meant
driving them directly from the AM35xx SDI serial interface was not possible.
We unfortunately do not get to select which LVDS deserializer is
used at the panel side..

Best regards,
Raphaël Assénat
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

On Tue, 2012-08-21 at 10:29 -0400, Raphaël Assénat wrote:
> On 21/08/12 06:49 AM, Tomi Valkeinen wrote:
> > On Wed, 2012-08-15 at 11:26 -0400, Raphaël Assénat wrote:
> > 
> >>> +
> >>> +	/* ChiMei G121S1-L01 */
> >>> +	{
> >>> +		{
> >>
> >> ...
> >>
> >>> +			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> >>> +			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> >>> +			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
> >>> +			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
> >>> +			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
> >>
> >> Actually those 3 panels only use the DE signal. The hsync/vsync signals
> >> are not used and on our system we mux them out to make sure they are
> >> kept low as recommended in the panel datasheets.
> >>
> >> Since vsync/hsync are not used, I think the vsync_level, hsync_level and
> >> sync_pclk_edge entries could be removed. Otherwise the updated patch
> >> works fine as is.
> > 
> > Okay. How do panels like that work? How can they know where a new frame
> > starts?
> 
> By DE being inactive for a different number of pixel clock cycles during
> the vertical and horizontal blanking periods.

Ok. Interesting architecture. I wonder what's the reason for a design
like that...

> > Actually, I now googled for those panels, and they are all LVDS panels,
> > not DPI panels. So the patch doesn't look correct at all.
> > 
> > Do you have a DPI-to-LVDS converter chip on your board?
> 
> Yes, we do. Depending on the board, we use a SN75LVDS83B or a SN65LVDS84.
> 
> The reason for using this approach was that the panels covered by
> this patch seemed not to be compatible with Flatlink 3G, which meant
> driving them directly from the AM35xx SDI serial interface was not possible.
> We unfortunately do not get to select which LVDS deserializer is
> used at the panel side..

Ok. I'm a bit reluctant to add the panels to panel-generic-dpi.c, as
they are not DPI panels at all. Also, you should have drivers for the
DPI-to-LVDS converters. However, this cannot be done properly with the
current DSS driver model, so I'm not sure what to do with your patch.

If you had just one panel and DPI-to-LVDS chip, I'd suggest to create a
combined driver which handles both the chip and the panel. But you have
two chips and three panels...

I'm hoping the common panel framework which is being discussed on the
mailinglists will help here. For the moment, I think it's best if you
keep the panel patches in your kernel tree, and see later how to add
them to common panel framework.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Raphaël Assénat]

On 24/08/12 04:38 AM, Tomi Valkeinen wrote:
> On Tue, 2012-08-21 at 10:29 -0400, Raphaël Assénat wrote:
>> On 21/08/12 06:49 AM, Tomi Valkeinen wrote:
>>> On Wed, 2012-08-15 at 11:26 -0400, Raphaël Assénat wrote:
>>>
>>>>> +
>>>>> +	/* ChiMei G121S1-L01 */
>>>>> +	{
>>>>> +		{
>>>>
>>>> ...
>>>>
>>>>> +			.vsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
>>>>> +			.hsync_level	= OMAPDSS_SIG_ACTIVE_HIGH,
>>>>> +			.data_pclk_edge	= OMAPDSS_DRIVE_SIG_RISING_EDGE,
>>>>> +			.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
>>>>> +			.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
>>>>
>>>> Actually those 3 panels only use the DE signal. The hsync/vsync signals
>>>> are not used and on our system we mux them out to make sure they are
>>>> kept low as recommended in the panel datasheets.
>>>>
>>>> Since vsync/hsync are not used, I think the vsync_level, hsync_level and
>>>> sync_pclk_edge entries could be removed. Otherwise the updated patch
>>>> works fine as is.
>>>
>>> Okay. How do panels like that work? How can they know where a new frame
>>> starts?
>>
>> By DE being inactive for a different number of pixel clock cycles during
>> the vertical and horizontal blanking periods.
> 
> Ok. Interesting architecture. I wonder what's the reason for a design
> like that...
> 
>>> Actually, I now googled for those panels, and they are all LVDS panels,
>>> not DPI panels. So the patch doesn't look correct at all.
>>>
>>> Do you have a DPI-to-LVDS converter chip on your board?
>>
>> Yes, we do. Depending on the board, we use a SN75LVDS83B or a SN65LVDS84.
>>
>> The reason for using this approach was that the panels covered by
>> this patch seemed not to be compatible with Flatlink 3G, which meant
>> driving them directly from the AM35xx SDI serial interface was not possible.
>> We unfortunately do not get to select which LVDS deserializer is
>> used at the panel side..
> 
> Ok. I'm a bit reluctant to add the panels to panel-generic-dpi.c, as
> they are not DPI panels at all. Also, you should have drivers for the
> DPI-to-LVDS converters. However, this cannot be done properly with the
> current DSS driver model, so I'm not sure what to do with your patch.

But internally, perhaps with the exception of the "DE only" operation,
all the same concepts apply. LVDS is just only one extra layer used to
transmit the parallel data more effectively (differential, less
conductors, etc).

Consider the case where an LVDS transmitter is used on the main board
in conjunction with a remote LVDS receiver on a board connected to
a "true" DPI panel as follows:

[OMAPDSS -> LVDS XMIT] -> [long cable] -> [LVDS RXCV -> DPI Panel]
|    parallel        |     lvds serial    |        parallel      |

Given the above use case, adding a new panel to panel-generic-dpi.c would
be obvious since doing without LVDS and driving the panel directly may
still happen if a specific design does not benefit from the use of LVDS.

There are a few different conventions for LVDS transmission where the total
number of bits transmitted by pairs as well as the bit order varies. The
hardware designer selects a compatible pair of chips based on the number of
color bits the panel supports and other factors. But no matter which are
selected and how they are wired, the LVDS layer remains transparent to the
graphics controller and the panel.

My situation is only different from the above by the fact that our panels
have an LVDS receiver built-in, which means the choice of transmitter is
limited to what will be compatible and that a specific bit order (i.e.
wiring)
must be observed. This is why I tend to consider those LVDS panels simply
as DPI panels having a built-in LVDS receiver.


> If you had just one panel and DPI-to-LVDS chip, I'd suggest to create a
> combined driver which handles both the chip and the panel. But you have
> two chips and three panels...

I'm not sure what the chip driver would do. The LVDS layer is totally
transparent to the operation. Why would the kernel need to be aware of
which chip is used?

One thing that might be useful would be the ability to control the
enable pin, but this is not different from the enable pin on a level shifter
placed between the OMAP and a real DPI panel. At the moment, our userspace
software simply controls this using a GPIO.

Best regards,
Raphaël Assénat
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAPDSS: Add timings for ChiMei G121S1-L01/L02 and G121X1-L01 LCD displays

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3363 bytes --]

On Fri, 2012-08-24 at 09:50 -0400, Raphaël Assénat wrote:

> But internally, perhaps with the exception of the "DE only" operation,
> all the same concepts apply. LVDS is just only one extra layer used to
> transmit the parallel data more effectively (differential, less
> conductors, etc).
> 
> Consider the case where an LVDS transmitter is used on the main board
> in conjunction with a remote LVDS receiver on a board connected to
> a "true" DPI panel as follows:
> 
> [OMAPDSS -> LVDS XMIT] -> [long cable] -> [LVDS RXCV -> DPI Panel]
> |    parallel        |     lvds serial    |        parallel      |
> 
> Given the above use case, adding a new panel to panel-generic-dpi.c would
> be obvious since doing without LVDS and driving the panel directly may
> still happen if a specific design does not benefit from the use of LVDS.
> 
> There are a few different conventions for LVDS transmission where the total
> number of bits transmitted by pairs as well as the bit order varies. The
> hardware designer selects a compatible pair of chips based on the number of
> color bits the panel supports and other factors. But no matter which are
> selected and how they are wired, the LVDS layer remains transparent to the
> graphics controller and the panel.
> 
> My situation is only different from the above by the fact that our panels
> have an LVDS receiver built-in, which means the choice of transmitter is
> limited to what will be compatible and that a specific bit order (i.e.
> wiring)
> must be observed. This is why I tend to consider those LVDS panels simply
> as DPI panels having a built-in LVDS receiver.

In your case the LVDS panels are "dummy", and require no configuration,
and thus look like DPI panels.

But consider an LVDS panel which can be controlled via, say, i2c, and
you can configure the number of LVDS lanes used.

And consider a SoC with LVDS transmitter, which again needs to be
configured depending on the LVDS lanes used (and probably some other
parameters).

In those cases the use doesn't look like DPI anymore.

> > If you had just one panel and DPI-to-LVDS chip, I'd suggest to create a
> > combined driver which handles both the chip and the panel. But you have
> > two chips and three panels...
> 
> I'm not sure what the chip driver would do. The LVDS layer is totally
> transparent to the operation. Why would the kernel need to be aware of
> which chip is used?

The chip must require power to operate, so it needs to enable
regulators. For example, SN75LVDS83B requires VCC, IOVCC, PLLVCC,
LVDSVCC. In your case the regulators may be always-on regulators, but a
driver cannot make such assumptions.

> One thing that might be useful would be the ability to control the
> enable pin, but this is not different from the enable pin on a level shifter
> placed between the OMAP and a real DPI panel. At the moment, our userspace
> software simply controls this using a GPIO.

Yes, the driver should also handle the enable GPIO (for the DPI case
also).

Now, as I said, the current omapdss model doesn't really allow
"chaining" of display devices, so you can't currently create proper
drivers. That's why I'm not sure what to do. It would be nice to support
your boards, but on the other hand, I think it's not correct to add
these panels.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]