Re: [PATCH v5 08/11] iio: accel: adxl345: add activity event feature

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 18 Mar 2025 23:08:40 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Make the sensor detect and issue interrupts at activity. Activity
> events are configured by a threshold stored in regmap cache. Initialize
> the activity threshold register to a reasonable default value in probe.
> The value is taken from the older ADXL345 input driver, to provide a
> similar behavior. Reset the activity/inactivity direction enabling
> register in probe. Reset and initialization shall bring the sensor in a
> defined initial state to prevent dangling settings when warm restarting
> the sensor.
> 
> Activity, ODR configuration together with the range setting prepare the
> activity/inactivity hystersesis setup, implemented in a follow up patch.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

> +static int adxl345_is_act_inact_en(struct adxl345_state *st,
> +				   enum iio_modifier axis,
> +				   enum adxl345_activity_type type, bool *en)
> +{
> +	unsigned int regval;
> +	bool axis_en;
> +	u32 axis_ctrl;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	if (type == ADXL345_ACTIVITY) {
> +		switch (axis) {
> +		case IIO_MOD_X:
> +			axis_en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl);
> +			break;
> +		case IIO_MOD_Y:
> +			axis_en = FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl);
> +			break;
> +		case IIO_MOD_Z:
> +			axis_en = FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
Same as in earlier patch; axis_en is never used.
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*en = (adxl345_act_int_reg[type] & regval) > 0;
> +
> +	return 0;
> +}
> +
> +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> +				    enum iio_modifier axis,
> +				    enum adxl345_activity_type type,
> +				    bool cmd_en)
> +{
> +	bool axis_en, en;
> +	unsigned int threshold;
> +	u32 axis_ctrl = 0;
> +	int ret;
> +
> +	if (type == ADXL345_ACTIVITY) {
> +		switch (axis) {
> +		case IIO_MOD_X:
> +			axis_ctrl = ADXL345_ACT_X_EN;
> +			break;
> +		case IIO_MOD_Y:
> +			axis_ctrl = ADXL345_ACT_Y_EN;
> +			break;
> +		case IIO_MOD_Z:
> +			axis_ctrl = ADXL345_ACT_Z_EN;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (cmd_en)
> +		ret = regmap_set_bits(st->regmap,
> +				      ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
> +	else
> +		ret = regmap_clear_bits(st->regmap,
> +					ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> +	if (ret)
> +		return ret;
> +
> +	en = false;
> +
> +	if (type == ADXL345_ACTIVITY) {
> +		axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) > 0;
The > 0 doesn't add anything as this can't be negative.

Drag declaration of axis_en down here as only used in this block.
or just combine with previous and next bit as
		en = (type === ADXL345_ACTIVITY) &&
		     FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
		     (threshold > 0);

> +		en = axis_en && threshold > 0;
> +	}
> +
> +	return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> +				  adxl345_act_int_reg[type],
> +				  en ? adxl345_act_int_reg[type] : 0);
> +}
> +
>  /* tap */
>  



> @@ -1347,6 +1542,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		if (ret)
>  			return ret;
>  
> +		ret = regmap_write(st->regmap, ADXL345_REG_ACT_INACT_CTRL, 0);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);

6 is a fairly random number. Add a comment for why this default.

> +		if (ret)
> +			return ret;
> +
>  		ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
>  		if (ret)
>  			return ret;