linux-kernel - Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vc30u=1jx3qNft-uOVCk49e4gTgyLf3+kgmUADQB56wEA@mail.gmail.com>
Date: Fri, 25 Apr 2025 07:33:20 +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
Subject: Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

On Fri, Apr 25, 2025 at 3:15 AM Gustavo Silva <gustavograzs@...il.com> wrote:
>
> Add support for generating events when the step counter reaches the
> configurable watermark.

With the below being addressed,
Reviewed-by: Andy Shevchenko <andy@...nel.org>

...

> +static int bmi270_write_event_config(struct iio_dev *indio_dev,
> +                                    const struct iio_chan_spec *chan,
> +                                    enum iio_event_type type,
> +                                    enum iio_event_direction dir, bool state)
> +{
> +       struct bmi270_data *data = iio_priv(indio_dev);
> +
> +       switch (type) {
> +       case IIO_EV_TYPE_CHANGE:
> +               return bmi270_step_wtrmrk_en(data, state);
> +       default:
> +               return -EINVAL;
> +       }

> +
> +       return 0;

Dead code.

> +}

...

> +       switch (type) {
> +       case IIO_EV_TYPE_CHANGE:

> +               if (!in_range(val, 0, 20461))

I prefer that + 1 to be separated and the value defined.

(0, _FOO + 1)

> +                       return -EINVAL;

> +               raw = val / 20;

Needs a comment.  Is this in the Datasheet? Then reference to the
section / table / formula would be nice to have.

> +               return bmi270_update_feature_reg(data, BMI270_SC_26_REG,
> +                                                BMI270_STEP_SC26_WTRMRK_MSK,
> +                                                FIELD_PREP(BMI270_STEP_SC26_WTRMRK_MSK,
> +                                                           raw));
> +       default:
> +               return -EINVAL;
> +       }

...

> +               raw = FIELD_GET(BMI270_STEP_SC26_WTRMRK_MSK, reg_val);
> +               *val = raw * 20;

Same.

> +               return IIO_VAL_INT;


-- 
With Best Regards,
Andy Shevchenko