linux-kernel - Re: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101124195523.GF27368@core.coreip.homeip.net>
Date:	Wed, 24 Nov 2010 11:55:24 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Fabien Marteau <fabien.marteau@...adeus.com>
Cc:	Scott Moreau <oreaus@...il.com>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] input: joystick:   Adding Austria Microsystem AS5011
 joystick driver

Hi Fabien,

On Tue, Nov 23, 2010 at 05:36:14PM +0100, Fabien Marteau wrote:
> Driver for EasyPoint AS5011 2 axis joystick chip. This chip is plugged
> on an I2C bus. It as been tested on ARM processor (i.MX27).
> 

Thank you for the patch. In addiitoon to comments you got from the other
reviewers I'd recommend switching to threaded IRQ instead of using a
separate workqueue, it greatly simplifies the code.

> +
> +static irqreturn_t button_interrupt(int irq, void *dev_id)
> +{
> +	struct as5011_platform_data *plat_dat =
> +		(struct as5011_platform_data *)dev_id;
> +	int ret;
> +
> +	ret = gpio_get_value(plat_dat->button_gpio);
> +	if (ret)
> +		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 0);
> +	else
> +		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 1);
> +	input_sync(plat_dat->input_dev);
> +	return IRQ_HANDLED;

That appears to be a hard IRQ; are you sure that gpio_get_value will not
sleep?

> +
> +	plat_dat->input_dev->name = "Austria Microsystem as5011 joystick";
> +	plat_dat->input_dev->uniq = "Austria0";
> +	plat_dat->input_dev->id.bustype = BUS_I2C;
> +	plat_dat->input_dev->phys = "/dev/input/event0";

Phys is not the path in device tree but rather physical device
connection path within the system. If there is not known connection path
it is better just leave it as NULL.

> +	plat_dat->input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	plat_dat->input_dev->keybit[BIT_WORD(BTN_JOYSTICK)] =
> +					BIT_MASK(BTN_JOYSTICK);
> +
> +	input_set_abs_params(plat_dat->input_dev,
> +			     ABS_X,
> +			     AS5011_MIN_AXIS,
> +			     AS5011_MAX_AXIS,
> +			     AS5011_FUZZ,
> +			     AS5011_FLAT);
> +	input_set_abs_params(plat_dat->input_dev,
> +			     ABS_Y,
> +			     AS5011_MIN_AXIS,
> +			     AS5011_MAX_AXIS,
> +			     AS5011_FUZZ,
> +			     AS5011_FLAT);
> +
> +	plat_dat->button_irq_name =
> +		kmalloc(sizeof(unsigned char)*SIZE_NAME,
> +					 GFP_KERNEL);

Just why does it have to be separately allocated? I'd even stick with
"as5011" for all IRQ names.

> +
> +	queue_work(plat_dat->workqueue, &plat_dat->update_axes_work);
> +

The device is quite likely not opened here so why do you want to send
events?

Thanks.

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