ROHM BM1390 Pressure sensor (BM1390GLV-Z) can measure pressures ranging
from 300 hPa to 1300 hPa with configurable measurement averaging and an
internal FIFO. The sensor does also provide temperature measurements
although, according to the data sheet, sensor performs internal
temperature compensation for the MEMS.
Sensor does also contain IIR filter implemented in HW. The data-sheet
says the IIR filter can be configured to be "weak", "middle" or
"strong". Some RMS noise figures are provided in data sheet but no
accurate maths for the filter configurations is provided.
I actually asked if we can define 3db frequencies corresponding to these
IIR filter settings - and I received values 0.452Hz, 0.167Hz, and 0.047Hz
but I am not at all sure we understood each others with the HW
colleagues... Hence, the IIR filter configuration is not supported by this
driver and the filter is just configured to the "middle" setting.
(at least for now)
It would also be possible to not use IIR filter but just do some simple
averaging. I wonder if it would make sense to implement the OVERSAMPLING
value setting so that if this value is written, IIR filter is disabled and
number of samples to be averaged is set to value requested by
OVERSAMPLING. The data-sheet has a mention that if IIR is used, the
number of averaged samples must be set to a fixed value.
The FIFO measurement mode (in sensor hardware) is only measuring the
pressure and not the temperature. The driver measures temperature when
FIFO is flushed and simply uses the same measured temperature value to
all reported temperatures. This should not be a problem when temperature
is not changing very rapidly (several degrees C / second) but allows users
to get the temperature measurements from sensor without any additional
logic.
This driver has received limited amount of testing this far. It's in a
state 'works on my machine, for my use cases' - and all feedback is
appreciated!
Revision history:
Major changes here, please see the head room of individual patches for
more detailed list.
v2 => v3:
rebased on v6.6-rc2
added three IIO fixup patches so numbering of patches changed
dt-bindings/MAINTAINERS: No changes
bm1390 driver:
- various cleanups and fixes
- do not disable IRQ
- fix temperature reading when FIFO is used
- separate buffer and trigger initialization
v1 => v2:
rebased on v6.6-rc1
dt-bindings:
- fix compatible in the example
sensor driver:
- drop unnecessary write_raw callback
- plenty of small improvements and fixes
MAINTAINERS:
- No changes
Matti Vaittinen (6):
tools: iio: iio_generic_buffer ensure alignment
iio: improve doc for available_scan_mask
iio: try searching for exact scan_mask
dt-bindings: Add ROHM BM1390 pressure sensor
iio: pressure: Support ROHM BU1390
MAINTAINERS: Add ROHM BM1390
.../bindings/iio/pressure/rohm,bm1390.yaml | 52 +
MAINTAINERS | 6 +
drivers/iio/industrialio-buffer.c | 25 +-
drivers/iio/pressure/Kconfig | 9 +
drivers/iio/pressure/Makefile | 1 +
drivers/iio/pressure/rohm-bm1390.c | 930 ++++++++++++++++++
include/linux/iio/iio.h | 4 +-
tools/iio/iio_generic_buffer.c | 15 +-
8 files changed, 1034 insertions(+), 8 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
create mode 100644 drivers/iio/pressure/rohm-bm1390.c
base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70
--
2.41.0
--
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 =]
BM1390GLV-Z is a pressure sensor which performs internal temperature
compensation for the MEMS. Pressure range is from 300 hPa to 1300 hPa
and sample averaging and IIR filtering is built in sensor. Temperature
measurement is also supported.
Add dt-bindings for the sensor.
Signed-off-by: Matti Vaittinen <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
Revision history:
v1 => v2:
- Improve spelling in description
- Drop unnecessary '|'
- Use correct example (fix compatible)
---
.../bindings/iio/pressure/rohm,bm1390.yaml | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
diff --git a/Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml b/Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
new file mode 100644
index 000000000000..7c4ca6322bf8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/rohm,bm1390.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BM1390 pressure sensor
+
+maintainers:
+ - Matti Vaittinen <[email protected]>
+
+description:
+ BM1390GLV-Z is a pressure sensor which performs internal temperature
+ compensation for the MEMS. Pressure range is from 300 hPa to 1300 hPa
+ and sample averaging and IIR filtering is built in. Temperature
+ measurement is also supported.
+
+properties:
+ compatible:
+ const: rohm,bm1390glv-z
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ vdd-supply: true
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pressure-sensor@5d {
+ compatible = "rohm,bm1390glv-z";
+ reg = <0x5d>;
+
+ interrupt-parent = <&gpio1>;
+ interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
+
+ vdd-supply = <&vdd>;
+ };
+ };
--
2.41.0
--
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 =]
The iio_generic_buffer can return garbage values when the total size of
scan data is not a multiple of largest element in the scan. This can be
demonstrated by reading a scan consisting for example of one 4 byte and
one 2 byte element, where the 4 byte elemnt is first in the buffer.
The IIO generic buffert code does not take into accunt the last two
padding bytes that are needed to ensure that the 4byte data for next
scan is correctly aligned.
Add padding bytes required to align the next sample into the scan size.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Please note, This one could have RFC in subject.:
I attempted to write the fix so that the alignment is done based on the
biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment
should be used instead? This patch can be dropped from the series if the
fix is not correct / agreed.
tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index 44bbf80f0cfd..fc562799a109 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -54,9 +54,12 @@ enum autochan {
static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
{
unsigned int bytes = 0;
- int i = 0;
+ int i = 0, max = 0;
+ unsigned int misalignment;
while (i < num_channels) {
+ if (channels[i].bytes > max)
+ max = channels[i].bytes;
if (bytes % channels[i].bytes == 0)
channels[i].location = bytes;
else
@@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
bytes = channels[i].location + channels[i].bytes;
i++;
}
+ /*
+ * We wan't the data in next sample to also be properly aligned so
+ * we'll add padding at the end if needed. TODO: should we use fixed
+ * 8 byte alignment instead of the size of the biggest samnple?
+ */
+ misalignment = bytes % max;
+ if (misalignment) {
+ printf("Misalignment %u. Adding Padding %u\n", misalignment, max - misalignment);
+ bytes += max - misalignment;
+ }
return bytes;
}
--
2.41.0
--
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 =]
On Fri, 22 Sep 2023 14:16:08 +0300
Matti Vaittinen <[email protected]> wrote:
> The iio_generic_buffer can return garbage values when the total size of
> scan data is not a multiple of largest element in the scan. This can be
> demonstrated by reading a scan consisting for example of one 4 byte and
> one 2 byte element, where the 4 byte elemnt is first in the buffer.
>
> The IIO generic buffert code does not take into accunt the last two
> padding bytes that are needed to ensure that the 4byte data for next
> scan is correctly aligned.
>
> Add padding bytes required to align the next sample into the scan size.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
> Please note, This one could have RFC in subject.:
> I attempted to write the fix so that the alignment is done based on the
> biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment
> should be used instead? This patch can be dropped from the series if the
> fix is not correct / agreed.
>
> tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index 44bbf80f0cfd..fc562799a109 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -54,9 +54,12 @@ enum autochan {
> static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
> {
> unsigned int bytes = 0;
> - int i = 0;
> + int i = 0, max = 0;
> + unsigned int misalignment;
>
> while (i < num_channels) {
> + if (channels[i].bytes > max)
> + max = channels[i].bytes;
> if (bytes % channels[i].bytes == 0)
> channels[i].location = bytes;
> else
> @@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
> bytes = channels[i].location + channels[i].bytes;
> i++;
> }
> + /*
> + * We wan't the data in next sample to also be properly aligned so
> + * we'll add padding at the end if needed. TODO: should we use fixed
> + * 8 byte alignment instead of the size of the biggest samnple?
> + */
Should be aligned to max size seen in the scan.
> + misalignment = bytes % max;
> + if (misalignment) {
> + printf("Misalignment %u. Adding Padding %u\n", misalignment, max - misalignment);
No print statement as this is correct behaviour (well the tool is buggy but the kernel generates it
correctly I believe). Fine to add a comment though!
> + bytes += max - misalignment;
> + }
>
> return bytes;
> }
On Fri, 22 Sep 2023 14:14:52 +0300
Matti Vaittinen <[email protected]> wrote:
> ROHM BM1390 Pressure sensor (BM1390GLV-Z) can measure pressures ranging
> from 300 hPa to 1300 hPa with configurable measurement averaging and an
> internal FIFO. The sensor does also provide temperature measurements
> although, according to the data sheet, sensor performs internal
> temperature compensation for the MEMS.
>
> Sensor does also contain IIR filter implemented in HW. The data-sheet
> says the IIR filter can be configured to be "weak", "middle" or
> "strong". Some RMS noise figures are provided in data sheet but no
> accurate maths for the filter configurations is provided.
>
> I actually asked if we can define 3db frequencies corresponding to these
> IIR filter settings - and I received values 0.452Hz, 0.167Hz, and 0.047Hz
> but I am not at all sure we understood each others with the HW
> colleagues... Hence, the IIR filter configuration is not supported by this
> driver and the filter is just configured to the "middle" setting.
> (at least for now)
>
> It would also be possible to not use IIR filter but just do some simple
> averaging. I wonder if it would make sense to implement the OVERSAMPLING
> value setting so that if this value is written, IIR filter is disabled and
> number of samples to be averaged is set to value requested by
> OVERSAMPLING. The data-sheet has a mention that if IIR is used, the
> number of averaged samples must be set to a fixed value.
>
> The FIFO measurement mode (in sensor hardware) is only measuring the
> pressure and not the temperature. The driver measures temperature when
> FIFO is flushed and simply uses the same measured temperature value to
> all reported temperatures. This should not be a problem when temperature
> is not changing very rapidly (several degrees C / second) but allows users
> to get the temperature measurements from sensor without any additional
> logic.
>
> This driver has received limited amount of testing this far. It's in a
> state 'works on my machine, for my use cases' - and all feedback is
> appreciated!
At somepoint we'll just have to decide it's enough. To be honest most
drivers in IIO get testing along those lines and we find bugs years
later when someone tries something a little different!
>
> Revision history:
> Major changes here, please see the head room of individual patches for
> more detailed list.
> v2 => v3:
> rebased on v6.6-rc2
Stick to rc1 though I doubt it makes much difference. The IIO tree
will remain based on v6.6-rc1 until it is rebased after Greg takes
a pull request.
Jonathan
> added three IIO fixup patches so numbering of patches changed
> dt-bindings/MAINTAINERS: No changes
> bm1390 driver:
> - various cleanups and fixes
> - do not disable IRQ
> - fix temperature reading when FIFO is used
> - separate buffer and trigger initialization
>
> v1 => v2:
> rebased on v6.6-rc1
> dt-bindings:
> - fix compatible in the example
> sensor driver:
> - drop unnecessary write_raw callback
> - plenty of small improvements and fixes
> MAINTAINERS:
> - No changes
>
> Matti Vaittinen (6):
> tools: iio: iio_generic_buffer ensure alignment
> iio: improve doc for available_scan_mask
> iio: try searching for exact scan_mask
> dt-bindings: Add ROHM BM1390 pressure sensor
> iio: pressure: Support ROHM BU1390
> MAINTAINERS: Add ROHM BM1390
>
> .../bindings/iio/pressure/rohm,bm1390.yaml | 52 +
> MAINTAINERS | 6 +
> drivers/iio/industrialio-buffer.c | 25 +-
> drivers/iio/pressure/Kconfig | 9 +
> drivers/iio/pressure/Makefile | 1 +
> drivers/iio/pressure/rohm-bm1390.c | 930 ++++++++++++++++++
> include/linux/iio/iio.h | 4 +-
> tools/iio/iio_generic_buffer.c | 15 +-
> 8 files changed, 1034 insertions(+), 8 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/pressure/rohm,bm1390.yaml
> create mode 100644 drivers/iio/pressure/rohm-bm1390.c
>
>
> base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70
Add myself as a maintainer for ROHM BM1390 pressure sensor driver.
Signed-off-by: Matti Vaittinen <[email protected]>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index bf0f54c24f81..2f296320d642 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18562,6 +18562,12 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/light/bh1750.yaml
F: drivers/iio/light/bh1750.c
+ROHM BM1390 PRESSURE SENSOR DRIVER
+M: Matti Vaittinen <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/iio/pressure/rohm-bm1390.c
+
ROHM BU270xx LIGHT SENSOR DRIVERs
M: Matti Vaittinen <[email protected]>
L: [email protected]
--
2.41.0
--
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 =]
On 9/24/23 18:57, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:16:08 +0300
> Matti Vaittinen <[email protected]> wrote:
>
>> The iio_generic_buffer can return garbage values when the total size of
>> scan data is not a multiple of largest element in the scan. This can be
>> demonstrated by reading a scan consisting for example of one 4 byte and
>> one 2 byte element, where the 4 byte elemnt is first in the buffer.
>>
>> The IIO generic buffert code does not take into accunt the last two
>> padding bytes that are needed to ensure that the 4byte data for next
>> scan is correctly aligned.
>>
>> Add padding bytes required to align the next sample into the scan size.
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>> ---
>> Please note, This one could have RFC in subject.:
>> I attempted to write the fix so that the alignment is done based on the
>> biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment
>> should be used instead? This patch can be dropped from the series if the
>> fix is not correct / agreed.
>>
>> tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
>> index 44bbf80f0cfd..fc562799a109 100644
>> --- a/tools/iio/iio_generic_buffer.c
>> +++ b/tools/iio/iio_generic_buffer.c
>> @@ -54,9 +54,12 @@ enum autochan {
>> static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
>> {
>> unsigned int bytes = 0;
>> - int i = 0;
>> + int i = 0, max = 0;
>> + unsigned int misalignment;
>>
>> while (i < num_channels) {
>> + if (channels[i].bytes > max)
>> + max = channels[i].bytes;
>> if (bytes % channels[i].bytes == 0)
>> channels[i].location = bytes;
>> else
>> @@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
>> bytes = channels[i].location + channels[i].bytes;
>> i++;
>> }
>> + /*
>> + * We wan't the data in next sample to also be properly aligned so
>> + * we'll add padding at the end if needed. TODO: should we use fixed
>> + * 8 byte alignment instead of the size of the biggest samnple?
>> + */
>
> Should be aligned to max size seen in the scan.
Or, maybe it should be
min(max_size_in_scan, 8);
?
I think my suggestion above may yield undesirable effects should the
scan elements be greater than 8 bytes. (Don't know if this is supported
though)
>
>> + misalignment = bytes % max;
>> + if (misalignment) {
>> + printf("Misalignment %u. Adding Padding %u\n", misalignment, max - misalignment);
>
> No print statement as this is correct behaviour (well the tool is buggy but the kernel generates it
> correctly I believe). Fine to add a comment though!
Oh, indeed. The print was forgotten from my test runs. Thanks for
pointing it out!
>
>> + bytes += max - misalignment;
>> + }
>>
>> return bytes;
>> }
>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 9/24/23 18:53, Jonathan Cameron wrote:
> On Fri, 22 Sep 2023 14:14:52 +0300
> Matti Vaittinen <[email protected]> wrote:
>>
>> Revision history:
>> Major changes here, please see the head room of individual patches for
>> more detailed list.
>> v2 => v3:
>> rebased on v6.6-rc2
>
> Stick to rc1 though I doubt it makes much difference.
It shouldn't probably change much - but it 's still good to know -rc1 is
what you'll stick at :) Thanks for pointing it out.
> The IIO tree
> will remain based on v6.6-rc1 until it is rebased after Greg takes
> a pull request.
>
Yours,
--Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 9/25/23 16:16, Jonathan Cameron wrote:
> On Mon, 25 Sep 2023 10:01:09 +0300
> Matti Vaittinen <[email protected]> wrote:
>
>> On 9/24/23 18:57, Jonathan Cameron wrote:
>>> On Fri, 22 Sep 2023 14:16:08 +0300
>>> Matti Vaittinen <[email protected]> wrote:
>>>
>>>> The iio_generic_buffer can return garbage values when the total size of
>>>> scan data is not a multiple of largest element in the scan. This can be
>>>> demonstrated by reading a scan consisting for example of one 4 byte and
>>>> one 2 byte element, where the 4 byte elemnt is first in the buffer.
>>>>
>>>> The IIO generic buffert code does not take into accunt the last two
>>>> padding bytes that are needed to ensure that the 4byte data for next
>>>> scan is correctly aligned.
>>>>
>>>> Add padding bytes required to align the next sample into the scan size.
>>>>
>>>> Signed-off-by: Matti Vaittinen <[email protected]>
>>>> ---
>>>> Please note, This one could have RFC in subject.:
>>>> I attempted to write the fix so that the alignment is done based on the
>>>> biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment
>>>> should be used instead? This patch can be dropped from the series if the
>>>> fix is not correct / agreed.
>>>>
>>>> tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
>>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
>>>> index 44bbf80f0cfd..fc562799a109 100644
>>>> --- a/tools/iio/iio_generic_buffer.c
>>>> +++ b/tools/iio/iio_generic_buffer.c
>>>> @@ -54,9 +54,12 @@ enum autochan {
>>>> static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
>>>> {
>>>> unsigned int bytes = 0;
>>>> - int i = 0;
>>>> + int i = 0, max = 0;
>>>> + unsigned int misalignment;
>>>>
>>>> while (i < num_channels) {
>>>> + if (channels[i].bytes > max)
>>>> + max = channels[i].bytes;
>>>> if (bytes % channels[i].bytes == 0)
>>>> channels[i].location = bytes;
>>>> else
>>>> @@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
>>>> bytes = channels[i].location + channels[i].bytes;
>>>> i++;
>>>> }
>>>> + /*
>>>> + * We wan't the data in next sample to also be properly aligned so
>>>> + * we'll add padding at the end if needed. TODO: should we use fixed
>>>> + * 8 byte alignment instead of the size of the biggest samnple?
>>>> + */
>>>
>>> Should be aligned to max size seen in the scan.
>>
>> Or, maybe it should be
>> min(max_size_in_scan, 8);
>> ?
>
> Definitely not. If you are grabbing just one channel of 8 bit data,
> we want it to be tightly packed.
I think that in this case the max_size_in_scan would be 1, and min(1, 8)
would be 1 as well, resulting a tightly packed data. I am just wondering
if we should use 8 as maximum alignment - eg, if our scan has 16 bytes
data + 1 byte data, we would add 7 bytes of padding, not 15 bytes of
padding. I am not sure what is the right thing to do.
> If we have a bug that already made that true then we might be stuck
> with it, but I'm fairly sure we don't.
>>
>> I think my suggestion above may yield undesirable effects should the
>> scan elements be greater than 8 bytes. (Don't know if this is supported
>> though)
>
> It is supported in theory, in practice not seen one yet.
So, whether to unconditionally use largest scan element sized alignment
- or largest scan element up to 8 bytes - is a question we haven't hit
yet :)
Actually, more I stare at the alignment code here, less sure I am it is
correct - but maybe I don't understand how the data should be aligned.
I think it works if allowed data sizes are 1, 2, 4, and 8. However, I
suspect it breaks for other sizes.
For non power of2 sizes, the alignment code will result strange
alignments. For example, scan consisting of two 6-byte elements would be
packed - meaning the second element would probably break the alignment
rules by starting from address '6'. I think that on most architectures
the proper access would require 2 padding bytes to be added at the end
of the first sample. Current code wouldn't do that.
If we allow only power of 2 sizes - I would expect a scan consisting of
a 8 byte element followed by a 16 byte element to be tightly packed. I'd
assume that for the 16 byte data, it'd be enough to ensure 8 byte
alignment. Current code would however add 8 bytes of padding at the end
of the first 8 byte element to make the 16 byte scan element to be
aligned at 16 byte address. To my uneducated mind this is not needed -
but maybe I just don't know what I am writing about :)
In any case, the patch here should fix things when allowed scan element
sizes are 1, 2, 4 and 8 and we have to add padding after last scan
element. It won't work for other sizes, but as I wrote, I suspect the
whole alignment code here may be broken for other sizes so things
shouldn't at least get worse with this patch... I think this should be
revised if we see samples of other sizes - and in any case, this might
at least warrant a comment here :) (I reserve a right to be wrong.
Haven't been sleeping too well lately and my head is humming...)
>>>> + misalignment = bytes % max;
>>>> + if (misalignment) {
>>>> + printf("Misalignment %u. Adding Padding %u\n", misalignment, max - misalignment);
>>>
>>> No print statement as this is correct behaviour (well the tool is buggy but the kernel generates it
>>> correctly I believe). Fine to add a comment though!
>>
>> Oh, indeed. The print was forgotten from my test runs. Thanks for
>> pointing it out!
>>
>>>
>>>> + bytes += max - misalignment;
>>>> + }
>>>>
>>>> return bytes;
>>>> }
>>>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Tue, 26 Sep 2023 13:29:02 +0300
Matti Vaittinen <[email protected]> wrote:
> On 9/25/23 16:16, Jonathan Cameron wrote:
> > On Mon, 25 Sep 2023 10:01:09 +0300
> > Matti Vaittinen <[email protected]> wrote:
> >
> >> On 9/24/23 18:57, Jonathan Cameron wrote:
> >>> On Fri, 22 Sep 2023 14:16:08 +0300
> >>> Matti Vaittinen <[email protected]> wrote:
> >>>
> >>>> The iio_generic_buffer can return garbage values when the total size of
> >>>> scan data is not a multiple of largest element in the scan. This can be
> >>>> demonstrated by reading a scan consisting for example of one 4 byte and
> >>>> one 2 byte element, where the 4 byte elemnt is first in the buffer.
> >>>>
> >>>> The IIO generic buffert code does not take into accunt the last two
> >>>> padding bytes that are needed to ensure that the 4byte data for next
> >>>> scan is correctly aligned.
> >>>>
> >>>> Add padding bytes required to align the next sample into the scan size.
> >>>>
> >>>> Signed-off-by: Matti Vaittinen <[email protected]>
> >>>> ---
> >>>> Please note, This one could have RFC in subject.:
> >>>> I attempted to write the fix so that the alignment is done based on the
> >>>> biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment
> >>>> should be used instead? This patch can be dropped from the series if the
> >>>> fix is not correct / agreed.
> >>>>
> >>>> tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
> >>>> 1 file changed, 14 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> >>>> index 44bbf80f0cfd..fc562799a109 100644
> >>>> --- a/tools/iio/iio_generic_buffer.c
> >>>> +++ b/tools/iio/iio_generic_buffer.c
> >>>> @@ -54,9 +54,12 @@ enum autochan {
> >>>> static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
> >>>> {
> >>>> unsigned int bytes = 0;
> >>>> - int i = 0;
> >>>> + int i = 0, max = 0;
> >>>> + unsigned int misalignment;
> >>>>
> >>>> while (i < num_channels) {
> >>>> + if (channels[i].bytes > max)
> >>>> + max = channels[i].bytes;
> >>>> if (bytes % channels[i].bytes == 0)
> >>>> channels[i].location = bytes;
> >>>> else
> >>>> @@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
> >>>> bytes = channels[i].location + channels[i].bytes;
> >>>> i++;
> >>>> }
> >>>> + /*
> >>>> + * We wan't the data in next sample to also be properly aligned so
> >>>> + * we'll add padding at the end if needed. TODO: should we use fixed
> >>>> + * 8 byte alignment instead of the size of the biggest samnple?
> >>>> + */
> >>>
> >>> Should be aligned to max size seen in the scan.
> >>
> >> Or, maybe it should be
> >> min(max_size_in_scan, 8);
> >> ?
> >
> > Definitely not. If you are grabbing just one channel of 8 bit data,
> > we want it to be tightly packed.
>
> I think that in this case the max_size_in_scan would be 1, and min(1, 8)
> would be 1 as well, resulting a tightly packed data. I am just wondering
> if we should use 8 as maximum alignment - eg, if our scan has 16 bytes
> data + 1 byte data, we would add 7 bytes of padding, not 15 bytes of
> padding. I am not sure what is the right thing to do.
Ah I read that backwards as you've noticed.
We don't have any such big channels, so indeed have some flexibility here.
I think we stick to naturally aligned so 16 bytes case would be 16 byte
aligned - mostly because I don't expect to see one any time soon and
because it makes the docs simpler.
>
> > If we have a bug that already made that true then we might be stuck
> > with it, but I'm fairly sure we don't.
> >>
> >> I think my suggestion above may yield undesirable effects should the
> >> scan elements be greater than 8 bytes. (Don't know if this is supported
> >> though)
> >
> > It is supported in theory, in practice not seen one yet.
>
> So, whether to unconditionally use largest scan element sized alignment
> - or largest scan element up to 8 bytes - is a question we haven't hit
> yet :)
>
> Actually, more I stare at the alignment code here, less sure I am it is
> correct - but maybe I don't understand how the data should be aligned.
>
> I think it works if allowed data sizes are 1, 2, 4, and 8. However, I
> suspect it breaks for other sizes.
Indeed - it's meant to be power of 2 only. More than possible we don't check
that rigorously enough or have it clearly documented though.
The aim is for the data to be padded for efficient accesses
+ because it is a pain to deal with arbitrary padding so we restrict
it to power of 2 naturally aligned only. One relaxation we've talked
about in the past is packing multiple channels per byte (for logic
analyser cases). Not done it yet though.
>
> For non power of2 sizes, the alignment code will result strange
> alignments. For example, scan consisting of two 6-byte elements would be
> packed - meaning the second element would probably break the alignment
> rules by starting from address '6'. I think that on most architectures
> the proper access would require 2 padding bytes to be added at the end
> of the first sample. Current code wouldn't do that.
>
> If we allow only power of 2 sizes - I would expect a scan consisting of
> a 8 byte element followed by a 16 byte element to be tightly packed. I'd
> assume that for the 16 byte data, it'd be enough to ensure 8 byte
> alignment. Current code would however add 8 bytes of padding at the end
> of the first 8 byte element to make the 16 byte scan element to be
> aligned at 16 byte address. To my uneducated mind this is not needed -
> but maybe I just don't know what I am writing about :)
16 byte alignement is probably not needed - but who knows for future
architecture. Note this has been really non obvious in the past and
is why we force alignment for timestamp elements in lots of drivers.
Most architectures align an s64 in a c structure to an 8 byte boundary
but x86 32 bit doesn't. Hence we have to force it all over the place.
Fixing that (because userspace ABI data placement should not vary across
architectures) was a pain. I don't want to do it again!
>
> In any case, the patch here should fix things when allowed scan element
> sizes are 1, 2, 4 and 8 and we have to add padding after last scan
> element. It won't work for other sizes, but as I wrote, I suspect the
> whole alignment code here may be broken for other sizes so things
> shouldn't at least get worse with this patch... I think this should be
> revised if we see samples of other sizes - and in any case, this might
> at least warrant a comment here :) (I reserve a right to be wrong.
> Haven't been sleeping too well lately and my head is humming...)
Might be worth a power of 2 only comment.
J