On 01/06/14 15:54, Marek Vasut wrote:
From: Martin Liska <marxin.liska@xxxxxxxxx>
Add basic implementation of the ACPI0008 Ambient Light Sensor driver.
This driver currently supports only the ALI property, yet is ready to
be easily extended to handle ALC, ALT, ALP ones as well.
Signed-off-by: Martin Liska <marxin.liska@xxxxxxxxx>
Signed-off-by: Marek Vasut <marex@xxxxxxx>
Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
---
drivers/iio/light/Kconfig | 10 ++
drivers/iio/light/Makefile | 1 +
drivers/iio/light/acpi-als.c | 315 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 326 insertions(+)
create mode 100644 drivers/iio/light/acpi-als.c
V2: Fix the channel mask, so it's really reading RAW data.
V3: Put scan timestamp into the buffer only when enabled,
Set the light sensor ID to 0 instead of 1
V4: Select IIO_TRIGGERED_BUFFER as we need it here
V5: Use irq_work to trigger the buffer
Use module_acpi_driver()
V6: Align with 3.15-rc7
Use iio_push_to_buffers_with_timestamp()
Use iio_trigger_set_drvdata()
Use .info_mask_separate
Use devm_iio_device_alloc()
Stuff the event buffer into struct acpi_als so we don't alloc mem twice
Compute the evt_buffer size automatically based on acpi_als_channels[]
Implement .validate_device() and .validate_trigger()
Tested on MacBook Air (Mid 2013) and Acer IconiaTab W510
Looking good but the trigger handling is 'interesting' (given it doesn't
do anything). Hence you need to revisit that. A couple of other bits
inline.
Jonathan
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index c89740d..d367c83 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -5,6 +5,16 @@
menu "Light sensors"
+config ACPI_ALS
+ tristate "ACPI Ambient Light Sensor"
+ depends on ACPI
+ select IIO_TRIGGERED_BUFFER
+ help
+ Support for the ACPI0008 Ambient Light Sensor.
+
+ This driver can also be built as a module. If so, the module
+ will be called acpi-als.
+
config ADJD_S311
tristate "ADJD-S311-CR999 digital color sensor"
select IIO_BUFFER
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 3eb36e5..3074394 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -3,6 +3,7 @@
#
# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_ACPI_ALS) += acpi-als.o
obj-$(CONFIG_ADJD_S311) += adjd_s311.o
obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_CM32181) += cm32181.o
diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
new file mode 100644
index 0000000..f4ab108
--- /dev/null
+++ b/drivers/iio/light/acpi-als.c
@@ -0,0 +1,315 @@
+/*
+ * ACPI Ambient Light Sensor Driver
+ *
+ * Based on ALS driver:
+ * Copyright (C) 2009 Zhang Rui <rui.zhang@xxxxxxxxx>
+ *
+ * Rework for IIO subsystem:
+ * Copyright (C) 2012-2013 Martin Liska <marxin.liska@xxxxxxxxx>
+ *
+ * Final cleanup and debugging:
+ * Copyright (C) 2013-2014 Marek Vasut <marex@xxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/acpi.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define ACPI_ALS_CLASS "als"
+#define ACPI_ALS_DEVICE_NAME "acpi-als"
+#define ACPI_ALS_NOTIFY_ILLUMINANCE 0x80
+
+ACPI_MODULE_NAME("acpi-als");
+
+/*
+ * So far, there's only one channel in here, but the specification for
+ * ACPI0008 says there can be more to what the block can report. Like
+ * chromaticity and such. We are ready for incoming additions!
+ */
+static const struct iio_chan_spec acpi_als_channels[] = {
+ {
+ .type = IIO_LIGHT,
Don't bother filling the zero values as these are the defaults
anyway.
+ .indexed = 0,
+ .channel = 0,
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 10,
+ .storagebits = 16,
+ },
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ },
+};
+
+/*
+ * 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.
+ */
+#define EVT_NR_SOURCES ARRAY_SIZE(acpi_als_channels)
+#define EVT_BUFFER_SIZE \
+ (sizeof(int64_t) + (EVT_NR_SOURCES * sizeof(uint16_t)))
+
+struct acpi_als {
+ struct acpi_device *device;
+ struct iio_trigger *trig;
+ struct mutex lock;
+
+ uint16_t evt_buffer[EVT_BUFFER_SIZE];
+};
+
+/*
+ * All types of properties the ACPI0008 block can report. The ALI, ALC, ALT
+ * and ALP can all be handled by als_read_value() below, while the ALR is
+ * special.
+ *
+ * The _ALR property returns tables that can be used to fine-tune the values
+ * reported by the other props based on the particular hardware type and it's
+ * location (it contains tables for "rainy", "bright inhouse lighting" etc.).
+ *
+ * So far, we support only ALI (illuminance).
+ */
+#define ACPI_ALS_ILLUMINANCE "_ALI"
+#define ACPI_ALS_CHROMATICITY "_ALC"
+#define ACPI_ALS_COLOR_TEMP "_ALT"
+#define ACPI_ALS_POLLING "_ALP"
+#define ACPI_ALS_TABLES "_ALR"
+
+static int32_t als_read_value(struct acpi_als *als, char *prop)
+{
+ unsigned long long illuminance;
+ acpi_status status;
This does rather look like a wrapper for the wrappers sake.
Is there any significant reason to have this wrapper at all
rather than calling the acpi_evaluate_integer directly at
the call sites?
+
+ status = acpi_evaluate_integer(als->device->handle,
+ prop, NULL, &illuminance);
+
+ if (ACPI_FAILURE(status)) {
+ ACPI_EXCEPTION((AE_INFO, status,
+ "Error reading ALS illuminance"));
+ /* Something went wrong, it's pitch black outside! */
+ illuminance = 0;
Can we not do better than this as an error value to pass back?
+ }
+
+ return illuminance;
+}
+
So this is the acpi equivalent of an interrupt (I think!)
We could probably blugeon this into our normal form (dataready type
trigger - but maybe it doesn't make sense here. If you want
to see how I would do that, see the sysfs-trigger in iio/triggers).
It is perfectly acceptable to have iio devices that don't have a trigger
but fill the buffer directly. The disadvantage of that approach is that
it we are then stuck with that interface and can never add a trigger.
Still who is going to want synchronized data from multiple sensors
where some of them are available only via acpi? Seems unlikely
to my mind.
+static void acpi_als_notify(struct acpi_device *device, u32 event)
+{
+ struct iio_dev *iio = acpi_driver_data(device);
+ struct acpi_als *als = iio_priv(iio);
+ uint16_t *buffer = als->evt_buffer;
+ int64_t time_ns = iio_get_time_ns();
+
+ mutex_lock(&als->lock);
+
+ memset(buffer, 0, EVT_BUFFER_SIZE);
+
+ switch (event) {
+ case ACPI_ALS_NOTIFY_ILLUMINANCE:
+ *buffer++ = als_read_value(als, ACPI_ALS_ILLUMINANCE);
+ break;
+ default:
+ /* Unhandled event */
+ dev_dbg(&device->dev, "Unhandled ACPI ALS event (%08x)!\n",
+ event);
+ return;
+ }
+
+ iio_push_to_buffers_with_timestamp(iio, (uint8_t *)als->evt_buffer,
+ time_ns);
+
+ mutex_unlock(&als->lock);
+}
+
+static int acpi_als_read_raw(struct iio_dev *iio,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct acpi_als *als = iio_priv(iio);
+
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ /* we support only illumination (_ALI) so far. */
+ if (chan->type != IIO_LIGHT)
+ return -EINVAL;
+
> + *val = als_read_value(als, ACPI_ALS_ILLUMINANCE);
A blank line here would be good for readability (slightly!).
+ return IIO_VAL_INT;
+}
+
+static int acpi_als_validate_trigger(struct iio_dev *iio,
+ struct iio_trigger *trig)
+{
+ if (iio->dev.parent != trig->dev.parent)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct iio_info acpi_als_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = acpi_als_read_raw,
+ .validate_trigger = acpi_als_validate_trigger,
+};
+
+static irqreturn_t acpi_als_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *iio = pf->indio_dev;
+ struct acpi_als *als = iio_priv(iio);
+
umm. So this does nothing? I'm a little confused.
This iio_trigger_notify_done just resets the mask to allow the
interrupt to fire again..
+ iio_trigger_notify_done(als->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int acpi_als_validate_device(struct iio_trigger *trig,
+ struct iio_dev *iio)
+{
+ if (iio->dev.parent != trig->dev.parent)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct iio_trigger_ops acpi_als_trigger_ops = {
+ .owner = THIS_MODULE,
+ .validate_device = acpi_als_validate_device,
+};
+
+static int acpi_als_trigger_init(struct iio_dev *iio)
+{
+ struct acpi_als *als = iio_priv(iio);
+ struct device *dev = &iio->dev;
+ struct iio_trigger *trig;
+ int ret;
+
+ trig = devm_iio_trigger_alloc(dev, "%s-dev%i", iio->name, iio->id);
+ if (!trig)
+ return -ENOMEM;
+
+ trig->dev.parent = dev->parent;
+ trig->ops = &acpi_als_trigger_ops;
+ iio_trigger_set_drvdata(trig, iio);
+
+ ret = iio_trigger_register(trig);
+ if (ret) {
No need to free, it's a devm allocation so it will get cleaned up
anyway as the failure to probe propagates out from this error.
Also, not using the devm_iio_trigger_free version will lead to a double
free an probably a kernel oops.
+ iio_trigger_free(trig);
+ return ret;
+ }
+
+ als->trig = trig;
+
+ return 0;
+}
+
+static void acpi_als_trigger_remove(struct iio_dev *iio)
+{
+ struct acpi_als *als = iio_priv(iio);
+ iio_trigger_unregister(als->trig);
+}
+
+static int acpi_als_add(struct acpi_device *device)
+{
+ struct acpi_als *als;
+ struct iio_dev *iio;
If it doesn't matter to you, I'd prefer using the naming that
has become pretty much a standard. struct iio_dev *indio_dev;
Just makes it ever so slightly easier to check for common
patterns in the code.
+ int ret;
+
+ iio = devm_iio_device_alloc(&device->dev, sizeof(*als));
+ if (!iio)
+ return -ENOMEM;
+
+ als = iio_priv(iio);
+
+ device->driver_data = iio;
Currious that there doesn't seem to be an opaque wrapper for setting
driver_data in acpi. Ah well.
+ als->device = device;
+ mutex_init(&als->lock);
+
+ iio->name = ACPI_ALS_DEVICE_NAME;
+ iio->dev.parent = &device->dev;
+ iio->info = &acpi_als_info;
+ iio->modes = INDIO_DIRECT_MODE;
+ iio->channels = acpi_als_channels;
+ iio->num_channels = ARRAY_SIZE(acpi_als_channels);
+
+ ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
+ &acpi_als_trigger_handler, NULL);
+ if (ret)
+ return ret;
+
I think I'd marginally prefer if the allocation of the trigger was
done outside this function as it would then give us nice symmetric
trigger_init and trigger_remove. Actually rename the init as
acpi_als_trigger_init_and_register to make the content obvious.
+ ret = acpi_als_trigger_init(iio);
+ if (ret)
+ goto err_trig;
+
+ ret = iio_device_register(iio);
+ if (ret < 0)
+ goto err_dev;
+
+ return 0;
+
+err_dev:
+ acpi_als_trigger_remove(iio);
+err_trig:
+ iio_triggered_buffer_cleanup(iio);
+ return ret;
+}
+
+static int acpi_als_remove(struct acpi_device *device)
+{
+ struct iio_dev *iio = acpi_driver_data(device);
+
+ iio_device_unregister(iio);
+ iio_triggered_buffer_cleanup(iio);
+ acpi_als_trigger_remove(iio);
+
+ return 0;
+}
+
+static const struct acpi_device_id acpi_als_device_ids[] = {
+ {"ACPI0008", 0},
+ {"", 0},
+};
+
+MODULE_DEVICE_TABLE(acpi, acpi_als_device_ids);
+
+static struct acpi_driver acpi_als_driver = {
+ .name = "acpi_als",
+ .class = ACPI_ALS_CLASS,
+ .ids = acpi_als_device_ids,
+ .ops = {
+ .add = acpi_als_add,
+ .remove = acpi_als_remove,
+ .notify = acpi_als_notify,
+ },
+};
+
+module_acpi_driver(acpi_als_driver);
+
+MODULE_AUTHOR("Zhang Rui <rui.zhang@xxxxxxxxx>");
+MODULE_AUTHOR("Martin Liska <marxin.liska@xxxxxxxxx>");
+MODULE_AUTHOR("Marek Vasut <marex@xxxxxxx>");
+MODULE_DESCRIPTION("ACPI Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
