-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Dmitry.
On 04/04/2012 09:10 PM, Dmitry Torokhov wrote:
> On Wed, Apr 04, 2012 at 08:27:59PM +0200, simon.budig@xxxxxxxxxxxxxxxxx wrote:
>> + if (!have_abs) {
>> + input_report_key(tsdata->input, BTN_TOUCH, 1);
>> + input_report_abs(tsdata->input, ABS_PRESSURE, 1);
>> + input_report_abs(tsdata->input, ABS_X,
>> + ((rdbuf[i*4+5] << 8) |
>> + rdbuf[i*4+6]) & 0x0fff);
>> + input_report_abs(tsdata->input, ABS_Y,
>> + ((rdbuf[i*4+7] << 8) |
>> + rdbuf[i*4+8]) & 0x0fff);
>> + have_abs = 1;
>
>
> The mt pointer emulation should do this for you.
Can you point me to some documentation on that? Do I need to enable this?
[...]
>> + mutex_lock(&tsdata->mutex);
>> +
>> + if (tsdata->factory_mode) {
>> + dev_err(dev,
>> + "setting register not available in factory mode\n");
>
> This will spam logs, just return error silently.
Hmm, the idea was to give the user a hint for an attempt to read the
attribute values in factory mode instead of just silently failing.
Where would be a proper place to document such device-specific behaviour?
[...]
> See if you could wrap an attribute into your own structure that will
> allow you to get to the address, field and limits without matching on
> attribute name.
will try.
>> + mutex_lock(&tsdata->mutex);
>
> Instead of taking mutex why don't you disable IRQ?
Does the Linux kernel guarantee that there is just one attribute access
at a time?
Otherwise:
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.
[...]
>> +static int edt_ft5x06_i2c_ts_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> +
>> + struct edt_ft5x06_i2c_ts_data *tsdata;
>> + struct edt_ft5x06_platform_data *pdata;
>
> const.
What do you mean?
[..]
>> + /* request IRQ pin */
>> + tsdata->irq_pin = pdata->irq_pin;
>> + tsdata->irq = gpio_to_irq(tsdata->irq_pin);
>
> Take from the client.
[...]
> Should this GPIO configuration be done by the driver or by the board
> code that configures i2c client? I think latter is better.
I got conflicting opinions when I asked for advice on this last december
(before changing it to a pin-based configuration). Why do you think that
your suggestion is better?
>> + mutex_lock(&tsdata->mutex);
>
> Why are you taking this lock here?
Paranoia. I wanted to ensure that the controller stays in
non-factory-mode because I am about to read some configuration
registers- However, the attributes are probably not yet available to
userspace, so I can probably skip the mutex in _probe.
>> + input->name = kstrdup(model_name, GFP_NOIO);
>
> Why don't you just allocate a few bytes in tsdata structure instead of
> allocating and managing this separately. You'll also avoid race when
> freeing it.
Yeah, I guess that is simpler.
Thanks for the review.
Simon
- --
Simon Budig kernel concepts GmbH
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/
iEYEARECAAYFAk98tI4ACgkQO2O/RXesiHAopwCfTnRzBEiFbN/tGcRl/TLBl7yR
KpwAn13Bzx+Q6Avj0edwwC+6qkPjAhfq
=Zg0q
-----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
