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/ |
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
