| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMz4kuJVghre3jFHHNZqZZoLrpZFYVmfxEEQoPf6eA5YDtknZg@mail.gmail.com>
Date: Thu, 14 Jun 2018 10:43:43 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Peter Meerwald-Stadler <pmeerw@...erw.net>
Cc: Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
freeman.liu@...eadtrum.com, linux-iio@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] iio: adc: Add Spreadtrum SC27XX PMICs ADC support
Hi Peter,
On 13 June 2018 at 17:17, Baolin Wang <baolin.wang@...aro.org> wrote:
> Hi Peter,
>
> On 13 June 2018 at 16:53, Peter Meerwald-Stadler <pmeerw@...erw.net> wrote:
>>
>>> From: Freeman Liu <freeman.liu@...eadtrum.com>
>>
>> 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@...eadtrum.com>
>>> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
>>> ---
>>> 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
Powered by blists - more mailing lists