lkml.org 
[lkml]   [2015]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 3/5] iio: health: Add driver for the TI AFE4404 heart monitor
From
Date
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


\
 
 \ /
  Last update: 2015-12-30 19:01    [W:0.048 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site