linux-kernel - Re: [PATCH v10 3/7] 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: <CAFXKEHbGThKzMxg=aZMgVEZ2S2hUoGAOoE5wu_vCuzEPqL0+cA@mail.gmail.com>
Date: Mon, 23 Jun 2025 22:57:39 +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 v10 3/7] iio: accel: adxl345: add activity event feature

Hi Andy,

Thank you so much. I really appreciate your quick feedback. I'll try
to implement
the changes in another version, as far as needed.

Talking about the 80 characters, let me give an anser inlined down below.

On Mon, Jun 23, 2025 at 11:34 AM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> On Sun, Jun 22, 2025 at 03:50:06PM +0000, Lothar Rubusch wrote:
> > Enable the sensor to detect activity and trigger interrupts accordingly.
> > Activity events are determined based on a threshold, which is initialized
> > to a sensible default during probe. This default value is adopted from the
> > legacy ADXL345 input driver to maintain consistent behavior.
> >
> > The combination of activity detection, ODR configuration, and range
> > settings lays the groundwork for the activity/inactivity hysteresis
> > mechanism, which will be implemented in a subsequent patch. As such,
> > portions of this patch prepare switch-case structures to support those
> > upcoming changes.
>
> ...
>
> > +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > +                                 enum adxl345_activity_type type,
> > +                                 bool cmd_en)
> > +{
> > +     unsigned int axis_ctrl;
> > +     unsigned int threshold;
> > +     int ret;
> > +
> > +     if (cmd_en) {
> > +             /* When turning on, check if threshold is valid */
>
> > +             ret = regmap_read(st->regmap,
> > +                               adxl345_act_thresh_reg[type],
> > +                               &threshold);
>
> Can occupy less LoCs.
>
> > +             if (ret)
> > +                     return ret;
> > +
> > +             if (!threshold) /* Just ignore the command if threshold is 0 */
> > +                     return 0;
> > +     }
> > +
> > +     /* Start modifying configuration registers */
> > +     ret = adxl345_set_measure_en(st, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Enable axis according to the command */
> > +     switch (type) {
> > +     case ADXL345_ACTIVITY:
>
> > +             axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> > +                             ADXL345_ACT_Z_EN;
>
> I think
>
>                 axis_ctrl =
>                         ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
>
> is slightly better to read.
>

Agree.

> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = regmap_assign_bits(st->regmap, ADXL345_REG_ACT_INACT_CTRL,
> > +                              axis_ctrl, cmd_en);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Enable the interrupt line, according to the command */
> > +     ret = regmap_assign_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > +                              adxl345_act_int_reg[type], cmd_en);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return adxl345_set_measure_en(st, true);
> > +}
>
> ...
>
> > +     case IIO_EV_TYPE_MAG:
> > +             return adxl345_read_mag_config(st, dir,
> > +                                            ADXL345_ACTIVITY);
>
> It looks like you set the editor to wrap at 72 characters, but here the single
> line less than 80! Note that the limit is *exactly* 80 character.
>

I have my setup adjusted to 80 characters. Anyway, the cases here is
different, it needs
to be seen in context of the follow up patches. I tried to prepare the
patches now in a way
where changes are mostly "added". Is this correct and desired patch preparation?

In the particular case, this patch now adds ACTIVITY. A follow up
patch will add INACTIVITY.
Since this is still building up, it will add yet another argument to
those functions, i.e.
> > +             return adxl345_write_mag_config(st, dir,
> > +                                             ADXL345_ACTIVITY,

will become, later
> >               return adxl345_write_mag_config(st, dir,
> >                                               ADXL345_ACTIVITY,
> > +                                             ADXL345_INACTIVITY,

To make the change more additive, I did linebreaks earlier than 80
characters. Is this
legitimate in this case?

If so, I'll keep all related formatting as is (and will only change
the other requests).
Otherwise, I can do it differently and adopt all the formatting
changes to prioritize 80 characters.

Please let me know, what you think.
Best,
L


> ...
>
> > +     case IIO_EV_TYPE_MAG:
> > +             return adxl345_write_mag_config(st, dir,
> > +                                             ADXL345_ACTIVITY,
>
> Ditto.
>
> ...
>
> > +             return adxl345_read_mag_value(st, dir, info,
> > +                                           ADXL345_ACTIVITY,
> > +                                           val, val2);
>
> Ditto and so on...
>
> --
> With Best Regards,
> Andy Shevchenko
>
>