Re: hp accelerometer: add freefall detection

[Ãric Piel]

From: Pavel Machek <pavel@xxxxxxx>
> 
> This adds freefall handling to hp_accel driver.  According to HP, it
> should just work, without us having to set the chip up by hand.
> 
> hpfall.c is example .c program that parks the disk when accelerometer
> detects free fall.  It should work; for now, it uses fixed 20seconds
> protection period.
> 
> Signed-off-by: Pavel Machek <pavel@xxxxxxx>
> Cc: Thomas Renninger <trenn@xxxxxxx>
> Cc: ïric Piel <eric.piel@xxxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Hi Pavel,
Sorry for being amazingly late, reading the latest patches from
Giuseppe, I noticed some parts in this patch which are strange.
Basically you scrap all the init and all the poweroff. Some of the
things there were put for some purpose, and I wonder if you've verified
everything keeps working correctly.


> @@ -110,26 +114,13 @@ static void lis3lv02d_get_xyz(acpi_handl
>  void lis3lv02d_poweroff(acpi_handle handle)
>  {
>  	adev.is_on = 0;
> -	/* disable X,Y,Z axis and power down */
> -	adev.write(handle, CTRL_REG1, 0x00);
So we are not turning off the device on poweroff()?! Why? Did you notice
problems when we were doing it?

Actually I've always wondered if it was worthy to turn it off because
I've never been able to measure a difference in power consumption. Have
you done the measurement and noticed there is no effect? I'd be glad we
can get rid of this power management thing, but then we can scratch much
more than just this line!


> -	/*
> -	 * BDU: LSB and MSB values are not updated until both have been read.
> -	 *      So the value read will always be correct.
> -	 * IEN: Interrupt for free-fall and DD, not for data-ready.
> -	 */
> -	adev.read(handle, CTRL_REG2, &val);
> -	val |= CTRL2_BDU | CTRL2_IEN;
> -	adev.write(handle, CTRL_REG2, val);
This part is rather scary. Did you read the comment? If you don't
activate BDU you cannot be sure that the data you read is valid. I see
no reason why this would interfere with the free-fall detection so
unless you have a really good case, let's keep it. On the other hand, I
don't mind removing IEN if you've noticed that it's not useful or
interfering.

Eric
--
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/