On Tue, May 11, 2010 at 7:13 PM, Nishanth Menon <nm@xxxxxx> wrote: > Venkatraman S had written, on 05/11/2010 07:19 AM, the following: >> >> Nishanth Menon <menon.nishanth@xxxxxxxxx> wrote: >>> >>> -linux-arm >>> >>> On 05/10/2010 10:53 PM, S, Venkatraman wrote: >>>>> >>>>> -----Original Message----- >>>>> From: menon.nishanth@xxxxxxxxx >>>>> [mailto:menon.nishanth@xxxxxxxxx] On Behalf Of Menon, Nishanth >>>>> Sent: Tuesday, May 11, 2010 5:02 AM >>>>> To: S, Venkatraman >>>>> Cc: linux-omap@xxxxxxxxxxxxxxx; >>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Tony Lindgren; >>>>> Chikkature Rajashekar, Madhusudhan >>>>> Subject: Re: [PATCH RESEND] update omap3 features bitmap and >>>>> API to generic names >>>>> >>>>> On Mon, May 10, 2010 at 2:55 PM, Venkatraman S >>>>> <svenkatr@xxxxxx> wrote: >>>>>> >>>>>> OMAP3 features bitmap is used a method to check for SoC >>>>>> specific features. This patch renames the global variables and the >>>>>> accessor functions to use a generic name from omap3_* to >>>>>> omap_* >>>>>> >>>>>> Signed-off-by: Venkatraman S<svenkatr@xxxxxx> >>>>>> CC: Nishant Menon<nm@xxxxxx> >>>>> >>>>> Just for the patchworks >>>>> NAK - discussed before - >>>>> http://marc.info/?l=linux-omap&m=127349696800651&w=2 >>>> >>>> This patch doesn't have the descriptor load changes, and just the >>>> rename. >>>> Did you take a look at it first? >>> >>> Arrgh - my bad - there was no reference to previous discussions or >>> anything >>> in this patch, please do add references in diffstat section if you are >>> refering to a previous discussed strategy to help the reviewers >>> differentiate and understand you better. >>> >>> overall this still opens up a question for me -> can we blindly replace >>> omap3-features with omap features? how do we think of omap1,2,3,4 are >>> handled consistently with a 32 bit field? >>> >>> I agree on the need to have a omap independent field, but is >>> omap3_features >>> the one we need to modify OR should we be introducing a new field? >>> >>> my vote goes for introducing a new field. >>> >> >> You are confusing the interface with implementation (or rather, >> worrying about both of them simultaneously). >> >> The interface has to be consistent and SoC independent, to the extent >> possible. >> So the macro changes are relevant and readable / extensible. >> >> Regarding the variable name (implementation):- >> Given the minimal set that we have (5 -6 fields today, so there is >> room for 25 more 'features" still), I don't think >> we should over-engineer it now to accomodate all permutations and use >> cases. It's not even found use >> beyond 1 or 2 files. YAGNI ? > > huh? I dont understand you. lets see what we we are doing here: > a) we are causing an ABI breakage by moving omap3_feature to omap_feature The omap3_feature variable is not supposed to be used by modules *querying* for the feature. They don't even have to know it's existence. That is what the macros/inline functions omap_has_***() are for. All the uses of omap3_has_** have been replaced by omap_has_** in this patch. This is an ABI change (due to the previous interface tying it to OMAP3) and *not* ABI breakage (All previous uses have been replaced) > (which by the way should also include changes to the macros as well..) This is different. There could be different ways in which the actual snooping of whether a feature exists on not is implemented. For OMAP3, it is looking up the control module. (This, I have preserved). For other SoCs, it could be different, I don't know how to snoop them for. Your comments are welcome. > b) we have a real need to have a cross OMAP feature distinction to > differentiate between generic omap feature and omap3/2/1/4 features. > > This patch does not address the same in a consistent scalable way. NOTE: we > are starting to introduce other OMAP4 features as well, SOC level feature > distinction should ideally be handled by FEATURE framework - that is the > reason it was introduced in the first place. I never claimed this patch to be a mother of all 'features framework'. It was just a name improvement to the bunch of macros that existed. Again, a) it preserves the separtion of interface. We can have a full fledged tree of generic / OMAP[12345] specific features, but the interface need not change. > > if your feel this is overengineering - well, I believe this is conceptual > definition -> Specific OMAP[1234] *family* features Vs OMAP generic > features.. two different things in my mind -> it does not matter if I have > 32 bits in the original variable OR if I had 64bit.. i dont prefer to reuse > a variable and mess up the conceptual distinction. > b) Are you simply saying that the existing features bitmap (all 6 of them) are tied to OMAP3 ? For good ? In that case, I really understand your comments and can change back the variable name. At the same time, it's not easy to classify what category a feature belongs to. What could a single SoC specific feature today (SDMA descriptor loading) could then become a series (OMAP4, OMAP5 etc). Or it could occur at disjoint intervals (OMAP3 and OMAP5). Even if it somehow could be boxed to a series, I think the interface would then be tied to that series, which would break future expansion. So a *flat* structure of "uniquely named" features storage is what we want, which eventually could flow over to an array of bitfields. In other words, if the interface is defined as omapX_has_feature(), it is not maintainable, for any X. It should always be omap_has_feature(), even if it is found in a single SoC. This is what I understand your intention in change of $SUBJECT. If you have another proposal, please post it here and I can work with you on that. Regards, Venkat. -- 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
