On Mon, Mar 23, 2020 at 10:28:30AM +0100, Mike Looijmans wrote:
> The BMI088 is a combined module with both accelerometer and gyroscope.
> This adds the accelerometer driver support for the SPI interface.
> The gyroscope part is already supported by the BMG160 driver.
Thank you, the comment about shared buffer given to v3 still applies.
Also see below.
...
> +enum bmi088_accel_axis {
> + AXIS_X,
> + AXIS_Y,
> + AXIS_Z,
> + AXIS_MAX,
If it's a terminator entry, comma is not needed.
> +};
...
> +/* Available ODR (output data rates) in Hz */
> +enum bmi088_odr_modes {
> + BMI088_ACCEL_MODE_ODR_12_5 = 0x5,
> + BMI088_ACCEL_MODE_ODR_25 = 0x6,
> + BMI088_ACCEL_MODE_ODR_50 = 0x7,
> + BMI088_ACCEL_MODE_ODR_100 = 0x8,
> + BMI088_ACCEL_MODE_ODR_200 = 0x9,
> + BMI088_ACCEL_MODE_ODR_400 = 0xa,
> + BMI088_ACCEL_MODE_ODR_800 = 0xb,
> + BMI088_ACCEL_MODE_ODR_1600 = 0xc,
> +};
I'm wondering if you need this enum at all? Only 3 out of 8 are in use, and 25
of them can be still derived from the 12.5 one.
Maybe replace with comment and ranges?
(But it's up to you, I have no strong opinion here)
...
> +static int bmi088_accel_set_power_state(struct bmi088_accel_data *data, bool on)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret;
> +
> + if (on) {
> + ret = pm_runtime_get_sync(dev);
if (ret < 0)
pm_runtime_put_noidle(dev);
See below.
> + } else {
> + pm_runtime_mark_last_busy(dev);
> + ret = pm_runtime_put_autosuspend(dev);
> + }
> +
> + if (ret < 0) {
> + dev_err(dev, "Failed: %s(%d)\n", __func__, on);
> + if (on)
> + pm_runtime_put_noidle(dev);
Perhaps refactor as above?
In this case it maybe simple...
> +
> + return ret;
> + }
> +
> + return 0;
...like this
if (ret < 0)
dev_err(dev, "Failed: %s(%d)\n", __func__, on);
return ret < 0 ? ret : 0;
(I guess compiler is clever enough to avoid condition twice, but again, I have
no strong opinion)
> +}
...
> +static int bmi088_accel_enable(struct bmi088_accel_data *data, bool on_off)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret;
> +
> + ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CTRL,
> + on_off ? 0x4 : 0x0);
I think
u32 val = on_off ? 0x4 : 0x0;
...
ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CTRL, val);
will look better.
> + if (ret) {
> + dev_err(dev, "Error writing ACC_PWR_CTRL reg\n");
> + return ret;
> + }
> + return 0;
> +}
> +
> +/* In suspend mode, only the accelerometer is powered down. */
> +static int bmi088_accel_set_mode(struct bmi088_accel_data *data,
> + enum bmi088_power_modes mode)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret;
> +
> + ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CONF,
> + mode == BMI088_ACCEL_MODE_SUSPEND ? 0x3 : 0x0);
Ditto.
> + if (ret) {
> + dev_err(dev, "Error writing ACCEL_PWR_CONF reg\n");
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +static int bmi088_accel_set_bw(struct bmi088_accel_data *data,
> + enum bmi088_odr_modes odr_mode,
> + enum bmi088_osr_modes osr_mode)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret;
> + u8 value = (osr_mode << 4) | (odr_mode & 0xF);
_MASK (GENMASK() + _SHIFT? (See also below)
u8 value = (osr_mode << _SHIFT) | (odr_mode & _MASK);
int ret;
(note reverse xmas tree ordering)
> +
> + ret = regmap_write(data->regmap, BMI088_ACCEL_REG_ACC_CONF, value);
> + if (ret) {
> + dev_err(dev, "Error writing ACCEL_PWR_CONF reg\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int bmi088_accel_get_sample_freq(struct bmi088_accel_data *data,
> + int* val, int *val2)
> +{
> + unsigned int value;
> + int ret;
> + value &= 0xf; /* ODR in lower 4 bits */
_MASK? See above.
> + if (value == BMI088_ACCEL_MODE_ODR_12_5) {
> + *val = 12;
> + *val2 = 500000;
> + ret = IIO_VAL_INT_PLUS_MICRO;
> + } else {
> + *val = 25 << (value - BMI088_ACCEL_MODE_ODR_25);
> + *val2 = 0;
> + ret = IIO_VAL_INT;
> + }
> +
> + return ret;
> +}
...
> +
> +static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
> +{
> + unsigned int value = BMI088_ACCEL_MODE_ODR_1600;
> +
> + if (val < 12 || val > 1600)
> + return -EINVAL;
> +
> + value = fls(val) + (BMI088_ACCEL_MODE_ODR_12_5 - 4);
Wouldn't be
value = fls(val) + 1;
more obvious? Or, perhaps,
roundup_pow_of_two()
?
> + return regmap_update_bits(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
> + 0x0f, value);
_MASK ?
> +}
> + if (ret)
> + return ret;
> +
> + *val = temp >> 5;
Magic shift.
...
> + s16 raw_val;
> + ret = regmap_bulk_read(data->regmap,
> + BMI088_ACCEL_AXIS_TO_REG(chan->scan_index),
> + data->buffer, 2);
> + raw_val = get_unaligned_le16(data->buffer);
I'm wondering if you can simple use le16_to_cpu()? I guess that buffer is
always aligned and since you access it always by even addresses, it implies
aligned access. Applies to other places as well.
...
> + case IIO_ACCEL:
> + {
Why do you need block?
> + ret = regmap_read(data->regmap,
> + BMI088_ACCEL_REG_ACC_RANGE, val);
> + if (ret)
> + return ret;
> +
> + *val2 = 15 - (*val & 0x3);
Extra spaces.
> + *val = 3 * 980;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> + default:
> + return -EINVAL;
> + }
...
> +static const unsigned long bmi088_accel_scan_masks[] = {
> + BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> + 0
+ comma or is it agreed value for termination the list?
> +};
...
> + usleep_range(BMI088_ACCEL_MAX_STARTUP_TIME_MS * 1000,
> + BMI088_ACCEL_MAX_STARTUP_TIME_MS * 1000 * 2);
unsigned long /* or what is used */ sleep = BMI088_ACCEL_MAX_STARTUP_TIME_MS * USEC_PER_MSEC;
...
usleep_range(sleep, 2 * sleep);
?
...
> + addr[0] |= 0x80; /* bit7 = RW = '1' */
BIT(7) ?
...
> +static const struct spi_device_id bmi088_accel_id[] = {
> + {"bmi088_accel", 0},
', 0' part is not needed.
> + {}
> +};
> +++ b/drivers/iio/accel/bmi088-accel.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef BMI088_ACCEL_H
> +#define BMI088_ACCEL_H
> +
> +extern const struct regmap_config bmi088_regmap_conf;
> +extern const struct dev_pm_ops bmi088_accel_pm_ops;
Do you need extern?
> +int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> + const char *name, bool block_supported);
> +int bmi088_accel_core_remove(struct device *dev);
This needs
#include <linux/types.h>
struct device;
struct regmap;
--
With Best Regards,
Andy Shevchenko
