On Sat, Dec 5, 2020 at 10:27 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 4 Dec 2020 12:37:55 -0800
> Gwendal Grignou <gwendal@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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);
> > }
>
