lkml.org 
[lkml]   [2011]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5 1/3] Input: cyttsp - Cypress TTSP capacitive multi-touch screen support
Hi Javier,

some more comments on the code. Presumable Dmitry will have additional
ones later on. Thank you for your patience.

> +/* Slots management */
> +#define CY_MAX_FINGER 4
> +#define CY_MAX_ID 15

I realize the firmware reports ids in the range [1..14], but it is not
visible from the code. Allowing all 16 values makes correctness
obvious.

> +static int cyttsp_load_bl_regs(struct cyttsp *ts)
> +{
> + int retval;
> +
> + memset(&(ts->bl_data), 0, sizeof(struct cyttsp_bootloader_data));
> +
> + ts->bl_data.bl_status = 0x10;
> +
> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
> + sizeof(ts->bl_data), &(ts->bl_data));
> +
> + return retval;
> +}

Looks like you missed the removal of retval here.

> +
> +static int cyttsp_bl_app_valid(struct cyttsp *ts)
> +{
> + int retval;
> +
> + retval = cyttsp_load_bl_regs(ts);
> +
> + if (retval < 0) {
> + retval = -ENODEV;
> + goto done;
> + }
> +
> + 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;
> + }
> + }
> +
> + 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;
> + }
> + }
> +
> + dev_dbg(ts->dev, "%s: Non-Operational failure\n",
> + __func__);
> + retval = -ENODEV;
> +done:
> + return retval;
> +
> +}

Are the debug paths still necessary? There are a lot of them.

> +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;
> +}

Regardless of if it can ever happen, programatically the (retval > 0)
&& GET_BOOTLOADERMODE(ts->bl_data.bl_status) case is slipping through here.

> +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;
> +}

So (xy_data.act_dist == CY_ACT_DIST_DFL) here is not an error?

> +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));

Expands to (retval || !ts->sysinfo_data.tts_verh &&
!ts->sysinfo_data.tts_verl) && (tries++ < CY_DELAY_MAX), which seems a
bit simpler to me. Also, &ts->sysinfo_data could be a pointer here.

Coding verh+verl as a 16-bit value?

Are the timeouted (tries >= CY_DELAY_MAX) cases not errors here either?

<snip>

> +static int cyttsp_soft_reset(struct cyttsp *ts)
> +{
> + int retval;
> + u8 cmd = CY_SOFT_RESET_MODE;
> + long wait_jiffies = msecs_to_jiffies(CY_DELAY_DFLT * CY_DELAY_MAX);
> + /* wait for interrupt to set ready completion */
> + INIT_COMPLETION(ts->bl_ready);
> +
> + retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
> + if (retval < 0)
> + return retval;
> +
> + retval = wait_for_completion_timeout(&ts->bl_ready, wait_jiffies);
> +
> + if (retval > 0)
> + retval = 0;

Seems this one slipped.

> +
> + 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;

Whoa, please redo this one correctly.

> +
> + retval = ttsp_write_block_data(ts, CY_REG_BASE,
> + sizeof(cmd), (u8 *)&cmd);
> +
> + return retval;
> +}

> +int cyttsp_resume(void *handle)
> +{
> + struct cyttsp *ts = handle;
> + int retval = 0;
> + struct cyttsp_xydata xydata;
> +
> + if (ts) {

Neater if returning on (!ts) here instead.

> + if (ts->platform_data->use_sleep && (ts->power_state !=
> + CY_ACTIVE_STATE)) {
> + if (ts->platform_data->wakeup) {
> + retval = ts->platform_data->wakeup();
> + if (retval < 0)
> + dev_dbg(ts->dev, "%s: Error, wakeup failed!\n",
> + __func__);
> + } else {
> + dev_dbg(ts->dev, "%s: Error, wakeup not implemented "
> + "(check board file).\n", __func__);
> + retval = -ENOSYS;
> + }
> + if (!(retval < 0)) {
> + retval = ttsp_read_block_data(ts, CY_REG_BASE,
> + sizeof(xydata),
> + &xydata);
> + if (!(retval < 0) &&
> + !GET_HSTMODE(xydata.hst_mode))
> + ts->power_state = CY_ACTIVE_STATE;
> + }
> + }
> + dev_dbg(ts->dev, "%s: Wake Up %s\n", __func__,
> + (retval < 0) ? "FAIL" : "PASS");
> + }
> + return retval;
> +}
> +EXPORT_SYMBOL_GPL(cyttsp_resume);

Thanks.
Henrik


\
 
 \ /
  Last update: 2011-10-13 19:59    [W:0.093 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site