Re: [PATCH 01/03] i2c-i801: Add basic interrupt support

[Ivo Manca <pinkel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hey Jean,
> Please provide patches that can be applied with patch -p1 from the root
> of the kernel source tree.
>   
Will do, sorry about that

> Having this here is a bit confusing, you should leave all the
> transaction types (from I801_QUICK to I801_I2C_BLOCK_LAST) as a block.
>   
Oops, not a smart place to put this constant, I agree. Moved to

> I801_HST_CNT_INTREN (which could probably be renamed to just
> I801_INTREN for brevity) would go first.
>
> Also note that the drivers already had a define for this flag, named
> ENABLE_INT9, although it was disabled. I agree that I801_INTREN is a
> better name, but it would probably be a good idea to delete ENABLE_INT9
> and all references thereto first, otherwise the code becomes a little
> confusing.
>   
Will do. I'll just delete ENABLE_INT9 all together.

> You never really use the irq value, only whether it is -1 or not. So it
> might make more sense to turn it into a flag and rename it to use_irq?
>   
Yup, seems like a waste to store the value :)

> Please make this HZ/2 a define so that it's easy to change. 
Done

> Ideally this define would replace MAX_TIMEOUT (in a separate patch) so 
> that both
> the poll-based and the interrupt-driven paths have the same timeout 
> value. Right now,
> the timeout handling of the poll-based path is rather ugly (the actual 
> timeout depends on
> the value of HZ.)
Hm, seems like a very sensible thing to do. However, I have no idea how 
to implement that, since you don't really know how long msleep slept, or 
not? I sadly don't really have time to look into this right now, sorry.

> Not sure why you make this wait interruptible. The poll-based path
> isn't interruptible, and you do not handle the interrupted case anyway.
>   
I'm not an expert, but I think this is the right way to use it.
The interupt handler i801_isr catches the interrupt, saves the status to 
algo_data and calls wake_up_interruptable. This will wake up the wait.

I'm not sure whether this is the correct way or not. Any input from an 
interrupt expert is very much welcome.
I did test it without the event_interruptable, which produces nonsense, 
like:

====
polling
====
[root@localhost busses]# time i2cdetect -y 0
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- -- -- -- -- -- -- -- -- --
50: 50 -- -- -- 54 55 -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- --
70: -- -- -- UU -- -- -- --

real    0m0.235s
user    0m0.000s
sys     0m0.003s

====
wait_event_interruptable_timeout
====
[root@localhost busses]# time i2cdetect -y 0
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- 08 -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- -- -- -- -- -- -- -- -- --
50: 50 -- -- -- 54 55 -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- --
70: -- -- -- UU -- -- -- --

real    0m0.130s
user    0m0.001s
sys     0m0.009s

====
wait_event_timeout
====
[root@localhost busses]# time i2cdetect -y 0
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- 0a -- -- -- -- --
10: 10 -- -- -- -- -- -- -- -- 19 -- -- -- -- 1e --
20: -- -- -- -- 24 -- -- -- -- 29 -- -- -- -- -- --
30: 30 -- -- 33 -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

real    0m4.090s
user    0m0.000s
sys     0m0.004s

> The polled-based path has additional logic to handle the timeout case
> (by resetting the controller.) I would guess you want the same for the
> interrupt-driven path?
>   
Agree. After merging to 2.6.27-rc3, I noticed a helper function called 
i801_check_post was added. Seems to be the ideal place to pass the 
return data to.

> Please add a trailing comma.
>   
Done
> i801_adapter.name is a rather long string, I think I'd rather use
> i801_driver.name to register the IRQ.
>   
Ok
> I think you have a race here. You free the irq before removing the i2c
> adapter. What will happen if someone initiates an I2C transaction after
> the IRQ has been freed? It would probably be better to remove the i2c
> adapter first.
>   
Quite an obvious race as well... Changed already.
> Other than that, it looks good to me, good work. Note though that I am
> in no way an expert of interrupt handling, as this is the first
> PC-style SMBus controller driver which gets converted to use interrupts
> instead of polling.
>   
Thanks. I'm by no means an expert as well, it's all quite new to me. 
Thanks a lot for your patience.

I'll send you an updated patch later this evening. I don't know anything 
about config files though, so that'll have to wait a bit... The module 
parameter will be added though.

Ivo

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