Re: ACPI ambient light sensor

Re: ACPI ambient light sensor

[Marek Vasut]

Dear Martin Liška,

> Hello,
>    Iconia Tab W500 contains Ambient Light Sensor (hid 0008), I started
> implementing IIO device drive having following questions:
> 
>    - chip could offer following variables: illuminance, color chromaticity
>    and color temperature, how should I connect last 2 vars to any of IIO
>    CHANNEL types?
>    - I'm not sure how to connect an illuminance mapping to IIO
>    infrastructure?

See IIO_DEVICE_ATTR() maybe ?

> Thank you,
> Martin Liška

Best regards,
Marek Vasut

Re: ACPI ambient light sensor

[Jonathan Cameron]

cc'd Jon Brenner and Peter Meerwald.
>
>> Hello,
>>     Iconia Tab W500 contains Ambient Light Sensor (hid 0008), I started
>> implementing IIO device drive having following questions:
>>
>>     - chip could offer following variables: illuminance, color chromaticity
>>     and color temperature, how should I connect last 2 vars to any of IIO
>>     CHANNEL types?
Jon's recent proposed additional modifiers cover temperature. 
Chromaticity is a new one I think though?
>>     - I'm not sure how to connect an illuminance mapping to IIO
>>     infrastructure?
When you say mapping, what do you mean? That term tends to get used
for quite a few different things...
>
> See IIO_DEVICE_ATTR() maybe ?
>
>> Thank you,
>> Martin Liška
>
> Best regards,
> Marek Vasut
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: ACPI ambient light sensor

[Marek Vasut]

Dear Martin Liška,

[...]

> +
> + if (kfifo_is_empty(&kf->kf))
> + r->stufftoread = false;
> + /* verify it is still empty to avoid race */
> + if (!kfifo_is_empty(&kf->kf))
> + r->stufftoread = true;

I'd say the formating got pretty messed up, please repost the patch via git 
send-email .

[...]
Best regards,
Marek Vasut

Re: ACPI ambient light sensor

[Marek Vasut]

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

Re: ACPI ambient light sensor

[Marek Vasut]

Dear Martin Liška,

> 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*
> *
> *
> 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?
> 
> 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_ILLUMINANCE 0x80
> #define ACPI_ALS_NOTIFY_COLOR_TEMP 0x81
> #define ACPI_ALS_NOTIFY_RESPONSE 0x82
> 
> #define ACPI_ALS_OUTPUTS 3
> 
> #define _COMPONENT ACPI_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 mutex lock;
> 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,
> },
> };
[...]

... again ...

_Please_ fix your mailer if you want to submit files inline, the indent is 
completely fucked. This behavior of yours definitelly does _NOT_ help anyone, it 
only puts unnecessary burden on the reader!

Re: ACPI ambient light sensor

[Jonathan Cameron]

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-ynQEQJNshbs@public.gmane.org <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
>
>

Re: ACPI ambient light sensor

[Jonathan Cameron]

On 10/27/2012 05:39 PM, Martin Liška wrote:
> Hello,
>    I hope being root should not bring any permission issues for the file:
> 
> ls -l /sys/bus/iio/devices/iio\:device0/scan_elements/in_illuminance1_en 
> *-rw-r--r--* 1 root root 4096 Oct 27 15:54 /sys/bus/iio/devices/iio:device0/scan_elements/in_illuminance1_en
> 
> After adding debug print to ii_scan_el_show gives me really *-1* and ***indio_dev->buffer->scan_mask is *1 *and echo
> command settings "1" is processed in *in_scan_el_store* function with ret == 0.
Chase it all the way back and find out where that -1 is coming form. I really can't
guess at the moment. There is only a test_bit and an sprintf if there... Not much scope
really. If it's giving a  string containing -1 then it must be from test bit, if returning
-1 from the sprintf.  I didn't think test_bit could return an error so we don't check
ret from that.

I'll admit this one has me thoroughly confused...

> 
> Thank you for new ideas,
> Martin
> 
> On 21 October 2012 20:05, Jonathan Cameron <jic23@kernel.org <mailto:jic23@kernel.org>> wrote:
> 
>     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> <mailto: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
>     >
>     >
> 
> 

Re: ACPI ambient light sensor

[Corentin Chary]

On Sat, Oct 27, 2012 at 6:08 PM, Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On 10/27/2012 05:39 PM, Martin Liška wrote:
>> Hello,
>>    I hope being root should not bring any permission issues for the file:
>>
>> ls -l /sys/bus/iio/devices/iio\:device0/scan_elements/in_illuminance1_en
>> *-rw-r--r--* 1 root root 4096 Oct 27 15:54 /sys/bus/iio/devices/iio:device0/scan_elements/in_illuminance1_en
>>
>> After adding debug print to ii_scan_el_show gives me really *-1* and ***indio_dev->buffer->scan_mask is *1 *and echo
>> command settings "1" is processed in *in_scan_el_store* function with ret == 0.
> Chase it all the way back and find out where that -1 is coming form. I really can't
> guess at the moment. There is only a test_bit and an sprintf if there... Not much scope
> really. If it's giving a  string containing -1 then it must be from test bit, if returning
> -1 from the sprintf.  I didn't think test_bit could return an error so we don't check
> ret from that.

Try to add some printk inside iio code directly, including iio_scan_el_show.
You can also build you kernel with debug symbols and trace support,
and use ftrace to trace the kernel calls:
http://lwn.net/Articles/365835/.

-- 
Corentin Chary
http://xf.iksaif.net

ACPI ALS patch

[marxin.liska]

Attached patch for ACPI ALS using iio subsystem handles all events in ACPI notify event handler. I would like to ask you how to modify iio trigger which is in fact not used and just initialized with buffer?

[PATCH] ACPI ALS driver for iio introduced.

[marxin.liska]

From: marxin <marxin.liska@gmail.com>

---
 drivers/iio/industrialio-buffer.c    |    4 +-
 drivers/staging/iio/light/Kconfig    |    6 +
 drivers/staging/iio/light/Makefile   |    1 +
 drivers/staging/iio/light/acpi-als.c |  486 ++++++++++++++++++++++++++++++++++
 4 files changed, 495 insertions(+), 2 deletions(-)
 create mode 100644 drivers/staging/iio/light/acpi-als.c

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index aaadd32..b8b377c 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -119,8 +119,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
 	int ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 
-	ret = test_bit(to_iio_dev_attr(attr)->address,
-		       indio_dev->buffer->scan_mask);
+	ret = !!(test_bit(to_iio_dev_attr(attr)->address,
+		       indio_dev->buffer->scan_mask));
 
 	return sprintf(buf, "%d\n", ret);
 }
diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
index 4bed30e..a164ecc 100644
--- a/drivers/staging/iio/light/Kconfig
+++ b/drivers/staging/iio/light/Kconfig
@@ -50,4 +50,10 @@ config TSL2x7x
 	 tmd2672, tsl2772, tmd2772 devices.
 	 Provides iio_events and direct access via sysfs.
 
+config ACPI_ALS
+	tristate "ACPI Ambient Light Sensor"
+	help
+	 Support for ACPI0008 Light Sensor.
+	 Provides direct access via sysfs.
+
 endmenu
diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
index 141af1e..13090e6 100644
--- a/drivers/staging/iio/light/Makefile
+++ b/drivers/staging/iio/light/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_SENSORS_ISL29018)	+= isl29018.o
 obj-$(CONFIG_SENSORS_ISL29028)	+= isl29028.o
 obj-$(CONFIG_TSL2583)	+= tsl2583.o
 obj-$(CONFIG_TSL2x7x)	+= tsl2x7x_core.o
+obj-$(CONFIG_ACPI_ALS)	+= acpi-als.o
diff --git a/drivers/staging/iio/light/acpi-als.c b/drivers/staging/iio/light/acpi-als.c
new file mode 100644
index 0000000..9ba0fc4
--- /dev/null
+++ b/drivers/staging/iio/light/acpi-als.c
@@ -0,0 +1,486 @@
+/*
+ * 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_ILLUMINANCE	0x80
+
+#define ACPI_ALS_OUTPUTS		1
+
+#define _COMPONENT			ACPI_ALS_COMPONENT
+ACPI_MODULE_NAME("acpi-als");
+
+MODULE_AUTHOR("Martin Liska <marxin.liska@gmail.com>");
+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 mutex		lock;
+    struct iio_trigger		*trig;
+
+    int				illuminance;
+    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_LOW		0
+#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_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;
+}
+
+/*
+ * get_illuminance - wrapper for getting the currect ambient light illuminance
+ */
+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 acpi_als_chip *chip = iio_priv(indio_dev);
+
+	s64 time_ns = iio_get_time_ns();
+	int len = sizeof(int);
+	u8 data[sizeof(s64) + ACPI_ALS_OUTPUTS * len];
+
+	switch(event) {
+	case ACPI_ALS_NOTIFY_ILLUMINANCE:
+		get_illuminance(chip, &illuminance);
+		*(int *)((u8 *)data) = illuminance;
+		*(s64 *)((u8 *)data + ALIGN(len, sizeof(s64))) = time_ns;
+		break;
+	default:
+		return;
+	}
+
+	if (iio_buffer_enabled(indio_dev))
+		iio_push_to_buffers(indio_dev, data);
+
+	return;
+}
+
+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,
+        .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);
+
+    printk("XXX: TRIGGER handler called :)");
+    iio_trigger_notify_done(chip->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)
+{
+
+    struct iio_dev *indio_dev = trig->private_data;
+    printk("XXX: set_state called\n");
+
+    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) {
+    int ret;
+    struct iio_trigger *trig;
+
+    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)
+{
+    struct acpi_als_chip *chip = iio_priv(idev);
+    int 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!\n");
+        ret = -ENOMEM;
+        goto error_trigger;
+    }
+
+    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);
+    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);
-- 
1.7.8.6

Re: [PATCH] ACPI ALS driver for iio introduced.

[Corentin Chary]

On Thu, Nov 29, 2012 at 2:46 AM,  <marxin.liska@gmail.com> wrote:
> From: marxin <marxin.liska@gmail.com>
>
> ---
>  drivers/iio/industrialio-buffer.c    |    4 +-
>  drivers/staging/iio/light/Kconfig    |    6 +
>  drivers/staging/iio/light/Makefile   |    1 +
>  drivers/staging/iio/light/acpi-als.c |  486 ++++++++++++++++++++++++++++++++++
>  4 files changed, 495 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/staging/iio/light/acpi-als.c
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index aaadd32..b8b377c 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -119,8 +119,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
>         int ret;
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>
> -       ret = test_bit(to_iio_dev_attr(attr)->address,
> -                      indio_dev->buffer->scan_mask);
> +       ret = !!(test_bit(to_iio_dev_attr(attr)->address,
> +                      indio_dev->buffer->scan_mask));

It's strange that you have to do that.
Can you give use the exact values that make test_bit return something
else that 1 or 0 ?
Maybe there is a bug to fix in test_bit here.

>
>         return sprintf(buf, "%d\n", ret);
>  }
> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
> index 4bed30e..a164ecc 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -50,4 +50,10 @@ config TSL2x7x
>          tmd2672, tsl2772, tmd2772 devices.
>          Provides iio_events and direct access via sysfs.
>
> +config ACPI_ALS
> +       tristate "ACPI Ambient Light Sensor"
> +       help
> +        Support for ACPI0008 Light Sensor.
> +        Provides direct access via sysfs.
> +
>  endmenu
> diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
> index 141af1e..13090e6 100644
> --- a/drivers/staging/iio/light/Makefile
> +++ b/drivers/staging/iio/light/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_SENSORS_ISL29018)  += isl29018.o
>  obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
>  obj-$(CONFIG_TSL2583)  += tsl2583.o
>  obj-$(CONFIG_TSL2x7x)  += tsl2x7x_core.o
> +obj-$(CONFIG_ACPI_ALS) += acpi-als.o
> diff --git a/drivers/staging/iio/light/acpi-als.c b/drivers/staging/iio/light/acpi-als.c
> new file mode 100644
> index 0000000..9ba0fc4
> --- /dev/null
> +++ b/drivers/staging/iio/light/acpi-als.c
> @@ -0,0 +1,486 @@
> +/*
> + * 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_ILLUMINANCE    0x80
> +
> +#define ACPI_ALS_OUTPUTS               1
> +
> +#define _COMPONENT                     ACPI_ALS_COMPONENT
> +ACPI_MODULE_NAME("acpi-als");
> +
> +MODULE_AUTHOR("Martin Liska <marxin.liska@gmail.com>");
> +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 mutex               lock;
> +    struct iio_trigger         *trig;
> +
> +    int                                illuminance;
> +    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_LOW          0
> +#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_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;
> +}
> +
> +/*
> + * get_illuminance - wrapper for getting the currect ambient light illuminance
> + */
> +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 acpi_als_chip *chip = iio_priv(indio_dev);
> +
> +       s64 time_ns = iio_get_time_ns();
> +       int len = sizeof(int);
> +       u8 data[sizeof(s64) + ACPI_ALS_OUTPUTS * len];
> +
> +       switch(event) {
> +       case ACPI_ALS_NOTIFY_ILLUMINANCE:
> +               get_illuminance(chip, &illuminance);
> +               *(int *)((u8 *)data) = illuminance;
> +               *(s64 *)((u8 *)data + ALIGN(len, sizeof(s64))) = time_ns;
> +               break;
> +       default:
> +               return;
> +       }
> +
> +       if (iio_buffer_enabled(indio_dev))
> +               iio_push_to_buffers(indio_dev, data);
> +
> +       return;
> +}
> +
> +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,
> +        .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);
> +
> +    printk("XXX: TRIGGER handler called :)");
> +    iio_trigger_notify_done(chip->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)
> +{
> +
> +    struct iio_dev *indio_dev = trig->private_data;
> +    printk("XXX: set_state called\n");
> +
> +    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) {
> +    int ret;
> +    struct iio_trigger *trig;
> +
> +    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)
> +{
> +    struct acpi_als_chip *chip = iio_priv(idev);
> +    int 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!\n");
> +        ret = -ENOMEM;
> +        goto error_trigger;
> +    }
> +
> +    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);
> +    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);
> --
> 1.7.8.6
>

Looks like "adjustment" is not exposed anymore. Is there a reason for
that ? adjustment is in my opinion more important than illuminance
because you can directly feed the backlight with that.
Any way to expose the mappings too ? Is this supposed to come in a
further patch ?

--
Corentin Chary
http://xf.iksaif.net

Re: [PATCH] ACPI ALS driver for iio introduced.

[Lars-Peter Clausen]

On 11/29/2012 09:02 AM, Corentin Chary wrote:
> On Thu, Nov 29, 2012 at 2:46 AM,  <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: marxin <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> ---
>>  drivers/iio/industrialio-buffer.c    |    4 +-
>>  drivers/staging/iio/light/Kconfig    |    6 +
>>  drivers/staging/iio/light/Makefile   |    1 +
>>  drivers/staging/iio/light/acpi-als.c |  486 ++++++++++++++++++++++++++++++++++
>>  4 files changed, 495 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/staging/iio/light/acpi-als.c
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index aaadd32..b8b377c 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -119,8 +119,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
>>         int ret;
>>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>
>> -       ret = test_bit(to_iio_dev_attr(attr)->address,
>> -                      indio_dev->buffer->scan_mask);
>> +       ret = !!(test_bit(to_iio_dev_attr(attr)->address,
>> +                      indio_dev->buffer->scan_mask));
> 
> It's strange that you have to do that.
> Can you give use the exact values that make test_bit return something
> else that 1 or 0 ?
> Maybe there is a bug to fix in test_bit here.

Even if this was a bug in IIO this should be in a separate patch.

> 
>>
>>         return sprintf(buf, "%d\n", ret);
>>  }
[...]

Re: [PATCH] ACPI ALS driver for iio introduced.

[Corentin Chary]

>
> Yeah, it's really strange. Following piece of code is called in
> non-atomic.h:
>
> static inline int test_bit(int nr, const volatile unsigned long *addr)
> {
> return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
> }
>
> Where nr == 0 and *addr == 1, giving me really -1 return value. If I am
> right it's something like: 1 >> 0 ?
>
> You are right, "adjustment" should be major observed variable. I could
> create a channel where adjustment will be visible according to given mapping
> table. Do you have any idea how to expose mentioned mapping table? Both will
> be added in further patch.

I don't see how test_bit(0, (long[]){1}) could return -1.
Are you sure of these values ? Could you add some printk before and
after the test, and show us the trace ?


--
Corentin Chary
http://xf.iksaif.net

Re: [PATCH] ACPI ALS driver for iio introduced.

[Lars-Peter Clausen]

Hi,

On 11/29/2012 03:46 AM, marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: marxin <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

The from tag should contain your full name. Also you need to add a
Signed-off-by tag. And a short description what the patch does wouldn't hurt
either. And the subject line prefix should match that of the subsystem, in
this case "iio:". Your subject line could for example be "iio: Add ACPI
ambient light sensor driver".

Also what's up with all the TODOs in the driver, shouldn't these be
addressed first before the driver is merged upstream? The driver looks a bit
as if it is only half finished, which is fine if you just want to get some
initial feedback, but you should definitely state this somewhere.

> 
> ---
>  drivers/iio/industrialio-buffer.c    |    4 +-
>  drivers/staging/iio/light/Kconfig    |    6 +
>  drivers/staging/iio/light/Makefile   |    1 +
>  drivers/staging/iio/light/acpi-als.c |  486 ++++++++++++++++++++++++++++++++++
>  4 files changed, 495 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/staging/iio/light/acpi-als.c
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index aaadd32..b8b377c 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -119,8 +119,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
>  	int ret;
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  
> -	ret = test_bit(to_iio_dev_attr(attr)->address,
> -		       indio_dev->buffer->scan_mask);
> +	ret = !!(test_bit(to_iio_dev_attr(attr)->address,
> +		       indio_dev->buffer->scan_mask));
>  
>  	return sprintf(buf, "%d\n", ret);
>  }
> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
> index 4bed30e..a164ecc 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig

Unless you have a really good reason we shouldn't add new iio drivers to
staging. Just put it in drivers/iio/light/

> @@ -50,4 +50,10 @@ config TSL2x7x
>  	 tmd2672, tsl2772, tmd2772 devices.
>  	 Provides iio_events and direct access via sysfs.
>  
> +config ACPI_ALS
> +	tristate "ACPI Ambient Light Sensor"

I suspect that the driver depends on CONFIG_ACPI


> +	help
> +	 Support for ACPI0008 Light Sensor.
> +	 Provides direct access via sysfs.
> +
>  endmenu
> diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
> index 141af1e..13090e6 100644
> --- a/drivers/staging/iio/light/Makefile
> +++ b/drivers/staging/iio/light/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_SENSORS_ISL29018)	+= isl29018.o
>  obj-$(CONFIG_SENSORS_ISL29028)	+= isl29028.o
>  obj-$(CONFIG_TSL2583)	+= tsl2583.o
>  obj-$(CONFIG_TSL2x7x)	+= tsl2x7x_core.o
> +obj-$(CONFIG_ACPI_ALS)	+= acpi-als.o
> diff --git a/drivers/staging/iio/light/acpi-als.c b/drivers/staging/iio/light/acpi-als.c
> new file mode 100644
> index 0000000..9ba0fc4
> --- /dev/null
> +++ b/drivers/staging/iio/light/acpi-als.c
> @@ -0,0 +1,486 @@
> +/*
> + * 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_ILLUMINANCE	0x80
> +
> +#define ACPI_ALS_OUTPUTS		1
> +
> +#define _COMPONENT			ACPI_ALS_COMPONENT
> +ACPI_MODULE_NAME("acpi-als");
> +
> +MODULE_AUTHOR("Martin Liska <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> +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;

Neither the struct is defined nor the field is used.

> +    struct mutex		lock;
> +    struct iio_trigger		*trig;
> +
> +    int				illuminance;
> +    int				polling;

Polling is only ever assigned, but never read.

> +
> +    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_LOW		0
> +#define ALS_INVALID_VALUE_HIGH		-1
> +

[...]
> +/*
> + * 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;
> +}
> +
> +/*
> + * get_illuminance - wrapper for getting the currect ambient light illuminance
> + */

Why is this wrapper necessary?

> +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 acpi_als_chip *chip = iio_priv(indio_dev);
> +
> +	s64 time_ns = iio_get_time_ns();
> +	int len = sizeof(int);
> +	u8 data[sizeof(s64) + ACPI_ALS_OUTPUTS * len];
> +
> +	switch(event) {
> +	case ACPI_ALS_NOTIFY_ILLUMINANCE:
> +		get_illuminance(chip, &illuminance);
> +		*(int *)((u8 *)data) = illuminance;
> +		*(s64 *)((u8 *)data + ALIGN(len, sizeof(s64))) = time_ns;

You don't have a timestamp channel in your channel spec.

> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		iio_push_to_buffers(indio_dev, data);
> +
> +	return;
> +}
> +
> +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:

You did not register a RAW attribute for this device.

> +    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);

You did register a scale attribute for the device, so whenever somebody
reads the scale attribute he will get this message. I don't think that makes
much sense. Better just remove IIO_CHAN_INFO_SCALE_SEPARATE_BIT from the
channel spec. And also remove this dev_err.

> +        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,
> +        .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);
> +
> +    printk("XXX: TRIGGER handler called :)");

Aha?

> +    iio_trigger_notify_done(chip->trig);
> +    return IRQ_HANDLED;
> +}
> +
[...]
> +
> +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;
> +    }
> +    */

What's with this?

> +
> +    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");

dev_err

> +        goto exit_iio_free;
> +    }
> +
> +    result = acpi_als_trigger_init(indio_dev);
> +    if (result) {
> +        printk("Couldn't setup the triggers.\n");

dev_err

> +        // 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");

This is just noise, please remove it.

> +    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) {

Can this ever happen? I suspect not.

> +        dev_err(&device->dev, "could not get indio_dev for ACPI device\n");
> +        return -1;
> +    }
> +
> +    iio_device_unregister(indio_dev);
> +    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);

Hm, there is no module_acpi_driver? We should probably add one.

Re: [PATCH] ACPI ALS driver for iio introduced.

[Martin Liška]

On 29 November 2012 11:15, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
> Hi,
>
> On 11/29/2012 03:46 AM, marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: marxin <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> The from tag should contain your full name. Also you need to add a
> Signed-off-by tag. And a short description what the patch does wouldn't hurt
> either. And the subject line prefix should match that of the subsystem, in
> this case "iio:". Your subject line could for example be "iio: Add ACPI
> ambient light sensor driver".
>
> Also what's up with all the TODOs in the driver, shouldn't these be
> addressed first before the driver is merged upstream? The driver looks a bit
> as if it is only half finished, which is fine if you just want to get some
> initial feedback, but you should definitely state this somewhere.
>
>>
>> ---
>>  drivers/iio/industrialio-buffer.c    |    4 +-
>>  drivers/staging/iio/light/Kconfig    |    6 +
>>  drivers/staging/iio/light/Makefile   |    1 +
>>  drivers/staging/iio/light/acpi-als.c |  486 ++++++++++++++++++++++++++++++++++
>>  4 files changed, 495 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/staging/iio/light/acpi-als.c
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index aaadd32..b8b377c 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -119,8 +119,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
>>       int ret;
>>       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>
>> -     ret = test_bit(to_iio_dev_attr(attr)->address,
>> -                    indio_dev->buffer->scan_mask);
>> +     ret = !!(test_bit(to_iio_dev_attr(attr)->address,
>> +                    indio_dev->buffer->scan_mask));
>>
>>       return sprintf(buf, "%d\n", ret);
>>  }
>> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
>> index 4bed30e..a164ecc 100644
>> --- a/drivers/staging/iio/light/Kconfig
>> +++ b/drivers/staging/iio/light/Kconfig
>
> Unless you have a really good reason we shouldn't add new iio drivers to
> staging. Just put it in drivers/iio/light/
>
>> @@ -50,4 +50,10 @@ config TSL2x7x
>>        tmd2672, tsl2772, tmd2772 devices.
>>        Provides iio_events and direct access via sysfs.
>>
>> +config ACPI_ALS
>> +     tristate "ACPI Ambient Light Sensor"
>
> I suspect that the driver depends on CONFIG_ACPI
>
>
>> +     help
>> +      Support for ACPI0008 Light Sensor.
>> +      Provides direct access via sysfs.
>> +
>>  endmenu
>> diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
>> index 141af1e..13090e6 100644
>> --- a/drivers/staging/iio/light/Makefile
>> +++ b/drivers/staging/iio/light/Makefile
>> @@ -7,3 +7,4 @@ obj-$(CONFIG_SENSORS_ISL29018)        += isl29018.o
>>  obj-$(CONFIG_SENSORS_ISL29028)       += isl29028.o
>>  obj-$(CONFIG_TSL2583)        += tsl2583.o
>>  obj-$(CONFIG_TSL2x7x)        += tsl2x7x_core.o
>> +obj-$(CONFIG_ACPI_ALS)       += acpi-als.o
>> diff --git a/drivers/staging/iio/light/acpi-als.c b/drivers/staging/iio/light/acpi-als.c
>> new file mode 100644
>> index 0000000..9ba0fc4
>> --- /dev/null
>> +++ b/drivers/staging/iio/light/acpi-als.c
>> @@ -0,0 +1,486 @@
>> +/*
>> + * 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_ILLUMINANCE  0x80
>> +
>> +#define ACPI_ALS_OUTPUTS             1
>> +
>> +#define _COMPONENT                   ACPI_ALS_COMPONENT
>> +ACPI_MODULE_NAME("acpi-als");
>> +
>> +MODULE_AUTHOR("Martin Liska <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
>> +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;
>
> Neither the struct is defined nor the field is used.
>
>> +    struct mutex             lock;
>> +    struct iio_trigger               *trig;
>> +
>> +    int                              illuminance;
>> +    int                              polling;
>
> Polling is only ever assigned, but never read.
>
>> +
>> +    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_LOW                0
>> +#define ALS_INVALID_VALUE_HIGH               -1
>> +
>
> [...]
>> +/*
>> + * 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;
>> +}
>> +
>> +/*
>> + * get_illuminance - wrapper for getting the currect ambient light illuminance
>> + */
>
> Why is this wrapper necessary?
>
>> +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 acpi_als_chip *chip = iio_priv(indio_dev);
>> +
>> +     s64 time_ns = iio_get_time_ns();
>> +     int len = sizeof(int);
>> +     u8 data[sizeof(s64) + ACPI_ALS_OUTPUTS * len];
>> +
>> +     switch(event) {
>> +     case ACPI_ALS_NOTIFY_ILLUMINANCE:
>> +             get_illuminance(chip, &illuminance);
>> +             *(int *)((u8 *)data) = illuminance;
>> +             *(s64 *)((u8 *)data + ALIGN(len, sizeof(s64))) = time_ns;
>
> You don't have a timestamp channel in your channel spec.
>
>> +             break;
>> +     default:
>> +             return;
>> +     }
>> +
>> +     if (iio_buffer_enabled(indio_dev))
>> +             iio_push_to_buffers(indio_dev, data);
>> +
>> +     return;
>> +}
>> +
>> +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:
>
> You did not register a RAW attribute for this device.
>
>> +    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);
>
> You did register a scale attribute for the device, so whenever somebody
> reads the scale attribute he will get this message. I don't think that makes
> much sense. Better just remove IIO_CHAN_INFO_SCALE_SEPARATE_BIT from the
> channel spec. And also remove this dev_err.
>
>> +        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,
>> +        .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);
>> +
>> +    printk("XXX: TRIGGER handler called :)");
>
> Aha?
>
>> +    iio_trigger_notify_done(chip->trig);
>> +    return IRQ_HANDLED;
>> +}
>> +
> [...]
>> +
>> +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;
>> +    }
>> +    */
>
> What's with this?
>
>> +
>> +    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");
>
> dev_err
>
>> +        goto exit_iio_free;
>> +    }
>> +
>> +    result = acpi_als_trigger_init(indio_dev);
>> +    if (result) {
>> +        printk("Couldn't setup the triggers.\n");
>
> dev_err
>
>> +        // 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");
>
> This is just noise, please remove it.
>
>> +    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) {
>
> Can this ever happen? I suspect not.
>
>> +        dev_err(&device->dev, "could not get indio_dev for ACPI device\n");
>> +        return -1;
>> +    }
>> +
>> +    iio_device_unregister(indio_dev);
>> +    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);
>
> Hm, there is no module_acpi_driver? We should probably add one.
>

Hello,
   you are right. The patch is really just half finished patch (let's
call it draft). I will merge all comments you added to my patch. But
the major stuff is that I don't know how to handle iio trigger. As I
wrote before, I use acpi_notify callback for filling iio buffer.
Guideline for IIO says each iio buffer should be completed with
appropriate trigger. In my case makes not sense because I can fill the
buffer for ACPI event handler?

Thank you for advice,
Martin

Re: [PATCH] ACPI ALS driver for iio introduced.

[Jonathan Cameron]

On 12/01/2012 04:46 PM, Martin Liška wrote:
> On 29 November 2012 11:15, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>> Hi,
>>
>> On 11/29/2012 03:46 AM, marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>>> From: marxin <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> The from tag should contain your full name. Also you need to add a
>> Signed-off-by tag. And a short description what the patch does wouldn't hurt
>> either. And the subject line prefix should match that of the subsystem, in
>> this case "iio:". Your subject line could for example be "iio: Add ACPI
>> ambient light sensor driver".
>>
>> Also what's up with all the TODOs in the driver, shouldn't these be
>> addressed first before the driver is merged upstream? The driver looks a bit
>> as if it is only half finished, which is fine if you just want to get some
>> initial feedback, but you should definitely state this somewhere.
>>
>>>
>>> ---
>>>  drivers/iio/industrialio-buffer.c    |    4 +-
>>>  drivers/staging/iio/light/Kconfig    |    6 +
>>>  drivers/staging/iio/light/Makefile   |    1 +
>>>  drivers/staging/iio/light/acpi-als.c |  486 ++++++++++++++++++++++++++++++++++
>>>  4 files changed, 495 insertions(+), 2 deletions(-)
>>>  create mode 100644 drivers/staging/iio/light/acpi-als.c
>>>
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index aaadd32..b8b377c 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -119,8 +119,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
>>>       int ret;
>>>       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>
>>> -     ret = test_bit(to_iio_dev_attr(attr)->address,
>>> -                    indio_dev->buffer->scan_mask);
>>> +     ret = !!(test_bit(to_iio_dev_attr(attr)->address,
>>> +                    indio_dev->buffer->scan_mask));
>>>
>>>       return sprintf(buf, "%d\n", ret);
>>>  }
>>> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
>>> index 4bed30e..a164ecc 100644
>>> --- a/drivers/staging/iio/light/Kconfig
>>> +++ b/drivers/staging/iio/light/Kconfig
>>
>> Unless you have a really good reason we shouldn't add new iio drivers to
>> staging. Just put it in drivers/iio/light/
>>
>>> @@ -50,4 +50,10 @@ config TSL2x7x
>>>        tmd2672, tsl2772, tmd2772 devices.
>>>        Provides iio_events and direct access via sysfs.
>>>
>>> +config ACPI_ALS
>>> +     tristate "ACPI Ambient Light Sensor"
>>
>> I suspect that the driver depends on CONFIG_ACPI
>>
>>
>>> +     help
>>> +      Support for ACPI0008 Light Sensor.
>>> +      Provides direct access via sysfs.
>>> +
>>>  endmenu
>>> diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
>>> index 141af1e..13090e6 100644
>>> --- a/drivers/staging/iio/light/Makefile
>>> +++ b/drivers/staging/iio/light/Makefile
>>> @@ -7,3 +7,4 @@ obj-$(CONFIG_SENSORS_ISL29018)        += isl29018.o
>>>  obj-$(CONFIG_SENSORS_ISL29028)       += isl29028.o
>>>  obj-$(CONFIG_TSL2583)        += tsl2583.o
>>>  obj-$(CONFIG_TSL2x7x)        += tsl2x7x_core.o
>>> +obj-$(CONFIG_ACPI_ALS)       += acpi-als.o
>>> diff --git a/drivers/staging/iio/light/acpi-als.c b/drivers/staging/iio/light/acpi-als.c
>>> new file mode 100644
>>> index 0000000..9ba0fc4
>>> --- /dev/null
>>> +++ b/drivers/staging/iio/light/acpi-als.c
>>> @@ -0,0 +1,486 @@
>>> +/*
>>> + * 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_ILLUMINANCE  0x80
>>> +
>>> +#define ACPI_ALS_OUTPUTS             1
>>> +
>>> +#define _COMPONENT                   ACPI_ALS_COMPONENT
>>> +ACPI_MODULE_NAME("acpi-als");
>>> +
>>> +MODULE_AUTHOR("Martin Liska <marxin.liska-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
>>> +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;
>>
>> Neither the struct is defined nor the field is used.
>>
>>> +    struct mutex             lock;
>>> +    struct iio_trigger               *trig;
>>> +
>>> +    int                              illuminance;
>>> +    int                              polling;
>>
>> Polling is only ever assigned, but never read.
>>
>>> +
>>> +    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_LOW                0
>>> +#define ALS_INVALID_VALUE_HIGH               -1
>>> +
>>
>> [...]
>>> +/*
>>> + * 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;
>>> +}
>>> +
>>> +/*
>>> + * get_illuminance - wrapper for getting the currect ambient light illuminance
>>> + */
>>
>> Why is this wrapper necessary?
>>
>>> +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 acpi_als_chip *chip = iio_priv(indio_dev);
>>> +
>>> +     s64 time_ns = iio_get_time_ns();
>>> +     int len = sizeof(int);
>>> +     u8 data[sizeof(s64) + ACPI_ALS_OUTPUTS * len];
>>> +
>>> +     switch(event) {
>>> +     case ACPI_ALS_NOTIFY_ILLUMINANCE:
>>> +             get_illuminance(chip, &illuminance);
>>> +             *(int *)((u8 *)data) = illuminance;
>>> +             *(s64 *)((u8 *)data + ALIGN(len, sizeof(s64))) = time_ns;
>>
>> You don't have a timestamp channel in your channel spec.
>>
>>> +             break;
>>> +     default:
>>> +             return;
>>> +     }
>>> +
>>> +     if (iio_buffer_enabled(indio_dev))
>>> +             iio_push_to_buffers(indio_dev, data);
>>> +
>>> +     return;
>>> +}
>>> +
>>> +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:
>>
>> You did not register a RAW attribute for this device.
>>
>>> +    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);
>>
>> You did register a scale attribute for the device, so whenever somebody
>> reads the scale attribute he will get this message. I don't think that makes
>> much sense. Better just remove IIO_CHAN_INFO_SCALE_SEPARATE_BIT from the
>> channel spec. And also remove this dev_err.
>>
>>> +        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,
>>> +        .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);
>>> +
>>> +    printk("XXX: TRIGGER handler called :)");
>>
>> Aha?
>>
>>> +    iio_trigger_notify_done(chip->trig);
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>> [...]
>>> +
>>> +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;
>>> +    }
>>> +    */
>>
>> What's with this?
>>
>>> +
>>> +    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");
>>
>> dev_err
>>
>>> +        goto exit_iio_free;
>>> +    }
>>> +
>>> +    result = acpi_als_trigger_init(indio_dev);
>>> +    if (result) {
>>> +        printk("Couldn't setup the triggers.\n");
>>
>> dev_err
>>
>>> +        // 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");
>>
>> This is just noise, please remove it.
>>
>>> +    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) {
>>
>> Can this ever happen? I suspect not.
>>
>>> +        dev_err(&device->dev, "could not get indio_dev for ACPI device\n");
>>> +        return -1;
>>> +    }
>>> +
>>> +    iio_device_unregister(indio_dev);
>>> +    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);
>>
>> Hm, there is no module_acpi_driver? We should probably add one.
>>
> 
> Hello,
>    you are right. The patch is really just half finished patch (let's
> call it draft). I will merge all comments you added to my patch. But
> the major stuff is that I don't know how to handle iio trigger. As I
> wrote before, I use acpi_notify callback for filling iio buffer.
> Guideline for IIO says each iio buffer should be completed with
> appropriate trigger. In my case makes not sense because I can fill the
> buffer for ACPI event handler?
Doing things via triggers is not a hard and fast rule. Often it makes
sense because multiple devices may use the same trigger.  Here for example
you might want to sample some other sensor as close as possible to when the
acpi als event occurs (no idea why, this makes a lot more sense for some
other types of sensor!)

If you want to push to the buffer directly because the trigger event includes
the data in question then that is also fine.

Jonathan
> 
> Thank you for advice,
> Martin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>