linux-kernel - Re: [PATCH v2 2/2] iio: accel: bma400: Add support for single and double tap 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220814181212.2171bd34@jic23-huawei>
Date:   Sun, 14 Aug 2022 18:12:12 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Jagath Jog J <jagathjog1996@...il.com>
Cc:     andy.shevchenko@...il.com, hadess@...ess.net, hdegoede@...hat.com,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: accel: bma400: Add support for single and
 double tap events

On Sat, 13 Aug 2022 12:48:03 +0530
Jagath Jog J <jagathjog1996@...il.com> wrote:

> Add support for single and double tap events based on the tap threshold
> value, minimum quiet time before and after the tap and minimum time
> between the taps in the double tap. The INT1 pin is used to interrupt
> and the event is pushed to userspace.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@...il.com>

Hi Jagath,

As, for this series, the difficult bit from review point of view is the ABI
I've only taken a quick look at the implementation.  Basically looks fine to me,
but one trivial thing below (and that might just be me failing to read a diff)

...
>  		ret = bma400_steps_event_enable(data, state);
> @@ -1157,10 +1379,10 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
>  				   int *val, int *val2)
>  {
>  	struct bma400_data *data = iio_priv(indio_dev);
> -	int ret, reg;
> +	int ret, reg, reg_val, raw;
>  
> -	switch (chan->type) {
> -	case IIO_ACCEL:

We've lost the check on ACCEL I think.  Might be worth putting
it back as
	if (chan->type != ACCEL)
		return -EINVAL;

> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:
>  		reg = get_gen_config_reg(dir);
>  		if (reg < 0)
>  			return -EINVAL;
> @@ -1196,6 +1418,39 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
> +					  &reg_val);
> +			if (ret)
> +				return ret;
> +
> +			*val = FIELD_GET(BMA400_TAP_SEN_MSK, reg_val);
> +			return IIO_VAL_INT;
> +		case IIO_EV_INFO_RESET_TIMEOUT:
> +			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
> +					  &reg_val);
> +			if (ret)
> +				return ret;
> +
> +			raw = FIELD_GET(BMA400_TAP_QUIET_MSK, reg_val);
> +			*val = 0;
> +			*val2 = tap_timeout[raw];
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_EV_INFO_TAP_2MIN_DELAY:
> +			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
> +					  &reg_val);
> +			if (ret)
> +				return ret;
> +
> +			raw = FIELD_GET(BMA400_TAP_QUIETDT_MSK, reg_val);
> +			*val = 0;
> +			*val2 = doubletap_2min_delay[raw];
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}