Re: [PATCH v2] OMAPDSS: provide default timings functions for panels

On Wed, 2012-03-14 at 18:33 +0200, Grazvydas Ignotas wrote:

On Wed, Mar 14, 2012 at 1:22 PM, Tomi Valkeinen [off-list ref] wrote:

quoted

Hi,

On Mon, 2012-03-12 at 13:27 +0200, Grazvydas Ignotas wrote:

quoted

With this we can eliminate some duplicate code in panel drivers.
Also lgphilips-lb035q02, nec-nl8048hl11-01b, picodlp and
tpo-td043mtea1 gain support of timings control over sysfs.

I don't like this patch.

Panels usually have a single, fixed timing configuration that should be
used, like the ones you mention above. There's no need to alter the
timings.

But they often have a range of timings they can tolerate, and that can
be used to alter refresh rate, for example. We do that on pandora to
match graphics drawing rate (or multiples of it) to create a feeling
smoothness.

True. And it's a valid operation anyway, so I guess there's no reason
why not to allow changing of the timings there.

quoted

But it's true that there's some duplicate code currently in the panel
drivers. However, adding just simple funcs like you did in this patch
doesn't work quite properly. There should be locking (for example to
prevent disabling the panel while timings are being set), and currently
the locking is panel driver specific.

Oh, and one more problem with the patch is that currently the panel
informs its inability to change timings by leaving set_timings and
check_timings as NULL, and this tells omapfb etc that the timings cannot
be changed, and the patch changes that behavior.

ok, what about a version of this with .get_timings only then?
This should not need a lock unless panel has a set function, but in
that case panel will be expected to provide safe version of .get and
.set itself.

I guess there's no harm in having default for get_timings(). It should
be present on all panel drivers anyway.

 Tomi