On Tue, Oct 28, 2014 at 11:31:30PM +0100, Bastien Nocera wrote:
> On Tue, 2014-10-28 at 12:14 -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 28, 2014 at 07:59:05PM +0100, Bastien Nocera wrote:
> > > On Tue, 2014-10-28 at 11:05 -0700, Dmitry Torokhov wrote:
> > > > On Tue, Oct 28, 2014 at 06:55:21PM +0100, Bastien Nocera wrote:
> > > > > On Tue, 2014-10-07 at 13:58 -0700, Dmitry Torokhov wrote:
> > > > > > On Wed, Sep 24, 2014 at 04:43:58PM +0200, Bastien Nocera wrote:
> > > > > > > +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.
> > > >
> > > > Ah, I see. In end_cmd I encoded the address as little endian, whereas it
> > > > needs to be beg endian. Just swap "GOODIX_READ_COOR_ADDR & 0xff" and
> > > > "GOODIX_READ_COOR_ADDR >> 8" around and I thin kit will work.
> > >
> > > Indeed, fixed in:
> > > https://github.com/hadess/gt9xx/commit/18e507c5c455a3d3d745380c4d0d561ea470a091
> > >
> > > As for the various FIXMEs, could you (that includes Benjamin that
> > > probably knows the driver more than he would like to) check whether
> > > that's sensible?
> > > https://github.com/hadess/gt9xx/commit/35b94f327edc04d95a7208a667553566faa871e3
> > >
> > > If it is, I'll respin the patch and send it for merging.
> >
> > No, the "header" is not 10 bytes as far as I can see. IIUIC the device
> > sends packet with 1st byte containing number of contacts + (n_contacts *
> > GOODIX_CONTACT_SIZE) worth of data. The driver tries to optimize for
> > single-touch scenario and reads the counter and the very first contact
> > data and if the counter indicates more than one contact it will issue
> > second i2c transaction to read in the rest.
> >
> > Your change did not alter the actual code, it just converted constant 10
> > into a define.
>
> Not quite, but that's besides the point.
>
> > Please try changing FIXMEs exactly as they were and see if the driver
> > still works properly.
>
> I did, and it still works properly. I also don't understand how it could
> have worked properly before...
>
> Should I still add those linefeeds to all the print statements in the
> driver?
Yes please.
--
Dmitry
--
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
