Re: [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Jeff,

On Sun, Feb 10, 2019 at 04:39:54PM -0600, Jeff LaBundy wrote:
> This patch adds support for the Azoteq IQS550/572/525 family of
> trackpad/touchscreen controllers.
> 
> The driver has been tested with an IQS550EV02 evaluation board. A
> demonstration of the driver's capabilities is available here:
> 
> https://youtu.be/sRNNx4XZBts

A few comments:

- we have request_ihex_firmware that requests and validates biary
  representation of ihex into the kernel. There is not really a good
  reason to parse text ihex in kernel.
  There is idex2bin in ./firmware to convert it.

- I would recommend against doing this whole business of automatic
  uprevving and updating firmware on probe. The controller mees to have
  persistent firmware, so just check whether the device comes up in
  bootloader or normal mode, and if it is in bootloader mode simply
  forego creating and registering input device and wait for the
  userspace to update firmware ad fix it up.
  Not having to deal with firmware on probe will also allow compiling
  the driver into the kernel (vs being a module) as firmware loading is
  not normally available until after root filesystem is mounted.

- firmware update is usually keyed off device model, not DT name.

> +static int iqs5xx_read_burst(struct i2c_client *client,
> +				u16 reg, u8 *val8, u8 len)

If you make val8 void * you won't need to cast.

> +
> +static int iqs5xx_write_burst(struct i2c_client *client,
> +				u16 reg, u8 *val8, u8 len)

const void *.

> +	error = devm_request_threaded_irq(&client->dev, client->irq,
> +			NULL, iqs5xx_irq,
> +			IRQF_ONESHOT | IRQF_TRIGGER_RISING,

Please do not specify IRQ trigger, let it come from device tree, so only
use IRQF_ONESHOT.

By the way, I'd recommend using level interrupts so you never miss the
edge.

Thanks.

-- 
Dmitry