Re: [PATCH v2 2/2] iio: health: Add driver for the TI AFE4404 heart monitor
From: Jonathan Cameron <jic23@kernel.org>
Date: 2015-12-05 18:22:01
On 02/12/15 19:57, Andrew F. Davis wrote:
Add driver for the TI AFE4404 heart rate monitor and pulse oximeter. This device detects reflected LED light fluctuations and presents an ADC value to the user space for further signal processing. Datasheet: http://www.ti.com/product/AFE4404/datasheet Signed-off-by: Andrew F. Davis <redacted>
I like this a lot. Seems much simpler to me. Various bits and bobs inline though. You quite rightly stated there was too much to describe in the change log so I've kind of started from scratch on the review as well. Thanks for your hard work on this. Jonathan
quoted hunk
--- .../ABI/testing/sysfs-bus-iio-health-afe4404 | 20 + drivers/iio/Kconfig | 1 + drivers/iio/Makefile | 1 + drivers/iio/health/Kconfig | 25 + drivers/iio/health/Makefile | 6 + drivers/iio/health/afe4404.c | 619 +++++++++++++++++++++ drivers/iio/health/afe440x.h | 168 ++++++ 7 files changed, 840 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-health-afe4404 create mode 100644 drivers/iio/health/Kconfig create mode 100644 drivers/iio/health/Makefile create mode 100644 drivers/iio/health/afe4404.c create mode 100644 drivers/iio/health/afe440x.hdiff --git a/Documentation/ABI/testing/sysfs-bus-iio-health-afe4404 b/Documentation/ABI/testing/sysfs-bus-iio-health-afe4404 new file mode 100644 index 0000000..c104d66 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-health-afe4404@@ -0,0 +1,20 @@ +What: /sys/bus/iio/devices/iio:deviceX/tia_resistanceY + /sys/bus/iio/devices/iio:deviceX/tia_capacitanceY +Date: December 2015 +KernelVersion: +Contact: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> +Description: + Get and set the resistance and the capacitance settings for the + Transimpedance Amplifier. Y is 1 for Rf1 and Cf1, Y is 2 for + Rf2 and Cf2 values. + Resistance setting is from 0 -> 7 + Capcitance setting is from 0 -> 15
No magic numbers if at all possible. These correspond to real resistances and capacitances. You also want an _available attribute for each of them for the different possible values. I'm not overly keep on the naming still but it's part specific enough that if you fix the units I'll let it go ;)
quoted hunk
+ +What: /sys/bus/iio/devices/iio:deviceX/tia_separate_en +Date: December 2015 +KernelVersion: +Contact: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> +Description: + Enable or disable separate settings for the TransImpedance + Amplifier above, when disabled both values are set by the + first channel.
Weird and wonderful but fine for a part specific attibute! As noted below, I think we need to document all the new ABI even though much of it is just extended names on standard channels.
quoted hunk
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig index 66792e7..ac085ab 100644 --- a/drivers/iio/Kconfig +++ b/drivers/iio/Kconfig@@ -52,6 +52,7 @@ source "drivers/iio/common/Kconfig" source "drivers/iio/dac/Kconfig" source "drivers/iio/frequency/Kconfig" source "drivers/iio/gyro/Kconfig" +source "drivers/iio/health/Kconfig" source "drivers/iio/humidity/Kconfig" source "drivers/iio/imu/Kconfig" source "drivers/iio/light/Kconfig"diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile index aeca726..6c5eb2a 100644 --- a/drivers/iio/Makefile +++ b/drivers/iio/Makefile@@ -18,6 +18,7 @@ obj-y += common/ obj-y += dac/ obj-y += gyro/ obj-y += frequency/ +obj-y += health/ obj-y += humidity/ obj-y += imu/ obj-y += light/diff --git a/drivers/iio/health/Kconfig b/drivers/iio/health/Kconfig new file mode 100644 index 0000000..526e7af --- /dev/null +++ b/drivers/iio/health/Kconfig@@ -0,0 +1,25 @@ +# +# Health drivers +# +# When adding new entries keep the list in alphabetical order + +menu "Health" + +menu "Heart Rate Monitors" + +config AFE4404
(this one is still 'interesting' as it's nothing to do with led1 other than it's taken temporily close to it - the led is presumably off at the time! - I can't think of a better way though - maybe another reviewer will)
quoted hunk
+ AFE440X_INTENSITY_CHAN(LED2, "led2", BIT(IIO_CHAN_INFO_OFFSET)), + AFE440X_INTENSITY_CHAN(ALED2, "led2_ambient", BIT(IIO_CHAN_INFO_OFFSET)), + AFE440X_INTENSITY_CHAN(LED3, "led3", BIT(IIO_CHAN_INFO_OFFSET)), + AFE440X_INTENSITY_CHAN(LED1_ALED1, "led1-led1_ambient", 0), + AFE440X_INTENSITY_CHAN(LED2_ALED2, "led2-led2_ambient", 0),
This trick for the differential cases is a cludge to work around deficiencies in our existing infrastructure, but it's straight forward one for an unusual case so fair enough.
+ AFE440X_INTENSITY_CHAN(AVG_LED1_ALED1, "led1-led1_ambient_mean", 0), + AFE440X_INTENSITY_CHAN(AVG_LED2_ALED2, "led2-led2_ambient_mean", 0),
I'd love to better describe these two with the filters made explicit as separate sysfs attributes rather than here. It's kind of backwards but we do have oversampling to define what is happening here. I also note that the true output rate of this is lower than the other channels. If we were to propose a decimation to match with oversampling and to explicitly document them as a pair it might work. Which is relevant for a channel is dependent on the 'natural - e.g. the trigger rate' at which we fill the buffer. So if the whole device is performing decimation and we read at the reduced frequency then it is oversampling - if we read at the original frequency it is decimation. What do you think?
quoted hunk
+ /* LED current */ + AFE440X_CURRENT_CHAN(ILED1, "led1"), + AFE440X_CURRENT_CHAN(ILED2, "led2"), + AFE440X_CURRENT_CHAN(ILED3, "led3"),
Whilst all these now just 'stretch' :) the current ABI structure they do need explicitly documenting somewhere. Probably odd enough that they'll want to go in your part specific doc rather than just adding the specific naming to the existing abi.
quoted hunk
+}; + +static ssize_t afe440x_show_register(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev); + struct afe440x_reg *afe440x_reg = to_afe440x_reg(attr); + unsigned int reg_val; + int ret; + + ret = regmap_read(afe440x->regmap, afe440x_reg->reg, ®_val); + if (ret) + return ret; + + reg_val >>= afe440x_reg->shift; + reg_val &= afe440x_reg->mask; + + return scnprintf(buf, PAGE_SIZE, "%u\n", reg_val); +} + +static ssize_t afe440x_store_register(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev); + struct afe440x_reg *afe440x_reg = to_afe440x_reg(attr); + unsigned int reg_val; + int val, ret; + + if (kstrtoint(buf, 0, &val)) + return -EINVAL; + + ret = regmap_read(afe440x->regmap, afe440x_reg->reg, ®_val); + if (ret) + return ret; + + reg_val &= ~afe440x_reg->mask;
why the local reg variable? Stick it dirrectly in the functional call.
quoted hunk
+ ret = regmap_read(afe440x->regmap, reg, &buffer[i++]); + if (ret) + goto err; + } + + iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp); +err: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + +static const struct iio_trigger_ops afe440x_trigger_ops = { + .owner = THIS_MODULE, +}; + +/* Default timings from data-sheet */ +#define AFE4404_TIMING_PAIRS \ + { AFE440X_PRPCOUNT, 39999 }, \ + { AFE440X_LED2LEDSTC, 0 }, \ + { AFE440X_LED2LEDENDC, 398 }, \ + { AFE440X_LED2STC, 80 }, \ + { AFE440X_LED2ENDC, 398 }, \ + { AFE440X_ADCRSTSTCT0, 5600 }, \ + { AFE440X_ADCRSTENDCT0, 5606 }, \ + { AFE440X_LED2CONVST, 5607 }, \ + { AFE440X_LED2CONVEND, 6066 }, \ + { AFE4404_LED3LEDSTC, 400 }, \ + { AFE4404_LED3LEDENDC, 798 }, \ + { AFE440X_ALED2STC, 480 }, \ + { AFE440X_ALED2ENDC, 798 }, \ + { AFE440X_ADCRSTSTCT1, 6068 }, \ + { AFE440X_ADCRSTENDCT1, 6074 }, \ + { AFE440X_ALED2CONVST, 6075 }, \ + { AFE440X_ALED2CONVEND, 6534 }, \ + { AFE440X_LED1LEDSTC, 800 }, \ + { AFE440X_LED1LEDENDC, 1198 }, \ + { AFE440X_LED1STC, 880 }, \ + { AFE440X_LED1ENDC, 1198 }, \ + { AFE440X_ADCRSTSTCT2, 6536 }, \
Some of the static code checkers (probably coccicheck) will point out that moving the return ret out of the above brackets will have the same effect as you have here in less code. It's worth running sparse, smatch and coccicheck on drivers before submitting as otherwise we miss things and then get a series of 'fix' patches after this hits a public tree rather than the list (you sometimes get them for posting on the list as well these days!)
quoted hunk
+ + return 0; +} + +static int afe440x_resume(struct device *dev) +{ + struct afe440x_data *afe440x = dev_get_drvdata(dev); + int ret; + + ret = regmap_update_bits(afe440x->regmap, AFE440X_CONTROL2, + AFE440X_CONTROL2_PDN_AFE, 0); + if (ret) + return ret;
I'd expect you want to turn the device off on remove given you turn it on during a resume...
quoted hunk
+ + ret = regulator_enable(afe440x->regulator); + if (ret) { + dev_err(dev, "Failed to enable regulator\n"); + return ret; + }
You turn the reg on here, but nowhere else that I can see. Why should it be on during the initial start?
quoted hunk
+ + return 0; +} + +SIMPLE_DEV_PM_OPS(afe440x_pm_ops, afe440x_suspend, afe440x_resume); + +static int afe440x_iio_setup(struct afe440x_data *afe440x) +{ + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(afe440x->dev, 0); + if (!indio_dev) { + dev_err(afe440x->dev, "Unable to allocate IIO device\n"); + return -ENOMEM; + } + + iio_device_set_drvdata(indio_dev, afe440x); + + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->dev.parent = afe440x->dev; + indio_dev->channels = afe440x->channels; + indio_dev->num_channels = afe440x->num_channels; + indio_dev->name = afe440x->name; + indio_dev->info = afe440x->info; + + if (afe440x->irq > 0) { + afe440x->trig = devm_iio_trigger_alloc(afe440x->dev, + "%s-dev%d", + indio_dev->name, + indio_dev->id); + if (!afe440x->trig) { + dev_err(afe440x->dev, "Unable to allocate IIO trigger\n"); + return -ENOMEM; + } + + iio_trigger_set_drvdata(afe440x->trig, indio_dev); + + afe440x->trig->ops = &afe440x_trigger_ops; + afe440x->trig->dev.parent = afe440x->dev;
This suprises me a little - I'd expect such a complex part to do at least some power saving or similar... If that is the case you'll want to do that in the remove after the device is unregistered (to avoid a race with userspace interfaces still available). Can always be added in a later patch however!
quoted hunk
+ if (ret) { + dev_err(afe440x->dev, "Unable to register IIO device\n"); + return ret; + } + + return 0; +} + +static int afe4404_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct afe440x_data *afe440x; + int ret; + + afe440x = devm_kzalloc(&client->dev, sizeof(*afe440x), GFP_KERNEL); + if (!afe440x) + return -ENOMEM;
This is all still backwards to my mind and could be easily refactored to provide the device specific stuff in a similar way to other IIO drivers do it with a mixture of const data and appropriate callbacks. I appreciate I haven't seen the other driver you mentioned shares a lot of the code, but I doubt it does anything that couldn't be handled that way around.
quoted hunk
+ + i2c_set_clientdata(client, afe440x); + + afe440x->dev = &client->dev; + afe440x->irq = client->irq; + + afe440x->regmap = devm_regmap_init_i2c(client, &afe4404_regmap_config); + if (IS_ERR(afe440x->regmap)) { + dev_err(afe440x->dev, "Unable to allocate register map\n"); + return PTR_ERR(afe440x->regmap); + } + + afe440x->regulator = devm_regulator_get(afe440x->dev, "tx_sup"); + if (IS_ERR(afe440x->regulator)) { + dev_err(afe440x->dev, "Unable to get regulator\n"); + return PTR_ERR(afe440x->regulator); + }
Given you turn the reg on during the resume, I'm guessing you want to turn it on here and off in remove? No reason to assume it is on when the board is powered up!
quoted hunk
+ + ret = regmap_write(afe440x->regmap, AFE440X_CONTROL0, + AFE440X_CONTROL0_SW_RESET); + if (ret) { + dev_err(afe440x->dev, "Unable to reset device\n"); + return ret; + } + + ret = regmap_register_patch(afe440x->regmap, afe4404_reg_sequences, + ARRAY_SIZE(afe4404_reg_sequences)); + if (ret) { + dev_err(afe440x->dev, "Unable to set register defaults\n"); + return ret; + } +
As I suggest below in the header, I'd put these elements into a separate structure (as they are constant for a given part)
quoted hunk
+ afe440x->channels = afe4404_channels; + afe440x->num_channels = ARRAY_SIZE(afe4404_channels); + afe440x->name = AFE4404_DRIVER_NAME; + afe440x->info = &afe4404_iio_info; + + return afe440x_iio_setup(afe440x); +} + +static const struct i2c_device_id afe4404_ids[] = { + { "afe4404", 0 }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(i2c, afe4404_ids); + +static struct i2c_driver afe4404_i2c_driver = { + .driver = { + .name = AFE4404_DRIVER_NAME, + .of_match_table = of_match_ptr(afe4404_of_match), + .pm = &afe440x_pm_ops, + }, + .probe = afe4404_probe, + .id_table = afe4404_ids, +}; +module_i2c_driver(afe4404_i2c_driver); + +MODULE_AUTHOR("Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>"); +MODULE_DESCRIPTION("TI AFE4404 Heart Rate and Pulse Oximeter"); +MODULE_LICENSE("GPL");diff --git a/drivers/iio/health/afe440x.h b/drivers/iio/health/afe440x.h new file mode 100644 index 0000000..73a2577 --- /dev/null +++ b/drivers/iio/health/afe440x.h@@ -0,0 +1,168 @@ +/* + * AFE440X Heart Rate Monitors and Low-Cost Pulse Oximeters + * + * Author: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> + * + * Copyright: (C) 2015 Texas Instruments, Inc.
I know you are aiming to support more devices and hence the header file spit may make sense, but please squish it into the .c file for now and pull it out as part of the series adding the new devices - avoids me getting patches 'tidying this up' in the interval before the other device support makes it.
quoted hunk
+ +/* AFE440X registers */ +#define AFE440X_CONTROL0 0x00 +#define AFE440X_LED2STC 0x01 +#define AFE440X_LED2ENDC 0x02 +#define AFE440X_LED1LEDSTC 0x03 +#define AFE440X_LED1LEDENDC 0x04 +#define AFE440X_ALED2STC 0x05 +#define AFE440X_ALED2ENDC 0x06 +#define AFE440X_LED1STC 0x07 +#define AFE440X_LED1ENDC 0x08 +#define AFE440X_LED2LEDSTC 0x09 +#define AFE440X_LED2LEDENDC 0x0a +#define AFE440X_ALED1STC 0x0b +#define AFE440X_ALED1ENDC 0x0c +#define AFE440X_LED2CONVST 0x0d +#define AFE440X_LED2CONVEND 0x0e +#define AFE440X_ALED2CONVST 0x0f +#define AFE440X_ALED2CONVEND 0x10 +#define AFE440X_LED1CONVST 0x11 +#define AFE440X_LED1CONVEND 0x12 +#define AFE440X_ALED1CONVST 0x13 +#define AFE440X_ALED1CONVEND 0x14 +#define AFE440X_ADCRSTSTCT0 0x15 +#define AFE440X_ADCRSTENDCT0 0x16 +#define AFE440X_ADCRSTSTCT1 0x17 +#define AFE440X_ADCRSTENDCT1 0x18 +#define AFE440X_ADCRSTSTCT2 0x19 +#define AFE440X_ADCRSTENDCT2 0x1a +#define AFE440X_ADCRSTSTCT3 0x1b +#define AFE440X_ADCRSTENDCT3 0x1c +#define AFE440X_PRPCOUNT 0x1d +#define AFE440X_CONTROL1 0x1e +#define AFE440X_TIAGAIN 0x20 +#define AFE440X_TIA_AMB_GAIN 0x21 +#define AFE440X_LEDCNTRL 0x22 +#define AFE440X_CONTROL2 0x23 +#define AFE440X_ALARM 0x29 +#define AFE440X_LED2VAL 0x2a +#define AFE440X_ALED2VAL 0x2b
I'd particularly like to see the above two definitions next to the arrays they are used to instantiate - makes for easier review!
quoted hunk
+ +struct afe440x_reg { + struct device_attribute dev_attr; + unsigned int reg; + unsigned int shift; + unsigned int mask; +}; + +#define to_afe440x_reg(_dev_attr) \ + container_of(_dev_attr, struct afe440x_reg, dev_attr) + +#define AFE440X_ATTR(_name, _reg, _field) \ + struct afe440x_reg afe440x_reg_##_name = { \ + .dev_attr = __ATTR(_name, (S_IRUGO | S_IWUSR), \ + afe440x_show_register, \ + afe440x_store_register), \ + .reg = _reg, \ + .shift = _field ## _SHIFT, \ + .mask = _field ## _MASK, \ + }
For now I'd like to see this in the .c file
quoted hunk
+ +/** + * struct afe440x_data + * @dev - Device structure + * @name - Device name
You still have a few bits in here that are presumably there to assist supporting multiple drivers. I don't think they belong in this structure. Split them out to say struct afe440x_device_info and pass that into your iio_setup function as well. Then they won't be in this struct for it's other uses which would be cleaner (at the cost of one extra parameter to that function call) It will also split out thsoe parts that are constant for a given type of device from those that are instance specific (such as Irq's etc).
quoted hunk
+ * @regmap - Register map of the device + * @regulator - Pointer to the regulator for the IC + * @channels - IIO channels + * @num_channels - Number of IIO channels + * @info - IIO info for device + * @trig - IIO trigger for this device + * @irq - ADC_RDY line interrupt number +**/
Nicely documented - thanks.
quoted hunk
+struct afe440x_data { + struct device *dev; + const char *name; + struct regmap *regmap; + struct regulator *regulator; + struct iio_chan_spec const *channels; + int num_channels; + const struct iio_info *info; + struct iio_trigger *trig; + int irq; +}; + +#endif /* _AFE440X_H */