| 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
| ||
|
Message-ID: <CAHp75VeDj1sE55_PdR6=QHAT-Nv0fwv3kss=7oeh8isdmOMoOw@mail.gmail.com>
Date: Tue, 17 Jun 2025 14:54:35 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
andy@...nel.org, corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de,
Michael.Hennerich@...log.com, bagasdotme@...il.com, linux-iio@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/8] iio: accel: adxl313: add inactivity sensing
On Tue, Jun 17, 2025 at 1:10 PM Lothar Rubusch <l.rubusch@...il.com> wrote:
> On Mon, Jun 16, 2025 at 12:59 PM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> > On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@...il.com> wrote:
[...]
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + ret = regmap_read(data->regmap,
> > > + adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > > + &act_threshold);
> > > + if (ret)
> > > + return ret;
> > > + *val = act_threshold * 15625;
> > > + *val2 = MICRO;
> > > + return IIO_VAL_FRACTIONAL;
> > > + case IIO_EV_DIR_FALLING:
> > > + ret = regmap_read(data->regmap,
> > > + adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> > > + &inact_threshold);
> > > + if (ret)
> > > + return ret;
> >
> > > + *val = inact_threshold * 15625;
> > > + *val2 = MICRO;
> > > + return IIO_VAL_FRACTIONAL;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + case IIO_EV_INFO_PERIOD:
> > > ret = regmap_read(data->regmap,
> > > - adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > > - &act_threshold);
> > > + ADXL313_REG_TIME_INACT,
> > > + &inact_time_s);
> > > if (ret)
> > > return ret;
> > > - *val = act_threshold * 15625;
> > > - *val2 = MICRO;
> > > - return IIO_VAL_FRACTIONAL;
> > > + *val = inact_time_s;
> > > + return IIO_VAL_INT;
> > > default:
> > > return -EINVAL;
> > > }
> >
> > I still don't get what's wrong with helpers for nested switches?
> > Instead of doing ping-pong with so many lines (due to indentation
> > changes), just create a helper from the beginning. In this case this
> > will look more like
> >
> > if (nfo == IIO_EV_INFO_VALUE)
> > return my_cool_helper_for_THIS_case(...);
> >
> > Note, I admit that not all the cases may be done like this, but just
> > look at this again and perhaps something can be optimised.
>
> First, about helpers dealing with nested switches. The resulting
> structure then is like
>
> switch (type) {
> case IIO_EV_TYPE_MAG:
> switch (info) {
> case IIO_EV_INFO_VALUE:
> switch (dir) {
> case IIO_EV_DIR_RISING: // activity
> ....
> case IIO_EV_DIR_FALLING: // inactivity
> ....
> }
> case IIO_EV_INFO_PERIOD:
> ...
> }
> case IIO_EV_TYPE_MAG_ADAPTIVE:
> // same as above, again for _AC events
> ...
> }
>
> Actually I'm using a helper for nested switches. But currently I'm
> adding it quite late, when I have cases for ACTIVITY, INACTIVITY and
> ACTIVITY_AC and INACTIVITY_AC, since this results in code duplication.
> The resulting structure the looks as follows.
>
> helper_mag(...)
> {
> switch (info) {
> case IIO_EV_INFO_VALUE:
> switch (dir) {
> case IIO_EV_DIR_RISING: // activity
> ....
> case IIO_EV_DIR_FALLING: // inactivity
> ....
> }
> case IIO_EV_INFO_PERIOD:
> ...
> }
> }
>
> switch (type) {
> case IIO_EV_TYPE_MAG:
> helper_mag(...);
> case IIO_EV_TYPE_MAG_ADAPTIVE:
> // same as above, again for _AC events
> helper_mag(...);
> }
>
> Is this reasonable? For the v6 now, I plan on introducing this helper
> when adding INACTIVITY sensing, having ACTIVITY already in place. This
> is, because INACTIVITY as distinguishable type is still not available,
> but would be needed as function argument as well. Does this make
> sense? Or, should I start with the helper right at the beginning? Is
> it ok to have once a nested switch in the helper?
Yes, that's what even would propose here, is to make a helper with the
current code (if it's not empty) and then fill it with the content. In
any case try and see.
The (end) idea is to have only one level of the switch-case per
function in this case and use helpers for the inner ones:
foo()
{
if (...)
return -EINVAL;
switch (dir) {
case BAZ:
...
break;
}
}
switch (type) {
case FOO:
return _do_foo();
}
> Second question is about the adxl313_read_event_config() functions,
> I'd like to have here 0 or 1 in regular cases (<0 for error). Is it ok
> if I adjust the functions slightly to guarantee this? Currently it
> generally returns >0 in cases of "true" which is correct. But this is
> most of the times 1, in some cases can be 8 or something. I just like
> it to be uniform for testing (which is not a valid argumentation). Is
> this legitimate?
If this is an ABI, better to unify this to have the same meaning of
each of the returned values independently on the functions, if this is
just an internal, who cares? However, there is, of course, a corner
case if MSB is set and you will return a (positive) value as an error
code. So, just look at the functions and decide which path you take.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists