Re: [PATCH v3 3/5] iio: health: Add driver for the TI AFE4404 heart monitor

["Andrew F. Davis" <afd-l0cyMroinI0@public.gmane.org>]

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-l0cyMroinI0@public.gmane.org>
> 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-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.
>> +		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-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.
>> +
>> +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-l0cyMroinI0@public.gmane.org>
>> +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-l0cyMroinI0@public.gmane.org>
>> +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-l0cyMroinI0@public.gmane.org>
>> +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-l0cyMroinI0@public.gmane.org>
>> +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-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..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-l0cyMroinI0@public.gmane.org>
>> + *
>> + * 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