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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.