Re: [PATCH v3] Input: Add driver for GOODiX GTx5 series touchsereen

[Bastien Nocera <hadess@hadess.net>]

Hey,

Very shallow review. There are tons of typos in the code and comments,
including in the subject line for this patch ("touchsereen"), including
"godix", "excuted", etc. Please run a spell-check over the whole patch,
and have somebody review all the text.

On Tue, 2017-06-20 at 21:00 +0800, Wang Yafei wrote:
> This driver is for GOODiX GTx5 series touchscreen controllers
> such as GT8589, GT7589. This driver designed with hierarchial 

"hierarchical"

> structure,
> for that can be modified to support subsequent controllers easily.
> Some zones of the touchscreen can be set to buttons(according to the
> hardware). That is why it handles button and multitouch events.
> 
> A brief description of driver structure
> - Core Layer: This layer responsible for basic input events report,
>   GPIO pinctrl, Interrupt, Power resources manager and submodules
> manager.
> - Hardware Layer: This layer responsible for controllers
> initialization,
>   irq handle as well as bus read/write.
> - External Module Layer: This layer used for support more features
>   such as firmware update, debug tools and gesture wakeup.

I think that the driver should be split up in multiple patches. The
first one should handle the basics for the device (I'd say parity with
the driver for the older models would be fine), then add the various
features on top (firmware loading, gesture wakeup, etc.) as those
features probably need more thought as to what the external APIs should
be.

> Signed-off-by: Wang Yafei <wangyafei@goodix.com>
> ---
> Goodix driver that the kernel already have, is for our gt9xx series
> touchscreen controllers. This new driver is for out new generation
> touchscreen controllers, consider this two generations are very
> different with each other. We wrote a new driver instead of make
> a patch on the old driver.

You'll probably need a preparatory patch which makes changes to the
description and KConfig for the old driver, spelling out exactly what
the old driver supports, maybe renaming it.

You'll also need to decide whether the company is called GOODiX, GOODIX
or Goodix, whether the new devices are "sunrise", "GTx5" or something
else.

> Changes in v2:
>  - replace touchscreen properties according to the description in
>    Documentation/devicetree/bindings/input/touchscreen/touchscreen.tx
> t
>  - Droped all compat stuff for older kernels
>  - Removed Android stuff (EARLY_SUSPEND, CONFIG_FB)

There's still an ifdef with "COMPAT" in the name, not sure compat with
what.

>  - Use device_property_read_*  get device properties
>  - use get-unaligned_*() API
>  - Use dev_err() dev_dbg() for logging
>  - remove pinctrl functions
>  - remove some unused functions

Cheers