Messages in this thread | | | Date | Mon, 18 Sep 2023 13:03:54 +0300 | From | Andy Shevchenko <> | Subject | Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU |
| |
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
| |