Re: [PATCH v4 2/2] i2c: Add Synopsys Advanced I2C (DWC_i2c) driver

[Mark Brown]

On Tue, Oct 27, 2026 at 05:00:36PM +0530, Aniket Limaye wrote:
> Add a new platform driver for the Synopsys Advanced I2C (DWC_i2c),
> controller mode only.

> The register offsets are defined with respect to header registers for
> each block. These header registers are discovered at runtime by walking
> the block header chain.

This mostly makes sense to me, I've not tested yet and will dig in a bit
more as part of that but a few minor comments below - I'll let you know
how my testing goes hopefully later this week.

> +config I2C_DWC_ADV
> +	tristate "Synopsys Advanced I2C (DWC_i2c) driver"
> +	depends on ARCH_K3 || COMPILE_TEST
> +	help

The dependency is there due to the whole sadness with being unable to
discover the FIFO depths so not having a fallback, but as soon as we add
ACPI bindings we'll have to either drop it or broaden it to be on
CONFIG_ACPI which gets so broad as to be meaningless.  There's a
reasonable chance the dependency list will end up unwieldy for the DT
platforms too.

Since I believe we're always working with configurable trigger levels
for the FIFOs I think it might be better to have a fallback compatible
that uses the IP minimum of two entries in each of the FIFOs, that ought
to work even if the FIFOs are in reality bigger.  Performance would
suffer but slow and functional beats not working at all.

It's really unfortunate that we can't enumerate this from the IP
directly.

> +static void dwc_adv_i2c_read(struct dwc_adv_i2c_dev *dev)
> +{

> +			if (msgs[dev->msg_read_idx].flags & I2C_M_RECV_LEN) {
> +				/*
> +				 * STOP is only set after receiving the block
> +				 * data response length. When that length is
> +				 * invalid, read one more byte with STOP set to
> +				 * complete the transaction.
> +				 */
> +				if (!data || data > I2C_SMBUS_BLOCK_MAX)
> +					data = 1;
> +
> +				len = dwc_adv_i2c_recv_len(dev, data);
> +			}

It feels off to silently fix up an invalid length, but possibly that's
the most sensible thing - I didn't cross check what's idiomatic for I2C
in general.

> +static irqreturn_t dwc_adv_i2c_handle_irq(struct dwc_adv_i2c_dev *dev)
> +{
> +	u32 enabled, stat;
> +
> +	enabled = dwc_adv_reg_read(dev, dev->regs.enable);
> +	stat = dwc_adv_reg_read(dev, dev->regs.raw_intr_stat);
> +
> +	if (!enabled || !(stat & ~DWC_ADV_I2C_INTR_ACTIVITY))
> +		return IRQ_NONE;

Should we check for enabled before we read the status register
(microoptimisation is fun!)?

Attachment: signature.asc
Description: PGP signature