* [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
@ 2026-05-24 8:53 ` Jinseob Kim
2026-05-24 9:03 ` sashiko-bot
` (2 more replies)
2026-05-24 8:53 ` [PATCH RFC v2 2/7] Documentation: iio: add Open Sensor Fusion protocol v0 reference Jinseob Kim
` (6 subsequent siblings)
7 siblings, 3 replies; 30+ messages in thread
From: Jinseob Kim @ 2026-05-24 8:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel,
Jinseob Kim
Add a binding for the OSF0 UART-attached sensor aggregation device.
Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
---
.../iio/imu/opensensorfusion,osf-uart.yaml | 33 +++++++++++++++++++
.../devicetree/bindings/vendor-prefixes.yaml | 2 ++
2 files changed, 35 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
diff --git a/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml b/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
new file mode 100644
index 000000000..d4a8f6819
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/imu/opensensorfusion,osf-uart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Open Sensor Fusion UART sensor aggregation device
+
+maintainers:
+ - Jinseob Kim <kimjinseob88@gmail.com>
+
+description:
+ UART-attached sensor aggregation device using the OSF0 frame format.
+ The device sends capability, status, and sample frames to the host.
+
+properties:
+ compatible:
+ const: opensensorfusion,osf-uart
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ serial {
+ osf {
+ compatible = "opensensorfusion,osf-uart";
+ };
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 28784d66a..121b83f70 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1239,6 +1239,8 @@ patternProperties:
description: OpenRISC.io
"^openwrt,.*":
description: OpenWrt
+ "^opensensorfusion,.*":
+ description: Open Sensor Fusion project
"^option,.*":
description: Option NV
"^oranth,.*":
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device
2026-05-24 8:53 ` [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device Jinseob Kim
@ 2026-05-24 9:03 ` sashiko-bot
2026-05-24 18:59 ` Krzysztof Kozlowski
2026-05-24 19:34 ` Conor Dooley
2 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-05-24 9:03 UTC (permalink / raw)
To: Jinseob Kim; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding lacks a reference to standard serial peripheral properties, restricting valid UART configurations.
- [Low] The new vendor prefix 'opensensorfusion' is inserted out of alphabetical order in vendor-prefixes.yaml.
- [Low] The device tree example uses a specific, non-generic node name ('osf').
--
commit d7438989beeea74dd5628fa3759c1cbb1ffb5900
Author: Jinseob Kim <kimjinseob88@gmail.com>
dt-bindings: iio: add Open Sensor Fusion UART device
This commit adds a device tree binding for the OSF0 UART-attached sensor
aggregation device. The device communicates capability, status, and sample
frames to the host via a serial connection.
> diff --git a/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml b/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
> new file mode 100644
> index 0000000000000..d4a8f68192e9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
[ ... ]
> +required:
> + - compatible
> +
> +additionalProperties: false
[Severity: Medium]
Since this is a UART-attached device, will board device trees need to specify
standard serial properties like current-speed? Without a $ref to
/schemas/serial/serial-peripheral-props.yaml and setting
unevaluatedProperties: false instead, setting standard UART properties will
cause schema validation to fail.
> +
> +examples:
> + - |
> + serial {
> + osf {
[Severity: Low]
This isn't a bug, but does this follow the devicetree specification for generic
node names? Usually, node names should reflect the primary function of the
device, such as sensor or imu, rather than the specific product name.
> + compatible = "opensensorfusion,osf-uart";
> + };
> + };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 28784d66ae7ba..121b83f7046ad 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1239,6 +1239,8 @@ patternProperties:
> description: OpenRISC.io
> "^openwrt,.*":
> description: OpenWrt
> + "^opensensorfusion,.*":
> + description: Open Sensor Fusion project
[Severity: Low]
This isn't a bug, but is this vendor prefix inserted out of alphabetical
order? "opensensorfusion" should alphabetically come before "openwrt".
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524085312.15369-1-kimjinseob88@gmail.com?part=1
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device
2026-05-24 8:53 ` [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device Jinseob Kim
2026-05-24 9:03 ` sashiko-bot
@ 2026-05-24 18:59 ` Krzysztof Kozlowski
2026-05-24 19:34 ` Conor Dooley
2 siblings, 0 replies; 30+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-24 18:59 UTC (permalink / raw)
To: Jinseob Kim, Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel
On 24/05/2026 10:53, Jinseob Kim wrote:
> Add a binding for the OSF0 UART-attached sensor aggregation device.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
> ---
> .../iio/imu/opensensorfusion,osf-uart.yaml | 33 +++++++++++++++++++
> .../devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 2 files changed, 35 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml b/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
> new file mode 100644
> index 000000000..d4a8f6819
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/imu/opensensorfusion,osf-uart.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Open Sensor Fusion UART sensor aggregation device
> +
> +maintainers:
> + - Jinseob Kim <kimjinseob88@gmail.com>
> +
> +description:
> + UART-attached sensor aggregation device using the OSF0 frame format.
> + The device sends capability, status, and sample frames to the host.
> +
> +properties:
> + compatible:
> + const: opensensorfusion,osf-uart
1. No such vendor. You need to test against entire bindings.
2. Looks heavily incomplete - no resources. Compatible and hardware
description looks also incomplete - what is osf? It's exactly ONE
device? Why do you use bus suffix? Please start with writing bindings or
DTS101 slides.
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + serial {
> + osf {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
If you cannot find a name matching your device, please check in kernel
sources for similar cases or you can grow the spec (via pull request to
DT spec repo).
> + compatible = "opensensorfusion,osf-uart";
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device
2026-05-24 8:53 ` [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device Jinseob Kim
2026-05-24 9:03 ` sashiko-bot
2026-05-24 18:59 ` Krzysztof Kozlowski
@ 2026-05-24 19:34 ` Conor Dooley
2026-05-25 2:06 ` Kim Jinseob
2 siblings, 1 reply; 30+ messages in thread
From: Conor Dooley @ 2026-05-24 19:34 UTC (permalink / raw)
To: Jinseob Kim
Cc: Jonathan Cameron, linux-iio, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 313 bytes --]
On Sun, May 24, 2026 at 05:53:06PM +0900, Jinseob Kim wrote:
> Add a binding for the OSF0 UART-attached sensor aggregation device.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
Most of my v1 review was ignored it seems. Please go back to v1 and
respond to the points I made.
Thanks,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device
2026-05-24 19:34 ` Conor Dooley
@ 2026-05-25 2:06 ` Kim Jinseob
0 siblings, 0 replies; 30+ messages in thread
From: Kim Jinseob @ 2026-05-25 2:06 UTC (permalink / raw)
To: Conor Dooley
Cc: Jonathan Cameron, linux-iio, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
devicetree, linux-kernel
> Most of my v1 review was ignored it seems. Please go back to v1 and
> respond to the points I made.
I missed the v1 DT binding feedback when preparing v2. I will go back to
the v1 thread and reply there before the next revision.
Jinseob
> Thanks,
> Conor.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH RFC v2 2/7] Documentation: iio: add Open Sensor Fusion protocol v0 reference
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
2026-05-24 8:53 ` [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device Jinseob Kim
@ 2026-05-24 8:53 ` Jinseob Kim
2026-05-28 13:28 ` Jonathan Cameron
2026-05-24 8:53 ` [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding Jinseob Kim
` (5 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Jinseob Kim @ 2026-05-24 8:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel,
Jinseob Kim
Document the OSF0 frame format and payloads used by the driver.
Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
---
.../iio/open-sensor-fusion-protocol-v0.rst | 267 ++++++++++++++++++
1 file changed, 267 insertions(+)
create mode 100644 Documentation/iio/open-sensor-fusion-protocol-v0.rst
diff --git a/Documentation/iio/open-sensor-fusion-protocol-v0.rst b/Documentation/iio/open-sensor-fusion-protocol-v0.rst
new file mode 100644
index 000000000..4800a3ce6
--- /dev/null
+++ b/Documentation/iio/open-sensor-fusion-protocol-v0.rst
@@ -0,0 +1,267 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Open Sensor Fusion protocol v0
+==============================
+
+This document describes the OSF0 UART wire format used by the Linux IIO
+driver. It is not a firmware programming interface.
+
+Device model
+------------
+
+An Open Sensor Fusion UART device is a sensor aggregation device. It sends
+binary frames from the device to the host. The host driver decodes the frames
+and maps supported sensors to IIO devices.
+
+The hardware used for smoke testing is an OSF GREEN prototype with an
+STM32F405RGT6 MCU, an ICM42688P-class IMU, and an MMC5983MA magnetometer. That
+hardware is a test target, not part of the binding ABI.
+
+Transport
+---------
+
+The transport is UART at 115200 baud, 8 data bits, no parity, and 1 stop bit.
+The Linux transport is serdev. The v0 upstream driver covers device-to-host
+frames. Flow control is not used by the tested stream.
+
+Byte order
+----------
+
+All multi-byte integer fields are little-endian. Samples use signed 32-bit
+little-endian integers when ``sample_format`` is ``S32``.
+
+Frame format
+------------
+
+Each frame has a fixed 38-byte header, a payload, and a 4-byte CRC.
+
+.. list-table::
+ :header-rows: 1
+
+ * - Offset
+ - Size
+ - Field
+ - Description
+ * - 0
+ - 4
+ - magic
+ - ASCII ``OSF0``
+ * - 4
+ - 1
+ - protocol_major
+ - Must be ``0``
+ * - 5
+ - 1
+ - protocol_minor
+ - Minor version
+ * - 6
+ - 2
+ - header_len
+ - Must be ``38``
+ * - 8
+ - 2
+ - message_type
+ - Message type
+ * - 10
+ - 4
+ - payload_len
+ - Payload length in bytes
+ * - 14
+ - 8
+ - sequence
+ - Monotonic device sequence
+ * - 22
+ - 8
+ - timestamp_us
+ - Device timestamp in microseconds
+ * - 30
+ - 4
+ - flags
+ - Message flags
+ * - 34
+ - 4
+ - reserved
+ - Must be zero for v0
+ * - 38
+ - payload_len
+ - payload
+ - Message payload
+ * - 38 + payload_len
+ - 4
+ - crc32
+ - CRC32 over header and payload
+
+The frame CRC is IEEE CRC32 as implemented by ``crc32_le()`` with initial
+value ``0xffffffff`` and final XOR value ``0xffffffff``. The CRC field is not
+included in the CRC input.
+
+Message types
+-------------
+
+.. list-table::
+ :header-rows: 1
+
+ * - Value
+ - Name
+ - Direction
+ * - ``0x0001``
+ - ``SENSOR_SAMPLE``
+ - device to host
+ * - ``0x0002``
+ - ``DEVICE_STATUS``
+ - device to host
+ * - ``0x0003``
+ - ``CAPABILITY_REPORT``
+ - device to host
+
+Message types ``0x7f00`` through ``0x7fff`` are reserved. Values at or above
+``0x8000`` are vendor private and are ignored by the upstream driver.
+
+``SENSOR_SAMPLE`` payload
+-------------------------
+
+The base payload size is 16 bytes.
+
+.. list-table::
+ :header-rows: 1
+
+ * - Offset
+ - Size
+ - Field
+ - Description
+ * - 0
+ - 2
+ - sensor_type
+ - Sensor type ID
+ * - 2
+ - 2
+ - sensor_index
+ - Instance index
+ * - 4
+ - 2
+ - channel_count
+ - Number of S32 channels
+ * - 6
+ - 2
+ - sample_format
+ - Must be ``1`` (``S32``)
+ * - 8
+ - 4
+ - scale_nano
+ - Scale factor in nano-units
+ * - 12
+ - 4
+ - reserved
+ - Must be zero for v0
+ * - 16
+ - 4 * channel_count
+ - samples
+ - Signed 32-bit channel samples
+
+``DEVICE_STATUS`` payload
+-------------------------
+
+The payload size is 20 bytes. Fields are ``uptime_s``, ``status_flags``,
+``error_flags``, ``dropped_frames``, and a reserved field. Each field is
+32 bits.
+
+``CAPABILITY_REPORT`` payload
+-----------------------------
+
+The base payload size is 4 bytes. It contains ``capability_count`` and a
+reserved field. Each capability entry is 20 bytes:
+
+.. list-table::
+ :header-rows: 1
+
+ * - Offset
+ - Size
+ - Field
+ - Description
+ * - 0
+ - 2
+ - sensor_type
+ - Sensor type ID
+ * - 2
+ - 2
+ - sensor_index
+ - Instance index
+ * - 4
+ - 2
+ - channel_count
+ - Number of channels
+ * - 6
+ - 2
+ - sample_format
+ - Must be ``1`` (``S32``)
+ * - 8
+ - 4
+ - scale_nano
+ - Scale factor in nano-units
+ * - 12
+ - 4
+ - flags
+ - Capability flags
+ * - 16
+ - 4
+ - reserved
+ - Must be zero for v0
+
+Capability flag bit 0 means enabled by default. Bit 1 means calibrated data can
+be provided by the device. Other bits are invalid for v0.
+
+Sensor type IDs
+---------------
+
+.. list-table::
+ :header-rows: 1
+
+ * - Value
+ - Sensor
+ - IIO mapping
+ * - ``0x0001``
+ - accelerometer
+ - ``IIO_ACCEL``, X/Y/Z
+ * - ``0x0002``
+ - gyroscope
+ - ``IIO_ANGL_VEL``, X/Y/Z
+ * - ``0x0003``
+ - magnetometer
+ - ``IIO_MAGN``, X/Y/Z
+ * - ``0x0004``
+ - barometer
+ - not mapped in the initial driver
+ * - ``0x0005``
+ - temperature
+ - ``IIO_TEMP``
+ * - ``0x0006``
+ - humidity
+ - not mapped in the initial driver
+ * - ``0x0007``
+ - ambient light
+ - not mapped in the initial driver
+ * - ``0x0008``
+ - proximity
+ - not mapped in the initial driver
+
+Scaling
+-------
+
+``scale_nano`` is the per-channel scale value in nano-units. The Linux driver
+maps it to ``IIO_CHAN_INFO_SCALE`` as integer plus nano. The exact physical
+unit depends on the IIO channel type.
+
+Timestamps
+----------
+
+The frame header carries ``timestamp_us``, a device-side timestamp in
+microseconds. v0 transports this value for ordering and diagnostics. The driver
+does not use it as a production-grade host-correlated timestamp. Buffered IIO
+timestamps follow IIO timestamp clock handling.
+
+Non-goals for v0 upstream
+-------------------------
+
+The v0 upstream driver does not include USB transport, fusion output,
+AHRS/Kalman output, calibration command ABI, custom sysfs control surface,
+production timestamp correlation, or runtime capability removal.
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 2/7] Documentation: iio: add Open Sensor Fusion protocol v0 reference
2026-05-24 8:53 ` [PATCH RFC v2 2/7] Documentation: iio: add Open Sensor Fusion protocol v0 reference Jinseob Kim
@ 2026-05-28 13:28 ` Jonathan Cameron
[not found] ` <CALMSew+3RVXxLJYtr3HkV7UeAf6Mqx6PpA2CehChoVaMFddpJw@mail.gmail.com>
0 siblings, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2026-05-28 13:28 UTC (permalink / raw)
To: Jinseob Kim
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Sun, 24 May 2026 17:53:07 +0900
Jinseob Kim <kimjinseob88@gmail.com> wrote:
> Document the OSF0 frame format and payloads used by the driver.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
> ---
> .../iio/open-sensor-fusion-protocol-v0.rst | 267 ++++++++++++++++++
> 1 file changed, 267 insertions(+)
> create mode 100644 Documentation/iio/open-sensor-fusion-protocol-v0.rst
>
> diff --git a/Documentation/iio/open-sensor-fusion-protocol-v0.rst b/Documentation/iio/open-sensor-fusion-protocol-v0.rst
> new file mode 100644
> index 000000000..4800a3ce6
> --- /dev/null
> +++ b/Documentation/iio/open-sensor-fusion-protocol-v0.rst
> @@ -0,0 +1,267 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +Open Sensor Fusion protocol v0
> +==============================
> +
> +This document describes the OSF0 UART wire format used by the Linux IIO
> +driver. It is not a firmware programming interface.
Can we have some background information. Where does this OSF thing come from?
Is it a general standard or something you are personally developing?
Some links would definitely help.
> +
> +Device model
> +------------
> +
> +An Open Sensor Fusion UART device is a sensor aggregation device. It sends
> +binary frames from the device to the host. The host driver decodes the frames
> +and maps supported sensors to IIO devices.
> +
> +The hardware used for smoke testing is an OSF GREEN prototype with an
> +STM32F405RGT6 MCU, an ICM42688P-class IMU, and an MMC5983MA magnetometer. That
> +hardware is a test target, not part of the binding ABI.
> +
> +Transport
> +---------
> +
> +The transport is UART at 115200 baud, 8 data bits, no parity, and 1 stop bit.
> +The Linux transport is serdev. The v0 upstream driver covers device-to-host
> +frames. Flow control is not used by the tested stream.
> +
> +Byte order
> +----------
> +
> +All multi-byte integer fields are little-endian. Samples use signed 32-bit
> +little-endian integers when ``sample_format`` is ``S32``.
s32 given this is kernel code and i'm not sure what else this is referring to.
> +``SENSOR_SAMPLE`` payload
> +-------------------------
> +
> +The base payload size is 16 bytes.
Not sure that is meaninful given expectation that there will be channels.
I'd describe it as a payload header, or express this as 16 + 4 * channel_count
> +
> +.. list-table::
> + :header-rows: 1
> +
> + * - Offset
> + - Size
> + - Field
> + - Description
> + * - 0
> + - 2
> + - sensor_type
> + - Sensor type ID
> + * - 2
> + - 2
> + - sensor_index
> + - Instance index
> + * - 4
> + - 2
> + - channel_count
> + - Number of S32 channels
> + * - 6
> + - 2
> + - sample_format
> + - Must be ``1`` (``S32``)
Is this spec defined, or just what the driver supports?
I think this doc needs to distinguish between those two cases
more clearly.
> + * - 8
> + - 4
> + - scale_nano
> + - Scale factor in nano-units
> + * - 12
> + - 4
> + - reserved
> + - Must be zero for v0
> + * - 16
> + - 4 * channel_count
> + - samples
> + - Signed 32-bit channel samples
> +
> +``DEVICE_STATUS`` payload
> +-------------------------
> +
> +The payload size is 20 bytes. Fields are ``uptime_s``, ``status_flags``,
> +``error_flags``, ``dropped_frames``, and a reserved field. Each field is
> +32 bits.
> +
> +``CAPABILITY_REPORT`` payload
> +-----------------------------
> +
> +The base payload size is 4 bytes. It contains ``capability_count`` and a
> +reserved field. Each capability entry is 20 bytes:
Probably want a separate table for that header.
> +
> +.. list-table::
> + :header-rows: 1
> +
> + * - Offset
> + - Size
> + - Field
> + - Description
> + * - 0
> + - 2
> + - sensor_type
> + - Sensor type ID
> + * - 2
> + - 2
> + - sensor_index
> + - Instance index
> + * - 4
> + - 2
> + - channel_count
> + - Number of channels
> + * - 6
> + - 2
> + - sample_format
> + - Must be ``1`` (``S32``)
> + * - 8
> + - 4
> + - scale_nano
> + - Scale factor in nano-units
> + * - 12
> + - 4
> + - flags
> + - Capability flags
> + * - 16
> + - 4
> + - reserved
> + - Must be zero for v0
> +
> +Capability flag bit 0 means enabled by default. Bit 1 means calibrated data can
> +be provided by the device. Other bits are invalid for v0.
> +
> +Scaling
> +-------
> +
> +``scale_nano`` is the per-channel scale value in nano-units. The Linux driver
> +maps it to ``IIO_CHAN_INFO_SCALE`` as integer plus nano. The exact physical
> +unit depends on the IIO channel type.
> +
> +Timestamps
> +----------
> +
> +The frame header carries ``timestamp_us``, a device-side timestamp in
> +microseconds. v0 transports this value for ordering and diagnostics. The driver
> +does not use it as a production-grade host-correlated timestamp. Buffered IIO
> +timestamps follow IIO timestamp clock handling.
Is there likely to be a non trivial delay? If so we should figure out how to use
that timestamp to get something more useful.
> +
> +Non-goals for v0 upstream
> +-------------------------
> +
> +The v0 upstream driver does not include USB transport, fusion output,
> +AHRS/Kalman output, calibration command ABI, custom sysfs control surface,
What is AHRS? I'd spell it out.
> +production timestamp correlation, or runtime capability removal.
This is where an external link would be helpful. Lets us have some visibility
of what is coming.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
2026-05-24 8:53 ` [PATCH RFC v2 1/7] dt-bindings: iio: add Open Sensor Fusion UART device Jinseob Kim
2026-05-24 8:53 ` [PATCH RFC v2 2/7] Documentation: iio: add Open Sensor Fusion protocol v0 reference Jinseob Kim
@ 2026-05-24 8:53 ` Jinseob Kim
2026-05-24 9:16 ` sashiko-bot
2026-05-28 13:48 ` Jonathan Cameron
2026-05-24 8:53 ` [PATCH RFC v2 4/7] iio: osf: add stream parser Jinseob Kim
` (4 subsequent siblings)
7 siblings, 2 replies; 30+ messages in thread
From: Jinseob Kim @ 2026-05-24 8:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel,
Jinseob Kim
Add OSF0 frame and payload decoders for device-to-host messages.
Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
---
drivers/iio/opensensorfusion/osf_protocol.c | 220 ++++++++++++++++++++
drivers/iio/opensensorfusion/osf_protocol.h | 100 +++++++++
2 files changed, 320 insertions(+)
create mode 100644 drivers/iio/opensensorfusion/osf_protocol.c
create mode 100644 drivers/iio/opensensorfusion/osf_protocol.h
diff --git a/drivers/iio/opensensorfusion/osf_protocol.c b/drivers/iio/opensensorfusion/osf_protocol.c
new file mode 100644
index 000000000..ac3c37ae2
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_protocol.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/crc32.h>
+#include <linux/errno.h>
+#include <linux/bits.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+
+#include "osf_protocol.h"
+
+#define OSF_CRC32_INIT GENMASK(31, 0)
+#define OSF_CRC32_XOROUT GENMASK(31, 0)
+
+static bool osf_sensor_type_valid(u16 sensor_type)
+{
+ return sensor_type >= OSF_SENSOR_ACCELEROMETER &&
+ sensor_type <= OSF_SENSOR_PROXIMITY;
+}
+
+static u32 osf_crc32_ieee(const u8 *buf, size_t len)
+{
+ return crc32_le(OSF_CRC32_INIT, buf, len) ^ OSF_CRC32_XOROUT;
+}
+
+int osf_protocol_decode_frame(const u8 *buf, size_t len,
+ struct osf_frame *frame, size_t *frame_len)
+{
+ u32 expected_crc;
+ u32 actual_crc;
+ u32 payload_len;
+ size_t total_len;
+ u8 major;
+
+ if (!buf || !frame || !frame_len)
+ return -EINVAL;
+
+ if (len < OSF_FRAME_MIN_LEN)
+ return -EMSGSIZE;
+
+ if (buf[0] != 'O' || buf[1] != 'S' || buf[2] != 'F' || buf[3] != '0')
+ return -EPROTO;
+
+ major = buf[4];
+ if (major != OSF_PROTOCOL_MAJOR)
+ return -EPROTO;
+
+ if (get_unaligned_le16(buf + 6) != OSF_FRAME_HEADER_LEN)
+ return -EPROTO;
+
+ payload_len = get_unaligned_le32(buf + 10);
+ if (payload_len > len - OSF_FRAME_MIN_LEN)
+ return -EMSGSIZE;
+
+ total_len = OSF_FRAME_HEADER_LEN + payload_len + OSF_FRAME_CRC_LEN;
+ expected_crc = osf_crc32_ieee(buf, OSF_FRAME_HEADER_LEN + payload_len);
+ actual_crc = get_unaligned_le32(buf + OSF_FRAME_HEADER_LEN + payload_len);
+
+ if (actual_crc != expected_crc)
+ return -EBADMSG;
+
+ frame->protocol_minor = buf[5];
+ frame->message_type = get_unaligned_le16(buf + 8);
+ frame->payload_len = payload_len;
+ frame->sequence = get_unaligned_le64(buf + 14);
+ frame->timestamp_us = get_unaligned_le64(buf + 22);
+ frame->flags = get_unaligned_le32(buf + 30);
+ frame->reserved = get_unaligned_le32(buf + 34);
+ frame->payload = buf + OSF_FRAME_HEADER_LEN;
+ frame->crc = actual_crc;
+ *frame_len = total_len;
+
+ return 0;
+}
+
+int osf_protocol_decode_sensor_sample(const struct osf_frame *frame,
+ struct osf_sensor_sample *sample)
+{
+ u16 channel_count;
+ u16 sample_format;
+ u32 expected_len;
+ const u8 *payload;
+
+ if (!frame || !sample || !frame->payload)
+ return -EINVAL;
+
+ if (frame->message_type != OSF_MSG_SENSOR_SAMPLE)
+ return -EPROTO;
+
+ if (frame->payload_len < OSF_SENSOR_SAMPLE_BASE_LEN)
+ return -EMSGSIZE;
+
+ payload = frame->payload;
+ channel_count = get_unaligned_le16(payload + 4);
+ sample_format = get_unaligned_le16(payload + 6);
+
+ if (sample_format != OSF_SAMPLE_FORMAT_S32)
+ return -EPROTO;
+
+ expected_len = OSF_SENSOR_SAMPLE_BASE_LEN + channel_count * sizeof(s32);
+ if (frame->payload_len != expected_len)
+ return -EMSGSIZE;
+
+ sample->sensor_type = get_unaligned_le16(payload);
+ sample->sensor_index = get_unaligned_le16(payload + 2);
+ sample->channel_count = channel_count;
+ sample->sample_format = sample_format;
+ sample->scale_nano = get_unaligned_le32(payload + 8);
+ sample->reserved = get_unaligned_le32(payload + 12);
+ sample->samples = payload + OSF_SENSOR_SAMPLE_BASE_LEN;
+
+ return 0;
+}
+
+int osf_protocol_sensor_sample_value(const struct osf_sensor_sample *sample,
+ unsigned int index, s32 *value)
+{
+ if (!sample || !sample->samples || !value)
+ return -EINVAL;
+
+ if (index >= sample->channel_count)
+ return -ERANGE;
+
+ *value = (s32)get_unaligned_le32(sample->samples + index * sizeof(s32));
+
+ return 0;
+}
+
+int osf_protocol_decode_device_status(const struct osf_frame *frame,
+ struct osf_device_status *status)
+{
+ const u8 *payload;
+
+ if (!frame || !status || !frame->payload)
+ return -EINVAL;
+
+ if (frame->message_type != OSF_MSG_DEVICE_STATUS)
+ return -EPROTO;
+
+ if (frame->payload_len != OSF_DEVICE_STATUS_LEN)
+ return -EMSGSIZE;
+
+ payload = frame->payload;
+ status->uptime_s = get_unaligned_le32(payload);
+ status->status_flags = get_unaligned_le32(payload + 4);
+ status->error_flags = get_unaligned_le32(payload + 8);
+ status->dropped_frames = get_unaligned_le32(payload + 12);
+ status->reserved = get_unaligned_le32(payload + 16);
+
+ return 0;
+}
+
+int osf_protocol_decode_capability_report(const struct osf_frame *frame,
+ struct osf_capability_report *report)
+{
+ u16 capability_count;
+ u32 expected_len;
+ const u8 *payload;
+
+ if (!frame || !report || !frame->payload)
+ return -EINVAL;
+
+ if (frame->message_type != OSF_MSG_CAPABILITY_REPORT)
+ return -EPROTO;
+
+ if (frame->payload_len < OSF_CAP_REPORT_BASE_LEN)
+ return -EMSGSIZE;
+
+ payload = frame->payload;
+ capability_count = get_unaligned_le16(payload);
+ expected_len = OSF_CAP_REPORT_BASE_LEN +
+ capability_count * OSF_CAP_SENSOR_ENTRY_LEN;
+
+ if (frame->payload_len != expected_len)
+ return -EMSGSIZE;
+
+ report->capability_count = capability_count;
+ report->reserved = get_unaligned_le16(payload + 2);
+ if (report->reserved)
+ return -EPROTO;
+
+ report->entries = payload + OSF_CAP_REPORT_BASE_LEN;
+
+ return 0;
+}
+
+int osf_protocol_decode_capability_entry(const struct osf_capability_report *report,
+ unsigned int index,
+ struct osf_capability_entry *entry)
+{
+ const u8 *payload;
+
+ if (!report || !report->entries || !entry)
+ return -EINVAL;
+
+ if (index >= report->capability_count)
+ return -ERANGE;
+
+ payload = report->entries + index * OSF_CAP_SENSOR_ENTRY_LEN;
+ entry->sensor_type = get_unaligned_le16(payload);
+ entry->sensor_index = get_unaligned_le16(payload + 2);
+ entry->channel_count = get_unaligned_le16(payload + 4);
+ entry->sample_format = get_unaligned_le16(payload + 6);
+ entry->scale_nano = get_unaligned_le32(payload + 8);
+ entry->flags = get_unaligned_le32(payload + 12);
+ entry->reserved = get_unaligned_le32(payload + 16);
+
+ if (!osf_sensor_type_valid(entry->sensor_type))
+ return -EPROTO;
+
+ if (entry->sample_format != OSF_SAMPLE_FORMAT_S32)
+ return -EPROTO;
+
+ if (entry->flags & ~OSF_CAPABILITY_FLAGS_MASK)
+ return -EPROTO;
+
+ if (entry->reserved)
+ return -EPROTO;
+
+ return 0;
+}
diff --git a/drivers/iio/opensensorfusion/osf_protocol.h b/drivers/iio/opensensorfusion/osf_protocol.h
new file mode 100644
index 000000000..fd6e9581f
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_protocol.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _OSF_PROTOCOL_H
+#define _OSF_PROTOCOL_H
+
+#include <linux/types.h>
+
+#define OSF_PROTOCOL_MAJOR 0
+#define OSF_PROTOCOL_MINOR 0
+#define OSF_FRAME_HEADER_LEN 38
+#define OSF_FRAME_CRC_LEN 4
+#define OSF_FRAME_MIN_LEN (OSF_FRAME_HEADER_LEN + OSF_FRAME_CRC_LEN)
+
+#define OSF_SENSOR_SAMPLE_BASE_LEN 16
+#define OSF_DEVICE_STATUS_LEN 20
+#define OSF_CAP_REPORT_BASE_LEN 4
+#define OSF_CAP_SENSOR_ENTRY_LEN 20
+#define OSF_CAPABILITY_FLAGS_MASK 0x00000003U
+
+enum osf_message_type {
+ OSF_MSG_SENSOR_SAMPLE = 0x0001,
+ OSF_MSG_DEVICE_STATUS = 0x0002,
+ OSF_MSG_CAPABILITY_REPORT = 0x0003,
+};
+
+enum osf_sensor_type {
+ OSF_SENSOR_ACCELEROMETER = 0x0001,
+ OSF_SENSOR_GYROSCOPE = 0x0002,
+ OSF_SENSOR_MAGNETOMETER = 0x0003,
+ OSF_SENSOR_BAROMETER = 0x0004,
+ OSF_SENSOR_TEMPERATURE = 0x0005,
+ OSF_SENSOR_HUMIDITY = 0x0006,
+ OSF_SENSOR_AMBIENT_LIGHT = 0x0007,
+ OSF_SENSOR_PROXIMITY = 0x0008,
+};
+
+enum osf_sample_format {
+ OSF_SAMPLE_FORMAT_S32 = 0x0001,
+};
+
+struct osf_frame {
+ u8 protocol_minor;
+ u16 message_type;
+ u32 payload_len;
+ u64 sequence;
+ u64 timestamp_us;
+ u32 flags;
+ u32 reserved;
+ const u8 *payload;
+ u32 crc;
+};
+
+struct osf_sensor_sample {
+ u16 sensor_type;
+ u16 sensor_index;
+ u16 channel_count;
+ u16 sample_format;
+ u32 scale_nano;
+ u32 reserved;
+ const u8 *samples;
+};
+
+struct osf_device_status {
+ u32 uptime_s;
+ u32 status_flags;
+ u32 error_flags;
+ u32 dropped_frames;
+ u32 reserved;
+};
+
+struct osf_capability_report {
+ u16 capability_count;
+ u16 reserved;
+ const u8 *entries;
+};
+
+struct osf_capability_entry {
+ u16 sensor_type;
+ u16 sensor_index;
+ u16 channel_count;
+ u16 sample_format;
+ u32 scale_nano;
+ u32 flags;
+ u32 reserved;
+};
+
+int osf_protocol_decode_frame(const u8 *buf, size_t len,
+ struct osf_frame *frame, size_t *frame_len);
+int osf_protocol_decode_sensor_sample(const struct osf_frame *frame,
+ struct osf_sensor_sample *sample);
+int osf_protocol_decode_device_status(const struct osf_frame *frame,
+ struct osf_device_status *status);
+int osf_protocol_decode_capability_report(const struct osf_frame *frame,
+ struct osf_capability_report *report);
+int osf_protocol_decode_capability_entry(const struct osf_capability_report *report,
+ unsigned int index,
+ struct osf_capability_entry *entry);
+int osf_protocol_sensor_sample_value(const struct osf_sensor_sample *sample,
+ unsigned int index, s32 *value);
+
+#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding
2026-05-24 8:53 ` [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding Jinseob Kim
@ 2026-05-24 9:16 ` sashiko-bot
2026-05-28 13:48 ` Jonathan Cameron
1 sibling, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-05-24 9:16 UTC (permalink / raw)
To: Jinseob Kim; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The function `osf_protocol_decode_sensor_sample` fails to validate the untrusted `sensor_type` field from the payload.
--
commit 771f82b33a67fcd96de20b3ff22ae83721808219
Author: Jinseob Kim <kimjinseob88@gmail.com>
iio: osf: add protocol v0 decoding
This commit introduces the OSF0 protocol frame and payload decoders for
device-to-host messages.
> diff --git a/drivers/iio/opensensorfusion/osf_protocol.c b/drivers/iio/opensensorfusion/osf_protocol.c
> new file mode 100644
> index 0000000000000..ac3c37ae28f32
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_protocol.c
[ ... ]
> +int osf_protocol_decode_sensor_sample(const struct osf_frame *frame,
> + struct osf_sensor_sample *sample)
> +{
> + u16 channel_count;
> + u16 sample_format;
> + u32 expected_len;
> + const u8 *payload;
> +
> + if (!frame || !sample || !frame->payload)
> + return -EINVAL;
> +
> + if (frame->message_type != OSF_MSG_SENSOR_SAMPLE)
> + return -EPROTO;
> +
> + if (frame->payload_len < OSF_SENSOR_SAMPLE_BASE_LEN)
> + return -EMSGSIZE;
> +
> + payload = frame->payload;
> + channel_count = get_unaligned_le16(payload + 4);
> + sample_format = get_unaligned_le16(payload + 6);
> +
> + if (sample_format != OSF_SAMPLE_FORMAT_S32)
> + return -EPROTO;
> +
> + expected_len = OSF_SENSOR_SAMPLE_BASE_LEN + channel_count * sizeof(s32);
> + if (frame->payload_len != expected_len)
> + return -EMSGSIZE;
> +
> + sample->sensor_type = get_unaligned_le16(payload);
[Severity: Medium]
Should this code validate the untrusted sensor_type field?
The parser validates the sample format but assigns sample->sensor_type
directly from the untrusted device payload without calling the available
osf_sensor_type_valid() function.
If an invalid sensor type is accepted and passed to the core handler, it
could be stored in the latest_samples cache. Since the cache has a fixed
limit (OSF_MAX_CAPABILITIES), storing invalid sensor types from a malformed
payload could exhaust the available slots, causing legitimate future sensor
samples to be permanently dropped with -E2BIG.
> + sample->sensor_index = get_unaligned_le16(payload + 2);
> + sample->channel_count = channel_count;
> + sample->sample_format = sample_format;
> + sample->scale_nano = get_unaligned_le32(payload + 8);
> + sample->reserved = get_unaligned_le32(payload + 12);
> + sample->samples = payload + OSF_SENSOR_SAMPLE_BASE_LEN;
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524085312.15369-1-kimjinseob88@gmail.com?part=3
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding
2026-05-24 8:53 ` [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding Jinseob Kim
2026-05-24 9:16 ` sashiko-bot
@ 2026-05-28 13:48 ` Jonathan Cameron
[not found] ` <CALMSew+wUH1H-2MTtexCFgyD4Y+upFuSfFzTWBn3VGN6CWYFNQ@mail.gmail.com>
1 sibling, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2026-05-28 13:48 UTC (permalink / raw)
To: Jinseob Kim
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Sun, 24 May 2026 17:53:08 +0900
Jinseob Kim <kimjinseob88@gmail.com> wrote:
> Add OSF0 frame and payload decoders for device-to-host messages.
https://sashiko.dev/#/patchset/20260524085312.15369-1-kimjinseob88%40gmail.com
Has some perhaps useful feedback. Please make sure to either exclude
them as false positives or address for next version.
Quite a bit of the feedback in here is about which checks are actually
useful. Normal kernel practice is assume we didn't shoot outselves in the
foot but hardware and userspace may have done. So we defend only against
things in their control.
Jonathan
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
> ---
> drivers/iio/opensensorfusion/osf_protocol.c | 220 ++++++++++++++++++++
> drivers/iio/opensensorfusion/osf_protocol.h | 100 +++++++++
> 2 files changed, 320 insertions(+)
> create mode 100644 drivers/iio/opensensorfusion/osf_protocol.c
> create mode 100644 drivers/iio/opensensorfusion/osf_protocol.h
>
> diff --git a/drivers/iio/opensensorfusion/osf_protocol.c b/drivers/iio/opensensorfusion/osf_protocol.c
> new file mode 100644
> index 000000000..ac3c37ae2
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_protocol.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/crc32.h>
> +#include <linux/errno.h>
> +#include <linux/bits.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +
> +#include "osf_protocol.h"
> +
> +#define OSF_CRC32_INIT GENMASK(31, 0)
> +#define OSF_CRC32_XOROUT GENMASK(31, 0)
> +
> +static bool osf_sensor_type_valid(u16 sensor_type)
> +{
> + return sensor_type >= OSF_SENSOR_ACCELEROMETER &&
> + sensor_type <= OSF_SENSOR_PROXIMITY;
ah. I talk about sanity checking this below for purposes of ensuring
we can use the ennum type. Looks like you have it all done already :)
> +}
> +
> +static u32 osf_crc32_ieee(const u8 *buf, size_t len)
> +{
> + return crc32_le(OSF_CRC32_INIT, buf, len) ^ OSF_CRC32_XOROUT;
> +}
> +
> +int osf_protocol_decode_frame(const u8 *buf, size_t len,
> + struct osf_frame *frame, size_t *frame_len)
> +{
> + u32 expected_crc;
> + u32 actual_crc;
> + u32 payload_len;
> + size_t total_len;
> + u8 major;
> +
> + if (!buf || !frame || !frame_len)
> + return -EINVAL;
> +
> + if (len < OSF_FRAME_MIN_LEN)
> + return -EMSGSIZE;
> +
> + if (buf[0] != 'O' || buf[1] != 'S' || buf[2] != 'F' || buf[3] != '0')
> + return -EPROTO;
> +
> + major = buf[4];
> + if (major != OSF_PROTOCOL_MAJOR)
> + return -EPROTO;
> +
> + if (get_unaligned_le16(buf + 6) != OSF_FRAME_HEADER_LEN)
> + return -EPROTO;
> +
> + payload_len = get_unaligned_le32(buf + 10);
> + if (payload_len > len - OSF_FRAME_MIN_LEN)
> + return -EMSGSIZE;
> +
> + total_len = OSF_FRAME_HEADER_LEN + payload_len + OSF_FRAME_CRC_LEN;
> + expected_crc = osf_crc32_ieee(buf, OSF_FRAME_HEADER_LEN + payload_len);
> + actual_crc = get_unaligned_le32(buf + OSF_FRAME_HEADER_LEN + payload_len);
> +
> + if (actual_crc != expected_crc)
> + return -EBADMSG;
> +
> + frame->protocol_minor = buf[5];
> + frame->message_type = get_unaligned_le16(buf + 8);
> + frame->payload_len = payload_len;
> + frame->sequence = get_unaligned_le64(buf + 14);
> + frame->timestamp_us = get_unaligned_le64(buf + 22);
> + frame->flags = get_unaligned_le32(buf + 30);
> + frame->reserved = get_unaligned_le32(buf + 34);
Seems little point in storing reserved. If you want to check it is spec
compliant do it here.
> + frame->payload = buf + OSF_FRAME_HEADER_LEN;
> + frame->crc = actual_crc;
> + *frame_len = total_len;
> +
> + return 0;
> +}
> +
> +int osf_protocol_sensor_sample_value(const struct osf_sensor_sample *sample,
> + unsigned int index, s32 *value)
> +{
> + if (!sample || !sample->samples || !value)
> + return -EINVAL;
Similar on whether this defensive code makes sense (see below and note I review
upwards in drivers as they make sense to me better that way!)
> +
> + if (index >= sample->channel_count)
> + return -ERANGE;
> +
> + *value = (s32)get_unaligned_le32(sample->samples + index * sizeof(s32));
I guess it might change in future, but for now why not make samples an s32 *
> +
> + return 0;
> +}
> +int osf_protocol_decode_capability_report(const struct osf_frame *frame,
> + struct osf_capability_report *report)
> +{
> + u16 capability_count;
> + u32 expected_len;
> + const u8 *payload;
> +
> + if (!frame || !report || !frame->payload)
> + return -EINVAL;
Similar sanity check questions.
> +
> + if (frame->message_type != OSF_MSG_CAPABILITY_REPORT)
> + return -EPROTO;
If that happens seems like something went very wrong elsewhere so
seems unlikely defense makes sense here.
> +
> + if (frame->payload_len < OSF_CAP_REPORT_BASE_LEN)
> + return -EMSGSIZE;
This one does belong in here.
> +
> + payload = frame->payload;
> + capability_count = get_unaligned_le16(payload);
> + expected_len = OSF_CAP_REPORT_BASE_LEN +
> + capability_count * OSF_CAP_SENSOR_ENTRY_LEN;
> +
> + if (frame->payload_len != expected_len)
> + return -EMSGSIZE;
> +
> + report->capability_count = capability_count;
> + report->reserved = get_unaligned_le16(payload + 2);
> + if (report->reserved)
> + return -EPROTO;
This check is fine as picks up on bad hardware, but why keep
the value of reserved?
> +
> + report->entries = payload + OSF_CAP_REPORT_BASE_LEN;
> +
> + return 0;
> +}
> +
> +int osf_protocol_decode_capability_entry(const struct osf_capability_report *report,
> + unsigned int index,
> + struct osf_capability_entry *entry)
> +{
> + const u8 *payload;
> +
> + if (!report || !report->entries || !entry)
> + return -EINVAL;
I'm not sure if there is a way to get here with any of those failing.
Generally for kernel code we defend against this sort of thing at higher levels.
Same applies to all this parser.
> +
> + if (index >= report->capability_count)
Likewise, I'd expect this to only be called on one that exists. This
defensive stuff costs us in complexity so only do it if needed. Do however
defend against values that are coming from the device where possible to
ensure they are consistent.
> + return -ERANGE;
> +
> + payload = report->entries + index * OSF_CAP_SENSOR_ENTRY_LEN;
As below - if entries had the right type this would be a simple + index
I think.
> + entry->sensor_type = get_unaligned_le16(payload);
I mention this below - nicer to use the enum type if possible.
> + entry->sensor_index = get_unaligned_le16(payload + 2);
> + entry->channel_count = get_unaligned_le16(payload + 4);
> + entry->sample_format = get_unaligned_le16(payload + 6);
> + entry->scale_nano = get_unaligned_le32(payload + 8);
> + entry->flags = get_unaligned_le32(payload + 12);
> + entry->reserved = get_unaligned_le32(payload + 16);
Why keep it?
I think you are setting them all, so would be neater as a designated
inializer:
*entry = (struct osf_capbility_entry) {
.sensor_type = get_unaligned_le16(payload),
.sensor_index = get_unaligned_le16(payload + 2),
};
Same applies to some of the other structures filled in here.
> +
> + if (!osf_sensor_type_valid(entry->sensor_type))
> + return -EPROTO;
> +
> + if (entry->sample_format != OSF_SAMPLE_FORMAT_S32)
> + return -EPROTO;
> +
> + if (entry->flags & ~OSF_CAPABILITY_FLAGS_MASK)
> + return -EPROTO;
> +
> + if (entry->reserved)
> + return -EPROTO;
> +
> + return 0;
> +}
> diff --git a/drivers/iio/opensensorfusion/osf_protocol.h b/drivers/iio/opensensorfusion/osf_protocol.h
> new file mode 100644
> index 000000000..fd6e9581f
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_protocol.h
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _OSF_PROTOCOL_H
> +#define _OSF_PROTOCOL_H
> +
> +#include <linux/types.h>
> +
> +#define OSF_PROTOCOL_MAJOR 0
> +#define OSF_PROTOCOL_MINOR 0
> +#define OSF_FRAME_HEADER_LEN 38
> +#define OSF_FRAME_CRC_LEN 4
> +#define OSF_FRAME_MIN_LEN (OSF_FRAME_HEADER_LEN + OSF_FRAME_CRC_LEN)
> +
> +#define OSF_SENSOR_SAMPLE_BASE_LEN 16
> +#define OSF_DEVICE_STATUS_LEN 20
> +#define OSF_CAP_REPORT_BASE_LEN 4
> +#define OSF_CAP_SENSOR_ENTRY_LEN 20
> +#define OSF_CAPABILITY_FLAGS_MASK 0x00000003U
> +
> +enum osf_message_type {
> + OSF_MSG_SENSOR_SAMPLE = 0x0001,
> + OSF_MSG_DEVICE_STATUS = 0x0002,
> + OSF_MSG_CAPABILITY_REPORT = 0x0003,
> +};
> +
> +enum osf_sensor_type {
> + OSF_SENSOR_ACCELEROMETER = 0x0001,
> + OSF_SENSOR_GYROSCOPE = 0x0002,
> + OSF_SENSOR_MAGNETOMETER = 0x0003,
> + OSF_SENSOR_BAROMETER = 0x0004,
> + OSF_SENSOR_TEMPERATURE = 0x0005,
> + OSF_SENSOR_HUMIDITY = 0x0006,
> + OSF_SENSOR_AMBIENT_LIGHT = 0x0007,
> + OSF_SENSOR_PROXIMITY = 0x0008,
> +};
> +
> +enum osf_sample_format {
> + OSF_SAMPLE_FORMAT_S32 = 0x0001,
> +};
> +
> +struct osf_frame {
> + u8 protocol_minor;
> + u16 message_type;
> + u32 payload_len;
> + u64 sequence;
> + u64 timestamp_us;
> + u32 flags;
> + u32 reserved;
Not obvious why to keep this.
> + const u8 *payload;
> + u32 crc;
> +};
> +
> +struct osf_sensor_sample {
> + u16 sensor_type;
> + u16 sensor_index;
> + u16 channel_count;
> + u16 sample_format;
We don't need to match types with original data so
nice to use the named enums if possible. Will need
to check limits though when decoding.
> + u32 scale_nano;
> + u32 reserved;
why store reserved in these?
> + const u8 *samples;
Compared to capabilities this one seems trickier for
types given it's variable size. Still nice to use something
that indicates the type if we can.
> +};
> +
> +struct osf_device_status {
> + u32 uptime_s;
> + u32 status_flags;
> + u32 error_flags;
> + u32 dropped_frames;
> + u32 reserved;
> +};
> +
> +struct osf_capability_report {
> + u16 capability_count;
> + u16 reserved;
> + const u8 *entries;
Are these not struct osf_capability_entry?
If they are then use that not a u8.
I'd not bother with a const marking.
With that type fixed you should be able to use
__counted_by_ptr(capability_count) to make that relationship
clear.
> +};
> +
> +struct osf_capability_entry {
> + u16 sensor_type;
> + u16 sensor_index;
> + u16 channel_count;
> + u16 sample_format;
> + u32 scale_nano;
> + u32 flags;
> + u32 reserved;
> +};
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH RFC v2 4/7] iio: osf: add stream parser
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
` (2 preceding siblings ...)
2026-05-24 8:53 ` [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding Jinseob Kim
@ 2026-05-24 8:53 ` Jinseob Kim
2026-05-24 9:41 ` sashiko-bot
2026-05-28 13:58 ` Jonathan Cameron
2026-05-24 8:53 ` [PATCH RFC v2 5/7] iio: osf: add UART serdev transport Jinseob Kim
` (3 subsequent siblings)
7 siblings, 2 replies; 30+ messages in thread
From: Jinseob Kim @ 2026-05-24 8:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel,
Jinseob Kim
Add byte stream assembly and resync for OSF0 frames.
Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
---
drivers/iio/opensensorfusion/osf_stream.c | 207 ++++++++++++++++++++++
drivers/iio/opensensorfusion/osf_stream.h | 31 ++++
2 files changed, 238 insertions(+)
create mode 100644 drivers/iio/opensensorfusion/osf_stream.c
create mode 100644 drivers/iio/opensensorfusion/osf_stream.h
diff --git a/drivers/iio/opensensorfusion/osf_stream.c b/drivers/iio/opensensorfusion/osf_stream.c
new file mode 100644
index 000000000..a2739c987
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_stream.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+
+#include "osf_core.h"
+#include "osf_protocol.h"
+#include "osf_stream.h"
+
+#define OSF_STREAM_MAGIC_LEN 4
+#define OSF_STREAM_MAX_PAYLOAD_LEN \
+ (OSF_STREAM_MAX_FRAME_LEN - OSF_FRAME_HEADER_LEN - OSF_FRAME_CRC_LEN)
+
+static const u8 osf_stream_magic[OSF_STREAM_MAGIC_LEN] = {
+ 'O', 'S', 'F', '0',
+};
+
+static void osf_stream_discard(struct osf_stream *stream, size_t count)
+{
+ if (count >= stream->len) {
+ stream->len = 0;
+ return;
+ }
+
+ memmove(stream->buf, stream->buf + count, stream->len - count);
+ stream->len -= count;
+}
+
+static void osf_stream_drop_invalid_head(struct osf_stream *stream)
+{
+ osf_stream_discard(stream, 1);
+}
+
+static bool osf_stream_magic_match(const u8 *buf, size_t len)
+{
+ return !memcmp(buf, osf_stream_magic, len);
+}
+
+static size_t osf_stream_discard_to_magic(struct osf_stream *stream)
+{
+ size_t old_len = stream->len;
+ size_t match_len;
+ size_t i;
+
+ for (i = 0; i < stream->len; i++) {
+ match_len = stream->len - i;
+ if (match_len > OSF_STREAM_MAGIC_LEN)
+ match_len = OSF_STREAM_MAGIC_LEN;
+
+ if (osf_stream_magic_match(stream->buf + i, match_len)) {
+ if (i)
+ osf_stream_discard(stream, i);
+ return i;
+ }
+ }
+
+ stream->len = 0;
+ return old_len;
+}
+
+static int osf_stream_process(struct osf_stream *stream)
+{
+ struct osf_frame frame;
+ size_t decoded_len;
+ size_t discarded;
+ size_t frame_len;
+ u32 payload_len;
+ int first_err = 0;
+ int ret;
+
+ while (stream->len) {
+ discarded = osf_stream_discard_to_magic(stream);
+ if (discarded) {
+ stream->stats.bad_magic_resyncs++;
+ stream->stats.dropped_bytes += discarded;
+ if (!first_err)
+ first_err = -EPROTO;
+ }
+
+ if (!stream->len)
+ break;
+
+ if (stream->len < OSF_FRAME_HEADER_LEN) {
+ stream->stats.partial_frames++;
+ break;
+ }
+
+ if (get_unaligned_le16(stream->buf + 6) !=
+ OSF_FRAME_HEADER_LEN) {
+ stream->stats.dropped_bytes++;
+ osf_stream_drop_invalid_head(stream);
+ if (!first_err)
+ first_err = -EPROTO;
+ continue;
+ }
+
+ payload_len = get_unaligned_le32(stream->buf + 10);
+ if (payload_len > OSF_STREAM_MAX_PAYLOAD_LEN) {
+ stream->stats.dropped_bytes++;
+ osf_stream_drop_invalid_head(stream);
+ if (!first_err)
+ first_err = -EMSGSIZE;
+ continue;
+ }
+
+ frame_len = OSF_FRAME_HEADER_LEN + payload_len + OSF_FRAME_CRC_LEN;
+ if (stream->len < frame_len) {
+ stream->stats.partial_frames++;
+ break;
+ }
+
+ ret = osf_protocol_decode_frame(stream->buf, frame_len, &frame,
+ &decoded_len);
+ if (ret) {
+ if (ret == -EBADMSG)
+ stream->stats.bad_crc_frames++;
+ stream->stats.dropped_bytes++;
+ osf_stream_drop_invalid_head(stream);
+ if (!first_err)
+ first_err = ret;
+ continue;
+ }
+
+ if (decoded_len != frame_len) {
+ stream->stats.dropped_bytes++;
+ osf_stream_drop_invalid_head(stream);
+ if (!first_err)
+ first_err = -EMSGSIZE;
+ continue;
+ }
+
+ ret = osf_core_receive_frame(stream->osf, stream->buf, frame_len);
+ if (ret) {
+ osf_stream_discard(stream, frame_len);
+ if (!first_err)
+ first_err = ret;
+ continue;
+ }
+
+ stream->stats.valid_frames++;
+ osf_stream_discard(stream, frame_len);
+ }
+
+ return first_err;
+}
+
+void osf_stream_init(struct osf_stream *stream, struct osf_device *osf)
+{
+ if (!stream)
+ return;
+
+ stream->osf = osf;
+ stream->len = 0;
+ memset(&stream->stats, 0, sizeof(stream->stats));
+}
+
+void osf_stream_reset(struct osf_stream *stream)
+{
+ if (stream) {
+ stream->len = 0;
+ memset(&stream->stats, 0, sizeof(stream->stats));
+ }
+}
+
+int osf_stream_receive_bytes(struct osf_stream *stream, const u8 *buf,
+ size_t len)
+{
+ size_t copy_len;
+ size_t space;
+ int first_err = 0;
+ int ret;
+
+ if (!stream || !stream->osf || (!buf && len))
+ return -EINVAL;
+
+ if (!len) {
+ ret = osf_stream_process(stream);
+ if (ret && !first_err)
+ first_err = ret;
+ return first_err;
+ }
+
+ while (len) {
+ space = OSF_STREAM_MAX_FRAME_LEN - stream->len;
+ if (!space) {
+ stream->stats.dropped_bytes++;
+ osf_stream_discard(stream, 1);
+ if (!first_err)
+ first_err = -EMSGSIZE;
+ continue;
+ }
+
+ copy_len = len < space ? len : space;
+ memcpy(stream->buf + stream->len, buf, copy_len);
+ stream->len += copy_len;
+ buf += copy_len;
+ len -= copy_len;
+
+ ret = osf_stream_process(stream);
+ if (ret && !first_err)
+ first_err = ret;
+ }
+
+ return first_err;
+}
diff --git a/drivers/iio/opensensorfusion/osf_stream.h b/drivers/iio/opensensorfusion/osf_stream.h
new file mode 100644
index 000000000..f7f9477fe
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_stream.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _OSF_STREAM_H
+#define _OSF_STREAM_H
+
+#include <linux/types.h>
+
+#define OSF_STREAM_MAX_FRAME_LEN 4096
+
+struct osf_device;
+
+struct osf_stream_stats {
+ u64 valid_frames;
+ u64 bad_magic_resyncs;
+ u64 bad_crc_frames;
+ u64 partial_frames;
+ u64 dropped_bytes;
+};
+
+struct osf_stream {
+ struct osf_device *osf;
+ u8 buf[OSF_STREAM_MAX_FRAME_LEN];
+ size_t len;
+ struct osf_stream_stats stats;
+};
+
+void osf_stream_init(struct osf_stream *stream, struct osf_device *osf);
+void osf_stream_reset(struct osf_stream *stream);
+int osf_stream_receive_bytes(struct osf_stream *stream, const u8 *buf,
+ size_t len);
+
+#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 4/7] iio: osf: add stream parser
2026-05-24 8:53 ` [PATCH RFC v2 4/7] iio: osf: add stream parser Jinseob Kim
@ 2026-05-24 9:41 ` sashiko-bot
2026-05-28 13:58 ` Jonathan Cameron
1 sibling, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-05-24 9:41 UTC (permalink / raw)
To: Jinseob Kim; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The 'partial_frames' statistic is incremented erroneously during normal chunked data reception, making the metric artificially spammed and useless.
- [Low] The frame is successfully parsed into a stack variable, but the populated structure is ignored and completely unused.
--
commit f278438df751263938f58f94af6a218883f9f11f
Author: Jinseob Kim <kimjinseob88@gmail.com>
iio: osf: add stream parser
This commit introduces byte stream assembly and frame synchronization
capabilities for OSF0 frames. It handles buffering, magic sequence matching,
and payload validation before passing frames to the core.
> diff --git a/drivers/iio/opensensorfusion/osf_stream.c b/drivers/iio/opensensorfusion/osf_stream.c
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_stream.c
[ ... ]
> +static int osf_stream_process(struct osf_stream *stream)
> +{
> + struct osf_frame frame;
> + size_t decoded_len;
> + size_t discarded;
> + size_t frame_len;
> + u32 payload_len;
> + int first_err = 0;
> + int ret;
> +
> + while (stream->len) {
> + discarded = osf_stream_discard_to_magic(stream);
[ ... ]
> + if (!stream->len)
> + break;
> +
> + if (stream->len < OSF_FRAME_HEADER_LEN) {
> + stream->stats.partial_frames++;
[Severity: Medium]
Is it intentional to increment this statistic during normal chunked data
reception?
Since stream data often arrives in partial fragments, breaking here is expected
behavior while waiting for the rest of the stream. Incrementing an error
counter on every normal yield might spam the metric and make it less useful
for debugging actual issues.
> + break;
> + }
> +
> + if (get_unaligned_le16(stream->buf + 6) !=
[ ... ]
> + frame_len = OSF_FRAME_HEADER_LEN + payload_len + OSF_FRAME_CRC_LEN;
> + if (stream->len < frame_len) {
> + stream->stats.partial_frames++;
[Severity: Medium]
Similarly, does this falsely increment the partial_frames statistic while
simply waiting for the rest of a valid payload to arrive?
> + break;
> + }
> +
> + ret = osf_protocol_decode_frame(stream->buf, frame_len, &frame,
> + &decoded_len);
[ ... ]
> + if (decoded_len != frame_len) {
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + if (!first_err)
> + first_err = -EMSGSIZE;
> + continue;
> + }
> +
> + ret = osf_core_receive_frame(stream->osf, stream->buf, frame_len);
[Severity: Low]
Does this code need to pass the raw buffer to osf_core_receive_frame()
instead of the parsed struct osf_frame?
The earlier call to osf_protocol_decode_frame() successfully parses the frame
into the stack variable frame, but it appears to go unused. Passing the
already-parsed structure to the core might avoid redundant decoding
inefficiency.
> + if (ret) {
> + osf_stream_discard(stream, frame_len);
> + if (!first_err)
> + first_err = ret;
> + continue;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524085312.15369-1-kimjinseob88@gmail.com?part=4
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 4/7] iio: osf: add stream parser
2026-05-24 8:53 ` [PATCH RFC v2 4/7] iio: osf: add stream parser Jinseob Kim
2026-05-24 9:41 ` sashiko-bot
@ 2026-05-28 13:58 ` Jonathan Cameron
2026-05-29 12:44 ` Kim Jinseob
1 sibling, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2026-05-28 13:58 UTC (permalink / raw)
To: Jinseob Kim
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Sun, 24 May 2026 17:53:09 +0900
Jinseob Kim <kimjinseob88@gmail.com> wrote:
> Add byte stream assembly and resync for OSF0 frames.
Various minor things inline.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
> ---
> drivers/iio/opensensorfusion/osf_stream.c | 207 ++++++++++++++++++++++
> drivers/iio/opensensorfusion/osf_stream.h | 31 ++++
> 2 files changed, 238 insertions(+)
> create mode 100644 drivers/iio/opensensorfusion/osf_stream.c
> create mode 100644 drivers/iio/opensensorfusion/osf_stream.h
>
> diff --git a/drivers/iio/opensensorfusion/osf_stream.c b/drivers/iio/opensensorfusion/osf_stream.c
> new file mode 100644
> index 000000000..a2739c987
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_stream.c
> @@ -0,0 +1,207 @@
> +
> +static size_t osf_stream_discard_to_magic(struct osf_stream *stream)
> +{
> + size_t old_len = stream->len;
> + size_t match_len;
> + size_t i;
> +
> + for (i = 0; i < stream->len; i++) {
for (size_t i = 0; i < stream->len; i++)
> + match_len = stream->len - i;
> + if (match_len > OSF_STREAM_MAGIC_LEN)
> + match_len = OSF_STREAM_MAGIC_LEN;
match_len = min(stream->len - i, OSF_STREAM_MAGIC_LEN);
> +
> + if (osf_stream_magic_match(stream->buf + i, match_len)) {
> + if (i)
> + osf_stream_discard(stream, i);
> + return i;
> + }
> + }
> +
> + stream->len = 0;
> + return old_len;
> +}
> +
> +static int osf_stream_process(struct osf_stream *stream)
> +{
> + struct osf_frame frame;
> + size_t decoded_len;
> + size_t discarded;
> + size_t frame_len;
> + u32 payload_len;
> + int first_err = 0;
> + int ret;
> +
> + while (stream->len) {
> + discarded = osf_stream_discard_to_magic(stream);
> + if (discarded) {
> + stream->stats.bad_magic_resyncs++;
> + stream->stats.dropped_bytes += discarded;
> + if (!first_err)
> + first_err = -EPROTO;
> + }
> +
> + if (!stream->len)
> + break;
> +
> + if (stream->len < OSF_FRAME_HEADER_LEN) {
> + stream->stats.partial_frames++;
> + break;
> + }
> +
> + if (get_unaligned_le16(stream->buf + 6) !=
> + OSF_FRAME_HEADER_LEN) {
if (get_unaligned_le16(stream->buf + 6) != OSF_FRAME_HEADER_LEN) {
is fine. We are a bit flexible on line length when it helps readability.
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + if (!first_err)
> + first_err = -EPROTO;
> + continue;
> + }
> +
> + payload_len = get_unaligned_le32(stream->buf + 10);
> + if (payload_len > OSF_STREAM_MAX_PAYLOAD_LEN) {
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + if (!first_err)
> + first_err = -EMSGSIZE;
> + continue;
> + }
> +
> + frame_len = OSF_FRAME_HEADER_LEN + payload_len + OSF_FRAME_CRC_LEN;
> + if (stream->len < frame_len) {
> + stream->stats.partial_frames++;
> + break;
> + }
> +
> + ret = osf_protocol_decode_frame(stream->buf, frame_len, &frame,
> + &decoded_len);
> + if (ret) {
> + if (ret == -EBADMSG)
> + stream->stats.bad_crc_frames++;
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + if (!first_err)
> + first_err = ret;
> + continue;
> + }
> +
> + if (decoded_len != frame_len) {
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + if (!first_err)
> + first_err = -EMSGSIZE;
> + continue;
> + }
> +
> + ret = osf_core_receive_frame(stream->osf, stream->buf, frame_len);
> + if (ret) {
> + osf_stream_discard(stream, frame_len);
> + if (!first_err)
> + first_err = ret;
> + continue;
> + }
> +
> + stream->stats.valid_frames++;
> + osf_stream_discard(stream, frame_len);
> + }
> +
> + return first_err;
> +}
> +int osf_stream_receive_bytes(struct osf_stream *stream, const u8 *buf,
> + size_t len)
> +{
> + size_t copy_len;
> + size_t space;
> + int first_err = 0;
> + int ret;
> +
> + if (!stream || !stream->osf || (!buf && len))
As in previous patch, how do we get here with any of those being possible?
The last one might make sense as it's about coupling between parameters, but
I'm not seeing the first two as useful unless you clear stream->osf in remove
path or similar. If you do maybe a comment.
> + return -EINVAL;
> +
> + if (!len) {
> + ret = osf_stream_process(stream);
> + if (ret && !first_err)
No way for first erro to be set, so same as:
return osf_stream_process(stream);
If this changes later in this patch series then fine to keep it like this.
> + first_err = ret;
> + return first_err;
> + }
> +
> + while (len) {
> + space = OSF_STREAM_MAX_FRAME_LEN - stream->len;
> + if (!space) {
> + stream->stats.dropped_bytes++;
> + osf_stream_discard(stream, 1);
> + if (!first_err)
> + first_err = -EMSGSIZE;
> + continue;
> + }
> +
> + copy_len = len < space ? len : space;
copy_len = min(len, space);
> + memcpy(stream->buf + stream->len, buf, copy_len);
> + stream->len += copy_len;
> + buf += copy_len;
> + len -= copy_len;
> +
> + ret = osf_stream_process(stream);
> + if (ret && !first_err)
> + first_err = ret;
> + }
> +
> + return first_err;
> +}
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 4/7] iio: osf: add stream parser
2026-05-28 13:58 ` Jonathan Cameron
@ 2026-05-29 12:44 ` Kim Jinseob
0 siblings, 0 replies; 30+ messages in thread
From: Kim Jinseob @ 2026-05-29 12:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
>
> On Thu, May 28, 2026 at 10:58:00PM +0100, Jonathan Cameron wrote:
> > Various minor things inline.
>
> Addressed in RFC v3.
>
> I applied the stream parser cleanups in the new series.
>
> > for (size_t i = 0; i < stream->len; i++)
> > match_len = min(stream->len - i, OSF_STREAM_MAGIC_LEN);
>
> Addressed in RFC v3.
>
> > if (get_unaligned_le16(stream->buf + 6) != OSF_FRAME_HEADER_LEN) {
> >
> > is fine. We are a bit flexible on line length when it helps readability.
>
> Addressed in RFC v3 where it improved readability.
>
> > As in previous patch, how do we get here with any of those being possible?
> > The last one might make sense as it's about coupling between parameters, but
> > I'm not seeing the first two as useful unless you clear stream->osf in remove
> > path or similar. If you do maybe a comment.
>
> Addressed in RFC v3. I removed the unnecessary internal-only checks and kept
> the boundary check for the invalid buffer/length relationship.
>
> > No way for first erro to be set, so same as:
> > return osf_stream_process(stream);
> >
> > If this changes later in this patch series then fine to keep it like this.
>
> Addressed in RFC v3.
>
> > copy_len = min(len, space);
>
> Addressed in RFC v3.
>
> I also reviewed the Sashiko feedback for this patch while preparing RFC v3.
> Normal chunked reception is no longer counted as an error-style partial frame
> condition, and the stream/core path was kept consistent with the revised
> validation boundary used in the rest of the series.
>
> Thanks,
>
> Jinseob
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH RFC v2 5/7] iio: osf: add UART serdev transport
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
` (3 preceding siblings ...)
2026-05-24 8:53 ` [PATCH RFC v2 4/7] iio: osf: add stream parser Jinseob Kim
@ 2026-05-24 8:53 ` Jinseob Kim
2026-05-28 14:06 ` Jonathan Cameron
2026-05-24 8:53 ` [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities Jinseob Kim
` (2 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Jinseob Kim @ 2026-05-24 8:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel,
Jinseob Kim
Add the serdev receive path that feeds decoded OSF0 frames to the core.
Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
---
drivers/iio/Kconfig | 1 +
drivers/iio/Makefile | 1 +
drivers/iio/opensensorfusion/Kconfig | 15 +++
drivers/iio/opensensorfusion/Makefile | 5 +
drivers/iio/opensensorfusion/osf_core.c | 107 +++++++++++++++++++++
drivers/iio/opensensorfusion/osf_core.h | 18 ++++
drivers/iio/opensensorfusion/osf_serdev.c | 111 ++++++++++++++++++++++
7 files changed, 258 insertions(+)
create mode 100644 drivers/iio/opensensorfusion/Kconfig
create mode 100644 drivers/iio/opensensorfusion/Makefile
create mode 100644 drivers/iio/opensensorfusion/osf_core.c
create mode 100644 drivers/iio/opensensorfusion/osf_core.h
create mode 100644 drivers/iio/opensensorfusion/osf_serdev.c
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 661127aed..939f6c546 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -101,6 +101,7 @@ source "drivers/iio/light/Kconfig"
source "drivers/iio/magnetometer/Kconfig"
source "drivers/iio/multiplexer/Kconfig"
source "drivers/iio/orientation/Kconfig"
+source "drivers/iio/opensensorfusion/Kconfig"
source "drivers/iio/test/Kconfig"
if IIO_TRIGGER
source "drivers/iio/trigger/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index cb80ef837..d864fe17b 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -37,6 +37,7 @@ obj-y += light/
obj-y += magnetometer/
obj-y += multiplexer/
obj-y += orientation/
+obj-y += opensensorfusion/
obj-y += position/
obj-y += potentiometer/
obj-y += potentiostat/
diff --git a/drivers/iio/opensensorfusion/Kconfig b/drivers/iio/opensensorfusion/Kconfig
new file mode 100644
index 000000000..360f25b4f
--- /dev/null
+++ b/drivers/iio/opensensorfusion/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config OPEN_SENSOR_FUSION
+ tristate "Open Sensor Fusion UART IIO driver"
+ depends on IIO
+ depends on SERIAL_DEV_BUS
+ select CRC32
+ help
+ Build the Open Sensor Fusion UART receive path.
+
+ The driver receives OSF0 frames over a serdev UART.
+ Frames are decoded and validated before being passed to the
+ driver core.
+ This patch only adds the transport path.
+ IIO device registration is added separately.
diff --git a/drivers/iio/opensensorfusion/Makefile b/drivers/iio/opensensorfusion/Makefile
new file mode 100644
index 000000000..940c82edd
--- /dev/null
+++ b/drivers/iio/opensensorfusion/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_OPEN_SENSOR_FUSION) += open-sensor-fusion.o
+
+open-sensor-fusion-y := osf_core.o osf_protocol.o osf_serdev.o osf_stream.o
diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
new file mode 100644
index 000000000..c867b3158
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_core.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "osf_core.h"
+#include "osf_protocol.h"
+
+#define OSF_RESERVED_MSG_FIRST 0x7f00
+#define OSF_RESERVED_MSG_LAST 0x7fff
+#define OSF_VENDOR_PRIVATE_FIRST 0x8000
+
+void osf_core_init(struct osf_device *osf, struct device *dev)
+{
+ memset(osf, 0, sizeof(*osf));
+ osf->dev = dev;
+}
+
+void osf_core_unregister_iio(struct osf_device *osf)
+{
+}
+
+static int osf_core_validate_sensor_sample(const struct osf_frame *frame)
+{
+ struct osf_sensor_sample sample;
+
+ return osf_protocol_decode_sensor_sample(frame, &sample);
+}
+
+static int osf_core_validate_device_status(const struct osf_frame *frame)
+{
+ struct osf_device_status status;
+ int ret;
+
+ ret = osf_protocol_decode_device_status(frame, &status);
+ if (ret)
+ return ret;
+
+ if (status.reserved)
+ return -EPROTO;
+
+ return 0;
+}
+
+static int osf_core_validate_capability_report(const struct osf_frame *frame)
+{
+ struct osf_capability_entry entry;
+ struct osf_capability_report report;
+ unsigned int i;
+ int ret;
+
+ ret = osf_protocol_decode_capability_report(frame, &report);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < report.capability_count; i++) {
+ ret = osf_protocol_decode_capability_entry(&report, i, &entry);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len)
+{
+ struct osf_frame frame;
+ size_t frame_len;
+ int ret;
+
+ if (!osf || !buf)
+ return -EINVAL;
+
+ ret = osf_protocol_decode_frame(buf, len, &frame, &frame_len);
+ if (ret)
+ return ret;
+
+ if (frame_len != len)
+ return -EMSGSIZE;
+
+ switch (frame.message_type) {
+ case OSF_MSG_SENSOR_SAMPLE:
+ ret = osf_core_validate_sensor_sample(&frame);
+ break;
+ case OSF_MSG_DEVICE_STATUS:
+ ret = osf_core_validate_device_status(&frame);
+ break;
+ case OSF_MSG_CAPABILITY_REPORT:
+ ret = osf_core_validate_capability_report(&frame);
+ break;
+ default:
+ if (frame.message_type >= OSF_RESERVED_MSG_FIRST &&
+ frame.message_type <= OSF_RESERVED_MSG_LAST)
+ ret = 0;
+ else if (frame.message_type >= OSF_VENDOR_PRIVATE_FIRST)
+ ret = 0;
+ else
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ if (!ret)
+ osf->last_sequence = frame.sequence;
+
+ return ret;
+}
diff --git a/drivers/iio/opensensorfusion/osf_core.h b/drivers/iio/opensensorfusion/osf_core.h
new file mode 100644
index 000000000..3680c8c9b
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_core.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _OSF_CORE_H
+#define _OSF_CORE_H
+
+#include <linux/types.h>
+
+struct device;
+
+struct osf_device {
+ struct device *dev;
+ u64 last_sequence;
+};
+
+void osf_core_init(struct osf_device *osf, struct device *dev);
+void osf_core_unregister_iio(struct osf_device *osf);
+int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len);
+
+#endif
diff --git a/drivers/iio/opensensorfusion/osf_serdev.c b/drivers/iio/opensensorfusion/osf_serdev.c
new file mode 100644
index 000000000..f121089ed
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_serdev.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/serdev.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "osf_core.h"
+#include "osf_stream.h"
+
+#define OSF_SERDEV_BAUD 115200
+
+struct osf_serdev {
+ struct serdev_device *serdev;
+ struct osf_device osf;
+ struct osf_stream stream;
+};
+
+static size_t osf_serdev_receive_buf(struct serdev_device *serdev,
+ const u8 *buf, size_t count)
+{
+ struct osf_serdev *osf_uart = serdev_device_get_drvdata(serdev);
+ const struct osf_stream_stats *stats;
+ u64 valid_before;
+ int ret;
+
+ valid_before = osf_uart->stream.stats.valid_frames;
+ ret = osf_stream_receive_bytes(&osf_uart->stream, buf, count);
+ stats = &osf_uart->stream.stats;
+
+ if (ret || stats->valid_frames != valid_before)
+ dev_dbg_ratelimited(&serdev->dev,
+ "rx count=%zu valid=%llu bad_magic=%llu bad_crc=%llu partial=%llu dropped=%llu ret=%d\n",
+ count,
+ (unsigned long long)stats->valid_frames,
+ (unsigned long long)stats->bad_magic_resyncs,
+ (unsigned long long)stats->bad_crc_frames,
+ (unsigned long long)stats->partial_frames,
+ (unsigned long long)stats->dropped_bytes,
+ ret);
+
+ return count;
+}
+
+static const struct serdev_device_ops osf_serdev_ops = {
+ .receive_buf = osf_serdev_receive_buf,
+};
+
+static int osf_serdev_probe(struct serdev_device *serdev)
+{
+ struct osf_serdev *osf_uart;
+ unsigned int baudrate;
+ int ret;
+
+ osf_uart = devm_kzalloc(&serdev->dev, sizeof(*osf_uart), GFP_KERNEL);
+ if (!osf_uart)
+ return -ENOMEM;
+
+ osf_uart->serdev = serdev;
+ osf_core_init(&osf_uart->osf, &serdev->dev);
+ osf_stream_init(&osf_uart->stream, &osf_uart->osf);
+
+ serdev_device_set_drvdata(serdev, osf_uart);
+ serdev_device_set_client_ops(serdev, &osf_serdev_ops);
+
+ ret = serdev_device_open(serdev);
+ if (ret)
+ return ret;
+
+ baudrate = serdev_device_set_baudrate(serdev, OSF_SERDEV_BAUD);
+ if (baudrate != OSF_SERDEV_BAUD)
+ dev_warn(&serdev->dev, "requested %u baud, controller set %u\n",
+ OSF_SERDEV_BAUD, baudrate);
+
+ serdev_device_set_flow_control(serdev, false);
+
+ return 0;
+}
+
+static void osf_serdev_remove(struct serdev_device *serdev)
+{
+ struct osf_serdev *osf_uart = serdev_device_get_drvdata(serdev);
+
+ serdev_device_close(serdev);
+ osf_stream_reset(&osf_uart->stream);
+ osf_core_unregister_iio(&osf_uart->osf);
+}
+
+static const struct of_device_id osf_serdev_of_match[] = {
+ { .compatible = "opensensorfusion,osf-uart" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, osf_serdev_of_match);
+
+static struct serdev_device_driver osf_serdev_driver = {
+ .probe = osf_serdev_probe,
+ .remove = osf_serdev_remove,
+ .driver = {
+ .name = "open-sensor-fusion-uart",
+ .of_match_table = osf_serdev_of_match,
+ },
+};
+
+module_serdev_device_driver(osf_serdev_driver);
+
+MODULE_DESCRIPTION("Open Sensor Fusion IIO driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 5/7] iio: osf: add UART serdev transport
2026-05-24 8:53 ` [PATCH RFC v2 5/7] iio: osf: add UART serdev transport Jinseob Kim
@ 2026-05-28 14:06 ` Jonathan Cameron
2026-05-29 12:47 ` Kim Jinseob
0 siblings, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2026-05-28 14:06 UTC (permalink / raw)
To: Jinseob Kim
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Sun, 24 May 2026 17:53:10 +0900
Jinseob Kim <kimjinseob88@gmail.com> wrote:
> Add the serdev receive path that feeds decoded OSF0 frames to the core.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
Various things inline.
> diff --git a/drivers/iio/opensensorfusion/Kconfig b/drivers/iio/opensensorfusion/Kconfig
> new file mode 100644
> index 000000000..360f25b4f
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config OPEN_SENSOR_FUSION
> + tristate "Open Sensor Fusion UART IIO driver"
> + depends on IIO
> + depends on SERIAL_DEV_BUS
> + select CRC32
> + help
> + Build the Open Sensor Fusion UART receive path.
> +
> + The driver receives OSF0 frames over a serdev UART.
> + Frames are decoded and validated before being passed to the
> + driver core.
> + This patch only adds the transport path.
> + IIO device registration is added separately.
Why is help text talking about a patch?
> diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
> new file mode 100644
> index 000000000..c867b3158
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_core.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include "osf_core.h"
> +#include "osf_protocol.h"
> +
> +#define OSF_RESERVED_MSG_FIRST 0x7f00
> +#define OSF_RESERVED_MSG_LAST 0x7fff
> +#define OSF_VENDOR_PRIVATE_FIRST 0x8000
> +
> +void osf_core_init(struct osf_device *osf, struct device *dev)
> +{
> + memset(osf, 0, sizeof(*osf));
You zero them memory before passing to this. That seems like a sensible
pattern in which case this memset is unneeded.
> + osf->dev = dev;
> +}
> +
> +int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len)
> +{
> + struct osf_frame frame;
> + size_t frame_len;
> + int ret;
> +
> + if (!osf || !buf)
> + return -EINVAL;
If these can happen add a comment on why. If not remove them as overly cautious
checking.
> +
> + ret = osf_protocol_decode_frame(buf, len, &frame, &frame_len);
> + if (ret)
> + return ret;
> +
> + if (frame_len != len)
> + return -EMSGSIZE;
> +
> + switch (frame.message_type) {
> + case OSF_MSG_SENSOR_SAMPLE:
> + ret = osf_core_validate_sensor_sample(&frame);
> + break;
> + case OSF_MSG_DEVICE_STATUS:
> + ret = osf_core_validate_device_status(&frame);
> + break;
> + case OSF_MSG_CAPABILITY_REPORT:
> + ret = osf_core_validate_capability_report(&frame);
Perhaps check ret in each of these and return early if set. Then we only
do the shared path below on success.
> + break;
> + default:
> + if (frame.message_type >= OSF_RESERVED_MSG_FIRST &&
> + frame.message_type <= OSF_RESERVED_MSG_LAST)
> + ret = 0;
> + else if (frame.message_type >= OSF_VENDOR_PRIVATE_FIRST)
> + ret = 0;
> + else
> + ret = -EOPNOTSUPP;
Given there is nothing else to do on error, return -EOPNOTSUPP; perhaps.
> + break;
> + }
> +
> + if (!ret)
> + osf->last_sequence = frame.sequence;
> +
> + return ret;
> +}
> diff --git a/drivers/iio/opensensorfusion/osf_serdev.c b/drivers/iio/opensensorfusion/osf_serdev.c
> new file mode 100644
> index 000000000..f121089ed
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_serdev.c
> +
> +static int osf_serdev_probe(struct serdev_device *serdev)
> +{
> + struct osf_serdev *osf_uart;
> + unsigned int baudrate;
> + int ret;
> +
> + osf_uart = devm_kzalloc(&serdev->dev, sizeof(*osf_uart), GFP_KERNEL);
> + if (!osf_uart)
> + return -ENOMEM;
> +
> + osf_uart->serdev = serdev;
> + osf_core_init(&osf_uart->osf, &serdev->dev);
> + osf_stream_init(&osf_uart->stream, &osf_uart->osf);
> +
> + serdev_device_set_drvdata(serdev, osf_uart);
> + serdev_device_set_client_ops(serdev, &osf_serdev_ops);
> +
> + ret = serdev_device_open(serdev);
> + if (ret)
> + return ret;
> +
> + baudrate = serdev_device_set_baudrate(serdev, OSF_SERDEV_BAUD);
> + if (baudrate != OSF_SERDEV_BAUD)
> + dev_warn(&serdev->dev, "requested %u baud, controller set %u\n",
> + OSF_SERDEV_BAUD, baudrate);
> +
> + serdev_device_set_flow_control(serdev, false);
> +
> + return 0;
> +}
> +
> +static void osf_serdev_remove(struct serdev_device *serdev)
> +{
> + struct osf_serdev *osf_uart = serdev_device_get_drvdata(serdev);
> +
> + serdev_device_close(serdev);
> + osf_stream_reset(&osf_uart->stream);
> + osf_core_unregister_iio(&osf_uart->osf);
Given these don't match up with things in probe() please add some comments
to explain what they are undoing.
> +}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 5/7] iio: osf: add UART serdev transport
2026-05-28 14:06 ` Jonathan Cameron
@ 2026-05-29 12:47 ` Kim Jinseob
0 siblings, 0 replies; 30+ messages in thread
From: Kim Jinseob @ 2026-05-29 12:47 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Thu, May 28, 2026 at 11:06:00PM +0100, Jonathan Cameron wrote:
> Various things inline.
Addressed in RFC v3.
> Why is help text talking about a patch?
Addressed in RFC v3. I updated the Kconfig help text so it describes the
driver rather than the patch split.
> You zero them memory before passing to this. That seems like a sensible
> pattern in which case this memset is unneeded.
Addressed in RFC v3. The unnecessary memset in osf_core_init() was removed.
> If these can happen add a comment on why. If not remove them as overly cautious
> checking.
Addressed in RFC v3. I removed the internal-only NULL checks in the core
receive path while keeping the protocol and stream boundary checks.
> Perhaps check ret in each of these and return early if set. Then we only
> do the shared path below on success.
Addressed in RFC v3. The receive path now uses a simpler early-return style.
> Given there is nothing else to do on error, return -EOPNOTSUPP; perhaps.
Addressed in RFC v3.
> Given these don't match up with things in probe() please add some comments
> to explain what they are undoing.
Addressed in RFC v3. I added a short remove-path ordering comment before the
serdev close/reset/unregister sequence.
Thanks
Jinseob
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
` (4 preceding siblings ...)
2026-05-24 8:53 ` [PATCH RFC v2 5/7] iio: osf: add UART serdev transport Jinseob Kim
@ 2026-05-24 8:53 ` Jinseob Kim
2026-05-24 10:57 ` sashiko-bot
2026-05-28 14:30 ` Jonathan Cameron
2026-05-24 8:53 ` [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver Jinseob Kim
2026-06-02 23:59 ` [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Andy Shevchenko
7 siblings, 2 replies; 30+ messages in thread
From: Jinseob Kim @ 2026-05-24 8:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel,
Jinseob Kim
Register supported IIO devices from the first valid capability report.
Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
---
drivers/iio/opensensorfusion/Kconfig | 12 +-
drivers/iio/opensensorfusion/Makefile | 3 +-
drivers/iio/opensensorfusion/osf_core.c | 248 ++++++++++++++++++--
drivers/iio/opensensorfusion/osf_core.h | 49 ++++
drivers/iio/opensensorfusion/osf_iio.c | 288 ++++++++++++++++++++++++
drivers/iio/opensensorfusion/osf_iio.h | 22 ++
6 files changed, 593 insertions(+), 29 deletions(-)
create mode 100644 drivers/iio/opensensorfusion/osf_iio.c
create mode 100644 drivers/iio/opensensorfusion/osf_iio.h
diff --git a/drivers/iio/opensensorfusion/Kconfig b/drivers/iio/opensensorfusion/Kconfig
index 360f25b4f..004d2aa8a 100644
--- a/drivers/iio/opensensorfusion/Kconfig
+++ b/drivers/iio/opensensorfusion/Kconfig
@@ -5,11 +5,11 @@ config OPEN_SENSOR_FUSION
depends on IIO
depends on SERIAL_DEV_BUS
select CRC32
+ select IIO_KFIFO_BUF
help
- Build the Open Sensor Fusion UART receive path.
+ Build the Open Sensor Fusion UART IIO driver.
- The driver receives OSF0 frames over a serdev UART.
- Frames are decoded and validated before being passed to the
- driver core.
- This patch only adds the transport path.
- IIO device registration is added separately.
+ The driver receives OSF0 frames over a serdev UART and registers
+ IIO devices for supported capability entries.
+ Raw reads use the latest decoded samples. Buffered reads use
+ fixed scan layouts and IIO timestamps.
diff --git a/drivers/iio/opensensorfusion/Makefile b/drivers/iio/opensensorfusion/Makefile
index 940c82edd..b4e03b80c 100644
--- a/drivers/iio/opensensorfusion/Makefile
+++ b/drivers/iio/opensensorfusion/Makefile
@@ -2,4 +2,5 @@
obj-$(CONFIG_OPEN_SENSOR_FUSION) += open-sensor-fusion.o
-open-sensor-fusion-y := osf_core.o osf_protocol.o osf_serdev.o osf_stream.o
+open-sensor-fusion-y := osf_core.o osf_iio.o osf_protocol.o osf_serdev.o \
+ osf_stream.o
diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
index c867b3158..a1e20ede9 100644
--- a/drivers/iio/opensensorfusion/osf_core.c
+++ b/drivers/iio/opensensorfusion/osf_core.c
@@ -5,7 +5,7 @@
#include <linux/types.h>
#include "osf_core.h"
-#include "osf_protocol.h"
+#include "osf_iio.h"
#define OSF_RESERVED_MSG_FIRST 0x7f00
#define OSF_RESERVED_MSG_LAST 0x7fff
@@ -19,17 +19,171 @@ void osf_core_init(struct osf_device *osf, struct device *dev)
void osf_core_unregister_iio(struct osf_device *osf)
{
+ unsigned int i;
+
+ for (i = 0; i < osf->iio_dev_count; i++)
+ osf_iio_unregister_sensor(osf->iio_devs[i].indio_dev);
+
+ osf->iio_dev_count = 0;
+}
+
+static struct iio_dev *osf_core_find_iio_dev(struct osf_device *osf,
+ u16 sensor_type, u16 sensor_index)
+{
+ const struct osf_iio_binding *binding;
+ unsigned int i;
+
+ for (i = 0; i < osf->iio_dev_count; i++) {
+ binding = &osf->iio_devs[i];
+ if (binding->sensor_type == sensor_type &&
+ binding->sensor_index == sensor_index)
+ return binding->indio_dev;
+ }
+
+ return NULL;
+}
+
+static struct osf_latest_sample *
+osf_core_find_latest_sample(struct osf_device *osf, u16 sensor_type,
+ u16 sensor_index)
+{
+ struct osf_latest_sample *latest;
+ unsigned int i;
+
+ for (i = 0; i < osf->latest_sample_count; i++) {
+ latest = &osf->latest_samples[i];
+ if (latest->sensor_type == sensor_type &&
+ latest->sensor_index == sensor_index)
+ return latest;
+ }
+
+ if (osf->latest_sample_count >= OSF_MAX_CAPABILITIES)
+ return NULL;
+
+ return &osf->latest_samples[osf->latest_sample_count++];
+}
+
+static bool osf_core_capability_duplicate(const struct osf_capability_cache *cache,
+ unsigned int index)
+{
+ const struct osf_capability_entry *entry = &cache->entries[index];
+ unsigned int i;
+
+ for (i = 0; i < index; i++) {
+ if (!osf_iio_sensor_supported(cache->entries[i].sensor_type,
+ cache->entries[i].channel_count))
+ continue;
+
+ if (cache->entries[i].sensor_type == entry->sensor_type &&
+ cache->entries[i].sensor_index == entry->sensor_index)
+ return true;
+ }
+
+ return false;
+}
+
+static int osf_core_register_capabilities(struct osf_device *osf,
+ const struct osf_capability_cache *cache)
+{
+ struct iio_dev *indio_dev;
+ unsigned int i;
+ int ret;
+
+ if (osf->capability_cache.valid)
+ return 0;
+
+ for (i = 0; i < cache->capability_count; i++) {
+ if (!osf_iio_sensor_supported(cache->entries[i].sensor_type,
+ cache->entries[i].channel_count))
+ continue;
+
+ if (osf_core_capability_duplicate(cache, i))
+ return -EEXIST;
+ }
+
+ for (i = 0; i < cache->capability_count; i++) {
+ if (!osf_iio_sensor_supported(cache->entries[i].sensor_type,
+ cache->entries[i].channel_count))
+ continue;
+
+ ret = osf_iio_register_sensor(osf->dev, &cache->entries[i],
+ osf, &indio_dev);
+ if (ret)
+ goto err_unregister;
+
+ osf->iio_devs[osf->iio_dev_count].sensor_type =
+ cache->entries[i].sensor_type;
+ osf->iio_devs[osf->iio_dev_count].sensor_index =
+ cache->entries[i].sensor_index;
+ osf->iio_devs[osf->iio_dev_count].indio_dev = indio_dev;
+ osf->iio_dev_count++;
+ }
+
+ return 0;
+
+err_unregister:
+ osf_core_unregister_iio(osf);
+
+ return ret;
}
-static int osf_core_validate_sensor_sample(const struct osf_frame *frame)
+static int osf_core_handle_sensor_sample(struct osf_device *osf,
+ const struct osf_frame *frame)
{
+ struct osf_latest_sample *latest;
struct osf_sensor_sample sample;
+ struct iio_dev *indio_dev;
+ unsigned int i;
+ int ret;
- return osf_protocol_decode_sensor_sample(frame, &sample);
+ ret = osf_protocol_decode_sensor_sample(frame, &sample);
+ if (ret)
+ return ret;
+
+ if (sample.channel_count > OSF_MAX_SAMPLE_CHANNELS)
+ return -E2BIG;
+
+ latest = osf_core_find_latest_sample(osf, sample.sensor_type,
+ sample.sensor_index);
+ if (!latest)
+ return -E2BIG;
+
+ for (i = 0; i < sample.channel_count; i++) {
+ ret = osf_protocol_sensor_sample_value(&sample, i,
+ &latest->values[i]);
+ if (ret)
+ return ret;
+ }
+
+ for (; i < OSF_MAX_SAMPLE_CHANNELS; i++)
+ latest->values[i] = 0;
+
+ latest->sensor_type = sample.sensor_type;
+ latest->sensor_index = sample.sensor_index;
+ latest->channel_count = sample.channel_count;
+ latest->sample_format = sample.sample_format;
+ latest->scale_nano = sample.scale_nano;
+ latest->sequence = frame->sequence;
+ latest->timestamp_us = frame->timestamp_us;
+ latest->valid = true;
+ osf->last_sequence = frame->sequence;
+
+ indio_dev = osf_core_find_iio_dev(osf, sample.sensor_type,
+ sample.sensor_index);
+ if (indio_dev) {
+ ret = osf_iio_push_sample(indio_dev, latest->values,
+ latest->channel_count);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
}
-static int osf_core_validate_device_status(const struct osf_frame *frame)
+static int osf_core_handle_device_status(struct osf_device *osf,
+ const struct osf_frame *frame)
{
+ struct osf_status_cache cache = { };
struct osf_device_status status;
int ret;
@@ -40,12 +194,22 @@ static int osf_core_validate_device_status(const struct osf_frame *frame)
if (status.reserved)
return -EPROTO;
+ cache.uptime_s = status.uptime_s;
+ cache.status_flags = status.status_flags;
+ cache.error_flags = status.error_flags;
+ cache.dropped_frames = status.dropped_frames;
+ cache.sequence = frame->sequence;
+ cache.valid = true;
+ osf->status_cache = cache;
+ osf->last_sequence = frame->sequence;
+
return 0;
}
-static int osf_core_validate_capability_report(const struct osf_frame *frame)
+static int osf_core_handle_capability_report(struct osf_device *osf,
+ const struct osf_frame *frame)
{
- struct osf_capability_entry entry;
+ struct osf_capability_cache cache = { };
struct osf_capability_report report;
unsigned int i;
int ret;
@@ -54,12 +218,32 @@ static int osf_core_validate_capability_report(const struct osf_frame *frame)
if (ret)
return ret;
+ if (report.capability_count > OSF_MAX_CAPABILITIES)
+ return -E2BIG;
+
+ if (osf->capability_cache.valid) {
+ osf->last_sequence = frame->sequence;
+ return 0;
+ }
+
for (i = 0; i < report.capability_count; i++) {
- ret = osf_protocol_decode_capability_entry(&report, i, &entry);
+ ret = osf_protocol_decode_capability_entry(&report, i,
+ &cache.entries[i]);
if (ret)
return ret;
}
+ cache.capability_count = report.capability_count;
+ cache.sequence = frame->sequence;
+ cache.valid = true;
+
+ ret = osf_core_register_capabilities(osf, &cache);
+ if (ret)
+ return ret;
+
+ osf->capability_cache = cache;
+ osf->last_sequence = frame->sequence;
+
return 0;
}
@@ -81,27 +265,47 @@ int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len)
switch (frame.message_type) {
case OSF_MSG_SENSOR_SAMPLE:
- ret = osf_core_validate_sensor_sample(&frame);
- break;
+ return osf_core_handle_sensor_sample(osf, &frame);
case OSF_MSG_DEVICE_STATUS:
- ret = osf_core_validate_device_status(&frame);
- break;
+ return osf_core_handle_device_status(osf, &frame);
case OSF_MSG_CAPABILITY_REPORT:
- ret = osf_core_validate_capability_report(&frame);
- break;
+ return osf_core_handle_capability_report(osf, &frame);
default:
if (frame.message_type >= OSF_RESERVED_MSG_FIRST &&
frame.message_type <= OSF_RESERVED_MSG_LAST)
- ret = 0;
- else if (frame.message_type >= OSF_VENDOR_PRIVATE_FIRST)
- ret = 0;
- else
- ret = -EOPNOTSUPP;
- break;
+ return 0;
+ if (frame.message_type >= OSF_VENDOR_PRIVATE_FIRST)
+ return 0;
+ return -EOPNOTSUPP;
}
+}
- if (!ret)
- osf->last_sequence = frame.sequence;
+int osf_core_read_latest_sample(struct osf_device *osf, u16 sensor_type,
+ u16 sensor_index, unsigned int channel,
+ s32 *value)
+{
+ const struct osf_latest_sample *latest;
+ unsigned int i;
- return ret;
+ if (!osf || !value)
+ return -EINVAL;
+
+ for (i = 0; i < osf->latest_sample_count; i++) {
+ latest = &osf->latest_samples[i];
+ if (latest->sensor_type == sensor_type &&
+ latest->sensor_index == sensor_index)
+ goto found;
+ }
+
+ return -ENODATA;
+
+found:
+ if (!latest->valid)
+ return -ENODATA;
+ if (channel >= latest->channel_count)
+ return -ENODATA;
+
+ *value = latest->values[channel];
+
+ return 0;
}
diff --git a/drivers/iio/opensensorfusion/osf_core.h b/drivers/iio/opensensorfusion/osf_core.h
index 3680c8c9b..e5c0d5081 100644
--- a/drivers/iio/opensensorfusion/osf_core.h
+++ b/drivers/iio/opensensorfusion/osf_core.h
@@ -4,15 +4,64 @@
#include <linux/types.h>
+#include "osf_protocol.h"
+
+#define OSF_MAX_SAMPLE_CHANNELS 3
+#define OSF_MAX_CAPABILITIES 16
+
struct device;
+struct iio_dev;
+
+struct osf_latest_sample {
+ u16 sensor_type;
+ u16 sensor_index;
+ u16 channel_count;
+ u16 sample_format;
+ u32 scale_nano;
+ s32 values[OSF_MAX_SAMPLE_CHANNELS];
+ u64 sequence;
+ u64 timestamp_us;
+ bool valid;
+};
+
+struct osf_capability_cache {
+ u16 capability_count;
+ struct osf_capability_entry entries[OSF_MAX_CAPABILITIES];
+ u64 sequence;
+ bool valid;
+};
+
+struct osf_status_cache {
+ u32 uptime_s;
+ u32 status_flags;
+ u32 error_flags;
+ u32 dropped_frames;
+ u64 sequence;
+ bool valid;
+};
+
+struct osf_iio_binding {
+ u16 sensor_type;
+ u16 sensor_index;
+ struct iio_dev *indio_dev;
+};
struct osf_device {
struct device *dev;
+ struct osf_latest_sample latest_samples[OSF_MAX_CAPABILITIES];
+ unsigned int latest_sample_count;
+ struct osf_capability_cache capability_cache;
+ struct osf_status_cache status_cache;
+ struct osf_iio_binding iio_devs[OSF_MAX_CAPABILITIES];
+ unsigned int iio_dev_count;
u64 last_sequence;
};
void osf_core_init(struct osf_device *osf, struct device *dev);
void osf_core_unregister_iio(struct osf_device *osf);
int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len);
+int osf_core_read_latest_sample(struct osf_device *osf, u16 sensor_type,
+ u16 sensor_index, unsigned int channel,
+ s32 *value);
#endif
diff --git a/drivers/iio/opensensorfusion/osf_iio.c b/drivers/iio/opensensorfusion/osf_iio.c
new file mode 100644
index 000000000..1f82ec063
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_iio.c
@@ -0,0 +1,288 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include "osf_core.h"
+#include "osf_iio.h"
+
+struct osf_iio_sensor_spec {
+ u16 sensor_type;
+ u16 channel_count;
+ const char *name;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+ const unsigned long *available_scan_masks;
+};
+
+struct osf_iio_state {
+ const struct osf_iio_sensor_spec *spec;
+ struct iio_buffer *buffer;
+ u32 scale_nano;
+ u16 sensor_index;
+ struct osf_device *osf;
+};
+
+#define OSF_SCAN_TYPE_S32 \
+ { \
+ .sign = 's', \
+ .realbits = 32, \
+ .storagebits = 32, \
+ .endianness = IIO_CPU, \
+ }
+
+#define OSF_MOD_CHAN(_type, _mod, _idx) \
+ { \
+ .type = (_type), \
+ .modified = 1, \
+ .channel2 = (_mod), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .scan_index = (_idx), \
+ .scan_type = OSF_SCAN_TYPE_S32, \
+ }
+
+#define OSF_CHAN(_type, _idx) \
+ { \
+ .type = (_type), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .scan_index = (_idx), \
+ .scan_type = OSF_SCAN_TYPE_S32, \
+ }
+
+static const struct iio_chan_spec osf_accel_channels[] = {
+ OSF_MOD_CHAN(IIO_ACCEL, IIO_MOD_X, 0),
+ OSF_MOD_CHAN(IIO_ACCEL, IIO_MOD_Y, 1),
+ OSF_MOD_CHAN(IIO_ACCEL, IIO_MOD_Z, 2),
+ IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static const struct iio_chan_spec osf_gyro_channels[] = {
+ OSF_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_X, 0),
+ OSF_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_Y, 1),
+ OSF_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_Z, 2),
+ IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static const struct iio_chan_spec osf_mag_channels[] = {
+ OSF_MOD_CHAN(IIO_MAGN, IIO_MOD_X, 0),
+ OSF_MOD_CHAN(IIO_MAGN, IIO_MOD_Y, 1),
+ OSF_MOD_CHAN(IIO_MAGN, IIO_MOD_Z, 2),
+ IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static const struct iio_chan_spec osf_temp_channels[] = {
+ OSF_CHAN(IIO_TEMP, 0),
+ IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const unsigned long osf_3axis_available_scan_masks[] = {
+ GENMASK(2, 0),
+ 0,
+};
+
+static const unsigned long osf_temp_available_scan_masks[] = {
+ BIT(0),
+ 0,
+};
+
+static const struct osf_iio_sensor_spec osf_iio_sensor_specs[] = {
+ {
+ .sensor_type = OSF_SENSOR_ACCELEROMETER,
+ .channel_count = 3,
+ .name = "osf-accel",
+ .channels = osf_accel_channels,
+ .num_channels = ARRAY_SIZE(osf_accel_channels),
+ .available_scan_masks = osf_3axis_available_scan_masks,
+ },
+ {
+ .sensor_type = OSF_SENSOR_GYROSCOPE,
+ .channel_count = 3,
+ .name = "osf-gyro",
+ .channels = osf_gyro_channels,
+ .num_channels = ARRAY_SIZE(osf_gyro_channels),
+ .available_scan_masks = osf_3axis_available_scan_masks,
+ },
+ {
+ .sensor_type = OSF_SENSOR_MAGNETOMETER,
+ .channel_count = 3,
+ .name = "osf-magn",
+ .channels = osf_mag_channels,
+ .num_channels = ARRAY_SIZE(osf_mag_channels),
+ .available_scan_masks = osf_3axis_available_scan_masks,
+ },
+ {
+ .sensor_type = OSF_SENSOR_TEMPERATURE,
+ .channel_count = 1,
+ .name = "osf-temp",
+ .channels = osf_temp_channels,
+ .num_channels = ARRAY_SIZE(osf_temp_channels),
+ .available_scan_masks = osf_temp_available_scan_masks,
+ },
+};
+
+static const struct osf_iio_sensor_spec *
+osf_iio_find_sensor_spec(u16 sensor_type, u16 channel_count)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(osf_iio_sensor_specs); i++) {
+ if (osf_iio_sensor_specs[i].sensor_type == sensor_type &&
+ osf_iio_sensor_specs[i].channel_count == channel_count)
+ return &osf_iio_sensor_specs[i];
+ }
+
+ return NULL;
+}
+
+bool osf_iio_sensor_supported(u16 sensor_type, u16 channel_count)
+{
+ return !!osf_iio_find_sensor_spec(sensor_type, channel_count);
+}
+
+const char *osf_iio_sensor_name(u16 sensor_type)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(osf_iio_sensor_specs); i++) {
+ if (osf_iio_sensor_specs[i].sensor_type == sensor_type)
+ return osf_iio_sensor_specs[i].name;
+ }
+
+ return NULL;
+}
+
+static int osf_iio_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int *val,
+ int *val2, long mask)
+{
+ struct osf_iio_state *state = iio_priv(indio_dev);
+ s32 raw;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (!state->osf)
+ return -ENODATA;
+
+ if (chan->scan_index < 0)
+ return -EINVAL;
+
+ ret = osf_core_read_latest_sample(state->osf,
+ state->spec->sensor_type,
+ state->sensor_index,
+ chan->scan_index, &raw);
+ if (ret)
+ return ret;
+
+ *val = raw;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ *val = state->scale_nano / NANO;
+ *val2 = state->scale_nano % NANO;
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info osf_iio_info = {
+ .read_raw = osf_iio_read_raw,
+};
+
+int osf_iio_register_sensor(struct device *dev,
+ const struct osf_capability_entry *entry,
+ struct osf_device *osf, struct iio_dev **indio_dev)
+{
+ const struct osf_iio_sensor_spec *spec;
+ struct osf_iio_state *state;
+ struct iio_dev *iio_dev;
+ int ret;
+
+ spec = osf_iio_find_sensor_spec(entry->sensor_type,
+ entry->channel_count);
+ if (!spec)
+ return -EOPNOTSUPP;
+
+ if (entry->sample_format != OSF_SAMPLE_FORMAT_S32)
+ return -EOPNOTSUPP;
+
+ iio_dev = iio_device_alloc(dev, sizeof(*state));
+ if (!iio_dev)
+ return -ENOMEM;
+
+ state = iio_priv(iio_dev);
+ state->spec = spec;
+ state->scale_nano = entry->scale_nano;
+ state->sensor_index = entry->sensor_index;
+ state->osf = osf;
+
+ iio_dev->name = spec->name;
+ iio_dev->info = &osf_iio_info;
+ iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+ iio_dev->channels = spec->channels;
+ iio_dev->num_channels = spec->num_channels;
+ iio_dev->available_scan_masks = spec->available_scan_masks;
+
+ state->buffer = iio_kfifo_allocate();
+ if (!state->buffer) {
+ ret = -ENOMEM;
+ goto err_free_iio;
+ }
+
+ ret = iio_device_attach_buffer(iio_dev, state->buffer);
+ if (ret)
+ goto err_free_buffer;
+
+ ret = iio_device_register(iio_dev);
+ if (ret)
+ goto err_free_buffer;
+
+ if (indio_dev)
+ *indio_dev = iio_dev;
+
+ return 0;
+
+err_free_buffer:
+ iio_kfifo_free(state->buffer);
+err_free_iio:
+ iio_device_free(iio_dev);
+
+ return ret;
+}
+
+void osf_iio_unregister_sensor(struct iio_dev *indio_dev)
+{
+ struct osf_iio_state *state;
+
+ if (!indio_dev)
+ return;
+
+ state = iio_priv(indio_dev);
+ iio_device_unregister(indio_dev);
+ iio_kfifo_free(state->buffer);
+ iio_device_free(indio_dev);
+}
+
+int osf_iio_push_sample(struct iio_dev *indio_dev, const s32 *values,
+ unsigned int channel_count)
+{
+ s64 timestamp;
+
+ if (!iio_buffer_enabled(indio_dev))
+ return 0;
+
+ timestamp = iio_get_time_ns(indio_dev);
+
+ return iio_push_to_buffers_with_ts_unaligned(indio_dev, values,
+ channel_count * sizeof(*values),
+ timestamp);
+}
diff --git a/drivers/iio/opensensorfusion/osf_iio.h b/drivers/iio/opensensorfusion/osf_iio.h
new file mode 100644
index 000000000..d90c58fc4
--- /dev/null
+++ b/drivers/iio/opensensorfusion/osf_iio.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _OSF_IIO_H
+#define _OSF_IIO_H
+
+#include <linux/types.h>
+
+#include "osf_protocol.h"
+
+struct device;
+struct iio_dev;
+struct osf_device;
+
+int osf_iio_register_sensor(struct device *dev,
+ const struct osf_capability_entry *entry,
+ struct osf_device *osf, struct iio_dev **indio_dev);
+void osf_iio_unregister_sensor(struct iio_dev *indio_dev);
+int osf_iio_push_sample(struct iio_dev *indio_dev, const s32 *values,
+ unsigned int channel_count);
+bool osf_iio_sensor_supported(u16 sensor_type, u16 channel_count);
+const char *osf_iio_sensor_name(u16 sensor_type);
+
+#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities
2026-05-24 8:53 ` [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities Jinseob Kim
@ 2026-05-24 10:57 ` sashiko-bot
2026-05-28 14:30 ` Jonathan Cameron
1 sibling, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-05-24 10:57 UTC (permalink / raw)
To: Jinseob Kim; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The OPEN_SENSOR_FUSION Kconfig entry selects IIO_KFIFO_BUF without satisfying its IIO_BUFFER dependency.
- [High] Uninitialized kernel heap memory is leaked to userspace via the IIO buffer due to an unvalidated channel count.
- [High] Concurrent access to the shared `latest_samples` array without locking or memory barriers.
--
commit 699c891149ed43c06353cf9d1622dbd226109bd7
Author: Jinseob Kim <kimjinseob88@gmail.com>
iio: osf: register IIO devices from capabilities
Register supported IIO devices from the first valid capability report.
> diff --git a/drivers/iio/opensensorfusion/Kconfig b/drivers/iio/opensensorfusion/Kconfig
> index 360f25b4fe60b..004d2aa8ae10b 100644
> --- a/drivers/iio/opensensorfusion/Kconfig
> +++ b/drivers/iio/opensensorfusion/Kconfig
> @@ -5,11 +5,11 @@ config OPEN_SENSOR_FUSION
> depends on IIO
> depends on SERIAL_DEV_BUS
> select CRC32
> + select IIO_KFIFO_BUF
[Severity: High]
Does this config also need to select or depend on IIO_BUFFER?
Since IIO_KFIFO_BUF is defined within an 'if IIO_BUFFER' block in the Kconfig
hierarchy, selecting it without having IIO_BUFFER enabled can cause unmet
direct dependency warnings and build failures if core IIO buffer support is
disabled.
> help
> - Build the Open Sensor Fusion UART receive path.
[ ... ]
> diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
> index c867b315865d2..a1e20ede9b149 100644
> --- a/drivers/iio/opensensorfusion/osf_core.c
> +++ b/drivers/iio/opensensorfusion/osf_core.c
[ ... ]
> +static int osf_core_handle_sensor_sample(struct osf_device *osf,
> + const struct osf_frame *frame)
> {
> + struct osf_latest_sample *latest;
[ ... ]
> + latest->sensor_type = sample.sensor_type;
> + latest->sensor_index = sample.sensor_index;
> + latest->channel_count = sample.channel_count;
> + latest->sample_format = sample.sample_format;
> + latest->scale_nano = sample.scale_nano;
> + latest->sequence = frame->sequence;
> + latest->timestamp_us = frame->timestamp_us;
> + latest->valid = true;
[Severity: High]
Can this array update cause data corruption for concurrent readers?
osf_core_handle_sensor_sample() updates the latest_samples array from the
serdev receive context, while osf_core_read_latest_sample() reads these
values from process context when userspace accesses sysfs attributes.
Since there is no synchronization like a mutex or seqlock protecting the
array, is it possible for readers to experience torn reads, partially
updated states, or see valid == true before the payload data is fully
committed?
> + osf->last_sequence = frame->sequence;
> +
> + indio_dev = osf_core_find_iio_dev(osf, sample.sensor_type,
> + sample.sensor_index);
> + if (indio_dev) {
> + ret = osf_iio_push_sample(indio_dev, latest->values,
> + latest->channel_count);
[Severity: High]
Could this leak uninitialized kernel heap memory to userspace?
The channel_count is assigned directly from the incoming network frame. If a
frame arrives with a smaller channel_count than the sensor's expected
num_channels, calling osf_iio_push_sample() ->
iio_push_to_buffers_with_ts_unaligned() allocates a bounce buffer using
devm_krealloc(..., GFP_KERNEL) which does not zero memory.
It then copies only channel_count * sizeof(s32) bytes into it. The remaining
space in the bounce buffer would be left uninitialized before being pushed to
the kfifo, potentially exposing kernel heap memory to userspace readers.
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524085312.15369-1-kimjinseob88@gmail.com?part=6
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities
2026-05-24 8:53 ` [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities Jinseob Kim
2026-05-24 10:57 ` sashiko-bot
@ 2026-05-28 14:30 ` Jonathan Cameron
2026-05-29 12:49 ` Kim Jinseob
1 sibling, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2026-05-28 14:30 UTC (permalink / raw)
To: Jinseob Kim
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Sun, 24 May 2026 17:53:11 +0900
Jinseob Kim <kimjinseob88@gmail.com> wrote:
> Register supported IIO devices from the first valid capability report.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
Various things inline.
> diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
> index c867b3158..a1e20ede9 100644
> --- a/drivers/iio/opensensorfusion/osf_core.c
> +++ b/drivers/iio/opensensorfusion/osf_core.c
> @@ -5,7 +5,7 @@
> #include <linux/types.h>
>
> #include "osf_core.h"
> -#include "osf_protocol.h"
Why?
> +#include "osf_iio.h"
>
> #define OSF_RESERVED_MSG_FIRST 0x7f00
> #define OSF_RESERVED_MSG_LAST 0x7fff
> +
> +static bool osf_core_capability_duplicate(const struct osf_capability_cache *cache,
This sounds like it makes a duplicate. Rename to _isduplicate() or something like that.
> + unsigned int index)
> +{
> + const struct osf_capability_entry *entry = &cache->entries[index];
> + unsigned int i;
> +
> + for (i = 0; i < index; i++) {
> + if (!osf_iio_sensor_supported(cache->entries[i].sensor_type,
> + cache->entries[i].channel_count))
> + continue;
> +
> + if (cache->entries[i].sensor_type == entry->sensor_type &&
> + cache->entries[i].sensor_index == entry->sensor_index)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int osf_core_register_capabilities(struct osf_device *osf,
> + const struct osf_capability_cache *cache)
> +{
> + struct iio_dev *indio_dev;
> + unsigned int i;
> + int ret;
> +
> + if (osf->capability_cache.valid)
> + return 0;
> +
> + for (i = 0; i < cache->capability_count; i++) {
> + if (!osf_iio_sensor_supported(cache->entries[i].sensor_type,
> + cache->entries[i].channel_count))
> + continue;
> +
> + if (osf_core_capability_duplicate(cache, i))
> + return -EEXIST;
> + }
> +
> + for (i = 0; i < cache->capability_count; i++) {
> + if (!osf_iio_sensor_supported(cache->entries[i].sensor_type,
> + cache->entries[i].channel_count))
> + continue;
> +
> + ret = osf_iio_register_sensor(osf->dev, &cache->entries[i],
> + osf, &indio_dev);
> + if (ret)
> + goto err_unregister;
> +
> + osf->iio_devs[osf->iio_dev_count].sensor_type =
> + cache->entries[i].sensor_type;
> + osf->iio_devs[osf->iio_dev_count].sensor_index =
> + cache->entries[i].sensor_index;
> + osf->iio_devs[osf->iio_dev_count].indio_dev = indio_dev;
Another place a designated intializer makes sense I think. See below for
other examples.
> + osf->iio_dev_count++;
> + }
> +
> + return 0;
> +
> +err_unregister:
> + osf_core_unregister_iio(osf);
> +
> + return ret;
> }
>
> -static int osf_core_validate_sensor_sample(const struct osf_frame *frame)
> +static int osf_core_handle_sensor_sample(struct osf_device *osf,
> + const struct osf_frame *frame)
> {
> + struct osf_latest_sample *latest;
> struct osf_sensor_sample sample;
> + struct iio_dev *indio_dev;
> + unsigned int i;
> + int ret;
>
> - return osf_protocol_decode_sensor_sample(frame, &sample);
> + ret = osf_protocol_decode_sensor_sample(frame, &sample);
> + if (ret)
> + return ret;
> +
> + if (sample.channel_count > OSF_MAX_SAMPLE_CHANNELS)
> + return -E2BIG;
> +
> + latest = osf_core_find_latest_sample(osf, sample.sensor_type,
> + sample.sensor_index);
> + if (!latest)
> + return -E2BIG;
> +
> + for (i = 0; i < sample.channel_count; i++) {
> + ret = osf_protocol_sensor_sample_value(&sample, i,
> + &latest->values[i]);
> + if (ret)
> + return ret;
> + }
> +
> + for (; i < OSF_MAX_SAMPLE_CHANNELS; i++)
> + latest->values[i] = 0;
> +
> + latest->sensor_type = sample.sensor_type;
> + latest->sensor_index = sample.sensor_index;
> + latest->channel_count = sample.channel_count;
> + latest->sample_format = sample.sample_format;
> + latest->scale_nano = sample.scale_nano;
> + latest->sequence = frame->sequence;
> + latest->timestamp_us = frame->timestamp_us;
> + latest->valid = true;
Maybe reorder so yo ucan use a designated initializer.
Technically it will zero the values before you then fill them, but the
code is a fair bit simpler.
> + osf->last_sequence = frame->sequence;
> +
> + indio_dev = osf_core_find_iio_dev(osf, sample.sensor_type,
> + sample.sensor_index);
> + if (indio_dev) {
I'd flip
if (!indio_dev)
return 0; /* Nothing to do */
> + ret = osf_iio_push_sample(indio_dev, latest->values,
> + latest->channel_count);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> }
>
> -static int osf_core_validate_device_status(const struct osf_frame *frame)
> +static int osf_core_handle_device_status(struct osf_device *osf,
> + const struct osf_frame *frame)
> {
> + struct osf_status_cache cache = { };
> struct osf_device_status status;
> int ret;
>
> @@ -40,12 +194,22 @@ static int osf_core_validate_device_status(const struct osf_frame *frame)
> if (status.reserved)
> return -EPROTO;
>
> + cache.uptime_s = status.uptime_s;
> + cache.status_flags = status.status_flags;
> + cache.error_flags = status.error_flags;
> + cache.dropped_frames = status.dropped_frames;
> + cache.sequence = frame->sequence;
> + cache.valid = true;
> + osf->status_cache = cache;
osf->status_cache = (struct osf_status_cache) {
.uptime_s = status.update_s;
};
> + osf->last_sequence = frame->sequence;
> +
> return 0;
> }
>
>
> @@ -81,27 +265,47 @@ int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len)
>
> switch (frame.message_type) {
> case OSF_MSG_SENSOR_SAMPLE:
> - ret = osf_core_validate_sensor_sample(&frame);
> - break;
> + return osf_core_handle_sensor_sample(osf, &frame);
Seems like these changes belong in the earlier patch? Can you refactor
that to avoid having to modify it here.
> case OSF_MSG_DEVICE_STATUS:
> - ret = osf_core_validate_device_status(&frame);
> - break;
> + return osf_core_handle_device_status(osf, &frame);
> case OSF_MSG_CAPABILITY_REPORT:
> - ret = osf_core_validate_capability_report(&frame);
> - break;
> + return osf_core_handle_capability_report(osf, &frame);
> default:
> if (frame.message_type >= OSF_RESERVED_MSG_FIRST &&
> frame.message_type <= OSF_RESERVED_MSG_LAST)
> - ret = 0;
> - else if (frame.message_type >= OSF_VENDOR_PRIVATE_FIRST)
> - ret = 0;
> - else
> - ret = -EOPNOTSUPP;
> - break;
> + return 0;
> + if (frame.message_type >= OSF_VENDOR_PRIVATE_FIRST)
> + return 0;
> + return -EOPNOTSUPP;
> }
> +}
>
> - if (!ret)
> - osf->last_sequence = frame.sequence;
> +int osf_core_read_latest_sample(struct osf_device *osf, u16 sensor_type,
> + u16 sensor_index, unsigned int channel,
> + s32 *value)
> +{
> + const struct osf_latest_sample *latest;
> + unsigned int i;
>
> - return ret;
> + if (!osf || !value)
> + return -EINVAL;
> +
> + for (i = 0; i < osf->latest_sample_count; i++) {
> + latest = &osf->latest_samples[i];
> + if (latest->sensor_type == sensor_type &&
> + latest->sensor_index == sensor_index)
> + goto found;
I'd put the found block here rather than a goto. That simpler
flow is worth the cost of indent. You can also flip the condition
giving
for(unsigned int i = 0; i < osf->latest_sample_count; i++) {
latest = &osf->latest_samples[i];
if (latest->sensor_type != sensor_type ||
latest->sensor_index != sensor_index)
continue;
if (!latest->valid)
return -ENODATA;
if (channel >= latest->channel_count)
return -ENODATA;
*value = latest->values[channel];
return 0;
}
return -ENODATA;
> + }
> +
> + return -ENODATA;
> +
> +found:
> + if (!latest->valid)
> + return -ENODATA;
> + if (channel >= latest->channel_count)
> + return -ENODATA;
> +
> + *value = latest->values[channel];
> +
> + return 0;
> }
> diff --git a/drivers/iio/opensensorfusion/osf_core.h b/drivers/iio/opensensorfusion/osf_core.h
> index 3680c8c9b..e5c0d5081 100644
> --- a/drivers/iio/opensensorfusion/osf_core.h
> +++ b/drivers/iio/opensensorfusion/osf_core.h
> @@ -4,15 +4,64 @@
>
> #include <linux/types.h>
>
> +#include "osf_protocol.h"
> +
> +#define OSF_MAX_SAMPLE_CHANNELS 3
> +#define OSF_MAX_CAPABILITIES 16
> +
> struct device;
> +struct iio_dev;
> +
> +struct osf_latest_sample {
> + u16 sensor_type;
> + u16 sensor_index;
> + u16 channel_count;
> + u16 sample_format;
> + u32 scale_nano;
> + s32 values[OSF_MAX_SAMPLE_CHANNELS];
> + u64 sequence;
> + u64 timestamp_us;
Some of each of these comes from an existing structure reflecting what we
read from the device, can we use that structure embedded her instead of having
to copy field by field?
> + bool valid;
> +};
> +
> +struct osf_capability_cache {
> + u16 capability_count;
> + struct osf_capability_entry entries[OSF_MAX_CAPABILITIES];
> + u64 sequence;
> + bool valid;
> +};
> +
> +struct osf_status_cache {
> + u32 uptime_s;
> + u32 status_flags;
> + u32 error_flags;
> + u32 dropped_frames;
> + u64 sequence;
> + bool valid;
> +};
> diff --git a/drivers/iio/opensensorfusion/osf_iio.c b/drivers/iio/opensensorfusion/osf_iio.c
> new file mode 100644
> index 000000000..1f82ec063
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_iio.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bitops.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/kernel.h>
Neither device.h nor kernel.h should be included in a driver, unless you
are using one of the few definitions that need them. Typically there are more
focused include like dev_printk.h which should be used instead.
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#define OSF_SCAN_TYPE_S32 \
> + { \
> + .sign = 's', \
> + .realbits = 32, \
> + .storagebits = 32, \
> + .endianness = IIO_CPU, \
> + }
> +
> +#define OSF_MOD_CHAN(_type, _mod, _idx) \
> + { \
> + .type = (_type), \
> + .modified = 1, \
> + .channel2 = (_mod), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_index = (_idx), \
> + .scan_type = OSF_SCAN_TYPE_S32, \
> + }
> +
> +#define OSF_CHAN(_type, _idx) \
> + { \
> + .type = (_type), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_index = (_idx), \
> + .scan_type = OSF_SCAN_TYPE_S32, \
> + }
Only seems likely to be used once. So why have an macro?
Maybe it is useful just to put it next to the others for visual comparison.
> +
> +static const struct iio_chan_spec osf_temp_channels[] = {
> + OSF_CHAN(IIO_TEMP, 0),
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static const unsigned long osf_3axis_available_scan_masks[] = {
> + GENMASK(2, 0),
> + 0,
As below.
> +};
> +
> +static const unsigned long osf_temp_available_scan_masks[] = {
> + BIT(0),
> + 0,
0 is a terminating entry so no trailing comma.
> +};
> +
> +static int osf_iio_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int *val,
> + int *val2, long mask)
> +{
> + struct osf_iio_state *state = iio_priv(indio_dev);
> + s32 raw;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (!state->osf)
> + return -ENODATA;
> +
> + if (chan->scan_index < 0)
> + return -EINVAL;
If this isn't needed to silence a compiler or similar warning: we control
that value in the driver so should know if this can happen or not.
So probably not needed.
> +
> + ret = osf_core_read_latest_sample(state->osf,
> + state->spec->sensor_type,
> + state->sensor_index,
> + chan->scan_index, &raw);
> + if (ret)
> + return ret;
> +
> + *val = raw;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = state->scale_nano / NANO;
> + *val2 = state->scale_nano % NANO;
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +void osf_iio_unregister_sensor(struct iio_dev *indio_dev)
> +{
> + struct osf_iio_state *state;
> +
> + if (!indio_dev)
If you are unregistering something that is null something went wrong
probably. Seems unlikely we need this defence.
> + return;
> +
> + state = iio_priv(indio_dev);
Then this can be done at declaration above.
> + iio_device_unregister(indio_dev);
> + iio_kfifo_free(state->buffer);
> + iio_device_free(indio_dev);
> +}
> +
> +int osf_iio_push_sample(struct iio_dev *indio_dev, const s32 *values,
> + unsigned int channel_count)
> +{
> + s64 timestamp;
> +
> + if (!iio_buffer_enabled(indio_dev))
Add a comment on what the race is (I assume) that gets us here with that not set.
> + return 0;
> +
> + timestamp = iio_get_time_ns(indio_dev);
> +
> + return iio_push_to_buffers_with_ts_unaligned(indio_dev, values,
> + channel_count * sizeof(*values),
> + timestamp);
> +}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities
2026-05-28 14:30 ` Jonathan Cameron
@ 2026-05-29 12:49 ` Kim Jinseob
0 siblings, 0 replies; 30+ messages in thread
From: Kim Jinseob @ 2026-05-29 12:49 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Thu, May 28, 2026 at 11:30:00PM +0100, Jonathan Cameron wrote:
> Various things inline.
Addressed in RFC v3.
> Why?
Addressed in the v3 patch split. The core/protocol/IIO changes were
reworked so the includes match the code introduced in each patch
> This sounds like it makes a duplicate. Rename to _isduplicate() or something like that.
Addressed in RFC v3.
> Another place a designated intializer makes sense I think. See below for
> other examples.
Addressed in RFC v3 where it made the code clearer.
> Maybe reorder so yo ucan use a designated initializer.
> Technically it will zero the values before you then fill them, but the
> code is a fair bit simpler.
Addressed in RFC v3 where it made the code clearer.
> I'd flip
> if (!indio_dev)
> return 0; /* Nothing to do */
Addressed in RFC v3.
> Seems like these changes belong in the earlier patch? Can you refactor
> that to avoid having to modify it here.
Addressed in the v3 patch split.
> I'd put the found block here rather than a goto. That simpler
> flow is worth the cost of indent. You can also flip the condition
> giving
Addressed in RFC v3.
> Some of each of these comes from an existing structure reflecting what we
> read from the device, can we use that structure embedded her instead of having
> to copy field by field?
I reviewed this while preparing RFC v3. I kept the cached runtime state
separate from the wire-format decode structures, but reduced the field-by-field
copying where the decoded data did not need to be retained.
> Neither device.h nor kernel.h should be included in a driver, unless you
> are using one of the few definitions that need them. Typically there are more
> focused include like dev_printk.h which should be used instead.
Addressed in RFC v3. I removed broad includes that were not needed.
linux/device.h
remains only where device APIs are used.
> Only seems likely to be used once. So why have an macro?
> Maybe it is useful just to put it next to the others for visual comparison.
Reviewed while preparing RFC v3. I kept the channel helper macros only where
they improved consistency between the channel definitions.
> 0 is a terminating entry so no trailing comma.
Addressed in RFC v3.
> If this isn't needed to silence a compiler or similar warning: we control
> that value in the driver so should know if this can happen or not.
> So probably not needed.
Addressed in RFC v3. I removed unnecessary defensive checks for values the
driver controls.
> If you are unregistering something that is null something went wrong
> probably. Seems unlikely we need this defence.
Addressed in RFC v3.
> Then this can be done at declaration above.
Addressed in RFC v3.
> Add a comment on what the race is (I assume) that gets us here with that not set.
Addressed in RFC v3. I added a short comment around the iio_buffer_enabled()
case.
I also addressed the Sashiko feedback for this patch in RFC v3 by fixing the
IIO_BUFFER / IIO_KFIFO_BUF dependency handling, rejecting channel_count
mismatches before pushing samples to IIO buffers, and adding locking around
cached latest samples.
Thanks,
Jinseob
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
` (5 preceding siblings ...)
2026-05-24 8:53 ` [PATCH RFC v2 6/7] iio: osf: register IIO devices from capabilities Jinseob Kim
@ 2026-05-24 8:53 ` Jinseob Kim
2026-05-25 12:11 ` Krzysztof Kozlowski
2026-05-28 14:31 ` Jonathan Cameron
2026-06-02 23:59 ` [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Andy Shevchenko
7 siblings, 2 replies; 30+ messages in thread
From: Jinseob Kim @ 2026-05-24 8:53 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel,
Jinseob Kim
Add file patterns for the driver, binding, and protocol reference.
Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index c2c6d7927..9238701f4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19977,6 +19977,15 @@ F: Documentation/networking/oa-tc6-framework.rst
F: drivers/net/ethernet/oa_tc6.c
F: include/linux/oa_tc6.h
+OPEN SENSOR FUSION IIO DRIVER
+M: Jinseob Kim <kimjinseob88@gmail.com>
+L: linux-iio@vger.kernel.org
+L: devicetree@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
+F: Documentation/iio/open-sensor-fusion-protocol-v0.rst
+F: drivers/iio/opensensorfusion/
+
OPEN FIRMWARE AND FLATTENED DEVICE TREE
M: Rob Herring <robh@kernel.org>
M: Saravana Kannan <saravanak@kernel.org>
--
2.43.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver
2026-05-24 8:53 ` [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver Jinseob Kim
@ 2026-05-25 12:11 ` Krzysztof Kozlowski
2026-05-28 14:31 ` Jonathan Cameron
1 sibling, 0 replies; 30+ messages in thread
From: Krzysztof Kozlowski @ 2026-05-25 12:11 UTC (permalink / raw)
To: Jinseob Kim, Jonathan Cameron, linux-iio
Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel
On 24/05/2026 10:53, Jinseob Kim wrote:
> Add file patterns for the driver, binding, and protocol reference.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
> ---
> MAINTAINERS | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c2c6d7927..9238701f4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19977,6 +19977,15 @@ F: Documentation/networking/oa-tc6-framework.rst
> F: drivers/net/ethernet/oa_tc6.c
> F: include/linux/oa_tc6.h
>
> +OPEN SENSOR FUSION IIO DRIVER
> +M: Jinseob Kim <kimjinseob88@gmail.com>
> +L: linux-iio@vger.kernel.org
> +L: devicetree@vger.kernel.org
Drop both lists.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver
2026-05-24 8:53 ` [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver Jinseob Kim
2026-05-25 12:11 ` Krzysztof Kozlowski
@ 2026-05-28 14:31 ` Jonathan Cameron
2026-05-29 12:51 ` Kim Jinseob
1 sibling, 1 reply; 30+ messages in thread
From: Jonathan Cameron @ 2026-05-28 14:31 UTC (permalink / raw)
To: Jinseob Kim
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Sun, 24 May 2026 17:53:12 +0900
Jinseob Kim <kimjinseob88@gmail.com> wrote:
> Add file patterns for the driver, binding, and protocol reference.
>
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
> ---
> MAINTAINERS | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c2c6d7927..9238701f4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19977,6 +19977,15 @@ F: Documentation/networking/oa-tc6-framework.rst
> F: drivers/net/ethernet/oa_tc6.c
> F: include/linux/oa_tc6.h
>
> +OPEN SENSOR FUSION IIO DRIVER
> +M: Jinseob Kim <kimjinseob88@gmail.com>
> +L: linux-iio@vger.kernel.org
> +L: devicetree@vger.kernel.org
As noted, no need for lists. The appropriate catch all listings in
maintainers cover IIO and DT-bindings.
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/imu/opensensorfusion,osf-uart.yaml
Bring this in with the first file, then update as you add additional files.
> +F: Documentation/iio/open-sensor-fusion-protocol-v0.rst
> +F: drivers/iio/opensensorfusion/
> +
> OPEN FIRMWARE AND FLATTENED DEVICE TREE
> M: Rob Herring <robh@kernel.org>
> M: Saravana Kannan <saravanak@kernel.org>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver
2026-05-28 14:31 ` Jonathan Cameron
@ 2026-05-29 12:51 ` Kim Jinseob
2026-05-29 13:07 ` Jonathan Cameron
0 siblings, 1 reply; 30+ messages in thread
From: Kim Jinseob @ 2026-05-29 12:51 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Thu, May 28, 2026 at 11:31:00PM +0100, Jonathan Cameron wrote:
> As noted, no need for lists. The appropriate catch all listings in
> maintainers cover IIO and DT-bindings.
Addressed in RFC v3.
I dropped the explicit linux-iio and devicetree list entries from the new
MAINTAINERS section.
> Bring this in with the first file, then update as you add additional files.
Addressed in RFC v3.
I removed the standalone MAINTAINERS patch and folded the MAINTAINERS updates
into the patches that add the corresponding files.
Thanks
Jinseob
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver
2026-05-29 12:51 ` Kim Jinseob
@ 2026-05-29 13:07 ` Jonathan Cameron
0 siblings, 0 replies; 30+ messages in thread
From: Jonathan Cameron @ 2026-05-29 13:07 UTC (permalink / raw)
To: Kim Jinseob
Cc: linux-iio, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
linux-kernel
On Fri, 29 May 2026 21:51:06 +0900
Kim Jinseob <kimjinseob88@gmail.com> wrote:
> On Thu, May 28, 2026 at 11:31:00PM +0100, Jonathan Cameron wrote:
>
> > As noted, no need for lists. The appropriate catch all listings in
> > maintainers cover IIO and DT-bindings.
>
> Addressed in RFC v3.
>
> I dropped the explicit linux-iio and devicetree list entries from the new
> MAINTAINERS section.
>
> > Bring this in with the first file, then update as you add additional files.
>
> Addressed in RFC v3.
>
> I removed the standalone MAINTAINERS patch and folded the MAINTAINERS updates
> into the patches that add the corresponding files.
>
> Thanks
>
> Jinseob
Hi.
A few kernel process things.
1) Don't reply just to say you have address comments - if you are saying that
for some things in an email, then crop the rest of the quoted text.
Keep thanks, and how you change it for the next version of the patch, mostly
in the change log.
2) Slow down a bit - I'd leave at least a week on the list between early versions
of a patch set of this complexity. You might have received some more review
before writing v3 or discussion might be continued on any feedback you commented
on (I didn't spot any but maybe lack of cropping meant I missed it?)
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver
2026-05-24 8:53 [PATCH RFC v2 0/7] iio: add Open Sensor Fusion UART driver Jinseob Kim
` (6 preceding siblings ...)
2026-05-24 8:53 ` [PATCH RFC v2 7/7] MAINTAINERS: add Open Sensor Fusion IIO driver Jinseob Kim
@ 2026-06-02 23:59 ` Andy Shevchenko
7 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2026-06-02 23:59 UTC (permalink / raw)
To: Jinseob Kim
Cc: Jonathan Cameron, linux-iio, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
devicetree, linux-kernel
On Sun, May 24, 2026 at 05:53:05PM +0900, Jinseob Kim wrote:
> This series adds an IIO driver for a UART-attached sensor aggregation
> device. The device sends OSF0 binary frames over serdev UART. The driver
> uses capability reports to register IIO devices for supported sensors.
>
> v2 preparation changes:
>
> - split the previous RFC driver patch into smaller patches
> - add a Device Tree binding
> - add an OSF0 protocol reference
> - use get_unaligned_le16/32/64() for wire fields
> - use IIO timestamp handling for buffered samples
> - push decoded samples directly to registered IIO devices
>
> Tested path:
>
> - STM32F405 OSF0 UART stream
> - Raspberry Pi 4 serdev
> - kernel 6.12.75+rpt-rpi-v8
> - IIO devices: osf-accel, osf-gyro, osf-magn, osf-temp
> - raw reads from accel, gyro, magn, and temp
> - buffer reads from accel, gyro, magn, and temp
>
> No OSF oops, panic, or call trace was observed in the Raspberry Pi
> runtime smoke.
My comments from the previous version seem still valid.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread