i2c: cyttsp i2c and spi touchscreen driver init submit
in [Kernel]
|
Prev: drivers/media/video/bt8xx: Adjust confusing if indentation
Next: drivers/media/video/bt8xx: Adjust confusing if indentation
From: Dmitry Torokhov on 5 Aug 2010 17:10 On Fri, Aug 06, 2010 at 02:15:44AM +0530, Trilok Soni wrote: > > + > > +/* ************************************************************************ > > + * The cyttsp_xy_worker function reads the XY coordinates and sends them to > > + * the input layer. It is scheduled from the interrupt (or timer). > > + * *************************************************************************/ > > +void handle_single_touch(struct cyttsp_xydata *xy, struct cyttsp_track_data *t, > > + struct cyttsp *ts) > > +{ > > functions should be "static". I would leave a decision to Dmitry if he wants the driver > to support old single touch protocol hacked up with HAT_xx bits so that driver can support > two touches with the old single touch protocol itself. The ABS_HAT* bits should go away. They were gross abuse even when we did not have MT protocol and shoudl not be used at all now that we do have it. I will be cleaning up older drivers (like Elantech) to get rid of them. Multitouch devices shouls support either A or B MT protocol. SInce this device seems to be supporting tracking in hardware it shoudl simply adhere to protocol B to limit kernel->userspace traffict and be done with it. .... > > + > > +/* schedulable worker entry for timer polling method */ > > +void cyttsp_xy_worker_(struct work_struct *work) > > +{ > > + struct cyttsp *ts = container_of(work, struct cyttsp, work); > > + cyttsp_xy_worker(ts); > > +} > > I would really prefer that you remove the polling method from this code as it will simplify > a code lot. We can delete the whole workqueue because as you will be using request_threaded_irq(...), > you will not need any workqueue. > Seconded. Polling is hardly useful on real production setup as it will drain battery in no time. > > + > > +static int cyttsp_i2c_init(void) > > +{ > > Please add __init like > > static int __init cyttsp_i2c_init(void) > > > + int retval; > > + retval = i2c_add_driver(&cyttsp_i2c_driver); > > + printk(KERN_INFO "%s: Cypress TrueTouch(R) Standard Product I2C " > > + "Touchscreen Driver (Built %s @ %s) returned %d\n", > > + __func__, __DATE__, __TIME__, retval); > > + And lose printk as well. The boot is exremely noisy as it is... return i2c_add_driver(&cyttsp_i2c_driver); is all that is needed. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Henrik Rydberg on 5 Aug 2010 19:10 On 08/05/2010 08:12 PM, Kevin McNeely wrote: > From: Fred <fwk(a)ubuntu.linuxcertified.com> > > This is a new touchscreen driver for the Cypress Semiconductor > cyttsp family of devices. This updated driver is for both the i2c and spi > versions of the devices. The driver code consists of common core code for > both i2c and spi driver. This submission is in response to review comments > from the initial submission. > > Signed-off-by: Kevin McNeely <kev(a)cypress.com> > --- General impression: There is a lot of useful code in here, but as already pointed out, well over half of it should not be part of the kernel driver. Suggestion 1: Clean out the internal state code and use MT protocol B instead. Suggestion 2: Cut out the single touch calculation, as already pointed out by Trilok. Why not submit it to the mtdev project instead? The problem is generic to all new MT drivers, so a common solution while waiting for full MT support in userspace could be useful. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Trilok Soni on 6 Aug 2010 05:10 Hi Kevin, Review for SPI code. > diff --git a/drivers/input/touchscreen/cyttsp_spi.c b/drivers/input/touchscreen/cyttsp_spi.c > new file mode 100755 > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> > +#include <linux/delay.h> > +#include <linux/cyttsp.h> > +#include "cyttsp_core.h" > + > +#define DBG(x) > + > +#define CY_SPI_WR_OP 0x00 /* r/~w */ > +#define CY_SPI_RD_OP 0x01 > +#define CY_SPI_CMD_BYTES 4 > +#define CY_SPI_SYNC_BYTES 2 > +#define CY_SPI_SYNC_ACK1 0x62 /* from protocol v.2 */ > +#define CY_SPI_SYNC_ACK2 0x9D /* from protocol v.2 */ > +#define CY_SPI_SYNC_NACK 0x69 > +#define CY_SPI_DATA_SIZE 64 > +#define CY_SPI_DATA_BUF_SIZE (CY_SPI_CMD_BYTES + CY_SPI_DATA_SIZE) > +#define CY_SPI_BITS_PER_WORD 8 > + > +struct cyttsp_spi { > + struct cyttsp_bus_ops ops; > + struct spi_device *spi_client; > + void *ttsp_client; > + u8 wr_buf[CY_SPI_DATA_BUF_SIZE]; > + u8 rd_buf[CY_SPI_DATA_BUF_SIZE]; > +}; > + > +static void spi_complete(void *arg) > +{ > + complete(arg); We don't need anything here on completion? > +} > + > +static int spi_sync_tmo(struct spi_device *spi, struct spi_message *message) > +{ > + DECLARE_COMPLETION_ONSTACK(done); > + int status; > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) No need. > + > + message->complete = spi_complete; > + message->context = &done; > + status = spi_async(spi, message); > + if (status == 0) { > + int ret = wait_for_completion_interruptible_timeout(&done, HZ); > + if (!ret) { > + printk(KERN_ERR "%s: timeout\n", __func__); > + status = -EIO; > + } else > + status = message->status; > + } > + message->context = NULL; > + return status; > +} > + > +static int cyttsp_spi_xfer_(u8 op, struct cyttsp_spi *ts_spi, > + u8 reg, u8 *buf, int length) > +{ > + struct spi_message msg; > + struct spi_transfer xfer = { 0 }; > + u8 *wr_buf = ts_spi->wr_buf; > + u8 *rd_buf = ts_spi->rd_buf; > + int retval; > + int i; > + DBG(printk(KERN_INFO "%s: Enter\n", __func__);) No need. > + > + if (length > CY_SPI_DATA_SIZE) { > + printk(KERN_ERR "%s: length %d is too big.\n", > + __func__, length); > + return -EINVAL; > + } > + DBG(printk(KERN_INFO "%s: OP=%s length=%d\n", __func__, > + op == CY_SPI_RD_OP ? "Read" : "Write", length);) dev_dbg if really needed. > + > + wr_buf[0] = 0x00; /* header byte 0 */ > + wr_buf[1] = 0xFF; /* header byte 1 */ > + wr_buf[2] = reg; /* reg index */ > + wr_buf[3] = op; /* r/~w */ > + if (op == CY_SPI_WR_OP) > + memcpy(wr_buf + CY_SPI_CMD_BYTES, buf, length); > + DBG( > + if (op == CY_SPI_RD_OP) > + memset(rd_buf, CY_SPI_SYNC_NACK, CY_SPI_DATA_BUF_SIZE);) > + DBG( > + for (i = 0; i < (length + CY_SPI_CMD_BYTES); i++) { > + if ((op == CY_SPI_RD_OP) && (i < CY_SPI_CMD_BYTES)) > + printk(KERN_INFO "%s: wr[%d]:0x%02x\n", > + __func__, i, wr_buf[i]); > + if (op == CY_SPI_WR_OP) > + printk(KERN_INFO "%s: wr[%d]:0x%02x\n", > + __func__, i, wr_buf[i]); > + }) Let remove such things. > + > + xfer.tx_buf = wr_buf; > + xfer.rx_buf = rd_buf; > + xfer.len = length + CY_SPI_CMD_BYTES; > + > + if ((op == CY_SPI_RD_OP) && (xfer.len < 32)) > + xfer.len += 1; > + > + spi_message_init(&msg); > + spi_message_add_tail(&xfer, &msg); > + retval = spi_sync_tmo(ts_spi->spi_client, &msg); > + if (retval < 0) { > + printk(KERN_ERR "%s: spi_sync_tmo() error %d\n", > + __func__, retval); > + retval = 0; > + } > + if (op == CY_SPI_RD_OP) { > + DBG( > + for (i = 0; i < (length + CY_SPI_CMD_BYTES); i++) > + printk(KERN_INFO "%s: rd[%d]:0x%02x\n", > + __func__, i, rd_buf[i]);) > + > + for (i = 0; i < (length + CY_SPI_CMD_BYTES - 1); i++) { > + if ((rd_buf[i] != CY_SPI_SYNC_ACK1) || > + (rd_buf[i + 1] != CY_SPI_SYNC_ACK2)) { > + continue; > + } > + if (i <= (CY_SPI_CMD_BYTES - 1)) { > + memcpy(buf, (rd_buf + i + CY_SPI_SYNC_BYTES), > + length); > + return 0; > + } > + } > + DBG(printk(KERN_INFO "%s: byte sync error\n", __func__);) dev_err if you really need to print for debugging purpose or pr_err(...) > + retval = 1; > + } > + return retval; > +} > + > +static int cyttsp_spi_xfer(u8 op, struct cyttsp_spi *ts, > + u8 reg, u8 *buf, int length) > +{ > + int tries; > + int retval; > + DBG(printk(KERN_INFO "%s: Enter\n", __func__);) No need. > + > + if (op == CY_SPI_RD_OP) { > + for (tries = CY_NUM_RETRY; tries; tries--) { > + retval = cyttsp_spi_xfer_(op, ts, reg, buf, length); > + if (retval == 0) > + break; > + else > + msleep(10); > + } > + } else { > + retval = cyttsp_spi_xfer_(op, ts, reg, buf, length); > + } > + return retval; > +} > + > +static s32 ttsp_spi_read_block_data(void *handle, u8 addr, > + u8 length, void *data) > +{ > + int retval; > + struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, ops); > + > + DBG(printk(KERN_INFO "%s: Enter\n", __func__);) No need. > + > + retval = cyttsp_spi_xfer(CY_SPI_RD_OP, ts, addr, data, length); > + if (retval < 0) > + printk(KERN_ERR "%s: ttsp_spi_read_block_data failed\n", > + __func__); dev_err(...) > + > + /* Do not print the above error if the data sync bytes were not found. > + This is a normal condition for the bootloader loader startup and need > + to retry until data sync bytes are found. */ Use kernel style for multi-line comments. /* * You should use this format * for multi-line commenting */ > + if (retval > 0) > + retval = -1; /* now signal fail; so retry can be done */ > + > + return retval; > +} > + > +static s32 ttsp_spi_write_block_data(void *handle, u8 addr, > + u8 length, const void *data) > +{ > + int retval; > + struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, ops); > + > + DBG(printk(KERN_INFO "%s: Enter\n", __func__);) No need. > + > + retval = cyttsp_spi_xfer(CY_SPI_WR_OP, ts, addr, (void *)data, length); > + if (retval < 0) > + printk(KERN_ERR "%s: ttsp_spi_write_block_data failed\n", > + __func__); > + > + if (retval == -EIO) > + return 0; > + else > + return retval; > +} > + > +static s32 ttsp_spi_tch_ext(void *handle, void *values) > +{ > + int retval = 0; > + struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, ops); > + > + DBG(printk(KERN_INFO "%s: Enter\n", __func__);) No need. > + > + /* Add custom touch extension handling code here */ You may want to add this as "TODO". > + /* set: retval < 0 for any returned system errors, > + retval = 0 if normal touch handling is required, > + retval > 0 if normal touch handling is *not* required */ > + if (!ts || !values) > + retval = -EIO; > + > + return retval; > +} > + > +static int __devinit cyttsp_spi_probe(struct spi_device *spi) > +{ > + struct cyttsp_spi *ts_spi; > + int retval; > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) Un-necessary printk. > + > + /* Set up SPI*/ > + spi->bits_per_word = CY_SPI_BITS_PER_WORD; > + spi->mode = SPI_MODE_0; > + retval = spi_setup(spi); > + if (retval < 0) { > + printk(KERN_ERR "%s: SPI setup error %d\n", __func__, retval); dev_err(...) > + return retval; > + } > + ts_spi = kzalloc(sizeof(*ts_spi), GFP_KERNEL); > + if (ts_spi == NULL) { > + printk(KERN_ERR "%s: Error, kzalloc\n", __func__); dev_err(...) > + retval = -ENOMEM; > + goto error_alloc_data_failed; > + } > + ts_spi->spi_client = spi; > + dev_set_drvdata(&spi->dev, ts_spi); > + ts_spi->ops.write = ttsp_spi_write_block_data; > + ts_spi->ops.read = ttsp_spi_read_block_data; > + ts_spi->ops.ext = ttsp_spi_tch_ext; > + > + ts_spi->ttsp_client = cyttsp_core_init(&ts_spi->ops, &spi->dev); > + if (!ts_spi->ttsp_client) > + goto ttsp_core_err; > + printk(KERN_INFO "%s: Successful registration %s\n", > + __func__, CY_SPI_NAME); > + > + return 0; > + > +ttsp_core_err: > + kfree(ts_spi); > +error_alloc_data_failed: > + return retval; > +} > + > +/* registered in driver struct */ No need. > +static int __devexit cyttsp_spi_remove(struct spi_device *spi) > +{ > + struct cyttsp_spi *ts_spi = dev_get_drvdata(&spi->dev); > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) Remove this printk. > + cyttsp_core_release(ts_spi->ttsp_client); > + kfree(ts_spi); > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int cyttsp_spi_suspend(struct spi_device *spi, pm_message_t message) > +{ > + return cyttsp_suspend(dev_get_drvdata(&spi->dev)); > +} > + > +static int cyttsp_spi_resume(struct spi_device *spi) > +{ > + return cyttsp_resume(dev_get_drvdata(&spi->dev)); > +} > +#endif > + > +static struct spi_driver cyttsp_spi_driver = { > + .driver = { > + .name = CY_SPI_NAME, > + .bus = &spi_bus_type, > + .owner = THIS_MODULE, > + }, > + .probe = cyttsp_spi_probe, > + .remove = __devexit_p(cyttsp_spi_remove), > +#ifdef CONFIG_PM > + .suspend = cyttsp_spi_suspend, > + .resume = cyttsp_spi_resume, > +#endif > +}; > + > +static int __init cyttsp_spi_init(void) > +{ > + int err; > + > + err = spi_register_driver(&cyttsp_spi_driver); > + printk(KERN_INFO "%s: Cypress TrueTouch(R) Standard Product SPI " > + "Touchscreen Driver (Built %s @ %s) returned %d\n", > + __func__, __DATE__, __TIME__, err); As Dmitry mentioned on another e-mail, remove such printks. They create un-necessary noise. > + > + return err; > +} > +module_init(cyttsp_spi_init); > + > +static void __exit cyttsp_spi_exit(void) > +{ > + spi_unregister_driver(&cyttsp_spi_driver); > + printk(KERN_INFO "%s: module exit\n", __func__); Ditto. > +} > +module_exit(cyttsp_spi_exit); > + > +MODULE_ALIAS("spi:cyttsp"); > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Cypress TrueTouch(R) Standard Product (TTSP) SPI driver"); > +MODULE_AUTHOR("Cypress"); > + > diff --git a/include/linux/cyttsp.h b/include/linux/cyttsp.h > new file mode 100755 > index 0000000..b2a289b > --- /dev/null > +++ b/include/linux/cyttsp.h You may want to move this to include/linux/input/cyttsp.h ---Trilok Soni -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kevin McNeely on 6 Aug 2010 20:40 Hello Henrick, > -----Original Message----- > From: Henrik Rydberg [mailto:rydberg(a)euromail.se] > Sent: Thursday, August 05, 2010 4:07 PM > To: Kevin McNeely > Cc: Dmitry Torokhov; David Brown; Trilok Soni; Fred; Samuel Ortiz; Eric > Miao; Ben Dooks; Simtec Linux Team; Todd Fischer; Arnaud Patard; Sascha > Hauer; linux-input(a)vger.kernel.org; linux-kernel(a)vger.kernel.org > Subject: Re: [PATCH] i2c: cyttsp i2c and spi touchscreen driver init > submit > > On 08/05/2010 08:12 PM, Kevin McNeely wrote: > > > From: Fred <fwk(a)ubuntu.linuxcertified.com> > > > > This is a new touchscreen driver for the Cypress Semiconductor > > cyttsp family of devices. This updated driver is for both the i2c > and spi > > versions of the devices. The driver code consists of common core code > for > > both i2c and spi driver. This submission is in response to review > comments > > from the initial submission. > > > > Signed-off-by: Kevin McNeely <kev(a)cypress.com> > > --- > > > General impression: There is a lot of useful code in here, but as > already > pointed out, well over half of it should not be part of the kernel > driver. > > Suggestion 1: Clean out the internal state code and use MT protocol B > instead. > > Suggestion 2: Cut out the single touch calculation, as already pointed > out by > Trilok. Why not submit it to the mtdev project instead? The problem is > generic > to all new MT drivers, so a common solution while waiting for full MT > support in > userspace could be useful. I will remove the ST code and polling completely. However, I would like to keep the MT Protocol A. Our solution allows The platform builder to select to use MT protocol B or not as part of platform_data in the board configuration. If it makes more sense, I can reverse the code to default to protocol B and allow the platform builder developer to select protocol A. Thank you for the invitation to submit to the mtdev project. I defer to Dmitry if I should make the next update into the mtdev project. Thank you for your review. -kev > > Thanks, > Henrik --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kevin McNeely on 6 Aug 2010 20:50
Hello Dmitry, > -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov(a)gmail.com] > Sent: Thursday, August 05, 2010 2:07 PM > To: Trilok Soni > Cc: Kevin McNeely; David Brown; Fred; Samuel Ortiz; Eric Miao; Ben > Dooks; Simtec Linux Team; Todd Fischer; Arnaud Patard; Sascha Hauer; > Henrik Rydberg; linux-input(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org > Subject: Re: [PATCH] i2c: cyttsp i2c and spi touchscreen driver init > submit > > On Fri, Aug 06, 2010 at 02:15:44AM +0530, Trilok Soni wrote: > > > + > > > +/* > *********************************************************************** > * > > > + * The cyttsp_xy_worker function reads the XY coordinates and > sends them to > > > + * the input layer. It is scheduled from the interrupt (or > timer). > > > + * > *********************************************************************** > **/ > > > +void handle_single_touch(struct cyttsp_xydata *xy, struct > cyttsp_track_data *t, > > > + struct cyttsp *ts) > > > +{ > > > > functions should be "static". I would leave a decision to Dmitry if > he wants the driver > > to support old single touch protocol hacked up with HAT_xx bits so > that driver can support > > two touches with the old single touch protocol itself. > > The ABS_HAT* bits should go away. They were gross abuse even when we > did > not have MT protocol and shoudl not be used at all now that we do have > it. I will be cleaning up older drivers (like Elantech) to get rid of > them. I will remove the ST code completely. > > Multitouch devices shouls support either A or B MT protocol. SInce this > device seems to be supporting tracking in hardware it shoudl simply > adhere to protocol B to limit kernel->userspace traffict and be done > with it. > > ... I have responded to Henrik's review about protocol A and B. I would like to keep protocol A for now and just change the default to Protocol B? > > > > + > > > +/* schedulable worker entry for timer polling method */ > > > +void cyttsp_xy_worker_(struct work_struct *work) > > > +{ > > > + struct cyttsp *ts = container_of(work, struct cyttsp, work); > > > + cyttsp_xy_worker(ts); > > > +} > > > > I would really prefer that you remove the polling method from this > code as it will simplify > > a code lot. We can delete the whole workqueue because as you will be > using request_threaded_irq(...), > > you will not need any workqueue. > > > > Seconded. Polling is hardly useful on real production setup as it will > drain battery in no time. > I will remove polling completely. > > > + > > > +static int cyttsp_i2c_init(void) > > > +{ > > > > Please add __init like > > > > static int __init cyttsp_i2c_init(void) > > > > > + int retval; > > > + retval = i2c_add_driver(&cyttsp_i2c_driver); > > > + printk(KERN_INFO "%s: Cypress TrueTouch(R) Standard Product I2C " > > > + "Touchscreen Driver (Built %s @ %s) returned %d\n", > > > + __func__, __DATE__, __TIME__, retval); > > > + > > And lose printk as well. The boot is exremely noisy as it is... > > return i2c_add_driver(&cyttsp_i2c_driver); > > is all that is needed. I will make this change. Thank you for your review. -kev > > -- > Dmitry --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |