LinuxLists.cc - Support for synaptic touchscreen in HTC dream

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi Pavel,

On Tue, Jul 14, 2009 at 3:36 PM, Pavel Machek<[email protected]> wrote:
> From: Arve Hj?nnev?g <[email protected]>
>
> This adds support for synaptic touchscreen, used in HTC dream
> cellphone.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index bb6486a..fa3404f 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -206,6 +213,14 @@ config TOUCHSCREEN_MIGOR
> ? ? ? ? ?To compile this driver as a module, choose M here: the
> ? ? ? ? ?module will be called migor_ts.
>
> +config TOUCHSCREEN_SYNAPTICS_I2C_RMI
> + ? ? ? tristate "Synaptics i2c touchscreen"
> + ? ? ? depends on I2C
> + ? ? ? help
> + ? ? ? ? This enables support for Synaptics RMI over I2C based touchscreens.
> + ? ? ? ? Such touchscreen is used in HTC Dream.
> +
> +
> ?config TOUCHSCREEN_TOUCHRIGHT
> ? ? ? ?tristate "Touchright serial touchscreen"
> ? ? ? ?select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index d3375af..959dcda 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -22,6 +23,7 @@ obj-$(CONFIG_TOUCHSCREEN_HP7XX) ? ? ? ? ? ? ? += jornada720_ts.o
> ?obj-$(CONFIG_TOUCHSCREEN_HTCPEN) ? ? ? += htcpen.o
> ?obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE) ? ? ? ?+= usbtouchscreen.o
> ?obj-$(CONFIG_TOUCHSCREEN_PENMOUNT) ? ? += penmount.o
> +obj-$(CONFIG_TOUCHSCREEN_SYNAPTICS_I2C_RMI) ? ?+= synaptics_i2c_rmi.o
> ?obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) ? += touchit213.o
> ?obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) ? += touchright.o
> ?obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) ? ? += touchwin.o
> diff --git a/drivers/input/touchscreen/synaptics_i2c_rmi.c b/drivers/input/touchscreen/synaptics_i2c_rmi.c
> new file mode 100644
> index 0000000..fe10e85
> --- /dev/null
> +++ b/drivers/input/touchscreen/synaptics_i2c_rmi.c
> @@ -0,0 +1,587 @@
> +/*
> + * Support for synaptics touchscreen.
> + *
> + * Copyright (C) 2007 Google, Inc.
> + * Author: Arve Hj?nnev?g <[email protected]>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * 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/module.h>
> +#include <linux/delay.h>
> +#include <linux/hrtimer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/synaptics_i2c_rmi.h>
> +
> +static struct workqueue_struct *synaptics_wq;
> +
> +struct synaptics_ts_data {
> + ? ? ? u16 addr;
> + ? ? ? struct i2c_client *client;
> + ? ? ? struct input_dev *input_dev;
> + ? ? ? int use_irq;
> + ? ? ? struct hrtimer timer;
> + ? ? ? struct work_struct ?work;
> + ? ? ? u16 max[2];
> + ? ? ? int snap_state[2][2];
> + ? ? ? int snap_down_on[2];
> + ? ? ? int snap_down_off[2];
> + ? ? ? int snap_up_on[2];
> + ? ? ? int snap_up_off[2];
> + ? ? ? int snap_down[2];
> + ? ? ? int snap_up[2];
> + ? ? ? u32 flags;
> + ? ? ? int (*power)(int on);
> +};
> +
> +static int i2c_set(struct synaptics_ts_data *ts, u8 reg, u8 val, char *msg)
> +{
> + ? ? ? int ret = i2c_smbus_write_byte_data(ts->client, reg, val);
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? pr_err("i2c_smbus_write_byte_data failed (%s)\n", msg);
> + ? ? ? return ret;
> +}
> +
> +static int i2c_read(struct synaptics_ts_data *ts, u8 reg, char *msg)
> +{
> + ? ? ? int ret = i2c_smbus_read_byte_data(ts->client, 0xf0);
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? pr_err("i2c_smbus_read_byte_data failed (%s)\n", msg);
> + ? ? ? return ret;
> +}
> +
> +static int synaptics_init_panel(struct synaptics_ts_data *ts)
> +{
> + ? ? ? int ret;
> +
> + ? ? ? ret = i2c_set(ts, 0xff, 0x10, "set page select");
> + ? ? ? if (ret == 0)
> + ? ? ? ? ? ? ? ret = i2c_set(ts, 0x41, 0x04, "set No Clip Z");
> +
> + ? ? ? ret = i2c_set(ts, 0xff, 0x04, "fallback page select");
> + ? ? ? ret = i2c_set(ts, 0xf0, 0x81, "select 80 reports per second");
> + ? ? ? return ret;
> +}
> +

> +static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
> +{

some documentation about this logic would be great.

> + ? ? ? int pos[2][2];
> + ? ? ? int f, a;
> + ? ? ? int base = 2;
> + ? ? ? int z = buf[1];
> + ? ? ? int w = buf[0] >> 4;
> + ? ? ? int finger = buf[0] & 7;
> + ? ? ? int finger2_pressed;
> +
> + ? ? ? for (f = 0; f < 2; f++) {
> + ? ? ? ? ? ? ? u32 flip_flag = SYNAPTICS_FLIP_X;
> + ? ? ? ? ? ? ? for (a = 0; a < 2; a++) {
> + ? ? ? ? ? ? ? ? ? ? ? int p = buf[base + 1];
> + ? ? ? ? ? ? ? ? ? ? ? p |= (u16)(buf[base] & 0x1f) << 8;
> + ? ? ? ? ? ? ? ? ? ? ? if (ts->flags & flip_flag)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p = ts->max[a] - p;
> + ? ? ? ? ? ? ? ? ? ? ? if (ts->flags & SYNAPTICS_SNAP_TO_INACTIVE_EDGE) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (ts->snap_state[f][a]) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (p <= ts->snap_down_off[a])
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p = ts->snap_down[a];
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? else if (p >= ts->snap_up_off[a])
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p = ts->snap_up[a];
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ts->snap_state[f][a] = 0;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (p <= ts->snap_down_on[a]) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p = ts->snap_down[a];
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ts->snap_state[f][a] = 1;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else if (p >= ts->snap_up_on[a]) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p = ts->snap_up[a];
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ts->snap_state[f][a] = 1;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? pos[f][a] = p;
> + ? ? ? ? ? ? ? ? ? ? ? base += 2;
> + ? ? ? ? ? ? ? ? ? ? ? flip_flag <<= 1;
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? base += 2;
> + ? ? ? ? ? ? ? if (ts->flags & SYNAPTICS_SWAP_XY)
> + ? ? ? ? ? ? ? ? ? ? ? swap(pos[f][0], pos[f][1]);
> + ? ? ? }
> + ? ? ? if (z) {
> + ? ? ? ? ? ? ? input_report_abs(ts->input_dev, ABS_X, pos[0][0]);
> + ? ? ? ? ? ? ? input_report_abs(ts->input_dev, ABS_Y, pos[0][1]);
> + ? ? ? }
> + ? ? ? input_report_abs(ts->input_dev, ABS_PRESSURE, z);
> + ? ? ? input_report_abs(ts->input_dev, ABS_TOOL_WIDTH, w);
> + ? ? ? input_report_key(ts->input_dev, BTN_TOUCH, finger);
> + ? ? ? finger2_pressed = finger > 1 && finger != 7;
> + ? ? ? input_report_key(ts->input_dev, BTN_2, finger2_pressed);
> + ? ? ? if (finger2_pressed) {
> + ? ? ? ? ? ? ? input_report_abs(ts->input_dev, ABS_HAT0X, pos[1][0]);
> + ? ? ? ? ? ? ? input_report_abs(ts->input_dev, ABS_HAT0Y, pos[1][1]);
> + ? ? ? }
> + ? ? ? input_sync(ts->input_dev);
> +}
> +
> +static void synaptics_ts_work_func(struct work_struct *work)
> +{
> + ? ? ? int i;
> + ? ? ? int ret;
> + ? ? ? int bad_data = 0;
> + ? ? ? struct i2c_msg msg[2];
> + ? ? ? u8 start_reg = 0;
> + ? ? ? u8 buf[15];
> + ? ? ? struct synaptics_ts_data *ts =
> + ? ? ? ? ? ? ? container_of(work, struct synaptics_ts_data, work);
> +
> + ? ? ? msg[0].addr = ts->client->addr;
> + ? ? ? msg[0].flags = 0;
> + ? ? ? msg[0].len = 1;
> + ? ? ? msg[0].buf = &start_reg;
> + ? ? ? msg[1].addr = ts->client->addr;
> + ? ? ? msg[1].flags = I2C_M_RD;
> + ? ? ? msg[1].len = sizeof(buf);
> + ? ? ? msg[1].buf = buf;
> +
> + ? ? ? for (i = 0; i < ((ts->use_irq && !bad_data) ? 1 : 10); i++) {
> + ? ? ? ? ? ? ? ret = i2c_transfer(ts->client->adapter, msg, 2);
> + ? ? ? ? ? ? ? if (ret < 0) {
> + ? ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "ts_work: i2c_transfer failed\n");
> + ? ? ? ? ? ? ? ? ? ? ? bad_data = 1;
> + ? ? ? ? ? ? ? ? ? ? ? continue;
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? if ((buf[14] & 0xc0) != 0x40) {
> + ? ? ? ? ? ? ? ? ? ? ? printk(KERN_WARNING "synaptics_ts_work_func:"
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?" bad read %x %x %x %x %x %x %x %x %x"
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?" %x %x %x %x %x %x, ret %d\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?buf[0], buf[1], buf[2], buf[3],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?buf[4], buf[5], buf[6], buf[7],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?buf[8], buf[9], buf[10], buf[11],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?buf[12], buf[13], buf[14], ret);
> + ? ? ? ? ? ? ? ? ? ? ? if (bad_data)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? synaptics_init_panel(ts);
> + ? ? ? ? ? ? ? ? ? ? ? bad_data = 1;
> + ? ? ? ? ? ? ? ? ? ? ? continue;
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? bad_data = 0;
> + ? ? ? ? ? ? ? if ((buf[14] & 1) == 0)
> + ? ? ? ? ? ? ? ? ? ? ? break;
> +
> + ? ? ? ? ? ? ? decode_report(ts, buf);
> + ? ? ? }
> + ? ? ? if (ts->use_irq)
> + ? ? ? ? ? ? ? enable_irq(ts->client->irq);
> +}
> +
> +static enum hrtimer_restart synaptics_ts_timer_func(struct hrtimer *timer)
> +{
> + ? ? ? struct synaptics_ts_data *ts =
> + ? ? ? ? ? ? ? container_of(timer, struct synaptics_ts_data, timer);
> +
> + ? ? ? queue_work(synaptics_wq, &ts->work);
> +
> + ? ? ? hrtimer_start(&ts->timer, ktime_set(0, 12500000), HRTIMER_MODE_REL);
> + ? ? ? return HRTIMER_NORESTART;
> +}
> +
> +static irqreturn_t synaptics_ts_irq_handler(int irq, void *dev_id)
> +{
> + ? ? ? struct synaptics_ts_data *ts = dev_id;
> +
> + ? ? ? disable_irq(ts->client->irq);

disable_irq_nosync or convert this to request_threaded_irq(...).
Please see recent discussion on linux-input for MAX key switch
controller.

> + ? ? ? queue_work(synaptics_wq, &ts->work);
> + ? ? ? return IRQ_HANDLED;
> +}
> +
> +static int detect(struct synaptics_ts_data *ts, u32 *panel_version)
> +{
> + ? ? ? int ret;
> + ? ? ? int retry = 10;
> +
> + ? ? ? ret = i2c_set(ts, 0xf4, 0x01, "reset device");
> +
> + ? ? ? while (retry-- > 0) {
> + ? ? ? ? ? ? ? ret = i2c_smbus_read_byte_data(ts->client, 0xe4);
> + ? ? ? ? ? ? ? if (ret >= 0)
> + ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? msleep(100);
> + ? ? ? }
> + ? ? ? if (ret < 0) {
> + ? ? ? ? ? ? ? pr_err("i2c_smbus_read_byte_data failed\n");
> + ? ? ? ? ? ? ? return ret;
> + ? ? ? }
> +
> + ? ? ? *panel_version = ret << 8;
> + ? ? ? ret = i2c_read(ts, 0xe5, "product minor");
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? return ret;
> + ? ? ? *panel_version |= ret;
> +
> + ? ? ? ret = i2c_read(ts, 0xe3, "property");
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? return ret;
> +
> + ? ? ? pr_info("synaptics: version %x, product property %x\n",
> + ? ? ? ? ? ? ? *panel_version, ret);
> + ? ? ? return 0;
> +}
> +
> +static void compute_areas(struct synaptics_ts_data *ts,
> + ? ? ? ? ? ? ? ? ? ? ? ? struct synaptics_i2c_rmi_platform_data *pdata,
> + ? ? ? ? ? ? ? ? ? ? ? ? u16 max_x, u16 max_y)
> +{
> + ? ? ? u32 inactive_area_left;
> + ? ? ? u32 inactive_area_right;
> + ? ? ? u32 inactive_area_top;
> + ? ? ? u32 inactive_area_bottom;
> + ? ? ? u32 snap_left_on;
> + ? ? ? u32 snap_left_off;
> + ? ? ? u32 snap_right_on;
> + ? ? ? u32 snap_right_off;
> + ? ? ? u32 snap_top_on;
> + ? ? ? u32 snap_top_off;
> + ? ? ? u32 snap_bottom_on;
> + ? ? ? u32 snap_bottom_off;
> + ? ? ? u32 fuzz_x;
> + ? ? ? u32 fuzz_y;
> + ? ? ? int fuzz_p;
> + ? ? ? int fuzz_w;
> + ? ? ? int swapped = !!(ts->flags & SYNAPTICS_SWAP_XY);
> +
> + ? ? ? inactive_area_left = pdata->inactive_left;
> + ? ? ? inactive_area_right = pdata->inactive_right;
> + ? ? ? inactive_area_top = pdata->inactive_top;
> + ? ? ? inactive_area_bottom = pdata->inactive_bottom;
> + ? ? ? snap_left_on = pdata->snap_left_on;
> + ? ? ? snap_left_off = pdata->snap_left_off;
> + ? ? ? snap_right_on = pdata->snap_right_on;
> + ? ? ? snap_right_off = pdata->snap_right_off;
> + ? ? ? snap_top_on = pdata->snap_top_on;
> + ? ? ? snap_top_off = pdata->snap_top_off;
> + ? ? ? snap_bottom_on = pdata->snap_bottom_on;
> + ? ? ? snap_bottom_off = pdata->snap_bottom_off;
> + ? ? ? fuzz_x = pdata->fuzz_x;
> + ? ? ? fuzz_y = pdata->fuzz_y;
> + ? ? ? fuzz_p = pdata->fuzz_p;
> + ? ? ? fuzz_w = pdata->fuzz_w;
> +
> + ? ? ? inactive_area_left = inactive_area_left * max_x / 0x10000;
> + ? ? ? inactive_area_right = inactive_area_right * max_x / 0x10000;
> + ? ? ? inactive_area_top = inactive_area_top * max_y / 0x10000;
> + ? ? ? inactive_area_bottom = inactive_area_bottom * max_y / 0x10000;
> + ? ? ? snap_left_on = snap_left_on * max_x / 0x10000;
> + ? ? ? snap_left_off = snap_left_off * max_x / 0x10000;
> + ? ? ? snap_right_on = snap_right_on * max_x / 0x10000;
> + ? ? ? snap_right_off = snap_right_off * max_x / 0x10000;
> + ? ? ? snap_top_on = snap_top_on * max_y / 0x10000;
> + ? ? ? snap_top_off = snap_top_off * max_y / 0x10000;
> + ? ? ? snap_bottom_on = snap_bottom_on * max_y / 0x10000;
> + ? ? ? snap_bottom_off = snap_bottom_off * max_y / 0x10000;
> + ? ? ? fuzz_x = fuzz_x * max_x / 0x10000;
> + ? ? ? fuzz_y = fuzz_y * max_y / 0x10000;
> +
> +
> + ? ? ? ts->snap_down[swapped] = -inactive_area_left;
> + ? ? ? ts->snap_up[swapped] = max_x + inactive_area_right;
> + ? ? ? ts->snap_down[!swapped] = -inactive_area_top;
> + ? ? ? ts->snap_up[!swapped] = max_y + inactive_area_bottom;
> + ? ? ? ts->snap_down_on[swapped] = snap_left_on;
> + ? ? ? ts->snap_down_off[swapped] = snap_left_off;
> + ? ? ? ts->snap_up_on[swapped] = max_x - snap_right_on;
> + ? ? ? ts->snap_up_off[swapped] = max_x - snap_right_off;
> + ? ? ? ts->snap_down_on[!swapped] = snap_top_on;
> + ? ? ? ts->snap_down_off[!swapped] = snap_top_off;
> + ? ? ? ts->snap_up_on[!swapped] = max_y - snap_bottom_on;
> + ? ? ? ts->snap_up_off[!swapped] = max_y - snap_bottom_off;
> + ? ? ? pr_info("synaptics_ts_probe: max_x %d, max_y %d\n", max_x, max_y);
> + ? ? ? pr_info("synaptics_ts_probe: inactive_x %d %d, inactive_y %d %d\n",
> + ? ? ? ? ? ? ?inactive_area_left, inactive_area_right,
> + ? ? ? ? ? ? ?inactive_area_top, inactive_area_bottom);
> + ? ? ? pr_info("synaptics_ts_probe: snap_x %d-%d %d-%d, snap_y %d-%d %d-%d\n",
> + ? ? ? ? ? ? ?snap_left_on, snap_left_off, snap_right_on, snap_right_off,
> + ? ? ? ? ? ? ?snap_top_on, snap_top_off, snap_bottom_on, snap_bottom_off);
> +
> + ? ? ? input_set_abs_params(ts->input_dev, ABS_X,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?-inactive_area_left, max_x + inactive_area_right,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?fuzz_x, 0);
> + ? ? ? input_set_abs_params(ts->input_dev, ABS_Y,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?-inactive_area_top, max_y + inactive_area_bottom,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?fuzz_y, 0);
> + ? ? ? input_set_abs_params(ts->input_dev, ABS_PRESSURE, 0, 255, fuzz_p, 0);
> + ? ? ? input_set_abs_params(ts->input_dev, ABS_TOOL_WIDTH, 0, 15, fuzz_w, 0);
> + ? ? ? input_set_abs_params(ts->input_dev, ABS_HAT0X, -inactive_area_left,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?max_x + inactive_area_right, fuzz_x, 0);
> + ? ? ? input_set_abs_params(ts->input_dev, ABS_HAT0Y, -inactive_area_top,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?max_y + inactive_area_bottom, fuzz_y, 0);
> +}
> +
> +static int synaptics_ts_probe(
> + ? ? ? struct i2c_client *client, const struct i2c_device_id *id)
> +{

__devinit ?

> + ? ? ? struct synaptics_ts_data *ts;
> + ? ? ? u8 buf0[4];
> + ? ? ? u8 buf1[8];
> + ? ? ? struct i2c_msg msg[2];
> + ? ? ? int ret = 0;
> + ? ? ? struct synaptics_i2c_rmi_platform_data *pdata;
> + ? ? ? u32 panel_version = 0;
> + ? ? ? u16 max_x, max_y;
> +
> + ? ? ? if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {

check for SMBUS? I have added linux-i2c as this driver has i2c bits,
so not removing any code.

> + ? ? ? ? ? ? ? printk(KERN_ERR "synaptics_ts_probe: need I2C_FUNC_I2C\n");

dev_xxx/pr_xxx friends?

> + ? ? ? ? ? ? ? ret = -ENODEV;
> + ? ? ? ? ? ? ? goto err_check_functionality_failed;
> + ? ? ? }
> +
> + ? ? ? ts = kzalloc(sizeof(*ts), GFP_KERNEL);
> + ? ? ? if (ts == NULL) {
> + ? ? ? ? ? ? ? ret = -ENOMEM;
> + ? ? ? ? ? ? ? goto err_alloc_data_failed;
> + ? ? ? }
> + ? ? ? INIT_WORK(&ts->work, synaptics_ts_work_func);
> + ? ? ? ts->client = client;
> + ? ? ? i2c_set_clientdata(client, ts);
> + ? ? ? pdata = client->dev.platform_data;
> + ? ? ? if (pdata)
> + ? ? ? ? ? ? ? ts->power = pdata->power;
> + ? ? ? else
> + ? ? ? ? ? ? ? pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +
> + ? ? ? if (ts->power) {
> + ? ? ? ? ? ? ? ret = ts->power(1);
> + ? ? ? ? ? ? ? if (ret < 0) {
> + ? ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "synaptics_ts_probe power on failed\n");

Ditto.

> + ? ? ? ? ? ? ? ? ? ? ? goto err_power_failed;
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> +
> + ? ? ? ret = detect(ts, &panel_version);
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? goto err_detect_failed;
> +
> + ? ? ? while (pdata->version > panel_version)
> + ? ? ? ? ? ? ? pdata++;
> + ? ? ? ts->flags = pdata->flags;
> +
> + ? ? ? ret = i2c_read(ts, 0xf0, "device control");
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? goto err_detect_failed;
> + ? ? ? pr_info("synaptics: device control %x\n", ret);
> +
> + ? ? ? ret = i2c_read(ts, 0xf1, "interrupt enable");
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? goto err_detect_failed;
> + ? ? ? pr_info("synaptics_ts_probe: interrupt enable %x\n", ret);
> +
> + ? ? ? ret = i2c_set(ts, 0xf1, 0, "disable interrupt");
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? goto err_detect_failed;
> +
> + ? ? ? msg[0].addr = ts->client->addr;
> + ? ? ? msg[0].flags = 0;
> + ? ? ? msg[0].len = 1;
> + ? ? ? msg[0].buf = buf0;
> + ? ? ? buf0[0] = 0xe0;
> + ? ? ? msg[1].addr = ts->client->addr;
> + ? ? ? msg[1].flags = I2C_M_RD;
> + ? ? ? msg[1].len = 8;
> + ? ? ? msg[1].buf = buf1;
> + ? ? ? ret = i2c_transfer(ts->client->adapter, msg, 2);
> + ? ? ? if (ret < 0) {
> + ? ? ? ? ? ? ? printk(KERN_ERR "i2c_transfer failed\n");
> + ? ? ? ? ? ? ? goto err_detect_failed;
> + ? ? ? }
> + ? ? ? printk(KERN_INFO "synaptics_ts_probe: 0xe0: %x %x %x %x %x %x %x %x\n",
> + ? ? ? ? ? ? ?buf1[0], buf1[1], buf1[2], buf1[3],
> + ? ? ? ? ? ? ?buf1[4], buf1[5], buf1[6], buf1[7]);
> +
> + ? ? ? ret = i2c_set(ts, 0xff, 0x10, "page select = 0x10");
> + ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? goto err_detect_failed;
> +
> + ? ? ? ret = i2c_smbus_read_word_data(ts->client, 0x04);
> + ? ? ? if (ret < 0) {
> + ? ? ? ? ? ? ? printk(KERN_ERR "i2c_smbus_read_word_data failed\n");
> + ? ? ? ? ? ? ? goto err_detect_failed;
> + ? ? ? }
> + ? ? ? ts->max[0] = max_x = (ret >> 8 & 0xff) | ((ret & 0x1f) << 8);
> + ? ? ? ret = i2c_smbus_read_word_data(ts->client, 0x06);
> + ? ? ? if (ret < 0) {
> + ? ? ? ? ? ? ? printk(KERN_ERR "i2c_smbus_read_word_data failed\n");
> + ? ? ? ? ? ? ? goto err_detect_failed;
> + ? ? ? }
> + ? ? ? ts->max[1] = max_y = (ret >> 8 & 0xff) | ((ret & 0x1f) << 8);
> + ? ? ? if (ts->flags & SYNAPTICS_SWAP_XY)
> + ? ? ? ? ? ? ? swap(max_x, max_y);
> +
> + ? ? ? /* will also switch back to page 0x04 */
> + ? ? ? ret = synaptics_init_panel(ts);
> + ? ? ? if (ret < 0) {
> + ? ? ? ? ? ? ? pr_err("synaptics_init_panel failed\n");
> + ? ? ? ? ? ? ? goto err_detect_failed;
> + ? ? ? }
> +
> + ? ? ? ts->input_dev = input_allocate_device();
> + ? ? ? if (ts->input_dev == NULL) {
> + ? ? ? ? ? ? ? ret = -ENOMEM;
> + ? ? ? ? ? ? ? pr_err("synaptics: Failed to allocate input device\n");
> + ? ? ? ? ? ? ? goto err_input_dev_alloc_failed;
> + ? ? ? }
> + ? ? ? ts->input_dev->name = "synaptics-rmi-touchscreen";

other parameters of input_dev, like vendor, bus etc.,

> + ? ? ? set_bit(EV_SYN, ts->input_dev->evbit);
> + ? ? ? set_bit(EV_KEY, ts->input_dev->evbit);
> + ? ? ? set_bit(BTN_TOUCH, ts->input_dev->keybit);
> + ? ? ? set_bit(BTN_2, ts->input_dev->keybit);
> + ? ? ? set_bit(EV_ABS, ts->input_dev->evbit);
> +

__set_bit or input_set_capability please.

> + ? ? ? compute_areas(ts, pdata, max_x, max_y);
> +
> +
> + ? ? ? ret = input_register_device(ts->input_dev);
> + ? ? ? if (ret) {
> + ? ? ? ? ? ? ? pr_err("synaptics: Unable to register %s input device\n",
> + ? ? ? ? ? ? ? ? ? ? ?ts->input_dev->name);
> + ? ? ? ? ? ? ? goto err_input_register_device_failed;
> + ? ? ? }
> + ? ? ? if (client->irq) {
> + ? ? ? ? ? ? ? ret = request_irq(client->irq, synaptics_ts_irq_handler,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0, client->name, ts);
> + ? ? ? ? ? ? ? if (ret == 0) {
> + ? ? ? ? ? ? ? ? ? ? ? ret = i2c_set(ts, 0xf1, 0x01, "enable abs int");
> + ? ? ? ? ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? free_irq(client->irq, ts);
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? if (ret == 0)
> + ? ? ? ? ? ? ? ? ? ? ? ts->use_irq = 1;
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&client->dev, "request_irq failed\n");
> + ? ? ? }
> + ? ? ? if (!ts->use_irq) {
> + ? ? ? ? ? ? ? hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + ? ? ? ? ? ? ? ts->timer.function = synaptics_ts_timer_func;
> + ? ? ? ? ? ? ? hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
> + ? ? ? }
> +
> + ? ? ? pr_info("synaptics: Start touchscreen %s in %s mode\n",
> + ? ? ? ? ? ? ? ts->input_dev->name, ts->use_irq ? "interrupt" : "polling");
> +
> + ? ? ? return 0;
> +
> + err_input_register_device_failed:
> + ? ? ? input_free_device(ts->input_dev);
> +
> + err_input_dev_alloc_failed:
> + err_detect_failed:
> + err_power_failed:
> + ? ? ? kfree(ts);
> + err_alloc_data_failed:
> + err_check_functionality_failed:
> + ? ? ? return ret;
> +}
> +
> +static int synaptics_ts_remove(struct i2c_client *client)
> +{

__devexit

> + ? ? ? struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> +
> + ? ? ? if (ts->use_irq)
> + ? ? ? ? ? ? ? free_irq(client->irq, ts);
> + ? ? ? else
> + ? ? ? ? ? ? ? hrtimer_cancel(&ts->timer);
> + ? ? ? input_unregister_device(ts->input_dev);
> + ? ? ? kfree(ts);
> + ? ? ? return 0;
> +}
> +
> +static int synaptics_ts_suspend(struct i2c_client *client, pm_message_t mesg)
> +{

#ifdef CONFIG_PM ?

> + ? ? ? int ret;
> + ? ? ? struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> +
> + ? ? ? if (ts->use_irq)
> + ? ? ? ? ? ? ? disable_irq(client->irq);
> + ? ? ? else
> + ? ? ? ? ? ? ? hrtimer_cancel(&ts->timer);
> + ? ? ? ret = cancel_work_sync(&ts->work);
> + ? ? ? if (ret && ts->use_irq) /* if work was pending disable-count is now 2 */
> + ? ? ? ? ? ? ? enable_irq(client->irq);
> + ? ? ? i2c_set(ts, 0xf1, 0, "disable interrupt");
> + ? ? ? i2c_set(ts, 0xf0, 0x86, "deep sleep");
> +
> + ? ? ? if (ts->power) {
> + ? ? ? ? ? ? ? ret = ts->power(0);
> + ? ? ? ? ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? ? ? ? ? pr_err("synaptics_ts_suspend power off failed\n");
> + ? ? ? }
> + ? ? ? return 0;
> +}
> +
> +static int synaptics_ts_resume(struct i2c_client *client)
> +{
> + ? ? ? int ret;
> + ? ? ? struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> +
> + ? ? ? if (ts->power) {
> + ? ? ? ? ? ? ? ret = ts->power(1);
> + ? ? ? ? ? ? ? if (ret < 0)
> + ? ? ? ? ? ? ? ? ? ? ? pr_err("synaptics_ts_resume power on failed\n");
> + ? ? ? }
> +
> + ? ? ? synaptics_init_panel(ts);
> +
> + ? ? ? if (ts->use_irq) {
> + ? ? ? ? ? ? ? enable_irq(client->irq);
> + ? ? ? ? ? ? ? i2c_set(ts, 0xf1, 0x01, "enable abs int");
> + ? ? ? } else
> + ? ? ? ? ? ? ? hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
> +
> + ? ? ? return 0;
> +}
> +
> +
> +static const struct i2c_device_id synaptics_ts_id[] = {
> + ? ? ? { SYNAPTICS_I2C_RMI_NAME, 0 },
> + ? ? ? { }
> +};
> +
> +static struct i2c_driver synaptics_ts_driver = {
> + ? ? ? .probe ? ? ? ? ?= synaptics_ts_probe,
> + ? ? ? .remove ? ? ? ? = synaptics_ts_remove,

__devexit_p

> + ? ? ? .suspend ? ? ? ?= synaptics_ts_suspend,
> + ? ? ? .resume ? ? ? ? = synaptics_ts_resume,
> + ? ? ? .id_table ? ? ? = synaptics_ts_id,
> + ? ? ? .driver = {
> + ? ? ? ? ? ? ? .name ? = SYNAPTICS_I2C_RMI_NAME,
> + ? ? ? },
> +};
> +
> +static int __devinit synaptics_ts_init(void)
> +{
> + ? ? ? synaptics_wq = create_singlethread_workqueue("synaptics_wq");
> + ? ? ? if (!synaptics_wq)
> + ? ? ? ? ? ? ? return -ENOMEM;
> + ? ? ? return i2c_add_driver(&synaptics_ts_driver);
> +}
> +
> +static void __exit synaptics_ts_exit(void)
> +{
> + ? ? ? i2c_del_driver(&synaptics_ts_driver);
> + ? ? ? if (synaptics_wq)
> + ? ? ? ? ? ? ? destroy_workqueue(synaptics_wq);
> +}
> +
> +module_init(synaptics_ts_init);
> +module_exit(synaptics_ts_exit);
> +
> +MODULE_DESCRIPTION("Synaptics Touchscreen Driver");
> +MODULE_LICENSE("GPL");

MODULE_ALIAS?

> diff --git a/include/linux/synaptics_i2c_rmi.h b/include/linux/synaptics_i2c_rmi.h
> new file mode 100644
> index 0000000..73e1de7
> --- /dev/null
> +++ b/include/linux/synaptics_i2c_rmi.h
> @@ -0,0 +1,53 @@
> +/*
> + * synaptics_i2c_rmi.h - platform data structure for f75375s sensor
> + *
> + * Copyright (C) 2008 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * 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.
> + *
> + */
> +
> +#ifndef _LINUX_SYNAPTICS_I2C_RMI_H
> +#define _LINUX_SYNAPTICS_I2C_RMI_H
> +
> +#define SYNAPTICS_I2C_RMI_NAME "synaptics-rmi-ts"
> +
> +enum {
> + ? ? ? SYNAPTICS_FLIP_X = 1UL << 0,
> + ? ? ? SYNAPTICS_FLIP_Y = 1UL << 1,
> + ? ? ? SYNAPTICS_SWAP_XY = 1UL << 2,
> + ? ? ? SYNAPTICS_SNAP_TO_INACTIVE_EDGE = 1UL << 3,
> +};
> +
> +struct synaptics_i2c_rmi_platform_data {
> + ? ? ? u32 version; ? ?/* Use this entry for panels with */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* (major << 8 | minor) version or above. */
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* If non-zero another array entry follows */
> + ? ? ? int (*power)(int on); ? /* Only valid in first array entry */
> + ? ? ? u32 flags;
> + ? ? ? u32 inactive_left; /* 0x10000 = screen width */
> + ? ? ? u32 inactive_right; /* 0x10000 = screen width */
> + ? ? ? u32 inactive_top; /* 0x10000 = screen height */
> + ? ? ? u32 inactive_bottom; /* 0x10000 = screen height */
> + ? ? ? u32 snap_left_on; /* 0x10000 = screen width */
> + ? ? ? u32 snap_left_off; /* 0x10000 = screen width */
> + ? ? ? u32 snap_right_on; /* 0x10000 = screen width */
> + ? ? ? u32 snap_right_off; /* 0x10000 = screen width */
> + ? ? ? u32 snap_top_on; /* 0x10000 = screen height */
> + ? ? ? u32 snap_top_off; /* 0x10000 = screen height */
> + ? ? ? u32 snap_bottom_on; /* 0x10000 = screen height */
> + ? ? ? u32 snap_bottom_off; /* 0x10000 = screen height */
> + ? ? ? u32 fuzz_x; /* 0x10000 = screen width */
> + ? ? ? u32 fuzz_y; /* 0x10000 = screen height */
> + ? ? ? int fuzz_p;
> + ? ? ? int fuzz_w;
> +};
> +
> +#endif /* _LINUX_SYNAPTICS_I2C_RMI_H */
>

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-14 17:52:21

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

On Tue, Jul 14, 2009 at 12:06:34PM +0200, Pavel Machek wrote:
> From: Arve Hj?nnev?g <[email protected]>
>
> This adds support for synaptic touchscreen, used in HTC dream
> cellphone.
>
> Signed-off-by: Pavel Machek <[email protected]>

This is pretty large body of code, could we get a sign-off from Arve as
well since he seems to be the author?

>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index bb6486a..fa3404f 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -206,6 +213,14 @@ config TOUCHSCREEN_MIGOR
> To compile this driver as a module, choose M here: the
> module will be called migor_ts.
>
> +config TOUCHSCREEN_SYNAPTICS_I2C_RMI
> + tristate "Synaptics i2c touchscreen"
> + depends on I2C
> + help
> + This enables support for Synaptics RMI over I2C based touchscreens.
> + Such touchscreen is used in HTC Dream.
> +

To compile this driver as a module...

> +
> config TOUCHSCREEN_TOUCHRIGHT
> tristate "Touchright serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index d3375af..959dcda 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -22,6 +23,7 @@ obj-$(CONFIG_TOUCHSCREEN_HP7XX) += jornada720_ts.o
> obj-$(CONFIG_TOUCHSCREEN_HTCPEN) += htcpen.o
> obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE) += usbtouchscreen.o
> obj-$(CONFIG_TOUCHSCREEN_PENMOUNT) += penmount.o
> +obj-$(CONFIG_TOUCHSCREEN_SYNAPTICS_I2C_RMI) += synaptics_i2c_rmi.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) += touchright.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) += touchwin.o
> diff --git a/drivers/input/touchscreen/synaptics_i2c_rmi.c b/drivers/input/touchscreen/synaptics_i2c_rmi.c
> new file mode 100644
> index 0000000..fe10e85
> --- /dev/null
> +++ b/drivers/input/touchscreen/synaptics_i2c_rmi.c
> @@ -0,0 +1,587 @@
> +/*
> + * Support for synaptics touchscreen.
> + *
> + * Copyright (C) 2007 Google, Inc.
> + * Author: Arve Hj?nnev?g <[email protected]>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * 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/module.h>
> +#include <linux/delay.h>
> +#include <linux/hrtimer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/synaptics_i2c_rmi.h>
> +
> +static struct workqueue_struct *synaptics_wq;

Do we need a separate workqueue? Is reading the device that slow that we
can use keventd?

> +
> +struct synaptics_ts_data {
> + u16 addr;
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + int use_irq;
> + struct hrtimer timer;
> + struct work_struct work;
> + u16 max[2];
> + int snap_state[2][2];
> + int snap_down_on[2];
> + int snap_down_off[2];
> + int snap_up_on[2];
> + int snap_up_off[2];
> + int snap_down[2];
> + int snap_up[2];
> + u32 flags;
> + int (*power)(int on);

'bool on'?

> +};
> +
> +static int i2c_set(struct synaptics_ts_data *ts, u8 reg, u8 val, char *msg)
> +{
> + int ret = i2c_smbus_write_byte_data(ts->client, reg, val);
> + if (ret < 0)
> + pr_err("i2c_smbus_write_byte_data failed (%s)\n", msg);
> + return ret;
> +}
> +
> +static int i2c_read(struct synaptics_ts_data *ts, u8 reg, char *msg)
> +{
> + int ret = i2c_smbus_read_byte_data(ts->client, 0xf0);

We are not using 'reg'?

> + if (ret < 0)
> + pr_err("i2c_smbus_read_byte_data failed (%s)\n", msg);
> + return ret;
> +}
> +
> +static int synaptics_init_panel(struct synaptics_ts_data *ts)
> +{
> + int ret;
> +
> + ret = i2c_set(ts, 0xff, 0x10, "set page select");
> + if (ret == 0)
> + ret = i2c_set(ts, 0x41, 0x04, "set No Clip Z");
> +
> + ret = i2c_set(ts, 0xff, 0x04, "fallback page select");
> + ret = i2c_set(ts, 0xf0, 0x81, "select 80 reports per second");
> + return ret;
> +}
> +
> +static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
> +{
> + int pos[2][2];
> + int f, a;
> + int base = 2;
> + int z = buf[1];
> + int w = buf[0] >> 4;
> + int finger = buf[0] & 7;
> + int finger2_pressed;
> +
> + for (f = 0; f < 2; f++) {
> + u32 flip_flag = SYNAPTICS_FLIP_X;
> + for (a = 0; a < 2; a++) {
> + int p = buf[base + 1];
> + p |= (u16)(buf[base] & 0x1f) << 8;
> + if (ts->flags & flip_flag)
> + p = ts->max[a] - p;
> + if (ts->flags & SYNAPTICS_SNAP_TO_INACTIVE_EDGE) {
> + if (ts->snap_state[f][a]) {
> + if (p <= ts->snap_down_off[a])
> + p = ts->snap_down[a];
> + else if (p >= ts->snap_up_off[a])
> + p = ts->snap_up[a];
> + else
> + ts->snap_state[f][a] = 0;
> + } else {
> + if (p <= ts->snap_down_on[a]) {
> + p = ts->snap_down[a];
> + ts->snap_state[f][a] = 1;
> + } else if (p >= ts->snap_up_on[a]) {
> + p = ts->snap_up[a];
> + ts->snap_state[f][a] = 1;
> + }
> + }
> + }
> + pos[f][a] = p;
> + base += 2;
> + flip_flag <<= 1;

Some more info as to what you are trying to do here would be nice.

> + }
> + base += 2;
> + if (ts->flags & SYNAPTICS_SWAP_XY)
> + swap(pos[f][0], pos[f][1]);
> + }
> + if (z) {
> + input_report_abs(ts->input_dev, ABS_X, pos[0][0]);
> + input_report_abs(ts->input_dev, ABS_Y, pos[0][1]);
> + }
> + input_report_abs(ts->input_dev, ABS_PRESSURE, z);
> + input_report_abs(ts->input_dev, ABS_TOOL_WIDTH, w);
> + input_report_key(ts->input_dev, BTN_TOUCH, finger);
> + finger2_pressed = finger > 1 && finger != 7;
> + input_report_key(ts->input_dev, BTN_2, finger2_pressed);
> + if (finger2_pressed) {
> + input_report_abs(ts->input_dev, ABS_HAT0X, pos[1][0]);
> + input_report_abs(ts->input_dev, ABS_HAT0Y, pos[1][1]);

ABS_HAT is pretty unusual for a touchscreen...

> + }
> + input_sync(ts->input_dev);
> +}
> +
> +static void synaptics_ts_work_func(struct work_struct *work)
> +{
> + int i;
> + int ret;
> + int bad_data = 0;
> + struct i2c_msg msg[2];
> + u8 start_reg = 0;
> + u8 buf[15];
> + struct synaptics_ts_data *ts =
> + container_of(work, struct synaptics_ts_data, work);
> +
> + msg[0].addr = ts->client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 1;
> + msg[0].buf = &start_reg;
> + msg[1].addr = ts->client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = sizeof(buf);
> + msg[1].buf = buf;
> +
> + for (i = 0; i < ((ts->use_irq && !bad_data) ? 1 : 10); i++) {
> + ret = i2c_transfer(ts->client->adapter, msg, 2);
> + if (ret < 0) {
> + printk(KERN_ERR "ts_work: i2c_transfer failed\n");
> + bad_data = 1;
> + continue;
> + }
> + if ((buf[14] & 0xc0) != 0x40) {
> + printk(KERN_WARNING "synaptics_ts_work_func:"
> + " bad read %x %x %x %x %x %x %x %x %x"
> + " %x %x %x %x %x %x, ret %d\n",
> + buf[0], buf[1], buf[2], buf[3],
> + buf[4], buf[5], buf[6], buf[7],
> + buf[8], buf[9], buf[10], buf[11],
> + buf[12], buf[13], buf[14], ret);

Umm... for ()?

> + if (bad_data)
> + synaptics_init_panel(ts);
> + bad_data = 1;
> + continue;
> + }
> + bad_data = 0;
> + if ((buf[14] & 1) == 0)
> + break;
> +
> + decode_report(ts, buf);
> + }
> + if (ts->use_irq)
> + enable_irq(ts->client->irq);
> +}
> +
> +static enum hrtimer_restart synaptics_ts_timer_func(struct hrtimer *timer)
> +{
> + struct synaptics_ts_data *ts =
> + container_of(timer, struct synaptics_ts_data, timer);
> +
> + queue_work(synaptics_wq, &ts->work);
> +
> + hrtimer_start(&ts->timer, ktime_set(0, 12500000), HRTIMER_MODE_REL);

I was always wondering why use high-resolution timer and then offload to
a workqueue which scheduling we have no idea about. Why can't we simply
reschedule work in case when we are in polling mode?

> + return HRTIMER_NORESTART;
> +}
> +
> +static irqreturn_t synaptics_ts_irq_handler(int irq, void *dev_id)
> +{
> + struct synaptics_ts_data *ts = dev_id;
> +
> + disable_irq(ts->client->irq);
> + queue_work(synaptics_wq, &ts->work);
> + return IRQ_HANDLED;
> +}
> +
> +static int detect(struct synaptics_ts_data *ts, u32 *panel_version)
> +{
> + int ret;
> + int retry = 10;
> +
> + ret = i2c_set(ts, 0xf4, 0x01, "reset device");
> +
> + while (retry-- > 0) {
> + ret = i2c_smbus_read_byte_data(ts->client, 0xe4);
> + if (ret >= 0)
> + break;
> + msleep(100);
> + }
> + if (ret < 0) {
> + pr_err("i2c_smbus_read_byte_data failed\n");
> + return ret;
> + }
> +
> + *panel_version = ret << 8;
> + ret = i2c_read(ts, 0xe5, "product minor");
> + if (ret < 0)
> + return ret;
> + *panel_version |= ret;
> +
> + ret = i2c_read(ts, 0xe3, "property");
> + if (ret < 0)
> + return ret;
> +
> + pr_info("synaptics: version %x, product property %x\n",
> + *panel_version, ret);
> + return 0;
> +}
> +
> +static void compute_areas(struct synaptics_ts_data *ts,
> + struct synaptics_i2c_rmi_platform_data *pdata,
> + u16 max_x, u16 max_y)
> +{
> + u32 inactive_area_left;
> + u32 inactive_area_right;
> + u32 inactive_area_top;
> + u32 inactive_area_bottom;
> + u32 snap_left_on;
> + u32 snap_left_off;
> + u32 snap_right_on;
> + u32 snap_right_off;
> + u32 snap_top_on;
> + u32 snap_top_off;
> + u32 snap_bottom_on;
> + u32 snap_bottom_off;
> + u32 fuzz_x;
> + u32 fuzz_y;
> + int fuzz_p;
> + int fuzz_w;
> + int swapped = !!(ts->flags & SYNAPTICS_SWAP_XY);
> +
> + inactive_area_left = pdata->inactive_left;
> + inactive_area_right = pdata->inactive_right;
> + inactive_area_top = pdata->inactive_top;
> + inactive_area_bottom = pdata->inactive_bottom;
> + snap_left_on = pdata->snap_left_on;
> + snap_left_off = pdata->snap_left_off;
> + snap_right_on = pdata->snap_right_on;
> + snap_right_off = pdata->snap_right_off;
> + snap_top_on = pdata->snap_top_on;
> + snap_top_off = pdata->snap_top_off;
> + snap_bottom_on = pdata->snap_bottom_on;
> + snap_bottom_off = pdata->snap_bottom_off;
> + fuzz_x = pdata->fuzz_x;
> + fuzz_y = pdata->fuzz_y;
> + fuzz_p = pdata->fuzz_p;
> + fuzz_w = pdata->fuzz_w;
> +
> + inactive_area_left = inactive_area_left * max_x / 0x10000;
> + inactive_area_right = inactive_area_right * max_x / 0x10000;
> + inactive_area_top = inactive_area_top * max_y / 0x10000;
> + inactive_area_bottom = inactive_area_bottom * max_y / 0x10000;
> + snap_left_on = snap_left_on * max_x / 0x10000;
> + snap_left_off = snap_left_off * max_x / 0x10000;
> + snap_right_on = snap_right_on * max_x / 0x10000;
> + snap_right_off = snap_right_off * max_x / 0x10000;
> + snap_top_on = snap_top_on * max_y / 0x10000;
> + snap_top_off = snap_top_off * max_y / 0x10000;
> + snap_bottom_on = snap_bottom_on * max_y / 0x10000;
> + snap_bottom_off = snap_bottom_off * max_y / 0x10000;
> + fuzz_x = fuzz_x * max_x / 0x10000;
> + fuzz_y = fuzz_y * max_y / 0x10000;
> +
> +
> + ts->snap_down[swapped] = -inactive_area_left;
> + ts->snap_up[swapped] = max_x + inactive_area_right;
> + ts->snap_down[!swapped] = -inactive_area_top;
> + ts->snap_up[!swapped] = max_y + inactive_area_bottom;
> + ts->snap_down_on[swapped] = snap_left_on;
> + ts->snap_down_off[swapped] = snap_left_off;
> + ts->snap_up_on[swapped] = max_x - snap_right_on;
> + ts->snap_up_off[swapped] = max_x - snap_right_off;
> + ts->snap_down_on[!swapped] = snap_top_on;
> + ts->snap_down_off[!swapped] = snap_top_off;
> + ts->snap_up_on[!swapped] = max_y - snap_bottom_on;
> + ts->snap_up_off[!swapped] = max_y - snap_bottom_off;
> + pr_info("synaptics_ts_probe: max_x %d, max_y %d\n", max_x, max_y);
> + pr_info("synaptics_ts_probe: inactive_x %d %d, inactive_y %d %d\n",
> + inactive_area_left, inactive_area_right,
> + inactive_area_top, inactive_area_bottom);
> + pr_info("synaptics_ts_probe: snap_x %d-%d %d-%d, snap_y %d-%d %d-%d\n",
> + snap_left_on, snap_left_off, snap_right_on, snap_right_off,
> + snap_top_on, snap_top_off, snap_bottom_on, snap_bottom_off);
> +
> + input_set_abs_params(ts->input_dev, ABS_X,
> + -inactive_area_left, max_x + inactive_area_right,
> + fuzz_x, 0);
> + input_set_abs_params(ts->input_dev, ABS_Y,
> + -inactive_area_top, max_y + inactive_area_bottom,
> + fuzz_y, 0);
> + input_set_abs_params(ts->input_dev, ABS_PRESSURE, 0, 255, fuzz_p, 0);
> + input_set_abs_params(ts->input_dev, ABS_TOOL_WIDTH, 0, 15, fuzz_w, 0);
> + input_set_abs_params(ts->input_dev, ABS_HAT0X, -inactive_area_left,
> + max_x + inactive_area_right, fuzz_x, 0);
> + input_set_abs_params(ts->input_dev, ABS_HAT0Y, -inactive_area_top,
> + max_y + inactive_area_bottom, fuzz_y, 0);
> +}
> +
> +static int synaptics_ts_probe(

__devinit()

> + struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct synaptics_ts_data *ts;
> + u8 buf0[4];
> + u8 buf1[8];
> + struct i2c_msg msg[2];
> + int ret = 0;
> + struct synaptics_i2c_rmi_platform_data *pdata;
> + u32 panel_version = 0;
> + u16 max_x, max_y;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + printk(KERN_ERR "synaptics_ts_probe: need I2C_FUNC_I2C\n");
> + ret = -ENODEV;
> + goto err_check_functionality_failed;
> + }
> +
> + ts = kzalloc(sizeof(*ts), GFP_KERNEL);
> + if (ts == NULL) {
> + ret = -ENOMEM;
> + goto err_alloc_data_failed;
> + }
> + INIT_WORK(&ts->work, synaptics_ts_work_func);
> + ts->client = client;
> + i2c_set_clientdata(client, ts);
> + pdata = client->dev.platform_data;
> + if (pdata)
> + ts->power = pdata->power;
> + else
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);

Where is it freed?

> +
> + if (ts->power) {
> + ret = ts->power(1);
> + if (ret < 0) {
> + printk(KERN_ERR "synaptics_ts_probe power on failed\n");
> + goto err_power_failed;
> + }
> + }
> +
> + ret = detect(ts, &panel_version);
> + if (ret)
> + goto err_detect_failed;
> +
> + while (pdata->version > panel_version)
> + pdata++;
> + ts->flags = pdata->flags;
> +
> + ret = i2c_read(ts, 0xf0, "device control");
> + if (ret < 0)
> + goto err_detect_failed;
> + pr_info("synaptics: device control %x\n", ret);
> +
> + ret = i2c_read(ts, 0xf1, "interrupt enable");
> + if (ret < 0)
> + goto err_detect_failed;
> + pr_info("synaptics_ts_probe: interrupt enable %x\n", ret);
> +
> + ret = i2c_set(ts, 0xf1, 0, "disable interrupt");
> + if (ret < 0)
> + goto err_detect_failed;
> +
> + msg[0].addr = ts->client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 1;
> + msg[0].buf = buf0;
> + buf0[0] = 0xe0;
> + msg[1].addr = ts->client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = 8;
> + msg[1].buf = buf1;
> + ret = i2c_transfer(ts->client->adapter, msg, 2);
> + if (ret < 0) {
> + printk(KERN_ERR "i2c_transfer failed\n");
> + goto err_detect_failed;
> + }
> + printk(KERN_INFO "synaptics_ts_probe: 0xe0: %x %x %x %x %x %x %x %x\n",
> + buf1[0], buf1[1], buf1[2], buf1[3],
> + buf1[4], buf1[5], buf1[6], buf1[7]);
> +
> + ret = i2c_set(ts, 0xff, 0x10, "page select = 0x10");
> + if (ret < 0)
> + goto err_detect_failed;
> +
> + ret = i2c_smbus_read_word_data(ts->client, 0x04);
> + if (ret < 0) {
> + printk(KERN_ERR "i2c_smbus_read_word_data failed\n");
> + goto err_detect_failed;
> + }
> + ts->max[0] = max_x = (ret >> 8 & 0xff) | ((ret & 0x1f) << 8);
> + ret = i2c_smbus_read_word_data(ts->client, 0x06);
> + if (ret < 0) {
> + printk(KERN_ERR "i2c_smbus_read_word_data failed\n");
> + goto err_detect_failed;
> + }
> + ts->max[1] = max_y = (ret >> 8 & 0xff) | ((ret & 0x1f) << 8);
> + if (ts->flags & SYNAPTICS_SWAP_XY)
> + swap(max_x, max_y);
> +
> + /* will also switch back to page 0x04 */
> + ret = synaptics_init_panel(ts);
> + if (ret < 0) {
> + pr_err("synaptics_init_panel failed\n");
> + goto err_detect_failed;
> + }
> +
> + ts->input_dev = input_allocate_device();
> + if (ts->input_dev == NULL) {
> + ret = -ENOMEM;
> + pr_err("synaptics: Failed to allocate input device\n");
> + goto err_input_dev_alloc_failed;
> + }
> + ts->input_dev->name = "synaptics-rmi-touchscreen";

BUS_I2C, etc.

> + set_bit(EV_SYN, ts->input_dev->evbit);

__set_bit(), no need to lock the bus.

> + set_bit(EV_KEY, ts->input_dev->evbit);
> + set_bit(BTN_TOUCH, ts->input_dev->keybit);
> + set_bit(BTN_2, ts->input_dev->keybit);

BTN_2 is kinda generic... Normally joysticks use it...

> + set_bit(EV_ABS, ts->input_dev->evbit);
> +
> + compute_areas(ts, pdata, max_x, max_y);
> +
> +
> + ret = input_register_device(ts->input_dev);
> + if (ret) {
> + pr_err("synaptics: Unable to register %s input device\n",
> + ts->input_dev->name);
> + goto err_input_register_device_failed;
> + }
> + if (client->irq) {
> + ret = request_irq(client->irq, synaptics_ts_irq_handler,
> + 0, client->name, ts);

I think threaded IRQ will fit the bill and will take care of
IRQ/workqueue shutdown races. Of course you still need to use workqueue
if polling.

> + if (ret == 0) {
> + ret = i2c_set(ts, 0xf1, 0x01, "enable abs int");
> + if (ret)
> + free_irq(client->irq, ts);
> + }
> + if (ret == 0)
> + ts->use_irq = 1;
> + else
> + dev_err(&client->dev, "request_irq failed\n");
> + }
> + if (!ts->use_irq) {
> + hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + ts->timer.function = synaptics_ts_timer_func;
> + hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
> + }
> +
> + pr_info("synaptics: Start touchscreen %s in %s mode\n",

Probably "synaptics_ts" throughout so nobody is confused?

> + ts->input_dev->name, ts->use_irq ? "interrupt" : "polling");
> +
> + return 0;
> +
> + err_input_register_device_failed:
> + input_free_device(ts->input_dev);
> +

Don't see canceling timer nor shutting off WQ here. Also, maybe
implement open() and close() so we don't reschedule WQ while polling?

> + err_input_dev_alloc_failed:
> + err_detect_failed:
> + err_power_failed:
> + kfree(ts);
> + err_alloc_data_failed:
> + err_check_functionality_failed:
> + return ret;
> +}
> +
> +static int synaptics_ts_remove(struct i2c_client *client)

__devexit()

> +{
> + struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> +
> + if (ts->use_irq)
> + free_irq(client->irq, ts);
> + else
> + hrtimer_cancel(&ts->timer);

What about the work?

> + input_unregister_device(ts->input_dev);
> + kfree(ts);
> + return 0;
> +}
> +
> +static int synaptics_ts_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> + int ret;
> + struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> +
> + if (ts->use_irq)
> + disable_irq(client->irq);
> + else
> + hrtimer_cancel(&ts->timer);
> + ret = cancel_work_sync(&ts->work);
> + if (ret && ts->use_irq) /* if work was pending disable-count is now 2 */
> + enable_irq(client->irq);
> + i2c_set(ts, 0xf1, 0, "disable interrupt");
> + i2c_set(ts, 0xf0, 0x86, "deep sleep");
> +
> + if (ts->power) {
> + ret = ts->power(0);
> + if (ret < 0)
> + pr_err("synaptics_ts_suspend power off failed\n");
> + }
> + return 0;
> +}
> +
> +static int synaptics_ts_resume(struct i2c_client *client)
> +{
> + int ret;
> + struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> +
> + if (ts->power) {
> + ret = ts->power(1);
> + if (ret < 0)
> + pr_err("synaptics_ts_resume power on failed\n");
> + }
> +
> + synaptics_init_panel(ts);
> +
> + if (ts->use_irq) {
> + enable_irq(client->irq);
> + i2c_set(ts, 0xf1, 0x01, "enable abs int");
> + } else
> + hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
> +
> + return 0;
> +}
> +
> +
> +static const struct i2c_device_id synaptics_ts_id[] = {
> + { SYNAPTICS_I2C_RMI_NAME, 0 },
> + { }
> +};
> +
> +static struct i2c_driver synaptics_ts_driver = {
> + .probe = synaptics_ts_probe,
> + .remove = synaptics_ts_remove,
> + .suspend = synaptics_ts_suspend,
> + .resume = synaptics_ts_resume,
> + .id_table = synaptics_ts_id,
> + .driver = {
> + .name = SYNAPTICS_I2C_RMI_NAME,
> + },
> +};
> +
> +static int __devinit synaptics_ts_init(void)
> +{
> + synaptics_wq = create_singlethread_workqueue("synaptics_wq");
> + if (!synaptics_wq)
> + return -ENOMEM;
> + return i2c_add_driver(&synaptics_ts_driver);
> +}
> +
> +static void __exit synaptics_ts_exit(void)
> +{
> + i2c_del_driver(&synaptics_ts_driver);
> + if (synaptics_wq)
> + destroy_workqueue(synaptics_wq);
> +}
> +
> +module_init(synaptics_ts_init);
> +module_exit(synaptics_ts_exit);
> +
> +MODULE_DESCRIPTION("Synaptics Touchscreen Driver");
> +MODULE_LICENSE("GPL");

MODULE_AUTHOR()?

> diff --git a/include/linux/synaptics_i2c_rmi.h b/include/linux/synaptics_i2c_rmi.h
> new file mode 100644
> index 0000000..73e1de7
> --- /dev/null
> +++ b/include/linux/synaptics_i2c_rmi.h
> @@ -0,0 +1,53 @@
> +/*
> + * synaptics_i2c_rmi.h - platform data structure for f75375s sensor
> + *
> + * Copyright (C) 2008 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * 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.
> + *
> + */
> +
> +#ifndef _LINUX_SYNAPTICS_I2C_RMI_H
> +#define _LINUX_SYNAPTICS_I2C_RMI_H
> +
> +#define SYNAPTICS_I2C_RMI_NAME "synaptics-rmi-ts"
> +
> +enum {
> + SYNAPTICS_FLIP_X = 1UL << 0,
> + SYNAPTICS_FLIP_Y = 1UL << 1,
> + SYNAPTICS_SWAP_XY = 1UL << 2,
> + SYNAPTICS_SNAP_TO_INACTIVE_EDGE = 1UL << 3,
> +};
> +
> +struct synaptics_i2c_rmi_platform_data {
> + u32 version; /* Use this entry for panels with */
> + /* (major << 8 | minor) version or above. */
> + /* If non-zero another array entry follows */
> + int (*power)(int on); /* Only valid in first array entry */
> + u32 flags;
> + u32 inactive_left; /* 0x10000 = screen width */
> + u32 inactive_right; /* 0x10000 = screen width */
> + u32 inactive_top; /* 0x10000 = screen height */
> + u32 inactive_bottom; /* 0x10000 = screen height */
> + u32 snap_left_on; /* 0x10000 = screen width */
> + u32 snap_left_off; /* 0x10000 = screen width */
> + u32 snap_right_on; /* 0x10000 = screen width */
> + u32 snap_right_off; /* 0x10000 = screen width */
> + u32 snap_top_on; /* 0x10000 = screen height */
> + u32 snap_top_off; /* 0x10000 = screen height */
> + u32 snap_bottom_on; /* 0x10000 = screen height */
> + u32 snap_bottom_off; /* 0x10000 = screen height */
> + u32 fuzz_x; /* 0x10000 = screen width */
> + u32 fuzz_y; /* 0x10000 = screen height */
> + int fuzz_p;
> + int fuzz_w;
> +};
> +
> +#endif /* _LINUX_SYNAPTICS_I2C_RMI_H */
>
>

Thanks.

--
Dmitry

2009-07-14 20:25:47

by Andreas Mohr

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi,

> + ts->snap_down[swapped] = -inactive_area_left;
> + ts->snap_up[swapped] = max_x + inactive_area_right;
> + ts->snap_down[!swapped] = -inactive_area_top;
> + ts->snap_up[!swapped] = max_y + inactive_area_bottom;
> + ts->snap_down_on[swapped] = snap_left_on;
> + ts->snap_down_off[swapped] = snap_left_off;
> + ts->snap_up_on[swapped] = max_x - snap_right_on;
> + ts->snap_up_off[swapped] = max_x - snap_right_off;
> + ts->snap_down_on[!swapped] = snap_top_on;
> + ts->snap_down_off[!swapped] = snap_top_off;
> + ts->snap_up_on[!swapped] = max_y - snap_bottom_on;
> + ts->snap_up_off[!swapped] = max_y - snap_bottom_off;

Could this perhaps be represented by _one_ struct definition
and two representations of it, one for swapped and one for non-swapped case or so?
(although sometimes it?s reverted logic, might need more thought)
Sounds like an awful lot of repeated array calculations for no overly good reason
(unless gcc happens to fully optimize it into oblivion automatically).

Andreas Mohr

2009-07-14 20:33:05

by Andreas Mohr

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi,

On Tue, Jul 14, 2009 at 10:25:45PM +0200, Andreas Mohr wrote:
> Hi,
>
> > + ts->snap_down[swapped] = -inactive_area_left;
> > + ts->snap_up[swapped] = max_x + inactive_area_right;
> > + ts->snap_down[!swapped] = -inactive_area_top;
> > + ts->snap_up[!swapped] = max_y + inactive_area_bottom;
> > + ts->snap_down_on[swapped] = snap_left_on;
> > + ts->snap_down_off[swapped] = snap_left_off;
> > + ts->snap_up_on[swapped] = max_x - snap_right_on;
> > + ts->snap_up_off[swapped] = max_x - snap_right_off;
> > + ts->snap_down_on[!swapped] = snap_top_on;
> > + ts->snap_down_off[!swapped] = snap_top_off;
> > + ts->snap_up_on[!swapped] = max_y - snap_bottom_on;
> > + ts->snap_up_off[!swapped] = max_y - snap_bottom_off;
>
> Could this perhaps be represented by _one_ struct definition
> and two representations of it, one for swapped and one for non-swapped case or so?
> (although sometimes it?s reverted logic, might need more thought)
> Sounds like an awful lot of repeated array calculations for no overly good reason
> (unless gcc happens to fully optimize it into oblivion automatically).

-ETOOMUCHBEER

Of course we need to swap orthogonal members individually,
thus this kind of code.
However maybe there?s still a more elegant way of doing this.

Andreas Mohr

2009-07-15 13:36:37

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi!

> > +static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
> > +{
>
> some documentation about this logic would be great.

Arve, can you help here?

> > + ? ? ? int pos[2][2];
> > + ? ? ? int f, a;
> > + ? ? ? int base = 2;
> > + ? ? ? int z = buf[1];
> > + ? ? ? int w = buf[0] >> 4;
> > + ? ? ? int finger = buf[0] & 7;
> > + ? ? ? int finger2_pressed;
> > +
> > + ? ? ? for (f = 0; f < 2; f++) {
> > + ? ? ? ? ? ? ? u32 flip_flag = SYNAPTICS_FLIP_X;
> > + ? ? ? ? ? ? ? for (a = 0; a < 2; a++) {
> > + ? ? ? ? ? ? ? ? ? ? ? int p = buf[base + 1];
> > + ? ? ? ? ? ? ? ? ? ? ? p |= (u16)(buf[base] & 0x1f) << 8;
> > + ? ? ? ? ? ? ? ? ? ? ? if (ts->flags & flip_flag)
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p = ts->max[a] - p;
> > + ? ? ? ? ? ? ? ? ? ? ? if (ts->flags & SYNAPTICS_SNAP_TO_INACTIVE_EDGE) {

> > +static irqreturn_t synaptics_ts_irq_handler(int irq, void *dev_id)
> > +{
> > + ? ? ? struct synaptics_ts_data *ts = dev_id;
> > +
> > + ? ? ? disable_irq(ts->client->irq);
>
> disable_irq_nosync or convert this to request_threaded_irq(...).
> Please see recent discussion on linux-input for MAX key switch
> controller.

Do you have a link? (I replaced it with disable_irq_nosync, if that is
enough...)

> > +static int synaptics_ts_probe(
> > + ? ? ? struct i2c_client *client, const struct i2c_device_id *id)
> > +{
>
> __devinit ?

Ok.

> > + ? ? ? if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>
> check for SMBUS? I have added linux-i2c as this driver has i2c bits,
> so not removing any code.

I guess this driver is only probed on mach-pxa... on machines that
have the neccessary hardware.

> > + ? ? ? ? ? ? ? printk(KERN_ERR "synaptics_ts_probe: need I2C_FUNC_I2C\n");
>
> dev_xxx/pr_xxx friends?

Fixed all occurences.

> > + ? ? ? ts->input_dev = input_allocate_device();
> > + ? ? ? if (ts->input_dev == NULL) {
> > + ? ? ? ? ? ? ? ret = -ENOMEM;
> > + ? ? ? ? ? ? ? pr_err("synaptics: Failed to allocate input device\n");
> > + ? ? ? ? ? ? ? goto err_input_dev_alloc_failed;
> > + ? ? ? }
> > + ? ? ? ts->input_dev->name = "synaptics-rmi-touchscreen";
>
> other parameters of input_dev, like vendor, bus etc.,

Ok, what are those for? I can probably invent some dummy values, but...

> > + ? ? ? set_bit(EV_SYN, ts->input_dev->evbit);
> > + ? ? ? set_bit(EV_KEY, ts->input_dev->evbit);
> > + ? ? ? set_bit(BTN_TOUCH, ts->input_dev->keybit);
> > + ? ? ? set_bit(BTN_2, ts->input_dev->keybit);
> > + ? ? ? set_bit(EV_ABS, ts->input_dev->evbit);
> > +
>
> __set_bit or input_set_capability please.

__set_bit is easier, done.

> > +static int synaptics_ts_remove(struct i2c_client *client)
> > +{
>
> __devexit

Applied.

> > +static int synaptics_ts_suspend(struct i2c_client *client, pm_message_t mesg)
> > +{
>
> #ifdef CONFIG_PM ?

Yep.

> > +MODULE_DESCRIPTION("Synaptics Touchscreen Driver");
> > +MODULE_LICENSE("GPL");
>
> MODULE_ALIAS?

Umm, why? This is loaded from board-*.c files, and I don't think i2c
has enumeration capabilities.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2009-07-15 13:49:08

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

On Tue 2009-07-14 10:52:12, Dmitry Torokhov wrote:
> On Tue, Jul 14, 2009 at 12:06:34PM +0200, Pavel Machek wrote:
> > From: Arve Hj?nnev?g <[email protected]>
> >
> > This adds support for synaptic touchscreen, used in HTC dream
> > cellphone.
> >
> > Signed-off-by: Pavel Machek <[email protected]>
>
> This is pretty large body of code, could we get a sign-off from Arve as
> well since he seems to be the author?

Arve?

> > +config TOUCHSCREEN_SYNAPTICS_I2C_RMI
> > + tristate "Synaptics i2c touchscreen"
> > + depends on I2C
> > + help
> > + This enables support for Synaptics RMI over I2C based touchscreens.
> > + Such touchscreen is used in HTC Dream.
> > +
>
> To compile this driver as a module...

Done.

> > +static struct workqueue_struct *synaptics_wq;
>
> Do we need a separate workqueue? Is reading the device that slow that we
> can use keventd?

Arve?

> > + int snap_up_on[2];
> > + int snap_up_off[2];
> > + int snap_down[2];
> > + int snap_up[2];
> > + u32 flags;
> > + int (*power)(int on);
>
> 'bool on'?

Ok.

> > +static int i2c_read(struct synaptics_ts_data *ts, u8 reg, char *msg)
> > +{
> > + int ret = i2c_smbus_read_byte_data(ts->client, 0xf0);
>
> We are not using 'reg'?

Uff... I thought I actually tested it?!

> > + input_report_abs(ts->input_dev, ABS_PRESSURE, z);
> > + input_report_abs(ts->input_dev, ABS_TOOL_WIDTH, w);
> > + input_report_key(ts->input_dev, BTN_TOUCH, finger);
> > + finger2_pressed = finger > 1 && finger != 7;
> > + input_report_key(ts->input_dev, BTN_2, finger2_pressed);
> > + if (finger2_pressed) {
> > + input_report_abs(ts->input_dev, ABS_HAT0X, pos[1][0]);
> > + input_report_abs(ts->input_dev, ABS_HAT0Y, pos[1][1]);
>
> ABS_HAT is pretty unusual for a touchscreen...

It is trying to do something useful for multitouch. People actually
use multitouch on HTC dream. I guess I can strip it from version being
merged...

> > + if ((buf[14] & 0xc0) != 0x40) {
> > + printk(KERN_WARNING "synaptics_ts_work_func:"
> > + " bad read %x %x %x %x %x %x %x %x %x"
> > + " %x %x %x %x %x %x, ret %d\n",
> > + buf[0], buf[1], buf[2], buf[3],
> > + buf[4], buf[5], buf[6], buf[7],
> > + buf[8], buf[9], buf[10], buf[11],
> > + buf[12], buf[13], buf[14], ret);
>
> Umm... for ()?

> > +static enum hrtimer_restart synaptics_ts_timer_func(struct hrtimer *timer)
> > +{
> > + struct synaptics_ts_data *ts =
> > + container_of(timer, struct synaptics_ts_data, timer);
> > +
> > + queue_work(synaptics_wq, &ts->work);
> > +
> > + hrtimer_start(&ts->timer, ktime_set(0, 12500000), HRTIMER_MODE_REL);
>
> I was always wondering why use high-resolution timer and then offload to
> a workqueue which scheduling we have no idea about. Why can't we simply
> reschedule work in case when we are in polling mode?

> > + INIT_WORK(&ts->work, synaptics_ts_work_func);
> > + ts->client = client;
> > + i2c_set_clientdata(client, ts);
> > + pdata = client->dev.platform_data;
> > + if (pdata)
> > + ts->power = pdata->power;
> > + else
> > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>
> Where is it freed?

It is not, but this should not trigger. Converted to static fake_pdata.

> > + ts->input_dev->name = "synaptics-rmi-touchscreen";
>
> BUS_I2C, etc.

I figured out BUS_I2C. Anything else I need to set?

> > + set_bit(EV_SYN, ts->input_dev->evbit);
>
> __set_bit(), no need to lock the bus.

Ok.

> > + set_bit(EV_KEY, ts->input_dev->evbit);
> > + set_bit(BTN_TOUCH, ts->input_dev->keybit);
> > + set_bit(BTN_2, ts->input_dev->keybit);
>
> BTN_2 is kinda generic... Normally joysticks use it...

Ok, what is the recommended protocol for multitouch? Should I strip it
out from merge version?

> > + if (client->irq) {
> > + ret = request_irq(client->irq, synaptics_ts_irq_handler,
> > + 0, client->name, ts);
>
> I think threaded IRQ will fit the bill and will take care of
> IRQ/workqueue shutdown races. Of course you still need to use workqueue
> if polling.

I guess we'll just strip polling version for now?

> > + if (!ts->use_irq) {
> > + hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > + ts->timer.function = synaptics_ts_timer_func;
> > + hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
> > + }
> > +
> > + pr_info("synaptics: Start touchscreen %s in %s mode\n",
>
> Probably "synaptics_ts" throughout so nobody is confused?

Well, there's no other hardware from synaptics in those machines :-).

> > + ts->input_dev->name, ts->use_irq ? "interrupt" : "polling");
> > +
> > + return 0;
> > +
> > + err_input_register_device_failed:
> > + input_free_device(ts->input_dev);
> > +
>
> Don't see canceling timer nor shutting off WQ here. Also, maybe
> implement open() and close() so we don't reschedule WQ while polling?

> > +{
> > + struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> > +
> > + if (ts->use_irq)
> > + free_irq(client->irq, ts);
> > + else
> > + hrtimer_cancel(&ts->timer);
>
> What about the work?

> > +static void __exit synaptics_ts_exit(void)
> > +{
> > + i2c_del_driver(&synaptics_ts_driver);
> > + if (synaptics_wq)
> > + destroy_workqueue(synaptics_wq);
> > +}
> > +
> > +module_init(synaptics_ts_init);
> > +module_exit(synaptics_ts_exit);
> > +
> > +MODULE_DESCRIPTION("Synaptics Touchscreen Driver");
> > +MODULE_LICENSE("GPL");
>
> MODULE_AUTHOR()?

Ok.

> Thanks.

Thanks for review!
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2009-07-15 13:51:50

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

2009-07-15 15:27:10

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi Pavel,

On Wed, Jul 15, 2009 at 03:48:55PM +0200, Pavel Machek wrote:
> On Tue 2009-07-14 10:52:12, Dmitry Torokhov wrote:
> > On Tue, Jul 14, 2009 at 12:06:34PM +0200, Pavel Machek wrote:
> > > + input_report_abs(ts->input_dev, ABS_PRESSURE, z);
> > > + input_report_abs(ts->input_dev, ABS_TOOL_WIDTH, w);
> > > + input_report_key(ts->input_dev, BTN_TOUCH, finger);
> > > + finger2_pressed = finger > 1 && finger != 7;
> > > + input_report_key(ts->input_dev, BTN_2, finger2_pressed);
> > > + if (finger2_pressed) {
> > > + input_report_abs(ts->input_dev, ABS_HAT0X, pos[1][0]);
> > > + input_report_abs(ts->input_dev, ABS_HAT0Y, pos[1][1]);
> >
> > ABS_HAT is pretty unusual for a touchscreen...
>
> It is trying to do something useful for multitouch. People actually
> use multitouch on HTC dream. I guess I can strip it from version being
> merged...
>

What about converting it to emit ABS_MT_* events added specifically
for the multitouch protocol?

>
> > > + ts->input_dev->name = "synaptics-rmi-touchscreen";
> >
> > BUS_I2C, etc.
>
> I figured out BUS_I2C. Anything else I need to set?
>

No, just set what you know. It is purely to help userspace identify the
device.

>
> > > + if (client->irq) {
> > > + ret = request_irq(client->irq, synaptics_ts_irq_handler,
> > > + 0, client->name, ts);
> >
> > I think threaded IRQ will fit the bill and will take care of
> > IRQ/workqueue shutdown races. Of course you still need to use workqueue
> > if polling.
>
> I guess we'll just strip polling version for now?
>

Well, is it useable in polling mode? Do we forsee any devices using it
that way? If no then shure, strip it out.

--
Dmitry

2009-07-15 15:33:44

by Brian Swetland

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

On Wed, Jul 15, 2009 at 8:27 AM, Dmitry
Torokhov<[email protected]> wrote:
> On Wed, Jul 15, 2009 at 03:48:55PM +0200, Pavel Machek wrote:
>> On Tue 2009-07-14 10:52:12, Dmitry Torokhov wrote:
>> >
>> > ABS_HAT is pretty unusual for a touchscreen...
>>
>> It is trying to do something useful for multitouch. People actually
>> use multitouch on HTC dream. I guess I can strip it from version being
>> merged...
>
> What about converting it to emit ABS_MT_* events added specifically
> for the multitouch protocol?

I believe at the time the driver was written, those events did not
exist. If the input framework has more appropriate events for the
"2nd finger" (which is how synaptics represents MT on this panel), we
should definitely switch to them.

Brian

2009-07-15 17:26:45

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi Brian,

On Wed, Jul 15, 2009 at 9:03 PM, Brian Swetland<[email protected]> wrote:
> On Wed, Jul 15, 2009 at 8:27 AM, Dmitry
> Torokhov<[email protected]> wrote:
>> On Wed, Jul 15, 2009 at 03:48:55PM +0200, Pavel Machek wrote:
>>> On Tue 2009-07-14 10:52:12, Dmitry Torokhov wrote:
>>> >
>>> > ABS_HAT is pretty unusual for a touchscreen...
>>>
>>> It is trying to do something useful for multitouch. People actually
>>> use multitouch on HTC dream. I guess I can strip it from version being
>>> merged...
>>
>> What about converting it to emit ABS_MT_* events added specifically
>> for the multitouch protocol?
>
> I believe at the time the driver was written, those events did not
> exist. ?If the input framework has more appropriate events for the
> "2nd finger" (which is how synaptics represents MT on this panel), we
> should definitely switch to them.
>

Yes, MT events were added recently and they are working. Please check
this demo based on MT events.

http://www.linuxfordevices.com/c/a/News/Linux-multitouch-technology-demod/

Also, please confirm if polling mode is really used in some h/w
configuration, if not as said by Dmitry, we can remove it and do clean
integration with threaded irq.

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-15 17:33:07

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi Pavel,

On Wed, Jul 15, 2009 at 7:06 PM, Pavel Machek<[email protected]> wrote:
> Hi!
>
>> > +static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
>> > +{
>>
>> some documentation about this logic would be great.
>
> Arve, can you help here?
>
>
>> > + ? ? ? int pos[2][2];
>> > + ? ? ? int f, a;
>> > + ? ? ? int base = 2;
>> > + ? ? ? int z = buf[1];
>> > + ? ? ? int w = buf[0] >> 4;
>> > + ? ? ? int finger = buf[0] & 7;
>> > + ? ? ? int finger2_pressed;
>> > +
>> > + ? ? ? for (f = 0; f < 2; f++) {
>> > + ? ? ? ? ? ? ? u32 flip_flag = SYNAPTICS_FLIP_X;
>> > + ? ? ? ? ? ? ? for (a = 0; a < 2; a++) {
>> > + ? ? ? ? ? ? ? ? ? ? ? int p = buf[base + 1];
>> > + ? ? ? ? ? ? ? ? ? ? ? p |= (u16)(buf[base] & 0x1f) << 8;
>> > + ? ? ? ? ? ? ? ? ? ? ? if (ts->flags & flip_flag)
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p = ts->max[a] - p;
>> > + ? ? ? ? ? ? ? ? ? ? ? if (ts->flags & SYNAPTICS_SNAP_TO_INACTIVE_EDGE) {
>
>
>> > +static irqreturn_t synaptics_ts_irq_handler(int irq, void *dev_id)
>> > +{
>> > + ? ? ? struct synaptics_ts_data *ts = dev_id;
>> > +
>> > + ? ? ? disable_irq(ts->client->irq);
>>
>> disable_irq_nosync or convert this to request_threaded_irq(...).
>> Please see recent discussion on linux-input for MAX key switch
>> controller.
>
> Do you have a link? (I replaced it with disable_irq_nosync, if that is
> enough...)
>

link: http://patchwork.kernel.org/patch/35515/

>> > +static int synaptics_ts_probe(
>> > + ? ? ? struct i2c_client *client, const struct i2c_device_id *id)
>> > +{
>>
>> __devinit ?
>
> Ok.
>
>> > + ? ? ? if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>>
>> check for SMBUS? I have added linux-i2c as this driver has i2c bits,
>> so not removing any code.
>
> I guess this driver is only probed on mach-pxa... on machines that
> have the neccessary hardware.

Because this driver is using smbus i2c apis, it will be good to add
that check too.

>
>> > + ? ? ? ? ? ? ? printk(KERN_ERR "synaptics_ts_probe: need I2C_FUNC_I2C\n");
>>
>> dev_xxx/pr_xxx friends?
>
> Fixed all occurences.
>
>
>> > + ? ? ? ts->input_dev = input_allocate_device();
>> > + ? ? ? if (ts->input_dev == NULL) {
>> > + ? ? ? ? ? ? ? ret = -ENOMEM;
>> > + ? ? ? ? ? ? ? pr_err("synaptics: Failed to allocate input device\n");
>> > + ? ? ? ? ? ? ? goto err_input_dev_alloc_failed;
>> > + ? ? ? }
>> > + ? ? ? ts->input_dev->name = "synaptics-rmi-touchscreen";
>>
>> other parameters of input_dev, like vendor, bus etc.,
>
> Ok, what are those for? I can probably invent some dummy values, but...

May be vendor, product, version would be good to add.

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-15 18:43:44

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

On Wed, Jul 15, 2009 at 8:27 AM, Dmitry
Torokhov<[email protected]> wrote:
...
>> > I think threaded IRQ will fit the bill and will take care of
>> > IRQ/workqueue shutdown races. Of course you still need to use workqueue
>> > if polling.
>>
>> I guess we'll just strip polling version for now?
>>
>
> Well, is it useable in polling mode? Do we forsee any devices using it
> that way? If no then shure, strip it out.

I would not consider polling mode acceptable in a shipping device. It
was somewhat useful during bringup since it allowed testing the
touchscreen without a working interrupt.

--
Arve Hj?nnev?g

2009-07-15 20:24:51

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

On Wed, Jul 15, 2009 at 6:48 AM, Pavel Machek<[email protected]> wrote:
> On Tue 2009-07-14 10:52:12, Dmitry Torokhov wrote:
>> On Tue, Jul 14, 2009 at 12:06:34PM +0200, Pavel Machek wrote:
>> > From: Arve Hj?nnev?g <[email protected]>
>> >
>> > This adds support for synaptic touchscreen, used in HTC dream
>> > cellphone.
>> >
>> > Signed-off-by: Pavel Machek <[email protected]>
>>
>> This is pretty large body of code, could we get a sign-off from Arve as
>> well since he seems to be the author?
>
> Arve?
>

The original patches has my sign-offs. Can you just keep that and add
a line describing your changes before your sign-off?

...
>> > +static struct workqueue_struct *synaptics_wq;
>>
>> Do we need a separate workqueue? Is reading the device that slow that we
>> can use keventd?
>

keventd has two problems. First, other work runs on it that can take
more than 12.5ms. This will case dropped events. Second, the i2c bus
sometimes locks up, which in turns would cause this driver to block
keventd for seconds.

...
>> > + ? if (client->irq) {
>> > + ? ? ? ? ? ret = request_irq(client->irq, synaptics_ts_irq_handler,
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0, client->name, ts);
>>
>> I think threaded IRQ will fit the bill and will take care of
>> IRQ/workqueue shutdown races. Of course you still need to use workqueue
>> if polling.
>
> I guess we'll just strip polling version for now?

Sounds good to me.

--
Arve Hj?nnev?g

2009-07-15 21:33:35

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

On Wed, Jul 15, 2009 at 6:36 AM, Pavel Machek<[email protected]> wrote:
> Hi!
>
>> > +static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
>> > +{
>>
>> some documentation about this logic would be great.
>
> Arve, can you help here?

This sensor sends two 6-byte absolute finger reports, an optional
2-byte relative report followed by a status byte
(http://www.synaptics.com/sites/default/files/511_000099_01F.pdf).
This function reads the two finger reports and transforms the
coordinates according the platform data so they can be aligned with
the lcd behind the touchscreen. typically we flip the y-axis since the
sensor uses the bottom left corner as the origin, but if the sensor is
mounted upside down the platform data will request that the x-axis
should be flipped instead. The snap to inactive edge border are used
to allow tapping the edges of the screen on the G1. The active area of
the touchscreen is smaller than the lcd. When the finger gets close
the edge of the screen we snap it to the edge. This allows ui elements
at the edge of the screen to be hit, and it prevents hitting ui
elements that are not at the edge of the screen when the finger is
touching the edge.

--
Arve Hj?nnev?g

2009-07-21 10:22:03

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi!

> >> disable_irq_nosync or convert this to request_threaded_irq(...).
> >> Please see recent discussion on linux-input for MAX key switch
> >> controller.
> >
> > Do you have a link? (I replaced it with disable_irq_nosync, if that is
> > enough...)
> >
>
> link: http://patchwork.kernel.org/patch/35515/

Thanks!

> >> > + ? ? ? if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> >>
> >> check for SMBUS? I have added linux-i2c as this driver has i2c bits,
> >> so not removing any code.
> >
> > I guess this driver is only probed on mach-pxa... on machines that
> > have the neccessary hardware.
>
> Because this driver is using smbus i2c apis, it will be good to add
> that check too.

So I should do something like

if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
...

in addition?
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2009-07-21 10:34:46

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi Pavel,

On Tue, Jul 21, 2009 at 3:51 PM, Pavel Machek<[email protected]> wrote:
> Hi!
>
>> >> disable_irq_nosync or convert this to request_threaded_irq(...).
>> >> Please see recent discussion on linux-input for MAX key switch
>> >> controller.
>> >
>> > Do you have a link? (I replaced it with disable_irq_nosync, if that is
>> > enough...)
>> >
>>
>> link: http://patchwork.kernel.org/patch/35515/
>
> Thanks!
>
>> >> > + ? ? ? if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> >>
>> >> check for SMBUS? I have added linux-i2c as this driver has i2c bits,
>> >> so not removing any code.
>> >
>> > I guess this driver is only probed on mach-pxa... on machines that
>> > have the neccessary hardware.
>>
>> Because this driver is using smbus i2c apis, it will be good to add
>> that check too.
>
> So I should do something like
>
> if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> ? ? ? ?...
>
> in addition?

Yes.

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-21 10:41:15

[permalink] [raw]

Subject: Re: Support for synaptic touchscreen in HTC dream

Hi!

> >> > +static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
> >> > +{
> >>
> >> some documentation about this logic would be great.
> >
> > Arve, can you help here?
>
> This sensor sends two 6-byte absolute finger reports, an optional
> 2-byte relative report followed by a status byte
> (http://www.synaptics.com/sites/default/files/511_000099_01F.pdf).
> This function reads the two finger reports and transforms the
> coordinates according the platform data so they can be aligned with
> the lcd behind the touchscreen. typically we flip the y-axis since the
> sensor uses the bottom left corner as the origin, but if the sensor is
> mounted upside down the platform data will request that the x-axis
> should be flipped instead. The snap to inactive edge border are used
> to allow tapping the edges of the screen on the G1. The active area of
> the touchscreen is smaller than the lcd. When the finger gets close
> the edge of the screen we snap it to the edge. This allows ui elements
> at the edge of the screen to be hit, and it prevents hitting ui
> elements that are not at the edge of the screen when the finger is
> touching the edge.

Thanks. I added it like this:

diff --git a/drivers/input/touchscreen/synaptics_i2c_rmi.c b/drivers/input/touchscreen/synaptics_i2c_rmi.c
index 771b710..3ff62b1 100644
--- a/drivers/input/touchscreen/synaptics_i2c_rmi.c
+++ b/drivers/input/touchscreen/synaptics_i2c_rmi.c
@@ -13,6 +13,7 @@
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
+ * http://www.synaptics.com/sites/default/files/511_000099_01F.pdf
*/

#include <linux/module.h>
@@ -87,6 +87,22 @@ static int synaptics_init_panel(struct synaptics_ts_data *ts)

static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
{
+/*
+ * This sensor sends two 6-byte absolute finger reports, an optional
+ * 2-byte relative report followed by a status byte. This function
+ * reads the two finger reports and transforms the coordinates
+ * according the platform data so they can be aligned with the lcd
+ * behind the touchscreen. Typically we flip the y-axis since the
+ * sensor uses the bottom left corner as the origin, but if the sensor
+ * is mounted upside down the platform data will request that the
+ * x-axis should be flipped instead. The snap to inactive edge border
+ * are used to allow tapping the edges of the screen on the G1. The
+ * active area of the touchscreen is smaller than the lcd. When the
+ * finger gets close the edge of the screen we snap it to the
+ * edge. This allows ui elements at the edge of the screen to be hit,
+ * and it prevents hitting ui elements that are not at the edge of the
+ * screen when the finger is touching the edge.
+ */
int pos[2][2];
int f, a;
int base = 2;

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2009-07-21 10:59:38

[permalink] [raw]

Subject: Threaded interrupts for synaptic touchscreen in HTC dream

Hi!

> > Do you have a link? (I replaced it with disable_irq_nosync, if that is
> > enough...)
> >
>
> link: http://patchwork.kernel.org/patch/35515/

Thanks!

Here's my attempt at that conversion; unfortunately I'm stuck in
2.6.29 for msm stuff, so I was not even able to compile-test it :-(.
Pavel

diff --git a/drivers/input/touchscreen/synaptics_i2c_rmi.c b/drivers/input/touchscreen/synaptics_i2c_rmi.c
index 771b710..4816262 100644
--- a/drivers/input/touchscreen/synaptics_i2c_rmi.c
+++ b/drivers/input/touchscreen/synaptics_i2c_rmi.c
@@ -13,6 +13,7 @@
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
+ * http://www.synaptics.com/sites/default/files/511_000099_01F.pdf
*/

#include <linux/module.h>
@@ -34,7 +35,6 @@ struct synaptics_ts_data {
u16 addr;
struct i2c_client *client;
struct input_dev *input_dev;
- int use_irq;
struct hrtimer timer;
struct work_struct work;
u16 max[2];
@@ -87,6 +87,22 @@ static int synaptics_init_panel(struct synaptics_ts_data *ts)

static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
{
+/*
+ * This sensor sends two 6-byte absolute finger reports, an optional
+ * 2-byte relative report followed by a status byte. This function
+ * reads the two finger reports and transforms the coordinates
+ * according the platform data so they can be aligned with the lcd
+ * behind the touchscreen. Typically we flip the y-axis since the
+ * sensor uses the bottom left corner as the origin, but if the sensor
+ * is mounted upside down the platform data will request that the
+ * x-axis should be flipped instead. The snap to inactive edge border
+ * are used to allow tapping the edges of the screen on the G1. The
+ * active area of the touchscreen is smaller than the lcd. When the
+ * finger gets close the edge of the screen we snap it to the
+ * edge. This allows ui elements at the edge of the screen to be hit,
+ * and it prevents hitting ui elements that are not at the edge of the
+ * screen when the finger is touching the edge.
+ */
int pos[2][2];
int f, a;
int base = 2;
@@ -144,7 +160,7 @@ static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
input_sync(ts->input_dev);
}

-static void synaptics_ts_work_func(struct work_struct *work)
+static void synaptics_ts_work(struct synaptics_ts_data *ts)
{
int i;
int ret;
@@ -152,8 +168,6 @@ static void synaptics_ts_work_func(struct work_struct *work)
struct i2c_msg msg[2];
u8 start_reg = 0;
u8 buf[15];
- struct synaptics_ts_data *ts =
- container_of(work, struct synaptics_ts_data, work);

msg[0].addr = ts->client->addr;
msg[0].flags = 0;
@@ -164,7 +178,7 @@ static void synaptics_ts_work_func(struct work_struct *work)
msg[1].len = sizeof(buf);
msg[1].buf = buf;

- for (i = 0; i < ((ts->use_irq && !bad_data) ? 1 : 10); i++) {
+ for (i = 0; i < (!bad_data ? 1 : 10); i++) {
ret = i2c_transfer(ts->client->adapter, msg, 2);
if (ret < 0) {
pr_err("ts_work: i2c_transfer failed\n");
@@ -190,27 +204,20 @@ static void synaptics_ts_work_func(struct work_struct *work)

decode_report(ts, buf);
}
- if (ts->use_irq)
- enable_irq(ts->client->irq);
+ enable_irq(ts->client->irq);
}

-static enum hrtimer_restart synaptics_ts_timer_func(struct hrtimer *timer)
-{
- struct synaptics_ts_data *ts =
- container_of(timer, struct synaptics_ts_data, timer);
-
- queue_work(synaptics_wq, &ts->work);

- hrtimer_start(&ts->timer, ktime_set(0, 12500000), HRTIMER_MODE_REL);
- return HRTIMER_NORESTART;
+static irqreturn_t synaptics_ts_hardirq(int irq, void *dev_id)
+{
+ disable_irq_nosync(irq);
+ return IRQ_WAKE_THREAD;
}

static irqreturn_t synaptics_ts_irq_handler(int irq, void *dev_id)
{
struct synaptics_ts_data *ts = dev_id;
-
- disable_irq_nosync(ts->client->irq);
- queue_work(synaptics_wq, &ts->work);
+ synaptics_ts_work(ts);
return IRQ_HANDLED;
}

@@ -469,24 +476,21 @@ static int __devinit synaptics_ts_probe(
ts->input_dev->name);
goto err_input_register_device_failed;
}
- if (client->irq) {
- ret = request_irq(client->irq, synaptics_ts_irq_handler,
- 0, client->name, ts);
- if (ret == 0) {
- ret = i2c_set(ts, 0xf1, 0x01, "enable abs int");
- if (ret)
- free_irq(client->irq, ts);
- }
- if (ret == 0)
- ts->use_irq = 1;
- else
- dev_err(&client->dev, "request_irq failed\n");
- }
- if (!ts->use_irq) {
- hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
- ts->timer.function = synaptics_ts_timer_func;
- hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
+
+ ret = request_threaded_irq(client->irq,
+ synatptics_ts_hardirq, synaptics_ts_irq_handler,
+ 0, client->name, ts);
+
+ if (ret)
+ pr_err("synaptics: could not register irq\n");
+
+
+ ret = i2c_set(ts, 0xf1, 0x01, "enable abs int");
+ if (ret) {
+ pr_err("synaptics: could not enable irq\n");
+ free_irq(client->irq, ts);
}
+
#ifdef CONFIG_HAS_EARLYSUSPEND
ts->early_suspend.level = EARLY_SUSPEND_LEVEL_BLANK_SCREEN + 1;
ts->early_suspend.suspend = synaptics_ts_early_suspend;
@@ -495,7 +499,7 @@ static int __devinit synaptics_ts_probe(
#endif

pr_info("synaptics: Start touchscreen %s in %s mode\n",
- ts->input_dev->name, ts->use_irq ? "interrupt" : "polling");
+ ts->input_dev->name, "interrupt");

return 0;

@@ -517,10 +521,7 @@ static int synaptics_ts_remove(struct i2c_client *client)
#ifdef CONFIG_HAS_EARLYSUSPEND
unregister_early_suspend(&ts->early_suspend);
#endif
- if (ts->use_irq)
- free_irq(client->irq, ts);
- else
- hrtimer_cancel(&ts->timer);
+ free_irq(client->irq, ts);
input_unregister_device(ts->input_dev);
kfree(ts);
return 0;
@@ -532,12 +533,9 @@ static int synaptics_ts_suspend(struct i2c_client *client, pm_message_t mesg)
int ret;
struct synaptics_ts_data *ts = i2c_get_clientdata(client);

- if (ts->use_irq)
- disable_irq(client->irq);
- else
- hrtimer_cancel(&ts->timer);
+ disable_irq(client->irq);
ret = cancel_work_sync(&ts->work);
- if (ret && ts->use_irq) /* if work was pending disable-count is now 2 */
+ if (ret) /* if work was pending disable-count is now 2 */
enable_irq(client->irq);
i2c_set(ts, 0xf1, 0, "disable interrupt");
i2c_set(ts, 0xf0, 0x86, "deep sleep");
@@ -563,11 +561,8 @@ static int synaptics_ts_resume(struct i2c_client *client)

synaptics_init_panel(ts);

- if (ts->use_irq) {
- enable_irq(client->irq);
- i2c_set(ts, 0xf1, 0x01, "enable abs int");
- } else
- hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
+ enable_irq(client->irq);
+ i2c_set(ts, 0xf1, 0x01, "enable abs int");

return 0;
}

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2009-07-21 11:37:22

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, Jul 21, 2009 at 12:59:25PM +0200, Pavel Machek wrote:

This looks like an unrelated (but useful) change:

> *
> + * http://www.synaptics.com/sites/default/files/511_000099_01F.pdf
> */

This too:

> static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
> {
> +/*
> + * This sensor sends two 6-byte absolute finger reports, an optional
> + * 2-byte relative report followed by a status byte. This function
> + * reads the two finger reports and transforms the coordinates

Worth splitting them out?

> +static irqreturn_t synaptics_ts_hardirq(int irq, void *dev_id)
> +{
> + disable_irq_nosync(irq);
> + return IRQ_WAKE_THREAD;

Are you sure that this is going to work? The IRQ thread code will not
call the thread function if the IRQ is marked as disabled so the thread
won't actually get called and the interrupt will just stay disabled (see
irq_thread() in kernel/irq/manage.c). As far as I can see the threaded
IRQ support can't be used for devices on interrupt driven buses that
can't interact with the hardware in hardirq context but I might be
missing something here.

2009-07-21 12:18:43

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

Hi Mark,

On Tue, Jul 21, 2009 at 5:06 PM, Mark
Brown<[email protected]> wrote:
> On Tue, Jul 21, 2009 at 12:59:25PM +0200, Pavel Machek wrote:
>
> This looks like an unrelated (but useful) change:
>
>> ? *
>> + * http://www.synaptics.com/sites/default/files/511_000099_01F.pdf
>> ? */
>
> This too:
>
>> ?static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
>> ?{
>> +/*
>> + * This sensor sends two 6-byte absolute finger reports, an optional
>> + * 2-byte relative report followed by a status byte. This function
>> + * reads the two finger reports and transforms the coordinates
>
> Worth splitting them out?
>
>> +static irqreturn_t synaptics_ts_hardirq(int irq, void *dev_id)
>> +{
>> + ? ? disable_irq_nosync(irq);
>> + ? ? return IRQ_WAKE_THREAD;
>
> Are you sure that this is going to work? ?The IRQ thread code will not
> call the thread function if the IRQ is marked as disabled so the thread
> won't actually get called and the interrupt will just stay disabled (see
> irq_thread() in kernel/irq/manage.c). ?As far as I can see the threaded
> IRQ support can't be used for devices on interrupt driven buses that
> can't interact with the hardware in hardirq context but I might be
> missing something here.
>

I think threaded irqs are used in USB drivers too, I can't locate the
patches link for that.

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-21 12:30:12

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

Hi Mark,

On Tue, Jul 21, 2009 at 5:48 PM, Trilok Soni<[email protected]> wrote:
> Hi Mark,
>
> On Tue, Jul 21, 2009 at 5:06 PM, Mark
> Brown<[email protected]> wrote:
>> On Tue, Jul 21, 2009 at 12:59:25PM +0200, Pavel Machek wrote:
>>
>> This looks like an unrelated (but useful) change:
>>
>>> ? *
>>> + * http://www.synaptics.com/sites/default/files/511_000099_01F.pdf
>>> ? */
>>
>> This too:
>>
>>> ?static void decode_report(struct synaptics_ts_data *ts, u8 *buf)
>>> ?{
>>> +/*
>>> + * This sensor sends two 6-byte absolute finger reports, an optional
>>> + * 2-byte relative report followed by a status byte. This function
>>> + * reads the two finger reports and transforms the coordinates
>>
>> Worth splitting them out?
>>
>>> +static irqreturn_t synaptics_ts_hardirq(int irq, void *dev_id)
>>> +{
>>> + ? ? disable_irq_nosync(irq);
>>> + ? ? return IRQ_WAKE_THREAD;
>>
>> Are you sure that this is going to work? ?The IRQ thread code will not
>> call the thread function if the IRQ is marked as disabled so the thread
>> won't actually get called and the interrupt will just stay disabled (see
>> irq_thread() in kernel/irq/manage.c). ?As far as I can see the threaded
>> IRQ support can't be used for devices on interrupt driven buses that
>> can't interact with the hardware in hardirq context but I might be
>> missing something here.
>>
>
> I think threaded irqs are used in USB drivers too, I can't locate the
> patches link for that.
>

Hopefully, this thread can give all details about threaded irq discussion.

http://lkml.org/lkml/2009/2/27/255

--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

2009-07-21 12:30:45

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, Jul 21, 2009 at 05:48:40PM +0530, Trilok Soni wrote:

> I think threaded irqs are used in USB drivers too, I can't locate the
> patches link for that.

The only user of any kind I can see in -next is dm355evm_keys which
relies on the IRQ it uses being configured edge triggered so that it
doesn't need to disable the interrupt to silence the hard IRQ.

2009-07-21 12:49:37

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, Jul 21, 2009 at 06:00:07PM +0530, Trilok Soni wrote:

> Hopefully, this thread can give all details about threaded irq discussion.

> http://lkml.org/lkml/2009/2/27/255

Yes, I'm aware of that - I read it at the time. It seemed to peter out
without any satisfactory solution, unfortunately. There's two separate
issues here:

- Ordinary devices on interrupt driven or slow buses like I2C. These
need something along the lines of request_threaded_irq() that's allows
them to schedule the main IRQ handler outside hardirq context so
that they can interact with the device. They need to do something in
hardirq context to disable the interrupt if it's level triggered but
most of the time the only option they've got is to disable the IRQ
and reenable it when the worker thread is done. This is the issue
here.

My immediate thought when I noticed this was that we should probably
fix request_threaded_irq() so that it's useful for them; I'd been
intending to do some digging and try to understand why it is
currently implemented as it is.

- Multi-function devices like the twl4030 which have an interrupt
controller on them and would like to expose that interrupt controller
via the generic IRQ subsystem. This was a large part of the
discussion in the thread above is a much trickier problem.

I've added the folks from Samsung posting the MELFAS MCS-5000 driver to
the thread since they're running into the same issue.

2009-07-21 16:04:48

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, Jul 21, 2009 at 01:49:33PM +0100, Mark Brown wrote:
> On Tue, Jul 21, 2009 at 06:00:07PM +0530, Trilok Soni wrote:
>
> > Hopefully, this thread can give all details about threaded irq discussion.
>
> > http://lkml.org/lkml/2009/2/27/255
>
> Yes, I'm aware of that - I read it at the time. It seemed to peter out
> without any satisfactory solution, unfortunately. There's two separate
> issues here:
>
> - Ordinary devices on interrupt driven or slow buses like I2C. These
> need something along the lines of request_threaded_irq() that's allows
> them to schedule the main IRQ handler outside hardirq context so
> that they can interact with the device. They need to do something in
> hardirq context to disable the interrupt if it's level triggered but
> most of the time the only option they've got is to disable the IRQ
> and reenable it when the worker thread is done. This is the issue
> here.
>
> My immediate thought when I noticed this was that we should probably
> fix request_threaded_irq() so that it's useful for them; I'd been
> intending to do some digging and try to understand why it is
> currently implemented as it is.
>
> - Multi-function devices like the twl4030 which have an interrupt
> controller on them and would like to expose that interrupt controller
> via the generic IRQ subsystem. This was a large part of the
> discussion in the thread above is a much trickier problem.
>
> I've added the folks from Samsung posting the MELFAS MCS-5000 driver to
> the thread since they're running into the same issue.
>

Let's also add Thomas and David since I believe they worked on the
feature.

>From my part I would like to have the threaded IRQ available to all
drivers since it seens to be hanlding driver shutdown cleanly and
without races which is a big plus for me since very few drivers get it
right.

--
Dmitry

2009-07-21 20:36:36

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, 21 Jul 2009, Dmitry Torokhov wrote:

> On Tue, Jul 21, 2009 at 01:49:33PM +0100, Mark Brown wrote:
> > On Tue, Jul 21, 2009 at 06:00:07PM +0530, Trilok Soni wrote:
> >
> > > Hopefully, this thread can give all details about threaded irq discussion.
> >
> > > http://lkml.org/lkml/2009/2/27/255
> >
> > Yes, I'm aware of that - I read it at the time. It seemed to peter out
> > without any satisfactory solution, unfortunately. There's two separate
> > issues here:
> >
> > - Ordinary devices on interrupt driven or slow buses like I2C. These
> > need something along the lines of request_threaded_irq() that's allows
> > them to schedule the main IRQ handler outside hardirq context so
> > that they can interact with the device. They need to do something in
> > hardirq context to disable the interrupt if it's level triggered but
> > most of the time the only option they've got is to disable the IRQ
> > and reenable it when the worker thread is done. This is the issue
> > here.

There is already a sane solution to the problem:

See http://lkml.org/lkml/2009/7/17/174

> > My immediate thought when I noticed this was that we should probably
> > fix request_threaded_irq() so that it's useful for them; I'd been
> > intending to do some digging and try to understand why it is
> > currently implemented as it is.

What's to fix there ?

> > - Multi-function devices like the twl4030 which have an interrupt
> > controller on them and would like to expose that interrupt controller
> > via the generic IRQ subsystem. This was a large part of the
> > discussion in the thread above is a much trickier problem.

Why ?

> > I've added the folks from Samsung posting the MELFAS MCS-5000 driver to
> > the thread since they're running into the same issue.
> >
>
> Let's also add Thomas and David since I believe they worked on the
> feature.
>
> >From my part I would like to have the threaded IRQ available to all
> drivers since it seens to be hanlding driver shutdown cleanly and
> without races which is a big plus for me since very few drivers get it
> right.

:)

Thanks,

tglx

2009-07-21 20:43:30

by Daniel Ribeiro

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

Em Ter, 2009-07-21 às 09:04 -0700, Dmitry Torokhov escreveu:
> On Tue, Jul 21, 2009 at 01:49:33PM +0100, Mark Brown wrote:

> > - Multi-function devices like the twl4030 which have an interrupt
> > controller on them and would like to expose that interrupt controller
> > via the generic IRQ subsystem. This was a large part of the
> > discussion in the thread above is a much trickier problem.
> >
> > I've added the folks from Samsung posting the MELFAS MCS-5000 driver to
> > the thread since they're running into the same issue.
> >
> Let's also add Thomas and David since I believe they worked on the
> feature.

Please, keep me on CC too, pcap has an interrupt controller, is
connected to SPI bus and exposes its interrupts via generic IRQ
subsystem.

--
Daniel Ribeiro

Attachments:

signature.asc (197.00 B)
Esta ? uma parte de mensagem assinada digitalmente

2009-07-21 20:58:56

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, Jul 21, 2009 at 10:30:26PM +0200, Thomas Gleixner wrote:
> On Tue, 21 Jul 2009, Dmitry Torokhov wrote:
>
> > On Tue, Jul 21, 2009 at 01:49:33PM +0100, Mark Brown wrote:
> > > On Tue, Jul 21, 2009 at 06:00:07PM +0530, Trilok Soni wrote:
> > >
> > > > Hopefully, this thread can give all details about threaded irq discussion.
> > >
> > > > http://lkml.org/lkml/2009/2/27/255
> > >
> > > Yes, I'm aware of that - I read it at the time. It seemed to peter out
> > > without any satisfactory solution, unfortunately. There's two separate
> > > issues here:
> > >
> > > - Ordinary devices on interrupt driven or slow buses like I2C. These
> > > need something along the lines of request_threaded_irq() that's allows
> > > them to schedule the main IRQ handler outside hardirq context so
> > > that they can interact with the device. They need to do something in
> > > hardirq context to disable the interrupt if it's level triggered but
> > > most of the time the only option they've got is to disable the IRQ
> > > and reenable it when the worker thread is done. This is the issue
> > > here.
>
> There is already a sane solution to the problem:
>
> See http://lkml.org/lkml/2009/7/17/174
>
> > > My immediate thought when I noticed this was that we should probably
> > > fix request_threaded_irq() so that it's useful for them; I'd been
> > > intending to do some digging and try to understand why it is
> > > currently implemented as it is.
>
> What's to fix there ?
>

duisable_irq_nosync() in the hard interrupt handler stops the thread
handler from running. Unfortunately there are devices where it is the
only thing we can do in the hard interrupt.

--
Dmitry

2009-07-21 21:49:51

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, 21 Jul 2009, Dmitry Torokhov wrote:

> On Tue, Jul 21, 2009 at 10:30:26PM +0200, Thomas Gleixner wrote:
> > On Tue, 21 Jul 2009, Dmitry Torokhov wrote:
> >
> > > On Tue, Jul 21, 2009 at 01:49:33PM +0100, Mark Brown wrote:
> > > > On Tue, Jul 21, 2009 at 06:00:07PM +0530, Trilok Soni wrote:
> > > >
> > > > > Hopefully, this thread can give all details about threaded irq discussion.
> > > >
> > > > > http://lkml.org/lkml/2009/2/27/255
> > > >
> > > > Yes, I'm aware of that - I read it at the time. It seemed to peter out
> > > > without any satisfactory solution, unfortunately. There's two separate
> > > > issues here:
> > > >
> > > > - Ordinary devices on interrupt driven or slow buses like I2C. These
> > > > need something along the lines of request_threaded_irq() that's allows
> > > > them to schedule the main IRQ handler outside hardirq context so
> > > > that they can interact with the device. They need to do something in
> > > > hardirq context to disable the interrupt if it's level triggered but
> > > > most of the time the only option they've got is to disable the IRQ
> > > > and reenable it when the worker thread is done. This is the issue
> > > > here.
> >
> > There is already a sane solution to the problem:
> >
> > See http://lkml.org/lkml/2009/7/17/174
> >
> > > > My immediate thought when I noticed this was that we should probably
> > > > fix request_threaded_irq() so that it's useful for them; I'd been
> > > > intending to do some digging and try to understand why it is
> > > > currently implemented as it is.
> >
> > What's to fix there ?
> >
>
> duisable_irq_nosync() in the hard interrupt handler stops the thread
> handler from running. Unfortunately there are devices where it is the
> only thing we can do in the hard interrupt.

Again:

There is already a sane solution to the problem:

See http://lkml.org/lkml/2009/7/17/174

Thanks,

tglx

2009-07-21 22:25:51

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, Jul 21, 2009 at 10:30:26PM +0200, Thomas Gleixner wrote:
> On Tue, 21 Jul 2009, Dmitry Torokhov wrote:
> > On Tue, Jul 21, 2009 at 01:49:33PM +0100, Mark Brown wrote:

> > > - Ordinary devices on interrupt driven or slow buses like I2C. These
> > > need something along the lines of request_threaded_irq() that's allows
> > > them to schedule the main IRQ handler outside hardirq context so
> > > that they can interact with the device. They need to do something in

> There is already a sane solution to the problem:

> See http://lkml.org/lkml/2009/7/17/174

I'll need to have a more detailed look at that but it's not immediately
clear to me how a driver (or even machine) should use that code - it
looks more like it's intended to be called from within the IRQ
infrastructure than from random driver code.

> > > My immediate thought when I noticed this was that we should probably
> > > fix request_threaded_irq() so that it's useful for them; I'd been
> > > intending to do some digging and try to understand why it is
> > > currently implemented as it is.

> What's to fix there ?

Nothing if the above works, though I guess more documentation wouldn't
hurt (and possibly a more friendly wrapper). From the name and
documentation request_threaded_irq() looks like it should be exactly
what's needed.

> > > - Multi-function devices like the twl4030 which have an interrupt
> > > controller on them and would like to expose that interrupt controller
> > > via the generic IRQ subsystem. This was a large part of the
> > > discussion in the thread above is a much trickier problem.

> Why ?

Partly just because it's idiomatic for the devices - these things are
from a software point of view essentially a small stack of devices glued
together and one of the devices on them is an interrupt controller so
the natural thing is to want to represent that interrupt controller in
the way Linux normally represents interrupt controllers and be able to
reuse all the core code rather than having to implement a clone of it.

The other part of it is that it gets you all the interfaces for
interrupts that the rest of the kernel expects which is needed when the
device interacts with others. The biggest issue here is that these
devices often have GPIOs on them (especially PMICs and audio CODECs).
These have all the facilities one expects of GPIOs, including being used
as interrupt sources. If we need to use chip-specific APIs to interact
with the interrupts they raise then the drivers for anything using them
need to know about those APIs and have special cases to work with them
which obviously doesn't scale.

> > >From my part I would like to have the threaded IRQ available to all
> > drivers since it seens to be hanlding driver shutdown cleanly and
> > without races which is a big plus for me since very few drivers get it
> > right.

> :)

It's also wasteful having to write the code in every single driver that
needs to handle interrupts on interrupt driven buses.

2009-07-22 10:45:37

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tue, 21 Jul 2009, Mark Brown wrote:

> On Tue, Jul 21, 2009 at 10:30:26PM +0200, Thomas Gleixner wrote:
> > On Tue, 21 Jul 2009, Dmitry Torokhov wrote:
> > > On Tue, Jul 21, 2009 at 01:49:33PM +0100, Mark Brown wrote:
>
> > > > - Ordinary devices on interrupt driven or slow buses like I2C. These
> > > > need something along the lines of request_threaded_irq() that's allows
> > > > them to schedule the main IRQ handler outside hardirq context so
> > > > that they can interact with the device. They need to do something in
>
> > There is already a sane solution to the problem:
>
> > See http://lkml.org/lkml/2009/7/17/174
>
> I'll need to have a more detailed look at that but it's not immediately
> clear to me how a driver (or even machine) should use that code - it
> looks more like it's intended to be called from within the IRQ
> infrastructure than from random driver code.

All it needs is to set handle_level_oneshot_irq for the interrupt line
of your I2C or whatever devices.

set_irq_handler(irq, handle_level_oneshot_irq);

The core code masks the interrupt in the hard irq path and runs the
primary handler. It's expected that the primary handler returns
IRQ_WAKE_THREAD, which will wake up the thread handler. Now the
handle_level_oneshot_irq() flow control does _NOT_ unmask the
interrupt line, so no interrupt storm can happen.

Now the thread handler runs handles its bus magic. When it returns to
the core code then desc->thread_eoi is called which unmasks the
interrupt line again.

> > > > My immediate thought when I noticed this was that we should probably
> > > > fix request_threaded_irq() so that it's useful for them; I'd been
> > > > intending to do some digging and try to understand why it is
> > > > currently implemented as it is.
>
> > What's to fix there ?
>
> Nothing if the above works, though I guess more documentation wouldn't
> hurt (and possibly a more friendly wrapper). From the name and

Wrapper for what ?

> documentation request_threaded_irq() looks like it should be exactly
> what's needed.
>
> > > > - Multi-function devices like the twl4030 which have an interrupt
> > > > controller on them and would like to expose that interrupt controller
> > > > via the generic IRQ subsystem. This was a large part of the
> > > > discussion in the thread above is a much trickier problem.
>
> > Why ?
>
> Partly just because it's idiomatic for the devices - these things are
> from a software point of view essentially a small stack of devices glued
> together and one of the devices on them is an interrupt controller so
> the natural thing is to want to represent that interrupt controller in
> the way Linux normally represents interrupt controllers and be able to
> reuse all the core code rather than having to implement a clone of it.

Sure.

> The other part of it is that it gets you all the interfaces for
> interrupts that the rest of the kernel expects which is needed when the
> device interacts with others. The biggest issue here is that these
> devices often have GPIOs on them (especially PMICs and audio CODECs).
> These have all the facilities one expects of GPIOs, including being used
> as interrupt sources. If we need to use chip-specific APIs to interact
> with the interrupts they raise then the drivers for anything using them
> need to know about those APIs and have special cases to work with them
> which obviously doesn't scale.

Ok, you have a main interrupt line which is triggered when any of the
interrupts in the device is raised. Now you cannot decide which of the
interrupt sources in the device triggered the interrupt because you
need to query the bus which can not be done in hard interrupt
context. Fine, use the oneshot handler for the main interrupt and do
the query in the irq thread.

The irq thread finds out which interrupt(s) are active in the
device. So it raises the interrupt handlers for those from the thread
which will wake up the relevant interrupt threads for those
devices. Once all the thread handlers have finished you return from
the main thread and the interrupt line gets unmasked again.

That's the easy part. Now some other details:

The driver which controls the interrupt device has to expose the
demultiplexed interrupts via its own irq_chip implementation. Of
course the chip functions like mask/ack/unmask cannot run in atomic
context as they require bus access again.

Here in deed we need to put some thought into common infrastructure
as it seems that such excellent hardware designs are becoming more
popular :(

The most interesting functions are request_irq, free_irq, enable_irq
and disable_irq. The main challange is to get the sychronization
straight. As Dmitry said the synchronization seems to be the most
common problem which driver writers get wrong. I can only agree with
that.

While writing this I looked into the code and came up with the
following completely untested patch.

The idea is to serialize the bus access for those operations in the
core code so that drivers which are behind that bus operated interrupt
controller do not have to worry about it and just can use the normal
interfaces. To achieve this we add two function pointers to the
irq_chip: bus_lock and bus_sync_unlock.

bus_lock() is called to serialize access to the interrupt controller
bus.

Now the core code can issue chip->mask/unmask ... commands without
changing the fast path code at all. The chip implementation merily
stores that information in a chip private data structure and
returns. No bus interaction as these functions are called from atomic
context.

After that bus_sync_unlock() is called outside the atomic context. Now
the chip implementation issues the bus commands, waits for completion
and unlocks the interrupt controller bus.

So for the interrupt controller this would look like:

struct irq_chip_data {
struct mutex mutex;
unsigned int irq_offset;
unsigned long mask;
unsigned long mask_status;
}

static void bus_lock(unsigned int irq)
{
struct irq_chip_data *data = get_irq_desc_chip_data(irq);

mutex_lock(&data->mutex);
}

static void mask(unsigned int irq)
{
struct irq_chip_data *data = get_irq_desc_chip_data(irq);

irq -= data->irq_offset;
data->mask |= (1 << irq);
}

static void unmask(unsigned int irq)
{
struct irq_chip_data *data = get_irq_desc_chip_data(irq);

irq -= data->irq_offset;
data->mask &= ~(1 << irq);
}

static void bus_sync_unlock(unsigned int irq)
{
struct irq_chip_data *data = get_irq_desc_chip_data(irq);

if (data->mask != data->mask_status) {
do_bus_magic_to_set_mask(data->mask);
data->mask_status = data->mask;
}
mutex_unlock(&data->mutex);
}

The device drivers can use request_threaded_irq, free_irq, disable_irq
and enable_irq as usual with the only restriction that the calls need
to come from non atomic context.

So in combination with the handle_onshot_level_irq patch this should
solve most of these problems.

Thanks,

tglx
----
Subject: genirq-sync-slow-bus-controlled-chips.patch
From: Thomas Gleixner <[email protected]>
Date: Wed, 22 Jul 2009 11:14:54 +0200

Signed-off-by: Thomas Gleixner <[email protected]>
---
include/linux/irq.h | 6 ++++++
kernel/irq/manage.c | 34 +++++++++++++++++++++++++++++++++-
2 files changed, 39 insertions(+), 1 deletion(-)

Index: linux-2.6-tip/include/linux/irq.h
===================================================================
--- linux-2.6-tip.orig/include/linux/irq.h
+++ linux-2.6-tip/include/linux/irq.h
@@ -100,6 +100,9 @@ struct msi_desc;
* @set_type: set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
* @set_wake: enable/disable power-management wake-on of an IRQ
*
+ * @bus_lock: function to lock access to slow bus (i2c) chips
+ * @bus_sync_unlock: function to sync and unlock slow bus (i2c) chips
+ *
* @release: release function solely used by UML
* @typename: obsoleted by name, kept as migration helper
*/
@@ -123,6 +126,9 @@ struct irq_chip {
int (*set_type)(unsigned int irq, unsigned int flow_type);
int (*set_wake)(unsigned int irq, unsigned int on);

+ void (*bus_lock)(unsigned int irq);
+ void (*bus_sync_unlock)(unsigned int irq);
+
/* Currently used only by UML, might disappear one day.*/
#ifdef CONFIG_IRQ_RELEASE_METHOD
void (*release)(unsigned int irq, void *dev_id);
Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/manage.c
+++ linux-2.6-tip/kernel/irq/manage.c
@@ -17,6 +17,22 @@

#include "internals.h"

+static inline void chip_bus_lock(unsigned int irq, struct irq_desc *desc)
+{
+ if (unlikely(desc->chip->bus_lock)) {
+ might_sleep();
+ desc->chip->bus_lock(irq);
+ }
+}
+
+static inline void chip_bus_sync_unlock(unsigned int irq, struct irq_desc *desc)
+{
+ if (unlikely(desc->chip->bus_sync_unlock)) {
+ might_sleep();
+ desc->chip->bus_sync_unlock(irq);
+ }
+}
+
/**
* synchronize_irq - wait for pending IRQ handlers (on other CPUs)
* @irq: interrupt number to wait for
@@ -222,9 +238,11 @@ void disable_irq_nosync(unsigned int irq
if (!desc)
return;

+ chip_bus_lock(irq, desc);
spin_lock_irqsave(&desc->lock, flags);
__disable_irq(desc, irq, false);
spin_unlock_irqrestore(&desc->lock, flags);
+ chip_bus_sync_unlock(irq, desc);
}
EXPORT_SYMBOL(disable_irq_nosync);

@@ -286,7 +304,8 @@ void __enable_irq(struct irq_desc *desc,
* matches the last disable, processing of interrupts on this
* IRQ line is re-enabled.
*
- * This function may be called from IRQ context.
+ * This function may be called from IRQ context only when
+ * desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
*/
void enable_irq(unsigned int irq)
{
@@ -296,9 +315,11 @@ void enable_irq(unsigned int irq)
if (!desc)
return;

+ chip_bus_lock(irq, desc);
spin_lock_irqsave(&desc->lock, flags);
__enable_irq(desc, irq, false);
spin_unlock_irqrestore(&desc->lock, flags);
+ chip_bus_sync_unlock(irq, desc);
}
EXPORT_SYMBOL(enable_irq);

@@ -831,7 +852,14 @@ EXPORT_SYMBOL_GPL(remove_irq);
*/
void free_irq(unsigned int irq, void *dev_id)
{
+ struct irq_desc *desc = irq_to_desc(irq);
+
+ if (!desc)
+ return;
+
+ chip_bus_sync_lock(irq, desc);
kfree(__free_irq(irq, dev_id));
+ chip_bus_sync_unlock(irq, desc);
}
EXPORT_SYMBOL(free_irq);

@@ -932,10 +960,14 @@ int request_threaded_irq(unsigned int ir
action->name = devname;
action->dev_id = dev_id;

+ chip_bus_lock(irq, desc);
+
retval = __setup_irq(irq, desc, action);
if (retval)
kfree(action);

+ chip_bus_sync_unlock(irq, desc);
+
#ifdef CONFIG_DEBUG_SHIRQ
if (irqflags & IRQF_SHARED) {
/*

2009-07-22 12:18:05

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 12:44:01PM +0200, Thomas Gleixner wrote:
> On Tue, 21 Jul 2009, Mark Brown wrote:

> > I'll need to have a more detailed look at that but it's not immediately
> > clear to me how a driver (or even machine) should use that code - it
> > looks more like it's intended to be called from within the IRQ
> > infrastructure than from random driver code.

> All it needs is to set handle_level_oneshot_irq for the interrupt line
> of your I2C or whatever devices.

> set_irq_handler(irq, handle_level_oneshot_irq);

Yeah, I know - the issue I was having was that the use of set_irq_handler()
seemed rather rude for driver code. Grepping around I see that there
are actually a small number of drivers using it already but at first
glance most of them appear to be implementing interrupt controllers. It
was setting off alarm bells about abstraction layer violation like
set_irq_type() does.

> > Nothing if the above works, though I guess more documentation wouldn't
> > hurt (and possibly a more friendly wrapper). From the name and

> Wrapper for what ?

Something to package up the set_irq_handler() and request_threaded_irq()
(possibly a flag for request_threaded_irq()). This is such a common
thing and request_threaded_irq() looks so much like it should Just Work
that I'd expect it'll help usability a lot to have a single function
which says "this is the idiomatic way to implement this".

> The irq thread finds out which interrupt(s) are active in the
> device. So it raises the interrupt handlers for those from the thread
> which will wake up the relevant interrupt threads for those
> devices. Once all the thread handlers have finished you return from
> the main thread and the interrupt line gets unmasked again.

Yes, this bit of it isn't too much of a problem, it's what's going on in
all the non-genirq infrastructures, but...

> The driver which controls the interrupt device has to expose the
> demultiplexed interrupts via its own irq_chip implementation. Of
> course the chip functions like mask/ack/unmask cannot run in atomic
> context as they require bus access again.

...as you say this is the tricky bit.

> Here in deed we need to put some thought into common infrastructure
> as it seems that such excellent hardware designs are becoming more
> popular :(

This isn't a new issue - it's more that you're seeing a greater degree
of mainline activity from embedded systems.

FWIW the hardware tradeoff here is in a large part down to the fact that
many of these devices are heavily size (and therefore pin count)
constrained, often on both the device and CPU end of things. Adding
more pins would either mean a bigger device or a device that's more
expensive to manufacture and use. This pushes to minimal wire, low
speed control interfaces and for the sort of low volume control these
things need there's not much operational problem there. These things
are *normally* either pushing events that either won't happen very often
(eg, RTC alarm expiry) or shouldn't happen at all (eg, power failures)
and so aren't performance critical.

> After that bus_sync_unlock() is called outside the atomic context. Now
> the chip implementation issues the bus commands, waits for completion
> and unlocks the interrupt controller bus.

I'll try to find time to implement some use of it and give it a spin -
it looks good at first glance but I'll need to convert one of my drivers
to genirq in order to test. Someone working on a chip that already uses
genirq might get there first.

2009-07-22 12:59:07

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

Mark,

On Wed, 22 Jul 2009, Mark Brown wrote:

> On Wed, Jul 22, 2009 at 12:44:01PM +0200, Thomas Gleixner wrote:
> > On Tue, 21 Jul 2009, Mark Brown wrote:
>
> > > I'll need to have a more detailed look at that but it's not immediately
> > > clear to me how a driver (or even machine) should use that code - it
> > > looks more like it's intended to be called from within the IRQ
> > > infrastructure than from random driver code.
>
> > All it needs is to set handle_level_oneshot_irq for the interrupt line
> > of your I2C or whatever devices.
>
> > set_irq_handler(irq, handle_level_oneshot_irq);
>
> Yeah, I know - the issue I was having was that the use of set_irq_handler()
> seemed rather rude for driver code. Grepping around I see that there
> are actually a small number of drivers using it already but at first
> glance most of them appear to be implementing interrupt controllers. It
> was setting off alarm bells about abstraction layer violation like
> set_irq_type() does.

I don't think it belongs into the driver code. It belongs into the
platform code which sets up the system and knows what's on which
interrupt line.

> > > Nothing if the above works, though I guess more documentation wouldn't
> > > hurt (and possibly a more friendly wrapper). From the name and
>
> > Wrapper for what ?
>
> Something to package up the set_irq_handler() and request_threaded_irq()
> (possibly a flag for request_threaded_irq()). This is such a common
> thing and request_threaded_irq() looks so much like it should Just Work
> that I'd expect it'll help usability a lot to have a single function
> which says "this is the idiomatic way to implement this".

Yeah, we might do with a flag, but I'd prefer that the platform code
aka arch/xxx/yourmachine/setup.c does this. That's the canonical place.

> > After that bus_sync_unlock() is called outside the atomic context. Now
> > the chip implementation issues the bus commands, waits for completion
> > and unlocks the interrupt controller bus.
>
> I'll try to find time to implement some use of it and give it a spin -
> it looks good at first glance but I'll need to convert one of my drivers
> to genirq in order to test. Someone working on a chip that already uses
> genirq might get there first.

That'd be great to get some feedback. Both patches need some more
thought, but I think they are a good start to do some testing. One
thing which I definitely want to change is the thread_eoi function. We
probably want to have it customizable for the following reason:

main interrupt
hardirq handler wakes main thread handler

main thread handler
bus magic
subdevice1 "hardirq" handler wakes subdevice1 irq thread
subdevice2 "hardirq" handler wakes subdevice2 irq thread
main thread handler waits for subdevice1/2 handlers to complete

subdevice1 thread handler
bus magic
....
thread_fn returns
signal main thread handler via completion

subdevice2 thread handler
bus magic
....
thread_fn returns
signal main thread handler via completion

main thread handler resumes
bus magic
main thread handler returns from thread_fn
unmask main interrupt

So the thread_eoi function is useful for both the main and the
subdevice handlers. The main handler probably just issues the unmask
while the subdevice handlers probably want to call a completion to
notify the main thread handler that they are done. That would be a
fairly proper design I think and would encourage driver writers to
follow the common scheme instead of doing their own black magic.

Thinking further we should even provide some infrastructure for that
in the common code which would handle the completion and attach it to
the interrupts in question, so the driver authors would not have to
deal with that at all. They just would return from thread_fn and let
the generic code deal with the notification. The notification has to
be set up by the interrupt controller code. That way you can use the
same device driver code w/o knowledge of the interrupt controller
implementation it is attached to.

Thoughts ?

Thanks,

tglx

2009-07-22 13:29:45

by Peter Zijlstra

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, 2009-07-22 at 14:58 +0200, Thomas Gleixner wrote:
> One
> thing which I definitely want to change is the thread_eoi function. We
> probably want to have it customizable for the following reason:
>
> main interrupt
> hardirq handler wakes main thread handler
>
> main thread handler
> bus magic
> subdevice1 "hardirq" handler wakes subdevice1 irq thread
> subdevice2 "hardirq" handler wakes subdevice2 irq thread
> main thread handler waits for subdevice1/2 handlers to complete
>
> subdevice1 thread handler
> bus magic
> ....
> thread_fn returns
> signal main thread handler via completion
>
> subdevice2 thread handler
> bus magic
> ....
> thread_fn returns
> signal main thread handler via completion
>
> main thread handler resumes
> bus magic
> main thread handler returns from thread_fn
> unmask main interrupt
>
> So the thread_eoi function is useful for both the main and the
> subdevice handlers. The main handler probably just issues the unmask
> while the subdevice handlers probably want to call a completion to
> notify the main thread handler that they are done. That would be a
> fairly proper design I think and would encourage driver writers to
> follow the common scheme instead of doing their own black magic.
>
> Thinking further we should even provide some infrastructure for that
> in the common code which would handle the completion and attach it to
> the interrupts in question, so the driver authors would not have to
> deal with that at all. They just would return from thread_fn and let
> the generic code deal with the notification. The notification has to
> be set up by the interrupt controller code. That way you can use the
> same device driver code w/o knowledge of the interrupt controller
> implementation it is attached to.

Wouldn't it be better if we could express the nesting property from
within genirq, so that we can do things like:

register_chip_nested(parent_chip, parent_irq, slave_chip);

And let genirq set-up the needed magic to make the nesting work.

Also, how important is it that subhandler1..n run in their own thread?
That is, can't we let them run from the thread that is otherwise waiting
for the completino anyway?

2009-07-22 13:31:35

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 02:58:24PM +0200, Thomas Gleixner wrote:
> On Wed, 22 Jul 2009, Mark Brown wrote:
> > On Wed, Jul 22, 2009 at 12:44:01PM +0200, Thomas Gleixner wrote:

> > > set_irq_handler(irq, handle_level_oneshot_irq);

> > Yeah, I know - the issue I was having was that the use of set_irq_handler()
> > seemed rather rude for driver code. Grepping around I see that there

> I don't think it belongs into the driver code. It belongs into the
> platform code which sets up the system and knows what's on which
> interrupt line.

Yeah, that is doable - but it'd be good for usability if it could be
handled transparently by the driver. Even in board code it's fairly
rare to need to explicitly set the handler if you're not actually
implementing an interrupt controller. Most of the board-specific code
doing this is doing so for a CPLD, FPGA or similar on the board rather
than configuring a preexisting IRQ for use by a leaf device.

Half the trouble is that if you're not implementing interrupt controller
code then calling set_irq_*() is normally a sign that you're doing
something wrong (for example, for set_irq_type() you should be using
flags on request_irq()).

> Thinking further we should even provide some infrastructure for that
> in the common code which would handle the completion and attach it to
> the interrupts in question, so the driver authors would not have to
> deal with that at all. They just would return from thread_fn and let
> the generic code deal with the notification. The notification has to
> be set up by the interrupt controller code. That way you can use the
> same device driver code w/o knowledge of the interrupt controller
> implementation it is attached to.

Yes, I think that'll be required for users like gpiolib. If someone's
done something like have a button implemented by attaching switch to a
GPIO input on for something like jack detect on an audio CODEC or a wake
source for a PMIC then we've got generic code that expects to just take
a gpio/irq and interact with it.

2009-07-22 14:19:44

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, 22 Jul 2009, Peter Zijlstra wrote:
> On Wed, 2009-07-22 at 14:58 +0200, Thomas Gleixner wrote:
> > Thinking further we should even provide some infrastructure for that
> > in the common code which would handle the completion and attach it to
> > the interrupts in question, so the driver authors would not have to
> > deal with that at all. They just would return from thread_fn and let
> > the generic code deal with the notification. The notification has to
> > be set up by the interrupt controller code. That way you can use the
> > same device driver code w/o knowledge of the interrupt controller
> > implementation it is attached to.
>
> Wouldn't it be better if we could express the nesting property from
> within genirq, so that we can do things like:
>
> register_chip_nested(parent_chip, parent_irq, slave_chip);
>
> And let genirq set-up the needed magic to make the nesting work.

Good point.

> Also, how important is it that subhandler1..n run in their own thread?
> That is, can't we let them run from the thread that is otherwise waiting
> for the completino anyway?

In those cases I suspect we can do that. I guess there can be async
handling as well: the main handler queries the pending interrupts,
masks them, wakes the handlers and returns. No wait for all threads to
finish necessary before unmasking the main interrupt line.

Thanks,

tglx

2009-07-22 14:32:15

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 04:18:26PM +0200, Thomas Gleixner wrote:
> On Wed, 22 Jul 2009, Peter Zijlstra wrote:

> > Also, how important is it that subhandler1..n run in their own thread?
> > That is, can't we let them run from the thread that is otherwise waiting
> > for the completino anyway?

> In those cases I suspect we can do that. I guess there can be async
> handling as well: the main handler queries the pending interrupts,
> masks them, wakes the handlers and returns. No wait for all threads to
> finish necessary before unmasking the main interrupt line.

The chips I'm deling with can certainly support doing that but I'm not
sure there'd be enormous advantages - a lot of the interrupt handlers
will either be trivial (eg, RTC ticks or alarms) or be serialised by a
need to interact with the chip anyway. I'd expect this to be generally
true, though ICBW.

2009-07-22 14:53:10

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, 22 Jul 2009, Mark Brown wrote:

> On Wed, Jul 22, 2009 at 04:18:26PM +0200, Thomas Gleixner wrote:
> > On Wed, 22 Jul 2009, Peter Zijlstra wrote:
>
> > > Also, how important is it that subhandler1..n run in their own thread?
> > > That is, can't we let them run from the thread that is otherwise waiting
> > > for the completino anyway?
>
> > In those cases I suspect we can do that. I guess there can be async
> > handling as well: the main handler queries the pending interrupts,
> > masks them, wakes the handlers and returns. No wait for all threads to
> > finish necessary before unmasking the main interrupt line.
>
> The chips I'm deling with can certainly support doing that but I'm not
> sure there'd be enormous advantages - a lot of the interrupt handlers
> will either be trivial (eg, RTC ticks or alarms) or be serialised by a
> need to interact with the chip anyway. I'd expect this to be generally
> true, though ICBW.

So in your case it would be possible to run the various subdevice
thread_fn handlers from your main interrupt thread one after each
other ?

Well, that indeed makes it a candidate for a nested structure
handling, where your main thread calls handle_nested_irq(irq) which
simply runs the thread function of that nested interrupt while keeping
the semantics vs. disable/enable ... intact.

Thanks,

tglx

2009-07-22 14:56:58

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 04:52:21PM +0200, Thomas Gleixner wrote:
> On Wed, 22 Jul 2009, Mark Brown wrote:

> > The chips I'm deling with can certainly support doing that but I'm not
> > sure there'd be enormous advantages - a lot of the interrupt handlers
> > will either be trivial (eg, RTC ticks or alarms) or be serialised by a
> > need to interact with the chip anyway. I'd expect this to be generally
> > true, though ICBW.

> So in your case it would be possible to run the various subdevice
> thread_fn handlers from your main interrupt thread one after each
> other ?

That's what they're all doing at present outside of genirq.

2009-07-22 15:17:21

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, 22 Jul 2009, Mark Brown wrote:

> On Wed, Jul 22, 2009 at 04:52:21PM +0200, Thomas Gleixner wrote:
> > On Wed, 22 Jul 2009, Mark Brown wrote:
>
> > > The chips I'm deling with can certainly support doing that but I'm not
> > > sure there'd be enormous advantages - a lot of the interrupt handlers
> > > will either be trivial (eg, RTC ticks or alarms) or be serialised by a
> > > need to interact with the chip anyway. I'd expect this to be generally
> > > true, though ICBW.
>
> > So in your case it would be possible to run the various subdevice
> > thread_fn handlers from your main interrupt thread one after each
> > other ?
>
> That's what they're all doing at present outside of genirq.

They just are racy against disable/enable/request/free I guess :)

2009-07-22 15:56:19

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 05:15:59PM +0200, Thomas Gleixner wrote:
> On Wed, 22 Jul 2009, Mark Brown wrote:

> > That's what they're all doing at present outside of genirq.

> They just are racy against disable/enable/request/free I guess :)

All of them are using a locks which means none of those can run at the
same time as an interrupt is being serviced.

2009-07-22 16:04:29

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

Hi Thomas,

On Wed, Jul 22, 2009 at 02:58:24PM +0200, Thomas Gleixner wrote:
> Mark,
>
> On Wed, 22 Jul 2009, Mark Brown wrote:
>
> > On Wed, Jul 22, 2009 at 12:44:01PM +0200, Thomas Gleixner wrote:
> > > On Tue, 21 Jul 2009, Mark Brown wrote:
> >
> > > > I'll need to have a more detailed look at that but it's not immediately
> > > > clear to me how a driver (or even machine) should use that code - it
> > > > looks more like it's intended to be called from within the IRQ
> > > > infrastructure than from random driver code.
> >
> > > All it needs is to set handle_level_oneshot_irq for the interrupt line
> > > of your I2C or whatever devices.
> >
> > > set_irq_handler(irq, handle_level_oneshot_irq);
> >
> > Yeah, I know - the issue I was having was that the use of set_irq_handler()
> > seemed rather rude for driver code. Grepping around I see that there
> > are actually a small number of drivers using it already but at first
> > glance most of them appear to be implementing interrupt controllers. It
> > was setting off alarm bells about abstraction layer violation like
> > set_irq_type() does.
>
> I don't think it belongs into the driver code. It belongs into the
> platform code which sets up the system and knows what's on which
> interrupt line.
>

I do think this should be set up by the driver - the platform/arch code
can't be 100% certain what model of servicing interrupts driver will
chose, nor the driver can know whether arch code set things up for
threaded or classic interrupt handling.

Since handle_level_oneshot_irq requires drivers to use threaded IRQ
model (in absence of thread interrupt will never be unmasked) it would
be better if we did set it up automatically, right there in
request_threaded_irq(). This would reduce maintenance issues between
platform and driver code.

Thanks.

--
Dmitry

2009-07-22 16:39:35

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tuesday 21 July 2009, Thomas Gleixner wrote:
> ? ? ? See http://lkml.org/lkml/2009/7/17/174

Interesting. The model is then to switch over to __set_irq_handler(),
or maybe set_irq_chip_and_handler(), to replace the toplevel dispatch
code for will-be-threaded IRQs which happen to be hooked up to inputs
that don't sense edges. (Agree, that seems like a goof. But hardware
designers sometimes have any choices there.)

The normal model is that boards don't get involved with that level of
logic ... all IRQs get set up once on generic code, and flow handlers
don't change. Or if they do change, it's done when changing the IRQ's
trigger mode from edge to level, or vice versa.

Can that be cleaned up a bit, so that the handle_level_oneshot_irq()
and unmask_oneshot_irq() stuff kicks in automatically when needed,
instead of requiring board-specific (or driver-specific) code to get
that stuff right?

- Dave

2009-07-22 16:41:22

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, 22 Jul 2009, Dmitry Torokhov wrote:
> > I don't think it belongs into the driver code. It belongs into the
> > platform code which sets up the system and knows what's on which
> > interrupt line.
> >
>
> I do think this should be set up by the driver - the platform/arch code
> can't be 100% certain what model of servicing interrupts driver will
> chose, nor the driver can know whether arch code set things up for
> threaded or classic interrupt handling.
>
> Since handle_level_oneshot_irq requires drivers to use threaded IRQ
> model (in absence of thread interrupt will never be unmasked) it would
> be better if we did set it up automatically, right there in
> request_threaded_irq(). This would reduce maintenance issues between
> platform and driver code.

No, it's the wrong way round.

The platform code sets up the platform devices. So there is no real
good reason that the platform code does not know about the details.

If it conveys the wrong irq number then it wont work, if it sets the
wrong handler it wont work either. So what ?

If the driver is implemented to use a threaded handler, which it
better is no matter what as it uses a bus, and the interrupt
controller logic is proper implemented as well then the driver does
not care about those details at all. It will magically work with any
interrupt controller you put in front of it.

If the platform maintainer sets the wrong handler or the wrong
platform data then it's not the driver writers problem.

Thanks,

tglx

2009-07-22 16:44:31

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, 22 Jul 2009, David Brownell wrote:
> On Tuesday 21 July 2009, Thomas Gleixner wrote:
>
> Interesting. The model is then to switch over to __set_irq_handler(),
> or maybe set_irq_chip_and_handler(), to replace the toplevel dispatch
> code for will-be-threaded IRQs which happen to be hooked up to inputs
> that don't sense edges. (Agree, that seems like a goof. But hardware
> designers sometimes have any choices there.)
>
> The normal model is that boards don't get involved with that level of
> logic ... all IRQs get set up once on generic code, and flow handlers
> don't change. Or if they do change, it's done when changing the IRQ's
> trigger mode from edge to level, or vice versa.
>
> Can that be cleaned up a bit, so that the handle_level_oneshot_irq()
> and unmask_oneshot_irq() stuff kicks in automatically when needed,
> instead of requiring board-specific (or driver-specific) code to get
> that stuff right?

The only way I can see is to set a special trigger flag like
IRQF_TRIGGER_RISING & Co.

i.e. IRQF_TRIGGER_LEVEL | IRQF_TRIGGER_ONESHOT

That might work.

Thanks,

tglx

2009-07-22 16:50:46

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 09:39:32AM -0700, David Brownell wrote:

> The normal model is that boards don't get involved with that level of
> logic ... all IRQs get set up once on generic code, and flow handlers
> don't change. Or if they do change, it's done when changing the IRQ's
> trigger mode from edge to level, or vice versa.

> Can that be cleaned up a bit, so that the handle_level_oneshot_irq()
> and unmask_oneshot_irq() stuff kicks in automatically when needed,
> instead of requiring board-specific (or driver-specific) code to get
> that stuff right?

Thomas was reluctant to do that but up till now every single user has
been asking for it. A flag in the driver requesting the behaviour
sounds OK to me, there are devices that can use threaded IRQs while
interacting with the device in the primary handler NAPI style.

2009-07-22 16:51:57

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wednesday 22 July 2009, Thomas Gleixner wrote:
> main interrupt
> ? ? ?hardirq handler wakes main thread handler
>
> main thread handler
> ? ? bus magic
> ? ??????subdevice1 "hardirq" handler wakes subdevice1 irq thread
> ? ??????subdevice2 "hardirq" handler wakes subdevice2 irq thread

Why force this wasteful/undesired "all subdevices must
have their own IRQ handling thread" policy?

As previously noted, a single thread typically suffices.
There's no need to waste a dozen (or so) pages of memory
for such purposes ... these events are (as noted most
recently by Mark) infrequent/rare and performance isn't
a functionality gatekeeper. Plus ...

> ? ? main thread handler waits for subdevice1/2 handlers to complete

... sharing that thread can eliminate that synchronization
problem, and simplify the whole process.

> subdevice1 thread handler
> ? ? bus magic
> ? ? ....
> ? ? thread_fn returns
> ? ? signal main thread handler via completion
>
> subdevice2 thread handler
> ? ? bus magic
> ? ? ....
> ? ? thread_fn returns
> ? ? signal main thread handler via completion
>
> main thread handler resumes
> ? ? bus magic
> ? ? main thread handler returns from thread_fn
> ? ? unmask main interrupt

2009-07-22 16:57:18

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wednesday 22 July 2009, Peter Zijlstra wrote:
> Wouldn't it be better if we could express the nesting property from
> within genirq, so that we can do things like:
>
> ? register_chip_nested(parent_chip, parent_irq, slave_chip);
>
> And let genirq set-up the needed magic to make the nesting work.

I've been requesting such IRQ chaining support for some time
now ... if the ears are now listening, that kind of direction
should be pursued.

> Also, how important is it that subhandler1..n run in their own thread?

Completely unimportant in a practical sense. Undesirable, even;
wasteful to allocate all those stack pages and keep them idle
most of the time.

There might be an argument that the design isn't technicaly done
until that model *can* be supported. On the flip side, last time
this came up there was no "customer demand" for that ... it was
all "supplier push".

> That is, can't we let them run from the thread that is otherwise waiting
> for the completino anyway?

That would be far preferable, yes.

- Dave

2009-07-22 16:57:59

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 06:40:21PM +0200, Thomas Gleixner wrote:

> If the platform maintainer sets the wrong handler or the wrong
> platform data then it's not the driver writers problem.

It becomes the driver writer's problem as soon as the people writing the
machine integration start e-mailing saying that the driver is buggy
because they've not set this stuff up for the driver, it not being
something that they'd expect to have to do for any other interrupt in
their system.

This isn't such an issue in the PC world there's relatively few people
doing the platform development but in the embedded space that's not the
case - there are vastly more people doing platform development since
essentially everyone building a new device has to do some.

2009-07-22 17:04:15

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wednesday 22 July 2009, Mark Brown wrote:
> Yes, I think that'll be required for users like gpiolib. ?If someone's
> done something like have a button implemented by attaching switch to a
> GPIO input on for something like jack detect on an audio CODEC or a wake
> source for a PMIC then we've got generic code that expects to just take
> a gpio/irq and interact with it.

Is there a problem with how it works now? GPIO calls come in
sleeping (e.g. over I2C or SPI) and non-sleeping (classic SoC
GPIOs) varieties. And it's not gpiolib which would handle any
IRQ support ... it's the driver for the GPIO chip, which would
expose both irq_chip and gpio_chip facets. (Just like classic
SoC GPIO drivers.)

So if a handler uses only non-sleeping calls, it needs at most
minor tweaks to make sure it can be used from a threaded context.

If it uses sleeping calls, it already has to arrange that it
runs in a threaded context.

2009-07-22 17:08:34

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 06:40:21PM +0200, Thomas Gleixner wrote:
> On Wed, 22 Jul 2009, Dmitry Torokhov wrote:
> > > I don't think it belongs into the driver code. It belongs into the
> > > platform code which sets up the system and knows what's on which
> > > interrupt line.
> > >
> >
> > I do think this should be set up by the driver - the platform/arch code
> > can't be 100% certain what model of servicing interrupts driver will
> > chose, nor the driver can know whether arch code set things up for
> > threaded or classic interrupt handling.
> >
> > Since handle_level_oneshot_irq requires drivers to use threaded IRQ
> > model (in absence of thread interrupt will never be unmasked) it would
> > be better if we did set it up automatically, right there in
> > request_threaded_irq(). This would reduce maintenance issues between
> > platform and driver code.
>
> No, it's the wrong way round.
>
> The platform code sets up the platform devices. So there is no real
> good reason that the platform code does not know about the details.
>
> If it conveys the wrong irq number then it wont work, if it sets the
> wrong handler it wont work either. So what ?
>
> If the driver is implemented to use a threaded handler, which it
> better is no matter what as it uses a bus, and the interrupt
> controller logic is proper implemented as well then the driver does
> not care about those details at all. It will magically work with any
> interrupt controller you put in front of it.
>
> If the platform maintainer sets the wrong handler or the wrong
> platform data then it's not the driver writers problem.
>

It is not matter of setting handler or platform data incorrctly. There
can be a perfectly working driver using I2C and non-threaded IRQ and
there can be a similar driver using threaded IRQ. The problem with your
approach that only _one_ can work with given platform setup. If platform
sets the oneshot interrup handler then threaded will work and standard
will break, otherwise standard will work and threaded will break. This
is not the best outcome, don't you agree?

--
Dmitry

2009-07-22 17:08:42

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 10:04:10AM -0700, David Brownell wrote:
> On Wednesday 22 July 2009, Mark Brown wrote:

> > source for a PMIC then we've got generic code that expects to just take
> > a gpio/irq and interact with it.

> Is there a problem with how it works now? GPIO calls come in
> sleeping (e.g. over I2C or SPI) and non-sleeping (classic SoC
> GPIOs) varieties. And it's not gpiolib which would handle any

I don't think there's any problem at all with gpiolib at all, it's just
an example user here.

> IRQ support ... it's the driver for the GPIO chip, which would
> expose both irq_chip and gpio_chip facets. (Just like classic
> SoC GPIO drivers.)

Ah, yeah. If the chip IRQ driver handles the waking of the core thread
then this'd not be a problem. For some reason I was thinking of the
driver using gpiolib when I read Thomas' post.

2009-07-22 17:13:35

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wednesday 22 July 2009, Thomas Gleixner wrote:
>
> > I do think this should be set up by the driver - the platform/arch code
> > can't be 100% certain what model of servicing interrupts driver will
> > chose, nor the driver can know whether arch code set things up for
> > threaded or classic interrupt handling.
> >
> > Since handle_level_oneshot_irq requires drivers to use threaded IRQ
> > model (in absence of thread interrupt will never be unmasked) it would
> > be better if we did set it up automatically, right there in
> > request_threaded_irq(). This would reduce maintenance issues between
> > platform and driver code.
>
> No, it's the wrong way round.
>
> The platform code sets up the platform devices. So there is no real
> good reason that the platform code does not know about the details.

Except for the "development board" family of exceptions to
such rules ... or the "Processor-on-Card" model, where the
same platform/card gets used in a variety of different
chassis configurations, with different peripherals.

It may not be possible to know *which* configuration is
being used at board setup time. However it most certainly
is known by the time a driver is configuring.

2009-07-22 17:34:08

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wednesday 22 July 2009, Thomas Gleixner wrote:
>
> > Can that be cleaned up a bit, so that the handle_level_oneshot_irq()
> > and unmask_oneshot_irq() stuff kicks in automatically when needed,
> > instead of requiring board-specific (or driver-specific) code to get
> > that stuff right?
>
> The only way I can see is to set a special trigger flag like
> IRQF_TRIGGER_RISING & Co.
>
> i.e. IRQF_TRIGGER_LEVEL | IRQF_TRIGGER_ONESHOT
>
> That might work.

That direction, yes ... request_threaded_irq() can't infer ONESHOT
from IRQF_TRIGGER_{LOW,HIGH} since the hardirq handler might be
able to get Real Work (tm) done in some non-I2C/non-SPI cases.

Another alternative syntax: magic cookies for the hardirq handler.
Example, pass NULL to get a default punt-to-irqthread behavior, with
ONESHOT set up if it sees IRQF_TRIGGER_HIGH or IRQF_TRIGGER_LOW.

2009-07-22 17:41:22

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Tuesday 21 July 2009, Thomas Gleixner wrote:
> > > - Multi-function devices like the twl4030 which have an interrupt
> > > controller on them and would like to expose that interrupt controller
> > > via the generic IRQ subsystem. This was a large part of the
> > > discussion in the thread above is a much trickier problem.
>
> Why ?

Because the current threaded IRQ framework stops short of supporting
the relevant IRQ chaining mechanisms.

It handles the first level IRQ. Not the second or third level
which needs to be dispatched from that one.

- Dave

2009-07-22 21:05:40

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, 22 Jul 2009, David Brownell wrote:
> On Wednesday 22 July 2009, Peter Zijlstra wrote:
> > Wouldn't it be better if we could express the nesting property from
> > within genirq, so that we can do things like:
> >
> > ? register_chip_nested(parent_chip, parent_irq, slave_chip);
> >
> > And let genirq set-up the needed magic to make the nesting work.
>
> I've been requesting such IRQ chaining support for some time
> now ... if the ears are now listening, that kind of direction
> should be pursued.

Well, I was all ear back then, but the disconnect between "embedded
only needs X be happy" and my repsonsibility to keep that all working
for everyone was way larger. :)

> > Also, how important is it that subhandler1..n run in their own thread?
>
> Completely unimportant in a practical sense. Undesirable, even;
> wasteful to allocate all those stack pages and keep them idle
> most of the time.
>
> There might be an argument that the design isn't technicaly done
> until that model *can* be supported. On the flip side, last time
> this came up there was no "customer demand" for that ... it was
> all "supplier push".
>
>
> > That is, can't we let them run from the thread that is otherwise waiting
> > for the completino anyway?
>
> That would be far preferable, yes.

Ok, so let me summarize what we came up with so far.

1) handle_level_oneshot_irq is the correct answer to the problem of
those "I'm behind a slow bus" interrupt controllers.

2) Some mechanism to request ONESHOT from the driver level is
required. Preferrably via a flag on request_threaded_irq

3) a function which allows to express the nested thread irq nature of
the interrupt controller and its subdevices.

4) a generic serializing mechanism which is implemented via irq_chip
functions to solve the chip->mask/unmask issue for the demultiplexed
interrupts. Something like the bus_lock/bus_sync_unlock patch I posted
earlier.

5) a common function which allows to call the thread handler of the
subdevice interrupts in the context of the main thread which takes
care of serialization against disable/enable/request/free irq et al.

Any more ?

Thanks,

tglx

2009-07-22 21:57:13

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 2:04 PM, Thomas Gleixner<[email protected]> wrote:
> On Wed, 22 Jul 2009, David Brownell wrote:
>> On Wednesday 22 July 2009, Peter Zijlstra wrote:
>> > Wouldn't it be better if we could express the nesting property from
>> > within genirq, so that we can do things like:
>> >
>> > ? register_chip_nested(parent_chip, parent_irq, slave_chip);
>> >
>> > And let genirq set-up the needed magic to make the nesting work.
>>
>> I've been requesting such IRQ chaining support for some time
>> now ... if the ears are now listening, that kind of direction
>> should be pursued.
>
> Well, I was all ear back then, but the disconnect between "embedded
> only needs X be happy" and my repsonsibility to keep that all working
> for everyone was way larger. :)
>
>> > Also, how important is it that subhandler1..n run in their own thread?
>>
>> Completely unimportant in a practical sense. ?Undesirable, even;
>> wasteful to allocate all those stack pages and keep them idle
>> most of the time.
>>
>> There might be an argument that the design isn't technicaly done
>> until that model *can* be supported. ?On the flip side, last time
>> this came up there was no "customer demand" for that ... it was
>> all "supplier push".
>>
>>
>> > That is, can't we let them run from the thread that is otherwise waiting
>> > for the completino anyway?
>>
>> That would be far preferable, yes.
>
> Ok, so let me summarize what we came up with so far.
>
> 1) handle_level_oneshot_irq is the correct answer to the problem of
> those "I'm behind a slow bus" interrupt controllers.
>
> 2) Some mechanism to request ONESHOT from the driver level is
> required. Preferrably via a flag on request_threaded_irq
>
> 3) a function which allows to express the nested thread irq nature of
> the interrupt controller and its subdevices.
>
> 4) a generic serializing mechanism which is implemented via irq_chip
> functions to solve the chip->mask/unmask issue for the demultiplexed
> interrupts. Something like the bus_lock/bus_sync_unlock patch I posted
> earlier.
>
> 5) a common function which allows to call the thread handler of the
> subdevice interrupts in the context of the main thread which takes
> care of serialization against disable/enable/request/free irq et al.
>
> Any more ?

It would also be useful to mask an edge triggered interrupt until the
thread handler has finished. The touchscreen on the G1 is connected to
an edge triggered interrupt, and the touchscreen may toggle the
interrupt line while reading its registers.

--
Arve Hj?nnev?g

2009-07-22 22:09:21

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wednesday 22 July 2009, Thomas Gleixner wrote:
> Ok, so let me summarize what we came up with so far.
>
> 1) handle_level_oneshot_irq is the correct answer to the problem of
> those "I'm behind a slow bus" interrupt controllers.

Where "slow" means "access needs to sleep" ... preventing
register access from hardirq contexts.

I think you must mean "IRQ source" not controller; in
the examples so far on this thread, the irq_chip in
these cases has been a typical SoC/ASIC thing, but the
device issuing the IRQ is over I2C/etc. (When the
irq_chip itself is across I2C/etc, #3 applies.)

> 2) Some mechanism to request ONESHOT from the driver level is
> required. Preferrably via a flag on request_threaded_irq

Preferably "explicit"; a flag implementation suffices. Yes.

> 3) a function which allows to express the nested thread irq nature of
> the interrupt controller and its subdevices.

That's one possible implementation. Basically, irq chaining
should work for threaded IRQs; some irq_chip devices will be
across sleeping/slow busses. Some will even chain to another
level of irq_chip across such a bus.

> 4) a generic serializing mechanism which is implemented via irq_chip
> functions to solve the chip->mask/unmask issue for the demultiplexed
> interrupts. Something like the bus_lock/bus_sync_unlock patch I posted
> earlier.

In general, all irq_chip methods would need to use the sleeping/slow
bus ... like set_type(), and more.

That patch somewhat resembles the twl4030_sih_irq_chip stuff.

> 5) a common function which allows to call the thread handler of the
> subdevice interrupts in the context of the main thread which takes
> care of serialization against disable/enable/request/free irq et al.

A mechanism like that, yes. ISTR sending a patch a while back with
a handle_threaded_irq() flow handler which you'd suggested. I can dig
that up if you like, but I suspect you've had more thoughts about it
since that time.

> Any more ?

Not that comes quickly to mind. If genirq can do all that, then
a lot of drivers/mfd/twl4030-irq.c can vanish ... I mention that
as probably the strongest "acceptance test" that's handy.

If you like to work with concrete use cases, that's one. Also, a
simpler "slow irq_chip" device is the mcp23s08 GPIO expander.

- Dave

2009-07-22 22:15:11

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wednesday 22 July 2009, Arve Hj?nnev?g wrote:
> It would also be useful to mask an edge triggered interrupt until the
> thread handler has finished. The touchscreen on the G1 is connected to
> an edge triggered interrupt, and the touchscreen may toggle the
> interrupt line while reading its registers.

To clarify: if it does toggle, do you want that event to be
queued up so the IRQ is re-issued -- or not? That "oneshot"
mechanism does some of that, though it's for level triggers.

Parts of that might be appropriate to handle in the threaded
IRQ handler itself. It seems a bit device-specific.

- Dave

2009-07-22 23:30:18

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

2009/7/22 David Brownell <[email protected]>:
> On Wednesday 22 July 2009, Arve Hj?nnev?g wrote:
>> It would also be useful to mask an edge triggered interrupt until the
>> thread handler has finished. The touchscreen on the G1 is connected to
>> an edge triggered interrupt, and the touchscreen may toggle the
>> interrupt line while reading its registers.
>
> To clarify: ?if it does toggle, do you want that event to be
> queued up so the IRQ is re-issued -- or not? ?That "oneshot"
> mechanism does some of that, though it's for level triggers.

For this driver yes the event should normally be queued up. If there
is an error detected though, there is no need to queue up interrupts
that are generated during the reinit.

Also, if you implement a keypad matrix driver using a threaded
interrupt handler, you will not want the interrupt to be re-issues
unless it occured after the last key was released since the scan
itself always generated more interrupts. This is not specific to
threaded interrupt handlers though, and could be handled by adding a
clear-interrupt function.

>
> Parts of that might be appropriate to handle in the threaded
> IRQ handler itself. ?It seems a bit device-specific.

Yes, the driver could request a threaded one-shot interrupt so that it
will work when it is connected to a level triggered interrupt, and
also call disable_interrupt/enable_interrrupt in the handler to avoid
extra interrupts when it is connected to an edge triggered source.
But, it would be nicer if requesting a one-shot interrupt always
works. Also, waiting until the threaded handler runs before masking
the interrupt would not work well if the interrupt line needs software
debouncing.

--
Arve Hj?nnev?g

2009-07-23 04:43:29

[permalink] [raw]

Subject: Re: Threaded interrupts for synaptic touchscreen in HTC dream

On Wed, Jul 22, 2009 at 11:04:33PM +0200, Thomas Gleixner wrote:
>
> Any more ?
>

Can we also have something like below by any chance?

--
Dmitry

genirq: provide dummy hard irq handler for threaded interrupts

From: Dmitry Torokhov <[email protected]>

Quite often drivers using threaded interrupts don't do anything
in the hard IRQ portion of their interrupt handler; everything
is offloaded to the threaded half. Instead of having every driver
implement a stub have one ready in the IRQ core.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

kernel/irq/manage.c | 22 ++++++++++++++++++----
1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 50da676..cdc52f6 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -695,6 +695,15 @@ out_thread:
return ret;
}

+/*
+ * Default hardirq handler for threaded irqs that is used when
+ * driver did not provide its own implementation.
+ */
+static irqreturn_t dummy_hardirq_handler(int irq, void *dev_id)
+{
+ return IRQ_WAKE_THREAD;
+}
+
/**
* setup_irq - setup an interrupt
* @irq: Interrupt line to setup
@@ -837,8 +846,11 @@ EXPORT_SYMBOL(free_irq);
* request_threaded_irq - allocate an interrupt line
* @irq: Interrupt line to allocate
* @handler: Function to be called when the IRQ occurs.
- * Primary handler for threaded interrupts
- * @thread_fn: Function called from the irq handler thread
+ * Primary handler for threaded interrupts.
+ * May be NULL, in which case a dummy interrupt
+ * handler is used that simply returns
+ * IRQ_WAKE_THREAD
+ * @thread_fn: Function called from the irq handler thread.
* If NULL, no irq thread is created
* @irqflags: Interrupt type flags
* @devname: An ascii name for the claiming device
@@ -917,14 +929,16 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,

if (desc->status & IRQ_NOREQUEST)
return -EINVAL;
- if (!handler)
+
+ if (!handler && !thread_fn) {
return -EINVAL;
+ }

action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
if (!action)
return -ENOMEM;

- action->handler = handler;
+ action->handler = handler ?: dummy_hardirq_handler;
action->thread_fn = thread_fn;
action->flags = irqflags;
action->name = devname;

2009-07-23 14:55:45