Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Lorenzo Bianconi <lorenzo@kernel.org>
Cc: Sean Nyekjaer <sean@geanix.com>,
	linux-iio@vger.kernel.org, lorenzo.bianconi83@gmail.com,
	denis.ciocca@st.com, martin@geanix.com
Subject: Re: [RFC PATCH 3/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold hook in sysfs
Date: Sun, 16 Jun 2019 14:42:02 +0100
Message-ID: <20190616144202.6ba14dbb@archlinux> (raw)
In-Reply-To: <20190615083557.GA5778@localhost.localdomain>

On Sat, 15 Jun 2019 10:35:58 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > This adds a wakeup threshold hook in sysfs, it enables us to
> > change the threshold value on the run.
> > For now this is the raw register value...
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 34 ++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >   
> 
> What about using write_event_config routine pointer for it instead od adding a sysfs
> entries?
> @Jonathan: what do you think?
I must admit this support has me a little confused.

Is what is actually going on here that we are implementing generic
threshold events, but instead of reporting them as interrupts and through the
usual userspace pipeline we are using a wakeup aware gpio to bring the
system out of suspend?

If that's the case then I'd like to see these supported as normal IIO events
first, and then look at adding the general ability to use these as wakeup
events if the particular interrupt line happens to support it.  This isn't
really a thing specific to this device, but rather something that we should
be enable on any threshold event.   Clearly for a driver to support it, there
must be a means of preventing it going too deeply to sleep so each
driver is likely to need some specific handling.

To get a remotely predictable interface we might need to have
enables more directly tied to the actual events than they would be
using the power/wakeup interface. That discussion would need to
involve the power management people as well. In particular I can see
you might want different events enabled when going to sleep than
at other times, so we might to have additional wakeup specific
enable attributes, and perhaps thresholds?

I can see something like:
/sys/bus/iio/devices/iio\:device0/events/in_accel_x_thresh_rising_[wakeup]value
/sys/bus/iio/devices/iio\:device0/events/in_accel_x_thresh_rising_wakeup

Making some sense.

Jonathan

> 
> Regards,
> Lorenzo
> 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > index 2c8ad7d65d2f..cbcd7920f05d 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -657,12 +657,45 @@ static ssize_t st_lsm6dsx_sysfs_set_wakeup_enabled(struct device *dev,
> >  	return len;
> >  }
> >  
> > +static ssize_t st_lsm6dsx_sysfs_get_wakeup_threshold(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    char *buf)
> > +{
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +
> > +	return sprintf(buf, "%d\n", hw->wake_threshold);
> > +}
> > +
> > +static ssize_t st_lsm6dsx_sysfs_set_wakeup_threshold(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    const char *buf, size_t len)
> > +{
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +	int threshold;
> > +
> > +	if (kstrtoint(buf, 0, &threshold))
> > +		return -EINVAL;
> > +
> > +	if ((threshold < 0) || (threshold > 31))
> > +		return -EINVAL;
> > +
> > +	if (!st_lsm6dsx_set_wake_threshold(hw, threshold))
> > +		return len;
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sysfs_sampling_frequency_avail);
> >  static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> >  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
> >  static IIO_DEVICE_ATTR(wakeup_enabled, 0644,
> >  		       st_lsm6dsx_sysfs_get_wakeup_enabled,
> >  		       st_lsm6dsx_sysfs_set_wakeup_enabled, 0);
> > +static IIO_DEVICE_ATTR(wakeup_threshold, 0644,
> > +		       st_lsm6dsx_sysfs_get_wakeup_threshold,
> > +		       st_lsm6dsx_sysfs_set_wakeup_threshold, 0);
> >  static IIO_DEVICE_ATTR(in_anglvel_scale_available, 0444,
> >  		       st_lsm6dsx_sysfs_scale_avail, NULL, 0);
> >  
> > @@ -670,6 +703,7 @@ static struct attribute *st_lsm6dsx_acc_attributes[] = {
> >  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> >  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> >  	&iio_dev_attr_wakeup_enabled.dev_attr.attr,
> > +	&iio_dev_attr_wakeup_threshold.dev_attr.attr,
> >  	NULL,
> >  };
> >  
> > -- 
> > 2.22.0
> >   


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-14 12:26 [RFC PATCH 0/3] io: imu: st_lsm6dsx: wake on acc event Sean Nyekjaer
2019-06-14 12:26 ` [RFC PATCH 1/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold Sean Nyekjaer
2019-06-15  8:30   ` Lorenzo Bianconi
2019-06-14 12:26 ` [RFC PATCH 2/3] iio: imu: st_lsm6dsx: add wake on accelerometer enable hook in sysfs Sean Nyekjaer
2019-06-15  8:38   ` Lorenzo Bianconi
2019-06-16 13:30   ` Jonathan Cameron
2019-06-17 16:29     ` Sean Nyekjaer
2019-06-14 12:26 ` [RFC PATCH 3/3] iio: imu: st_lsm6dsx: add wake on accelerometer threshold " Sean Nyekjaer
2019-06-15  8:35   ` Lorenzo Bianconi
2019-06-16 13:42     ` Jonathan Cameron [this message]
2019-06-16 13:24 ` [RFC PATCH 0/3] io: imu: st_lsm6dsx: wake on acc event Jonathan Cameron
2019-06-16 13:27   ` Jonathan Cameron

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190616144202.6ba14dbb@archlinux \
    --to=jic23@kernel.org \
    --cc=denis.ciocca@st.com \
    --cc=linux-iio@vger.kernel.org \
    --cc=lorenzo.bianconi83@gmail.com \
    --cc=lorenzo@kernel.org \
    --cc=martin@geanix.com \
    --cc=sean@geanix.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox