diff mbox series

[v4,3/3] iio: acpi_als: Add trigger support

Message ID 20201216060233.597689-4-gwendal@chromium.org (mailing list archive)
State New, archived
Headers show
Series iio: acpi_als: Add sotfware trigger support | expand

Commit Message

Gwendal Grignou Dec. 16, 2020, 6:02 a.m. UTC
As some firmware does not notify on illuminance changes, add a
trigger to be able to query light via software (sysfs-trigger or
hrtrigger).

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:
  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 v4:
 Added comments when pf->timestamp is 0 and valid. Fix spelling.

 drivers/iio/light/acpi-als.c | 88 +++++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 22 deletions(-)

Comments

Andy Shevchenko Dec. 16, 2020, 1:56 p.m. UTC | #1
On Wed, Dec 16, 2020 at 8:02 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).
>
> 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:

+ blank line

>   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&

space before &

>   echo 1 > trigger2/trigger_now

You may improve readability by using a temporary variable:

  IIO_DEV="iio:device0"
  ...
  cat trigger2/name > $IIO_DEV/trigger/current_trigger
  for i in $IIO_DEV/scan_elements/*_en $IIO_DEV/buffer/enable ; do
  ...
  od -x /dev/$IIO_DEV &

Commit message keeps silent about the side effect of these changes on
the old setups.
Is there any?

...

>  drivers/iio/light/acpi-als.c | 88 +++++++++++++++++++++++++++---------

Question here, does it need a documentation update?

...

>  #include <linux/iio/kfifo_buf.h>

Is it still in use?
Gwendal Grignou Dec. 18, 2020, 12:40 a.m. UTC | #2
On Wed, Dec 16, 2020 at 5:56 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Dec 16, 2020 at 8:02 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).
> >
> > 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:
>
> + blank line
>
> >   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&
>
> space before &
Fixed in v5
>
> >   echo 1 > trigger2/trigger_now
>
> You may improve readability by using a temporary variable:
>
>   IIO_DEV="iio:device0"
>   ...
>   cat trigger2/name > $IIO_DEV/trigger/current_trigger
>   for i in $IIO_DEV/scan_elements/*_en $IIO_DEV/buffer/enable ; do
>   ...
>   od -x /dev/$IIO_DEV &
Fixed in v5
>
> Commit message keeps silent about the side effect of these changes on
> the old setups.
> Is there any?
No, add note in commit message.
>
> ...
>
> >  drivers/iio/light/acpi-als.c | 88 +++++++++++++++++++++++++++---------
>
> Question here, does it need a documentation update?
No, as the driver follows the current iio ABI.
>
> ...
>
> >  #include <linux/iio/kfifo_buf.h>
>
> Is it still in use?
Fixed in v5.
>
> --
> With Best Regards,
Thanks for thorough review,

Gwendal.
> Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index 32cd0f8438c33..57d558ea5c0f1 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,12 +152,47 @@  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 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(dev, sizeof(*als));
 	if (!indio_dev)
@@ -179,15 +206,32 @@  static int acpi_als_add(struct acpi_device *device)
 
 	indio_dev->name = ACPI_ALS_DEVICE_NAME;
 	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(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(dev, indio_dev);
 }