linux-kernel - Re: [PATCH] driver: input: touchscreen: add Raydium i2c touchscreen driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141119180639.GD37989@dtor-ws>
Date:	Wed, 19 Nov 2014 10:06:39 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Daniel Mack <daniel@...que.org>
Cc:	"jeffrey.lin" <yajohn@...il.com>, rydberg@...omail.se,
	bleung@...omium.org, dh.herrmann@...il.com,
	charliemooney@...omium.org, floe@...terbrot.org,
	jeffrey.lin@...-ic.com, roger.yang@...-ic.com, KP.li@...-ic.com,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	"jeffrey.lin" <jeffrey.lin@...il.com>
Subject: Re: [PATCH] driver: input: touchscreen:  add Raydium i2c touchscreen
 driver

On Wed, Nov 19, 2014 at 05:44:48PM +0100, Daniel Mack wrote:
> On 11/19/2014 04:30 PM, jeffrey.lin wrote:
> > From: "jeffrey.lin" <jeffrey.lin@...il.com>
> > 
> > this patch is porting Raydium I2C touch driver. Developer can enable
> > raydium touch driver by modifying define "CONFIG_TOUCHSCREEN_RM_TS".
> > 
> > BUG: None
> > TEST: built and test with peppy
> > 
> > Signed-off-by: jeffrey.lin@...-ic.com
> > Change-Id: I05d54e5ef29249d2a6eae97222c90bed11e839f9
> > ---
> 
> Just some general remarks that IMO need to be addressed before a more
> thorough review can happen:
> 
> 
> * Please remove all dead code, such as code in comments and in #if 0
>   blocks.
> 
> * Use the regmap framework to abstract the hardware access on the I2C
>   layer (see drivers/input/keyboard/cap1106.c and many other drivers
>   as an example, and check include/linux/regmap.h). That makes the code
>   a lot shorter and more comprehensible to read.
> 
> * By using request_threaded_irq(), you don't have to manually control
>   your worker and can simplify your code quite a bit.
> 
> * See if you can claim all the resources the driver needs by using
>   devm_* variants.
> 
> * Don't use uppercase names for filenames, structs, functions and IDs
> 
> * Why do you need a miscdevice for this? Isn't the input event layer
>   enough?
> 
> * Also, run scripts/checkpatch.pl on the new patch, which will help
>   you find some more typical pitfalls.

Also please convert driver to MT-B (slotted) protocol (you are currently
using MT-Ai version).

And please remove join_bytes(). It is called get_unaligned_le16().

Most of the things that need fixing have already been listed in response
to your original submission, please do address them before posting the
driver again.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/