Re: [RFC PATCH 0/1] input/touchscreen: Synaptics Touchscreen Driver

[Christopher Heiny <cheiny@synaptics.com>]

On 08/25/2010 02:29 AM, Naveen Kumar GADDIPATI wrote:
> Hi Christopher,
>
> Some generic review comments for this patch
>
> 1. Normally, defconfig should be a separate patch. You need not
> post the entire defconfig, but only the changes to the Makefiles and
> the Kconfig files.
>
> 2. All RMI specific files can be moved to a separate driver folder
> in input/ like drivers/input/rmi/*.
>
> 3. Lots of Hungarian notation in the code. Please refer this snippet.
>
>> ------------------------------------
>> CODE CONTAINS HUNGARIAN NOTATION
>>
>> Some coding guidlines at some companies require that you type the name
>> of your variables using the infamous hungarian notation:
>> http://en.wikipedia.org/wiki/Hungarian_notation
>>
>> We have an internal coding guideline that touches on the
>> subject:
>> http://swadvice.lud.stericsson.com/guideline.aspx?nr=57&ver=latest
>>
>> According to the Linux kernel inventor Linus Torvalds in his document
>> Documentation/CodingStyle in the Linux kernel:
>>
>> "Encoding the type of a function into the name (so-called Hungarian
>> notation) is brain damaged-the compiler knows the types anyway and can
>> check those, and it only confuses the programmer."
>>
>> Remove all instances of hungariang notation in the code please.
>> Includes prefixing pointers with p_*, naming structs with s_*, enums
>> with e_*, suffixes like *32b etc etc.
>> -----------------------------------
>>
>
> We are also using the Synaptics RMI4 touch pad on our U8500 platform.
> We had to make some changes to these posted drivers to make it work on
> our platform. Also, we modified the touch screen driver to be compliant
> to the kernel coding guidelines, but as a stand alone driver though.
>
> I will soon post out an RFC for our patch which we got working on our board.
> Please do have a look and we would like you guys to incorporate these changes
> into your final patch sets as well, so that we can avoid any re-works later on
> when your patch set gets merged into the mainline kernel.

Hi Naveen,

Thanks very much for your input!

We'll be changing the patch structure with our next submission, as the 
current patch file is getting kind of large and clumsy.  We'll split the 
defconfig out into its own patch at that point.

We'll also be eliminating the Hungarian notation in the next submission.

Your suggestion regarding the location of the RMI specific files is 
interesting.  We'll defer to Dmitry on where he'd like to see those 
files placed, though.

Unfortunately, the patch you submitted appears to be based on the older 
monolithic RMI3/Android driver, which is limited in functionality and 
maintainability.  Also, your submitted patch appears to be specific to 
the U8500 platform.

The aim of the current work is to develop a platform independent modular 
driver that will support all RMI4 functionality in a generic fashion. 
We'll be more than happy to work with your team to develop support for 
the U8500 under the modular structure.  If we proceed in that direction, 
it eliminates redundant effort and the need for any rework when our 
driver gets merged into the mainline.

I think the first step there should be for your team to resubmit your 
work as modifications of the current development effort, rather than 
backporting the current effort onto the obsolete framework.  Although 
that involves upfront work for your team in the near term, it provides a 
reduction in effort all around going forward:
     - your team doesn't need to keep back-patching changes onto
       the obsolete driver structure
     - our team doesn't need to keep forward-patching U8500
       related changes into the current driver structure

In a recent email conversation with me, Dmitry indicated that he will be 
creating a development branch for this work in the near future.  That 
will greatly facilitate this effort.

					Thanks,
						Chris