[lm-sensors] [PATCH 2/2] hwmon: w83781d: use new style driver binding
Wolfgang Grandegger
wg at grandegger.com
Tue Oct 7 10:13:40 CEST 2008
Jean Delvare wrote:
> 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?
If you have time, please go ahead. You will do a better job anyhow. I
will not have time to catch up this and maybe the next week as well, sorry.
> 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.
OK, I will re-gain access to the embedded hardware end of this week.
Thanks.
Wolfgang.
>
> Thanks,
More information about the lm-sensors
mailing list