Subject: Re: CVS commit: src
To: Quentin Garnier <firstname.lastname@example.org>
From: Juan RP <email@example.com>
Date: 08/06/2006 20:26:59
On Sun, 6 Aug 2006 19:38:41 +0200
Quentin Garnier <firstname.lastname@example.org> 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)
> - 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.