[PATCH v3 0/2] iio: acpi_als: Add sotfware trigger support

[PATCH v3 0/2] iio: acpi_als: Add sotfware trigger support

[Gwendal Grignou]

Some devices (chromebooks) present the ACPI ALS device but do not have
ability to notify when a new sample is present.
Add support for software trigger (hrtimer/sysfs-trigger) to allow
retrieving samples using iio buffers.

The first path fully adds timestamp channel, the next one adds trigger
support.

This patch assumes the patch "iio: set default trig->dev.parent" is
applied.

Fix a pointer error in v3.

Gwendal Grignou (2):
  iio: acpi_als: Add timestamp channel
  iio: acpi_als: Add trigger support

 drivers/iio/light/acpi-als.c | 103 ++++++++++++++++++++++++-----------
 1 file changed, 72 insertions(+), 31 deletions(-)

-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v3 1/2] iio: acpi_als: Add timestamp channel

[Gwendal Grignou]

Add timestamp channel in list of channel, to allow retrieving timestamps
when events are produced.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 Changes in v3: none.

 drivers/iio/light/acpi-als.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index 1eafd0b24e182..ff0ecec65fae4 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -45,24 +45,23 @@ static const struct iio_chan_spec acpi_als_channels[] = {
 		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
 					  BIT(IIO_CHAN_INFO_PROCESSED),
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
 };
 
 /*
  * The event buffer contains timestamp and all the data from
  * the ACPI0008 block. There are multiple, but so far we only
- * support _ALI (illuminance). Once someone adds new channels
- * to acpi_als_channels[], the evt_buffer below will grow
- * automatically.
+ * support _ALI (illuminance):
+ * One channel, paddind and timestamp.
  */
-#define ACPI_ALS_EVT_NR_SOURCES		ARRAY_SIZE(acpi_als_channels)
 #define ACPI_ALS_EVT_BUFFER_SIZE		\
-	(sizeof(s64) + (ACPI_ALS_EVT_NR_SOURCES * sizeof(s32)))
+	(sizeof(s32) + sizeof(s32) + sizeof(s64))
 
 struct acpi_als {
 	struct acpi_device	*device;
 	struct mutex		lock;
 
-	s32			evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE];
+	s32 evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE / sizeof(s32)]  __aligned(8);
 };
 
 /*
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v3 1/2] iio: acpi_als: Add timestamp channel

[Jonathan Cameron]

On Thu, 10 Dec 2020 14:15:40 -0800
Gwendal Grignou <gwendal@chromium.org> wrote:

> Add timestamp channel in list of channel, to allow retrieving timestamps
> when events are produced.

Giving IIO naming, events would be thresholds rather than buffered flow
that you are talking about here.

Not hugely important though.

> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  Changes in v3: none.
> 
>  drivers/iio/light/acpi-als.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index 1eafd0b24e182..ff0ecec65fae4 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -45,24 +45,23 @@ static const struct iio_chan_spec acpi_als_channels[] = {
>  		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>  					  BIT(IIO_CHAN_INFO_PROCESSED),
>  	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
>  };
>  
>  /*
>   * The event buffer contains timestamp and all the data from
>   * the ACPI0008 block. There are multiple, but so far we only
> - * support _ALI (illuminance). Once someone adds new channels
> - * to acpi_als_channels[], the evt_buffer below will grow
> - * automatically.
> + * support _ALI (illuminance):
> + * One channel, paddind and timestamp.

padding

>   */
> -#define ACPI_ALS_EVT_NR_SOURCES		ARRAY_SIZE(acpi_als_channels)
>  #define ACPI_ALS_EVT_BUFFER_SIZE		\
> -	(sizeof(s64) + (ACPI_ALS_EVT_NR_SOURCES * sizeof(s32)))
> +	(sizeof(s32) + sizeof(s32) + sizeof(s64))
>  
>  struct acpi_als {
>  	struct acpi_device	*device;
>  	struct mutex		lock;
>  
> -	s32			evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE];
> +	s32 evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE / sizeof(s32)]  __aligned(8);
>  };
>  
>  /*

[PATCH v3 2/2] iio: acpi_als: Add trigger support

[Gwendal Grignou]

As some firmware does not notify on illuminance changes, add a
trigger to be able to query light via software (sysfs-trigger or
hrtrigger).

BUG=b:172408337
TEST=Check iio_info reports the sensor as buffer capable:
iio:device0: acpi-als (buffer capable)
Check we can get data on demand on volteer:
echo 1 > iio_sysfs_trigger/add_trigger
cat trigger2/name > iio\:device0/trigger/current_trigger
for i in iio\:device0/scan_elements/*_en iio\:device0/buffer/enable ; do
  echo 1 > $i
done
od -x /dev/iio\:device0&
echo 1 > trigger2/trigger_now

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 Changes in v3:
 -- should not increase buffer pointer before call iio_push_buffer.

 drivers/iio/light/acpi-als.c | 92 ++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index ff0ecec65fae4..d506242eefabe 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -16,11 +16,15 @@
 #include <linux/module.h>
 #include <linux/acpi.h>
 #include <linux/err.h>
+#include <linux/irq.h>
 #include <linux/mutex.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/kfifo_buf.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #define ACPI_ALS_CLASS			"als"
 #define ACPI_ALS_DEVICE_NAME		"acpi-als"
@@ -60,6 +64,7 @@ static const struct iio_chan_spec acpi_als_channels[] = {
 struct acpi_als {
 	struct acpi_device	*device;
 	struct mutex		lock;
+	struct iio_trigger	*trig;
 
 	s32 evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE / sizeof(s32)]  __aligned(8);
 };
@@ -103,33 +108,20 @@ static void acpi_als_notify(struct acpi_device *device, u32 event)
 {
 	struct iio_dev *indio_dev = acpi_driver_data(device);
 	struct acpi_als *als = iio_priv(indio_dev);
-	s32 *buffer = als->evt_buffer;
-	s64 time_ns = iio_get_time_ns(indio_dev);
-	s32 val;
-	int ret;
-
-	mutex_lock(&als->lock);
 
-	memset(buffer, 0, ACPI_ALS_EVT_BUFFER_SIZE);
+	if (!iio_buffer_enabled(indio_dev) ||
+	    !iio_trigger_using_own(indio_dev))
+		return;
 
 	switch (event) {
 	case ACPI_ALS_NOTIFY_ILLUMINANCE:
-		ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
-		if (ret < 0)
-			goto out;
-		*buffer++ = val;
+		iio_trigger_poll_chained(als->trig);
 		break;
 	default:
 		/* Unhandled event */
 		dev_dbg(&device->dev, "Unhandled ACPI ALS event (%08x)!\n",
 			event);
-		goto out;
 	}
-
-	iio_push_to_buffers_with_timestamp(indio_dev, als->evt_buffer, time_ns);
-
-out:
-	mutex_unlock(&als->lock);
 }
 
 static int acpi_als_read_raw(struct iio_dev *indio_dev,
@@ -160,13 +152,46 @@ static const struct iio_info acpi_als_info = {
 	.read_raw		= acpi_als_read_raw,
 };
 
+static irqreturn_t acpi_als_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct acpi_als *als = iio_priv(indio_dev);
+	s32 *buffer = als->evt_buffer;
+	s32 val;
+	int ret;
+
+	mutex_lock(&als->lock);
+
+	ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
+	if (ret < 0)
+		goto out;
+	*buffer = val;
+
+	/*
+	 * when coming from own trigger via polls, set timestamp here.
+	 * Given ACPI notifier is already in a thread and call function directly,
+	 * there is no need to set the timestamp in the notify function.
+	 */
+	if (!pf->timestamp)
+		pf->timestamp = iio_get_time_ns(indio_dev);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
+out:
+	mutex_unlock(&als->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static int acpi_als_add(struct acpi_device *device)
 {
 	struct acpi_als *als;
 	struct iio_dev *indio_dev;
-	struct iio_buffer *buffer;
+	struct device *dev = &device->dev;
+	int ret;
 
-	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*als));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -177,19 +202,36 @@ static int acpi_als_add(struct acpi_device *device)
 	mutex_init(&als->lock);
 
 	indio_dev->name = ACPI_ALS_DEVICE_NAME;
-	indio_dev->dev.parent = &device->dev;
+	indio_dev->dev.parent = dev;
 	indio_dev->info = &acpi_als_info;
-	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
+	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = acpi_als_channels;
 	indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
 
-	buffer = devm_iio_kfifo_allocate(&device->dev);
-	if (!buffer)
+	als->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+					   indio_dev->name,
+					   indio_dev->id);
+	if (!als->trig)
 		return -ENOMEM;
 
-	iio_device_attach_buffer(indio_dev, buffer);
+	iio_trigger_set_drvdata(als->trig, indio_dev);
+	ret = devm_iio_trigger_register(dev, als->trig);
+	if (ret)
+		return ret;
+	/*
+	 * Set hardware trigger by default to let events flow when
+	 * BIOS support notification.
+	 */
+	indio_dev->trig = iio_trigger_get(als->trig);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      acpi_als_trigger_handler,
+					      NULL);
+	if (ret)
+		return ret;
 
-	return devm_iio_device_register(&device->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static const struct acpi_device_id acpi_als_device_ids[] = {
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v3 2/2] iio: acpi_als: Add trigger support

[Andy Shevchenko]

On Fri, Dec 11, 2020 at 12:15 AM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> As some firmware does not notify on illuminance changes, add a
> trigger to be able to query light via software (sysfs-trigger or
> hrtrigger).
>
> BUG=b:172408337
> TEST=Check iio_info reports the sensor as buffer capable:
> iio:device0: acpi-als (buffer capable)
> Check we can get data on demand on volteer:
> echo 1 > iio_sysfs_trigger/add_trigger
> cat trigger2/name > iio\:device0/trigger/current_trigger
> for i in iio\:device0/scan_elements/*_en iio\:device0/buffer/enable ; do
>   echo 1 > $i
> done
> od -x /dev/iio\:device0&

Hard to read. Also I recommend to indent a bit (couple of lead spaces,
for example)

> echo 1 > trigger2/trigger_now

...

> +       /*
> +        * when coming from own trigger via polls, set timestamp here.

Capitalize sentences.

from its own?
a timestamp

> +        * Given ACPI notifier is already in a thread and call function directly,
> +        * there is no need to set the timestamp in the notify function.
> +        */

...

> +       struct device *dev = &device->dev;

This is not related. Please, split the preparatory patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: acpi_als: Add trigger support

[Jonathan Cameron]

On Thu, 10 Dec 2020 14:15:41 -0800
Gwendal Grignou <gwendal@chromium.org> wrote:

> As some firmware does not notify on illuminance changes, add a
> trigger to be able to query light via software (sysfs-trigger or
> hrtrigger).
> 
> BUG=b:172408337
> TEST=Check iio_info reports the sensor as buffer capable:
> iio:device0: acpi-als (buffer capable)
> Check we can get data on demand on volteer:
> echo 1 > iio_sysfs_trigger/add_trigger
> cat trigger2/name > iio\:device0/trigger/current_trigger
> for i in iio\:device0/scan_elements/*_en iio\:device0/buffer/enable ; do
>   echo 1 > $i
> done
> od -x /dev/iio\:device0&
> echo 1 > trigger2/trigger_now
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Probably need a rebase on top of what Andy picked up on.

thanks,

Jonathan

> ---
>  Changes in v3:
>  -- should not increase buffer pointer before call iio_push_buffer.
> 
>  drivers/iio/light/acpi-als.c | 92 ++++++++++++++++++++++++++----------
>  1 file changed, 67 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index ff0ecec65fae4..d506242eefabe 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -16,11 +16,15 @@
>  #include <linux/module.h>
>  #include <linux/acpi.h>
>  #include <linux/err.h>
> +#include <linux/irq.h>
>  #include <linux/mutex.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #define ACPI_ALS_CLASS			"als"
>  #define ACPI_ALS_DEVICE_NAME		"acpi-als"
> @@ -60,6 +64,7 @@ static const struct iio_chan_spec acpi_als_channels[] = {
>  struct acpi_als {
>  	struct acpi_device	*device;
>  	struct mutex		lock;
> +	struct iio_trigger	*trig;
>  
>  	s32 evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE / sizeof(s32)]  __aligned(8);
>  };
> @@ -103,33 +108,20 @@ static void acpi_als_notify(struct acpi_device *device, u32 event)
>  {
>  	struct iio_dev *indio_dev = acpi_driver_data(device);
>  	struct acpi_als *als = iio_priv(indio_dev);
> -	s32 *buffer = als->evt_buffer;
> -	s64 time_ns = iio_get_time_ns(indio_dev);
> -	s32 val;
> -	int ret;
> -
> -	mutex_lock(&als->lock);
>  
> -	memset(buffer, 0, ACPI_ALS_EVT_BUFFER_SIZE);
> +	if (!iio_buffer_enabled(indio_dev) ||
> +	    !iio_trigger_using_own(indio_dev))
> +		return;
>  
>  	switch (event) {
>  	case ACPI_ALS_NOTIFY_ILLUMINANCE:
> -		ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
> -		if (ret < 0)
> -			goto out;
> -		*buffer++ = val;
> +		iio_trigger_poll_chained(als->trig);
>  		break;
>  	default:
>  		/* Unhandled event */
>  		dev_dbg(&device->dev, "Unhandled ACPI ALS event (%08x)!\n",
>  			event);
> -		goto out;
>  	}
> -
> -	iio_push_to_buffers_with_timestamp(indio_dev, als->evt_buffer, time_ns);
> -
> -out:
> -	mutex_unlock(&als->lock);
>  }
>  
>  static int acpi_als_read_raw(struct iio_dev *indio_dev,
> @@ -160,13 +152,46 @@ static const struct iio_info acpi_als_info = {
>  	.read_raw		= acpi_als_read_raw,
>  };
>  
> +static irqreturn_t acpi_als_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct acpi_als *als = iio_priv(indio_dev);
> +	s32 *buffer = als->evt_buffer;
> +	s32 val;
> +	int ret;
> +
> +	mutex_lock(&als->lock);
> +
> +	ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
> +	if (ret < 0)
> +		goto out;
> +	*buffer = val;
> +
> +	/*
> +	 * when coming from own trigger via polls, set timestamp here.
> +	 * Given ACPI notifier is already in a thread and call function directly,
> +	 * there is no need to set the timestamp in the notify function.
> +	 */
> +	if (!pf->timestamp)
> +		pf->timestamp = iio_get_time_ns(indio_dev);
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> +out:
> +	mutex_unlock(&als->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int acpi_als_add(struct acpi_device *device)
>  {
>  	struct acpi_als *als;
>  	struct iio_dev *indio_dev;
> -	struct iio_buffer *buffer;
> +	struct device *dev = &device->dev;
> +	int ret;
>  
> -	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*als));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> @@ -177,19 +202,36 @@ static int acpi_als_add(struct acpi_device *device)
>  	mutex_init(&als->lock);
>  
>  	indio_dev->name = ACPI_ALS_DEVICE_NAME;
> -	indio_dev->dev.parent = &device->dev;
> +	indio_dev->dev.parent = dev;

This isn't there in my togreg branch any more as the core does it.

>  	indio_dev->info = &acpi_als_info;
> -	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = acpi_als_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
>  
> -	buffer = devm_iio_kfifo_allocate(&device->dev);
> -	if (!buffer)
> +	als->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +					   indio_dev->name,
> +					   indio_dev->id);
> +	if (!als->trig)
>  		return -ENOMEM;
>  
> -	iio_device_attach_buffer(indio_dev, buffer);
> +	iio_trigger_set_drvdata(als->trig, indio_dev);
> +	ret = devm_iio_trigger_register(dev, als->trig);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Set hardware trigger by default to let events flow when
> +	 * BIOS support notification.
> +	 */
> +	indio_dev->trig = iio_trigger_get(als->trig);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      acpi_als_trigger_handler,
> +					      NULL);
> +	if (ret)
> +		return ret;
>  
> -	return devm_iio_device_register(&device->dev, indio_dev);
> +	return devm_iio_device_register(dev, indio_dev);
>  }
>  
>  static const struct acpi_device_id acpi_als_device_ids[] = {