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

Hi Henrik,

On 05/23/2012 08:52 PM, Henrik Rydberg wrote:

Hi Joonyoung,

quoted

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<redacted>
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[off-list ref]

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

Henrik

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+	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