Re: [PATCH 1/5] Add touchscreen filter API

["Nelson Castillo" <arhuaco@freaks-unidos.net>]

On Fri, Jan 16, 2009 at 1:55 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 16 Jan 2009 00:05:20 -0500 "Nelson Castillo" <arhuaco@freaks-unidos.net> wrote:
>
>> On Thu, Jan 15, 2009 at 6:47 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > On Tue, 13 Jan 2009 18:39:39 -0500
>> > Nelson <arhuaco@freaks-unidos.net> wrote:
>> >
>> >> From: Nelson Castillo <arhuaco@freaks-unidos.net>
>> >>
>> >> Generic filters are not useful by themselves. They define an API that
>> >> actual filters have to implement.
>> >>
>> >> Signed-off-by: Nelson Castillo <arhuaco@freaks-unidos.net>
>> >> ---
>>
>> (cut)
>>
>> >> +int ts_filter_create_chain(struct platform_device *pdev,
>> >> +                        struct ts_filter_api **api, void **config,
>> >> +                        struct ts_filter **list, int count_coords)
>> >> +{
>> >> +     int count = 0;
>> >> +     struct ts_filter *last = NULL;
>> >> +
>> >> +     if (!api)
>> >> +             return 0;
>> >> +
>> >> +     while (*api && count < MAX_TS_FILTER_CHAIN) {
>> >
>> >Why does MAX_TS_FILTER_CHAIN exist?  Why impose limits?
>>
>> "api" is actually a static array that is not zero_terminated by
>> convention. It can be zero-terminated, though. This check avoids
>> overrunning the loop if MAX_TS_FILTER_CHAIN filters are used.
>>
>> This snippet is taken from mach-gta02.c (link below) and
>> "filter_sequence" is what
>> we pass as the "api" array.
>>
>> static struct s3c2410_ts_mach_info gta02_ts_cfg = {
>>         .delay = 10000,
>>         .presc = 0xff, /* slow as we can go */
>>         .filter_sequence = {
>>                 [0] = &ts_filter_group_api,
>>                 [1] = &ts_filter_median_api,
>>                 [2] = &ts_filter_mean_api,
>>                 [3] = &ts_filter_linear_api,
>>         },
>>         .filter_config = {
>>                 [0] = &gta02_ts_group_config,
>>                 [1] = &gta02_ts_median_config,
>>                 [2] = &gta02_ts_mean_config,
>>                 [3] = &gta02_ts_linear_config,
>>         },
>> };
>
> so...  change the interface to require that the array be
> null-terminated, then fix callers.
>
> Null-terminating variable-length arrays in this manner is a quite
> common paradigm in the kernel.

Sure. We will change it.

>> >> diff --git a/drivers/input/touchscreen/ts_filter.c b/drivers/input/touchscreen/ts_filter.c
>> >> new file mode 100644
>> >> index 0000000..1508388
>> >> --- /dev/null
>> >> +++ b/drivers/input/touchscreen/ts_filter.c
>> >
>> > Confused.  Nothing appears to use the two functions which this file adds.
>>
>> http://git.openmoko.org/?p=kernel.git;a=shortlog;h=stable-tracking
>>
>> This branch is tracking the upstream Linux kernel and patches are
>> rebased/updated
>> when things change upstream. Openmoko has been doing this since one
>> goal is to be able to use upstream kernels with no patches for OM
>> devices.
>>
>> This is the driver can use the filters (s3c2410_ts.c) if they are
>> defined in the machine configuration:
>>
>> http://git.openmoko.org/?p=kernel.git;a=blob_plain;f=drivers/input/touchscreen/s3c2410_ts.c;hb=stable-tracking
>>
>> We are ready to send this driver upstream but before we need to trim
>> this file (mach-gta02.c) and send it:
>>
>> http://git.openmoko.org/?p=kernel.git;a=blob_plain;f=arch/arm/mach-s3c2440/mach-gta02.c;hb=stable-tracking
>>
>> We are depending on this mach-gta02.c file now and a streamlined
>> version of it must be sent upstream so that we can start adding
>> drivers. This file in turn depends on patches that are being sent
>> upstream also.
>>
>> Thus we have a chicken and egg problem and we decided to send the
>> filters for review because they are in the stable-tracking branch and
>> they need to be suitable for upstream inclusion / included upstream.
>
> Please hold off on the unused code until some code which actually uses
> is is submitted as well.

Sure, thanks for the review.