Re: [PATCH v3] hwmon: Add driver for AMD family 15h processor power information

[Jean Delvare <khali@linux-fr.org>]

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@amd.com>

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