On 04/10/15 17:04, H. Nikolaus Schaller wrote: > > 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 again! > > Worked into V2 (coming right after this mail). > Comments below, where/why we have not exactly followed your feedback. Responses inline, but we haven't disagreed on anything important so none of it really matters! Jonathan > > BR, > 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. > Is mostly defined in the Palmas data sheet but I have added some comments. >>> + 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. > > I don't know what all of them are doing, so I have only added the next two and some more. > >> >>> + u8 ch0_current; >>> + u8 ch3_current; > > If I understand them correctly, they just store 2 bits each written into the > current source registers. The value is only calculated in the probe function > and internal to the driver. The bit pattern required is defined by the data sheet. > > All TWL4030/TWL603x have such current sources. In the DT, just uA are specified > and this is to temporarily store the bit pattern until it is sent to the chip. So it is quite > a deep driver internal, but should be obvious to everyone who has the data sheet. > >>> + 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. > > Since I am not the original author and don't know how to present that to user space, > I would leave this open for future improvements. Fair enough. > >> >>> + 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? > > It appears that this function is only called for calibrated values. > > The condition appears to detect that the ADC has reported a value smaller than > the calibration allows. > > This indicates to be some error condition (maybe a floating ADC input?). But > I don't know enough of the Palmas details to judge this. So I have only moved > it behind the (optional) calibration calculation and detect if either would report > negative values. So it essentially clamps voltage reports at 0V. > >>> + >> Umm. what is is_correct_code? Should be documented somewhere > > have renamed it to is_uncalibrated which means that there is no register > on the chip to read out calibration data for that channel. > >>> + 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). > > IMHO would duplicate code (not sure if gcc can detect). Therefore I have left it as is. > >> >> 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? > > "Units after application of scale and offset are millivolts." > > The driver reports millivolts on OMAP5 uEVM, e.g. VBUS = 5013 (mV). cool > >> 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? > > There are two channels that are or can be temperature values. Presented as > voltages across an NTC/Diode. Since the NTC is outside the Palmas chip the > conversion into temperature is left to user-space. The type value is in preparation > for this and I have changed it to IIO_TEMPERATURE. > >>> + .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). > > If I understand the driver correctty (I am not the original author), the calibration > is indeed linear, but translating the calibration data into *_RAW, *_OFFSET and *_SCALE > is different from how Texas suggests to do the calculation (maybe avoiding rounding > and truncation errors). > > Well, we could simply remove the _RAW values - but IMHO it is sometimes good > to be able to see what is going on. They can still be ignored by the consumer. hmm.. I'm not totally happy with having both, but will let it go I think. > >> >>> + .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. > > Well, enum constants allow to check against typos and that it matches the > channel numbers defined in the header. > > So PALMAS_ADC_CHAN_IIO(IN24, ...) would make gcc warn while. > PALMAS_ADC_CHAN_IIO(24, ...) would fail in operation. > > Of course we have no typo, because it is already checked to compile :) > > So I understand by which good C coding practise it was probably introduced > and am undecided if it is good to keep or not. hmm. not writing silly bugs seems like a better idea to me, or convoluted tricky to check code. I'm not that fussed however. > >>> +} >>> + >>> +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. > > I think the best location to make that clear is the DT bindings where you > can set the values (in microamperes and not encoded values). > > Basically you specify in the adc_pdata the uA you want and this function > does a quantization to perpare 2 bits that can be written into the control > registers later on. > >>> + 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!) > > Same for me and I have no idea how to test. But it looks reasonable to call > device_wakeup_disable(). Therefore I have added. > >>> + >>> + 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. > > has also been proposed by Peter Meerwald and has been reworked. > >> >> 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. > > There is indeed a proper macro in palmas.h > >> >>> + } >>> + >>> + 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. > > We forgot to delete... > >> >>> + >>> +#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... > > As said it adds some (questionable) check by the compiler, > >>> + PALMAS_ADC_CH_MAX, > > and this constant is indirectly defined. > >>> +}; >>> + >>> +/* 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
