Messages in this thread | | | Date | Wed, 6 Apr 2011 17:35:43 +0200 | From | Jean Delvare <> | Subject | Re: [PATCH v2] hwmon: Add driver for AMD family 15h processor power information |
| |
On Wed, 6 Apr 2011 17:19:01 +0200, Andreas Herrmann wrote: > On Wed, Apr 06, 2011 at 04:14:01PM +0200, Jean Delvare wrote: > > On Tue, 5 Apr 2011 16:45:36 +0200, Andreas Herrmann wrote: > > > +static ssize_t show_power(struct device *dev, > > > + struct device_attribute *attr, char *buf) > > > +{ > > > + u32 val, btdp, tdpl, tdp2w, arange; > > > + s32 acap; > > > + u64 ctdp; > > > > These variable names aren't easy to understand. > > Just random names which eventually map to the spec: > > btdp - base_tdp > tdpl - tdp_limit > tdp2w - tdp_to_watt > acap - average_accumulator_capture (or even worse how about "processor_tdp_running_average_accumulator":( > arange - average_range
avg_cap and avg_range would do, respectively, for the last two.
> I don't think that changing the names make it much easier to > reconstruct the calculation but if you insist in changing it I'll > adapt it.
I do prefer the "extended" names, really. Sure, this doesn't change the calculations, but it helps the reader understand what's going on. Which will be useful if one ever has to fix a bug in the code or extend it for a different CPU family.
But maybe this is just me. Guenter, do you have an opinion?
> > > (...) > > > + dev_set_drvdata(&pdev->dev, hwmon_dev); > > > > pci_set_drvdata() > > Are you sure?
Yes, I'm sure. This function exists, so there is no reason not to use it.
> No single hwmon driver is using this wrapper around dev_set_drvdata() so far. (Or I just missed it.)
$ grep i2c_set_clientdata drivers/hwmon/*.c | wc -l 67 $ grep spi_set_drvdata drivers/hwmon/*.c | wc -l 2 $ grep platform_set_drvdata drivers/hwmon/*.c | wc -l 66 $
> At the moment only dev_set_drvdata() is used by > > drivers/hwmon/adcxx.c > drivers/hwmon/lm70.c > drivers/hwmon/ultra45_env.c > drivers/hwmon/ibmpex.c > drivers/hwmon/k10temp.c > drivers/hwmon/ibmaem.c > drivers/hwmon/k8temp.c
Most of these (at least spi and pci drivers) should be fixed. I'll send a patch later.
-- Jean Delvare
| |