Re: [PATCH v6 4/8] iio: accel: adxl313: add activity sensing

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 22 Jun 2025 12:29:33 +0000
Lothar Rubusch <l.rubusch@gmail.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@gmail.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