Hi Wolfgang, On Sat, 13 Sep 2008 10:55:22 +0200, Wolfgang Grandegger wrote: > This patch modifies the w83781d driver to use new style driver binding. > Substantial code modifications are required to deal with the new > interface, especially legacy device detection. > > Signed-off-by: Wolfgang Grandegger <wg at grandegger.com> > --- > drivers/hwmon/w83781d.c | 615 +++++++++++++++++++++--------------------------- > 1 file changed, 278 insertions(+), 337 deletions(-) Ouch. Once again, that's a very large patch. I did the exact same conversion for the lm78 driver a few weeks ago: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-hwmon/hwmon-lm78-04-dont-abuse-i2c_client.patch http://khali.linux-fr.org/devel/linux-2.6/jdelvare-hwmon/hwmon-lm78-05-convert-to-new-style-i2c.patch The two patches sum up to 238 lines changed. Compare to 615 for your patch... I know that the w83781d driver is a little more complex but that doesn't explain all the difference. I found that moving w83781d_detect(), w83781d_probe(), w83781d_remove() where w83781d_detect() and w83781d_detach_client() were originally, and reverting a couple of unrelated cleanups, lowers the count to 400, which will make the patch way easier to review for me. So, I will be commenting on that variant of the patch instead of yours. > --- linux-2.6.27-rc8.orig/drivers/hwmon/w83781d.c 2008-10-06 16:22:19.000000000 +0200 > +++ linux-2.6.27-rc8/drivers/hwmon/w83781d.c 2008-10-06 17:20:56.000000000 +0200 > @@ -209,7 +209,7 @@ DIV_TO_REG(long val, enum chips type) > /* For ISA chips, we abuse the i2c_client addr and name fields. We also use > the driver field to differentiate between I2C and ISA chips. */ This comment and the one above it can be deleted. > struct w83781d_data { > - struct i2c_client client; > + struct i2c_client *client; > struct device *hwmon_dev; > struct mutex lock; > enum chips type; This isn't sufficient. Now, ISA devices no longer have a struct i2c_client to abuse, so they need a couple fields to store the device name and address. See: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-hwmon/hwmon-lm78-04-dont-abuse-i2c_client.patch for how I handled that in the lm78 driver. It might help to do the same kind of preliminary patch for the w83781d driver as well. > @@ -244,26 +244,15 @@ struct w83781d_data { > u8 vrm; > }; > > -static struct w83781d_data *w83781d_data_if_isa(void); > -static int w83781d_alias_detect(struct i2c_client *client, u8 chipid); > - > -static int w83781d_attach_adapter(struct i2c_adapter *adapter); > -static int w83781d_detect(struct i2c_adapter *adapter, int address, int kind); > -static int w83781d_detach_client(struct i2c_client *client); > +static int w83781d_read_value_i2c(struct i2c_client *client, u16 reg); > +static int w83781d_write_value_i2c(struct i2c_client *client, u16 reg, u16 value); > +static int w83781d_alias_detect(struct i2c_client *client); > > static int w83781d_read_value(struct w83781d_data *data, u16 reg); > static int w83781d_write_value(struct w83781d_data *data, u16 reg, u16 value); > static struct w83781d_data *w83781d_update_device(struct device *dev); > static void w83781d_init_device(struct device *dev); > > -static struct i2c_driver w83781d_driver = { > - .driver = { > - .name = "w83781d", > - }, > - .attach_adapter = w83781d_attach_adapter, > - .detach_client = w83781d_detach_client, > -}; > - > /* following are the sysfs callback functions */ > #define show_in_reg(reg) \ > static ssize_t show_##reg (struct device *dev, struct device_attribute *da, \ > @@ -825,46 +814,16 @@ static SENSOR_DEVICE_ATTR(temp2_type, S_ > static SENSOR_DEVICE_ATTR(temp3_type, S_IRUGO | S_IWUSR, > show_sensor, store_sensor, 2); > > -/* This function is called when: > - * w83781d_driver is inserted (when this module is loaded), for each > - available adapter > - * when a new adapter is inserted (and w83781d_driver is still present) > - We block updates of the ISA device to minimize the risk of concurrent > - access to the same W83781D chip through different interfaces. */ > -static int > -w83781d_attach_adapter(struct i2c_adapter *adapter) > -{ > - struct w83781d_data *data = w83781d_data_if_isa(); > - int err; > - > - if (!(adapter->class & I2C_CLASS_HWMON)) > - return 0; > - > - if (data) > - mutex_lock(&data->update_lock); > - err = i2c_probe(adapter, &addr_data, w83781d_detect); > - if (data) > - mutex_unlock(&data->update_lock); > - return err; > -} You can't just get rid of this function. You still need to hold the ISA device mutex during I2C detection. This part must go in function w83781d_detect(). See for example: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-hwmon/hwmon-lm78-05-convert-to-new-style-i2c.patch > - > /* Assumes that adapter is of I2C, not ISA variety. > * OTHERWISE DON'T CALL THIS > */ > static int > -w83781d_detect_subclients(struct i2c_adapter *adapter, int address, int kind, > - struct i2c_client *new_client) > +w83781d_detect_subclients(struct i2c_client *new_client) Note: that's the kind of change that could have been done in a preliminary patch. Small incremental patches make testing and review way easier. > { > - int i, val1 = 0, id; > - int err; > - const char *client_name = ""; > struct w83781d_data *data = i2c_get_clientdata(new_client); > - > - data->lm75[0] = kzalloc(sizeof(struct i2c_client), GFP_KERNEL); > - if (!(data->lm75[0])) { > - err = -ENOMEM; > - goto ERROR_SC_0; > - } > + struct i2c_adapter *adapter = new_client->adapter; > + int i, val1 = 0, id, err; > + int address = new_client->addr; > > id = i2c_adapter_id(adapter); > > @@ -876,31 +835,39 @@ w83781d_detect_subclients(struct i2c_ada > "address %d; must be 0x48-0x4f\n", > force_subclients[i]); > err = -EINVAL; > - goto ERROR_SC_1; > + goto ERROR_SC_0; > } > } > w83781d_write_value(data, W83781D_REG_I2C_SUBADDR, > (force_subclients[2] & 0x07) | > ((force_subclients[3] & 0x07) << 4)); > - data->lm75[0]->addr = force_subclients[2]; > + data->lm75[0] = i2c_new_dummy(adapter, force_subclients[2]); > } else { > val1 = w83781d_read_value(data, W83781D_REG_I2C_SUBADDR); > - data->lm75[0]->addr = 0x48 + (val1 & 0x07); > + data->lm75[0] = i2c_new_dummy(adapter, 0x48 + (val1 & 0x07)); > + } > + if (!data->lm75[0]) { > + dev_err(&new_client->dev, > + "subclient 0 registration failed\n"); For debugging/support purposes, it might be useful to mention in the message which address was tried. For example, the asb100 driver does the following: dev_err(&client->dev, "subclient %d registration " "at address 0x%x failed.\n", 1, sc_addr[0]); It would also be good to be consistent with that driver and number the subclients 1 and 2 rather than 0 and 1. > + err = -ENOMEM; > + goto ERROR_SC_0; > } > > - if (kind != w83783s) { > - data->lm75[1] = kzalloc(sizeof(struct i2c_client), GFP_KERNEL); > - if (!(data->lm75[1])) { > + if (data->type != w83783s) { > + if (force_subclients[0] == id && > + force_subclients[1] == address) > + data->lm75[1] = > + i2c_new_dummy(adapter, force_subclients[3]); > + else > + data->lm75[1] = > + i2c_new_dummy(adapter, > + 0x48 + ((val1 >> 4) & 0x07)); > + if (!data->lm75[1]) { > + dev_err(&new_client->dev, > + "subclient 1 registration failed\n"); > err = -ENOMEM; > goto ERROR_SC_1; > } > - > - if (force_subclients[0] == id && > - force_subclients[1] == address) { > - data->lm75[1]->addr = force_subclients[3]; > - } else { > - data->lm75[1]->addr = 0x48 + ((val1 >> 4) & 0x07); > - } > if (data->lm75[0]->addr == data->lm75[1]->addr) { > dev_err(&new_client->dev, > "Duplicate addresses 0x%x for subclients.\n", > @@ -909,45 +876,13 @@ w83781d_detect_subclients(struct i2c_ada > goto ERROR_SC_2; > } > } > - > - if (kind == w83781d) > - client_name = "w83781d subclient"; > - else if (kind == w83782d) > - client_name = "w83782d subclient"; > - else if (kind == w83783s) > - client_name = "w83783s subclient"; > - else if (kind == as99127f) > - client_name = "as99127f subclient"; > - > - for (i = 0; i <= 1; i++) { > - /* store all data in w83781d */ > - i2c_set_clientdata(data->lm75[i], NULL); > - data->lm75[i]->adapter = adapter; > - data->lm75[i]->driver = &w83781d_driver; > - data->lm75[i]->flags = 0; > - strlcpy(data->lm75[i]->name, client_name, > - I2C_NAME_SIZE); > - if ((err = i2c_attach_client(data->lm75[i]))) { > - dev_err(&new_client->dev, "Subclient %d " > - "registration at address 0x%x " > - "failed.\n", i, data->lm75[i]->addr); > - if (i == 1) > - goto ERROR_SC_3; > - goto ERROR_SC_2; > - } > - if (kind == w83783s) > - break; > - } > - > return 0; > > -/* Undo inits in case of errors */ > -ERROR_SC_3: > - i2c_detach_client(data->lm75[0]); > + /* Undo inits in case of errors */ > ERROR_SC_2: > - kfree(data->lm75[1]); > + i2c_unregister_device(data->lm75[1]); > ERROR_SC_1: > - kfree(data->lm75[0]); > + i2c_unregister_device(data->lm75[0]); > ERROR_SC_0: > return err; > } > @@ -1114,87 +1049,63 @@ w83781d_create_files(struct device *dev, > return 0; > } > > +/* Return 0 if detection is successful, -ENODEV otherwise */ > static int > -w83781d_detect(struct i2c_adapter *adapter, int address, int kind) > +w83781d_detect(struct i2c_client *client, int kind, > + struct i2c_board_info *info) > { > int val1 = 0, val2; > - struct i2c_client *client; > - struct device *dev; > - struct w83781d_data *data; > - int err; > + struct i2c_adapter *adapter = client->adapter; > + int address = client->addr; > const char *client_name = ""; > enum vendor { winbond, asus } vendid; > > - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { > - err = -EINVAL; > - goto ERROR1; > - } > - > - /* OK. For now, we presume we have a valid client. We now create the > - client structure, even though we cannot fill it completely yet. > - But it allows us to access w83781d_{read,write}_value. */ > - > - if (!(data = kzalloc(sizeof(struct w83781d_data), GFP_KERNEL))) { > - err = -ENOMEM; > - goto ERROR1; > - } > - > - client = &data->client; > - i2c_set_clientdata(client, data); > - client->addr = address; > - mutex_init(&data->lock); > - client->adapter = adapter; > - client->driver = &w83781d_driver; > - dev = &client->dev; > - > - /* Now, we do the remaining detection. */ > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) > + return -ENODEV; > > /* The w8378?d may be stuck in some other bank than bank 0. This may > make reading other information impossible. Specify a force=... or > force_*=... parameter, and the Winbond will be reset to the right > bank. */ > if (kind < 0) { > - if (w83781d_read_value(data, W83781D_REG_CONFIG) & 0x80) { > + if (w83781d_read_value_i2c(client, W83781D_REG_CONFIG) & 0x80) { You are right that calling w83781d_read_value() in this detection function isn't correct, however calling w83781d_read_value_i2c() is no better. By definition, in a detection function, we don't know if the device we are accessing is ours or not. So we should only use standard i2c_smbus_read_byte_data() calls at this point. This is the best way to guarantee that we only issue read transactions to the chip. One advantage is that you no longer have to change the prototype of function w83781d_read_value_i2c(), so smaller patch. > dev_dbg(&adapter->dev, "Detection of w83781d chip " > "failed at step 3\n"); > - err = -ENODEV; > - goto ERROR2; > + return -ENODEV; > } > - val1 = w83781d_read_value(data, W83781D_REG_BANK); > - val2 = w83781d_read_value(data, W83781D_REG_CHIPMAN); > + val1 = w83781d_read_value_i2c(client, W83781D_REG_BANK); > + val2 = w83781d_read_value_i2c(client, W83781D_REG_CHIPMAN); > /* Check for Winbond or Asus ID if in bank 0 */ > if ((!(val1 & 0x07)) && > (((!(val1 & 0x80)) && (val2 != 0xa3) && (val2 != 0xc3)) > || ((val1 & 0x80) && (val2 != 0x5c) && (val2 != 0x12)))) { > dev_dbg(&adapter->dev, "Detection of w83781d chip " > "failed at step 4\n"); > - err = -ENODEV; > - goto ERROR2; > + return -ENODEV; > } > /* If Winbond SMBus, check address at 0x48. > Asus doesn't support, except for as99127f rev.2 */ > if ((!(val1 & 0x80) && (val2 == 0xa3)) || > ((val1 & 0x80) && (val2 == 0x5c))) { > - if (w83781d_read_value > - (data, W83781D_REG_I2C_ADDR) != address) { > + if (w83781d_read_value_i2c > + (client, W83781D_REG_I2C_ADDR) != address) { > dev_dbg(&adapter->dev, "Detection of w83781d " > "chip failed at step 5\n"); > - err = -ENODEV; > - goto ERROR2; > + return -ENODEV; > } > } > } > > /* We have either had a force parameter, or we have already detected the > Winbond. Put it now into bank 0 and Vendor ID High Byte */ > - w83781d_write_value(data, W83781D_REG_BANK, > - (w83781d_read_value(data, W83781D_REG_BANK) > - & 0x78) | 0x80); > + w83781d_write_value_i2c(client, W83781D_REG_BANK, > + (w83781d_read_value_i2c(client, W83781D_REG_BANK) > + & 0x78) | 0x80); Should be i2c_smbus_write_byte_data(). > > /* Determine the chip type. */ > if (kind <= 0) { > /* get vendor ID */ > - val2 = w83781d_read_value(data, W83781D_REG_CHIPMAN); > + val2 = w83781d_read_value_i2c(client, W83781D_REG_CHIPMAN); Should be i2c_smbus_read_byte_data(), and again below. > if (val2 == 0x5c) > vendid = winbond; > else if (val2 == 0x12) > @@ -1202,11 +1113,10 @@ w83781d_detect(struct i2c_adapter *adapt > else { > dev_dbg(&adapter->dev, "w83781d chip vendor is " > "neither Winbond nor Asus\n"); > - err = -ENODEV; > - goto ERROR2; > + return -ENODEV; > } > > - val1 = w83781d_read_value(data, W83781D_REG_WCHIPID); > + val1 = w83781d_read_value_i2c(client, W83781D_REG_WCHIPID); > if ((val1 == 0x10 || val1 == 0x11) && vendid == winbond) > kind = w83781d; > else if (val1 == 0x30 && vendid == winbond) > @@ -1220,16 +1130,7 @@ w83781d_detect(struct i2c_adapter *adapt > dev_warn(&adapter->dev, "Ignoring 'force' " > "parameter for unknown chip at " > "address 0x%02x\n", address); > - err = -EINVAL; > - goto ERROR2; > - } > - > - if ((kind == w83781d || kind == w83782d) > - && w83781d_alias_detect(client, val1)) { > - dev_dbg(&adapter->dev, "Device at 0x%02x appears to " > - "be the same as ISA device\n", address); > - err = -ENODEV; > - goto ERROR2; > + return -ENODEV; Why did you move this to the probe() function? In the lm78 driver I left it in the detect() function. It can be discussed where it better fits (both have their advantages) but we need to be consistent across drivers. And leaving it here avoids a number of changes across the driver. > } > } > > @@ -1241,89 +1142,115 @@ w83781d_detect(struct i2c_adapter *adapt > client_name = "w83783s"; > } else if (kind == as99127f) { > client_name = "as99127f"; > - } > + } else > + return -ENODEV; This can't actually happen. > > /* Fill in the remaining client fields and put into the global list */ This comment most go away, it no longer applies. > - strlcpy(client->name, client_name, I2C_NAME_SIZE); > - data->type = kind; > + strlcpy(info->type, client_name, I2C_NAME_SIZE); > > - /* Tell the I2C layer a new client has arrived */ > - if ((err = i2c_attach_client(client))) > - goto ERROR2; > + return 0; > +} > + > +static int > +w83781d_probe(struct i2c_client *client, const struct i2c_device_id *id) > +{ > + struct i2c_adapter *adapter = client->adapter; > + struct device *dev = &client->dev; > + struct w83781d_data *data; > + int err; > + > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { > + err = -EINVAL; > + goto ERROR1; > + } This test was already done in the detect() function, so no point in doing it again. > + > + data = kzalloc(sizeof(struct w83781d_data), GFP_KERNEL); > + if (!data) { > + err = -ENOMEM; > + goto ERROR1; > + } > + > + i2c_set_clientdata(client, data); > + mutex_init(&data->lock); > + mutex_init(&data->update_lock); > + > + data->type = id->driver_data; > + data->client = client; > + > + if ((data->type == w83781d || data->type == w83782d) && > + w83781d_alias_detect(client)) { > + dev_dbg(&adapter->dev, "Device at 0x%02x appears to " > + "be the same as ISA device\n", client->addr); > + err = -ENODEV; > + goto ERROR1; > + } > > /* attach secondary i2c lm75-like clients */ > - if ((err = w83781d_detect_subclients(adapter, address, > - kind, client))) > - goto ERROR3; > + err = w83781d_detect_subclients(client); > + if (err) > + goto ERROR2; > > /* Initialize the chip */ > w83781d_init_device(dev); > > /* Register sysfs hooks */ > - err = w83781d_create_files(dev, kind, 0); > + err = w83781d_create_files(dev, data->type, 0); > if (err) > - goto ERROR4; > + goto ERROR3; > > data->hwmon_dev = hwmon_device_register(dev); > if (IS_ERR(data->hwmon_dev)) { > err = PTR_ERR(data->hwmon_dev); > - goto ERROR4; > + goto ERROR3; > } > > + dev_info(&client->dev, "%s: sensor '%s'\n", > + data->hwmon_dev->bus_id, client->name); No other driver does this. If we were to do that, this would go in the common hwmon code, not in each and every driver. > + > return 0; > > -ERROR4: > +ERROR3: You can keep this one named ERROR4 to minimize the changes. Ideally these labels should be renamed to something better anyway (but not in this patch.) > sysfs_remove_group(&dev->kobj, &w83781d_group); > sysfs_remove_group(&dev->kobj, &w83781d_group_opt); > > - if (data->lm75[1]) { > - i2c_detach_client(data->lm75[1]); > - kfree(data->lm75[1]); > - } > - if (data->lm75[0]) { > - i2c_detach_client(data->lm75[0]); > - kfree(data->lm75[0]); > - } > -ERROR3: > - i2c_detach_client(client); > + if (data->lm75[1]) You obviously mean data->lm75[0]; > + i2c_unregister_device(data->lm75[0]); > + if (data->lm75[1]) > + i2c_unregister_device(data->lm75[1]); > ERROR2: > + i2c_set_clientdata(client, NULL); > kfree(data); > ERROR1: > return err; > } > > static int > -w83781d_detach_client(struct i2c_client *client) > +w83781d_remove(struct i2c_client *client) > { > struct w83781d_data *data = i2c_get_clientdata(client); > - int err; > + struct device *dev = &client->dev; > > - /* main client */ > - if (data) { > - hwmon_device_unregister(data->hwmon_dev); > - sysfs_remove_group(&client->dev.kobj, &w83781d_group); > - sysfs_remove_group(&client->dev.kobj, &w83781d_group_opt); > - } > + hwmon_device_unregister(data->hwmon_dev); > > - if ((err = i2c_detach_client(client))) > - return err; > + sysfs_remove_group(&dev->kobj, &w83781d_group); > + sysfs_remove_group(&dev->kobj, &w83781d_group_opt); > > - /* main client */ > - if (data) > - kfree(data); > + if (data->lm75[1]) Here again you mean data->lm75[0]; > + i2c_unregister_device(data->lm75[0]); > + if (data->lm75[1]) > + i2c_unregister_device(data->lm75[1]); > > - /* subclient */ > - else > - kfree(client); > + i2c_set_clientdata(client, NULL); > + kfree(data); > > return 0; > } > > static int > -w83781d_read_value_i2c(struct w83781d_data *data, u16 reg) > +w83781d_read_value_i2c(struct i2c_client *client, u16 reg) > { > - struct i2c_client *client = &data->client; > - int res, bank; > + struct w83781d_data *data = i2c_get_clientdata(client); > + int res = 0, bank; > struct i2c_client *cl; > > bank = (reg >> 8) & 0x0f; > @@ -1333,7 +1260,7 @@ w83781d_read_value_i2c(struct w83781d_da > bank); > if (bank == 0 || bank > 2) { > res = i2c_smbus_read_byte_data(client, reg & 0xff); > - } else { > + } else if (data) { This change is not needed. You should never call this function before the client data is set. > /* switch to subclient */ > cl = data->lm75[bank - 1]; > /* convert from ISA to LM75 I2C addresses */ > @@ -1360,9 +1287,9 @@ w83781d_read_value_i2c(struct w83781d_da > } > > static int > -w83781d_write_value_i2c(struct w83781d_data *data, u16 reg, u16 value) > +w83781d_write_value_i2c(struct i2c_client *client, u16 reg, u16 value) > { > - struct i2c_client *client = &data->client; > + struct w83781d_data *data = i2c_get_clientdata(client); > int bank; > struct i2c_client *cl; > > @@ -1374,7 +1301,7 @@ w83781d_write_value_i2c(struct w83781d_d > if (bank == 0 || bank > 2) { > i2c_smbus_write_byte_data(client, reg & 0xff, > value & 0xff); > - } else { > + } else if (data) { Same here. > /* switch to subclient */ > cl = data->lm75[bank - 1]; > /* convert from ISA to LM75 I2C addresses */ > @@ -1499,7 +1426,7 @@ w83781d_init_device(struct device *dev) > static struct w83781d_data *w83781d_update_device(struct device *dev) > { > struct w83781d_data *data = dev_get_drvdata(dev); > - struct i2c_client *client = &data->client; > + struct i2c_client *client = data->client; > int i; > > mutex_lock(&data->update_lock); > @@ -1612,6 +1539,30 @@ static struct w83781d_data *w83781d_upda > return data; > } > > +static const struct i2c_device_id w83781d_ids[] = { > + { "w83781d", w83781d, }, > + { "w83782d", w83782d, }, > + { "w83783s", w83783s, }, > + { "as99127f", as99127f }, > + { /* LIST END */ } > +}; > +MODULE_DEVICE_TABLE(i2c, w83781d_ids); > + > +static struct i2c_driver w83781d_driver = { > + .class = I2C_CLASS_HWMON, > + .driver = { > + .name = "w83781d", > + }, > + .probe = w83781d_probe, > + .remove = w83781d_remove, > + .id_table = w83781d_ids, > + .detect = w83781d_detect, > + .address_data = &addr_data, > +}; > + > +/* > + * ISA related code > + */ > #ifdef CONFIG_ISA > > /* ISA device, if found */ > @@ -1625,19 +1576,15 @@ static ssize_t > show_name(struct device *dev, struct device_attribute *devattr, char *buf) > { > struct w83781d_data *data = dev_get_drvdata(dev); > - return sprintf(buf, "%s\n", data->client.name); > + return sprintf(buf, "%s\n", data->client->name); > } > static DEVICE_ATTR(name, S_IRUGO, show_name, NULL); This won't work. ISA devices do not have data->client, so this will crash. You need to add data->name for ISA devices. > > -static struct w83781d_data *w83781d_data_if_isa(void) > -{ > - return pdev ? platform_get_drvdata(pdev) : NULL; > -} > - > /* Returns 1 if the I2C chip appears to be an alias of the ISA chip */ > -static int w83781d_alias_detect(struct i2c_client *client, u8 chipid) > +static int w83781d_alias_detect(struct i2c_client *client) > { > struct w83781d_data *i2c, *isa; > + u8 chipid; > int i; > > if (!pdev) /* No ISA chip */ > @@ -1648,6 +1595,7 @@ static int w83781d_alias_detect(struct i > > if (w83781d_read_value(isa, W83781D_REG_I2C_ADDR) != client->addr) > return 0; /* Address doesn't match */ > + chipid = w83781d_read_value(i2c, W83781D_REG_WCHIPID); > if (w83781d_read_value(isa, W83781D_REG_WCHIPID) != chipid) > return 0; /* Chip type doesn't match */ > > @@ -1671,7 +1619,7 @@ static int w83781d_alias_detect(struct i > static int > w83781d_read_value_isa(struct w83781d_data *data, u16 reg) > { > - struct i2c_client *client = &data->client; > + struct i2c_client *client = data->client; > int word_sized, res; > > word_sized = (((reg & 0xff00) == 0x100) > @@ -1705,7 +1653,7 @@ w83781d_read_value_isa(struct w83781d_da > static void > w83781d_write_value_isa(struct w83781d_data *data, u16 reg, u16 value) > { > - struct i2c_client *client = &data->client; > + struct i2c_client *client = data->client; > int word_sized; > > word_sized = (((reg & 0xff00) == 0x100) > @@ -1742,12 +1690,12 @@ w83781d_write_value_isa(struct w83781d_d > static int > w83781d_read_value(struct w83781d_data *data, u16 reg) > { > - struct i2c_client *client = &data->client; > + struct i2c_client *client = data->client; > int res; > > mutex_lock(&data->lock); > if (client->driver) > - res = w83781d_read_value_i2c(data, reg); > + res = w83781d_read_value_i2c(client, reg); > else > res = w83781d_read_value_isa(data, reg); > mutex_unlock(&data->lock); > @@ -1757,11 +1705,11 @@ w83781d_read_value(struct w83781d_data * > static int > w83781d_write_value(struct w83781d_data *data, u16 reg, u16 value) > { > - struct i2c_client *client = &data->client; > + struct i2c_client *client = data->client; > > mutex_lock(&data->lock); > if (client->driver) > - w83781d_write_value_i2c(data, reg, value); > + w83781d_write_value_i2c(client, reg, value); > else > w83781d_write_value_isa(data, reg, value); > mutex_unlock(&data->lock); > @@ -1790,8 +1738,8 @@ w83781d_isa_probe(struct platform_device > goto exit_release_region; > } > mutex_init(&data->lock); > - data->client.addr = res->start; > - i2c_set_clientdata(&data->client, data); > + data->client->addr = res->start; This won't work either. > + i2c_set_clientdata(data->client, data); Don't even think of it ;) > platform_set_drvdata(pdev, data); > > reg = w83781d_read_value(data, W83781D_REG_WCHIPID); > @@ -1804,7 +1752,7 @@ w83781d_isa_probe(struct platform_device > data->type = w83781d; > name = "w83781d"; > } > - strlcpy(data->client.name, name, I2C_NAME_SIZE); > + strlcpy(data->client->name, name, I2C_NAME_SIZE); > > /* Initialize the W83781D chip */ > w83781d_init_device(&pdev->dev); > @@ -1846,7 +1794,7 @@ w83781d_isa_remove(struct platform_devic > sysfs_remove_group(&pdev->dev.kobj, &w83781d_group); > sysfs_remove_group(&pdev->dev.kobj, &w83781d_group_opt); > device_remove_file(&pdev->dev, &dev_attr_name); > - release_region(data->client.addr + W83781D_ADDR_REG_OFFSET, 2); > + release_region(data->client->addr + W83781D_ADDR_REG_OFFSET, 2); > kfree(data); > > return 0; > @@ -2030,13 +1978,8 @@ w83781d_isa_unregister(void) > } > #else /* !CONFIG_ISA */ > > -static struct w83781d_data *w83781d_data_if_isa(void) > -{ > - return NULL; > -} > - > static int > -w83781d_alias_detect(struct i2c_client *client, u8 chipid) > +w83781d_alias_detect(struct i2c_client *client) > { > return 0; > } > @@ -2044,10 +1987,11 @@ w83781d_alias_detect(struct i2c_client * > static int > w83781d_read_value(struct w83781d_data *data, u16 reg) > { > + struct i2c_client *client = data->client; > int res; > > mutex_lock(&data->lock); > - res = w83781d_read_value_i2c(data, reg); > + res = w83781d_read_value_i2c(client, reg); > mutex_unlock(&data->lock); > > return res; > @@ -2056,8 +2000,10 @@ w83781d_read_value(struct w83781d_data * > static int > w83781d_write_value(struct w83781d_data *data, u16 reg, u16 value) > { > + struct i2c_client *client = data->client; > + > mutex_lock(&data->lock); > - w83781d_write_value_i2c(data, reg, value); > + w83781d_write_value_i2c(client, reg, value); > mutex_unlock(&data->lock); > > return 0; > @@ -2092,9 +2038,9 @@ sensors_w83781d_init(void) > > return 0; > > - exit_unreg_isa: > +exit_unreg_isa: > w83781d_isa_unregister(); > - exit: > +exit: Useless changes, please revert. > return res; > } > Please let me know how you want to proceed from here. Do you want to rework this patch and post an update (one or more patches)? Or do you prefer that I make all the updates myself, and then you test and review the patch? My current set of patches for the w83781d driver is here: http://jdelvare.pck.nerim.net/sensors/w83781d/ Any patch update you send should apply on top of that. Also, the first 3 patches need to be tested, reviewed and acked before I can push them upstream. Can you please help me with this? Ideally I would like to get all the w83781d patches sorted out quickly so that they can spend some time in linux-next and then be sent to Linus during the 2.6.28 merge window. Thanks, -- Jean Delvare
