On Mon, 2010-08-02 at 14:05 +0200, ext Taneja, Archit wrote: > Hi, > > > On Tue, 2010-07-27 at 10:27 +0200, ext Taneja, Archit wrote: > > > Hi, > > > > > > We had pushed three patch series while you were out. We > > received some > > > good comments on them. We wanted to know if you can point out more > > > fixes and susuggestions before we rework them. > > > > > > https://patchwork.kernel.org/patch/111901/ > > > > > > https://patchwork.kernel.org/patch/112648/ > > > > > > https://patchwork.kernel.org/patch/112653/ > > > > Do you have updated patches for these? It would be easier to > > review new ones than the old ones. > > > > Tomi > > The comments on the existing patches have been mostly related > to minimizing the number of patches in each series, moving some > #defines to header files and one or two typos. > > I haven't reworked these patches yet, I wanted to know if you > could review the changes you had suggested on the v1 of the series > below: I think the register handling looks much better now. Although defines like this: +#define DISPC_DEFAULT_COLOR(ch) DISPC_REG(ch == OMAP_DSS_CHANNEL_LCD \ + ? (0x004C) : (ch == OMAP_DSS_CHANNEL_DIGIT ? \ + (0x0050) : (0x03AC))) makes me wonder if a separate lookup table would be cleaner. But perhaps it's not too bad yet. Also, we should think how to reduce if (cpu_is_omap44xx()) lines. There should be some kind of DSS capability list somewhere, which would tell the features available. I haven't thought this more, but it'd be very nice if we could use the DSS HW version number to decide what features there are. However, TI answered that information about DSS HW version numbers is not available, and thus cannot be used =(. Perhaps you could try to dig out some information from inside TI? And, as others have commented, consider how to split the patches. The most important rule is that every stage in the patch set has to compile. One thing that I personally think makes patch sets clearer, is to divide a new functionality into two parts: 1) modify the old code, without adding new functionality, so that the new functionality is easier to add 2) add the new functionality. As an example, if we look at the new DISPC registers patch: There could first be a patch that introduces DISPC_XXX(channel) style registers (even if there's only one channel available) and modifies the functions accordingly. The second patch would introduce OMAP4 registers and whatever new functionality these registers require. This way it would be easy to study and test the first patch, as it shouldn't introduce any new functionality, but just "shuffles the code around". And it would be easy to study the second patch, as the needed base functionality is already there, and there's lot less new stuff added. Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
