Re: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver

Previous thread: [PATCH 0/2] Blackfin I2C TWI driver updates for 2.6.24 by Bryan Wu on Thursday, October 11, 2007 - 3:16 am. (3 messages)

Next thread: Regression from 2.6.21 to 2.6.22 and above by Stefan Scheler on Thursday, October 11, 2007 - 3:41 am. (3 messages)
From: Bryan Wu
Date: Thursday, October 11, 2007 - 3:23 am

From: Bryan Wu
Date: Thursday, October 11, 2007 - 3:23 am

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.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 |  450 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 467 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/input/joystick/Makefile
@@ -5,6 +5,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_JOYSTICK_A3D)		+= a3d.o
+obj-$(CONFIG_JOYSTICK_AD7142)		+= ad7142.o
 obj-$(CONFIG_JOYSTICK_ADI)		+= adi.o
 obj-$(CONFIG_JOYSTICK_AMIGA)		+= amijoy.o
 obj-$(CONFIG_JOYSTICK_ANALOG)		+= analog.o
diff --git a/drivers/input/joystick/ad7142.c b/drivers/input/joystick/ad7142.c
new file mode 100644
index 0000000..c7ab976
--- /dev/null
+++ b/drivers/input/joystick/ad7142.c
@@ -0,0 +1,450 @@
+/*
+ * File:         drivers/input/joystick/ad7142.c
+ * Based ...
From: Dmitry Torokhov
Date: Thursday, October 11, 2007 - 5:44 am

Hi Brian, Michael,


Thank you for the patch. The formatting of the patch is unorthodox,



Just use literals right in the _probe() function - that is the only


Does not this condition cause driver to fail completely? If so I'd
move it in probe function and  not even try initializing the device if






No need to count, input core serializes open and close and makes sure

What are the chances for IRQ to be used by 2 drivers? Maybe just




Thanks!

-- 
Dmitry
-

From: Bryan Wu
Date: Thursday, October 11, 2007 - 3:23 am

Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 drivers/serial/Kconfig           |   43 +++
 drivers/serial/Makefile          |    1 +
 drivers/serial/bfin_sport_uart.c |  614 ++++++++++++++++++++++++++++++++++++++
 drivers/serial/bfin_sport_uart.h |   63 ++++
 include/linux/serial_core.h      |    2 +
 5 files changed, 723 insertions(+), 0 deletions(-)
 create mode 100644 drivers/serial/bfin_sport_uart.c
 create mode 100644 drivers/serial/bfin_sport_uart.h

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 81b52b7..f3bfbe1 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -1259,4 +1259,47 @@ config SERIAL_OF_PLATFORM
 	  Currently, only 8250 compatible ports are supported, but
 	  others can easily be added.
 
+config SERIAL_BFIN_SPORT
+	tristate "Blackfin SPORT emulate UART (EXPERIMENTAL)"
+	depends on BFIN && EXPERIMENTAL
+	select SERIAL_CORE
+	help
+	  Enble support SPORT emulate UART on Blackfin series.
+
+	  To compile this driver as a module, choose M here: the 
+	  module will be called bfin_sport_uart.
+
+choice
+	prompt "Baud rate for Blackfin SPORT UART"
+	depends on SERIAL_BFIN_SPORT
+	default SERIAL_SPORT_BAUD_RATE_57600
+	help
+	  Choose a baud rate for the SPORT UART, other uart settings are
+	  8 bit, 1 stop bit, no parity, no flow control.
+
+config SERIAL_SPORT_BAUD_RATE_115200
+	bool "115200"
+
+config SERIAL_SPORT_BAUD_RATE_57600
+	bool "57600"
+
+config SERIAL_SPORT_BAUD_RATE_38400
+	bool "38400"
+
+config SERIAL_SPORT_BAUD_RATE_19200
+	bool "19200"
+
+config SERIAL_SPORT_BAUD_RATE_9600
+	bool "9600"
+endchoice
+
+config SPORT_BAUD_RATE
+	int
+	depends on SERIAL_BFIN_SPORT
+	default 115200 if (SERIAL_SPORT_BAUD_RATE_115200)
+	default 57600 if (SERIAL_SPORT_BAUD_RATE_57600)
+	default 38400 if (SERIAL_SPORT_BAUD_RATE_38400)
+	default 19200 if (SERIAL_SPORT_BAUD_RATE_19200)
+	default 9600 if (SERIAL_SPORT_BAUD_RATE_9600)
+
 endmenu
diff --git a/drivers/serial/Makefile ...
From: Andrew Morton
Date: Monday, October 15, 2007 - 1:33 pm

That's a bit hard to parse.

On Thu, 11 Oct 2007 18:23:40 +0800

There's a typo there.  Also the text is pretty meaningless - I'd fix it up

Probably this shouldn't be here at all - DEBUG is passed in from the gcc



It is not a good idea to create files in /proc which have spaces in their
names.  Yes, userspace _should_ be able to cope with that in all cases, but
all software sucks, even including userspace ;)



That's a bit ugly.  Perhaps using container_of() would be clearer.  it
would also remove the requirement that uart_port be the first member of

Please use checkpatch.  This patch generates a world record number of

Does it need to have global scope?


-

From: Mike Frysinger
Date: Monday, October 15, 2007 - 3:03 pm

here it is in english ;)

This driver emulates a standard UART using the SPORT peripherals on a

i'm not sure i follow ... these are the names given to request_irq()
which means this is what shows up in /proc/interrupts ... does this
function also create an actual file somewhere in /proc that i am not
aware of ?

the rest of the comments look good to me, thanks !
-mike
-

From: Andrew Morton
Date: Monday, October 15, 2007 - 3:22 pm

On Mon, 15 Oct 2007 18:03:35 -0400

err, umm, yeah.  But the same argument applies: it is imprudent to have
space-containing records in /proc/interrupts.

However it seems that we've already done that in several places so I guess
any /proc/interrupts-parsing programs are already coping with it OK.
-

From: Robin Getz
Date: Monday, October 15, 2007 - 3:10 pm

Blackfin's have a synchronous Serial Peripheral pORT (SPORT). 

Unlike SPI, UART, I2C, or CAN interfaces which are designed for specific 
industry standard compatible communication only, the SPORT support a variety 
(software programmable) serial data communication protocols:
 - A-law or µ-law companding according to G.711 specification
 - Multichannel or Time-Division-Multiplexed (TDM) modes
 - Stereo Audio I2S Mode
 - TDM Modes for Multi-Channel audio codecs
 - H.100 Telephony standard support
 - others, but if anyone really cares, they need to read the chip specs...

Bryan's patch takes the SPORT, and makes a standard UART out of it 
(exposing /dev/ttySS0) for those people who don't have enough hardware UARTs 
in their system.

Is it a SPORT driver that emulates a UART, or a UART driver on the SPORT? I 
think it is the latter...

Maybe:

[PATCH 3/3] Blackfin serial driver: enables a UART interface for the SPORT


Which still doesn't make any sense, until you know what a SPORT is :)

-Robin
-

From: Mike Frysinger
Date: Monday, October 15, 2007 - 3:04 pm

this looks wrong to me ... why cant the standard infrastructure for
managing baud rate be used ?
-mike
-

From: Andrew Morton
Date: Sunday, February 3, 2008 - 8:35 pm

My review comments for this patch remain unaddressed so
I have put it on hold.
--

From: Bryan Wu
Date: Monday, February 4, 2008 - 2:36 am

I forgot to send out the patch. It will be there soon.

Thanks
-Bryan
--

From: Bryan Wu
Date: Thursday, October 11, 2007 - 3:23 am

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 drivers/input/touchscreen/Kconfig  |   13 +
 drivers/input/touchscreen/Makefile |    1 +
 drivers/input/touchscreen/ad7877.c |  973 ++++++++++++++++++++++++++++++++++++
 3 files changed, 987 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/touchscreen/ad7877.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index f929fcd..d755048 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -29,6 +29,19 @@ config TOUCHSCREEN_ADS7846
 	  To compile this driver as a module, choose M here: the
 	  module will be called ads7846.
 
+config TOUCHSCREEN_AD7877
+	tristate "AD7877 based touchscreens"
+	depends on SPI_MASTER
+	help
+	  Say Y here if you have a touchscreen interface using the
+	  AD7877 controller, and your board-specific initialization
+	  code includes that in its table of SPI devices.
+
+	  If unsure, say N (but it's safe to say "Y").
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7877.
+
 config TOUCHSCREEN_BITSY
 	tristate "Compaq iPAQ H3600 (Bitsy) touchscreen"
 	depends on SA1100_BITSY
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 5de8933..5ee3c2b 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -4,6 +4,7 @@
 
 # Each configuration option enables a list of files.
 
+obj-$(CONFIG_TOUCHSCREEN_AD7877)	+= ad7877.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
 obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
 obj-$(CONFIG_TOUCHSCREEN_CORGI)		+= corgi_ts.o
diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
new file mode 100644
index 0000000..68554e2
--- /dev/null
+++ b/drivers/input/touchscreen/ad7877.c
@@ -0,0 +1,973 @@
+/*
+ * File:   ...
From: Dmitry Torokhov
Date: Thursday, October 11, 2007 - 6:27 am

Hi Bryan, Michael,



How many reads can happen at once? Maybe allocate 1 ser_req per

Confused about the logic - you set sync only if Rt != 0 so why don't

This looks scary. Can you try moving locking inside ad7877_enable and


You shoudl use input_unregister_device() once it was registered. So
you need something like
             goto err_unregister_idev;
...
err_unregister_idev:
        input_unregister_device(input_dev);
        input-dve = NULL; /* so we don't try to free it later */
err_remove_attr:

You don't want to unregister device before stopping
interrupts/kthread. Otherwise there is a chance you will try to use
just freed device to send event through.

The driver also contains some indenting damage, please re-check.

Thanks!

-- 
Dmitry
-

Previous thread: [PATCH 0/2] Blackfin I2C TWI driver updates for 2.6.24 by Bryan Wu on Thursday, October 11, 2007 - 3:16 am. (3 messages)

Next thread: Regression from 2.6.21 to 2.6.22 and above by Stefan Scheler on Thursday, October 11, 2007 - 3:41 am. (3 messages)