> ...
>
> > if (type == ADXL345_ACTIVITY) {
> > axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> > ADXL345_ACT_Z_EN;
> > } else {
> > - axis_ctrl = 0x00;
> > + axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> > + ADXL345_INACT_Z_EN;
> > }
>
> Now this can be as simple as
>
> axis_ctrl = ADXL345_ACT_X_EN;
That flag is only set in the activity case. Confused with ADXL345_INACT_X_EN?
(initially I thought you'd run into a bug!)
> if (type == ADXL345_ACTIVITY)
> axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> else
> axis_ctrl |= ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;
>
> Yeah, I don't know how to make the diff better (it gets worse), but the end
> result is better.
>
> One way, which I don't like much is to previously have this conditional written as:
>
> axis_ctrl = ADXL345_ACT_X_EN;
> if (type == ADXL345_ACTIVITY)
> axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> else
> axis_ctrl = 0;
>
> ...
>
> > + ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> > + (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),
>
> Unneeded parentheses.
>
> > + en);
> > if (ret)
> > return ret;
>
> ...
>
> > static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> > {
> > - return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> > + int ret;
> > +
> > + ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> > ADXL345_BW_RATE_MSK,
> > FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
> > + if (ret)
> > + return ret;
> > +
> > + /* update inactivity time by ODR */
> > + return adxl345_set_inact_time(st, 0);
>
> Okay, in this case the initial form of
>
> int ret;
>
> ret = ...
> if (ret)
> return ret;
>
> return 0;
>
>
> will be better with the respectful comment (as Jonathan suggested) in that
> change that this is not optimal as standalone change, but it will help reduce
> churn in the next change(s).
>
Worth noting we (reviewers) may well forget about this and moan on
some future version. If we do, feel free to remind us!
