Re: patch staging-htc-dream-touchscreen-driver-for-staging.patch added to gregkh-2.6 tree

Re: patch staging-htc-dream-touchscreen-driver-for-staging.patch added to gregkh-2.6 tree

[Pavel Machek]

On Mon 2009-07-27 15:45:48, Arve Hj?nnev?g wrote:
> On Mon, Jul 27, 2009 at 9:06 AM, <gregkh@suse.de> wrote:
> ...
> > This is original version from Arve, fixed only to compile; I do have
> > cleaner version, but I broken something inside, and this will preserve
> > authorship better. I originally tried to push the driver directly to
> > input/touchscreen, but the driver has some issues with interrupt
> > handling that are more difficult to fix than I expected at first.
> >
> 
> While it would be nice to use the threaded interrupt support, I don't
> see why that change cannot be postponed until the the threaded
> interrupt code supports this class of devices where the interrupt
> needs to be masked before the thread can run.

That's up to linux-input people, I guess. But... 

>From the discussion, it seems that the driver has more issues. I have
quite long list.

1) either it needs to switch to threaded interrupt supports, or all
the races need to be fixed.

[Comments I remember here is:]

(
> > +           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?_
)

2) it uses non-standard multitouch support. Either multitouch needs to
be disabled for merge, or it needs to switch to standard config.

3) poll mode should probably be removed.

4) minor stuff, like properly filling identification fields
(vendor/device/product) etc.

Yes, ideally it should be 3 or 4 patches and it is out of staging and
in the kernel proper. But as I already got patches for people trying
to help with cleaning, I believe staging is right place for
it.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: patch staging-htc-dream-touchscreen-driver-for-staging.patch added to gregkh-2.6 tree

[Arve Hjønnevåg]

On Mon, Jul 27, 2009 at 3:55 PM, Pavel Machek<pavel@ucw.cz> wrote:
> On Mon 2009-07-27 15:45:48, Arve Hj?nnev?g wrote:
>> On Mon, Jul 27, 2009 at 9:06 AM, <gregkh@suse.de> wrote:
>> ...
>> > This is original version from Arve, fixed only to compile; I do have
>> > cleaner version, but I broken something inside, and this will preserve
>> > authorship better. I originally tried to push the driver directly to
>> > input/touchscreen, but the driver has some issues with interrupt
>> > handling that are more difficult to fix than I expected at first.
>> >
>>
>> While it would be nice to use the threaded interrupt support, I don't
>> see why that change cannot be postponed until the the threaded
>> interrupt code supports this class of devices where the interrupt
>> needs to be masked before the thread can run.
>
> That's up to linux-input people, I guess. But...
>
> From the discussion, it seems that the driver has more issues. I have
> quite long list.
>
> 1) either it needs to switch to threaded interrupt supports, or all
> the races need to be fixed.
>
> [Comments I remember here is:]
>
> (
>> > +           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?_
> )

Unless you changed something there should be no failure path after
starting the timer or enabling the interrupt.

synaptics_ts_remove is missing a flush_work call though.

>
> 2) it uses non-standard multitouch support. Either multitouch needs to
> be disabled for merge, or it needs to switch to standard config.
>

I have a patch that adds support for the new protocol, but it does not
yet remove the old hack.

> 3) poll mode should probably be removed.
>
> 4) minor stuff, like properly filling identification fields
> (vendor/device/product) etc.
>
> Yes, ideally it should be 3 or 4 patches and it is out of staging and
> in the kernel proper. But as I already got patches for people trying
> to help with cleaning, I believe staging is right place for
> it.
>                                                                        Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>



-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch staging-htc-dream-touchscreen-driver-for-staging.patch added to gregkh-2.6 tree

[Pavel Machek]

Hi!

> >> Don't see canceling timer nor shutting off WQ here. Also, maybe
> >> implement open() and close() so we don't reschedule WQ while polling?_
> > )
> 
> Unless you changed something there should be no failure path after
> starting the timer or enabling the interrupt.
> 
> synaptics_ts_remove is missing a flush_work call though.

Can you submit patch to fix that?

> > 2) it uses non-standard multitouch support. Either multitouch needs to
> > be disabled for merge, or it needs to switch to standard config.
> 
> I have a patch that adds support for the new protocol, but it does not
> yet remove the old hack.

Yes, such patch would be welcome. I guess we could wrap old, hacky
protocol in ifdef, and then remove it when moving to drivers/input.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: patch staging-htc-dream-touchscreen-driver-for-staging.patch added to gregkh-2.6 tree

[Arve Hjønnevåg]

On Tue, Jul 28, 2009 at 4:14 AM, Pavel Machek<pavel@ucw.cz> wrote:
> Hi!
>
>> >> Don't see canceling timer nor shutting off WQ here. Also, maybe
>> >> implement open() and close() so we don't reschedule WQ while polling?_
>> > )
>>
>> Unless you changed something there should be no failure path after
>> starting the timer or enabling the interrupt.
>>
>> synaptics_ts_remove is missing a flush_work call though.
>
> Can you submit patch to fix that?
>

When fix it, sure. This code is unreachable in the configurations we
use though so I never noticed it before.

>> > 2) it uses non-standard multitouch support. Either multitouch needs to
>> > be disabled for merge, or it needs to switch to standard config.
>>
>> I have a patch that adds support for the new protocol, but it does not
>> yet remove the old hack.
>
> Yes, such patch would be welcome. I guess we could wrap old, hacky
> protocol in ifdef, and then remove it when moving to drivers/input.

I included the patch below. It is not against your cleaned up version
of the driver, but it could use a review in its current form since I'm
unclear on how up events should be handled. Another touchscreen only
sends events for the finger that moved, so the absence of a finger
report is not a good indication that it was released.

-- 
Arve Hjønnevåg

---

From edc71cab6a21fd0415b626bf2cbe737af29e3491 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Arve=20Hj=C3=B8nnev=C3=A5g?= <arve@android.com>
Date: Mon, 20 Jul 2009 20:11:31 -0700
Subject: [PATCH] Input: synaptics_i2c_rmi: Send new multi touch events
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Send both fingers using the new multitouch protocol in addition to
the old single finger event and second finger ABS_HAT* hack.

The new protocol does not specify how to report up events. To make
sure the user-space frameworks can generate up events, we send at
least one event where ABS_MT_TOUCH_MAJOR is zero before we stop
sending events for a finger.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/input/touchscreen/synaptics_i2c_rmi.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/synaptics_i2c_rmi.c
b/drivers/input/touchscreen/synaptics_i2c_rmi.c
index 56cf371..e003e42 100644
--- a/drivers/input/touchscreen/synaptics_i2c_rmi.c
+++ b/drivers/input/touchscreen/synaptics_i2c_rmi.c
@@ -43,6 +43,7 @@ struct synaptics_ts_data {
        int snap_down[2];
        int snap_up[2];
        uint32_t flags;
+       int reported_finger_count;
        int8_t sensitivity_adjust;
        int (*power)(int on);
        struct early_suspend early_suspend;
@@ -204,6 +205,26 @@ static void synaptics_ts_work_func(struct
work_struct *work)

input_report_abs(ts->input_dev, ABS_HAT0X, pos[1][0]);

input_report_abs(ts->input_dev, ABS_HAT0Y, pos[1][1]);
                                }
+
+                               if (!finger)
+                                       z = 0;
+                               input_report_abs(ts->input_dev,
ABS_MT_TOUCH_MAJOR, z);
+                               input_report_abs(ts->input_dev,
ABS_MT_WIDTH_MAJOR, w);
+                               input_report_abs(ts->input_dev,
ABS_MT_POSITION_X, pos[0][0]);
+                               input_report_abs(ts->input_dev,
ABS_MT_POSITION_Y, pos[0][1]);
+                               input_mt_sync(ts->input_dev);
+                               if (finger2_pressed) {
+
input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, z);
+
input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, w);
+
input_report_abs(ts->input_dev, ABS_MT_POSITION_X, pos[1][0]);
+
input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, pos[1][1]);
+                                       input_mt_sync(ts->input_dev);
+                               } else if (ts->reported_finger_count > 1) {
+
input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0);
+
input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0);
+                                       input_mt_sync(ts->input_dev);
+                               }
+                               ts->reported_finger_count = finger;
                                input_sync(ts->input_dev);
                        }
                }
@@ -480,6 +501,10 @@ static int synaptics_ts_probe(
        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);+
input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
-inactive_area_left, max_x + inactive_area_right, fuzz_x, 0);
+       input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
-inactive_area_top, max_y + inactive_area_bottom, fuzz_y, 0);
+       input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0,
255, fuzz_p, 0);
+       input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 15,
fuzz_w, 0);
        /* ts->input_dev->name = ts->keypad_info->name; */
        ret = input_register_device(ts->input_dev);
        if (ret) {
--
1.6.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html