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@xxxxxxxxxx <mailto:jic23@xxxxxxxxxx>> 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@xxxxxxx <mailto:marex@xxxxxxx> <mailto:marex@xxxxxxx <mailto:marex@xxxxxxx>>> 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 > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
