* RFC Introducing Heart Monitors for IIO
@ 2014-10-14 19:13 Dan Murphy
2014-10-14 19:13 ` [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors Dan Murphy
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Dan Murphy @ 2014-10-14 19:13 UTC (permalink / raw)
To: linux-iio; +Cc: jic23
All
I am introducing a new sensor category for IIO. This is for
heart rate monitors.
Putting this out as a RFC to see if anyone else is developing the
same and we can build upon each others patches.
I have another sensor to follow up on this series but don't want
to start that driver until I understand if there is anyone else
doing the same.
The first patch introduces the heart rate monitoring into the IIO
sub-systems
The second patch introduces the DT documentation heart_monitors directory
along with the documentation for the TI AFE4403 part.
The final patch introduces the heart_monitors directory along with the source
for the TI AFE4403 device
Looking for feedback.
Thanks in advance
Dan
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors
2014-10-14 19:13 RFC Introducing Heart Monitors for IIO Dan Murphy
@ 2014-10-14 19:13 ` Dan Murphy
2014-10-15 7:23 ` Daniel Baluta
2014-10-15 19:35 ` Jonathan Cameron
2014-10-14 19:13 ` [RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation Dan Murphy
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: Dan Murphy @ 2014-10-14 19:13 UTC (permalink / raw)
To: linux-iio; +Cc: jic23, Dan Murphy
Add a type for heart rate monitors
Add the modifier name in beats per minute.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
drivers/iio/industrialio-core.c | 2 ++
.../staging/iio/Documentation/iio_event_monitor.c | 2 ++
include/linux/iio/types.h | 4 +++-
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index af3e76d..d87ca35 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -70,6 +70,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_CCT] = "cct",
[IIO_PRESSURE] = "pressure",
[IIO_HUMIDITYRELATIVE] = "humidityrelative",
+ [IIO_HEARTRATE] = "heartrate",
};
static const char * const iio_modifier_names[] = {
@@ -91,6 +92,7 @@ static const char * const iio_modifier_names[] = {
[IIO_MOD_NORTH_TRUE] = "from_north_true",
[IIO_MOD_NORTH_MAGN_TILT_COMP] = "from_north_magnetic_tilt_comp",
[IIO_MOD_NORTH_TRUE_TILT_COMP] = "from_north_true_tilt_comp",
+ [IIO_MOD_HEART_BPM] = "heartrate_beats_per_minute",
};
/* relies on pairs of these shared then separate */
diff --git a/drivers/staging/iio/Documentation/iio_event_monitor.c b/drivers/staging/iio/Documentation/iio_event_monitor.c
index 569d6f8..b06dab7 100644
--- a/drivers/staging/iio/Documentation/iio_event_monitor.c
+++ b/drivers/staging/iio/Documentation/iio_event_monitor.c
@@ -49,6 +49,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_CCT] = "cct",
[IIO_PRESSURE] = "pressure",
[IIO_HUMIDITYRELATIVE] = "humidityrelative",
+ [IIO_HEARTRATE] = "heartrate"
};
static const char * const iio_ev_type_text[] = {
@@ -108,6 +109,7 @@ static bool event_is_known(struct iio_event_data *event)
case IIO_CCT:
case IIO_PRESSURE:
case IIO_HUMIDITYRELATIVE:
+ case IIO_MOD_HEART_BPM:
break;
default:
return false;
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 4a2af8a..50b1456 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -30,6 +30,7 @@ enum iio_chan_type {
IIO_CCT,
IIO_PRESSURE,
IIO_HUMIDITYRELATIVE,
+ IIO_HEARTRATE
};
enum iio_modifier {
@@ -59,7 +60,8 @@ enum iio_modifier {
IIO_MOD_NORTH_MAGN,
IIO_MOD_NORTH_TRUE,
IIO_MOD_NORTH_MAGN_TILT_COMP,
- IIO_MOD_NORTH_TRUE_TILT_COMP
+ IIO_MOD_NORTH_TRUE_TILT_COMP,
+ IIO_MOD_HEART_BPM
};
enum iio_event_type {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation
2014-10-14 19:13 RFC Introducing Heart Monitors for IIO Dan Murphy
2014-10-14 19:13 ` [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors Dan Murphy
@ 2014-10-14 19:13 ` Dan Murphy
2014-10-15 9:52 ` Peter Meerwald
2014-10-14 19:13 ` [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor Dan Murphy
2014-10-15 7:37 ` RFC Introducing Heart Monitors for IIO Daniel Baluta
3 siblings, 1 reply; 15+ messages in thread
From: Dan Murphy @ 2014-10-14 19:13 UTC (permalink / raw)
To: linux-iio; +Cc: jic23, Dan Murphy
Add the TI afe4403 heart monitor device tree
binding documentation. heart_monitors directory
created under iio.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
.../bindings/iio/heart_monitors/ti_afe4403.txt | 23 ++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt
diff --git a/Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt b/Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt
new file mode 100644
index 0000000..d612dd6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt
@@ -0,0 +1,23 @@
+* Texas Instruments - AFE4403 Heart rate and Pulse Oximeter
+
+The device consists of a low-noise receiver channel
+with an integrated analog-to-digital converter (ADC),
+an LED transmit section, and diagnostics for sensor and LED fault detection.
+
+Required properties:
+ - compatible: Must contain "ti,afe4403".
+ - ste-gpio: GPIO for the spi control line
+ - data-ready-gpio: GPIO interrupt when the afe4403 has data
+ - led-supply: Chip supply to the device
+ -
+Optional properties:
+ - reset-gpio: GPIO used to reset the device via HW
+
+Example:
+
+&heart_rate {
+ compatible = "ti,afe4403";
+ ste-gpio = <&gpio1 29 GPIO_ACTIVE_HIGH>;
+ data-ready-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+ led-supply = <&vbat>;
+};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor
2014-10-14 19:13 RFC Introducing Heart Monitors for IIO Dan Murphy
2014-10-14 19:13 ` [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors Dan Murphy
2014-10-14 19:13 ` [RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation Dan Murphy
@ 2014-10-14 19:13 ` Dan Murphy
2014-10-15 7:35 ` Daniel Baluta
2014-10-15 9:52 ` Peter Meerwald
2014-10-15 7:37 ` RFC Introducing Heart Monitors for IIO Daniel Baluta
3 siblings, 2 replies; 15+ messages in thread
From: Dan Murphy @ 2014-10-14 19:13 UTC (permalink / raw)
To: linux-iio; +Cc: jic23, Dan Murphy
Add the TI afe4403 heart rate monitor.
This device detects reflected LED
wave length fluctuations and presents an ADC
value to the user space to be converted to a
heart rate.
Data sheet located here:
http://www.ti.com/product/AFE4403/datasheet
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
drivers/iio/Kconfig | 1 +
drivers/iio/Makefile | 1 +
drivers/iio/heart_monitors/Kconfig | 16 +
drivers/iio/heart_monitors/Makefile | 6 +
drivers/iio/heart_monitors/afe4403.c | 610 ++++++++++++++++++++++++++++++++++
5 files changed, 634 insertions(+)
create mode 100644 drivers/iio/heart_monitors/Kconfig
create mode 100644 drivers/iio/heart_monitors/Makefile
create mode 100644 drivers/iio/heart_monitors/afe4403.c
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 345395e..e3a7945 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -66,6 +66,7 @@ source "drivers/iio/common/Kconfig"
source "drivers/iio/dac/Kconfig"
source "drivers/iio/frequency/Kconfig"
source "drivers/iio/gyro/Kconfig"
+source "drivers/iio/heart_monitors/Kconfig"
source "drivers/iio/humidity/Kconfig"
source "drivers/iio/imu/Kconfig"
source "drivers/iio/light/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 698afc2..46c0960 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -18,6 +18,7 @@ obj-y += common/
obj-y += dac/
obj-y += gyro/
obj-y += frequency/
+obj-y += heart_monitors/
obj-y += humidity/
obj-y += imu/
obj-y += light/
diff --git a/drivers/iio/heart_monitors/Kconfig b/drivers/iio/heart_monitors/Kconfig
new file mode 100644
index 0000000..7060694
--- /dev/null
+++ b/drivers/iio/heart_monitors/Kconfig
@@ -0,0 +1,16 @@
+#
+# Heart Rate Monitor drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Heart Rate Monitors"
+
+config AFE4403
+ tristate "TI AFE4403 Heart Rate Monitor"
+ depends on SPI
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ select SYSFS
+ help
+
+endmenu
diff --git a/drivers/iio/heart_monitors/Makefile b/drivers/iio/heart_monitors/Makefile
new file mode 100644
index 0000000..77cc5c5
--- /dev/null
+++ b/drivers/iio/heart_monitors/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for IIO Heart Rate Monitor drivers
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AFE4403) += afe4403.o
diff --git a/drivers/iio/heart_monitors/afe4403.c b/drivers/iio/heart_monitors/afe4403.c
new file mode 100644
index 0000000..d285769
--- /dev/null
+++ b/drivers/iio/heart_monitors/afe4403.c
@@ -0,0 +1,610 @@
+/*
+ * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
+ *
+ * Author: Dan Murphy <dmurphy@ti.com>
+ *
+ * Copyright: (C) 2014 Texas Instruments, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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/device.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/driver.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/events.h>
+#include <linux/iio/kfifo_buf.h>
+
+#define AFE4403_CONTROL0 0x00
+#define AFE4403_LED2STC 0x01
+#define AFE4403_LED2ENDC 0x02
+#define AFE4403_LED2LEDSTC 0x03
+#define AFE4403_LED2LEDENDC 0x04
+#define AFE4403_ALED2STC 0x05
+#define AFE4403_ALED2ENDC 0x06
+#define AFE4403_LED1STC 0x07
+#define AFE4403_LED1ENDC 0x08
+#define AFE4403_LED1LEDSTC 0x09
+#define AFE4403_LED1LEDENDC 0x0a
+#define AFE4403_ALED1STC 0x0b
+#define AFE4403_ALED1ENDC 0x0c
+#define AFE4403_LED2CONVST 0x0d
+#define AFE4403_LED2CONVEND 0x0e
+#define AFE4403_ALED2CONVST 0x0f
+#define AFE4403_ALED2CONVEND 0x10
+#define AFE4403_LED1CONVST 0x11
+#define AFE4403_LED1CONVEND 0x12
+#define AFE4403_ALED1CONVST 0x13
+#define AFE4403_ALED1CONVEND 0x14
+#define AFE4403_ADCRSTSTCT0 0x15
+#define AFE4403_ADCRSTENDCT0 0x16
+#define AFE4403_ADCRSTSTCT1 0x17
+#define AFE4403_ADCRSTENDCT1 0x18
+#define AFE4403_ADCRSTSTCT2 0x19
+#define AFE4403_ADCRSTENDCT2 0x1a
+#define AFE4403_ADCRSTSTCT3 0x1b
+#define AFE4403_ADCRSTENDCT3 0x1c
+#define AFE4403_PRPCOUNT 0x1d
+#define AFE4403_CONTROL1 0x1e
+#define AFE4403_SPARE1 0x1f
+#define AFE4403_TIAGAIN 0x20
+#define AFE4403_TIA_AMB_GAIN 0x21
+#define AFE4403_LEDCNTRL 0x22
+#define AFE4403_CONTROL2 0x23
+#define AFE4403_SPARE2 0x24
+#define AFE4403_SPARE3 0x25
+#define AFE4403_SPARE4 0x26
+#define AFE4403_ALARM 0x29
+#define AFE4403_LED2VAL 0x2A
+#define AFE4403_ALED2VAL 0x2B
+#define AFE4403_LED1VAL 0x2C
+#define AFE4403_ALED1VAL 0x2D
+#define AFE4403_LED2_ALED2VAL 0x2E
+#define AFE4403_LED1_ALED1VAL 0x2F
+#define AFE4403_DIAG 0x30
+#define AFE4403_CONTROL3 0x31
+#define AFE4403_PDNCYCLESTC 0x32
+#define AFE4403_PDNCYCLEENDC 0x33
+
+#define AFE4403_SPI_READ (1 << 0)
+#define AFE4403_SW_RESET (1 << 3)
+#define AFE4403_PWR_DWN (1 << 0)
+
+/**
+ * struct afe4403_data -
+ * @work - Work item used to off load the enable/disable of the vibration
+ * @indio_dev -
+ * @map -
+ * @spi -
+ * @regulator - Pointer to the regulator for the IC
+ * @ste_gpio -
+ * @data_gpio -
+ * @reset_gpio -
+ * @data_buffer -
+ * @irq - AFE4403 interrupt number
+**/
+struct afe4403_data {
+ struct work_struct work;
+ struct iio_dev *indio_dev;
+ struct iio_map *map;
+ struct spi_device *spi;
+ struct regulator *regulator;
+ struct gpio_desc *ste_gpio;
+ struct gpio_desc *data_gpio;
+ struct gpio_desc *reset_gpio;
+ int data_buffer[5];
+ int irq;
+};
+
+enum afe4403_reg_id {
+ LED1VAL,
+ ALED1VAL,
+ LED2VAL,
+ ALED2VAL,
+ DIAG,
+ TIAGAIN,
+ TIA_AMB_GAIN,
+ LEDCNTRL,
+ CONTROL3,
+};
+
+static const struct iio_event_spec afe4403_events[] = {
+ {
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+#define AFE4403_COMMON_CHAN(index, name) \
+ { \
+ .type = IIO_HEARTRATE, \
+ .indexed = 1, \
+ .channel = index, \
+ .datasheet_name = name, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+ .scan_index = index + 1, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 24, \
+ .storagebits = 24, \
+ }, \
+ .event_spec = afe4403_events, \
+ .num_event_specs = ARRAY_SIZE(afe4403_events), \
+ }
+
+static const struct iio_chan_spec afe4403_channels[] = {
+ /* Read only values from the IC */
+ [LED1VAL] = AFE4403_COMMON_CHAN(0, "LED1VAL"),
+ [ALED1VAL] = AFE4403_COMMON_CHAN(1, "ALED1VAL"),
+ [LED2VAL] = AFE4403_COMMON_CHAN(2, "LED2VAL"),
+ [ALED1VAL] = AFE4403_COMMON_CHAN(3, "ALED2VAL"),
+ [DIAG] = AFE4403_COMMON_CHAN(4, "DIAG"),
+
+ /* Required writing for calibration */
+ [TIAGAIN] = AFE4403_COMMON_CHAN(5, "TIAGAIN"),
+ [TIA_AMB_GAIN] = AFE4403_COMMON_CHAN(6, "TIA_AMB_GAIN"),
+ [LEDCNTRL] = AFE4403_COMMON_CHAN(7, "LEDCNTRL"),
+ [CONTROL3] = AFE4403_COMMON_CHAN(8, "CONTROL3"),
+};
+
+static struct iio_map afe4403_default_iio_maps[] = {
+ {
+ .consumer_dev_name = "afe4403_heartrate",
+ .consumer_channel = "afe4403_led1value",
+ .adc_channel_label = "LED1VAL",
+ },
+ {
+ .consumer_dev_name = "afe4403_heartrate",
+ .consumer_channel = "afe4403_aled1value",
+ .adc_channel_label = "ALED1VAL",
+ },
+ {
+ .consumer_dev_name = "afe4403_heartrate",
+ .consumer_channel = "afe4403_led2value",
+ .adc_channel_label = "LED2VAL",
+ },
+ {
+ .consumer_dev_name = "afe4403_heartrate",
+ .consumer_channel = "afe4403_aled2value",
+ .adc_channel_label = "ALED2VAL",
+ },
+ {
+ .consumer_dev_name = "afe4403_heartrate",
+ .consumer_channel = "afe4403_diagnostics",
+ .adc_channel_label = "DIAG",
+ },
+};
+
+static void afe4403_toggle_ste(struct afe4403_data *afe4403)
+{
+ gpiod_set_value(afe4403->ste_gpio, 1);
+ udelay(800);
+ gpiod_set_value(afe4403->ste_gpio, 0);
+}
+
+static int afe4403_read(struct afe4403_data *afe4403, u8 reg,
+ unsigned int *data)
+{
+ int ret;
+
+ ret = spi_write_then_read(afe4403->spi, (u8 *)®, 1, (u8 *)data, 3);
+ if (ret)
+ return -EIO;
+
+ afe4403_toggle_ste(afe4403);
+
+ return ret;
+};
+
+static int afe4403_write(struct afe4403_data *afe4403, u8 reg,
+ unsigned int data)
+{
+ int ret;
+ u8 tx[4] = {0x0, 0x0, 0x0, 0x0};
+
+ /* Enable write to the device */
+
+ tx[0] = AFE4403_CONTROL0;
+ tx[3] = 0x0;
+ ret = spi_write(afe4403->spi, (u8 *)tx, 4);
+ if (ret)
+ return -EIO;
+
+ afe4403_toggle_ste(afe4403);
+
+ tx[0] = reg;
+ tx[1] = (data & 0x0f0000) >> 16;
+ tx[2] = (data & 0x00ff00) >> 8;
+ tx[3] = data & 0xff;
+ ret = spi_write(afe4403->spi, (u8 *)tx, 4);
+ if (ret)
+ return -EIO;
+
+ afe4403_toggle_ste(afe4403);
+
+ /* Re-Enable reading from the device */
+ tx[0] = AFE4403_CONTROL0;
+ tx[1] = 0x0;
+ tx[2] = 0x0;
+ tx[3] = AFE4403_SPI_READ;
+ ret = spi_write(afe4403->spi, (u8 *)tx, 4);
+ if (ret)
+ return -EIO;
+
+ afe4403_toggle_ste(afe4403);
+
+ return ret;
+};
+
+static int afe4403_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct afe4403_data *data = iio_priv(indio_dev);
+ u8 reg;
+
+ if (chan->channel > ARRAY_SIZE(afe4403_channels))
+ return -EINVAL;
+
+ if (chan->channel == 5)
+ reg = AFE4403_TIAGAIN;
+ else if (chan->channel == 6)
+ reg = AFE4403_TIA_AMB_GAIN;
+ else if (chan->channel == 7)
+ reg = AFE4403_LEDCNTRL;
+ else if (chan->channel == 8)
+ reg = AFE4403_CONTROL2;
+ else
+ return -EINVAL;
+
+ return afe4403_write(data, reg, val);
+}
+
+static int afe4403_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct afe4403_data *data = iio_priv(indio_dev);
+
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
+ if (chan->channel > ARRAY_SIZE(afe4403_channels))
+ return -EINVAL;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ *val = data->data_buffer[chan->channel];
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int afe4403_write_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ int state)
+{
+ struct afe4403_data *afe4403 = iio_priv(indio_dev);
+ int ret;
+ int control_val;
+
+ ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
+ if (ret)
+ return ret;
+
+ printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN);
+ if (state)
+ control_val &= ~AFE4403_PWR_DWN;
+ else
+ control_val |= AFE4403_PWR_DWN;
+
+ printk("***after control 0x%X\n", control_val);
+
+ ret = afe4403_write(afe4403, AFE4403_CONTROL2, control_val);
+ if (ret)
+ return ret;
+
+ ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
+ if (ret)
+ return ret;
+
+ printk("***control 0x%X\n", control_val);
+
+ return 0;
+}
+
+static const struct iio_info afe4403_iio_info = {
+ .read_raw = &afe4403_read_raw,
+ .write_raw = &afe4403_write_raw,
+ .write_event_config = &afe4403_write_event,
+ .driver_module = THIS_MODULE,
+};
+
+static irqreturn_t afe4403_event_handler(int irq, void *data)
+{
+ struct iio_dev *indio_dev = data;
+ struct afe4403_data *afe4403 = iio_priv(indio_dev);
+ int ret;
+
+ ret = afe4403_read(afe4403, AFE4403_LED1VAL, &afe4403->data_buffer[0]);
+ if (ret)
+ goto done;
+
+ ret = afe4403_read(afe4403, AFE4403_ALED1VAL, &afe4403->data_buffer[1]);
+ if (ret)
+ goto done;
+
+ ret = afe4403_read(afe4403, AFE4403_LED2VAL, &afe4403->data_buffer[2]);
+ if (ret)
+ goto done;
+
+ ret = afe4403_read(afe4403, AFE4403_ALED2VAL, &afe4403->data_buffer[3]);
+ if (ret)
+ goto done;
+
+ ret = afe4403_read(afe4403, AFE4403_DIAG, &afe4403->data_buffer[4]);
+ if (ret)
+ goto done;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &afe4403->data_buffer,
+ iio_get_time_ns());
+done:
+ return IRQ_HANDLED;
+}
+
+struct afe4403_reg {
+ uint8_t reg;
+ u32 value;
+} afe4403_init_regs[] = {
+ { AFE4403_LED2STC, 0x0820},
+ { AFE4403_LED2ENDC, 0x0f9e },
+ { AFE4403_LED2LEDSTC, 0x07d0 },
+ { AFE4403_LED2LEDENDC, 0x0f9f },
+ { AFE4403_ALED2STC, 0x0050 },
+ { AFE4403_ALED2ENDC, 0x07ce },
+ { AFE4403_LED1STC, 0xc350 },
+ { AFE4403_LED1ENDC, 0xc350 },
+ { AFE4403_LED1LEDSTC, 0xc350 },
+ { AFE4403_LED1LEDENDC, 0xc350 },
+ { AFE4403_ALED1STC, 0x0ff0 },
+ { AFE4403_ALED1ENDC, 0x176e },
+ { AFE4403_LED2CONVST, 0x1775 },
+ { AFE4403_LED2CONVEND, 0x1f3f },
+ { AFE4403_ALED2CONVST, 0x1f45 },
+ { AFE4403_ALED2CONVEND, 0x270f },
+ { AFE4403_LED1CONVST, 0x2715 },
+ { AFE4403_LED1CONVEND, 0x2edf },
+ { AFE4403_ALED1CONVST, 0x2ee5 },
+ { AFE4403_ALED1CONVEND, 0x36af },
+ { AFE4403_ADCRSTSTCT0, 0x1770 },
+ { AFE4403_ADCRSTENDCT0, 0x1774 },
+ { AFE4403_ADCRSTSTCT1, 0x1f40 },
+ { AFE4403_ADCRSTENDCT1, 0x1f44 },
+ { AFE4403_ADCRSTSTCT2, 0x2710 },
+ { AFE4403_ADCRSTENDCT2, 0x2714 },
+ { AFE4403_ADCRSTSTCT3, 0x2ee0 },
+ { AFE4403_ADCRSTENDCT3, 0x2ee4 },
+ { AFE4403_PRPCOUNT, 0x09c3f },
+ { AFE4403_CONTROL1, 0x0107 },
+ { AFE4403_TIAGAIN, 0x8006 },
+ { AFE4403_TIA_AMB_GAIN, 0x06 },
+ { AFE4403_LEDCNTRL, 0x11414 },
+ { AFE4403_CONTROL2, 0x20000 },
+};
+
+static int afe4403_init(struct afe4403_data *afe4403)
+{
+ int ret, i, reg_count;
+
+ /* Hard reset the device */
+ if (afe4403->reset_gpio) {
+ gpiod_set_value(afe4403->reset_gpio, 0);
+ udelay(800);
+ gpiod_set_value(afe4403->reset_gpio, 1);
+ } else {
+ /* Soft reset the device */
+ ret = afe4403_write(afe4403, AFE4403_CONTROL0, AFE4403_SW_RESET);
+ if (ret)
+ return ret;
+ }
+
+ udelay(800);
+
+ reg_count = sizeof(afe4403_init_regs) / sizeof(afe4403_init_regs[0]);
+ for (i = 0; i < reg_count; i++) {
+ ret = afe4403_write(afe4403, afe4403_init_regs[i].reg,
+ afe4403_init_regs[i].value);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+};
+
+static int afe4403_iio_buffered_hardware_setup(struct iio_dev *indio_dev)
+{
+ struct iio_buffer *buffer;
+ int ret;
+
+ buffer = iio_kfifo_allocate(indio_dev);
+ if (!buffer)
+ return -ENOMEM;
+
+ iio_device_attach_buffer(indio_dev, buffer);
+
+ ret = iio_buffer_register(indio_dev,
+ indio_dev->channels,
+ indio_dev->num_channels);
+ if (ret)
+ goto error_kfifo_free;
+
+ return 0;
+
+error_kfifo_free:
+ iio_kfifo_free(indio_dev->buffer);
+ return ret;
+}
+
+static int afe4403_spi_probe(struct spi_device *spi)
+{
+ struct afe4403_data *afe4403;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*afe4403));
+ if (indio_dev == NULL) {
+ dev_err(&spi->dev, "Failed to allocate iio device\n");
+ return -ENOMEM;
+ }
+
+ spi_set_drvdata(spi, indio_dev);
+
+ afe4403 = iio_priv(indio_dev);
+ afe4403->spi = spi;
+
+ ret = iio_map_array_register(indio_dev, afe4403_default_iio_maps);
+ if (ret) {
+ dev_err(&indio_dev->dev, "iio map err: %d\n", ret);
+ return ret;
+ }
+ afe4403->map = afe4403_default_iio_maps;
+
+ indio_dev->dev.parent = &spi->dev;
+ indio_dev->name = "afe4403";
+ indio_dev->info = &afe4403_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;
+ indio_dev->channels = afe4403_channels;
+ indio_dev->num_channels = ARRAY_SIZE(afe4403_channels);
+
+ afe4403->ste_gpio = devm_gpiod_get(&spi->dev, "ste");
+ if (IS_ERR(afe4403->ste_gpio)) {
+ ret = PTR_ERR(afe4403->ste_gpio);
+ if (ret != -ENOENT && ret != -ENOSYS) {
+ dev_err(&spi->dev, "Failed to allocate spi gpio\n");
+ return ret;
+ }
+ afe4403->ste_gpio = NULL;
+ } else {
+ gpiod_direction_output(afe4403->ste_gpio, 0);
+ }
+
+ afe4403->data_gpio = devm_gpiod_get(&spi->dev, "data-ready");
+ if (IS_ERR(afe4403->data_gpio)) {
+ ret = PTR_ERR(afe4403->data_gpio);
+ if (ret != -ENOENT && ret != -ENOSYS) {
+ dev_err(&spi->dev, "Failed to allocate data_ready gpio\n");
+ return ret;
+ }
+ afe4403->data_gpio = NULL;
+ } else {
+ gpiod_direction_input(afe4403->data_gpio);
+ afe4403->irq = gpiod_to_irq(afe4403->data_gpio);
+ }
+
+ afe4403->reset_gpio = devm_gpiod_get(&spi->dev, "reset");
+ if (IS_ERR(afe4403->reset_gpio)) {
+ ret = PTR_ERR(afe4403->reset_gpio);
+ if (ret != -ENOENT && ret != -ENOSYS) {
+ dev_err(&spi->dev, "Failed to allocate reset gpio\n");
+ return ret;
+ }
+ afe4403->reset_gpio = NULL;
+ } else {
+ gpiod_direction_output(afe4403->reset_gpio, 1);
+ }
+
+ ret = afe4403_iio_buffered_hardware_setup(indio_dev);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to allocate iio buffers %i\n", ret);
+ return ret;
+ }
+
+ ret = devm_request_threaded_irq(&spi->dev, afe4403->irq,
+ NULL,
+ afe4403_event_handler,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ "afe4403 int",
+ indio_dev);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to allocate thread %i\n", ret);
+ return ret;
+ }
+
+ ret = devm_iio_device_register(&spi->dev, indio_dev);
+ if (ret < 0)
+ return ret;
+
+ ret = afe4403_init(afe4403);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to init device\n");
+ return ret;
+ }
+
+ return 0;
+
+}
+
+static int afe4403_spi_remove(struct spi_device *spi)
+{
+ struct afe4403_data *afe4403 = dev_get_drvdata(&spi->dev);
+
+ iio_kfifo_free(afe4403->indio_dev->buffer);
+ iio_buffer_unregister(afe4403->indio_dev);
+
+ if (!IS_ERR(afe4403->regulator))
+ regulator_disable(afe4403->regulator);
+
+ return 0;
+}
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id afe4403_of_match[] = {
+ { .compatible = "ti,afe4403", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, afe4403_of_match);
+#endif
+
+static struct spi_driver afe4403_spi_driver = {
+ .driver = {
+ .name = "afe4403",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(afe4403_of_match),
+ },
+ .probe = afe4403_spi_probe,
+ .remove = afe4403_spi_remove,
+};
+
+static int __init afe4403_spi_init(void)
+{
+ return spi_register_driver(&afe4403_spi_driver);
+}
+module_init(afe4403_spi_init);
+
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_DESCRIPTION("TI AFE4403 Heart Rate and Pulse Oximeter");
+MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors
2014-10-14 19:13 ` [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors Dan Murphy
@ 2014-10-15 7:23 ` Daniel Baluta
2014-10-15 19:35 ` Jonathan Cameron
1 sibling, 0 replies; 15+ messages in thread
From: Daniel Baluta @ 2014-10-15 7:23 UTC (permalink / raw)
To: Dan Murphy, linux-iio; +Cc: jic23
On 10/14/2014 10:13 PM, Dan Murphy wrote:
> Add a type for heart rate monitors
> Add the modifier name in beats per minute.
>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> drivers/iio/industrialio-core.c | 2 ++
> .../staging/iio/Documentation/iio_event_monitor.c | 2 ++
> include/linux/iio/types.h | 4 +++-
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index af3e76d..d87ca35 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -70,6 +70,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_CCT] = "cct",
> [IIO_PRESSURE] = "pressure",
> [IIO_HUMIDITYRELATIVE] = "humidityrelative",
> + [IIO_HEARTRATE] = "heartrate",
> };
>
> static const char * const iio_modifier_names[] = {
> @@ -91,6 +92,7 @@ static const char * const iio_modifier_names[] = {
> [IIO_MOD_NORTH_TRUE] = "from_north_true",
> [IIO_MOD_NORTH_MAGN_TILT_COMP] = "from_north_magnetic_tilt_comp",
> [IIO_MOD_NORTH_TRUE_TILT_COMP] = "from_north_true_tilt_comp",
> + [IIO_MOD_HEART_BPM] = "heartrate_beats_per_minute",
s/heartrate_beats_per_minute/beats_per_minute
Anyhow, this is not used, consider removing it.
> };
>
> /* relies on pairs of these shared then separate */
> diff --git a/drivers/staging/iio/Documentation/iio_event_monitor.c b/drivers/staging/iio/Documentation/iio_event_monitor.c
> index 569d6f8..b06dab7 100644
> --- a/drivers/staging/iio/Documentation/iio_event_monitor.c
> +++ b/drivers/staging/iio/Documentation/iio_event_monitor.c
> @@ -49,6 +49,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_CCT] = "cct",
> [IIO_PRESSURE] = "pressure",
> [IIO_HUMIDITYRELATIVE] = "humidityrelative",
> + [IIO_HEARTRATE] = "heartrate"
Add comma after last element.
> };
>
> static const char * const iio_ev_type_text[] = {
> @@ -108,6 +109,7 @@ static bool event_is_known(struct iio_event_data *event)
> case IIO_CCT:
> case IIO_PRESSURE:
> case IIO_HUMIDITYRELATIVE:
> + case IIO_MOD_HEART_BPM:
> break;
> default:
> return false;
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 4a2af8a..50b1456 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -30,6 +30,7 @@ enum iio_chan_type {
> IIO_CCT,
> IIO_PRESSURE,
> IIO_HUMIDITYRELATIVE,
> + IIO_HEARTRATE
Same here.
> };
>
> enum iio_modifier {
> @@ -59,7 +60,8 @@ enum iio_modifier {
> IIO_MOD_NORTH_MAGN,
> IIO_MOD_NORTH_TRUE,
> IIO_MOD_NORTH_MAGN_TILT_COMP,
> - IIO_MOD_NORTH_TRUE_TILT_COMP
> + IIO_MOD_NORTH_TRUE_TILT_COMP,
> + IIO_MOD_HEART_BPM
Same here.
> };
>
> enum iio_event_type {
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor
2014-10-14 19:13 ` [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor Dan Murphy
@ 2014-10-15 7:35 ` Daniel Baluta
2014-10-15 17:37 ` Dan Murphy
2014-10-15 9:52 ` Peter Meerwald
1 sibling, 1 reply; 15+ messages in thread
From: Daniel Baluta @ 2014-10-15 7:35 UTC (permalink / raw)
To: Dan Murphy, linux-iio; +Cc: jic23
On 10/14/2014 10:13 PM, Dan Murphy wrote:
> Add the TI afe4403 heart rate monitor.
> This device detects reflected LED
> wave length fluctuations and presents an ADC
> value to the user space to be converted to a
> heart rate.
>
> Data sheet located here:
> http://www.ti.com/product/AFE4403/datasheet
<snip>
> +obj-y += heart_monitors/
s/heart_monitors/heart_monitor
> obj-y += humidity/
> obj-y += imu/
> obj-y += light/
> diff --git a/drivers/iio/heart_monitors/Kconfig b/drivers/iio/heart_monitors/Kconfig
> new file mode 100644
> index 0000000..7060694
> --- /dev/null
> +++ b/drivers/iio/heart_monitors/Kconfig
> @@ -0,0 +1,16 @@
> +#
> +# Heart Rate Monitor drivers
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Heart Rate Monitors"
> +
> +config AFE4403
> + tristate "TI AFE4403 Heart Rate Monitor"
> + depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + select SYSFS
> + help
Final version should contain a real help here.
> +
> +endmenu
> diff --git a/drivers/iio/heart_monitors/Makefile b/drivers/iio/heart_monitors/Makefile
> new file mode 100644
> index 0000000..77cc5c5
> --- /dev/null
> +++ b/drivers/iio/heart_monitors/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for IIO Heart Rate Monitor drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_AFE4403) += afe4403.o
> diff --git a/drivers/iio/heart_monitors/afe4403.c b/drivers/iio/heart_monitors/afe4403.c
> new file mode 100644
> index 0000000..d285769
> --- /dev/null
> +++ b/drivers/iio/heart_monitors/afe4403.c
> @@ -0,0 +1,610 @@
> +/*
> + * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>
> + *
> + * Copyright: (C) 2014 Texas Instruments, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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/device.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/kfifo_buf.h>
> +
> +#define AFE4403_CONTROL0 0x00
> +#define AFE4403_LED2STC 0x01
> +#define AFE4403_LED2ENDC 0x02
> +#define AFE4403_LED2LEDSTC 0x03
> +#define AFE4403_LED2LEDENDC 0x04
> +#define AFE4403_ALED2STC 0x05
> +#define AFE4403_ALED2ENDC 0x06
> +#define AFE4403_LED1STC 0x07
> +#define AFE4403_LED1ENDC 0x08
> +#define AFE4403_LED1LEDSTC 0x09
> +#define AFE4403_LED1LEDENDC 0x0a
> +#define AFE4403_ALED1STC 0x0b
> +#define AFE4403_ALED1ENDC 0x0c
> +#define AFE4403_LED2CONVST 0x0d
> +#define AFE4403_LED2CONVEND 0x0e
> +#define AFE4403_ALED2CONVST 0x0f
> +#define AFE4403_ALED2CONVEND 0x10
> +#define AFE4403_LED1CONVST 0x11
> +#define AFE4403_LED1CONVEND 0x12
> +#define AFE4403_ALED1CONVST 0x13
> +#define AFE4403_ALED1CONVEND 0x14
> +#define AFE4403_ADCRSTSTCT0 0x15
> +#define AFE4403_ADCRSTENDCT0 0x16
> +#define AFE4403_ADCRSTSTCT1 0x17
> +#define AFE4403_ADCRSTENDCT1 0x18
> +#define AFE4403_ADCRSTSTCT2 0x19
> +#define AFE4403_ADCRSTENDCT2 0x1a
> +#define AFE4403_ADCRSTSTCT3 0x1b
> +#define AFE4403_ADCRSTENDCT3 0x1c
> +#define AFE4403_PRPCOUNT 0x1d
> +#define AFE4403_CONTROL1 0x1e
> +#define AFE4403_SPARE1 0x1f
> +#define AFE4403_TIAGAIN 0x20
> +#define AFE4403_TIA_AMB_GAIN 0x21
> +#define AFE4403_LEDCNTRL 0x22
> +#define AFE4403_CONTROL2 0x23
> +#define AFE4403_SPARE2 0x24
> +#define AFE4403_SPARE3 0x25
> +#define AFE4403_SPARE4 0x26
> +#define AFE4403_ALARM 0x29
> +#define AFE4403_LED2VAL 0x2A
> +#define AFE4403_ALED2VAL 0x2B
> +#define AFE4403_LED1VAL 0x2C
> +#define AFE4403_ALED1VAL 0x2D
> +#define AFE4403_LED2_ALED2VAL 0x2E
> +#define AFE4403_LED1_ALED1VAL 0x2F
> +#define AFE4403_DIAG 0x30
> +#define AFE4403_CONTROL3 0x31
> +#define AFE4403_PDNCYCLESTC 0x32
> +#define AFE4403_PDNCYCLEENDC 0x33
> +
> +#define AFE4403_SPI_READ (1 << 0)
> +#define AFE4403_SW_RESET (1 << 3)
> +#define AFE4403_PWR_DWN (1 << 0)
Could use BIT(x) macro here.
> +
> +/**
> + * struct afe4403_data -
> + * @work - Work item used to off load the enable/disable of the vibration
> + * @indio_dev -
> + * @map -
> + * @spi -
> + * @regulator - Pointer to the regulator for the IC
> + * @ste_gpio -
> + * @data_gpio -
> + * @reset_gpio -
> + * @data_buffer -
> + * @irq - AFE4403 interrupt number
> +**/
:), i assume this will be documented in the final version.
> +struct afe4403_data {
> + struct work_struct work;
> + struct iio_dev *indio_dev;
> + struct iio_map *map;
> + struct spi_device *spi;
> + struct regulator *regulator;
> + struct gpio_desc *ste_gpio;
> + struct gpio_desc *data_gpio;
> + struct gpio_desc *reset_gpio;
> + int data_buffer[5];
> + int irq;
> +};
> +
<snip>
> +static void afe4403_toggle_ste(struct afe4403_data *afe4403)
> +{
> + gpiod_set_value(afe4403->ste_gpio, 1);
> + udelay(800);
This hardcoded value should be documented.
> + gpiod_set_value(afe4403->ste_gpio, 0);
> +}
> +
<snip>
> +static int afe4403_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
> + int ret;
> + int control_val;
> +
> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
> + if (ret)
> + return ret;
> +
Use dev_dbg instead of printks.
> + printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN);
> + if (state)
> + control_val &= ~AFE4403_PWR_DWN;
> + else
> + control_val |= AFE4403_PWR_DWN;
> +
> + printk("***after control 0x%X\n", control_val);
> +
> + ret = afe4403_write(afe4403, AFE4403_CONTROL2, control_val);
> + if (ret)
> + return ret;
> +
> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
> + if (ret)
> + return ret;
> +
> + printk("***control 0x%X\n", control_val);
> +
> + return 0;
> +}
Also, run checkpatch.pl over your patches. There are small whitespace
issues.
thanks,
Daniel.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFC Introducing Heart Monitors for IIO
2014-10-14 19:13 RFC Introducing Heart Monitors for IIO Dan Murphy
` (2 preceding siblings ...)
2014-10-14 19:13 ` [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor Dan Murphy
@ 2014-10-15 7:37 ` Daniel Baluta
2014-10-15 14:37 ` Dan Murphy
3 siblings, 1 reply; 15+ messages in thread
From: Daniel Baluta @ 2014-10-15 7:37 UTC (permalink / raw)
To: Dan Murphy, linux-iio; +Cc: jic23, laurentiu.palcu
On 10/14/2014 10:13 PM, Dan Murphy wrote:
> All
>
> I am introducing a new sensor category for IIO. This is for
> heart rate monitors.
>
> Putting this out as a RFC to see if anyone else is developing the
> same and we can build upon each others patches.
>
> I have another sensor to follow up on this series but don't want
> to start that driver until I understand if there is anyone else
> doing the same.
>
> The first patch introduces the heart rate monitoring into the IIO
> sub-systems
>
> The second patch introduces the DT documentation heart_monitors directory
> along with the documentation for the TI AFE4403 part.
>
> The final patch introduces the heart_monitors directory along with the source
> for the TI AFE4403 device
We are happy to see this! What is your second sensor?
Daniel.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor
2014-10-14 19:13 ` [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor Dan Murphy
2014-10-15 7:35 ` Daniel Baluta
@ 2014-10-15 9:52 ` Peter Meerwald
2014-10-22 15:36 ` Dan Murphy
1 sibling, 1 reply; 15+ messages in thread
From: Peter Meerwald @ 2014-10-15 9:52 UTC (permalink / raw)
To: Dan Murphy; +Cc: linux-iio, jic23
> Add the TI afe4403 heart rate monitor.
> This device detects reflected LED
> wave length fluctuations and presents an ADC
> value to the user space to be converted to a
> heart rate.
> Data sheet located here:
> http://www.ti.com/product/AFE4403/datasheet
s/heart_monitor/heart_monitors
you need to update Documentation/ABI/testing/sysfs-bus-iio
more minor comments below
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> drivers/iio/Kconfig | 1 +
> drivers/iio/Makefile | 1 +
> drivers/iio/heart_monitors/Kconfig | 16 +
> drivers/iio/heart_monitors/Makefile | 6 +
> drivers/iio/heart_monitors/afe4403.c | 610 ++++++++++++++++++++++++++++++++++
> 5 files changed, 634 insertions(+)
> create mode 100644 drivers/iio/heart_monitors/Kconfig
> create mode 100644 drivers/iio/heart_monitors/Makefile
> create mode 100644 drivers/iio/heart_monitors/afe4403.c
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 345395e..e3a7945 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -66,6 +66,7 @@ source "drivers/iio/common/Kconfig"
> source "drivers/iio/dac/Kconfig"
> source "drivers/iio/frequency/Kconfig"
> source "drivers/iio/gyro/Kconfig"
> +source "drivers/iio/heart_monitors/Kconfig"
heartrate would be my preference to name the category
(feel free to ignore)
and use heartrate / HEARTRATE consistently, not HEART_, not HEARTBEAT_
> source "drivers/iio/humidity/Kconfig"
> source "drivers/iio/imu/Kconfig"
> source "drivers/iio/light/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 698afc2..46c0960 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -18,6 +18,7 @@ obj-y += common/
> obj-y += dac/
> obj-y += gyro/
> obj-y += frequency/
> +obj-y += heart_monitors/
> obj-y += humidity/
> obj-y += imu/
> obj-y += light/
> diff --git a/drivers/iio/heart_monitors/Kconfig b/drivers/iio/heart_monitors/Kconfig
> new file mode 100644
> index 0000000..7060694
> --- /dev/null
> +++ b/drivers/iio/heart_monitors/Kconfig
> @@ -0,0 +1,16 @@
> +#
> +# Heart Rate Monitor drivers
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Heart Rate Monitors"
> +
> +config AFE4403
> + tristate "TI AFE4403 Heart Rate Monitor"
> + depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + select SYSFS
other drivers 'depend' on SYSFS rather than 'select', and use SPI_MASTER
> + help
> +
> +endmenu
> diff --git a/drivers/iio/heart_monitors/Makefile b/drivers/iio/heart_monitors/Makefile
> new file mode 100644
> index 0000000..77cc5c5
> --- /dev/null
> +++ b/drivers/iio/heart_monitors/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for IIO Heart Rate Monitor drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_AFE4403) += afe4403.o
> diff --git a/drivers/iio/heart_monitors/afe4403.c b/drivers/iio/heart_monitors/afe4403.c
> new file mode 100644
> index 0000000..d285769
> --- /dev/null
> +++ b/drivers/iio/heart_monitors/afe4403.c
> @@ -0,0 +1,610 @@
> +/*
> + * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>
> + *
> + * Copyright: (C) 2014 Texas Instruments, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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/device.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/kfifo_buf.h>
> +
> +#define AFE4403_CONTROL0 0x00
> +#define AFE4403_LED2STC 0x01
> +#define AFE4403_LED2ENDC 0x02
> +#define AFE4403_LED2LEDSTC 0x03
> +#define AFE4403_LED2LEDENDC 0x04
> +#define AFE4403_ALED2STC 0x05
> +#define AFE4403_ALED2ENDC 0x06
> +#define AFE4403_LED1STC 0x07
> +#define AFE4403_LED1ENDC 0x08
> +#define AFE4403_LED1LEDSTC 0x09
> +#define AFE4403_LED1LEDENDC 0x0a
> +#define AFE4403_ALED1STC 0x0b
> +#define AFE4403_ALED1ENDC 0x0c
> +#define AFE4403_LED2CONVST 0x0d
> +#define AFE4403_LED2CONVEND 0x0e
> +#define AFE4403_ALED2CONVST 0x0f
> +#define AFE4403_ALED2CONVEND 0x10
> +#define AFE4403_LED1CONVST 0x11
> +#define AFE4403_LED1CONVEND 0x12
> +#define AFE4403_ALED1CONVST 0x13
> +#define AFE4403_ALED1CONVEND 0x14
> +#define AFE4403_ADCRSTSTCT0 0x15
> +#define AFE4403_ADCRSTENDCT0 0x16
> +#define AFE4403_ADCRSTSTCT1 0x17
> +#define AFE4403_ADCRSTENDCT1 0x18
> +#define AFE4403_ADCRSTSTCT2 0x19
> +#define AFE4403_ADCRSTENDCT2 0x1a
> +#define AFE4403_ADCRSTSTCT3 0x1b
> +#define AFE4403_ADCRSTENDCT3 0x1c
> +#define AFE4403_PRPCOUNT 0x1d
> +#define AFE4403_CONTROL1 0x1e
> +#define AFE4403_SPARE1 0x1f
> +#define AFE4403_TIAGAIN 0x20
> +#define AFE4403_TIA_AMB_GAIN 0x21
> +#define AFE4403_LEDCNTRL 0x22
> +#define AFE4403_CONTROL2 0x23
> +#define AFE4403_SPARE2 0x24
> +#define AFE4403_SPARE3 0x25
> +#define AFE4403_SPARE4 0x26
> +#define AFE4403_ALARM 0x29
> +#define AFE4403_LED2VAL 0x2A
> +#define AFE4403_ALED2VAL 0x2B
> +#define AFE4403_LED1VAL 0x2C
> +#define AFE4403_ALED1VAL 0x2D
> +#define AFE4403_LED2_ALED2VAL 0x2E
> +#define AFE4403_LED1_ALED1VAL 0x2F
> +#define AFE4403_DIAG 0x30
> +#define AFE4403_CONTROL3 0x31
> +#define AFE4403_PDNCYCLESTC 0x32
> +#define AFE4403_PDNCYCLEENDC 0x33
> +
> +#define AFE4403_SPI_READ (1 << 0)
> +#define AFE4403_SW_RESET (1 << 3)
> +#define AFE4403_PWR_DWN (1 << 0)
can use BIT()
> +
> +/**
> + * struct afe4403_data -
> + * @work - Work item used to off load the enable/disable of the vibration
> + * @indio_dev -
> + * @map -
> + * @spi -
> + * @regulator - Pointer to the regulator for the IC
> + * @ste_gpio -
> + * @data_gpio -
> + * @reset_gpio -
> + * @data_buffer -
> + * @irq - AFE4403 interrupt number
> +**/
> +struct afe4403_data {
> + struct work_struct work;
> + struct iio_dev *indio_dev;
> + struct iio_map *map;
> + struct spi_device *spi;
> + struct regulator *regulator;
> + struct gpio_desc *ste_gpio;
> + struct gpio_desc *data_gpio;
> + struct gpio_desc *reset_gpio;
> + int data_buffer[5];
magic 5
> + int irq;
> +};
> +
> +enum afe4403_reg_id {
> + LED1VAL,
> + ALED1VAL,
> + LED2VAL,
> + ALED2VAL,
> + DIAG,
> + TIAGAIN,
> + TIA_AMB_GAIN,
> + LEDCNTRL,
> + CONTROL3,
> +};
> +
> +static const struct iio_event_spec afe4403_events[] = {
> + {
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> +#define AFE4403_COMMON_CHAN(index, name) \
> + { \
> + .type = IIO_HEARTRATE, \
> + .indexed = 1, \
> + .channel = index, \
> + .datasheet_name = name, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .scan_index = index + 1, \
could do (index), but doesn't matter
why +1?
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 24, \
top 2 bits are ignored
> + .storagebits = 24, \
.endianness = IIO_BE
> + }, \
> + .event_spec = afe4403_events, \
> + .num_event_specs = ARRAY_SIZE(afe4403_events), \
> + }
> +
> +static const struct iio_chan_spec afe4403_channels[] = {
> + /* Read only values from the IC */
> + [LED1VAL] = AFE4403_COMMON_CHAN(0, "LED1VAL"),
> + [ALED1VAL] = AFE4403_COMMON_CHAN(1, "ALED1VAL"),
> + [LED2VAL] = AFE4403_COMMON_CHAN(2, "LED2VAL"),
> + [ALED1VAL] = AFE4403_COMMON_CHAN(3, "ALED2VAL"),
> + [DIAG] = AFE4403_COMMON_CHAN(4, "DIAG"),
> +
> + /* Required writing for calibration */
> + [TIAGAIN] = AFE4403_COMMON_CHAN(5, "TIAGAIN"),
> + [TIA_AMB_GAIN] = AFE4403_COMMON_CHAN(6, "TIA_AMB_GAIN"),
> + [LEDCNTRL] = AFE4403_COMMON_CHAN(7, "LEDCNTRL"),
> + [CONTROL3] = AFE4403_COMMON_CHAN(8, "CONTROL3"),
> +};
> +
> +static struct iio_map afe4403_default_iio_maps[] = {
> + {
> + .consumer_dev_name = "afe4403_heartrate",
> + .consumer_channel = "afe4403_led1value",
> + .adc_channel_label = "LED1VAL",
> + },
> + {
> + .consumer_dev_name = "afe4403_heartrate",
> + .consumer_channel = "afe4403_aled1value",
> + .adc_channel_label = "ALED1VAL",
> + },
> + {
> + .consumer_dev_name = "afe4403_heartrate",
> + .consumer_channel = "afe4403_led2value",
> + .adc_channel_label = "LED2VAL",
> + },
> + {
> + .consumer_dev_name = "afe4403_heartrate",
> + .consumer_channel = "afe4403_aled2value",
> + .adc_channel_label = "ALED2VAL",
> + },
> + {
> + .consumer_dev_name = "afe4403_heartrate",
> + .consumer_channel = "afe4403_diagnostics",
> + .adc_channel_label = "DIAG",
> + },
> +};
> +
> +static void afe4403_toggle_ste(struct afe4403_data *afe4403)
> +{
> + gpiod_set_value(afe4403->ste_gpio, 1);
> + udelay(800);
> + gpiod_set_value(afe4403->ste_gpio, 0);
> +}
> +
> +static int afe4403_read(struct afe4403_data *afe4403, u8 reg,
> + unsigned int *data)
> +{
> + int ret;
> +
> + ret = spi_write_then_read(afe4403->spi, (u8 *)®, 1, (u8 *)data, 3);
> + if (ret)
> + return -EIO;
> +
> + afe4403_toggle_ste(afe4403);
> +
> + return ret;
> +};
> +
> +static int afe4403_write(struct afe4403_data *afe4403, u8 reg,
> + unsigned int data)
> +{
> + int ret;
> + u8 tx[4] = {0x0, 0x0, 0x0, 0x0};
> +
> + /* Enable write to the device */
> +
> + tx[0] = AFE4403_CONTROL0;
> + tx[3] = 0x0;
> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
> + if (ret)
> + return -EIO;
> +
> + afe4403_toggle_ste(afe4403);
> +
> + tx[0] = reg;
> + tx[1] = (data & 0x0f0000) >> 16;
> + tx[2] = (data & 0x00ff00) >> 8;
> + tx[3] = data & 0xff;
> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
> + if (ret)
> + return -EIO;
> +
> + afe4403_toggle_ste(afe4403);
> +
> + /* Re-Enable reading from the device */
> + tx[0] = AFE4403_CONTROL0;
> + tx[1] = 0x0;
> + tx[2] = 0x0;
> + tx[3] = AFE4403_SPI_READ;
> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
> + if (ret)
> + return -EIO;
> +
> + afe4403_toggle_ste(afe4403);
> +
> + return ret;
> +};
> +
> +static int afe4403_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct afe4403_data *data = iio_priv(indio_dev);
> + u8 reg;
> +
> + if (chan->channel > ARRAY_SIZE(afe4403_channels))
> + return -EINVAL;
> +
check for channel < 5?
check for val2 == 0
this is not how to control iio channels
can't _SCALE be used?
> + if (chan->channel == 5)
> + reg = AFE4403_TIAGAIN;
> + else if (chan->channel == 6)
> + reg = AFE4403_TIA_AMB_GAIN;
> + else if (chan->channel == 7)
> + reg = AFE4403_LEDCNTRL;
> + else if (chan->channel == 8)
> + reg = AFE4403_CONTROL2;
> + else
> + return -EINVAL;
> +
> + return afe4403_write(data, reg, val);
> +}
> +
> +static int afe4403_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct afe4403_data *data = iio_priv(indio_dev);
> +
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> +
> + if (chan->channel > ARRAY_SIZE(afe4403_channels))
>=
and it is data_buffer[5]; 5 is smaller than ARRAY_SIZE(afe4003_channels)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + *val = data->data_buffer[chan->channel];
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int afe4403_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
> + int ret;
> + int control_val;
> +
> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
_read() only read 3 bytes, leaving the remaining bytes of control_val
uninitialized; control_val should be unsigned int
wondering about endianness...
> + if (ret)
> + return ret;
> +
> + printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN);
no printk()s please
> + if (state)
> + control_val &= ~AFE4403_PWR_DWN;
> + else
> + control_val |= AFE4403_PWR_DWN;
> +
> + printk("***after control 0x%X\n", control_val);
> +
> + ret = afe4403_write(afe4403, AFE4403_CONTROL2, control_val);
> + if (ret)
> + return ret;
> +
> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
> + if (ret)
> + return ret;
> +
> + printk("***control 0x%X\n", control_val);
> +
> + return 0;
> +}
> +
> +static const struct iio_info afe4403_iio_info = {
> + .read_raw = &afe4403_read_raw,
> + .write_raw = &afe4403_write_raw,
> + .write_event_config = &afe4403_write_event,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static irqreturn_t afe4403_event_handler(int irq, void *data)
> +{
> + struct iio_dev *indio_dev = data;
> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
> + int ret;
> +
> + ret = afe4403_read(afe4403, AFE4403_LED1VAL, &afe4403->data_buffer[0]);
> + if (ret)
> + goto done;
can't this be done all in one go?
> +
> + ret = afe4403_read(afe4403, AFE4403_ALED1VAL, &afe4403->data_buffer[1]);
> + if (ret)
> + goto done;
> +
> + ret = afe4403_read(afe4403, AFE4403_LED2VAL, &afe4403->data_buffer[2]);
> + if (ret)
> + goto done;
> +
> + ret = afe4403_read(afe4403, AFE4403_ALED2VAL, &afe4403->data_buffer[3]);
> + if (ret)
> + goto done;
> +
> + ret = afe4403_read(afe4403, AFE4403_DIAG, &afe4403->data_buffer[4]);
> + if (ret)
> + goto done;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &afe4403->data_buffer,
> + iio_get_time_ns());
> +done:
> + return IRQ_HANDLED;
> +}
> +
> +struct afe4403_reg {
> + uint8_t reg;
> + u32 value;
> +} afe4403_init_regs[] = {
> + { AFE4403_LED2STC, 0x0820},
> + { AFE4403_LED2ENDC, 0x0f9e },
> + { AFE4403_LED2LEDSTC, 0x07d0 },
> + { AFE4403_LED2LEDENDC, 0x0f9f },
> + { AFE4403_ALED2STC, 0x0050 },
> + { AFE4403_ALED2ENDC, 0x07ce },
> + { AFE4403_LED1STC, 0xc350 },
> + { AFE4403_LED1ENDC, 0xc350 },
> + { AFE4403_LED1LEDSTC, 0xc350 },
> + { AFE4403_LED1LEDENDC, 0xc350 },
> + { AFE4403_ALED1STC, 0x0ff0 },
> + { AFE4403_ALED1ENDC, 0x176e },
> + { AFE4403_LED2CONVST, 0x1775 },
> + { AFE4403_LED2CONVEND, 0x1f3f },
> + { AFE4403_ALED2CONVST, 0x1f45 },
> + { AFE4403_ALED2CONVEND, 0x270f },
> + { AFE4403_LED1CONVST, 0x2715 },
> + { AFE4403_LED1CONVEND, 0x2edf },
> + { AFE4403_ALED1CONVST, 0x2ee5 },
> + { AFE4403_ALED1CONVEND, 0x36af },
> + { AFE4403_ADCRSTSTCT0, 0x1770 },
> + { AFE4403_ADCRSTENDCT0, 0x1774 },
> + { AFE4403_ADCRSTSTCT1, 0x1f40 },
> + { AFE4403_ADCRSTENDCT1, 0x1f44 },
> + { AFE4403_ADCRSTSTCT2, 0x2710 },
> + { AFE4403_ADCRSTENDCT2, 0x2714 },
> + { AFE4403_ADCRSTSTCT3, 0x2ee0 },
> + { AFE4403_ADCRSTENDCT3, 0x2ee4 },
> + { AFE4403_PRPCOUNT, 0x09c3f },
> + { AFE4403_CONTROL1, 0x0107 },
> + { AFE4403_TIAGAIN, 0x8006 },
> + { AFE4403_TIA_AMB_GAIN, 0x06 },
> + { AFE4403_LEDCNTRL, 0x11414 },
> + { AFE4403_CONTROL2, 0x20000 },
> +};
> +
> +static int afe4403_init(struct afe4403_data *afe4403)
> +{
> + int ret, i, reg_count;
> +
> + /* Hard reset the device */
> + if (afe4403->reset_gpio) {
> + gpiod_set_value(afe4403->reset_gpio, 0);
> + udelay(800);
> + gpiod_set_value(afe4403->reset_gpio, 1);
> + } else {
> + /* Soft reset the device */
> + ret = afe4403_write(afe4403, AFE4403_CONTROL0, AFE4403_SW_RESET);
> + if (ret)
> + return ret;
> + }
> +
> + udelay(800);
> +
> + reg_count = sizeof(afe4403_init_regs) / sizeof(afe4403_init_regs[0]);
ARRAY_SIZE()
> + for (i = 0; i < reg_count; i++) {
> + ret = afe4403_write(afe4403, afe4403_init_regs[i].reg,
> + afe4403_init_regs[i].value);
> + if (ret)
> + return ret;
> + }
> +
> + return ret;
> +};
> +
> +static int afe4403_iio_buffered_hardware_setup(struct iio_dev *indio_dev)
> +{
> + struct iio_buffer *buffer;
> + int ret;
> +
> + buffer = iio_kfifo_allocate(indio_dev);
> + if (!buffer)
> + return -ENOMEM;
> +
> + iio_device_attach_buffer(indio_dev, buffer);
> +
> + ret = iio_buffer_register(indio_dev,
> + indio_dev->channels,
> + indio_dev->num_channels);
> + if (ret)
> + goto error_kfifo_free;
> +
> + return 0;
> +
> +error_kfifo_free:
> + iio_kfifo_free(indio_dev->buffer);
> + return ret;
> +}
> +
> +static int afe4403_spi_probe(struct spi_device *spi)
> +{
> + struct afe4403_data *afe4403;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*afe4403));
> + if (indio_dev == NULL) {
> + dev_err(&spi->dev, "Failed to allocate iio device\n");
> + return -ENOMEM;
> + }
> +
> + spi_set_drvdata(spi, indio_dev);
> +
> + afe4403 = iio_priv(indio_dev);
> + afe4403->spi = spi;
> +
> + ret = iio_map_array_register(indio_dev, afe4403_default_iio_maps);
> + if (ret) {
> + dev_err(&indio_dev->dev, "iio map err: %d\n", ret);
> + return ret;
> + }
> + afe4403->map = afe4403_default_iio_maps;
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->name = "afe4403";
> + indio_dev->info = &afe4403_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;
> + indio_dev->channels = afe4403_channels;
> + indio_dev->num_channels = ARRAY_SIZE(afe4403_channels);
> +
> + afe4403->ste_gpio = devm_gpiod_get(&spi->dev, "ste");
> + if (IS_ERR(afe4403->ste_gpio)) {
> + ret = PTR_ERR(afe4403->ste_gpio);
> + if (ret != -ENOENT && ret != -ENOSYS) {
> + dev_err(&spi->dev, "Failed to allocate spi gpio\n");
> + return ret;
> + }
> + afe4403->ste_gpio = NULL;
> + } else {
> + gpiod_direction_output(afe4403->ste_gpio, 0);
> + }
> +
> + afe4403->data_gpio = devm_gpiod_get(&spi->dev, "data-ready");
> + if (IS_ERR(afe4403->data_gpio)) {
> + ret = PTR_ERR(afe4403->data_gpio);
> + if (ret != -ENOENT && ret != -ENOSYS) {
> + dev_err(&spi->dev, "Failed to allocate data_ready gpio\n");
> + return ret;
> + }
> + afe4403->data_gpio = NULL;
> + } else {
> + gpiod_direction_input(afe4403->data_gpio);
> + afe4403->irq = gpiod_to_irq(afe4403->data_gpio);
> + }
> +
> + afe4403->reset_gpio = devm_gpiod_get(&spi->dev, "reset");
> + if (IS_ERR(afe4403->reset_gpio)) {
> + ret = PTR_ERR(afe4403->reset_gpio);
> + if (ret != -ENOENT && ret != -ENOSYS) {
> + dev_err(&spi->dev, "Failed to allocate reset gpio\n");
> + return ret;
> + }
> + afe4403->reset_gpio = NULL;
> + } else {
> + gpiod_direction_output(afe4403->reset_gpio, 1);
> + }
> +
> + ret = afe4403_iio_buffered_hardware_setup(indio_dev);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to allocate iio buffers %i\n", ret);
> + return ret;
> + }
> +
> + ret = devm_request_threaded_irq(&spi->dev, afe4403->irq,
> + NULL,
> + afe4403_event_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + "afe4403 int",
> + indio_dev);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to allocate thread %i\n", ret);
threaded irq?
> + return ret;
> + }
> +
> + ret = devm_iio_device_register(&spi->dev, indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = afe4403_init(afe4403);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to init device\n");
and now what? no cleanup?
> + return ret;
> + }
> +
> + return 0;
> +
> +}
> +
> +static int afe4403_spi_remove(struct spi_device *spi)
> +{
> + struct afe4403_data *afe4403 = dev_get_drvdata(&spi->dev);
> +
> + iio_kfifo_free(afe4403->indio_dev->buffer);
> + iio_buffer_unregister(afe4403->indio_dev);
> +
> + if (!IS_ERR(afe4403->regulator))
> + regulator_disable(afe4403->regulator);
regulator never enabled?
> +
> + return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id afe4403_of_match[] = {
> + { .compatible = "ti,afe4403", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, afe4403_of_match);
> +#endif
> +
> +static struct spi_driver afe4403_spi_driver = {
> + .driver = {
> + .name = "afe4403",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(afe4403_of_match),
> + },
> + .probe = afe4403_spi_probe,
> + .remove = afe4403_spi_remove,
> +};
> +
> +static int __init afe4403_spi_init(void)
> +{
> + return spi_register_driver(&afe4403_spi_driver);
> +}
> +module_init(afe4403_spi_init);
> +
> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> +MODULE_DESCRIPTION("TI AFE4403 Heart Rate and Pulse Oximeter");
> +MODULE_LICENSE("GPL");
>
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation
2014-10-14 19:13 ` [RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation Dan Murphy
@ 2014-10-15 9:52 ` Peter Meerwald
0 siblings, 0 replies; 15+ messages in thread
From: Peter Meerwald @ 2014-10-15 9:52 UTC (permalink / raw)
To: Dan Murphy; +Cc: linux-iio, jic23
> Add the TI afe4403 heart monitor device tree
> binding documentation. heart_monitors directory
> created under iio.
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> .../bindings/iio/heart_monitors/ti_afe4403.txt | 23 ++++++++++++++++++++
> 1 file changed, 23 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt b/Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt
> new file mode 100644
> index 0000000..d612dd6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt
> @@ -0,0 +1,23 @@
> +* Texas Instruments - AFE4403 Heart rate and Pulse Oximeter
> +
> +The device consists of a low-noise receiver channel
> +with an integrated analog-to-digital converter (ADC),
> +an LED transmit section, and diagnostics for sensor and LED fault detection.
> +
> +Required properties:
> + - compatible: Must contain "ti,afe4403".
> + - ste-gpio: GPIO for the spi control line
> + - data-ready-gpio: GPIO interrupt when the afe4403 has data
> + - led-supply: Chip supply to the device
> + -
extra -
> +Optional properties:
> + - reset-gpio: GPIO used to reset the device via HW
> +
> +Example:
> +
> +&heart_rate {
> + compatible = "ti,afe4403";
> + ste-gpio = <&gpio1 29 GPIO_ACTIVE_HIGH>;
> + data-ready-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> + led-supply = <&vbat>;
> +};
>
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFC Introducing Heart Monitors for IIO
2014-10-15 7:37 ` RFC Introducing Heart Monitors for IIO Daniel Baluta
@ 2014-10-15 14:37 ` Dan Murphy
2014-10-15 14:43 ` Karol Wrona
0 siblings, 1 reply; 15+ messages in thread
From: Dan Murphy @ 2014-10-15 14:37 UTC (permalink / raw)
To: Daniel Baluta, linux-iio; +Cc: jic23, laurentiu.palcu
Daniel
On 10/15/2014 02:37 AM, Daniel Baluta wrote:
>
>
> On 10/14/2014 10:13 PM, Dan Murphy wrote:
>> All
>>
>> I am introducing a new sensor category for IIO. This is for
>> heart rate monitors.
>>
>> Putting this out as a RFC to see if anyone else is developing the
>> same and we can build upon each others patches.
>>
>> I have another sensor to follow up on this series but don't want
>> to start that driver until I understand if there is anyone else
>> doing the same.
>>
>> The first patch introduces the heart rate monitoring into the IIO
>> sub-systems
>>
>> The second patch introduces the DT documentation heart_monitors directory
>> along with the documentation for the TI AFE4403 part.
>>
>> The final patch introduces the heart_monitors directory along with the source
>> for the TI AFE4403 device
>
> We are happy to see this! What is your second sensor?
>
Well right now I cannot give the details as it has not been released but it part of this family
with an I2C interface.
Dan
> Daniel.
>
>
--
------------------
Dan Murphy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFC Introducing Heart Monitors for IIO
2014-10-15 14:37 ` Dan Murphy
@ 2014-10-15 14:43 ` Karol Wrona
0 siblings, 0 replies; 15+ messages in thread
From: Karol Wrona @ 2014-10-15 14:43 UTC (permalink / raw)
To: Dan Murphy, linux-iio; +Cc: jic23
On 10/14/2014 10:13 PM, Dan Murphy wrote:
> All
>
> I am introducing a new sensor category for IIO. This is for
> heart rate monitors.
>
> Putting this out as a RFC to see if anyone else is developing the
> same and we can build upon each others patches.
>
> I have another sensor to follow up on this series but don't want
> to start that driver until I understand if there is anyone else
> doing the same.
>
> The first patch introduces the heart rate monitoring into the IIO
> sub-systems
>
> The second patch introduces the DT documentation heart_monitors directory
> along with the documentation for the TI AFE4403 part.
>
> The final patch introduces the heart_monitors directory along with the source
> for the TI AFE4403 device
Hi,
I use heart rate monitor too so I am also interested in this category.
I depends on Jonathan opinion but I would be happy to see the type in the near
future.
Karol
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor
2014-10-15 7:35 ` Daniel Baluta
@ 2014-10-15 17:37 ` Dan Murphy
0 siblings, 0 replies; 15+ messages in thread
From: Dan Murphy @ 2014-10-15 17:37 UTC (permalink / raw)
To: Daniel Baluta, linux-iio; +Cc: jic23
Daniel
Thanks for the review
On 10/15/2014 02:35 AM, Daniel Baluta wrote:
>
>
> On 10/14/2014 10:13 PM, Dan Murphy wrote:
>> Add the TI afe4403 heart rate monitor.
>> This device detects reflected LED
>> wave length fluctuations and presents an ADC
>> value to the user space to be converted to a
>> heart rate.
>>
>> Data sheet located here:
>> http://www.ti.com/product/AFE4403/datasheet
>
> <snip>
>
>> +obj-y += heart_monitors/
> s/heart_monitors/heart_monitor
Yeah that aligns with the rest of the directories in iio. Singular not plural
>
>> obj-y += humidity/
>> obj-y += imu/
>> obj-y += light/
>> diff --git a/drivers/iio/heart_monitors/Kconfig b/drivers/iio/heart_monitors/Kconfig
>> new file mode 100644
>> index 0000000..7060694
>> --- /dev/null
>> +++ b/drivers/iio/heart_monitors/Kconfig
>> @@ -0,0 +1,16 @@
>> +#
>> +# Heart Rate Monitor drivers
>> +#
>> +# When adding new entries keep the list in alphabetical order
>> +
>> +menu "Heart Rate Monitors"
>> +
>> +config AFE4403
>> + tristate "TI AFE4403 Heart Rate Monitor"
>> + depends on SPI
>> + select IIO_BUFFER
>> + select IIO_TRIGGERED_BUFFER
>> + select SYSFS
>> + help
>
> Final version should contain a real help here.
It will this was just a place holder for RFC
>
>> +
>> +endmenu
>> diff --git a/drivers/iio/heart_monitors/Makefile b/drivers/iio/heart_monitors/Makefile
>> new file mode 100644
>> index 0000000..77cc5c5
>> --- /dev/null
>> +++ b/drivers/iio/heart_monitors/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for IIO Heart Rate Monitor drivers
>> +#
>> +
>> +# When adding new entries keep the list in alphabetical order
>> +obj-$(CONFIG_AFE4403) += afe4403.o
>> diff --git a/drivers/iio/heart_monitors/afe4403.c b/drivers/iio/heart_monitors/afe4403.c
>> new file mode 100644
>> index 0000000..d285769
>> --- /dev/null
>> +++ b/drivers/iio/heart_monitors/afe4403.c
>> @@ -0,0 +1,610 @@
>> +/*
>> + * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.com>
>> + *
>> + * Copyright: (C) 2014 Texas Instruments, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +
>> +#define AFE4403_CONTROL0 0x00
>> +#define AFE4403_LED2STC 0x01
>> +#define AFE4403_LED2ENDC 0x02
>> +#define AFE4403_LED2LEDSTC 0x03
>> +#define AFE4403_LED2LEDENDC 0x04
>> +#define AFE4403_ALED2STC 0x05
>> +#define AFE4403_ALED2ENDC 0x06
>> +#define AFE4403_LED1STC 0x07
>> +#define AFE4403_LED1ENDC 0x08
>> +#define AFE4403_LED1LEDSTC 0x09
>> +#define AFE4403_LED1LEDENDC 0x0a
>> +#define AFE4403_ALED1STC 0x0b
>> +#define AFE4403_ALED1ENDC 0x0c
>> +#define AFE4403_LED2CONVST 0x0d
>> +#define AFE4403_LED2CONVEND 0x0e
>> +#define AFE4403_ALED2CONVST 0x0f
>> +#define AFE4403_ALED2CONVEND 0x10
>> +#define AFE4403_LED1CONVST 0x11
>> +#define AFE4403_LED1CONVEND 0x12
>> +#define AFE4403_ALED1CONVST 0x13
>> +#define AFE4403_ALED1CONVEND 0x14
>> +#define AFE4403_ADCRSTSTCT0 0x15
>> +#define AFE4403_ADCRSTENDCT0 0x16
>> +#define AFE4403_ADCRSTSTCT1 0x17
>> +#define AFE4403_ADCRSTENDCT1 0x18
>> +#define AFE4403_ADCRSTSTCT2 0x19
>> +#define AFE4403_ADCRSTENDCT2 0x1a
>> +#define AFE4403_ADCRSTSTCT3 0x1b
>> +#define AFE4403_ADCRSTENDCT3 0x1c
>> +#define AFE4403_PRPCOUNT 0x1d
>> +#define AFE4403_CONTROL1 0x1e
>> +#define AFE4403_SPARE1 0x1f
>> +#define AFE4403_TIAGAIN 0x20
>> +#define AFE4403_TIA_AMB_GAIN 0x21
>> +#define AFE4403_LEDCNTRL 0x22
>> +#define AFE4403_CONTROL2 0x23
>> +#define AFE4403_SPARE2 0x24
>> +#define AFE4403_SPARE3 0x25
>> +#define AFE4403_SPARE4 0x26
>> +#define AFE4403_ALARM 0x29
>> +#define AFE4403_LED2VAL 0x2A
>> +#define AFE4403_ALED2VAL 0x2B
>> +#define AFE4403_LED1VAL 0x2C
>> +#define AFE4403_ALED1VAL 0x2D
>> +#define AFE4403_LED2_ALED2VAL 0x2E
>> +#define AFE4403_LED1_ALED1VAL 0x2F
>> +#define AFE4403_DIAG 0x30
>> +#define AFE4403_CONTROL3 0x31
>> +#define AFE4403_PDNCYCLESTC 0x32
>> +#define AFE4403_PDNCYCLEENDC 0x33
>> +
>> +#define AFE4403_SPI_READ (1 << 0)
>> +#define AFE4403_SW_RESET (1 << 3)
>> +#define AFE4403_PWR_DWN (1 << 0)
>
> Could use BIT(x) macro here.
OK
>
>> +
>> +/**
>> + * struct afe4403_data -
>> + * @work - Work item used to off load the enable/disable of the vibration
>> + * @indio_dev -
>> + * @map -
>> + * @spi -
>> + * @regulator - Pointer to the regulator for the IC
>> + * @ste_gpio -
>> + * @data_gpio -
>> + * @reset_gpio -
>> + * @data_buffer -
>> + * @irq - AFE4403 interrupt number
>> +**/
>
> :), i assume this will be documented in the final version.
Yes this is a little out of date.
>
>> +struct afe4403_data {
>> + struct work_struct work;
>> + struct iio_dev *indio_dev;
>> + struct iio_map *map;
>> + struct spi_device *spi;
>> + struct regulator *regulator;
>> + struct gpio_desc *ste_gpio;
>> + struct gpio_desc *data_gpio;
>> + struct gpio_desc *reset_gpio;
>> + int data_buffer[5];
>> + int irq;
>> +};
>> +
>
> <snip>
>
>> +static void afe4403_toggle_ste(struct afe4403_data *afe4403)
>> +{
>> + gpiod_set_value(afe4403->ste_gpio, 1);
>> + udelay(800);
>
> This hardcoded value should be documented.
I will have some documentation or at the very least an explanation.
This may change as well.
>
>> + gpiod_set_value(afe4403->ste_gpio, 0);
>> +}
>> +
>
> <snip>
>
>> +static int afe4403_write_event(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + int state)
>> +{
>> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
>> + int ret;
>> + int control_val;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
>> + if (ret)
>> + return ret;
>> +
>
> Use dev_dbg instead of printks.
Yeah accidentally left these in when debugging these will be removed with official submission
>
>> + printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN);
>> + if (state)
>> + control_val &= ~AFE4403_PWR_DWN;
>> + else
>> + control_val |= AFE4403_PWR_DWN;
>> +
>> + printk("***after control 0x%X\n", control_val);
>> +
>> + ret = afe4403_write(afe4403, AFE4403_CONTROL2, control_val);
>> + if (ret)
>> + return ret;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
>> + if (ret)
>> + return ret;
>> +
>> + printk("***control 0x%X\n", control_val);
>> +
>> + return 0;
>> +}
>
> Also, run checkpatch.pl over your patches. There are small whitespace issues.
Sorry for the low quality patch I was not thinking when I submitted this low quality series
of patches
>
> thanks,
> Daniel.
--
------------------
Dan Murphy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors
2014-10-14 19:13 ` [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors Dan Murphy
2014-10-15 7:23 ` Daniel Baluta
@ 2014-10-15 19:35 ` Jonathan Cameron
1 sibling, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2014-10-15 19:35 UTC (permalink / raw)
To: Dan Murphy, linux-iio; +Cc: jic23
On 14/10/14 20:13, Dan Murphy wrote:
> Add a type for heart rate monitors
> Add the modifier name in beats per minute.
The heart rate type is probably fine. The modifier is unnecessary as
we simply define heartrate to be in beats per minute. Modifiers
are used to distinguish things like direction or colour, not to
specify units.
Beats per minute is a little clunky from a general units point of
view, but I can see that beats per second seems a little silly here.
The issue is generality as there are plenty of other sources of
rate signals. Clearly we would like equivalent base units for all
of them to avoid confusion. To that end I wonder if it is
worth the slightly unusual option of beats per second here on
the basis any userspace util can easily multiply by 60.
Also, when introducing a new type - updating
Documentation/ABI/testing/sysfs-bus-iio is both useful in explaining
the use of the ABI and as documentation going forward.
Jonathan
>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> drivers/iio/industrialio-core.c | 2 ++
> .../staging/iio/Documentation/iio_event_monitor.c | 2 ++
> include/linux/iio/types.h | 4 +++-
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index af3e76d..d87ca35 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -70,6 +70,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_CCT] = "cct",
> [IIO_PRESSURE] = "pressure",
> [IIO_HUMIDITYRELATIVE] = "humidityrelative",
> + [IIO_HEARTRATE] = "heartrate",
> };
>
> static const char * const iio_modifier_names[] = {
> @@ -91,6 +92,7 @@ static const char * const iio_modifier_names[] = {
> [IIO_MOD_NORTH_TRUE] = "from_north_true",
> [IIO_MOD_NORTH_MAGN_TILT_COMP] = "from_north_magnetic_tilt_comp",
> [IIO_MOD_NORTH_TRUE_TILT_COMP] = "from_north_true_tilt_comp",
> + [IIO_MOD_HEART_BPM] = "heartrate_beats_per_minute",
> };
>
> /* relies on pairs of these shared then separate */
> diff --git a/drivers/staging/iio/Documentation/iio_event_monitor.c b/drivers/staging/iio/Documentation/iio_event_monitor.c
> index 569d6f8..b06dab7 100644
> --- a/drivers/staging/iio/Documentation/iio_event_monitor.c
> +++ b/drivers/staging/iio/Documentation/iio_event_monitor.c
> @@ -49,6 +49,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_CCT] = "cct",
> [IIO_PRESSURE] = "pressure",
> [IIO_HUMIDITYRELATIVE] = "humidityrelative",
> + [IIO_HEARTRATE] = "heartrate"
> };
>
> static const char * const iio_ev_type_text[] = {
> @@ -108,6 +109,7 @@ static bool event_is_known(struct iio_event_data *event)
> case IIO_CCT:
> case IIO_PRESSURE:
> case IIO_HUMIDITYRELATIVE:
> + case IIO_MOD_HEART_BPM:
> break;
> default:
> return false;
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 4a2af8a..50b1456 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -30,6 +30,7 @@ enum iio_chan_type {
> IIO_CCT,
> IIO_PRESSURE,
> IIO_HUMIDITYRELATIVE,
> + IIO_HEARTRATE
> };
>
> enum iio_modifier {
> @@ -59,7 +60,8 @@ enum iio_modifier {
> IIO_MOD_NORTH_MAGN,
> IIO_MOD_NORTH_TRUE,
> IIO_MOD_NORTH_MAGN_TILT_COMP,
> - IIO_MOD_NORTH_TRUE_TILT_COMP
> + IIO_MOD_NORTH_TRUE_TILT_COMP,
> + IIO_MOD_HEART_BPM
> };
>
> enum iio_event_type {
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor
2014-10-15 9:52 ` Peter Meerwald
@ 2014-10-22 15:36 ` Dan Murphy
2014-10-25 10:13 ` Jonathan Cameron
0 siblings, 1 reply; 15+ messages in thread
From: Dan Murphy @ 2014-10-22 15:36 UTC (permalink / raw)
To: Peter Meerwald; +Cc: linux-iio, jic23
Peter
Sorry for the delayed reply I was working on the code and comments
On 10/15/2014 04:52 AM, Peter Meerwald wrote:
>> Add the TI afe4403 heart rate monitor.
>> This device detects reflected LED
>> wave length fluctuations and presents an ADC
>> value to the user space to be converted to a
>> heart rate.
>> Data sheet located here:
>> http://www.ti.com/product/AFE4403/datasheet
> s/heart_monitor/heart_monitors
>
> you need to update Documentation/ABI/testing/sysfs-bus-iio
>
> more minor comments below
Will update
>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> drivers/iio/Kconfig | 1 +
>> drivers/iio/Makefile | 1 +
>> drivers/iio/heart_monitors/Kconfig | 16 +
>> drivers/iio/heart_monitors/Makefile | 6 +
>> drivers/iio/heart_monitors/afe4403.c | 610 ++++++++++++++++++++++++++++++++++
>> 5 files changed, 634 insertions(+)
>> create mode 100644 drivers/iio/heart_monitors/Kconfig
>> create mode 100644 drivers/iio/heart_monitors/Makefile
>> create mode 100644 drivers/iio/heart_monitors/afe4403.c
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 345395e..e3a7945 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -66,6 +66,7 @@ source "drivers/iio/common/Kconfig"
>> source "drivers/iio/dac/Kconfig"
>> source "drivers/iio/frequency/Kconfig"
>> source "drivers/iio/gyro/Kconfig"
>> +source "drivers/iio/heart_monitors/Kconfig"
> heartrate would be my preference to name the category
> (feel free to ignore)
>
> and use heartrate / HEARTRATE consistently, not HEART_, not HEARTBEAT_
Noted but not changed unless there are are other objections
>
>> source "drivers/iio/humidity/Kconfig"
>> source "drivers/iio/imu/Kconfig"
>> source "drivers/iio/light/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 698afc2..46c0960 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -18,6 +18,7 @@ obj-y += common/
>> obj-y += dac/
>> obj-y += gyro/
>> obj-y += frequency/
>> +obj-y += heart_monitors/
>> obj-y += humidity/
>> obj-y += imu/
>> obj-y += light/
>> diff --git a/drivers/iio/heart_monitors/Kconfig b/drivers/iio/heart_monitors/Kconfig
>> new file mode 100644
>> index 0000000..7060694
>> --- /dev/null
>> +++ b/drivers/iio/heart_monitors/Kconfig
>> @@ -0,0 +1,16 @@
>> +#
>> +# Heart Rate Monitor drivers
>> +#
>> +# When adding new entries keep the list in alphabetical order
>> +
>> +menu "Heart Rate Monitors"
>> +
>> +config AFE4403
>> + tristate "TI AFE4403 Heart Rate Monitor"
>> + depends on SPI
>> + select IIO_BUFFER
>> + select IIO_TRIGGERED_BUFFER
>> + select SYSFS
> other drivers 'depend' on SYSFS rather than 'select', and use SPI_MASTER
Fixed
>
>> + help
>> +
>> +endmenu
>> diff --git a/drivers/iio/heart_monitors/Makefile b/drivers/iio/heart_monitors/Makefile
>> new file mode 100644
>> index 0000000..77cc5c5
>> --- /dev/null
>> +++ b/drivers/iio/heart_monitors/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for IIO Heart Rate Monitor drivers
>> +#
>> +
>> +# When adding new entries keep the list in alphabetical order
>> +obj-$(CONFIG_AFE4403) += afe4403.o
>> diff --git a/drivers/iio/heart_monitors/afe4403.c b/drivers/iio/heart_monitors/afe4403.c
>> new file mode 100644
>> index 0000000..d285769
>> --- /dev/null
>> +++ b/drivers/iio/heart_monitors/afe4403.c
>> @@ -0,0 +1,610 @@
>> +/*
>> + * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.com>
>> + *
>> + * Copyright: (C) 2014 Texas Instruments, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +
>> +#define AFE4403_CONTROL0 0x00
>> +#define AFE4403_LED2STC 0x01
>> +#define AFE4403_LED2ENDC 0x02
>> +#define AFE4403_LED2LEDSTC 0x03
>> +#define AFE4403_LED2LEDENDC 0x04
>> +#define AFE4403_ALED2STC 0x05
>> +#define AFE4403_ALED2ENDC 0x06
>> +#define AFE4403_LED1STC 0x07
>> +#define AFE4403_LED1ENDC 0x08
>> +#define AFE4403_LED1LEDSTC 0x09
>> +#define AFE4403_LED1LEDENDC 0x0a
>> +#define AFE4403_ALED1STC 0x0b
>> +#define AFE4403_ALED1ENDC 0x0c
>> +#define AFE4403_LED2CONVST 0x0d
>> +#define AFE4403_LED2CONVEND 0x0e
>> +#define AFE4403_ALED2CONVST 0x0f
>> +#define AFE4403_ALED2CONVEND 0x10
>> +#define AFE4403_LED1CONVST 0x11
>> +#define AFE4403_LED1CONVEND 0x12
>> +#define AFE4403_ALED1CONVST 0x13
>> +#define AFE4403_ALED1CONVEND 0x14
>> +#define AFE4403_ADCRSTSTCT0 0x15
>> +#define AFE4403_ADCRSTENDCT0 0x16
>> +#define AFE4403_ADCRSTSTCT1 0x17
>> +#define AFE4403_ADCRSTENDCT1 0x18
>> +#define AFE4403_ADCRSTSTCT2 0x19
>> +#define AFE4403_ADCRSTENDCT2 0x1a
>> +#define AFE4403_ADCRSTSTCT3 0x1b
>> +#define AFE4403_ADCRSTENDCT3 0x1c
>> +#define AFE4403_PRPCOUNT 0x1d
>> +#define AFE4403_CONTROL1 0x1e
>> +#define AFE4403_SPARE1 0x1f
>> +#define AFE4403_TIAGAIN 0x20
>> +#define AFE4403_TIA_AMB_GAIN 0x21
>> +#define AFE4403_LEDCNTRL 0x22
>> +#define AFE4403_CONTROL2 0x23
>> +#define AFE4403_SPARE2 0x24
>> +#define AFE4403_SPARE3 0x25
>> +#define AFE4403_SPARE4 0x26
>> +#define AFE4403_ALARM 0x29
>> +#define AFE4403_LED2VAL 0x2A
>> +#define AFE4403_ALED2VAL 0x2B
>> +#define AFE4403_LED1VAL 0x2C
>> +#define AFE4403_ALED1VAL 0x2D
>> +#define AFE4403_LED2_ALED2VAL 0x2E
>> +#define AFE4403_LED1_ALED1VAL 0x2F
>> +#define AFE4403_DIAG 0x30
>> +#define AFE4403_CONTROL3 0x31
>> +#define AFE4403_PDNCYCLESTC 0x32
>> +#define AFE4403_PDNCYCLEENDC 0x33
>> +
>> +#define AFE4403_SPI_READ (1 << 0)
>> +#define AFE4403_SW_RESET (1 << 3)
>> +#define AFE4403_PWR_DWN (1 << 0)
> can use BIT()
Fixed. I need to update these defines per the data sheet to get rid of the
magic numbers below.
I am wondering if a header file may be in order thoughts?
>
>> +
>> +/**
>> + * struct afe4403_data -
>> + * @work - Work item used to off load the enable/disable of the vibration
>> + * @indio_dev -
>> + * @map -
>> + * @spi -
>> + * @regulator - Pointer to the regulator for the IC
>> + * @ste_gpio -
>> + * @data_gpio -
>> + * @reset_gpio -
>> + * @data_buffer -
>> + * @irq - AFE4403 interrupt number
>> +**/
>> +struct afe4403_data {
>> + struct work_struct work;
>> + struct iio_dev *indio_dev;
>> + struct iio_map *map;
>> + struct spi_device *spi;
>> + struct regulator *regulator;
>> + struct gpio_desc *ste_gpio;
>> + struct gpio_desc *data_gpio;
>> + struct gpio_desc *reset_gpio;
>> + int data_buffer[5];
> magic 5
I can #define this but this is the number of registers we read and report from the IC
>
>> + int irq;
>> +};
>> +
>> +enum afe4403_reg_id {
>> + LED1VAL,
>> + ALED1VAL,
>> + LED2VAL,
>> + ALED2VAL,
>> + DIAG,
>> + TIAGAIN,
>> + TIA_AMB_GAIN,
>> + LEDCNTRL,
>> + CONTROL3,
>> +};
>> +
>> +static const struct iio_event_spec afe4403_events[] = {
>> + {
>> + .type = IIO_EV_TYPE_MAG,
>> + .dir = IIO_EV_DIR_EITHER,
>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE),
>> + },
>> +};
>> +
>> +#define AFE4403_COMMON_CHAN(index, name) \
>> + { \
>> + .type = IIO_HEARTRATE, \
>> + .indexed = 1, \
>> + .channel = index, \
>> + .datasheet_name = name, \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
>> + .scan_index = index + 1, \
> could do (index), but doesn't matter
> why +1?
Fixed
>
>> + .scan_type = { \
>> + .sign = 'u', \
>> + .realbits = 24, \
> top 2 bits are ignored
Not sure what you mean here. How are the top two bits ignored?
>
>> + .storagebits = 24, \
> .endianness = IIO_BE
Fixed
>
>> + }, \
>> + .event_spec = afe4403_events, \
>> + .num_event_specs = ARRAY_SIZE(afe4403_events), \
>> + }
>> +
>> +static const struct iio_chan_spec afe4403_channels[] = {
>> + /* Read only values from the IC */
>> + [LED1VAL] = AFE4403_COMMON_CHAN(0, "LED1VAL"),
>> + [ALED1VAL] = AFE4403_COMMON_CHAN(1, "ALED1VAL"),
>> + [LED2VAL] = AFE4403_COMMON_CHAN(2, "LED2VAL"),
>> + [ALED1VAL] = AFE4403_COMMON_CHAN(3, "ALED2VAL"),
>> + [DIAG] = AFE4403_COMMON_CHAN(4, "DIAG"),
>> +
>> + /* Required writing for calibration */
>> + [TIAGAIN] = AFE4403_COMMON_CHAN(5, "TIAGAIN"),
>> + [TIA_AMB_GAIN] = AFE4403_COMMON_CHAN(6, "TIA_AMB_GAIN"),
>> + [LEDCNTRL] = AFE4403_COMMON_CHAN(7, "LEDCNTRL"),
>> + [CONTROL3] = AFE4403_COMMON_CHAN(8, "CONTROL3"),
>> +};
>> +
>> +static struct iio_map afe4403_default_iio_maps[] = {
>> + {
>> + .consumer_dev_name = "afe4403_heartrate",
>> + .consumer_channel = "afe4403_led1value",
>> + .adc_channel_label = "LED1VAL",
>> + },
>> + {
>> + .consumer_dev_name = "afe4403_heartrate",
>> + .consumer_channel = "afe4403_aled1value",
>> + .adc_channel_label = "ALED1VAL",
>> + },
>> + {
>> + .consumer_dev_name = "afe4403_heartrate",
>> + .consumer_channel = "afe4403_led2value",
>> + .adc_channel_label = "LED2VAL",
>> + },
>> + {
>> + .consumer_dev_name = "afe4403_heartrate",
>> + .consumer_channel = "afe4403_aled2value",
>> + .adc_channel_label = "ALED2VAL",
>> + },
>> + {
>> + .consumer_dev_name = "afe4403_heartrate",
>> + .consumer_channel = "afe4403_diagnostics",
>> + .adc_channel_label = "DIAG",
>> + },
>> +};
>> +
>> +static void afe4403_toggle_ste(struct afe4403_data *afe4403)
>> +{
>> + gpiod_set_value(afe4403->ste_gpio, 1);
>> + udelay(800);
>> + gpiod_set_value(afe4403->ste_gpio, 0);
>> +}
>> +
>> +static int afe4403_read(struct afe4403_data *afe4403, u8 reg,
>> + unsigned int *data)
>> +{
>> + int ret;
>> +
>> + ret = spi_write_then_read(afe4403->spi, (u8 *)®, 1, (u8 *)data, 3);
>> + if (ret)
>> + return -EIO;
>> +
>> + afe4403_toggle_ste(afe4403);
>> +
>> + return ret;
>> +};
>> +
>> +static int afe4403_write(struct afe4403_data *afe4403, u8 reg,
>> + unsigned int data)
>> +{
>> + int ret;
>> + u8 tx[4] = {0x0, 0x0, 0x0, 0x0};
>> +
>> + /* Enable write to the device */
>> +
>> + tx[0] = AFE4403_CONTROL0;
>> + tx[3] = 0x0;
>> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
>> + if (ret)
>> + return -EIO;
>> +
>> + afe4403_toggle_ste(afe4403);
>> +
>> + tx[0] = reg;
>> + tx[1] = (data & 0x0f0000) >> 16;
>> + tx[2] = (data & 0x00ff00) >> 8;
>> + tx[3] = data & 0xff;
>> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
>> + if (ret)
>> + return -EIO;
>> +
>> + afe4403_toggle_ste(afe4403);
>> +
>> + /* Re-Enable reading from the device */
>> + tx[0] = AFE4403_CONTROL0;
>> + tx[1] = 0x0;
>> + tx[2] = 0x0;
>> + tx[3] = AFE4403_SPI_READ;
>> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
>> + if (ret)
>> + return -EIO;
>> +
>> + afe4403_toggle_ste(afe4403);
>> +
>> + return ret;
>> +};
>> +
>> +static int afe4403_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2, long mask)
>> +{
>> + struct afe4403_data *data = iio_priv(indio_dev);
>> + u8 reg;
>> +
>> + if (chan->channel > ARRAY_SIZE(afe4403_channels))
>> + return -EINVAL;
>> +
> check for channel < 5?
> check for val2 == 0
>
> this is not how to control iio channels
> can't _SCALE be used?
I will need to investigate this. The device needs to be calibrated and
these are the registers we need to write.
I did struggle a little with understanding how these things are put together.
>
>> + if (chan->channel == 5)
>> + reg = AFE4403_TIAGAIN;
>> + else if (chan->channel == 6)
>> + reg = AFE4403_TIA_AMB_GAIN;
>> + else if (chan->channel == 7)
>> + reg = AFE4403_LEDCNTRL;
>> + else if (chan->channel == 8)
>> + reg = AFE4403_CONTROL2;
>> + else
>> + return -EINVAL;
>> +
>> + return afe4403_write(data, reg, val);
>> +}
>> +
>> +static int afe4403_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct afe4403_data *data = iio_priv(indio_dev);
>> +
>> + if (iio_buffer_enabled(indio_dev))
>> + return -EBUSY;
>> +
>> + if (chan->channel > ARRAY_SIZE(afe4403_channels))
>> =
> and it is data_buffer[5]; 5 is smaller than ARRAY_SIZE(afe4003_channels)
It is meant to be smaller we only read and report 5 registers to the user space.
The other registers are written to during calibration.
>
>> + return -EINVAL;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_PROCESSED:
>> + *val = data->data_buffer[chan->channel];
>> + return IIO_VAL_INT;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int afe4403_write_event(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + int state)
>> +{
>> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
>> + int ret;
>> + int control_val;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
> _read() only read 3 bytes, leaving the remaining bytes of control_val
> uninitialized; control_val should be unsigned int
>
> wondering about endianness...
Fixed
>
>> + if (ret)
>> + return ret;
>> +
>> + printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN);
> no printk()s please
My bad. This was an artifact from debugging.
>
>> + if (state)
>> + control_val &= ~AFE4403_PWR_DWN;
>> + else
>> + control_val |= AFE4403_PWR_DWN;
>> +
>> + printk("***after control 0x%X\n", control_val);
>> +
>> + ret = afe4403_write(afe4403, AFE4403_CONTROL2, control_val);
>> + if (ret)
>> + return ret;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
>> + if (ret)
>> + return ret;
>> +
>> + printk("***control 0x%X\n", control_val);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct iio_info afe4403_iio_info = {
>> + .read_raw = &afe4403_read_raw,
>> + .write_raw = &afe4403_write_raw,
>> + .write_event_config = &afe4403_write_event,
>> + .driver_module = THIS_MODULE,
>> +};
>> +
>> +static irqreturn_t afe4403_event_handler(int irq, void *data)
>> +{
>> + struct iio_dev *indio_dev = data;
>> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
>> + int ret;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_LED1VAL, &afe4403->data_buffer[0]);
>> + if (ret)
>> + goto done;
> can't this be done all in one go?
I was thinking this I just need to adjust the read api in the code to take an argument
for the number of registers to read.
>
>> +
>> + ret = afe4403_read(afe4403, AFE4403_ALED1VAL, &afe4403->data_buffer[1]);
>> + if (ret)
>> + goto done;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_LED2VAL, &afe4403->data_buffer[2]);
>> + if (ret)
>> + goto done;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_ALED2VAL, &afe4403->data_buffer[3]);
>> + if (ret)
>> + goto done;
>> +
>> + ret = afe4403_read(afe4403, AFE4403_DIAG, &afe4403->data_buffer[4]);
>> + if (ret)
>> + goto done;
>> +
>> + iio_push_to_buffers_with_timestamp(indio_dev, &afe4403->data_buffer,
>> + iio_get_time_ns());
>> +done:
>> + return IRQ_HANDLED;
>> +}
>> +
>> +struct afe4403_reg {
>> + uint8_t reg;
>> + u32 value;
>> +} afe4403_init_regs[] = {
>> + { AFE4403_LED2STC, 0x0820},
>> + { AFE4403_LED2ENDC, 0x0f9e },
>> + { AFE4403_LED2LEDSTC, 0x07d0 },
>> + { AFE4403_LED2LEDENDC, 0x0f9f },
>> + { AFE4403_ALED2STC, 0x0050 },
>> + { AFE4403_ALED2ENDC, 0x07ce },
>> + { AFE4403_LED1STC, 0xc350 },
>> + { AFE4403_LED1ENDC, 0xc350 },
>> + { AFE4403_LED1LEDSTC, 0xc350 },
>> + { AFE4403_LED1LEDENDC, 0xc350 },
>> + { AFE4403_ALED1STC, 0x0ff0 },
>> + { AFE4403_ALED1ENDC, 0x176e },
>> + { AFE4403_LED2CONVST, 0x1775 },
>> + { AFE4403_LED2CONVEND, 0x1f3f },
>> + { AFE4403_ALED2CONVST, 0x1f45 },
>> + { AFE4403_ALED2CONVEND, 0x270f },
>> + { AFE4403_LED1CONVST, 0x2715 },
>> + { AFE4403_LED1CONVEND, 0x2edf },
>> + { AFE4403_ALED1CONVST, 0x2ee5 },
>> + { AFE4403_ALED1CONVEND, 0x36af },
>> + { AFE4403_ADCRSTSTCT0, 0x1770 },
>> + { AFE4403_ADCRSTENDCT0, 0x1774 },
>> + { AFE4403_ADCRSTSTCT1, 0x1f40 },
>> + { AFE4403_ADCRSTENDCT1, 0x1f44 },
>> + { AFE4403_ADCRSTSTCT2, 0x2710 },
>> + { AFE4403_ADCRSTENDCT2, 0x2714 },
>> + { AFE4403_ADCRSTSTCT3, 0x2ee0 },
>> + { AFE4403_ADCRSTENDCT3, 0x2ee4 },
>> + { AFE4403_PRPCOUNT, 0x09c3f },
>> + { AFE4403_CONTROL1, 0x0107 },
>> + { AFE4403_TIAGAIN, 0x8006 },
>> + { AFE4403_TIA_AMB_GAIN, 0x06 },
>> + { AFE4403_LEDCNTRL, 0x11414 },
>> + { AFE4403_CONTROL2, 0x20000 },
>> +};
>> +
>> +static int afe4403_init(struct afe4403_data *afe4403)
>> +{
>> + int ret, i, reg_count;
>> +
>> + /* Hard reset the device */
>> + if (afe4403->reset_gpio) {
>> + gpiod_set_value(afe4403->reset_gpio, 0);
>> + udelay(800);
>> + gpiod_set_value(afe4403->reset_gpio, 1);
>> + } else {
>> + /* Soft reset the device */
>> + ret = afe4403_write(afe4403, AFE4403_CONTROL0, AFE4403_SW_RESET);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + udelay(800);
>> +
>> + reg_count = sizeof(afe4403_init_regs) / sizeof(afe4403_init_regs[0]);
> ARRAY_SIZE()
Fixed
>
>> + for (i = 0; i < reg_count; i++) {
>> + ret = afe4403_write(afe4403, afe4403_init_regs[i].reg,
>> + afe4403_init_regs[i].value);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return ret;
>> +};
>> +
>> +static int afe4403_iio_buffered_hardware_setup(struct iio_dev *indio_dev)
>> +{
>> + struct iio_buffer *buffer;
>> + int ret;
>> +
>> + buffer = iio_kfifo_allocate(indio_dev);
>> + if (!buffer)
>> + return -ENOMEM;
>> +
>> + iio_device_attach_buffer(indio_dev, buffer);
>> +
>> + ret = iio_buffer_register(indio_dev,
>> + indio_dev->channels,
>> + indio_dev->num_channels);
>> + if (ret)
>> + goto error_kfifo_free;
>> +
>> + return 0;
>> +
>> +error_kfifo_free:
>> + iio_kfifo_free(indio_dev->buffer);
>> + return ret;
>> +}
>> +
>> +static int afe4403_spi_probe(struct spi_device *spi)
>> +{
>> + struct afe4403_data *afe4403;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*afe4403));
>> + if (indio_dev == NULL) {
>> + dev_err(&spi->dev, "Failed to allocate iio device\n");
>> + return -ENOMEM;
>> + }
>> +
>> + spi_set_drvdata(spi, indio_dev);
>> +
>> + afe4403 = iio_priv(indio_dev);
>> + afe4403->spi = spi;
>> +
>> + ret = iio_map_array_register(indio_dev, afe4403_default_iio_maps);
>> + if (ret) {
>> + dev_err(&indio_dev->dev, "iio map err: %d\n", ret);
>> + return ret;
>> + }
>> + afe4403->map = afe4403_default_iio_maps;
>> +
>> + indio_dev->dev.parent = &spi->dev;
>> + indio_dev->name = "afe4403";
>> + indio_dev->info = &afe4403_iio_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;
>> + indio_dev->channels = afe4403_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(afe4403_channels);
>> +
>> + afe4403->ste_gpio = devm_gpiod_get(&spi->dev, "ste");
>> + if (IS_ERR(afe4403->ste_gpio)) {
>> + ret = PTR_ERR(afe4403->ste_gpio);
>> + if (ret != -ENOENT && ret != -ENOSYS) {
>> + dev_err(&spi->dev, "Failed to allocate spi gpio\n");
>> + return ret;
>> + }
>> + afe4403->ste_gpio = NULL;
>> + } else {
>> + gpiod_direction_output(afe4403->ste_gpio, 0);
>> + }
>> +
>> + afe4403->data_gpio = devm_gpiod_get(&spi->dev, "data-ready");
>> + if (IS_ERR(afe4403->data_gpio)) {
>> + ret = PTR_ERR(afe4403->data_gpio);
>> + if (ret != -ENOENT && ret != -ENOSYS) {
>> + dev_err(&spi->dev, "Failed to allocate data_ready gpio\n");
>> + return ret;
>> + }
>> + afe4403->data_gpio = NULL;
>> + } else {
>> + gpiod_direction_input(afe4403->data_gpio);
>> + afe4403->irq = gpiod_to_irq(afe4403->data_gpio);
>> + }
>> +
>> + afe4403->reset_gpio = devm_gpiod_get(&spi->dev, "reset");
>> + if (IS_ERR(afe4403->reset_gpio)) {
>> + ret = PTR_ERR(afe4403->reset_gpio);
>> + if (ret != -ENOENT && ret != -ENOSYS) {
>> + dev_err(&spi->dev, "Failed to allocate reset gpio\n");
>> + return ret;
>> + }
>> + afe4403->reset_gpio = NULL;
>> + } else {
>> + gpiod_direction_output(afe4403->reset_gpio, 1);
>> + }
>> +
>> + ret = afe4403_iio_buffered_hardware_setup(indio_dev);
>> + if (ret) {
>> + dev_err(&spi->dev, "Failed to allocate iio buffers %i\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = devm_request_threaded_irq(&spi->dev, afe4403->irq,
>> + NULL,
>> + afe4403_event_handler,
>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> + "afe4403 int",
>> + indio_dev);
>> + if (ret) {
>> + dev_err(&spi->dev, "Failed to allocate thread %i\n", ret);
> threaded irq?
Yes. What is the objection? Would there be a preference that I read the data
from the IC in an IRQ context?
>
>> + return ret;
>> + }
>> +
>> + ret = devm_iio_device_register(&spi->dev, indio_dev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = afe4403_init(afe4403);
>> + if (ret) {
>> + dev_err(&spi->dev, "Failed to init device\n");
> and now what? no cleanup?
What is it that needs cleanup? I was careful to use devm calls which should self clean up on
probe failure.
Since the init is all spi writes should the spi write fail then the probe fails and all
subsequent devm calls self cleanup.
The only call made that is not devm is
iio_map_array_register
>
>> + return ret;
>> + }
>> +
>> + return 0;
>> +
>> +}
>> +
>> +static int afe4403_spi_remove(struct spi_device *spi)
>> +{
>> + struct afe4403_data *afe4403 = dev_get_drvdata(&spi->dev);
>> +
>> + iio_kfifo_free(afe4403->indio_dev->buffer);
>> + iio_buffer_unregister(afe4403->indio_dev);
>> +
>> + if (!IS_ERR(afe4403->regulator))
>> + regulator_disable(afe4403->regulator);
> regulator never enabled?
Fixed
>
>> +
>> + return 0;
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_OF)
>> +static const struct of_device_id afe4403_of_match[] = {
>> + { .compatible = "ti,afe4403", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, afe4403_of_match);
>> +#endif
>> +
>> +static struct spi_driver afe4403_spi_driver = {
>> + .driver = {
>> + .name = "afe4403",
>> + .owner = THIS_MODULE,
>> + .of_match_table = of_match_ptr(afe4403_of_match),
>> + },
>> + .probe = afe4403_spi_probe,
>> + .remove = afe4403_spi_remove,
>> +};
>> +
>> +static int __init afe4403_spi_init(void)
>> +{
>> + return spi_register_driver(&afe4403_spi_driver);
>> +}
>> +module_init(afe4403_spi_init);
>> +
>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
>> +MODULE_DESCRIPTION("TI AFE4403 Heart Rate and Pulse Oximeter");
>> +MODULE_LICENSE("GPL");
>>
--
------------------
Dan Murphy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor
2014-10-22 15:36 ` Dan Murphy
@ 2014-10-25 10:13 ` Jonathan Cameron
0 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2014-10-25 10:13 UTC (permalink / raw)
To: Dan Murphy, Peter Meerwald; +Cc: linux-iio
On 22/10/14 16:36, Dan Murphy wrote:
> Peter
> Sorry for the delayed reply I was working on the code and comments
>
> On 10/15/2014 04:52 AM, Peter Meerwald wrote:
>>> Add the TI afe4403 heart rate monitor.
>>> This device detects reflected LED
>>> wave length fluctuations and presents an ADC
>>> value to the user space to be converted to a
>>> heart rate.
>>> Data sheet located here:
>>> http://www.ti.com/product/AFE4403/datasheet
>> s/heart_monitor/heart_monitors
>>
>> you need to update Documentation/ABI/testing/sysfs-bus-iio
>>
>> more minor comments below
>
> Will update
>
>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>> drivers/iio/Kconfig | 1 +
>>> drivers/iio/Makefile | 1 +
>>> drivers/iio/heart_monitors/Kconfig | 16 +
>>> drivers/iio/heart_monitors/Makefile | 6 +
>>> drivers/iio/heart_monitors/afe4403.c | 610 ++++++++++++++++++++++++++++++++++
>>> 5 files changed, 634 insertions(+)
>>> create mode 100644 drivers/iio/heart_monitors/Kconfig
>>> create mode 100644 drivers/iio/heart_monitors/Makefile
>>> create mode 100644 drivers/iio/heart_monitors/afe4403.c
>>>
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 345395e..e3a7945 100644
>>> --- a/drivers/iio/Kconfig
>>> +++ b/drivers/iio/Kconfig
>>> @@ -66,6 +66,7 @@ source "drivers/iio/common/Kconfig"
>>> source "drivers/iio/dac/Kconfig"
>>> source "drivers/iio/frequency/Kconfig"
>>> source "drivers/iio/gyro/Kconfig"
>>> +source "drivers/iio/heart_monitors/Kconfig"
>> heartrate would be my preference to name the category
>> (feel free to ignore)
>>
>> and use heartrate / HEARTRATE consistently, not HEART_, not HEARTBEAT_
>
> Noted but not changed unless there are are other objections
My only thought here is that we might at somepoint get more sophisticated
heart monitoring devices (uneven pulses or some such) but honestly
I'm not that fussed either way (both are clear to my mind).
The type needs to be heartrate but the directory etc can be more general.
>
>>
>>> source "drivers/iio/humidity/Kconfig"
>>> source "drivers/iio/imu/Kconfig"
>>> source "drivers/iio/light/Kconfig"
>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>> index 698afc2..46c0960 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -18,6 +18,7 @@ obj-y += common/
>>> obj-y += dac/
>>> obj-y += gyro/
>>> obj-y += frequency/
>>> +obj-y += heart_monitors/
>>> obj-y += humidity/
>>> obj-y += imu/
>>> obj-y += light/
>>> diff --git a/drivers/iio/heart_monitors/Kconfig b/drivers/iio/heart_monitors/Kconfig
>>> new file mode 100644
>>> index 0000000..7060694
>>> --- /dev/null
>>> +++ b/drivers/iio/heart_monitors/Kconfig
>>> @@ -0,0 +1,16 @@
>>> +#
>>> +# Heart Rate Monitor drivers
>>> +#
>>> +# When adding new entries keep the list in alphabetical order
>>> +
>>> +menu "Heart Rate Monitors"
>>> +
>>> +config AFE4403
>>> + tristate "TI AFE4403 Heart Rate Monitor"
>>> + depends on SPI
>>> + select IIO_BUFFER
>>> + select IIO_TRIGGERED_BUFFER
>>> + select SYSFS
>> other drivers 'depend' on SYSFS rather than 'select', and use SPI_MASTER
>
> Fixed
>
>>
>>> + help
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/heart_monitors/Makefile b/drivers/iio/heart_monitors/Makefile
>>> new file mode 100644
>>> index 0000000..77cc5c5
>>> --- /dev/null
>>> +++ b/drivers/iio/heart_monitors/Makefile
>>> @@ -0,0 +1,6 @@
>>> +#
>>> +# Makefile for IIO Heart Rate Monitor drivers
>>> +#
>>> +
>>> +# When adding new entries keep the list in alphabetical order
>>> +obj-$(CONFIG_AFE4403) += afe4403.o
>>> diff --git a/drivers/iio/heart_monitors/afe4403.c b/drivers/iio/heart_monitors/afe4403.c
>>> new file mode 100644
>>> index 0000000..d285769
>>> --- /dev/null
>>> +++ b/drivers/iio/heart_monitors/afe4403.c
>>> @@ -0,0 +1,610 @@
>>> +/*
>>> + * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
>>> + *
>>> + * Author: Dan Murphy <dmurphy@ti.com>
>>> + *
>>> + * Copyright: (C) 2014 Texas Instruments, Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * 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/device.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/err.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/sysfs.h>
>>> +#include <linux/regulator/consumer.h>
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/driver.h>
>>> +#include <linux/iio/machine.h>
>>> +#include <linux/iio/events.h>
>>> +#include <linux/iio/kfifo_buf.h>
>>> +
>>> +#define AFE4403_CONTROL0 0x00
>>> +#define AFE4403_LED2STC 0x01
>>> +#define AFE4403_LED2ENDC 0x02
>>> +#define AFE4403_LED2LEDSTC 0x03
>>> +#define AFE4403_LED2LEDENDC 0x04
>>> +#define AFE4403_ALED2STC 0x05
>>> +#define AFE4403_ALED2ENDC 0x06
>>> +#define AFE4403_LED1STC 0x07
>>> +#define AFE4403_LED1ENDC 0x08
>>> +#define AFE4403_LED1LEDSTC 0x09
>>> +#define AFE4403_LED1LEDENDC 0x0a
>>> +#define AFE4403_ALED1STC 0x0b
>>> +#define AFE4403_ALED1ENDC 0x0c
>>> +#define AFE4403_LED2CONVST 0x0d
>>> +#define AFE4403_LED2CONVEND 0x0e
>>> +#define AFE4403_ALED2CONVST 0x0f
>>> +#define AFE4403_ALED2CONVEND 0x10
>>> +#define AFE4403_LED1CONVST 0x11
>>> +#define AFE4403_LED1CONVEND 0x12
>>> +#define AFE4403_ALED1CONVST 0x13
>>> +#define AFE4403_ALED1CONVEND 0x14
>>> +#define AFE4403_ADCRSTSTCT0 0x15
>>> +#define AFE4403_ADCRSTENDCT0 0x16
>>> +#define AFE4403_ADCRSTSTCT1 0x17
>>> +#define AFE4403_ADCRSTENDCT1 0x18
>>> +#define AFE4403_ADCRSTSTCT2 0x19
>>> +#define AFE4403_ADCRSTENDCT2 0x1a
>>> +#define AFE4403_ADCRSTSTCT3 0x1b
>>> +#define AFE4403_ADCRSTENDCT3 0x1c
>>> +#define AFE4403_PRPCOUNT 0x1d
>>> +#define AFE4403_CONTROL1 0x1e
>>> +#define AFE4403_SPARE1 0x1f
>>> +#define AFE4403_TIAGAIN 0x20
>>> +#define AFE4403_TIA_AMB_GAIN 0x21
>>> +#define AFE4403_LEDCNTRL 0x22
>>> +#define AFE4403_CONTROL2 0x23
>>> +#define AFE4403_SPARE2 0x24
>>> +#define AFE4403_SPARE3 0x25
>>> +#define AFE4403_SPARE4 0x26
>>> +#define AFE4403_ALARM 0x29
>>> +#define AFE4403_LED2VAL 0x2A
>>> +#define AFE4403_ALED2VAL 0x2B
>>> +#define AFE4403_LED1VAL 0x2C
>>> +#define AFE4403_ALED1VAL 0x2D
>>> +#define AFE4403_LED2_ALED2VAL 0x2E
>>> +#define AFE4403_LED1_ALED1VAL 0x2F
>>> +#define AFE4403_DIAG 0x30
>>> +#define AFE4403_CONTROL3 0x31
>>> +#define AFE4403_PDNCYCLESTC 0x32
>>> +#define AFE4403_PDNCYCLEENDC 0x33
>>> +
>>> +#define AFE4403_SPI_READ (1 << 0)
>>> +#define AFE4403_SW_RESET (1 << 3)
>>> +#define AFE4403_PWR_DWN (1 << 0)
>> can use BIT()
>
> Fixed. I need to update these defines per the data sheet to get rid of the
> magic numbers below.
>
> I am wondering if a header file may be in order thoughts?
Not unless you are ending up with multiple c files. Kernel
style is definitely moving towards as few headers as possible!
>
>>
>>> +
>>> +/**
>>> + * struct afe4403_data -
>>> + * @work - Work item used to off load the enable/disable of the vibration
>>> + * @indio_dev -
>>> + * @map -
>>> + * @spi -
>>> + * @regulator - Pointer to the regulator for the IC
>>> + * @ste_gpio -
>>> + * @data_gpio -
>>> + * @reset_gpio -
>>> + * @data_buffer -
>>> + * @irq - AFE4403 interrupt number
>>> +**/
>>> +struct afe4403_data {
>>> + struct work_struct work;
>>> + struct iio_dev *indio_dev;
>>> + struct iio_map *map;
>>> + struct spi_device *spi;
>>> + struct regulator *regulator;
>>> + struct gpio_desc *ste_gpio;
>>> + struct gpio_desc *data_gpio;
>>> + struct gpio_desc *reset_gpio;
>>> + int data_buffer[5];
>> magic 5
>
> I can #define this but this is the number of registers we read and report from the IC
>
>>
>>> + int irq;
>>> +};
>>> +
>>> +enum afe4403_reg_id {
>>> + LED1VAL,
>>> + ALED1VAL,
>>> + LED2VAL,
>>> + ALED2VAL,
>>> + DIAG,
>>> + TIAGAIN,
>>> + TIA_AMB_GAIN,
>>> + LEDCNTRL,
>>> + CONTROL3,
>>> +};
>>> +
>>> +static const struct iio_event_spec afe4403_events[] = {
>>> + {
>>> + .type = IIO_EV_TYPE_MAG,
>>> + .dir = IIO_EV_DIR_EITHER,
>>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>>> + BIT(IIO_EV_INFO_ENABLE),
>>> + },
>>> +};
>>> +
>>> +#define AFE4403_COMMON_CHAN(index, name) \
>>> + { \
>>> + .type = IIO_HEARTRATE, \
>>> + .indexed = 1, \
>>> + .channel = index, \
>>> + .datasheet_name = name, \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
>>> + .scan_index = index + 1, \
>> could do (index), but doesn't matter
>> why +1?
>
> Fixed
>
>>
>>> + .scan_type = { \
>>> + .sign = 'u', \
>>> + .realbits = 24, \
>> top 2 bits are ignored
>
> Not sure what you mean here. How are the top two bits ignored?
>
>>
>>> + .storagebits = 24, \
>> .endianness = IIO_BE
>
> Fixed
>
>>
>>> + }, \
>>> + .event_spec = afe4403_events, \
>>> + .num_event_specs = ARRAY_SIZE(afe4403_events), \
>>> + }
>>> +
>>> +static const struct iio_chan_spec afe4403_channels[] = {
>>> + /* Read only values from the IC */
>>> + [LED1VAL] = AFE4403_COMMON_CHAN(0, "LED1VAL"),
>>> + [ALED1VAL] = AFE4403_COMMON_CHAN(1, "ALED1VAL"),
>>> + [LED2VAL] = AFE4403_COMMON_CHAN(2, "LED2VAL"),
>>> + [ALED1VAL] = AFE4403_COMMON_CHAN(3, "ALED2VAL"),
>>> + [DIAG] = AFE4403_COMMON_CHAN(4, "DIAG"),
>>> +
>>> + /* Required writing for calibration */
>>> + [TIAGAIN] = AFE4403_COMMON_CHAN(5, "TIAGAIN"),
>>> + [TIA_AMB_GAIN] = AFE4403_COMMON_CHAN(6, "TIA_AMB_GAIN"),
>>> + [LEDCNTRL] = AFE4403_COMMON_CHAN(7, "LEDCNTRL"),
>>> + [CONTROL3] = AFE4403_COMMON_CHAN(8, "CONTROL3"),
>>> +};
>>> +
>>> +static struct iio_map afe4403_default_iio_maps[] = {
>>> + {
>>> + .consumer_dev_name = "afe4403_heartrate",
>>> + .consumer_channel = "afe4403_led1value",
>>> + .adc_channel_label = "LED1VAL",
>>> + },
>>> + {
>>> + .consumer_dev_name = "afe4403_heartrate",
>>> + .consumer_channel = "afe4403_aled1value",
>>> + .adc_channel_label = "ALED1VAL",
>>> + },
>>> + {
>>> + .consumer_dev_name = "afe4403_heartrate",
>>> + .consumer_channel = "afe4403_led2value",
>>> + .adc_channel_label = "LED2VAL",
>>> + },
>>> + {
>>> + .consumer_dev_name = "afe4403_heartrate",
>>> + .consumer_channel = "afe4403_aled2value",
>>> + .adc_channel_label = "ALED2VAL",
>>> + },
>>> + {
>>> + .consumer_dev_name = "afe4403_heartrate",
>>> + .consumer_channel = "afe4403_diagnostics",
>>> + .adc_channel_label = "DIAG",
>>> + },
>>> +};
>>> +
>>> +static void afe4403_toggle_ste(struct afe4403_data *afe4403)
>>> +{
>>> + gpiod_set_value(afe4403->ste_gpio, 1);
>>> + udelay(800);
>>> + gpiod_set_value(afe4403->ste_gpio, 0);
>>> +}
>>> +
>>> +static int afe4403_read(struct afe4403_data *afe4403, u8 reg,
>>> + unsigned int *data)
>>> +{
>>> + int ret;
>>> +
>>> + ret = spi_write_then_read(afe4403->spi, (u8 *)®, 1, (u8 *)data, 3);
>>> + if (ret)
>>> + return -EIO;
>>> +
>>> + afe4403_toggle_ste(afe4403);
>>> +
>>> + return ret;
>>> +};
>>> +
>>> +static int afe4403_write(struct afe4403_data *afe4403, u8 reg,
>>> + unsigned int data)
>>> +{
>>> + int ret;
>>> + u8 tx[4] = {0x0, 0x0, 0x0, 0x0};
>>> +
>>> + /* Enable write to the device */
>>> +
>>> + tx[0] = AFE4403_CONTROL0;
>>> + tx[3] = 0x0;
>>> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
>>> + if (ret)
>>> + return -EIO;
>>> +
>>> + afe4403_toggle_ste(afe4403);
>>> +
>>> + tx[0] = reg;
>>> + tx[1] = (data & 0x0f0000) >> 16;
>>> + tx[2] = (data & 0x00ff00) >> 8;
>>> + tx[3] = data & 0xff;
>>> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
>>> + if (ret)
>>> + return -EIO;
>>> +
>>> + afe4403_toggle_ste(afe4403);
>>> +
>>> + /* Re-Enable reading from the device */
>>> + tx[0] = AFE4403_CONTROL0;
>>> + tx[1] = 0x0;
>>> + tx[2] = 0x0;
>>> + tx[3] = AFE4403_SPI_READ;
>>> + ret = spi_write(afe4403->spi, (u8 *)tx, 4);
>>> + if (ret)
>>> + return -EIO;
>>> +
>>> + afe4403_toggle_ste(afe4403);
>>> +
>>> + return ret;
>>> +};
>>> +
>>> +static int afe4403_write_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int val, int val2, long mask)
>>> +{
>>> + struct afe4403_data *data = iio_priv(indio_dev);
>>> + u8 reg;
>>> +
>>> + if (chan->channel > ARRAY_SIZE(afe4403_channels))
>>> + return -EINVAL;
>>> +
>> check for channel < 5?
>> check for val2 == 0
>>
>> this is not how to control iio channels
>> can't _SCALE be used?
>
> I will need to investigate this. The device needs to be calibrated and
> these are the registers we need to write.
>
> I did struggle a little with understanding how these things are put together.
Basically have scale if it is applied post driver (e.g. apply to the output
of the driver to get real world units) and calibscale if applied internally
(e.g. is a trim control on the hardware or perhaps a trim in the driver -
though the in driver option is normally only done for channels with complex
conversions from the raw reading to real world units)
>
>>
>>> + if (chan->channel == 5)
>>> + reg = AFE4403_TIAGAIN;
>>> + else if (chan->channel == 6)
>>> + reg = AFE4403_TIA_AMB_GAIN;
>>> + else if (chan->channel == 7)
>>> + reg = AFE4403_LEDCNTRL;
>>> + else if (chan->channel == 8)
>>> + reg = AFE4403_CONTROL2;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + return afe4403_write(data, reg, val);
>>> +}
>>> +
>>> +static int afe4403_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct afe4403_data *data = iio_priv(indio_dev);
>>> +
>>> + if (iio_buffer_enabled(indio_dev))
>>> + return -EBUSY;
>>> +
>>> + if (chan->channel > ARRAY_SIZE(afe4403_channels))
>>> =
>> and it is data_buffer[5]; 5 is smaller than ARRAY_SIZE(afe4003_channels)
>
> It is meant to be smaller we only read and report 5 registers to the user space.
> The other registers are written to during calibration.
>
>>
>>> + return -EINVAL;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_PROCESSED:
>>> + *val = data->data_buffer[chan->channel];
>>> + return IIO_VAL_INT;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +static int afe4403_write_event(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + enum iio_event_type type,
>>> + enum iio_event_direction dir,
>>> + int state)
>>> +{
>>> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
>>> + int ret;
>>> + int control_val;
>>> +
>>> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
>> _read() only read 3 bytes, leaving the remaining bytes of control_val
>> uninitialized; control_val should be unsigned int
>>
>> wondering about endianness...
>
> Fixed
>
>>
>>> + if (ret)
>>> + return ret;
>>> +
>>> + printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN);
>> no printk()s please
>
> My bad. This was an artifact from debugging.
>
>>
>>> + if (state)
>>> + control_val &= ~AFE4403_PWR_DWN;
>>> + else
>>> + control_val |= AFE4403_PWR_DWN;
>>> +
>>> + printk("***after control 0x%X\n", control_val);
>>> +
>>> + ret = afe4403_write(afe4403, AFE4403_CONTROL2, control_val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + printk("***control 0x%X\n", control_val);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct iio_info afe4403_iio_info = {
>>> + .read_raw = &afe4403_read_raw,
>>> + .write_raw = &afe4403_write_raw,
>>> + .write_event_config = &afe4403_write_event,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static irqreturn_t afe4403_event_handler(int irq, void *data)
>>> +{
>>> + struct iio_dev *indio_dev = data;
>>> + struct afe4403_data *afe4403 = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + ret = afe4403_read(afe4403, AFE4403_LED1VAL, &afe4403->data_buffer[0]);
>>> + if (ret)
>>> + goto done;
>> can't this be done all in one go?
>
> I was thinking this I just need to adjust the read api in the code to take an argument
> for the number of registers to read.
>
>>
>>> +
>>> + ret = afe4403_read(afe4403, AFE4403_ALED1VAL, &afe4403->data_buffer[1]);
>>> + if (ret)
>>> + goto done;
>>> +
>>> + ret = afe4403_read(afe4403, AFE4403_LED2VAL, &afe4403->data_buffer[2]);
>>> + if (ret)
>>> + goto done;
>>> +
>>> + ret = afe4403_read(afe4403, AFE4403_ALED2VAL, &afe4403->data_buffer[3]);
>>> + if (ret)
>>> + goto done;
>>> +
>>> + ret = afe4403_read(afe4403, AFE4403_DIAG, &afe4403->data_buffer[4]);
>>> + if (ret)
>>> + goto done;
>>> +
>>> + iio_push_to_buffers_with_timestamp(indio_dev, &afe4403->data_buffer,
>>> + iio_get_time_ns());
>>> +done:
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +struct afe4403_reg {
>>> + uint8_t reg;
>>> + u32 value;
>>> +} afe4403_init_regs[] = {
>>> + { AFE4403_LED2STC, 0x0820},
>>> + { AFE4403_LED2ENDC, 0x0f9e },
>>> + { AFE4403_LED2LEDSTC, 0x07d0 },
>>> + { AFE4403_LED2LEDENDC, 0x0f9f },
>>> + { AFE4403_ALED2STC, 0x0050 },
>>> + { AFE4403_ALED2ENDC, 0x07ce },
>>> + { AFE4403_LED1STC, 0xc350 },
>>> + { AFE4403_LED1ENDC, 0xc350 },
>>> + { AFE4403_LED1LEDSTC, 0xc350 },
>>> + { AFE4403_LED1LEDENDC, 0xc350 },
>>> + { AFE4403_ALED1STC, 0x0ff0 },
>>> + { AFE4403_ALED1ENDC, 0x176e },
>>> + { AFE4403_LED2CONVST, 0x1775 },
>>> + { AFE4403_LED2CONVEND, 0x1f3f },
>>> + { AFE4403_ALED2CONVST, 0x1f45 },
>>> + { AFE4403_ALED2CONVEND, 0x270f },
>>> + { AFE4403_LED1CONVST, 0x2715 },
>>> + { AFE4403_LED1CONVEND, 0x2edf },
>>> + { AFE4403_ALED1CONVST, 0x2ee5 },
>>> + { AFE4403_ALED1CONVEND, 0x36af },
>>> + { AFE4403_ADCRSTSTCT0, 0x1770 },
>>> + { AFE4403_ADCRSTENDCT0, 0x1774 },
>>> + { AFE4403_ADCRSTSTCT1, 0x1f40 },
>>> + { AFE4403_ADCRSTENDCT1, 0x1f44 },
>>> + { AFE4403_ADCRSTSTCT2, 0x2710 },
>>> + { AFE4403_ADCRSTENDCT2, 0x2714 },
>>> + { AFE4403_ADCRSTSTCT3, 0x2ee0 },
>>> + { AFE4403_ADCRSTENDCT3, 0x2ee4 },
>>> + { AFE4403_PRPCOUNT, 0x09c3f },
>>> + { AFE4403_CONTROL1, 0x0107 },
>>> + { AFE4403_TIAGAIN, 0x8006 },
>>> + { AFE4403_TIA_AMB_GAIN, 0x06 },
>>> + { AFE4403_LEDCNTRL, 0x11414 },
>>> + { AFE4403_CONTROL2, 0x20000 },
>>> +};
>>> +
>>> +static int afe4403_init(struct afe4403_data *afe4403)
>>> +{
>>> + int ret, i, reg_count;
>>> +
>>> + /* Hard reset the device */
>>> + if (afe4403->reset_gpio) {
>>> + gpiod_set_value(afe4403->reset_gpio, 0);
>>> + udelay(800);
>>> + gpiod_set_value(afe4403->reset_gpio, 1);
>>> + } else {
>>> + /* Soft reset the device */
>>> + ret = afe4403_write(afe4403, AFE4403_CONTROL0, AFE4403_SW_RESET);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + udelay(800);
>>> +
>>> + reg_count = sizeof(afe4403_init_regs) / sizeof(afe4403_init_regs[0]);
>> ARRAY_SIZE()
>
> Fixed
>
>>
>>> + for (i = 0; i < reg_count; i++) {
>>> + ret = afe4403_write(afe4403, afe4403_init_regs[i].reg,
>>> + afe4403_init_regs[i].value);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + return ret;
>>> +};
>>> +
>>> +static int afe4403_iio_buffered_hardware_setup(struct iio_dev *indio_dev)
>>> +{
>>> + struct iio_buffer *buffer;
>>> + int ret;
>>> +
>>> + buffer = iio_kfifo_allocate(indio_dev);
>>> + if (!buffer)
>>> + return -ENOMEM;
>>> +
>>> + iio_device_attach_buffer(indio_dev, buffer);
>>> +
>>> + ret = iio_buffer_register(indio_dev,
>>> + indio_dev->channels,
>>> + indio_dev->num_channels);
>>> + if (ret)
>>> + goto error_kfifo_free;
>>> +
>>> + return 0;
>>> +
>>> +error_kfifo_free:
>>> + iio_kfifo_free(indio_dev->buffer);
>>> + return ret;
>>> +}
>>> +
>>> +static int afe4403_spi_probe(struct spi_device *spi)
>>> +{
>>> + struct afe4403_data *afe4403;
>>> + struct iio_dev *indio_dev;
>>> + int ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*afe4403));
>>> + if (indio_dev == NULL) {
>>> + dev_err(&spi->dev, "Failed to allocate iio device\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + spi_set_drvdata(spi, indio_dev);
>>> +
>>> + afe4403 = iio_priv(indio_dev);
>>> + afe4403->spi = spi;
>>> +
>>> + ret = iio_map_array_register(indio_dev, afe4403_default_iio_maps);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "iio map err: %d\n", ret);
>>> + return ret;
>>> + }
>>> + afe4403->map = afe4403_default_iio_maps;
>>> +
>>> + indio_dev->dev.parent = &spi->dev;
>>> + indio_dev->name = "afe4403";
>>> + indio_dev->info = &afe4403_iio_info;
>>> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;
>>> + indio_dev->channels = afe4403_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(afe4403_channels);
>>> +
>>> + afe4403->ste_gpio = devm_gpiod_get(&spi->dev, "ste");
>>> + if (IS_ERR(afe4403->ste_gpio)) {
>>> + ret = PTR_ERR(afe4403->ste_gpio);
>>> + if (ret != -ENOENT && ret != -ENOSYS) {
>>> + dev_err(&spi->dev, "Failed to allocate spi gpio\n");
>>> + return ret;
>>> + }
>>> + afe4403->ste_gpio = NULL;
>>> + } else {
>>> + gpiod_direction_output(afe4403->ste_gpio, 0);
>>> + }
>>> +
>>> + afe4403->data_gpio = devm_gpiod_get(&spi->dev, "data-ready");
>>> + if (IS_ERR(afe4403->data_gpio)) {
>>> + ret = PTR_ERR(afe4403->data_gpio);
>>> + if (ret != -ENOENT && ret != -ENOSYS) {
>>> + dev_err(&spi->dev, "Failed to allocate data_ready gpio\n");
>>> + return ret;
>>> + }
>>> + afe4403->data_gpio = NULL;
>>> + } else {
>>> + gpiod_direction_input(afe4403->data_gpio);
>>> + afe4403->irq = gpiod_to_irq(afe4403->data_gpio);
>>> + }
>>> +
>>> + afe4403->reset_gpio = devm_gpiod_get(&spi->dev, "reset");
>>> + if (IS_ERR(afe4403->reset_gpio)) {
>>> + ret = PTR_ERR(afe4403->reset_gpio);
>>> + if (ret != -ENOENT && ret != -ENOSYS) {
>>> + dev_err(&spi->dev, "Failed to allocate reset gpio\n");
>>> + return ret;
>>> + }
>>> + afe4403->reset_gpio = NULL;
>>> + } else {
>>> + gpiod_direction_output(afe4403->reset_gpio, 1);
>>> + }
>>> +
>>> + ret = afe4403_iio_buffered_hardware_setup(indio_dev);
>>> + if (ret) {
>>> + dev_err(&spi->dev, "Failed to allocate iio buffers %i\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = devm_request_threaded_irq(&spi->dev, afe4403->irq,
>>> + NULL,
>>> + afe4403_event_handler,
>>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> + "afe4403 int",
>>> + indio_dev);
>>> + if (ret) {
>>> + dev_err(&spi->dev, "Failed to allocate thread %i\n", ret);
>> threaded irq?
>
> Yes. What is the objection? Would there be a preference that I read the data
> from the IC in an IRQ context?
Comment is on the error message - not that you used a threaded_irq
(which is probably correct!)
>
>>
>>> + return ret;
>>> + }
>>> +
>>> + ret = devm_iio_device_register(&spi->dev, indio_dev);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = afe4403_init(afe4403);
>>> + if (ret) {
>>> + dev_err(&spi->dev, "Failed to init device\n");
>> and now what? no cleanup?
>
> What is it that needs cleanup? I was careful to use devm calls which should self clean up on
> probe failure.
>
> Since the init is all spi writes should the spi write fail then the probe fails and all
> subsequent devm calls self cleanup.
>
> The only call made that is not devm is
>
> iio_map_array_register
Then that needs unwinding ;)
>
>
>>
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +}
>>> +
>>> +static int afe4403_spi_remove(struct spi_device *spi)
>>> +{
>>> + struct afe4403_data *afe4403 = dev_get_drvdata(&spi->dev);
>>> +
>>> + iio_kfifo_free(afe4403->indio_dev->buffer);
>>> + iio_buffer_unregister(afe4403->indio_dev);
>>> +
>>> + if (!IS_ERR(afe4403->regulator))
>>> + regulator_disable(afe4403->regulator);
>> regulator never enabled?
>
> Fixed
>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#if IS_ENABLED(CONFIG_OF)
>>> +static const struct of_device_id afe4403_of_match[] = {
>>> + { .compatible = "ti,afe4403", },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, afe4403_of_match);
>>> +#endif
>>> +
>>> +static struct spi_driver afe4403_spi_driver = {
>>> + .driver = {
>>> + .name = "afe4403",
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = of_match_ptr(afe4403_of_match),
>>> + },
>>> + .probe = afe4403_spi_probe,
>>> + .remove = afe4403_spi_remove,
>>> +};
>>> +
>>> +static int __init afe4403_spi_init(void)
>>> +{
>>> + return spi_register_driver(&afe4403_spi_driver);
>>> +}
>>> +module_init(afe4403_spi_init);
>>> +
>>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
>>> +MODULE_DESCRIPTION("TI AFE4403 Heart Rate and Pulse Oximeter");
>>> +MODULE_LICENSE("GPL");
>>>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-10-25 18:31 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-14 19:13 RFC Introducing Heart Monitors for IIO Dan Murphy
2014-10-14 19:13 ` [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors Dan Murphy
2014-10-15 7:23 ` Daniel Baluta
2014-10-15 19:35 ` Jonathan Cameron
2014-10-14 19:13 ` [RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation Dan Murphy
2014-10-15 9:52 ` Peter Meerwald
2014-10-14 19:13 ` [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor Dan Murphy
2014-10-15 7:35 ` Daniel Baluta
2014-10-15 17:37 ` Dan Murphy
2014-10-15 9:52 ` Peter Meerwald
2014-10-22 15:36 ` Dan Murphy
2014-10-25 10:13 ` Jonathan Cameron
2014-10-15 7:37 ` RFC Introducing Heart Monitors for IIO Daniel Baluta
2014-10-15 14:37 ` Dan Murphy
2014-10-15 14:43 ` Karol Wrona
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).