[lm-sensors] [PATCH] Apple Motion Sensor driver

[Andrew Morton]

On Wed, 2 Aug 2006 21:15:20 +0200
Michael Hanselmann <linux-kernel at hansmi.ch> wrote:

> This driver adds support for the Apple Motion Sensor (AMS) as found in
> 2005 revisions of Apple PowerBooks and iBooks. It implements both the
> PMU and I²C variants. The I²C driver and mouse emulation is based on
> code by Stelian Pop <stelian at popies.net>, while the PMU driver has been
> developped by myself. HD parking support will be added later.
> 
> ...
>
> +/* There is only one motion sensor per machine */
> +struct ams ams;

That's a somewhat risky name for a global variable.

Then again, nobody else is likely to be so bold as to add another
three-character global ;)

> +/* Call with lock held! */
> +int ams_sensor_attach(void)

Which lock?

> +static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
> +{
> +	s32 result;
> +	int i;
> +
> +	ams_i2c_write(AMS_COMMAND, cmd);
> +	for (i = 0; i < 10; i++) {
> +		mdelay(5);
> +		result = ams_i2c_read(AMS_COMMAND);
> +		if (result == 0 || result & 0x80)
> +			return 0;
> +	}
> +	return -1;
> +}

That's a potentially very long busy wait.

> +static void ams_i2c_set_irq(enum ams_irq reg, char enable)
> +{
> +	if (reg & AMS_IRQ_FREEFALL) {
> +		u8 val = ams_i2c_read(AMS_CTRLX);
> +		if (enable)     val |= 0x80;
> +		else            val &= ~0x80;
> +		ams_i2c_write(AMS_CTRLX, val);
> +	}
> +
> +	if (reg & AMS_IRQ_SHOCK) {
> +		u8 val = ams_i2c_read(AMS_CTRLY);
> +		if (enable)     val |= 0x80;
> +		else            val &= ~0x80;
> +		ams_i2c_write(AMS_CTRLY, val);
> +	}
> +
> +	if (reg & AMS_IRQ_GLOBAL) {
> +		u8 val = ams_i2c_read(AMS_CTRLZ);
> +		if (enable)     val |= 0x80;
> +		else            val &= ~0x80;
> +		ams_i2c_write(AMS_CTRLZ, val);
> +	}
> +}

Please, let's use

	if (a)
		b;
	else
		c;

> +/* Call with lock held! */
> +static void ams_mouse_enable(void)

Which lock?

> +{
> +	s8 x, y, z;
> +
> +	if (ams.idev)
> +		return;
> +
> +	ams_sensors(&x, &y, &z);
> +	ams.xcalib = x;
> +	ams.ycalib = y;
> +	ams.zcalib = z;
> +
> +	ams.idev = input_allocate_device();
> +	if (!ams.idev)
> +		return;
> +
> +	ams.idev->name = "Apple Motion Sensor";
> +	ams.idev->id.bustype = ams.bustype;
> +	ams.idev->id.vendor = 0;
> +	ams.idev->cdev.dev = &ams.of_dev->dev;
> +
> +	input_set_abs_params(ams.idev, ABS_X, -50, 50, 3, 0);
> +	input_set_abs_params(ams.idev, ABS_Y, -50, 50, 3, 0);
> +	input_set_abs_params(ams.idev, ABS_Z, -50, 50, 3, 0);
> +
> +	set_bit(EV_ABS, ams.idev->evbit);
> +	set_bit(EV_KEY, ams.idev->evbit);
> +
> +	if (input_register_device(ams.idev)) {
> +		input_free_device(ams.idev);
> +		ams.idev = NULL;
> +		return;
> +	}
> +
> +	ams.kthread = kthread_run(ams_mouse_kthread, NULL, "kams");
> +	if (IS_ERR(ams.kthread)) {
> +		input_unregister_device(ams.idev);
> +		ams.idev = NULL;
> +		return;

Didn't we just leak ams.idev?  Or does the disconnect handler handle that?

<goes to input_unregister_device() for the API description, comes away
discouraged>

> +		ams.idev = NULL;
> +	}
> +}
> +
> +static ssize_t ams_mouse_show_mouse(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", mouse);
> +}
> +
> +static ssize_t ams_mouse_store_mouse(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	if (sscanf(buf, "%d\n", &mouse) != 1)
> +		return -EINVAL;
> +
> +	mouse = !!mouse;

That statement isn't needed.

> +	mutex_lock(&ams.lock);
> +
> +	if (mouse)
> +		ams_mouse_enable();
> +	else
> +		ams_mouse_disable();
> +
> +	mutex_unlock(&ams.lock);
> +
> +	return count;
> +}
>
> ...
>
> +/* Enables or disables the specified interrupts */
> +static void ams_pmu_set_irq(enum ams_irq reg, char enable)
> +{
> +	if (reg & AMS_IRQ_FREEFALL) {
> +		u8 val = ams_pmu_get_register(AMS_FF_ENABLE);
> +		if (enable)	val |= 0x80;
> +		else		val &= ~0x80;
> +		ams_pmu_set_register(AMS_FF_ENABLE, val);
> +	}
> +
> +	if (reg & AMS_IRQ_SHOCK) {
> +		u8 val = ams_pmu_get_register(AMS_SHOCK_ENABLE);
> +		if (enable)	val |= 0x80;
> +		else		val &= ~0x80;
> +		ams_pmu_set_register(AMS_SHOCK_ENABLE, val);
> +	}
> +
> +	if (reg & AMS_IRQ_GLOBAL) {
> +		u8 val = ams_pmu_get_register(AMS_CONTROL);
> +		if (enable)	val |= 0x80;
> +		else		val &= ~0x80;
> +		ams_pmu_set_register(AMS_CONTROL, val);
> +	}
> +}

Again - coding style is wrong.

> +/* Call with lock held! */
> +int __init ams_pmu_init(struct device_node *np)

Which lock??

> +{
> +	u32 *prop;
> +	int result;
> +
> +	mutex_lock(&ams.lock);

Obviously not that one...