Irina Tirdea schrieb am 27.01.2015 um 19:41:
> Add support for Freescale MMA9553L Intelligent Pedometer Platform.
>
> The following functionalities are supported:
> - step counter (counts the number of steps using a HW register)
> - step detector (generates an iio event at every step the user takes)
> - activity recognition (rest, walking, jogging, running)
> - speed
> - calories
> - distance
>
> To get accurate pedometer results, the user's height, weight and gender
> need to be configured.
>
> The specifications can be downloaded from:
> http://www.freescale.com/files/sensors/doc/ref_manual/MMA955xLSWRM.pdf
> http://www.freescale.com/files/sensors/doc/ref_manual/MMA9553LSWRM.pdf
>
Hi Irina,
I still spotted a few issues and have some recommendations which you
could address in a follow-up patch. Also, there are some minor style
issues (spaces after cast, alignment issues, unnecessary blank lines),
which checkpatch.pl --strict reveals.
One other issue, where I would like to get the opinion of the other
maintainers as well, concerns some of the functions you added to
mma9551_core.c. The way they are used so far doesn't pose a threat, yet.
But I am worried about the __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS],
where the elements are accessed in relation to the len parameter. So,
should there be a check in those functions to prevent buffer overrun
problems, or just be very careful when using those functions?
> Signed-off-by: Irina Tirdea <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-iio | 49 +-
> drivers/iio/accel/Kconfig | 10 +
> drivers/iio/accel/Makefile | 1 +
> drivers/iio/accel/mma9551_core.c | 183 +++++
> drivers/iio/accel/mma9551_core.h | 17 +-
> drivers/iio/accel/mma9553.c | 1334 +++++++++++++++++++++++++++++++
> 6 files changed, 1589 insertions(+), 5 deletions(-)
> create mode 100644 drivers/iio/accel/mma9553.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 588620e..9a70c31 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -343,7 +343,30 @@ Description:
> production inaccuracies). If shared across all channels,
> <type>_calibscale is used.
>
> -What: /sys/bus/iio/devices/iio:deviceX/in_steps_calibheight
> +What: /sys/bus/iio/devices/iio:deviceX/in_activity_calibgender
> +What: /sys/bus/iio/devices/iio:deviceX/in_energy_calibgender
> +What: /sys/bus/iio/devices/iio:deviceX/in_distance_calibgender
> +What: /sys/bus/iio/devices/iio:deviceX/in_velocity_calibgender
> +KernelVersion: 3.20
> +Contact: [email protected]
> +Description:
> + Gender of the user (e.g.: male, female) used by some pedometers
> + to compute the stride length, distance, speed and activity
> + type.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_activity_calibgender_available
> +What: /sys/bus/iio/devices/iio:deviceX/in_energy_calibgender_available
> +What: /sys/bus/iio/devices/iio:deviceX/in_distance_calibgender_available
> +What: /sys/bus/iio/devices/iio:deviceX/in_velocity_calibgender_available
> +KernelVersion: 3.20
> +Contact: [email protected]
> +Description:
> + Lists all available gender values (e.g.: male, female).
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_activity_calibheight
> +What: /sys/bus/iio/devices/iio:deviceX/in_energy_calibheight
> +What: /sys/bus/iio/devices/iio:deviceX/in_distance_calibheight
> +What: /sys/bus/iio/devices/iio:deviceX/in_velocity_calibheight
> KernelVersion: 3.19
> Contact: [email protected]
> Description:
> @@ -818,6 +841,14 @@ What: /sys/.../events/in_tempY_roc_falling_period
> What: /sys/.../events/in_accel_x&y&z_mag_falling_period
> What: /sys/.../events/in_intensity0_thresh_period
> What: /sys/.../events/in_proximity0_thresh_period
> +What: /sys/.../events/in_activity_still_thresh_rising_period
> +What: /sys/.../events/in_activity_still_thresh_falling_period
> +What: /sys/.../events/in_activity_walking_thresh_rising_period
> +What: /sys/.../events/in_activity_walking_thresh_falling_period
> +What: /sys/.../events/in_activity_jogging_thresh_rising_period
> +What: /sys/.../events/in_activity_jogging_thresh_falling_period
> +What: /sys/.../events/in_activity_running_thresh_rising_period
> +What: /sys/.../events/in_activity_running_thresh_falling_period
> KernelVersion: 2.6.37
> Contact: [email protected]
> Description:
> @@ -1142,6 +1173,12 @@ Description:
> This attribute is used to get/set the integration time in
> seconds.
>
> +What: /sys/.../iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_integration_time
> +KernelVersion: 3.20
> +Contact: [email protected]
> +Description:
> + Number of seconds in which to compute speed.
> +
> What: /sys/bus/iio/devices/iio:deviceX/in_rot_quaternion_raw
> KernelVersion: 3.15
> Contact: [email protected]
> @@ -1170,13 +1207,17 @@ Description:
> present, output should be considered as processed with the
> unit in milliamps.
>
> +What: /sys/.../iio:deviceX/in_energy_en
> +What: /sys/.../iio:deviceX/in_distance_en
> +What: /sys/.../iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_en
> What: /sys/.../iio:deviceX/in_steps_en
> KernelVersion: 3.19
> Contact: [email protected]
> Description:
> - Activates the step counter. After activation, the number of steps
> - taken by the user will be counted in hardware and exported through
> - in_steps_input.
> + Activates a device feature that runs in firmware/hardware.
> + E.g. for steps: the pedometer saves power while not used;
> + when activated, it will count the steps taken by the user in
> + firmware and export them through in_steps_input.
>
> What: /sys/.../iio:deviceX/in_steps_input
> KernelVersion: 3.19
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index c53047d..7c9a9a9 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -126,4 +126,14 @@ config MMA9551
> To compile this driver as a module, choose M here: the module
> will be called mma9551.
>
> +config MMA9553
> + tristate "Freescale MMA9553L Intelligent Pedometer Platform Driver"
> + depends on I2C
> + select MMA9551_CORE
> + help
> + Say yes here to build support for the Freescale MMA9553L
> + Intelligent Pedometer Platform Driver.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called mma9553.
> endmenu
> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> index 8105316..f815695 100644
> --- a/drivers/iio/accel/Makefile
> +++ b/drivers/iio/accel/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_MMA8452) += mma8452.o
>
> obj-$(CONFIG_MMA9551_CORE) += mma9551_core.o
> obj-$(CONFIG_MMA9551) += mma9551.o
> +obj-$(CONFIG_MMA9553) += mma9553.o
>
> obj-$(CONFIG_IIO_ST_ACCEL_3AXIS) += st_accel.o
> st_accel-y := st_accel_core.o
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 7f1a73e..7f55a6d 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -53,6 +53,11 @@
> #define MMA9551_AFE_Y_ACCEL_REG 0x02
> #define MMA9551_AFE_Z_ACCEL_REG 0x04
>
> +/* Reset/Suspend/Clear application */
> +#define MMA9551_RSC_RESET 0x00
> +#define MMA9551_RSC_OFFSET(mask) (3 - (ffs(mask) - 1) / 8)
> +#define MMA9551_RSC_VAL(mask) (mask >> (((ffs(mask) - 1) / 8) * 8))
> +
> /*
> * A response is composed of:
> * - control registers: MB0-3
> @@ -275,6 +280,64 @@ int mma9551_read_status_byte(struct i2c_client *client, u8 app_id,
> EXPORT_SYMBOL(mma9551_read_status_byte);
>
> /**
> + * mma9551_read_config_word() - read 1 config word
> + * @client: I2C client
> + * @app_id: Application ID
> + * @reg: Application register
> + * @val: Pointer to store value read
> + *
> + * Read one configuration word from the device using MMA955xL command format.
> + * Commands to the MMA955xL platform consist of a write followed by one or
> + * more reads.
> + *
> + * Locking note: This function must be called with the device lock held.
> + * Locking is not handled inside the function. Callers should ensure they
> + * serialize access to the HW.
> + *
> + * Returns: 0 on success, negative value on failure.
> + */
> +int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
> + u16 reg, u16 *val)
> +{
> + int ret;
> + __be16 v;
> +
> + ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> + reg, NULL, 0, (u8 *)&v, 2);
> + *val = be16_to_cpu(v);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(mma9551_read_config_word);
> +
> +/**
> + * mma9551_write_config_word() - write 1 config word
> + * @client: I2C client
> + * @app_id: Application ID
> + * @reg: Application register
> + * @val: Value to write
> + *
> + * Write one configuration word from the device using MMA955xL command format.
> + * Commands to the MMA955xL platform consist of a write followed by one or
> + * more reads.
> + *
> + * Locking note: This function must be called with the device lock held.
> + * Locking is not handled inside the function. Callers should ensure they
> + * serialize access to the HW.
> + *
> + * Returns: 0 on success, negative value on failure.
> + */
> +int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> + u16 reg, u16 val)
> +{
> + __be16 v = cpu_to_be16(val);
> +
> + return mma9551_transfer(client, app_id, MMA9551_CMD_WRITE_CONFIG, reg,
> + (u8 *) &v, 2, NULL, 0);
> +}
> +EXPORT_SYMBOL(mma9551_write_config_word);
> +
> +/**
> * mma9551_read_status_word() - read 1 status word
> * @client: I2C client
> * @app_id: Application ID
> @@ -306,6 +369,107 @@ int mma9551_read_status_word(struct i2c_client *client, u8 app_id,
> EXPORT_SYMBOL(mma9551_read_status_word);
>
> /**
> + * mma9551_read_config_words() - read multiple config words
> + * @client: I2C client
> + * @app_id: Application ID
> + * @reg: Application register
> + * @len: Length of array to read in bytes
> + * @val: Array of words to read
val is called buf in the function declaration.
> + *
> + * Read multiple configuration registers (word-sized registers).
> + *
> + * Locking note: This function must be called with the device lock held.
> + * Locking is not handled inside the function. Callers should ensure they
> + * serialize access to the HW.
> + *
> + * Returns: 0 on success, negative value on failure.
> + */
> +int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> + u16 reg, u8 len, u16 *buf)
> +{
> + int ret, i;
> + int len_words = len / sizeof(u16);
len_words could be unsigned.
> + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> +
> + ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> + reg, NULL, 0, (u8 *) be_buf, len);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < len_words; i++)
> + buf[i] = be16_to_cpu(be_buf[i]);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(mma9551_read_config_words);
> +
> +/**
> + * mma9551_read_status_words() - read multiple status words
> + * @client: I2C client
> + * @app_id: Application ID
> + * @reg: Application register
> + * @len: Length of array to read in bytes
> + * @val: Array of words to read
Also here, val is called buf in the function declaration.
> + *
> + * Read multiple status registers (word-sized registers).
> + *
> + * Locking note: This function must be called with the device lock held.
> + * Locking is not handled inside the function. Callers should ensure they
> + * serialize access to the HW.
> + *
> + * Returns: 0 on success, negative value on failure.
> + */
> +int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> + u16 reg, u8 len, u16 *buf)
> +{
> + int ret, i;
> + int len_words = len / sizeof(u16);
Same here.
> + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> +
> + ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> + reg, NULL, 0, (u8 *) be_buf, len);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < len_words; i++)
> + buf[i] = be16_to_cpu(be_buf[i]);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(mma9551_read_status_words);
> +
> +/**
> + * mma9551_write_config_words() - write multiple config words
> + * @client: I2C client
> + * @app_id: Application ID
> + * @reg: Application register
> + * @len: Length of array to write in bytes
> + * @val: Array of words to write
Same with val/buf here.
> + *
> + * Write multiple configuration registers (word-sized registers).
> + *
> + * Locking note: This function must be called with the device lock held.
> + * Locking is not handled inside the function. Callers should ensure they
> + * serialize access to the HW.
> + *
> + * Returns: 0 on success, negative value on failure.
> + */
> +int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> + u16 reg, u8 len, u16 *buf)
> +{
> + int i;
> + int len_words = len / sizeof(u16);
Same here.
> + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> +
> + for (i = 0; i < len_words; i++)
> + be_buf[i] = cpu_to_be16(buf[i]);
> +
> + return mma9551_transfer(client, app_id, MMA9551_CMD_WRITE_CONFIG,
> + reg, (u8 *) be_buf, len, NULL, 0);
> +}
> +EXPORT_SYMBOL(mma9551_write_config_words);
> +
> +/**
> * mma9551_update_config_bits() - update bits in register
> * @client: I2C client
> * @app_id: Application ID
> @@ -609,6 +773,25 @@ int mma9551_read_accel_scale(int *val, int *val2)
> }
> EXPORT_SYMBOL(mma9551_read_accel_scale);
>
> +/**
> + * mma9551_app_reset() - reset application
> + * @client: I2C client
> + * @app_mask: Application to reset
> + *
> + * Reset the given application (using the Reset/Suspend/Clear
> + * Control Application)
> + *
> + * Returns: 0 on success, negative value on failure.
> + */
> +int mma9551_app_reset(struct i2c_client *client, u32 app_mask)
> +{
> + return mma9551_write_config_byte(client, MMA9551_APPID_RCS,
Shouldn't this be MMA9551_APPID_RSC?
> + MMA9551_RSC_RESET +
> + MMA9551_RSC_OFFSET(app_mask),
> + MMA9551_RSC_VAL(app_mask));
> +}
> +EXPORT_SYMBOL(mma9551_app_reset);
> +
> MODULE_AUTHOR("Irina Tirdea <[email protected]>");
> MODULE_AUTHOR("Vlad Dogaru <[email protected]>");
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
> index e6efd02..edaa56b 100644
> --- a/drivers/iio/accel/mma9551_core.h
> +++ b/drivers/iio/accel/mma9551_core.h
> @@ -21,9 +21,13 @@
> #define MMA9551_APPID_AFE 0x06
> #define MMA9551_APPID_TILT 0x0B
> #define MMA9551_APPID_SLEEP_WAKE 0x12
> -#define MMA9551_APPID_RESET 0x17
> +#define MMA9551_APPID_PEDOMETER 0x15
> +#define MMA9551_APPID_RCS 0x17
And here as well _RSC?
> #define MMA9551_APPID_NONE 0xff
>
> +/* Reset/Suspend/Clear application app masks */
> +#define MMA9551_RSC_PED BIT(21)
> +
> #define MMA9551_AUTO_SUSPEND_DELAY_MS 2000
>
> enum mma9551_gpio_pin {
> @@ -48,8 +52,18 @@ int mma9551_write_config_byte(struct i2c_client *client, u8 app_id,
> u16 reg, u8 val);
> int mma9551_read_status_byte(struct i2c_client *client, u8 app_id,
> u16 reg, u8 *val);
> +int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
> + u16 reg, u16 *val);
> +int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> + u16 reg, u16 val);
> int mma9551_read_status_word(struct i2c_client *client, u8 app_id,
> u16 reg, u16 *val);
> +int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> + u16 reg, u8 len, u16 *buf);
> +int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> + u16 reg, u8 len, u16 *buf);
> +int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> + u16 reg, u8 len, u16 *buf);
> int mma9551_update_config_bits(struct i2c_client *client, u8 app_id,
> u16 reg, u8 mask, u8 val);
> int mma9551_gpio_config(struct i2c_client *client, enum mma9551_gpio_pin pin,
> @@ -62,5 +76,6 @@ int mma9551_read_accel_chan(struct i2c_client *client,
> const struct iio_chan_spec *chan,
> int *val, int *val2);
> int mma9551_read_accel_scale(int *val, int *val2);
> +int mma9551_app_reset(struct i2c_client *client, u32 app_mask);
>
> #endif /* _MMA9551_CORE_H_ */
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> new file mode 100644
> index 0000000..d23ebf1
> --- /dev/null
> +++ b/drivers/iio/accel/mma9553.c
> @@ -0,0 +1,1334 @@
> +/*
> + * Freescale MMA9553L Intelligent Pedometer driver
> + * Copyright (c) 2014, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/acpi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/pm_runtime.h>
> +#include "mma9551_core.h"
> +
> +#define MMA9553_DRV_NAME "mma9553"
> +#define MMA9553_IRQ_NAME "mma9553_event"
> +#define MMA9553_GPIO_NAME "mma9553_int"
> +
> +/* Pedometer configuration registers (R/W) */
> +#define MMA9553_REG_CONF_SLEEPMIN 0x00
> +#define MMA9553_REG_CONF_SLEEPMAX 0x02
> +#define MMA9553_REG_CONF_SLEEPTHD 0x04
> +#define MMA9553_MASK_CONF_WORD GENMASK(15, 0)
> +
> +#define MMA9553_REG_CONF_CONF_STEPLEN 0x06
> +#define MMA9553_MASK_CONF_CONFIG BIT(15)
> +#define MMA9553_MASK_CONF_ACT_DBCNTM BIT(14)
> +#define MMA9553_MASK_CONF_SLP_DBCNTM BIT(13)
> +#define MMA9553_MASK_CONF_STEPLEN GENMASK(7, 0)
> +
> +#define MMA9553_REG_CONF_HEIGHT_WEIGHT 0x08
> +#define MMA9553_MASK_CONF_HEIGHT GENMASK(15, 8)
> +#define MMA9553_MASK_CONF_WEIGHT GENMASK(7, 0)
> +
> +#define MMA9553_REG_CONF_FILTER 0x0A
> +#define MMA9553_MASK_CONF_FILTSTEP GENMASK(15, 8)
> +#define MMA9553_MASK_CONF_MALE BIT(7)
> +#define MMA9553_MASK_CONF_FILTTIME GENMASK(6, 0)
> +
> +#define MMA9553_REG_CONF_SPEED_STEP 0x0C
> +#define MMA9553_MASK_CONF_SPDPRD GENMASK(15, 8)
> +#define MMA9553_MASK_CONF_STEPCOALESCE GENMASK(7, 0)
> +
> +#define MMA9553_REG_CONF_ACTTHD 0x0E
> +
> +/* Pedometer status registers (R-only) */
> +#define MMA9553_REG_STATUS 0x00
> +#define MMA9553_MASK_STATUS_MRGFL BIT(15)
> +#define MMA9553_MASK_STATUS_SUSPCHG BIT(14)
> +#define MMA9553_MASK_STATUS_STEPCHG BIT(13)
> +#define MMA9553_MASK_STATUS_ACTCHG BIT(12)
> +#define MMA9553_MASK_STATUS_SUSP BIT(11)
> +#define MMA9553_MASK_STATUS_ACTIVITY (BIT(10) | BIT(9) | BIT(8))
This could use a GENMASK.
> +#define MMA9553_MASK_STATUS_VERSION 0x00FF
This is also a typical use-case for GENMASK.
> +
> +#define MMA9553_REG_STEPCNT 0x02
> +#define MMA9553_REG_DISTANCE 0x04
> +#define MMA9553_REG_SPEED 0x06
> +#define MMA9553_REG_CALORIES 0x08
> +#define MMA9553_REG_SLEEPCNT 0x0A
> +
> +/* Pedometer events are always mapped to this pin. */
> +#define MMA9553_DEFAULT_GPIO_PIN mma9551_gpio6
> +#define MMA9553_DEFAULT_GPIO_POLARITY 0
> +
> +/* Bitnum used for gpio configuration = bit number in high status byte */
> +#define STATUS_TO_BITNUM(bit) (ffs(bit) - 9)
This macro is missing its prefix.
> +
> +#define MMA9553_DEFAULT_SAMPLE_RATE 30 /* Hz */
> +
> +/*
> + * The internal activity level must be stable for ACTTHD samples before
> + * ACTIVITY is updated.The ACTIVITY variable contains the current activity
A whitespace is missing at the end of the first sentence.
> + * level and is updated every time a step is detected or once a second
> + * if there are no steps.
> + */
> +#define MMA9553_ACTIVITY_THD_TO_SEC(thd) ((thd) / MMA9553_DEFAULT_SAMPLE_RATE)
> +#define MMA9553_ACTIVITY_SEC_TO_THD(sec) ((sec) * MMA9553_DEFAULT_SAMPLE_RATE)
> +
> +/*
> + * Autonomously suspend pedometer if acceleration vector magnitude
> + * is near 1g (4096 at 0.244 mg/LSB resolution) for 30 seconds.
> + */
> +#define MMA9553_DEFAULT_SLEEPMIN 3688 /* 0,9 g */
> +#define MMA9553_DEFAULT_SLEEPMAX 4508 /* 1,1 g */
> +#define MMA9553_DEFAULT_SLEEPTHD (MMA9553_DEFAULT_SAMPLE_RATE * 30)
> +
> +#define MMA9553_CONFIG_RETRIES 2
> +
> +/* Status register - activity field */
> +enum activity_level {
> + ACTIVITY_UNKNOWN,
> + ACTIVITY_REST,
> + ACTIVITY_WALKING,
> + ACTIVITY_JOGGING,
> + ACTIVITY_RUNNING,
> +};
> +
> +static struct mma9553_event_info {
> + enum iio_chan_type type;
> + enum iio_modifier mod;
> + enum iio_event_direction dir;
> +} mma9553_events_info[] = {
> + {
> + .type = IIO_STEPS,
> + .mod = IIO_NO_MOD,
> + .dir = IIO_EV_DIR_NONE,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_STILL,
> + .dir = IIO_EV_DIR_RISING,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_STILL,
> + .dir = IIO_EV_DIR_FALLING,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_WALKING,
> + .dir = IIO_EV_DIR_RISING,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_WALKING,
> + .dir = IIO_EV_DIR_FALLING,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_JOGGING,
> + .dir = IIO_EV_DIR_RISING,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_JOGGING,
> + .dir = IIO_EV_DIR_FALLING,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_RUNNING,
> + .dir = IIO_EV_DIR_RISING,
> + },
> + {
> + .type = IIO_ACTIVITY,
> + .mod = IIO_MOD_RUNNING,
> + .dir = IIO_EV_DIR_FALLING,
> + },
> +};
> +
> +#define MMA9553_EVENTS_INFO_SIZE ARRAY_SIZE(mma9553_events_info)
> +
> +struct mma9553_event {
> + struct mma9553_event_info *info;
> + bool enabled;
> +};
> +
> +struct mma9553_conf_regs {
> + u16 sleepmin;
> + u16 sleepmax;
> + u16 sleepthd;
> + u16 config;
> + u16 height_weight;
> + u16 filter;
> + u16 speed_step;
> + u16 actthd;
> +} __packed;
> +
> +struct mma9553_data {
> + struct i2c_client *client;
> + struct mutex mutex;
> + struct mma9553_conf_regs conf;
> + struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];
> + int num_events;
num_events could be unsigned.
> + u8 gpio_bitnum;
> + /*
> + * This is used for all features that depend on step count:
> + * step count, distance, speed, calories.
> + */
> + bool stepcnt_enabled;
> + u16 stepcnt;
> + u8 activity;
> + s64 timestamp;
> +};
> +
> +static u8 mma9553_get_bits(u16 val, u16 mask)
> +{
> + return (val & mask) >> (ffs(mask) - 1);
> +}
> +
> +static u16 mma9553_set_bits(u16 current_val, u16 val, u16 mask)
> +{
> + return (current_val & ~mask) | (val << (ffs(mask) - 1));
> +}
> +
> +static enum iio_modifier mma9553_activity_to_mod(enum activity_level activity)
> +{
> + switch (activity) {
> + case ACTIVITY_RUNNING:
> + return IIO_MOD_RUNNING;
> + case ACTIVITY_JOGGING:
> + return IIO_MOD_JOGGING;
> + case ACTIVITY_WALKING:
> + return IIO_MOD_WALKING;
> + case ACTIVITY_REST:
> + return IIO_MOD_STILL;
> + case ACTIVITY_UNKNOWN:
> + default:
> + return IIO_NO_MOD;
> + }
> +}
> +
> +static void mma9553_init_events(struct mma9553_data *data)
> +{
> + int i;
> +
> + data->num_events = MMA9553_EVENTS_INFO_SIZE;
> + for (i = 0; i < data->num_events; i++) {
> + data->events[i].info = &mma9553_events_info[i];
> + data->events[i].enabled = false;
> + }
> +}
> +
> +static struct mma9553_event *mma9553_get_event(struct mma9553_data *data,
> + enum iio_chan_type type,
> + enum iio_modifier mod,
> + enum iio_event_direction dir)
> +{
> + int i;
> +
> + for (i = 0; i < data->num_events; i++)
> + if (data->events[i].info->type == type &&
> + data->events[i].info->mod == mod &&
> + data->events[i].info->dir == dir)
> + return &data->events[i];
> +
> + return NULL;
> +}
> +
> +static bool mma9553_is_any_event_enabled(struct mma9553_data *data,
> + bool check_type,
> + enum iio_chan_type type)
> +{
> + int i;
> +
> + for (i = 0; i < data->num_events; i++)
> + if ((check_type && data->events[i].info->type == type &&
> + data->events[i].enabled) ||
> + (!check_type && data->events[i].enabled))
> + return true;
> +
> + return false;
> +}
> +
> +static int mma9553_set_config(struct mma9553_data *data, u16 reg,
> + u16 *p_reg_val, u16 val, u16 mask)
> +{
> + int ret, retries;
> + u16 reg_val, config;
> +
> + reg_val = *p_reg_val;
> + if (val == mma9553_get_bits(reg_val, mask))
> + return 0;
> +
> + reg_val = mma9553_set_bits(reg_val, val, mask);
> + ret = mma9551_write_config_word(data->client, MMA9551_APPID_PEDOMETER,
> + reg, reg_val);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "error writing config register 0x%x\n", reg);
> + return ret;
> + }
> +
> + *p_reg_val = reg_val;
> +
> + /* Reinitializes the pedometer with current configuration values */
> + config = mma9553_set_bits(data->conf.config, 1,
> + MMA9553_MASK_CONF_CONFIG);
> +
> + ret = mma9551_write_config_word(data->client, MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_CONF_CONF_STEPLEN, config);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "error writing config register 0x%x\n",
> + MMA9553_REG_CONF_CONF_STEPLEN);
> + return ret;
> + }
> +
> + retries = MMA9553_CONFIG_RETRIES;
> + do {
> + mma9551_sleep(MMA9553_DEFAULT_SAMPLE_RATE);
> + ret = mma9551_read_config_word(data->client,
> + MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_CONF_CONF_STEPLEN,
> + &config);
> + if (ret < 0)
> + return ret;
> + } while (mma9553_get_bits(config, MMA9553_MASK_CONF_CONFIG) &&
> + --retries > 0);
> +
> + return 0;
> +}
> +
> +static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
> + u8 *activity, u16 *stepcnt)
> +{
> + u32 status_stepcnt;
> + u16 status;
> + int ret;
> +
> + ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_STATUS, sizeof(u32),
> + (u16 *) &status_stepcnt);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "error reading status and stepcnt\n");
> + return ret;
> + }
> +
I think this could be done a bit simpler by using u16 buf[2] instead of
status_stepcnt (making status obsolete). That's what it would boil down
to:
*activity = mma9553_get_bits(buf[0], MMA9553_MASK_STATUS_ACTIVITY);
*stepcnt = buf[1];
> + status = status_stepcnt & MMA9553_MASK_CONF_WORD;
> + *activity = mma9553_get_bits(status, MMA9553_MASK_STATUS_ACTIVITY);
> + *stepcnt = status_stepcnt >> 16;
> +
> + return 0;
> +}
> +
> +static int mma9553_conf_gpio(struct mma9553_data *data)
> +{
> + u8 bitnum = 0, appid = MMA9551_APPID_PEDOMETER;
> + int ret;
> + struct mma9553_event *ev_step_detect;
> + bool activity_enabled;
> +
> + activity_enabled =
> + mma9553_is_any_event_enabled(data, true, IIO_ACTIVITY);
> + ev_step_detect =
> + mma9553_get_event(data, IIO_STEPS, IIO_NO_MOD, IIO_EV_DIR_NONE);
Usually the parameters are wrapped, if possible. A matter of taste.
> +
> + /*
> + * If both step detector and activity are enabled, use the MRGFL bit.
> + * This bit is the logical OR of the SUSPCHG, STEPCHG, and ACTCHG flags.
> + */
> + if (activity_enabled && ev_step_detect->enabled)
> + bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_MRGFL);
> + else if (ev_step_detect->enabled)
> + bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
> + else if (activity_enabled)
> + bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
> + else /* Reset */
> + appid = MMA9551_APPID_NONE;
> +
> + if (data->gpio_bitnum == bitnum)
> + return 0;
> +
> + /* Save initial values for activity and stepcnt */
> + if (activity_enabled || ev_step_detect->enabled)
> + mma9553_read_activity_stepcnt(data, &data->activity,
> + &data->stepcnt);
Check for errors here?
> +
> + ret = mma9551_gpio_config(data->client,
> + MMA9553_DEFAULT_GPIO_PIN,
> + appid, bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
These parameters could be consolidated to fit into two lines.
> + if (ret < 0)
> + return ret;
> + data->gpio_bitnum = bitnum;
> +
> + return 0;
> +}
> +
> +static int mma9553_init(struct mma9553_data *data)
> +{
> + int ret;
> +
> + ret = mma9551_read_version(data->client);
> + if (ret)
> + return ret;
> +
> + /*
> + * Read all the pedometer configuration registers. This is used as
> + * a device identification command to differentiate the MMA9553L
> + * from the MMA9550L.
> + */
> + ret =
> + mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_CONF_SLEEPMIN,
> + sizeof(data->conf), (u16 *) &data->conf);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "device is not MMA9553L: failed to read cfg regs\n");
I would reduce the error message to the second part.
> + return ret;
> + }
> +
> +
> + /* Reset gpio */
> + data->gpio_bitnum = -1;
gpio_bitnum is u8, so you need to find a different solution here.
> + ret = mma9553_conf_gpio(data);
> + if (ret < 0)
> + return ret;
> +
> + ret = mma9551_app_reset(data->client, MMA9551_RSC_PED);
> + if (ret < 0)
> + return ret;
> +
> + /* Init config registers */
> + data->conf.sleepmin = MMA9553_DEFAULT_SLEEPMIN;
> + data->conf.sleepmax = MMA9553_DEFAULT_SLEEPMAX;
> + data->conf.sleepthd = MMA9553_DEFAULT_SLEEPTHD;
> + data->conf.config =
> + mma9553_set_bits(data->conf.config, 1, MMA9553_MASK_CONF_CONFIG);
Again, I would prefer to wrap parameters.
> + /*
> + * Clear the activity debounce counter when the activity level changes,
> + * so that the confidence level applies for any activity level.
> + */
> + data->conf.config = mma9553_set_bits(data->conf.config, 1,
> + MMA9553_MASK_CONF_ACT_DBCNTM);
> + ret =
> + mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_CONF_SLEEPMIN,
> + sizeof(data->conf), (u16 *) &data->conf);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "failed to write configuration registers\n");
> + return ret;
> + }
> +
> + return mma9551_set_device_state(data->client, true);
> +}
> +
> +static int mma9553_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct mma9553_data *data = iio_priv(indio_dev);
> + int ret;
> + u16 tmp;
> + u8 activity;
> + bool powered_on;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + switch (chan->type) {
> + case IIO_STEPS:
> + /*
> + * The HW only counts steps and other dependent
> + * parameters (speed, distance, calories, activity)
> + * if power is on (from enabling an event or the
> + * step counter */
The multiline comment should end on a separate line. Also, the parenthesis
should be closed and the sentence ended with a full stop.
> + powered_on =
> + mma9553_is_any_event_enabled(data, false, 0) ||
> + data->stepcnt_enabled;
> + if (!powered_on) {
> + dev_err(&data->client->dev,
> + "No channels enabled\n");
> + return -EINVAL;
> + }
> + mutex_lock(&data->mutex);
> + ret = mma9551_read_status_word(data->client,
> + MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_STEPCNT,
> + &tmp);
> + mutex_unlock(&data->mutex);
This block is repeatedly used with small changes only. So, better put it into
a separate function like this:
static int mma9553_fancy_name(mma9553_data *data, u16 reg, u16 *tmp)
> + if (ret < 0)
> + return ret;
> + *val = tmp;
> + return IIO_VAL_INT;
> + case IIO_DISTANCE:
> + powered_on =
> + mma9553_is_any_event_enabled(data, false, 0) ||
> + data->stepcnt_enabled;
> + if (!powered_on) {
> + dev_err(&data->client->dev,
> + "No channels enabled\n");
> + return -EINVAL;
> + }
> + mutex_lock(&data->mutex);
> + ret = mma9551_read_status_word(data->client,
> + MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_DISTANCE,
> + &tmp);
> + mutex_unlock(&data->mutex);
> + if (ret < 0)
> + return ret;
> + *val = tmp;
> + return IIO_VAL_INT;
> + case IIO_ACTIVITY:
> + powered_on =
> + mma9553_is_any_event_enabled(data, false, 0) ||
> + data->stepcnt_enabled;
> + if (!powered_on) {
> + dev_err(&data->client->dev,
> + "No channels enabled\n");
> + return -EINVAL;
> + }
> + mutex_lock(&data->mutex);
> + ret = mma9551_read_status_word(data->client,
> + MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_STATUS,
> + &tmp);
> + mutex_unlock(&data->mutex);
> + if (ret < 0)
> + return ret;
> +
> + activity =
> + mma9553_get_bits(tmp, MMA9553_MASK_STATUS_ACTIVITY);
> +
> + /*
> + * The device does not support confidence value levels,
> + * so we will always have 100% for current activity and
> + * 0% for the others.
> + */
> + if (chan->channel2 == mma9553_activity_to_mod(activity))
> + *val = 100;
> + else
> + *val = 0;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_VELOCITY: /* m/h */
> + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> + return -EINVAL;
> + powered_on =
> + mma9553_is_any_event_enabled(data, false, 0) ||
> + data->stepcnt_enabled;
> + if (!powered_on) {
> + dev_err(&data->client->dev,
> + "No channels enabled\n");
> + return -EINVAL;
> + }
> + mutex_lock(&data->mutex);
> + ret = mma9551_read_status_word(data->client,
> + MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_SPEED, &tmp);
> + mutex_unlock(&data->mutex);
> + if (ret < 0)
> + return ret;
> + *val = tmp;
> + return IIO_VAL_INT;
> + case IIO_ENERGY: /* Cal or kcal */
> + powered_on =
> + mma9553_is_any_event_enabled(data, false, 0) ||
> + data->stepcnt_enabled;
> + if (!powered_on) {
> + dev_err(&data->client->dev,
> + "No channels enabled\n");
> + return -EINVAL;
> + }
> + mutex_lock(&data->mutex);
> + ret = mma9551_read_status_word(data->client,
> + MMA9551_APPID_PEDOMETER,
> + MMA9553_REG_CALORIES,
> + &tmp);
> + mutex_unlock(&data->mutex);
> + if (ret < 0)
> + return ret;
> + *val = tmp;
> + return IIO_VAL_INT;
> + case IIO_ACCEL:
> + mutex_lock(&data->mutex);
> + ret = mma9551_read_accel_chan(data->client,
> + chan, val, val2);
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_VELOCITY: /* m/h to m/s */
> + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> + return -EINVAL;
> + *val = 0;
> + *val2 = 277; /* 0.000277 */
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_ENERGY: /* Cal or kcal to J */
> + *val = 4184;
> + return IIO_VAL_INT;
> + case IIO_ACCEL:
> + return mma9551_read_accel_scale(val, val2);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_ENABLE:
> + *val = data->stepcnt_enabled;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBHEIGHT:
> + tmp = mma9553_get_bits(data->conf.height_weight,
> + MMA9553_MASK_CONF_HEIGHT);
> + *val = tmp / 100; /* cm to m */
> + *val2 = (tmp % 100) * 10000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_CALIBWEIGHT:
> + *val = mma9553_get_bits(data->conf.height_weight,
> + MMA9553_MASK_CONF_WEIGHT);
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_DEBOUNCE_COUNT:
> + switch (chan->type) {
> + case IIO_STEPS:
> + *val = mma9553_get_bits(data->conf.filter,
> + MMA9553_MASK_CONF_FILTSTEP);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_DEBOUNCE_TIME:
> + switch (chan->type) {
> + case IIO_STEPS:
> + *val = mma9553_get_bits(data->conf.filter,
> + MMA9553_MASK_CONF_FILTTIME);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_INT_TIME:
> + switch (chan->type) {
> + case IIO_VELOCITY:
> + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> + return -EINVAL;
> + *val = mma9553_get_bits(data->conf.speed_step,
> + MMA9553_MASK_CONF_SPDPRD);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mma9553_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct mma9553_data *data = iio_priv(indio_dev);
> + int ret, tmp;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_ENABLE:
> + if (data->stepcnt_enabled == !!val)
> + return 0;
> + mutex_lock(&data->mutex);
> + ret = mma9551_set_power_state(data->client, val);
> + if (ret < 0) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> + data->stepcnt_enabled = val;
> + mutex_unlock(&data->mutex);
> + return 0;
> + case IIO_CHAN_INFO_CALIBHEIGHT:
> + /* m to cm */
> + tmp = val * 100 + val2 / 10000;
> + if (tmp < 0 || tmp > 255)
> + return -EINVAL;
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data,
> + MMA9553_REG_CONF_HEIGHT_WEIGHT,
> + &data->conf.height_weight,
> + tmp, MMA9553_MASK_CONF_HEIGHT);
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_CHAN_INFO_CALIBWEIGHT:
> + if (val < 0 || val > 255)
> + return -EINVAL;
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data,
> + MMA9553_REG_CONF_HEIGHT_WEIGHT,
> + &data->conf.height_weight,
> + val, MMA9553_MASK_CONF_WEIGHT);
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_CHAN_INFO_DEBOUNCE_COUNT:
> + switch (chan->type) {
> + case IIO_STEPS:
> + /*
> + * Set to 0 to disable step filtering. If the value
> + * specified is greater than 6, then 6 will be used.
> + */
> + if (val < 0)
> + return -EINVAL;
> + if (val > 6)
> + val = 6;
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> + &data->conf.filter, val,
> + MMA9553_MASK_CONF_FILTSTEP);
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_DEBOUNCE_TIME:
> + switch (chan->type) {
> + case IIO_STEPS:
> + if (val < 0 || val > 127)
> + return -EINVAL;
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> + &data->conf.filter, val,
> + MMA9553_MASK_CONF_FILTTIME);
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_INT_TIME:
> + switch (chan->type) {
> + case IIO_VELOCITY:
> + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> + return -EINVAL;
> + /*
> + * If set to a value greater than 5, then 5 will be
> + * used. Warning: Do not set SPDPRD to 0 or 1 as
> + * this may cause undesirable behavior.
> + */
> + if (val < 2)
> + return -EINVAL;
> + if (val > 5)
> + val = 5;
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data,
> + MMA9553_REG_CONF_SPEED_STEP,
> + &data->conf.speed_step, val,
> + MMA9553_MASK_CONF_SPDPRD);
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mma9553_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> +
> + struct mma9553_data *data = iio_priv(indio_dev);
> + struct mma9553_event *event;
> +
> + event = mma9553_get_event(data, chan->type, chan->channel2, dir);
> + if (!event)
> + return -EINVAL;
> +
> + return event->enabled;
> +}
> +
> +static int mma9553_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + struct mma9553_data *data = iio_priv(indio_dev);
> + struct mma9553_event *event;
> + int ret;
> +
> + event = mma9553_get_event(data, chan->type, chan->channel2, dir);
> + if (!event)
> + return -EINVAL;
> +
> + if (event->enabled == state)
> + return 0;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = mma9551_set_power_state(data->client, state);
> + if (ret < 0)
> + goto err_out;
> + event->enabled = state;
> +
> + ret = mma9553_conf_gpio(data);
> + if (ret < 0)
> + goto err_conf_gpio;
> +
> + mutex_unlock(&data->mutex);
> +
> + return ret;
Return 0 as indication of success?
> +
> +err_conf_gpio:
> + if (state) {
> + event->enabled = false;
> + mma9551_set_power_state(data->client, false);
> + }
> +err_out:
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +
> +static int mma9553_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct mma9553_data *data = iio_priv(indio_dev);
> +
> + *val2 = 0;
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + switch (chan->type) {
> + case IIO_STEPS:
> + *val = mma9553_get_bits(data->conf.speed_step,
> + MMA9553_MASK_CONF_STEPCOALESCE);
> + return IIO_VAL_INT;
> + case IIO_ACTIVITY:
> + /*
> + * The device does not support confidence value levels.
> + * We set an average of 50%.
> + */
> + *val = 50;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_EV_INFO_PERIOD:
> + switch (chan->type) {
> + case IIO_ACTIVITY:
> + *val = MMA9553_ACTIVITY_THD_TO_SEC(data->conf.actthd);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mma9553_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct mma9553_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + switch (chan->type) {
> + case IIO_STEPS:
> + if (val < 0 || val > 255)
> + return -EINVAL;
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data,
> + MMA9553_REG_CONF_SPEED_STEP,
> + &data->conf.speed_step, val,
> + MMA9553_MASK_CONF_STEPCOALESCE);
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + case IIO_EV_INFO_PERIOD:
> + switch (chan->type) {
> + case IIO_ACTIVITY:
Have a boundary check of val here, as well?
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data, MMA9553_REG_CONF_ACTTHD,
> + &data->conf.actthd,
> + MMA9553_ACTIVITY_SEC_TO_THD
> + (val), MMA9553_MASK_CONF_WORD);
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mma9553_get_calibgender_mode(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct mma9553_data *data = iio_priv(indio_dev);
> + u8 gender;
> +
> + gender = mma9553_get_bits(data->conf.filter, MMA9553_MASK_CONF_MALE);
> + /*
> + * HW expects 0 for female and 1 for male,
> + * while iio index is 0 for male and 1 for female
End this sentence with a full stop?
> + */
> + return !gender;
> +}
> +
> +static int mma9553_set_calibgender_mode(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int mode)
> +{
> + struct mma9553_data *data = iio_priv(indio_dev);
> + u8 gender = !mode;
> + int ret;
> +
> + if ((mode != 0) && (mode != 1))
> + return -EINVAL;
> + mutex_lock(&data->mutex);
> + ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> + &data->conf.filter, gender,
> + MMA9553_MASK_CONF_MALE);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static const struct iio_event_spec mma9553_step_event = {
> + .type = IIO_EV_TYPE_CHANGE,
> + .dir = IIO_EV_DIR_NONE,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | BIT(IIO_EV_INFO_VALUE),
> +};
> +
> +static const struct iio_event_spec mma9553_activity_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_PERIOD),
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_PERIOD),
> + },
> +};
> +
> +static const char * const calibgender_modes[] = { "male", "female" };
Shouldn't this be prefixed?
> +
> +static const struct iio_enum mma9553_calibgender_enum = {
> + .items = calibgender_modes,
> + .num_items = ARRAY_SIZE(calibgender_modes),
> + .get = mma9553_get_calibgender_mode,
> + .set = mma9553_set_calibgender_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info mma9553_ext_info[] = {
> + IIO_ENUM("calibgender", IIO_SHARED_BY_TYPE, &mma9553_calibgender_enum),
> + IIO_ENUM_AVAILABLE("calibgender", &mma9553_calibgender_enum),
> + {},
> +};
> +
> +#define MMA9553_PEDOMETER_CHANNEL(_type, _mask) { \
> + .type = _type, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) | \
> + BIT(IIO_CHAN_INFO_CALIBHEIGHT) | \
> + _mask, \
> + .ext_info = mma9553_ext_info, \
> +}
> +
> +#define MMA9553_ACTIVITY_CHANNEL(_chan2) { \
> + .type = IIO_ACTIVITY, \
> + .modified = 1, \
> + .channel2 = _chan2, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT), \
> + .event_spec = mma9553_activity_events, \
> + .num_event_specs = ARRAY_SIZE(mma9553_activity_events), \
> + .ext_info = mma9553_ext_info, \
> +}
> +
> +static const struct iio_chan_spec mma9553_channels[] = {
> + MMA9551_ACCEL_CHANNEL(IIO_MOD_X),
> + MMA9551_ACCEL_CHANNEL(IIO_MOD_Y),
> + MMA9551_ACCEL_CHANNEL(IIO_MOD_Z),
> +
> + {
> + .type = IIO_STEPS,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_ENABLE) |
> + BIT(IIO_CHAN_INFO_DEBOUNCE_COUNT) |
> + BIT(IIO_CHAN_INFO_DEBOUNCE_TIME),
> + .event_spec = &mma9553_step_event,
> + .num_event_specs = 1,
> + },
> +
> + MMA9553_PEDOMETER_CHANNEL(IIO_DISTANCE, BIT(IIO_CHAN_INFO_PROCESSED)),
> + {
> + .type = IIO_VELOCITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_ENABLE),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT),
> + .ext_info = mma9553_ext_info,
> + },
> + MMA9553_PEDOMETER_CHANNEL(IIO_ENERGY, BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_CALIBWEIGHT)),
> +
> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_RUNNING),
> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_JOGGING),
> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_WALKING),
> + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_STILL),
> +};
> +
> +static const struct iio_info mma9553_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = mma9553_read_raw,
> + .write_raw = mma9553_write_raw,
> + .read_event_config = mma9553_read_event_config,
> + .write_event_config = mma9553_write_event_config,
> + .read_event_value = mma9553_read_event_value,
> + .write_event_value = mma9553_write_event_value,
> +};
> +
> +static irqreturn_t mma9553_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct mma9553_data *data = iio_priv(indio_dev);
> +
> + data->timestamp = iio_get_time_ns();
> + /*
> + * Since we only configure the interrupt pin when an
> + * event is enabled, we are sure we have at least
> + * one event enabled at this point.
> + */
> + return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t mma9553_event_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct mma9553_data *data = iio_priv(indio_dev);
> + u16 stepcnt;
> + u8 activity;
> + struct mma9553_event *ev_activity, *ev_prev_activity, *ev_step_detect;
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = mma9553_read_activity_stepcnt(data, &activity, &stepcnt);
> + if (ret < 0) {
> + mutex_unlock(&data->mutex);
> + return IRQ_HANDLED;
> + }
> +
> + ev_prev_activity =
> + mma9553_get_event(data, IIO_ACTIVITY,
> + mma9553_activity_to_mod(data->activity),
> + IIO_EV_DIR_FALLING);
> + ev_activity =
> + mma9553_get_event(data, IIO_ACTIVITY,
> + mma9553_activity_to_mod(activity),
> + IIO_EV_DIR_RISING);
> + ev_step_detect =
> + mma9553_get_event(data, IIO_STEPS, IIO_NO_MOD, IIO_EV_DIR_NONE);
> +
> + if (ev_step_detect->enabled && (stepcnt != data->stepcnt)) {
> + data->stepcnt = stepcnt;
> + iio_push_event(indio_dev,
> + IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> + IIO_EV_DIR_NONE, IIO_EV_TYPE_CHANGE, 0, 0, 0),
> + data->timestamp);
> + }
> +
> + if (activity != data->activity) {
> + data->activity = activity;
> + /* ev_activity can be NULL if activity == ACTIVITY_UNKNOWN */
> + if (ev_prev_activity && ev_prev_activity->enabled)
> + iio_push_event(indio_dev,
> + IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> + ev_prev_activity->info->mod,
> + IIO_EV_DIR_FALLING,
> + IIO_EV_TYPE_THRESH, 0, 0, 0),
> + data->timestamp);
> +
> + if (ev_activity && ev_activity->enabled)
> + iio_push_event(indio_dev,
> + IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> + ev_activity->info->mod,
> + IIO_EV_DIR_RISING,
> + IIO_EV_TYPE_THRESH, 0, 0, 0),
> + data->timestamp);
> + }
> + mutex_unlock(&data->mutex);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mma9553_gpio_probe(struct i2c_client *client)
> +{
> + struct device *dev;
> + struct gpio_desc *gpio;
> + int ret;
> +
> + if (!client)
> + return -EINVAL;
> +
> + dev = &client->dev;
> +
> + /* data ready gpio interrupt pin */
Better use upper case for abreviations like GPIO and ACPI here and in
the messages below.
> + gpio = devm_gpiod_get_index(dev, MMA9553_GPIO_NAME, 0);
> + if (IS_ERR(gpio)) {
> + dev_err(dev, "acpi gpio get index failed\n");
> + return PTR_ERR(gpio);
> + }
> +
> + ret = gpiod_direction_input(gpio);
> + if (ret)
> + return ret;
> +
> + ret = gpiod_to_irq(gpio);
> +
> + dev_dbg(dev, "gpio resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> +
> + return ret;
> +}
> +
> +static const char *mma9553_match_acpi_device(struct device *dev)
> +{
> + const struct acpi_device_id *id;
> +
> + id = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!id)
> + return NULL;
> +
> + return dev_name(dev);
> +}
> +
> +static int mma9553_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct mma9553_data *data;
> + struct iio_dev *indio_dev;
> + const char *name = NULL;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> +
> + if (id)
> + name = id->name;
> + else if (ACPI_HANDLE(&client->dev))
> + name = mma9553_match_acpi_device(&client->dev);
> + else
> + return -ENOSYS;
> +
> + mutex_init(&data->mutex);
> + mma9553_init_events(data);
> +
> + ret = mma9553_init(data);
> + if (ret < 0)
> + return ret;
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->channels = mma9553_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mma9553_channels);
> + indio_dev->name = name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &mma9553_info;
> +
> + if (client->irq < 0)
> + client->irq = mma9553_gpio_probe(client);
> +
> + if (client->irq >= 0) {
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + mma9553_irq_handler,
> + mma9553_event_handler,
> + IRQF_TRIGGER_RISING,
> + MMA9553_IRQ_NAME, indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "request irq %d failed\n",
> + client->irq);
> + goto out_poweroff;
> + }
> +
> + }
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "unable to register iio device\n");
> + goto out_poweroff;
> + }
> +
> + ret = pm_runtime_set_active(&client->dev);
> + if (ret < 0)
> + goto out_iio_unregister;
> +
> + pm_runtime_enable(&client->dev);
> + pm_runtime_set_autosuspend_delay(&client->dev,
> + MMA9551_AUTO_SUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(&client->dev);
> +
> + dev_dbg(&indio_dev->dev, "Registered device %s\n", name);
> +
> + return 0;
> +
> +out_iio_unregister:
> + iio_device_unregister(indio_dev);
> +out_poweroff:
> + mma9551_set_device_state(client, false);
> + return ret;
> +}
> +
> +static int mma9553_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct mma9553_data *data = iio_priv(indio_dev);
> +
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> +
> + iio_device_unregister(indio_dev);
> + mutex_lock(&data->mutex);
> + mma9551_set_device_state(data->client, false);
> + mutex_unlock(&data->mutex);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int mma9553_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mma9553_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = mma9551_set_device_state(data->client, false);
> + mutex_unlock(&data->mutex);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "powering off device failed\n");
> + return -EAGAIN;
> + }
> +
> + return 0;
> +}
> +
> +static int mma9553_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mma9553_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = mma9551_set_device_state(data->client, true);
> + if (ret < 0)
> + return ret;
> +
> + mma9551_sleep(MMA9553_DEFAULT_SAMPLE_RATE);
> +
> + return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mma9553_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mma9553_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = mma9551_set_device_state(data->client, false);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int mma9553_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mma9553_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = mma9551_set_device_state(data->client, true);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +#endif
> +
> +static const struct dev_pm_ops mma9553_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume)
> + SET_RUNTIME_PM_OPS(mma9553_runtime_suspend,
> + mma9553_runtime_resume, NULL)
> +};
> +
> +static const struct acpi_device_id mma9553_acpi_match[] = {
> + {"MMA9553", 0},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, mma9553_acpi_match);
> +
> +static const struct i2c_device_id mma9553_id[] = {
> + {"mma9553", 0},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, mma9553_id);
> +
> +static struct i2c_driver mma9553_driver = {
> + .driver = {
> + .name = MMA9553_DRV_NAME,
> + .acpi_match_table = ACPI_PTR(mma9553_acpi_match),
> + .pm = &mma9553_pm_ops,
> + },
> + .probe = mma9553_probe,
> + .remove = mma9553_remove,
> + .id_table = mma9553_id,
> +};
> +
> +module_i2c_driver(mma9553_driver);
> +
> +MODULE_AUTHOR("Irina Tirdea <[email protected]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MMA9553L pedometer platform driver");
> -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> -----Original Message-----
> From: Hartmut Knaack [mailto:[email protected]]
> Sent: 05 April, 2015 2:18
> To: Tirdea, Irina; Jonathan Cameron; [email protected]
> Cc: [email protected]; Baluta, Daniel; Lars-Peter Clausen; Peter Meerwald
> Subject: Re: [PATCH v3 3/3] iio: add driver for Freescale MMA9553
>
> Irina Tirdea schrieb am 27.01.2015 um 19:41:
> > Add support for Freescale MMA9553L Intelligent Pedometer Platform.
> >
> > The following functionalities are supported:
> > - step counter (counts the number of steps using a HW register)
> > - step detector (generates an iio event at every step the user takes)
> > - activity recognition (rest, walking, jogging, running)
> > - speed
> > - calories
> > - distance
> >
> > To get accurate pedometer results, the user's height, weight and gender
> > need to be configured.
> >
> > The specifications can be downloaded from:
> > http://www.freescale.com/files/sensors/doc/ref_manual/MMA955xLSWRM.pdf
> > http://www.freescale.com/files/sensors/doc/ref_manual/MMA9553LSWRM.pdf
> >
>
> Hi Irina,
> I still spotted a few issues and have some recommendations which you
> could address in a follow-up patch. Also, there are some minor style
> issues (spaces after cast, alignment issues, unnecessary blank lines),
> which checkpatch.pl --strict reveals.
>
Hi Hartmut,
Thanks for the review!
I will run checkpatch.pl --strict and fix any issues as well as all the problems you mentioned below.
> One other issue, where I would like to get the opinion of the other
> maintainers as well, concerns some of the functions you added to
> mma9551_core.c. The way they are used so far doesn't pose a threat, yet.
> But I am worried about the __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS],
> where the elements are accessed in relation to the len parameter. So,
> should there be a check in those functions to prevent buffer overrun
> problems, or just be very careful when using those functions?
>
I think a check wouldn't hurt and it will prevent any future issues with using this API.
I will add this check in a separate patch from the other fixes so we can see if anybody else has a different opinion.
> > Signed-off-by: Irina Tirdea <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-bus-iio | 49 +-
> > drivers/iio/accel/Kconfig | 10 +
> > drivers/iio/accel/Makefile | 1 +
> > drivers/iio/accel/mma9551_core.c | 183 +++++
> > drivers/iio/accel/mma9551_core.h | 17 +-
> > drivers/iio/accel/mma9553.c | 1334 +++++++++++++++++++++++++++++++
> > 6 files changed, 1589 insertions(+), 5 deletions(-)
> > create mode 100644 drivers/iio/accel/mma9553.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index 588620e..9a70c31 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -343,7 +343,30 @@ Description:
> > production inaccuracies). If shared across all channels,
> > <type>_calibscale is used.
> >
> > -What: /sys/bus/iio/devices/iio:deviceX/in_steps_calibheight
> > +What: /sys/bus/iio/devices/iio:deviceX/in_activity_calibgender
> > +What: /sys/bus/iio/devices/iio:deviceX/in_energy_calibgender
> > +What: /sys/bus/iio/devices/iio:deviceX/in_distance_calibgender
> > +What: /sys/bus/iio/devices/iio:deviceX/in_velocity_calibgender
> > +KernelVersion: 3.20
> > +Contact: [email protected]
> > +Description:
> > + Gender of the user (e.g.: male, female) used by some pedometers
> > + to compute the stride length, distance, speed and activity
> > + type.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/in_activity_calibgender_available
> > +What: /sys/bus/iio/devices/iio:deviceX/in_energy_calibgender_available
> > +What: /sys/bus/iio/devices/iio:deviceX/in_distance_calibgender_available
> > +What: /sys/bus/iio/devices/iio:deviceX/in_velocity_calibgender_available
> > +KernelVersion: 3.20
> > +Contact: [email protected]
> > +Description:
> > + Lists all available gender values (e.g.: male, female).
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/in_activity_calibheight
> > +What: /sys/bus/iio/devices/iio:deviceX/in_energy_calibheight
> > +What: /sys/bus/iio/devices/iio:deviceX/in_distance_calibheight
> > +What: /sys/bus/iio/devices/iio:deviceX/in_velocity_calibheight
> > KernelVersion: 3.19
> > Contact: [email protected]
> > Description:
> > @@ -818,6 +841,14 @@ What: /sys/.../events/in_tempY_roc_falling_period
> > What: /sys/.../events/in_accel_x&y&z_mag_falling_period
> > What: /sys/.../events/in_intensity0_thresh_period
> > What: /sys/.../events/in_proximity0_thresh_period
> > +What: /sys/.../events/in_activity_still_thresh_rising_period
> > +What: /sys/.../events/in_activity_still_thresh_falling_period
> > +What: /sys/.../events/in_activity_walking_thresh_rising_period
> > +What: /sys/.../events/in_activity_walking_thresh_falling_period
> > +What: /sys/.../events/in_activity_jogging_thresh_rising_period
> > +What: /sys/.../events/in_activity_jogging_thresh_falling_period
> > +What: /sys/.../events/in_activity_running_thresh_rising_period
> > +What: /sys/.../events/in_activity_running_thresh_falling_period
> > KernelVersion: 2.6.37
> > Contact: [email protected]
> > Description:
> > @@ -1142,6 +1173,12 @@ Description:
> > This attribute is used to get/set the integration time in
> > seconds.
> >
> > +What: /sys/.../iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_integration_time
> > +KernelVersion: 3.20
> > +Contact: [email protected]
> > +Description:
> > + Number of seconds in which to compute speed.
> > +
> > What: /sys/bus/iio/devices/iio:deviceX/in_rot_quaternion_raw
> > KernelVersion: 3.15
> > Contact: [email protected]
> > @@ -1170,13 +1207,17 @@ Description:
> > present, output should be considered as processed with the
> > unit in milliamps.
> >
> > +What: /sys/.../iio:deviceX/in_energy_en
> > +What: /sys/.../iio:deviceX/in_distance_en
> > +What: /sys/.../iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_en
> > What: /sys/.../iio:deviceX/in_steps_en
> > KernelVersion: 3.19
> > Contact: [email protected]
> > Description:
> > - Activates the step counter. After activation, the number of steps
> > - taken by the user will be counted in hardware and exported through
> > - in_steps_input.
> > + Activates a device feature that runs in firmware/hardware.
> > + E.g. for steps: the pedometer saves power while not used;
> > + when activated, it will count the steps taken by the user in
> > + firmware and export them through in_steps_input.
> >
> > What: /sys/.../iio:deviceX/in_steps_input
> > KernelVersion: 3.19
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index c53047d..7c9a9a9 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -126,4 +126,14 @@ config MMA9551
> > To compile this driver as a module, choose M here: the module
> > will be called mma9551.
> >
> > +config MMA9553
> > + tristate "Freescale MMA9553L Intelligent Pedometer Platform Driver"
> > + depends on I2C
> > + select MMA9551_CORE
> > + help
> > + Say yes here to build support for the Freescale MMA9553L
> > + Intelligent Pedometer Platform Driver.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called mma9553.
> > endmenu
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index 8105316..f815695 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_MMA8452) += mma8452.o
> >
> > obj-$(CONFIG_MMA9551_CORE) += mma9551_core.o
> > obj-$(CONFIG_MMA9551) += mma9551.o
> > +obj-$(CONFIG_MMA9553) += mma9553.o
> >
> > obj-$(CONFIG_IIO_ST_ACCEL_3AXIS) += st_accel.o
> > st_accel-y := st_accel_core.o
> > diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> > index 7f1a73e..7f55a6d 100644
> > --- a/drivers/iio/accel/mma9551_core.c
> > +++ b/drivers/iio/accel/mma9551_core.c
> > @@ -53,6 +53,11 @@
> > #define MMA9551_AFE_Y_ACCEL_REG 0x02
> > #define MMA9551_AFE_Z_ACCEL_REG 0x04
> >
> > +/* Reset/Suspend/Clear application */
> > +#define MMA9551_RSC_RESET 0x00
> > +#define MMA9551_RSC_OFFSET(mask) (3 - (ffs(mask) - 1) / 8)
> > +#define MMA9551_RSC_VAL(mask) (mask >> (((ffs(mask) - 1) / 8) * 8))
> > +
> > /*
> > * A response is composed of:
> > * - control registers: MB0-3
> > @@ -275,6 +280,64 @@ int mma9551_read_status_byte(struct i2c_client *client, u8 app_id,
> > EXPORT_SYMBOL(mma9551_read_status_byte);
> >
> > /**
> > + * mma9551_read_config_word() - read 1 config word
> > + * @client: I2C client
> > + * @app_id: Application ID
> > + * @reg: Application register
> > + * @val: Pointer to store value read
> > + *
> > + * Read one configuration word from the device using MMA955xL command format.
> > + * Commands to the MMA955xL platform consist of a write followed by one or
> > + * more reads.
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
> > + u16 reg, u16 *val)
> > +{
> > + int ret;
> > + __be16 v;
> > +
> > + ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> > + reg, NULL, 0, (u8 *)&v, 2);
> > + *val = be16_to_cpu(v);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(mma9551_read_config_word);
> > +
> > +/**
> > + * mma9551_write_config_word() - write 1 config word
> > + * @client: I2C client
> > + * @app_id: Application ID
> > + * @reg: Application register
> > + * @val: Value to write
> > + *
> > + * Write one configuration word from the device using MMA955xL command format.
> > + * Commands to the MMA955xL platform consist of a write followed by one or
> > + * more reads.
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> > + u16 reg, u16 val)
> > +{
> > + __be16 v = cpu_to_be16(val);
> > +
> > + return mma9551_transfer(client, app_id, MMA9551_CMD_WRITE_CONFIG, reg,
> > + (u8 *) &v, 2, NULL, 0);
> > +}
> > +EXPORT_SYMBOL(mma9551_write_config_word);
> > +
> > +/**
> > * mma9551_read_status_word() - read 1 status word
> > * @client: I2C client
> > * @app_id: Application ID
> > @@ -306,6 +369,107 @@ int mma9551_read_status_word(struct i2c_client *client, u8 app_id,
> > EXPORT_SYMBOL(mma9551_read_status_word);
> >
> > /**
> > + * mma9551_read_config_words() - read multiple config words
> > + * @client: I2C client
> > + * @app_id: Application ID
> > + * @reg: Application register
> > + * @len: Length of array to read in bytes
> > + * @val: Array of words to read
>
> val is called buf in the function declaration.
Will fix all instances.
>
> > + *
> > + * Read multiple configuration registers (word-sized registers).
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 len, u16 *buf)
> > +{
> > + int ret, i;
> > + int len_words = len / sizeof(u16);
> len_words could be unsigned.
Agree. Will fix all instances of unsigned counters.
> > + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +
> > + ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_CONFIG,
> > + reg, NULL, 0, (u8 *) be_buf, len);
> > + if (ret < 0)
> > + return ret;
> > +
> > + for (i = 0; i < len_words; i++)
> > + buf[i] = be16_to_cpu(be_buf[i]);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(mma9551_read_config_words);
> > +
> > +/**
> > + * mma9551_read_status_words() - read multiple status words
> > + * @client: I2C client
> > + * @app_id: Application ID
> > + * @reg: Application register
> > + * @len: Length of array to read in bytes
> > + * @val: Array of words to read
>
> Also here, val is called buf in the function declaration.
>
> > + *
> > + * Read multiple status registers (word-sized registers).
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 len, u16 *buf)
> > +{
> > + int ret, i;
> > + int len_words = len / sizeof(u16);
>
> Same here.
>
> > + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +
> > + ret = mma9551_transfer(client, app_id, MMA9551_CMD_READ_STATUS,
> > + reg, NULL, 0, (u8 *) be_buf, len);
> > + if (ret < 0)
> > + return ret;
> > +
> > + for (i = 0; i < len_words; i++)
> > + buf[i] = be16_to_cpu(be_buf[i]);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(mma9551_read_status_words);
> > +
> > +/**
> > + * mma9551_write_config_words() - write multiple config words
> > + * @client: I2C client
> > + * @app_id: Application ID
> > + * @reg: Application register
> > + * @len: Length of array to write in bytes
> > + * @val: Array of words to write
>
> Same with val/buf here.
>
> > + *
> > + * Write multiple configuration registers (word-sized registers).
> > + *
> > + * Locking note: This function must be called with the device lock held.
> > + * Locking is not handled inside the function. Callers should ensure they
> > + * serialize access to the HW.
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 len, u16 *buf)
> > +{
> > + int i;
> > + int len_words = len / sizeof(u16);
>
> Same here.
>
> > + __be16 be_buf[MMA9551_MAX_MAILBOX_DATA_REGS];
> > +
> > + for (i = 0; i < len_words; i++)
> > + be_buf[i] = cpu_to_be16(buf[i]);
> > +
> > + return mma9551_transfer(client, app_id, MMA9551_CMD_WRITE_CONFIG,
> > + reg, (u8 *) be_buf, len, NULL, 0);
> > +}
> > +EXPORT_SYMBOL(mma9551_write_config_words);
> > +
> > +/**
> > * mma9551_update_config_bits() - update bits in register
> > * @client: I2C client
> > * @app_id: Application ID
> > @@ -609,6 +773,25 @@ int mma9551_read_accel_scale(int *val, int *val2)
> > }
> > EXPORT_SYMBOL(mma9551_read_accel_scale);
> >
> > +/**
> > + * mma9551_app_reset() - reset application
> > + * @client: I2C client
> > + * @app_mask: Application to reset
> > + *
> > + * Reset the given application (using the Reset/Suspend/Clear
> > + * Control Application)
> > + *
> > + * Returns: 0 on success, negative value on failure.
> > + */
> > +int mma9551_app_reset(struct i2c_client *client, u32 app_mask)
> > +{
> > + return mma9551_write_config_byte(client, MMA9551_APPID_RCS,
>
> Shouldn't this be MMA9551_APPID_RSC?
Yes, it should. Will fix.
>
> > + MMA9551_RSC_RESET +
> > + MMA9551_RSC_OFFSET(app_mask),
> > + MMA9551_RSC_VAL(app_mask));
> > +}
> > +EXPORT_SYMBOL(mma9551_app_reset);
> > +
> > MODULE_AUTHOR("Irina Tirdea <[email protected]>");
> > MODULE_AUTHOR("Vlad Dogaru <[email protected]>");
> > MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
> > index e6efd02..edaa56b 100644
> > --- a/drivers/iio/accel/mma9551_core.h
> > +++ b/drivers/iio/accel/mma9551_core.h
> > @@ -21,9 +21,13 @@
> > #define MMA9551_APPID_AFE 0x06
> > #define MMA9551_APPID_TILT 0x0B
> > #define MMA9551_APPID_SLEEP_WAKE 0x12
> > -#define MMA9551_APPID_RESET 0x17
> > +#define MMA9551_APPID_PEDOMETER 0x15
> > +#define MMA9551_APPID_RCS 0x17
>
> And here as well _RSC?
>
> > #define MMA9551_APPID_NONE 0xff
> >
> > +/* Reset/Suspend/Clear application app masks */
> > +#define MMA9551_RSC_PED BIT(21)
> > +
> > #define MMA9551_AUTO_SUSPEND_DELAY_MS 2000
> >
> > enum mma9551_gpio_pin {
> > @@ -48,8 +52,18 @@ int mma9551_write_config_byte(struct i2c_client *client, u8 app_id,
> > u16 reg, u8 val);
> > int mma9551_read_status_byte(struct i2c_client *client, u8 app_id,
> > u16 reg, u8 *val);
> > +int mma9551_read_config_word(struct i2c_client *client, u8 app_id,
> > + u16 reg, u16 *val);
> > +int mma9551_write_config_word(struct i2c_client *client, u8 app_id,
> > + u16 reg, u16 val);
> > int mma9551_read_status_word(struct i2c_client *client, u8 app_id,
> > u16 reg, u16 *val);
> > +int mma9551_read_config_words(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 len, u16 *buf);
> > +int mma9551_read_status_words(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 len, u16 *buf);
> > +int mma9551_write_config_words(struct i2c_client *client, u8 app_id,
> > + u16 reg, u8 len, u16 *buf);
> > int mma9551_update_config_bits(struct i2c_client *client, u8 app_id,
> > u16 reg, u8 mask, u8 val);
> > int mma9551_gpio_config(struct i2c_client *client, enum mma9551_gpio_pin pin,
> > @@ -62,5 +76,6 @@ int mma9551_read_accel_chan(struct i2c_client *client,
> > const struct iio_chan_spec *chan,
> > int *val, int *val2);
> > int mma9551_read_accel_scale(int *val, int *val2);
> > +int mma9551_app_reset(struct i2c_client *client, u32 app_mask);
> >
> > #endif /* _MMA9551_CORE_H_ */
> > diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> > new file mode 100644
> > index 0000000..d23ebf1
> > --- /dev/null
> > +++ b/drivers/iio/accel/mma9553.c
> > @@ -0,0 +1,1334 @@
> > +/*
> > + * Freescale MMA9553L Intelligent Pedometer driver
> > + * Copyright (c) 2014, Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > + * more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/slab.h>
> > +#include <linux/acpi.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/pm_runtime.h>
> > +#include "mma9551_core.h"
> > +
> > +#define MMA9553_DRV_NAME "mma9553"
> > +#define MMA9553_IRQ_NAME "mma9553_event"
> > +#define MMA9553_GPIO_NAME "mma9553_int"
> > +
> > +/* Pedometer configuration registers (R/W) */
> > +#define MMA9553_REG_CONF_SLEEPMIN 0x00
> > +#define MMA9553_REG_CONF_SLEEPMAX 0x02
> > +#define MMA9553_REG_CONF_SLEEPTHD 0x04
> > +#define MMA9553_MASK_CONF_WORD GENMASK(15, 0)
> > +
> > +#define MMA9553_REG_CONF_CONF_STEPLEN 0x06
> > +#define MMA9553_MASK_CONF_CONFIG BIT(15)
> > +#define MMA9553_MASK_CONF_ACT_DBCNTM BIT(14)
> > +#define MMA9553_MASK_CONF_SLP_DBCNTM BIT(13)
> > +#define MMA9553_MASK_CONF_STEPLEN GENMASK(7, 0)
> > +
> > +#define MMA9553_REG_CONF_HEIGHT_WEIGHT 0x08
> > +#define MMA9553_MASK_CONF_HEIGHT GENMASK(15, 8)
> > +#define MMA9553_MASK_CONF_WEIGHT GENMASK(7, 0)
> > +
> > +#define MMA9553_REG_CONF_FILTER 0x0A
> > +#define MMA9553_MASK_CONF_FILTSTEP GENMASK(15, 8)
> > +#define MMA9553_MASK_CONF_MALE BIT(7)
> > +#define MMA9553_MASK_CONF_FILTTIME GENMASK(6, 0)
> > +
> > +#define MMA9553_REG_CONF_SPEED_STEP 0x0C
> > +#define MMA9553_MASK_CONF_SPDPRD GENMASK(15, 8)
> > +#define MMA9553_MASK_CONF_STEPCOALESCE GENMASK(7, 0)
> > +
> > +#define MMA9553_REG_CONF_ACTTHD 0x0E
> > +
> > +/* Pedometer status registers (R-only) */
> > +#define MMA9553_REG_STATUS 0x00
> > +#define MMA9553_MASK_STATUS_MRGFL BIT(15)
> > +#define MMA9553_MASK_STATUS_SUSPCHG BIT(14)
> > +#define MMA9553_MASK_STATUS_STEPCHG BIT(13)
> > +#define MMA9553_MASK_STATUS_ACTCHG BIT(12)
> > +#define MMA9553_MASK_STATUS_SUSP BIT(11)
> > +#define MMA9553_MASK_STATUS_ACTIVITY (BIT(10) | BIT(9) | BIT(8))
>
> This could use a GENMASK.
Cannot figure out why I did not use GENMASK here as well. Will fix it.
>
> > +#define MMA9553_MASK_STATUS_VERSION 0x00FF
>
> This is also a typical use-case for GENMASK.
>
> > +
> > +#define MMA9553_REG_STEPCNT 0x02
> > +#define MMA9553_REG_DISTANCE 0x04
> > +#define MMA9553_REG_SPEED 0x06
> > +#define MMA9553_REG_CALORIES 0x08
> > +#define MMA9553_REG_SLEEPCNT 0x0A
> > +
> > +/* Pedometer events are always mapped to this pin. */
> > +#define MMA9553_DEFAULT_GPIO_PIN mma9551_gpio6
> > +#define MMA9553_DEFAULT_GPIO_POLARITY 0
> > +
> > +/* Bitnum used for gpio configuration = bit number in high status byte */
> > +#define STATUS_TO_BITNUM(bit) (ffs(bit) - 9)
>
> This macro is missing its prefix.
Ok.
>
> > +
> > +#define MMA9553_DEFAULT_SAMPLE_RATE 30 /* Hz */
> > +
> > +/*
> > + * The internal activity level must be stable for ACTTHD samples before
> > + * ACTIVITY is updated.The ACTIVITY variable contains the current activity
>
> A whitespace is missing at the end of the first sentence.
>
> > + * level and is updated every time a step is detected or once a second
> > + * if there are no steps.
> > + */
> > +#define MMA9553_ACTIVITY_THD_TO_SEC(thd) ((thd) / MMA9553_DEFAULT_SAMPLE_RATE)
> > +#define MMA9553_ACTIVITY_SEC_TO_THD(sec) ((sec) * MMA9553_DEFAULT_SAMPLE_RATE)
> > +
> > +/*
> > + * Autonomously suspend pedometer if acceleration vector magnitude
> > + * is near 1g (4096 at 0.244 mg/LSB resolution) for 30 seconds.
> > + */
> > +#define MMA9553_DEFAULT_SLEEPMIN 3688 /* 0,9 g */
> > +#define MMA9553_DEFAULT_SLEEPMAX 4508 /* 1,1 g */
> > +#define MMA9553_DEFAULT_SLEEPTHD (MMA9553_DEFAULT_SAMPLE_RATE * 30)
> > +
> > +#define MMA9553_CONFIG_RETRIES 2
> > +
> > +/* Status register - activity field */
> > +enum activity_level {
> > + ACTIVITY_UNKNOWN,
> > + ACTIVITY_REST,
> > + ACTIVITY_WALKING,
> > + ACTIVITY_JOGGING,
> > + ACTIVITY_RUNNING,
> > +};
> > +
> > +static struct mma9553_event_info {
> > + enum iio_chan_type type;
> > + enum iio_modifier mod;
> > + enum iio_event_direction dir;
> > +} mma9553_events_info[] = {
> > + {
> > + .type = IIO_STEPS,
> > + .mod = IIO_NO_MOD,
> > + .dir = IIO_EV_DIR_NONE,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_STILL,
> > + .dir = IIO_EV_DIR_RISING,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_STILL,
> > + .dir = IIO_EV_DIR_FALLING,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_WALKING,
> > + .dir = IIO_EV_DIR_RISING,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_WALKING,
> > + .dir = IIO_EV_DIR_FALLING,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_JOGGING,
> > + .dir = IIO_EV_DIR_RISING,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_JOGGING,
> > + .dir = IIO_EV_DIR_FALLING,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_RUNNING,
> > + .dir = IIO_EV_DIR_RISING,
> > + },
> > + {
> > + .type = IIO_ACTIVITY,
> > + .mod = IIO_MOD_RUNNING,
> > + .dir = IIO_EV_DIR_FALLING,
> > + },
> > +};
> > +
> > +#define MMA9553_EVENTS_INFO_SIZE ARRAY_SIZE(mma9553_events_info)
> > +
> > +struct mma9553_event {
> > + struct mma9553_event_info *info;
> > + bool enabled;
> > +};
> > +
> > +struct mma9553_conf_regs {
> > + u16 sleepmin;
> > + u16 sleepmax;
> > + u16 sleepthd;
> > + u16 config;
> > + u16 height_weight;
> > + u16 filter;
> > + u16 speed_step;
> > + u16 actthd;
> > +} __packed;
> > +
> > +struct mma9553_data {
> > + struct i2c_client *client;
> > + struct mutex mutex;
> > + struct mma9553_conf_regs conf;
> > + struct mma9553_event events[MMA9553_EVENTS_INFO_SIZE];
> > + int num_events;
>
> num_events could be unsigned.
>
> > + u8 gpio_bitnum;
> > + /*
> > + * This is used for all features that depend on step count:
> > + * step count, distance, speed, calories.
> > + */
> > + bool stepcnt_enabled;
> > + u16 stepcnt;
> > + u8 activity;
> > + s64 timestamp;
> > +};
> > +
> > +static u8 mma9553_get_bits(u16 val, u16 mask)
> > +{
> > + return (val & mask) >> (ffs(mask) - 1);
> > +}
> > +
> > +static u16 mma9553_set_bits(u16 current_val, u16 val, u16 mask)
> > +{
> > + return (current_val & ~mask) | (val << (ffs(mask) - 1));
> > +}
> > +
> > +static enum iio_modifier mma9553_activity_to_mod(enum activity_level activity)
> > +{
> > + switch (activity) {
> > + case ACTIVITY_RUNNING:
> > + return IIO_MOD_RUNNING;
> > + case ACTIVITY_JOGGING:
> > + return IIO_MOD_JOGGING;
> > + case ACTIVITY_WALKING:
> > + return IIO_MOD_WALKING;
> > + case ACTIVITY_REST:
> > + return IIO_MOD_STILL;
> > + case ACTIVITY_UNKNOWN:
> > + default:
> > + return IIO_NO_MOD;
> > + }
> > +}
> > +
> > +static void mma9553_init_events(struct mma9553_data *data)
> > +{
> > + int i;
> > +
> > + data->num_events = MMA9553_EVENTS_INFO_SIZE;
> > + for (i = 0; i < data->num_events; i++) {
> > + data->events[i].info = &mma9553_events_info[i];
> > + data->events[i].enabled = false;
> > + }
> > +}
> > +
> > +static struct mma9553_event *mma9553_get_event(struct mma9553_data *data,
> > + enum iio_chan_type type,
> > + enum iio_modifier mod,
> > + enum iio_event_direction dir)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < data->num_events; i++)
> > + if (data->events[i].info->type == type &&
> > + data->events[i].info->mod == mod &&
> > + data->events[i].info->dir == dir)
> > + return &data->events[i];
> > +
> > + return NULL;
> > +}
> > +
> > +static bool mma9553_is_any_event_enabled(struct mma9553_data *data,
> > + bool check_type,
> > + enum iio_chan_type type)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < data->num_events; i++)
> > + if ((check_type && data->events[i].info->type == type &&
> > + data->events[i].enabled) ||
> > + (!check_type && data->events[i].enabled))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static int mma9553_set_config(struct mma9553_data *data, u16 reg,
> > + u16 *p_reg_val, u16 val, u16 mask)
> > +{
> > + int ret, retries;
> > + u16 reg_val, config;
> > +
> > + reg_val = *p_reg_val;
> > + if (val == mma9553_get_bits(reg_val, mask))
> > + return 0;
> > +
> > + reg_val = mma9553_set_bits(reg_val, val, mask);
> > + ret = mma9551_write_config_word(data->client, MMA9551_APPID_PEDOMETER,
> > + reg, reg_val);
> > + if (ret < 0) {
> > + dev_err(&data->client->dev,
> > + "error writing config register 0x%x\n", reg);
> > + return ret;
> > + }
> > +
> > + *p_reg_val = reg_val;
> > +
> > + /* Reinitializes the pedometer with current configuration values */
> > + config = mma9553_set_bits(data->conf.config, 1,
> > + MMA9553_MASK_CONF_CONFIG);
> > +
> > + ret = mma9551_write_config_word(data->client, MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_CONF_CONF_STEPLEN, config);
> > + if (ret < 0) {
> > + dev_err(&data->client->dev,
> > + "error writing config register 0x%x\n",
> > + MMA9553_REG_CONF_CONF_STEPLEN);
> > + return ret;
> > + }
> > +
> > + retries = MMA9553_CONFIG_RETRIES;
> > + do {
> > + mma9551_sleep(MMA9553_DEFAULT_SAMPLE_RATE);
> > + ret = mma9551_read_config_word(data->client,
> > + MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_CONF_CONF_STEPLEN,
> > + &config);
> > + if (ret < 0)
> > + return ret;
> > + } while (mma9553_get_bits(config, MMA9553_MASK_CONF_CONFIG) &&
> > + --retries > 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
> > + u8 *activity, u16 *stepcnt)
> > +{
> > + u32 status_stepcnt;
> > + u16 status;
> > + int ret;
> > +
> > + ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_STATUS, sizeof(u32),
> > + (u16 *) &status_stepcnt);
> > + if (ret < 0) {
> > + dev_err(&data->client->dev,
> > + "error reading status and stepcnt\n");
> > + return ret;
> > + }
> > +
>
> I think this could be done a bit simpler by using u16 buf[2] instead of
> status_stepcnt (making status obsolete). That's what it would boil down
> to:
> *activity = mma9553_get_bits(buf[0], MMA9553_MASK_STATUS_ACTIVITY);
> *stepcnt = buf[1];
>
That does look more simple. Will change it accordingly.
> > + status = status_stepcnt & MMA9553_MASK_CONF_WORD;
> > + *activity = mma9553_get_bits(status, MMA9553_MASK_STATUS_ACTIVITY);
> > + *stepcnt = status_stepcnt >> 16;
> > +
> > + return 0;
> > +}
> > +
> > +static int mma9553_conf_gpio(struct mma9553_data *data)
> > +{
> > + u8 bitnum = 0, appid = MMA9551_APPID_PEDOMETER;
> > + int ret;
> > + struct mma9553_event *ev_step_detect;
> > + bool activity_enabled;
> > +
> > + activity_enabled =
> > + mma9553_is_any_event_enabled(data, true, IIO_ACTIVITY);
> > + ev_step_detect =
> > + mma9553_get_event(data, IIO_STEPS, IIO_NO_MOD, IIO_EV_DIR_NONE);
>
> Usually the parameters are wrapped, if possible. A matter of taste.
>
Yes, somehow I mixed the two approaches. I will fix all instances.
> > +
> > + /*
> > + * If both step detector and activity are enabled, use the MRGFL bit.
> > + * This bit is the logical OR of the SUSPCHG, STEPCHG, and ACTCHG flags.
> > + */
> > + if (activity_enabled && ev_step_detect->enabled)
> > + bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_MRGFL);
> > + else if (ev_step_detect->enabled)
> > + bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_STEPCHG);
> > + else if (activity_enabled)
> > + bitnum = STATUS_TO_BITNUM(MMA9553_MASK_STATUS_ACTCHG);
> > + else /* Reset */
> > + appid = MMA9551_APPID_NONE;
> > +
> > + if (data->gpio_bitnum == bitnum)
> > + return 0;
> > +
> > + /* Save initial values for activity and stepcnt */
> > + if (activity_enabled || ev_step_detect->enabled)
> > + mma9553_read_activity_stepcnt(data, &data->activity,
> > + &data->stepcnt);
>
> Check for errors here?
Ok.
>
> > +
> > + ret = mma9551_gpio_config(data->client,
> > + MMA9553_DEFAULT_GPIO_PIN,
> > + appid, bitnum, MMA9553_DEFAULT_GPIO_POLARITY);
>
> These parameters could be consolidated to fit into two lines.
>
> > + if (ret < 0)
> > + return ret;
> > + data->gpio_bitnum = bitnum;
> > +
> > + return 0;
> > +}
> > +
> > +static int mma9553_init(struct mma9553_data *data)
> > +{
> > + int ret;
> > +
> > + ret = mma9551_read_version(data->client);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * Read all the pedometer configuration registers. This is used as
> > + * a device identification command to differentiate the MMA9553L
> > + * from the MMA9550L.
> > + */
> > + ret =
> > + mma9551_read_config_words(data->client, MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_CONF_SLEEPMIN,
> > + sizeof(data->conf), (u16 *) &data->conf);
> > + if (ret < 0) {
> > + dev_err(&data->client->dev,
> > + "device is not MMA9553L: failed to read cfg regs\n");
>
> I would reduce the error message to the second part.
>
Will fix.
> > + return ret;
> > + }
> > +
> > +
> > + /* Reset gpio */
> > + data->gpio_bitnum = -1;
>
> gpio_bitnum is u8, so you need to find a different solution here.
>
Good point! I will use a positive invalid value to force reset.
> > + ret = mma9553_conf_gpio(data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = mma9551_app_reset(data->client, MMA9551_RSC_PED);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Init config registers */
> > + data->conf.sleepmin = MMA9553_DEFAULT_SLEEPMIN;
> > + data->conf.sleepmax = MMA9553_DEFAULT_SLEEPMAX;
> > + data->conf.sleepthd = MMA9553_DEFAULT_SLEEPTHD;
> > + data->conf.config =
> > + mma9553_set_bits(data->conf.config, 1, MMA9553_MASK_CONF_CONFIG);
>
> Again, I would prefer to wrap parameters.
>
> > + /*
> > + * Clear the activity debounce counter when the activity level changes,
> > + * so that the confidence level applies for any activity level.
> > + */
> > + data->conf.config = mma9553_set_bits(data->conf.config, 1,
> > + MMA9553_MASK_CONF_ACT_DBCNTM);
> > + ret =
> > + mma9551_write_config_words(data->client, MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_CONF_SLEEPMIN,
> > + sizeof(data->conf), (u16 *) &data->conf);
> > + if (ret < 0) {
> > + dev_err(&data->client->dev,
> > + "failed to write configuration registers\n");
> > + return ret;
> > + }
> > +
> > + return mma9551_set_device_state(data->client, true);
> > +}
> > +
> > +static int mma9553_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u16 tmp;
> > + u8 activity;
> > + bool powered_on;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + switch (chan->type) {
> > + case IIO_STEPS:
> > + /*
> > + * The HW only counts steps and other dependent
> > + * parameters (speed, distance, calories, activity)
> > + * if power is on (from enabling an event or the
> > + * step counter */
>
> The multiline comment should end on a separate line. Also, the parenthesis
> should be closed and the sentence ended with a full stop.
>
> > + powered_on =
> > + mma9553_is_any_event_enabled(data, false, 0) ||
> > + data->stepcnt_enabled;
> > + if (!powered_on) {
> > + dev_err(&data->client->dev,
> > + "No channels enabled\n");
> > + return -EINVAL;
> > + }
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_read_status_word(data->client,
> > + MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_STEPCNT,
> > + &tmp);
> > + mutex_unlock(&data->mutex);
>
> This block is repeatedly used with small changes only. So, better put it into
> a separate function like this:
> static int mma9553_fancy_name(mma9553_data *data, u16 reg, u16 *tmp)
>
That would look much better! Will refactor the code. Thanks for the suggestion!
> > + if (ret < 0)
> > + return ret;
> > + *val = tmp;
> > + return IIO_VAL_INT;
> > + case IIO_DISTANCE:
> > + powered_on =
> > + mma9553_is_any_event_enabled(data, false, 0) ||
> > + data->stepcnt_enabled;
> > + if (!powered_on) {
> > + dev_err(&data->client->dev,
> > + "No channels enabled\n");
> > + return -EINVAL;
> > + }
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_read_status_word(data->client,
> > + MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_DISTANCE,
> > + &tmp);
> > + mutex_unlock(&data->mutex);
> > + if (ret < 0)
> > + return ret;
> > + *val = tmp;
> > + return IIO_VAL_INT;
> > + case IIO_ACTIVITY:
> > + powered_on =
> > + mma9553_is_any_event_enabled(data, false, 0) ||
> > + data->stepcnt_enabled;
> > + if (!powered_on) {
> > + dev_err(&data->client->dev,
> > + "No channels enabled\n");
> > + return -EINVAL;
> > + }
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_read_status_word(data->client,
> > + MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_STATUS,
> > + &tmp);
> > + mutex_unlock(&data->mutex);
> > + if (ret < 0)
> > + return ret;
> > +
> > + activity =
> > + mma9553_get_bits(tmp, MMA9553_MASK_STATUS_ACTIVITY);
> > +
> > + /*
> > + * The device does not support confidence value levels,
> > + * so we will always have 100% for current activity and
> > + * 0% for the others.
> > + */
> > + if (chan->channel2 == mma9553_activity_to_mod(activity))
> > + *val = 100;
> > + else
> > + *val = 0;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VELOCITY: /* m/h */
> > + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > + return -EINVAL;
> > + powered_on =
> > + mma9553_is_any_event_enabled(data, false, 0) ||
> > + data->stepcnt_enabled;
> > + if (!powered_on) {
> > + dev_err(&data->client->dev,
> > + "No channels enabled\n");
> > + return -EINVAL;
> > + }
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_read_status_word(data->client,
> > + MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_SPEED, &tmp);
> > + mutex_unlock(&data->mutex);
> > + if (ret < 0)
> > + return ret;
> > + *val = tmp;
> > + return IIO_VAL_INT;
> > + case IIO_ENERGY: /* Cal or kcal */
> > + powered_on =
> > + mma9553_is_any_event_enabled(data, false, 0) ||
> > + data->stepcnt_enabled;
> > + if (!powered_on) {
> > + dev_err(&data->client->dev,
> > + "No channels enabled\n");
> > + return -EINVAL;
> > + }
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_read_status_word(data->client,
> > + MMA9551_APPID_PEDOMETER,
> > + MMA9553_REG_CALORIES,
> > + &tmp);
> > + mutex_unlock(&data->mutex);
> > + if (ret < 0)
> > + return ret;
> > + *val = tmp;
> > + return IIO_VAL_INT;
> > + case IIO_ACCEL:
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_read_accel_chan(data->client,
> > + chan, val, val2);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->type) {
> > + case IIO_VELOCITY: /* m/h to m/s */
> > + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > + return -EINVAL;
> > + *val = 0;
> > + *val2 = 277; /* 0.000277 */
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_ENERGY: /* Cal or kcal to J */
> > + *val = 4184;
> > + return IIO_VAL_INT;
> > + case IIO_ACCEL:
> > + return mma9551_read_accel_scale(val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_ENABLE:
> > + *val = data->stepcnt_enabled;
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_CALIBHEIGHT:
> > + tmp = mma9553_get_bits(data->conf.height_weight,
> > + MMA9553_MASK_CONF_HEIGHT);
> > + *val = tmp / 100; /* cm to m */
> > + *val2 = (tmp % 100) * 10000;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_CALIBWEIGHT:
> > + *val = mma9553_get_bits(data->conf.height_weight,
> > + MMA9553_MASK_CONF_WEIGHT);
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_DEBOUNCE_COUNT:
> > + switch (chan->type) {
> > + case IIO_STEPS:
> > + *val = mma9553_get_bits(data->conf.filter,
> > + MMA9553_MASK_CONF_FILTSTEP);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_DEBOUNCE_TIME:
> > + switch (chan->type) {
> > + case IIO_STEPS:
> > + *val = mma9553_get_bits(data->conf.filter,
> > + MMA9553_MASK_CONF_FILTTIME);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_INT_TIME:
> > + switch (chan->type) {
> > + case IIO_VELOCITY:
> > + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > + return -EINVAL;
> > + *val = mma9553_get_bits(data->conf.speed_step,
> > + MMA9553_MASK_CONF_SPDPRD);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int mma9553_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + int ret, tmp;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_ENABLE:
> > + if (data->stepcnt_enabled == !!val)
> > + return 0;
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_set_power_state(data->client, val);
> > + if (ret < 0) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > + data->stepcnt_enabled = val;
> > + mutex_unlock(&data->mutex);
> > + return 0;
> > + case IIO_CHAN_INFO_CALIBHEIGHT:
> > + /* m to cm */
> > + tmp = val * 100 + val2 / 10000;
> > + if (tmp < 0 || tmp > 255)
> > + return -EINVAL;
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data,
> > + MMA9553_REG_CONF_HEIGHT_WEIGHT,
> > + &data->conf.height_weight,
> > + tmp, MMA9553_MASK_CONF_HEIGHT);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_CHAN_INFO_CALIBWEIGHT:
> > + if (val < 0 || val > 255)
> > + return -EINVAL;
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data,
> > + MMA9553_REG_CONF_HEIGHT_WEIGHT,
> > + &data->conf.height_weight,
> > + val, MMA9553_MASK_CONF_WEIGHT);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_CHAN_INFO_DEBOUNCE_COUNT:
> > + switch (chan->type) {
> > + case IIO_STEPS:
> > + /*
> > + * Set to 0 to disable step filtering. If the value
> > + * specified is greater than 6, then 6 will be used.
> > + */
> > + if (val < 0)
> > + return -EINVAL;
> > + if (val > 6)
> > + val = 6;
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> > + &data->conf.filter, val,
> > + MMA9553_MASK_CONF_FILTSTEP);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_DEBOUNCE_TIME:
> > + switch (chan->type) {
> > + case IIO_STEPS:
> > + if (val < 0 || val > 127)
> > + return -EINVAL;
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> > + &data->conf.filter, val,
> > + MMA9553_MASK_CONF_FILTTIME);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_INT_TIME:
> > + switch (chan->type) {
> > + case IIO_VELOCITY:
> > + if (chan->channel2 != IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z)
> > + return -EINVAL;
> > + /*
> > + * If set to a value greater than 5, then 5 will be
> > + * used. Warning: Do not set SPDPRD to 0 or 1 as
> > + * this may cause undesirable behavior.
> > + */
> > + if (val < 2)
> > + return -EINVAL;
> > + if (val > 5)
> > + val = 5;
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data,
> > + MMA9553_REG_CONF_SPEED_STEP,
> > + &data->conf.speed_step, val,
> > + MMA9553_MASK_CONF_SPDPRD);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int mma9553_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > +
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + struct mma9553_event *event;
> > +
> > + event = mma9553_get_event(data, chan->type, chan->channel2, dir);
> > + if (!event)
> > + return -EINVAL;
> > +
> > + return event->enabled;
> > +}
> > +
> > +static int mma9553_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir, int state)
> > +{
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + struct mma9553_event *event;
> > + int ret;
> > +
> > + event = mma9553_get_event(data, chan->type, chan->channel2, dir);
> > + if (!event)
> > + return -EINVAL;
> > +
> > + if (event->enabled == state)
> > + return 0;
> > +
> > + mutex_lock(&data->mutex);
> > +
> > + ret = mma9551_set_power_state(data->client, state);
> > + if (ret < 0)
> > + goto err_out;
> > + event->enabled = state;
> > +
> > + ret = mma9553_conf_gpio(data);
> > + if (ret < 0)
> > + goto err_conf_gpio;
> > +
> > + mutex_unlock(&data->mutex);
> > +
> > + return ret;
>
> Return 0 as indication of success?
>
Yes.
> > +
> > +err_conf_gpio:
> > + if (state) {
> > + event->enabled = false;
> > + mma9551_set_power_state(data->client, false);
> > + }
> > +err_out:
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > +}
> > +
> > +static int mma9553_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > +
> > + *val2 = 0;
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + switch (chan->type) {
> > + case IIO_STEPS:
> > + *val = mma9553_get_bits(data->conf.speed_step,
> > + MMA9553_MASK_CONF_STEPCOALESCE);
> > + return IIO_VAL_INT;
> > + case IIO_ACTIVITY:
> > + /*
> > + * The device does not support confidence value levels.
> > + * We set an average of 50%.
> > + */
> > + *val = 50;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_EV_INFO_PERIOD:
> > + switch (chan->type) {
> > + case IIO_ACTIVITY:
> > + *val = MMA9553_ACTIVITY_THD_TO_SEC(data->conf.actthd);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int mma9553_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + switch (chan->type) {
> > + case IIO_STEPS:
> > + if (val < 0 || val > 255)
> > + return -EINVAL;
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data,
> > + MMA9553_REG_CONF_SPEED_STEP,
> > + &data->conf.speed_step, val,
> > + MMA9553_MASK_CONF_STEPCOALESCE);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_EV_INFO_PERIOD:
> > + switch (chan->type) {
> > + case IIO_ACTIVITY:
>
> Have a boundary check of val here, as well?
>
Seems this is the only value missing boundary check. Will fix this.
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data, MMA9553_REG_CONF_ACTTHD,
> > + &data->conf.actthd,
> > + MMA9553_ACTIVITY_SEC_TO_THD
> > + (val), MMA9553_MASK_CONF_WORD);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int mma9553_get_calibgender_mode(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + u8 gender;
> > +
> > + gender = mma9553_get_bits(data->conf.filter, MMA9553_MASK_CONF_MALE);
> > + /*
> > + * HW expects 0 for female and 1 for male,
> > + * while iio index is 0 for male and 1 for female
>
> End this sentence with a full stop?
>
> > + */
> > + return !gender;
> > +}
> > +
> > +static int mma9553_set_calibgender_mode(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + unsigned int mode)
> > +{
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + u8 gender = !mode;
> > + int ret;
> > +
> > + if ((mode != 0) && (mode != 1))
> > + return -EINVAL;
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_set_config(data, MMA9553_REG_CONF_FILTER,
> > + &data->conf.filter, gender,
> > + MMA9553_MASK_CONF_MALE);
> > + mutex_unlock(&data->mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct iio_event_spec mma9553_step_event = {
> > + .type = IIO_EV_TYPE_CHANGE,
> > + .dir = IIO_EV_DIR_NONE,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | BIT(IIO_EV_INFO_VALUE),
> > +};
> > +
> > +static const struct iio_event_spec mma9553_activity_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> > + BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> > + BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD),
> > + },
> > +};
> > +
> > +static const char * const calibgender_modes[] = { "male", "female" };
>
> Shouldn't this be prefixed?
>
It should. Will fix.
> > +
> > +static const struct iio_enum mma9553_calibgender_enum = {
> > + .items = calibgender_modes,
> > + .num_items = ARRAY_SIZE(calibgender_modes),
> > + .get = mma9553_get_calibgender_mode,
> > + .set = mma9553_set_calibgender_mode,
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info mma9553_ext_info[] = {
> > + IIO_ENUM("calibgender", IIO_SHARED_BY_TYPE, &mma9553_calibgender_enum),
> > + IIO_ENUM_AVAILABLE("calibgender", &mma9553_calibgender_enum),
> > + {},
> > +};
> > +
> > +#define MMA9553_PEDOMETER_CHANNEL(_type, _mask) { \
> > + .type = _type, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) | \
> > + BIT(IIO_CHAN_INFO_CALIBHEIGHT) | \
> > + _mask, \
> > + .ext_info = mma9553_ext_info, \
> > +}
> > +
> > +#define MMA9553_ACTIVITY_CHANNEL(_chan2) { \
> > + .type = IIO_ACTIVITY, \
> > + .modified = 1, \
> > + .channel2 = _chan2, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT), \
> > + .event_spec = mma9553_activity_events, \
> > + .num_event_specs = ARRAY_SIZE(mma9553_activity_events), \
> > + .ext_info = mma9553_ext_info, \
> > +}
> > +
> > +static const struct iio_chan_spec mma9553_channels[] = {
> > + MMA9551_ACCEL_CHANNEL(IIO_MOD_X),
> > + MMA9551_ACCEL_CHANNEL(IIO_MOD_Y),
> > + MMA9551_ACCEL_CHANNEL(IIO_MOD_Z),
> > +
> > + {
> > + .type = IIO_STEPS,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > + BIT(IIO_CHAN_INFO_ENABLE) |
> > + BIT(IIO_CHAN_INFO_DEBOUNCE_COUNT) |
> > + BIT(IIO_CHAN_INFO_DEBOUNCE_TIME),
> > + .event_spec = &mma9553_step_event,
> > + .num_event_specs = 1,
> > + },
> > +
> > + MMA9553_PEDOMETER_CHANNEL(IIO_DISTANCE, BIT(IIO_CHAN_INFO_PROCESSED)),
> > + {
> > + .type = IIO_VELOCITY,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_ROOT_SUM_SQUARED_X_Y_Z,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_INT_TIME) |
> > + BIT(IIO_CHAN_INFO_ENABLE),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBHEIGHT),
> > + .ext_info = mma9553_ext_info,
> > + },
> > + MMA9553_PEDOMETER_CHANNEL(IIO_ENERGY, BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_CALIBWEIGHT)),
> > +
> > + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_RUNNING),
> > + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_JOGGING),
> > + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_WALKING),
> > + MMA9553_ACTIVITY_CHANNEL(IIO_MOD_STILL),
> > +};
> > +
> > +static const struct iio_info mma9553_info = {
> > + .driver_module = THIS_MODULE,
> > + .read_raw = mma9553_read_raw,
> > + .write_raw = mma9553_write_raw,
> > + .read_event_config = mma9553_read_event_config,
> > + .write_event_config = mma9553_write_event_config,
> > + .read_event_value = mma9553_read_event_value,
> > + .write_event_value = mma9553_write_event_value,
> > +};
> > +
> > +static irqreturn_t mma9553_irq_handler(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > +
> > + data->timestamp = iio_get_time_ns();
> > + /*
> > + * Since we only configure the interrupt pin when an
> > + * event is enabled, we are sure we have at least
> > + * one event enabled at this point.
> > + */
> > + return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t mma9553_event_handler(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + u16 stepcnt;
> > + u8 activity;
> > + struct mma9553_event *ev_activity, *ev_prev_activity, *ev_step_detect;
> > + int ret;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = mma9553_read_activity_stepcnt(data, &activity, &stepcnt);
> > + if (ret < 0) {
> > + mutex_unlock(&data->mutex);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + ev_prev_activity =
> > + mma9553_get_event(data, IIO_ACTIVITY,
> > + mma9553_activity_to_mod(data->activity),
> > + IIO_EV_DIR_FALLING);
> > + ev_activity =
> > + mma9553_get_event(data, IIO_ACTIVITY,
> > + mma9553_activity_to_mod(activity),
> > + IIO_EV_DIR_RISING);
> > + ev_step_detect =
> > + mma9553_get_event(data, IIO_STEPS, IIO_NO_MOD, IIO_EV_DIR_NONE);
> > +
> > + if (ev_step_detect->enabled && (stepcnt != data->stepcnt)) {
> > + data->stepcnt = stepcnt;
> > + iio_push_event(indio_dev,
> > + IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > + IIO_EV_DIR_NONE, IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > + data->timestamp);
> > + }
> > +
> > + if (activity != data->activity) {
> > + data->activity = activity;
> > + /* ev_activity can be NULL if activity == ACTIVITY_UNKNOWN */
> > + if (ev_prev_activity && ev_prev_activity->enabled)
> > + iio_push_event(indio_dev,
> > + IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> > + ev_prev_activity->info->mod,
> > + IIO_EV_DIR_FALLING,
> > + IIO_EV_TYPE_THRESH, 0, 0, 0),
> > + data->timestamp);
> > +
> > + if (ev_activity && ev_activity->enabled)
> > + iio_push_event(indio_dev,
> > + IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> > + ev_activity->info->mod,
> > + IIO_EV_DIR_RISING,
> > + IIO_EV_TYPE_THRESH, 0, 0, 0),
> > + data->timestamp);
> > + }
> > + mutex_unlock(&data->mutex);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mma9553_gpio_probe(struct i2c_client *client)
> > +{
> > + struct device *dev;
> > + struct gpio_desc *gpio;
> > + int ret;
> > +
> > + if (!client)
> > + return -EINVAL;
> > +
> > + dev = &client->dev;
> > +
> > + /* data ready gpio interrupt pin */
>
> Better use upper case for abreviations like GPIO and ACPI here and in
> the messages below.
>
Will fix all instances.
> > + gpio = devm_gpiod_get_index(dev, MMA9553_GPIO_NAME, 0);
> > + if (IS_ERR(gpio)) {
> > + dev_err(dev, "acpi gpio get index failed\n");
> > + return PTR_ERR(gpio);
> > + }
> > +
> > + ret = gpiod_direction_input(gpio);
> > + if (ret)
> > + return ret;
> > +
> > + ret = gpiod_to_irq(gpio);
> > +
> > + dev_dbg(dev, "gpio resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> > +
> > + return ret;
> > +}
> > +
> > +static const char *mma9553_match_acpi_device(struct device *dev)
> > +{
> > + const struct acpi_device_id *id;
> > +
> > + id = acpi_match_device(dev->driver->acpi_match_table, dev);
> > + if (!id)
> > + return NULL;
> > +
> > + return dev_name(dev);
> > +}
> > +
> > +static int mma9553_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct mma9553_data *data;
> > + struct iio_dev *indio_dev;
> > + const char *name = NULL;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > + data->client = client;
> > +
> > + if (id)
> > + name = id->name;
> > + else if (ACPI_HANDLE(&client->dev))
> > + name = mma9553_match_acpi_device(&client->dev);
> > + else
> > + return -ENOSYS;
> > +
> > + mutex_init(&data->mutex);
> > + mma9553_init_events(data);
> > +
> > + ret = mma9553_init(data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->channels = mma9553_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(mma9553_channels);
> > + indio_dev->name = name;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &mma9553_info;
> > +
> > + if (client->irq < 0)
> > + client->irq = mma9553_gpio_probe(client);
> > +
> > + if (client->irq >= 0) {
> > + ret = devm_request_threaded_irq(&client->dev, client->irq,
> > + mma9553_irq_handler,
> > + mma9553_event_handler,
> > + IRQF_TRIGGER_RISING,
> > + MMA9553_IRQ_NAME, indio_dev);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "request irq %d failed\n",
> > + client->irq);
> > + goto out_poweroff;
> > + }
> > +
> > + }
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "unable to register iio device\n");
> > + goto out_poweroff;
> > + }
> > +
> > + ret = pm_runtime_set_active(&client->dev);
> > + if (ret < 0)
> > + goto out_iio_unregister;
> > +
> > + pm_runtime_enable(&client->dev);
> > + pm_runtime_set_autosuspend_delay(&client->dev,
> > + MMA9551_AUTO_SUSPEND_DELAY_MS);
> > + pm_runtime_use_autosuspend(&client->dev);
> > +
> > + dev_dbg(&indio_dev->dev, "Registered device %s\n", name);
> > +
> > + return 0;
> > +
> > +out_iio_unregister:
> > + iio_device_unregister(indio_dev);
> > +out_poweroff:
> > + mma9551_set_device_state(client, false);
> > + return ret;
> > +}
> > +
> > +static int mma9553_remove(struct i2c_client *client)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > +
> > + pm_runtime_disable(&client->dev);
> > + pm_runtime_set_suspended(&client->dev);
> > + pm_runtime_put_noidle(&client->dev);
> > +
> > + iio_device_unregister(indio_dev);
> > + mutex_lock(&data->mutex);
> > + mma9551_set_device_state(data->client, false);
> > + mutex_unlock(&data->mutex);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int mma9553_runtime_suspend(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_set_device_state(data->client, false);
> > + mutex_unlock(&data->mutex);
> > + if (ret < 0) {
> > + dev_err(&data->client->dev, "powering off device failed\n");
> > + return -EAGAIN;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mma9553_runtime_resume(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = mma9551_set_device_state(data->client, true);
> > + if (ret < 0)
> > + return ret;
> > +
> > + mma9551_sleep(MMA9553_DEFAULT_SAMPLE_RATE);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mma9553_suspend(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_set_device_state(data->client, false);
> > + mutex_unlock(&data->mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static int mma9553_resume(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct mma9553_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = mma9551_set_device_state(data->client, true);
> > + mutex_unlock(&data->mutex);
> > +
> > + return ret;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops mma9553_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume)
> > + SET_RUNTIME_PM_OPS(mma9553_runtime_suspend,
> > + mma9553_runtime_resume, NULL)
> > +};
> > +
> > +static const struct acpi_device_id mma9553_acpi_match[] = {
> > + {"MMA9553", 0},
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, mma9553_acpi_match);
> > +
> > +static const struct i2c_device_id mma9553_id[] = {
> > + {"mma9553", 0},
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, mma9553_id);
> > +
> > +static struct i2c_driver mma9553_driver = {
> > + .driver = {
> > + .name = MMA9553_DRV_NAME,
> > + .acpi_match_table = ACPI_PTR(mma9553_acpi_match),
> > + .pm = &mma9553_pm_ops,
> > + },
> > + .probe = mma9553_probe,
> > + .remove = mma9553_remove,
> > + .id_table = mma9553_id,
> > +};
> > +
> > +module_i2c_driver(mma9553_driver);
> > +
> > +MODULE_AUTHOR("Irina Tirdea <[email protected]>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MMA9553L pedometer platform driver");
> > -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to
> [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Tirdea, Irina
> Sent: 06 April, 2015 17:33
> To: Hartmut Knaack; Jonathan Cameron; [email protected]
> Cc: [email protected]; Baluta, Daniel; Lars-Peter Clausen; Peter Meerwald
> Subject: RE: [PATCH v3 3/3] iio: add driver for Freescale MMA9553
>
>
>
> > -----Original Message-----
> > From: Hartmut Knaack [mailto:[email protected]]
> > Sent: 05 April, 2015 2:18
> > To: Tirdea, Irina; Jonathan Cameron; [email protected]
> > Cc: [email protected]; Baluta, Daniel; Lars-Peter Clausen; Peter Meerwald
> > Subject: Re: [PATCH v3 3/3] iio: add driver for Freescale MMA9553
> >
> > Irina Tirdea schrieb am 27.01.2015 um 19:41:
> > > Add support for Freescale MMA9553L Intelligent Pedometer Platform.
> > >
> > > The following functionalities are supported:
> > > - step counter (counts the number of steps using a HW register)
> > > - step detector (generates an iio event at every step the user takes)
> > > - activity recognition (rest, walking, jogging, running)
> > > - speed
> > > - calories
> > > - distance
> > >
<snip>
> > > +static int mma9553_read_activity_stepcnt(struct mma9553_data *data,
> > > + u8 *activity, u16 *stepcnt)
> > > +{
> > > + u32 status_stepcnt;
> > > + u16 status;
> > > + int ret;
> > > +
> > > + ret = mma9551_read_status_words(data->client, MMA9551_APPID_PEDOMETER,
> > > + MMA9553_REG_STATUS, sizeof(u32),
> > > + (u16 *) &status_stepcnt);
> > > + if (ret < 0) {
> > > + dev_err(&data->client->dev,
> > > + "error reading status and stepcnt\n");
> > > + return ret;
> > > + }
> > > +
> >
> > I think this could be done a bit simpler by using u16 buf[2] instead of
> > status_stepcnt (making status obsolete). That's what it would boil down
> > to:
> > *activity = mma9553_get_bits(buf[0], MMA9553_MASK_STATUS_ACTIVITY);
> > *stepcnt = buf[1];
> >
> That does look more simple. Will change it accordingly.
This does not only look more simple, it actually fixes an endianness bug on big endian hosts.
Thanks for the suggestion!
Irina
<snip>