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@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?
>
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@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-----
--
Dmitry
--
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
