[lm-sensors] New style driver for w83781d?

[lm-sensors] New style driver for w83781d?

[Wolfgang Grandegger]

Hello,

I realized that various HWMON drivers have been converted to new style.
Has the w83781d been ported as well? Or is somebody already working on it?

Wolfgang.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Jean Delvare]

Hi Wolfgang,

On Wed, 06 Aug 2008 15:19:51 +0200, Wolfgang Grandegger wrote:
> I realized that various HWMON drivers have been converted to new style.
> Has the w83781d been ported as well? Or is somebody already working on it?

This is one of the two hwmon drivers that have not yet been converted
(the other one is lm78.) I had been delaying them because they are
special (hybrid drivers, supporting both I2C and ISA devices) and also
because there are pending patches for these drivers and I was hoping
that the pending patches could be merged first (but it didn't happen
yet.) If you want to work on either of these drivers, please do. I can
do some testing on both. If you have a particular interest in these
drivers, maybe we can work together.

For a general overview of the conversion of i2c drivers to new-style,
please see:

http://jdelvare.pck.nerim.net/linux/legacy_i2c_driver.list

** means the patch is already upstream, * means that the patch is
available but needs testing and/or review. The driver list might not be
totally up-to-date, I didn't check yet if there have been legacy i2c
drivers added upstream lately.

My initial hope was to have everything converted in 2.6.28, but now I
suspect that only hwmon and rtc drivers will be completely converted by
then

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Wolfgang Grandegger]

Jean Delvare wrote:
> Hi Wolfgang,
> 
> On Wed, 06 Aug 2008 15:19:51 +0200, Wolfgang Grandegger wrote:
>> I realized that various HWMON drivers have been converted to new style.
>> Has the w83781d been ported as well? Or is somebody already working on it?
> 
> This is one of the two hwmon drivers that have not yet been converted
> (the other one is lm78.) I had been delaying them because they are
> special (hybrid drivers, supporting both I2C and ISA devices) and also
> because there are pending patches for these drivers and I was hoping
> that the pending patches could be merged first (but it didn't happen
> yet.) If you want to work on either of these drivers, please do. I can
> do some testing on both. If you have a particular interest in these
> drivers, maybe we can work together.

OK. Can you point me to the pending patches?

> For a general overview of the conversion of i2c drivers to new-style,
> please see:
> 
> http://jdelvare.pck.nerim.net/linux/legacy_i2c_driver.list
> 
> ** means the patch is already upstream, * means that the patch is
> available but needs testing and/or review. The driver list might not be
> totally up-to-date, I didn't check yet if there have been legacy i2c
> drivers added upstream lately.
> 
> My initial hope was to have everything converted in 2.6.28, but now I
> suspect that only hwmon and rtc drivers will be completely converted by
> then

I see.

Wolfgang.



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Jean Delvare]

Hi Wolfgang,

On Thu, 07 Aug 2008 08:49:45 +0200, Wolfgang Grandegger wrote:
> Jean Delvare wrote:
> > Hi Wolfgang,
> > 
> > On Wed, 06 Aug 2008 15:19:51 +0200, Wolfgang Grandegger wrote:
> >> I realized that various HWMON drivers have been converted to new style.
> >> Has the w83781d been ported as well? Or is somebody already working on it?
> > 
> > This is one of the two hwmon drivers that have not yet been converted
> > (the other one is lm78.) I had been delaying them because they are
> > special (hybrid drivers, supporting both I2C and ISA devices) and also
> > because there are pending patches for these drivers and I was hoping
> > that the pending patches could be merged first (but it didn't happen
> > yet.) If you want to work on either of these drivers, please do. I can
> > do some testing on both. If you have a particular interest in these
> > drivers, maybe we can work together.
> 
> OK. Can you point me to the pending patches?

Certainly.

http://jdelvare.pck.nerim.net/sensors/w83781d/

http://jdelvare.pck.nerim.net/sensors/lm78/

If you can help reviewing and/or testing some of these patches, that
would be very welcome.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Wolfgang Grandegger]

Hi Jean,

Jean Delvare wrote:
> Hi Wolfgang,
> 
> On Wed, 06 Aug 2008 15:19:51 +0200, Wolfgang Grandegger wrote:
>> I realized that various HWMON drivers have been converted to new style.
>> Has the w83781d been ported as well? Or is somebody already working on it?
> 
> This is one of the two hwmon drivers that have not yet been converted
> (the other one is lm78.) I had been delaying them because they are
> special (hybrid drivers, supporting both I2C and ISA devices) and also
> because there are pending patches for these drivers and I was hoping
> that the pending patches could be merged first (but it didn't happen
> yet.) If you want to work on either of these drivers, please do. I can
> do some testing on both. If you have a particular interest in these
> drivers, maybe we can work together.

We need support for the MON35W42 on an embedded MPC8544 board, which 
seems to be compatible with the w83782d. There are more issues, e.g. the 
early ISA bus probing of the existing driver hangs the system. Maybe i2c 
probing should be done first!?

Wolfgang.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Jean Delvare]

Hi Wolfgang,

On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote:
> We need support for the MON35W42 on an embedded MPC8544 board, which 
> seems to be compatible with the w83782d. There are more issues, e.g. the 
> early ISA bus probing of the existing driver hangs the system. Maybe i2c 
> probing should be done first!?

If both interfaces are available, we want to use the ISA one, because
it's much faster, so it must be registered first.

What could be done though would be disabling the ISA interface
altogether on architectures where it isn't supported (in particular
PPC.) This shouldn't be too difficult. If you write a patch doing this,
I'll be happy to review it. Affected drivers would be w83781d and lm78.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Wolfgang Grandegger]

Jean Delvare wrote:
> Hi Wolfgang,
> 
> On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote:
>> We need support for the MON35W42 on an embedded MPC8544 board, which 
>> seems to be compatible with the w83782d. There are more issues, e.g. the 
>> early ISA bus probing of the existing driver hangs the system. Maybe i2c 
>> probing should be done first!?
> 
> If both interfaces are available, we want to use the ISA one, because
> it's much faster, so it must be registered first.
> 
> What could be done though would be disabling the ISA interface
> altogether on architectures where it isn't supported (in particular
> PPC.) This shouldn't be too difficult. If you write a patch doing this,
> I'll be happy to review it. Affected drivers would be w83781d and lm78.

Some PPC have an ISA bus as well:

   http://lxr.linux.no/linux+v2.6.26.2/arch/powerpc/Kconfig#L494

I think selecting the ISA code with '#ifdef CONFIG_ISA' would be the 
right solution?

Wolfgang.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Jean Delvare]

On Tue, 12 Aug 2008 11:28:54 +0200, Wolfgang Grandegger wrote:
> Jean Delvare wrote:
> > Hi Wolfgang,
> > 
> > On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote:
> >> We need support for the MON35W42 on an embedded MPC8544 board, which 
> >> seems to be compatible with the w83782d. There are more issues, e.g. the 
> >> early ISA bus probing of the existing driver hangs the system. Maybe i2c 
> >> probing should be done first!?
> > 
> > If both interfaces are available, we want to use the ISA one, because
> > it's much faster, so it must be registered first.
> > 
> > What could be done though would be disabling the ISA interface
> > altogether on architectures where it isn't supported (in particular
> > PPC.) This shouldn't be too difficult. If you write a patch doing this,
> > I'll be happy to review it. Affected drivers would be w83781d and lm78.
> 
> Some PPC have an ISA bus as well:
> 
>    http://lxr.linux.no/linux+v2.6.26.2/arch/powerpc/Kconfig#L494
> 
> I think selecting the ISA code with '#ifdef CONFIG_ISA' would be the 
> right solution?

Agreed.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Wolfgang Grandegger]

Jean Delvare wrote:
> On Tue, 12 Aug 2008 11:28:54 +0200, Wolfgang Grandegger wrote:
>> Jean Delvare wrote:
>>> Hi Wolfgang,
>>>
>>> On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote:
>>>> We need support for the MON35W42 on an embedded MPC8544 board, which 
>>>> seems to be compatible with the w83782d. There are more issues, e.g. the 
>>>> early ISA bus probing of the existing driver hangs the system. Maybe i2c 
>>>> probing should be done first!?
>>> If both interfaces are available, we want to use the ISA one, because
>>> it's much faster, so it must be registered first.
>>>
>>> What could be done though would be disabling the ISA interface
>>> altogether on architectures where it isn't supported (in particular
>>> PPC.) This shouldn't be too difficult. If you write a patch doing this,
>>> I'll be happy to review it. Affected drivers would be w83781d and lm78.
>> Some PPC have an ISA bus as well:
>>
>>    http://lxr.linux.no/linux+v2.6.26.2/arch/powerpc/Kconfig#L494
>>
>> I think selecting the ISA code with '#ifdef CONFIG_ISA' would be the 
>> right solution?
> 
> Agreed.

See the attached patch below. When I have some more free time, I will
convert the driver to new style as well.

Wolfgang


=================================From: Wolfgang Grandegger <wg@grandegger.com>
Subject: HWMON: w83781d: make ISA interface depend on CONFIG_ISA

Probing the ISA bus on systems without ISA bus may hang the system.
This patch makes the ISA bus related code depend on the kernel
configuration parameter CONFIG_ISA.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/hwmon/w83781d.c |  525 ++++++++++++++++++++++++++----------------------
 1 file changed, 291 insertions(+), 234 deletions(-)

Index: linux-2.6-denx/drivers/hwmon/w83781d.c
=================================--- linux-2.6-denx.orig/drivers/hwmon/w83781d.c
+++ linux-2.6-denx/drivers/hwmon/w83781d.c
@@ -49,14 +49,9 @@
 #include <asm/io.h>
 #include "lm75.h"
 
-/* ISA device, if found */
-static struct platform_device *pdev;
-
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = { 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d,
 						0x2e, 0x2f, I2C_CLIENT_END };
-static unsigned short isa_address = 0x290;
-
 /* Insmod parameters */
 I2C_CLIENT_INSMOD_4(w83781d, w83782d, w83783s, as99127f);
 I2C_CLIENT_MODULE_PARM(force_subclients, "List of subclient addresses: "
@@ -253,14 +248,16 @@ static int w83781d_attach_adapter(struct
 static int w83781d_detect(struct i2c_adapter *adapter, int address, int kind);
 static int w83781d_detach_client(struct i2c_client *client);
 
-static int __devinit w83781d_isa_probe(struct platform_device *pdev);
-static int __devexit w83781d_isa_remove(struct platform_device *pdev);
-
-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 int w83781d_read_value_i2c(struct w83781d_data *data, u16 reg);
+static int w83781d_write_value_i2c(struct w83781d_data *data, u16 reg, u16 val);
 static struct w83781d_data *w83781d_update_device(struct device *dev);
 static void w83781d_init_device(struct device *dev);
 
+static int (*w83781d_read_value)(struct w83781d_data *data, u16 reg) +	w83781d_read_value_i2c;
+static int (*w83781d_write_value)(struct w83781d_data *data, u16 reg, u16 val) +	w83781d_write_value_i2c;
+
 static struct i2c_driver w83781d_driver = {
 	.driver = {
 		.name = "w83781d",
@@ -269,16 +266,6 @@ static struct i2c_driver w83781d_driver 
 	.detach_client = w83781d_detach_client,
 };
 
-static struct platform_driver w83781d_isa_driver = {
-	.driver = {
-		.owner = THIS_MODULE,
-		.name = "w83781d",
-	},
-	.probe = w83781d_isa_probe,
-	.remove = w83781d_isa_remove,
-};
-
-
 /* following are the sysfs callback functions */
 #define show_in_reg(reg) \
 static ssize_t show_##reg (struct device *dev, struct device_attribute *da, \
@@ -866,16 +853,6 @@ static SENSOR_DEVICE_ATTR(temp2_type, S_
 static SENSOR_DEVICE_ATTR(temp3_type, S_IRUGO | S_IWUSR,
 	show_sensor, store_sensor, 2);
 
-/* I2C devices get this name attribute automatically, but for ISA devices
-   we must create it by ourselves. */
-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);
-}
-static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
-
 /* This function is called when:
      * w83781d_driver is inserted (when this module is loaded), for each
        available adapter
@@ -1151,12 +1128,6 @@ w83781d_create_files(struct device *dev,
 		}
 	}
 
-	if (is_isa) {
-		err = device_create_file(&pdev->dev, &dev_attr_name);
-		if (err)
-			return err;
-	}
-
 	return 0;
 }
 
@@ -1297,7 +1268,7 @@ w83781d_detect(struct i2c_adapter *adapt
 	/* Initialize the chip */
 	w83781d_init_device(dev);
 
-	/* Register sysfs hooks */
+	/* Register common sysfs hooks */
 	err = w83781d_create_files(dev, kind, 0);
 	if (err)
 		goto ERROR4;
@@ -1357,85 +1328,6 @@ w83781d_detach_client(struct i2c_client 
 	return 0;
 }
 
-static int __devinit
-w83781d_isa_probe(struct platform_device *pdev)
-{
-	int err, reg;
-	struct w83781d_data *data;
-	struct resource *res;
-	const char *name;
-
-	/* Reserve the ISA region */
-	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (!request_region(res->start + W83781D_ADDR_REG_OFFSET, 2,
-			    "w83781d")) {
-		err = -EBUSY;
-		goto exit;
-	}
-
-	if (!(data = kzalloc(sizeof(struct w83781d_data), GFP_KERNEL))) {
-		err = -ENOMEM;
-		goto exit_release_region;
-	}
-	mutex_init(&data->lock);
-	data->client.addr = res->start;
-	i2c_set_clientdata(&data->client, data);
-	platform_set_drvdata(pdev, data);
-
-	reg = w83781d_read_value(data, W83781D_REG_WCHIPID);
-	switch (reg) {
-	case 0x30:
-		data->type = w83782d;
-		name = "w83782d";
-		break;
-	default:
-		data->type = w83781d;
-		name = "w83781d";
-	}
-	strlcpy(data->client.name, name, I2C_NAME_SIZE);
-
-	/* Initialize the W83781D chip */
-	w83781d_init_device(&pdev->dev);
-
-	/* Register sysfs hooks */
-	err = w83781d_create_files(&pdev->dev, data->type, 1);
-	if (err)
-		goto exit_remove_files;
-
-	data->hwmon_dev = hwmon_device_register(&pdev->dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		err = PTR_ERR(data->hwmon_dev);
-		goto exit_remove_files;
-	}
-
-	return 0;
-
- exit_remove_files:
-	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);
-	kfree(data);
- exit_release_region:
-	release_region(res->start + W83781D_ADDR_REG_OFFSET, 2);
- exit:
-	return err;
-}
-
-static int __devexit
-w83781d_isa_remove(struct platform_device *pdev)
-{
-	struct w83781d_data *data = platform_get_drvdata(pdev);
-
-	hwmon_device_unregister(data->hwmon_dev);
-	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);
-	kfree(data);
-
-	return 0;
-}
-
 /* The SMBus locks itself, usually, but nothing may access the Winbond between
    bank switches. ISA access must always be locked explicitly!
    We ignore the W83781D BUSY flag at this moment - it could lead to deadlocks,
@@ -1443,134 +1335,80 @@ w83781d_isa_remove(struct platform_devic
    There are some ugly typecasts here, but the good news is - they should
    nowhere else be necessary! */
 static int
-w83781d_read_value(struct w83781d_data *data, u16 reg)
+w83781d_read_value_i2c(struct w83781d_data *data, u16 reg)
 {
 	struct i2c_client *client = &data->client;
-	int res, word_sized, bank;
 	struct i2c_client *cl;
+	int res, bank;
 
 	mutex_lock(&data->lock);
-	if (!client->driver) { /* ISA device */
-		word_sized = (((reg & 0xff00) = 0x100)
-			      || ((reg & 0xff00) = 0x200))
-		    && (((reg & 0x00ff) = 0x50)
-			|| ((reg & 0x00ff) = 0x53)
-			|| ((reg & 0x00ff) = 0x55));
-		if (reg & 0xff00) {
-			outb_p(W83781D_REG_BANK,
-			       client->addr + W83781D_ADDR_REG_OFFSET);
-			outb_p(reg >> 8,
-			       client->addr + W83781D_DATA_REG_OFFSET);
-		}
-		outb_p(reg & 0xff, client->addr + W83781D_ADDR_REG_OFFSET);
-		res = inb_p(client->addr + W83781D_DATA_REG_OFFSET);
-		if (word_sized) {
-			outb_p((reg & 0xff) + 1,
-			       client->addr + W83781D_ADDR_REG_OFFSET);
-			res -			    (res << 8) + inb_p(client->addr +
-					       W83781D_DATA_REG_OFFSET);
-		}
-		if (reg & 0xff00) {
-			outb_p(W83781D_REG_BANK,
-			       client->addr + W83781D_ADDR_REG_OFFSET);
-			outb_p(0, client->addr + W83781D_DATA_REG_OFFSET);
-		}
+	bank = (reg >> 8) & 0x0f;
+	if (bank > 2)
+		/* switch banks */
+		i2c_smbus_write_byte_data(client, W83781D_REG_BANK,
+					  bank);
+	if (bank = 0 || bank > 2) {
+		res = i2c_smbus_read_byte_data(client, reg & 0xff);
 	} else {
-		bank = (reg >> 8) & 0x0f;
-		if (bank > 2)
-			/* switch banks */
-			i2c_smbus_write_byte_data(client, W83781D_REG_BANK,
-						  bank);
-		if (bank = 0 || bank > 2) {
-			res = i2c_smbus_read_byte_data(client, reg & 0xff);
-		} else {
-			/* switch to subclient */
-			cl = data->lm75[bank - 1];
-			/* convert from ISA to LM75 I2C addresses */
-			switch (reg & 0xff) {
-			case 0x50:	/* TEMP */
-				res = swab16(i2c_smbus_read_word_data(cl, 0));
-				break;
-			case 0x52:	/* CONFIG */
-				res = i2c_smbus_read_byte_data(cl, 1);
-				break;
-			case 0x53:	/* HYST */
-				res = swab16(i2c_smbus_read_word_data(cl, 2));
-				break;
-			case 0x55:	/* OVER */
-			default:
-				res = swab16(i2c_smbus_read_word_data(cl, 3));
-				break;
-			}
+		/* switch to subclient */
+		cl = data->lm75[bank - 1];
+		/* convert from ISA to LM75 I2C addresses */
+		switch (reg & 0xff) {
+		case 0x50:	/* TEMP */
+			res = swab16(i2c_smbus_read_word_data(cl, 0));
+			break;
+		case 0x52:	/* CONFIG */
+			res = i2c_smbus_read_byte_data(cl, 1);
+			break;
+		case 0x53:	/* HYST */
+			res = swab16(i2c_smbus_read_word_data(cl, 2));
+			break;
+		case 0x55:	/* OVER */
+		default:
+			res = swab16(i2c_smbus_read_word_data(cl, 3));
+			break;
 		}
-		if (bank > 2)
-			i2c_smbus_write_byte_data(client, W83781D_REG_BANK, 0);
 	}
+	if (bank > 2)
+		i2c_smbus_write_byte_data(client, W83781D_REG_BANK, 0);
 	mutex_unlock(&data->lock);
 	return res;
 }
 
 static int
-w83781d_write_value(struct w83781d_data *data, u16 reg, u16 value)
+w83781d_write_value_i2c(struct w83781d_data *data, u16 reg, u16 val)
 {
 	struct i2c_client *client = &data->client;
-	int word_sized, bank;
 	struct i2c_client *cl;
+	int bank;
 
 	mutex_lock(&data->lock);
-	if (!client->driver) { /* ISA device */
-		word_sized = (((reg & 0xff00) = 0x100)
-			      || ((reg & 0xff00) = 0x200))
-		    && (((reg & 0x00ff) = 0x53)
-			|| ((reg & 0x00ff) = 0x55));
-		if (reg & 0xff00) {
-			outb_p(W83781D_REG_BANK,
-			       client->addr + W83781D_ADDR_REG_OFFSET);
-			outb_p(reg >> 8,
-			       client->addr + W83781D_DATA_REG_OFFSET);
-		}
-		outb_p(reg & 0xff, client->addr + W83781D_ADDR_REG_OFFSET);
-		if (word_sized) {
-			outb_p(value >> 8,
-			       client->addr + W83781D_DATA_REG_OFFSET);
-			outb_p((reg & 0xff) + 1,
-			       client->addr + W83781D_ADDR_REG_OFFSET);
-		}
-		outb_p(value & 0xff, client->addr + W83781D_DATA_REG_OFFSET);
-		if (reg & 0xff00) {
-			outb_p(W83781D_REG_BANK,
-			       client->addr + W83781D_ADDR_REG_OFFSET);
-			outb_p(0, client->addr + W83781D_DATA_REG_OFFSET);
-		}
+	bank = (reg >> 8) & 0x0f;
+	if (bank > 2)
+		/* switch banks */
+		i2c_smbus_write_byte_data(client, W83781D_REG_BANK,
+					  bank);
+	if (bank = 0 || bank > 2) {
+		i2c_smbus_write_byte_data(client, reg & 0xff,
+					  val & 0xff);
 	} else {
-		bank = (reg >> 8) & 0x0f;
-		if (bank > 2)
-			/* switch banks */
-			i2c_smbus_write_byte_data(client, W83781D_REG_BANK,
-						  bank);
-		if (bank = 0 || bank > 2) {
-			i2c_smbus_write_byte_data(client, reg & 0xff,
-						  value & 0xff);
-		} else {
-			/* switch to subclient */
-			cl = data->lm75[bank - 1];
-			/* convert from ISA to LM75 I2C addresses */
-			switch (reg & 0xff) {
-			case 0x52:	/* CONFIG */
-				i2c_smbus_write_byte_data(cl, 1, value & 0xff);
-				break;
-			case 0x53:	/* HYST */
-				i2c_smbus_write_word_data(cl, 2, swab16(value));
-				break;
-			case 0x55:	/* OVER */
-				i2c_smbus_write_word_data(cl, 3, swab16(value));
-				break;
-			}
+		/* switch to subclient */
+		cl = data->lm75[bank - 1];
+		/* convert from ISA to LM75 I2C addresses */
+		switch (reg & 0xff) {
+		case 0x52:	/* CONFIG */
+			i2c_smbus_write_byte_data(cl, 1, val & 0xff);
+			break;
+		case 0x53:	/* HYST */
+			i2c_smbus_write_word_data(cl, 2, swab16(val));
+			break;
+		case 0x55:	/* OVER */
+			i2c_smbus_write_word_data(cl, 3, swab16(val));
+			break;
 		}
-		if (bank > 2)
-			i2c_smbus_write_byte_data(client, W83781D_REG_BANK, 0);
 	}
+	if (bank > 2)
+		i2c_smbus_write_byte_data(client, W83781D_REG_BANK, 0);
 	mutex_unlock(&data->lock);
 	return 0;
 }
@@ -1792,6 +1630,96 @@ static struct w83781d_data *w83781d_upda
 	return data;
 }
 
+#ifdef CONFIG_ISA
+/*
+ * ISA device
+ */
+static struct platform_device *pdev;
+
+static unsigned short isa_address = 0x290;
+
+/* I2C devices get this name attribute automatically, but for ISA devices
+   we must create it by ourselves. */
+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);
+}
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+/* The SMBus locks itself, usually, but nothing may access the Winbond between
+   bank switches. ISA access must always be locked explicitly!
+   We ignore the W83781D BUSY flag at this moment - it could lead to deadlocks,
+   would slow down the W83781D access and should not be necessary.
+   There are some ugly typecasts here, but the good news is - they should
+   nowhere else be necessary! */
+static int
+w83781d_read_value_i2c_isa(struct w83781d_data *data, u16 reg)
+{
+	int res, word_sized, bank;
+
+	mutex_lock(&data->lock);
+	word_sized = (((reg & 0xff00) = 0x100) || ((reg & 0xff00) = 0x200))
+		&& (((reg & 0x00ff) = 0x50) || ((reg & 0x00ff) = 0x53)
+		    || ((reg & 0x00ff) = 0x55));
+	if (reg & 0xff00) {
+		outb_p(W83781D_REG_BANK,
+		       client->addr + W83781D_ADDR_REG_OFFSET);
+		outb_p(reg >> 8,
+		       client->addr + W83781D_DATA_REG_OFFSET);
+	}
+	outb_p(reg & 0xff, client->addr + W83781D_ADDR_REG_OFFSET);
+	res = inb_p(client->addr + W83781D_DATA_REG_OFFSET);
+	if (word_sized) {
+		outb_p((reg & 0xff) + 1,
+		       client->addr + W83781D_ADDR_REG_OFFSET);
+		res +			(res << 8) + inb_p(client->addr +
+					   W83781D_DATA_REG_OFFSET);
+	}
+	if (reg & 0xff00) {
+		outb_p(W83781D_REG_BANK,
+		       client->addr + W83781D_ADDR_REG_OFFSET);
+		outb_p(0, client->addr + W83781D_DATA_REG_OFFSET);
+	}
+	mutex_unlock(&data->lock);
+	return res;
+}
+
+static int
+w83781d_write_value_i2c_isa(struct w83781d_data *data, u16 reg, u16 val)
+{
+	struct i2c_client *client = &data->client;
+	int word_sized, bank;
+	struct i2c_client *cl;
+
+	mutex_lock(&data->lock);
+	word_sized = (((reg & 0xff00) = 0x100) || ((reg & 0xff00) = 0x200))
+		&& (((reg & 0x00ff) = 0x53) || ((reg & 0x00ff) = 0x55));
+	if (reg & 0xff00) {
+		outb_p(W83781D_REG_BANK,
+		       client->addr + W83781D_ADDR_REG_OFFSET);
+		outb_p(reg >> 8,
+		       client->addr + W83781D_DATA_REG_OFFSET);
+	}
+	outb_p(reg & 0xff, client->addr + W83781D_ADDR_REG_OFFSET);
+	if (word_sized) {
+		outb_p(val >> 8,
+		       client->addr + W83781D_DATA_REG_OFFSET);
+		outb_p((reg & 0xff) + 1,
+		       client->addr + W83781D_ADDR_REG_OFFSET);
+	}
+	outb_p(val & 0xff, client->addr + W83781D_DATA_REG_OFFSET);
+	if (reg & 0xff00) {
+		outb_p(W83781D_REG_BANK,
+		       client->addr + W83781D_ADDR_REG_OFFSET);
+		outb_p(0, client->addr + W83781D_DATA_REG_OFFSET);
+	}
+	mutex_unlock(&data->lock);
+	return 0;
+}
+
 /* return 1 if a supported chip is found, 0 otherwise */
 static int __init
 w83781d_isa_found(unsigned short address)
@@ -1927,43 +1855,172 @@ w83781d_isa_device_add(unsigned short ad
 	return err;
 }
 
-static int __init
-sensors_w83781d_init(void)
+static int __devinit
+w83781d_isa_probe(struct platform_device *pdev)
 {
-	int res;
+	int err, reg;
+	struct w83781d_data *data;
+	struct resource *res;
+	const char *name;
 
-	res = i2c_add_driver(&w83781d_driver);
-	if (res)
+	/* Reserve the ISA region */
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!request_region(res->start + W83781D_ADDR_REG_OFFSET, 2,
+			    "w83781d")) {
+		err = -EBUSY;
 		goto exit;
+	}
+
+	data = kzalloc(sizeof(struct w83781d_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit_release_region;
+	}
+	mutex_init(&data->lock);
+	data->client.addr = res->start;
+	i2c_set_clientdata(&data->client, data);
+	platform_set_drvdata(pdev, data);
+
+	reg = w83781d_read_value(data, W83781D_REG_WCHIPID);
+	switch (reg) {
+	case 0x30:
+		data->type = w83782d;
+		name = "w83782d";
+		break;
+	default:
+		data->type = w83781d;
+		name = "w83781d";
+	}
+	strlcpy(data->client.name, name, I2C_NAME_SIZE);
+
+	/* Initialize the W83781D chip */
+	w83781d_init_device(&pdev->dev);
+
+	/* Register sysfs common hooks */
+	err = w83781d_create_files(&pdev->dev, data->type, 1);
+	if (err)
+		goto exit_remove_files;
+
+	err = device_create_file(&pdev->dev, &dev_attr_name);
+	if (err)
+		goto exit_remove_files;
+
+	data->hwmon_dev = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		goto exit_remove_files;
+	}
+
+	return 0;
+
+ exit_remove_files:
+	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);
+	kfree(data);
+ exit_release_region:
+	release_region(res->start + W83781D_ADDR_REG_OFFSET, 2);
+ exit:
+	return err;
+}
+
+static int __devexit
+w83781d_isa_remove(struct platform_device *pdev)
+{
+	struct w83781d_data *data = platform_get_drvdata(pdev);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	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);
+	kfree(data);
+
+	return 0;
+}
+
+static struct platform_driver w83781d_isa_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "w83781d",
+	},
+	.probe = w83781d_isa_probe,
+	.remove = w83781d_isa_remove,
+};
+
+static int __devinit
+w83781d_isa_add_driver(void)
+{
+	int res;
 
 	if (w83781d_isa_found(isa_address)) {
 		res = platform_driver_register(&w83781d_isa_driver);
 		if (res)
-			goto exit_unreg_i2c_driver;
+			goto exit;
 
 		/* Sets global pdev as a side effect */
 		res = w83781d_isa_device_add(isa_address);
 		if (res)
 			goto exit_unreg_isa_driver;
-	}
 
+		/* Use ISA bus to access registers */
+		w83781d_read_value = w83781d_read_value_isa;
+		w83781d_write_value = w83781d_write_value_isa;
+	}
 	return 0;
 
  exit_unreg_isa_driver:
 	platform_driver_unregister(&w83781d_isa_driver);
- exit_unreg_i2c_driver:
-	i2c_del_driver(&w83781d_driver);
  exit:
 	return res;
 }
 
-static void __exit
-sensors_w83781d_exit(void)
+static __devexit void w83781d_isa_del_driver(void)
 {
 	if (pdev) {
 		platform_device_unregister(pdev);
 		platform_driver_unregister(&w83781d_isa_driver);
 	}
+}
+
+#else /* !CONFIG_ISA */
+
+static int w83781d_isa_add_driver(void)
+{
+	return 0;
+}
+
+static void w83781d_isa_del_driver(void)
+{
+}
+
+#endif /* CONFIG_ISA */
+
+static int __init
+sensors_w83781d_init(void)
+{
+	int res;
+
+	res = i2c_add_driver(&w83781d_driver);
+	if (res)
+		goto exit;
+
+	res = w83781d_isa_add_driver();
+	if (res)
+		goto exit_unreg_i2c_driver;
+
+	return 0;
+
+ exit_unreg_i2c_driver:
+	i2c_del_driver(&w83781d_driver);
+ exit:
+	return res;
+}
+
+static void __exit
+sensors_w83781d_exit(void)
+{
+	w83781d_isa_del_driver();
 	i2c_del_driver(&w83781d_driver);
 }
 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Jean Delvare]

Hi Wolfgang,

On Wed, 13 Aug 2008 12:14:29 +0200, Wolfgang Grandegger wrote:
> Jean Delvare wrote:
> > On Tue, 12 Aug 2008 11:28:54 +0200, Wolfgang Grandegger wrote:
> >> Jean Delvare wrote:
> >>> Hi Wolfgang,
> >>>
> >>> On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote:
> >>>> We need support for the MON35W42 on an embedded MPC8544 board, which 
> >>>> seems to be compatible with the w83782d. There are more issues, e.g. the 
> >>>> early ISA bus probing of the existing driver hangs the system. Maybe i2c 
> >>>> probing should be done first!?
> >>> If both interfaces are available, we want to use the ISA one, because
> >>> it's much faster, so it must be registered first.
> >>>
> >>> What could be done though would be disabling the ISA interface
> >>> altogether on architectures where it isn't supported (in particular
> >>> PPC.) This shouldn't be too difficult. If you write a patch doing this,
> >>> I'll be happy to review it. Affected drivers would be w83781d and lm78.
> >> Some PPC have an ISA bus as well:
> >>
> >>    http://lxr.linux.no/linux+v2.6.26.2/arch/powerpc/Kconfig#L494
> >>
> >> I think selecting the ISA code with '#ifdef CONFIG_ISA' would be the 
> >> right solution?
> > 
> > Agreed.
> 
> See the attached patch below. When I have some more free time, I will
> convert the driver to new style as well.

Weren't you supposed to work on top of my 2 patches? Please do,
especially when my patches have an impact on the relation between the
ISA device and potential I2C devices.

Your patch breaks the build with CONFIG_ISA=y:

  CC [M]  drivers/hwmon/w83781d.o
drivers/hwmon/w83781d.c: In function `w83781d_read_value_i2c_isa':
drivers/hwmon/w83781d.c:1668: error: `client' undeclared (first use in this function)
drivers/hwmon/w83781d.c:1668: error: (Each undeclared identifier is reported only once
drivers/hwmon/w83781d.c:1668: error: for each function it appears in.)
drivers/hwmon/w83781d.c:1660: warning: unused variable `bank'
drivers/hwmon/w83781d.c: In function `w83781d_write_value_i2c_isa':
drivers/hwmon/w83781d.c:1694: warning: unused variable `bank'
drivers/hwmon/w83781d.c:1695: warning: unused variable `cl'
drivers/hwmon/w83781d.c: In function `w83781d_isa_add_driver':
drivers/hwmon/w83781d.c:1967: error: `w83781d_read_value_isa' undeclared (first use in this function)
drivers/hwmon/w83781d.c:1968: error: `w83781d_write_value_isa' undeclared (first use in this function)
drivers/hwmon/w83781d.c: At top level:
drivers/hwmon/w83781d.c:1659: warning: 'w83781d_read_value_i2c_isa' defined but not used
drivers/hwmon/w83781d.c:1692: warning: 'w83781d_write_value_i2c_isa' defined but not used
make[2]: *** [drivers/hwmon/w83781d.o] Error 1
make[1]: *** [drivers/hwmon] Error 2
make: *** [drivers] Error 2

Apparently you didn't test that case - you really have to.

I am surprised by the size of your patch. I expected something much
smaller and less intrusive than that. As I understand it, you tried to
optimize the case CONFIG_ISA=n. Did you check how much exactly this was
saving? I guess not much.

I don't think that going to this level of optimization is needed. Or,
if you insist on doing it, that would be a separate patch. The first
patch should really only move things around to group them inside the
#ifdef/#endif, and skip the detection of the ISA device and
registration of the platform driver. You shouldn't have to modify the
actual driver code.

> (...)
> +static int __devinit
> +w83781d_isa_add_driver(void)
> +{
> +	int res;
>  
>  	if (w83781d_isa_found(isa_address)) {
>  		res = platform_driver_register(&w83781d_isa_driver);
>  		if (res)
> -			goto exit_unreg_i2c_driver;
> +			goto exit;
>  
>  		/* Sets global pdev as a side effect */
>  		res = w83781d_isa_device_add(isa_address);
>  		if (res)
>  			goto exit_unreg_isa_driver;
> -	}
>  
> +		/* Use ISA bus to access registers */
> +		w83781d_read_value = w83781d_read_value_isa;
> +		w83781d_write_value = w83781d_write_value_isa;

You can't do that. It is possible, and perfectly valid, to have two
supported chips on the same system, one on the ISA bus and one on an
I2C bus. I do have a system like that, with a W83781D on the
motherboard's ISA bus and a W83783S on the graphics adapter. The bus
type (and thus the register access method) is really a chip-specific
and not driver-wide.

> +	}
>  	return 0;
>  
>   exit_unreg_isa_driver:
>  	platform_driver_unregister(&w83781d_isa_driver);
> - exit_unreg_i2c_driver:
> -	i2c_del_driver(&w83781d_driver);
>   exit:
>  	return res;
>  }

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Wolfgang Grandegger]

Jean Delvare wrote:
> Hi Wolfgang,
> 
> On Wed, 13 Aug 2008 12:14:29 +0200, Wolfgang Grandegger wrote:
>> Jean Delvare wrote:
>>> On Tue, 12 Aug 2008 11:28:54 +0200, Wolfgang Grandegger wrote:
>>>> Jean Delvare wrote:
>>>>> Hi Wolfgang,
>>>>>
>>>>> On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote:
>>>>>> We need support for the MON35W42 on an embedded MPC8544 board, which 
>>>>>> seems to be compatible with the w83782d. There are more issues, e.g. the 
>>>>>> early ISA bus probing of the existing driver hangs the system. Maybe i2c 
>>>>>> probing should be done first!?
>>>>> If both interfaces are available, we want to use the ISA one, because
>>>>> it's much faster, so it must be registered first.
>>>>>
>>>>> What could be done though would be disabling the ISA interface
>>>>> altogether on architectures where it isn't supported (in particular
>>>>> PPC.) This shouldn't be too difficult. If you write a patch doing this,
>>>>> I'll be happy to review it. Affected drivers would be w83781d and lm78.
>>>> Some PPC have an ISA bus as well:
>>>>
>>>>    http://lxr.linux.no/linux+v2.6.26.2/arch/powerpc/Kconfig#L494
>>>>
>>>> I think selecting the ISA code with '#ifdef CONFIG_ISA' would be the 
>>>> right solution?
>>> Agreed.
>> See the attached patch below. When I have some more free time, I will
>> convert the driver to new style as well.
> 
> Weren't you supposed to work on top of my 2 patches? Please do,
> especially when my patches have an impact on the relation between the
> ISA device and potential I2C devices.

Sorry, I forgot about them :-(.

> 
> Your patch breaks the build with CONFIG_ISA=y:
> 
>   CC [M]  drivers/hwmon/w83781d.o
> drivers/hwmon/w83781d.c: In function `w83781d_read_value_i2c_isa':
> drivers/hwmon/w83781d.c:1668: error: `client' undeclared (first use in this function)
> drivers/hwmon/w83781d.c:1668: error: (Each undeclared identifier is reported only once
> drivers/hwmon/w83781d.c:1668: error: for each function it appears in.)
> drivers/hwmon/w83781d.c:1660: warning: unused variable `bank'
> drivers/hwmon/w83781d.c: In function `w83781d_write_value_i2c_isa':
> drivers/hwmon/w83781d.c:1694: warning: unused variable `bank'
> drivers/hwmon/w83781d.c:1695: warning: unused variable `cl'
> drivers/hwmon/w83781d.c: In function `w83781d_isa_add_driver':
> drivers/hwmon/w83781d.c:1967: error: `w83781d_read_value_isa' undeclared (first use in this function)
> drivers/hwmon/w83781d.c:1968: error: `w83781d_write_value_isa' undeclared (first use in this function)
> drivers/hwmon/w83781d.c: At top level:
> drivers/hwmon/w83781d.c:1659: warning: 'w83781d_read_value_i2c_isa' defined but not used
> drivers/hwmon/w83781d.c:1692: warning: 'w83781d_write_value_i2c_isa' defined but not used
> make[2]: *** [drivers/hwmon/w83781d.o] Error 1
> make[1]: *** [drivers/hwmon] Error 2
> make: *** [drivers] Error 2

> Apparently you didn't test that case - you really have to.

I thought I did but... Anyway, I cannot really test the ISA interface 
because I just have an embedded PowerPC board with a w83782.

> I am surprised by the size of your patch. I expected something much
> smaller and less intrusive than that. As I understand it, you tried to
> optimize the case CONFIG_ISA=n. Did you check how much exactly this was
> saving? I guess not much.

I mainly moved all ISA related code within one #ifdef ... #endif block 
to avoild plenty of #ifdef's. Furthermore I splitted up the register 
read and write function for i2c and isa and introduced function pointers 
for w83781d_read_value and w83781d_write_value.

> I don't think that going to this level of optimization is needed. Or,
> if you insist on doing it, that would be a separate patch. The first
> patch should really only move things around to group them inside the
> #ifdef/#endif, and skip the detection of the ISA device and
> registration of the platform driver. You shouldn't have to modify the
> actual driver code.

I did not do any optimization saving code.

>> (...)
>> +static int __devinit
>> +w83781d_isa_add_driver(void)
>> +{
>> +	int res;
>>  
>>  	if (w83781d_isa_found(isa_address)) {
>>  		res = platform_driver_register(&w83781d_isa_driver);
>>  		if (res)
>> -			goto exit_unreg_i2c_driver;
>> +			goto exit;
>>  
>>  		/* Sets global pdev as a side effect */
>>  		res = w83781d_isa_device_add(isa_address);
>>  		if (res)
>>  			goto exit_unreg_isa_driver;
>> -	}
>>  
>> +		/* Use ISA bus to access registers */
>> +		w83781d_read_value = w83781d_read_value_isa;
>> +		w83781d_write_value = w83781d_write_value_isa;
> 
> You can't do that. It is possible, and perfectly valid, to have two
> supported chips on the same system, one on the ISA bus and one on an
> I2C bus. I do have a system like that, with a W83781D on the
> motherboard's ISA bus and a W83783S on the graphics adapter. The bus
> type (and thus the register access method) is really a chip-specific
> and not driver-wide.

I do not see a functional difference to the existing driver. If ISA 
probing is  successful, the device will be accessd with ISA bus, 
otherwise via I2C bus. Did I miss something?

Sorry for the bad quality of my patch. It was done in a hurry :-(.

Wolfgang.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Wolfgang Grandegger]

Wolfgang Grandegger wrote:
> Jean Delvare wrote:
>> Hi Wolfgang,
>>
>> On Wed, 13 Aug 2008 12:14:29 +0200, Wolfgang Grandegger wrote:
>>> Jean Delvare wrote:
>>>> On Tue, 12 Aug 2008 11:28:54 +0200, Wolfgang Grandegger wrote:
>>>>> Jean Delvare wrote:
>>>>>> Hi Wolfgang,
>>>>>>
>>>>>> On Tue, 12 Aug 2008 10:55:58 +0200, Wolfgang Grandegger wrote:
>>>>>>> We need support for the MON35W42 on an embedded MPC8544 board, which 
>>>>>>> seems to be compatible with the w83782d. There are more issues, e.g. the 
>>>>>>> early ISA bus probing of the existing driver hangs the system. Maybe i2c 
>>>>>>> probing should be done first!?
>>>>>> If both interfaces are available, we want to use the ISA one, because
>>>>>> it's much faster, so it must be registered first.
>>>>>>
>>>>>> What could be done though would be disabling the ISA interface
>>>>>> altogether on architectures where it isn't supported (in particular
>>>>>> PPC.) This shouldn't be too difficult. If you write a patch doing this,
>>>>>> I'll be happy to review it. Affected drivers would be w83781d and lm78.
>>>>> Some PPC have an ISA bus as well:
>>>>>
>>>>>    http://lxr.linux.no/linux+v2.6.26.2/arch/powerpc/Kconfig#L494
>>>>>
>>>>> I think selecting the ISA code with '#ifdef CONFIG_ISA' would be the 
>>>>> right solution?
>>>> Agreed.
>>> See the attached patch below. When I have some more free time, I will
>>> convert the driver to new style as well.
>> Weren't you supposed to work on top of my 2 patches? Please do,
>> especially when my patches have an impact on the relation between the
>> ISA device and potential I2C devices.
> 
> Sorry, I forgot about them :-(.
> 
>> Your patch breaks the build with CONFIG_ISA=y:
>>
>>   CC [M]  drivers/hwmon/w83781d.o
>> drivers/hwmon/w83781d.c: In function `w83781d_read_value_i2c_isa':
>> drivers/hwmon/w83781d.c:1668: error: `client' undeclared (first use in this function)
>> drivers/hwmon/w83781d.c:1668: error: (Each undeclared identifier is reported only once
>> drivers/hwmon/w83781d.c:1668: error: for each function it appears in.)
>> drivers/hwmon/w83781d.c:1660: warning: unused variable `bank'
>> drivers/hwmon/w83781d.c: In function `w83781d_write_value_i2c_isa':
>> drivers/hwmon/w83781d.c:1694: warning: unused variable `bank'
>> drivers/hwmon/w83781d.c:1695: warning: unused variable `cl'
>> drivers/hwmon/w83781d.c: In function `w83781d_isa_add_driver':
>> drivers/hwmon/w83781d.c:1967: error: `w83781d_read_value_isa' undeclared (first use in this function)
>> drivers/hwmon/w83781d.c:1968: error: `w83781d_write_value_isa' undeclared (first use in this function)
>> drivers/hwmon/w83781d.c: At top level:
>> drivers/hwmon/w83781d.c:1659: warning: 'w83781d_read_value_i2c_isa' defined but not used
>> drivers/hwmon/w83781d.c:1692: warning: 'w83781d_write_value_i2c_isa' defined but not used
>> make[2]: *** [drivers/hwmon/w83781d.o] Error 1
>> make[1]: *** [drivers/hwmon] Error 2
>> make: *** [drivers] Error 2
> 
>> Apparently you didn't test that case - you really have to.
> 
> I thought I did but... Anyway, I cannot really test the ISA interface 
> because I just have an embedded PowerPC board with a w83782.
> 
>> I am surprised by the size of your patch. I expected something much
>> smaller and less intrusive than that. As I understand it, you tried to
>> optimize the case CONFIG_ISA=n. Did you check how much exactly this was
>> saving? I guess not much.
> 
> I mainly moved all ISA related code within one #ifdef ... #endif block 
> to avoild plenty of #ifdef's. Furthermore I splitted up the register 
> read and write function for i2c and isa and introduced function pointers 
> for w83781d_read_value and w83781d_write_value.
> 
>> I don't think that going to this level of optimization is needed. Or,
>> if you insist on doing it, that would be a separate patch. The first
>> patch should really only move things around to group them inside the
>> #ifdef/#endif, and skip the detection of the ISA device and
>> registration of the platform driver. You shouldn't have to modify the
>> actual driver code.
> 
> I did not do any optimization saving code.
> 
>>> (...)
>>> +static int __devinit
>>> +w83781d_isa_add_driver(void)
>>> +{
>>> +	int res;
>>>  
>>>  	if (w83781d_isa_found(isa_address)) {
>>>  		res = platform_driver_register(&w83781d_isa_driver);
>>>  		if (res)
>>> -			goto exit_unreg_i2c_driver;
>>> +			goto exit;
>>>  
>>>  		/* Sets global pdev as a side effect */
>>>  		res = w83781d_isa_device_add(isa_address);
>>>  		if (res)
>>>  			goto exit_unreg_isa_driver;
>>> -	}
>>>  
>>> +		/* Use ISA bus to access registers */
>>> +		w83781d_read_value = w83781d_read_value_isa;
>>> +		w83781d_write_value = w83781d_write_value_isa;
>> You can't do that. It is possible, and perfectly valid, to have two
>> supported chips on the same system, one on the ISA bus and one on an
>> I2C bus. I do have a system like that, with a W83781D on the
>> motherboard's ISA bus and a W83783S on the graphics adapter. The bus
>> type (and thus the register access method) is really a chip-specific
>> and not driver-wide.
> 
> I do not see a functional difference to the existing driver. If ISA 
> probing is  successful, the device will be accessd with ISA bus, 
> otherwise via I2C bus. Did I miss something?

Obviously I missed something :-(. The function pointers must be per 
device, of course. I'm going to prepare a patch with just #idef's added 
where necessary. A more comprehensive restructuring could be done when 
converting to new-style.

Wolfgang.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] New style driver for w83781d?

[Jean Delvare]

On Wed, 13 Aug 2008 15:48:14 +0200, Wolfgang Grandegger wrote:
> Jean Delvare wrote:
> > Your patch breaks the build with CONFIG_ISA=y:
> > 
> >   CC [M]  drivers/hwmon/w83781d.o
> > drivers/hwmon/w83781d.c: In function `w83781d_read_value_i2c_isa':
> > drivers/hwmon/w83781d.c:1668: error: `client' undeclared (first use in this function)
> > drivers/hwmon/w83781d.c:1668: error: (Each undeclared identifier is reported only once
> > drivers/hwmon/w83781d.c:1668: error: for each function it appears in.)
> > drivers/hwmon/w83781d.c:1660: warning: unused variable `bank'
> > drivers/hwmon/w83781d.c: In function `w83781d_write_value_i2c_isa':
> > drivers/hwmon/w83781d.c:1694: warning: unused variable `bank'
> > drivers/hwmon/w83781d.c:1695: warning: unused variable `cl'
> > drivers/hwmon/w83781d.c: In function `w83781d_isa_add_driver':
> > drivers/hwmon/w83781d.c:1967: error: `w83781d_read_value_isa' undeclared (first use in this function)
> > drivers/hwmon/w83781d.c:1968: error: `w83781d_write_value_isa' undeclared (first use in this function)
> > drivers/hwmon/w83781d.c: At top level:
> > drivers/hwmon/w83781d.c:1659: warning: 'w83781d_read_value_i2c_isa' defined but not used
> > drivers/hwmon/w83781d.c:1692: warning: 'w83781d_write_value_i2c_isa' defined but not used
> > make[2]: *** [drivers/hwmon/w83781d.o] Error 1
> > make[1]: *** [drivers/hwmon] Error 2
> > make: *** [drivers] Error 2
> 
> > Apparently you didn't test that case - you really have to.
> 
> I thought I did but... Anyway, I cannot really test the ISA interface 
> because I just have an embedded PowerPC board with a w83782.

I can do the testing, no problem with that. Please just make sure that
it builds OK.

> > (...)
> > I don't think that going to this level of optimization is needed. Or,
> > if you insist on doing it, that would be a separate patch. The first
> > patch should really only move things around to group them inside the
> > #ifdef/#endif, and skip the detection of the ISA device and
> > registration of the platform driver. You shouldn't have to modify the
> > actual driver code.
> 
> I did not do any optimization saving code.

I was referring to the split of the register access functions.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors