[lm-sensors] Patch for i2c-elektor driver

[khali@linux-fr.org (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