Inactivity and free-fall events are essentially the same type of sensor
events. Therefore, inactivity detection (normally set for periods between 1
and 255 seconds) can be extended for shorter durations to support free-fall
detection.
For periods shorter than 1 second, the driver automatically configures the
threshold and duration using the free-fall register. For periods longer
than 1 second, it uses the inactivity threshold and duration using the
inactivity registers.
When using the free-fall register, the link bit is not set, which means
auto-sleep cannot be enabled if activity detection is also active.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 158 +++++++++++++++++++++----------
1 file changed, 109 insertions(+), 49 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index c5a3dac5f938..f1f92635bc21 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -40,12 +40,15 @@
#define ADXL345_REG_INACT_AXIS_MSK GENMASK(2, 0)
#define ADXL345_REG_ACT_ACDC_MSK BIT(7)
#define ADXL345_REG_INACT_ACDC_MSK BIT(3)
+#define ADXL345_REG_NO_ACDC_MSK 0x00
#define ADXL345_POWER_CTL_INACT_MSK (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK)
#define ADXL345_TAP_Z_EN BIT(0)
#define ADXL345_TAP_Y_EN BIT(1)
#define ADXL345_TAP_X_EN BIT(2)
+#define ADXL345_ACT_INACT_NO_AXIS_EN 0x00
+
#define ADXL345_INACT_Z_EN BIT(0)
#define ADXL345_INACT_Y_EN BIT(1)
#define ADXL345_INACT_X_EN BIT(2)
@@ -86,6 +89,7 @@ enum adxl345_activity_type {
ADXL345_INACTIVITY,
ADXL345_ACTIVITY_AC,
ADXL345_INACTIVITY_AC,
+ ADXL345_INACTIVITY_FF,
};
static const unsigned int adxl345_act_int_reg[] = {
@@ -93,6 +97,7 @@ static const unsigned int adxl345_act_int_reg[] = {
[ADXL345_INACTIVITY] = ADXL345_INT_INACTIVITY,
[ADXL345_ACTIVITY_AC] = ADXL345_INT_ACTIVITY,
[ADXL345_INACTIVITY_AC] = ADXL345_INT_INACTIVITY,
+ [ADXL345_INACTIVITY_FF] = ADXL345_INT_FREE_FALL,
};
static const unsigned int adxl345_act_thresh_reg[] = {
@@ -100,6 +105,7 @@ static const unsigned int adxl345_act_thresh_reg[] = {
[ADXL345_INACTIVITY] = ADXL345_REG_THRESH_INACT,
[ADXL345_ACTIVITY_AC] = ADXL345_REG_THRESH_ACT,
[ADXL345_INACTIVITY_AC] = ADXL345_REG_THRESH_INACT,
+ [ADXL345_INACTIVITY_FF] = ADXL345_REG_THRESH_FF,
};
static const unsigned int adxl345_act_acdc_msk[] = {
@@ -107,6 +113,7 @@ static const unsigned int adxl345_act_acdc_msk[] = {
[ADXL345_INACTIVITY] = ADXL345_REG_INACT_ACDC_MSK,
[ADXL345_ACTIVITY_AC] = ADXL345_REG_ACT_ACDC_MSK,
[ADXL345_INACTIVITY_AC] = ADXL345_REG_INACT_ACDC_MSK,
+ [ADXL345_INACTIVITY_FF] = ADXL345_REG_NO_ACDC_MSK,
};
enum adxl345_odr {
@@ -189,6 +196,9 @@ struct adxl345_state {
u8 watermark;
u8 fifo_mode;
+ u8 inact_threshold;
+ u32 inact_time_ms;
+
u32 tap_duration_us;
u32 tap_latent_us;
u32 tap_window_us;
@@ -333,10 +343,29 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
/* activity / inactivity */
+static int adxl345_set_inact_threshold(struct adxl345_state *st,
+ unsigned int threshold)
+{
+ int ret;
+
+ st->inact_threshold = min(U8_MAX, threshold);
+
+ ret = regmap_write(st->regmap,
+ adxl345_act_thresh_reg[ADXL345_INACTIVITY],
+ st->inact_threshold);
+ if (ret)
+ return ret;
+
+ return regmap_write(st->regmap,
+ adxl345_act_thresh_reg[ADXL345_INACTIVITY_FF],
+ st->inact_threshold);
+}
+
/**
* adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
* @st: The sensor state instance.
- * @val_s: A desired time value, between 0 and 255.
+ * @val_int: The inactivity time, integer part.
+ * @val_fract: The inactivity time, fractional part when val_int is 0.
*
* Inactivity time can be configured between 1 and 255 seconds. If a user sets
* val_s to 0, a default inactivity time is calculated automatically (since 0 is
@@ -357,16 +386,18 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
*
* Return: 0 or error value.
*/
-static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
+static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
+ u32 val_fract)
{
int max_boundary = U8_MAX;
int min_boundary = 10;
- unsigned int val = min(val_s, U8_MAX);
+ unsigned int val;
enum adxl345_odr odr;
unsigned int regval;
int ret;
- if (val == 0) {
+ if (val_int == 0 && val_fract == 0) {
+ /* Generated inactivity time based on ODR */
ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
if (ret)
return ret;
@@ -374,9 +405,31 @@ static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
val = clamp(max_boundary - adxl345_odr_tbl[odr][0],
min_boundary, max_boundary);
+ st->inact_time_ms = MILLI * val;
+
+ /* Inactivity time in s */
+ return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
+ } else if (val_int == 0 && val_fract > 0) {
+ /* time < 1s, free-fall */
+
+ /*
+ * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
+ *
+ * Recommended values between 100ms and 350ms (0x14 to 0x46)
+ */
+ st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
+
+ return regmap_write(st->regmap, ADXL345_REG_TIME_FF,
+ DIV_ROUND_CLOSEST(val_fract, 5));
+ } else if (val_int > 0) {
+ /* Time >= 1s, inactivity */
+ st->inact_time_ms = MILLI * val_int;
+
+ return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
}
- return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
+ /* Do not support negative or wrong input. */
+ return -EINVAL;
}
/**
@@ -399,6 +452,9 @@ static int adxl345_is_act_inact_ac(struct adxl345_state *st,
bool coupling;
int ret;
+ if (type == ADXL345_INACTIVITY_FF)
+ return true;
+
ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, ®val);
if (ret)
return ret;
@@ -511,6 +567,9 @@ static int adxl345_is_act_inact_en(struct adxl345_state *st,
if (!en)
return false;
break;
+ case ADXL345_INACTIVITY_FF:
+ en = true;
+ break;
default:
return -EINVAL;
}
@@ -542,15 +601,20 @@ static int adxl345_set_act_inact_linkbit(struct adxl345_state *st,
if (act_ac_en < 0)
return act_ac_en;
- inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
- if (inact_en < 0)
- return inact_en;
+ if (type == ADXL345_INACTIVITY_FF) {
+ inact_en = false;
+ } else {
+ inact_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY);
+ if (inact_en < 0)
+ return inact_en;
- inact_ac_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY_AC);
- if (inact_ac_en < 0)
- return inact_ac_en;
+ inact_ac_en = adxl345_is_act_inact_en(st, ADXL345_INACTIVITY_AC);
+ if (inact_ac_en < 0)
+ return inact_ac_en;
+
+ inact_en = inact_en || inact_ac_en;
+ }
- inact_en = inact_en || inact_ac_en;
act_en = act_en || act_ac_en;
return regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
@@ -569,11 +633,15 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
if (cmd_en) {
/* When turning on, check if threshold is valid */
- ret = regmap_read(st->regmap,
- adxl345_act_thresh_reg[type],
- &threshold);
- if (ret)
- return ret;
+ if (type == ADXL345_ACTIVITY || type == ADXL345_ACTIVITY_AC) {
+ ret = regmap_read(st->regmap,
+ adxl345_act_thresh_reg[type],
+ &threshold);
+ if (ret)
+ return ret;
+ } else {
+ threshold = st->inact_threshold;
+ }
if (!threshold) /* Just ignore the command if threshold is 0 */
return 0;
@@ -618,6 +686,9 @@ static int adxl345_set_act_inact_en(struct adxl345_state *st,
axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
ADXL345_INACT_Z_EN;
break;
+ case ADXL345_INACTIVITY_FF:
+ axis_ctrl = ADXL345_ACT_INACT_NO_AXIS_EN;
+ break;
default:
return -EINVAL;
}
@@ -884,7 +955,7 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
return ret;
/* update inactivity time by ODR */
- return adxl345_set_inact_time(st, 0);
+ return adxl345_set_inact_time(st, 0, 0);
}
static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
@@ -921,12 +992,6 @@ static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
if (ret)
return ret;
- ret = regmap_read(st->regmap,
- adxl345_act_thresh_reg[ADXL345_INACTIVITY],
- &inact_threshold);
- if (ret)
- return ret;
-
ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
ADXL345_DATA_FORMAT_RANGE,
FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
@@ -937,6 +1002,7 @@ static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
/ adxl345_range_factor_tbl[range];
act_threshold = min(U8_MAX, max(1, act_threshold));
+ inact_threshold = st->inact_threshold;
inact_threshold = inact_threshold * adxl345_range_factor_tbl[range_old]
/ adxl345_range_factor_tbl[range];
inact_threshold = min(U8_MAX, max(1, inact_threshold));
@@ -946,8 +1012,7 @@ static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
if (ret)
return ret;
- return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
- inact_threshold);
+ return adxl345_set_inact_threshold(st, inact_threshold);
}
static int adxl345_read_avail(struct iio_dev *indio_dev,
@@ -1192,7 +1257,6 @@ static int adxl345_read_mag_value(struct adxl345_state *st,
int *val, int *val2)
{
unsigned int threshold;
- unsigned int period;
int ret;
switch (info) {
@@ -1208,25 +1272,16 @@ static int adxl345_read_mag_value(struct adxl345_state *st,
*val2 = MICRO;
return IIO_VAL_FRACTIONAL;
case IIO_EV_DIR_FALLING:
- ret = regmap_read(st->regmap,
- adxl345_act_thresh_reg[type_inact],
- &threshold);
- if (ret)
- return ret;
- *val = 62500 * threshold;
+ *val = 62500 * st->inact_threshold;
*val2 = MICRO;
return IIO_VAL_FRACTIONAL;
default:
return -EINVAL;
}
case IIO_EV_INFO_PERIOD:
- ret = regmap_read(st->regmap,
- ADXL345_REG_TIME_INACT,
- &period);
- if (ret)
- return ret;
- *val = period;
- return IIO_VAL_INT;
+ *val = st->inact_time_ms;
+ *val2 = MILLI;
+ return IIO_VAL_FRACTIONAL;
default:
return -EINVAL;
}
@@ -1249,14 +1304,12 @@ static int adxl345_write_mag_value(struct adxl345_state *st,
adxl345_act_thresh_reg[type_act],
val);
case IIO_EV_DIR_FALLING:
- return regmap_write(st->regmap,
- adxl345_act_thresh_reg[type_inact],
- val);
+ return adxl345_set_inact_threshold(st, val);
default:
return -EINVAL;
}
case IIO_EV_INFO_PERIOD:
- return adxl345_set_inact_time(st, val);
+ return adxl345_set_inact_time(st, val, val2);
default:
return -EINVAL;
}
@@ -1669,6 +1722,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
return ret;
}
+ if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
+ ret = iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_AND_Y_AND_Z,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_FALLING),
+ ts);
+ if (ret)
+ return ret;
+ }
+
if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
samples = adxl345_get_samples(st);
if (samples < 0)
@@ -1907,15 +1971,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
if (ret)
return ret;
- ret = regmap_write(st->regmap, ADXL345_REG_TIME_INACT, 0x37);
- if (ret)
- return ret;
-
ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
if (ret)
return ret;
- ret = regmap_write(st->regmap, ADXL345_REG_THRESH_INACT, 4);
+ ret = adxl345_set_inact_threshold(st, 4);
if (ret)
return ret;
--
2.39.5On Sun, Jun 22, 2025 at 03:50:09PM +0000, Lothar Rubusch wrote:
> Inactivity and free-fall events are essentially the same type of sensor
> events. Therefore, inactivity detection (normally set for periods between 1
> and 255 seconds) can be extended for shorter durations to support free-fall
> detection.
>
> For periods shorter than 1 second, the driver automatically configures the
> threshold and duration using the free-fall register. For periods longer
> than 1 second, it uses the inactivity threshold and duration using the
> inactivity registers.
>
> When using the free-fall register, the link bit is not set, which means
> auto-sleep cannot be enabled if activity detection is also active.
...
> -static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
> + u32 val_fract)
> {
> int max_boundary = U8_MAX;
> int min_boundary = 10;
> - unsigned int val = min(val_s, U8_MAX);
> + unsigned int val;
You see, I even suggested splitting this assignment to begin with.
The change will be clearer with that done.
> enum adxl345_odr odr;
> unsigned int regval;
> int ret;
>
> - if (val == 0) {
> + if (val_int == 0 && val_fract == 0) {
> + /* Generated inactivity time based on ODR */
> ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> if (ret)
> return ret;
> odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> val = clamp(max_boundary - adxl345_odr_tbl[odr][0],
> min_boundary, max_boundary);
> + st->inact_time_ms = MILLI * val;
> +
> + /* Inactivity time in s */
> + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> + } else if (val_int == 0 && val_fract > 0) {
val_fract check is not needed here.
> + /* time < 1s, free-fall */
> +
> + /*
> + * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
> + *
> + * Recommended values between 100ms and 350ms (0x14 to 0x46)
> + */
> + st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
> +
> + return regmap_write(st->regmap, ADXL345_REG_TIME_FF,
> + DIV_ROUND_CLOSEST(val_fract, 5));
> + } else if (val_int > 0) {
if now is redundant here, right?
> + /* Time >= 1s, inactivity */
> + st->inact_time_ms = MILLI * val_int;
> +
> + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
> }
>
> - return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> + /* Do not support negative or wrong input. */
> + return -EINVAL;
> }
--
With Best Regards,
Andy Shevchenko
Hi Andy,
This is a tricky one, I'll give some examples why (I think) the code
is needed as is.
On Mon, Jun 23, 2025 at 12:17 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Sun, Jun 22, 2025 at 03:50:09PM +0000, Lothar Rubusch wrote:
> > Inactivity and free-fall events are essentially the same type of sensor
> > events. Therefore, inactivity detection (normally set for periods between 1
> > and 255 seconds) can be extended for shorter durations to support free-fall
> > detection.
> >
> > For periods shorter than 1 second, the driver automatically configures the
> > threshold and duration using the free-fall register. For periods longer
> > than 1 second, it uses the inactivity threshold and duration using the
> > inactivity registers.
> >
> > When using the free-fall register, the link bit is not set, which means
> > auto-sleep cannot be enabled if activity detection is also active.
>
> ...
>
> > -static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> > +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
> > + u32 val_fract)
> > {
> > int max_boundary = U8_MAX;
> > int min_boundary = 10;
> > - unsigned int val = min(val_s, U8_MAX);
> > + unsigned int val;
>
> You see, I even suggested splitting this assignment to begin with.
> The change will be clearer with that done.
>
> > enum adxl345_odr odr;
> > unsigned int regval;
> > int ret;
> >
> > - if (val == 0) {
> > + if (val_int == 0 && val_fract == 0) {
The case for 0sec, 0.0 or setting "0" and fract will consequently be
"0". 0 is an invalid input for this period and sensor, so it will
default to an optimized period based on given ODR.
> > + /* Generated inactivity time based on ODR */
> > ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > if (ret)
> > return ret;
>
> > odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > val = clamp(max_boundary - adxl345_odr_tbl[odr][0],
> > min_boundary, max_boundary);
> > + st->inact_time_ms = MILLI * val;
> > +
> > + /* Inactivity time in s */
> > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > + } else if (val_int == 0 && val_fract > 0) {
>
> val_fract check is not needed here.
>
Case for e.g. 0.123, numbers under 1s. This goes into the free-fall register.
> > + /* time < 1s, free-fall */
> > +
> > + /*
> > + * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
> > + *
> > + * Recommended values between 100ms and 350ms (0x14 to 0x46)
> > + */
> > + st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
> > +
> > + return regmap_write(st->regmap, ADXL345_REG_TIME_FF,
> > + DIV_ROUND_CLOSEST(val_fract, 5));
> > + } else if (val_int > 0) {
>
> if now is redundant here, right?
>
So, this will be 1s through 255s. Periods above 1sec. This goes into
the inactivity register.
> > + /* Time >= 1s, inactivity */
> > + st->inact_time_ms = MILLI * val_int;
> > +
> > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
> > }
> >
> > - return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > + /* Do not support negative or wrong input. */
> > + return -EINVAL;
> > }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Jun 23, 2025 at 11:21:01PM +0200, Lothar Rubusch wrote:
> On Mon, Jun 23, 2025 at 12:17 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Sun, Jun 22, 2025 at 03:50:09PM +0000, Lothar Rubusch wrote:
...
> > > -static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> > > +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
> > > + u32 val_fract)
> > > {
> > > int max_boundary = U8_MAX;
> > > int min_boundary = 10;
> > > - unsigned int val = min(val_s, U8_MAX);
> > > + unsigned int val;
> >
> > You see, I even suggested splitting this assignment to begin with.
> > The change will be clearer with that done.
> >
> > > enum adxl345_odr odr;
> > > unsigned int regval;
> > > int ret;
> > >
> > > - if (val == 0) {
> > > + if (val_int == 0 && val_fract == 0) {
>
> The case for 0sec, 0.0 or setting "0" and fract will consequently be
> "0". 0 is an invalid input for this period and sensor, so it will
> default to an optimized period based on given ODR.
>
> > > + /* Generated inactivity time based on ODR */
> > > ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > > if (ret)
> > > return ret;
> >
> > > odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > val = clamp(max_boundary - adxl345_odr_tbl[odr][0],
> > > min_boundary, max_boundary);
> > > + st->inact_time_ms = MILLI * val;
> > > +
> > > + /* Inactivity time in s */
> > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > + } else if (val_int == 0 && val_fract > 0) {
> >
> > val_fract check is not needed here.
>
> Case for e.g. 0.123, numbers under 1s. This goes into the free-fall register.
0.0 is already checked above, and since the val_fract is unsigned this is check
is redundant.
> > > + /* time < 1s, free-fall */
> > > +
> > > + /*
> > > + * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
> > > + *
> > > + * Recommended values between 100ms and 350ms (0x14 to 0x46)
> > > + */
> > > + st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
> > > +
> > > + return regmap_write(st->regmap, ADXL345_REG_TIME_FF,
> > > + DIV_ROUND_CLOSEST(val_fract, 5));
> > > + } else if (val_int > 0) {
> >
> > if now is redundant here, right?
>
> So, this will be 1s through 255s. Periods above 1sec. This goes into
> the inactivity register.
See above,
> > > + /* Time >= 1s, inactivity */
> > > + st->inact_time_ms = MILLI * val_int;
> > > +
> > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
> > > }
> > >
> > > - return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > + /* Do not support negative or wrong input. */
> > > + return -EINVAL;
> > > }
--
With Best Regards,
Andy Shevchenko
On Tue, Jun 24, 2025 at 9:38 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 23, 2025 at 11:21:01PM +0200, Lothar Rubusch wrote:
> > On Mon, Jun 23, 2025 at 12:17 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Sun, Jun 22, 2025 at 03:50:09PM +0000, Lothar Rubusch wrote:
>
> ...
>
> > > > -static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> > > > +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_int,
> > > > + u32 val_fract)
> > > > {
> > > > int max_boundary = U8_MAX;
> > > > int min_boundary = 10;
> > > > - unsigned int val = min(val_s, U8_MAX);
> > > > + unsigned int val;
> > >
> > > You see, I even suggested splitting this assignment to begin with.
> > > The change will be clearer with that done.
> > >
> > > > enum adxl345_odr odr;
> > > > unsigned int regval;
> > > > int ret;
> > > >
> > > > - if (val == 0) {
> > > > + if (val_int == 0 && val_fract == 0) {
> >
> > The case for 0sec, 0.0 or setting "0" and fract will consequently be
> > "0". 0 is an invalid input for this period and sensor, so it will
> > default to an optimized period based on given ODR.
> >
> > > > + /* Generated inactivity time based on ODR */
> > > > ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > > > if (ret)
> > > > return ret;
> > >
> > > > odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > val = clamp(max_boundary - adxl345_odr_tbl[odr][0],
> > > > min_boundary, max_boundary);
> > > > + st->inact_time_ms = MILLI * val;
> > > > +
> > > > + /* Inactivity time in s */
> > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > + } else if (val_int == 0 && val_fract > 0) {
> > >
> > > val_fract check is not needed here.
> >
> > Case for e.g. 0.123, numbers under 1s. This goes into the free-fall register.
>
> 0.0 is already checked above, and since the val_fract is unsigned this is check
> is redundant.
>
> > > > + /* time < 1s, free-fall */
> > > > +
> > > > + /*
> > > > + * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
> > > > + *
> > > > + * Recommended values between 100ms and 350ms (0x14 to 0x46)
> > > > + */
> > > > + st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
> > > > +
> > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_FF,
> > > > + DIV_ROUND_CLOSEST(val_fract, 5));
> > > > + } else if (val_int > 0) {
> > >
> > > if now is redundant here, right?
> >
> > So, this will be 1s through 255s. Periods above 1sec. This goes into
> > the inactivity register.
>
> See above,
>
I agree, that checking for val_fract is actually done as a sub case of
val_int, and only if val_int was 0. So, would the following make it
clearer?
if (val_int == 0) {
if (val_fract == 0) {
// 0 provided, default values
} else {
// >0s, e.g. 0.123s, use free-fall register
} else {
// 1s - 255s, use inactivity register
}
Actually - I did not touch that - I saw some places where I'm already
using nested if/else in the third level. I guess, by the style advice
according to switch/case, this also applies to if/else, right?
If yes, when the according parts go into another round, I might give
it a try to separate as well using helper functions.
Best,
L
> > > > + /* Time >= 1s, inactivity */
> > > > + st->inact_time_ms = MILLI * val_int;
> > > > +
> > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
> > > > }
> > > >
> > > > - return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > + /* Do not support negative or wrong input. */
> > > > + return -EINVAL;
> > > > }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Tue, Jun 24, 2025 at 10:18:35AM +0200, Lothar Rubusch wrote:
> On Tue, Jun 24, 2025 at 9:38 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Jun 23, 2025 at 11:21:01PM +0200, Lothar Rubusch wrote:
> > > On Mon, Jun 23, 2025 at 12:17 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Sun, Jun 22, 2025 at 03:50:09PM +0000, Lothar Rubusch wrote:
...
> > > > > - if (val == 0) {
> > > > > + if (val_int == 0 && val_fract == 0) {
> > >
> > > The case for 0sec, 0.0 or setting "0" and fract will consequently be
> > > "0". 0 is an invalid input for this period and sensor, so it will
> > > default to an optimized period based on given ODR.
> > >
> > > > > + /* Generated inactivity time based on ODR */
> > > > > ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > > > > if (ret)
> > > > > return ret;
> > > >
> > > > > odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
> > > > > val = clamp(max_boundary - adxl345_odr_tbl[odr][0],
> > > > > min_boundary, max_boundary);
> > > > > + st->inact_time_ms = MILLI * val;
> > > > > +
> > > > > + /* Inactivity time in s */
> > > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > > > > + } else if (val_int == 0 && val_fract > 0) {
> > > >
> > > > val_fract check is not needed here.
> > >
> > > Case for e.g. 0.123, numbers under 1s. This goes into the free-fall register.
> >
> > 0.0 is already checked above, and since the val_fract is unsigned this is check
> > is redundant.
> >
> > > > > + /* time < 1s, free-fall */
> > > > > +
> > > > > + /*
> > > > > + * Datasheet max. value is 255 * 5000 us = 1.275000 seconds.
> > > > > + *
> > > > > + * Recommended values between 100ms and 350ms (0x14 to 0x46)
> > > > > + */
> > > > > + st->inact_time_ms = DIV_ROUND_UP(val_fract, MILLI);
> > > > > +
> > > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_FF,
> > > > > + DIV_ROUND_CLOSEST(val_fract, 5));
> > > > > + } else if (val_int > 0) {
> > > >
> > > > if now is redundant here, right?
> > >
> > > So, this will be 1s through 255s. Periods above 1sec. This goes into
> > > the inactivity register.
> >
> > See above,
> >
>
> I agree, that checking for val_fract is actually done as a sub case of
> val_int, and only if val_int was 0. So, would the following make it
> clearer?
>
> if (val_int == 0) {
> if (val_fract == 0) {
> // 0 provided, default values
> } else {
> // >0s, e.g. 0.123s, use free-fall register
> } else {
> // 1s - 255s, use inactivity register
> }
>
> Actually - I did not touch that - I saw some places where I'm already
> using nested if/else in the third level. I guess, by the style advice
> according to switch/case, this also applies to if/else, right?
>
> If yes, when the according parts go into another round, I might give
> it a try to separate as well using helper functions.
You can think through the patches. It might make sense to consider as well this
helper_1()
{
// for default
}
helper_2()
{
// for free-fall
}
helper_3()
{
// for inactive
}
...
if ()
helper_1();
else if ()
helper_2();
else
helper_3();
> > > > > + /* Time >= 1s, inactivity */
> > > > > + st->inact_time_ms = MILLI * val_int;
> > > > > +
> > > > > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val_int);
> > > > > }
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.