On 23/09/15 13:48, H. Nikolaus Schaller wrote: > This driver code was found as: > > https://android.googlesource.com/kernel/tegra/+/aaabb2e045f31e5a970109ffdaae900dd403d17e/drivers/staging/iio/adc > > Fixed various compilation issues and test this driver on omap5 evm. > > Signed-off-by: Pradeep Goudagunta <pgoudagunta@xxxxxxxxxx> > Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> > Signed-off-by: Marek Belisko <marek@xxxxxxxxxxxxx> Various bits inline. Jonathan > --- > drivers/iio/adc/Kconfig | 9 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/palmas_gpadc.c | 797 +++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/palmas.h | 59 ++- > 4 files changed, 862 insertions(+), 4 deletions(-) > create mode 100644 drivers/iio/adc/palmas_gpadc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index eb0cd89..f6df9db 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -242,6 +242,15 @@ config NAU7802 > To compile this driver as a module, choose M here: the > module will be called nau7802. > > +config PALMAS_GPADC > + tristate "TI Palmas General Purpose ADC" > + depends on MFD_PALMAS > + help > + Palmas series pmic chip by texas Instruments (twl6035/6037) > + is used in smartphones and tablets and supports a 16 channel > + general purpose ADC. Add iio driver to read different channel > + of the GPADCs. > + > config QCOM_SPMI_IADC > tristate "Qualcomm SPMI PMIC current ADC" > depends on SPMI > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index a096210..716f112 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o > obj-$(CONFIG_MCP3422) += mcp3422.o > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o > obj-$(CONFIG_NAU7802) += nau7802.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 > obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > new file mode 100644 > index 0000000..17abb28 > --- /dev/null > +++ b/drivers/iio/adc/palmas_gpadc.c > @@ -0,0 +1,797 @@ > +/* > + * palmas-adc.c -- TI PALMAS GPADC. > + * > + * Copyright (c) 2013, NVIDIA Corporation. All rights reserved. > + * > + * Author: Pradeep Goudagunta <pgoudagunta@xxxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + */ > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/irq.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/pm.h> > +#include <linux/mfd/palmas.h> > +#include <linux/completion.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/machine.h> > +#include <linux/iio/driver.h> > + > +#define MOD_NAME "palmas-gpadc" > +#define ADC_CONVERSION_TIMEOUT (msecs_to_jiffies(5000)) > +#define TO_BE_CALCULATED 0 > + > +struct palmas_gpadc_info { > +/* calibration codes and regs */ Full docs on this would be appreciated. > + int x1; > + int x2; > + int v1; > + int v2; > + u8 trim1_reg; > + u8 trim2_reg; > + int gain; > + int offset; > + int gain_error; > + bool is_correct_code; > +}; > + > +#define PALMAS_ADC_INFO(_chan, _x1, _x2, _v1, _v2, _t1, _t2, _is_correct_code)\ > +[PALMAS_ADC_CH_##_chan] = { \ > + .x1 = _x1, \ > + .x2 = _x2, \ > + .v1 = _v1, \ > + .v2 = _v2, \ > + .gain = TO_BE_CALCULATED, \ > + .offset = TO_BE_CALCULATED, \ > + .gain_error = TO_BE_CALCULATED, \ > + .trim1_reg = PALMAS_GPADC_TRIM##_t1, \ > + .trim2_reg = PALMAS_GPADC_TRIM##_t2, \ > + .is_correct_code = _is_correct_code \ > + } > + > +static struct palmas_gpadc_info palmas_gpadc_info[] = { > + PALMAS_ADC_INFO(IN0, 2064, 3112, 630, 950, 1, 2, false), > + PALMAS_ADC_INFO(IN1, 2064, 3112, 630, 950, 1, 2, false), > + PALMAS_ADC_INFO(IN2, 2064, 3112, 1260, 1900, 3, 4, false), > + PALMAS_ADC_INFO(IN3, 2064, 3112, 630, 950, 1, 2, false), > + PALMAS_ADC_INFO(IN4, 2064, 3112, 630, 950, 1, 2, false), > + PALMAS_ADC_INFO(IN5, 2064, 3112, 630, 950, 1, 2, false), > + PALMAS_ADC_INFO(IN6, 2064, 3112, 2520, 3800, 5, 6, false), > + PALMAS_ADC_INFO(IN7, 2064, 3112, 2520, 3800, 7, 8, false), > + PALMAS_ADC_INFO(IN8, 2064, 3112, 3150, 4750, 9, 10, false), > + PALMAS_ADC_INFO(IN9, 2064, 3112, 5670, 8550, 11, 12, false), > + PALMAS_ADC_INFO(IN10, 2064, 3112, 3465, 5225, 13, 14, false), > + PALMAS_ADC_INFO(IN11, 0, 0, 0, 0, INVALID, INVALID, true), > + PALMAS_ADC_INFO(IN12, 0, 0, 0, 0, INVALID, INVALID, true), > + PALMAS_ADC_INFO(IN13, 0, 0, 0, 0, INVALID, INVALID, true), > + PALMAS_ADC_INFO(IN14, 2064, 3112, 3645, 5225, 15, 16, false), > + PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true), > +}; > + > +struct palmas_gpadc { > + struct device *dev; > + struct palmas *palmas; As there are some non obvious elements in here (such as the next two) kernel-doc for the whole stucture would be good. > + u8 ch0_current; > + u8 ch3_current; > + bool extended_delay; > + int irq; > + int irq_auto_0; > + int irq_auto_1; > + struct palmas_gpadc_info *adc_info; > + struct completion conv_completion; > + struct palmas_adc_wakeup_property wakeup1_data; > + struct palmas_adc_wakeup_property wakeup2_data; > + bool wakeup1_enable; > + bool wakeup2_enable; > + int auto_conversion_period; > +}; > + > +/* > + * GPADC lock issue in AUTO mode. > + * Impact: In AUTO mode, GPADC conversion can be locked after disabling AUTO > + * mode feature. > + * Details: > + * When the AUTO mode is the only conversion mode enabled, if the AUTO > + * mode feature is disabled with bit GPADC_AUTO_CTRL. AUTO_CONV1_EN = 0 > + * or bit GPADC_AUTO_CTRL. AUTO_CONV0_EN = 0 during a conversion, the > + * conversion mechanism can be seen as locked meaning that all following > + * conversion will give 0 as a result. Bit GPADC_STATUS.GPADC_AVAILABLE > + * will stay at 0 meaning that GPADC is busy. An RT conversion can unlock > + * the GPADC. > + * > + * Workaround(s): > + * To avoid the lock mechanism, the workaround to follow before any stop > + * conversion request is: > + * Force the GPADC state machine to be ON by using the GPADC_CTRL1. > + * GPADC_FORCE bit = 1 > + * Shutdown the GPADC AUTO conversion using > + * GPADC_AUTO_CTRL.SHUTDOWN_CONV[01] = 0. > + * After 100us, force the GPADC state machine to be OFF by using the > + * GPADC_CTRL1. GPADC_FORCE bit = 0 > + */ > +static int palmas_disable_auto_conversion(struct palmas_gpadc *adc) > +{ > + int ret; > + > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_CTRL1, > + PALMAS_GPADC_CTRL1_GPADC_FORCE, > + PALMAS_GPADC_CTRL1_GPADC_FORCE); > + if (ret < 0) { > + dev_err(adc->dev, "GPADC_CTRL1 update failed: %d\n", ret); > + return ret; > + } > + > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_AUTO_CTRL, > + PALMAS_GPADC_AUTO_CTRL_SHUTDOWN_CONV1 | > + PALMAS_GPADC_AUTO_CTRL_SHUTDOWN_CONV0, > + 0); > + if (ret < 0) { > + dev_err(adc->dev, "AUTO_CTRL update failed: %d\n", ret); > + return ret; > + } > + > + udelay(100); > + > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_CTRL1, > + PALMAS_GPADC_CTRL1_GPADC_FORCE, 0); > + if (ret < 0) { > + dev_err(adc->dev, "GPADC_CTRL1 update failed: %d\n", ret); > + return ret; > + } return ret and drop the return above. Coccicheck will moan at you about this. > + return 0; > +} > + > +static irqreturn_t palmas_gpadc_irq(int irq, void *data) > +{ > + struct palmas_gpadc *adc = data; > + > + complete(&adc->conv_completion); blank line. > + return IRQ_HANDLED; > +} > + > +static irqreturn_t palmas_gpadc_irq_auto(int irq, void *data) > +{ > + struct palmas_gpadc *adc = data; > + > + dev_info(adc->dev, "Threshold interrupt %d occurs\n", irq); Could indicate this to userspace... other than through the logs. > + palmas_disable_auto_conversion(adc); blank line generally before all function returns.. > + return IRQ_HANDLED; > +} > + > +static int palmas_gpadc_start_mask_interrupt(struct palmas_gpadc *adc, int mask) mask is boolean. Make it explicitly so for readability. > +{ > + int ret; > + > + if (!mask) > + ret = palmas_update_bits(adc->palmas, PALMAS_INTERRUPT_BASE, > + PALMAS_INT3_MASK, > + PALMAS_INT3_MASK_GPADC_EOC_SW, 0); > + else > + ret = palmas_update_bits(adc->palmas, PALMAS_INTERRUPT_BASE, > + PALMAS_INT3_MASK, > + PALMAS_INT3_MASK_GPADC_EOC_SW, > + PALMAS_INT3_MASK_GPADC_EOC_SW); > + if (ret < 0) > + dev_err(adc->dev, "GPADC INT MASK update failed: %d\n", ret); > + > + return ret; > +} > + > +static int palmas_gpadc_enable(struct palmas_gpadc *adc, int adc_chan, > + int enable) > +{ > + unsigned int mask, val; > + int ret; > + > + if (enable) { > + val = (adc->extended_delay > + << PALMAS_GPADC_RT_CTRL_EXTEND_DELAY_SHIFT); > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_RT_CTRL, > + PALMAS_GPADC_RT_CTRL_EXTEND_DELAY, val); > + if (ret < 0) { > + dev_err(adc->dev, "RT_CTRL update failed: %d\n", ret); > + return ret; > + } > + > + mask = (PALMAS_GPADC_CTRL1_CURRENT_SRC_CH0_MASK | > + PALMAS_GPADC_CTRL1_CURRENT_SRC_CH3_MASK | > + PALMAS_GPADC_CTRL1_GPADC_FORCE); > + val = (adc->ch0_current > + << PALMAS_GPADC_CTRL1_CURRENT_SRC_CH0_SHIFT); > + val |= (adc->ch3_current > + << PALMAS_GPADC_CTRL1_CURRENT_SRC_CH3_SHIFT); > + val |= PALMAS_GPADC_CTRL1_GPADC_FORCE; > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_CTRL1, mask, val); > + if (ret < 0) { > + dev_err(adc->dev, > + "Failed to update current setting: %d\n", ret); > + return ret; > + } > + > + mask = (PALMAS_GPADC_SW_SELECT_SW_CONV0_SEL_MASK | > + PALMAS_GPADC_SW_SELECT_SW_CONV_EN); > + val = (adc_chan | PALMAS_GPADC_SW_SELECT_SW_CONV_EN); > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_SW_SELECT, mask, val); > + if (ret < 0) { > + dev_err(adc->dev, "SW_SELECT update failed: %d\n", ret); > + return ret; > + } > + } else { > + ret = palmas_write(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_SW_SELECT, 0); > + if (ret < 0) > + dev_err(adc->dev, "SW_SELECT write failed: %d\n", ret); > + > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_CTRL1, > + PALMAS_GPADC_CTRL1_GPADC_FORCE, 0); > + if (ret < 0) { > + dev_err(adc->dev, "CTRL1 update failed: %d\n", ret); > + return ret; > + } > + } > + > + return ret; > +} > + > +static int palmas_gpadc_read_prepare(struct palmas_gpadc *adc, int adc_chan) > +{ > + int ret; > + > + ret = palmas_gpadc_enable(adc, adc_chan, true); > + if (ret < 0) > + return ret; > + > + return palmas_gpadc_start_mask_interrupt(adc, 0); > +} > + > +static void palmas_gpadc_read_done(struct palmas_gpadc *adc, int adc_chan) > +{ > + palmas_gpadc_start_mask_interrupt(adc, 1); > + palmas_gpadc_enable(adc, adc_chan, false); > +} > + > +static int palmas_gpadc_calibrate(struct palmas_gpadc *adc, int adc_chan) > +{ > + int k; > + int d1; > + int d2; > + int ret; > + int gain; > + int x1 = adc->adc_info[adc_chan].x1; > + int x2 = adc->adc_info[adc_chan].x2; > + int v1 = adc->adc_info[adc_chan].v1; > + int v2 = adc->adc_info[adc_chan].v2; > + > + ret = palmas_read(adc->palmas, PALMAS_TRIM_GPADC_BASE, > + adc->adc_info[adc_chan].trim1_reg, &d1); > + if (ret < 0) { > + dev_err(adc->dev, "TRIM read failed: %d\n", ret); > + goto scrub; > + } > + > + ret = palmas_read(adc->palmas, PALMAS_TRIM_GPADC_BASE, > + adc->adc_info[adc_chan].trim2_reg, &d2); > + if (ret < 0) { > + dev_err(adc->dev, "TRIM read failed: %d\n", ret); > + goto scrub; > + } > + > + /*Gain error Calculation*/ > + k = (1000 + (1000 * (d2 - d1)) / (x2 - x1)); > + > + /*Gain Calculation*/ > + gain = ((v2 - v1) * 1000) / (x2 - x1); > + > + adc->adc_info[adc_chan].gain_error = k; > + adc->adc_info[adc_chan].gain = gain; > + /*offset Calculation*/ > + adc->adc_info[adc_chan].offset = (d1 * 1000) - ((k - 1000) * x1); > + > +scrub: > + return ret; > +} > + > +static int palmas_gpadc_start_conversion(struct palmas_gpadc *adc, int adc_chan) > +{ > + unsigned int val; > + int ret; > + > + init_completion(&adc->conv_completion); > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_SW_SELECT, > + PALMAS_GPADC_SW_SELECT_SW_START_CONV0, > + PALMAS_GPADC_SW_SELECT_SW_START_CONV0); > + if (ret < 0) { > + dev_err(adc->dev, "ADC_SW_START write failed: %d\n", ret); > + return ret; > + } > + > + ret = wait_for_completion_timeout(&adc->conv_completion, > + ADC_CONVERSION_TIMEOUT); > + if (ret == 0) { > + dev_err(adc->dev, "ADC conversion not completed\n"); > + ret = -ETIMEDOUT; > + return ret; return -ETIMEDOUT; Might be worth you setting up to do sparse, smatch and coccicheck runs on the code as they'll pick up on a lot of issues like this. > + } > + > + ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_SW_CONV0_LSB, &val, 2); > + if (ret < 0) { > + dev_err(adc->dev, "ADCDATA read failed: %d\n", ret); > + return ret; > + } > + > + ret = (val & 0xFFF); nitpick : blank line here. > + return ret; > +} > + > +static int palmas_gpadc_get_calibrated_code(struct palmas_gpadc *adc, > + int adc_chan, int val) > +{ > + if (((val*1000) - adc->adc_info[adc_chan].offset) < 0) { > + dev_err(adc->dev, "No Input Connected\n"); > + return 0; > + } Interesting, but should this not be caught for raw channel reads as well? > + Umm. what is is_correct_code? Should be documented somewhere > + if (!(adc->adc_info[adc_chan].is_correct_code)) > + val = ((val*1000) - adc->adc_info[adc_chan].offset) / > + adc->adc_info[adc_chan].gain_error; > + > + val = (val * adc->adc_info[adc_chan].gain) / 1000; > + return val; > +} > + > +static int palmas_gpadc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, int *val2, long mask) > +{ > + struct palmas_gpadc *adc = iio_priv(indio_dev); > + int adc_chan = chan->channel; > + int ret = 0; > + > + if (adc_chan > PALMAS_ADC_CH_MAX) >= given I think your channels are 0 indexed? > + return -EINVAL; > + > + mutex_lock(&indio_dev->mlock); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + case IIO_CHAN_INFO_PROCESSED: I'd be tempted to split the two code paths here as that will be slightly easier to read (if longer). I'm also highly suspicious of any driver that does processed output and has a return type of IIO_VAL_INT. Are you processed values in the units documented in Documentation/ABI/testing/sysfs-bus-iio? They could be, but I would like a comment saying that. > + ret = palmas_gpadc_read_prepare(adc, adc_chan); > + if (ret < 0) > + goto out; > + > + ret = palmas_gpadc_start_conversion(adc, adc_chan); > + if (ret < 0) { > + dev_err(adc->dev, > + "ADC start coversion failed\n"); > + goto out; > + } > + > + if (mask == IIO_CHAN_INFO_PROCESSED) > + ret = palmas_gpadc_get_calibrated_code( > + adc, adc_chan, ret); > + > + *val = ret; > + > + ret = IIO_VAL_INT; > + goto out; > + } > + > + mutex_unlock(&indio_dev->mlock); > + return ret; > + > +out: > + palmas_gpadc_read_done(adc, adc_chan); > + mutex_unlock(&indio_dev->mlock); > + return ret; > +} > + > +static const struct iio_info palmas_gpadc_iio_info = { > + .read_raw = palmas_gpadc_read_raw, > + .driver_module = THIS_MODULE, > +}; > + > +#define PALMAS_ADC_CHAN_IIO(chan, _type) \ > +{ \ > + .datasheet_name = PALMAS_DATASHEET_NAME(chan), \ > + .type = _type, \ Right now they are all voltage channels, why have that specifiable in this macro? > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_PROCESSED), \ Hmm. This is very very rarely justified. Either the driver has nice linear relationship between the raw value and the processed one, in which case you should be providing *_RAW, *_OFFSET and *_SCALE and leaving the maths to userspace (or the in kernel wrappers), or they are non linear in which case only the processed value is of interest and the raw one not directly so. (the only exception to this is light sensors where the processed channel is often a computed channel from several input _raw readings). > + .indexed = 1, \ > + .channel = PALMAS_ADC_CH_##chan, \ Given this maps back to the value of chan, just put that in as a number and loose the enum. Channel is used in the naming so it doesn't make sense to obscure that behind an enum anyway. > +} > + > +static const struct iio_chan_spec palmas_gpadc_iio_channel[] = { > + PALMAS_ADC_CHAN_IIO(IN0, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN1, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN2, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN3, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN4, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN5, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN6, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN7, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN8, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN9, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN10, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN11, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN12, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN13, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN14, IIO_VOLTAGE), > + PALMAS_ADC_CHAN_IIO(IN15, IIO_VOLTAGE), > +}; > + > +static int palmas_gpadc_probe(struct platform_device *pdev) > +{ > + struct palmas_gpadc *adc; > + struct palmas_platform_data *pdata; > + struct palmas_gpadc_platform_data *adc_pdata; > + struct iio_dev *iodev; > + int ret, i; > + > + pdata = dev_get_platdata(pdev->dev.parent); > + if (!pdata || !pdata->gpadc_pdata) { > + dev_err(&pdev->dev, "No platform data\n"); > + return -ENODEV; > + } > + adc_pdata = pdata->gpadc_pdata; > + > + iodev = iio_device_alloc(sizeof(*adc)); > + if (!iodev) { > + dev_err(&pdev->dev, "iio_device_alloc failed\n"); > + return -ENOMEM; > + } > + > + if (adc_pdata->iio_maps) { > + ret = iio_map_array_register(iodev, adc_pdata->iio_maps); > + if (ret < 0) { > + dev_err(&pdev->dev, "iio_map_array_register failed\n"); > + goto out; > + } > + } > + > + adc = iio_priv(iodev); > + adc->dev = &pdev->dev; > + adc->palmas = dev_get_drvdata(pdev->dev.parent); > + adc->adc_info = palmas_gpadc_info; > + init_completion(&adc->conv_completion); > + dev_set_drvdata(&pdev->dev, iodev); > + > + adc->auto_conversion_period = adc_pdata->auto_conversion_period_ms; > + adc->irq = palmas_irq_get_virq(adc->palmas, PALMAS_GPADC_EOC_SW_IRQ); > + ret = request_threaded_irq(adc->irq, NULL, > + palmas_gpadc_irq, > + IRQF_ONESHOT | IRQF_EARLY_RESUME, dev_name(adc->dev), > + adc); > + if (ret < 0) { > + dev_err(adc->dev, > + "request irq %d failed: %dn", adc->irq, ret); > + goto out_unregister_map; > + } > + > + if (adc_pdata->adc_wakeup1_data) { > + memcpy(&adc->wakeup1_data, adc_pdata->adc_wakeup1_data, > + sizeof(adc->wakeup1_data)); > + adc->wakeup1_enable = true; > + adc->irq_auto_0 = platform_get_irq(pdev, 1); > + ret = request_threaded_irq(adc->irq_auto_0, NULL, > + palmas_gpadc_irq_auto, > + IRQF_ONESHOT | IRQF_EARLY_RESUME, > + "palmas-adc-auto-0", adc); > + if (ret < 0) { > + dev_err(adc->dev, "request auto0 irq %d failed: %dn", > + adc->irq_auto_0, ret); > + goto out_irq_free; > + } > + } > + > + if (adc_pdata->adc_wakeup2_data) { > + memcpy(&adc->wakeup2_data, adc_pdata->adc_wakeup2_data, > + sizeof(adc->wakeup2_data)); > + adc->wakeup2_enable = true; > + adc->irq_auto_1 = platform_get_irq(pdev, 2); > + ret = request_threaded_irq(adc->irq_auto_1, NULL, > + palmas_gpadc_irq_auto, > + IRQF_ONESHOT | IRQF_EARLY_RESUME, > + "palmas-adc-auto-1", adc); > + if (ret < 0) { > + dev_err(adc->dev, "request auto1 irq %d failed: %dn", > + adc->irq_auto_1, ret); > + goto out_irq_auto0_free; > + } > + } > + I've requested kernel-doc above for ch0_current, but if that doesn't make it clear what matic is going on here then some comments here would be good. > + if (adc_pdata->ch0_current == 0) > + adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0; > + else if (adc_pdata->ch0_current <= 5) > + adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_5; > + else if (adc_pdata->ch0_current <= 15) > + adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_15; > + else > + adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_20; > + > + if (adc_pdata->ch3_current == 0) > + adc->ch3_current = PALMAS_ADC_CH3_CURRENT_SRC_0; > + else if (adc_pdata->ch3_current <= 10) > + adc->ch3_current = PALMAS_ADC_CH3_CURRENT_SRC_10; > + else if (adc_pdata->ch3_current <= 400) > + adc->ch3_current = PALMAS_ADC_CH3_CURRENT_SRC_400; > + else > + adc->ch3_current = PALMAS_ADC_CH3_CURRENT_SRC_800; > + > + adc->extended_delay = adc_pdata->extended_delay; > + > + iodev->name = MOD_NAME; > + iodev->dev.parent = &pdev->dev; > + iodev->info = &palmas_gpadc_iio_info; > + iodev->modes = INDIO_DIRECT_MODE; > + iodev->channels = palmas_gpadc_iio_channel; > + iodev->num_channels = ARRAY_SIZE(palmas_gpadc_iio_channel); > + > + ret = iio_device_register(iodev); > + if (ret < 0) { > + dev_err(adc->dev, "iio_device_register() failed: %d\n", ret); > + goto out_irq_auto1_free; > + } > + > + device_set_wakeup_capable(&pdev->dev, 1); > + for (i = 0; i < PALMAS_ADC_CH_MAX; i++) { > + if (!(adc->adc_info[i].is_correct_code)) > + palmas_gpadc_calibrate(adc, i); > + } > + > + if (adc->wakeup1_enable || adc->wakeup2_enable) > + device_wakeup_enable(&pdev->dev); There is no match for this in the remove... Should there be one? (not an interface I know anything about!) > + > + return 0; > + > +out_irq_auto1_free: > + if (adc_pdata->adc_wakeup2_data) > + free_irq(adc->irq_auto_1, adc); > +out_irq_auto0_free: > + if (adc_pdata->adc_wakeup1_data) > + free_irq(adc->irq_auto_0, adc); > +out_irq_free: > + free_irq(adc->irq, adc); > +out_unregister_map: > + if (adc_pdata->iio_maps) > + iio_map_array_unregister(iodev); > +out: > + iio_device_free(iodev); > + return ret; > +} > + > +static int palmas_gpadc_remove(struct platform_device *pdev) > +{ > + struct iio_dev *iodev = dev_to_iio_dev(&pdev->dev); > + struct palmas_gpadc *adc = iio_priv(iodev); > + struct palmas_platform_data *pdata = dev_get_platdata(pdev->dev.parent); > + > + if (pdata->gpadc_pdata->iio_maps) > + iio_map_array_unregister(iodev); > + iio_device_unregister(iodev); > + free_irq(adc->irq, adc); > + if (adc->wakeup1_enable) > + free_irq(adc->irq_auto_0, adc); > + if (adc->wakeup2_enable) > + free_irq(adc->irq_auto_1, adc); > + iio_device_free(iodev); Could use demv_iio_device_alloc and not need the free here or on error path. Given location of irq frees you could do devm allocations of them as well which would cut out a fair bit of code without reordering anything. nit: blank line here. > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc) > +{ > + int adc_period, conv; > + int i; > + int ch0 = 0, ch1 = 0; > + int thres; > + int ret; > + > + adc_period = adc->auto_conversion_period; > + for (i = 0; i < 16; ++i) { spacing around the / > + if (((1000 * (1 << i))/32) < adc_period) > + continue; > + } > + if (i > 0) > + i--; > + adc_period = i; > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_AUTO_CTRL, > + PALMAS_GPADC_AUTO_CTRL_COUNTER_CONV_MASK, > + adc_period); > + if (ret < 0) { > + dev_err(adc->dev, "AUTO_CTRL write failed: %d\n", ret); > + return ret; > + } > + > + conv = 0; > + if (adc->wakeup1_enable) { > + int is_high; > + > + ch0 = adc->wakeup1_data.adc_channel_number; > + conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV0_EN; > + if (adc->wakeup1_data.adc_high_threshold > 0) { > + thres = adc->wakeup1_data.adc_high_threshold; > + is_high = 0; > + } else { > + thres = adc->wakeup1_data.adc_low_threshold; > + is_high = BIT(7); BIT(7) is a bit random, so I'd suggest defining an appropriate macro for it this register field. > + } > + > + ret = palmas_write(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_THRES_CONV0_LSB, thres & 0xFF); > + if (ret < 0) { > + dev_err(adc->dev, > + "THRES_CONV0_LSB write failed: %d\n", ret); > + return ret; > + } > + > + ret = palmas_write(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_THRES_CONV0_MSB, > + ((thres >> 8) & 0xF) | is_high); > + if (ret < 0) { > + dev_err(adc->dev, > + "THRES_CONV0_MSB write failed: %d\n", ret); > + return ret; > + } > + } > + > + if (adc->wakeup2_enable) { > + int is_high; > + > + ch1 = adc->wakeup2_data.adc_channel_number; > + conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV1_EN; > + if (adc->wakeup2_data.adc_high_threshold > 0) { > + thres = adc->wakeup2_data.adc_high_threshold; > + is_high = 0; > + } else { > + thres = adc->wakeup2_data.adc_low_threshold; > + is_high = BIT(7); > + } > + > + ret = palmas_write(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_THRES_CONV1_LSB, thres & 0xFF); > + if (ret < 0) { > + dev_err(adc->dev, > + "THRES_CONV1_LSB write failed: %d\n", ret); > + return ret; > + } > + > + ret = palmas_write(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_THRES_CONV1_MSB, > + ((thres >> 8) & 0xF) | is_high); > + if (ret < 0) { > + dev_err(adc->dev, > + "THRES_CONV1_MSB write failed: %d\n", ret); > + return ret; > + } > + } > + > + ret = palmas_write(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_AUTO_SELECT, (ch1 << 4) | ch0); > + if (ret < 0) { > + dev_err(adc->dev, "AUTO_SELECT write failed: %d\n", ret); > + return ret; > + } > + > + ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_AUTO_CTRL, > + PALMAS_GPADC_AUTO_CTRL_AUTO_CONV1_EN | > + PALMAS_GPADC_AUTO_CTRL_AUTO_CONV0_EN, conv); > + if (ret < 0) { > + dev_err(adc->dev, "AUTO_CTRL write failed: %d\n", ret); > + return ret; > + } nitpick. Blank line here please. > + return 0; > +} > + > +static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc) > +{ > + int ret; > + > + ret = palmas_write(adc->palmas, PALMAS_GPADC_BASE, > + PALMAS_GPADC_AUTO_SELECT, 0); > + if (ret < 0) { > + dev_err(adc->dev, "AUTO_SELECT write failed: %d\n", ret); > + return ret; > + } > + > + ret = palmas_disable_auto_conversion(adc); > + if (ret < 0) { > + dev_err(adc->dev, "Disable auto conversion failed: %d\n", ret); > + return ret; > + } > + return 0; > +} > + > +static int palmas_gpadc_suspend(struct device *dev) > +{ > + struct iio_dev *iodev = dev_to_iio_dev(dev); > + struct palmas_gpadc *adc = iio_priv(iodev); > + int wakeup = adc->wakeup1_enable || adc->wakeup2_enable; > + int ret; > + > + if (!device_may_wakeup(dev) || !wakeup) > + return 0; > + > + ret = palmas_adc_wakeup_configure(adc); > + if (ret < 0) > + return ret; > + > + if (adc->wakeup1_enable) > + enable_irq_wake(adc->irq_auto_0); > + > + if (adc->wakeup2_enable) > + enable_irq_wake(adc->irq_auto_1); nitpick. Blank line here please. > + return 0; > +} > + > +static int palmas_gpadc_resume(struct device *dev) > +{ > + struct iio_dev *iodev = dev_to_iio_dev(dev); > + struct palmas_gpadc *adc = iio_priv(iodev); > + int wakeup = adc->wakeup1_enable || adc->wakeup2_enable; > + int ret; > + > + if (!device_may_wakeup(dev) || !wakeup) > + return 0; > + > + ret = palmas_adc_wakeup_reset(adc); > + if (ret < 0) > + return ret; > + > + if (adc->wakeup1_enable) > + disable_irq_wake(adc->irq_auto_0); > + > + if (adc->wakeup2_enable) > + disable_irq_wake(adc->irq_auto_1); > + > + return 0; > +}; > +#endif > + > +static const struct dev_pm_ops palmas_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(palmas_gpadc_suspend, > + palmas_gpadc_resume) > +}; > + > +static struct platform_driver palmas_gpadc_driver = { > + .probe = palmas_gpadc_probe, > + .remove = palmas_gpadc_remove, > + .driver = { > + .name = MOD_NAME, > + .owner = THIS_MODULE, > + .pm = &palmas_pm_ops, > + }, > +}; > + > +static int __init palmas_gpadc_init(void) > +{ > + return platform_driver_register(&palmas_gpadc_driver); > +} > +module_init(palmas_gpadc_init); > + > +static void __exit palmas_gpadc_exit(void) > +{ > + platform_driver_unregister(&palmas_gpadc_driver); > +} > +module_exit(palmas_gpadc_exit); > + > +MODULE_DESCRIPTION("palmas GPADC driver"); > +MODULE_AUTHOR("Pradeep Goudagunta<pgoudagunta@xxxxxxxxxx>"); > +MODULE_ALIAS("platform:palmas-gpadc"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h > index bb270bd..60acfa2 100644 > --- a/include/linux/mfd/palmas.h > +++ b/include/linux/mfd/palmas.h > @@ -133,21 +133,32 @@ struct palmas_pmic_driver_data { > struct regulator_config config); > }; > > +struct palmas_adc_wakeup_property { > + int adc_channel_number; > + int adc_high_threshold; > + int adc_low_threshold; > +}; > + > struct palmas_gpadc_platform_data { > /* Channel 3 current source is only enabled during conversion */ > int ch3_current; > - > /* Channel 0 current source can be used for battery detection. > - * If used for battery detection this will cause a permanent current > + * If used for battery detection this will cause a permanent current > * consumption depending on current level set here. > - */ > + */ Some oddness occured here in patch creation. Do check your own patches as we all end up with stuff like this from time to time that should be picked up on beofre sending out. It just adds noise. > int ch0_current; > + bool extended_delay; > > /* default BAT_REMOVAL_DAT setting on device probe */ > int bat_removal; > > /* Sets the START_POLARITY bit in the RT_CTRL register */ > int start_polarity; > + > + struct iio_map *iio_maps; > + int auto_conversion_period_ms; > + struct palmas_adc_wakeup_property *adc_wakeup1_data; > + struct palmas_adc_wakeup_property *adc_wakeup2_data; > }; > > struct palmas_reg_init { > @@ -403,7 +414,7 @@ struct palmas_gpadc_calibration { > s32 gain_error; > s32 offset_error; > }; > - > +/* > struct palmas_gpadc { > struct device *dev; > struct palmas *palmas; > @@ -426,6 +437,9 @@ struct palmas_gpadc { > int conv1_channel; > int rt_channel; > }; > +*/ So there's a commented out bit of code in this patch? Sort that out before the next version. > + > +#define PALMAS_DATASHEET_NAME(_name) "palmas-gpadc-chan-"#_name > > struct palmas_gpadc_result { > s32 raw_code; > @@ -519,6 +533,42 @@ enum palmas_irqs { > PALMAS_NUM_IRQ, > }; > > +/* Palma GPADC Channels */ > +enum { > + PALMAS_ADC_CH_IN0, > + PALMAS_ADC_CH_IN1, > + PALMAS_ADC_CH_IN2, > + PALMAS_ADC_CH_IN3, > + PALMAS_ADC_CH_IN4, > + PALMAS_ADC_CH_IN5, > + PALMAS_ADC_CH_IN6, > + PALMAS_ADC_CH_IN7, > + PALMAS_ADC_CH_IN8, > + PALMAS_ADC_CH_IN9, > + PALMAS_ADC_CH_IN10, > + PALMAS_ADC_CH_IN11, > + PALMAS_ADC_CH_IN12, > + PALMAS_ADC_CH_IN13, > + PALMAS_ADC_CH_IN14, > + PALMAS_ADC_CH_IN15, This does rather feel like an enum that doesn't add anything as the enum values = the index given at the end anyway... > + PALMAS_ADC_CH_MAX, > +}; > + > +/* Palma GPADC Channel0 Current Source */ > +enum { > + PALMAS_ADC_CH0_CURRENT_SRC_0, > + PALMAS_ADC_CH0_CURRENT_SRC_5, > + PALMAS_ADC_CH0_CURRENT_SRC_15, > + PALMAS_ADC_CH0_CURRENT_SRC_20, > +}; Nitpick: new line here. > +/* Palma GPADC Channel3 Current Source */ > +enum { > + PALMAS_ADC_CH3_CURRENT_SRC_0, > + PALMAS_ADC_CH3_CURRENT_SRC_10, > + PALMAS_ADC_CH3_CURRENT_SRC_400, > + PALMAS_ADC_CH3_CURRENT_SRC_800, > +}; > + > struct palmas_pmic { > struct palmas *palmas; > struct device *dev; > @@ -2999,6 +3049,7 @@ enum usb_irq_events { > #define PALMAS_GPADC_TRIM14 0x0D > #define PALMAS_GPADC_TRIM15 0x0E > #define PALMAS_GPADC_TRIM16 0x0F > +#define PALMAS_GPADC_TRIMINVALID -1 > > /* TPS659038 regen2_ctrl offset iss different from palmas */ > #define TPS659038_REGEN2_CTRL 0x12 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
