Re: [PATCH v2 1/4] hid-multitouch: Auto detection of maxcontacts

[Benjamin Tissoires]

Hi Henrik,

thanks for the review.

On Thu, Mar 10, 2011 at 13:45, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
> Hi Benjamin,
>
> thanks for this, just a couple of things...
>
>> @@ -126,9 +127,19 @@ struct mt_class mt_classes[] = {
>>  static void mt_feature_mapping(struct hid_device *hdev,
>>               struct hid_field *field, struct hid_usage *usage)
>>  {
>> -     if (usage->hid == HID_DG_INPUTMODE) {
>> -             struct mt_device *td = hid_get_drvdata(hdev);
>> +     struct mt_device *td = hid_get_drvdata(hdev);
>> +
>> +     switch (usage->hid) {
>> +     case HID_DG_INPUTMODE:
>>               td->inputmode = field->report->id;
>> +             break;
>> +     case HID_DG_CONTACTMAX:
>> +             td->maxcontacts = *(field->value);
>
> field->value[0] seems more standard-ish

If you want

>
>> +             if (td->mtclass->maxcontacts > td->maxcontacts)
>> +                     /* check if the maxcontacts is given by the class */
>> +                     td->maxcontacts = td->mtclass->maxcontacts;
>> +
>> +             break;
>>       }
>>  }
>>
>> @@ -317,7 +327,7 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>       struct mt_device *td = hid_get_drvdata(hid);
>>       __s32 quirks = td->mtclass->quirks;
>>
>> -     if (hid->claimed & HID_CLAIMED_INPUT) {
>> +     if (hid->claimed & HID_CLAIMED_INPUT && td->slots) {
>
> The extra test could in principle be placed after the usage switch, which would prevent partially filled contact data, should this ever occur. Something like
>
> if (!td->slots)
>   break;

I don't know where these "break" comes from. We can add the test after
the switch before the mt_complete_slot and mt_emit_event. But please
note than it can in all cases introduce truncated slots reports as all
the slots in the report may not have been completed. To be honest, I'm
not sure this will ever matter as it's a race at the connection of the
device, and the reports will come to correct anything wrong.

>
>>               switch (usage->hid) {
>>               case HID_DG_INRANGE:
>>                       if (quirks & MT_QUIRK_VALID_IS_INRANGE)
>> @@ -415,9 +425,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>        */
>>       hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
>>
>> -     td = kzalloc(sizeof(struct mt_device) +
>> -                             mtclass->maxcontacts * sizeof(struct mt_slot),
>> -                             GFP_KERNEL);
>> +     td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
>>       if (!td) {
>>               dev_err(&hdev->dev, "cannot allocate multitouch data\n");
>>               return -ENOMEM;
>> @@ -434,6 +442,14 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>       if (ret)
>>               goto fail;
>>
>> +     td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
>> +                             GFP_KERNEL);
>> +     if (!td->slots) {
>> +             dev_err(&hdev->dev, "cannot allocate multitouch slots\n");
>> +             ret = -ENOMEM;
>> +             goto fail;
>
> This exit leaves the device running.

Yep, will do sth.

Thanks
Benjamin

>
>> +     }
>> +
>>       mt_set_input_mode(hdev);
>>
>>       return 0;
>> @@ -455,6 +471,7 @@ static void mt_remove(struct hid_device *hdev)
>>  {
>>       struct mt_device *td = hid_get_drvdata(hdev);
>>       hid_hw_stop(hdev);
>> +     kfree(td->slots);
>>       kfree(td);
>>       hid_set_drvdata(hdev, NULL);
>>  }
>> --
>> 1.7.4
>>
>
> Thanks,
> Henrik
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
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/