Re: [PATCH] Touchscreen driver for FT5x06 based EDT displays

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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:
> > On Wed, Apr 04, 2012 at 08:27:59PM +0200, simon.budig@kernelconcepts.de 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?
> 

Just do

	input_mt_report_pointer_emulation(tsdata->input, true);

> [...]
> 
> >> +	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?

> 
> [...]
> 
> > 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.

Ah, I see.

> 
> [...]
> >> +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;

> 
> [..]
> >> +	/* 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?

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.

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