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.
Thanks.
--
Dmitry
