Hi Tony,
On Thu, Dec 16, 2010 at 09:45:25AM +0900, chinyeow.sim.xt@xxxxxxxxxxx wrote:
> From: Tony SIM <chinyeow.sim.xt@xxxxxxxxxxx>
>
> This patch set introduces for Sitronix ST1232 touchscreen controller
> driver. This is an integrated capacitive touchscreen with LCD module
> which can detect two fingers's touch X/Y coordinate.
>
> Signed-off-by: Tony SIM <chinyeow.sim.xt@xxxxxxxxxxx>
> ---
>
> Thanks for review!
>
> Changes since v3:
> - Removed unused variable.
> - Changed touch counting flag.
This looks great now, just a couple more comments:
> +
> +struct st1232_ts_finger {
> + uint8_t is_valid;
> + uint16_t x;
> + uint16_t y;
> + uint8_t t;
> +};
If you regroup fields the structure can be packed better. Also we prefer
u8, u16 instead of uint8_t, uint16_t in the kernel.
> + ts->input_dev->name = "st1232-touchscreen";
Also need to set input->id.bus = BUS_I2C and parent device.
> +
> + __set_bit(EV_SYN, ts->input_dev->evbit);
> + __set_bit(EV_KEY, ts->input_dev->evbit);
> + __set_bit(EV_ABS, ts->input_dev->evbit);
> +
> + input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR,
> + 0, MAX_AREA, 0, 0);
> + input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
> + MIN_X, MAX_X, 0, 0);
> + input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
> + MIN_Y, MAX_Y, 0, 0);
> +
> + ret = input_register_device(ts->input_dev);
> + if (ret) {
> + dev_err(&client->dev, "Unable to register %s input device\n",
> + ts->input_dev->name);
> + goto err_free_input_device;
> + }
> +
> + ret = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
> + IRQF_ONESHOT, client->name, ts);
> + if (ret) {
> + dev_err(&client->dev, "Failed to register interrupt\n");
> + goto err_free_input_device;
Registered devices should be destroyed by call to
input_unregister_device() instead of input_free_device().
Does the following (applied on top of your patch) still work? Thanks!
--
Dmitry
Input: st1232 - assorted changes
Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
---
drivers/input/touchscreen/Kconfig | 21 +++---
drivers/input/touchscreen/st1232.c | 133 ++++++++++++++++++------------------
2 files changed, 78 insertions(+), 76 deletions(-)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 3ca7700..07ac77d 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -659,17 +659,17 @@ config TOUCHSCREEN_PCAP
To compile this driver as a module, choose M here: the
module will be called pcap_ts.
-config TOUCHSCREEN_TPS6507X
- tristate "TPS6507x based touchscreens"
+config TOUCHSCREEN_ST1232
+ tristate "Sitronix ST1232 touchscreen controllers"
depends on I2C
help
- Say Y here if you have a TPS6507x based touchscreen
- controller.
+ Say Y here if you want to support Sitronix ST1232
+ touchscreen controller.
If unsure, say N.
To compile this driver as a module, choose M here: the
- module will be called tps6507x_ts.
+ module will be called st1232_ts.
config TOUCHSCREEN_STMPE
tristate "STMicroelectronics STMPE touchscreens"
@@ -681,15 +681,16 @@ config TOUCHSCREEN_STMPE
To compile this driver as a module, choose M here: the
module will be called stmpe-ts.
-config TOUCHSCREEN_ST1232
- tristate "Sitronix ST1232 touchscreen controllers"
+config TOUCHSCREEN_TPS6507X
+ tristate "TPS6507x based touchscreens"
depends on I2C
help
- Say Y here if you want to support Sitronix ST1232
- touchscreen controller.
+ Say Y here if you have a TPS6507x based touchscreen
+ controller.
If unsure, say N.
To compile this driver as a module, choose M here: the
- module will be called st1232_ts.
+ module will be called tps6507x_ts.
+
endif
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index f94a718..4ab3713 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -16,7 +16,6 @@
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
- *
*/
#include <linux/delay.h>
@@ -25,21 +24,22 @@
#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/types.h>
#define ST1232_TS_NAME "st1232-ts"
-#define MIN_X 0x00
-#define MIN_Y 0x00
-#define MAX_X 0x31f /* (800 - 1) */
-#define MAX_Y 0x1df /* (480 - 1) */
+#define MIN_X 0x00
+#define MIN_Y 0x00
+#define MAX_X 0x31f /* (800 - 1) */
+#define MAX_Y 0x1df /* (480 - 1) */
#define MAX_AREA 0xff
#define MAX_FINGERS 2
struct st1232_ts_finger {
- uint8_t is_valid;
- uint16_t x;
- uint16_t y;
- uint8_t t;
+ u16 x;
+ u16 y;
+ u8 t;
+ bool is_valid;
};
struct st1232_ts_data {
@@ -50,14 +50,15 @@ struct st1232_ts_data {
static int st1232_ts_read_data(struct st1232_ts_data *ts)
{
- int ret;
- struct i2c_msg msg[2];
- uint8_t start_reg;
- uint8_t buf[10];
struct st1232_ts_finger *finger = ts->finger;
+ struct i2c_client *client = ts->client;
+ struct i2c_msg msg[2];
+ int error;
+ u8 start_reg;
+ u8 buf[10];
/* read touchscreen data from ST1232 */
- msg[0].addr = ts->client->addr;
+ msg[0].addr = client->addr;
msg[0].flags = 0;
msg[0].len = 1;
msg[0].buf = &start_reg;
@@ -68,11 +69,11 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
msg[1].len = sizeof(buf);
msg[1].buf = buf;
- ret = i2c_transfer(ts->client->adapter, msg, 2);
- if (ret < 0)
- return ret;
+ error = i2c_transfer(client->adapter, msg, 2);
+ if (error < 0)
+ return error;
- /* get valid bit */
+ /* get "valid" bits */
finger[0].is_valid = buf[2] >> 7;
finger[1].is_valid = buf[5] >> 7;
@@ -94,10 +95,11 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
{
- int i, ret;
- int count = 0;
struct st1232_ts_data *ts = dev_id;
struct st1232_ts_finger *finger = ts->finger;
+ struct input_dev *input_dev = ts->input_dev;
+ int count = 0;
+ int i, ret;
ret = st1232_ts_read_data(ts);
if (ret < 0)
@@ -108,20 +110,19 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
if (!finger[i].is_valid)
continue;
- input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
- finger[i].t);
- input_report_abs(ts->input_dev, ABS_MT_POSITION_X, finger[i].x);
- input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, finger[i].y);
- input_mt_sync(ts->input_dev);
+ input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
+ input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
+ input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
+ input_mt_sync(input_dev);
count++;
}
/* SYN_MT_REPORT only if no contact */
if (!count)
- input_mt_sync(ts->input_dev);
+ input_mt_sync(input_dev);
/* SYN_REPORT */
- input_sync(ts->input_dev);
+ input_sync(input_dev);
end:
return IRQ_HANDLED;
@@ -131,65 +132,67 @@ static int __devinit st1232_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct st1232_ts_data *ts;
- int ret;
+ struct input_dev *input_dev;
+ int error;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
dev_err(&client->dev, "need I2C_FUNC_I2C\n");
return -EIO;
}
- ts = kzalloc(sizeof(struct st1232_ts_data), GFP_KERNEL);
- if (!ts) {
- ret = -ENOMEM;
- goto err;
+ if (!client->irq) {
+ dev_err(&client->dev, "no IRQ?\n");
+ return -EINVAL;
}
- ts->client = client;
- i2c_set_clientdata(client, ts);
- ts->input_dev = input_allocate_device();
- if (!ts->input_dev) {
- ret = -ENOMEM;
- dev_err(&client->dev, "Failed to allocate input device\n");
+ ts = kzalloc(sizeof(struct st1232_ts_data), GFP_KERNEL);
+ input_dev = input_allocate_device();
+ if (!ts || !input_dev) {
+ error = -ENOMEM;
goto err_free_mem;
}
- ts->input_dev->name = "st1232-touchscreen";
- __set_bit(EV_SYN, ts->input_dev->evbit);
- __set_bit(EV_KEY, ts->input_dev->evbit);
- __set_bit(EV_ABS, ts->input_dev->evbit);
+ ts->client = client;
+ ts->input_dev = input_dev;
- input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR,
- 0, MAX_AREA, 0, 0);
- input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
- MIN_X, MAX_X, 0, 0);
- input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
- MIN_Y, MAX_Y, 0, 0);
+ input_dev->name = "st1232-touchscreen";
+ input_dev->id.bustype = BUS_I2C;
+ input_dev->dev.parent = &client->dev;
- ret = input_register_device(ts->input_dev);
- if (ret) {
- dev_err(&client->dev, "Unable to register %s input device\n",
- ts->input_dev->name);
- goto err_free_input_device;
- }
+ __set_bit(EV_SYN, input_dev->evbit);
+ __set_bit(EV_KEY, input_dev->evbit);
+ __set_bit(EV_ABS, input_dev->evbit);
- ret = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
- IRQF_ONESHOT, client->name, ts);
- if (ret) {
+ input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 0);
+
+ error = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
+ IRQF_ONESHOT, client->name, ts);
+ if (error) {
dev_err(&client->dev, "Failed to register interrupt\n");
- goto err_free_input_device;
+ goto err_free_mem;
}
+ error = input_register_device(ts->input_dev);
+ if (error) {
+ dev_err(&client->dev, "Unable to register %s input device\n",
+ input_dev->name);
+ goto err_free_irq;
+ }
+
+ i2c_set_clientdata(client, ts);
device_init_wakeup(&client->dev, 1);
return 0;
-err_free_input_device:
- input_free_device(ts->input_dev);
+err_free_irq:
+ free_irq(client->irq, ts);
err_free_mem:
+ input_free_device(input_dev);
kfree(ts);
-err:
- return ret;
+ return error;
}
static int __devexit st1232_ts_remove(struct i2c_client *client)
@@ -207,8 +210,7 @@ static int __devexit st1232_ts_remove(struct i2c_client *client)
#ifdef CONFIG_PM
static int st1232_ts_suspend(struct device *dev)
{
- struct st1232_ts_data *ts = dev_get_drvdata(dev);
- struct i2c_client *client = ts->client;
+ struct i2c_client *client = to_i2c_client(dev);
if (device_may_wakeup(&client->dev))
enable_irq_wake(client->irq);
@@ -220,8 +222,7 @@ static int st1232_ts_suspend(struct device *dev)
static int st1232_ts_resume(struct device *dev)
{
- struct st1232_ts_data *ts = dev_get_drvdata(dev);
- struct i2c_client *client = ts->client;
+ struct i2c_client *client = to_i2c_client(dev);
if (device_may_wakeup(&client->dev))
disable_irq_wake(client->irq);
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
