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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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
> <dmitry.torokhov@gmail.com> wrote:
> > 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.

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

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