Re: [PATCH v5 02/11] iio: accel: adxl345: add single tap feature

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 18 Mar 2025 23:08:34 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add the single tap feature with a threshold in 62.5mg/LSB points and a
> scaled duration in us. Keep singletap threshold in regmap cache but
> the scaled value of duration in us as member variable.
> 
> Both use IIO channels for individual enable of the x/y/z axis. Initializes
> threshold and duration with reasonable content. When an interrupt is
> caught it will be pushed to the according IIO channel.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

A couple of build issues plus one comment inline
(I was going to tweak that thing whilst applying but the axis_en one is more complex).

  drivers/iio/accel/adxl345_core.c: In function ‘adxl345_is_tap_en’:
  drivers/iio/accel/adxl345_core.c:201:14: warning: variable ‘axis_en’ set but not used [-Wunused-but-set-variable]
    201 |         bool axis_en;
        |              ^~~~~~~
Not sure what intent was in this code.

  drivers/iio/accel/adxl345_core.c: At top level:
  drivers/iio/accel/adxl345_core.c:818:31: error: initialization of ‘int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type,  enum iio_event_direction, bool)’ {aka ‘int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type,  enum iio_event_direction,  _Bool)’} from incompatible pointer type ‘int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type,  enum iio_event_direction,  int)’ [-Wincompatible-pointer-types]
     818 |         .write_event_config = adxl345_write_event_config,
        |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/accel/adxl345_core.c:818:31: note: (near initialization for ‘adxl345_info.write_event_config’)

This is a change in the core to take a boolean for the last parameter.  That one I'd have
just fixed up whilst applying if not for the axis_en one above.

>  
> -static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
> +static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> +			      enum iio_modifier tap_dir)
>  {
> +	s64 ts = iio_get_time_ns(indio_dev);
>  	struct adxl345_state *st = iio_priv(indio_dev);
>  	int samples;
> +	int ret = -ENOENT;
> +
> +	if (FIELD_GET(ADXL345_INT_SINGLE_TAP, int_stat)) {
> +		ret = iio_push_event(indio_dev,
> +				     IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, tap_dir,
> +							IIO_EV_TYPE_GESTURE,
> +							IIO_EV_DIR_SINGLETAP),
> +				     ts);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
>  		samples = adxl345_get_samples(st);
> @@ -428,9 +751,11 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
>  
>  		if (adxl345_fifo_push(indio_dev, samples) < 0)
>  			return -EINVAL;
> +
> +		return 0;
I'm normally a fan of early returns but in this one corner case I think
		ret = 0;
makes it more consistent with the other field matches.
It is just coincidence that the other one sets the value of ret inside
the if block.

>  	}
>  
> -	return 0;
> +	return ret;
>  }