On 12/21/2018 09:32 AM, Dmitry Torokhov wrote:
> On Fri, Dec 21, 2018 at 03:25:45AM +0100, Marek Vasut wrote:
>> On 12/21/2018 02:52 AM, Dmitry Torokhov wrote:
>>> On Thu, Dec 20, 2018 at 09:43:02PM +0100, Marek Vasut wrote:
>>>> Get rid of the packed structures for representing data as that does not
>>>> apply to other similar Ilitek touchscreens. Instead, implement a function
>>>> which parses the data and reports touch events and coordinates.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@xxxxxxx>
>>>> Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>>>> Cc: Henrik Rydberg <rydberg@xxxxxxxxxxx>
>>>> Cc: Olivier Sobrie <olivier@xxxxxxxxx>
>>>> Cc: Philipp Puschmann <pp@xxxxxxxxx>
>>>> To: linux-input@xxxxxxxxxxxxxxx
>>>> ---
>>>> V2: Replace touchdata.status with touchdata[0] and move it into
>>>> ili210x_report_events() in anticipation of ILI251x specific changes,
>>>> and to to synchronize the patch with changes in
>>>> "input: touchscreen: ili210x: Convert to devm_ functions"
>>>> ---
>>>> drivers/input/touchscreen/ili210x.c | 59 +++++++++++++++++------------
>>>> 1 file changed, 34 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
>>>> index a84ca555829e..ab0e6c9a614c 100644
>>>> --- a/drivers/input/touchscreen/ili210x.c
>>>> +++ b/drivers/input/touchscreen/ili210x.c
>>>> @@ -16,20 +16,11 @@
>>>> #define REG_FIRMWARE_VERSION 0x40
>>>> #define REG_CALIBRATE 0xcc
>>>>
>>>> -struct finger {
>>>> +struct panel_info {
>>>> u8 x_low;
>>>> u8 x_high;
>>>> u8 y_low;
>>>> u8 y_high;
>>>> -} __packed;
>>>> -
>>>> -struct touchdata {
>>>> - u8 status;
>>>> - struct finger finger[MAX_TOUCHES];
>>>> -} __packed;
>>>> -
>>>> -struct panel_info {
>>>> - struct finger finger_max;
>>>> u8 xchannel_num;
>>>> u8 ychannel_num;
>>>> } __packed;
>>>> @@ -74,25 +65,40 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>>> return 0;
>>>> }
>>>>
>>>> -static void ili210x_report_events(struct input_dev *input,
>>>> - const struct touchdata *touchdata)
>>>> +static bool ili210x_touchdata_to_coords(struct ili210x *priv, u8 *touchdata,
>>>> + unsigned int finger,
>>>> + unsigned int *x, unsigned int *y)
>>>> +{
>>>> + u8 x_low, x_high, y_low, y_high;
>>>> +
>>>> + if (finger >= MAX_TOUCHES)
>>>> + return false;
>>>> +
>>>> + if (touchdata[0] & BIT(finger))
>>>> + return false;
>>>> +
>>>> + x_high = touchdata[1 + (finger * 4) + 0];
>>>> + x_low = touchdata[1 + (finger * 4) + 1];
>>>
>>> Sounds like get_unaligned_be16().
>>
>> Fixed
>>
>>>> + y_high = touchdata[1 + (finger * 4) + 2];
>>>> + y_low = touchdata[1 + (finger * 4) + 3];
>>>> + *x = x_low | (x_high << 8);
>>>> + *y = y_low | (y_high << 8);
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +static bool ili210x_report_events(struct ili210x *priv, u8 *touchdata)
>>>> {
>>>> int i;
>>>> bool touch;
>>>> unsigned int x, y;
>>>> - const struct finger *finger;
>>>>
>>>> for (i = 0; i < MAX_TOUCHES; i++) {
>>>> input_mt_slot(input, i);
>>>>
>>>> - finger = &touchdata->finger[i];
>>>> -
>>>> - touch = touchdata->status & (1 << i);
>>>> + touch = ili210x_touchdata_to_coords(priv, touchdata, i, &x, &y);
>>>> input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
>>>> if (touch) {
>>>> - x = finger->x_low | (finger->x_high << 8);
>>>> - y = finger->y_low | (finger->y_high << 8);
>>>> -
>>>> input_report_abs(input, ABS_MT_POSITION_X, x);
>>>> input_report_abs(input, ABS_MT_POSITION_Y, y);
>>>> }
>>>> @@ -100,6 +106,8 @@ static void ili210x_report_events(struct input_dev *input,
>>>>
>>>> input_mt_report_pointer_emulation(input, false);
>>>> input_sync(input);
>>>> +
>>>> + return touchdata[0] & 0xf3;
>>>> }
>>>>
>>>> static void ili210x_work(struct work_struct *work)
>>>> @@ -107,20 +115,21 @@ static void ili210x_work(struct work_struct *work)
>>>> struct ili210x *priv = container_of(work, struct ili210x,
>>>> dwork.work);
>>>> struct i2c_client *client = priv->client;
>>>> - struct touchdata touchdata;
>>>> + u8 touchdata[1 + 4 * MAX_TOUCHES];
>>>
>>> Would it make sense to make the buffer part of struct ili210x and mark
>>> it ____cacheline_aligned so that we can use dmasafe I2C APIs?
>>
>> Given how few (up to 32) bytes are transferred from the device, I'm not
>> sure. Would it ?
>
> It is really up to you. We do not have many I2C masters doing DMA I
> think, but they do exist.
The iMX6 one I use does not do I2C DMA, so I guess we can leave this as
is. Plus, I think the overhead of setting up DMA for 31 byte transfer is
not worth it. However, if someone proves me wrong by actually testing it
on hardware that can do I2C DMA, good :)
--
Best regards,
Marek Vasut
