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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Nov 02, 2026 at 09:11:55PM +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.

A few nit-picks below, otherwise feel free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

...

> +static int x1600_adc_read_raw_channel(struct x1600_adc *adc, unsigned int ch, int *val)
> +{
> +	unsigned int bank, shift;
> +	int ret = 0;
> +	u32 enable, data;

Keep reversed xmas tree order, also split assignment.

	unsigned int bank, shift;
	u32 enable, data;
	int ret;


> +	guard(mutex)(&adc->lock);
> +
> +	reinit_completion(&adc->done);
> +	x1600_adc_write(adc, X1600_ADC_REG_STATUS, BIT(ch));
> +	enable = x1600_adc_read(adc, X1600_ADC_REG_ENABLE);
> +	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, enable | BIT(ch));

> +	if (!wait_for_completion_timeout(&adc->done,
> +					 msecs_to_jiffies(X1600_ADC_TIMEOUT_MS)))
> +		ret = -ETIMEDOUT;

#define _TIMEOUT		msecs_to_jiffies(...plain number...)

	if (wait_for_completion_timeout(&adc->done, X1600_ADC_TIMEOUT))
		ret = 0;
	else
		ret = -ETIMEDOUT;

> +	enable = x1600_adc_read(adc, X1600_ADC_REG_ENABLE);
> +	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, enable & ~BIT(ch));
> +	if (ret)
> +		return ret;
> +
> +	/* Two results to a data register, the even channel's in the low half. */
> +	bank = ch / 2;
> +	shift = ch % 2;
> +	data = x1600_adc_read(adc, X1600_ADC_REG_DATA(bank));
> +	*val = field_get(X1600_ADC_DATA_MASK(shift), data);

+ blank line.

> +	return 0;
> +}

...

> +static int x1600_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *iio_dev;
> +	struct x1600_adc *adc;
> +	unsigned long rate;
> +	u32 div, reg;
> +	int irq, ret;
> +
> +	iio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(iio_dev);
> +	init_completion(&adc->done);
> +	ret = devm_mutex_init(dev, &adc->lock);
> +	if (ret)
> +		return ret;
> +
> +	adc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(adc->base))
> +		return PTR_ERR(adc->base);
> +
> +	adc->clk = devm_clk_get_enabled(dev, NULL);

Can we have clocks always be named? Otherwise if any new dual-clock HW appear
here, the code will be disrupted.

> +	if (IS_ERR(adc->clk))
> +		return dev_err_probe(dev, PTR_ERR(adc->clk), "Unable to get clock\n");
> +
> +	/* Powered down, interrupts masked and cleared, as the vendor does. */
> +	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, X1600_ADC_ENABLE_POWER_DOWN);
> +	x1600_adc_write(adc, X1600_ADC_REG_CTRL, X1600_ADC_CHANNELS_MASK);
> +	x1600_adc_write(adc, X1600_ADC_REG_STATUS, X1600_ADC_CHANNELS_MASK);

> +	rate = clamp(clk_get_rate(adc->clk), 2UL * X1600_ADC_CLK_RATE_HZ,
> +		     254UL * X1600_ADC_CLK_RATE_HZ);

I would put it as

	rate = clamp(clk_get_rate(adc->clk),
		     2UL * X1600_ADC_CLK_RATE_HZ, 254UL * X1600_ADC_CLK_RATE_HZ);

for the sake of the logical split (and yes, 81 characters in this case is okay).

> +	div = round_up(DIV_ROUND_UP(rate, X1600_ADC_CLK_RATE_HZ), 2);
> +	/* US and MS at their reset values: single conversions don't use them. */
> +	x1600_adc_write(adc, X1600_ADC_REG_CLKDIV,
> +			FIELD_PREP(X1600_ADC_CLKDIV_CLKDIV, div) |
> +			FIELD_PREP(X1600_ADC_CLKDIV_US, 16) |
> +			FIELD_PREP(X1600_ADC_CLKDIV_MS, 4));
> +	reg = x1600_adc_read(adc, X1600_ADC_REG_STABLE) & ~X1600_ADC_STABLE_TIME;
> +	x1600_adc_write(adc, X1600_ADC_REG_STABLE,
> +			reg | FIELD_PREP(X1600_ADC_STABLE_TIME, 1));
> +
> +	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 ret;
> +
> +	/*
> +	 * Left powered up: a typical use, such as buttons read through adc-keys,
> +	 * polls every few tens of milliseconds, and powering up takes 2 ms.
> +	 */
> +	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, 0);
> +	fsleep(2 * USEC_PER_MSEC);
> +	x1600_adc_write(adc, X1600_ADC_REG_CTRL, 0);
> +
> +	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);
> +}

-- 
With Best Regards,
Andy Shevchenko