| 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: <577A8ED5.3080909@roeck-us.net>
Date: Mon, 4 Jul 2016 09:29:09 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Quentin Schulz <quentin.schulz@...e-electrons.com>,
Jonathan Cameron <jic23@...nel.org>, jdelvare@...e.com,
knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
maxime.ripard@...e-electrons.com, wens@...e.org,
lee.jones@...aro.org
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-iio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
thomas.petazzoni@...e-electrons.com,
antoine.tenart@...e-electrons.com
Subject: Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC
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@...e-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
> 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@...e-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@...r.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
Powered by blists - more mailing lists