[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [i2c] [patch 5/6] pca-isa driver uses the new pca-algorithm
From:       Wolfram Sang <w.sang () pengutronix ! de>
Date:       2008-01-24 14:01:45
Message-ID: fna5oa$fp0$1 () ger ! gmane ! org
[Download RAW message or body]

Jean Delvare wrote:
> Hi Wolfram,
> 
> On Mon, 21 Jan 2008 15:20:15 +0100, w.sang@pengutronix.de 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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/i2c


[prev in list] [next in list] [prev in thread] [next in thread]