i2c-parport: Add SMBus alert support for ADM1032EB

RFC In review Jean Delvare 2 participants 4 messages Nov 21, 2008 – Nov 25, 2008 3 messages not in the archive

Subsystems (4)

I2C subsystem host drivers Hardware monitoring LM90 hardware monitor driver I2C over parallel port

lore

Share & tools

lore

Jean Delvare

author

2008-11-21 20:29 UTC #0 lore raw

RFC 1/1 i2c-parport: Add SMBus alert support for ADM1032EB

Hi David,

Here is my attempt to add support for the SMBus alert mechanism to my ADM1032 evaluation board (ADM1032 thermal sensor over parallel port.) I'm not totally sure about the order in which things should be done in i2c_parport_attach() and i2c_parport_detach() (in particular whether irq should be enabled/disabled before/after registering/unregistering the i2c adapter) but all in all it seems to work fine for me. Can you please take a look and tell me if you see any mistake? Thanks.

* * * * *

Add support for the SMBus alert mechanism on the ADM1032 evaluation board. At this point this is more of a proof-of-concept, we don't do anything terribly useful on SMBus alert: we simply log the event. But this could later evolve into libsensors signaling so that user-space applications can take an appropriate action.

Signed-off-by Jean Delvare <[email protected]>

Cc David Brownell <[email protected]>

---
 drivers/hwmon/lm90.c             |   28 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-parport.c |   26 ++++++++++++++++++++++----
 drivers/i2c/busses/i2c-parport.h |    4 +++-
 3 files changed, 53 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-parport.c +22 -4

@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.c I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2007 Jean Delvare <[email protected]>
+   Copyright (C) 2003-2008 Jean Delvare <[email protected]>
    
    Based on older i2c-philips-par.c driver
    Copyright (C) 1995-2000 Simon G. Vogl

@@ -143,6 +143,14 @@ static struct i2c_algo_bit_data parport_
 
 /* ----- I2c and parallel port call-back functions and structures --------- */
 
+void i2c_parport_irq(void *data)
+{
+	struct i2c_par *adapter = data;
+
+	dev_dbg(&adapter->adapter.dev, "SMBus alert received\n");
+	schedule_work(&adapter->adapter.alert);
+}
+
 static void i2c_parport_attach (struct parport *port)
 {
 	struct i2c_par *adapter;

@@ -155,7 +163,7 @@ static void i2c_parport_attach (struct p
 
 	pr_debug("i2c-parport: attaching to %s\n", port->name);
 	adapter->pdev = parport_register_device(port, "i2c-parport",
-		NULL, NULL, NULL, PARPORT_FLAG_EXCL, NULL);
+		NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto ERROR0;

@@ -177,7 +185,7 @@ static void i2c_parport_attach (struct p
 	adapter->adapter.algo_data = &adapter->algo_data;
 	adapter->adapter.dev.parent = port->physport->dev;
 
-	if (parport_claim_or_block(adapter->pdev) < 0) {
+	if (parport_claim(adapter->pdev) < 0) {
 		printk(KERN_ERR "i2c-parport: Could not claim parallel port\n");
 		goto ERROR1;
 	}

@@ -189,12 +197,18 @@ static void i2c_parport_attach (struct p
 	if (adapter_parm[type].init.val)
 		line_set(port, 1, &adapter_parm[type].init);
 
-	parport_release(adapter->pdev);
+	/* Setup SMBus alert if supported */
+	if (adapter_parm[type].smbus_alert) {
+		adapter->adapter.do_setup_alert = 1;
+		adapter->adapter.alert_edge_triggered = 1;
+	}
 
 	if (i2c_bit_add_bus(&adapter->adapter) < 0) {
 		printk(KERN_ERR "i2c-parport: Unable to register with I2C\n");
 		goto ERROR1;
 	}
+	if (adapter->adapter.do_setup_alert)
+		parport_enable_irq(port);
 
 	/* Add the new adapter to the list */
 	adapter->next = adapter_list;

@@ -202,6 +216,7 @@ static void i2c_parport_attach (struct p
         return;
 
 ERROR1:
+	parport_release(adapter->pdev);
 	parport_unregister_device(adapter->pdev);
 ERROR0:
 	kfree(adapter);

@@ -215,12 +230,15 @@ static void i2c_parport_detach (struct p
 	for (prev = NULL, adapter = adapter_list; adapter;
 	     prev = adapter, adapter = adapter->next) {
 		if (adapter->pdev->port == port) {
+			if (adapter->adapter.do_setup_alert)
+				parport_disable_irq(port);
 			i2c_del_adapter(&adapter->adapter);
 
 			/* Un-init if needed (power off...) */
 			if (adapter_parm[type].init.val)
 				line_set(port, 0, &adapter_parm[type].init);
 				
+			parport_release(adapter->pdev);
 			parport_unregister_device(adapter->pdev);
 			if (prev)
 				prev->next = adapter->next;

drivers/i2c/busses/i2c-parport.h +3 -1

@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.h I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2004 Jean Delvare <[email protected]>
+   Copyright (C) 2003-2008 Jean Delvare <[email protected]>
    
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by

@@ -38,6 +38,7 @@ struct adapter_parm {
 	struct lineop getsda;
 	struct lineop getscl;
 	struct lineop init;
+	int smbus_alert;
 };
 
 static struct adapter_parm adapter_parm[] = {

@@ -73,6 +74,7 @@ static struct adapter_parm adapter_parm[
 		.setscl	= { 0x01, DATA, 1 },
 		.getsda	= { 0x10, STAT, 1 },
 		.init	= { 0xf0, DATA, 0 },
+		.smbus_alert = 1,
 	},
 	/* type 5: ADM1025, ADM1030 and ADM1031 evaluation boards */
 	{

drivers/hwmon/lm90.c +28 -0

@@ -157,6 +157,7 @@ static int lm90_detect(struct i2c_client
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id);
 static void lm90_init_client(struct i2c_client *client);
+static void lm90_alert(struct i2c_client *client, bool flag);
 static int lm90_remove(struct i2c_client *client);
 static struct lm90_data *lm90_update_device(struct device *dev);

@@ -190,6 +191,7 @@ static struct i2c_driver lm90_driver = {
 	},
 	.probe		= lm90_probe,
 	.remove		= lm90_remove,
+	.alert		= lm90_alert,
 	.id_table	= lm90_id,
 	.detect		= lm90_detect,
 	.address_data	= &addr_data,

@@ -921,6 +923,18 @@ static int lm90_remove(struct i2c_client
 	return 0;
 }
 
+static void lm90_alert(struct i2c_client *client, bool flag)
+{
+	u8 config;
+
+	/* Disable ALERT# output */
+	lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+	config |= 0x80;
+	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
+
+	dev_warn(&client->dev, "Temperature out of range, please check!\n");
+}
+
 static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
 {
 	int err;

@@ -1008,6 +1022,20 @@ static struct lm90_data *lm90_update_dev
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
 
+		/* Re-enable ALERT# output if alarms are all clear */
+		if ((data->alarms & 0x7c) == 0) {
+			u8 config;
+
+			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+			if (config & 0x80) {
+				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
+				config &= ~0x80;
+				i2c_smbus_write_byte_data(client,
+							  LM90_REG_W_CONFIG1,
+							  config);
+			}
+		}
+
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Brownell

2008-11-22 21:18 UTC #2 lore raw

Quoted 8 lines · Jean Delvare

Hi David,
Here is my attempt to add support for the SMBus alert mechanism to my ADM1032 evaluation board (ADM1032 thermal sensor over parallel port.) I'm not totally sure about the order in which things should be done in i2c_parport_attach() and i2c_parport_detach() (in particular whether irq should be enabled/disabled before/after registering/unregistering the i2c adapter) but all in all it seems to work fine for me.

General policy is not to enable IRQs until you're ready to handle them ... so "after", like you do it, is good here.

Quoted 2 lines

Can you
please take a look and tell me if you see any mistake? Thanks.

You're going the "bus adapter handles IRQ" route, and in this case you can rely on the parport framework and the fact that this seems to be edge-triggered to cover up an interface glitch.

It'd be simpler to use the parport-light code, and delegate all that handling to i2c-core. After all, you're not likely to have a collection of such i2c adapter dongles sharing a single parallel port ...

Alternatively, maybe *both* parport adapter drivers should be updated.

- Dave

Quoted 7 lines

* * * * *
Add support for the SMBus alert mechanism on the ADM1032 evaluation board. At this point this is more of a proof-of-concept, we don't do anything terribly useful on SMBus alert: we simply log the event. But this could later evolve into libsensors signaling so that user-space applications can take an appropriate action.

And there are non-alert changes too...

Quoted 21 lines

Signed-off-by Jean Delvare <[email protected]>

Cc David Brownell <[email protected]>

---
 drivers/hwmon/lm90.c             |   28 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-parport.c |   26 ++++++++++++++++++++++----
 drivers/i2c/busses/i2c-parport.h |    4 +++-
 3 files changed, 53 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-parport.c +1 -1

@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.c I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2007 Jean Delvare <[email protected]>
+   Copyright (C) 2003-2008 Jean Delvare <[email protected]>
 
    Based on older i2c-philips-par.c driver
    Copyright (C) 1995-2000 Simon G. Vogl

drivers/i2c/busses/i2c-parport.c static struct i2c_algo_bit_data parport_ · line 152

Quoted 10 lines

drivers/i2c/busses/i2c-parport.c +7 -0

@@ -143,6 +143,14 @@ static struct i2c_algo_bit_data parport_
 
 /* ----- I2c and parallel port call-back functions and structures --------- */
 
+void i2c_parport_irq(void *data)
+{
+	struct i2c_par *adapter = data;
+
+	dev_dbg(&adapter->adapter.dev, "SMBus alert received\n");
+	schedule_work(&adapter->adapter.alert);
+}

It'd be nice if this weren't needed, but the parport stack seems to force indirection like this.

Quoted 14 lines

drivers/i2c/busses/i2c-parport.c +2 -1

+
 static void i2c_parport_attach (struct parport *port)
 {
 	struct i2c_par *adapter;

@@ -155,7 +163,7 @@ static void i2c_parport_attach (struct p
 
 	pr_debug("i2c-parport: attaching to %s\n", port->name);
 	adapter->pdev = parport_register_device(port, "i2c-parport",
-		NULL, NULL, NULL, PARPORT_FLAG_EXCL, NULL);
+		NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto ERROR0;

drivers/i2c/busses/i2c-parport.c static void i2c_parport_attach (struct p · line 188

Quoted 5 lines

drivers/i2c/busses/i2c-parport.c +1 -1

@@ -177,7 +185,7 @@ static void i2c_parport_attach (struct p
 	adapter->adapter.algo_data = &adapter->algo_data;
 	adapter->adapter.dev.parent = port->physport->dev;
 
-	if (parport_claim_or_block(adapter->pdev) < 0) {
+	if (parport_claim(adapter->pdev) < 0) {

... unrelated change ...

drivers/i2c/busses/i2c-parport.c static void i2c_parport_attach (struct p · line 203

Quoted 12 lines

drivers/i2c/busses/i2c-parport.c +4 -1

 		printk(KERN_ERR "i2c-parport: Could not claim parallel port\n");
 		goto ERROR1;
 	}

@@ -189,12 +197,18 @@ static void i2c_parport_attach (struct p
 	if (adapter_parm[type].init.val)
 		line_set(port, 1, &adapter_parm[type].init);
 
-	parport_release(adapter->pdev);
+	/* Setup SMBus alert if supported */
+	if (adapter_parm[type].smbus_alert) {
+		adapter->adapter.do_setup_alert = 1;
+		adapter->adapter.alert_edge_triggered = 1;

The fact that you need to set this trigger flag reflects a bug in the SMBALERT# patch. Since "you" are handling the IRQ, it has no business caring about that. It should suffice to set the do_setup_alert flag.

Quoted 12 lines

drivers/i2c/busses/i2c-parport.c +3 -0

+	}
 
 	if (i2c_bit_add_bus(&adapter->adapter) < 0) {
 		printk(KERN_ERR "i2c-parport: Unable to register with I2C\n");
 		goto ERROR1;
 	}
+	if (adapter->adapter.do_setup_alert)
+		parport_enable_irq(port);
 
 	/* Add the new adapter to the list */
 	adapter->next = adapter_list;

drivers/i2c/busses/i2c-parport.c static void i2c_parport_attach (struct p · line 219

Quoted 4 lines

drivers/i2c/busses/i2c-parport.c +1 -0

@@ -202,6 +216,7 @@ static void i2c_parport_attach (struct p
         return;
 
 ERROR1:
+	parport_release(adapter->pdev);

This presumably goes with the unrelated change above, dropping the "_or_block" ...

Quoted 6 lines

drivers/i2c/busses/i2c-parport.c +0 -0

 	parport_unregister_device(adapter->pdev);
 ERROR0:
 	kfree(adapter);

@@ -215,12 +230,15 @@ static void i2c_parport_detach (struct p
 	for (prev = NULL, adapter = adapter_list; adapter;
 	     prev = adapter, adapter = adapter->next) {

drivers/i2c/busses/i2c-parport.c static void i2c_parport_detach (struct p · line 241

Quoted 10 lines

drivers/i2c/busses/i2c-parport.c +3 -0

 		if (adapter->pdev->port == port) {
+			if (adapter->adapter.do_setup_alert)
+				parport_disable_irq(port);
 			i2c_del_adapter(&adapter->adapter);
 
 			/* Un-init if needed (power off...) */
 			if (adapter_parm[type].init.val)
 				line_set(port, 0, &adapter_parm[type].init);
 
+			parport_release(adapter->pdev);

... ditto ...

drivers/i2c/busses/i2c-parport.h

Quoted 5 lines

drivers/i2c/busses/i2c-parport.c +0 -0
 			parport_unregister_device(adapter->pdev);
 			if (prev)
 				prev->next = adapter->next;
drivers/i2c/busses/i2c-parport.h +0 -0

This change would seem to be shared between the two parport drivers ... maybe it should be a separate patch, paired with patches for the individual parport drivers.

drivers/hwmon/lm90.c

Quoted 27 lines

drivers/i2c/busses/i2c-parport.h +3 -1

@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.h I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2004 Jean Delvare <[email protected]>
+   Copyright (C) 2003-2008 Jean Delvare <[email protected]>
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by

@@ -38,6 +38,7 @@ struct adapter_parm {
 	struct lineop getsda;
 	struct lineop getscl;
 	struct lineop init;
+	int smbus_alert;
 };
 
 static struct adapter_parm adapter_parm[] = {

@@ -73,6 +74,7 @@ static struct adapter_parm adapter_parm[
 		.setscl	= { 0x01, DATA, 1 },
 		.getsda	= { 0x10, STAT, 1 },
 		.init	= { 0xf0, DATA, 0 },
+		.smbus_alert = 1,
 	},
 	/* type 5: ADM1025, ADM1030 and ADM1031 evaluation boards */
 	{

drivers/hwmon/lm90.c +0 -0

Separate patch?

drivers/hwmon/lm90.c static int lm90_detect(struct i2c_client · line 160

Quoted 5 lines

drivers/hwmon/lm90.c +1 -0

@@ -157,6 +157,7 @@ static int lm90_detect(struct i2c_client
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id);
 static void lm90_init_client(struct i2c_client *client);
+static void lm90_alert(struct i2c_client *client, bool flag);

Forward declarations. We hates them ... we hates them forever!!

drivers/hwmon/lm90.c static struct i2c_driver lm90_driver = { · line 197

Quoted 11 lines

drivers/hwmon/lm90.c +1 -0

 static int lm90_remove(struct i2c_client *client);
 static struct lm90_data *lm90_update_device(struct device *dev);

@@ -190,6 +191,7 @@ static struct i2c_driver lm90_driver = {
 	},
 	.probe		= lm90_probe,
 	.remove		= lm90_remove,
+	.alert		= lm90_alert,
 	.id_table	= lm90_id,
 	.detect		= lm90_detect,
 	.address_data	= &addr_data,

... move this after all the functions, and the forward decls can vanish; as a nice cleanup, someday.

drivers/hwmon/lm90.c static int lm90_remove(struct i2c_client · line 930

Quoted 9 lines

drivers/hwmon/lm90.c +5 -0

@@ -921,6 +923,18 @@ static int lm90_remove(struct i2c_client
 	return 0;
 }
 
+static void lm90_alert(struct i2c_client *client, bool flag)
+{
+	u8 config;
+
+	/* Disable ALERT# output */

... "because this chip doesn't implement SMBALERT# correctly; it should only holding the alert line low briefly."

Quoted 37 lines

drivers/hwmon/lm90.c +21 -0

+	lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+	config |= 0x80;
+	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
+
+	dev_warn(&client->dev, "Temperature out of range, please check!\n");
+}
+
 static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
 {
 	int err;

@@ -1008,6 +1022,20 @@ static struct lm90_data *lm90_update_dev
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
 
+		/* Re-enable ALERT# output if alarms are all clear */
+		if ((data->alarms & 0x7c) == 0) {
+			u8 config;
+
+			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+			if (config & 0x80) {
+				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
+				config &= ~0x80;
+				i2c_smbus_write_byte_data(client,
+							  LM90_REG_W_CONFIG1,
+							  config);
+			}
+		}
+
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}

Signature · 2 lines

-- 
Jean Delvare

A message that is not in the archive (sent off-list or not yet received): 200811221318.44271.david-b-yBeKhBN/[email protected]

Jean Delvare

author

2008-11-25 10:05 UTC #4 lore raw

Hi David,

Thanks for your review and comments.

Quoted 16 lines · David Brownell

Quoted 8 lines · Jean Delvare
Hi David,
Here is my attempt to add support for the SMBus alert mechanism to my ADM1032 evaluation board (ADM1032 thermal sensor over parallel port.) I'm not totally sure about the order in which things should be done in i2c_parport_attach() and i2c_parport_detach() (in particular whether irq should be enabled/disabled before/after registering/unregistering the i2c adapter) but all in all it seems to work fine for me.
General policy is not to enable IRQs until you're ready to handle them ... so "after", like you do it, is good here.
Quoted 2 lines
Can you
please take a look and tell me if you see any mistake? Thanks.

Quoted 8 lines

You're going the "bus adapter handles IRQ" route, and in this case you can rely on the parport framework and the fact that this seems to be edge-triggered to cover up an interface glitch.
It'd be simpler to use the parport-light code, and delegate all that handling to i2c-core. After all, you're not likely to have a collection of such i2c adapter dongles sharing a single parallel port ...

Even the i2c-parport driver only supports one device at the moment, because we request the parallel port with flag PARPORT_FLAG_EXCL. Honestly I don't really recall why I did that, maybe just paranoia while I was developing the driver and then I forgot to change it. OTOH I'm not sure if it is really possible to grab the parallel port non-exclusively especially now that we can receive an interrupt at about any time.

The daisy chaining of parallel port devices always looked like a hack to me anyway.

Quoted 2 lines

Alternatively, maybe *both* parport adapter drivers should be updated.

Yes, that would make sense. However, I always have a hard time testing i2c-parport-light, as apparently loading the parport_pc driver is enough for the parallel port hardware to lose its pristine state and then i2c-parport-light no longer works. And of course parport_pc tends to load automatically on my systems.

Quoted 62 lines

Quoted 7 lines

* * * * *
Add support for the SMBus alert mechanism on the ADM1032 evaluation board. At this point this is more of a proof-of-concept, we don't do anything terribly useful on SMBus alert: we simply log the event. But this could later evolve into libsensors signaling so that user-space applications can take an appropriate action.

And there are non-alert changes too...

Quoted 20 lines

Signed-off-by Jean Delvare <[email protected]>

Cc David Brownell <[email protected]>

---
 drivers/hwmon/lm90.c             |   28 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-parport.c |   26 ++++++++++++++++++++++----
 drivers/i2c/busses/i2c-parport.h |    4 +++-
 3 files changed, 53 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-parport.c +1 -1

@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.c I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2007 Jean Delvare <[email protected]>
+   Copyright (C) 2003-2008 Jean Delvare <[email protected]>
 
    Based on older i2c-philips-par.c driver
    Copyright (C) 1995-2000 Simon G. Vogl

drivers/i2c/busses/i2c-parport.c static struct i2c_algo_bit_data parport_ · line 152

Quoted 10 lines

drivers/i2c/busses/i2c-parport.c +7 -0

@@ -143,6 +143,14 @@ static struct i2c_algo_bit_data parport_
 
 /* ----- I2c and parallel port call-back functions and structures --------- */
 
+void i2c_parport_irq(void *data)
+{
+	struct i2c_par *adapter = data;
+
+	dev_dbg(&adapter->adapter.dev, "SMBus alert received\n");
+	schedule_work(&adapter->adapter.alert);
+}

It'd be nice if this weren't needed, but the parport stack seems to force indirection like this.

Quoted 17 lines

drivers/i2c/busses/i2c-parport.c +2 -1

+
 static void i2c_parport_attach (struct parport *port)
 {
 	struct i2c_par *adapter;

@@ -155,7 +163,7 @@ static void i2c_parport_attach (struct p
 
 	pr_debug("i2c-parport: attaching to %s\n", port->name);
 	adapter->pdev = parport_register_device(port, "i2c-parport",
-		NULL, NULL, NULL, PARPORT_FLAG_EXCL, NULL);
+		NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter);
 	if (!adapter->pdev) {
 		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
 		goto ERROR0;

@@ -177,7 +185,7 @@ static void i2c_parport_attach (struct p
 	adapter->adapter.algo_data = &adapter->algo_data;
 	adapter->adapter.dev.parent = port->physport->dev;

Quoted 4 lines

drivers/i2c/busses/i2c-parport.c static void i2c_parport_attach (struct p · line 188
Quoted 2 lines
drivers/i2c/busses/i2c-parport.c +1 -1
-	if (parport_claim_or_block(adapter->pdev) < 0) {
+	if (parport_claim(adapter->pdev) < 0) {
... unrelated change ...

Correct, I'll back it out. I'm really curious why I used parport_claim_or_block in the first place. Maybe it doesn't matter given that we use PARPORT_FLAG_EXCL.

Quoted 8 lines

drivers/i2c/busses/i2c-parport.c +0 -0

 		printk(KERN_ERR "i2c-parport: Could not claim parallel port\n");
 		goto ERROR1;
 	}

@@ -189,12 +197,18 @@ static void i2c_parport_attach (struct p
 	if (adapter_parm[type].init.val)
 		line_set(port, 1, &adapter_parm[type].init);

Quoted 10 lines

drivers/i2c/busses/i2c-parport.c static void i2c_parport_attach (struct p · line 203
Quoted 5 lines
drivers/i2c/busses/i2c-parport.c +4 -1
-	parport_release(adapter->pdev);
+	/* Setup SMBus alert if supported */
+	if (adapter_parm[type].smbus_alert) {
+		adapter->adapter.do_setup_alert = 1;
+		adapter->adapter.alert_edge_triggered = 1;
The fact that you need to set this trigger flag reflects a bug in the SMBALERT# patch. Since "you" are handling the IRQ, it has no business caring about that. It should suffice to set the do_setup_alert flag.

I'm unsure about this. For edge-triggered interrupts, you indeed shouldn't do anything in i2c-core if the adapter driver takes care of the interrupt. For level-triggered interrupts though, the interrupt needs to be disabled and re-enabled. smbus_alert() appears to be the right place to re-enable the interrupt. Else how would the adapter driver code know when to re-enable it?

Quoted 14 lines

drivers/i2c/busses/i2c-parport.c +3 -0

+	}
 
 	if (i2c_bit_add_bus(&adapter->adapter) < 0) {
 		printk(KERN_ERR "i2c-parport: Unable to register with I2C\n");
 		goto ERROR1;
 	}
+	if (adapter->adapter.do_setup_alert)
+		parport_enable_irq(port);
 
 	/* Add the new adapter to the list */
 	adapter->next = adapter_list;

@@ -202,6 +216,7 @@ static void i2c_parport_attach (struct p
         return;

Quoted 5 lines

drivers/i2c/busses/i2c-parport.c static void i2c_parport_attach (struct p · line 219
Quoted 2 lines
drivers/i2c/busses/i2c-parport.c +1 -0
 ERROR1:
+	parport_release(adapter->pdev);
This presumably goes with the unrelated change above, dropping the "_or_block" ...

No, this is a different change. This is fixing a bug in the original code, which was releasing the parallel port early during initialization, while I presume we should at least acquire it again for each I2C transfer. But more importantly, we no longer receive interrupts if we have released the parallel port (this is why my patch originally didn't work), so SMBus alert support requires that we grab the parallel port and don't release it before the driver is removed.

Still I agree that it could go in a separate patch for clarity.

Quoted 19 lines

Quoted 6 lines

drivers/i2c/busses/i2c-parport.c +0 -0

 	parport_unregister_device(adapter->pdev);
 ERROR0:
 	kfree(adapter);

@@ -215,12 +230,15 @@ static void i2c_parport_detach (struct p
 	for (prev = NULL, adapter = adapter_list; adapter;
 	     prev = adapter, adapter = adapter->next) {

drivers/i2c/busses/i2c-parport.c static void i2c_parport_detach (struct p · line 241

Quoted 10 lines

drivers/i2c/busses/i2c-parport.c +3 -0

 		if (adapter->pdev->port == port) {
+			if (adapter->adapter.do_setup_alert)
+				parport_disable_irq(port);
 			i2c_del_adapter(&adapter->adapter);
 
 			/* Un-init if needed (power off...) */
 			if (adapter_parm[type].init.val)
 				line_set(port, 0, &adapter_parm[type].init);
 
+			parport_release(adapter->pdev);

... ditto ...

Quoted 9 lines

drivers/i2c/busses/i2c-parport.h
Quoted 5 lines
drivers/i2c/busses/i2c-parport.c +0 -0
 			parport_unregister_device(adapter->pdev);
 			if (prev)
 				prev->next = adapter->next;
drivers/i2c/busses/i2c-parport.h +0 -0
This change would seem to be shared between the two parport drivers ... maybe it should be a separate patch, paired with patches for the individual parport drivers.

i2c-parport.h contains shared device definitions. It is not written anywhere that both drivers must handle all attributes that are listed. So I don't see any problem adding SMBus alert information here for the first driver that needs it, and updating the other driver later, if ever.

I don't really want to split this patch in tiny pieces. Splitting is good, but other-splitting doesn't help.

Quoted 16 lines

drivers/i2c/busses/i2c-parport.h +2 -1

@@ -1,7 +1,7 @@
 /* ------------------------------------------------------------------------ *
  * i2c-parport.h I2C bus over parallel port                                 *
  * ------------------------------------------------------------------------ *
-   Copyright (C) 2003-2004 Jean Delvare <[email protected]>
+   Copyright (C) 2003-2008 Jean Delvare <[email protected]>
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by

@@ -38,6 +38,7 @@ struct adapter_parm {
 	struct lineop getsda;
 	struct lineop getscl;
 	struct lineop init;
+	int smbus_alert;
 };

Quoted 13 lines

drivers/hwmon/lm90.c

Quoted 11 lines

drivers/i2c/busses/i2c-parport.h +1 -0

 static struct adapter_parm adapter_parm[] = {

@@ -73,6 +74,7 @@ static struct adapter_parm adapter_parm[
 		.setscl	= { 0x01, DATA, 1 },
 		.getsda	= { 0x10, STAT, 1 },
 		.init	= { 0xf0, DATA, 0 },
+		.smbus_alert = 1,
 	},
 	/* type 5: ADM1025, ADM1030 and ADM1031 evaluation boards */
 	{

drivers/hwmon/lm90.c +0 -0

Separate patch?

I kept it in the same patch to help review and testing. When I submit it upstream, I will split appropriately.

Note that this is one more reason too make sure that the i2c-core code is robust against misbehaving devices. At some point in time, i2c-parport might support SMBus alert and the lm90 driver might not, or vice-versa.

Quoted 7 lines

drivers/hwmon/lm90.c static int lm90_detect(struct i2c_client · line 160
Quoted 5 lines
drivers/hwmon/lm90.c +1 -0
@@ -157,6 +157,7 @@ static int lm90_detect(struct i2c_client
 static int lm90_probe(struct i2c_client *client,
 		      const struct i2c_device_id *id);
 static void lm90_init_client(struct i2c_client *client);
+static void lm90_alert(struct i2c_client *client, bool flag);
Forward declarations. We hates them ... we hates them forever!!

This is true (but obviously unrelated to this patch.) This is a legacy from the lm-sensors project, many hwmon drivers could be reworked to get rid of them.

Quoted 20 lines

drivers/hwmon/lm90.c static struct i2c_driver lm90_driver = { · line 197
Quoted 11 lines
drivers/hwmon/lm90.c +1 -0
 static int lm90_remove(struct i2c_client *client);
 static struct lm90_data *lm90_update_device(struct device *dev);
 
@@ -190,6 +191,7 @@ static struct i2c_driver lm90_driver = {
 	},
 	.probe		= lm90_probe,
 	.remove		= lm90_remove,
+	.alert		= lm90_alert,
 	.id_table	= lm90_id,
 	.detect		= lm90_detect,
 	.address_data	= &addr_data,
... move this after all the functions, and the forward decls can vanish; as a nice cleanup, someday.
Quoted 4 lines
drivers/hwmon/lm90.c +0 -0
@@ -921,6 +923,18 @@ static int lm90_remove(struct i2c_client
 	return 0;
 }
 

Quoted 8 lines

drivers/hwmon/lm90.c static int lm90_remove(struct i2c_client · line 930
Quoted 5 lines
drivers/hwmon/lm90.c +5 -0
+static void lm90_alert(struct i2c_client *client, bool flag)
+{
+	u8 config;
+
+	/* Disable ALERT# output */
... "because this chip doesn't implement SMBALERT# correctly; it should only holding the alert line low briefly."

Good point, I'll add this to make it clearer.

Quoted 31 lines

drivers/hwmon/lm90.c +21 -0

+	lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+	config |= 0x80;
+	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
+
+	dev_warn(&client->dev, "Temperature out of range, please check!\n");
+}
+
 static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
 {
 	int err;

@@ -1008,6 +1022,20 @@ static struct lm90_data *lm90_update_dev
 		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
 
+		/* Re-enable ALERT# output if alarms are all clear */
+		if ((data->alarms & 0x7c) == 0) {
+			u8 config;
+
+			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+			if (config & 0x80) {
+				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
+				config &= ~0x80;
+				i2c_smbus_write_byte_data(client,
+							  LM90_REG_W_CONFIG1,
+							  config);
+			}
+		}
+
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}

Signature · 1 line

Jean Delvare

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Brownell

2008-11-25 10:45 UTC #6 lore raw

Quoted 13 lines · Jean Delvare

Quoted 4 lines
It'd be simpler to use the parport-light code, and delegate all that handling to i2c-core. After all, you're not likely to have a collection of such i2c adapter dongles sharing a single parallel port ...
Even the i2c-parport driver only supports one device at the moment, because we request the parallel port with flag PARPORT_FLAG_EXCL. Honestly I don't really recall why I did that, maybe just paranoia while I was developing the driver and then I forgot to change it. OTOH I'm not sure if it is really possible to grab the parallel port non-exclusively especially now that we can receive an interrupt at about any time.

Quoted 2 lines

The daisy chaining of parallel port devices always looked like a hack to me anyway.

Yes, a hack. I'm happy to see it wither away.

Quoted 8 lines

Quoted 2 lines
Alternatively, maybe *both* parport adapter drivers should be updated.
Yes, that would make sense. However, I always have a hard time testing i2c-parport-light, as apparently loading the parport_pc driver is enough for the parallel port hardware to lose its pristine state and then i2c-parport-light no longer works. And of course parport_pc tends to load automatically on my systems.

Oh. I never used it enough to notice such stuff, I guess. Somehow I expect parport-light bugs are low priority. ;)

Quoted 15 lines

Quoted 4 lines
Quoted 4 lines
drivers/i2c/busses/i2c-parport.c +0 -0
@@ -189,12 +197,18 @@ static void i2c_parport_attach (struct p
 	if (adapter_parm[type].init.val)
 		line_set(port, 1, &adapter_parm[type].init);
 
Quoted 10 lines
drivers/i2c/busses/i2c-parport.c static void i2c_parport_attach (struct p · line 203
Quoted 5 lines
drivers/i2c/busses/i2c-parport.c +4 -1
-	parport_release(adapter->pdev);
+	/* Setup SMBus alert if supported */
+	if (adapter_parm[type].smbus_alert) {
+		adapter->adapter.do_setup_alert = 1;
+		adapter->adapter.alert_edge_triggered = 1;
The fact that you need to set this trigger flag reflects a bug in the SMBALERT# patch. Since "you" are handling the IRQ, it has no business caring about that. It should suffice to set the do_setup_alert flag.

Quoted 3 lines

I'm unsure about this. For edge-triggered interrupts, you indeed shouldn't do anything in i2c-core if the adapter driver takes care of the interrupt.

If the adapter driver is taking care of the interrupt, then i2c-core should *never* try touching it. That part is easy to fix.

Quoted 4 lines

For level-triggered interrupts though, the interrupt needs to be disabled and re-enabled. smbus_alert() appears to be the right place to re-enable the interrupt. Else how would the adapter driver code know when to re-enable it?

That's another aspect of the bug. You'll recall I mentioned needing a hook for that, maybe a callback ...

Quoted 5 lines

Quoted 4 lines
drivers/hwmon/lm90.c
Quoted 2 lines
drivers/hwmon/lm90.c +0 -0
Separate patch?

Quoted 7 lines

I kept it in the same patch to help review and testing. When I submit it upstream, I will split appropriately.
Note that this is one more reason too make sure that the i2c-core code is robust against misbehaving devices. At some point in time, i2c-parport might support SMBus alert and the lm90 driver might not, or vice-versa.

Right; I certainly wasn't arguing aginst robustness.

(And testing against ... quirkier ... hardware than I used is of course a great way to shake loose issues in new code.)