Re: [PATCH v4 12/15] iio: adc: xilinx-xadc: Add helper functions for the device setup

[Jonathan Cameron]

On Mon, 5 Oct 2026 15:12:02 +0530
Dileep Kumar Nagavarapu <dileepkumar.nagavarapu@xxxxxxx> wrote:

> Refactor the platform driver probe function by extracting device
> setup and configuration logic into reusable helper functions:
> xadc_device_setup(): handles IIO device allocation and basic setup
> xadc_device_configure(): handles device tree parsing and bipolar mask
> configuration
> 
> Move the bipolar channel mask calculation immediately after
> xadc_parse_dt(), as the channel information used to build
> the mask is populated there and remains unchanged before it is consumed.
> 
> This refactoring reduces code duplication and prepares for sharing the
> common setup logic between platform and I2C drivers.
> 
> Co-developed-by: Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx>
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx>
> Signed-off-by: Dileep Kumar Nagavarapu <dileepkumar.nagavarapu@xxxxxxx>
> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 105 +++++++++++++++++++----------
>  1 file changed, 71 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 92cd38cd1834..d95a84e7e561 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -933,6 +933,33 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int xadc_setup_buffer_and_triggers(struct iio_dev *indio_dev, int irq)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct xadc *xadc = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!(xadc->ops->flags & XADC_FLAGS_BUFFERED))
> +		return 0;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, &iio_pollfunc_store_time,
> +					      &xadc_trigger_handler, &xadc_buffer_ops);
> +	if (ret)
> +		return ret;
> +
> +	if (irq > 0) {
Unless it gets modified in later patches, flip this to reduce indent a bit + early exit
in the simple case of no irq.
	if (irq <= 0)
		return 0;

	xadc->convst_...

> +		xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
> +		if (IS_ERR(xadc->convst_trigger))
> +			return PTR_ERR(xadc->convst_trigger);
> +
> +		xadc->samplerate_trigger = xadc_alloc_trigger(indio_dev, "samplerate");
> +		if (IS_ERR(xadc->samplerate_trigger))
> +			return PTR_ERR(xadc->samplerate_trigger);
> +	}
> +
> +	return 0;
> +}

> +static struct iio_dev *xadc_device_setup(struct device *dev, int size, const struct xadc_ops **ops)
> +{
> +	struct iio_dev *indio_dev;
> +
> +	*ops = device_get_match_data(dev);
> +	if (!*ops)
> +		return ERR_PTR(-ENODEV);
Having this in here seems more complex to me than moving it
to the caller and passing (*ops)->type in as a parameter
for this function.

Whilst I appreciate the aim of reducing repetition, I'm not
totally convinced this helper has the right balance of
saving code to readability.

> +
> +	indio_dev = devm_iio_device_alloc(dev, size);
> +	if (!indio_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	indio_dev->name = xadc_type_names[(*ops)->type];
> +	indio_dev->info = &xadc_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return indio_dev;
> +}
>