"Menon, Nishanth" <nm@xxxxxx> writes: [...] >> >>> +/** >> >>> + * struct omap_opp_def - OMAP OPP Definition >> >>> + * @enabled: True/false - is this OPP enabled/disabled by default >> >>> + * @freq: Frequency in hertz corresponding to this OPP >> >>> + * @u_volt: Nominal voltage in microvolts corresponding to this OPP >> >>> + * >> >>> + * OMAP SOCs have a standard set of tuples consisting of frequency >> and voltage >> >>> + * pairs that the device will support per voltage domain. This is >> called >> >>> + * Operating Points or OPP. The actual definitions of OMAP Operating >> Points >> >>> + * varies over silicon within the same family of devices. For a >> specific >> >>> + * domain, you can have a set of {frequency, voltage} pairs and this >> is denoted >> >>> + * by an array of omap_opp_def. As the kernel boots and more >> information is >> >>> + * available, a set of these are activated based on the precise >> nature of >> >>> + * device the kernel boots up on. It is interesting to remember that >> each IP >> >>> + * which belongs to a voltage domain may define their own set of OPPs >> on top >> >>> + * of this - but this is handled by the appropriate driver. >> >>> + */ >> >>> +struct omap_opp_def { >> >>> + bool enabled; >> >>> + unsigned long freq; >> >>> + u32 u_volt; >> > Comment to self: I should really make the u32 as unsigned long to be >> > in sync with what is used elsewhere..(get_voltage) >> > >> >>> +}; >> >> >> >> See above comment on 'struct omap_opp'. I think these two should be >> >> combined. >> >> >> >> I think the initial intent of having them separated so that the >> >> internal struct of 'struct omap_opp' could eventually move to the C >> >> file was the original intent, but I think it aids readability to just >> >> have a single OPP struct. >> > >> > In a few weeks we wont have the struct omap_opp exposed out(once all >> > the cleanups are done).. at that point, how would one define an OPP >> > and expect to get an handle which they cannot manipulate? >> >> Understood. But, when we get to that point, we'll have a 'struct >> omap_opp' and a 'struct omap_opp_def' which are identical. > Is'nt this a temporary status? Once we get there, here is how it might look like: > > Omap_opp as a list: > Voltage > frequency > pointer to next > OR: > Omap_opp might be a flexible array > > OR > Omap_opp might be something altogether different like a hash table or something. > > Omap_def wont change. Good point. >> Personally, I'd rather just keep a single struct defined in the >> header. > I think that is an invitation for something slipping through.. esp in private trees.. and end up with variant code bases. > >> >> Since this is core OMAP code, I'm not too concerned (anymore) about >> direct manipulation of OPP structs since I will NAK any such changes >> anyways. > > Trouble I will face is in private trees and incapability of those patches > Being send back upstream if we allow direct accesses (I know this will not > Prevent people from exposing omap_opp and then doing what they please in > Private trees, but why make it easy?). Agreed. >> >> Primarily, I see having two different structs for essentially the same >> thing being a readability issue. > It is not. It is meant for two different things: > a) Def: register the OPPs that the configuration has. > b) omap_opp: opp query/operation -> it can be whatever we choose it to be. > > These two can independently be modified without constraints to either. OK, I'm convinced. No further objections your honor. ;) [...] >> >> Understood, but I still prefer without the bulk add, but again it's a >> very minor issue to me and I'll leave it to you to make the final call. > > Going with bulk add if there are no further disapprovals from others. > ok Kevin -- 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