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

[Aniket Limaye]

On 27/10/26 22:02, Mark Brown wrote:
> 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.

Sure, let me know how your testing goes.

> > +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.

Well, I was going off of direction from v1 [0], where feedback was to 
avoid generic compatible.

Although I do understand that the feedback then was for when I was 
adding support in existing driver, so may not apply directly when adding 
a separate new driver, where having one fallback may make sense?

I personally would rather we keep it simple and not add a fallback, 
which may not really get used since one would want an IP specific fifo 
depth configured in the driver, but willing to add one if that's the norm.

In case we do want a fallback, shall I add a fallback of fifo depths=2 
for both rx/tx? That's what the IP doc seems to say as a minimum for 
their respective configuration parameters.

[0]: 
https://lore.kernel.org/all/20260921141031.GX106095@xxxxxxxxxxxxxxxxxxx/

> > +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.

Yeah, will return with -EPROTO in next version

> > +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!)?

Will take care of in next version

Thanks for the review!
Aniket