Re: [PATCH RFC v5 3/6] iio: osf: add protocol decoding

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 16 Jun 2026 16:22:39 +0900
Jinseob Kim <kimjinseob88@gmail.com> wrote:

> Add helpers for validating and decoding Open Sensor Fusion frames and the
> message payloads used by the initial receive path.
> 
> Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>
A few things inline.

> ---
>  drivers/iio/opensensorfusion/osf_protocol.c | 249 ++++++++++++++++++++
>  drivers/iio/opensensorfusion/osf_protocol.h |  97 ++++++++
>  2 files changed, 346 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..5bee545f3
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_protocol.c

> +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 (get_unaligned_le32(buf) != OSF_FRAME_MAGIC)
> +		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;
> +
> +	if (get_unaligned_le32(buf + 34))
> +		return -EPROTO;
> +
> +	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->payload = buf + OSF_FRAME_HEADER_LEN;
> +	frame->crc = actual_crc;

Same as below wrt to to using designated initializers for these
structure fills

> +	*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;
> +	u16 sensor_type;
> +	size_t 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;
> +	sensor_type = get_unaligned_le16(payload);
> +	channel_count = get_unaligned_le16(payload + 4);
> +	sample_format = get_unaligned_le16(payload + 6);
> +
> +	if (!osf_sensor_type_valid(sensor_type))
> +		return -EPROTO;
> +
> +	if (!channel_count)
> +		return -EPROTO;
> +
> +	if (sample_format != OSF_SAMPLE_FORMAT_S32)
> +		return -EPROTO;
> +
> +	if (get_unaligned_le32(payload + 12))
> +		return -EPROTO;
> +
> +	if (channel_count > (SIZE_MAX - OSF_SENSOR_SAMPLE_BASE_LEN) / sizeof(s32))
> +		return -EOVERFLOW;
> +
> +	expected_len = OSF_SENSOR_SAMPLE_BASE_LEN + channel_count * sizeof(s32);
> +	if (frame->payload_len != expected_len)
> +		return -EMSGSIZE;
> +
> +	sample->sensor_type = sensor_type;
> +	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->samples = payload + OSF_SENSOR_SAMPLE_BASE_LEN;
See below. Designated initializer would help readability a little here.

> +
> +	return 0;
> +}
> +
> +int osf_protocol_sensor_sample_value(const struct osf_sensor_sample *sample,
> +				     unsigned int index, s32 *value)

Given channel count is a u16 and we can't be equal or bigger than it, perhaps
use a u16 for index as well.

> +{
> +	if (!sample || !sample->samples || !value)
> +		return -EINVAL;
> +
> +	if (index >= sample->channel_count)
> +		return -ERANGE;
> +
> +	/* Samples are little-endian two's-complement signed values. */
> +	*value = (s32)get_unaligned_le32(sample->samples + index * sizeof(s32));

sizeof(__le32) slightly more appropriate given that is what you are treating it
as.

> +
> +	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;
> +	if (get_unaligned_le32(payload + 16))
> +		return -EPROTO;
> +
> +	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);
Similar to below. I'd use a designated initializer for status as it
is all written in one place.

> +
> +	return 0;
> +}

> +
> +int osf_protocol_decode_capability_entry(const struct osf_capability_report *report,
> +					 unsigned int index,
> +					 struct osf_capability_entry *entry)
> +{
> +	u16 sample_format;
> +	u16 sensor_type;
> +	u32 flags;
> +	const u8 *payload;
> +
> +	if (!report || !report->entries || !entry)
> +		return -EINVAL;
> +
> +	if (index >= report->capability_count)

Neater to size index to match capability_count.  Not that
important though.

> +		return -ERANGE;
> +
> +	payload = report->entries + index * OSF_CAP_SENSOR_ENTRY_LEN;
> +	sensor_type = get_unaligned_le16(payload);
> +	sample_format = get_unaligned_le16(payload + 6);
> +	flags = get_unaligned_le32(payload + 12);
> +
> +	if (!osf_sensor_type_valid(sensor_type))
> +		return -EPROTO;
> +
> +	if (sample_format != OSF_SAMPLE_FORMAT_S32)
> +		return -EPROTO;
> +
> +	if (flags & ~OSF_CAPABILITY_FLAGS_MASK)
> +		return -EPROTO;
> +
> +	if (get_unaligned_le32(payload + 16))
> +		return -EPROTO;
> +
> +	entry->sensor_type = sensor_type;
> +	entry->sensor_index = get_unaligned_le16(payload + 2);
> +	entry->channel_count = get_unaligned_le16(payload + 4);
> +	entry->sample_format = sample_format;
> +	entry->scale_nano = get_unaligned_le32(payload + 8);
> +	entry->flags = flags;
neater as designated initializer I think.

	*entry = (struct osf_capability_entry) {
		.sensor_type = sensor_type,
		.sensor_index = get_unaligned_le16(payload + 2),
etc
	};
> +
> +	return 0;
> +}