Hi Andreas,
On Wed, 6 Apr 2011 15:54:24 +0200, Andreas Herrmann wrote:
> --- /dev/null
> +++ b/Documentation/hwmon/f15h_power
> @@ -0,0 +1,37 @@
> +Kernel driver f15h_power
> +========================
> +
> +Supported chips:
> +* AMD Family 15h Processors
> +
> + Prefix: 'f15h_power'
> + Addresses scanned: PCI space
> + Datasheets:
> + BIOS and Kernel Developer's Guide (BKDG) For AMD Family 15h Processors
> + (not yet published)
> +
> +Author: Andreas Herrmann <andreas.herrmann3@xxxxxxx>
BTW, please consider adding an entry in MAINTAINERS.
> +
> +Description
> +-----------
> +
> +This driver permits reading of registers providing power information
> +of AMD Family 15h processors.
> +
> +For AMD Family 15h processors the following power values can be
> +calculated using different processor northbridge function registers
A trailing ":" would be nice.
> +
> +* BasePwrWatts: Specifies in watts the maximum amount of power
> + consumed by the processor for NB and logic external to the core.
> +* ProcessorPwrWatts: Specifies in watts the maximum amount of power
> + the processor can support.
> +* CurrPwrWatts: Specifies in watts the current amount of power being
> + consumed by the processor.
> +
> +This driver provides ProcessorPwrWatts and CurrPwrWatts:
> +* power1_max (ProcessorPwrWatts)
I see you changed this name once already, but... What is expected to
happen if the power consumed exceeds this limit? If you expect the CPU
to get damaged, then power1_crit would be more appropriate.
Another way to decide if _max or _crit is more appropriate is by
answering the question: if a second limit was added in the future,
would it more likely be below or above this one?
> +* power1_input (CurrPwrWatts)
> +
> +On multi-node processors the calculated value is for the entire
> +package and not for a single node. Thus the driver creates sysfs
> +attributes only for internal node0 of a multi-node processor.
> (...)
> + return sprintf(buf, "%u\n", (u32) ptdp);
Maybe I'm just nitpicking, but I still don't think it's correct. %u
wants unsigned int, not u32. It might be the same but that's by luck.
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -205,7 +205,7 @@ static void __devexit k10temp_remove(struct pci_dev *pdev)
> dev_set_drvdata(&pdev->dev, NULL);
> }
>
> -static const struct pci_device_id k10temp_id_table[] = {
> +static DEFINE_PCI_DEVICE_TABLE(k10temp_id_table) = {
> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_11H_NB_MISC) },
> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_CNB17H_F3) },
Such cleanups should go to separate patches, as they don't have
anything to do with your initial effort. And that way you can fix
k8temp i5k_amb too.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
