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

From: Olof Johansson
Date: Thu Dec 15 2005 - 14:49:48 EST


On Thu, Dec 15, 2005 at 12:31:08AM +0100, Michael Hanselmann wrote:
> This patch adds support for relayfs to the appletouch driver to make debugging
> easier.

I think I agree with previous comments regarding debug code in the driver:
It's unlikely to ever be used by more than a couple of people at very
rare occasions (new hardware releases), and the barrier to using it is
still high; new users need to learn how to parse the data anyway. I don't
see a reason to include this in mainline.

That aside, comments on the patch below.

> diff -rup linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c b/drivers/usb/input/appletouch.c
> --- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 22:44:24.000000000 +0100
> +++ b/drivers/usb/input/appletouch.c 2005-12-15 00:25:09.000000000 +0100
> @@ -6,6 +6,8 @@
> * Copyright (C) 2005 Stelian Pop (stelian@xxxxxxxxxx)
> * Copyright (C) 2005 Frank Arnold (frank@xxxxxxxxxxxxxxxxxxxx)
> * Copyright (C) 2005 Peter Osterlund (petero2@xxxxxxxxx)
> + * Copyright (C) 2005 Parag Warudkar (parag.warudkar@xxxxxxxxx)
> + * Copyright (C) 2005 Michael Hanselmann (linux-kernel@xxxxxxxxx)
> *
> * Thanks to Alex Harper <basilisk@xxxxxxxxxx> for his inputs.
> *
> @@ -35,6 +37,10 @@
> #include <linux/input.h>
> #include <linux/usb_input.h>
>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +#include <linux/relayfs_fs.h>
> +#endif
> +
> /* Apple has powerbooks which have the keyboard with different Product IDs */
> #define APPLE_VENDOR_ID 0x05AC
>
> @@ -73,6 +79,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);
>
> /* maximum pressure this driver will report */
> #define ATP_PRESSURE 300
> +

Whitespace change

> /*
>
> * multiplication factor for the X and Y coordinates.
> * We try to keep the touchpad aspect ratio while still doing only simple
> @@ -124,7 +131,7 @@ struct atp {
> if (debug) printk(format, ##a); \
> } while (0)
>
> -MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold");
> +MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Parag Warudkar, Michael Hanselmann");
> MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
> MODULE_LICENSE("GPL");
>
> @@ -132,6 +139,68 @@ static int debug = 1;
> module_param(debug, int, 0644);
> MODULE_PARM_DESC(debug, "Activate debugging output");
>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +static int relayfs;
> +module_param(relayfs, int, 0444);
> +MODULE_PARM_DESC(relayfs, "Activate relayfs support");
> +
> +struct rchan *rch;
> +struct rchan_callbacks *rcb;

static, please.

> +
> +static inline void atp_relayfs_dump(struct atp *dev)
> +{
> + /* "rch" is NULL if relayfs is disabled */
> + if (rch && dev->data) {
> + relay_write(rch, dev->data, dev->urb->actual_length);
> + }
> +}
> +
> +static int appletouch_relayfs_init(void)
> +{
> + // Make sure the variables aren't initialized to some bogus value when
> + // relayfs is disabled
> + rcb = NULL;
> + rch = NULL;

Huh? BSS is initialized to 0, this is unneccessary.
(Also, the comment is C++-style, please use /* */ comments.)

> +
> + if (relayfs) {

Please do:
if (!relayfs)
return 0;

To save indentation.

> + rcb = kmalloc(sizeof(struct rchan_callbacks), GFP_KERNEL);
> + if (!rcb)
> + return -ENOMEM;
> +
> + rcb->subbuf_start = NULL;
> + rcb->buf_mapped = NULL;
> + rcb->buf_unmapped = NULL;
> +
> + rch = relay_open("atpdata", NULL, 256, 256, NULL);
> + if (!rch) {
> + kfree(rcb);
> + return -ENOMEM;

ENOMEM for a failed open? Seems odd to me.
Should you also set rcb = NULL?

> + }
> +
> + printk(KERN_INFO "appletouch: Relayfs enabled.\n");
> + }
> +
> + return 0;
> +}
> +
> +static void appletouch_relayfs_exit(void)
> +{
> + if (rch) {
> + printk(KERN_INFO "appletouch: Relayfs disabled\n");
> + relay_close(rch);
> + rch = NULL;
> + }
> + if (rcb) {
> + kfree(rcb);
> + rcb = NULL;

No need to check arguments to kfree.

> + }
> +}
> +#else
> +static inline void atp_relayfs_dump(struct atp *dev) { }
> +static int appletouch_relayfs_init(void) { return 0; }
> +static void appletouch_relayfs_exit(void) { }
> +#endif
> +
> static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
> int *z, int *fingers)
> {
> @@ -194,6 +263,8 @@ static void atp_complete(struct urb* urb
> goto exit;
> }
>
> + atp_relayfs_dump(dev);
> +
> /* reorder the sensors values */
> for (i = 0; i < 8; i++) {
> /* X values */
> @@ -297,6 +368,11 @@ exit:
> static int atp_open(struct input_dev *input)
> {
> struct atp *dev = input->private;
> + int result;
> +
> + result = appletouch_relayfs_init();
> + if (result < 0)
> + return result;

Sounds harsh to deny USB open just because relayfs couldn't be setup.

Actually, this way you could just make the init function not return
anything; it doesn't matter if it fails or not -- life will go on.

>
> if (usb_submit_urb(dev->urb, GFP_ATOMIC))
> return -EIO;
> @@ -310,6 +386,8 @@ static void atp_close(struct input_dev *
> struct atp *dev = input->private;
>
> usb_kill_urb(dev->urb);
> + appletouch_relayfs_exit();
> +
> dev->open = 0;
> }
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@xxxxxxxxxx
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
-
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/