On 12/11/2012 07:35 PM, Ge Gao wrote: > Dear Lars-Peter, > Thanks for the comments. I will modify the code according the > comments. But before that, I have some questions about the comments. > 1. About power management: My previous design goal is that once master > enable is on, which is the buffer/enable under iio:deviceX/ directory, no > parameters should change. The parameter change should only happen when > enable is zero. The parameters include scale, sampling rate and there > could be more if secondary I2C slave or DMP is added. This would simplify > the design and avoid racing problems. However, writing into the scale and > sampling rate register would need the power to be on. Ok, let me see if I understand it correctly. The enable field is set when the trigger is enabled and the device is in buffered sampling mode. is_asleep gets set when the device is powered down from sysfs. inv_check_enable() checks whether is asleep is set or enable is set. Which means it returns true if the device is either powered down or in buffered sampling mode. Given this I guess it makes sense to check for it in write_raw, but the name is in my opinion a bit misleading. But still leaves the question unsolved whether the power up/down can be done on demand rather than manually. > 2. For the inv_mpu_misc.c file: I think this file is pretty big and is not > very useful in normal operations. It is only used when the program > started. It may also contain some more other stuff. If all into the > inv_mpu_core.c, it is too big. Hm, ok. > 3. For the i2c read and write function: I used to have inv_i2c_read and > inv_i2c_write functions in my previous submission, but I was told that it > is not immediate clear that this function is actually calling the smbus > version of I2C and was suggested to use the original function. Now you > want this function to be wrapped. Which is the correct way to do that? > I guess there is no wrong or right in this case, only preference. I clearly prefer result = mpu_write_reg(st, reg->lpf, 5); over int d; d = 5; result = i2c_smbus_write_i2c_block_data(st->client, reg->lpf, 1, &d); For two reasons: 1) It's much shorter 2) It also explains what it does. It writes to a register. I mean that's why we have stuff like abstractions and helper functions to focus on what is done and not how it is done. Whether mpu_write_reg uses smbus functions or magic ponies in the background to get the value written to the register should not matter ;) - Lars > Best Regards, > > Ge GAO > > > -----Original Message----- > From: Lars-Peter Clausen [mailto:lars@xxxxxxxxxx] > Sent: Tuesday, December 11, 2012 2:28 AM > To: Ge GAO > Cc: jic23@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] [V5] Invensense MPU6050 Device Driver. > > On 12/08/2012 12:46 AM, Ge GAO wrote: >> From: Ge Gao <ggao@xxxxxxxxxxxxxx> >> >> --This the basic function of Invensense MPU6050 Deivce driver. >> --MPU6050 is a gyro/accel combo device. >> --This does not contain any secondary I2C slave devices. >> --This does not contain DMP functions. DMP is a on-chip engine >> that can do powerful process of motion gestures such as tap, >> screen orientation, etc. >> >> Signed-off-by: Ge Gao <ggao@xxxxxxxxxxxxxx> > > Hi, > > Looks pretty good, but there are a few review comments from last time > which have not been addressed. There is only one major issue left > everything else is basically just minor things or nitpicking. > > The major issue is how the driver deals with power management. You have a > sysfs attribute which allows to enable or disable the device. Jonathon is > not really a fan of this and I'm neither. So the question is can power > management be managed dynamically? Which means power it up when it is > needed, e.g. sampling is started, power it down if it is not needed > anymore, e.g. sampling is stopped again. > >> --- >> Documentation/ABI/testing/sysfs-bus-iio-mpu6050 | 14 + >> drivers/iio/Kconfig | 2 +- >> drivers/iio/imu/Kconfig | 2 + >> drivers/iio/imu/Makefile | 2 + >> drivers/iio/imu/inv_mpu6050/Kconfig | 12 + >> drivers/iio/imu/inv_mpu6050/Makefile | 11 + >> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 781 > +++++++++++++++++++++++ >> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 394 ++++++++++++ >> drivers/iio/imu/inv_mpu6050/inv_mpu_misc.c | 244 +++++++ >> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 346 ++++++++++ >> drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 77 +++ >> include/linux/iio/imu/mpu.h | 33 + >> 12 files changed, 1917 insertions(+), 1 deletions(-) create mode >> 100644 Documentation/ABI/testing/sysfs-bus-iio-mpu6050 >> create mode 100644 drivers/iio/imu/inv_mpu6050/Kconfig >> create mode 100644 drivers/iio/imu/inv_mpu6050/Makefile >> create mode 100644 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> create mode 100644 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h >> create mode 100644 drivers/iio/imu/inv_mpu6050/inv_mpu_misc.c >> create mode 100644 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c >> create mode 100644 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> create mode 100644 include/linux/iio/imu/mpu.h >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-mpu6050 >> b/Documentation/ABI/testing/sysfs-bus-iio-mpu6050 >> new file mode 100644 >> index 0000000..6a8a2b4 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-bus-iio-mpu6050 >> @@ -0,0 +1,14 @@ >> +What: /sys/bus/iio/devices/iio:deviceX/gyro_matrix >> +What: /sys/bus/iio/devices/iio:deviceX/accel_matrix >> +What: /sys/bus/iio/devices/iio:deviceX/magn_matrix >> +KernelVersion: 3.4.0 > > KernelVersion should probably be 3.9 > >> +Contact: linux-iio@xxxxxxxxxxxxxxx >> +Description: >> + This is mounting matrix for motion sensors. Mounting > matrix >> + is a 3x3 unitary matrix. A typical mounting matrix would > look like >> + [0, 1, 0; 1, 0, 0; 0, 0, -1]. Using this information, it > would be >> + easy to tell the relative positions among sensors as well > as their >> + positions relative to the board that holds these sensors. > Identity matrix >> + [1, 0, 0; 0, 1, 0; 0, 0, 1] means sensor chip and device > are perfectly >> + aligned with each other. All axes are exactly the same. >> + >> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig index >> b2f963b..f4692e9 100644 >> --- a/drivers/iio/Kconfig >> +++ b/drivers/iio/Kconfig >> @@ -36,7 +36,7 @@ config IIO_KFIFO_BUF >> often to read from the buffer. >> >> config IIO_TRIGGERED_BUFFER >> - tristate >> + tristate "helper function to setup triggered buffer" > > Hu? What's up with this change? > IIO_TRIGGERED_BUFFER should not be user selectable, a driver which needs > it should select it not depend on it. > >> select IIO_TRIGGER >> select IIO_KFIFO_BUF >> help >> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig index >> 3d79a40..723563b 100644 >> --- a/drivers/iio/imu/Kconfig >> +++ b/drivers/iio/imu/Kconfig >> @@ -25,3 +25,5 @@ config IIO_ADIS_LIB_BUFFER >> help >> A set of buffer helper functions for the Analog Devices ADIS* > device >> family. >> + >> +source "drivers/iio/imu/inv_mpu6050/Kconfig" >> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile index >> cfe5763..1b41584 100644 >> --- a/drivers/iio/imu/Makefile >> +++ b/drivers/iio/imu/Makefile >> @@ -8,3 +8,5 @@ adis_lib-y += adis.o >> adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_trigger.o >> adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o >> obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o >> + >> +obj-y += inv_mpu6050/ >> diff --git a/drivers/iio/imu/inv_mpu6050/Kconfig >> b/drivers/iio/imu/inv_mpu6050/Kconfig >> new file mode 100644 >> index 0000000..62b475e >> --- /dev/null >> +++ b/drivers/iio/imu/inv_mpu6050/Kconfig >> @@ -0,0 +1,12 @@ >> +# >> +# inv-mpu6050 drivers for Invensense MPU devices and combos # >> + >> +config INV_MPU6050_IIO >> + tristate "Invensense MPU6050 devices" >> + depends on I2C && SYSFS && IIO && IIO_KFIFO_BUF && >> +IIO_TRIGGERED_BUFFER > > select IIO_TRIGGERED_BUFFER instead of depends on > >> + help >> + This driver supports the Invensense MPU6050 devices. >> + It is a gyroscope/accelerometer combo device. >> + This driver can be built as a module. The module will be called >> + inv-mpu6050. >> diff --git a/drivers/iio/imu/inv_mpu6050/Makefile >> b/drivers/iio/imu/inv_mpu6050/Makefile >> new file mode 100644 >> index 0000000..5161777 >> --- /dev/null >> +++ b/drivers/iio/imu/inv_mpu6050/Makefile >> @@ -0,0 +1,11 @@ >> +# >> +# Makefile for Invensense MPU6050 device. >> +# >> + >> +obj-$(CONFIG_INV_MPU6050_IIO) += inv-mpu6050.o >> + >> +inv-mpu6050-objs := inv_mpu_core.o >> +inv-mpu6050-objs += inv_mpu_ring.o >> +inv-mpu6050-objs += inv_mpu_misc.o >> +inv-mpu6050-objs += inv_mpu_trigger.o >> + >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> new file mode 100644 >> index 0000000..c2c7e9c >> --- /dev/null >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c >> @@ -0,0 +1,781 @@ > [...] >> +int inv_switch_engine(struct inv_mpu_iio_s *st, bool en, u32 mask) { >> + struct inv_reg_map_s *reg; >> + u8 data, mgmt_1; >> + int result; >> + >> + reg = st->reg; >> + /* switch clock needs to be careful. Only when gyro is on, can >> + clock source be switched to gyro. Otherwise, it must be set to >> + internal clock */ >> + if (MPU_BIT_PWR_GYRO_STBY == mask) { >> + result = i2c_smbus_read_i2c_block_data(st->client, >> + reg->pwr_mgmt_1, 1, > &mgmt_1); >> + if (result != 1) >> + return result; >> + >> + mgmt_1 &= ~MPU_BIT_CLK_MASK; >> + } >> + >> + if ((MPU_BIT_PWR_GYRO_STBY == mask) && (!en)) { > > Stricktly speaking the extra parenthis are not neccesary and it is rahter > uncommon to see them in kernel code. > >> + /* turning off gyro requires switch to internal clock > first. >> + Then turn off gyro engine */ >> + mgmt_1 |= INV_CLK_INTERNAL; >> + result = i2c_smbus_write_i2c_block_data(st->client, >> + reg->pwr_mgmt_1, 1, > &mgmt_1); >> + if (result) >> + return result; >> + } >> + >> + result = i2c_smbus_read_i2c_block_data(st->client, >> + reg->pwr_mgmt_2, 1, &data); >> + if (result != 1) >> + return result; >> + if (en) >> + data &= ~mask; >> + else >> + data |= mask; >> + result = i2c_smbus_write_i2c_block_data(st->client, >> + reg->pwr_mgmt_2, 1, > &data); >> + if (result) >> + return result; >> + >> + if ((MPU_BIT_PWR_GYRO_STBY == mask) && en) { > > strictly speaking the extra parenthesis around the comparison are not > required. > >> + /* only gyro on needs sensor up time */ >> + msleep(SENSOR_UP_TIME); >> + /* after gyro is on & stable, switch internal clock to PLL > */ >> + mgmt_1 |= INV_CLK_PLL; >> + result = i2c_smbus_write_i2c_block_data(st->client, >> + reg->pwr_mgmt_1, 1, > &mgmt_1); >> + if (result) >> + return result; >> + } >> + >> + return 0; >> +} >> + > [...] >> + >> +/** >> + * inv_init_config() - Initialize hardware, disable FIFO. >> + * @indio_dev: Device driver instance. >> + * Initial configuration: >> + * FSR: +/- 2000DPS >> + * DLPF: 20Hz >> + * FIFO rate: 50Hz >> + * Clock source: Gyro PLL >> + */ >> +static int inv_init_config(struct iio_dev *indio_dev) { >> + struct inv_reg_map_s *reg; >> + int result; >> + u8 d; >> + struct inv_mpu_iio_s *st = iio_priv(indio_dev); >> + >> + if (st->chip_config.is_asleep) >> + return -EPERM; >> + reg = st->reg; >> + result = set_inv_enable(indio_dev, false); >> + if (result) >> + return result; >> + >> + d = (INV_FSR_2000DPS << GYRO_CONFIG_FSR_SHIFT); >> + result = i2c_smbus_write_i2c_block_data(st->client, > reg->gyro_config, >> + 1, &d); >> + if (result) >> + return result; >> + >> + d = INV_FILTER_20HZ; >> + result = i2c_smbus_write_i2c_block_data(st->client, reg->lpf, 1, > &d); >> + if (result) >> + return result; >> + >> + d = ONE_K_HZ / INIT_FIFO_RATE - 1; >> + result = i2c_smbus_write_i2c_block_data(st->client, >> + reg->sample_rate_div, >> + 1, &d); >> + if (result) >> + return result; >> + st->irq_dur_ns = INIT_DUR_TIME, >> + >> + d = (INV_FS_02G << ACCL_CONFIG_FSR_SHIFT); > > No need for the extra brackets. > >> + result = i2c_smbus_write_i2c_block_data(st->client, > reg->accl_config, >> + 1, &d); >> + if (result) >> + return result; >> + >> + memcpy(&st->chip_config, &chip_config_6050, >> + sizeof(struct inv_chip_config_s)); >> + >> + return 0; >> +} >> + >> +static int inv_q30_mult(int a, int b) { >> + long long temp; >> + int result; >> + temp = (long long)a * b; >> + result = (int)(temp >> Q30_MULTI_SHIFT); >> + >> + return result; >> +} >> + >> +/** >> + * inv_temperature_show() - Read temperature data directly from > registers. >> + */ >> +static int inv_temperature_show(struct inv_mpu_iio_s *st, int *val) >> +{ >> + struct inv_reg_map_s *reg; >> + int result; >> + s16 temp; >> + u8 data[2]; >> + >> + reg = st->reg; >> + if (st->chip_config.is_asleep) >> + return -EPERM; >> + result = i2c_smbus_read_i2c_block_data(st->client, >> + reg->temperature, 2, data); >> + if (result != 2) { >> + dev_err(&st->client->adapter->dev, "%s failed %d\n", > __func__, >> + result); >> + return -EINVAL; >> + } >> + temp = (s16)(be16_to_cpup((s16 *)&data[0])); >> + >> + *val = MPU6050_TEMP_OFFSET + >> + inv_q30_mult((int)temp << MPU_TEMP_SHIFT, >> + MPU6050_TEMP_SCALE); > > It would be better to have scale and offset for the temperature channel > and just return the raw value. > >> + >> + return IIO_VAL_INT; >> +} > >> +/** >> + * mpu_read_raw() - read raw method. >> + */ >> +static int mpu_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int *val, >> + int *val2, >> + long mask) { >> + struct inv_mpu_iio_s *st = iio_priv(indio_dev); >> + >> + if (st->chip_config.is_asleep) > > Sometimes you just check is_asleep sometimes you use inv_check_enable, is > this on purpose? Also can the scale still be queried if the device is > asleep? > >> + return -EINVAL; >> + switch (mask) { >> + case 0: >> + if (!st->chip_config.enable) >> + return -EPERM; >> + switch (chan->type) { >> + case IIO_ANGL_VEL: >> + return inv_sensor_show(st, st->reg->raw_gyro, >> + chan->channel2, val); >> + case IIO_ACCEL: >> + return inv_sensor_show(st, st->reg->raw_accl, >> + chan->channel2, val); >> + case IIO_TEMP: >> + return inv_temperature_show(st, val); >> + default: >> + return -EINVAL; >> + } >> + case IIO_CHAN_INFO_SCALE: >> + switch (chan->type) { >> + case IIO_ANGL_VEL: >> + *val = gyro_scale_6050[st->chip_config.fsr]; >> + >> + return IIO_VAL_INT; >> + case IIO_ACCEL: >> + *val = accel_scale[st->chip_config.accl_fs]; >> + >> + return IIO_VAL_INT; >> + default: >> + return -EINVAL; >> + } >> + default: >> + return -EINVAL; >> + } >> +} >> + > >> + >> +/** >> + * mpu_write_raw() - write raw method. >> + */ >> +static int mpu_write_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int val, >> + int val2, >> + long mask) { >> + struct inv_mpu_iio_s *st = iio_priv(indio_dev); >> + int result; >> + >> + if (inv_check_enable(st)) >> + return -EPERM; > > Can the scale really only be updated if the device is enabled? > >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_SCALE: >> + switch (chan->type) { >> + case IIO_ANGL_VEL: >> + result = inv_write_fsr(st, val); >> + break; >> + case IIO_ACCEL: >> + result = inv_write_accel_fs(st, val); >> + break; >> + default: >> + result = -EINVAL; >> + break; >> + } >> + return result; >> + default: >> + return -EINVAL; >> + } >> +} > [...] >> +/** >> + * inv_check_setup_chip() - check and setup chip type. >> + */ >> +static int inv_check_and_setup_chip(struct inv_mpu_iio_s *st, >> + const struct i2c_device_id *id) >> +{ >> + struct inv_reg_map_s *reg; >> + int result; >> + u8 d; >> + >> + st->chip_type = INV_MPU6050; >> + st->hw = &hw_info[st->chip_type]; >> + st->reg = (struct inv_reg_map_s *)®_set_6050; > > The cast should not be neccessary. > >> + reg = st->reg; >> + /* reset to make sure previous state are not there */ >> + d = MPU_BIT_H_RESET; >> + result = i2c_smbus_write_i2c_block_data(st->client, > reg->pwr_mgmt_1, >> + 1, &d); >> + if (result) >> + return result; >> + msleep(POWER_UP_TIME); >> + /* toggle power state */ >> + result = inv_set_power_itg(st, false); >> + if (result) >> + return result; >> + result = inv_set_power_itg(st, true); >> + if (result) >> + return result; >> + /* turn off MEMS engine at start up */ >> + result = inv_switch_engine(st, false, MPU_BIT_PWR_GYRO_STBY); >> + if (result) >> + return result; >> + >> + result = inv_switch_engine(st, false, MPU_BIT_PWR_ACCL_STBY); >> + if (result) >> + return result; >> + >> + result = inv_get_silicon_rev_mpu6050(st); >> + if (result) { >> + dev_err(&st->client->adapter->dev, "%s failed %d\n", > __func__, >> + result); >> + inv_set_power_itg(st, false); >> + return result; >> + } >> + >> + return 0; >> +} >> + >> +/** >> + * inv_mpu_probe() - probe function. >> + */ >> +static int inv_mpu_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct inv_mpu_iio_s *st; >> + struct iio_dev *indio_dev; >> + int result; >> + >> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { >> + result = -ENOSYS; >> + goto out_no_free; >> + } >> + indio_dev = iio_device_alloc(sizeof(*st)); >> + if (indio_dev == NULL) { >> + result = -ENOMEM; >> + goto out_no_free; >> + } >> + st = iio_priv(indio_dev); >> + st->client = client; >> + st->plat_data = >> + *(struct mpu_platform_data > *)dev_get_platdata(&client->dev); > > No cast neccesary. > >> + /* power is turned on inside check chip type*/ >> + result = inv_check_and_setup_chip(st, id); >> + if (result) >> + goto out_free; >> + >> + result = inv_init_config(indio_dev); >> + if (result) { >> + dev_err(&client->adapter->dev, > > This should be &client->dev, There are a few other similar places where > this comment applies as well. > >> + "Could not initialize device.\n"); > > It makes sense to also print the error code, same applies to the other > error messages in probe(). > >> + goto out_free; >> + } >> + >> + i2c_set_clientdata(client, indio_dev); >> + indio_dev->dev.parent = &client->dev; >> + indio_dev->name = id->name; >> + indio_dev->channels = inv_mpu_channels; >> + indio_dev->num_channels = ARRAY_SIZE(inv_mpu_channels); >> + >> + indio_dev->info = &mpu_info; >> + indio_dev->modes = INDIO_BUFFER_TRIGGERED; >> + >> + result = inv_mpu_configure_ring(indio_dev); >> + if (result) { >> + dev_err(&st->client->adapter->dev, "configure buffer > fail\n"); >> + goto out_free; >> + } >> + st->irq = client->irq; >> + result = inv_mpu_probe_trigger(indio_dev); >> + if (result) { >> + dev_err(&st->client->adapter->dev, "trigger probe > fail\n"); >> + goto out_unreg_ring; >> + } >> + >> + INIT_KFIFO(st->timestamps); >> + spin_lock_init(&st->time_stamp_lock); >> + result = iio_device_register(indio_dev); >> + if (result) { >> + dev_err(&st->client->adapter->dev, "IIO register fail\n"); >> + goto out_remove_trigger; >> + } >> + >> + return 0; >> + >> +out_remove_trigger: >> + if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) > > Will this ever be false? Considering that you set modes to > INDIO_BUFFER_TRIGGERED a few lines above I don't think so. > >> + inv_mpu_remove_trigger(indio_dev); >> +out_unreg_ring: >> + iio_triggered_buffer_cleanup(indio_dev); >> +out_free: >> + iio_device_free(indio_dev); >> +out_no_free: >> + dev_err(&client->adapter->dev, "%s failed %d\n", __func__, > result); > > The generic device driver layer will already print a similar message if > probe fails, so this one is kind of redudant and can be removed. > >> + >> + return result; >> +} >> + >> +/** >> + * inv_mpu_remove() - remove function. >> + */ >> +static int inv_mpu_remove(struct i2c_client *client) { >> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >> + struct inv_mpu_iio_s *st = iio_priv(indio_dev); >> + kfifo_free(&st->timestamps); >> + iio_device_unregister(indio_dev); >> + iio_triggered_buffer_cleanup(indio_dev); >> + iio_device_free(indio_dev); >> + >> + if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) >> + inv_mpu_remove_trigger(indio_dev); >> + >> + return 0; >> +} > [...] >> + */ >> +static const struct i2c_device_id inv_mpu_id[] = { >> + {"mpu6050", INV_MPU6050}, >> + {} >> +}; >> + >> +MODULE_DEVICE_TABLE(i2c, inv_mpu_id); >> + > [...] >> +MODULE_ALIAS("inv-mpu6050"); > > You don't need MODULE_ALIAS if you have a MODULE_DEVICE_TABLE. > MODULE_DEVICE_TABLE creates a MODULE_ALIAS for each device table entry. > > [...] >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_misc.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_misc.c >> new file mode 100644 >> index 0000000..99f3dfe >> --- /dev/null >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_misc.c > > I wonder why this needs a extra file and can't go into the main file. > >> @@ -0,0 +1,244 @@ > [...] >> +static int mpu_memory_read(struct inv_mpu_iio_s *st, >> + u16 mem_addr, u32 len, u8 *data) { >> + >> + res = i2c_transfer(st->client->adapter, msgs, 4); > > ARRAY_SIZE(msgs) > >> + if (res != 4) { >> + if (res >= 0) >> + res = -EIO; >> + return res; >> + } else { >> + return 0; >> + } >> +} > [...] >> + >> +int inv_get_silicon_rev_mpu6050(struct inv_mpu_iio_s *st) { >> + int result; >> + struct inv_reg_map_s *reg; >> + u8 prod_ver = 0x00, prod_rev = 0x00; >> + struct prod_rev_map_t *p_rev; >> + u8 d; >> + u8 bank = (MPU_BIT_PRFTCH_EN | MPU_BIT_CFG_USER_BANK | >> + MPU_MEM_OTP_BANK_0); >> + u16 mem_addr = ((bank << 8) | MEM_ADDR_PROD_REV); >> + u16 key; >> + u8 regs[SOFT_PROD_VER_BYTES]; >> + u16 sw_rev; >> + s16 index; >> + struct inv_chip_info_s *chip_info = &st->chip_info; > > This is a bit ugly, maybe reorder the declarations in christmas tree or > reverse christmas tree order (means sort them by line length). > >> + >> + reg = st->reg; >> + result = i2c_smbus_read_i2c_block_data(st->client, >> + MPU_REG_PRODUCT_ID, >> + 1, &prod_ver); >> + if (result != 1) >> + return result; >> + prod_ver &= 0xf; >> + /*memory read need more time after power up */ > > missing space after the /* > > [...] >> +} >> + >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c >> new file mode 100644 >> index 0000000..c231e36 >> --- /dev/null >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > [...] >> + >> +/** >> + * reset_fifo() - Reset FIFO related registers. >> + * @indio_dev: Device driver instance. >> + */ >> +static int inv_reset_fifo(struct iio_dev *indio_dev) { >> + struct inv_reg_map_s *reg; >> + int result; >> + u8 d; >> + struct inv_mpu_iio_s *st = iio_priv(indio_dev); >> + >> + reg = st->reg; >> + d = 0; >> + /* disable interrupt */ >> + result = i2c_smbus_write_i2c_block_data(st->client, > reg->int_enable, >> + 1, &d); > > Considering how often you need to read or write a single byte it may make > sense to add a helper function for this which wraps > i2c_smbus_write_i2c_block_data and takes the value as a parameter, this > would allow to reduce the code size a bit, which also makes the code > easier to read. > > cause you could e.g. write the line above > result = inv_write_reg(st, reg->int_enable, 0); > >> + if (result) { >> + dev_err(&st->client->adapter->dev, "int_enable failed\n"); >> + return result; >> + } > [...] > >> +} >> + >> +/** >> + * set_inv_enable() - enable chip functions. >> + * @indio_dev: Device driver instance. >> + * @enable: enable/disable >> + */ >> +int set_inv_enable(struct iio_dev *indio_dev, bool enable) { > [...] >> + } >> + st->chip_config.enable = !!enable; > > enable is already a bool, so !! is a no-op. > >> + >> + return 0; >> +} >> + > > [...] >> + >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> new file mode 100644 >> index 0000000..c5ae262 >> --- /dev/null >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c >> @@ -0,0 +1,77 @@ >> +/* >> +* Copyright (C) 2012 Invensense, Inc. >> +* >> +* This software is licensed under the terms of the GNU General Public >> +* License version 2, as published by the Free Software Foundation, >> +and >> +* may be copied, distributed, and modified under those terms. >> +* >> +* 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. >> +* >> +*/ >> + >> +/** >> + * @addtogroup DRIVERS >> + * @brief Hardware drivers. >> + * >> + * @{ >> + * @file inv_mpu_trigger.c >> + * @brief A sysfs device driver for Invensense devices >> + * @details This file is part of inv mpu iio driver code >> + */ > > Hm, what kind of comment is that? > > [...] >> diff --git a/include/linux/iio/imu/mpu.h b/include/linux/iio/imu/mpu.h >> new file mode 100644 index 0000000..5ef4b91 >> --- /dev/null >> +++ b/include/linux/iio/imu/mpu.h > > This file should go into include/linux/platform_data/ and mabye it should > get a less generic name. Maybe invensense_mpu.h > >> @@ -0,0 +1,33 @@ >> +/* >> +* Copyright (C) 2012 Invensense, Inc. >> +* >> +* This software is licensed under the terms of the GNU General Public >> +* License version 2, as published by the Free Software Foundation, and >> +* may be copied, distributed, and modified under those terms. >> +* >> +* 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 __MPU_H_ >> +#define __MPU_H_ >> + >> +#define INV_PROD_KEY(ver, rev) (ver * 100 + rev) > > Does this macro need to be in this file? Will it be used by code outside > of > the driver? > >> +/** >> + * struct mpu_platform_data - Platform data for the mpu driver >> + * @orientation: Orientation matrix of the chip >> + * >> + * Contains platform specific information on how to configure the > MPU6050 to >> + * work on this platform. The orientation matricies are 3x3 rotation > matricies >> + * that are applied to the data to rotate from the mounting orientation > to the >> + * platform orientation. The values must be one of 0, 1, or -1 and > each row and >> + * column should have exactly 1 non-zero value. >> + */ >> +struct mpu_platform_data { >> + __s8 orientation[9]; >> +}; >> + >> +#endif /* __MPU_H_ */ >> -- >> 1.7.0.4 >> >> -- >> 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 -- 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
