Karol Wrona schrieb am 21.11.2014 um 19:19: > Sensorhub is MCU dedicated to collect data and manage several sensors. > Sensorhub is a spi device which provides a layer for IIO devices. It provides > some data parsing and common mechanism for sensorhub sensors. > > Adds common sensorhub library for sensorhub driver and iio drivers > which uses sensorhub MCU to communicate with sensors. Quite massive. One major issue, also in the other patches, is that you miss to add the ssp_ prefix to functions, variables and definitions in some cases. Please take care of all of them. Also find some comments inline. > > Signed-off-by: Karol Wrona <k.wrona@xxxxxxxxxxx> > Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > --- > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/sensorhub/Kconfig | 13 + > drivers/misc/sensorhub/Makefile | 6 + > drivers/misc/sensorhub/ssp.h | 279 +++++++++++ > drivers/misc/sensorhub/ssp_dev.c | 828 ++++++++++++++++++++++++++++++++ > drivers/misc/sensorhub/ssp_spi.c | 653 +++++++++++++++++++++++++ > include/linux/iio/common/ssp_sensors.h | 79 +++ > 8 files changed, 1860 insertions(+) > create mode 100644 drivers/misc/sensorhub/Kconfig > create mode 100644 drivers/misc/sensorhub/Makefile > create mode 100644 drivers/misc/sensorhub/ssp.h > create mode 100644 drivers/misc/sensorhub/ssp_dev.c > create mode 100644 drivers/misc/sensorhub/ssp_spi.c > create mode 100644 include/linux/iio/common/ssp_sensors.h > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index b2e68c1..89001ce 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -529,4 +529,5 @@ source "drivers/misc/genwqe/Kconfig" > source "drivers/misc/echo/Kconfig" > source "drivers/misc/cxl/Kconfig" > source "drivers/misc/stm32fwu/Kconfig" > +source "drivers/misc/sensorhub/Kconfig" > endmenu > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 88c8999..27d0881 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -57,3 +57,4 @@ obj-$(CONFIG_ECHO) += echo/ > obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o > obj-$(CONFIG_CXL_BASE) += cxl/ > obj-$(CONFIG_STM32_UPGRADE_PROTOCOL) += stm32fwu/ > +obj-$(CONFIG_SENSORS_SAMSUNG_SSP) += sensorhub/ > diff --git a/drivers/misc/sensorhub/Kconfig b/drivers/misc/sensorhub/Kconfig > new file mode 100644 > index 0000000..a77dc1f > --- /dev/null > +++ b/drivers/misc/sensorhub/Kconfig > @@ -0,0 +1,13 @@ > +# > +# sensor drivers configuration > +# > +config SENSORS_SAMSUNG_SSP > + tristate "Samsung Sensorhub driver" > + depends on SPI > + select STM32_UPGRADE_PROTOCOL > + help > + SSP driver for sensor hub. > + If you say yes here you get ssp support for > + sensor hub. > + To compile this driver as a module, choose M here: the > + module will be called ssp_dev. > diff --git a/drivers/misc/sensorhub/Makefile b/drivers/misc/sensorhub/Makefile > new file mode 100644 > index 0000000..c40ed72 > --- /dev/null > +++ b/drivers/misc/sensorhub/Makefile > @@ -0,0 +1,6 @@ > +# > +# Makefile for the sensor drivers. > +# > + > +obj-$(CONFIG_SENSORS_SAMSUNG_SSP) += ssp_dev.o ssp_spi.o > + > diff --git a/drivers/misc/sensorhub/ssp.h b/drivers/misc/sensorhub/ssp.h > new file mode 100644 > index 0000000..e19ad21 > --- /dev/null > +++ b/drivers/misc/sensorhub/ssp.h > @@ -0,0 +1,279 @@ > +/* > + * Copyright (C) 2011, Samsung Electronics Co. Ltd. All Rights Reserved. Maybe add 2014? > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that 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. > + * > + */ > + > +#ifndef __SSP_SENSORHUB_H__ > +#define __SSP_SENSORHUB_H__ > + > +#include <linux/delay.h> > +#include <linux/gpio.h> > +#include <linux/iio/common/ssp_sensors.h> > +#include <linux/iio/iio.h> > +#include <linux/spi/spi.h> > + > +#define SSP_DEVICE_ID 0x55 > + > +#ifdef SSP_DBG > +#define ssp_dbg(format, ...) pr_info("[SSP] "format, ##__VA_ARGS__) > +#else > +#define ssp_dbg(format, ...) > +#endif > + > +#define SSP_SW_RESET_TIME 3000 > +/* Sensor polling in ms */ > +#define SSP_DEFUALT_POLLING_DELAY 200 Typo: SSP_DEFAULT_POLLING_DELAY > +#define SSP_DEFAULT_RETRIES 3 > +#define SSP_DATA_PACKET_SIZE 960 > + > +enum { > + SSP_KERNEL_BINARY = 0, > + SSP_KERNEL_CRASHED_BINARY, > +}; > + > +enum { > + SSP_INITIALIZATION_STATE = 0, > + SSP_NO_SENSOR_STATE, > + SSP_ADD_SENSOR_STATE, > + SSP_RUNNING_SENSOR_STATE, > +}; > + > +/* Firmware download STATE */ > +enum { > + SSP_FW_DL_STATE_FAIL = -1, > + SSP_FW_DL_STATE_NONE = 0, > + SSP_FW_DL_STATE_NEED_TO_SCHEDULE, > + SSP_FW_DL_STATE_SCHEDULED, > + SSP_FW_DL_STATE_DOWNLOADING, > + SSP_FW_DL_STATE_SYNC, > + SSP_FW_DL_STATE_DONE, > +}; > + > +#define SSP_INVALID_REVISION 99999 > +#define SSP_INVALID_REVISION2 0xffffff > + > +/* AP -> SSP Instruction */ > +#define SSP_MSG2SSP_INST_BYPASS_SENSOR_ADD 0xa1 > +#define SSP_MSG2SSP_INST_BYPASS_SENSOR_REMOVE 0xa2 > +#define SSP_MSG2SSP_INST_REMOVE_ALL 0xa3 > +#define SSP_MSG2SSP_INST_CHANGE_DELAY 0xa4 > +#define SSP_MSG2SSP_INST_LIBRARY_ADD 0xb1 > +#define SSP_MSG2SSP_INST_LIBRARY_REMOVE 0xb2 > +#define SSP_MSG2SSP_INST_LIB_NOTI 0xb4 > +#define SSP_MSG2SSP_INST_LIB_DATA 0xc1 > + > +#define SSP_MSG2SSP_AP_MCU_SET_GYRO_CAL 0xcd > +#define SSP_MSG2SSP_AP_MCU_SET_ACCEL_CAL 0xce > +#define SSP_MSG2SSP_AP_STATUS_SHUTDOWN 0xd0 > +#define SSP_MSG2SSP_AP_STATUS_WAKEUP 0xd1 > +#define SSP_MSG2SSP_AP_STATUS_SLEEP 0xd2 > +#define SSP_MSG2SSP_AP_STATUS_RESUME 0xd3 > +#define SSP_MSG2SSP_AP_STATUS_SUSPEND 0xd4 > +#define SSP_MSG2SSP_AP_STATUS_RESET 0xd5 > +#define SSP_MSG2SSP_AP_STATUS_POW_CONNECTED 0xd6 > +#define SSP_MSG2SSP_AP_STATUS_POW_DISCONNECTED 0xd7 > +#define SSP_MSG2SSP_AP_TEMPHUMIDITY_CAL_DONE 0xda > +#define SSP_MSG2SSP_AP_MCU_SET_DUMPMODE 0xdb > +#define SSP_MSG2SSP_AP_MCU_DUMP_CHECK 0xdc > +#define SSP_MSG2SSP_AP_MCU_BATCH_FLUSH 0xdd > +#define SSP_MSG2SSP_AP_MCU_BATCH_COUNT 0xdf > + > +#define SSP_MSG2SSP_AP_WHOAMI 0x0f > +#define SSP_MSG2SSP_AP_FIRMWARE_REV 0xf0 > +#define SSP_MSG2SSP_AP_SENSOR_FORMATION 0xf1 > +#define SSP_MSG2SSP_AP_SENSOR_PROXTHRESHOLD 0xf2 > +#define SSP_MSG2SSP_AP_SENSOR_BARCODE_EMUL 0xf3 > +#define SSP_MSG2SSP_AP_SENSOR_SCANNING 0xf4 > +#define SSP_MSG2SSP_AP_SET_MAGNETIC_HWOFFSET 0xf5 > +#define SSP_MSG2SSP_AP_GET_MAGNETIC_HWOFFSET 0xf6 > +#define SSP_MSG2SSP_AP_SENSOR_GESTURE_CURRENT 0xf7 > +#define SSP_MSG2SSP_AP_GET_THERM 0xf8 > +#define SSP_MSG2SSP_AP_GET_BIG_DATA 0xf9 > +#define SSP_MSG2SSP_AP_SET_BIG_DATA 0xfa Double check these two values, as in other cases _SET has lower value than _GET. > +#define SSP_MSG2SSP_AP_START_BIG_DATA 0xfb > +#define SSP_MSG2SSP_AP_SET_MAGNETIC_STATIC_MATRIX 0xfd > +#define SSP_MSG2SSP_AP_SENSOR_TILT 0xea > +#define SSP_MSG2SSP_AP_MCU_SET_TIME 0xfe > +#define SSP_MSG2SSP_AP_MCU_GET_TIME 0xff > + > +#define SSP_MSG2SSP_AP_FUSEROM 0x01 Leave an empty line? > +/* voice data */ > +#define SSP_TYPE_WAKE_UP_VOICE_SERVICE 0x01 > +#define SSP_TYPE_WAKE_UP_VOICE_SOUND_SOURCE_AM 0x01 > +#define SSP_TYPE_WAKE_UP_VOICE_SOUND_SOURCE_GRAMMER 0x02 > + > +/* Factory Test */ > +#define SSP_ACCELEROMETER_FACTORY 0x80 > +#define SSP_GYROSCOPE_FACTORY 0x81 > +#define SSP_GEOMAGNETIC_FACTORY 0x82 > +#define SSP_PRESSURE_FACTORY 0x85 > +#define SSP_GESTURE_FACTORY 0x86 > +#define SSP_TEMPHUMIDITY_CRC_FACTORY 0x88 > +#define SSP_GYROSCOPE_TEMP_FACTORY 0x8a > +#define SSP_GYROSCOPE_DPS_FACTORY 0x8b > +#define SSP_MCU_FACTORY 0x8c > +#define SSP_MCU_SLEEP_FACTORY 0x8d > + > +/* SSP -> AP ACK about write CMD */ > +#define SSP_MSG_ACK 0x80 /* ACK from SSP to AP */ > +#define SSP_MSG_NAK 0x70 /* NAK from SSP to AP */ > + > +/* Accelerometer sensor*/ > +/* 16bits */ > +/* FIXME rev min max for others */ Use multiline comment style? Otherwise mind a space at the end of the first comment line. > +#define SSP_MAX_ACCEL_1G 16384 > +#define SSP_MAX_ACCEL_2G 32767 > +#define SSP_MIN_ACCEL_2G -32768 > +#define SSP_MAX_ACCEL_4G 65536 > + > +#define SSP_MAX_GYRO 32767 > +#define SSP_MIN_GYRO -32768 > + > +struct ssp_sensorhub_info { > + char *fw_name; > + char *fw_crashed_name; > + unsigned int fw_rev; > + const u8 * const mag_table; > + const unsigned int mag_length; > +}; > + > +/* ssp_msg options bit*/ Mind a space. > +#define SSP_RW 0 > +#define SSP_INDEX 3 > + > +enum { > + SSP_AP2HUB_READ = 0, > + SSP_AP2HUB_WRITE, > + SSP_HUB2AP_WRITE, > + SSP_AP2HUB_READY, > + SSP_AP2HUB_RETURN These values should be explicitly defined, to prevent failures. > +}; > + > +enum { > + SSP_BIG_TYPE_DUMP = 0, > + SSP_BIG_TYPE_READ_LIB, > + SSP_BIG_TYPE_VOICE_NET, > + SSP_BIG_TYPE_VOICE_GRAM, > + SSP_BIG_TYPE_VOICE_PCM, > + SSP_BIG_TYPE_TEMP, > + SSP_BIG_TYPE_MAX, > +}; > + > +/** > + * struct ssp_data - ssp platformdata structure > + * @spi: spi device > + * @sensorhub_info: info about sensorhub board specific features > + * @wdt_timer: watchdog timer > + * @work_wdt: watchdog work > + * @work_firmware: firmware upgrade work queue > + * @work_refresh: refresh worqueue for reset request from MCU typo: work queue > + * @shut_down: shut down flag > + * @mcu_dump_mode: mcu dump mode for debug > + * @time_syncing: time syncing indication flag > + * @timestamp: previous time in ns calculated for time syncing > + * @check_status: status table for each sensor > + * @com_fail_cnt: communication fail count > + * @reset_cnt: reset count > + * @time_out_cnt: timeout count > + * @available_sensors: available sensors seen by sensorhub (bit array) > + * @cur_firm_rev: cached current firmware revision > + * @last_resume_state: last AP resume/suspend state used to handle > + * the PM state of ssp Move some parts into the previous line and indent the rest more to indicate its status of description. > + * @last_ap_state: (obsolete) sleep notification for MCU > + * @sensor_enable: sensor enable mask > + * @delay_buf: data acquisition intervals table > + * @batch_latency_buf: jet unknown but existing in communication protocol > + * @batch_opt_buf: jet unknown but existing in communication protocol > + * @accel_position: jet unknown but existing in communication protocol > + * @mag_position: jet unknown but existing in communication protocol Typo: yet > + * @fw_dl_state: firmware download state > + * @comm_lock: lock protecting the handshake > + * @pending_lock: lock protecting pending list and completion > + * @mcu_reset: mcu reset line > + * @ap_mcu_gpio: ap to mcu gpio line > + * @mcu_ap_gpio: mcu to ap gpio line > + * @pending_list: pending list for messages queued to be sent/read > + * @sensor_devs: registered IIO devices table > + * @enable_refcount: enable reference count for wdt timer (watchdog) for wdt (watchdog timer) > + */ > +struct ssp_data { > + struct spi_device *spi; > + struct ssp_sensorhub_info *sensorhub_info; > + struct timer_list wdt_timer; > + struct work_struct work_wdt; > + struct delayed_work work_firmware; > + struct delayed_work work_refresh; > + > + bool shut_down; > + bool mcu_dump_mode; > + bool time_syncing; > + int64_t timestamp; > + > + int check_status[SSP_SENSOR_MAX]; > + > + unsigned int com_fail_cnt; > + unsigned int reset_cnt; > + unsigned int time_out_cnt; rename to timeout_cnt? > + > + unsigned int available_sensors; > + unsigned int cur_firm_rev; > + > + char last_resume_state; > + char last_ap_state; > + > + unsigned int sensor_enable; > + u32 delay_buf[SSP_SENSOR_MAX]; > + s32 batch_latency_buf[SSP_SENSOR_MAX]; > + s8 batch_opt_buf[SSP_SENSOR_MAX]; > + > + int accel_position; > + int mag_position; > + int fw_dl_state; > + > + struct mutex comm_lock; > + struct mutex pending_lock; > + > + int mcu_reset; > + int ap_mcu_gpio; > + int mcu_ap_gpio; > + > + struct list_head pending_list; > + > + struct iio_dev *sensor_devs[SSP_SENSOR_MAX]; > + atomic_t enable_refcount; > +}; > + > +void ssp_clean_pending_list(struct ssp_data *data); > + > +int ssp_command(struct ssp_data *data, char command, int arg); > + > +int ssp_send_instruction(struct ssp_data *, u8 inst, u8 sensor_type, > + u8 *send_buf, u8 length); int ssp_send_instruction(struct ssp_data *data, u8 inst, u8 sensor_type, u8 *send_buf, u8 length); > + > +int ssp_irq_msg(struct ssp_data *data); > + > +int ssp_get_chipid(struct ssp_data *data); > + > +int ssp_get_fuserom_data(struct ssp_data *data); Unused? > + > +int ssp_set_sensor_position(struct ssp_data *data); > + > +int ssp_set_magnetic_matrix(struct ssp_data *data); > + > +unsigned int ssp_get_sensor_scanning_info(struct ssp_data *data); > + > +unsigned int ssp_get_firmware_rev(struct ssp_data *data); > + > +int ssp_queue_ssp_refresh_task(struct ssp_data *data, int delay); > + > +#endif /* __SSP_SENSORHUB_H__ */ > diff --git a/drivers/misc/sensorhub/ssp_dev.c b/drivers/misc/sensorhub/ssp_dev.c > new file mode 100644 > index 0000000..09954c5 > --- /dev/null > +++ b/drivers/misc/sensorhub/ssp_dev.c > @@ -0,0 +1,828 @@ > +/* > +* Copyright (C) 2012, Samsung Electronics Co. Ltd. All Rights Reserved. 2014? > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that 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/firmware.h> > +#include <linux/iio/iio.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/of_platform.h> > +#include <linux/stm32fwu.h> > +#include "ssp.h" > + > +#define SSP_WDT_TIME 10000 > +#define LIMIT_RESET_CNT 20 > +#define LIMIT_TIMEOUT_CNT 3 > + > +/* It is possible that it is max clk rate for version 1.0 of bootcode */ > +#define BOOT_SPI_HZ 400000 > + > +static const u8 magnitude_table[] = {110, 85, 171, 71, 203, 195, 0, 67, 208, > + 56, 175, 244, 206, 213, 0, 92, 250, 0, 55, 48, 189, 252, 171, 243, 13, > + 45, 250}; > + > +static const struct ssp_sensorhub_info rinato_info = { > + .fw_name = "ssp_B2.fw", > + .fw_crashed_name = "ssp_crashed.fw", > + .fw_rev = 14052300, > + .mag_table = magnitude_table, > + .mag_length = ARRAY_SIZE(magnitude_table), > +}; > + > +static const struct ssp_sensorhub_info thermostat_info = { > + .fw_name = "thermostat_B2.fw", > + .fw_crashed_name = "ssp_crashed.fw", > + .fw_rev = 14080600, > + .mag_table = magnitude_table, > + .mag_length = ARRAY_SIZE(magnitude_table), > +}; > + > +static void start_reset_sequence(struct ssp_data *data) > +{ > + int i; > + > + dev_info(&data->spi->dev, "%s\n", __func__); > + > + gpio_set_value(data->mcu_reset, 0); > + usleep_range(4000, 4100); > + gpio_set_value(data->mcu_reset, 1); > + msleep(45); > + > + for (i = 0; i < 9; i++) { > + gpio_set_value(data->mcu_reset, 0); > + usleep_range(4000, 4100); > + gpio_set_value(data->mcu_reset, 1); > + usleep_range(15000, 15500); > + } > +} > + > +static void ssp_toggle_mcu_reset(struct ssp_data *data) > +{ > + gpio_set_value(data->mcu_reset, 0); > + usleep_range(1000, 1200); > + gpio_set_value(data->mcu_reset, 1); > + msleep(50); > +} > + > +static void sync_available_sensors(struct ssp_data *data) > +{ > + int i, ret; > + > + for (i = 0; i < SSP_SENSOR_MAX; ++i) { > + if ((data->available_sensors << i) == 1) You probably mean: if (data->available_sensors & BIT(i)) { > + ret = ssp_enable_sensor(data, i, data->delay_buf[i]); > + if (ret < 0) > + continue; So, continue on error, but continue loop anyway if everything is fine? Maybe better output an error message. } > + } > + > + ret = ssp_command(data, SSP_MSG2SSP_AP_MCU_SET_DUMPMODE, > + data->mcu_dump_mode); Double space and too much indentation on second line. > + if (ret < 0) { > + pr_err("[SSP]: %s - SSP_MSG2SSP_AP_MCU_SET_DUMPMODE failed\n", > + __func__); > + } > +} > + > +static void ssp_enable_mcu(struct ssp_data *data, bool enable) > +{ > + dev_info(&data->spi->dev, "Enable = %d, old enable = %d\n", enable, > + data->shut_down); This message is misleading, as data->shut_down doesn't mean the same as enable. > + > + if (enable && data->shut_down) { > + data->shut_down = false; > + enable_irq(data->spi->irq); > + enable_irq_wake(data->spi->irq); > + } else if (!enable && !data->shut_down) { > + data->shut_down = true; > + disable_irq(data->spi->irq); > + disable_irq_wake(data->spi->irq); > + } else { > + dev_err(&data->spi->dev, > + "Error / enable = %d, old enable = %d\n", enable, > + data->shut_down); Does this situation qualify as error? > + } > +} > + > +static int ssp_forced_to_download(struct ssp_data *data, int bin_type) Parameter bin_type unused. > +{ > + int ret = 0, retry = 3; ret doesn't need to be initialized. > + unsigned int speed; > + struct stm32fwu_fw *fwu; > + const struct firmware *fw = NULL; > + > + ssp_dbg("%s, mcu binany update!\n", __func__); > + > + ssp_enable_mcu(data, false); > + > + data->fw_dl_state = SSP_FW_DL_STATE_DOWNLOADING; > + dev_info(&data->spi->dev, "%s, DL state = %d\n", __func__, > + data->fw_dl_state); > + > + speed = data->spi->max_speed_hz; > + data->spi->max_speed_hz = BOOT_SPI_HZ; > + ret = spi_setup(data->spi); > + if (ret < 0) { > + dev_err(&data->spi->dev, "failed to setup spi for ssp_boot\n"); > + goto _err_begin; > + } > + > + dev_info(&data->spi->dev, "ssp_load_fw start\n"); > + > + ret = request_firmware(&fw, data->sensorhub_info->fw_name, > + &data->spi->dev); > + if (ret < 0) > + goto _err_begin; > + > + fwu = stm32fwu_init(&data->spi->dev, STM32_SPI_SAMSUNG, fw->data, > + fw->size); > + if (fwu == NULL) { > + dev_err(&data->spi->dev, "stm32fw init fail\n"); > + goto _err_fw; > + } > + > + start_reset_sequence(data); > + > + ret = stm32fwu_spi_send_sync(fwu); > + if (ret < 0) { > + dev_err(&data->spi->dev, "send sync failed with %d\n", ret); > + return ret; goto _err_fwu; instead? > + } > + > + do { > + dev_info(&data->spi->dev, "%d try\n", 3 - retry); > + ret = stm32fwu_update(fwu); > + } while (retry-- > 0 && ret < 0); What if update failed 3 times? > + > + data->spi->max_speed_hz = speed; > + ret = spi_setup(data->spi); > + if (ret < 0) { > + dev_err(&data->spi->dev, "failed to setup spi for ssp_norm\n"); > + goto _err_fwu; > + } > + > + if (ret < 0) > + goto _err_fwu; Should that be moved up after the update loop? > + > + data->fw_dl_state = SSP_FW_DL_STATE_SYNC; > + dev_info(&data->spi->dev, "%s, DL state = %d\n", __func__, > + data->fw_dl_state); > + > + ssp_enable_mcu(data, true); > + > + data->fw_dl_state = SSP_FW_DL_STATE_DONE; > + dev_info(&data->spi->dev, "%s, DL state = %d\n", __func__, > + data->fw_dl_state); 3 times the same message here. Maybe define a macro? > + > +_err_fwu: > + stm32fwu_destroy(fwu); > +_err_fw: > + release_firmware(fw); > +_err_begin: > + if (ret < 0) { > + data->fw_dl_state = SSP_FW_DL_STATE_FAIL; > + dev_err(&data->spi->dev, > + "%s - update_mcu_bin failed!\n", __func__); > + } > + > + return ret; > +} > + > +/* > + * This function is the first one which communicates with the mcu so it is > + * possible that the first attempt will fail > + */ > +static int ssp_check_fwbl(struct ssp_data *data) > +{ > + int retries = 0; > + > + while (retries++ < 5) { > + data->cur_firm_rev = ssp_get_firmware_rev(data); > + if (data->cur_firm_rev == SSP_INVALID_REVISION > + || data->cur_firm_rev == SSP_INVALID_REVISION2 > + || data->cur_firm_rev < 0) { data->cur_firm_rev is unsigned. > + dev_warn(&data->spi->dev, > + "Invalid revision, trying %d time\n", retries); > + } else > + break; > + } > + > + if (data->cur_firm_rev == SSP_INVALID_REVISION > + || data->cur_firm_rev == SSP_INVALID_REVISION2) { > + dev_err(&data->spi->dev, "SSP_INVALID_REVISION\n"); > + return SSP_FW_DL_STATE_NEED_TO_SCHEDULE; > + > + } else { No need for else. > + if (data->cur_firm_rev != data->sensorhub_info->fw_rev) { > + dev_info(&data->spi->dev, > + "MCU Firm Rev : Old = %8u, New = %8u\n", > + data->cur_firm_rev, > + data->sensorhub_info->fw_rev); > + > + return SSP_FW_DL_STATE_NEED_TO_SCHEDULE; > + } > + dev_info(&data->spi->dev, > + "MCU Firm Rev : Old = %8u, New = %8u\n", > + data->cur_firm_rev, > + data->sensorhub_info->fw_rev); Maybe restructure, to first do dev_info, and then compare firmware revision. > + } > + > + return SSP_FW_DL_STATE_NONE; > +} > + > +static void reset_mcu(struct ssp_data *data) > +{ > + ssp_enable_mcu(data, false); > + ssp_clean_pending_list(data); > + ssp_toggle_mcu_reset(data); > + ssp_enable_mcu(data, true); > +} > + > +static void wdt_work_func(struct work_struct *work) > +{ > + struct ssp_data *data = container_of(work, struct ssp_data, work_wdt); > + > + dev_err(&data->spi->dev, "%s - Sensor state: 0x%x, RC: %u, CC: %u\n", > + __func__, data->available_sensors, data->reset_cnt, > + data->com_fail_cnt); > + > + reset_mcu(data); > + data->com_fail_cnt = 0; > + data->time_out_cnt = 0; > +} > + > +static void wdt_timer_func(unsigned long ptr) > +{ > + struct ssp_data *data = (struct ssp_data *)ptr; > + > + switch (data->fw_dl_state) { > + case SSP_FW_DL_STATE_FAIL: > + case SSP_FW_DL_STATE_DOWNLOADING: > + case SSP_FW_DL_STATE_SYNC: > + goto _mod; > + } > + > + if (data->time_out_cnt > LIMIT_TIMEOUT_CNT > + || data->com_fail_cnt > LIMIT_RESET_CNT) > + queue_work(system_power_efficient_wq, &data->work_wdt); > +_mod: > + mod_timer(&data->wdt_timer, jiffies + msecs_to_jiffies(SSP_WDT_TIME)); > +} > + > +static void enable_wdt_timer(struct ssp_data *data) > +{ > + mod_timer(&data->wdt_timer, jiffies + msecs_to_jiffies(SSP_WDT_TIME)); > +} > + > +static void disable_wdt_timer(struct ssp_data *data) > +{ > + del_timer_sync(&data->wdt_timer); > + cancel_work_sync(&data->work_wdt); > +} > + > +/** > + * ssp_get_sensor_delay() - gets sensor data acquisition period > + * @data: sensorhub structure > + * @type: SSP sensor type > + * > + * Returns acquisition period in ms > + */ > +u32 ssp_get_sensor_delay(struct ssp_data *data, enum ssp_sensor_type type) > +{ > + return data->delay_buf[type]; > +} > +EXPORT_SYMBOL(ssp_get_sensor_delay); > + > +/** > + * ssp_enable_sensor() - enables data acquisition for sensor > + * @data: sensorhub structure > + * @type: SSP sensor type > + * @delay: delay in ms > + * > + * Returns 0 or negative value in case of error > + */ > +int ssp_enable_sensor(struct ssp_data *data, enum ssp_sensor_type type, > + u32 delay) > +{ > + int ret = 0; No need to initialize ret. > + struct { > + __le32 a; > + __le32 b; > + u8 c; > + } __attribute__((__packed__)) to_send; > + > + to_send.a = cpu_to_le32(delay); > + to_send.b = cpu_to_le32(data->batch_latency_buf[type]); > + to_send.c = data->batch_opt_buf[type]; > + > + switch (data->check_status[type]) { > + case SSP_INITIALIZATION_STATE: > + /* do calibration step */ Is this supposed to fall through? > + case SSP_ADD_SENSOR_STATE: > + ret = ssp_send_instruction(data, > + SSP_MSG2SSP_INST_BYPASS_SENSOR_ADD, > + type, > + (char *)&to_send, sizeof(to_send)); (u8 *)&to_send (if at all necessary)? > + if (ret < 0) { > + dev_err(&data->spi->dev, "Enabling sensor failed\n"); > + data->check_status[type] = SSP_NO_SENSOR_STATE; > + goto derror; > + } > + > + data->sensor_enable |= 1 << type; Could be done as: data->sensor_enable |= BIT(type); > + data->check_status[type] = SSP_RUNNING_SENSOR_STATE; > + break; > + case SSP_RUNNING_SENSOR_STATE: > + ret = ssp_send_instruction(data, > + SSP_MSG2SSP_INST_CHANGE_DELAY, type, > + (char *)&to_send, sizeof(to_send)); Same as above, and improve indentation. > + if (ret < 0) { > + dev_err(&data->spi->dev, > + "Changing delay sensor failed\n"); "Changing sensor delay failed\n" > + goto derror; > + } > + break; > + default: > + data->check_status[type] = SSP_ADD_SENSOR_STATE; > + break; > + } > + > + data->delay_buf[type] = delay; > + > + if (atomic_inc_return(&data->enable_refcount) == 1) > + enable_wdt_timer(data); > + > + return 0; > + > +derror: > + return -EIO; return ret; > +} > +EXPORT_SYMBOL(ssp_enable_sensor); > + > +/** > + * ssp_change_delay() - changes data acquisition for sensor > + * @data: sensorhub structure > + * @type: SSP sensor type > + * @delay: delay in ms > + * > + * Returns 0 or negative value in case of error > + */ > +int ssp_change_delay(struct ssp_data *data, enum ssp_sensor_type type, > + u32 delay) > +{ > + int ret = 0; No need to initialize ret. > + struct { > + __le32 a; > + __le32 b; > + u8 c; > + } __attribute__((__packed__)) to_send; > + > + to_send.a = cpu_to_le32(delay); > + to_send.b = cpu_to_le32(data->batch_latency_buf[type]); > + to_send.c = data->batch_opt_buf[type]; > + > + ret = ssp_send_instruction(data, SSP_MSG2SSP_INST_CHANGE_DELAY, type, > + (char *)&to_send, sizeof(to_send)); Improve indentation. > + if (ret < 0) { > + dev_err(&data->spi->dev, > + "Changing delay sensor failed\n"); > + return ret; > + } > + > + data->delay_buf[type] = delay; > + > + return ret; return 0; > +} > +EXPORT_SYMBOL(ssp_change_delay); > + > +/** > + * ssp_disable_sensor() - disables sensor > + * > + * @data: sensorhub structure > + * @type: SSP sensor type > + * > + * Returns 0 or negative value in case of error > + */ > +int ssp_disable_sensor(struct ssp_data *data, enum ssp_sensor_type type) > +{ > + int ret = 0; > + __le32 command; > + > + if (data->sensor_enable & (1 << type)) { (1 << type) could be written as BIT(type). > + command = cpu_to_le32(data->delay_buf[type]); Double whitespace. > + ret = ssp_send_instruction(data, > + SSP_MSG2SSP_INST_BYPASS_SENSOR_REMOVE, > + type, (char *)&command, sizeof(__le32)); sizeof(command)? Also improve intendation. > + if (ret < 0) { > + dev_err(&data->spi->dev, "Remove sensor fail\n"); > + return ret; > + } > + > + data->sensor_enable &= (~(1 << type)); > + } > + > + data->check_status[type] = SSP_ADD_SENSOR_STATE; > + > + if (atomic_dec_and_test(&data->enable_refcount)) > + disable_wdt_timer(data); > + > + return ret; return 0; > +} > +EXPORT_SYMBOL(ssp_disable_sensor); > + > +static irqreturn_t sensordata_irq_thread_fn(int irq, void *dev_id) > +{ > + struct ssp_data *data = dev_id; > + > + ssp_irq_msg(data); > + > + return IRQ_HANDLED; > +} > + > +static int ssp_initialize_mcu(struct ssp_data *data) > +{ > + int ret; > + > + ssp_clean_pending_list(data); > + > + ret = ssp_get_chipid(data); > + if (ret != SSP_DEVICE_ID) { > + dev_err(&data->spi->dev, "%s - MCU %s ret = %d\n", __func__, > + ret < 0 ? "is not working" : "identyfication failed", Typo: identification. Also improve indentation. > + ret); > + return ret >= 0 ? -ENODEV : ret; Turning it around to return ret < 0 ? ret : -ENODEV; makes it easier comparable to the message above. > + } > + > + dev_info(&data->spi->dev, "MCU device ID = %d, reading ID = %d\n", > + SSP_DEVICE_ID, ret); Whats the sense of a comparing message, since at this point ret == SSP_DEVICE_ID. > + > + ret = ssp_set_sensor_position(data); > + if (ret < 0) { > + dev_err(&data->spi->dev, "ssp_set_sensor_position failed\n"); > + return ret; > + } > + > + ret = ssp_set_magnetic_matrix(data); > + if (ret < 0) > + dev_err(&data->spi->dev, > + "%s - ssp_set_magnetic_matrix failed\n", __func__); Maybe return in case of error? > + > + data->available_sensors = ssp_get_sensor_scanning_info(data); > + if (data->available_sensors == 0) { > + dev_err(&data->spi->dev, > + "%s - ssp_get_sensor_scanning_info failed\n", __func__); > + return -EIO; > + } > + > + data->cur_firm_rev = ssp_get_firmware_rev(data); > + dev_info(&data->spi->dev, "MCU Firm Rev : New = %8u\n", > + data->cur_firm_rev); > + > + return ssp_command(data, SSP_MSG2SSP_AP_MCU_DUMP_CHECK, 0); > +} > + > +static void ssp_refresh_task(struct work_struct *work) > +{ > + struct ssp_data *data = container_of((struct delayed_work *)work, > + struct ssp_data, work_refresh); Improve indentation. > + > + dev_info(&data->spi->dev, "refreshing\n"); > + > + data->reset_cnt++; > + > + if (ssp_initialize_mcu(data) > 0) { Check for >= 0. > + sync_available_sensors(data); > + if (data->last_ap_state != 0) > + ssp_command(data, data->last_ap_state, 0); > + > + if (data->last_resume_state != 0) > + ssp_command(data, data->last_resume_state, 0); > + > + data->time_out_cnt = 0; > + data->com_fail_cnt = 0; > + } > +} > + > +int ssp_queue_ssp_refresh_task(struct ssp_data *data, int delay) Unsigned delay? > +{ > + cancel_delayed_work_sync(&data->work_refresh); > + > + return queue_delayed_work(system_power_efficient_wq, > + &data->work_refresh, > + msecs_to_jiffies(delay)); > +} > + > +static void work_function_firmware_update(struct work_struct *work) > +{ > + int ret; > + struct ssp_data *data = container_of((struct delayed_work *)work, > + struct ssp_data, work_firmware); Improve indentation. > + > + dev_info(&data->spi->dev, "%s, is called\n", __func__); > + > + ret = ssp_forced_to_download(data, SSP_KERNEL_BINARY); > + if (ret < 0) { > + dev_err(&data->spi->dev, > + "%s, ssp_forced_to_download failed!\n", Improve indentation. > + __func__); > + return; > + } > + > + ssp_queue_ssp_refresh_task(data, SSP_SW_RESET_TIME); > + > + dev_info(&data->spi->dev, "%s done\n", __func__); > +} > + > +#ifdef CONFIG_OF > +static struct of_device_id ssp_of_match[] = { > + { > + .compatible = "samsung,sensorhub-rinato", > + .data = &rinato_info, > + }, { > + .compatible = "samsung,sensorhub-thermostat", > + .data = &thermostat_info, > + }, > +}; > +MODULE_DEVICE_TABLE(of, ssp_of_match); > + > +static struct ssp_data *ssp_parse_dt(struct device *dev) > +{ > + int ret; > + struct ssp_data *pdata; Name it data, like in all other cases before? > + struct device_node *node = dev->of_node; > + const struct of_device_id *match; > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return NULL; > + > + pdata->mcu_ap_gpio = of_get_named_gpio(node, "mcu-ap-gpio", 0); > + if (pdata->mcu_ap_gpio < 0) > + goto err_free_pd; > + > + pdata->ap_mcu_gpio = of_get_named_gpio(node, "ap-mcu-gpio", 0); > + if (pdata->ap_mcu_gpio < 0) > + goto err_free_pd; > + > + pdata->mcu_reset = of_get_named_gpio(node, "mcu-reset", 0); > + if (pdata->mcu_reset < 0) > + goto err_free_pd; > + > + ret = devm_gpio_request_one(dev, pdata->ap_mcu_gpio, > + GPIOF_OUT_INIT_HIGH, "ap-mcu-gpio"); > + if (ret) > + goto err_free_pd; > + > + ret = devm_gpio_request_one(dev, pdata->mcu_reset, > + GPIOF_OUT_INIT_HIGH, "mcu-reset"); > + if (ret) > + goto err_ap_mcu; > + > + match = of_match_node(ssp_of_match, node); > + if (!match) > + goto err_mcu_reset; > + > + pdata->sensorhub_info = (struct ssp_sensorhub_info *) match->data; > + > + ret = of_platform_populate(node, NULL, NULL, dev); > + if (ret < 0) { > + dev_err(dev, "of_platform_populate fail\n"); > + goto err_mcu_reset; > + } > + > + return pdata; > + > +err_mcu_reset: > + devm_gpio_free(dev, pdata->mcu_reset); > +err_ap_mcu: > + devm_gpio_free(dev, pdata->ap_mcu_gpio); > +err_free_pd: > + devm_kfree(dev, pdata); > + return NULL; > +} > +#else > +static struct ssp_data *ssp_parse_dt(struct platform_device *pdev) > +{ > + return NULL; > +} > +#endif > + > +/** > + * ssp_register_consumer() - registers iio consumer in ssp framework > + * > + * @indio_dev: consumer iio device > + * @type: ssp sensor type > + */ > +void ssp_register_consumer(struct iio_dev *indio_dev, enum ssp_sensor_type type) > +{ > + /*TODO 3rd level device - hide it*/ Missing whitespace at beginning and end of comment. > + struct ssp_data *data = dev_get_drvdata(indio_dev->dev.parent->parent); > + > + data->sensor_devs[type] = indio_dev; > +} > +EXPORT_SYMBOL(ssp_register_consumer); > + > +static int ssp_probe(struct spi_device *spi) > +{ > + int ret, i; > + struct ssp_data *data; > + > + data = spi->dev.platform_data; > + if (!data) { > + data = ssp_parse_dt(&spi->dev); > + if (!data) { > + dev_err(&spi->dev, > + "%s:Failed to find platform data\n", __func__); > + return -ENODEV; > + } > + } > + > + spi->mode = SPI_MODE_1; > + ret = spi_setup(spi); > + if (ret < 0) { > + dev_err(&spi->dev, "Failed to setup spi\n"); > + goto err_setup; Add __func__ to error message and return ret here? > + } > + > + data->fw_dl_state = SSP_FW_DL_STATE_NONE; > + data->spi = spi; > + spi_set_drvdata(spi, data); > + > + mutex_init(&data->comm_lock); > + mutex_init(&data->pending_lock); > + > + for (i = 0; i < SSP_SENSOR_MAX; ++i) { > + data->delay_buf[i] = SSP_DEFUALT_POLLING_DELAY; > + data->batch_latency_buf[i] = 0; > + data->batch_opt_buf[i] = 0; > + data->check_status[i] = SSP_INITIALIZATION_STATE; > + } > + > + data->delay_buf[SSP_BIO_HRM_LIB] = 100; > + > + data->time_syncing = true; > + > + INIT_LIST_HEAD(&data->pending_list); > + > + atomic_set(&data->enable_refcount, 0); > + > + INIT_DELAYED_WORK(&data->work_firmware, work_function_firmware_update); > + INIT_WORK(&data->work_wdt, wdt_work_func); > + INIT_DELAYED_WORK(&data->work_refresh, ssp_refresh_task); > + > + setup_timer(&data->wdt_timer, wdt_timer_func, (unsigned long)data); > + > + ret = request_threaded_irq(data->spi->irq, NULL, > + sensordata_irq_thread_fn, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "SSP_Int", data); > + if (ret < 0) { > + dev_err(&spi->dev, "Irq request fail\n"); > + goto err_setup_irq; > + } > + > + /* Let's start with enabled one so irq balance could be ok */ > + data->shut_down = false; > + > + /* just to avoid unbalanced irq set wake up */ > + enable_irq_wake(data->spi->irq); > + > + data->fw_dl_state = ssp_check_fwbl(data); > + if (data->fw_dl_state == SSP_FW_DL_STATE_NONE) { > + ret = ssp_initialize_mcu(data); > + if (ret < 0) { > + dev_err(&spi->dev, "Initialize_mcu failed\n"); > + goto err_read_reg; > + } > + } > + > + if (data->fw_dl_state == SSP_FW_DL_STATE_NEED_TO_SCHEDULE) { > + dev_info(&spi->dev, "Firmware update is scheduled\n"); > + queue_delayed_work(system_power_efficient_wq, > + &data->work_firmware, msecs_to_jiffies(1000)); Improve indentation. > + data->fw_dl_state = SSP_FW_DL_STATE_SCHEDULED; > + } else if (data->fw_dl_state == SSP_FW_DL_STATE_FAIL) { > + data->shut_down = true; Is this an error condition? > + } > + Could work with switch (data->fw_dl_state). > + return 0; > + > +err_read_reg: > + free_irq(data->spi->irq, data); > +err_setup_irq: > + mutex_destroy(&data->pending_lock); > + mutex_destroy(&data->comm_lock); > +err_setup: > + dev_err(&spi->dev, "Probe failed!\n"); > + > + return ret; > +} > + > +static int ssp_remove(struct spi_device *spi) > +{ > + struct ssp_data *data = spi_get_drvdata(spi); > + > + if (data->fw_dl_state >= SSP_FW_DL_STATE_SCHEDULED && > + data->fw_dl_state < SSP_FW_DL_STATE_DONE) { Improve indentation. > + dev_err(&data->spi->dev, > + "cancel_delayed_work_sync state = %d\n", > + data->fw_dl_state); > + cancel_delayed_work_sync(&data->work_firmware); > + } > + > + if (ssp_command(data, SSP_MSG2SSP_AP_STATUS_SHUTDOWN, 0) < 0) > + dev_err(&data->spi->dev, "SSP_MSG2SSP_AP_STATUS_SHUTDOWN failed\n"); Line too long. > + > + ssp_enable_mcu(data, false); > + disable_wdt_timer(data); > + > + ssp_clean_pending_list(data); > + > + free_irq(data->spi->irq, data); > + > + del_timer_sync(&data->wdt_timer); > + cancel_work_sync(&data->work_wdt); > + > + mutex_destroy(&data->comm_lock); > + mutex_destroy(&data->pending_lock); > + > +#ifdef CONFIG_OF > + of_platform_depopulate(&spi->dev); > +#endif > + > + return 0; > +} > + > +static int ssp_suspend(struct device *dev) > +{ > + int ret = 0; > + struct ssp_data *data = spi_get_drvdata(to_spi_device(dev)); > + > + data->last_resume_state = SSP_MSG2SSP_AP_STATUS_SUSPEND; > + > + if (atomic_read(&data->enable_refcount) > 0) > + disable_wdt_timer(data); > + > + if (ssp_command(data, SSP_MSG2SSP_AP_STATUS_SUSPEND, 0) < 0) > + dev_err(&data->spi->dev, > + "%s SSP_MSG2SSP_AP_STATUS_SUSPEND failed\n", __func__); > + > + data->time_syncing = false; > + disable_irq(data->spi->irq); > + > + return ret; ret is kind of unused, so it should be used or dropped. return 0 directly here to indicate success. > +} > + > +static int ssp_resume(struct device *dev) > +{ > + int ret = 0; > + struct ssp_data *data = spi_get_drvdata(to_spi_device(dev)); > + > + enable_irq(data->spi->irq); > + Re-enable data->time_syncing? > + if (atomic_read(&data->enable_refcount) > 0) > + enable_wdt_timer(data); > + > + if (ssp_command(data, SSP_MSG2SSP_AP_STATUS_RESUME, 0) < 0) > + dev_err(&data->spi->dev, > + "%s SSP_MSG2SSP_AP_STATUS_RESUME failed\n", __func__); > + > + data->last_resume_state = SSP_MSG2SSP_AP_STATUS_RESUME; > + > + return ret; ret is kind of unused, so it should be used or dropped. return 0 directly here to indicate success. > +} > + > +static const struct dev_pm_ops ssp_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(ssp_suspend, ssp_resume) > +}; > + > +static struct spi_driver ssp_driver = { > + .probe = ssp_probe, > + .remove = ssp_remove, > + .driver = { > + .pm = &ssp_pm_ops, > + .bus = &spi_bus_type, > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ssp_of_match), > + .name = "sensorhub" > + }, > +}; > + > +module_spi_driver(ssp_driver); > + > +MODULE_DESCRIPTION("ssp sensorhub driver"); > +MODULE_AUTHOR("Samsung Electronics"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/misc/sensorhub/ssp_spi.c b/drivers/misc/sensorhub/ssp_spi.c > new file mode 100644 > index 0000000..c599e35 > --- /dev/null > +++ b/drivers/misc/sensorhub/ssp_spi.c > @@ -0,0 +1,653 @@ > +/* > + * Copyright (C) 2012, Samsung Electronics Co. Ltd. All Rights Reserved. 2014? > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that 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 "ssp.h" > + > +#define SSP_DEV (&data->spi->dev) > +#define GET_MESSAGE_TYPE(data) (data & (3 << SSP_RW)) > + > +/* > + * SSP -> AP Instruction > + * They tell what packet type can be expected. In the future there will > + * be less of them. BYPASS means common sensor packets with accel, gyro, > + * hrm etc. data. LIBRARY and META are mock-up's for now. > + */ > +#define MSG2AP_INST_BYPASS_DATA 0x37 > +#define MSG2AP_INST_LIBRARY_DATA 0x01 > +#define MSG2AP_INST_DEBUG_DATA 0x03 > +#define MSG2AP_INST_BIG_DATA 0x04 > +#define MSG2AP_INST_META_DATA 0x05 > +#define MSG2AP_INST_TIME_SYNC 0x06 > +#define MSG2AP_INST_RESET 0x07 > + > +#define UNINPLEMENTED -1 > + > +struct ssp_msg_header { > + u8 cmd; > + __le16 length; > + __le16 options; > + __le32 data; > +} __attribute__((__packed__)); > + > +struct ssp_msg { > + u16 length; > + u16 options; > + struct list_head list; > + struct completion *done; > + struct ssp_msg_header *h; > + char *buffer; > +}; > + > +static const int offset_map[SSP_SENSOR_MAX] = { > + [SSP_ACCELEROMETER_SENSOR] = SSP_ACCELEROMETER_SIZE + > + SSP_TIME_SIZE, Improve indentation. > + [SSP_GYROSCOPE_SENSOR] = SSP_GYROSCOPE_SIZE + > + SSP_TIME_SIZE, Improve indentation. > + [SSP_GEOMAGNETIC_UNCALIB_SENSOR] = UNINPLEMENTED, > + [SSP_GEOMAGNETIC_RAW] = UNINPLEMENTED, > + [SSP_GEOMAGNETIC_SENSOR] = UNINPLEMENTED, > + [SSP_PRESSURE_SENSOR] = UNINPLEMENTED, > + [SSP_GESTURE_SENSOR] = UNINPLEMENTED, > + [SSP_PROXIMITY_SENSOR] = UNINPLEMENTED, > + [SSP_TEMPERATURE_HUMIDITY_SENSOR] = UNINPLEMENTED, > + [SSP_LIGHT_SENSOR] = UNINPLEMENTED, > + [SSP_PROXIMITY_RAW] = UNINPLEMENTED, > + [SSP_ORIENTATION_SENSOR] = UNINPLEMENTED, > + [SSP_STEP_DETECTOR] = UNINPLEMENTED, > + [SSP_SIG_MOTION_SENSOR] = UNINPLEMENTED, > + [SSP_GYRO_UNCALIB_SENSOR] = UNINPLEMENTED, > + [SSP_GAME_ROTATION_VECTOR] = UNINPLEMENTED, > + [SSP_ROTATION_VECTOR] = UNINPLEMENTED, > + [SSP_STEP_COUNTER] = UNINPLEMENTED , > + [SSP_BIO_HRM_RAW] = SSP_BIO_HRM_RAW_SIZE + > + SSP_TIME_SIZE, > + [SSP_BIO_HRM_RAW_FAC] = SSP_BIO_HRM_RAW_FAC_SIZE + > + SSP_TIME_SIZE, > + [SSP_BIO_HRM_LIB] = SSP_BIO_HRM_LIB_SIZE + > + SSP_TIME_SIZE, > +}; > + > +#define SSP_HEADER_SIZE (sizeof(struct ssp_msg_header)) > +#define SSP_HEADER_SIZE_ALIGNED (ALIGN(SSP_HEADER_SIZE, 4)) > + > +static struct ssp_msg *create_msg(u8 cmd, u16 len, u16 opt, u32 data) > +{ > + struct ssp_msg_header h; > + struct ssp_msg *msg; > + > + msg = kzalloc(sizeof(*msg), GFP_KERNEL); > + if (!msg) > + return NULL; > + > + h.cmd = cmd; > + h.length = cpu_to_le16(len); > + h.options = cpu_to_le16(opt); > + h.data = cpu_to_le32(data); > + > + msg->buffer = kzalloc(SSP_HEADER_SIZE_ALIGNED + len, > + GFP_KERNEL | GFP_DMA); > + if (msg->buffer == NULL) { > + kfree(msg); > + return NULL; > + } > + > + msg->length = len; > + msg->options = opt; > + > + memcpy(msg->buffer, &h, SSP_HEADER_SIZE); > + > + return msg; > +} > + > +static inline void fill_buffer(struct ssp_msg *m, unsigned int offset, > + const void *src, unsigned int len) > +{ > + memcpy(&m->buffer[SSP_HEADER_SIZE_ALIGNED + offset], src, len); > +} > + > +static inline void get_buffer(struct ssp_msg *m, unsigned int offset, > + void *dest, unsigned int len) > +{ > + memcpy(dest, &m->buffer[SSP_HEADER_SIZE_ALIGNED + offset], len); > +} > + > +#define GET_BUFFER_AT_INDEX(m, index) \ > + (m->buffer[SSP_HEADER_SIZE_ALIGNED + index]) > +#define SET_BUFFER_AT_INDEX(m, index, val) \ > + (m->buffer[SSP_HEADER_SIZE_ALIGNED + index] = val) > + > +static void clean_msg(struct ssp_msg *m) > +{ > + kfree(m->buffer); > + kfree(m); > +} > + > +static int print_mcu_debug(char *data_frame, int *data_index, > + int received_len) > +{ > + int length = data_frame[(*data_index)++]; > + > + if (length > received_len - *data_index || length <= 0) { > + ssp_dbg("[SSP]: MSG From MCU-invalid debug length(%d/%d)\n", > + length, received_len); > + return length ? length : -1; Return a valid errorcode? > + } > + > + ssp_dbg("[SSP]: MSG From MCU - %s\n", &data_frame[*data_index]); > + > + *data_index += length; > + > + return 0; > +} > + > +/* > + * It was designed that way - additional lines to some kind of handshake, > + * please do not ask why - only the firmware guy can know it > + */ > +static int check_lines(struct ssp_data *data, bool state) > +{ > + int delay_cnt = 0, status = 0; Get rid of status. > + > + gpio_set_value_cansleep(data->ap_mcu_gpio, state); > + > + while (gpio_get_value_cansleep(data->mcu_ap_gpio) != state) { > + > + usleep_range(3000, 3500); > + > + if (data->shut_down || delay_cnt++ > 500) { > + dev_err(SSP_DEV, "%s:timeout, hw ack wait fail %d\n", > + __func__, state); > + > + if (!state) { > + gpio_set_value_cansleep(data->ap_mcu_gpio, 1); > + status = -1; Return directly here (preferably a valid error code) > + } else > + status = -2; Return directly here (also preferably an error code). Or just make the gpio-setting conditional, and return -ETIMEDOUT unconditionally. The exact return value isn't checked, anyway, and could then just be passed up in do_transfer(). > + > + break; No need for break then > + } > + } > + > + return status; return 0; > +} > + > +static int do_transfer(struct ssp_data *data, struct ssp_msg *msg, > + struct completion *done, int timeout) > +{ > + int status = 0; No need to initialize status > + /* > + * check if this is a short one way message or the whole transfer has > + * second part after an interrupt > + */ > + const bool use_no_irq = msg->length == 0; > + > + if (data->shut_down) > + return -EPERM; > + > + msg->done = done; > + > + mutex_lock(&data->comm_lock); > + > + status = check_lines(data, false); > + if (status < 0) { > + status = -ETIMEDOUT; Would be obsolete if check_lines() would return this code on error. > + goto _error_locked; > + } > + > + status = spi_write(data->spi, msg->buffer, SSP_HEADER_SIZE); > + if (status < 0) { > + gpio_set_value_cansleep(data->ap_mcu_gpio, 1); > + dev_err(SSP_DEV, "%s spi_write fail\n", __func__); > + goto _error_locked; > + } > + > + if (!use_no_irq) { > + mutex_lock(&data->pending_lock); > + list_add_tail(&msg->list, &data->pending_list); > + mutex_unlock(&data->pending_lock); > + } > + > + status = check_lines(data, true); > + if (status < 0) { > + status = -ETIMEDOUT; Same here. > + if (!use_no_irq) { > + mutex_lock(&data->pending_lock); > + list_del(&msg->list); > + mutex_unlock(&data->pending_lock); > + } > + goto _error_locked; > + } > + > + mutex_unlock(&data->comm_lock); > + > + if (use_no_irq) > + return status; Drop this test. > + > + if (done != NULL) if(!use_no_irq && done != NULL) > + if (wait_for_completion_timeout(done, > + msecs_to_jiffies(timeout)) == 0) { Could be following indentation rules, if formated this way: if (!wait_for_completion_timeout(done, msecs_to_jiffies(timeout)) { > + mutex_lock(&data->pending_lock); > + list_del(&msg->list); > + mutex_unlock(&data->pending_lock); > + > + status = -ETIMEDOUT; > + data->time_out_cnt++; Don't populate status, instead return -ETIMEDOUT directly here. > + } > + > + return status; return 0 is more obvious. > + > +_error_locked: > + mutex_unlock(&data->comm_lock); > + data->time_out_cnt++; > + return status; > +} > + > +static inline int ssp_spi_sync_command(struct ssp_data *data, > + struct ssp_msg *msg) > +{ > + return do_transfer(data, msg, NULL, 0); > +} > + > +static int ssp_spi_sync(struct ssp_data *data, struct ssp_msg *msg, > + int timeout) > +{ > + DECLARE_COMPLETION_ONSTACK(done); > + > + if (WARN_ON(!msg->length)) > + return -EPERM; > + > + return do_transfer(data, msg, &done, timeout); > +} > + > +static int handle_big_data(struct ssp_data *data, char *dataframe, > + int *idx) { Parameters fit in one line. > + /* mock-up, it will be changed with adding another sensor types */ > + *idx += 8; > + return 0; > +} > + > +static int parse_dataframe(struct ssp_data *data, char *dataframe, > + int len) Parameters fit in one line. > +{ > + int idx, sd, ret = 0; ret is unnecessary. > + u16 length = 0; length is just a placebo. > + struct timespec ts; > + struct ssp_sensor_data *sdata; > + struct iio_dev **indio_devs = data->sensor_devs; > + > + getnstimeofday(&ts); > + > + for (idx = 0; idx < len;) { > + switch (dataframe[idx++]) { > + case MSG2AP_INST_BYPASS_DATA: > + sd = dataframe[idx++]; > + if (sd < 0 || sd >= SSP_SENSOR_MAX) { > + dev_err(SSP_DEV, > + "Mcu data frame1 error %d\n", sd); > + return -EPROTO; > + } > + > + if (indio_devs[sd] != NULL) { > + sdata = iio_priv(indio_devs[sd]); > + if (sdata->process_data) > + sdata->process_data( Start parameters in first line. > + indio_devs[sd], > + &dataframe[idx], > + data->timestamp); > + } else > + dev_err(SSP_DEV, "no client for frame\n"); > + > + idx += offset_map[sd]; > + break; > + case MSG2AP_INST_DEBUG_DATA: > + sd = print_mcu_debug(dataframe, &idx, len); > + if (sd) { > + dev_err(SSP_DEV, > + "Mcu data frame3 error %d\n", sd); > + return -EPROTO; return sd? > + } > + break; > + case MSG2AP_INST_LIBRARY_DATA: > + idx += length; Placebo? > + break; > + case MSG2AP_INST_BIG_DATA: > + handle_big_data(data, dataframe, &idx); > + break; > + case MSG2AP_INST_TIME_SYNC: > + data->time_syncing = true; > + break; > + case MSG2AP_INST_RESET: > + ssp_queue_ssp_refresh_task(data, 0); > + break; > + } > + } > + > + if (data->time_syncing) > + data->timestamp = ts.tv_sec * 1000000000ULL + ts.tv_nsec; > + > + return ret; return 0 directly. > +} > + > +/* threaded irq */ > +int ssp_irq_msg(struct ssp_data *data) > +{ > + bool found = false; > + char *buffer; > + __le16 *temp_buf; > + u8 msg_type = 0; No need to initialize msg_type. > + int ret = 0; No need to initialize ret. > + u16 length, msg_options; > + struct ssp_msg *msg, *n; > + > + temp_buf = kmalloc(4, GFP_KERNEL | GFP_DMA); > + if (temp_buf == NULL) > + return -ENOMEM; > + > + ret = spi_read(data->spi, temp_buf, 4); > + if (ret < 0) { > + dev_err(SSP_DEV, "header read fail\n"); > + kfree(temp_buf); > + return ret; > + } > + > + length = le16_to_cpu(temp_buf[1]); > + msg_options = le16_to_cpu(temp_buf[0]); > + > + kfree(temp_buf); > + > + if (length == 0) { > + dev_err(SSP_DEV, "length received from mcu is 0\n"); > + return -EINVAL; > + } > + > + msg_type = GET_MESSAGE_TYPE(msg_options); > + > + switch (msg_type) { > + case SSP_AP2HUB_READ: > + case SSP_AP2HUB_WRITE: > + /* > + * this is a small list, a few elements - the packets can be > + * received with no order > + */ > + mutex_lock(&data->pending_lock); > + list_for_each_entry_safe(msg, n, &data->pending_list, list) { > + if (msg->options == msg_options) { > + list_del(&msg->list); > + found = true; > + break; > + } > + } > + > + if (!found) { > + /* > + * here can be implemented dead messages handling > + * but the slave should not send such ones - it is to > + * check but let's handle this > + */ > + buffer = kmalloc(length, GFP_KERNEL | GFP_DMA); > + if (!buffer) { > + ret = -ENOMEM; > + goto _unlock; > + } > + > + ret = spi_read(data->spi, buffer, length); Check ret for errors? > + dev_err(SSP_DEV, "No match error %x\n", > + msg_options); > + ret = -EIO; > + /* got dead packet so it is always an error */ > + kfree(buffer); > + goto _unlock; > + } > + > + if (msg_type == SSP_AP2HUB_READ) > + ret = spi_read(data->spi, > + &msg->buffer[SSP_HEADER_SIZE_ALIGNED], > + msg->length); > + > + if (msg_type == SSP_AP2HUB_WRITE) { > + ret = spi_write(data->spi, > + &msg->buffer[SSP_HEADER_SIZE_ALIGNED], > + msg->length); > + if (msg_options & SSP_AP2HUB_RETURN) { > + msg->options = > + SSP_AP2HUB_READ | SSP_AP2HUB_RETURN; > + msg->length = 1; > + > + list_add_tail(&msg->list, > + &data->pending_list); These parameters fit into the first line. > + goto _unlock; > + } > + } > + > + if (msg->done) > + if (!completion_done(msg->done)) > + complete(msg->done); > +_unlock: > + mutex_unlock(&data->pending_lock); > + break; > + case SSP_HUB2AP_WRITE: > + buffer = kzalloc(length, GFP_KERNEL | GFP_DMA); > + if (buffer == NULL) > + return -ENOMEM; Double whitespace. > + > + ret = spi_read(data->spi, buffer, length); > + if (ret < 0) { > + dev_err(SSP_DEV, "spi read fail\n"); > + kfree(buffer); > + break; > + } > + > + parse_dataframe(data, buffer, length); Assign the result to ret? > + > + kfree(buffer); > + break; > + > + default: > + dev_err(SSP_DEV, "unknown msg type\n"); Doesn't this qualify as error? > + break; > + } > + > + return ret; > +} > + > +void ssp_clean_pending_list(struct ssp_data *data) > +{ > + struct ssp_msg *msg, *n; > + > + mutex_lock(&data->pending_lock); > + list_for_each_entry_safe(msg, n, &data->pending_list, list) { > + list_del(&msg->list); > + > + if (msg->done) > + if (!completion_done(msg->done)) > + complete(msg->done); > + } > + mutex_unlock(&data->pending_lock); > +} > + > +int ssp_command(struct ssp_data *data, char command, int arg) > +{ > + int ret; > + struct ssp_msg *msg; > + > + msg = create_msg(command, 0, SSP_AP2HUB_WRITE, arg); > + if (!msg) > + return -ENOMEM; > + > + ssp_dbg("%s - command 0x%x %d\n", __func__, command, arg); > + > + ret = ssp_spi_sync_command(data, msg); > + clean_msg(msg); > + > + return ret; > +} > + > +int ssp_send_instruction(struct ssp_data *data, u8 inst, u8 sensor_type, > + u8 *send_buf, u8 length) > +{ > + int ret; > + struct ssp_msg *msg; > + > + if (data->fw_dl_state == SSP_FW_DL_STATE_DOWNLOADING) { > + dev_err(SSP_DEV, "%s - Skip Inst! DL state = %d\n", > + __func__, data->fw_dl_state); > + return -EBUSY; > + } else if ((!(data->available_sensors & (1 << sensor_type))) One pair of parenthesis can be dropped. Could use BIT(sensor_type). > + && (inst <= SSP_MSG2SSP_INST_CHANGE_DELAY)) { > + dev_err(SSP_DEV, "%s - Bypass Inst Skip! - %u\n", > + __func__, sensor_type); > + return -EIO; /* just fail */ > + } > + > + msg = create_msg(inst, length + 2, SSP_AP2HUB_WRITE, 0); > + if (!msg) > + return -ENOMEM; > + > + fill_buffer(msg, 0, &sensor_type, 1); > + fill_buffer(msg, 1, send_buf, length); > + > + ssp_dbg("%s - Inst = 0x%x, Sensor Type = 0x%x, data = %u\n", > + __func__, inst, sensor_type, send_buf[1]); > + > + ret = ssp_spi_sync(data, msg, 1000); > + clean_msg(msg); > + > + return ret; > +} > + > +int ssp_get_chipid(struct ssp_data *data) > +{ > + int ret; > + char buffer = 0; No need to initialize buffer. > + struct ssp_msg *msg; > + > + msg = create_msg(SSP_MSG2SSP_AP_WHOAMI, 1, SSP_AP2HUB_READ, 0); > + if (!msg) > + return -ENOMEM; > + > + Just one empty line. > + ret = ssp_spi_sync(data, msg, 1000); > + > + buffer = GET_BUFFER_AT_INDEX(msg, 0); > + > + clean_msg(msg); > + > + return ret < 0 ? ret : buffer; > +} > + > +int ssp_set_sensor_position(struct ssp_data *data) > +{ > + int ret = 0; No need to initialize ret. > + > + struct ssp_msg *msg = create_msg(SSP_MSG2SSP_AP_SENSOR_FORMATION, 3, > + SSP_AP2HUB_WRITE, 0); > + if (!msg) > + return -ENOMEM; > + > + /* > + * this needs some calrification with the protocol, default they will Typo: clarification > + * be 0 so it is ok > + */ > + SET_BUFFER_AT_INDEX(msg, 0, data->accel_position); > + SET_BUFFER_AT_INDEX(msg, 1, data->accel_position); > + SET_BUFFER_AT_INDEX(msg, 2, data->mag_position); > + > + ret = ssp_spi_sync(data, msg, 1000); > + if (ret < 0) { > + dev_err(SSP_DEV, "%s -fail to ssp_set_sensor_position %d\n", Missing whitespace before fail. > + __func__, ret); > + } else { > + dev_info(SSP_DEV, > + "Sensor Posision A : %u, G : %u, M: %u, P: %u\n", Typo: Position > + data->accel_position, data->accel_position, > + data->mag_position, 0); > + } > + > + clean_msg(msg); > + > + return ret; > +} > + > +int ssp_set_magnetic_matrix(struct ssp_data *data) > +{ > + int ret; > + struct ssp_msg *msg = > + create_msg(SSP_MSG2SSP_AP_SET_MAGNETIC_STATIC_MATRIX, > + data->sensorhub_info->mag_length, > + SSP_AP2HUB_WRITE, > + 0); Initialize msg on a separate line, that will give proper indentation. > + if (!msg) > + return -ENOMEM; > + > + fill_buffer(msg, 0, data->sensorhub_info->mag_table, > + data->sensorhub_info->mag_length); > + > + ret = ssp_spi_sync(data, msg, 1000); > + clean_msg(msg); > + > + return ret; > +} > + > +unsigned int ssp_get_sensor_scanning_info(struct ssp_data *data) > +{ > + int ret; > + __le32 result; > + u32 cpu_result = 0; > + > + struct ssp_msg *msg = create_msg(SSP_MSG2SSP_AP_SENSOR_SCANNING, 4, > + SSP_AP2HUB_READ, 0); > + if (!msg) > + return 0; > + > + ret = ssp_spi_sync(data, msg, 1000); > + if (ret < 0) { > + dev_err(SSP_DEV, "%s - spi read fail %d\n", __func__, ret); > + goto _exit; > + } > + > + get_buffer(msg, 0, &result, 4); > + cpu_result = le32_to_cpu(result); > + > + dev_info(SSP_DEV, "%s state: 0x%08x\n", __func__, cpu_result); > + > +_exit: > + clean_msg(msg); > + return cpu_result; > +} > + > +unsigned int ssp_get_firmware_rev(struct ssp_data *data) > +{ > + int ret; > + __le32 result; > + > + struct ssp_msg *msg = create_msg(SSP_MSG2SSP_AP_FIRMWARE_REV, 4, > + SSP_AP2HUB_READ, 0); > + if (!msg) > + return SSP_INVALID_REVISION; Double whitespace. > + > + ret = ssp_spi_sync(data, msg, 1000); > + get_buffer(msg, 0, &result, 4); > + if (ret < 0) { Double whitespace. > + dev_err(SSP_DEV, "%s - transfer fail %d\n", __func__, ret); > + ret = SSP_INVALID_REVISION; > + goto _exit; > + } > + > + ret = le32_to_cpu(result); > + > +_exit: > + clean_msg(msg); > + return ret; > +} > diff --git a/include/linux/iio/common/ssp_sensors.h b/include/linux/iio/common/ssp_sensors.h > new file mode 100644 > index 0000000..a53e3be > --- /dev/null > +++ b/include/linux/iio/common/ssp_sensors.h > @@ -0,0 +1,79 @@ > +/* > + * Copyright (C) 2014, Samsung Electronics Co. Ltd. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that 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. > + * > + */ > +#ifndef _SSP_SENSORS_H_ > +#define _SSP_SENSORS_H_ > + > +#include <linux/iio/iio.h> > + > +#define SSP_TIME_SIZE 4 > +#define SSP_ACCELEROMETER_SIZE 6 > +#define SSP_GYROSCOPE_SIZE 6 > +#define SSP_BIO_HRM_RAW_SIZE 8 > +#define SSP_BIO_HRM_RAW_FAC_SIZE 36 > +#define SSP_BIO_HRM_LIB_SIZE 8 > + > +/** > + * enum ssp_sensor_tyoe - SSP sensor type Typo: ssp_sensor_type > + */ > +enum ssp_sensor_type { > + SSP_ACCELEROMETER_SENSOR = 0, > + SSP_GYROSCOPE_SENSOR, > + SSP_GEOMAGNETIC_UNCALIB_SENSOR, > + SSP_GEOMAGNETIC_RAW, > + SSP_GEOMAGNETIC_SENSOR, > + SSP_PRESSURE_SENSOR, > + SSP_GESTURE_SENSOR, > + SSP_PROXIMITY_SENSOR, > + SSP_TEMPERATURE_HUMIDITY_SENSOR, > + SSP_LIGHT_SENSOR, > + SSP_PROXIMITY_RAW, > + SSP_ORIENTATION_SENSOR, > + SSP_STEP_DETECTOR, > + SSP_SIG_MOTION_SENSOR, > + SSP_GYRO_UNCALIB_SENSOR, > + SSP_GAME_ROTATION_VECTOR, > + SSP_ROTATION_VECTOR, > + SSP_STEP_COUNTER, > + SSP_BIO_HRM_RAW, > + SSP_BIO_HRM_RAW_FAC, > + SSP_BIO_HRM_LIB, > + SSP_SENSOR_MAX, > +}; > + > +struct ssp_data; > + > +/** > + * struct ssp_sensor_data - Sensor object > + * @process_data: Callback to feed sensor data. > + * @type: Used sensor type. > + */ > +struct ssp_sensor_data { > + int (*process_data)(struct iio_dev *indio_dev, void *buf, > + int64_t timestamp); > + enum ssp_sensor_type type; > +}; > + > +void ssp_register_consumer(struct iio_dev *indio_dev, enum ssp_sensor_type); void ssp_register_consumer(struct iio_dev *indio_dev, enum ssp_sensor_type type); > + > +int ssp_enable_sensor(struct ssp_data *data, enum ssp_sensor_type type, > + u32 delay); > + > +int ssp_disable_sensor(struct ssp_data *data, enum ssp_sensor_type type); > + > +u32 ssp_get_sensor_delay(struct ssp_data *data, enum ssp_sensor_type); > + > +int ssp_change_delay(struct ssp_data *data, enum ssp_sensor_type type, > + u32 delay); > +#endif /* _SSP_SENSORS_H_ */ > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html