Hello Andy,
A few comments inline.
On Mon, Mar 18, 2019 at 12:51:03PM +0300, Andy Shevchenko wrote:
> From: Vincent Pelletier <plr.vincent@xxxxxxxxx>
>
> Exposes the ADC device present on, at least, Intel Merrifield platform.
>
> Based on work done by:
> Yang Bin <bin.yang@xxxxxxxxx>
> Huiquan Zhong <huiquan.zhong@xxxxxxxxx>
> Sumeet Pawnikar <sumeet.r.pawnikar@xxxxxxxxx>
> Pavan Kumar S <pavan.kumar.s@xxxxxxxxx>
>
> Though it has been heavily rewritten for upstream.
>
> Signed-off-by: Vincent Pelletier <plr.vincent@xxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/iio/adc/Kconfig | 13 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/intel_mrfld_adc.c | 264 ++++++++++++++++++++++++++++++
> 3 files changed, 278 insertions(+)
> create mode 100644 drivers/iio/adc/intel_mrfld_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 76db6e5cc296..f30c531fc588 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -419,6 +419,19 @@ config INGENIC_ADC
> This driver can also be built as a module. If so, the module will be
> called ingenic_adc.
>
> +config INTEL_MRFLD_ADC
> + tristate "Intel Merrifield Basin Cove ADC driver"
> + depends on INTEL_SOC_PMIC_MRFLD
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
Looks you're not using iio buffering hence these should be dropped.
Instead you should select regmap here.
> + help
> + Say yes here to have support for Basin Cove power management IC (PMIC) ADC
> + device. Depending on platform configuration, this general purpose ADC can
> + be used for sampling sensors such as thermal resistors.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called intel_mrfld_adc.
> +
> config IMX7D_ADC
> tristate "Freescale IMX7D ADC driver"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 6fcebd167524..6cbf2e5a51a7 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_HX711) += hx711.o
> obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
> obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
> +obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
> obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
> diff --git a/drivers/iio/adc/intel_mrfld_adc.c b/drivers/iio/adc/intel_mrfld_adc.c
> new file mode 100644
> index 000000000000..de3bc034f651
> --- /dev/null
> +++ b/drivers/iio/adc/intel_mrfld_adc.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADC driver for Basin Cove PMIC
> + *
> + * Copyright (C) 2012 Intel Corporation
> + * Author: Bin Yang <bin.yang@xxxxxxxxx>
> + *
> + * Rewritten for upstream by:
> + * Vincent Pelletier <plr.vincent@xxxxxxxxx>
> + * Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/mfd/intel_soc_pmic_mrfld.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
Alphabetical order is preferred. Generally it's also good idea to include
all headers which export functionality used by the driver. At least a few
seems to be missing from the list.
> +
> +#define BCOVE_GPADCREQ 0xDC
> +#define BCOVE_GPADCREQ_BUSY BIT(0)
> +#define BCOVE_GPADCREQ_IRQEN BIT(1)
> +
> +#define BCOVE_ADCIRQ_ALL ( \
> + BCOVE_ADCIRQ_BATTEMP | \
> + BCOVE_ADCIRQ_SYSTEMP | \
> + BCOVE_ADCIRQ_BATTID | \
> + BCOVE_ADCIRQ_VIBATT | \
> + BCOVE_ADCIRQ_CCTICK)
> +
> +#define BCOVE_ADC_TIMEOUT msecs_to_jiffies(1000)
> +
> +static const u8 mrfld_adc_requests[] = {
> + BCOVE_ADCIRQ_VIBATT,
> + BCOVE_ADCIRQ_BATTID,
> + BCOVE_ADCIRQ_VIBATT,
> + BCOVE_ADCIRQ_SYSTEMP,
> + BCOVE_ADCIRQ_BATTEMP,
> + BCOVE_ADCIRQ_BATTEMP,
> + BCOVE_ADCIRQ_SYSTEMP,
> + BCOVE_ADCIRQ_SYSTEMP,
> + BCOVE_ADCIRQ_SYSTEMP,
> +};
> +
> +struct mrfld_adc {
> + struct regmap *regmap;
> + struct completion completion;
> +};
> +
> +static irqreturn_t mrfld_adc_thread_isr(int irq, void *data)
> +{
> + struct iio_dev *indio_dev = data;
> + struct mrfld_adc *adc = iio_priv(indio_dev);
> +
> + complete(&adc->completion);
> + return IRQ_HANDLED;
> +}
> +
> +static int mrfld_adc_single_conv(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *result)
> +{
> + struct mrfld_adc *adc = iio_priv(indio_dev);
> + struct regmap *regmap = adc->regmap;
> + unsigned int req;
> + long timeout;
> + u8 buf[2];
> + int ret;
> +
> + reinit_completion(&adc->completion);
> +
> + regmap_update_bits(regmap, BCOVE_MADCIRQ, BCOVE_ADCIRQ_ALL, 0);
> + regmap_update_bits(regmap, BCOVE_MIRQLVL1, BCOVE_LVL1_ADC, 0);
> +
> + ret = regmap_read_poll_timeout(regmap, BCOVE_GPADCREQ, req,
> + !(req & BCOVE_GPADCREQ_BUSY),
> + 2000, 1000000);
> + if (ret)
> + goto done;
> +
> + req = mrfld_adc_requests[chan->channel];
> + ret = regmap_write(regmap, BCOVE_GPADCREQ, BCOVE_GPADCREQ_IRQEN | req);
> + if (ret)
> + goto done;
> +
> + timeout = wait_for_completion_interruptible_timeout(&adc->completion,
> + BCOVE_ADC_TIMEOUT);
> + if (timeout < 0) {
> + ret = timeout;
> + goto done;
> + }
> + if (timeout == 0) {
> + ret = -ETIMEDOUT;
> + goto done;
> + }
> +
> + ret = regmap_bulk_read(regmap, chan->address, buf, 2);
> + if (ret)
> + goto done;
> +
> + *result = (buf[0] << 8) | buf[1];
be/le16_to_cpu() will do it for you.
> + ret = IIO_VAL_INT;
> +
> +done:
> + regmap_update_bits(regmap, BCOVE_MIRQLVL1, BCOVE_LVL1_ADC, 0xff);
> + regmap_update_bits(regmap, BCOVE_MADCIRQ, BCOVE_ADCIRQ_ALL, 0xff);
> +
> + return ret;
> +}
> +
> +static int mrfld_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = mrfld_adc_single_conv(indio_dev, chan, val);
> +
> + iio_device_release_direct_mode(indio_dev);
claim/release api is slightly abused here. Legitimate usecase is when
you want protect against transitions between driver modes and not
concurrent reads. Mutex will work here just fine.
> + return ret;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info mrfld_adc_iio_info = {
> + .read_raw = &mrfld_adc_read_raw,
> +};
> +
> +#define BCOVE_ADC_CHANNEL(_type, _channel, _datasheet_name, _address) \
> + { \
> + .indexed = 1, \
> + .type = _type, \
> + .channel = _channel, \
> + .address = _address, \
> + .datasheet_name = _datasheet_name, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 10, \
> + .storagebits = 16, \
> + .shift = 0, \
> + .endianness = IIO_BE, \
> + } \
Scan elements make sense only in buffered mode which is not supported
by this driver.
> + }
> +
> +static const struct iio_chan_spec mrfld_adc_channels[] = {
> + BCOVE_ADC_CHANNEL(IIO_VOLTAGE, 0, "CH0", 0xE9),
> + BCOVE_ADC_CHANNEL(IIO_RESISTANCE, 1, "CH1", 0xEB),
> + BCOVE_ADC_CHANNEL(IIO_CURRENT, 2, "CH2", 0xED),
> + BCOVE_ADC_CHANNEL(IIO_TEMP, 3, "CH3", 0xCC),
> + BCOVE_ADC_CHANNEL(IIO_TEMP, 4, "CH4", 0xC8),
> + BCOVE_ADC_CHANNEL(IIO_TEMP, 5, "CH5", 0xCA),
> + BCOVE_ADC_CHANNEL(IIO_TEMP, 6, "CH6", 0xC2),
> + BCOVE_ADC_CHANNEL(IIO_TEMP, 7, "CH7", 0xC4),
> + BCOVE_ADC_CHANNEL(IIO_TEMP, 8, "CH8", 0xC6),
> +};
> +
> +static struct iio_map iio_maps[] = {
> + IIO_MAP("CH0", "bcove-battery", "VBATRSLT"),
> + IIO_MAP("CH1", "bcove-battery", "BATTID"),
> + IIO_MAP("CH2", "bcove-battery", "IBATRSLT"),
> + IIO_MAP("CH3", "bcove-temp", "PMICTEMP"),
> + IIO_MAP("CH4", "bcove-temp", "BATTEMP0"),
> + IIO_MAP("CH5", "bcove-temp", "BATTEMP1"),
> + IIO_MAP("CH6", "bcove-temp", "SYSTEMP0"),
> + IIO_MAP("CH7", "bcove-temp", "SYSTEMP1"),
> + IIO_MAP("CH8", "bcove-temp", "SYSTEMP2"),
> + {}
> +};
> +
> +static int mrfld_adc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent);
> + struct iio_dev *indio_dev;
> + struct mrfld_adc *adc;
> + int irq;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*indio_dev));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + adc = iio_priv(indio_dev);
> +
> + init_completion(&adc->completion);
> + adc->regmap = pmic->regmap;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL, mrfld_adc_thread_isr,
> + IRQF_ONESHOT | IRQF_SHARED, pdev->name,
> + indio_dev);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + indio_dev->dev.parent = dev;
> + indio_dev->name = pdev->name;
> +
> + indio_dev->channels = mrfld_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mrfld_adc_channels);
> + indio_dev->info = &mrfld_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = iio_map_array_register(indio_dev, iio_maps);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret < 0)
> + goto err_array_unregister;
> +
> + return 0;
> +
> +err_array_unregister:
> + iio_map_array_unregister(indio_dev);
> + return ret;
> +}
> +
> +static int mrfld_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> + iio_map_array_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct platform_device_id mrfld_adc_id_table[] = {
> + { .name = "mrfld_bcove_adc" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(platform, mrfld_adc_id_table);
> +
> +static struct platform_driver mrfld_adc_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + },
> + .probe = mrfld_adc_probe,
> + .remove = mrfld_adc_remove,
> + .id_table = mrfld_adc_id_table,
> +};
> +module_platform_driver(mrfld_adc_driver);
> +
> +MODULE_AUTHOR("Bin Yang <bin.yang@xxxxxxxxx>");
> +MODULE_AUTHOR("Vincent Pelletier <plr.vincent@xxxxxxxxx>");
> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("ADC driver for Basin Cove PMIC");
> +MODULE_LICENSE("GPL v2");
> --
> 2.20.1
>
