* [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes
@ 2023-01-29 13:37 Mehdi Djait
2023-01-29 13:37 ` [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table Mehdi Djait
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Mehdi Djait @ 2023-01-29 13:37 UTC (permalink / raw)
To: mazziesaccount, jic23; +Cc: linux-iio, Mehdi Djait
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(-)
--
2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table 2023-01-29 13:37 [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Mehdi Djait @ 2023-01-29 13:37 ` Mehdi Djait 2023-01-30 7:37 ` Matti Vaittinen 2023-01-29 13:37 ` [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range Mehdi Djait 2023-01-30 8:15 ` [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Matti Vaittinen 2 siblings, 1 reply; 10+ messages in thread From: Mehdi Djait @ 2023-01-29 13:37 UTC (permalink / raw) To: mazziesaccount, jic23; +Cc: linux-iio, Mehdi Djait rd_table points to a regmap_access_table with valid ranges for read access and should be set to &kx022a_ro_regs which points to the read_only regs. The same for wr_table. Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> --- drivers/iio/accel/kionix-kx022a.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c index f866859855cd..1d3af42ec0e1 100644 --- a/drivers/iio/accel/kionix-kx022a.c +++ b/drivers/iio/accel/kionix-kx022a.c @@ -142,8 +142,8 @@ const struct regmap_config kx022a_regmap = { .reg_bits = 8, .val_bits = 8, .volatile_table = &kx022a_volatile_regs, - .rd_table = &kx022a_wo_regs, - .wr_table = &kx022a_ro_regs, + .rd_table = &kx022a_ro_regs, + .wr_table = &kx022a_wo_regs, .rd_noinc_table = &kx022a_nir_regs, .precious_table = &kx022a_precious_regs, .max_register = KX022A_MAX_REGISTER, -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table 2023-01-29 13:37 ` [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table Mehdi Djait @ 2023-01-30 7:37 ` Matti Vaittinen 2023-01-31 19:32 ` Mehdi Djait 0 siblings, 1 reply; 10+ messages in thread From: Matti Vaittinen @ 2023-01-30 7:37 UTC (permalink / raw) To: Mehdi Djait, jic23; +Cc: linux-iio Hi Mehdi, Thank you for the patch. On 1/29/23 15:37, Mehdi Djait wrote: > rd_table points to a regmap_access_table with valid ranges for read access > and should be set to &kx022a_ro_regs which points to the read_only regs. > The same for wr_table. > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > --- > drivers/iio/accel/kionix-kx022a.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > index f866859855cd..1d3af42ec0e1 100644 > --- a/drivers/iio/accel/kionix-kx022a.c > +++ b/drivers/iio/accel/kionix-kx022a.c > @@ -142,8 +142,8 @@ const struct regmap_config kx022a_regmap = { > .reg_bits = 8, > .val_bits = 8, > .volatile_table = &kx022a_volatile_regs, > - .rd_table = &kx022a_wo_regs, > - .wr_table = &kx022a_ro_regs, > + .rd_table = &kx022a_ro_regs, > + .wr_table = &kx022a_wo_regs, Have you tested this? If I interpret the code correctly, the current code (before this patch) adds read-only registers - Eg, registers which are not writable - to wr_table no-range. I think it is correct way. Same for write-only registers - eg, registers which are not readable - are stored in rd-table no-range. Do you think I am misunderstanding something? > .rd_noinc_table = &kx022a_nir_regs, > .precious_table = &kx022a_precious_regs, > .max_register = KX022A_MAX_REGISTER, Best Regards -- Matti -- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table 2023-01-30 7:37 ` Matti Vaittinen @ 2023-01-31 19:32 ` Mehdi Djait 0 siblings, 0 replies; 10+ messages in thread From: Mehdi Djait @ 2023-01-31 19:32 UTC (permalink / raw) To: Matti Vaittinen, jic23; +Cc: linux-iio Hi Matti, On Mon, Jan 30, 2023 at 09:37:12AM +0200, Matti Vaittinen wrote: > Hi Mehdi, > > Thank you for the patch. > > On 1/29/23 15:37, Mehdi Djait wrote: > > rd_table points to a regmap_access_table with valid ranges for read access > > and should be set to &kx022a_ro_regs which points to the read_only regs. > > The same for wr_table. > > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > > --- > > drivers/iio/accel/kionix-kx022a.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > > index f866859855cd..1d3af42ec0e1 100644 > > --- a/drivers/iio/accel/kionix-kx022a.c > > +++ b/drivers/iio/accel/kionix-kx022a.c > > @@ -142,8 +142,8 @@ const struct regmap_config kx022a_regmap = { > > .reg_bits = 8, > > .val_bits = 8, > > .volatile_table = &kx022a_volatile_regs, > > - .rd_table = &kx022a_wo_regs, > > - .wr_table = &kx022a_ro_regs, > > + .rd_table = &kx022a_ro_regs, > > + .wr_table = &kx022a_wo_regs, > > Have you tested this? If I interpret the code correctly, the current code > (before this patch) adds read-only registers - Eg, registers which are not > writable - to wr_table no-range. I think it is correct way. Same for > write-only registers - eg, registers which are not readable - are stored in > rd-table no-range. Do you think I am misunderstanding something? > I am the person totally misunderstanding everything here (which is expected). I was confused by the assignment of write_only_regs to readable_table and read_only_regs to writeable_table, it's not an excuse I should have verified more before sending a patch. -- Best Regards, Mehdi Djait > > .rd_noinc_table = &kx022a_nir_regs, > > .precious_table = &kx022a_precious_regs, > > .max_register = KX022A_MAX_REGISTER, > > Best Regards > -- Matti > > -- > Matti Vaittinen > Linux kernel developer at ROHM Semiconductors > Oulu Finland > > ~~ When things go utterly wrong vim users can always type :help! ~~ > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range 2023-01-29 13:37 [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Mehdi Djait 2023-01-29 13:37 ` [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table Mehdi Djait @ 2023-01-29 13:37 ` Mehdi Djait 2023-01-30 8:17 ` Matti Vaittinen 2023-01-30 8:15 ` [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Matti Vaittinen 2 siblings, 1 reply; 10+ messages in thread From: Mehdi Djait @ 2023-01-29 13:37 UTC (permalink / raw) To: mazziesaccount, jic23; +Cc: linux-iio, Mehdi Djait The g_range member of the driver data struct is not used and should therefore be removed. Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> --- drivers/iio/accel/kionix-kx022a.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c index 1d3af42ec0e1..bb43cb0acce4 100644 --- a/drivers/iio/accel/kionix-kx022a.c +++ b/drivers/iio/accel/kionix-kx022a.c @@ -162,7 +162,6 @@ struct kx022a_data { int inc_reg; int ien_reg; - unsigned int g_range; unsigned int state; unsigned int odr_ns; -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range 2023-01-29 13:37 ` [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range Mehdi Djait @ 2023-01-30 8:17 ` Matti Vaittinen 2023-02-05 14:56 ` Jonathan Cameron 0 siblings, 1 reply; 10+ messages in thread From: Matti Vaittinen @ 2023-01-30 8:17 UTC (permalink / raw) To: Mehdi Djait, jic23; +Cc: linux-iio On 1/29/23 15:37, Mehdi Djait wrote: > The g_range member of the driver data struct is not used and > should therefore be removed Well spotted, thanks. > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > --- > drivers/iio/accel/kionix-kx022a.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > index 1d3af42ec0e1..bb43cb0acce4 100644 > --- a/drivers/iio/accel/kionix-kx022a.c > +++ b/drivers/iio/accel/kionix-kx022a.c > @@ -162,7 +162,6 @@ struct kx022a_data { > int inc_reg; > int ien_reg; > > - unsigned int g_range; > unsigned int state; > unsigned int odr_ns; > -- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range 2023-01-30 8:17 ` Matti Vaittinen @ 2023-02-05 14:56 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2023-02-05 14:56 UTC (permalink / raw) To: Matti Vaittinen; +Cc: Mehdi Djait, linux-iio On Mon, 30 Jan 2023 10:17:00 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 1/29/23 15:37, Mehdi Djait wrote: > > The g_range member of the driver data struct is not used and > > should therefore be removed > > Well spotted, thanks. > > > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> Applied > > > --- > > drivers/iio/accel/kionix-kx022a.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > > index 1d3af42ec0e1..bb43cb0acce4 100644 > > --- a/drivers/iio/accel/kionix-kx022a.c > > +++ b/drivers/iio/accel/kionix-kx022a.c > > @@ -162,7 +162,6 @@ struct kx022a_data { > > int inc_reg; > > int ien_reg; > > > > - unsigned int g_range; > > unsigned int state; > > unsigned int odr_ns; > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes 2023-01-29 13:37 [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Mehdi Djait 2023-01-29 13:37 ` [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table Mehdi Djait 2023-01-29 13:37 ` [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range Mehdi Djait @ 2023-01-30 8:15 ` Matti Vaittinen 2023-01-31 19:48 ` Mehdi Djait 2 siblings, 1 reply; 10+ messages in thread From: Matti Vaittinen @ 2023-01-30 8:15 UTC (permalink / raw) To: Mehdi Djait, jic23; +Cc: linux-iio 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! ~~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes 2023-01-30 8:15 ` [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Matti Vaittinen @ 2023-01-31 19:48 ` Mehdi Djait 2023-02-01 5:36 ` Matti Vaittinen 0 siblings, 1 reply; 10+ messages in thread From: Mehdi Djait @ 2023-01-31 19:48 UTC (permalink / raw) To: Matti Vaittinen, jic23; +Cc: linux-iio 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes 2023-01-31 19:48 ` Mehdi Djait @ 2023-02-01 5:36 ` Matti Vaittinen 0 siblings, 0 replies; 10+ messages in thread From: Matti Vaittinen @ 2023-02-01 5:36 UTC (permalink / raw) To: Mehdi Djait, jic23; +Cc: linux-iio 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 -- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-02-05 14:42 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-29 13:37 [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Mehdi Djait 2023-01-29 13:37 ` [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table Mehdi Djait 2023-01-30 7:37 ` Matti Vaittinen 2023-01-31 19:32 ` Mehdi Djait 2023-01-29 13:37 ` [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range Mehdi Djait 2023-01-30 8:17 ` Matti Vaittinen 2023-02-05 14:56 ` Jonathan Cameron 2023-01-30 8:15 ` [PATCH 0/2] iio: accel: kionix-kx022a: Minor fixes Matti Vaittinen 2023-01-31 19:48 ` Mehdi Djait 2023-02-01 5:36 ` Matti Vaittinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.