| 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: <CAFXKEHaYdwvi64+yBEiYuv62Vaa8exYFji2gBXQciJ=CWdhbsw@mail.gmail.com>
Date: Tue, 1 Jul 2025 00:32:57 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: 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 v6 4/8] iio: accel: adxl313: add activity sensing
On Sat, Jun 28, 2025 at 7:27 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sun, 22 Jun 2025 12:29:33 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Add support for configuring an activity detection threshold. Extend the
> > interrupt handler to process activity-related interrupts, and provide
> > functions to set the threshold as well as to enable or disable activity
> > sensing. Additionally, introduce a virtual channel that represents the
> > logical AND of the x, y, and z axes in the IIO channel.
> >
> > This patch serves as a preparatory step; some definitions and functions
> > introduced here are intended to be extended later to support inactivity
> > detection.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> Hi Lothar.
>
> I think this is suffering from function naming evolution and we need
> to rethink (slightly) what we ended up with. See inline.
> I walked into the same trap recently so was on the look out for it.
>
> > ---
> > drivers/iio/accel/adxl313_core.c | 326 +++++++++++++++++++++++++++++++
> > 1 file changed, 326 insertions(+)
> >
> > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > index ac4cc16399fc..d2c625f27555 100644
> > --- a/drivers/iio/accel/adxl313_core.c
> > +++ b/drivers/iio/accel/adxl313_core.c
> > @@ -13,8 +13,10 @@
>
> > +
> > +static int _adxl313_read_mag_value(struct adxl313_data *data,
> > + enum iio_event_direction dir,
> > + enum adxl313_activity_type type_act,
> > + int *val, int *val2)
> > +{
> > + unsigned int threshold;
> > + int ret;
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + ret = regmap_read(data->regmap,
> > + adxl313_act_thresh_reg[type_act],
> > + &threshold);
> > + if (ret)
> > + return ret;
> > + *val = threshold * 15625;
> > + *val2 = MICRO;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int _adxl313_write_mag_value(struct adxl313_data *data,
> > + enum iio_event_direction dir,
> > + enum adxl313_activity_type type_act,
> > + int val, int val2)
> > +{
> > + unsigned int regval;
> > +
> > + /* Scale factor 15.625 mg/LSB */
> > + regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return regmap_write(data->regmap,
> > + adxl313_act_thresh_reg[type_act],
> > + regval);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_read_mag_value(struct adxl313_data *data,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + enum adxl313_activity_type type_act,
> > + int *val, int *val2)
> > +{
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + return _adxl313_read_mag_value(data, dir,
>
> Same issue as below for read functions.
>
> > + type_act,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_write_mag_value(struct adxl313_data *data,
>
> This has me a little confused. It's called
> adxl313_write_mag_value() which seems pretty specific but
> then calls another level of _adxl313_write_mag_value.
>
> In the next patch (assuming diff isn't leading me astray) we have
>
> @@ -600,13 +687,17 @@ static int adxl313_write_mag_value(struct adxl313_data *data,
> enum iio_event_direction dir,
> enum iio_event_info info,
> enum adxl313_activity_type type_act,
> + enum adxl313_activity_type type_inact,
> int val, int val2)
> {
> switch (info) {
> case IIO_EV_INFO_VALUE:
> return _adxl313_write_mag_value(data, dir,
> type_act,
> + type_inact,
> val, val2);
> + case IIO_EV_INFO_PERIOD:
> + return adxl313_set_inact_time_s(data, val);
> default:
> return -EINVAL;
> }
>
>
> Which is adding PERIOD to something called write_mag_value()
>
> Whilst I can see why you ended up with naming as:
>
> adxl313_write_mag_value() as the magnitude event specific part of
> adxl13_event_write_value()
>
> and indeed
>
> _adxl313_write_mag_value() as the thing that writes IIO_EV_INFO_VALUE
> value (i.e. the threshold) for the magnitude events.
>
> Last time I hit a similar naming stack, I spinkled in some _info
>
> So have the inner one called something slightly more specific like
>
> adxl313_write_mag_info_value()
>
>
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + enum adxl313_activity_type type_act,
> > + int val, int val2)
> > +{
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + return _adxl313_write_mag_value(data, dir,
> > + type_act,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct adxl313_data *data = iio_priv(indio_dev);
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_MAG:
> > + return adxl313_read_mag_value(data, dir, info,
> > + ADXL313_ACTIVITY,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int adxl313_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct adxl313_data *data = iio_priv(indio_dev);
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_MAG:
> > + return adxl313_write_mag_value(data, dir, info,
> > + ADXL313_ACTIVITY,
> > + val, val2);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
>
> Otherwise LGTM
>
Hi, I'm about to wrap this up for the final version (let's see...).
I understand that three levels of switch/case are not good. Instead
here I did a particular function/helper per switch/case level.
Finally, I ended up with, e.g.
adxl313_write_event_value() // calls
\-> adxl313_write_mag_value() // calls
\-> _adxl313_write_mag_value()
Personally, I think, why not just having the following calls hierarchy:
adxl313_write_event_value() // calls
\-> adxl313_write_mag_value()
First question: Regarding the adxl345 driver, with a little higher
level of complexity, I adopted such a solution keeping still 2 levels
of switch case inside the helper. Would this be ok for the ADXL313,
too? I mean, having just one helper, but that one containing one level
of nested switch case inside a switch/case?
Another question about the naming of the helper. As you saw, I went
"creative" and used the commonly used name for such functions
replacing "_event_" by "_mag_". I see this can be confusing, but also
it might make clear where the (only locally used) helper belongs to.
I understand names with leading '_' are not likely to be a decent
choice here. But in general in case of adxl313_write_mag_value() -like
names. What would be a better name for it, or would it be ok?
By the answers given to the above, and if you don't object I would
like to prepare the single level of helper approach (then having one
nested switch/case) and keep just the adxl313_*_mag_value() or
..._config() functions. Let me know what you think.
> Jonathan
Powered by blists - more mailing lists