Re: [PATCH] v3 of IBM power meter driver

["Darrick J. Wong" <djwong@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3296 bytes --]

On Tue, Sep 11, 2007 at 09:23:35AM -0400, Mark M. Hoffman wrote:

> I am not an IPMI expert, so I would appreciate getting an Acked-by from
> someone who knows more about that subsystem.
> 
> Anyway, some comments are below.  This is nowhere near a complete review yet.

Thank you for the review!  Comments interspersed below, though for
brevity the one-liners have been fixed.

> > +config SENSORS_IBMPEX
> > +	tristate "IBM PowerExecutive temperature/power sensors"
> > +	depends on IPMI_SI
> 
> Open question: can we use "select" here?  As written, it took some hunting to
> even get this driver to show up as an option in menuconfig.

Changed, since it seems reasonable that someone looking for PEx support
might not necessarily know that it is based upon IPMI.

> > +struct ibmpex_bmc_data {
> > +	struct list_head	list;
> > +	struct class_device	*class_dev;
> 
> My current stack of patches includes one which requires that this be changed
> to 'struct device *hwmon_dev', as 'struct class_device' is going away soon.
> You may rebase on my testing tree[1], or else I will just follow up with a
> patch to fix this up after I eventually merge yours.
> 
> [1] http://lm-sensors.org/kernel?p=kernel/mhoffman/hwmon-2.6.git;a=shortlog;h=testing

Done.

> > +static ssize_t ibmpex_show_sensor(struct device *dev,
> > +				  struct device_attribute *devattr,
> > +				  char *buf)
> > +{
> > +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +	int iface = PEX_INTERFACE(attr->index);
> > +	int sensor = PEX_SENSOR(attr->index);
> > +	int func = PEX_FUNC(attr->index);
> > +	struct ibmpex_bmc_data *data = get_bmc_data(iface);
> 
> ... especially given how many times you're going to call it.  Is there any
> reason you can't use the driver_data field of struct device *dev for that?

I can (and did) update the code to use dev_get/set_drvdata for the
accessors.  However, the "iface" field exists as a mechanism to map
interface numbers to struct ibmpex_bmc_data/struct device data because
the callback that IPMI uses to notify clients that BMCs are going away
only passes the interface number, not the struct device itself.
Unfortunately, this means that get_bmc_data() must remain, but now it is
only used once at the end of life.

> E.g. i2c based hwmon drivers do this at some point during the probe:
> 
> 	i2c_set_clientdata(new_client, data);
> 
> (which becomes)
> 
> 	dev_set_drvdata(&new_client->dev, data);
> 
> If you could do that, then you no longer need 'iface' at all in the function
> above... *that* may allow you to use the SENSOR_ATTR_2 mechanism from
> hwmon-sysfs.h - much easier to read than the manual number packing for 'sensor'
> and 'func'.

Doesn't look too hard; I'll have a go at it and see how it does.

> > +	err = ibmpex_query_sensor_count(data);
> > +	if (err < 0)
> > +		return -ENOENT;
> > +	data->num_sensors = err;
> > +
> 
> Did you mean 'if (err <= 0)' ?

Yes.

> > +		/* Create attributes */
> > +		for (j = 0; j < PEX_NUM_SENSOR_FUNCS; j++)
> > +			if (create_sensor(data, sensor_type, sensor_counter,
> > +					  i, j))
> 
> Why not 'err = create_sensor(...)' and propagate the actual error here?

Rough draft syndrome?  'tis fixed. :)

--D

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]