On 12/01/2012 04:46 PM, Martin Liška wrote: > On 29 November 2012 11:15, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: >> Hi, >> >> On 11/29/2012 03:46 AM, marxin.liska@xxxxxxxxx wrote: >>> From: marxin <marxin.liska@xxxxxxxxx> >> >> 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@xxxxxxxxx>"); >>> +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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
