Re: [PATCH 08/10] iio: accel: BMA220 add events

[Jonathan Cameron <jic23@kernel.org>]

On Mon,  1 Sep 2025 22:47:34 +0300
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Add events for tap detection and low-g, high-g, slope conditions.
> Ignored the 80-column rule for readability.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
It might be worth splitting this up into types of event just to reduce
the amount of code and allow some description of event types in the patch
intro.

> ---
>  drivers/iio/accel/bma220_core.c | 686 ++++++++++++++++++++++++++++++++
>  1 file changed, 686 insertions(+)
> 
> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> index 41889cdcef76..c8da6cc2eaf3 100644
> --- a/drivers/iio/accel/bma220_core.c
> +++ b/drivers/iio/accel/bma220_core.c
> @@ -13,6 +13,37 @@
>   * scale (range)             | in_accel_scale                 | see _available
>   * cut-off freq (filt_config)| in_accel_filter_low_pass_...   | see _available
>   * ---------------------------------------------------------------------------
> + *
> + * Events:
> + * Register                  | IIO ABI (sysfs)                   | valid values
> + * --------------------------+-----------------------------------+------------

This feels like documentation that ultimately may grow to the point
where it isn't useful enough to justify it's presence. Maybe we'll see what
this looks like once all the code is ready to merge and then consider if it
is worth having this.

> + * high-g detection          |                                   |
> + *   enable/disable irq      | in_accel_*_thresh_rising_en       | 0/1
> + *   threshold (high_th)     | in_accel_thresh_rising_value      | 0-15
> + *   hysteresis (high_hy)    | in_accel_thresh_rising_hysteresis | 0-3
> + *   duration (high_dur)     | in_accel_thresh_rising_period     | 0-63
> + * low-g detection           |                                   |
> + *   enable/disable irq      | in_accel_*_thresh_falling_en      | 0/1
> + *   threshold (low_th)      | in_accel_thresh_falling_value     | 0-15
> + *   hysteresis (low_hy)     | in_accel_thresh_falling_hysteresis| 0-3
> + *   duration (low_dur)      | in_accel_thresh_falling_period    | 0-63
> + * slope detection           |                                   |
> + *   enable/disable irq      | in_accel_*_thresh_either_en       | 0/1
> + *   threshold (slope_th)    | in_accel_thresh_either_value      | 0-15
> + *   duration (slope_dur)    | in_accel_thresh_either_period     | 0-3
> + * tap sensing               |                                   |
> + *   enable/disable singletap| in_accel_*_gesture_singletap_en   | 0/1 [2]
> + *   enable/disable doubletap| in_accel_*_gesture_doubletap_en   | 0/1 [2]
> + *   threshold (tt_th)       | in_accel_gesture_singletap_value  | 0-15
> + *   duration (tt_dur)       | in_accel_gesture_doubletap_period | see [1]
> + * ----------------------------------------------------------------------------
> + *
> + * [1] The event related sysfs interface provides and expects raw register values
> + *         (unshifted bitfields) based on the chip specifications.
> + * [2] Do not mix singletap and doubletap interrupt enable flags.
> + *
> + * To be on the safe side do not enable two or more concurrent interrupt events
> + * of different types.
>   */

> 
> +static int bma220_reset_int(struct bma220_data *data)
> +{
> +	return regmap_update_bits(data->regmap, BMA220_REG_IE1, BMA220_INT_RST_MSK,
> +				  FIELD_PREP(BMA220_INT_RST_MSK, 1));

regmap_set_bits() is handy in these cases.

> +}
>

> +
> +static int bma220_set_slope_en(struct bma220_data *data,
> +			       enum iio_modifier axis,
> +			       bool en)

Wrap closer to 80 chars.

> +
> +static int bma220_set_high_en(struct bma220_data *data,
> +			      enum iio_modifier axis,
> +			      bool en)
> +{
> +	int ret;
> +
> +	switch (axis) {
> +	case IIO_MOD_X:
> +		ret = regmap_update_bits(data->regmap, BMA220_REG_IE1,
> +					 BMA220_INT_EN_HIGH_X_MSK,
> +					 FIELD_PREP(BMA220_INT_EN_HIGH_X_MSK, en));
> +		break;
> +	case IIO_MOD_Y:
> +		ret = regmap_update_bits(data->regmap, BMA220_REG_IE1,
> +					 BMA220_INT_EN_HIGH_Y_MSK,
> +					 FIELD_PREP(BMA220_INT_EN_HIGH_Y_MSK, en));
> +		break;
> +	case IIO_MOD_Z:
> +		ret = regmap_update_bits(data->regmap, BMA220_REG_IE1,
> +					 BMA220_INT_EN_HIGH_Z_MSK,
> +					 FIELD_PREP(BMA220_INT_EN_HIGH_Z_MSK, en));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	if (ret)

As below. Early returns simplify this.

> +		return ret;
> +
> +	return 0;
return ret;

> +}
>
> +// set if the event is enabled

All IIO comments are /* */ except the SPDX tags where appropriate.


> +static int bma220_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     bool state)
> +{
> +	struct bma220_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	guard(mutex)(&data->lock);
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (dir) {
> +		case IIO_EV_DIR_SINGLETAP:
> +			ret = bma220_set_tap_en(data, chan->channel2,
> +						IIO_EV_DIR_SINGLETAP, state);
> +			break;
> +		case IIO_EV_DIR_DOUBLETAP:
> +			ret = bma220_set_tap_en(data, chan->channel2,
> +						IIO_EV_DIR_DOUBLETAP, state);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case IIO_EV_TYPE_THRESH:
> +		switch (dir) {
> +		case IIO_EV_DIR_EITHER:
> +			ret = bma220_set_slope_en(data, chan->channel2, state);
> +			break;
> +		case IIO_EV_DIR_RISING:
> +			ret = bma220_set_high_en(data, chan->channel2, state);
> +			break;
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_update_bits(data->regmap, BMA220_REG_IE1,
> +						 BMA220_INT_EN_LOW_MSK,
> +						 FIELD_PREP(BMA220_INT_EN_LOW_MSK, state));
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	return bma220_reset_int(data);
> +}
> +
> +static int bma220_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info,
> +				   int *val, int *val2)
> +{
> +	struct bma220_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = regmap_read(data->regmap, BMA220_REG_CONF3, &reg_val);
Whilst it is more code, convention would be to be side effect free on error so.
			if (ret)
				return ret;

			*val = FIELD_GET(B...);
			return IIO_VAL_INT;

> +			*val = FIELD_GET(BMA220_TT_TH_MSK, reg_val);
> +			break;
> +		case IIO_EV_INFO_PERIOD:
> +			ret = regmap_read(data->regmap, BMA220_REG_CONF3, &reg_val);
> +			*val = FIELD_GET(BMA220_TT_DUR_MSK, reg_val);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case IIO_EV_TYPE_THRESH:
> +		switch (dir) {
> +		case IIO_EV_DIR_EITHER:
> +			switch (info) {
> +			case IIO_EV_INFO_VALUE:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF4,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_SLOPE_TH_MSK, reg_val);
> +				break;
> +			case IIO_EV_INFO_PERIOD:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF4,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_SLOPE_DUR_MSK, reg_val);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			break;
> +		case IIO_EV_DIR_RISING:
> +			switch (info) {
> +			case IIO_EV_INFO_VALUE:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF1,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_HIGH_TH_MSK, reg_val);
> +				break;
> +			case IIO_EV_INFO_PERIOD:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF0,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_HIGH_DUR_MSK, reg_val);
> +				break;
> +			case IIO_EV_INFO_HYSTERESIS:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF0,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_HIGH_HY_MSK, reg_val);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			break;
> +		case IIO_EV_DIR_FALLING:
> +			switch (info) {
> +			case IIO_EV_INFO_VALUE:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF1,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_LOW_TH_MSK, reg_val);
> +				break;
> +			case IIO_EV_INFO_PERIOD:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF2,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_LOW_DUR_MSK, reg_val);
> +				break;
> +			case IIO_EV_INFO_HYSTERESIS:
> +				ret = regmap_read(data->regmap, BMA220_REG_CONF2,
> +						  &reg_val);
> +				*val = FIELD_GET(BMA220_LOW_HY_MSK, reg_val);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	return IIO_VAL_INT;
As below. I'd do early returns.  you may end up with a few more checks
but the code will be easier to follow (slightly!)
> +}
> +
> +static int bma220_write_event_value(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info,
> +				    int val, int val2)
> +{
> +	struct bma220_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			if (!FIELD_FIT(BMA220_TT_TH_MSK, val))
> +				return -EINVAL;
> +			ret = regmap_update_bits(data->regmap, BMA220_REG_CONF3,
> +						 BMA220_TT_TH_MSK,
> +						 FIELD_PREP(BMA220_TT_TH_MSK, val));
I'm a fan of returning early if there is nothing else to do.  Saves
scrolling up and down if trying to follow the code flow for a specific
set of inputs.
			return regmap_update_bits();

> +			break;
> +		case IIO_EV_INFO_PERIOD:
> +			if (!FIELD_FIT(BMA220_TT_DUR_MSK, val))
> +				return -EINVAL;
> +			ret = regmap_update_bits(data->regmap, BMA220_REG_CONF3,
> +						 BMA220_TT_DUR_MSK,
> +						 FIELD_PREP(BMA220_TT_DUR_MSK, val));
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;

With early returns this will be dead code that should be dropped.

> +	case IIO_EV_TYPE_THRESH:
> +		switch (dir) {
> +		case IIO_EV_DIR_EITHER:
> +			switch (info) {
> +			case IIO_EV_INFO_VALUE:
> +				if (!FIELD_FIT(BMA220_SLOPE_TH_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF4,
> +							 BMA220_SLOPE_TH_MSK,
> +							 FIELD_PREP(BMA220_SLOPE_TH_MSK, val));
> +				break;
> +			case IIO_EV_INFO_PERIOD:
> +				if (!FIELD_FIT(BMA220_SLOPE_DUR_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF4,
> +							 BMA220_SLOPE_DUR_MSK,
> +							 FIELD_PREP(BMA220_SLOPE_DUR_MSK, val));
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			break;
> +		case IIO_EV_DIR_RISING:
> +			switch (info) {
> +			case IIO_EV_INFO_VALUE:
> +				if (!FIELD_FIT(BMA220_HIGH_TH_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF1,
> +							 BMA220_HIGH_TH_MSK,
> +							 FIELD_PREP(BMA220_HIGH_TH_MSK, val));
> +				break;
> +			case IIO_EV_INFO_PERIOD:
> +				if (!FIELD_FIT(BMA220_HIGH_DUR_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF0,
> +							 BMA220_HIGH_DUR_MSK,
> +							 FIELD_PREP(BMA220_HIGH_DUR_MSK, val));
> +				break;
> +			case IIO_EV_INFO_HYSTERESIS:
> +				if (!FIELD_FIT(BMA220_HIGH_HY_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF0,
> +							 BMA220_HIGH_HY_MSK,
> +							 FIELD_PREP(BMA220_HIGH_HY_MSK, val));
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			break;
> +		case IIO_EV_DIR_FALLING:
> +			switch (info) {
> +			case IIO_EV_INFO_VALUE:
> +				if (!FIELD_FIT(BMA220_LOW_TH_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF1,
> +							 BMA220_LOW_TH_MSK,
> +							 FIELD_PREP(BMA220_LOW_TH_MSK, val));
> +				break;
> +			case IIO_EV_INFO_PERIOD:
> +				if (!FIELD_FIT(BMA220_LOW_DUR_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF2,
> +							 BMA220_LOW_DUR_MSK,
> +							 FIELD_PREP(BMA220_LOW_DUR_MSK, val));
> +				break;
> +			case IIO_EV_INFO_HYSTERESIS:
> +				if (!FIELD_FIT(BMA220_LOW_HY_MSK, val))
> +					return -EINVAL;
> +				ret = regmap_update_bits(data->regmap,
> +							 BMA220_REG_CONF2,
> +							 BMA220_LOW_HY_MSK,
> +							 FIELD_PREP(BMA220_LOW_HY_MSK, val));
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
With all early returns as above this isn't needed as we'll never get here.

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

> 
> @@ -584,6 +1230,7 @@ static irqreturn_t bma220_irq_handler(int irq, void *private)
>  	struct bma220_data *data = iio_priv(indio_dev);
>  	int rv;
>  	u8 bma220_reg_if[2];
> +	s64 timestamp = iio_get_time_ns(indio_dev);
> 
>  	guard(mutex)(&data->lock);
>  	rv = regmap_bulk_read(data->regmap, BMA220_REG_IF0, bma220_reg_if,
> @@ -596,6 +1243,45 @@ static irqreturn_t bma220_irq_handler(int irq, void *private)
>  		goto done;
>  	}
> 
> +	if (FIELD_GET(BMA220_IF_HIGH, bma220_reg_if[1]))
> +		iio_push_event(indio_dev,
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +						  IIO_MOD_X_OR_Y_OR_Z,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_RISING),
> +			       timestamp);
> +	if (FIELD_GET(BMA220_IF_LOW, bma220_reg_if[1]))
> +		iio_push_event(indio_dev,
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +						  IIO_MOD_X_OR_Y_OR_Z,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_FALLING),
> +			       timestamp);
> +	if (FIELD_GET(BMA220_IF_SLOPE, bma220_reg_if[1]))
> +		iio_push_event(indio_dev,

Sounds like a ROC event. (Rate of change) not a simple threshold.

> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +						  IIO_MOD_X_OR_Y_OR_Z,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_EITHER),
> +			       timestamp);
> +	if (FIELD_GET(BMA220_IF_TT, bma220_reg_if[1])) {
> +
> +		if (data->tap_type == BMA220_TAP_TYPE_SINGLE)
> +			iio_push_event(indio_dev,
> +				       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							  IIO_MOD_X_OR_Y_OR_Z,
> +							  IIO_EV_TYPE_GESTURE,
> +							  IIO_EV_DIR_SINGLETAP),
> +				       timestamp);
> +		else
> +			iio_push_event(indio_dev,
> +				       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +							  IIO_MOD_X_OR_Y_OR_Z,
> +							  IIO_EV_TYPE_GESTURE,
> +							  IIO_EV_DIR_DOUBLETAP),
> +				       timestamp);
> +	}