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

[Andy Shevchenko]

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@xxxxxxxxxxxxxxx> 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