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

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

[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

[PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table

[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

[PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range

[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

Re: [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table

[Matti Vaittinen]

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

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

[Matti Vaittinen]

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

Re: [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range

[Matti Vaittinen]

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

Re: [PATCH 1/2] iio: accel: kionix-kx022a: Fix the setting of regmap_config rd_table and wr_table

[Mehdi Djait]

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

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

[Mehdi Djait]

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

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

[Matti Vaittinen]

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

Re: [PATCH 2/2] iio: accel: kionix-kx022a: Remove the unused member g_range

[Jonathan Cameron]

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;
> >     
>