Re: [PATCH v2 1/2] Documentation: afe4404: Add DT bindings for the AFE4404 heart monitor

Re: [PATCH v2 1/2] Documentation: afe4404: Add DT bindings for the AFE4404 heart monitor

[Jonathan Cameron]

On 02/12/15 19:57, Andrew F. Davis wrote:
> Add the TI afe4404 heart monitor DT bindings documentation.
> Create health directory created under iio.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
One little point inline - otherwise this is straightforward and
looks good to me.

Could add cross references to the interrupt and gpio docs.

Jonathan
> ---
>  .../devicetree/bindings/iio/health/afe4404.txt     | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/health/afe4404.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/health/afe4404.txt b/Documentation/devicetree/bindings/iio/health/afe4404.txt
> new file mode 100644
> index 0000000..25b77a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/health/afe4404.txt
> @@ -0,0 +1,27 @@
> +* Texas Instruments AFE4404 Heart rate and Pulse Oximeter
> +
> +Required properties:
> + - compatible		: Should be "ti,afe4404".
> + - reg			: I2C address of the device.
> + - tx_sup-supply	: Regulator supply to LEDs.
If the sup bit is for supply I'd drop it and just have
tx-supply.
> + - interrupt-parent	: Phandle to he parent interrupt controller.
> + - interrupts		: The interrupt line the device ADC_RDY pin is connected to.
> +
> +Optional properties:
> + - reset-gpios		: GPIO used to reset the device.
> +
> +Example:
> +
> +&i2c2 {
> +	heart_mon@58 {
> +		compatible = "ti,afe4404";
> +		reg = <0x58>;
> +
> +		tx_sup-supply = <&vbat>;
> +
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <28 IRQ_TYPE_EDGE_RISING>;
> +
> +		reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +	};
> +};
> 

Re: [PATCH v2 1/2] Documentation: afe4404: Add DT bindings for the AFE4404 heart monitor

[Andrew F. Davis]

On 12/05/2015 11:38 AM, Jonathan Cameron wrote:
> On 02/12/15 19:57, Andrew F. Davis wrote:
>> Add the TI afe4404 heart monitor DT bindings documentation.
>> Create health directory created under iio.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
> One little point inline - otherwise this is straightforward and
> looks good to me.
>
> Could add cross references to the interrupt and gpio docs.
>

Will add.

> Jonathan
>> ---
>>   .../devicetree/bindings/iio/health/afe4404.txt     | 27 ++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/health/afe4404.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/health/afe4404.txt b/Documentation/devicetree/bindings/iio/health/afe4404.txt
>> new file mode 100644
>> index 0000000..25b77a4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/health/afe4404.txt
>> @@ -0,0 +1,27 @@
>> +* Texas Instruments AFE4404 Heart rate and Pulse Oximeter
>> +
>> +Required properties:
>> + - compatible		: Should be "ti,afe4404".
>> + - reg			: I2C address of the device.
>> + - tx_sup-supply	: Regulator supply to LEDs.
> If the sup bit is for supply I'd drop it and just have
> tx-supply.

It is, the name of the pin in the datasheet is "tx_sup", so
I went with that.

>> + - interrupt-parent	: Phandle to he parent interrupt controller.
>> + - interrupts		: The interrupt line the device ADC_RDY pin is connected to.
>> +
>> +Optional properties:
>> + - reset-gpios		: GPIO used to reset the device.
>> +
>> +Example:
>> +
>> +&i2c2 {
>> +	heart_mon@58 {
>> +		compatible = "ti,afe4404";
>> +		reg = <0x58>;
>> +
>> +		tx_sup-supply = <&vbat>;
>> +
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <28 IRQ_TYPE_EDGE_RISING>;
>> +
>> +		reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>> +	};
>> +};
>>
>

Re: [PATCH v2 1/2] Documentation: afe4404: Add DT bindings for the AFE4404 heart monitor

[Jonathan Cameron]

On 05/12/15 17:53, Andrew F. Davis wrote:
> On 12/05/2015 11:38 AM, Jonathan Cameron wrote:
>> On 02/12/15 19:57, Andrew F. Davis wrote:
>>> Add the TI afe4404 heart monitor DT bindings documentation.
>>> Create health directory created under iio.
>>>
>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> One little point inline - otherwise this is straightforward and
>> looks good to me.
>>
>> Could add cross references to the interrupt and gpio docs.
>>
> 
> Will add.
> 
>> Jonathan
>>> ---
>>>   .../devicetree/bindings/iio/health/afe4404.txt     | 27 ++++++++++++++++++++++
>>>   1 file changed, 27 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/iio/health/afe4404.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/health/afe4404.txt b/Documentation/devicetree/bindings/iio/health/afe4404.txt
>>> new file mode 100644
>>> index 0000000..25b77a4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/health/afe4404.txt
>>> @@ -0,0 +1,27 @@
>>> +* Texas Instruments AFE4404 Heart rate and Pulse Oximeter
>>> +
>>> +Required properties:
>>> + - compatible        : Should be "ti,afe4404".
>>> + - reg            : I2C address of the device.
>>> + - tx_sup-supply    : Regulator supply to LEDs.
>> If the sup bit is for supply I'd drop it and just have
>> tx-supply.
> 
> It is, the name of the pin in the datasheet is "tx_sup", so
> I went with that.
Drop it - I suspect the device tree guys would prefer it without
and it's not hard to make the connection with the datasheet name!
> 
>>> + - interrupt-parent    : Phandle to he parent interrupt controller.
>>> + - interrupts        : The interrupt line the device ADC_RDY pin is connected to.
>>> +
>>> +Optional properties:
>>> + - reset-gpios        : GPIO used to reset the device.
>>> +
>>> +Example:
>>> +
>>> +&i2c2 {
>>> +    heart_mon@58 {
>>> +        compatible = "ti,afe4404";
>>> +        reg = <0x58>;
>>> +
>>> +        tx_sup-supply = <&vbat>;
>>> +
>>> +        interrupt-parent = <&gpio1>;
>>> +        interrupts = <28 IRQ_TYPE_EDGE_RISING>;
>>> +
>>> +        reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>>> +    };
>>> +};
>>>
>>

Re: [PATCH v2 2/2] iio: health: Add driver for the TI AFE4404 heart monitor

[Jonathan Cameron]

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 <afd@ti.com>
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
> ---
>  .../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.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..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@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.
> +		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 ;)
> +
> +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.
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.

> 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
> +	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..d36c1d9
> --- /dev/null
> +++ b/drivers/iio/health/afe4404.c
> @@ -0,0 +1,619 @@
> +/*
> + * AFE4404 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.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#include "afe440x.h"
> +
> +#define AFE4404_DRIVER_NAME		"afe4404"
> +
> +/* AFE4404 registers */
> +#define AFE4404_TIA_GAIN_SEP		0x20
> +#define AFE4404_TIA_GAIN		0x21
> +#define AFE4404_PROG_TG_STC		0x34
> +#define AFE4404_PROG_TG_ENDC		0x35
> +#define AFE4404_LED3LEDSTC		0x36
> +#define AFE4404_LED3LEDENDC		0x37
> +#define AFE4404_CLKDIV_PRF		0x39
> +#define AFE4404_OFFDAC			0x3a
> +#define AFE4404_DEC			0x3d
> +#define AFE4404_AVG_LED2_ALED2VAL	0x3f
> +#define AFE4404_AVG_LED1_ALED1VAL	0x40
> +
> +/* AFE4404 GAIN register fields */
> +#define AFE4404_TIA_GAIN_RES_MASK	GENMASK(2, 0)
> +#define AFE4404_TIA_GAIN_RES_SHIFT	0
> +#define AFE4404_TIA_GAIN_CAP_MASK	GENMASK(5, 3)
> +#define AFE4404_TIA_GAIN_CAP_SHIFT	3
> +
> +/* AFE4404 LEDCNTRL register fields */
> +#define AFE4404_LEDCNTRL_ILED1_MASK	GENMASK(5, 0)
> +#define AFE4404_LEDCNTRL_ILED1_SHIFT	0
> +#define AFE4404_LEDCNTRL_ILED2_MASK	GENMASK(11, 6)
> +#define AFE4404_LEDCNTRL_ILED2_SHIFT	6
> +#define AFE4404_LEDCNTRL_ILED3_MASK	GENMASK(17, 12)
> +#define AFE4404_LEDCNTRL_ILED3_SHIFT	12
> +
> +/* AFE4404 CONTROL2 register fields */
> +#define AFE440X_CONTROL2_ILED_2X_MASK	BIT(17)
> +#define AFE440X_CONTROL2_ILED_2X_SHIFT	17
> +
> +/* AFE4404 CONTROL3 register fields */
> +#define AFE440X_CONTROL3_OSC_ENABLE	BIT(9)
> +
> +/* AFE4404 OFFDAC register current fields */
> +#define AFE4404_OFFDAC_CURR_LED1_MASK	GENMASK(9, 5)
> +#define AFE4404_OFFDAC_CURR_LED1_SHIFT	5
> +#define AFE4404_OFFDAC_CURR_LED2_MASK	GENMASK(19, 15)
> +#define AFE4404_OFFDAC_CURR_LED2_SHIFT	15
> +#define AFE4404_OFFDAC_CURR_LED3_MASK	GENMASK(4, 0)
> +#define AFE4404_OFFDAC_CURR_LED3_SHIFT	0
> +#define AFE4404_OFFDAC_CURR_ALED1_MASK	GENMASK(14, 10)
> +#define AFE4404_OFFDAC_CURR_ALED1_SHIFT	10
> +#define AFE4404_OFFDAC_CURR_ALED2_MASK	GENMASK(4, 0)
> +#define AFE4404_OFFDAC_CURR_ALED2_SHIFT	0
> +
> +/* AFE4404 NULL fields */
> +#define NULL_MASK	0
> +#define NULL_SHIFT	0
> +
> +/* AFE4404 TIA_GAIN_CAP values */
> +#define AFE4404_TIA_GAIN_CAP_5_P	0x0
> +#define AFE4404_TIA_GAIN_CAP_2_5_P	0x1
> +#define AFE4404_TIA_GAIN_CAP_10_P	0x2
> +#define AFE4404_TIA_GAIN_CAP_7_5_P	0x3
> +#define AFE4404_TIA_GAIN_CAP_20_P	0x4
> +#define AFE4404_TIA_GAIN_CAP_17_5_P	0x5
> +#define AFE4404_TIA_GAIN_CAP_25_P	0x6
> +#define AFE4404_TIA_GAIN_CAP_22_5_P	0x7
> +
> +/* AFE4404 TIA_GAIN_RES values */
> +#define AFE4404_TIA_GAIN_RES_500_K	0x0
> +#define AFE4404_TIA_GAIN_RES_250_K	0x1
> +#define AFE4404_TIA_GAIN_RES_100_K	0x2
> +#define AFE4404_TIA_GAIN_RES_50_K	0x3
> +#define AFE4404_TIA_GAIN_RES_25_K	0x4
> +#define AFE4404_TIA_GAIN_RES_10_K	0x5
> +#define AFE4404_TIA_GAIN_RES_1_M	0x6
> +#define AFE4404_TIA_GAIN_RES_2_M	0x7
> +
> +enum afe4404_chan_id {
> +	LED1,
> +	ALED1,
> +	LED2,
> +	ALED2,
> +	LED3,
> +	LED1_ALED1,
> +	LED2_ALED2,
> +	AVG_LED1_ALED1,
> +	AVG_LED2_ALED2,
> +	ILED1,
> +	ILED2,
> +	ILED3,
> +};
> +
> +#define AFE4404_REG_INFO(_id, _reg, _offreg, _sm)		\
> +	[_id] = {						\
> +		.reg = _reg,					\
> +		.offreg = _offreg,				\
> +		.shift = _sm ## _SHIFT,				\
> +		.mask = _sm ## _MASK,				\
> +	}
> +
> +struct {
> +	unsigned int reg;
> +	unsigned int offreg;
> +	unsigned int shift;
> +	unsigned int mask;
> +} reg_info[] = {
> +	AFE4404_REG_INFO(LED1, AFE440X_LED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED1),
> +	AFE4404_REG_INFO(ALED1, AFE440X_ALED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED1),
> +	AFE4404_REG_INFO(LED2, AFE440X_LED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED2),
> +	AFE4404_REG_INFO(ALED2, AFE440X_ALED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED2),
> +	AFE4404_REG_INFO(LED3, AFE440X_ALED2VAL, 0, NULL),
> +	AFE4404_REG_INFO(LED1_ALED1, AFE440X_LED1_ALED1VAL, 0, NULL),
> +	AFE4404_REG_INFO(LED2_ALED2, AFE440X_LED2_ALED2VAL, 0, NULL),
> +	AFE4404_REG_INFO(AVG_LED1_ALED1, AFE4404_AVG_LED1_ALED1VAL, 0, NULL),
> +	AFE4404_REG_INFO(AVG_LED2_ALED2, AFE4404_AVG_LED2_ALED2VAL, 0, NULL),
> +	AFE4404_REG_INFO(ILED1, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED1),
> +	AFE4404_REG_INFO(ILED2, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED2),
> +	AFE4404_REG_INFO(ILED3, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED3),
> +};
> +
> +static const struct iio_chan_spec afe4404_channels[] = {
> +	/* ADC values */
> +	AFE440X_INTENSITY_CHAN(LED1, "led1", BIT(IIO_CHAN_INFO_OFFSET)),
> +	AFE440X_INTENSITY_CHAN(ALED1, "led1_ambient", BIT(IIO_CHAN_INFO_OFFSET)),
(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)
> +	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?
> +	/* 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.

> +};
> +
> +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, &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, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	reg_val &= ~afe440x_reg->mask;
> +	reg_val |= ((val << afe440x_reg->shift) & afe440x_reg->mask);
> +
> +	ret = regmap_write(afe440x->regmap, afe440x_reg->reg, reg_val);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +AFE440X_ATTR(tia_separate_en, AFE4404_TIA_GAIN_SEP, AFE440X_TIAGAIN_ENSEPGAIN);
> +
> +AFE440X_ATTR(tia_resistance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES);
> +AFE440X_ATTR(tia_capacitance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_CAP);
> +
> +AFE440X_ATTR(tia_resistance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_RES);
> +AFE440X_ATTR(tia_capacitance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_CAP);
> +
> +static struct attribute *afe4404_attributes[] = {
> +	&afe440x_reg_tia_separate_en.dev_attr.attr,
> +	&afe440x_reg_tia_resistance1.dev_attr.attr,
> +	&afe440x_reg_tia_capacitance1.dev_attr.attr,
> +	&afe440x_reg_tia_resistance2.dev_attr.attr,
> +	&afe440x_reg_tia_capacitance2.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group afe4404_attribute_group = {
> +	.attrs = afe4404_attributes
> +};
> +
> +static int afe440x_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
> +	int ret;
> +
> +	switch (chan->type) {
> +	case IIO_INTENSITY:
> +		switch (mask) {
> +		case IIO_CHAN_INFO_RAW:
> +			ret = regmap_read(afe440x->regmap,
> +					  reg_info[chan->address].reg, val);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +		case IIO_CHAN_INFO_OFFSET:
> +			ret = regmap_read(afe440x->regmap,
> +					  reg_info[chan->address].offreg, val);
> +			if (ret)
> +				return ret;
> +			*val &= reg_info[chan->address].mask;
> +			*val >>= reg_info[chan->address].shift;
> +			return IIO_VAL_INT;
> +		}
> +		break;
> +	case IIO_CURRENT:
> +		switch (mask) {
> +		case IIO_CHAN_INFO_RAW:
> +			ret = regmap_read(afe440x->regmap,
> +					  reg_info[chan->address].reg, val);
> +			if (ret)
> +				return ret;
> +			*val &= reg_info[chan->address].mask;
> +			*val >>= reg_info[chan->address].shift;
> +			return IIO_VAL_INT;
> +		case IIO_CHAN_INFO_SCALE:
> +			*val = 0;
> +			*val2 = 800000;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int afe440x_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
> +	unsigned int reg_val;
> +	int ret;
> +
> +	switch (chan->type) {
> +	case IIO_INTENSITY:
> +		switch (mask) {
> +		case IIO_CHAN_INFO_OFFSET:
> +			ret = regmap_read(afe440x->regmap,
> +					  reg_info[chan->address].offreg,
> +					  &reg_val);
> +			if (ret)
> +				return ret;
> +			reg_val &= ~reg_info[chan->address].mask;
> +			reg_val |= ((val << reg_info[chan->address].shift) &
> +					reg_info[chan->address].mask);
> +			return regmap_write(afe440x->regmap,
> +					    reg_info[chan->address].offreg,
> +					    reg_val);
> +		}
> +		break;
> +	case IIO_CURRENT:
> +		switch (mask) {
> +		case IIO_CHAN_INFO_RAW:
> +			ret = regmap_read(afe440x->regmap,
> +					  reg_info[chan->address].reg,
> +					  &reg_val);
> +			if (ret)
> +				return ret;
> +			reg_val &= ~reg_info[chan->address].mask;
> +			reg_val |= ((val << reg_info[chan->address].shift) &
> +					reg_info[chan->address].mask);
> +			return regmap_write(afe440x->regmap,
> +					    reg_info[chan->address].reg,
> +					    reg_val);
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info afe4404_iio_info = {
> +	.attrs	= &afe4404_attribute_group,
> +	.read_raw = afe440x_read_raw,
> +	.write_raw = afe440x_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +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, reg, i = 0;
> +	s32 buffer[12];
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		reg = reg_info[bit].reg;
why the local reg variable? Stick it dirrectly in the functional call.
> +		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	},	\
> +	{ AFE440X_ADCRSTENDCT2,	6542	},	\
> +	{ AFE440X_LED1CONVST,	6543	},	\
> +	{ AFE440X_LED1CONVEND,	7003	},	\
> +	{ AFE440X_ALED1STC,	1280	},	\
> +	{ AFE440X_ALED1ENDC,	1598	},	\
> +	{ AFE440X_ADCRSTSTCT3,	7005	},	\
> +	{ AFE440X_ADCRSTENDCT3,	7011	},	\
> +	{ AFE440X_ALED1CONVST,	7012	},	\
> +	{ AFE440X_ALED1CONVEND,	7471	},	\
> +	{ AFE440X_PDNCYCLESTC,	7671	},	\
> +	{ AFE440X_PDNCYCLEENDC,	39199	}
> +
> +static const struct reg_sequence afe4404_reg_sequences[] = {
> +	AFE4404_TIMING_PAIRS,
> +	{ AFE440X_CONTROL1, AFE440X_CONTROL1_TIMEREN },
> +	{ AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES_50_K },
> +	{ AFE440X_LEDCNTRL, (0xf << AFE4404_LEDCNTRL_ILED1_SHIFT) |
> +			    (0x3 << AFE4404_LEDCNTRL_ILED2_SHIFT) |
> +			    (0x3 << AFE4404_LEDCNTRL_ILED3_SHIFT) },
> +	{ AFE440X_CONTROL2, AFE440X_CONTROL3_OSC_ENABLE	},
> +};
> +
> +static const struct regmap_range afe4404_yes_ranges[] = {
> +	regmap_reg_range(AFE440X_LED2VAL, AFE440X_LED1_ALED1VAL),
> +	regmap_reg_range(AFE4404_AVG_LED2_ALED2VAL, AFE4404_AVG_LED1_ALED1VAL),
> +};
> +
> +static const struct regmap_access_table afe4404_volatile_table = {
> +	.yes_ranges = afe4404_yes_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(afe4404_yes_ranges),
> +};
> +
> +static const struct regmap_config afe4404_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 24,
> +
> +	.max_register = AFE4404_AVG_LED1_ALED1VAL,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_table = &afe4404_volatile_table,
> +};
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id afe4404_of_match[] = {
> +	{ .compatible = "ti,afe4404", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, afe4404_of_match);
> +#endif
> +
> +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, "Failed to disable regulator\n");
> +		return ret;
> +	}
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!)
> +
> +	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...
> +
> +	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?
> +
> +	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;
> +
> +		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->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);
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!
> +	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.
> +
> +	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!

> +
> +	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)
> +	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@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..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@ti.com>
> + *
> + * Copyright: (C) 2015 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.
> + */
> +
> +#ifndef _AFE440X_H
> +#define _AFE440X_H
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.
> +
> +/* 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
> +#define AFE440X_LED1VAL			0x2c
> +#define AFE440X_ALED1VAL		0x2d
> +#define AFE440X_LED2_ALED2VAL		0x2e
> +#define AFE440X_LED1_ALED1VAL		0x2f
> +#define AFE440X_CONTROL3		0x31
> +#define AFE440X_PDNCYCLESTC		0x32
> +#define AFE440X_PDNCYCLEENDC		0x33
> +
> +/* CONTROL0 register fields */
> +#define AFE440X_CONTROL0_REG_READ	BIT(0)
> +#define AFE440X_CONTROL0_TM_COUNT_RST	BIT(1)
> +#define AFE440X_CONTROL0_SW_RESET	BIT(3)
> +
> +/* CONTROL1 register fields */
> +#define AFE440X_CONTROL1_TIMEREN	BIT(8)
> +
> +/* TIAGAIN register fields */
> +#define AFE440X_TIAGAIN_ENSEPGAIN_MASK	BIT(15)
> +#define AFE440X_TIAGAIN_ENSEPGAIN_SHIFT	15
> +
> +/* CONTROL2 register fields */
> +#define AFE440X_CONTROL2_PDN_AFE	BIT(0)
> +#define AFE440X_CONTROL2_PDN_RX		BIT(1)
> +#define AFE440X_CONTROL2_DYNAMIC4	BIT(3)
> +#define AFE440X_CONTROL2_DYNAMIC3	BIT(4)
> +#define AFE440X_CONTROL2_DYNAMIC2	BIT(14)
> +#define AFE440X_CONTROL2_DYNAMIC1	BIT(20)
> +
> +/* CONTROL3 register fields */
> +#define AFE440X_CONTROL3_CLKDIV		GENMASK(2, 0)
> +
> +/* CONTROL0 values */
> +#define AFE440X_CONTROL0_WRITE		0x0
> +#define AFE440X_CONTROL0_READ		0x1
> +
> +#define AFE440X_INTENSITY_CHAN(_index, _name, _mask)		\
> +	{							\
> +		.type = IIO_INTENSITY,				\
> +		.channel = _index,				\
> +		.address = _index,				\
> +		.scan_index = _index,				\
> +		.scan_type = {					\
> +				.sign = 's',			\
> +				.realbits = 24,			\
> +				.storagebits = 32,		\
> +				.endianness = IIO_CPU,		\
> +		},						\
> +		.extend_name = _name,				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +			_mask,					\
> +	}
> +
> +#define AFE440X_CURRENT_CHAN(_index, _name)			\
> +	{							\
> +		.type = IIO_CURRENT,				\
> +		.channel = _index,				\
> +		.address = _index,				\
> +		.scan_index = _index,				\
> +		.extend_name = _name,				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +			BIT(IIO_CHAN_INFO_SCALE),		\
> +		.output = true,					\
> +	}
I'd particularly like to see the above two definitions next to the
arrays they are used to instantiate - makes for easier review!

> +
> +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 
> +
> +/**
> + * 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).

> + * @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.
> +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 */
> 

Re: [PATCH v2 2/2] iio: health: Add driver for the TI AFE4404 heart monitor

[Andrew F. Davis]

On 12/05/2015 12:21 PM, Jonathan Cameron wrote:
> 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 <afd@ti.com>
> 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
>> ---
>>   .../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.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..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@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.
>> +		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 ;)

I'm not sure if there is a clean way to do this, these are not channels
so parsing input will need checks based on what kind of register and value
we send in, these are raw input to the registers for other registers, I
changed the name to attempt to make it clear to users these are not
channels but raw registers (out_resistance1_raw -> tia_resistance1).

I could still make this more clear in the ABI doc:

	Valid capacitance settings are 0 -> 7 which correspond to
	5pF, 2.5pF, 10pF, 7.5pF, 20pF, 17.5pF, 25pF, and 22.5pF
	respectively.

or something similar.

>> +
>> +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.
> 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.
>

Works for me, I'll add them back.

>> 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
>> +	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..d36c1d9
>> --- /dev/null
>> +++ b/drivers/iio/health/afe4404.c
>> @@ -0,0 +1,619 @@
>> +/*
>> + * AFE4404 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.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +
>> +#include "afe440x.h"
>> +
>> +#define AFE4404_DRIVER_NAME		"afe4404"
>> +
>> +/* AFE4404 registers */
>> +#define AFE4404_TIA_GAIN_SEP		0x20
>> +#define AFE4404_TIA_GAIN		0x21
>> +#define AFE4404_PROG_TG_STC		0x34
>> +#define AFE4404_PROG_TG_ENDC		0x35
>> +#define AFE4404_LED3LEDSTC		0x36
>> +#define AFE4404_LED3LEDENDC		0x37
>> +#define AFE4404_CLKDIV_PRF		0x39
>> +#define AFE4404_OFFDAC			0x3a
>> +#define AFE4404_DEC			0x3d
>> +#define AFE4404_AVG_LED2_ALED2VAL	0x3f
>> +#define AFE4404_AVG_LED1_ALED1VAL	0x40
>> +
>> +/* AFE4404 GAIN register fields */
>> +#define AFE4404_TIA_GAIN_RES_MASK	GENMASK(2, 0)
>> +#define AFE4404_TIA_GAIN_RES_SHIFT	0
>> +#define AFE4404_TIA_GAIN_CAP_MASK	GENMASK(5, 3)
>> +#define AFE4404_TIA_GAIN_CAP_SHIFT	3
>> +
>> +/* AFE4404 LEDCNTRL register fields */
>> +#define AFE4404_LEDCNTRL_ILED1_MASK	GENMASK(5, 0)
>> +#define AFE4404_LEDCNTRL_ILED1_SHIFT	0
>> +#define AFE4404_LEDCNTRL_ILED2_MASK	GENMASK(11, 6)
>> +#define AFE4404_LEDCNTRL_ILED2_SHIFT	6
>> +#define AFE4404_LEDCNTRL_ILED3_MASK	GENMASK(17, 12)
>> +#define AFE4404_LEDCNTRL_ILED3_SHIFT	12
>> +
>> +/* AFE4404 CONTROL2 register fields */
>> +#define AFE440X_CONTROL2_ILED_2X_MASK	BIT(17)
>> +#define AFE440X_CONTROL2_ILED_2X_SHIFT	17
>> +
>> +/* AFE4404 CONTROL3 register fields */
>> +#define AFE440X_CONTROL3_OSC_ENABLE	BIT(9)
>> +
>> +/* AFE4404 OFFDAC register current fields */
>> +#define AFE4404_OFFDAC_CURR_LED1_MASK	GENMASK(9, 5)
>> +#define AFE4404_OFFDAC_CURR_LED1_SHIFT	5
>> +#define AFE4404_OFFDAC_CURR_LED2_MASK	GENMASK(19, 15)
>> +#define AFE4404_OFFDAC_CURR_LED2_SHIFT	15
>> +#define AFE4404_OFFDAC_CURR_LED3_MASK	GENMASK(4, 0)
>> +#define AFE4404_OFFDAC_CURR_LED3_SHIFT	0
>> +#define AFE4404_OFFDAC_CURR_ALED1_MASK	GENMASK(14, 10)
>> +#define AFE4404_OFFDAC_CURR_ALED1_SHIFT	10
>> +#define AFE4404_OFFDAC_CURR_ALED2_MASK	GENMASK(4, 0)
>> +#define AFE4404_OFFDAC_CURR_ALED2_SHIFT	0
>> +
>> +/* AFE4404 NULL fields */
>> +#define NULL_MASK	0
>> +#define NULL_SHIFT	0
>> +
>> +/* AFE4404 TIA_GAIN_CAP values */
>> +#define AFE4404_TIA_GAIN_CAP_5_P	0x0
>> +#define AFE4404_TIA_GAIN_CAP_2_5_P	0x1
>> +#define AFE4404_TIA_GAIN_CAP_10_P	0x2
>> +#define AFE4404_TIA_GAIN_CAP_7_5_P	0x3
>> +#define AFE4404_TIA_GAIN_CAP_20_P	0x4
>> +#define AFE4404_TIA_GAIN_CAP_17_5_P	0x5
>> +#define AFE4404_TIA_GAIN_CAP_25_P	0x6
>> +#define AFE4404_TIA_GAIN_CAP_22_5_P	0x7
>> +
>> +/* AFE4404 TIA_GAIN_RES values */
>> +#define AFE4404_TIA_GAIN_RES_500_K	0x0
>> +#define AFE4404_TIA_GAIN_RES_250_K	0x1
>> +#define AFE4404_TIA_GAIN_RES_100_K	0x2
>> +#define AFE4404_TIA_GAIN_RES_50_K	0x3
>> +#define AFE4404_TIA_GAIN_RES_25_K	0x4
>> +#define AFE4404_TIA_GAIN_RES_10_K	0x5
>> +#define AFE4404_TIA_GAIN_RES_1_M	0x6
>> +#define AFE4404_TIA_GAIN_RES_2_M	0x7
>> +
>> +enum afe4404_chan_id {
>> +	LED1,
>> +	ALED1,
>> +	LED2,
>> +	ALED2,
>> +	LED3,
>> +	LED1_ALED1,
>> +	LED2_ALED2,
>> +	AVG_LED1_ALED1,
>> +	AVG_LED2_ALED2,
>> +	ILED1,
>> +	ILED2,
>> +	ILED3,
>> +};
>> +
>> +#define AFE4404_REG_INFO(_id, _reg, _offreg, _sm)		\
>> +	[_id] = {						\
>> +		.reg = _reg,					\
>> +		.offreg = _offreg,				\
>> +		.shift = _sm ## _SHIFT,				\
>> +		.mask = _sm ## _MASK,				\
>> +	}
>> +
>> +struct {
>> +	unsigned int reg;
>> +	unsigned int offreg;
>> +	unsigned int shift;
>> +	unsigned int mask;
>> +} reg_info[] = {
>> +	AFE4404_REG_INFO(LED1, AFE440X_LED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED1),
>> +	AFE4404_REG_INFO(ALED1, AFE440X_ALED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED1),
>> +	AFE4404_REG_INFO(LED2, AFE440X_LED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED2),
>> +	AFE4404_REG_INFO(ALED2, AFE440X_ALED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED2),
>> +	AFE4404_REG_INFO(LED3, AFE440X_ALED2VAL, 0, NULL),
>> +	AFE4404_REG_INFO(LED1_ALED1, AFE440X_LED1_ALED1VAL, 0, NULL),
>> +	AFE4404_REG_INFO(LED2_ALED2, AFE440X_LED2_ALED2VAL, 0, NULL),
>> +	AFE4404_REG_INFO(AVG_LED1_ALED1, AFE4404_AVG_LED1_ALED1VAL, 0, NULL),
>> +	AFE4404_REG_INFO(AVG_LED2_ALED2, AFE4404_AVG_LED2_ALED2VAL, 0, NULL),
>> +	AFE4404_REG_INFO(ILED1, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED1),
>> +	AFE4404_REG_INFO(ILED2, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED2),
>> +	AFE4404_REG_INFO(ILED3, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED3),
>> +};
>> +
>> +static const struct iio_chan_spec afe4404_channels[] = {
>> +	/* ADC values */
>> +	AFE440X_INTENSITY_CHAN(LED1, "led1", BIT(IIO_CHAN_INFO_OFFSET)),
>> +	AFE440X_INTENSITY_CHAN(ALED1, "led1_ambient", BIT(IIO_CHAN_INFO_OFFSET)),
> (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)
>> +	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?

I'm thinking these last two are kind of a mess right now, also I don't
expose the setting to enable these yet anyway, so, I think I will just
drop them and add them back in a latter patch that adds all their
required functionality together.

>> +	/* 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.
>

ACK

>> +};
>> +
>> +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, &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, &reg_val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reg_val &= ~afe440x_reg->mask;
>> +	reg_val |= ((val << afe440x_reg->shift) & afe440x_reg->mask);
>> +
>> +	ret = regmap_write(afe440x->regmap, afe440x_reg->reg, reg_val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>> +AFE440X_ATTR(tia_separate_en, AFE4404_TIA_GAIN_SEP, AFE440X_TIAGAIN_ENSEPGAIN);
>> +
>> +AFE440X_ATTR(tia_resistance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES);
>> +AFE440X_ATTR(tia_capacitance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_CAP);
>> +
>> +AFE440X_ATTR(tia_resistance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_RES);
>> +AFE440X_ATTR(tia_capacitance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_CAP);
>> +
>> +static struct attribute *afe4404_attributes[] = {
>> +	&afe440x_reg_tia_separate_en.dev_attr.attr,
>> +	&afe440x_reg_tia_resistance1.dev_attr.attr,
>> +	&afe440x_reg_tia_capacitance1.dev_attr.attr,
>> +	&afe440x_reg_tia_resistance2.dev_attr.attr,
>> +	&afe440x_reg_tia_capacitance2.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group afe4404_attribute_group = {
>> +	.attrs = afe4404_attributes
>> +};
>> +
>> +static int afe440x_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
>> +	int ret;
>> +
>> +	switch (chan->type) {
>> +	case IIO_INTENSITY:
>> +		switch (mask) {
>> +		case IIO_CHAN_INFO_RAW:
>> +			ret = regmap_read(afe440x->regmap,
>> +					  reg_info[chan->address].reg, val);
>> +			if (ret)
>> +				return ret;
>> +			return IIO_VAL_INT;
>> +		case IIO_CHAN_INFO_OFFSET:
>> +			ret = regmap_read(afe440x->regmap,
>> +					  reg_info[chan->address].offreg, val);
>> +			if (ret)
>> +				return ret;
>> +			*val &= reg_info[chan->address].mask;
>> +			*val >>= reg_info[chan->address].shift;
>> +			return IIO_VAL_INT;
>> +		}
>> +		break;
>> +	case IIO_CURRENT:
>> +		switch (mask) {
>> +		case IIO_CHAN_INFO_RAW:
>> +			ret = regmap_read(afe440x->regmap,
>> +					  reg_info[chan->address].reg, val);
>> +			if (ret)
>> +				return ret;
>> +			*val &= reg_info[chan->address].mask;
>> +			*val >>= reg_info[chan->address].shift;
>> +			return IIO_VAL_INT;
>> +		case IIO_CHAN_INFO_SCALE:
>> +			*val = 0;
>> +			*val2 = 800000;
>> +			return IIO_VAL_INT_PLUS_MICRO;
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int afe440x_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
>> +	unsigned int reg_val;
>> +	int ret;
>> +
>> +	switch (chan->type) {
>> +	case IIO_INTENSITY:
>> +		switch (mask) {
>> +		case IIO_CHAN_INFO_OFFSET:
>> +			ret = regmap_read(afe440x->regmap,
>> +					  reg_info[chan->address].offreg,
>> +					  &reg_val);
>> +			if (ret)
>> +				return ret;
>> +			reg_val &= ~reg_info[chan->address].mask;
>> +			reg_val |= ((val << reg_info[chan->address].shift) &
>> +					reg_info[chan->address].mask);
>> +			return regmap_write(afe440x->regmap,
>> +					    reg_info[chan->address].offreg,
>> +					    reg_val);
>> +		}
>> +		break;
>> +	case IIO_CURRENT:
>> +		switch (mask) {
>> +		case IIO_CHAN_INFO_RAW:
>> +			ret = regmap_read(afe440x->regmap,
>> +					  reg_info[chan->address].reg,
>> +					  &reg_val);
>> +			if (ret)
>> +				return ret;
>> +			reg_val &= ~reg_info[chan->address].mask;
>> +			reg_val |= ((val << reg_info[chan->address].shift) &
>> +					reg_info[chan->address].mask);
>> +			return regmap_write(afe440x->regmap,
>> +					    reg_info[chan->address].reg,
>> +					    reg_val);
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info afe4404_iio_info = {
>> +	.attrs	= &afe4404_attribute_group,
>> +	.read_raw = afe440x_read_raw,
>> +	.write_raw = afe440x_write_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +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, reg, i = 0;
>> +	s32 buffer[12];
>> +
>> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
>> +			 indio_dev->masklength) {
>> +		reg = reg_info[bit].reg;
> why the local reg variable? Stick it dirrectly in the functional call.

No reason, just looked better to me. Will fix.

>> +		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	},	\
>> +	{ AFE440X_ADCRSTENDCT2,	6542	},	\
>> +	{ AFE440X_LED1CONVST,	6543	},	\
>> +	{ AFE440X_LED1CONVEND,	7003	},	\
>> +	{ AFE440X_ALED1STC,	1280	},	\
>> +	{ AFE440X_ALED1ENDC,	1598	},	\
>> +	{ AFE440X_ADCRSTSTCT3,	7005	},	\
>> +	{ AFE440X_ADCRSTENDCT3,	7011	},	\
>> +	{ AFE440X_ALED1CONVST,	7012	},	\
>> +	{ AFE440X_ALED1CONVEND,	7471	},	\
>> +	{ AFE440X_PDNCYCLESTC,	7671	},	\
>> +	{ AFE440X_PDNCYCLEENDC,	39199	}
>> +
>> +static const struct reg_sequence afe4404_reg_sequences[] = {
>> +	AFE4404_TIMING_PAIRS,
>> +	{ AFE440X_CONTROL1, AFE440X_CONTROL1_TIMEREN },
>> +	{ AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES_50_K },
>> +	{ AFE440X_LEDCNTRL, (0xf << AFE4404_LEDCNTRL_ILED1_SHIFT) |
>> +			    (0x3 << AFE4404_LEDCNTRL_ILED2_SHIFT) |
>> +			    (0x3 << AFE4404_LEDCNTRL_ILED3_SHIFT) },
>> +	{ AFE440X_CONTROL2, AFE440X_CONTROL3_OSC_ENABLE	},
>> +};
>> +
>> +static const struct regmap_range afe4404_yes_ranges[] = {
>> +	regmap_reg_range(AFE440X_LED2VAL, AFE440X_LED1_ALED1VAL),
>> +	regmap_reg_range(AFE4404_AVG_LED2_ALED2VAL, AFE4404_AVG_LED1_ALED1VAL),
>> +};
>> +
>> +static const struct regmap_access_table afe4404_volatile_table = {
>> +	.yes_ranges = afe4404_yes_ranges,
>> +	.n_yes_ranges = ARRAY_SIZE(afe4404_yes_ranges),
>> +};
>> +
>> +static const struct regmap_config afe4404_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 24,
>> +
>> +	.max_register = AFE4404_AVG_LED1_ALED1VAL,
>> +	.cache_type = REGCACHE_RBTREE,
>> +	.volatile_table = &afe4404_volatile_table,
>> +};
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id afe4404_of_match[] = {
>> +	{ .compatible = "ti,afe4404", },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, afe4404_of_match);
>> +#endif
>> +
>> +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, "Failed to disable regulator\n");
>> +		return ret;
>> +	}
> 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!)

Makes sense, will do.

>> +
>> +	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...
>> +
>> +	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?

I'm not sure but I belive the regulator will be enabled on 'get'
and when all devices using the regulator have 'put' it will be
turned off. I manualy disable/enable in the PM code so I don't
have to put/get them agian.

>> +
>> +	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;
>> +
>> +		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->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);
> 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!

Why do today what you can put off until tomorrow :) I'll look into that
in a later patch.

>> +	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.

I'm thinking now that this driver may be getting close to correct, I will
add the other part as part of this series if that is OK. Should clear up
some of the issues below to have them at the same time.

>> +
>> +	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!
>
>> +
>> +	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)

ACK

>> +	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@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..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@ti.com>
>> + *
>> + * Copyright: (C) 2015 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.
>> + */
>> +
>> +#ifndef _AFE440X_H
>> +#define _AFE440X_H
> 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.

As above would it be better for me to push both now?

>> +
>> +/* 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
>> +#define AFE440X_LED1VAL			0x2c
>> +#define AFE440X_ALED1VAL		0x2d
>> +#define AFE440X_LED2_ALED2VAL		0x2e
>> +#define AFE440X_LED1_ALED1VAL		0x2f
>> +#define AFE440X_CONTROL3		0x31
>> +#define AFE440X_PDNCYCLESTC		0x32
>> +#define AFE440X_PDNCYCLEENDC		0x33
>> +
>> +/* CONTROL0 register fields */
>> +#define AFE440X_CONTROL0_REG_READ	BIT(0)
>> +#define AFE440X_CONTROL0_TM_COUNT_RST	BIT(1)
>> +#define AFE440X_CONTROL0_SW_RESET	BIT(3)
>> +
>> +/* CONTROL1 register fields */
>> +#define AFE440X_CONTROL1_TIMEREN	BIT(8)
>> +
>> +/* TIAGAIN register fields */
>> +#define AFE440X_TIAGAIN_ENSEPGAIN_MASK	BIT(15)
>> +#define AFE440X_TIAGAIN_ENSEPGAIN_SHIFT	15
>> +
>> +/* CONTROL2 register fields */
>> +#define AFE440X_CONTROL2_PDN_AFE	BIT(0)
>> +#define AFE440X_CONTROL2_PDN_RX		BIT(1)
>> +#define AFE440X_CONTROL2_DYNAMIC4	BIT(3)
>> +#define AFE440X_CONTROL2_DYNAMIC3	BIT(4)
>> +#define AFE440X_CONTROL2_DYNAMIC2	BIT(14)
>> +#define AFE440X_CONTROL2_DYNAMIC1	BIT(20)
>> +
>> +/* CONTROL3 register fields */
>> +#define AFE440X_CONTROL3_CLKDIV		GENMASK(2, 0)
>> +
>> +/* CONTROL0 values */
>> +#define AFE440X_CONTROL0_WRITE		0x0
>> +#define AFE440X_CONTROL0_READ		0x1
>> +
>> +#define AFE440X_INTENSITY_CHAN(_index, _name, _mask)		\
>> +	{							\
>> +		.type = IIO_INTENSITY,				\
>> +		.channel = _index,				\
>> +		.address = _index,				\
>> +		.scan_index = _index,				\
>> +		.scan_type = {					\
>> +				.sign = 's',			\
>> +				.realbits = 24,			\
>> +				.storagebits = 32,		\
>> +				.endianness = IIO_CPU,		\
>> +		},						\
>> +		.extend_name = _name,				\
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>> +			_mask,					\
>> +	}
>> +
>> +#define AFE440X_CURRENT_CHAN(_index, _name)			\
>> +	{							\
>> +		.type = IIO_CURRENT,				\
>> +		.channel = _index,				\
>> +		.address = _index,				\
>> +		.scan_index = _index,				\
>> +		.extend_name = _name,				\
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
>> +			BIT(IIO_CHAN_INFO_SCALE),		\
>> +		.output = true,					\
>> +	}
> I'd particularly like to see the above two definitions next to the
> arrays they are used to instantiate - makes for easier review!
>
>> +
>> +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
>> +
>> +/**
>> + * 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).
>

ACK

>> + * @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.
>> +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 */
>>
>

Re: [PATCH v2 2/2] iio: health: Add driver for the TI AFE4404 heart monitor

[Jonathan Cameron]

On 07/12/15 17:26, Andrew F. Davis wrote:
> On 12/05/2015 12:21 PM, Jonathan Cameron wrote:
>> 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 <afd@ti.com>
>> 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
Reponses inline.
>>> ---
>>>   .../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.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..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@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.
>>> +        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 ;)
> 
> I'm not sure if there is a clean way to do this, these are not channels
> so parsing input will need checks based on what kind of register and value
> we send in, these are raw input to the registers for other registers, I
> changed the name to attempt to make it clear to users these are not
> channels but raw registers (out_resistance1_raw -> tia_resistance1).
> 
> I could still make this more clear in the ABI doc:
> 
>     Valid capacitance settings are 0 -> 7 which correspond to
>     5pF, 2.5pF, 10pF, 7.5pF, 20pF, 17.5pF, 25pF, and 22.5pF
>     respectively.
Just do a simple mapping in the driver from magic number to real value
and the other way around + provide the available attribute.  Rule 2 of sysfs
interfaces: no magic values where a real meaningful one can be provided for
the cost of a few more lines of code.

You can use the existing floating point to value pair functions from IIO
to get you to a couple of values that can be trivially matched against
a const table.

It's not free, but probably only 20 lines including the static const table
of values.
> 
> or something similar.
> 
>>> +
>>> +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.
>> 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.
>>
> 
> Works for me, I'll add them back.
> 
>>> 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
>>> +    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..d36c1d9
>>> --- /dev/null
>>> +++ b/drivers/iio/health/afe4404.c
>>> @@ -0,0 +1,619 @@
>>> +/*
>>> + * AFE4404 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.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/err.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/sysfs.h>
>>> +#include <linux/regulator/consumer.h>
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +
>>> +#include "afe440x.h"
>>> +
>>> +#define AFE4404_DRIVER_NAME        "afe4404"
>>> +
>>> +/* AFE4404 registers */
>>> +#define AFE4404_TIA_GAIN_SEP        0x20
>>> +#define AFE4404_TIA_GAIN        0x21
>>> +#define AFE4404_PROG_TG_STC        0x34
>>> +#define AFE4404_PROG_TG_ENDC        0x35
>>> +#define AFE4404_LED3LEDSTC        0x36
>>> +#define AFE4404_LED3LEDENDC        0x37
>>> +#define AFE4404_CLKDIV_PRF        0x39
>>> +#define AFE4404_OFFDAC            0x3a
>>> +#define AFE4404_DEC            0x3d
>>> +#define AFE4404_AVG_LED2_ALED2VAL    0x3f
>>> +#define AFE4404_AVG_LED1_ALED1VAL    0x40
>>> +
>>> +/* AFE4404 GAIN register fields */
>>> +#define AFE4404_TIA_GAIN_RES_MASK    GENMASK(2, 0)
>>> +#define AFE4404_TIA_GAIN_RES_SHIFT    0
>>> +#define AFE4404_TIA_GAIN_CAP_MASK    GENMASK(5, 3)
>>> +#define AFE4404_TIA_GAIN_CAP_SHIFT    3
>>> +
>>> +/* AFE4404 LEDCNTRL register fields */
>>> +#define AFE4404_LEDCNTRL_ILED1_MASK    GENMASK(5, 0)
>>> +#define AFE4404_LEDCNTRL_ILED1_SHIFT    0
>>> +#define AFE4404_LEDCNTRL_ILED2_MASK    GENMASK(11, 6)
>>> +#define AFE4404_LEDCNTRL_ILED2_SHIFT    6
>>> +#define AFE4404_LEDCNTRL_ILED3_MASK    GENMASK(17, 12)
>>> +#define AFE4404_LEDCNTRL_ILED3_SHIFT    12
>>> +
>>> +/* AFE4404 CONTROL2 register fields */
>>> +#define AFE440X_CONTROL2_ILED_2X_MASK    BIT(17)
>>> +#define AFE440X_CONTROL2_ILED_2X_SHIFT    17
>>> +
>>> +/* AFE4404 CONTROL3 register fields */
>>> +#define AFE440X_CONTROL3_OSC_ENABLE    BIT(9)
>>> +
>>> +/* AFE4404 OFFDAC register current fields */
>>> +#define AFE4404_OFFDAC_CURR_LED1_MASK    GENMASK(9, 5)
>>> +#define AFE4404_OFFDAC_CURR_LED1_SHIFT    5
>>> +#define AFE4404_OFFDAC_CURR_LED2_MASK    GENMASK(19, 15)
>>> +#define AFE4404_OFFDAC_CURR_LED2_SHIFT    15
>>> +#define AFE4404_OFFDAC_CURR_LED3_MASK    GENMASK(4, 0)
>>> +#define AFE4404_OFFDAC_CURR_LED3_SHIFT    0
>>> +#define AFE4404_OFFDAC_CURR_ALED1_MASK    GENMASK(14, 10)
>>> +#define AFE4404_OFFDAC_CURR_ALED1_SHIFT    10
>>> +#define AFE4404_OFFDAC_CURR_ALED2_MASK    GENMASK(4, 0)
>>> +#define AFE4404_OFFDAC_CURR_ALED2_SHIFT    0
>>> +
>>> +/* AFE4404 NULL fields */
>>> +#define NULL_MASK    0
>>> +#define NULL_SHIFT    0
>>> +
>>> +/* AFE4404 TIA_GAIN_CAP values */
>>> +#define AFE4404_TIA_GAIN_CAP_5_P    0x0
>>> +#define AFE4404_TIA_GAIN_CAP_2_5_P    0x1
>>> +#define AFE4404_TIA_GAIN_CAP_10_P    0x2
>>> +#define AFE4404_TIA_GAIN_CAP_7_5_P    0x3
>>> +#define AFE4404_TIA_GAIN_CAP_20_P    0x4
>>> +#define AFE4404_TIA_GAIN_CAP_17_5_P    0x5
>>> +#define AFE4404_TIA_GAIN_CAP_25_P    0x6
>>> +#define AFE4404_TIA_GAIN_CAP_22_5_P    0x7
>>> +
>>> +/* AFE4404 TIA_GAIN_RES values */
>>> +#define AFE4404_TIA_GAIN_RES_500_K    0x0
>>> +#define AFE4404_TIA_GAIN_RES_250_K    0x1
>>> +#define AFE4404_TIA_GAIN_RES_100_K    0x2
>>> +#define AFE4404_TIA_GAIN_RES_50_K    0x3
>>> +#define AFE4404_TIA_GAIN_RES_25_K    0x4
>>> +#define AFE4404_TIA_GAIN_RES_10_K    0x5
>>> +#define AFE4404_TIA_GAIN_RES_1_M    0x6
>>> +#define AFE4404_TIA_GAIN_RES_2_M    0x7
>>> +
>>> +enum afe4404_chan_id {
>>> +    LED1,
>>> +    ALED1,
>>> +    LED2,
>>> +    ALED2,
>>> +    LED3,
>>> +    LED1_ALED1,
>>> +    LED2_ALED2,
>>> +    AVG_LED1_ALED1,
>>> +    AVG_LED2_ALED2,
>>> +    ILED1,
>>> +    ILED2,
>>> +    ILED3,
>>> +};
>>> +
>>> +#define AFE4404_REG_INFO(_id, _reg, _offreg, _sm)        \
>>> +    [_id] = {                        \
>>> +        .reg = _reg,                    \
>>> +        .offreg = _offreg,                \
>>> +        .shift = _sm ## _SHIFT,                \
>>> +        .mask = _sm ## _MASK,                \
>>> +    }
>>> +
>>> +struct {
>>> +    unsigned int reg;
>>> +    unsigned int offreg;
>>> +    unsigned int shift;
>>> +    unsigned int mask;
>>> +} reg_info[] = {
>>> +    AFE4404_REG_INFO(LED1, AFE440X_LED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED1),
>>> +    AFE4404_REG_INFO(ALED1, AFE440X_ALED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED1),
>>> +    AFE4404_REG_INFO(LED2, AFE440X_LED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED2),
>>> +    AFE4404_REG_INFO(ALED2, AFE440X_ALED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED2),
>>> +    AFE4404_REG_INFO(LED3, AFE440X_ALED2VAL, 0, NULL),
>>> +    AFE4404_REG_INFO(LED1_ALED1, AFE440X_LED1_ALED1VAL, 0, NULL),
>>> +    AFE4404_REG_INFO(LED2_ALED2, AFE440X_LED2_ALED2VAL, 0, NULL),
>>> +    AFE4404_REG_INFO(AVG_LED1_ALED1, AFE4404_AVG_LED1_ALED1VAL, 0, NULL),
>>> +    AFE4404_REG_INFO(AVG_LED2_ALED2, AFE4404_AVG_LED2_ALED2VAL, 0, NULL),
>>> +    AFE4404_REG_INFO(ILED1, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED1),
>>> +    AFE4404_REG_INFO(ILED2, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED2),
>>> +    AFE4404_REG_INFO(ILED3, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED3),
>>> +};
>>> +
>>> +static const struct iio_chan_spec afe4404_channels[] = {
>>> +    /* ADC values */
>>> +    AFE440X_INTENSITY_CHAN(LED1, "led1", BIT(IIO_CHAN_INFO_OFFSET)),
>>> +    AFE440X_INTENSITY_CHAN(ALED1, "led1_ambient", BIT(IIO_CHAN_INFO_OFFSET)),
>> (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)
>>> +    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?
> 
> I'm thinking these last two are kind of a mess right now, also I don't
> expose the setting to enable these yet anyway, so, I think I will just
> drop them and add them back in a latter patch that adds all their
> required functionality together.
Fair enough.
> 
>>> +    /* 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.
>>
> 
> ACK
> 
>>> +};
>>> +
>>> +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, &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, &reg_val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    reg_val &= ~afe440x_reg->mask;
>>> +    reg_val |= ((val << afe440x_reg->shift) & afe440x_reg->mask);
>>> +
>>> +    ret = regmap_write(afe440x->regmap, afe440x_reg->reg, reg_val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return count;
>>> +}
>>> +
>>> +AFE440X_ATTR(tia_separate_en, AFE4404_TIA_GAIN_SEP, AFE440X_TIAGAIN_ENSEPGAIN);
>>> +
>>> +AFE440X_ATTR(tia_resistance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES);
>>> +AFE440X_ATTR(tia_capacitance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_CAP);
>>> +
>>> +AFE440X_ATTR(tia_resistance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_RES);
>>> +AFE440X_ATTR(tia_capacitance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_CAP);
>>> +
>>> +static struct attribute *afe4404_attributes[] = {
>>> +    &afe440x_reg_tia_separate_en.dev_attr.attr,
>>> +    &afe440x_reg_tia_resistance1.dev_attr.attr,
>>> +    &afe440x_reg_tia_capacitance1.dev_attr.attr,
>>> +    &afe440x_reg_tia_resistance2.dev_attr.attr,
>>> +    &afe440x_reg_tia_capacitance2.dev_attr.attr,
>>> +    NULL
>>> +};
>>> +
>>> +static const struct attribute_group afe4404_attribute_group = {
>>> +    .attrs = afe4404_attributes
>>> +};
>>> +
>>> +static int afe440x_read_raw(struct iio_dev *indio_dev,
>>> +                struct iio_chan_spec const *chan,
>>> +                int *val, int *val2, long mask)
>>> +{
>>> +    struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
>>> +    int ret;
>>> +
>>> +    switch (chan->type) {
>>> +    case IIO_INTENSITY:
>>> +        switch (mask) {
>>> +        case IIO_CHAN_INFO_RAW:
>>> +            ret = regmap_read(afe440x->regmap,
>>> +                      reg_info[chan->address].reg, val);
>>> +            if (ret)
>>> +                return ret;
>>> +            return IIO_VAL_INT;
>>> +        case IIO_CHAN_INFO_OFFSET:
>>> +            ret = regmap_read(afe440x->regmap,
>>> +                      reg_info[chan->address].offreg, val);
>>> +            if (ret)
>>> +                return ret;
>>> +            *val &= reg_info[chan->address].mask;
>>> +            *val >>= reg_info[chan->address].shift;
>>> +            return IIO_VAL_INT;
>>> +        }
>>> +        break;
>>> +    case IIO_CURRENT:
>>> +        switch (mask) {
>>> +        case IIO_CHAN_INFO_RAW:
>>> +            ret = regmap_read(afe440x->regmap,
>>> +                      reg_info[chan->address].reg, val);
>>> +            if (ret)
>>> +                return ret;
>>> +            *val &= reg_info[chan->address].mask;
>>> +            *val >>= reg_info[chan->address].shift;
>>> +            return IIO_VAL_INT;
>>> +        case IIO_CHAN_INFO_SCALE:
>>> +            *val = 0;
>>> +            *val2 = 800000;
>>> +            return IIO_VAL_INT_PLUS_MICRO;
>>> +        }
>>> +        break;
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static int afe440x_write_raw(struct iio_dev *indio_dev,
>>> +                 struct iio_chan_spec const *chan,
>>> +                 int val, int val2, long mask)
>>> +{
>>> +    struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
>>> +    unsigned int reg_val;
>>> +    int ret;
>>> +
>>> +    switch (chan->type) {
>>> +    case IIO_INTENSITY:
>>> +        switch (mask) {
>>> +        case IIO_CHAN_INFO_OFFSET:
>>> +            ret = regmap_read(afe440x->regmap,
>>> +                      reg_info[chan->address].offreg,
>>> +                      &reg_val);
>>> +            if (ret)
>>> +                return ret;
>>> +            reg_val &= ~reg_info[chan->address].mask;
>>> +            reg_val |= ((val << reg_info[chan->address].shift) &
>>> +                    reg_info[chan->address].mask);
>>> +            return regmap_write(afe440x->regmap,
>>> +                        reg_info[chan->address].offreg,
>>> +                        reg_val);
>>> +        }
>>> +        break;
>>> +    case IIO_CURRENT:
>>> +        switch (mask) {
>>> +        case IIO_CHAN_INFO_RAW:
>>> +            ret = regmap_read(afe440x->regmap,
>>> +                      reg_info[chan->address].reg,
>>> +                      &reg_val);
>>> +            if (ret)
>>> +                return ret;
>>> +            reg_val &= ~reg_info[chan->address].mask;
>>> +            reg_val |= ((val << reg_info[chan->address].shift) &
>>> +                    reg_info[chan->address].mask);
>>> +            return regmap_write(afe440x->regmap,
>>> +                        reg_info[chan->address].reg,
>>> +                        reg_val);
>>> +        }
>>> +        break;
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static const struct iio_info afe4404_iio_info = {
>>> +    .attrs    = &afe4404_attribute_group,
>>> +    .read_raw = afe440x_read_raw,
>>> +    .write_raw = afe440x_write_raw,
>>> +    .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +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, reg, i = 0;
>>> +    s32 buffer[12];
>>> +
>>> +    for_each_set_bit(bit, indio_dev->active_scan_mask,
>>> +             indio_dev->masklength) {
>>> +        reg = reg_info[bit].reg;
>> why the local reg variable? Stick it dirrectly in the functional call.
> 
> No reason, just looked better to me. Will fix.
> 
>>> +        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    },    \
>>> +    { AFE440X_ADCRSTENDCT2,    6542    },    \
>>> +    { AFE440X_LED1CONVST,    6543    },    \
>>> +    { AFE440X_LED1CONVEND,    7003    },    \
>>> +    { AFE440X_ALED1STC,    1280    },    \
>>> +    { AFE440X_ALED1ENDC,    1598    },    \
>>> +    { AFE440X_ADCRSTSTCT3,    7005    },    \
>>> +    { AFE440X_ADCRSTENDCT3,    7011    },    \
>>> +    { AFE440X_ALED1CONVST,    7012    },    \
>>> +    { AFE440X_ALED1CONVEND,    7471    },    \
>>> +    { AFE440X_PDNCYCLESTC,    7671    },    \
>>> +    { AFE440X_PDNCYCLEENDC,    39199    }
>>> +
>>> +static const struct reg_sequence afe4404_reg_sequences[] = {
>>> +    AFE4404_TIMING_PAIRS,
>>> +    { AFE440X_CONTROL1, AFE440X_CONTROL1_TIMEREN },
>>> +    { AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES_50_K },
>>> +    { AFE440X_LEDCNTRL, (0xf << AFE4404_LEDCNTRL_ILED1_SHIFT) |
>>> +                (0x3 << AFE4404_LEDCNTRL_ILED2_SHIFT) |
>>> +                (0x3 << AFE4404_LEDCNTRL_ILED3_SHIFT) },
>>> +    { AFE440X_CONTROL2, AFE440X_CONTROL3_OSC_ENABLE    },
>>> +};
>>> +
>>> +static const struct regmap_range afe4404_yes_ranges[] = {
>>> +    regmap_reg_range(AFE440X_LED2VAL, AFE440X_LED1_ALED1VAL),
>>> +    regmap_reg_range(AFE4404_AVG_LED2_ALED2VAL, AFE4404_AVG_LED1_ALED1VAL),
>>> +};
>>> +
>>> +static const struct regmap_access_table afe4404_volatile_table = {
>>> +    .yes_ranges = afe4404_yes_ranges,
>>> +    .n_yes_ranges = ARRAY_SIZE(afe4404_yes_ranges),
>>> +};
>>> +
>>> +static const struct regmap_config afe4404_regmap_config = {
>>> +    .reg_bits = 8,
>>> +    .val_bits = 24,
>>> +
>>> +    .max_register = AFE4404_AVG_LED1_ALED1VAL,
>>> +    .cache_type = REGCACHE_RBTREE,
>>> +    .volatile_table = &afe4404_volatile_table,
>>> +};
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id afe4404_of_match[] = {
>>> +    { .compatible = "ti,afe4404", },
>>> +    { /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, afe4404_of_match);
>>> +#endif
>>> +
>>> +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, "Failed to disable regulator\n");
>>> +        return ret;
>>> +    }
>> 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!)
> 
> Makes sense, will do.
> 
>>> +
>>> +    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...
>>> +
>>> +    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?
> 
> I'm not sure but I belive the regulator will be enabled on 'get'
It won't - has to be explicity enabled.  Now as likely as not on most
boards it will be on anyway, but you never know!
> and when all devices using the regulator have 'put' it will be
> turned off.
Not true either.  Will leave it on unless explicitly disabled.
>I manualy disable/enable in the PM code so I don't
> have to put/get them agian.
> 
>>> +
>>> +    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;
>>> +
>>> +        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->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);
>> 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!
> 
> Why do today what you can put off until tomorrow :) I'll look into that
> in a later patch.
> 
>>> +    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.
> 
> I'm thinking now that this driver may be getting close to correct, I will
> add the other part as part of this series if that is OK. Should clear up
> some of the issues below to have them at the same time.
Cool
> 
>>> +
>>> +    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!
>>
>>> +
>>> +    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)
> 
> ACK
> 
>>> +    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@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..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@ti.com>
>>> + *
>>> + * Copyright: (C) 2015 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.
>>> + */
>>> +
>>> +#ifndef _AFE440X_H
>>> +#define _AFE440X_H
>> 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.
> 
> As above would it be better for me to push both now?
Probably, easier to look at whether any refactoring make sense with
both infront of us.
> 
>>> +
>>> +/* 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
>>> +#define AFE440X_LED1VAL            0x2c
>>> +#define AFE440X_ALED1VAL        0x2d
>>> +#define AFE440X_LED2_ALED2VAL        0x2e
>>> +#define AFE440X_LED1_ALED1VAL        0x2f
>>> +#define AFE440X_CONTROL3        0x31
>>> +#define AFE440X_PDNCYCLESTC        0x32
>>> +#define AFE440X_PDNCYCLEENDC        0x33
>>> +
>>> +/* CONTROL0 register fields */
>>> +#define AFE440X_CONTROL0_REG_READ    BIT(0)
>>> +#define AFE440X_CONTROL0_TM_COUNT_RST    BIT(1)
>>> +#define AFE440X_CONTROL0_SW_RESET    BIT(3)
>>> +
>>> +/* CONTROL1 register fields */
>>> +#define AFE440X_CONTROL1_TIMEREN    BIT(8)
>>> +
>>> +/* TIAGAIN register fields */
>>> +#define AFE440X_TIAGAIN_ENSEPGAIN_MASK    BIT(15)
>>> +#define AFE440X_TIAGAIN_ENSEPGAIN_SHIFT    15
>>> +
>>> +/* CONTROL2 register fields */
>>> +#define AFE440X_CONTROL2_PDN_AFE    BIT(0)
>>> +#define AFE440X_CONTROL2_PDN_RX        BIT(1)
>>> +#define AFE440X_CONTROL2_DYNAMIC4    BIT(3)
>>> +#define AFE440X_CONTROL2_DYNAMIC3    BIT(4)
>>> +#define AFE440X_CONTROL2_DYNAMIC2    BIT(14)
>>> +#define AFE440X_CONTROL2_DYNAMIC1    BIT(20)
>>> +
>>> +/* CONTROL3 register fields */
>>> +#define AFE440X_CONTROL3_CLKDIV        GENMASK(2, 0)
>>> +
>>> +/* CONTROL0 values */
>>> +#define AFE440X_CONTROL0_WRITE        0x0
>>> +#define AFE440X_CONTROL0_READ        0x1
>>> +
>>> +#define AFE440X_INTENSITY_CHAN(_index, _name, _mask)        \
>>> +    {                            \
>>> +        .type = IIO_INTENSITY,                \
>>> +        .channel = _index,                \
>>> +        .address = _index,                \
>>> +        .scan_index = _index,                \
>>> +        .scan_type = {                    \
>>> +                .sign = 's',            \
>>> +                .realbits = 24,            \
>>> +                .storagebits = 32,        \
>>> +                .endianness = IIO_CPU,        \
>>> +        },                        \
>>> +        .extend_name = _name,                \
>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |    \
>>> +            _mask,                    \
>>> +    }
>>> +
>>> +#define AFE440X_CURRENT_CHAN(_index, _name)            \
>>> +    {                            \
>>> +        .type = IIO_CURRENT,                \
>>> +        .channel = _index,                \
>>> +        .address = _index,                \
>>> +        .scan_index = _index,                \
>>> +        .extend_name = _name,                \
>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |    \
>>> +            BIT(IIO_CHAN_INFO_SCALE),        \
>>> +        .output = true,                    \
>>> +    }
>> I'd particularly like to see the above two definitions next to the
>> arrays they are used to instantiate - makes for easier review!
>>
>>> +
>>> +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
>>> +
>>> +/**
>>> + * 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).
>>
> 
> ACK
> 
>>> + * @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.
>>> +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 */
>>>
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 2/2] iio: health: Add driver for the TI AFE4404 heart monitor

[Andrew F. Davis]

On 12/12/2015 09:41 AM, Jonathan Cameron wrote:
> On 07/12/15 17:26, Andrew F. Davis wrote:
>> On 12/05/2015 12:21 PM, Jonathan Cameron wrote:
>>> 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 <afd@ti.com>
>>> 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
> Reponses inline.
>>>> ---
>>>>    .../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.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..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@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.
>>>> +        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 ;)
>>
>> I'm not sure if there is a clean way to do this, these are not channels
>> so parsing input will need checks based on what kind of register and value
>> we send in, these are raw input to the registers for other registers, I
>> changed the name to attempt to make it clear to users these are not
>> channels but raw registers (out_resistance1_raw -> tia_resistance1).
>>
>> I could still make this more clear in the ABI doc:
>>
>>      Valid capacitance settings are 0 -> 7 which correspond to
>>      5pF, 2.5pF, 10pF, 7.5pF, 20pF, 17.5pF, 25pF, and 22.5pF
>>      respectively.
> Just do a simple mapping in the driver from magic number to real value
> and the other way around + provide the available attribute.  Rule 2 of sysfs
> interfaces: no magic values where a real meaningful one can be provided for
> the cost of a few more lines of code.
>
> You can use the existing floating point to value pair functions from IIO
> to get you to a couple of values that can be trivially matched against
> a const table.
>
> It's not free, but probably only 20 lines including the static const table
> of values.

Took a bit more work than expected but done for the next version.

>>
>> or something similar.
>>
>>>> +
>>>> +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.
>>> 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.
>>>
>>
>> Works for me, I'll add them back.
>>
>>>> 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
>>>> +    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..d36c1d9
>>>> --- /dev/null
>>>> +++ b/drivers/iio/health/afe4404.c
>>>> @@ -0,0 +1,619 @@
>>>> +/*
>>>> + * AFE4404 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.
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_gpio.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/sysfs.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/sysfs.h>
>>>> +#include <linux/iio/buffer.h>
>>>> +#include <linux/iio/trigger.h>
>>>> +#include <linux/iio/triggered_buffer.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +
>>>> +#include "afe440x.h"
>>>> +
>>>> +#define AFE4404_DRIVER_NAME        "afe4404"
>>>> +
>>>> +/* AFE4404 registers */
>>>> +#define AFE4404_TIA_GAIN_SEP        0x20
>>>> +#define AFE4404_TIA_GAIN        0x21
>>>> +#define AFE4404_PROG_TG_STC        0x34
>>>> +#define AFE4404_PROG_TG_ENDC        0x35
>>>> +#define AFE4404_LED3LEDSTC        0x36
>>>> +#define AFE4404_LED3LEDENDC        0x37
>>>> +#define AFE4404_CLKDIV_PRF        0x39
>>>> +#define AFE4404_OFFDAC            0x3a
>>>> +#define AFE4404_DEC            0x3d
>>>> +#define AFE4404_AVG_LED2_ALED2VAL    0x3f
>>>> +#define AFE4404_AVG_LED1_ALED1VAL    0x40
>>>> +
>>>> +/* AFE4404 GAIN register fields */
>>>> +#define AFE4404_TIA_GAIN_RES_MASK    GENMASK(2, 0)
>>>> +#define AFE4404_TIA_GAIN_RES_SHIFT    0
>>>> +#define AFE4404_TIA_GAIN_CAP_MASK    GENMASK(5, 3)
>>>> +#define AFE4404_TIA_GAIN_CAP_SHIFT    3
>>>> +
>>>> +/* AFE4404 LEDCNTRL register fields */
>>>> +#define AFE4404_LEDCNTRL_ILED1_MASK    GENMASK(5, 0)
>>>> +#define AFE4404_LEDCNTRL_ILED1_SHIFT    0
>>>> +#define AFE4404_LEDCNTRL_ILED2_MASK    GENMASK(11, 6)
>>>> +#define AFE4404_LEDCNTRL_ILED2_SHIFT    6
>>>> +#define AFE4404_LEDCNTRL_ILED3_MASK    GENMASK(17, 12)
>>>> +#define AFE4404_LEDCNTRL_ILED3_SHIFT    12
>>>> +
>>>> +/* AFE4404 CONTROL2 register fields */
>>>> +#define AFE440X_CONTROL2_ILED_2X_MASK    BIT(17)
>>>> +#define AFE440X_CONTROL2_ILED_2X_SHIFT    17
>>>> +
>>>> +/* AFE4404 CONTROL3 register fields */
>>>> +#define AFE440X_CONTROL3_OSC_ENABLE    BIT(9)
>>>> +
>>>> +/* AFE4404 OFFDAC register current fields */
>>>> +#define AFE4404_OFFDAC_CURR_LED1_MASK    GENMASK(9, 5)
>>>> +#define AFE4404_OFFDAC_CURR_LED1_SHIFT    5
>>>> +#define AFE4404_OFFDAC_CURR_LED2_MASK    GENMASK(19, 15)
>>>> +#define AFE4404_OFFDAC_CURR_LED2_SHIFT    15
>>>> +#define AFE4404_OFFDAC_CURR_LED3_MASK    GENMASK(4, 0)
>>>> +#define AFE4404_OFFDAC_CURR_LED3_SHIFT    0
>>>> +#define AFE4404_OFFDAC_CURR_ALED1_MASK    GENMASK(14, 10)
>>>> +#define AFE4404_OFFDAC_CURR_ALED1_SHIFT    10
>>>> +#define AFE4404_OFFDAC_CURR_ALED2_MASK    GENMASK(4, 0)
>>>> +#define AFE4404_OFFDAC_CURR_ALED2_SHIFT    0
>>>> +
>>>> +/* AFE4404 NULL fields */
>>>> +#define NULL_MASK    0
>>>> +#define NULL_SHIFT    0
>>>> +
>>>> +/* AFE4404 TIA_GAIN_CAP values */
>>>> +#define AFE4404_TIA_GAIN_CAP_5_P    0x0
>>>> +#define AFE4404_TIA_GAIN_CAP_2_5_P    0x1
>>>> +#define AFE4404_TIA_GAIN_CAP_10_P    0x2
>>>> +#define AFE4404_TIA_GAIN_CAP_7_5_P    0x3
>>>> +#define AFE4404_TIA_GAIN_CAP_20_P    0x4
>>>> +#define AFE4404_TIA_GAIN_CAP_17_5_P    0x5
>>>> +#define AFE4404_TIA_GAIN_CAP_25_P    0x6
>>>> +#define AFE4404_TIA_GAIN_CAP_22_5_P    0x7
>>>> +
>>>> +/* AFE4404 TIA_GAIN_RES values */
>>>> +#define AFE4404_TIA_GAIN_RES_500_K    0x0
>>>> +#define AFE4404_TIA_GAIN_RES_250_K    0x1
>>>> +#define AFE4404_TIA_GAIN_RES_100_K    0x2
>>>> +#define AFE4404_TIA_GAIN_RES_50_K    0x3
>>>> +#define AFE4404_TIA_GAIN_RES_25_K    0x4
>>>> +#define AFE4404_TIA_GAIN_RES_10_K    0x5
>>>> +#define AFE4404_TIA_GAIN_RES_1_M    0x6
>>>> +#define AFE4404_TIA_GAIN_RES_2_M    0x7
>>>> +
>>>> +enum afe4404_chan_id {
>>>> +    LED1,
>>>> +    ALED1,
>>>> +    LED2,
>>>> +    ALED2,
>>>> +    LED3,
>>>> +    LED1_ALED1,
>>>> +    LED2_ALED2,
>>>> +    AVG_LED1_ALED1,
>>>> +    AVG_LED2_ALED2,
>>>> +    ILED1,
>>>> +    ILED2,
>>>> +    ILED3,
>>>> +};
>>>> +
>>>> +#define AFE4404_REG_INFO(_id, _reg, _offreg, _sm)        \
>>>> +    [_id] = {                        \
>>>> +        .reg = _reg,                    \
>>>> +        .offreg = _offreg,                \
>>>> +        .shift = _sm ## _SHIFT,                \
>>>> +        .mask = _sm ## _MASK,                \
>>>> +    }
>>>> +
>>>> +struct {
>>>> +    unsigned int reg;
>>>> +    unsigned int offreg;
>>>> +    unsigned int shift;
>>>> +    unsigned int mask;
>>>> +} reg_info[] = {
>>>> +    AFE4404_REG_INFO(LED1, AFE440X_LED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED1),
>>>> +    AFE4404_REG_INFO(ALED1, AFE440X_ALED1VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED1),
>>>> +    AFE4404_REG_INFO(LED2, AFE440X_LED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_LED2),
>>>> +    AFE4404_REG_INFO(ALED2, AFE440X_ALED2VAL, AFE4404_OFFDAC, AFE4404_OFFDAC_CURR_ALED2),
>>>> +    AFE4404_REG_INFO(LED3, AFE440X_ALED2VAL, 0, NULL),
>>>> +    AFE4404_REG_INFO(LED1_ALED1, AFE440X_LED1_ALED1VAL, 0, NULL),
>>>> +    AFE4404_REG_INFO(LED2_ALED2, AFE440X_LED2_ALED2VAL, 0, NULL),
>>>> +    AFE4404_REG_INFO(AVG_LED1_ALED1, AFE4404_AVG_LED1_ALED1VAL, 0, NULL),
>>>> +    AFE4404_REG_INFO(AVG_LED2_ALED2, AFE4404_AVG_LED2_ALED2VAL, 0, NULL),
>>>> +    AFE4404_REG_INFO(ILED1, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED1),
>>>> +    AFE4404_REG_INFO(ILED2, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED2),
>>>> +    AFE4404_REG_INFO(ILED3, AFE440X_LEDCNTRL, 0, AFE4404_LEDCNTRL_ILED3),
>>>> +};
>>>> +
>>>> +static const struct iio_chan_spec afe4404_channels[] = {
>>>> +    /* ADC values */
>>>> +    AFE440X_INTENSITY_CHAN(LED1, "led1", BIT(IIO_CHAN_INFO_OFFSET)),
>>>> +    AFE440X_INTENSITY_CHAN(ALED1, "led1_ambient", BIT(IIO_CHAN_INFO_OFFSET)),
>>> (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)
>>>> +    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?
>>
>> I'm thinking these last two are kind of a mess right now, also I don't
>> expose the setting to enable these yet anyway, so, I think I will just
>> drop them and add them back in a latter patch that adds all their
>> required functionality together.
> Fair enough.
>>
>>>> +    /* 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.
>>>
>>
>> ACK
>>
>>>> +};
>>>> +
>>>> +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, &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, &reg_val);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    reg_val &= ~afe440x_reg->mask;
>>>> +    reg_val |= ((val << afe440x_reg->shift) & afe440x_reg->mask);
>>>> +
>>>> +    ret = regmap_write(afe440x->regmap, afe440x_reg->reg, reg_val);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    return count;
>>>> +}
>>>> +
>>>> +AFE440X_ATTR(tia_separate_en, AFE4404_TIA_GAIN_SEP, AFE440X_TIAGAIN_ENSEPGAIN);
>>>> +
>>>> +AFE440X_ATTR(tia_resistance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES);
>>>> +AFE440X_ATTR(tia_capacitance1, AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_CAP);
>>>> +
>>>> +AFE440X_ATTR(tia_resistance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_RES);
>>>> +AFE440X_ATTR(tia_capacitance2, AFE4404_TIA_GAIN_SEP, AFE4404_TIA_GAIN_CAP);
>>>> +
>>>> +static struct attribute *afe4404_attributes[] = {
>>>> +    &afe440x_reg_tia_separate_en.dev_attr.attr,
>>>> +    &afe440x_reg_tia_resistance1.dev_attr.attr,
>>>> +    &afe440x_reg_tia_capacitance1.dev_attr.attr,
>>>> +    &afe440x_reg_tia_resistance2.dev_attr.attr,
>>>> +    &afe440x_reg_tia_capacitance2.dev_attr.attr,
>>>> +    NULL
>>>> +};
>>>> +
>>>> +static const struct attribute_group afe4404_attribute_group = {
>>>> +    .attrs = afe4404_attributes
>>>> +};
>>>> +
>>>> +static int afe440x_read_raw(struct iio_dev *indio_dev,
>>>> +                struct iio_chan_spec const *chan,
>>>> +                int *val, int *val2, long mask)
>>>> +{
>>>> +    struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
>>>> +    int ret;
>>>> +
>>>> +    switch (chan->type) {
>>>> +    case IIO_INTENSITY:
>>>> +        switch (mask) {
>>>> +        case IIO_CHAN_INFO_RAW:
>>>> +            ret = regmap_read(afe440x->regmap,
>>>> +                      reg_info[chan->address].reg, val);
>>>> +            if (ret)
>>>> +                return ret;
>>>> +            return IIO_VAL_INT;
>>>> +        case IIO_CHAN_INFO_OFFSET:
>>>> +            ret = regmap_read(afe440x->regmap,
>>>> +                      reg_info[chan->address].offreg, val);
>>>> +            if (ret)
>>>> +                return ret;
>>>> +            *val &= reg_info[chan->address].mask;
>>>> +            *val >>= reg_info[chan->address].shift;
>>>> +            return IIO_VAL_INT;
>>>> +        }
>>>> +        break;
>>>> +    case IIO_CURRENT:
>>>> +        switch (mask) {
>>>> +        case IIO_CHAN_INFO_RAW:
>>>> +            ret = regmap_read(afe440x->regmap,
>>>> +                      reg_info[chan->address].reg, val);
>>>> +            if (ret)
>>>> +                return ret;
>>>> +            *val &= reg_info[chan->address].mask;
>>>> +            *val >>= reg_info[chan->address].shift;
>>>> +            return IIO_VAL_INT;
>>>> +        case IIO_CHAN_INFO_SCALE:
>>>> +            *val = 0;
>>>> +            *val2 = 800000;
>>>> +            return IIO_VAL_INT_PLUS_MICRO;
>>>> +        }
>>>> +        break;
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return -EINVAL;
>>>> +}
>>>> +
>>>> +static int afe440x_write_raw(struct iio_dev *indio_dev,
>>>> +                 struct iio_chan_spec const *chan,
>>>> +                 int val, int val2, long mask)
>>>> +{
>>>> +    struct afe440x_data *afe440x = iio_device_get_drvdata(indio_dev);
>>>> +    unsigned int reg_val;
>>>> +    int ret;
>>>> +
>>>> +    switch (chan->type) {
>>>> +    case IIO_INTENSITY:
>>>> +        switch (mask) {
>>>> +        case IIO_CHAN_INFO_OFFSET:
>>>> +            ret = regmap_read(afe440x->regmap,
>>>> +                      reg_info[chan->address].offreg,
>>>> +                      &reg_val);
>>>> +            if (ret)
>>>> +                return ret;
>>>> +            reg_val &= ~reg_info[chan->address].mask;
>>>> +            reg_val |= ((val << reg_info[chan->address].shift) &
>>>> +                    reg_info[chan->address].mask);
>>>> +            return regmap_write(afe440x->regmap,
>>>> +                        reg_info[chan->address].offreg,
>>>> +                        reg_val);
>>>> +        }
>>>> +        break;
>>>> +    case IIO_CURRENT:
>>>> +        switch (mask) {
>>>> +        case IIO_CHAN_INFO_RAW:
>>>> +            ret = regmap_read(afe440x->regmap,
>>>> +                      reg_info[chan->address].reg,
>>>> +                      &reg_val);
>>>> +            if (ret)
>>>> +                return ret;
>>>> +            reg_val &= ~reg_info[chan->address].mask;
>>>> +            reg_val |= ((val << reg_info[chan->address].shift) &
>>>> +                    reg_info[chan->address].mask);
>>>> +            return regmap_write(afe440x->regmap,
>>>> +                        reg_info[chan->address].reg,
>>>> +                        reg_val);
>>>> +        }
>>>> +        break;
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return -EINVAL;
>>>> +}
>>>> +
>>>> +static const struct iio_info afe4404_iio_info = {
>>>> +    .attrs    = &afe4404_attribute_group,
>>>> +    .read_raw = afe440x_read_raw,
>>>> +    .write_raw = afe440x_write_raw,
>>>> +    .driver_module = THIS_MODULE,
>>>> +};
>>>> +
>>>> +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, reg, i = 0;
>>>> +    s32 buffer[12];
>>>> +
>>>> +    for_each_set_bit(bit, indio_dev->active_scan_mask,
>>>> +             indio_dev->masklength) {
>>>> +        reg = reg_info[bit].reg;
>>> why the local reg variable? Stick it dirrectly in the functional call.
>>
>> No reason, just looked better to me. Will fix.
>>
>>>> +        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    },    \
>>>> +    { AFE440X_ADCRSTENDCT2,    6542    },    \
>>>> +    { AFE440X_LED1CONVST,    6543    },    \
>>>> +    { AFE440X_LED1CONVEND,    7003    },    \
>>>> +    { AFE440X_ALED1STC,    1280    },    \
>>>> +    { AFE440X_ALED1ENDC,    1598    },    \
>>>> +    { AFE440X_ADCRSTSTCT3,    7005    },    \
>>>> +    { AFE440X_ADCRSTENDCT3,    7011    },    \
>>>> +    { AFE440X_ALED1CONVST,    7012    },    \
>>>> +    { AFE440X_ALED1CONVEND,    7471    },    \
>>>> +    { AFE440X_PDNCYCLESTC,    7671    },    \
>>>> +    { AFE440X_PDNCYCLEENDC,    39199    }
>>>> +
>>>> +static const struct reg_sequence afe4404_reg_sequences[] = {
>>>> +    AFE4404_TIMING_PAIRS,
>>>> +    { AFE440X_CONTROL1, AFE440X_CONTROL1_TIMEREN },
>>>> +    { AFE4404_TIA_GAIN, AFE4404_TIA_GAIN_RES_50_K },
>>>> +    { AFE440X_LEDCNTRL, (0xf << AFE4404_LEDCNTRL_ILED1_SHIFT) |
>>>> +                (0x3 << AFE4404_LEDCNTRL_ILED2_SHIFT) |
>>>> +                (0x3 << AFE4404_LEDCNTRL_ILED3_SHIFT) },
>>>> +    { AFE440X_CONTROL2, AFE440X_CONTROL3_OSC_ENABLE    },
>>>> +};
>>>> +
>>>> +static const struct regmap_range afe4404_yes_ranges[] = {
>>>> +    regmap_reg_range(AFE440X_LED2VAL, AFE440X_LED1_ALED1VAL),
>>>> +    regmap_reg_range(AFE4404_AVG_LED2_ALED2VAL, AFE4404_AVG_LED1_ALED1VAL),
>>>> +};
>>>> +
>>>> +static const struct regmap_access_table afe4404_volatile_table = {
>>>> +    .yes_ranges = afe4404_yes_ranges,
>>>> +    .n_yes_ranges = ARRAY_SIZE(afe4404_yes_ranges),
>>>> +};
>>>> +
>>>> +static const struct regmap_config afe4404_regmap_config = {
>>>> +    .reg_bits = 8,
>>>> +    .val_bits = 24,
>>>> +
>>>> +    .max_register = AFE4404_AVG_LED1_ALED1VAL,
>>>> +    .cache_type = REGCACHE_RBTREE,
>>>> +    .volatile_table = &afe4404_volatile_table,
>>>> +};
>>>> +
>>>> +#ifdef CONFIG_OF
>>>> +static const struct of_device_id afe4404_of_match[] = {
>>>> +    { .compatible = "ti,afe4404", },
>>>> +    { /* sentinel */ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, afe4404_of_match);
>>>> +#endif
>>>> +
>>>> +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, "Failed to disable regulator\n");
>>>> +        return ret;
>>>> +    }
>>> 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!)
>>
>> Makes sense, will do.
>>
>>>> +
>>>> +    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...
>>>> +
>>>> +    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?
>>
>> I'm not sure but I belive the regulator will be enabled on 'get'
> It won't - has to be explicity enabled.  Now as likely as not on most
> boards it will be on anyway, but you never know!
>> and when all devices using the regulator have 'put' it will be
>> turned off.
> Not true either.  Will leave it on unless explicitly disabled.

Interesting, well, I'll add this stuff then.

>> I manualy disable/enable in the PM code so I don't
>> have to put/get them agian.
>>
>>>> +
>>>> +    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;
>>>> +
>>>> +        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->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);
>>> 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!
>>
>> Why do today what you can put off until tomorrow :) I'll look into that
>> in a later patch.
>>
>>>> +    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.
>>
>> I'm thinking now that this driver may be getting close to correct, I will
>> add the other part as part of this series if that is OK. Should clear up
>> some of the issues below to have them at the same time.
> Cool
>>
>>>> +
>>>> +    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!
>>>
>>>> +
>>>> +    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)
>>
>> ACK
>>
>>>> +    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@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..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@ti.com>
>>>> + *
>>>> + * Copyright: (C) 2015 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.
>>>> + */
>>>> +
>>>> +#ifndef _AFE440X_H
>>>> +#define _AFE440X_H
>>> 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.
>>
>> As above would it be better for me to push both now?
> Probably, easier to look at whether any refactoring make sense with
> both infront of us.

OK, next series will have both.

>>
>>>> +
>>>> +/* 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
>>>> +#define AFE440X_LED1VAL            0x2c
>>>> +#define AFE440X_ALED1VAL        0x2d
>>>> +#define AFE440X_LED2_ALED2VAL        0x2e
>>>> +#define AFE440X_LED1_ALED1VAL        0x2f
>>>> +#define AFE440X_CONTROL3        0x31
>>>> +#define AFE440X_PDNCYCLESTC        0x32
>>>> +#define AFE440X_PDNCYCLEENDC        0x33
>>>> +
>>>> +/* CONTROL0 register fields */
>>>> +#define AFE440X_CONTROL0_REG_READ    BIT(0)
>>>> +#define AFE440X_CONTROL0_TM_COUNT_RST    BIT(1)
>>>> +#define AFE440X_CONTROL0_SW_RESET    BIT(3)
>>>> +
>>>> +/* CONTROL1 register fields */
>>>> +#define AFE440X_CONTROL1_TIMEREN    BIT(8)
>>>> +
>>>> +/* TIAGAIN register fields */
>>>> +#define AFE440X_TIAGAIN_ENSEPGAIN_MASK    BIT(15)
>>>> +#define AFE440X_TIAGAIN_ENSEPGAIN_SHIFT    15
>>>> +
>>>> +/* CONTROL2 register fields */
>>>> +#define AFE440X_CONTROL2_PDN_AFE    BIT(0)
>>>> +#define AFE440X_CONTROL2_PDN_RX        BIT(1)
>>>> +#define AFE440X_CONTROL2_DYNAMIC4    BIT(3)
>>>> +#define AFE440X_CONTROL2_DYNAMIC3    BIT(4)
>>>> +#define AFE440X_CONTROL2_DYNAMIC2    BIT(14)
>>>> +#define AFE440X_CONTROL2_DYNAMIC1    BIT(20)
>>>> +
>>>> +/* CONTROL3 register fields */
>>>> +#define AFE440X_CONTROL3_CLKDIV        GENMASK(2, 0)
>>>> +
>>>> +/* CONTROL0 values */
>>>> +#define AFE440X_CONTROL0_WRITE        0x0
>>>> +#define AFE440X_CONTROL0_READ        0x1
>>>> +
>>>> +#define AFE440X_INTENSITY_CHAN(_index, _name, _mask)        \
>>>> +    {                            \
>>>> +        .type = IIO_INTENSITY,                \
>>>> +        .channel = _index,                \
>>>> +        .address = _index,                \
>>>> +        .scan_index = _index,                \
>>>> +        .scan_type = {                    \
>>>> +                .sign = 's',            \
>>>> +                .realbits = 24,            \
>>>> +                .storagebits = 32,        \
>>>> +                .endianness = IIO_CPU,        \
>>>> +        },                        \
>>>> +        .extend_name = _name,                \
>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |    \
>>>> +            _mask,                    \
>>>> +    }
>>>> +
>>>> +#define AFE440X_CURRENT_CHAN(_index, _name)            \
>>>> +    {                            \
>>>> +        .type = IIO_CURRENT,                \
>>>> +        .channel = _index,                \
>>>> +        .address = _index,                \
>>>> +        .scan_index = _index,                \
>>>> +        .extend_name = _name,                \
>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |    \
>>>> +            BIT(IIO_CHAN_INFO_SCALE),        \
>>>> +        .output = true,                    \
>>>> +    }
>>> I'd particularly like to see the above two definitions next to the
>>> arrays they are used to instantiate - makes for easier review!
>>>
>>>> +
>>>> +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
>>>> +
>>>> +/**
>>>> + * 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).
>>>
>>
>> ACK
>>
>>>> + * @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.
>>>> +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 */
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>