Hi Simon,

quoted

I would very much prefer if the driver functioned well without
such settings, since they complicate userspace and are not likely
to ever go away. Oh well.

I would prefer to have the touchscreen adjust itself as well,
unfortunately this is not available and you definitely need different
settings depending for different touch setups.

Would an ioctl() be more acceptable? Would make it harder to adjust it
in startup scripts etc. though.

The format (ioctl, sysfs or whatnot) is of no consequence, it is the
a) special handling of certain drivers and b) forcing userspace to
tune the driver that makes maintenance problematic in the long
run. With every screen project ending in its own solution, this
quickly becomes a problem, both in the kernel and in
userspace. Preventing such predicaments is my concern.

quoted

quoted

+	if (error) { +		dev_err(&tsdata->client->dev, +			"Unable to
write to fetch data, error: %d\n", error); +		goto out; +	}

No risk of flooding the logs here? Perhaps rate-limit the outputs?

Hmm, possible. Can you point me to a driver that does this in a sane
fashion?

Different methods all over the kernel; there is dev_err_ratelimited(),
pr_warn_once(), printk_once(), special solutions... take you pick.

quoted

quoted

+	for (i = 0; i < MAX_SUPPORT_POINTS; i++) { +		u8 *buf =
&rdbuf[i * 4]; +		bool down; + +		type = buf[5] >> 6; +		/*
ignore Reserved events */ +		if (type == TOUCH_EVENT_RESERVED) +
continue;

As per the implementation by Olivier, it seems these touches may
get stuck in the down position. No?

Not if you do the loop over all 5 event entries in the report. The
n_touches field really contains the number of fingers on the touch,
not the number of events in the report. Since the "down"-events
conveniently are sorted to the beginning of the report for the type A
protocol it was enough to to just iterate over these (we ignored the
UP-events anyway). Now that we need them we just iterate over all of
the events and there they are.

Ok, good.

quoted

quoted

+	tsdata->threshold = edt_ft5x06_i2c_register_read(tsdata, +
WORK_REGISTER_THRESHOLD); +	tsdata->gain =
edt_ft5x06_i2c_register_read(tsdata, +						WORK_REGISTER_GAIN); 
+	tsdata->offset = edt_ft5x06_i2c_register_read(tsdata, +
WORK_REGISTER_OFFSET); +	tsdata->report_rate =
edt_ft5x06_i2c_register_read(tsdata, +
WORK_REGISTER_REPORT_RATE); +	tsdata->num_x =
edt_ft5x06_i2c_register_read(tsdata, +
WORK_REGISTER_NUM_X); +	tsdata->num_y =
edt_ft5x06_i2c_register_read(tsdata, +
WORK_REGISTER_NUM_Y); + +	dev_dbg(&client->dev, +		"Model \"%s\",
Rev. \"%s\", %dx%d sensors\n", +		tsdata->name, fw_version,
tsdata->num_x, tsdata->num_y); + +	input->name = tsdata->name; +
input->id.bustype = BUS_I2C; +	input->dev.parent = &client->dev; 
+ +	__set_bit(EV_SYN, input->evbit); +	__set_bit(EV_KEY,
input->evbit); +	__set_bit(EV_ABS, input->evbit); +
__set_bit(BTN_TOUCH, input->keybit); +
input_set_abs_params(input, ABS_X, 0, tsdata->num_x * 64 - 1, 0,
0); +	input_set_abs_params(input, ABS_Y, 0, tsdata->num_y * 64 -
1, 0, 0); +	input_set_abs_params(input, ABS_MT_POSITION_X, +
0, tsdata->num_x * 64 - 1, 0, 0); +	input_set_abs_params(input,
ABS_MT_POSITION_Y, +			     0, tsdata->num_y * 64 - 1, 0, 0); +
input_mt_init_slots(input, MAX_SUPPORT_POINTS);

No error checking here?

I guess you're referring to the _register_read()s? Yeah, they probably
could use some.

I was thinking of input_mt_init_slots(). Yes, not all drivers check it
because the kernel wont crash, but it wont function as planned either.

Thanks,
Henrik