[lm-sensors] Patch for i2c-elektor driver

[lm-sensors] Patch for i2c-elektor driver

[Stig Telfer]

Hi -

Please excuse me if this is not the right place to send a patch.

I have updated the i2c-elektor driver to work on kernel 2.6.13.  The 
driver now compiles cleanly and works on my system.  However, I only 
have access to a UP2000 Alpha system to test the i2c-elektor driver.  I 
don't think anything else will be broken by this patch, but I don't 
have an Elektor ISA card to test to be certain.  Does anybody have one? 
  Are they still in use?

The patch is available at:

http://www.lizardlogic.co.uk/software/hwmon/elektor-alpha.patch

Share and Enjoy,
Stig Telfer

[lm-sensors] Patch for i2c-elektor driver

[Jean Delvare]

Hi Stig,

On 2005-10-04, Stig Telfer wrote:
> Please excuse me if this is not the right place to send a patch.

This is the right place.

> I have updated the i2c-elektor driver to work on kernel 2.6.13.  The
> driver now compiles cleanly and works on my system.  However, I only
> have access to a UP2000 Alpha system to test the i2c-elektor driver.  I
> don't think anything else will be broken by this patch, but I don't
> have an Elektor ISA card to test to be certain.  Does anybody have one?
> Are they still in use?

No idea. I don't have such a card myself, and don't remember anyone
mentioning it for the past two years.

> The patch is available at:
>
> http://www.lizardlogic.co.uk/software/hwmon/elektor-alpha.patch

Could you please follow the usual patch submission procedure? I would
like to apply your patch but can't accept it in its current form. I
would like you to:

1* Sumbit the file itself rather than linking to it. Please also sign it,
by adding a line like:

Signed-off-by: Stig Telfer <email@address>

at the top of the patch.

See Documentation/SubmittingPatches in the Linux kernel source tree for
additional information.

2* Explain in detail why your patch is needed, and how it fixes the
problem you were having.

3* Not include code cleanups in a patch which fixes bugs. I accept code
cleanups, but as a separate patch, to be applied before or after the
bugfixing patch, at your option.

Thanks,
--
Jean Delvare

[lm-sensors] Patch for i2c-elektor driver

[Stig Telfer]

Hi Jean -

I have split the patch into two, annotated and signed off.  Patch 1 is 
the bug fixes.  Patch 2 is a bit of code cleanup.  Some of patch 2 you 
may think is not worth changing.  I have no problem with that.

On my system, the current elektor driver will oops on module load and 
has no chance of working because the IO regions are not mapped.  Patch 
1 fixes it (for me).

Regards,
Stig Telfer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: elektor-alpha-1.patch
Type: application/octet-stream
Size: 4121 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051004/038fd7c6/elektor-alpha-1.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: elektor-alpha-2.patch
Type: application/octet-stream
Size: 2089 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051004/038fd7c6/elektor-alpha-2.obj
-------------- next part --------------

[lm-sensors] Patch for i2c-elektor driver

[Jean Delvare]

Hi Stig,

One preliminary question: Did you attempt to contact Hans Berglund or
Oleg I. Vdovikin for their opinion on and testing of your patch?

> I have split the patch into two, annotated and signed off.  Patch 1 is 
> the bug fixes.  Patch 2 is a bit of code cleanup.  Some of patch 2 you 
> may think is not worth changing.  I have no problem with that.
> 
> On my system, the current elektor driver will oops on module load and 
> has no chance of working because the IO regions are not mapped.  Patch 
> 1 fixes it (for me).

Are you using mmapped=0 or 1? Wasn't it just a matter of using the
other? I guess there is a reason why this option exists.

I don't really feel qualified to review the first patch, as I have
relatively little idea how I/O mapping is supposed to work. Greg, want
to comment? All the (relatively) recent changes to the driver are
yours, although I'm not sure it actually means anything.

All I can offer myself are a few comments on the rest:

> +	pr_debug("i2c-elektor: Write %p 0x%02X\n", address, val & 255);

Can we get rid of that "& 255"? Looks pointless to me.

> +#if __alpha__
> +	/* API UP2000 needs some hardware fudging to make the write stick */
> +	iowrite8(val, address);
> +#endif

My understanding is that adding this here means that the following lines
later in the code:

>					/* I don't know why we need to
> 					   write twice */
>  					mmapped = 2;

could (and should) be removed. Right?

> +			printk(KERN_ERR "i2c-elektor: requested mem region (%#x:2) "
> +					"is in use.\n", base );

No dot at end of message please. Also a coding style problem, no space
before closing parenthesis.

> +		if( base_iomem = NULL ) {

Coding style.

> +			printk(KERN_ERR "i2c-elecktor: remap of memory region failed\n");

s/elecktor/elektor/

> +	pr_debug("registers %#x remapped to %p\n", base, base_iomem);

Please add an "i2c-elektor: " prefix as the other messages have.

As for the second patch, I'm globally happy with it, only a few minor
comments as well:

> -			       "i2c-elektor: requested I/O region (0x%X:2) "
> +			       "i2c-elektor: requested I/O region (%#x:2) "
>  			       "is in use.\n", base);

You could additionally drop the trailing dot.

> -					printk(KERN_INFO "i2c-elektor: found API UP2000 like board, will probe PCF8584 later.\n");
> +					printk(KERN_INFO
> +					       "i2c-elektor: found API UP2000 like board, will probe PCF8584 later.\n");

This is still too wide after your change. Please split the string, and
drop the training dot while you're there.

> -		printk(KERN_ERR "i2c-elektor: incorrect base address (0x%0X) specified for mmapped I/O.\n", base);
> +		printk(KERN_ERR "i2c-elektor: incorrect base address (%#x) specified for mmapped I/O.\n", base);

Likewise, please split into pieces so that it fits in 80 columns, and
drop the trailing dot.

Additional possible cleanup:

> 	printk(KERN_ERR "i2c-elektor: found device at %#x.\n", base);

This certainly isn't an error, and trailing dot can be dropped.

Care to respin the patches with the suggested changes? Then I'll push
them upwards.

Thanks for the good work,
-- 
Jean Delvare

[lm-sensors] Patch for i2c-elektor driver

[Jean Delvare]

Oh, BTW...

> +#if __alpha__

This really should be #ifdef __alpha__, else I get a warning at
compilation time (due to -Wundef which was recently added to the build
system).

Thanks,
-- 
Jean Delvare

[lm-sensors] Patch for i2c-elektor driver

[Greg KH]

On Tue, Oct 04, 2005 at 11:38:13PM +0200, Jean Delvare wrote:
> Hi Stig,
> 
> One preliminary question: Did you attempt to contact Hans Berglund or
> Oleg I. Vdovikin for their opinion on and testing of your patch?
> 
> > I have split the patch into two, annotated and signed off.  Patch 1 is 
> > the bug fixes.  Patch 2 is a bit of code cleanup.  Some of patch 2 you 
> > may think is not worth changing.  I have no problem with that.
> > 
> > On my system, the current elektor driver will oops on module load and 
> > has no chance of working because the IO regions are not mapped.  Patch 
> > 1 fixes it (for me).
> 
> Are you using mmapped=0 or 1? Wasn't it just a matter of using the
> other? I guess there is a reason why this option exists.
> 
> I don't really feel qualified to review the first patch, as I have
> relatively little idea how I/O mapping is supposed to work. Greg, want
> to comment? All the (relatively) recent changes to the driver are
> yours, although I'm not sure it actually means anything.

the iomap stuff looks good, did you run it through sparse to make sure
you got it all correct?

Also, all of the printk() and pr_debug() lines should be changed to use
dev_dbg(), dev_err() and friends.

thanks,

greg k-h

[lm-sensors] Patch for i2c-elektor driver

[Stig Telfer]

Wow, this is getting to be quite a spring-cleaning now.

I have incorporated all your suggestions into this patch, except for 
these two:

* Jean: use of val & 255 in printk statement in pcf_isa_setbyte() - I 
think this is actually required because parameter val is an integer 
quantity and could have been sign-extended?  I would prefer to change 
the function parameter to u8, but this violates the types of struct 
i2c_algo_pcf_data.

* Greg: I haven't changed the printk and pr_debug calls yet, because 
there are some printk calls before a struct device is initialised (this 
is done when the device is registered to i2c_algo_pcf at the end of 
driver init).  What is the policy relating to driver output before 
there is an associated struct device initialised?

I took a look at the code with sparse, there were no reported problems. 
  However, am I right in thinking that sparse is x86-centric?  I was 
running the compilation on an Alpha system.  With more than a little 
irony considering the political harangue against gcc in the sparse 
documentation, it didn't compile on my RH8 x86 system: gcc gave an 
internal compiler error...

The attached patch is for code cleanup and is a direct replacement for 
patch 2.

Regards,
Stig Telfer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: elektor-alpha-3.patch
Type: application/octet-stream
Size: 5054 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051006/196ffd32/elektor-alpha-3.obj

[lm-sensors] Patch for i2c-elektor driver

[Jean Delvare]

Hi Stig,

> I have incorporated all your suggestions into this patch, except for 
> these two:
> 
> * Jean: use of val & 255 in printk statement in pcf_isa_setbyte() - I 
> think this is actually required because parameter val is an integer 
> quantity and could have been sign-extended?  I would prefer to change 
> the function parameter to u8, but this violates the types of struct 
> i2c_algo_pcf_data.

The "& 255" is done in the debug printk only, iowrite8 is still called
with val unmasked, so if the masking actually did something, something
bas is happening, and the debug trace won't ever show it.

Looking at i2c-algo-pcf, it's quite clear that val will always fit in
an u8, so the masking is actually needless.

> * Greg: I haven't changed the printk and pr_debug calls yet, because 
> there are some printk calls before a struct device is initialised (this 
> is done when the device is registered to i2c_algo_pcf at the end of 
> driver init).  What is the policy relating to driver output before 
> there is an associated struct device initialised?

pr_debug and pr_info are fine to use in these cases (or even explicit
printk for other cases).

> I took a look at the code with sparse, there were no reported problems. 
>   However, am I right in thinking that sparse is x86-centric?  I was 
> running the compilation on an Alpha system.  With more than a little 
> irony considering the political harangue against gcc in the sparse 
> documentation, it didn't compile on my RH8 x86 system: gcc gave an 
> internal compiler error...

I had the exact same problem :(

> The attached patch is for code cleanup and is a direct replacement for 
> patch 2.

Comments:

> * all printed messages are prefixed by "i2c-elektor: ", via new preprocessor
>   definitions DRVNAME and DRVPFX

This is pretty pointless considering that we want to convert all of
these to dev_{dbg,err,warn,info} anyway, and these do not need a prefix.

> * #if __alpha__ converted to #ifdef __alpha__

This is a bug you introduced in patch #1! Don't fix it in patch #2, fix
patch #1 not to introduce it in the first place.

So, here comes a respin of both patches. I'd like to enqueue them and
push them to Greg soon. Can you test them and confirm them work OK for
you?

Thanks,
-- 
Jean Delvare
-------------- next part --------------
This patch updates the i2c-elektor driver, enabling it to compile
cleanly, load and run.  The key change is that it uses the new
__iomem/iowrite8/ioread8 functions to abstract the direct or
memory-mapped variants of register access.  Also, the original driver
would crash on module load on the Alpha because the PCI memory region
was not remapped into kernel memory.

I have managed the following testing: 

* compiled and tested it on my Alpha UP2000+ system.
* compiles cleanly for x86 but I don't have the hardware to test.

Signed-off-by: Stig Telfer <stig@lizardlogic.co.uk>
Signed-off-by: Jean Delvare <khali@linux-fr.org>

 drivers/i2c/busses/i2c-elektor.c |   74 +++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 25 deletions(-)

--- linux-2.6.14-rc3.orig/drivers/i2c/busses/i2c-elektor.c	2005-10-05 21:55:34.000000000 +0200
+++ linux-2.6.14-rc3/drivers/i2c/busses/i2c-elektor.c	2005-10-06 22:18:20.000000000 +0200
@@ -46,6 +46,8 @@
 #define DEFAULT_BASE 0x330
 
 static int base;
+static u8 __iomem *base_iomem;
+
 static int irq;
 static int clock  = 0x1c;
 static int own    = 0x55;
@@ -64,36 +66,27 @@
 
 static void pcf_isa_setbyte(void *data, int ctl, int val)
 {
-	int address = ctl ? (base + 1) : base;
+	u8 __iomem *address = ctl ? (base_iomem + 1) : base_iomem;
 
 	/* enable irq if any specified for serial operation */
 	if (ctl && irq && (val & I2C_PCF_ESO)) {
 		val |= I2C_PCF_ENI;
 	}
 
-	pr_debug("i2c-elektor: Write 0x%X 0x%02X\n", address, val & 255);
-
-	switch (mmapped) {
-	case 0: /* regular I/O */
-		outb(val, address);
-		break;
-	case 2: /* double mapped I/O needed for UP2000 board,
-                   I don't know why this... */
-		writeb(val, (void *)address);
-		/* fall */
-	case 1: /* memory mapped I/O */
-		writeb(val, (void *)address);
-		break;
-	}
+	pr_debug("i2c-elektor: Write %p 0x%02X\n", address, val);
+	iowrite8(val, address);
+#ifdef __alpha__
+	/* API UP2000 needs some hardware fudging to make the write stick */
+	iowrite8(val, address);
+#endif
 }
 
 static int pcf_isa_getbyte(void *data, int ctl)
 {
-	int address = ctl ? (base + 1) : base;
-	int val = mmapped ? readb((void *)address) : inb(address);
-
-	pr_debug("i2c-elektor: Read 0x%X 0x%02X\n", address, val);
+	u8 __iomem *address = ctl ? (base_iomem + 1) : base_iomem;
+	int val = ioread8(address);
 
+	pr_debug("i2c-elektor: Read %p 0x%02X\n", address, val);
 	return (val);
 }
 
@@ -155,7 +148,30 @@
 			       "is in use.\n", base);
 			return -ENODEV;
 		}
+		base_iomem = ioport_map(base, 2);
+		if (!base_iomem) {
+			printk(KERN_ERR "i2c-elektor: remap of I/O region "
+			       "%#x failed\n", base);
+			release_region(base, 2);
+			return -ENODEV;
+		}
+	} else {
+		if (!request_mem_region(base, 2, "i2c-elektor")) {
+			printk(KERN_ERR "i2c-elektor: requested memory region "
+			       "(%#x:2) is in use\n", base);
+			return -ENODEV;
+		}
+		base_iomem = ioremap(base, 2);
+		if (base_iomem = NULL) {
+			printk(KERN_ERR "i2c-elektor: remap of memory region "
+			       "%#x failed\n", base);
+			release_mem_region(base, 2);
+			return -ENODEV;
+		}
 	}
+	pr_debug("i2c-elektor: registers %#x remapped to %p\n", base,
+		 base_iomem);
+
 	if (irq > 0) {
 		if (request_irq(irq, pcf_isa_handler, 0, "PCF8584", NULL) < 0) {
 			printk(KERN_ERR "i2c-elektor: Request irq%d failed\n", irq);
@@ -200,7 +216,7 @@
 		cy693_dev = pci_get_device(PCI_VENDOR_ID_CONTAQ, 
 					   PCI_DEVICE_ID_CONTAQ_82C693, NULL);
 		if (cy693_dev) {
-			char config;
+			unsigned char config;
 			/* yeap, we've found cypress, let's check config */
 			if (!pci_read_config_byte(cy693_dev, 0x47, &config)) {
 				
@@ -219,9 +235,7 @@
 				if ((config & 0x7f) = 0x61) {
 					/* seems to be UP2000 like board */
 					base = 0xe0000;
-                                        /* I don't know why we need to
-                                           write twice */
-					mmapped = 2;
+					mmapped = 1;
                                         /* UP2000 drives ISA with
 					   8.25 MHz (PCI/4) clock
 					   (this can be read from cypress) */
@@ -262,8 +276,13 @@
 		free_irq(irq, NULL);
 	}
 
-	if (!mmapped)
+	if (!mmapped) {
+		ioport_unmap(base_iomem);
 		release_region(base , 2);
+	} else {
+		iounmap(base_iomem);
+		release_mem_region(base, 2);
+	}
 	return -ENODEV;
 }
 
@@ -276,8 +295,13 @@
 		free_irq(irq, NULL);
 	}
 
-	if (!mmapped)
+	if (!mmapped) {
+		ioport_unmap(base_iomem);
 		release_region(base , 2);
+	} else {
+		iounmap(base_iomem);
+		release_mem_region(base, 2);
+	}
 }
 
 MODULE_AUTHOR("Hans Berglund <hb@spacetec.no>");
-------------- next part --------------
Cleanups to the i2c-elektor driver:

* Set the i2c_adapter name field to "i2c-elektor" and use this string
  in all resource requests and printks.
* Change space-padding for tab indentation, kill trailing white space.
* Use dev_dbg and dev_info instead of printk where possible, and
  pr_debug and pr_info where not.
* Lines chopped to 80 columns.

Signed-off-by: Stig Telfer <stig@lizardlogic.co.uk>
Signed-off-by: Jean Delvare <khali@linux-fr.org>

 drivers/i2c/busses/i2c-elektor.c |   80 ++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

--- linux-2.6.14-rc3.orig/drivers/i2c/busses/i2c-elektor.c	2005-10-06 22:18:20.000000000 +0200
+++ linux-2.6.14-rc3/drivers/i2c/busses/i2c-elektor.c	2005-10-06 22:19:39.000000000 +0200
@@ -22,7 +22,7 @@
 /* With some changes from Kyösti Mälkki <kmalkki@cc.hut.fi> and even
    Frodo Looijaard <frodol@dds.nl> */
 
-/* Partialy rewriten by Oleg I. Vdovikin for mmapped support of 
+/* Partialy rewriten by Oleg I. Vdovikin for mmapped support of
    for Alpha Processor Inc. UP-2000(+) boards */
 
 #include <linux/kernel.h>
@@ -53,7 +53,7 @@
 static int own    = 0x55;
 static int mmapped;
 
-/* vdovikin: removed static struct i2c_pcf_isa gpi; code - 
+/* vdovikin: removed static struct i2c_pcf_isa gpi; code -
   this module in real supports only one device, due to missing arguments
   in some functions, called from the algo-pcf module. Sometimes it's
   need to be rewriten - but for now just remove this for simpler reading */
@@ -62,6 +62,8 @@
 static int pcf_pending;
 static spinlock_t lock;
 
+static struct i2c_adapter pcf_isa_ops;
+
 /* ----- local functions ----------------------------------------------	*/
 
 static void pcf_isa_setbyte(void *data, int ctl, int val)
@@ -73,7 +75,7 @@
 		val |= I2C_PCF_ENI;
 	}
 
-	pr_debug("i2c-elektor: Write %p 0x%02X\n", address, val);
+	dev_dbg(&pcf_isa_ops.dev, "Write %p 0x%02X\n", address, val);
 	iowrite8(val, address);
 #ifdef __alpha__
 	/* API UP2000 needs some hardware fudging to make the write stick */
@@ -86,7 +88,7 @@
 	u8 __iomem *address = ctl ? (base_iomem + 1) : base_iomem;
 	int val = ioread8(address);
 
-	pr_debug("i2c-elektor: Read %p 0x%02X\n", address, val);
+	dev_dbg(&pcf_isa_ops.dev, "Read %p 0x%02X\n", address, val);
 	return (val);
 }
 
@@ -142,39 +144,40 @@
 {
 	spin_lock_init(&lock);
 	if (!mmapped) {
-		if (!request_region(base, 2, "i2c (isa bus adapter)")) {
-			printk(KERN_ERR
-			       "i2c-elektor: requested I/O region (0x%X:2) "
-			       "is in use.\n", base);
+		if (!request_region(base, 2, pcf_isa_ops.name)) {
+			printk(KERN_ERR "%s: requested I/O region (%#x:2) is "
+			       "in use\n", pcf_isa_ops.name, base);
 			return -ENODEV;
 		}
 		base_iomem = ioport_map(base, 2);
 		if (!base_iomem) {
-			printk(KERN_ERR "i2c-elektor: remap of I/O region "
-			       "%#x failed\n", base);
+			printk(KERN_ERR "%s: remap of I/O region %#x failed\n",
+			       pcf_isa_ops.name, base);
 			release_region(base, 2);
 			return -ENODEV;
 		}
 	} else {
-		if (!request_mem_region(base, 2, "i2c-elektor")) {
-			printk(KERN_ERR "i2c-elektor: requested memory region "
-			       "(%#x:2) is in use\n", base);
+		if (!request_mem_region(base, 2, pcf_isa_ops.name)) {
+			printk(KERN_ERR "%s: requested memory region (%#x:2) "
+			       "is in use\n", pcf_isa_ops.name, base);
 			return -ENODEV;
 		}
 		base_iomem = ioremap(base, 2);
 		if (base_iomem = NULL) {
-			printk(KERN_ERR "i2c-elektor: remap of memory region "
-			       "%#x failed\n", base);
+			printk(KERN_ERR "%s: remap of memory region %#x "
+			       "failed\n", pcf_isa_ops.name, base);
 			release_mem_region(base, 2);
 			return -ENODEV;
 		}
 	}
-	pr_debug("i2c-elektor: registers %#x remapped to %p\n", base,
+	pr_debug("%s: registers %#x remapped to %p\n", pcf_isa_ops.name, base,
 		 base_iomem);
 
 	if (irq > 0) {
-		if (request_irq(irq, pcf_isa_handler, 0, "PCF8584", NULL) < 0) {
-			printk(KERN_ERR "i2c-elektor: Request irq%d failed\n", irq);
+		if (request_irq(irq, pcf_isa_handler, 0, pcf_isa_ops.name,
+				NULL) < 0) {
+			printk(KERN_ERR "%s: Request irq%d failed\n",
+			       pcf_isa_ops.name, irq);
 			irq = 0;
 		} else
 			enable_irq(irq);
@@ -202,45 +205,49 @@
 	.class		= I2C_CLASS_HWMON,
 	.id		= I2C_HW_P_ELEK,
 	.algo_data	= &pcf_isa_data,
-	.name		= "PCF8584 ISA adapter",
+	.name		= "i2c-elektor",
 };
 
-static int __init i2c_pcfisa_init(void) 
+static int __init i2c_pcfisa_init(void)
 {
 #ifdef __alpha__
-	/* check to see we have memory mapped PCF8584 connected to the 
+	/* check to see we have memory mapped PCF8584 connected to the
 	Cypress cy82c693 PCI-ISA bridge as on UP2000 board */
 	if (base = 0) {
 		struct pci_dev *cy693_dev;
-		
-		cy693_dev = pci_get_device(PCI_VENDOR_ID_CONTAQ, 
+
+		cy693_dev = pci_get_device(PCI_VENDOR_ID_CONTAQ,
 					   PCI_DEVICE_ID_CONTAQ_82C693, NULL);
 		if (cy693_dev) {
 			unsigned char config;
 			/* yeap, we've found cypress, let's check config */
 			if (!pci_read_config_byte(cy693_dev, 0x47, &config)) {
-				
-				pr_debug("i2c-elektor: found cy82c693, config register 0x47 = 0x%02x.\n", config);
+
+				pr_debug("%s: found cy82c693, config "
+					 "register 0x47 = 0x%02x\n",
+					 pcf_isa_ops.name, config);
 
 				/* UP2000 board has this register set to 0xe1,
-                                   but the most significant bit as seems can be 
+				   but the most significant bit as seems can be
 				   reset during the proper initialisation
-                                   sequence if guys from API decides to do that
-                                   (so, we can even enable Tsunami Pchip
-                                   window for the upper 1 Gb) */
+				   sequence if guys from API decides to do that
+				   (so, we can even enable Tsunami Pchip
+				   window for the upper 1 Gb) */
 
 				/* so just check for ROMCS at 0xe0000,
-                                   ROMCS enabled for writes
+				   ROMCS enabled for writes
 				   and external XD Bus buffer in use. */
 				if ((config & 0x7f) = 0x61) {
 					/* seems to be UP2000 like board */
 					base = 0xe0000;
 					mmapped = 1;
-                                        /* UP2000 drives ISA with
+					/* UP2000 drives ISA with
 					   8.25 MHz (PCI/4) clock
 					   (this can be read from cypress) */
 					clock = I2C_PCF_CLK | I2C_PCF_TRNS90;
-					printk(KERN_INFO "i2c-elektor: found API UP2000 like board, will probe PCF8584 later.\n");
+					pr_info("%s: found API UP2000 like "
+						"board, will probe PCF8584 "
+						"later\n", pcf_isa_ops.name);
 				}
 			}
 			pci_dev_put(cy693_dev);
@@ -250,12 +257,11 @@
 
 	/* sanity checks for mmapped I/O */
 	if (mmapped && base < 0xc8000) {
-		printk(KERN_ERR "i2c-elektor: incorrect base address (0x%0X) specified for mmapped I/O.\n", base);
+		printk(KERN_ERR "%s: incorrect base address (%#x) specified "
+		       "for mmapped I/O\n", pcf_isa_ops.name, base);
 		return -ENODEV;
 	}
 
-	printk(KERN_INFO "i2c-elektor: i2c pcf8584-isa adapter driver\n");
-
 	if (base = 0) {
 		base = DEFAULT_BASE;
 	}
@@ -265,8 +271,8 @@
 		return -ENODEV;
 	if (i2c_pcf_add_bus(&pcf_isa_ops) < 0)
 		goto fail;
-	
-	printk(KERN_ERR "i2c-elektor: found device at %#x.\n", base);
+
+	dev_info(&pcf_isa_ops.dev, "found device at %#x\n", base);
 
 	return 0;
 

[lm-sensors] Patch for i2c-elektor driver

[Stig Telfer]

Hi Jean -

Thanks for the new patches.  I tested them and they appear to work.  
However, the problem with dev_dbg message prefixes early in driver 
initialisation is still there:

i2c-elektor: found cy82c693, config register 0x47 = 0xe1
i2c-elektor: found API UP2000 like board, will probe PCF8584 later
i2c-elektor: registers 0xe0000 remapped to fffffd00000e0000
  : Write fffffd00000e0001 0x80
  : Read fffffd00000e0001 0x00
  : Write fffffd00000e0000 0x55
  : Read fffffd00000e0000 0x55
  : Write fffffd00000e0001 0xA0
  : Read fffffd00000e0001 0x20
  : Write fffffd00000e0000 0x18
  : Read fffffd00000e0000 0xF8
  : Write fffffd00000e0001 0xC1
  : Read fffffd00000e0001 0x81
i2c-algo-pcf.o: deteted and initialized PCF8584.
i2c_adapter i2c-0: Registered as minor 0
i2c_adapter i2c-0: registered as adapter #0
i2c_adapter i2c-0: found device at 0xe0000

We don't get the driver name until we call i2c_add_adapter in 
i2c-algo-pcf.c.  Should we revert the dev_dbg statements to pr_debugs 
for device byte read/writes?  That doesn't sound very consistent 
though...

>> * Jean: use of val & 255 in printk statement in pcf_isa_setbyte() - I
>> think this is actually required because parameter val is an integer
>> quantity and could have been sign-extended?  I would prefer to change
>> the function parameter to u8, but this violates the types of struct
>> i2c_algo_pcf_data.
>
> The "& 255" is done in the debug printk only, iowrite8 is still called
> with val unmasked, so if the masking actually did something, something
> bas is happening, and the debug trace won't ever show it.
>
> Looking at i2c-algo-pcf, it's quite clear that val will always fit in
> an u8, so the masking is actually needless.

Perhaps the author's original intention would have been better 
described by casting val as (u8)val.  This avoids any sign-extended 
mangling from implicit size conversions, like this one:

	char val=0x80;
	printf("Write 0x%02X\n", val);

As you say, it doesn't matter for this case because our only caller 
(i2c-algo-pcf) does not generate arguments to pcf_isa_setbyte in this 
way.

Best wishes,
Stig

[lm-sensors] Patch for i2c-elektor driver

[Jean Delvare]

Hi Stig,

> Thanks for the new patches.  I tested them and they appear to work.  
> However, the problem with dev_dbg message prefixes early in driver 
> initialisation is still there:
> 
> i2c-elektor: found cy82c693, config register 0x47 = 0xe1
> i2c-elektor: found API UP2000 like board, will probe PCF8584 later
> i2c-elektor: registers 0xe0000 remapped to fffffd00000e0000
>   : Write fffffd00000e0001 0x80
>   : Read fffffd00000e0001 0x00
>   : Write fffffd00000e0000 0x55
>   : Read fffffd00000e0000 0x55
>   : Write fffffd00000e0001 0xA0
>   : Read fffffd00000e0001 0x20
>   : Write fffffd00000e0000 0x18
>   : Read fffffd00000e0000 0xF8
>   : Write fffffd00000e0001 0xC1
>   : Read fffffd00000e0001 0x81
> i2c-algo-pcf.o: deteted and initialized PCF8584.
> i2c_adapter i2c-0: Registered as minor 0
> i2c_adapter i2c-0: registered as adapter #0
> i2c_adapter i2c-0: found device at 0xe0000
> 
> We don't get the driver name until we call i2c_add_adapter in 
> i2c-algo-pcf.c.

I get it now. I had missed the fact that pcf_isa_{set,get}byte would be
called during the initialization step.

> Should we revert the dev_dbg statements to pr_debugs for device byte
> read/writes?  That doesn't sound very consistent though...

That's the only option as far as I can see. I'll do that.

Thanks,
-- 
Jean Delvare