RFC Introducing Heart Monitors for IIO

RFC PATCH Superseded Dan Murphy 6 participants 15 messages Oct 14, 2014 – Oct 25, 2014

Subsystems (4)

Device tree bindings Staging IIO Staging - industrial io

lore

Share & tools

lore

Dan Murphy

author

2014-10-14 19:13 UTC #0 lore raw

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

Dan Murphy

author

2014-10-14 19:13 UTC #1 lore raw

RFC PATCH 1/3 iio: heart_monitors: Add support for heart rate monitors

Add a type for heart rate monitors
Add the modifier name in beats per minute.

Signed-off-by Dan Murphy <[email protected]>

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

drivers/iio/industrialio-core.c +2 -0

index af3e76d..d87ca35 100644
@@ -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 */

drivers/staging/iio/Documentation/iio_event_monitor.c +2 -0

index 569d6f8..b06dab7 100644
@@ -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;

include/linux/iio/types.h +3 -1

index 4a2af8a..50b1456 100644
@@ -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 {

Signature · 1 line

1.7.9.5

Daniel Baluta

2014-10-15 07:19 UTC #2 lore raw

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

Quoted 23 lines · Dan Murphy

Add a type for heart rate monitors
Add the modifier name in beats per minute.

Signed-off-by Dan Murphy <[email protected]>

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

drivers/iio/industrialio-core.c +1 -0

index af3e76d..d87ca35 100644
@@ -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[] = {

drivers/iio/industrialio-core.c static const char * const iio_modifier_names[] = { · line 95

Quoted 4 lines

drivers/iio/industrialio-core.c +1 -0

@@ -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.

drivers/staging/iio/Documentation/iio_event_monitor.c static const char * const iio_chan_type_name_spec[] = { · line 52

Quoted 12 lines

drivers/iio/industrialio-core.c +0 -0

  };
 
  /* relies on pairs of these shared then separate */

drivers/staging/iio/Documentation/iio_event_monitor.c +1 -0

index 569d6f8..b06dab7 100644
@@ -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.

Quoted 16 lines

drivers/staging/iio/Documentation/iio_event_monitor.c +1 -0

  };
 
  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;

include/linux/iio/types.h +0 -0

index 4a2af8a..50b1456 100644

include/linux/iio/types.h enum iio_chan_type { · line 33

Quoted 4 lines

include/linux/iio/types.h +1 -0

@@ -30,6 +30,7 @@ enum iio_chan_type {
  	IIO_CCT,
  	IIO_PRESSURE,
  	IIO_HUMIDITYRELATIVE,
+	IIO_HEARTRATE

Same here.

include/linux/iio/types.h enum iio_modifier { · line 64

Quoted 10 lines

include/linux/iio/types.h +2 -1

  };
 
  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.

Quoted 4 lines

include/linux/iio/types.h +0 -0
  };
 
  enum iio_event_type {

Jonathan Cameron

2014-10-15 19:35 UTC #3 lore raw

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

In reply to Dan Murphy

Quoted 2 lines · Dan Murphy

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

Quoted 71 lines

Signed-off-by Dan Murphy <[email protected]>

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

drivers/iio/industrialio-core.c +2 -0

index af3e76d..d87ca35 100644
@@ -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 */

drivers/staging/iio/Documentation/iio_event_monitor.c +2 -0

index 569d6f8..b06dab7 100644
@@ -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;

include/linux/iio/types.h +3 -1

index 4a2af8a..50b1456 100644
@@ -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 {

Dan Murphy

author

2014-10-14 19:14 UTC #4 lore raw

RFC PATCH 2/3 iio: bindings: Add TI afe4403 heart monitor documentation

In reply to Dan Murphy

Add the TI afe4403 heart monitor device tree
binding documentation. heart_monitors directory
created under iio.

Signed-off-by Dan Murphy <[email protected]>

---
 .../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

Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt new file +23 -0

new file mode 100644
index 0000000..d612dd6
@@ -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>;
+};

Signature · 1 line

1.7.9.5

Peter Meerwald

2014-10-15 10:03 UTC #5 lore raw

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

Quoted 20 lines

Add the TI afe4403 heart monitor device tree
binding documentation. heart_monitors directory
created under iio.
Signed-off-by Dan Murphy <[email protected]>
---
 .../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
Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt new file +2 -0
new file mode 100644
index 0000000..d612dd6
@@ -0,0 +1,23 @@
+* Texas Instruments - AFE4403 Heart rate and Pulse Oximeter
+

Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt line 12

Quoted 10 lines

Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt new file +10 -0

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

Quoted 12 lines

Documentation/devicetree/bindings/iio/heart_monitors/ti_afe4403.txt new file +11 -0

+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>;
+};

Signature · 2 lines

Peter Meerwald
+43-664-2444418 (mobile)

Dan Murphy

author

2014-10-14 19:13 UTC #6 lore raw

RFC PATCH 3/3 iio: heart_monitor: Add TI afe4403 heart monitor

In reply to 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 <[email protected]>

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

drivers/iio/Kconfig +1 -0

index 345395e..e3a7945 100644
@@ -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"

drivers/iio/Makefile +1 -0

index 698afc2..46c0960 100644
@@ -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/

drivers/iio/heart_monitors/Kconfig new file +16 -0

new file mode 100644
index 0000000..7060694
@@ -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

drivers/iio/heart_monitors/Makefile new file +6 -0

new file mode 100644
index 0000000..77cc5c5
@@ -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

drivers/iio/heart_monitors/afe4403.c new file +610 -0

610 lines not on this page · raw message

Signature · 1 line

1.7.9.5

Daniel Baluta

2014-10-15 07:35 UTC #7 lore raw

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

Quoted 8 lines · 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

<snip>

Quoted 1 line

+obj-y += heart_monitors/

s/heart_monitors/heart_monitor

Quoted 13 lines

obj-y += humidity/
obj-y += imu/
obj-y += light/
drivers/iio/heart_monitors/Kconfig new file +4 -0
new file mode 100644
index 0000000..7060694
@@ -0,0 +1,16 @@
+#
+# Heart Rate Monitor drivers
+#
+# When adding new entries keep the list in alphabetical order

drivers/iio/heart_monitors/Kconfig line 14

Quoted 10 lines

drivers/iio/heart_monitors/Kconfig new file +10 -0

+
+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.

Quoted 100 lines

drivers/iio/heart_monitors/Kconfig new file +2 -0

+
+endmenu

drivers/iio/heart_monitors/Makefile new file +6 -0

new file mode 100644
index 0000000..77cc5c5
@@ -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

drivers/iio/heart_monitors/afe4403.c new file +80 -0

new file mode 100644
index 0000000..d285769
@@ -0,0 +1,610 @@
+/*
+ * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
+ *
+ * Author: Dan Murphy <[email protected]>
+ *
+ * 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

drivers/iio/heart_monitors/afe4403.c line 90

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+#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.

Quoted 4 lines

drivers/iio/heart_monitors/afe4403.c new file +4 -0

+
+/**
+ * struct afe4403_data -
+ * @work - Work item used to off load the enable/disable of the vibration

drivers/iio/heart_monitors/afe4403.c line 104

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+ * @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.

Quoted 13 lines

drivers/iio/heart_monitors/afe4403.c new file +13 -0

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

drivers/iio/heart_monitors/afe4403.c line 202

Quoted 4 lines

drivers/iio/heart_monitors/afe4403.c new file +4 -0

+static void afe4403_toggle_ste(struct afe4403_data *afe4403)
+{
+	gpiod_set_value(afe4403->ste_gpio, 1);
+	udelay(800);

This hardcoded value should be documented.

Quoted 3 lines

drivers/iio/heart_monitors/afe4403.c new file +3 -0
+	gpiod_set_value(afe4403->ste_gpio, 0);
+}
+

<snip>

Quoted 4 lines

drivers/iio/heart_monitors/afe4403.c new file +4 -0

+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,

drivers/iio/heart_monitors/afe4403.c line 318

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+				     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.

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

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

drivers/iio/heart_monitors/afe4403.c line 338

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+		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.

Dan Murphy

author

2014-10-15 17:37 UTC #8 lore raw

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

Daniel

Thanks for the review

Quoted 9 lines · Daniel Baluta

Quoted 6 lines · 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.

Quoted 7 lines

Quoted 2 lines
Data sheet located here:
http://www.ti.com/product/AFE4403/datasheet
<snip>
Quoted 1 line
+obj-y += heart_monitors/
s/heart_monitors/heart_monitor

Yeah that aligns with the rest of the directories in iio. Singular not plural

Quoted 18 lines

Quoted 18 lines
obj-y += humidity/
obj-y += imu/
obj-y += light/
drivers/iio/heart_monitors/Kconfig new file +8 -0
new file mode 100644
index 0000000..7060694
@@ -0,0 +1,16 @@
+#
+# Heart Rate Monitor drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Heart Rate Monitors"
+
+config AFE4403

Quoted 8 lines

drivers/iio/heart_monitors/Kconfig line 14
Quoted 6 lines
drivers/iio/heart_monitors/Kconfig new file +6 -0
+    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

Quoted 105 lines

drivers/iio/heart_monitors/Kconfig new file +2 -0

+
+endmenu

drivers/iio/heart_monitors/Makefile new file +6 -0

new file mode 100644
index 0000000..77cc5c5
@@ -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

drivers/iio/heart_monitors/afe4403.c new file +84 -0

new file mode 100644
index 0000000..d285769
@@ -0,0 +1,610 @@
+/*
+ * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
+ *
+ * Author: Dan Murphy <[email protected]>
+ *
+ * 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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 90

Quoted 6 lines

drivers/iio/heart_monitors/afe4403.c new file +6 -0

+#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.

Quoted 17 lines

Quoted 5 lines
drivers/iio/heart_monitors/afe4403.c new file +4 -0
+
+/**
+ * struct afe4403_data -
+ * @work - Work item used to off load the enable/disable of the vibration
drivers/iio/heart_monitors/afe4403.c line 104
Quoted 10 lines
drivers/iio/heart_monitors/afe4403.c new file +10 -0
+ * @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.

Quoted 17 lines

Quoted 14 lines

drivers/iio/heart_monitors/afe4403.c new file +13 -0

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

Quoted 6 lines

drivers/iio/heart_monitors/afe4403.c line 202
Quoted 4 lines
drivers/iio/heart_monitors/afe4403.c new file +4 -0
+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.

Quoted 7 lines

Quoted 4 lines
drivers/iio/heart_monitors/afe4403.c new file +3 -0
+    gpiod_set_value(afe4403->ste_gpio, 0);
+}
+
<snip>

Quoted 16 lines

Quoted 4 lines

drivers/iio/heart_monitors/afe4403.c new file +4 -0

+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,

drivers/iio/heart_monitors/afe4403.c line 318

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+                     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

Quoted 15 lines

drivers/iio/heart_monitors/afe4403.c new file +14 -0

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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 338
Quoted 6 lines
drivers/iio/heart_monitors/afe4403.c new file +6 -0
+        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

Quoted 3 lines

thanks,
Daniel.

Signature · 2 lines

------------------
Dan Murphy

Peter Meerwald

2014-10-15 09:52 UTC #9 lore raw

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

In reply to Dan Murphy

Quoted 9 lines

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

Quoted 17 lines

Signed-off-by Dan Murphy <[email protected]>

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

drivers/iio/Kconfig +0 -0

index 345395e..e3a7945 100644

drivers/iio/Kconfig source "drivers/iio/common/Kconfig" · line 69

Quoted 4 lines

drivers/iio/Kconfig +1 -0

@@ -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_

Quoted 24 lines

drivers/iio/Kconfig +0 -0

 source "drivers/iio/humidity/Kconfig"
 source "drivers/iio/imu/Kconfig"
 source "drivers/iio/light/Kconfig"

drivers/iio/Makefile +1 -0

index 698afc2..46c0960 100644
@@ -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/

drivers/iio/heart_monitors/Kconfig new file +3 -0

new file mode 100644
index 0000000..7060694
@@ -0,0 +1,16 @@
+#
+# Heart Rate Monitor drivers
+#

drivers/iio/heart_monitors/Kconfig line 13

Quoted 10 lines

drivers/iio/heart_monitors/Kconfig new file +10 -0

+# 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

Quoted 101 lines

drivers/iio/heart_monitors/Kconfig new file +3 -0

+	help
+
+endmenu

drivers/iio/heart_monitors/Makefile new file +6 -0

new file mode 100644
index 0000000..77cc5c5
@@ -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

drivers/iio/heart_monitors/afe4403.c new file +80 -0

new file mode 100644
index 0000000..d285769
@@ -0,0 +1,610 @@
+/*
+ * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
+ *
+ * Author: Dan Murphy <[email protected]>
+ *
+ * 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

drivers/iio/heart_monitors/afe4403.c line 90

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+#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()

Quoted 14 lines

drivers/iio/heart_monitors/afe4403.c new file +14 -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
+**/

drivers/iio/heart_monitors/afe4403.c line 114

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+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

Quoted 22 lines

drivers/iio/heart_monitors/afe4403.c new file +22 -0

+	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),
+	},

drivers/iio/heart_monitors/afe4403.c line 146

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+};
+
+#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?

drivers/iio/heart_monitors/afe4403.c line 149

Quoted 3 lines

drivers/iio/heart_monitors/afe4403.c new file +3 -0
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 24,					\

top 2 bits are ignored

drivers/iio/heart_monitors/afe4403.c line 150

Quoted 1 line

drivers/iio/heart_monitors/afe4403.c new file +1 -0
+			.storagebits = 24,				\

.endianness = IIO_BE

Quoted 109 lines

drivers/iio/heart_monitors/afe4403.c new file +109 -0

+		},							\
+		.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;
+};
+

drivers/iio/heart_monitors/afe4403.c line 269

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+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?

Quoted 25 lines

drivers/iio/heart_monitors/afe4403.c new file +24 -0

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

Quoted 1 line

=

and it is data_buffer[5]; 5 is smaller than ARRAY_SIZE(afe4003_channels)

Quoted 12 lines

drivers/iio/heart_monitors/afe4403.c new file +12 -0

+		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,

drivers/iio/heart_monitors/afe4403.c line 315

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+				     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...

drivers/iio/heart_monitors/afe4403.c line 319

Quoted 4 lines

drivers/iio/heart_monitors/afe4403.c new file +4 -0

+	if (ret)
+		return ret;
+
+	printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN);

no printk()s please

Quoted 26 lines

drivers/iio/heart_monitors/afe4403.c new file +26 -0

+	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,
+};

drivers/iio/heart_monitors/afe4403.c line 355

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+
+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?

Quoted 72 lines

drivers/iio/heart_monitors/afe4403.c new file +72 -0

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

drivers/iio/heart_monitors/afe4403.c line 437

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+	} 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()

Quoted 106 lines

drivers/iio/heart_monitors/afe4403.c new file +106 -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;

drivers/iio/heart_monitors/afe4403.c line 553

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+	}
+
+	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?

drivers/iio/heart_monitors/afe4403.c line 563

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+		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?

Quoted 6 lines

drivers/iio/heart_monitors/afe4403.c new file +6 -0
+		return ret;
+	}
+
+	return 0;
+
+}

drivers/iio/heart_monitors/afe4403.c line 579

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -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?

Quoted 32 lines

drivers/iio/heart_monitors/afe4403.c new file +31 -0

+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("TI AFE4403 Heart Rate and Pulse Oximeter");
+MODULE_LICENSE("GPL");

Signature · 2 lines

Peter Meerwald
+43-664-2444418 (mobile)

Dan Murphy

author

2014-10-22 15:36 UTC #10 lore raw

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

Peter
Sorry for the delayed reply I was working on the code and comments

Quoted 7 lines · Peter Meerwald

Quoted 7 lines
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

Quoted 5 lines

s/heart_monitor/heart_monitors
you need to update Documentation/ABI/testing/sysfs-bus-iio
more minor comments below

Will update

Quoted 25 lines

Quoted 18 lines

Signed-off-by Dan Murphy <[email protected]>

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

drivers/iio/Kconfig +0 -0

index 345395e..e3a7945 100644

drivers/iio/Kconfig source "drivers/iio/common/Kconfig" · line 69

Quoted 4 lines

drivers/iio/Kconfig +1 -0

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

Quoted 1 line

and use heartrate / HEARTRATE consistently, not HEART_, not HEARTBEAT_

Noted but not changed unless there are are other objections

Quoted 28 lines

drivers/iio/Kconfig +0 -0

 source "drivers/iio/humidity/Kconfig"
 source "drivers/iio/imu/Kconfig"
 source "drivers/iio/light/Kconfig"

drivers/iio/Makefile +1 -0

index 698afc2..46c0960 100644
@@ -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/

drivers/iio/heart_monitors/Kconfig new file +6 -0

new file mode 100644
index 0000000..7060694
@@ -0,0 +1,16 @@
+#
+# Heart Rate Monitor drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Heart Rate Monitors"

Quoted 8 lines

drivers/iio/heart_monitors/Kconfig line 13
Quoted 7 lines
drivers/iio/heart_monitors/Kconfig new file +7 -0
+
+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

Quoted 105 lines

drivers/iio/heart_monitors/Kconfig new file +3 -0

+	help
+
+endmenu

drivers/iio/heart_monitors/Makefile new file +6 -0

new file mode 100644
index 0000000..77cc5c5
@@ -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

drivers/iio/heart_monitors/afe4403.c new file +83 -0

new file mode 100644
index 0000000..d285769
@@ -0,0 +1,610 @@
+/*
+ * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
+ *
+ * Author: Dan Murphy <[email protected]>
+ *
+ * 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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 90

Quoted 7 lines

drivers/iio/heart_monitors/afe4403.c new file +7 -0

+#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?

Quoted 18 lines

drivers/iio/heart_monitors/afe4403.c new file +17 -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;

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 114
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+	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

Quoted 27 lines

drivers/iio/heart_monitors/afe4403.c new file +26 -0

+	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)						\
+	{								\

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 146
Quoted 6 lines
drivers/iio/heart_monitors/afe4403.c new file +6 -0
+		.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

Quoted 5 lines

drivers/iio/heart_monitors/afe4403.c line 149
Quoted 4 lines
drivers/iio/heart_monitors/afe4403.c new file +3 -0
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 24,					\
top 2 bits are ignored

Not sure what you mean here. How are the top two bits ignored?

Quoted 3 lines

drivers/iio/heart_monitors/afe4403.c line 150
Quoted 2 lines
drivers/iio/heart_monitors/afe4403.c new file +1 -0
+			.storagebits = 24,				\
.endianness = IIO_BE

Fixed

Quoted 123 lines

Quoted 110 lines

drivers/iio/heart_monitors/afe4403.c new file +109 -0

+		},							\
+		.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;
+};
+

drivers/iio/heart_monitors/afe4403.c line 269

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+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

Quoted 2 lines

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.

Quoted 19 lines

drivers/iio/heart_monitors/afe4403.c new file +18 -0

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

Quoted 8 lines

Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +6 -0
+	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.

Quoted 26 lines

Quoted 13 lines
drivers/iio/heart_monitors/afe4403.c new file +12 -0
+		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,
drivers/iio/heart_monitors/afe4403.c line 315
Quoted 10 lines
drivers/iio/heart_monitors/afe4403.c new file +10 -0
+				     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

Quoted 1 line

wondering about endianness...

Fixed

Quoted 6 lines

drivers/iio/heart_monitors/afe4403.c line 319
Quoted 5 lines
drivers/iio/heart_monitors/afe4403.c new file +4 -0
+	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.

Quoted 30 lines

drivers/iio/heart_monitors/afe4403.c new file +29 -0

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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 355
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+	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.

Quoted 76 lines

drivers/iio/heart_monitors/afe4403.c new file +75 -0

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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 437
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+		if (ret)
+			return ret;
+	}
+
+	udelay(800);
+
+	reg_count = sizeof(afe4403_init_regs) / sizeof(afe4403_init_regs[0]);
ARRAY_SIZE()

Fixed

Quoted 110 lines

drivers/iio/heart_monitors/afe4403.c new file +109 -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,

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 553

Quoted 7 lines

drivers/iio/heart_monitors/afe4403.c new file +7 -0

+				   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?

Quoted 12 lines

drivers/iio/heart_monitors/afe4403.c line 563
Quoted 11 lines
drivers/iio/heart_monitors/afe4403.c new file +10 -0
+		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

Quoted 10 lines

Quoted 10 lines
drivers/iio/heart_monitors/afe4403.c new file +9 -0
+		return ret;
+	}
+
+	return 0;
+
+}
+
+static int afe4403_spi_remove(struct spi_device *spi)
+{

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 579
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+	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

Quoted 33 lines

drivers/iio/heart_monitors/afe4403.c new file +31 -0

+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("TI AFE4403 Heart Rate and Pulse Oximeter");
+MODULE_LICENSE("GPL");

Signature · 2 lines

------------------
Dan Murphy

Jonathan Cameron

maintainer

2014-10-25 18:31 UTC #11 lore raw

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

Quoted 44 lines · Dan Murphy

Peter
Sorry for the delayed reply I was working on the code and comments
Quoted 7 lines · Peter Meerwald
Quoted 7 lines
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
Quoted 5 lines
s/heart_monitor/heart_monitors
you need to update Documentation/ABI/testing/sysfs-bus-iio
more minor comments below
Will update
Quoted 25 lines
Quoted 18 lines
Signed-off-by Dan Murphy <[email protected]>
---
 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
drivers/iio/Kconfig +0 -0
index 345395e..e3a7945 100644
drivers/iio/Kconfig source "drivers/iio/common/Kconfig" · line 69
Quoted 4 lines
drivers/iio/Kconfig +1 -0
@@ -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)

Quoted 3 lines

Quoted 1 line
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.

Quoted 154 lines

Quoted 29 lines

drivers/iio/Kconfig +0 -0

 source "drivers/iio/humidity/Kconfig"
 source "drivers/iio/imu/Kconfig"
 source "drivers/iio/light/Kconfig"

drivers/iio/Makefile +1 -0

index 698afc2..46c0960 100644
@@ -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/

drivers/iio/heart_monitors/Kconfig new file +6 -0

new file mode 100644
index 0000000..7060694
@@ -0,0 +1,16 @@
+#
+# Heart Rate Monitor drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Heart Rate Monitors"

Quoted 8 lines

drivers/iio/heart_monitors/Kconfig line 13
Quoted 7 lines
drivers/iio/heart_monitors/Kconfig new file +7 -0
+
+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

Quoted 105 lines

drivers/iio/heart_monitors/Kconfig new file +3 -0

+	help
+
+endmenu

drivers/iio/heart_monitors/Makefile new file +6 -0

new file mode 100644
index 0000000..77cc5c5
@@ -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

drivers/iio/heart_monitors/afe4403.c new file +83 -0

new file mode 100644
index 0000000..d285769
@@ -0,0 +1,610 @@
+/*
+ * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters
+ *
+ * Author: Dan Murphy <[email protected]>
+ *
+ * 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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 90

Quoted 7 lines

drivers/iio/heart_monitors/afe4403.c new file +7 -0

+#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()

Quoted 4 lines

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!

Quoted 205 lines

Quoted 19 lines

drivers/iio/heart_monitors/afe4403.c new file +17 -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;

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 114
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+	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

Quoted 27 lines

drivers/iio/heart_monitors/afe4403.c new file +26 -0

+	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)						\
+	{								\

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 146
Quoted 6 lines
drivers/iio/heart_monitors/afe4403.c new file +6 -0
+		.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

Quoted 5 lines

drivers/iio/heart_monitors/afe4403.c line 149
Quoted 4 lines
drivers/iio/heart_monitors/afe4403.c new file +3 -0
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 24,					\
top 2 bits are ignored

Not sure what you mean here. How are the top two bits ignored?

Quoted 3 lines

drivers/iio/heart_monitors/afe4403.c line 150
Quoted 2 lines
drivers/iio/heart_monitors/afe4403.c new file +1 -0
+			.storagebits = 24,				\
.endianness = IIO_BE

Fixed

Quoted 123 lines

Quoted 110 lines

drivers/iio/heart_monitors/afe4403.c new file +109 -0

+		},							\
+		.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;
+};
+

drivers/iio/heart_monitors/afe4403.c line 269

Quoted 10 lines

drivers/iio/heart_monitors/afe4403.c new file +10 -0

+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

Quoted 7 lines

Quoted 2 lines
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)

Quoted 319 lines

Quoted 20 lines

drivers/iio/heart_monitors/afe4403.c new file +18 -0

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

Quoted 8 lines

Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +6 -0
+	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.

Quoted 26 lines

Quoted 13 lines
drivers/iio/heart_monitors/afe4403.c new file +12 -0
+		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,
drivers/iio/heart_monitors/afe4403.c line 315
Quoted 10 lines
drivers/iio/heart_monitors/afe4403.c new file +10 -0
+				     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

Quoted 1 line

wondering about endianness...

Fixed

Quoted 6 lines

drivers/iio/heart_monitors/afe4403.c line 319
Quoted 5 lines
drivers/iio/heart_monitors/afe4403.c new file +4 -0
+	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.

Quoted 30 lines

drivers/iio/heart_monitors/afe4403.c new file +29 -0

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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 355
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+	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.

Quoted 76 lines

drivers/iio/heart_monitors/afe4403.c new file +75 -0

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

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 437
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+		if (ret)
+			return ret;
+	}
+
+	udelay(800);
+
+	reg_count = sizeof(afe4403_init_regs) / sizeof(afe4403_init_regs[0]);
ARRAY_SIZE()

Fixed

Quoted 110 lines

drivers/iio/heart_monitors/afe4403.c new file +109 -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,

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 553

Quoted 7 lines

drivers/iio/heart_monitors/afe4403.c new file +7 -0

+				   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?

Quoted 2 lines

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

Quoted 17 lines

Quoted 13 lines
drivers/iio/heart_monitors/afe4403.c line 563
Quoted 12 lines
drivers/iio/heart_monitors/afe4403.c new file +10 -0
+		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.

Quoted 6 lines

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

Quoted 58 lines

Quoted 12 lines

Quoted 12 lines
drivers/iio/heart_monitors/afe4403.c new file +9 -0
+		return ret;
+	}
+
+	return 0;
+
+}
+
+static int afe4403_spi_remove(struct spi_device *spi)
+{

Quoted 8 lines

drivers/iio/heart_monitors/afe4403.c line 579
Quoted 7 lines
drivers/iio/heart_monitors/afe4403.c new file +7 -0
+	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

Quoted 33 lines

drivers/iio/heart_monitors/afe4403.c new file +31 -0

+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("TI AFE4403 Heart Rate and Pulse Oximeter");
+MODULE_LICENSE("GPL");

Daniel Baluta

2014-10-15 07:31 UTC #12 lore raw

In reply to Dan Murphy

Quoted 12 lines · 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.

Quoted 8 lines

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.

Dan Murphy

author

2014-10-15 14:37 UTC #13 lore raw

Daniel

Quoted 18 lines · Daniel Baluta

Quoted 15 lines · 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

Quoted 8 lines

Quoted 5 lines
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

Quoted 3 lines

Daniel.

Signature · 2 lines

------------------
Dan Murphy

Karol Wrona

2014-10-15 14:43 UTC #14 lore raw

Quoted 12 lines · 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.

Quoted 8 lines

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