Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU
From: Andy Shevchenko
Date: Mon Sep 18 2023 - 06:05:15 EST
On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> acceleration, angular rate, and temperature. This sensor includes
> motion-triggered interrupt features, such as a step counter, tap detection,
> and activity/inactivity interrupt capabilities.
>
> The driver supports various functionalities, including data ready, FIFO
> data handling, and events such as tap detection, step counting, and
> activity interrupts
Missing period.
...
> +#include <linux/regmap.h>
> +#include <linux/bits.h>
Ordered?
Missing units.h.
...
> +#define BMI323_INT_MICRO_TO_RAW(val, val2, scale) (((val) * (scale)) + \
> + (((val2) * (scale)) / MEGA))
Better to split:
#define BMI323_INT_MICRO_TO_RAW(val, val2, scale)
((val) * (scale) + ((val2) * (scale)) / MEGA)
Also note dropped redundant parentheses.
...
> +#define BMI323_RAW_TO_MICRO(raw, scale) ((((raw) % (scale)) * MEGA) / scale)
...
> +struct bmi323_data {
> + u32 odrns[2];
> + u32 odrhz[2];
> + __le16 steps_count[2];
> +};
I'm wondering if these 2:s anyhow semantically the same? Shouldn't a definition
be used instead of magic number?
...
> + 320 * MEGA,
> + 160 * MEGA,
> + 80 * MEGA,
> + 40 * MEGA,
> + 20 * MEGA,
> + 10 * MEGA,
> + 5 * MEGA,
> + 2500000,
2500 * KILO,
for the sake of consistency?
> + 1250000,
1250 * KILO,
> +};
...
> +static int bmi323_write_ext_reg(struct bmi323_data *data, unsigned int ext_addr,
> + unsigned int ext_data)
> +{
> + int ret, feature_status;
> +
> + mutex_lock(&data->mutex);
You can start using cleanup.h, it will reduce your code by a few percents!
But the point is it makes it less error prone and less verbose.
Ditto for the entire code base.
> + ret = regmap_read(data->regmap, BMI323_FEAT_DATA_STATUS,
> + &feature_status);
> + if (ret)
> + goto unlock_out;
> +
> + if (!FIELD_GET(BMI323_FEAT_DATA_TX_RDY_MSK, feature_status)) {
> + ret = -EBUSY;
> + goto unlock_out;
> + }
> +
> + ret = regmap_write(data->regmap, BMI323_FEAT_DATA_ADDR, ext_addr);
> + if (ret)
> + goto unlock_out;
> +
> + ret = regmap_write(data->regmap, BMI323_FEAT_DATA_TX, ext_data);
> +
> +unlock_out:
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
...
> +static int bmi323_update_ext_reg(struct bmi323_data *data,
> + unsigned int ext_addr,
> + unsigned int mask, unsigned int ext_data)
> +{
> + unsigned int value;
> + int ret;
> +
> + ret = bmi323_read_ext_reg(data, ext_addr, &value);
> + if (ret)
> + return ret;
> +
> + set_mask_bits(&value, mask, ext_data);
> + ret = bmi323_write_ext_reg(data, ext_addr, value);
> + if (ret)
> + return ret;
> +
> + return 0;
return _write_ext_reg(...);
> +}
...
unsigned int state_value = GENMASK();
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + msk = BMI323_FEAT_IO0_XYZ_MOTION_MSK;
> + raw = 512;
> + config = BMI323_ANYMO1_REG;
> + irq_msk = BMI323_MOTION_MSK;
> + set_mask_bits(&irq_field_val, BMI323_MOTION_MSK,
> + FIELD_PREP(BMI323_MOTION_MSK, motion_irq));
> + set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> + FIELD_PREP(BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> + state ? 7 : 0));
state_value
> + break;
> + case IIO_EV_DIR_FALLING:
> + msk = BMI323_FEAT_IO0_XYZ_NOMOTION_MSK;
> + raw = 0;
> + config = BMI323_NOMO1_REG;
> + irq_msk = BMI323_NOMOTION_MSK;
> + set_mask_bits(&irq_field_val, BMI323_NOMOTION_MSK,
> + FIELD_PREP(BMI323_NOMOTION_MSK, motion_irq));
> + set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> + FIELD_PREP(BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> + state ? 7 : 0));
Ditto.
> + break;
> + default:
> + return -EINVAL;
> + }
...
> +static ssize_t in_accel_gesture_tap_max_dur_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi323_data *data = iio_priv(indio_dev);
> + unsigned int reg_value, raw;
> + int ret, val[2];
Why val is int (i.e. not unsigned)?
> + ret = bmi323_read_ext_reg(data, BMI323_TAP2_REG, ®_value);
> + if (ret)
> + return ret;
> +
> + raw = FIELD_GET(BMI323_TAP2_MAX_DUR_MSK, reg_value);
> + val[0] = raw / BMI323_MAX_GES_DUR_SCALE;
> + val[1] = BMI323_RAW_TO_MICRO(raw, BMI323_MAX_GES_DUR_SCALE);
> + return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, 2, val);
ARRAY_SIZE()
> +}
...
> +static ssize_t in_accel_gesture_tap_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct bmi323_data *data = iio_priv(indio_dev);
> + int ret, val;
> + if (kstrtoint(buf, 10, &val))
> + return -EINVAL;
Don't shadow the error code.
> + if (!(val == 0 || val == 1))
> + return -EINVAL;
So, effectively you should use kstrtobool().
> + ret = bmi323_update_ext_reg(data, BMI323_TAP1_REG,
> + BMI323_TAP1_TIMOUT_MSK,
> + FIELD_PREP(BMI323_TAP1_TIMOUT_MSK, val));
> + if (ret)
> + return ret;
> +
> + return len;
> +}
...
> +static const struct attribute_group bmi323_event_attribute_group = {
> + .attrs = bmi323_event_attributes,
> +};
ATTRIBUTE_GROUPS() ?
...
> +{
> + struct bmi323_data *data = iio_priv(indio_dev);
> + int ret;
Unneeded variable, return directly.
> +
> + switch (type) {
> + case IIO_EV_TYPE_MAG:
> + ret = bmi323_motion_event_en(data, dir, state);
> + return ret;
> + case IIO_EV_TYPE_GESTURE:
> + return bmi323_tap_event_en(data, dir, state);
> + case IIO_EV_TYPE_CHANGE:
> + ret = bmi323_step_wtrmrk_en(data, state);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
...
> + case IIO_EV_INFO_RESET_TIMEOUT:
> + if (val != 0 || val2 < 40000 || val2 > 600000)
> + return -EINVAL;
if (val || ...
Use is_range() from minmax.h.
> +
> + raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> + BMI323_QUITE_TIM_GES_SCALE);
> +
> + return bmi323_update_ext_reg(data, BMI323_TAP3_REG,
> + BMI323_TAP3_QT_AFT_GES_MSK,
> + FIELD_PREP(BMI323_TAP3_QT_AFT_GES_MSK,
...
> + case IIO_EV_INFO_TAP2_MIN_DELAY:
> + if (val != 0 || val2 < 5000 || val2 > 75000)
Ditto.
> + return -EINVAL;
> +
> + raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> + BMI323_DUR_BW_TAP_SCALE);
> +
> + return bmi323_update_ext_reg(data, BMI323_TAP3_REG,
> + BMI323_TAP3_QT_BW_TAP_MSK,
> + FIELD_PREP(BMI323_TAP3_QT_BW_TAP_MSK,
> + raw));
...
> + case IIO_EV_INFO_VALUE:
> + if (val < 0 || val > 7)
> + return -EINVAL;
Ditto.
> + raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> + BMI323_MOTION_THRES_SCALE);
> +
> + return bmi323_update_ext_reg(data, reg,
> + BMI323_MO1_SLOPE_TH_MSK,
> + FIELD_PREP(BMI323_MO1_SLOPE_TH_MSK,
> + raw));
> + case IIO_EV_INFO_PERIOD:
> + if (val < 0 || val > 162)
Ditto.
> + return -EINVAL;
> +
> + raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> + BMI323_MOTION_DURAT_SCALE);
> +
> + return bmi323_update_ext_reg(data,
> + reg + BMI323_MO3_OFFSET,
> + BMI323_MO3_DURA_MSK,
> + FIELD_PREP(BMI323_MO3_DURA_MSK,
> + raw));
> + case IIO_EV_INFO_HYSTERESIS:
> + if (!(val == 0 || val == 1))
Ditto.
> + return -EINVAL;
> +
> + raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> + BMI323_MOTION_HYSTR_SCALE);
> +
> + return bmi323_update_ext_reg(data,
> + reg + BMI323_MO2_OFFSET,
> + BMI323_MO2_HYSTR_MSK,
> + FIELD_PREP(BMI323_MO2_HYSTR_MSK,
> + raw));
...
> + case IIO_EV_TYPE_CHANGE:
> + if (val < 0 || val > 20460)
Ditto.
> + return -EINVAL;
> +
> + raw = val / 20;
> + return bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
> + BMI323_STEP_SC1_WTRMRK_MSK,
> + FIELD_PREP(BMI323_STEP_SC1_WTRMRK_MSK,
> + raw));
...
> + if (val > BMI323_FIFO_FULL_IN_FRAMES)
> + val = BMI323_FIFO_FULL_IN_FRAMES;
min()
...
> + ret = regmap_update_bits(data->regmap, BMI323_FIFO_CONF_REG,
> + BMI323_FIFO_CONF_ACC_GYR_EN_MSK,
> + FIELD_PREP(BMI323_FIFO_CONF_ACC_GYR_EN_MSK,
> + 3));
GENMASK() ?
> + if (ret)
> + goto unlock_out;
...
> + state = data->state == BMI323_BUFFER_FIFO ? true : false;
== already results in boolean type.
...
> +static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
> +static IIO_DEVICE_ATTR_RO(hwfifo_enabled, 0);
Place them closer to the respective callbacks.
...
> + if (!status || FIELD_GET(BMI323_STATUS_ERROR_MSK, status)) {
> + dev_err(data->dev, "status error = 0x%x\n", status);
If it's not your interrupt you may flood the logs here.
> + return IRQ_NONE;
> + }
...
> + int ret, raw;
Why raw is signed?
> + for (raw = 0; raw < ARRAY_SIZE(bmi323_accel_gyro_avrg); raw++)
> + if (avg == bmi323_accel_gyro_avrg[raw])
> + break;
> + if (raw >= ARRAY_SIZE(bmi323_accel_gyro_avrg))
When is the > part true?
> + return -EINVAL;
...
> +static const struct attribute_group bmi323_attrs_group = {
> + .attrs = bmi323_attributes,
> +};
ATTRIBUTE_GROUPS() ?
...
> + ret = bmi323_feature_engine_events(data, BMI323_FEAT_IO0_STP_CNT_MSK,
> + val ? 1 : 0);
Ternary here...
> + if (ret)
> + return ret;
> +
> + set_mask_bits(&data->feature_events, BMI323_FEAT_IO0_STP_CNT_MSK,
> + FIELD_PREP(BMI323_FEAT_IO0_STP_CNT_MSK, val ? 1 : 0));
...and here are dups.
> + return ret;
Can it be not 0 here?
...
> +static int bmi323_get_temp_data(struct bmi323_data *data, int *val)
> +{
> + unsigned int value;
Why it's not defined as __le16 to begin with?
> + int ret;
> +
> + ret = bmi323_get_error_status(data);
> + if (ret)
> + return -EINVAL;
> +
> + ret = regmap_read(data->regmap, BMI323_TEMP_REG, &value);
> + if (ret)
> + return ret;
> +
> + *val = sign_extend32(le16_to_cpup((const __le16 *)&value), 15);
No, simply no castings here.
> + return IIO_VAL_INT;
> +}
...
> + if (bmi323_acc_gyro_odr[odr_index][0] <= 25)
Why not positive check: if (... > 25) ... else ...?
> + mode = ACC_GYRO_MODE_DUTYCYCLE;
> + else
> + mode = ACC_GYRO_MODE_CONTINOUS;
...
> + int odr_raw, ret;
Why odr_raw is signed?
> +
> + odr_raw = ARRAY_SIZE(bmi323_acc_gyro_odr);
> +
> + while (odr_raw--)
> + if (odr == bmi323_acc_gyro_odr[odr_raw][0] &&
> + uodr == bmi323_acc_gyro_odr[odr_raw][1])
> + break;
> + if (odr_raw < 0)
> + return -EINVAL;
In one case in the code you used for-loop, here is while-loop. Maybe a bit of
consistency?
...
> +static int bmi323_set_scale(struct bmi323_data *data,
> + enum bmi323_sensor_type sensor, int val, int val2)
> +{
> + int scale_raw;
> +
> + if (data->state != BMI323_IDLE)
> + return -EBUSY;
> +
> + scale_raw = bmi323_hw[sensor].scale_table_len;
> +
> + while (scale_raw--)
> + if (val == bmi323_hw[sensor].scale_table[scale_raw][0] &&
> + val2 == bmi323_hw[sensor].scale_table[scale_raw][1])
> + break;
> + if (scale_raw < 0)
> + return -EINVAL;
Note, here is a different case and hence fine to be while-loop.
> + return regmap_update_bits(data->regmap, bmi323_hw[sensor].config,
> + BMI323_ACC_GYRO_CONF_SCL_MSK,
> + FIELD_PREP(BMI323_ACC_GYRO_CONF_SCL_MSK,
> + scale_raw));
> +}
...
> + fwnode = dev_fwnode(data->dev);
> + if (!fwnode)
> + return -ENODEV;
> +
> + irq = fwnode_irq_get_byname(fwnode, "INT1");
> + if (irq > 0) {
> + irq_pin = BMI323_IRQ_INT1;
> + } else {
> + irq = fwnode_irq_get_byname(fwnode, "INT2");
> + if (irq <= 0)
When can it be == 0?
> + return 0;
> +
> + irq_pin = BMI323_IRQ_INT2;
> + }
...
> + irq_type = irqd_get_trigger_type(desc);
> +
Redundant blank line.
> + switch (irq_type) {
> + case IRQF_TRIGGER_RISING:
> + latch = false;
> + active_high = true;
> + break;
> + case IRQF_TRIGGER_HIGH:
> + latch = true;
> + active_high = true;
> + break;
> + case IRQF_TRIGGER_FALLING:
> + latch = false;
> + active_high = false;
> + break;
> + case IRQF_TRIGGER_LOW:
> + latch = true;
> + active_high = false;
> + break;
> + default:
> + dev_err(data->dev, "Invalid interrupt type 0x%x specified\n",
> + irq_type);
> + return -EINVAL;
Here and above, why not dev_err_probe() as you used it already below?
> + }
...
> + if (en) {
> + ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
> + 0x012c);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
> + BMI323_FEAT_IO_STATUS_MSK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
> + BMI323_FEAT_ENG_EN_MSK);
> + if (ret)
> + return ret;
> + i = 5;
> + do {
> + ret = regmap_read(data->regmap, BMI323_FEAT_IO1_REG,
> + &feature_status);
> + if (ret)
> + return ret;
> +
> + i--;
> + mdelay(2);
> + } while (feature_status != 0x01 && i);
NIH regmap_read_poll_timeout().
> + if (feature_status != 0x01) {
> + dev_err(data->dev, "Failed to enable feature engine\n");
> + return -EINVAL;
> + }
> +
> + return ret;
> + } else {
Redundant. But here it's okay to leave (I can understand the justification).
> + return regmap_write(data->regmap, BMI323_FEAT_CTRL_REG, 0);
> + }
...
> + if ((val & 0xFF) != BMI323_CHIP_ID_VAL) {
GENMASK() ? (BIT(x) - 1) ? A defined constant?
> + dev_err(data->dev, "Chip ID mismatch\n");
> + return -EINVAL;
Why not dev_err_probe() in this entire function?
> + }
...
> + ret = devm_add_action_or_reset(data->dev, bmi323_disable, data);
> + if (ret)
> + return ret;
> +
> + return 0;
return devm_...
...
> + regmap = dev_get_regmap(dev, NULL);
> + if (!regmap) {
> + dev_err(dev, "No regmap\n");
> + return -EINVAL;
Why not dev_err_probe()?
> + }
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
...
> +static int bmi323_regmap_i2c_read(void *context, const void *reg_buf,
> + size_t reg_size, void *val_buf,
> + size_t val_size)
> +{
> + struct device *dev = context;
> + struct i2c_client *i2c = to_i2c_client(dev);
> + struct i2c_msg msgs[2];
> + u8 *buff = NULL;
Redundant assignment.
> + int ret;
> +
> + buff = kmalloc(val_size + BMI323_I2C_DUMMY, GFP_KERNEL);
size_add() and include overflow.h for it.
> + if (!buff)
> + return -ENOMEM;
> +
> + msgs[0].addr = i2c->addr;
> + msgs[0].flags = i2c->flags;
> + msgs[0].len = reg_size;
> + msgs[0].buf = (u8 *)reg_buf;
> +
> + msgs[1].addr = i2c->addr;
> + msgs[1].len = val_size + BMI323_I2C_DUMMY;
> + msgs[1].buf = (u8 *)buff;
> + msgs[1].flags = i2c->flags | I2C_M_RD;
> +
> + ret = i2c_transfer(i2c->adapter, msgs, ARRAY_SIZE(msgs));
> + if (ret < 0) {
> + kfree(buff);
With cleanup.h this will look better.
> + return -EIO;
> + }
> +
> + memcpy(val_buf, buff + BMI323_I2C_DUMMY, val_size);
> + kfree(buff);
> +
> + return 0;
> +}
...
> +static int bmi323_regmap_i2c_write(void *context, const void *data,
> + size_t count)
> +{
> + struct device *dev = context;
> + struct i2c_client *i2c = to_i2c_client(dev);
> + int ret;
> + u8 reg;
> +
> + reg = *(u8 *)data;
> + ret = i2c_smbus_write_i2c_block_data(i2c, reg, count - 1,
> + data + sizeof(u8));
> +
> + return ret;
> +}
Hmm... Don't we have a better approach for these? regmap doesn't provide SMBus
accessors?
...
> +static const struct i2c_device_id bmi323_i2c_ids[] = {
> + { "bmi323", 0 },
', 0' is redundant
> + { }
> +};
...
> +static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
> + size_t reg_size, void *val_buf,
> + size_t val_size)
> +{
> + struct spi_device *spi = context;
> + u8 reg, *buff = NULL;
> + int ret;
> +
> + buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);
As per i2c part.
> + if (!buff)
> + return -ENOMEM;
> +
> + reg = *(u8 *)reg_buf | 0x80;
Doesn't regmap configuration provide a way to set this?
> + ret = spi_write_then_read(spi, ®, sizeof(reg), buff,
> + val_size + BMI323_SPI_DUMMY);
> + if (ret) {
> + kfree(buff);
> + return ret;
> + }
> +
> + memcpy(val_buf, buff + BMI323_SPI_DUMMY, val_size);
> + kfree(buff);
> +
> + return ret;
> +}
...
> + regmap = devm_regmap_init(dev, &bmi323_regmap_bus, dev,
> + &bmi323_regmap_config);
> +
Redundant blank line.
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to initialize SPI Regmap\n");
> +
...
> +static const struct spi_device_id bmi323_spi_ids[] = {
> + { "bmi323", 0 },
As per above.
> + { }
> +};
--
With Best Regards,
Andy Shevchenko