Re: ACPI ambient light sensor

[Jonathan Cameron <jic23@kernel.org>]

On 10/21/2012 06:02 PM, Martin Liška wrote:
> Hello,
>    my kernel driver is still unable to start capturing in a proper way. First step for listening is
>
> /* echo 1 > /sys/bus/iio/devices/iio\:device0/scan_elements/in_illuminance1_en*/
> /
> /
> but
>
> /*cat /sys/bus/iio/devices/iio\:device0/scan_elements/in_illuminance1_en*/
> /*-1*/

That is 'odd' to put it lightly.  That should just call iio_scan_el_show which
in turn calls test_bit on a bitmask.  I'm not sure that can return anything other
than 0 or 1. This is then fed directly into an sprintf call.
Could there be a permissions issue or something similar?

> /*
> */
> I tried to decorate all result codes with printf.
> Dmesg dump:
> /[    0.927335] XXX: trigger init called/
> /[    0.928148] XXX: acpi_als_allocate_trigger: 0/
> /[    0.928275] XXX: acpi_als_trigger_init: 0/
> /[    3.255305] XXX: getting data for filling buffer/
> /[    3.255352] XXX: buffer is ready/
> /[   27.423650] XXX: getting data for filling buffer/
> /[   27.423698] XXX: buffer is ready/
> /[   30.444120] XXX: getting data for filling buffer/
>
> Do you have any advices how to figure out where is problem?
Yes, put some printk's into the core code and see what is going wrong.
Right now that cat giving you -1 is certainly suspicious.

Please do get your email client to handle patches nicely!
>
> Thank you,
> Martin
>
>
> /*
>  * ACPI Ambient Light Sensor Driver
>  *
>  * 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 <trace/events/printk.h>
> #include <acpi/acpi_bus.h>
> #include <acpi/acpi_drivers.h>
> #include <linux/err.h>
> #include <linux/mutex.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/buffer.h>
> #include <linux/iio/sysfs.h>
> #include <linux/iio/trigger.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
>
> #define PREFIX "ACPI: "
>
> #define ACPI_ALS_CLASS"als"
> #define ACPI_ALS_DEVICE_NAME"acpi-als"
> #define ACPI_ALS_NOTIFY_ILLUMINANCE0x80
> #define ACPI_ALS_NOTIFY_COLOR_TEMP0x81
> #define ACPI_ALS_NOTIFY_RESPONSE0x82
>
> #define ACPI_ALS_OUTPUTS3
>
> #define _COMPONENTACPI_ALS_COMPONENT
> ACPI_MODULE_NAME("acpi-als");
>
> MODULE_AUTHOR("Martin Liska");
> MODULE_DESCRIPTION("ACPI Ambient Light Sensor Driver");
> MODULE_LICENSE("GPL");
>
> struct acpi_als_chip {
> struct acpi_device*device;
> struct acpi_als_device*acpi_als_sys;
> struct mutexlock;
> struct iio_trigger*trig;
>
> int illuminance;
> int temperature;
> int chromaticity;
> int polling;
>
> int count;
> struct acpi_als_mapping *mappings;
> };
>
> static int acpi_als_add(struct acpi_device *device);
> static int acpi_als_remove(struct acpi_device *device, int type);
> static void acpi_als_notify(struct acpi_device *device, u32 event);
>
> 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,
> },
> };
>
> struct acpi_als_mapping {
> int adjustment;
> int illuminance;
> };
>
> #define ALS_INVALID_VALUE_LOW0
> #define ALS_INVALID_VALUE_HIGH-1
>
> /* --------------------------------------------------------------------------
> Ambient Light Sensor device Management
>    -------------------------------------------------------------------------- */
>
> /*
>  * acpi_als_get_illuminance - get the current ambient light illuminance
>  */
> static int acpi_als_get_illuminance(struct acpi_als_chip *als)
> {
> acpi_status status;
> unsigned long long illuminance;
>
> status = acpi_evaluate_integer(als->device->handle,
> "_ALI", NULL, &illuminance);
>
> if (ACPI_FAILURE(status)) {
> ACPI_EXCEPTION((AE_INFO, status, "Error reading ALS illuminance"));
> als->illuminance = ALS_INVALID_VALUE_LOW;
> return -ENODEV;
> }
> als->illuminance = illuminance;
>
> return 0;
> }
>
> /*
>  * acpi_als_get_color_chromaticity - get the ambient light color chromaticity
>  */
> static int acpi_als_get_color_chromaticity(struct acpi_als_chip *chip)
> {
> acpi_status status;
> unsigned long long chromaticity;
>
> status =
>     acpi_evaluate_integer(chip->device->handle, "_ALC", NULL,
>   &chromaticity);
> if (ACPI_FAILURE(status)) {
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "_ALC not available\n"));
> return -ENODEV;
> }
> chip->chromaticity = chromaticity;
> return 0;
> }
>
> /*
>  * acpi_als_get_color_temperature - get the ambient light color temperature
>  */
> static int acpi_als_get_color_temperature(struct acpi_als_chip *chip)
> {
> acpi_status status;
> unsigned long long temperature;
>
> status =
>     acpi_evaluate_integer(chip->device->handle, "_ALT", NULL,
>   &temperature);
> if (ACPI_FAILURE(status)) {
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "_ALT not available\n"));
> return -ENODEV;
> }
> chip->temperature = temperature;
> return 0;
> }
>
> /*
>  * acpi_als_get_mappings - get the ALS illuminance mappings
>  *
>  * Return a package of ALS illuminance to display adjustment mappings
>  * that can be used by OS to calibrate its ambient light policy
>  * for a given sensor configuration.
>  */
> static int acpi_als_get_mappings(struct acpi_als_chip *chip)
> {
> int result = 0;
> acpi_status status;
> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> union acpi_object *alr;
> int i, j;
>
> /* Free the old mappings */
> kfree(chip->mappings);
> chip->mappings = NULL;
>
> status =
>     acpi_evaluate_object(chip->device->handle, "_ALR", NULL, &buffer);
> if (ACPI_FAILURE(status)) {
> ACPI_EXCEPTION((AE_INFO, status, "Error reading ALS mappings"));
> return -ENODEV;
> }
>
> alr = buffer.pointer;
> if (!alr || (alr->type != ACPI_TYPE_PACKAGE)) {
> printk(KERN_ERR PREFIX "Invalid _ALR data\n");
> result = -EFAULT;
> goto end;
> }
>
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d illuminance mappings\n",
>   alr->package.count));
>
> chip->count = alr->package.count;
>
> if (!chip->count)
> return 0;
>
> chip->mappings =
>     kmalloc(sizeof(struct acpi_als_mapping) * chip->count, GFP_KERNEL);
> if (!chip->mappings) {
> result = -ENOMEM;
> goto end;
> }
>
> for (i = 0, j = 0; i < chip->count; i++) {
> struct acpi_als_mapping *mapping = &(chip->mappings[j]);
> union acpi_object *element = &(alr->package.elements[i]);
>
> if (element->type != ACPI_TYPE_PACKAGE)
> continue;
>
> if (element->package.count != 2)
> continue;
>
> if (element->package.elements[0].type != ACPI_TYPE_INTEGER ||
>     element->package.elements[1].type != ACPI_TYPE_INTEGER)
> continue;
>
> mapping->adjustment =
>     element->package.elements[0].integer.value;
> mapping->illuminance =
>     element->package.elements[1].integer.value;
> j++;
>
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Mapping [%d]: "
>   "adjustment [%d] illuminance[%d]\n",
>   i, mapping->adjustment,
>   mapping->illuminance));
> }
>
> end:
> kfree(buffer.pointer);
> return result;
> }
>
> /*
>  * acpi_als_get_polling - get a recommended polling frequency
>  *   for the Ambient Light Sensor device
>  */
> static int acpi_als_get_polling(struct acpi_als_chip *chip)
> {
> acpi_status status;
> unsigned long long polling;
>
> status =
>     acpi_evaluate_integer(chip->device->handle, "_ALP", NULL, &polling);
> if (ACPI_FAILURE(status)) {
> ACPI_DEBUG_PRINT((ACPI_DB_INFO, "_ALP not available\n"));
> return -ENODEV;
> }
>
> chip->polling = polling;
> return 0;
> }
>
> static int get_illuminance(struct acpi_als_chip *als, int *illuminance)
> {
> int result;
>
> result = acpi_als_get_illuminance(als);
> if (!result)
> *illuminance = als->illuminance;
>
> return result;
> }
>
> static void acpi_als_notify(struct acpi_device *device, u32 event)
> {
> int illuminance;
> struct iio_dev *indio_dev = acpi_driver_data(device);
>         struct iio_buffer *buffer = indio_dev->buffer;
> struct acpi_als_chip *chip = iio_priv(indio_dev);
>         s64 time_ns = iio_get_time_ns();
> int len = 2;
>
> u8 data[sizeof(s64) + len];
>
> printk("XXX: getting data for filling buffer\n");
> get_illuminance(chip, &illuminance);
> *(int *)((u8 *)data) = illuminance;
> *(s64 *)((u8 *)data + ALIGN(len, sizeof(s64))) = time_ns;
>
> printk("XXX: buffer is ready\n");
>
>         if (iio_buffer_enabled(indio_dev)) {
> printk("XXX: pushing to buffer\n");
> iio_push_to_buffer(buffer, data);
> printk("XXX: push to buffer called\n");
> }
> }
>
> static int acpi_als_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> {
> struct acpi_als_chip *chip = iio_priv(indio_dev);
> int ret = -EINVAL;
>
> mutex_lock(&chip->lock);
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> case IIO_CHAN_INFO_PROCESSED:
> switch(chan->type) {
> case IIO_LIGHT:
> ret = get_illuminance(chip, val);
> break;
> default:
> break;
> }
>
> if(!ret)
> ret = IIO_VAL_INT;
>
> break;
> default:
> dev_err(&chip->device->dev, "mask value 0x%08lx not supported\n", mask);
> break;
> }
> mutex_unlock(&chip->lock);
>
> return ret;
> }
>
> static const struct iio_chan_spec acpi_als_channels[] = {
> {
> .type = IIO_LIGHT,
> .indexed = 1,
> .channel = 1,
> .scan_type.sign = 'u',
> .scan_type.realbits = 10,
> .scan_type.storagebits = 16,
Doesn't matter as only one channel, but I'd prefer to see
the scan_index specified here.
> .info_mask = IIO_CHAN_INFO_PROCESSED_SEPARATE_BIT |
> IIO_CHAN_INFO_SCALE_SEPARATE_BIT,
> },
> };
>
> static const struct iio_info acpi_als_info = {
> .driver_module = THIS_MODULE,
> .read_raw = &acpi_als_read_raw,
> .write_raw = NULL,
> };
>
> static irqreturn_t acpi_als_trigger_handler(int irq, void *p)
> {
>
> struct iio_poll_func *pf = p;
> struct iio_dev *idev = pf->indio_dev;
>
>
> struct acpi_als_chip *chip = iio_priv(idev);
> struct iio_buffer *buffer = idev->buffer;
> int i, j = 0;
>
> printk("XXX: TRIGGER handler called :)");
> iio_trigger_notify_done(chip->trig);
> return IRQ_HANDLED;
>
>
> /* TODO: remove */
> u8 b[64];
>
> for (i = 0; i < idev->masklength; i++) {
> if (!test_bit(i, idev->active_scan_mask))
> continue;
>
> // TODO: read
> j++;
> }
>
> if (idev->scan_timestamp) {
> s64 *timestamp = (s64 *)((u8 *)b +
> ALIGN(j, sizeof(s64)));
> *timestamp = pf->timestamp;
> }
>
> //buffer->access->store_to(buffer, (u8 *)b, pf->timestamp);
Well not storing to the buffer is certainly not going to help.
Also there is a iio_push_to_buffer for this.
>
> iio_trigger_notify_done(idev->trig);
> return IRQ_HANDLED;
> }
>
> /**
>  * acpi_als_data_rdy_trigger_set_state() set datardy interrupt state
>  **/
> static int acpi_als_data_rdy_trigger_set_state(struct iio_trigger *trig,
> bool state)
> {
> printk("XXX: set_state called\n");
>
> struct iio_dev *indio_dev = trig->private_data;
>
> dev_dbg(&indio_dev->dev, "%s (%d)\n", __func__, state);
>
> return 0;
> }
>
> static const struct iio_trigger_ops acpi_als_trigger_ops = {
> .owner = THIS_MODULE,
> .set_trigger_state = &acpi_als_data_rdy_trigger_set_state
> };
>
> // TODO
> static const struct iio_buffer_setup_ops acpi_als_buffer_ops = {
> .preenable = &iio_sw_buffer_preenable, // TODO ?
> .postenable = &iio_triggered_buffer_postenable,
> .predisable = &iio_triggered_buffer_predisable,
> };
>
> static struct iio_trigger *acpi_als_allocate_trigger(struct iio_dev *idev)
> {
> printk("XX: allocate trigger called!\n");
>
> int ret;
> struct iio_trigger *trig;
> struct acpi_als_chip *chip = iio_priv(idev);
>
> trig = iio_trigger_alloc("acpi-als");
> if (trig == NULL)
> return NULL;
>
> trig->dev.parent = idev->dev.parent;
> trig->private_data = idev;
> trig->ops = &acpi_als_trigger_ops;
>
> ret = iio_trigger_register(trig);
> if (ret)
> return NULL;
>
> printk("XXX: acpi_als_allocate_trigger: %d\n", ret);
> return trig;
> }
>
> static int acpi_als_trigger_init(struct iio_dev *idev)
> {
> printk("XXX: trigger init called\n");
>
> struct acpi_als_chip *chip = iio_priv(idev);
> int i, ret;
>
> chip->trig = devm_kzalloc(&idev->dev, sizeof(chip->trig), GFP_KERNEL);
>
> if (chip->trig == NULL) {
> ret = -ENOMEM;
> goto error_ret;
> }
>
> chip->trig = acpi_als_allocate_trigger(idev);
> if (chip->trig == NULL) {
> dev_err(&idev->dev, "Could not allocate trigger %d\n");
> ret = -ENOMEM;
> goto error_trigger;
> }
>
> printk("XXX: acpi_als_trigger_init: 0\n");
> return 0;
>
> error_trigger:
> iio_trigger_free(chip->trig);
> error_ret:
> return ret;
> }
>
>
> static int acpi_als_add(struct acpi_device *device)
> {
> int result;
> struct acpi_als_chip *chip;
> struct iio_dev *indio_dev;
>
> /*
> if (unlikely(als_id >= 10)) {
> printk(KERN_WARNING PREFIX "Too many ALS device found\n");
> return -ENODEV;
> }
> */
> indio_dev = iio_device_alloc(sizeof(*chip));
> if (!indio_dev) {
> dev_err(&device->dev, "iio allocation fails\n");
> return -ENOMEM;
> }
>
> chip = iio_priv(indio_dev);
>
> device->driver_data = indio_dev;
> chip->device = device;
> mutex_init(&chip->lock);
>
> indio_dev->info = &acpi_als_info;
> indio_dev->channels = acpi_als_channels;
> indio_dev->num_channels = ARRAY_SIZE(acpi_als_channels);
> indio_dev->name = ACPI_ALS_DEVICE_NAME;
> indio_dev->dev.parent = &device->dev;
> indio_dev->modes = INDIO_DIRECT_MODE;
>
> result = iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
> &acpi_als_trigger_handler, NULL);
>
> if(result) {
> printk("Could not setup buffer for iio device\n");
> goto exit_iio_free;
> }
>
> result = acpi_als_trigger_init(indio_dev);
> if (result) {
> printk("Couldn't setup the triggers.\n");
> // TODO
> //goto error_unregister_buffer;
> }
>
> result = iio_device_register(indio_dev);
> if (result < 0) {
> dev_err(&chip->device->dev, "iio registration fails with error %d\n",
> result);
> goto exit_iio_free;
> }
>
> printk("ACPI ALS initialized");
>
> return 0;
>
> exit_iio_free:
> iio_device_free(indio_dev);
> return result;
> }
>
> static int acpi_als_remove(struct acpi_device *device, int type)
> {
> struct iio_dev *indio_dev;
>
> indio_dev = acpi_driver_data(device);
> if(!indio_dev) {
> dev_err(&device->dev, "could not get indio_dev for ACPI device\n");
> return -1;
> }
>
> iio_device_unregister(indio_dev);
Not relevant to your problem, but you need to unregister the buffer.

> iio_device_free(indio_dev);
>
> return 0;
> }
>
> static int __init acpi_als_init(void)
> {
> return acpi_bus_register_driver(&acpi_als_driver);
> }
>
> static void __exit acpi_als_exit(void)
> {
> acpi_bus_unregister_driver(&acpi_als_driver);
> }
>
> module_init(acpi_als_init);
> module_exit(acpi_als_exit);
>
> On 11 September 2012 11:21, Marek Vasut <marex@denx.de <mailto:marex@denx.de>> wrote:
>
>     Dear Martin Liška,
>
>     [...]
>
>     > +static int acpi_als_write_raw(struct iio_dev *indio_dev,
>     > +     struct iio_chan_spec const *chan, int val, int val2, long mask)
>     > +{
>     > + return 0;
>     > +}
>
>     Simply set write_raw = NULL below (aka. don't put it into the structure at all).
>
>     > +static const struct iio_chan_spec acpi_als_channels[] = {
>     > + {
>     > + .type = IIO_LIGHT,
>     > + .indexed = 1,
>     > + .channel = 1,
>     > + .scan_type.sign = 'u',
>     > + .scan_type.realbits = 10,
>     > + .scan_type.storagebits = 16,
>     > + .info_mask = IIO_CHAN_INFO_PROCESSED_SEPARATE_BIT |
>     > + IIO_CHAN_INFO_SCALE_SEPARATE_BIT,
>     > + },
>     > +};
>     > +
>     > +static const struct iio_info acpi_als_info = {
>     > + .driver_module = THIS_MODULE,
>     > + .read_raw = &acpi_als_read_raw,
>     > + .write_raw = &acpi_als_write_raw,
>     > +};
>     [...]
>     Best regards,
>     Marek Vasut
>
>