RE: [PATCH v5 3/7] OMAP: DSS2: Introduce omap_channel as a omap_dss_device parameter

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

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