| Message ID | 5728839377cefd20cdb95913b43dbdab530c1e81.1529040864.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 15 Jun 2018 15:03:36 +0800 Baolin Wang <baolin.wang@linaro.org> wrote: > From: Freeman Liu <freeman.liu@spreadtrum.com> > > 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> 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@spreadtrum.com>"); > +MODULE_DESCRIPTION("Spreadtrum SC27XX ADC Driver"); > +MODULE_LICENSE("GPL v2"); -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jonathan, On 17 June 2018 at 02:35, Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 15 Jun 2018 15:03:36 +0800 > Baolin Wang <baolin.wang@linaro.org> wrote: > >> From: Freeman Liu <freeman.liu@spreadtrum.com> >> >> 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> > > 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.
On 17 June 2018 09:03:04 BST, Baolin Wang <baolin.wang@linaro.org> wrote: >Hi Jonathan, > >On 17 June 2018 at 02:35, Jonathan Cameron <jic23@kernel.org> wrote: >> On Fri, 15 Jun 2018 15:03:36 +0800 >> Baolin Wang <baolin.wang@linaro.org> wrote: >> >>> From: Freeman Liu <freeman.liu@spreadtrum.com> >>> >>> 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> >> >> 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. You can rename as this is a common problem throughout drivers. There is no good solution. Given mfd naming, just leave it with wild cards as better than a name no one will recognise > >>> --- >>> 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? Goes wrong just as quickly as wild cards... > >> >>> @@ -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. It's still a question of one or the other. Channels should not do processed and raw without a very good reason. Issue with processed is that you can't easily do buffered chrdev streaming in future. > >>> + >>> + 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. Hmm fair enough, scale is fine for that. But don't provide raw unless real voltage is scale *raw >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. That's fine but drop the raw access or you are not obeying the abi. > >>> + 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.
Hi Jonathan, On 18 June 2018 at 18:20, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > > > On 17 June 2018 09:03:04 BST, Baolin Wang <baolin.wang@linaro.org> wrote: >>Hi Jonathan, >> >>On 17 June 2018 at 02:35, Jonathan Cameron <jic23@kernel.org> wrote: >>> On Fri, 15 Jun 2018 15:03:36 +0800 >>> Baolin Wang <baolin.wang@linaro.org> wrote: >>> >>>> From: Freeman Liu <freeman.liu@spreadtrum.com> >>>> >>>> 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> >>> >>> 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. > > You can rename as this is a common problem throughout drivers. There is no good solution. > > Given mfd naming, just leave it with wild cards as better than a name no one will recognise OK. >> >>>> --- >>>> 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? > Goes wrong just as quickly as wild cards... OK. >>>> + >>>> +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. > > It's still a question of one or the other. Channels should not do processed and raw without a very good reason. I think I have explained why we need our special processed approach as below. > > Issue with processed is that you can't easily do buffered chrdev streaming in future. > > >> >>>> + >>>> + 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. > > Hmm fair enough, scale is fine for that. But don't provide raw unless real voltage is scale *raw > >>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. > > That's fine but drop the raw access or you are not obeying the abi. Sorry, I think I did not get your points. Could you elaborate on why we can not provide raw and processed? I saw many drivers not only provide the raw access but also the processed access. Especially for our special processed approach, I think the raw access and the processed access are both needed for consumers. Thanks.
On Mon, 18 Jun 2018 18:47:18 +0800 Baolin Wang <baolin.wang@linaro.org> wrote: > Hi Jonathan, > > On 18 June 2018 at 18:20, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > > > > > > On 17 June 2018 09:03:04 BST, Baolin Wang <baolin.wang@linaro.org> wrote: > >>Hi Jonathan, > >> > >>On 17 June 2018 at 02:35, Jonathan Cameron <jic23@kernel.org> wrote: > >>> On Fri, 15 Jun 2018 15:03:36 +0800 > >>> Baolin Wang <baolin.wang@linaro.org> wrote: > >>> > >>>> From: Freeman Liu <freeman.liu@spreadtrum.com> > >>>> > >>>> 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> > >>> > >>> 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. > > > > You can rename as this is a common problem throughout drivers. There is no good solution. > > > > Given mfd naming, just leave it with wild cards as better than a name no one will recognise > > OK. > > >> > >>>> --- > >>>> 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? > > Goes wrong just as quickly as wild cards... > > OK. > > >>>> + > >>>> +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. > > > > It's still a question of one or the other. Channels should not do processed and raw without a very good reason. > > I think I have explained why we need our special processed approach as below. > > > > > Issue with processed is that you can't easily do buffered chrdev streaming in future. > > > > > >> > >>>> + > >>>> + 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. > > > > Hmm fair enough, scale is fine for that. But don't provide raw unless real voltage is scale *raw > > > >>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. > > > > That's fine but drop the raw access or you are not obeying the abi. > > Sorry, I think I did not get your points. Could you elaborate on why > we can not provide raw and processed? > I saw many drivers not only provide the raw access but also the > processed access. Especially for our special processed approach, I > think the raw access and the processed access are both needed for > consumers. Thanks. > That's not true. There are a few 'very special cases' where this happens. We had cases where the driver was already putting out RAW values when it turned out there was no sensible way of converting it to processed values (non linear as in your case). As the RAW interface was existing ABI we had to maintain it even though in some senses this was a bug. The other cases that look a bit like this are when multiple input sensors are fused to produce a computed output value. This happens with light sensors. However, the raw and processed channels are different channels (as there is no one to one mapping). It is not uncommon to provide a 'mixture' of raw and processed but they are not for the same channels. So in general we simply do not allow both to be provided for a single channel. There is no useful purpose in doing so and it complicates the exposed ABI. Jonathan -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jonathan, On 22 June 2018 at 22:26, Jonathan Cameron <jonathan.cameron@huawei.com> wrote: > On Mon, 18 Jun 2018 18:47:18 +0800 > Baolin Wang <baolin.wang@linaro.org> wrote: > >> Hi Jonathan, >> >> On 18 June 2018 at 18:20, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: >> > >> > >> > On 17 June 2018 09:03:04 BST, Baolin Wang <baolin.wang@linaro.org> wrote: >> >>Hi Jonathan, >> >> >> >>On 17 June 2018 at 02:35, Jonathan Cameron <jic23@kernel.org> wrote: >> >>> On Fri, 15 Jun 2018 15:03:36 +0800 >> >>> Baolin Wang <baolin.wang@linaro.org> wrote: >> >>> >> >>>> From: Freeman Liu <freeman.liu@spreadtrum.com> >> >>>> >> >>>> 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> >> >>> >> >>> 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. >> > >> > You can rename as this is a common problem throughout drivers. There is no good solution. >> > >> > Given mfd naming, just leave it with wild cards as better than a name no one will recognise >> >> OK. >> >> >> >> >>>> --- >> >>>> 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? >> > Goes wrong just as quickly as wild cards... >> >> OK. >> >> >>>> + >> >>>> +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. >> > >> > It's still a question of one or the other. Channels should not do processed and raw without a very good reason. >> >> I think I have explained why we need our special processed approach as below. >> >> > >> > Issue with processed is that you can't easily do buffered chrdev streaming in future. >> > >> > >> >> >> >>>> + >> >>>> + 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. >> > >> > Hmm fair enough, scale is fine for that. But don't provide raw unless real voltage is scale *raw >> > >> >>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. >> > >> > That's fine but drop the raw access or you are not obeying the abi. >> >> Sorry, I think I did not get your points. Could you elaborate on why >> we can not provide raw and processed? >> I saw many drivers not only provide the raw access but also the >> processed access. Especially for our special processed approach, I >> think the raw access and the processed access are both needed for >> consumers. Thanks. >> > > That's not true. There are a few 'very special cases' where this happens. > We had cases where the driver was already putting out RAW values when > it turned out there was no sensible way of converting it to processed values > (non linear as in your case). As the RAW interface was existing ABI we had > to maintain it even though in some senses this was a bug. > > The other cases that look a bit like this are when multiple input sensors > are fused to produce a computed output value. This happens with light > sensors. However, the raw and processed channels are different channels > (as there is no one to one mapping). > > It is not uncommon to provide a 'mixture' of raw and processed but they > are not for the same channels. > > So in general we simply do not allow both to be provided for a single channel. > There is no useful purpose in doing so and it complicates the exposed ABI. Thanks for your patient explanation, now I understood.
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 @@ -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 +#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; + } + + 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); + 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); + 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 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); + 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) { + dev_err(&pdev->dev, "could not register iio (ADC)"); + goto disable_adc; + } + + platform_set_drvdata(pdev, indio_dev); + 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); + return 0; +} + +static const struct of_device_id sc27xx_adc_of_match[] = { + { + .compatible = "sprd,sc2731-adc", + .data = (void *)&sc27xx_adc_2731_ratio, + }, + { } +}; + +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@spreadtrum.com>"); +MODULE_DESCRIPTION("Spreadtrum SC27XX ADC Driver"); +MODULE_LICENSE("GPL v2");