Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Vaittinen, Matti]

On 9/21/22 14:45, Matti Vaittinen wrote:
> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
> 
> Add support for the basic accelerometer features such as getting the
> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> using the WMI IRQ).
> 
> Important things to be added include the double-tap, motion
> detection and wake-up as well as the runtime power management.
> 
> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
> I noticed that filling up the hardware FIFO might mess-up the sample
> count. My sensor ended up in a state where amount of data in FIFO was
> reported to be 0xff bytes, which equals to 42,5 samples. Specification
> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
> least once the FIFO was stuck in a state where reading data from
> hwardware FIFO did not decrease the amount of data reported to be in the
> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
> reads with invalid FIFO data count will cause the fifo contents to be
> dropped.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> ---

//snip

> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c

//snip

> +static int kx022a_write_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +	int ret;
> +
> +	/*
> +	 * We should not allow changing scale or frequency when FIFO is running
> +	 * as it will mess the timestamp/scale for samples existing in the
> +	 * buffer. If this turns out to be an issue we can later change logic
> +	 * to internally flush the fifo before reconfiguring so the samples in
> +	 * fifo keep matching the freq/scale settings. (Such setup could cause
> +	 * issues if users trust the watermark to be reached within known
> +	 * time-limit).
> +	 */
> +	mutex_lock(&data->mutex);
> +	if (iio_buffer_enabled(idev)) {
> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0],
> +					      ARRAY_SIZE(kx022a_accel_samp_freq_table),
> +					      val, val2);
> +		/* Configure if we found valid ODR */
> +		if (ret >= 0) {
> +			int odr = ret;
> +
> +			ret = __kx022a_turn_off_unlocked(data);
> +			if (ret)
> +				goto unlock_out;
> +
> +			ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL,
> +						 KX022A_MASK_ODR, odr);
> +			data->odr_ns = kx022a_odrs[odr];
> +			__kx022a_turn_on_unlocked(data);
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = kx022a_find_tuple_index(&kx022a_scale_table[0],
> +					      ARRAY_SIZE(kx022a_scale_table),
> +					      val, val2);
> +		/* Configure if we found valid scale */
> +		if (ret >= 0) {
> +			ret = __kx022a_turn_off_unlocked(data);
> +			if (ret)
> +				goto unlock_out;
> +
> +			ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
> +						 KX022A_MASK_GSEL,
> +						 ret << KX022A_GSEL_SHIFT);
> +			__kx022a_turn_on_unlocked(data);
> +		}

I did a last minute change to the locking here. As a result I missed the 
break causing -EINVAL to be returned for scale setting. The build bot 
pinged me about it :) Will be fixed when I respin this!

> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
Yours,
	--Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~