On 11/3/2010 9:55 PM, Ken Lierman wrote:
since many people seem to be very interested in this one I'll take a
deeper look.
From: Ulf Samuelsson<u...@atmel.com>
Includes initial driver from vendor, plus self test support,
cleanup, and integration.
Please include the following the the default config for handset:
CONFIG_TOUCHSCREEN_ATMEL_MAXTOUCH=y
CONFIG_TOUCHSCREEN_ATMEL_MAXTOUCH_BL=y
Signed-off-by: Ken Lierman<ken.lier...@windriver.com>
Approval from GPL release to meego provided by:
Signed-off-by: Jon Kiachian<jon.kiach...@atmel.com>
well that's not needed since there already is an open source driver for
this...
nobody can object to that and put additional restrictions in place for
that while also doing another GPL driver...
first of all, your driver violates rule 2.11 (must enable hardware
efficiently... eg in existing drivers when possible), but you knew that
alreadydiff --git a/drivers/input/touchscreen/Kconfig
b/drivers/input/touchscreen/Kconfig
you're also late on your "latest kernel" submission (rule 1.2)
+config TOUCHSCREEN_ATMEL_MAXTOUCH_BL
+ tristate "Atmel maXTouch bootloader"
+ depends on I2C
+ help
+ Say Y here if you have an Atmel maXTouch touchscreen and you
+ want to enable the bootloader functionality.
this is not very useful.. nobody will know if to say yes or no.
+choice
+ prompt "maXTouch I2C port"
+ default TOUCHSCREEN_ATMEL_MAXTOUCH_I2C2
+ depends on TOUCHSCREEN_ATMEL_MAXTOUCH
+ depends on ARCH_OMAP35XX
+config TOUCHSCREEN_ATMEL_MAXTOUCH_I2C0
+ depends on !ARCH_OMAP35XX
+ bool "I2C Bus #0"
+config TOUCHSCREEN_ATMEL_MAXTOUCH_I2C1
+ depends on !ARCH_OMAP35XX
+ bool "I2C Bus #1"
+config TOUCHSCREEN_ATMEL_MAXTOUCH_I2C2
+ depends on ARCH_OMAP35XX
+ bool "I2C Bus #2"
+config TOUCHSCREEN_ATMEL_MAXTOUCH_I2C3
+ depends on ARCH_OMAP35XX
+ bool "I2C Bus #3"
+endchoice
these all don't really make sense.diff --git
a/drivers/input/touchscreen/atmel_maxtouch.c
b/drivers/input/touchscreen/atmel_maxtouch.c
+#include <linux/atmel_maxtouch.h>
it's kinda obscene to put a driver private header in include/linux ...
+
+/* Routines for memory access within a 16 bit address space */
+
+static int mxt_read_block(struct i2c_client *client, u16 addr, u16 length,
+ u8 *value);
+static int mxt_write_byte(struct i2c_client *client, u16 addr, u8 value);
+static int mxt_write_block(struct i2c_client *client, u16 addr, u16 length,
+ u8 *value);
if you had the right ordering in the driver you would not need static
prototypes like this.
+
+#define DRIVER_VERSION "0.9a"
+
+static int debug = DEBUG_INFO;
+static int comms;
+module_param(debug, int, 0644);
+module_param(comms, int, 0644);
+
+MODULE_PARM_DESC(debug, "Activate debugging output");
+MODULE_PARM_DESC(comms, "Select communications mode");
it's customary to keep all the pieces of one module parameter together...
+
+/* Device Info descriptor */
+/* Parsed from maXTouch "Id information" inside device */
+struct mxt_device_info {
+ u8 family_id;
+ u8 variant_id;
+ u8 major;
+ u8 minor;
+ u8 build;
+ u8 num_objs;
+ u8 x_size;
+ u8 y_size;
+ char family_name[16]; /* Family name */
+ char variant_name[16]; /* Variant name */
+ u16 num_nodes; /* Number of sensor nodes */
+};
are these 2 strings truely 0 terminated in the device?
+/* Returns the start address of object in mXT memory. */
+#define MXT_BASE_ADDR(object_type, mxt)
\
+ get_object_address(object_type, 0, mxt->object_table, \
+ mxt->device_info.num_objs)
+
+/* Maps a report ID to an object type (object type number). */
+#define REPORT_ID_TO_OBJECT(rid, mxt) \
+ (((rid) == 0xff) ? 0 : mxt->rid_map[rid].object)
+
+/* Maps a report ID to an object type (string). */
+#define REPORT_ID_TO_OBJECT_NAME(rid, mxt) \
+ object_type_name[REPORT_ID_TO_OBJECT(rid, mxt)]
+
+/* Returns non-zero if given object is a touch object */
+#define IS_TOUCH_OBJECT(object) \
+ ((object == MXT_TOUCH_MULTITOUCHSCREEN_T9) || \
+ (object == MXT_TOUCH_KEYARRAY_T15) || \
+ (object == MXT_TOUCH_PROXIMITY_T23) || \
+ (object == MXT_TOUCH_SINGLETOUCHSCREEN_T10) || \
+ (object == MXT_TOUCH_XSLIDER_T11) || \
+ (object == MXT_TOUCH_YSLIDER_T12) || \
+ (object == MXT_TOUCH_XWHEEL_T13) || \
+ (object == MXT_TOUCH_YWHEEL_T14) || \
+ (object == MXT_TOUCH_KEYSET_T31) || \
+ (object == MXT_TOUCH_XSLIDERSET_T32) ? 1 : 0)
these macros seem rather misplaced; this last one should be an inline,
the others are so trivial wrappers that they likely are better off not
being there atall
+
+#define mxt_debug(level, ...) \
+ do { \
+ if (debug>= (level)) \
+ pr_debug(__VA_ARGS__); \
+ } while (0)
and now you violate rule 2.6: Kernel code must use the in-kernel debug
macros
this is not a useful wrapper around pr_debug() since there already is
runtime control for pr_debug().. you just duplicated it.
+#ifdef ABS_MT_TRACKING_ID
who sets this, and when ??
if it's always set, can we just not have this ifdef ?
+static inline void report_gesture(int data, struct mxt_data *mxt)
+{
+ input_event(mxt->input, EV_MSC, MSC_GESTURE, data);
+}
a one line inline? that's such a trivial wrapper that you just are
better off open coded it.
+
+
+static const u8 *object_type_name[] = {
+ [0] = "Reserved",
+ [5] = "GEN_MESSAGEPROCESSOR_T5",
+ [6] = "GEN_COMMANDPROCESSOR_T6",
+ [7] = "GEN_POWERCONFIG_T7",
+ [8] = "GEN_ACQUIRECONFIG_T8",
+ [9] = "TOUCH_MULTITOUCHSCREEN_T9",
+ [15] = "TOUCH_KEYARRAY_T15",
+ [17] = "SPT_COMMSCONFIG_T18",
+ [19] = "SPT_GPIOPWM_T19",
+ [20] = "PROCI_GRIPFACESUPPRESSION_T20",
+ [22] = "PROCG_NOISESUPPRESSION_T22",
+ [23] = "TOUCH_PROXIMITY_T23",
+ [24] = "PROCI_ONETOUCHGESTUREPROCESSOR_T24",
+ [25] = "SPT_SELFTEST_T25",
+ [27] = "PROCI_TWOTOUCHGESTUREPROCESSOR_T27",
+ [28] = "SPT_CTECONFIG_T28",
+ [37] = "DEBUG_DIAGNOSTICS_T37",
+ [38] = "SPT_USER_DATA_T38",
+ [40] = "PROCI_GRIPSUPPRESSION_T40",
+ [41] = "PROCI_PALMSUPPRESSION_T41",
+ [42] = "PROCI_FACESUPPRESSION_T42",
+ [43] = "SPT_DIGITIZER_T43",
+ [44] = "SPT_MESSAGECOUNT_T44",
+};
hmm what are these used for? do these really have to live in kernel space??
+
+
+static u16 get_object_address(uint8_t object_type,
+ uint8_t instance,
+ struct mxt_object *object_table,
+ int max_objs);
+
+static int mxt_write_ap(struct mxt_data *mxt, u16 ap);
+
+static int mxt_read_block_wo_addr(struct i2c_client *client,
+ u16 length,
+ u8 *value);
hmm more prototypes... didn't you already have teh prototypes way up in
this driver?
(and you should not need them)
+
+int debug_data_open(struct inode *inode, struct file *file)
+{
+ struct mxt_data *mxt;
+ int i;
+ mxt = inode->i_private;
+ if (mxt == NULL)
+ return -EIO;
how does this condition ever not become true ?
have you tested this code personally ?
++ssize_t mxt_memory_read(struct file *file, char *buf, size_t count,
+ loff_t *ppos)
+{
+ int i;
+ struct mxt_data *mxt;
+ mxt = file->private_data;
not checking for NULL here? you do in all other cases
+static int mxt_ioctl(struct inode *inode, struct file *file,
great, a custom userspace interface.
but you're not using the existing interface (rule 2.14)
what is the result of the upstream review of this new userspace ABI ?
where was this discussed ?
+ unsigned int cmd, unsigned long arg)
+{
+ int retval;
+ struct mxt_data *mxt;
+ u8 data[6];
+
+ retval = 0;
+ mxt = file->private_data;
+
+ switch (cmd) {
+ case MXT_SET_ADDRESS_IOCTL:
+ retval = mxt_write_ap(mxt, (u16) arg);
+ if (retval>= 0) {
+ mxt->address_pointer = (u16) arg;
+ mxt->valid_ap = 1;
+ }
+ break;
this one looks like it really wants a security check, or should anyone
just be able to set this to ay value?
there's no range checking or sanity checking done on "arg"
+ default:
+ return -EIO;
-EIO is not a valid return code for unknown ioctls.
+
+
+/* Writes one byte to given address in mXT chip. */
+
+static int mxt_write_byte(struct i2c_client *client, u16 addr, u8 value)
+{
+ struct {
+ __le16 le_addr;
+ u8 data;
+
+ } i2c_byte_transfer;
hmm struct definitiosn private to a function... that's almost always a bug.
and it's not even packed, yet you seem to try to send this thing over
the wire. Bad idea.
+
+
+
+
+/*
+ * The maXTouch device will signal the host about a new message by asserting
+ * the CHG line. This ISR schedules a worker routine to read the message when
+ * that happens.
+ */
+
+static irqreturn_t mxt_irq_handler(int irq, void *_mxt)
+{
+ struct mxt_data *mxt = _mxt;
+
+ mxt->irq_counter++;
+ if (mxt->valid_interrupt()) {
+ /* Send the signal only if falling edge generated the irq. */
+ cancel_delayed_work(&mxt->dwork);
+ schedule_delayed_work(&mxt->dwork, 0);
this really wants to use threaded interrupts instead.
much better fit for this device driver.
+static int __devinit mxt_read_object_table(struct i2c_client *client,
+ struct mxt_data *mxt,
+ u8 *raw_id_data)
+{
+ u16 report_id_count;
+ u8 buf[MXT_OBJECT_TABLE_ELEMENT_SIZE];
how big is this? are you blowing your stack this way?++ if (mxt->irq) {
+ /* Try to request IRQ with falling edge first. This is
+ * not always supported. If it fails, try with any edge. */
+ error = request_irq(mxt->irq,
+ mxt_irq_handler,
+ IRQF_TRIGGER_FALLING,
+ client->dev.driver->name,
+ mxt);
+ if (error< 0) {
+ /* TODO: why only 0 works on STK1000? */
+ error = request_irq(mxt->irq,
+ mxt_irq_handler,
+ 0,
+ client->dev.driver->name,
+ mxt);
+ }
needs to be a shared interrupt at minimum
also from this point on you can actually get an interrupt...
+
+ if (error< 0) {
+ dev_err(&client->dev,
+ "failed to allocate irq %d\n", mxt->irq);
+ goto err_irq;
+ }
+ }
+
+ if (debug> DEBUG_INFO)
+ dev_info(&client->dev, "touchscreen, irq %d\n", mxt->irq);
+
+ /* Store the number of multi-touches allowed, default was 0. */
+ mxt->numtouch = pdata->numtouch;
... but you're still filling in data structures.
usually a bad idea.
+ class_destroy(mxt->mxt_class);
hnmmmmm
+#if defined(CONFIG_PM)
+static int mxt_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+ struct mxt_data *mxt = i2c_get_clientdata(client);
+
+ if (device_may_wakeup(&client->dev))
+ enable_irq_wake(mxt->irq);
+
+ return 0;
+}
that's not doing much
(but we'll never call this anyway)
It would be nice if there was actual powermanagement via device runtime
in this driver though.
_______________________________________________
MeeGo-kernel mailing list
MeeGo-kernel@lists.meego.com
http://lists.meego.com/listinfo/meego-kernel
