linux-kernel - Re: [PATCH] Add driver for GOODiX GTx5 series touchsereen

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170607063311.GB23434@dtor-ws>
Date:   Tue, 6 Jun 2017 23:33:11 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Wang Yafei <wangyafei@...dix.com>
Cc:     Henrik Rydberg <rydberg@...math.org>, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org, andrew@...dix.com, mouse@...dix.com
Subject: Re: [PATCH] Add driver for GOODiX GTx5 series touchsereen

Hi Wang,

On Wed, Jun 07, 2017 at 12:05:51PM +0800, Wang Yafei wrote:
> This driver is for GOODiX GTx5 series touchscreen controllers
> such as GT8589, GT7589. This driver designed with hierarchial 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.


What is the difference between this version and the version you posted a
couple of days ago?

A few high level comments/questions:

- is this for a different chip that what is handled by the Goodix driver
  that we already have in the kernel?

- you should be using standard touchscreen bindings described in
  Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

- please use gpiod API without gpio fallbacks: you are submitting the
  driver for inclusion into mainline that does have gpiod API and for
  mainline it does not make sense to use fallbacks.

- similarly drop other compat stuff for older kernels

- please drop Android stiff (EARLY_SUSPEND, ets).

- use generic device properties (device_property_read_*()) instead of
  OF-specific variants.

- use get_unaligned_*() API to convert data on wire to CPU format
  instead of doing the same by hand.

- use dev_err(), dev_dbg(), etc for logging.

Thanks.

-- 
Dmitry