[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-input
Subject: Re: [PATCH v4] input: Add MELFAS mms114 touchscreen driver
From: Joonyoung Shim <jy0922.shim () samsung ! com>
Date: 2012-05-24 1:32:04
Message-ID: 4FBD8F94.70605 () samsung ! com
[Download RAW message or body]
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
>
--
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic