linux-kernel - Re: [PATCH 7/9] iio: imu: st_lsm6dsx: make event management functions generic

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <78f1502c131a7f5bd6dc24c364aceff154501ece.camel@baylibre.com>
Date: Thu, 30 Oct 2025 12:17:52 +0100
From: Francesco Lavra <flavra@...libre.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>, 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 7/9] iio: imu: st_lsm6dsx: make event management
 functions generic

On Thu, 2025-10-30 at 10:15 +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 08:27:50AM +0100, Francesco Lavra wrote:
> > In preparation for adding support for more event types, use an
> > array indexed by event ID instead of a scalar value to store
> > enabled events, and refactor the functions to configure and report
> > events so that their implementation is not specific for wakeup
> > events. Move the logic to update the global event interrupt enable
> > flag from st_lsm6dsx_event_setup() to its calling function, so that
> > it can take into account also event sources different from the
> > source being configured. While changing the signature of the
> > st_lsm6dsx_event_setup() function, opportunistically add the
> > currently unused `axis` parameter, which will be used when adding
> > support for enabling and disabling events on a per axis basis.
> 
> ...
> 
> >         mutex_lock(&hw->conf_lock);
> > -       if (enable_event || !(hw->fifo_mask & BIT(sensor->id)))
> > +       if (!enable_event) {
> > +               enum st_lsm6dsx_event_id other_event;
> > +
> > +               for (other_event = 0; other_event <
> > ST_LSM6DSX_EVENT_MAX; other_event++) {
> > +                       if (other_event != event && hw-
> > >enable_event[other_event]) {
> > +                               any_events_enabled = true;
> > +                               break;
> > +                       }
> > +               }
> > +       }
> > +       if (enable_event || !any_events_enabled) {
> > +               const struct st_lsm6dsx_reg *reg = &hw->settings-
> > >event_settings.enable_reg;
> > +
> > +               if (reg->addr) {
> > +                       err = regmap_update_bits(hw->regmap, reg->addr,
> > reg->mask,
> > +                                               
> > ST_LSM6DSX_SHIFT_VAL(state, reg->mask));
> > +                       if (err < 0)
> > +                               goto unlock_out;
> > +               }
> > +       }
> > +       if (enable_event || (!any_events_enabled && !(hw->fifo_mask &
> > BIT(sensor->id))))
> >                 err = __st_lsm6dsx_sensor_set_enable(sensor, state);
> > +unlock_out:
> >         mutex_unlock(&hw->conf_lock);
> >         if (err < 0)
> >                 return err;
> 
> This whole block is hard to read. Perhaps you need to refactor it to have
> something like
> 
>         if (enable_event) {
>                 err = call_helper1();
>                 ...
>                 err = __st_lsm6dsx_sensor_set_enable(sensor, state);
>         } else {
>                 any_events_enabled = call_helper2();
>                 if (!any_events_enabled) {
>                         err = call_helper1();
>                         ...
>                         if (!(hw->fifo_mask & BIT(sensor->id)))
>                                 err =
> __st_lsm6dsx_sensor_set_enable(sensor, state);
>                 }
>         }
> 
> With this you can see that actually helper1 can be modified (with one
> additional parameter) to combination of
> 
> new_helper1()
> {
>         err = call_helper1();
>         ...
>         if (!(hw->fifo_mask & BIT(sensor->id)))
>                 return __st_lsm6dsx_sensor_set_enable(sensor, state);
>         return 0;
> }
> 
> And the above goes as
> 
>         if (enable_event) {
>                 err = new_helper1(false);
>         } else {
>                 any_events_enabled = call_helper2();
>                 if (!any_events_enabled)
>                         err = new_helper1(hw->fifo_mask & BIT(sensor-
> >id));
>         }
> 
> with assumed good names given this looks to me much easier to understand.

Will do

> 
> > +static bool
> > +st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
> 
> Why not one line?

This function was already there as is, even though the diff makes it appear
as a new function. Will make it one line.

> 
> > +{
> > +       bool events_found;
> 
> Seems useless. Is this function going to be expanded down in the series?

Yes, when adding support for tap events in patch 9/9.

> > +       events_found = st_lsm6dsx_report_events(hw,
> > ST_LSM6DSX_EVENT_WAKEUP, IIO_EV_TYPE_THRESH,
> > +                                               IIO_EV_DIR_EITHER);
> 
> Indentation.

Seems good to me, what should I change?

> > +       return events_found;
> > +}
> 


Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)