Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 16 Apr 2022 17:47:57 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Jagath Jog J <jagathjog1996@gmail.com>
Cc:     dan@dlrobertson.com, andy.shevchenko@gmail.com,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 7/9] iio: accel: bma400: Add activity recognition
 support
Message-ID: <20220416174757.24346a41@jic23-huawei>
In-Reply-To: <20220411203133.19929-8-jagathjog1996@gmail.com>
References: <20220411203133.19929-1-jagathjog1996@gmail.com>
        <20220411203133.19929-8-jagathjog1996@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 12 Apr 2022 02:01:31 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Add support for activity recognition like STILL, WALKING, RUNNING
> and these events are pushed to the userspace whenever the STEP
> interrupt occurs.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
Hi Jagath,

Other than more questions on locking and a suggestion for some
code deduplication this looks good to me.

Jonathan

> ---
>  drivers/iio/accel/bma400_core.c | 104 ++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 37f38626a9aa..69d2caa4ed18 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -59,6 +59,12 @@ struct bma400_sample_freq {
>  	int uhz;
>  };
>  
> +enum bma400_activity {
> +	BMA400_STILL,
> +	BMA400_WALKING,
> +	BMA400_RUNNING,
> +};
> +
>  struct bma400_data {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -72,6 +78,7 @@ struct bma400_data {
>  	struct iio_trigger *trig;
>  	int steps_enabled;
>  	bool step_event_en;
> +	bool activity_event_en;
>  	/* Correct time stamp alignment */
>  	struct {
>  		__le16 buff[3];
> @@ -175,6 +182,12 @@ static const struct iio_event_spec bma400_step_detect_event = {
>  	.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>  };
>  
> +static const struct iio_event_spec bma400_activity_event = {
> +	.type = IIO_EV_TYPE_CHANGE,
> +	.dir = IIO_EV_DIR_NONE,
> +	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> +};
> +
>  #define BMA400_ACC_CHANNEL(_index, _axis) { \
>  	.type = IIO_ACCEL, \
>  	.modified = 1, \
> @@ -196,6 +209,16 @@ static const struct iio_event_spec bma400_step_detect_event = {
>  	},				\
>  }
>  
> +#define BMA400_ACTIVITY_CHANNEL(_chan2) {	\
> +	.type = IIO_ACTIVITY,			\
> +	.modified = 1,				\
> +	.channel2 = _chan2,			\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),	\
> +	.scan_index = -1, /* No buffer support */		\
> +	.event_spec = &bma400_activity_event,			\
> +	.num_event_specs = 1,					\
> +}
> +
>  static const struct iio_chan_spec bma400_channels[] = {
>  	BMA400_ACC_CHANNEL(0, X),
>  	BMA400_ACC_CHANNEL(1, Y),
> @@ -220,6 +243,9 @@ static const struct iio_chan_spec bma400_channels[] = {
>  		.event_spec = &bma400_step_detect_event,
>  		.num_event_specs = 1,
>  	},
> +	BMA400_ACTIVITY_CHANNEL(IIO_MOD_STILL),
> +	BMA400_ACTIVITY_CHANNEL(IIO_MOD_WALKING),
> +	BMA400_ACTIVITY_CHANNEL(IIO_MOD_RUNNING),
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> @@ -626,6 +652,20 @@ static void bma400_power_disable(void *data_ptr)
>  			 ERR_PTR(ret));
>  }
>  
> +static enum iio_modifier bma400_act_to_mod(enum bma400_activity activity)
> +{
> +	switch (activity) {
> +	case BMA400_STILL:
> +		return IIO_MOD_STILL;
> +	case BMA400_WALKING:
> +		return IIO_MOD_WALKING;
> +	case BMA400_RUNNING:
> +		return IIO_MOD_RUNNING;
> +	default:
> +		return IIO_NO_MOD;
> +	}
> +}
> +
>  static int bma400_init(struct bma400_data *data)
>  {
>  	unsigned int val;
> @@ -725,6 +765,7 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
>  	struct bma400_data *data = iio_priv(indio_dev);
>  	int ret;
>  	u8 steps_raw[3];
> +	unsigned int activity;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_PROCESSED:
> @@ -743,6 +784,23 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
>  				return ret;
>  			*val = get_unaligned_le24(steps_raw);
>  			return IIO_VAL_INT;
> +		case IIO_ACTIVITY:
> +			mutex_lock(&data->mutex);
> +			ret = regmap_read(data->regmap, BMA400_STEP_STAT_REG,
> +					  &activity);

More locking in here to think about. Not clear what you are protecting here.

> +			mutex_unlock(&data->mutex);
> +			if (ret)
> +				return ret;
> +			/*
> +			 * The device does not support confidence value levels,
> +			 * so we will always have 100% for current activity and
> +			 * 0% for the others.
I was thinking ahead when I came up with confidence for these. One day someone
will implement a sensor that is honest that it isn't that sure what the activity is.
:)  When they do we'll be ready!
> +			 */
> +			if (chan->channel2 == bma400_act_to_mod(activity))
> +				*val = 100;
> +			else
> +				*val = 0;
> +			return IIO_VAL_INT;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -898,6 +956,8 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
>  	switch (chan->type) {
>  	case IIO_STEPS:
>  		return data->step_event_en;
> +	case IIO_ACTIVITY:
> +		return data->activity_event_en;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -937,6 +997,32 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
>  			return ret;
>  		data->step_event_en = state;
>  		return 0;
> +	case IIO_ACTIVITY:
> +		if (!data->step_event_en) {
> +			mutex_lock(&data->mutex);
> +			ret = regmap_update_bits(data->regmap,
> +						 BMA400_INT_CONFIG1_REG,
> +						 BMA400_STEP_INT_MSK,
> +						 FIELD_PREP(BMA400_STEP_INT_MSK,
> +							    1));
> +			if (ret) {
> +				mutex_unlock(&data->mutex);
> +				return ret;
> +			}
> +			data->steps_enabled = 1;
> +
> +			ret = regmap_update_bits(data->regmap,
> +						 BMA400_INT12_MAP_REG,
> +						 BMA400_STEP_INT_MSK,
> +						 FIELD_PREP(BMA400_STEP_INT_MSK,
> +							    1));

This looks like some code that could be sensibly factored out in the earlier
patch to provide a function to call here rather than having another copy
of more or less the same code.

> +			mutex_unlock(&data->mutex);
> +			if (ret)
> +				return ret;
> +			data->step_event_en = 1;

I can understand logic of using lock to keep the internal tracking in sync
with the device, but here you are doing that outside the lock which doesn't seem
to do that.

> +		}
> +		data->activity_event_en = state;
> +		return 0;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1015,6 +1101,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
>  	s64 timestamp = iio_get_time_ns(indio_dev);
>  	int ret;
>  	__le16 status;
> +	unsigned int act;
>  
>  	mutex_lock(&data->mutex);
>  	ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
> @@ -1029,6 +1116,23 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
>  					      IIO_EV_DIR_NONE,
>  					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
>  			       timestamp);
> +
> +		if (data->activity_event_en) {
> +			mutex_lock(&data->mutex);
> +			ret = regmap_read(data->regmap, BMA400_STEP_STAT_REG,
> +					  &act);
> +			mutex_unlock(&data->mutex);
> +			if (ret)
> +				return IRQ_NONE;
> +
> +			iio_push_event(indio_dev,
> +				       IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> +						      bma400_act_to_mod(act),
> +						      IIO_EV_DIR_NONE,
> +						      IIO_EV_TYPE_CHANGE, 0,
> +						      0, 0),
> +				       timestamp);
> +		}
>  		return IRQ_HANDLED;
>  	}
>