Re: [PATCH v9 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

From: Andy Shevchenko
Date: Mon Mar 23 2020 - 11:57:00 EST


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

...

> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/jiffies.h>
> +#include <linux/iopoll.h>

Perhaps ordered?

> +enum i2c_mode {
> + I2C_MASTER,
> + I2C_SLAVE

+ Comma.

> +};

...

> +#define NPCM_I2CSEGCTL 0xE4

Indentation issue, see the below lines.

> +#define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
> +
> +// Common regs
> +#define NPCM_I2CSDA 0x00

...

> +#define I2C_FREQ_100KHZ 100
> +#define I2C_FREQ_400KHZ 400
> +#define I2C_FREQ_1MHZ 1000

Can you rather use standard definitions in Hz?

...

> +#define NPCM_I2C_EVENT_LOG(event) (bus->event_log |= event)

Useless macro, and even harmful to some extend - it hides one of the parameter
(proper should take bus and event, but it will be quite longer than simple
conditional). Use in-place the conditional.

...

> +struct npcm_i2c {

> + u32 xmits;

> +

Unnecessary blank line.
I have noticed other places with the same issue. Fix 'em all.

> +};

...

> +static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
> +{
> + if (bus->operation == I2C_READ_OPER)
> + return bus->rd_ind;

> + else if (bus->operation == I2C_WRITE_OPER)

Redundant 'else'

> + return bus->wr_ind;
> + return 0;
> +}

...

> +static void npcm_i2c_disable(struct npcm_i2c *bus)
> +{
> + u8 i2cctl2;

+ blank line.

> + // Disable module.
> + i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);
> + i2cctl2 = i2cctl2 & ~I2CCTL2_ENABLE;
> + iowrite8(i2cctl2, bus->reg + NPCM_I2CCTL2);
> +
> + bus->state = I2C_DISABLE;
> +}

...

> +static void npcm_i2c_enable(struct npcm_i2c *bus)
> +{
> + u8 i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);

In this case direct assignment like above is okay...

> +
> + i2cctl2 = i2cctl2 | I2CCTL2_ENABLE;
> + iowrite8(i2cctl2, bus->reg + NPCM_I2CCTL2);
> + bus->state = I2C_IDLE;
> +}
> +
> +// enable\disable end of busy (EOB) interrupt
> +static inline void npcm_i2c_eob_int(struct npcm_i2c *bus, bool enable)
> +{

> + u8 val = ioread8(bus->reg + NPCM_I2CCST3);

...but not here. Better...

> +

> + // Clear EO_BUSY pending bit:

...to perform it explicitly here.

> + val = val | NPCM_I2CCST3_EO_BUSY;
> + iowrite8(val, bus->reg + NPCM_I2CCST3);
> +
> + val = ioread8(bus->reg + NPCM_I2CCTL1);
> + if (enable)
> + val = (val | NPCM_I2CCTL1_EOBINTE) & ~NPCM_I2CCTL1_RWS;
> + else
> + val = (val & ~NPCM_I2CCTL1_EOBINTE) & ~NPCM_I2CCTL1_RWS;
> + iowrite8(val, bus->reg + NPCM_I2CCTL1);
> +}

...

> +static void npcm_i2c_int_enable(struct npcm_i2c *bus, bool enable)
> +{
> + u8 val = ioread8(bus->reg + NPCM_I2CCTL1);
> +
> + if (enable)
> + val = (val | NPCM_I2CCTL1_INTEN) & ~NPCM_I2CCTL1_RWS;
> + else
> + val = (val & ~NPCM_I2CCTL1_INTEN) & ~NPCM_I2CCTL1_RWS;
> + iowrite8(val, bus->reg + NPCM_I2CCTL1);

if (enable)
val |= NPCM_I2CCTL1_INTEN;
else
val &= ~NPCM_I2CCTL1_INTEN;
iowrite8(val & ~NPCM_I2CCTL1_RWS, bus->reg + NPCM_I2CCTL1);

Ditto for the rest similar cases.

> +}

...

> + val = (val | NPCM_I2CCTL1_START) &
> + ~(NPCM_I2CCTL1_STOP | NPCM_I2CCTL1_ACK);

val |= NPCM_I2CCTL1_START;
val &= ~(NPCM_I2CCTL1_STOP | NPCM_I2CCTL1_ACK);

Ditto for other similar cases.

...

> +static inline void npcm_i2c_master_stop(struct npcm_i2c *bus)
> +{

> + if (bus->fifo_use) {

if (...)
return;

> + npcm_i2c_select_bank(bus, I2C_BANK_1);
> +
> + if (bus->operation == I2C_READ_OPER)
> + npcm_i2c_clear_rx_fifo(bus);
> + else
> + npcm_i2c_clear_tx_fifo(bus);
> +
> + npcm_i2c_clear_fifo_int(bus);
> +
> + iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
> + }
> +}

...

> + NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_CB);

Some magic happens here...

...

> +static u32 npcm_i2c_get_fifo_fullness(struct npcm_i2c *bus)
> +{
> + if (bus->operation == I2C_WRITE_OPER)
> + return FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
> + ioread8(bus->reg + NPCM_I2CTXF_STS));

> + else if (bus->operation == I2C_READ_OPER)

Redundant 'else'

> + return FIELD_GET(NPCM_I2CRXF_STS_RX_BYTES,
> + ioread8(bus->reg + NPCM_I2CRXF_STS));
> + return 0;
> +}

...

> +// configure the FIFO before using it. If nread is -1 RX FIFO will not be
> +// configured. same for nwrite

TABs/spaces mix.

...

> +static void npcm_i2c_read_from_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)
> +{
> + u8 data;
> +
> + while (bytes_in_fifo--) {
> + data = npcm_i2c_rd_byte(bus);
> +
> + if (bus->master_or_slave == I2C_MASTER) {
> + if (bus->rd_ind < bus->rd_size)
> + bus->rd_buf[bus->rd_ind++] = data;

> + } else { // I2C_SLAVE:

Redundant (at least in this patch).

> + }
> + }
> +}

...

> + int rcount;
> + int fifo_bytes;

> + if (rcount < (2 * I2C_HW_FIFO_SIZE) && rcount > I2C_HW_FIFO_SIZE)

> + fifo_bytes = (u8)(rcount - I2C_HW_FIFO_SIZE);

Why explicit casting?

> + if ((rcount - fifo_bytes) <= 0) {

Why not positive conditional and drop those parentheses?

> + // last bytes are about to be read - end of transaction.
> + // Stop should be set before reading last byte.
> + NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ4);
> +
> + bus->state = I2C_STOP_PENDING;
> + bus->stop_ind = ind;
> +
> + npcm_i2c_eob_int(bus, true);
> + npcm_i2c_master_stop(bus);
> + npcm_i2c_read_from_fifo(bus, fifo_bytes);
> + } else {
> + NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ3);
> + npcm_i2c_read_from_fifo(bus, fifo_bytes);
> + rcount = bus->rd_size - bus->rd_ind;
> + npcm_i2c_set_fifo(bus, rcount, -1);
> + }

...

> +static void npcm_i2c_int_master_handler_read(struct npcm_i2c *bus)
> +{
> + u16 block_extra_bytes_size;
> + u8 data;
> +
> + // Master read operation (pure read or following a write operation).
> + NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ);
> +
> + // added bytes to the packet:
> + block_extra_bytes_size = (u8)bus->read_block_use + (u8)bus->PEC_use;

Why explicit castings?

> +
> + // Perform master read, distinguishing between last byte and the rest of
> + // the bytes. The last byte should be read when the clock is stopped
> + if (bus->rd_ind == 0) { //first byte handling:
> + // in block protocol first byte is the size
> + NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ1);
> + if (bus->read_block_use) {
> + // first byte in block protocol is the size:
> + data = npcm_i2c_rd_byte(bus);

> + // if slave returned illegal size. read up to 32 bytes.
> + if (data >= I2C_SMBUS_BLOCK_MAX)
> + data = I2C_SMBUS_BLOCK_MAX;

min() / min_t() ? See below...

> +
> + // is data is 0 -> not supported. read at least one byte
> + if (data == 0)
> + data = 1;

...actually clamp_val().

> + bus->rd_size = data + block_extra_bytes_size;
> +
> + bus->rd_buf[bus->rd_ind++] = data;
> +
> + // clear RX FIFO interrupt status:
> + if (bus->fifo_use) {
> + data = ioread8(bus->reg + NPCM_I2CFIF_CTS);
> + data = data | NPCM_I2CFIF_CTS_RXF_TXE;
> + iowrite8(data, bus->reg + NPCM_I2CFIF_CTS);
> + }

> + npcm_i2c_set_fifo(bus, (bus->rd_size - 1), -1);

Too many parentheses.

> + npcm_i2c_stall_after_start(bus, false);
> + } else {
> + npcm_i2c_clear_tx_fifo(bus);
> + npcm_i2c_master_fifo_read(bus);
> + }
> + } else {
> + if (bus->rd_size == block_extra_bytes_size &&
> + bus->read_block_use) {
> + bus->state = I2C_STOP_PENDING;
> + bus->stop_ind = I2C_BLOCK_BYTES_ERR_IND;
> + bus->cmd_err = -EIO;
> + npcm_i2c_eob_int(bus, true);
> + npcm_i2c_master_stop(bus);

> + npcm_i2c_read_from_fifo(bus,
> + npcm_i2c_get_fifo_fullness(bus)
> + );

Bad style. Combine last two lines together.

> + } else {
> + NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_READ2);
> + npcm_i2c_master_fifo_read(bus);
> + }
> + }
> +}

...

> +static irqreturn_t npcm_i2c_int_master_handler(struct npcm_i2c *bus)
> +{

> + npcm_i2c_eob_int(bus, false);

Extra spaces.
I have noticed more places with the same issue, fix 'em all.

> + val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK |
> + NPCM_I2CST_STASTR;

Make it temporary variable and use here and below...

> + val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK |
> + NPCM_I2CST_STASTR;

...here.

> + return ret;
> +}

Refactoring of such a big function will be benefit to all. And you may decrease
indentation level at the same time.

...

> + fif_cts = (fif_cts & ~NPCM_I2CFIF_CTS_SLVRSTR) |
> + NPCM_I2CFIF_CTS_CLR_FIFO;

fif_cts &= ~NPCM_I2CFIF_CTS_SLVRSTR;
fif_cts |= NPCM_I2CFIF_CTS_CLR_FIFO;

...

> + if (npcm_i2c_get_SDA(_adap) == 0) {
> + // Repeat the following sequence until SDA is released
> + do {
> + // Issue a single SCL cycle
> + iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST);

> + retries = 10;
> + while (retries != 0 &&
> + FIELD_GET(NPCM_I2CCST_TGSCL,
> + ioread8(bus->reg + NPCM_I2CCST))) {
> + udelay(20);
> + retries--;
> + }

timeout loops more natural in do {} while form. But here is
readx_poll_timeout() NIH.

> + // tgclk failed to toggle
> + if (retries == 0)
> + dev_err(bus->dev, "recovery toggle timeout");

If it's an error, why we are still continuing?

> + // If SDA line is inactive (high), stop
> + if (npcm_i2c_get_SDA(_adap))
> + done = true;

> + } while ((!done) && (--iter != 0));

Too many parentheses. done can be dropped (below you may use iter in condition).

> + // If SDA line is released: send start-addr-stop, to re-sync.
> + if (done) {
> + npcm_i2c_master_start(bus);
>
> + // Wait until START condition is sent, or RETRIES_NUM
> + retries = RETRIES_NUM;
> + while (retries && !npcm_i2c_is_master(bus)) {
> + udelay(20);
> + retries--;
> + }

do {} while ().

> + // If START condition was sent
> + if (retries > 0) {
> + // Send an address byte in write direction:
> + npcm_i2c_wr_byte(bus, bus->dest_addr);
> + udelay(200);
> + npcm_i2c_master_stop(bus);
> + udelay(200);
> + status = 0;
> + }
> + }
> + }

...

> + if (unlikely(npcm_i2c_get_SDA(_adap) == 0)) {
> + // Generate a START, to synchronize Master and Slave
> + npcm_i2c_master_start(bus);
> +
> + // Wait until START condition is sent, or RETRIES_NUM
> + retries = RETRIES_NUM;

> + while (retries && !npcm_i2c_is_master(bus))
> + retries--;

do {} while (). Fix them all.

> + // set SCL low for a long time (note: this is unlikely)
> + usleep_range(25000, 35000);
> + npcm_i2c_master_stop(bus);
> + udelay(200);
> + status = 0;
> + }

...

> +static bool npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq)
> +{
> + u32 k1 = 0;
> + u32 k2 = 0;
> + u8 dbnct = 0;
> + u32 sclfrq = 0;
> + u8 hldt = 7;
> + bool fast_mode = false;
> + u32 src_clk_freq; // in KHz

This is very cryptic.

The function deserve a good comment above which shows all formulas in use with
reference to datasheet pages, etc.

> + // Master or Slave with frequency > 25 MHZ
> + else if (src_clk_freq > 25000) {

> + hldt = (u8)__KERNEL_DIV_ROUND_UP(src_clk_freq * 300,
> + 1000000) + 7;
> +
> + k1 = __KERNEL_DIV_ROUND_UP(src_clk_freq * 1600,
> + 1000000);
> + k2 = __KERNEL_DIV_ROUND_UP(src_clk_freq * 900,
> + 1000000);

Why casting? Why __ special macro? Isn't DIV_ROUND_UP() enough?
Ditto for other similar cases.

> + k1 = round_up(k1, 2);
> + k2 = round_up(k2 + 1, 2);
> + if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
> + k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
> + return false;
> + }

> + // After clock parameters calculation update reg (ENABLE should be 0):
> + iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, sclfrq & 0x7F),

Magic number.

> + bus->reg + NPCM_I2CCTL2);
> +
> + // force to bank 0, set SCL and fast mode
> + iowrite8(FIELD_PREP(I2CCTL3_400K_MODE, fast_mode) |
> + FIELD_PREP(I2CCTL3_SCLFRQ8_7, (sclfrq >> 7) & 0x3),
> + bus->reg + NPCM_I2CCTL3);
> +
> + // Select Bank 0 to access NPCM_I2CCTL4/NPCM_I2CCTL5
> + npcm_i2c_select_bank(bus, I2C_BANK_0);

> + iowrite8((u8)k1 / 2, bus->reg + NPCM_I2CSCLLT);
> + iowrite8((u8)k2 / 2, bus->reg + NPCM_I2CSCLHT);

Why casting?

...

> + if (FIELD_GET(I2C_VER_FIFO_EN, ioread8(bus->reg + I2C_VER))) {
> + bus->fifo_use = true;
> + npcm_i2c_select_bank(bus, I2C_BANK_0);
> + val = ioread8(bus->reg + NPCM_I2CFIF_CTL);

> + iowrite8(val | NPCM_I2CFIF_CTL_FIFO_EN,
> + bus->reg + NPCM_I2CFIF_CTL);

Do it in two operations.

> + npcm_i2c_select_bank(bus, I2C_BANK_1);
> + } else {
> + bus->fifo_use = false;
> + }
> +
> + // Configure I2C module clock frequency
> + if (!npcm_i2c_init_clk(bus, bus_freq)) {
> + dev_err(bus->dev, "npcm_i2c_init_clk failed\n");
> + return false;
> + }
> +
> + // Enable module (before configuring CTL1)
> + npcm_i2c_enable(bus);
> + bus->state = I2C_IDLE;
> +
> + // Enable I2C int and New Address Match int source
> + val = ioread8(bus->reg + NPCM_I2CCTL1);

> + val = (val | NPCM_I2CCTL1_NMINTE) & ~NPCM_I2CCTL1_RWS;

Usually we mask first, then disjunct with new bits.
Actually this applies to all code where I also suggest to split to two
operations.

> + iowrite8(val, bus->reg + NPCM_I2CCTL1);

...

> + ret = of_property_read_u32(pdev->dev.of_node,
> + "bus-frequency", &clk_freq);

With ugly ifdefery this seems suspicious. Probably you imply
device_property_read_u32()?

> + if (ret < 0) {
> + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> + clk_freq = 100000;
> + }
> +
> + ret = npcm_i2c_init_module(bus, I2C_MASTER, clk_freq / 1000);

> + if (!ret) {

0 is error condition?!

> + dev_err(&pdev->dev, "npcm_i2c_init_module() failed\n");

> + return -1;

Use proper error coded.

> + }

...

> + bus->dest_addr = (u8)(slave_addr << 1);// Translate 7bit to 8bit format

Awful formatting (comments) and casting.

> + i2cfif_cts = (i2cfif_cts & ~NPCM_I2CFIF_CTS_SLVRSTR) |
> + NPCM_I2CFIF_CTS_CLR_FIFO;

Do it in two operations.

...

> + if (unlikely(bus->state == I2C_DISABLE)) {

Why unlikely() is in use? Any performance improvement? Show numbers.

> + dev_err(bus->dev, "I2C%d module is disabled", bus->num);
> + return -EINVAL;
> + }

...

> + time_left = jiffies +
> + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
> + do {
> + /* we must clear slave address immediately when the bus is not
> + * busy, so we spinlock it, but we don't keep the lock for the
> + * entire while since it is too long.
> + */

> + spin_lock_irqsave(&bus->lock, flags);
> + bus_busy = ioread8(bus->reg + NPCM_I2CCST) & NPCM_I2CCST_BB;
> + spin_unlock_irqrestore(&bus->lock, flags);

> + if (!bus_busy)
> + break;

This can be part of below condition.

> +
> + } while (time_is_after_jiffies(time_left));

...

> + if (!npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
> + write_data, read_data, read_PEC,
> + read_block))

> + ret = -(EBUSY);
> +
> + if (ret != -(EBUSY)) {

Too many parentheses. Fix them all in entire driver.

> + time_left = wait_for_completion_timeout(&bus->cmd_complete,
> + timeout);
> +
> + if (time_left == 0) {
> + NPCM_I2C_EVENT_LOG(NPCM_I2C_EVENT_TO);
> + if (bus->master_or_slave == I2C_MASTER) {

> + dev_dbg(bus->dev,
> + "i2c%d TO = %d\n", bus->num, timeout);

Why not first line fit enough?

> + i2c_recover_bus(adap);
> + bus->cmd_err = -EIO;
> + bus->state = I2C_IDLE;
> + }
> + }
> + }
> + ret = bus->cmd_err;

> + // If nothing went wrong, return number of messages x-ferred.
> + if (ret >= 0)
> + return num;
> +
> + return ret;

Why not traditional pattern, i.e.

if (ret < 0)
return ret;

?

> +}

...

> +static u32 npcm_i2c_functionality(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_EMUL |
> + I2C_FUNC_SMBUS_BLOCK_DATA |
> + I2C_FUNC_SMBUS_PEC
> + ;

Awful indentation.

> +}

...

> +static const struct i2c_adapter_quirks npcm_i2c_quirks = {
> + .max_read_len = 32768,
> + .max_write_len = 32768,
> + .max_num_msgs = 2,
> + .flags = I2C_AQ_COMB_WRITE_THEN_READ

Missed comma.

> +};

...

> +static int npcm_i2c_probe_bus(struct platform_device *pdev)
> +{

> +#ifdef CONFIG_OF

Why this ugly ifdefery?

> + num = of_alias_get_id(pdev->dev.of_node, "i2c");
> + bus->num = num;

> + i2c_clk = devm_clk_get(&pdev->dev, NULL);

Can't be optional?

> + if (IS_ERR(i2c_clk))

> + return -EPROBE_DEFER;

Why shadow an error code? Fix them all.

> + bus->apb_clk = clk_get_rate(i2c_clk);
> +#endif // CONFIG_OF

> + bus->irq = platform_get_irq(pdev, 0);
> + if (bus->irq < 0)

> + return -ENODEV;

Why shadowed error code?

> + ret = i2c_add_numbered_adapter(&bus->adap);

> + if (ret < 0)

What about positive? Do they ever happen?
Hint: drop ' < 0' part in each case you have in this driver where it's not
needed.

> + return ret;
> +
> + platform_set_drvdata(pdev, bus);
> +
> + return 0;
> +}

...

> +MODULE_VERSION("0.1.1");

Does it make any sense?

--
With Best Regards,
Andy Shevchenko