Date:   Tue, 28 Dec 2021 20:58:05 +0000
From:   Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
To:     "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com>
Cc:     Cosmin Tanislav <demonsingur@gmail.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Hennerich, Michael" <Michael.Hennerich@analog.com>,
        Rob Herring <robh+dt@kernel.org>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] iio: accel: add ADXL367 driver
Message-ID: <20211228205757.7654cb66@jic23-huawei>
In-Reply-To: <edb634a17ba04f4cb5e77fa3b5c69358@analog.com>
References: <20211217114548.1659721-1-cosmin.tanislav@analog.com>
        <20211217114548.1659721-3-cosmin.tanislav@analog.com>
        <20211223130100.059231d6@jic23-huawei>
        <edb634a17ba04f4cb5e77fa3b5c69358@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Cosmin,

Happy New year for a few day's time.

> > ...
> >   
> > > +
> > > +static bool adxl367_push_event(struct iio_dev *indio_dev, u8 status)
> > > +{
> > > +	unsigned int ev_dir;
> > > +
> > > +	if (FIELD_GET(ADXL367_STATUS_ACT_MASK, status))
> > > +		ev_dir = IIO_EV_DIR_RISING;
> > > +	else if (FIELD_GET(ADXL367_STATUS_INACT_MASK, status))
> > > +		ev_dir = IIO_EV_DIR_FALLING;
> > > +	else
> > > +		return false;
> > > +
> > > +	iio_push_event(indio_dev,
> > > +		       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,  
> > IIO_MOD_X_OR_Y_OR_Z,  
> > > +					  IIO_EV_TYPE_THRESH, ev_dir),  
> > This is unusual for event detection as it's a simple or of separately
> > applied thresholds on X, Y and Z axes.  Given the effect of gravity that
> > means you have to set the thresholds very wide.
> > 
> > Also, I'd expect these to be magnitudes, not THRESH - no data sheet that
> > I can find though so can't be sure.
> >   
> 
> Actually, the chip has a referenced, and an absolute mode. We use reference mode
> in this driver, as configured in write_event_config.
> The motion detection details are about the same as ADXL362 (page 14).
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL362.pdf

Interesting.  We should figure out some way to make that clear to userspace
given right now it has no way of knowing that and might set inappropriate limits
without that information.

It's kind of similar to some of the adaptive thresholds, just that it uses
the value at a particular moment.

Worth noting that for the adxl362 at least the maths is
ABS(Acceleration - reference) > Threshold which is a magnitude not a threshold
unless you want to represent it as a pair of thresholds (above and below) which
gets fiddly as I assume there is only one control

> 
> 
> > > +		       iio_get_time_ns(indio_dev));
> > > +
> > > +	return true;
> > > +}

...

> > > +static int adxl367_write_event_config(struct iio_dev *indio_dev,
> > > +				      const struct iio_chan_spec *chan,
> > > +				      enum iio_event_type type,
> > > +				      enum iio_event_direction dir,
> > > +				      int state)
> > > +{
> > > +	struct adxl367_state *st = iio_priv(indio_dev);
> > > +	enum adxl367_activity_type act;
> > > +	int ret;
> > > +
> > > +	switch (dir) {
> > > +	case IIO_EV_DIR_RISING:
> > > +		act = ADXL367_ACTIVITY;
> > > +		break;
> > > +	case IIO_EV_DIR_FALLING:
> > > +		act = ADXL367_INACTIVITY;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = iio_device_claim_direct_mode(indio_dev);  
> > 
> > It's unusual (though not unheard of) to have events that cannot be enabled
> > at the same time as a fifo.  If that's true here, please add some comments
> > to explain why.  Or is this just about the impact of having to disable
> > the measurement to turn it on and the resulting interruption of data
> > capture?
> > 
> > If so that needs more thought as we have a situation where you can (I think)
> > have events as long as you enable them before the fifo based capture is
> > started,
> > but cannot enable them after.
> >   
> 
> That is indeed the case. You mentioned in a previous patchset that various
> attributes could toggle measurement mode while the FIFO capture was running,
> so I checked all the possible places where that could happen and added claim
> direct mode. Not too nice, but it's the nature of the chip...

Hmm. I'm not sure what the right thing to do here is. Maybe we need a docs update
to explicitly call out that this might happen for the event enables?  Calling
it out for all devices is fine because all we are doing is saying userspace would
ideally cope with this situation and make the decision to disable the buffered
mode if it wants to enable events then reenable it afterwards if that is what
is desired.

Jonathan