Hi Peter, On 13 June 2018 at 17:17, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > Hi Peter, > > On 13 June 2018 at 16:53, Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> wrote: >> >>> From: Freeman Liu <freeman.liu@xxxxxxxxxxxxxx> >> >> some comments below >> >>> The Spreadtrum SC27XX PMICs ADC controller contains 32 channels, >>> which is used to sample voltages with 12 bits conversion. >>> >>> Signed-off-by: Freeman Liu <freeman.liu@xxxxxxxxxxxxxx> >>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx> >>> --- >>> drivers/iio/adc/Kconfig | 10 + >>> drivers/iio/adc/Makefile | 1 + >>> drivers/iio/adc/sc27xx_adc.c | 558 ++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 569 insertions(+) >>> create mode 100644 drivers/iio/adc/sc27xx_adc.c >>> >>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >>> index 9da7907..985b73e 100644 >>> --- a/drivers/iio/adc/Kconfig >>> +++ b/drivers/iio/adc/Kconfig >>> @@ -621,6 +621,16 @@ config ROCKCHIP_SARADC >>> To compile this driver as a module, choose M here: the >>> module will be called rockchip_saradc. >>> >>> +config SC27XX_ADC >>> + tristate "Spreadtrum SC27xx series PMICs ADC" >>> + depends on MFD_SC27XX_PMIC || COMPILE_TEST >>> + help >>> + Say yes here to build support for the integrated ADC inside the >>> + Spreadtrum SC27xx series PMICs. >>> + >>> + This driver can also be built as a module. If so, the module >>> + will be called sc27xx_adc. >>> + >>> config SPEAR_ADC >>> tristate "ST SPEAr ADC" >>> depends on PLAT_SPEAR || COMPILE_TEST >>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >>> index 28a9423..03db7b5 100644 >>> --- a/drivers/iio/adc/Makefile >>> +++ b/drivers/iio/adc/Makefile >>> @@ -59,6 +59,7 @@ obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o >>> obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o >>> obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o >>> obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o >>> +obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o >>> obj-$(CONFIG_SPEAR_ADC) += spear_adc.o >>> obj-$(CONFIG_STX104) += stx104.o >>> obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o >>> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c >>> new file mode 100644 >>> index 0000000..d74310a >>> --- /dev/null >>> +++ b/drivers/iio/adc/sc27xx_adc.c >>> @@ -0,0 +1,558 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +// Copyright (C) 2018 Spreadtrum Communications Inc. >>> + >>> +#include <linux/hwspinlock.h> >>> +#include <linux/iio/iio.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/module.h> >>> +#include <linux/of.h> >>> +#include <linux/of_device.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> + >>> +/* PMIC global registers definition */ >>> +#define SC27XX_MODULE_EN 0xc08 >>> +#define SC27XX_MODULE_ADC_EN BIT(5) >>> +#define SC27XX_ARM_CLK_EN 0xc10 >>> +#define SC27XX_CLK_ADC_EN BIT(5) >>> +#define SC27XX_CLK_ADC_CLK_EN BIT(6) >>> + >>> +/* ADC controller registers definition */ >>> +#define SC27XX_ADC_CTL 0x0 >>> +#define SC27XX_ADC_CH_CFG 0x4 >>> +#define SC27XX_ADC_DATA 0x4c >>> +#define SC27XX_ADC_INT_EN 0x50 >>> +#define SC27XX_ADC_INT_CLR 0x54 >>> +#define SC27XX_ADC_INT_STS 0x58 >>> +#define SC27XX_ADC_INT_RAW 0x5c >>> + >>> +/* Bits and mask definition for SC27XX_ADC_CTL register */ >>> +#define SC27XX_ADC_EN BIT(0) >>> +#define SC27XX_ADC_CHN_RUN BIT(1) >>> +#define SC27XX_ADC_12BIT_MODE BIT(2) >>> +#define SC27XX_ADC_RUN_NUM_MASK GENMASK(7, 4) >>> +#define SC27XX_ADC_RUN_NUM_SHIFT 4 >>> + >>> +/* Bits and mask definition for SC27XX_ADC_CH_CFG register */ >>> +#define SC27XX_ADC_CHN_ID_MASK GENMASK(4, 0) >>> +#define SC27XX_ADC_SCALE_MASK GENMASK(10, 8) >>> +#define SC27XX_ADC_SCALE_SHIFT 8 >>> + >>> +/* Bits definitions for SC27XX_ADC_INT_EN registers */ >>> +#define SC27XX_ADC_IRQ_EN BIT(0) >>> + >>> +/* Bits definitions for SC27XX_ADC_INT_CLR registers */ >>> +#define SC27XX_ADC_IRQ_CLR BIT(0) >>> + >>> +/* Mask definition for SC27XX_ADC_DATA register */ >>> +#define SC27XX_ADC_DATA_MASK GENMASK(11, 0) >>> + >>> +/* Timeout (ms) for the trylock of hardware spinlocks */ >>> +#define SC27XX_ADC_HWLOCK_TIMEOUT 5000 >>> + >>> +/* Maximum ADC channel number */ >>> +#define SC27XX_ADC_CHANNEL_MAX 32 >>> + >>> +/* ADC voltage ratio definition */ >>> +#define SC27XX_VOLT_RATIO(n, d) \ >>> + (((n) << SC27XX_RATIO_NUMERATOR_OFFSET) | (d)) >>> +#define SC27XX_RATIO_NUMERATOR_OFFSET 16 >>> +#define SC27XX_RATIO_DENOMINATOR_MASK GENMASK(15, 0) >>> + >>> +/* Covert ADC values to voltage values */ >> >> Convert > > Sorry for typo and will fix in next version. > >> >>> +#define SC27XX_ADC_TO_VOLTAGE(volt0, volt1, adc0, adc1, value) \ >> >> I'd rather define a function than a macro for this > > Sure. > >> >>> + ({ \ >>> + int tmp; \ >>> + tmp = (volt0) - (volt1); \ >>> + tmp = tmp * ((value) - (adc1)); \ >>> + tmp = tmp / ((adc0) - (adc1)); \ >>> + tmp = tmp + (volt1); \ >>> + if (tmp < 0) \ >>> + tmp = 0; \ >>> + \ >>> + tmp; \ >>> + }) >>> + >>> +struct sc27xx_adc_data { >>> + struct device *dev; >>> + struct regmap *regmap; >>> + /* >>> + * One hardware spinlock to synchronize between the multiple >>> + * subsystems which will access the unique ADC controller. >>> + */ >>> + struct hwspinlock *hwlock; >>> + struct completion completion; >>> + int channel_scale[SC27XX_ADC_CHANNEL_MAX]; >>> + int (*get_volt_ratio)(u32 channel, int scale); >>> + u32 base; >>> + int value; >>> + int irq; >>> +}; >>> + >>> +struct sc27xx_adc_linear_graph { >>> + int volt0; >>> + int adc0; >>> + int volt1; >>> + int adc1; >>> +}; >>> + >>> +/* >>> + * According to the datasheet, we can convert one ADC value to one voltage value >>> + * through 2 points in the linear graph. If the voltage is less than 1.2v, we >>> + * should use the small-scale graph, and if more than 1.2v, we should use the >>> + * big-scale graph. >>> + */ >>> +static struct sc27xx_adc_linear_graph big_scale_graph = { >> >> const > > Sure > >> >>> + 4200, 3310, >>> + 3600, 2832, >>> +}; >>> + >>> +static struct sc27xx_adc_linear_graph small_scale_graph = { >> >> const > > Sure. > >> >>> + 1000, 3413, >>> + 100, 341, >>> +}; >>> + >>> +static int sc27xx_adc_2731_ratio(u32 channel, int scale) >> >> can scale be bool? >> or unsigned? > > It can not be a bool, it can be set more than 1 for other PMIC ADC. > >> >> why use u32 for channel? maybe u8 or just unsigned? > > OK, u8 is enough. After more check, the channel type was passed from IIO core, so I think I should keep it as 'int' type and I do not want to truncate it to 'u8'. >> >>> +{ >>> + switch (channel) { >>> + case 1: >>> + case 2: >>> + case 3: >>> + case 4: >>> + return scale ? SC27XX_VOLT_RATIO(400, 1025) : >>> + SC27XX_VOLT_RATIO(1, 1); >>> + case 5: >>> + return SC27XX_VOLT_RATIO(7, 29); >>> + case 6: >>> + return SC27XX_VOLT_RATIO(375, 9000); >>> + case 7: >>> + case 8: >>> + return scale ? SC27XX_VOLT_RATIO(100, 125) : >>> + SC27XX_VOLT_RATIO(1, 1); >>> + case 19: >>> + return SC27XX_VOLT_RATIO(1, 3); >>> + default: >>> + return SC27XX_VOLT_RATIO(1, 1); >>> + } >>> + return SC27XX_VOLT_RATIO(1, 1); >>> +} >>> + >>> +static int sc27xx_adc_read(struct sc27xx_adc_data *data, u32 channel, >>> + int scale, int *val) >>> +{ >>> + int ret; >>> + u32 tmp; >>> + >>> + reinit_completion(&data->completion); >>> + >>> + ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT); >>> + if (ret) { >>> + dev_err(data->dev, "timeout to get the hwspinlock\n"); >>> + return ret; >>> + } >>> + >>> + ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL, >>> + SC27XX_ADC_EN, SC27XX_ADC_EN); >>> + if (ret) >>> + goto unlock_adc; >>> + >>> + /* Configure the channel id and scale */ >>> + tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK; >>> + tmp |= channel & SC27XX_ADC_CHN_ID_MASK; >>> + ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG, >>> + SC27XX_ADC_CHN_ID_MASK | SC27XX_ADC_SCALE_MASK, >>> + tmp); >>> + if (ret) >>> + goto disable_adc; >>> + >>> + /* Select 12bit conversion mode, and only sample 1 time */ >>> + tmp = SC27XX_ADC_12BIT_MODE; >>> + tmp |= (0 << SC27XX_ADC_RUN_NUM_SHIFT) & SC27XX_ADC_RUN_NUM_MASK; >> >> this is still 0 Sorry I missed this comment last time, since other system (synchronize with hwlock) may set this configuration of the unique ADC controller, we should re-set this. -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html