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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFXKEHbdeomMfEBwO+Cvkn5dkN4h47CEAMfmEGQC2V82zQ+U8Q@mail.gmail.com>
Date: Sat, 21 Jun 2025 20:06:49 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org, 
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, corbet@....net, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v9 07/11] iio: accel: adxl345: add activity event feature

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
>
>