Re: [PATCH v2 2/2] iio: adc: Add a driver for the Ingenic X1600 ADC

[Joshua Crofts <joshua.crofts1@gmail.com>]

On Sat, 24 Oct 2026 06:13:57 +0200
Olivier Louvignes <olivier@mgcrea.io> wrote:

> diff --git a/MAINTAINERS b/MAINTAINERS
> index ecc22d1149d1..bbb63228e330 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12925,6 +12925,13 @@ F:	include/linux/mfd/ingenic-tcu.h
>  F:	sound/soc/codecs/jz47*
>  F:	sound/soc/jz4740/
>  
> +INGENIC X1600 ADC DRIVER
> +M:	Olivier Louvignes <olivier@mgcrea.io>
> +L:	linux-iio@vger.kernel.org

Drop the list, get_maintainer gets this via the directory.

> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml

The changes above should go into patch 1 (checkpatch will
raise a warning if you're creating a new file and it's not
documented in MAINTAINERS).

> +F:	drivers/iio/adc/ingenic-x1600-adc.c

And this addition goes into patch 2.

...

> diff --git a/drivers/iio/adc/ingenic-x1600-adc.c b/drivers/iio/adc/ingenic-x1600-adc.c
> new file mode 100644
> index 000000000000..60683f26c693
> --- /dev/null
> +++ b/drivers/iio/adc/ingenic-x1600-adc.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADC driver for the Ingenic X1600 SoC
> + *

+ Copyright (c) 2026 YOUR_NAME <YOUR_EMAIL>

> + * The X1600's SAR ADC has four auxiliary channels, each with its own start
> + * bit, interrupt and 12-bit result. The register layout and the clock set-up
> + * were read from the vendor 4.4 kernel's driver.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>

+ bits.h, types.h, array_size.h, cleanup.h, jiffies.h, minmax.h

> +
> +#define X1600_ADC_REG_ENABLE		0x00
> +#define X1600_ADC_REG_CTRL		0x08
> +#define X1600_ADC_REG_STATUS		0x0c
> +#define X1600_ADC_REG_DATA(ch)		(0x10 + ((ch) / 2) * 4)
> +#define X1600_ADC_REG_CLKDIV		0x20
> +#define X1600_ADC_REG_STABLE		0x24
> +
> +/* ENABLE: bits 3..0 start a channel's conversion; bit 15 powers down. */
> +#define X1600_ADC_ENABLE_POWER_DOWN	BIT(15)
> +/* CTRL: bits 3..0 mask the channels' interrupts. STATUS: write 1 to clear. */
> +#define X1600_ADC_CHANNELS_MASK		GENMASK(3, 0)
> +#define X1600_ADC_DATA_MASK(ch)		(GENMASK(11, 0) << ((ch) % 2 * 16))
> +#define X1600_ADC_CLKDIV_DIV0		GENMASK(7, 0)
> +#define X1600_ADC_CLKDIV_DIV1		GENMASK(15, 8)
> +#define X1600_ADC_CLKDIV_DIV2		GENMASK(31, 16)
> +#define X1600_ADC_STABLE_TIME		GENMASK(15, 0)
> +
> +#define X1600_ADC_NUM_CHANNELS		4
> +#define X1600_ADC_VREF_mV		3300
> +#define X1600_ADC_BITS			12
> +/* The vendor kernel brings its 140 MHz APB clock down to about this. */
> +#define X1600_ADC_CLK_RATE		1166667
> +#define X1600_ADC_TIMEOUT_MS		20
> +
> +struct x1600_adc {
> +	void __iomem *base;
> +	struct clk *clk;
> +	struct completion done;
> +	/* Serialises conversions: there is one completion for all channels. */
> +	struct mutex lock;
> +};
> +
> +static irqreturn_t x1600_adc_irq(int irq, void *data)
> +{
> +	struct x1600_adc *adc = data;
> +	u32 status;
> +
> +	status = readl(adc->base + X1600_ADC_REG_STATUS) & X1600_ADC_CHANNELS_MASK;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	writel(status, adc->base + X1600_ADC_REG_STATUS);
> +	complete(&adc->done);
> +	return IRQ_HANDLED;
> +}
> +
> +static int x1600_adc_read_raw_channel(struct x1600_adc *adc, unsigned int ch, int *val)
> +{
> +	u32 data;
> +	int ret = 0;

Reverse xmas tree.

> +
> +	guard(mutex)(&adc->lock);
> +
> +	reinit_completion(&adc->done);
> +	writel(BIT(ch), adc->base + X1600_ADC_REG_STATUS);
> +	writel(readl(adc->base + X1600_ADC_REG_ENABLE) | BIT(ch),
> +	       adc->base + X1600_ADC_REG_ENABLE);
> +
> +	if (!wait_for_completion_timeout(&adc->done,
> +					 msecs_to_jiffies(X1600_ADC_TIMEOUT_MS)))
> +		ret = -ETIMEDOUT;
> +
> +	writel(readl(adc->base + X1600_ADC_REG_ENABLE) & ~BIT(ch),
> +	       adc->base + X1600_ADC_REG_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	data = readl(adc->base + X1600_ADC_REG_DATA(ch));
> +	*val = field_get(X1600_ADC_DATA_MASK(ch), data);
> +	return 0;
> +}
> +
> +static int x1600_adc_read_raw(struct iio_dev *iio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	struct x1600_adc *adc = iio_priv(iio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = x1600_adc_read_raw_channel(adc, chan->channel, val);
> +		return ret ? ret : IIO_VAL_INT;

You can use the Elvis operator here.

> +	case IIO_CHAN_INFO_SCALE:
> +		*val = X1600_ADC_VREF_mV;
> +		*val2 = X1600_ADC_BITS;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info x1600_adc_info = {
> +	.read_raw = x1600_adc_read_raw,
> +};
> +
> +#define X1600_ADC_CHANNEL(ch) {					\

Small nit, but align the \ with the others.

> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = (ch),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +}
> +
x
...

> +	/* Powered down, interrupts masked and cleared, as the vendor does. */
> +	writel(X1600_ADC_ENABLE_POWER_DOWN, adc->base + X1600_ADC_REG_ENABLE);
> +	writel(X1600_ADC_CHANNELS_MASK, adc->base + X1600_ADC_REG_CTRL);
> +	writel(X1600_ADC_CHANNELS_MASK, adc->base + X1600_ADC_REG_STATUS);
> +
> +	rate = clk_get_rate(adc->clk);
> +	div0 = clamp(DIV_ROUND_UP(rate, X1600_ADC_CLK_RATE), 1UL, 256UL) - 1;
> +	/* DIV1 and DIV2 as the vendor kernel sets them. */
> +	writel(FIELD_PREP(X1600_ADC_CLKDIV_DIV0, div0) |
> +	       FIELD_PREP(X1600_ADC_CLKDIV_DIV1, 1) |
> +	       FIELD_PREP(X1600_ADC_CLKDIV_DIV2, 99),
> +	       adc->base + X1600_ADC_REG_CLKDIV);
> +	reg = readl(adc->base + X1600_ADC_REG_STABLE) & ~X1600_ADC_STABLE_TIME;
> +	writel(reg | FIELD_PREP(X1600_ADC_STABLE_TIME, 1),
> +	       adc->base + X1600_ADC_REG_STABLE);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(dev, irq, x1600_adc_irq, 0, dev_name(dev), adc);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request irq\n");

devm_request_irq() already prints an error message on failure, just
"return ret;" is fine.

> +
> +	/*
> +	 * Powered up for good: the keys poll it every few tens of milliseconds,
> +	 * and powering up takes 2 ms.
> +	 */
> +	writel(0, adc->base + X1600_ADC_REG_ENABLE);
> +	fsleep(2000);
> +	writel(0, adc->base + X1600_ADC_REG_CTRL);

+ blank line.

> +	ret = devm_add_action_or_reset(dev, x1600_adc_power_down, adc);
> +	if (ret)
> +		return ret;
> +
> +	iio_dev->name = "x1600-adc";
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +	iio_dev->channels = x1600_adc_channels;
> +	iio_dev->num_channels = ARRAY_SIZE(x1600_adc_channels);
> +	iio_dev->info = &x1600_adc_info;
> +
> +	return devm_iio_device_register(dev, iio_dev);
> +}
> +
> +static const struct of_device_id x1600_adc_of_match[] = {
> +	{ .compatible = "ingenic,x1600-adc" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, x1600_adc_of_match);
> +
> +static struct platform_driver x1600_adc_driver = {
> +	.driver = {
> +		.name = "ingenic-x1600-adc",
> +		.of_match_table = x1600_adc_of_match,
> +	},
> +	.probe = x1600_adc_probe,
> +};
> +module_platform_driver(x1600_adc_driver);
> +MODULE_DESCRIPTION("ADC driver for the Ingenic X1600 SoC");

MODULE_AUTHOR()?

> +MODULE_LICENSE("GPL");



-- 
Kind regards,
Joshua Crofts