| 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: <20250628182706.2af83c1c@jic23-huawei>
Date: Sat, 28 Jun 2025 18:27:06 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
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 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
Jonathan
Powered by blists - more mailing lists