Re: [PATCH v2 2/2] iio: adc: Add Spreadtrum SC27XX PMICs ADC support

[Jonathan Cameron]

On Fri, 15 Jun 2018 15:03:36 +0800
Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:

> From: Freeman Liu <freeman.liu@xxxxxxxxxxxxxx>
> 
> 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>

Hi,

There are some race conditions around the probe and remove.
More care is needed when we have a mixture of managed and unmanaged cleanup
like here.

I'm not understanding the way you have exposed a simple _raw and _scale
attributes with what looks to be different scaling to that applied
in _processed.   As I say below, we should not have both of those interface
options anyway.  The ABI is that (X_raw + X_offset)*X_scale = X_processed.
(with defaults of X_scale = 1 and X_offset = 0).

Please rename to avoid using wild cards in the name.  That's gone
wrong so many times in the past you wouldn't believe it!
Hmm Awkward though if the MFD is already upstream. Ah well, I guess
for consistency we should follow that and groan when it goes wrong.


> ---
> Changes since v1:
>  - Add const for static structures definition.
>  - Change SC27XX_ADC_TO_VOLTAGE macro to be one function.
>  - Move channel scale accessing into mutex protection.
>  - Fix some typos.
> ---
>  drivers/iio/adc/Kconfig      |   10 +
>  drivers/iio/adc/Makefile     |    1 +
>  drivers/iio/adc/sc27xx_adc.c |  547 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 558 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..52e5b74
> --- /dev/null
> +++ b/drivers/iio/adc/sc27xx_adc.c

In general (i.e. when we notice in time) we don't allow wild cards in names.
Far too many times we did this in the past and ended up with later parts
that fitted the name, but could not be supported by the driver.

The convention is to name everything after the first part supported.
So here, sc2731. (I relaxed my thoughts on this later having seen the mfd
has this naming - so there are no ideal options left..)

> @@ -0,0 +1,547 @@
> +// 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
Please avoid wild cards.  This goes wrong an awful lot of the time
when a company comes out with an incompatible part that fits in the
existing wild cards.

> +#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)
> +
> +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)(int 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 const struct sc27xx_adc_linear_graph big_scale_graph = {
> +	4200, 3310,
> +	3600, 2832,
> +};
> +
> +static const struct sc27xx_adc_linear_graph small_scale_graph = {
> +	1000, 3413,
> +	100, 341,
> +};
> +
> +static int sc27xx_adc_2731_ratio(int channel, int scale)
> +{
> +	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, int 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;
> +	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,
> +				  int 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_to_volt(const struct sc27xx_adc_linear_graph *graph,
> +			      int raw_adc)
> +{
> +	int tmp;
> +
> +	tmp = (graph->volt0 - graph->volt1) * (raw_adc - graph->adc1);
> +	tmp /= (graph->adc0 - graph->adc1);
> +	tmp += graph->volt1;
> +
> +	return tmp < 0 ? 0 : tmp;
> +}
> +
> +static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data, int 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.
> +	 */
> +	switch (channel) {
> +	case 5:
> +		return sc27xx_adc_to_volt(&big_scale_graph, raw_adc);
> +
> +	case 1:
> +		return sc27xx_adc_to_volt(&small_scale_graph, raw_adc);
> +
> +	default:
> +		volt = sc27xx_adc_to_volt(&small_scale_graph, raw_adc);
> +		break;
> +	}

This looks a lot more complex than simple scaling that is indicated by the 
raw and scale attributes? They can't both be right..
> +
> +	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,
> +				     int 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, ret, tmp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		scale = data->channel_scale[chan->channel];
> +		ret = sc27xx_adc_read(data, chan->channel, scale, &tmp);
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret)
> +			return ret;
> +
> +		*val = tmp;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_PROCESSED:
> +		mutex_lock(&indio_dev->mlock);
> +		scale = data->channel_scale[chan->channel];
> +		ret = sc27xx_adc_read_processed(data, chan->channel, scale,
> +						&tmp);

To keep to the rule of 'one way to read a value' we don't tend to support
both raw and processed.  The only exception is made for devices where we got
this wrong in the first place and so have to support both to avoid a potential
regression due to ABI changes.

If it is a simple linear scaling (like here I think) then the preferred option
is to not supply the processed version.  Just do raw.


> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret)
> +			return ret;
> +
> +		*val = tmp;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		mutex_lock(&indio_dev->mlock);
> +		*val = data->channel_scale[chan->channel];
> +		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;
> +		mutex_unlock(&indio_dev->mlock);

This is rather heavy weight locking for an integer write that will
be atomic (I hope!) anyway.  Hence I don't think you need to
lock around this value at all.

It 'might' change during a read, but given you take a snapshot
of it anyway I'm not sure why that would matter?

> +		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;
Hmm. We allow this function to return errors, but don't really clean up properly
if it happens.

So errors in later paths than this one should ensure this is undone.  The state
on exit from this function (when an error occurs) should be as close as possible
to the state on entry.

Now things get interesting if the failure indicates we probably have a hardware
failure, but it would still be nice to be consistent and try and unwind.
> +
> +	/* 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 different channel voltage
> +	 * ratios, so we should save the implementation of getting voltage
> +	 * ratio for corresponding PMIC ADC in the device data structure.
> +	 */
> +	sc27xx_data->get_volt_ratio = data;
> +	sc27xx_data->dev = &pdev->dev;
> +
> +	ret = sc27xx_adc_enable(sc27xx_data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable ADC module\n");
> +		goto free_hwlock;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, sc27xx_data->irq, NULL,
> +					sc27xx_adc_isr, IRQF_ONESHOT,
> +					pdev->name, sc27xx_data);

This worries me from a race point of view as well. You shouldn't have
an interrupt still in use once the adc_disable in the remove is
called.  It might be safe, but it's not immediately obvious that it
is.  As such I'd rather we didn't use the managed interrupt request here.
So use request_threaded_irq and free_irq as appropriate instead.


> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request ADC irq\n");
> +		goto disable_adc;
> +	}
> +
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &sc27xx_info;
> +	indio_dev->channels = sc27xx_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(sc27xx_channels);
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret) {
The moment I see any unwinding happening after a devm call I know
there is probably a race.

Here the race is that you will be turning the ADC off and unlocking
before the userspace interface is removed (as devm unwind will happen
after remove is finished).

You'll have to use the non managed version of iio_device_register
and unwind by hand to avoid this.

Or you could if you prefer register some additional actions with devm
core to call the adc disable and hw_spinlock_free as appropriate.

> +		dev_err(&pdev->dev, "could not register iio (ADC)");
> +		goto disable_adc;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
Generally put a blank line before simple returns like this,
Aids readability (a very small amount!)

> +	return 0;
> +
> +disable_adc:
> +	sc27xx_adc_disable(sc27xx_data);
> +free_hwlock:
> +	hwspin_lock_free(sc27xx_data->hwlock);
> +	return ret;
> +}
> +
> +static int sc27xx_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct sc27xx_adc_data *sc27xx_data = iio_priv(indio_dev);
> +
> +	sc27xx_adc_disable(sc27xx_data);
> +	hwspin_lock_free(sc27xx_data->hwlock);

blank line here please.

> +	return 0;
> +}
> +
> +static const struct of_device_id sc27xx_adc_of_match[] = {
> +	{
> +		.compatible = "sprd,sc2731-adc",
> +		.data = (void *)&sc27xx_adc_2731_ratio,

There is no need to cast to (void *) Implicit casting too
and from void pointers is always fine under the c spec.

However, for cleanness I would put that function pointer into
a const structure.  Chances are you'll need something else in there
for some future feature and this would make it a lot cleaner.

Also, a little unusual todo this when submitting a driver
supporting only one part.  I'm fine leaving it if you confirm there
are other parts going to be supported by this driver in the near future.
Otherwise drop this unnecessary abstraction.

> +	},
> +	{ }
> +};
> +
> +static struct platform_driver sc27xx_adc_driver = {
> +	.probe = sc27xx_adc_probe,
> +	.remove = sc27xx_adc_remove,
> +	.driver = {
> +		.name = "sc27xx-adc",
> +		.of_match_table = sc27xx_adc_of_match,
> +	},
> +};
> +
> +module_platform_driver(sc27xx_adc_driver);
> +
> +MODULE_AUTHOR("Freeman Liu <freeman.liu@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Spreadtrum SC27XX ADC Driver");
> +MODULE_LICENSE("GPL v2");