| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aWFVYefhhVn27vku@smile.fi.intel.com>
Date: Fri, 9 Jan 2026 21:22:09 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Francesco Lavra <flavra@...libre.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor
On Fri, Jan 09, 2026 at 07:15:28PM +0100, Francesco Lavra wrote:
> Some IMU chips in the LSM6DSX family have sensor fusion features that
> combine data from the accelerometer and gyroscope. One of these features
> generates rotation vector data and makes it available in the hardware
> FIFO as a quaternion (more specifically, the X, Y and Z components of the
> quaternion vector, expressed as 16-bit half-precision floating-point
> numbers).
>
> Add support for a new sensor instance that allows receiving sensor fusion
> data, by defining a new struct st_lsm6dsx_sf_settings (which contains
> chip-specific details for the sensor fusion functionality), and adding this
> struct as a new field in struct st_lsm6dsx_settings. In st_lsm6dsx_core.c,
> populate this new struct for the LSM6DSV and LSM6DSV16X chips, and add the
> logic to initialize an additional IIO device if this struct is populated
> for the hardware type being probed.
> Note: a new IIO device is being defined (as opposed to adding channels to
> an existing device) because each of the existing devices handles data
> coming from a single sensor, while sensor fusion data comes from multiple
> sensors.
>
> Tested on LSMDSV16X.
...
> enum st_lsm6dsx_sensor_id {
> ST_LSM6DSX_ID_EXT0,
> ST_LSM6DSX_ID_EXT1,
> ST_LSM6DSX_ID_EXT2,
> + ST_LSM6DSX_ID_SF,
> ST_LSM6DSX_ID_MAX,
At some point please either get rid of _ID_MAX, or drop the trailing comma
(maybe some other places also need the same treatment).
...
> +static int st_lsm6dsx_sf_set_page(struct st_lsm6dsx_hw *hw, bool enable)
> +{
> + const struct st_lsm6dsx_reg *mux;
> + int err;
> +
> + mux = &hw->settings->sf_settings.page_mux;
> + if (enable)
> + mutex_lock(&hw->page_lock);
> + err = regmap_assign_bits(hw->regmap, mux->addr, mux->mask, enable);
> + if (!enable || err < 0)
> + mutex_unlock(&hw->page_lock);
> +
> + return err;
> +}
Why not having properly made functions with the respective sparse annotations?
And drop this "enable" parameter.
...
> +int st_lsm6dsx_sf_set_enable(struct st_lsm6dsx_sensor *sensor, bool enable)
> +{
> + struct st_lsm6dsx_hw *hw = sensor->hw;
> + const struct st_lsm6dsx_reg *enable_reg;
> + int err;
> +
> + enable_reg = &hw->settings->sf_settings.enable;
> + err = st_lsm6dsx_sf_set_page(hw, true);
> + if (err < 0)
> + return err;
> +
> + err = regmap_assign_bits(hw->regmap, enable_reg->addr, enable_reg->mask,
> + enable);
One line? The variable name can be shortened as well.
> + st_lsm6dsx_sf_set_page(hw, false);
> +
> + return err;
> +}
...
> +static int st_lsm6dsx_sf_read_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *ch,
> + int *val, int *val2, long mask)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = sensor->hwfifo_odr_mHz / MILLI;
> + *val2 = (sensor->hwfifo_odr_mHz % MILLI) * MILLI;
Strictly speaking the multiplier in the second one should be "(MICRO / MILLI)".
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int st_lsm6dsx_sf_write_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> + const struct st_lsm6dsx_sf_settings *settings;
> + int err;
> +
> + settings = &sensor->hw->settings->sf_settings;
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ: {
> + u32 odr_mHz;
> + u8 odr_val;
> +
> + odr_mHz = val * MILLI + val2 / MILLI;
Ditto.
> + err = st_lsm6dsx_sf_get_odr_val(settings, odr_mHz, &odr_val);
> + if (err)
> + return err;
> +
> + sensor->hwfifo_odr_mHz = odr_mHz;
> + break;
> + }
> + default:
> + return -EINVAL;
> + }
> + return 0;
Perhaps move it to the only case that needs it?
> +}
...
> +static ssize_t st_lsm6dsx_sf_sampling_freq_avail(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct st_lsm6dsx_sensor *sensor = iio_priv(dev_to_iio_dev(dev));
> + const struct st_lsm6dsx_sf_settings *settings;
> + int i, len = 0;
> +
> + settings = &sensor->hw->settings->sf_settings;
> + for (i = 0; i < settings->odr_table.odr_len; i++) {
for (unsigned int i ...) {
> + u32 val = settings->odr_table.odr_avl[i].milli_hz;
> +
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%lu.%03lu ",
> + val / MILLI, val % MILLI);
Hmm... I think this has to be sysfs_emit_at().
> + }
> + buf[len - 1] = '\n';
> +
> + return len;
> +}
...
> +static struct attribute *st_lsm6dsx_sf_attributes[] = {
> + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> + NULL,
No comma here.
> +};
...
> +int st_lsm6dsx_sf_probe(struct st_lsm6dsx_hw *hw, const char *name)
> +{
> + const struct st_lsm6dsx_sf_settings *settings;
> + struct st_lsm6dsx_sensor *sensor;
> + struct iio_dev *iio_dev;
> +
> + iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
> + if (!iio_dev)
> + return -ENOMEM;
> +
> + settings = &hw->settings->sf_settings;
> + sensor = iio_priv(iio_dev);
> + sensor->id = ST_LSM6DSX_ID_SF;
> + sensor->hw = hw;
> + sensor->hwfifo_odr_mHz = settings->odr_table.odr_avl[0].milli_hz;
> + sensor->watermark = 1;
> + iio_dev->modes = INDIO_DIRECT_MODE;
> + iio_dev->info = &st_lsm6dsx_sf_info;
> + iio_dev->channels = settings->chan;
> + iio_dev->num_channels = settings->chan_len;
> + scnprintf(sensor->name, sizeof(sensor->name), "%s_sf", name);
What's the point of "c" here, please?
> + iio_dev->name = sensor->name;
> +
> + /**
Huh?!
> + * Put the IIO device pointer in the iio_devs array so that the caller
> + * can set up a buffer and register this IIO device.
> + */
> + hw->iio_devs[ST_LSM6DSX_ID_SF] = iio_dev;
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists