On Sun, 6 Aug 2006 19:38:41 +0200
Quentin Garnier <cube@cubidou.net> wrote:

> Hmm, why do I get thanked?  Sure I made a few comments after a quick
> glance at the code but you ignored them apparently.
> 
> I expect this to break on EM64T, but I told you that already...

Can you please explain why EM64T won't work?
 
> Also, why didn't you try adding support for that on i386, too?  I
> haven't seen anything in the spec that restricts Cool'n'Quiet to long
> mode.  And I still feel bitmask_snprintf would be more suitable than
> your own equivalent.

Do we want to have another i386/i386/powernow_k8.c? my goal is
to share most of the code between drivers.

You are right that bitmask_snprintf was made for this kind of things,
but it's not very critical.

> Some other comments:
> 
>  - k8_powernow_setperf could return meaningful errors (e.g., EBUSY in
>    the PN8_STA_PENDING(status) case), which would then be returned by
>    the sysctl handler

Maybe in the future...

>  - you don't free freq_names if k8pnow_current_state is NULL (what if
>    the PSB doesn't list the current state?  you know vendors...  I
>    certainly expect one to ship a laptop with a broken PSB but a correct
>    ACPI table)

Fixed.
 
>  - the spec indicates that FID and VID changes of a dual-core processor
>    must me co-ordinated.

I'll take a look at linux driver.

> Glad I could help.  Please have your code reviewed on tech-kern before
> you commit.

Yes, I try to do that... but sometimes you wait for a review that never happens.