Re: [PATCH V2 1/3] Input: cyttsp - Cypress TTSP capacitivemulti-touch screen support

[Javier Martinez Canillas]

On Wed, Sep 14, 2011 at 12:50 PM, Mohan Pallaka <mpallaka@xxxxxxxxxxxxxx> wrote:
> ÂHi Javier,
>
> Please find my review comments.
>

Hello Mohan,

Thank you very much for the review and your comments.

> On 9/3/2011 11:20 AM, Javier Martinez Canillas wrote:
>> +
>> +struct cyttsp {
>> + Â Â Â struct device *dev;
>> + Â Â Â int irq;
>> + Â Â Â struct input_dev *input;
>> + Â Â Â char phys[32];
>> + Â Â Â const struct bus_type *bus_type;
>
> Do we need to know the bus type? Rest of the code doesn't seem to
> be using this information.

You are right, will remove it.

>> +
>> +static int ttsp_write_block_data(struct cyttsp *ts, u8 command,
>> + Â Â Â u8 length, void *buf)
>> +{
>> + Â Â Â int retval;
>> + Â Â Â if (!buf || !length)
>> + Â Â Â Â Â Â Â return -EINVAL;
>> +
>> + Â Â Â retval = ts->bus_ops->write(ts->bus_ops, command, length, buf);
>> + Â Â Â if (retval)
>> + Â Â Â Â Â Â Â msleep(CY_DELAY_DFLT);
>
> Don't we need to retry if write fails?

Yes, I'll add the retry logic also to ttsp_write_block_data().

>> +
>> +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 {
>> + Â Â Â Â Â Â Â msleep(CY_DELAY_DFLT);
>> + Â Â Â Â Â Â Â 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))&&
>
> what is the need for this check,"xy_data.act_dist == CY_ACT_DIST_DFLT"?
> we can pass a different value for act_dist from pdata. Another point to
> concerns me
> is that in case of i2c/spi failures "ttsp_read_block_data" will it self try
> for NUM_TRY
> and this particular code block tries till CY_DELAY_MAX, I think we are
> overdoing
> in this case. ÂHow about keeping the "ttsp_read_block_dat" simple and let
> the
> code that uses do the retry job.

Yes, there are too many msleep() and retry here, will clean this code
and let ttsp_read_block_data() do the retry.

>> +
>> +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_xy_worker(ts);
>
> can we have a different name than cyttsp_xy_worker as we can misinterpret as
> a workqueue,
> like cyttsp_handle_tchdata. Also, we seem to be doing good amount of work in
> this function,
> how about deferring it to a workqueue to make irq handler fast to not to
> miss any irqs.

Yes, your are right worker sounds like a workqueue. Will change the
name of the handler function.

Well cyttsp_xy_worker() is calling from cyttsp_irq() which is
initialized has a threaded irq with request_threaded_irq(). I thought
it was the way to go for new drivers, isn't using a workqueue inside a
threaded irq overkill?

>> +
>> +#ifdef CONFIG_PM
>> +int cyttsp_resume(void *handle)
>> +{
>> + Â Â Â struct cyttsp *ts = handle;
>
> "handle" is passed from a different path, so it is possible to be a "NULL"
> which
> could crash the machine. so I think we can have an extra check here like you
> did for core_release.

Ok, will add the check.

>>
>> + Â Â Â int retval = 0;
>> + Â Â Â struct cyttsp_xydata xydata;
>> +
>> + Â Â Â 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;
>
> I think we need to have a error msg when it fails to resume. Also, say after
> resume if it goes to
> "bootloader"(which happens when the chip is reset) mode then touch will not
> work. So, we
> can get it out of the bootloader mode in cyttsp_xy_worker as interrupts are
> fired even in bootloader
> mode.

Yes, will add the error msg and add the logic in the irq handler to go
out from bootloader mode.

>> +
>> +int cyttsp_suspend(void *handle)
>> +{
>> + Â Â Â struct cyttsp *ts = handle;
>> + Â Â Â u8 sleep_mode = 0;
>> + Â Â Â int retval = 0;
>> +
>> + Â Â Â if (ts->platform_data->use_sleep&&
>> + Â Â Â Â Â Â Â (ts->power_state == CY_ACTIVE_STATE)) {
>> + Â Â Â Â Â Â Â sleep_mode = CY_DEEP_SLEEP_MODE;
>> + Â Â Â Â Â Â Â retval = ttsp_write_block_data(ts,
>> + Â Â Â Â Â Â Â Â Â Â Â CY_REG_BASE, sizeof(sleep_mode),&sleep_mode);
>> + Â Â Â Â Â Â Â if (!(retval< Â0))
>> + Â Â Â Â Â Â Â Â Â Â Â ts->power_state = CY_SLEEP_STATE;
>> + Â Â Â }
>> + Â Â Â dev_dbg(ts->dev, "%s: Sleep Power state is %s\n", __func__,
>> + Â Â Â Â Â Â Â (ts->power_state == CY_ACTIVE_STATE) ?
>> + Â Â Â Â Â Â Â "ACTIVE" :
>> + Â Â Â Â Â Â Â ((ts->power_state == CY_SLEEP_STATE) ?
>> + Â Â Â Â Â Â Â "SLEEP" : "LOW POWER"));
>
> This code will not let the chip to go to low power mode since we are setting
> CY_DEEP_SLEEP_MODE directly. So, lets use platform_data's "use_sleep" flag
> to
> determine if we want to go to deep sleep or low power mode.

Ok, will change to check platform_data's to take the decision.


>> +
>> + Â Â Â if (input_register_device(input_device)) {
>
> Please avoid this style of check, we will not know why it is failed.

Ok, will get the error value to know what happened.

>>
>> + Â Â Â Â Â Â Â dev_dbg(ts->dev, "%s: Error, failed to register input
>> device\n",
>> + Â Â Â Â Â Â Â Â Â Â Â __func__);
>> + Â Â Â Â Â Â Â goto error_input_register_device;
>> + Â Â Â }
>> +
>> + Â Â Â goto no_error;
>> +
>> +error_input_register_device:
>> + Â Â Â input_unregister_device(input_device);
>
> Here we should use input_free_device() rather than unregister since it
> failed in
> registration.

Ok, will use input_free_device() instead.

>
> --Mohan.
>
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum.
>
>
>

Again, thank you very much for taking the time to look at the code and
for your suggestions. Will work on this 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/