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

From: Jean Delvare
Date: Wed Apr 06 2011 - 11:35:58 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/