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