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

From: Jonathan Cameron
Date: Sat Dec 12 2015 - 10:41:15 EST

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@xxxxxx>
>> 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@xxxxxx>
>>> +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@xxxxxx>
>>> +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@xxxxxx>
>>> + *
>>> + * 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@xxxxxx>");
>>> +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@xxxxxx>
>>> + *
>>> + * 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/