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:
signature.asc
Description: OpenPGP digital signature
