On Tue, 2014-10-07 at 13:58 -0700, Dmitry Torokhov wrote:
> Hi Bastien,
>
> On Wed, Sep 24, 2014 at 04:43:58PM +0200, Bastien Nocera wrote:
<snip>
> > + if (touch_num > 1) {
> > + ret = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR + 10,
> > + &data[10], 8 * (touch_num - 1));
> > + if (ret < 0)
> > + return ret;
> > + }
>
> I am a bit confused about this function. It looks like contact packet
> size is 8 bytes, and they preceded by a byte with total number of
> contacts reported, so why instead of 9 bytes we are reading 10?
I have no idea. We didn't change the original code here:
https://github.com/hadess/gt9xx/commit/82b141220e8bce00060e0de697735d0a70af2678#diff-5d71019f9b92cc9d6e2e31ed2e6520b6R363
<snip>
> > +/**
> > + * goodix_process_events - Process incoming events
> > + *
> > + * @ts: our goodix_ts_data pointer
> > + *
> > + * Called when the IRQ is triggered. Read the current device state, and push
> > + * the input events to the user space.
> > + */
> > +static void goodix_process_events(struct goodix_ts_data *ts)
> > +{
> > + u8 point_data[1 + 8 * GOODIX_MAX_TOUCH + 1];
>
> Here again, why do we need extra byte?
Same answer:
https://github.com/hadess/gt9xx/commit/82b141220e8bce00060e0de697735d0a70af2678#diff-5d71019f9b92cc9d6e2e31ed2e6520b6R315
<snip>
> > +/**
> > + * goodix_ts_irq_handler - The IRQ handler
> > + *
> > + * @irq: interrupt number.
> > + * @dev_id: private data pointer.
> > + */
> > +static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> > +{
> > + struct goodix_ts_data *ts = dev_id;
> > + u8 end_cmd[1] = {0};
> > +
> > + goodix_process_events(ts);
> > +
> > + if (goodix_i2c_write(ts->client,
> > + GOODIX_READ_COOR_ADDR, end_cmd, 1) < 0)
> > + dev_err(&ts->client->dev, "I2C write end_cmd error");
>
> I am not happy that we need to allocate/deallocate memory for each
> interrupt. We only write one command to the driver, we could simply use
> i2c_master_send() with a constant buffer.
Sure. But I've split up the patch you sent us, and committed the
different bits separately in:
https://github.com/hadess/gt9xx/commits/master
And this one commit about removing goodix_i2c_write():
https://github.com/hadess/gt9xx/commit/146b4cc2eed5c67bcf1cb91e845bf9f97da4be1e
Breaks the driver.
> BTW, you need terminate kernel messages with \n.
All of them? If so, we have a few more that are still missing in the
latest version of the driver, which I can take care of.
> Also, below is a patch with a few assorted changes that I'd like you to
> try if you have time.
As mentioned, all the changes seem fine apart from the one removing
goodix_i2c_write().
I'll test to see if we can reduce the size of the point_data structure,
and see what's breaking the goodix_i2c_write() patch.
Thanks for the patch!
Cheers
--
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
