Re: [patch 5/6] pca-isa driver uses the new pca-algorithm

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Jean Delvare wrote:
> Hi Wolfram,
> 
> On Mon, 21 Jan 2008 15:20:15 +0100, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
>> This is untested, due to no hardware!
> Did you at least try to build it? I guess not, because it fails here:
> 
>   CC [M]  drivers/i2c/busses/i2c-pca-isa.o
> drivers/i2c/busses/i2c-pca-isa.c: In function `pca_isa_waitforcompletion':
> drivers/i2c/busses/i2c-pca-isa.c:84: error: `adap' undeclared (first use in this function)
> drivers/i2c/busses/i2c-pca-isa.c:84: error: (Each undeclared identifier is reported only once
> drivers/i2c/busses/i2c-pca-isa.c:84: error: for each function it appears in.)
> drivers/i2c/busses/i2c-pca-isa.c: In function `pca_isa_resetchip':
> drivers/i2c/busses/i2c-pca-isa.c:96: error: syntax error before "DRIVER"
> drivers/i2c/busses/i2c-pca-isa.c: At top level:
> drivers/i2c/busses/i2c-pca-isa.c:108: error: unknown field `wait_for_competion' specified in initializer
> drivers/i2c/busses/i2c-pca-isa.c:110: error: initializer element is not constant
> drivers/i2c/busses/i2c-pca-isa.c:110: error: (near initialization for `pca_isa_data.my_slave_addr')
> drivers/i2c/busses/i2c-pca-isa.c:111: error: initializer element is not constant
> drivers/i2c/busses/i2c-pca-isa.c:111: error: (near initialization for `pca_isa_data.i2c_clock')
> drivers/i2c/busses/i2c-pca-isa.c: In function `pca_isa_probe':
> drivers/i2c/busses/i2c-pca-isa.c:140: error: implicit declaration of function `i2c_pca_add_bus'
> make[3]: *** [drivers/i2c/busses/i2c-pca-isa.o] Error 1
> make[2]: *** [drivers/i2c/busses] Error 2
> make[1]: *** [drivers/i2c] Error 2
> make: *** [drivers] Error 2
> 
> Please fix.
Err? Seems like a bug in my workflow. Will fix!

> No history in drivers.
Will be deleted.

>> -#include <asm/io.h>
>> -#include <asm/irq.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
> Why? Many drivers include <asm/io.h> and <asm/irq.h>, it looks OK to me.
checkpatch.pl told me so.

>>  #undef DEBUG_IO
>> -//#define DEBUG_IO
> While you're here, the line before could be discarded as well.
OK.

  >>  static struct i2c_algo_pca_data pca_isa_data = {
>> -	.get_own		= pca_isa_getown,
>> -	.get_clock		= pca_isa_getclock,
>> +	.data			= NULL,
> No need to initialize to NULL, the compiler does it for you. On a side
> note though, I fail to see how this could work, given that you changed
> the callbacks so that you pass this private data pointer to them
> instead of a pointer to struct i2c_algo_pca_data. This probably needs
> some more thinking.
The pointer is passed along, but never used with the ISA-driver. 
Everything needed is in static variables (base address, wait_queue). I 
set the pointer explicitly to NULL, so the assignment is "marked" to be 
fully intentional. Then again, a comment would also do.

Note: I was thinking about removing the static variables and creating an 
apropriate struct dynamically; but I fear this change is too big for not 
having the hardware.

>> -	if (irq > 0) {
>> +	if (irq > -1) {
> This deserves an explanation... In the light of previous discussions on
> the i2c list, I'd rather expect comparisons with NO_IRQ.
Everything inside the ISA-driver checked against -1, so I assumed this 
to be a bug. Should have explained this in detail, sorry, I forgot. 
After reading the recent thread about NO_IRQ, I also came to the 
conclusion, that I better should use this, too.

Thanks again for your review!

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c