[PATCH v7 0/3] iio: acpi_als: Add sotfware trigger support

[PATCH v7 0/3] 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 second adds a local
variable in probe routine and the last one adds trigger support.

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

Changes since v7:
  rebase on iio git testing branch.

Gwendal Grignou (3):
  iio: acpi_als: Add timestamp channel
  iio: acpi_als: Add local variable dev in probe
  iio: acpi_als: Add trigger support

 drivers/iio/light/acpi-als.c | 113 +++++++++++++++++++++++------------
 1 file changed, 76 insertions(+), 37 deletions(-)

-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH v7 1/3] 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 No changes in v7
 No changes in v6
 Changes in v5:
 Fill comment line.
 Changes in v4:
 Fix spelling.

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

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index f8e547fd35e7a..784868aa8fbf7 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -45,24 +45,22 @@ 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, padding 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.31.0.rc2.261.g7f71774620-goog

[PATCH v7 2/3] iio: acpi_als: Add local variable dev in probe

[Gwendal Grignou]

Use dev = &device->dev in probe routine for clarity.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 Changes in v7:
 Rebased, replace &device->dev at new locations.
 No changes in v6.
 Changes in v5:
 Follow inverted xmas tree declaration convention.
 Added in v4.

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

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index 784868aa8fbf7..a2107945cff93 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -161,11 +161,12 @@ static const struct iio_info acpi_als_info = {
 
 static int acpi_als_add(struct acpi_device *device)
 {
-	struct acpi_als *als;
+	struct device *dev = &device->dev;
 	struct iio_dev *indio_dev;
+	struct acpi_als *als;
 	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;
 
@@ -180,12 +181,12 @@ static int acpi_als_add(struct acpi_device *device)
 	indio_dev->channels = acpi_als_channels;
 	indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
 
-	ret = devm_iio_kfifo_buffer_setup(&device->dev, indio_dev,
+	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
 					  INDIO_BUFFER_SOFTWARE, 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.31.0.rc2.261.g7f71774620-goog

[PATCH v7 3/3] 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).
Add a hardware trigger set as the default trigger to maintain backward
compatibility.

Check iio_info reports the sensor as buffer capable:
  iio:device0: acpi-als (buffer capable)

To test, check we can get data on demand on an Intel based chromebook:

  IIO_DEV="iio:device0"
  echo 1 > iio_sysfs_trigger/add_trigger
  cat trigger2/name > ${IIO_DEV}/trigger/current_trigger
  for i in ${IIO_DEV}/scan_elements/*_en ${IIO_DEV}/buffer/enable ; do
    echo 1 > $i
  done
  od -x /dev/${IIO_DEV} &
  echo 1 > trigger2/trigger_now

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 Changes in v7:
 Rebased : no need to set indio_dev->modes directly anymore.
 Changes in v6:
 Inverted logic in acpi_als_notify()
 Put timestamp comments in the same paragraph.
 Changes in v5:
 Improve commit message readability, add note about backward
 compatibility.
 Remove unneeded include file.
 Changes in v4:
 Added comments when pf->timestamp is 0 and valid. Fix spelling.

 drivers/iio/light/acpi-als.c | 96 +++++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index a2107945cff93..30393f08e0825 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -16,11 +16,14 @@
 #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"
@@ -59,6 +62,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);
 };
@@ -102,33 +106,19 @@ 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);
-
-	switch (event) {
-	case ACPI_ALS_NOTIFY_ILLUMINANCE:
-		ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
-		if (ret < 0)
-			goto out;
-		*buffer++ = val;
-		break;
-	default:
-		/* Unhandled event */
-		dev_dbg(&device->dev, "Unhandled ACPI ALS event (%08x)!\n",
-			event);
-		goto out;
+	if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev)) {
+		switch (event) {
+		case ACPI_ALS_NOTIFY_ILLUMINANCE:
+			iio_trigger_poll_chained(als->trig);
+			break;
+		default:
+			/* Unhandled event */
+			dev_dbg(&device->dev,
+				"Unhandled ACPI ALS event (%08x)!\n",
+				event);
+		}
 	}
-
-	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,
@@ -159,6 +149,41 @@ 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 polling function
+	 * 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 the timestamp was actually 0, the timestamp is set one more time.
+	 */
+	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 device *dev = &device->dev;
@@ -181,8 +206,23 @@ static int acpi_als_add(struct acpi_device *device)
 	indio_dev->channels = acpi_als_channels;
 	indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
 
-	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE, NULL);
+	als->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name, indio_dev->id);
+	if (!als->trig)
+		return -ENOMEM;
+
+	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;
 
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH v7 0/3] iio: acpi_als: Add sotfware trigger support

[Jonathan Cameron]

On Wed, 17 Mar 2021 00:40:09 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> 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 second adds a local
> variable in probe routine and the last one adds trigger support.
> 
> This patch assumes the patch "iio: set default trig->dev.parent" is
> applied.
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to poke at it.

Thanks,

Jonathan

> 
> Changes since v7:
>   rebase on iio git testing branch.
> 
> Gwendal Grignou (3):
>   iio: acpi_als: Add timestamp channel
>   iio: acpi_als: Add local variable dev in probe
>   iio: acpi_als: Add trigger support
> 
>  drivers/iio/light/acpi-als.c | 113 +++++++++++++++++++++++------------
>  1 file changed, 76 insertions(+), 37 deletions(-)
>