Re: [PATCH] driver: input: touchscreen: add Raydium i2c touchscreen driver

[Dmitry Torokhov]

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@xxxxxxxxx>
> > 
> > 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@xxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/