Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J <jagathjog1996@gmail.com>]

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, &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, &reg, 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