Re: [PATCH] omapdrm: Make fixed resolution panels work

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

On 2013-02-18 09:23, Archit Taneja wrote:

>>> diff --git a/drivers/staging/omapdrm/omap_connector.c
>>> b/drivers/staging/omapdrm/omap_connector.c
>>> index 4cc9ee7..839c6e4 100644
>>> --- a/drivers/staging/omapdrm/omap_connector.c
>>> +++ b/drivers/staging/omapdrm/omap_connector.c
>>> @@ -110,6 +110,11 @@ static enum drm_connector_status
>>> omap_connector_detect(
>>>                          ret = connector_status_connected;
>>>                  else
>>>                          ret = connector_status_disconnected;
>>> +       } else if (dssdev->type == OMAP_DISPLAY_TYPE_DPI ||
>>> +                       dssdev->type == OMAP_DISPLAY_TYPE_DBI ||
>>> +                       dssdev->type == OMAP_DISPLAY_TYPE_SDI ||
>>> +                       dssdev->type == OMAP_DISPLAY_TYPE_DSI) {
>>> +               ret = connector_status_connected;
>>
>> hmm, why not just have a default detect fxn for panel drivers which
>> always returns true?  Seems a bit cleaner.. otherwise, I guess we
>> could just change omap_connector so that if dssdev->detect is null,
>> assume always connected.  Seems maybe a slightly better way since
>> currently omap_connector doesn't really know too much about different
>> sorts of panels.  But I'm not too picky if that is a big pain because
>> we probably end up changing all this as CFP starts coming into
>> existence.
>>
>> Same goes for check_timings/set_timings..  it seems a bit cleaner just
>> to have default fxns in the panel drivers that do-the-right-thing,
>> although I suppose it might be a bit more convenient for out-of-tree
>> panel drivers to just assume fixed panel if these fxns are null.

Yeah, I can't make my mind about null pointer. It's nice to inform with
a null pointer that the panel doesn't support the given operation, or
that it doesn't make sense, but handling the null value makes the code a
bit complex.

For example, our VENC driver doesn't know if there's a TV connected or
not, so neither true or false as connect return value makes sense there.
Or, for a DSI command mode panel, set_timings doesn't really make much
sense.

> Maybe having default functions in omapdss might be a better idea. There
> is an option of adding default functions in omap_dss_register_driver()
> (drivers/video/omap2/dss/core.c).
> 
> Tomi, do you think it's fine to add some more default panel driver funcs?

We can add default funcs. However, adding them in
omap_dss_register_driver is not so nice, as it makes the omap_dss_driver
(which is essentially an "ops" struct) non-constable. Instead, we should
move the setting of the default funcs to the drivers.

If I'm not mistaken, we could manage that with a helper macro. Assigning
a value to the same field of a struct should be ok by the compiler, and
should cause only the last assignment to be taken into use. So:

static struct foo zab = {
	.bar = 123, /* ignored */
	.bar = 234, /* used */
};

And so we could have:

static struct omap_dss_driver my_panel_driver = {
	OMAPDSS_DEFAULT_PANEL_OPS, /* macro for default ops */

	.enable = my_panel_enable,
	/* ... */
};

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]