RFC Introducing Heart Monitors for IIO

RFC Introducing Heart Monitors for IIO

[Dan Murphy]

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

[RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors

[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

Re: [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors

[Daniel Baluta]

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 {
>

Re: [RFC Patch 1/3] iio: heart_monitors: Add support for heart rate monitors

[Jonathan Cameron]

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 {
> 

[RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation

[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

Re: [RFC Patch 2/3] iio: bindings: Add TI afe4403 heart monitor documentation

[Peter Meerwald]

> 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)

[RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor

[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 *)&reg, 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

Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor

[Daniel Baluta]

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.

Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor

[Dan Murphy]

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

Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor

[Peter Meerwald]

> 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 *)&reg, 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)

Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor

[Dan Murphy]

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 *)&reg, 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

Re: [RFC Patch 3/3] iio: heart_monitor: Add TI afe4403 heart monitor

[Jonathan Cameron]

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 *)&reg, 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");
>>>
>
>

Re: RFC Introducing Heart Monitors for IIO

[Daniel Baluta]

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.

Re: RFC Introducing Heart Monitors for IIO

[Dan Murphy]

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

Re: RFC Introducing Heart Monitors for IIO

[Karol Wrona]

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