[PATCH] iio: acpi_als: Add trigger support

[PATCH] iio: acpi_als: Add trigger support

[Gwendal Grignou]

Add timestamp channel: use standard procedure to collect timestamp.
As some firmware do not notify on illuminance changes, add a
trigger to periodically query light.
We can either use the device trigger, or a software trigger like sysfs
or hrtimer.

This change is not backward compatible. To get samples from bios that
supports notification, we need to register the hardware trigger first:

echo acpi-als-dev${X} > iio\:device${X}/trigger/current_trigger

Check iio_info reports the sensor as buffer capable:
iio:device0: acpi-als (buffer capable)
Check we can get data on demand:
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>
---
 drivers/iio/light/acpi-als.c | 86 +++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index 1eafd0b24e182..2619e4b073a59 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"
@@ -45,22 +49,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, 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;
+	struct iio_trigger	*trig;
 
 	s32			evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE];
 };
@@ -104,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,
@@ -161,11 +152,37 @@ 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);
+
+	memset(buffer, 0, ACPI_ALS_EVT_BUFFER_SIZE);
+	ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
+	if (ret < 0)
+		goto out;
+	*buffer++ = val;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, als->evt_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;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
 	if (!indio_dev)
@@ -180,15 +197,30 @@ static int acpi_als_add(struct acpi_device *device)
 	indio_dev->name = ACPI_ALS_DEVICE_NAME;
 	indio_dev->dev.parent = &device->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(&device->dev,
+					   "%s-dev%d",
+					   indio_dev->name,
+					   indio_dev->id);
+	if (!als->trig)
 		return -ENOMEM;
 
-	iio_device_attach_buffer(indio_dev, buffer);
+	als->trig->dev.parent = &device->dev;
+	iio_trigger_set_drvdata(als->trig, indio_dev);
+	ret = iio_trigger_register(als->trig);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(&device->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);
 }
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH] iio: acpi_als: Add trigger support

[Jonathan Cameron]

On Fri,  4 Dec 2020 12:37:55 -0800
Gwendal Grignou <gwendal@chromium.org> wrote:

> Add timestamp channel: use standard procedure to collect timestamp.
> As some firmware do not notify on illuminance changes, add a
> trigger to periodically query light.
> We can either use the device trigger, or a software trigger like sysfs
> or hrtimer.
> 
> This change is not backward compatible. To get samples from bios that
> supports notification, we need to register the hardware trigger first:
> 
> echo acpi-als-dev${X} > iio\:device${X}/trigger/current_trigger

That's a problem as we can't break backwards compatibility.
Note that you can set a default trigger for a device with

indio_dev->trig = iio_trigger_get(trig);
If we do that then it can still be overridden but at least
we don't break backwards compatibility.
We also have an immutable variant if we really don't want
it to be changed later but I don't think that is true here.

> 
> Check iio_info reports the sensor as buffer capable:
> iio:device0: acpi-als (buffer capable)
> Check we can get data on demand:
> 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>
> ---
>  drivers/iio/light/acpi-als.c | 86 +++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index 1eafd0b24e182..2619e4b073a59 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"
> @@ -45,22 +49,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, 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;
> +	struct iio_trigger	*trig;
>  
>  	s32			evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE];
This gets used in iio_push_to_buffers_with_timestamp and that
has a few not well documented quirks (on list to fix)
In particular it relies on being able to put the timestamp in
the right place and side effect of that is that the buffer
passed to that function must be 8 byte aligned.
Note I've been working through fixing drivers for this but clearly
still some to go.

You can probably do that here with __aligned(8)

As a side note, isn't the buffer way too big?  I think should be
ACPI_ALS_EVT_BUFFER_SIZE / sizeof(u32)

>  };
> @@ -104,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);

One issue with this path is it won't have set the poll function timestamp.
It's a long term problem that there is no way for a device to know with
certainty if the top half of it's trigger handler was ever called.

I have been thinking about adding accessors to pf->timestamp to set
and clear it which could also set a flag to say it had been set.
Hence the get would return an error if not and we could grab a local
timestamp if that happens as it would be the best available.

Anyhow, I would assume you will get all 0 timestamps currently.

>  		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,
> @@ -161,11 +152,37 @@ 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);
> +
> +	memset(buffer, 0, ACPI_ALS_EVT_BUFFER_SIZE);

This shouldn't be needed. The buffer was originally kzalloc'd
I believe and we write all the values that might contain stale
data.

> +	ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
> +	if (ret < 0)
> +		goto out;
> +	*buffer++ = val;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, als->evt_buffer,
> +					   pf->timestamp);

Use buffer for evt_buffer here for consistency.

> +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;
> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
>  	if (!indio_dev)
> @@ -180,15 +197,30 @@ static int acpi_als_add(struct acpi_device *device)
>  	indio_dev->name = ACPI_ALS_DEVICE_NAME;
>  	indio_dev->dev.parent = &device->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(&device->dev,
> +					   "%s-dev%d",
> +					   indio_dev->name,
> +					   indio_dev->id);
> +	if (!als->trig)
>  		return -ENOMEM;
>  
> -	iio_device_attach_buffer(indio_dev, buffer);
> +	als->trig->dev.parent = &device->dev;

Hmm. we should probably push this boilerplate into
devm_iio_trigger_alloc() like we have done for iio devices.

> +	iio_trigger_set_drvdata(als->trig, indio_dev);
> +	ret = iio_trigger_register(als->trig);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(&device->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);
>  }

Re: [PATCH] iio: acpi_als: Add trigger support

[Gwendal Grignou]

On Sat, Dec 5, 2020 at 10:27 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri,  4 Dec 2020 12:37:55 -0800
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> > Add timestamp channel: use standard procedure to collect timestamp.
> > As some firmware do not notify on illuminance changes, add a
> > trigger to periodically query light.
> > We can either use the device trigger, or a software trigger like sysfs
> > or hrtimer.
> >
> > This change is not backward compatible. To get samples from bios that
> > supports notification, we need to register the hardware trigger first:
> >
> > echo acpi-als-dev${X} > iio\:device${X}/trigger/current_trigger
>
> That's a problem as we can't break backwards compatibility.
> Note that you can set a default trigger for a device with
>
> indio_dev->trig = iio_trigger_get(trig);
> If we do that then it can still be overridden but at least
> we don't break backwards compatibility.
Done in v2, I now understand the code better.
> We also have an immutable variant if we really don't want
> it to be changed later but I don't think that is true here.
>
> >
> > Check iio_info reports the sensor as buffer capable:
> > iio:device0: acpi-als (buffer capable)
> > Check we can get data on demand:
> > 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>
> > ---
> >  drivers/iio/light/acpi-als.c | 86 +++++++++++++++++++++++++-----------
> >  1 file changed, 59 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> > index 1eafd0b24e182..2619e4b073a59 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"
> > @@ -45,22 +49,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, 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;
> > +     struct iio_trigger      *trig;
> >
> >       s32                     evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE];
> This gets used in iio_push_to_buffers_with_timestamp and that
> has a few not well documented quirks (on list to fix)
> In particular it relies on being able to put the timestamp in
> the right place and side effect of that is that the buffer
> passed to that function must be 8 byte aligned.
> Note I've been working through fixing drivers for this but clearly
> still some to go.
>
> You can probably do that here with __aligned(8)
Done in v2.
>
> As a side note, isn't the buffer way too big?  I think should be
> ACPI_ALS_EVT_BUFFER_SIZE / sizeof(u32)
Done in v2.
>
> >  };
> > @@ -104,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);
>
> One issue with this path is it won't have set the poll function timestamp.
> It's a long term problem that there is no way for a device to know with
> certainty if the top half of it's trigger handler was ever called.
>
> I have been thinking about adding accessors to pf->timestamp to set
> and clear it which could also set a flag to say it had been set.
> Hence the get would return an error if not and we could grab a local
> timestamp if that happens as it would be the best available.
Looking at other devices that support sw/hw riggers (for instance
light/rpr0521.c), pf->timestamp == 0 indicates that the top half was
not called.
>
> Anyhow, I would assume you will get all 0 timestamps currently.
>
> >               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,
> > @@ -161,11 +152,37 @@ 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);
> > +
> > +     memset(buffer, 0, ACPI_ALS_EVT_BUFFER_SIZE);
>
> This shouldn't be needed. The buffer was originally kzalloc'd
> I believe and we write all the values that might contain stale
> data.
Done
>
> > +     ret = acpi_als_read_value(als, ACPI_ALS_ILLUMINANCE, &val);
> > +     if (ret < 0)
> > +             goto out;
> > +     *buffer++ = val;
> > +
> > +     iio_push_to_buffers_with_timestamp(indio_dev, als->evt_buffer,
> > +                                        pf->timestamp);
>
> Use buffer for evt_buffer here for consistency.
Done
>
> > +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;
> > +     int ret;
> >
> >       indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
> >       if (!indio_dev)
> > @@ -180,15 +197,30 @@ static int acpi_als_add(struct acpi_device *device)
> >       indio_dev->name = ACPI_ALS_DEVICE_NAME;
> >       indio_dev->dev.parent = &device->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(&device->dev,
> > +                                        "%s-dev%d",
> > +                                        indio_dev->name,
> > +                                        indio_dev->id);
> > +     if (!als->trig)
> >               return -ENOMEM;
> >
> > -     iio_device_attach_buffer(indio_dev, buffer);
> > +     als->trig->dev.parent = &device->dev;
>
> Hmm. we should probably push this boilerplate into
> devm_iio_trigger_alloc() like we have done for iio devices.
Done in a separate patch. I only clean up obvious calls to set
trig->dev.parent.
Looking at the code, some triggers parents are set to the request
device in more convoluted ways or the grand parent.
>
> > +     iio_trigger_set_drvdata(als->trig, indio_dev);
> > +     ret = iio_trigger_register(als->trig);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = devm_iio_triggered_buffer_setup(&device->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);
> >  }
>

Re: [PATCH] iio: acpi_als: Add trigger support

[Jonathan Cameron]

...

> >  
> > >  };
> > > @@ -104,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);  
> >
> > One issue with this path is it won't have set the poll function timestamp.
> > It's a long term problem that there is no way for a device to know with
> > certainty if the top half of it's trigger handler was ever called.
> >
> > I have been thinking about adding accessors to pf->timestamp to set
> > and clear it which could also set a flag to say it had been set.
> > Hence the get would return an error if not and we could grab a local
> > timestamp if that happens as it would be the best available.  
> Looking at other devices that support sw/hw riggers (for instance
> light/rpr0521.c), pf->timestamp == 0 indicates that the top half was
> not called.

While it should be vanishingly rare, in theory timestamp == 0 is
a valid time.  I agree in practice we could use that but I would
prefer clearer semantics / code readability as would occur with
explicit accessors.

> >
> > Anyhow, I would assume you will get all 0 timestamps currently.
> >  
> > >               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,
> > > @@ -161,11 +152,37 @@ static const struct iio_info acpi_als_info = {
> > >       .read_raw               = acpi_als_read_raw,
> > >  };
...

> > >  static int acpi_als_add(struct acpi_device *device)
> > >  {
> > >       struct acpi_als *als;
> > >       struct iio_dev *indio_dev;
> > > -     struct iio_buffer *buffer;
> > > +     int ret;
> > >
> > >       indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
> > >       if (!indio_dev)
> > > @@ -180,15 +197,30 @@ static int acpi_als_add(struct acpi_device *device)
> > >       indio_dev->name = ACPI_ALS_DEVICE_NAME;
> > >       indio_dev->dev.parent = &device->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(&device->dev,
> > > +                                        "%s-dev%d",
> > > +                                        indio_dev->name,
> > > +                                        indio_dev->id);
> > > +     if (!als->trig)
> > >               return -ENOMEM;
> > >
> > > -     iio_device_attach_buffer(indio_dev, buffer);
> > > +     als->trig->dev.parent = &device->dev;  
> >
> > Hmm. we should probably push this boilerplate into
> > devm_iio_trigger_alloc() like we have done for iio devices.  
> Done in a separate patch. I only clean up obvious calls to set
> trig->dev.parent.
> Looking at the code, some triggers parents are set to the request
> device in more convoluted ways or the grand parent.

Yup. There are a few interesting devices out there, particularly
when mfd's are involved.  Whilst I'm not sure it's actually
necessary for them to set the parents as they do, it is now
ABI so we are stuck with it.

Thanks for that series btw.

Jonathan


> >  
> > > +     iio_trigger_set_drvdata(als->trig, indio_dev);
> > > +     ret = iio_trigger_register(als->trig);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = devm_iio_triggered_buffer_setup(&device->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);
> > >  }  
> >  

Re: [PATCH] iio: acpi_als: Add trigger support

[Andy Shevchenko]

On Fri, Dec 4, 2020 at 10:40 PM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> Add timestamp channel: use standard procedure to collect timestamp.
> As some firmware do not notify on illuminance changes, add a

do -> does

> trigger to periodically query light.

Sounds like two things in one change, you have to split.

> We can either use the device trigger, or a software trigger like sysfs
> or hrtimer.
>
> This change is not backward compatible.

You mean you are breaking ABI? It won't go like this.

> To get samples from bios that

bios -> BIOS

> supports notification, we need to register the hardware trigger first:

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: acpi_als: Add trigger support

[Gwendal Grignou]

On Wed, Dec 9, 2020 at 7:16 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Dec 4, 2020 at 10:40 PM Gwendal Grignou <gwendal@chromium.org> wrote:
> >
> > Add timestamp channel: use standard procedure to collect timestamp.
> > As some firmware do not notify on illuminance changes, add a
>
> do -> does
Done in v2
>
> > trigger to periodically query light.
>
> Sounds like two things in one change, you have to split.
Done in v2
>
> > We can either use the device trigger, or a software trigger like sysfs
> > or hrtimer.
> >
> > This change is not backward compatible.
>
> You mean you are breaking ABI? It won't go like this.
No, I was not setting a default trigger: on machines capable of
sending ALS readout asynchronously, the user had to set a trigger in
v1. Fixed in v2, now backward compatible.
>
> > To get samples from bios that
>
> bios -> BIOS
Done in v2, submitting soon.
>
> > supports notification, we need to register the hardware trigger first:
>
> --
> With Best Regards,
> Andy Shevchenko