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

[Sean Nyekjaer <sean@geanix.com>]

On 16/09/2019 13.55, Lorenzo Bianconi wrote:
>>
>>
>> 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;

I actually think this is needed as if a channel is enabled and another 
channel is enabled, then 'state' will be 1 and '0 < hw->enable_event' is 
true. Then we don't want to do the event_setup again.

> 
>>          } 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
Like wise here, if we don't have this and to channels is enabled, if you 
deactivate one of the channels then enable_event indicates that a 
channel is active but the sensor is disabled.
Guess that is not what we want :-)

> 
>>          }
>>
>>          /* 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;
>>