Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Adam Ford]

On Mon, Nov 4, 2019 at 12:37 PM Sven Van Asbroeck <thesven73@xxxxxxxxx> wrote:
>
> Ok, so here are my test results on an ili211x :
>
> Using Marek's patch:
> https://patchwork.kernel.org/patch/10836651/#22811657
> It works fine.

I am using IRQ_TYPE_EDGE_RISING for the 2117A.  Is that correct?  For
my touchscreen, the IRQ line is low until a touch is detected, so I
assume we want to capure on the rising edge.

I noticed the example uses IRQ_TYPE_EDGE_FALLING.  If rising is
correct, we should probably update the binding to show an example for
the 2117.

>
> Using Dmitry's patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=ili2xxx-touchscreen
> Does not work at all - the driver even enters an infinite loop.
>

Regarding Dmitry's patch,
Is it a good idea to use msleep in an IRQ?  It seems like using the
schedule_delayed_work() call seems like it will get in and get out of
the ISR faster.

If we use msleep and scan again, isn't it possible to starve other processes?



> I tracked this down to two separate issues:
> 1. the ili211x does not have a touchdata register; but the driver tries to
>         read from one.
> 2. the ili211x should never poll - otherwise it will read all zeros, which
>         passes the crc check (!), then results in ten finger touches all
>         at (x=0, y=0).
>
> The patch at the end of this e-mail addresses these two issues. When it is
> applied, the ili211x works fine.
>
> Of course, this does not address the issue Marek saw with Dmitry's patch
>         on the ili251x.
>

Sven's patches appear to work for me when manually applied on top of
Dmity' and Marek's patches.


> Sven
>
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 7a9c46b8a079..f51a3a19075f 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -36,7 +36,7 @@ struct ili2xxx_chip {
>         int (*get_touch_data)(struct i2c_client *client, u8 *data);
>         bool (*parse_touch_data)(const u8 *data, unsigned int finger,
>                                  unsigned int *x, unsigned int *y);
> -       bool (*continue_polling)(const u8 *data);
> +       bool (*continue_polling)(const u8 *data, bool has_touch);
>         unsigned int max_touches;
>  };
>
> @@ -96,7 +96,7 @@ static bool ili210x_touchdata_to_coords(const u8 *touchdata,
>         return true;
>  }
>
> -static bool ili210x_check_continue_polling(const u8 *data)
> +static bool ili210x_check_continue_polling(const u8 *data, bool has_touch)
>  {
>         return data[0] & 0xf3;
>  }
> @@ -111,14 +111,19 @@ static const struct ili2xxx_chip ili210x_chip = {
>
>  static int ili211x_read_touch_data(struct i2c_client *client, u8 *data)
>  {
> +       struct i2c_msg msg = {
> +               .addr   = client->addr,
> +               .flags  = I2C_M_RD,
> +               .len    = ILI211X_DATA_SIZE,
> +               .buf    = data,
> +       };
>         s16 sum = 0;
>         int i;
> -       int error;
>
> -       error = ili210x_read_reg(client, REG_TOUCHDATA,
> -                                data, ILI211X_DATA_SIZE);
> -       if (error)
> -               return error;
> +       if (i2c_transfer(client->adapter, &msg, 1) != 1) {
> +               dev_err(&client->dev, "i2c transfer failed\n");
> +               return -EIO;
> +       }
>
>         /* This chip uses custom checksum at the end of data */
>         for (i = 0; i <= ILI211X_DATA_SIZE - 2; i++)
> @@ -152,7 +157,7 @@ static bool ili211x_touchdata_to_coords(const u8 *touchdata,
>         return true;
>  }
>
> -static bool ili2xxx_decline_polling(const u8 *data)
> +static bool ili2xxx_decline_polling(const u8 *data, bool has_touch)
>  {
>         return false;
>  }
> @@ -216,11 +221,16 @@ static bool ili251x_touchdata_to_coords(const u8 *touchdata,
>         return true;
>  }
>
> +static bool ili251x_check_continue_polling(const u8 *data, bool has_touch)
> +{
> +       return has_touch;
> +}
> +
>  static const struct ili2xxx_chip ili251x_chip = {
>         .read_reg               = ili251x_read_reg,
>         .get_touch_data         = ili251x_read_touch_data,
>         .parse_touch_data       = ili251x_touchdata_to_coords,
> -       .continue_polling       = ili2xxx_decline_polling,
> +       .continue_polling       = ili251x_check_continue_polling,
>         .max_touches            = 10,
>  };
>
> @@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
>                 }
>
>                 touch = ili210x_report_events(priv, touchdata);
> -               keep_polling = touch || chip->continue_polling(touchdata);
> +               keep_polling = chip->continue_polling(touchdata, touch);
>                 if (keep_polling)

Why not just check the value of touch instead of invoking the function
pointer which takes the value of touch in as a parameter?

>                         msleep(ILI2XXX_POLL_PERIOD);
>         } while (!priv->stop && keep_polling);
> --
> 2.17.1

adam