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

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

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.

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

Thanks.

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

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.

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?

        .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.

Thanks.

-- 
Dmitry