Re: [PATCH RFC v5 5/6] iio: osf: add UART transport

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Jun 16, 2026 at 04:22:41PM +0900, Jinseob Kim wrote:
> Add the serdev UART transport and the initial OSF core receive path.
> 
> Enable the required vcc regulator with devm_regulator_get_enable()
> before opening the UART, keeping power handling limited to the simple
> probe-time requirement for this RFC.

...

> +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 OSF protocol 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.

What is this paragraph supposed to mean?

...

> +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++) {

	for (unsigned int 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;

How can this be called with osf == NULL?

> +	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;

You may invert this and return directly

		if ((frame.message_type < OSF_VENDOR_PRIVATE_FIRST) &&
		    (frame.message_type < OSF_RESERVED_MSG_FIRST ||
		     frame.message_type > OSF_RESERVED_MSG_LAST))
			return -EOPNOTSUPP;

> +	}

> +	if (!ret)
> +		osf->last_sequence = frame.sequence;
> +
> +	return ret;

No. Use regular pattern

	if (ret)
		return ret;
	...
	return 0;

> +}

...

> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

What is this for?

> +#include <linux/regulator/consumer.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,

Why casting?

> +				    ret);
> +
> +	return count;
> +}

...

> +static int osf_serdev_probe(struct serdev_device *serdev)
> +{

	struct device *dev = &serdev->dev;

makes the below look better.

> +	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 = devm_regulator_get_enable(&serdev->dev, "vcc");
> +	if (ret)
> +		return dev_err_probe(&serdev->dev, ret,
> +				     "failed to enable vcc regulator\n");
> +
> +	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 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,
> +	},
> +};

> +

No blank line needed here.

> +module_serdev_device_driver(osf_serdev_driver);

-- 
With Best Regards,
Andy Shevchenko