Re: [PATCH] input synaptics-rmi4: PDT scan cleanup

[Christopher Heiny <cheiny@synaptics.com>]

On 01/10/2014 12:31 PM, Dmitry Torokhov wrote:
> On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
>> >On 01/07/2014 12:33 PM, Christopher Heiny wrote:
>>> > >Eliminates copy-paste code that handled scans of the Page Descriptor
>>> > >Table,
>>> > >replacing it with a single PDT scan routine that invokes a callback
>>> > >function. The scan routine is not static so that it can be used by the
>>> > >firmware update code (under development, not yet submitted).
>>> > >
>>> > >Updated the copyright dates while we were at it.
>> >
>> >Hi Dmitry,
>> >
>> >Could you apply this or provide some feedback on it?  We've got a
>> >pending patch that depends on it, and that pending work will bring the
>> >driver back to a working (if not necessarily beautiful) state.  I don't
>> >want to submit it if this change isn't satisfactory, though.
 >
> Speaking of the devil. I was just thinking about it and I wanted to ask
> you to send me an example of how it is used as I can;t make my mind about
> it.
>
> In general it is OK to submit a few patches at a time even if they are
> depend on each other - it gives me better context (as long as there
> aren't 80 of those so that I down in them ;) ).
>
> Thanks.

No problem!

Currently the rmi_driver.c iterates over the PDT twice during probe():

1) find F01 and reset the ASIC;
2) create and initialize the function devices & count the number of 
interrupt sources.

followed by

3) a loop over the function devices allocating each one's interrupt 
related bitmasks.

Unfortunately, depending on how the function drivers are loaded, a 
function driver might access the device's and the function device's 
bitmasks before step (3) is complete.  This causes all kinds of 
unintended consequences, none of which are amusing.

We considered fixing this by splitting the function device creation and 
initialization, like so:

1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources and create the function devices

followed by

3) a loop over the function devices to allocate their interrupt related 
bitmasks and initialize the function devices

However, this led to function device setup being in two different 
places, which obscured the code and could lead to bugs if someone added 
new code in the wrong place (initialization instead of creation, or vice 
versa).

The PDT scan loops have a lot of redundant boilerplate and were quite 
large compared to the core code that was being iterated, obscuring just 
what the loop was supposed to be doing.  We found it clearer to 
implement a single PDT scan routine, and put the logic for the different 
step in callbacks.  Plus it eliminated the maintenance headaches of 
three (or more, see reflash, below) copy-paste PDT scan loops.

The pending patch (I'll send that right after this note) changes 
rmi_driver.c the order to scan the PDT 3 times:

1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources
3) create and initialize the function devices

Similar logic applies in the reflash code, which has rescan the PDT 
after forcing the touch sensor firmware into bootloader mode (the 
bootloader might have a different register map, unfortunately).  Once we 
had the scan+callback routine in rmi_driver.c, it only made sense for 
the reflash library to use that as well.

				Thanks,
					Chris