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
