[PATCH 06/12] drm/mgag200: Store values (not bits) in struct mgag200_pll_values

Sam Ravnborg sam at ravnborg.org
Mon Jul 12 14:18:27 UTC 2021


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


More information about the dri-devel mailing list