Hi Andy,
[...]
> ...
>
> > +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).
>
What is the matter with the indention here? I'm doing `checkpatch.pl
--strict --codespell` on that and don't get an issue? Would you expect
cases like the FIELD_GET() calls on the next line, linked by a binary
OR, to be indented by yet another TAB?
Best,
L
> > + 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
>
>
