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

[Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>]

Hi Andy Shevchenko,

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@intel.com>
> Sent: Monday, March 23, 2026 4:22 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; David Lechner
> <dlechner@baylibre.com>; Nuno Sa <nuno.sa@analog.com>; Andy
> Shevchenko <andy@kernel.org>; Simek, Michal <michal.simek@amd.com>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; saikrishna12468@gmail.com; git (AMD-Xilinx)
> <git@amd.com>
> Subject: Re: [PATCH v2 3/4] iio: adc: xilinx-xadc: Add I2C interface support
> 
> On Mon, Mar 23, 2026 at 01:15:04PM +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 NULL checks for get_dclk_rate callback function in sampling rate
> >   functions to support interfaces without clock control
> > - Create separate iio_info structure(xadc_i2c_info) without event
> >   callbacks for I2C devices
> > - Add xadc_i2c_transaction() function to handle I2C read/write
> > operations
> > - Add XADC_TYPE_US_I2C type to distinguish I2C interface from AXI
> 
> ...
> 
> > -	if (xadc->ops->type == XADC_TYPE_US)
> > +	if (xadc->ops->type == XADC_TYPE_US ||
> > +	    xadc->ops->type == XADC_TYPE_US_I2C)
> 
> Can be one line.
> 
> >  		return 0;
> 
> ...
> 
> >  	/* UltraScale has only one ADC and supports only continuous mode
> */
> > -	if (xadc->ops->type == XADC_TYPE_US)
> > +	if (xadc->ops->type == XADC_TYPE_US ||
> > +	    xadc->ops->type == XADC_TYPE_US_I2C)
> 
> 
> Ditto.
> 
> >  		return XADC_CONF1_SEQ_CONTINUOUS;
> 
> ...
> 
> >  int xadc_write_samplerate(struct xadc *xadc, int val)  {
> > -	unsigned long clk_rate = xadc_get_dclk_rate(xadc);
> > +	unsigned long clk_rate;
> >  	unsigned int div;
> >
> > +	if (!xadc->ops->get_dclk_rate)
> > +		return -EOPNOTSUPP;
> 
> > +	clk_rate = xadc_get_dclk_rate(xadc);
> > +
> 
> Unneeded blank line.
> 
> Also, don't you asked for options?
This callback is defined for all other platforms except I2C.
Currently for I2c interface we are not supporting any configuration, it
is used only to read the channels.

> 
> >  	if (!clk_rate)
> >  		return -EINVAL;
> 
> ...
> 
> > +	i2c_set_clientdata(client, indio_dev);
> 
> Is it used?
No, will remove this one.

Regards
Sai Krishna