On Wed, Apr 04, 2012 at 10:52:30PM +0200, Simon Budig wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Dmitry.

On 04/04/2012 09:10 PM, Dmitry Torokhov wrote:

quoted

On Wed, Apr 04, 2012 at 08:27:59PM +0200, simon.budig@kernelconcepts.de wrote:

quoted

+		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?

Just do

	input_mt_report_pointer_emulation(tsdata->input, true);

[...]

quoted

quoted

+	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?

Maybe Documentation/input/...? Thisis hardware-mandated behavior, right?

[...]

quoted

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.

quoted

quoted

+	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.

Ah, I see.

[...]

quoted

quoted

+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?

	const struct edt_ft5x06_platform_data *pdata = client->dev.platform_data;

[..]

quoted

quoted

+	/* request IRQ pin */
+	tsdata->irq_pin = pdata->irq_pin;
+	tsdata->irq = gpio_to_irq(tsdata->irq_pin);

Take from the client.

[...]

quoted

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?

Can the device be connected via a pin not plugged into gpio subsystem?
Other than deriving IRQ from it you do not seem to be using it.

quoted

quoted

+	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.

quoted

quoted

+	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@kernelconcepts.de      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-----

-- 
Dmitry