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. -- 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
