Re: [PATCH 4/5] iio: adc: xilinx-xadc: Add I2C interface support

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

On Fri, Feb 20, 2026 at 11:09:40AM +0530, Sai Krishna Potthuri wrote:
> Add I2C interface support for Xilinx System Management Wizard IP along
> with the existing AXI memory-mapped interface. This support enables
> monitoring the voltage and temperature on UltraScale+ devices where the
> System Management Wizard is connected via I2C.
> 
> Key changes:
> - Implement 32-bit DRP(Dynamic Reconfiguration Port) packet format as per
>   Xilinx PG185 specification.
> - Add separate I2C probe with xadc_i2c_of_match_table to handle same
>   compatible string("xlnx,system-management-wiz-1.3") on I2C bus.
> - Implement delayed version of hardware initialization for I2C interface
>   to handle the case where System Management Wizard IP is not ready during
>   the I2C probe.
> - Add xadc_i2c_transaction() function to handle I2C read/write operations
> - Add XADC_TYPE_US_I2C type to distinguish I2C interface from AXI.

...

>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/i2c.h>

No, split driver to a few files:
_core.c
_platform.c
_i2c.c

The last two will be just a glue code.

...

> +#ifdef CONFIG_XILINX_XADC_I2C

No way for ugly ifdeffery!

...

> +static int xadc_i2c_transaction(struct xadc *xadc, unsigned int reg, uint16_t write_data,
> +				bool is_write, uint16_t *read_data)

Should be simply u16.

...

> +	char write_buffer[XADC_I2C_WRITE_DATA_SIZE] = { 0 };

No '0' is needed.

...

> +	if (is_write) {

Bad design. Instead make two functions, one for read, one for write and drop
the boolean parameter.

> +	} else {
> +	}

> +	/* Read response for read operations */
> +	if (!is_write) {

> +	}

...

> +static int xadc_hardware_init(struct xadc *xadc)
> +{
> +	int ret, i;

Use unsigned iterator inside the loop.

> +	for (i = 0; i < 16; i++) {

Magic 16.

> +		ret = xadc_i2c_transaction(xadc, XADC_REG_THRESHOLD(i), 0, false,
> +					   &xadc->threshold[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = xadc_i2c_transaction(xadc, XADC_REG_CONF0, xadc->conf0, true, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = xadc_i2c_transaction(xadc, XADC_REG_INPUT_MODE(0), xadc->bipolar_mask, true, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = xadc_i2c_transaction(xadc, XADC_REG_INPUT_MODE(1), xadc->bipolar_mask >> 16, true,
> +				   NULL);
> +	if (ret)
> +		return ret;
> +
> +	xadc->hw_initialized = true;
> +
> +	return 0;
> +}

...

> +static const struct of_device_id xadc_i2c_of_match_table[] = {
> +	{
> +		.compatible = "xlnx,system-management-wiz-1.3",
> +		.data = &xadc_system_mgmt_wiz_i2c_ops

Leave trailing comma as it's not a sentinel.

> +	},
> +	{ },

The opposite. IIO has established way besides the fact that sentinel by
definition must be the last, trailing comma is confusing for a bare minimum.

> +};

-- 
With Best Regards,
Andy Shevchenko