Message-ID: <nSoiRmruHeLNNxpRCxJ5M5aQ-Vx7lE3U9wtVwYh6MVZHr0pkk9Cwl5ggSN3xAZ09zA8bk_RJS6mRAgxWkCIrNGogaElh4x8VKaQPO_Rzrqs=@protonmail.com>
Date: Thu, 17 Jul 2025 14:31:17 +0000
From: Yassine Oudjana <y.oudjana@...tonmail.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Yassine Oudjana via B4 Relay <devnull+y.oudjana.protonmail.com@...nel.org>, 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 Sunday, July 13th, 2025 at 4:40 PM, Jonathan Cameron <jic23@...nel.org> wrote:

> 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.

Ok

> 
> 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.

That was not supposed to be here still. My bad.

> 
> > 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.

I forgot to copy the timestamp into iio_data. Will fix.

> 
> > +}
> 
> > +
> > +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()

I added this while debugging and missed it while cleaning up to remove it. Will remove
entirely.

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

There is no (known) method to request a single reading from the QMI
service. The only known way to get sensor data is to send a buffering
request to initiate sending data, then the remoteproc sends QMI
indications at a regular interval carrying sensor data which I am
pushing to the IIO buffers. The only way to implement direct sysfs
access would be to store the last received value somewhere then pass
it to sysfs when requested. This will also require enabling buffering
if disabled at the time of reading, then waiting until new data is
received. I didn't like this solution so I skipped direct sysfs access
altogether. Buffer access is enough for the current use case with
iio-sensor-proxy in userspace.

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

Because the same struct is used for this and 3-axis sensors, so we should
skip the unused values.

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

The hardware (or firmware rather) passes an unsigned 32-bit timestamp
value in a 64-bit QMI field. I was previously passing it as-is to IIO
but now since I introduced a new struct I can make it 32-bit storagebits.

But below you said s64 for timestamp so which is it going to be?

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

Ack

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

I think smgr->dev is good, as is used everywhere else in the driver.
Will change this one.

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

A function containing a single function call felt unnecessary to
me but I will add a wrapper if you prefer it that way.

> 
> > + &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.

Ok. Gotta keep track of all the conventions used in different subsystems.

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

Ok

> 
> > 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.

Ack

Powered by blists - more mailing lists