[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