Re: [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes

[Matti Vaittinen <mazziesaccount@gmail.com>]

Hi Mehdi,

On 1/29/23 15:37, Mehdi Djait wrote:
> Two minor fixes. Swap the setting of rd_table and wr_table and remove
> the g_range member.
> 
> Matti, I thought about defining an unsigned int array for the 4 possible
> g ranges, setting a g_range initial value in the probe function and
> updating it in the write_raw callback (case IIO_CHAN_INFO_SCALE)

How would it differ from current write_raw behaviour (below)?

[mvaittin@dc75zzyyyyyyyyyyyyycy-3 linux]$ grep -A70 write_raw 
drivers/iio/accel/kionix-kx022a.c
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, n;

	/*
	 * 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).
	 */
	ret = iio_device_claim_direct_mode(idev);
	if (ret)
		return ret;

	switch (mask) {

//snip

	case IIO_CHAN_INFO_SCALE:
		n = ARRAY_SIZE(kx022a_scale_table);

		while (n-- > 0)
			if (val == kx022a_scale_table[n][0] &&
			    val2 == kx022a_scale_table[n][1])
				break;
		if (n < 0) {
			ret = -EINVAL;
			goto unlock_out;
		}

		ret = kx022a_turn_off_lock(data);
		if (ret)
			break;

		ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
					 KX022A_MASK_GSEL,
					 n << KX022A_GSEL_SHIFT);
		kx022a_turn_on_unlock(data);
		break;
//snip


  but
> does it make sense to keep track of the g_range value ?

Do you mean caching the g_range instead of retrieving it from the 
hardware? I don't know an use-case where reading the range would be 
time-critical - and even if it was, the regmap should cache the value 
anyways. (unless KX022A_REG_CNTL is in volatile range). So no, I don't 
think caching the g_range is worth it.

Yours,
	-- Matti

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

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