linux-kernel - Re: [PATCH v9 3/3] i2c: npcm7xx: Add support for slave mode for Nuvoton

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200323160158.GR1922688@smile.fi.intel.com>
Date:   Mon, 23 Mar 2020 18:01:58 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Tali Perry <tali.perry1@...il.com>
Cc:     brendanhiggins@...gle.com, avifishman70@...il.com,
        tmaimon77@...il.com, kfting@...oton.com, venture@...gle.com,
        yuenn@...gle.com, benjaminfair@...gle.com, robh+dt@...nel.org,
        wsa@...-dreams.de, linux-arm-kernel@...ts.infradead.org,
        linux-i2c@...r.kernel.org, openbmc@...ts.ozlabs.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 3/3] i2c: npcm7xx: Add support for slave mode for
 Nuvoton

On Mon, Mar 23, 2020 at 03:44:37PM +0200, Tali Perry wrote:
> Add support for slave mode for Nuvoton
> NPCM BMC I2C controller driver.

Same comments apply as per previous patch.

...

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

Perhaps create a separate module and compile it when user selects?
See I²C DesignWare split.

...

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	if (bus->slave) {
> +		npcm_i2c_slave_enable_l(bus, I2C_SLAVE_ADDR1, bus->slave->addr,
> +					true);
> +	}

Put it on one line and drop {} -- it will be much better to read.

> +#endif

...

> +static void npcm_i2c_write_to_fifo_slave(struct npcm_i2c *bus,
> +					 u16 max_bytes_to_send)
> +{
> +	// Fill the FIFO, while the FIFO is not full and there are more bytes to
> +	// write
> +	npcm_i2c_clear_fifo_int(bus);
> +	npcm_i2c_clear_tx_fifo(bus);
> +	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
> +
> +	while ((max_bytes_to_send--) && (I2C_HW_FIFO_SIZE -
> +					 npcm_i2c_get_fifo_fullness(bus))) {
> +		if (bus->slv_wr_size > 0) {
> +			npcm_i2c_wr_byte(bus,
> +					 bus->slv_wr_buf[bus->slv_wr_ind %
> +					 I2C_HW_FIFO_SIZE]);
> +			bus->slv_wr_ind = (bus->slv_wr_ind + 1) %
> +					   I2C_HW_FIFO_SIZE;
> +			bus->slv_wr_size--; // size indicates the # of bytes in
> +					    // the SW FIFO, not HW.
> +		} else {
> +			break;
> +		}

This looks ugly. Can you redo the style.

> +	}
> +}

...

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +			if (bus->slave) {
> +				bus->slv_rd_buf[bus->slv_rd_ind %
> +						I2C_HW_FIFO_SIZE] = data;
> +				bus->slv_rd_ind++;
> +				if (bus->slv_rd_ind == 1 && bus->read_block_use)
> +					// 1st byte is length in block protocol
> +					bus->slv_rd_size = data +
> +							   (u8)bus->PEC_use +
> +							(u8)bus->read_block_use;
> +			}
> +#endif

Ditto.

...

>  		   I2C_FUNC_SMBUS_EMUL |
>  		   I2C_FUNC_SMBUS_BLOCK_DATA |
>  		   I2C_FUNC_SMBUS_PEC
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +		   | I2C_FUNC_SLAVE
> +#endif

Completely inconsistent style.

>  		   ;

-- 
With Best Regards,
Andy Shevchenko