Daniel Thanks for the review On 10/15/2014 02:35 AM, Daniel Baluta wrote: > > > On 10/14/2014 10:13 PM, Dan Murphy wrote: >> Add the TI afe4403 heart rate monitor. >> This device detects reflected LED >> wave length fluctuations and presents an ADC >> value to the user space to be converted to a >> heart rate. >> >> Data sheet located here: >> http://www.ti.com/product/AFE4403/datasheet > > <snip> > >> +obj-y += heart_monitors/ > s/heart_monitors/heart_monitor Yeah that aligns with the rest of the directories in iio. Singular not plural > >> obj-y += humidity/ >> obj-y += imu/ >> obj-y += light/ >> diff --git a/drivers/iio/heart_monitors/Kconfig b/drivers/iio/heart_monitors/Kconfig >> new file mode 100644 >> index 0000000..7060694 >> --- /dev/null >> +++ b/drivers/iio/heart_monitors/Kconfig >> @@ -0,0 +1,16 @@ >> +# >> +# Heart Rate Monitor drivers >> +# >> +# When adding new entries keep the list in alphabetical order >> + >> +menu "Heart Rate Monitors" >> + >> +config AFE4403 >> + tristate "TI AFE4403 Heart Rate Monitor" >> + depends on SPI >> + select IIO_BUFFER >> + select IIO_TRIGGERED_BUFFER >> + select SYSFS >> + help > > Final version should contain a real help here. It will this was just a place holder for RFC > >> + >> +endmenu >> diff --git a/drivers/iio/heart_monitors/Makefile b/drivers/iio/heart_monitors/Makefile >> new file mode 100644 >> index 0000000..77cc5c5 >> --- /dev/null >> +++ b/drivers/iio/heart_monitors/Makefile >> @@ -0,0 +1,6 @@ >> +# >> +# Makefile for IIO Heart Rate Monitor drivers >> +# >> + >> +# When adding new entries keep the list in alphabetical order >> +obj-$(CONFIG_AFE4403) += afe4403.o >> diff --git a/drivers/iio/heart_monitors/afe4403.c b/drivers/iio/heart_monitors/afe4403.c >> new file mode 100644 >> index 0000000..d285769 >> --- /dev/null >> +++ b/drivers/iio/heart_monitors/afe4403.c >> @@ -0,0 +1,610 @@ >> +/* >> + * AFE4403 Heart Rate Monitors and Low-Cost Pulse Oximeters >> + * >> + * Author: Dan Murphy <dmurphy@xxxxxx> >> + * >> + * Copyright: (C) 2014 Texas Instruments, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/delay.h> >> +#include <linux/err.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_gpio.h> >> +#include <linux/spi/spi.h> >> +#include <linux/slab.h> >> +#include <linux/sysfs.h> >> +#include <linux/regulator/consumer.h> >> + >> +#include <linux/iio/iio.h> >> +#include <linux/iio/buffer.h> >> +#include <linux/iio/driver.h> >> +#include <linux/iio/machine.h> >> +#include <linux/iio/events.h> >> +#include <linux/iio/kfifo_buf.h> >> + >> +#define AFE4403_CONTROL0 0x00 >> +#define AFE4403_LED2STC 0x01 >> +#define AFE4403_LED2ENDC 0x02 >> +#define AFE4403_LED2LEDSTC 0x03 >> +#define AFE4403_LED2LEDENDC 0x04 >> +#define AFE4403_ALED2STC 0x05 >> +#define AFE4403_ALED2ENDC 0x06 >> +#define AFE4403_LED1STC 0x07 >> +#define AFE4403_LED1ENDC 0x08 >> +#define AFE4403_LED1LEDSTC 0x09 >> +#define AFE4403_LED1LEDENDC 0x0a >> +#define AFE4403_ALED1STC 0x0b >> +#define AFE4403_ALED1ENDC 0x0c >> +#define AFE4403_LED2CONVST 0x0d >> +#define AFE4403_LED2CONVEND 0x0e >> +#define AFE4403_ALED2CONVST 0x0f >> +#define AFE4403_ALED2CONVEND 0x10 >> +#define AFE4403_LED1CONVST 0x11 >> +#define AFE4403_LED1CONVEND 0x12 >> +#define AFE4403_ALED1CONVST 0x13 >> +#define AFE4403_ALED1CONVEND 0x14 >> +#define AFE4403_ADCRSTSTCT0 0x15 >> +#define AFE4403_ADCRSTENDCT0 0x16 >> +#define AFE4403_ADCRSTSTCT1 0x17 >> +#define AFE4403_ADCRSTENDCT1 0x18 >> +#define AFE4403_ADCRSTSTCT2 0x19 >> +#define AFE4403_ADCRSTENDCT2 0x1a >> +#define AFE4403_ADCRSTSTCT3 0x1b >> +#define AFE4403_ADCRSTENDCT3 0x1c >> +#define AFE4403_PRPCOUNT 0x1d >> +#define AFE4403_CONTROL1 0x1e >> +#define AFE4403_SPARE1 0x1f >> +#define AFE4403_TIAGAIN 0x20 >> +#define AFE4403_TIA_AMB_GAIN 0x21 >> +#define AFE4403_LEDCNTRL 0x22 >> +#define AFE4403_CONTROL2 0x23 >> +#define AFE4403_SPARE2 0x24 >> +#define AFE4403_SPARE3 0x25 >> +#define AFE4403_SPARE4 0x26 >> +#define AFE4403_ALARM 0x29 >> +#define AFE4403_LED2VAL 0x2A >> +#define AFE4403_ALED2VAL 0x2B >> +#define AFE4403_LED1VAL 0x2C >> +#define AFE4403_ALED1VAL 0x2D >> +#define AFE4403_LED2_ALED2VAL 0x2E >> +#define AFE4403_LED1_ALED1VAL 0x2F >> +#define AFE4403_DIAG 0x30 >> +#define AFE4403_CONTROL3 0x31 >> +#define AFE4403_PDNCYCLESTC 0x32 >> +#define AFE4403_PDNCYCLEENDC 0x33 >> + >> +#define AFE4403_SPI_READ (1 << 0) >> +#define AFE4403_SW_RESET (1 << 3) >> +#define AFE4403_PWR_DWN (1 << 0) > > Could use BIT(x) macro here. OK > >> + >> +/** >> + * struct afe4403_data - >> + * @work - Work item used to off load the enable/disable of the vibration >> + * @indio_dev - >> + * @map - >> + * @spi - >> + * @regulator - Pointer to the regulator for the IC >> + * @ste_gpio - >> + * @data_gpio - >> + * @reset_gpio - >> + * @data_buffer - >> + * @irq - AFE4403 interrupt number >> +**/ > > :), i assume this will be documented in the final version. Yes this is a little out of date. > >> +struct afe4403_data { >> + struct work_struct work; >> + struct iio_dev *indio_dev; >> + struct iio_map *map; >> + struct spi_device *spi; >> + struct regulator *regulator; >> + struct gpio_desc *ste_gpio; >> + struct gpio_desc *data_gpio; >> + struct gpio_desc *reset_gpio; >> + int data_buffer[5]; >> + int irq; >> +}; >> + > > <snip> > >> +static void afe4403_toggle_ste(struct afe4403_data *afe4403) >> +{ >> + gpiod_set_value(afe4403->ste_gpio, 1); >> + udelay(800); > > This hardcoded value should be documented. I will have some documentation or at the very least an explanation. This may change as well. > >> + gpiod_set_value(afe4403->ste_gpio, 0); >> +} >> + > > <snip> > >> +static int afe4403_write_event(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + enum iio_event_type type, >> + enum iio_event_direction dir, >> + int state) >> +{ >> + struct afe4403_data *afe4403 = iio_priv(indio_dev); >> + int ret; >> + int control_val; >> + >> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val); >> + if (ret) >> + return ret; >> + > > Use dev_dbg instead of printks. Yeah accidentally left these in when debugging these will be removed with official submission > >> + printk("***state 0x%X control 0x%X 0x%X\n", state, control_val, ~AFE4403_PWR_DWN); >> + if (state) >> + control_val &= ~AFE4403_PWR_DWN; >> + else >> + control_val |= AFE4403_PWR_DWN; >> + >> + printk("***after control 0x%X\n", control_val); >> + >> + ret = afe4403_write(afe4403, AFE4403_CONTROL2, control_val); >> + if (ret) >> + return ret; >> + >> + ret = afe4403_read(afe4403, AFE4403_CONTROL2, &control_val); >> + if (ret) >> + return ret; >> + >> + printk("***control 0x%X\n", control_val); >> + >> + return 0; >> +} > > Also, run checkpatch.pl over your patches. There are small whitespace issues. Sorry for the low quality patch I was not thinking when I submitted this low quality series of patches > > thanks, > Daniel. -- ------------------ Dan Murphy -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
