Messages in this thread | | | Subject | Re: [PATCH v3 3/5] iio: health: Add driver for the TI AFE4404 heart monitor | From | "Andrew F. Davis" <> | Date | Wed, 30 Dec 2015 11:23:41 -0600 |
| |
On 12/22/2015 11:37 AM, Jonathan Cameron wrote: > On 14/12/15 22:35, 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 <afd@ti.com> > Hi Andrew, > > Sorry I didn't get to this a the weekend. We are now likely to just > miss the merge window so lets get this cleaned up and ready to go > for the start of the next cycle (probably 3rd week of January) >
No problem, I've been on holiday these last few days anyway.
> Looking good mostly. A few minor bits and bobs I either missed before > or that got introduced as side effects of your changes. > I thought about fixing them up and applying, but I think it's just > a shade to many changes and I need your feedback on a couple of them > anyway. > > Jonathan > >> --- >> .../ABI/testing/sysfs-bus-iio-health-afe4404 | 60 ++ >> drivers/iio/Kconfig | 1 + >> drivers/iio/Makefile | 1 + >> drivers/iio/health/Kconfig | 25 + >> drivers/iio/health/Makefile | 6 + >> drivers/iio/health/afe4404.c | 700 +++++++++++++++++++++ >> drivers/iio/health/afe440x.h | 202 ++++++ >> 7 files changed, 995 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.h >> >> diff --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..b01ca47 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-bus-iio-health-afe4404 >> @@ -0,0 +1,60 @@ >> +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@ti.com> >> +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. >> + Valid Resistance settings are 500000, 250000, 100000, 50000, >> + 25000, 10000, 1000000, and 2000000 Ohms. >> + Valid capacitance settings are 5, 2.5, 10, 7.5, 20, 17.5, 25, >> + and 22.5 picoFarads. > Defined units for IIO are nanofarads - see sysfs-bus-iio. > Would really like to keep this consistent and doesn't look like that should > be terribly hard to do here. >
Ah, for some reason I thought pico, will fix.
>> + >> +What: /sys/bus/iio/devices/iio:deviceX/tia_separate_en >> +Date: December 2015 >> +KernelVersion: >> +Contact: Andrew F. Davis <afd@ti.com> >> +Description: >> + Enable or disable separate settings for the TransImpedance >> + Amplifier above, when disabled both values are set by the >> + first channel. >> + >> +What: /sys/bus/iio/devices/iio:deviceX/in_intensity_ledY_raw >> + /sys/bus/iio/devices/iio:deviceX/in_intensity_ledY_ambient_raw >> +Date: December 2015 >> +KernelVersion: >> +Contact: Andrew F. Davis <afd@ti.com> >> +Description: >> + Get measured values from the ADC for these stages. Y is the >> + specific LED number. The values are expressed in 24-bit twos >> + complement. >> + >> +What: /sys/bus/iio/devices/iio:deviceX/in_intensity_ledY-ledY_ambient_raw >> +Date: December 2015 >> +KernelVersion: >> +Contact: Andrew F. Davis <afd@ti.com> >> +Description: >> + Get differential values from the ADC for these stages. Y is the >> + specific LED number. The values are expressed in 24-bit twos >> + complement for the specified LEDs. >> + >> +What: /sys/bus/iio/devices/iio:deviceX/out_current_ledY_offset >> + /sys/bus/iio/devices/iio:deviceX/out_current_ledY_ambient_offset >> +Date: December 2015 >> +KernelVersion: >> +Contact: Andrew F. Davis <afd@ti.com> >> +Description: >> + Get and set the offset cancellation DAC setting for these >> + stages. The values are expressed in 5-bit sign–magnitude. >> + >> +What: /sys/bus/iio/devices/iio:deviceX/out_current_ledY_raw >> +Date: December 2015 >> +KernelVersion: >> +Contact: Andrew F. Davis <afd@ti.com> >> +Description: >> + Get and set the LED current for the specified LED. Y is the >> + specific LED number. >> + Values range from 0 -> 63. Current is calculated by >> + current = value * 0.8mA > Otherwise, we both know that the ABI is less than ideal, but it's probably > the best solution we can easily manage so lets go with this. > While it's not easy to revise this in future it can be done should we > end up with a more generic way of representing the complex cases seen here > (no idea what that might be!) >
Sounds good.
>> 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" > I'll merge this as appropriate given we now have a health > driver in tree (the simpler maxim one that got going after this one).
ACK
>> + >> +menu "Heart Rate Monitors" >> + >> +config AFE4404 >> + tristate "TI AFE4404 Heart Rate Monitor" >> + depends on I2C >> + select REGMAP_I2C >> + select IIO_BUFFER >> + select IIO_TRIGGERED_BUFFER >> + help >> + Say yes to choose the Texas Instruments AFE4404 >> + heart rate monitor and low-cost pulse oximeter. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called afe4404. >> + >> +endmenu >> + >> +endmenu >> diff --git a/drivers/iio/health/Makefile b/drivers/iio/health/Makefile >> new file mode 100644 >> index 0000000..c108c8d >> --- /dev/null >> +++ b/drivers/iio/health/Makefile >> @@ -0,0 +1,6 @@ >> +# >> +# Makefile for IIO Health drivers >> +# >> +# When adding new entries keep the list in alphabetical order >> + >> +obj-$(CONFIG_AFE4404) += afe4404.o >> diff --git a/drivers/iio/health/afe4404.c b/drivers/iio/health/afe4404.c >> new file mode 100644 >> index 0000000..d199a35 >> --- /dev/null >> +++ b/drivers/iio/health/afe4404.c >> @@ -0,0 +1,700 @@
[...]
>> +static irqreturn_t afe440x_trigger_handler(int irq, void *private) >> +{ >> + struct iio_poll_func *pf = private; >> + struct iio_dev *indio_dev = pf->indio_dev; >> + struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev); >> + int ret, bit, i = 0; >> + s32 buffer[12]; > 7 input channels, align to 64bytes + one timestamp = 10 not 12? > I may well be missing something or losing the ability to count! >
Left over from when we had 9 channels, will fix, your counting abilities are still present :)
[...]
>> + >> +static int afe440x_suspend(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, >> + AFE440X_CONTROL2_PDN_AFE); >> + if (ret) >> + return ret; >> + >> + ret = regulator_disable(afe440x->regulator); >> + if (ret) { >> + dev_err(dev, "Unable to disable regulator\n"); >> + return ret; >> + } > Might get a static checker pointing out that a successful regulator_disable > returns 0 anyway so you might as well have a single > return ret; here. I don't care either way other than reducing noise > from those autobuilds.
I didn't get any warnings with mine, so if you don't care either way I would prefer this way as it feels more consistent to me, we also can see the successful return value without tracking down what the last 'ret' assignment considers success.
>> + >> + 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; >> + >> + ret = regulator_enable(afe440x->regulator); >> + if (ret) { >> + dev_err(dev, "Unable to enable regulator\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +SIMPLE_DEV_PM_OPS(afe440x_pm_ops, afe440x_suspend, afe440x_resume); >> + > This looks pretty good and clean though not quite traditional from > an IIO driver point of view (separate private data allocation) > > I'd personally have had the devm_iio_device_alloc in the main probe > function then populated it in this function. However, > I can happily live with this slight varient as it is easy enough > to follow with your nice const info structure approach. >
The rational for this should be more clear when I break off all the common code that this allows us.
>> +static int afe440x_iio_setup(struct afe440x_data *afe440x, >> + const struct afe440x_info *afe440x_info) >> +{ >> + 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_info->channels; >> + indio_dev->num_channels = afe440x_info->num_channels; >> + indio_dev->name = afe440x_info->name; >> + indio_dev->info = afe440x_info->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; >> + >> + ret = iio_trigger_register(afe440x->trig); >> + if (ret) { >> + dev_err(afe440x->dev, "Unable to register IIO trigger\n"); >> + return ret; >> + } >> + >> + ret = devm_request_threaded_irq(afe440x->dev, afe440x->irq, >> + iio_trigger_generic_data_rdy_poll, >> + NULL, IRQF_ONESHOT, >> + afe440x_info->name, >> + afe440x->trig); >> + if (ret) { >> + dev_err(afe440x->dev, "Unable to request IRQ\n"); >> + return ret; >> + } >> + } >> + >> + ret = iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time, >> + &afe440x_trigger_handler, NULL); >> + if (ret) { >> + dev_err(afe440x->dev, "Unable to setup buffer\n"); >> + return ret; >> + } >> + >> + ret = devm_iio_device_register(afe440x->dev, indio_dev); > Ah. It took a lot of persuading to get me to allow the devm version > of this function in the first place. It is only appropriate for > very simple devices where there is nothing to turn off. The issue > is that it creates a race condition in the remove function (see below). >> + if (ret) { >> + dev_err(afe440x->dev, "Unable to register IIO device\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static const struct afe440x_info afe4404_info = { >> + .name = AFE4404_DRIVER_NAME, >> + .channels = afe4404_channels, >> + .num_channels = ARRAY_SIZE(afe4404_channels), >> + .info = &afe440x_iio_info, >> +}; >> + >> +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; >> + >> + 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); >> + } >> + ret = regulator_enable(afe440x->regulator); >> + if (ret) { >> + dev_err(afe440x->dev, "Unable to enable regulator\n"); >> + return ret; >> + } >> + >> + 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; >> + } >> + >> + return afe440x_iio_setup(afe440x, &afe4404_info); >> +} >> + >> +static int afe4404_remove(struct i2c_client *client) >> +{ >> + struct afe440x_data *afe440x = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = regulator_disable(afe440x->regulator); >> + if (ret) { >> + dev_err(afe440x->dev, "Unable to disable regulator\n"); >> + return ret; >> + } > As the iio_device_unregister does not occur until after this function the > result is that we turn this regulator off whilst the userspace / kernelspace > interfaces to the device are still active. Hence you can get a read in > the meantime and a rather unpredictable result. > > The sad result of this is you need to not use the devm_ version of > iio_device_register but instead the unmanaged one and remove it by > hand at the start of the remove function. Now if there was a managed > regulator enable then you could do it all with managed interfaces > and not have a remove function at all, but there are probably good > reasons why that doesn't exist.
Possibly similar reason to this, will fix.
>> + >> + return 0; >> +} >> + >> +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, >> + .remove = afe4404_remove, >> + .id_table = afe4404_ids, >> +}; >> +module_i2c_driver(afe4404_i2c_driver); >> + >> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>"); >> +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..575e528 >> --- /dev/null >> +++ b/drivers/iio/health/afe440x.h >> @@ -0,0 +1,202 @@ >> +/* >> + * AFE440X Heart Rate Monitors and Low-Cost Pulse Oximeters >> + * >> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/ >> + * Andrew F. Davis <afd@ti.com> >> + * >> + * 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. >> + */ >> + >> +#ifndef _AFE440X_H >> +#define _AFE440X_H >> + >> +/* 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 > Just a small point, but you have some of these redefined > in the afe4403 drivers additional header. > Now the contents is different but the register numbering > matches. I'd be tempted to make the difference more > obvious by renaming the AFE4404 versions without the X where > their contents is considerably different (even if they have the > same address.. > > I'd also push them down into the C file so only shared elements > across the two drivers are defined here. > > Just makes the scope of the definitions more obvious. >
This was my original intent, but it looks like some have gotten away from me, I'll see what I can do to clean this up.
Thanks, Andrew
| |