Does anybody have more comments on [PATCHv2] ?<br><br><div class="gmail_quote">On Thu, Oct 7, 2010 at 8:00 PM, Yong Shen <span dir="ltr"><<a href="mailto:yong.shen@linaro.org">yong.shen@linaro.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><br><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div>
> Using wp_tbl is because that it also contains information like regulator<br>
> voltage.<br>
<br>
</div>The clock code does not handle the regulators, not even in the fsl<br>
kernel.<br>
<div></div></blockquote></div><div>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. <br></div>
<div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div> </div></blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div>
> Since the regulator driver for imx51 have not been upstreamed, you<br>
> don't see any regulator operation here. Also, like you mentioned, there are<br>
> two ways to change cpufreq, by post divider or pll change. And post divider<br>
> change is a simpler way and also the only way for babbage, since cpu freq<br>
> and ddr freq are all root from the same pll on babbage and they will<br>
> interfere each other.<br>
<br>
</div>I understand what you have to do, but the way you did really looks<br>
strange. You introduce get_cpu_wp() to get the complete array of<br>
workpoints. In the cpufreq driver you have this complete array, pick<br>
the desired workpoint, pass the frequency to the clock layer which in<br>
turn calls get_cpu_wp() to get the array and loop around it to get<br>
the workpoint from the frequency again. Addionally you maintain a<br>
pointer to what the clock code thinks the current workpoint is.<br>
<br>
How about making the clock code agnostic of such workpoints and<br>
calculate the pll values and dividers for a given frequency based<br>
on the frequency. If that's to complicated you could maintain<br>
a table in the clock code. If that's not flexible enough you could<br>
pass a pointer to this table to mx51_clocks_init. This array could<br>
carry information relevant only to the clock code and only relevant<br>
to the i.MX51, In the fsl kernel struct cpu_wp is a catch-all struct<br>
for all i.MXs. The i.MX51 uses the pdf/mfi/mfd/mfn/cpu_podf fields,<br>
the i.MX35 uses the pll_reg and pdr0_reg fields and who knows what<br>
the next i.MX needs.<br></blockquote></div><div><br>I strongly agree with you about this, and it is better to letter clock calculate by itself. <br></div><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div> </div></blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div>
><br>
> > > +static struct cpufreq_frequency_table imx_freq_table[4];<br>
> ><br>
> > Three frequencies should be enough for everyone, right? This should be<br>
> > dynamically allocated like in other cpufreq drivers.<br>
> ><br>
><br>
> Yes, we only support 3 work points, which is for sure. Actually, we only<br>
> support 2 points on most mx51 chips. I supposed it is ok to use static array<br>
> here.<br>
<br>
</div>Please just add dynamic allocation, then we are safe for any potential<br>
future use.<br><br></blockquote></div><div>Ok! sorry for misunderstanding.<br><br>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.<br>
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. :)<br>Thanks for comments.<br>
<br>Yong<br></div></div>
</blockquote></div><br>