Re: [PATCH 02/10] mfd: cros_ec: update MOTIONSENSE definitions and commands.
From: Enric Balletbo Serra
Date: Tue Jul 19 2016 - 14:00:45 EST
Hi Jonathan,
Many thanks for your comments.
2016-07-18 15:49 GMT+02:00 Jonathan Cameron <jic23@xxxxxxxxxx>:
> On 18/07/16 08:02, Enric Balletbo i Serra wrote:
>> Let's update the command header to include the definitions related to
>> the sensors attached behind the ChromeOS Embedded Controller. The new
>> commands and definitions allow us to get information from these sensors.
>>
>> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> Again, I'd be happier seeing this stuff introduced as and when it
> is needed rather than in a magic patch. It's hard to review stuff
> if it's broken up across multiple patches like this.
>
> A few other bits and pieces inline.
>
> Jonathan
>> ---
>> include/linux/mfd/cros_ec_commands.h | 260 +++++++++++++++++++++++++++++++----
>> 1 file changed, 231 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
>> index 76728ff..f26a806 100644
>> --- a/include/linux/mfd/cros_ec_commands.h
>> +++ b/include/linux/mfd/cros_ec_commands.h
>> @@ -1290,7 +1290,13 @@ enum motionsense_command {
>>
>> /*
>> * EC Rate command is a setter/getter command for the EC sampling rate
>> - * of all motion sensors in milliseconds.
>> + * in milliseconds.
>> + * It is per sensor, the EC run sample task at the minimum of all
>> + * sensors EC_RATE.
>> + * For sensors without hardware FIFO, EC_RATE should be equals to 1/ODR
>> + * to collect all the sensor samples.
>> + * For sensor with hardware FIFO, EC_RATE is used as the maximal delay
>> + * to process of all motion sensors in milliseconds.
>> */
>> MOTIONSENSE_CMD_EC_RATE = 2,
>>
>> @@ -1315,37 +1321,138 @@ enum motionsense_command {
>> */
>> MOTIONSENSE_CMD_KB_WAKE_ANGLE = 5,
>>
>> - /* Number of motionsense sub-commands. */
>> - MOTIONSENSE_NUM_CMDS
>> -};
>> + /*
>> + * Returns a single sensor data.
>> + */
> Please use standard kernel documentation formats throughout.
> If not you may face a Linus rant ;)
Seems that this specific file does not follow the kernel-doc format
strictly [1], should I add the new entries in kernel-doc format or
follow the file style?
Or maybe I should first do a patch to change all the file to the
kernel-doc format?
Or you meant only replace the
/*
* one line
*/
for
/* one line */
[1] https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>> + MOTIONSENSE_CMD_DATA = 6,
>> +
>> + /*
>> + * Return sensor fifo info.
>> + */
>> + MOTIONSENSE_CMD_FIFO_INFO = 7,
>> +
>> + /*
>> + * Insert a flush element in the fifo and return sensor fifo info.
>> + * The host can use that element to synchronize its operation.
>> + */
>> + MOTIONSENSE_CMD_FIFO_FLUSH = 8,
>>
>> -enum motionsensor_id {
>> - EC_MOTION_SENSOR_ACCEL_BASE = 0,
>> - EC_MOTION_SENSOR_ACCEL_LID = 1,
>> - EC_MOTION_SENSOR_GYRO = 2,
>> + /*
>> + * Return a portion of the fifo.
>> + */
>> + MOTIONSENSE_CMD_FIFO_READ = 9,
>> +
>> + /*
>> + * Perform low level calibration.
>> + * On sensors that support it, ask to do offset calibration.
>> + */
>> + MOTIONSENSE_CMD_PERFORM_CALIB = 10,
>> +
>> + /*
>> + * Sensor Offset command is a setter/getter command for the offset
>> + * used for calibration.
>> + * The offsets can be calculated by the host, or via
>> + * PERFORM_CALIB command.
>> + */
>> + MOTIONSENSE_CMD_SENSOR_OFFSET = 11,
>> +
>> + /*
>> + * List available activities for a MOTION sensor.
>> + * Indicates if they are enabled or disabled.
>> + */
>> + MOTIONSENSE_CMD_LIST_ACTIVITIES = 12,
>>
>> /*
>> - * Note, if more sensors are added and this count changes, the padding
>> - * in ec_response_motion_sense dump command must be modified.
>> + * Activity management
>> + * Enable/Disable activity recognition.
>> */
>> - EC_MOTION_SENSOR_COUNT = 3
>> + MOTIONSENSE_CMD_SET_ACTIVITY = 13,
>> +
>> + /* Number of motionsense sub-commands. */
>> + MOTIONSENSE_NUM_CMDS
>> };
>>
>> /* List of motion sensor types. */
>> enum motionsensor_type {
>> MOTIONSENSE_TYPE_ACCEL = 0,
>> MOTIONSENSE_TYPE_GYRO = 1,
>> + MOTIONSENSE_TYPE_MAG = 2,
>> + MOTIONSENSE_TYPE_PROX = 3,
>> + MOTIONSENSE_TYPE_LIGHT = 4,
>> + MOTIONSENSE_TYPE_ACTIVITY = 5,
>> + MOTIONSENSE_TYPE_MAX,
>> };
>>
>> /* List of motion sensor locations. */
>> enum motionsensor_location {
>> MOTIONSENSE_LOC_BASE = 0,
>> MOTIONSENSE_LOC_LID = 1,
>> + MOTIONSENSE_LOC_MAX,
>> };
>>
>> /* List of motion sensor chips. */
>> enum motionsensor_chip {
>> MOTIONSENSE_CHIP_KXCJ9 = 0,
>> + MOTIONSENSE_CHIP_LSM6DS0 = 1,
>> + MOTIONSENSE_CHIP_BMI160 = 2,
>> + MOTIONSENSE_CHIP_SI1141 = 3,
>> + MOTIONSENSE_CHIP_SI1142 = 4,
>> + MOTIONSENSE_CHIP_SI1143 = 5,
>> + MOTIONSENSE_CHIP_KX022 = 6,
>> + MOTIONSENSE_CHIP_L3GD20H = 7,
> Interesting. So the driver needs some knowledge of what
> is behind it. I'll read on with interest ;)
>> +};
>> +
>> +struct ec_response_motion_sensor_data {
>> + /* Flags for each sensor. */
>> + uint8_t flags;
>> + /* sensor number the data comes from */
>> + uint8_t sensor_num;
>> + /* Each sensor is up to 3-axis. */
>> + union {
>> + int16_t data[3];
>> + struct {
>> + uint16_t rsvd;
>> + uint32_t timestamp;
>> + } __packed;
>> + struct {
>> + uint8_t activity; /* motionsensor_activity */
>> + uint8_t state;
>> + int16_t add_info[2];
>> + };
>> + };
>> +} __packed;
>> +
>> +struct ec_response_motion_sense_fifo_info {
>> + /* Size of the fifo */
>> + uint16_t size;
>> + /* Amount of space used in the fifo */
>> + uint16_t count;
>> + /* TImestamp recorded in us */
> Timestamp
>> + uint32_t timestamp;
>> + /* Total amount of vector lost */
>> + uint16_t total_lost;
>> + /* Lost events since the last fifo_info, per sensors */
>> + uint16_t lost[0];
>> +} __packed;
>> +
>> +struct ec_response_motion_sense_fifo_data {
>> + uint32_t number_data;
>> + struct ec_response_motion_sensor_data data[0];
>> +} __packed;
>> +
>> +/* List supported activity recognition */
>> +enum motionsensor_activity {
>> + MOTIONSENSE_ACTIVITY_RESERVED = 0,
>> + MOTIONSENSE_ACTIVITY_SIG_MOTION = 1,
>> + MOTIONSENSE_ACTIVITY_DOUBLE_TAP = 2,
>> +};
>> +
>> +struct ec_motion_sense_activity {
>> + uint8_t sensor_num;
>> + uint8_t activity; /* one of enum motionsensor_activity */
>> + uint8_t enable; /* 1: enable, 0: disable */
>> + uint8_t reserved;
>> + uint16_t parameters[3]; /* activity dependent parameters */
>> };
>>
>> /* Module flag masks used for the dump sub-command. */
>> @@ -1355,41 +1462,61 @@ enum motionsensor_chip {
>> #define MOTIONSENSE_SENSOR_FLAG_PRESENT (1<<0)
>>
>> /*
>> + * Flush entry for synchronisation.
>> + * data contains time stamp
>> + */
>> +#define MOTIONSENSE_SENSOR_FLAG_FLUSH (1<<0)
>> +#define MOTIONSENSE_SENSOR_FLAG_TIMESTAMP (1<<1)
>> +#define MOTIONSENSE_SENSOR_FLAG_WAKEUP (1<<2)
>> +
>> +/*
>> * Send this value for the data element to only perform a read. If you
>> * send any other value, the EC will interpret it as data to set and will
>> * return the actual value set.
>> */
>> #define EC_MOTION_SENSE_NO_VALUE -1
>>
>> +#define EC_MOTION_SENSE_INVALID_CALIB_TEMP 0x8000
>> +
>> +/* MOTIONSENSE_CMD_SENSOR_OFFSET subcommand flag */
>> +/* Set Calibration information */
>> +#define MOTION_SENSE_SET_OFFSET 1
>> +
>> struct ec_params_motion_sense {
>> uint8_t cmd;
>> union {
>> - /* Used for MOTIONSENSE_CMD_DUMP. */
>> + /* Used for MOTIONSENSE_CMD_DUMP */
>> struct {
>> - /* no args */
>> + /*
>> + * Maximal number of sensor the host is expecting.
>> + * 0 means the host is only interested in the number
>> + * of sensors controlled by the EC.
>> + */
>> + uint8_t max_sensor_count;
>> } dump;
>>
>> /*
>> - * Used for MOTIONSENSE_CMD_EC_RATE and
>> - * MOTIONSENSE_CMD_KB_WAKE_ANGLE.
>> + * Used for MOTIONSENSE_CMD_KB_WAKE_ANGLE.
>> */
>> struct {
>> - /* Data to set or EC_MOTION_SENSE_NO_VALUE to read. */
>> + /* Data to set or EC_MOTION_SENSE_NO_VALUE to read.
>> + * kb_wake_angle: angle to wakup AP.
>> + */
>> int16_t data;
>> - } ec_rate, kb_wake_angle;
>> + } kb_wake_angle;
>>
>> - /* Used for MOTIONSENSE_CMD_INFO. */
>> + /* Used for MOTIONSENSE_CMD_INFO, MOTIONSENSE_CMD_DATA
>> + * and MOTIONSENSE_CMD_PERFORM_CALIB.
>> + */
>> struct {
>> - /* Should be element of enum motionsensor_id. */
>> uint8_t sensor_num;
>> - } info;
>> + } info, data, fifo_flush, perform_calib, list_activities;
>>
>> /*
>> - * Used for MOTIONSENSE_CMD_SENSOR_ODR and
>> - * MOTIONSENSE_CMD_SENSOR_RANGE.
>> + * Used for MOTIONSENSE_CMD_EC_RATE, MOTIONSENSE_CMD_SENSOR_ODR
>> + * and MOTIONSENSE_CMD_SENSOR_RANGE.
>> */
>> struct {
>> - /* Should be element of enum motionsensor_id. */
>> uint8_t sensor_num;
>>
>> /* Rounding flag, true for round-up, false for down. */
>> @@ -1399,22 +1526,69 @@ struct ec_params_motion_sense {
>>
>> /* Data to set or EC_MOTION_SENSE_NO_VALUE to read. */
>> int32_t data;
>> - } sensor_odr, sensor_range;
>> + } ec_rate, sensor_odr, sensor_range;
>> +
>> + /* Used for MOTIONSENSE_CMD_SENSOR_OFFSET */
>> + struct {
>> + uint8_t sensor_num;
>> +
>> + /*
>> + * bit 0: If set (MOTION_SENSE_SET_OFFSET), set
>> + * the calibration information in the EC.
>> + * If unset, just retrieve calibration information.
>> + */
>> + uint16_t flags;
>> +
>> + /*
>> + * Temperature at calibration, in units of 0.01 C
>> + * 0x8000: invalid / unknown.
>> + * 0x0: 0C
>> + * 0x7fff: +327.67C
>> + */
>> + int16_t temp;
>> +
>> + /*
>> + * Offset for calibration.
>> + * Unit:
>> + * Accelerometer: 1/1024 g
>> + * Gyro: 1/1024 deg/s
>> + * Compass: 1/16 uT
>> + */
>> + int16_t offset[3];
>> + } __packed sensor_offset;
>> +
>> + /* Used for MOTIONSENSE_CMD_FIFO_INFO */
>> + struct {
>> + } fifo_info;
>> +
>> + /* Used for MOTIONSENSE_CMD_FIFO_READ */
>> + struct {
>> + /*
>> + * Number of expected vector to return.
>> + * EC may return less or 0 if none available.
>> + */
>> + uint32_t max_data_vector;
>> + } fifo_read;
>> +
>> + struct ec_motion_sense_activity set_activity;
>> };
>> } __packed;
>>
>> struct ec_response_motion_sense {
>> union {
>> - /* Used for MOTIONSENSE_CMD_DUMP. */
>> + /* Used for MOTIONSENSE_CMD_DUMP */
>> struct {
>> /* Flags representing the motion sensor module. */
>> uint8_t module_flags;
>>
>> - /* Flags for each sensor in enum motionsensor_id. */
>> - uint8_t sensor_flags[EC_MOTION_SENSOR_COUNT];
>> + /* Number of sensors managed directly by the EC */
>> + uint8_t sensor_count;
>>
>> - /* Array of all sensor data. Each sensor is 3-axis. */
>> - int16_t data[3*EC_MOTION_SENSOR_COUNT];
>> + /*
>> + * sensor data is truncated if response_max is too small
>> + * for holding all the data.
>> + */
>> + struct ec_response_motion_sensor_data sensor[0];
>> } dump;
>>
>> /* Used for MOTIONSENSE_CMD_INFO. */
>> @@ -1429,6 +1603,9 @@ struct ec_response_motion_sense {
>> uint8_t chip;
>> } info;
>>
>> + /* Used for MOTIONSENSE_CMD_DATA */
>> + struct ec_response_motion_sensor_data data;
>> +
>> /*
>> * Used for MOTIONSENSE_CMD_EC_RATE, MOTIONSENSE_CMD_SENSOR_ODR,
>> * MOTIONSENSE_CMD_SENSOR_RANGE, and
>> @@ -1438,6 +1615,25 @@ struct ec_response_motion_sense {
>> /* Current value of the parameter queried. */
>> int32_t ret;
>> } ec_rate, sensor_odr, sensor_range, kb_wake_angle;
>> +
>> + /* Used for MOTIONSENSE_CMD_SENSOR_OFFSET */
>> + struct {
>> + int16_t temp;
>> + int16_t offset[3];
>> + } sensor_offset, perform_calib;
>> +
>> + struct ec_response_motion_sense_fifo_info fifo_info, fifo_flush;
>> +
>> + struct ec_response_motion_sense_fifo_data fifo_read;
>> +
>> + struct {
>> + uint16_t reserved;
>> + uint32_t enabled;
>> + uint32_t disabled;
>> + } __packed list_activities;
>> +
>> + struct {
>> + } set_activity;
>> };
>> } __packed;
>>
>> @@ -1819,6 +2015,12 @@ union ec_response_get_next_data {
>>
>> /* Unaligned */
>> uint32_t host_event;
>> +
>> + struct {
>> + /* For aligning the fifo_info */
>> + uint8_t rsvd[3];
>> + struct ec_response_motion_sense_fifo_info info;
>> + } sensor_fifo;
>> } __packed;
>>
>> struct ec_response_get_next_event {
>>
>