lkml.org 
[lkml]   [2011]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRE: [v3 1/3] 1/3 Touchscreen: Cypress TTSP G3 MTDEV Core Driver
    Date
    From
    Hi Henrik,

    Thanks for your comments. Comments and questions inline below...

    > -----Original Message-----
    > From: Henrik Rydberg [mailto:rydberg@euromail.se]
    > Sent: Friday, December 31, 2010 3:53 AM
    > To: Kevin McNeely
    > Cc: Dmitry Torokhov; David Brown; Trilok Soni; Samuel Ortiz; Eric
    Miao;
    > Mike Frysinger; Alan Cox; linux-input@vger.kernel.org; linux-
    > kernel@vger.kernel.org
    > Subject: Re: [v3 1/3] 1/3 Touchscreen: Cypress TTSP G3 MTDEV Core
    > Driver
    >
    > Hi Kevin,
    >
    > Thanks for the changes, it looks much better now. Some comments on the
    > MT
    > part inline.
    >
    > > diff --git a/drivers/input/touchscreen/Kconfig
    > b/drivers/input/touchscreen/Kconfig
    > > index 06ea8da..7d886bc 100644
    > > --- a/drivers/input/touchscreen/Kconfig
    > > +++ b/drivers/input/touchscreen/Kconfig
    > > @@ -124,6 +124,11 @@ config TOUCHSCREEN_CY8CTMG110
    > > To compile this driver as a module, choose M here: the
    > > module will be called cy8ctmg110_ts.
    > >
    > > +config TOUCHSCREEN_CYTTSP_CORE
    > > + bool "Cypress TTSP touchscreen core"
    > > + help
    > > + Always activated for Cypress TTSP touchscreen
    > > +
    >
    > Tristate please.

    This will be changed.

    >
    > [...]
    > > +
    > > +struct cyttsp_tch {
    > > + u16 x;
    > > + u16 y;
    > > + u8 z;
    > > +};
    >
    > Does not appear to be used anywhere.

    This will be removed.

    >
    > [...]
    > > +struct cyttsp {
    > > + struct device *dev;
    > > + int irq;
    > > + struct input_dev *input;
    > > + struct mutex mutex;
    >
    > Is this really used?

    No. This will be removed.

    >
    > > + char phys[32];
    > > + const struct bus_type *bus_type;
    > > + const struct cyttsp_platform_data *platform_data;
    > > + struct cyttsp_bus_ops *bus_ops;
    > > + struct cyttsp_xydata xy_data;
    > > + struct cyttsp_bootloader_data bl_data;
    > > + struct cyttsp_sysinfo_data sysinfo_data;
    > > + struct completion bl_ready;
    > > + enum cyttsp_powerstate power_state;
    > > +};
    > > +
    > > +static const u8 bl_command[] = {
    > > + CY_BL_FILE0, CY_BL_CMD, CY_BL_EXIT,
    > > + CY_BL_KEY0, CY_BL_KEY1, CY_BL_KEY2,
    > > + CY_BL_KEY3, CY_BL_KEY4, CY_BL_KEY5,
    > > + CY_BL_KEY6, CY_BL_KEY7
    > > +};
    >
    > If the CY_BL* values are only used here, how about putting the numbers
    > here directly?

    This will be done.

    >
    > [...]
    > > +static int ttsp_read_block_data(struct cyttsp *ts, u8 command,
    > > + u8 length, void *buf)
    > > +{
    > > + int retval;
    > > + int tries;
    > > +
    > > + if (!buf || !length)
    > > + return -EIO;
    >
    > -EINVAL?

    This will be changed.

    >
    > > +
    > > + for (tries = 0, retval = -1;
    > > + tries < CY_NUM_RETRY && (retval < 0);
    > > + tries++)
    > > + retval = ts->bus_ops->read(ts->bus_ops, command, length,
    > buf);
    > > +
    > > + return retval;
    > > +}
    >
    > [...]
    > > +/* process current touches */
    > > +static int cyttsp_xy_worker(struct cyttsp *ts)
    > > +{
    > > + u8 num_cur_tch = 0;
    >
    > Does not appear to need initialization.

    This will be cleaned up.

    >
    > > + u16 x;
    > > + u16 y;
    > > + u8 z;
    >
    > Perhaps x[2], y[2] etc here.

    This will be implemented.

    >
    > > +
    > > + /* Get touch data from CYTTSP device */
    > > + if (ttsp_read_block_data(ts,
    > > + CY_REG_BASE, sizeof(struct cyttsp_xydata),
    &ts->xy_data))
    > > + return 0;
    > > +
    > > + /* touch extension handling */
    > > + if (ttsp_tch_ext(ts, &ts->xy_data))
    > > + return 0;
    > > +
    > > + /* provide flow control handshake */
    > > + if (ts->platform_data->use_hndshk)
    > > + if (cyttsp_hndshk(ts, ts->xy_data.hst_mode))
    > > + return 0;
    > > +
    > > + /* determine number of currently active touches */
    > > + num_cur_tch = GET_NUM_TOUCHES(ts->xy_data.tt_stat);
    > > +
    > > + /* check for any error conditions */
    > > + if (ts->power_state == CY_IDLE_STATE)
    > > + return 0;
    > > + else if (GET_BOOTLOADERMODE(ts->xy_data.tt_mode)) {
    > > + return -1;
    > > + } else if (IS_LARGE_AREA(ts->xy_data.tt_stat) == 1) {
    > > + /* terminate all active tracks */
    > > + num_cur_tch = 0;
    > > + dev_dbg(ts->dev, "%s: Large area detected\n", __func__);
    > > + } else if (num_cur_tch > 2) {
    > > + /* terminate all active tracks */
    > > + num_cur_tch = 0;
    > > + dev_dbg(ts->dev, "%s: Num touch error detected\n",
    > __func__);
    > > + } else if (IS_BAD_PKT(ts->xy_data.tt_mode)) {
    > > + /* terminate all active tracks */
    > > + num_cur_tch = 0;
    > > + dev_dbg(ts->dev, "%s: Invalid buffer detected\n",
    > __func__);
    > > + }
    > > +
    > > + /* send touches */
    > > + if (!num_cur_tch)
    > > + /* terminate previous active touch */
    > > + input_mt_sync(ts->input);
    > > +
    > > + if (num_cur_tch) {
    > > + /* send touch 1 */
    > > + /*
    > > + * If there is only one current active touch,
    > > + * it will be reported in the touch 1 regardless
    > > + * if it was reported in the touch 2 previously
    > > + */
    >
    > Perhaps "touch" should be "track" or "slot" here?

    I plan to keep the "touch" in order to match our part documentation.

    >
    > > + x = (ts->xy_data.tch1_xhi << 8) + ts->xy_data.tch1_xlo;
    > > + y = (ts->xy_data.tch1_yhi << 8) + ts->xy_data.tch1_ylo;
    > > + z = ts->xy_data.tch1_z;
    >
    > Perhaps x[0] = ... here

    This will be implemented.

    >
    > > + input_report_abs(ts->input, ABS_MT_POSITION_X, x);
    > > + input_report_abs(ts->input, ABS_MT_POSITION_Y, y);
    > > + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, z);
    > > + input_mt_sync(ts->input);
    >
    > Then these could still be collected in a loop.

    The loop will be added.

    >
    > > + }
    > > +
    > > + if (num_cur_tch > 1) {
    > > + /* send touch 2 */
    > > + x = (ts->xy_data.tch2_xhi << 8) + ts->xy_data.tch2_xlo;
    > > + y = (ts->xy_data.tch2_yhi << 8) + ts->xy_data.tch2_ylo;
    > > + z = ts->xy_data.tch2_z;
    >
    > And x[1] = ...

    The same, x[1] and the loop will be added.

    >
    > > + input_report_abs(ts->input, ABS_MT_POSITION_X, x);
    > > + input_report_abs(ts->input, ABS_MT_POSITION_Y, y);
    > > + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, z);
    > > + input_mt_sync(ts->input);
    > > + }
    >
    > Ditto.

    The same.

    >
    > > +
    > > + input_sync(ts->input);
    > > +
    > > + return 0;
    > > +}
    >
    > If the in-kernel tracking module was in place, would you consider
    > switching to the slots protocol?

    Hi Henrik, can you please clarify? Should I remove MTDEV from the patch
    title?

    Thanks and best regards,
    Kevin

    >
    > 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.
    ---------------------------------------------------------------



    \
     
     \ /
      Last update: 2011-01-03 18:11    [W:0.039 / U:0.656 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site