| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <566F3D56.4010509@ti.com>
Date: Mon, 14 Dec 2015 16:06:14 -0600
From: "Andrew F. Davis" <afd@...com>
To: Jonathan Cameron <jic23@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>
CC: <devicetree@...r.kernel.org>, <linux-iio@...r.kernel.org>,
<linux-api@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: health: Add driver for the TI AFE4404 heart
monitor
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@...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@...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@...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@...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@...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@...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@...r.kernel.org
>> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists