All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] iio: driver for Semtech SX9500 proximity solution
@ 2014-12-22 12:41 Vlad Dogaru
  2014-12-26 10:31 ` Jonathan Cameron
  0 siblings, 1 reply; 4+ messages in thread
From: Vlad Dogaru @ 2014-12-22 12:41 UTC (permalink / raw)
  To: linux-iio, knaack.h, jic23; +Cc: Vlad Dogaru

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@intel.com>
---
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, &regval, 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, &regval);
+	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.
+	 */
+	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);
+
+	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);
+	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);
+
+	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);
+
+	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@intel.com>");
+MODULE_DESCRIPTION("Driver for Semtech SX9500 proximity sensor");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] iio: driver for Semtech SX9500 proximity solution
  2014-12-22 12:41 [PATCH v4] iio: driver for Semtech SX9500 proximity solution Vlad Dogaru
@ 2014-12-26 10:31 ` Jonathan Cameron
  2014-12-29 11:33   ` Vlad Dogaru
  0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2014-12-26 10:31 UTC (permalink / raw)
  To: Vlad Dogaru, linux-iio, knaack.h

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@intel.com>
Please pick up any reviewed-by or acked-by tags from
previous versions here.
Hartmut gave you a reviewed-by.

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, &regval, 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, &regval);
> +	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.
> +	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);

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

> +
> +	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@intel.com>");
> +MODULE_DESCRIPTION("Driver for Semtech SX9500 proximity sensor");
> +MODULE_LICENSE("GPL v2");
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] iio: driver for Semtech SX9500 proximity solution
  2014-12-26 10:31 ` Jonathan Cameron
@ 2014-12-29 11:33   ` Vlad Dogaru
  2015-01-01 10:26     ` Jonathan Cameron
  0 siblings, 1 reply; 4+ messages in thread
From: Vlad Dogaru @ 2014-12-29 11:33 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio, knaack.h

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@intel.com>
> 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, &regval, 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, &regval);
> > +	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.

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.

> > +	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@intel.com>");
> > +MODULE_DESCRIPTION("Driver for Semtech SX9500 proximity sensor");
> > +MODULE_LICENSE("GPL v2");
> > 
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] iio: driver for Semtech SX9500 proximity solution
  2014-12-29 11:33   ` Vlad Dogaru
@ 2015-01-01 10:26     ` Jonathan Cameron
  0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2015-01-01 10:26 UTC (permalink / raw)
  To: Vlad Dogaru; +Cc: linux-iio, knaack.h

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@intel.com>
>> 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, &regval, 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, &regval);
>>> +	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@intel.com>");
>>> +MODULE_DESCRIPTION("Driver for Semtech SX9500 proximity sensor");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-01-01 10:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-22 12:41 [PATCH v4] iio: driver for Semtech SX9500 proximity solution Vlad Dogaru
2014-12-26 10:31 ` Jonathan Cameron
2014-12-29 11:33   ` Vlad Dogaru
2015-01-01 10:26     ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.