Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

[Dmitry Torokhov]

Hi,

On 12/14/05, Michael Hanselmann <linux-kernel@xxxxxxxxx> wrote:
>  *
> + * Nov 2005 - Parag Warudkar
> + *  o Added ability to export data via relayfs
> + *
> + * Nov/Dec 2005 - Michael Hanselmann
> + *  o Compile relayfs support only if enabled in the kernel
> + *  o Enable relayfs only if requested by the user
> + *

We have an SCM, not need to have a changelog in the driver.

>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +struct rchan* rch = NULL;
> +struct rchan_callbacks* rcb = NULL;

Initializing with 0/NULL adds to the size of the image for no reason.

>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +static int relayfs = 0;

Same here.

> +module_param(relayfs, int, 0444);
> +MODULE_PARM_DESC(relayfs, "Activate relayfs support");
> +#endif
> +
>  static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
>                             int *z, int *fingers)
>  {
> @@ -194,6 +219,13 @@ static void atp_complete(struct urb* urb
>                goto exit;
>        }
>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +       /* "rch" is NULL if relayfs is not enabled */
> +       if (rch && dev->data) {
> +               relay_write(rch, dev->data, dev->urb->actual_length);
> +       }
> +#endif
> +

Haveing ifdefs in the middle of the code is not too nice. can we
please have something like this:

#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
static inline void appletouch_relayfs_dump(...)
{
...
}
static int appletouch_relayfs_init(void)
{
...
}
static void appletouch_relayfs_exit()
{
...
}
#else
static inline void appletouch_relayfs_dump(...) { }
static int appletouch_relayfs_init(void) { return 0; }
static void appletouch_relayfs_exit() { }
#endif

Also, would not it be better to initialize relayfs only when device is
opened because there won't be any data otehrwise?

--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/