Re: [PATCH v1] iio: adc: intel_mrfld_adc: Add Basin Cove ADC driver

[Tomasz Duszynski <tduszyns@gmail.com>]

Hello Andy,

A few comments inline.

On Mon, Mar 18, 2019 at 12:51:03PM +0300, Andy Shevchenko wrote:
> From: Vincent Pelletier <plr.vincent@gmail.com>
>
> Exposes the ADC device present on, at least, Intel Merrifield platform.
>
> Based on work done by:
>   Yang Bin <bin.yang@intel.com>
>   Huiquan Zhong <huiquan.zhong@intel.com>
>   Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>
>   Pavan Kumar S <pavan.kumar.s@intel.com>
>
> Though it has been heavily rewritten for upstream.
>
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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@intel.com>
> + *
> + * Rewritten for upstream by:
> + *	 Vincent Pelletier <plr.vincent@gmail.com>
> + *	 Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> + */
> +
> +#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@intel.com>");
> +MODULE_AUTHOR("Vincent Pelletier <plr.vincent@gmail.com>");
> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
> +MODULE_DESCRIPTION("ADC driver for Basin Cove PMIC");
> +MODULE_LICENSE("GPL v2");
> --
> 2.20.1
>