On Thu, 2010-12-02 at 13:27 +0530, ext Taneja, Archit wrote:
> Hi,
>
> Tomi Valkeinen wrote:
> > Hi,
> >
> > - The files are getting quite crowded with code that checks
> > for the channel and then do the work with bits/irqs depending
> > on the channel.
> > This makes the code a bit difficult to read. I don't have any
> > clear ideas right now how to make it clearer, but some
> > methods to generalize these kinds of functions would be nice.
> > But this is not so important for the time being, and we can improve it
> > later.
>
> I am assuming that you are referring to 'DISPC_IRQ_MASK_ERROR' and the
> registering/unregistering of
> irq masks in manager.c
Not only the irqs, but also, for example, code like this:
if (channel == OMAP_DSS_CHANNEL_LCD ||
channel == OMAP_DSS_CHANNEL_LCD2)
bit = 5; /* GOLCD */
else
bit = 6; /* GODIGIT */
if (channel == OMAP_DSS_CHANNEL_LCD2)
return REG_GET(DISPC_CONTROL2, bit, bit) == 1;
else
return REG_GET(DISPC_CONTROL, bit, bit) == 1;
So lots of the functions contain ifs based on the channel. I don't know
right now what would be the best way to make the code cleaner (probably
some kinds of look-up tables, but they incur some overhead), and as I
said, it's cosmetical and can be cleaned up later (presuming we come up
with a good way).
> I guess we could have a dss_features function which could return the mask
> based on what omap
> it is. But this way the mask values/contents would be totally invisble in
> manager.c and dispc.c, one
> would need to check it in dss_features.c, which also isn't readable.
Right. I agree that that solution isn't perhaps the best one either.
> One thing which I would like to add is that this series doesn't need to touch
> any board file for now.
> The present dss_recheck_connections() doesn't try to differentiate between
> LCD and DIGIT channels, it just
> uses 'omap_display_type' to differentiate between them. Only a panel which
> needs to connect to LCD2 has to do this,
> This method wouldn't have worked if we didn't switch to uniform use of
> dssdev->manager->id instead of
> dssdev->channel. We will need to change dss_recheck_connections() in the
> future to make it more uniform.
Ok.
If you can split this patch into the two parts I suggested (if that's ok
for you, you didn't comment on that one), and check if there's anything
to add to the commit descriptions, I think we can go and apply this
patch set.
Btw, on what platforms have you tested this (or generally any patches
you send?). I only have 3430SDP currently that I can easily use to test,
so my testing is a bit limited.
Tomi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
