Re: [PATCH v3 3/3] iio: imu: bmi270: add support for motion events

[Andy Shevchenko]

On Tue, Jun 17, 2025 at 2:53 AM Gustavo Silva <gustavograzs@xxxxxxxxx> wrote:
>
> Any-motion event can be enabled on a per-axis basis and triggers a
> combined event when motion is detected on any axis.
>
> No-motion event is triggered if the rate of change on all axes falls
> below a specified threshold for a configurable duration. A fake channel
> is used to report this event.
>
> Threshold and duration can be configured from userspace.

...

> +#define BMI270_INT_MICRO_TO_RAW(val, val2, scale) ((val) * (scale) + \
> +                                                 ((val2) * (scale)) / MEGA)

Much easier to read and maintain when it's split logically, i.e.

#define BMI270_INT_MICRO_TO_RAW(val, val2, scale) \
        ((val) * (scale) + ((val2) * (scale)) / MEGA)

...

> +#define BMI270_RAW_TO_MICRO(raw, scale) ((((raw) % (scale)) * MEGA) / scale)

Ditto for the sake of consistency with the above.

...

> +       case IIO_ACCEL:
> +               switch (type) {
> +               case IIO_EV_TYPE_ROC:
> +                       return FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK,
> +                                        regval) ? 1 : 0;

I would do it with logical split despite being longer (than 80) line

                       return
FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK, regval) ?
                                        1 : 0;

Or even
                       return
!!FIELD_GET(BMI270_INT_MAP_FEAT_NOMOTION_MSK, regval);

if 1 is important here, or w/o !! if not.

> +               case IIO_EV_TYPE_MAG_ADAPTIVE:
> +                       ret = bmi270_read_feature_reg(data, BMI270_ANYMO1_REG,
> +                                                     &motion_reg);
> +                       if (ret)
> +                               return ret;
> +
> +                       feat_en = FIELD_GET(BMI270_INT_MAP_FEAT_ANYMOTION_MSK,
> +                                           regval);
> +                       switch (chan->channel2) {
> +                       case IIO_MOD_X:
> +                               axis_en = FIELD_GET(BMI270_FEAT_MOTION_X_EN_MSK,
> +                                                   motion_reg);
> +                               break;
> +                       case IIO_MOD_Y:
> +                               axis_en = FIELD_GET(BMI270_FEAT_MOTION_Y_EN_MSK,
> +                                                   motion_reg);
> +                               break;
> +                       case IIO_MOD_Z:
> +                               axis_en = FIELD_GET(BMI270_FEAT_MOTION_Z_EN_MSK,
> +                                                   motion_reg);
> +                               break;
> +                       default:
> +                               return -EINVAL;
> +                       }
> +                       return (axis_en && feat_en) ? 1 : 0;

Do you expect boolean to be not 1 when it's true? IIRC this is part of
the C standard.

> +               default:
> +                       return -EINVAL;
> +               }

...

> +       int ret, reg;
>         u16 regval;
> -       int ret;

Why? And why is reg signed? Also check other cases with reg and
semantically similar variables.

-- 
With Best Regards,
Andy Shevchenko