Re: [PATCH] hid-multitouch: changes from the review process

[Henrik Rydberg]

Hi Benjamin,

seems we are cloing in now, although there is still an outstanding
issue with the setting of curvalid, as replied in your previous
mail. Some comments on this patch, too.

>  struct mt_class mt_classes[] = {
> -	{ 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
> -	{ MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
> -	{ MT_QUIRK_SLOT_IS_CONTACTNUMBER, 0, 0, 10 },    /* MT_CLS_DUAL2 */
> -	{ MT_QUIRK_CYPRESS | MT_QUIRK_NOT_SEEN_MEANS_UP, 0, 0, 10 }, /* MT_CLS_CYPRESS */
> +	{ .name = MT_CLS_DEFAULT,
> +		.quirks = 0,

Please do not zero-initialize.

> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 10 },
> +	{ .name = MT_CLS_DUAL1,
> +		.quirks = MT_QUIRK_SLOT_IS_CONTACTID,
> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 2 },
> +	{ .name = MT_CLS_DUAL2,
> +		.quirks = MT_QUIRK_SLOT_IS_CONTACTNUMBER,
> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 2 },
> +	{ .name = MT_CLS_CYPRESS,
> +		.quirks = MT_QUIRK_CYPRESS,
> +		.sn_move = 0,
> +		.sn_pressure = 0,
> +		.maxcontacts = 10 },
> +
> +	{ }
>  };

So no device is marked as NOT_SEEN_MEANS_UP, although allegedly some devices should...

> @@ -282,11 +297,10 @@ static void mt_emit_event(struct mt_device *td, struct input_dev *input)
>  
>  	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>  		struct mt_slot *s = &(td->slots[i]);
> -		if ((td->mtclass->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
> -			!s->seen_in_this_frame) {
> +		if (!s->seen_in_this_frame) {
>  			/*
> -			 * this slot does not contain useful data,
> -			 * notify its closure
> +			 * FixMe: use MT_QUIRK_NOT_SEEN_MEANS_UP here.
> +			 * This requires to change the curvalid logic.
>  			 */
>  			s->touch_state = false;
>  		}

If we were to push this broken behavior, simply to avoid changing the
currently tested code, we would only push the problem of testing onto
the next cycle, with a larger risk of introducing regressions. I
really think we need to sort this out now.

> @@ -392,9 +407,16 @@ static void mt_set_input_mode(struct hid_device *hdev)
>  
>  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
> -	int ret;
> +	int ret, i;
>  	struct mt_device *td;
> -	struct mt_class *mtclass = mt_classes + id->driver_data;
> +	struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
> +
> +	for (i = 0; mt_classes[i].name ; i++) {
> +		if (id->driver_data == mt_classes[i].name) {
> +			mtclass = &(mt_classes[i]);
> +			break;
> +		}
> +	}

Nice.

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/