[PATCH 0/3] iio: Fix integration time units for iio-gts

[PATCH 0/3] iio: Fix integration time units for iio-gts

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

The newly added iio-gts-helper (gain time scale helper) errorneously
reports available integration times as micro seconds. The same mistake
is in newly added BU27034 light sensor driver. Fix these by adding new
IIO_VAL type, IIO_VAL_INT_MICRO - which can be used for micro <unit>
values where the integer part is zero.

NOTE:
I did not have the time to test the gts-helpers with integration times
which are greater than 1 second. Currently there is no other in-tree users
besides the bu27034, which does always use integration times smaller
than 1 second. When greater than 1 second times are needed, this needs
to be revised. (Or, when the devm_* interface kunit test support gets
added). Right now this should be a quick fix to integration time
handling before the bug manifests itself in the user-space).

I am planning to test (and re-work if needed) the gts-helpers available
integration time list for > 1Sec times - but I most probably don't have
the time for that during this or next week. (Don't know about the
weekend though - but probably not.)

--

Matti Vaittinen (3):
  iio: core: add IIO_VAL_INT_MICRO
  iio: gts: fix units of available integration times
  iio: bu27034: Fix integration time units

 drivers/iio/industrialio-core.c       | 4 ++++
 drivers/iio/industrialio-gts-helper.c | 2 +-
 drivers/iio/light/rohm-bu27034.c      | 7 +++++--
 include/linux/iio/types.h             | 1 +
 4 files changed, 11 insertions(+), 3 deletions(-)


base-commit: c86b0e73f0bebbb0245ef2bac4cf269d61ff828c
-- 
2.39.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/3] iio: core: add IIO_VAL_INT_MICRO

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2566 bytes --]

There are a few cases like light sensor integration times, where values
returned from *_available() and read_raw() are smaller than 1 and often
in the units of micro. (Like micro second scale integration times,
always smaller than 1 second). Currently those are often handled using
IIO_VAL_INT_PLUS_MICRO, which requires drivers to initialize the integer
part to zero. Furthermore, using IIO_VAL_INT_PLUS_MICRO in iio lists
requires one to always allocate the 'dummy' integer part too.

Introduce IIO_VAL_INT_MICRO which allows omitting the always zero integer.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/industrialio-core.c | 4 ++++
 include/linux/iio/types.h       | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c117f50d0cf3..c5ae965e9961 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -628,6 +628,8 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
 	switch (type) {
 	case IIO_VAL_INT:
 		return sysfs_emit_at(buf, offset, "%d", vals[0]);
+	case IIO_VAL_INT_MICRO:
+		return sysfs_emit_at(buf, offset, "0.%06u", vals[0]);
 	case IIO_VAL_INT_PLUS_MICRO_DB:
 		scale_db = true;
 		fallthrough;
@@ -758,6 +760,7 @@ static ssize_t iio_format_list(char *buf, const int *vals, int type, int length,
 
 	switch (type) {
 	case IIO_VAL_INT:
+	case IIO_VAL_INT_MICRO:
 		stride = 1;
 		break;
 	default:
@@ -952,6 +955,7 @@ static ssize_t iio_write_channel_info(struct device *dev,
 		case IIO_VAL_INT_PLUS_MICRO_DB:
 			scale_db = true;
 			fallthrough;
+		case IIO_VAL_INT_MICRO:
 		case IIO_VAL_INT_PLUS_MICRO:
 			fract_mult = 100000;
 			break;
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 82faa98c719a..b4e316172c7f 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -30,6 +30,7 @@ enum iio_event_info {
 #define IIO_VAL_FRACTIONAL 10
 #define IIO_VAL_FRACTIONAL_LOG2 11
 #define IIO_VAL_CHAR 12
+#define IIO_VAL_INT_MICRO 13 /* val is micro <units>. Integer part is 0 */
 
 enum iio_available_type {
 	IIO_AVAIL_LIST,
-- 
2.39.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] iio: core: add IIO_VAL_INT_MICRO

[Jonathan Cameron]

On Wed, 12 Apr 2023 15:27:14 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> There are a few cases like light sensor integration times, where values
> returned from *_available() and read_raw() are smaller than 1 and often
> in the units of micro. (Like micro second scale integration times,
> always smaller than 1 second). Currently those are often handled using
> IIO_VAL_INT_PLUS_MICRO, which requires drivers to initialize the integer
> part to zero. Furthermore, using IIO_VAL_INT_PLUS_MICRO in iio lists
> requires one to always allocate the 'dummy' integer part too.
> 
> Introduce IIO_VAL_INT_MICRO which allows omitting the always zero integer.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Hi Matti,

I'm not keen on adding yet another case just to avoid having to
have the integer part for IIO_VAL_INT_PLUS_MICRO.
Seems like the wrong trade off of maintainability vs ease of use.

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 4 ++++
>  include/linux/iio/types.h       | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c117f50d0cf3..c5ae965e9961 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -628,6 +628,8 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
>  	switch (type) {
>  	case IIO_VAL_INT:
>  		return sysfs_emit_at(buf, offset, "%d", vals[0]);
> +	case IIO_VAL_INT_MICRO:
> +		return sysfs_emit_at(buf, offset, "0.%06u", vals[0]);
>  	case IIO_VAL_INT_PLUS_MICRO_DB:
>  		scale_db = true;
>  		fallthrough;
> @@ -758,6 +760,7 @@ static ssize_t iio_format_list(char *buf, const int *vals, int type, int length,
>  
>  	switch (type) {
>  	case IIO_VAL_INT:
> +	case IIO_VAL_INT_MICRO:
>  		stride = 1;
>  		break;
>  	default:
> @@ -952,6 +955,7 @@ static ssize_t iio_write_channel_info(struct device *dev,
>  		case IIO_VAL_INT_PLUS_MICRO_DB:
>  			scale_db = true;
>  			fallthrough;
> +		case IIO_VAL_INT_MICRO:
>  		case IIO_VAL_INT_PLUS_MICRO:
>  			fract_mult = 100000;
>  			break;
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 82faa98c719a..b4e316172c7f 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -30,6 +30,7 @@ enum iio_event_info {
>  #define IIO_VAL_FRACTIONAL 10
>  #define IIO_VAL_FRACTIONAL_LOG2 11
>  #define IIO_VAL_CHAR 12
> +#define IIO_VAL_INT_MICRO 13 /* val is micro <units>. Integer part is 0 */
>  
>  enum iio_available_type {
>  	IIO_AVAIL_LIST,

Re: [PATCH 1/3] iio: core: add IIO_VAL_INT_MICRO

[Matti Vaittinen]

Hi Jonathan,

On 4/12/23 23:32, Jonathan Cameron wrote:
> On Wed, 12 Apr 2023 15:27:14 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> There are a few cases like light sensor integration times, where values
>> returned from *_available() and read_raw() are smaller than 1 and often
>> in the units of micro. (Like micro second scale integration times,
>> always smaller than 1 second). Currently those are often handled using
>> IIO_VAL_INT_PLUS_MICRO, which requires drivers to initialize the integer
>> part to zero. Furthermore, using IIO_VAL_INT_PLUS_MICRO in iio lists
>> requires one to always allocate the 'dummy' integer part too.
>>
>> Introduce IIO_VAL_INT_MICRO which allows omitting the always zero integer.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Hi Matti,
> 
> I'm not keen on adding yet another case just to avoid having to
> have the integer part for IIO_VAL_INT_PLUS_MICRO.
> Seems like the wrong trade off of maintainability vs ease of use.

I see your point. I would still argue that adding the IIO_VAL_INT_MICRO 
was not really an intrusive change and I'd expect the maintenance effort 
should not be increased that much.

While the inconvenience for users in read_raw (initializing the *val = 
0) is minor (meaning the benefit of adding IIO_VAL_INT_MICRO is also 
minor in this regard), iio_lists are stronger reason to consider this. 
With IIO_VAL_INT_MICRO the iio-list memory footprint will be halved. In 
my opinion, this benefit would exceed the cost of maintenance effort 
increase - sure thing it's easy for me to say as I am not the maintainer 
;) (And as I wrote, this series was cooked in a hurry - I had no time to 
go through existing drivers to see how many could benefit from the new 
IIO_VAL_INT_MICRO. I may do this later when I get some pretty urgent 
things off my shoulders - assuming you're not opposing this change so 
strongly that this is out of the question no matter how many existing 
users could benefit from IIO_VAL_INT_MICRO).

Anyways, if this is your final stance, then I need to rework the 
integration time list allocations in the gts helper, but I am most 
likely not able to do this until a week or two from now - meaning it 
might be better to revert the bu27034 and iio-gts-helpers until this 
gets fixed. (I reserve the right to do this during some night if I can't 
get sleep though.)

Yours,
	-- Matti

> Jonathan
> 
>> ---
>>   drivers/iio/industrialio-core.c | 4 ++++
>>   include/linux/iio/types.h       | 1 +
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index c117f50d0cf3..c5ae965e9961 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -628,6 +628,8 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
>>   	switch (type) {
>>   	case IIO_VAL_INT:
>>   		return sysfs_emit_at(buf, offset, "%d", vals[0]);
>> +	case IIO_VAL_INT_MICRO:
>> +		return sysfs_emit_at(buf, offset, "0.%06u", vals[0]);
>>   	case IIO_VAL_INT_PLUS_MICRO_DB:
>>   		scale_db = true;
>>   		fallthrough;
>> @@ -758,6 +760,7 @@ static ssize_t iio_format_list(char *buf, const int *vals, int type, int length,
>>   
>>   	switch (type) {
>>   	case IIO_VAL_INT:
>> +	case IIO_VAL_INT_MICRO:
>>   		stride = 1;
>>   		break;
>>   	default:
>> @@ -952,6 +955,7 @@ static ssize_t iio_write_channel_info(struct device *dev,
>>   		case IIO_VAL_INT_PLUS_MICRO_DB:
>>   			scale_db = true;
>>   			fallthrough;
>> +		case IIO_VAL_INT_MICRO:
>>   		case IIO_VAL_INT_PLUS_MICRO:
>>   			fract_mult = 100000;
>>   			break;
>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>> index 82faa98c719a..b4e316172c7f 100644
>> --- a/include/linux/iio/types.h
>> +++ b/include/linux/iio/types.h
>> @@ -30,6 +30,7 @@ enum iio_event_info {
>>   #define IIO_VAL_FRACTIONAL 10
>>   #define IIO_VAL_FRACTIONAL_LOG2 11
>>   #define IIO_VAL_CHAR 12
>> +#define IIO_VAL_INT_MICRO 13 /* val is micro <units>. Integer part is 0 */
>>   
>>   enum iio_available_type {
>>   	IIO_AVAIL_LIST,
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH 1/3] iio: core: add IIO_VAL_INT_MICRO

[Jonathan Cameron]

On Thu, 13 Apr 2023 08:48:08 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Jonathan,
> 
> On 4/12/23 23:32, Jonathan Cameron wrote:
> > On Wed, 12 Apr 2023 15:27:14 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> There are a few cases like light sensor integration times, where values
> >> returned from *_available() and read_raw() are smaller than 1 and often
> >> in the units of micro. (Like micro second scale integration times,
> >> always smaller than 1 second). Currently those are often handled using
> >> IIO_VAL_INT_PLUS_MICRO, which requires drivers to initialize the integer
> >> part to zero. Furthermore, using IIO_VAL_INT_PLUS_MICRO in iio lists
> >> requires one to always allocate the 'dummy' integer part too.
> >>
> >> Introduce IIO_VAL_INT_MICRO which allows omitting the always zero integer.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> > Hi Matti,
> > 
> > I'm not keen on adding yet another case just to avoid having to
> > have the integer part for IIO_VAL_INT_PLUS_MICRO.
> > Seems like the wrong trade off of maintainability vs ease of use.  
> 
> I see your point. I would still argue that adding the IIO_VAL_INT_MICRO 
> was not really an intrusive change and I'd expect the maintenance effort 
> should not be increased that much.

Little things add up. :(

> 
> While the inconvenience for users in read_raw (initializing the *val = 
> 0) is minor (meaning the benefit of adding IIO_VAL_INT_MICRO is also 
> minor in this regard), iio_lists are stronger reason to consider this. 
> With IIO_VAL_INT_MICRO the iio-list memory footprint will be halved. In 
> my opinion, this benefit would exceed the cost of maintenance effort 
> increase - sure thing it's easy for me to say as I am not the maintainer 
> ;) (And as I wrote, this series was cooked in a hurry - I had no time to 
> go through existing drivers to see how many could benefit from the new 
> IIO_VAL_INT_MICRO. I may do this later when I get some pretty urgent 
> things off my shoulders - assuming you're not opposing this change so 
> strongly that this is out of the question no matter how many existing 
> users could benefit from IIO_VAL_INT_MICRO).
> 
> Anyways, if this is your final stance, then I need to rework the 
> integration time list allocations in the gts helper, but I am most 
> likely not able to do this until a week or two from now - meaning it 
> might be better to revert the bu27034 and iio-gts-helpers until this 
> gets fixed. (I reserve the right to do this during some night if I can't 
> get sleep though.)

I don't think I'll be convinced on this for a couple of reasons:
1) It will inevitably lead to IIO_VAL_INT_NANO etc
2) There are other places this matters such as consumer drivers that
   have to cope with a bunch of possible return types from accessing
   read_raw and write_raw via the in kernel interfaces.
   For example see drivers/iio/afe/iio-rescale.c 

That might feel like it doesn't matter for your device (which is true)
we would want wide use and that means a bunch of special casing
in the more generic consumer drivers and potentially even in the
specific ones that are scattered in various other kernel subsystems.

Hence please rework this to add the annoying 0s.

Thanks,

Jonathan


> 
> Yours,
> 	-- Matti
> 
> > Jonathan
> >   
> >> ---
> >>   drivers/iio/industrialio-core.c | 4 ++++
> >>   include/linux/iio/types.h       | 1 +
> >>   2 files changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> >> index c117f50d0cf3..c5ae965e9961 100644
> >> --- a/drivers/iio/industrialio-core.c
> >> +++ b/drivers/iio/industrialio-core.c
> >> @@ -628,6 +628,8 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
> >>   	switch (type) {
> >>   	case IIO_VAL_INT:
> >>   		return sysfs_emit_at(buf, offset, "%d", vals[0]);
> >> +	case IIO_VAL_INT_MICRO:
> >> +		return sysfs_emit_at(buf, offset, "0.%06u", vals[0]);
> >>   	case IIO_VAL_INT_PLUS_MICRO_DB:
> >>   		scale_db = true;
> >>   		fallthrough;
> >> @@ -758,6 +760,7 @@ static ssize_t iio_format_list(char *buf, const int *vals, int type, int length,
> >>   
> >>   	switch (type) {
> >>   	case IIO_VAL_INT:
> >> +	case IIO_VAL_INT_MICRO:
> >>   		stride = 1;
> >>   		break;
> >>   	default:
> >> @@ -952,6 +955,7 @@ static ssize_t iio_write_channel_info(struct device *dev,
> >>   		case IIO_VAL_INT_PLUS_MICRO_DB:
> >>   			scale_db = true;
> >>   			fallthrough;
> >> +		case IIO_VAL_INT_MICRO:
> >>   		case IIO_VAL_INT_PLUS_MICRO:
> >>   			fract_mult = 100000;
> >>   			break;
> >> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> >> index 82faa98c719a..b4e316172c7f 100644
> >> --- a/include/linux/iio/types.h
> >> +++ b/include/linux/iio/types.h
> >> @@ -30,6 +30,7 @@ enum iio_event_info {
> >>   #define IIO_VAL_FRACTIONAL 10
> >>   #define IIO_VAL_FRACTIONAL_LOG2 11
> >>   #define IIO_VAL_CHAR 12
> >> +#define IIO_VAL_INT_MICRO 13 /* val is micro <units>. Integer part is 0 */
> >>   
> >>   enum iio_available_type {
> >>   	IIO_AVAIL_LIST,  
> >   
> 

[PATCH 2/3] iio: gts: fix units of available integration times

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

The available integration times should be in the units of seconds, not
micro seconds.

Use the new IIO_VAL_INT_MICRO to return times in seconds

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/industrialio-gts-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 8bb68975b259..e242d0eb8b94 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -683,7 +683,7 @@ int iio_gts_avail_times(struct iio_gts *gts,  const int **vals, int *type,
 		return -EINVAL;
 
 	*vals = gts->avail_time_tables;
-	*type = IIO_VAL_INT;
+	*type = IIO_VAL_INT_MICRO;
 	*length = gts->num_avail_time_tables;
 
 	return IIO_AVAIL_LIST;
-- 
2.39.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] iio: bu27034: Fix integration time units

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

The integration time was presented by BU27034 in micro seconds. The ABI
documentation says this should be in seconds.

Fix integration time to be in seconds.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/light/rohm-bu27034.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
index e486dcf35eba..6044db52abfa 100644
--- a/drivers/iio/light/rohm-bu27034.c
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -1171,7 +1171,7 @@ static int bu27034_read_raw(struct iio_dev *idev,
 		if (*val < 0)
 			return *val;
 
-		return IIO_VAL_INT;
+		return IIO_VAL_INT_MICRO;
 
 	case IIO_CHAN_INFO_SCALE:
 		return bu27034_get_scale(data, chan->channel, val, val2);
@@ -1229,7 +1229,10 @@ static int bu27034_write_raw(struct iio_dev *idev,
 		ret = bu27034_set_scale(data, chan->channel, val, val2);
 		break;
 	case IIO_CHAN_INFO_INT_TIME:
-		ret = bu27034_try_set_int_time(data, val);
+		if (val)
+			return -EINVAL;
+
+		ret = bu27034_try_set_int_time(data, val2);
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.39.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]