Subject: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver
[try #2] Changelog:

 - Coding style issues fixed, passed checkpatch.pl

 - Kill uselss "ad7142_used"

 - Move request_irq to probe

 - Move i2c_check_functionality to probe

 - Error handling added
[try #3] Changelog:

 - Use kzalloc

 - Use strlcpy

 - Use completion instead of wait queue

 - Move input_allocate_device and input_register_device to ad7142_probe()

 - Fix some issues pointed by LKML
Cc: Andrey Panin <pazke@donpac.ru>

Cc: Roel Kluin <12o3l@tiscali.nl>

Cc: Ahmed S. Darwish <darwish.07@gmail.com>

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Bryan Wu <bryan.wu@analog.com>

---

 drivers/input/joystick/Kconfig  |   16 ++

 drivers/input/joystick/Makefile |    1 +

 drivers/input/joystick/ad7142.c |  477 +++++++++++++++++++++++++++++++++++++++

 3 files changed, 494 insertions(+), 0 deletions(-)

 create mode 100644 drivers/input/joystick/ad7142.c
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig

index 7c662ee..aeb7cc9 100644

--- a/drivers/input/joystick/Kconfig

+++ b/drivers/input/joystick/Kconfig

@@ -282,4 +282,20 @@ config JOYSTICK_XPAD_LEDS

 	  This option enables support for the LED which surrounds the Big X on

 	  XBox 360 controller.
+config JOYSTICK_AD7142

+         tristate "Analog Devices AD7142 Joystick support"

+         depends on BFIN && I2C

+         help

+           Say Y here if you want to support an AD7142 joystick

+

+

+ config BFIN_JOYSTICK_IRQ_PFX

+         int "GPIO for Interrupt"

+         depends on (BFIN && JOYSTICK_AD7142)

+         range 33 120

+         default "55" if BFIN537_STAMP

+         default "39" if BFIN533_STAMP

+         help

+           Choose an GPIO as Keypad interrupt.[0..15]

+

 endif

diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile

index e855abb..8df388f 100644

--- a/drivers/input/joystick/Makefile

+++ b/drivers/in...
[ message continues ]

Hi Bryan,
No, this is not going to work well:

- you at least need to reinitialize the completion before enabling

IRQ, otherwise you will spin in a very tight loop

- if noone would touch the joystick ad7142_clsoe would() block

infinitely because noone would signal the completion and

ad7142_thread() would never stop.
Completion is just not a good abstraction here... Please use work
Don't you need to write something over i2c to tell the device to shut

down? As it is now I expect the device to continue raising its IRQ
Ok, so you freeing IRQ here, but it is allocated in ad7142_probe().
input_unregister_device() should be in ad7142_detach_client? I am not

sure i2c - there seems to be 2 interface styles and you probably need
--

Dmitry

-

The new style is preferred, yes.
--

Jean Delvare

-

Bryan, I'm very interested in the technical advantage of using a completion

here.
In my _not-experienced_ opinion, I remember completions was created mainly for

"create_task, wait till task got finished, go on" case. Why using it in a

different context while workqueues was created for a similar situation to

ad7142 one (non-irq context bottom-half) ?
Regards,
--

Ahmed S. Darwish

HomePage: http://darwish.07.googlepages.com

Blog: http://darwish-07.blogspot.com

-

I like completion because it is simple to use and understand. Your

understanding is right. But there is no limit for using different

context with completion. completion is a wrapper of waitqueue+done

flag. For some drivers, in process context call

wait_for_completetion(), then schedule out and in irq handler call

complete(). This is very simple and helpful for driver design (For

example, you call dma function to transfer data, then you schedule out

and then DMA IRQ handler will call complete() to wakeup you).
But in this driver, a) can not call ad7142_decode() in IRQ handler,

because it will sleep in IRQ context by calling some i2c API, b) in

original design, creating a new kthread and using some waitqueue API

is the same way as using workqueue, c) cannot use completion as Dmitry

said.
I am going to use workqueue here.
Any idea?
Thanks

-Bryan Wu

-

Regards :),
--

Ahmed S. Darwish

HomePage: http://darwish.07.googlepages.com

Blog: http://darwish-07.blogspot.com

-

Yes, I agree with you now, although I have a little concern about the
Yes, no need input_unregister_device() here.
Thanks a lot for you kindly review.

I will resend update patch later.
Best Regards,

-Bryan Wu

-

Having a separate workqueue should isolate the driver from users

hogging keventd. Otherwise the speed should be pretty much the same as
Thank you for not getting frustrated with all my change requests. Btw,

blackfin keypad driver is in my tree and should be in mainline once

Linus does the pull I requested.
--

Dmitry

-

Does this driver need the create a new kthread instead of keventd?
Oh, your help is very useful. It encourages us to send out our drivers
Thanks again, I noticed it was merged already.
-Bryan Wu

-

No it does not have to start a new workqueue. I'd start with keventd

and only implement a separate workqueue later if I saw the driver

being starved by other keventd users.
--

Dmitry

-