Messages in this thread | | | From | "Henrik Rydberg" <> | Date | Thu, 13 Oct 2011 20:03:20 +0200 | Subject | Re: [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
| |