Hi Andy,
Thank you for reviewing.
On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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
