Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 25 Jun 2026 19:06:38 +0800
Chi-Wen Weng <cwweng.linux@gmail.com> wrote:

> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
> 
> The driver supports direct raw reads and triggered buffered capture. The
> controller end-of-conversion interrupt is exposed as the device trigger
> and is used to push samples into the IIO buffer.
> 
> Channels are described by firmware child nodes and can be configured as
> single-ended or differential inputs. Since the differential enable bit is
> global, mixed single-ended and differential buffered scans are rejected.
> 
> DMA support is intentionally not included in this initial upstream driver;
> conversions are handled through the interrupt-driven path.
> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>

A few minor additional comments in line. I've tried to avoid
duplication but there may be some there!

Jonathan
> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
> new file mode 100644
> index 000000000000..0c075126e139
> --- /dev/null
> +++ b/drivers/iio/adc/ma35d1_eadc.c

...

> +}
> +
> +static int ma35d1_adc_read_raw(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2, long mask)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		mutex_lock(&adc->lock);

Look at the ACQUIRE() macros for claim direct stuff (in iio.h)
and then use guard() for this.  Be careful to add {} to define scope
to being this case block.  May not save much code but it will be easeir
to read than this currently is.

> +		ret = ma35d1_adc_read_conversion(indio_dev, chan, val);
> +		mutex_unlock(&adc->lock);
> +
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
> +				     struct device *dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS);
> +	struct fwnode_handle *child;
> +	struct iio_chan_spec *channels;
> +	int num_channels;
> +	int scan_index = 0;

Move assignment down to just above the loop.  Makes it easier for
reviewers to associate the initial value with what is going on.

> +	int ret;
> +
> +	bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS);
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (!num_channels)
> +		return dev_err_probe(dev, -ENODATA,
> +				     "no ADC channels configured\n");
> +
> +	if (num_channels > MA35D1_EADC_MAX_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
> +
> +	channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels),
> +				GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	device_for_each_child_node(dev, child) {
...

> +
> +static int ma35d1_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct ma35d1_adc *adc;
> +	int irq;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	adc = iio_priv(indio_dev);
> +	adc->dev = dev;
> +	mutex_init(&adc->lock);
For new code
	ret = devm_mutex_init(&adc->lock);
	if (ret)
		return ret;

The advantage this brings to debug is small but it's also very simple
so we are no doing it for all new code in IIO.

> +	init_completion(&adc->completion);
> +
> +	adc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(adc->regs))
> +		return dev_err_probe(dev, PTR_ERR(adc->regs),
> +				     "failed to map registers\n");
> +
> +	adc->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(adc->clk))
> +		return dev_err_probe(dev, PTR_ERR(adc->clk),
> +				     "failed to get and enable ADC clock\n");
> +
> +	indio_dev->name = "ma35d1-eadc";
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;

Second part of this is set by the triggered_buffer call later. So don't set it
here.

> +	indio_dev->info = &ma35d1_adc_info;
> +
> +	ret = ma35d1_adc_parse_channels(indio_dev, dev);
> +	if (ret)
> +		return ret;
> +
> +	ma35d1_adc_hw_init(adc);
> +
> +	ret = devm_add_action_or_reset(dev, ma35d1_adc_hw_disable, adc);
> +	if (ret)
> +		return ret;
> +
> +	ret = ma35d1_adc_setup_trigger(indio_dev, dev);
> +	if (ret)
> +		return ret;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev),
> +			       indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ma35d1_adc_trigger_handler,
> +					      &ma35d1_adc_buffer_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to setup triggered buffer\n");
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register IIO device\n");
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;

Failing suspend because a buffer is enabled is unlikely to be popular.
Can you not save necessary state and restore so buffered capture continues
on resume?

> +
> +	ma35d1_adc_hw_disable(adc);
> +	clk_disable_unprepare(adc->clk);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(adc->clk);
> +	if (ret)
> +		return ret;
> +
> +	ma35d1_adc_hw_init(adc);
> +
> +	return 0;
> +}