Re: [PATCH v1 09/12] iio: accel: adxl313: add activity sensing

[Andy Shevchenko <andy@kernel.org>]

On Sun, May 18, 2025 at 11:13:18AM +0000, Lothar Rubusch wrote:
> Add possibilities to set a threshold for activity sensing. Extend the
> interrupt handler to process activity interrupts. Provide functions to set
> the activity threshold and to enable/disable activity sensing. Further add
> a fake channel for having x, y and z axis anded on the iio channel.
> 
> This is a preparatory patch. Some of the definitions and functions are
> supposed to be extended for inactivity later on.

...

> +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> +				   enum adxl313_activity_type type,
> +				   bool *en)
> +{
> +	unsigned int axis_ctrl;
> +	unsigned int regval;
> +	int ret;

> +	*en = false;

Even in case of an error? The rule of thumb is to avoid assigning output when
we know that the error will be returned to the caller.

> +	ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> +	if (ret)
> +		return ret;

> +	if (type == ADXL313_ACTIVITY)
> +		*en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +
> +	if (*en) {

This doesn't need to re-write the value of *en. Just declare local boolean
temporary variable and use it and only assign it on success.

> +		ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> +		if (ret)
> +			return ret;
> +
> +		*en = adxl313_act_int_reg[type] & regval;
> +	}
> +
> +	return 0;
> +}

...

> +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> +				    enum adxl313_activity_type type,
> +				    bool cmd_en)
> +{
> +	unsigned int axis_ctrl = 0;
> +	unsigned int threshold;
> +	bool en;
> +	int ret;
> +
> +	if (type == ADXL313_ACTIVITY)
> +		axis_ctrl = ADXL313_ACT_XYZ_EN;
> +
> +	ret = regmap_update_bits(data->regmap,
> +				 ADXL313_REG_ACT_INACT_CTL,
> +				 axis_ctrl,
> +				 cmd_en ? 0xff : 0x00);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> +	if (ret)
> +		return ret;

> +	en = false;

Instead...

> +	if (type == ADXL313_ACTIVITY)
> +		en = cmd_en && threshold;

	else
		en = false;

> +	return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> +				  adxl313_act_int_reg[type],
> +				  en ? adxl313_act_int_reg[type] : 0);
> +}

...

> +static int adxl313_read_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir)
> +{
> +	struct adxl313_data *data = iio_priv(indio_dev);

> +	bool int_en;

Why? You return the int here... I would expect rather to see unsigned int...

> +	int ret;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = adxl313_is_act_inact_en(data,
> +						      ADXL313_ACTIVITY,
> +						      &int_en);
> +			if (ret)
> +				return ret;
> +			return int_en;

...or even simply

			return adx1313...(...);

> +		default:
> +			return -EINVAL;
> +		}
> +	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);
> +	unsigned int act_threshold;
> +	int ret;
> +
> +	/* measurement stays enabled, reading from regmap cache */
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		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 = 1000000;

MICRO?

> +				return IIO_VAL_FRACTIONAL;
> +			default:
> +				return -EINVAL;
> +			}
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	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);
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = adxl313_set_measure_en(data, false);
> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:

This can be collapsed to the conditional, making indentation better overall.
Same applies to the other parts of the code outside of this function.

> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			/* The scale factor is 15.625 mg/LSB */
> +			regval = DIV_ROUND_CLOSEST(1000000 * val + val2, 15625);

MICRO?

> +			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);
> +			default:
> +				return -EINVAL;
> +			}
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +		ret = regmap_write(data->regmap, ADXL313_REG_ACT_INACT_CTL, 0);

0x00 ?

> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, ADXL313_REG_THRESH_ACT, 0x52);
> +		if (ret)
> +			return ret;

-- 
With Best Regards,
Andy Shevchenko