[PATCH 0/2] AS3935 lightning sensor support

[PATCH 0/2] AS3935 lightning sensor support

[Matt Ranostay]

This series adds support for the AMS AS3935 lightning sensor that allows
reporting back estimated storm distance and strike events.

Matt Ranostay (2):
  iio:as3935: Add DT binding docs for AS3935 driver
  iio: Add AS3935 lightning sensor support

 .../ABI/testing/sysfs-bus-iio-proximity-as3935     |  18 +
 .../devicetree/bindings/iio/proximity/as3935.txt   |  28 ++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/proximity/Kconfig                      |  19 +
 drivers/iio/proximity/Makefile                     |   6 +
 drivers/iio/proximity/as3935.c                     | 437 +++++++++++++++++++++
 8 files changed, 511 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/as3935.txt
 create mode 100644 drivers/iio/proximity/Kconfig
 create mode 100644 drivers/iio/proximity/Makefile
 create mode 100644 drivers/iio/proximity/as3935.c

-- 
1.8.3.2

[PATCH 1/2] iio:as3935: Add DT binding docs for AS3935 driver

[Matt Ranostay]

Document compatible string, required and optional DT properties for
AS3935 chipset driver.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 .../devicetree/bindings/iio/proximity/as3935.txt   | 28 ++++++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 2 files changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/as3935.txt

diff --git a/Documentation/devicetree/bindings/iio/proximity/as3935.txt b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
new file mode 100644
index 0000000..574d49c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
@@ -0,0 +1,28 @@
+Austrian Microsystems AS3935 Franklin lightning sensor device driver
+
+Required properties:
+	- compatible: must be "ams,as3935"
+	- reg: SPI chip select number for the device
+	- spi-cpha: SPI Mode 1. Refer to spi/spi-bus.txt for generic SPI
+	slave node bindings.
+	- interrupt-parent : should be the phandle for the interrupt controller
+	- interrupts : interrupt mapping for GPIO IRQ
+
+	Refer to interrupt-controller/interrupts.txt for generic
+	interrupt client node bindings.
+
+Optional properties:
+	- ams,tune-cap: Calibration tuning capacitor stepping value 0 - 15.
+	  Range of 0 to 120 pF, 8pF steps. This will require using the
+	  calibration data from the manufacturer.
+
+Example:
+
+as3935@0 {
+	compatible = "ams,as3935";
+	reg = <0>;
+	spi-cpha;
+	interrupt-parent = <&gpio1>;
+	interrupts = <16 1>;
+	ams,tune-cap = <10>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 3c31b40..9dd66ca 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -10,6 +10,7 @@ aeroflexgaisler	Aeroflex Gaisler AB
 ak	Asahi Kasei Corp.
 altr	Altera Corp.
 amcc	Applied Micro Circuits Corporation (APM, formally AMCC)
+ams	AMS AG
 amstaos	AMS-Taos Inc.
 apm	Applied Micro Circuits Corporation (APM)
 arm	ARM Ltd.
-- 
1.8.3.2

Re: [PATCH 1/2] iio:as3935: Add DT binding docs for AS3935 driver

[Jonathan Cameron]

On 06/02/14 07:00, Matt Ranostay wrote:
> Document compatible string, required and optional DT properties for
> AS3935 chipset driver.
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>   .../devicetree/bindings/iio/proximity/as3935.txt   | 28 ++++++++++++++++++++++
>   .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>   2 files changed, 29 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/iio/proximity/as3935.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/as3935.txt b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
> new file mode 100644
> index 0000000..574d49c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
> @@ -0,0 +1,28 @@
> +Austrian Microsystems AS3935 Franklin lightning sensor device driver
> +
> +Required properties:
> +	- compatible: must be "ams,as3935"
> +	- reg: SPI chip select number for the device
> +	- spi-cpha: SPI Mode 1. Refer to spi/spi-bus.txt for generic SPI
> +	slave node bindings.
> +	- interrupt-parent : should be the phandle for the interrupt controller
> +	- interrupts : interrupt mapping for GPIO IRQ
Why does it need to be a GPIO IRQ?  As far as I can see any irq will do
(note there are devices that provide suitable interrupts without the ability
to read the level).  Oops, we have a couple of other IIO bindings stating
exactly this.  I doubt they need to be GPIOs either.

> +
> +	Refer to interrupt-controller/interrupts.txt for generic
> +	interrupt client node bindings.
> +
> +Optional properties:
> +	- ams,tune-cap: Calibration tuning capacitor stepping value 0 - 15.
> +	  Range of 0 to 120 pF, 8pF steps. This will require using the
> +	  calibration data from the manufacturer.
Why not have this as the actual value rather than a magic number?  Might
as well keep the device tree as human readable as possible.
> +
> +Example:
> +
> +as3935@0 {
> +	compatible = "ams,as3935";
> +	reg = <0>;
> +	spi-cpha;
> +	interrupt-parent = <&gpio1>;
> +	interrupts = <16 1>;
> +	ams,tune-cap = <10>;
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 3c31b40..9dd66ca 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -10,6 +10,7 @@ aeroflexgaisler	Aeroflex Gaisler AB
>   ak	Asahi Kasei Corp.
>   altr	Altera Corp.
>   amcc	Applied Micro Circuits Corporation (APM, formally AMCC)
> +ams	AMS AG
>   amstaos	AMS-Taos Inc.
>   apm	Applied Micro Circuits Corporation (APM)
>   arm	ARM Ltd.
>

Re: [PATCH 1/2] iio:as3935: Add DT binding docs for AS3935 driver

[Matt Ranostay]

On Sun, Feb 9, 2014 at 1:54 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 06/02/14 07:00, Matt Ranostay wrote:
>>
>> Document compatible string, required and optional DT properties for
>> AS3935 chipset driver.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>   .../devicetree/bindings/iio/proximity/as3935.txt   | 28
>> ++++++++++++++++++++++
>>   .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>>   2 files changed, 29 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/iio/proximity/as3935.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/proximity/as3935.txt
>> b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
>> new file mode 100644
>> index 0000000..574d49c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
>> @@ -0,0 +1,28 @@
>> +Austrian Microsystems AS3935 Franklin lightning sensor device driver
>> +
>> +Required properties:
>> +       - compatible: must be "ams,as3935"
>> +       - reg: SPI chip select number for the device
>> +       - spi-cpha: SPI Mode 1. Refer to spi/spi-bus.txt for generic SPI
>> +       slave node bindings.
>> +       - interrupt-parent : should be the phandle for the interrupt
>> controller
>> +       - interrupts : interrupt mapping for GPIO IRQ
>
> Why does it need to be a GPIO IRQ?  As far as I can see any irq will do
> (note there are devices that provide suitable interrupts without the ability
> to read the level).  Oops, we have a couple of other IIO bindings stating
> exactly this.  I doubt they need to be GPIOs either.
>
No it doesn't it can be any interrupt type... Will reword better


>
>> +
>> +       Refer to interrupt-controller/interrupts.txt for generic
>> +       interrupt client node bindings.
>> +
>> +Optional properties:
>> +       - ams,tune-cap: Calibration tuning capacitor stepping value 0 -
>> 15.
>> +         Range of 0 to 120 pF, 8pF steps. This will require using the
>> +         calibration data from the manufacturer.
>
> Why not have this as the actual value rather than a magic number?  Might
> as well keep the device tree as human readable as possible.
>

Makes sense.

>> +
>> +Example:
>> +
>> +as3935@0 {
>> +       compatible = "ams,as3935";
>> +       reg = <0>;
>> +       spi-cpha;
>> +       interrupt-parent = <&gpio1>;
>> +       interrupts = <16 1>;
>> +       ams,tune-cap = <10>;
>> +};
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> index 3c31b40..9dd66ca 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> @@ -10,6 +10,7 @@ aeroflexgaisler       Aeroflex Gaisler AB
>>   ak    Asahi Kasei Corp.
>>   altr  Altera Corp.
>>   amcc  Applied Micro Circuits Corporation (APM, formally AMCC)
>> +ams    AMS AG
>>   amstaos       AMS-Taos Inc.
>>   apm   Applied Micro Circuits Corporation (APM)
>>   arm   ARM Ltd.
>>
>

[PATCH 2/2] iio: Add AS3935 lightning sensor support

[Matt Ranostay]

AS3935 chipset can detect lightning strikes and reports those back as
events and the estimated distance to the storm.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 .../ABI/testing/sysfs-bus-iio-proximity-as3935     |  18 +
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/proximity/Kconfig                      |  19 +
 drivers/iio/proximity/Makefile                     |   6 +
 drivers/iio/proximity/as3935.c                     | 437 +++++++++++++++++++++
 6 files changed, 482 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
 create mode 100644 drivers/iio/proximity/Kconfig
 create mode 100644 drivers/iio/proximity/Makefile
 create mode 100644 drivers/iio/proximity/as3935.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935 b/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
new file mode 100644
index 0000000..f6d9e6f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
@@ -0,0 +1,18 @@
+What		/sys/bus/iio/devices/iio:deviceX/in_proximity_raw
+Date:		January 2014
+KernelVersion:	3.15
+Contact:	Matt Ranostay <mranostay@gmail.com>
+Description:
+		Get the current distance in kilometers of storm
+		1    = storm overhead
+		1-40 = distance in kilometers
+		63   = out of range
+
+What		/sys/bus/iio/devices/iio:deviceX/gain_boost
+Date:		January 2014
+KernelVersion:	3.15
+Contact:	Matt Ranostay <mranostay@gmail.com>
+Description:
+		Show or set the gain boost of the amp, from 0-31 range.
+		18 = indoors (default)
+		14 = outdoors
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 5dd0e12..743485e 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -74,6 +74,7 @@ if IIO_TRIGGER
    source "drivers/iio/trigger/Kconfig"
 endif #IIO_TRIGGER
 source "drivers/iio/pressure/Kconfig"
+source "drivers/iio/proximity/Kconfig"
 source "drivers/iio/temperature/Kconfig"
 
 endif # IIO
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 887d390..698afc2 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -24,5 +24,6 @@ obj-y += light/
 obj-y += magnetometer/
 obj-y += orientation/
 obj-y += pressure/
+obj-y += proximity/
 obj-y += temperature/
 obj-y += trigger/
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
new file mode 100644
index 0000000..0c8cdf5
--- /dev/null
+++ b/drivers/iio/proximity/Kconfig
@@ -0,0 +1,19 @@
+#
+# Proximity sensors
+#
+
+menu "Lightning sensors"
+
+config AS3935
+	tristate "AS3935 Franklin lightning sensor"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on SPI
+	help
+	  Say Y here to build SPI interface support for the Austrian
+	  Microsystems AS3935 lightning detection sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called as3935
+
+endmenu
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
new file mode 100644
index 0000000..743adee
--- /dev/null
+++ b/drivers/iio/proximity/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for IIO proximity sensors
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AS3935)		+= as3935.o
diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
new file mode 100644
index 0000000..109759e
--- /dev/null
+++ b/drivers/iio/proximity/as3935.c
@@ -0,0 +1,437 @@
+/*
+ * as3935.c - Support for AS3935 Franklin lightning sensor
+ *
+ * Copyright (C) 2014 Matt Ranostay <mranostay@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/workqueue.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/irq.h>
+#include <linux/gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/pm_runtime.h>
+#include <linux/of_gpio.h>
+
+
+#define AS3935_AFE_GAIN		0x00
+#define AS3935_AFE_MASK		0x3F
+#define AS3935_AFE_GAIN_MAX	0x1F
+#define AS3935_AFE_PWR_BIT	BIT(0)
+
+#define AS3935_INT		0x03
+#define AS3935_INT_MASK		0x07
+#define AS3935_EVENT_INT	BIT(3)
+#define AS3935_NOISE_INT	BIT(1)
+
+#define AS3935_DATA		0x07
+#define AS3935_DATA_MASK	0x3F
+
+#define AS3935_TUNE_CAP		0x08
+#define AS3935_CALIBRATE	0x3D
+
+#define AS3935_WRITE_DATA	BIT(15)
+#define AS3935_READ_DATA	BIT(14)
+#define AS3935_ADDRESS(x)	(x<<8)
+
+struct as3935_state {
+	struct spi_device *spi;
+	struct iio_trigger *trig;
+	struct mutex lock;
+	struct delayed_work work;
+
+	u32 tune_cap;
+};
+
+static const struct iio_chan_spec as3935_channels[] = {
+	{
+		.type           = IIO_PROXIMITY,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW),
+		.scan_index     = 0,
+		.scan_type = {
+			.sign           = 'u',
+			.realbits       = 6,
+			.storagebits    = 8,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static int as3935_read(struct as3935_state *st, unsigned int reg, int *val)
+{
+	u8 tx, rx;
+	int ret;
+
+	struct spi_transfer xfers[] = {
+		{
+			.tx_buf = &tx,
+			.bits_per_word = 8,
+			.len = 1,
+		}, {
+			.rx_buf = &rx,
+			.bits_per_word = 8,
+			.len = 1,
+		},
+	};
+	tx = (AS3935_READ_DATA | AS3935_ADDRESS(reg)) >> 8;
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	*val = rx;
+
+	return ret;
+};
+
+static int as3935_write(struct as3935_state *st,
+				unsigned int reg,
+				unsigned int val)
+{
+	u8 buf[2];
+
+	buf[0] = (AS3935_WRITE_DATA | AS3935_ADDRESS(reg)) >> 8;
+	buf[1] = val;
+
+	return spi_write(st->spi, (u8 *) &buf, 2);
+};
+
+static ssize_t as3935_gain_boost_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct as3935_state *st = iio_priv(dev_to_iio_dev(dev));
+	int val, ret;
+
+	ret = as3935_read(st, AS3935_AFE_GAIN, &val);
+	if (ret)
+		return ret;
+	val = (val & AS3935_AFE_MASK) >> 1;
+
+	return sprintf(buf, "%d\n", val);
+};
+
+static ssize_t as3935_gain_boost_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t len)
+{
+	struct as3935_state *st = iio_priv(dev_to_iio_dev(dev));
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul((const char *) buf, 10, &val);
+	if (ret)
+		return -EINVAL;
+
+	if (val > AS3935_AFE_GAIN_MAX)
+		return -EINVAL;
+
+	as3935_write(st, AS3935_AFE_GAIN, val << 1);
+
+	return len;
+};
+
+static IIO_DEVICE_ATTR(gain_boost, S_IRUGO | S_IWUSR,
+	as3935_gain_boost_show, as3935_gain_boost_store, 0);
+
+
+static struct attribute *as3935_attributes[] = {
+	&iio_dev_attr_gain_boost.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group as3935_attribute_group = {
+	.attrs = as3935_attributes,
+};
+
+static int as3935_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+	struct as3935_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (m != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	*val2 = 0;
+	ret = as3935_read(st, AS3935_DATA, val);
+	if (ret)
+		return ret;
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info as3935_info = {
+	.driver_module = THIS_MODULE,
+	.attrs = &as3935_attribute_group,
+	.read_raw = &as3935_read_raw,
+};
+
+static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
+	.postenable = &iio_triggered_buffer_postenable,
+	.predisable = &iio_triggered_buffer_predisable,
+};
+
+static irqreturn_t as3935_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct as3935_state *st = iio_priv(indio_dev);
+	int val, ret;
+
+	ret = as3935_read(st, AS3935_DATA, &val);
+	if (ret)
+		goto err_read;
+	val &= AS3935_DATA_MASK;
+	iio_push_to_buffers_with_timestamp(indio_dev, &val, iio_get_time_ns());
+err_read:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+};
+
+static const struct iio_trigger_ops iio_interrupt_trigger_ops = {
+	.owner = THIS_MODULE,
+};
+
+static void as3935_event_work(struct work_struct *work)
+{
+	struct as3935_state *st;
+	struct spi_device *spi;
+	int val;
+
+	st = container_of(work, struct as3935_state, work.work);
+	spi = st->spi;
+
+	as3935_read(st, AS3935_INT, &val);
+	val &= AS3935_INT_MASK;
+
+	switch (val) {
+	case AS3935_EVENT_INT:
+		iio_trigger_poll(st->trig, 0);
+		break;
+	case AS3935_NOISE_INT:
+		dev_warn(&spi->dev, "noise level is too high");
+		break;
+	}
+};
+
+static irqreturn_t as3935_interrupt_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct as3935_state *st = iio_priv(indio_dev);
+
+	cancel_delayed_work(&st->work);
+	schedule_delayed_work(&st->work, jiffies_to_msecs(3));
+	return IRQ_HANDLED;
+}
+
+static void calibrate_as3935(struct as3935_state *st)
+{
+	mutex_lock(&st->lock);
+
+	/* mask disturber interrupt bit */
+	as3935_write(st, AS3935_INT, 1 << 5);
+
+	as3935_write(st, AS3935_CALIBRATE, 0x96);
+	as3935_write(st, AS3935_TUNE_CAP, 1 << 5 | st->tune_cap);
+
+	mdelay(2);
+	as3935_write(st, AS3935_TUNE_CAP, st->tune_cap);
+
+	mutex_unlock(&st->lock);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int as3935_suspend(struct spi_device *spi, pm_message_t msg)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct as3935_state *st = iio_priv(indio_dev);
+	int val, ret;
+
+	mutex_lock(&st->lock);
+	ret = as3935_read(st, AS3935_AFE_GAIN, &val);
+	if (ret)
+		return ret;
+	val |= AS3935_AFE_PWR_BIT;
+
+	ret = as3935_write(st, AS3935_AFE_GAIN, val);
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int as3935_resume(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct as3935_state *st = iio_priv(indio_dev);
+	int val, ret;
+
+	mutex_lock(&st->lock);
+	ret = as3935_read(st, AS3935_AFE_GAIN, &val);
+	if (ret)
+		return ret;
+	val &= ~AS3935_AFE_PWR_BIT;
+	ret = as3935_write(st, AS3935_AFE_GAIN, val);
+	mutex_unlock(&st->lock);
+	return ret;
+}
+#else
+#define as3935_suspend	NULL
+#define as3935_resume	NULL
+#endif
+
+static int as3935_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct iio_trigger *trig;
+	struct as3935_state *st;
+	struct device_node *np = spi->dev.of_node;
+	int ret;
+
+	/* Be sure lightning event interrupt */
+	if (!spi->irq) {
+		dev_err(&spi->dev, "unable to get event interrupt\n");
+		return -EINVAL;
+	}
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+	st->tune_cap = 0;
+
+	spi_set_drvdata(spi, indio_dev);
+	mutex_init(&st->lock);
+	INIT_DELAYED_WORK(&st->work, as3935_event_work);
+
+	ret = of_property_read_u32(np, "ams,tune-cap", &st->tune_cap);
+	if (ret) {
+		st->tune_cap = 0;
+		dev_warn(&spi->dev,
+			"no tune-cap set, defaulting to %d", st->tune_cap);
+	}
+
+	if (st->tune_cap > 15) {
+		dev_err(&spi->dev,
+			"wrong tune-cap setting of %d\n", st->tune_cap);
+		return -EINVAL;
+	}
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->channels = as3935_channels;
+	indio_dev->num_channels = ARRAY_SIZE(as3935_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &as3935_info;
+
+	trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
+				      indio_dev->name, indio_dev->id);
+
+	if (!trig)
+		return -ENOMEM;
+
+	st->trig = trig;
+	trig->dev.parent = indio_dev->dev.parent;
+	iio_trigger_set_drvdata(trig, indio_dev);
+	trig->ops = &iio_interrupt_trigger_ops;
+
+	ret = iio_trigger_register(trig);
+	if (ret) {
+		dev_err(&spi->dev, "failed to register trigger\n");
+		return ret;
+	}
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					&as3935_trigger_handler,
+					&iio_triggered_buffer_setup_ops);
+
+	if (ret) {
+		dev_err(&spi->dev, "cannot setup iio trigger\n");
+		goto unregister_trigger;
+	}
+
+	calibrate_as3935(st);
+
+	ret = devm_request_irq(&spi->dev, spi->irq,
+				&as3935_interrupt_handler,
+				IRQF_TRIGGER_RISING,
+				dev_name(&spi->dev),
+				indio_dev);
+
+	if (ret) {
+		dev_err(&spi->dev, "unable to request irq\n");
+		goto unregister_trigger;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&spi->dev, "unable to register device\n");
+		goto unregister_trigger;
+	}
+	return 0;
+
+unregister_trigger:
+	iio_trigger_unregister(st->trig);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return ret;
+};
+
+static int as3935_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct as3935_state *st = iio_priv(indio_dev);
+
+	iio_trigger_unregister(st->trig);
+	iio_triggered_buffer_cleanup(indio_dev);
+	iio_device_unregister(indio_dev);
+
+	return 0;
+};
+
+static const struct spi_device_id as3935_id[] = {
+	{"as3935", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(spi, as3935_id);
+
+static struct spi_driver as3935_driver = {
+	.driver = {
+		.name	= "as3935",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= as3935_probe,
+	.remove		= as3935_remove,
+	.id_table	= as3935_id,
+	.suspend	= as3935_suspend,
+	.resume		= as3935_resume,
+};
+module_spi_driver(as3935_driver);
+
+MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
+MODULE_DESCRIPTION("AS3935 lightning sensor");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:as3935");
-- 
1.8.3.2

Re: [PATCH 2/2] iio: Add AS3935 lightning sensor support

[Jonathan Cameron]

On 06/02/14 07:00, Matt Ranostay wrote:
> AS3935 chipset can detect lightning strikes and reports those back as
> events and the estimated distance to the storm.
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
Hi Matt,

Sorry for my somewhat late entry in the discussion of the interface for this
device, but I'm wondering if we don't have an opportunity to create a more
flexible interface for 'position' measurements.  Afterall sooner or later
we'll get a magetic position sensor or similar coming through.

So how about defining a new iio_chan_type IIO_POSITION and adding some
more modifiers to allow for polar coordinates (spherical coordinates I guess
given we are in 3D). For now we'd only need IIO_MOD_R though later I guess
we would have IIO_MOD_THETA and IIO_MOD_PHI.  That would in this case give
us.

in_position_r_raw with the unit after applying scaling being meters.

Clearly the ideal would have been to bring this suggestion up before we
had the proximity sensors but such is life.  Those tend to simply indicate
something is 'near' rather than at an explicity distance.  The difference
is clearly minor though.

What do people think of this approach?

Comments on the code inline. Note I review from the end (usually makes
more sense) so comments may only make sense in that direction!

Beware of the quirks of spi buses as highlighted inline.  You have to allow
for the buffers you provide being used directly for DMA transfers (unlike
say i2c which copies everything). As such there are some restrictions on
where these buffers must lie.

Jonathan
> ---
>   .../ABI/testing/sysfs-bus-iio-proximity-as3935     |  18 +
>   drivers/iio/Kconfig                                |   1 +
>   drivers/iio/Makefile                               |   1 +
>   drivers/iio/proximity/Kconfig                      |  19 +
>   drivers/iio/proximity/Makefile                     |   6 +
>   drivers/iio/proximity/as3935.c                     | 437 +++++++++++++++++++++
>   6 files changed, 482 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
>   create mode 100644 drivers/iio/proximity/Kconfig
>   create mode 100644 drivers/iio/proximity/Makefile
>   create mode 100644 drivers/iio/proximity/as3935.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935 b/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
> new file mode 100644
> index 0000000..f6d9e6f
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
> @@ -0,0 +1,18 @@
> +What		/sys/bus/iio/devices/iio:deviceX/in_proximity_raw
> +Date:		January 2014
> +KernelVersion:	3.15
> +Contact:	Matt Ranostay <mranostay@gmail.com>
> +Description:
> +		Get the current distance in kilometers of storm
> +		1    = storm overhead
> +		1-40 = distance in kilometers
> +		63   = out of range
This attribute should be a general purpose one given it will apply to lots
of other drivers over time.  Write it as such and distances should definitely
be in meters not kilometers.

> +
> +What		/sys/bus/iio/devices/iio:deviceX/gain_boost
> +Date:		January 2014
> +KernelVersion:	3.15
> +Contact:	Matt Ranostay <mranostay@gmail.com>
> +Description:
> +		Show or set the gain boost of the amp, from 0-31 range.
> +		18 = indoors (default)
> +		14 = outdoors
What does this gain boost actually mean?  I couldn't immediately tell from
the datasheet.  Does it effect the distance estimates, or is it about
sensitivity to detect them in the first place?  Even though you have defined
it just for this one device, we are always looking to generalize interfaces
so it is a good to have an idea of what it actually is.
   
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 5dd0e12..743485e 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -74,6 +74,7 @@ if IIO_TRIGGER
>      source "drivers/iio/trigger/Kconfig"
>   endif #IIO_TRIGGER
>   source "drivers/iio/pressure/Kconfig"
> +source "drivers/iio/proximity/Kconfig"
>   source "drivers/iio/temperature/Kconfig"
>
>   endif # IIO
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 887d390..698afc2 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -24,5 +24,6 @@ obj-y += light/
>   obj-y += magnetometer/
>   obj-y += orientation/
>   obj-y += pressure/
> +obj-y += proximity/
>   obj-y += temperature/
>   obj-y += trigger/
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> new file mode 100644
> index 0000000..0c8cdf5
> --- /dev/null
> +++ b/drivers/iio/proximity/Kconfig
> @@ -0,0 +1,19 @@
> +#
> +# Proximity sensors
> +#
> +
> +menu "Lightning sensors"
> +
> +config AS3935
> +	tristate "AS3935 Franklin lightning sensor"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	depends on SPI
> +	help
> +	  Say Y here to build SPI interface support for the Austrian
> +	  Microsystems AS3935 lightning detection sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called as3935
> +
> +endmenu
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> new file mode 100644
> index 0000000..743adee
> --- /dev/null
> +++ b/drivers/iio/proximity/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for IIO proximity sensors
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_AS3935)		+= as3935.o
> diff --git a/drivers/iio/proximity/as3935.c b/drivers/iio/proximity/as3935.c
> new file mode 100644
> index 0000000..109759e
> --- /dev/null
> +++ b/drivers/iio/proximity/as3935.c
> @@ -0,0 +1,437 @@
> +/*
> + * as3935.c - Support for AS3935 Franklin lightning sensor
> + *
> + * Copyright (C) 2014 Matt Ranostay <mranostay@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/workqueue.h>
> +#include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +#include <linux/gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of_gpio.h>
> +
> +
> +#define AS3935_AFE_GAIN		0x00
> +#define AS3935_AFE_MASK		0x3F
> +#define AS3935_AFE_GAIN_MAX	0x1F
> +#define AS3935_AFE_PWR_BIT	BIT(0)
> +
> +#define AS3935_INT		0x03
> +#define AS3935_INT_MASK		0x07
> +#define AS3935_EVENT_INT	BIT(3)
> +#define AS3935_NOISE_INT	BIT(1)
> +
> +#define AS3935_DATA		0x07
> +#define AS3935_DATA_MASK	0x3F
> +
> +#define AS3935_TUNE_CAP		0x08
> +#define AS3935_CALIBRATE	0x3D
> +
> +#define AS3935_WRITE_DATA	BIT(15)
> +#define AS3935_READ_DATA	BIT(14)
((x) << 8) to protect against strange elements being passed in as x.
Obviously you have this tightly controlled here, but who knows what others
might add in future, so best to do it right!
> +#define AS3935_ADDRESS(x)	(x<<8)
> +
> +struct as3935_state {
> +	struct spi_device *spi;
> +	struct iio_trigger *trig;
> +	struct mutex lock;
> +	struct delayed_work work;
> +
> +	u32 tune_cap;
> +};
> +
> +static const struct iio_chan_spec as3935_channels[] = {
> +	{
> +		.type           = IIO_PROXIMITY,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index     = 0,
> +		.scan_type = {
> +			.sign           = 'u',
> +			.realbits       = 6,
> +			.storagebits    = 8,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
This looks very similar to val = spi_w8r8(st->spi, reg);

> +static int as3935_read(struct as3935_state *st, unsigned int reg, int *val)
> +{
> +	u8 tx, rx;
Spi buffers should never be on the stack. You can't guarantee alignment and
they must be cache aligned.  spi_w8r8 deals with this by using a small
buffer allocated on the heap.  The alternative is to carefully allocate
a cache line aligned buffer in your state structure.  Here I'd definitely
use spi_w8r8 though!
> +	int ret;
> +
> +	struct spi_transfer xfers[] = {
> +		{
> +			.tx_buf = &tx,
> +			.bits_per_word = 8,
> +			.len = 1,
> +		}, {
> +			.rx_buf = &rx,
> +			.bits_per_word = 8,
> +			.len = 1,
> +		},
> +	};
> +	tx = (AS3935_READ_DATA | AS3935_ADDRESS(reg)) >> 8;
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	*val = rx;
> +
> +	return ret;
> +};
> +

> +static int as3935_write(struct as3935_state *st,
> +				unsigned int reg,
> +				unsigned int val)
> +{
> +	u8 buf[2];
You could use a C99 style asignment here (entirely optional!)
e.g.
	u8 buf[2] = { (AS3935_WRITE_DATA | AS3935_ADDRESS(reg)) >> 8,
	   	      val };
However, this buffer must be cacheline aligned seeing as spi_write
does not use a suitably aligned bounce buffer (unlike spi_w8r8 which does).
This can cause nasty, difficult to track down corruptions with spi controllers
that do DMA.
> +
> +	buf[0] = (AS3935_WRITE_DATA | AS3935_ADDRESS(reg)) >> 8;
> +	buf[1] = val;
> +
> +	return spi_write(st->spi, (u8 *) &buf, 2);
> +};
> +
> +static ssize_t as3935_gain_boost_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct as3935_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int val, ret;
> +
> +	ret = as3935_read(st, AS3935_AFE_GAIN, &val);
> +	if (ret)
> +		return ret;
> +	val = (val & AS3935_AFE_MASK) >> 1;
> +
> +	return sprintf(buf, "%d\n", val);
> +};
> +
> +static ssize_t as3935_gain_boost_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct as3935_state *st = iio_priv(dev_to_iio_dev(dev));
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul((const char *) buf, 10, &val);
> +	if (ret)
> +		return -EINVAL;
> +
> +	if (val > AS3935_AFE_GAIN_MAX)
> +		return -EINVAL;
> +
> +	as3935_write(st, AS3935_AFE_GAIN, val << 1);
> +
> +	return len;
> +};
> +
> +static IIO_DEVICE_ATTR(gain_boost, S_IRUGO | S_IWUSR,
> +	as3935_gain_boost_show, as3935_gain_boost_store, 0);
> +
> +
> +static struct attribute *as3935_attributes[] = {
> +	&iio_dev_attr_gain_boost.dev_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group as3935_attribute_group = {
> +	.attrs = as3935_attributes,
> +};
> +
> +static int as3935_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	struct as3935_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (m != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	*val2 = 0;
> +	ret = as3935_read(st, AS3935_DATA, val);
> +	if (ret)
> +		return ret;
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info as3935_info = {
> +	.driver_module = THIS_MODULE,
> +	.attrs = &as3935_attribute_group,
> +	.read_raw = &as3935_read_raw,
> +};
> +
> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
> +	.postenable = &iio_triggered_buffer_postenable,
> +	.predisable = &iio_triggered_buffer_predisable,
> +};
> +
> +static irqreturn_t as3935_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct as3935_state *st = iio_priv(indio_dev);
> +	int val, ret;
> +
> +	ret = as3935_read(st, AS3935_DATA, &val);
> +	if (ret)
> +		goto err_read;
> +	val &= AS3935_DATA_MASK;
This timestamp is probably rather late given I think the interrupt indicated
the actual point in time?
> +	iio_push_to_buffers_with_timestamp(indio_dev, &val, iio_get_time_ns());
> +err_read:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +};
> +
> +static const struct iio_trigger_ops iio_interrupt_trigger_ops = {
> +	.owner = THIS_MODULE,
> +};
> +
> +static void as3935_event_work(struct work_struct *work)
> +{
> +	struct as3935_state *st;
> +	struct spi_device *spi;
> +	int val;
> +
> +	st = container_of(work, struct as3935_state, work.work);
> +	spi = st->spi;
Don't bother with the local variable for spi. It doesn't add any clarity.
> +
> +	as3935_read(st, AS3935_INT, &val);
> +	val &= AS3935_INT_MASK;
> +
> +	switch (val) {
> +	case AS3935_EVENT_INT:
You could save a timestamp in the interrupt handler then pass it on here.
> +		iio_trigger_poll(st->trig, 0);
> +		break;
> +	case AS3935_NOISE_INT:
Hmm. This brings me back to one of my favourite kernel questions,
'Why isn't there a better way of handling hardware errors?'
There isn't as far as I know!
> +		dev_warn(&spi->dev, "noise level is too high");
> +		break;
> +	}
> +};
> +
> +static irqreturn_t as3935_interrupt_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct as3935_state *st = iio_priv(indio_dev);
> +
This is 'unusual' so can you add a comment explaining what is going on here.
My guess is that you want to avoid grabing new data for a short window after
a strike is detected?  The cancel is to avoid running the delayed work twice?

If so the reason a sleep in a threaded handler wouldn't work is that it would
not 'cancel' if new data arrived I guess.  You still have a window for problems
if the interrupt occurs whilst the work queue is actually running the
scheduled work.

I'm wondering if this is overcomplicating things and it is better to just
occasionally handle an extra interrupt and do this with a conventional
threaded interrupt handler.  Of course I may be missing something.
> +	cancel_delayed_work(&st->work);
I'm guessing this should be msecs_to_jiffies seeing as schedule_delayed_work
takes a value in jiffies.
> +	schedule_delayed_work(&st->work, jiffies_to_msecs(3));
blank line here.
> +	return IRQ_HANDLED;
> +}
> +
> +static void calibrate_as3935(struct as3935_state *st)
> +{
> +	mutex_lock(&st->lock);
> +
> +	/* mask disturber interrupt bit */
> +	as3935_write(st, AS3935_INT, 1 << 5);
> +
> +	as3935_write(st, AS3935_CALIBRATE, 0x96);
> +	as3935_write(st, AS3935_TUNE_CAP, 1 << 5 | st->tune_cap);
> +
> +	mdelay(2);
> +	as3935_write(st, AS3935_TUNE_CAP, st->tune_cap);
> +
> +	mutex_unlock(&st->lock);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int as3935_suspend(struct spi_device *spi, pm_message_t msg)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct as3935_state *st = iio_priv(indio_dev);
> +	int val, ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = as3935_read(st, AS3935_AFE_GAIN, &val);
> +	if (ret)
> +		return ret;
Mutex not released.

> +	val |= AS3935_AFE_PWR_BIT;
> +
> +	ret = as3935_write(st, AS3935_AFE_GAIN, val);
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +static int as3935_resume(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct as3935_state *st = iio_priv(indio_dev);
> +	int val, ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = as3935_read(st, AS3935_AFE_GAIN, &val);
> +	if (ret)
> +		return ret;
You haven't released the mutex in this error path.  This is a classic
case for using a goto rather than returning directly here.

> +	val &= ~AS3935_AFE_PWR_BIT;
> +	ret = as3935_write(st, AS3935_AFE_GAIN, val);
> +	mutex_unlock(&st->lock);
blank line before returns in simple cases like this.  Just makes things
ever so slightly easier to read ;)
> +	return ret;
> +}
> +#else
> +#define as3935_suspend	NULL
> +#define as3935_resume	NULL
> +#endif
> +
> +static int as3935_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct iio_trigger *trig;
> +	struct as3935_state *st;
> +	struct device_node *np = spi->dev.of_node;
> +	int ret;
> +
/* Make sure the lightning event interrupt is specified. */
You probably don't need to do this as the request will fail if supplied
with a 0. Even if you want to keep this, it would be cleaner to have it
down where the request for the irq is made rather than here at the start.
> +	/* Be sure lightning event interrupt */
> +	if (!spi->irq) {
> +		dev_err(&spi->dev, "unable to get event interrupt\n");
> +		return -EINVAL;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +	st->tune_cap = 0;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	mutex_init(&st->lock);
> +	INIT_DELAYED_WORK(&st->work, as3935_event_work);
> +
> +	ret = of_property_read_u32(np, "ams,tune-cap", &st->tune_cap);
> +	if (ret) {
> +		st->tune_cap = 0;
> +		dev_warn(&spi->dev,
> +			"no tune-cap set, defaulting to %d", st->tune_cap);
> +	}
> +
> +	if (st->tune_cap > 15) {
> +		dev_err(&spi->dev,
> +			"wrong tune-cap setting of %d\n", st->tune_cap);
> +		return -EINVAL;
> +	}
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->channels = as3935_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(as3935_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &as3935_info;
> +
> +	trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> +				      indio_dev->name, indio_dev->id);
> +
> +	if (!trig)
> +		return -ENOMEM;
> +
> +	st->trig = trig;
> +	trig->dev.parent = indio_dev->dev.parent;
> +	iio_trigger_set_drvdata(trig, indio_dev);
> +	trig->ops = &iio_interrupt_trigger_ops;
> +
> +	ret = iio_trigger_register(trig);
> +	if (ret) {
> +		dev_err(&spi->dev, "failed to register trigger\n");
> +		return ret;
> +	}
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					&as3935_trigger_handler,
> +					&iio_triggered_buffer_setup_ops);
> +
> +	if (ret) {
> +		dev_err(&spi->dev, "cannot setup iio trigger\n");
> +		goto unregister_trigger;
> +	}
> +
> +	calibrate_as3935(st);
> +
> +	ret = devm_request_irq(&spi->dev, spi->irq,
> +				&as3935_interrupt_handler,
> +				IRQF_TRIGGER_RISING,
> +				dev_name(&spi->dev),
> +				indio_dev);
> +
> +	if (ret) {
> +		dev_err(&spi->dev, "unable to request irq\n");
> +		goto unregister_trigger;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "unable to register device\n");
> +		goto unregister_trigger;
> +	}
> +	return 0;
> +
> +unregister_trigger:
> +	iio_trigger_unregister(st->trig);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return ret;
> +};
> +
> +static int as3935_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct as3935_state *st = iio_priv(indio_dev);
> +
> +	iio_trigger_unregister(st->trig);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	iio_device_unregister(indio_dev);
This should be in the reverse order of what goes on in the probe.
This helps both for review purposes and to avoid actual bugs. Here for
example, the buffer has been destroyed before the userspace interface
is removed in the device_unregister.

> +
> +	return 0;
> +};
> +
> +static const struct spi_device_id as3935_id[] = {
> +	{"as3935", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(spi, as3935_id);
> +
> +static struct spi_driver as3935_driver = {
> +	.driver = {
> +		.name	= "as3935",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= as3935_probe,
> +	.remove		= as3935_remove,
> +	.id_table	= as3935_id,
> +	.suspend	= as3935_suspend,
> +	.resume		= as3935_resume,
> +};
> +module_spi_driver(as3935_driver);
> +
> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
> +MODULE_DESCRIPTION("AS3935 lightning sensor");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:as3935");
>

Re: [PATCH 2/2] iio: Add AS3935 lightning sensor support

[Matt Ranostay]

On Sun, Feb 9, 2014 at 1:48 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 06/02/14 07:00, Matt Ranostay wrote:
>>
>> AS3935 chipset can detect lightning strikes and reports those back as
>> events and the estimated distance to the storm.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>
> Hi Matt,
>
> Sorry for my somewhat late entry in the discussion of the interface for this
> device, but I'm wondering if we don't have an opportunity to create a more
> flexible interface for 'position' measurements.  Afterall sooner or later
> we'll get a magetic position sensor or similar coming through.
>
> So how about defining a new iio_chan_type IIO_POSITION and adding some
> more modifiers to allow for polar coordinates (spherical coordinates I guess
> given we are in 3D). For now we'd only need IIO_MOD_R though later I guess
> we would have IIO_MOD_THETA and IIO_MOD_PHI.  That would in this case give
> us.
>
> in_position_r_raw with the unit after applying scaling being meters.
>
> Clearly the ideal would have been to bring this suggestion up before we
> had the proximity sensors but such is life.  Those tend to simply indicate
> something is 'near' rather than at an explicity distance.  The difference
> is clearly minor though.
>
> What do people think of this approach?
>
> Comments on the code inline. Note I review from the end (usually makes
> more sense) so comments may only make sense in that direction!
>
> Beware of the quirks of spi buses as highlighted inline.  You have to allow
> for the buffers you provide being used directly for DMA transfers (unlike
> say i2c which copies everything). As such there are some restrictions on
> where these buffers must lie.
>
> Jonathan
>
>> ---
>>   .../ABI/testing/sysfs-bus-iio-proximity-as3935     |  18 +
>>   drivers/iio/Kconfig                                |   1 +
>>   drivers/iio/Makefile                               |   1 +
>>   drivers/iio/proximity/Kconfig                      |  19 +
>>   drivers/iio/proximity/Makefile                     |   6 +
>>   drivers/iio/proximity/as3935.c                     | 437
>> +++++++++++++++++++++
>>   6 files changed, 482 insertions(+)
>>   create mode 100644
>> Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
>>   create mode 100644 drivers/iio/proximity/Kconfig
>>   create mode 100644 drivers/iio/proximity/Makefile
>>   create mode 100644 drivers/iio/proximity/as3935.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
>> b/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
>> new file mode 100644
>> index 0000000..f6d9e6f
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935
>> @@ -0,0 +1,18 @@
>> +What           /sys/bus/iio/devices/iio:deviceX/in_proximity_raw
>> +Date:          January 2014
>> +KernelVersion: 3.15
>> +Contact:       Matt Ranostay <mranostay@gmail.com>
>> +Description:
>> +               Get the current distance in kilometers of storm
>> +               1    = storm overhead
>> +               1-40 = distance in kilometers
>> +               63   = out of range
>
> This attribute should be a general purpose one given it will apply to lots
> of other drivers over time.  Write it as such and distances should
> definitely
> be in meters not kilometers.
>
>
>> +
>> +What           /sys/bus/iio/devices/iio:deviceX/gain_boost
>> +Date:          January 2014
>> +KernelVersion: 3.15
>> +Contact:       Matt Ranostay <mranostay@gmail.com>
>> +Description:
>> +               Show or set the gain boost of the amp, from 0-31 range.
>> +               18 = indoors (default)
>> +               14 = outdoors
>
> What does this gain boost actually mean?  I couldn't immediately tell from
> the datasheet.  Does it effect the distance estimates, or is it about
> sensitivity to detect them in the first place?  Even though you have defined
> it just for this one device, we are always looking to generalize interfaces
> so it is a good to have an idea of what it actually is.
>

Controls the goo

>
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 5dd0e12..743485e 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -74,6 +74,7 @@ if IIO_TRIGGER
>>      source "drivers/iio/trigger/Kconfig"
>>   endif #IIO_TRIGGER
>>   source "drivers/iio/pressure/Kconfig"
>> +source "drivers/iio/proximity/Kconfig"
>>   source "drivers/iio/temperature/Kconfig"
>>
>>   endif # IIO
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 887d390..698afc2 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -24,5 +24,6 @@ obj-y += light/
>>   obj-y += magnetometer/
>>   obj-y += orientation/
>>   obj-y += pressure/
>> +obj-y += proximity/
>>   obj-y += temperature/
>>   obj-y += trigger/
>> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
>> new file mode 100644
>> index 0000000..0c8cdf5
>> --- /dev/null
>> +++ b/drivers/iio/proximity/Kconfig
>> @@ -0,0 +1,19 @@
>> +#
>> +# Proximity sensors
>> +#
>> +
>> +menu "Lightning sensors"
>> +
>> +config AS3935
>> +       tristate "AS3935 Franklin lightning sensor"
>> +       select IIO_BUFFER
>> +       select IIO_TRIGGERED_BUFFER
>> +       depends on SPI
>> +       help
>> +         Say Y here to build SPI interface support for the Austrian
>> +         Microsystems AS3935 lightning detection sensor.
>> +
>> +         To compile this driver as a module, choose M here: the
>> +         module will be called as3935
>> +
>> +endmenu
>> diff --git a/drivers/iio/proximity/Makefile
>> b/drivers/iio/proximity/Makefile
>> new file mode 100644
>> index 0000000..743adee
>> --- /dev/null
>> +++ b/drivers/iio/proximity/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for IIO proximity sensors
>> +#
>> +
>> +# When adding new entries keep the list in alphabetical order
>> +obj-$(CONFIG_AS3935)           += as3935.o
>> diff --git a/drivers/iio/proximity/as3935.c
>> b/drivers/iio/proximity/as3935.c
>> new file mode 100644
>> index 0000000..109759e
>> --- /dev/null
>> +++ b/drivers/iio/proximity/as3935.c
>> @@ -0,0 +1,437 @@
>> +/*
>> + * as3935.c - Support for AS3935 Franklin lightning sensor
>> + *
>> + * Copyright (C) 2014 Matt Ranostay <mranostay@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/delay.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/mutex.h>
>> +#include <linux/err.h>
>> +#include <linux/irq.h>
>> +#include <linux/gpio.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/of_gpio.h>
>> +
>> +
>> +#define AS3935_AFE_GAIN                0x00
>> +#define AS3935_AFE_MASK                0x3F
>> +#define AS3935_AFE_GAIN_MAX    0x1F
>> +#define AS3935_AFE_PWR_BIT     BIT(0)
>> +
>> +#define AS3935_INT             0x03
>> +#define AS3935_INT_MASK                0x07
>> +#define AS3935_EVENT_INT       BIT(3)
>> +#define AS3935_NOISE_INT       BIT(1)
>> +
>> +#define AS3935_DATA            0x07
>> +#define AS3935_DATA_MASK       0x3F
>> +
>> +#define AS3935_TUNE_CAP                0x08
>> +#define AS3935_CALIBRATE       0x3D
>> +
>> +#define AS3935_WRITE_DATA      BIT(15)
>> +#define AS3935_READ_DATA       BIT(14)
>
> ((x) << 8) to protect against strange elements being passed in as x.
> Obviously you have this tightly controlled here, but who knows what others
> might add in future, so best to do it right!
>
>> +#define AS3935_ADDRESS(x)      (x<<8)
>> +
>> +struct as3935_state {
>> +       struct spi_device *spi;
>> +       struct iio_trigger *trig;
>> +       struct mutex lock;
>> +       struct delayed_work work;
>> +
>> +       u32 tune_cap;
>> +};
>> +
>> +static const struct iio_chan_spec as3935_channels[] = {
>> +       {
>> +               .type           = IIO_PROXIMITY,
>> +               .info_mask_separate =
>> +                       BIT(IIO_CHAN_INFO_RAW),
>> +               .scan_index     = 0,
>> +               .scan_type = {
>> +                       .sign           = 'u',
>> +                       .realbits       = 6,
>> +                       .storagebits    = 8,
>> +               },
>> +       },
>> +       IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>
> This looks very similar to val = spi_w8r8(st->spi, reg);
>
>
>> +static int as3935_read(struct as3935_state *st, unsigned int reg, int
>> *val)
>> +{
>> +       u8 tx, rx;
>
> Spi buffers should never be on the stack. You can't guarantee alignment and
> they must be cache aligned.  spi_w8r8 deals with this by using a small
> buffer allocated on the heap.  The alternative is to carefully allocate
> a cache line aligned buffer in your state structure.  Here I'd definitely
> use spi_w8r8 though!
>

Noted will do the former.. having it the in the as3935_state struct
would make sense if the buffer was needed all over.

>> +       int ret;
>> +
>> +       struct spi_transfer xfers[] = {
>> +               {
>> +                       .tx_buf = &tx,
>> +                       .bits_per_word = 8,
>> +                       .len = 1,
>> +               }, {
>> +                       .rx_buf = &rx,
>> +                       .bits_per_word = 8,
>> +                       .len = 1,
>> +               },
>> +       };
>> +       tx = (AS3935_READ_DATA | AS3935_ADDRESS(reg)) >> 8;
>> +
>> +       ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
>> +       *val = rx;
>> +
>> +       return ret;
>> +};
>> +
>
>
>> +static int as3935_write(struct as3935_state *st,
>> +                               unsigned int reg,
>> +                               unsigned int val)
>> +{
>> +       u8 buf[2];
>
> You could use a C99 style asignment here (entirely optional!)
> e.g.
>         u8 buf[2] = { (AS3935_WRITE_DATA | AS3935_ADDRESS(reg)) >> 8,
>                       val };
> However, this buffer must be cacheline aligned seeing as spi_write
> does not use a suitably aligned bounce buffer (unlike spi_w8r8 which does).
> This can cause nasty, difficult to track down corruptions with spi
> controllers
> that do DMA.
>
>> +
>> +       buf[0] = (AS3935_WRITE_DATA | AS3935_ADDRESS(reg)) >> 8;
>> +       buf[1] = val;
>> +
>> +       return spi_write(st->spi, (u8 *) &buf, 2);
>> +};
>> +
>> +static ssize_t as3935_gain_boost_show(struct device *dev,
>> +                               struct device_attribute *attr,
>> +                               char *buf)
>> +{
>> +       struct as3935_state *st = iio_priv(dev_to_iio_dev(dev));
>> +       int val, ret;
>> +
>> +       ret = as3935_read(st, AS3935_AFE_GAIN, &val);
>> +       if (ret)
>> +               return ret;
>> +       val = (val & AS3935_AFE_MASK) >> 1;
>> +
>> +       return sprintf(buf, "%d\n", val);
>> +};
>> +
>> +static ssize_t as3935_gain_boost_store(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       const char *buf, size_t len)
>> +{
>> +       struct as3935_state *st = iio_priv(dev_to_iio_dev(dev));
>> +       unsigned long val;
>> +       int ret;
>> +
>> +       ret = kstrtoul((const char *) buf, 10, &val);
>> +       if (ret)
>> +               return -EINVAL;
>> +
>> +       if (val > AS3935_AFE_GAIN_MAX)
>> +               return -EINVAL;
>> +
>> +       as3935_write(st, AS3935_AFE_GAIN, val << 1);
>> +
>> +       return len;
>> +};
>> +
>> +static IIO_DEVICE_ATTR(gain_boost, S_IRUGO | S_IWUSR,
>> +       as3935_gain_boost_show, as3935_gain_boost_store, 0);
>> +
>> +
>> +static struct attribute *as3935_attributes[] = {
>> +       &iio_dev_attr_gain_boost.dev_attr.attr,
>> +       NULL,
>> +};
>> +
>> +static struct attribute_group as3935_attribute_group = {
>> +       .attrs = as3935_attributes,
>> +};
>> +
>> +static int as3935_read_raw(struct iio_dev *indio_dev,
>> +                          struct iio_chan_spec const *chan,
>> +                          int *val,
>> +                          int *val2,
>> +                          long m)
>> +{
>> +       struct as3935_state *st = iio_priv(indio_dev);
>> +       int ret;
>> +
>> +       if (m != IIO_CHAN_INFO_RAW)
>> +               return -EINVAL;
>> +
>> +       *val2 = 0;
>> +       ret = as3935_read(st, AS3935_DATA, val);
>> +       if (ret)
>> +               return ret;
>> +       return IIO_VAL_INT;
>> +}
>> +
>> +static const struct iio_info as3935_info = {
>> +       .driver_module = THIS_MODULE,
>> +       .attrs = &as3935_attribute_group,
>> +       .read_raw = &as3935_read_raw,
>> +};
>> +
>> +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops =
>> {
>> +       .postenable = &iio_triggered_buffer_postenable,
>> +       .predisable = &iio_triggered_buffer_predisable,
>> +};
>> +
>> +static irqreturn_t as3935_trigger_handler(int irq, void *private)
>> +{
>> +       struct iio_poll_func *pf = private;
>> +       struct iio_dev *indio_dev = pf->indio_dev;
>> +       struct as3935_state *st = iio_priv(indio_dev);
>> +       int val, ret;
>> +
>> +       ret = as3935_read(st, AS3935_DATA, &val);
>> +       if (ret)
>> +               goto err_read;
>> +       val &= AS3935_DATA_MASK;
>
> This timestamp is probably rather late given I think the interrupt indicated
> the actual point in time?
>
>> +       iio_push_to_buffers_with_timestamp(indio_dev, &val,
>> iio_get_time_ns());
>> +err_read:
>> +       iio_trigger_notify_done(indio_dev->trig);
>> +
>> +       return IRQ_HANDLED;
>> +};
>> +
>> +static const struct iio_trigger_ops iio_interrupt_trigger_ops = {
>> +       .owner = THIS_MODULE,
>> +};
>> +
>> +static void as3935_event_work(struct work_struct *work)
>> +{
>> +       struct as3935_state *st;
>> +       struct spi_device *spi;
>> +       int val;
>> +
>> +       st = container_of(work, struct as3935_state, work.work);
>> +       spi = st->spi;
>
> Don't bother with the local variable for spi. It doesn't add any clarity.
>
>> +
>> +       as3935_read(st, AS3935_INT, &val);
>> +       val &= AS3935_INT_MASK;
>> +
>> +       switch (val) {
>> +       case AS3935_EVENT_INT:
>
> You could save a timestamp in the interrupt handler then pass it on here.
>
Noted.

>> +               iio_trigger_poll(st->trig, 0);
>> +               break;
>> +       case AS3935_NOISE_INT:
>
> Hmm. This brings me back to one of my favourite kernel questions,
> 'Why isn't there a better way of handling hardware errors?'
> There isn't as far as I know!
>
>> +               dev_warn(&spi->dev, "noise level is too high");
>> +               break;
>> +       }
>> +};
>> +
>> +static irqreturn_t as3935_interrupt_handler(int irq, void *private)
>> +{
>> +       struct iio_dev *indio_dev = private;
>> +       struct as3935_state *st = iio_priv(indio_dev);
>> +
>
> This is 'unusual' so can you add a comment explaining what is going on here.
> My guess is that you want to avoid grabing new data for a short window after
> a strike is detected?  The cancel is to avoid running the delayed work
> twice?
>
> If so the reason a sleep in a threaded handler wouldn't work is that it
> would
> not 'cancel' if new data arrived I guess.  You still have a window for
> problems
> if the interrupt occurs whilst the work queue is actually running the
> scheduled work.
>
> I'm wondering if this is overcomplicating things and it is better to just
> occasionally handle an extra interrupt and do this with a conventional
> threaded interrupt handler.  Of course I may be missing something.
>>
>> +       cancel_delayed_work(&st->work);
>
> I'm guessing this should be msecs_to_jiffies seeing as schedule_delayed_work
> takes a value in jiffies.
>>
>> +       schedule_delayed_work(&st->work, jiffies_to_msecs(3));
>

Yes it should be msec_to_jiffies(3) :/
Noted the cancel_delayed_work() probably is verbose and will drop in
the next version.

Reason for this is the datasheet requires (suggests?) a 2+ millisecond
delay before reading
estimated distance value.

> blank line here.
>
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static void calibrate_as3935(struct as3935_state *st)
>> +{
>> +       mutex_lock(&st->lock);
>> +
>> +       /* mask disturber interrupt bit */
>> +       as3935_write(st, AS3935_INT, 1 << 5);
>> +
>> +       as3935_write(st, AS3935_CALIBRATE, 0x96);
>> +       as3935_write(st, AS3935_TUNE_CAP, 1 << 5 | st->tune_cap);
>> +
>> +       mdelay(2);
>> +       as3935_write(st, AS3935_TUNE_CAP, st->tune_cap);
>> +
>> +       mutex_unlock(&st->lock);
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int as3935_suspend(struct spi_device *spi, pm_message_t msg)
>> +{
>> +       struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +       struct as3935_state *st = iio_priv(indio_dev);
>> +       int val, ret;
>> +
>> +       mutex_lock(&st->lock);
>> +       ret = as3935_read(st, AS3935_AFE_GAIN, &val);
>> +       if (ret)
>> +               return ret;
>
> Mutex not released.
>
>
>> +       val |= AS3935_AFE_PWR_BIT;
>> +
>> +       ret = as3935_write(st, AS3935_AFE_GAIN, val);
>> +       mutex_unlock(&st->lock);
>> +       return ret;
>> +}
>> +
>> +static int as3935_resume(struct spi_device *spi)
>> +{
>> +       struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +       struct as3935_state *st = iio_priv(indio_dev);
>> +       int val, ret;
>> +
>> +       mutex_lock(&st->lock);
>> +       ret = as3935_read(st, AS3935_AFE_GAIN, &val);
>> +       if (ret)
>> +               return ret;
>
> You haven't released the mutex in this error path.  This is a classic
> case for using a goto rather than returning directly here.
>

Yeah this a rookie mistake on my behalf.

>
>> +       val &= ~AS3935_AFE_PWR_BIT;
>> +       ret = as3935_write(st, AS3935_AFE_GAIN, val);
>> +       mutex_unlock(&st->lock);
>
> blank line before returns in simple cases like this.  Just makes things
> ever so slightly easier to read ;)
>>
>> +       return ret;
>> +}
>>
>> +#else
>> +#define as3935_suspend NULL
>> +#define as3935_resume  NULL
>> +#endif
>> +
>> +static int as3935_probe(struct spi_device *spi)
>> +{
>> +       struct iio_dev *indio_dev;
>> +       struct iio_trigger *trig;
>> +       struct as3935_state *st;
>> +       struct device_node *np = spi->dev.of_node;
>> +       int ret;
>> +
>
> /* Make sure the lightning event interrupt is specified. */
> You probably don't need to do this as the request will fail if supplied
> with a 0. Even if you want to keep this, it would be cleaner to have it
> down where the request for the irq is made rather than here at the start.
>

Didn't know that.. all for no

>> +       /* Be sure lightning event interrupt */
>> +       if (!spi->irq) {
>> +               dev_err(&spi->dev, "unable to get event interrupt\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(st));
>> +       if (!indio_dev)
>> +               return -ENOMEM;
>> +
>> +       st = iio_priv(indio_dev);
>> +       st->spi = spi;
>> +       st->tune_cap = 0;
>> +
>> +       spi_set_drvdata(spi, indio_dev);
>> +       mutex_init(&st->lock);
>> +       INIT_DELAYED_WORK(&st->work, as3935_event_work);
>> +
>> +       ret = of_property_read_u32(np, "ams,tune-cap", &st->tune_cap);
>> +       if (ret) {
>> +               st->tune_cap = 0;
>> +               dev_warn(&spi->dev,
>> +                       "no tune-cap set, defaulting to %d",
>> st->tune_cap);
>> +       }
>> +
>> +       if (st->tune_cap > 15) {
>> +               dev_err(&spi->dev,
>> +                       "wrong tune-cap setting of %d\n", st->tune_cap);
>> +               return -EINVAL;
>> +       }
>> +
>> +       indio_dev->dev.parent = &spi->dev;
>> +       indio_dev->name = spi_get_device_id(spi)->name;
>> +       indio_dev->channels = as3935_channels;
>> +       indio_dev->num_channels = ARRAY_SIZE(as3935_channels);
>> +       indio_dev->modes = INDIO_DIRECT_MODE;
>> +       indio_dev->info = &as3935_info;
>> +
>> +       trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
>> +                                     indio_dev->name, indio_dev->id);
>> +
>> +       if (!trig)
>> +               return -ENOMEM;
>> +
>> +       st->trig = trig;
>> +       trig->dev.parent = indio_dev->dev.parent;
>> +       iio_trigger_set_drvdata(trig, indio_dev);
>> +       trig->ops = &iio_interrupt_trigger_ops;
>> +
>> +       ret = iio_trigger_register(trig);
>> +       if (ret) {
>> +               dev_err(&spi->dev, "failed to register trigger\n");
>> +               return ret;
>> +       }
>> +
>> +       ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                                       &as3935_trigger_handler,
>> +                                       &iio_triggered_buffer_setup_ops);
>> +
>> +       if (ret) {
>> +               dev_err(&spi->dev, "cannot setup iio trigger\n");
>> +               goto unregister_trigger;
>> +       }
>> +
>> +       calibrate_as3935(st);
>> +
>> +       ret = devm_request_irq(&spi->dev, spi->irq,
>> +                               &as3935_interrupt_handler,
>> +                               IRQF_TRIGGER_RISING,
>> +                               dev_name(&spi->dev),
>> +                               indio_dev);
>> +
>> +       if (ret) {
>> +               dev_err(&spi->dev, "unable to request irq\n");
>> +               goto unregister_trigger;
>> +       }
>> +
>> +       ret = iio_device_register(indio_dev);
>> +       if (ret < 0) {
>> +               dev_err(&spi->dev, "unable to register device\n");
>> +               goto unregister_trigger;
>> +       }
>> +       return 0;
>> +
>> +unregister_trigger:
>> +       iio_trigger_unregister(st->trig);
>> +       iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +       return ret;
>> +};
>> +
>> +static int as3935_remove(struct spi_device *spi)
>> +{
>> +       struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +       struct as3935_state *st = iio_priv(indio_dev);
>> +
>> +       iio_trigger_unregister(st->trig);
>> +       iio_triggered_buffer_cleanup(indio_dev);
>> +       iio_device_unregister(indio_dev);
>
> This should be in the reverse order of what goes on in the probe.
> This helps both for review purposes and to avoid actual bugs. Here for
> example, the buffer has been destroyed before the userspace interface
> is removed in the device_unregister.
>

Noted.

>> +
>> +       return 0;
>> +};
>> +
>> +static const struct spi_device_id as3935_id[] = {
>> +       {"as3935", 0},
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(spi, as3935_id);
>> +
>> +static struct spi_driver as3935_driver = {
>> +       .driver = {
>> +               .name   = "as3935",
>> +               .owner  = THIS_MODULE,
>> +       },
>> +       .probe          = as3935_probe,
>> +       .remove         = as3935_remove,
>> +       .id_table       = as3935_id,
>> +       .suspend        = as3935_suspend,
>> +       .resume         = as3935_resume,
>> +};
>> +module_spi_driver(as3935_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>> +MODULE_DESCRIPTION("AS3935 lightning sensor");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("spi:as3935");
>>
>