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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHp75Ve+6BjG0vFy0ohMsODkybH3L+4EM6RODJYDRw+W6Gdtmg@mail.gmail.com>
Date: Tue, 17 Jun 2025 09:32:56 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Gustavo Silva <gustavograzs@...il.com>
Cc: Alex Lanzano <lanzano.alex@...il.com>, Jonathan Cameron <jic23@...nel.org>, 
	David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Lothar Rubusch <l.rubusch@...il.com>
Subject: Re: [PATCH v3 3/3] iio: imu: bmi270: add support for motion events

On Tue, Jun 17, 2025 at 2:53 AM Gustavo Silva <gustavograzs@...il.com> 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