| Message ID | 20230918080314.11959-1-jagathjog1996@gmail.com |
|---|---|
| Headers | show |
| Series | iio: imu: Add driver and dt-bindings for BMI323 | expand |
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. > + { } > +};
Hi Andy, Thank you for reviewing. On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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. Sure I will correct these in the next series. Please note that I omitted certain portions of your reviews while responding, and I fully agree with the comments that I didn't address. I intend to make the necessary corrections in the next series. .... > > +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? The arrays odrns[] and odrhz[] are used to store the ODR in nanoseconds and frequency for both the accelerometer and gyro. Instead of the magic number 2, I will define an enum. For steps_count[] array is of size 2 words and I will define a separate macro. > ... > > > +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. Sure, thanks for pointing this I will go through cleanup.h. If required I will get back with some questions. > > > + 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; > > +} > ... > > 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 Sorry I didn't get this, I am updating field_value based on state value. What is the purpose of 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)? iio_format_value() expects int* so I used int. > > > + 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() Okay, I will use ARRAY_SIZE() instead of number. > > > +static const struct attribute_group bmi323_event_attribute_group = { > > + .attrs = bmi323_event_attributes, > > +}; > > ATTRIBUTE_GROUPS() ? Okay, I will use ATTRIBUTE_GROUPS. > > ... > > > + state = data->state == BMI323_BUFFER_FIFO ? true : false; > > == already results in boolean type. Sure I will directly assign the result of comparison. state = (data->state == BMI323_BUFFER_FIFO); > ... > > > + int ret, raw; > > Why raw is signed? I don't have a specific reason for this; however, since it stores register value, it should be unsigned. I will make this correction in the next series > > > + 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; > I missed this, > is not possible, I should have used while() here also, I will correct this in the next series. > > + 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. Is the ternary operator not permitted to use? > > > + 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? To match the regmap_read() val parameter I used unsigned int*. Note: each sensor register values are 16 bit wide and regmap is configured with .val_bits = 16. > > + > > + 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; Sure, this can also be used. I will update this > > ... > > > + int odr_raw, ret; > > Why odr_raw is signed? In the below conditions, I am checking for -ve value so 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? Sure, for other case, I will use a while loop instead of a for loop. > > > + 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? Right, fwnode_irq_get_byname won't return 0, I will correct this in the next series. > > > + 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(). Okay. > > > + 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? Okay I will make use of dev_err_probe() here and in all probe paths. > > > + 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()? There was no int return value from dev_get_regmap(), I think I can use -EINVAL for err in dev_err_probe as well. > > > + } > > > > +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? Custom implementation is required for the 'read' operation, while 'write' can utilize the regmap SMBus accessors. Is it okay to have only custom read while write uses the SMBus accessor? > > > +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? Okay, I will make use of regmap .read_flag_mask member. I will update this in the next series. > > > + 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; > > +} > > -- > With Best Regards, > Andy Shevchenko > > Thank you Jagath
On Wed, Sep 20, 2023 at 04:13:51AM +0530, Jagath Jog J wrote: > On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote: ... > > 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 > > Sorry I didn't get this, I am updating field_value based on state value. > What is the purpose of state_value? Something like this I meant: unsigned int state_value = state ? GENMASK(2, 0) : 0; ... switch (dir) { case IIO_EV_DIR_RISING: ... set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_MOTION_MSK, FIELD_PREP(BMI323_FEAT_IO0_XYZ_MOTION_MSK, 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; > > > + } ... > > > + 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. > > Is the ternary operator not permitted to use? Yes, it's permitted. My point that you can calculate once the value unsigned int value = val ? 1 : 0; and use it everywhere where it makes sense. ... > > > +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? > > To match the regmap_read() val parameter I used unsigned int*. > > Note: each sensor register values are 16 bit wide > and regmap is configured with .val_bits = 16. > > > + 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. This is an interesting case. Your regmap configuration tells about endianess of the accessors. Default IIRC is defined either by bus or by driver itself. That said, since you are not using _raw variants of the accessors, the above will give you a wrong result on BE. Hence *val = sign_extend32(&value), 15); should be enough. (Note, the _raw variants take void pointer on purpose.) ... > > > + regmap = dev_get_regmap(dev, NULL); > > > + if (!regmap) { > > > + dev_err(dev, "No regmap\n"); > > > + return -EINVAL; > > > > Why not dev_err_probe()? > > There was no int return value from dev_get_regmap(), > I think I can use -EINVAL for err in dev_err_probe as well. Yes, it's fine. > > > + } ... > > > +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? > > Custom implementation is required for the 'read' operation, while > 'write' can utilize the regmap SMBus accessors. Is it okay to have > only custom read while write uses the SMBus accessor? Yes, why not, but I don't know if regmap API allows this.
On Mon, 18 Sep 2023 13:33:14 +0530 Jagath Jog J <jagathjog1996@gmail.com> 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 > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> Given Andy has done a thorough review (as he always does!) and there is quite a bit in my review queue, I'll just scan through quickly and call out a few things. Will take a closer look at next version. One thing that is useful for a complex driver like this is to include (typically in the cover letter) a full listing of what shows up in sysfs. That gives an easy way to check the ABI looks right without having to figure out what all the generated file names are from the code. Thanks, Jonathan > --- > MAINTAINERS | 7 + > drivers/iio/imu/Kconfig | 1 + > drivers/iio/imu/Makefile | 1 + > drivers/iio/imu/bmi323/Kconfig | 33 + > drivers/iio/imu/bmi323/Makefile | 7 + > drivers/iio/imu/bmi323/bmi323.h | 198 +++ > drivers/iio/imu/bmi323/bmi323_core.c | 2260 ++++++++++++++++++++++++++ > drivers/iio/imu/bmi323/bmi323_i2c.c | 115 ++ > drivers/iio/imu/bmi323/bmi323_spi.c | 106 ++ > 9 files changed, 2728 insertions(+) > create mode 100644 drivers/iio/imu/bmi323/Kconfig > create mode 100644 drivers/iio/imu/bmi323/Makefile > create mode 100644 drivers/iio/imu/bmi323/bmi323.h > create mode 100644 drivers/iio/imu/bmi323/bmi323_core.c > create mode 100644 drivers/iio/imu/bmi323/bmi323_i2c.c > create mode 100644 drivers/iio/imu/bmi323/bmi323_spi.c > > diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c > new file mode 100644 > index 000000000000..adbcfbc6b97d > --- /dev/null > +++ b/drivers/iio/imu/bmi323/bmi323_core.c > @@ -0,0 +1,2260 @@ > +struct bmi323_data { > + struct device *dev; > + struct regmap *regmap; > + struct iio_mount_matrix orientation; > + enum bmi323_irq_pin irq_pin; > + struct iio_trigger *trig; > + bool drdy_trigger_enabled; > + enum bmi323_state state; > + s64 fifo_tstamp, old_fifo_tstamp; > + u32 odrns[2]; > + u32 odrhz[2]; > + unsigned int feature_events; > + > + /* > + * Lock to protect the members of device's private data from concurrent > + * access and also to serialize the access of extended registers. > + * See bmi323_write_ext_reg(..) for more info. > + */ > + struct mutex mutex; > + int watermark; > + __le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN); > + struct { > + __le16 channels[6]; > + s64 ts __aligned(8); Hopefully Andy's aligned_s64 set will land soon and we can tidy this up. I'm a bit unsure of this, but can you overlap some of these buffers or are they used concurrently? (if they are then we have problems with DMA safety.) Perhaps an anonymous union is appropriate? > + } buffer; > + __le16 steps_count[2]; > +}; > +static int bmi323_buffer_preenable(struct iio_dev *indio_dev) > +{ > + struct bmi323_data *data = iio_priv(indio_dev); > + int ret = 0; > + > + mutex_lock(&data->mutex); > + /* > + * When the ODR of the accelerometer and gyroscope do not match, the > + * maximum ODR value between the accelerometer and gyroscope is used > + * for FIFO and the signal with lower ODR will insert dummy frame. yuk. > + * So allow buffer read only when ODR's of accelero and gyro are equal. > + * See datasheet section 5.7 "FIFO Data Buffering". Good :) > + */ > + if (data->odrns[BMI323_ACCEL] != data->odrns[BMI323_GYRO]) { > + dev_err(data->dev, "Accelero and Gyro ODR doesn't match\n"); > + ret = -EINVAL; > + } > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static int bmi323_set_drdy_irq(struct bmi323_data *data, > + enum bmi323_irq_pin irq_pin) > +{ > + int ret; > + > + ret = regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG, > + BMI323_GYR_DRDY_MSK, > + FIELD_PREP(BMI323_GYR_DRDY_MSK, irq_pin)); > + if (ret) > + return ret; > + > + return regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG, > + BMI323_ACC_DRDY_MSK, > + FIELD_PREP(BMI323_ACC_DRDY_MSK, irq_pin)); > +} > + > +static int bmi323_data_rdy_trigger_set_state(struct iio_trigger *trig, > + bool state) > +{ > + struct bmi323_data *data = iio_trigger_get_drvdata(trig); > + enum bmi323_irq_pin irq_pin; > + int ret; > + > + mutex_lock(&data->mutex); guard(mutex)(&data->mutex); > + > + if (data->state == BMI323_BUFFER_FIFO) { > + dev_warn(data->dev, "Can't set trigger when FIFO enabled\n"); > + ret = -EBUSY; > + goto unlock_out; > + } > + > + if (state) { > + data->state = BMI323_BUFFER_DRDY_TRIGGERED; > + irq_pin = data->irq_pin; > + } else { > + data->state = BMI323_IDLE; > + irq_pin = BMI323_IRQ_DISABLED; > + } > + > + ret = bmi323_set_drdy_irq(data, irq_pin); > + > +unlock_out: > + mutex_unlock(&data->mutex); > + return ret; > +} > + > +static const struct iio_trigger_ops bmi323_trigger_ops = { > + .set_trigger_state = &bmi323_data_rdy_trigger_set_state, > +}; > + > +static irqreturn_t bmi323_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bmi323_data *data = iio_priv(indio_dev); > + int ret; > + > + /* Lock to protect the data->buffer */ > + mutex_lock(&data->mutex); scoped_guard(mutex)(&data->mutex); might work well here though it will push up the indent. > + ret = regmap_bulk_read(data->regmap, BMI323_ACCEL_X_REG, > + &data->buffer.channels, > + ARRAY_SIZE(data->buffer.channels)); > + if (ret) > + goto unlock_out; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > + iio_get_time_ns(indio_dev)); > + mutex_unlock(&data->mutex); > + iio_trigger_notify_done(indio_dev->trig); > + return IRQ_HANDLED; > + > +unlock_out: > + mutex_unlock(&data->mutex); > + return IRQ_NONE; > +} > + > +static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0); > +static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64"); > + > +static struct attribute *bmi323_attributes[] = { > + &iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr, > + &iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr, So averaging often maps directly to oversampling. Kind of different names for the same thing. Perhaps that standard ABI can be used? It tends to make sampling frequency reporting need to take it into account though as that drops as divided by oversampling ratio. > + NULL > +}; > + > +} > + > +static int bmi323_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + struct bmi323_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + mutex_lock(&data->mutex); > + ret = bmi323_set_odr(data, bmi323_iio_to_sensor(chan->type), > + val, val2); > + mutex_unlock(&data->mutex); > + iio_device_release_direct_mode(indio_dev); > + return ret; > + case IIO_CHAN_INFO_SCALE: > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + mutex_lock(&data->mutex); > + ret = bmi323_set_scale(data, bmi323_iio_to_sensor(chan->type), > + val, val2); > + mutex_unlock(&data->mutex); > + iio_device_release_direct_mode(indio_dev); > + return ret; > + case IIO_CHAN_INFO_ENABLE: > + mutex_lock(&data->mutex); > + ret = bmi323_enable_steps(data, val); > + mutex_unlock(&data->mutex); > + return ret; > + case IIO_CHAN_INFO_PROCESSED: > + if (val || !FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK, > + data->feature_events)) > + return -EINVAL; > + > + /* Clear step counter value */ > + return bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG, > + BMI323_STEP_SC1_RST_CNT_MSK, > + FIELD_PREP(BMI323_STEP_SC1_RST_CNT_MSK, > + 1)); > + default: > + return -EINVAL; > + } > +} > + > +static int bmi323_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct bmi323_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + mutex_lock(&data->mutex); scoped_guard() might be mice for some of these cases or push the guards down into the function being called perhaps. > + ret = bmi323_read_steps(data, val); > + mutex_unlock(&data->mutex); > + return ret; > + case IIO_CHAN_INFO_RAW: > + switch (chan->type) { > + case IIO_ACCEL: > + case IIO_ANGL_VEL: > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + mutex_lock(&data->mutex); > + ret = bmi323_read_axis(data, chan, val); > + mutex_unlock(&data->mutex); > + > + iio_device_release_direct_mode(indio_dev); > + return ret; > + case IIO_TEMP: > + mutex_lock(&data->mutex); > + ret = bmi323_get_temp_data(data, val); > + mutex_unlock(&data->mutex); > + return ret; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_SAMP_FREQ: > + mutex_lock(&data->mutex); > + ret = bmi323_get_odr(data, chan->type, val, val2); > + mutex_unlock(&data->mutex); > + return ret; > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { > + case IIO_ACCEL: > + case IIO_ANGL_VEL: > + *val = 0; > + mutex_lock(&data->mutex); > + ret = bmi323_get_scale(data, chan->type, val2); > + mutex_unlock(&data->mutex); > + return ret; > + case IIO_TEMP: > + *val = BMI323_TEMP_SCALE / MEGA; > + *val2 = BMI323_TEMP_SCALE % MEGA; > + return IIO_VAL_INT_PLUS_MICRO; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_OFFSET: > + switch (chan->type) { > + case IIO_TEMP: > + *val = BMI323_TEMP_OFFSET; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_ENABLE: > + mutex_lock(&data->mutex); > + *val = FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK, > + data->feature_events); > + mutex_unlock(&data->mutex); > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info bmi323_info = { > + .read_raw = bmi323_read_raw, > + .write_raw = bmi323_write_raw, > + .read_avail = bmi323_read_avail, > + .attrs = &bmi323_attrs_group, > + .hwfifo_set_watermark = bmi323_set_watermark, > + .write_event_config = bmi323_write_event_config, > + .read_event_config = bmi323_read_event_config, > + .write_event_value = bmi323_write_event_value, > + .read_event_value = bmi323_read_event_value, > + .event_attrs = &bmi323_event_attribute_group, > +}; > + > +#define BMI323_SCAN_MASK_ACCEL_3AXIS \ > + (BIT(BMI323_ACCEL_X) | BIT(BMI323_ACCEL_Y) | BIT(BMI323_ACCEL_Z)) > + > +#define BMI323_SCAN_MASK_GYRO_3AXIS \ > + (BIT(BMI323_GYRO_X) | BIT(BMI323_GYRO_Y) | BIT(BMI323_GYRO_Z)) > + > +static const unsigned long bmi323_avail_scan_masks[] = { > + /* 3-axis accel + 3-axis gyro */ > + BMI323_SCAN_MASK_ACCEL_3AXIS | BMI323_SCAN_MASK_GYRO_3AXIS, Can you poke this an see if you get what you expect which is the minimum sufficient set of channels. Matti pointed out earlier that the search logic isn't well documented in iio_scan_mask_match() but it looks to match against first case where the requested values are a subset. So this would need to be in the opposite order or you will always get everything turned on. Chances are we have this wrong in other drivers as well :( Won't break things, but may mean that we over read in some configurations. > + /* 3-axis accel */ > + BMI323_SCAN_MASK_ACCEL_3AXIS, > + /* 3-axis gyro */ > + BMI323_SCAN_MASK_GYRO_3AXIS, > + 0 > +}; > + > +static int bmi323_int_pin_config(struct bmi323_data *data, > + enum bmi323_irq_pin irq_pin, > + bool active_high, bool open_drain, bool latch) > +{ > + unsigned int mask, field_value = 0; > + int ret; > + > + switch (irq_pin) { > + case BMI323_IRQ_INT1: > + mask = BMI323_IO_INT1_LVL_OD_OP_MSK; > + > + set_mask_bits(&field_value, BMI323_IO_INT1_LVL_MSK, > + FIELD_PREP(BMI323_IO_INT1_LVL_MSK, active_high)); > + > + set_mask_bits(&field_value, BMI323_IO_INT1_OD_MSK, > + FIELD_PREP(BMI323_IO_INT1_OD_MSK, open_drain)); > + > + set_mask_bits(&field_value, BMI323_IO_INT1_OP_EN_MSK, > + FIELD_PREP(BMI323_IO_INT1_OP_EN_MSK, 1)); Given you are filling in a value that starts as zero and don't overwrite values you set earlier this is much simpler as mask = ... field_value = FIELD_PREP(...) | FIELD_PREP(...) | FIELD_PREP(...); Also move working this out to just before it is used. Right now it is hard to spot where that is. > + break; > + case BMI323_IRQ_INT2: > + mask = BMI323_IO_INT2_LVL_OD_OP_MSK; > + > + set_mask_bits(&field_value, BMI323_IO_INT2_LVL_MSK, > + FIELD_PREP(BMI323_IO_INT2_LVL_MSK, active_high)); > + > + set_mask_bits(&field_value, BMI323_IO_INT2_OD_MSK, > + FIELD_PREP(BMI323_IO_INT2_OD_MSK, open_drain)); > + > + set_mask_bits(&field_value, BMI323_IO_INT2_OP_EN_MSK, > + FIELD_PREP(BMI323_IO_INT2_OP_EN_MSK, 1)); > + break; > + default: > + return -EINVAL; > + } > + > + ret = regmap_update_bits(data->regmap, BMI323_IO_INT_CONF_REG, > + BMI323_IO_INT_LTCH_MSK, > + FIELD_PREP(BMI323_IO_INT_LTCH_MSK, latch)); > + if (ret) > + return ret; > + > + ret = bmi323_update_ext_reg(data, BMI323_GEN_SET1_REG, > + BMI323_GEN_HOLD_DUR_MSK, > + FIELD_PREP(BMI323_GEN_HOLD_DUR_MSK, 0)); > + if (ret) > + return ret; > + > + return regmap_update_bits(data->regmap, BMI323_IO_INT_CTR_REG, mask, > + field_value); > +} > + > +static int bmi323_trigger_probe(struct bmi323_data *data, > + struct iio_dev *indio_dev) > +{ > + bool open_drain, active_high, latch; > + struct fwnode_handle *fwnode; > + enum bmi323_irq_pin irq_pin; > + int ret, irq, irq_type; > + struct irq_data *desc; > + > + 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) > + return 0; > + > + irq_pin = BMI323_IRQ_INT2; > + } > + > + desc = irq_get_irq_data(irq); > + if (!desc) { > + dev_err(data->dev, "Could not find IRQ %d\n", irq); > + return -EINVAL; > + } > + > + irq_type = irqd_get_trigger_type(desc); > + > + 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; > + } > + > + open_drain = fwnode_property_read_bool(fwnode, "drive-open-drain"); > + > + ret = bmi323_int_pin_config(data, irq_pin, active_high, open_drain, > + latch); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "Failed to configure irq line\n"); > + > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-trig-%d", > + indio_dev->name, irq_pin); > + if (!data->trig) > + return -ENOMEM; > + > + data->trig->ops = &bmi323_trigger_ops; > + iio_trigger_set_drvdata(data->trig, data); > + > + ret = devm_request_threaded_irq(data->dev, irq, NULL, > + bmi323_irq_thread_handler, > + irq_type | IRQF_ONESHOT, I think if you leave the irq_type bit out, it will be set up to match what was specified in firmware anyway. Could be wrong on that though so check. > + "bmi323-int", indio_dev); > + if (ret) > + return dev_err_probe(data->dev, ret, "Failed to request IRQ\n"); > + > + ret = devm_iio_trigger_register(data->dev, data->trig); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "Trigger registration failed\n"); > + > + data->irq_pin = irq_pin; > + > + return ret; > +} > + > +static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en) > +{ > + unsigned int feature_status; > + int ret, i; > + > + 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; Why 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); GET_FIELD() always rather than directly masking out values It means we have a clear name for what is being checked. > + > + if (feature_status != 0x01) { > + dev_err(data->dev, "Failed to enable feature engine\n"); > + return -EINVAL; > + } > + > + return ret; > + } else { > + return regmap_write(data->regmap, BMI323_FEAT_CTRL_REG, 0); Flip the logic for readability by reducing indent if (!en) return regmap_write() ret = regmap_write() etc > + } > +} > +static int bmi323_init(struct bmi323_data *data) > +{ > + int ret, val; > + > + /* > + * Perform soft reset to make sure the device is in a known state after > + * start up. A delay of 1.5 ms is required after reset. > + * See datasheet section 5.17 "Soft Reset". > + */ > + ret = regmap_write(data->regmap, BMI323_CMD_REG, BMI323_RST_VAL); > + if (ret) > + return ret; > + > + usleep_range(1500, 2000); > + > + /* > + * Dummy read is required to enable SPI interface after reset. > + * See datasheet section 7.2.1 "Protocol Selection". > + */ > + regmap_read(data->regmap, BMI323_CHIP_ID_REG, &val); > + > + ret = regmap_read(data->regmap, BMI323_CHIP_ID_REG, &val); > + if (ret) > + return ret; > + > + if ((val & 0xFF) != BMI323_CHIP_ID_VAL) { FIELD_GET() and appropriate mask given this is half of a 16 bit register. > + dev_err(data->dev, "Chip ID mismatch\n"); > + return -EINVAL; > + } > + > + ret = bmi323_feature_engine_enable(data, true); > + if (ret) > + return ret; > + > + ret = regmap_read(data->regmap, BMI323_ERR_REG, &val); > + if (ret) > + return ret; > + > + if (val) { > + dev_err(data->dev, "Sensor power error = 0x%x\n", val); > + return -EINVAL; I think this is only called in probe() so an use dev_err_probe() in here as well. > + } > + > + ret = regmap_read(data->regmap, BMI323_STATUS_REG, &val); > + if (ret) > + return ret; > + > + if (val != 0x01) { FIELD_GET() and appropriate definition. > + dev_err(data->dev, "Sensor initialization error = 0x%x\n", val); > + return -EINVAL; > + } > + > + /* > + * Set the Bandwidth coefficient which defines the 3 dB cutoff > + * frequency in relation to the ODR. > + */ > + ret = bmi323_set_bw(data, BMI323_ACCEL, BMI323_BW_ODR_BY_2); > + if (ret) > + return -EINVAL; > + > + ret = bmi323_set_bw(data, BMI323_GYRO, BMI323_BW_ODR_BY_2); > + if (ret) > + return -EINVAL; > + > + ret = bmi323_set_odr(data, BMI323_ACCEL, 25, 0); > + if (ret) > + return -EINVAL; > + > + ret = bmi323_set_odr(data, BMI323_GYRO, 25, 0); > + if (ret) > + return -EINVAL; > + > + ret = devm_add_action_or_reset(data->dev, bmi323_disable, data); > + if (ret) > + return ret; > + > + return 0; > +} > + > +int bmi323_core_probe(struct device *dev) > +{ > + static const char * const regulator_names[] = { "vdd", "vddio" }; > + struct iio_dev *indio_dev; > + struct bmi323_data *data; > + struct regmap *regmap; > + int ret; > + > + regmap = dev_get_regmap(dev, NULL); > + if (!regmap) { > + dev_err(dev, "No regmap\n"); > + return -EINVAL; > + } > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names), > + regulator_names); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable regulators\n"); > + > + data = iio_priv(indio_dev); > + data->dev = dev; > + data->regmap = regmap; > + mutex_init(&data->mutex); > + > + ret = bmi323_init(data); > + if (ret) > + return -EINVAL; > + > + ret = iio_read_mount_matrix(dev, &data->orientation); > + if (ret) > + return ret; > + > + indio_dev->name = "bmi323-imu"; > + indio_dev->info = &bmi323_info; > + indio_dev->channels = bmi323_channels; > + indio_dev->num_channels = ARRAY_SIZE(bmi323_channels); > + indio_dev->available_scan_masks = bmi323_avail_scan_masks; > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; > + dev_set_drvdata(data->dev, indio_dev); > + > + ret = bmi323_trigger_probe(data, indio_dev); What if interrupt isn't wired? Do we need it for basic read of channels? Would expect the interfaces provided to be more limited, but not that we would provide none at all. > + if (ret) > + return -EINVAL; > + > + ret = devm_iio_triggered_buffer_setup_ext(data->dev, indio_dev, > + &iio_pollfunc_store_time, > + bmi323_trigger_handler, > + IIO_BUFFER_DIRECTION_IN, > + &bmi323_buffer_ops, > + bmi323_fifo_attributes); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "Failed to setup trigger buffer\n"); > + > + ret = devm_iio_device_register(data->dev, indio_dev); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "Unable to register iio device\n"); > + > + return ret; > +} > +EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323); > + > +MODULE_DESCRIPTION("Bosch BMI323 IMU driver"); > +MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c > new file mode 100644 > index 000000000000..afbaa3d3c638 > --- /dev/null > +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c > @@ -0,0 +1,115 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * I2C driver for Bosch BMI323 6-Axis IMU. > + * > + * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com> > + */ > + > +#include <linux/i2c.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > + > +#include "bmi323.h" > + > +/* > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support. > + * Each I2C register read operation requires to read two dummy bytes before > + * the actual payload. > + */ > +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; > + int ret; > + > + buff = kmalloc(val_size + BMI323_I2C_DUMMY, GFP_KERNEL); > + 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); > + return -EIO; > + } > + > + memcpy(val_buf, buff + BMI323_I2C_DUMMY, val_size); Annoyingly can't do same trick as I suggest for SPI as we need the address send vs when data turns up to be correct. Whilst this code is 'generic' do you know the max size of the buffer that might be read? If it's small I'd just use an array on the stack. If large, then the cleanup.h stuff will help with code, but it's still annoying to have to do an allocation in here. You can probably put something in context alongside dev. > + 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 i2c_smbus... > + return ret; > +} > + > +static struct regmap_bus bmi323_regmap_bus = { > + .read = bmi323_regmap_i2c_read, > + .write = bmi323_regmap_i2c_write, > +}; > + > +static int bmi323_i2c_probe(struct i2c_client *i2c) > +{ > + struct device *dev = &i2c->dev; > + struct regmap *regmap; > + > + regmap = devm_regmap_init(dev, &bmi323_regmap_bus, dev, > + &bmi323_regmap_config); > + if (IS_ERR(regmap)) > + return dev_err_probe(dev, PTR_ERR(regmap), > + "Failed to initialize I2C Regmap\n"); > + > + return bmi323_core_probe(dev); > +} > + > +static const struct i2c_device_id bmi323_i2c_ids[] = { > + { "bmi323", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, bmi323_i2c_ids); > + > +static const struct of_device_id bmi323_of_i2c_match[] = { > + { .compatible = "bosch,bmi323" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, bmi323_of_i2c_match); > + > +static struct i2c_driver bmi323_i2c_driver = { > + .driver = { > + .name = "bmi323", > + .of_match_table = bmi323_of_i2c_match, > + }, > + .probe_new = bmi323_i2c_probe, > + .id_table = bmi323_i2c_ids, > +}; > +module_i2c_driver(bmi323_i2c_driver); > + > +MODULE_DESCRIPTION("Bosch BMI323 IMU driver"); > +MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(IIO_BMI323); > diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c > new file mode 100644 > index 000000000000..2b802a0c6d9d > --- /dev/null > +++ b/drivers/iio/imu/bmi323/bmi323_spi.c > @@ -0,0 +1,106 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * SPI driver for Bosch BMI323 6-Axis IMU. > + * > + * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com> > + */ > + > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/spi/spi.h> > + > +#include "bmi323.h" > + > +/* > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support. > + * Each SPI register read operation requires to read one dummy byte before > + * the actual payload. > + */ > +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); Hmm. Regmap has pad_bits (which can be multiple bytes) but this case is unusual in that they only apply to reads. I wonder if we can make this cheaper though rather than having to handle either some context or having dynamic allocations in here. How about making the write bigger? Does that have any effect? Looks like don't care state in Figure 31. If that's the case, send some zeros on that as it's known fixed size (2 bytes including the padding) and then you can directly use the read buffer without yet another memcpy. > + if (!buff) > + return -ENOMEM; > + > + reg = *(u8 *)reg_buf | 0x80; > + 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; > +} > + > +static int bmi323_regmap_spi_write(void *context, const void *data, > + size_t count) > +{ > + struct spi_device *spi = context; > + > + return spi_write(spi, data, count); > +} > + > +static struct regmap_bus bmi323_regmap_bus = { > + .read = bmi323_regmap_spi_read, > + .write = bmi323_regmap_spi_write, > +};
Hello, 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 > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf Maybe put this link better in the driver. > +static struct i2c_driver bmi323_i2c_driver = { > + .driver = { > + .name = "bmi323", > + .of_match_table = bmi323_of_i2c_match, > + }, > + .probe_new = bmi323_i2c_probe, > + .id_table = bmi323_i2c_ids, > +}; > +module_i2c_driver(bmi323_i2c_driver); If you want to compile this driver after v6.6-rc2 (which includes commit 5eb1e6e459cf ("i2c: Drop legacy callback .probe_new()")) better use .probe here instead of .probe_new(). Best regards Uwe
On Wed, Sep 27, 2023 at 11:57:08AM +0200, Uwe Kleine-König wrote: > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote: ... > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf > > Maybe put this link better in the driver. Why? We have a handful commits with this and it's better to see the link to the datasheet without browsing the source code.
On Wed, Sep 27, 2023 at 03:35:02PM +0300, Andy Shevchenko wrote: > On Wed, Sep 27, 2023 at 11:57:08AM +0200, Uwe Kleine-König wrote: > > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote: > > ... > > > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf > > > > Maybe put this link better in the driver. > > Why? We have a handful commits with this and it's better to see the link > to the datasheet without browsing the source code. But if you later work on a problem in the driver, it's better to see the link without browsing git history. :-) Best regards Uwe
Hi Jonathan, Thank you for the review. On Sun, Sep 24, 2023 at 8:01 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 18 Sep 2023 13:33:14 +0530 > Jagath Jog J <jagathjog1996@gmail.com> 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 > > > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> > > Given Andy has done a thorough review (as he always does!) > and there is quite a bit in my review queue, I'll just scan > through quickly and call out a few things. Will take a closer > look at next version. > > One thing that is useful for a complex driver like this is to include > (typically in the cover letter) a full listing of what shows up in sysfs. > That gives an easy way to check the ABI looks right without having to figure > out what all the generated file names are from the code. Sure, I will add all ABI's in the cover letter. Please note that I omitted certain portions of your reviews while responding, and I agree with the review comments that I didn't address. I intend to make the necessary corrections in the next series. > > Thanks, > > Jonathan > > > +struct bmi323_data { > > + struct device *dev; > > + struct regmap *regmap; > > + struct iio_mount_matrix orientation; > > + enum bmi323_irq_pin irq_pin; > > + struct iio_trigger *trig; > > + bool drdy_trigger_enabled; > > + enum bmi323_state state; > > + s64 fifo_tstamp, old_fifo_tstamp; > > + u32 odrns[2]; > > + u32 odrhz[2]; > > + unsigned int feature_events; > > + > > + /* > > + * Lock to protect the members of device's private data from concurrent > > + * access and also to serialize the access of extended registers. > > + * See bmi323_write_ext_reg(..) for more info. > > + */ > > + struct mutex mutex; > > + int watermark; > > + __le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN); > > + struct { > > + __le16 channels[6]; > > + s64 ts __aligned(8); > > Hopefully Andy's aligned_s64 set will land soon and we can tidy this up. > I'm a bit unsure of this, but can you overlap some of these buffers or are > they used concurrently? (if they are then we have problems with DMA safety.) > > Perhaps an anonymous union is appropriate? Yes both buffers are used at the same time. In fifo_flush fifo_buff is used to store all fifo data, and buffer is used to push a single data frame to iio buffers, overlapping will corrupt the data, so I used separate buffers for both. > > +static int bmi323_data_rdy_trigger_set_state(struct iio_trigger *trig, > > + bool state) > > +{ > > + struct bmi323_data *data = iio_trigger_get_drvdata(trig); > > + enum bmi323_irq_pin irq_pin; > > + int ret; > > + > > + mutex_lock(&data->mutex); > guard(mutex)(&data->mutex); Andy also suggested the same. Sure I will use helpers from cleanup.h. > > +static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0); > > +static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64"); > > + > > +static struct attribute *bmi323_attributes[] = { > > + &iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr, > > + &iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr, > > So averaging often maps directly to oversampling. Kind of different names > for the same thing. Perhaps that standard ABI can be used? > It tends to make sampling frequency reporting need to take it into account > though as that drops as divided by oversampling ratio. Yes, oversampling can be used, but changing the average value doesn't alter the sampling frequency. The sampling frequency is same even with the increase in averaging value. > > +#define BMI323_SCAN_MASK_ACCEL_3AXIS \ > > + (BIT(BMI323_ACCEL_X) | BIT(BMI323_ACCEL_Y) | BIT(BMI323_ACCEL_Z)) > > + > > +#define BMI323_SCAN_MASK_GYRO_3AXIS \ > > + (BIT(BMI323_GYRO_X) | BIT(BMI323_GYRO_Y) | BIT(BMI323_GYRO_Z)) > > + > > +static const unsigned long bmi323_avail_scan_masks[] = { > > + /* 3-axis accel + 3-axis gyro */ > > + BMI323_SCAN_MASK_ACCEL_3AXIS | BMI323_SCAN_MASK_GYRO_3AXIS, > > Can you poke this an see if you get what you expect which is the minimum > sufficient set of channels. Matti pointed out earlier that the search > logic isn't well documented in iio_scan_mask_match() but it > looks to match against first case where the requested values are a subset. > So this would need to be in the opposite order or you will always > get everything turned on. Sure, I will check and update you further on this. > > Chances are we have this wrong in other drivers as well :( > Won't break things, but may mean that we over read in some configurations. > > > + /* 3-axis accel */ > > + BMI323_SCAN_MASK_ACCEL_3AXIS, > > + /* 3-axis gyro */ > > + BMI323_SCAN_MASK_GYRO_3AXIS, > > + 0 > > +}; > > + > > + irq_type = irqd_get_trigger_type(desc); > > + > > + 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; > > + } > > + > > + open_drain = fwnode_property_read_bool(fwnode, "drive-open-drain"); > > + > > + ret = bmi323_int_pin_config(data, irq_pin, active_high, open_drain, > > + latch); > > + if (ret) > > + return dev_err_probe(data->dev, ret, > > + "Failed to configure irq line\n"); > > + > > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-trig-%d", > > + indio_dev->name, irq_pin); > > + if (!data->trig) > > + return -ENOMEM; > > + > > + data->trig->ops = &bmi323_trigger_ops; > > + iio_trigger_set_drvdata(data->trig, data); > > + > > + ret = devm_request_threaded_irq(data->dev, irq, NULL, > > + bmi323_irq_thread_handler, > > + irq_type | IRQF_ONESHOT, > > I think if you leave the irq_type bit out, it will be set up to match what was > specified in firmware anyway. Could be wrong on that though so check. Yes, irq_type is not required __setup_irq() is handling that. Thanks for pointing it out. > > + > > +static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en) > > +{ > > + unsigned int feature_status; > > + int ret, i; > > + > > + 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; > > Why 5? No specific reason, during testing the feature engine was taking around 4 milliseconds, so I thought of checking every 2 milliseconds and max of 5 trials. > > > + do { > > + ret = regmap_read(data->regmap, BMI323_FEAT_IO1_REG, > > + &feature_status); > > + if (ret) > > + return ret; > > + > > + i--; > > + mdelay(2); > > + } while (feature_status != 0x01 && i); > > + > > + indio_dev->name = "bmi323-imu"; > > + indio_dev->info = &bmi323_info; > > + indio_dev->channels = bmi323_channels; > > + indio_dev->num_channels = ARRAY_SIZE(bmi323_channels); > > + indio_dev->available_scan_masks = bmi323_avail_scan_masks; > > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; > > + dev_set_drvdata(data->dev, indio_dev); > > + > > + ret = bmi323_trigger_probe(data, indio_dev); > > What if interrupt isn't wired? Do we need it for basic read of channels? > Would expect the interfaces provided to be more limited, but not that we > would provide none at all. Yes, the basic read of channels will be available even if none of the interrupts are wired and trigger buffer through sysfs or hrt timer is also available. > > > + if (ret) > > + return -EINVAL; > > + > > + ret = devm_iio_triggered_buffer_setup_ext(data->dev, indio_dev, > > + &iio_pollfunc_store_time, > > + bmi323_trigger_handler, > > + IIO_BUFFER_DIRECTION_IN, > > + &bmi323_buffer_ops, > > + bmi323_fifo_attributes); > > + if (ret) > > + return dev_err_probe(data->dev, ret, > > + "Failed to setup trigger buffer\n"); > > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support. > > + * Each I2C register read operation requires to read two dummy bytes before > > + * the actual payload. > > + */ > > +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; > > + int ret; > > + > > + buff = kmalloc(val_size + BMI323_I2C_DUMMY, GFP_KERNEL); > > + 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); > > + return -EIO; > > + } > > + > > + memcpy(val_buf, buff + BMI323_I2C_DUMMY, val_size); > > Annoyingly can't do same trick as I suggest for SPI as we need > the address send vs when data turns up to be correct. > > Whilst this code is 'generic' do you know the max size of the > buffer that might be read? If it's small I'd just use an array > on the stack. If large, then the cleanup.h stuff will help > with code, but it's still annoying to have to do an allocation > in here. You can probably put something in context alongside > dev. A buffer size of 2028 bytes is required when configuring the FIFO watermark for maximum capacity. Since the necessary buffer size is substantial, I am allocating it dynamically. I will try to use an additional buffer from the device's private structure and pass it through the context. This approach will help reduce the memory allocation and deallocation on every device access. > > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support. > > + * Each SPI register read operation requires to read one dummy byte before > > + * the actual payload. > > + */ > > +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); > > Hmm. Regmap has pad_bits (which can be multiple bytes) but this case > is unusual in that they only apply to reads. > > I wonder if we can make this cheaper though rather than having > to handle either some context or having dynamic allocations in here. > > How about making the write bigger? Does that have any effect? > Looks like don't care state in Figure 31. If that's the case, > send some zeros on that as it's known fixed size (2 bytes including > the padding) and then you can directly use the read buffer without > yet another memcpy. For spi with pad_bits=8 and without any custom read and write functions, regmap_read() works but regmap_write() does not. Write is also adding 8 bits of padding and the device is treating it as data. (7.2.3 SPI Protocol Figure 30) > > > > + if (!buff) > > + return -ENOMEM; > > + > > + reg = *(u8 *)reg_buf | 0x80; > > + 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; Regards Jagath
Hello, Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe. The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check. I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor. What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore? TIA for your time. Best regards, Denis Benato
Hi Uwe Kleine-König, On Wed, Sep 27, 2023 at 3:27 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > 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 > > > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf > > Maybe put this link better in the driver. Yes, if there are multiple commits on the driver, the datasheet link will move further down with the initial commit. I will add datasheet link in the driver. > > > +static struct i2c_driver bmi323_i2c_driver = { > > + .driver = { > > + .name = "bmi323", > > + .of_match_table = bmi323_of_i2c_match, > > + }, > > + .probe_new = bmi323_i2c_probe, > > + .id_table = bmi323_i2c_ids, > > +}; > > +module_i2c_driver(bmi323_i2c_driver); > > If you want to compile this driver after v6.6-rc2 (which includes > commit 5eb1e6e459cf ("i2c: Drop legacy callback .probe_new()")) better > use .probe here instead of .probe_new(). Thanks for pointing it out. I switched to v6.6-rc3 and I will change to .probe. Regards Jagath Jagath > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Sep 28, 2023 at 11:49:01PM +0530, Jagath Jog J wrote: > Hi Uwe Kleine-König, > > On Wed, Sep 27, 2023 at 3:27 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Hello, > > > > 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 > > > > > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf > > > > Maybe put this link better in the driver. > > Yes, if there are multiple commits on the driver, the datasheet > link will move further down with the initial commit. I will add > datasheet link in the driver. > > > > > > +static struct i2c_driver bmi323_i2c_driver = { > > > + .driver = { > > > + .name = "bmi323", > > > + .of_match_table = bmi323_of_i2c_match, > > > + }, > > > + .probe_new = bmi323_i2c_probe, > > > + .id_table = bmi323_i2c_ids, > > > +}; > > > +module_i2c_driver(bmi323_i2c_driver); > > > > If you want to compile this driver after v6.6-rc2 (which includes > > commit 5eb1e6e459cf ("i2c: Drop legacy callback .probe_new()")) better > > use .probe here instead of .probe_new(). > > Thanks for pointing it out. > I switched to v6.6-rc3 and I will change to .probe. Note that you can use .probe (with that prototype) already since v6.3-rc2. So there is no hard requirement to go to v6.6-rc3. Best regards Uwe
Hi Denis, On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote: > > Hello, > > Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe. > > The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check. bmc150 driver supports multiple accelerometer sensors such as bma222, bma280, bmi055 and all of them are having similar register map, but the bmi323 register map is completely different from bmc150. > > I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor. > > What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore? > > TIA for your time. > > Best regards, > Denis Benato Regards Jagath
> > > > +struct bmi323_data { > > > + struct device *dev; > > > + struct regmap *regmap; > > > + struct iio_mount_matrix orientation; > > > + enum bmi323_irq_pin irq_pin; > > > + struct iio_trigger *trig; > > > + bool drdy_trigger_enabled; > > > + enum bmi323_state state; > > > + s64 fifo_tstamp, old_fifo_tstamp; > > > + u32 odrns[2]; > > > + u32 odrhz[2]; > > > + unsigned int feature_events; > > > + > > > + /* > > > + * Lock to protect the members of device's private data from concurrent > > > + * access and also to serialize the access of extended registers. > > > + * See bmi323_write_ext_reg(..) for more info. > > > + */ > > > + struct mutex mutex; > > > + int watermark; > > > + __le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN); > > > + struct { > > > + __le16 channels[6]; > > > + s64 ts __aligned(8); > > > > Hopefully Andy's aligned_s64 set will land soon and we can tidy this up. > > I'm a bit unsure of this, but can you overlap some of these buffers or are > > they used concurrently? (if they are then we have problems with DMA safety.) > > > > Perhaps an anonymous union is appropriate? > > Yes both buffers are used at the same time. In fifo_flush > fifo_buff is used to store all fifo data, and buffer is > used to push a single data frame to iio buffers, overlapping > will corrupt the data, so I used separate buffers for both. Ah. So the structure is used in 2 ways. 1. As a target for DMA, which means it should live in the cacheline we are saving for that purpsoe. 2. As a place to build up data. In general we should be careful with doing 2 as that could race with DMA and end up with data corruption, however you only use it that way in flush_fifo where both the DMA and this usage under under the mutex. Hence I think you are fine. > > > > +static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0); > > > +static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64"); > > > + > > > +static struct attribute *bmi323_attributes[] = { > > > + &iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr, > > > + &iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr, > > > > So averaging often maps directly to oversampling. Kind of different names > > for the same thing. Perhaps that standard ABI can be used? > > It tends to make sampling frequency reporting need to take it into account > > though as that drops as divided by oversampling ratio. > > Yes, oversampling can be used, but changing the average > value doesn't alter the sampling frequency. The sampling > frequency is same even with the increase in averaging value. Ok. That's unusual so good to know. > > > +static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en) > > > +{ > > > + unsigned int feature_status; > > > + int ret, i; > > > + > > > + 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; > > > > Why 5? > > No specific reason, during testing the feature engine was > taking around 4 milliseconds, so I thought of checking > every 2 milliseconds and max of 5 trials. That's a good reason. Just add a comment to that say that. > > > > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support. > > > + * Each SPI register read operation requires to read one dummy byte before > > > + * the actual payload. > > > + */ > > > +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); > > > > Hmm. Regmap has pad_bits (which can be multiple bytes) but this case > > is unusual in that they only apply to reads. > > > > I wonder if we can make this cheaper though rather than having > > to handle either some context or having dynamic allocations in here. > > > > How about making the write bigger? Does that have any effect? > > Looks like don't care state in Figure 31. If that's the case, > > send some zeros on that as it's known fixed size (2 bytes including > > the padding) and then you can directly use the read buffer without > > yet another memcpy. > > For spi with pad_bits=8 and without any custom read and > write functions, regmap_read() works but regmap_write() > does not. Write is also adding 8 bits of padding and > the device is treating it as data. > (7.2.3 SPI Protocol Figure 30) Understood. I looked it up too before suggesting this local hack. You'll still need a custom regmap, but at least this trick would allow you to avoid allocating a buffer in the read function. Jonathan
On Fri, 29 Sep 2023 13:29:13 +0530 Jagath Jog J <jagathjog1996@gmail.com> wrote: > Hi Denis, > > On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote: > > > > Hello, > > > > Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe. > > > > The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check. > > bmc150 driver supports multiple accelerometer sensors such as > bma222, bma280, bmi055 and all of them are having similar > register map, but the bmi323 register map is completely different > from bmc150. Horrible bios hacks that depend on a particular driver stack are always a pain. Hmm. Andy (handy ACPI expert), any suggestion? We could maybe do a wrapper driver that does appropriate checks and wraps the probe + remove from the two drivers? Whilst we can obviously have a single driver that deals with radically different devices I'm not particularly keen on that as it tends to make things less maintainable. Jonathan > > > > > > I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor. > > > > What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore? > > > > TIA for your time. > > > > Best regards, > > Denis Benato > > Regards > > Jagath
On Wed, Sep 27, 2023 at 04:34:43PM +0200, Uwe Kleine-König wrote: > On Wed, Sep 27, 2023 at 03:35:02PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 27, 2023 at 11:57:08AM +0200, Uwe Kleine-König wrote: > > > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote: ... > > > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf > > > > > > Maybe put this link better in the driver. > > > > Why? We have a handful commits with this and it's better to see the link > > to the datasheet without browsing the source code. > > But if you later work on a problem in the driver, it's better to see the > link without browsing git history. :-) Both make sense.
Hello Jagath, On 9/29/23 09:59, Jagath Jog J wrote: > Hi Denis, > > On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote: >> >> Hello, >> >> Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe. >> >> The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check. > > bmc150 driver supports multiple accelerometer sensors such as > bma222, bma280, bmi055 and all of them are having similar > register map, but the bmi323 register map is completely different > from bmc150. I apologize for the confusion. What I was trying to say is that to have the bmi323 working in those aforementioned devices bmc150 will need to be modified: that is the probe function that ends up being executed, fact that cannot be changed because it depends on the ACPI implementation shipped on those devices. Therefore I was asking about the best way of handing control to the new driver and how that should be organized: in my implementation the new bmi323 code was written inside the bmc150-accel-core.c and only shares sleep/suspend, probe and removal functions in addition to checking for the new chip presence before checking for any bmc150 chip as that issues an i2c write, while the check for the bmi323 only requires an i2c read. We also have done duplicate work as I have written a driver for that chip myself, but it's not as good as yours because my hardware didn't come with the IRQ pin connected and so I couldn't develop triggers and I only got the i2c interface working. > > >> >> I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor. >> >> What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore? >> >> TIA for your time. >> >> Best regards, >> Denis Benato > > Regards > > Jagath TIA for your time. Best regards, Denis Benato
Hi Denis, Jonathan On Sun, Oct 1, 2023 at 7:23 PM Denis Benato <benato.denis96@gmail.com> wrote: > > Hello Jagath, > > On 9/29/23 09:59, Jagath Jog J wrote: > > Hi Denis, > > > > On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote: > >> > >> Hello, > >> > >> Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe. > >> > >> The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check. > > > > bmc150 driver supports multiple accelerometer sensors such as > > bma222, bma280, bmi055 and all of them are having similar > > register map, but the bmi323 register map is completely different > > from bmc150. > > I apologize for the confusion. > > What I was trying to say is that to have the bmi323 working in those aforementioned devices bmc150 will need to be modified: that is the probe function that ends up being executed, fact that cannot be changed because it depends on the ACPI implementation shipped on those devices. > > Therefore I was asking about the best way of handing control to the new driver and how that should be organized: in my implementation the new bmi323 code was written inside the bmc150-accel-core.c and only shares sleep/suspend, probe and removal functions in addition to checking for the new chip presence before checking for any bmc150 chip as that issues an i2c write, while the check for the bmi323 only requires an i2c read. Means you want to handle control to the standalone driver from bmc150. Sorry, I didn't find any examples. Important thing to handle is the bmi323 private structure and call required exported functions from another driver. Jonathan: Can you suggest any example wrapper drivers which handle that way? > > We also have done duplicate work as I have written a driver for that chip myself, but it's not as good as yours because my hardware didn't come with the IRQ pin connected and so I couldn't develop triggers and I only got the i2c interface working. > > > > > > >> > >> I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor. > >> > >> What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore? > >> > >> TIA for your time. > >> > >> Best regards, > >> Denis Benato > > > > Regards > > > > Jagath > > TIA for your time. > > Best regards, > Denis Benato Regards Jagath
Hi Andy, On Wed, Sep 20, 2023 at 4:13 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > 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. > > > ... > > > +static const struct attribute_group bmi323_event_attribute_group = { > > > + .attrs = bmi323_event_attributes, > > > +}; > > > > ATTRIBUTE_GROUPS() ? > > Okay, I will use ATTRIBUTE_GROUPS. The ATTRIBUTE_GROUP(bmi323_event) macro will define two variables, bmi323_event_groups and bmi323_event_group. The event_attrs member of iio_info is of type struct attribute_group*, which means that bmi323_event_groups will remain unused. Since I am using a single event group, Can I keep it the same way? Regards Jagath Jagath
On Sun, 8 Oct 2023 11:55:32 +0530 Jagath Jog J <jagathjog1996@gmail.com> wrote: > Hi Andy, > > > On Wed, Sep 20, 2023 at 4:13 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > > > On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > 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. > > > > > > ... > > > > > +static const struct attribute_group bmi323_event_attribute_group = { > > > > + .attrs = bmi323_event_attributes, > > > > +}; > > > > > > ATTRIBUTE_GROUPS() ? > > > > Okay, I will use ATTRIBUTE_GROUPS. > > The ATTRIBUTE_GROUP(bmi323_event) macro will define two variables, > bmi323_event_groups and bmi323_event_group. The event_attrs member > of iio_info is of type struct attribute_group*, which means that > bmi323_event_groups will remain unused. Since I am using a single > event group, Can I keep it the same way? Yes, don't use that macro as not appropriate in this case. > > > Regards > Jagath > > Jagath