On Tue, Jun 10, 2025 at 09:59:29PM +0000, Lothar Rubusch wrote:
> Make the sensor detect and issue interrupts at activity. Activity
> events are configured by a threshold. 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.
>
> Activity, ODR configuration together with the range setting prepare the
> activity/inactivity hysteresis setup, implemented in a follow up patch.
> Thus parts of this patch prepare switch/case setups for the follow up
> patches.
...
> +/* act/inact */
Useless comment. If you want it to stay, decrypt it and make it useful.
...
> +static int adxl345_is_act_inact_en(struct adxl345_state *st,
> + enum adxl345_activity_type type)
> +{
> + unsigned int regval;
> + u32 axis_ctrl;
> + bool en;
> + int ret;
> +
> + ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> + if (ret)
> + return ret;
> +
> + switch (type) {
> + case ADXL345_ACTIVITY:
> + en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl) |
> + FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl) |
> + FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
Something happened to the indentation.
Ditto for several places in the code (upper and lower from this).
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (!en)
> + return en;
> +
> + /* Check if corresponding interrupts are enabled */
> + ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
> + if (ret)
> + return ret;
> +
> + return adxl345_act_int_reg[type] & regval;
> +}
...
> + if (type == ADXL345_ACTIVITY) {
> + axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> + ADXL345_ACT_Z_EN;
> + } else {
> + axis_ctrl = 0x00;
> + }
Besides an indentation issue, {} are redundant.
...
> + en = false;
This line makes no sense. When it will, it should be there, not in this change.
> + switch (type) {
> + case ADXL345_ACTIVITY:
> + en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
> + threshold;
> + break;
> + default:
> + return -EINVAL;
> + }
...
> switch (type) {
> + case IIO_EV_TYPE_MAG:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + ret = regmap_read(st->regmap,
> + adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> + &act_threshold);
> + if (ret)
> + return ret;
> + *val = 62500 * act_threshold;
> + *val2 = MICRO;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
As I mentioned before, try to avoid nested switch cases like this. Use helpers
to make this gone to 1 level or so.
> case IIO_EV_TYPE_GESTURE:
> switch (info) {
> case IIO_EV_INFO_VALUE:
Ditto for other similar cases.
...
> static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> - enum iio_modifier tap_dir)
> + enum iio_modifier tap_dir,
> + enum iio_modifier act_dir)
If the order of parameters is not so important, I would squeeze new one to be
before the last argument.
--
With Best Regards,
Andy Shevchenko
