-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Dmitry
Thanks for your review. I used a lot of your hints.
Sorry, the patch was developed for an embedded platform which
unfortunately still is stuck on 2.6.38. I now have adapted the patch for
current GIT head, but I could not yet test it properly (for the lack of
the proper kernel for my testhardware). So the following patch should
probably be taken with a grain of salt.
On 09/28/2011 07:47 AM, Dmitry Torokhov wrote:
>> + mutex_lock (&tsdata->mutex);
>> + ret = edt_ft5x06_ts_readwrite (tsdata->client,
>> + 1, wrbuf,
>> + sizeof (rdbuf), rdbuf);
>
> i2c_transfer() already provides necessary locking on adapter level so
> several transfers would not race with each other. This locking seems
> excessive.
The reason for this locking is two fold.
First, the touch sensor has two modes, a "operation mode" and a "factory
mode". The problem is, that the register numbering in the two modes is
mostly disjunct. I.e. reading the "gain" register in factory mode gives
a number, which has no real connection to the actual gain value. Since
the mode can be changed via a sysfs file I have a potential race
condition when e.g. concurrent access to the sysfs entries happen:
Lets say I want to read the gain value, while something else tries to
switch to factory mode.
1) edt_ft5x06_i2c_setting_show checks for factory_mode == 0
2) edt_ft5x06_i2c_mode_store changes factory_mode to 1
3) edt_ft5x06_i2c_setting_show reads register 0x30, reading and
returning a bogus value.
Also reading raw values is a series of i2c transactions (for each column
of the sensor) and I need to avoid at least a mode change inbetween.
That is the purpose of the mutex.
>> + input_report_abs (tsdata->input, ABS_PRESSURE, 1);
>
> If the device does not report true pressure readings please do not fake
> them. BTN_TOUCH alone should suffice.
Hum, ok. I need to check again, but at some point tslib had a problem
with a missing ABS_PRESSURE axis. Yeah, this would need fixing in tslib
obviously. Not sure what the current state of this problem is though.
>> + input_report_abs (tsdata->input, ABS_MT_POSITION_X, ((rdbuf[i*4+5] << 8) | rdbuf[i*4+6]) & 0x0fff);
>> + input_report_abs (tsdata->input, ABS_MT_POSITION_Y, ((rdbuf[i*4+7] << 8) | rdbuf[i*4+8]) & 0x0fff);
>> + input_report_abs (tsdata->input, ABS_MT_TRACKING_ID, (rdbuf[i*4+7] >> 4) & 0x0f);
>> + input_mt_sync (tsdata->input);
>
> Any change to use stateful MT-B protocol here?
I will look into it. For now the application software expects MT-A,
although changing this shouldnt be too hard.
>> + disable_irq (tsdata->irq);
>
> Do you really need to disable IRQ here? We already established that
> i2c_transefr()s won't race.
I need to think about that. Not sure at the moment.
>> + mutex_lock (&tsdata->mutex);
>> +
>> + if (tsdata->factory_mode == 1) {
>> + dev_err (dev, "setting register not available in factory mode\n");
>
> You just left with mutex locked. Mutex is most likely not needed at all.
Oops, thanks.
>> +static ssize_t edt_ft5x06_i2c_raw_data_show (struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata (dev);
>> + int i, ret;
>> + char *ptr, wrbuf[3];
>> +
>> + if (tsdata->factory_mode == 0) {
>> + dev_err (dev, "raw data not available in work mode\n");
>> + return -EIO;
>> + }
>> +
>> + mutex_lock (&tsdata->mutex);
>> + ret = edt_ft5x06_i2c_register_write (tsdata, 0x08, 0x01);
>> + for (i = 0; i < 100; i++) {
>> + ret = edt_ft5x06_i2c_register_read (tsdata, 0x08);
>> + if (ret < 1)
>> + break;
>> + udelay (1000);
>> + }
>> +
>> + if (i == 100 || ret < 0) {
>> + dev_err (dev, "waiting time exceeded or error: %d\n", ret);
>> + mutex_unlock (&tsdata->mutex);
>> + return ret || ETIMEDOUT;
>
> -ENOPARSE.
Ok, maybe this is written a bit convoluted.
I need to trigger the readout of the raw values (the register_write) and
then wait until the raw values have been prepared. That is the loop
which waits up to 100 msecs. The loop ends when the register_read either
returns an error (< 0) or the register value has changed to 0 (== 0).
If ret is < 0 (an error happened) or I ended up doing 100 iterations of
the loop (the timeout) I return either the error or ETIMEDOUT.
...at least that was the intention. I just realize there indeed is a
bug, the return statement is bogus. I have adressed this in the next
iteration.
>> +
>> + return 0;
>> +
>> +err_free_irq:
>> + free_irq (client->irq, tsdata);
>> +err_unregister_device:
>> + input_unregister_device (input);
>> +err_free_input_device:
>> + kfree (input->name);
>> + input_free_device (input);
>
> Calls to input_free_device() are prohibited after calling
> input_unregister_device().
Hmm, Ok. I missed that other modules set the input pointer to NULL in
their cleanup path.
I will change that, the updated patch follows soon.
Thanks,
Simon
- --
Simon Budig kernel concepts GbR
simon.budig@xxxxxxxxxxxxxxxxx Sieghuetter Hauptweg 48
+49-271-771091-17 D-57072 Siegen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAk6EktEACgkQO2O/RXesiHDWrgCfUb7hZ5v+Fs4/js92QfRGbj8o
aFQAn3MENdFAM503X1QE0dMppnnxrDQh
=EeeG
-----END PGP SIGNATURE-----
--
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
