Messages in this thread | | | Subject | Re: [PATCH v2 2/2] iio: health: Add driver for the TI AFE4404 heart monitor | From | "Andrew F. Davis" <> | Date | Mon, 14 Dec 2015 16:06:14 -0600 |
| |
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, ®_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, ®_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, >>>> + ®_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, >>>> + ®_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 >> >
| |