Re: [PATCH 2/4] Input: atmel_mxt_ts - Support 12bit resolution

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

Hi,

On 2011-03-31 오후 10:43, Iiro Valkonen wrote:
> Hi Joonyoung,
>
> my comments below. They a bit late, but looks like the patch is not applied yet.
>
> On 03/07/2011 10:42 AM, Joonyoung Shim wrote:
>> Atmel touchscreen chip can support 12bit resolution and this patch
>> modifies to get maximum x and y size from platform data.
>>
>> Signed-off-by: Joonyoung Shim<jy0922.shim@samsung.com>
>> ---
>>   drivers/input/touchscreen/atmel_mxt_ts.c |   50 +++++++++++++++++++++--------
>>   1 files changed, 36 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 0986fa4..0ed0b1f 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -192,9 +192,12 @@
>>   #define MXT_PRESS              (1<<  6)
>>   #define MXT_DETECT             (1<<  7)
>>
>> +/* Touch orient bits */
>> +#define MXT_XY_SWITCH          (1<<  0)
>> +#define MXT_X_INVERT           (1<<  1)
>> +#define MXT_Y_INVERT           (1<<  2)
>> +
>>   /* Touchscreen absolute values */
>> -#define MXT_MAX_XC             0x3ff
>> -#define MXT_MAX_YC             0x3ff
>>   #define MXT_MAX_AREA           0xff
>>
>>   #define MXT_MAX_FINGER         10
>> @@ -242,6 +245,8 @@ struct mxt_data {
>>          struct mxt_info info;
>>          struct mxt_finger finger[MXT_MAX_FINGER];
>>          unsigned int irq;
>> +       unsigned int x_size;
>> +       unsigned int y_size;
>>   };
>>
>>   static bool mxt_object_readable(unsigned int type)
>> @@ -541,8 +546,13 @@ static void mxt_input_touchevent(struct mxt_data *data,
>>          if (!(status&  (MXT_PRESS | MXT_MOVE)))
>>                  return;
>>
>> -       x = (message->message[1]<<  2) | ((message->message[3]&  ~0x3f)>>  6);
>> -       y = (message->message[2]<<  2) | ((message->message[3]&  ~0xf3)>>  2);
>> +       x = (message->message[1]<<  4) | ((message->message[3]>>  4)&  0xf);
>> +       y = (message->message[2]<<  4) | ((message->message[3]&  0xf));
>> +       if (data->x_size - 1<  1024)
>> +               x = x>>  2;
>> +       if (data->y_size - 1<  1024)
>> +               y = y>>  2;
>> +
>
> Not a big deal, but I would just store x_max_value instead of x_size to avoid
> the subtraction. Same for y.
>

OK.

>>          area = message->message[4];
>>
>>          dev_dbg(dev, "[%d] %s x: %d, y: %d, area: %d\n", id,
>> @@ -837,6 +847,17 @@ static int mxt_initialize(struct mxt_data *data)
>>          return 0;
>>   }
>>
>> +static void mxt_calc_resolution(struct mxt_data *data)
>> +{
>> +       if (data->pdata->orient&  MXT_XY_SWITCH) {
>> +               data->x_size = data->pdata->y_size;
>> +               data->y_size = data->pdata->x_size;
>> +       } else {
>> +               data->x_size = data->pdata->x_size;
>> +               data->y_size = data->pdata->y_size;
>> +       }
>> +}
>> +
>
> What's the reason for this? If we have set the x/y switch in the config, then
> we probably want to swap the axes. Or is this axis swap something that should be
> done on upper layers? Even so, then we shouldn't have the MXT_XY_SWITCH bit set
> in the config, and we could just say "data->x_max_value = data->pdata->xsize - 1"
> (and same for y) in the probe function. We wouldn't need Touch orient bit defines
> either.
>

If we set XY_SWITCH for special purpose then the axis is swapped and
driver will report also coordinates out of max value.

>>   static ssize_t mxt_object_show(struct device *dev,
>>                                      struct device_attribute *attr, char *buf)
>>   {
>> @@ -1044,31 +1065,32 @@ static int __devinit mxt_probe(struct i2c_client *client,
>>          input_dev->open = mxt_input_open;
>>          input_dev->close = mxt_input_close;
>>
>> +       data->client = client;
>> +       data->input_dev = input_dev;
>> +       data->pdata = pdata;
>> +       data->irq = client->irq;
>> +
>> +       mxt_calc_resolution(data);
>> +
>>          __set_bit(EV_ABS, input_dev->evbit);
>>          __set_bit(EV_KEY, input_dev->evbit);
>>          __set_bit(BTN_TOUCH, input_dev->keybit);
>>
>>          /* For single touch */
>>          input_set_abs_params(input_dev, ABS_X,
>> -                            0, MXT_MAX_XC, 0, 0);
>> +                            0, data->x_size, 0, 0);
>>          input_set_abs_params(input_dev, ABS_Y,
>> -                            0, MXT_MAX_YC, 0, 0);
>> +                            0, data->y_size, 0, 0);
>>
>>          /* For multi touch */
>>          input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR,
>>                               0, MXT_MAX_AREA, 0, 0);
>>          input_set_abs_params(input_dev, ABS_MT_POSITION_X,
>> -                            0, MXT_MAX_XC, 0, 0);
>> +                            0, data->x_size, 0, 0);
>>          input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
>> -                            0, MXT_MAX_YC, 0, 0);
>> +                            0, data->y_size, 0, 0);
>>
>
> Should these be "data->x_size - 1"? Or if we store max_x_value like suggested
> above, this too will be fixed.
>

Right, i also think better to use you suggest.

>>          input_set_drvdata(input_dev, data);
>> -
>> -       data->client = client;
>> -       data->input_dev = input_dev;
>> -       data->pdata = pdata;
>> -       data->irq = client->irq;
>> -
>>          i2c_set_clientdata(client, data);
>>
>>          error = mxt_initialize(data);
>> --
>> 1.7.0.4
>>
>
> Thanks,
>

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