Re: [PATCH v2 2/2] iio: accel: bma400: Add support for single and double tap events

From: Jonathan Cameron <jic23@kernel.org>
To: Jagath Jog J <jagathjog1996@gmail.com>
Cc: andy.shevchenko@gmail.com, hadess@hadess.net,
	hdegoede@redhat.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] iio: accel: bma400: Add support for single and double tap events
Date: Sun, 14 Aug 2022 18:12:12 +0100	[thread overview]
Message-ID: <20220814181212.2171bd34@jic23-huawei> (raw)
In-Reply-To: <20220813071803.4692-3-jagathjog1996@gmail.com>

On Sat, 13 Aug 2022 12:48:03 +0530
Jagath Jog J <jagathjog1996@gmail.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@gmail.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;
>  	}