On Mon, Aug 23, 2010 at 08:19:44PM -0700, James wrote:
> On 08/19/2010 05:10 PM, Greg KH wrote:
>> On Thu, Aug 19, 2010 at 04:28:39PM -0700, James wrote:
>>
> ...
>>> The above is enabled via CONFIG_TOUCHSCREEN_CY8CTMG110_MULTIPLE_INPUT
>>> with the default set to 'Y' (to enable current Qt 4.7 based applications
>>> to leverage this capability)
>>>
> ...
>>> +config TOUCHSCREEN_CY8CTMG110_MULTIPLE_INPUT
>>> + bool "cy8ctmg110 multiple interface support"
>>> + default y
>>>
>> If it's default y, why even have a config option?
>>
> So that it can be turned off if someone doesn't want that behavior.
Why would they not want this? Why make it a config option at all?
>> And note, we don't do default y for new config options, unless you can't
>> boot your machine without it.
>>
> But it is 'Y' as a sub-option of the TOUCHSCREEN_CY8CTMG110:
Doesn't matter.
> + depends on TOUCHSCREEN_CY8CTMG110
>
> so unless the user says Y/m to TOUCHSCREEN_CY8CTMG110, the MULTIPLE_INPUT
> won't be enabled, correct?
True, but again, it doesn't matter. Either default to 'N' or just don't
have a config option.
>>> --- a/drivers/input/touchscreen/cy8ctmg110_ts.c
>>> +++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
>>> @@ -81,8 +81,13 @@ struct ts_event {
>>> * The touch driver structure.
>>> */
>>> struct cy8ctmg110 {
>>> +#ifdef CONFIG_TOUCHSCREEN_CY8CTMG110_MULTIPLE_INPUT
>>> + struct input_dev *input[MAX_FINGERS];
>>> + char phys[MAX_FINGERS][32];
>>> +#else
>>> struct input_dev *input;
>>> char phys[32];
>>> +#endif
>>>
>> No #ifdefs in .c files please.
>>
> I'll work on an improvement. Long term, I suspect this patch will
> disappear in favor of a full multi-touch-protocol implementation. However,
> if you want to deploy an entire multi-touch stack *now*, user space doesn't
> use the multi-touch protocol, so we're stuck with having to write the
> driver to the legacy mechanism.
Doesn't that sound like a userspace bug? Why are you trying to fix it
in the kernel?
> A positive attribute of placing that logic in #ifdef's is that it clearly
> isolates that code path within the .c file such that a maintainers can more
> readily remove that code without having oddly obfuscated static inline
> functions floating around (I'm not arguing for #ifdef approaches in
> general, just noting a benefit) One might just redefine MAX_FINGERS to 1
> and remove all of the #ifdef logic... but in the real multi-touch-protocol
> solution, we won't need to have code capable of creating one input device
> per contact point.
Sorry, but no, no #ifdefs please. That way lies madness in the long
run.
And if someone really wanted to revert these changes, a simple 'git
revert' would be all that is needed sometime in the future. Don't think
that adding #ifdef would help that effort.
thanks,
greg k-h
_______________________________________________
MeeGo-dev mailing list
[email protected]
http://lists.meego.com/listinfo/meego-dev
