All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] iio: driver for Semtech SX9500 proximity solution
@ 2014-11-26 12:50 Vlad Dogaru
  2014-12-07 12:15 ` Hartmut Knaack
  0 siblings, 1 reply; 5+ messages in thread
From: Vlad Dogaru @ 2014-11-26 12:50 UTC (permalink / raw)
  To: linux-iio; +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 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  |  14 +
 drivers/iio/proximity/Makefile |   1 +
 drivers/iio/proximity/sx9500.c | 755 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 770 insertions(+)
 create mode 100644 drivers/iio/proximity/sx9500.c

diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 0c8cdf5..623fb03 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -17,3 +17,17 @@ 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.
+
+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..07f2a2e
--- /dev/null
+++ b/drivers/iio/proximity/sx9500.c
@@ -0,0 +1,755 @@
+/*
+ * 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		0x70
+
+/*
+ * 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.
+	 */
+	u8 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 = 1,				\
+		.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;
+	unsigned int bits;
+} sx9500_samp_freq_table[] = {
+	{2, 500000, 0x70},
+	{3, 333333, 0x60},
+	{5, 0, 0x50},
+	{6, 666666, 0x40},
+	{8, 333333, 0x30},
+	{11, 111111, 0x20},
+	{16, 666666, 0x10},
+	{33, 333333, 0x00},
+};
+
+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 i, ret;
+	unsigned int regval;
+
+	mutex_lock(&data->mutex);
+
+	ret = regmap_read(data->regmap, SX9500_REG_PROX_CTRL0, &regval);
+	if (ret < 0)
+		goto out;
+
+	ret = -EINVAL;
+	for (i = 0; i < ARRAY_SIZE(sx9500_samp_freq_table); i++)
+		if (sx9500_samp_freq_table[i].bits ==
+		    (regval & SX9500_SCAN_PERIOD_MASK)) {
+			*val = sx9500_samp_freq_table[i].val;
+			*val2 = sx9500_samp_freq_table[i].val2;
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		}
+
+out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+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, idx;
+
+	idx = -1;
+	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) {
+			idx = i;
+			break;
+		}
+
+	if (idx == -1)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+
+	ret = regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
+				 SX9500_SCAN_PERIOD_MASK,
+				 sx9500_samp_freq_table[idx].bits);
+
+	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, chan;
+	unsigned int val;
+
+	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;
+		u8 new_prox = !(val & (1 << 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_RISING :
+			IIO_EV_DIR_FALLING;
+		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)
+		return -EINVAL;
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+	if (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;
+	int i;
+	bool any_active;
+	unsigned int irqmask;
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+	if (dir != IIO_EV_DIR_EITHER)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+
+	data->event_enabled[chan->channel] = state;
+
+	any_active = 0;
+	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
+		if (data->event_enabled[chan->channel]) {
+			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);
+	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 = kmalloc(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++] = (s16)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: 400 */
+		.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, num_defaults;
+	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;
+
+	num_defaults = ARRAY_SIZE(sx9500_default_regs);
+	for (i = 0; i < num_defaults; 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 = id->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;
+
+	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:
+	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);
+	iio_triggered_buffer_cleanup(indio_dev);
+	iio_trigger_unregister(data->trig);
+
+	return 0;
+}
+
+static const struct acpi_device_id sx9500_acpi_match[] = {
+	{"SSX9500", 0}, /* TODO is there a better ACPI handle? */
+	{ },
+};
+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] 5+ messages in thread
* Re: [PATCH v2] iio: driver for Semtech SX9500 proximity solution
  2014-11-26 12:50 [PATCH v2] iio: driver for Semtech SX9500 proximity solution Vlad Dogaru
@ 2014-12-07 12:15 ` Hartmut Knaack
  2014-12-08 11:48   ` Vlad Dogaru
  0 siblings, 1 reply; 5+ messages in thread
From: Hartmut Knaack @ 2014-12-07 12:15 UTC (permalink / raw)
  To: Vlad Dogaru, linux-iio

Vlad Dogaru schrieb am 26.11.2014 um 13:50:
> Supports buffering, IIO events and changing sampling frequency.
> 
> Datasheet available at:
> http://www.semtech.com/images/datasheet/sx9500_ag.pdf
> 
Please find some issues and suggestions inline.
> Signed-off-by: Vlad Dogaru <vlad.dogaru@intel.com>
> ---
> 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  |  14 +
>  drivers/iio/proximity/Makefile |   1 +
>  drivers/iio/proximity/sx9500.c | 755 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 770 insertions(+)
>  create mode 100644 drivers/iio/proximity/sx9500.c
> 
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index 0c8cdf5..623fb03 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -17,3 +17,17 @@ 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.
It can also be built as a module, called...
> +
> +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..07f2a2e
> --- /dev/null
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -0,0 +1,755 @@
> +/*
> + * 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		0x70
Could be a GENMASK(6, 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.
> +	 */
> +	u8 prox_stat[SX9500_NUM_CHANNELS];
prox_stat should be bool.
> +	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 = 1,				\
= ARRAY_SIZE(sx9500_events)?
> +		.scan_index = idx,				\
> +		.scan_type = {					\
> +			.sign = 'u',				\
Datasheet says: Signed, 2's complement format.
> +			.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;
> +	unsigned int bits;
> +} sx9500_samp_freq_table[] = {
> +	{2, 500000, 0x70},
> +	{3, 333333, 0x60},
> +	{5, 0, 0x50},
> +	{6, 666666, 0x40},
> +	{8, 333333, 0x30},
> +	{11, 111111, 0x20},
> +	{16, 666666, 0x10},
> +	{33, 333333, 0x00},
> +};
In reverse order, bits could be dropped from the struct and be used as
the index instead (with the 4 bit shift applied in access functions).
> +
> +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);
Unless I'm missing something, it should be:
*val = (s16)be16_to_cpu(regval);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int sx9500_read_samp_freq(struct sx9500_data *data,
> +				 int *val, int *val2)
> +{
> +	int i, ret;
> +	unsigned int regval;
> +
> +	mutex_lock(&data->mutex);
> +
> +	ret = regmap_read(data->regmap, SX9500_REG_PROX_CTRL0, &regval);
mutex_unlock could be moved up here.
> +	if (ret < 0)
> +		goto out;
> +
With my suggested format of sx9500_samp_freq_table, it could be as simple as that:
	regval = (regval & SX9500_SCAN_PERIOD_MASK) >> 4;
	*val = sx9500_samp_freq_table[regval].val;
	*val2 = sx9500_samp_freq_table[regval].val2;
	ret = IIO_VAL_INT_PLUS_MICRO;
> +	ret = -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(sx9500_samp_freq_table); i++)
> +		if (sx9500_samp_freq_table[i].bits ==
> +		    (regval & SX9500_SCAN_PERIOD_MASK)) {
> +			*val = sx9500_samp_freq_table[i].val;
> +			*val2 = sx9500_samp_freq_table[i].val2;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			break;
> +		}
> +
> +out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +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, idx;
> +
Similar to the _write_frequency approach in ad799x.c, this could be done without idx.
> +	idx = -1;
> +	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) {
> +			idx = i;
Just break in case of a match.
> +			break;
> +		}
> +
> +	if (idx == -1)
If we had no match, i will be equal to 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,
> +				 sx9500_samp_freq_table[idx].bits);
Using my suggested version, this would change to:
	ret = regmap_update_bits(data->regmap, SX9500_REG_PROX_CTRL0,
				 SX9500_SCAN_PERIOD_MASK, i << 4);
> +
> +	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
Double whitespace.
> +	 * 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, chan;
chan could be unsigned.
> +	unsigned int val;
> +
> +	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;
> +		u8 new_prox = !(val & (1 << chan));
new_prox should be bool. (1 << chan) could be expressed as BIT(chan).
Why the negation? If PROXSTATx gets set, doesn't this indicate, that an object
gets close to the sensor? Shouldn't such an event cause IIO_EV_DIR_RISING?
> +
> +		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_RISING :
> +			IIO_EV_DIR_FALLING;
> +		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)
> +		return -EINVAL;
> +	if (type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +	if (dir != IIO_EV_DIR_EITHER)
> +		return -EINVAL;
These checks could be consolidated to just one if and one return.
> +
> +	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;
> +	int i;
i could be grouped up with ret or irqmask.
> +	bool any_active;
any_active could be initialized with false.
> +	unsigned int irqmask;
> +
> +	if (chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +	if (type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +	if (dir != IIO_EV_DIR_EITHER)
> +		return -EINVAL;
These checks could also be consolidated.
> +
> +	mutex_lock(&data->mutex);
> +
> +	data->event_enabled[chan->channel] = state;
> +
> +	any_active = 0;
true or false should be used for bools, but better initialize during declaration.
> +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> +		if (data->event_enabled[chan->channel]) {
Don't you mean 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);
> +	data->trigger_enabled = state;
Only set data->trigger_enabled if regmap_update_bits was successful.
> +
> +	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 = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
Use kzalloc for a clean buffer?
> +	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++] = (s16)val;
Which need is there to treat val as s16?
> +	}
> +
> +	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: 400 */
> +		.def = 0x0e,
.def should be 0x11.
> +	},
> +	{
> +		.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, num_defaults;
> +	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;
> +
> +	num_defaults = ARRAY_SIZE(sx9500_default_regs);
num_defaults doesn't serve an essential purpose.
> +	for (i = 0; i < num_defaults; 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 = id->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;
> +
> +	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:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +out_trigger_unregister:
Trigger was only registered, if client->irq > 0.
> +	iio_trigger_unregister(data->trig);
> +
> +	return ret;
> +}
i2c_set_clientdata() is missing in _probe.
> +
> +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);
> +	iio_triggered_buffer_cleanup(indio_dev);
Trigger was only registered, if client->irq > 0.
> +	iio_trigger_unregister(data->trig);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id sx9500_acpi_match[] = {
> +	{"SSX9500", 0}, /* TODO is there a better ACPI handle? */
Is this a typo? Shouldn't it be "SX9500"?
> +	{ },
> +};
> +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] 5+ messages in thread
* Re: [PATCH v2] iio: driver for Semtech SX9500 proximity solution
  2014-12-07 12:15 ` Hartmut Knaack
@ 2014-12-08 11:48   ` Vlad Dogaru
  2014-12-09  0:26     ` Hartmut Knaack
  0 siblings, 1 reply; 5+ messages in thread
From: Vlad Dogaru @ 2014-12-08 11:48 UTC (permalink / raw)
  To: Hartmut Knaack; +Cc: linux-iio

On Sun, Dec 07, 2014 at 01:15:25PM +0100, Hartmut Knaack wrote:
> Vlad Dogaru schrieb am 26.11.2014 um 13:50:
> > Supports buffering, IIO events and changing sampling frequency.
> > 
> > Datasheet available at:
> > http://www.semtech.com/images/datasheet/sx9500_ag.pdf
> > 
> Please find some issues and suggestions inline.

Thanks for the review.  I'll answer some points inline and hold off the
next version until I hear back from you.

As for the points I didn't address, I will fix them in the next
iteration.

[snip]

> > +#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 = 1,				\
> = ARRAY_SIZE(sx9500_events)?
> > +		.scan_index = idx,				\
> > +		.scan_type = {					\
> > +			.sign = 'u',				\
> Datasheet says: Signed, 2's complement format.

See my comment below.

[snip]

> > +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);
> Unless I'm missing something, it should be:
> *val = (s16)be16_to_cpu(regval);

I've chosen the formula so that when a finger is touching the sensor,
the reported value is 0; as the distance grows, so does the value.
Your version would report a large value when a finger is touching the
sensor, with the value decreasing as distance grows.

The ABI documentation of in_proximity_raw mentions that, where possible,
the value after scaling should be meters.  In this case it is not
possible to convert the readings to meters, but I think it would be
beneficial to have a value that at least behaves like a distance.

[snip]

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

I've consistently used double spaces after full stops in the comments
(and in my emails).  AFAICT, this is an open discussion [1].  The kernel
uses both standards, so as long as comments across a file are consistent
I don't see a problem with using double spaces to separate sentences.

[1] http://en.wikipedia.org/wiki/Sentence_spacing#Digital_age

> > +	 * 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, chan;
> chan could be unsigned.
> > +	unsigned int val;
> > +
> > +	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;
> > +		u8 new_prox = !(val & (1 << chan));
> new_prox should be bool. (1 << chan) could be expressed as BIT(chan).
> Why the negation? If PROXSTATx gets set, doesn't this indicate, that an object
> gets close to the sensor? Shouldn't such an event cause IIO_EV_DIR_RISING?

Maybe I'm misreading the ABI, but if PROXSTATx is set, it means
proximity is detected.  If it wasn't detected previously, this means
that an object has moved closer to the sensor, thus the distance to the
object has decreased and we should cause an IIO_EV_DIR_FALLING.

[snip]

> > +	any_active = 0;
> true or false should be used for bools, but better initialize during declaration.
> > +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
> > +		if (data->event_enabled[chan->channel]) {
> Don't you mean data->event_enabled[i]?

I definitely do, that was silly of me :)

> > +			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);
> > +	data->trigger_enabled = state;
> Only set data->trigger_enabled if regmap_update_bits was successful.
> > +
> > +	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 = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> Use kzalloc for a clean buffer?

Is this just cosmetic or am I missing something?  Assuming that
scan_bytes has the correct value, won't all of 'buf' be written with
channel data and the timestamp?

[snip]

> > +static int sx9500_init_device(struct iio_dev *indio_dev)
> > +{
> > +	struct sx9500_data *data = iio_priv(indio_dev);
> > +	int ret, i, num_defaults;
> > +	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;
> > +
> > +	num_defaults = ARRAY_SIZE(sx9500_default_regs);
> num_defaults doesn't serve an essential purpose.

Thought I'd avoid it being calculated at every step of the for loop.
But it's not on a hot path, so I'll get rid of it.

[snip]

> > +static const struct acpi_device_id sx9500_acpi_match[] = {
> > +	{"SSX9500", 0}, /* TODO is there a better ACPI handle? */
> Is this a typo? Shouldn't it be "SX9500"?

ACPI rules prohibit such a device ID:

ssdt.dsl     11:                 Name (_HID, "SX9500")
Error    6033 -                                    ^ _HID string must be
exactly 7 or 8 characters (SX9500)

I was unable to find a common method to fix such an issue and was hoping
someone can help, that's why I left the TODO in the code.

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: driver for Semtech SX9500 proximity solution
  2014-12-08 11:48   ` Vlad Dogaru
@ 2014-12-09  0:26     ` Hartmut Knaack
  2014-12-12 11:33       ` Jonathan Cameron
  0 siblings, 1 reply; 5+ messages in thread
From: Hartmut Knaack @ 2014-12-09  0:26 UTC (permalink / raw)
  To: Vlad Dogaru; +Cc: linux-iio, Jonathan Cameron

Vlad Dogaru schrieb am 08.12.2014 um 12:48:
> On Sun, Dec 07, 2014 at 01:15:25PM +0100, Hartmut Knaack wrote:
>> Vlad Dogaru schrieb am 26.11.2014 um 13:50:
>>> Supports buffering, IIO events and changing sampling frequency.
>>>
>>> Datasheet available at:
>>> http://www.semtech.com/images/datasheet/sx9500_ag.pdf
>>>
>> Please find some issues and suggestions inline.
> 
> Thanks for the review.  I'll answer some points inline and hold off the
> next version until I hear back from you.
> 
> As for the points I didn't address, I will fix them in the next
> iteration.
> 
> [snip]
> 
>>> +#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 = 1,				\
>> = ARRAY_SIZE(sx9500_events)?
>>> +		.scan_index = idx,				\
>>> +		.scan_type = {					\
>>> +			.sign = 'u',				\
>> Datasheet says: Signed, 2's complement format.
> 
> See my comment below.
> 
> [snip]
> 
>>> +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);
>> Unless I'm missing something, it should be:
>> *val = (s16)be16_to_cpu(regval);
> 
> I've chosen the formula so that when a finger is touching the sensor,
> the reported value is 0; as the distance grows, so does the value.
> Your version would report a large value when a finger is touching the
> sensor, with the value decreasing as distance grows.
> 
> The ABI documentation of in_proximity_raw mentions that, where possible,
> the value after scaling should be meters.  In this case it is not
> possible to convert the readings to meters, but I think it would be
> beneficial to have a value that at least behaves like a distance.
> 
Good points. Given that the channel has got the IIO_CHAN_INFO_RAW flag, I would
expect to get the raw value here. ABI also isn't clear yet, so I'll leave it to
Jonathan or the others to decide.
> [snip]
> 
>>> +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
>> Double whitespace.
> 
> I've consistently used double spaces after full stops in the comments
> (and in my emails).  AFAICT, this is an open discussion [1].  The kernel
> uses both standards, so as long as comments across a file are consistent
> I don't see a problem with using double spaces to separate sentences.
> 
> [1] http://en.wikipedia.org/wiki/Sentence_spacing#Digital_age
Thanks for sharing, I haven't heard of that before (autocorrect function in
Office program even allowed to indicate these issues). But since even the
CodingStyle document uses both ways, I can just say: never mind then :-)
> 
>>> +	 * 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, chan;
>> chan could be unsigned.
>>> +	unsigned int val;
>>> +
>>> +	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;
>>> +		u8 new_prox = !(val & (1 << chan));
>> new_prox should be bool. (1 << chan) could be expressed as BIT(chan).
>> Why the negation? If PROXSTATx gets set, doesn't this indicate, that an object
>> gets close to the sensor? Shouldn't such an event cause IIO_EV_DIR_RISING?
> 
> Maybe I'm misreading the ABI, but if PROXSTATx is set, it means
> proximity is detected.  If it wasn't detected previously, this means
> that an object has moved closer to the sensor, thus the distance to the
> object has decreased and we should cause an IIO_EV_DIR_FALLING.
Sounds good to me. Thanks.
> 
> [snip]
> 
>>> +	any_active = 0;
>> true or false should be used for bools, but better initialize during declaration.
>>> +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
>>> +		if (data->event_enabled[chan->channel]) {
>> Don't you mean data->event_enabled[i]?
> 
> I definitely do, that was silly of me :)
> 
>>> +			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);
>>> +	data->trigger_enabled = state;
>> Only set data->trigger_enabled if regmap_update_bits was successful.
>>> +
>>> +	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 = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> Use kzalloc for a clean buffer?
> 
> Is this just cosmetic or am I missing something?  Assuming that
> scan_bytes has the correct value, won't all of 'buf' be written with
> channel data and the timestamp?
> 
I think it's a safety measure. Think about future code changes which might
introduce some bug, that wouldn't completely fill the buffer. If it was
cleared during allocation, there can't be too much harm. But pushing out
unwanted data could be exploited. Comparing the use of kmalloc and kzalloc
nowadays, you get a relation of about 2500 vs. 4500 in favor of kzalloc.
> [snip]
> 
>>> +static int sx9500_init_device(struct iio_dev *indio_dev)
>>> +{
>>> +	struct sx9500_data *data = iio_priv(indio_dev);
>>> +	int ret, i, num_defaults;
>>> +	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;
>>> +
>>> +	num_defaults = ARRAY_SIZE(sx9500_default_regs);
>> num_defaults doesn't serve an essential purpose.
> 
> Thought I'd avoid it being calculated at every step of the for loop.
> But it's not on a hot path, so I'll get rid of it.
> 
The compiler replaces ARRAY_SIZE in an early step with its calculated value,
so this won't be happening during runtime.
> [snip]
> 
>>> +static const struct acpi_device_id sx9500_acpi_match[] = {
>>> +	{"SSX9500", 0}, /* TODO is there a better ACPI handle? */
>> Is this a typo? Shouldn't it be "SX9500"?
> 
> ACPI rules prohibit such a device ID:
> 
> ssdt.dsl     11:                 Name (_HID, "SX9500")
> Error    6033 -                                    ^ _HID string must be
> exactly 7 or 8 characters (SX9500)
> 
> I was unable to find a common method to fix such an issue and was hoping
> someone can help, that's why I left the TODO in the code.
Good point, thanks. ACPI is not exactly my favorite.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: driver for Semtech SX9500 proximity solution
  2014-12-09  0:26     ` Hartmut Knaack
@ 2014-12-12 11:33       ` Jonathan Cameron
  0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2014-12-12 11:33 UTC (permalink / raw)
  To: Hartmut Knaack, Vlad Dogaru; +Cc: linux-iio

On 09/12/14 00:26, Hartmut Knaack wrote:
> Vlad Dogaru schrieb am 08.12.2014 um 12:48:
>> On Sun, Dec 07, 2014 at 01:15:25PM +0100, Hartmut Knaack wrote:
>>> Vlad Dogaru schrieb am 26.11.2014 um 13:50:
>>>> Supports buffering, IIO events and changing sampling frequency.
>>>>
>>>> Datasheet available at:
>>>> http://www.semtech.com/images/datasheet/sx9500_ag.pdf
>>>>
>>> Please find some issues and suggestions inline.
>>
>> Thanks for the review.  I'll answer some points inline and hold off the
>> next version until I hear back from you.
>>
>> As for the points I didn't address, I will fix them in the next
>> iteration.
>>
>> [snip]
>>
>>>> +#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 = 1,				\
>>> = ARRAY_SIZE(sx9500_events)?
>>>> +		.scan_index = idx,				\
>>>> +		.scan_type = {					\
>>>> +			.sign = 'u',				\
>>> Datasheet says: Signed, 2's complement format.
>>
>> See my comment below.
>>
>> [snip]
>>
>>>> +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);
>>> Unless I'm missing something, it should be:
>>> *val = (s16)be16_to_cpu(regval);
>>
>> I've chosen the formula so that when a finger is touching the sensor,
>> the reported value is 0; as the distance grows, so does the value.
>> Your version would report a large value when a finger is touching the
>> sensor, with the value decreasing as distance grows.
>>
>> The ABI documentation of in_proximity_raw mentions that, where possible,
>> the value after scaling should be meters.  In this case it is not
>> possible to convert the readings to meters, but I think it would be
>> beneficial to have a value that at least behaves like a distance.
>>
> Good points. Given that the channel has got the IIO_CHAN_INFO_RAW flag, I would
> expect to get the raw value here. ABI also isn't clear yet, so I'll leave it to
> Jonathan or the others to decide.
Certainly an interesting case.  As no 'scaling' can be provided we are
in an odd corner.  I think what you have done is sensible and will encourage
the same on other 'unscalable' devices.  Having said that I think a short note
that it should increase with distance (all other factors being equal) in the
proximity abi Docs would make this apparent to both userspace app developers
(assuming they ever read the docs;) and writers of future drivers.
>> [snip]
>>
>>>> +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
>>> Double whitespace.
>>
>> I've consistently used double spaces after full stops in the comments
>> (and in my emails).  AFAICT, this is an open discussion [1].  The kernel
>> uses both standards, so as long as comments across a file are consistent
>> I don't see a problem with using double spaces to separate sentences.
>>
>> [1] http://en.wikipedia.org/wiki/Sentence_spacing#Digital_age
> Thanks for sharing, I haven't heard of that before (autocorrect function in
> Office program even allowed to indicate these issues). But since even the
> CodingStyle document uses both ways, I can just say: never mind then :-)
>>
>>>> +	 * 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, chan;
>>> chan could be unsigned.
>>>> +	unsigned int val;
>>>> +
>>>> +	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;
>>>> +		u8 new_prox = !(val & (1 << chan));
>>> new_prox should be bool. (1 << chan) could be expressed as BIT(chan).
>>> Why the negation? If PROXSTATx gets set, doesn't this indicate, that an object
>>> gets close to the sensor? Shouldn't such an event cause IIO_EV_DIR_RISING?
>>
>> Maybe I'm misreading the ABI, but if PROXSTATx is set, it means
>> proximity is detected.  If it wasn't detected previously, this means
>> that an object has moved closer to the sensor, thus the distance to the
>> object has decreased and we should cause an IIO_EV_DIR_FALLING.
> Sounds good to me. Thanks.
Drat - that abi is indeed somewhat confusing.  Should have gone for
distance in the first place rather than proximity.  Ah well, we get
to live with our mistakes in ABIs for a long time.
>>
>> [snip]
>>
>>>> +	any_active = 0;
>>> true or false should be used for bools, but better initialize during declaration.
>>>> +	for (i = 0; i < SX9500_NUM_CHANNELS; i++)
>>>> +		if (data->event_enabled[chan->channel]) {
>>> Don't you mean data->event_enabled[i]?
>>
>> I definitely do, that was silly of me :)
>>
>>>> +			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);
>>>> +	data->trigger_enabled = state;
>>> Only set data->trigger_enabled if regmap_update_bits was successful.
>>>> +
>>>> +	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 = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>>> Use kzalloc for a clean buffer?
>>
>> Is this just cosmetic or am I missing something?  Assuming that
>> scan_bytes has the correct value, won't all of 'buf' be written with
>> channel data and the timestamp?
>>
> I think it's a safety measure. Think about future code changes which might
> introduce some bug, that wouldn't completely fill the buffer. If it was
> cleared during allocation, there can't be too much harm. But pushing out
> unwanted data could be exploited. Comparing the use of kmalloc and kzalloc
> nowadays, you get a relation of about 2500 vs. 4500 in favor of kzalloc.
>> [snip]
>>
>>>> +static int sx9500_init_device(struct iio_dev *indio_dev)
>>>> +{
>>>> +	struct sx9500_data *data = iio_priv(indio_dev);
>>>> +	int ret, i, num_defaults;
>>>> +	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;
>>>> +
>>>> +	num_defaults = ARRAY_SIZE(sx9500_default_regs);
>>> num_defaults doesn't serve an essential purpose.
>>
>> Thought I'd avoid it being calculated at every step of the for loop.
>> But it's not on a hot path, so I'll get rid of it.
>>
> The compiler replaces ARRAY_SIZE in an early step with its calculated value,
> so this won't be happening during runtime.
>> [snip]
>>
>>>> +static const struct acpi_device_id sx9500_acpi_match[] = {
>>>> +	{"SSX9500", 0}, /* TODO is there a better ACPI handle? */
>>> Is this a typo? Shouldn't it be "SX9500"?
>>
>> ACPI rules prohibit such a device ID:
>>
>> ssdt.dsl     11:                 Name (_HID, "SX9500")
>> Error    6033 -                                    ^ _HID string must be
>> exactly 7 or 8 characters (SX9500)
>>
>> I was unable to find a common method to fix such an issue and was hoping
>> someone can help, that's why I left the TODO in the code.
> Good point, thanks. ACPI is not exactly my favorite.
ouch. That's dumb...  Ah well.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-12-12 11:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-26 12:50 [PATCH v2] iio: driver for Semtech SX9500 proximity solution Vlad Dogaru
2014-12-07 12:15 ` Hartmut Knaack
2014-12-08 11:48   ` Vlad Dogaru
2014-12-09  0:26     ` Hartmut Knaack
2014-12-12 11:33       ` 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.