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

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

Hi Henrik,

On 05/23/2012 08:52 PM, Henrik Rydberg wrote:
> Hi Joonyoung,
>
>> This is a initial driver for new touchscreen chip mms114 of MELFAS.
>> It uses I2C interface and supports 10 multi touch.
>>
>> Signed-off-by: Joonyoung Shim<jy0922.shim@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
> Look good overall, some minor comments below which may be
> ignored. Thank you,
>
>      Reviewed-by: Henrik Rydberg<rydberg@euromail.se>

I will update some codes with your comments. Thanks for review.

> Henrik
>
>> +static void mms114_proc_mt(struct mms114_data *data, u8 *buf)
>> +{
>> +	const struct mms114_platform_data *pdata = data->pdata;
>> +	struct i2c_client *client = data->client;
>> +	struct input_dev *input_dev = data->input_dev;
>> +	unsigned int id;
>> +	unsigned int type;
>> +	unsigned int pressed;
>> +	unsigned int x;
>> +	unsigned int y;
>> +	unsigned int width;
>> +	unsigned int strength;
>> +
>> +	id = (buf[0]&  MMS114_ID_MASK) - 1;
>> +	if (id>= MMS114_MAX_TOUCH) {
>> +		dev_dbg(&client->dev, "Wrong touch id (%d)\n", id);
>> +		return;
>> +	}
>> +
>> +	type = (buf[0]>>  MMS114_TYPE_OFFSET)&  MMS114_TYPE_MASK;
>> +	if (type != MMS114_TYPE_TOUCHSCREEN) {
>> +		dev_dbg(&client->dev, "Wrong touch type (%d)\n", type);
>> +		return;
>> +	}
>> +
>> +	x = buf[2] | (buf[1]&  0xf)<<  8;
>> +	y = buf[3] | ((buf[1]>>  4)&  0xf)<<  8;
>> +	if (x>  pdata->x_size || y>  pdata->y_size) {
>> +		dev_dbg(&client->dev, "Wrong touch coordinates (%d, %d)\n",
>> +				x, y);
>> +		return;
>> +	}
>> +
>> +	if (pdata->x_invert)
>> +		x = pdata->x_size - x;
>> +	if (pdata->y_invert)
>> +		y = pdata->y_size - y;
>> +
>> +	pressed = (buf[0]>>  MMS114_ACT_OFFSET)&  MMS114_ACT_MASK;
>> +	width = buf[4];
>> +	strength = buf[5];
> Why not pass a struct like this instead:
>
> struct mms114_touch {
>         u8 id : 4, type : 3, pressed : 1;
>         u8 x_hi : 4, y_hi : 4;
>         u8 x_lo;
>         u8 y_lo;
>         u8 width;
>         u8 strength;
> } __packed;
>
> Most of the code above would go away, together with some defines.

Yes, good idea.

>> +
>> +	dev_dbg(&client->dev, "id: %d, type: %d, pressed: %d\n",
>> +			id, type, pressed);
>> +	dev_dbg(&client->dev, "x: %d, y: %d, width: %d, strength: %d\n",
>> +			x, y, width, strength);
>> +
>> +	input_mt_slot(input_dev, id);
>> +	input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, pressed);
>> +
>> +	if (pressed) {
>> +		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, width);
>> +		input_report_abs(input_dev, ABS_MT_POSITION_X, x);
>> +		input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
>> +		input_report_abs(input_dev, ABS_MT_PRESSURE, strength);
>> +	}
>> +}
>> +static int mms114_start(struct mms114_data *data)
>> +{
>> +	int error;
>> +
>> +	mutex_lock(&data->mutex);
>> +	if (!data->enabled) {
>> +		if (data->core_reg)
>> +			regulator_enable(data->core_reg);
>> +		if (data->io_reg)
>> +			regulator_enable(data->io_reg);
>> +		mdelay(MMS114_POWERON_DELAY);
>> +
>> +		error = mms114_setup_regs(data);
>> +		if (error<  0) {
>> +			mutex_unlock(&data->mutex);
>> +			return error;
>> +		}
>> +
>> +		data->enabled = true;
>> +	}
>> +	mutex_unlock(&data->mutex);
>> +	return 0;
>> +}
> A more canonical formulation of the above function would be:
>
>    int error = 0;
>
>    mutex_lock(&data->mutex);
>    if (data->enabled)
>            goto out;
>
>    if (data->core_reg)
>            regulator_enable(data->core_reg);
>    if (data->io_reg)
>            regulator_enable(data->io_reg);
>    mdelay(MMS114_POWERON_DELAY);
>
>    error = mms114_setup_regs(data);
>    if (error)
>            goto out;
>
>    data->enabled = true;
> out:
>    mutex_unlock(&data->mutex);
>    return error;
>

I don't want to return the variable of "error" name on error == 0 case,
just return 0, but i can use goto statement.

>> +static void mms114_stop(struct mms114_data *data)
>> +{
>> +	mutex_lock(&data->mutex);
>> +	if (data->enabled) {
>> +		if (data->io_reg)
>> +			regulator_disable(data->io_reg);
>> +		if (data->core_reg)
>> +			regulator_disable(data->core_reg);
>> +
>> +		data->enabled = false;
>> +	}
>> +	mutex_unlock(&data->mutex);
>> +}
> Ditto.
>
>> +static int mms114_suspend(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct mms114_data *data = i2c_get_clientdata(client);
>> +	struct input_dev *input_dev = data->input_dev;
>> +	int id;
>> +
>> +	disable_irq(client->irq);
>> +
>> +	/* Release all touch */
>> +	for (id = 0; id<  MMS114_MAX_TOUCH; id++) {
>> +		input_mt_slot(input_dev, id);
>> +		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, false);
>> +	}
>> +
>> +	input_mt_report_pointer_emulation(input_dev, true);
>> +	input_sync(input_dev);
>> +
>> +	mutex_lock(&input_dev->mutex);
>> +	if (input_dev->users)
>> +		mms114_stop(data);
> It should really stop unconditionally here, but the above ought to
> yield the same result.

Right.

>> +	mutex_unlock(&input_dev->mutex);
>> +
>> +	if (data->pdata->cfg_pin)
>> +		data->pdata->cfg_pin(false);
>> +
>> +	return 0;
>> +}
>> +static int mms114_resume(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct mms114_data *data = i2c_get_clientdata(client);
>> +	struct input_dev *input_dev = data->input_dev;
>> +	int error;
>> +
>> +	if (data->pdata->cfg_pin)
>> +		data->pdata->cfg_pin(true);
>> +
>> +	mutex_lock(&input_dev->mutex);
>> +	if (input_dev->users) {
>> +		error = mms114_start(data);
>> +		if (error<  0) {
>> +			mutex_unlock(&input_dev->mutex);
>> +			return error;
>> +		}
>> +	}
>> +	mutex_unlock(&input_dev->mutex);
>> +
>> +	enable_irq(client->irq);
>> +	return 0;
>> +}
> Could use more gotos here as well.
>
> Thanks,
> Henrik
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>