Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

> 
> 
> On 16/09/2019 11.16, Lorenzo Bianconi wrote:
> > > +	if (hw->event_en_mask & BIT(chan->channel2))
> > > +		goto out;
> > Why do we need this?
> > 
> 
> Guess we need to check if 0 < hw->enable_event before disabling the
> sensor...
> > > +
> > >   	/* do not enable events if they are already enabled */
> > >   	if (state && hw->enable_event)
> > > -		return 0;
> > > +		goto out;
> 
> static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>                                            const struct iio_chan_spec *chan,
>                                            enum iio_event_type type,
>                                            enum iio_event_direction dir,
>                                            int state)
> {
>         struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>         struct st_lsm6dsx_hw *hw = sensor->hw;
>         u8 _enable_event;

please use enable_event instead of _enable_event

>         int err = 0;
> 
>         if (type != IIO_EV_TYPE_THRESH)
>                 return -EINVAL;
> 
>         if (state) {
>                 _enable_event = hw->enable_event | BIT(chan->channel2);
> 
>                 /* do not enable events if they are already enabled */
>                 if (0 < hw->enable_event)
>                         goto out;

we do not need this since there is the check:
if (hw->enable_event == enable_event)
	return 0;

>         } else {
>                 _enable_event = hw->enable_event & ~BIT(chan->channel2);
> 
>                 /* only turn off sensor if no events is enabled */
> 
> 
> 
> 
>                 if (0 != _enable_event)
>                         goto out;

we do not need this as well

>         }
> 
>         /* stop here if no changes have been made */
>         if (hw->enable_event == _enable_event)
>                 return 0;
> 
>         err = st_lsm6dsx_event_setup(hw, state);
>         if (err < 0)
>                 return err;
> 
>         err = st_lsm6dsx_sensor_set_enable(sensor, state);
>         if (err < 0)
>                 return err;
> 
> out:
>         hw->enable_event = _enable_event;
> 
>         return 0;
> }
> 
> Is something like this better?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]