Re: [PATCH v5 08/10] i2c: npcm: Remove own slave addresses 2:10

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]


> NPCM can support up to 10 own slave addresses. In practice, only one
> address is actually being used. In order to access addresses 2 and above,
> need to switch register banks. The switch needs spinlock.
> To avoid using spinlock for this useless feature removed support of SA >=
> 2. Also fix returned slave event enum.

Is the spinlock contention so high? The code paths do not really look
like hot paths to me. A bit sad to see this feature go.

>  static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
>  	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
>  	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,

Why do we keep this array if we drop the support?

> @@ -604,8 +602,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
>  			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
>  		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
>  		return 0;
> -	}
> -	if (addr_type == I2C_ARP_ADDR) {
> +	} else if (addr_type == I2C_ARP_ADDR) {

I might be wrong but this looks like a seperate change?

> @@ -924,11 +918,15 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
>  	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
>  		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
>  			break;
> -		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +		if (bus->state == I2C_SLAVE_MATCH) {
> +			i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +			bus->state = I2C_OPER_STARTED;
> +		} else {
> +			i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
> +		}
>  		ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
>  		bus->slv_wr_buf[ind] = value;
>  		bus->slv_wr_size++;
> -		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
>  	}
>  	return I2C_HW_FIFO_SIZE - ret;
>  }
> @@ -976,7 +974,6 @@ static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
>  	if (nwrite == 0)
>  		return;
>  
> -	bus->state = I2C_OPER_STARTED;
>  	bus->operation = I2C_WRITE_OPER;

This is definately a seperate change!

All the best!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]