Re: [PATCH] input: Add MELFAS mms114 touchscreen driver

[Joonyoung Shim <jy0922.shim@samsung.com>]

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.