Re: [PATCH v9 07/11] iio: accel: adxl345: add activity event feature

[Lothar Rubusch]

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, &regval);
> > +     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
>
>