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

From: Wolfram Sang
Date: Wed Mar 27 2019 - 18:14:57 EST



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