| Message ID | 1468576754-3273-3-git-send-email-quentin.schulz@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Jul 15, 2016 at 11:59:12AM +0200, 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> > --- > > v2: > - add SUNXI_GPADC_ prefixes for defines, > - correct typo in Kconfig, > - reorder alphabetically includes, makefile, > - add license header, > - fix architecture variations not being handled in interrupt handlers or > read raw functions, > - fix unability to return negative values from thermal sensor, > - add gotos to reduce code repetition, > - fix irq variable being unsigned int instead of int, > - remove useless dev_err and dev_info, > - deactivate all interrupts if probe fails, > - fix iio_device_register on NULL variable, > - deactivate ADC in the IP when probe fails or when removing driver, > > drivers/iio/adc/Kconfig | 12 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 430 insertions(+) > create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 25378c5..184856f 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -338,6 +338,18 @@ config NAU7802 > To compile this driver as a module, choose M here: the > module will be called nau7802. > > +config SUNXI_ADC We try to avoid the SUNXI prefix usually, otherwise this driver will have a generic name (or at least is implicitly saying that it supports all the sunxi SoCs), while it supports only a subset of those SoCs. > + tristate "ADC driver for sunxi platforms" And you should also mention which ADC is supported, since we usually have several of them. Something like "Support for the Allwinner SoCs GPADC" > + depends on IIO > + depends on MFD_SUNXI_ADC The order of your patches is quite weird. You depend on an option that is not present yet? > + help > + Say yes here to build support for Allwinner (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 Your indentation is weird here, and the wrapping is likely to be wrong too. > + 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 38638d4..3e60a1d 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -37,6 +37,7 @@ 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 > obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o > +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o > obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o > obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o > obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.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..87cc913 > --- /dev/null > +++ b/drivers/iio/adc/sunxi-gpadc-iio.c > @@ -0,0 +1,417 @@ > +/* ADC driver for sunxi platforms > + * > + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons> > + * > + * 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. Your wrapping is wrong. > + */ > + > +#include <linux/completion.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/driver.h> > +#include <linux/iio/machine.h> > +#include <linux/mfd/sunxi-gpadc-mfd.h> > + > +#define SUNXI_GPADC_TP_CTRL0 0x00 > +#define SUNXI_GPADC_TP_CTRL1 0x04 > +#define SUNXI_GPADC_TP_CTRL2 0x08 > +#define SUNXI_GPADC_TP_CTRL3 0x0c > +#define SUNXI_GPADC_TP_TPR 0x18 > +#define SUNXI_GPADC_TP_CDAT 0x1c > +#define SUNXI_GPADC_TEMP_DATA 0x20 > +#define SUNXI_GPADC_TP_DATA 0x24 > + > +/* TP_CTRL0 bits */ > +#define SUNXI_GPADC_ADC_FIRST_DLY(x) ((x) << 24) /* 8 bits */ > +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE BIT(23) > +#define SUNXI_GPADC_ADC_CLK_SELECT BIT(22) > +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x) ((x) << 20) /* 2 bits */ > +#define SUNXI_GPADC_FS_DIV(x) ((x) << 16) /* 4 bits */ > +#define SUNXI_GPADC_T_ACQ(x) ((x) << 0) /* 16 bits */ We usually prefer to have the bits defined directly after the registers, and prefixed with the name of the register they belong to. Something like SUNXI_GPADC_TP_CTRL_T_ACQ in this case > + > +/* TP_CTRL1 bits */ > +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x) ((x) << 12) /* 8 bits */ > +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN BIT(9) > +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN BIT(6) > +#define SUNXI_GPADC_TP_DUAL_EN BIT(5) > +#define SUNXI_GPADC_TP_MODE_EN BIT(4) > +#define SUNXI_GPADC_TP_ADC_SELECT BIT(3) > +#define SUNXI_GPADC_ADC_CHAN_SELECT(x) ((x) << 0) /* 3 bits */ Usually the comments are on the line above. However, if you really want to enforce something, you should rather mask the value. Otherwise, that comment is pretty useless. > + > +/* TP_CTRL1 bits for sun6i SOCs */ > +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN BIT(7) > +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN BIT(6) > +#define SUNXI_GPADC_SUN6I_TP_MODE_EN BIT(5) > +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT BIT(4) Shouldn't that go in either a common define or the touchscreen driver? > +#define SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(x) BIT(x) /* 4 bits */ > + > +/* TP_CTRL2 bits */ > +#define SUNXI_GPADC_TP_SENSITIVE_ADJUST(x) ((x) << 28) /* 4 bits */ > +#define SUNXI_GPADC_TP_MODE_SELECT(x) ((x) << 26) /* 2 bits */ > +#define SUNXI_GPADC_PRE_MEA_EN BIT(24) > +#define SUNXI_GPADC_PRE_MEA_THRE_CNT(x) ((x) << 0) /* 24 bits */ > + > +/* TP_CTRL3 bits */ > +#define SUNXI_GPADC_FILTER_EN BIT(2) > +#define SUNXI_GPADC_FILTER_TYPE(x) ((x) << 0) /* 2 bits */ > + > +/* TP_INT_FIFOC irq and fifo mask / control bits */ > +#define SUNXI_GPADC_TEMP_IRQ_EN BIT(18) > +#define SUNXI_GPADC_TP_OVERRUN_IRQ_EN BIT(17) > +#define SUNXI_GPADC_TP_DATA_IRQ_EN BIT(16) > +#define SUNXI_GPADC_TP_DATA_XY_CHANGE BIT(13) > +#define SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(x) ((x) << 8) /* 5 bits */ > +#define SUNXI_GPADC_TP_DATA_DRQ_EN BIT(7) > +#define SUNXI_GPADC_TP_FIFO_FLUSH BIT(4) > +#define SUNXI_GPADC_TP_UP_IRQ_EN BIT(1) > +#define SUNXI_GPADC_TP_DOWN_IRQ_EN BIT(0) > + > +/* TP_INT_FIFOS irq and fifo status bits */ > +#define SUNXI_GPADC_TEMP_DATA_PENDING BIT(18) > +#define SUNXI_GPADC_FIFO_OVERRUN_PENDING BIT(17) > +#define SUNXI_GPADC_FIFO_DATA_PENDING BIT(16) > +#define SUNXI_GPADC_TP_IDLE_FLG BIT(2) > +#define SUNXI_GPADC_TP_UP_PENDING BIT(1) > +#define SUNXI_GPADC_TP_DOWN_PENDING BIT(0) > + > +/* TP_TPR bits */ > +#define SUNXI_GPADC_TEMP_ENABLE(x) ((x) << 16) > +/* t = x * 256 * 16 / clkin */ That comment would be better next to the code that does that computation. > +#define SUNXI_GPADC_TEMP_PERIOD(x) ((x) << 0) > + > +#define SUNXI_GPADC_ARCH_SUN4I BIT(0) > +#define SUNXI_GPADC_ARCH_SUN5I BIT(1) > +#define SUNXI_GPADC_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 SUNXI_GPADC_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", > + }, > + { /* sentinel */ }, > +}; > + > +static const struct iio_chan_spec sunxi_gpadc_channels[] = { > + SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"), > + SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"), > + SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"), > + SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"), > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > + .datasheet_name = "temp_adc", > + .extend_name = "SoC temperature", > + }, > + { /* sentinel */ }, > +}; > + > +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel, > + int *val) > +{ > + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); > + int ret = 0; > + > + mutex_lock(&indio_dev->mlock); > + > + reinit_completion(&info->completion); > + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_SUN6I_TP_MODE_EN | > + SUNXI_GPADC_SUN6I_TP_ADC_SELECT | > + SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel)); > + else > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_TP_MODE_EN | > + SUNXI_GPADC_TP_ADC_SELECT | > + SUNXI_GPADC_ADC_CHAN_SELECT(channel)); Using a function pointer that would compute this, or some fields in a structure to store this would be better. > + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, > + SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) | > + SUNXI_GPADC_TP_FIFO_FLUSH); > + enable_irq(info->fifo_data_irq); > + > + if (!wait_for_completion_timeout(&info->completion, > + msecs_to_jiffies(100))) { > + ret = -ETIMEDOUT; > + goto out; > + } > + > + *val = info->adc_data; > + > +out: > + disable_irq(info->fifo_data_irq); > + mutex_unlock(&indio_dev->mlock); > + > + return ret; > +} > + > +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > +{ > + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); > + int ret = 0; > + > + mutex_lock(&indio_dev->mlock); > + > + reinit_completion(&info->completion); > + > + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, > + SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) | > + SUNXI_GPADC_TP_FIFO_FLUSH); > + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_SUN6I_TP_MODE_EN); > + else > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_TP_MODE_EN); > + enable_irq(info->temp_data_irq); > + > + if (!wait_for_completion_timeout(&info->completion, > + msecs_to_jiffies(100))) { > + ret = -ETIMEDOUT; > + goto out; > + } > + > + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) > + *val = info->temp_data * 133 - 257000; > + else if (info->flags & SUNXI_GPADC_ARCH_SUN5I) > + *val = info->temp_data * 100 - 144700; > + else if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + *val = info->temp_data * 167 - 271000; Ditto, having functions to comptue this and just store the function pointer would be better. > + > +out: > + disable_irq(info->temp_data_irq); > + mutex_unlock(&indio_dev->mlock); > + > + return ret; > + > +} > + > +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + ret = sunxi_gpadc_temp_read(indio_dev, val); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_RAW: > + ret = sunxi_gpadc_adc_read(indio_dev, chan->channel, val); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + > + 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, SUNXI_GPADC_TEMP_DATA, > + &info->temp_data); > + if (ret == 0) > + complete(&info->completion); > + > + return IRQ_HANDLED; > +} > + > +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, SUNXI_GPADC_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; > + struct iio_dev *indio_dev; > + int ret, 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) > + 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, SUNXI_GPADC_TP_CTRL0, SUNXI_GPADC_FS_DIV(7) | > + SUNXI_GPADC_ADC_CLK_DIVIDER(2) | SUNXI_GPADC_T_ACQ(63)); > + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_SUN6I_TP_MODE_EN); > + else > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_TP_MODE_EN); > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3, SUNXI_GPADC_FILTER_EN | > + SUNXI_GPADC_FILTER_TYPE(1)); > + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, > + SUNXI_GPADC_TEMP_ENABLE(1) | > + SUNXI_GPADC_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"); > + ret = irq; > + goto err; > + } > + > + 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); > + goto err; > + } > + > + info->temp_data_irq = irq; > + disable_irq(irq); > + > + irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING"); > + if (irq < 0) { > + dev_err(&pdev->dev, > + "no FIFO_DATA_PENDING interrupt registered\n"); > + ret = irq; > + goto err; > + } > + > + 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); > + goto err; > + } > + > + info->fifo_data_irq = irq; > + disable_irq(irq); request_irq starts with irq enabled, which means that you can have an interrupt showing up between your call to request_irq and the disable_irq. How would that work? > + > + ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to register iio map array\n"); > + goto err; > + } > + > + 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); That should go in a separate label. > + goto err; > + } > + > + return 0; > + > +err: > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); > + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); Why is that needed? > + return ret; > +} > + > +static int sunxi_gpadc_remove(struct platform_device *pdev) > +{ > + struct sunxi_gpadc_dev *info; > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + > + info = iio_priv(indio_dev); > + iio_device_unregister(indio_dev); > + iio_map_array_unregister(indio_dev); > + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0); > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); > + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); > + > + return 0; > +} > + > +static const struct platform_device_id sunxi_gpadc_id[] = { > + { "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I }, > + { "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I }, > + { "sun6i-a31-gpadc-iio", SUNXI_GPADC_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, > +}; Having some runtime_pm support for this would be great too. Thanks! Maxime
On 15/07/16 10:59, 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, Various bits inline. In particular the irq handling looks flakey / racey to me. Definitely need some explanatory comments. Also another note on the craziness that using extended_name to provide the hwmon labels will cause :) Jonathan > --- > > v2: > - add SUNXI_GPADC_ prefixes for defines, > - correct typo in Kconfig, > - reorder alphabetically includes, makefile, > - add license header, > - fix architecture variations not being handled in interrupt handlers or > read raw functions, > - fix unability to return negative values from thermal sensor, > - add gotos to reduce code repetition, > - fix irq variable being unsigned int instead of int, > - remove useless dev_err and dev_info, > - deactivate all interrupts if probe fails, > - fix iio_device_register on NULL variable, > - deactivate ADC in the IP when probe fails or when removing driver, > > drivers/iio/adc/Kconfig | 12 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 430 insertions(+) > create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 25378c5..184856f 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -338,6 +338,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 (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 38638d4..3e60a1d 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -37,6 +37,7 @@ 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 > obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o > +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o > obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o > obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o > obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.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..87cc913 > --- /dev/null > +++ b/drivers/iio/adc/sunxi-gpadc-iio.c > @@ -0,0 +1,417 @@ > +/* ADC driver for sunxi platforms > + * > + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons> > + * > + * 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. > + */ > + > +#include <linux/completion.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/driver.h> > +#include <linux/iio/machine.h> > +#include <linux/mfd/sunxi-gpadc-mfd.h> > + > +#define SUNXI_GPADC_TP_CTRL0 0x00 > +#define SUNXI_GPADC_TP_CTRL1 0x04 > +#define SUNXI_GPADC_TP_CTRL2 0x08 > +#define SUNXI_GPADC_TP_CTRL3 0x0c > +#define SUNXI_GPADC_TP_TPR 0x18 > +#define SUNXI_GPADC_TP_CDAT 0x1c > +#define SUNXI_GPADC_TEMP_DATA 0x20 > +#define SUNXI_GPADC_TP_DATA 0x24 > + > +/* TP_CTRL0 bits */ > +#define SUNXI_GPADC_ADC_FIRST_DLY(x) ((x) << 24) /* 8 bits */ > +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE BIT(23) > +#define SUNXI_GPADC_ADC_CLK_SELECT BIT(22) > +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x) ((x) << 20) /* 2 bits */ > +#define SUNXI_GPADC_FS_DIV(x) ((x) << 16) /* 4 bits */ > +#define SUNXI_GPADC_T_ACQ(x) ((x) << 0) /* 16 bits */ > + > +/* TP_CTRL1 bits */ > +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x) ((x) << 12) /* 8 bits */ > +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN BIT(9) > +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN BIT(6) > +#define SUNXI_GPADC_TP_DUAL_EN BIT(5) > +#define SUNXI_GPADC_TP_MODE_EN BIT(4) > +#define SUNXI_GPADC_TP_ADC_SELECT BIT(3) > +#define SUNXI_GPADC_ADC_CHAN_SELECT(x) ((x) << 0) /* 3 bits */ > + > +/* TP_CTRL1 bits for sun6i SOCs */ > +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN BIT(7) > +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN BIT(6) > +#define SUNXI_GPADC_SUN6I_TP_MODE_EN BIT(5) > +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT BIT(4) > +#define SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(x) BIT(x) /* 4 bits */ > + > +/* TP_CTRL2 bits */ > +#define SUNXI_GPADC_TP_SENSITIVE_ADJUST(x) ((x) << 28) /* 4 bits */ > +#define SUNXI_GPADC_TP_MODE_SELECT(x) ((x) << 26) /* 2 bits */ > +#define SUNXI_GPADC_PRE_MEA_EN BIT(24) > +#define SUNXI_GPADC_PRE_MEA_THRE_CNT(x) ((x) << 0) /* 24 bits */ > + > +/* TP_CTRL3 bits */ > +#define SUNXI_GPADC_FILTER_EN BIT(2) > +#define SUNXI_GPADC_FILTER_TYPE(x) ((x) << 0) /* 2 bits */ > + > +/* TP_INT_FIFOC irq and fifo mask / control bits */ > +#define SUNXI_GPADC_TEMP_IRQ_EN BIT(18) > +#define SUNXI_GPADC_TP_OVERRUN_IRQ_EN BIT(17) > +#define SUNXI_GPADC_TP_DATA_IRQ_EN BIT(16) > +#define SUNXI_GPADC_TP_DATA_XY_CHANGE BIT(13) > +#define SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(x) ((x) << 8) /* 5 bits */ > +#define SUNXI_GPADC_TP_DATA_DRQ_EN BIT(7) > +#define SUNXI_GPADC_TP_FIFO_FLUSH BIT(4) > +#define SUNXI_GPADC_TP_UP_IRQ_EN BIT(1) > +#define SUNXI_GPADC_TP_DOWN_IRQ_EN BIT(0) > + > +/* TP_INT_FIFOS irq and fifo status bits */ > +#define SUNXI_GPADC_TEMP_DATA_PENDING BIT(18) > +#define SUNXI_GPADC_FIFO_OVERRUN_PENDING BIT(17) > +#define SUNXI_GPADC_FIFO_DATA_PENDING BIT(16) > +#define SUNXI_GPADC_TP_IDLE_FLG BIT(2) > +#define SUNXI_GPADC_TP_UP_PENDING BIT(1) > +#define SUNXI_GPADC_TP_DOWN_PENDING BIT(0) > + > +/* TP_TPR bits */ > +#define SUNXI_GPADC_TEMP_ENABLE(x) ((x) << 16) > +/* t = x * 256 * 16 / clkin */ > +#define SUNXI_GPADC_TEMP_PERIOD(x) ((x) << 0) > + > +#define SUNXI_GPADC_ARCH_SUN4I BIT(0) > +#define SUNXI_GPADC_ARCH_SUN5I BIT(1) > +#define SUNXI_GPADC_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; I'd prefer something more explicit than this. Right now only one bit can ever be set - indicating a particular chip family. Why not just have an enum instead? > +}; > + > +#define SUNXI_GPADC_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", > + }, > + { /* sentinel */ }, > +}; > + > +static const struct iio_chan_spec sunxi_gpadc_channels[] = { > + SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"), > + SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"), > + SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"), > + SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"), > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > + .datasheet_name = "temp_adc", > + .extend_name = "SoC temperature", Just curious, did you look at the resultling sysfs entries? Going to be something like in_temp_SoC\ Temperature_input... Not good. If there is a strong enough reason (and there may be) to add a 'help string' type label to struct iio_chan_spec then my only real worries would be that we would be adding a whole pile of ABI that would be in the, near impossible to change in future, category... > + }, > + { /* sentinel */ }, > +}; > + > +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel, > + int *val) > +{ > + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); > + int ret = 0; > + > + mutex_lock(&indio_dev->mlock); > + > + reinit_completion(&info->completion); > + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_SUN6I_TP_MODE_EN | > + SUNXI_GPADC_SUN6I_TP_ADC_SELECT | > + SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel)); > + else > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_TP_MODE_EN | > + SUNXI_GPADC_TP_ADC_SELECT | > + SUNXI_GPADC_ADC_CHAN_SELECT(channel)); > + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, > + SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) | > + SUNXI_GPADC_TP_FIFO_FLUSH); > + enable_irq(info->fifo_data_irq); > + > + if (!wait_for_completion_timeout(&info->completion, > + msecs_to_jiffies(100))) { > + ret = -ETIMEDOUT; > + goto out; > + } > + > + *val = info->adc_data; > + > +out: > + disable_irq(info->fifo_data_irq); > + mutex_unlock(&indio_dev->mlock); > + > + return ret; > +} > + > +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val) > +{ > + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); > + int ret = 0; > + > + mutex_lock(&indio_dev->mlock); > + > + reinit_completion(&info->completion); > + > + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, > + SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) | > + SUNXI_GPADC_TP_FIFO_FLUSH); > + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_SUN6I_TP_MODE_EN); > + else > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_TP_MODE_EN); > + enable_irq(info->temp_data_irq); Is this hardware spitting out extra irqs? If not, much better to just leave it enabled all the time and control whether it can occur or not by controlling the device state.. > + > + if (!wait_for_completion_timeout(&info->completion, > + msecs_to_jiffies(100))) { > + ret = -ETIMEDOUT; > + goto out; > + } > + > + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) > + *val = info->temp_data * 133 - 257000; Why report as processed? I'd just report them as raw with the scale and offset provided. It's not a big thing, but if we can leave it so that the conversion only occurs when desired, why not? For in kernel users, this all happen 'automagically' anyway ;) > + else if (info->flags & SUNXI_GPADC_ARCH_SUN5I) > + *val = info->temp_data * 100 - 144700; > + else if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + *val = info->temp_data * 167 - 271000; > + > +out: > + disable_irq(info->temp_data_irq); > + mutex_unlock(&indio_dev->mlock); mlock has a very specific purpose - locking the state changes of between 'buffered' (push) and poll modes. Don't use it for anything else. In fact it's almost always a bug to access it directly at all. We have nice wrappers now for checking and locking the access mode. Just have a local lock in your iio_priv structure. > + > + return ret; > + > +} > + > +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + ret = sunxi_gpadc_temp_read(indio_dev, val); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_RAW: > + ret = sunxi_gpadc_adc_read(indio_dev, chan->channel, val); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + > + 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, SUNXI_GPADC_TEMP_DATA, > + &info->temp_data); > + if (ret == 0) > + complete(&info->completion); > + > + return IRQ_HANDLED; > +} > + > +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, SUNXI_GPADC_TP_DATA, &info->adc_data); > + if (ret == 0) > + complete(&info->completion); if (!regmap_read(...))? > + > + return IRQ_HANDLED; > +} > + > +static int sunxi_gpadc_probe(struct platform_device *pdev) > +{ > + struct sunxi_gpadc_dev *info; > + struct iio_dev *indio_dev; > + int ret, 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) > + 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, SUNXI_GPADC_TP_CTRL0, SUNXI_GPADC_FS_DIV(7) | > + SUNXI_GPADC_ADC_CLK_DIVIDER(2) | SUNXI_GPADC_T_ACQ(63)); > + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_SUN6I_TP_MODE_EN); > + else > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, > + SUNXI_GPADC_TP_MODE_EN); > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3, SUNXI_GPADC_FILTER_EN | > + SUNXI_GPADC_FILTER_TYPE(1)); > + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, > + SUNXI_GPADC_TEMP_ENABLE(1) | > + SUNXI_GPADC_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"); > + ret = irq; > + goto err; > + } > + > + 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); > + goto err; > + } > + > + info->temp_data_irq = irq; > + disable_irq(irq); As below, I want a comment explaining why you are disabling the irq here. This is clearly racey.. > + > + irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING"); > + if (irq < 0) { > + dev_err(&pdev->dev, > + "no FIFO_DATA_PENDING interrupt registered\n"); > + ret = irq; > + goto err; > + } > + > + 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); > + goto err; > + } > + > + info->fifo_data_irq = irq; There's a race here - this may be the only way of doing it though. However, I want a comment explaining why here... Are we looking at a hardware bug? > + disable_irq(irq); > + > + ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to register iio map array\n"); > + goto err; > + } > + > + 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); Once you have an unwinding path (which is sensible here) put everything in that block - it makes it easier to review (initially I compared that block with the remove and thought you'd missed this error path). > + goto err; > + } > + > + return 0; > + > +err: > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); > + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); > + > + return ret; > +} > + > +static int sunxi_gpadc_remove(struct platform_device *pdev) > +{ > + struct sunxi_gpadc_dev *info; > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + > + info = iio_priv(indio_dev); > + iio_device_unregister(indio_dev); > + iio_map_array_unregister(indio_dev); Slight disrepancy here between the cleanup in the error path in probe vs what we have in remove (next line doesn't exist in probe cleanup) Perhaps a comment here would make it clear why... > + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0); > + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); > + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); > + > + return 0; > +} > + > +static const struct platform_device_id sunxi_gpadc_id[] = { > + { "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I }, > + { "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I }, > + { "sun6i-a31-gpadc-iio", SUNXI_GPADC_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 18/07/2016 15:18, Jonathan Cameron wrote: > On 15/07/16 10:59, 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, > > Various bits inline. In particular the irq handling looks flakey / racey > to me. Definitely need some explanatory comments. > > Also another note on the craziness that using extended_name to provide > the hwmon labels will cause :) > > Jonathan [...] >> +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; > I'd prefer something more explicit than this. Right now only one > bit can ever be set - indicating a particular chip family. Why not > just have an enum instead? ACK. [...] >> +static const struct iio_chan_spec sunxi_gpadc_channels[] = { >> + SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"), >> + SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"), >> + SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"), >> + SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"), >> + { >> + .type = IIO_TEMP, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), >> + .datasheet_name = "temp_adc", >> + .extend_name = "SoC temperature", > Just curious, did you look at the resultling sysfs entries? > Going to be something like > in_temp_SoC\ Temperature_input... Not good. Just encountered this after further testing, not good as you said. > If there is a strong enough reason (and there may be) to add a 'help string' > type label to struct iio_chan_spec then my only real worries would be that > we would be adding a whole pile of ABI that would be in the, near impossible > to change in future, category... I don't understand your "adding a whole pile of ABI" thing. Same as datasheet_name variable: const char* in struct iio_chan_spec and used whenever needed with some NULL checks. This is the easiest way to do it. Or maybe you're thinking of adding an item to iio_chan_info_enum and all its underlying modifications? This could allow us to expose a _label sysfs file in the iio_device per type/channel. I understand the "near impossible to change in future" concern though. [...] >> + enable_irq(info->temp_data_irq); > Is this hardware spitting out extra irqs? If not, much better to just > leave it enabled all the time and control whether it can occur or not > by controlling the device state.. The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in the current state of the driver: 2s). What do you mean by controlling the device state? Enabling or disabling the hardware part of the IP responsible of getting the temperature (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)? Once the interrupt is activated, the IP periodically performs conversions. We don't really want interrupts to be activated when not needed. [...] >> + >> + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) >> + *val = info->temp_data * 133 - 257000; > Why report as processed? I'd just report them as raw with the scale > and offset provided. It's not a big thing, but if we can leave it so > that the conversion only occurs when desired, why not? > > For in kernel users, this all happen 'automagically' anyway ;) > ACK. [...] >> + mutex_unlock(&indio_dev->mlock); > mlock has a very specific purpose - locking the state changes of > between 'buffered' (push) and poll modes. Don't use it for anything else. > In fact it's almost always a bug to access it directly at all. We have > nice wrappers now for checking and locking the access mode. > Just have a local lock in your iio_priv structure. ACK. There is still a lot of drivers using iio_dev mlock though. [...] >> +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, SUNXI_GPADC_TP_DATA, &info->adc_data); >> + if (ret == 0) >> + complete(&info->completion); > if (!regmap_read(...))? > ACK. [...] >> + >> + info->temp_data_irq = irq; >> + disable_irq(irq); > As below, I want a comment explaining why you are disabling the irq here. > This is clearly racey.. Once the interrupt is activated, the IP performs continuous conversions (temp_data_irq only periodically). I want these interrupts to be enabled only when I read the sysfs file or we get useless interrupts. In the current state of this driver's irq handlers, I only set values in structures and all the needed structures are already initialized before requesting irqs. So it does not look like a race. I can prevent races in future versions by adding an atomic flag if wanted. >> + >> + irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING"); >> + if (irq < 0) { >> + dev_err(&pdev->dev, >> + "no FIFO_DATA_PENDING interrupt registered\n"); >> + ret = irq; >> + goto err; >> + } >> + >> + 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); >> + goto err; >> + } >> + >> + info->fifo_data_irq = irq; > There's a race here - this may be the only way of doing it though. > > However, I want a comment explaining why here... Are we looking at a hardware > bug? >> + disable_irq(irq); >> + >> + ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to register iio map array\n"); >> + goto err; >> + } >> + >> + 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); > Once you have an unwinding path (which is sensible here) put everything in > that block - it makes it easier to review (initially I compared that block > with the remove and thought you'd missed this error path). > ACK. As suggested by Maxime, I will put that in a different goto label. >> + goto err; >> + } >> + >> + return 0; >> + >> +err: >> + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); >> + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); >> + >> + return ret; >> +} >> + >> +static int sunxi_gpadc_remove(struct platform_device *pdev) >> +{ >> + struct sunxi_gpadc_dev *info; >> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >> + >> + info = iio_priv(indio_dev); >> + iio_device_unregister(indio_dev); >> + iio_map_array_unregister(indio_dev); > Slight disrepancy here between the cleanup in the error path in > probe vs what we have in remove (next line doesn't exist in probe cleanup) > Perhaps a comment here would make it clear why... >> + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0); SUNXI_GPADC_TP_INT_FIFO is the register in charge of activating hardware interrupts. I disable all interrupts. In the probe, the interrupts are already disabled (with disable_irq(irq)). This is actually just a shortcut compared to "disable_irq(info->fifo_data_irq); disable_irq(inf->temp_data_irq);". [...] -- 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 18/07/2016 14:57, Maxime Ripard wrote: > On Fri, Jul 15, 2016 at 11:59:12AM +0200, 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> >> --- >> >> v2: >> - add SUNXI_GPADC_ prefixes for defines, >> - correct typo in Kconfig, >> - reorder alphabetically includes, makefile, >> - add license header, >> - fix architecture variations not being handled in interrupt handlers or >> read raw functions, >> - fix unability to return negative values from thermal sensor, >> - add gotos to reduce code repetition, >> - fix irq variable being unsigned int instead of int, >> - remove useless dev_err and dev_info, >> - deactivate all interrupts if probe fails, >> - fix iio_device_register on NULL variable, >> - deactivate ADC in the IP when probe fails or when removing driver, >> >> drivers/iio/adc/Kconfig | 12 ++ >> drivers/iio/adc/Makefile | 1 + >> drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 430 insertions(+) >> create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index 25378c5..184856f 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -338,6 +338,18 @@ config NAU7802 >> To compile this driver as a module, choose M here: the >> module will be called nau7802. >> >> +config SUNXI_ADC > > We try to avoid the SUNXI prefix usually, otherwise this driver will > have a generic name (or at least is implicitly saying that it supports > all the sunxi SoCs), while it supports only a subset of those SoCs. > ACK. Will be replaced by SUN4I_GPADC. >> + tristate "ADC driver for sunxi platforms" > > And you should also mention which ADC is supported, since we usually > have several of them. > > Something like "Support for the Allwinner SoCs GPADC" > ACK. >> + depends on IIO >> + depends on MFD_SUNXI_ADC > > The order of your patches is quite weird. You depend on an option that > is not present yet? > ACK. Will modify the order of patches to reflect the real order. >> + help >> + Say yes here to build support for Allwinner (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 > > Your indentation is weird here, and the wrapping is likely to be wrong > too. > ACK. [...] >> @@ -0,0 +1,417 @@ >> +/* ADC driver for sunxi platforms >> + * >> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons> >> + * >> + * 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. > > Your wrapping is wrong. > ACK. >> + */ >> + >> +#include <linux/completion.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +#include <linux/iio/iio.h> >> +#include <linux/iio/driver.h> >> +#include <linux/iio/machine.h> >> +#include <linux/mfd/sunxi-gpadc-mfd.h> >> + >> +#define SUNXI_GPADC_TP_CTRL0 0x00 >> +#define SUNXI_GPADC_TP_CTRL1 0x04 >> +#define SUNXI_GPADC_TP_CTRL2 0x08 >> +#define SUNXI_GPADC_TP_CTRL3 0x0c >> +#define SUNXI_GPADC_TP_TPR 0x18 >> +#define SUNXI_GPADC_TP_CDAT 0x1c >> +#define SUNXI_GPADC_TEMP_DATA 0x20 >> +#define SUNXI_GPADC_TP_DATA 0x24 >> + >> +/* TP_CTRL0 bits */ >> +#define SUNXI_GPADC_ADC_FIRST_DLY(x) ((x) << 24) /* 8 bits */ >> +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE BIT(23) >> +#define SUNXI_GPADC_ADC_CLK_SELECT BIT(22) >> +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x) ((x) << 20) /* 2 bits */ >> +#define SUNXI_GPADC_FS_DIV(x) ((x) << 16) /* 4 bits */ >> +#define SUNXI_GPADC_T_ACQ(x) ((x) << 0) /* 16 bits */ > > We usually prefer to have the bits defined directly after the > registers, and prefixed with the name of the register they belong to. > > Something like SUNXI_GPADC_TP_CTRL_T_ACQ in this case > This modification induces the name of the bits to be really long: SUNXI_GPADC_TP_CTRL1_SUN6I_TOUCH_PAN_CALI_EN for example. ACK anyway. >> + >> +/* TP_CTRL1 bits */ >> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x) ((x) << 12) /* 8 bits */ >> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN BIT(9) >> +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN BIT(6) >> +#define SUNXI_GPADC_TP_DUAL_EN BIT(5) >> +#define SUNXI_GPADC_TP_MODE_EN BIT(4) >> +#define SUNXI_GPADC_TP_ADC_SELECT BIT(3) >> +#define SUNXI_GPADC_ADC_CHAN_SELECT(x) ((x) << 0) /* 3 bits */ > > Usually the comments are on the line above. However, if you really > want to enforce something, you should rather mask the > value. Otherwise, that comment is pretty useless. > Do you mean something like that: #define SUNXI_GPADC_ADC_CHAN_SELECT(x) (GENMASK(2,0) & x) ? >> + >> +/* TP_CTRL1 bits for sun6i SOCs */ >> +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN BIT(7) >> +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN BIT(6) >> +#define SUNXI_GPADC_SUN6I_TP_MODE_EN BIT(5) >> +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT BIT(4) > > Shouldn't that go in either a common define or the touchscreen driver? > Then shouldn't I put all defines in a common header? (sunxi-gpadc-mfd.h) [...] >> +/* TP_TPR bits */ >> +#define SUNXI_GPADC_TEMP_ENABLE(x) ((x) << 16) >> +/* t = x * 256 * 16 / clkin */ > > That comment would be better next to the code that does that > computation. > ACK. [...] >> + reinit_completion(&info->completion); >> + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) >> + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, >> + SUNXI_GPADC_SUN6I_TP_MODE_EN | >> + SUNXI_GPADC_SUN6I_TP_ADC_SELECT | >> + SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel)); >> + else >> + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, >> + SUNXI_GPADC_TP_MODE_EN | >> + SUNXI_GPADC_TP_ADC_SELECT | >> + SUNXI_GPADC_ADC_CHAN_SELECT(channel)); > > Using a function pointer that would compute this, or some fields in a > structure to store this would be better. > ACK. [...] >> + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) >> + *val = info->temp_data * 133 - 257000; >> + else if (info->flags & SUNXI_GPADC_ARCH_SUN5I) >> + *val = info->temp_data * 100 - 144700; >> + else if (info->flags & SUNXI_GPADC_ARCH_SUN6I) >> + *val = info->temp_data * 167 - 271000; > > Ditto, having functions to comptue this and just store the function > pointer would be better. > As Jonathan suggests, we should better go with separate read_raws (IIO_CHAN_RAW returns info->temp_data, INFO_CHAN_SCALE and INFO_CHAN_OFFSET return a different value depending on the architecture). So this would split the above code in separate functions as you wanted. [...] >> + irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING"); >> + if (irq < 0) { >> + dev_err(&pdev->dev, >> + "no TEMP_DATA_PENDING interrupt registered\n"); >> + ret = irq; >> + goto err; >> + } >> + >> + 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); >> + goto err; >> + } >> + >> + info->temp_data_irq = irq; >> + disable_irq(irq); >> + >> + irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING"); >> + if (irq < 0) { >> + dev_err(&pdev->dev, >> + "no FIFO_DATA_PENDING interrupt registered\n"); >> + ret = irq; >> + goto err; >> + } >> + >> + 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); >> + goto err; >> + } >> + >> + info->fifo_data_irq = irq; >> + disable_irq(irq); > > request_irq starts with irq enabled, which means that you can have an > interrupt showing up between your call to request_irq and the > disable_irq. > > How would that work? > Same as what I answered in Jonathan's mail: "Once the interrupt is activated, the IP performs continuous conversions (temp_data_irq only periodically). I want these interrupts to be enabled only when I read the sysfs file or we get useless interrupts. In the current state of this driver's irq handlers, I only set values in structures and all the needed structures are already initialized before requesting irqs. So it does not look like a race. I can prevent races in future versions by adding an atomic flag if wanted." >> + >> + ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to register iio map array\n"); >> + goto err; >> + } >> + >> + 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); > > That should go in a separate label. > ACK. >> + goto err; >> + } >> + >> + return 0; >> + >> +err: >> + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); >> + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); > > Why is that needed? > This disables ADC and Temperature on the hardware side of the IP. (mainly a shortcut to SUNXI_GPADC_TP_MODE_EN (or its architecture variant) and SUNXI_GPADC_TEMP_ENABLE set to 0. >> + return ret; >> +} >> + >> +static int sunxi_gpadc_remove(struct platform_device *pdev) >> +{ >> + struct sunxi_gpadc_dev *info; >> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >> + >> + info = iio_priv(indio_dev); >> + iio_device_unregister(indio_dev); >> + iio_map_array_unregister(indio_dev); >> + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0); >> + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); >> + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); >> + >> + return 0; >> +} >> + >> +static const struct platform_device_id sunxi_gpadc_id[] = { >> + { "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I }, >> + { "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I }, >> + { "sun6i-a31-gpadc-iio", SUNXI_GPADC_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, >> +}; > > Having some runtime_pm support for this would be great too. > Basically disabling the ADC and interrupts (as in the remove) in _suspend and _idle and reenabling everything in "before _suspend"-state in _resume I guess?
On Tue, Jul 19, 2016 at 11:04:23AM +0200, Quentin Schulz wrote: > >> +/* TP_CTRL1 bits */ > >> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x) ((x) << 12) /* 8 bits */ > >> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN BIT(9) > >> +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN BIT(6) > >> +#define SUNXI_GPADC_TP_DUAL_EN BIT(5) > >> +#define SUNXI_GPADC_TP_MODE_EN BIT(4) > >> +#define SUNXI_GPADC_TP_ADC_SELECT BIT(3) > >> +#define SUNXI_GPADC_ADC_CHAN_SELECT(x) ((x) << 0) /* 3 bits */ > > > > Usually the comments are on the line above. However, if you really > > want to enforce something, you should rather mask the > > value. Otherwise, that comment is pretty useless. > > > > Do you mean something like that: > #define SUNXI_GPADC_ADC_CHAN_SELECT(x) (GENMASK(2,0) & x) ? Yes. > >> +/* TP_CTRL1 bits for sun6i SOCs */ > >> +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN BIT(7) > >> +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN BIT(6) > >> +#define SUNXI_GPADC_SUN6I_TP_MODE_EN BIT(5) > >> +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT BIT(4) > > > > Shouldn't that go in either a common define or the touchscreen driver? > > > > Then shouldn't I put all defines in a common header? (sunxi-gpadc-mfd.h) You should :) > >> + 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); > >> + goto err; > >> + } > >> + > >> + info->fifo_data_irq = irq; > >> + disable_irq(irq); > > > > request_irq starts with irq enabled, which means that you can have an > > interrupt showing up between your call to request_irq and the > > disable_irq. > > > > How would that work? > > > > Same as what I answered in Jonathan's mail: > > "Once the interrupt is activated, the IP performs continuous conversions > (temp_data_irq only periodically). I want these interrupts to be enabled > only when I read the sysfs file or we get useless interrupts. > In the current state of this driver's irq handlers, I only set values in > structures and all the needed structures are already initialized before > requesting irqs. So it does not look like a race. I can prevent races in > future versions by adding an atomic flag if wanted." Then please have a nice comment explaining that. > >> +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, > >> +}; > > > > Having some runtime_pm support for this would be great too. > > > > Basically disabling the ADC and interrupts (as in the remove) in > _suspend and _idle and reenabling everything in "before _suspend"-state > in _resume I guess? Well, yes and no. Your probe would not do any hardware initialisation. You can pm_runtime_get_sync before using it, which will call the suspend hook. Once you're done, call pm_runtime_put, and that will disable the hardware. Maxime
On 18/07/2016 15:18, Jonathan Cameron wrote: [...] >> + >> + if (!wait_for_completion_timeout(&info->completion, >> + msecs_to_jiffies(100))) { >> + ret = -ETIMEDOUT; >> + goto out; >> + } >> + >> + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) >> + *val = info->temp_data * 133 - 257000; > Why report as processed? I'd just report them as raw with the scale > and offset provided. It's not a big thing, but if we can leave it so > that the conversion only occurs when desired, why not? > > For in kernel users, this all happen 'automagically' anyway ;) > Mmmmh, in the code above we apply the scale on the raw value and then the offset. While in iio_convert_raw_to_processed (http://lxr.free-electrons.com/source/drivers/iio/inkern.c#L507), the offset is applied before the scale. The way would be to factorize the computation by scale: Now: *val = raw * scale + offset Then: *val = (raw + offset/scale) * scale But the offset is an integer and offset/scale is therefore rounded. Currently, we have the following values: sun4i: -257000/133 = -1932.3308270676691 sun5i: -144700/100 = -1447 sun6i: -271000/167 = -1622.754491017964 Do we accept such rounding? If not, we either stay with the processed value in read_raw or patch inkern to add an offset to apply after having applied the scale to the raw value (val2 from iio_channel_read is yet unused with IIO_CHAN_INFO_OFFSET for example, we could use that to specify an offset2 to apply after the switch(scale_type)-case). [...] Quentin
On 20 July 2016 at 14:37, Quentin Schulz <quentin.schulz@free-electrons.com> wrote: > On 18/07/2016 15:18, Jonathan Cameron wrote: > [...] >>> + >>> + if (!wait_for_completion_timeout(&info->completion, >>> + msecs_to_jiffies(100))) { >>> + ret = -ETIMEDOUT; >>> + goto out; >>> + } >>> + >>> + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) >>> + *val = info->temp_data * 133 - 257000; >> Why report as processed? I'd just report them as raw with the scale >> and offset provided. It's not a big thing, but if we can leave it so >> that the conversion only occurs when desired, why not? >> >> For in kernel users, this all happen 'automagically' anyway ;) >> > > Mmmmh, in the code above we apply the scale on the raw value and then > the offset. While in iio_convert_raw_to_processed > (http://lxr.free-electrons.com/source/drivers/iio/inkern.c#L507), the > offset is applied before the scale. > > The way would be to factorize the computation by scale: > Now: *val = raw * scale + offset > Then: *val = (raw + offset/scale) * scale > > But the offset is an integer and offset/scale is therefore rounded. > Currently, we have the following values: > sun4i: -257000/133 = -1932.3308270676691 > sun5i: -144700/100 = -1447 > sun6i: -271000/167 = -1622.754491017964 > > Do we accept such rounding? How much does this rounding bring as mistake on end result and is this acceptable for you? You can always multiply it by 1000 or more to get the precision you want out. > > If not, we either stay with the processed value in read_raw or patch > inkern to add an offset to apply after having applied the scale to the > raw value (val2 from iio_channel_read is yet unused with > IIO_CHAN_INFO_OFFSET for example, we could use that to specify an > offset2 to apply after the switch(scale_type)-case). > > [...] > Quentin > -- 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 19/07/16 09:33, Quentin Schulz wrote: > On 18/07/2016 15:18, Jonathan Cameron wrote: >> On 15/07/16 10:59, 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, >> >> Various bits inline. In particular the irq handling looks flakey / racey >> to me. Definitely need some explanatory comments. >> >> Also another note on the craziness that using extended_name to provide >> the hwmon labels will cause :) >> >> Jonathan > [...] >>> +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; >> I'd prefer something more explicit than this. Right now only one >> bit can ever be set - indicating a particular chip family. Why not >> just have an enum instead? > > ACK. > > [...] >>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = { >>> + SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"), >>> + SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"), >>> + SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"), >>> + SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"), >>> + { >>> + .type = IIO_TEMP, >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), >>> + .datasheet_name = "temp_adc", >>> + .extend_name = "SoC temperature", >> Just curious, did you look at the resultling sysfs entries? >> Going to be something like >> in_temp_SoC\ Temperature_input... Not good. > > Just encountered this after further testing, not good as you said. > >> If there is a strong enough reason (and there may be) to add a 'help string' >> type label to struct iio_chan_spec then my only real worries would be that >> we would be adding a whole pile of ABI that would be in the, near impossible >> to change in future, category... > > I don't understand your "adding a whole pile of ABI" thing. Adding a 'free form' description field to the sysfs ABI of IIO is nasty in that such things tend not to be as reviewed as they should be and the moment an unclear / wrong statement is there, it's in the kernel ABI and chances are someone will go and write a script using it. Userspace breakage ensues so we get stuck with garbage. >Same as > datasheet_name variable: const char* in struct iio_chan_spec and used > whenever needed with some NULL checks. This is the easiest way to do it. > Or maybe you're thinking of adding an item to iio_chan_info_enum and all > its underlying modifications? This could allow us to expose a _label > sysfs file in the iio_device per type/channel. As you suggest - new field in iio_chan_spec. > > I understand the "near impossible to change in future" concern though. > > [...] >>> + enable_irq(info->temp_data_irq); >> Is this hardware spitting out extra irqs? If not, much better to just >> leave it enabled all the time and control whether it can occur or not >> by controlling the device state.. > > The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in > the current state of the driver: 2s). What do you mean by controlling > the device state? Enabling or disabling the hardware part of the IP > responsible of getting the temperature > (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)? Yes, or something along those lines if it wakes up fast enough. > Once the interrupt is activated, the IP periodically performs > conversions. We don't really want interrupts to be activated when not > needed. > > [...] >>> + >>> + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) >>> + *val = info->temp_data * 133 - 257000; >> Why report as processed? I'd just report them as raw with the scale >> and offset provided. It's not a big thing, but if we can leave it so >> that the conversion only occurs when desired, why not? >> >> For in kernel users, this all happen 'automagically' anyway ;) >> > > ACK. > > [...] >>> + mutex_unlock(&indio_dev->mlock); >> mlock has a very specific purpose - locking the state changes of >> between 'buffered' (push) and poll modes. Don't use it for anything else. >> In fact it's almost always a bug to access it directly at all. We have >> nice wrappers now for checking and locking the access mode. >> Just have a local lock in your iio_priv structure. > > ACK. There is still a lot of drivers using iio_dev mlock though. > > [...] >>> +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, SUNXI_GPADC_TP_DATA, &info->adc_data); >>> + if (ret == 0) >>> + complete(&info->completion); >> if (!regmap_read(...))? >> > > ACK. > > [...] >>> + >>> + info->temp_data_irq = irq; >>> + disable_irq(irq); >> As below, I want a comment explaining why you are disabling the irq here. >> This is clearly racey.. > > Once the interrupt is activated, the IP performs continuous conversions > (temp_data_irq only periodically). I want these interrupts to be enabled > only when I read the sysfs file or we get useless interrupts. > In the current state of this driver's irq handlers, I only set values in > structures and all the needed structures are already initialized before > requesting irqs. So it does not look like a race. I can prevent races in > future versions by adding an atomic flag if wanted. > >>> + >>> + irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING"); >>> + if (irq < 0) { >>> + dev_err(&pdev->dev, >>> + "no FIFO_DATA_PENDING interrupt registered\n"); >>> + ret = irq; >>> + goto err; >>> + } >>> + >>> + 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); >>> + goto err; >>> + } >>> + >>> + info->fifo_data_irq = irq; >> There's a race here - this may be the only way of doing it though. >> >> However, I want a comment explaining why here... Are we looking at a hardware >> bug? >>> + disable_irq(irq); >>> + >>> + ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps); >>> + if (ret < 0) { >>> + dev_err(&pdev->dev, "failed to register iio map array\n"); >>> + goto err; >>> + } >>> + >>> + 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); >> Once you have an unwinding path (which is sensible here) put everything in >> that block - it makes it easier to review (initially I compared that block >> with the remove and thought you'd missed this error path). >> > > ACK. As suggested by Maxime, I will put that in a different goto label. > >>> + goto err; >>> + } >>> + >>> + return 0; >>> + >>> +err: >>> + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); >>> + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); >>> + >>> + return ret; >>> +} >>> + >>> +static int sunxi_gpadc_remove(struct platform_device *pdev) >>> +{ >>> + struct sunxi_gpadc_dev *info; >>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev); >>> + >>> + info = iio_priv(indio_dev); >>> + iio_device_unregister(indio_dev); >>> + iio_map_array_unregister(indio_dev); >> Slight disrepancy here between the cleanup in the error path in >> probe vs what we have in remove (next line doesn't exist in probe cleanup) >> Perhaps a comment here would make it clear why... >>> + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0); > > SUNXI_GPADC_TP_INT_FIFO is the register in charge of activating hardware > interrupts. I disable all interrupts. In the probe, the interrupts are > already disabled (with disable_irq(irq)). This is actually just a > shortcut compared to "disable_irq(info->fifo_data_irq); > disable_irq(inf->temp_data_irq);". That disables reception of interrupts, I'd suggest it would be cleaner to ensure you also disable their generation if an error occurs. Basically it looks nicer that way ;) > > [...] > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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 20/07/16 13:37, Quentin Schulz wrote: > On 18/07/2016 15:18, Jonathan Cameron wrote: > [...] >>> + >>> + if (!wait_for_completion_timeout(&info->completion, >>> + msecs_to_jiffies(100))) { >>> + ret = -ETIMEDOUT; >>> + goto out; >>> + } >>> + >>> + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) >>> + *val = info->temp_data * 133 - 257000; >> Why report as processed? I'd just report them as raw with the scale >> and offset provided. It's not a big thing, but if we can leave it so >> that the conversion only occurs when desired, why not? >> >> For in kernel users, this all happen 'automagically' anyway ;) >> > > Mmmmh, in the code above we apply the scale on the raw value and then > the offset. While in iio_convert_raw_to_processed > (http://lxr.free-electrons.com/source/drivers/iio/inkern.c#L507), the > offset is applied before the scale. > > The way would be to factorize the computation by scale: > Now: *val = raw * scale + offset > Then: *val = (raw + offset/scale) * scale > > But the offset is an integer and offset/scale is therefore rounded. > Currently, we have the following values: > sun4i: -257000/133 = -1932.3308270676691 > sun5i: -144700/100 = -1447 > sun6i: -271000/167 = -1622.754491017964 > > Do we accept such rounding? Yes - how accurate do you think a temp sensor is - really not a problem. > > If not, we either stay with the processed value in read_raw or patch > inkern to add an offset to apply after having applied the scale to the > raw value (val2 from iio_channel_read is yet unused with > IIO_CHAN_INFO_OFFSET for example, we could use that to specify an > offset2 to apply after the switch(scale_type)-case). I don't really care that much if you just want to keep them as processed values. J > > [...] > Quentin > -- 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 20/07/2016 16:57, Jonathan Cameron wrote: > On 19/07/16 09:33, Quentin Schulz wrote: >> On 18/07/2016 15:18, Jonathan Cameron wrote: >>> On 15/07/16 10:59, Quentin Schulz wrote: [...] >>>> + enable_irq(info->temp_data_irq); >>> Is this hardware spitting out extra irqs? If not, much better to just >>> leave it enabled all the time and control whether it can occur or not >>> by controlling the device state.. >> >> The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in >> the current state of the driver: 2s). What do you mean by controlling >> the device state? Enabling or disabling the hardware part of the IP >> responsible of getting the temperature >> (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)? > Yes, or something along those lines if it wakes up fast enough. The ADC wakes up fast enough but resets its internal time clock (I don't know if it's the right term to use). Note that the temperature interrupt occurs by period of X seconds in this IP. This means that each time we disable the ADC on the hardware side, no temperature interrupt will occur within the first X seconds. I don't think this is what we want. [...] Quentin -- 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 21/07/16 14:15, Quentin Schulz wrote: > On 20/07/2016 16:57, Jonathan Cameron wrote: >> On 19/07/16 09:33, Quentin Schulz wrote: >>> On 18/07/2016 15:18, Jonathan Cameron wrote: >>>> On 15/07/16 10:59, Quentin Schulz wrote: > [...] >>>>> + enable_irq(info->temp_data_irq); >>>> Is this hardware spitting out extra irqs? If not, much better to just >>>> leave it enabled all the time and control whether it can occur or not >>>> by controlling the device state.. >>> >>> The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in >>> the current state of the driver: 2s). What do you mean by controlling >>> the device state? Enabling or disabling the hardware part of the IP >>> responsible of getting the temperature >>> (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)? >> Yes, or something along those lines if it wakes up fast enough. > > The ADC wakes up fast enough but resets its internal time clock (I don't > know if it's the right term to use). Note that the temperature interrupt > occurs by period of X seconds in this IP. > > This means that each time we disable the ADC on the hardware side, no > temperature interrupt will occur within the first X seconds. I don't > think this is what we want. I'm guessing X is non trivial ;) So fair enough. Could you add this justification as a comment in the driver somewhere so that people coming back to this in a few years time will know what the justification for this 'unusual' handling is. Thanks, Jonathan > > [...] > > Quentin > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 25378c5..184856f 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -338,6 +338,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 (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 38638d4..3e60a1d 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -37,6 +37,7 @@ 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 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.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..87cc913 --- /dev/null +++ b/drivers/iio/adc/sunxi-gpadc-iio.c @@ -0,0 +1,417 @@ +/* ADC driver for sunxi platforms + * + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons> + * + * 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. + */ + +#include <linux/completion.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include <linux/iio/iio.h> +#include <linux/iio/driver.h> +#include <linux/iio/machine.h> +#include <linux/mfd/sunxi-gpadc-mfd.h> + +#define SUNXI_GPADC_TP_CTRL0 0x00 +#define SUNXI_GPADC_TP_CTRL1 0x04 +#define SUNXI_GPADC_TP_CTRL2 0x08 +#define SUNXI_GPADC_TP_CTRL3 0x0c +#define SUNXI_GPADC_TP_TPR 0x18 +#define SUNXI_GPADC_TP_CDAT 0x1c +#define SUNXI_GPADC_TEMP_DATA 0x20 +#define SUNXI_GPADC_TP_DATA 0x24 + +/* TP_CTRL0 bits */ +#define SUNXI_GPADC_ADC_FIRST_DLY(x) ((x) << 24) /* 8 bits */ +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE BIT(23) +#define SUNXI_GPADC_ADC_CLK_SELECT BIT(22) +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x) ((x) << 20) /* 2 bits */ +#define SUNXI_GPADC_FS_DIV(x) ((x) << 16) /* 4 bits */ +#define SUNXI_GPADC_T_ACQ(x) ((x) << 0) /* 16 bits */ + +/* TP_CTRL1 bits */ +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x) ((x) << 12) /* 8 bits */ +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN BIT(9) +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN BIT(6) +#define SUNXI_GPADC_TP_DUAL_EN BIT(5) +#define SUNXI_GPADC_TP_MODE_EN BIT(4) +#define SUNXI_GPADC_TP_ADC_SELECT BIT(3) +#define SUNXI_GPADC_ADC_CHAN_SELECT(x) ((x) << 0) /* 3 bits */ + +/* TP_CTRL1 bits for sun6i SOCs */ +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN BIT(7) +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN BIT(6) +#define SUNXI_GPADC_SUN6I_TP_MODE_EN BIT(5) +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT BIT(4) +#define SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(x) BIT(x) /* 4 bits */ + +/* TP_CTRL2 bits */ +#define SUNXI_GPADC_TP_SENSITIVE_ADJUST(x) ((x) << 28) /* 4 bits */ +#define SUNXI_GPADC_TP_MODE_SELECT(x) ((x) << 26) /* 2 bits */ +#define SUNXI_GPADC_PRE_MEA_EN BIT(24) +#define SUNXI_GPADC_PRE_MEA_THRE_CNT(x) ((x) << 0) /* 24 bits */ + +/* TP_CTRL3 bits */ +#define SUNXI_GPADC_FILTER_EN BIT(2) +#define SUNXI_GPADC_FILTER_TYPE(x) ((x) << 0) /* 2 bits */ + +/* TP_INT_FIFOC irq and fifo mask / control bits */ +#define SUNXI_GPADC_TEMP_IRQ_EN BIT(18) +#define SUNXI_GPADC_TP_OVERRUN_IRQ_EN BIT(17) +#define SUNXI_GPADC_TP_DATA_IRQ_EN BIT(16) +#define SUNXI_GPADC_TP_DATA_XY_CHANGE BIT(13) +#define SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(x) ((x) << 8) /* 5 bits */ +#define SUNXI_GPADC_TP_DATA_DRQ_EN BIT(7) +#define SUNXI_GPADC_TP_FIFO_FLUSH BIT(4) +#define SUNXI_GPADC_TP_UP_IRQ_EN BIT(1) +#define SUNXI_GPADC_TP_DOWN_IRQ_EN BIT(0) + +/* TP_INT_FIFOS irq and fifo status bits */ +#define SUNXI_GPADC_TEMP_DATA_PENDING BIT(18) +#define SUNXI_GPADC_FIFO_OVERRUN_PENDING BIT(17) +#define SUNXI_GPADC_FIFO_DATA_PENDING BIT(16) +#define SUNXI_GPADC_TP_IDLE_FLG BIT(2) +#define SUNXI_GPADC_TP_UP_PENDING BIT(1) +#define SUNXI_GPADC_TP_DOWN_PENDING BIT(0) + +/* TP_TPR bits */ +#define SUNXI_GPADC_TEMP_ENABLE(x) ((x) << 16) +/* t = x * 256 * 16 / clkin */ +#define SUNXI_GPADC_TEMP_PERIOD(x) ((x) << 0) + +#define SUNXI_GPADC_ARCH_SUN4I BIT(0) +#define SUNXI_GPADC_ARCH_SUN5I BIT(1) +#define SUNXI_GPADC_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 SUNXI_GPADC_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", + }, + { /* sentinel */ }, +}; + +static const struct iio_chan_spec sunxi_gpadc_channels[] = { + SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"), + SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"), + SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"), + SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"), + { + .type = IIO_TEMP, + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), + .datasheet_name = "temp_adc", + .extend_name = "SoC temperature", + }, + { /* sentinel */ }, +}; + +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel, + int *val) +{ + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); + int ret = 0; + + mutex_lock(&indio_dev->mlock); + + reinit_completion(&info->completion); + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, + SUNXI_GPADC_SUN6I_TP_MODE_EN | + SUNXI_GPADC_SUN6I_TP_ADC_SELECT | + SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel)); + else + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, + SUNXI_GPADC_TP_MODE_EN | + SUNXI_GPADC_TP_ADC_SELECT | + SUNXI_GPADC_ADC_CHAN_SELECT(channel)); + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, + SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) | + SUNXI_GPADC_TP_FIFO_FLUSH); + enable_irq(info->fifo_data_irq); + + if (!wait_for_completion_timeout(&info->completion, + msecs_to_jiffies(100))) { + ret = -ETIMEDOUT; + goto out; + } + + *val = info->adc_data; + +out: + disable_irq(info->fifo_data_irq); + mutex_unlock(&indio_dev->mlock); + + return ret; +} + +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val) +{ + struct sunxi_gpadc_dev *info = iio_priv(indio_dev); + int ret = 0; + + mutex_lock(&indio_dev->mlock); + + reinit_completion(&info->completion); + + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, + SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) | + SUNXI_GPADC_TP_FIFO_FLUSH); + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, + SUNXI_GPADC_SUN6I_TP_MODE_EN); + else + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, + SUNXI_GPADC_TP_MODE_EN); + enable_irq(info->temp_data_irq); + + if (!wait_for_completion_timeout(&info->completion, + msecs_to_jiffies(100))) { + ret = -ETIMEDOUT; + goto out; + } + + if (info->flags & SUNXI_GPADC_ARCH_SUN4I) + *val = info->temp_data * 133 - 257000; + else if (info->flags & SUNXI_GPADC_ARCH_SUN5I) + *val = info->temp_data * 100 - 144700; + else if (info->flags & SUNXI_GPADC_ARCH_SUN6I) + *val = info->temp_data * 167 - 271000; + +out: + disable_irq(info->temp_data_irq); + mutex_unlock(&indio_dev->mlock); + + return ret; + +} + +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + int ret; + + switch (mask) { + case IIO_CHAN_INFO_PROCESSED: + ret = sunxi_gpadc_temp_read(indio_dev, val); + if (ret) + return ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_RAW: + ret = sunxi_gpadc_adc_read(indio_dev, chan->channel, val); + if (ret) + return ret; + + return IIO_VAL_INT; + default: + return -EINVAL; + } + + 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, SUNXI_GPADC_TEMP_DATA, + &info->temp_data); + if (ret == 0) + complete(&info->completion); + + return IRQ_HANDLED; +} + +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, SUNXI_GPADC_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; + struct iio_dev *indio_dev; + int ret, 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) + 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, SUNXI_GPADC_TP_CTRL0, SUNXI_GPADC_FS_DIV(7) | + SUNXI_GPADC_ADC_CLK_DIVIDER(2) | SUNXI_GPADC_T_ACQ(63)); + if (info->flags & SUNXI_GPADC_ARCH_SUN6I) + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, + SUNXI_GPADC_SUN6I_TP_MODE_EN); + else + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, + SUNXI_GPADC_TP_MODE_EN); + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3, SUNXI_GPADC_FILTER_EN | + SUNXI_GPADC_FILTER_TYPE(1)); + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, + SUNXI_GPADC_TEMP_ENABLE(1) | + SUNXI_GPADC_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"); + ret = irq; + goto err; + } + + 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); + goto err; + } + + info->temp_data_irq = irq; + disable_irq(irq); + + irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING"); + if (irq < 0) { + dev_err(&pdev->dev, + "no FIFO_DATA_PENDING interrupt registered\n"); + ret = irq; + goto err; + } + + 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); + goto err; + } + + info->fifo_data_irq = irq; + disable_irq(irq); + + ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps); + if (ret < 0) { + dev_err(&pdev->dev, "failed to register iio map array\n"); + goto err; + } + + 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); + goto err; + } + + return 0; + +err: + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); + + return ret; +} + +static int sunxi_gpadc_remove(struct platform_device *pdev) +{ + struct sunxi_gpadc_dev *info; + struct iio_dev *indio_dev = platform_get_drvdata(pdev); + + info = iio_priv(indio_dev); + iio_device_unregister(indio_dev); + iio_map_array_unregister(indio_dev); + regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0); + regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0); + regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0); + + return 0; +} + +static const struct platform_device_id sunxi_gpadc_id[] = { + { "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I }, + { "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I }, + { "sun6i-a31-gpadc-iio", SUNXI_GPADC_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");
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> --- v2: - add SUNXI_GPADC_ prefixes for defines, - correct typo in Kconfig, - reorder alphabetically includes, makefile, - add license header, - fix architecture variations not being handled in interrupt handlers or read raw functions, - fix unability to return negative values from thermal sensor, - add gotos to reduce code repetition, - fix irq variable being unsigned int instead of int, - remove useless dev_err and dev_info, - deactivate all interrupts if probe fails, - fix iio_device_register on NULL variable, - deactivate ADC in the IP when probe fails or when removing driver, drivers/iio/adc/Kconfig | 12 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+) create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c