Hi, On Mon, Sep 07, 2009 at 11:33:32AM +0530, Premi, Sanjeev wrote: > > What's the purpose of most of these checks in the first place? I can > > see two immediate needs: > > > > 1) To check for various errata and do appropriate workarounds > > [sp] I believe the only need would be to make easy check if the version > has multiple errata fixes and/or enhancements. And, of course, a > verbose information to user who may not have (and may not need) > information of all the errata/enhancements. > > Today, there are multiple ways of doing the same thing... Each way > builds upon minor issues in the existing one; but adds its own. Yeah, ok. I was just fearing that this added ES-level checking would be used to clutter up the code with extra testing instead of hiding it away in header files. But it seems like most of it is covered already. > > 2) To check if the current chip has a certain feature > > > > Both of these could just as well be abstracted away such that you use > > tests on the form: > > > > if (OMAP_HAS_ERRATA_FOO) ... > > > > or: > > if (OMAP_FEATURE_FOO) ... > > > > And then move the actual checking of a feature into the header file > > where the errata/feature setups are defined. > > [sp I have submitted a patch that takes the first step toward this: > http://marc.info/?l=linux-omap&m=125050987112798&w=2 > ...still waiting to hear from Tony on this. Looks good. I didn't subscribe to linux-omap until recently so I don't have the original emails (and thus can't comment on them), but my only feedback is that it really doesn't belong in /proc/cpuinfo whether a SoC has SGX on-chip or not. It'd be better to move that information somewhere else. Keep in mind that adding information to a /proc file means that you are changing an API that we would need to live with forever. > > There's two major benefits to this: > > > > 1) Readability. No need to sit and look up in a manual why there's a > > check for version X here. > > (and/or no need to add a specific comment about it). > > > > 2) Keeping changes centralized. If there's a new revision or chip, > > there's just one header file to update, not 20 different source files. > > > > For example, a bunch of the checks in pm34xx.c would be nicer > > to have as: > > > > if (OMAP_HAS_USBHOST()) > > [sp] Can you look and comment on this discussion as well: > http://marc.info/?l=linux-omap&m=125017671720718&w=2 I think it is a great step in the right direction. My only concern is that the usage conventions are confusing: OMAP3_HAS_FEATURE(neon, NEON) Just seeing that, what is the difference between neon and NEON? Also, it always requires two consecutive calls to check for a feature (get the flags, then check the mask). I would personally prefer to keep a global that is initialized early in boot (called __omap_features or so -- the __-prefix would indicate that it is not for direct use in code), then have OMAP3_HAS_FEATURE() just look at that value instead of having it passed in. It's not like it changes during runtime anyway. It can be debated if the format should then be OMAP3_HAS_FEATURE_SGX or OMAP3_HAS_FEATURE(SGX), but either is fine with me. -Olof -- 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
