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

[Andy Shevchenko]

On Mon, Oct 26, 2026 at 09:36:31AM +0100, Olivier Louvignes wrote:
> The X1600's SAR ADC has four auxiliary channels, each with its own start
> bit, interrupt flag and 12-bit result, and none of the touchscreen or
> battery engines of the JZ47xx ADCs that ingenic-adc drives. Add a small
> driver that exposes the four channels as voltages against the 3.3 V
> reference.
> 
> The registers are in chapter 19 of the X1600 programming manual. The
> ADC clock divider is even, as the manual has it (an odd value divides
> by one less); the vendor kernel writes the divider minus one, which
> gives a slightly faster clock than it asks for.
> 
> Tested on a HiBy R1, whose play/pause and volume keys sit on channel 0
> behind adc-keys: all three read and report correctly.

...

> +config INGENIC_X1600_ADC
> +	tristate "Ingenic X1600 ADC driver"

Make this text...

> +	depends on MIPS || COMPILE_TEST
> +	help
> +	  Say yes here to build support for the SAR ADC of the Ingenic X1600
> +	  SoC and its four auxiliary channels.

...

> +/*
> + * ADC driver for the Ingenic X1600 SoC

...and this comment along with the MODULE_DESCRIPTION() be the same.

> + * Copyright (c) 2026 Olivier Louvignes <olivier@xxxxxxxxx>
> + *
> + * The X1600's SAR ADC has four auxiliary channels, each with its own start
> + * bit, interrupt and 12-bit result (X1600 programming manual, chapter 19).
> + */

...

> +#define X1600_ADC_REG_DATA(ch)		(0x10 + ((ch) / 2) * 4)

> +#define X1600_ADC_DATA_MASK(ch)		(GENMASK(11, 0) << ((ch) % 2 * 16))

Drop /2 and %2 here, see below why. This probably need to have ch be renamed to
something else as it's like bank and shift.

...

> +#define X1600_ADC_CLK_RATE		1166667

Is this in Hz?

...

> +static int x1600_adc_read_raw_channel(struct x1600_adc *adc, unsigned int ch, int *val)
> +{
> +	int ret = 0;
> +	u32 data;
> +
> +	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);

This is the only case when these two macros are being used. While  / 2 and % 2
are power-of-2 based calculations, it's still make sense to have temporary
variable for both of them (see the proposed naming above) and use in a form

	bank = ch / 2;
	shift = ch % 2;

this can be optimised on some architectures to a single assembly instruction
(yes, both calculations). The problem is that they should be tighten to each
other so compiler sees it. In the current code they are go via macros and
compiler most likely can't deduce that.

All the above is based on the real life example, see 9b3cd5c7099f ("regmap:
place foo / 8 and foo % 8 closer to each other") for that.

> +	return 0;
> +}

...

> +	writel(X1600_ADC_CHANNELS_MASK, adc->base + X1600_ADC_REG_CTRL);

It makes sense to have read and write wrappers that will add adc->base.

static inline u32 void x1600_read(adc, unsigned int offset)
{
	return readl(adc->base + offset);
}

static inline void x1600_write(adc, unsigned int offset, u32 value)
{
	writel(value, adc->base + offset);
}

Also you may consider using regmap MMIO.

...

> +	rate = clk_get_rate(adc->clk);
> +	div = clamp(round_up(DIV_ROUND_UP(rate, X1600_ADC_CLK_RATE), 2), 2UL, 254UL);

Wouldn't it be better to clamp the rate first and then divide with rounding up?

-- 
With Best Regards,
Andy Shevchenko