Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 15 Feb 2020 15:52:55 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Alexandru Tachici <alexandru.tachici@analog.com>
Cc:     <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/5] iio: accel: adxl372: Add support for FIFO peak mode
Message-ID: <20200215155255.5902fe3c@archlinux>
In-Reply-To: <20200214092919.24351-2-alexandru.tachici@analog.com>
References: <20200214092919.24351-1-alexandru.tachici@analog.com>
        <20200214092919.24351-2-alexandru.tachici@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 14 Feb 2020 11:29:15 +0200
Alexandru Tachici <alexandru.tachici@analog.com> wrote:

> From: Stefan Popa <stefan.popa@analog.com>
> 
> By default, if all three channels (x, y, z) are enabled, sample sets of
> concurrent 3-axis data is stored in the FIFO. This patch adds the option
> to configure the FIFO to store peak acceleration (x, y and z) of every
> over-threshold event. Since we cannot store 1 or 2 axis peak acceleration
> data in the FIFO, then all three axis need to be enabled in order for this
> mode to work.
> 
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
I've left comments on the interface until the documentation patch.
A few other bits in here.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/adxl372.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index 67b8817995c0..bb6c2bf1a457 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -264,6 +264,7 @@ struct adxl372_state {
>  	u8				int2_bitmask;
>  	u16				watermark;
>  	__be16				fifo_buf[ADXL372_FIFO_SIZE];
> +	bool				peak_fifo_mode_en;
>  };
>  
>  static const unsigned long adxl372_channel_masks[] = {
> @@ -722,6 +723,36 @@ static int adxl372_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static ssize_t adxl372_peak_fifo_en_get(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct adxl372_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", st->peak_fifo_mode_en);
> +}
> +
> +static ssize_t adxl372_peak_fifo_en_set(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct adxl372_state *st = iio_priv(dev_to_iio_dev(dev));
> +	bool val;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	st->peak_fifo_mode_en = val;

Should reject the attempt if the buffer is already enabled.  Otherwise
the userspace interface will be rather confusing as you'll read back that
it is enabled, but not see it working.

> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(peak_fifo_mode_enable, 0644,
> +		       adxl372_peak_fifo_en_get,
> +		       adxl372_peak_fifo_en_set, 0);
> +
>  static ssize_t adxl372_show_filter_freq_avail(struct device *dev,
>  					      struct device_attribute *attr,
>  					      char *buf)
> @@ -817,11 +848,16 @@ static int adxl372_buffer_postenable(struct iio_dev *indio_dev)
>  	st->fifo_format = adxl372_axis_lookup_table[i].fifo_format;
>  	st->fifo_set_size = bitmap_weight(indio_dev->active_scan_mask,
>  					  indio_dev->masklength);
> +
> +	/* Configure the FIFO to store sets of impact event peak. */
> +	if (st->fifo_set_size == 3 && st->peak_fifo_mode_en)
> +		st->fifo_format = ADXL372_XYZ_PEAK_FIFO;

We could perhaps make this more intuitive by always enabling the 3 axis
if peak mode is on and filtering the data on it's way to the
push_to_buffer to reflect only channels enabled.

If not, perhaps a warning message?

>  	/*
>  	 * The 512 FIFO samples can be allotted in several ways, such as:
>  	 * 170 sample sets of concurrent 3-axis data
>  	 * 256 sample sets of concurrent 2-axis data (user selectable)
>  	 * 512 sample sets of single-axis data
> +	 * 170 sets of impact event peak (x, y, z)
>  	 */
>  	if ((st->watermark * st->fifo_set_size) > ADXL372_FIFO_SIZE)
>  		st->watermark = (ADXL372_FIFO_SIZE  / st->fifo_set_size);
> @@ -894,6 +930,7 @@ static IIO_DEVICE_ATTR(in_accel_filter_low_pass_3db_frequency_available,
>  static struct attribute *adxl372_attributes[] = {
>  	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_filter_low_pass_3db_frequency_available.dev_attr.attr,
> +	&iio_dev_attr_peak_fifo_mode_enable.dev_attr.attr,
>  	NULL,
>  };
>