[PATCH 0/5] mfd: rn5t618: add ADC support

[PATCH 0/5] mfd: rn5t618: add ADC support

[Andreas Kemnade]

This series adds support for the ADC in the RN5T618/RC5T619.
It depends on the IRQ support added in the RTC support series here:
https://lore.kernel.org/lkml/20191220122416.31881-1-andreas@kemnade.info/

First regmap setup has to be fixed because there is a register
wrongly classified as volatile.

I tested the driver only with the RC5T619 but it should work with the with 
the RN5T618 as well based on these facts:
- The corresponding register definitions originally went into the kernel
  for the RN5T618
- Public datasheet sections about the ADC look same.
- Out-of-tree code for these chips look same regarding to ADC

I marked these untested patches as RFC, and IMHO they require a Tested-By.
Feel free to ignore them if the whole series would be delayed just because
of missing Tested-By for those.

Andreas Kemnade (5):
  mfd: rn5t618: mark ADC control register volatile
  mfd: rn5t618: add ADC subdevice for RC5T619
  iio: adc: rn5t618: Add ADC driver for RN5T618/RC5T619
  mfd: rn5t618: add IRQ definitions for RN5T618
  mfd: rn5t618: add ADC subdevice for RN5T618

 drivers/iio/adc/Kconfig       |  10 ++
 drivers/iio/adc/Makefile      |   1 +
 drivers/iio/adc/rn5t618-adc.c | 266 ++++++++++++++++++++++++++++++++++
 drivers/mfd/rn5t618.c         |  49 ++++++-
 4 files changed, 324 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iio/adc/rn5t618-adc.c

-- 
2.20.1

[PATCH 1/5] mfd: rn5t618: mark ADC control register volatile

[Andreas Kemnade]

There is a bit which gets cleared after conversion.

Fixes: 9bb9e29c78f8 ("mfd: Add Ricoh RN5T618 PMIC core driver")
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/mfd/rn5t618.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index ec378715137b..073de8e0e78b 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -35,6 +35,7 @@ static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
 	case RN5T618_WATCHDOGCNT:
 	case RN5T618_DCIRQ:
 	case RN5T618_ILIMDATAH ... RN5T618_AIN0DATAL:
+	case RN5T618_ADCCNT3:
 	case RN5T618_IR_ADC1 ... RN5T618_IR_ADC3:
 	case RN5T618_IR_GPR:
 	case RN5T618_IR_GPF:
-- 
2.20.1

[PATCH 2/5] mfd: rn5t618: add ADC subdevice for RC5T619

[Andreas Kemnade]

This adds a subdevice for the ADC in the RC5T619.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/mfd/rn5t618.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index 073de8e0e78b..321836f78120 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -24,6 +24,7 @@ static const struct mfd_cell rn5t618_cells[] = {
 };
 
 static const struct mfd_cell rc5t619_cells[] = {
+	{ .name = "rn5t618-adc" },
 	{ .name = "rn5t618-regulator" },
 	{ .name = "rc5t619-rtc" },
 	{ .name = "rn5t618-wdt" },
-- 
2.20.1

[PATCH 3/5] iio: adc: rn5t618: Add ADC driver for RN5T618/RC5T619

[Andreas Kemnade]

Both chips have an A/D converter capable of measuring
things like VBAT, VUSB and analog inputs.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/iio/adc/Kconfig       |  10 ++
 drivers/iio/adc/Makefile      |   1 +
 drivers/iio/adc/rn5t618-adc.c | 266 ++++++++++++++++++++++++++++++++++
 3 files changed, 277 insertions(+)
 create mode 100644 drivers/iio/adc/rn5t618-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f0af3a42f53c..9ea9489e3f0a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -735,6 +735,16 @@ config RCAR_GYRO_ADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called rcar-gyroadc.
 
+config RN5T618_ADC
+	tristate "ADC for the RN5T618/RC5T619 family of chips"
+	depends on MFD_RN5T618
+	help
+	  Say yes here to build support for the integrated ADC inside the
+	  RN5T618/619 series PMICs:
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rn5t618-adc.
+
 config ROCKCHIP_SARADC
 	tristate "Rockchip SARADC driver"
 	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ef9cc485fb67..2aea70556ed0 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o
 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_RN5T618_ADC) += rn5t618-adc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
diff --git a/drivers/iio/adc/rn5t618-adc.c b/drivers/iio/adc/rn5t618-adc.c
new file mode 100644
index 000000000000..81f872a7ad7f
--- /dev/null
+++ b/drivers/iio/adc/rn5t618-adc.c
@@ -0,0 +1,266 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ADC driver for the RICOH RN5T618 power management chip family
+ *
+ * Copyright (C) 2019 Andreas Kemnade
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mfd/rn5t618.h>
+#include <linux/platform_device.h>
+#include <linux/completion.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/slab.h>
+#include <linux/irqdomain.h>
+
+#define RN5T618_ADC_CONVERSION_TIMEOUT   (msecs_to_jiffies(500))
+#define REFERENCE_VOLT 2500
+
+/* mask for selecting channels for single conversion */
+#define ADCCNT3_CHANNEL_MASK 0x7
+/* average 4-time conversion mode */
+#define ADCCNT3_AVG BIT(3)
+/* set for starting a single conversion, gets cleared by hw when done */
+#define ADCCNT3_GODONE BIT(4)
+/* automatic conversion, period is in ADCCNT2, selected channels are
+ * in ADCCNT1
+ */
+#define ADCCNT3_AUTO BIT(5)
+#define ADCEND_IRQ BIT(0)
+
+struct rn5t618_adc_data {
+	struct device *dev;
+	struct rn5t618 *rn5t618;
+	struct completion conv_completion;
+	int irq;
+};
+
+struct rn5t618_channel_ratios {
+	u16 numerator;
+	u16 denominator;
+};
+
+static const struct rn5t618_channel_ratios rn5t618_ratios[8] = {
+	{50, 32}, /* LIMMON measured across 20mOhm, amplified by 32 */
+	{2, 1}, /* VBAT */
+	{3, 1}, /* VADP */
+	{3, 1}, /* VUSB */
+	{3, 1}, /* VSYS */
+	{1, 1}, /* VTHM */
+	{1, 1}, /* AIN1 */
+	{1, 1}, /* AIN0 */
+};
+
+static int rn5t618_read_adc_reg(struct rn5t618 *rn5t618, int reg, u16 *val)
+{
+	unsigned int h;
+	unsigned int l;
+	int ret;
+
+	ret = regmap_read(rn5t618->regmap, reg, &h);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(rn5t618->regmap, reg + 1, &l);
+	if (ret < 0)
+		return ret;
+
+	h <<= 4;
+	h |= (l & 0xF);
+	h &= 0xFFF;
+	*val = h;
+
+	return 0;
+}
+
+static irqreturn_t rn5t618_adc_irq(int irq, void *data)
+{
+	struct rn5t618_adc_data *adc = data;
+	unsigned int r = 0;
+	int ret;
+
+	/* clear low & high threshold irqs */
+	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC1, 0);
+	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC2, 0);
+
+	ret = regmap_read(adc->rn5t618->regmap, RN5T618_IR_ADC3, &r);
+	if (ret < 0)
+		dev_err(adc->dev, "failed to read IRQ status: %d\n", ret);
+
+	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC3, 0);
+
+	if (r & ADCEND_IRQ)
+		complete(&adc->conv_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int rn5t618_adc_read(struct iio_dev *iio_dev,
+			    const struct iio_chan_spec *chan,
+			    int *val, int *val2, long mask)
+{
+	struct rn5t618_adc_data *adc = iio_priv(iio_dev);
+	u16 raw;
+	int ret;
+
+	/* select channel */
+	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
+				 ADCCNT3_CHANNEL_MASK,
+				 chan->channel);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(adc->rn5t618->regmap, RN5T618_EN_ADCIR3, ADCEND_IRQ);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
+				 ADCCNT3_AVG,
+				 mask == IIO_CHAN_INFO_AVERAGE_RAW ?
+				 ADCCNT3_AVG : 0);
+	if (ret < 0)
+		return ret;
+
+	init_completion(&adc->conv_completion);
+	/* single conversion */
+	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
+				 ADCCNT3_GODONE, ADCCNT3_GODONE);
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_timeout(&adc->conv_completion,
+					  RN5T618_ADC_CONVERSION_TIMEOUT);
+	if (ret == 0) {
+		dev_warn(adc->dev, "timeout waiting for adc result\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = rn5t618_read_adc_reg(adc->rn5t618,
+				   RN5T618_ILIMDATAH + 2 * chan->channel,
+				   &raw);
+	if (ret < 0)
+		return ret;
+
+	*val = raw;
+	if (mask == IIO_CHAN_INFO_PROCESSED)
+		*val = *val * REFERENCE_VOLT *
+		       rn5t618_ratios[chan->channel].numerator /
+		       rn5t618_ratios[chan->channel].denominator / 4095;
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info rn5t618_adc_iio_info = {
+	.read_raw = &rn5t618_adc_read,
+};
+
+#define RN5T618_ADC_CHANNEL(_channel, _type, _name) { \
+	.type = _type, \
+	.channel = _channel, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			      BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
+			      BIT(IIO_CHAN_INFO_PROCESSED), \
+	.datasheet_name = _name, \
+	.indexed = 1. \
+}
+
+static const struct iio_chan_spec rn5t618_adc_iio_channels[] = {
+	RN5T618_ADC_CHANNEL(0, IIO_CURRENT, "LIMMON"),
+	RN5T618_ADC_CHANNEL(1, IIO_VOLTAGE, "VBAT"),
+	RN5T618_ADC_CHANNEL(2, IIO_VOLTAGE, "VADP"),
+	RN5T618_ADC_CHANNEL(3, IIO_VOLTAGE, "VUSB"),
+	RN5T618_ADC_CHANNEL(4, IIO_VOLTAGE, "VSYS"),
+	RN5T618_ADC_CHANNEL(5, IIO_VOLTAGE, "VTHM"),
+	RN5T618_ADC_CHANNEL(6, IIO_VOLTAGE, "AIN1"),
+	RN5T618_ADC_CHANNEL(7, IIO_VOLTAGE, "AIN0")
+};
+
+static int rn5t618_adc_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct iio_dev *iio_dev;
+	struct rn5t618_adc_data *adc;
+	struct rn5t618 *rn5t618 = dev_get_drvdata(pdev->dev.parent);
+
+	iio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc));
+	if (!iio_dev) {
+		dev_err(&pdev->dev, "failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	adc = iio_priv(iio_dev);
+	adc->dev = &pdev->dev;
+	adc->rn5t618 = rn5t618;
+	adc->irq = -ENOENT;
+
+	if (rn5t618->irq_data)
+		adc->irq = regmap_irq_get_virq(rn5t618->irq_data,
+					       RN5T618_IRQ_ADC);
+
+	if (adc->irq  < 0) {
+		dev_err(&pdev->dev, "get virq failed\n");
+		return adc->irq;
+	}
+
+	init_completion(&adc->conv_completion);
+
+	iio_dev->name = dev_name(&pdev->dev);
+	iio_dev->dev.parent = &pdev->dev;
+	iio_dev->info = &rn5t618_adc_iio_info;
+	iio_dev->modes = INDIO_DIRECT_MODE;
+	iio_dev->channels = rn5t618_adc_iio_channels;
+	iio_dev->num_channels = ARRAY_SIZE(rn5t618_adc_iio_channels);
+
+	/* stop any auto-conversion */
+	ret = regmap_write(rn5t618->regmap, RN5T618_ADCCNT3, 0);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, iio_dev);
+
+	ret = request_threaded_irq(adc->irq, NULL,
+				   rn5t618_adc_irq,
+				   IRQF_ONESHOT, dev_name(adc->dev),
+				   adc);
+	if (ret < 0) {
+		dev_err(adc->dev, "request irq %d failed: %d\n", adc->irq, ret);
+		return ret;
+	}
+
+	ret = iio_device_register(iio_dev);
+	if (ret < 0)
+		free_irq(adc->irq, adc);
+
+	return ret;
+}
+
+static int rn5t618_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *iio_dev = platform_get_drvdata(pdev);
+	struct rn5t618_adc_data *adc = iio_priv(iio_dev);
+
+	iio_device_unregister(iio_dev);
+	free_irq(adc->irq, adc);
+
+	return 0;
+}
+
+static struct platform_driver rn5t618_adc_driver = {
+	.driver = {
+		.name   = "rn5t618-adc",
+	},
+	.probe = rn5t618_adc_probe,
+	.remove = rn5t618_adc_remove,
+};
+
+module_platform_driver(rn5t618_adc_driver);
+MODULE_ALIAS("platform:rn5t618-adc");
+MODULE_DESCRIPTION("RICOH RN5T618 ADC driver");
+MODULE_LICENSE("GPL");
+
-- 
2.20.1

[RFC PATCH 4/5] mfd: rn5t618: add IRQ definitions for RN5T618

[Andreas Kemnade]

Previously, only the definitions for the RC5T619 were
available.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Untested, IMHO only acceptable with a Tested-By

 drivers/mfd/rn5t618.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index 321836f78120..6828fd40b0a1 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -58,6 +58,24 @@ static const struct regmap_config rn5t618_regmap_config = {
 	.cache_type	= REGCACHE_RBTREE,
 };
 
+static const struct regmap_irq rn5t618_irqs[] = {
+	REGMAP_IRQ_REG(RN5T618_IRQ_SYS, 0, BIT(0)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_DCDC, 0, BIT(1)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_ADC, 0, BIT(3)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_GPIO, 0, BIT(4)),
+	REGMAP_IRQ_REG(RN5T618_IRQ_CHG, 0, BIT(6)),
+};
+
+static const struct regmap_irq_chip rn5t618_irq_chip = {
+	.name = "rn5t618",
+	.irqs = rn5t618_irqs,
+	.num_irqs = ARRAY_SIZE(rn5t618_irqs),
+	.num_regs = 1,
+	.status_base = RN5T618_INTMON,
+	.mask_base = RN5T618_INTEN,
+	.mask_invert = true,
+};
+
 static const struct regmap_irq rc5t619_irqs[] = {
 	REGMAP_IRQ_REG(RN5T618_IRQ_SYS, 0, BIT(0)),
 	REGMAP_IRQ_REG(RN5T618_IRQ_DCDC, 0, BIT(1)),
@@ -92,6 +110,9 @@ static int rn5t618_irq_init(struct rn5t618 *rn5t618)
 	case RC5T619:
 		irq_chip = &rc5t619_irq_chip;
 		break;
+	case RN5T618:
+		irq_chip = &rn5t618_irq_chip;
+		break;
 	default:
 		irq_chip = NULL;
 		break;
-- 
2.20.1

[RFC PATCH 5/5] mfd: rn5t618: add ADC subdevice for RN5T618

[Andreas Kemnade]

RN5T618 has an ADC but RN5T567 has not, so
we need separate subdevice lists for both.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Untested, IMHO only acceptable with a Tested-By
 drivers/mfd/rn5t618.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c
index 6828fd40b0a1..d37d7a31cf26 100644
--- a/drivers/mfd/rn5t618.c
+++ b/drivers/mfd/rn5t618.c
@@ -21,6 +21,7 @@
 static const struct mfd_cell rn5t618_cells[] = {
 	{ .name = "rn5t618-regulator" },
 	{ .name = "rn5t618-wdt" },
+	{ .name = "rn5t618-adc" },
 };
 
 static const struct mfd_cell rc5t619_cells[] = {
@@ -30,6 +31,11 @@ static const struct mfd_cell rc5t619_cells[] = {
 	{ .name = "rn5t618-wdt" },
 };
 
+static const struct mfd_cell rn5t567_cells[] = {
+	{ .name = "rn5t618-regulator" },
+	{ .name = "rn5t618-wdt" },
+};
+
 static bool rn5t618_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -203,16 +209,32 @@ static int rn5t618_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	if (priv->variant == RC5T619)
+	switch (priv->variant) {
+	case RC5T619:
 		ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_NONE,
 					   rc5t619_cells,
 					   ARRAY_SIZE(rc5t619_cells),
 					   NULL, 0, NULL);
-	else
+		break;
+	case RN5T618:
 		ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_NONE,
 					   rn5t618_cells,
 					   ARRAY_SIZE(rn5t618_cells),
 					   NULL, 0, NULL);
+		break;
+	case RN5T567:
+		ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_NONE,
+					   rn5t567_cells,
+					   ARRAY_SIZE(rn5t567_cells),
+					   NULL, 0, NULL);
+		break;
+	/*
+	 * Should not happen because we come here only with a valid device
+	 * tree match, so variant contains any of the above.
+	 */
+	default:
+		return -ENOENT;
+	}
 	if (ret) {
 		dev_err(&i2c->dev, "failed to add sub-devices: %d\n", ret);
 		return ret;
-- 
2.20.1

Re: [PATCH 3/5] iio: adc: rn5t618: Add ADC driver for RN5T618/RC5T619

[Jonathan Cameron]

On Fri, 17 Jan 2020 22:59:24 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> Both chips have an A/D converter capable of measuring
> things like VBAT, VUSB and analog inputs.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
A few comments inline, but looks pretty good on the whole.

Jonathan

> ---
>  drivers/iio/adc/Kconfig       |  10 ++
>  drivers/iio/adc/Makefile      |   1 +
>  drivers/iio/adc/rn5t618-adc.c | 266 ++++++++++++++++++++++++++++++++++
>  3 files changed, 277 insertions(+)
>  create mode 100644 drivers/iio/adc/rn5t618-adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f0af3a42f53c..9ea9489e3f0a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -735,6 +735,16 @@ config RCAR_GYRO_ADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called rcar-gyroadc.
>  
> +config RN5T618_ADC
> +	tristate "ADC for the RN5T618/RC5T619 family of chips"
> +	depends on MFD_RN5T618
> +	help
> +	  Say yes here to build support for the integrated ADC inside the
> +	  RN5T618/619 series PMICs:
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rn5t618-adc.
> +
>  config ROCKCHIP_SARADC
>  	tristate "Rockchip SARADC driver"
>  	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ef9cc485fb67..2aea70556ed0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o
>  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_RN5T618_ADC) += rn5t618-adc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
>  obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
> diff --git a/drivers/iio/adc/rn5t618-adc.c b/drivers/iio/adc/rn5t618-adc.c
> new file mode 100644
> index 000000000000..81f872a7ad7f
> --- /dev/null
> +++ b/drivers/iio/adc/rn5t618-adc.c
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ADC driver for the RICOH RN5T618 power management chip family
> + *
> + * Copyright (C) 2019 Andreas Kemnade
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mfd/rn5t618.h>
> +#include <linux/platform_device.h>
> +#include <linux/completion.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/slab.h>
> +#include <linux/irqdomain.h>
> +
> +#define RN5T618_ADC_CONVERSION_TIMEOUT   (msecs_to_jiffies(500))
> +#define REFERENCE_VOLT 2500
> +
> +/* mask for selecting channels for single conversion */
> +#define ADCCNT3_CHANNEL_MASK 0x7
> +/* average 4-time conversion mode */
> +#define ADCCNT3_AVG BIT(3)
> +/* set for starting a single conversion, gets cleared by hw when done */
> +#define ADCCNT3_GODONE BIT(4)
> +/* automatic conversion, period is in ADCCNT2, selected channels are
> + * in ADCCNT1
> + */
> +#define ADCCNT3_AUTO BIT(5)
> +#define ADCEND_IRQ BIT(0)
> +
> +struct rn5t618_adc_data {
> +	struct device *dev;
> +	struct rn5t618 *rn5t618;
> +	struct completion conv_completion;
> +	int irq;
> +};
> +
> +struct rn5t618_channel_ratios {
> +	u16 numerator;
> +	u16 denominator;
> +};
> +
> +static const struct rn5t618_channel_ratios rn5t618_ratios[8] = {
> +	{50, 32}, /* LIMMON measured across 20mOhm, amplified by 32 */
Define an enum for the channel.  Then you can use explicit element
setting to make this code self docuemnting.

[LIMMON] = {50, 32},
[VBAT] = {2, 1}, etc.

Use the enum to fill in the channel numbers below as well and
it all becomes 'obviously' correct rather than having to check the
two things are in the same order.

> +	{2, 1}, /* VBAT */
> +	{3, 1}, /* VADP */
> +	{3, 1}, /* VUSB */
> +	{3, 1}, /* VSYS */
> +	{1, 1}, /* VTHM */
> +	{1, 1}, /* AIN1 */
> +	{1, 1}, /* AIN0 */
> +};
> +
> +static int rn5t618_read_adc_reg(struct rn5t618 *rn5t618, int reg, u16 *val)
> +{
> +	unsigned int h;
> +	unsigned int l;
> +	int ret;
> +
> +	ret = regmap_read(rn5t618->regmap, reg, &h);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read(rn5t618->regmap, reg + 1, &l);
> +	if (ret < 0)
> +		return ret;

regmap_bulk_read perhaps? 

> +
> +	h <<= 4;
> +	h |= (l & 0xF);
> +	h &= 0xFFF;

I'd mask h before the shift.  More readable I think than
masking the l part twice.

> +	*val = h;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t rn5t618_adc_irq(int irq, void *data)
> +{
> +	struct rn5t618_adc_data *adc = data;
> +	unsigned int r = 0;
> +	int ret;
> +
> +	/* clear low & high threshold irqs */
> +	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC1, 0);
> +	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC2, 0);
> +
> +	ret = regmap_read(adc->rn5t618->regmap, RN5T618_IR_ADC3, &r);
> +	if (ret < 0)
> +		dev_err(adc->dev, "failed to read IRQ status: %d\n", ret);
> +
> +	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC3, 0);
> +
> +	if (r & ADCEND_IRQ)
> +		complete(&adc->conv_completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int rn5t618_adc_read(struct iio_dev *iio_dev,
> +			    const struct iio_chan_spec *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct rn5t618_adc_data *adc = iio_priv(iio_dev);
> +	u16 raw;
> +	int ret;
> +
> +	/* select channel */
> +	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
> +				 ADCCNT3_CHANNEL_MASK,
> +				 chan->channel);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(adc->rn5t618->regmap, RN5T618_EN_ADCIR3, ADCEND_IRQ);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
> +				 ADCCNT3_AVG,
> +				 mask == IIO_CHAN_INFO_AVERAGE_RAW ?
> +				 ADCCNT3_AVG : 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	init_completion(&adc->conv_completion);
> +	/* single conversion */
> +	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
> +				 ADCCNT3_GODONE, ADCCNT3_GODONE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = wait_for_completion_timeout(&adc->conv_completion,
> +					  RN5T618_ADC_CONVERSION_TIMEOUT);
> +	if (ret == 0) {
> +		dev_warn(adc->dev, "timeout waiting for adc result\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = rn5t618_read_adc_reg(adc->rn5t618,
> +				   RN5T618_ILIMDATAH + 2 * chan->channel,
> +				   &raw);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = raw;
> +	if (mask == IIO_CHAN_INFO_PROCESSED)
> +		*val = *val * REFERENCE_VOLT *
> +		       rn5t618_ratios[chan->channel].numerator /
> +		       rn5t618_ratios[chan->channel].denominator / 4095;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info rn5t618_adc_iio_info = {
> +	.read_raw = &rn5t618_adc_read,
> +};
> +
> +#define RN5T618_ADC_CHANNEL(_channel, _type, _name) { \
> +	.type = _type, \
> +	.channel = _channel, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
> +			      BIT(IIO_CHAN_INFO_PROCESSED), \
> +	.datasheet_name = _name, \
> +	.indexed = 1. \
> +}
> +
> +static const struct iio_chan_spec rn5t618_adc_iio_channels[] = {
> +	RN5T618_ADC_CHANNEL(0, IIO_CURRENT, "LIMMON"),
> +	RN5T618_ADC_CHANNEL(1, IIO_VOLTAGE, "VBAT"),
> +	RN5T618_ADC_CHANNEL(2, IIO_VOLTAGE, "VADP"),
> +	RN5T618_ADC_CHANNEL(3, IIO_VOLTAGE, "VUSB"),
> +	RN5T618_ADC_CHANNEL(4, IIO_VOLTAGE, "VSYS"),
> +	RN5T618_ADC_CHANNEL(5, IIO_VOLTAGE, "VTHM"),
> +	RN5T618_ADC_CHANNEL(6, IIO_VOLTAGE, "AIN1"),
> +	RN5T618_ADC_CHANNEL(7, IIO_VOLTAGE, "AIN0")
> +};
> +
> +static int rn5t618_adc_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct iio_dev *iio_dev;
> +	struct rn5t618_adc_data *adc;
> +	struct rn5t618 *rn5t618 = dev_get_drvdata(pdev->dev.parent);
> +
> +	iio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc));
> +	if (!iio_dev) {
> +		dev_err(&pdev->dev, "failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	adc = iio_priv(iio_dev);
> +	adc->dev = &pdev->dev;
> +	adc->rn5t618 = rn5t618;
> +	adc->irq = -ENOENT;
> +
> +	if (rn5t618->irq_data)
> +		adc->irq = regmap_irq_get_virq(rn5t618->irq_data,
> +					       RN5T618_IRQ_ADC);
> +
> +	if (adc->irq  < 0) {

Extra space before the <
For an irq 0 usually counts as 'no irq'.  Can that particular
path ever give that?

> +		dev_err(&pdev->dev, "get virq failed\n");
> +		return adc->irq;
> +	}
> +
> +	init_completion(&adc->conv_completion);
> +
> +	iio_dev->name = dev_name(&pdev->dev);
> +	iio_dev->dev.parent = &pdev->dev;
> +	iio_dev->info = &rn5t618_adc_iio_info;
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +	iio_dev->channels = rn5t618_adc_iio_channels;
> +	iio_dev->num_channels = ARRAY_SIZE(rn5t618_adc_iio_channels);
> +
> +	/* stop any auto-conversion */
> +	ret = regmap_write(rn5t618->regmap, RN5T618_ADCCNT3, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, iio_dev);
> +
> +	ret = request_threaded_irq(adc->irq, NULL,
> +				   rn5t618_adc_irq,
> +				   IRQF_ONESHOT, dev_name(adc->dev),
> +				   adc);
> +	if (ret < 0) {
> +		dev_err(adc->dev, "request irq %d failed: %d\n", adc->irq, ret);
> +		return ret;
> +	}
> +
> +	ret = iio_device_register(iio_dev);
> +	if (ret < 0)
> +		free_irq(adc->irq, adc);
> +
> +	return ret;
> +}
> +
> +static int rn5t618_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *iio_dev = platform_get_drvdata(pdev);
> +	struct rn5t618_adc_data *adc = iio_priv(iio_dev);
> +
> +	iio_device_unregister(iio_dev);
> +	free_irq(adc->irq, adc);

If this is all we are going to have in remove, why not just use
the device managed form to do it for us and get rid of remove.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver rn5t618_adc_driver = {
> +	.driver = {
> +		.name   = "rn5t618-adc",
> +	},
> +	.probe = rn5t618_adc_probe,
> +	.remove = rn5t618_adc_remove,
> +};
> +
> +module_platform_driver(rn5t618_adc_driver);
> +MODULE_ALIAS("platform:rn5t618-adc");
> +MODULE_DESCRIPTION("RICOH RN5T618 ADC driver");
> +MODULE_LICENSE("GPL");
> +

Re: [PATCH 3/5] iio: adc: rn5t618: Add ADC driver for RN5T618/RC5T619

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 11429 bytes --]

On Sat, 18 Jan 2020 14:53:18 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 17 Jan 2020 22:59:24 +0100
> Andreas Kemnade <andreas@kemnade.info> wrote:
> 
> > Both chips have an A/D converter capable of measuring
> > things like VBAT, VUSB and analog inputs.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>  
> A few comments inline, but looks pretty good on the whole.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/Kconfig       |  10 ++
> >  drivers/iio/adc/Makefile      |   1 +
> >  drivers/iio/adc/rn5t618-adc.c | 266 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 277 insertions(+)
> >  create mode 100644 drivers/iio/adc/rn5t618-adc.c
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index f0af3a42f53c..9ea9489e3f0a 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -735,6 +735,16 @@ config RCAR_GYRO_ADC
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called rcar-gyroadc.
> >  
> > +config RN5T618_ADC
> > +	tristate "ADC for the RN5T618/RC5T619 family of chips"
> > +	depends on MFD_RN5T618
> > +	help
> > +	  Say yes here to build support for the integrated ADC inside the
> > +	  RN5T618/619 series PMICs:
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called rn5t618-adc.
> > +
> >  config ROCKCHIP_SARADC
> >  	tristate "Rockchip SARADC driver"
> >  	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index ef9cc485fb67..2aea70556ed0 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -69,6 +69,7 @@ obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o
> >  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_RN5T618_ADC) += rn5t618-adc.o
> >  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> >  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
> >  obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
> > diff --git a/drivers/iio/adc/rn5t618-adc.c b/drivers/iio/adc/rn5t618-adc.c
> > new file mode 100644
> > index 000000000000..81f872a7ad7f
> > --- /dev/null
> > +++ b/drivers/iio/adc/rn5t618-adc.c
> > @@ -0,0 +1,266 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * ADC driver for the RICOH RN5T618 power management chip family
> > + *
> > + * Copyright (C) 2019 Andreas Kemnade
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/mfd/rn5t618.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/completion.h>
> > +#include <linux/regmap.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/slab.h>
> > +#include <linux/irqdomain.h>
> > +
> > +#define RN5T618_ADC_CONVERSION_TIMEOUT   (msecs_to_jiffies(500))
> > +#define REFERENCE_VOLT 2500
> > +
> > +/* mask for selecting channels for single conversion */
> > +#define ADCCNT3_CHANNEL_MASK 0x7
> > +/* average 4-time conversion mode */
> > +#define ADCCNT3_AVG BIT(3)
> > +/* set for starting a single conversion, gets cleared by hw when done */
> > +#define ADCCNT3_GODONE BIT(4)
> > +/* automatic conversion, period is in ADCCNT2, selected channels are
> > + * in ADCCNT1
> > + */
> > +#define ADCCNT3_AUTO BIT(5)
> > +#define ADCEND_IRQ BIT(0)
> > +
> > +struct rn5t618_adc_data {
> > +	struct device *dev;
> > +	struct rn5t618 *rn5t618;
> > +	struct completion conv_completion;
> > +	int irq;
> > +};
> > +
> > +struct rn5t618_channel_ratios {
> > +	u16 numerator;
> > +	u16 denominator;
> > +};
> > +
> > +static const struct rn5t618_channel_ratios rn5t618_ratios[8] = {
> > +	{50, 32}, /* LIMMON measured across 20mOhm, amplified by 32 */  
> Define an enum for the channel.  Then you can use explicit element
> setting to make this code self docuemnting.
> 
> [LIMMON] = {50, 32},
> [VBAT] = {2, 1}, etc.
> 
> Use the enum to fill in the channel numbers below as well and
> it all becomes 'obviously' correct rather than having to check the
> two things are in the same order.
> 
oh, yes that makes sense.

> > +	{2, 1}, /* VBAT */
> > +	{3, 1}, /* VADP */
> > +	{3, 1}, /* VUSB */
> > +	{3, 1}, /* VSYS */
> > +	{1, 1}, /* VTHM */
> > +	{1, 1}, /* AIN1 */
> > +	{1, 1}, /* AIN0 */
> > +};
> > +
> > +static int rn5t618_read_adc_reg(struct rn5t618 *rn5t618, int reg, u16 *val)
> > +{
> > +	unsigned int h;
> > +	unsigned int l;
> > +	int ret;
> > +
> > +	ret = regmap_read(rn5t618->regmap, reg, &h);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_read(rn5t618->regmap, reg + 1, &l);
> > +	if (ret < 0)
> > +		return ret;  
> 
> regmap_bulk_read perhaps? 
> 
ok, can do that.
> > +
> > +	h <<= 4;
> > +	h |= (l & 0xF);
> > +	h &= 0xFFF;  
> 
> I'd mask h before the shift.  More readable I think than
> masking the l part twice.
> 
or simply don't mask it at all. It is an 8 bit register, so masking it with
0xff does not make sense at all.

> > +	*val = h;
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t rn5t618_adc_irq(int irq, void *data)
> > +{
> > +	struct rn5t618_adc_data *adc = data;
> > +	unsigned int r = 0;
> > +	int ret;
> > +
> > +	/* clear low & high threshold irqs */
> > +	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC1, 0);
> > +	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC2, 0);
> > +
> > +	ret = regmap_read(adc->rn5t618->regmap, RN5T618_IR_ADC3, &r);
> > +	if (ret < 0)
> > +		dev_err(adc->dev, "failed to read IRQ status: %d\n", ret);
> > +
> > +	regmap_write(adc->rn5t618->regmap, RN5T618_IR_ADC3, 0);
> > +
> > +	if (r & ADCEND_IRQ)
> > +		complete(&adc->conv_completion);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int rn5t618_adc_read(struct iio_dev *iio_dev,
> > +			    const struct iio_chan_spec *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct rn5t618_adc_data *adc = iio_priv(iio_dev);
> > +	u16 raw;
> > +	int ret;
> > +
> > +	/* select channel */
> > +	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
> > +				 ADCCNT3_CHANNEL_MASK,
> > +				 chan->channel);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_write(adc->rn5t618->regmap, RN5T618_EN_ADCIR3, ADCEND_IRQ);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
> > +				 ADCCNT3_AVG,
> > +				 mask == IIO_CHAN_INFO_AVERAGE_RAW ?
> > +				 ADCCNT3_AVG : 0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	init_completion(&adc->conv_completion);
> > +	/* single conversion */
> > +	ret = regmap_update_bits(adc->rn5t618->regmap, RN5T618_ADCCNT3,
> > +				 ADCCNT3_GODONE, ADCCNT3_GODONE);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = wait_for_completion_timeout(&adc->conv_completion,
> > +					  RN5T618_ADC_CONVERSION_TIMEOUT);
> > +	if (ret == 0) {
> > +		dev_warn(adc->dev, "timeout waiting for adc result\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	ret = rn5t618_read_adc_reg(adc->rn5t618,
> > +				   RN5T618_ILIMDATAH + 2 * chan->channel,
> > +				   &raw);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = raw;
> > +	if (mask == IIO_CHAN_INFO_PROCESSED)
> > +		*val = *val * REFERENCE_VOLT *
> > +		       rn5t618_ratios[chan->channel].numerator /
> > +		       rn5t618_ratios[chan->channel].denominator / 4095;
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static const struct iio_info rn5t618_adc_iio_info = {
> > +	.read_raw = &rn5t618_adc_read,
> > +};
> > +
> > +#define RN5T618_ADC_CHANNEL(_channel, _type, _name) { \
> > +	.type = _type, \
> > +	.channel = _channel, \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > +			      BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
> > +			      BIT(IIO_CHAN_INFO_PROCESSED), \
> > +	.datasheet_name = _name, \
> > +	.indexed = 1. \
> > +}
> > +
> > +static const struct iio_chan_spec rn5t618_adc_iio_channels[] = {
> > +	RN5T618_ADC_CHANNEL(0, IIO_CURRENT, "LIMMON"),
> > +	RN5T618_ADC_CHANNEL(1, IIO_VOLTAGE, "VBAT"),
> > +	RN5T618_ADC_CHANNEL(2, IIO_VOLTAGE, "VADP"),
> > +	RN5T618_ADC_CHANNEL(3, IIO_VOLTAGE, "VUSB"),
> > +	RN5T618_ADC_CHANNEL(4, IIO_VOLTAGE, "VSYS"),
> > +	RN5T618_ADC_CHANNEL(5, IIO_VOLTAGE, "VTHM"),
> > +	RN5T618_ADC_CHANNEL(6, IIO_VOLTAGE, "AIN1"),
> > +	RN5T618_ADC_CHANNEL(7, IIO_VOLTAGE, "AIN0")
> > +};
> > +
> > +static int rn5t618_adc_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +	struct iio_dev *iio_dev;
> > +	struct rn5t618_adc_data *adc;
> > +	struct rn5t618 *rn5t618 = dev_get_drvdata(pdev->dev.parent);
> > +
> > +	iio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc));
> > +	if (!iio_dev) {
> > +		dev_err(&pdev->dev, "failed allocating iio device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	adc = iio_priv(iio_dev);
> > +	adc->dev = &pdev->dev;
> > +	adc->rn5t618 = rn5t618;
> > +	adc->irq = -ENOENT;
> > +
> > +	if (rn5t618->irq_data)
> > +		adc->irq = regmap_irq_get_virq(rn5t618->irq_data,
> > +					       RN5T618_IRQ_ADC);
> > +
> > +	if (adc->irq  < 0) {  
> 
> Extra space before the <
> For an irq 0 usually counts as 'no irq'.  Can that particular
> path ever give that?
> 
hmm, regmap_irq_get_virq seems to return < 0 or 0 on error depending
on situation. Unless there is a bug in the mfd parent, it should not fail.
But maybe better catch that. If the mfd parent itself does not have an IRQ
irq_data will also be NULL.
BTW: palmas_gpadc.c does the same strange check, so maybe it should be fixed
there too.

> > +		dev_err(&pdev->dev, "get virq failed\n");
> > +		return adc->irq;
> > +	}
> > +
> > +	init_completion(&adc->conv_completion);
> > +
> > +	iio_dev->name = dev_name(&pdev->dev);
> > +	iio_dev->dev.parent = &pdev->dev;
> > +	iio_dev->info = &rn5t618_adc_iio_info;
> > +	iio_dev->modes = INDIO_DIRECT_MODE;
> > +	iio_dev->channels = rn5t618_adc_iio_channels;
> > +	iio_dev->num_channels = ARRAY_SIZE(rn5t618_adc_iio_channels);
> > +
> > +	/* stop any auto-conversion */
> > +	ret = regmap_write(rn5t618->regmap, RN5T618_ADCCNT3, 0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, iio_dev);
> > +
> > +	ret = request_threaded_irq(adc->irq, NULL,
> > +				   rn5t618_adc_irq,
> > +				   IRQF_ONESHOT, dev_name(adc->dev),
> > +				   adc);
> > +	if (ret < 0) {
> > +		dev_err(adc->dev, "request irq %d failed: %d\n", adc->irq, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = iio_device_register(iio_dev);
> > +	if (ret < 0)
> > +		free_irq(adc->irq, adc);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rn5t618_adc_remove(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *iio_dev = platform_get_drvdata(pdev);
> > +	struct rn5t618_adc_data *adc = iio_priv(iio_dev);
> > +
> > +	iio_device_unregister(iio_dev);
> > +	free_irq(adc->irq, adc);  
> 
> If this is all we are going to have in remove, why not just use
> the device managed form to do it for us and get rid of remove.
> 
yes, should be a very good idea.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/5] mfd: rn5t618: mark ADC control register volatile

[Lee Jones]

On Fri, 17 Jan 2020, Andreas Kemnade wrote:

> There is a bit which gets cleared after conversion.
> 
> Fixes: 9bb9e29c78f8 ("mfd: Add Ricoh RN5T618 PMIC core driver")
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/mfd/rn5t618.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/5] mfd: rn5t618: add ADC subdevice for RC5T619

[Lee Jones]

On Fri, 17 Jan 2020, Andreas Kemnade wrote:

> This adds a subdevice for the ADC in the RC5T619.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/mfd/rn5t618.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/5] mfd: rn5t618: add ADC subdevice for RC5T619

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

Hi,

hmm, I cannot find this in any branch/repo I know of and not in linux-next,
just wondering...
I guess the iio part is something to go towards 5.7 unless 5.5
is delayed mucch.

Regards,
Andreas

On Mon, 20 Jan 2020 08:49:34 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Fri, 17 Jan 2020, Andreas Kemnade wrote:
> 
> > This adds a subdevice for the ADC in the RC5T619.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  drivers/mfd/rn5t618.c | 1 +
> >  1 file changed, 1 insertion(+)  
> 
> Applied, thanks.
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] mfd: rn5t618: add ADC subdevice for RC5T619

[Lee Jones]

On Fri, 24 Jan 2020, Andreas Kemnade wrote:
> hmm, I cannot find this in any branch/repo I know of and not in linux-next,
> just wondering...
> I guess the iio part is something to go towards 5.7 unless 5.5
> is delayed mucch.

Oh, it looks like there was a conflict.  Could you collect any Acks
(including mine) rebase and resend please?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/5] mfd: rn5t618: add ADC subdevice for RC5T619

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Mon, 27 Jan 2020 09:28:15 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Fri, 24 Jan 2020, Andreas Kemnade wrote:
> > hmm, I cannot find this in any branch/repo I know of and not in linux-next,
> > just wondering...
> > I guess the iio part is something to go towards 5.7 unless 5.5
> > is delayed mucch.  
> 
> Oh, it looks like there was a conflict.  Could you collect any Acks
> (including mine) rebase and resend please?
> 
on what? It is based on top of my RTC v5 patch series. The ADC series
as a whole depends on that build-time and runtime as documented in
the cover letter. Maybe I should have put it into the patches itself.
I have hopefully addressed every comment in the v5 series.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]