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