Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.21.1806131034001.18339@vps.pmeerw.net>
References: <9b6743bb6782041b7fec9ed0e166faf2b6456de4.1528871668.git.baolin.wang@linaro.org>
 <25f44d68e5bb003a5d89545dd6493175af41d107.1528871668.git.baolin.wang@linaro.org>
 <alpine.DEB.2.21.1806131034001.18339@vps.pmeerw.net>
From:   Baolin Wang <baolin.wang@linaro.org>
Date:   Wed, 13 Jun 2018 17:17:58 +0800
Message-ID: <CAMz4ku+bkt_OB_rchYmOkmCx6XcNZkONSgfdifOi4hz9Xyv+ig@mail.gmail.com>
Subject: Re: [PATCH 2/2] iio: adc: Add Spreadtrum SC27XX PMICs ADC support
To:     Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        freeman.liu@spreadtrum.com, linux-iio@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Peter,

On 13 June 2018 at 16:53, Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
>
>> From: Freeman Liu <freeman.liu@spreadtrum.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@spreadtrum.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.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.

>
>> +{
>> +     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
>
>> +     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
>> +                              SC27XX_ADC_RUN_NUM_MASK | SC27XX_ADC_12BIT_MODE,
>> +                              tmp);
>> +     if (ret)
>> +             goto disable_adc;
>> +
>> +     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
>> +                              SC27XX_ADC_CHN_RUN, SC27XX_ADC_CHN_RUN);
>> +     if (ret)
>> +             goto disable_adc;
>> +
>> +     wait_for_completion(&data->completion);
>> +
>> +disable_adc:
>> +     regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
>> +                        SC27XX_ADC_EN, 0);
>> +unlock_adc:
>> +     hwspin_unlock_raw(data->hwlock);
>> +
>> +     if (!ret)
>> +             *val = data->value;
>> +
>> +     return ret;
>> +}
>> +
>> +static irqreturn_t sc27xx_adc_isr(int irq, void *dev_id)
>> +{
>> +     struct sc27xx_adc_data *data = dev_id;
>> +     int ret;
>> +
>> +     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
>> +                              SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
>> +     if (ret)
>> +             return IRQ_RETVAL(ret);
>> +
>> +     ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA,
>> +                       &data->value);
>> +     if (ret)
>> +             return IRQ_RETVAL(ret);
>> +
>> +     data->value &= SC27XX_ADC_DATA_MASK;
>> +     complete(&data->completion);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
>> +                               u32 channel, int scale,
>> +                               u32 *div_numerator, u32 *div_denominator)
>> +{
>> +     u32 ratio = data->get_volt_ratio(channel, scale);
>> +
>> +     *div_numerator = ratio >> SC27XX_RATIO_NUMERATOR_OFFSET;
>> +     *div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK;
>> +}
>> +
>> +static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data, u32 channel,
>> +                                int scale, int raw_adc)
>> +{
>> +     u32 numerator, denominator;
>> +     u32 volt;
>> +
>> +     /*
>> +      * Convert ADC values to voltage values according to the linear graph,
>> +      * and channel 5 and channel 1 has been calibrated, so we can just
>> +      * return the voltage values calculated by the linear graph. But other
>> +      * channels need be calculated to the real voltage values with the
>> +      * voltage ratio.
>> +      */
>> +     if (channel == 5) {
>> +             return SC27XX_ADC_TO_VOLTAGE(big_scale_graph.volt0,
>> +                                          big_scale_graph.volt1,
>> +                                          big_scale_graph.adc0,
>> +                                          big_scale_graph.adc1,
>> +                                          raw_adc);
>> +     } else if (channel == 1) {
>> +             return SC27XX_ADC_TO_VOLTAGE(small_scale_graph.volt0,
>> +                                          small_scale_graph.volt1,
>> +                                          small_scale_graph.adc0,
>> +                                          small_scale_graph.adc1,
>> +                                          raw_adc);
>> +     } else {
>> +             volt = SC27XX_ADC_TO_VOLTAGE(small_scale_graph.volt0,
>> +                                          small_scale_graph.volt1,
>> +                                          small_scale_graph.adc0,
>> +                                          small_scale_graph.adc1,
>> +                                          raw_adc);
>> +     }
>> +
>> +     sc27xx_adc_volt_ratio(data, channel, scale, &numerator, &denominator);
>> +
>> +     return (volt * denominator + numerator / 2) / numerator;
>> +}
>> +
>> +static int sc27xx_adc_read_processed(struct sc27xx_adc_data *data,
>> +                                  u32 channel, int scale, int *val)
>> +{
>> +     int ret, raw_adc;
>> +
>> +     ret = sc27xx_adc_read(data, channel, scale, &raw_adc);
>> +     if (ret)
>> +             return ret;
>> +
>> +     *val = sc27xx_adc_convert_volt(data, channel, scale, raw_adc);
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_adc_read_raw(struct iio_dev *indio_dev,
>> +                            struct iio_chan_spec const *chan,
>> +                            int *val, int *val2, long mask)
>> +{
>> +     struct sc27xx_adc_data *data = iio_priv(indio_dev);
>> +     int scale = data->channel_scale[chan->channel];
>
> channel_scale is accesses outside the mutex lock (see adc_write_raw()
> where it is locked)

Ah, will fix this in next version.

>
>> +     int ret, tmp;
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_RAW:
>> +     case IIO_CHAN_INFO_AVERAGE_RAW:
>> +             mutex_lock(&indio_dev->mlock);
>> +             ret = sc27xx_adc_read(data, chan->channel, scale, &tmp);
>
> move mutex_unlock() to here?
> and similar for the other cases

Sure.

>
>> +             if (ret) {
>> +                     mutex_unlock(&indio_dev->mlock);
>> +                     return ret;
>> +             }
>> +
>> +             *val = tmp;
>> +             mutex_unlock(&indio_dev->mlock);
>> +             return IIO_VAL_INT;
>> +
>> +     case IIO_CHAN_INFO_PROCESSED:
>> +             mutex_lock(&indio_dev->mlock);
>> +             ret = sc27xx_adc_read_processed(data, chan->channel, scale,
>> +                                             &tmp);
>> +             if (ret) {
>> +                     mutex_unlock(&indio_dev->mlock);
>> +                     return ret;
>> +             }
>> +
>> +             *val = tmp;
>> +             mutex_unlock(&indio_dev->mlock);
>> +             return IIO_VAL_INT;
>> +
>> +     case IIO_CHAN_INFO_SCALE:
>> +             mutex_lock(&indio_dev->mlock);
>> +             *val = scale;
>> +             mutex_unlock(&indio_dev->mlock);
>> +             return IIO_VAL_INT;
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
>> +
>> +static int sc27xx_adc_write_raw(struct iio_dev *indio_dev,
>> +                             struct iio_chan_spec const *chan,
>> +                             int val, int val2, long mask)
>> +{
>> +     struct sc27xx_adc_data *data = iio_priv(indio_dev);
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_SCALE:
>> +             mutex_lock(&indio_dev->mlock);
>> +             data->channel_scale[chan->channel] = val;
>
> this is strange, arbitraty numbers can be written here?
>
> what happens with val2? must be 0?

Yes, the channel consumer can set the channel scale value, which is
used to set the scale into ADC registers when running the channel.

>
>> +             mutex_unlock(&indio_dev->mlock);
>> +             return IIO_VAL_INT;
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
>> +
>> +static const struct iio_info sc27xx_info = {
>> +     .read_raw = &sc27xx_adc_read_raw,
>> +     .write_raw = &sc27xx_adc_write_raw,
>> +};
>> +
>> +#define SC27XX_ADC_CHANNEL(index) {                          \
>> +     .type = IIO_VOLTAGE,                                    \
>> +     .channel = index,                                       \
>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
>> +                           BIT(IIO_CHAN_INFO_AVERAGE_RAW) |  \
>> +                           BIT(IIO_CHAN_INFO_PROCESSED) |    \
>> +                           BIT(IIO_CHAN_INFO_SCALE),         \
>> +     .datasheet_name = "CH##index",                          \
>> +     .indexed = 1,                                           \
>> +}
>> +
>> +static const struct iio_chan_spec sc27xx_channels[] = {
>> +     SC27XX_ADC_CHANNEL(0),
>> +     SC27XX_ADC_CHANNEL(1),
>> +     SC27XX_ADC_CHANNEL(2),
>> +     SC27XX_ADC_CHANNEL(3),
>> +     SC27XX_ADC_CHANNEL(4),
>> +     SC27XX_ADC_CHANNEL(5),
>> +     SC27XX_ADC_CHANNEL(6),
>> +     SC27XX_ADC_CHANNEL(7),
>> +     SC27XX_ADC_CHANNEL(8),
>> +     SC27XX_ADC_CHANNEL(9),
>> +     SC27XX_ADC_CHANNEL(10),
>> +     SC27XX_ADC_CHANNEL(11),
>> +     SC27XX_ADC_CHANNEL(12),
>> +     SC27XX_ADC_CHANNEL(13),
>> +     SC27XX_ADC_CHANNEL(14),
>> +     SC27XX_ADC_CHANNEL(15),
>> +     SC27XX_ADC_CHANNEL(16),
>> +     SC27XX_ADC_CHANNEL(17),
>> +     SC27XX_ADC_CHANNEL(18),
>> +     SC27XX_ADC_CHANNEL(19),
>> +     SC27XX_ADC_CHANNEL(20),
>> +     SC27XX_ADC_CHANNEL(21),
>> +     SC27XX_ADC_CHANNEL(22),
>> +     SC27XX_ADC_CHANNEL(23),
>> +     SC27XX_ADC_CHANNEL(24),
>> +     SC27XX_ADC_CHANNEL(25),
>> +     SC27XX_ADC_CHANNEL(26),
>> +     SC27XX_ADC_CHANNEL(27),
>> +     SC27XX_ADC_CHANNEL(28),
>> +     SC27XX_ADC_CHANNEL(29),
>> +     SC27XX_ADC_CHANNEL(30),
>> +     SC27XX_ADC_CHANNEL(31),
>> +};
>> +
>> +static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
>> +{
>> +     int ret;
>> +
>> +     ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
>> +                              SC27XX_MODULE_ADC_EN, SC27XX_MODULE_ADC_EN);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Enable ADC work clock and controller clock */
>> +     ret = regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
>> +                              SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN,
>> +                              SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
>> +                              SC27XX_ADC_IRQ_EN, SC27XX_ADC_IRQ_EN);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
>> +                               SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
>> +}
>> +
>> +static void sc27xx_adc_disable(struct sc27xx_adc_data *data)
>> +{
>> +     regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_EN,
>> +                        SC27XX_ADC_IRQ_EN, 0);
>> +
>> +     /* Disable ADC work clock and controller clock */
>> +     regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
>> +                        SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
>> +
>> +     regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
>> +                        SC27XX_MODULE_ADC_EN, 0);
>> +}
>> +
>> +static int sc27xx_adc_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct sc27xx_adc_data *sc27xx_data;
>> +     struct iio_dev *indio_dev;
>> +     const void *data;
>> +     int ret;
>> +
>> +     data = of_device_get_match_data(&pdev->dev);
>> +     if (!data) {
>> +             dev_err(&pdev->dev, "failed to get match data\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sc27xx_data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     sc27xx_data = iio_priv(indio_dev);
>> +
>> +     sc27xx_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +     if (!sc27xx_data->regmap) {
>> +             dev_err(&pdev->dev, "failed to get ADC regmap\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     ret = of_property_read_u32(np, "reg", &sc27xx_data->base);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "failed to get ADC base address\n");
>> +             return ret;
>> +     }
>> +
>> +     sc27xx_data->irq = platform_get_irq(pdev, 0);
>> +     if (sc27xx_data->irq < 0) {
>> +             dev_err(&pdev->dev, "failed to get ADC irq number\n");
>> +             return sc27xx_data->irq;
>> +     }
>> +
>> +     ret = of_hwspin_lock_get_id(np, 0);
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev, "failed to get hwspinlock id\n");
>> +             return ret;
>> +     }
>> +
>> +     sc27xx_data->hwlock = hwspin_lock_request_specific(ret);
>> +     if (!sc27xx_data->hwlock) {
>> +             dev_err(&pdev->dev, "failed to request hwspinlock\n");
>> +             return -ENXIO;
>> +     }
>> +
>> +     init_completion(&sc27xx_data->completion);
>> +
>> +     /*
>> +      * Different PMIC ADC controllers can have differnt channel voltage
>
> different

Sure, will fix the typo. Thanks for your helpful comments.

-- 
Baolin.wang
Best Regards