[2/3] iio: adc: add support for Allwinner SoCs ADC

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. This patch adds the ADC driver which is
based on the MFD for the same SoCs ADC.

This also registers the thermal adc channel in the iio map array so
iio_hwmon could use it without modifying the Device Tree.

This driver probes on three different platform_device_id to take into
account slight differences between Allwinner SoCs ADCs.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

---
 drivers/iio/adc/Kconfig           |  12 ++
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c | 371 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 384 insertions(+)
 create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c

-- 
2.5.0

On 03/07/2016 17:43, Guenter Roeck wrote:
> On 07/03/2016 04:54 AM, Jonathan Cameron wrote:

>> On 28/06/16 09:18, Quentin Schulz wrote:

>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen

>>> controller and a thermal sensor. This patch adds the ADC driver which is

>>> based on the MFD for the same SoCs ADC.

>>>

>>> This also registers the thermal adc channel in the iio map array so

>>> iio_hwmon could use it without modifying the Device Tree.

>>>

>>> This driver probes on three different platform_device_id to take into

>>> account slight differences between Allwinner SoCs ADCs.

>>>

>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

>> Hi Quentin.

>>

>> I'm a bit in two minds about some of this.  That temperature sensor is

>> so obviously meant for hwmon purposes, I'm tempted to suggest it might

>> actually make sense to put it directly in hwmon rather than using the

>> bridge.  That obviously makes it less flexible in some ways (i.e. for

>> use within the thermal subsystem at some point).

>>

>> Guenter, what do you think?

>>

> 

> With the upcoming new hwmon API, thermal registration is handled in the

> hwmon core, so that should not be an issue. Besides, other hwmon sensors

> already register with the thermal subsystem as well.

> 

> This is difficult to evaluate without datasheet; I am not sure if

> the chip supports limits or trip points. If it supports trip points,

> thermal may be a better target.

> 

> Overall it does look like the temperature sensor would warrant

> a separate driver. Only question is thermal or hwmon.

> 

> Guenter


Actually, the publicly available documentation for the temperature
sensor is almost inexistent. We only have the registers for it. For most
of the work on temperature sensor, it has been taken from the current
driver (sun4i-ts in input/touschreen) from Hans de Goede.

You can find the documentation here:
http://dl.linux-sunxi.org/A10/A10%20User%20manual%20V1.50.pdf
http://dl.linux-sunxi.org/A13/A13%20User%20Manual%20V1.30.pdf
http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf

The ADC is either called TP controller, Touch Panel controller or GPADC.
The temperature sensor's registers are only defined in the User Manual
of the A10 (first link).

Nothing to be found for limits or trip points I think.

Quentin

>> I'm guessing detailed docs for this part aren't avaiable publicly? :(

>>

>> So the rest of my comments are kind of predicated on me having roughtly

>> understood how this device works from the structure of the driver.

>>

>> The temperature sensor is really effectively as separate ADC?

>> The main interest in this is for key detection?

>>

>> Anyhow, if the data flow for the temperatures sensor is not synced with

>> the other ADC channels, adding buffered (pushed) output from the

>> driver in

>> future will be fiddly and with a 250Hz device you'll probably want it.

>> Basically IIO buffered supports assumes each iio device will sample data

>> at a particular frequency. If channels are not synchronized in that

>> fashion

>> then you have to register multiple devices or only pick a subset of

>> channels

>> to export.

>>

>> For the key detection you have already observed that IIO needs some

>> additions to be able to have consumers of what we term 'events' e.g.

>> threshold

>> interrupts.

>>

>> Looking at the lradc-keys driver in tree, it looks like we only really

>> have

>> really simple threshold interrups - configured to detect a very low

>> voltage?

>> + only one per channel.

>>

>> So not too nasty a case, but you are right some work is needed in IIO as

>> we simply don't have a means of passing these on as yet or configuring

>> them

>> from in kernel consumers.

>> If we take the easy route and don't demux incoming events then it

>> shouldn't

>> be too hard to add (demux can follow later).  Hence any client device

>> can try

>> to enable events it wants, but may get events that other client

>> devices wanted

>> as well.

>>

>> Config interface should be much the same as the write support for

>> channels.

>> Data flow marginally harder, but pretty much a list of callbacks within

>> iio_push_event.

>>

>> Not trivial, but not too tricky either.

>>

>> The events subsystem has a few 'limitations' we need to address long term

>> but as this is in kernel interface only, we can do this now and fix stuff

>> up in future without any ABI breakage. (limitations are things like only

>> one event of a given type and direction per channel - main challenge on

>> that is finding a way of doing it without abi breakage).

>>

>> Anyhow, sounds fun - wish I had the time to do it myself!

>>

>> Otherwise, your remove is never going to work as indio_dev is always

>> NULL.

>>

>> Jonathan

>>

>>> ---

>>>   drivers/iio/adc/Kconfig           |  12 ++

>>>   drivers/iio/adc/Makefile          |   1 +

>>>   drivers/iio/adc/sunxi-gpadc-iio.c | 371

>>> ++++++++++++++++++++++++++++++++++++++

>>>   3 files changed, 384 insertions(+)

>>>   create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c

>>>

>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig

>>> index 82c718c..b7b566a 100644

>>> --- a/drivers/iio/adc/Kconfig

>>> +++ b/drivers/iio/adc/Kconfig

>>> @@ -328,6 +328,18 @@ config NAU7802

>>>         To compile this driver as a module, choose M here: the

>>>         module will be called nau7802.

>>>

>>> +config SUNXI_ADC

>>> +    tristate "ADC driver for sunxi platforms"

>>> +    depends on IIO

>>> +    depends on MFD_SUNXI_ADC

>>> +    help

>>> +      Say yes here to build support for Allwinner SoCs (A10, A13 and

>>> A31)

>>> +      SoCs ADC. This ADC provides 4 channels which can be used as an

>>> ADC or

>>> +      as a touchscreen input and one channel for thermal sensor.

>>> +

>>> +          To compile this driver as a module, choose M here: the

>>> +          module will be called sunxi-gpadc-iio.

>>> +

>>>   config PALMAS_GPADC

>>>       tristate "TI Palmas General Purpose ADC"

>>>       depends on MFD_PALMAS

>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile

>>> index 0cb7921..2996a5b 100644

>>> --- a/drivers/iio/adc/Makefile

>>> +++ b/drivers/iio/adc/Makefile

>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o

>>>   obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o

>>>   obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o

>>>   obj-$(CONFIG_NAU7802) += nau7802.o

>>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o

>>>   obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o

>>>   obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o

>>>   obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o

>>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c

>>> b/drivers/iio/adc/sunxi-gpadc-iio.c

>>> new file mode 100644

>>> index 0000000..5840f43

>>> --- /dev/null

>>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c

>>> @@ -0,0 +1,371 @@

>>> +#include <linux/module.h>

>>> +#include <linux/platform_device.h>

>>> +#include <linux/io.h>

>>> +#include <linux/of.h>

>>> +#include <linux/of_device.h>

>>> +#include <linux/clk.h>

>>> +#include <linux/completion.h>

>>> +#include <linux/iio/iio.h>

>>> +#include <linux/iio/machine.h>

>>> +#include <linux/iio/driver.h>

>>> +#include <linux/interrupt.h>

>>> +#include <linux/mfd/sunxi-gpadc-mfd.h>

>>> +#include <linux/regmap.h>

>>> +

>> I'm fussy about this:  Please prefix all defines as some of these

>> are generic enough they may well end up defined in an included header

>> sometime in the future.

>>

>>> +#define TP_CTRL0            0x00

>>> +#define TP_CTRL1            0x04

>>> +#define TP_CTRL2            0x08

>>> +#define TP_CTRL3            0x0c

>>> +#define TP_TPR                0x18

>>> +#define TP_CDAT                0x1c

>>> +#define TEMP_DATA            0x20

>>> +#define TP_DATA                0x24

>>> +

>>> +/* TP_CTRL0 bits */

>>> +#define ADC_FIRST_DLY(x)        ((x) << 24) /* 8 bits */

>>> +#define ADC_FIRST_DLY_MODE        BIT(23)

>>> +#define ADC_CLK_SELECT            BIT(22)

>>> +#define ADC_CLK_DIVIDER(x)        ((x) << 20) /* 2 bits */

>>> +#define FS_DIV(x)            ((x) << 16) /* 4 bits */

>>> +#define T_ACQ(x)            ((x) << 0)  /* 16 bits*/

>>> +

>>> +/* TP_CTRL1 bits */

>>> +#define STYLUS_UP_DEBOUNCE(x)        ((x) << 12) /* 8 bits */

>>> +#define STYLUS_UP_DEBOUNCE_EN        BIT(9)

>>> +#define TOUCH_PAN_CALI_EN        BIT(6)

>>> +#define TP_DUAL_EN            BIT(5)

>>> +#define TP_MODE_EN            BIT(4)

>>> +#define TP_ADC_SELECT            BIT(3)

>>> +#define ADC_CHAN_SELECT(x)        ((x) << 0)  /* 3 bits */

>>> +

>>> +/* TP_CTRL1 bits for sun6i SOCs */

>>> +#define SUN6I_TOUCH_PAN_CALI_EN        BIT(7)

>>> +#define SUN6I_TP_DUAL_EN        BIT(6)

>>> +#define SUN6I_TP_MODE_EN        BIT(5)

>>> +#define SUN6I_TP_ADC_SELECT        BIT(4)

>>> +#define SUN6I_ADC_CHAN_SELECT(x)    BIT(x)  /* 4 bits */

>>> +

>>> +/* TP_CTRL2 bits */

>>> +#define TP_SENSITIVE_ADJUST(x)        ((x) << 28) /* 4 bits */

>>> +#define TP_MODE_SELECT(x)        ((x) << 26) /* 2 bits */

>>> +#define PRE_MEA_EN            BIT(24)

>>> +#define PRE_MEA_THRE_CNT(x)        ((x) << 0)  /* 24 bits*/

>>> +

>>> +/* TP_CTRL3 bits */

>>> +#define FILTER_EN            BIT(2)

>>> +#define FILTER_TYPE(x)            ((x) << 0)  /* 2 bits */

>>> +

>>> +/* TP_INT_FIFOC irq and fifo mask / control bits */

>>> +#define TEMP_IRQ_EN            BIT(18)

>>> +#define TP_OVERRUN_IRQ_EN        BIT(17)

>>> +#define TP_DATA_IRQ_EN            BIT(16)

>>> +#define TP_DATA_XY_CHANGE        BIT(13)

>>> +#define TP_FIFO_TRIG_LEVEL(x)        ((x) << 8)  /* 5 bits */

>>> +#define TP_DATA_DRQ_EN            BIT(7)

>>> +#define TP_FIFO_FLUSH            BIT(4)

>>> +#define TP_UP_IRQ_EN            BIT(1)

>>> +#define TP_DOWN_IRQ_EN            BIT(0)

>>> +

>>> +/* TP_INT_FIFOS irq and fifo status bits */

>>> +#define TEMP_DATA_PENDING        BIT(18)

>>> +#define FIFO_OVERRUN_PENDING        BIT(17)

>>> +#define FIFO_DATA_PENDING        BIT(16)

>>> +#define TP_IDLE_FLG            BIT(2)

>>> +#define TP_UP_PENDING            BIT(1)

>>> +#define TP_DOWN_PENDING            BIT(0)

>>> +

>>> +/* TP_TPR bits */

>>> +#define TEMP_ENABLE(x)            ((x) << 16)

>>> +#define TEMP_PERIOD(x)            ((x) << 0)  /*t = x * 256 * 16 /

>>> clkin*/

>>> +

>>> +#define ARCH_SUN4I            BIT(0)

>>> +#define ARCH_SUN5I            BIT(1)

>>> +#define ARCH_SUN6I            BIT(2)

>>> +

>>> +struct sunxi_gpadc_dev {

>>> +    void __iomem            *regs;

>>> +    struct completion        completion;

>>> +    int                temp_data;

>>> +    u32                adc_data;

>>> +    struct regmap            *regmap;

>>> +    unsigned int            fifo_data_irq;

>>> +    unsigned int            temp_data_irq;

>>> +    unsigned int            flags;

>>> +};

>>> +

>>> +#define ADC_CHANNEL(_channel, _name) {                \

>>> +    .type = IIO_VOLTAGE,                    \

>>> +    .indexed = 1,                        \

>>> +    .channel = _channel,                    \

>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \

>>> +    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \

>>> +    .datasheet_name = _name,                \

>>> +}

>>> +

>>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {

>>> +    {

>>> +        .adc_channel_label = "temp_adc",

>>> +        .consumer_dev_name = "iio_hwmon.0",

>>> +    },

>>> +    {},

>>> +};

>>> +

>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {

>>> +    ADC_CHANNEL(0, "adc_chan0"),

>>> +    ADC_CHANNEL(1, "adc_chan1"),

>>> +    ADC_CHANNEL(2, "adc_chan2"),

>>> +    ADC_CHANNEL(3, "adc_chan3"),

>>> +    {

>>> +        .type = IIO_TEMP,

>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),

>>> +        .datasheet_name = "temp_adc",

>>> +    },

>>> +};

>>> +

>>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel)

>>> +{

>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);

>>> +    int val = 0;

>>> +

>>> +    mutex_lock(&indio_dev->mlock);

>>> +

>>> +    reinit_completion(&info->completion);

>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |

>>> +                         SUN6I_TP_ADC_SELECT |

>>> +                         SUN6I_ADC_CHAN_SELECT(channel));

>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |

>>> +                         TP_FIFO_FLUSH);

>>> +    enable_irq(info->fifo_data_irq);

>>> +

>>> +    if (!wait_for_completion_timeout(&info->completion,

>>> +                     msecs_to_jiffies(100))) {

>>> +        disable_irq(info->fifo_data_irq);

>>> +        mutex_unlock(&indio_dev->mlock);

>>> +        return -ETIMEDOUT;

>>> +    }

>>> +

>>> +    val = info->adc_data;

>>> +    disable_irq(info->fifo_data_irq);

>>> +    mutex_unlock(&indio_dev->mlock);

>>> +

>>> +    return val;

>>> +}

>>> +

>>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)

>>> +{

>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);

>>> +    int val = 0;

>>> +

>>> +    mutex_lock(&indio_dev->mlock);

>>> +

>>> +    reinit_completion(&info->completion);

>>> +

>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |

>>> +                         TP_FIFO_FLUSH);

>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);

>>> +    enable_irq(info->temp_data_irq);

>>> +

>>> +    if (!wait_for_completion_timeout(&info->completion,

>>> +                     msecs_to_jiffies(100))) {

>>> +        disable_irq(info->temp_data_irq);

>>> +        mutex_unlock(&indio_dev->mlock);

>>> +        return -ETIMEDOUT;

>>> +    }

>>> +

>>> +    if (info->flags & ARCH_SUN4I)

>>> +        val = info->temp_data * 133 - 257000;

>>> +    else if (info->flags & ARCH_SUN5I)

>>> +        val = info->temp_data * 100 - 144700;

>>> +    else if (info->flags & ARCH_SUN6I)

>>> +        val = info->temp_data * 167 - 271000;

>>> +

>>> +    disable_irq(info->temp_data_irq);

>>> +    mutex_unlock(&indio_dev->mlock);

>>> +    return val;

>>> +}

>>> +

>>> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,

>>> +                struct iio_chan_spec const *chan,

>>> +                int *val, int *val2, long mask)

>>> +{

>>> +    switch (mask) {

>>> +    case IIO_CHAN_INFO_PROCESSED:

>>> +        *val = sunxi_gpadc_temp_read(indio_dev);

>>> +        if (*val < 0)

>>> +            return *val;

>>> +

>>> +        return IIO_VAL_INT;

>>> +    case IIO_CHAN_INFO_RAW:

>>> +        *val = sunxi_gpadc_adc_read(indio_dev, chan->channel);

>>> +        if (*val < 0)

>>> +            return *val;

>>> +

>>> +        return IIO_VAL_INT;

>>> +    default:

>>> +        break;

>>> +    }

>>> +

>>> +    return -EINVAL;

>>> +}

>>> +

>>> +static const struct iio_info sunxi_gpadc_iio_info = {

>>> +    .read_raw = sunxi_gpadc_read_raw,

>>> +    .driver_module = THIS_MODULE,

>>> +};

>>> +

>>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void

>>> *dev_id)

>>> +{

>>> +    struct sunxi_gpadc_dev *info = dev_id;

>>> +    int ret;

>>> +

>>> +    ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);

>>> +    if (ret == 0)

>>> +        complete(&info->completion);

>>> +

>>> +    return IRQ_HANDLED;

>>> +}

>> Interesting that it's a separate data flow for the temperature sensor.

>> That means that if you add buffered (pushed data) support in future

>> either

>> you'll need to split the device in two or not allow the temperature

>> channel

>> to go through the buffer interface.

>>> +

>>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void

>>> *dev_id)

>>> +{

>>> +    struct sunxi_gpadc_dev *info = dev_id;

>>> +    int ret;

>>> +

>>> +    ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);

>>> +    if (ret == 0)

>>> +        complete(&info->completion);

>>> +

>>> +    return IRQ_HANDLED;

>>> +}

>>> +

>>> +static int sunxi_gpadc_probe(struct platform_device *pdev)

>>> +{

>>> +    struct sunxi_gpadc_dev *info = NULL;

>>> +    struct iio_dev *indio_dev = NULL;

>>> +    int ret = 0;

>>> +    unsigned int irq;

>>> +    struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;

>>> +

>>> +    sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);

>>> +

>>> +    indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));

>>> +    if (!indio_dev) {

>>> +        dev_err(&pdev->dev, "failed to allocate iio device.\n");

>>> +        return -ENOMEM;

>>> +    }

>>> +    info = iio_priv(indio_dev);

>>> +

>>> +    info->regmap = sunxi_gpadc_mfd_dev->regmap;

>>> +    init_completion(&info->completion);

>>> +    indio_dev->name = dev_name(&pdev->dev);

>>> +    indio_dev->dev.parent = &pdev->dev;

>>> +    indio_dev->dev.of_node = pdev->dev.of_node;

>>> +    indio_dev->info = &sunxi_gpadc_iio_info;

>>> +    indio_dev->modes = INDIO_DIRECT_MODE;

>>> +    indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);

>>> +    indio_dev->channels = sunxi_gpadc_channels;

>>> +

>>> +    info->flags = platform_get_device_id(pdev)->driver_data;

>>> +

>>> +    regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |

>>> +                         FS_DIV(7) |

>>> +                         T_ACQ(63));

>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);

>>> +    regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));

>>> +    regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) |

>>> TEMP_PERIOD(1953));

>>> +

>>> +    irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");

>>> +    if (irq < 0) {

>>> +        dev_err(&pdev->dev,

>>> +            "no TEMP_DATA_PENDING interrupt registered\n");

>>> +        return irq;

>>> +    }

>>> +

>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);

>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,

>>> +                       sunxi_gpadc_temp_data_irq_handler,

>>> +                       0, "temp_data", info);

>>> +    if (ret < 0) {

>>> +        dev_err(&pdev->dev,

>>> +            "could not request TEMP_DATA_PENDING interrupt: %d\n",

>>> +            ret);

>>> +        return ret;

>>> +    }

>>> +

>>> +    info->temp_data_irq = irq;

>>> +    disable_irq(irq);

>>> +

>>> +    irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");

>> hohum.  A fifo?  This part is getting more interesting ;) I'll have to

>> dig out the datasheet at some point (if public).

>>> +    if (irq < 0) {

>>> +        dev_err(&pdev->dev,

>>> +            "no FIFO_DATA_PENDING interrupt registered\n");

>>> +        return irq;

>>> +    }

>>> +

>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);

>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,

>>> +                       sunxi_gpadc_fifo_data_irq_handler,

>>> +                       0, "fifo_data", info);

>>> +    if (ret < 0) {

>>> +        dev_err(&pdev->dev,

>>> +            "could not request FIFO_DATA_PENDING interrupt: %d\n",

>>> +            ret);

>>> +        return ret;

>>> +    }

>>> +

>>> +    info->fifo_data_irq = irq;

>>> +    disable_irq(irq);

>>> +

>>> +    ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);

>> As mentioned for previous patch I think this should be described

>> externally.

>> Chances are that some of those other adc channels are also going to be

>> in reality used for hwmon anyway so doing it in the device tree will give

>> you more flexibility.

>>

>>> +    if (ret < 0) {

>>> +        dev_err(&pdev->dev, "failed to register iio map array\n");

>>> +        return ret;

>>> +    }

>>> +

>>> +    platform_set_drvdata(pdev, indio_dev);

>>> +

>>> +    ret = iio_device_register(indio_dev);

>>> +    if (ret < 0) {

>>> +        dev_err(&pdev->dev, "could not register the device\n");

>>> +        iio_map_array_unregister(indio_dev);

>>> +        return ret;

>>> +    }

>>> +

>> This is kind of self evident when the device turns up so I'd not bother

>> cluttering up the logs with it as no additional information is given.

>>> +    dev_info(&pdev->dev, "successfully loaded\n");

>>> +

>>> +    return ret;

>>> +}

>>> +

>>> +static int sunxi_gpadc_remove(struct platform_device *pdev)

>>> +{

>>> +    struct iio_dev *indio_dev = NULL;

>> ?  If it's null the below isn't going to work as intended.

>> Missing a platform_get_drvdata which I'd just roll into the above

>> local variable definition.

>>

>>> +    struct sunxi_gpadc_dev *info = NULL;

>>> +

>>> +    iio_device_unregister(indio_dev);

>>> +    iio_map_array_unregister(indio_dev);

>>> +    info = iio_priv(indio_dev);

>> I'd roll this into the local variable declaration above

>> (it's only a trivial bit of pointer arithemetic after all.

>> struct sunxi_gpadc_dev *info = iio_priv(indio_dev);

>>

>>> +    regmap_write(info->regmap, TP_INT_FIFOC, 0);

>>> +

>>> +    return 0;

>>> +}

>>> +

>>> +static const struct platform_device_id sunxi_gpadc_id[] = {

>>> +    { "sun4i-a10-gpadc-iio", ARCH_SUN4I },

>>> +    { "sun5i-a13-gpadc-iio", ARCH_SUN5I },

>>> +    { "sun6i-a31-gpadc-iio", ARCH_SUN6I },

>>> +    { /*sentinel*/ },

>>> +};

>>> +

>>> +static struct platform_driver sunxi_gpadc_driver = {

>>> +    .driver = {

>>> +        .name = "sunxi-gpadc-iio",

>>> +    },

>>> +    .id_table = sunxi_gpadc_id,

>>> +    .probe = sunxi_gpadc_probe,

>>> +    .remove = sunxi_gpadc_remove,

>>> +};

>>> +

>>> +module_platform_driver(sunxi_gpadc_driver);

>>> +

>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");

>>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");

>>> +MODULE_LICENSE("GPL v2");

>>>

>>

>> -- 

>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in

>> the body of a message to majordomo@vger.kernel.org

>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

>>

>

On 04/07/2016 18:29, Guenter Roeck wrote:
> On 07/04/2016 12:26 AM, Quentin Schulz wrote:

>> On 03/07/2016 17:43, Guenter Roeck wrote:

>>> On 07/03/2016 04:54 AM, Jonathan Cameron wrote:

>>>> On 28/06/16 09:18, Quentin Schulz wrote:

>>>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen

>>>>> controller and a thermal sensor. This patch adds the ADC driver

>>>>> which is

>>>>> based on the MFD for the same SoCs ADC.

>>>>>

>>>>> This also registers the thermal adc channel in the iio map array so

>>>>> iio_hwmon could use it without modifying the Device Tree.

>>>>>

>>>>> This driver probes on three different platform_device_id to take into

>>>>> account slight differences between Allwinner SoCs ADCs.

>>>>>

>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

>>>> Hi Quentin.

>>>>

>>>> I'm a bit in two minds about some of this.  That temperature sensor is

>>>> so obviously meant for hwmon purposes, I'm tempted to suggest it might

>>>> actually make sense to put it directly in hwmon rather than using the

>>>> bridge.  That obviously makes it less flexible in some ways (i.e. for

>>>> use within the thermal subsystem at some point).

>>>>

>>>> Guenter, what do you think?

>>>>

>>>

>>> With the upcoming new hwmon API, thermal registration is handled in the

>>> hwmon core, so that should not be an issue. Besides, other hwmon sensors

>>> already register with the thermal subsystem as well.

>>>

>>> This is difficult to evaluate without datasheet; I am not sure if

>>> the chip supports limits or trip points. If it supports trip points,

>>> thermal may be a better target.

>>>

>>> Overall it does look like the temperature sensor would warrant

>>> a separate driver. Only question is thermal or hwmon.

>>>

>>> Guenter

>>

>> Actually, the publicly available documentation for the temperature

>> sensor is almost inexistent. We only have the registers for it. For most

>> of the work on temperature sensor, it has been taken from the current

>> driver (sun4i-ts in input/touschreen) from Hans de Goede.

>>

> Does this mean that the temperature data will be exported twice, once

> through

> iio and once through the touchscreen driver ?

> 

> Guenter


Currently, the temperature data is exported once in thermal framework
from the sun4i-ts input driver.

In the proposed patch, the temperature is exported twice: once in iio
framework from the IIO driver and once in hwmon framework from iio_hwmon
driver (which takes data from a channel of the IIO driver).

Quentin

>> You can find the documentation here:

>> http://dl.linux-sunxi.org/A10/A10%20User%20manual%20V1.50.pdf

>> http://dl.linux-sunxi.org/A13/A13%20User%20Manual%20V1.30.pdf

>> http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf

>>

>> The ADC is either called TP controller, Touch Panel controller or GPADC.

>> The temperature sensor's registers are only defined in the User Manual

>> of the A10 (first link).

>>

>> Nothing to be found for limits or trip points I think.

>>

>> Quentin

>>

>>>> I'm guessing detailed docs for this part aren't avaiable publicly? :(

>>>>

>>>> So the rest of my comments are kind of predicated on me having roughtly

>>>> understood how this device works from the structure of the driver.

>>>>

>>>> The temperature sensor is really effectively as separate ADC?

>>>> The main interest in this is for key detection?

>>>>

>>>> Anyhow, if the data flow for the temperatures sensor is not synced with

>>>> the other ADC channels, adding buffered (pushed) output from the

>>>> driver in

>>>> future will be fiddly and with a 250Hz device you'll probably want it.

>>>> Basically IIO buffered supports assumes each iio device will sample

>>>> data

>>>> at a particular frequency. If channels are not synchronized in that

>>>> fashion

>>>> then you have to register multiple devices or only pick a subset of

>>>> channels

>>>> to export.

>>>>

>>>> For the key detection you have already observed that IIO needs some

>>>> additions to be able to have consumers of what we term 'events' e.g.

>>>> threshold

>>>> interrupts.

>>>>

>>>> Looking at the lradc-keys driver in tree, it looks like we only really

>>>> have

>>>> really simple threshold interrups - configured to detect a very low

>>>> voltage?

>>>> + only one per channel.

>>>>

>>>> So not too nasty a case, but you are right some work is needed in

>>>> IIO as

>>>> we simply don't have a means of passing these on as yet or configuring

>>>> them

>>>> from in kernel consumers.

>>>> If we take the easy route and don't demux incoming events then it

>>>> shouldn't

>>>> be too hard to add (demux can follow later).  Hence any client device

>>>> can try

>>>> to enable events it wants, but may get events that other client

>>>> devices wanted

>>>> as well.

>>>>

>>>> Config interface should be much the same as the write support for

>>>> channels.

>>>> Data flow marginally harder, but pretty much a list of callbacks within

>>>> iio_push_event.

>>>>

>>>> Not trivial, but not too tricky either.

>>>>

>>>> The events subsystem has a few 'limitations' we need to address long

>>>> term

>>>> but as this is in kernel interface only, we can do this now and fix

>>>> stuff

>>>> up in future without any ABI breakage. (limitations are things like

>>>> only

>>>> one event of a given type and direction per channel - main challenge on

>>>> that is finding a way of doing it without abi breakage).

>>>>

>>>> Anyhow, sounds fun - wish I had the time to do it myself!

>>>>

>>>> Otherwise, your remove is never going to work as indio_dev is always

>>>> NULL.

>>>>

>>>> Jonathan

>>>>

>>>>> ---

>>>>>    drivers/iio/adc/Kconfig           |  12 ++

>>>>>    drivers/iio/adc/Makefile          |   1 +

>>>>>    drivers/iio/adc/sunxi-gpadc-iio.c | 371

>>>>> ++++++++++++++++++++++++++++++++++++++

>>>>>    3 files changed, 384 insertions(+)

>>>>>    create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c

>>>>>

>>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig

>>>>> index 82c718c..b7b566a 100644

>>>>> --- a/drivers/iio/adc/Kconfig

>>>>> +++ b/drivers/iio/adc/Kconfig

>>>>> @@ -328,6 +328,18 @@ config NAU7802

>>>>>          To compile this driver as a module, choose M here: the

>>>>>          module will be called nau7802.

>>>>>

>>>>> +config SUNXI_ADC

>>>>> +    tristate "ADC driver for sunxi platforms"

>>>>> +    depends on IIO

>>>>> +    depends on MFD_SUNXI_ADC

>>>>> +    help

>>>>> +      Say yes here to build support for Allwinner SoCs (A10, A13 and

>>>>> A31)

>>>>> +      SoCs ADC. This ADC provides 4 channels which can be used as an

>>>>> ADC or

>>>>> +      as a touchscreen input and one channel for thermal sensor.

>>>>> +

>>>>> +          To compile this driver as a module, choose M here: the

>>>>> +          module will be called sunxi-gpadc-iio.

>>>>> +

>>>>>    config PALMAS_GPADC

>>>>>        tristate "TI Palmas General Purpose ADC"

>>>>>        depends on MFD_PALMAS

>>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile

>>>>> index 0cb7921..2996a5b 100644

>>>>> --- a/drivers/iio/adc/Makefile

>>>>> +++ b/drivers/iio/adc/Makefile

>>>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o

>>>>>    obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o

>>>>>    obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o

>>>>>    obj-$(CONFIG_NAU7802) += nau7802.o

>>>>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o

>>>>>    obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o

>>>>>    obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o

>>>>>    obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o

>>>>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c

>>>>> b/drivers/iio/adc/sunxi-gpadc-iio.c

>>>>> new file mode 100644

>>>>> index 0000000..5840f43

>>>>> --- /dev/null

>>>>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c

>>>>> @@ -0,0 +1,371 @@

>>>>> +#include <linux/module.h>

>>>>> +#include <linux/platform_device.h>

>>>>> +#include <linux/io.h>

>>>>> +#include <linux/of.h>

>>>>> +#include <linux/of_device.h>

>>>>> +#include <linux/clk.h>

>>>>> +#include <linux/completion.h>

>>>>> +#include <linux/iio/iio.h>

>>>>> +#include <linux/iio/machine.h>

>>>>> +#include <linux/iio/driver.h>

>>>>> +#include <linux/interrupt.h>

>>>>> +#include <linux/mfd/sunxi-gpadc-mfd.h>

>>>>> +#include <linux/regmap.h>

>>>>> +

>>>> I'm fussy about this:  Please prefix all defines as some of these

>>>> are generic enough they may well end up defined in an included header

>>>> sometime in the future.

>>>>

>>>>> +#define TP_CTRL0            0x00

>>>>> +#define TP_CTRL1            0x04

>>>>> +#define TP_CTRL2            0x08

>>>>> +#define TP_CTRL3            0x0c

>>>>> +#define TP_TPR                0x18

>>>>> +#define TP_CDAT                0x1c

>>>>> +#define TEMP_DATA            0x20

>>>>> +#define TP_DATA                0x24

>>>>> +

>>>>> +/* TP_CTRL0 bits */

>>>>> +#define ADC_FIRST_DLY(x)        ((x) << 24) /* 8 bits */

>>>>> +#define ADC_FIRST_DLY_MODE        BIT(23)

>>>>> +#define ADC_CLK_SELECT            BIT(22)

>>>>> +#define ADC_CLK_DIVIDER(x)        ((x) << 20) /* 2 bits */

>>>>> +#define FS_DIV(x)            ((x) << 16) /* 4 bits */

>>>>> +#define T_ACQ(x)            ((x) << 0)  /* 16 bits*/

>>>>> +

>>>>> +/* TP_CTRL1 bits */

>>>>> +#define STYLUS_UP_DEBOUNCE(x)        ((x) << 12) /* 8 bits */

>>>>> +#define STYLUS_UP_DEBOUNCE_EN        BIT(9)

>>>>> +#define TOUCH_PAN_CALI_EN        BIT(6)

>>>>> +#define TP_DUAL_EN            BIT(5)

>>>>> +#define TP_MODE_EN            BIT(4)

>>>>> +#define TP_ADC_SELECT            BIT(3)

>>>>> +#define ADC_CHAN_SELECT(x)        ((x) << 0)  /* 3 bits */

>>>>> +

>>>>> +/* TP_CTRL1 bits for sun6i SOCs */

>>>>> +#define SUN6I_TOUCH_PAN_CALI_EN        BIT(7)

>>>>> +#define SUN6I_TP_DUAL_EN        BIT(6)

>>>>> +#define SUN6I_TP_MODE_EN        BIT(5)

>>>>> +#define SUN6I_TP_ADC_SELECT        BIT(4)

>>>>> +#define SUN6I_ADC_CHAN_SELECT(x)    BIT(x)  /* 4 bits */

>>>>> +

>>>>> +/* TP_CTRL2 bits */

>>>>> +#define TP_SENSITIVE_ADJUST(x)        ((x) << 28) /* 4 bits */

>>>>> +#define TP_MODE_SELECT(x)        ((x) << 26) /* 2 bits */

>>>>> +#define PRE_MEA_EN            BIT(24)

>>>>> +#define PRE_MEA_THRE_CNT(x)        ((x) << 0)  /* 24 bits*/

>>>>> +

>>>>> +/* TP_CTRL3 bits */

>>>>> +#define FILTER_EN            BIT(2)

>>>>> +#define FILTER_TYPE(x)            ((x) << 0)  /* 2 bits */

>>>>> +

>>>>> +/* TP_INT_FIFOC irq and fifo mask / control bits */

>>>>> +#define TEMP_IRQ_EN            BIT(18)

>>>>> +#define TP_OVERRUN_IRQ_EN        BIT(17)

>>>>> +#define TP_DATA_IRQ_EN            BIT(16)

>>>>> +#define TP_DATA_XY_CHANGE        BIT(13)

>>>>> +#define TP_FIFO_TRIG_LEVEL(x)        ((x) << 8)  /* 5 bits */

>>>>> +#define TP_DATA_DRQ_EN            BIT(7)

>>>>> +#define TP_FIFO_FLUSH            BIT(4)

>>>>> +#define TP_UP_IRQ_EN            BIT(1)

>>>>> +#define TP_DOWN_IRQ_EN            BIT(0)

>>>>> +

>>>>> +/* TP_INT_FIFOS irq and fifo status bits */

>>>>> +#define TEMP_DATA_PENDING        BIT(18)

>>>>> +#define FIFO_OVERRUN_PENDING        BIT(17)

>>>>> +#define FIFO_DATA_PENDING        BIT(16)

>>>>> +#define TP_IDLE_FLG            BIT(2)

>>>>> +#define TP_UP_PENDING            BIT(1)

>>>>> +#define TP_DOWN_PENDING            BIT(0)

>>>>> +

>>>>> +/* TP_TPR bits */

>>>>> +#define TEMP_ENABLE(x)            ((x) << 16)

>>>>> +#define TEMP_PERIOD(x)            ((x) << 0)  /*t = x * 256 * 16 /

>>>>> clkin*/

>>>>> +

>>>>> +#define ARCH_SUN4I            BIT(0)

>>>>> +#define ARCH_SUN5I            BIT(1)

>>>>> +#define ARCH_SUN6I            BIT(2)

>>>>> +

>>>>> +struct sunxi_gpadc_dev {

>>>>> +    void __iomem            *regs;

>>>>> +    struct completion        completion;

>>>>> +    int                temp_data;

>>>>> +    u32                adc_data;

>>>>> +    struct regmap            *regmap;

>>>>> +    unsigned int            fifo_data_irq;

>>>>> +    unsigned int            temp_data_irq;

>>>>> +    unsigned int            flags;

>>>>> +};

>>>>> +

>>>>> +#define ADC_CHANNEL(_channel, _name) {                \

>>>>> +    .type = IIO_VOLTAGE,                    \

>>>>> +    .indexed = 1,                        \

>>>>> +    .channel = _channel,                    \

>>>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \

>>>>> +    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \

>>>>> +    .datasheet_name = _name,                \

>>>>> +}

>>>>> +

>>>>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {

>>>>> +    {

>>>>> +        .adc_channel_label = "temp_adc",

>>>>> +        .consumer_dev_name = "iio_hwmon.0",

>>>>> +    },

>>>>> +    {},

>>>>> +};

>>>>> +

>>>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {

>>>>> +    ADC_CHANNEL(0, "adc_chan0"),

>>>>> +    ADC_CHANNEL(1, "adc_chan1"),

>>>>> +    ADC_CHANNEL(2, "adc_chan2"),

>>>>> +    ADC_CHANNEL(3, "adc_chan3"),

>>>>> +    {

>>>>> +        .type = IIO_TEMP,

>>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),

>>>>> +        .datasheet_name = "temp_adc",

>>>>> +    },

>>>>> +};

>>>>> +

>>>>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int

>>>>> channel)

>>>>> +{

>>>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);

>>>>> +    int val = 0;

>>>>> +

>>>>> +    mutex_lock(&indio_dev->mlock);

>>>>> +

>>>>> +    reinit_completion(&info->completion);

>>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |

>>>>> +                         SUN6I_TP_ADC_SELECT |

>>>>> +                         SUN6I_ADC_CHAN_SELECT(channel));

>>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |

>>>>> +                         TP_FIFO_FLUSH);

>>>>> +    enable_irq(info->fifo_data_irq);

>>>>> +

>>>>> +    if (!wait_for_completion_timeout(&info->completion,

>>>>> +                     msecs_to_jiffies(100))) {

>>>>> +        disable_irq(info->fifo_data_irq);

>>>>> +        mutex_unlock(&indio_dev->mlock);

>>>>> +        return -ETIMEDOUT;

>>>>> +    }

>>>>> +

>>>>> +    val = info->adc_data;

>>>>> +    disable_irq(info->fifo_data_irq);

>>>>> +    mutex_unlock(&indio_dev->mlock);

>>>>> +

>>>>> +    return val;

>>>>> +}

>>>>> +

>>>>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)

>>>>> +{

>>>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);

>>>>> +    int val = 0;

>>>>> +

>>>>> +    mutex_lock(&indio_dev->mlock);

>>>>> +

>>>>> +    reinit_completion(&info->completion);

>>>>> +

>>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |

>>>>> +                         TP_FIFO_FLUSH);

>>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);

>>>>> +    enable_irq(info->temp_data_irq);

>>>>> +

>>>>> +    if (!wait_for_completion_timeout(&info->completion,

>>>>> +                     msecs_to_jiffies(100))) {

>>>>> +        disable_irq(info->temp_data_irq);

>>>>> +        mutex_unlock(&indio_dev->mlock);

>>>>> +        return -ETIMEDOUT;

>>>>> +    }

>>>>> +

>>>>> +    if (info->flags & ARCH_SUN4I)

>>>>> +        val = info->temp_data * 133 - 257000;

>>>>> +    else if (info->flags & ARCH_SUN5I)

>>>>> +        val = info->temp_data * 100 - 144700;

>>>>> +    else if (info->flags & ARCH_SUN6I)

>>>>> +        val = info->temp_data * 167 - 271000;

>>>>> +

>>>>> +    disable_irq(info->temp_data_irq);

>>>>> +    mutex_unlock(&indio_dev->mlock);

>>>>> +    return val;

>>>>> +}

>>>>> +

>>>>> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,

>>>>> +                struct iio_chan_spec const *chan,

>>>>> +                int *val, int *val2, long mask)

>>>>> +{

>>>>> +    switch (mask) {

>>>>> +    case IIO_CHAN_INFO_PROCESSED:

>>>>> +        *val = sunxi_gpadc_temp_read(indio_dev);

>>>>> +        if (*val < 0)

>>>>> +            return *val;

>>>>> +

>>>>> +        return IIO_VAL_INT;

>>>>> +    case IIO_CHAN_INFO_RAW:

>>>>> +        *val = sunxi_gpadc_adc_read(indio_dev, chan->channel);

>>>>> +        if (*val < 0)

>>>>> +            return *val;

>>>>> +

>>>>> +        return IIO_VAL_INT;

>>>>> +    default:

>>>>> +        break;

>>>>> +    }

>>>>> +

>>>>> +    return -EINVAL;

>>>>> +}

>>>>> +

>>>>> +static const struct iio_info sunxi_gpadc_iio_info = {

>>>>> +    .read_raw = sunxi_gpadc_read_raw,

>>>>> +    .driver_module = THIS_MODULE,

>>>>> +};

>>>>> +

>>>>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void

>>>>> *dev_id)

>>>>> +{

>>>>> +    struct sunxi_gpadc_dev *info = dev_id;

>>>>> +    int ret;

>>>>> +

>>>>> +    ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);

>>>>> +    if (ret == 0)

>>>>> +        complete(&info->completion);

>>>>> +

>>>>> +    return IRQ_HANDLED;

>>>>> +}

>>>> Interesting that it's a separate data flow for the temperature sensor.

>>>> That means that if you add buffered (pushed data) support in future

>>>> either

>>>> you'll need to split the device in two or not allow the temperature

>>>> channel

>>>> to go through the buffer interface.

>>>>> +

>>>>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void

>>>>> *dev_id)

>>>>> +{

>>>>> +    struct sunxi_gpadc_dev *info = dev_id;

>>>>> +    int ret;

>>>>> +

>>>>> +    ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);

>>>>> +    if (ret == 0)

>>>>> +        complete(&info->completion);

>>>>> +

>>>>> +    return IRQ_HANDLED;

>>>>> +}

>>>>> +

>>>>> +static int sunxi_gpadc_probe(struct platform_device *pdev)

>>>>> +{

>>>>> +    struct sunxi_gpadc_dev *info = NULL;

>>>>> +    struct iio_dev *indio_dev = NULL;

>>>>> +    int ret = 0;

>>>>> +    unsigned int irq;

>>>>> +    struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;

>>>>> +

>>>>> +    sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);

>>>>> +

>>>>> +    indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));

>>>>> +    if (!indio_dev) {

>>>>> +        dev_err(&pdev->dev, "failed to allocate iio device.\n");

>>>>> +        return -ENOMEM;

>>>>> +    }

>>>>> +    info = iio_priv(indio_dev);

>>>>> +

>>>>> +    info->regmap = sunxi_gpadc_mfd_dev->regmap;

>>>>> +    init_completion(&info->completion);

>>>>> +    indio_dev->name = dev_name(&pdev->dev);

>>>>> +    indio_dev->dev.parent = &pdev->dev;

>>>>> +    indio_dev->dev.of_node = pdev->dev.of_node;

>>>>> +    indio_dev->info = &sunxi_gpadc_iio_info;

>>>>> +    indio_dev->modes = INDIO_DIRECT_MODE;

>>>>> +    indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);

>>>>> +    indio_dev->channels = sunxi_gpadc_channels;

>>>>> +

>>>>> +    info->flags = platform_get_device_id(pdev)->driver_data;

>>>>> +

>>>>> +    regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |

>>>>> +                         FS_DIV(7) |

>>>>> +                         T_ACQ(63));

>>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);

>>>>> +    regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));

>>>>> +    regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) |

>>>>> TEMP_PERIOD(1953));

>>>>> +

>>>>> +    irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");

>>>>> +    if (irq < 0) {

>>>>> +        dev_err(&pdev->dev,

>>>>> +            "no TEMP_DATA_PENDING interrupt registered\n");

>>>>> +        return irq;

>>>>> +    }

>>>>> +

>>>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);

>>>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,

>>>>> +                       sunxi_gpadc_temp_data_irq_handler,

>>>>> +                       0, "temp_data", info);

>>>>> +    if (ret < 0) {

>>>>> +        dev_err(&pdev->dev,

>>>>> +            "could not request TEMP_DATA_PENDING interrupt: %d\n",

>>>>> +            ret);

>>>>> +        return ret;

>>>>> +    }

>>>>> +

>>>>> +    info->temp_data_irq = irq;

>>>>> +    disable_irq(irq);

>>>>> +

>>>>> +    irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");

>>>> hohum.  A fifo?  This part is getting more interesting ;) I'll have to

>>>> dig out the datasheet at some point (if public).

>>>>> +    if (irq < 0) {

>>>>> +        dev_err(&pdev->dev,

>>>>> +            "no FIFO_DATA_PENDING interrupt registered\n");

>>>>> +        return irq;

>>>>> +    }

>>>>> +

>>>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);

>>>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,

>>>>> +                       sunxi_gpadc_fifo_data_irq_handler,

>>>>> +                       0, "fifo_data", info);

>>>>> +    if (ret < 0) {

>>>>> +        dev_err(&pdev->dev,

>>>>> +            "could not request FIFO_DATA_PENDING interrupt: %d\n",

>>>>> +            ret);

>>>>> +        return ret;

>>>>> +    }

>>>>> +

>>>>> +    info->fifo_data_irq = irq;

>>>>> +    disable_irq(irq);

>>>>> +

>>>>> +    ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);

>>>> As mentioned for previous patch I think this should be described

>>>> externally.

>>>> Chances are that some of those other adc channels are also going to be

>>>> in reality used for hwmon anyway so doing it in the device tree will

>>>> give

>>>> you more flexibility.

>>>>

>>>>> +    if (ret < 0) {

>>>>> +        dev_err(&pdev->dev, "failed to register iio map array\n");

>>>>> +        return ret;

>>>>> +    }

>>>>> +

>>>>> +    platform_set_drvdata(pdev, indio_dev);

>>>>> +

>>>>> +    ret = iio_device_register(indio_dev);

>>>>> +    if (ret < 0) {

>>>>> +        dev_err(&pdev->dev, "could not register the device\n");

>>>>> +        iio_map_array_unregister(indio_dev);

>>>>> +        return ret;

>>>>> +    }

>>>>> +

>>>> This is kind of self evident when the device turns up so I'd not bother

>>>> cluttering up the logs with it as no additional information is given.

>>>>> +    dev_info(&pdev->dev, "successfully loaded\n");

>>>>> +

>>>>> +    return ret;

>>>>> +}

>>>>> +

>>>>> +static int sunxi_gpadc_remove(struct platform_device *pdev)

>>>>> +{

>>>>> +    struct iio_dev *indio_dev = NULL;

>>>> ?  If it's null the below isn't going to work as intended.

>>>> Missing a platform_get_drvdata which I'd just roll into the above

>>>> local variable definition.

>>>>

>>>>> +    struct sunxi_gpadc_dev *info = NULL;

>>>>> +

>>>>> +    iio_device_unregister(indio_dev);

>>>>> +    iio_map_array_unregister(indio_dev);

>>>>> +    info = iio_priv(indio_dev);

>>>> I'd roll this into the local variable declaration above

>>>> (it's only a trivial bit of pointer arithemetic after all.

>>>> struct sunxi_gpadc_dev *info = iio_priv(indio_dev);

>>>>

>>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, 0);

>>>>> +

>>>>> +    return 0;

>>>>> +}

>>>>> +

>>>>> +static const struct platform_device_id sunxi_gpadc_id[] = {

>>>>> +    { "sun4i-a10-gpadc-iio", ARCH_SUN4I },

>>>>> +    { "sun5i-a13-gpadc-iio", ARCH_SUN5I },

>>>>> +    { "sun6i-a31-gpadc-iio", ARCH_SUN6I },

>>>>> +    { /*sentinel*/ },

>>>>> +};

>>>>> +

>>>>> +static struct platform_driver sunxi_gpadc_driver = {

>>>>> +    .driver = {

>>>>> +        .name = "sunxi-gpadc-iio",

>>>>> +    },

>>>>> +    .id_table = sunxi_gpadc_id,

>>>>> +    .probe = sunxi_gpadc_probe,

>>>>> +    .remove = sunxi_gpadc_remove,

>>>>> +};

>>>>> +

>>>>> +module_platform_driver(sunxi_gpadc_driver);

>>>>> +

>>>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");

>>>>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");

>>>>> +MODULE_LICENSE("GPL v2");

>>>>>

>>>>

>>>> -- 

>>>> To unsubscribe from this list: send the line "unsubscribe

>>>> linux-hwmon" in

>>>> the body of a message to majordomo@vger.kernel.org

>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

>>>>

>>>

>>

>