Re: [PATCH v4] iio: mma8452: add freefall detection for Freescale's accelerometers

[Martin Kepplinger]

Am 2016-01-13 um 14:53 schrieb Peter Meerwald-Stadler:
> Hello,
> 
>> This adds freefall event detection to the supported devices. It adds
>> the in_accel_x&y&z_mag_falling_en iio event attribute, which activates
>> freefall mode.
>>
>> In freefall mode, the current acceleration magnitude (AND combination
>> of all axis values) is compared to the specified threshold.
>> If it falls under the threshold (in_accel_mag_falling_value),
>> the appropriate IIO event code is generated.
>>
>> This is what the sysfs "events" directory for these devices looks
>> like after this change:
>>
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_mag_falling_period
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_mag_falling_value
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_mag_rising_period
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_mag_rising_value
>> -r--r--r--    4096 Oct 23 08:45 in_accel_scale
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_x&y&z_mag_falling_en
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_x_mag_rising_en
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_y_mag_rising_en
>> -rw-r--r--    4096 Oct 23 08:45 in_accel_z_mag_rising_en
> 
> I think it is a very good idea to have this for review, thanks!
> 
> minor comments below
> 
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Christoph Muellner <christoph.muellner@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> revision history
>> ----------------
>> v1:
>>         initial post
>> v2:
>>         build all from correct event and channel spec structs
>> v3:
>>         rising and falling are treated as equal now. Until last time, I had
>>         misunderstood the iio events' user API definition. This works and
>>         values always reflect the current state of operation.
>> v4:
>> 	fix error that caused a build warning
>>
>>  drivers/iio/accel/mma8452.c | 156 +++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 139 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index ccc632a..9534c3a 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -15,7 +15,7 @@
>>   *
>>   * 7-bit I2C slave address 0x1c/0x1d (pin selectable)
>>   *
>> - * TODO: orientation / freefall events, autosleep
>> + * TODO: orientation events, autosleep
>>   */
>>  
>>  #include <linux/module.h>
>> @@ -416,6 +416,46 @@ fail:
>>  	return ret;
>>  }
>>  
>> +static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
>> +{
>> +	int val;
>> +	const struct mma_chip_info *chip = data->chip_info;
>> +
>> +	val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	return !(val & MMA8452_FF_MT_CFG_OAE);
> 
> I'd appreciate a comment what the possible return values of this function 
> are and their purpose
> 

would it also be clearer if the return value would be bool?

>> +}
>> +
>> +static int mma8452_set_freefall_mode(struct mma8452_data *data, u8 state)
> 
> state seems to be used as a bool, maybe it should be bool
> 

You're right. I'll change this.

>> +{
>> +	int val, ret;
> 
> strictly, only one of the two variable is necessary
> 

true.

>> +	const struct mma_chip_info *chip = data->chip_info;
>> +
>> +	val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	if (state && !(mma8452_freefall_mode_enabled(data))) {
>> +		val |= BIT(idx_x + chip->ev_cfg_chan_shift);
>> +		val |= BIT(idx_y + chip->ev_cfg_chan_shift);
>> +		val |= BIT(idx_z + chip->ev_cfg_chan_shift);
>> +		val &= ~MMA8452_FF_MT_CFG_OAE;
>> +	} else if (!state && mma8452_freefall_mode_enabled(data)) {
>> +		val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
>> +		val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
>> +		val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
>> +		val |= MMA8452_FF_MT_CFG_OAE;
>> +	}
>> +
>> +	ret = mma8452_change_config(data, chip->ev_cfg, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>>  static int mma8452_set_hp_filter_frequency(struct mma8452_data *data,
>>  					   int val, int val2)
>>  {
>> @@ -609,12 +649,22 @@ static int mma8452_read_event_config(struct iio_dev *indio_dev,
> 
> I find the return values of this functions difficult to understand, 
> comment would be good

This is part of struct iio_info so shouldn't it be documented elsewhere,
not in a driver?

thanks for reviewing!

                               martin