Re: [PATCH 1/5] iio: adc: xilinx-xadc: Add helper functions for the device setup

[Andy Shevchenko]

On Fri, Feb 20, 2026 at 11:09:37AM +0530, Sai Krishna Potthuri 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

Reformat text like:

- xadc_device_setup():
  handles IIO device allocation and basic setup

- xadc_device_configure():
  handles device tree parsing and bipolar mask configuration

> This refactoring reduces code duplication and prepares for sharing the
> common setup logic between platform and I2C drivers.

TBH without seeing the rest, this patch looks like an unneeded churn.

...

> +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);

This is a dead code as it seems that ops is mandatory. So in case if it's
missed it will mean that the contribution was never ever being tested.
If it's tested, the check above is a dead code and hence no need to put
it here.

> +	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;
> +}
> +
> +static int xadc_device_configure(struct device *dev, struct iio_dev *indio_dev,
> +				 int irq, unsigned int *conf0, unsigned int *bipolar_mask)
> +{
> +	int ret, i;

Why is 'i' signed? Also see below.

> +
> +	ret = xadc_parse_dt(indio_dev, conf0, irq);
> +	if (ret)
> +		return ret;
> +
> +	*bipolar_mask = 0;
> +	for (i = 0; i < indio_dev->num_channels; i++) {

The iterator is not used outside the loop, hence

	for (unsigned int i = 0; i < indio_dev->num_channels; i++) {

will be good choice.

> +		if (indio_dev->channels[i].scan_type.sign == 's')
> +			*bipolar_mask |= BIT(indio_dev->channels[i].scan_index);
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko