| Message ID | cover.1674996464.git.mehdi.djait.k@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: accel: kionix-kx022a: Minor fixes | expand |
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
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
On 1/31/23 21:48, Mehdi Djait wrote: > Hi Matti, > > On Mon, Jan 30, 2023 at 10:15:51AM +0200, Matti Vaittinen wrote: >> On 1/29/23 15:37, Mehdi Djait wrote: > > /* > * 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]; > Ok. Thanks for the clarification. In that case, as I mentioned below: >> 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. I don't think it makes much difference. The regmap should cache the value anyways. So, to be honest, I don't think it's worth the space and code - unless the GSEL reg was in volatile range for some reason - and even in that case the benefit might not be so big because AFAIR, the g-range is not read for every sample. Yours, -- Matti
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) but does it make sense to keep track of the g_range value ? Mehdi Djait (2): iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table iio: accel: kionix-kx022a: Remove the unused member g_range drivers/iio/accel/kionix-kx022a.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)