Re: [PATCH 15/21] i2c: Add driver for ADI ADSP-SC5xx platforms

[Arnd Bergmann]

On Thu, Sep 12, 2024, at 18:25, Arturs Artamonovs via B4 Relay wrote:
> +
> +config I2C_ADI_TWI_CLK_KHZ
> +    int "ADI TWI I2C clock (kHz)"
> +    depends on I2C_ADI_TWI
> +    range 21 400
> +    default 50
> +    help
> +      The unit of the TWI clock is kHz.

This does not look like something that should be a compile-time
option, the kernel needs to be able to run on different
configurations.

> +
> +static void adi_twi_handle_interrupt(struct adi_twi_iface *iface,
> +					unsigned short twi_int_status,
> +					bool polling)
> +{
> +	u16 writeValue;
> +	unsigned short mast_stat = ioread16(&iface->regs_base->master_stat);

It's a bit unusual to use ioread16()/iowrite16() instead of the
normal readw()/writew().

> +			} else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> +				   iface->cur_msg + 1 < iface->msg_num) {
> +
> +				if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD) {
> +					writeValue = ioread16(&iface->regs_base->master_ctl)
> +							      | MDIR;
> +					iowrite16(writeValue, &iface->regs_base->master_ctl);
> +				} else {
> +					writeValue = ioread16(&iface->regs_base->master_ctl)
> +							      & ~MDIR;
> +					iowrite16(writeValue, &iface->regs_base->master_ctl);

The use of a structure instead of register offset macros makes
these lines rather long, especially at five levels of indentation.
Maybe this can be restructured for readability.

> +		if (ioread16(&iface->regs_base->master_stat) & SDASEN) {
> +			int cnt = 9;
> +
> +			do {
> +				iowrite16(SCLOVR, &iface->regs_base->master_ctl);
> +				udelay(6);
> +				iowrite16(0, &iface->regs_base->master_ctl);
> +				udelay(6);
> +			} while ((ioread16(&iface->regs_base->master_stat) & SDASEN)

Since writes on device mappings are posted, the delay between
the two iowrite16() is not really meaningful, unless you add
another ioread16() or readw() before the delay. Mapping these
with ioremap_np() should also work.

> +			iowrite16(SDAOVR | SCLOVR, &iface->regs_base->master_ctl);
> +			udelay(6);
> +			iowrite16(SDAOVR, &iface->regs_base->master_ctl);
> +			udelay(6);
> +			iowrite16(0, &iface->regs_base->master_ctl);
> +		}

Same here.

> +/* Interrupt handler */
> +static irqreturn_t adi_twi_handle_all_interrupts(struct adi_twi_iface 
> *iface,
> +						 bool polling)
> +{
> +	irqreturn_t handled = IRQ_NONE;
> +	unsigned short twi_int_status;
> +
> +	while (1) {
> +		twi_int_status = ioread16(&iface->regs_base->int_stat);
> +		if (!twi_int_status)
> +			return handled;
> +		/* Clear interrupt status */
> +		iowrite16(twi_int_status, &iface->regs_base->int_stat);
> +		adi_twi_handle_interrupt(iface, twi_int_status, polling);
> +		handled = IRQ_HANDLED;
> +	}
> +}
> +
> +static irqreturn_t adi_twi_interrupt_entry(int irq, void *dev_id)
> +{
> +	struct adi_twi_iface *iface = dev_id;
> +	unsigned long flags;
> +	irqreturn_t handled;
> +
> +	spin_lock_irqsave(&iface->lock, flags);
> +	handled = adi_twi_handle_all_interrupts(iface, false);
> +	spin_unlock_irqrestore(&iface->lock, flags);
> +	return handled;
> +}

Interrupt handlers are called with IRQs disabled, so no
need to turn them off again.

> +static SIMPLE_DEV_PM_OPS(i2c_adi_twi_pm,
> +			 i2c_adi_twi_suspend, i2c_adi_twi_resume);
> +#define I2C_ADI_TWI_PM_OPS	(&i2c_adi_twi_pm)
> +#else
> +#define I2C_ADI_TWI_PM_OPS	NULL
> +#endif

Please convert to DEFINE_SIMPLE_DEV_PM_OPS() and remove the
#ifdef.

> +#ifdef CONFIG_OF
> +static const struct of_device_id adi_twi_of_match[] = {
> +	{
> +		.compatible = "adi,twi",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, adi_twi_of_match);
> +#endif

No need to optimize for non-CONFIG_OF builds, we don't
support traditional board files on arm64.

> +	match = of_match_device(of_match_ptr(adi_twi_of_match), &pdev->dev);

This of_match_ptr() and the second one later should also
get removed then.

> \ No newline at end of file
>

Whitespace damage.

      Arnd