Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

[Wolfram Sang]

> +static void bcm_iproc_i2c_slave_init(
> +	struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
> +{
> +	u32 val;
> +
> +	if (need_reset) {
> +		/* put controller in reset */
> +		val = readl(iproc_i2c->base + CFG_OFFSET);
> +		val |= BIT(CFG_RESET_SHIFT);
> +		writel(val, iproc_i2c->base + CFG_OFFSET);
> +
> +		/* wait 100 usec per spec */
> +		udelay(100);
> +
> +		/* bring controller out of reset */
> +		val &= ~(BIT(CFG_RESET_SHIFT));
> +		writel(val, iproc_i2c->base + CFG_OFFSET);
> +	}
> +
> +	/* flush TX/RX FIFOs */
> +	val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
> +	writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);

Will flushing FIFOs work when a slave is register while a master
transfer is on-going at the same time?

> +
> +	/* RANDOM SLAVE STRETCH time - 20ms*/

What is a "random stretch time"? 20ms sounds like a lot. Also, missing
space before comment terminator.

> @@ -224,22 +473,25 @@ static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
>  
>  	/* put controller in reset */
>  	val = readl(iproc_i2c->base + CFG_OFFSET);
> -	val |= 1 << CFG_RESET_SHIFT;
> -	val &= ~(1 << CFG_EN_SHIFT);
> +	val |= BIT(CFG_RESET_SHIFT);
> +	val &= ~(BIT(CFG_EN_SHIFT));
>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>  
>  	/* wait 100 usec per spec */
>  	udelay(100);
>  
>  	/* bring controller out of reset */
> -	val &= ~(1 << CFG_RESET_SHIFT);
> +	val &= ~(BIT(CFG_RESET_SHIFT));
>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>  
>  	/* flush TX/RX FIFOs and set RX FIFO threshold to zero */
> -	val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
> +	val = (BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT));
>  	writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
>  	/* disable all interrupts */
> -	writel(0, iproc_i2c->base + IE_OFFSET);
> +	val = readl(iproc_i2c->base + IE_OFFSET);
> +	val &= ~(IE_M_ALL_INTERRUPT_MASK <<
> +			IE_M_ALL_INTERRUPT_SHIFT);
> +	writel(val, iproc_i2c->base + IE_OFFSET);

This block looks unrelated, but I won't be too strict here...

> +	case M_CMD_STATUS_FIFO_UNDERRUN:
> +		dev_dbg(iproc_i2c->device, "FIFO under-run\n");
> +		return -ENXIO;
> +
> +	case M_CMD_STATUS_RX_FIFO_FULL:
> +		dev_dbg(iproc_i2c->device, "Master Rx FIFO full > 10ms\n");
> +		return -ETIMEDOUT;
> +

... however, this looks really unrelated to me. This is about master
transmission, or?

Rest looks OK.

Attachment: signature.asc
Description: PGP signature