Re: [PATCH] LIS3LV02Dx Accelerometer driver (take 4)

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

On Sat, 18 Oct 2008 21:05:12 +0200
Eric Piel <eric.piel@tremplin-utc.net> wrote:

>
> ...
>
> LIS3LV02Dx Accelerometer driver
> 
> This adds a driver to the accelerometer sensor found in several HP laptops
> (under the commercial names of "HP Mobile Data Protection System 3D" and
> "HP 3D driveguard"). It tries to have more or less the same interfaces
> as the hdaps and other accelerometer drivers: in sysfs and as a
> joystick. 
> 
> This driver was first written by Yan Burman. Eric Piel has updated it
> and slimed it up (including the removal of an interface to access to the
> free-fall feature of the sensor because it is not reliable enough for
> now).  Several people have contributed to the database of the axes.
>
> ...
>
> +struct acpi_lis3lv02d {
> +	struct acpi_device	*device;   /* The ACPI device */
> +	struct input_dev	*idev;     /* input device */
> +	struct task_struct	*kthread;  /* kthread for input */
> +	struct timer_list	poff_timer; /* for automatic power off */
> +	struct semaphore	poff_sem;  /* lock to handle on/off timer */

OK, this can't be a mutex because it's upped from a timer handler
(which seems a bit odd, but I assume you know what you're doing ;))

> +	struct platform_device	*pdev;     /* platform device */
> +	atomic_t		count;     /* interrupt count after last read */
> +	int			xcalib;    /* calibrated null value for x */
> +	int			ycalib;    /* calibrated null value for y */
> +	int			zcalib;    /* calibrated null value for z */
> +	unsigned char		is_on;     /* whether the device is on or off */
> +	unsigned char		usage;     /* usage counter */
> +	struct axis_conversion	ac;        /* hw -> logical axis */
> +};
> +
> +static struct acpi_lis3lv02d adev = {
> +	.poff_sem   = __SEMAPHORE_INITIALIZER(adev.poff_sem, 1),
> +	.usage       = 0,

the .foo=0 isn't strictly needed.  It can be retained if you believe it
has documentary value.

> +};
> +
> +static int lis3lv02d_remove_fs(void);
> +static int lis3lv02d_add_fs(struct acpi_device *device);
> +
> +/* For automatic insertion of the module */
> +static struct acpi_device_id lis3lv02d_device_ids[] = {
> +	{"HPQ0004", 0}, /* HP Mobile Data Protection System PNP */
> +	{"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, lis3lv02d_device_ids);
> +
> +/**
> + * acpi_init() - ACPI _INI method: initialize the device.

Should be "lis3lv02d_acpi_init"

> + * @handle: the handle of the device
> + *
> + * Returns AE_OK on success.
> + */
> +static inline acpi_status lis3lv02d_acpi_init(acpi_handle handle)
> +{
> +	return acpi_evaluate_object(handle, METHOD_NAME__INI, NULL, NULL);
> +}
> +
> +/**
> + * lis3lv02d_acpi_read() - ACPI ALRD method: read a register

All the kerneldoc comments include the () after the function name. 
That is unconventional and I do not know what effect it will have upon
kerneldoc processing and output.

> + * @handle: the handle of the device
> + * @reg:    the register to read
> + * @ret:    result of the operation
> + *
> + * Returns AE_OK on success.
> + */
> +static acpi_status lis3lv02d_acpi_read(acpi_handle handle, int reg, u8 *ret)
> +{
> +	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> +	struct acpi_object_list args = { 1, &arg0 };
> +	unsigned long lret;
> +	acpi_status status;
> +
> +	arg0.integer.value = reg;
> +
> +	status = acpi_evaluate_integer(handle, "ALRD", &args, &lret);
> +	*ret = lret;
> +	return status;
> +}
> +
>
> ...
>
> +static int lis3lv02d_resume(struct acpi_device *device)
> +{
> +	/* put back the device in the right state (ACPI might turn it on) */
> +	down(&adev.poff_sem);
> +	if (adev.usage > 0)
> +		lis3lv02d_poweron(device->handle);
> +	else
> +		lis3lv02d_poweroff(device->handle);
> +	up(&adev.poff_sem);
> +	return 0;
> +}

This function is unused if CONFIG_PM=n.

Please move this inside the ifdef then add

#else
#define lis3lv02d_suspend NULL
#define lis3lv02d_resume NULL
#endif

and remove the ifdefs in the lis3lv02d_driver definition.  Standard
stuff.


>
> ...
>
> +static void lis3lv02d_poweroff_timeout(unsigned long data)
> +{
> +	struct acpi_lis3lv02d *dev = (void *)data;
> +
> +	up(&dev->poff_sem);
> +	lis3lv02d_poweroff(dev->device->handle);
> +	down(&dev->poff_sem);

eek, no, we cannot down a semaphore from a timer handler!  It will lead
to might_sleep() warnings, scheduling-in-atomic warnings and kernel
deadlocks.

> +}
> +
>
> ...
>
> +static int lis3lv02d_remove(struct acpi_device *device, int type)
> +{
> +	if (!device)
> +		return -EINVAL;
> +
> +	lis3lv02d_joystick_disable();
> +	del_timer(&adev.poff_timer);

Should this be del_timer_sync()?

Bear in mind that the timer handler might be running on another CPU
after del_timer() returns...

> +	lis3lv02d_poweroff(device->handle);
> +
> +	return lis3lv02d_remove_fs();

so the above things can occur in parallel with the execution of the
timer handler.

>
> ...
>