* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-01 12:20 ` [PATCH 1/3] iio: adc: hi-843x: " Vladimir Barinov
@ 2015-06-01 12:55 ` Peter Meerwald
2015-06-01 14:41 ` Vladimir Barinov
2015-06-03 7:05 ` Paul Bolle
2015-06-07 16:11 ` Jonathan Cameron
2 siblings, 1 reply; 14+ messages in thread
From: Peter Meerwald @ 2015-06-01 12:55 UTC (permalink / raw)
To: Vladimir Barinov
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, linux-iio,
devicetree
> Add Holt descrete ADC driver for HI-8435/8436/8437 chips
discrete
link to datasheet would be nice, comments below
what is the purpose of the driver?
the driver-specific ABI needs to be documented under
Documentation/ABI/testing/sys-bus-iio-*
>
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> ---
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/hi-843x.c | 777 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 790 insertions(+)
> create mode 100644 drivers/iio/adc/hi-843x.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index e36a73e..71b0efc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -164,6 +164,18 @@ config EXYNOS_ADC
> of SoCs for drivers such as the touchscreen and hwmon to use to share
> this resource.
>
> +config HI_843X
we recommend against using a placeholder in a drivers name;
we suggest to name the driver after the first/primary chip it supports
(that you test with most)
> + tristate "Holt Integrated Circuits HI-8435/8436/8437"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + depends on SPI
> + help
> + If you say yes here you get support for Holt Integrated Circuits
> + HI-8435/8436/8437 chip.
> +
> + This driver can also be built as a module. If so, the module will be
> + called hi-843x.
> +
> config LP8788_ADC
> tristate "LP8788 ADC driver"
> depends on MFD_LP8788
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 3930e63..65f54c2 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> +obj-$(CONFIG_HI_843X) += hi-843x.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_MAX1027) += max1027.o
> obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/hi-843x.c b/drivers/iio/adc/hi-843x.c
> new file mode 100644
> index 0000000..ccc46e7
> --- /dev/null
> +++ b/drivers/iio/adc/hi-843x.c
> @@ -0,0 +1,777 @@
> +/*
> + * Holt Integrated Circuits HI-8435/8436/8437 discrete ADC driver
> + *
> + * Copyright (C) 2015 Zodiac Inflight Innovations
> + * Copyright (C) 2015 Cogent Embedded, Inc.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/iio/buffer.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/triggered_buffer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/workqueue.h>
> +
> +#include <linux/interrupt.h>
> +
> +#define DRV_NAME "hi-843x"
HI84.. prefix
> +
> +/* Register offsets for HI-843X */
> +#define HI843X_CTRL_REG 0x02
> +#define HI843X_PSEN_REG 0x04
> +#define HI843X_TMDATA_REG 0x1E
> +#define HI843X_GOCENHYS_REG 0x3A
> +#define HI843X_SOCENHYS_REG 0x3C
> +#define HI843X_SO7_0_REG 0x10
> +#define HI843X_SO15_8_REG 0x12
> +#define HI843X_SO23_16_REG 0x14
> +#define HI843X_SO31_24_REG 0x16
> +#define HI843X_SO31_0_REG 0x78
> +
> +#define HI843X_WRITE_OPCODE 0x00
> +#define HI843X_READ_OPCODE 0x80
> +
> +/* THRESHOLD mask */
> +#define HI843X_THRESHOLD_MAX 0x3f
> +#define HI843X_THRESHOLD_MASK 0xff
> +
> +/* CTRL register bits */
> +#define HI843X_CTRL_TEST 0x01
> +#define HI843X_CTRL_SRST 0x02
> +
> +#define HI843X_DEBOUNCE_SOFT_DELAY_MAX 1000 /* ms */
> +#define HI843X_DEBOUNCE_SOFT_DELAY_DEF 100 /* ms */
> +
> +struct hi843x_priv {
> + struct spi_device *spi;
> + struct mutex lock;
> + struct delayed_work work;
> +
> + int mr_gpio;
> + bool debounce_soft;
> + int debounce_soft_delay;
> + int debounce_soft_val;
> +
> + void *iio_buffer;
> + u8 reg_buffer[4] ____cacheline_aligned;
> +};
> +
> +static int hi843x_readb(struct hi843x_priv *priv, u8 reg, u8 *val)
> +{
> + reg |= HI843X_READ_OPCODE;
> + return spi_write_then_read(priv->spi, ®, 1, val, 1);
> +}
> +
> +static int hi843x_readw(struct hi843x_priv *priv, u8 reg, u16 *val)
> +{
> + int ret;
> +
> + reg |= HI843X_READ_OPCODE;
> + ret = spi_write_then_read(priv->spi, ®, 1, val, 2);
> + *val = swab16p(val);
will this work on big- and little-endian CPUs?
> +
> + return ret;
> +}
> +
> +static int hi843x_readl(struct hi843x_priv *priv, u8 reg, u32 *val)
> +{
> + int ret;
> +
> + reg |= HI843X_READ_OPCODE;
> + ret = spi_write_then_read(priv->spi, ®, 1, val, 4);
> + *val = swab32p(val);
> +
> + return ret;
> +}
> +
> +static int hi843x_writeb(struct hi843x_priv *priv, u8 reg, u8 val)
> +{
> + priv->reg_buffer[0] = reg | HI843X_WRITE_OPCODE;
> + priv->reg_buffer[1] = val;
> +
> + return spi_write(priv->spi, priv->reg_buffer, 2);
> +}
> +
> +static int hi843x_writew(struct hi843x_priv *priv, u8 reg, u16 val)
> +{
> + priv->reg_buffer[0] = reg | HI843X_WRITE_OPCODE;
> + priv->reg_buffer[1] = (val >> 8) & 0xff;
> + priv->reg_buffer[2] = val & 0xff;
> +
> + return spi_write(priv->spi, priv->reg_buffer, 3);
> +}
> +
> +ssize_t hi843x_debounce_soft_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "%d\n", priv->debounce_soft);
> +}
> +
> +ssize_t hi843x_debounce_soft_delay_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "%d\n", priv->debounce_soft_delay);
> +}
> +
> +ssize_t hi843x_sensing_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u8 reg;
> +
> + ret = hi843x_readb(priv, HI843X_PSEN_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg);
> +}
> +
> +ssize_t hi843x_test_enable_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u8 reg;
> +
> + ret = hi843x_readb(priv, HI843X_CTRL_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg & HI843X_CTRL_TEST);
> +}
> +
> +ssize_t hi843x_test_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u8 reg;
> +
> + ret = hi843x_readb(priv, HI843X_TMDATA_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg);
> +}
> +
> +ssize_t hi843x_debounce_soft_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + priv->debounce_soft = !!val;
> +
> + return len;
> +}
> +
> +ssize_t hi843x_debounce_soft_delay_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_DEBOUNCE_SOFT_DELAY_MAX)
> + val = HI843X_DEBOUNCE_SOFT_DELAY_MAX;
> +
> + priv->debounce_soft_delay = val;
> +
> + return len;
> +}
> +
> +ssize_t hi843x_sensing_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + hi843x_writeb(priv, HI843X_PSEN_REG, val & 0xf);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_test_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + hi843x_writeb(priv, HI843X_CTRL_REG, val ? HI843X_CTRL_TEST : 0);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_test_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + hi843x_writeb(priv, HI843X_TMDATA_REG, val & 0xf);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_gohys_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", (reg >> 8) & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_gocval_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_sohys_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", (reg >> 8) & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_socval_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_gohys_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~(HI843X_THRESHOLD_MASK << 8);
> + reg |= (val << 8);
> + hi843x_writew(priv, HI843X_GOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_gocval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~HI843X_THRESHOLD_MASK;
> + reg |= val;
> + hi843x_writew(priv, HI843X_GOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_sohys_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~(HI843X_THRESHOLD_MASK << 8);
> + reg |= (val << 8);
> + hi843x_writew(priv, HI843X_SOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_socval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~HI843X_THRESHOLD_MASK;
> + reg |= val;
> + hi843x_writew(priv, HI843X_SOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(debounce_soft, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_show, hi843x_debounce_soft_store, 0);
> +static IIO_DEVICE_ATTR(debounce_soft_delay, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_delay_show, hi843x_debounce_soft_delay_store, 0);
> +static IIO_DEVICE_ATTR(sensing_mode, S_IRUGO | S_IWUSR,
> + hi843x_sensing_mode_show, hi843x_sensing_mode_store, 0);
> +static IIO_DEVICE_ATTR(test_enable, S_IRUGO | S_IWUSR,
> + hi843x_test_enable_show, hi843x_test_enable_store, 0);
> +static IIO_DEVICE_ATTR(test_mode, S_IRUGO | S_IWUSR,
> + hi843x_test_mode_show, hi843x_test_mode_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gohys_show, hi843x_threshold_gohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gocval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gocval_show, hi843x_threshold_gocval_store, 0);
> +static IIO_DEVICE_ATTR(threshold_sohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_sohys_show, hi843x_threshold_sohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_socval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_socval_show, hi843x_threshold_socval_store, 0);
> +
> +static struct attribute *hi843x_attributes[] = {
> + &iio_dev_attr_debounce_soft.dev_attr.attr,
> + &iio_dev_attr_debounce_soft_delay.dev_attr.attr,
> + &iio_dev_attr_sensing_mode.dev_attr.attr,
> + &iio_dev_attr_test_enable.dev_attr.attr,
> + &iio_dev_attr_test_mode.dev_attr.attr,
> + &iio_dev_attr_threshold_gohys.dev_attr.attr,
> + &iio_dev_attr_threshold_gocval.dev_attr.attr,
> + &iio_dev_attr_threshold_sohys.dev_attr.attr,
> + &iio_dev_attr_threshold_socval.dev_attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group hi843x_attribute_group = {
> + .attrs = hi843x_attributes,
> +};
> +
> +static int hi843x_read_raw(struct iio_dev *idev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + case IIO_CHAN_INFO_RAW:
> + ret = hi843x_readl(priv, HI843X_SO31_0_REG, val);
> + if (ret < 0)
> + return ret;
> +
> + if (mask == IIO_CHAN_INFO_RAW)
> + *val = !!(*val & BIT(channel->channel));
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define HI843X_VOLTAGE_CHANNEL(num) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = num, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_index = num, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 1, \
huh? this is unusual for an ADC
> + .storagebits = 8, \
> + }, \
> + }
> +
> +static const struct iio_chan_spec hi843x_channels[] = {
> + HI843X_VOLTAGE_CHANNEL(0),
> + HI843X_VOLTAGE_CHANNEL(1),
> + HI843X_VOLTAGE_CHANNEL(2),
> + HI843X_VOLTAGE_CHANNEL(3),
> + HI843X_VOLTAGE_CHANNEL(4),
> + HI843X_VOLTAGE_CHANNEL(5),
> + HI843X_VOLTAGE_CHANNEL(6),
> + HI843X_VOLTAGE_CHANNEL(7),
> + HI843X_VOLTAGE_CHANNEL(8),
> + HI843X_VOLTAGE_CHANNEL(9),
> + HI843X_VOLTAGE_CHANNEL(10),
> + HI843X_VOLTAGE_CHANNEL(11),
> + HI843X_VOLTAGE_CHANNEL(12),
> + HI843X_VOLTAGE_CHANNEL(13),
> + HI843X_VOLTAGE_CHANNEL(14),
> + HI843X_VOLTAGE_CHANNEL(15),
> + HI843X_VOLTAGE_CHANNEL(16),
> + HI843X_VOLTAGE_CHANNEL(17),
> + HI843X_VOLTAGE_CHANNEL(18),
> + HI843X_VOLTAGE_CHANNEL(19),
> + HI843X_VOLTAGE_CHANNEL(20),
> + HI843X_VOLTAGE_CHANNEL(21),
> + HI843X_VOLTAGE_CHANNEL(22),
> + HI843X_VOLTAGE_CHANNEL(23),
> + HI843X_VOLTAGE_CHANNEL(24),
> + HI843X_VOLTAGE_CHANNEL(25),
> + HI843X_VOLTAGE_CHANNEL(26),
> + HI843X_VOLTAGE_CHANNEL(27),
> + HI843X_VOLTAGE_CHANNEL(28),
> + HI843X_VOLTAGE_CHANNEL(29),
> + HI843X_VOLTAGE_CHANNEL(30),
> + HI843X_VOLTAGE_CHANNEL(31),
> + {
> + .type = IIO_ALTVOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .scan_index = 32,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(33),
> +};
> +
> +static const struct iio_info hi843x_info = {
> + .driver_module = THIS_MODULE,
> + .attrs = &hi843x_attribute_group,
> + .read_raw = hi843x_read_raw,
> +};
> +
> +static void h843x_iio_push_to_buffers(struct iio_dev *idev, int val)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> + u8 *pbuffer = priv->iio_buffer;
> + int i;
> +
> + for_each_set_bit(i, idev->active_scan_mask, idev->masklength) {
> + if (idev->channels[i].type == IIO_ALTVOLTAGE) {
> + *(u32 *)pbuffer = val;
> + pbuffer += 4;
> + } else {
> + *pbuffer = !!(val & BIT(i));
> + pbuffer++;
> + }
> + }
> + iio_push_to_buffers_with_timestamp(idev, priv->iio_buffer,
> + iio_get_time_ns());
> +}
> +
> +static void hi843x_debounce_soft_work(struct work_struct *work)
> +{
> + struct hi843x_priv *priv = container_of(work, struct hi843x_priv,
> + work.work);
> + struct iio_dev *idev = spi_get_drvdata(priv->spi);
> + int val, ret;
> +
> + ret = hi843x_readl(priv, HI843X_SO31_0_REG, &val);
> + if (ret < 0)
> + return;
> +
> + if (val == priv->debounce_soft_val)
> + h843x_iio_push_to_buffers(idev, val);
> + else
> + dev_warn(&priv->spi->dev, "filtered by soft debounce");
> +}
> +
> +static irqreturn_t hi843x_trigger_handler(int irq, void *private)
> +{
> + struct iio_poll_func *pf = private;
> + struct iio_dev *idev = pf->indio_dev;
> + struct hi843x_priv *priv = iio_priv(idev);
> + int val, ret;
> +
> + ret = hi843x_readl(priv, HI843X_SO31_0_REG, &val);
> + if (ret < 0)
> + goto err_read;
> +
> + if (priv->debounce_soft) {
> + priv->debounce_soft_val = val;
> + schedule_delayed_work(&priv->work,
> + msecs_to_jiffies(priv->debounce_soft_delay));
> + } else
> + h843x_iio_push_to_buffers(idev, val);
> +
> +err_read:
> + iio_trigger_notify_done(idev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hi843x_buffer_postenable(struct iio_dev *idev)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> +
> + priv->iio_buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
> + if (!priv->iio_buffer)
> + return -ENOMEM;
> +
> + return iio_triggered_buffer_postenable(idev);
> +}
> +
> +static int hi843x_buffer_predisable(struct iio_dev *idev)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> + int ret;
> +
> + ret = iio_triggered_buffer_predisable(idev);
> + if (!ret)
> + kfree(priv->iio_buffer);
> +
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops hi843x_buffer_setup_ops = {
> + .postenable = &hi843x_buffer_postenable,
> + .predisable = &hi843x_buffer_predisable,
> +};
> +
> +static const struct iio_trigger_ops hi843x_trigger_ops = {
> + .owner = THIS_MODULE,
> +};
> +
> +static void hi843x_parse_dt(struct hi843x_priv *priv)
> +{
> + struct device_node *np = priv->spi->dev.of_node;
> + int ret;
> +
> + ret = of_get_named_gpio(np, "holt,mr-gpio", 0);
> + priv->mr_gpio = ret < 0 ? 0 : ret;
> +
> + if (of_find_property(np, "holt,debounce-soft", NULL))
> + priv->debounce_soft = 1;
> +
> + ret = of_property_read_u32(np, "holt,debounce-soft-delay",
> + &priv->debounce_soft_delay);
> + if (ret)
> + priv->debounce_soft_delay = HI843X_DEBOUNCE_SOFT_DELAY_DEF;
> +}
> +
> +static int hi843x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *idev;
> + struct hi843x_priv *priv;
> + int ret;
> +
> + idev = devm_iio_device_alloc(&spi->dev, sizeof(*priv));
> + if (!idev)
> + return -ENOMEM;
> +
> + priv = iio_priv(idev);
> + priv->spi = spi;
> +
> + if (spi->dev.of_node)
> + hi843x_parse_dt(priv);
> +
> + spi_set_drvdata(spi, idev);
> + mutex_init(&priv->lock);
> + INIT_DELAYED_WORK(&priv->work, hi843x_debounce_soft_work);
> +
> + idev->dev.parent = &spi->dev;
> + idev->name = spi_get_device_id(spi)->name;
> + idev->modes = INDIO_DIRECT_MODE;
> + idev->info = &hi843x_info;
> + idev->channels = hi843x_channels;
> + idev->num_channels = ARRAY_SIZE(hi843x_channels);
> +
> + if (priv->mr_gpio) {
> + ret = devm_gpio_request(&spi->dev, priv->mr_gpio, idev->name);
> + if (!ret) {
> + /* chip hardware reset */
> + gpio_direction_output(priv->mr_gpio, 0);
> + udelay(5);
> + gpio_direction_output(priv->mr_gpio, 1);
> + }
> + } else {
> + /* chip software reset */
> + hi843x_writeb(priv, HI843X_CTRL_REG, HI843X_CTRL_SRST);
> + /* get out from reset state */
> + hi843x_writeb(priv, HI843X_CTRL_REG, 0);
> + }
> +
> + ret = iio_triggered_buffer_setup(idev, NULL, hi843x_trigger_handler,
> + &hi843x_buffer_setup_ops);
> + if (ret)
> + return ret;
> +
> + ret = iio_device_register(idev);
> + if (ret < 0) {
> + dev_err(&spi->dev, "unable to register device\n");
> + goto unregister_buffer;
> + }
> +
> + return 0;
> +
> +unregister_buffer:
> + iio_triggered_buffer_cleanup(idev);
> + return ret;
> +}
> +
> +static int hi843x_remove(struct spi_device *spi)
> +{
> + struct iio_dev *idev = spi_get_drvdata(spi);
> + struct hi843x_priv *priv = iio_priv(idev);
> +
> + cancel_delayed_work_sync(&priv->work);
> + iio_device_unregister(idev);
> + iio_triggered_buffer_cleanup(idev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id hi843x_dt_ids[] = {
> + { .compatible = "holt,hi-8435" },
> + { .compatible = "holt,hi-8436" },
> + { .compatible = "holt,hi-8437" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, hi843x_dt_ids);
> +
> +static const struct spi_device_id hi843x_id[] = {
> + { "hi-8435", 0},
> + { "hi-8436", 0},
> + { "hi-8437", 0},
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, hi843x_id);
> +
> +static struct spi_driver hi843x_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .of_match_table = of_match_ptr(hi843x_dt_ids),
> + },
> + .probe = hi843x_probe,
> + .remove = hi843x_remove,
> + .id_table = hi843x_id,
> +};
> +module_spi_driver(hi843x_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Vladimir Barinov");
> +MODULE_DESCRIPTION("HI-8435/8436/8437 discrete ADC");
>
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-01 12:55 ` Peter Meerwald
@ 2015-06-01 14:41 ` Vladimir Barinov
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Barinov @ 2015-06-01 14:41 UTC (permalink / raw)
To: Peter Meerwald
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, linux-iio,
devicetree
Hello Peter,
Thank you for the review.
On 01.06.2015 15:55, Peter Meerwald wrote:
>> Add Holt descrete ADC driver for HI-8435/8436/8437 chips
> discrete
Thx, will replace in the next try.
>
> link to datasheet would be nice, comments below
the official link is here:
http://www.holtic.com/products/3081-hi-8435.aspx
>
> what is the purpose of the driver?
Support hi-8435/36/37 chips in linux kernel via IIO subsystem with use
of iio-buffer and triggered-buffer features.
>
> the driver-specific ABI needs to be documented under
> Documentation/ABI/testing/sys-bus-iio-*
Thanks, I will add this in the next try.
>
>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> ---
>> drivers/iio/adc/Kconfig | 12 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/hi-843x.c | 777 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 790 insertions(+)
>> create mode 100644 drivers/iio/adc/hi-843x.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index e36a73e..71b0efc 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -164,6 +164,18 @@ config EXYNOS_ADC
>> of SoCs for drivers such as the touchscreen and hwmon to use to share
>> this resource.
>>
>> +config HI_843X
> we recommend against using a placeholder in a drivers name;
> we suggest to name the driver after the first/primary chip it supports
> (that you test with most)
Ok, I will name the driver as hi8435_adc/HI8435_ADC in the next try.
> +#include <linux/interrupt.h>
> +
> +#define DRV_NAME "hi-843x"
> HI84.. prefix
sure, as mentioned above I will also use hi8435_adc name and rename all
functions appropriately with this prefix.
> +static int hi843x_readw(struct hi843x_priv *priv, u8 reg, u16 *val)
> +{
> + int ret;
> +
> + reg |= HI843X_READ_OPCODE;
> + ret = spi_write_then_read(priv->spi, ®, 1, val, 2);
> + *val = swab16p(val);
> will this work on big- and little-endian CPUs?
Actually I've tested it with little-endian CPU.
I will replace it with be16_to_cpup in the next try and the same for
swab32p -> be32_to_cpup.
>
> +#define HI843X_VOLTAGE_CHANNEL(num) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = num, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_index = num, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 1, \
> huh? this is unusual for an ADC
This is discrete ADC, only 2 possible results: 0 or 1.
Regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-01 12:20 ` [PATCH 1/3] iio: adc: hi-843x: " Vladimir Barinov
2015-06-01 12:55 ` Peter Meerwald
@ 2015-06-03 7:05 ` Paul Bolle
2015-06-03 7:46 ` Vladimir Barinov
2015-06-07 16:11 ` Jonathan Cameron
2 siblings, 1 reply; 14+ messages in thread
From: Paul Bolle @ 2015-06-03 7:05 UTC (permalink / raw)
To: Vladimir Barinov
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, linux-kernel, linux-iio, devicetree
Just a nit, that I spotted while scanning for other issues.
On Mon, 2015-06-01 at 15:20 +0300, Vladimir Barinov wrote:
> --- /dev/null
> +++ b/drivers/iio/adc/hi-843x.c
> +ssize_t hi843x_debounce_soft_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + [...]
> +}
>+
>+[...]
>+
> +ssize_t hi843x_threshold_socval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + [...]
> +}
It seems all these *_show and *_store functions should be made static,
right?
> +static IIO_DEVICE_ATTR(debounce_soft, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_show, hi843x_debounce_soft_store, 0);
> +static IIO_DEVICE_ATTR(debounce_soft_delay, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_delay_show, hi843x_debounce_soft_delay_store, 0);
> +static IIO_DEVICE_ATTR(sensing_mode, S_IRUGO | S_IWUSR,
> + hi843x_sensing_mode_show, hi843x_sensing_mode_store, 0);
> +static IIO_DEVICE_ATTR(test_enable, S_IRUGO | S_IWUSR,
> + hi843x_test_enable_show, hi843x_test_enable_store, 0);
> +static IIO_DEVICE_ATTR(test_mode, S_IRUGO | S_IWUSR,
> + hi843x_test_mode_show, hi843x_test_mode_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gohys_show, hi843x_threshold_gohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gocval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gocval_show, hi843x_threshold_gocval_store, 0);
> +static IIO_DEVICE_ATTR(threshold_sohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_sohys_show, hi843x_threshold_sohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_socval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_socval_show, hi843x_threshold_socval_store, 0);
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-03 7:05 ` Paul Bolle
@ 2015-06-03 7:46 ` Vladimir Barinov
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Barinov @ 2015-06-03 7:46 UTC (permalink / raw)
To: Paul Bolle
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, linux-kernel, linux-iio, devicetree
Hi Paul,
On 03.06.2015 10:05, Paul Bolle wrote:
> Just a nit, that I spotted while scanning for other issues.
>
> On Mon, 2015-06-01 at 15:20 +0300, Vladimir Barinov wrote:
>> --- /dev/null
>> +++ b/drivers/iio/adc/hi-843x.c
>> +ssize_t hi843x_debounce_soft_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + [...]
>> +}
>> +
>> +[...]
>> +
>> +ssize_t hi843x_threshold_socval_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + [...]
>> +}
> It seems all these *_show and *_store functions should be made static,
> right?
Right. Thank you for pointing to this.
Regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-01 12:20 ` [PATCH 1/3] iio: adc: hi-843x: " Vladimir Barinov
2015-06-01 12:55 ` Peter Meerwald
2015-06-03 7:05 ` Paul Bolle
@ 2015-06-07 16:11 ` Jonathan Cameron
2015-06-18 19:33 ` Lars-Peter Clausen
2 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2015-06-07 16:11 UTC (permalink / raw)
To: Vladimir Barinov
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, linux-kernel,
linux-iio, devicetree
On 01/06/15 13:20, Vladimir Barinov wrote:
> Add Holt descrete ADC driver for HI-8435/8436/8437 chips
>
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Hmm. The main issue here is one man's discrete ADC is another man's
configurable general purpose input device.
Anyhow, from an IIO point of view what we care about is consistent
userspace interface. A discrete ADC to userspace is definitely a generic
input, be it one with a configurable threshold level and other bells
and whistles.
Right now IIO does not have explicit support for digital I/O channels,
but it's been discussed many times before. We do want some buy in from
the GPIO infrastructure guys though to avoid stepping on peoples toes!
Also I suspect we'd need an IIO to gpio bridge pretty soon as well
as the likelihood of someone using the gpios in an IIO device to save
themselves some pins on their soc is very high.
There are generic gpios on some of the IMUs and similar, but they have
always been ignored in drivers, or just handled by registering them as
a GPIO rather than through the buffered interfaces etc. I suspect if
the core support was in place, there would be interesting in this
functionality.
Lars, you've looked at the demux stuff a lot more recently than I have.
One issue this driver raises is single bit channels. For those I think
we need to support 1 bit packing throughout. How hard do you think it
would be?
(1 bit, 8 bit, 16 bit, 32 bit, 64 bit etc) rather than any more complex
packing.
> ---
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/hi-843x.c | 777 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 790 insertions(+)
> create mode 100644 drivers/iio/adc/hi-843x.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index e36a73e..71b0efc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -164,6 +164,18 @@ config EXYNOS_ADC
> of SoCs for drivers such as the touchscreen and hwmon to use to share
> this resource.
>
> +config HI_843X
> + tristate "Holt Integrated Circuits HI-8435/8436/8437"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + depends on SPI
> + help
> + If you say yes here you get support for Holt Integrated Circuits
> + HI-8435/8436/8437 chip.
> +
> + This driver can also be built as a module. If so, the module will be
> + called hi-843x.
> +
> config LP8788_ADC
> tristate "LP8788 ADC driver"
> depends on MFD_LP8788
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 3930e63..65f54c2 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> +obj-$(CONFIG_HI_843X) += hi-843x.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_MAX1027) += max1027.o
> obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/hi-843x.c b/drivers/iio/adc/hi-843x.c
> new file mode 100644
> index 0000000..ccc46e7
> --- /dev/null
> +++ b/drivers/iio/adc/hi-843x.c
> @@ -0,0 +1,777 @@
> +/*
> + * Holt Integrated Circuits HI-8435/8436/8437 discrete ADC driver
> + *
> + * Copyright (C) 2015 Zodiac Inflight Innovations
> + * Copyright (C) 2015 Cogent Embedded, Inc.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/iio/buffer.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/triggered_buffer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/workqueue.h>
> +
> +#include <linux/interrupt.h>
> +
> +#define DRV_NAME "hi-843x"
> +
> +/* Register offsets for HI-843X */
> +#define HI843X_CTRL_REG 0x02
> +#define HI843X_PSEN_REG 0x04
> +#define HI843X_TMDATA_REG 0x1E
> +#define HI843X_GOCENHYS_REG 0x3A
> +#define HI843X_SOCENHYS_REG 0x3C
> +#define HI843X_SO7_0_REG 0x10
> +#define HI843X_SO15_8_REG 0x12
> +#define HI843X_SO23_16_REG 0x14
> +#define HI843X_SO31_24_REG 0x16
> +#define HI843X_SO31_0_REG 0x78
> +
> +#define HI843X_WRITE_OPCODE 0x00
> +#define HI843X_READ_OPCODE 0x80
> +
> +/* THRESHOLD mask */
> +#define HI843X_THRESHOLD_MAX 0x3f
> +#define HI843X_THRESHOLD_MASK 0xff
> +
> +/* CTRL register bits */
> +#define HI843X_CTRL_TEST 0x01
> +#define HI843X_CTRL_SRST 0x02
> +
> +#define HI843X_DEBOUNCE_SOFT_DELAY_MAX 1000 /* ms */
> +#define HI843X_DEBOUNCE_SOFT_DELAY_DEF 100 /* ms */
> +
> +struct hi843x_priv {
> + struct spi_device *spi;
> + struct mutex lock;
> + struct delayed_work work;
> +
> + int mr_gpio;
> + bool debounce_soft;
> + int debounce_soft_delay;
> + int debounce_soft_val;
> +
> + void *iio_buffer;
> + u8 reg_buffer[4] ____cacheline_aligned;
> +};
> +
> +static int hi843x_readb(struct hi843x_priv *priv, u8 reg, u8 *val)
> +{
> + reg |= HI843X_READ_OPCODE;
> + return spi_write_then_read(priv->spi, ®, 1, val, 1);
> +}
> +
> +static int hi843x_readw(struct hi843x_priv *priv, u8 reg, u16 *val)
> +{
> + int ret;
> +
> + reg |= HI843X_READ_OPCODE;
> + ret = spi_write_then_read(priv->spi, ®, 1, val, 2);
> + *val = swab16p(val);
> +
> + return ret;
> +}
> +
> +static int hi843x_readl(struct hi843x_priv *priv, u8 reg, u32 *val)
> +{
> + int ret;
> +
> + reg |= HI843X_READ_OPCODE;
> + ret = spi_write_then_read(priv->spi, ®, 1, val, 4);
> + *val = swab32p(val);
These all need checking as unlikely to be true for both be and le
platforms.
> +
> + return ret;
> +}
> +
> +static int hi843x_writeb(struct hi843x_priv *priv, u8 reg, u8 val)
> +{
> + priv->reg_buffer[0] = reg | HI843X_WRITE_OPCODE;
> + priv->reg_buffer[1] = val;
> +
> + return spi_write(priv->spi, priv->reg_buffer, 2);
> +}
> +
> +static int hi843x_writew(struct hi843x_priv *priv, u8 reg, u16 val)
> +{
> + priv->reg_buffer[0] = reg | HI843X_WRITE_OPCODE;
> + priv->reg_buffer[1] = (val >> 8) & 0xff;
> + priv->reg_buffer[2] = val & 0xff;
> +
> + return spi_write(priv->spi, priv->reg_buffer, 3);
> +}
> +
> +ssize_t hi843x_debounce_soft_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "%d\n", priv->debounce_soft);
> +}
> +
> +ssize_t hi843x_debounce_soft_delay_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "%d\n", priv->debounce_soft_delay);
> +}
> +
> +ssize_t hi843x_sensing_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u8 reg;
> +
> + ret = hi843x_readb(priv, HI843X_PSEN_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg);
> +}
> +
> +ssize_t hi843x_test_enable_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u8 reg;
> +
> + ret = hi843x_readb(priv, HI843X_CTRL_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg & HI843X_CTRL_TEST);
> +}
> +
> +ssize_t hi843x_test_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u8 reg;
> +
> + ret = hi843x_readb(priv, HI843X_TMDATA_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg);
> +}
> +
> +ssize_t hi843x_debounce_soft_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + priv->debounce_soft = !!val;
> +
> + return len;
> +}
> +
> +ssize_t hi843x_debounce_soft_delay_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_DEBOUNCE_SOFT_DELAY_MAX)
> + val = HI843X_DEBOUNCE_SOFT_DELAY_MAX;
> +
> + priv->debounce_soft_delay = val;
> +
> + return len;
> +}
> +
> +ssize_t hi843x_sensing_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
This definitely looks like magic number territory.
ABI must be easy to understand without the datasheet to hand etc.
> +
> + hi843x_writeb(priv, HI843X_PSEN_REG, val & 0xf);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_test_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + hi843x_writeb(priv, HI843X_CTRL_REG, val ? HI843X_CTRL_TEST : 0);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_test_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + hi843x_writeb(priv, HI843X_TMDATA_REG, val & 0xf);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_gohys_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", (reg >> 8) & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_gocval_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_sohys_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", (reg >> 8) & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_socval_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int ret;
> + u16 reg;
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", reg & HI843X_THRESHOLD_MASK);
> +}
> +
> +ssize_t hi843x_threshold_gohys_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~(HI843X_THRESHOLD_MASK << 8);
> + reg |= (val << 8);
> + hi843x_writew(priv, HI843X_GOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_gocval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_GOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~HI843X_THRESHOLD_MASK;
> + reg |= val;
> + hi843x_writew(priv, HI843X_GOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_sohys_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~(HI843X_THRESHOLD_MASK << 8);
> + reg |= (val << 8);
> + hi843x_writew(priv, HI843X_SOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +ssize_t hi843x_threshold_socval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct hi843x_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + unsigned int val, ret;
> + u16 reg;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val > HI843X_THRESHOLD_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->lock);
> +
> + ret = hi843x_readw(priv, HI843X_SOCENHYS_REG, ®);
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> + reg &= ~HI843X_THRESHOLD_MASK;
> + reg |= val;
> + hi843x_writew(priv, HI843X_SOCENHYS_REG, reg);
> +
> + mutex_unlock(&priv->lock);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(debounce_soft, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_show, hi843x_debounce_soft_store, 0);
> +static IIO_DEVICE_ATTR(debounce_soft_delay, S_IRUGO | S_IWUSR,
> + hi843x_debounce_soft_delay_show, hi843x_debounce_soft_delay_store, 0);
> +static IIO_DEVICE_ATTR(sensing_mode, S_IRUGO | S_IWUSR,
> + hi843x_sensing_mode_show, hi843x_sensing_mode_store, 0);
> +static IIO_DEVICE_ATTR(test_enable, S_IRUGO | S_IWUSR,
> + hi843x_test_enable_show, hi843x_test_enable_store, 0);
> +static IIO_DEVICE_ATTR(test_mode, S_IRUGO | S_IWUSR,
> + hi843x_test_mode_show, hi843x_test_mode_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gohys_show, hi843x_threshold_gohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_gocval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_gocval_show, hi843x_threshold_gocval_store, 0);
> +static IIO_DEVICE_ATTR(threshold_sohys, S_IRUGO | S_IWUSR,
> + hi843x_threshold_sohys_show, hi843x_threshold_sohys_store, 0);
> +static IIO_DEVICE_ATTR(threshold_socval, S_IRUGO | S_IWUSR,
> + hi843x_threshold_socval_show, hi843x_threshold_socval_store, 0);
> +
> +static struct attribute *hi843x_attributes[] = {
> + &iio_dev_attr_debounce_soft.dev_attr.attr,
> + &iio_dev_attr_debounce_soft_delay.dev_attr.attr,
Userspace doesn't care that the debounce is software or hardware so
we want to come up with a generic interface that covers the common
ways debouncing is done.
> + &iio_dev_attr_sensing_mode.dev_attr.attr,
> + &iio_dev_attr_test_enable.dev_attr.attr,
> + &iio_dev_attr_test_mode.dev_attr.attr,
> + &iio_dev_attr_threshold_gohys.dev_attr.attr,
> + &iio_dev_attr_threshold_gocval.dev_attr.attr,
> + &iio_dev_attr_threshold_sohys.dev_attr.attr,
> + &iio_dev_attr_threshold_socval.dev_attr.attr,
Thse all need documentation in Documenation/ABI/testing/sysfs-bus-iio-*
> + NULL,
> +};
> +
> +static struct attribute_group hi843x_attribute_group = {
> + .attrs = hi843x_attributes,
> +};
> +
> +static int hi843x_read_raw(struct iio_dev *idev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + case IIO_CHAN_INFO_RAW:
> + ret = hi843x_readl(priv, HI843X_SO31_0_REG, val);
> + if (ret < 0)
> + return ret;
> +
> + if (mask == IIO_CHAN_INFO_RAW)
> + *val = !!(*val & BIT(channel->channel));
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define HI843X_VOLTAGE_CHANNEL(num) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = num, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_index = num, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 1, \
> + .storagebits = 8, \
> + }, \
> + }
As commented at the start of this review. These are not voltage channels
as userspace understands them (I understand from a hardware point of
view where you are coming from but we have to care about what the software
using them cares about!)
These are digital signals and need to be treated as such.
I wonder if we want to take this oportunity to add 1 bit packing to the
demux etc in the IIO core so we can have tighter packing on these
values. Shouldn't be too hard to do and we probably do want it if we are
going to support these sorts of devices.
Will take a bit of shuffling to pack the relevant channels together if only
a subset are enabled and to notice when no repacking at all is needed.
This will probably first one implementing in the core and pushing out into
the dummy driver to allow for testing of corner cases.
> +
> +static const struct iio_chan_spec hi843x_channels[] = {
> + HI843X_VOLTAGE_CHANNEL(0),
> + HI843X_VOLTAGE_CHANNEL(1),
> + HI843X_VOLTAGE_CHANNEL(2),
> + HI843X_VOLTAGE_CHANNEL(3),
> + HI843X_VOLTAGE_CHANNEL(4),
> + HI843X_VOLTAGE_CHANNEL(5),
> + HI843X_VOLTAGE_CHANNEL(6),
> + HI843X_VOLTAGE_CHANNEL(7),
> + HI843X_VOLTAGE_CHANNEL(8),
> + HI843X_VOLTAGE_CHANNEL(9),
> + HI843X_VOLTAGE_CHANNEL(10),
> + HI843X_VOLTAGE_CHANNEL(11),
> + HI843X_VOLTAGE_CHANNEL(12),
> + HI843X_VOLTAGE_CHANNEL(13),
> + HI843X_VOLTAGE_CHANNEL(14),
> + HI843X_VOLTAGE_CHANNEL(15),
> + HI843X_VOLTAGE_CHANNEL(16),
> + HI843X_VOLTAGE_CHANNEL(17),
> + HI843X_VOLTAGE_CHANNEL(18),
> + HI843X_VOLTAGE_CHANNEL(19),
> + HI843X_VOLTAGE_CHANNEL(20),
> + HI843X_VOLTAGE_CHANNEL(21),
> + HI843X_VOLTAGE_CHANNEL(22),
> + HI843X_VOLTAGE_CHANNEL(23),
> + HI843X_VOLTAGE_CHANNEL(24),
> + HI843X_VOLTAGE_CHANNEL(25),
> + HI843X_VOLTAGE_CHANNEL(26),
> + HI843X_VOLTAGE_CHANNEL(27),
> + HI843X_VOLTAGE_CHANNEL(28),
> + HI843X_VOLTAGE_CHANNEL(29),
> + HI843X_VOLTAGE_CHANNEL(30),
> + HI843X_VOLTAGE_CHANNEL(31),
> + {
> + .type = IIO_ALTVOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .scan_index = 32,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 32,
> + .storagebits = 32,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(33),
> +};
> +
> +static const struct iio_info hi843x_info = {
> + .driver_module = THIS_MODULE,
> + .attrs = &hi843x_attribute_group,
> + .read_raw = hi843x_read_raw,
> +};
> +
> +static void h843x_iio_push_to_buffers(struct iio_dev *idev, int val)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> + u8 *pbuffer = priv->iio_buffer;
> + int i;
> +
> + for_each_set_bit(i, idev->active_scan_mask, idev->masklength) {
> + if (idev->channels[i].type == IIO_ALTVOLTAGE) {
In what way is the is an AC voltage?
> + *(u32 *)pbuffer = val;
> + pbuffer += 4;
> + } else {
> + *pbuffer = !!(val & BIT(i));
> + pbuffer++;
> + }
> + }
> + iio_push_to_buffers_with_timestamp(idev, priv->iio_buffer,
> + iio_get_time_ns());
> +}
> +
This soft debounce is interesting. I wonder if we want to work
this into a more generic form for use on other devices? Maybe something
to look at once we have more drivers where it is useful.
If it was an even sampling thing then we could do it as a generic
in kernel buffer client, but this requires a sport of spurt of readings
(well 2 in this case) which is not something we currently handle.
> +static void hi843x_debounce_soft_work(struct work_struct *work)
> +{
> + struct hi843x_priv *priv = container_of(work, struct hi843x_priv,
> + work.work);
> + struct iio_dev *idev = spi_get_drvdata(priv->spi);
> + int val, ret;
> +
> + ret = hi843x_readl(priv, HI843X_SO31_0_REG, &val);
> + if (ret < 0)
> + return;
> +
> + if (val == priv->debounce_soft_val)
> + h843x_iio_push_to_buffers(idev, val);
> + else
> + dev_warn(&priv->spi->dev, "filtered by soft debounce");
> +}
> +
> +static irqreturn_t hi843x_trigger_handler(int irq, void *private)
> +{
> + struct iio_poll_func *pf = private;
> + struct iio_dev *idev = pf->indio_dev;
> + struct hi843x_priv *priv = iio_priv(idev);
> + int val, ret;
> +
your readl explicitly takes a u32* why do you use an int here?
> + ret = hi843x_readl(priv, HI843X_SO31_0_REG, &val);
> + if (ret < 0)
> + goto err_read;
> +
> + if (priv->debounce_soft) {
> + priv->debounce_soft_val = val;
> + schedule_delayed_work(&priv->work,
> + msecs_to_jiffies(priv->debounce_soft_delay));
> + } else
> + h843x_iio_push_to_buffers(idev, val);
> +
> +err_read:
> + iio_trigger_notify_done(idev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hi843x_buffer_postenable(struct iio_dev *idev)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> +
> + priv->iio_buffer = kmalloc(idev->scan_bytes, GFP_KERNEL);
> + if (!priv->iio_buffer)
> + return -ENOMEM;
> +
> + return iio_triggered_buffer_postenable(idev);
> +}
> +
> +static int hi843x_buffer_predisable(struct iio_dev *idev)
> +{
> + struct hi843x_priv *priv = iio_priv(idev);
> + int ret;
> +
> + ret = iio_triggered_buffer_predisable(idev);
> + if (!ret)
> + kfree(priv->iio_buffer);
Two options here, either use the update_scan_mask hook that is
meant for exactly this sort of thing, or take the view that
it is probably less wasteful to allocate the largest size you will
even need as part of the private structure and
remove the need for these custom ops functions entirely.
I'd go with the second option.
> +
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops hi843x_buffer_setup_ops = {
> + .postenable = &hi843x_buffer_postenable,
> + .predisable = &hi843x_buffer_predisable,
> +};
> +
> +static const struct iio_trigger_ops hi843x_trigger_ops = {
> + .owner = THIS_MODULE,
> +};
> +
> +static void hi843x_parse_dt(struct hi843x_priv *priv)
> +{
> + struct device_node *np = priv->spi->dev.of_node;
> + int ret;
> +
> + ret = of_get_named_gpio(np, "holt,mr-gpio", 0);
> + priv->mr_gpio = ret < 0 ? 0 : ret;
> +
> + if (of_find_property(np, "holt,debounce-soft", NULL))
> + priv->debounce_soft = 1;
> +
> + ret = of_property_read_u32(np, "holt,debounce-soft-delay",
> + &priv->debounce_soft_delay);
> + if (ret)
> + priv->debounce_soft_delay = HI843X_DEBOUNCE_SOFT_DELAY_DEF;
> +}
> +
> +static int hi843x_probe(struct spi_device *spi)
> +{
> + struct iio_dev *idev;
> + struct hi843x_priv *priv;
> + int ret;
> +
> + idev = devm_iio_device_alloc(&spi->dev, sizeof(*priv));
> + if (!idev)
> + return -ENOMEM;
> +
> + priv = iio_priv(idev);
> + priv->spi = spi;
> +
> + if (spi->dev.of_node)
> + hi843x_parse_dt(priv);
> +
> + spi_set_drvdata(spi, idev);
> + mutex_init(&priv->lock);
> + INIT_DELAYED_WORK(&priv->work, hi843x_debounce_soft_work);
> +
> + idev->dev.parent = &spi->dev;
> + idev->name = spi_get_device_id(spi)->name;
> + idev->modes = INDIO_DIRECT_MODE;
> + idev->info = &hi843x_info;mmc35240
> + idev->channels = hi843x_channels;
> + idev->num_channels = ARRAY_SIZE(hi843x_channels);
> +
> + if (priv->mr_gpio) {
> + ret = devm_gpio_request(&spi->dev, priv->mr_gpio, idev->name);
> + if (!ret) {
> + /* chip hardware reset */
> + gpio_direction_output(priv->mr_gpio, 0);
> + udelay(5);
> + gpio_direction_output(priv->mr_gpio, 1);
> + }
> + } else {
> + /* chip software reset */
> + hi843x_writeb(priv, HI843X_CTRL_REG, HI843X_CTRL_SRST);
> + /* get out from reset state */
> + hi843x_writeb(priv, HI843X_CTRL_REG, 0);
> + }
> +
> + ret = iio_triggered_buffer_setup(idev, NULL, hi843x_trigger_handler,
> + &hi843x_buffer_setup_ops);
> + if (ret)
> + return ret;
> +
> + ret = iio_device_register(idev);
> + if (ret < 0) {
> + dev_err(&spi->dev, "unable to register device\n");
> + goto unregister_buffer;
> + }
> +
> + return 0;
> +
> +unregister_buffer:
> + iio_triggered_buffer_cleanup(idev);
> + return ret;
> +}
> +
> +static int hi843x_remove(struct spi_device *spi)
> +{
> + struct iio_dev *idev = spi_get_drvdata(spi);
> + struct hi843x_priv *priv = iio_priv(idev);
> +
> + cancel_delayed_work_sync(&priv->work);
> + iio_device_unregister(idev);
> + iio_triggered_buffer_cleanup(idev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id hi843x_dt_ids[] = {
> + { .compatible = "holt,hi-8435" },
> + { .compatible = "holt,hi-8436" },
> + { .compatible = "holt,hi-8437" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, hi843x_dt_ids);
> +
> +static const struct spi_device_id hi843x_id[] = {
> + { "hi-8435", 0},
> + { "hi-8436", 0},
> + { "hi-8437", 0},
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, hi843x_id);
> +
> +static struct spi_driver hi843x_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .of_match_table = of_match_ptr(hi843x_dt_ids),
> + },
> + .probe = hi843x_probe,
> + .remove = hi843x_remove,
> + .id_table = hi843x_id,
> +};
> +module_spi_driver(hi843x_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Vladimir Barinov");
> +MODULE_DESCRIPTION("HI-8435/8436/8437 discrete ADC");
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-07 16:11 ` Jonathan Cameron
@ 2015-06-18 19:33 ` Lars-Peter Clausen
2015-06-18 21:41 ` Vladimir Barinov
0 siblings, 1 reply; 14+ messages in thread
From: Lars-Peter Clausen @ 2015-06-18 19:33 UTC (permalink / raw)
To: Jonathan Cameron, Vladimir Barinov
Cc: Hartmut Knaack, Peter Meerwald, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, linux-kernel, linux-iio,
devicetree
On 06/07/2015 06:11 PM, Jonathan Cameron wrote:
> On 01/06/15 13:20, Vladimir Barinov wrote:
>> Add Holt descrete ADC driver for HI-8435/8436/8437 chips
>>
>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Hmm. The main issue here is one man's discrete ADC is another man's
> configurable general purpose input device.
The term discrete ADC is a bit ambiguous and I'm not even sure if this is
the right term for this kind of device.
I'd call this a threshold detector. The device seems to have two comparators
for each channel, one for the lower threshold, one for the upper threshold.
If the voltage level goes above the upper threshold a FF is set, if it goes
below the lower threshold the FF is cleared. Both transitions happen
asynchronously as soon has the signal is below/above the threshold. And
while converts a analog signal to digital one this is not what you typically
call a ADC.
>
> Anyhow, from an IIO point of view what we care about is consistent
> userspace interface. A discrete ADC to userspace is definitely a generic
> input, be it one with a configurable threshold level and other bells
> and whistles.
>
> Right now IIO does not have explicit support for digital I/O channels,
> but it's been discussed many times before. We do want some buy in from
> the GPIO infrastructure guys though to avoid stepping on peoples toes!
> Also I suspect we'd need an IIO to gpio bridge pretty soon as well
> as the likelihood of someone using the gpios in an IIO device to save
> themselves some pins on their soc is very high.
>
> There are generic gpios on some of the IMUs and similar, but they have
> always been ignored in drivers, or just handled by registering them as
> a GPIO rather than through the buffered interfaces etc. I suspect if
> the core support was in place, there would be interesting in this
> functionality.
>
> Lars, you've looked at the demux stuff a lot more recently than I have.
> One issue this driver raises is single bit channels. For those I think
> we need to support 1 bit packing throughout. How hard do you think it
> would be?
> (1 bit, 8 bit, 16 bit, 32 bit, 64 bit etc) rather than any more complex
> packing.
Yea, good question. Would definitely need some changes to the core. But it
is also definitely something we should do. The current approach taken by
this driver is rather hackish and works around the limitations of the
framework. This is something that more often than not leads to long term issues.
We'd kind of need a mechanism to say that multiple channels are contained
within the same word. Maybe something like setting the same scan_index for
those channels but with different shift offsets could work?
So say storagebits is 8, realbits is 1 and shift is depending on the
position of the bit in the byte. All channels in the same byte would get the
same scan index.
This could also be used to handle some of the devices which have additional
metadata status information in the otherwise unused bits in the result word.
[...]
>> +#define HI843X_VOLTAGE_CHANNEL(num) \
>> + { \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = num, \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + .scan_index = num, \
>> + .scan_type = { \
>> + .sign = 'u', \
>> + .realbits = 1, \
>> + .storagebits = 8, \
>> + }, \
>> + }
> As commented at the start of this review. These are not voltage channels
> as userspace understands them (I understand from a hardware point of
> view where you are coming from but we have to care about what the software
> using them cares about!)
>
> These are digital signals and need to be treated as such.
Exactly my thoughts as well.
>
> I wonder if we want to take this oportunity to add 1 bit packing to the
> demux etc in the IIO core so we can have tighter packing on these
> values. Shouldn't be too hard to do and we probably do want it if we are
> going to support these sorts of devices.
>
> Will take a bit of shuffling to pack the relevant channels together if only
> a subset are enabled and to notice when no repacking at all is needed.
> This will probably first one implementing in the core and pushing out into
> the dummy driver to allow for testing of corner cases.
Yeah, the bit shuffling gets quite cumbersome and potentially expensive. I
think we should try to avoid it if at least one of the channels in the same
bank is enabled all of them are read. And then let userspace figure out
which bits it wants to use.
But how exactly is the typical expect usage of this device. Like how would a
userspace application use it? Is buffered mode where samples are taken in a
continuous mode something that is really needed?
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-18 19:33 ` Lars-Peter Clausen
@ 2015-06-18 21:41 ` Vladimir Barinov
2015-06-21 14:14 ` Jonathan Cameron
0 siblings, 1 reply; 14+ messages in thread
From: Vladimir Barinov @ 2015-06-18 21:41 UTC (permalink / raw)
To: Lars-Peter Clausen, Jonathan Cameron
Cc: Hartmut Knaack, Peter Meerwald, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, linux-kernel, linux-iio,
devicetree
Hi Lars,
Thank you for the review.
On 18.06.2015 22:33, Lars-Peter Clausen wrote:
> On 06/07/2015 06:11 PM, Jonathan Cameron wrote:
>> On 01/06/15 13:20, Vladimir Barinov wrote:
>>> Add Holt descrete ADC driver for HI-8435/8436/8437 chips
>>>
>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> Hmm. The main issue here is one man's discrete ADC is another man's
>> configurable general purpose input device.
>
> The term discrete ADC is a bit ambiguous and I'm not even sure if this
> is the right term for this kind of device.
>
> I'd call this a threshold detector. The device seems to have two
> comparators for each channel, one for the lower threshold, one for the
> upper threshold. If the voltage level goes above the upper threshold a
> FF is set, if it goes below the lower threshold the FF is cleared.
> Both transitions happen asynchronously as soon has the signal is
> below/above the threshold. And while converts a analog signal to
> digital one this is not what you typically call a ADC.
Should this be a separate/new directory in the drivers/iio/ for such driver?
What the type of iio_chan_spec should I use instead of IIO_VOLTAGE?
>
>>
>> I wonder if we want to take this oportunity to add 1 bit packing to the
>> demux etc in the IIO core so we can have tighter packing on these
>> values. Shouldn't be too hard to do and we probably do want it if we
>> are
>> going to support these sorts of devices.
>>
>> Will take a bit of shuffling to pack the relevant channels together
>> if only
>> a subset are enabled and to notice when no repacking at all is needed.
>> This will probably first one implementing in the core and pushing out
>> into
>> the dummy driver to allow for testing of corner cases.
>
> Yeah, the bit shuffling gets quite cumbersome and potentially
> expensive. I think we should try to avoid it if at least one of the
> channels in the same bank is enabled all of them are read. And then
> let userspace figure out which bits it wants to use.
>
> But how exactly is the typical expect usage of this device. Like how
> would a userspace application use it? Is buffered mode where samples
> are taken in a continuous mode something that is really needed?
I was expecting to use triggered buffer for this device:
1) setup threshold levels via sysfs
2) enable scan elements
3) setup trigger
4) grab data from triggered iio buffer like the
tools/iio/generic_buffer.c does, f.e.
./generic_buffer -n hi-8435 -t irqtrig0 -l 100 -c 1000
Actually I understand that I can just read manually the
/sysfs/.../in_voltageXX_raw (or new/other name) values but using of iio
generic irq trigger would be very good.
About bits shuffling/separating. I do think we can use banks byte
length for one iio channel instead of 1-bit length to avoid such complexity.
Then let user space separate bank's channels by itself.
Regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-18 21:41 ` Vladimir Barinov
@ 2015-06-21 14:14 ` Jonathan Cameron
2015-06-24 9:25 ` Vladimir Barinov
0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2015-06-21 14:14 UTC (permalink / raw)
To: Vladimir Barinov, Lars-Peter Clausen
Cc: Hartmut Knaack, Peter Meerwald, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, linux-kernel, linux-iio,
devicetree
On 18/06/15 22:41, Vladimir Barinov wrote:
> Hi Lars,
>
> Thank you for the review.
>
> On 18.06.2015 22:33, Lars-Peter Clausen wrote:
>> On 06/07/2015 06:11 PM, Jonathan Cameron wrote:
>>> On 01/06/15 13:20, Vladimir Barinov wrote:
>>>> Add Holt descrete ADC driver for HI-8435/8436/8437 chips
>>>>
>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>>> Hmm. The main issue here is one man's discrete ADC is another man's
>>> configurable general purpose input device.
>>
>> The term discrete ADC is a bit ambiguous and I'm not even sure if this is the right term for this kind of device.
>>
>> I'd call this a threshold detector. The device seems to have two
>> comparators for each channel, one for the lower threshold, one for
>> the upper threshold. If the voltage level goes above the upper
>> threshold a FF is set, if it goes below the lower threshold the FF
>> is cleared. Both transitions happen asynchronously as soon has the
>> signal is below/above the threshold. And while converts a analog
>> signal to digital one this is not what you typically call a ADC.
>
> Should this be a separate/new directory in the drivers/iio/ for such driver?
Given we could in theory have comparators for any type of channel it probably doesn't
make sense to have a whole new directory. Perhaps a section in the config.
> What the type of iio_chan_spec should I use instead of IIO_VOLTAGE?
I've been thinking about this. Maybe we should use the same approach we already
use for 'computed' channel values such as root sum squared accelerations that turn
up occasionally and do this as a modifier.
So the channel type would be voltage, but with the modifier comparator (shortening
it would just get confusing). This also maps nicely to devices that offer both
normal adc channels and comparators on the same pin (if that ever happens!)
Hence the attributes etc would be:
in_voltage0_comparator_raw
Do you think that would be clear / flexible enough?
>
>>
>>>
>>> I wonder if we want to take this oportunity to add 1 bit packing to the
>>> demux etc in the IIO core so we can have tighter packing on these
>>> values. Shouldn't be too hard to do and we probably do want it if we are
>>> going to support these sorts of devices.
>>>
>>> Will take a bit of shuffling to pack the relevant channels together if only
>>> a subset are enabled and to notice when no repacking at all is needed.
>>> This will probably first one implementing in the core and pushing out into
>>> the dummy driver to allow for testing of corner cases.
>>
>> Yeah, the bit shuffling gets quite cumbersome and potentially
>> expensive. I think we should try to avoid it if at least one of the
>> channels in the same bank is enabled all of them are read. And then
>> let userspace figure out which bits it wants to use.
>>
>> But how exactly is the typical expect usage of this device. Like
>> how would a userspace application use it? Is buffered mode where
>> samples are taken in a continuous mode something that is really
>> needed?
> I was expecting to use triggered buffer for this device:
> 1) setup threshold levels via sysfs
> 2) enable scan elements
> 3) setup trigger
> 4) grab data from triggered iio buffer like the
> tools/iio/generic_buffer.c does, f.e. ./generic_buffer -n hi-8435 -t
> irqtrig0 -l 100 -c 1000
Good, you are going about it the right way then. Makes sense as this
is what you'd do for similar devices such as a logic analyzer.
>
> Actually I understand that I can just read manually the
> /sysfs/.../in_voltageXX_raw (or new/other name) values but using of
> iio generic irq trigger would be very good.
What is generating the interrupt? Are we looking at a 'dataready'
type interrupt or some other pseudo (or actual) fixed frequency, or
are we talking an interrupt on the state of one of the inputs changing?
If the second case, then an event based approach may make more sense
than using buffers.
>
> About bits shuffling/separating. I do think we can use banks byte
> length for one iio channel instead of 1-bit length to avoid such
> complexity. Then let user space separate bank's channels by itself.
On high channel count devices this could get nasty quickly.
I'd like to explore Lars' suggestion that we use shared scan_indexes
for a set of channels, and different shift / mask values. I don't
think it would need any particularly substantial changes in kernel
and obviously only usespace code using this new type of device would
need to know about it.
I'd relax the suggestion he made to have it always sample all channels
in a given scan index, to say that it can if it makes sense. We won't
bother masking them out, but if it is quicker for the device to not read
those that aren't enabled, then it is up to the device whether it does.
The issue is that if we let a device in now with the 8bits per bit
interface, we kind of commit to at least having userspace support that
long term which isn't nice (though not too bad I suppose as it is just
the least efficient case of what we are talking about doing anyway).
Jonathan
>
> Regards,
> Vladimir
>
> --
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] iio: adc: hi-843x: Holt HI-8435/8436/8437 descrete ADC
2015-06-21 14:14 ` Jonathan Cameron
@ 2015-06-24 9:25 ` Vladimir Barinov
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Barinov @ 2015-06-24 9:25 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen
Cc: Hartmut Knaack, Peter Meerwald, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, linux-kernel, linux-iio,
devicetree
On 21.06.2015 17:14, Jonathan Cameron wrote:
>>> I'd call this a threshold detector. The device seems to have two
>>> comparators for each channel, one for the lower threshold, one for
>>> the upper threshold. If the voltage level goes above the upper
>>> threshold a FF is set, if it goes below the lower threshold the FF
>>> is cleared. Both transitions happen asynchronously as soon has the
>>> signal is below/above the threshold. And while converts a analog
>>> signal to digital one this is not what you typically call a ADC.
>> Should this be a separate/new directory in the drivers/iio/ for such driver?
> Given we could in theory have comparators for any type of channel it probably doesn't
> make sense to have a whole new directory. Perhaps a section in the config.
>> What the type of iio_chan_spec should I use instead of IIO_VOLTAGE?
> I've been thinking about this. Maybe we should use the same approach we already
> use for 'computed' channel values such as root sum squared accelerations that turn
> up occasionally and do this as a modifier.
>
> So the channel type would be voltage, but with the modifier comparator (shortening
> it would just get confusing). This also maps nicely to devices that offer both
> normal adc channels and comparators on the same pin (if that ever happens!)
>
> Hence the attributes etc would be:
> in_voltage0_comparator_raw
>
> Do you think that would be clear / flexible enough?
yes
>>>> I wonder if we want to take this oportunity to add 1 bit packing to the
>>>> demux etc in the IIO core so we can have tighter packing on these
>>>> values. Shouldn't be too hard to do and we probably do want it if we are
>>>> going to support these sorts of devices.
>>>>
>>>> Will take a bit of shuffling to pack the relevant channels together if only
>>>> a subset are enabled and to notice when no repacking at all is needed.
>>>> This will probably first one implementing in the core and pushing out into
>>>> the dummy driver to allow for testing of corner cases.
>>> Yeah, the bit shuffling gets quite cumbersome and potentially
>>> expensive. I think we should try to avoid it if at least one of the
>>> channels in the same bank is enabled all of them are read. And then
>>> let userspace figure out which bits it wants to use.
>>>
>>> But how exactly is the typical expect usage of this device. Like
>>> how would a userspace application use it? Is buffered mode where
>>> samples are taken in a continuous mode something that is really
>>> needed?
>> I was expecting to use triggered buffer for this device:
>> 1) setup threshold levels via sysfs
>> 2) enable scan elements
>> 3) setup trigger
>> 4) grab data from triggered iio buffer like the
>> tools/iio/generic_buffer.c does, f.e. ./generic_buffer -n hi-8435 -t
>> irqtrig0 -l 100 -c 1000
> Good, you are going about it the right way then. Makes sense as this
> is what you'd do for similar devices such as a logic analyzer.
>> Actually I understand that I can just read manually the
>> /sysfs/.../in_voltageXX_raw (or new/other name) values but using of
>> iio generic irq trigger would be very good.
> What is generating the interrupt? Are we looking at a 'dataready'
> type interrupt or some other pseudo (or actual) fixed frequency, or
> are we talking an interrupt on the state of one of the inputs changing?
>
> If the second case, then an event based approach may make more sense
> than using buffers.
Actually it is a interrupt that should occur on the state change of one
of the inputs.
But hi8435 chip does not have it's own dedicated/hardware interrupt line
for this purposes, so it
requires any kind of polling (hardware from pwm/gpio, or s/w polling
inside the driver)
Probably I'm not right by trying to use "iio irq trigger" from external
hardware line (like gpio/pwm)
and I have to implement polling inside driver.
Event based approach is good enough.
>> About bits shuffling/separating. I do think we can use banks byte
>> length for one iio channel instead of 1-bit length to avoid such
>> complexity. Then let user space separate bank's channels by itself.
> On high channel count devices this could get nasty quickly.
>
> I'd like to explore Lars' suggestion that we use shared scan_indexes
> for a set of channels, and different shift / mask values. I don't
> think it would need any particularly substantial changes in kernel
> and obviously only usespace code using this new type of device would
> need to know about it.
>
> I'd relax the suggestion he made to have it always sample all channels
> in a given scan index, to say that it can if it makes sense. We won't
> bother masking them out, but if it is quicker for the device to not read
> those that aren't enabled, then it is up to the device whether it does.
>
> The issue is that if we let a device in now with the 8bits per bit
> interface, we kind of commit to at least having userspace support that
> long term which isn't nice (though not too bad I suppose as it is just
> the least efficient case of what we are talking about doing anyway).
>
Ok, I see
Regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread