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

[Andrew Morton <akpm@linux-foundation.org>]

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.

> >> 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.