Re: [PATCH] driver: input: touchscreen: add Raydium i2c touchscreen driver
HI Jeffrey,
On Mon, Dec 01, 2014 at 06:17:11PM +0800, jeffrey.lin wrote:
> From: "jeffrey.lin"
>
> This patch is porting Raydium I2C touch driver. Developer can enable
> raydium touch driver by modifying define "CONFIG_TOUCHSCREEN_RM_TS".
Thank you for making the changes requested earlier.
>
> Change-Id: I5f33cfdf0e895de6e7d535c11dd4b3ce8b49fa48
> Signed-off-by: jeffrey@rad-ic.com
> ---
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/rm31100_ts.c | 968
> +
> include/linux/input/rm31100_ts.h | 60 ++
> 4 files changed, 1041 insertions(+)
> create mode 100644 drivers/input/touchscreen/rm31100_ts.c
> create mode 100644 include/linux/input/rm31100_ts.h
>
> diff --git a/drivers/input/touchscreen/Kconfig
> b/drivers/input/touchscreen/Kconfig
> index 3ce9181..d0324d2 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -955,4 +955,16 @@ config TOUCHSCREEN_ZFORCE
> To compile this driver as a module, choose M here: the
> module will be called zforce_ts.
>
> +config TOUCHSCREEN_RM_TS
> + tristate "Raydium I2C Touchscreen"
> + depends on I2C
> + help
> + Say Y here if you have Raydium series I2C touchscreen,
> + such as RM31100 , connected to your system.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called rm31100_ts.
> +
> endif
> diff --git a/drivers/input/touchscreen/Makefile
> b/drivers/input/touchscreen/Makefile
> index 687d5a7..aae4af2 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE) +=
> zylonite-wm97xx.o
> obj-$(CONFIG_TOUCHSCREEN_W90X900)+= w90p910_ts.o
> obj-$(CONFIG_TOUCHSCREEN_TPS6507X) += tps6507x-ts.o
> obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_RM_TS) += rm31100_ts.o
> diff --git a/drivers/input/touchscreen/rm31100_ts.c
> b/drivers/input/touchscreen/rm31100_ts.c
> new file mode 100644
> index 000..6cb09a4
> --- /dev/null
> +++ b/drivers/input/touchscreen/rm31100_ts.c
> @@ -0,0 +1,968 @@
> +/* Source for:
> + * Raydium rm31100_ts Prototype touchscreen driver.
> + * drivers/input/touchscreen/rm31100_ts.c
No need for the file name. And we know it is a source. So just say:
Raydium RM31100 touchscreen driver.
Why does it say it's a prototype?
> + *
> + * Copyright (C) 2012,
Whose copyright is this?
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2, and only version 2, as published by the
> + * Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Raydium reserves the right to make changes without further notice
> + * to the materials described herein. Raydium does not assume any
> + * liability arising out of the application described herein.
> + *
> + * Contact Raydium Semiconductor Corporation at www.rad-ic.com
> + *
> + * History:
> + * (C) 2012 Raydium - Update for GPL distribution
> + * (C) 2009 Enea - Original prototype
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
We are not using workier anymore, please remove.
> +#include
> +#include
> +#include
> +#include
> +#include
> +/*#include */
> +#ifdef CONFIG_MISC_DEV
> +#include
> +#endif
> +/*#include copy_to_user() */
> +#include
> +
> +#define rm31100 0x0
> +#define rm3110x 0x1
> +
> +#define INVALID_DATA 0xff
> +#define MAX_REPORT_TOUCHED_POINTS10
> +
> +#define TOUCHSCREEN_TIMEOUT (msecs_to_jiffies(10))
Does not seem to be used.
> +#define INITIAL_DELAY(msecs_to_jiffies(25000))
Does not seem to be used.
> +
> +#define EVAL_REPORT_RATE1
> +
> +#define I2C_CLIENT_ADDR 0x39
> +#define I2C_DMA_CLIENT_ADDR 0x5A
> +#undef CONFIG_PM
What on earth is this
> +struct rm31100_ts_data {
> + u8 x_index;
> + u8 y_index;
> + u8 z_index;
> + u8 id_index;
> + u8 touch_index;
> + u8 data_reg;
> + u8 status_reg;
> + u8 data_size;
> + u8 touch_bytes;
> + u8 update_data;
> + u8 touch_meta_data;
> + u8 finger_size;
> +};
> +
> +static struct rm31100_ts_data devices[] = {
> + [0] = {
> + .x_index = 2,
> + .y_index = 4,
> + .z_index = 6,
> + .id_index = 1,
> + .data_reg = 0x1,
> + .status_reg = 0,
> + .update_data = 0x0,/*0x4*/
>
Re: [PATCH] driver: input: touchscreen: add Raydium i2c touchscreen driver
On Mon, Dec 1, 2014 at 2:17 AM, jeffrey.lin wrote: > From: "jeffrey.lin" > > This patch is porting Raydium I2C touch driver. Developer can enable > raydium touch driver by modifying define "CONFIG_TOUCHSCREEN_RM_TS". > > Change-Id: I5f33cfdf0e895de6e7d535c11dd4b3ce8b49fa48 Once again, please remove this bit of Gerrit change-id from your patch submissions in the future. > Signed-off-by: jeffrey@rad-ic.com -- Benson Leung Software Engineer, Chrom* OS ble...@chromium.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] driver: input: touchscreen: add Raydium i2c touchscreen driver
On Wed, Nov 19, 2014 at 7:30 AM, jeffrey.lin wrote: > From: "jeffrey.lin" Also, this email address appears to not be active. Please do not list dead email addresses as the author. > > 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@rad-ic.com > Change-Id: I05d54e5ef29249d2a6eae97222c90bed11e839f9 > --- -- Benson Leung Software Engineer, Chrom* OS ble...@chromium.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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" > > > > 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@rad-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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] driver: input: touchscreen: add Raydium i2c touchscreen driver
Hi Jeffrey, On Wed, Nov 19, 2014 at 7:30 AM, jeffrey.lin wrote: > From: "jeffrey.lin" > > 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 These two lines serve no purpose in the upstream kernel. Please remove them. > > Signed-off-by: jeffrey@rad-ic.com > Change-Id: I05d54e5ef29249d2a6eae97222c90bed11e839f9 The Change-Id: line is an artifact of the gerrit code review system. Please remove it before sending patches upstream. -- Benson Leung Software Engineer, Chrom* OS ble...@chromium.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] driver: input: touchscreen: add Raydium i2c touchscreen driver
On 11/19/2014 04:30 PM, jeffrey.lin wrote: > From: "jeffrey.lin" > > 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@rad-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. Thanks, Daniel > drivers/input/touchscreen/Kconfig | 12 + > drivers/input/touchscreen/Makefile | 1 + > drivers/input/touchscreen/RM31100.c | 972 > > include/linux/input/RM31100.h | 60 +++ > 4 files changed, 1045 insertions(+) > create mode 100644 drivers/input/touchscreen/RM31100.c > create mode 100644 include/linux/input/RM31100.h > > diff --git a/drivers/input/touchscreen/Kconfig > b/drivers/input/touchscreen/Kconfig > index 3ce9181..b692036 100644 > --- a/drivers/input/touchscreen/Kconfig > +++ b/drivers/input/touchscreen/Kconfig > @@ -98,6 +98,18 @@ config TOUCHSCREEN_ATMEL_MXT > To compile this driver as a module, choose M here: the > module will be called atmel_mxt_ts. > > +config TOUCHSCREEN_RM_TS > + tristate "Raydium I2C Touchscreen" > + depends on I2C > + help > + Say Y here if you have Raydium series I2C touchscreen, > + such as RM31100 , connected to your system. > + > + If unsure, say N. > + > + To compile this driver as a module, choose M here: the > + module will be called RM31100. > + > config TOUCHSCREEN_AUO_PIXCIR > tristate "AUO in-cell touchscreen using Pixcir ICs" > depends on I2C > diff --git a/drivers/input/touchscreen/Makefile > b/drivers/input/touchscreen/Makefile > index 687d5a7..d991815 100644 > --- a/drivers/input/touchscreen/Makefile > +++ b/drivers/input/touchscreen/Makefile > @@ -73,6 +73,7 @@ wm97xx-ts-$(CONFIG_TOUCHSCREEN_WM9705) += wm9705.o > wm97xx-ts-$(CONFIG_TOUCHSCREEN_WM9712) += wm9712.o > wm97xx-ts-$(CONFIG_TOUCHSCREEN_WM9713) += wm9713.o > obj-$(CONFIG_TOUCHSCREEN_WM97XX_ATMEL) += atmel-wm97xx.o > +obj-$(CONFIG_TOUCHSCREEN_RM_TS) += RM31100.o > obj-$(CONFIG_TOUCHSCREEN_WM97XX_MAINSTONE) += mainstone-wm97xx.o > obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE)+= zylonite-wm97xx.o > obj-$(CONFIG_TOUCHSCREEN_W90X900)+= w90p910_ts.o > diff --git a/drivers/input/touchscreen/RM31100.c > b/drivers/input/touchscreen/RM31100.c > new file mode 100644 > index 000..78cc80d > --- /dev/null > +++ b/drivers/input/touchscreen/RM31100.c > @@ -0,0 +1,972 @@ > +/* Source for: > + * Raydium RM31100 Prototype touchscreen driver. > + * drivers/input/touchscreen/RM31100.c > + * > + * Copyright (C) 2012, > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * version 2, and only version 2, as published by the > + * Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Raydium reserves the right to make changes without further notice > + * to the materials described herein. Raydium does not assume any > + * liability arising out of the application described herein. > + * > + * Contact Raydium Semiconductor Corporation at www.rad-ic.com > + * > + * History: > + * (C) 2012 Raydium - Update for GPL distribution > + * (C) 2009 Enea - Original prototype > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +/*#include */ > +#include > +/*#include copy_to_user() */ > +#include > + > + > +#if 0/*defined(CONFIG_HAS_EARLYSUSPEND)*/ > +#include > + > +/* Early-suspend level */ > +#define RM31100_TS_SUSPEND_LEVEL 1 > +#endif > + > +#define RM31100 0x
