Does anybody have more comments on [PATCHv2] ? On Thu, Oct 7, 2010 at 8:00 PM, Yong Shen <yong.s...@linaro.org> wrote:
> > > Using wp_tbl is because that it also contains information like regulator >> > voltage. >> >> The clock code does not handle the regulators, not even in the fsl >> kernel. >> > I did not mean to say clock code will handle regulators. I mean this table > also contains such information which may be needed in other freescale > drivers, like dvfs and busfreq. > >> >> > > Since the regulator driver for imx51 have not been upstreamed, you >> > don't see any regulator operation here. Also, like you mentioned, there >> are >> > two ways to change cpufreq, by post divider or pll change. And post >> divider >> > change is a simpler way and also the only way for babbage, since cpu >> freq >> > and ddr freq are all root from the same pll on babbage and they will >> > interfere each other. >> >> I understand what you have to do, but the way you did really looks >> strange. You introduce get_cpu_wp() to get the complete array of >> workpoints. In the cpufreq driver you have this complete array, pick >> the desired workpoint, pass the frequency to the clock layer which in >> turn calls get_cpu_wp() to get the array and loop around it to get >> the workpoint from the frequency again. Addionally you maintain a >> pointer to what the clock code thinks the current workpoint is. >> >> How about making the clock code agnostic of such workpoints and >> calculate the pll values and dividers for a given frequency based >> on the frequency. If that's to complicated you could maintain >> a table in the clock code. If that's not flexible enough you could >> pass a pointer to this table to mx51_clocks_init. This array could >> carry information relevant only to the clock code and only relevant >> to the i.MX51, In the fsl kernel struct cpu_wp is a catch-all struct >> for all i.MXs. The i.MX51 uses the pdf/mfi/mfd/mfn/cpu_podf fields, >> the i.MX35 uses the pll_reg and pdr0_reg fields and who knows what >> the next i.MX needs. >> > > I strongly agree with you about this, and it is better to letter clock > calculate by itself. > >> >> > > >> > > > +static struct cpufreq_frequency_table imx_freq_table[4]; >> > > >> > > Three frequencies should be enough for everyone, right? This should be >> > > dynamically allocated like in other cpufreq drivers. >> > > >> > >> > Yes, we only support 3 work points, which is for sure. Actually, we only >> > support 2 points on most mx51 chips. I supposed it is ok to use static >> array >> > here. >> >> Please just add dynamic allocation, then we are safe for any potential >> future use. >> >> Ok! sorry for misunderstanding. > > Here, I am not trying to find excuse for the cpufreq driver designer which > is my colleague, however the truth is that since in our company the schedule > is tight for all the tasks, that's why we always code with the first rough > idea in the head, and having less time to think about the flexibility and > other good conventions in programming. You can see that even freescale code > upstreaming is done by people out of freescale, because of we lacking of > resource. > Therefore, I would say we did not mean to have some short-sight design, we > also want to achieve a better life of coding like you guys are doing, which > is my direct feeling while I am in Linaro. :) > Thanks for comments. > > Yong >
_______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
