Hi Jonathan,
On 17 June 2018 at 02:35, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> 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.
Thanks to point the race issue.
>
> 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).
See below comments.
>
> 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.
Can I rename to be 'sprd-pmic-adc.c'? I can not rename it as
'sc2731-adc', we have differnet PMICs (SC2730, SC2731, SC2720 etc.),
but they are all integrated the same ADC controller.
>> ---
>> 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..)
Like I explained above, maybe change to 'sprd_pmic_adc.c', is this OK for you?
>
>> @@ -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.
Sure.
>
>> +#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..
Since this is special for our ADC controller, we have 2 channels that
has been calibrated in hardware, but for other
none-calibrated-channels, we should care about the channel voltage
ratio when converting to a real voltage values, that is because some
channel's voltage is larger so we need one voltage ratio to sample the
ADC values.
>> +
>> + 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.
Unfortunately, we can not use the formula ( (X_raw + X_offset)*X_scale
= X_processed) for our ADC controller to get a processed value.
Firstly, the ADC hardware will do the sampling with the scale value.
Secondly we should convert a raw value to a voltage value by the
linear graph table, for some channels, we should also use the channel
voltage ratio to get a real voltage value. So I think we should keep
our special processed approach for consumers.
>> + 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?
OK, I can remove the lock.
>
>> + 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.
You are right, we should ensure previous operations are undone. Will
fix in next version.
>> +
>> + /* 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.
Thanks for pointing this issue out and will fix in next version.
>
>
>> + 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.
That is a good idea and I will add some additional actions with devm
to avoid the race issue.
>
>> + 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!)
Sure.
>
>> + 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.
Sure.
>
>> + 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.
Yes, this is redundant.
>
> 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.
Make sense and I will remove it. Very appreciate for your comments.
--
Baolin.wang
Best Regards
--
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
