On 29/12/14 11:33, Vlad Dogaru wrote: > On Fri, Dec 26, 2014 at 10:31:37AM +0000, Jonathan Cameron wrote: >> On 22/12/14 12:41, Vlad Dogaru wrote: >>> Supports buffering, IIO events and changing sampling frequency. >>> >>> Datasheet available at: >>> http://www.semtech.com/images/datasheet/sx9500_ag.pdf >>> >>> Signed-off-by: Vlad Dogaru <vlad.dogaru@xxxxxxxxx> >> Please pick up any reviewed-by or acked-by tags from >> previous versions here. >> Hartmut gave you a reviewed-by. > > Will do that in the next revision, sorry for missing it. > >> Anyhow, one real issue with the unregistering of the trigger and >> buffer stuff (buffer doesn't depend on irq being present, whereas trigger >> does). You have it the other way around for unregister (it's correct for >> the register). >> >> Otherwise a few comments inline. >> >> Looking pretty good. >> >> Jonathan >>> --- >>> Changes since v3: >>> - remove unnecessary typecast and double negation; >>> >>> Changes since v2: >>> - use GENMASK, BIT, ARRAY_SIZE macros where appropriate; >>> - use bool instead of int for prox_stat; >>> - rework sample frequency table code; >>> - consolidate event config sanity checks to a single condition; >>> - fix bug when deciding to disable proximity IRQ; >>> - prefer kzalloc to kmalloc; >>> - only cleanup trigger if it was registered (irq > 0); >>> - add mention for building as module in Kconfig help text; >>> >>> Changes since v1: >>> - report raw readings of the channels instead of just 0 or 1. >>> - add a new Kconfig section for proximity and leave the lightning sensor in its >>> own one. >>> >>> drivers/iio/proximity/Kconfig | 17 + >>> drivers/iio/proximity/Makefile | 1 + >>> drivers/iio/proximity/sx9500.c | 739 +++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 757 insertions(+) >>> create mode 100644 drivers/iio/proximity/sx9500.c >>> >>> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig >>> index 0c8cdf5..41a8d8f 100644 >>> --- a/drivers/iio/proximity/Kconfig >>> +++ b/drivers/iio/proximity/Kconfig >>> @@ -17,3 +17,20 @@ config AS3935 >>> module will be called as3935 >>> >>> endmenu >>> + >>> +menu "Proximity sensors" >>> + >>> +config SX9500 >>> + tristate "SX9500 Semtech proximity sensor" >>> + select IIO_BUFFER >>> + select IIO_TRIGGERED_BUFFER >>> + select REGMAP_I2C >>> + depends on I2C >>> + help >>> + Say Y here to build a driver for Semtech's SX9500 capacitive >>> + proximity/button sensor. >>> + >>> + To compile this driver as a module, choose M here: the >>> + module will be called sx9500. >>> + >>> +endmenu >>> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile >>> index 743adee..9818dc5 100644 >>> --- a/drivers/iio/proximity/Makefile >>> +++ b/drivers/iio/proximity/Makefile >>> @@ -4,3 +4,4 @@ >>> >>> # When adding new entries keep the list in alphabetical order >>> obj-$(CONFIG_AS3935) += as3935.o >>> +obj-$(CONFIG_SX9500) += sx9500.o >>> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c >>> new file mode 100644 >>> index 0000000..24ab48c >>> --- /dev/null >>> +++ b/drivers/iio/proximity/sx9500.c >>> @@ -0,0 +1,739 @@ >>> +/* >>> + * Copyright (c) 2014 Intel Corporation >>> + * >>> + * Driver for Semtech's SX9500 capacitive proximity/button solution. >>> + * Datasheet available at >>> + * <http://www.semtech.com/images/datasheet/sx9500.pdf>. >>> + * >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms of the GNU General Public License version 2 as published by >>> + * the Free Software Foundation. >>> + */ >>> + >>> +#include <linux/kernel.h> >>> +#include <linux/slab.h> >>> +#include <linux/module.h> >>> +#include <linux/i2c.h> >>> +#include <linux/irq.h> >>> +#include <linux/acpi.h> >>> +#include <linux/gpio/consumer.h> >>> +#include <linux/regmap.h> >>> + >>> +#include <linux/iio/iio.h> >>> +#include <linux/iio/buffer.h> >>> +#include <linux/iio/sysfs.h> >>> +#include <linux/iio/events.h> >>> +#include <linux/iio/trigger.h> >>> +#include <linux/iio/triggered_buffer.h> >>> +#include <linux/iio/trigger_consumer.h> >>> + >>> +#define SX9500_DRIVER_NAME "sx9500" >>> +#define SX9500_IRQ_NAME "sx9500_event" >>> +#define SX9500_GPIO_NAME "sx9500_gpio" >>> + >>> +/* Register definitions. */ >>> +#define SX9500_REG_IRQ_SRC 0x00 >>> +#define SX9500_REG_STAT 0x01 >>> +#define SX9500_REG_IRQ_MSK 0x03 >>> + >>> +#define SX9500_REG_PROX_CTRL0 0x06 >>> +#define SX9500_REG_PROX_CTRL1 0x07 >>> +#define SX9500_REG_PROX_CTRL2 0x08 >>> +#define SX9500_REG_PROX_CTRL3 0x09 >>> +#define SX9500_REG_PROX_CTRL4 0x0a >>> +#define SX9500_REG_PROX_CTRL5 0x0b >>> +#define SX9500_REG_PROX_CTRL6 0x0c >>> +#define SX9500_REG_PROX_CTRL7 0x0d >>> +#define SX9500_REG_PROX_CTRL8 0x0e >>> + >>> +#define SX9500_REG_SENSOR_SEL 0x20 >>> +#define SX9500_REG_USE_MSB 0x21 >>> +#define SX9500_REG_USE_LSB 0x22 >>> +#define SX9500_REG_AVG_MSB 0x23 >>> +#define SX9500_REG_AVG_LSB 0x24 >>> +#define SX9500_REG_DIFF_MSB 0x25 >>> +#define SX9500_REG_DIFF_LSB 0x26 >>> +#define SX9500_REG_OFFSET_MSB 0x27 >>> +#define SX9500_REG_OFFSET_LSB 0x28 >>> + >>> +#define SX9500_REG_RESET 0x7f >>> + >>> +/* Write this to REG_RESET to do a soft reset. */ >>> +#define SX9500_SOFT_RESET 0xde >>> + >>> +#define SX9500_SCAN_PERIOD_MASK GENMASK(6, 4) >>> +#define SX9500_SCAN_PERIOD_SHIFT 4 >>> + >>> +/* >>> + * These serve for identifying IRQ source in the IRQ_SRC register, and >>> + * also for masking the IRQs in the IRQ_MSK register. >>> + */ >>> +#define SX9500_CLOSE_IRQ BIT(6) >>> +#define SX9500_FAR_IRQ BIT(5) >>> +#define SX9500_CONVDONE_IRQ BIT(3) >>> + >>> +#define SX9500_PROXSTAT_SHIFT 4 >>> + >>> +#define SX9500_NUM_CHANNELS 4 >>> + >>> +struct sx9500_data { >>> + struct mutex mutex; >>> + struct i2c_client *client; >>> + struct iio_trigger *trig; >>> + struct regmap *regmap; >>> + /* >>> + * Last reading of the proximity status for each channel. We >>> + * only send an event to user space when this changes. >>> + */ >>> + bool prox_stat[SX9500_NUM_CHANNELS]; >>> + bool event_enabled[SX9500_NUM_CHANNELS]; >>> + bool trigger_enabled; >>> +}; >>> + >>> +static const struct iio_event_spec sx9500_events[] = { >>> + { >>> + .type = IIO_EV_TYPE_THRESH, >>> + .dir = IIO_EV_DIR_EITHER, >>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE), >>> + }, >>> +}; >>> + >>> +#define SX9500_CHANNEL(idx) \ >>> + { \ >>> + .type = IIO_PROXIMITY, \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >>> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ >>> + .indexed = 1, \ >>> + .channel = idx, \ >>> + .event_spec = sx9500_events, \ >>> + .num_event_specs = ARRAY_SIZE(sx9500_events), \ >>> + .scan_index = idx, \ >>> + .scan_type = { \ >>> + .sign = 'u', \ >>> + .realbits = 16, \ >>> + .storagebits = 16, \ >>> + .shift = 0, \ >>> + }, \ >>> + } >>> + >>> +static const struct iio_chan_spec sx9500_channels[] = { >>> + SX9500_CHANNEL(0), >>> + SX9500_CHANNEL(1), >>> + SX9500_CHANNEL(2), >>> + SX9500_CHANNEL(3), >>> + IIO_CHAN_SOFT_TIMESTAMP(4), >>> +}; >>> + >>> +static const struct { >>> + int val; >>> + int val2; >>> +} sx9500_samp_freq_table[] = { >>> + {33, 333333}, >>> + {16, 666666}, >>> + {11, 111111}, >>> + {8, 333333}, >>> + {6, 666666}, >>> + {5, 0}, >>> + {3, 333333}, >>> + {2, 500000}, >>> +}; >>> + >>> +static const struct regmap_range sx9500_writable_reg_ranges[] = { >>> + regmap_reg_range(SX9500_REG_IRQ_MSK, SX9500_REG_IRQ_MSK), >>> + regmap_reg_range(SX9500_REG_PROX_CTRL0, SX9500_REG_PROX_CTRL8), >>> + regmap_reg_range(SX9500_REG_SENSOR_SEL, SX9500_REG_SENSOR_SEL), >>> + regmap_reg_range(SX9500_REG_OFFSET_MSB, SX9500_REG_OFFSET_LSB), >>> + regmap_reg_range(SX9500_REG_RESET, SX9500_REG_RESET), >>> +}; >>> + >>> +static const struct regmap_access_table sx9500_writeable_regs = { >>> + .yes_ranges = sx9500_writable_reg_ranges, >>> + .n_yes_ranges = ARRAY_SIZE(sx9500_writable_reg_ranges), >>> +}; >>> + >>> +/* >>> + * All allocated registers are readable, so we just list unallocated >>> + * ones. >>> + */ >>> +static const struct regmap_range sx9500_non_readable_reg_ranges[] = { >>> + regmap_reg_range(SX9500_REG_STAT + 1, SX9500_REG_STAT + 1), >>> + regmap_reg_range(SX9500_REG_IRQ_MSK + 1, SX9500_REG_PROX_CTRL0 - 1), >>> + regmap_reg_range(SX9500_REG_PROX_CTRL8 + 1, SX9500_REG_SENSOR_SEL - 1), >>> + regmap_reg_range(SX9500_REG_OFFSET_LSB + 1, SX9500_REG_RESET - 1), >>> +}; >>> + >>> +static const struct regmap_access_table sx9500_readable_regs = { >>> + .no_ranges = sx9500_non_readable_reg_ranges, >>> + .n_no_ranges = ARRAY_SIZE(sx9500_non_readable_reg_ranges), >>> +}; >>> + >>> +static const struct regmap_range sx9500_volatile_reg_ranges[] = { >>> + regmap_reg_range(SX9500_REG_IRQ_SRC, SX9500_REG_STAT), >>> + regmap_reg_range(SX9500_REG_USE_MSB, SX9500_REG_OFFSET_LSB), >>> + regmap_reg_range(SX9500_REG_RESET, SX9500_REG_RESET), >>> +}; >>> + >>> +static const struct regmap_access_table sx9500_volatile_regs = { >>> + .yes_ranges = sx9500_volatile_reg_ranges, >>> + .n_yes_ranges = ARRAY_SIZE(sx9500_volatile_reg_ranges), >>> +}; >>> + >>> +static const struct regmap_config sx9500_regmap_config = { >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + >>> + .max_register = SX9500_REG_RESET, >>> + .cache_type = REGCACHE_RBTREE, >>> + >>> + .wr_table = &sx9500_writeable_regs, >>> + .rd_table = &sx9500_readable_regs, >>> + .volatile_table = &sx9500_volatile_regs, >>> +}; >>> + >>> +static int sx9500_read_proximity(struct sx9500_data *data, >>> + const struct iio_chan_spec *chan, >>> + int *val) >>> +{ >>> + int ret; >>> + __be16 regval; >>> + >>> + ret = regmap_write(data->regmap, SX9500_REG_SENSOR_SEL, chan->channel); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_bulk_read(data->regmap, SX9500_REG_USE_MSB, ®val, 2); >>> + if (ret < 0) >>> + return ret; >>> + >>> + *val = 32767 - (s16)be16_to_cpu(regval); >>> + >>> + return IIO_VAL_INT; >>> +} >>> + >>> +static int sx9500_read_samp_freq(struct sx9500_data *data, >>> + int *val, int *val2) >>> +{ >>> + int ret; >>> + unsigned int regval; >>> + >>> + mutex_lock(&data->mutex); >>> + ret = regmap_read(data->regmap, SX9500_REG_PROX_CTRL0, ®val); >>> + mutex_unlock(&data->mutex); >>> + >>> + if (ret < 0) >>> + return ret; >>> + >>> + regval = (regval & SX9500_SCAN_PERIOD_MASK) >> SX9500_SCAN_PERIOD_SHIFT; >>> + *val = sx9500_samp_freq_table[regval].val; >>> + *val2 = sx9500_samp_freq_table[regval].val2; >>> + >>> + return IIO_VAL_INT_PLUS_MICRO; >>> +} >>> + >>> +static int sx9500_read_raw(struct iio_dev *indio_dev, >>> + const struct iio_chan_spec *chan, >>> + int *val, int *val2, long mask) >>> +{ >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + switch (chan->type) { >>> + case IIO_PROXIMITY: >>> + switch (mask) { >>> + case IIO_CHAN_INFO_RAW: >>> + if (iio_buffer_enabled(indio_dev)) >>> + return -EBUSY; >>> + mutex_lock(&data->mutex); >>> + ret = sx9500_read_proximity(data, chan, val); >>> + mutex_unlock(&data->mutex); >>> + return ret; >>> + case IIO_CHAN_INFO_SAMP_FREQ: >>> + return sx9500_read_samp_freq(data, val, val2); >>> + default: >>> + return -EINVAL; >>> + } >>> + default: >>> + return -EINVAL; >>> + } >>> +} >>> + >>> +static int sx9500_set_samp_freq(struct sx9500_data *data, >>> + int val, int val2) >>> +{ >>> + int i, ret; >>> + >>> + for (i = 0; i < ARRAY_SIZE(sx9500_samp_freq_table); i++) >>> + if (val == sx9500_samp_freq_table[i].val && >>> + val2 == sx9500_samp_freq_table[i].val2) >>> + break; >>> + >>> + if (i == ARRAY_SIZE(sx9500_samp_freq_table)) >>> + return -EINVAL; >>> + >>> + mutex_lock(&data->mutex); >>> + >>> + ret = regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0, >>> + SX9500_SCAN_PERIOD_MASK, >>> + i << SX9500_SCAN_PERIOD_SHIFT); >>> + >>> + mutex_unlock(&data->mutex); >>> + >>> + return ret; >>> +} >>> + >>> +static int sx9500_write_raw(struct iio_dev *indio_dev, >>> + const struct iio_chan_spec *chan, >>> + int val, int val2, long mask) >>> +{ >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + >>> + switch (chan->type) { >>> + case IIO_PROXIMITY: >>> + switch (mask) { >>> + case IIO_CHAN_INFO_SAMP_FREQ: >>> + return sx9500_set_samp_freq(data, val, val2); >>> + default: >>> + return -EINVAL; >>> + } >>> + default: >>> + return -EINVAL; >>> + } >>> +} >>> + >>> +static irqreturn_t sx9500_irq_handler(int irq, void *private) >>> +{ >>> + struct iio_dev *indio_dev = private; >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + >>> + if (data->trigger_enabled) >>> + iio_trigger_poll(data->trig); >>> + >>> + /* >>> + * Even if no event is enabled, we need to wake the thread to >>> + * clear the interrupt state by reading SX9500_REG_IRQ_SRC. It >>> + * is not possible to do that here because regmap_read takes a >>> + * mutex. >>> + */ >> Hmm. Normally do that in the try_reenable callback of the trigger. >> (pretty much what it is there for). The point of doing that, is that if >> we have a slow device using the trigger we should prevent the trigger >> refiring until all are ready. As it stands here, if we get a big delay >> on the pollfunc (filling the buffer) the interrupt sitting above that >> (and below this) might be masked for long enough that the 'event handling' >> side of things here will have cleared it. Mind you, if we are running that >> close to the edge of being able to handle the data rates then all bets >> are pretty much off anyway wrt not dropping samples. >> >> Probably best to leave it as it is, given there is no obvious solution. > > Understood, I'll leave this as it is for now. > >>> + return IRQ_WAKE_THREAD; >>> +} >>> + >>> +static irqreturn_t sx9500_irq_thread_handler(int irq, void *private) >>> +{ >>> + struct iio_dev *indio_dev = private; >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + int ret; >>> + unsigned int val, chan; >>> + >>> + mutex_lock(&data->mutex); >>> + >>> + ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val); >>> + if (ret < 0) { >>> + dev_err(&data->client->dev, "i2c transfer error in irq\n"); >>> + goto out; >>> + } >>> + >>> + if (!(val & (SX9500_CLOSE_IRQ | SX9500_FAR_IRQ))) >>> + goto out; >>> + >>> + ret = regmap_read(data->regmap, SX9500_REG_STAT, &val); >>> + if (ret < 0) { >>> + dev_err(&data->client->dev, "i2c transfer error in irq\n"); >>> + goto out; >>> + } >>> + >>> + val >>= SX9500_PROXSTAT_SHIFT; >>> + for (chan = 0; chan < SX9500_NUM_CHANNELS; chan++) { >>> + int dir; >>> + u64 ev; >>> + bool new_prox = val & BIT(chan); >>> + >>> + if (!data->event_enabled[chan]) >>> + continue; >>> + if (new_prox == data->prox_stat[chan]) >>> + /* No change on this channel. */ >>> + continue; >>> + >>> + dir = new_prox ? IIO_EV_DIR_FALLING : >>> + IIO_EV_DIR_RISING; >>> + ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, >>> + chan, >>> + IIO_EV_TYPE_THRESH, >>> + dir); >>> + iio_push_event(indio_dev, ev, iio_get_time_ns()); >>> + data->prox_stat[chan] = new_prox; >>> + } >>> + >>> +out: >>> + mutex_unlock(&data->mutex); >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static int sx9500_read_event_config(struct iio_dev *indio_dev, >>> + const struct iio_chan_spec *chan, >>> + enum iio_event_type type, >>> + enum iio_event_direction dir) >>> +{ >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + >>> + if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH || >>> + dir != IIO_EV_DIR_EITHER) >>> + return -EINVAL; >>> + >>> + return data->event_enabled[chan->channel]; >>> +} >>> + >>> +static int sx9500_write_event_config(struct iio_dev *indio_dev, >>> + const struct iio_chan_spec *chan, >>> + enum iio_event_type type, >>> + enum iio_event_direction dir, >>> + int state) >>> +{ >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + int ret, i; >>> + bool any_active = false; >>> + unsigned int irqmask; >>> + >>> + if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH || >>> + dir != IIO_EV_DIR_EITHER) >>> + return -EINVAL; >>> + >>> + mutex_lock(&data->mutex); >>> + >>> + data->event_enabled[chan->channel] = state; >>> + >>> + for (i = 0; i < SX9500_NUM_CHANNELS; i++) >>> + if (data->event_enabled[i]) { >>> + any_active = true; >>> + break; >>> + } >>> + >>> + irqmask = SX9500_CLOSE_IRQ | SX9500_FAR_IRQ; >>> + ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK, >>> + irqmask, any_active ? irqmask : 0); >> This logic would be more readable (to my mind) as. >> if (any_active) >> irqmask = SX9500... >> else >> irqmask = 0; >> >> ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK, >> irqmask, irqmask); > > That won't work as it is above, I think. The third parameter of the > call ("mask") is constant with respect to any_active. Only the last > parameter ("val") depends on any_active. good point. oops. > > I'll change it to: > > irqmask = SX9500_CLOSE_IRQ | SX9500_FAR_IRQ; > > if (any_active) > ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK, > irqmask, irqmask); > else > ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK, > irqmask, 0); > >> Putting a trinary operator in an argument list is just uggly ;) >>> + >>> + mutex_unlock(&data->mutex); >>> + >>> + return ret; >>> +} >>> + >>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL( >>> + "2.500000 3.333333 5 6.666666 8.333333 11.111111 16.666666 33.333333"); >>> + >>> +static struct attribute *sx9500_attributes[] = { >>> + &iio_const_attr_sampling_frequency_available.dev_attr.attr, >>> + NULL, >>> +}; >>> + >>> +static const struct attribute_group sx9500_attribute_group = { >>> + .attrs = sx9500_attributes, >>> +}; >>> + >>> +static const struct iio_info sx9500_info = { >>> + .driver_module = THIS_MODULE, >>> + .attrs = &sx9500_attribute_group, >>> + .read_raw = &sx9500_read_raw, >>> + .write_raw = &sx9500_write_raw, >>> + .read_event_config = &sx9500_read_event_config, >>> + .write_event_config = &sx9500_write_event_config, >>> +}; >>> + >>> +static int sx9500_set_trigger_state(struct iio_trigger *trig, >>> + bool state) >>> +{ >>> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + int ret; >>> + >>> + mutex_lock(&data->mutex); >>> + >>> + ret = regmap_update_bits(data->regmap, SX9500_REG_IRQ_MSK, >>> + SX9500_CONVDONE_IRQ, >>> + state ? SX9500_CONVDONE_IRQ : 0); >>> + if (ret == 0) >>> + data->trigger_enabled = state; >>> + >>> + mutex_unlock(&data->mutex); >>> + >>> + return ret; >>> +} >>> + >>> +static const struct iio_trigger_ops sx9500_trigger_ops = { >>> + .set_trigger_state = sx9500_set_trigger_state, >>> + .owner = THIS_MODULE, >>> +}; >>> + >>> +static irqreturn_t sx9500_trigger_handler(int irq, void *private) >>> +{ >>> + struct iio_poll_func *pf = private; >>> + struct iio_dev *indio_dev = pf->indio_dev; >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + s16 *buf; >>> + int val, bit, ret, i = 0; >>> + >>> + buf = kzalloc(indio_dev->scan_bytes, GFP_KERNEL); >> Generally a bad idea to have allocations in here (they don't change >> from scan to scan of the device channels). We have the update_scan_mode >> callback to allow this to be done whenever the scan mode (and hence scan_bytes) >> changes. Mind you, this part isn't terribly fast so it doesn't matter >> that much if you'd rather leave the allocation here for clarity. > > I thought that a small allocation would be harmless (at most 12 bytes, > including timestamp). Still, I had forgotten about update_scan_code, > I'll use that in the next revision. > > I think it's a good idea to change the dummy driver as well, as right > now it's also using kmalloc in a trigger handler. I'll send an RFC for > that. good idea. > >>> + if (!buf) { >>> + dev_err(&data->client->dev, >>> + "failed to allocate memory in trigger handler\n"); >>> + return IRQ_HANDLED; >>> + } >>> + >>> + mutex_lock(&data->mutex); >>> + >>> + for_each_set_bit(bit, indio_dev->buffer->scan_mask, >>> + indio_dev->masklength) { >>> + ret = sx9500_read_proximity(data, &indio_dev->channels[bit], >>> + &val); >>> + if (ret < 0) >>> + goto out; >>> + >>> + buf[i++] = val; >>> + } >>> + >>> + iio_push_to_buffers_with_timestamp(indio_dev, buf, >>> + iio_get_time_ns()); >>> + >>> +out: >>> + mutex_unlock(&data->mutex); >>> + >>> + kfree(buf); >>> + >>> + iio_trigger_notify_done(indio_dev->trig); >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> +struct sx9500_reg_default { >>> + u8 reg; >>> + u8 def; >>> +}; >>> + >>> +static const struct sx9500_reg_default sx9500_default_regs[] = { >>> + { >>> + .reg = SX9500_REG_PROX_CTRL1, >>> + /* Shield enabled, small range. */ >>> + .def = 0x43, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL2, >>> + /* x8 gain, 167kHz frequency, finest resolution. */ >>> + .def = 0x77, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL3, >>> + /* Doze enabled, 2x scan period doze, no raw filter. */ >>> + .def = 0x40, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL4, >>> + /* Average threshold. */ >>> + .def = 0x30, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL5, >>> + /* >>> + * Debouncer off, lowest average negative filter, >>> + * highest average postive filter. >>> + */ >>> + .def = 0x0f, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL6, >>> + /* Proximity detection threshold: 280 */ >>> + .def = 0x0e, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL7, >>> + /* >>> + * No automatic compensation, compensate each pin >>> + * independently, proximity hysteresis: 32, close >>> + * debouncer off, far debouncer off. >>> + */ >>> + .def = 0x00, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL8, >>> + /* No stuck timeout, no periodic compensation. */ >>> + .def = 0x00, >>> + }, >>> + { >>> + .reg = SX9500_REG_PROX_CTRL0, >>> + /* Scan period: 30ms, all sensors enabled. */ >>> + .def = 0x0f, >>> + }, >>> +}; >>> + >>> +static int sx9500_init_device(struct iio_dev *indio_dev) >>> +{ >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + int ret, i; >>> + unsigned int val; >>> + >>> + ret = regmap_write(data->regmap, SX9500_REG_IRQ_MSK, 0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(data->regmap, SX9500_REG_RESET, >>> + SX9500_SOFT_RESET); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_read(data->regmap, SX9500_REG_IRQ_SRC, &val); >>> + if (ret < 0) >>> + return ret; >>> + >>> + for (i = 0; i < ARRAY_SIZE(sx9500_default_regs); i++) { >>> + ret = regmap_write(data->regmap, >>> + sx9500_default_regs[i].reg, >>> + sx9500_default_regs[i].def); >>> + if (ret < 0) >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int sx9500_gpio_probe(struct i2c_client *client, >>> + struct sx9500_data *data) >>> +{ >>> + struct device *dev; >>> + struct gpio_desc *gpio; >>> + int ret; >>> + >>> + if (!client) >>> + return -EINVAL; >>> + >>> + dev = &client->dev; >>> + >>> + /* data ready gpio interrupt pin */ >>> + gpio = devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0); >>> + if (IS_ERR(gpio)) { >>> + dev_err(dev, "acpi gpio get index failed\n"); >>> + return PTR_ERR(gpio); >>> + } >>> + >>> + ret = gpiod_direction_input(gpio); >>> + if (ret) >>> + return ret; >>> + >>> + ret = gpiod_to_irq(gpio); >>> + >>> + dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret); >>> + >>> + return ret; >>> +} >>> + >>> +static int sx9500_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) >>> +{ >>> + int ret; >>> + struct iio_dev *indio_dev; >>> + struct sx9500_data *data; >>> + >>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); >>> + if (indio_dev == NULL) >>> + return -ENOMEM; >>> + >>> + data = iio_priv(indio_dev); >>> + data->client = client; >>> + mutex_init(&data->mutex); >>> + data->trigger_enabled = false; >>> + >>> + data->regmap = devm_regmap_init_i2c(client, &sx9500_regmap_config); >>> + if (IS_ERR(data->regmap)) >>> + return PTR_ERR(data->regmap); >>> + >>> + sx9500_init_device(indio_dev); >>> + >>> + indio_dev->dev.parent = &client->dev; >>> + indio_dev->name = SX9500_DRIVER_NAME; >>> + indio_dev->channels = sx9500_channels; >>> + indio_dev->num_channels = ARRAY_SIZE(sx9500_channels); >>> + indio_dev->info = &sx9500_info; >>> + indio_dev->modes = INDIO_DIRECT_MODE; >>> + i2c_set_clientdata(client, indio_dev); >>> + >>> + if (client->irq <= 0) >>> + client->irq = sx9500_gpio_probe(client, data); >>> + >>> + if (client->irq > 0) { >>> + ret = devm_request_threaded_irq(&client->dev, client->irq, >>> + sx9500_irq_handler, sx9500_irq_thread_handler, >>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >>> + SX9500_IRQ_NAME, indio_dev); >>> + if (ret < 0) >>> + return ret; >>> + >>> + data->trig = devm_iio_trigger_alloc(&client->dev, >>> + "%s-dev%d", indio_dev->name, indio_dev->id); >>> + if (!data->trig) >>> + return -ENOMEM; >>> + >>> + data->trig->dev.parent = &client->dev; >>> + data->trig->ops = &sx9500_trigger_ops; >>> + iio_trigger_set_drvdata(data->trig, indio_dev); >>> + >>> + ret = iio_trigger_register(data->trig); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + ret = iio_triggered_buffer_setup(indio_dev, NULL, >>> + sx9500_trigger_handler, NULL); >>> + if (ret < 0) >>> + goto out_trigger_unregister; >>> + >>> + ret = iio_device_register(indio_dev); >>> + if (ret < 0) >>> + goto out_buffer_cleanup; >>> + >>> + return 0; >>> + >>> +out_buffer_cleanup: >>> + if (client->irq > 0) >>> + iio_triggered_buffer_cleanup(indio_dev); >>> +out_trigger_unregister: >>> + iio_trigger_unregister(data->trig); >> Trigger registration is dependent on the irq being valid, >> not the triggered_buffer (could perhaps use a trigger from elsewhere). >> >>> + >>> + return ret; >>> +} >>> + >>> +static int sx9500_remove(struct i2c_client *client) >>> +{ >>> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >>> + struct sx9500_data *data = iio_priv(indio_dev); >>> + >>> + iio_device_unregister(indio_dev); >>> + if (client->irq > 0) >>> + iio_triggered_buffer_cleanup(indio_dev); >>> + iio_trigger_unregister(data->trig); >> You have the wrong line protected by if (client->irq > 0) >> It's the trigger that needs the irq, not the triggered_buffer. > > Makes sense, good catch! > >> >>> + >>> + return 0; >>> +} >>> + >>> +static const struct acpi_device_id sx9500_acpi_match[] = { >>> + {"SSX9500", 0}, >>> + { }, >>> +}; >>> +MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match); >>> + >>> +static const struct i2c_device_id sx9500_id[] = { >>> + {"sx9500", 0}, >>> + {} >>> +}; >>> +MODULE_DEVICE_TABLE(i2c, sx9500_id); >>> + >>> +static struct i2c_driver sx9500_driver = { >>> + .driver = { >>> + .name = SX9500_DRIVER_NAME, >>> + .acpi_match_table = ACPI_PTR(sx9500_acpi_match), >>> + }, >>> + .probe = sx9500_probe, >>> + .remove = sx9500_remove, >>> + .id_table = sx9500_id, >>> +}; >>> +module_i2c_driver(sx9500_driver); >>> + >>> +MODULE_AUTHOR("Vlad Dogaru <vlad.dogaru@xxxxxxxxx>"); >>> +MODULE_DESCRIPTION("Driver for Semtech SX9500 proximity sensor"); >>> +MODULE_LICENSE("GPL v2"); >>> >> -- 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
