From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Francesco Lavra <flavra@baylibre.com>
Cc: "Jonathan Cameron" <jic23@kernel.org>,
"David Lechner" <dlechner@baylibre.com>,
"Nuno Sá" <nuno.sa@analog.com>,
"Andy Shevchenko" <andy@kernel.org>,
linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data
Date: Thu, 30 Oct 2025 17:42:24 +0100 [thread overview]
Message-ID: <aQOVcCinTd-ZJJX3@lore-desk> (raw)
In-Reply-To: <20251030072752.349633-2-flavra@baylibre.com>
[-- Attachment #1: Type: text/plain, Size: 11806 bytes --]
> Using the ST_LSM6DSX_CHANNEL_ACC() macro as a static initializer
> for the iio_chan_spec struct arrays makes all sensors advertise
> channel event capabilities regardless of whether they actually
> support event generation. And if userspace tries to configure
> accelerometer wakeup events on a sensor device that does not
> support them (e.g. LSM6DS0), st_lsm6dsx_write_event() dereferences
> a NULL pointer when trying to write to the wakeup register.
> Replace usage of the ST_LSM6DSX_CHANNEL_ACC() and
> ST_LSM6DSX_CHANNEL() macros with dynamic allocation and
> initialization of struct iio_chan_spec arrays, where the
> st_lsm6dsx_event structure is only used for sensors that support
> wakeup events; besides fixing the above bug, this serves as a
> preliminary step for adding support for more event types.
I agree we are missing the Fixes tag here.
>
> Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 26 +--
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 ++++++++-----------
> 2 files changed, 71 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index a4f558899767..db863bd1898d 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -80,27 +80,6 @@ enum st_lsm6dsx_hw_id {
> * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
> #define ST_LSM6DSX_SHIFT_VAL(val, mask) (((val) << __ffs(mask)) & (mask))
>
> -#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx) \
> -{ \
> - .type = chan_type, \
> - .address = addr, \
> - .modified = 1, \
> - .channel2 = mod, \
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> - .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> - .scan_index = scan_idx, \
> - .scan_type = { \
> - .sign = 's', \
> - .realbits = 16, \
> - .storagebits = 16, \
> - .endianness = IIO_LE, \
> - }, \
> - .event_spec = &st_lsm6dsx_event, \
> - .ext_info = st_lsm6dsx_ext_info, \
> - .num_event_specs = 1, \
> -}
> -
> #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx) \
> { \
> .type = chan_type, \
> @@ -328,10 +307,7 @@ struct st_lsm6dsx_settings {
> const char *name;
> u8 wai;
> } id[ST_LSM6DSX_MAX_ID];
> - struct {
> - const struct iio_chan_spec *chan;
> - int len;
> - } channels[2];
> + u8 chan_addr_base[2];
nit: chan_addr[2]
> struct {
> struct st_lsm6dsx_reg irq1;
> struct st_lsm6dsx_reg irq2;
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 216160549b5a..17b46e15cce5 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -96,26 +96,7 @@
>
> #define ST_LSM6DSX_TS_SENSITIVITY 25000UL /* 25us */
>
> -static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> - ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
> - ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2a, IIO_MOD_Y, 1),
> - ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2c, IIO_MOD_Z, 2),
> - IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> -
> -static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x22, IIO_MOD_X, 0),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x24, IIO_MOD_Y, 1),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x26, IIO_MOD_Z, 2),
> - IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> -
> -static const struct iio_chan_spec st_lsm6ds0_gyro_channels[] = {
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x18, IIO_MOD_X, 0),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1a, IIO_MOD_Y, 1),
> - ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1c, IIO_MOD_Z, 2),
> - IIO_CHAN_SOFT_TIMESTAMP(3),
> -};
> +#define ST_LSM6DSX_CHAN_COUNT 4
>
> static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> {
> @@ -142,15 +123,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x68,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6ds0_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6ds0_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x18,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -246,15 +221,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x69,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -412,15 +381,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x69,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -590,15 +553,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x6a,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -847,15 +804,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x6d,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .drdy_mask = {
> .addr = 0x13,
> @@ -1060,15 +1011,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x6b,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .drdy_mask = {
> .addr = 0x13,
> @@ -1237,15 +1182,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x70,
> },
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .drdy_mask = {
> .addr = 0x13,
> @@ -1443,15 +1382,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .wai = 0x22,
> }
> },
> - .channels = {
> - [ST_LSM6DSX_ID_ACC] = {
> - .chan = st_lsm6dsx_acc_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> - },
> - [ST_LSM6DSX_ID_GYRO] = {
> - .chan = st_lsm6dsx_gyro_channels,
> - .len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
> - },
> + .chan_addr_base = {
> + [ST_LSM6DSX_ID_ACC] = 0x28,
> + [ST_LSM6DSX_ID_GYRO] = 0x22,
> },
> .odr_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -2366,21 +2299,64 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> return st_lsm6dsx_init_hw_timer(hw);
> }
>
> +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6dsx_hw *hw,
> + enum st_lsm6dsx_sensor_id id, int index)
please try to respect the 79 column limit (I still like it :))
> +{
> + struct iio_chan_spec *chan = &channels[index];
> +
> + chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL : IIO_ANGL_VEL;
I think you should return an error here if id is not ST_LSM6DSX_ID_ACC or
ST_LSM6DSX_ID_GYRO.
> + chan->address = hw->settings->chan_addr_base[id] + index * ST_LSM6DSX_CHAN_SIZE;
> + chan->modified = 1;
> + chan->channel2 = IIO_MOD_X + index;
> + chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> + chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> + chan->scan_index = index;
> + chan->scan_type.sign = 's';
> + chan->scan_type.realbits = 16;
> + chan->scan_type.storagebits = 16;
> + chan->scan_type.endianness = IIO_LE;
what about reducing the scope of ST_LSM6DSX_CHANNEL_ACC/ST_LSM6DSX_CHANNEL here
to improve the iio_chan_spec struct initialization since most of the fields are
always the same between different sensors.
> + chan->ext_info = st_lsm6dsx_ext_info;
> + if (id == ST_LSM6DSX_ID_ACC) {
> + if (hw->settings->event_settings.wakeup_reg.addr) {
if (id == ST_LSM6DSX_ID_ACC &&
hw->settings->event_settings.wakeup_reg.addr) {
...
}
> + chan->event_spec = &st_lsm6dsx_event;
> + chan->num_event_specs = 1;
> + }
> + }
> + return 0;
> +}
> +
> static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> enum st_lsm6dsx_sensor_id id,
> const char *name)
> {
> struct st_lsm6dsx_sensor *sensor;
> struct iio_dev *iio_dev;
> + struct iio_chan_spec *channels;
nit: chan to be consistent
> + int i;
>
> iio_dev = devm_iio_device_alloc(hw->dev, sizeof(*sensor));
> if (!iio_dev)
> return NULL;
>
> + channels = devm_kzalloc(hw->dev, sizeof(*channels) * ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);
79 column limit here. I guess you can use even devm_kcalloc() here.
> + if (!channels)
> + return NULL;
> +
> + for (i = 0; i < 3; i++) {
> + if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> + return NULL;
> + }
new line here.
> + channels[3].type = IIO_TIMESTAMP;
> + channels[3].channel = -1;
> + channels[3].scan_index = 3;
> + channels[3].scan_type.sign = 's';
> + channels[3].scan_type.realbits = 64;
> + channels[3].scan_type.storagebits = 64;
> iio_dev->modes = INDIO_DIRECT_MODE;
> iio_dev->available_scan_masks = st_lsm6dsx_available_scan_masks;
> - iio_dev->channels = hw->settings->channels[id].chan;
> - iio_dev->num_channels = hw->settings->channels[id].len;
> + iio_dev->channels = channels;
> + iio_dev->num_channels = ST_LSM6DSX_CHAN_COUNT;
>
> sensor = iio_priv(iio_dev);
> sensor->id = id;
> --
> 2.39.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2025-10-30 16:42 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-30 7:27 [PATCH 0/9] st_lsm6dsx: add tap event detection Francesco Lavra
2025-10-30 7:27 ` [PATCH 1/9] iio: imu: st_lsm6dsx: dynamically initialize iio_chan_spec data Francesco Lavra
2025-10-30 7:57 ` Andy Shevchenko
2025-10-30 11:03 ` Francesco Lavra
2025-10-30 16:42 ` Lorenzo Bianconi [this message]
2025-10-31 8:04 ` Francesco Lavra
2025-10-31 8:09 ` Andy Shevchenko
2025-10-31 8:26 ` Francesco Lavra
2025-10-31 8:32 ` Andy Shevchenko
2025-10-31 11:43 ` Francesco Lavra
2025-11-02 11:16 ` Jonathan Cameron
2025-11-03 9:24 ` Francesco Lavra
2025-11-09 13:32 ` Jonathan Cameron
2025-10-30 7:27 ` [PATCH 2/9] iio: imu: st_lsm6dsx: make event_settings more generic Francesco Lavra
2025-10-30 16:44 ` Lorenzo Bianconi
2025-10-31 8:08 ` Francesco Lavra
2025-10-30 7:27 ` [PATCH 3/9] iio: imu: st_lsm6dsx: move wakeup event enable mask to event_src Francesco Lavra
2025-10-30 7:59 ` Andy Shevchenko
2025-10-30 7:27 ` [PATCH 4/9] iio: imu: st_lsm6dsx: dynamically allocate iio_event_spec structs Francesco Lavra
2025-11-02 11:22 ` Jonathan Cameron
2025-10-30 7:27 ` [PATCH 5/9] iio: imu: st_lsm6dsx: rework code to check for enabled events Francesco Lavra
2025-10-30 7:27 ` [PATCH 6/9] iio: imu: st_lsm6dsx: remove event_threshold field from hw struct Francesco Lavra
2025-10-30 8:01 ` Andy Shevchenko
2025-10-30 11:10 ` Francesco Lavra
2025-10-30 13:49 ` Andy Shevchenko
2025-11-02 11:29 ` Jonathan Cameron
2025-11-02 13:45 ` Andy Shevchenko
2025-11-03 9:34 ` Francesco Lavra
2025-11-03 9:40 ` Andy Shevchenko
2025-11-03 14:53 ` David Lechner
2025-11-09 13:31 ` Jonathan Cameron
2025-10-30 7:27 ` [PATCH 7/9] iio: imu: st_lsm6dsx: make event management functions generic Francesco Lavra
2025-10-30 8:15 ` Andy Shevchenko
2025-10-30 11:17 ` Francesco Lavra
2025-10-30 13:36 ` Andy Shevchenko
2025-11-02 11:33 ` Jonathan Cameron
2025-10-30 7:27 ` [PATCH 8/9] iio: imu: st_lsm6dsx: add event configurability on a per axis basis Francesco Lavra
2025-10-30 8:24 ` Andy Shevchenko
2025-10-30 11:23 ` Francesco Lavra
2025-10-30 13:56 ` Andy Shevchenko
2025-11-17 19:23 ` Francesco Lavra
2025-11-18 10:44 ` Andy Shevchenko
2025-11-18 11:01 ` Francesco Lavra
2025-11-20 9:05 ` Andy Shevchenko
2025-11-20 11:43 ` Francesco Lavra
2025-11-20 13:59 ` Andy Shevchenko
2025-11-20 18:31 ` Andy Shevchenko
2025-11-21 9:14 ` Francesco Lavra
2025-11-21 9:31 ` Andy Shevchenko
2025-11-21 14:57 ` Francesco Lavra
2025-12-07 15:11 ` Jonathan Cameron
2025-10-30 7:27 ` [PATCH 9/9] iio: imu: st_lsm6dsx: add tap event detection Francesco Lavra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aQOVcCinTd-ZJJX3@lore-desk \
--to=lorenzo@kernel.org \
--cc=andy@kernel.org \
--cc=dlechner@baylibre.com \
--cc=flavra@baylibre.com \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nuno.sa@analog.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox