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

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: <20250331112205.2457d829@jic23-huawei>
Date: Mon, 31 Mar 2025 11:22:05 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 eraretuya@...il.com
Subject: Re: [PATCH v5 02/11] iio: accel: adxl345: add single tap feature

On Tue, 18 Mar 2025 23:08:34 +0000
Lothar Rubusch <l.rubusch@...il.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@...il.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;
>  }