[lm-sensors] [PATCH] hwmon: add driver for ADT7411

Wolfram Sang w.sang at pengutronix.de
Thu Jan 21 16:45:02 CET 2010


Hi Jean,

some questions arouse:

> > +static int __devinit adt7411_probe(struct i2c_client *client,
> > +				   const struct i2c_device_id *id)
> > +{
> > +	int val;
> > +	struct adt7411_data *data;
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > +		return -ENODEV;
> > +
> > +	val = i2c_smbus_read_byte_data(client, ADT7411_REG_MANUFACTURE_ID);
> > +	if (val < 0 || val != ADT7411_MANUFACTURE_ID) {
> > +		dev_err(&client->dev, "Wrong manufacturer ID. Got %d, "
> > +			"expected %d\n", val, ADT7411_MANUFACTURE_ID);
> > +		return -ENODEV;
> > +	}
> > +
> > +	val = i2c_smbus_read_byte_data(client, ADT7411_REG_DEVICE_ID);
> > +	if (val < 0 || val != ADT7411_DEVICE_ID) {
> > +		dev_err(&client->dev, "Wrong device ID. Got %d, "
> > +			"expected %d\n", val, ADT7411_DEVICE_ID);
> > +		return -ENODEV;
> > +	}
> 
> This is the wrong place for this check. Device identification goes into
> a .detect() function. By the time .probe() is called, you already know
> what device is there.

Just came to think of it: That essentially kills all sanity checks for platform
devices (or static board_infos), or am I wrong? Would it be an idea to call
detect() for the address in the board_info, or are board_infos considered per
se correct?

> > +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&data->lsb_msb_lock);
> > +
> > +	data->config[0] = ADT7411_CFG1_START_MONITOR | ADT7411_CFG1_RESERVED1;
> > +	data->config[1] = ADT7411_CFG2_ENABLE_SMBUS_TO;
> > +	data->config[2] = ADT7411_CFG3_RESERVED1 | ADT7411_CFG3_REF_VDD;
> > +
> > +	i2c_smbus_write_byte_data(client, ADT7411_REG_CFG2, ADT7411_CFG2_RESET);
> > +	i2c_smbus_write_byte_data(client, ADT7411_REG_CFG2, data->config[1]);
> > +	i2c_smbus_write_byte_data(client, ADT7411_REG_CFG3, data->config[2]);
> > +	i2c_smbus_write_byte_data(client, ADT7411_REG_CFG1, data->config[0]);
> 
> This is bad practice. The BIOS/firmware might have setup the chip
> already. Resetting and then overwriting this configuration is bad. If
> you want to enforce a specific setup, this is a platform decision and
> you should provide the initialization data as platform data.

I assume ORing ADT7411_CFG1_START_MONITOR is OK, though? What about
ADT7411_CFG2_ENABLE_SMBUS_TO (SMBus Timeout)? Is this a user preference or a
driver trying to keep his bus alive?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20100121/87cf3d53/attachment.sig>


More information about the lm-sensors mailing list