Re: [PATCH 0/7] A few patches to cyttsp

On Wed, Nov 16, 2011 at 8:38 PM, Dmitry Torokhov
[off-list ref] wrote:

On Wed, Nov 16, 2011 at 08:17:02PM +0100, Javier Martinez Canillas wrote:

quoted

On Mon, Nov 14, 2011 at 9:15 AM, Dmitry Torokhov
[off-list ref] wrote:

quoted

Hi Javier,

I am sending a few patches that I made while trying to understand the
Cypress TTSP driver. I am still not quite happy with the SPI read method
and I am also wondering if cyttsp_power_on could be made less messy.

Hi Dmitry,

Thank you very much for the review and the patches. One question, when
sending new versions of the patch-set, do you want me to keep the
history of your patches or should I merge all of them and submit just
three patches (core, i2c, spi) to make easier future revisions?

You may merge them, I think it will be easier.

Ok, thanks.

quoted

Sure, Kevin will cleanup the SPI read method and I will make
cyttsp_power_on more cleaner.

Thanks.

quoted

quoted

Current open/close methods seem to be not very useful: even though we
stop interrupts we do not put the chip into low power mode. Is there a
way to do so?

Yes, the controller can be put to sleep (low power mode). I will add
that to the close function for the next version of the patch-set.

Excellent.

quoted

quoted

Regarding PM methods - why do we have conditional 'use sleep' and what
exactly wakeup() method is supposed to do? Why is it a platform method?

It was meant to let each board to decide if it wants to go sleep and
to define a specific handler for wakeup.

Why would a board not want to go to sleep? Normally boards want to
specify whether they want to have device as a wakeup source and such
devices, instead of shutting down in suspend method, just configure
their IRQ as a wakeup IRQ and that is it.

Yes, you are right. Also, the chip has different low power modes
(light, medium and deep) and use_sleep was also thought so board code
could decide on which low power mode the device has to enter on
suspend.

But, yes. The sleep shouldn't be conditional and we should have a low
power mode by default. Also if the user defines a wakeup() handler,
then the device should enter in a low power mode where interrupts are
still fired (so the board can use this device as a wakeup source).

quoted

This is for example the board
file of the OMAP Encore machine type (B & N Nook's color)

static struct cyttsp_platform_data cyttsp_i2c_platform_data = {
        .wakeup = cyttsp_i2c_wakeup,

OK, so in this particular case what does cyttsp_i2c_wakeup actually
does?

In this case in particular, nothing...

static int cyttsp_i2c_wakeup(void)
{
        return 0;
}

I think is because the resume code returned -ENOSYS if use_sleep was
set and not wakeup handler was defined. So a dummy handler was
defined.

int cyttsp_resume(void *handle)
{
...
		if (ts->platform_data->wakeup)
			retval = ts->platform_data->wakeup();
		else
			retval = -ENOSYS;
...
}

quoted

        .init = cyttsp_i2c_init,
.....
}

In the same way it exists an init board specific function that gets
called from cyttsp_core_init() if the board has defined one:

void *cyttsp_core_init(struct cyttsp_bus_ops *bus_ops,
                     struct device *dev, int irq)
{
....
      if (ts->platform_data->init) {
              if (ts->platform_data->init()) {
                      dev_dbg(ts->dev, "%s: Error, platform init failed!\n",
                              __func__);
                      goto error_init;
              }
      }
....
}

This is necessary because each board may need to configure differently
the connection with the chip. For example, OMAP boards will have to
configure the mux pins and so on.

Is this correct? or the driver should not have these handlers and
trust that the platform specific code did all the set up before?

I think it depends on the kind of set up is happening. Static setup
(such as registering memory regsions, configuring IRQ type, etc) is
probably done upfront; if you need to talk to the hardware then custom
method is fine.

Great, I'll keep this init handler then.

Thanks.

--
Dmitry

Thank you and 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html