Re: [PATCH 12/20] Input: atmel_mxt_ts - simplify event reporting

From: Daniel Kurtz
Date: Mon Mar 19 2012 - 05:06:32 EST


On Mon, Mar 19, 2012 at 4:26 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
> Hi Daniel,
>
>> Instead of carrying around per-finger state in the driver instance, just
>> report each finger as it arrives to the input layer, and let the input
>> layer (evdev) hold the event state (which it does anyway).
>>
>> Also, the atmel pad reports "amplitude", which is reported to userspace
>> using the "PRESSURE" event type.  The variables now reflect this.
>>
>> Note: this driver does not really do MT-B properly. Each input report
>> (a goup of input events followed by a SYN_REPORT) only contains data for
>> a single contact.  When multiple fingers are present on a device, each is
>> properly reported in its own MT_SLOT.  However, there is only ever one
>> MT_SLOT per SYN_REPORT.  This is fixed in a subsequent patch.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@xxxxxxxxxxxx>
>> ---
>
> Simplification looking good overall, together with that later
> patch. Please find some comments below.
>
>>  drivers/input/touchscreen/atmel_mxt_ts.c |  122 +++++++++---------------------
>>  1 files changed, 36 insertions(+), 86 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 4363511..fa692e5 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -194,6 +194,7 @@
>>  #define MXT_BOOT_STATUS_MASK 0x3f
>>
>>  /* Touch status */
>> +#define MXT_UNGRIP           (1 << 0)
>>  #define MXT_SUPPRESS         (1 << 1)
>>  #define MXT_AMP                      (1 << 2)
>>  #define MXT_VECTOR           (1 << 3)
>> @@ -238,14 +239,6 @@ struct mxt_message {
>>       u8 message[7];
>>  };
>>
>> -struct mxt_finger {
>> -     int status;
>> -     int x;
>> -     int y;
>> -     int area;
>> -     int pressure;
>> -};
>> -
>>  /* Each client has this additional data */
>>  struct mxt_data {
>>       struct i2c_client *client;
>> @@ -253,7 +246,6 @@ struct mxt_data {
>>       const struct mxt_platform_data *pdata;
>>       struct mxt_object *object_table;
>>       struct mxt_info info;
>> -     struct mxt_finger finger[MXT_MAX_FINGER];
>>       unsigned int irq;
>>       unsigned int max_x;
>>       unsigned int max_y;
>> @@ -526,97 +518,55 @@ static int mxt_write_object(struct mxt_data *data,
>>       return mxt_write_reg(data->client, reg + offset, 1, &val);
>>  }
>>
>> -static void mxt_input_report(struct mxt_data *data, int single_id)
>> -{
>> -     struct mxt_finger *finger = data->finger;
>> -     struct input_dev *input_dev = data->input_dev;
>> -     int status = finger[single_id].status;
>> -     int finger_num = 0;
>> -     int id;
>> -
>> -     for (id = 0; id < MXT_MAX_FINGER; id++) {
>> -             if (!finger[id].status)
>> -                     continue;
>> -
>> -             input_mt_slot(input_dev, id);
>> -             input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
>> -                             finger[id].status != MXT_RELEASE);
>> -
>> -             if (finger[id].status != MXT_RELEASE) {
>> -                     finger_num++;
>> -                     input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
>> -                                     finger[id].area);
>> -                     input_report_abs(input_dev, ABS_MT_POSITION_X,
>> -                                     finger[id].x);
>> -                     input_report_abs(input_dev, ABS_MT_POSITION_Y,
>> -                                     finger[id].y);
>> -                     input_report_abs(input_dev, ABS_MT_PRESSURE,
>> -                                     finger[id].pressure);
>> -             } else {
>> -                     finger[id].status = 0;
>> -             }
>> -     }
>> -
>> -     input_report_key(input_dev, BTN_TOUCH, finger_num > 0);
>> -
>> -     if (status != MXT_RELEASE) {
>> -             input_report_abs(input_dev, ABS_X, finger[single_id].x);
>> -             input_report_abs(input_dev, ABS_Y, finger[single_id].y);
>> -             input_report_abs(input_dev,
>> -                              ABS_PRESSURE, finger[single_id].pressure);
>> -     }
>> -
>> -     input_sync(input_dev);
>> -}
>> -
>>  static void mxt_input_touchevent(struct mxt_data *data,
>> -                                   struct mxt_message *message, int id)
>> +                              struct mxt_message *message, int id)
>>  {
>> -     struct mxt_finger *finger = data->finger;
>>       struct device *dev = &data->client->dev;
>> -     u8 status = message->message[0];
>> +     struct input_dev *input_dev = data->input_dev;
>> +     u8 status;
>>       int x;
>>       int y;
>>       int area;
>> -     int pressure;
>> -
>> -     /* Check the touch is present on the screen */
>> -     if (!(status & MXT_DETECT)) {
>> -             if (status & MXT_RELEASE) {
>> -                     dev_dbg(dev, "[%d] released\n", id);
>> -
>> -                     finger[id].status = MXT_RELEASE;
>> -                     mxt_input_report(data, id);
>> -             }
>> -             return;
>> -     }
>> -
>> -     /* Check only AMP detection */
>> -     if (!(status & (MXT_PRESS | MXT_MOVE)))
>> -             return;
>> +     int amplitude;
>>
>> +     status = message->message[0];
>>       x = (message->message[1] << 4) | ((message->message[3] >> 4) & 0xf);
>>       y = (message->message[2] << 4) | ((message->message[3] & 0xf));
>>       if (data->max_x < 1024)
>> -             x = x >> 2;
>> +             x >>= 2;
>>       if (data->max_y < 1024)
>> -             y = y >> 2;
>> +             y >>= 2;
>>
>>       area = message->message[4];
>> -     pressure = message->message[5];
>> -
>> -     dev_dbg(dev, "[%d] %s x: %d, y: %d, area: %d\n", id,
>> -             status & MXT_MOVE ? "moved" : "pressed",
>> -             x, y, area);
>> -
>> -     finger[id].status = status & MXT_MOVE ?
>> -                             MXT_MOVE : MXT_PRESS;
>> -     finger[id].x = x;
>> -     finger[id].y = y;
>> -     finger[id].area = area;
>> -     finger[id].pressure = pressure;
>> +     amplitude = message->message[5];
>> +
>> +     dev_dbg(dev,
>> +             "[%d] %c%c%c%c%c%c%c%c x: %d y: %d area: %d amp: %d\n",
>> +             id,
>> +             (status & MXT_DETECT) ? 'D' : '.',
>> +             (status & MXT_PRESS) ? 'P' : '.',
>> +             (status & MXT_RELEASE) ? 'R' : '.',
>> +             (status & MXT_MOVE) ? 'M' : '.',
>> +             (status & MXT_VECTOR) ? 'V' : '.',
>> +             (status & MXT_AMP) ? 'A' : '.',
>> +             (status & MXT_SUPPRESS) ? 'S' : '.',
>> +             (status & MXT_UNGRIP) ? 'U' : '.',
>> +             x, y, area, amplitude);
>> +
>> +     input_mt_slot(input_dev, id);
>> +     input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
>> +                                status & MXT_DETECT);
>> +
>> +     if (status & MXT_DETECT) {
>> +             input_report_abs(input_dev, ABS_MT_POSITION_X, x);
>> +             input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
>> +             input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
>> +             /* TODO: This should really be sqrt(area) */
>> +             input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
>
> The functional relationship might not be perfect, but at least the
> reported scale should match the position units, as several userspace
> drivers depend on its accuracy. If the line size looks reasonable in
> mtview, for instance, it should be fine.

Please note this patch doesn't actually change how TOUCH_MAJOR is
reported. All I did here was add the TODO, since reporting 'area' as
TOUCH_MAJOR looks suspect to me :). Unfortunately, I don't actually
know the relationship between what the firmware sends as 'area' and
the position units... perhaps someone with more experience with how
the firmware works could help us here.

-Dan

>
>> +     }
>>
>> -     mxt_input_report(data, id);
>> +     input_mt_report_pointer_emulation(input_dev, false);
>> +     input_sync(input_dev);
>>  }
>>
>>  static irqreturn_t mxt_interrupt(int irq, void *dev_id)
>> --
>
> Thanks,
> Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/