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

[Mehdi Djait <mehdi.djait.k@gmail.com>]

Hi Matti,

On Mon, Jan 30, 2023 at 10:15:51AM +0200, Matti Vaittinen wrote:
> 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);

        /*
        * The only difference would be to store the g_range value in the
        * driver private struct when the user changes it from sysfs
        * (in this case I defined an array called kx022a_g_range_table)
        */

        data->g_range = kx022a_g_range_table[n];

> 		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! ~~
> 

--
Best Regards,
Mehdi Djait