On 20/07/15 01:07, Matt Ranostay wrote: > On Sun, Jul 19, 2015 at 3:45 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: >> On 13/07/15 03:20, Matt Ranostay wrote: >>> APDS9960 is a combination of ALS, proximity, and gesture sensors. >>> >>> This patch adds support for these functions along with gain control, >>> integration time, and event thresholds. >>> >>> Signed-off-by: Matt Ranostay <mranostay@xxxxxxxxx> >> Mostly looking good. You don't need to repeat standard docs and there >> are a few bits and pieces inline. >> >> Jonathan >>> --- >>> .../ABI/testing/sysfs-bus-iio-light-apds9960 | 128 +++ >>> drivers/iio/light/Kconfig | 12 + >>> drivers/iio/light/Makefile | 1 + >>> drivers/iio/light/apds9960.c | 1208 ++++++++++++++++++++ >>> 4 files changed, 1349 insertions(+) >>> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-light-apds9960 >>> create mode 100644 drivers/iio/light/apds9960.c >>> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-light-apds9960 b/Documentation/ABI/testing/sysfs-bus-iio-light-apds9960 >>> new file mode 100644 >>> index 0000000..010b8c8 >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-light-apds9960 >>> @@ -0,0 +1,128 @@ >> As a general rule, don't document any attributes covered by more generic >> docs. Just leads to more ways to have out of date documentation! >>> +What: /sys/bus/iio/devices/triggerX/name = "apds9960-gestureX" >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + The APDS9960 kernel module provides a trigger which enables >>> + the gesture engine that pushes motion data to the buffer when >>> + it becomes available. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_proximity0_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the non-gesture proximity sensor value. >> Looks like the generic docs need a wild card indexed version of these added >> (curriously the 0 index channel is there which makes no sense without allowing >> for higher indexes). >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_proximity1_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the UP gesture photodiode proximity value. >>> + Access is disabled to prevent the side effect of >>> + userspace clearing the hardware FIFO. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_proximity2_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the DOWN gesture photodiode proximity value. >>> + Access is disabled to prevent the side effect of >>> + userspace clearing the hardware FIFO. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_proximity3_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the LEFT gesture photodiode proximity value. >>> + Access is disabled to prevent the side effect of >>> + userspace clearing the hardware FIFO. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_proximity4_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the RIGHT gesture photodiode proximity value. >>> + Access is disabled to prevent the side effect of >>> + userspace clearing the hardware FIFO. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_intensity_clear_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the current unprocessed illuminance for the clear/ALS >>> + photodiode. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_intensity_red_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the current unprocessed illuminance value for the >>> + red light wavelength photodiode. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the current unprocessed illuminance value for the >>> + green light wavelength photodiode. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_intensity_blue_raw >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Get the current unprocessed illuminance value for the >>> + blue light wavelength photodiode. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/integration_time_available >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Provide allowed integration time values in fractions of a second >>> + for the ALS + RGB sensor engines. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_intensity_integration_time >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Set integration time in fractions of a second for the ALS + RGB >>> + sensor engines. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/proximity_scale_available >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Provide the allowed gain values for the proximity + gesture >>> + engines. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_proximity_scale >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Set the gain value for the proximity + gesture engines. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/intensity_scale_available >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Provide the allowed gain values for the ALS + RGB sensor >>> + engines. >>> + >>> +What /sys/bus/iio/devices/iio:deviceX/in_intensity_scale >>> +Date: July 2015 >>> +KernelVersion: 4.2 >>> +Contact: Matt Ranostay <mranostay@xxxxxxxxx> >>> +Description: >>> + Set the gain value for the ALS + RGB sensor engines. >> Nothing at all in here is non standard, so as things currently stand >> you don't need the additional ABI document. > Ok will drop in v4 > >>> >> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig >>> index e6198b7..6a4a47c7 100644 >>> --- a/drivers/iio/light/Kconfig >>> +++ b/drivers/iio/light/Kconfig >>> @@ -50,6 +50,18 @@ config APDS9300 >>> To compile this driver as a module, choose M here: the >>> module will be called apds9300. >>> >>> +config APDS9960 >>> + tristate "Avago APDS9960 gesture/RGB/ALS/proximity sensor" >>> + select IIO_BUFFER >> missing regmap_i2c dependency. > > Noted > >>> + select IIO_TRIGGERED_BUFFER >>> + depends on I2C >>> + help >>> + Say Y here to build I2C interface support for the Avago >>> + APDS9960 gesture/RGB/ALS/proximity sensor. >>> + >>> + To compile this driver as a module, choose M here: the >>> + module will be called apds9960 >>> + >>> config BH1750 >>> tristate "ROHM BH1750 ambient light sensor" >>> depends on I2C >>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile >>> index e2d50fd..0189ac7 100644 >>> --- a/drivers/iio/light/Makefile >>> +++ b/drivers/iio/light/Makefile >>> @@ -7,6 +7,7 @@ obj-$(CONFIG_ACPI_ALS) += acpi-als.o >>> obj-$(CONFIG_ADJD_S311) += adjd_s311.o >>> obj-$(CONFIG_AL3320A) += al3320a.o >>> obj-$(CONFIG_APDS9300) += apds9300.o >>> +obj-$(CONFIG_APDS9960) += apds9960.o >>> obj-$(CONFIG_BH1750) += bh1750.o >>> obj-$(CONFIG_CM32181) += cm32181.o >>> obj-$(CONFIG_CM3232) += cm3232.o >>> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c >>> new file mode 100644 >>> index 0000000..39de418 >>> --- /dev/null >>> +++ b/drivers/iio/light/apds9960.c >>> @@ -0,0 +1,1208 @@ >>> +/* >>> + * apds9960.c - Support for Avago APDS9960 gesture/RGB/ALS/proximity sensor >>> + * >>> + * Copyright (C) 2015 Matt Ranostay <mranostay@xxxxxxxxx> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; either version 2 of the License, or >>> + * (at your option) any later version. >>> + * >>> + * This program is distributed in the hope that 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. >>> + * >>> + * TODO: gesture + proximity calib offsets >>> + */ >>> + >>> +#include <linux/module.h> >>> +#include <linux/init.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/delay.h> >>> +#include <linux/mutex.h> >>> +#include <linux/err.h> >>> +#include <linux/irq.h> >>> +#include <linux/gpio.h> >>> +#include <linux/i2c.h> >>> +#include <linux/pm_runtime.h> >>> +#include <linux/regmap.h> >>> +#include <linux/iio/iio.h> >>> +#include <linux/iio/buffer.h> >>> +#include <linux/iio/events.h> >>> +#include <linux/iio/sysfs.h> >>> +#include <linux/iio/trigger.h> >>> +#include <linux/iio/trigger_consumer.h> >>> +#include <linux/iio/triggered_buffer.h> >>> +#include <linux/of_gpio.h> >>> + >>> +#define APDS9960_REGMAP_NAME "apds9960_regmap" >>> +#define APDS9960_DRV_NAME "apds9960" >>> + >>> +#define APDS9960_REG_RAM_START 0x00 >>> +#define APDS9960_REG_RAM_END 0x7f >>> + >>> +#define APDS9960_REG_ENABLE 0x80 >>> +#define APDS9960_REG_ENABLE_PON BIT(0) >>> + >>> +#define APDS9960_REG_ATIME 0x81 >>> +#define APDS9960_REG_WTIME 0x83 >>> + >>> +#define APDS9960_REG_AILTL 0x84 >>> +#define APDS9960_REG_AILTH 0x85 >>> +#define APDS9960_REG_AIHTL 0x86 >>> +#define APDS9960_REG_AIHTH 0x87 >>> + >>> +#define APDS9960_REG_PILT 0x89 >>> +#define APDS9960_REG_PIHT 0x8b >>> +#define APDS9960_REG_PERS 0x8c >>> + >>> +#define APDS9960_REG_CONFIG_1 0x8d >>> +#define APDS9960_REG_PPULSE 0x8e >>> + >>> +#define APDS9960_REG_CONTROL 0x8f >>> +#define APDS9960_REG_CONTROL_AGAIN_MASK 0x03 >>> +#define APDS9960_REG_CONTROL_PGAIN_MASK 0x0c >>> +#define APDS9960_REG_CONTROL_AGAIN_MASK_SHIFT 0 >>> +#define APDS9960_REG_CONTROL_PGAIN_MASK_SHIFT 2 >>> + >>> +#define APDS9960_REG_CONFIG_2 0x90 >>> +#define APDS9960_REG_CONFIG_2_GGAIN_MASK 0x60 >>> +#define APDS9960_REG_CONFIG_2_GGAIN_MASK_SHIFT 5 >>> + >>> +#define APDS9960_REG_ID 0x92 >>> + >>> +#define APDS9960_REG_STATUS 0x93 >>> +#define APDS9960_REG_STATUS_PS_INT BIT(5) >>> +#define APDS9960_REG_STATUS_ALS_INT BIT(4) >>> +#define APDS9960_REG_STATUS_GINT BIT(2) >>> + >>> +#define APDS9960_REG_PDATA 0x9c >>> +#define APDS9960_REG_POFFSET_UR 0x9d >>> +#define APDS9960_REG_POFFSET_DL 0x9e >>> +#define APDS9960_REG_CONFIG_3 0x9f >>> + >>> +#define APDS9960_REG_GPENTH 0xa0 >>> +#define APDS9960_REG_GEXTH 0xa1 >>> + >>> +#define APDS9960_REG_GCONF_1 0xa2 >>> +#define APDS9960_REG_GCONF_1_GFIFO_THRES_MASK 0xc0 >>> +#define APDS9960_REG_GCONF_1_GFIFO_THRES_MASK_SHIFT 6 >>> + >>> +#define APDS9960_REG_GCONF_2 0xa3 >>> +#define APDS9960_REG_GOFFSET_U 0xa4 >>> +#define APDS9960_REG_GOFFSET_D 0xa5 >>> +#define APDS9960_REG_GPULSE 0xa6 >>> +#define APDS9960_REG_GOFFSET_L 0xa7 >>> +#define APDS9960_REG_GOFFSET_R 0xa9 >>> +#define APDS9960_REG_GCONF_3 0xaa >>> + >>> +#define APDS9960_REG_GCONF_4 0xab >>> +#define APDS9960_REG_GFLVL 0xae >>> +#define APDS9960_REG_GSTATUS 0xaf >>> + >>> +#define APDS9960_REG_IFORCE 0xe4 >>> +#define APDS9960_REG_PICLEAR 0xe5 >>> +#define APDS9960_REG_CICLEAR 0xe6 >>> +#define APDS9960_REG_AICLEAR 0xe7 >>> + >>> +#define APDS9960_DEFAULT_PERS 0x33 >>> +#define APDS9960_DEFAULT_GPENTH 0x50 >>> +#define APDS9960_DEFAULT_GEXTH 0x40 >>> + >>> +#define APDS9960_MAX_INT_TIME_IN_US 1000000 >>> + >>> +enum apds9960_als_channel_idx { >>> + IDX_ALS_CLEAR, IDX_ALS_RED, IDX_ALS_GREEN, IDX_ALS_BLUE, >>> +}; >>> + >>> +#define APDS9960_REG_ALS_BASE 0x94 >>> +#define APDS9960_REG_ALS_CHANNEL(_colour) \ >>> + (APDS9960_REG_ALS_BASE + (IDX_ALS_##_colour * 2)) >>> + >>> +enum apds9960_gesture_channel_idx { >>> + IDX_DIR_UP, IDX_DIR_DOWN, IDX_DIR_LEFT, IDX_DIR_RIGHT, >>> +}; >>> + >>> +#define APDS9960_REG_GFIFO_BASE 0xfc >>> +#define APDS9960_REG_GFIFO_DIR(_dir) \ >>> + (APDS9960_REG_GFIFO_BASE + IDX_DIR_##_dir) >>> + >>> +struct apds9960_data { >>> + struct i2c_client *client; >>> + struct iio_trigger *trig; >>> + struct iio_dev *indio_dev; >>> + struct mutex lock; >>> + >>> + /* regmap fields */ >>> + struct regmap *regmap; >>> + struct regmap_field *reg_int_als; >>> + struct regmap_field *reg_int_ges; >>> + struct regmap_field *reg_int_pxs; >>> + >>> + struct regmap_field *reg_enable_als; >>> + struct regmap_field *reg_enable_ges; >>> + struct regmap_field *reg_enable_pxs; >>> + >>> + /* state */ >>> + int als_int; >>> + int pxs_int; >>> + int gesture_mode_running; >>> + >>> + /* gain values */ >>> + int als_gain; >>> + int pxs_gain; >>> + >>> + /* integration time value in us */ >>> + int als_adc_int_us; >>> + >>> + /* gesture buffer */ >>> + u8 buffer[16]; /* 4 8-bit channels + 64-bit timestamp */ >>> +}; >>> + >>> +static const struct reg_default apds9960_reg_defaults[] = { >>> + { APDS9960_REG_ATIME, 0xff }, >> Could you add some explantion of these. > Ok will do in v4 > >>> + { APDS9960_REG_WTIME, 0xff }, >>> + { APDS9960_REG_CONFIG_1, 0x40 }, >> Where it can be defined in terms of other various elements, please break >> them down. Particuarly true, when as with the above, the bit being set >> looks to be marked reserved but so are other bits in that register... > > This is the POR defaults per the datasheet. >> >>> + { APDS9960_REG_CONFIG_2, 0x01 }, >>> + { APDS9960_REG_PPULSE, 0x40 }, >>> + { APDS9960_REG_GPULSE, 0x40 }, >>> +}; >>> + >>> +static const struct regmap_range apds9960_volatile_ranges[] = { >>> + regmap_reg_range(APDS9960_REG_STATUS, >>> + APDS9960_REG_PDATA), >>> + regmap_reg_range(APDS9960_REG_GFLVL, >>> + APDS9960_REG_GSTATUS), >>> + regmap_reg_range(APDS9960_REG_GFIFO_DIR(UP), >>> + APDS9960_REG_GFIFO_DIR(RIGHT)), >>> + regmap_reg_range(APDS9960_REG_IFORCE, >>> + APDS9960_REG_AICLEAR), >>> +}; >>> + >>> +static const struct regmap_access_table apds9960_volatile_table = { >>> + .yes_ranges = apds9960_volatile_ranges, >>> + .n_yes_ranges = ARRAY_SIZE(apds9960_volatile_ranges), >>> +}; >>> + >>> +static const struct regmap_range apds9960_precious_ranges[] = { >>> + regmap_reg_range(APDS9960_REG_RAM_START, APDS9960_REG_RAM_END), >>> +}; >>> + >>> +static const struct regmap_access_table apds9960_precious_table = { >>> + .yes_ranges = apds9960_precious_ranges, >>> + .n_yes_ranges = ARRAY_SIZE(apds9960_precious_ranges), >>> +}; >>> + >>> +static const struct regmap_range apds9960_readable_ranges[] = { >>> + regmap_reg_range(APDS9960_REG_ENABLE, >>> + APDS9960_REG_GSTATUS), >>> + regmap_reg_range(APDS9960_REG_GFIFO_DIR(UP), >>> + APDS9960_REG_GFIFO_DIR(RIGHT)), >>> +}; >>> + >>> +static const struct regmap_access_table apds9960_readable_table = { >>> + .yes_ranges = apds9960_readable_ranges, >>> + .n_yes_ranges = ARRAY_SIZE(apds9960_readable_ranges), >>> +}; >>> + >>> +static const struct regmap_range apds9960_writeable_ranges[] = { >>> + regmap_reg_range(APDS9960_REG_ENABLE, APDS9960_REG_CONFIG_2), >>> + regmap_reg_range(APDS9960_REG_POFFSET_UR, APDS9960_REG_GCONF_4), >>> + regmap_reg_range(APDS9960_REG_IFORCE, APDS9960_REG_AICLEAR), >>> +}; >>> + >>> +static const struct regmap_access_table apds9960_writeable_table = { >>> + .yes_ranges = apds9960_writeable_ranges, >>> + .n_yes_ranges = ARRAY_SIZE(apds9960_writeable_ranges), >>> +}; >>> + >>> +static const struct regmap_config apds9960_regmap_config = { >>> + .name = APDS9960_REGMAP_NAME, >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + .use_single_rw = 1, >>> + >>> + .volatile_table = &apds9960_volatile_table, >>> + .precious_table = &apds9960_precious_table, >>> + .rd_table = &apds9960_readable_table, >>> + .wr_table = &apds9960_writeable_table, >>> + >>> + .reg_defaults = apds9960_reg_defaults, >>> + .num_reg_defaults = ARRAY_SIZE(apds9960_reg_defaults), >>> + .max_register = APDS9960_REG_GFIFO_DIR(RIGHT), >>> + .cache_type = REGCACHE_RBTREE, >>> +}; >>> + >>> +static const struct iio_event_spec apds9960_pxs_event_spec[] = { >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_RISING, >>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >>> + BIT(IIO_EV_INFO_ENABLE), >>> + }, >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >>> + BIT(IIO_EV_INFO_ENABLE), >>> + }, >>> +}; >>> + >>> +static const struct iio_event_spec apds9960_als_event_spec[] = { >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_RISING, >>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >>> + BIT(IIO_EV_INFO_ENABLE), >>> + }, >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_FALLING, >>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >>> + BIT(IIO_EV_INFO_ENABLE), >>> + }, >>> +}; >>> + >>> +#define APDS9960_GESTURE_CHANNEL(_dir, _si) { \ >>> + .type = IIO_PROXIMITY, \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ >>> + .address = APDS9960_REG_GFIFO_DIR(_dir), \ >>> + .channel = _si + 1, \ >>> + .scan_index = _si, \ >>> + .indexed = 1, \ >>> + .scan_type = { \ >>> + .sign = 'u', \ >>> + .realbits = 8, \ >>> + .storagebits = 8, \ >>> + }, \ >>> +} >>> + >>> +#define APDS9960_INTENSITY_CHANNEL(_colour) { \ >>> + .type = IIO_INTENSITY, \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ >>> + BIT(IIO_CHAN_INFO_INT_TIME), \ >>> + .channel2 = IIO_MOD_LIGHT_##_colour, \ >>> + .address = APDS9960_REG_ALS_CHANNEL(_colour), \ >>> + .modified = 1, \ >>> + .scan_index = -1, \ >>> +} >>> + >>> +static const struct iio_chan_spec apds9960_channels[] = { >> Interesting approach. Guess it doesn't make sense to push the normal >> proximity / als through the buffer.. > > Also when in gesture mode the ALS + pxs sensor is disabled till it exits > Ah, I'd missed that. Makes sense I guess. <snip> >>> +static irqreturn_t apds9960_interrupt_handler(int irq, void *private) >>> +{ >>> + struct iio_dev *indio_dev = private; >>> + struct apds9960_data *data = iio_priv(indio_dev); >>> + int ret, status; >>> + >>> + ret = regmap_read(data->regmap, APDS9960_REG_STATUS, &status); >>> + if (ret < 0) { >>> + dev_err(&data->client->dev, "irq status reg read failed\n"); >>> + return IRQ_HANDLED; >>> + } >>> + >>> + if ((status & APDS9960_REG_STATUS_ALS_INT) && data->als_int) { >>> + iio_push_event(indio_dev, >>> + IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0, >>> + IIO_EV_TYPE_THRESH, >>> + IIO_EV_DIR_EITHER), >>> + iio_get_time_ns()); >>> + } >>> + >>> + if ((status & APDS9960_REG_STATUS_PS_INT) && data->pxs_int) { >>> + iio_push_event(indio_dev, >>> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0, >>> + IIO_EV_TYPE_THRESH, >>> + IIO_EV_DIR_EITHER), >>> + iio_get_time_ns()); >>> + } >>> + >>> + if (status & APDS9960_REG_STATUS_GINT) >>> + iio_trigger_poll_chained(data->trig); >> >> Using the chained form restricts (more or less) using this trigger to >> driver any other sensors. Perhaps fine for now, but we may want to >> do the more complex approach to fire on a real interrupt later. > > Issue with a real interrupt is the FIFO reads take too long to > complete (32 * 4 buffer with 32 i2c reads).. have to offload to > threads for now. Not quite what I meant. If you use a real interrupt for the data->trig then you can still quite happily hang a threaded interrupt of that, but it allows you to also use the trigger to drive auxiliary sensors with top half handlers that grab timestamps and similar. > >> >>> + >>> + regmap_write(data->regmap, APDS9960_REG_AICLEAR, 1); >> Might be ideal to clear the als and prox interrupts separately as in theory >> you have a race condition here where you are clearing an interrupt you >> haven't necessarily seen (between the status read and the clear). > > Noted. > >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static inline int apds9660_fifo_is_empty(struct apds9960_data *data) >>> +{ >>> + int cnt; >>> + int ret; >>> + >>> + ret = regmap_read(data->regmap, APDS9960_REG_GFLVL, &cnt); >>> + if (ret) >>> + return ret; >> blank line here. >>> + return cnt; >>> +} >>> + >>> +static irqreturn_t apds9960_trigger_handler(int irq, void *private) >>> +{ >>> + struct iio_poll_func *pf = private; >>> + struct iio_dev *indio_dev = pf->indio_dev; >>> + struct apds9960_data *data = iio_priv(indio_dev); >>> + u8 buffer[4]; >>> + int ret; >>> + >>> + mutex_lock(&data->lock); >>> + data->gesture_mode_running = 1; >>> + >>> + while (apds9660_fifo_is_empty(data) > 0) { >>> + int i, j = 0; >>> + >>> + memset(&data->buffer, 0, sizeof(data->buffer)); >> Why bother zeroing out? You are going to fill it anyway below. (or is there >> a risk of leaking info if the below fails? Don't think so. >>> + >>> + ret = regmap_bulk_read(data->regmap, >>> + APDS9960_REG_GFIFO_BASE, &buffer, 4); >>> + if (ret) >>> + goto err_read; >>> + >>> + /* >>> + * Even if we don't care about scan_index channels we need to >>> + * read them to clear the FIFO. >>> + */ >>> + for_each_set_bit(i, indio_dev->active_scan_mask, >>> + indio_dev->masklength) { >>> + data->buffer[j++] = buffer[i]; >>> + } >> >> Don't need brackets around the above. Also, why not read directly into the >> data->buffer and drop the copy? If you have to read them anyway use >> the scan_masks_available to specify all channels are enabled together then >> let the in core demux logic handle dropping unwanted channels. >> Never any need to do this in driver any more. >> >> Or for that mater just use the lcoal buffer and don't bother with the one >> in data. >> >>> + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer, >>> + iio_get_time_ns()); >>> + } >>> + >>> +err_read: >>> + data->gesture_mode_running = 0; >>> + mutex_unlock(&data->lock); >>> + iio_trigger_notify_done(indio_dev->trig); >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static const struct iio_trigger_ops iio_interrupt_trigger_ops = { >>> + .owner = THIS_MODULE, >>> +}; >>> + >>> +static int apds9960_set_powermode(struct apds9960_data *data, bool state) >>> +{ >>> + int ret; >>> + >>> + ret = regmap_update_bits(data->regmap, APDS9960_REG_ENABLE, >>> + APDS9960_REG_ENABLE_PON, state); >>> + if (ret) >>> + return ret; >> return directly. Given only false value is 0 it'll be the same and 6 lines >> shorter. > Got it. >>> + >>> + return 0; >>> +} >>> + >>> +static int apds9960_buffer_preenable(struct iio_dev *indio_dev) >>> +{ >>> + struct apds9960_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + ret = regmap_field_write(data->reg_int_ges, 1); >>> + if (ret) >>> + return ret; >>> + >>> + ret = regmap_field_write(data->reg_enable_ges, 1); >>> + if (ret) >>> + return ret; >>> + >>> + pm_runtime_get_sync(&data->client->dev); >>> + >>> + return 0; >>> +} >>> + >>> +static int apds9960_buffer_postdisable(struct iio_dev *indio_dev) >>> +{ >>> + struct apds9960_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + ret = regmap_field_write(data->reg_enable_ges, 0); >>> + if (ret) >>> + return ret; >>> + >>> + ret = regmap_field_write(data->reg_int_ges, 0); >>> + if (ret) >>> + return ret; >>> + >>> + pm_runtime_put_autosuspend(&data->client->dev); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct iio_buffer_setup_ops apds9960_buffer_setup_ops = { >>> + .preenable = apds9960_buffer_preenable, >>> + .postenable = iio_triggered_buffer_postenable, >>> + .predisable = iio_triggered_buffer_predisable, >>> + .postdisable = apds9960_buffer_postdisable, >>> +}; >>> + >>> +static int apds9960_regfield_init(struct apds9960_data *data) >>> +{ >>> + struct device *dev = &data->client->dev; >>> + struct regmap *regmap = data->regmap; >>> + >> This feels like a rather long function for what it is doing. >> Worth trying to do these with an enum indexed array? Might not >> be worthwhile. > > I agree this is a bit long, but any better way to alloc all the regmap > bitfields? > Not that simple, but if you made all of these data->regfields[<fieldenumentry>] and provided a matched array of the field definitions, then it could be done as a loop. >>> + data->reg_int_als = >>> + devm_regmap_field_alloc(dev, regmap, reg_field_int_als); >>> + if (IS_ERR(data->reg_int_als)) { >>> + dev_err(dev, "INT ALS reg field init failed\n"); >>> + return PTR_ERR(data->reg_int_als); >>> + } >>> + -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html