Re: [PATCH 2/2 v5] Input: cy8ctma140 - add driver

[Linus Walleij <linus.walleij@linaro.org>]

Hi Dmitry!

First: THANKS for fixing my driver.
Second: sorry for slow answer :(

On Thu, Apr 23, 2020 at 4:05 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> > +config TOUCHSCREEN_CY8CTMA140
> > +     tristate "cy8ctma140 touchscreen"
> > +     depends on I2C
> > +     depends on GPIOLIB || COMPILE_TEST
>
> Why do we need gpiolib here?

Not really. Dropped it.

> > +     ret = __i2c_transfer(ts->client->adapter, msg, ARRAY_SIZE(msg));
>
> Why do we use "unlocked" __i2c_transfer()? Can't we use normal
> i2c_transfer()? For __i2c_transfer() someone has to lock the segment...

Good point. Your patch works fine here.

> Hmm, I think if we do use loop the code is actually _much_ simpler,
> please see the patch below.

Indeed. Works like a charm after some fixes.

> > +     addr[0] = CY8CTMA140_GET_FW_INFO;
> > +     ret = i2c_master_send(ts->client, addr, 1);
> > +     if (ret < 0) {
> > +             dev_err(ts->dev, "error sending FW info message\n");
> > +             return ret;
> > +     }
> > +     ret = i2c_master_recv(ts->client, buf, 5);
> > +     if (ret < 0) {
> > +             dev_err(ts->dev, "error recieveing FW info message\n");
> > +             return ret;
> > +     }
>
> Does it have to be 2 separate transactions? Can we use write/read
> i2c_transfer() here?

I think it is necessary for the FW info test, this is not a standard
I2C transaction, notice that addr[0] does not contain the I2C
address of the device.

I tried to do it some other way but this was the
only way I could get it to work and detect the device properly.

> Can you please try the patch below on top of yours?

Folded it in and will resend with my changes as v6.

> @@ -187,26 +144,28 @@ static int cy8ctma140_init(struct cy8ctma140 *ts)
>  {
>         u8 addr[1];
>         u8 buf[5];
> +       int error;
>         int ret;
>
>         addr[0] = CY8CTMA140_GET_FW_INFO;
> -       ret = i2c_master_send(ts->client, addr, 1);
> -       if (ret < 0) {
> +       error = i2c_master_send(ts->client, addr, 1);
> +       if (error) {

This didn't work as i2c_master_send() returns the number of
bytes sent. I reverted this function back to using ret.

After that everything started to work!

Yours,
Linus Walleij