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.
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];
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)
+{
+ struct iio_chan_spec *chan = &channels[index];
+
+ chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL : IIO_ANGL_VEL;
+ 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;
+ chan->ext_info = st_lsm6dsx_ext_info;
+ if (id == ST_LSM6DSX_ID_ACC) {
+ if (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;
+ 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);
+ if (!channels)
+ return NULL;
+
+ for (i = 0; i < 3; i++) {
+ if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
+ return NULL;
+ }
+ 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.5On Thu, 30 Oct 2025 08:27:44 +0100 Francesco Lavra <flavra@baylibre.com> wrote: > 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. > > Signed-off-by: Francesco Lavra <flavra@baylibre.com> In cases where there are only a small number of options for what the channel arrays should contain, my normal preference would be more data over moving the complexity into code. That is have two struct iio_chan_spec arrays and pick between them based on availability of the interrupt. I haven't checked the whole series yet, but how many channel arrays would we need to support the features you are introducing here? That is how many different combinations exist in the supported chips? Jonathan
On Sun, 2025-11-02 at 11:16 +0000, Jonathan Cameron wrote: > On Thu, 30 Oct 2025 08:27:44 +0100 > Francesco Lavra <flavra@baylibre.com> wrote: > > > 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. > > > > Signed-off-by: Francesco Lavra <flavra@baylibre.com> > > In cases where there are only a small number of options for what the > channel > arrays should contain, my normal preference would be more data over > moving > the complexity into code. That is have two struct iio_chan_spec arrays > and > pick between them based on availability of the interrupt. > > I haven't checked the whole series yet, but how many channel arrays > would we need to support the features you are introducing here? That is > how many different combinations exist in the supported chips? In the current code there are 3 struct iio_chan_spec arrays; we would need one more to fix the above bug, and one more to add tap event support; so a total of 5 arrays (each of length 4). As for struct iio_event_spec, the current code has one array (of length 1), and to add tap event support we would need another array (of length 2).
On Mon, 03 Nov 2025 10:24:54 +0100 Francesco Lavra <flavra@baylibre.com> wrote: > On Sun, 2025-11-02 at 11:16 +0000, Jonathan Cameron wrote: > > On Thu, 30 Oct 2025 08:27:44 +0100 > > Francesco Lavra <flavra@baylibre.com> wrote: > > > > > 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. > > > > > > Signed-off-by: Francesco Lavra <flavra@baylibre.com> > > > > In cases where there are only a small number of options for what the > > channel > > arrays should contain, my normal preference would be more data over > > moving > > the complexity into code. That is have two struct iio_chan_spec arrays > > and > > pick between them based on availability of the interrupt. > > > > I haven't checked the whole series yet, but how many channel arrays > > would we need to support the features you are introducing here? That is > > how many different combinations exist in the supported chips? > > In the current code there are 3 struct iio_chan_spec arrays; we would need > one more to fix the above bug, and one more to add tap event support; so a > total of 5 arrays (each of length 4). > As for struct iio_event_spec, the current code has one array (of length 1), > and to add tap event support we would need another array (of length 2). That sounds small enough to me that I'd prefer const data that you pick between rather than dynamic creation. Jonathan
> 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
>
[missed this one]
On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:
>
> > + 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) {
> ...
> }
In patch 4/9, the inner conditional will be replaced by more generic code,
so we would revert to if (id == ST_LSM6DSX_ID_ACC) [...]
On Fri, Oct 31, 2025 at 09:26:19AM +0100, Francesco Lavra wrote:
> On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:
> > > + 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) {
> > ...
> > }
>
> In patch 4/9, the inner conditional will be replaced by more generic code,
> so we would revert to if (id == ST_LSM6DSX_ID_ACC) [...]
Hmm... The obvious follow up question is why can't we stick with the original
conditional to begin with?
--
With Best Regards,
Andy Shevchenko
On Fri, 2025-10-31 at 10:32 +0200, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 09:26:19AM +0100, Francesco Lavra wrote:
> > On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:
>
> > > > + 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) {
> > > ...
> > > }
> >
> > In patch 4/9, the inner conditional will be replaced by more generic
> > code,
> > so we would revert to if (id == ST_LSM6DSX_ID_ACC) [...]
>
> Hmm... The obvious follow up question is why can't we stick with the
> original
> conditional to begin with?
There is no original conditional, this is new code.
So the code here is `if (cond1) {if (cond2) {}}`; in patch 4/9 it will
become `if (cond1) {something else}`.
Or, better yet, as you suggested earlier, in the next revision the code
here will be `if (cond1) helper()`, and in the patch 4/9 this will stay the
same and only the code inside the helper will change.
On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote:
> > 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.
Ack
[...]
> > +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 :))
OK
> > +{
> > + 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.
Will do
> > + 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.
Do you mean declaring a local struct variable initialized via
ST_LSM6DSX_CHANNEL() and then copying its contents to the dynamically
allocated struct? I'm not clear what benefits that would give us; in fact,
I think it would increase the code size (both in terms of LOC and compiled
binary size), besides the additional overhead of memory copying.
>
> > + 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.
Will do
> > + 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
> >
On Fri, Oct 31, 2025 at 09:04:58AM +0100, Francesco Lavra wrote: > On Thu, 2025-10-30 at 17:42 +0100, Lorenzo Bianconi wrote: [...] > > > +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 :)) > > OK FWIW, the limit is the exact 80. Don't waste that 1 characters when it's the case. (And moreover there is a note in the documentation that allows, and we follow that from time to time in IIO, slightly longer lines if it really increases readability.) -- With Best Regards, Andy Shevchenko
On Thu, Oct 30, 2025 at 08:27:44AM +0100, Francesco Lavra wrote:
> 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.
Sounds like a bug fix. Fixes tag?
...
> +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct st_lsm6dsx_hw *hw,
> + enum st_lsm6dsx_sensor_id id, int index)
> +{
> + struct iio_chan_spec *chan = &channels[index];
> +
> + chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL : IIO_ANGL_VEL;
> + 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;
> + chan->ext_info = st_lsm6dsx_ext_info;
+ blank line
> + if (id == ST_LSM6DSX_ID_ACC) {
> + if (hw->settings->event_settings.wakeup_reg.addr) {
> + chan->event_spec = &st_lsm6dsx_event;
> + chan->num_event_specs = 1;
> + }
> + }
if (foo) { if (bar) {} } == if (foo && bar).
Based on this I'm in doubt what to suggest here as to me sounds like those
couple of lines might deserve for a helper.
Hence two options:
1) do an equivalent conditional and reduce indentation level;
2) do a helper with the inner conditional.
+ blank line
> + return 0;
> +}
...
> + channels = devm_kzalloc(hw->dev, sizeof(*channels) * ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);
devm_kcalloc()
> + if (!channels)
> + return NULL;
I would expect comment here...
> + for (i = 0; i < 3; i++) {
3 might need to be defined.
> + if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> + return NULL;
> + }
+ blank line
...and perhaps here to explain what's going on 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;
+ blank line.
> 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;
--
With Best Regards,
Andy Shevchenko
On Thu, 2025-10-30 at 09:57 +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 08:27:44AM +0100, Francesco Lavra wrote:
> > 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.
>
>
> Sounds like a bug fix. Fixes tag?
Will add
>
> > +static int st_lsm6dsx_chan_init(struct iio_chan_spec *channels, struct
> > st_lsm6dsx_hw *hw,
> > + enum st_lsm6dsx_sensor_id id, int
> > index)
> > +{
> > + struct iio_chan_spec *chan = &channels[index];
> > +
> > + chan->type = (id == ST_LSM6DSX_ID_ACC) ? IIO_ACCEL :
> > IIO_ANGL_VEL;
> > + 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;
> > + chan->ext_info = st_lsm6dsx_ext_info;
>
> + blank line
>
> > + if (id == ST_LSM6DSX_ID_ACC) {
> > + if (hw->settings->event_settings.wakeup_reg.addr) {
> > + chan->event_spec = &st_lsm6dsx_event;
> > + chan->num_event_specs = 1;
> > + }
> > + }
>
> if (foo) { if (bar) {} } == if (foo && bar).
>
> Based on this I'm in doubt what to suggest here as to me sounds like
> those
> couple of lines might deserve for a helper.
>
> Hence two options:
> 1) do an equivalent conditional and reduce indentation level;
> 2) do a helper with the inner conditional.
Will do a helper with the inner conditional.
> + blank line
>
> > + return 0;
> > +}
>
> ...
>
> > + channels = devm_kzalloc(hw->dev, sizeof(*channels) *
> > ST_LSM6DSX_CHAN_COUNT, GFP_KERNEL);
>
> devm_kcalloc()
>
> > + if (!channels)
> > + return NULL;
>
> I would expect comment here...
This function returns a pointer to the struct iio_dev it allocates and
initializes; if there are any errors, it returns NULL. What kind of comment
do you expect here? It seems obvious that it's returning NULL because of an
allocation error.
>
> > + for (i = 0; i < 3; i++) {
>
> 3 might need to be defined.
Will make an enum to replace the ST_LSM6DSX_CHAN_COUNT #define, and use an
enum value instead of 3.
>
> > + if (st_lsm6dsx_chan_init(channels, hw, id, i) < 0)
> > + return NULL;
> > + }
>
> + blank line
>
> ...and perhaps here to explain what's going on here.
Same here, what comment do you expect?
> > + 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;
>
> + blank line.
>
> > 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;
>
© 2016 - 2026 Red Hat, Inc.