Hi, Am 27.09.2015 um 17:21 schrieb Jonathan Cameron <jic23@xxxxxxxxxx>: > 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. thanks for the valuable comments! Will work into V2. Nikolaus > > 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
