Message-ID: <20250713164033.3488db3c@jic23-huawei>
Date: Sun, 13 Jul 2025 16:40:33 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Yassine Oudjana via B4 Relay
 <devnull+y.oudjana.protonmail.com@...nel.org>
Cc: y.oudjana@...tonmail.com, Manivannan Sadhasivam <mani@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Bjorn Andersson
 <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Masahiro
 Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>,
 Nicolas Schier <nicolas.schier@...ux.dev>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, Luca Weiss <luca@...aweiss.eu>,
 linux-arm-msm@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
 linux-iio@...r.kernel.org
Subject: Re: [PATCH v2 4/4] iio: Add Qualcomm Sensor Manager driver

On Thu, 10 Jul 2025 09:06:30 +0100
Yassine Oudjana via B4 Relay <devnull+y.oudjana.protonmail.com@...nel.org> wrote:

> From: Yassine Oudjana <y.oudjana@...tonmail.com>
> 
> Add a driver for sensors exposed by the Qualcomm Sensor Manager service,
> which is provided by SLPI or ADSP on Qualcomm SoCs. Supported sensors
> include accelerometers, gyroscopes, pressure sensors, proximity sensors
> and magnetometers.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@...tonmail.com>

As Andy commented - this is big.  Break it up for v3.

So far I haven't understood why a separate accelerometer driver was necessary.
Some comments in the patch description would perhaps help me understand that.

> diff --git a/drivers/iio/accel/qcom_smgr_accel.c b/drivers/iio/accel/qcom_smgr_accel.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ce854312d1d9386300785f1965d5886c16995806
> --- /dev/null
> +++ b/drivers/iio/accel/qcom_smgr_accel.c
> @@ -0,0 +1,138 @@


> +static void qcom_smgr_accel_remove(struct platform_device *pdev)
> +{
> +	struct qcom_smgr_sensor *sensor = platform_get_drvdata(pdev);
> +
> +	sensor->iio_dev = NULL;

Add a comment for why this is needed. I can't immediately spot anything
explicitly checking it so it doesn't seem to be about safe handling
of device removal or similar.


> +}
> +
> +static const struct platform_device_id qcom_smgr_accel_ids[] = {
> +	{ .name = "qcom-smgr-accel" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, qcom_smgr_accel_ids);
> +
> +static struct platform_driver qcom_smgr_accel_driver = {
> +	.probe = qcom_smgr_accel_probe,
> +	.remove = qcom_smgr_accel_remove,
> +	.driver	= {
> +		.name = "qcom_smgr_accel",
> +	},
> +	.id_table = qcom_smgr_accel_ids,
> +};
> +module_platform_driver(qcom_smgr_accel_driver);
> +
> +MODULE_AUTHOR("Yassine Oudjana <y.oudjana@...tonmail.com>");
> +MODULE_DESCRIPTION("Qualcomm Sensor Manager accelerometer driver");
> +MODULE_LICENSE("GPL");

I'm struggling to understand what the relationship between this driver
the main sensor driver is.

> diff --git a/drivers/iio/common/qcom_smgr/qcom_smgr.c b/drivers/iio/common/qcom_smgr/qcom_smgr.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..79d1160f7a5c32f1a9e0a20f29e304e5cb18be8f
> --- /dev/null
> +++ b/drivers/iio/common/qcom_smgr/qcom_smgr.c
> @@ -0,0 +1,840 @@

> +static void qcom_smgr_buffering_report_handler(struct qmi_handle *hdl,
> +					       struct sockaddr_qrtr *sq,
> +					       struct qmi_txn *txn,
> +					       const void *data)
> +{
> +	struct qcom_smgr *smgr =
> +		container_of(hdl, struct qcom_smgr, sns_smgr_hdl);
> +	const struct sns_smgr_buffering_report_ind *ind = data;
> +	struct qcom_smgr_sensor *sensor;
> +	struct qcom_smgr_iio_data iio_data;
> +	int temp;
> +	u8 i, j;
> +
> +	for (i = 0; i < smgr->sensor_count; ++i) {
> +		sensor = &smgr->sensors[i];
> +
> +		/* Find sensor matching report */
> +		if (sensor->id == ind->report_id)
> +			break;
> +	}
> +
> +	if (i == smgr->sensor_count) {
> +		dev_warn_ratelimited(smgr->dev,
> +			"Received buffering report with unknown ID: %02x",
> +			ind->report_id);
> +		return;
> +	}
> +
> +	/*
> +	 * Construct data to be passed to IIO. Since we are matching report rate
> +	 * with sample rate, we only get a single sample in every report.
> +	 */
> +	for (j = 0; j < ARRAY_SIZE(ind->samples[0].values); ++j)
> +		iio_data.values[j] = ind->samples[0].values[j];
> +
> +	/*
> +	 * SMGR reports sensor data in 32-bit fixed-point values, with 16 bits
> +	 * holding the integer part and the other 16 bits holding the numerator
> +	 * of a fraction with the denominator 2**16.
> +	 *
> +	 * Proximity sensor values are reported differently from other sensors.
> +	 * The value reported is a boolean (0 or 1, still in the same fixed-point
> +	 * format) where 1 means the sensor is activated, i.e. something is
> +	 * within its range. Use the reported range to pass an actual distance
> +	 * value to IIO. We pass the sensor range when nothing is within range
> +	 * (sensor maxed out) and 0 when something is within range (assume
> +	 * sensor is covered).
> +	 */
> +	if (sensor->type == SNS_SMGR_SENSOR_TYPE_PROX_LIGHT) {
> +		temp = le32_to_cpu(iio_data.values[0]);
> +		temp >>= SMGR_VALUE_DECIMAL_OFFSET;
> +		temp = ~temp & 1;
> +		temp *= sensor->data_types[0].range;
> +		iio_data.values[0] = cpu_to_le32(temp);
> +	}
> +
> +	iio_push_to_buffers(sensor->iio_dev, &iio_data);
You have a structure with space for timestamps but don't provide one which
is odd.  Either don't make space, or provide it.

> +}

> +
> +static int qcom_smgr_sensor_predisable(struct iio_dev *iio_dev)
> +{
> +	struct qcom_smgr *smgr = dev_get_drvdata(iio_dev->dev.parent);
> +	struct qcom_smgr_iio_priv *priv = iio_priv(iio_dev);
> +	struct qcom_smgr_sensor *sensor = priv->sensor;
> +
> +	dev_info(smgr->dev, "disable buffering %02x\n", sensor->id);

Too nosy. dev_dbg()

> +	return qcom_smgr_request_buffering(smgr, sensor, false);
> +}

> +static int qcom_smgr_iio_read_raw(struct iio_dev *iio_dev,
> +				  struct iio_chan_spec const *chan, int *val,
> +				  int *val2, long mask)
> +{
> +	struct qcom_smgr_iio_priv *priv = iio_priv(iio_dev);
> +
> +	switch (mask) {

No sysfs access at all to data is unusual but not completely unheard of.

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = priv->sensor->data_types[0].cur_sample_rate;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1;
> +		*val2 = 1 << SMGR_VALUE_DECIMAL_OFFSET;
> +		return IIO_VAL_FRACTIONAL;
> +	default:
> +		return -EINVAL;
> +	}

> +
> +static const struct iio_chan_spec qcom_smgr_pressure_iio_channels[] = {
> +	{
> +		.type = IIO_PRESSURE,
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 32,
> +			.storagebits = 32,
> +			.endianness = IIO_LE,
> +		},
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ)
> +	},
> +	{
> +		.type = IIO_TIMESTAMP,
> +		.channel = -1,
> +		.scan_index = 3,

Why 3?

> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 32,

If it's realbits 32 and no shift, why not store it in a 32 bit value?
I assume this is a hardware provided timestamp rather than typical software
filled in one?  Anyhow, I'm not immediately spotting it being used yet
so for now perhaps best to drop the channel descriptions.

> +			.storagebits = 64,
> +			.endianness = IIO_LE,
> +		},
> +	}
> +};

> +static int qcom_smgr_register_sensor(struct qcom_smgr *smgr,
> +				     struct qcom_smgr_sensor *sensor)
> +{
> +	struct iio_dev *iio_dev;
> +	struct qcom_smgr_iio_priv *priv;
> +	int ret;

> +	sensor->iio_dev = iio_dev;
> +
> +	ret = devm_iio_kfifo_buffer_setup(smgr->dev, iio_dev,
> +					  &qcom_smgr_buffer_ops);
> +	if (ret) {
> +		dev_err(smgr->dev, "Failed to setup buffer: %pe\n",

Use return dev_err_probe() for all errors that occur in code that only
runs at probe() time.

> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = devm_iio_device_register(smgr->dev, iio_dev);
> +	if (ret) {
> +		dev_err(smgr->dev, "Failed to register IIO device: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smgr_probe(struct qrtr_device *qdev)
> +{
> +	struct qcom_smgr *smgr;
> +	int i, j;
> +	int ret;
> +
> +	smgr = devm_kzalloc(&qdev->dev, sizeof(*smgr), GFP_KERNEL);
> +	if (!smgr)
> +		return -ENOMEM;
> +
> +	smgr->dev = &qdev->dev;
> +
> +	smgr->sns_smgr_info.sq_family = AF_QIPCRTR;
> +	smgr->sns_smgr_info.sq_node = qdev->node;
> +	smgr->sns_smgr_info.sq_port = qdev->port;
> +
> +	dev_set_drvdata(&qdev->dev, smgr);
This code is a bit random on whether it uses qdev->dev, or smgr->dev

I'd be tempted to just introce
	struct device *dev = &qdev->dev; and use that pretty much everywhere.

> +
> +	ret = qmi_handle_init(&smgr->sns_smgr_hdl,
> +			      SNS_SMGR_SINGLE_SENSOR_INFO_RESP_MAX_LEN, NULL,
> +			      qcom_smgr_msg_handlers);
> +	if (ret < 0)
> +		return dev_err_probe(smgr->dev, ret,
> +			"Failed to initialize sensor manager handle\n");
> +
> +	ret = devm_add_action_or_reset(smgr->dev,
> +				       (void(*)(void *))qmi_handle_release,

I'd much prefer a local wrapper to casting types of functions.

> +				       &smgr->sns_smgr_hdl);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_smgr_request_all_sensor_info(smgr, &smgr->sensors);
> +	if (ret < 0)
> +		return dev_err_probe(smgr->dev, ret,
> +			"Failed to get available sensors\n");
> +
> +	smgr->sensor_count = ret;
> +
> +	/* Get primary and secondary sensors from each sensor ID */
> +	for (i = 0; i < smgr->sensor_count; i++) {
> +		ret = qcom_smgr_request_single_sensor_info(smgr,
> +							   &smgr->sensors[i]);
> +		if (ret < 0)
> +			return dev_err_probe(smgr->dev, ret,
> +				"Failed to get sensors from ID 0x%02x\n",
> +				smgr->sensors[i].id);
> +
> +		for (j = 0; j < smgr->sensors[i].data_type_count; j++) {
> +			/* Default to maximum sample rate */
> +			smgr->sensors[i].data_types->cur_sample_rate =
> +				smgr->sensors[i].data_types->max_sample_rate;
> +
> +			dev_dbg(smgr->dev, "0x%02x,%d: %s %s\n",
> +				smgr->sensors[i].id, j,
> +				smgr->sensors[i].data_types[j].vendor,
> +				smgr->sensors[i].data_types[j].name);
> +		}
> +
> +		/* Skip if sensor type is not supported */
> +		if (smgr->sensors[i]->type == SNS_SMGR_SENSOR_TYPE_UNKNOWN ||
> +		    !qcom_smgr_sensor_type_iio_channels[smgr->sensors[i]->type])
> +			continue;
> +
> +		ret = qcom_smgr_register_sensor(smgr, &smgr->sensors[i]);
> +		if (ret)
> +			return dev_err_probe(smgr->dev, ret,
> +				"Failed to register sensor 0x%02x\n",
> +				smgr->sensors[i].id);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct qrtr_device_id qcom_smgr_qrtr_match[] = {
> +	{
> +		.service = SNS_SMGR_QMI_SVC_ID,
> +		/* Found on MSM8953 */
> +		.instance = QRTR_INSTANCE_CONST(0, 1)
> +	},
> +	{
> +		.service = SNS_SMGR_QMI_SVC_ID,
> +		/* Found on MSM8974 and MSM8226 */
> +		.instance = QRTR_INSTANCE_CONST(1, 0)
> +	},
> +	{
> +		.service = SNS_SMGR_QMI_SVC_ID,
> +		/* Found on MSM8996 and SDM660 */
> +		.instance = QRTR_INSTANCE_CONST(1, 50)
> +	},
> +	{ },

No comma on a terminating entry like this.

> +};
> +MODULE_DEVICE_TABLE(qrtr, qcom_smgr_qrtr_match);


> +const struct qmi_elem_info sns_smgr_buffering_report_ind_ei[] = {
> +	{
> +		.data_type = QMI_UNSIGNED_1_BYTE,
> +		.elem_len = 1,
> +		.elem_size = sizeof_field(struct sns_smgr_buffering_report_ind,
> +					  report_id),
> +		.array_type = NO_ARRAY,
> +		.tlv_type = 0x01,
> +		.offset = offsetof(struct sns_smgr_buffering_report_ind,
> +				   report_id),
> +	},
> +	{
> +		.data_type = QMI_STRUCT,
> +		.elem_len = 1,
> +		.elem_size = sizeof_field(struct sns_smgr_buffering_report_ind,
> +					  metadata),
> +		.array_type = NO_ARRAY,
> +		.tlv_type = 0x02,
> +		.offset = offsetof(struct sns_smgr_buffering_report_ind,
> +				   metadata),
> +		.ei_array = sns_smgr_buffering_report_metadata_ei,
> +	},
> +	{
> +		.data_type = QMI_DATA_LEN,
> +		.elem_len = 1,
> +		.elem_size = sizeof_field(struct sns_smgr_buffering_report_ind,
> +					  samples_len),
> +		.array_type = NO_ARRAY,
> +		.tlv_type = 0x03,
> +		.offset = offsetof(struct sns_smgr_buffering_report_ind,
> +				   samples_len),
> +	},
> +	{
> +		.data_type = QMI_STRUCT,
> +		.elem_len = SNS_SMGR_SAMPLES_MAX_LEN,
> +		.elem_size = sizeof(struct sns_smgr_buffering_report_sample),
> +		.array_type = VAR_LEN_ARRAY,
> +		.tlv_type = 0x03,
> +		.offset =
> +			offsetof(struct sns_smgr_buffering_report_ind, samples),

I'm fine with slightly over 80 chars to avoid readability issues like this.


> diff --git a/include/linux/iio/common/qcom_smgr.h b/include/linux/iio/common/qcom_smgr.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..fdd3de12bb0a48f1fb9e51cd0463c9a9b9ed500f
> --- /dev/null
> +++ b/include/linux/iio/common/qcom_smgr.h
> @@ -0,0 +1,80 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __QCOM_SMGR_H__
> +#define __QCOM_SMGR_H__
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +#include <linux/soc/qcom/qmi.h>
> +#include <linux/types.h>

> +
> +struct qcom_smgr_sensor {
> +	u8 id;
> +	enum qcom_smgr_sensor_type type;
> +
> +	u8 data_type_count;
> +	/*
> +	 * Only SNS_SMGR_DATA_TYPE_PRIMARY is used at the moment, but we store
> +	 * SNS_SMGR_DATA_TYPE_SECONDARY when available as well for future use.
> +	 */
> +	struct qcom_smgr_data_type_item *data_types;
> +
> +	struct iio_dev *iio_dev;
> +};
> +
> +struct qcom_smgr_iio_priv {
> +	struct qcom_smgr_sensor *sensor;
> +
> +	int samp_freq_vals[3];
> +};
> +
> +
> +struct qcom_smgr_iio_data {
> +	u32 values[3];
> +	u64 timestamp;

Timestamps in kernel tend to be s64. Also for IIO buffers aligned_s64 required.
Whilst this will probably never be used on an architecture that doesn't naturally
align 8 byte variables, we should still code for it.

Given this tends to be used locally maybe just define it where you need it.


> +};

> 

Powered by blists - more mailing lists