On Fri, Oct 7, 2011 at 1:55 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
> Hi Javier,
>
> thanks for the changes. Some general comments below, but the MT
> implementation looks good now.
>
Hi Henrik,
Your welcome, thank you for reviewing.
Good to know that the MT implementation looks good to you now.
>> Cypress TrueTouch(tm) Standard Product controllers are found in
>> a wide range of embedded devices. This driver add support for a
>> variety of TTSP controllers.
>>
>> The driver is composed of a core driver that process the data sent by
>> the contacts and a set of bus specific interface modules. This patch
>> adds the base core TTSP driver.
>>
>> The original author of the driver is Kevin McNeely <kev@xxxxxxxxxxx>
>>
>> Since the hardware is capable of tracking identifiable contacts and the
>> original driver used multi-touch protocol type A (stateless), multi-touch
>> protocol type B (stateful) support was added by Javier Martinez Canillas.
>>
>> Signed-off-by: Javier Martinez Canillas <martinez.javier@xxxxxxxxx>
>> ---
>>
>> v2: Fix issues called out by Dmitry Torokhov
>> - Add msleep() delays between retries for read and write operations
>> - Change cyttsp_core_init() to receive the IRQ from the client data
>> instead of obtaining from the platform_data
>>
>> v3: Fix issues called out by Henrik Rydberg and Mohan Pallaka
>> - Map each possible track id to a multitouch input slot
>> - Remove bus type info since it is not used
>> - Add retry logic to ttsp_write_block_data()
>> - ttsp_read_block_data() already msleep() remove the sleep in the caller
>> - cyttsp_xy_worker() sounds as if it's a workqueue, change the function name
>> - Check if handle is NULL in cyttsp_resume()
>> - Use platform data's use_sleep to decide to go deep sleep or low power mode
>> - input_register_device() error path has to call input_free_device()
>>
>> v4: Fix issues called out by Henrik Rydberg
>> - Remove unnecesary code and cleanup contact handler since input core is able
>> to detect duplicates
>>
>> +
>> +static int ttsp_read_block_data(struct cyttsp *ts, u8 command,
>> + u8 length, void *buf)
>> +{
>> + int retval;
>> + int tries;
>> +
>> + if (!buf || !length)
>> + return -EINVAL;
>> +
>> + for (tries = 0, retval = -1;
>> + tries < CY_NUM_RETRY && (retval < 0);
>> + tries++) {
>
> Possible to use less than three lines here?
>
Yes it is, the retval = -1 doesn't make to much sense since it can be
set at variable declaration.
>> + retval = ts->bus_ops->read(ts->bus_ops, command, length, buf);
>> + if (retval)
>> + msleep(CY_DELAY_DFLT);
>
> The loop breaks on retval < 0, but delays on retval - is a) the delay
> on retval < 0 really necessary, and b) is there an assumption that
> retval > 0 all mean retry?
>
Every bus_ops->read function (I2C, SPI, etc) returns 0 on success and
propagate the corresponding error otherwise. So retval >= 0 is what
breaks the loop not retval < 0. The delay on retval < 0 is to wait
some miliseconds and retry the operations a few times (CY_NUM_RETRY)
before desist.
In early versions of the patch each bus read and write function
implemented the retry logic so moving to the core read/write
operations made sense to remove duplicate code.
>> + }
>> +
>> + return retval;
>> +}
>> +
>> +static int ttsp_write_block_data(struct cyttsp *ts, u8 command,
>> + u8 length, void *buf)
>> +{
>> + int retval;
>> + int tries;
>> +
>> + if (!buf || !length)
>> + return -EINVAL;
>> +
>> + for (tries = 0, retval = -1;
>> + tries < CY_NUM_RETRY && (retval < 0);
>> + tries++) {
>> + retval = ts->bus_ops->write(ts->bus_ops, command, length, buf);
>> + if (retval)
>> + msleep(CY_DELAY_DFLT);
>> + }
>
> Ditto.
>
Same here retval < 0 indicates an error so retval == 0 breaks the
loop, will clean the for so it uses one line instead of three.
>> +
>> + return retval;
>> +}
>> +
>> +static int ttsp_tch_ext(struct cyttsp *ts, void *buf)
>> +{
>> + int retval;
>> +
>> + if (!buf)
>> + return -EIO;
>> +
>> + retval = ts->bus_ops->ext(ts->bus_ops, buf);
>> +
>> + return retval;
>> +}
>
> Seems this could be simplified or even removed.
>
Yes, the core was thought to handle touch extended information but is
not used so I will remove this.
>> +
>> +static int cyttsp_load_bl_regs(struct cyttsp *ts)
>> +{
>> + int retval;
>> +
>> + memset(&(ts->bl_data), 0, sizeof(struct cyttsp_bootloader_data));
>> +
>> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
>> + sizeof(ts->bl_data), &(ts->bl_data));
>> +
>> + return retval;
>> +}
>
> Ditto.
>
This function is used to get the bootloader data from the device when
it is open and for exiting the bootloader.
>> +
>> +static int cyttsp_bl_app_valid(struct cyttsp *ts)
>> +{
>> + int retval;
>> +
>> + retval = cyttsp_load_bl_regs(ts);
>> +
>> + if (retval < 0)
>> + return -ENODEV;
>> +
>> + if (GET_BOOTLOADERMODE(ts->bl_data.bl_status)) {
>> + if (IS_VALID_APP(ts->bl_data.bl_status)) {
>> + dev_dbg(ts->dev, "%s: App found; normal boot\n",
>> + __func__);
>> + return 0;
>> + } else {
>> + dev_dbg(ts->dev, "%s: NO APP; load firmware!!\n",
>> + __func__);
>> + return -ENODEV;
>> + }
>> + } else if (GET_HSTMODE(ts->bl_data.bl_file) == CY_OPERATE_MODE) {
>> + if (!(IS_OPERATIONAL_ERR(ts->bl_data.bl_status))) {
>> + dev_dbg(ts->dev, "%s: Operational\n",
>> + __func__);
>> + return 1;
>> + } else {
>> + dev_dbg(ts->dev, "%s: Operational failure\n",
>> + __func__);
>> + return -ENODEV;
>> + }
>> + } else {
>> + dev_dbg(ts->dev, "%s: Non-Operational failure\n",
>> + __func__);
>> + return -ENODEV;
>> + }
>
> No return here is slightly confusing. Seems return -ENODEV could be moved here.
>
Yes, this code can be cleaned. I´will change it to always return
retval and setting its value.
>> +
>> +}
>> +
>> +static int cyttsp_exit_bl_mode(struct cyttsp *ts)
>> +{
>> + int retval;
>> + int tries;
>> + u8 bl_cmd[sizeof(bl_command)];
>> +
>> + memcpy(bl_cmd, bl_command, sizeof(bl_command));
>> + if (ts->platform_data->bl_keys)
>> + memcpy(&bl_cmd[sizeof(bl_command) - CY_NUM_BL_KEYS],
>> + ts->platform_data->bl_keys, sizeof(bl_command));
>> +
>> + dev_dbg(ts->dev,
>> + "%s: bl_cmd= "
>> + "%02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n",
>> + __func__, bl_cmd[0], bl_cmd[1], bl_cmd[2],
>> + bl_cmd[3], bl_cmd[4], bl_cmd[5], bl_cmd[6],
>> + bl_cmd[7], bl_cmd[8], bl_cmd[9], bl_cmd[10]);
>> +
>> + retval = ttsp_write_block_data(ts, CY_REG_BASE,
>> + sizeof(bl_cmd), (void *)bl_cmd);
>> + if (retval < 0)
>> + return retval;
>> +
>> + /* wait for TTSP Device to complete switch to Operational mode */
>> + tries = 0;
>> + do {
>> + msleep(CY_DELAY_DFLT);
>> + retval = cyttsp_load_bl_regs(ts);
>> + } while (!((retval == 0) &&
>> + !GET_BOOTLOADERMODE(ts->bl_data.bl_status)) &&
>> + (tries++ < CY_DELAY_MAX));
>> +
>> + dev_dbg(ts->dev, "%s: check bl ready tries=%d ret=%d stat=%02X\n",
>> + __func__, tries, retval, ts->bl_data.bl_status);
>> +
>> + if (retval < 0)
>> + return retval;
>> + else if (GET_BOOTLOADERMODE(ts->bl_data.bl_status))
>> + return -ENODEV;
>> + else
>> + return 0;
>> +}
>
> Is retval > 0 && !GET_BOOTLOADERMODE(ts->bl_data.bl_status) a possible
> state, and if so, valid?
>
cyttsp_load_bl_regs() memset the structure with 0s before reading from
the bus so the registers state should be correct.
I will modify cyttsp_load_bl_regs() to set the bootloader mode bit
after the memset but before the read to ensure that a bit clear is
because that value was really readed from the bus.
>> +
>> +static int cyttsp_set_operational_mode(struct cyttsp *ts)
>> +{
>> + struct cyttsp_xydata xy_data;
>> + int retval;
>> + int tries;
>> + u8 cmd = CY_OPERATE_MODE;
>> +
>> + retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
>> +
>> + if (retval < 0)
>> + return retval;
>> +
>> + /* wait for TTSP Device to complete switch to Operational mode */
>> + tries = 0;
>> + do {
>> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
>> + sizeof(xy_data), &(xy_data));
>> + } while (!((retval == 0) &&
>> + (xy_data.act_dist == CY_ACT_DIST_DFLT)) &&
>> + (tries++ < CY_DELAY_MAX));
>> +
>> + dev_dbg(ts->dev, "%s: check op ready tries=%d ret=%d dist=%02X\n",
>> + __func__, tries, retval, xy_data.act_dist);
>> +
>> + return retval;
>> +}
>> +
>> +static int cyttsp_set_sysinfo_mode(struct cyttsp *ts)
>> +{
>> + int retval;
>> + int tries;
>> + u8 cmd = CY_SYSINFO_MODE;
>> +
>> + memset(&(ts->sysinfo_data), 0, sizeof(struct cyttsp_sysinfo_data));
>> +
>> + /* switch to sysinfo mode */
>> + retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
>> + if (retval < 0)
>> + return retval;
>> +
>> + /* read sysinfo registers */
>> + tries = 0;
>> + do {
>> + msleep(CY_DELAY_DFLT);
>> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
>> + sizeof(ts->sysinfo_data), &(ts->sysinfo_data));
>> + } while (!((retval == 0) &&
>> + !((ts->sysinfo_data.tts_verh == 0) &&
>> + (ts->sysinfo_data.tts_verl == 0))) &&
>> + (tries++ < CY_DELAY_MAX));
>> +
>> + dev_dbg(ts->dev, "%s: check sysinfo ready tries=%d ret=%d\n",
>> + __func__, tries, retval);
>> +
>> + dev_info(ts->dev, "%s: tv=%02X%02X ai=0x%02X%02X "
>> + "av=0x%02X%02X ci=0x%02X%02X%02X\n", "cyttsp",
>> + ts->sysinfo_data.tts_verh, ts->sysinfo_data.tts_verl,
>> + ts->sysinfo_data.app_idh, ts->sysinfo_data.app_idl,
>> + ts->sysinfo_data.app_verh, ts->sysinfo_data.app_verl,
>> + ts->sysinfo_data.cid[0], ts->sysinfo_data.cid[1],
>> + ts->sysinfo_data.cid[2]);
>> +
>> + return retval;
>> +}
>
> Similar questions here; apparently not only retval matters, but only retval is returned.
>
Yes, this is for the semantic of the bus read functions. At the end of
each read function (I2C, SPI) only is check if the return value is <
0. i.e:
retval = i2c_master_recv(ts->client, values, length);
return (retval < 0) ? retval : 0;
So, in other words we don't care the bytes count, only if an error has
occurred or not. It this correct? Or should I propagate the byte count
and check if is equal to sizeof(struct) read?
>> +
>> +static int cyttsp_set_sysinfo_regs(struct cyttsp *ts)
>> +{
>> + int retval = 0;
>> +
>> + if (ts->platform_data->act_intrvl != CY_ACT_INTRVL_DFLT ||
>> + ts->platform_data->tch_tmout != CY_TCH_TMOUT_DFLT ||
>> + ts->platform_data->lp_intrvl != CY_LP_INTRVL_DFLT) {
>> +
>> + u8 intrvl_ray[3];
>> +
>> + intrvl_ray[0] = ts->platform_data->act_intrvl;
>> + intrvl_ray[1] = ts->platform_data->tch_tmout;
>> + intrvl_ray[2] = ts->platform_data->lp_intrvl;
>> +
>> + /* set intrvl registers */
>> + retval = ttsp_write_block_data(ts,
>> + CY_REG_ACT_INTRVL,
>> + sizeof(intrvl_ray), intrvl_ray);
>> +
>> + msleep(CY_DELAY_DFLT);
>
> What happens if there is no delay here?
>
The delay here is to make sure that the sysinfo register settings are complete.
We wait since we usually change mode to operational after this function.
>> + }
>> +
>> + return retval;
>> +}
>> +
>> +static int cyttsp_soft_reset(struct cyttsp *ts)
>> +{
>> + int retval;
>> + u8 cmd = CY_SOFT_RESET_MODE;
>> +
>> + retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
>> + if (retval < 0)
>> + return retval;
>> +
>> + /* wait for interrupt to set ready completion */
>> + INIT_COMPLETION(ts->bl_ready);
>> +
>> + retval = wait_for_completion_interruptible_timeout(&ts->bl_ready,
>> + msecs_to_jiffies(CY_DELAY_DFLT * CY_DELAY_MAX));
>> +
>> + if (retval > 0)
>> + retval = 0;
>> +
>> + return retval;
>
> Please simplify/correct the logic here.
>
Ok, will do.
>> +}
>> +
>> +static int cyttsp_act_dist_setup(struct cyttsp *ts)
>> +{
>> + int retval;
>> + u8 act_dist_setup;
>> +
>> + /* Init gesture; active distance setup */
>> + act_dist_setup = ts->platform_data->act_dist;
>> + retval = ttsp_write_block_data(ts, CY_REG_ACT_DIST,
>> + sizeof(act_dist_setup), &act_dist_setup);
>> +
>> + return retval;
>> +}
>> +
>> +static int cyttsp_hndshk(struct cyttsp *ts, u8 hst_mode)
>> +{
>> + int retval;
>> + u8 cmd;
>> +
>> + cmd = hst_mode & CY_HNDSHK_BIT ?
>> + hst_mode & ~CY_HNDSHK_BIT :
>> + hst_mode | CY_HNDSHK_BIT;
>
> why not XOR?
>
OK, will change
>> +
>> +static irqreturn_t cyttsp_irq(int irq, void *handle)
>> +{
>> + struct cyttsp *ts = handle;
>> + int retval;
>> +
>> + if (ts->power_state == CY_BL_STATE)
>> + complete(&ts->bl_ready);
>> + else {
>> + /* process the touches */
>> + retval = cyttsp_handle_tchdata(ts);
>> +
>> + if (retval < 0) {
>> + /*
>> + * TTSP device has reset back to bootloader mode.
>> + * Restore to operational mode.
>> + */
>> + retval = cyttsp_exit_bl_mode(ts);
>> + if (retval)
>> + ts->power_state = CY_IDLE_STATE;
>> + else
>> + ts->power_state = CY_ACTIVE_STATE;
>> + cyttsp_pr_state(ts);
>
> You really want to call the above from the interrupt handler?
>
Is good to notice that this is the scheduled half of the interrupt
process. So, since the handler is a threaded IRQ technically we are in
process context and not in interrupt context.
If retval<0, then the touch handler found that the bootloader is running.
If the part is glitched, then it will reset itself back to bootloader mode and
the bootloader heartbeat interrupt would have occurred which will be the
cause of the interrupt.
So we want to exit the bootloader immediately and continue running.
>
> Thanks,
> Henrik
>
Thank you very much for your time to review this. I will try to fix
these issues on the weekend and resend.
Best regards,
--
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
