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@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> ---
Look good overall, some minor comments below which may be
ignored. Thank you,
Reviewed-by: Henrik Rydberg <rydberg@xxxxxxxxxxx>
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.
> +
> + 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;
> +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.
> + 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
