Hi Thomas, > As I mention, struct mgag200_pll_values should store the PLL values. But the > individual compute and set functions for each hardware revision mess this up > completely. Sometimes they use 'function values' sometimes they use > 'register values'. If you'd try to debug a failed modeset operation and have > to look at the PLL, the stored values would be meaningless, because there's > simply no logic behind it. So this is the reason for this chage - and then it makes perferct sense to do it. Without this explanation is was to my eay useless chrunch, but this explanation makes sense. > > The purpose of this patch is to make all compute functions store function > values in the struct and make all update function compute the register > values internally. > > Do you think the change would be easier to understand if I change the > original _set_plls() functions *before* splitting them into compute and > update steps? Na, this would still be very difficult to track down. The only way I would trust myself doing a proper review would be do code it myself and compare the final result. Alas, I am not going to do this. I will take a look again when you post the next revision, and unless I stumble on something I can ack the code as in I have at least looked at all the code changes. That should be enough to have it committed and the time will tell if there is some fall-out. Sam
