On 05/14/2012 03:56 PM, Henrik Rydberg wrote:
+struct mms114_data {
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ struct mutex mutex;
Other similar drivers seem to get by with the input mutex.
This is the mutex for i2c synchronization, not for input.
Yes, but you have disable_irq()/enable_irq() for that.
+static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
+ unsigned int len, u8 *val)
+{
+ struct i2c_client *client = data->client;
+ struct i2c_msg xfer;
+ u8 buf = reg& 0xff;
+ int ret;
+
+ if (reg == MMS114_MODE_CONTROL) {
+ dev_err(&client->dev, "No support to read mode control reg\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&data->mutex);
Looks like this mutex is malplaced. The function is called both from
interrupt context and from user-driven context.
This driver uses threaded irq, it is not interrupt context.
Interrupt-driven context, I meant to say. Technically, your code
works, but the pattern is unusual. The serialization is needed to
remove the race between a call initiated by the interrupt and a call
initiated by the user, coming in through suspend/resume. The standard
pattern for the latter is to turn interrupts off and wait for
interrupt completion, which you already do, then continue
execution. The effect is the same, without the mutex.
I just thought, if i don't have any locking here, there is no guarantee
that another i2c access won't happen between i2c_transfer() and
i2c_master_recv(). But this can be replaced to one i2c_transfer().
+ touchdata->strength = buf[5];
Does not seem to be used anywhere.
It seems to be used for pressure.
It is assigned a variable, but it is not reported to userland.
Right, I should add it.
+static int mms114_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct mms114_data *data = i2c_get_clientdata(client);
+ struct mms114_touchdata *touchdata = data->touchdata;
+ int id;
+ int ret;
+
It would seem the mutex should be here instead.
Any reasons? I did not feel the mutex needs here.
Oh well, as long as suspend/resume is the only user intervention, and
because those are already serialized, then maybe so. Something to
revisit in the next patch version, I presume.
Thanks,
Henrik
Thanks.
--
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
