* [PATCH v5 0/3] iio: acpi_als: Add sotfware trigger support
@ 2020-12-16 21:41 Gwendal Grignou
2020-12-16 21:41 ` [PATCH v5 1/3] iio: acpi_als: Add timestamp channel Gwendal Grignou
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Gwendal Grignou @ 2020-12-16 21:41 UTC (permalink / raw)
To: gabriele.mzt, jic23, lars, andy.shevchenko; +Cc: linux-iio, 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.
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 | 105 ++++++++++++++++++++++++-----------
1 file changed, 74 insertions(+), 31 deletions(-)
--
2.29.2.729.g45daf8777d-goog
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v5 1/3] iio: acpi_als: Add timestamp channel 2020-12-16 21:41 [PATCH v5 0/3] iio: acpi_als: Add sotfware trigger support Gwendal Grignou @ 2020-12-16 21:41 ` Gwendal Grignou 2020-12-16 22:39 ` Andy Shevchenko 2020-12-16 21:41 ` [PATCH v5 2/3] iio: acpi_als: Add local variable dev in probe Gwendal Grignou 2020-12-16 21:41 ` [PATCH v5 3/3] iio: acpi_als: Add trigger support Gwendal Grignou 2 siblings, 1 reply; 11+ messages in thread From: Gwendal Grignou @ 2020-12-16 21:41 UTC (permalink / raw) To: gabriele.mzt, jic23, lars, andy.shevchenko; +Cc: linux-iio, 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 v4: Fix spelling. Changes in v5: Fill comment line. 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 2be7180e2cbfa..6dbaeeebe2041 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.29.2.729.g45daf8777d-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5 1/3] iio: acpi_als: Add timestamp channel 2020-12-16 21:41 ` [PATCH v5 1/3] iio: acpi_als: Add timestamp channel Gwendal Grignou @ 2020-12-16 22:39 ` Andy Shevchenko 0 siblings, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2020-12-16 22:39 UTC (permalink / raw) To: Gwendal Grignou Cc: Gabriele Mazzotta, Jonathan Cameron, Lars-Peter Clausen, linux-iio On Wed, Dec 16, 2020 at 11:41 PM Gwendal Grignou <gwendal@chromium.org> wrote: > > Add timestamp channel in list of channel, to allow retrieving timestamps > when events are produced. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > Changes in v4: > Fix spelling. > Changes in v5: > Fill comment line. > > 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 2be7180e2cbfa..6dbaeeebe2041 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.29.2.729.g45daf8777d-goog > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 2/3] iio: acpi_als: Add local variable dev in probe 2020-12-16 21:41 [PATCH v5 0/3] iio: acpi_als: Add sotfware trigger support Gwendal Grignou 2020-12-16 21:41 ` [PATCH v5 1/3] iio: acpi_als: Add timestamp channel Gwendal Grignou @ 2020-12-16 21:41 ` Gwendal Grignou 2020-12-16 22:40 ` Andy Shevchenko 2020-12-16 21:41 ` [PATCH v5 3/3] iio: acpi_als: Add trigger support Gwendal Grignou 2 siblings, 1 reply; 11+ messages in thread From: Gwendal Grignou @ 2020-12-16 21:41 UTC (permalink / raw) To: gabriele.mzt, jic23, lars, andy.shevchenko; +Cc: linux-iio, Gwendal Grignou Use dev = &device->dev in probe routine for clarity. Remove setting parent of iio device, already done in iio_device_alloc(). Signed-off-by: Gwendal Grignou <gwendal@chromium.org> --- Added in v4. Changes in v5: Follow inverted xmas tree declaration convention. 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 6dbaeeebe2041..fd20808d4a119 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 iio_buffer *buffer; + struct acpi_als *als; - indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als)); + indio_dev = devm_iio_device_alloc(dev, sizeof(*als)); if (!indio_dev) return -ENOMEM; @@ -181,13 +182,13 @@ static int acpi_als_add(struct acpi_device *device) indio_dev->channels = acpi_als_channels; indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels); - buffer = devm_iio_kfifo_allocate(&device->dev); + buffer = devm_iio_kfifo_allocate(dev); if (!buffer) return -ENOMEM; iio_device_attach_buffer(indio_dev, buffer); - 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.729.g45daf8777d-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5 2/3] iio: acpi_als: Add local variable dev in probe 2020-12-16 21:41 ` [PATCH v5 2/3] iio: acpi_als: Add local variable dev in probe Gwendal Grignou @ 2020-12-16 22:40 ` Andy Shevchenko 0 siblings, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2020-12-16 22:40 UTC (permalink / raw) To: Gwendal Grignou Cc: Gabriele Mazzotta, Jonathan Cameron, Lars-Peter Clausen, linux-iio On Wed, Dec 16, 2020 at 11:41 PM Gwendal Grignou <gwendal@chromium.org> wrote: > > Use dev = &device->dev in probe routine for clarity. > Remove setting parent of iio device, already done in iio_device_alloc(). Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > Added in v4. > Changes in v5: > Follow inverted xmas tree declaration convention. > > 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 6dbaeeebe2041..fd20808d4a119 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; I was talking only about dev, but I don't think it's a big deal for maintainers to have this change here. > + struct device *dev = &device->dev; > struct iio_dev *indio_dev; > struct iio_buffer *buffer; > + struct acpi_als *als; > > - indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als)); > + indio_dev = devm_iio_device_alloc(dev, sizeof(*als)); > if (!indio_dev) > return -ENOMEM; > > @@ -181,13 +182,13 @@ static int acpi_als_add(struct acpi_device *device) > indio_dev->channels = acpi_als_channels; > indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels); > > - buffer = devm_iio_kfifo_allocate(&device->dev); > + buffer = devm_iio_kfifo_allocate(dev); > if (!buffer) > return -ENOMEM; > > iio_device_attach_buffer(indio_dev, buffer); > > - 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.729.g45daf8777d-goog > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 3/3] iio: acpi_als: Add trigger support 2020-12-16 21:41 [PATCH v5 0/3] iio: acpi_als: Add sotfware trigger support Gwendal Grignou 2020-12-16 21:41 ` [PATCH v5 1/3] iio: acpi_als: Add timestamp channel Gwendal Grignou 2020-12-16 21:41 ` [PATCH v5 2/3] iio: acpi_als: Add local variable dev in probe Gwendal Grignou @ 2020-12-16 21:41 ` Gwendal Grignou 2020-12-16 22:38 ` Andy Shevchenko 2 siblings, 1 reply; 11+ messages in thread From: Gwendal Grignou @ 2020-12-16 21:41 UTC (permalink / raw) To: gabriele.mzt, jic23, lars, andy.shevchenko; +Cc: linux-iio, 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> --- Changes in v4: Added comments when pf->timestamp is 0 and valid. Fix spelling. Changes in v5: Improve commit message readability, add note about backward compatibility. Remove unneeded include file. drivers/iio/light/acpi-als.c | 89 ++++++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c index fd20808d4a119..80fe0da51fad3 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,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, @@ -159,12 +150,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 device *dev = &device->dev; struct iio_dev *indio_dev; - struct iio_buffer *buffer; struct acpi_als *als; + int ret; indio_dev = devm_iio_device_alloc(dev, sizeof(*als)); if (!indio_dev) @@ -178,15 +204,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); } -- 2.29.2.729.g45daf8777d-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/3] iio: acpi_als: Add trigger support 2020-12-16 21:41 ` [PATCH v5 3/3] iio: acpi_als: Add trigger support Gwendal Grignou @ 2020-12-16 22:38 ` Andy Shevchenko 2020-12-29 17:24 ` Jonathan Cameron 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2020-12-16 22:38 UTC (permalink / raw) To: Gwendal Grignou Cc: Gabriele Mazzotta, Jonathan Cameron, Lars-Peter Clausen, linux-iio On Wed, Dec 16, 2020 at 11:41 PM 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). > 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 Few nitpicks below. After addressing, take my Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> assuming that backward compatibility has been tested as well. > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > Changes in v4: > Added comments when pf->timestamp is 0 and valid. Fix spelling. > Changes in v5: > Improve commit message readability, add note about backward > compatibility. > Remove unneeded include file. > > drivers/iio/light/acpi-als.c | 89 ++++++++++++++++++++++++++---------- > 1 file changed, 66 insertions(+), 23 deletions(-) > > diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c > index fd20808d4a119..80fe0da51fad3 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,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)) I guess it can be located on one line. I hope those functions have no side effects. In that case you may invert logic (save 2 characters) 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, > @@ -159,12 +150,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. Continue the second sentence of the previous line. No need to have it in a separate paragraph. > + * > + * 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; > struct iio_dev *indio_dev; > - struct iio_buffer *buffer; > struct acpi_als *als; > + int ret; > > indio_dev = devm_iio_device_alloc(dev, sizeof(*als)); > if (!indio_dev) > @@ -178,15 +204,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); > } > -- > 2.29.2.729.g45daf8777d-goog > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/3] iio: acpi_als: Add trigger support 2020-12-16 22:38 ` Andy Shevchenko @ 2020-12-29 17:24 ` Jonathan Cameron 2020-12-29 17:33 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2020-12-29 17:24 UTC (permalink / raw) To: Andy Shevchenko Cc: Gwendal Grignou, Gabriele Mazzotta, Lars-Peter Clausen, linux-iio On Thu, 17 Dec 2020 00:38:06 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Dec 16, 2020 at 11:41 PM 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). > > 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 > > Few nitpicks below. After addressing, take my > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > assuming that backward compatibility has been tested as well. > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > > --- > > Changes in v4: > > Added comments when pf->timestamp is 0 and valid. Fix spelling. > > Changes in v5: > > Improve commit message readability, add note about backward > > compatibility. > > Remove unneeded include file. > > > > drivers/iio/light/acpi-als.c | 89 ++++++++++++++++++++++++++---------- > > 1 file changed, 66 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c > > index fd20808d4a119..80fe0da51fad3 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,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)) > > I guess it can be located on one line. > > I hope those functions have no side effects. In that case you may > invert logic (save 2 characters) > > if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev)) You can but at cost of indenting the whole following block on level further. I'm not that fussed, but in general that doesn't seem like a good idea to save two characters here. Gwendal made that not too bad in v6 by removing the read that appears to be unnecessary. > > > + 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, > > @@ -159,12 +150,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. > > Continue the second sentence of the previous line. No need to have it > in a separate paragraph. > > > + * > > + * 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; > > struct iio_dev *indio_dev; > > - struct iio_buffer *buffer; > > struct acpi_als *als; > > + int ret; > > > > indio_dev = devm_iio_device_alloc(dev, sizeof(*als)); > > if (!indio_dev) > > @@ -178,15 +204,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); > > } > > -- > > 2.29.2.729.g45daf8777d-goog > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/3] iio: acpi_als: Add trigger support 2020-12-29 17:24 ` Jonathan Cameron @ 2020-12-29 17:33 ` Andy Shevchenko 2020-12-29 17:52 ` Jonathan Cameron 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2020-12-29 17:33 UTC (permalink / raw) To: Jonathan Cameron Cc: Gwendal Grignou, Gabriele Mazzotta, Lars-Peter Clausen, linux-iio On Tue, Dec 29, 2020 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 17 Dec 2020 00:38:06 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Wed, Dec 16, 2020 at 11:41 PM 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). > > > 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 > > > > Few nitpicks below. After addressing, take my > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > assuming that backward compatibility has been tested as well. ... > > > + if (!iio_buffer_enabled(indio_dev) || > > > + !iio_trigger_using_own(indio_dev)) > > > > I guess it can be located on one line. > > > > I hope those functions have no side effects. In that case you may > > invert logic (save 2 characters) > > > > if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev)) > > You can but at cost of indenting the whole following block on level further. > I'm not that fussed, but in general that doesn't seem like a good idea to > save two characters here. I didn't get it. The proposed change, in case of no side effect, is an equivalent to the existing one, just 2 characters less. How does it affect code block indentation? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/3] iio: acpi_als: Add trigger support 2020-12-29 17:33 ` Andy Shevchenko @ 2020-12-29 17:52 ` Jonathan Cameron 2020-12-29 18:13 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2020-12-29 17:52 UTC (permalink / raw) To: Andy Shevchenko Cc: Gwendal Grignou, Gabriele Mazzotta, Lars-Peter Clausen, linux-iio On Tue, 29 Dec 2020 19:33:39 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Dec 29, 2020 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Thu, 17 Dec 2020 00:38:06 +0200 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Wed, Dec 16, 2020 at 11:41 PM 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). > > > > 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 > > > > > > Few nitpicks below. After addressing, take my > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > assuming that backward compatibility has been tested as well. > > ... > > > > > + if (!iio_buffer_enabled(indio_dev) || > > > > + !iio_trigger_using_own(indio_dev)) > > > > > > I guess it can be located on one line. > > > > > > I hope those functions have no side effects. In that case you may > > > invert logic (save 2 characters) > > > > > > if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev)) > > > > You can but at cost of indenting the whole following block on level further. > > I'm not that fussed, but in general that doesn't seem like a good idea to > > save two characters here. > > I didn't get it. The proposed change, in case of no side effect, is an > equivalent to the existing one, just 2 characters less. > How does it affect code block indentation? It's not the same. As stated it's the inverse condition. Could add some brackets and a ! or, flip the logic of the whole if condition and not return early. Jonathan > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5 3/3] iio: acpi_als: Add trigger support 2020-12-29 17:52 ` Jonathan Cameron @ 2020-12-29 18:13 ` Andy Shevchenko 0 siblings, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2020-12-29 18:13 UTC (permalink / raw) To: Jonathan Cameron Cc: Gwendal Grignou, Gabriele Mazzotta, Lars-Peter Clausen, linux-iio On Tue, Dec 29, 2020 at 7:52 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 29 Dec 2020 19:33:39 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > It's not the same. As stated it's the inverse condition. Could add some brackets > and a ! or, flip the logic of the whole if condition and not return early. My bad, you are right. I think I had something in mind about indentation as well, perhaps a separate function or so... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-12-29 18:14 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-16 21:41 [PATCH v5 0/3] iio: acpi_als: Add sotfware trigger support Gwendal Grignou 2020-12-16 21:41 ` [PATCH v5 1/3] iio: acpi_als: Add timestamp channel Gwendal Grignou 2020-12-16 22:39 ` Andy Shevchenko 2020-12-16 21:41 ` [PATCH v5 2/3] iio: acpi_als: Add local variable dev in probe Gwendal Grignou 2020-12-16 22:40 ` Andy Shevchenko 2020-12-16 21:41 ` [PATCH v5 3/3] iio: acpi_als: Add trigger support Gwendal Grignou 2020-12-16 22:38 ` Andy Shevchenko 2020-12-29 17:24 ` Jonathan Cameron 2020-12-29 17:33 ` Andy Shevchenko 2020-12-29 17:52 ` Jonathan Cameron 2020-12-29 18:13 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.