Re: [PATCH v7 5/5] iio: osf: add UART IIO driver

[Jonathan Cameron]

On Tue,  7 Jul 2026 10:45:25 +0900
Jinseob Kim <kimjinseob88@xxxxxxxxx> wrote:

> Add the Open Sensor Fusion serdev transport, driver core, and IIO
> registration path as one complete driver patch.
> 
> The driver enables the required vcc regulator, receives OSF frames over
> UART, registers IIO devices from capability reports, supports direct raw
> reads from the latest sample cache, and pushes buffered samples into
> software kfifo buffers.
> 
> Use final Kconfig and Makefile contents from the start, claim IIO buffer
> mode while pushing samples, and use zeroed scan storage with explicit
> timestamp alignment so the driver does not depend on IIO core
> bounce-buffer padding behavior.
> 
> Signed-off-by: Jinseob Kim <kimjinseob88@xxxxxxxxx>

A few things inline.

> diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
> new file mode 100644
> index 000000000..02d55a201
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_core.c

> +
> +int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len)

This is only defined in this patch but is used in patch 4.  Make sure your
code builds after each patch so as to avoid breaking code bisection.

> +{
> +	struct osf_frame frame;
> +	size_t frame_len;
> +	int ret;
> +
> +	ret = osf_protocol_decode_frame(buf, len, &frame, &frame_len);
> +	if (ret)
> +		return ret;
> +
> +	if (frame_len != len)
...


> diff --git a/drivers/iio/opensensorfusion/osf_iio.c b/drivers/iio/opensensorfusion/osf_iio.c
> new file mode 100644
> index 000000000..91afcf3b8
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_iio.c

> +
> +#define OSF_SCAN_TYPE_S32						\
> +	{								\
> +		.sign = 's',						\
> +		.realbits = 32,					\
> +		.storagebits = 32,					\
> +		.endianness = IIO_CPU,					\
> +	}

This feels like going too far to deduplicate just this.  Put a copy inline
in each of the other macros where it is used and drop this one.

> +
> +#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,			\
> +	}


> +int osf_iio_push_sample(struct iio_dev *indio_dev, const s32 *values,
> +			u16 channel_count)
> +{
> +	struct osf_iio_state *state = iio_priv(indio_dev);
> +	s64 timestamp;
> +	int ret;
> +
> +	if (channel_count != state->spec->channel_count)
> +		return -EPROTO;
> +
> +	if (!iio_device_try_claim_buffer_mode(indio_dev))
> +		return 0;

Looking at this again, why do we care about holding the device
in buffered mode for this?  Races should be safe without that
big hammer. A simple check on iio_buffer_enabled() should ensure
data is only pushed when it is enabled, or just after it is disabled
(which should always be safe).

> +
> +	timestamp = iio_get_time_ns(indio_dev);
> +
> +	switch (channel_count) {
> +	case 1: {
> +		struct osf_iio_scan_1axis scan = { };
> +
> +		scan.value = values[0];
> +		ret = iio_push_to_buffers_with_ts(indio_dev, &scan,
> +						  sizeof(scan), timestamp);

With change above direct returns here

> +		break;
> +	}
> +	case 3: {
> +		struct osf_iio_scan_3axis scan = { };
> +
> +		scan.values[0] = values[0];
> +		scan.values[1] = values[1];
> +		scan.values[2] = values[2];
> +		ret = iio_push_to_buffers_with_ts(indio_dev, &scan,

here 

> +						  sizeof(scan), timestamp);
> +		break;
> +	}
> +	default:
> +		ret = -EPROTO;

and here should be fine as no need to release it.

> +		break;
> +	}
> +
> +	iio_device_release_buffer_mode(indio_dev);
> +
> +	return ret;
> +}