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

[Christopher Heiny <cheiny@synaptics.com>]

Ping - any followup on this and related patches?

On 01/10/2014 01:50 PM, Christopher Heiny wrote:
> 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
> --
> 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


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated