[PATCH RESEND 0/2] Solve data access delay of ST sensors

[PATCH RESEND 0/2] Solve data access delay of ST sensors

[Massimo Toscanelli]

Reading raw data from ST sensors implies enabling the device and
waiting for its activation.
This leads to significant delays every time the data is fetched,
especially if the operation has to be repeated.

The introduction of the 'always_on' flag as sysfs attribute can
solve this issue, by allowing the user to keep the device enabled
during the entire read session and therefore, to perform a much
faster data access.

This implementation has been already tested on a ST magnetometer.

I forgot to add maintainers as recepients, so I resend the patches.

Massimo Toscanelli (2):
  iio: st_sensors: add always_on flag
  iio: st_magn_core.c: activate always_on attribute

 .../iio/common/st_sensors/st_sensors_core.c   | 85 +++++++++++++++++--
 drivers/iio/magnetometer/st_magn_core.c       |  2 +
 include/linux/iio/common/st_sensors.h         | 14 +++
 3 files changed, 96 insertions(+), 5 deletions(-)


base-commit: dcb85f85fa6f142aae1fe86f399d4503d49f2b60
-- 
2.25.1

[PATCH 1/2] iio: st_sensors: add always_on flag

[Massimo Toscanelli]

The st_sensors_read_info_raw() implementation allows to get raw data
from st_sensors, enabling and disabling the device at every read.
This leads to delays in data access, caused by the msleep that waits
the hardware to be ready after every read.

Introduced always_on flag in st_sensor_data, to allow the user to
keep the device always enabled. In this way, every data access to the
device can be performed with no delays.

Add always_on sysfs attribute.

Signed-off-by: Massimo Toscanelli <massimo.toscanelli@leica-geosystems.com>
---
 .../iio/common/st_sensors/st_sensors_core.c   | 85 +++++++++++++++++--
 include/linux/iio/common/st_sensors.h         | 14 +++
 2 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index eb452d0c423c..5d16eab30853 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -378,6 +378,8 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
 	if (err < 0)
 		return err;
 
+	sdata->always_on = false;
+
 	/* Disable DRDY, this might be still be enabled after reboot. */
 	err = st_sensors_set_dataready_irq(indio_dev, false);
 	if (err < 0)
@@ -554,18 +556,21 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
 		err = -EBUSY;
 		goto out;
 	} else {
-		err = st_sensors_set_enable(indio_dev, true);
-		if (err < 0)
-			goto out;
+		if (!sdata->enabled) {
+			err = st_sensors_set_enable(indio_dev, true);
+			if (err < 0)
+				goto out;
 
-		msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
+			msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
+		}
 		err = st_sensors_read_axis_data(indio_dev, ch, val);
 		if (err < 0)
 			goto out;
 
 		*val = *val >> ch->scan_type.shift;
 
-		err = st_sensors_set_enable(indio_dev, false);
+		if (!sdata->always_on)
+			err = st_sensors_set_enable(indio_dev, false);
 	}
 out:
 	mutex_unlock(&indio_dev->mlock);
@@ -680,6 +685,76 @@ ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
 }
 EXPORT_SYMBOL(st_sensors_sysfs_scale_avail);
 
+/*
+ * st_sensors_sysfs_show_always_on() - get the value of the always_on flag.
+ *
+ * @dev: device reference.
+ * @attr: device attribute.
+ * @buf: sysfs buffer.
+ *
+ * Return: Number of bytes printed into the buffer
+ */
+ssize_t st_sensors_sysfs_show_always_on(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct st_sensor_data *sdata = iio_priv(indio_dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", sdata->always_on);
+}
+EXPORT_SYMBOL(st_sensors_sysfs_show_always_on);
+
+/*
+ * st_sensors_sysfs_store_always_on() - set/unset always_on flag.
+ *				       Accepted values are:
+ *				       - 1: to set the flag and keep the
+ *					    device always enabled.
+ *				       - 0: to unset the flag and enable the
+ *					    device just during data access.
+ *
+ * @dev: device reference.
+ * @attr: device attribute.
+ * @buf: sysfs buffer.
+ * @count: number of bytes used from the buffer.
+ *
+ * Return: Either the number of bytes used from the buffer or an error code.
+ */
+ssize_t st_sensors_sysfs_store_always_on(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct st_sensor_data *sdata = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	if (!!val == sdata->always_on)
+		return count;
+
+	sdata->always_on = !!val;
+	if (sdata->always_on)
+		ret = st_sensors_set_enable(indio_dev, true);
+	else
+		ret = st_sensors_set_enable(indio_dev, false);
+
+	if (ret < 0)
+		return ret;
+
+	if (sdata->always_on)
+		msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
+
+	return count;
+}
+EXPORT_SYMBOL(st_sensors_sysfs_store_always_on);
+
 MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
 MODULE_DESCRIPTION("STMicroelectronics ST-sensors core");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 22f67845cdd3..a4d4f374487d 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -82,6 +82,10 @@
 		IIO_DEVICE_ATTR(name, S_IRUGO, \
 			st_sensors_sysfs_scale_avail, NULL , 0);
 
+#define ST_SENSORS_DEV_ATTR_ALWAYS_ON() \
+		IIO_DEVICE_ATTR(always_on, 0644, st_sensors_sysfs_show_always_on, \
+				st_sensors_sysfs_store_always_on, 0)
+
 struct st_sensor_odr_avl {
 	unsigned int hz;
 	u8 value;
@@ -228,6 +232,7 @@ struct st_sensor_settings {
  * @vdd_io: Pointer to sensor's Vdd-IO power supply
  * @regmap: Pointer to specific sensor regmap configuration.
  * @enabled: Status of the sensor (false->off, true->on).
+ * @always_on: Flag to keep the sensor always enabled (false->off, true->on).
  * @odr: Output data rate of the sensor [Hz].
  * num_data_channels: Number of data channels used in buffer.
  * @drdy_int_pin: Redirect DRDY on pin 1 (1) or pin 2 (2).
@@ -248,6 +253,7 @@ struct st_sensor_data {
 	struct regmap *regmap;
 
 	bool enabled;
+	bool always_on;
 
 	unsigned int odr;
 	unsigned int num_data_channels;
@@ -318,6 +324,14 @@ ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
 
 void st_sensors_dev_name_probe(struct device *dev, char *name, int len);
 
+ssize_t st_sensors_sysfs_show_always_on(struct device *dev,
+					struct device_attribute *attr,
+					char *buf);
+
+ssize_t st_sensors_sysfs_store_always_on(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count);
+
 /* Accelerometer */
 const struct st_sensor_settings *st_accel_get_settings(const char *name);
 int st_accel_common_probe(struct iio_dev *indio_dev);
-- 
2.25.1

Re: [PATCH 1/2] iio: st_sensors: add always_on flag

[Linus Walleij]

On Mon, Feb 7, 2022 at 10:05 AM Massimo Toscanelli
<massimo.toscanelli@leica-geosystems.com> wrote:

> The st_sensors_read_info_raw() implementation allows to get raw data
> from st_sensors, enabling and disabling the device at every read.
> This leads to delays in data access, caused by the msleep that waits
> the hardware to be ready after every read.
>
> Introduced always_on flag in st_sensor_data, to allow the user to
> keep the device always enabled. In this way, every data access to the
> device can be performed with no delays.
>
> Add always_on sysfs attribute.
>
> Signed-off-by: Massimo Toscanelli <massimo.toscanelli@leica-geosystems.com>

This creates special dependencies on sysfs poking etc.

Couldn't the runtime PM solve this problem in a better way?

If you look in for example:
drivers/iio/accel/kxsd9.c
how the different pm_runtime* primitives are used, you get an
idea.

Especially note

        /*
         * Set autosuspend to two orders of magnitude larger than the
         * start-up time. 20ms start-up time means 2000ms autosuspend,
         * i.e. 2 seconds.
         */
        pm_runtime_set_autosuspend_delay(dev, 2000);

This creates a "hysteresis window" around when the device is
on, so it is not repeatedly shut off and on, but only after 2 seconds
of inactivity.

This way no special userspace is needed to achieve what you want,
and it benefits everyone.

I wanted to fix this for all the ST sensors but never got around to.

Yours,
Linus Walleij

[PATCH 2/2] iio: st_magn_core.c: activate always_on attribute

[Massimo Toscanelli]

Activate always_on sysfs attribute to add the always_on feature in
st magnetometers.

Signed-off-by: Massimo Toscanelli <massimo.toscanelli@leica-geosystems.com>
---
 drivers/iio/magnetometer/st_magn_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index 0806a1e65ce4..2ab4c286d262 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -563,9 +563,11 @@ static int st_magn_write_raw(struct iio_dev *indio_dev,
 static ST_SENSORS_DEV_ATTR_SAMP_FREQ_AVAIL();
 static ST_SENSORS_DEV_ATTR_SCALE_AVAIL(in_magn_scale_available);
 
+static ST_SENSORS_DEV_ATTR_ALWAYS_ON();
 static struct attribute *st_magn_attributes[] = {
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	&iio_dev_attr_in_magn_scale_available.dev_attr.attr,
+	&iio_dev_attr_always_on.dev_attr.attr,
 	NULL,
 };
 
-- 
2.25.1

Re: [PATCH RESEND 0/2] Solve data access delay of ST sensors

[Jonathan Cameron]

On Mon,  7 Feb 2022 09:04:41 +0000
Massimo Toscanelli <massimo.toscanelli@leica-geosystems.com> wrote:

> Reading raw data from ST sensors implies enabling the device and
> waiting for its activation.
> This leads to significant delays every time the data is fetched,
> especially if the operation has to be repeated.
> 
> The introduction of the 'always_on' flag as sysfs attribute can
> solve this issue, by allowing the user to keep the device enabled
> during the entire read session and therefore, to perform a much
> faster data access.
> 
> This implementation has been already tested on a ST magnetometer.
> 
> I forgot to add maintainers as recepients, so I resend the patches.
> 
> Massimo Toscanelli (2):
>   iio: st_sensors: add always_on flag
>   iio: st_magn_core.c: activate always_on attribute
> 
>  .../iio/common/st_sensors/st_sensors_core.c   | 85 +++++++++++++++++--
>  drivers/iio/magnetometer/st_magn_core.c       |  2 +
>  include/linux/iio/common/st_sensors.h         | 14 +++
>  3 files changed, 96 insertions(+), 5 deletions(-)
> 
> 
> base-commit: dcb85f85fa6f142aae1fe86f399d4503d49f2b60

Hi Massimo,

The standard approach to avoiding rapid power up and power down cycles
is to use runtime_pm with autosuspend and a period set at a period
of perhaps 1 second.  Would that work for you?  You'll pay the costs
of power up only on the first read after a period of not reading.

Exposing the control to userspace for this sort of thing is normally
a bad idea because userspace generally has no idea if it should use it
as there is no clean way of telling userspace the costs of not using
it (as those will be device specific).

If you have a usecase that requires regular reading you should also
think about whether using the buffered interface is more appropriate.
IIRC that will keep these devices powered up continuously whilst
the buffer is enabled and will provide a lower overhead way of
reading data than sysfs reads.

Jonathan

Re: [PATCH RESEND 0/2] Solve data access delay of ST sensors

[Linus Walleij]

On Mon, Feb 7, 2022 at 2:59 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Mon,  7 Feb 2022 09:04:41 +0000
> Massimo Toscanelli <massimo.toscanelli@leica-geosystems.com> wrote:

> The standard approach to avoiding rapid power up and power down cycles
> is to use runtime_pm with autosuspend and a period set at a period
> of perhaps 1 second.  Would that work for you?  You'll pay the costs
> of power up only on the first read after a period of not reading.
>
> Exposing the control to userspace for this sort of thing is normally
> a bad idea because userspace generally has no idea if it should use it
> as there is no clean way of telling userspace the costs of not using
> it (as those will be device specific).
>
> If you have a usecase that requires regular reading you should also
> think about whether using the buffered interface is more appropriate.
> IIRC that will keep these devices powered up continuously whilst
> the buffer is enabled and will provide a lower overhead way of
> reading data than sysfs reads.

I see that I have repeted similar comments.

Sorry for the hammering.

I agree with Jonathan's stance here.

Yours,
Linus Walleij

RE: [PATCH RESEND 0/2] Solve data access delay of ST sensors

[TOSCANELLI Massimo]

Hello Linus and Jonathan

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: 11 February 2022 02:12
> To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: TOSCANELLI Massimo <massimo.toscanelli@leica-geosystems.com>;
> linux-kernel@vger.kernel.org; jic23@kernel.org; lars@metafoo.de;
> caihuoqing@baidu.com; aardelean@deviqon.com;
> andy.shevchenko@gmail.com; hdegoede@redhat.com; LI Qingwu <qing-
> wu.li@leica-geosystems.com.cn>; stephan@gerhold.net; linux-
> iio@vger.kernel.org; GEO-CHHER-bsp-development <bsp-
> development.geo@leica-geosystems.com>
> Subject: Re: [PATCH RESEND 0/2] Solve data access delay of ST sensors
> 
> 
> On Mon, Feb 7, 2022 at 2:59 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > On Mon,  7 Feb 2022 09:04:41 +0000
> > Massimo Toscanelli <massimo.toscanelli@leica-geosystems.com> wrote:
> 
> > The standard approach to avoiding rapid power up and power down cycles
> > is to use runtime_pm with autosuspend and a period set at a period of
> > perhaps 1 second.  Would that work for you?  You'll pay the costs of
> > power up only on the first read after a period of not reading.
> >
> > Exposing the control to userspace for this sort of thing is normally a
> > bad idea because userspace generally has no idea if it should use it
> > as there is no clean way of telling userspace the costs of not using
> > it (as those will be device specific).
> >
> > If you have a usecase that requires regular reading you should also
> > think about whether using the buffered interface is more appropriate.
> > IIRC that will keep these devices powered up continuously whilst the
> > buffer is enabled and will provide a lower overhead way of reading
> > data than sysfs reads.
> 
> I see that I have repeted similar comments.
> 
Thanks a lot for your suggestions, I decided to go for the second solution proposed by Jonathan.
Using the buffered interface should be enough, you can discard the patches.

Massimo