Message-ID: <52F7F79C.1010001@kernel.org>
Date:	Sun, 09 Feb 2014 21:48:12 +0000
From:	Jonathan Cameron <jic23@...nel.org>
To:	Matt Ranostay <mranostay@...il.com>, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org
CC:	matt.porter@...aro.org, pantelis.antoniou@...il.com
Subject: Re: [PATCH 2/2] iio: Add AS3935 lightning sensor support

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@...il.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@...il.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@...il.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@...il.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@...il.com>");
> +MODULE_DESCRIPTION("AS3935 lightning sensor");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:as3935");
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists