| 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: <20250611175216.602d6a3f@jic23-huawei>
Date: Wed, 11 Jun 2025 17:52:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, 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 v4 08/11] iio: accel: adxl313: add inactivity sensing
On Wed, 11 Jun 2025 17:36:49 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:
> On Sun, Jun 1, 2025 at 9:46 PM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> >
> > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@...il.com> wrote:
> > >
> > > Extend the interrupt handler to process interrupts as inactivity events.
> > > Add functions to set threshold and period registers for inactivity. Add
> > > functions to enable / disable inactivity. Extend the fake iio channel to
> >
> > IIO
> >
> > > deal with inactivity events on x, y and z combined with AND.
> >
> > ...
> >
> > > +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> > > + unsigned int val_s)
> > > +{
> > > + unsigned int max_boundary = 255;
> >
> > This is unclear how it's defined. What is the limit behind? Size of a
> > bit field? Decimal value from the datasheet?
> >
> > The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
> > the answers to the above questions.
> >
> > > + unsigned int val = min(val_s, max_boundary);
> > > +
> > > + return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> > > +}
> >
> > ...
> >
> > > - axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > > + if (type == ADXL313_ACTIVITY)
> > > + axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> > > + else
> > > + axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
> >
> > Even with this change my previous comment stays.
> >
> > ...
> >
> > > + en = cmd_en && threshold;
> > > + if (type == ADXL313_INACTIVITY) {
> > > + ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + en = en && inact_time_s;
> > > + }
> >
> > ...
> >
> > > - if (info != IIO_EV_INFO_VALUE)
> > > - return -EINVAL;
> > > -
> > > - /* Scale factor 15.625 mg/LSB */
> > > - regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > > - switch (dir) {
> > > - case IIO_EV_DIR_RISING:
> > > - ret = regmap_write(data->regmap,
> > > - adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > > - regval);
> >
> > Hmm... This was added by the previous patches, right? Why can't it be
> > done as a switch case to begin with? I remember one of the previous
> > versions had some nested switch-cases, perhaps you need to rethink on
> > how to split the code between functions to avoid too much nesting (add
> > some helper functions?).
>
> The point here is, as I mentioned in the other mail:
> Initially, I wanted to build up the final switch/case struct i.e.
> going by MAG/MAG_ADAPTIVE, then INFO_VALUE -> RISING / FALLING and
> PERIOD.
>
> This will distinguish properties for four different types of events,
> of course it then also will use separate functions. As I uderstood
> your review, why starting with switch/case, do
> if (!MAG event) then, return right away. I implemented that as I
> understood. For further switch/case-ing, I did the same.
> Now, patch by patch, it grows. Thus the if-not-back-out lines will be
> moved out and replaced by switch/case. Worse, with every level switch
> case, all existing code needs indention, thus reading through the
> patches show (too) many changes.
>
> How can I improve to help you reviewing this or make the feedback more
> useful for me? Or is my approach wrong? I'd like to start with the
> switch case right away, then just add up what comes in with every
> other patch. If so, you'd only see the changes, since the final
> structure of this is already clear, because very similar to all
> iio/accel drivers at least (as you probably know better than me).
As mentioned earlier, reviewers tend to look at patches in a linear
fashion (and forget them more or less entirely between versions posted!)
So feel free to push back on earlier review comments that say 'you could
simplify this as X' with 'I did this because it becomes more complex in patch 4
and this reduces churn'.
Maybe here factoring out some elements into helpers will reduce the churn
anyway (to a couple of lines) and that discussion become unnecessary.
J
>
> >
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + /* Scale factor 15.625 mg/LSB */
> > > + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + ret = regmap_write(data->regmap,
> > > + adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> > > + regval);
> > > + if (ret)
> > > + return ret;
> > > + return adxl313_set_measure_en(data, true);
> > > + case IIO_EV_DIR_FALLING:
> > > + ret = regmap_write(data->regmap,
> > > + adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> > > + regval);
> > > + if (ret)
> > > + return ret;
> > > + return adxl313_set_measure_en(data, true);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + case IIO_EV_INFO_PERIOD:
> > > + ret = adxl313_set_inact_time_s(data, val);
> > > if (ret)
> > > return ret;
> > > return adxl313_set_measure_en(data, true);
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
Powered by blists - more mailing lists